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>
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>
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>
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>
PR #651 stopped `fabro validate` and `fabro create` from judging model and
provider availability locally, but left the `fabro_run_create` tool path
doing exactly that. Both of its callers build a *client-side* catalog and
then POST the manifest to the server, so an agent naming a server-owned
model got `Model selection failed: unknown model provider '...'` while the
same workflow succeeded through the CLI.
- `build_run_tool_manifest` now validates structurally, matching the CLI.
It no longer takes a catalog at all.
- The MCP builder drops its `load_llm_catalog_settings` +
`Catalog::from_builtin_with_overrides` pair, and `WorkerRunManifestBuilder`
drops its catalog field, becoming a unit struct.
- `validate_manifest_with_catalog` had no callers left, so it is gone.
`validate_manifest` documents why every remaining caller is catalog-free.
The new test fails with the pre-fix client-side check, reproducing the
reported error exactly.
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>
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 test helper set server.storage.root directly, leaving the derived
local object-store roots (artifacts, slatedb) pointing at the real
~/.fabro/storage. Route the redirect through
ServerSettings::with_storage_override so every derived root moves to the
test directory together.
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>
Test settings usually omit `[server.storage] root`, so it resolved to the
production default. Handlers that walk that tree read whatever the machine
happened to have.
That is why all_spec_routes_are_routable was slow. Timing every request in
it showed 91% of the runtime in two routes:
6304ms GET /api/v1/system/resources
4574ms GET /api/v1/system/df
583ms POST /api/v1/system/prune/runs
...
the remaining 134 operations: 8ms combined
Both size Fabro-managed storage. On this machine that meant 193MB and 90,795
entries under scratch/, so the test's duration tracked how long the developer
had been running Fabro locally. Run-creating tests were writing there too.
Redirect settings that still carry the production default to a `storage`
directory beside the test vault, alongside the existing `server.env` and
`settings.toml` siblings. A test that chose its own root keeps it.
all_spec_routes_are_routable drops from ~15s to 0.6s, and the full workspace
run from ~33s to ~21s. All 7402 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Test settings usually omit `[server.storage] root`, so it resolved to the
production default. Handlers that walk that tree read whatever the machine
happened to have.
That is why all_spec_routes_are_routable was slow. Timing every request in
it showed 91% of the runtime in two routes:
6304ms GET /api/v1/system/resources
4574ms GET /api/v1/system/df
583ms POST /api/v1/system/prune/runs
...
the remaining 134 operations: 8ms combined
Both size Fabro-managed storage. On this machine that meant 193MB and 90,795
entries under scratch/, so the test's duration tracked how long the developer
had been running Fabro locally. Run-creating tests were writing there too.
Redirect settings that still carry the production default to a `storage`
directory beside the test vault, alongside the existing `server.env` and
`settings.toml` siblings. A test that chose its own root keeps it.
all_spec_routes_are_routable drops from ~15s to 0.6s, and the full workspace
run from ~33s to ~21s. All 7402 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The prompt used hand-rolled `{placeholder}` substitution via `str::replace`.
The app already has a MiniJinja layer for exactly this, and every other
checked-in prompt uses it, so use it here too.
`prompts/run_title.md` becomes `prompts/run_title.md.j2` with `{{ inputs.* }}`
variables, rendered through `fabro_template::render_named`. Strict undefined
handling now catches a variable the template asks for and the caller does not
supply, which the old `.replace()` chain silently left as literal text.
`build_title_prompt` returns `Result` accordingly. A checked-in template that
will not render is a bug rather than a transient failure, so the caller logs
it at `warn` — louder than the `debug` used for a generation miss — and keeps
the deterministic title.
Re-checked against claude-haiku-4-5: same titles as before the change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Move the run title prompt into `src/prompts/run_title.md` and load it with
`include_str!`, matching the playground and ask-fabro prompts. Placeholder
substitution replaces `format!`, so the literal `{"title":"..."}` in the
prompt no longer needs brace escaping.
The old instructions only said "concise" and "preserve ticket IDs", so a
work order run titled itself with the raw file path. The prompt now asks for
a pull-request-shaped title: leading verb, identifier in canonical uppercase,
then a description with paths, date prefixes, and extensions stripped and
slug hyphens turned back into words. Three worked examples carry the shape.
Checked against claude-haiku-4-5 at the existing 64-token budget:
Implement Conveyor Work Order docs/planning/orders/2026-07-22-wrk-004-operational-diagnostics.md
-> Implement WRK-004: Operational diagnostics
Fix flaky checkout test (input branch release-9.2)
-> Fix flaky checkout test on release-9.2
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>
Diagnostics have carried a `fix` field all along, but the CLI renderer
never printed it — the suggestion was only reachable through --json. The
actionable half of every validation failure was invisible to the person
running the command.
print_diagnostics now emits the fix as a dim-labelled continuation line
under any diagnostic that has one, at both error and warning severity.
Gating it behind --verbose would defeat the point, and printing it only
for errors would read as "this warning has no fix" — the warning
suggestions are useful on their own. Diagnostics that set no fix simply
omit the line.
The severity match moved into print_diagnostic so the fix line is
appended once in the loop rather than copied into all five arms; the
rest of the diff is reindentation.
print_diagnostics is shared by validate, preflight, graph, exec, and
dry-run, so this covers all five. Eleven inline snapshots across four
files gain a fix line; every change is additive.
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>
