Uh oh!
There was an error while loading. Please reload this page.
refactor(wash): consolidate duplicated cross-module contracts into shared seams - #64
Merged
Conversation
The lockfile was gitignored as if it were build output, so the repo pinned nothing for `relayburn-sdk = "2.3"`. A stale local lock at 2.3.0 (async `ingest`) no longer matched the synchronous `ingest(...)?` call in the Stop hook, breaking `cargo build`/`cargo test`. Commit the lockfile (reproducible builds for a binary-distribution crate), pinned at relayburn-sdk 2.10.2 where `ingest` is synchronous, and drop wash's now dead-weight direct `tokio` runtime dependency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Build, TestRun, GitState and GhPR each hand-rolled the same
spawn -> capture -> lossy-decode -> wrap sequence with small accidental
differences. The worst was measurable: Build priced its savings baseline
as stdout+1+stderr while TestRun used stdout+stderr, so baselineBytes --
the product's headline metric -- meant two different things.
Add `process::{Captured, subprocess_baseline, run}` as the single
definition of how wash spawns a child (stdin closed), decodes its output,
and prices vanilla cost, plus `tools::ok_with_meta` as the single place a
result `_meta` is built. Route all four tools through them.
- Drop Build's synthetic `+1` (the "\n" stitched between streams is a
relaywash formatting detail, not vanilla output) -- baseline shifts down
by one byte per call; recorded in the changelog.
- gh now spawns with stdin closed (was inherited); none of the gh read ops
want stdin, and this stops gh from ever consuming MCP protocol bytes.
- Behavior otherwise preserved: identical result JSON, identical Meta
(replaces/collapsedCalls), git/gh non-zero-exit error text unchanged.
169 tests pass (3 new in process.rs).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>GitState and GhPR truncate aggressively -- that's their whole point -- but reported no baselineBytes, so their savings were invisible in the ledger. Thread an explicit `&mut u64` accumulator through `git()`/`gh()` (each adds the raw bytes from `process::run`) and the op helpers; a single op fans out into several subprocess calls, so the baseline is their sum. `run()` now returns `(Value, u64)` and the handler attaches `Some(baseline)`. Honest accounting: small git ops (e.g. `status` on a near-clean tree) can report baseline < responseBytes -- the structured form isn't always smaller. The ledger now sees that truthfully instead of recording nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lection Log-file persistence was split: build.rs owned log_dir()/write_log() and test_run.rs reached across the module boundary (crate::tools::build::...) both to write its log and, in fetch_failure_slice, to read the latest one. Extract `tools::logs` (dir / write / latest) so neither tool reaches into the other. While moving the reader, fix a latent bug: it sorted ALL `.log` files by filename and took the last, but names are `<prefix>-<millis>.log` so `testrun-` always outsorts `build-` regardless of time -- getFailureLog could read a build (or other) log instead of the latest test log. `latest` now takes a prefix and scopes to one log family; getFailureLog asks for "testrun". Also clamp the failure-slice byte window to char boundaries so multibyte log content near the match can't panic the slice. 170 tests pass (logs has its own prefix-isolation test); live MCP confirms TestRun writes a testrun log and getFailureLog finds the failure marker. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `cwd` arg resolution (parse string, else current_dir, else ".") was copy-pasted identically in build, gh_pr, test_run and search, with a String variant in git_state. Hoist it to `tools::cwd_arg` and call it from all five. Pure dedup -- resolution is byte-identical, including the current_dir-fails fallback. 170 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Six tools repeated `get(key).as_u64().map(|n| n as usize).unwrap_or(D)` for their fixed-default integer args. Hoist to `tools::usize_arg`. Search's max_results keeps its own chain -- its default comes from the learned profile (`.or(prof.max_results)`), a different shape. Pure dedup. 170 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
session_start (`ledger_home_default`) and edit_batching_nudge (`nudge_dir_default`) each hand-rolled the RELAYBURN_HOME / HOME/.relayburn fallback, inlining the ".relayburn" literal, while profile::ledger_home is the canonical version (DEFAULT_HOME const) already used by compaction, post_tool_observe and accounting. Delete both copies and call the shared one, so the ledger root has a single definition. Behavior-identical. 170 tests pass; both hooks live-tested to write under the canonical home. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "read JSONL, skip blank + malformed lines" loop was implemented three times: hooks/compaction.rs::read_jsonl, accounting::parse_turns (inline), and an accounting test helper -- three subtly different takes on the same external-format contract (one logged, one was silent, one panicked on a bad line). When Claude Code's transcript shape shifts, that's three places to fix. Add `crate::transcript` with `parse_lines(text, on_bad_line)` (caller picks the malformed-line policy) and `read_file(path)` (logs + skips). Route all three through it: compaction keeps its log-and-continue behavior, accounting keeps its silent skip via a no-op closure. Behavior-preserving aside from one reworded stderr diagnostic. 172 tests pass (2 new in transcript.rs); live pre/post-compact run confirms the shared reader skips a malformed line and still writes the compaction event. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Every hook (and the accounting ingest) hand-rolled
`payload.get("x").or_else(|| payload.get("xCamel"))` to tolerate Claude
Code's two spellings of each field -- 11 copies, each a place to forget the
camelCase fallback when a field is added. Add `hooks::payload_field(payload,
snake, camel)` and route all 11 through it.
Deliberately scoped to the fallback lookup only: each caller keeps its own
tail (defaults "default"/"unknown"/"", the post_tool_observe prefix-strip,
the accounting empty-string filter), so behavior is exactly preserved.
172 tests pass; live MCP confirms builtin-block / tool-redirect /
post-tool-observe all resolve camelCase payload fields through the helper.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>The `{"content":[{"type":"text","text":...}],"isError":true}` envelope for a
tool *execution* failure was hand-rolled in two places: the MCP server's
`tools/call` Err branch and the bench harness's `run_task`, which replays tool
calls outside the server. The bench copy could silently drift from what the
live server emits.
Extract `mcp::error_tool_result(message)` as a sibling to the existing
`format_tool_result` (already the shared success-path wire shape) and call it
from both sites. Behavior is byte-identical.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>`build_event` built the zero-initialized `ToolSurvival` accumulator via the
same all-zeros struct literal in four places. Adding a fifth field later would
mean updating four sites, any of which is easy to miss.
Derive `Default` (all fields are `u64`, so the default is all-zeros — identical
to the literal) and replace the four `.entry(k).or_insert(ToolSurvival { .. })`
calls with `.or_default()`. Behavior is unchanged; the per-tool survival tests
and a live pre/post-compact run confirm identical output.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>The MCP server runs `tools/call` synchronously on its read loop, so a child that never exits — a build waiting on stdin, a `gh` call stuck on an auth prompt, a hung test — stalls the entire session with no way to recover. Add a deadline to the shared `process::run` helper (all four process-backed tools already route through it, so each inherits the timeout): Build/TestRun 900s, git 60s, gh 120s. Build/TestRun return the partial output captured before the kill; git/gh surface a clear "timed out" error. The capture path is rewritten to be hang-proof: `child.kill()` only signals the direct child, so a grandchild that inherited the stdout/stderr pipe (cargo→rustc, sh→backgrounded process, gh helpers) would keep it open and a plain `read_to_end` + `join` would block forever — reintroducing the very stall this fixes. Instead the readers drain into shared buffers and signal completion on a channel; the caller waits only up to a bounded grace period, then returns the output captured so far and lets any orphaned reader exit when the pipe finally closes. Still std-only, no new dependencies. Re-scope of plan 002: the plan predates the spawn-helper dedup (plan 005) and assumed it would create `process.rs`; since that helper already exists, the deadline lives there rather than in a parallel runner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`normalize_for_match` (needle path) and `normalize_with_map` (haystack path) encoded the same whitespace/Unicode normalization contract twice, in two different styles. They MUST agree byte-for-byte or `fuzzy_find_all`'s `normalized.find(&norm_needle)` silently fails to match — and they had already drifted once (the EOF trailing-space trim was hand-patched into the haystack side to re-sync them). Make `normalize_for_match` a thin wrapper over `normalize_with_map(s).0` so there is one source of truth. The needle path discards the back-map but gets identical normalization for free. Verified byte-identical to the old implementation across tabs, newline boundaries, EOF trimming, and the U+00A0/U+200B remap cases; the existing contract tests and a live Edit with a tab/smart-quote/trailing-space-divergent needle confirm the splice still lands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `_meta` wire-field names (`responseBytes`, `baselineBytes`) are defined once via serde renames on `Meta`, but the read side re-spelled them as string literals in three places: the bench harness (two extractors) and the post-tool-observe hook. A `SCHEMA_VERSION` bump that renamed a field would update the writer in one spot while these readers silently returned `None` — degrading savings telemetry with no compile error. Add `Meta::response_bytes_of` / `Meta::baseline_bytes_of` next to the serde renames so the wire names have one home, and route the three readers through them. Each caller still passes its own container, since the nesting genuinely differs (bench reads `structuredContent._meta`, the hook reads root `_meta`) — the shared accessor makes that distinction explicit rather than accidental. Behavior-preserving (verified: identical navigation and `as_u64` semantics). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…olicy The `tools/call` arm of `dispatch` inlined the spec-sensitive error policy — protocol failures (missing name, unknown tool) become a JSON-RPC error, while a tool execution failure becomes a normal result with `isError: true` — but that contract was only reachable through the full method-string match, so the only tests covering it had to spawn the binary and drive stdio. Pull the arm into `McpServer::call_tool(&self, params) -> Result<Value>` and have dispatch delegate. Behavior is identical (verified live across all three paths: success, execution error, protocol error). Adds four in-process unit tests for the error policy that previously required end-to-end stdio tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The run loop treated a frame whose body was not valid UTF-8 as fatal — `String::from_utf8(..).context(..)?` propagated out of `run` and terminated the process — while the JSON-parse failure one line below merely `continue`d. For a long-lived stdio server, a single corrupt frame must not take down every subsequent valid request. Skip the frame instead, matching both the adjacent JSON-parse recovery and the malformed-header recovery in `take_framed_message`. Also delete the `drop_header` closure in `take_framed_message`: it captured nothing and always returned `None`, so the two call sites now `return None` directly. Adds an end-to-end regression test that feeds a non-UTF-8 frame followed by a valid `initialize` and asserts the server still answers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`HitSink::record` decoded match lines with `from_utf8(bytes).unwrap_or("")`, so a
line carrying a stray non-UTF-8 byte (Latin-1 text, or a binary smudge in a file
that cleared the NUL check) collapsed to an empty snippet body — while the match
was still counted. The agent saw a hit at line N with a blank line and no reason
why, silently losing the content the search exists to surface.
Decode with `from_utf8_lossy` so the line renders with U+FFFD replacement chars,
matching ripgrep. Adds a test feeding a Latin-1 byte that asserts the body
survives with the replacement char rather than vanishing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>`leading_ws` was defined byte-for-byte identically in both `ast/mod.rs` and `ast/line_regex.rs` — a verbatim duplicate inside the same module tree, pure drift risk for no benefit. `line_regex` is a child module, so it can use the parent's definition via `super::leading_ws`; the local copy is removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The rule for recognizing a Python `:`-body past a trailing `# comment`
(`s.split('#').next().unwrap_or(s).trim_end()`) was duplicated byte-for-byte in
`find_body_end` and the line-regex extractor. Extract it as
`strip_python_comment` so the detection rule has one definition; both call sites
now route through it. Behavior-preserving (the helper is identical to both
originals; the Python inline-comment tests and a live signatures read confirm).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>read.rs hand-rolled the _meta envelope (a local read_result helper plus an inline ToolResult::new(...).with_meta(Meta::new([Read], 1)...) block) instead of using tools::ok_with_meta — the single funnel mod.rs was built to own so a future Meta change can't silently miss a tool. read_result now delegates to ok_with_meta(.., None); the baseline branch calls it with Some(baseline). Byte-identical output (verified live + adversarial review); the now-unused Meta import is dropped. Search/Edit deliberately keep their own Meta::new — they carry multi-label replaces / computed collapsedCalls that ok_with_meta's single-label, collapsed=1 shape doesn't fit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The 2-prefix tool-name canonicalization (strip mcp__relaywash__, else relaywash__, else keep raw) was duplicated byte-for-byte in accounting's extract_tools and post_tool_observe. Extracted hooks::bare_relaywash_name; both call it. accounting already depends on hooks (payload_field, sanitize_session_id) so the seam has one natural home. Deliberately NOT merged with categorize's canonical(), which additionally strips mcp__github__ — folding that would change categorization output (a github tool would newly canonicalize), a product decision, not a mechanical rename. The helper's doc comment records why. Behavior-identical (verified live across both prefixes + adversarial review). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Warning Review limit reached
More reviews will be available in 29 minutes and 9 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (41)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:baac4f8354
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match child.try_wait() { | ||
| Ok(Some(status)) => break status.code(), | ||
| Ok(None) if Instant::now() >= deadline => { | ||
| let _ = child.kill(); |
There was a problem hiding this comment.
When a timed-out tool command has spawned workers (for example the Build/TestRun paths that run npm, npx, cargo, or go), child.kill() only terminates the direct wrapper process. Any grandchildren keep running with inherited stdout/stderr pipes while run returns after the reader grace period, leaving orphaned builds/tests consuming CPU and potentially racing later tool calls. Start the child in its own process group/job and kill the group on timeout so the timeout actually stops the whole command.
Useful? React with 👍 / 👎.
CI used dtolnay/rust-toolchain@stable (a floating channel) and there was no rust-toolchain.toml, so every machine's rustfmt could format differently — which is how ~30 files drifted with no code change. Pin channel 1.95.0 with rustfmt+clippy so local and CI agree byte-for-byte. Bump deliberately, and reformat in the same commit, so formatting changes stay intentional. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
One-shot reformat under the newly pinned 1.95.0 toolchain so the repo is fmt-clean and the CI gate (next commit) starts from green. Pure layout — rustfmt never changes semantics; build + 182 tests unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a fast fmt job running `cargo fmt --all --check`, and pin both jobs' toolchain to 1.95.0 (matching rust-toolchain.toml) so CI can't format differently from local. Add rust-toolchain.toml to the path filter so a future toolchain bump triggers CI. This is what stops the drift recurring: unformatted code now fails the build. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A multi-round refactor of the
washcrate focused on collapsing duplicated cross-module contracts into single-source-of-truth seams, plus a few correctness and robustness fixes surfaced along the way. Every commit was live-tested against the real MCP server/hooks and independently reviewed; the suite stays green throughout (182 tests).The throughline: no tool hand-rolls a
Command, reaches into a sibling module, or re-implements a cross-module wire shape / parse / lookup.Shared seams introduced
process(spawn/decode/baseline/timeout)Command+ decode + baseline plumbing in Build/TestRun/GitState/GhPRtools::ok_with_meta_metaenvelopes (now incl. Read)tools::{cwd_arg, usize_arg}tools::logsbuild::for log dir/write/latestcrate::transcripthooks::{payload_field, bare_relaywash_name}profile::ledger_home.relayburndefaultsmeta::Meta_metaaccessors_metafield readsmcp::{format_tool_result, error_tool_result, call_tool}fuzzynormalization,ast::{leading_ws, strip_python_comment}Robustness
process::runseam, so the single-threaded MCP server can't hang. Build/TestRun return partial output on timeout; git/gh surface a clear error. The std-only design uses a bounded reader-grace so an orphaned grandchild holding the pipe can never stall the server (review caught this as a live bug — it had been masking the suite at 30s; now back to ~2.3s).Correctness fixes
Cargo.lockcommitted (was ignored as build output) → reproducible builds; pinnedrelayburn-sdkto a version with the syncingestthe Stop hook calls; dropped the deadtokiodep. (This unblocked the build.)Searchrenders non-UTF-8 lines lossily (U+FFFD, ripgrep behavior) instead of dropping the line body.getFailureLogreads the most recent test log instead of whichever log sorted last by filename.GitState/GhPRnow reportbaselineBytes;Buildno longer counts a synthetic newline in its baseline.Testing
cargo build --release+cargo test --releasegreen — 182 tests (matches CI; CI runs build+test).Deliberately not done (flagged for operator)
categorizealso stripsmcp__github__): unifying changes categorization output — a product decision, not a mechanical dedup.🤖 Generated with Claude Code
Formatting: pinned toolchain + CI gate
The crate had accumulated rustfmt drift (no code change, just layout) because CI ran
dtolnay/rust-toolchain@stable(a floating channel) and there was no toolchain pin — so every machine'srustfmtcould format differently. This makes that impossible going forward:rust-toolchain.tomlpins the toolchain to 1.95.0 withrustfmt+clippy. This is the single source of truth — every developer'srustupreads it, so local and CI format identically.cargo fmt --allnormalized the repo (30 files, pure layout —style:commit, skippable in review).fmtjob runscargo fmt --all --checkand fails the build on any unformatted file; both CI jobs are pinned to 1.95.0 to match the toolchain file.rust-toolchain.tomladded to the path filter so a future bump triggers CI.To bump the toolchain later: change the version in
rust-toolchain.tomlandci.ymltogether, in a commit that also runscargo fmt.