Finish the blob-hash vocabulary unification at the defining signatures:
RunStoreBackend::read_blob, RunStoreHandle, LocalRunStoreBackend, the
HTTP backend impl, RunDatabase::read_blob, and BlobStore::read/exists
all said `id`, which kept re-teaching the old vocabulary at every impl
site and inlay hint.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Move the static-reference vocabulary out of fabro-workflow so every
consumer shares one definition: ReferenceKind, AttributeScope, and
reference_kind_for_attribute land in fabro-types::graph, and
validate_static_reference plus a new visit_graph_references walker land
in fabro-template. The manifest bundler drops its ad-hoc graph scan and
walks references through the shared walker.
Unifying the walkers forces three semantic alignments, each matching
what the engine actually executes rather than what the old scanners
happened to match:
- stack.child_dotfile is no longer classified as a child-workflow
reference; the engine never resolved it as one.
- import and stack.child_workflow only count at node scope; graph- and
edge-level occurrences were scanned but never executed.
- @@-escaped goals flow through the shared walker's escape handling
instead of the bundler's own prefix stripping.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
One conflict, in StructuredOutputError::repair_message. main (#709)
added a `previous_error` parameter and richer validation-error
rendering; this branch had replaced the inline expectation match with
OutputSchemaKind::expectation().
Resolved by keeping both: main's new signature and section assembly,
calling schema.expectation() for the expectation text. The method
already supersedes main's inline match and carries this branch's intent
of embedding the resolved JSON Schema instead of naming it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both contract helpers dispatched on OutputSchemaKind, so they belong on
the type. Moves expectation() and agent_prompt() into an impl block and
drops the free functions.
Splits the combined agent test: assertions no longer run inside the
backend's run(), where a failure surfaces as a panic from execute().
Adds coverage for the Routing branch of the contract, which was
previously untested.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolve the model catalog table conflict in docs/public/core-concepts/models.mdx
by keeping both changes: this branch's `kimi` -> `moonshot` provider rename for
the Kimi rows, and main's new DeepSeek V4 rows.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses Copilot review feedback on the repeated-failure check.
serde_json runs with preserve_order, and jsonschema builds the
additionalProperties `unexpected` list by walking the instance in
document order. So the same leftover keys emitted in a different order
produced a different Vec and compared as a different problem, which
suppressed the "unchanged from your previous repair" nudge.
Sorting at capture also makes the MAX_UNEXPECTED_PROPERTIES truncation
pick the same subset every time instead of an order-dependent one, and
stabilizes the rendered message.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up review of the repair-error work. Behavior is the same or better;
the machinery is smaller.
Fixes a false "unchanged from your previous repair" nudge. same_problem_as
fell through to `_ => true`, so any two non-Required issues at the same
instance path, schema path and keyword compared equal. A model that removed
one unexpected property and added another was told it had changed nothing.
SchemaValidationIssue already derives PartialEq, so the 17-line comparison
is now `previous.contains(issue)`.
Drops the hand-written Type and Enum rendering. jsonschema already renders
both, and its messages name the offending value, which the hand-written
ones did not. Also switches masked() back to to_string(): masking replaced
the bad value with a placeholder, working against the goal of an actionable
message, and buys no privacy since the full response is already in the
prompt.
Resolves the schema fragment when the issue is captured rather than
threading Option<&OutputSchemaKind> through rendering. That reverts the
command.rs change and drops the test-only messages() shim. The fragment is
now attached only to Other, where it adds information; for required, type,
enum and additionalProperties it just repeated the prose.
Also: caps the model-controlled unexpected-property list so a wide object
cannot turn the repair prompt into megabytes; drops evaluation_path, which
was dead except under $ref, where it printed a pointer that does not
resolve; drops the keyword field, already named by the schema path; and
records the previous error only after the agent session accepted the
repair, since failover rebuilds the session from the original prompt.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot review flagged two backward-compatibility breaks with events
persisted by pre-model-keyed releases; both are stored data that can
never be rewritten, so accept the old shapes on read:
- FailoverProps: original_provider/original_model/attempt are Option
again with serde defaults. New events always set them; failover events
recorded before model-keyed fallbacks lack them. Restores the
historical-event test.
- RunModelSettings: temporary custom deserializer accepts the legacy
flat-array fallbacks shape inside stored run.created events, keying
the chain under the requested model name when one is set. Remove once
pre-0.311 run logs are out of the support window.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot review flagged that a failover event's from route can be a
candidate that failed during activation and never served traffic. That
is intentional — events chain (one event's to is the next one's from)
so the stream records every candidate tried, with the error explaining
why each was abandoned. Document it at the emit site.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Consolidation pass over the fallback feature, no intended behavior
changes beyond noted validation and event-shape cleanups:
- Unify the two parallel notice types: FallbackPlanNotice is gone;
ModelFallbackNotice now owns the runtime NoNearbyReasoningLevel case
and the shared ChainEmpty wording. Notices emit through a new
Emitter::notice_scoped with their own level, and each distinct notice
is emitted once per run instead of on every LLM call.
- Move canonical_model_id onto Catalog so chain keys are written and
read through one function; reject provider-qualified fallback keys,
which could never match at dispatch and were silently dead config.
- Type FallbackTarget as ProviderId/ModelId, removing repeated
ProviderId::new re-wrapping at every use site.
- Derive FallbackPlan's current route from a position index instead of
storing current/requested_controls copies; advance() no longer has
unreachable None branches.
- Bundle the agent invocation's live state (session, bridge, lease,
forwarder, accounting) into LiveAgentInvocation; failover_agent_session
drops from 21 parameters to 7 and the six copies of the
abort/discard/classify teardown collapse into two methods.
- Share one route_request builder between one_shot and its failover
loop; complete_one_shot_request takes the request by value instead of
deep-cloning the message payload per call.
- Event::Failover carries FailoverProps directly; the props' original
route and attempt fields are now required, and reasoning efforts are
typed ReasoningEffort instead of strings.
- Reuse RunModelSettings/RunModelControls in fabro-api via
with_replacement, add the missing controls property to the OpenAPI
schema, regenerate the TS client, and add the type-identity/JSON
parity test.
- Smaller cleanups: ReasoningEffort::closest_supported uses enum
discriminants; ModelFallbackPolicy gains len(); resolve_model_fallbacks
takes a provider slice; duplicate-target filtering lives only in the
resolver.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 10 MiB cap on resolved stdin_source values is tight for wide
fan-in: a context.parallel.results batch from a large for_each round
carries tens of structured agent outputs, and a merge step that feeds
them to a deterministic command hits the ceiling as a hard
deterministic failure. Raise the ceiling to 30 MiB; it still bounds
peak memory and remote uploads, just with headroom matched to the
fan-out sizes for_each already allows.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Workflow agent sessions run at PermissionLevel::Full with the whole
tool registry exposed, so blocking pre_tool_use hooks are the only
policy boundary they have. The child-session factory built for
spawn_agent dropped tool_hooks from the child's SessionOptions, so a
subagent's tool calls never reached the run's hooks: any agent that
could spawn a subagent got an unguarded read-write-shell escape from
every hook-enforced policy.
Clone the parent's tool_hooks into the factory, the same way the
permission level is already carried, so child sessions inherit the
parent's hook boundary. The new end-to-end test drives the real
create_session path against a scripted mock provider: the parent
spawns a child, the child executes read_file, and the hooks must see
both the parent's spawn_agent and the child's read_file.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CommandHandler also serves type="tool" nodes, so the runtime message now
says "Node '...'" to match the stdin_source_valid lint wording.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- ExecStreamingRequest: drop #[non_exhaustive] and the six Option-taking
builder setters; call sites use struct literals over ::new(), matching
GrepOptions/WalkOptions, and providers can destructure exhaustively
- Docker: pass ExecStreamingRequest through docker_exec_shell_streaming
instead of seven positional args; revert the no-op StartExecOptions
- Daytona: stdin temp-file cleanup is now best-effort (mirrors
DaytonaSession::close) so a failed delete cannot fail a completed
command or double-delete from Drop; upload overlaps session creation;
one shared DAYTONA_CLEANUP_TIMEOUT
- write_process_stdin tolerates ConnectionReset/ConnectionAborted so a
command that stops reading stdin does not fail on TCP Docker daemons
- Local sandbox aborts the stdin writer after process exit instead of
joining unbounded
- Cap stdin_source payloads at 10 MiB, mirroring the for_each bound
- Add Node::context_key_attr() tri-state so the handler and lint rule
share one definition of a valid context-key attribute
- inert_attribute canonicalizes handler types via StageHandler, fixing
false warnings for command attrs on tool nodes
- Share resolve_flat_context_value between command stdin and for_each;
resolve_json_value takes Value by value, removing a deep clone
- Reuse MockSandbox in command handler stdin tests instead of extending
SpySandbox with a hand-rolled streaming override
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses a Copilot review comment on #653.
The source array is runtime data, usually produced by a model, so its
length is not something a workflow author reviewed. Two changes, so an
over-long array degrades into a clear error rather than memory pressure.
Cap the item count at 1000. Above that the stage fails deterministically
before `parallel.started`, alongside the other for_each contract
violations, and the message says how to reduce the array.
Fork the parent context inside the branch task, after it acquires a
`max_parallel` slot, instead of at dispatch time. Live context copies now
track `max_parallel` rather than item count. Only the branch's own
preamble entry is moved into the task, so the shared stash is not cloned
per branch either.
The reviewer also suggested replacing spawn-all with `max_parallel`
workers pulling from a queue. Not done here: with the fork deferred, a
pending task holds little beyond its item, and reshaping the dispatch
loop would change cancellation and scope-reservation ordering, which
deserves its own review.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses three Copilot review comments on #653.
`item_label` comes from a model or a workflow author, and it reaches the
terminal through the CLI progress display. A label could carry ANSI
escapes, newlines, or bidi overrides and rewrite what the operator sees.
It could also be whitespace-only, giving a branch a blank identity.
Add `text::sanitize_display_label`: strip ANSI sequences, drop control
and bidi-reordering characters, trim, and elide past 80 characters.
Return an empty string when nothing printable survives so callers fall
back to an identity they control.
Apply it where the label is created, so events, the store, and the web
UI all get a clean value instead of each consumer having to remember.
`parallel_branch_display` sanitizes again, because a run recorded before
this commit still has raw labels in its event log.
`emit_branch_retrying` now sets `stage.retrying`'s `index` from the
branch stage's execution ordinal, matching the envelope `stage_id` and
the meaning every other emitter gives that field. The branch's position
in the fan-out is already on `parallel.branch.started`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
build_branch_plan read the runtime array before run_branches looked at
`simulated`, so every for_each workflow failed under --dry-run with
"for_each source '...' was not found in workflow context". Nothing had
populated the key yet: upstream LLM nodes take Handler::simulate, which
returns no context updates.
A dry run now stands in one placeholder item when the source is absent or
unusable, and simulates the template target once. Graph-shape mistakes
still fail, since catching those is the point of a dry run.
Also from review:
- ITEM_FENCE_PREFIX replaces the bare "untrusted-" literal that
render_item_data and its test each spelled out.
- ItemRecordingHandler no longer guesses an item label by substring
search. Nothing asserted it, and the third item's label "2" matched
stray hex from the random fence tag about two thirds of the time.
- The twin test reads keys::PARALLEL_RESULTS instead of the raw string.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Simplification pass over the for_each branch. No behavior change.
Share what was duplicated:
- Node::prompt_or_label replaces the "prompt, else label" fallback that
agent, prompt, and the for_each item injector each wrote out.
- context::lookup_flat replaces the "exact key, then strip context."
lookup that condition.rs had twice and the for_each source had again.
- is_llm_handler_type replaces the inline agent/prompt match, so the
runtime and the for_each_contract rule agree by construction.
- find_join_node now takes branch ids, so a for_each fan-out passes its
template target instead of needing find_join_for_target.
- collect_events moves to test_support; parallel and integration tests
shared one copy already.
- One ScriptedHandler replaces four test handlers that differed only in
what they returned.
Straighten the branch retry loop:
- Reserve the branch scope once before the loop instead of guarding it
with an Option, which removes three expect() calls.
- acquire_branch_permit and backoff_or_cancel replace the cancel-aware
select! blocks the loop repeated verbatim.
- Keep the match arms in Executor::execute_with_retry order so the two
loops stay easy to compare.
Drop redundant state:
- BranchPlan::is_for_each derives from template_target_id.
- for_each_contract checks node type before source shape, so one
mistake reports one diagnostic.
Prove the new wire fields survive the OpenAPI boundary: the fabro-api
round-trip fixture now carries index and item_label.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Conflict in apps/fabro-web/app/components/interview-dock.tsx. Main moved
the dock onto the shared collapsible `RunDockShell` and replaced the
local button constants with shared ones.
Kept main's structure whole and re-applied the review target rendering
onto it: the question paragraph in the shell's `body` becomes the linked
`ReviewTargetQuestion` when the target passes `safeReviewTarget`, and
plain text otherwise. Both now share main's paragraph classes through
`QUESTION_TEXT`, so the two renderings stay visually identical.
`peek` keeps using `question.text`, which is the correct plain-text
collapsed summary for a review target question.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replace the include_nested flag and its two wrapper functions with a
single outermost-only scanner. Routing extraction used the nested scan
and reverse iteration, so a routing object nested inside a wrapper could
win over its parent -- the same bug class this branch fixes for custom
schemas. No caller needs nested candidates.
Custom schema validation now walks candidates from the end and takes the
last one that parses, instead of parsing only the final candidate. Prose
after the object can contain braces, and outermost-only scanning made
that trailing text a candidate that shadowed the real JSON. Schema errors
are still reported from the last parsable object, so an earlier object
that happens to validate cannot mask a later violation.
Add direct scanner coverage for nesting, adjacent objects, unclosed
braces, and braces inside strings.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Take `Arc<Catalog>` by value again through the validation entry points.
`AppState::catalog()` returns an owned `Arc`, so `&state.catalog()` was
cloning, borrowing the temporary, then cloning again at the leaf. Every
consumer ends up owning the `Arc`, so by-value is the honest shape and it
drops one clone per call. The one caller holding the catalog in a field
now says `Arc::clone(&self.catalog)` explicitly.
- Correct the `RenderMode` doc comment. It claimed `Strict` is "used by
run-create", but run-create renders `Structural` and promotes the
resulting warnings to errors itself; `Strict` has no production caller.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ModelResolutionOptions` was a field-for-field duplicate of the existing
public `ModelResolutionTransform`, down to a verbatim copy of its `new()`.
`pipeline::transform` then unpacked one to rebuild the other, cloning the
catalog Arc and the eligible-provider set on the way.
- Delete `ModelResolutionOptions`. `TransformOptions.model_resolution` now
holds an `Option<ModelResolutionTransform>` directly, so the TRANSFORM
step is `resolution.apply(graph)?` with no rebuild and no clones. This
is consistent with `custom_transforms`, which already holds transforms.
- Add `ModelResolutionTransform::catalog()` so the VALIDATE step can reach
the same catalog for its lint rules. That is the only new code needed.
- Drop `CatalogScope` from `operations::validate`, which was a third copy
of the same fields. The three entry points now hand a partially built
transform to `validate_resolving_models`, which completes it with the
workflow's default provider once the workflow is resolved.
- Extract `validate_child_workflow` in `manager_loop`, collapsing two
near-identical validate-and-unwrap blocks.
- Point the transform tests at their own `transform_options()` helper via
struct-update syntax instead of respelling all seven fields, and drop a
HashSet -> Vec -> HashSet round trip from the create test helper.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`EnvCredentialSource` resolved provider credentials from the process
environment. It had no production entry point of its own — it was only
ever reached as the `None` arm of an `Option<Vault>` in three places:
`build_llm_source`, `configured_providers_for_start`, and
`configured_providers_from_process_env`.
That optional vault is not a state the product can be in. Every run has a
server behind it, the server always spawns workers with `--storage-dir`
(`worker_runtime.rs`), and `SqlVaultCredentialSource` backs both the
server and the CLI. So the fallback only served to silently degrade
credential resolution to whatever the worker process happened to have in
its environment.
Make the vault required across the run path — `RunOptions`,
`StartServices`, `build_llm_source`, `tool_secrets_from_configured_sources`,
`vault_token_lookup`, and the CLI GitHub helpers — so the invariant is
enforced by types rather than assumed. A worker spawned without
`--storage-dir` now fails with a clear message instead of quietly
continuing without a vault.
`configured_providers_from_process_env` had no callers at all and is
deleted. `AgentApiBackend::new_from_env` was public but only ever called
from its own tests; it is deleted too.
Test-only credential sources move to a feature-gated
`fabro_auth::test_support`, wired through dev-dependencies so they never
link into production builds. The CLI worker tests now pass
`--storage-dir`, matching what the server actually does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A node with no `shape` defaulted to `box`, which resolves to the agent
handler. That made a shapeless `script` node run as an LLM call prompted
with its own label, while the `script` was reported as inert — wrong
behavior behind a warning.
`script` is read by the command handler and by nothing else, so a
shapeless node that sets it is unambiguously a command node. `shape()`
now infers `parallelogram` in that case. An explicit `shape` still wins.
Two rules keep the inference honest:
- `script_prompt_conflict` — setting both `script` and `prompt` is an
error. No handler reads both. It fires regardless of shape so that
adding one cannot downgrade the error to a warning.
- `command_requires_script` — a command node without a script is an
error. Without this the original trap just moves: a node meant as a
command that omits its script silently becomes an agent again.
Also drops the `tool_command` alias in favor of `script` alone, routing
the six read sites through a new `Node::script()` accessor.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
