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>
`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 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 interview question panel could take half the viewport, and the page
reserved a fixed 18rem beneath it regardless of how tall it actually was,
so a long question covered the stage rows it was asking about.
Add a shared `RunDockShell` for the two controls docked at the bottom of
the run detail route. It is three zones: a header that is always visible
and doubles as the collapsed bar, a body that scrolls, and actions that
stay pinned so the controls needed to answer or send never scroll out of
reach.
The interview dock drops the question-type subtitle the answer buttons
already state, turns the 160px context box into a closed disclosure with
a first-line preview, drops the "or" divider row, and reveals the
keyboard hint on focus inside the composer row instead of standing below
it. Options stack into a list once a label is too long to sit in a pill.
For the sample question this is 506px down to 325px, or 43px collapsed.
The steering dock gains the same header. `Interrupt` moves into it,
because it acts on the run rather than on the message being composed, and
the waiting notice folds into the header status instead of adding a row.
Both docks now share one composer.
Clearance is measured from the rendered dock rather than assumed. The
former constants remain as the pre-measurement first frame.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test needed room only because it was walking the developer's real
~/.fabro/storage. Now that it runs in about a second, the package-wide
fabro-server timeout covers it with plenty of margin.
The override was not doing anything anyway: nextest resolves each setting
from the first matching override, and `package(fabro-server)` was defined
above it, so the narrower filter never applied. Removing it makes the
config say what was already true.
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 artifacts page grouped captures by stage, which is the storage key
`(stage, retry, path)` rather than anything a reader thinks in. A file
rewritten by four stages appeared as four separate rows under four
headings, with no indication they were the same file.
Group by path instead. Each file is one row showing its latest capture;
earlier captures disclose inline behind a chevron with the producing
stage, size, and the byte change that capture introduced.
Three fixes fall out of the regrouping:
- Order versions by the producing stage's `startedAt`. The previous sort
was alphabetical by stage label, which scrambled history — a report
that grew 8.42 KB -> 13.16 -> 14.32 -> 17.48 rendered newest-first
under a heading implying it was the earliest.
- Drop captures from graph control nodes (`start`, `exit`) via the
existing `isVisibleStage` helper. Those nodes run no work, so the
files they match are pre-existing workspace files swept up by the
capture globs, not run output. This is display-side only; the capture
path still stores them.
- Show the retry badge at `retry > 1` rather than `retry > 0`. Attempts
are 1-based, so the old condition matched every capture and rendered
a "retry 1" badge on every group.
Grouping lives in a separate module so it is testable without React.
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>
The board cards showed wall-clock duration in the footer's bottom-right
corner. Replace it with the same SizeChip the list view and run detail
header use, so the cost signal is consistent across all three views.
The chip inherits the tooltip, which names the tier and adds the cost
once a run has terminal billing.
Add SizeChip tests pinning the tooltip label for each tier.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Size column in the runs list rendered SizeChip without the billed
total, so its tooltip read "Size M" while the run detail header showed
"Size M · $12.34 billed".
The tooltip was also unreachable: the row title link paints a
`before:absolute before:inset-0` overlay across the whole row, which sat
above the chip and swallowed hover. Wrapping the chip in `relative z-10`
lifts it above that overlay, matching how the created-by and pull request
cells already handle interactive content.
Runs without terminal billing keep the plain "Size M" label, same as the
header.
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>
The check tested for `never reconstruct it from memory`, a phrase the
Kimi edit description no longer contains after it was reworded to match
Kimi Code. A one-sided `!contains` against a literal cannot tell "the
phrase is absent because nothing leaked" from "the phrase is absent
everywhere", so it silently stopped protecting anything.
Assert the marker is present in Kimi's own description and absent from
the stock one. Removing the marker from the description now fails the
test instead of quietly disarming it, verified by doing exactly that.
Also correct the grep docs: all three sandbox implementations probe for
`rg` and fall back to POSIX `grep`, so the page should not imply a
single engine. Pre-existing, adjacent to the lines this branch touched.
Reported by Copilot review on #646.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Neither harness puts read-before-edit mechanics in the system prompt.
Kimi Code's `system.md` has no such section; the rules live in
`edit.md`, `write.md`, and `read.md`. Codex's prompts say nothing about
reading before an edit at all, and its editing guidance is attached to
`apply_patch`.
Drop the `# Reading Before Writing` section from the Kimi prompt and
carry its content in the Edit, Write, and Read descriptions, worded as
Kimi Code words it. Nothing is lost: every bullet in the removed
section was already covered by a tool description.
Two behaviors change to match upstream. Edit now says not to issue
consecutive edits against the same file, since the first invalidates
the second's `old_string` -- Kimi Code's stated reason. Read now says
not to re-read solely to confirm a write landed, which both harnesses
call out as waste; the previous prompt asked for exactly that re-read.
The gpt56 profile already followed the Codex split and is unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`active_time_ms` was only ever computed from terminal stage events, so a
stage still running contributed zero to the run rollup. A run parked in one
long agent stage reported 2m 8s of active time against 16m 53s of wall
clock — the two finished stages — while the running stage had been doing
continuous inference and tool work for over 14 minutes.
`live_run_timing` summed `filter_map(|stage| stage.timing)`, and
`stage.timing` is only written at finalization. Wall time ticked live off
`start_time`; active time did not tick at all.
Stage projections now accumulate brackets from the event log:
- Closing an inference bracket folds its span into `live_inference_ms`
instead of discarding it, including across retries, matching the
in-process stopwatch.
- Tool calls open a batch on the first outstanding call and close it when
the last one drains, so tools running concurrently within a turn count
once — the same span `execute_tool_calls` is bracketed by. Summing
per-call durations would over-count parallel tool use. Subagent tool
events are excluded; they run inside the root call's span already.
- `StageProjection::live_timing(now)` composes accumulators with any open
bracket, per handler: agent stages use the brackets, prompt and command
stages count elapsed time as inference and tool respectively, and
handlers that wait on a human, timer, condition, or child branches
report zero.
Active is clamped to wall per stage. A worker killed mid-turn leaves its
bracket open forever, and without the clamp it would tick up unbounded.
The clamp does not need to detect the dead worker: a stage cannot have been
active longer than it has existed. `watchdog.timeout` remains the authority
on whether a run is stuck. The clamp is deliberately not applied at run
level, where concurrent branches can legitimately sum past run wall time.
Timing is derived from events rather than emitted by the worker, so this
needs no event-schema change and applies to runs already stored.
`StageProjection.timing` keeps its terminal-only meaning, and the
authoritative breakdown still replaces the live estimate at terminal
events.
The billing endpoint had the same hole behind its `wall_only` fallback:
running stages reported zero inference/tool/active. Not visible in the
product, which renders only `wall_time_ms`, but wrong for any other
consumer of `GET /runs/{id}/billing`.
Parallel branch stages lose their breakdown permanently, even after
completion, because `parallel.branch.completed` carries only `duration_ms`.
That is a separate data-loss bug, tracked in #644.
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>
A tab left open across a deploy keeps running the previous build's
JavaScript indefinitely. index.html is fetched only on a full page load,
all later navigation is client-side, and hashed bundles are served
`immutable`, so nothing reveals that the code is stale. This produced a
false-positive bug report where two correctly-deployed fixes appeared to
be missing.
Publishes a build id and offers a reload when the running document falls
behind. The toast never reloads on its own; the only automatic reload is
recovery from a chunk that no longer exists.
Build id derivation
-------------------
The obvious approach — hash the emitted asset filenames, which already
embed content hashes — does not work: Bun's minified identifier naming is
not deterministic. Building an unchanged tree twice produces byte-different
output roughly one run in three (same length, ~100k differing bytes, all of
it mangled names). Output hashes therefore move with no source change,
which would fire the toast on redeploys of identical code and train people
to ignore it.
The id is instead derived from the bundle's source inputs, so it changes if
and only if something we control changed. Verified stable across eight
consecutive builds while the entry hash flipped between both variants.
This non-determinism also means two builds of the same commit embed
different bytes into the server binary, which is worth addressing
separately for reproducible builds.
Detection
---------
SWR with `refreshInterval` + `revalidateOnFocus`, per the repo's React
effects policy. SWR does not poll while the document is hidden, so
background tabs stay quiet without extra gating. Unknown state on either
side — missing meta tag, failed fetch, 503 during a dev rebuild — never
produces a prompt.
Stylesheet hashing
------------------
Tailwind's output was stable-named and therefore served `no-cache`, letting
a tab revalidate into new CSS while running old JS. Tailwind purges unused
classes per build, so classes the old bundle still emits could silently
lose their styles. It is now content-hashed and moves with the build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
apps/fabro-web is bundled by a custom Bun script (scripts/build.ts), not
Vite. The stale reference sends agents toward Vite-specific APIs — most
notably `vite:preloadError`, which does not exist in this codebase — when
reasoning about the SPA build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
