feat(review): experiment with mutation-journal reviews - #179
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe PR adds a review change journal for workspace mutations, shared review-diff parsing, pre-write patch callbacks, and server integration. Tests cover net edits, renames, Git-independent reviews, empty files, and unrelated changes. ChangesReview change journaling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WriteTool
participant ReviewChangeJournal
participant Workspace
participant show_changes
WriteTool->>ReviewChangeJournal: prepare mutation
WriteTool->>Workspace: write or edit file
WriteTool->>ReviewChangeJournal: commit successful mutation
show_changes->>ReviewChangeJournal: review tracked mutations
ReviewChangeJournal->>Workspace: read current file states
ReviewChangeJournal-->>show_changes: return net changes and patch
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Waishnav
changed the title
Greptile SummaryThis PR adds an in-memory change journal so successful write, edit, and patch operations can be reviewed independently of Git, while retaining Git checkpoints as a fallback.
Confidence Score: 3/5The PR should not merge until move journaling and failed checkpoint synchronization preserve accurate, monotonic review results. Supported overwrite moves are rendered with the wrong file relationship, and a transient checkpoint failure can cause changes that were already shown to reappear on the next review. Files Needing Attention: src/review-change-journal.ts, src/server.ts
|
| Filename | Overview |
|---|---|
| src/review-change-journal.ts | Implements net-change tracking and diff generation, but its strict move-state validation misrepresents supported overwrite and compound move scenarios. |
| src/server.ts | Integrates mutation journaling into tools and review output, but clears journal state even when fallback-checkpoint synchronization fails. |
| src/apply-patch.ts | Adds a pre-apply callback after staging and before filesystem mutation, enabling accurate baseline capture. |
| src/review-diff.ts | Extracts existing review-patch parsing and summary behavior without a detected contract change. |
| src/review-change-journal.test.ts | Covers basic edits, net-zero changes, simple moves, review advancement, and empty additions, but not overwrite moves or synchronization failures. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
M[Write, edit, or apply_patch] --> P[Capture pre-mutation file states]
P --> C[Commit successful mutation to journal]
C --> S[show_changes]
S -->|Journal has mutations| J[Render journal-owned net diff]
J --> G[Advance Git checkpoint]
G --> R[Clear journal baseline]
S -->|No journal mutations| F[Render Git checkpoint diff]
Reviews (1): Last reviewed commit: "test(review): cover journal patch moves" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ( | ||
| beforeSource.kind !== "file" || | ||
| beforeDestination.kind !== "missing" || | ||
| afterSource.kind !== "missing" || | ||
| afterDestination.kind !== "file" | ||
| ) { | ||
| continue; |
There was a problem hiding this comment.
When apply_patch moves a file onto an existing destination, this check rejects the recorded move because the destination baseline is a file rather than missing, causing show_changes to report an independent deletion and modification instead of the successful rename.
| } catch (error) { | ||
| logEvent(config.logging, "debug", "review_checkpoint_comparison_unavailable", { | ||
| workspaceId, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| reviewJournal.markReviewed({ workspaceId, root: workspace.root }); |
There was a problem hiding this comment.
If Git checkpoint creation or update fails, this catch suppresses the error and markReviewed still clears the journal, causing the next show_changes call to use the stale checkpoint and present already-reviewed changes again.
![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
🧹 Nitpick comments (5)
src/apply-patch.test.ts (1)
66-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a rejecting
beforeApply.The current test pins the happy path. It does not pin the failure contract: if
beforeApplyrejects,applyPatchmust reject and must not write or remove any staged file.src/server.tsdepends on that property, because a failedprepareReviewMutationmust not leave the filesystem mutated with no journal baseline.Add a case that throws from
beforeApplyand then asserts the source file is unchanged and the destination is absent.🤖 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/apply-patch.test.ts` around lines 66 - 92, Extend the applyPatch tests around the existing beforeApply callback to cover a callback that rejects. Assert that applyPatch rejects, the original source file remains unchanged, and the destination file is still absent, confirming no staged file is written or removed when beforeApply fails.src/server.test.ts (1)
128-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a failed mutation.
src/server.tscallsprepareReviewMutationbefore the tool runs andcommitReviewMutationonly after the error branch returns. No test pins that ordering. A regression that movescommitReviewMutationabove theresponse.isErrorcheck would make failed writes appear inshow_changes, and the suite would stay green.Add a case that triggers a failing
editcall, for example with anoldTextthat does not match, then assertsshow_changesreports no changes.🤖 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/server.test.ts` around lines 128 - 184, Add a test alongside the existing show_changes review tests that creates a workspace, invokes the edit tool with an oldText value that cannot match, then calls show_changes and asserts it reports no changes. Use the existing fixture, callOpen, and response helpers so the test verifies failed mutations are not committed to review state.src/apply-patch.ts (1)
23-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe mutation-capture contract mixes absolute and workspace-relative paths without naming either form. The journal accepts absolute paths in
prepareMutation.pathsand workspace-relative paths inReviewMove.applyPatchhands both forms tobeforeApplyin one payload. The wiring insrc/server.tsis correct today only becauseapplyPatchhappens to return relative display paths. Nothing in the types states the rule, so a future caller can pass the wrong form and get a silently unmatched move entry.
src/apply-patch.ts#L23-L28: add a doc comment onApplyPatchOptions.beforeApplythat statespathsare absolute andfiles[].pathandfiles[].previousPathare workspace-relative.src/review-change-journal.test.ts#L59-L65: document the same split onReviewChangeJournal.prepareMutationandReviewMoveinsrc/review-change-journal.ts, which this test relies on when it passes"before.txt"and"after.txt"as move endpoints while passing absolute paths toprepareMutation.As per coding guidelines: "Use glossary terms precisely in schemas, types, documentation, and errors" and "Represent important behavior through schemas, types, checks, or explicit tool results rather than hidden prompt conventions."
🤖 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/apply-patch.ts` around lines 23 - 28, The mutation-capture path contract must explicitly distinguish path forms: document ApplyPatchOptions.beforeApply so paths are absolute while files[].path and files[].previousPath are workspace-relative. Also document the same distinction for ReviewChangeJournal.prepareMutation and ReviewMove in src/review-change-journal.ts, as exercised by src/review-change-journal.test.ts lines 59-65; no test logic change is required.Source: Coding guidelines
src/review-change-journal.test.ts (1)
52-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a move onto an existing file.
reviewChangesrequiresbeforeDestination.kind === "missing"to report a rename. If the destination existed before the move, the rename branch callscontinue, and the fall-through loop reports the source asdeletedand the destination aschange. That degradation is reasonable, but no test pins it. A later change to the rename conditions could alter it silently.Do you want me to generate that test case?
🤖 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/review-change-journal.test.ts` around lines 52 - 81, Add a test covering a move onto an already existing destination, using createReviewChangeJournal and reviewChanges. Assert the current fallback behavior: the original source is reported as deleted and the pre-existing destination is reported as changed, rather than as a rename. Keep the existing move-across-later-edits test unchanged.src/review-change-journal.ts (1)
47-91: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAdd a disposal path for journal workspace state.
statesgains an entry perworkspaceIdand never loses one.markReviewedclearsbaselinesandmoves, but theWorkspaceJournalStateentry stays for the process lifetime.createServercreates one journal and shares it across all MCP sessions, so the map grows for every workspace opened by the server.
baselinesalso retains the full original bytes of every mutated file untilmarkReviewedruns. If a turn writes several large files and never callsshow_changes, those buffers stay resident.Consider adding a
closeWorkspace(workspaceId)method and calling it when the workspace is closed, plus a byte budget above which the journal stores a marker instead of the full content.As per coding guidelines: "Prefer explicit lifecycle and state over hidden autonomy; make tasks, inputs, outputs, failures, and ownership inspectable."
🤖 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/review-change-journal.ts` around lines 47 - 91, Add an explicit closeWorkspace(workspaceId) operation to the object returned by createReviewChangeJournal, removing that workspace’s WorkspaceJournalState from states when its lifecycle ends. Invoke this disposal from the server’s workspace-close path, while preserving markReviewed behavior. Add a configured byte budget for captured originals in prepareMutation/readState handling, storing a non-content marker once the budget is exceeded instead of retaining full file bytes.Source: Coding guidelines
🤖 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/server.ts`:
- Around line 1279-1292: Move commitReviewMutation into a finally block
surrounding the applyPatch call so the captured reviewMutation is committed
whether applyPatch succeeds or throws during partial application. Preserve the
existing move-path mapping from applied.files on success, and ensure the failure
path still commits the captured baselines without attempting to access
unavailable applied results.
---
Nitpick comments:
In `@src/apply-patch.test.ts`:
- Around line 66-92: Extend the applyPatch tests around the existing beforeApply
callback to cover a callback that rejects. Assert that applyPatch rejects, the
original source file remains unchanged, and the destination file is still
absent, confirming no staged file is written or removed when beforeApply fails.
In `@src/apply-patch.ts`:
- Around line 23-28: The mutation-capture path contract must explicitly
distinguish path forms: document ApplyPatchOptions.beforeApply so paths are
absolute while files[].path and files[].previousPath are workspace-relative.
Also document the same distinction for ReviewChangeJournal.prepareMutation and
ReviewMove in src/review-change-journal.ts, as exercised by
src/review-change-journal.test.ts lines 59-65; no test logic change is required.
In `@src/review-change-journal.test.ts`:
- Around line 52-81: Add a test covering a move onto an already existing
destination, using createReviewChangeJournal and reviewChanges. Assert the
current fallback behavior: the original source is reported as deleted and the
pre-existing destination is reported as changed, rather than as a rename. Keep
the existing move-across-later-edits test unchanged.
In `@src/review-change-journal.ts`:
- Around line 47-91: Add an explicit closeWorkspace(workspaceId) operation to
the object returned by createReviewChangeJournal, removing that workspace’s
WorkspaceJournalState from states when its lifecycle ends. Invoke this disposal
from the server’s workspace-close path, while preserving markReviewed behavior.
Add a configured byte budget for captured originals in prepareMutation/readState
handling, storing a non-content marker once the budget is exceeded instead of
retaining full file bytes.
In `@src/server.test.ts`:
- Around line 128-184: Add a test alongside the existing show_changes review
tests that creates a workspace, invokes the edit tool with an oldText value that
cannot match, then calls show_changes and asserts it reports no changes. Use the
existing fixture, callOpen, and response helpers so the test verifies failed
mutations are not committed to review state.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 532ddf30-1e4b-4a91-ae15-97205c4befe1
📒 Files selected for processing (9)
package.jsonsrc/apply-patch.test.tssrc/apply-patch.tssrc/review-change-journal.test.tssrc/review-change-journal.tssrc/review-checkpoints.tssrc/review-diff.tssrc/server.test.tssrc/server.ts
| let reviewMutation: ReviewMutationCapture | undefined; | ||
| const applied = await applyPatch(workspace.root, patch, { | ||
| beforeApply: async ({ paths }) => { | ||
| reviewMutation = await prepareReviewMutation(workspaceId, workspace.root, paths); | ||
| }, | ||
| }); | ||
| commitReviewMutation( | ||
| reviewMutation, | ||
| applied.files.flatMap((file) => | ||
| file.operation === "move" && file.previousPath | ||
| ? [{ fromPath: file.previousPath, toPath: file.path }] | ||
| : [], | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A partially applied patch is dropped from journal-backed reviews.
applyPatch is intentionally partial-apply: the write and remove loops run after beforeApply, and a failure in those loops leaves earlier files already mutated. When that happens, the exception propagates out of line 1280 and commitReviewMutation at line 1285 never runs. The captured baselines in reviewMutation are discarded.
The filesystem is now mutated with no journal baseline for those files. If any other tool call in the same turn already journaled a mutation, hasTrackedMutations returns true, show_changes takes the journal path, and the partially applied files are absent from the review. The Git checkpoint fallback does not run in that case, so the user sees an incomplete diff.
Commit the capture in a finally block so the baselines survive a partial apply.
🐛 Proposed fix
let reviewMutation: ReviewMutationCapture | undefined;
- const applied = await applyPatch(workspace.root, patch, {
- beforeApply: async ({ paths }) => {
- reviewMutation = await prepareReviewMutation(workspaceId, workspace.root, paths);
- },
- });
- commitReviewMutation(
- reviewMutation,
- applied.files.flatMap((file) =>
- file.operation === "move" && file.previousPath
- ? [{ fromPath: file.previousPath, toPath: file.path }]
- : [],
- ),
- );
+ let applied: Awaited<ReturnType<typeof applyPatch>>;
+ try {
+ applied = await applyPatch(workspace.root, patch, {
+ beforeApply: async ({ paths }) => {
+ reviewMutation = await prepareReviewMutation(workspaceId, workspace.root, paths);
+ },
+ });
+ } catch (error) {
+ // apply_patch is partial-apply; keep baselines for files already mutated.
+ commitReviewMutation(reviewMutation);
+ throw error;
+ }
+ commitReviewMutation(
+ reviewMutation,
+ applied.files.flatMap((file) =>
+ file.operation === "move" && file.previousPath
+ ? [{ fromPath: file.previousPath, toPath: file.path }]
+ : [],
+ ),
+ );Committing on failure is safe. reviewChanges compares each baseline against the current filesystem state and skips paths whose state did not change, so baselines for files that were never written produce no review entry.
Based on learnings: "For DevSpace Codex mode, the apply_patch operation is intentionally partial-apply: earlier successful patch operations may remain even if a later patch action fails." This comment does not ask for atomic rollback. It asks that the journal record the files that partial-apply already mutated.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let reviewMutation: ReviewMutationCapture | undefined; | |
| const applied = await applyPatch(workspace.root, patch, { | |
| beforeApply: async ({ paths }) => { | |
| reviewMutation = await prepareReviewMutation(workspaceId, workspace.root, paths); | |
| }, | |
| }); | |
| commitReviewMutation( | |
| reviewMutation, | |
| applied.files.flatMap((file) => | |
| file.operation === "move" && file.previousPath | |
| ? [{ fromPath: file.previousPath, toPath: file.path }] | |
| : [], | |
| ), | |
| ); | |
| let reviewMutation: ReviewMutationCapture | undefined; | |
| let applied: Awaited<ReturnType<typeof applyPatch>>; | |
| try { | |
| applied = await applyPatch(workspace.root, patch, { | |
| beforeApply: async ({ paths }) => { | |
| reviewMutation = await prepareReviewMutation(workspaceId, workspace.root, paths); | |
| }, | |
| }); | |
| } catch (error) { | |
| // apply_patch is partial-apply; keep baselines for files already mutated. | |
| commitReviewMutation(reviewMutation); | |
| throw error; | |
| } | |
| commitReviewMutation( | |
| reviewMutation, | |
| applied.files.flatMap((file) => | |
| file.operation === "move" && file.previousPath | |
| ? [{ fromPath: file.previousPath, toPath: file.path }] | |
| : [], | |
| ), | |
| ); |
🤖 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/server.ts` around lines 1279 - 1292, Move commitReviewMutation into a
finally block surrounding the applyPatch call so the captured reviewMutation is
committed whether applyPatch succeeds or throws during partial application.
Preserve the existing move-path mapping from applied.files on success, and
ensure the failure path still commits the captured baselines without attempting
to access unavailable applied results.
Source: Learnings
1 participant
Stacks on #178. This experiments with making successful DevSpace file-mutation tools define the reviewed file set instead of relying solely on Git working-tree provenance.
The journal captures each path's original state on first touch by
write,edit, orapply_patch, then computes one net original-to-final diff atshow_changes. Repeated edits collapse naturally, net-zero changes disappear, moves retain provenance, unrelated working-tree edits are excluded, and non-Git workspaces can produce review cards. Git checkpoints still advance in parallel as a synchronized fallback/comparison path, so this PR can evaluate journal semantics without removing the existing recovery mechanism yet.No OS-level filesystem watcher or new user-facing review-mode switch is introduced.
Summary by CodeRabbit
New Features
Bug Fixes