Review cleanups for the tools/reasoning-effort model test change:
- Extract a shared parse_query_enum helper in the models handler in
place of two copy-pasted parse-or-400 match blocks.
- Collapse the duplicated basic-probe pipeline in fabro-llm behind a
single basic_probe core; name the shared EXPANDED_MAX_TOKENS budget.
- Pass &ModelTestArgs to test_models_via_server instead of threading
five of its fields positionally.
- Dedupe the two forwarding CLI integration tests behind a helper.
- Derive clap::ValueEnum for ReasoningEffort behind a feature-gated
clap dep (same pattern as MergeStrategy in fabro-types) so --help,
cli.mdx, and error output list effort values from the enum instead
of a hand-written list that drifts.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDdjG18d2AHh7mFWXFkBLn
The [BLOB_HASH] placeholder was defined both here and in the shared
json_snapshot_filters regexes, which had to be edited in lockstep. The
fabro_json_snapshot! macro always applies the shared filters to the
rendered string, so the normalizer copies were redundant.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two root-cause fixes for the sandbox failure where an inline Dockerfile
came back from the store as `ARG REDACTED` and the Daytona snapshot
build died on the unset variable.
Entropy redaction measures values, not assignment pairs. The detector
matched `NAME=value` as one token, so an uppercase name merged its
charset into a pure-hex value (which alone can never exceed 4.0 bits)
and pushed the pair over the 4.5-bit threshold — then replaced the
whole pair, destroying the name. `find_entropy_regions` now strips an
identifier-shaped `NAME=` prefix before measuring and redacts only the
value, matching the gitleaks layer's `key=REDACTED` shape.
Execution no longer reads redacted content. Every stored event passes
through the redaction sink, and `load_from_store` rehydrated the
worker's RunSpec from the projection folded from those events — so a
redactor false positive silently rewrote the spec the sandbox builds
from (and changed its snapshot identity). The creation path now writes
the exact spec bytes to the content-addressed blob store and records
`spec_blob` on run.created; `load_from_store` loads the spec from the
blob, keeping the event stream authoritative for run identity,
provenance, and event-recorded blob ids. Retry and fork carry the
source run's `spec_blob` forward, so derived runs stop inheriting the
redacted copy. Runs created before the blob existed fall back to the
folded spec.
The projection and every API surface keep serving the redacted fold;
blobs were already stored unredacted (the workflow bundle carries the
same bytes), so this adds no new exposure at rest.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The manifest builder's best-effort pre-run push converted every result
into a PreRunPushOutcome that was serialized into GitContext, expanded
into five OpenAPI union arms, and generated into API clients — but no
production path ever read it; every field read was a test.
Delete the concept while preserving the behavior:
- Drop the PreRunPushOutcome enum and GitContext.push_outcome from
fabro-types; GitContext keeps origin_url, branch, optional sha, and
dirty, which remain real execution inputs and provenance.
- Rename the manifest outcome builder to push_manifest_branch_best_effort,
a side-effect-only helper with the same decision rules: skip without an
origin, skip on configured-repository mismatch, skip when the branch is
already synced, otherwise push noninteractively and discard the result
without failing manifest creation or logging raw Git stderr.
- Prove the push through repository state instead of the deleted enum: a
branch ahead of a local bare origin is pushed during manifest build, a
mismatched configured repository is not, and a failing remote helper
still cannot fail manifest creation.
- Remove push_outcome from GitContext in OpenAPI, delete the five-arm
union schemas, and drop the fabro-api type replacement and re-export.
- Keep one regression proving historical run.created events with a nested
push_outcome still deserialize through ordinary unknown-field tolerance
and reserialize to the reduced shape. No migration or event rewrite.
Old JSON carrying the removed field stays readable. Newly generated
clients omit a field older servers required, so new-client-to-old-server
compatibility is intentionally not promised for this pre-1.0 contract.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Drop ManifestTarget.identifier (the raw token the user typed) and
ManifestGoal.path (the original goal-file path) from the OpenAPI
manifest schema, the Rust manifest builder, the regenerated Rust and
TypeScript client types, and every canonical test fixture. Neither
field had a production reader: the server selects the workflow by
target.path and consumes only the resolved goal type and text.
Target path, goal type/text, manifest versioning, and submitted-byte
persistence are unchanged. Old request bodies that still carry the
removed properties remain accepted through unknown-field tolerance,
pinned by a dedicated public-route regression test.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AgentPermissions duplicated fabro_types::PermissionLevel: same variants,
same kebab-case wire form, same crate. PermissionLevel is strictly richer
(Hash, strum, clap::ValueEnum) and is already the with_replacement target
for the OpenAPI PermissionLevel schema, whose values are identical to the
AgentPermissions schema this branch deletes.
Delete AgentPermissions and type the [cli.exec.agent] permissions setting
as PermissionLevel. This drops the adapter match in `fabro exec` and the
`as AgentPermissionLevel` alias that existed only to tell the two names
apart. The TOML wire form is unchanged.
Removing run.agent.permissions also changed the serialized run spec, but
two fabro-cli inline snapshots still carried "permissions": null. They
failed on this branch and passed on main. Accept the updated snapshots.
Also tighten the removed-setting test to assert the exact unknown-field
message, rename its module to run_agent now that it covers more than
fabro_tools, and drop three doc references to the removed setting.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolve the model catalog table conflict in docs/public/core-concepts/models.mdx
by keeping both changes: this branch's `kimi` -> `moonshot` provider rename for
the Kimi rows, and main's new DeepSeek V4 rows.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The preamble removal changed the `stage.completed` payload, so the
`attach --json` inline snapshot no longer matched. Drop the stale
`current.preamble` line.
Reword the `context_values` doc row. `stage_context_values` only strips
runtime-only keys; it does not normalize artifact pointers to blob refs
the way `artifact::durable_context_snapshot` does, so calling it a
durable snapshot overstated it. Point readers at `checkpoint.completed`
for the durable projection.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Raise the replacement test's deadline to 20s. The server takes up to 1s to
notice the replacement and then bounds its own shutdown at 5s, so the old
5s deadline sat below the worst case and could fail a healthy server on a
loaded runner. A passing run still exits in about a second.
Reword the SHUTDOWN_TIMEOUT comment.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Exit the process from `main` for every command instead of returning. The
`mcp start` command parks Tokio's stdin reader on a read only the MCP host
can end, so dropping the runtime waits forever. Exiting in `main` also keeps
the CLI telemetry event, which the previous exit inside the MCP command
skipped.
That removes the reason for the `McpServerExit` enum, whose only job was to
carry an implementation detail out to the CLI so it could exit.
Watch the executable through its device and inode on Unix. That is a
complete file identity, so the length and modification time no longer add
anything. Drop the PATH scan: `current_exe` reports the symlink itself on
macOS, so it detects a Homebrew relink without it. This also drops the
`fabro-static` dependency and a clippy suppression.
Bound the shutdown wait after an upgrade is detected. The transport closes
by writing to a stdout the host may already have stopped reading, which
could hang the exit the change is supposed to trigger.
Log a warning when upgrade detection cannot start, rather than disabling it
silently.
Share one spawn helper between the two raw stdio tests, and link the test
executable instead of copying 200 MB of binary.
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.
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>
`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>
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>
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>
