The worker subprocess is spawned with env_clear+allowlist by the server, so
the only sensitive value in its env is FABRO_WORKER_TOKEN itself. Read the
token and remove_var it from the process env in main() before Tokio starts
worker threads, then thread it explicitly through runner::execute(&str).
Every descendant (hooks, local sandbox, devcontainer initializeCommand,
MCP stdio, etc.) now inherits a worker env with no bearer in it, so an
unscrubbed spawn site cannot leak the token. This makes the prior denylist
scrub in fabro-hooks and fabro-sandbox redundant — delete it and the shared
WORKER_SECRET_ENV_DENYLIST constant. The sandbox keeps its _api_key/_secret/
_token/_password/_credential suffix heuristic for user-supplied env_vars
hygiene.
Extend the server-dispatched-worker env-leak integration test to also
assert a Bash stage running in the worker does not observe FABRO_WORKER_TOKEN.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- fabro-client: collapse identical match arms for DevToken/Worker bearer
- fabro-hooks: rewrite filter_map(bool::then) as filter().map() chain
- fabro-sandbox: import WORKER_SECRET_ENV_DENYLIST rather than absolute path
- fabro-server: box large execute_run_in_process future; take path: &str in
test-only bearer_request; use let-else in session-secret test; replace unit
pattern _ with () in worker_token request_parts helper; import StatusCode
- fabro-cli run/mod.rs: box large runner::execute future
- fabro-cli worker_auth.rs: drop unused async on shutdown, allow
clippy::unwrap_used at file level for subprocess test harness setup
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Collapse splice_model_fallbacks and splice_events into a single
generic splice_combine guarded by a new SpliceMarker trait; the
Combine impl for Vec<T: SpliceMarker> replaces the two per-enum impls.
- Drop inherent iter/iter_mut (redundant with Deref/DerefMut),
AsRef/AsMut, and IntoIterator for &_/&mut _ on ReplaceMap/StickyMap/
MergeMap — they had zero external callers. DerefMut and IntoIterator
for Self stay because labels.extend(...) relies on both.
- Drop unused _api parameter on resolve_web.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The check `ctx.user_settings().cli.output.verbosity == OutputVerbosity::Verbose`
repeated 6 times across doctor, preflight, run command/resume/mod. Add a
`verbose()` method and replace every call site. `cargo fix` handles the
now-unused `OutputVerbosity` imports.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Auto-applied by `cargo +nightly-2026-04-14 fmt --all`. `cargo fix`
earlier inserted the import in a position that violated the grouped
ordering.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After StubEnv was removed, the EnvSource trait had only two impls
(ProcessEnv unit struct and a blanket HashMap impl) and tests already
passed HashMaps. Replace the trait with a free `process_env_snapshot()`
function and take `HashMap<String, String>` by value in
`ServerSecrets::load` and the startup validators.
Also flatten `StartupResolution` to a `(AuthMode, ServerSecrets)` tuple
and drop the `StartupValidationError` wrapper in favor of
`anyhow::Result`, and inline the `*_with_lookup` test-only wrappers in
`spawn_env.rs` so tests call `apply_allowlist` directly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The cfg(test) helper only re-implemented the production `with_server_mode`
struct literal so tests could inject settings without disk I/O. Three
tests used it, but each one was self-referential — asserting what the
helper itself does rather than exercising production code. Remove the
helper and those three tests.
`synthetic_context_with_settings` is no longer needed either (its
flexibility only mattered for the removed tests); collapse it into
`synthetic_context`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`ResolvedBaseContext` was a staging struct that held pre-context
settings so main.rs could read `user_settings.cli` before committing
to a full `CommandContext`. Now that install no longer needs the
indirection, the staging step doesn't earn its weight.
Give `CommandContext` a direct `from_disk(cli_layer, process_local_json)`
constructor that does the load + printer derivation + struct build in
one shot. main.rs builds one `base_ctx` before the dispatch match and
every arm borrows it — the `build_base_ctx` closure and ~20 duplicate
`let base_ctx = build_base_ctx()?;` lines disappear.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Public entry points (`execute`, `run_install`, `run_install_github_command`,
`run_install_inner`, `run_install_github_inner`) now take `&CommandContext`
instead of a 4-tuple of `(cli, cli_layer, process_local_json, printer)`.
Extract cli/printer/json from the context once at the top of each.
The nested doctor invocation inside run_install_inner previously built a
fresh `ResolvedBaseContext::from_disk(...).to_context()` with
`process_local_json = false`. Doctor only reads the resolved output
format (`base_ctx.json_output()`), not the invocation flag, so passing
the parent ctx through is behaviorally equivalent and avoids a second
disk load.
main.rs drops the `cli_settings` local entirely — no remaining command
needs it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
These top-level commands still took `&CliNamespace` + `Printer`
separately. Thread `&CommandContext` through the public entry points
and pull what's needed (`user_settings().cli`, `printer()`,
`json_output()`) from the context:
- parse: both args were unused — drop entirely.
- workflow list/create: use `ctx.json_output()` / `ctx.printer()`.
- exec: bind `cli = &ctx.user_settings().cli` at the top; drop unused
printer param.
- upgrade: extract cli/printer inside run_upgrade; leave the private
run_upgrade_brew helper with its existing signature (unit tests use
`CliNamespace::default()` directly).
- uninstall: use `ctx.json_output()` / `ctx.printer()`.
Install remains on the old signature — its nested callback structure
makes a larger refactor than this simplification pass warrants.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
StubEnv was a thin newtype only used by tests but compiled into every
build. Implementing EnvSource directly on HashMap<String, String> lets
test sites pass a HashMap and removes the type entirely.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The helper was a 2-line indirection with a single caller. Inline its
body so the whole command fits in one function.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`load_pr_record` built a `with_target`-derived context internally and
threw it away, forcing callers to re-derive or fall back to `base_ctx`.
Return the context alongside the record and let close/merge/view use
it directly for printer/json access and github-credentials lookup.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `fabro store dump` -> `fabro dump` rename left `commands/store/` as a
vestigial directory with a stale one-line `StoreRunExport` alias. Move
`dump.rs` and `rebuild.rs` up to `commands/`, import `RunDump` directly,
rename `dump::dump_command` -> `dump::run`, and clean up stale docs and
a noise test that only asserted clap's default error output.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`CommandContext::base` had a single caller (install.rs) and duplicated
the disk-load path that `ResolvedBaseContext::from_disk` already
provides. Route install through `ResolvedBaseContext::from_disk(...).to_context()`
and drop the standalone constructor. `base_with_settings` stays as the
private shared helper behind both `to_context` entry points.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `cli_settings` local was a standalone clone of `user_settings.cli`.
Drop the clone and rebind it as `&resolved_base.user_settings().cli`
inside the async block — all callers already borrowed it anyway.
Pre-async uses inline `resolved_base.user_settings().cli.<field>`
directly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The secret dispatcher pre-computed json/printer and threaded them into
every subcommand. Pass the context directly so each subcommand pulls
what it needs, dropping the fabro_util:🖨️:Printer import from
three files along the way.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`run_bulk` and `remove_from` took separate `json: bool` + `printer`
parameters. Thread the context through instead and pull json/printer
out of it inside the helper.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The helper only exists to let tests inject pre-loaded settings. Inline
the struct-literal into the sole production caller and mark the helper
`#[cfg(test)]` so the test-seam intent is explicit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The check `ctx.user_settings().cli.output.format == OutputFormat::Json`
(and its `!=` variant) was repeated 51 times across 36 files. Add a
`json_output()` method on CommandContext and replace every call site.
`cargo fix` handles the now-unused `OutputFormat` imports.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- ssh/graph: remove `explicit_json_requested() &&` guard before
`require_no_json_override()` (the call already no-ops without --json).
- pr close/merge/view: drop the outer `with_target` derivation that was
used only for `printer()` and the output format — both match base_ctx,
so the derivation was an unused disk-read + settings re-merge.
- command_context tests: collapse `synthetic_context` to delegate to
`synthetic_context_with_settings`, removing duplicated struct literals.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The fresh `Resolver` API takes a `&SettingsLayer`, not a file path, so
its constructor should match the existing `ServerSettings::from_layer`
and `UserSettings::from_layer` naming. The `*_from_file` suffix on the
older free helpers is a legacy choice (their input was historically
loaded from a file); leave those names alone since they're a stable
public API used across many call sites.
Also drop two doc-comment references to specific call sites (the simplify
guidelines treat those as rot bait — call sites move, the doc lies).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously, each per-namespace `resolve_*_from_file` helper, together with
`resolve_storage_root` and the `*Settings::from_layer` constructors, called
`apply_builtin_defaults(file.clone())` independently. That meant every
batch resolve cloned the entire `SettingsLayer` (including hooks, MCPs,
sandbox config, etc.) and merged the static defaults layer once per call.
The worst offender, `fabro_workflow::operations::create::resolve_settings_tree`,
ran that pipeline four times back-to-back per `create_run` request.
Add `fabro_config::Resolver`, which applies builtin defaults exactly once
on construction and exposes per-namespace methods (`server`, `cli`,
`features`, `project`, `run`, `workflow`, `storage_root`) plus low-level
`*_into(&mut errors)` variants for callers that want to merge errors
across multiple namespaces. The standalone `resolve_*_from_file` helpers
and `resolve_storage_root` remain on the public API, but each is now a
one-liner that delegates to `Resolver::from_file(...)` so single-namespace
callers see no behavior change.
Migrate the multi-namespace consumers:
- `ServerSettings::from_layer` and `UserSettings::from_layer` build one
`Resolver` and call the `*_into` pair, preserving the original
"surface all errors from both namespaces" semantics.
- `resolve_settings_tree` builds one `Resolver` and pulls all four
namespaces from it, dropping three redundant defaulting+clone passes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three byte-identical copies of `render_resolve_errors` had drifted into
`fabro-server/src/run_manifest.rs`, `fabro-workflow/src/operations/start.rs`,
and `fabro-workflow/src/operations/create.rs`. Each one folded a
`&[ResolveError]` into a semicolon-separated string for surfacing through
`anyhow!` / `Error::Precondition` envelopes.
Promote the helper to `fabro_config::render_resolve_errors` (it lives next
to `ResolveError`, the type it acts on) and rewrite the four call sites
in workflow ops plus the one in run_manifest to call the shared version.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Promote `apply_storage_dir_override` to `fabro_config::user` so the
serve startup path stops carrying its own copy of the storage-root
mutation that already lived in `fabro-cli/user_config.rs`.
- Inline the `load_settings` and `router_web_enabled` one-liner wrappers
in `fabro-server/src/serve.rs` and drop the dead
`let _ = CliLayer::default()` marker.
- Cache the demo `server_settings()` JSON in a `OnceLock` so the demo
mode stops re-parsing TOML, re-resolving, and re-serializing the same
static fixture on every `GET /api/v1/settings` request.
- Standardize the four `state.settings.read().unwrap()` callsites in
`fabro-server/src/server.rs` on `.expect("settings lock poisoned")`
to match the existing convention.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related correctness bugs surfaced by the failing test suite:
1. Server-owned settings didn't flow into run settings, and the few
server-only fields that did leak in made run snapshots bulky and let
callers re-resolve server state from the run layer.
- effective_settings::materialize_settings_layer now treats the
server's run/features stanzas as base defaults (client layers
still win where set), and enforce_server_authority keeps the
original cherry-pick of storage/scheduler/artifacts/web/api but
no longer lets the rest of the server namespace propagate. auth,
listen, ip_allowlist, slatedb, logging, and integrations stay on
the server, where AppState::server_settings() already has them.
- run_preflight, the scheduler start-path, and operations::start
now read GitHub integrations from state.server_settings() (or
StartServices::github_permissions, which the server populates)
instead of re-resolving the server namespace from the run's
settings layer.
- create_app_state{_with_options,_with_env_lookup,_with_options_and_registry_factory}
and create_app_state_with_store_and_env_lookup all route through
ensure_test_auth_methods so the strict resolver accepts
SettingsLayer::default() in tests.
- Fixed the start_run_persists_full_settings_snapshot assertion
that expected server.integrations.github.app_id in the run's
persisted settings — the new design deliberately omits it.
2. Unit and integration tests were hitting live AWS S3.
- Added a NoProxyReqwestConnector (behind a dedicated reqwest 0.12
dep aliased as object_store_reqwest) and wired it through
AmazonS3Builder::with_http_connector. macOS SystemConfiguration
proxy discovery in the default reqwest client was blowing past
nextest's 20s kill timeout on serve.rs's S3 builder unit tests;
the no-proxy connector brings them under 15ms.
- InstallAppState::for_test_with_paths now sets
FABRO_TEST_IN_MEMORY_STORE=1 so /install/finish's artifact-metadata
sentinel write short-circuits to the in-memory object store and
never contacts AWS. The install integration tests verify
persistence/redaction, not S3 reachability.
`cargo nextest run --workspace`: 4495/4495 passing.
`cargo +nightly-2026-04-14 fmt --check --all`: clean.
`cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings`: clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Import serde:🇩🇪:Error trait so the `custom` fn pointer uses `D::Error`
instead of the absolute `serde:🇩🇪:Error::custom` path.
- Import `fabro_api::types::ServerSettings` / `fabro_config::UserSettings`
directly rather than through absolute paths.
- Gate sync `std::fs::write` fixture setup in new config resolver tests
with a file-level `#![expect(clippy::disallowed_methods, …)]`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Remove CommandContext::cli_settings and cascade through 11 functions
whose only use of `cli: &CliNamespace` was constructing it; dispatchers
now forward only cli_layer.
- Drop `ServerSettings as CurrentServerSettings` /
`ServerNamespace as ResolvedServerSettings` rename aliases; use the
canonical type names in fabro-server.
- Inline `local_server::server_settings` and `user_config::{resolve_user_settings,
resolve_cli_settings}` wrappers; callers use `ServerSettings::from_layer`
/ `UserSettings::from_layer` directly (anyhow converts via `?`).
- Trim narrative module doc in fabro-config/src/lib.rs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clean up clippy -D warnings violations that slipped through in 4bf0c4031:
- envfile.rs: extend the existing module-level #![expect] to also cover clippy::disallowed_types so the intentional std::io::Write usage stops tripping the workspace lint.
- install.rs: import ServerSecrets and EnvFileUpdate at the top and drop the fully-qualified call sites (unused_qualifications); collapse the nested if-let around the post-finish manual-credentials cleanup (collapsible_match); gate the test's std::fs::write with #[expect(clippy::disallowed_methods, reason="...")].
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Derive strum::IntoStaticStr on InstallObjectStoreProvider/CredentialMode and use it in as_session_value instead of a hand-written match.
- Split resolve_install_object_store_state: extract resolve_s3_manual_credentials and fold the redundant outer "missing credentials" guard into its (None, None) arm.
- Replace the per-endpoint installFetch boilerplate with installRequest / installJsonRequest<T> so each install-api wrapper is a single call.
- Extract runStepSubmit inside InstallApp; the LLM, server, object-store, and GitHub step handlers now share the setSubmitting / try / refresh-session / navigate / finally scaffolding.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
