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>
Addresses review feedback on the fallback notice work.
A provider-only fallback such as `openrouter` needs the primary model's
catalog entry to find the closest capability match. When the primary is
itself a passthrough selector there is no entry, so every provider-only
candidate was skipped with "provider `X` has no compatible model" even
when that provider had plenty. Adds a `PrimaryNotInCatalog` notice that
names the missing primary instead of blaming the provider.
Also from review:
- `code()` was a wildcard fallthrough, which docs/internal/events-strategy.md
forbids for new variants. Now exhaustive.
- `NoConfiguredOffering` discarded the `providers` list that
`NoEligibleOffering` hands it. The notice now names the providers that do
offer the model.
- `ModelFallbackNotice::reference` was a rendered `String`; it is now the
`ModelRef` it came from, which also drops the per-candidate double
allocation the previous refactor introduced.
- `ResolvedStartLlm` unpacked and repacked `ResolvedFallbackChain`
field-for-field; it now holds it directly.
- Added `FallbackTarget: Display` as `provider:model`, replacing two
hand-written `"{}:{}"` format strings.
- `Catalog::select` still inlined the `require_provider` body.
- Emission moved to `ModelFallbackNotice::emit_all`, covered by a new test
proving notices reach the event stream with the right level, code, and
message. Nothing tested that hand-off before.
Documented in `resolve_fallback_chain` why an unknown provider stays a hard
error while an unconfigured one is skipped, and that an unqualified unknown
selector pins to the primary's provider.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The doc comment promised blank names were ignored, but the guard only
rejected the empty string. Stylesheet selectors match class names exactly,
so a padded name would sit in `classes` and match no rule.
No current caller can pass one: the parser splits on whitespace, and the
subgraph and import paths strip everything but alphanumerics and hyphens.
This makes the public contract on the shared type match what it claims.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up cleanup on the portable fallback chain work.
- Add `Catalog::require_provider` and `Catalog::provider_id`, replacing the
`catalog_provider_id` free function in `start.rs` and two copies of the same
`provider(..).ok_or_else(UnknownProvider)` block inside the catalog.
- Add `FallbackTarget::new` and use it for the six struct literals that each
stringified a provider and model by hand.
- Extract per-candidate resolution into `resolve_fallback_candidate`, returning
a `FallbackCandidate` that is either a target or the skip reason. This flattens
`resolve_fallback_chain` from four levels of nesting to one loop and splits the
qualified/unqualified model arms into separate match patterns.
- Drop the `seen` HashSet and its per-candidate key clones in favor of a
`contains` check on the chain being built; fallback chains hold a handful of
entries.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One row per separator rule, so a regression names the input that broke
instead of pointing at a combined fixture string. Also record why
`add_class` keeps insertion order: `fidelity` falls back to the first class
for the thread ID.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Node classes were built in two places. The parser split the `class`
attribute on commas and whitespace, but the import transform re-split the
raw attribute on commas only. A space-separated class on an import
placeholder became a single class name, so stylesheet rules did not match.
That included the `class="fast shared"` example in the imports docs.
- add `Node::add_class`, replacing the duplicate append helpers in
`SemanticState` and `ImportTransform`
- read `node.classes` in `placeholder_config` instead of re-parsing the raw
attribute, so class splitting happens in exactly one place
- name the separator rule `split_class_attr`, splitting on commas and then
whitespace so empty entries need no trimming
- drop the unused `Node::class` accessor that invited the re-parse
- keep the comma-compatibility note in the DOT attribute reference only
Co-Authored-By: Claude Opus 5 (1M context) <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 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>
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>
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>
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.
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>
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>
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>
The review question sentence was written in four places and the URL
safety rules in three. Collapse each to one definition.
- Add `ReviewTarget::question_text_with_link` as the single definition of
the question wording. `question_text()` and the Slack header both use
it, so a wording change is now one edit.
- Delete `ReviewTargetKind::noun()`. The enum already derives
`strum::Display` with the same snake_case output.
- Share one `review_target_line` helper between the console interviewer
and the CLI attach client, which held a byte-identical copy. Print only
the URL: `question.text` already carries the label and the noun.
- Trim the web-side check to the URL scheme, host, and credentials, which
are what a raw `href` can act on. Label length and control characters
cannot affect the DOM and stay server-side.
- Split validation from presentation in the web UI. `safeReviewTarget`
returns the target or null, and each caller picks its own fallback, so
an unsafe target now falls back to the same Markdown rendering as a
question with no target.
- Derive the resource noun from `kind` in the web UI instead of
hardcoding "document".
- Use `ReviewTargetKind.DOCUMENT` and the shared `isRecord` guard when
parsing events, instead of a raw string and a hand-rolled object check
that accepted arrays.
- Drop `deny_unknown_fields` from the wire struct. The OpenAPI schema
leaves `additionalProperties` permissive, so an added field would
otherwise make persisted events unreadable.
- Import `ReviewTarget` by name, and stop naming Slack in a fabro-types
error message.
- Document that `review_target=true` replaces the gate's `label`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
