feat: [ENG-744] Add vc diff command

[bao-byterover]

Summary

• Problem: brv vc had no way to view diffs from the CLI or REPL. Users could only see per-file status, not the actual line changes, and had to drop back to system git to inspect edits.
• Why it matters: Closes ENG-744 (Issue 6.3 of the Git Semantics II project) so brv vc reaches parity with the core git diff verbs users already expect. Makes isomorphic-git based workflows self-sufficient without shelling out.
• What changed:
  • Added brv vc diff (oclif) and /vc diff (REPL) supporting four modes: unstaged (no args), --staged, <ref> (ref vs working tree), and <ref1>..<ref2> (range).
  • Extended the vc:diffs transport event with a discriminated mode variant: {kind: 'unstaged' | 'staged'} | {kind: 'ref-vs-worktree', ref} | {kind: 'range', from, to}. Server auto-discovers changed files and returns IVcDiffFile[] with status + short oids; binary files are filtered out.
  • Server-side diff collection in isomorphic-git-service.ts (HEAD / index / worktree / arbitrary refs), and a new diffs(mode) handler branch in vc-handler.ts.
  • Client-side formatter in src/tui/features/vc/diff/utils/format-diff.ts that emits git diff compatible unified output using the diff package's structuredPatch, with chalk coloring (green adds, red deletes, cyan hunk headers, bold file headers).
  • Tightened WebUI types: getVcDiffsQueryOptions now accepts only the {paths, side} variant (WebUI never called the mode shape).
• What did NOT change (scope boundary):
  • WebUI runtime behavior. Only a type-level narrowing of getVcDiffsQueryOptions; the request it sends on the wire is identical.
  • The existing single-file IVcDiffRequest / IVcDiffResponse surface and vc:diff event.
  • Three-dot range syntax (a...b) is explicitly rejected with a user-facing error (two-dot only, matching the ticket's AC).
  • Binary files: still filtered out upstream, not rendered. No --binary flag.
  • No pager, no --word-diff, no --stat, no pathspec filtering.

Type of change

• Bug fix
• New feature
• Refactor (no behavior change)
• Documentation
• Test
• Chore (build, dependencies, CI)

Scope (select all touched areas)

• TUI / REPL
• Agent / Tools
• LLM Providers
• Server / Daemon
• Shared (constants, types, transport events)
• CLI Commands (oclif)
• Hub / Connectors
• Cloud Sync
• CI/CD / Infra

Also touched: WebUI (type-only narrowing in src/webui/features/vc/api/get-vc-diffs.ts, no runtime change).

Root cause (bug fixes only, otherwise write N/A)

N/A (new feature)

Test plan

• Coverage added:
  • Unit test
  • Integration test
  • Manual verification only
• Test file(s):

File	Purpose
test/unit/tui/features/vc/diff/parse-mode.test.ts	Mode parsing: unstaged, --staged, <ref>, <a>..<b>, ... rejection, --staged + ref conflict
test/unit/tui/features/vc/diff/format-diff.test.ts	Unified-diff formatter: headers, hunk ranges (count 0/1/N), added / deleted / modified, binary placeholder, whitespace-in-path escape, hunk context line
test/unit/tui/features/commands/definitions/vc-diff.test.ts	Slash-command wiring (/vc diff)
test/unit/infra/transport/handlers/vc-handler.diffs-mode.test.ts	Handler routing for each VcDiffMode variant + error paths
test/unit/infra/transport/handlers/vc-handler.test.ts	Existing handler tests updated to cover new branch without regressing
test/integration/infra/git/isomorphic-git-service.test.ts	Real fs + isomorphic-git: HEAD/index/worktree diff collection, range diff, binary filter
test/unit/webui/features/vc/api/diff-contract.test.ts	Guards that WebUI only uses the {paths, side} request shape

• Key scenario(s) covered:
  • brv vc diff shows unstaged changes (working tree vs index).
  • brv vc diff --staged shows staged changes (index vs HEAD).
  • brv vc diff <commit> and brv vc diff <branch> show ref vs working tree.
  • brv vc diff <a>..<b> shows diff between two refs.
  • Added / deleted / modified files each produce the right index, new file mode, deleted file mode, --- /dev/null, +++ /dev/null markers.
  • Binary files emit Binary files a/... and b/... differ with no hunks.
  • Paths containing spaces get the trailing tab so unified-diff parsers can locate the path end.
  • Three-dot syntax errors out with a clear message; --staged + positional ref errors out.
  • WebUI contract test ensures only the {paths, side} shape is wired up from the browser.

User-visible changes

New commands:

• brv vc diff (oclif): brv vc diff [ref] [--staged]
• /vc diff (REPL slash command): same args and flag

Output: unified diff matching git diff format, colored with chalk (green +, red -, cyan hunk headers, bold file headers). No output when there are no changes (CLI) / No changes. (REPL).

No other defaults, config keys, or existing CLI output changed.

Evidence

• Unit + integration tests added (see Test plan table). The handler mode dispatch, formatter, and isomorphic-git collection each have dedicated specs; the failing-before / passing-after shape is the set of new specs in this PR (they do not exist on main).
• Manual smoke script added at scripts/e2e-vc-diff.sh (untracked, kept local) exercising the four modes against a temp repo.

Checklist

• Tests added or updated and passing (npm test)
• Lint passes (npm run lint)
• Type check passes (npm run typecheck)
• Build succeeds (npm run build)
• Commits follow Conventional Commits format (feat: [ENG-744] Add vc diff command)
• Documentation updated (if applicable) — N/A, covered by --help output
• No breaking changes (or clearly documented above)
• Branch is up to date with main (one commit ahead of main tip)

Risks and mitigations

• Risk: New runtime dependency diff@^9 (plus @types/diff@^7 devDep) adds a small bundle cost on the daemon/CLI side.
  • Mitigation: diff is widely used, stable, and isolated to format-diff.ts. The formatter is the only import site, so it is easy to swap or remove later.
• Risk: The IVcDiffsRequest discriminated union now has two shapes ({mode} for CLI/TUI, {paths, side} for WebUI). A future caller could pick the wrong one.
  • Mitigation: Union form is exclusive at the type level; WebUI's getVcDiffsQueryOptions is narrowed to only the {paths, side} variant, and diff-contract.test.ts asserts that shape.
• Risk: Formatter output is "close to" but not byte-identical with git diff (hunk-context heuristic, short-oid length, no rename detection).
  • Mitigation: Output is for human viewing, not machine parsing. Tests lock current behavior and document the heuristic (findHunkContext).
• Risk: Large diffs render synchronously and may feel slow in the REPL.
  • Mitigation: Binary files are filtered out before formatting; for extreme cases users can fall back to brv vc diff <ref> on the CLI which streams to stdout. No pager in this PR, flagged as follow-up if feedback requires it.

[github-actions]

Claude finished @bao-byterover's task in 4m 7s —— View job

---

PR Review: feat: [ENG-744] Add vc diff command

• Gather context and read changed files
• Review transport/shared types (vc-events.ts)
• Review server-side implementation (isomorphic-git-service.ts, vc-handler.ts, i-git-service.ts)
• Review TUI/CLI implementation (diff.ts, vc-diff.ts, format-diff.ts, parse-mode.ts, etc.)
• Review tests for correctness and TDD compliance
• Review WebUI type change
• Formulate and post final review

---

Summary

Overall score: 4 / 5

Solid feature addition. The discriminated-union transport design is clean, test coverage is thorough (7 new test files covering all four modes), and the implementation follows established codebase patterns throughout (fired-ref mutation pattern, Outside-In structure, transport event style). The one issue worth fixing before merge is the missing error translation in handleDiffs — a bad ref currently leaks a raw isomorphic-git internal error to the user. Everything else is minor.

---

Issues

suggestion (should fix): Missing INVALID_REF error translation in handleDiffs

src/server/infra/transport/handlers/vc-handler.ts — handleDiffs, lines 763–769

The mode-based path has no try/catch around listChangedFiles. When a user passes a nonexistent branch or commit (e.g. brv vc diff typo-branch), resolveRefExpression inside listChangedBetweenCommits / listChangedFromMatrix throws an isomorphic-git NotFoundError. That raw internal error reaches the client with no friendly message and no VcErrorCode.

Every other ref-accepting handler wraps this with classifyIsomorphicGitError(error, VcErrorCode.INVALID_REF). handleDiffs is the only one that skips this step.

// current (bare throw propagates)
const changed = await this.gitService.listChangedFiles({directory, from, to})

// suggested fix
let changed: ChangedFile[]
try {
  changed = await this.gitService.listChangedFiles({directory, from, to})
} catch (error) {
  const classified = classifyIsomorphicGitError(error, VcErrorCode.INVALID_REF)
  if (classified) throw classified
  throw error
}

The test suite at test/unit/infra/transport/handlers/vc-handler.diffs-mode.test.ts also has no test for this path — a mode: {kind: 'ref-vs-worktree', ref: 'nonexistent'} case rejecting with VcError(INVALID_REF) should be added.

Fix this →

---

nitpick: parseMode silent bad-range edge case

src/tui/features/vc/diff/utils/parse-mode.ts — lines 14–15

Input .. (two dots, no ref on either side) passes all guards and produces {from: '', kind: 'range', to: ''}. The server will then throw a cryptic ref-resolve error. A one-line guard before the regex would surface a user-readable message:

if (rangeMatch && (!rangeMatch[1] || !rangeMatch[2])) {
  throw new Error("invalid range: both <ref1> and <ref2> are required")
}

---

nitpick: Block comment in VcDiffFlow violates CLAUDE.md style

src/tui/features/vc/diff/components/vc-diff-flow.tsx — lines 1–6

CLAUDE.md says "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max" and "Default to writing no comments". The component opens with a four-line block comment. The other vc-*-flow.tsx components in the same directory (e.g., vc-add-flow.tsx) have no file-level comment.

---

Praise

praise: Discriminated union design for IVcDiffsRequest is clean

src/shared/transport/events/vc-events.ts — lines 301–343

The {mode: VcDiffMode} | {paths: string[]; side: VcDiffSide} union makes it a compile-time error to mix the two call shapes. The getVcDiffsQueryOptions narrowing to VcDiffsPathsRequest and the accompanying diff-contract.test.ts lock this down end-to-end. Good design.

---

praise: resolveSingleRef short-SHA fallback

src/server/infra/git/isomorphic-git-service.ts — resolveSingleRef method

Falling back to git.expandOid for 4–39 hex-char inputs is exactly the right fix — git.resolveRef only handles symbolic refs and full OIDs, so short commit hashes would previously have thrown a NotFoundError for any brv vc diff <short-sha> invocation. This is a clean, focused improvement.

---

praise: fired ref pattern matches established convention

src/tui/features/vc/diff/components/vc-diff-flow.tsx — lines 32–48

The fired.current guard on useEffect(fn, []) is the same fire-once pattern used across all other vc-*-flow.tsx components (vc-add-flow.tsx, vc-commit-flow.tsx, etc.). Consistent with the codebase — no ESLint override needed.

---

praise: classifyUnstagedRow correctly excludes untracked files

src/server/infra/git/isomorphic-git-service.ts — classifyUnstagedRow

The if (head === 0 && stage === 0) return undefined guard exactly matches git diff no-args behavior which ignores untracked files. The status-matrix values are well-understood and the three cases cover the reachable combinations given isomorphic-git's matrix semantics.

[github-actions]

Claude finished @bao-byterover's task in 5m 30s —— View job

---

PR Review: feat: [ENG-744] Add vc diff command

• Read changed files and gather context
• Review transport/shared types
• Review server-side implementation
• Review TUI/CLI implementation and tests
• Formulate and post final review

---

Summary

Overall score: 4.5 / 5 (up from 4/5)

The one "should fix" from the previous review was addressed: handleDiffs now wraps listChangedFiles and the Promise.all(buildDiffFile(...)) in a single try/catch that calls classifyIsomorphicGitError(error, VcErrorCode.INVALID_REF), and the matching test (lines 330–343 in vc-handler.diffs-mode.test.ts) covers both the listChangedFiles throw and the readSideEntry throw paths. The multi-line file-level comment in vc-diff-flow.tsx was also removed. The implementation is clean, well-tested, and consistent with established codebase patterns.

One unresolved nitpick from the previous review remains. No new blocking issues found.

---

Previous Review: Issues Resolved

✅ handleDiffs INVALID_REF error translation — Fixed

src/server/infra/transport/handlers/vc-handler.ts, lines 765–776

The try/catch around the entire mode-based path now correctly classifies isomorphic-git NotFoundError for both listChangedFiles (bad ref) and buildDiffFile → readSideEntry → getTextBlob (bad ref when reading a specific file at that ref). Two new tests at lines 330–358 of vc-handler.diffs-mode.test.ts cover both throw sites and the non-isomorphic-git passthrough case.

✅ Block comment in vc-diff-flow.tsx — Removed

The four-line file-level block comment is gone; the file now starts directly with imports, consistent with all other vc-*-flow.tsx siblings.

---

Remaining Issue

nitpick (unresolved from previous review): parseMode silent bad-range edge cases

src/tui/features/vc/diff/utils/parse-mode.ts — line 14

Inputs like .. (two dots, empty both sides), main.. (empty to), or ..feature (empty from) all bypass the includes('...') guard and do not match the regex ^(.+?)\.\.(.+)$ (both capture groups require ≥1 character). They fall through to {kind: 'ref-vs-worktree', ref: '..'} (or 'main..', etc.), producing a cryptic NotFoundError from the server instead of a user-readable message.

// before the rangeMatch return, add:
if (!rangeMatch?.[1] || !rangeMatch?.[2]) {
  // only reachable if the arg contained ".." but the regex didn't capture two non-empty parts
  throw new Error("invalid range: both <ref1> and <ref2> are required (e.g. 'main..feature')")
}

A cleaner alternative is to widen the regex to ^(.*?)\.\.(.*)$ and then check for empty captures explicitly. Either way, .., main.., and ..feature should surface a clear error at parse time.

Fix this →

---

Praise

praise: try/catch scope is exactly right in handleDiffs

src/server/infra/transport/handlers/vc-handler.ts, lines 765–776

Wrapping the whole listChangedFiles + Promise.all(buildDiffFile) block in one try/catch is correct: a NotFoundError can come from either resolveRefExpression inside listChangedBetweenCommits (bad ref at enumeration time) or from getTextBlob inside buildDiffFile → readSideEntry (bad ref when reading a specific file). Both are caught and translated. The readSideEntry WORKDIR path wraps its own readFile and returns undefined on failure, so only the git-side errors bubble up — no double-catch confusion.

praise: classifyUnstagedRow logic is correct for all status-matrix cases

src/server/infra/git/isomorphic-git-service.ts, lines 995–1001

The condition workdir === 2 && (stage === 1 || stage === 3) correctly covers all three unstaged-change cases: [1,2,1] (committed, workdir-modified, not staged), [1,2,3] (partially staged modification), and [0,2,3] (newly added file staged, then further modified in workdir). The key insight is that stage === 2 always means "workdir content == stage content" in isomorphic-git's matrix encoding, so it correctly produces no unstaged diff entry for that case. Integration test coverage at line 358 ([0,2,3]) verifies the trickiest path.

praise: Error classification covers both throw sites in the mode path

test/unit/infra/transport/handlers/vc-handler.diffs-mode.test.ts, lines 330–373

Three tests — listChangedFiles NotFoundError, readSideEntry (getTextBlob) NotFoundError, and a non-isomorphic-git passthrough — give full branch coverage of the try/catch block added in handleDiffs. The non-isomorphic-git errors unchanged case at line 361 is particularly important: it guards against the classified path swallowing unrelated errors.

---

[cuongdo-byterover]

lgtm