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>
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>
`slack_link` builds `<url|label>`, and `escape_slack_controls` covers
Slack's documented escapes (`&`, `<`, `>`) but not `|`. Slack has no
escape for `|`, so a label containing one splits the markup and can make
Slack reject the block.
`is_safe_slack_link_url` already guards the URL half against `|`; the
label half was unguarded. It did not matter before because the only
labels were "Open in Fabro" and a PR number. Review target labels are
model-authored, so this is now reachable.
Replace `|` inside link labels, which keeps the link working. Plain-text
labels are untouched, since `|` is fine outside link markup.
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>
`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>
A Daytona-backed run could fail five seconds after start when the sandbox
git clone hit a transient GitHub "Repository not found" error. Clone-based
providers mint an installation access token and clone with it in the same
breath, but GitHub replicates a new token to its edge cache sites
asynchronously. A clone that starts within a second of the mint can be
rejected before the token is visible to the site serving it, and on a
private repo that rejection arrives as "Repository not found" because
GitHub answers unauthorized reads with 404.
Nothing retried the clone, and the failure classified as `deterministic`,
which is the one category `loop_restart` refuses to restart. An identical
run relaunched 46 seconds later succeeded with no changes.
A successful mint is what makes the message safe to retry.
`resolve_clone_credentials` already fails loudly on every deterministic
explanation for a clone 404: the installation lookup 404s when the App is
not installed for the owner, and token creation 422s when the installation
does not cover the repo. Once credentials are in hand, "not found" from the
clone itself cannot mean "no access".
Add `clone_retry` and use it from both clone-based providers: 3 attempts
with 3s then 9s backoff, reusing the same token so replication keeps making
progress instead of restarting the clock. Token-replication signatures
retry only when credentials are present, so a public clone of a wrong URL
still fails fast. Infrastructure failures retry either way.
The Docker provider had the identical single-shot clone and is the default
runtime provider, so it is covered too.
Also fix two nearby issues found while reading the area:
- The GitHub-URL-parse path in the Daytona clone skipped `fail_init`,
unlike every sibling path, so `InitializeFailed` was never emitted.
- The `classify_exec_failure` hint for "repository not found" asserted the
App installation may not cover the repo. After a successful scoped mint
that diagnosis is impossible, and it sent operators hunting a
configuration problem that did not exist.
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>
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>
`small_default_for_provider` fell back to the provider's normal default
when no model was marked `small_default`. That turned "give me the small
utility model" into "give me the flagship" for any provider without one.
Run title generation asks for the small default, then gives it a 64-token
budget and a 10s timeout. On a server where kimi is the highest-priority
configured provider, that resolved to kimi-k3 — an always-reasoning model
that burned 161 reasoning tokens before emitting anything. The structured
output never completed, `generate_object` returned NoObjectGenerated, and
the caller silently kept the deterministic title.
Return `None` instead, and have `small_default_for_configured_ids` move on
to the next configured provider. The ordinary default is used only when no
configured provider marks a small model, and it now comes from the
configured set rather than the global catalog default.
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>
