[kylehgc]

Stacked on #3203 (submit/bom-config-reads) — only the last commit (fix(json,deps): tolerate UTF-8 BOM at the three remaining JSON parse sites) is new; the first two commits are #3203 itself and will drop out of the diff once it merges. Happy to rebase onto develop after #3203 lands.

Follow-up covering the three parse sites @KuSh flagged in the #3203 review as needing the same BOM tolerance, which were scoped out of that PR:

• src/cmds/system/deps.rs — summarize_package_json_str: parsed package.json with a raw serde_json::from_str(&content)? (also missing .context()). A BOM'd package.json — written by Notepad or PowerShell 5.1 Out-File -Encoding utf8 — made rtk deps hard-fail with a bare expected value at line 1 column 1 that didn't name the file. Now strips the BOM via strip_leading_bom and carries the file path in both read and parse errors.
• src/cmds/system/json_cmd.rs — filter_json_compact and filter_json_string: rtk json <file> (compact and --keys-only) errored on BOM-prefixed files — the single most likely command to be pointed at human-saved JSON. Both entry points (file and stdin) route through these functions, so both are covered.

Repro

# PowerShell 5.1: Out-File -Encoding utf8 writes a BOM
'{"name": "demo", "version": "1.0.0"}' | Out-File -Encoding utf8 package.json
rtk json package.json   # before: "Failed to parse JSON: expected value at line 1 column 1" (exit 1) — after: prints the JSON
rtk deps .              # before: "expected value at line 1 column 1" (exit 1) — after: summarizes normally

Verified before/after on all three paths, including --keys-only.

Tests

Per site: BOM-prefixed input parses and produces output identical to the plain input; a plain-input test guards existing behavior; the invalid-package.json error now contains the path.

cargo fmt --all && cargo clippy --all-targets && cargo test --all — clean.

🤖 Generated with Claude Code

[kylehgc]

Updated: merged the current #3203 branch (which now carries the from_json_str wrapper from its review round) and converted this PR's three sites from the raw serde_json::from_str(strip_leading_bom(..)) combination to the wrapper, so the whole series states the policy one way. Stacked on #3203 — the first two commits here are its; the diff net of that base is the three-site fix plus the wrapper conversion. Gate: fmt clean, clippy zero warnings, 2502 tests passing.

[KuSh]

Reviewed the full cumulative diff (5 commits, 10 files). Verified on a clean worktree at beee9df: cargo fmt --all -- --check clean, cargo clippy --all-targets zero warnings, cargo test --all 2568 passed / 0 failed.

The core change is sound. from_json_str keeps the zero-copy <'a, T: Deserialize<'a>> signature, and the strip_leading_bom -> trim().is_empty() ordering is correct at every site that needed it: U+FEFF is Cf, not whitespace, so the empty-file guard has to run after stripping, and the new BOM-only / BOM-plus-invalid-JSON tests pin exactly that. Panic safety on the write paths is intact, and the remaining serde_json::from_str call sites in the tree are command-output parsers (aws, pnpm, vitest, ruff, ...) or RTK-written state (trust.rs), correctly left alone.

Three things to address before merge, see the inline comments. The hook_cmd.rs one matters most, given the PR is scoped as "the remaining parse sites".

[kylehgc]

All three addressed as of b6f354c.

Half-installed state: patch_gemini_settings now treats an unparseable settings file the way PatchMode::Skip does — warns with the file path and parse error, prints the same manual-setup instructions (factored into a shared helper), and lets the rest of the install report success. The no-overwrite guarantee is unchanged: the parse-failure path returns before any mutation, and the regression test now asserts Ok + byte-identical file + the warn path.

Hook stdin: run_gemini and run_claude both strip the BOM now, matching the copilot/cursor/droid paths — all five entry points are uniform. run_gemini's parse logic moved into a run_gemini_inner that production and the new BOM test share, so the test exercises the real path (a dropped strip breaks the actual hook, not a mirrored copy).

never_worse fallback: the BOM is stripped once at the read, in a shared render_json used by both the file and stdin paths, before the compare — so the raw fallback for an already-compact file emits BOM-free output and rtk json foo.json | jq . works on exactly the case you named. Pinned by a test forcing the fallback path with a minified BOM'd input.

Gate: fmt clean, clippy zero warnings, 2516 tests passing.

[KuSh]

Thanks for the quick turnaround — I re-reviewed b6f354c and all three earlier findings are properly fixed; I've resolved those threads. The run_gemini_inner extraction in particular is the right shape: routing production run_gemini through it (rather than mirroring the parse in a test) means a dropped BOM strip fails for real.

Re-ran the gate on the head: cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test --all all clean on a default environment.

One blocker and three non-blocking notes, all on the new commit:

• Blocking — test_gemini_strips_utf8_bom is non-hermetic. It reads the developer's real ~/.gemini/settings.json; I reproduced a red test on a machine that uses Gemini CLI. Details inline.
• Non-blocking: the lost serde diagnostic in run_gemini, the "installed" summary printed after a failed settings patch, and an avoidable full copy of the input in render_json. Plus one nitpick on the tracking baseline.

Happy to see the rest land as-is if you'd rather keep the PR tight — only the first one needs to change before merge.

[kylehgc]

All addressed as of ed8b0eb — the blocker plus the three notes; one nit deliberately declined, reasoning below.

The hermetic test (blocker): went with your second option — rules injected through a shared run_gemini_inner_impl, the same pattern as gemini_render/decide_with_rules, with production run_gemini_inner and the test-only run_gemini_inner_with_rules both routing through the one parse/strip/render implementation. So the test is deterministic under any HOME while still exercising the identical production BOM path — dropping the strip inside the shared impl fails it. Reproduced your exact scenario (a HOME with run_shell_command(git status) allow-listed) and ran the full suite under it: green, including this test.

Serde diagnostic: run_gemini_inner returns serde_json::Result<String>, so the line/column detail is chained under the context message again; pinned by a test asserting the diagnostic survives.

Honest summary: patch_gemini_settings signals the parse-failure outcome distinctly, and the summary now prints settings.json: NOT patched (existing file could not be parsed; see warning above) instead of the unconditional installed line. Kept exit 0 — the point of the graceful-degrade was that the rest of the install genuinely succeeded — but stdout no longer claims more than happened. PatchMode::Skip is untouched.

Fallback copy: render_json returns Cow<'_, str>; the raw-fallback branch borrows, only the genuinely-filtered branch allocates, and the test asserts Cow::Borrowed on the fallback path.

The timer.track baseline — acknowledged and deliberately left as-is: the baseline is meant to be the bytes the raw command would have emitted, and cat on a BOM'd file genuinely emits those bytes; counting them keeps the metric consistent with how every other filter accounts. Your framing ("arguably correct as-is") matches our read, so we kept the current semantics rather than special-casing 3 bytes.

Gate: fmt clean, clippy zero warnings, 2517 tests passing — under both a clean HOME and your dirty-HOME reproduction.

[KuSh]

Approving. Three rounds in, everything raised is addressed and the BOM coverage is now genuinely complete: all six hook stdin entry points strip, and every non-test serde_json::from_str over a human- or tool-written file routes through from_json_str. The sites deliberately left raw — the trust.rs trust store, analytics JSONL, nested toolArgs strings, fixtures — are RTK/machine-written and correctly untouched.

Worth restating what the patch_gemini_settings change actually buys: the old unwrap_or(json!({})) would silently overwrite an unparseable ~/.gemini/settings.json with a fresh {} plus the hook, destroying user config. That's a real data-loss fix, not just BOM tolerance.

Two things I liked in the review responses: routing production run_gemini through run_gemini_inner_impl rather than mirroring the parse in a test, so a dropped strip fails for real; and returning Ok(true) from patch_gemini_settings only on the parse-failure path, so the summary can distinguish it from Skip, already-patched, declined-at-prompt and dry-run.

Disclosure — the last three commits are mine, pushed via maintainer edit rather than sent back for another round:

• 9d78af2 — the three round-3 findings: std::ptr::eq on the &str fat pointers instead of as_ptr() (an address-only compare also matches a sub-slice, so a future truncating never_worse would silently re-expand the output); run_droid and its test entry point now share a droid_payload helper, closing a gap where droid's BOM strip had zero coverage; and .with_context(...) on the four remaining bare reads in deps.rs.
• 3865a84 — merge of develop. The conflict was in hook_cmd.rs only: develop added the Vibe hook exactly where this branch refactored run_gemini's tail. Kept both sides, dropped the now-unused print_allow/print_gemini.
• ac3389a — run_vibe landed on develop mid-review as the sixth hook stdin entry point and the only one not stripping, so merging develop in would have quietly broken the invariant this PR exists to establish. One line plus a test.

Both new tests are mutation-verified: deleting the strip fails test_droid_strips_utf8_bom / test_vibe_strips_utf8_bom and nothing else. Gate on ac3389a: cargo fmt --all -- --check clean, cargo clippy --all-targets -- -D warnings clean, cargo test --all 2735 passed / 0 failed.

Please sanity-check those three commits — happy to drop or rework any of them if you'd rather they were separate.

Two follow-ups filed from this review, both pre-existing and out of scope here: #3767 (one malformed manifest aborts the whole rtk deps scan instead of degrading) and #3768 (test_resolved_command_executes_known_command asserts on rustup's exit status and fails whenever HOME is redirected).

[kylehgc]

Sanity-checked all three commits — keep them all, and thanks for pushing directly rather than spending another round.

• 9d78af2: the fat-pointer compare is strictly better than the as_ptr() version we wrote — comparing the whole slice means a future sub-slice-returning never_worse can't masquerade as the raw input. And droid_payload closes exactly the mirrored-copy test trap we'd just fixed for gemini; fair catch that we didn't sweep the other agents for the same pattern after that lesson.
• 3865a84: verified the merge kept everything intact on our side — run_gemini_inner, the "NOT patched" summary path, and all strip sites present post-merge.
• ac3389a: right call — landing the merge without it would have broken the six-entry-point invariant this PR exists to establish.

Independently re-ran the gate on ac3389a here: fmt clean, clippy zero warnings, full suite 0 failed. Nothing to drop or rework — and thanks for the thorough rounds on this one; the review made it a much better change than the one we started with.