diff --git a/Cargo.lock b/Cargo.lock index 8e3b8270d..888deaae2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1520,6 +1520,7 @@ dependencies = [ "fabro-mcp", "fabro-model", "fabro-sandbox", + "fabro-static", "fabro-test", "fabro-types", "fabro-util", @@ -1572,6 +1573,7 @@ dependencies = [ "fabro-http", "fabro-model", "fabro-oauth", + "fabro-static", "fabro-util", "fabro-vault", "httpmock", @@ -1641,6 +1643,7 @@ dependencies = [ "fabro-retro", "fabro-sandbox", "fabro-server", + "fabro-static", "fabro-store", "fabro-telemetry", "fabro-test", @@ -1699,6 +1702,7 @@ dependencies = [ "fabro-api", "fabro-http", "fabro-model", + "fabro-static", "fabro-types", "fabro-util", "fs2", @@ -1727,6 +1731,7 @@ dependencies = [ "dirs", "fabro-macros", "fabro-proc", + "fabro-static", "fabro-types", "fabro-util", "ipnet", @@ -1761,6 +1766,7 @@ name = "fabro-devcontainer" version = "0.213.0-nightly.0" dependencies = [ "fabro-http", + "fabro-static", "fabro-util", "insta", "serde", @@ -1780,6 +1786,7 @@ dependencies = [ "chrono", "fabro-http", "fabro-macros", + "fabro-static", "fabro-test", "fabro-types", "jsonwebtoken", @@ -1832,6 +1839,7 @@ dependencies = [ name = "fabro-http" version = "0.213.0-nightly.0" dependencies = [ + "fabro-static", "http", "reqwest 0.13.2", "thiserror 2.0.18", @@ -1844,6 +1852,7 @@ dependencies = [ "anyhow", "base64", "fabro-config", + "fabro-static", "fabro-types", "fabro-vault", "ring", @@ -1877,6 +1886,7 @@ dependencies = [ "fabro-http", "fabro-macros", "fabro-model", + "fabro-static", "fabro-test", "fabro-util", "futures", @@ -1924,6 +1934,7 @@ dependencies = [ name = "fabro-model" version = "0.213.0-nightly.0" dependencies = [ + "fabro-static", "insta", "serde", "serde_json", @@ -1937,6 +1948,7 @@ dependencies = [ "axum", "base64", "fabro-http", + "fabro-static", "fabro-test", "fabro-util", "hex", @@ -1990,6 +2002,7 @@ dependencies = [ "fabro-config", "fabro-github", "fabro-proc", + "fabro-static", "fabro-types", "futures", "git2", @@ -2039,6 +2052,7 @@ dependencies = [ "fabro-sandbox", "fabro-slack", "fabro-spa", + "fabro-static", "fabro-store", "fabro-test", "fabro-types", @@ -2092,6 +2106,7 @@ version = "0.213.0-nightly.0" dependencies = [ "fabro-http", "fabro-interview", + "fabro-static", "fabro-workflow", "futures-util", "rustls", @@ -2112,6 +2127,10 @@ dependencies = [ "rust-embed", ] +[[package]] +name = "fabro-static" +version = "0.213.0-nightly.0" + [[package]] name = "fabro-store" version = "0.213.0-nightly.0" @@ -2148,6 +2167,7 @@ dependencies = [ "dirs", "exec", "fabro-http", + "fabro-static", "fabro-util", "fork", "git2", @@ -2184,6 +2204,7 @@ dependencies = [ "fabro-config", "fabro-http", "fabro-proc", + "fabro-static", "fabro-types", "fabro-util", "insta", @@ -2238,6 +2259,7 @@ dependencies = [ "anyhow", "console 0.15.11", "dirs", + "fabro-static", "insta", "open", "rand 0.9.4", @@ -2301,6 +2323,7 @@ dependencies = [ "fabro-model", "fabro-retro", "fabro-sandbox", + "fabro-static", "fabro-store", "fabro-template", "fabro-test", @@ -6946,6 +6969,7 @@ dependencies = [ "async-stream", "axum", "fabro-http", + "fabro-static", "futures-util", "http", "serde", diff --git a/Cargo.toml b/Cargo.toml index 9843cd239..8868a1d2c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -79,6 +79,7 @@ rust-embed = "8" percent-encoding = "2" minijinja = "2" fabro-http = { path = "lib/crates/fabro-http" } +fabro-static = { path = "lib/crates/fabro-static" } graphviz-sys = { git = "https://github.com/fabro-sh/graphviz-sys" } strum = { version = "0.28", features = ["derive"] } zeroize = "1" diff --git a/clippy.toml b/clippy.toml index 033174d94..29f8bce12 100644 --- a/clippy.toml +++ b/clippy.toml @@ -22,6 +22,10 @@ disallowed-methods = [ { path = "std::fs::OpenOptions::open", reason = "Blocking open; prefer tokio::fs::OpenOptions::open on Tokio paths. OS file-lock semantics may require spawn_blocking instead. Document intentional sync I/O with #[expect(clippy::disallowed_methods, reason = \"...\")]" }, { path = "std::env::set_var", reason = "Server/process env must be injected at construction or child-process spawn time, not mutated globally. See docs-internal/server-secrets-strategy.md" }, { path = "std::env::remove_var", reason = "Server/process env must be injected at construction or child-process spawn time, not mutated globally. See docs-internal/server-secrets-strategy.md" }, + { path = "std::env::var", reason = "Use fabro_static::EnvVars for fixed environment variable names; document intentional process-env lookup facades with #[expect(clippy::disallowed_methods, reason = \"...\")]", allow-invalid = true }, + { path = "std::env::var_os", reason = "Use fabro_static::EnvVars for fixed environment variable names; document intentional process-env lookup facades with #[expect(clippy::disallowed_methods, reason = \"...\")]", allow-invalid = true }, + { path = "std::env::vars", reason = "Snapshotting the ambient process env must be limited to documented subprocess/test/bootstrap facades.", allow-invalid = true }, + { path = "std::env::vars_os", reason = "Snapshotting the ambient process env must be limited to documented subprocess/test/bootstrap facades.", allow-invalid = true }, { path = "reqwest::Client::new", reason = "Use fabro_http::http_client() or fabro_http::test_http_client()", allow-invalid = true }, { path = "reqwest::Client::builder", reason = "Use fabro_http::HttpClientBuilder::new()", allow-invalid = true }, { path = "reqwest::blocking::Client::new", reason = "Use fabro_http::blocking_http_client() or fabro_http::blocking_test_http_client()", allow-invalid = true }, diff --git a/docs/plans/2026-04-24-001-refactor-adopt-uv-patterns-plan.md b/docs/plans/2026-04-24-001-refactor-adopt-uv-patterns-plan.md new file mode 100644 index 000000000..7e854271f --- /dev/null +++ b/docs/plans/2026-04-24-001-refactor-adopt-uv-patterns-plan.md @@ -0,0 +1,988 @@ +--- +title: "refactor: adopt six engineering patterns from uv" +type: refactor +status: active +date: 2026-04-24 +deepened: 2026-04-24 +--- + +# refactor: adopt six engineering patterns from uv + +## For the engineer picking this up + +This plan adopts six patterns from `astral-sh/uv` into fabro. The patterns are independent and land as six phases in dependency order; most phases are one PR, while `fabro-dev` and `OptionsMetadata` may split into one PR per implementation unit. You do not need to do them all — each phase is atomic and valuable alone. But the whole arc is coherent: it hardens secret handling, centralizes conventions, and kills drift between code and docs. + +The uv reference paths below are on the local filesystem at `/Users/bhelmkamp/p/astral-sh/uv`. Read the referenced files before starting each phase — they are short and the patterns copy closely. + +This plan derives from conversation, not a brainstorm doc. Scope was confirmed: all six patterns, phased. + +## Overview + +Six uv patterns, ordered by risk/value so each phase is individually shippable: + +| # | Phase | New crate(s) | Scope | +|---|---|---|---| +| 1 | `fabro-static` env var registry | `fabro-static` | Replace ~113 `env::var` string literals and 7 clap `env =` sites with constants on an `EnvVars` struct | +| 2 | `DisplaySafeUrl` newtype | `fabro-redacted` | Add credential-redacting `Url` wrapper; migrate high-risk URL logging call sites | +| 3 | Expanded snapshot helpers | (extend `fabro-test`) | Lift `fabro_json_snapshot!` into `fabro-test`; add value-shaped wrapper parallel to existing `fabro_snapshot!` | +| 4 | `miette` CLI diagnostics | (extend `fabro-cli`) | Wrap CLI root error with `miette::Diagnostic` for styled chained errors and help footers while preserving existing exit-class hint logic | +| 5 | `cargo dev` unified CLI | `fabro-dev` | New binary crate replacing `bin/dev/*.sh` and `scripts/*-spa*.sh`; add `dev = "run -p fabro-dev --"` alias | +| 6 | `OptionsMetadata` + auto-gen docs | `fabro-options-metadata`; extend `fabro-macros`, `fabro-dev` | Runtime metadata model plus proc-macro extracting clap help/`ValueEnum` metadata; `cargo dev generate-cli-reference` regenerates `docs/reference/cli.mdx` | + +## Problem Frame + +Fabro is maturing from a single-author codebase into a multi-contributor project. Six pattern gaps are starting to show: + +1. **Env var literals are scattered across 42 distinct strings** in ~113 call sites. Renaming or auditing env var surface requires cross-repo grep. Three partial registry constants already exist in random crates, suggesting the convention has been discovered but not applied. +2. **URL-bearing values are logged directly** in at least 10-15 sites including `fabro-github` (which produces `https://x-access-token:@github.com/...` strings that flow through `anyhow::Error::chain`). `docs-internal/logging-strategy.md` bans credential-shaped strings at every log level including TRACE. Today this is enforced by review, not the type system. +3. **`docs/reference/cli.mdx` duplicates clap doc-comments** by hand. Any change to `args.rs` risks silent drift. Same for `docs/reference/user-configuration.mdx` against `lib/crates/fabro-types/src/settings/**/*.rs`. +4. **`fabro_json_snapshot!` lives in one crate's `tests/it/support/`** and is reinvented per-test with ad-hoc filter additions. `fabro-test`'s `fabro_snapshot!` covers command-shaped tests but not value-shaped ones. +5. **CLI errors render as plain concatenated `anyhow::Error::chain()`** in `fabro-cli/src/main.rs:138-170`. Contributors have asked about prettier output for config/validation failures. +6. **Dev workflow is 5 shell scripts** (`bin/dev/{check-boundary,docker-build,release}.sh`, `scripts/{refresh,check-budgets}-fabro-spa*.sh`). No discoverability (`cargo dev --help`), no shared plumbing, no Windows support, no reuse of workspace dep versions. + +None of these is a crisis today. Each is a cheap fix when done deliberately, and expensive when done reactively after an incident (particularly #2). + +## Requirements Trace + +### Phase 1 — Env Var Registry +- **R1.** Fixed env var names are referenced via `fabro_static::EnvVars::FOO` in all non-build-script production code; the three existing partial-registry constants are consolidated. Dynamic env resolver/facade paths (`Env`, interpolation closures, subprocess allowlists, and test harness plumbing) are explicitly classified and either use the registry for known names or carry narrow `#[expect]`/module-level lint allowances with reasons. + +### Phase 2 — DisplaySafeUrl +- **R2.** `DisplaySafeUrl` exists as a `#[repr(transparent)]` newtype over `url::Url`; `Display` and `Debug` redact credentials by default; existing high-risk URL logging sites migrate to it. + +### Phase 3 — Snapshot Helpers +- **R4.** `fabro-test` exports a `fabro_json_snapshot!` macro with the same baseline filters as `fabro_snapshot!` plus JSON-specific normalizations; the crate-local copy in `fabro-cli/tests/it/support/mod.rs` deletes. + +### Phase 4 — miette CLI Diagnostics +- **R5.** CLI errors render via `miette`'s fancy style (colors, indentation, linked `help:` footer), preserving `exit::exit_class_for(&err)` auth-hint behavior and the existing telemetry/shutdown lifecycle. Source-highlighting and span labels are **explicitly out of scope** for Phase 4 — no fabro error type currently carries span context. A follow-up can introduce `miette::Diagnostic`-aware error types (e.g., for TOML parse errors via `toml::de::Error`, or workflow validation via `fabro_validate::Diagnostic`) once there's a concrete consumer. + +### Phase 5 — fabro-dev Unified CLI +- **R6.** A single `fabro-dev` binary replaces `bin/dev/*.sh` and `scripts/*-fabro-spa*.sh` after every local, AGENTS.md, and GitHub Actions caller has moved; `cargo dev --help` lists all subcommands. + +### Phase 6 — OptionsMetadata + Doc Generation +- **R3.** Generated sections of `docs/reference/cli.mdx` (CLI options and flags) and `docs/reference/user-configuration.mdx` (settings schema) are produced from a normal runtime metadata crate (`fabro-options-metadata`) plus `#[derive(OptionsMetadata)]`; CI fails on drift in those sections. Non-generated prose (conceptual intros, examples) remains hand-authored and is demarcated by `` / `` fences. + +### Cross-Phase +- **R7.** Each phase ships as an atomic delivery slice; simple phases are one PR, while larger phases may split by implementation unit. No phase silently depends on another beyond what the plan states. + +## Scope Boundaries + +- **Not a code audit.** This plan replaces patterns, not logic. Out of scope: finding bugs in `fabro-github`'s tokenized URL construction, rewriting `fabro-llm` provider logic, rethinking the OAuth flow. +- **Not a test infrastructure overhaul.** Phase 3 lifts one macro and rounds its surface area. It does not migrate existing `insta::assert_snapshot!` call sites — that's a separate cleanup. +- **Not a full Mintlify refresh.** Phase 6 generates only the pages that currently duplicate clap/settings metadata (`cli.mdx`, `user-configuration.mdx` derivable portions). Concept pages, tutorials, and anything hand-written stays hand-written. +- **Not a dependency refresh.** No clap/tracing/serde upgrades bundled in. +- **Not a Windows port.** `cargo dev` subcommands may still shell out where the underlying script does (e.g., `docker buildx`) — we're consolidating, not rewriting to pure Rust. +- **Not a miette everywhere push.** Only `fabro-cli`'s `main` wraps. Library crates keep `anyhow`/`thiserror` unchanged. Source spans, labels, and source-code snippets are out of scope until a concrete fabro error type carries that context. +- **Not a public-API DTO migration.** `DisplaySafeUrl` is for server-internal types where logging/serialization can leak credentials. Public API DTOs (`AvatarUrl`, `UserUrl`, anything in `fabro-api` / `fabro-types` that renders into the TypeScript client) keep `String` / `url::Url`. A separate follow-up can audit DTO fields for credentialed URLs if needed. +- **Not an auth-header redaction push.** `lib/crates/fabro-client/src/client.rs:1304` (`HeaderValue::from_str(&format!("Bearer {token}"))`) is in the same risk class as URL tokens but Phase 2 does not cover non-URL credential paths. A `RedactedHeaderValue` or similar is a follow-up. +- **Not a non-credentialed URL migration.** Sites that log URLs that cannot carry credentials (public webhooks, canonical server URL, Tailscale funnel URL) stay on `url::Url`. The plan enumerates explicit out-of-scope sites under Unit 2.2. + +## Context & Research + +### Relevant Code and Patterns + +**Existing fabro infrastructure to reuse:** +- `lib/crates/fabro-http/src/lib.rs:12` — re-exports `url::Url` as `fabro_http::Url`. Already has `#![allow(clippy::disallowed_methods, clippy::disallowed_types)]`. DisplaySafeUrl cohabitating would be natural, but standalone `fabro-redacted` matches uv's narrower-dependency philosophy and lets leaf crates depend on it without pulling in reqwest. +- `lib/crates/fabro-util/src/env.rs` — `Env` trait with `SystemEnv` / `TestEnv` impls. Orthogonal to the name registry; keep it. The registry is about *names*; the trait is about *injection*. +- `lib/crates/fabro-macros/src/lib.rs` — proc-macro crate with `Combine` derive and `e2e_test` attr already in place. Deps (`syn`, `quote`, `proc-macro2`) are already wired; `OptionsMetadata` macro plumbing slots in cleanly. The runtime trait/data model cannot live in this proc-macro crate; Phase 6 must add a normal crate (`fabro-options-metadata`) parallel to uv's `uv-options-metadata`. +- `lib/crates/fabro-test/src/lib.rs:61` — `INSTA_FILTERS` with 10 pre-baked filters. Line 1716: `fabro_snapshot!` macro. Line 1214: `TestContext::add_filter`. Good foundation for JSON variant. +- `lib/crates/fabro-cli/src/main.rs:138-170` — existing error rendering loop walking `err.chain()`. Line 152: `exit::exit_class_for(&err)` hint logic. Must survive. +- `lib/crates/fabro-cli/src/args.rs:22-40` — clap `GlobalArgs` with 5 `#[arg(env = ...)]` sites. Existing `ValueEnum` derives at lines 122, 345, 362, 537, 789, 1360. +- `.cargo/config.toml` — currently has one alias (`t = "test -- --format terse"`) and `FABRO_HTTP_PROXY_POLICY` env. Add `dev = "run -p fabro-dev --"`. + +**Existing partial env var registries to absorb:** +- `lib/crates/fabro-config/src/user.rs:14` — `FABRO_CONFIG_ENV` +- `lib/crates/fabro-util/src/browser.rs:5` — `SUPPRESS_ENV_VAR` +- `lib/crates/fabro-http/src/lib.rs:22` — `HTTP_PROXY_POLICY_ENV` + +**Highest-risk URL logging sites (Phase 2 migration targets):** +- `lib/crates/fabro-github/src/lib.rs:917,923` — `embed_token_in_url` returns raw `String` with installation token. Top priority. +- `lib/crates/fabro-github/src/lib.rs:250,259,263,1442`, `fabro-oauth/src/lib.rs:825` — `format!("...{url}")` in error messages that bubble through `anyhow`. +- `lib/crates/fabro-oauth/src/lib.rs:324,362,374,403` — `debug!` of token URLs. +- `lib/crates/fabro-llm/src/providers/fabro_server.rs:122,157` — `debug!(base_url = %url, ...)`. +- `lib/crates/fabro-server/src/web_auth.rs:398,419,547` — OAuth `redirect_uri` at `debug!`. +- `lib/crates/fabro-server/src/{install.rs:1454,1461, github_webhooks.rs:62,67, serve.rs:331}` — URL logging at `info!`. + +**High-density env var call sites (Phase 1 migration targets):** +| Crate | Count | +|---|---| +| `fabro-llm/src/client.rs` | 13 | +| `fabro-server/src/server.rs` | 6 | +| `fabro-test/src/lib.rs` | 6 | +| `fabro-server/src/install.rs` | 4 | +| `fabro-workflow/tests/it/daytona_integration.rs` | 4 | +| `fabro-cli/src/server_client.rs` | 3 | + +**Dev scripts to migrate (Phase 5):** +- `bin/dev/check-boundary.sh` — enforces CLI/server symbol allowlist via `rg -l` fallback. +- `bin/dev/docker-build.sh` — musl cross-compile via cargo-zigbuild in rust:1-bookworm container; flags `--arch {amd64,arm64} --tag --compile-only`. +- `bin/dev/release.sh` — computes version from days since 2026-01-01; supports `DRY_RUN`, `SKIP_TESTS`. +- `scripts/refresh-fabro-spa.sh` — AGENTS.md:24 mandates running before committing `apps/fabro-web/` changes. `cd apps/fabro-web && bun run build` + stage into `lib/crates/fabro-spa/assets/`. +- `scripts/check-fabro-spa-budgets.sh` — 15 MiB total / 5 MiB gzipped budget check. + +### uv Reference Implementations + +- `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-static/src/env_vars.rs` — single `EnvVars` struct; `pub const UV_FOO: &'static str = "UV_FOO"`. +- `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-redacted/src/lib.rs` (528 lines) — `DisplaySafeUrl(Url)`, `#[repr(transparent)]`, `RefCast` for zero-cost `&Url` → `&DisplaySafeUrl`. `DisplaySafeUrlError::AmbiguousAuthority` for URLs like `https://user/name:password@host` that parse but probably shouldn't. `has_credential_like_pattern` helper handles nested proxy URLs (`git+https://proxy.com/https://user:pw@github.com/...`). +- `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-options-metadata/src/lib.rs` — normal runtime crate defining `OptionsMetadata`, `Visit`, `OptionField`, `OptionSet`, and serialization/display helpers. Fabro needs the same split; proc-macro crates cannot own the runtime trait/data types consumers import. +- `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-macros/src/lib.rs` (202 lines) + `options_metadata.rs` (14KB) — `#[proc_macro_derive(OptionsMetadata, attributes(option, doc, option_group))]`. Emits impls against `uv_options_metadata::OptionsMetadata`; fabro's macro should emit against `fabro_options_metadata::OptionsMetadata`. +- `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-dev/src/` — binary crate with subcommands: `generate-cli-reference`, `generate-env-vars-reference`, `generate-options-reference`, `generate-json-schema`, `generate-all`, `clear-compile`, `compile`, `list-packages`, `render-benchmarks`, `validate-zip`, `wheel-metadata`. Uses `clap` subcommand derive. +- `/Users/bhelmkamp/p/astral-sh/uv/.cargo/config.toml:1-2` — `[alias] dev = "run --package uv-dev --features dev"`. +- `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-test/` — pre-baked insta filters + `uv_snapshot!` macro combining a command with those filters. + +### Institutional Learnings + +From `docs-internal/logging-strategy.md` and `docs-internal/server-secrets-strategy.md`: + +- **Credential-shaped strings banned at every log level including TRACE** (logging-strategy.md:184-195). DisplaySafeUrl directly supports this. +- **`std::env::set_var`/`remove_var` clippy-banned workspace-wide** (server-secrets-strategy.md). Registry must be name-only; tests use `Env` trait stubs, not process env. One documented exception: `fabro-cli/src/main.rs:82-94` scrubs `FABRO_WORKER_TOKEN` with `#[expect]`. +- **Subprocess env uses `env_clear()` + explicit allowlist** (`lib/crates/fabro-server/src/spawn_env.rs`). If Phase 1 also exposes "known env vars" iteration, subprocess spawn could consume that allowlist — deferred to Phase 6 or a follow-up. +- **Insta workflow requires `cargo insta pending-snapshots` before `accept`** (AGENTS.md:129-137). Never bulk-accept. + +No existing learnings on miette, cargo-dev tooling, or CLI docs drift — those are greenfield. + +### External References + +- `ref-cast` crate (already a workspace dep candidate; check `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-redacted/Cargo.toml`) — used for zero-cost reference conversion. +- `miette` (https://crates.io/crates/miette) — `Diagnostic` trait, `set_hook` for `main`-level rendering, `IntoDiagnostic` to bridge from other error types. + +## Key Technical Decisions + +- **New `fabro-static` crate rather than adding `EnvVars` to `fabro-util`.** Rationale: `fabro-util` is a grab-bag; `fabro-static` mirrors uv's narrow-purpose crate and reads as a first-class convention. Leaf crates can depend on `fabro-static` without pulling in `fabro-util`'s broader surface. +- **New `fabro-redacted` crate rather than adding `DisplaySafeUrl` to `fabro-http`.** Rationale: the redaction adapter should be usable from lower-level/internal crates without depending on `reqwest`/`fabro-http`. `fabro-http` sits above `fabro-github`/`fabro-oauth`/`fabro-llm` in the dep graph, so adding it there would work for some logging-path migrations but would block lower-level crates from using the type. Public API DTOs remain raw `String` / `url::Url` unless a separate DTO audit says otherwise. Matches uv's `uv-redacted`. +- **`Debug` delegates to `Display` (fabro diverges from uv).** uv's `Debug` impl is a `debug_struct` with separate `scheme`/`username`/`password`/`host`/`port`/`path`/`query`/`fragment` fields — redacting only username/password and leaving query/path/fragment raw. That is unsafe for fabro because our token-bearing URLs put tokens in query strings (`?token=...`), not userinfo. Fabro's `Display` redacts both userinfo and query-string keys from an allowlist; `Debug` delegates to `Display` so `?url` in `tracing::debug!(?url, ...)` does not leak tokens. This is the single most important design choice in Phase 2; get it wrong and the whole phase is security theater. +- **`DisplaySafeUrl` redacts query-string keys from an allowlist, not just userinfo.** uv's implementation only redacts the password in the URL authority. Fabro needs more: install tokens, CSRF `state` tokens, API keys, auth codes, and access tokens all ride in query strings. The allowlist at minimum: `token`, `install_token`, `access_token`, `refresh_token`, `api_key`, `apikey`, `code`, `state`, `password`, `secret`, `key`. Allowlist lives in `fabro-redacted` and is documented in `docs-internal/logging-strategy.md`. +- **Serialization must be redacted when it exists, but a blanket impl is optional.** The "display-only" redaction story is insufficient for DTO/output paths: if a JSON response contains a redacted URL type, the response body must render redacted. However, implementing `Serialize` directly on `DisplaySafeUrl` can silently persist `****` if the type leaks into config/cache/storage structs. Unit 2.1 must choose between (a) a blanket redacted `Serialize` impl plus strong persistence bans/tests, or (b) a separate output-only wrapper for JSON/schema boundaries. In both designs, no serializable redacted URL type ever serializes raw credentials. +- **Scope of Phase 2 migration is narrowed to credential-bearing paths only.** The research surfaced ~15 URL-logging sites; many of them (`fabro-server/src/install.rs:1454`, `github_webhooks.rs:62`, `serve.rs:331`, `canonical_origin.rs:2`) log URLs that never carry credentials. Migrating them to `DisplaySafeUrl` is churn without security benefit. Phase 2 covers only sites that construct, log, or serialize URLs that can carry tokens; non-credentialed URL handling stays on `url::Url`. +- **`DisplaySafeUrl` is a display-layer adapter, not a URL type.** The type exists at exactly three kinds of boundaries: + - **Logging fields** — `tracing::debug!(?url, ...)`, `tracing::info!(url = %url, ...)`. + - **Error formatting** — `format!("request to {url} failed")` inside `anyhow::Error::context`, return types of fallible URL-producing helpers like `embed_token_in_url`. + - **Server-internal debug output** — stderr lines during the install flow that are visible to operators but intended for human-readable diagnostics only. + + `DisplaySafeUrl` is **banned** everywhere the URL must travel outward: + - **HTTP `Location:` headers and response bodies** — use raw `url::Url` or `String`. The redirect URLs at `lib/crates/fabro-server/src/install.rs:1227,1546` emit `Location:` headers to the browser, which must follow them to claim the install token; the credential transit is the point. These sites keep `String`. + - **Install-flow stderr prompts that the user clicks/copies** — `lib/crates/fabro-cli/src/commands/server/mod.rs:266-270` prints the install URL for the user. The raw token must be visible. These sites keep `String`. The security concern is any *tracing* call logging the same URL; tracing emitters nearby use `DisplaySafeUrl`. + - **`reqwest::Request` URL fields** — use `url.as_raw_url()` or plain `url::Url`. + - **Subprocess arguments (shell-quoted or otherwise)** — use `url.as_raw_url().as_str()`. Accidentally passing the `Display` form to `shell_quote` produces a broken command and a confusing auth failure. + - **Persistence** (TOML/JSON config files, SQLite cache, work-queue messages, insta snapshots of errors) — use `url::Url` directly. If a persisted struct field legitimately needs credential-redaction semantics on one read path AND raw round-trip on another, introduce a `RawUrl` newtype or a separate output-only redaction wrapper rather than reusing a display adapter in persistent data. + + **Neutralizing the `.to_string()` trap.** `DisplaySafeUrl` provides two explicit string conversion methods — `url.redacted_string() -> String` and `url.raw_string() -> String` — and implements `Display` (which matches `redacted_string`). `ToString` is derived from `Display` as usual, so `url.to_string()` returns the *redacted* form. This is the safe default but it IS a trap for callers who want the raw URL and reflexively reach for `.to_string()`. Mitigation: (1) the `disallowed-types` ban in credential-handling crates means any site that can see a raw URL must be explicit about which form it wants; (2) a clippy `disallowed-methods` entry for `::to_string` with a message pointing at the two explicit methods — callers who want the redacted form use `.redacted_string()` for self-documenting code, callers who want the raw form use `.raw_string()` or `.as_raw_url()`; (3) tests in the shell-quote / reqwest / Location-header paths assert the raw token survives (see Unit 2.2 test scenarios). +- **Option-b miette integration: wrap anyhow in main, don't swap everywhere.** Rationale: 14 crates use `anyhow::Result`; swapping is high-churn. A `miette::Diagnostic`-implementing newtype in `fabro-cli/src/main.rs` wrapping `anyhow::Error` preserves all existing context-chaining while handing final rendering to miette's styled reporter. `source_code()` and `labels()` return `None` in this phase, so source highlighting is not promised. Existing `exit::exit_class_for(&err)` auth-hint logic keeps working because the wrapper holds the original `anyhow::Error`. +- **Lift `fabro_json_snapshot!` into `fabro-test` rather than build a new uv-parity value macro from scratch.** Rationale: the existing crate-local macro in `fabro-cli/tests/it/support/mod.rs:8-46` already encodes the shape tests want; promoting it is cheaper than designing fresh and loses no coverage. +- **`fabro-dev` as a new binary crate under `lib/crates/`.** Rationale: consistency with workspace layout. `cargo dev ` alias routes via `.cargo/config.toml`. Shell scripts are replaced one-for-one initially (same flags, same behavior), then optionally reshaped. +- **`OptionsMetadata` lands last and is split like uv.** Rationale: the runtime metadata model is a normal crate (`fabro-options-metadata`) with `OptionsMetadata`, `Visit`, `OptionField`, and `OptionSet`; `fabro-macros` only generates impls for that trait. The proc macro is the biggest surface and depends on clap/settings structs being stable. Generating docs from it also needs the `fabro-dev` host crate to exist. Keeping it last also means cli.mdx drift catches up in one PR rather than churning twice. +- **Clippy enforcement: workspace-wide bans with crate-level opt-outs.** `clippy.toml` is workspace-global — there is no per-crate include/exclude mechanism. Each phase ships bans with this model: + - **Workspace-wide ban** in `clippy.toml` (with `allow-invalid = true` so the ban survives even when the facade crate isn't in scope). + - **Crate-level `#![allow(...)]`** only at the root of crates whose entire purpose legitimately owns/facades the banned symbol (`fabro-static`, `fabro-redacted`, `fabro-http`, test-support crates). Mixed crates that contain both sensitive and non-sensitive paths (especially `fabro-server`) do **not** get root-level allows; they use module-level allows or narrow `#[expect]` at the raw-use site. + - **`#[expect(..., reason = "...")]`** at individual call sites within credential-handling crates where a raw reference is unavoidable. + - Phase 1 adds `disallowed-methods` for `std::env::var`/`var_os` workspace-wide. `fabro-static` and every `build.rs` carry `#![allow(clippy::disallowed_methods)]`; dynamic resolver/facade paths such as `fabro-util::env::SystemEnv`, interpolation closures, and subprocess env allowlists carry narrow `#[expect]` or module-level allowances with reasons. + - Phase 2 adds `disallowed-types` for raw `url::Url`/`reqwest::Url` workspace-wide once migration is complete. `fabro-redacted` (owner), `fabro-http` (reqwest facade), and test-only support can use crate-level allowances; mixed production crates use module/call-site allowances so credential-bearing modules remain protected. Phase 2 also adds a workspace-wide `disallowed-macros` or regex-CI check against inline `format!("https://{}:{}@{}", ...)` construction (no legitimate callers). + - The purpose of the bans is regression prevention, not migration gating. If a crate accumulates many `#[expect]`s, that's a signal to keep migrating — not to bake the exceptions in. + +## Open Questions + +### Resolved During Planning + +- **Where does `DisplaySafeUrl` live?** → New `fabro-redacted` crate (see Key Decisions — dep-graph rationale). +- **Where does `EnvVars` live?** → New `fabro-static` crate. +- **Where does `OptionsMetadata` runtime metadata live?** → New `fabro-options-metadata` crate. `fabro-macros` only owns the derive macro and emits impls against that normal crate. +- **Miette full swap or wrap?** → Wrap-in-main (option b). +- **Does fabro already have a snapshot-filter macro?** → Yes, `fabro_snapshot!` in `fabro-test`. Phase 3 adds a JSON/value variant, not a greenfield macro. +- **Does `Debug` delegate to `Display`, matching uv?** → No. uv's `Debug` leaves query/path raw; fabro delegates `Debug` to `Display`. Query-string tokens are redacted in both. This is a deliberate divergence documented in Key Technical Decisions. +- **Does `DisplaySafeUrl` redact query-string keys?** → Yes, from an allowlist maintained in `fabro-redacted`. uv's impl does not. +- **Does a serializable redacted URL produce the raw URL (uv parity) or the redacted form?** → Redacted form. Whether that is a blanket `Serialize` impl on `DisplaySafeUrl` or a separate output-only wrapper is deferred to Unit 2.1; either way, callers needing raw-on-wire data use `.as_raw_url()` / `.raw_string()` / `Deref`, not serialization. +- **Do non-credentialed URL logging sites migrate to `DisplaySafeUrl`?** → No. Migration scope is narrowed to credential-bearing paths. Sites like `fabro-server/src/install.rs:1454` (public webhook URL logging) stay on `url::Url`. +- **Do the three existing partial env var consts get per-crate decisions?** → No. Two have cross-crate importers but migration is uniform. Delete all three; update importers to reference `fabro_static::EnvVars::*` directly. +- **Does test code migrate to `EnvVars`?** → Yes. Test-only env vars (`FABRO_TEST_MODE`, `NEXTEST_*`) live in `EnvVars` alongside production ones. +- **What's the naming convention for the registry constants?** → Const identifier equals env var value verbatim, no `_ENV` or `_VAR` suffix. Matches uv and the majority of the 42 literals in the current codebase. +- **Should we extend `disallowed-methods` to ban raw `url::Url` / `env::var`?** → Yes, with each phase (not deferred). Use `allow-invalid = true` and `#[expect(...)]` on the migration tail. See updated Key Technical Decisions. +- **Should the registry expose an iteration API for subprocess allowlisting?** → Not required for R1-R7. Leave as a follow-up if `spawn_env.rs` wants to consume it. +- **Do any public API DTOs carry credentialed URLs today?** → Research surfaced none. `web_auth.rs` DTOs use non-credentialed `avatar_url`/`user_url` strings. Rule established: `DisplaySafeUrl` is for server-internal types; public-API DTOs keep `Url`/`String`. + +### Deferred to Implementation + +- **Exact shape of `OptionsMetadata` derive attributes** — uv uses `#[option]`, `#[option_group]`, `#[doc]`. Fabro's `args.rs` doc-comment idioms may need minor normalization; discover during Phase 6. +- **Whether `DisplaySafeUrl` should implement general `Serialize` or expose an output-only wrapper** — the current default is redacted serialization, but implementation must re-evaluate the data-loss risk before adding a blanket `Serialize` impl. If the risk feels too high, prefer a separate `RedactedUrlForDisplay`/`SerializableRedactedUrl` wrapper and keep `DisplaySafeUrl` display/debug-only. +- **Windows parity for `cargo dev docker-build`** — the script uses docker + cargo-zigbuild. Rust rewrite may still shell out. Decide during Phase 5. +- **Whether `cargo dev check-boundary` should promote the temporary-exemption marker mechanism to a Rust-typed list** — likely yes, but implementation can discover the right shape. +- **miette source-highlighting reach** — deferred follow-up. Phase 4 intentionally sets `source_code()`/`labels()` to `None`; span-aware diagnostics should wait until a concrete fabro error type carries source context. +- **Whether `cargo dev generate-cli-reference` replaces `docs/reference/cli.mdx` in full or only the table-shaped portions** — depends on what `OptionsMetadata` can extract cleanly. Decide during Phase 6. + +## High-Level Technical Design + +> *This illustrates the intended approach and is directional guidance for review, not implementation specification. The implementing agent should treat it as context, not code to reproduce.* + +``` +lib/crates/ +├── fabro-static/ (NEW — Phase 1) +│ └── EnvVars struct, all FABRO_* and upstream constants +├── fabro-redacted/ (NEW — Phase 2) +│ └── DisplaySafeUrl, DisplaySafeUrlError, ref-cast impls +├── fabro-test/ (MODIFIED — Phase 3) +│ └── add fabro_json_snapshot! + JSON filter set +├── fabro-cli/src/main.rs (MODIFIED — Phase 4) +│ └── MietteAnyhow wrapper, set_hook, preserve exit_class_for + ├── fabro-dev/ (NEW — Phase 5) + │ ├── main.rs (clap entrypoint) + │ └── commands/{check_boundary, docker_build, release, refresh_spa, check_spa_budgets}.rs + ├── fabro-options-metadata/ (NEW — Phase 6) + │ └── OptionsMetadata trait, Visit, OptionField, OptionSet + ├── fabro-macros/src/lib.rs (MODIFIED — Phase 6) + │ └── #[derive(OptionsMetadata)] emitting impls against fabro-options-metadata + └── fabro-dev/src/commands/ (MODIFIED — Phase 6) + └── generate_cli_reference.rs, generate_options_reference.rs + +.cargo/config.toml (MODIFIED — Phase 5) +└── + alias dev = "run -p fabro-dev --" + +docs/reference/cli.mdx (REGENERATED — Phase 6) +``` + +Dependency graph between phases (TB): + +```mermaid +flowchart TB + P1[Phase 1: fabro-static] --> P6[Phase 6: OptionsMetadata + generator] + P2[Phase 2: fabro-redacted] -.independent.-> P1 + P3[Phase 3: snapshot helpers] -.independent.-> P1 + P4[Phase 4: miette wrap] -.independent.-> P1 + P5[Phase 5: fabro-dev] --> P6 + P1 -.registry enables gen.-> P6 +``` + +Solid arrows are real dependencies. Dashed lines show phases that are independent and can interleave. + +## Implementation Units + +### Phase 1 — fabro-static env var registry + +- [x] **Unit 1.1: Create `fabro-static` crate with `EnvVars` struct** + +**Goal:** Ship a new leaf crate exposing all fabro + upstream env var names as `pub const` fields. + +**Requirements:** R1, R7. + +**Dependencies:** None. + +**Files:** +- Create: `lib/crates/fabro-static/Cargo.toml` +- Create: `lib/crates/fabro-static/src/lib.rs` +- Create: `lib/crates/fabro-static/src/env_vars.rs` +- Modify: `Cargo.toml` (workspace members, add `fabro-static`) +- Test: `lib/crates/fabro-static/src/env_vars.rs` (inline unit test asserting `EnvVars::FABRO_CONFIG == "FABRO_CONFIG"`) + +**Approach:** +- Mirror `uv-static/src/env_vars.rs` shape: single `struct EnvVars;` with `impl EnvVars { pub const FOO: &'static str = "FOO"; }` per name. +- Include every one of the 42 literals found in research (FABRO_*, provider keys, OAUTH_*, upstream like `HOME`/`CI`/`PATH`). +- Group by domain in source order with section comments (`// Fabro core`, `// LLM providers`, `// OAuth`, `// Upstream`). +- No doc-comment enforcement macro yet — that arrives in Phase 6. Consts should still carry brief doc comments where the purpose isn't obvious. + +**Patterns to follow:** +- uv: `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-static/src/env_vars.rs` +- fabro existing: partial consts in `fabro-config/src/user.rs:14`, `fabro-util/src/browser.rs:5`, `fabro-http/src/lib.rs:22`. + +**Test scenarios:** +- Happy path: `EnvVars::FABRO_CONFIG` compiles and equals `"FABRO_CONFIG"`. +- Happy path: all const values are non-empty and contain no whitespace. +- Test expectation: minimal — this unit is structural. Broader validation happens in 1.2 via compiling call sites. + +**Verification:** +- `cargo build -p fabro-static` succeeds. +- `cargo test -p fabro-static` passes. + +--- + +- [x] **Unit 1.2: Migrate fixed env var names and classify dynamic reads** + +**Goal:** Replace every fixed string-literal env var name in production code with a reference to `EnvVars`, and explicitly document the remaining dynamic env lookup facades that cannot name a single constant. + +**Requirements:** R1. + +**Dependencies:** Unit 1.1. + +**Files:** +- Modify: `lib/crates/fabro-llm/src/client.rs` (13 sites) +- Modify: `lib/crates/fabro-server/src/server.rs` (6 sites) +- Modify: `lib/crates/fabro-server/src/install.rs` (4 sites) +- Modify: `lib/crates/fabro-cli/src/server_client.rs` (3 sites) +- Modify: `lib/crates/fabro-test/src/lib.rs` (6 sites) +- Modify: `lib/crates/fabro-workflow/tests/it/daytona_integration.rs` (4 sites) +- Modify: all remaining crates with `env::var(` calls (see research inventory), either to use `EnvVars` for known names or to add a narrow `#[expect]` / module allowance for dynamic resolver/facade paths. +- Modify: `lib/crates/fabro-cli/src/args.rs` (7 sites — clap `#[arg(env = EnvVars::FOO)]`) +- Modify: `lib/crates/fabro-config/src/user.rs:14`, `lib/crates/fabro-util/src/browser.rs:5`, `lib/crates/fabro-http/src/lib.rs:22` — delete local constants, re-export or reference `EnvVars`. +- Test: existing tests — no new tests; regression covered by compile + test suite. + +**Approach:** +- Use repo-wide grep (`env::var\("`, `env::var_os\("`) to enumerate, then migrate crate-by-crate within one PR. +- For clap: `#[arg(env = EnvVars::FOO)]` — clap derive accepts const expressions. +- For build scripts: no migration needed. All five `build.rs` files (fabro-api, fabro-cli, fabro-proc, fabro-server, fabro-util) only read cargo-injected vars (`PROFILE`, `CARGO_CFG_TARGET_OS`, `OUT_DIR`, `CARGO_MANIFEST_DIR`). None reference `FABRO_*` names. +- For the three existing partial registries: **delete all three**. `fabro_config::user::FABRO_CONFIG_ENV` is imported by 3 files in `fabro-cli`; `fabro_util::browser::SUPPRESS_ENV_VAR` is referenced from `fabro-test/src/lib.rs:171`; `fabro_http::HTTP_PROXY_POLICY_ENV` is single-crate. All three migrate uniformly in this PR to `fabro_static::EnvVars::*`; no `pub use` re-export needed. +- Test code migrates too. `fabro-test/src/lib.rs` and integration tests use `EnvVars` just like production code. Test-only env vars (`FABRO_TEST_MODE`, `NEXTEST_*`) live in `EnvVars` alongside production ones. +- **Naming convention**: Const identifier equals env var value verbatim; no `_ENV`/`_VAR` suffix. The three legacy consts (`FABRO_CONFIG_ENV`, `SUPPRESS_ENV_VAR`, `HTTP_PROXY_POLICY_ENV`) are inconsistent — the new registry is uniform. Matches uv. +- **Dynamic lookup classification**: raw env reads whose name is not known statically are not converted to fake constants. Examples include `fabro-util::env::SystemEnv`, settings interpolation closures (`resolve(|name| std::env::var(name).ok())`), vault/env fallback helpers, and subprocess env allowlists. Each gets either a narrow `#[expect(clippy::disallowed_methods, reason = "...")]` or a small module-level allowance explaining that the site is an env lookup facade, not a place where new env var names should be introduced. +- **Add clippy lint with the migration**: `clippy.toml` gains a workspace-wide `disallowed-methods` entry for `std::env::var` and `std::env::var_os` (`allow-invalid = true`). `fabro-static/src/lib.rs` carries `#![allow(clippy::disallowed_methods)]`; every `build.rs` does the same. Dynamic facades and test-only call sites that legitimately need raw env reads carry narrow `#[expect(...)]` / module-level allowances. The lint's purpose is regression prevention for future PRs, not a migration gate. + +**Execution note:** Start with `fabro-llm` (highest density, bounded scope) as the proof-of-pattern, then sweep remaining crates. + +**Patterns to follow:** +- uv: `#[arg(env = EnvVars::UV_CACHE_DIR)]` in `crates/uv-cache/src/cli.rs:30`. +- fabro existing: see Unit 1.1 partial-const list. + +**Test scenarios:** +- Happy path: `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` passes. +- Happy path: `cargo nextest run --workspace` passes. +- Happy path: binary behavior unchanged — a pre-migration snapshot of `fabro --help` output matches post-migration. +- Edge case: build scripts still compile (they keep their literals). +- Edge case: dynamic env lookup facades still compile with documented lint expectations and no new fixed-name literals. +- Integration: the three old partial-const crates still compile (the re-export or delete worked). + +**Verification:** +- `rg 'env::var(_os)?\("FABRO_' lib/crates/ | grep -v 'fabro-static\|build.rs' | wc -l` returns 0. +- `rg 'env::var(_os)?\("' lib/crates/ | grep -v 'fabro-static\|build.rs'` shows only documented dynamic facades or test/support exceptions with lint expectations. +- All workspace tests pass. + +--- + +### Phase 2 — fabro-redacted DisplaySafeUrl + +- [ ] **Unit 2.1: Create `fabro-redacted` crate with `DisplaySafeUrl` newtype** + +**Goal:** Ship a new leaf crate providing a credential-redacting `Url` wrapper. + +**Requirements:** R2, R7. + +**Dependencies:** None (independent of Phase 1; can interleave). + +**Files:** +- Create: `lib/crates/fabro-redacted/Cargo.toml` +- Create: `lib/crates/fabro-redacted/src/lib.rs` +- Modify: `Cargo.toml` (workspace members; add `ref-cast` and `url` to workspace deps if not present — `url` is currently only in `fabro-server/Cargo.toml`, not in the workspace table) +- Test: `lib/crates/fabro-redacted/src/lib.rs` (inline unit tests mirroring uv's) + +**Approach:** +- Use `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-redacted/src/lib.rs` as a starting template — structure, `#[repr(transparent)]`, `RefCast`, `DisplaySafeUrlError::AmbiguousAuthority`, `has_credential_like_pattern`. But fabro **diverges in three material ways** (see Key Technical Decisions): + 1. **`Debug` delegates to `Display`.** uv's `Debug` is a `debug_struct` that leaves query/path raw. Fabro's `Debug` prints the `Display`-rendered redacted form. A `tracing::debug!(?url, ...)` on a `DisplaySafeUrl` must never emit a token. + 2. **`Display` redacts query-string keys from an allowlist**, not just the userinfo password. Baseline allowlist: `token`, `install_token`, `access_token`, `refresh_token`, `api_key`, `apikey`, `code`, `state`, `password`, `secret`, `key`. Constructed via a `HashSet<&'static str>` initialized at module load (or `phf_set!`). Every matching query key becomes `****`; other keys render verbatim. + 3. **Serialization is a deliberate decision, not automatic uv parity.** The planned default is that any serializable redacted URL renders redacted, not raw, but a blanket `Serialize` impl on `DisplaySafeUrl` can cause data loss if the type leaks into persistence. Before adding that impl, evaluate whether a separate output-only wrapper (`SerializableRedactedUrl` / `RedactedUrlForDisplay`) is cleaner. `Deserialize` of raw URLs is only needed if the type is intentionally accepted at a boundary; otherwise keep deserialization off the display adapter too. +- `FromStr`, `Deref`, `DerefMut`; `Serialize`/`Deserialize` only if the implementation chooses the blanket redacted-serialization route after the data-loss check above. Optional `schemars` feature parity — `schemars(transparent)` so OpenAPI/JSON-schema consumers see a plain URL string, but only for an explicitly serializable redacted-output type. +- **Explicit conversion methods**: `fn redacted_string(&self) -> String` (same output as `Display`) and `fn raw_string(&self) -> String` (same output as `self.as_raw_url().to_string()`). These exist specifically so call sites name the form they want. `as_raw_url(&self) -> &url::Url` exposes the inner URL for reqwest/shell-quote/persistence. +- **Eq/PartialEq/Hash/Ord** are derived on the raw inner `Url` (uv parity). Two `DisplaySafeUrl` values with different credentials produce distinct hashes and comparisons — necessary for `HashMap` caches of authenticated remotes. Never implement these against the redacted form. +- Document the divergences from uv (Debug delegates to Display, query-string keys are redacted from an allowlist, and any serializable redacted-output path serializes redacted rather than raw) at the top of `lib.rs` with a `// Diverges from uv: ...` comment block and in the crate-level rustdoc. + +**Execution note:** Mirror uv's test cases for the shared behavior, then add fabro-specific tests for the three divergences. Before coding, write an integration test in a throwaway fixture that captures `tracing` output from `debug!(?url, ...)` for a token-bearing URL — it must be empty of the token. This is the security invariant the whole crate exists to enforce. + +**Patterns to follow:** +- uv `uv-redacted` crate (primary reference). + +**Test scenarios:** +- Happy path (userinfo): `DisplaySafeUrl::parse("https://user:secret@example.com")` renders `Display` as `https://user:****@example.com/`. +- Happy path (plain): `DisplaySafeUrl::parse("https://example.com/path")` renders identically to the inner `Url`. +- Happy path (Debug=Display): `format!("{:?}", url)` matches `format!("{}", url)` for any input. +- Happy path: `username()` and `password()` still return the raw values; internal code has access. +- Happy path: `set_username` / `set_password` / `remove_credentials` work on the inner `Url`. +- Happy path (query-string redact, single key): `https://example.com/install?token=ghs_abc` renders as `https://example.com/install?token=****`. +- Happy path (query-string redact, multiple keys): `https://example.com/cb?code=X&state=Y&keep=Z` renders with `code` and `state` masked, `keep` preserved. +- Happy path (query-string redact, case-insensitive): `?Token=abc` and `?API_KEY=xyz` both redact. +- Happy path (raw access): `url.as_raw_url().to_string()` or `(&*url).to_string()` returns the unredacted URL. +- Happy path (serde, if blanket serialization is chosen): `serde_json::from_str::("\"https://user:pw@host/\"")` succeeds; re-serializing produces the redacted form. +- Edge case: nested proxy URL `git+https://proxy.com/https://user:pw@github.com/repo` — credentials in inner URL are handled. +- Edge case: URL with no password (`https://user@example.com/`) — username shown, no `:****`. +- Edge case: URL with only password (`https://:secret@example.com/`) — password masked. +- Edge case: URL with a query key that's a prefix of an allowlist entry (`?tokenish=val`) — NOT redacted (exact match only). +- Edge case: empty query string — renders without `?`. +- Error path: ambiguous authority `https://user/name:pw@host` returns `DisplaySafeUrlError::AmbiguousAuthority`. +- Integration (tracing): `debug!(?url, ...)` on a token-bearing URL produces a tracing event whose formatted output contains `****` and does NOT contain the token substring. +- Integration (JSON DTO, if blanket serialization is chosen): a struct `#[derive(Serialize)] { url: DisplaySafeUrl }` with a token-bearing URL, when serialized via `serde_json::to_string`, produces JSON where the token is replaced by `****`. +- Integration (schemars, if a serializable redacted-output type exists): `schemars::schema_for!(DisplaySafeUrl)` or the output wrapper produces a schema with `type: string, format: uri` (transparent). + +**Verification:** +- `cargo test -p fabro-redacted` passes. +- `cargo doc -p fabro-redacted` produces docs without warnings. + +--- + +- [ ] **Unit 2.2: Migrate credential-bearing URL paths to `DisplaySafeUrl`** + +**Goal:** Convert URL construction, logging, and serialization paths that can carry credentials to `DisplaySafeUrl`. Non-credentialed URL handling stays on `url::Url`. + +**Requirements:** R2. + +**Dependencies:** Unit 2.1. + +**Files (credential-bearing — IN scope):** +- Modify: `lib/crates/fabro-github/src/lib.rs:916-935` — `embed_token_in_url` returns `DisplaySafeUrl` (was `String`). Callers update (see shell-quote trap below). +- Modify: `lib/crates/fabro-github/src/lib.rs:250,259,263,1442` — `format!("...{url}")` in error messages on token-embedded URLs. +- Modify: `lib/crates/fabro-sandbox/src/daytona/mod.rs:610-614` — **inline token URL construction** (`https://x-access-token:{token}@...`). Replace the `format!` with `DisplaySafeUrl::embed_token(...)` (new helper on `fabro-redacted` or a `fabro-github` helper). Critical: this site bypasses `embed_token_in_url` today, so a `DisplaySafeUrl`-returning `embed_token_in_url` alone does not fix it. +- Modify: `lib/crates/fabro-sandbox/src/daytona/mod.rs:816-826`, `lib/crates/fabro-workflow/src/sandbox_git.rs:153-174` — `resolve_authenticated_url` callers. **Shell-quote trap**: current code calls `shell_quote(&auth_url)` on a `&str`. After the migration, the correct call is `shell_quote(auth_url.as_raw_url().as_str())`, NOT `shell_quote(&auth_url.to_string())` — the latter would shell-quote the `****`-redacted form and break git. Document this trap in the unit comments. +- Modify: `lib/crates/fabro-oauth/src/lib.rs:324,362,374,403,825` — OAuth token URLs and `format!("...{url}")` errors. +- Modify: `lib/crates/fabro-server/src/web_auth.rs:398,419,547` — OAuth `redirect_uri` logging. Note: the `state` query param (CSRF token) is covered by Phase 2.1's query-string allowlist, so migration here is a matter of wrapping the URL type, not custom redaction. +- Modify: `lib/crates/fabro-llm/src/providers/fabro_server.rs:122,157` — provider URLs may carry auth query params; if the base URL can contain credentials, logging uses a redacted wrapper while request construction keeps the raw string/URL. +- Modify: `lib/crates/fabro-oauth/src/lib.rs` (etc.) — the credential-bearing sites above. + +**Files (explicitly kept as raw `String`/`url::Url` — credential transit is the point):** +- `lib/crates/fabro-server/src/install.rs:1227,1546` — install token URLs emitted in HTTP `Location:` headers during browser redirects. The browser must follow the redirect to claim the install token. `DisplaySafeUrl` is banned from `Location` values (see Key Technical Decisions). These sites stay raw. Add a tracing test that asserts the token is NOT logged by any nearby `tracing::` call when these redirects are constructed (the concern is logging, not transport). +- `lib/crates/fabro-cli/src/commands/server/mod.rs:266-270` — install token URLs printed to stderr for the user to copy/click. The user must see the raw token to claim the install. These sites stay raw. If any nearby `tracing::` call emits the same URL, that tracing call uses `DisplaySafeUrl` — the print-to-stderr stays as-is. + +**Files (explicitly OUT of scope — non-credentialed):** +- `lib/crates/fabro-server/src/install.rs:1454,1461` — `restart_url` (no credentials). +- `lib/crates/fabro-server/src/github_webhooks.rs:62,67` — webhook URL, Tailscale funnel URL (no credentials). +- `lib/crates/fabro-server/src/serve.rs:331` — canonical server URL (no credentials). +- `lib/crates/fabro-server/src/canonical_origin.rs:2`, `lib/crates/fabro-server/src/auth/cli_flow.rs:731,734,744` — direct `url::Url` use on non-credential paths. +- `lib/crates/fabro-server/src/web_auth.rs` DTO fields `avatar_url`/`user_url` — public API surface; keep `String`. + +**Out-of-scope but documented as Phase 2 non-goals** (separate follow-up if warranted): +- `lib/crates/fabro-client/src/client.rs:1304` — `HeaderValue::from_str(&format!("Bearer {token}"))`. Not URL-bearing; same risk class. Mention in plan's Scope Boundaries so readers don't assume Phase 2 covers auth headers. + +**Test files:** +- Add: `lib/crates/fabro-github/tests/it/redaction.rs` — tracing-capture integration test; assert no token substring in any event after calling `embed_token_in_url` + logging through it. +- Add: similar redaction integration tests in `fabro-oauth/tests/it/`, `fabro-server/tests/it/` (install flow). +- Update: any existing tests that assert on raw URL string contents from these call sites. + +**Approach:** +- Priority order: (1) `fabro-github::embed_token_in_url` return-type change, (2) the inline `daytona/mod.rs:610` construction, (3) OAuth token-URL logging, (4) install token URLs, (5) LLM provider URLs. +- For function signatures returning token-bearing URLs: return `DisplaySafeUrl`. Callers that need the raw value for reqwest or git operations use `.as_raw_url().as_str()` (or `Deref`). +- For `format!("... {url}")` in error messages on token-bearing URLs: `Display` on `DisplaySafeUrl` redacts — use directly. +- For tracing on token-bearing URLs: both `%url` and `?url` are safe once the type is `DisplaySafeUrl` (Debug delegates to Display per Phase 2.1). +- Do not migrate build scripts or test-only URL construction that never logs or serializes. + +**Execution note:** Start with `fabro-github::embed_token_in_url` + the `daytona/mod.rs:610` inline site together — they both produce `x-access-token:...@` URLs, and fixing only one leaves the other as a silent bypass. + +**Patterns to follow:** +- uv usage sites (grep `DisplaySafeUrl::` in `/Users/bhelmkamp/p/astral-sh/uv/crates/`). +- fabro's existing `fabro-http::Url` re-export pattern for wrapping. + +**Test scenarios:** +- Happy path (userinfo token): `embed_token_in_url("https://github.com/org/repo", "ghs_abc")` returns a `DisplaySafeUrl` whose `Display` shows `https://x-access-token:****@github.com/org/repo`. +- Happy path (raw access): `.as_raw_url().as_str()` on the same value returns `https://x-access-token:ghs_abc@github.com/org/repo` — used by shell-quote and git-remote paths. +- Happy path (query token): an install URL `https://example.com/install?token=xyz` wrapped as `DisplaySafeUrl` displays with `token=****`. +- Happy path (sandbox inline): the direct construction at `daytona/mod.rs:610` now goes through a `DisplaySafeUrl`-returning helper; `.as_raw_url().as_str()` flows into `shell_quote`. +- Error path (tracing capture): integration test using `tracing-test` or `tracing-subscriber::fmt` to a buffer captures events from a code path that constructs a token-bearing URL and emits `debug!(?url, "...")`. Assert formatted output contains `****` and does NOT contain the token substring. Repeat for `%url`. +- Error path (anyhow chain): an error bubbled via `anyhow::Error::context(format!("request to {url} failed", url = token_url))` renders redacted under `{:?}`. +- Error path (JSON DTO / output wrapper): if a token-bearing URL flows into a response body (not just a Location header), serialization uses the redacted-output type and produces `token=****`. +- Edge case (shell-quote trap): a unit test on the sandbox caller asserts that the string passed to `shell_quote` equals the RAW URL (contains `ghs_abc`), not the redacted form. Prevents regression into the trap described in the unit's Approach. +- Integration (wire-level): a mocked HTTP endpoint receives the full credentialed URL — redaction does not reach the wire when callers use `.as_raw_url()`. + +**Verification:** +- `rg 'format!\("https?://[^"]*:[^"]*@' lib/crates/` returns zero matches (no more inline token URL construction). +- Tracing-capture integration tests in `fabro-github`, `fabro-oauth`, `fabro-server` (install flow) pass and assert no token substring. +- `clippy.toml` gains workspace-wide `disallowed-types` bans on `url::Url` and `reqwest::Url`. `fabro-redacted` (owner), `fabro-http` (reqwest facade), and test-support code can carry crate-level `#![allow(clippy::disallowed_types)]`. Mixed production crates such as `fabro-server` use module-level allowances or per-site `#[expect(clippy::disallowed_types, reason = "...")]` so sensitive modules are still protected. +- Existing `cargo nextest run` passes. + +--- + +### Phase 3 — Expanded snapshot helpers + +- [ ] **Unit 3.1: Lift `fabro_json_snapshot!` into `fabro-test`** + +**Goal:** Promote the crate-local JSON snapshot macro in `fabro-cli/tests/it/support/mod.rs:8-46` to a workspace-wide export in `fabro-test`, with the same baseline filter set as `fabro_snapshot!` plus JSON-specific normalizations. + +**Requirements:** R4. + +**Dependencies:** None. Independent of Phases 1, 2, 4, 5. + +**Files:** +- Modify: `lib/crates/fabro-test/src/lib.rs` (add `fabro_json_snapshot!` macro with `#[macro_export]`; add JSON-specific default filters to `INSTA_FILTERS` or a parallel static). +- Modify: `lib/crates/fabro-cli/tests/it/support/mod.rs` (delete lines 8-46; callers import `fabro_test::fabro_json_snapshot`). +- Test: `lib/crates/fabro-test/src/lib.rs` (inline doc-test showing the macro snapshotting a sample `serde_json::Value`). + +**Approach:** +- Copy the existing macro body; parameterize filter additions using the same `(cmd, additional_filters)` vs `()` arm pattern as `fabro_snapshot!`. +- JSON-specific filters to include (from the existing crate-local copy): timestamp normalization, event id normalization, `duration_ms` normalization, `manifest_blob` / `definition_blob` normalization, `run_dir` normalization, package version normalization. +- Match `fabro_snapshot!`'s invocation shape so call sites converting from the crate-local macro need only the import change. + +**Patterns to follow:** +- Existing `fabro_snapshot!` in `lib/crates/fabro-test/src/lib.rs:1716`. +- Existing `fabro_json_snapshot!` in `lib/crates/fabro-cli/tests/it/support/mod.rs:8-46`. + +**Test scenarios:** +- Happy path: calling `fabro_json_snapshot!(value)` with a `serde_json::Value` containing a timestamp produces a snapshot with the timestamp normalized. +- Happy path: calling with `(value, extra_filters)` adds filters on top of the defaults. +- Edge case: deeply nested JSON with multiple fields that need normalization — all matches replaced. +- Integration: a pre-existing call site in `fabro-cli` tests (e.g., a test that previously used the crate-local macro) produces the same snapshot output after the lift. + +**Verification:** +- `cargo nextest run -p fabro-cli` passes with no pending insta snapshots. +- `rg 'fabro_json_snapshot' lib/crates/` shows only the `fabro-test` definition and import sites — no duplicate definition. + +--- + +### Phase 4 — miette CLI diagnostics wrapper + +- [ ] **Unit 4.1: Wrap `fabro-cli` root error with `miette::Diagnostic`** + +**Goal:** Render CLI errors through `miette` at the `main` boundary, preserving existing exit-class hint behavior while gaining styled chained errors and `help:` footer rendering. Source-highlighted output is out of scope until fabro has errors that carry span/source context. + +**Requirements:** R5. + +**Dependencies:** None. Independent of Phases 1, 2, 3, 5. + +**Files:** +- Modify: `lib/crates/fabro-cli/Cargo.toml` (add `miette = { workspace = true, features = ["fancy"] }`). +- Modify: `Cargo.toml` (add `miette` to workspace deps if not already present). +- Modify: `lib/crates/fabro-cli/src/main.rs` (replace the error chain loop at lines 138-170 with a miette-rendered path; preserve `exit::exit_class_for(&err)` hint footer). +- Test: `lib/crates/fabro-cli/tests/it/` (add or extend a test that runs `fabro` with a known failing command and snapshots the rendered error output via `fabro_snapshot!`). + +**Approach:** +- Define `MietteAnyhow(anyhow::Error)` (or similar newtype) in `fabro-cli/src/main.rs` implementing `miette::Diagnostic`. `Display` delegates to `anyhow::Error::Display`; `source()` walks the anyhow chain so miette renders the full cause list. `source_code` and `labels` return `None` — no fabro error type currently carries span context, and introducing span-aware errors is out of scope for this unit. `help()` returns the auth-hint text when `exit_class_for(&err)` indicates auth is required, otherwise `None`. +- **`main` signature does not change.** `async fn main() -> ()` keeps returning `()` and retains the existing telemetry lifecycle (emit `CLI Errored` event, `fabro_telemetry::shutdown()`, then `std::process::exit(exit_code)`). Miette integration happens *inside* the existing error branch: call `miette::set_hook` early, wrap `err` with `MietteAnyhow`, use `miette`'s renderer directly to produce the string, print to stderr, then fall through to the existing telemetry/exit flow. Do NOT replace `main` with `fn main() -> miette::Result<()>` — that would bypass telemetry shutdown on error paths. +- Preserve `exit::exit_class_for(&err)` behavior: the auth-hint is returned from `MietteAnyhow::help()`, so miette's renderer includes it in the styled output. +- Library crates stay on `anyhow`/`thiserror`. No change below the `main` boundary. + +**Execution note:** Start with a failing integration test that asserts the new rendering shape — it anchors what "done" looks like before the main refactor. + +**Patterns to follow:** +- miette docs: https://docs.rs/miette/latest/miette/ (set_hook, IntoDiagnostic). +- Existing `fabro_util::exit::exit_code_for` — must keep invoking at the same point. + +**Test scenarios:** +- Happy path: a command that fails with a plain `anyhow::Error` still renders with the expected exit code. +- Happy path: auth-required errors still show the cyan "hint: run `fabro auth login`" footer. +- Happy path: the error chain (all `.context(...)` layers) appears in the rendered output, not just the outermost cause. +- Edge case: `SIGINT`-induced error path still renders cleanly. +- Edge case: a future error type that implements `miette::Diagnostic` itself (not via the wrapper) renders with labels/highlights if present. +- Integration: snapshot the rendered error for a known-failing scenario; commit the snapshot; verify CI passes. + +**Verification:** +- `cargo nextest run -p fabro-cli` passes. +- Manual: `fabro ` renders with miette's fancy style; exit code is unchanged. +- `rg 'use miette' lib/crates/` shows usage only in `fabro-cli` (no accidental library spread). + +--- + +### Phase 5 — fabro-dev unified CLI + +- [ ] **Unit 5.1: Create `fabro-dev` binary crate with clap subcommand scaffolding** + +**Goal:** Ship an empty-but-structured `fabro-dev` binary and wire the `cargo dev` alias. + +**Requirements:** R6, R7. + +**Dependencies:** None. Independent of Phases 1, 2, 3, 4. + +**Files:** +- Create: `lib/crates/fabro-dev/Cargo.toml` +- Create: `lib/crates/fabro-dev/src/main.rs` +- Create: `lib/crates/fabro-dev/src/commands/mod.rs` +- Modify: `Cargo.toml` (add workspace member). +- Modify: `.cargo/config.toml` (add `dev = "run --package fabro-dev --"` alias). +- Test: `lib/crates/fabro-dev/tests/it/` (integration test asserting `cargo run -p fabro-dev -- --help` succeeds and lists commands; use `assert_cmd`). + +**Approach:** +- Mirror `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-dev/src/main.rs` structure: `clap::Parser` entrypoint with subcommand dispatch. +- Scaffold empty subcommands (returning "not yet implemented") for what Phase 5 will fill: `check-boundary`, `docker-build`, `release`, `refresh-spa`, `check-spa-budgets`. +- Use `anyhow::Result` internally (CLI boundary; miette wrap is fabro-cli-specific, not needed here). +- Share `tracing-subscriber` setup with `fabro-cli` helpers if feasible; otherwise a small local setup is fine. + +**Patterns to follow:** +- uv: `uv-dev/src/main.rs`, `uv-dev/src/lib.rs`. +- fabro: existing clap derive usage in `fabro-cli/src/args.rs`. + +**Test scenarios:** +- Happy path: `cargo dev --help` (via alias) prints the subcommand list. +- Happy path: `cargo dev check-boundary --help` prints subcommand help. +- Edge case: unknown subcommand exits with clap's error message and exit code 2. +- Test expectation: no business logic yet; smoke-test the clap wiring only. + +**Verification:** +- `cargo build -p fabro-dev` succeeds. +- `cargo dev --help` succeeds (alias resolves). + +--- + +- [ ] **Unit 5.2: Port `check-boundary.sh` to `fabro-dev check-boundary`** + +**Goal:** Replace `bin/dev/check-boundary.sh` with a Rust subcommand that enforces the same allowlist check. + +**Requirements:** R6. + +**Dependencies:** Unit 5.1. + +**Files:** +- Create: `lib/crates/fabro-dev/src/commands/check_boundary.rs` +- Modify: `lib/crates/fabro-dev/src/main.rs` (wire subcommand). +- Delete: `bin/dev/check-boundary.sh` (after CI migration). +- Modify: `AGENTS.md` (reference `cargo dev check-boundary` instead of `bin/dev/check-boundary.sh`). +- Modify: any CI config invoking the old script. +- Test: `lib/crates/fabro-dev/tests/it/check_boundary.rs` (fixture project with allowed and disallowed uses; assert correct exit codes). + +**Approach:** +- Port the hardcoded symbol allowlists from the script into Rust constants (or a `fabro-dev/src/commands/check_boundary/allowlist.rs` data file). +- Use `ignore` crate (workspace dep candidate) or `walkdir` for file traversal; use `regex` for symbol matching. The original script uses `rg -l` then `grep -R` fallback; Rust port can use either crate directly. +- Preserve the temporary-exemption marker mechanism (comments that waive the check for a single file). + +**Execution note:** Port behavior-for-behavior before reshaping. Do not refactor the allowlist semantics in this unit. + +**Patterns to follow:** +- Original script: `bin/dev/check-boundary.sh`. + +**Test scenarios:** +- Happy path: a file using only allowed symbols passes. +- Happy path: a file using a gated symbol with the temporary-exemption marker passes. +- Error path: a file using a gated symbol without the marker returns non-zero exit and names the offending file/line. +- Edge case: empty workspace returns success. +- Edge case: multiple offending files — all are reported, not just the first. +- Integration: running against the current fabro-3 tree produces the same result as the shell script. + +**Verification:** +- `cargo dev check-boundary` on the current fabro-3 tree exits 0. +- Running it against a synthetic "bad" fixture exits non-zero. +- CI that previously ran `bin/dev/check-boundary.sh` now runs `cargo dev check-boundary` and passes. + +--- + +- [ ] **Unit 5.3: Port `docker-build.sh` to `fabro-dev docker-build`** + +**Goal:** Replace `bin/dev/docker-build.sh` with a subcommand. + +**Requirements:** R6. + +**Dependencies:** Unit 5.1. + +**Files:** +- Create: `lib/crates/fabro-dev/src/commands/docker_build.rs` +- Modify: `lib/crates/fabro-dev/src/main.rs`. +- Delete: `bin/dev/docker-build.sh` (after every documented and CI caller has migrated). +- Modify: `AGENTS.md`, release/local Docker docs, and any CI references. +- Test: `lib/crates/fabro-dev/tests/it/docker_build.rs` (smoke-test `--help` and arg parsing only; don't invoke docker in CI). + +**Approach:** +- Shell out to `docker`, `cargo-zigbuild`, and `docker buildx` via `std::process::Command` (or tokio equivalent). This is a script-orchestration task; don't rewrite the container logic in Rust. +- Preserve flags: `--arch {amd64,arm64}`, `--tag ` (default `fabro`), `--compile-only`. +- Preserve named Docker volume caching (cargo-registry, target, zig, cargo-tools). + +**Patterns to follow:** +- Original script: `bin/dev/docker-build.sh`. + +**Test scenarios:** +- Happy path: `cargo dev docker-build --help` prints all three flags. +- Edge case: `--arch invalid` errors cleanly (clap validation). +- Test expectation: integration test for actual docker build is out of scope (CI-unfriendly); verify arg parsing and command construction via a dry-run mode that prints the command instead of executing it. + +**Verification:** +- Manually: `cargo dev docker-build --compile-only` produces a binary staged into `docker-context/amd64/fabro` (matching the old script). +- `cargo dev docker-build --dry-run` prints the equivalent shell command (if dry-run mode added). + +--- + +- [ ] **Unit 5.4: Port `release.sh` to `fabro-dev release`** + +**Goal:** Replace `bin/dev/release.sh` with a subcommand. + +**Requirements:** R6. + +**Dependencies:** Unit 5.1. + +**Files:** +- Create: `lib/crates/fabro-dev/src/commands/release.rs` +- Modify: `lib/crates/fabro-dev/src/main.rs`. +- Delete: `bin/dev/release.sh` (after nightly/release automation has migrated). +- Modify: `AGENTS.md`, `.github/workflows/nightly.yml`, and any release docs that invoke `bin/dev/release.sh`. +- Test: `lib/crates/fabro-dev/tests/it/release.rs` (test the version-computation logic in isolation; smoke-test `--help`). + +**Approach:** +- Port the "days since 2026-01-01" version computation to pure Rust (`chrono` or `time`). +- Preserve prerelease-label validation, `DRY_RUN`, `SKIP_TESTS` env var equivalents (as flags now). +- Shell out to `cargo publish`, `git tag`, etc. where the original did. + +**Patterns to follow:** +- Original script: `bin/dev/release.sh`. + +**Test scenarios:** +- Happy path: version computation returns a valid semver for a known "today" input. +- Happy path: `--dry-run` prints what would happen without running git or cargo. +- Edge case: prerelease label validation rejects invalid labels. +- Edge case: `--skip-tests` flag is honored. +- Error path: a dirty working tree errors unless `--dry-run`. + +**Verification:** +- `cargo dev release --dry-run` produces the same commands the shell script would have. +- Nightly release workflow invokes `cargo dev release nightly` instead of `bin/dev/release.sh nightly`. + +--- + +- [ ] **Unit 5.5: Port `refresh-fabro-spa.sh` and `check-fabro-spa-budgets.sh` to `fabro-dev`** + +**Goal:** Replace the two SPA-related scripts with `cargo dev refresh-spa` and `cargo dev check-spa-budgets`. + +**Requirements:** R6. + +**Dependencies:** Unit 5.1. + +**Files:** +- Create: `lib/crates/fabro-dev/src/commands/refresh_spa.rs` +- Create: `lib/crates/fabro-dev/src/commands/check_spa_budgets.rs` +- Modify: `lib/crates/fabro-dev/src/main.rs`. +- Delete: `scripts/refresh-fabro-spa.sh`, `scripts/check-fabro-spa-budgets.sh` (after AGENTS.md and GitHub Actions callers migrate). +- Modify: `AGENTS.md:24` (update the "run before commit" reference). +- Modify: `.github/workflows/typescript.yml` and `.github/workflows/release.yml` (replace `scripts/refresh-fabro-spa.sh` / `scripts/check-fabro-spa-budgets.sh` invocations). +- Test: `lib/crates/fabro-dev/tests/it/spa.rs` (test the budget-check arithmetic against fixture assets). + +**Approach:** +- `refresh-spa`: `cd apps/fabro-web && bun run build`, then mirror dist/ into `lib/crates/fabro-spa/assets/` with `.map` files removed. Use `std::process::Command` for `bun`; use `walkdir` or similar for the file mirror. +- `check-spa-budgets`: compute total + gzipped size; compare against 15 MiB / 5 MiB thresholds. + +**Patterns to follow:** +- Original scripts: `scripts/refresh-fabro-spa.sh`, `scripts/check-fabro-spa-budgets.sh`. + +**Test scenarios:** +- Happy path (refresh): after running, `lib/crates/fabro-spa/assets/` contains the bun output minus `.map` files. +- Happy path (budgets): a fixture of ~5 MiB total passes; ~20 MiB fails. +- Edge case: missing `apps/fabro-web/dist/` (bun not run) errors cleanly with a pointer to run `bun run build`. +- Edge case: `.map` file accidentally present in assets — budget check warns or fails. + +**Verification:** +- `cargo dev refresh-spa` produces the same directory contents as the old script. +- `cargo dev check-spa-budgets` produces the same pass/fail output. +- `AGENTS.md:24` now says `cargo dev refresh-spa`. +- TypeScript and release workflows call `cargo dev refresh-spa`; TypeScript budget check calls `cargo dev check-spa-budgets`. + +--- + +### Phase 6 — OptionsMetadata and CLI reference generation + +- [ ] **Unit 6.1: Add `fabro-options-metadata` and `#[derive(OptionsMetadata)]`** + +**Goal:** Ship the runtime metadata model plus a proc macro that extracts clap field help, `ValueEnum` possibilities, and option descriptions into that model. + +**Requirements:** R3. + +**Dependencies:** None functionally (Phase 1 not required), but lands after Phase 5 because its primary consumer is a `fabro-dev` generator. + +**Files:** +- Create: `lib/crates/fabro-options-metadata/Cargo.toml` +- Create: `lib/crates/fabro-options-metadata/src/lib.rs` +- Create: `lib/crates/fabro-macros/src/options_metadata.rs` +- Modify: `lib/crates/fabro-macros/src/lib.rs` (export the derive). +- Modify: `Cargo.toml` (workspace members; add `fabro-options-metadata`). +- Test: `lib/crates/fabro-options-metadata/src/lib.rs` (unit tests for `OptionSet` traversal/display/serialization helpers). +- Test: `lib/crates/fabro-macros/tests/options_metadata.rs` (compile-test a struct with the derive; assert metadata shape through `fabro_options_metadata`). + +**Approach:** +- Mirror `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-options-metadata/src/lib.rs` for the normal runtime crate. This crate owns `OptionsMetadata`, `Visit`, `OptionField`, `OptionSet`, grouping, display, and any serde helpers. It has no proc-macro behavior. +- Mirror `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-macros/src/options_metadata.rs` (14KB). It's the biggest new surface in this plan. +- Supported attributes: `#[option]`, `#[option_group]`, `#[doc]`. Walks struct fields, extracts `///` doc-comments, pulls `value_parser` constraints from clap attrs, captures `ValueEnum` variants. +- Emits an impl against the runtime crate, e.g. `impl ::fabro_options_metadata::OptionsMetadata for RunArgs { fn record(visit: &mut dyn ::fabro_options_metadata::Visit) { ... } }`. Do not try to export the trait or `OptionField` types from the proc-macro crate; Rust proc-macro crates are not the right home for runtime API. + +**Technical design:** *(directional; implementation details vary)* + +```rust +// Attribute sketch — not to be used verbatim +#[derive(clap::Parser, fabro_macros::OptionsMetadata)] +struct RunArgs { + /// Skip retro generation after the run + #[arg(long)] + #[option(added_in = "0.213.0")] + skip_retro: bool, + + /// Override default LLM model + #[arg(long, value_enum)] + model: Option, +} + +// Generated metadata shape +impl fabro_options_metadata::OptionsMetadata for RunArgs { + fn record(visit: &mut dyn fabro_options_metadata::Visit) { + visit.record_field("skip_retro", OptionField { /* ... */ }); + visit.record_field("model", OptionField { /* ... */ }); + } +} +``` + +**Patterns to follow:** +- uv: `uv-options-metadata/src/lib.rs` (runtime metadata model). +- uv: `uv-macros/src/options_metadata.rs` (direct port with fabro naming). +- fabro: existing `Combine` derive in `fabro-macros/src/lib.rs:151` for proc-macro plumbing shape. + +**Test scenarios:** +- Happy path: a struct with `#[derive(OptionsMetadata)]` compiles and exposes `metadata()` / `record()` data through `fabro_options_metadata`. +- Happy path: doc-comments appear in the metadata. +- Happy path: `ValueEnum` variants appear in a `possible_values` field on the metadata entry. +- Happy path: nested metadata can be traversed with a `Visit` implementation without depending on `fabro-macros` at runtime. +- Edge case: a field with no doc-comment produces a metadata entry with `None` for doc. +- Edge case: nested structs with `#[option_group]` produce grouped metadata. +- Error path: invalid attribute syntax produces a clear compile error. + +**Verification:** +- `cargo test -p fabro-options-metadata` passes. +- `cargo test -p fabro-macros` passes. +- A small downstream crate (test fixture) compiles with the derive and produces expected metadata. + +--- + +- [ ] **Unit 6.2: Add `cargo dev generate-cli-reference` to regenerate `docs/reference/cli.mdx`** + +**Goal:** Replace hand-authored clap-mirroring portions of `docs/reference/cli.mdx` with generator output; add CI check for drift. + +**Requirements:** R3. + +**Dependencies:** Unit 5.1, Unit 6.1. + +**Files:** +- Create: `lib/crates/fabro-dev/src/commands/generate_cli_reference.rs` +- Modify: `lib/crates/fabro-dev/src/main.rs`. +- Modify: `lib/crates/fabro-cli/src/lib.rs` and/or `lib/crates/fabro-cli/src/main.rs`, or create `lib/crates/fabro-cli-args/`, to expose the clap metadata surface chosen in the Approach. +- Modify: `lib/crates/fabro-cli/src/args.rs` (add `#[derive(OptionsMetadata)]` to relevant structs; may require minor doc-comment normalization). +- Modify: `docs/reference/cli.mdx` (regenerate; commit the generated form). +- Modify: CI config to run `cargo dev generate-cli-reference --check` on PRs. +- Test: `lib/crates/fabro-dev/tests/it/generate_cli_reference.rs` (snapshot the generator output for a known subcommand structure). + +**Approach:** +- Mirror `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-dev/src/generate_cli_reference.rs`. +- Walk the clap `Command` tree + `OptionsMetadata` to emit Mintlify-compatible MDX. +- **Prerequisite:** `fabro-cli` currently has no `lib.rs`; `Cli` lives in `main.rs`, while `Commands`/`GlobalArgs` in `args.rs` are `pub(crate)`. `fabro-dev` cannot import those types until the CLI argument surface is explicitly exposed. This unit must either (a) move `Cli` and the relevant clap types into a library module with a deliberately public metadata surface, while `main.rs` remains only the binary entrypoint, or (b) extract the clap types to a new `fabro-cli-args` crate. This is a real, bounded structural change; record the choice and rationale in the PR. +- **Fenced-region commitment:** `docs/reference/cli.mdx` is restructured once in this unit to introduce `` / `` fences around the CLI option tables. Hand-written intro prose stays above/below the fences. The generator only writes between the fences; `--check` mode only compares between the fences. This is pre-work: audit the 998-line `cli.mdx`, decide which content becomes generated, insert fences, commit, then run the generator for the first time. +- `--check` mode: generate to a buffer, compare against the content between fences in the committed file, fail on mismatch. Determinism requirements: sorted iteration order for options, LF-only line endings, trimmed trailing whitespace. Add a test that re-running the generator three times on unchanged inputs produces byte-identical output each time. + +**Execution note:** Run once against current fabro-cli to see what the output looks like; iterate doc-comment shape until generated output is acceptable, then commit both the generator and the regenerated `cli.mdx`. + +**Patterns to follow:** +- uv: `uv-dev/src/generate_cli_reference.rs`. +- uv: `uv-dev/src/generate_options_reference.rs` (for `user-configuration.mdx` if also migrated — may be a follow-up). + +**Test scenarios:** +- Happy path: `cargo dev generate-cli-reference` produces a non-empty MDX file. +- Happy path: `cargo dev generate-cli-reference --check` on a clean tree exits 0. +- Edge case: a new clap arg added without updating `cli.mdx` causes `--check` to exit non-zero. +- Edge case: a clap arg with no doc-comment produces a stub entry with a visible TODO, not a crash. +- Integration: CI on a PR that changes `args.rs` without regenerating `cli.mdx` fails. + +**Verification:** +- Content between `` fences in `docs/reference/cli.mdx` is byte-identical to `cargo dev generate-cli-reference` output. +- Running the generator three times on unchanged inputs produces identical output (determinism test). +- CI job added and passing. +- Manual `fabro --help` and `fabro run --help` visually match `cli.mdx` content. + +--- + +- [ ] **Unit 6.3: Add `cargo dev generate-options-reference` for `user-configuration.mdx`** + +**Goal:** Close the settings-schema drift gap in `docs/reference/user-configuration.mdx` with a generator driven by `OptionsMetadata` on settings structs. + +**Requirements:** R3. + +**Dependencies:** Unit 5.1, Unit 6.1, Unit 6.2 (reuses the fenced-region convention and generator/check-mode plumbing). + +**Files:** +- Create: `lib/crates/fabro-dev/src/commands/generate_options_reference.rs` +- Modify: `lib/crates/fabro-dev/src/main.rs` (wire subcommand). +- Modify: `lib/crates/fabro-types/src/settings/**/*.rs` (12 files — add `#[derive(OptionsMetadata)]` to settings structs; may require minor doc-comment normalization to match what the generator expects). +- Modify: `docs/reference/user-configuration.mdx` (introduce `` fences; regenerate inside them; commit the generated form). +- Modify: CI config to run `cargo dev generate-options-reference --check`. +- Test: `lib/crates/fabro-dev/tests/it/generate_options_reference.rs`. + +**Approach:** +- Mirror `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-dev/src/generate_options_reference.rs`. +- Walk each settings struct via `OptionsMetadata::metadata()`; emit the `[cli.*]` / `[run.*]` / `[server.*]` TOML-shaped sections that currently appear hand-authored in `user-configuration.mdx` (lines ~139-157 for `[cli.output]` and analogous blocks). +- Use the same fenced-region approach as Unit 6.2: restructure `user-configuration.mdx` once to introduce fences, commit, then regenerate. +- Same determinism requirements: sorted order, LF-only, trimmed whitespace. + +**Patterns to follow:** +- Unit 6.2 (same generator shape, different source structs and different target file). +- uv: `uv-dev/src/generate_options_reference.rs`. + +**Test scenarios:** +- Happy path: generator produces non-empty MDX for every settings struct carrying `#[derive(OptionsMetadata)]`. +- Happy path: `--check` on a clean tree exits 0. +- Edge case: adding a new settings field without regenerating `user-configuration.mdx` fails `--check`. +- Edge case: a settings field whose doc-comment starts with a code fence renders correctly inside MDX. +- Integration: CI on a PR that changes a settings struct without regenerating fails the check step. + +**Verification:** +- Content between `` fences in `user-configuration.mdx` is byte-identical to `cargo dev generate-options-reference` output. +- Determinism test passes (three-run equivalence). +- CI job added and passing. + +--- + +## System-Wide Impact + +- **Interaction graph:** `fabro-static`, `fabro-redacted`, and `fabro-options-metadata` are new leaf/runtime crates with no existing consumers; adding them to workspace members is safe. `fabro-cli` gains a direct dep on `miette` for the CLI boundary and an exposed metadata surface for docs generation. `fabro-dev` depends on that exposed clap/metadata surface at generator time (not in the shipped `fabro` binary runtime path). +- **Error propagation:** Phase 4 wraps at the `main` boundary; library error types unchanged. `anyhow::Error::chain()` still flows through `.context(...)` calls; only the final render changes. +- **State lifecycle risks:** None from Phases 1, 3, 4, 5, 6 (pure refactors). Phase 2's redaction reaches Display and Debug, plus any explicitly serializable redacted-output type. Callers that need the raw URL for wire-level operations (reqwest, git CLI, subprocess args, HTTP `Location:` headers, stderr prompts the user acts on) must use `.as_raw_url()` / `.raw_string()`. `url.to_string()` on a `DisplaySafeUrl` returns the *redacted* form — an intentional trap that's neutralized by the Key Technical Decisions mitigation stack (explicit methods, clippy deny on `::to_string`, `disallowed-types` ban, and per-call-site tests in sandbox/install paths). +- **API surface parity:** `fabro --help` output stays identical through Phase 1 (clap consts vs literals produce the same help). Phase 6 may reshape `cli.mdx` rendering but not `--help`. Phase 2 does NOT touch public API DTOs — `avatar_url`, `user_url`, and any OpenAPI-generated schemas remain `string`/`url::Url`. +- **Integration coverage:** Phase 2 requires tracing-capture tests per migrated crate asserting no token substring leaks via `tracing::debug!(?url)` / `%url`. Phase 2 also requires a test that `shell_quote(auth_url.as_raw_url().as_str())` passes the raw token through (no redaction on subprocess args). Phase 6 requires a CI check that `docs/reference/cli.mdx` matches `cargo dev generate-cli-reference --check`. +- **Unchanged invariants:** + - `Env` trait in `fabro-util/src/env.rs` — still the injection path for tests. `EnvVars` registry is orthogonal. + - `fabro_snapshot!` macro — unchanged; Phase 3 adds a sibling macro, doesn't alter the existing one. + - `exit::exit_class_for(&err)` auth-hint — preserved in Phase 4. + - `fabro-macros::Combine` / `e2e_test` — unchanged; Phase 6 adds a third derive. + - `clippy.toml` existing bans (`std::fs`/`tokio::fs` redirects, `reqwest::Client::{new,builder}` redirects, thread/process bans, `std::env::set_var`/`remove_var`) — unchanged. Phase 1 and Phase 2 **add** new bans per Key Technical Decisions; they do not modify existing ones. + +## Risks & Dependencies + +| Risk | Mitigation | +|------|------------| +| Phase 1 churn (~113 call sites, 20 crates) produces a large diff and high merge-conflict probability against in-flight work | Ship Phase 1 alone in a dedicated PR. Coordinate timing with any other heavy refactors. Stage per-crate commits inside the PR for reviewability. | +| Phase 2 migration misses a credential-bearing URL path; a leak survives | Tracing-capture integration tests in `fabro-github`, `fabro-oauth`, and `fabro-server` assert no token substring. Phase 2 also ships a workspace `clippy.toml` `disallowed-types` ban on raw `url::Url`/`reqwest::Url`; mixed crates use module/call-site expectations rather than root-level allows, and a `format!("https://...:...@")` guard catches inline token URL construction. | +| Query-string-token URLs (`?token=...`, `?state=...`) not covered by uv's DisplaySafeUrl design | Fabro diverges from uv with an allowlist-based query-key redactor baked into `Display`. Allowlist lives in `fabro-redacted` and is documented in `docs-internal/logging-strategy.md`. | +| `Debug` impl diverging from uv breaks snapshot tests that asserted against `?url` or struct-shaped debug output | Audit existing snapshots before Phase 2 lands; `fabro_snapshot!` default filters can normalize URL rendering in snapshots that should be agnostic to the redaction format. | +| `shell_quote` / `.to_string()` trap: `url.to_string()` on a `DisplaySafeUrl` returns the redacted form, silently breaking git remotes, reqwest requests, and any transport use | Multi-layered mitigation: (1) explicit `.redacted_string()` / `.raw_string()` methods encourage callers to name the form; (2) clippy `disallowed-methods` entry for `::to_string` pointing callers at the explicit methods; (3) `disallowed-types` ban on `url::Url`/`reqwest::Url` in credential-handling crates forces all URL handling through the typed API; (4) per-call-site test in the sandbox caller (`fabro-sandbox/src/daytona/mod.rs:826`, `fabro-workflow/src/sandbox_git.rs:174`) that asserts the string passed to `shell_quote` contains the raw token, not `****`. | +| JSON DTO Serialize leaks raw tokens if serialization matches uv's transparent behavior | Fabro diverges: any serializable redacted-output path renders redacted. Unit 2.1 must decide whether that is a blanket `DisplaySafeUrl` impl or a separate output-only wrapper, and must include a data-loss test/usage audit so persistence paths do not accidentally store `****`. | +| `embed_token_in_url` return-type change affects callers that concat/compare as `String` (e.g., `==`, `Hash`, TOML serialization) | Grep callers of `embed_token_in_url` and `resolve_authenticated_url` before migration; each caller's usage shape determines whether it needs `.as_raw_url()`, `Display`, or something else. | +| OpenAPI / schemars schema for `DisplaySafeUrl` must remain `type: string, format: uri` — otherwise the generated TypeScript client in `apps/` breaks | Use `schemars(transparent)` on the newtype. Include a schema assertion in unit tests. Reinforced by the "not a public-API DTO migration" scope boundary — `DisplaySafeUrl` should not appear in DTOs anyway. | +| Phase 4 miette wrapper hides error detail that the current chain loop exposes | Pre-migration: snapshot the current error rendering for 3-5 representative failure scenarios. Post-migration: verify the new rendering contains the same substrings (cause chain, hint, exit class). | +| Phase 5 shell-to-Rust port introduces subtle behavior differences (e.g., `rg` vs Rust regex semantics for `check-boundary`) | Port behavior-for-behavior; run both old and new against the current tree and diff the outputs before deleting the script. Keep the shell script in git history for reference. | +| Phase 6 `OptionsMetadata` proc macro/runtime model is the largest new surface and may surface clap-version compatibility issues | Port uv's split closely (`uv-options-metadata` runtime crate + `uv-macros` derive); uv and fabro both use clap 4. Write dedicated tests in both `fabro-options-metadata` and `fabro-macros` before wiring into `fabro-cli`. | +| `docs/reference/cli.mdx` regeneration drops or reformats hand-written Mintlify features (frontmatter, Tabs, Callouts) | Keep hand-written sections outside the generated region (marked with ` ... ` fences). Generator only touches the fenced region. | +| Ref-cast crate isn't in workspace deps yet | Add to workspace `Cargo.toml` in Phase 2; check current transitive deps first via `cargo tree`. | +| CI boundary check fails transiently during Phase 5 cutover | Land `cargo dev check-boundary` first, run both old and new in CI for one week, then delete the script. | +| Nightly pinned clippy toolchain (`nightly-2026-04-14`) may lag new miette/ref-cast features | Stay on the pinned version; if a crate requires newer nightly, coordinate a toolchain bump as a separate plan. | + +## Documentation / Operational Notes + +- **AGENTS.md updates:** + - After Phase 5: replace `scripts/refresh-fabro-spa.sh` reference at line 24 with `cargo dev refresh-spa`. + - After Phase 5: replace `bin/dev/*.sh` references with `cargo dev ` equivalents. + - After Phase 6: document the `cargo dev generate-cli-reference --check` CI gate; mention that PR authors touching `args.rs` must regenerate. +- **New strategy docs:** Not required. The existing `logging-strategy.md` and `server-secrets-strategy.md` already cover the underlying principles Phase 1 and Phase 2 support. +- **Rollout:** Each phase is independently shippable. Phases 1-4 are expected to be one PR each; Phase 5 may ship one PR per script port; Phase 6 should split runtime/macro, CLI docs generator, and settings docs generator. No feature flag or runtime rollout needed — all changes compile-check or surface via tests. +- **Monitoring:** None needed; no runtime behavior changes outside Phase 2's display-redaction. +- **Migration notes for other contributors:** + - Phase 1 lands first; subsequent in-flight PRs adding new env vars should add them to `EnvVars` rather than as literals. Consider documenting this in AGENTS.md under a "Adding an env var" section post-Phase 1. + - Phase 2: new URL-bearing types should default to `DisplaySafeUrl`. Plain `url::Url` in new code should justify itself. + +## Phased Delivery + +### Phase 1: `fabro-static` EnvVars registry (~1 PR) +- Unit 1.1 + Unit 1.2. Largest mechanical change; lowest semantic risk. +- **Why first:** Foundation for hygienic env var handling; unblocks future work (e.g., registry-driven subprocess allowlist in `spawn_env.rs`, doc generation in Phase 6). + +### Phase 2: `fabro-redacted` DisplaySafeUrl (~1 PR) +- Unit 2.1 + Unit 2.2. Highest security value. +- **Why second:** Independent of Phase 1. Shipping after Phase 1 keeps the mechanical diff separate from the behavior-affecting diff. + +### Phase 3: Snapshot helpers lift (~1 small PR) +- Unit 3.1. Test-infra-only. +- **Why here:** Smallest phase; good palate cleanser between larger phases. Can interleave anywhere. + +### Phase 4: miette CLI diagnostics (~1 small PR) +- Unit 4.1. +- **Why here:** Isolated to `fabro-cli/src/main.rs`. Independent of other phases. + +### Phase 5: `fabro-dev` unified CLI (~1 PR per unit, or one large PR) +- Units 5.1 through 5.5. Consider one PR per unit if reviewers prefer smaller increments; otherwise bundle 5.1-5.2 together (scaffold + first real port) and ship remaining ports incrementally. +- **Why before Phase 6:** `fabro-dev` hosts the generator subcommand. + +### Phase 6: `OptionsMetadata` + docs generation (~3 PRs) +- Unit 6.1 (`fabro-options-metadata` runtime crate + macro), then Unit 6.2 (`cli.mdx` generator + fenced-region introduction + `fabro-cli` library/args exposure), then Unit 6.3 (`user-configuration.mdx` generator). Ship as three PRs so the metadata model/macro, each generator, and each doc restructure are independently reviewable. +- **Why last:** Largest technical surface; depends on Phase 5 to host the generator subcommands and on Unit 6.2 for the `fabro-cli` library/args exposure decision. + +## Sources & References + +- **Origin document:** None — this plan derives from conversation. +- **Related fabro plans:** `docs/plans/2026-04-23-002-refactor-combine-trait-uv-pattern-plan.md` (the seventh uv pattern already adopted). +- **uv references:** + - `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-static/src/env_vars.rs` + - `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-redacted/src/lib.rs` + - `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-options-metadata/src/lib.rs` + - `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-macros/src/lib.rs` + `options_metadata.rs` + - `/Users/bhelmkamp/p/astral-sh/uv/crates/uv-dev/src/` (all files) + - `/Users/bhelmkamp/p/astral-sh/uv/.cargo/config.toml` +- **fabro strategy docs:** + - `docs-internal/logging-strategy.md` (Phase 2 alignment) + - `docs-internal/server-secrets-strategy.md` (Phase 1 constraint: no env mutation) + - `files-internal/testing-strategy.md` (Phase 3 guidance) +- **AGENTS.md:** `/Users/bhelmkamp/p/fabro-sh/fabro-3/AGENTS.md` (nightly clippy, strum, insta workflow, refresh-spa mandate). +- **External docs:** + - `miette`: https://docs.rs/miette/ + - `ref-cast`: https://docs.rs/ref-cast/ diff --git a/lib/crates/fabro-agent/Cargo.toml b/lib/crates/fabro-agent/Cargo.toml index 2a1334a31..058c76d25 100644 --- a/lib/crates/fabro-agent/Cargo.toml +++ b/lib/crates/fabro-agent/Cargo.toml @@ -31,6 +31,7 @@ fabro-llm = { path = "../fabro-llm" } fabro-model = { path = "../fabro-model" } fabro-mcp = { path = "../fabro-mcp" } fabro-sandbox = { path = "../fabro-sandbox" } +fabro-static.workspace = true fabro-util = { path = "../fabro-util" } fabro-vault = { path = "../fabro-vault" } fabro-http.workspace = true diff --git a/lib/crates/fabro-agent/src/tools.rs b/lib/crates/fabro-agent/src/tools.rs index 285f4c4ca..e997396e9 100644 --- a/lib/crates/fabro-agent/src/tools.rs +++ b/lib/crates/fabro-agent/src/tools.rs @@ -5,6 +5,7 @@ use std::sync::Arc; use fabro_llm::client::Client; use fabro_llm::types::{Message, Request, ToolDefinition}; use fabro_model::ModelHandle; +use fabro_static::EnvVars; use crate::config::SessionOptions; use crate::sandbox::GrepOptions; @@ -467,8 +468,12 @@ fn format_brave_results(body: &serde_json::Value) -> String { } #[must_use] +#[expect( + clippy::disallowed_methods, + reason = "Web search tool setup reads the documented Brave API key override from process env." +)] pub(crate) fn make_web_search_tool() -> RegisteredTool { - make_web_search_tool_with_api_key(std::env::var("BRAVE_SEARCH_API_KEY").ok()) + make_web_search_tool_with_api_key(std::env::var(EnvVars::BRAVE_SEARCH_API_KEY).ok()) } fn make_web_search_tool_with_api_key(api_key: Option) -> RegisteredTool { @@ -492,7 +497,10 @@ fn make_web_search_tool_with_api_key(api_key: Option) -> RegisteredTool let api_key = api_key.clone(); Box::pin(async move { let api_key = api_key.ok_or_else(|| { - "BRAVE_SEARCH_API_KEY environment variable is not set".to_string() + format!( + "{} environment variable is not set", + EnvVars::BRAVE_SEARCH_API_KEY + ) })?; let query = required_str(&args, "query")?; @@ -1407,8 +1415,12 @@ mod tests { } #[fabro_macros::e2e_test(live("BRAVE_SEARCH_API_KEY"))] + #[expect( + clippy::disallowed_methods, + reason = "Live web-search integration test reads its required API key from process env." + )] async fn web_search_returns_results() { - let api_key = std::env::var("BRAVE_SEARCH_API_KEY") + let api_key = std::env::var(EnvVars::BRAVE_SEARCH_API_KEY) .expect("BRAVE_SEARCH_API_KEY must be set to run this test"); let tool = make_web_search_tool_with_api_key(Some(api_key)); let env: Arc = Arc::new(MockSandbox::default()); diff --git a/lib/crates/fabro-auth/Cargo.toml b/lib/crates/fabro-auth/Cargo.toml index 1b7ebeb0d..881ef5549 100644 --- a/lib/crates/fabro-auth/Cargo.toml +++ b/lib/crates/fabro-auth/Cargo.toml @@ -17,6 +17,7 @@ chrono = { workspace = true, features = ["serde"] } fabro-http.workspace = true fabro-model = { path = "../fabro-model" } fabro-oauth = { path = "../fabro-oauth" } +fabro-static.workspace = true fabro-util = { path = "../fabro-util" } fabro-vault = { path = "../fabro-vault" } serde.workspace = true diff --git a/lib/crates/fabro-auth/src/env_source.rs b/lib/crates/fabro-auth/src/env_source.rs index c387a1e84..2f136f013 100644 --- a/lib/crates/fabro-auth/src/env_source.rs +++ b/lib/crates/fabro-auth/src/env_source.rs @@ -2,6 +2,7 @@ use std::sync::Arc; use async_trait::async_trait; use fabro_model::Provider; +use fabro_static::EnvVars; use crate::credential_source::{CredentialSource, ResolvedCredentials}; use crate::{ApiCredential, EnvLookup}; @@ -13,6 +14,10 @@ pub struct EnvCredentialSource { impl EnvCredentialSource { #[must_use] + #[expect( + clippy::disallowed_methods, + reason = "EnvCredentialSource is the provider API-key process-env facade." + )] pub fn new() -> Self { Self::with_env_lookup(Arc::new(|name| std::env::var(name).ok())) } @@ -35,13 +40,13 @@ impl EnvCredentialSource { let mut cred = ApiCredential::from_api_key(provider, key); match provider { Provider::Anthropic => { - cred.base_url = self.lookup("ANTHROPIC_BASE_URL"); + cred.base_url = self.lookup(EnvVars::ANTHROPIC_BASE_URL); } Provider::OpenAi => { - cred.base_url = self.lookup("OPENAI_BASE_URL"); - cred.org_id = self.lookup("OPENAI_ORG_ID"); - cred.project_id = self.lookup("OPENAI_PROJECT_ID"); - if let Some(account_id) = self.lookup("CHATGPT_ACCOUNT_ID") { + cred.base_url = self.lookup(EnvVars::OPENAI_BASE_URL); + cred.org_id = self.lookup(EnvVars::OPENAI_ORG_ID); + cred.project_id = self.lookup(EnvVars::OPENAI_PROJECT_ID); + if let Some(account_id) = self.lookup(EnvVars::CHATGPT_ACCOUNT_ID) { cred.base_url = Some("https://chatgpt.com/backend-api/codex".to_string()); cred.codex_mode = true; cred.extra_headers @@ -51,7 +56,7 @@ impl EnvCredentialSource { } } Provider::Gemini => { - cred.base_url = self.lookup("GEMINI_BASE_URL"); + cred.base_url = self.lookup(EnvVars::GEMINI_BASE_URL); } Provider::Kimi | Provider::Zai | Provider::Minimax | Provider::Inception => {} // OpenAiCompatible has no api_key_env_vars, so find_map returned None above. diff --git a/lib/crates/fabro-auth/src/resolve.rs b/lib/crates/fabro-auth/src/resolve.rs index 03643d6c8..f1a63c984 100644 --- a/lib/crates/fabro-auth/src/resolve.rs +++ b/lib/crates/fabro-auth/src/resolve.rs @@ -2,6 +2,7 @@ use std::collections::HashMap; use std::sync::Arc; use fabro_model::Provider; +use fabro_static::EnvVars; use fabro_vault::Vault; use shlex::try_quote; use tokio::sync::RwLock as AsyncRwLock; @@ -115,6 +116,10 @@ pub struct CredentialResolver { impl CredentialResolver { #[must_use] + #[expect( + clippy::disallowed_methods, + reason = "CredentialResolver owns the process-env fallback used after vault lookup." + )] pub fn new(vault: Arc>) -> Self { Self::with_env_lookup(vault, Arc::new(|name| std::env::var(name).ok())) } @@ -229,12 +234,12 @@ impl CredentialResolver { fn to_api_credential(&self, vault: &Vault, credential: &AuthCredential) -> ApiCredential { let base_url = match credential.provider { - Provider::Anthropic => self.lookup_env_or_vault(vault, "ANTHROPIC_BASE_URL"), - Provider::OpenAi => self.lookup_env_or_vault(vault, "OPENAI_BASE_URL"), - Provider::Gemini => self.lookup_env_or_vault(vault, "GEMINI_BASE_URL"), + Provider::Anthropic => self.lookup_env_or_vault(vault, EnvVars::ANTHROPIC_BASE_URL), + Provider::OpenAi => self.lookup_env_or_vault(vault, EnvVars::OPENAI_BASE_URL), + Provider::Gemini => self.lookup_env_or_vault(vault, EnvVars::GEMINI_BASE_URL), Provider::Kimi | Provider::Zai | Provider::Minimax | Provider::Inception => None, Provider::OpenAiCompatible => { - self.lookup_env_or_vault(vault, "OPENAI_COMPATIBLE_BASE_URL") + self.lookup_env_or_vault(vault, EnvVars::OPENAI_COMPATIBLE_BASE_URL) } }; match &credential.details { @@ -242,8 +247,8 @@ impl CredentialResolver { let mut cred = ApiCredential::from_api_key(credential.provider, key.clone()); cred.base_url = base_url; if credential.provider == Provider::OpenAi { - cred.org_id = self.lookup_env_or_vault(vault, "OPENAI_ORG_ID"); - cred.project_id = self.lookup_env_or_vault(vault, "OPENAI_PROJECT_ID"); + cred.org_id = self.lookup_env_or_vault(vault, EnvVars::OPENAI_ORG_ID); + cred.project_id = self.lookup_env_or_vault(vault, EnvVars::OPENAI_PROJECT_ID); } cred } @@ -261,8 +266,8 @@ impl CredentialResolver { extra_headers, base_url: Some("https://chatgpt.com/backend-api/codex".to_string()), codex_mode: true, - org_id: self.lookup_env_or_vault(vault, "OPENAI_ORG_ID"), - project_id: self.lookup_env_or_vault(vault, "OPENAI_PROJECT_ID"), + org_id: self.lookup_env_or_vault(vault, EnvVars::OPENAI_ORG_ID), + project_id: self.lookup_env_or_vault(vault, EnvVars::OPENAI_PROJECT_ID), } } } @@ -272,7 +277,7 @@ impl CredentialResolver { let mut env_vars = HashMap::new(); let login_command = match (&credential.provider, &credential.details, kind) { (Provider::OpenAi, AuthDetails::ApiKey { key }, CliAgentKind::Codex) => { - env_vars.insert("OPENAI_API_KEY".to_string(), key.clone()); + env_vars.insert(EnvVars::OPENAI_API_KEY.to_string(), key.clone()); Some(codex_login_command(key)) } ( @@ -282,9 +287,12 @@ impl CredentialResolver { }, CliAgentKind::Codex, ) => { - env_vars.insert("OPENAI_API_KEY".to_string(), tokens.access_token.clone()); + env_vars.insert( + EnvVars::OPENAI_API_KEY.to_string(), + tokens.access_token.clone(), + ); if let Some(account_id) = account_id { - env_vars.insert("CHATGPT_ACCOUNT_ID".to_string(), account_id.clone()); + env_vars.insert(EnvVars::CHATGPT_ACCOUNT_ID.to_string(), account_id.clone()); } Some(codex_login_command(&tokens.access_token)) } @@ -295,7 +303,10 @@ impl CredentialResolver { None } (_, AuthDetails::CodexOAuth { tokens, .. }, _) => { - env_vars.insert("OPENAI_API_KEY".to_string(), tokens.access_token.clone()); + env_vars.insert( + EnvVars::OPENAI_API_KEY.to_string(), + tokens.access_token.clone(), + ); None } }; @@ -319,16 +330,22 @@ pub async fn configured_providers_from_process_env( None => Provider::ALL .iter() .copied() - .filter(|provider| { - provider - .api_key_env_vars() - .iter() - .any(|env_var| std::env::var(env_var).is_ok()) - }) + .filter(|provider| provider_has_process_env_api_key(*provider)) .collect(), } } +#[expect( + clippy::disallowed_methods, + reason = "Provider discovery intentionally checks documented API-key env names." +)] +fn provider_has_process_env_api_key(provider: Provider) -> bool { + provider + .api_key_env_vars() + .iter() + .any(|env_var| std::env::var(env_var).is_ok()) +} + fn codex_login_command(api_key: &str) -> String { let quoted = try_quote(api_key).map_or_else(|_| api_key.to_string(), std::borrow::Cow::into_owned); diff --git a/lib/crates/fabro-cli/Cargo.toml b/lib/crates/fabro-cli/Cargo.toml index f22653cc3..1676334d6 100644 --- a/lib/crates/fabro-cli/Cargo.toml +++ b/lib/crates/fabro-cli/Cargo.toml @@ -47,6 +47,7 @@ fabro-vault = { path = "../fabro-vault" } fabro-types = { path = "../fabro-types", features = ["clap"] } fabro-util = { path = "../fabro-util" } fabro-http.workspace = true +fabro-static.workspace = true clap.workspace = true clap_complete.workspace = true cli-table.workspace = true diff --git a/lib/crates/fabro-cli/src/args.rs b/lib/crates/fabro-cli/src/args.rs index 9d2e4578e..54d651231 100644 --- a/lib/crates/fabro-cli/src/args.rs +++ b/lib/crates/fabro-cli/src/args.rs @@ -4,6 +4,7 @@ use std::path::{Path, PathBuf}; use clap::{Args, Subcommand, ValueEnum}; use fabro_agent::cli::AgentArgs; use fabro_config::{CliLayer, CliLoggingLayer, CliOutputLayer, CliUpdatesLayer}; +use fabro_static::EnvVars; use fabro_types::settings::cli::{OutputFormat, OutputVerbosity}; use fabro_types::settings::run::MergeStrategy; use fabro_util::printer::Printer; @@ -21,23 +22,23 @@ pub(crate) const LONG_VERSION: &str = concat!( #[derive(Args)] pub(crate) struct GlobalArgs { /// Output as JSON - #[arg(long, global = true, env = "FABRO_JSON", value_parser = clap::builder::BoolishValueParser::new())] + #[arg(long, global = true, env = EnvVars::FABRO_JSON, value_parser = clap::builder::BoolishValueParser::new())] pub json: bool, /// Enable DEBUG-level logging (default is INFO) - #[arg(long, global = true, env = "FABRO_DEBUG", value_parser = clap::builder::BoolishValueParser::new())] + #[arg(long, global = true, env = EnvVars::FABRO_DEBUG, value_parser = clap::builder::BoolishValueParser::new())] pub debug: bool, /// Disable automatic upgrade check - #[arg(long, global = true, env = "FABRO_NO_UPGRADE_CHECK", value_parser = clap::builder::BoolishValueParser::new())] + #[arg(long, global = true, env = EnvVars::FABRO_NO_UPGRADE_CHECK, value_parser = clap::builder::BoolishValueParser::new())] pub no_upgrade_check: bool, /// Suppress non-essential output - #[arg(long, global = true, env = "FABRO_QUIET", value_parser = clap::builder::BoolishValueParser::new(), conflicts_with = "verbose")] + #[arg(long, global = true, env = EnvVars::FABRO_QUIET, value_parser = clap::builder::BoolishValueParser::new(), conflicts_with = "verbose")] pub quiet: bool, /// Enable verbose output - #[arg(long, global = true, env = "FABRO_VERBOSE", value_parser = clap::builder::BoolishValueParser::new(), conflicts_with = "quiet")] + #[arg(long, global = true, env = EnvVars::FABRO_VERBOSE, value_parser = clap::builder::BoolishValueParser::new(), conflicts_with = "quiet")] pub verbose: bool, } @@ -84,7 +85,7 @@ pub(crate) fn require_no_json_override(process_local_json: bool) -> anyhow::Resu #[derive(Args, Debug, Clone, Default)] pub(crate) struct StorageDirArgs { /// Local storage directory (default: ~/.fabro/storage) - #[arg(long, env = "FABRO_STORAGE_DIR")] + #[arg(long, env = EnvVars::FABRO_STORAGE_DIR)] pub(crate) storage_dir: Option, } @@ -101,7 +102,7 @@ impl StorageDirArgs { #[derive(Args, Debug, Clone, Default)] pub(crate) struct ServerTargetArgs { /// Fabro server target: http(s) URL or absolute Unix socket path - #[arg(long = "server", env = "FABRO_SERVER")] + #[arg(long = "server", env = EnvVars::FABRO_SERVER)] pub(crate) server: Option, } diff --git a/lib/crates/fabro-cli/src/commands/auth/status.rs b/lib/crates/fabro-cli/src/commands/auth/status.rs index fa2f1af56..692c1f381 100644 --- a/lib/crates/fabro-cli/src/commands/auth/status.rs +++ b/lib/crates/fabro-cli/src/commands/auth/status.rs @@ -1,6 +1,7 @@ use anyhow::Result; use chrono::{DateTime, Utc}; use fabro_client::{AuthEntry, AuthStore}; +use fabro_static::EnvVars; use fabro_util::dev_token::{read_dev_token_file, validate_dev_token_format}; use serde::Serialize; @@ -166,8 +167,12 @@ fn human_state(state: OAuthState) -> &'static str { } } +#[expect( + clippy::disallowed_methods, + reason = "Auth status reports whether the documented dev-token env source is configured." +)] fn load_dev_token_if_available() -> bool { - let env_token = std::env::var("FABRO_DEV_TOKEN") + let env_token = std::env::var(EnvVars::FABRO_DEV_TOKEN) .ok() .filter(|token| validate_dev_token_format(token)); env_token.is_some() diff --git a/lib/crates/fabro-cli/src/commands/run/checkpoints.rs b/lib/crates/fabro-cli/src/commands/run/checkpoints.rs new file mode 100644 index 000000000..02e65721d --- /dev/null +++ b/lib/crates/fabro-cli/src/commands/run/checkpoints.rs @@ -0,0 +1,118 @@ +use anyhow::Result; +use cli_table::format::{Border, Separator}; +use cli_table::{Cell, CellStruct, Color, Style, Table}; +use fabro_api::types::TimelineEntryResponse; +use fabro_types::RunId; +use fabro_util::printer::Printer; +use fabro_util::terminal::Styles; +use git2::Repository; +use serde::Serialize; + +use crate::server_client::Client; +use crate::shared::color_if; +use crate::shared::repo::ensure_matching_repo_origin; + +#[derive(Serialize)] +pub(crate) struct TimelineEntryJson { + ordinal: usize, + node_name: String, + visit: usize, + run_commit_sha: Option, +} + +pub(crate) async fn ensure_origin_if_local( + client: &Client, + run_id: &RunId, + verb: &str, +) -> Result<()> { + if Repository::discover(".").is_err() { + return Ok(()); + } + + let state = client.get_run_state(run_id).await?; + if let Some(run_spec) = state.spec { + ensure_matching_repo_origin(run_spec.repo_origin_url.as_deref(), verb)?; + } + Ok(()) +} + +pub(crate) fn timeline_entries_json(entries: &[TimelineEntryResponse]) -> Vec { + entries + .iter() + .map(|entry| TimelineEntryJson { + ordinal: usize::try_from(entry.ordinal.get()) + .expect("timeline ordinal should fit in usize"), + node_name: entry.node_name.clone(), + visit: usize::try_from(entry.visit.get()) + .expect("timeline visit should fit in usize"), + run_commit_sha: entry.run_commit_sha.clone(), + }) + .collect() +} + +pub(crate) fn short_id(run_id: &str) -> &str { + &run_id[..8.min(run_id.len())] +} + +pub(crate) fn print_timeline(entries: &[TimelineEntryJson], styles: &Styles, printer: Printer) { + if entries.is_empty() { + fabro_util::printerr!(printer, "No checkpoints found."); + return; + } + + let use_color = styles.use_color; + let title = vec![ + "@".cell().bold(use_color), + "Node".cell().bold(use_color), + "Details".cell().bold(use_color), + ]; + + let rows: Vec> = entries + .iter() + .map(|entry| { + let ordinal_str = format!("@{}", entry.ordinal); + let mut details = Vec::new(); + if entry.visit > 1 { + details.push(format!("visit {}, loop", entry.visit)); + } + if entry.run_commit_sha.is_none() { + details.push("no run commit".to_string()); + } + + let detail_str = if details.is_empty() { + String::new() + } else { + format!("({})", details.join(", ")) + }; + + vec![ + ordinal_str + .cell() + .foreground_color(color_if(use_color, Color::Cyan)), + entry.node_name.clone().cell(), + detail_str + .cell() + .foreground_color(color_if(use_color, Color::Ansi256(8))), + ] + }) + .collect(); + + let color_choice = if use_color { + cli_table::ColorChoice::Auto + } else { + cli_table::ColorChoice::Never + }; + let table = rows + .table() + .title(title) + .color_choice(color_choice) + .border(Border::builder().build()) + .separator(Separator::builder().build()); + #[allow( + clippy::print_stderr, + reason = "The checkpoint timeline table is operator feedback, not command output." + )] + if let Ok(display) = table.display() { + eprintln!("{display}"); + } +} diff --git a/lib/crates/fabro-cli/src/commands/run/fork.rs b/lib/crates/fabro-cli/src/commands/run/fork.rs index ad83d6fdf..fc22d3258 100644 --- a/lib/crates/fabro-cli/src/commands/run/fork.rs +++ b/lib/crates/fabro-cli/src/commands/run/fork.rs @@ -11,16 +11,16 @@ pub(crate) async fn run(args: &ForkArgs, styles: &Styles, base_ctx: &CommandCont let ctx = base_ctx.with_target(&args.server)?; let client = ctx.server().await?; let run_id = client.resolve_run(&args.run_id).await?.run_id; - super::rewind::ensure_origin_if_local(client.as_ref(), &run_id, "fork").await?; + super::checkpoints::ensure_origin_if_local(client.as_ref(), &run_id, "fork").await?; if args.list { let timeline = client.run_timeline(&run_id).await?; if ctx.json_output() { - print_json_pretty(&super::rewind::timeline_entries_json(&timeline))?; + print_json_pretty(&super::checkpoints::timeline_entries_json(&timeline))?; return Ok(()); } - let entries = super::rewind::timeline_entries_json(&timeline); - super::rewind::print_timeline(&entries, styles, printer); + let entries = super::checkpoints::timeline_entries_json(&timeline); + super::checkpoints::print_timeline(&entries, styles, printer); return Ok(()); } @@ -32,22 +32,18 @@ pub(crate) async fn run(args: &ForkArgs, styles: &Styles, base_ctx: &CommandCont .await?; if ctx.json_output() { - print_json_pretty(&serde_json::json!({ - "source_run_id": response.source_run_id, - "new_run_id": response.new_run_id, - "target": response.target, - }))?; + print_json_pretty(&response)?; } else { fabro_util::printerr!( printer, "\nForked run {} -> {}", - super::rewind::short_id(&response.source_run_id), - super::rewind::short_id(&response.new_run_id) + super::checkpoints::short_id(&response.source_run_id), + super::checkpoints::short_id(&response.new_run_id) ); fabro_util::printerr!( printer, "To resume: fabro resume {}", - super::rewind::short_id(&response.new_run_id) + super::checkpoints::short_id(&response.new_run_id) ); } diff --git a/lib/crates/fabro-cli/src/commands/run/mod.rs b/lib/crates/fabro-cli/src/commands/run/mod.rs index 45b10757f..48dedee9b 100644 --- a/lib/crates/fabro-cli/src/commands/run/mod.rs +++ b/lib/crates/fabro-cli/src/commands/run/mod.rs @@ -8,6 +8,7 @@ use crate::shared::print_json_pretty; use crate::sleep_inhibitor; pub(crate) mod attach; +pub(crate) mod checkpoints; pub(crate) mod command; pub(crate) mod cp; pub(crate) mod create; diff --git a/lib/crates/fabro-cli/src/commands/run/rewind.rs b/lib/crates/fabro-cli/src/commands/run/rewind.rs index d2ffc02b0..51e2d6e6e 100644 --- a/lib/crates/fabro-cli/src/commands/run/rewind.rs +++ b/lib/crates/fabro-cli/src/commands/run/rewind.rs @@ -1,26 +1,11 @@ use anyhow::Result; -use cli_table::format::{Border, Separator}; -use cli_table::{Cell, CellStruct, Color, Style, Table}; -use fabro_api::types::{RewindRequest, TimelineEntryResponse}; -use fabro_types::RunId; -use fabro_util::printer::Printer; +use fabro_api::types::RewindRequest; use fabro_util::terminal::Styles; -use git2::Repository; -use serde::Serialize; +use super::checkpoints::{ensure_origin_if_local, print_timeline, short_id, timeline_entries_json}; use crate::args::RewindArgs; use crate::command_context::CommandContext; -use crate::server_client::Client; -use crate::shared::repo::ensure_matching_repo_origin; -use crate::shared::{color_if, print_json_pretty}; - -#[derive(Serialize)] -pub(crate) struct TimelineEntryJson { - ordinal: usize, - node_name: String, - visit: usize, - run_commit_sha: Option, -} +use crate::shared::print_json_pretty; pub(crate) async fn run( args: &RewindArgs, @@ -88,100 +73,3 @@ pub(crate) async fn run( Ok(()) } - -pub(crate) async fn ensure_origin_if_local( - client: &Client, - run_id: &RunId, - verb: &str, -) -> Result<()> { - if Repository::discover(".").is_err() { - return Ok(()); - } - - let state = client.get_run_state(run_id).await?; - if let Some(run_spec) = state.spec { - ensure_matching_repo_origin(run_spec.repo_origin_url.as_deref(), verb)?; - } - Ok(()) -} - -pub(crate) fn timeline_entries_json(entries: &[TimelineEntryResponse]) -> Vec { - entries - .iter() - .map(|entry| TimelineEntryJson { - ordinal: usize::try_from(entry.ordinal.get()) - .expect("timeline ordinal should fit in usize"), - node_name: entry.node_name.clone(), - visit: usize::try_from(entry.visit.get()) - .expect("timeline visit should fit in usize"), - run_commit_sha: entry.run_commit_sha.clone(), - }) - .collect() -} - -pub(crate) fn short_id(run_id: &str) -> &str { - &run_id[..8.min(run_id.len())] -} - -pub(crate) fn print_timeline(entries: &[TimelineEntryJson], styles: &Styles, printer: Printer) { - if entries.is_empty() { - fabro_util::printerr!(printer, "No checkpoints found."); - return; - } - - let use_color = styles.use_color; - let title = vec![ - "@".cell().bold(use_color), - "Node".cell().bold(use_color), - "Details".cell().bold(use_color), - ]; - - let rows: Vec> = entries - .iter() - .map(|entry| { - let ordinal_str = format!("@{}", entry.ordinal); - let mut details = Vec::new(); - if entry.visit > 1 { - details.push(format!("visit {}, loop", entry.visit)); - } - if entry.run_commit_sha.is_none() { - details.push("no run commit".to_string()); - } - - let detail_str = if details.is_empty() { - String::new() - } else { - format!("({})", details.join(", ")) - }; - - vec![ - ordinal_str - .cell() - .foreground_color(color_if(use_color, Color::Cyan)), - entry.node_name.clone().cell(), - detail_str - .cell() - .foreground_color(color_if(use_color, Color::Ansi256(8))), - ] - }) - .collect(); - - let color_choice = if use_color { - cli_table::ColorChoice::Auto - } else { - cli_table::ColorChoice::Never - }; - let table = rows - .table() - .title(title) - .color_choice(color_choice) - .border(Border::builder().build()) - .separator(Separator::builder().build()); - #[allow( - clippy::print_stderr, - reason = "The rewind preview table is operator feedback, not command output." - )] - if let Ok(display) = table.display() { - eprintln!("{display}"); - } -} diff --git a/lib/crates/fabro-cli/src/commands/server/mod.rs b/lib/crates/fabro-cli/src/commands/server/mod.rs index b2c56a60c..4d523c0c1 100644 --- a/lib/crates/fabro-cli/src/commands/server/mod.rs +++ b/lib/crates/fabro-cli/src/commands/server/mod.rs @@ -9,9 +9,10 @@ use anyhow::Result; use base64::Engine as _; use base64::engine::general_purpose::URL_SAFE_NO_PAD; use fabro_config::bind::{self, Bind, BindRequest}; -use fabro_config::user::{FABRO_CONFIG_ENV, active_settings_path, default_storage_dir}; +use fabro_config::user::{active_settings_path, default_storage_dir}; use fabro_server::install::{self, InstallAppState}; use fabro_server::serve::{self, ServeArgs}; +use fabro_static::EnvVars; use fabro_util::browser; use fabro_util::printer::Printer; use fabro_util::terminal::Styles; @@ -167,7 +168,7 @@ fn maybe_install_bootstrap( storage_dir: Option<&std::path::Path>, serve_args: &ServeArgs, ) -> Result> { - if explicit_config.is_some() || std::env::var_os(FABRO_CONFIG_ENV).is_some() { + if explicit_config.is_some() || has_config_env_override() { return Ok(None); } @@ -191,6 +192,14 @@ fn maybe_install_bootstrap( })) } +#[expect( + clippy::disallowed_methods, + reason = "Install bootstrap checks whether the documented FABRO_CONFIG override is set." +)] +fn has_config_env_override() -> bool { + std::env::var_os(EnvVars::FABRO_CONFIG).is_some() +} + async fn run_install_mode(bootstrap: InstallBootstrap, printer: Printer) -> Result<()> { let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); let token = bootstrap.token.clone(); @@ -261,8 +270,12 @@ fn install_mode_next_step_message(supervised: bool) -> &'static str { } } +#[expect( + clippy::disallowed_methods, + reason = "Install-mode URL hints honor Railway's documented public-domain env var." +)] fn install_url_hint(bind: &Bind, token: &str) -> Option { - if let Some(domain) = std::env::var("RAILWAY_PUBLIC_DOMAIN") + if let Some(domain) = std::env::var(EnvVars::RAILWAY_PUBLIC_DOMAIN) .ok() .filter(|value| !value.is_empty()) { @@ -289,10 +302,14 @@ fn default_install_bind_request() -> BindRequest { } } +#[expect( + clippy::disallowed_methods, + reason = "Install-mode bind defaults inspect known container platform env markers." +)] fn running_in_container() -> bool { - std::env::var_os("RAILWAY_PUBLIC_DOMAIN").is_some() - || std::env::var_os("RAILWAY_ENVIRONMENT").is_some() - || std::env::var_os("KUBERNETES_SERVICE_HOST").is_some() + std::env::var_os(EnvVars::RAILWAY_PUBLIC_DOMAIN).is_some() + || std::env::var_os(EnvVars::RAILWAY_ENVIRONMENT).is_some() + || std::env::var_os(EnvVars::KUBERNETES_SERVICE_HOST).is_some() || std::path::Path::new("/.dockerenv").exists() || std::path::Path::new("/run/.containerenv").exists() } diff --git a/lib/crates/fabro-cli/src/commands/server/start.rs b/lib/crates/fabro-cli/src/commands/server/start.rs index e479749c2..47d5cdff9 100644 --- a/lib/crates/fabro-cli/src/commands/server/start.rs +++ b/lib/crates/fabro-cli/src/commands/server/start.rs @@ -10,10 +10,11 @@ use anyhow::{Context, Result, anyhow, bail}; use fabro_config::RuntimeDirectory; use fabro_config::bind::{Bind, BindRequest}; use fabro_config::daemon::ServerDaemon; -use fabro_config::user::{FABRO_CONFIG_ENV, default_settings_path}; +use fabro_config::user::default_settings_path; use fabro_server::jwt_auth::auth_method_name; use fabro_server::serve::{DEFAULT_TCP_PORT, ServeArgs, resolve_runtime_server_settings_for_start}; use fabro_server::{process_env_snapshot, validate_startup}; +use fabro_static::EnvVars; use fabro_types::settings::ServerAuthMethod; use fabro_util::printer::Printer; use fabro_util::terminal::Styles; @@ -92,7 +93,7 @@ pub(crate) async fn ensure_server_running_for_storage( config_path: &Path, ) -> Result { ensure_storage_server_autostart_allowed( - std::env::var_os(FABRO_CONFIG_ENV).as_deref(), + std::env::var_os(EnvVars::FABRO_CONFIG).as_deref(), config_path, &default_settings_path(), )?; @@ -208,7 +209,7 @@ fn bind_matches_request(existing: &Bind, requested: &BindRequest) -> bool { } fn server_max_concurrent_runs_override() -> Option { - std::env::var("FABRO_SERVER_MAX_CONCURRENT_RUNS") + std::env::var(EnvVars::FABRO_SERVER_MAX_CONCURRENT_RUNS) .ok() .and_then(|value| value.parse::().ok()) .filter(|value| *value > 0) diff --git a/lib/crates/fabro-cli/src/commands/uninstall.rs b/lib/crates/fabro-cli/src/commands/uninstall.rs index f29f3b73e..6fd793f74 100644 --- a/lib/crates/fabro-cli/src/commands/uninstall.rs +++ b/lib/crates/fabro-cli/src/commands/uninstall.rs @@ -15,6 +15,7 @@ use std::time::Duration; use anyhow::{Context, Result}; use fabro_config::Storage; use fabro_config::daemon::ServerDaemon; +use fabro_static::EnvVars; use fabro_util::Home; use fabro_util::printer::Printer; use serde::Serialize; @@ -120,13 +121,17 @@ fn dir_size(path: &Path) -> u64 { total } +#[expect( + clippy::disallowed_methods, + reason = "Uninstall scans shell config paths honoring the conventional ZDOTDIR env var." +)] fn find_shell_configs_with_sentinel() -> Vec { let mut found = Vec::new(); let Some(home) = dirs::home_dir() else { return found; }; - let zdotdir = std::env::var("ZDOTDIR") + let zdotdir = std::env::var(EnvVars::ZDOTDIR) .ok() .map_or_else(|| home.clone(), PathBuf::from); diff --git a/lib/crates/fabro-cli/src/local_server.rs b/lib/crates/fabro-cli/src/local_server.rs index 21f6cf427..5b15c8690 100644 --- a/lib/crates/fabro-cli/src/local_server.rs +++ b/lib/crates/fabro-cli/src/local_server.rs @@ -65,7 +65,15 @@ impl LocalServerConfig { } pub(crate) fn storage_dir_from_toml(source: &str) -> Result { - storage_dir_from_toml_with_lookup(source, &|name| std::env::var(name).ok()) + storage_dir_from_toml_with_lookup(source, &process_env_var) +} + +#[expect( + clippy::disallowed_methods, + reason = "Local server config interpolation owns a process-env lookup facade for {{ env.* }} values." +)] +fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() } fn storage_dir_from_toml_with_lookup( diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index f711abd21..f1dfb8d32 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -27,6 +27,7 @@ use args::{ global_args_cli_layer, require_no_json_override, }; use clap::{CommandFactory, Parser}; +use fabro_static::EnvVars; use fabro_telemetry::{git, panic as tel_panic, sanitize, sender}; use fabro_util::exit::ExitClass; use fabro_util::printer::Printer; @@ -79,14 +80,14 @@ async fn main() { // unscrubbed spawn site cannot leak it. The token flows to `runner::execute` // through an explicit function argument instead of the environment. let worker_token = if subcommand == Some("__run-worker") { - let token = std::env::var("FABRO_WORKER_TOKEN").ok(); + let token = process_env_var(EnvVars::FABRO_WORKER_TOKEN); #[expect( clippy::disallowed_methods, reason = "Scrub the worker bearer from this process's env before any \ child process is spawned, so no descendant can inherit it." )] { - std::env::remove_var("FABRO_WORKER_TOKEN"); + std::env::remove_var(EnvVars::FABRO_WORKER_TOKEN); } token } else { @@ -108,7 +109,7 @@ async fn main() { if !command_name.is_empty() { let command = sanitize::sanitize_command(&raw_args, &command_name); let repository = git::repository_identifier(); - let ci = std::env::var("CI").is_ok(); + let ci = process_env_var(EnvVars::CI).is_some(); if is_error { fabro_telemetry::track!("CLI Errored", { "subcommand": command_name, @@ -166,6 +167,14 @@ async fn main() { } } +#[expect( + clippy::disallowed_methods, + reason = "CLI main reads documented process-env controls before telemetry and worker dispatch." +)] +fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() +} + async fn main_inner(worker_token: Option) -> (String, Result<()>) { let _ = default_provider().install_default(); diff --git a/lib/crates/fabro-cli/src/server_client.rs b/lib/crates/fabro-cli/src/server_client.rs index d6c2551f2..233b6b6a6 100644 --- a/lib/crates/fabro-cli/src/server_client.rs +++ b/lib/crates/fabro-cli/src/server_client.rs @@ -10,6 +10,7 @@ use fabro_client::{ }; pub(crate) use fabro_client::{Client, RunEventStream}; use fabro_config::bind::Bind; +use fabro_static::EnvVars; pub(crate) use fabro_types::RunProjection; use fabro_types::UserSettings; use fabro_util::dev_token::validate_dev_token_format; @@ -208,10 +209,18 @@ fn local_dev_token_fallback(target: &ServerTarget) -> bool { } fn load_cli_dev_token() -> Option { - let env_token = std::env::var("FABRO_DEV_TOKEN").ok(); + let env_token = process_env_var(EnvVars::FABRO_DEV_TOKEN); load_cli_dev_token_from_sources(env_token.as_deref(), &Home::from_env()) } +#[expect( + clippy::disallowed_methods, + reason = "Server client authentication supports the documented local dev-token env source." +)] +fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() +} + fn load_cli_dev_token_from_sources(env_token: Option<&str>, home: &Home) -> Option { if let Some(token) = env_token.filter(|token| validate_dev_token_format(token)) { return Some(token.to_owned()); @@ -307,7 +316,7 @@ fn resolve_local_tcp_credential_with_store( } fn resolve_local_tcp_credential(target: &ServerTarget) -> Result> { - let env_token = std::env::var("FABRO_DEV_TOKEN").ok(); + let env_token = process_env_var(EnvVars::FABRO_DEV_TOKEN); let store = AuthStore::default(); resolve_local_tcp_credential_with_store( target, @@ -321,7 +330,7 @@ fn resolve_target_credential( target: &ServerTarget, allow_local_dev_token_fallback: bool, ) -> Result> { - let env_token = std::env::var("FABRO_DEV_TOKEN").ok(); + let env_token = process_env_var(EnvVars::FABRO_DEV_TOKEN); let store = AuthStore::default(); if let Some(credential) = resolve_local_tcp_credential_with_store( target, diff --git a/lib/crates/fabro-cli/src/shared/github.rs b/lib/crates/fabro-cli/src/shared/github.rs index aebb119b3..7eea66df3 100644 --- a/lib/crates/fabro-cli/src/shared/github.rs +++ b/lib/crates/fabro-cli/src/shared/github.rs @@ -1,5 +1,6 @@ use anyhow::anyhow; use fabro_github::GitHubCredentials; +use fabro_static::EnvVars; use fabro_types::settings::server::GithubIntegrationStrategy; use fabro_vault::Vault; @@ -27,9 +28,14 @@ pub(crate) fn build_github_credentials( /// Look up GitHub token: GITHUB_TOKEN env -> vault GITHUB_TOKEN -> GH_TOKEN env /// -> vault GH_TOKEN fn lookup_github_token(vault: Option<&Vault>) -> Option { - lookup_env_or_vault("GITHUB_TOKEN", vault).or_else(|| lookup_env_or_vault("GH_TOKEN", vault)) + lookup_env_or_vault(EnvVars::GITHUB_TOKEN, vault) + .or_else(|| lookup_env_or_vault(EnvVars::GH_TOKEN, vault)) } +#[expect( + clippy::disallowed_methods, + reason = "GitHub credential resolution intentionally falls back from vault to documented process-env names." +)] fn lookup_env_or_vault(name: &str, vault: Option<&Vault>) -> Option { std::env::var(name) .ok() diff --git a/lib/crates/fabro-cli/src/shared/provider_auth.rs b/lib/crates/fabro-cli/src/shared/provider_auth.rs index a3ed147ac..7b22bfcf5 100644 --- a/lib/crates/fabro-cli/src/shared/provider_auth.rs +++ b/lib/crates/fabro-cli/src/shared/provider_auth.rs @@ -116,6 +116,10 @@ fn read_api_key_from_stdin() -> Result { normalize_api_key_input(&raw) } +#[expect( + clippy::disallowed_methods, + reason = "The user explicitly selected an API-key env var as the credential source." +)] fn read_api_key_from_env_var(name: &str) -> Result { let value = std::env::var(name).with_context(|| format!("environment variable {name} is not set"))?; diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index 078eab5c7..d6bf4bec9 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -3,11 +3,12 @@ use std::str::FromStr; use anyhow::Result; pub(crate) use fabro_client::ServerTarget; -pub(crate) use fabro_config::user::{FABRO_CONFIG_ENV, active_settings_path, default_storage_dir}; +pub(crate) use fabro_config::user::{active_settings_path, default_storage_dir}; use fabro_config::user::{default_settings_path, default_socket_path}; use fabro_config::{ CliLayer, ParseError, RunSettingsBuilder, ServerSettingsBuilder, UserSettingsBuilder, }; +use fabro_static::EnvVars; use fabro_types::settings::cli::CliTargetSettings; use fabro_types::settings::{CliNamespace, InterpString, RunNamespace}; use fabro_types::{ServerSettings, UserSettings}; @@ -52,7 +53,7 @@ pub(crate) fn load_resolved_settings( } fn load_settings_document(config_path: Option<&Path>) -> anyhow::Result { - load_settings_document_with_lookup(config_path, |name| std::env::var_os(name)) + load_settings_document_with_lookup(config_path, process_env_var_os) } #[expect( @@ -65,7 +66,7 @@ fn load_settings_document_with_lookup( ) -> anyhow::Result { let config_path = config_path .map(Path::to_path_buf) - .or_else(|| lookup(FABRO_CONFIG_ENV).map(PathBuf::from)); + .or_else(|| lookup(EnvVars::FABRO_CONFIG).map(PathBuf::from)); let path = if let Some(path) = config_path { path @@ -125,7 +126,23 @@ fn storage_dir_from_document( document: &toml::Value, storage_dir: Option<&Path>, ) -> anyhow::Result { - storage_dir_from_document_with_lookup(document, storage_dir, &|name| std::env::var(name).ok()) + storage_dir_from_document_with_lookup(document, storage_dir, &process_env_var) +} + +#[expect( + clippy::disallowed_methods, + reason = "CLI settings loading owns the process-env facade for interpolation." +)] +fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() +} + +#[expect( + clippy::disallowed_methods, + reason = "CLI settings loading owns the process-env facade for config path lookup." +)] +fn process_env_var_os(name: &str) -> Option { + std::env::var_os(name) } fn storage_dir_from_document_with_lookup( diff --git a/lib/crates/fabro-cli/tests/it/cmd/install.rs b/lib/crates/fabro-cli/tests/it/cmd/install.rs index 509e4dce9..e61be1632 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/install.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/install.rs @@ -1,10 +1,10 @@ #![expect( clippy::disallowed_methods, - reason = "integration tests stage fixtures with sync std::fs; test infrastructure, not Tokio-hot path" + reason = "integration tests stage fixtures and subprocess env with sync test infrastructure" )] use fabro_config::{Storage, envfile}; -use fabro_test::{fabro_snapshot, test_context}; +use fabro_test::{EnvVars, fabro_snapshot, test_context}; use fabro_vault::{SecretType, Vault}; #[test] @@ -270,10 +270,14 @@ mode = "keep-me" std::fs::set_permissions(&fake_gh, std::fs::Permissions::from_mode(0o755)).unwrap(); } - let path = format!("{}:{}", fake_bin.display(), std::env::var("PATH").unwrap()); + let path = format!( + "{}:{}", + fake_bin.display(), + std::env::var(EnvVars::PATH).unwrap() + ); let output = context .command() - .env("PATH", path) + .env(EnvVars::PATH, path) .args([ "install", "github", diff --git a/lib/crates/fabro-cli/tests/it/cmd/upgrade.rs b/lib/crates/fabro-cli/tests/it/cmd/upgrade.rs index fd25902d6..3b0cd7a9e 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/upgrade.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/upgrade.rs @@ -1,10 +1,10 @@ #![expect( clippy::disallowed_methods, - reason = "integration tests stage fixtures with sync std::fs; test infrastructure, not Tokio-hot path" + reason = "integration tests stage fixtures and subprocess env with sync test infrastructure" )] use assert_cmd::Command; -use fabro_test::{TestContext, fabro_snapshot, test_context}; +use fabro_test::{EnvVars, TestContext, fabro_snapshot, test_context}; fn hard_link_or_copy(src: &std::path::Path, dest: &std::path::Path) { if std::fs::hard_link(src, dest).is_ok() { @@ -48,13 +48,13 @@ fn brew_command(context: &TestContext, formula: &str, version: &str) -> Command } } } - cmd.env("NO_COLOR", "1"); - cmd.env("HOME", &context.home_dir); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true") - .env("FABRO_HTTP_PROXY_POLICY", "disabled") - .env("FABRO_TELEMETRY", "off") - .env("FABRO_SERVER_MAX_CONCURRENT_RUNS", "64") - .env("FABRO_TEST_IN_MEMORY_STORE", "1"); + cmd.env(EnvVars::NO_COLOR, "1"); + cmd.env(EnvVars::HOME, &context.home_dir); + cmd.env(EnvVars::FABRO_NO_UPGRADE_CHECK, "true") + .env(EnvVars::FABRO_HTTP_PROXY_POLICY, "disabled") + .env(EnvVars::FABRO_TELEMETRY, "off") + .env(EnvVars::FABRO_SERVER_MAX_CONCURRENT_RUNS, "64") + .env(EnvVars::FABRO_TEST_IN_MEMORY_STORE, "1"); cmd } @@ -177,9 +177,13 @@ esac "[TARGET]".to_string(), )); - let path = format!("{}:{}", fake_bin.display(), std::env::var("PATH").unwrap()); + let path = format!( + "{}:{}", + fake_bin.display(), + std::env::var(EnvVars::PATH).unwrap() + ); let mut cmd = context.command(); - cmd.env("PATH", path).args(["upgrade", "--dry-run"]); + cmd.env(EnvVars::PATH, path).args(["upgrade", "--dry-run"]); fabro_snapshot!(filters, cmd, @" success: true diff --git a/lib/crates/fabro-cli/tests/it/support/mod.rs b/lib/crates/fabro-cli/tests/it/support/mod.rs index d48eb2bce..679639fe7 100644 --- a/lib/crates/fabro-cli/tests/it/support/mod.rs +++ b/lib/crates/fabro-cli/tests/it/support/mod.rs @@ -3,7 +3,7 @@ mod auth_tokens; use assert_cmd::Command; use fabro_store::EventEnvelope; -use fabro_test::{TestContext, preserve_coverage_env}; +use fabro_test::{EnvVars, TestContext, preserve_coverage_env}; use fabro_types::RunId; macro_rules! fabro_json_snapshot { ($context:expr, $value:expr, @$snapshot:literal) => {{ @@ -99,17 +99,21 @@ impl LightweightCli { } } + #[expect( + clippy::disallowed_methods, + reason = "Lightweight CLI test harness reconstructs a minimal process env for subprocesses." + )] pub(crate) fn command(&self) -> Command { let mut cmd = Command::new(env!("CARGO_BIN_EXE_fabro")); cmd.env_clear(); preserve_coverage_env!(cmd); - if let Some(path) = std::env::var_os("PATH") { - cmd.env("PATH", path); + if let Some(path) = std::env::var_os(EnvVars::PATH) { + cmd.env(EnvVars::PATH, path); } - cmd.env("HOME", self.home_dir.path()); - cmd.env("NO_COLOR", "1"); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true") - .env("FABRO_HTTP_PROXY_POLICY", "disabled"); + cmd.env(EnvVars::HOME, self.home_dir.path()); + cmd.env(EnvVars::NO_COLOR, "1"); + cmd.env(EnvVars::FABRO_NO_UPGRADE_CHECK, "true") + .env(EnvVars::FABRO_HTTP_PROXY_POLICY, "disabled"); cmd.current_dir(self.home_dir.path()); cmd } diff --git a/lib/crates/fabro-client/Cargo.toml b/lib/crates/fabro-client/Cargo.toml index e0c426a1c..e2601ad8b 100644 --- a/lib/crates/fabro-client/Cargo.toml +++ b/lib/crates/fabro-client/Cargo.toml @@ -19,6 +19,7 @@ chrono = { workspace = true, features = ["serde"] } fabro-api = { path = "../fabro-api" } fabro-http.workspace = true fabro-model = { path = "../fabro-model" } +fabro-static.workspace = true fabro-types = { path = "../fabro-types" } fabro-util = { path = "../fabro-util" } fs2.workspace = true diff --git a/lib/crates/fabro-client/src/auth_store.rs b/lib/crates/fabro-client/src/auth_store.rs index e436e3e78..a66455d26 100644 --- a/lib/crates/fabro-client/src/auth_store.rs +++ b/lib/crates/fabro-client/src/auth_store.rs @@ -13,6 +13,7 @@ use std::io::Write as _; use std::path::{Path, PathBuf}; use chrono::{DateTime, Utc}; +use fabro_static::EnvVars; use fs2::FileExt; use rand::Rng; use serde::{Deserialize, Serialize}; @@ -20,8 +21,6 @@ use thiserror::Error; use crate::target::ServerTarget; -const AUTH_FILE_ENV: &str = "FABRO_AUTH_FILE"; - #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct StoredSubject { pub idp_issuer: String, @@ -83,7 +82,8 @@ pub enum AuthStoreError { #[derive(Debug, Error)] pub enum LockError { #[error( - "the filesystem backing {path} does not support file locking; move the auth store to a local filesystem or set {AUTH_FILE_ENV} to a local path" + "the filesystem backing {path} does not support file locking; move the auth store to a local filesystem or set {env} to a local path", + env = EnvVars::FABRO_AUTH_FILE )] FilesystemDoesNotSupportLocking { path: PathBuf }, #[error("failed to lock auth store at {path}: {source}")] @@ -106,7 +106,7 @@ struct AuthFile { impl Default for AuthStore { fn default() -> Self { - let path = std::env::var_os(AUTH_FILE_ENV).map_or_else( + let path = std::env::var_os(EnvVars::FABRO_AUTH_FILE).map_or_else( || fabro_util::Home::from_env().root().join("auth.json"), PathBuf::from, ); @@ -362,10 +362,11 @@ mod tests { use std::thread; use chrono::Duration; + use fabro_static::EnvVars; - #[cfg(unix)] - use super::{AUTH_FILE_ENV, LockError, classify_lock_error}; use super::{AuthEntry, AuthStore, StoredSubject, key_for_target}; + #[cfg(unix)] + use super::{LockError, classify_lock_error}; use crate::target::ServerTarget; fn entry(login: &str) -> AuthEntry { @@ -552,6 +553,6 @@ mod tests { err, LockError::FilesystemDoesNotSupportLocking { path: ref error_path } if error_path == &path )); - assert!(err.to_string().contains(AUTH_FILE_ENV)); + assert!(err.to_string().contains(EnvVars::FABRO_AUTH_FILE)); } } diff --git a/lib/crates/fabro-config/Cargo.toml b/lib/crates/fabro-config/Cargo.toml index 65252066c..c837202cd 100644 --- a/lib/crates/fabro-config/Cargo.toml +++ b/lib/crates/fabro-config/Cargo.toml @@ -22,6 +22,7 @@ clap = { workspace = true, optional = true } chrono.workspace = true fabro-macros = { path = "../fabro-macros" } fabro-proc = { path = "../fabro-proc" } +fabro-static.workspace = true fabro-types = { path = "../fabro-types" } fabro-util = { path = "../fabro-util" } dirs.workspace = true diff --git a/lib/crates/fabro-config/src/run.rs b/lib/crates/fabro-config/src/run.rs index 86e4f0866..08e5f99cf 100644 --- a/lib/crates/fabro-config/src/run.rs +++ b/lib/crates/fabro-config/src/run.rs @@ -95,7 +95,7 @@ fn resolve_goal_file( base_dir: &Path, ) -> std::result::Result { let resolved = file - .resolve(|name| std::env::var(name).ok()) + .resolve(process_env_var) .map_err(|err| ResolveRunGoalError::EnvLookup { var: err.name })?; let path = resolve_goal_file_path(&resolved.value, base_dir); let text = std::fs::read_to_string(&path).map_err(|source| ResolveRunGoalError::Io { @@ -108,6 +108,14 @@ fn resolve_goal_file( }) } +#[expect( + clippy::disallowed_methods, + reason = "Run config interpolation owns a process-env lookup facade for {{ env.* }} values." +)] +fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() +} + fn resolve_layer_goal( goal: &RunGoalLayer, base_dir: &Path, diff --git a/lib/crates/fabro-config/src/user.rs b/lib/crates/fabro-config/src/user.rs index 49f36a0f4..030a234b2 100644 --- a/lib/crates/fabro-config/src/user.rs +++ b/lib/crates/fabro-config/src/user.rs @@ -6,12 +6,13 @@ use std::path::{Path, PathBuf}; +use fabro_static::EnvVars; + use crate::home::Home; use crate::load::load_settings_path; use crate::{Result, SettingsLayer}; pub const SETTINGS_CONFIG_FILENAME: &str = "settings.toml"; -pub const FABRO_CONFIG_ENV: &str = "FABRO_CONFIG"; pub fn default_settings_path() -> PathBuf { Home::from_env().user_config() @@ -25,6 +26,10 @@ pub fn default_socket_path() -> PathBuf { Home::from_env().root().join("fabro.sock") } +#[expect( + clippy::disallowed_methods, + reason = "Config loading owns the process-env facade used to resolve user settings paths." +)] pub fn active_settings_path(path: Option<&Path>) -> PathBuf { active_settings_path_with_lookup(path, |name| std::env::var_os(name)) } @@ -34,17 +39,21 @@ fn active_settings_path_with_lookup( lookup: impl Fn(&str) -> Option, ) -> PathBuf { path.map(Path::to_path_buf) - .or_else(|| lookup(FABRO_CONFIG_ENV).map(PathBuf::from)) + .or_else(|| lookup(EnvVars::FABRO_CONFIG).map(PathBuf::from)) .unwrap_or_else(default_settings_path) } /// Load settings config from an explicit path or `~/.fabro/settings.toml`, /// returning defaults if the default file doesn't exist. An explicit path that /// doesn't exist is an error. +#[expect( + clippy::disallowed_methods, + reason = "Config loading owns the process-env facade used to resolve user settings paths." +)] pub(crate) fn load_settings_config(path: Option<&Path>) -> Result { if let Some(explicit) = path .map(Path::to_path_buf) - .or_else(|| std::env::var_os(FABRO_CONFIG_ENV).map(PathBuf::from)) + .or_else(|| std::env::var_os(EnvVars::FABRO_CONFIG).map(PathBuf::from)) { return load_v2_layer_from_path(&explicit); } diff --git a/lib/crates/fabro-devcontainer/Cargo.toml b/lib/crates/fabro-devcontainer/Cargo.toml index 89a26c394..5139fab07 100644 --- a/lib/crates/fabro-devcontainer/Cargo.toml +++ b/lib/crates/fabro-devcontainer/Cargo.toml @@ -13,6 +13,7 @@ doctest = false workspace = true [dependencies] +fabro-static.workspace = true fabro-util = { path = "../fabro-util" } fabro-http.workspace = true serde = { workspace = true } diff --git a/lib/crates/fabro-devcontainer/src/features.rs b/lib/crates/fabro-devcontainer/src/features.rs index 2717bd448..14aef01ee 100644 --- a/lib/crates/fabro-devcontainer/src/features.rs +++ b/lib/crates/fabro-devcontainer/src/features.rs @@ -2,6 +2,7 @@ use std::collections::{HashMap, HashSet, VecDeque}; use std::fmt::Write; use std::path::Path; +use fabro_static::EnvVars; use tokio::fs; use tokio::process::Command; use tracing::info; @@ -63,6 +64,10 @@ fn dir_name_from_id(feature_id: &str) -> String { } /// Ensure `oras` CLI is available, installing it if necessary. +#[expect( + clippy::disallowed_methods, + reason = "OCI feature fetching installs oras under the user's HOME on Linux." +)] async fn ensure_oras() -> crate::Result<()> { let check = Command::new("which") .arg("oras") @@ -92,7 +97,7 @@ async fn ensure_oras() -> crate::Result<()> { } } else { // Linux: download from GitHub releases to ~/.local/bin/ - let home = std::env::var("HOME") + let home = std::env::var(EnvVars::HOME) .map_err(|_| DevcontainerError::OrasInstall("HOME not set".to_string()))?; let bin_dir = format!("{home}/.local/bin"); @@ -1089,8 +1094,12 @@ mod tests { #[tokio::test] #[ignore = "requires oras"] + #[expect( + clippy::disallowed_methods, + reason = "Ignored OCI integration test is opt-in via a documented process-env flag." + )] async fn fetch_feature_oci_integration() { - if std::env::var_os("FABRO_ENABLE_FETCH_FEATURE_OCI_INTEGRATION").is_none() { + if std::env::var_os(EnvVars::FABRO_ENABLE_FETCH_FEATURE_OCI_INTEGRATION).is_none() { return; } diff --git a/lib/crates/fabro-github/Cargo.toml b/lib/crates/fabro-github/Cargo.toml index 416e17a2a..69603226e 100644 --- a/lib/crates/fabro-github/Cargo.toml +++ b/lib/crates/fabro-github/Cargo.toml @@ -16,6 +16,7 @@ workspace = true serde.workspace = true serde_json.workspace = true fabro-http.workspace = true +fabro-static.workspace = true fabro-types = { path = "../fabro-types" } jsonwebtoken.workspace = true chrono.workspace = true diff --git a/lib/crates/fabro-github/src/lib.rs b/lib/crates/fabro-github/src/lib.rs index 521e45415..2adfa0a0f 100644 --- a/lib/crates/fabro-github/src/lib.rs +++ b/lib/crates/fabro-github/src/lib.rs @@ -1,5 +1,6 @@ use base64::Engine; use base64::engine::general_purpose::STANDARD; +use fabro_static::EnvVars; use fabro_types::PullRequestGithubDetail; use fabro_types::settings::run::MergeStrategy; use serde::Deserialize; @@ -9,8 +10,12 @@ pub const GITHUB_API_BASE_URL: &str = "https://api.github.com"; /// Returns the GitHub API base URL, allowing override via `GITHUB_BASE_URL` env /// var. +#[expect( + clippy::disallowed_methods, + reason = "GitHub API client exposes a documented process-env base URL override." +)] pub fn github_api_base_url() -> String { - std::env::var("GITHUB_BASE_URL").unwrap_or_else(|_| GITHUB_API_BASE_URL.to_string()) + std::env::var(EnvVars::GITHUB_BASE_URL).unwrap_or_else(|_| GITHUB_API_BASE_URL.to_string()) } /// Bundle of GitHub credentials and the API base URL, threaded through every @@ -94,11 +99,15 @@ pub struct GitHubAppCredentials { } impl GitHubAppCredentials { + #[expect( + clippy::disallowed_methods, + reason = "GitHub App credentials support a documented private-key env source." + )] pub fn private_key_from_env() -> Result, String> { - let Ok(raw) = std::env::var("GITHUB_APP_PRIVATE_KEY") else { + let Ok(raw) = std::env::var(EnvVars::GITHUB_APP_PRIVATE_KEY) else { return Ok(None); }; - decode_pem_env("GITHUB_APP_PRIVATE_KEY", &raw).map(Some) + decode_pem_env(EnvVars::GITHUB_APP_PRIVATE_KEY, &raw).map(Some) } pub fn from_env(app_id: Option<&str>) -> Result, String> { diff --git a/lib/crates/fabro-http/Cargo.toml b/lib/crates/fabro-http/Cargo.toml index 6629d9879..6b9706fda 100644 --- a/lib/crates/fabro-http/Cargo.toml +++ b/lib/crates/fabro-http/Cargo.toml @@ -13,6 +13,7 @@ doctest = false workspace = true [dependencies] +fabro-static.workspace = true reqwest = { workspace = true, features = ["blocking", "cookies"] } thiserror.workspace = true diff --git a/lib/crates/fabro-http/src/lib.rs b/lib/crates/fabro-http/src/lib.rs index 06d94ad56..27ef1414d 100644 --- a/lib/crates/fabro-http/src/lib.rs +++ b/lib/crates/fabro-http/src/lib.rs @@ -8,6 +8,7 @@ use std::path::Path; use std::time::Duration; +use fabro_static::EnvVars; pub use reqwest::header::{HeaderMap, HeaderName, HeaderValue}; pub use reqwest::{ Body, Method, RequestBuilder, Response, StatusCode, Url, header, multipart, tls, @@ -19,8 +20,6 @@ pub type BlockingRequestBuilder = reqwest::blocking::RequestBuilder; pub type BlockingResponse = reqwest::blocking::Response; pub type Proxy = reqwest::Proxy; -pub const HTTP_PROXY_POLICY_ENV: &str = "FABRO_HTTP_PROXY_POLICY"; - #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum ProxyPolicy { System, @@ -51,7 +50,7 @@ impl ProxyPolicy { } fn resolve(explicit: Option) -> Result { - match std::env::var(HTTP_PROXY_POLICY_ENV) { + match std::env::var(EnvVars::FABRO_HTTP_PROXY_POLICY) { Ok(value) => Self::resolve_with_env_value(explicit, Some(&value)), Err(std::env::VarError::NotPresent) => Self::resolve_with_env_value(explicit, None), Err(std::env::VarError::NotUnicode(value)) => Err( @@ -63,7 +62,7 @@ impl ProxyPolicy { #[derive(Debug, thiserror::Error)] pub enum HttpClientBuildError { - #[error("invalid {HTTP_PROXY_POLICY_ENV} value `{0}`; expected `system` or `disabled`")] + #[error("invalid {env} value `{0}`; expected `system` or `disabled`", env = EnvVars::FABRO_HTTP_PROXY_POLICY)] InvalidProxyPolicy(String), #[error(transparent)] diff --git a/lib/crates/fabro-install/Cargo.toml b/lib/crates/fabro-install/Cargo.toml index 8c56b059f..025a87fb4 100644 --- a/lib/crates/fabro-install/Cargo.toml +++ b/lib/crates/fabro-install/Cargo.toml @@ -15,6 +15,7 @@ base64.workspace = true ring = "0.17" toml.workspace = true fabro-config = { path = "../fabro-config" } +fabro-static.workspace = true fabro-types = { path = "../fabro-types" } fabro-vault = { path = "../fabro-vault" } diff --git a/lib/crates/fabro-install/src/lib.rs b/lib/crates/fabro-install/src/lib.rs index e4ebf580c..2dc1cde21 100644 --- a/lib/crates/fabro-install/src/lib.rs +++ b/lib/crates/fabro-install/src/lib.rs @@ -7,6 +7,7 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; use fabro_config::{Storage, envfile}; +use fabro_static::EnvVars; use fabro_vault::{SecretType as VaultSecretType, Vault}; pub struct PendingSettingsWrite<'a> { @@ -16,8 +17,8 @@ pub struct PendingSettingsWrite<'a> { } pub const OBJECT_STORE_MANAGED_COMMENT: &str = "managed by fabro-install: object-store"; -pub const OBJECT_STORE_ACCESS_KEY_ID_ENV: &str = "AWS_ACCESS_KEY_ID"; -pub const OBJECT_STORE_SECRET_ACCESS_KEY_ENV: &str = "AWS_SECRET_ACCESS_KEY"; +pub const OBJECT_STORE_ACCESS_KEY_ID_ENV: &str = EnvVars::AWS_ACCESS_KEY_ID; +pub const OBJECT_STORE_SECRET_ACCESS_KEY_ENV: &str = EnvVars::AWS_SECRET_ACCESS_KEY; #[derive(Debug, Clone, PartialEq, Eq)] pub struct VaultSecretWrite { diff --git a/lib/crates/fabro-llm/Cargo.toml b/lib/crates/fabro-llm/Cargo.toml index 648b1880b..a298c54c4 100644 --- a/lib/crates/fabro-llm/Cargo.toml +++ b/lib/crates/fabro-llm/Cargo.toml @@ -35,6 +35,7 @@ tracing.workspace = true fabro-http.workspace = true fabro-auth = { path = "../fabro-auth" } fabro-model = { path = "../fabro-model" } +fabro-static.workspace = true fabro-util = { path = "../fabro-util" } [dev-dependencies] diff --git a/lib/crates/fabro-llm/src/providers/common.rs b/lib/crates/fabro-llm/src/providers/common.rs index 9b592bfbf..7f7b84b51 100644 --- a/lib/crates/fabro-llm/src/providers/common.rs +++ b/lib/crates/fabro-llm/src/providers/common.rs @@ -1,6 +1,7 @@ use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use fabro_http::HeaderMap; +use fabro_static::EnvVars; use tokio::{fs, time}; use tracing::warn; @@ -92,11 +93,15 @@ pub fn mime_from_extension(path: &str) -> &str { /// /// # Errors /// Returns an error if the file cannot be read. +#[expect( + clippy::disallowed_methods, + reason = "Attachment path expansion supports the conventional HOME env var." +)] pub async fn load_file_as_base64(path: &str) -> Result<(String, String), std::io::Error> { let expanded = path.strip_prefix("~/").map_or_else( || path.to_string(), |rest| { - let home = std::env::var("HOME").unwrap_or_else(|_| "/".to_string()); + let home = std::env::var(EnvVars::HOME).unwrap_or_else(|_| "/".to_string()); format!("{home}/{rest}") }, ); diff --git a/lib/crates/fabro-llm/tests/integration.rs b/lib/crates/fabro-llm/tests/integration.rs index 24199cf1f..c1ba43463 100644 --- a/lib/crates/fabro-llm/tests/integration.rs +++ b/lib/crates/fabro-llm/tests/integration.rs @@ -1,7 +1,13 @@ +#![expect( + clippy::disallowed_methods, + reason = "Live provider integration tests read required API keys from process env." +)] + use fabro_llm::error::ProviderErrorKind; use fabro_llm::provider::ProviderAdapter; use fabro_llm::providers::{AnthropicAdapter, GeminiAdapter, OpenAiAdapter}; use fabro_llm::types::{FinishReason, Message, Request}; +use fabro_static::EnvVars; fn make_request(model: &str) -> Request { Request { @@ -24,7 +30,7 @@ fn make_request(model: &str) -> Request { #[fabro_macros::e2e_test(live("ANTHROPIC_API_KEY"))] async fn anthropic_complete() { - let api_key = std::env::var("ANTHROPIC_API_KEY").expect("ANTHROPIC_API_KEY must be set"); + let api_key = std::env::var(EnvVars::ANTHROPIC_API_KEY).expect("ANTHROPIC_API_KEY must be set"); let adapter = AnthropicAdapter::new(api_key); let request = make_request("claude-haiku-4-5"); let response = adapter.complete(&request).await.unwrap(); @@ -108,7 +114,7 @@ async fn openai_server_error() { #[fabro_macros::e2e_test(live("GEMINI_API_KEY"))] async fn gemini_complete() { - let api_key = std::env::var("GEMINI_API_KEY").expect("GEMINI_API_KEY must be set"); + let api_key = std::env::var(EnvVars::GEMINI_API_KEY).expect("GEMINI_API_KEY must be set"); let adapter = GeminiAdapter::new(api_key); let request = make_request("gemini-2.5-flash"); let response = adapter.complete(&request).await.unwrap(); @@ -202,21 +208,21 @@ async fn run_multi_turn_cache_test( #[fabro_macros::e2e_test(live("ANTHROPIC_API_KEY"))] async fn anthropic_multi_turn_cache() { - let api_key = std::env::var("ANTHROPIC_API_KEY").expect("ANTHROPIC_API_KEY must be set"); + let api_key = std::env::var(EnvVars::ANTHROPIC_API_KEY).expect("ANTHROPIC_API_KEY must be set"); let adapter = AnthropicAdapter::new(api_key); run_multi_turn_cache_test(&adapter, "claude-haiku-4-5", 0.5).await; } #[fabro_macros::e2e_test(live("OPENAI_API_KEY"))] async fn openai_multi_turn_cache() { - let api_key = std::env::var("OPENAI_API_KEY").expect("OPENAI_API_KEY must be set"); + let api_key = std::env::var(EnvVars::OPENAI_API_KEY).expect("OPENAI_API_KEY must be set"); let adapter = OpenAiAdapter::new(api_key); run_multi_turn_cache_test(&adapter, "gpt-4o-mini", 0.5).await; } #[fabro_macros::e2e_test(live("GEMINI_API_KEY"))] async fn gemini_multi_turn_cache() { - let api_key = std::env::var("GEMINI_API_KEY").expect("GEMINI_API_KEY must be set"); + let api_key = std::env::var(EnvVars::GEMINI_API_KEY).expect("GEMINI_API_KEY must be set"); let adapter = GeminiAdapter::new(api_key); run_multi_turn_cache_test(&adapter, "gemini-2.5-flash", 0.5).await; } diff --git a/lib/crates/fabro-macros/src/lib.rs b/lib/crates/fabro-macros/src/lib.rs index aaf508520..31007fd75 100644 --- a/lib/crates/fabro-macros/src/lib.rs +++ b/lib/crates/fabro-macros/src/lib.rs @@ -100,12 +100,21 @@ pub fn e2e_test(attr: TokenStream, item: TokenStream) -> TokenStream { let env_guards = if env_vars.is_empty() { quote! {} } else { + let env_lookup_helper = quote! { + #[expect( + clippy::disallowed_methods, + reason = "e2e_test live-mode guard intentionally checks process env for declared live secrets." + )] + fn __fabro_e2e_env_var_is_missing(name: &str) -> bool { + ::std::env::var(name).is_err() + } + }; let guards = env_vars.iter().map(|env_var| { let env_name = env_var.value(); let strict_message = format!("{env_name} not set (FABRO_TEST_MODE=strict)"); let skip_message = format!("skipping: {env_name} not set"); quote! { - if ::std::env::var(#env_var).is_err() { + if __fabro_e2e_env_var_is_missing(#env_var) { if __mode == ::fabro_test::TestMode::Strict { panic!(#strict_message); } @@ -118,6 +127,8 @@ pub fn e2e_test(attr: TokenStream, item: TokenStream) -> TokenStream { if has_twin { // dual-mode: only check env vars when in live/strict quote! { + #env_lookup_helper + if __mode.is_live() { #(#guards)* } @@ -125,6 +136,8 @@ pub fn e2e_test(attr: TokenStream, item: TokenStream) -> TokenStream { } else { // live-only: always check env vars (mode guard already skipped twin) quote! { + #env_lookup_helper + #(#guards)* } } diff --git a/lib/crates/fabro-model/Cargo.toml b/lib/crates/fabro-model/Cargo.toml index e45b29d6a..70fc3a44c 100644 --- a/lib/crates/fabro-model/Cargo.toml +++ b/lib/crates/fabro-model/Cargo.toml @@ -13,6 +13,7 @@ doctest = false workspace = true [dependencies] +fabro-static.workspace = true serde.workspace = true serde_json.workspace = true strum.workspace = true diff --git a/lib/crates/fabro-model/src/provider.rs b/lib/crates/fabro-model/src/provider.rs index 2e0787417..966a63075 100644 --- a/lib/crates/fabro-model/src/provider.rs +++ b/lib/crates/fabro-model/src/provider.rs @@ -1,3 +1,4 @@ +use fabro_static::EnvVars; use serde::{Deserialize, Serialize}; use strum::{Display, EnumString, IntoStaticStr}; @@ -55,13 +56,13 @@ impl Provider { #[must_use] pub fn api_key_env_vars(self) -> &'static [&'static str] { match self { - Self::Anthropic => &["ANTHROPIC_API_KEY"], - Self::OpenAi => &["OPENAI_API_KEY"], - Self::Gemini => &["GEMINI_API_KEY", "GOOGLE_API_KEY"], - Self::Kimi => &["KIMI_API_KEY"], - Self::Zai => &["ZAI_API_KEY"], - Self::Minimax => &["MINIMAX_API_KEY"], - Self::Inception => &["INCEPTION_API_KEY"], + Self::Anthropic => &[EnvVars::ANTHROPIC_API_KEY], + Self::OpenAi => &[EnvVars::OPENAI_API_KEY], + Self::Gemini => &[EnvVars::GEMINI_API_KEY, EnvVars::GOOGLE_API_KEY], + Self::Kimi => &[EnvVars::KIMI_API_KEY], + Self::Zai => &[EnvVars::ZAI_API_KEY], + Self::Minimax => &[EnvVars::MINIMAX_API_KEY], + Self::Inception => &[EnvVars::INCEPTION_API_KEY], Self::OpenAiCompatible => &[], } } @@ -69,6 +70,10 @@ impl Provider { /// Returns `true` if at least one of the provider's API key env vars is /// set. #[must_use] + #[expect( + clippy::disallowed_methods, + reason = "Provider discovery intentionally checks the process env for known API-key names." + )] pub fn has_api_key(self) -> bool { self.api_key_env_vars() .iter() diff --git a/lib/crates/fabro-oauth/Cargo.toml b/lib/crates/fabro-oauth/Cargo.toml index 1baa78079..68261dccb 100644 --- a/lib/crates/fabro-oauth/Cargo.toml +++ b/lib/crates/fabro-oauth/Cargo.toml @@ -23,6 +23,7 @@ hex.workspace = true tokio.workspace = true tracing.workspace = true axum.workspace = true +fabro-static.workspace = true fabro-util = { path = "../fabro-util" } [dev-dependencies] diff --git a/lib/crates/fabro-oauth/examples/login.rs b/lib/crates/fabro-oauth/examples/login.rs index 2311c3d57..634fbb7a4 100644 --- a/lib/crates/fabro-oauth/examples/login.rs +++ b/lib/crates/fabro-oauth/examples/login.rs @@ -1,23 +1,27 @@ #![allow( clippy::print_stdout, clippy::print_stderr, - reason = "This example intentionally writes normal output and diagnostics to stdio." + clippy::disallowed_methods, + reason = "This example intentionally reads OAuth env vars and writes normal output/diagnostics." )] use std::env; use fabro_oauth::run_browser_flow; +use fabro_static::EnvVars; #[tokio::main] async fn main() { - let issuer = env::var("OAUTH_ISSUER").expect("set OAUTH_ISSUER"); - let client_id = env::var("OAUTH_CLIENT_ID").expect("set OAUTH_CLIENT_ID"); - let scope = env::var("OAUTH_SCOPE").unwrap_or_else(|_| "openid profile email".to_string()); - let port: u16 = env::var("OAUTH_PORT") + let issuer = env::var(EnvVars::OAUTH_ISSUER).expect("set OAUTH_ISSUER"); + let client_id = env::var(EnvVars::OAUTH_CLIENT_ID).expect("set OAUTH_CLIENT_ID"); + let scope = + env::var(EnvVars::OAUTH_SCOPE).unwrap_or_else(|_| "openid profile email".to_string()); + let port: u16 = env::var(EnvVars::OAUTH_PORT) .ok() .and_then(|value| value.parse().ok()) .unwrap_or(0); - let callback_path = env::var("OAUTH_CALLBACK_PATH").unwrap_or_else(|_| "/callback".to_string()); + let callback_path = + env::var(EnvVars::OAUTH_CALLBACK_PATH).unwrap_or_else(|_| "/callback".to_string()); match run_browser_flow(&issuer, &client_id, &scope, port, &callback_path).await { Ok(tokens) => { diff --git a/lib/crates/fabro-proc/build.rs b/lib/crates/fabro-proc/build.rs index 2c97ef50c..5f06d7639 100644 --- a/lib/crates/fabro-proc/build.rs +++ b/lib/crates/fabro-proc/build.rs @@ -1,3 +1,8 @@ +#![allow( + clippy::disallowed_methods, + reason = "Build scripts run at compile time and read Cargo-provided env vars." +)] + fn main() { println!("cargo:rerun-if-changed=c/capture_argv.c"); diff --git a/lib/crates/fabro-sandbox/Cargo.toml b/lib/crates/fabro-sandbox/Cargo.toml index 5b03e9039..7fa117d27 100644 --- a/lib/crates/fabro-sandbox/Cargo.toml +++ b/lib/crates/fabro-sandbox/Cargo.toml @@ -30,6 +30,7 @@ strum.workspace = true tracing.workspace = true base64.workspace = true fabro-proc = { path = "../fabro-proc" } +fabro-static.workspace = true shlex = "1" # local diff --git a/lib/crates/fabro-sandbox/src/local.rs b/lib/crates/fabro-sandbox/src/local.rs index af6d54ddd..c32162a48 100644 --- a/lib/crates/fabro-sandbox/src/local.rs +++ b/lib/crates/fabro-sandbox/src/local.rs @@ -2,6 +2,7 @@ use std::path::{Path, PathBuf}; use std::time::Instant; use async_trait::async_trait; +use fabro_static::EnvVars; use tokio::io::AsyncReadExt; use tokio::process::{Child, Command}; use tokio::task::spawn_blocking; @@ -41,16 +42,16 @@ impl LocalSandbox { } const ENV_SAFELIST: &'static [&'static str] = &[ - "PATH", - "HOME", - "USER", - "SHELL", - "LANG", - "TERM", - "TMPDIR", - "GOPATH", - "CARGO_HOME", - "NVM_DIR", + EnvVars::PATH, + EnvVars::HOME, + EnvVars::USER, + EnvVars::SHELL, + EnvVars::LANG, + EnvVars::TERM, + EnvVars::TMPDIR, + EnvVars::GOPATH, + EnvVars::CARGO_HOME, + EnvVars::NVM_DIR, ]; fn should_filter_env_var(key: &str) -> bool { @@ -74,13 +75,17 @@ impl LocalSandbox { } } + #[expect( + clippy::disallowed_methods, + reason = "Local sandbox command execution checks PATH/PATHEXT to select optional helpers." + )] fn binary_on_path(binary: &str) -> bool { - let Some(paths) = std::env::var_os("PATH") else { + let Some(paths) = std::env::var_os(EnvVars::PATH) else { return false; }; #[cfg(windows)] - let extensions: Vec = std::env::var_os("PATHEXT") + let extensions: Vec = std::env::var_os(EnvVars::PATHEXT) .map(|value| { value .to_string_lossy() @@ -112,6 +117,14 @@ impl LocalSandbox { } } +#[expect( + clippy::disallowed_methods, + reason = "Local sandbox must snapshot the ambient process env before applying its fail-closed filter." +)] +fn process_env_vars() -> Vec<(String, String)> { + std::env::vars().collect() +} + #[async_trait] impl Sandbox for LocalSandbox { async fn read_file( @@ -221,7 +234,8 @@ impl Sandbox for LocalSandbox { ) -> Result { let start = Instant::now(); - let mut filtered_env: Vec<(String, String)> = std::env::vars() + let mut filtered_env: Vec<(String, String)> = process_env_vars() + .into_iter() .filter(|(key, _)| !Self::should_filter_env_var(key)) .collect(); diff --git a/lib/crates/fabro-server/Cargo.toml b/lib/crates/fabro-server/Cargo.toml index a7851a872..51655e3f9 100644 --- a/lib/crates/fabro-server/Cargo.toml +++ b/lib/crates/fabro-server/Cargo.toml @@ -36,6 +36,7 @@ fabro-api = { path = "../fabro-api" } fabro-store = { path = "../fabro-store" } fabro-vault = { path = "../fabro-vault" } fabro-http.workspace = true +fabro-static.workspace = true chrono.workspace = true futures-util.workspace = true axum.workspace = true diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index 160480a7a..5477bb2d1 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -6,6 +6,7 @@ use fabro_auth::auth_issue_message; use fabro_llm::client::Client as LlmClient; use fabro_llm::types::{Message, Request}; use fabro_model::{Catalog, Provider}; +use fabro_static::EnvVars; use fabro_types::settings::server::GithubIntegrationStrategy; use fabro_types::settings::{InterpString, ServerAuthMethod}; use fabro_util::check_report::{CheckDetail, CheckResult, CheckSection, CheckStatus}; @@ -276,10 +277,14 @@ async fn check_github_app(state: &AppState) -> CheckResult { .slug .as_ref() .map(InterpString::as_source); - let private_key_raw = state.server_secret("GITHUB_APP_PRIVATE_KEY"); + let private_key_raw = state.server_secret(EnvVars::GITHUB_APP_PRIVATE_KEY); let client_id = settings.server.integrations.github.client_id.is_some(); - let client_secret = state.server_secret("GITHUB_APP_CLIENT_SECRET").is_some(); - let webhook_secret = state.server_secret("GITHUB_APP_WEBHOOK_SECRET").is_some(); + let client_secret = state + .server_secret(EnvVars::GITHUB_APP_CLIENT_SECRET) + .is_some(); + let webhook_secret = state + .server_secret(EnvVars::GITHUB_APP_WEBHOOK_SECRET) + .is_some(); if app_id.is_none() && private_key_raw.is_none() @@ -317,7 +322,7 @@ async fn check_github_app(state: &AppState) -> CheckResult { }; }; - let private_key = match decode_pem_value("GITHUB_APP_PRIVATE_KEY", &private_key_raw) { + let private_key = match decode_pem_value(EnvVars::GITHUB_APP_PRIVATE_KEY, &private_key_raw) { Ok(value) => value, Err(err) => { return CheckResult { @@ -378,7 +383,7 @@ async fn check_github_app(state: &AppState) -> CheckResult { } fn check_sandbox(state: &AppState) -> CheckResult { - if state.vault_or_env("DAYTONA_API_KEY").is_some() { + if state.vault_or_env(EnvVars::DAYTONA_API_KEY).is_some() { CheckResult { name: "Sandbox".to_string(), status: CheckStatus::Pass, @@ -440,7 +445,7 @@ fn check_storage_dir_path(path: &std::path::Path) -> CheckResult { } async fn check_brave_search(state: &AppState) -> CheckResult { - let Some(api_key) = state.vault_or_env("BRAVE_SEARCH_API_KEY") else { + let Some(api_key) = state.vault_or_env(EnvVars::BRAVE_SEARCH_API_KEY) else { return CheckResult { name: "Web Search (Brave)".to_string(), status: CheckStatus::Warning, @@ -507,7 +512,7 @@ fn check_crypto(state: &AppState) -> CheckResult { let mut errors = Vec::new(); if resolved_server_settings.server.web.enabled { - match state.server_secret("SESSION_SECRET") { + match state.server_secret(EnvVars::SESSION_SECRET) { Some(secret) => { if let Err(err) = validate_session_secret(&secret) { errors.push(err); @@ -519,7 +524,7 @@ fn check_crypto(state: &AppState) -> CheckResult { let methods = &resolved_server_settings.server.auth.methods; if methods.contains(&ServerAuthMethod::DevToken) { - match state.server_secret("FABRO_DEV_TOKEN") { + match state.server_secret(EnvVars::FABRO_DEV_TOKEN) { Some(token) if validate_dev_token_format(&token) => {} Some(_) => errors.push("FABRO_DEV_TOKEN has invalid format".to_string()), None => errors.push("FABRO_DEV_TOKEN not set".to_string()), @@ -535,7 +540,10 @@ fn check_crypto(state: &AppState) -> CheckResult { { errors.push("server.integrations.github.client_id is not configured".to_string()); } - if state.server_secret("GITHUB_APP_CLIENT_SECRET").is_none() { + if state + .server_secret(EnvVars::GITHUB_APP_CLIENT_SECRET) + .is_none() + { errors.push("GITHUB_APP_CLIENT_SECRET not set".to_string()); } } diff --git a/lib/crates/fabro-server/src/github_webhooks.rs b/lib/crates/fabro-server/src/github_webhooks.rs index caef9374e..dfc74491c 100644 --- a/lib/crates/fabro-server/src/github_webhooks.rs +++ b/lib/crates/fabro-server/src/github_webhooks.rs @@ -1,3 +1,4 @@ +use fabro_static::EnvVars; use hmac::{Hmac, Mac}; use sha2::Sha256; use tokio::process::Command; @@ -6,7 +7,7 @@ use tracing::{info, warn}; type HmacSha256 = Hmac; /// Name of the server secret holding the GitHub App webhook HMAC key. -pub(crate) const WEBHOOK_SECRET_ENV: &str = "GITHUB_APP_WEBHOOK_SECRET"; +pub(crate) const WEBHOOK_SECRET_ENV: &str = EnvVars::GITHUB_APP_WEBHOOK_SECRET; /// Route path where Fabro receives GitHub App webhook deliveries. pub(crate) const WEBHOOK_ROUTE: &str = "/api/v1/webhooks/github"; diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index b7bea4926..c793c308d 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -22,6 +22,7 @@ use fabro_install::{ write_github_app_settings, write_object_store_settings, write_token_settings, }; use fabro_model::Provider; +use fabro_static::EnvVars; use fabro_store::ArtifactStore; use fabro_types::ServerSettings; use fabro_types::settings::interp::InterpString; @@ -79,7 +80,6 @@ const DEFAULT_GEMINI_BASE_URL: &str = "https://generativelanguage.googleapis.com const REDACTED_SECRET_VALUE: &str = "[REDACTED]"; const VALIDATION_TIMEOUT: Duration = Duration::from_secs(20); const VALIDATION_CONNECT_TIMEOUT: Duration = Duration::from_secs(5); -const AWS_SESSION_TOKEN_ENV: &str = "AWS_SESSION_TOKEN"; impl InstallAppState { #[must_use] @@ -122,7 +122,7 @@ impl InstallAppState { // reachability. Force the in-memory object store shortcut so // /install/finish can't hang on an unreachable bucket. unsafe { - std::env::set_var("FABRO_TEST_IN_MEMORY_STORE", "1"); + std::env::set_var(EnvVars::FABRO_TEST_IN_MEMORY_STORE, "1"); } Self { install_token: Arc::from(token), @@ -952,7 +952,7 @@ fn install_object_store_lookup<'a>( (Some(credentials), OBJECT_STORE_SECRET_ACCESS_KEY_ENV) => { Some(credentials.secret_access_key.expose_secret().to_string()) } - (Some(_), AWS_SESSION_TOKEN_ENV) => None, + (Some(_), EnvVars::AWS_SESSION_TOKEN) => None, _ => server_secrets.get(name), } } @@ -1351,7 +1351,7 @@ async fn post_install_finish( return install_error_response(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()); } vault_secrets.push(VaultSecretWrite { - name: "GITHUB_TOKEN".to_string(), + name: EnvVars::GITHUB_TOKEN.to_string(), value: github.token, secret_type: VaultSecretType::Environment, description: None, @@ -1388,15 +1388,15 @@ async fn post_install_finish( return install_error_response(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()); } server_env_writes.push(make_env_write( - "GITHUB_APP_PRIVATE_KEY", + EnvVars::GITHUB_APP_PRIVATE_KEY, BASE64_STANDARD.encode(github.pem.as_bytes()), )); server_env_writes.push(make_env_write( - "GITHUB_APP_CLIENT_SECRET", + EnvVars::GITHUB_APP_CLIENT_SECRET, github.client_secret, )); if let Some(secret) = github.webhook_secret { - server_env_writes.push(make_env_write("GITHUB_APP_WEBHOOK_SECRET", secret)); + server_env_writes.push(make_env_write(EnvVars::GITHUB_APP_WEBHOOK_SECRET, secret)); } } } @@ -1409,9 +1409,9 @@ async fn post_install_finish( }; let session_secret = session_secret::generate_session_secret(); - server_env_writes.push(make_env_write("SESSION_SECRET", session_secret)); + server_env_writes.push(make_env_write(EnvVars::SESSION_SECRET, session_secret)); if let Some(token) = dev_token.as_ref() { - server_env_writes.push(make_env_write("FABRO_DEV_TOKEN", token.clone())); + server_env_writes.push(make_env_write(EnvVars::FABRO_DEV_TOKEN, token.clone())); } #[expect( @@ -1829,6 +1829,10 @@ async fn validate_llm_provider( } } +#[expect( + clippy::disallowed_methods, + reason = "Install flow checks documented provider base-url overrides while building defaults." +)] fn provider_base_url(state: &InstallAppState, provider: Provider) -> String { state .upstreams @@ -1836,11 +1840,11 @@ fn provider_base_url(state: &InstallAppState, provider: Provider) -> String { .get(&provider) .cloned() .or_else(|| match provider { - Provider::Anthropic => std::env::var("ANTHROPIC_BASE_URL").ok(), - Provider::OpenAi => std::env::var("OPENAI_BASE_URL").ok(), - Provider::Gemini => std::env::var("GEMINI_BASE_URL").ok(), + Provider::Anthropic => std::env::var(EnvVars::ANTHROPIC_BASE_URL).ok(), + Provider::OpenAi => std::env::var(EnvVars::OPENAI_BASE_URL).ok(), + Provider::Gemini => std::env::var(EnvVars::GEMINI_BASE_URL).ok(), Provider::Kimi | Provider::Zai | Provider::Minimax | Provider::Inception => None, - Provider::OpenAiCompatible => std::env::var("OPENAI_COMPATIBLE_BASE_URL").ok(), + Provider::OpenAiCompatible => std::env::var(EnvVars::OPENAI_COMPATIBLE_BASE_URL).ok(), }) .unwrap_or_else(|| match provider { Provider::Anthropic => DEFAULT_ANTHROPIC_BASE_URL.to_string(), @@ -1987,16 +1991,17 @@ mod tests { use axum::http::HeaderMap; use fabro_install::{OBJECT_STORE_ACCESS_KEY_ID_ENV, OBJECT_STORE_SECRET_ACCESS_KEY_ENV}; + use fabro_static::EnvVars; use object_store::Error as ObjectStoreError; use serde_json::json; use super::{ - AWS_SESSION_TOKEN_ENV, DEFAULT_INSTALL_GITHUB_API_BASE_URL, InstallAppState, - InstallAwsCredentialPair, InstallFinishGuard, InstallObjectStoreCredentialMode, - InstallObjectStoreInput, InstallObjectStoreProvider, InstallObjectStoreState, - PendingInstall, ServerSecrets, classify_object_store_validation_error, - detect_canonical_url, install_object_store_lookup, lock_unpoisoned, - resolve_install_object_store_state, token_is_valid, write_artifact_store_metadata, + DEFAULT_INSTALL_GITHUB_API_BASE_URL, InstallAppState, InstallAwsCredentialPair, + InstallFinishGuard, InstallObjectStoreCredentialMode, InstallObjectStoreInput, + InstallObjectStoreProvider, InstallObjectStoreState, PendingInstall, ServerSecrets, + classify_object_store_validation_error, detect_canonical_url, install_object_store_lookup, + lock_unpoisoned, resolve_install_object_store_state, token_is_valid, + write_artifact_store_metadata, }; #[test] @@ -2209,9 +2214,9 @@ AWS_WEB_IDENTITY_TOKEN_FILE=/tmp/fabro-web-identity-token\n", lookup(OBJECT_STORE_SECRET_ACCESS_KEY_ENV).as_deref(), Some("submitted-secret") ); - assert_eq!(lookup(AWS_SESSION_TOKEN_ENV), None); + assert_eq!(lookup(EnvVars::AWS_SESSION_TOKEN), None); assert_eq!( - lookup("AWS_WEB_IDENTITY_TOKEN_FILE").as_deref(), + lookup(EnvVars::AWS_WEB_IDENTITY_TOKEN_FILE).as_deref(), Some("/tmp/fabro-web-identity-token") ); } diff --git a/lib/crates/fabro-server/src/jwt_auth.rs b/lib/crates/fabro-server/src/jwt_auth.rs index 6e1e7a7ea..f791189b4 100644 --- a/lib/crates/fabro-server/src/jwt_auth.rs +++ b/lib/crates/fabro-server/src/jwt_auth.rs @@ -2,6 +2,7 @@ use anyhow::{Result, anyhow}; use axum::extract::FromRequestParts; use axum::http::header; use axum::http::request::Parts; +use fabro_static::EnvVars; use fabro_types::settings::{ServerAuthMethod, ServerNamespace}; use fabro_types::{IdpIdentity, RunAuthMethod}; use fabro_util::dev_token::validate_dev_token_format; @@ -52,7 +53,15 @@ pub enum AuthMode { } pub fn resolve_auth_mode(settings: &ServerNamespace) -> Result { - resolve_auth_mode_with_lookup(settings, |name| std::env::var(name).ok()) + resolve_auth_mode_with_lookup(settings, process_env_var) +} + +#[expect( + clippy::disallowed_methods, + reason = "Server auth startup validation intentionally reads process env for server secrets." +)] +fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() } pub fn resolve_auth_mode_with_lookup(settings: &ServerNamespace, lookup: F) -> Result @@ -78,13 +87,13 @@ where "Fabro server refuses to start: github auth is enabled but server.integrations.github.client_id is not configured." )); } - if github_enabled && lookup("GITHUB_APP_CLIENT_SECRET").is_none() { + if github_enabled && lookup(EnvVars::GITHUB_APP_CLIENT_SECRET).is_none() { return Err(anyhow!( "Fabro server refuses to start: github auth is enabled but GITHUB_APP_CLIENT_SECRET is not set." )); } - let session_secret = lookup("SESSION_SECRET"); + let session_secret = lookup(EnvVars::SESSION_SECRET); let secret = session_secret.as_deref().ok_or_else(|| { anyhow!("Fabro server refuses to start: auth is configured but SESSION_SECRET is not set.") })?; @@ -93,7 +102,7 @@ where } let dev_token = if methods.contains(&ServerAuthMethod::DevToken) { - let token = lookup("FABRO_DEV_TOKEN").ok_or_else(|| { + let token = lookup(EnvVars::FABRO_DEV_TOKEN).ok_or_else(|| { anyhow!( "Fabro server refuses to start: dev-token auth is enabled but FABRO_DEV_TOKEN is not set." ) diff --git a/lib/crates/fabro-server/src/run_files.rs b/lib/crates/fabro-server/src/run_files.rs index 90400d42a..981929a07 100644 --- a/lib/crates/fabro-server/src/run_files.rs +++ b/lib/crates/fabro-server/src/run_files.rs @@ -31,6 +31,7 @@ use fabro_api::types::{ RunFilesMeta, RunFilesMetaDegradedReason, RunFilesMetaToSha, }; use fabro_sandbox::reconnect::reconnect; +use fabro_static::EnvVars; use fabro_types::RunId; use fabro_workflow::sandbox_git::{ DiffError, RawDiffEntry, SubmoduleChange, SymlinkChange, list_binary_paths, @@ -587,7 +588,7 @@ async fn try_reconnect_run_sandbox( let Some(record) = projection.sandbox.clone() else { return Ok(None); }; - let daytona_api_key = state.vault_or_env_pub("DAYTONA_API_KEY"); + let daytona_api_key = state.vault_or_env_pub(EnvVars::DAYTONA_API_KEY); match reconnect(&record, daytona_api_key).await { Ok(sandbox) => Ok(Some(sandbox)), Err(_) => Ok(None), diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 8275725ea..ab6501ac6 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -18,6 +18,7 @@ use fabro_sandbox::config::{ }; use fabro_sandbox::daytona::DaytonaConfig; use fabro_sandbox::{DockerSandboxOptions, Sandbox, SandboxProvider, SandboxSpec}; +use fabro_static::EnvVars; use fabro_types::settings::ServerNamespace; use fabro_types::settings::cli::OutputVerbosity; use fabro_types::settings::interp::InterpString; @@ -416,7 +417,7 @@ async fn build_preflight_report( None }; - let daytona_api_key = state.vault_or_env("DAYTONA_API_KEY"); + let daytona_api_key = state.vault_or_env(EnvVars::DAYTONA_API_KEY); let sandbox_ok = run_sandbox_check( &mut checks, sandbox_provider, diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 390c8ab3d..c9eadcf4e 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -12,6 +12,7 @@ use fabro_config::{ }; use fabro_install::{OBJECT_STORE_ACCESS_KEY_ID_ENV, OBJECT_STORE_SECRET_ACCESS_KEY_ENV}; use fabro_sandbox::SandboxProvider; +use fabro_static::EnvVars; use fabro_types::ServerSettings; use fabro_types::settings::server::{GithubIntegrationStrategy, WebhookStrategy}; use fabro_types::settings::{ @@ -40,8 +41,6 @@ use crate::server::{ use crate::server_secrets::{ServerSecrets, process_env_snapshot}; use crate::startup::resolve_startup; -const TEST_IN_MEMORY_STORE_ENV: &str = "FABRO_TEST_IN_MEMORY_STORE"; -const AWS_SESSION_TOKEN_ENV: &str = "AWS_SESSION_TOKEN"; pub const DEFAULT_TCP_PORT: u16 = 32276; type EnvLookup = Arc Option + Send + Sync>; @@ -340,9 +339,15 @@ async fn start_webhook_strategy( } } +#[expect( + clippy::disallowed_methods, + reason = "Test-only server object-store shortcut reads a documented Fabro env var." +)] fn use_in_memory_store() -> bool { !matches!( - std::env::var(TEST_IN_MEMORY_STORE_ENV).ok().as_deref(), + std::env::var(EnvVars::FABRO_TEST_IN_MEMORY_STORE) + .ok() + .as_deref(), None | Some("" | "0" | "false" | "no") ) } @@ -374,7 +379,7 @@ where let access_key_id = env_lookup(OBJECT_STORE_ACCESS_KEY_ID_ENV); let secret_access_key = env_lookup(OBJECT_STORE_SECRET_ACCESS_KEY_ENV); - let session_token = env_lookup(AWS_SESSION_TOKEN_ENV); + let session_token = env_lookup(EnvVars::AWS_SESSION_TOKEN); match (access_key_id, secret_access_key) { (Some(access_key_id), Some(secret_access_key)) => { builder = builder @@ -394,26 +399,38 @@ where for (name, key) in [ ( - "AWS_WEB_IDENTITY_TOKEN_FILE", + EnvVars::AWS_WEB_IDENTITY_TOKEN_FILE, AmazonS3ConfigKey::WebIdentityTokenFile, ), - ("AWS_ROLE_ARN", AmazonS3ConfigKey::RoleArn), - ("AWS_ROLE_SESSION_NAME", AmazonS3ConfigKey::RoleSessionName), - ("AWS_ENDPOINT_URL_STS", AmazonS3ConfigKey::StsEndpoint), + (EnvVars::AWS_ROLE_ARN, AmazonS3ConfigKey::RoleArn), ( - "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI", + EnvVars::AWS_ROLE_SESSION_NAME, + AmazonS3ConfigKey::RoleSessionName, + ), + ( + EnvVars::AWS_ENDPOINT_URL_STS, + AmazonS3ConfigKey::StsEndpoint, + ), + ( + EnvVars::AWS_CONTAINER_CREDENTIALS_RELATIVE_URI, AmazonS3ConfigKey::ContainerCredentialsRelativeUri, ), ( - "AWS_CONTAINER_CREDENTIALS_FULL_URI", + EnvVars::AWS_CONTAINER_CREDENTIALS_FULL_URI, AmazonS3ConfigKey::ContainerCredentialsFullUri, ), ( - "AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE", + EnvVars::AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE, AmazonS3ConfigKey::ContainerAuthorizationTokenFile, ), - ("AWS_METADATA_ENDPOINT", AmazonS3ConfigKey::MetadataEndpoint), - ("AWS_IMDSV1_FALLBACK", AmazonS3ConfigKey::ImdsV1Fallback), + ( + EnvVars::AWS_METADATA_ENDPOINT, + AmazonS3ConfigKey::MetadataEndpoint, + ), + ( + EnvVars::AWS_IMDSV1_FALLBACK, + AmazonS3ConfigKey::ImdsV1Fallback, + ), ] { if let Some(value) = env_lookup(name) { builder = builder.with_config(key, value); @@ -492,11 +509,19 @@ fn resolved_bind_request( fn resolve_interp(value: &InterpString) -> anyhow::Result { value - .resolve(|name| std::env::var(name).ok()) + .resolve(process_env_var) .map(|resolved| resolved.value) .with_context(|| format!("failed to resolve {}", value.as_source())) } +#[expect( + clippy::disallowed_methods, + reason = "Server settings interpolation owns a process-env lookup facade for {{ env.* }} values." +)] +fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() +} + fn resolve_interp_path(value: &InterpString) -> anyhow::Result { Ok(PathBuf::from(resolve_interp(value)?)) } @@ -638,7 +663,7 @@ where &server_secrets, )?; let artifact_store = fabro_store::ArtifactStore::new(artifact_object_store, artifact_prefix); - let env_lookup: EnvLookup = Arc::new(|name| std::env::var(name).ok()); + let env_lookup: EnvLookup = Arc::new(process_env_var); resolve_canonical_origin(&resolved_server_settings, &env_lookup).map_err(anyhow::Error::msg)?; let state = build_app_state(AppStateConfig { resolved_settings: resolved_app_settings, diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 8a869a9bb..ee2b9d329 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -62,6 +62,7 @@ use fabro_slack::config::resolve_credentials as resolve_slack_credentials; use fabro_slack::payload::SlackAnswerSubmission; use fabro_slack::threads::ThreadRegistry; use fabro_slack::{blocks as slack_blocks, connection as slack_connection}; +use fabro_static::EnvVars; use fabro_store::{ ArtifactStore, Database, EventEnvelope, EventPayload, PendingInterviewRecord, StageId, }; @@ -734,7 +735,7 @@ impl AppState { } pub(crate) fn vault_or_env(&self, name: &str) -> Option { - std::env::var(name).ok().or_else(|| { + process_env_var(name).or_else(|| { self.vault .try_read() .ok() @@ -774,7 +775,7 @@ impl AppState { } pub(crate) fn session_key(&self) -> Option { - self.server_secret("SESSION_SECRET") + self.server_secret(EnvVars::SESSION_SECRET) .and_then(|value| auth::derive_cookie_key(value.as_bytes()).ok()) } @@ -787,11 +788,11 @@ impl AppState { let Some(app_id) = settings.app_id.as_ref().map(InterpString::as_source) else { return Ok(None); }; - let raw = self.server_secret("GITHUB_APP_PRIVATE_KEY"); + let raw = self.server_secret(EnvVars::GITHUB_APP_PRIVATE_KEY); let Some(raw) = raw else { return Ok(None); }; - let private_key_pem = decode_secret_pem("GITHUB_APP_PRIVATE_KEY", &raw)?; + let private_key_pem = decode_secret_pem(EnvVars::GITHUB_APP_PRIVATE_KEY, &raw)?; Ok(Some(fabro_github::GitHubCredentials::App( fabro_github::GitHubAppCredentials { app_id, @@ -801,8 +802,8 @@ impl AppState { } GithubIntegrationStrategy::Token => { let token = self - .vault_or_env("GITHUB_TOKEN") - .or_else(|| self.vault_or_env("GH_TOKEN")) + .vault_or_env(EnvVars::GITHUB_TOKEN) + .or_else(|| self.vault_or_env(EnvVars::GH_TOKEN)) .as_deref() .map(str::trim) .filter(|token| !token.is_empty()) @@ -870,11 +871,19 @@ fn decode_secret_pem(name: &str, raw: &str) -> Result { fn resolve_interp_string(value: &InterpString) -> anyhow::Result { value - .resolve(|name| std::env::var(name).ok()) + .resolve(process_env_var) .map(|resolved| resolved.value) .map_err(anyhow::Error::from) } +#[expect( + clippy::disallowed_methods, + reason = "Server state owns process-env lookup facades for interpolation and vault fallbacks." +)] +fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() +} + fn start_optional_slack_service(state: &Arc) { let Some(service) = state.slack_service.clone() else { return; @@ -2515,7 +2524,7 @@ pub fn create_app_state_with_runtime_settings_and_options( server_settings, manifest_run_defaults, max_concurrent_runs, - |name| std::env::var(name).ok(), + process_env_var, &HashMap::new(), ) } @@ -2710,17 +2719,17 @@ pub fn create_app_state_with_store_and_runtime_settings( } fn default_env_lookup() -> EnvLookup { - Arc::new(|name| std::env::var(name).ok()) + Arc::new(process_env_var) } fn load_test_server_secrets(path: PathBuf, env: HashMap) -> ServerSecrets { let mut env = env; let file_has_session_secret = envfile::read_env_file(&path) .ok() - .is_some_and(|entries| entries.contains_key("SESSION_SECRET")); - if !env.contains_key("SESSION_SECRET") && !file_has_session_secret { + .is_some_and(|entries| entries.contains_key(EnvVars::SESSION_SECRET)); + if !env.contains_key(EnvVars::SESSION_SECRET) && !file_has_session_secret { env.insert( - "SESSION_SECRET".to_string(), + EnvVars::SESSION_SECRET.to_string(), "server-test-session-key-0123456789".to_string(), ); } @@ -2731,7 +2740,7 @@ fn worker_token_keys_from_server_secrets( server_secrets: &ServerSecrets, ) -> anyhow::Result { let session_secret = server_secrets - .get("SESSION_SECRET") + .get(EnvVars::SESSION_SECRET) .ok_or_else(|| jwt_auth::session_secret_key_error(&auth::KeyDeriveError::Empty))?; WorkerTokenKeys::from_master_secret(session_secret.as_bytes()) .map_err(|err| jwt_auth::session_secret_key_error(&err)) @@ -2772,7 +2781,7 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result anyhow::Result { let current_exe = std::env::current_exe().context("reading current executable path")?; - let exe = std::env::var_os("CARGO_BIN_EXE_fabro").map_or(current_exe, PathBuf::from); + let exe = std::env::var_os(EnvVars::CARGO_BIN_EXE_FABRO).map_or(current_exe, PathBuf::from); let storage_dir = state.server_storage_dir(); let runtime_directory = Storage::new(&storage_dir).runtime_directory(); let daemon = ServerDaemon::read(&runtime_directory)?.with_context(|| { @@ -3906,8 +3919,8 @@ fn worker_command( .stderr(Stdio::piped()); apply_worker_env(&mut cmd); - cmd.env_remove("FABRO_WORKER_TOKEN"); - cmd.env("FABRO_WORKER_TOKEN", worker_token); + cmd.env_remove(EnvVars::FABRO_WORKER_TOKEN); + cmd.env(EnvVars::FABRO_WORKER_TOKEN, worker_token); #[cfg(unix)] fabro_proc::pre_exec_setpgid(cmd.as_std_mut()); @@ -4639,7 +4652,7 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { .iter() .map(|(name, value)| { let resolved = value - .resolve(|env| std::env::var(env).ok()) + .resolve(process_env_var) .map_or_else(|_| value.as_source(), |resolved| resolved.value); (name.clone(), resolved) }) @@ -6594,7 +6607,7 @@ async fn reconnect_run_sandbox( run_id: &RunId, ) -> Result, Response> { let record = load_run_sandbox_record(state, run_id).await?; - let daytona_api_key = state.vault_or_env("DAYTONA_API_KEY"); + let daytona_api_key = state.vault_or_env(EnvVars::DAYTONA_API_KEY); reconnect(&record, daytona_api_key) .await .map_err(|err| ApiError::new(StatusCode::CONFLICT, format!("{err}")).into_response()) @@ -6619,7 +6632,7 @@ async fn reconnect_daytona_sandbox( ) .into_response()); }; - let daytona_api_key = state.vault_or_env("DAYTONA_API_KEY"); + let daytona_api_key = state.vault_or_env(EnvVars::DAYTONA_API_KEY); DaytonaSandbox::reconnect(name, daytona_api_key) .await .map_err(|err| ApiError::new(StatusCode::CONFLICT, err.clone()).into_response()) @@ -7121,14 +7134,13 @@ async fn rewind_run( source_run_id, new_run_id, target, - archived, }) => ( StatusCode::OK, Json(RewindResponse { source_run_id: source_run_id.to_string(), - new_run_id: new_run_id.to_string(), - target: target.response_target(), - archived, + new_run_id: new_run_id.to_string(), + target: target.response_target(), + archived: true, archive_error: None, }), ) @@ -7719,13 +7731,17 @@ async fn create_completion( } } +#[expect( + clippy::disallowed_methods, + reason = "Render-graph subprocess startup resolves Cargo's test binary env override when present." +)] fn render_graph_subprocess_exe( exe_override: Option<&std::path::Path>, ) -> Result { if let Some(path) = exe_override { Ok(path.to_path_buf()) } else { - if let Some(path) = std::env::var_os("CARGO_BIN_EXE_fabro").map(PathBuf::from) { + if let Some(path) = std::env::var_os(EnvVars::CARGO_BIN_EXE_FABRO).map(PathBuf::from) { return Ok(path); } @@ -7789,7 +7805,7 @@ async fn render_dot_subprocess( let mut cmd = Command::new(exe); apply_render_graph_env(&mut cmd); cmd.arg("__render-graph") - .env("FABRO_TELEMETRY", "off") + .env(EnvVars::FABRO_TELEMETRY, "off") .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped()); diff --git a/lib/crates/fabro-server/src/server_secrets.rs b/lib/crates/fabro-server/src/server_secrets.rs index 5db4919e7..64ce1bea2 100644 --- a/lib/crates/fabro-server/src/server_secrets.rs +++ b/lib/crates/fabro-server/src/server_secrets.rs @@ -6,6 +6,10 @@ use fabro_config::envfile; use fabro_llm::client::Client; use fabro_model::Provider; +#[expect( + clippy::disallowed_methods, + reason = "ServerSecrets snapshots process env once at startup by design." +)] pub fn process_env_snapshot() -> HashMap { std::env::vars().collect() } diff --git a/lib/crates/fabro-server/src/spawn_env.rs b/lib/crates/fabro-server/src/spawn_env.rs index 2aba371dc..54ab7bd45 100644 --- a/lib/crates/fabro-server/src/spawn_env.rs +++ b/lib/crates/fabro-server/src/spawn_env.rs @@ -1,28 +1,35 @@ use std::ffi::OsString; +use fabro_static::EnvVars; use tokio::process::Command; const WORKER_ENV_ALLOWLIST: &[&str] = &[ - "PATH", - "HOME", - "TMPDIR", - "USER", - "RUST_LOG", - "RUST_BACKTRACE", - "FABRO_HOME", - "FABRO_STORAGE_ROOT", + EnvVars::PATH, + EnvVars::HOME, + EnvVars::TMPDIR, + EnvVars::USER, + EnvVars::RUST_LOG, + EnvVars::RUST_BACKTRACE, + EnvVars::FABRO_HOME, + EnvVars::FABRO_STORAGE_ROOT, ]; -const RENDER_GRAPH_ENV_ALLOWLIST: &[&str] = &["PATH", "HOME", "TMPDIR"]; +const RENDER_GRAPH_ENV_ALLOWLIST: &[&str] = &[EnvVars::PATH, EnvVars::HOME, EnvVars::TMPDIR]; pub(crate) fn apply_worker_env(cmd: &mut Command) { - apply_allowlist(cmd, WORKER_ENV_ALLOWLIST, &|name| std::env::var_os(name)); + apply_allowlist(cmd, WORKER_ENV_ALLOWLIST, &process_env_var_os); } pub(crate) fn apply_render_graph_env(cmd: &mut Command) { - apply_allowlist(cmd, RENDER_GRAPH_ENV_ALLOWLIST, &|name| { - std::env::var_os(name) - }); + apply_allowlist(cmd, RENDER_GRAPH_ENV_ALLOWLIST, &process_env_var_os); +} + +#[expect( + clippy::disallowed_methods, + reason = "Subprocess env allowlists intentionally copy a narrow process-env subset." +)] +fn process_env_var_os(name: &str) -> Option { + std::env::var_os(name) } fn apply_allowlist(cmd: &mut Command, keys: &[&str], lookup: &dyn Fn(&str) -> Option) { diff --git a/lib/crates/fabro-server/src/startup.rs b/lib/crates/fabro-server/src/startup.rs index 6507d86e0..40ea7f6d1 100644 --- a/lib/crates/fabro-server/src/startup.rs +++ b/lib/crates/fabro-server/src/startup.rs @@ -29,6 +29,7 @@ mod tests { use std::collections::HashMap; use fabro_config::ServerSettingsBuilder; + use fabro_static::EnvVars; use fabro_types::settings::ServerNamespace; use super::validate_startup; @@ -56,11 +57,11 @@ methods = [{}] let dir = tempfile::tempdir().unwrap(); let env = HashMap::from([ ( - "SESSION_SECRET".to_string(), + EnvVars::SESSION_SECRET.to_string(), "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef".to_string(), ), ( - "FABRO_DEV_TOKEN".to_string(), + EnvVars::FABRO_DEV_TOKEN.to_string(), "fabro_dev_abababababababababababababababababababababababababababababababab" .to_string(), ), diff --git a/lib/crates/fabro-server/src/web_auth.rs b/lib/crates/fabro-server/src/web_auth.rs index 3d54f5cd1..04b7344f8 100644 --- a/lib/crates/fabro-server/src/web_auth.rs +++ b/lib/crates/fabro-server/src/web_auth.rs @@ -7,6 +7,7 @@ use axum::routing::{get, post}; use axum::{Extension, Json, Router}; use cookie::time::Duration; use cookie::{Cookie, CookieJar, Key, SameSite}; +use fabro_static::EnvVars; use fabro_types::settings::ServerAuthMethod; use fabro_types::{IdpIdentity, RunAuthMethod}; use fabro_util::dev_token::validate_dev_token_format; @@ -293,10 +294,18 @@ fn session_cookie_secure(state: &AppState) -> bool { .server .web .url - .resolve(|name| std::env::var(name).ok()) + .resolve(process_env_var) .is_ok_and(|resolved| resolved.value.starts_with("https://")) } +#[expect( + clippy::disallowed_methods, + reason = "Web auth resolves configured {{ env.* }} URLs through this process-env facade." +)] +fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() +} + async fn login_dev_token( State(state): State>, Extension(auth_mode): Extension, @@ -534,7 +543,7 @@ async fn callback_github( ); } }; - let Some(client_secret) = state.server_secret("GITHUB_APP_CLIENT_SECRET") else { + let Some(client_secret) = state.server_secret(EnvVars::GITHUB_APP_CLIENT_SECRET) else { error!("OAuth callback failed: GITHUB_APP_CLIENT_SECRET not configured"); return json_response( StatusCode::CONFLICT, diff --git a/lib/crates/fabro-slack/Cargo.toml b/lib/crates/fabro-slack/Cargo.toml index 5134ed76b..336368656 100644 --- a/lib/crates/fabro-slack/Cargo.toml +++ b/lib/crates/fabro-slack/Cargo.toml @@ -16,6 +16,7 @@ workspace = true fabro-interview = { path = "../fabro-interview" } fabro-workflow = { path = "../fabro-workflow" } fabro-http.workspace = true +fabro-static.workspace = true futures-util.workspace = true serde.workspace = true serde_json.workspace = true diff --git a/lib/crates/fabro-slack/src/client.rs b/lib/crates/fabro-slack/src/client.rs index b3b38b493..32311812b 100644 --- a/lib/crates/fabro-slack/src/client.rs +++ b/lib/crates/fabro-slack/src/client.rs @@ -1,3 +1,4 @@ +use fabro_static::EnvVars; use serde_json::{Value, json}; use tracing::debug; @@ -17,9 +18,13 @@ pub struct SlackClient { } impl SlackClient { + #[expect( + clippy::disallowed_methods, + reason = "Slack client supports a documented process-env API base URL override." + )] pub fn new(bot_token: String) -> Self { let api_base = - std::env::var("SLACK_BASE_URL").unwrap_or_else(|_| SLACK_API_BASE.to_string()); + std::env::var(EnvVars::SLACK_BASE_URL).unwrap_or_else(|_| SLACK_API_BASE.to_string()); Self { bot_token, api_base, diff --git a/lib/crates/fabro-slack/src/config.rs b/lib/crates/fabro-slack/src/config.rs index f0e3222d0..e04f55141 100644 --- a/lib/crates/fabro-slack/src/config.rs +++ b/lib/crates/fabro-slack/src/config.rs @@ -1,3 +1,4 @@ +use fabro_static::EnvVars; use serde::Deserialize; #[derive(Debug, Clone, Default, Deserialize, PartialEq)] @@ -11,13 +12,17 @@ pub struct SlackCredentials { pub app_token: String, } +#[expect( + clippy::disallowed_methods, + reason = "Slack credential resolution intentionally reads documented token env vars." +)] fn non_empty_env(name: &str) -> Option { std::env::var(name).ok().filter(|s| !s.is_empty()) } pub fn resolve_credentials() -> Option { - let bot_token = non_empty_env("FABRO_SLACK_BOT_TOKEN")?; - let app_token = non_empty_env("FABRO_SLACK_APP_TOKEN")?; + let bot_token = non_empty_env(EnvVars::FABRO_SLACK_BOT_TOKEN)?; + let app_token = non_empty_env(EnvVars::FABRO_SLACK_APP_TOKEN)?; Some(SlackCredentials { bot_token, app_token, diff --git a/lib/crates/fabro-slack/src/connection.rs b/lib/crates/fabro-slack/src/connection.rs index 567ea5b91..1491b7ae0 100644 --- a/lib/crates/fabro-slack/src/connection.rs +++ b/lib/crates/fabro-slack/src/connection.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use fabro_static::EnvVars; use futures_util::{SinkExt, StreamExt}; use tokio::time::sleep; use tokio_tungstenite::tungstenite::Message; @@ -56,6 +57,10 @@ pub fn process_message( } /// Fetch a WebSocket URL from Slack's `apps.connections.open` endpoint. +#[expect( + clippy::disallowed_methods, + reason = "Slack socket setup supports a documented process-env API base URL override." +)] pub async fn open_socket_url( http: &fabro_http::HttpClient, app_token: &str, @@ -63,7 +68,8 @@ pub async fn open_socket_url( let resp = http .post(format!( "{}/apps.connections.open", - std::env::var("SLACK_BASE_URL").unwrap_or_else(|_| "https://slack.com/api".to_string()) + std::env::var(EnvVars::SLACK_BASE_URL) + .unwrap_or_else(|_| "https://slack.com/api".to_string()) )) .bearer_auth(app_token) .header("Content-Type", "application/x-www-form-urlencoded") diff --git a/lib/crates/fabro-static/Cargo.toml b/lib/crates/fabro-static/Cargo.toml new file mode 100644 index 000000000..70e37f8fa --- /dev/null +++ b/lib/crates/fabro-static/Cargo.toml @@ -0,0 +1,13 @@ +[package] +name = "fabro-static" +edition.workspace = true +version.workspace = true +publish = false +license.workspace = true +description = "Static string registries for shared Fabro conventions" + +[lib] +doctest = false + +[lints] +workspace = true diff --git a/lib/crates/fabro-static/src/env_vars.rs b/lib/crates/fabro-static/src/env_vars.rs new file mode 100644 index 000000000..30e8594a0 --- /dev/null +++ b/lib/crates/fabro-static/src/env_vars.rs @@ -0,0 +1,256 @@ +//! Fixed process environment variable names used or supported by Fabro. + +/// Declares the environment variable names used throughout Fabro and its +/// crates. +pub struct EnvVars; + +impl EnvVars { + // Fabro core + pub const FABRO_AUTH_FILE: &'static str = "FABRO_AUTH_FILE"; + pub const FABRO_BUILD_DATE: &'static str = "FABRO_BUILD_DATE"; + pub const FABRO_BUILD_PROFILE: &'static str = "FABRO_BUILD_PROFILE"; + pub const FABRO_BUILD_PROFILE_SUFFIX: &'static str = "FABRO_BUILD_PROFILE_SUFFIX"; + pub const FABRO_CONFIG: &'static str = "FABRO_CONFIG"; + pub const FABRO_DEBUG: &'static str = "FABRO_DEBUG"; + pub const FABRO_DEV_TOKEN: &'static str = "FABRO_DEV_TOKEN"; + pub const FABRO_ENABLE_FETCH_FEATURE_OCI_INTEGRATION: &'static str = + "FABRO_ENABLE_FETCH_FEATURE_OCI_INTEGRATION"; + pub const FABRO_GIT_SHA: &'static str = "FABRO_GIT_SHA"; + pub const FABRO_HOME: &'static str = "FABRO_HOME"; + pub const FABRO_HTTP_PROXY_POLICY: &'static str = "FABRO_HTTP_PROXY_POLICY"; + pub const FABRO_JSON: &'static str = "FABRO_JSON"; + pub const FABRO_NO_UPGRADE_CHECK: &'static str = "FABRO_NO_UPGRADE_CHECK"; + pub const FABRO_QUIET: &'static str = "FABRO_QUIET"; + pub const FABRO_SERVER: &'static str = "FABRO_SERVER"; + pub const FABRO_SERVER_MAX_CONCURRENT_RUNS: &'static str = "FABRO_SERVER_MAX_CONCURRENT_RUNS"; + pub const FABRO_SLACK_APP_TOKEN: &'static str = "FABRO_SLACK_APP_TOKEN"; + pub const FABRO_SLACK_BOT_TOKEN: &'static str = "FABRO_SLACK_BOT_TOKEN"; + pub const FABRO_STORAGE_DIR: &'static str = "FABRO_STORAGE_DIR"; + pub const FABRO_STORAGE_ROOT: &'static str = "FABRO_STORAGE_ROOT"; + pub const FABRO_SUPPRESS_OPEN_BROWSER: &'static str = "FABRO_SUPPRESS_OPEN_BROWSER"; + pub const FABRO_TELEMETRY: &'static str = "FABRO_TELEMETRY"; + pub const FABRO_TEST_IN_MEMORY_STORE: &'static str = "FABRO_TEST_IN_MEMORY_STORE"; + pub const FABRO_TEST_MODE: &'static str = "FABRO_TEST_MODE"; + pub const FABRO_VERBOSE: &'static str = "FABRO_VERBOSE"; + pub const FABRO_WEB_URL: &'static str = "FABRO_WEB_URL"; + pub const FABRO_WORKER_TOKEN: &'static str = "FABRO_WORKER_TOKEN"; + + // LLM providers and tool integrations + pub const ANTHROPIC_API_KEY: &'static str = "ANTHROPIC_API_KEY"; + pub const ANTHROPIC_BASE_URL: &'static str = "ANTHROPIC_BASE_URL"; + pub const BRAVE_SEARCH_API_KEY: &'static str = "BRAVE_SEARCH_API_KEY"; + pub const CHATGPT_ACCOUNT_ID: &'static str = "CHATGPT_ACCOUNT_ID"; + pub const GEMINI_API_KEY: &'static str = "GEMINI_API_KEY"; + pub const GEMINI_BASE_URL: &'static str = "GEMINI_BASE_URL"; + pub const GOOGLE_API_KEY: &'static str = "GOOGLE_API_KEY"; + pub const GOPATH: &'static str = "GOPATH"; + pub const INCEPTION_API_KEY: &'static str = "INCEPTION_API_KEY"; + pub const KIMI_API_KEY: &'static str = "KIMI_API_KEY"; + pub const MINIMAX_API_KEY: &'static str = "MINIMAX_API_KEY"; + pub const OPENAI_API_KEY: &'static str = "OPENAI_API_KEY"; + pub const OPENAI_BASE_URL: &'static str = "OPENAI_BASE_URL"; + pub const OPENAI_COMPATIBLE_BASE_URL: &'static str = "OPENAI_COMPATIBLE_BASE_URL"; + pub const OPENAI_ORGANIZATION: &'static str = "OPENAI_ORGANIZATION"; + pub const OPENAI_PROJECT: &'static str = "OPENAI_PROJECT"; + pub const OPENAI_ORG_ID: &'static str = "OPENAI_ORG_ID"; + pub const OPENAI_PROJECT_ID: &'static str = "OPENAI_PROJECT_ID"; + pub const ZAI_API_KEY: &'static str = "ZAI_API_KEY"; + + // GitHub, OAuth, and Slack + pub const GH_TOKEN: &'static str = "GH_TOKEN"; + pub const GITHUB_APP_CLIENT_SECRET: &'static str = "GITHUB_APP_CLIENT_SECRET"; + pub const GITHUB_APP_PRIVATE_KEY: &'static str = "GITHUB_APP_PRIVATE_KEY"; + pub const GITHUB_APP_WEBHOOK_SECRET: &'static str = "GITHUB_APP_WEBHOOK_SECRET"; + pub const GITHUB_BASE_URL: &'static str = "GITHUB_BASE_URL"; + pub const GITHUB_TOKEN: &'static str = "GITHUB_TOKEN"; + pub const OAUTH_CALLBACK_PATH: &'static str = "OAUTH_CALLBACK_PATH"; + pub const OAUTH_CLIENT_ID: &'static str = "OAUTH_CLIENT_ID"; + pub const OAUTH_ISSUER: &'static str = "OAUTH_ISSUER"; + pub const OAUTH_PORT: &'static str = "OAUTH_PORT"; + pub const OAUTH_SCOPE: &'static str = "OAUTH_SCOPE"; + pub const SLACK_BASE_URL: &'static str = "SLACK_BASE_URL"; + + // Server, sandbox, and cloud provider integration + pub const AWS_ACCESS_KEY_ID: &'static str = "AWS_ACCESS_KEY_ID"; + pub const AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE: &'static str = + "AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE"; + pub const AWS_CONTAINER_CREDENTIALS_FULL_URI: &'static str = + "AWS_CONTAINER_CREDENTIALS_FULL_URI"; + pub const AWS_CONTAINER_CREDENTIALS_RELATIVE_URI: &'static str = + "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI"; + pub const AWS_ENDPOINT: &'static str = "AWS_ENDPOINT"; + pub const AWS_ENDPOINT_URL_S3: &'static str = "AWS_ENDPOINT_URL_S3"; + pub const AWS_ENDPOINT_URL_STS: &'static str = "AWS_ENDPOINT_URL_STS"; + pub const AWS_IMDSV1_FALLBACK: &'static str = "AWS_IMDSV1_FALLBACK"; + pub const AWS_METADATA_ENDPOINT: &'static str = "AWS_METADATA_ENDPOINT"; + pub const AWS_ROLE_ARN: &'static str = "AWS_ROLE_ARN"; + pub const AWS_ROLE_SESSION_NAME: &'static str = "AWS_ROLE_SESSION_NAME"; + pub const AWS_SECRET_ACCESS_KEY: &'static str = "AWS_SECRET_ACCESS_KEY"; + pub const AWS_SESSION_TOKEN: &'static str = "AWS_SESSION_TOKEN"; + pub const AWS_WEB_IDENTITY_TOKEN_FILE: &'static str = "AWS_WEB_IDENTITY_TOKEN_FILE"; + pub const DAYTONA_API_KEY: &'static str = "DAYTONA_API_KEY"; + pub const DAYTONA_API_URL: &'static str = "DAYTONA_API_URL"; + pub const DAYTONA_SERVER_URL: &'static str = "DAYTONA_SERVER_URL"; + pub const SESSION_SECRET: &'static str = "SESSION_SECRET"; + + // Platform and test harness + pub const CARGO_BIN_EXE_FABRO: &'static str = "CARGO_BIN_EXE_fabro"; + pub const CARGO_CFG_TARGET_OS: &'static str = "CARGO_CFG_TARGET_OS"; + pub const CARGO_HOME: &'static str = "CARGO_HOME"; + pub const CARGO_MANIFEST_DIR: &'static str = "CARGO_MANIFEST_DIR"; + pub const CI: &'static str = "CI"; + pub const HOME: &'static str = "HOME"; + pub const KUBERNETES_SERVICE_HOST: &'static str = "KUBERNETES_SERVICE_HOST"; + pub const LANG: &'static str = "LANG"; + pub const LLVM_PROFILE_FILE: &'static str = "LLVM_PROFILE_FILE"; + pub const NEXTEST_PROFILE: &'static str = "NEXTEST_PROFILE"; + pub const NEXTEST_RUN_ID: &'static str = "NEXTEST_RUN_ID"; + pub const NO_COLOR: &'static str = "NO_COLOR"; + pub const NVM_DIR: &'static str = "NVM_DIR"; + pub const OUT_DIR: &'static str = "OUT_DIR"; + pub const PATH: &'static str = "PATH"; + pub const PATHEXT: &'static str = "PATHEXT"; + pub const PROFILE: &'static str = "PROFILE"; + pub const RAILWAY_ENVIRONMENT: &'static str = "RAILWAY_ENVIRONMENT"; + pub const RAILWAY_PUBLIC_DOMAIN: &'static str = "RAILWAY_PUBLIC_DOMAIN"; + pub const RUST_BACKTRACE: &'static str = "RUST_BACKTRACE"; + pub const RUST_LOG: &'static str = "RUST_LOG"; + pub const SHELL: &'static str = "SHELL"; + pub const TERM: &'static str = "TERM"; + pub const TMPDIR: &'static str = "TMPDIR"; + pub const TWIN_OPENAI_BIND_ADDR: &'static str = "TWIN_OPENAI_BIND_ADDR"; + pub const TWIN_OPENAI_ENABLE_ADMIN: &'static str = "TWIN_OPENAI_ENABLE_ADMIN"; + pub const TWIN_OPENAI_LIVE_BASE_URL: &'static str = "TWIN_OPENAI_LIVE_BASE_URL"; + pub const TWIN_OPENAI_LIVE_MODEL: &'static str = "TWIN_OPENAI_LIVE_MODEL"; + pub const TWIN_OPENAI_REQUIRE_AUTH: &'static str = "TWIN_OPENAI_REQUIRE_AUTH"; + pub const USER: &'static str = "USER"; + pub const ZDOTDIR: &'static str = "ZDOTDIR"; +} + +#[cfg(test)] +mod tests { + use super::EnvVars; + + #[test] + fn env_var_constants_match_their_names() { + assert_eq!(EnvVars::FABRO_CONFIG, "FABRO_CONFIG"); + } + + #[test] + fn env_var_constants_are_non_empty_and_single_tokens() { + let values = [ + EnvVars::FABRO_AUTH_FILE, + EnvVars::FABRO_BUILD_DATE, + EnvVars::FABRO_BUILD_PROFILE, + EnvVars::FABRO_BUILD_PROFILE_SUFFIX, + EnvVars::FABRO_CONFIG, + EnvVars::FABRO_DEBUG, + EnvVars::FABRO_DEV_TOKEN, + EnvVars::FABRO_ENABLE_FETCH_FEATURE_OCI_INTEGRATION, + EnvVars::FABRO_GIT_SHA, + EnvVars::FABRO_HOME, + EnvVars::FABRO_HTTP_PROXY_POLICY, + EnvVars::FABRO_JSON, + EnvVars::FABRO_NO_UPGRADE_CHECK, + EnvVars::FABRO_QUIET, + EnvVars::FABRO_SERVER, + EnvVars::FABRO_SERVER_MAX_CONCURRENT_RUNS, + EnvVars::FABRO_SLACK_APP_TOKEN, + EnvVars::FABRO_SLACK_BOT_TOKEN, + EnvVars::FABRO_STORAGE_DIR, + EnvVars::FABRO_STORAGE_ROOT, + EnvVars::FABRO_SUPPRESS_OPEN_BROWSER, + EnvVars::FABRO_TELEMETRY, + EnvVars::FABRO_TEST_IN_MEMORY_STORE, + EnvVars::FABRO_TEST_MODE, + EnvVars::FABRO_VERBOSE, + EnvVars::FABRO_WEB_URL, + EnvVars::FABRO_WORKER_TOKEN, + EnvVars::ANTHROPIC_API_KEY, + EnvVars::ANTHROPIC_BASE_URL, + EnvVars::BRAVE_SEARCH_API_KEY, + EnvVars::CHATGPT_ACCOUNT_ID, + EnvVars::GEMINI_API_KEY, + EnvVars::GEMINI_BASE_URL, + EnvVars::GOOGLE_API_KEY, + EnvVars::GOPATH, + EnvVars::INCEPTION_API_KEY, + EnvVars::KIMI_API_KEY, + EnvVars::MINIMAX_API_KEY, + EnvVars::OPENAI_API_KEY, + EnvVars::OPENAI_BASE_URL, + EnvVars::OPENAI_COMPATIBLE_BASE_URL, + EnvVars::OPENAI_ORGANIZATION, + EnvVars::OPENAI_PROJECT, + EnvVars::OPENAI_ORG_ID, + EnvVars::OPENAI_PROJECT_ID, + EnvVars::ZAI_API_KEY, + EnvVars::GH_TOKEN, + EnvVars::GITHUB_APP_CLIENT_SECRET, + EnvVars::GITHUB_APP_PRIVATE_KEY, + EnvVars::GITHUB_APP_WEBHOOK_SECRET, + EnvVars::GITHUB_BASE_URL, + EnvVars::GITHUB_TOKEN, + EnvVars::OAUTH_CALLBACK_PATH, + EnvVars::OAUTH_CLIENT_ID, + EnvVars::OAUTH_ISSUER, + EnvVars::OAUTH_PORT, + EnvVars::OAUTH_SCOPE, + EnvVars::SLACK_BASE_URL, + EnvVars::AWS_ACCESS_KEY_ID, + EnvVars::AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE, + EnvVars::AWS_CONTAINER_CREDENTIALS_FULL_URI, + EnvVars::AWS_CONTAINER_CREDENTIALS_RELATIVE_URI, + EnvVars::AWS_ENDPOINT, + EnvVars::AWS_ENDPOINT_URL_S3, + EnvVars::AWS_ENDPOINT_URL_STS, + EnvVars::AWS_IMDSV1_FALLBACK, + EnvVars::AWS_METADATA_ENDPOINT, + EnvVars::AWS_ROLE_ARN, + EnvVars::AWS_ROLE_SESSION_NAME, + EnvVars::AWS_SECRET_ACCESS_KEY, + EnvVars::AWS_SESSION_TOKEN, + EnvVars::AWS_WEB_IDENTITY_TOKEN_FILE, + EnvVars::DAYTONA_API_KEY, + EnvVars::DAYTONA_API_URL, + EnvVars::DAYTONA_SERVER_URL, + EnvVars::SESSION_SECRET, + EnvVars::CARGO_BIN_EXE_FABRO, + EnvVars::CARGO_CFG_TARGET_OS, + EnvVars::CARGO_HOME, + EnvVars::CARGO_MANIFEST_DIR, + EnvVars::CI, + EnvVars::HOME, + EnvVars::KUBERNETES_SERVICE_HOST, + EnvVars::LANG, + EnvVars::LLVM_PROFILE_FILE, + EnvVars::NEXTEST_PROFILE, + EnvVars::NEXTEST_RUN_ID, + EnvVars::NO_COLOR, + EnvVars::NVM_DIR, + EnvVars::OUT_DIR, + EnvVars::PATH, + EnvVars::PATHEXT, + EnvVars::PROFILE, + EnvVars::RAILWAY_ENVIRONMENT, + EnvVars::RAILWAY_PUBLIC_DOMAIN, + EnvVars::RUST_BACKTRACE, + EnvVars::RUST_LOG, + EnvVars::SHELL, + EnvVars::TERM, + EnvVars::TMPDIR, + EnvVars::TWIN_OPENAI_BIND_ADDR, + EnvVars::TWIN_OPENAI_ENABLE_ADMIN, + EnvVars::TWIN_OPENAI_LIVE_BASE_URL, + EnvVars::TWIN_OPENAI_LIVE_MODEL, + EnvVars::TWIN_OPENAI_REQUIRE_AUTH, + EnvVars::USER, + EnvVars::ZDOTDIR, + ]; + + for value in values { + assert!(!value.is_empty()); + assert!(!value.chars().any(char::is_whitespace)); + } + } +} diff --git a/lib/crates/fabro-static/src/lib.rs b/lib/crates/fabro-static/src/lib.rs new file mode 100644 index 000000000..58d35ac25 --- /dev/null +++ b/lib/crates/fabro-static/src/lib.rs @@ -0,0 +1,8 @@ +#![allow( + clippy::disallowed_methods, + reason = "This crate owns the process environment variable name registry." +)] + +mod env_vars; + +pub use env_vars::EnvVars; diff --git a/lib/crates/fabro-telemetry/Cargo.toml b/lib/crates/fabro-telemetry/Cargo.toml index 8eb26a92e..c8c17aa2c 100644 --- a/lib/crates/fabro-telemetry/Cargo.toml +++ b/lib/crates/fabro-telemetry/Cargo.toml @@ -19,6 +19,7 @@ chrono.workspace = true dirs.workspace = true exec.workspace = true fabro-http.workspace = true +fabro-static.workspace = true fabro-util = { path = "../fabro-util" } fork.workspace = true git2.workspace = true diff --git a/lib/crates/fabro-telemetry/src/context.rs b/lib/crates/fabro-telemetry/src/context.rs index 347f28e3d..63cec563d 100644 --- a/lib/crates/fabro-telemetry/src/context.rs +++ b/lib/crates/fabro-telemetry/src/context.rs @@ -1,3 +1,4 @@ +use fabro_static::EnvVars; use serde_json::{Value, json}; pub fn build_context() -> Value { @@ -9,8 +10,12 @@ pub fn build_context() -> Value { }) } +#[expect( + clippy::disallowed_methods, + reason = "Telemetry context captures the conventional LANG locale from process env." +)] fn current_locale() -> String { - let lang = std::env::var("LANG").unwrap_or_default(); + let lang = std::env::var(EnvVars::LANG).unwrap_or_default(); parse_locale(&lang) } diff --git a/lib/crates/fabro-telemetry/src/lib.rs b/lib/crates/fabro-telemetry/src/lib.rs index 9cdbb3910..d80dea4f9 100644 --- a/lib/crates/fabro-telemetry/src/lib.rs +++ b/lib/crates/fabro-telemetry/src/lib.rs @@ -13,6 +13,7 @@ use std::thread::JoinHandle; use chrono::Utc; use event::{Track, User}; +use fabro_static::EnvVars; use serde_json::Value; use uuid::Uuid; @@ -186,8 +187,12 @@ macro_rules! track { }; } +#[expect( + clippy::disallowed_methods, + reason = "Telemetry initialization reads the documented FABRO_TELEMETRY process-env control." +)] pub fn telemetry_level() -> TelemetryLevel { - telemetry_level_from(std::env::var("FABRO_TELEMETRY").ok().as_deref()) + telemetry_level_from(std::env::var(EnvVars::FABRO_TELEMETRY).ok().as_deref()) } pub fn telemetry_level_from(env_value: Option<&str>) -> TelemetryLevel { diff --git a/lib/crates/fabro-telemetry/src/spawn.rs b/lib/crates/fabro-telemetry/src/spawn.rs index 08e4abe05..0636b6773 100644 --- a/lib/crates/fabro-telemetry/src/spawn.rs +++ b/lib/crates/fabro-telemetry/src/spawn.rs @@ -3,6 +3,8 @@ reason = "sync pre-fork filesystem interaction; the whole module runs before fork/exec" )] +use fabro_static::EnvVars; + /// Spawn a fully detached subprocess that survives parent exit and terminal /// close. /// @@ -150,8 +152,8 @@ pub fn spawn_fabro_subcommand(subcommand: &str, filename: &str, json: &[u8]) { spawn_detached( &[&exe, subcommand, &path_str], - &[("FABRO_TELEMETRY", "off")], - &["FABRO_JSON"], + &[(EnvVars::FABRO_TELEMETRY, "off")], + &[EnvVars::FABRO_JSON], ); } diff --git a/lib/crates/fabro-test/Cargo.toml b/lib/crates/fabro-test/Cargo.toml index 8fde15dbf..afe5e3db8 100644 --- a/lib/crates/fabro-test/Cargo.toml +++ b/lib/crates/fabro-test/Cargo.toml @@ -17,6 +17,7 @@ assert_cmd = "2" axum = { workspace = true } fabro-config = { path = "../fabro-config" } fabro-proc = { path = "../fabro-proc" } +fabro-static.workspace = true fabro-types = { path = "../fabro-types" } fabro-util = { path = "../fabro-util" } fabro-http.workspace = true diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 4c9f690bf..1376454c3 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -1,8 +1,8 @@ #![expect( clippy::disallowed_methods, reason = "fabro-test: shared test infrastructure; sync std::fs throughout is intentional for \ - test fixtures, snapshots, and scratch directories. Tokio-path code under test sits \ - in other crates." + test fixtures, snapshots, scratch directories, and process-env harnessing. \ + Tokio-path code under test sits in other crates." )] use std::collections::HashMap; @@ -15,8 +15,8 @@ use std::time::{Duration, SystemTime, UNIX_EPOCH}; use assert_cmd::Command; use fabro_config::daemon::ServerDaemon; use fabro_config::{RuntimeDirectory, Storage, envfile}; +pub use fabro_static::EnvVars; use fabro_types::RunId; -use fabro_util::browser; use regex::Regex; use serde_json::{Map, Value, json}; use toml::Value as TomlValue; @@ -39,8 +39,8 @@ pub use http_assert::{ #[macro_export] macro_rules! preserve_coverage_env { ($cmd:expr) => {{ - if let Some(val) = ::std::env::var_os("LLVM_PROFILE_FILE") { - $cmd.env("LLVM_PROFILE_FILE", val); + if let Some(val) = ::std::env::var_os($crate::EnvVars::LLVM_PROFILE_FILE) { + $cmd.env($crate::EnvVars::LLVM_PROFILE_FILE, val); } }}; } @@ -77,7 +77,6 @@ static INSTA_FILTERS: &[(&str, &str)] = &[ ]; const MANAGED_STORAGE_MARKER: &str = "# fabro-test managed storage_dir"; -const TEST_IN_MEMORY_STORE_ENV: &str = "FABRO_TEST_IN_MEMORY_STORE"; const SESSION_LOCK_TIMEOUT: Duration = Duration::from_secs(20); const TEST_SESSION_SECRET: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; @@ -95,10 +94,10 @@ pub enum TestMode { impl TestMode { #[must_use] pub fn from_env() -> Self { - match std::env::var("FABRO_TEST_MODE").as_deref() { + match std::env::var(EnvVars::FABRO_TEST_MODE).as_deref() { Ok("live") => Self::Live, Ok("strict") => Self::Strict, - _ => match std::env::var("NEXTEST_PROFILE").as_deref() { + _ => match std::env::var(EnvVars::NEXTEST_PROFILE).as_deref() { Ok("e2e") => Self::Strict, _ => Self::Twin, }, @@ -157,20 +156,20 @@ fn apply_test_isolation_with_lookup( lookup: impl Fn(&str) -> Option, ) { cmd.env_clear(); - if let Some(coverage) = lookup("LLVM_PROFILE_FILE") { - cmd.env("LLVM_PROFILE_FILE", coverage); + if let Some(coverage) = lookup(EnvVars::LLVM_PROFILE_FILE) { + cmd.env(EnvVars::LLVM_PROFILE_FILE, coverage); } - if let Some(path) = lookup("PATH") { - cmd.env("PATH", path); + if let Some(path) = lookup(EnvVars::PATH) { + cmd.env(EnvVars::PATH, path); } - cmd.env("NO_COLOR", "1"); - cmd.env("HOME", home_dir); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true") - .env("FABRO_HTTP_PROXY_POLICY", "disabled") - .env("FABRO_TELEMETRY", "off") - .env(browser::SUPPRESS_ENV_VAR, "1"); - cmd.env("FABRO_SERVER_MAX_CONCURRENT_RUNS", "64"); - cmd.env(TEST_IN_MEMORY_STORE_ENV, "1"); + cmd.env(EnvVars::NO_COLOR, "1"); + cmd.env(EnvVars::HOME, home_dir); + cmd.env(EnvVars::FABRO_NO_UPGRADE_CHECK, "true") + .env(EnvVars::FABRO_HTTP_PROXY_POLICY, "disabled") + .env(EnvVars::FABRO_TELEMETRY, "off") + .env(EnvVars::FABRO_SUPPRESS_OPEN_BROWSER, "1"); + cmd.env(EnvVars::FABRO_SERVER_MAX_CONCURRENT_RUNS, "64"); + cmd.env(EnvVars::FABRO_TEST_IN_MEMORY_STORE, "1"); } /// Create a fresh tempdir containing an empty `storage/` subdirectory, for @@ -355,7 +354,7 @@ fn current_pid() -> u32 { } fn session_paths() -> (SessionMode, String, SessionPaths) { - let run_id = std::env::var("NEXTEST_RUN_ID").ok(); + let run_id = std::env::var(EnvVars::NEXTEST_RUN_ID).ok(); session_paths_for_run_id(run_id.as_deref()) } @@ -1778,8 +1777,8 @@ impl TwinGitHub { impl TwinOpenAi { pub fn configure_command(&self, cmd: &mut Command, namespace: &str) { - cmd.env("OPENAI_BASE_URL", &self.base_url); - cmd.env("OPENAI_API_KEY", namespace); + cmd.env(EnvVars::OPENAI_BASE_URL, &self.base_url); + cmd.env(EnvVars::OPENAI_API_KEY, namespace); } #[must_use] @@ -2077,9 +2076,9 @@ macro_rules! e2e_openai { let api_key = format!("{}::{}", module_path!(), line!()); (twin.base_url.clone(), api_key) } else { - let base_url = std::env::var("OPENAI_BASE_URL") + let base_url = std::env::var($crate::EnvVars::OPENAI_BASE_URL) .unwrap_or_else(|_| "https://api.openai.com/v1".to_string()); - let api_key = std::env::var("OPENAI_API_KEY") + let api_key = std::env::var($crate::EnvVars::OPENAI_API_KEY) .expect("OPENAI_API_KEY must be set in live/strict mode"); (base_url, api_key) } @@ -2108,11 +2107,11 @@ mod tests { let envs = cmd.get_envs().collect::>(); assert!(envs.iter().any(|(key, value)| { - *key == std::ffi::OsStr::new("OPENAI_BASE_URL") + *key == std::ffi::OsStr::new(EnvVars::OPENAI_BASE_URL) && *value == Some(std::ffi::OsStr::new("http://127.0.0.1:3000/v1")) }),); assert!(envs.iter().any(|(key, value)| { - *key == std::ffi::OsStr::new("OPENAI_API_KEY") + *key == std::ffi::OsStr::new(EnvVars::OPENAI_API_KEY) && *value == Some(std::ffi::OsStr::new("test-namespace")) }),); } @@ -2127,10 +2126,12 @@ mod tests { let home = tempfile::tempdir().expect("temp home should be created"); let mut cmd = std::process::Command::new("/usr/bin/env"); apply_test_isolation_with_lookup(&mut cmd, home.path(), |name| match name { - "PATH" => Some(std::ffi::OsString::from("/usr/bin:/bin")), - "LLVM_PROFILE_FILE" => Some(std::ffi::OsString::from("/tmp/coverage.profraw")), - "GITHUB_TOKEN" => Some(std::ffi::OsString::from("sentinel-should-not-leak")), - "ANTHROPIC_API_KEY" => Some(std::ffi::OsString::from("sentinel-also-should-not-leak")), + EnvVars::PATH => Some(std::ffi::OsString::from("/usr/bin:/bin")), + EnvVars::LLVM_PROFILE_FILE => Some(std::ffi::OsString::from("/tmp/coverage.profraw")), + EnvVars::GITHUB_TOKEN => Some(std::ffi::OsString::from("sentinel-should-not-leak")), + EnvVars::ANTHROPIC_API_KEY => { + Some(std::ffi::OsString::from("sentinel-also-should-not-leak")) + } _ => None, }); let output = cmd.output().expect("/usr/bin/env should execute"); diff --git a/lib/crates/fabro-util/Cargo.toml b/lib/crates/fabro-util/Cargo.toml index 68efa2166..79abe6bbf 100644 --- a/lib/crates/fabro-util/Cargo.toml +++ b/lib/crates/fabro-util/Cargo.toml @@ -14,6 +14,7 @@ workspace = true [dependencies] console.workspace = true +fabro-static.workspace = true rand.workspace = true regex.workspace = true termimad.workspace = true diff --git a/lib/crates/fabro-util/src/browser.rs b/lib/crates/fabro-util/src/browser.rs index 5de379f90..a18af2436 100644 --- a/lib/crates/fabro-util/src/browser.rs +++ b/lib/crates/fabro-util/src/browser.rs @@ -1,11 +1,15 @@ +use fabro_static::EnvVars; + /// When this environment variable is set to any value, [`try_open`] returns /// `Ok(())` without launching a browser. Test harnesses set it so spawned /// `fabro` subprocesses do not pop real browser windows during CI or local /// runs. -pub const SUPPRESS_ENV_VAR: &str = "FABRO_SUPPRESS_OPEN_BROWSER"; - +#[expect( + clippy::disallowed_methods, + reason = "Browser launching checks the documented process-env escape hatch." +)] pub fn try_open(url: &str) -> std::io::Result<()> { - if std::env::var_os(SUPPRESS_ENV_VAR).is_some() { + if std::env::var_os(EnvVars::FABRO_SUPPRESS_OPEN_BROWSER).is_some() { return Ok(()); } open::that(url) diff --git a/lib/crates/fabro-util/src/env.rs b/lib/crates/fabro-util/src/env.rs index e3109fed3..25bcad6c5 100644 --- a/lib/crates/fabro-util/src/env.rs +++ b/lib/crates/fabro-util/src/env.rs @@ -12,6 +12,10 @@ pub trait Env: Send + Sync { pub struct SystemEnv; impl Env for SystemEnv { + #[expect( + clippy::disallowed_methods, + reason = "SystemEnv is the intentional process-env facade behind the Env trait." + )] fn var(&self, key: &str) -> Result { std::env::var(key) } diff --git a/lib/crates/fabro-util/src/home.rs b/lib/crates/fabro-util/src/home.rs index f8d77cfb8..89d1da9b8 100644 --- a/lib/crates/fabro-util/src/home.rs +++ b/lib/crates/fabro-util/src/home.rs @@ -1,5 +1,7 @@ use std::path::{Path, PathBuf}; +use fabro_static::EnvVars; + #[derive(Clone, Debug, PartialEq, Eq)] pub struct Home { root: PathBuf, @@ -12,6 +14,10 @@ impl Home { } #[must_use] + #[expect( + clippy::disallowed_methods, + reason = "Home::from_env is the process-env facade for resolving Fabro home paths." + )] pub fn from_env() -> Self { Self::from_env_with_lookup(|name| std::env::var_os(name), dirs::home_dir()) } @@ -21,11 +27,11 @@ impl Home { lookup: impl Fn(&str) -> Option, fallback_home: Option, ) -> Self { - if let Some(root) = lookup("FABRO_HOME") { + if let Some(root) = lookup(EnvVars::FABRO_HOME) { return Self::new(root); } - if let Some(home) = lookup("HOME") { + if let Some(home) = lookup(EnvVars::HOME) { return Self::new(PathBuf::from(home).join(".fabro")); } @@ -77,7 +83,7 @@ impl Home { #[cfg(test)] mod tests { - use super::Home; + use super::{EnvVars, Home}; #[test] fn accessors_are_relative_to_root() { @@ -115,7 +121,7 @@ mod tests { fn from_env_prefers_home_env_when_fabro_home_is_absent() { let home = Home::from_env_with_lookup( |name| match name { - "HOME" => Some(std::ffi::OsString::from("/tmp/fabro-home-env")), + EnvVars::HOME => Some(std::ffi::OsString::from("/tmp/fabro-home-env")), _ => None, }, None, diff --git a/lib/crates/fabro-workflow/Cargo.toml b/lib/crates/fabro-workflow/Cargo.toml index b04d92e8d..932e092b4 100644 --- a/lib/crates/fabro-workflow/Cargo.toml +++ b/lib/crates/fabro-workflow/Cargo.toml @@ -37,6 +37,7 @@ fabro-model = { path = "../fabro-model" } fabro-retro = { path = "../fabro-retro" } fabro-core = { path = "../fabro-core" } fabro-store = { path = "../fabro-store" } +fabro-static.workspace = true fabro-types = { path = "../fabro-types" } fabro-http.workspace = true thiserror.workspace = true diff --git a/lib/crates/fabro-workflow/src/handler/llm/cli.rs b/lib/crates/fabro-workflow/src/handler/llm/cli.rs index bcc78df43..fe54690f9 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/cli.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/cli.rs @@ -563,7 +563,7 @@ impl CodergenBackend for AgentCliBackend { } else { let mut env = HashMap::new(); for name in provider.api_key_env_vars() { - if let Ok(val) = std::env::var(name) { + if let Some(val) = process_env_var(name) { env.insert((*name).to_string(), val); } } @@ -740,6 +740,14 @@ impl CodergenBackend for AgentCliBackend { } } +#[expect( + clippy::disallowed_methods, + reason = "CLI agent fallback credentials intentionally read provider API-key env vars." +)] +fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() +} + /// Routes codergen invocations to either the API backend or CLI backend /// based on node attributes and model type. pub struct BackendRouter { diff --git a/lib/crates/fabro-workflow/src/operations/archive.rs b/lib/crates/fabro-workflow/src/operations/archive.rs index b41145d06..4c3ecb707 100644 --- a/lib/crates/fabro-workflow/src/operations/archive.rs +++ b/lib/crates/fabro-workflow/src/operations/archive.rs @@ -1,16 +1,10 @@ -use fabro_store::{Database, Error as StoreError}; +use fabro_store::Database; use fabro_types::{ActorRef, RunId, RunStatus, TerminalStatus}; +use super::run_store::map_open_run_error; use crate::error::Error; use crate::event::{self, Event}; -fn map_open_run_error(run_id: &RunId, err: StoreError) -> Error { - match err { - StoreError::RunNotFound(id) => Error::RunNotFound(id), - other => Error::engine(format!("failed to open run {run_id}: {other}")), - } -} - /// The canonical "run is archived — mutation rejected" error message. Shared /// by the operations layer, the CLI rewind precheck, and the server HTTP /// guards so the user sees the same actionable guidance everywhere. diff --git a/lib/crates/fabro-workflow/src/operations/fork.rs b/lib/crates/fabro-workflow/src/operations/fork.rs index 4306b0993..091b4594c 100644 --- a/lib/crates/fabro-workflow/src/operations/fork.rs +++ b/lib/crates/fabro-workflow/src/operations/fork.rs @@ -6,7 +6,7 @@ use fabro_types::RunId; use git2::{Oid, Signature}; use super::run_git; -use super::timeline::{ForkTarget, TimelineEntry, build_timeline}; +use super::timeline::{ForkTarget, RunTimeline, TimelineEntry, build_timeline}; use crate::error::Error; use crate::event::{self, Event}; use crate::git::{MetadataStore, RUN_BRANCH_PREFIX, push_run_branches}; @@ -51,14 +51,10 @@ struct ForkedRun { /// checkpoint. /// /// Returns the new run ID. -pub fn fork(store: &Store, input: &ForkRunInput) -> Result { +#[cfg(test)] +fn fork(store: &Store, input: &ForkRunInput) -> Result { let timeline = build_timeline(store, &input.source_run_id.to_string())?; - let entry = match input.target.as_ref() { - Some(target) => timeline.resolve(target)?, - None => timeline.entries.last().ok_or_else(|| { - anyhow::anyhow!("no checkpoints found for run {}", input.source_run_id) - })?, - }; + let entry = resolve_fork_entry(&timeline, &input.source_run_id, input.target.as_ref())?; Ok(fork_from_entry(store, &input.source_run_id, entry, input.push)?.new_run_id) } @@ -71,14 +67,8 @@ pub async fn fork_run(store: &Database, input: &ForkRunInput) -> Result timeline - .resolve(target) - .map_err(|err| Error::Validation(err.to_string()))?, - None => timeline.entries.last().ok_or_else(|| { - Error::Validation(format!("no checkpoints found for run {source_run_id}")) - })?, - }; + let entry = resolve_fork_entry(&timeline, &source_run_id, target.as_ref()) + .map_err(|err| Error::Validation(err.to_string()))?; let resolved = ResolvedForkTarget { checkpoint_ordinal: entry.ordinal, node_id: entry.node_name.clone(), @@ -100,6 +90,20 @@ pub async fn fork_run(store: &Database, input: &ForkRunInput) -> Result( + timeline: &'a RunTimeline, + source_run_id: &RunId, + target: Option<&ForkTarget>, +) -> Result<&'a TimelineEntry> { + match target { + Some(target) => timeline.resolve(target), + None => timeline + .entries + .last() + .ok_or_else(|| anyhow::anyhow!("no checkpoints found for run {source_run_id}")), + } +} + fn fork_from_entry( store: &Store, source_run_id: &RunId, diff --git a/lib/crates/fabro-workflow/src/operations/mod.rs b/lib/crates/fabro-workflow/src/operations/mod.rs index 92fc64252..368b1c4e9 100644 --- a/lib/crates/fabro-workflow/src/operations/mod.rs +++ b/lib/crates/fabro-workflow/src/operations/mod.rs @@ -5,6 +5,7 @@ mod rebuild_meta; mod resume; mod rewind; mod run_git; +mod run_store; mod source; mod start; #[cfg(test)] @@ -17,7 +18,7 @@ pub use archive::{ unarchive, }; pub use create::{CreateRunInput, CreatedRun, create, make_run_dir}; -pub use fork::{ForkOutcome, ForkRunInput, ResolvedForkTarget, fork, fork_run}; +pub use fork::{ForkOutcome, ForkRunInput, ResolvedForkTarget, fork_run}; pub use rebuild_meta::{ build_timeline_or_rebuild, find_run_id_by_prefix_or_store, rebuild_metadata_branch, }; diff --git a/lib/crates/fabro-workflow/src/operations/rewind.rs b/lib/crates/fabro-workflow/src/operations/rewind.rs index 5f4db7be7..69883b4d4 100644 --- a/lib/crates/fabro-workflow/src/operations/rewind.rs +++ b/lib/crates/fabro-workflow/src/operations/rewind.rs @@ -1,5 +1,5 @@ use fabro_store::Database; -use fabro_types::{ActorRef, RunId, RunStatus}; +use fabro_types::{ActorRef, RunId}; use tracing::error; use super::fork::{self, ForkOutcome, ForkRunInput, ResolvedForkTarget}; @@ -21,7 +21,6 @@ pub enum RewindOutcome { source_run_id: RunId, new_run_id: RunId, target: ResolvedForkTarget, - archived: bool, }, Partial { source_run_id: RunId, @@ -41,15 +40,8 @@ pub async fn rewind( Error::Precondition(format!("run {} has no status; cannot rewind", input.run_id)) })?; - if matches!(current, RunStatus::Archived { .. }) { - return Err(Error::Precondition(archive::archived_rejection_message( - &input.run_id, - ))); - } - if !matches!( - current, - RunStatus::Succeeded { .. } | RunStatus::Failed { .. } | RunStatus::Dead - ) { + archive::ensure_not_archived(Some(current), &input.run_id)?; + if current.terminal_status().is_none() { return Err(Error::Precondition(format!( "run {} must be terminal (succeeded, failed, or dead) to rewind; current status is {current}", input.run_id @@ -65,12 +57,11 @@ pub async fn rewind( match archive::archive(store, &input.run_id, actor).await { Ok(_) => { - append_superseded_event(store, &forked).await; + append_superseded_event_best_effort(store, &forked).await; Ok(RewindOutcome::Full { source_run_id: forked.source_run_id, new_run_id: forked.new_run_id, target: forked.target, - archived: true, }) } Err(err) => Ok(RewindOutcome::Partial { @@ -82,7 +73,7 @@ pub async fn rewind( } } -async fn append_superseded_event(store: &Database, forked: &ForkOutcome) { +async fn append_superseded_event_best_effort(store: &Database, forked: &ForkOutcome) { let run_store = match store.open_run(&forked.source_run_id).await { Ok(run_store) => run_store, Err(err) => { diff --git a/lib/crates/fabro-workflow/src/operations/run_git.rs b/lib/crates/fabro-workflow/src/operations/run_git.rs index e4945ea33..b35c7c252 100644 --- a/lib/crates/fabro-workflow/src/operations/run_git.rs +++ b/lib/crates/fabro-workflow/src/operations/run_git.rs @@ -1,18 +1,12 @@ use fabro_checkpoint::git::Store as GitStore; -use fabro_store::{Database, Error as StoreError}; +use fabro_store::Database; use fabro_types::{RunId, RunProjection}; use git2::Repository; use tokio::task::spawn_blocking; +use super::run_store::map_open_run_error; use crate::error::Error; -fn map_open_run_error(run_id: &RunId, err: StoreError) -> Error { - match err { - StoreError::RunNotFound(id) => Error::RunNotFound(id), - other => Error::engine(format!("failed to open run {run_id}: {other}")), - } -} - pub(crate) async fn load_projection( store: &Database, run_id: &RunId, diff --git a/lib/crates/fabro-workflow/src/operations/run_store.rs b/lib/crates/fabro-workflow/src/operations/run_store.rs new file mode 100644 index 000000000..d60e56ce2 --- /dev/null +++ b/lib/crates/fabro-workflow/src/operations/run_store.rs @@ -0,0 +1,11 @@ +use fabro_store::Error as StoreError; +use fabro_types::RunId; + +use crate::error::Error; + +pub(super) fn map_open_run_error(run_id: &RunId, err: StoreError) -> Error { + match err { + StoreError::RunNotFound(id) => Error::RunNotFound(id), + other => Error::engine(format!("failed to open run {run_id}: {other}")), + } +} diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 7cff959ba..02f3f0947 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -16,6 +16,7 @@ use fabro_sandbox::config::{ }; use fabro_sandbox::daytona::{DaytonaConfig, detect_repo_info}; use fabro_sandbox::{SandboxProvider, SandboxSpec}; +use fabro_static::EnvVars; use fabro_types::RunId; use fabro_types::settings::InterpString; use fabro_types::settings::run::{ @@ -362,7 +363,11 @@ impl RunSession { }, SandboxProvider::Daytona => { let api_key = match &services.vault { - Some(v) => v.read().await.get("DAYTONA_API_KEY").map(str::to_string), + Some(v) => v + .read() + .await + .get(EnvVars::DAYTONA_API_KEY) + .map(str::to_string), None => None, }; SandboxSpec::Daytona { @@ -451,10 +456,18 @@ impl RunSession { fn resolve_interp(value: &InterpString) -> String { value - .resolve(|name| std::env::var(name).ok()) + .resolve(process_env_var) .map_or_else(|_| value.as_source(), |resolved| resolved.value) } +#[expect( + clippy::disallowed_methods, + reason = "Run startup interpolation owns a process-env lookup facade for {{ env.* }} values." +)] +fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() +} + async fn load_accepted_run_definition( run_store: &RunStoreHandle, blob_id: fabro_types::RunBlobId, diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index 8361f2a7d..9d14c14f5 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -13,7 +13,7 @@ )] #![expect( clippy::disallowed_methods, - reason = "These Daytona integration tests use the real git CLI to prepare remote-repo fixtures for workflow runs." + reason = "These Daytona integration tests use real process env and git CLI fixtures for workflow runs." )] use std::collections::HashMap; @@ -26,6 +26,7 @@ use fabro_agent::Sandbox; use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; use fabro_llm::provider::Provider; use fabro_sandbox::daytona::{DaytonaConfig, DaytonaSandbox, DaytonaSnapshotConfig}; +use fabro_static::EnvVars; use fabro_store::{ArtifactStore, Database}; use fabro_types::{RunId, StageId, WorkflowSettings}; use fabro_workflow::artifact::sync_artifacts_to_env; @@ -194,7 +195,8 @@ fn load_github_app_credentials() -> fabro_github::GitHubCredentials { .app_id .expect("app_id not set in settings.toml [git] section"); - let raw = std::env::var("GITHUB_APP_PRIVATE_KEY").expect("GITHUB_APP_PRIVATE_KEY not set"); + let raw = + std::env::var(EnvVars::GITHUB_APP_PRIVATE_KEY).expect("GITHUB_APP_PRIVATE_KEY not set"); let private_key_pem = if raw.starts_with("-----") { raw } else { @@ -1719,13 +1721,13 @@ async fn daytona_toolbox_idle_diagnostic() { eprintln!("[t=+{sleep_secs}s] FAILED: {e}"); // Diagnose with raw HTTP calls - let api_key = std::env::var("DAYTONA_API_KEY").unwrap_or_default(); + let api_key = std::env::var(EnvVars::DAYTONA_API_KEY).unwrap_or_default(); let client = fabro_http::HttpClientBuilder::new() .timeout(std::time::Duration::from_secs(15)) .build() .unwrap(); - let api_url = std::env::var("DAYTONA_API_URL") - .or_else(|_| std::env::var("DAYTONA_SERVER_URL")) + let api_url = std::env::var(EnvVars::DAYTONA_API_URL) + .or_else(|_| std::env::var(EnvVars::DAYTONA_SERVER_URL)) .unwrap_or_else(|_| "https://app.daytona.io/api".to_string()); // Check sandbox state diff --git a/test/twin/openai/Cargo.toml b/test/twin/openai/Cargo.toml index d66836c33..4641a47ed 100644 --- a/test/twin/openai/Cargo.toml +++ b/test/twin/openai/Cargo.toml @@ -17,6 +17,7 @@ anyhow.workspace = true async-stream = "0.3" axum = { workspace = true, features = ["macros"] } fabro-http.workspace = true +fabro-static.workspace = true futures-util.workspace = true http = "1" serde.workspace = true diff --git a/test/twin/openai/src/config.rs b/test/twin/openai/src/config.rs index f7cd31dbd..1a1cf146b 100644 --- a/test/twin/openai/src/config.rs +++ b/test/twin/openai/src/config.rs @@ -1,6 +1,7 @@ use std::net::{IpAddr, Ipv4Addr, SocketAddr}; use anyhow::{Context, Result}; +use fabro_static::EnvVars; #[derive(Clone, Debug)] pub struct Config { @@ -11,22 +12,22 @@ pub struct Config { impl Config { pub fn from_env() -> Result { - Self::from_lookup(&|name| std::env::var(name).ok()) + Self::from_lookup(&process_env_var) } pub fn from_lookup(lookup: &dyn Fn(&str) -> Option) -> Result { - let bind_addr = lookup("TWIN_OPENAI_BIND_ADDR") + let bind_addr = lookup(EnvVars::TWIN_OPENAI_BIND_ADDR) .map(|value| value.parse().context("invalid TWIN_OPENAI_BIND_ADDR")) .transpose()? .unwrap_or_else(|| SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 3000)); - let require_auth = lookup("TWIN_OPENAI_REQUIRE_AUTH") - .map(|value| parse_bool_env(&value, "TWIN_OPENAI_REQUIRE_AUTH")) + let require_auth = lookup(EnvVars::TWIN_OPENAI_REQUIRE_AUTH) + .map(|value| parse_bool_env(&value, EnvVars::TWIN_OPENAI_REQUIRE_AUTH)) .transpose()? .unwrap_or(true); - let enable_admin = lookup("TWIN_OPENAI_ENABLE_ADMIN") - .map(|value| parse_bool_env(&value, "TWIN_OPENAI_ENABLE_ADMIN")) + let enable_admin = lookup(EnvVars::TWIN_OPENAI_ENABLE_ADMIN) + .map(|value| parse_bool_env(&value, EnvVars::TWIN_OPENAI_ENABLE_ADMIN)) .transpose()? .unwrap_or(true); @@ -38,6 +39,14 @@ impl Config { } } +#[expect( + clippy::disallowed_methods, + reason = "twin-openai config owns a process-env lookup facade for its test server settings." +)] +fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() +} + impl Default for Config { fn default() -> Self { Self::from_env().unwrap_or(Self { diff --git a/test/twin/openai/tests/live_openai_contract.rs b/test/twin/openai/tests/live_openai_contract.rs index d33db3a44..dfe7998d6 100644 --- a/test/twin/openai/tests/live_openai_contract.rs +++ b/test/twin/openai/tests/live_openai_contract.rs @@ -6,6 +6,7 @@ mod common; use anyhow::{Context, Result, anyhow, bail, ensure}; +use fabro_static::EnvVars; use serde_json::{Value, json}; const DEFAULT_LIVE_BASE_URL: &str = "https://api.openai.com"; @@ -210,15 +211,15 @@ async fn live_openai_contract_smoke_suite() { impl LiveOptions { fn from_env() -> Result> { - let Some(api_key) = non_empty_env("OPENAI_API_KEY") else { + let Some(api_key) = non_empty_env(EnvVars::OPENAI_API_KEY) else { return Ok(None); }; - let base_url = non_empty_env("TWIN_OPENAI_LIVE_BASE_URL") + let base_url = non_empty_env(EnvVars::TWIN_OPENAI_LIVE_BASE_URL) .unwrap_or_else(|| DEFAULT_LIVE_BASE_URL.to_owned()); - let model = non_empty_env("TWIN_OPENAI_LIVE_MODEL") + let model = non_empty_env(EnvVars::TWIN_OPENAI_LIVE_MODEL) .unwrap_or_else(|| DEFAULT_LIVE_MODEL.to_owned()); - let organization = non_empty_env("OPENAI_ORGANIZATION"); - let project = non_empty_env("OPENAI_PROJECT"); + let organization = non_empty_env(EnvVars::OPENAI_ORGANIZATION); + let project = non_empty_env(EnvVars::OPENAI_PROJECT); Ok(Some(Self { api: common::ApiClient::new(base_url, Some(api_key), organization, project)?, @@ -1836,6 +1837,10 @@ where Ok(()) } +#[expect( + clippy::disallowed_methods, + reason = "Live twin-openai contract tests read documented OpenAI env inputs." +)] fn non_empty_env(name: &str) -> Option { std::env::var(name) .ok()