Remove legacy-ts and apply Rust review fixes#18
Merged
Conversation
- Drop legacy-ts/ directory and README references; the Rust crate has been the source of truth since the rewrite. - mcp/server: emit compact JSON in `content[].text`. Pretty-printed JSON roughly doubles whitespace tokens in nested results, defeating the point of relaywash. The `structuredContent` mirror is unchanged. - tools/edit: write atomically (temp file + rename) so a crash mid-write cannot corrupt the target. `std::fs::write` truncates first. - search: cache the relativized path per file (was recomputed per snippet) and drop a redundant sort over BTreeMap keys (already sorted). - tools/search: drop the duplicated `meta_for_tool` helper; clone the Meta the json! macro needs instead of building it twice. - ast/line_regex: cache `strip_inline_comments`'s block-comment regex in a OnceLock — was recompiled on every call.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR performs targeted optimizations on the Rust crate while undertaking a comprehensive cleanup/migration of the legacy TypeScript codebase. The Rust changes focus on performance (regex caching, compact JSON, search optimization) and durability (atomic file writes). The TypeScript changes remove deprecated hook scripts and test suites while introducing a file-backed ledger and measurement infrastructure. ChangesRust Crate Optimizations
Legacy TypeScript Cleanup & Ledger Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5ec72b076
ℹ️ 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".
Comment on lines
+308
to
+312
| let mut f = std::fs::File::create(&tmp)?; | ||
| f.write_all(contents.as_bytes())?; | ||
| f.sync_all().ok(); | ||
| drop(f); | ||
| if let Err(e) = std::fs::rename(&tmp, target) { |
There was a problem hiding this comment.
Creating a brand-new temp file and then rename-ing it over the target replaces the original inode with one that has default permissions, so editing an executable or otherwise non-default-mode file will silently drop its mode bits (for example, 0755 scripts become non-executable). This is a regression from std::fs::write (truncate-in-place), and it can break files immediately after a successful Edit operation unless atomic_write copies the original metadata/permissions onto the temp file before rename.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Good catch — fixed in 6af12ec. atomic_write now reads metadata().permissions() before opening the temp file and applies it via set_permissions before the rename, so the executable bit (and any other mode bits) survives. Added a Unix test that creates a 0755 .sh file, edits it, and asserts the mode is still 0755 afterward.
Generated by Claude Code
![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 `@crates/wash/src/tools/edit.rs`:
- Around line 294-315: The atomic_write function currently discards sync errors
and can change file permissions on replace; modify atomic_write to (1) read the
existing target's metadata (std::fs::metadata) and capture its permissions
before creating the temp file, (2) create the temp file with
OpenOptions::new().write(true).create_new(true) to avoid races, (3) apply the
captured permissions to the temp file via std::fs::set_permissions (or use
set_mode on Unix) before writing, (4) propagate any sync_all() error instead of
calling .ok(), and finally perform the rename and cleanup as before so that
durability failures are returned and original permissions are preserved on
replacement; use the local symbols target, dir, file_name, tmp, and f to locate
the changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b2d07248-de8f-4905-9cd4-369f2757f94f
📒 Files selected for processing (37)
README.mdcrates/wash/src/ast/line_regex.rscrates/wash/src/mcp/server.rscrates/wash/src/search.rscrates/wash/src/tools/edit.rscrates/wash/src/tools/search.rslegacy-ts/build/build.mjslegacy-ts/scripts/builtin-block-hook.jslegacy-ts/scripts/burn-compare.jslegacy-ts/scripts/edit-batching-nudge.jslegacy-ts/scripts/generate-large-fixture.mjslegacy-ts/scripts/relaywash-savings/run.jslegacy-ts/scripts/session-start-hook.jslegacy-ts/scripts/session-stop-ingest-hook.jslegacy-ts/scripts/tool-redirect-hook.jslegacy-ts/servers/relaywash-server.jslegacy-ts/src/ast/index.jslegacy-ts/src/burn/meta.jslegacy-ts/src/burn/sdk.jslegacy-ts/src/fuzzy/index.jslegacy-ts/src/index.jslegacy-ts/src/mcp/server.jslegacy-ts/src/measure.jslegacy-ts/src/tools/build.jslegacy-ts/src/tools/edit.jslegacy-ts/src/tools/gh-pr.jslegacy-ts/src/tools/git-state.jslegacy-ts/src/tools/ping.jslegacy-ts/src/tools/read.jslegacy-ts/src/tools/search.jslegacy-ts/src/tools/test-run.jslegacy-ts/test/build-tool.test.jslegacy-ts/test/burn-sdk.test.jslegacy-ts/test/edit.test.jslegacy-ts/test/git-state.test.jslegacy-ts/test/read.test.jslegacy-ts/test/search.test.js
💤 Files with no reviewable changes (31)
- legacy-ts/src/tools/git-state.js
- legacy-ts/src/ast/index.js
- legacy-ts/src/tools/test-run.js
- legacy-ts/src/burn/meta.js
- legacy-ts/src/tools/ping.js
- legacy-ts/scripts/builtin-block-hook.js
- legacy-ts/build/build.mjs
- legacy-ts/scripts/tool-redirect-hook.js
- legacy-ts/test/git-state.test.js
- legacy-ts/scripts/relaywash-savings/run.js
- legacy-ts/src/mcp/server.js
- legacy-ts/scripts/edit-batching-nudge.js
- legacy-ts/test/search.test.js
- legacy-ts/test/burn-sdk.test.js
- legacy-ts/servers/relaywash-server.js
- legacy-ts/scripts/session-start-hook.js
- legacy-ts/scripts/session-stop-ingest-hook.js
- legacy-ts/test/build-tool.test.js
- legacy-ts/scripts/generate-large-fixture.mjs
- legacy-ts/src/index.js
- legacy-ts/src/tools/edit.js
- legacy-ts/src/measure.js
- legacy-ts/test/edit.test.js
- legacy-ts/src/fuzzy/index.js
- legacy-ts/src/tools/gh-pr.js
- legacy-ts/src/tools/build.js
- legacy-ts/src/tools/read.js
- legacy-ts/src/burn/sdk.js
- legacy-ts/scripts/burn-compare.js
- legacy-ts/src/tools/search.js
- legacy-ts/test/read.test.js
Addresses review feedback on PR #18: - Capture the target file's permissions before opening the temp file and set_permissions() on the temp before write+rename. Without this, rename swaps in our fresh inode with default mode bits and (e.g.) a 0755 script silently becomes non-executable after one Edit. On Windows, MoveFileEx assigns the destination directory's default ACL on rename, so copying perms forward gives equivalent behavior on both platforms. - Use OpenOptions::create_new to avoid the (very narrow, but real) race where another process recreates the temp filename between our format!() and File::create. - Propagate sync_all() errors instead of swallowing with .ok(). A failed fsync should not look like a successful write. - Bracket the write phase so any failure cleans up the temp file before returning. - New Unix-only test asserts that a 0755 file keeps its executable bit through an Edit.
4 tasks
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Drops the retired
legacy-ts/tree (the Rust crate has been the source of truth since the rewrite) and applies a handful of focused fixes from a review of the crate.Cleanup
legacy-ts/(31 files, ~3000 lines) and its README references.Code fixes
mcp/server.rs). Thetextblock — which is what the model actually reads — was being pretty-printed. For nested results, that roughly doubles whitespace tokens, which directly contradicts the project's stated goal. Single biggest token win in this PR. ThestructuredContentmirror is unchanged.tools/edit.rs).std::fs::writetruncates first; a crash mid-write left files corrupt. Now stages to a sibling.tmpand renames atomically (withsync_allbefore rename, cleanup on rename failure).sort_unstableover already-sorted BTreeMap keys.meta_for_toolhelper intools/search.rs;Metais alreadyClone.strip_inline_comments's block-comment regex in aOnceLock— was recompiled on every line of every signature extraction.Follow-ups (filed as separate issues)
The review surfaced larger items that deserve their own PRs — Edit rollback semantics, parallel walking/searching, lazy Read, signature line-map matching, MCP tool-error semantics, profile cache reload, redirect-pattern alignment, GhPR repo resolution, Search size caps, and streaming Ledger summaries.
Test plan
cargo build --releasecleancargo test --release— 83 unit + 2 stdio integration tests passhttps://claude.ai/code/session_01SmRqMmBjQXM4KyDod2Mrfw
Generated by Claude Code