fix(vcs): collapse review diff modes to git + branch + turn#1044
Merged
Conversation
The review panel exposed four diff modes (unstaged / staged / branch /
turn) backed by three independent slice helpers per ref. Each slice was
internally consistent but never showed the user the full picture:
- unstaged hid staged work, staged hid unstaged work
- branch ran `git diff <mergeBase> HEAD`, silently dropping every
uncommitted (staged + working-tree) change
Collapse to upstream's two-mode shape plus PawWork's turn snapshot:
- `git` = working tree vs HEAD (covers staged + unstaged + untracked)
- `branch` = working tree vs merge-base (the full accumulated branch
delta, now including uncommitted work)
- `turn` = session snapshot, unchanged
Mechanical drop of the per-mode Git helpers (diffUnstaged / diffStaged /
diffHead / statsUnstaged / statsStaged / statsHead / statusUnstaged /
patchStaged / patchUnstaged / patchHead / patchStagedAll) in favour of
the existing generic `diff(cwd, ref)`, `stats(cwd, ref)`, and
`patch(cwd, ref, file)`. `Vcs.diff` routes both modes through a single
`track()` helper; `Vcs.diffRaw` keeps PawWork's no-HEAD branch via
`patchUntracked` on every status entry.
Front-end follows: `ReviewChangeMode` becomes `git | branch | turn`,
the cache store loses the unstaged/staged slots, i18n keys land at
`ui.sessionReview.title.git` and `session.review.noGitChanges`, and the
SDK regenerates the new mode union.
Verification:
- bun run typecheck (packages/opencode, packages/sdk/js, packages/app)
- bun test (packages/opencode: 3416 pass / 9 skip / 1 todo; packages/app:
1628 pass)
- bun run lint
- /tmp scratch repo confirmed `git diff main` reports the staged + worktree
delta the old `git diff main HEAD` was hiding
Contributor
|
Warning Review limit reached
More reviews will be available in 52 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR consolidates VCS diff modes from ChangesGit VCS Mode Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
app
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/i18n/en.ts, packages/app/src/i18n/parity.test.ts, packages/app/src/i18n/zh.ts, packages/app/src/pages/session/execution-scope.test.ts, packages/app/src/pages/session/review-change-mode.test.ts, packages/app/src/pages/session/review-change-mode.ts, packages/app/src/pages/session/review-panel-view.tsx, packages/app/src/pages/session/use-session-review-state.test.ts, packages/app/src/pages/session/use-session-review-state.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the version control system (VCS) diff logic by consolidating the separate 'unstaged' and 'staged' modes into a single 'git' mode across translation files, frontend components, API routes, and the generated SDK. It also refactors the underlying Git utilities to use a unified tracking helper. A high-severity issue was identified in Vcs.diff where Git operations are run against Instance.directory instead of checking for a separate worktree via Instance.worktree ?? Instance.directory, which could lead to incorrect or missing diffs when a worktree is active.
The previous commit changed the server-side `Vcs.Mode` schema to `["git", "branch"]` but only regenerated the TypeScript SDK in packages/sdk/js/src/v2/gen/. The checked-in OpenAPI source under packages/sdk/openapi.json still declared the old `unstaged | staged | branch` enum, so any client generated from that file would send requests the server now rejects with a query validation error. Patch the enum and the operation description in place. A full regen of this file pulls in unrelated drift from schema-tool behaviour changes (stray pattern fields, leading shell echo); keeping the diff surgical isolates the contract change to the VCS endpoint. Verification: - git diff --stat packages/sdk/openapi.json shows +2/-3 (mode enum + description only) - Found by codex review on PR #1044
`git diff <ref>` only walks worktree vs ref, so when a file is staged and
the worktree is then restored to match the ref (e.g. edit → `git add` →
overwrite back to HEAD), porcelain status shows `MM` but the diff returns
nothing. The previous track() merged only `git.diff(...)` + untracked
status entries, so the new git mode silently dropped this state — the
file was about to be committed and the review panel was empty.
Carry `--cached` through the existing helpers instead of adding parallel
ones:
- Git.DiffOptions { cached } and PatchOptions extending it
- diff / stats / patch / patchAll thread `--cached` when requested
- track() folds three sources into the file list: tracked (worktree-vs-ref),
staged-only entries detected via porcelain status (index-vs-ref where
the worktree happens to match), and untracked entries
- stat map blends head-vs-ref over cached-vs-ref so worktree edits keep
their additions/deletions while staged-only files inherit the cached
numbers
- per-file patch falls back to `git.patch(..., { cached: true })` when
the worktree-vs-ref patch is empty but the index still differs
Same helper surface, one extra `cached` boolean. branch mode benefits
from the same fix when a feature-branch file is staged then reverted to
the merge base.
Verification:
- bun run typecheck (opencode, sdk/js, app)
- bun test (opencode: 3417 pass / 9 skip / 1 todo; targeted git+vcs+routes: 54 pass)
- bun run lint
- New regression test in test/project/vcs.test.ts pins the stage→restore
case to the new surfaced-with-cached behaviour
- Raised by codex review on PR #1044
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/opencode/src/project/vcs.ts`:
- Around line 343-359: diffRaw currently builds tracked patches using
git.patchAll(..., "HEAD") which misses index-only (staged-only) changes; update
diffRaw to prefer the provided stagedAgainstRef when present (use
git.patchAll(worktree, stagedAgainstRef, ... ) instead of "HEAD") and if that
still misses staged-only deltas call the cached/index patch fallback (via
rawPatch calling git.patchCached or git.patchIndex for the same ref) and merge
that into the tracked variable before appending untracked extras; modify the
tracked assignment (and keep rawPatch, git.patchAll, git.patchUntracked,
stagedAgainstRef, and diffRaw identifiers) so staged-only changes are included.
- Around line 325-334: The git-path resolution in the git branch of the
conditional uses Instance.directory directly, which can point at the base
checkout instead of an attached worktree; update calls to use the resolved
worktree path (Instance.worktree ?? Instance.directory) whenever calling
git.hasHead, git.mergeBase, and track so status/diff/apply semantics match other
methods (change the ref variable computation and the three calls:
git.hasHead(Instance.directory) -> git.hasHead(worktree),
git.mergeBase(Instance.directory, ...) -> git.mergeBase(worktree, ...), and
track(git, Instance.directory, ref) -> track(git, worktree, ref)).
🪄 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: 8a1b9a3a-b790-4174-8efa-346ae46835fe
⛔ Files ignored due to path filters (2)
packages/sdk/js/src/v2/gen/sdk.gen.tsis excluded by!**/gen/**packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (15)
packages/app/src/i18n/en.tspackages/app/src/i18n/parity.test.tspackages/app/src/i18n/zh.tspackages/app/src/pages/session/execution-scope.test.tspackages/app/src/pages/session/review-change-mode.test.tspackages/app/src/pages/session/review-change-mode.tspackages/app/src/pages/session/review-panel-view.tsxpackages/app/src/pages/session/use-session-review-state.test.tspackages/app/src/pages/session/use-session-review-state.tspackages/opencode/src/git/index.tspackages/opencode/src/project/vcs.tspackages/opencode/src/server/instance/index.tspackages/opencode/test/git/git.test.tspackages/opencode/test/project/vcs.test.tspackages/sdk/openapi.json
Vcs.status, Vcs.diffRaw, and Vcs.apply all resolve the working directory via Instance.worktree ?? Instance.directory; only Vcs.diff still pointed at Instance.directory, so the review panel showed the wrong (or empty) diff whenever a session ran inside a separate worktree.
In a pre-first-commit repo a path can be staged-added and then deleted from the worktree (status "AD"), which the collapsed diffRaw lost: tracked was empty because hasHead was false, and patchUntracked could not read the already-deleted file. Restore the pre-collapse three-section layout for the no-HEAD path — staged-vs-empty + worktree-vs-index + untracked — and add patchAllUnstaged() so the worktree-vs-index slice has a dedicated git helper. The result is review-oriented unified text, not git-apply-clean for mixed index/worktree states; the route description and an inline comment make that contract explicit.
Astro-Han
added a commit
that referenced
this pull request
Jun 2, 2026
dev #1044 collapsed Vcs.Mode to git|branch; the route test still queried mode=unstaged, which the validator now rejects with 400 after rebase. Use mode=git (working-tree vs HEAD), the direct equivalent of the old unstaged.
Astro-Han
added a commit
that referenced
this pull request
Jun 2, 2026
Migrate the JSON route handlers in the instance root router (packages/opencode/src/server/instance/index.ts) to the Effect runtime path via `AppRuntime.runPromise` + `Effect.gen`: POST /instance/dispose, GET /path, GET /vcs, GET /vcs/diff, GET /vcs/status, GET /command, GET /agent, GET /skill, GET /lsp, GET /formatter. Part of #936 Hono-to-Effect route migration; no route, schema, or response-shape change. WebSocket and non-JSON surfaces in this file are untouched. Change boundary: - packages/opencode/src/server/instance/index.ts: handler bodies only. Each handler resolves the relevant Service (Vcs/Command/Agent/Skill/LSP/Format) or runs existing logic inside `AppRuntime.runPromise`. Route registrations, validators, the `Vcs.Mode` (git|branch) query schema, OpenAPI descriptions, and JSON responses are unchanged. - packages/opencode/test/server/instance-root-routes.test.ts: new tests for /path + metadata routes (/agent /skill /command /lsp /formatter), VCS routes (/vcs, /vcs/diff, /vcs/status), and /instance/dispose through the real Server runtime. Verification (isolated worktree, rebased onto origin/dev and pushed so PR CI runs the integrated state): - bun install --frozen-lockfile - bun test test/server/instance-root-routes.test.ts -> 3 pass - bun run typecheck (opencode) -> clean - codex review --base origin/dev -> no actionable issues - full PR CI green (ci + windows-advisory) Review follow-up: - Two Gemini "high" comments warned about AsyncLocalStorage context loss when reading Instance.current / Instance.worktree / Instance.directory inside AppRuntime effects after async yields. Verified empirically as a non-issue here: attach() (run-service.ts) captures the instance into the Effect-side InstanceRef synchronously at runPromise time, and Node ALS is also preserved across Effect.promise yields under our runtime (probed: Instance.directory resolves correctly before AND after a yield, 3/3 runs). Instance.dispose() additionally reads ctx synchronously at its first statement. On Effect.promise vs Effect.tryPromise: a rejection becomes an Effect defect that still rejects runPromise and is surfaced by Hono as 500, the same observable behavior as the prior await. Both threads resolved with rationale; no code change. - Rebase integration fix: dev #1044 collapsed Vcs.Mode to git|branch; the new /vcs/diff test queried mode=unstaged, now rejected with 400. Updated the test to mode=git (working-tree vs HEAD, the direct equivalent of the old unstaged). Test-only change. Linked: #936 Residual risk: low. Read routes preserved; ALS safety verified empirically; VCS mode contract follows current dev (#1044).
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
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
git/branch/turn), aligned with upstream OpenCode's two-mode shape plus PawWork'sturnsnapshot.branchmode:git.diffHeadrangit diff <mergeBase> HEAD, which silently dropped every staged and working-tree change.branchmode now usesgit diff <mergeBase>(working tree vs ref), so the full branch delta reaches the user.diffUnstaged/diffStaged/diffHead/statsUnstaged/statsStaged/statsHead/statusUnstaged/patchStaged/patchUnstaged/patchHead/patchStagedAll) in favour of the existing genericdiff(cwd, ref)/stats(cwd, ref)/patch(cwd, ref, file).Why
The split into unstaged/staged/branch surfaced Git's internal index/HEAD concepts as the product's main path. Each tab was internally consistent in its own narrow Git semantics but never gave the user a complete answer:
unstagedhid staged workstagedhid working-tree workbranch(the most-clicked) hid every uncommitted change because of the spuriousHEADendrefWith the new shape, every tab answers a coherent product question:
git= "what have I changed since the last commit",branch= "what does this branch accumulate against main",turn= "what did the agent change this round".Changes
packages/opencode/src/git/index.ts: drop the per-mode helpers, keep only the genericdiff/stats/patch/patchAll/patchUntracked/statUntracked.packages/opencode/src/project/vcs.ts:Mode = ["git", "branch"]. Both routes go through onetrack(git, cwd, ref?)helper that mergesgit.diff(cwd, ref)+ status??entries (and falls back to status-only when there is noHEADyet).diffRawsimplifies in the same direction.packages/opencode/src/server/instance/index.ts:/vcs/diffdescription updated to match the new modes.packages/sdk/js: regenerated;VcsDiffData["query"]["mode"]is now"git" | "branch".packages/app/src/pages/session/review-change-mode.ts+use-session-review-state.ts:ReviewChangeMode = "git" | "branch" | "turn", cache store shape collapses to two VCS slots, options array becomes["git", "branch", "turn"].packages/app/src/pages/session/review-panel-view.tsx: empty-state branches collapse togit/branch.packages/app/src/i18n/{zh,en}.ts+parity.test.ts: drop*.title.unstaged/*.title.staged/noUnstagedChanges/noStagedChanges, add*.title.git/noGitChanges.git.test.ts,vcs.test.ts,review-change-mode.test.ts,use-session-review-state.test.ts,execution-scope.test.tsupdated. New regression coverage invcs.test.tsassertsdiff('branch')includes staged + unstaged + untracked work on top of branch commits — the case the olddiffHeadform silently dropped.Verification
bun run typecheck(packages/opencode, packages/sdk/js, packages/app)bun test(packages/opencode: 3416 pass / 9 skip / 1 todo; packages/app: 1628 pass)bun run lint/tmpconfirmed thatgit diff <mergeBase>reports the staged add and the working-tree edit the oldgit diff <mergeBase> HEADform was hiding.Test plan
Related
Touches the same code path that #1019 references (Status panel branch stats reuse).
Summary by CodeRabbit
Release Notes