Commit graph

367 commits

Author SHA1 Message Date
Bryan Helmkamp
9c3c66c59a
test(http): improve HTTP test failure diagnostics
Add shared axum/reqwest response assertion helpers in fabro-test,
migrate the Rust HTTP test surface to use them, and document the
new rule in the testing strategy.
2026-04-20 08:06:14 -04:00
Bryan Helmkamp
f2a512185e
test(auth): add remaining CLI auth integration coverage 2026-04-20 08:05:22 -04:00
Bryan Helmkamp
9c68c57bcc
feat(auth): add CLI GitHub login and logout flow
Add the server-side CLI OAuth endpoints and token persistence needed to
mint JWT access tokens and rotating refresh tokens from the existing
GitHub web auth flow.

Add CLI auth storage plus `fabro auth login`, `logout`, and `status`, and
prefer stored OAuth access tokens when building target clients.
2026-04-20 07:12:52 -04:00
Bryan Helmkamp
4b3835b8ee
test(server): cover resolve_head_sha_and_time parser + two-phase fetch
Two coverage gaps closed:

1. `parse_head_show_output` — extracted from `resolve_head_sha_and_time`
   as a pure function so it can be tested without a sandbox. Six tests
   cover: well-formed sha+iso line, non-UTC timezone normalization,
   sha-only output (missing %cI), malformed date (parser tolerates
   and returns sha with None date), empty-input rejection, and
   surrounding-whitespace tolerance. New code from the simplify pass,
   previously unverified.

2. `fetch_blob_table` two-phase error isolation — `ScriptedBlobSandbox`
   (hand-written minimal Sandbox impl) returns different exec responses
   for `cat-file --batch-check` vs `cat-file --batch`. The phase-2
   failure test proves that a malformed --batch parse outcome doesn't
   corrupt phase-1-classified oversized entries — the doc-comment's
   promise that the two phases are isolated now has a regression test
   behind it. The phase-1-skip test enforces the
   METADATA_PHASE_SHA_THRESHOLD contract by making phase 1's
   batch-check response an error: if the threshold logic regressed
   and phase 1 ran, the test would fail with a 503.

Also adds `Debug` to `ApiError` (required by `Result::expect` in the
new tests) and adds `async-trait`/`tokio-util` as dev-dependencies
plus the `test-support` feature on fabro-sandbox.

Total workspace test count: 4173 -> 4180.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-19 19:21:19 -04:00
Bryan Helmkamp
0c77a184ab
fix(web,server): pierre 1.1 API fit + unicode-safe path normalize
Three follow-ups from verification against the actual @pierre/diffs
1.1.15 type definitions:

1. Deep-link expand uses `options.expandUnchanged: true` on the
   targeted MultiFileDiff rather than firing `el.click()` on the outer
   wrapper. Pierre 1.1.x exposes no imperative expand API — click on
   the row container was a no-op. Per-file expansion now fires on
   mount when the file name matches the URL hash.

2. Enter/Space binding removed from useFileKeyboardNav — click on the
   outer row doesn't trigger anything in pierre's model, and binding
   it just delayed default browser scroll behavior on Space. j/k
   focus navigation remains the working keyboard affordance. When a
   pierre imperative expand API appears, Enter/Space can be re-added
   to call it.

3. normalize_for_match strip loop now iterates to a fixed point
   against the fully-lowercased string so repeated `./` / `../` / `/`
   prefixes are all stripped. Added Windows-path and Unicode-uppercase
   regression tests for is_sensitive to verify basename matching
   survives both.

Virtualizer usage verified against the 1.1.x type definitions: the
`{ children: ReactNode }` signature accepts the wrapped file list
directly with no Virtualizer.Item wrapper needed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-19 17:55:20 -04:00
Bryan Helmkamp
1ef72fb6bc
refactor(server): extract run_files_security with globset denylist (P3-2)
Moves the sensitive-path denylist, sandbox-git env helper, and metrics
emitter into a dedicated run_files_security module so the Run Files
Changed endpoint has a single, testable surface for security controls.

Denylist upgrades to globset::GlobSet with two explicit lists:
- Basename globs: .env, .env.*, *.pem, id_rsa, id_rsa.*, id_ed25519*,
  *.p12, *.keystore, *.key
- Path-suffix globs: .aws/credentials, .git/config, .ssh/**

Matching semantics explicitly pinned:
- Case-insensitive via lowercased normalization
- Path traversal (`../`, `./`, leading `/`) stripped before match
- Basename globs match the final segment only — prevents
  `log/.env_audit/data.txt` from matching `.env.*`
- Empty/pathological paths fail closed (sensitive=true safe default)

Also ships:
- sandbox_git_env() returning the env-hardening map
- RunFilesMetrics struct + emit() so tracing never leaks paths/contents

Handler migrates to consume the new module; inline denylist and inline
info!() call removed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-19 17:35:26 -04:00
Bryan Helmkamp
c6ff36d9fb
refactor(install): consolidate shared primitives in fabro-install crate
The `fabro-install` crate was introduced for the web wizard but the CLI
kept its own copies of the same JWT keypair generation, TOML merging,
and GitHub auth settings helpers. Delete the duplicates and route the
CLI through `fabro_install::*`. The CLI keeps a thin
`merge_server_settings` wrapper because it only ever binds TCP and
derives the authority from `--web-url`.

Also tighten `persist_install_outputs_direct` to take its
`PendingSettingsWrite` argument by reference (satisfies
`needless_pass_by_value`) and pull the remaining absolute paths in the
crate's test module into `use` statements, clearing the nightly clippy
warnings that this branch was carrying.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-19 13:54:47 -04:00
Bryan Helmkamp
985373cad4
chore(lockfile): sync merged workspace versions 2026-04-19 13:37:10 -04:00
Bryan Helmkamp
dd4e467bfc
Merge remote-tracking branch 'origin/main'
# Conflicts:
#	lib/crates/fabro-server/tests/it/api/mod.rs
2026-04-19 13:36:54 -04:00
Bryan Helmkamp
e9daf2db3a
Merge remote-tracking branch 'origin/main' 2026-04-19 11:48:15 -04:00
Bryan Helmkamp
ecdfdd82d8
feat(install): add browser-based setup flow
Implement the web-first install experience across the server, CLI, API spec,
web app, and packaged SPA assets.

This also removes test-side process env mutation by pushing env-dependent
decision points behind explicit helpers and test wiring.
2026-04-19 11:20:58 -04:00
Bryan Helmkamp
914778c8a8
refactor(server): remove inbound TLS termination
Remove server-side TLS listener support so Fabro only binds plain TCP
or Unix sockets, and update docs/tests around proxy-terminated HTTPS.
This also drops the removed [server.listen.tls] config shape and the
inbound TLS-specific diagnostics, fixtures, and integration coverage.
2026-04-19 10:43:57 -04:00
fabro-releases[bot]
51faecf266 Bump version to 0.208.0-nightly.1 2026-04-19 14:27:05 +00:00
Bryan Helmkamp
1e03216161
feat(server): add IP allowlist middleware with GitHub webhook support
Introduces a configurable IP allowlist applied to the main API router
and the GitHub webhook listener. Supports CIDR literals plus a
`github_meta_hooks` keyword that resolves live against GitHub's meta
API for the webhooks override. Adds trusted-proxy handling for
X-Forwarded-For, validation that rejects Unix socket listeners without
a trusted proxy count, and deep-merge logic for the new
server.ip_allowlist and per-integration override layers.
2026-04-19 09:47:22 -04:00
fabro-releases[bot]
7933e1ed5c Bump version to 0.208.0-nightly.0
Some checks are pending
Rust / Format (push) Waiting to run
Rust / Clippy (push) Waiting to run
Rust / Test (Linux) (push) Waiting to run
Rust / Test (macOS) (push) Waiting to run
TypeScript / Test (push) Waiting to run
TypeScript / Build (push) Waiting to run
TypeScript / Typecheck (push) Waiting to run
2026-04-19 00:27:15 +00:00
fabro-releases[bot]
769f078bc8 Bump version to 0.207.0-nightly.6 2026-04-18 21:15:59 +00:00
fabro-releases[bot]
6ebe609e71 Bump version to 0.207.0-nightly.5 2026-04-18 17:26:06 +00:00
Bryan Helmkamp
977f4b4a77
deps: bump rand 0.9.2 -> 0.9.4
Patches GHSA-cq8v-f236-94qc (RUSTSEC-2026-0097) for direct rand usage.
The transitive rand 0.8.x remains in the lockfile via cookie, sentry,
slatedb, and phf_generator pending upstream bumps.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-18 13:16:59 -04:00
fabro-releases[bot]
d066592362 Bump version to 0.207.0-nightly.4 2026-04-18 16:47:30 +00:00
fabro-releases[bot]
0bd72d0b2d Bump version to 0.207.0-nightly.3 2026-04-18 16:24:51 +00:00
fabro-releases[bot]
da0999b697 Bump version to 0.207.0-nightly.2 2026-04-18 15:05:58 +00:00
fabro-releases[bot]
459433baa5 Bump version to 0.207.0-nightly.1 2026-04-18 10:05:08 +00:00
Bryan Helmkamp
70d6f929d2
chore: update Cargo.lock for vendored openssl dep
Records openssl 0.10 + openssl-src 300.6.0 in the lockfile for the
target-specific musl dep added in the previous commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-18 01:14:54 -04:00
Bryan Helmkamp
2f2f14562d
fix(api): convert OpenAPI spec to valid 3.1 syntax
The spec declared openapi 3.1.0 but used nullable: true (3.0 idiom)
in 78 places, which Mintlify's parser rejected, breaking doc deploys.

Convert to proper 3.1 patterns (type arrays and oneOf with type: null),
switch the server conformance test from openapiv3 (3.0-only) to a
YAML-level walk so it accepts 3.1 input, and regenerate the typescript
client — it now correctly emits `| null` on nullable fields.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-18 01:11:19 -04:00
Bryan Helmkamp
c8efd5d2d1
Bump version to 0.205.0-nightly.1 2026-04-17 15:39:59 -04:00
Bryan Helmkamp
72399ed1d3
Bump version to 0.205.0-nightly.0 2026-04-17 11:52:58 -04:00
Bryan Helmkamp
3d2c586eda
Bump version to 0.205.0-beta.2 2026-04-17 10:12:06 -04:00
Bryan Helmkamp
8ce451869f
Bump version to 0.205.0-beta.1 2026-04-17 09:21:20 -04:00
Bryan Helmkamp
5946047202
deps: bump rand 0.8 → 0.9 (#163)
## Summary
- Bumps `rand` from 0.8 to 0.9 to resolve Dependabot alert #16 (low
severity: "Rand is unsound with a custom logger using rand::rng()",
fixed in 0.9.3).
- Migrates call sites to the renamed 0.9 APIs.

## API changes applied
- `rand::thread_rng()` → `rand::rng()`
- `Rng::gen_range(..)` → `Rng::random_range(..)`
- `Rng::gen::<T>()` → `Rng::random::<T>()`
- `OsRng.fill_bytes(..)` → `OsRng.try_fill_bytes(..).expect("OS RNG")` —
in 0.9 `OsRng` implements `TryRngCore` instead of `RngCore` directly.

## Test plan
- [x] `cargo build --workspace`
- [x] `cargo nextest run -p fabro-util -p fabro-server -p fabro-workflow
-p fabro-oauth` (1270 passed)
- [x] `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D
warnings`

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-17 07:57:10 -04:00
Bryan Helmkamp
31b4bd801e
Bump version to 0.205.0-beta.0 2026-04-17 07:55:47 -04:00
Bryan Helmkamp
0b16993b30
Bump version to 0.204.0-beta.1
Some checks are pending
Rust / Clippy (push) Waiting to run
Rust / Test (Linux) (push) Waiting to run
Rust / Test (macOS) (push) Waiting to run
Rust / Format (push) Waiting to run
TypeScript / Typecheck (push) Waiting to run
TypeScript / Test (push) Waiting to run
TypeScript / Build (push) Waiting to run
2026-04-16 20:20:37 -04:00
Bryan Helmkamp
fa0c1616f2
Bump version to 0.204.0-beta.0 2026-04-16 20:02:45 -04:00
Bryan Helmkamp
4e7026413a
deps: bump aws-lc-rs 1.16.1 → 1.16.3 (pulls aws-lc-sys 0.40.0)
Resolves Dependabot alerts #4, #5 (aws-lc-sys CRL scope check and X.509
name-constraint bypass; both fixed in 0.39.0+).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-16 19:18:10 -04:00
Bryan Helmkamp
def772ae6f
deps: bump rustls-webpki 0.103.9 → 0.103.12
Resolves Dependabot alerts #7, #17, #18 (name constraint handling).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-16 19:13:32 -04:00
Bryan Helmkamp
8f26a080cf
feat(store): enable Zstd compression for SlateDB
Reduces S3 storage cost and read latency for run data by compressing
SST blocks with Zstd. Existing uncompressed data remains readable.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-14 20:09:30 -04:00
Bryan Helmkamp
3e9683b6a0 refactor(graphviz): extract graphviz-sys into fabro-sh/graphviz-sys
Move the vendored Graphviz FFI crate to its own repo so it can be
reused independently and reduce this repo's footprint (~250 C/H files).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-14 19:03:01 -04:00
Bryan Helmkamp
286eee7efa fix(graphviz): isolate rendering in a subprocess
Run Graphviz through an internal fabro subprocess so renderer failures no
longer share process fate with the server. Keep expected DOT parse failures
on the 400 path via an explicit stdout protocol, and treat child crashes or
protocol violations as 500s.
2026-04-14 18:15:23 -04:00
Bryan Helmkamp
05c7fedd31 refactor(server): remove implicit dry-run fallback
Remove the server startup path that inferred dry-run from provider
availability and let run.execution.mode inherit normally from
settings.

Model tests now return skip for unconfigured providers at request
time, completions use the real error path, and the CLI/docs/tests are
updated for the removed server --dry-run flag.
2026-04-14 12:28:55 -04:00
Bryan Helmkamp
15cf4d8640 refactor: wire vendored Graphviz into fabro-graphviz, remove dot dependency
Replace the Command::new("dot") shell-out in render_dot() with a direct
FFI call to the vendored Graphviz library. Drop PNG support (SVG only).
Remove GraphFormat enum, dot_is_available() helpers, dot-related
diagnostics/doctor checks, and the graphviz install prompt. Update
OpenAPI spec to remove png format and 502 responses. Update CLI help
text, snapshot tests, and documentation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-13 22:46:29 -04:00
Bryan Helmkamp
0d4e4a3cbb feat: add fabro-graphviz-sys crate with vendored Graphviz 14.1.5
Vendor the Graphviz C source code into a new fabro-graphviz-sys crate,
compiled via the cc crate. This eliminates the system dependency on the
dot binary. Pre-generated parser files (grammar.c, scan.c, htmlparse.c)
and table files (colortbl.h, entities.h) are committed alongside the
vendored source. A global Mutex serializes FFI calls to work around
Graphviz's non-thread-safe internal state.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-13 22:34:50 -04:00
Bryan Helmkamp
67819457ad fix(install): harden GitHub app bootstrap
Replace the install-time OpenSSL Ed25519 shell-out with Rust-native key
material generation, drop the stale OpenSSL doctor requirement, and make
GitHub App setup persist valid auth settings and secrets together.

This also fixes the live non-interactive app install path by enabling
GitHub auth, populating allowed usernames, and avoiding half-written
settings when later persistence fails.
2026-04-13 21:20:10 -04:00
Bryan Helmkamp
a1d8b738d8 Add typed provider credential auth flow 2026-04-12 22:10:11 -04:00
Bryan Helmkamp
3fa7b65182 Split server runtime secrets from vault secrets 2026-04-12 14:03:54 -04:00
Bryan Helmkamp
e8ad12fc30 fix: simplify fabro-http crate and fix correctness issues
- Replace unwrap_or_default() with expect() in hooks/llm HTTP client
  builders — Default silently discards all config (timeouts, TLS, proxy)
- Route fabro-mcp through fabro_http instead of raw reqwest, respecting
  FABRO_HTTP_PROXY_POLICY for MCP HTTP transport connections
- Deduplicate HttpClientBuilder / BlockingHttpClientBuilder via macro
- Extract helpers for repeated http_client error handling in diagnostics
  and web_auth
- Remove duplicate test_http_client() in fabro-cli and fabro-llm

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-12 12:20:55 -04:00
Bryan Helmkamp
3b2cffceaf refactor(http): centralize reqwest behind fabro-http
Add the shared fabro-http transport crate and route hand-written HTTP client construction through it.

Use FABRO_HTTP_PROXY_POLICY for test no-proxy defaults, remove direct reqwest deps from ordinary crates, and add clippy bans for raw reqwest entrypoints.
2026-04-12 11:48:54 -04:00
Bryan Helmkamp
f76e28b7a7 refactor: standardize crate error types 2026-04-11 11:53:53 -04:00
Bryan Helmkamp
5ed04c3c57 feat(template): unify workflow and config template syntax
Add a shared MiniJinja-based template crate and migrate workflow prompts,
imports, hooks, and InterpString env references to the new {{ ... }}
syntax. This also threads typed run inputs through workflow rendering and
updates docs and tests to match the new templating model.
2026-04-11 10:58:50 -04:00
Bryan Helmkamp
6572761a82 refactor(settings): resolve server settings in fabro-config
Add the server-side resolved settings view and move server startup,
auth, OAuth, TLS, and settings redaction paths onto that validated
shape. This lands the server pilot slice of the settings refactor
without changing the sparse persisted/API settings model.
2026-04-09 22:37:02 -04:00
Bryan Helmkamp
2b2f158c16 feat(settings): run.goal tagged union (inline | file)
`--goal-file` was broken in the v2 path: `TryFrom<&RunArgs> for ConfigLayer`
did `let _ = &args.goal_file;`, so clap accepted the flag listed in
`--help` and then silently dropped it. Users running
`fabro run demo --goal-file prompts/goal.md` ended up with no goal at
all (or the DOT graph-level fallback), a regression from the legacy
flat `Settings` shape.

This commit adds first-class support for both inline and file-sourced
goals via a tagged union on `run.goal`. Greenfield decisions:

- **Single field, two variants.** `RunGoalLayer` is an untagged enum
  of `Inline(InterpString)` and `File { file: InterpString }`. Makes
  `goal XOR goal_file` un-representable in the type system and lets
  the v2 merge matrix treat `run.goal` as a single scalar
  (last-writer-wins) instead of needing a custom mutual-exclusion
  merge rule. Matches the existing `DaytonaDockerfileLayer` pattern.

- **Relative paths are anchored at the file that declared them.**
  `ConfigLayer::load(path)` walks the just-parsed `SettingsFile` and
  rewrites any literal relative `run.goal.file` path to absolute
  using `path.parent()` as the base, via new
  `fabro_config::config::resolve_goal_file_paths`. CLI-sourced paths
  via `--goal-file` are anchored at CWD in
  `overrides::goal_layer_from_args`. Env-interpolated paths
  (`${env.GOALS_DIR}/goal.md`) are left unresolved until consume time
  and then resolved against the run's working_directory.

- **New accessors, no shims.**
  - `run_goal_layer() -> Option<&RunGoalLayer>` — raw variant access.
  - `run_goal_inline_str() -> Option<String>` — inline-only, returns
    `None` for file-sourced goals.
  - `resolve_run_goal(base_dir) -> Result<Option<ResolvedRunGoal>>` —
    reads the file from disk if needed, returns text + provenance
    (`ResolvedGoalSource::Inline | File { path }`).
  - New `ResolveGoalError` enum covers env-lookup and I/O failures.
  - Old `run_goal() / run_goal_str()` are **deleted** outright; every
    call site has been updated to pick the right variant.

- **CLI wiring (the actual bug fix).** `overrides::goal_layer_from_args`
  replaces the two `let _ = &args.goal_file;` lines with real
  resolution: `(Some(text), None)` → `Inline`, `(None, Some(path))` →
  `File { file: absolute }`. Both-set is rejected by a helper error
  and clap already had `conflicts_with = "goal"` as a belt-and-
  braces check. Applied to both `RunArgs` and `PreflightArgs`.

- **Manifest builder.** `resolve_manifest_goal` now calls
  `args_layer.as_v2().resolve_run_goal()` and
  `settings.resolve_run_goal()` in precedence order, then falls
  through to the graph-level `@file` sugar if both are absent. The
  resolved goal is translated to a `ManifestGoal { text, type_, path }`
  by a new `resolved_goal_to_manifest` helper — inline goals get
  `type = Value`, file-sourced goals get `type = File` with the
  absolute path echoed for provenance.

- **Workflow pipeline.** `fabro-workflow::operations::source::
  resolve_goal_override` is rewritten to use `resolve_run_goal`
  against the working_directory. The orphaned helper `resolve_goal_file`
  (a stub from Stage 4 that was always called with `None`) is
  deleted.

- **Server-side manifest.** `fabro-server::run_manifest::
  prepare_manifest` stores the CLI-resolved goal as
  `RunGoalLayer::Inline`, matching the Stage 4 plan's "CLI owns goal
  file reads; server never touches the filesystem for goals"
  contract.

## Tests

**Schema** (`fabro-types::settings::accessors`):
- `run_goal_inline_str_returns_source_value` — literal inline variant
- `run_goal_inline_str_is_none_for_file_variant` — file variant
  explicitly yields `None` from the inline accessor
- `resolve_run_goal_reads_file_variant_from_disk` — end-to-end file
  read with provenance assertion
- `resolve_run_goal_inline_passes_text_through` — inline passthrough

**Config load** (`fabro-config::config`):
- `parse_accepts_inline_goal` + `parse_accepts_file_variant`
- `parse_rejects_goal_with_unknown_sibling_fields` — untagged enum
  correctly rejects mixed-shape TOML
- `combine_replaces_file_goal_with_inline_from_higher_layer` and the
  reverse — confirms the tagged union merges as a single scalar with
  no custom rule needed
- `load_rewrites_relative_goal_file_to_absolute`
- `load_leaves_absolute_goal_file_untouched`
- `load_leaves_env_interpolated_goal_file_untouched`

**CLI overrides** (`fabro-cli::commands::run::overrides`):
- `goal_and_goal_file_together_is_rejected`
- `goal_file_is_anchored_at_cwd_when_relative`
- `absolute_goal_file_is_preserved`
- `inline_goal_builds_inline_variant`
- `empty_args_produce_no_goal_layer`

**CLI integration** (`fabro-cli::tests:🇮🇹:cmd::run`):
- `dry_run_with_goal_file_reads_contents_into_goal` — end-to-end
  `fabro run --dry-run --auto-approve --goal-file <path>` and asserts
  the file contents appear in the preflight summary. Explicit
  regression test for the silently-ignored flag.
- `dry_run_rejects_goal_and_goal_file_together` — clap conflicts_with

## Callsite churn

Every `run_goal() / run_goal_str()` call site updated:
- `fabro-config/src/effective_settings.rs` — 2 test assertions →
  `run_goal_inline_str()`
- `fabro-cli/tests/it/cmd/{config,create}.rs` — 3 sites → inline
- `fabro-cli/src/manifest_builder.rs` — rewritten to use
  `resolve_run_goal`
- `fabro-workflow/src/operations/create.rs` — 2 sites, test + set
- `fabro-workflow/src/operations/source.rs` — rewritten
- `fabro-server/src/{run_manifest,server}.rs` — set + test assertion

3,782 workspace tests pass (was 3,765, +17 new). `cargo fmt
--check --all` and `cargo clippy --workspace -- -D warnings` are
clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-09 19:47:12 -04:00
Bryan Helmkamp
f185873bf9 feat(server): preserve comments in setup_register TOML edits
`setup_register` in `web_auth.rs` used to round-trip the user's
settings file through `toml::Value` + `toml::to_string_pretty`, which
strips every comment, blank line, and explicit key ordering on the
way out. A user who'd hand-commented their `~/.fabro/settings.toml`
would see all of that lost on the next GitHub App registration.

Switches the edit path to `toml_edit::DocumentMut`, which preserves
prefix decoration (comments, blank lines) on every key. Adds
`toml_edit = "0.22"` as a workspace dependency (already pulled in
transitively via `toml 0.8`) and declares it in `fabro-server`.

Implementation notes:

- New `ensure_nested_table(doc, &["server", "web"])` walks a dotted
  path and `or_insert`s missing intermediate tables without touching
  existing ones.
- New `set_preserving_decor(table, key, value)` replaces an entry's
  value while copying the old key's `leaf_decor` forward. Without
  that workaround, `toml_edit::Table::insert` drops the prefix
  decoration of the replaced key -- which would strip a top-of-file
  comment attached to `_version = 1` or any other value we update.
- `_version` is only inserted when missing; it's always `1` today, so
  rewriting it every time is unnecessary and would trample its decor.
- `merge_settings_keys` now takes `&mut toml_edit::DocumentMut`
  instead of `&mut toml::Value`. The flow in `setup_register` parses
  the file on disk into a `DocumentMut`, applies the merge, and
  writes `doc.to_string()` back.

Adds a new test
`merge_settings_keys_preserves_comments_and_unrelated_keys` that
round-trips a fixture file containing:

- A top-of-file comment attached to `_version`
- A comment above `[server.storage]`
- A comment above a pre-existing `[server.integrations.slack]` table
- Unrelated keys in `[server.storage]`, `[server.integrations.slack]`,
  and `[run.model]`

and asserts that every comment and every unrelated key survives the
merge, that the new GitHub App keys are present, and that the final
output still parses as a valid v2 `SettingsFile` via
`fabro_config::ConfigLayer::parse`.

Also strengthens the existing
`merge_settings_keys_writes_v2_server_integrations_github` test with
a round-trip parse of the emitted TOML through `ConfigLayer::parse`
to ensure the output is real v2 config, not just a JSON-shaped blob.

3,765 workspace tests pass (+1 new). `cargo fmt --check --all` and
`cargo clippy --workspace -- -D warnings` are clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-09 19:18:10 -04:00