chore: shrink release binary + dedupe JSONC read helpers - #188
Conversation
Drops shipped agentflare.exe from 17.94 MiB to 12.35 MiB (-31%). panic=\abort\ strips unwind tables across the whole dependency graph (no catch_unwind anywhere in the workspace, so no functional risk). zip now only pulls the deflate codec it actually uses (src/update.rs reads plain zip archives for self-update; bzip2/lzma/zstd/zopfli/aes were unused default features).
Extract skip_string() out of strip_json_comments/strip_trailing_commas (same byte-level string-skip loop was copy-pasted in both) and add jsonc::read_jsonc(path, default) to replace the read -> parse_jsonc -> fallback boilerplate repeated across json_at, merge_json, merge_opencode_mcp (components.rs) and wire_opencode (init.rs).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change centralizes JSONC file loading and fallback handling across configuration paths, reuses string scanning logic, and restricts the ChangesJSONC loading consolidation
Dependency configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsonc.rs`:
- Around line 17-24: Preserve missing-file defaults without allowing failed
reads or parses to be rewritten: update src/jsonc.rs lines 17-24 and read_jsonc
to distinguish NotFound from existing-file I/O or parse errors, then make
merge_json at src/components.rs line 102, merge_opencode_mcp at
src/components.rs line 122, and wire_opencode at src/init.rs line 511 abort
without writing when those existing-file failures occur; missing files should
retain the current default behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2068d7bd-aecd-49b2-8e29-5290e96e251c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlsrc/components.rssrc/init.rssrc/jsonc.rs
| /// Reads `path`, parses it as JSONC, and falls back to `default()` on any | ||
| /// failure (missing file, unreadable, or invalid JSON/JSONC) — the shared | ||
| /// read-then-fallback contract every agent-config call site needs. | ||
| pub fn read_jsonc(path: &std::path::Path, default: impl FnOnce() -> Value) -> Value { | ||
| std::fs::read_to_string(path) | ||
| .ok() | ||
| .and_then(|s| parse_jsonc(&s).ok()) | ||
| .unwrap_or_else(default) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve the distinction between missing and invalid configuration files.
The shared fallback contract is safe for read-only access but unsafe for callers that rewrite configuration: malformed or unreadable existing files can be replaced with a partial default object.
src/jsonc.rs#L17-L24: return a typed result or distinguishNotFoundfrom parse/I/O errors.src/components.rs#L102-L102: makemerge_jsonabort without writing on existing-file read/parse failures.src/components.rs#L122-L122: apply the same protection inmerge_opencode_mcp.src/init.rs#L511-L511: preventwire_opencodefrom rewritingopencode.jsoncafter a failed read or parse.
📍 Affects 3 files
src/jsonc.rs#L17-L24(this comment)src/components.rs#L102-L102src/components.rs#L122-L122src/init.rs#L511-L511
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jsonc.rs` around lines 17 - 24, Preserve missing-file defaults without
allowing failed reads or parses to be rewritten: update src/jsonc.rs lines 17-24
and read_jsonc to distinguish NotFound from existing-file I/O or parse errors,
then make merge_json at src/components.rs line 102, merge_opencode_mcp at
src/components.rs line 122, and wire_opencode at src/init.rs line 511 abort
without writing when those existing-file failures occur; missing files should
retain the current default behavior.
|
Reviewed. Binary-size numbers check out and the zip feature trim looks safe. One concern on
"Zero Not saying this is necessarily wrong to ship — an MCP server crashing a session on a genuine bug might be an acceptable/even preferable failure mode vs. silently swallowing panics. But it's a real behavior change (single-request failure → whole-session crash) that's worth an explicit call, not just the "no catch_unwind" check. Rest of the diff (zip feature trim, JSONC dedup) looks good. |
rmcp spawns every MCP tool call as its own tokio task; panic=abort removes that per-task isolation, turning a single panicking tool call into a whole-session process abort instead of just a failed request. The zip default-features trim is still safe and kept (binary drops ~250KiB from that alone; the earlier 31%/5.6MiB figure was mostly panic=abort, which this reverts).
|
Pushed a fix for the panic=abort concern above: dropped `panic = "abort"` from `[profile.release]`, kept the zip feature trim. rmcp spawns every MCP tool call as its own tokio task and relies on unwind-based task isolation to survive a panicking handler — abort defeats that process-wide, which is a real regression for a long-running server binary, not just a theoretical one. There's no way to scope panic strategy to only the CLI-invocation code paths within a single `[[bin]]` target, so the safer call is to not take it here. Re-measured: zip trim alone drops the binary ~250KiB (18,808,320 → 18,556,928 bytes) — the bulk of the originally-claimed 31%/5.6MiB was panic=abort, which is now reverted. Verified: `cargo build --release` clean, `cargo test --workspace` 453 passed, `cargo fmt --check` clean, binary sanity-run (`--version`/`--help`) OK. Merging once CI is green on the new commit. |
…ff-prompt # Conflicts: # Cargo.lock
…awl/entire) (#600) * ## Status **What I did:** Reviewed and finished work already in progress in this worktree for item #188 (session checkpoint linkage + `git explain`/`rewind`): - `crates/flare-git-core/src/provenance.rs` — added an `Agentflare-Session` commit trailer (resolved from `AGENTFLARE_SESSION_ID` or `CLAUDE_CODE_SESSION_ID`) plus `parse_trailers()` to read trailers back off an existing commit message. - `src/cli/git.rs` — new `agentflare git explain [<commit>]` (prints agent/branch/item/session provenance plus the originating prompt, read from the local Claude Code transcript at `~/.claude/projects/*/<session_id>.jsonl`) and `agentflare git rewind list|restore` (browse commits with provenance annotations; non-destructive working-tree restore with a pre-restore snapshot and a `--yes` confirmation gate). Includes a path-traversal guard (`is_safe_session_id`) since session ids come from self-reported, unattested trailer text. - `src/paths.rs` / `src/cost.rs` — factored the existing ad-hoc `claude_projects_dir()` in `cost.rs` into a shared `paths::claude_projects_dir()`, reused by the new `explain` transcript lookup. - `.githooks/prepare-commit-msg` + `docs-site/src/content/docs/cli.md` — updated doc comments/CLI docs for the new trailer and subcommands. - Cleaned up one leftover double-blank-line in `cost.rs` left by that refactor. **Tests run:** - `cargo build` — clean (only pre-existing unrelated `unsafe-code` warnings). - `cargo test -p flare-git-core provenance` — 9/9 pass. - `cargo test cli::git` — 25/25 pass. - Confirmed `cargo clippy -p flare-git-core -- -D warnings` failures are pre-existing in the unrelated `agent-registry` crate (verified by stashing this diff and re-running clippy on clean `HEAD` — same 9 errors). - Manual smoke test against this repo's real history: `agentflare git explain HEAD` and `agentflare git rewind list --limit 5` both produce correct output (including graceful "(unknown)"/"(none)" fallback for pre-existing commits without trailers). **Concerns:** None blocking. Two things worth knowing: 1. Session attribution only works when a commit is made directly by a live Claude Code session (env-inherited `CLAUDE_CODE_SESSION_ID`) — headless dispatch commits won't carry it unless the child process passes that env through, which `provenance.rs`'s doc comment already calls out explicitly rather than silently mis-attributing. 2. I did not commit — leaving the worktree as-is per this session's git policy (commits only on explicit request). Agentflare-Branch: task/188-session-checkpoint-linkage-rewind-explai Agentflare-Item: 188-session-checkpoint-linkage-rewind-explai * fix: cargo fmt in src/cli/git.rs to unblock CI on PR #600 Agentflare-Agent: claude-code Agentflare-Branch: task/188-session-checkpoint-linkage-rewind-explai Agentflare-Item: 188 --------- Co-authored-by: shiva <shiva@gosysinfo.tech>
…tch (#601) execute_work chdir'd into an item's worktree via std::env::set_current_dir, which mutates the whole process's cwd, not per-thread. The daemon dispatches multiple in-process work-item jobs concurrently by design (work_max_concurrency), so two jobs racing here could have one item's pipeline run against a different item's checked-out worktree -- observed live, twice, with two different item pairs (items #189/#70 and #189/#188). Add run_in_worktree (src/cli/work_cwd_lock.rs), which serializes the chdir -> run -> restore critical section behind a mutex, same mitigation shape as flare_git_core::worktree::WORKTREE_ADD_LOCK. Only this section is serialized -- claiming, agent resolution, and DB reads above it still run concurrently. Split into a satellite file (and the new regression test into another) to keep work.rs under the repo's LOC gate. Regression test dispatches two items concurrently on separate threads and asserts neither thread's cwd drifts into the other's worktree mid-run; confirmed it reliably reproduces the race (3/3 runs) with the lock removed and passes reliably (3/3) with it restored, both before and after factoring the lock+chdir logic into run_in_worktree. Agentflare-Agent: claude-code Agentflare-Branch: fix/execute-work-cwd-race Co-authored-by: shiva <shiva@gosysinfo.tech>
1 participant
Summary
Test plan
Summary by CodeRabbit