Skip to content

fix(cli): steer docs, skills, and hooks through a CLI-neutral project-local runner (#1939)#1945

Open
magyargergo wants to merge 45 commits into
mainfrom
issue-1939
Open

fix(cli): steer docs, skills, and hooks through a CLI-neutral project-local runner (#1939)#1945
magyargergo wants to merge 45 commits into
mainfrom
issue-1939

Conversation

@magyargergo
Copy link
Copy Markdown
Collaborator

@magyargergo magyargergo commented May 31, 2026

Summary

Mitigates the npm 11.x arborist node.target is null install crash (#1939) by steering every GitNexus surface — committed skills, generated AGENTS.md/CLAUDE.md, and the editor stale-index hooks — through one CLI-neutral command instead of a hardcoded npx gitnexus.

gitnexus analyze drops a small runner at <repo>/.gitnexus/run.cjs (a copy of the canonical hooks/claude/resolve-analyze-cmd.cjs). Docs reference node .gitnexus/run.cjs <cmd>, which auto-selects a working runner at call time: a global gitnexus binary → pnpm dlx gitnexus@latest (npm 11 + pnpm) → npx gitnexus@latest. Committed docs therefore bake in no package-manager assumption and no per-machine content, and the resolver is cross-platform-correct on Windows.

Fixes #1939.

Problem & approach

The crash happens inside npm/arborist during npx install, before gitnexus runs, so runtime error handling cannot catch it — the only lever is steering installs toward working paths and documenting workarounds.

An earlier revision of this PR hardcoded pnpm --allow-build=… dlx gitnexus@latest <cmd> into committed docs/skills. That assumes pnpm is installed — a worse default for the npm majority just to dodge the npx crash. The final design defers the package-manager choice to call time via the project-local runner, so the committed instruction stays universal:

  • Project-local runner, not a ~/.claude/ helperAGENTS.md is the multi-CLI anchor (Cursor/Codex/Antigravity/…), so a Claude-specific path would be wrong for non-Claude readers. The runner lives next to the index, is gitignored, and is refreshed on every analyze.
  • First run stays universal — the README quickstart and an inline bootstrap note use npx gitnexus analyze (the runner can't exist before the first analyze); the npm-11 escape hatch (npm i -g gitnexus, or pnpm … dlx) is documented there.
  • Windows-safe execution — the runner's exec tail and the version/PATH probes resolve .cmd/.ps1/.exe shims through a shell on Windows (execFileSync does no PATHEXT resolution and Node blocks .cmd without a shell — CVE-2024-27980), and print a diagnostic instead of a silent exit 1.

What changed

  • Resolver / runner. resolve-analyze-cmd.cjs (canonical in hooks/claude/, byte-identical twin in gitnexus-claude-plugin/hooks/, parity-tested) gains buildRunnerArgv + a require.main === module exec tail. gitnexus analyze drops it at .gitnexus/run.cjs; failure to copy is non-fatal (docs carry a bootstrap fallback).
  • Generated docs. ai-context.ts emits node .gitnexus/run.cjs <cmd> (machine-independent, no version churn), kept under the #856 CLAUDE.md token budget; Cross-Repo Group commands route through the runner too.
  • Hooks + setup. The stale-index hooks emit the resolver's recommendation; gitnexus setup fails closed — it skips hook registration with an actionable error when a required helper (or adapter) is missing, rather than registering a hook that MODULE_NOT_FOUNDs on every tool event.
  • Skill steering. All committed skill copies (gitnexus/skills, .claude, gitnexus-claude-plugin, gitnexus-cursor-integration) route through the runner, guarded by skills-steering.test.ts.
  • Startup discipline. The npm-11 npx-crash warning runs from the analyze command (after the heap re-exec guard), not at CLI module load, so gitnexus mcp and other commands pay no startup probe cost.

Review history

This PR was hardened across several three-method tri-reviews (Codex + Compound-Engineering personas + GitNexus swarm). Findings from each round are addressed:

  • CI-blocking antigravity hook MODULE_NOT_FOUND (helper not staged); false-passing stdout-only tests.
  • Startup regression (invocation probe at module load) → moved into analyze, memoized.
  • Version-ref drift → standardized hints on gitnexus@latest; the separate, version-pinned MCP-registration ref in setup.ts is intentional and unchanged (renamed to MCP_PINNED_REF to end the same-name collision).
  • AI-context churn, unified fail-closed helper copy, Windows global-shim presence detection (resolveOnPath recognizes .exe/extensionless shims), hook-budget probe bounds, and repo-wide skill steering with a regression guard.

Latest round (the Windows probe defect). The most recent review (Codex + adversarial, cross-engine) found that the --version probes (probeVersion in the cjs, getNpmMajorVersion in the CLI) spawned npm/pnpm --version via execFileSync without a shell, so on Windows the .cmd shims ENOENT'd, the probe reported a present tool as absent, and the stale-index hook recommended the exact npx crash command this PR exists to avoid (the runner's --allow-build version gate was defeated the same way). Not a regression (the pre-pivot hook was unconditionally npx), but the headline npm-11 → pnpm steering was a no-op on the Windows hook path. Fixed:

  • shell: process.platform === 'win32' on the version probes (mirroring the exec tail), so .cmd shims resolve on Windows.
  • Banner-tolerant version parsing (a Corepack/notice line on stdout no longer defeats the parse).
  • Presence carried separately from version, so a present-but-unparseable pnpm still selects pnpm rather than falling to the npx crash path.
  • resolve-analyze-cmd.cjs (+ twin) brought under the shell-injection and windowsHide source-regression guards; the runner exec-tail's Windows shell branch now runs on the windows-latest CI leg (was previously untested everywhere); --embeddings=N (equals form) now widens the pnpm allow-build set; a broken README troubleshooting anchor and a generated-doc Cannot find module recovery hint were fixed.

Test plan

  • tsc --noEmit clean; prettier + typecheck pass (pre-commit) on every commit.
  • Targeted + consolidated unit/integration suites green locally (resolver, hooks, runner exec-tail, ai-context, setup, antigravity e2e, hooks e2e, skills-steering) — the Windows-only cases self-skip on POSIX.
  • runner-exec-tail.test.ts registered in scripts/cross-platform-tests.ts (SPAWN_CLI) so the Windows shell branch runs on windows-latest.
  • Full CI on all three OS runners (the windows-latest legs are the real validator for the .cmd-shim resolution, which can't be exercised on a POSIX dev host).
  • Windows / npm 11 reporter confirms the runner / pnpm dlx guidance resolves their install crash.

Known Residuals

Deliberately out of scope (tracked for follow-up), not regressions:

  • [P2] Runner exec-tail Windows arg-forwarding. The require.main === module exec tail forwards process.argv through execFileSync with shell: process.platform === 'win32', so on Windows cmd.exe would interpret & | ^ in forwarded args. Safe under the PR's actual usage — generated docs/skills emit fixed subcommands; the group commands (group impact --target <symbol> --repo <path>, group sync <name>) carry argument slots, so the trust boundary is "forwarded args must not contain cmd.exe metacharacters." A deeper fix (explicit .cmd resolution) conflicts with the repo's mandated shell idiom and has no clean in-repo primitive; the source-regression guard now locks the file so shell: true can't be introduced.
  • The cli skill's non-analyze npx subcommands and the no-install-free-universal-command-on-npm-11 tradeoff remain documented as-is.

Post-Deploy Monitoring & Validation

  • Watch: CI vitest on all three OS runners — especially tests / windows-latest and the windows-latest cross-platform-tests job (runner-exec-tail + the Windows probe path), the primary signal for the cross-platform fixes; plus setup-antigravity / hooks-e2e.
  • Healthy signals: green CI on Linux/macOS/Windows; gitnexus mcp starts with no pre-protocol stderr; regenerating AGENTS.md/CLAUDE.md on an unchanged repo produces no diff (no machine/version churn); on npm ≥ 11 with pnpm present, the stale-index hint recommends the pnpm dlx (not npx) form on Windows.
  • Failure signals → mitigation: MODULE_NOT_FOUND / Cannot find module in hook logs → a required helper failed to copy (re-run gitnexus setup); stray stderr before MCP protocol output → a startup probe regressed; doc churn on regenerate → AI-context emitted machine-specific content again.
  • Validation window / owner: the PR author across the first merges, plus the next Windows / npm-11 repro confirmation on Follow-up to #819: npx gitnexus still crashes with "node.target is null" on npm 11.x — clean cache doesn't help #1939. No data migration; no dashboards beyond CI.

Made with Cursor; hardened via automated multi-engine tri-review and remediation.

Prefer global gitnexus or pnpm dlx in hooks and generated AI context, warn
when npm 11.x would use the broken npx path, and document workarounds for
the arborist node.target null failure mode.

Co-authored-by: Cursor <cursoragent@cursor.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented May 31, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
gitnexus Ready Ready Preview, Comment Jun 2, 2026 7:24am

Request Review

Comment thread gitnexus/test/integration/antigravity-hook-e2e.test.ts Fixed
Comment thread gitnexus/test/integration/hooks-e2e.test.ts Fixed
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 31, 2026

CI Report

All checks passed

Pipeline Status

Stage Status Details
✅ Typecheck success tsc --noEmit
✅ Tests success unit tests, 3 platforms
✅ E2E success gitnexus-web changes only

Test Results

Tests Passed Failed Skipped Duration
10838 10828 0 10 675s

✅ All 10828 tests passed

10 test(s) skipped — expand for details
  • COBOL pipeline benchmark > scales with file count
  • C# pipeline benchmark > scales with file count — namespaces spread across the solution
  • C# pipeline benchmark > scales with file count — all types in one (global) namespace bucket
  • C# pipeline benchmark > scales with file count — all types in one (named) namespace bucket
  • Go pipeline benchmark > scales with file count (workers enabled)
  • Go pipeline benchmark — worker pool (issue Worker idle timeout kills long Go scope extraction and surfaces as Napi::Error during analyze #1848) > does not quarantine the large generated Go file on sub-batch idle timeout
  • PHP pipeline benchmark > scales with file count (workers enabled)
  • Ruby pipeline benchmark > scales with file count (workers enabled)
  • Rust pipeline benchmark > scales with file count (workers enabled)
  • buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature

Code Coverage

Tests

Metric Coverage Covered Base Delta Status
Statements 80.35% 37536/46711 79.84% 📈 +0.5 🟢 ████████████████░░░░
Branches 68.91% 23895/34671 68.5% 📈 +0.4 🟢 █████████████░░░░░░░
Functions 85.47% 3889/4550 84.94% 📈 +0.5 🟢 █████████████████░░░
Lines 83.92% 33776/40246 83.36% 📈 +0.6 🟢 ████████████████░░░░

📋 View full run · Generated by CI

Copy link
Copy Markdown
Collaborator Author

@magyargergo magyargergo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Tri-Review — #1945 fix(cli): steer npm 11 users away from npx install crash

Verdict: not production-ready · Merge state: checks failing · Branch hygiene: clean feature/fix PR (single commit, no merge-from-main).

Methods & engines (read first). This was a two-method, both-Claude review: the GitNexus swarm (risk-architect, test/CI-verifier) + Compound-Engineering personas (correctness, adversarial, maintainability) — five lanes, but all the same engine. The Codex lane (the only independent engine) was dispatched, but its background job never registered/returned, so there is no independent-engine corroboration here; treat cross-lane agreement as "consistent across personas," not independent confirmation. The coordinator independently reproduced the blocking findings.

Headline (blocking — P0): the PR turns CI red on all three OS runners

test/unit/setup-antigravity.test.ts has 3 failing tests (AfterTool emits stale-index hint…, ignores unknown tool names without crashing, does not crash on empty stdin). Reproduced locally: npx vitest run test/unit/setup-antigravity.test.ts3 failed | 14 passed. The antigravity hook's new top-level require('./resolve-analyze-cmd.cjs') (gitnexus-antigravity-hook.cjs:28) is unsatisfied — the source hooks/antigravity/ dir doesn't contain the helper and the test's stageAdapter() doesn't copy it — so the staged adapter crashes with MODULE_NOT_FOUND. The 4 sibling adapter tests that still pass only assert stdout.trim()==='' (setup-antigravity.test.ts:339–452), which a crash also satisfies, so the suite under-reports its own breakage. (See inline comment.)

Other inline findings

  • [P2] Module-load side effect on every CLI start (incl. gitnexus mcp). warnIfNpm11NpxRisk() runs at index.ts:14 top level, spawning which/where (+ npm --version) — up to 3 synchronous 5s-timeout subprocesses — before command dispatch. Reproduced: GITNEXUS_INVOCATION=npx node dist/cli/index.js --version emits the warning. Re-adds load-time work to the mcp path the file header says was optimized, and to the high-frequency augment hook. Adversarial refuted the scarier variant: the write is stderr-only, so JSON-RPC/stdout is not corrupted.
  • [P2] Version pinning is inconsistent and largely illusory. The plugin copy hardcodes gitnexus@latest (gitnexus-claude-plugin/hooks/resolve-analyze-cmd.cjs:8) while the TS/in-repo copies pin gitnexus@<version>; and the in-repo .cjs's package.json read resolves to a non-existent path once setup.ts copies it to ~/.claude/hooks/gitnexus/, so it silently falls back to latest in production too. The "Keep in sync" comment is already false, and a @latest stale-index hint can re-suggest the npx path #1939 steers users away from. (Consistent across risk + correctness + maintainability — same engine.)

Notable, lower-priority (body only)

  • [P2] ai-context.ts:131 bakes machine-local resolution into git-tracked docs. formatAnalyzeCommand() resolves on the analyzing machine and the result is written into the committed AGENTS.md/CLAUDE.md stale-index hint (verified: both tracked; the default regeneration path rewrites that line). Teammates on a different setup get a wrong command, and the pinned version churns the tracked files on every analyze/version-bump — the same volatile-content churn class as #1706 (symbol-count churn in committed AI-context files; now closed). Single-lane (correctness) + coordinator-verified.
  • [P3] The runtime npm-11 warning may never reach its audience. #1939 crashes during npx install (before gitnexus runs); a successful npx install puts gitnexus on PATH, so resolveInvocationMode() returns gitnexus and the warning is gated off (resolve-invocation.ts:94). README/install guidance is the effective channel.
  • Test gaps. No assertion that the Claude install path copies resolve-analyze-cmd.cjs, and that copy is silently swallowed (setup.ts:400–407, empty catch); warnIfNpm11NpxRisk suppression branches (mode≠npx, npm<11, npm=null) untested and the module-level npm11Warned flag (resolve-invocation.ts:23) is never reset (order-dependent); no parity test across the 3 resolver copies; the Windows .cmd/.bat wrapper branch isn't in the cross-platform matrix.
  • [P3] Windows .exe-only shims (Volta/scoop) aren't matched by the .cmd/.bat filter → suboptimal (not broken) hint. [P3] resolve-invocation.ts:13 throws at module load if package.json#version is missing — unreachable in normal tsc packaging (refuted as a live crash) but a behavior change vs. the graceful CJS latest fallback.

Credit (validated by the review)

env-var parsing/precedence correct; getNpmMajorVersion() null-safe (handles garbage/prerelease); CRLF trimmed per line; the hook-test-helpers env-merge is correct and in fact required for forced-mode children; quality / typecheck / lint / format + packaged install smoke all green. The preference chain (global → pnpm dlx → pinned npx) is a sound design.

CI: CI Gate + tests (ubuntu/macos/windows) failing (the P0 above); all other checks pass.

Coverage limit: the lanes and I read the changed-path suites and the new module; the full ~2000-test suite was not run, so an unrelated flake elsewhere isn't excluded — but every test touching the changed code is accounted for, and the 3 failures are the only PR-caused CI failures.


Automated multi-tool digest (two Claude methods; Codex unavailable). Verify each item before acting.

Comment thread gitnexus/hooks/antigravity/gitnexus-antigravity-hook.cjs
Comment thread gitnexus/src/cli/index.ts Outdated
Comment thread gitnexus-claude-plugin/hooks/resolve-analyze-cmd.cjs
magyargergo and others added 8 commits May 31, 2026 09:33
…arden load checks

The antigravity adapter gained a top-level require('./resolve-analyze-cmd.cjs')
but stageAdapter() did not copy it, so the spawned adapter crashed with
MODULE_NOT_FOUND. Three load-sensitive tests failed; four silent-path tests
false-passed on empty stdout.

Stage the helper alongside the other sibling helpers, and assert status===0 and
no MODULE_NOT_FOUND on the four silent-path tests so a non-loading hook can never
pass green again. Force a deterministic invocation mode in the stale-index test
so the emitted analyze command no longer varies by CI-runner PATH.

Co-authored-by: Cursor <cursoragent@cursor.com>
…rce CJS helper

NPX_REF becomes a literal `gitnexus@latest` in resolve-invocation.ts, dropping
the package.json require and the module-load throw (a malformed/absent version
can no longer crash any CLI command at import). The safety this PR delivers is
the install method steered to (global / pnpm dlx), not a pinned gitnexus
version, and the in-repo CJS mirror already degraded to `latest` once copied
outside the package.

Make the two resolve-analyze-cmd.cjs copies byte-identical and add a parity
test that fails on drift. The separate, version-pinned NPX_REF that setup.ts
writes into the MCP server registration is intentional and left unchanged.

Co-authored-by: Cursor <cursoragent@cursor.com>
…n mode

warnIfNpm11NpxRisk() ran at index.ts module load, so every CLI invocation
(including the `gitnexus mcp` stdio hot path) paid which/where + npm --version
spawns — against the lazy-startup/MCP-stdout discipline (#207, #1383). Move the
call into analyzeCommand, after the ensureHeap() re-exec guard, so it fires once
in the working process and only for `analyze`.

Memoize the PATH-probe-derived invocation mode (the GITNEXUS_INVOCATION override
stays uncached) so repeated callers don't re-probe, and add a test-only reset so
the cache + once-only warning flag don't leak across the unit suite. Covers the
mode!=='npx', npm<11, and npm-absent suppression branches.

Co-authored-by: Cursor <cursoragent@cursor.com>
The winGitnexusWrapper branch only matched .cmd/.bat, so a global gitnexus
installed by Volta or scoop (a .exe or an extensionless shim) was missed and the
hint fell back to pnpm/npx. Accept .exe and treat any non-empty `where` hit as
on-PATH (the emitted hint is `gitnexus analyze` regardless of which shim
resolves it). Mirror the change into both resolve-analyze-cmd.cjs copies so the
TS source and the byte-identical hook mirrors stay in sync.

Add Windows-mocked test cases (.exe-only, extensionless, .cmd preference, CRLF
stripping) and register resolve-invocation.test.ts in cross-platform-tests.ts so
the windows-latest runner exercises the branch.

Co-authored-by: Cursor <cursoragent@cursor.com>
…CLAUDE.md

ai-context baked a machine-resolved command (formatAnalyzeCommand) into
git-tracked AGENTS.md/CLAUDE.md, so the stale-index hint varied per machine and
churned across branches (the #1706 class). Emit the fixed string
`pnpm dlx gitnexus@latest analyze` instead: committed AI-context is the most
authoritative instruction an agent reads, so it must name an install-free,
crash-free method — never `npx`, the npm-11 path #1939 steers away from.

formatAnalyzeCommand stays exported and unit-tested in resolve-invocation.ts
(it still mirrors the two .cjs hook copies); ai-context just no longer calls it.

Co-authored-by: Cursor <cursoragent@cursor.com>
installClaudeCodeHooks copied its four hook helpers in separate try/catch blocks
that silently swallowed failures, while installAntigravityHooks recorded an
error per failed copy. Extract one copyHookHelpers(srcDir, destDir, label,
result) with a single canonical helper list (including resolve-analyze-cmd.cjs)
and the antigravity loop's error-reporting policy, and use it from both paths so
a missing helper surfaces as a setup error instead of a silent runtime crash.

Assert both the Claude and Antigravity install paths co-locate
resolve-analyze-cmd.cjs next to the adapter, and that a failed copy records an
error rather than passing silently.

Co-authored-by: Cursor <cursoragent@cursor.com>
The extracted HOOK_HELPERS/copyHookHelpers block landed between the
installClaudeCodeHooks JSDoc and its function, leaving the doc reading as if it
described the helper list. Move the block above the doc so it documents the
function again. No behavior change.

Co-authored-by: Cursor <cursoragent@cursor.com>
…osture

Tier-2 review found two in-scope gaps in the #1945 follow-up:

- The "mirrors resolve-invocation.ts / test enforces parity" comments overclaimed:
  the parity test only compared the two .cjs copies to each other, so the TS
  source and the CJS hook copies could silently drift (NPX_REF, the per-mode
  command, and the Windows shim regex were hand-edited in all three this PR).
  Add TS<->CJS value parity (NPX_REF + formatAnalyzeCommand for every forced
  mode) and a source-level shim-regex parity check, and make the mirror comments
  accurately describe what is enforced.

- No test locked the R3/R4 startup posture, so re-adding warnIfNpm11NpxRisk()
  (or any resolve-invocation import) at index.ts module scope -- the #207/#1383
  lazy-startup regression -- would pass CI. Add a guard asserting index.ts has
  no module-load invocation probe and the warning is wired into analyzeCommand.

Co-authored-by: Cursor <cursoragent@cursor.com>
PR #1945 carried the gitnexus/pnpm/npx selection in three hand-synced
places — the canonical hook helper, its byte-identical plugin copy, and a
full TypeScript re-implementation in resolve-invocation.ts — kept in lockstep
by per-mode-command and regex-extracted-by-regex parity tests. The TS
formatAnalyzeCommand had no production caller (ai-context emits a fixed
string), and the module memoized + exposed a test-only reset for a "repeated
callers" case that has exactly one caller.

Make hooks/claude/resolve-analyze-cmd.cjs the single source: extract the
Windows-shim line-picking into a pure, exported pickPathMatch() and add an
injectable probe to resolveInvocationMode() so the shipped logic is testable
without spawning or global mocks. resolve-invocation.ts (118 -> 59 lines) now
consumes that cjs via createRequire for resolveInvocationMode/NPX_REF and adds
only the CLI-only npm-version probe and warning; the relative path resolves
identically from src/cli/ (tsx, vitest) and dist/cli/ (shipped, hooks/ is a
published sibling of dist/). Tests exercise the real shipped artifact, the
NPX_REF/mode-command parity scaffolding is dropped (one implementation can't
drift), and parity narrows to the two cjs copies staying byte-identical.

No behavior change: hook stale-index hints and the analyze warning are
byte-identical; the pre-existing setup.ts resolveGitnexusBin is untouched.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator Author

@magyargergo magyargergo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Tri-Review — #1945 fix(cli): steer npm 11 users away from npx install crash

Final verdict: production-ready with minor follow-ups. No blocking defects. Two P2 follow-ups (both squarely in the PR's own theme) plus minor test/doc nits. Branch hygiene: clean feature/fix branch. Merge state: CI green on all 3 OS — BLOCKED only on required-review approval.

Methods & engines (read first). Genuine three-method review: Codex (the one independent engine — live this run via the GitHub connector), the GitNexus swarm (risk-architect, security-boundary; the test/CI lane ended mid-investigation, its domain covered by the testing reviewer), and Compound-Engineering personas (correctness, adversarial, maintainability, testing). 2 of the 3 methods are Claude under different prompts, so Claude-only agreement is "consistent across personas," not independent confirmation — only Codex+Claude agreement is weighted strong.

What's solid (validated, not assumed):

  • The refactor's riskiest move — createRequire('../../hooks/claude/resolve-analyze-cmd.cjs') from the CLI (resolve-invocation.ts) — resolves in the published package, confirmed by Codex + 3 Claude lanes: package.json files:["dist","hooks"], bin:"dist/cli/index.js", and ../../ is identical from src/cli/ and dist/cli/.
  • No injection / trust-boundary issue: execFileSync uses fixed argv (no shell), GITNEXUS_INVOCATION is allowlisted, the warning interpolates only a parsed int.
  • The two resolve-analyze-cmd.cjs copies (gitnexus/hooks/claude/ and gitnexus-claude-plugin/hooks/) are byte-identical (Codex: same git blob SHA; enforced by the parity test in gitnexus/test/unit/resolve-invocation.test.ts).
  • The prior review's two P0s are confirmed fixed: the antigravity-hook MODULE_NOT_FOUND (helper now in HOOK_HELPERS) and the index.ts module-load PATH probe (warning moved into analyzeCommand; posture-guard test added).

Inline findings (3):

  1. [P2] Generated guidance still funnels group repos into npx. ai-context.ts:170 (Cross-Repo Groups block) emits npx gitnexus group list/sync/impact, untouched by this PR — so a group-enabled repo's generated AGENTS.md/CLAUDE.md still steers npm-11 users into the exact arborist crash this PR fixes. (Codex — independent engine.)
  2. [P2] Stale-index hook now spawns which/where under a 10s budget. The PostToolUse hint was a pure string (0 spawns); it now calls formatAnalyzeCommand() → 1–2 PATH probes (timeout:5000ms each) on top of git rev-parse (3s), under the hook's timeout:10s. Worst case ≈ 3+5+5 = 13s > 10s → the nudge is killed mid-run. (risk + adversarial.)
  3. [P3] Dead NPX_REF/PKG_VERSION + loose version regex in the e2e tests. Both unused; the cjs always emits @latest, so toContain('npx gitnexus@latest analyze') is exact and stronger than /@\S+/. (maintainability + testing + risk — 4 lanes.)

Lower-priority (body only):

  • [P2, pre-existing] Partial-install crash: copyHookHelpers (setup.ts:334-363) records a copy failure but the caller still registers the hook → deferred MODULE_NOT_FOUND on first fire. Corroborated by Codex + 3 Claude lanes; already listed as a Known Residual in the PR body — confirming it's real, and the new required resolve-analyze-cmd.cjs widens the blast radius by one file. (Pre-existing class; not introduced here.)
  • Design tradeoff: the fixed pnpm dlx gitnexus@latest analyze in generated docs (ai-context.ts:133) strands users without pnpm (no npx fallback shown). Deliberate per #1706, but consider a fallback hint. Relatedly, this repo's own committed CLAUDE.md:61/AGENTS.md:80 still say npx gitnexus analyze — regenerate to dogfood the fix.
  • Test gaps: warnIfNpm11NpxRisk gitnexus-mode branch not exercised (test only sets pnpm); getNpmMajorVersion edge inputs (empty / pre-release) untested; pickPathMatch(isWin:true, gitnexusWrapper:false) (Windows pnpm probe) untested; some e2e tests don't pin GITNEXUS_INVOCATION (host-dependent); copyHookHelpers error-count assertion hardcodes 4.
  • Nits: InvocationResolver TS interface omits the probe param; gitnexus/README.md:279 pnpm dlx gitnexus missing @latest; NPX_REF name covers all three modes, not just npx.

Refuted / not bugs: memoization removal is not a regression (only one caller, analyze.ts:624); the .ps1-only Windows shim edge is unrealistic (npm co-installs a .cmd); the createRequire fail-closed concern doesn't trigger in a correctly-published package.

Coverage: all 18 changed files reviewed; CLI/cjs/test source read in full, golden/fixture blobs skipped. CI green across ubuntu/macos/windows tests, packaged-install smoke, lint/format/typecheck.

Automated multi-tool digest — verify before acting. Two of three methods are the same engine (Claude); only Codex is independent.

Comment thread gitnexus/src/cli/ai-context.ts Outdated
Comment thread gitnexus/hooks/claude/gitnexus-hook.cjs
Comment thread gitnexus/test/integration/hooks-e2e.test.ts Outdated
magyargergo and others added 5 commits May 31, 2026 12:46
The PostToolUse stale-index hint calls formatAnalyzeCommand(), which probes which/where; named PROBE_TIMEOUT_MS=2000 keeps git rev-parse (~3s) + up to two probes well under Claude Code's 10s hook timeout while preserving the machine-correct hint. Byte-identical in the plugin copy.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Cross-Repo Groups block in generated AGENTS.md/CLAUDE.md still emitted bare 'npx gitnexus group ...', funneling npm-11 users into the arborist crash; switch to fixed 'pnpm dlx gitnexus@latest group ...'. Export generateGitNexusContent and add a group-branch test asserting no 'npx gitnexus' literal survives.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
README troubleshooting uses gitnexus@latest; the repo's own committed CLAUDE.md/AGENTS.md stale-index hint now matches the generated output (pnpm dlx gitnexus@latest analyze) so the repo dogfoods the fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mode (U4)

Drop dead PKG_VERSION/NPX_REF version-pinned constants; the cjs always emits gitnexus@latest, so assert exact toContain(...) instead of the /@\\S+/ wildcard; pin GITNEXUS_INVOCATION in the --embeddings tests for host-independent determinism.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add coverage for the gitnexus-mode warn suppression, getNpmMajorVersion edge inputs (empty/pre-release/non-numeric), and the Windows non-wrapper pickPathMatch branch; widen the InvocationResolver interface to document the optional probe param.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
magyargergo and others added 2 commits May 31, 2026 22:03
The stale-index hook resolved pnpm twice — `which pnpm` for mode selection
then `pnpm --version` for the allow-build gate — two spawns for one tool in a
~9s/10s budget. Capture the version once in formatAnalyzeCommand and thread it
through the existing deps seam (a successful `pnpm --version` proves presence),
sharing a memoized PATH probe with resolveInvocationMode. Add explicit pnpm
10.0-suppress / 10.2-emit boundary tests and relabel the unknown-minor case.
Both byte-identical cjs copies updated together.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…plied (#1939)

The hook `command` written into editor settings is shell-evaluated; the
double-quoted `node "<path>"` form left `$`, backtick, and other metacharacters
live in an adversarial $HOME. Single-quote the path on POSIX (Windows keeps the
double-quoted form — those chars are illegal in Windows filenames). Also assert
the cliPath source-literal replace() actually matched, recording an actionable
error on drift instead of silently shipping a hook with an unresolved relative
path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The new POSIX-escaping test built its expected hook path with path.join,
which emits backslashes on the Windows runner, while setup.ts forward-slash-
normalizes the path before quoting — so `expect(cmd).toBe(node '<path>')`
mismatched on tests/windows-latest. Normalize the expected path the same way.
Production code was already correct; only the test's expected value was
platform-fragile.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fault (#1939)

The prior approach hardcoded `pnpm --allow-build=… dlx gitnexus@latest <cmd>`
into every committed skill + the generated AGENTS.md/CLAUDE.md, which assumes
pnpm is installed. Replace it with a CLI-neutral project-local runner:

- `gitnexus analyze` drops `.gitnexus/run.cjs` (a copy of the canonical
  `resolve-analyze-cmd.cjs`, which gains `buildRunnerArgv` + a `require.main`
  exec tail) next to the index. Docs/skills reference `node .gitnexus/run.cjs
  <cmd>`, which auto-selects the runner (global `gitnexus` → `pnpm dlx` → `npx`)
  at call time — no package-manager assumption. README first-run + an inline
  bootstrap note stay universal `npx gitnexus analyze`.
- The exec tail uses `shell` on Windows so `.cmd`/`.ps1`/`.exe` shims resolve
  (execFileSync can't otherwise; Node blocks `.cmd` without a shell,
  CVE-2024-27980), and prints a diagnostic instead of a silent exit 1.

Tests: runner exec-tail (real spawn, exit-code propagation + ENOENT diagnostic),
copy-failure graceful degradation, and per-subcommand routing + pnpm-fallback
vacuity guards. The generated CLAUDE.md block stays under the #856 token budget.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@magyargergo magyargergo changed the title fix(cli): steer npm 11 users away from npx install crash (#1939) fix(cli): steer docs/skills via a project-local runner, no pnpm default (#1939) Jun 1, 2026
Copy link
Copy Markdown
Collaborator Author

@magyargergo magyargergo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Tri-Review — #1945 fix(cli): steer docs/skills via a project-local runner, no pnpm default @ 402f80b0

⚠️ This is the 5th tri-review of this PR and supersedes the four earlier ones (#1–4, all 2026-05-31). The design pivoted after the last review: it dropped the "pnpm-by-default" approach for a project-local node .gitnexus/run.cjs runner, and the prior P0s (pnpm --allow-build placed after dlx; a failing test) are resolved on this HEAD. This review covers the new runner surface, which no prior review saw.

Final verdict: production-ready with minor follow-ups. No merge-blocking regression vs main on this HEAD. The headline item is a Windows-only gap in the new mitigation (not a new crash): the pnpm-steering silently no-ops on the Windows stale-index hook path and falls back to npx — which equals the pre-PR behavior, so it makes nothing worse, but it means #1939's core fix doesn't reach Windows hook users. Branch hygiene: clean feature/fix branch — MERGEABLE, with routine merge-from-main commits that are harmless and merge-safe. Merge state: BLOCKED on required review (CI not failing). CI: all checks pending on the just-pushed 402f80b0 (nothing failing yet).

Methods & engines (read first). Genuine three-method review: Codex (gpt-5.5, the one independent engine — live this run) + GitNexus swarm + Compound-Engineering personas. Six Claude lanes returned full findings (security-boundary, correctness, adversarial, maintainability, performance, testing); two (risk-architect, test/CI-verifier) ended mid-investigation and their domains were covered by the others. Independence is asymmetric: five of the six lanes are Claude under different prompts (agreement = "consistent across personas," not independent confirmation), so the strong signal here is Codex + a Claude lane agreeing — which is exactly what produced the headline.

Headline (P2 — Windows-only; Codex + adversarial, cross-engine): the pnpm steering can't fire on Windows because the version probes can't run .cmd shims

probeVersion() (resolve-analyze-cmd.cjs:84) spawns npm/pnpm --version via execFileSync(command, ['--version']) without shell:true. On Windows npm/pnpm/npx are .cmd shims and execFileSync does no PATHEXT resolution (the PR's own comment at resolve-analyze-cmd.cjs:253 says exactly this, which is why the exec tail uses shell:win32) — so those probes throw and the catch (lines 97-99) swallows it, returning {major:null, minor:null}. Consequences on Windows + npm 11 + pnpm present, no global gitnexus:

  • The stale-index hook hint injects pnpmMajor:null; resolveInvocationMode's injected-deps branch treats pnpmMajor === null as pnpm absent (deps.pnpmMajor !== null → false), npmMajor is also null, so it falls through to return 'npx' — the hook recommends npx gitnexus@latest analyze, the exact #1939 crash command.
  • The node .gitnexus/run.cjs runner still works (it detects pnpm via where.exe and execs with shell:true), but its --allow-build version gate is defeated (version parse → null → flags always emitted), so a Windows user on pnpm < 10.2 gets --allow-build flags their pnpm rejects.

This is not a regression (the pre-PR hook hardcoded npx gitnexus analyze unconditionally — verified at BASE), and the committed docs (which use the non-probing formatDocumentationDlxCommand) are unaffected. But the PR's headline npm-11→pnpm steering is a no-op on the Windows hook path and the runner's pnpm-version gate is wrong on Windows. Fix: add shell: process.platform === 'win32' to the --version probes (mirroring the exec tail), and/or separate "tool present on PATH" from "version parsed" so a failed parse doesn't hide a present pnpm. Codex (independent) and the adversarial lane reached this independently; I verified the mechanism by code-read against the file's own PATHEXT comment. [code-read]

Inline findings

  • [P2] cross-platform-tests.ts:33runner-exec-tail.test.ts was not registered in any cross-platform category (only resolve-invocation.test.ts was added). The runner test is POSIX-only (it.skipIf(!onPosix)) and its comment claims "exercised by CI's windows-latest job" — it isn't, so the Windows shell:true exec branch and the Windows probe-failure path above are untested on every platform. [code-read]
  • [P3] gitnexus/README.md:33 — the new "See Troubleshooting →" link targets #npx-gitnexus-crashes-with-nodetarget-is-null-npm-11, but the actual heading slug is #cannot-destructure-property-package-of-nodetarget-as-it-is-null. Broken in-page anchor. [code-read]

Lower-priority / body-only

  • [P2-latent · Codex P0 ↔ security P3] shell:true + verbatim process.argv in the run.cjs exec tail (resolve-analyze-cmd.cjs:257). Both engines agree on the mechanism (on Windows, cmd.exe interprets & | ^ in forwarded args); they disagree on severity. For the PR's actual usage it's safe — the generated docs/skills emit only fixed subcommands (analyze, status, group list) with no untrusted interpolation, so the only trigger is a user typing metacharacters into their own shell (self-injection, no privilege gain). It becomes a real injection surface only if a future caller forwards untrusted input on Windows. Recommend not pairing shell:true with verbatim argv (resolve the .cmd shim explicitly, or use cross-spawn). [code-read]
  • [P3] --embeddings=N equals-form drops onnxruntime-node allow-build (resolve-analyze-cmd.cjs:213): gitnexusArgs.includes('--embeddings') misses the equals form Commander also accepts, so node .gitnexus/run.cjs analyze --embeddings=5000 omits --allow-build=onnxruntime-node on pnpm 10.2+. Docs/hooks emit the space form, so impact is narrow. [code-read]
  • [P3] antigravity-hook-e2e beforeAll sanity loop (antigravity-hook-e2e.test.ts:66-71, unchanged → not inlined) checks for hook-lock/hook-db-lock-probe/win-rm-list-json but not the newly-required resolve-analyze-cmd.cjs. The test still exercises the require indirectly (a missing helper would MODULE_NOT_FOUND at spawn), so coverage isn't truly lost — just add it to the guard list.
  • [P3] runner-absent → MODULE_NOT_FOUND (adversarial): generated docs make node .gitnexus/run.cjs analyze the primary command, but .gitnexus/ is gitignored (*), so on a fresh clone / git clean / failed copy the agent hits a raw loader stack trace. A bootstrap fallback ("No .gitnexus/run.cjs yet? npx gitnexus analyze") is present inline in the same doc note, so it's recoverable; consider a friendlier missing-runner message.
  • Minor (maintainability/perf): dead probe ?? resolveOnPath (default param already guarantees non-undefined); two same-named NPX_REF constants with different values (@latest in the cjs vs @version in setup.ts); the createRequire cast under-types the cjs exports; header comment overcounts probes (says 4, actually 3 → ~8s worst case, not ~9s); double npm --version spawn in warnIfNpm11NpxRisk (one-shot CLI path, negligible); setup.ts is now ~1041 lines (consider extracting the hook-helper concern).

Validated / refuted (validation is a feature)

  • POSIX hook-command escaping is correct — the '\'' single-quote idiom neutralizes $/backtick/;; the prior double-quoted form's $/backtick risk is genuinely fixed. (Codex + security-boundary agree — cross-engine.)
  • No startup-crash from the top-level createRequirehooks/ is in package.json files, so ../../hooks/claude/resolve-analyze-cmd.cjs resolves in the shipped package; an export-shape drift guard throws a clear error otherwise. (Verified.)
  • Hook-budget is bounded — probes are each capped at 1s; no unbounded loop introduced. (Codex + performance lane agree.)
  • The node run.cjs direct-exec works on POSIX (args pass through as discrete argv) and on Windows (where.exe presence + shell:true exec); exit codes propagate; missing-runner prints a diagnostic. The prior pnpm --allow-build-after-dlx P0 and the prior failing test are resolved.
  • GITNEXUS_INVOCATION override uses an exact allowlist; unrecognized values fall through to probing safely.
  • 2 committed cjs copies are byte-identical (test-enforced); .gitnexus/run.cjs is gitignored (*), no git-status pollution; storagePath is always <repo>/.gitnexus, so the runner path never escapes the repo.

Coverage note: I read the source substance (the resolver cjs, CLI wiring, setup.ts, the hooks, the READMEs/skills) and the changed test files. I did not separately re-verify each of the ~30 one-line doc/skill edits beyond confirming they route consistently to node .gitnexus/run.cjs / pnpm dlx gitnexus@latest.

Automated multi-tool digest (Codex gpt-5.5 + GitNexus swarm + Compound-Engineering personas). Verify findings before acting; inline anchors are best-effort to in-diff lines.

// / formatPnpmAllowBuildArgs), so this stays a pure real-process probe.
function probeVersion(command) {
try {
const output = execFileSync(command, ['--version'], {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2 · Windows-only · Codex + adversarial, cross-engine] probeVersion spawns npm/pnpm --version here via execFileSync(command, ['--version']) with no shell option. On Windows npm/pnpm/npx are .cmd shims and execFileSync does no PATHEXT resolution — the PR's own comment at line 253 documents exactly this for the exec tail (which is why line 257 uses shell: process.platform === 'win32'). So on Windows these probes throw, the catch (97-99) returns {major:null, minor:null}, and resolveInvocationMode's injected-deps branch reads pnpmMajor === null as pnpm absent (deps.pnpmMajor !== null → false). Result on Windows + npm 11 + pnpm present, no global gitnexus: the stale-index hook recommends npx gitnexus@latest analyze — the exact #1939 crash command — so the PR's headline npm-11→pnpm steering is a no-op on the Windows hook path; and the node .gitnexus/run.cjs runner's --allow-build version gate is defeated (version → null → flags always emitted), breaking pnpm < 10.2 on Windows.

Not a regression (the pre-PR hook hardcoded npx gitnexus analyze unconditionally), and the committed docs use the non-probing formatDocumentationDlxCommand, so they're fine.

Fix: add shell: process.platform === 'win32' to the --version probes (mirror the exec tail), and/or separate "present on PATH" from "version parsed" so a failed parse doesn't hide a present pnpm. [code-read]

'test/unit/setup-jsonc.test.ts',
'test/unit/setup-codex.test.ts',
'test/unit/setup-antigravity.test.ts',
'test/unit/resolve-invocation.test.ts',
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2 · testing + maintainability, consistent] This adds resolve-invocation.test.ts to the cross-platform set, but runner-exec-tail.test.ts was not added to any category here. That test is POSIX-only (it.skipIf(!onPosix)) and its own comment claims the Windows branch is "exercised by CI's windows-latest job" — but since the file isn't registered in cross-platform-tests.ts, the Windows-latest job never runs it. So the node run.cjs Windows shell:true exec branch and the Windows .cmd probe-failure path (see the resolve-analyze-cmd.cjs:84 comment) are untested on every platform.

Fix: add 'test/unit/runner-exec-tail.test.ts' to the appropriate (SPAWN_CLI) array, and add an it.skipIf(onPosix) companion that stages a .cmd fake runner to exercise the Windows shell branch. [code-read]

Comment thread gitnexus/README.md Outdated
magyargergo and others added 7 commits June 2, 2026 07:22
…1939)

probeVersion (and the TS getNpmMajorVersion mirror) spawned npm/pnpm
--version via execFileSync with no shell, so on Windows the .cmd shims
ENOENT'd, the probe reported a present tool as absent, and the stale-index
hook recommended the npx crash path #1939 exists to avoid. Add
shell: process.platform === 'win32' to the version probes (the exec tail
already does this). Parse the first version-shaped line so a Corepack/notice
banner on stdout no longer defeats the parse. Carry pnpm presence separately
from version so a present-but-unparseable pnpm still selects pnpm. Drop the
dead probe ?? resolveOnPath coalesce. Cover resolve-analyze-cmd.cjs (+ plugin
twin) with the shell-injection and windowsHide source-regression guards.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…1945)

buildRunnerArgv detected embeddings via gitnexusArgs.includes('--embeddings'),
which missed the equals form (--embeddings=5000) that Commander also accepts,
dropping --allow-build=onnxruntime-node on pnpm 10.2+. Match both forms.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
runner-exec-tail.test.ts was POSIX-only and unregistered in
cross-platform-tests.ts, so the run.cjs Windows shell:true exec branch ran on
no platform despite the file comment claiming windows-latest covered it. Add a
.cmd-shim it.skipIf(onPosix) case and register the file in SPAWN_CLI so the
windows-latest job runs it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The npm-11 quick-start note linked to #npx-gitnexus-crashes-with-nodetarget-is-null-npm-11,
which matches no heading; the actual troubleshooting heading slugifies to
#cannot-destructure-property-package-of-nodetarget-as-it-is-null. Repoint the link.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…check (#1945)

The antigravity adapter top-level require()s resolve-analyze-cmd.cjs, but the
beforeAll helper-presence loop did not check for it — a failed copy would
surface as noisy MODULE_NOT_FOUND in downstream tests instead of the intended
actionable 'Helper not installed' error. Add it to the loop.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ry (#1945)

Generated CLAUDE.md/AGENTS.md make `node .gitnexus/run.cjs` the primary
command, but the runner is gitignored, so a fresh clone or git clean leaves an
agent facing a raw MODULE_NOT_FOUND. The CLAUDE.md block is token-budget-capped
(#856), so the recovery guidance lives in the cli skill (its documented home):
the bootstrap note now names the `Cannot find module` error and points at
`npx gitnexus analyze` to (re)generate the runner.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…1945)

setup.ts and resolve-analyze-cmd.cjs both exported a constant named NPX_REF
with different values (version-pinned for the persisted MCP entry vs.
gitnexus@latest for hints). Rename setup.ts's module-private constant to
MCP_PINNED_REF (value and behavior unchanged — the MCP pin stays pinned),
leaving the cjs hint ref and its re-export alone. Also route the createRequire
cast through 'unknown' so it reads as an explicit narrowing to the subset this
module uses rather than a claim about the cjs's full export shape.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@magyargergo magyargergo changed the title fix(cli): steer docs/skills via a project-local runner, no pnpm default (#1939) fix(cli): steer docs, skills, and hooks through a CLI-neutral project-local runner (#1939) Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow-up to #819: npx gitnexus still crashes with "node.target is null" on npm 11.x — clean cache doesn't help

2 participants