fix(hooks): resolve gitnexus on PATH with a pure-Node scan, all-OS (#1938)#1980
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✨ PR AutofixFound fixable formatting / unused-import issues across 15 changed lines. Comment |
5a8ae45 to
f6ed59c
Compare
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 10926 tests passed 12 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
0339f9d to
096e145
Compare
096e145 to
0cde75b
Compare
…1938) The stale-index hint must prefer a PATH-installed `gitnexus` over `npx gitnexus` (which crashes npm 11's arborist, #1939). #1945 already does this via `formatAnalyzeCommand`, but its `resolveOnPath` shelled out to `where`/`which` — which fails outright when the probe binary is itself unreachable (a sanitized hook PATH without System32 / `/usr/bin`), exactly the Windows case #1938 reports. Replace the subprocess (and a hand-rolled fallback) with a single spawn-free PATH scan — issue #1938's preferred "Option A". One code path, identical on every OS: - no dependency on `where`/`which` being reachable; - no shell-spawn surface (CVE-2024-27980) and no spawn timeout to tune; - Windows matches PATHEXT extensions only (mirrors `where`/cmd.exe), so neither a `.ps1`-only shim nor a bare extensionless file is a false positive; - `preferExecExt` keeps the `.cmd`/`.bat`/`.exe` wrapper preference. `resolveOnPath` is now pure (platform/env injectable) and exported; `pickPathMatch` and the interim `scanPathForCommand` are removed. Version detection still spawns `npm`/`pnpm --version` (unavoidable) and is unchanged. Tests: drive `resolveOnPath` directly across POSIX/Windows cases (PATHEXT, `.ps1`, extensionless, preferExecExt, X_OK) + a cross-platform `formatAnalyzeCommand` end-to-end case, plus the existing hook e2e auto-detection tests and new `pathWithoutGitNexus`/`envWithPath`/`createGitNexusPathEntry` helpers. Both `resolve-analyze-cmd.cjs` copies kept byte-identical. Validated: tsc clean, prettier clean, eslint 0 errors, 312 targeted tests pass (unit + hook e2e + the resolver's other consumers). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0cde75b to
547172d
Compare
magyargergo
left a comment
There was a problem hiding this comment.
Summary
Reviewed 7 lanes across 2 engines. GitNexus lanes (Claude): risk-architect, test-ci-verifier. Compound-Engineering lanes (Claude): correctness, adversarial, maintainability, testing. Independent engine: Codex — gpt-5.5, reasoning effort xhigh — the only non-Claude reviewer. Six lanes share Claude's priors, so the strong signal is Codex + a Claude lane agreeing. Supplemental claude-mem context was requested but its index was degraded, so prior-session grounding came from local memory.
Disclosure: the PR author and this review's coordinator are the same. Independence rests on the agent lanes + Codex and on CI, not on the coordinator.
The review materially reshaped this PR, which is the point of running it:
- It started as a pure-Node
scanPathForCommandfallback behind the existingwhere/whichsubprocess. Review (and a maintainer ask to make it elegant + all-OS) drove a redesign into issue #1938's "Option A":resolveOnPathis now a single spawn-free PATH scan,where/which+pickPathMatchremoved. - Codex caught that a squash commit had not actually included the refactor (it read the pushed blob, not the working tree) — fixed and re-verified on
origin. - Codex also flagged a PATH-delimiter seam; an attempt to derive the delimiter from the injected platform then failed the real
windows-latestjob (it split aC:\…path at the drive colon under a POSIX-injected test). Reverted to hostpath.delimiter(production-identical), whichwindows-latestnow confirms green.
Final design (what's merging)
resolveOnPath(command, preferExecExt, { platform, env }) scans each PATH dir × the platform's executable extensions (PATHEXT on Windows; bare name + X_OK on POSIX). One code path, identical on every OS, no subprocess — so the original #1938 failure mode (the probe binary unreachable on a sanitized PATH) is gone, along with the shell-spawn surface and the .ps1/extensionless false positives (Windows matches PATHEXT only; preferExecExt keeps the .cmd/.bat/.exe preference). Version detection still spawns npm/pnpm --version (unavoidable) and is unchanged. Both resolve-analyze-cmd.cjs copies stay byte-identical.
Validated (credit)
- Common path correct: the scan models exactly what
where/whichresolved (PATH × PATHEXT/X_OK);analyzeCLI path still works on Windows. (correctness + adversarial + Codex) resolveOnPathlogic verified by simulation and unit tests: PATHEXT parse/normalize, precedence, dir-skip, symlink-follow,X_OK,.ps1-not-matched, extensionless-not-matched,preferExecExt.- Export surface:
pickPathMatch/scanPathForCommandremoved,resolveOnPathexported; the CLI'sresolve-invocation.tscreateRequire drift-guard surface (resolveInvocationMode/formatDocumentationDlxCommand/NPX_REF) intact. Byte-identical parity holds;setup.tspropagates to the antigravity install. - Tests leak-safe (try/finally + afterEach);
--embeddingsvariant non-redundant; theRun \gitnexus analyze`` assertion is tight.
Lower-priority follow-ups (not blocking · Claude-only, weaker signal)
- P3
runHook's test-helper env contract is replace-not-merge; a future partial-env caller would lose PATH (no current caller affected — worth a one-line JSDoc note). - P3 the scan has no per-iteration cap (acceptable: ~37 ms / 10k local-fs misses; only relevant on network-mounted PATH dirs).
CI / merge state
All checks green on 547172de: 30 success, 1 skipped, mergeable: CLEAN. Includes tests / windows-latest (platform-sensitive) and macos-latest (which run the changed resolve-invocation.test.ts + hooks-e2e.test.ts), ubuntu / coverage, scope-parity, and quality (typecheck/format/lint). One transient cli-e2e.test.ts Windows flake (eval-server/EPIPE/stdout-fd, #324 — unrelated to this diff) cleared on re-run with no code change. Local: tsc clean, prettier clean, eslint 0 errors, 312 targeted tests pass.
Verdict
Production-ready. No blocking issues; the design simplification and both review findings are resolved and CI-confirmed across the OS matrix. Two optional P3 follow-ups noted above.
Automated multi-tool digest (pr-tri-review): 6 Claude lanes + Codex gpt-5.5/xhigh. The coordinator authored the PR — verify before acting.
Summary
Finishes #1938 with the approach its reporter actually preferred ("Option A"): the stale-index hint resolves a PATH-installed
gitnexuswith one spawn-free, pure-Node PATH scan, replacing thewhere/whichsubprocess.#1945 already routes all three hooks through
formatAnalyzeCommandand prefers a globalgitnexusover the npm-11-crashingnpx. But itsresolveOnPathshelled out towhere/which, which fails outright when the probe binary itself is unreachable — a sanitized hook PATH stripped ofSystem32(Windows) or/usr/bin(a minimal container) — leaving the hint to degrade toward thenpxcrash path even thoughgitnexusis installed. That is exactly the Windows scenario #1938 reports.Change
resolveOnPathis now a single pure-Node scan: each PATH dir × the platform's executable extensions (PATHEXT on Windows; bare name +X_OKon POSIX). One code path, identical on every OS:where/whichbeing reachable — the original bug class disappears.where/cmd.exe — so neither an un-spawnable.ps1-only shim (absent from default PATHEXT) nor a bare extensionless file (which a shell can't launch asgitnexus) is a false positive.preferExecExtkeeps the.cmd/.bat/.exewrapper preference for the gitnexus lookup.resolveOnPathis now pure (platform/env injectable) and exported;pickPathMatchis removed. Version detection still spawnsnpm/pnpm --version(unavoidable — that needs to run the tool) and is unchanged. Bothresolve-analyze-cmd.cjscopies stay byte-identical (parity test enforced).Why this is equivalent-or-better, not a downgrade
where/whichonly ever resolve PATH dirs × executable rules — which this models exactly (Windows PATHEXT; POSIXX_OK). It does not lose the Windows "App Paths" registry, becausewheredoesn't consult that for bare-command resolution either. So the scan is a faithful model of "willgitnexus analyzeactually run", with no subprocess to be missing, hang, or need a shell.Tests
resolveOnPathunit cases (injected platform/env, no host-PATH dependency): POSIX exec found / non-exec skipped (X_OK,skipIfon Windows) / absent / empty PATH; Windows PATHEXT.cmd/.ps1-only-not-matched / extensionless ignored when a shim exists / extensionless-only → null /preferExecExtprefers.cmdover.COMbut accepts a lone.COM.formatAnalyzeCommandend-to-end case: a launcher alone on PATH resolvesgitnexus analyze(works on every OS now — nowhere/whichreachability caveat).pathWithoutGitNexus/envWithPath/createGitNexusPathEntryhelpers (mirror the hook's ownisFile()+X_OKdetection).Validation
npx tsc --noEmitclean;prettier --checkclean;eslint0 errors.vitest run— 312 targeted tests pass (unit + both hook e2e suites + the resolver's other consumershooks.test.ts/cursor-hook.test.ts).which/whereunreachable but a launcher installed, the old resolver emitsnpx gitnexus@latest analyze; the pure scan emitsgitnexus analyze.Impact / risk
resolveOnPathis not in the GitNexus index (newer than the snapshot), so blast radius assessed manually: callers areresolveInvocationMode(default probe) andformatAnalyzeCommand(memoized probe), consuming the result only for truthiness; plus the CLI'sresolve-invocation.tsvia the unchanged export surface. This does change the common resolution path (every probe now scans instead of spawning), so it's validated across the cross-platform CI matrix, not just locally. Risk: LOW — equivalent model, simpler, no new failure modes.🤖 Generated with Claude Code