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>
Restore the documented SDK env credential facade without reintroducing run fallback behavior. Fail closed on GitHub permission resolution, require worker storage at the CLI boundary, and align interpolation names and generated docs.
Splitting on the first colon in FromStr broke bare model IDs that
legitimately contain one. A reference like "llama3:8b" parsed as
provider "llama3" selector "8b", and since "llama3" is not a provider
the lookup failed instead of passing the ID through to the pinned
provider. Verified against origin/main: canonical_session_model with
"future-model:latest" pinned to openrouter returned the passthrough
before and a 400 after.
This is not fixable by choosing a different separator. Bedrock
inference-profile ARNs contain both colons and slashes, and
docs/public/integrations/bedrock.mdx tells users to put arbitrary
inference-profile IDs in api_id. Only the registry can tell a provider
prefix from a model ID that happens to contain the separator.
FromStr now leaves colon-bearing tokens bare, and ModelRef::qualify
promotes only those whose prefix names a known provider. resolve()
applies it, so the fallback path is covered; sessions.rs applies it
before its own match so it keeps its tailored ambiguity messages.
ModelRegistry is now implemented for Catalog in fabro-types, replacing
the CatalogModelRegistry wrapper that existed only in start.rs, so both
call sites share one registry view.
Covered by regression tests at both surfaces, plus qualify unit tests
for ollama tags and Bedrock ARNs. The pre-existing passthrough test
canonical_session_model_preserves_unknown_passthrough_on_selected_provider
passes again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both from Copilot review feedback on #652.
Do not fall back to `base_sha` for `final_git_commit_sha`:
`base_sha` is where the run started, not what it produced. When a run made
commits but no SHA was tracked, the conclusion reported the base commit as the
run's final commit — a durable, API-exposed field — and publish then checked
the pushed branch against it, failing a branch that was pushed correctly.
The SHA is now only required where it is actually used: verifying the remote
head before opening a pull request. Pushing never needed it, since the refspec
sends whatever the branch points at. A run with no tracked SHA therefore still
pushes its branch and succeeds; it fails only if a pull request is requested,
where an unverifiable head is a real problem.
Route branch names with slashes in the GitHub twin:
Run branches are `fabro/run/<id>`. GitHub routes the branch as the remainder
of the path, but the twin declared a single-segment `{branch}` capture, so
every real run branch 404'd against it. Now a wildcard, with a test covering
the slashed case that the existing single-segment tests missed.
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>
Follow-up cleanup on the provider-qualified selector work.
- build_model_indexes now takes the paired (Model, CatalogModelSettings)
slice it is built from, instead of a separate settings map. This drops
a per-model map lookup with two cloned key components and removes the
expect() panic path for an invariant the caller already guarantees.
- get_on_provider expresses the exact-then-legacy lookup as one closure
applied twice, rather than a nested then/flatten chain.
- ModelRef::from_str selects the separator first and then checks both
sides once, so the empty-side check is no longer duplicated across two
branches and the slash split no longer allocates a Vec.
- Shorten the TooManySlashes message to the action the user should take.
- Merge the two near-identical fallback chain tests into one that runs
both qualified selector forms through the same assertion.
- The fallbacks splice test now asserts through the existing Serialize
impl instead of hand-rolling the ModelRefOrSplice rendering.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three follow-ups from the efficiency review of the publish pipeline.
Check the branch before spending an LLM call:
`open_pull_request` generated the PR title and body first and only then
verified the remote branch pointed at the run's final commit. Every stale
branch therefore cost a full content generation before failing. The
verification is the cheap check, so it now runs first.
Tolerate GitHub read-after-write lag:
`GET /repos/{owner}/{repo}/branches/{branch}` is replica-served and can briefly
report the previous commit, or 404 for a branch that is new on the remote,
right after the push publish just made. It was read once with no retry. Since
publish failures are terminal, a replica that had not caught up yet would
discard a fully successful run. It is now read up to three times.
These two land together on purpose: the LLM call was the only thing buying
slack against the race, so reordering without the retry would have made it
more likely.
Keep commit SHAs out of failure classification:
`classify_failure_reason` substring-matches bare "500", "502", "503" and "504"
as transient-infra hints. Both publish messages embed a commit SHA, and a
40-char hex string contains one of those often enough to matter, so a
deterministic failure could be reported as transient. Long hex runs are now
masked before matching; the three-digit status codes those hints look for are
too short to be affected. The hex regex is shared with
`normalize_failure_reason`, which already had its own copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up cleanup on the publish-failures change.
Error model:
- Collapse `Error::{Engine, Publish, Handler}` into one `Error::Stage` with an
`ErrorStage` discriminator. The three shared a field shape and had to be
edited together in four match groups; nine near-identical constructors
become two private helpers.
- Add `Error::failure_reason()`, replacing the same error -> FailureReason
mapping written out in four places.
- Publish errors are now terminal. Publish runs once, after execution, so no
caller could ever act on the retryable classification.
Publish phase:
- Fix: a branch that was pushed is now still reported when pull request
creation fails afterwards. `PublishOutcome` records what happened and
carries the error separately, instead of hiding both behind a `Result`.
- Drop `PublishOutcome::NoChanges`, which no consumer distinguished from
`Published { pr_url: None }`.
- Move publish onto `Concluded` as methods and replace three near-identical
precondition guards with one `publish_target()`.
Pull requests:
- `maybe_open_pull_request` -> `open_pull_request` returning the record
directly. Both callers already reject empty diffs, so the `Ok(None)` path
was unreachable.
- Drop `CreatedPullRequest.head_sha`, which echoed back its own input.
GitHub client:
- Delete `branch_exists`, which had no callers and duplicated
`branch_head_sha`. Give `branch_head_sha` the `_with_client` split every
sibling has and port the tests to `MockHttpClient`.
- Collapse the copy-pasted credential match in `resolve_clone_credentials`.
Events:
- `PullRequestCreated.head_sha` is `Option<String>` instead of using an empty
string to mean absent.
- Centralize the run-branch refspec in `lifecycle::push_run_branch`, so
`git.push` reports a branch name from both emitters as documented.
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>
Follow-up cleanup on the catalog-free validation split. Same behavior,
fewer parallel code paths.
- Make the catalog an explicit `Option<&Catalog>` on `pipeline::validate`
instead of a `validate` / `validate_with_catalog` pair, so each call
site states whether catalog rules run.
- Collapse `preprocess_and_validate`, `preprocess_and_validate_structural`,
and `preprocess` into one function that takes `TransformOptions`. Its
`model_resolution` field is now the single source of truth for catalog
awareness, which drops a 12-argument signature and the
`too_many_arguments` allow.
- Replace the duplicated resolve-and-preprocess block in
`operations::validate` with one `validate_in_scope` helper, and drop the
HashSet -> Vec -> HashSet round trip on the catalog path.
- Extract `configured_default_provider`, previously duplicated between
`operations::create` and `operations::validate`.
- Delete `validate_manifest_with_environment_defaults`, which had no
callers outside its own module.
- Share the `server-model.fabro` fixture between the two CLI tests instead
of inlining it twice. The validate test now asserts the rendered output
through the usual snapshot helper, which also removes a hand-rolled
`std::fs::write` and its clippy allow.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The process environment is no longer a configuration source. `{{ vars.NAME }}`
(non-sensitive, server-stored) and `{{ secrets.NAME }}` (vault-backed) cover
both cases, and reading the worker's ambient environment made a run's inputs
depend on how its process happened to be launched.
`Namespace::Env` is kept but wired to nothing, so `{{ env.NAME }}` still
parses and fails with a message naming its replacement rather than reaching
a consumer as literal text. `ResolveCtx::with_env` is gone, so no call site
can opt back in.
Two long-standing warts were env-only and go with it:
- `InterpString::resolve_or_source`, the "fall back to the raw template
source on failure" path, which let an unresolved token reach a sandbox or
the GitHub API as literal `{{ ... }}` text. Its own comment noted it was
slated for hard-error semantics.
- `RunEnvironmentSettings::resolve_env`'s matching source fallback for
env-only values.
Both carried `#[expect(clippy::disallowed_methods)]` escape hatches. Every
run-boundary resolver — sandbox env, prepare steps, MCP transports, GitHub
permissions, Slack channels, run goal files, provider extra_headers — now
fails closed instead.
Hooks lose their `allowed_env_vars` allowlist, `resolve_header`, and
`HeaderResolveError` along with the `E: Env` generic threaded through the
executor. They keep `{{ vars.* }}`, which `RunSettings::substitute_variables`
already substitutes server-side at run creation.
`allowed_env_vars` is removed from the OpenAPI spec and the generated
TypeScript client. The docs example showing `{{ env.* }}` in
`[server.slatedb.s3].bucket` was already wrong — that field is a plain
String and never interpolated — and is now a literal.
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>
Command node `script` attributes were literal text: a `{{ inputs.x }}`
reached bash verbatim, and the only signal was a `detemplated_attribute`
warning. Scripts now substitute `{{ goal }}`, `{{ inputs.NAME }}`, and
`{{ vars.NAME }}` at run creation, alongside goals and prompts.
Scripts use `InterpString` token substitution rather than the MiniJinja
pass that renders prompts. Shell source is full of brace syntax that must
survive untouched — jq filters, awk programs, Go templates, brace
expansion — and `InterpString` claims only the narrow token forms,
leaving everything else literal.
`env` and `secrets` are deliberately not wired and now fail loudly
instead of passing through as text. A script reads the environment with
`$NAME`, which needs no interpolation, and a resolved secret would be
baked into the `CommandStarted` event that records the script verbatim.
The error points at `[environments.<slug>.env]` for the secret case.
`ResolveCtx` gains opt-in `with_inputs` and `with_goal`. Namespace
availability stays scope-determined per call site, so every existing
config-layer context leaves both unwired and keeps its current behavior.
`goal` names a single value rather than a namespace of them, so it has
no dotted form: only the exact body `goal` produces a token and
`{{ goal.title }}` stays literal.
Values substitute verbatim without shell quoting, matching
`[[run.prepare.steps]].script` where the snippet is the author's to
quote. Substituted text is never rescanned.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The model indicator on a stage page hovered to provider, model, and
reasoning effort only. Seeing what a stage actually spent meant leaving
for the Billing tab, which reports per node rather than per visit.
The stage list had no token data to show, so add a per-visit `billing`
block to `GET /runs/{id}/stages`. The Billing tab's pricing rule (a
provider-reported cost wins, otherwise the server catalog prices the
tokens) was private to `billing_rollup`; move it to
`StageProjection::billed_usage` and drive both call sites from it so the
two views cannot drift.
The popover's buckets use the Billing tab's labels verbatim. It stays
scoped to one visit, so a looped node's row on the Billing tab is the sum
of what each of its visits shows here.
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>
The DOT parser created a node for every edge endpoint, and nothing
recorded whether a node came from a declaration or was synthesized from
an edge. The edge_target_exists rule only checked whether the node id
was present in the graph, which was always true by then, so a misspelled
endpoint became an attribute-free node that defaulted to shape=box — an
LLM stage. Validation emitted a prompt_on_llm_nodes warning and exited 0.
Node now carries `implicit`, set only when the parser synthesizes the
node from an edge endpoint. A declaration anywhere in the workflow
clears it, so order does not matter and subgraph declarations count.
Node::new leaves it false, so programmatic construction and graphs
deserialized from older checkpoints read as declared.
edge_target_exists treats an endpoint as valid only when it exists and
is declared, reporting each undeclared node once. The near-identical
missing-source and missing-target branches collapse into one path. The
import transform copies the flag onto spliced nodes so an edge-only node
inside an imported fragment is caught too.
parse_and_validate_human_gate had two edge-only nodes and now declares
them; it was an instance of the bug rather than a casualty of the fix.
No shipped workflow, docs example, or CLI fixture relied on the old
behavior.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ReadBeforeWriteSandbox` blocked writes to any existing file the agent
had not read, tracked by a session read set populated only by
`read_file`, `grep`, `read_many_files`, and the Kimi `Read`.
The gpt56 profile has none of those. It mirrors Codex's tool contract --
`shell_command`, `apply_patch`/`edit_file`, `update_plan`, `web_search`
-- and reads through the shell, so its read set stayed permanently
empty and every edit to an existing file failed. In run
01KYD4360GN6SED4BYEVGYP4XT all 28 `edit_file` calls failed, 25 of them
on the guard. The agent read `package.json` with `sed` and `cat`,
hex-dumped it trying to diagnose the rejections, then routed around the
guard with `sed -i`, which the guard never covered. It prevented no
blind write; it converted content-anchored edits into an unreviewed
in-place shell rewrite.
Neither Codex nor Kimi Code enforces read-before-write at runtime.
Codex's `apply_patch` `Add File` overwrites an existing path silently;
Kimi Code's `Write` has no check at all. Both rely on the exact-match
requirement in their edit tools, which is stronger proof of inspection
than a read set, plus per-write approval.
Tool descriptions and the Kimi prompt keep telling the model to read
before editing -- that guidance matches Kimi Code's own `edit.md` and
still prevents `old_string not found` -- but no longer claim the
workspace refuses unread writes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
