[1/4] feat(git): add commit context collector - #1227
[1/4] feat(git): add commit context collector#1227Rafael-Silva-Oliveira wants to merge 1 commit into
Conversation
Adds `getCommitContext()`, which gathers the changes a commit message should describe. Part 1 of 4 for AI commit-message generation; nothing consumes it yet. Staged changes are collected first, since that is what a commit will actually contain. When nothing is staged it falls back to the working tree so callers still have something to summarize before staging. The fallback reads `git status --short` rather than a diff because untracked files appear in no diff and would otherwise be invisible. The fallback deliberately runs `git diff` rather than `git diff HEAD`: the index is known to be empty at that point so the output is identical, but `HEAD` does not resolve in a repository without an initial commit, where it would fail. Reuses the existing `checkGitInstalled`, `checkGitRepo`, and `truncateOutput` helpers. `maxBuffer` is raised past Node's 1MB `exec` default, which real diffs routinely exceed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesGit context collection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant getCommitContext
participant Git
Caller->>getCommitContext: Request commit context
getCommitContext->>Git: Check repository and staged changes
Git-->>getCommitContext: Return status and diff
getCommitContext->>Git: Read working-tree changes when no staged changes exist
Git-->>getCommitContext: Return fallback status and diff
getCommitContext-->>Caller: Return formatted context or null
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/utils/__tests__/git.spec.ts (2)
401-426: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winReject all relevant shell metacharacters.
The assertion permits
&,|,<,>,%, and^.cmd.exetreats these characters specially. Use an allowlist of the expected Git commands, or reject the complete metacharacter set.🤖 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/utils/__tests__/git.spec.ts` around lines 401 - 426, Update the command validation assertion in the “should build diff arguments that need no shell quoting” test to reject all relevant cmd.exe shell metacharacters, including &, |, <, >, %, and ^, rather than only quotes and parentheses. Keep the existing diffCommands filtering and command safety check intact.
378-378: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument or remove the double assertion.
The nearby comment explains the empty
ChildProcessreturn value. It does not explain whyimplementation as unknown as typeof execis safe. Type the mock against the requiredexecoverload, or add a nearby comment that explains why the double assertion is necessary.As per coding guidelines, “Use double assertions only as a last resort and explain them with a comment.”
🤖 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/utils/__tests__/git.spec.ts` at line 378, Update the mock setup around vitest.mocked(exec) to avoid the implementation as unknown as typeof exec double assertion by typing the mock implementation against the required exec overload; if the assertion is unavoidable, add a nearby comment explaining why it is safe and necessary.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/utils/__tests__/git.spec.ts`:
- Line 358: Remove the duplicate ExecResult and gitAvailable declarations in the
test scope, retaining only one declaration of each and updating references as
needed so the git tests compile without changing their behavior.
In `@src/utils/git.ts`:
- Line 16: Update truncateOutput usage in the commit-context output paths to
enforce both the existing GIT_OUTPUT_LINE_LIMIT and a character limit, including
the alternate return paths near the referenced locations. Preserve complete-line
truncation while adding the character cap, and add a focused test covering a
diff containing one very long changed line.
---
Nitpick comments:
In `@src/utils/__tests__/git.spec.ts`:
- Around line 401-426: Update the command validation assertion in the “should
build diff arguments that need no shell quoting” test to reject all relevant
cmd.exe shell metacharacters, including &, |, <, >, %, and ^, rather than only
quotes and parentheses. Keep the existing diffCommands filtering and command
safety check intact.
- Line 378: Update the mock setup around vitest.mocked(exec) to avoid the
implementation as unknown as typeof exec double assertion by typing the mock
implementation against the required exec overload; if the assertion is
unavoidable, add a nearby comment explaining why it is safe and necessary.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 737f916c-e49f-4b5b-8453-ccbf4fe7ed77
📒 Files selected for processing (2)
src/utils/__tests__/git.spec.tssrc/utils/git.ts
| describe("getCommitContext", () => { | ||
| const mockDiff = "@@ -1,1 +1,2 @@\n-old line\n+new line" | ||
|
|
||
| type ExecResult = { stdout: string; stderr: string } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate declarations.
Lines 358 and 381 redeclare ExecResult and gitAvailable in the same scope. TypeScript rejects these declarations, so the test file cannot compile. Keep one declaration of each.
Also applies to: 381-381
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { ExecException } from "child_process"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/utils/__tests__/git.spec.ts` at line 358, Remove the duplicate ExecResult
and gitAvailable declarations in the test scope, retaining only one declaration
of each and updating references as needed so the git tests compile without
changing their behavior.
| const GIT_OUTPUT_LINE_LIMIT = 500 | ||
|
|
||
| // Node's default `exec` buffer is 1MB, which real-world diffs routinely exceed. | ||
| const GIT_DIFF_MAX_BUFFER = 10 * 1024 * 1024 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a character limit to commit context output.
GIT_DIFF_MAX_BUFFER permits up to 10 MiB of diff output. truncateOutput(output, GIT_OUTPUT_LINE_LIMIT) limits lines only. A diff with one very long changed line remains almost unbounded because the helper preserves complete retained lines.
Pass a character limit in both return paths. Add a focused test with a long single-line diff.
Also applies to: 387-387, 403-403
🤖 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/utils/git.ts` at line 16, Update truncateOutput usage in the
commit-context output paths to enforce both the existing GIT_OUTPUT_LINE_LIMIT
and a character limit, including the alternate return paths near the referenced
locations. Preserve complete-line truncation while adding the character cap, and
add a focused test covering a diff containing one very long changed line.
github-actions
Bot
added
the
awaiting-review
|
|
||
| if (!status.trim()) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
Could we include bounded contents for untracked files rather than only their paths? Without the contents, an untracked-only change does not give the model enough information to write an accurate commit message.
|
|
||
| if (!status.trim()) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
Could we use NUL-delimited, machine-readable Git output and parse it into structured file entries? The current human-readable output can ambiguously represent renames, binary files, and filenames containing unusual characters.
|
|
||
| if (stagedSummary.trim()) { | ||
| const { stdout: stagedDiff } = await execAsync(`git diff --cached ${COMMIT_DIFF_ARGS}`, options) | ||
| const output = `Staged changes:\n\n${stagedSummary.trim()}\n\n${stagedDiff.trim()}` |
There was a problem hiding this comment.
Could we also pass a finite character limit to truncateOutput()? A minified or generated file can contain a multi-megabyte single line that bypasses the current 500-line limit.
|
|
||
| const options = { cwd, maxBuffer: GIT_DIFF_MAX_BUFFER } | ||
|
|
||
| const { stdout: stagedSummary } = await execAsync("git diff --cached --stat", options) |
There was a problem hiding this comment.
Could we handle failures from the status and diff commands through a clear nullable or typed-error contract? Right now an expected Git failure, such as exceeding maxBuffer, rejects even though this function is documented as returning context or null.
2 participants
Related GitHub Issue
Closes: #282
Part of: #145 · Stack 1 of 4 · Replaces the all-in-one #1218
Description
Adds
getCommitContext(), the Git-reading half of AI commit-message generation.Nothing consumes it yet — that arrives in stack 2. This PR is self-contained and
introduces no user-facing behavior.
Staged first, working tree as fallback. Staged changes are what a commit will
actually contain, so they take priority. When nothing is staged the collector falls
back to the working tree, so a caller still has something to summarize before the
user has staged anything.
The fallback reads
git status --short, not a diff. Untracked files appear in nodiff, so a diff-only fallback would silently omit brand-new files — usually the most
interesting thing in the change.
No
HEADin the fallback diff.git diffis used rather thangit diff HEAD.The index is known to be empty on that path so the two are equivalent, but
HEADdoesnot resolve in a repository without an initial commit, where it fails outright. This
is covered by a regression test.
Reuses the existing
checkGitInstalled,checkGitRepo, andtruncateOutputhelpersrather than adding new ones.
maxBufferis raised past Node's 1 MBexecdefault,which real diffs routinely exceed.
Scope note: this deliberately shells out to
git diffand passes the output through.It does not parse rename/copy status codes or summarize binary files. #298 takes a
much more thorough approach to the same problem — see "Relationship to #298-#301" below.
Test Procedure
src/utils/__tests__/git.spec.tscovers: staged path, working-tree fallback,untracked-only repository with no initial commit, clean tree returning
null, git notinstalled, and not-a-repository.
There is also a test asserting the diff argument string contains no shell
metacharacters. That guards a real bug found during development: an earlier revision
used
:(exclude)pathspecs to skip lockfiles, which unit tests happily passed becausethey mock
exec— but real git rejected the command, sinceexecruns throughcmd.exeon Windows and does not strip the single quotes those pathspecs require. Themocked tests cannot validate git syntax, so that class of bug needs the guard.
Verified against real repositories, not just mocks:
git init'd repo with no commits → returns context instead of throwing.Local checks:
pnpm lint,pnpm check-types(11/11 packages), fullsrcsuite(7379 passed, 37 skipped),
node scripts/find-missing-translations.js.Pre-Submission Checklist
Visual Snapshots
Not applicable. This PR adds a service function with no UI.
Documentation Updates
Nothing user-facing lands until stack 3.
Additional Notes
Review order: 1 → #1228 → #1229 → #1230. Each targets
mainbecause GitHub cannotbase a cross-fork PR on another fork's branch, so later PRs show cumulative diffs until
their parents merge. The Commits tab shows only that PR's own commit — that is the
reviewable unit.
Relationship to #298-#301. @Mirrowel has an open stack covering this same feature,
untouched since 2026-06-30. I built this independently before finding it. Where they
overlap, that stack is more thorough: #298 is ~996 lines handling rename/copy status
codes,
-znull-delimited parsing, synthetic diffs for untracked files, and binary-filesummarization. This is ~190 lines and does none of that.
The tradeoff is size against completeness. If #298 is revived I would rather see that
land, and I am happy to close this. If it stays stale, this is ready now.
Summary by CodeRabbit
New Features
Bug Fixes