These were untracked working-tree files unrelated to for_each item
injection. They were swept in by an over-broad `git add` and do not
belong on this branch.
Co-Authored-By: Claude Opus 5 (1M context) <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>
The schema advertises defaults for `block` and `timeout`, but the
executor errored when either was absent. Apply the advertised defaults
instead, and keep the type check for values that are present.
The required list stays as the Claude 5 contract declares it. Constants
now hold the defaults and the maximum so the schema and the executor
cannot drift.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reduce duplication and over-specification introduced with the Modal
provider, without changing shipped behavior.
- Extract enabled_provider_catalog and assert_deep_tool_round_trip in
the fabro-llm integration tests. The Poolside, Fireworks, OpenRouter,
and Modal deep round trips were four near-identical copies.
- Add ApiCredential::with_extra_headers for providers that authenticate
with request headers instead of an API key.
- Replace the unreachable require_env guards in the Modal e2e test with
the std::env::var form used by every sibling test, and register
MODAL_TOKEN_ID and MODAL_TOKEN_SECRET in EnvVars.
- Collapse modal_requires_both_vault_proxy_tokens to a single case. The
loop rebuilt the whole built-in catalog per iteration.
- Drop tautological and over-specified assertions: the api_key_url doc
URL, the forced default/probe lookups on a single-model provider, and
the get_on_provider loop that could not fail.
- Inline the single-use modal_env_catalog fixture and note why it
overrides the shipped secrets templates.
- Sort the MODAL_* keys in .env.example, and record in modal.toml why
api_id keeps the Hugging Face capitalization.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`parallel_group_id` used `oneOf: [$ref StageId, null]`, and the generator
drops a sibling description in that position, so the TypeScript client
documented the field as "Canonical stage execution identifier in
`node_id@visit` form" — the shared StageId text, which says nothing about
what this field means. Switching to `allOf` lets the field's own
description through.
Dropping `type: "null"` also makes the contract match the server, which
omits both fields rather than sending null (`skip_serializing_if` on
`Option`, pinned by list_run_stages_exposes_parallel_branch_identity).
The Rust types are unchanged — still `Option<StageId>` and `Option<u32>`,
which accept an explicit null on input either way — so this only narrows
what clients are told to expect on the wire. Wording updated to match,
and reworded to avoid an apostrophe the generator escapes into the
JSDoc.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses Copilot review feedback on #668.
Checking for `:` before the legacy `/` form broke `provider/model`
references whose selector contains a colon, which Bedrock API IDs do:
`bedrock/us.anthropic.claude-haiku-4-5-20251001-v1:0` parsed as the
whole path up to the last colon, then `0`. On main it is a pin to
bedrock with the full ID as the selector.
Now whichever separator appears first decides. A `/` before any `:` is
the legacy pin and its selector may contain colons. Otherwise the token
stays bare and `qualify` promotes it only when the prefix names a
provider, so `openrouter:moonshotai/kimi-k3` still qualifies.
Adds the regression test Copilot asked for, covering both Bedrock-style
legacy input and the colon-before-slash case.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Conflicts were between this branch's parallel-branch identity work and
main's stage billing, review targets, and live stage timing.
- Stage fixtures: main added `billing` to each per-file `makeStage`; this
branch had hoisted one builder into `lib/test-utils`. Kept the hoisted
builder and gave it `billing: makeBilledTokenCounts()`, so both intents
hold and the field list stays in one place. `stage-sidebar.test.ts` also
builds raw `RunStage` wire payloads, so it keeps importing
`makeBilledTokenCounts` directly.
- Import lists (`run_projection.rs`, `fabro-api/src/lib.rs`,
`run_state.rs`, `stage_projection_round_trip.rs`): unioned both sides —
`ParallelBranchId` alongside `timing`, `ReviewTarget`,
`ReviewTargetKind`, `AttrValue`, `Node`, and
`StageToolBatchProjection`.
- `fabro-server` tests: git interleaved two unrelated new tests into one
body. Split them back into
`list_run_stages_exposes_parallel_branch_identity` and
`run_billing_includes_live_stage_timing_in_rows_and_totals`.
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.
Branch indexes are sparse. A branch queued behind max_parallel reserves
no stage identity until it acquires the semaphore, tokio task order is
not index order, and a branch cancelled while queued never reserves one
at all. Sizing the row list by `stagesByBranchIndex.size` treated an
entry count as a dense index range, so a running branch at index 2 with
nothing at 0 or 1 rendered as a single "pending" placeholder and the
running branch disappeared. Size from the highest index observed.
Also:
- Derive the Succeeded/Failed tiles from the rendered rows instead of the
completed-event rollup, so the tiles cannot contradict the list. This
drops the isComplete fork and both live counters.
- Show the fallback branch count in the Branches tile, which previously
read "-" above N rows in exactly the case the fallback exists for.
- BranchRow carries `label` and `stageId`; ChildRow owns the route it
links to. `id` had become a display label on one path and a raw node id
on the other, and a view model should not hold a URL.
- Drop `branchIndex`, which only ever served as the React key and always
equalled the array index.
- Cover the sparse-index and pending-placeholder paths, and derive the
rollup counts in `completedEvent` instead of passing contradictory ones.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Branches bypass the engine's stage.started/stage.completed lifecycle, so
no SWR key invalidated the stages list while a fork ran. The new live
branch rows stayed frozen at their first observed state until an
incidental refetch. Map parallel.* events to the stages list, run events,
and graph keys.
Also:
- Label branch rows with formatStageLabel so a re-entered branch renders
as `review_glm@2`, matching the sidebar and waterfall.
- Build branch rows in one pass and count live outcomes in one loop.
- Name ParallelBranchId in the OpenAPI spec and reuse fabro_types::
ParallelBranchId, replacing two copies of an inline string format.
- Hoist makeStage and textContent into lib/test-utils so widening Stage
cannot leave per-file fixtures stale (tests are excluded from
typecheck, so the two component-test copies had already gone stale).
- Query stat tiles by data-stat instead of an exact Tailwind class.
- Reuse append_scoped_stage_event's body via append_event_with_scope and
add test_branch_event instead of poking envelope fields.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
catalog_from_settings_rejects_duplicate_provider_api_ids was a copy of
catalog_from_settings_rejects_duplicate_model_aliases with the alias
declaration swapped for an api_id. Fold them into one table-driven test
so the shared invariant is stated once: canonical IDs, aliases, and API
IDs occupy a single identifier namespace per provider.
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>
