fix(git): suppress stderr leak in getCurrentCommit and getGitRoot (#1172)#1341
Conversation
…higyanpatwari#1172) Node's execSync forwards the child's stderr to the parent process when the stdio option is not explicitly set. getCurrentCommit and getGitRoot both caught the resulting error but did not suppress the stderr output, causing "fatal: not a git repository" messages to leak to the terminal whenever they were called on a path outside a git worktree. Add stdio: ['ignore', 'pipe', 'ignore'] to both functions, matching the pattern already used by getRemoteUrl, getRemoteOriginUrl, and getCanonicalRepoRoot in the same file.
|
@azizur100389 is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 8021 tests passed 1 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Claude finished @magyargergo's task in 3m 51s —— View job PR #1341 Review — fix(git): suppress stderr leak in getCurrentCommit and getGitRootTasks
Review bar for this PR
Current PR state
Understanding of the changeNode's
This mirrors the pattern already established by Findings[MINOR]
|
| Caller | Usage | Impact |
|---|---|---|
status.ts:21 |
getGitRoot(cwd) ?? cwd |
No longer leaks stderr when cwd is outside a git repo |
status.ts:33 |
getCurrentCommit(repo.repoPath) |
Quiet on failure; '' fallback unchanged |
analyze.ts:225 |
getGitRoot(process.cwd()) |
Quiet on failure; null check at line 226 triggers user-friendly console.log message (not affected) |
index-repo.ts:50 |
getGitRoot(process.cwd()) |
Same; null handled cleanly |
wiki.ts:101 |
getGitRoot(process.cwd()) |
Same |
git-staleness.ts:118 |
getCurrentCommit(cwdGitRoot) |
Quiet on failure; ` |
run-analyze.ts:168 |
getCurrentCommit(repoPath) behind repoHasGit guard |
Unchanged path; guard still controls whether it's called |
No caller relied on git stderr being visible. All callers use null/'' fallbacks. No legitimate GitNexus-owned diagnostics are suppressed — those go through console.error or console.log, not through child-process stderr. Group status command no longer prints fatal: not a git repository when probing repos from a non-git parent directory. ✅
Test assessment
New test (does not leak git stderr to process.stderr (#1172), lines 104–119):
- Uses real git (consistent with suite) ✅
- Isolated
mkdtempwithfinallycleanup ✅ git init -qwithout commits reliably failsgit rev-parse HEAD— this is the exact failure class reported in group status prints "fatal: not a git repository" for valid repos when run from non-git parent directory #1172 ✅process.stderr.writespy restored infinally✅- Asserts
getCurrentCommit(tmpDir) === ''✅ - Asserts no
'fatal'in captured stderr ✅ - Would fail on the pre-fix code (stderr was inherited, not suppressed) ✅
- Cross-platform stable: no hardcoded paths, no timing dependencies, no network ✅
- No git state leaked:
rmSyncinfinally✅
Gap: No equivalent stderr regression test for getGitRoot(). The existing test (returns null for a plain temp directory, lines 125–133) only asserts the return value. This is the main follow-up item.
Pre-existing worktree test (lines 246–288): calls getGitRoot(worktreeDir) in the success path — validates correct behaviour but not quiet-failure. Unrelated to this gap.
Hidden Unicode / hygiene assessment
| File | Finding | Severity |
|---|---|---|
git.ts:20 |
— (U+2014 EM DASH) in new comment |
Low — comment only, not bidi |
git.ts:101 |
— (U+2014 EM DASH) in new comment |
Low — comment only, not bidi |
git-utils.test.ts |
─ (U+2500 BOX DRAWINGS) in section headers |
Pre-existing, not introduced by this PR |
No bidirectional control characters (U+202A–U+202E, U+2066–U+2069) found in either file. No invisible zero-width characters. No non-ASCII in string literals or stdio arrays. The GitHub Unicode warning is triggered by the em dashes in comments — this is cosmetic, not a security or correctness concern.
Scope hygiene: exactly 2 files changed, no package-lock/dependency/generated-file churn, no unrelated formatting, no AGENTS.md/CLAUDE.md edits. ✅
Elegance assessment
This is the correct size of fix. Two stdio option additions and one forwarding comment bring the two previously-inconsistent helpers into line with the rest of the file. The comment on getCurrentCommit (5 lines) explains the Node.js execSync stdio inheritance behaviour clearly — appropriate detail for a non-obvious platform behaviour. The getGitRoot comment (// Suppress stderr — see getCurrentCommit comment and #1172.) avoids duplication by cross-referencing rather than repeating.
A shared quietExec helper is not warranted for two call sites at this point. Adding one would be premature for a targeted fix. If more helpers are added later that need the same pattern, a helper could be introduced then.
Final verdict
production-ready with minor follow-ups
The core fix is correct, consistent, and minimal. Both getCurrentCommit() and getGitRoot() now suppress child-process stderr without changing return semantics, stdout capture, or Windows path normalization. The stdio pattern is now uniform across all stdout-capturing helpers in the file. CI is fully green, typecheck is clean, and the fix lands exactly on the affected functions with no scope creep.
The only gap is that getGitRoot()'s stderr suppression, while demonstrably correct by code inspection, lacks a matching regression test to guard against future reversion. This is a coverage miss on half the changed surface — worth fixing as a small follow-up to the same PR before merge, but not a correctness or production-safety blocker. The em dashes in new comments are harmless cosmetic noise that could be normalized to silence the GitHub Unicode warning banner.
- Add matching process.stderr.write spy test for getGitRoot (abhigyanpatwari#1172) - Replace U+2014 em dashes with ASCII -- in new comments
2 participants
Summary
execSyncforwards child stderr to the parent process when thestdiooption is not explicitly set (documented Node.js behaviour).getCurrentCommitandgetGitRootcaught the resulting error but never suppressed the stderr output, sogit rev-parsefailures printedfatal: not a git repositoryto the user's terminal.stdio: ['ignore', 'pipe', 'ignore']to both functions, matching the pattern already used bygetRemoteUrl,getRemoteOriginUrl, andgetCanonicalRepoRootin the same file.git init-s a directory without commits (forcesgit rev-parse HEADto fail), then assertsprocess.stderr.writeis never called with afatalmessage.Closes #1172
Test plan
npx vitest run test/unit/git-utils.test.ts— 15/18 pass (3 pre-existing failures unrelated to this change)does not leak git stderr to process.stderr (#1172)test passestsc --noEmit— no new type errorsprettier --check— clean