cycle/0006: graft diff — structural git diff#2
Conversation
Cycle 0006: git helpers (7 tests) — changed file list, file content at ref, added/deleted files. graft diff operation (9 tests) — modified/added/deleted files, multi-file, working tree vs HEAD, changed signatures, path filter, non-supported extensions.
Cycle 0006: diff any two git refs structurally. - src/git/diff.ts: getChangedFiles, getFileAtRef wrappers - src/operations/graft-diff.ts: orchestrates git + parser + diffOutlines for modified/added/deleted files across commits or working tree - graft_diff registered as MCP tool (#10) with base/head/path params - Supports .ts/.js/.tsx/.jsx; other extensions listed with empty diff - 225 tests across 20 files, all passing
Cycle 0006 closed. Structural git diff works across commits and working tree. Suppressed noisy git stderr on added/deleted files. 225 tests, 10 MCP tools.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughAdds a graft-diff feature that computes symbol-level structural diffs between two git points (refs or working tree). Introduces git helper utilities, a graftDiff orchestration, MCP tool registration, tests, and supporting design/retro/backlog docs. Unsupported extensions yield status-only entries with empty structural diffs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP_Server
participant GraftOp as graftDiff
participant Git as GitCLI
participant FS as FileSystem
participant Extract as extractOutline
participant Diff as diffOutlines
Client->>MCP_Server: call graft_diff(base?, head?, path?)
MCP_Server->>GraftOp: invoke graftDiff({cwd, base, head, path})
GraftOp->>Git: git diff --name-only (base..head or base vs worktree)
Git-->>GraftOp: list of changed paths
loop per changed file
GraftOp->>Git: git show <base>:<path>
Git-->>GraftOp: baseContent or null
alt head is ref
GraftOp->>Git: git show <head>:<path>
Git-->>GraftOp: headContent or null
else head is worktree
GraftOp->>FS: readFileSync(path)
FS-->>GraftOp: headContent or error
end
GraftOp->>Extract: extractOutline(content, lang)
Extract-->>GraftOp: outline
GraftOp->>Diff: diffOutlines(baseOutline, headOutline)
Diff-->>GraftOp: OutlineDiff
end
GraftOp-->>MCP_Server: GraftDiffResult
MCP_Server-->>Client: result (with receipt)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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: 1bed38ef88
ℹ️ 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".
| : `diff --name-only ${base}`; | ||
|
|
||
| try { | ||
| const output = execSync(`git ${args}`, { |
There was a problem hiding this comment.
getChangedFiles interpolates base/head directly into execSync("git ..."), and these values come from graft_diff tool input as arbitrary strings. Because execSync string commands run in a shell, a crafted ref like HEAD; <command> can execute arbitrary OS commands with the server’s privileges. Switch to an argument-vector API (for example execFileSync) or strictly validate refs before invocation.
Useful? React with 👍 / 👎.
| cwd: string, | ||
| ): string | null { | ||
| try { | ||
| return execSync(`git show ${ref}:${filePath}`, { |
There was a problem hiding this comment.
getFileAtRef builds git show ${ref}:${filePath} as a raw shell string, so legitimate filenames containing spaces or shell-special characters are split/misparsed by the shell. In those cases git show fails and the catch path returns null, which makes graftDiff misclassify a modified file as added/deleted and return an incorrect structural diff. Invoke git with explicit argv entries so filePath is passed as one argument.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Fixed in 86dc650. execFileSync passes args as array — no shell quoting needed.
| } catch { | ||
| return []; |
There was a problem hiding this comment.
The broad catch in getChangedFiles converts every git failure (invalid refs, not-a-repo, permission errors) into [], so callers receive a successful-looking empty diff instead of an error. This silently corrupts results for typoed refs (for example HEAD~999) and makes debugging hard because the failure is suppressed. Propagate a structured error or return an error state instead of treating all failures as “no changes.”
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Fixed in f727133. getChangedFiles now throws GitError for invalid refs instead of returning empty array.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/unit/mcp/tools.test.ts (1)
5-17:⚠️ Potential issue | 🟡 MinorTest asserts length 10 but only verifies 8 tool names explicitly.
You've bumped the count to 10 but the test lacks
expect(toolNames).toContain("graft_diff")andexpect(toolNames).toContain("changed_since"). This means you could register a tool named"oops_typo"instead, the length check would pass, and you'd never notice.Add explicit assertions for all registered tools.
🔧 Proposed fix
expect(toolNames).toContain("doctor"); expect(toolNames).toContain("stats"); + expect(toolNames).toContain("changed_since"); + expect(toolNames).toContain("graft_diff"); expect(toolNames).toHaveLength(10);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/mcp/tools.test.ts` around lines 5 - 17, The test creates a server via createGraftServer(), collects registered tools into toolNames, and asserts total length 10 but only checks 8 specific names; add explicit expects for the two missing tools by asserting expect(toolNames).toContain("graft_diff") and expect(toolNames).toContain("changed_since") so the test verifies all registered tool names rather than relying solely on the length check.test/integration/mcp/server.test.ts (1)
26-38:⚠️ Potential issue | 🟡 MinorTest description says "lists all 8 tools" but asserts
toHaveLength(10).The description is stale and contradicts the assertion. Additionally, same issue as the unit test: you verify only 8 tool names explicitly but expect 10 total.
🔧 Proposed fix
- it("lists all 8 tools", async () => { + it("lists all 10 tools", async () => { const tools = await client.listTools(); const names = tools.tools.map((t) => t.name); expect(names).toContain("safe_read"); expect(names).toContain("file_outline"); expect(names).toContain("read_range"); expect(names).toContain("run_capture"); expect(names).toContain("state_save"); expect(names).toContain("state_load"); expect(names).toContain("doctor"); expect(names).toContain("stats"); + expect(names).toContain("changed_since"); + expect(names).toContain("graft_diff"); expect(names).toHaveLength(10); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/mcp/server.test.ts` around lines 26 - 38, The test "lists all 8 tools" is inconsistent: the description says 8, the code asserts 10 and only checks 8 specific names. Update the test in the it block that calls client.listTools() so the description, the explicit expects (names checks), and the length assertion all match the real number of tools; either change the description to "lists all 10 tools" and add expectations for the two missing tool names (or remove/change the length assertion to 8), or adjust the length to 8 to match the listed names—modify the it("lists all 8 tools") block, the names array assertions, and the expect(names).toHaveLength(...) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/0006-graft-diff/design.md`:
- Around line 41-43: Add a language identifier to the two fenced code blocks:
the one showing the function signature "graft_diff(base?, head?, path?)" and the
file tree block showing "src/ git/ diff.ts ... operations/ graft-diff.ts". Edit
the markdown to replace the opening triple backticks with ```text for both
blocks (the signature block around "graft_diff(...)" and the code block spanning
the file tree shown around lines ~101-107) so markdownlint MD040 warnings are
resolved.
In `@src/git/diff.ts`:
- Around line 14-30: The getChangedFiles function builds a shell string using
opts.base and opts.head which allows command injection; change it to call
execFileSync (or execFile) with an argument array instead of interpolating into
a single command string: construct a args array like ["diff","--name-only",
base, head?] using the values from ChangedFilesOptions and pass that array to
execFileSync with cwd and encoding, handle empty output and errors the same way;
ensure you no longer call execSync with a backticked command string and remove
any direct string interpolation of user-provided base/head.
- Around line 27-29: The catch that unconditionally returns [] in
src/git/diff.ts silently hides real failures; replace it with an explicit
error-handling path: change the bare catch to catch (err) { /*...*/ } and either
log the diagnostic and rethrow the error or return a discriminated result type
such as DiffResult = { ok: true; files: string[] } | { ok: false; error: Error }
so callers can distinguish "no changes" from an error; update the function in
this module (the try/catch that currently returns []) to emit the error
(logger.error) and/or return the discriminated error variant instead of
returning [].
- Around line 36-50: The getFileAtRef function is vulnerable to command
injection because it interpolates ref and filePath into a shell string passed to
execSync; replace the shell invocation with a non-shell execFileSync call by
invoking git as the executable and passing ['show', `${ref}:${filePath}`] as
args (keep cwd, encoding, stdio options), and ensure you import/use execFileSync
instead of execSync in getFileAtRef to eliminate shell parsing of user input.
In `@src/operations/graft-diff.ts`:
- Around line 83-91: The current status determination treats baseContent ===
null && headContent === null as "modified"; add a defensive check before the
existing if/else that detects if both baseContent and headContent are null and
handle it explicitly (e.g., throw a descriptive Error or skip the file) so we
don't mis-classify files — reference the variables status, baseContent,
headContent and the call site getFileAtRef/getChangedFiles in graft-diff.ts when
crafting the error message or skip logic.
- Around line 61-64: The loop over changedFiles redundantly checks file
extensions after calling detectLang; remove the SUPPORTED_EXTENSIONS.has(ext)
check and rely on detectLang(filePath) returning null for unsupported files
(i.e., keep the detectLang result check and the ext variable only if needed
elsewhere), so update the for (const filePath of changedFiles) block to skip
files when detectLang returns null and eliminate the
SUPPORTED_EXTENSIONS.has(ext) branch (referencing detectLang, changedFiles,
SUPPORTED_EXTENSIONS, and ext to locate the code).
In `@test/unit/operations/graft-diff.test.ts`:
- Around line 8-10: The duplicate git helper function (git(cwd: string, cmd:
string)) is defined in multiple test files; extract it into a single shared test
helper module (e.g., create a new test/helpers/git.ts exporting function git)
and replace the inline definitions in the test suites with imports from that
helper. Update the tests that use git (search for the git function in test
files) to import the shared helper and remove their local duplicate definitions
to avoid repetition.
---
Outside diff comments:
In `@test/integration/mcp/server.test.ts`:
- Around line 26-38: The test "lists all 8 tools" is inconsistent: the
description says 8, the code asserts 10 and only checks 8 specific names. Update
the test in the it block that calls client.listTools() so the description, the
explicit expects (names checks), and the length assertion all match the real
number of tools; either change the description to "lists all 10 tools" and add
expectations for the two missing tool names (or remove/change the length
assertion to 8), or adjust the length to 8 to match the listed names—modify the
it("lists all 8 tools") block, the names array assertions, and the
expect(names).toHaveLength(...) accordingly.
In `@test/unit/mcp/tools.test.ts`:
- Around line 5-17: The test creates a server via createGraftServer(), collects
registered tools into toolNames, and asserts total length 10 but only checks 8
specific names; add explicit expects for the two missing tools by asserting
expect(toolNames).toContain("graft_diff") and
expect(toolNames).toContain("changed_since") so the test verifies all registered
tool names rather than relying solely on the length check.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: d1368c2f-40a3-400f-a16c-df70f41171f0
📒 Files selected for processing (10)
docs/design/0006-graft-diff/design.mddocs/method/backlog/asap/CORE_graft-diff.mddocs/method/retro/0006-graft-diff/retro.mdsrc/git/diff.tssrc/mcp/server.tssrc/operations/graft-diff.tstest/integration/mcp/server.test.tstest/unit/git/diff.test.tstest/unit/mcp/tools.test.tstest/unit/operations/graft-diff.test.ts
💤 Files with no reviewable changes (1)
- docs/method/backlog/asap/CORE_graft-diff.md
| function git(cwd: string, cmd: string): string { | ||
| return execSync(`git ${cmd}`, { cwd, encoding: "utf-8" }).trim(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicate git helper function across test files.
This exact helper appears in both test/unit/git/diff.test.ts and here. For test code this is tolerable, but if you add more git-related test suites, consider extracting to test/helpers/git.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/operations/graft-diff.test.ts` around lines 8 - 10, The duplicate
git helper function (git(cwd: string, cmd: string)) is defined in multiple test
files; extract it into a single shared test helper module (e.g., create a new
test/helpers/git.ts exporting function git) and replace the inline definitions
in the test suites with imports from that helper. Update the tests that use git
(search for the git function in test files) to import the shared helper and
remove their local duplicate definitions to avoid repetition.
There was a problem hiding this comment.
Acknowledged. Tracked in backlog: docs/method/backlog/bad-code/CORE_duplicate-test-fixtures.md with done criteria added in f727133.
There was a problem hiding this comment.
@flyingrobots, understood — the backlog item and done criteria are a clean way to track this. The duplicate createTestRepo fixture setup is already well-documented for extraction into test/helpers/git.ts when the time comes. Carrying on! 🫡
🧠 Learnings used
Learnt from: CR
Repo: flyingrobots/graft PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-03T13:51:07.896Z
Learning: Applies to **/*.test.{js,ts,tsx} : Use vitest for test execution
- Replaced execSync with execFileSync in git wrappers to prevent shell injection via ref/path arguments - Removed redundant SUPPORTED_EXTENSIONS set (detectLang already gates language support) - Fixed CHANGELOG test count (209 → 225)
Bad code (new): - server.ts god file (~420 lines, needs extraction) - Language detection missing in MCP server diff/changed_since paths - Duplicate git test fixture setup - VISION.md stale (cycle/test/tool counts wrong) Cool ideas (new): - Diff summary line per file (structural --stat) - MCP tool descriptions for agent UX - Receipt compression ratio field Promoted to ASAP: - run_capture stub (must resolve before 0.1.0)
- STYLE.md: how the standard applies to graft - CLEAN_CODE legend: structural quality migration domain - Logged debt: shape soup (plain objects → domain classes), hexagonal violations (Node APIs in core) - Existing code predates the standard; new code follows it, migration is incremental
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/operations/graft-diff.ts (1)
72-87:⚠️ Potential issue | 🟠 MajorI/O failures are being silently converted into diff statuses.
Line 72-Line 76 treats every read failure as
null, and Line 85-Line 87 then classifies both-null as"modified". That masks filesystem errors and can emit incorrect statuses.Suggested fix
} else { // Working tree const fullPath = path.join(cwd, filePath); try { headContent = fs.readFileSync(fullPath, "utf-8"); - } catch { - headContent = null; + } catch (error) { + const err = error as NodeJS.ErrnoException; + if (err.code === "ENOENT") { + headContent = null; + } else { + throw new Error(`WORKTREE_READ_FAILED: ${String(error)}`); + } } } // Determine status let status: "modified" | "added" | "deleted"; if (baseContent === null && headContent !== null) { status = "added"; } else if (baseContent !== null && headContent === null) { status = "deleted"; + } else if (baseContent === null && headContent === null) { + throw new Error("GRAFT_DIFF_BOTH_SIDES_MISSING"); } else { status = "modified"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/operations/graft-diff.ts` around lines 72 - 87, The code currently treats any fs.readFileSync failure as null and later marks both-null as "modified", masking I/O errors; update the read logic around headContent and baseContent in graft-diff.ts so the catch inspects the thrown error (e.g., err.code) and only treat ENOENT (file not found) as null, but rethrow or surface any other errors instead of swallowing them; then compute status ("added" | "deleted" | "modified") using those explicit nulls—this prevents non-ENOENT I/O failures from being misclassified and avoids the incorrect both-null => "modified" case.src/git/diff.ts (1)
27-29:⚠️ Potential issue | 🟠 MajorReturning
[]on failure hides real git errors.Line 27-Line 29 turns invalid refs/repo failures into “no changed files”, so callers cannot distinguish failure from a true empty diff.
Suggested fix
-export function getChangedFiles(opts: ChangedFilesOptions): string[] { +export function getChangedFiles(opts: ChangedFilesOptions): string[] { const base = opts.base ?? "HEAD"; const args = opts.head !== undefined ? ["diff", "--name-only", base, opts.head] : ["diff", "--name-only", base]; try { const output = execFileSync("git", args, { cwd: opts.cwd, encoding: "utf-8", }).trim(); if (output === "") return []; return output.split("\n"); - } catch { - return []; + } catch (error) { + throw new Error(`GIT_DIFF_FAILED: ${String(error)}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/git/diff.ts` around lines 27 - 29, The catch block in src/git/diff.ts that currently swallows errors and returns [] should stop hiding real git failures; replace the empty catch (which returns []) with an error-propagating implementation (e.g., catch (err) { throw new Error(`git diff failed: ${String(err)}`); } or rethrow the original err) so callers can distinguish genuine empty diffs from git/ref errors — update the catch surrounding the git-diff invocation in this file to include the error object and rethrow or wrap it with contextual text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/method/backlog/bad-code/CORE_duplicate-test-fixtures.md`:
- Around line 7-10: The backlog item needs explicit completion criteria: add a
shared createTestRepo() factory in test/helpers/git.ts with a clear API (e.g.,
async createTestRepo(options) returning {repoPath, cleanup, initCommit}), update
both test suites (test/unit/git/ and test/unit/operations/) to import and use
that helper, remove duplicated fixture code, and assert no behavior drift by
running the full test suite and ensuring all tests pass; also add a short README
or comment describing the helper API and include a small migration checklist
(helper API shape, both suites migrated, no behavior drift, CI green) to the
backlog entry so ownership and completion are unambiguous.
In `@src/git/diff.ts`:
- Around line 41-49: The try/catch in getFileAtRef currently returns null for
both "file missing at ref" and any git execution error, collapsing two distinct
cases; update getFileAtRef to expose a tri-state result (e.g., return a
discriminated object or throw on execution errors) rather than string | null so
callers can distinguish "file absent at ref" from "git failed". Specifically,
modify getFileAtRef to: 1) detect the git exit/STDERR that indicates a missing
path at the ref and return an explicit missing marker (e.g., { found: false }),
2) propagate or return an error variant for real git failures (or rethrow) so
callers like the code in graft-diff.ts can infer added/deleted/modified
correctly, and 3) update callers to handle the new discriminated shape (or catch
thrown errors) instead of treating null as both cases.
---
Duplicate comments:
In `@src/git/diff.ts`:
- Around line 27-29: The catch block in src/git/diff.ts that currently swallows
errors and returns [] should stop hiding real git failures; replace the empty
catch (which returns []) with an error-propagating implementation (e.g., catch
(err) { throw new Error(`git diff failed: ${String(err)}`); } or rethrow the
original err) so callers can distinguish genuine empty diffs from git/ref errors
— update the catch surrounding the git-diff invocation in this file to include
the error object and rethrow or wrap it with contextual text.
In `@src/operations/graft-diff.ts`:
- Around line 72-87: The code currently treats any fs.readFileSync failure as
null and later marks both-null as "modified", masking I/O errors; update the
read logic around headContent and baseContent in graft-diff.ts so the catch
inspects the thrown error (e.g., err.code) and only treat ENOENT (file not
found) as null, but rethrow or surface any other errors instead of swallowing
them; then compute status ("added" | "deleted" | "modified") using those
explicit nulls—this prevents non-ENOENT I/O failures from being misclassified
and avoids the incorrect both-null => "modified" case.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 6971d4fe-71ef-4bf3-b0c4-e0dc759a8c82
📒 Files selected for processing (11)
CHANGELOG.mddocs/method/backlog/asap/CORE_run-capture-stub.mddocs/method/backlog/bad-code/CORE_duplicate-test-fixtures.mddocs/method/backlog/bad-code/CORE_lang-detection-missing-in-server.mddocs/method/backlog/bad-code/CORE_server-ts-god-file.mddocs/method/backlog/bad-code/CORE_stale-vision-doc.mddocs/method/backlog/cool-ideas/CORE_diff-summary-line.mddocs/method/backlog/cool-ideas/CORE_mcp-tool-descriptions.mddocs/method/backlog/cool-ideas/CORE_receipt-compression-ratio.mdsrc/git/diff.tssrc/operations/graft-diff.ts
- getFileAtRef: returns null for clean file absence, throws GitError for invalid refs or git failures (tri-state contract) - getChangedFiles: throws GitError for invalid refs instead of silently returning empty array - graft-diff: handles both-null edge case (skip), catches GitError from getFileAtRef gracefully - Design doc: added language tags to code fences, fixed execSync reference to execFileSync - Backlog: added done criteria to duplicate-test-fixtures item - 2 new tests for GitError on invalid refs
All 12 review issues addressed
227 tests passing, lint + typecheck clean. @coderabbitai review 🥕 |
|
227 passing, lint clean, typecheck clean — kicking off the re-review now. 🥕 ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/git/diff.ts (1)
25-29:⚠️ Potential issue | 🟠 MajorEmpty
baseand prose-based error classification both violate contract and guidelines.
opts.base === ""produces invalidgit diff --name-only ""(exit 128: "ambiguous argument"), breaking index mode. Additionally,getFileAtRef(lines 61–65) classifies errors by substring-matching English prose instead of machine-stable signals, violating the coding guideline requiring enum reason codes. Both require fixes.Fix 1: Empty-base handling
When
opts.base === ""andopts.head === undefined, emit["diff", "--name-only"](index vs working tree), not["diff", "--name-only", ""].Fix 2: Prose-coupled error classification
Replace
msg.includes("does not exist") || msg.includes("exists on disk, but not in")with machine-stable error classification (e.g., parse git exit code, usegit rev-parsefor validation, or define an enum for error types). Locale-dependent prose is brittle and violates guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/git/diff.ts` around lines 25 - 29, Handle empty base by treating opts.base === "" as unspecified when opts.head is undefined: in the args construction for the diff call (where base is set from opts.base ?? "HEAD" and args is built with ["diff","--name-only", base, opts.head] or ["diff","--name-only", base]) ensure the branch that would append an empty string instead emits ["diff","--name-only"] (i.e., omit base entirely) when base === "" and opts.head === undefined. For getFileAtRef, remove brittle substring checks of error messages and introduce a machine-stable classification: call git rev-parse (or inspect git exit codes) to validate refs before diffing and/or create an enum (e.g., RefErrorReason) and map git command exit codes or rev-parse results into those enum reasons instead of using msg.includes(...); update the error handling paths in getFileAtRef to use that enum for downstream logic.src/operations/graft-diff.ts (1)
82-86:⚠️ Potential issue | 🟠 MajorFix bare error suppression that corrupts diff semantics by mislabeling I/O failures as deletions.
At lines 84–86, the
catchblock swallows every filesystem error and coerces it to null. This is indefensible given that 10 lines above, the same function correctly discriminatesGitErrorfrom other exceptions.When
headContentbecomes null due to permission denial, disk I/O error, or other non-ENOENT conditions, line 97 incorrectly marks the file as"deleted". This silently corrupts the diff semantics and violates the error handling discipline already established in this codebase.The suggested fix is correct: discriminate ENOENT and ENOTDIR (genuine absence) from all other errors, and re-throw permission/I/O failures instead of hiding them.
Suggested fix
} else { const fullPath = path.join(cwd, filePath); try { headContent = fs.readFileSync(fullPath, "utf-8"); - } catch { - headContent = null; + } catch (err: unknown) { + if ( + err && + typeof err === "object" && + "code" in err && + ((err as { code?: string }).code === "ENOENT" || + (err as { code?: string }).code === "ENOTDIR") + ) { + headContent = null; + } else { + throw err; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/operations/graft-diff.ts` around lines 82 - 86, The catch around fs.readFileSync that sets headContent = null is swallowing all filesystem errors and causing non-existent-file logic to mislabel I/O/permission failures as deletions; update the try/catch in the code that reads headContent (fs.readFileSync(fullPath, "utf-8")) to check the caught error.code and only treat ENOENT and ENOTDIR as "missing" (set headContent = null), but re-throw any other errors (e.g., EACCES, EIO) so they surface instead of being converted to a deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/0006-graft-diff/design.md`:
- Around line 58-59: Update the extension→language mapping in the design doc
where it currently shows "Detect language from extension (`.ts` → `\"ts\"`,
`.js` → `\"js\"`)" to include JSX/TSX (add `.tsx` → `\"tsx\"`, `.jsx` →
`\"jsx\"`) and update any related mentions (e.g., the step referencing
`extractOutline`) so the supported extensions list and parsing steps
consistently reflect `.ts/.tsx/.js/.jsx` support; ensure the same changes are
applied to the other occurrence noted (around the other mention of the mapping).
In `@docs/method/backlog/bad-code/CORE_shape-soup.md`:
- Around line 28-29: The sentence fragment "**MCP tool args** — cast from
`Record<string, unknown>` with `as`." should be rewritten to include a subject
and form a complete sentence; update it to something like "MCP tool args are
currently cast from `Record<string, unknown>` using `as`, but should be parsed
through zod into domain types." Locate this text in CORE_shape-soup.md (the
phrase "**MCP tool args** — cast from `Record<string, unknown>` with `as`.") and
replace it with the complete sentence that includes the subject and the
suggested wording.
In `@docs/method/backlog/up-next/CC_json-codec-port.md`:
- Around line 24-26: The docs currently leave codec selection implicit around
codec.encode(value), which risks sending CBOR where MCP/NDJSON expect JSON;
explicitly define a codec-selection contract: add/require a selector parameter
or API (e.g., selectCodec(context|surface)) and document the canonical mapping
(MCP and NDJSON surfaces => JSON; storage surfaces => CBOR), update all callers
of codec.encode(value) and codec.decode(...) to pass the selected codec and
validate the I/O type, and add runtime checks in encode/decode to reject
mismatched formats; reference and update the codec.encode/codec.decode usage and
surfaces named MCP, NDJSON, and storage in the docs so callers know who must
choose the codec.
- Around line 16-18: Update the documentation to choose a single canonical JSON
algorithm for CanonicalJsonCodec (used for MCP responses, NDJSON logs,
receipts): replace the ambiguous "RFC 8785 or sorted-keys + no whitespace"
wording with a single statement that the codec uses RFC 8785 exclusively, remove
the alternative ad-hoc approach throughout the doc (including the paragraphs
that currently allow either method), and explicitly state that RFC 8785 is
required to guarantee deterministic byte-for-byte outputs across producers.
- Around line 37-38: Update the backlog doc sentence that estimates migration
scope from “~20 raw JSON.stringify calls” to the accurate current count by
inspecting and referencing the actual JSON.stringify usage sites; replace the
"~20" estimate with "3" (the three current usages in the mcp server, metrics
logger, and operations safe-read modules) and keep mention of CanonicalJsonCodec
and CborCodec as planned follow-ups so the entry accurately reflects current
scope.
In `@src/git/diff.ts`:
- Around line 61-65: The catch block in src/git/diff.ts that inspects
err.message after the git "show" call is brittle; replace that approach by
probing the ref and object existence explicitly: before calling git show, run
`git rev-parse --verify ${ref}` and if it exits non-zero treat the ref as
invalid/absent and return null; if rev-parse succeeds then run `git cat-file -e
${ref}:${filePath}` and if that exits non-zero treat the file-as-of-ref as
missing and return null; only call the existing git show logic when both probes
succeed, and otherwise rethrow unexpected errors — update the error-handling
around the git show call (the catch block in src/git/diff.ts) to use these
probes instead of matching human-readable messages.
In `@src/operations/graft-diff.ts`:
- Around line 64-69: The try/catch blocks in graft-diff that catch GitError and
coerce it to null must be removed: directly call getFileAtRef(...) to assign
baseContent and headContent (do not catch GitError), so invalid refs/ repo
errors propagate per getFileAtRef's tri-state contract; specifically remove the
try/catch around the getFileAtRef call that sets baseContent and the analogous
one that sets headContent (references: getFileAtRef, GitError, baseContent,
headContent, filePath, cwd).
---
Duplicate comments:
In `@src/git/diff.ts`:
- Around line 25-29: Handle empty base by treating opts.base === "" as
unspecified when opts.head is undefined: in the args construction for the diff
call (where base is set from opts.base ?? "HEAD" and args is built with
["diff","--name-only", base, opts.head] or ["diff","--name-only", base]) ensure
the branch that would append an empty string instead emits
["diff","--name-only"] (i.e., omit base entirely) when base === "" and opts.head
=== undefined. For getFileAtRef, remove brittle substring checks of error
messages and introduce a machine-stable classification: call git rev-parse (or
inspect git exit codes) to validate refs before diffing and/or create an enum
(e.g., RefErrorReason) and map git command exit codes or rev-parse results into
those enum reasons instead of using msg.includes(...); update the error handling
paths in getFileAtRef to use that enum for downstream logic.
In `@src/operations/graft-diff.ts`:
- Around line 82-86: The catch around fs.readFileSync that sets headContent =
null is swallowing all filesystem errors and causing non-existent-file logic to
mislabel I/O/permission failures as deletions; update the try/catch in the code
that reads headContent (fs.readFileSync(fullPath, "utf-8")) to check the caught
error.code and only treat ENOENT and ENOTDIR as "missing" (set headContent =
null), but re-throw any other errors (e.g., EACCES, EIO) so they surface instead
of being converted to a deletion.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9e319376-1cb0-42f0-9ccb-f85cb7e18146
📒 Files selected for processing (10)
STYLE.mddocs/design/0006-graft-diff/design.mddocs/method/backlog/bad-code/CORE_duplicate-test-fixtures.mddocs/method/backlog/bad-code/CORE_hexagonal-violation.mddocs/method/backlog/bad-code/CORE_shape-soup.mddocs/method/backlog/up-next/CC_json-codec-port.mddocs/method/legends/CLEAN_CODE.mdsrc/git/diff.tssrc/operations/graft-diff.tstest/unit/git/diff.test.ts
- getFileAtRef uses git rev-parse --verify + git cat-file -e for ref/object existence checks. No error message parsing. - Removed redundant GitError catch blocks in graft-diff.ts — getFileAtRef's contract is now clean (null = absent, throw = error) - Design doc: updated language support to include .tsx/.jsx - Backlog fixes: sentence fragment, canonical JSON algorithm choice, codec selection rule, corrected migration scope
Round 2: All 7 new review issues addressed
Cumulative: 19 issues across 2 rounds, 19 resolved. 227 tests, lint + typecheck clean. @coderabbitai review 🥕 |
Rate Limit Exceeded
|
Promoted to ASAP (blocks 0.1.0): - lang detection bug (.js parsed as TS in server diff paths) - stale VISION.md (public-facing doc has wrong numbers) - MCP tool descriptions (empty schemas = bad agent UX) ASAP now has 5 pre-release items + 1 post-release (WARP).
1 participant
Summary
graft_diffMCP tool: structural diff between any two git refs(or working tree vs HEAD). Shows added/removed/changed symbols
with signatures, not line hunks.
src/git/diff.ts: git command wrappers (changed files, file at ref)src/operations/graft-diff.ts: orchestrates git + parser + diffOutlinesPlayback witness
Test plan
changed signatures, path filter, non-supported extensions