feat: review the change journal instead of git snapshots - #174
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 SummaryThe PR adds a persistent SQLite-backed change journal and uses it to implement show_changes for non-git workspaces.
Confidence Score: 2/5The PR should not merge until binary deletions are preserved and journal reads enforce canonical workspace containment. The new review path can permanently omit recorded binary deletions and can read symlink targets outside the workspace into a user-visible patch; the multibyte size calculation also bypasses the intended diff cap. Files Needing Attention: src/change-journal.ts and src/server.ts
|
| Filename | Overview |
|---|---|
| src/change-journal.ts | Implements journal capture and review, but drops binary deletions, permits symlink-following reads outside the root, and inconsistently enforces the byte cap. |
| src/server.ts | Integrates journal recording and review into file tools; recording before mutation makes journal path safety particularly important. |
| src/db/migrations.ts | Adds the version-5 change_journal table with workspace-session cascade cleanup. |
| src/db/schema.ts | Defines the Drizzle schema and inferred types corresponding to the new migration. |
| src/change-journal.test.ts | Covers common journal behavior and binary modification, but not binary deletion or symlink targets. |
Sequence Diagram
sequenceDiagram
participant Tool as write/edit/apply_patch
participant Journal as Change journal
participant DB as SQLite
participant FS as Workspace filesystem
participant Review as show_changes
Tool->>Journal: recordTouch(path)
Journal->>FS: Read original content
Journal->>DB: Persist first baseline
Tool->>FS: Apply mutation
Review->>Journal: "reviewChanges(markReviewed=true)"
Journal->>DB: Load baselines
Journal->>FS: Read current content
Journal-->>Review: Summary, files, unified patch
Journal->>DB: Re-baseline or remove rows
Reviews (1): Last reviewed commit: "test: cover the change journal and non-g..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const { row, currentContent, currentBinary } = entry; | ||
|
|
||
| if (!row.previousPath) { | ||
| if (currentContent === null && row.originalContent === null) continue; |
There was a problem hiding this comment.
When an existing binary file is deleted, its stored originalContent and current content are both null, so this branch skips the entry and re-baselining permanently removes it, causing show_changes to report no deletion.
| if (currentContent === null && row.originalContent === null) continue; | |
| if (currentContent === null && row.originalContent === null && !row.originalBinary) continue; |
| function assertJournalPath(root: string, path: string): void { | ||
| if (!isPathInsideRoot(join(root, path), root)) { | ||
| throw new Error(`Path is outside workspace root: ${path}`); |
There was a problem hiding this comment.
When a workspace-relative path is a symlink to an external text file, the lexical containment check accepts it and readFile follows the link, allowing show_changes to expose the external file's contents in its patch. How this was verified: The path passes the resolve-based containment check and is then read with symlink-following readFile before the generated patch is returned.
| const originalBytes = row.originalBinary | ||
| ? 0 | ||
| : Buffer.byteLength(row.originalContent ?? ""); | ||
| const oversized = originalBytes + (currentContent?.length ?? 0) > MAX_JOURNAL_DIFF_BYTES; |
There was a problem hiding this comment.
The size calculation measures the original with Buffer.byteLength but the current text in UTF-16 code units, so multibyte UTF-8 content can bypass the intended diff cap and produce an unnecessarily large patch.
| const oversized = originalBytes + (currentContent?.length ?? 0) > MAX_JOURNAL_DIFF_BYTES; | |
| const oversized = originalBytes + Buffer.byteLength(currentContent ?? "") > MAX_JOURNAL_DIFF_BYTES; |
1760bb2 to
e9b3308
Compare
The git-backed review snapshots cannot see edits made through write, edit, and apply_patch in workspaces that are not git repositories. Add a change_journal table keyed by workspace session and path that records the original content captured at first touch, so reviews can diff current file state against what existed before this work started.
The journal manager captures original file content on first touch and produces a net diff at review time. New files, deletions, and moves are classified from journal facts instead of diff heuristics, and re-review re-baselines the journal so each review shows only changes since the last one. Binary and oversized changes degrade to a file list instead of a text patch. unifiedFilePatch is exported from apply-patch so the journal reuses the same patch builder the patch tools use.
show_changes now diffs the change journal by default, so review works in non-git workspaces and no longer pays per-call git snapshot costs. The git-backed review stays available with DEVSPACE_REVIEW_MODE=git. The write, edit, and apply_patch handlers record journal touches before executing, gated on change-review widgets being enabled.
Manager tests exercise net diffs, revert-to-original, create-then-delete, deletions, moves and re-baselining, binary and oversized degradation, first-touch semantics, restart persistence, and path containment. The server test drives show_changes end to end in a non-git workspace through write and re-baseline.
A touched binary that was deleted never appeared in a review: its stored original is null, so the revert-to-original check treated the deletion as no change. Deleted binaries now surface as deletions once and are dropped by re-baselining, and the revert check no longer applies to binaries whose original content was never stored. Journal reads now resolve symlinks and enforce containment on the resolved path, so a symlink pointing outside the workspace cannot leak foreign file content into a review patch. Symlinks that stay inside the workspace keep working. The diff size cap now compares bytes on both sides instead of UTF-16 code units for the current content, matching the documented 512 KiB limit.
e9b3308 to
b8c2d40
Compare
1 participant
Makes
show_changesreview the change journal instead of git snapshots.What changed
change_journaltable (migration v5) records, per workspace session and path, the original content captured at first touch by the write, edit, and apply_patch tools, plus move provenance.change-journal.tsmanager computes a net diff per path at review time: new files, edits, deletions, and moves are classified from journal facts (not diff heuristics), binary and oversized changes degrade to a file list (512 KB cap), and re-review re-baselines the journal so each review shows only changes since the last one.show_changesdiffs the journal by default. This works in non-git workspaces, needs no repository scanning, and survives restarts (SQLite).DEVSPACE_REVIEW_MODE=git.Why
The git snapshot path failed in non-git workspaces, tied every review to a repo, and recomputed whole-snapshot diffs per call. The journal makes review a cheap, workspace-scoped read of what the model actually touched.
The middle PR of three; sits on #173 and is a dependency of #175.