Skip to content

fix(hooks): prefer PATH gitnexus in stale hint#1958

Closed
sainiranjannallam wants to merge 2 commits into
abhigyanpatwari:mainfrom
sainiranjannallam:fix/hook-path-analyze-command
Closed

fix(hooks): prefer PATH gitnexus in stale hint#1958
sainiranjannallam wants to merge 2 commits into
abhigyanpatwari:mainfrom
sainiranjannallam:fix/hook-path-analyze-command

Conversation

@sainiranjannallam

Copy link
Copy Markdown

Fixes #1938.

Summary

  • Prefer gitnexus analyze in stale-index hints when a spawnable gitnexus launcher is on PATH, including Windows launcher suffixes.
  • Preserve the existing npx gitnexus analyze fallback when no PATH launcher exists.
  • Apply the behavior to the Claude hook, Claude plugin hook, and Antigravity hook adapter.

Validation

  • npx vitest run test/integration/hooks-e2e.test.ts test/integration/antigravity-hook-e2e.test.ts
  • npx tsc --noEmit
  • npx --yes gitnexus detect-changes

@vercel

vercel Bot commented Jun 1, 2026

Copy link
Copy Markdown

@sainiranjannallam is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

@sainiranjannallam sainiranjannallam marked this pull request as ready for review June 1, 2026 06:13

@magyargergo magyargergo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Reviewed with 7 lanes across 2 engines: GitNexus risk + test/CI, Compound Engineering (correctness, adversarial, maintainability, testing), and Codex (independent). Claude lanes share one engine; strong corroboration requires Codex + Claude agreement — here, Codex and multiple Claude lanes agree on Windows launcher policy drift.

Headline: This is a solid fix for #1938 — stale-index hints now prefer gitnexus analyze when a launcher is on PATH, with good e2e coverage and PATH isolation for the npx fallback. Before merge, align Windows launcher detection with the existing setup.ts policy (.cmd/.bat wrappers, not .ps1-only shims) and dedupe the plugin hook’s two PATH probes.

What looks good

  • Pure-Node PATH scan avoids Windows which failures (#1938); Unix adds X_OK.
  • Tests scrub ambient PATH (pathWithoutGitNexus) and add a synthetic launcher case — fixes flake when gitnexus is on the runner PATH.
  • Covers CJS, plugin, and antigravity hook surfaces.
  • Coordinator ran npx vitest run test/integration/hooks-e2e.test.ts test/integration/antigravity-hook-e2e.test.ts -t "gitnexus analyze"3/3 passed in the PR worktree.

Inline findings

  1. P2.ps1-only installs treated as on-PATH (all three hook copies).
  2. P2 — Plugin hook uses two different PATH probes for hint vs CLI spawn.

Lower priority / follow-ups

  • P3: ~42 lines duplicated across gitnexus/hooks/claude/gitnexus-hook.cjs, gitnexus/hooks/antigravity/gitnexus-antigravity-hook.cjs, and gitnexus-claude-plugin/hooks/gitnexus-hook.js — extract shared helper (same pattern as hook-lock.cjs).
  • P2 test gap: No e2e for PATH-on and stats.embeddings > 0 → expect `gitnexus analyze --embeddings` (hooks-e2e.test.ts ~96–129, antigravity-hook-e2e.test.ts ~132–161).
  • P3: pathWithoutGitNexus() uses existsSync while hooks require isFile() + X_OK (hook-test-helpers.ts:48–57).

Refuted / not blocking

  • GITNEXUS_HOOK_CLI_PATH hint mismatch: Pre-existing — stale hint was always npx gitnexus analyze before this PR; resolveCliPath() already handled augment separately (gitnexus/hooks/claude/gitnexus-hook.cjs:194–208). Optional follow-up, not a regression.
  • UNC PATH hang: Theoretical enterprise edge case; statSync is wrapped in try/catch per candidate. Not reproduced; not a merge blocker.

CI / visibility

  • gh pr checks shows only Vercel FAILURE (authorization required — unrelated to hooks).
  • GitHub Actions status was not visible via the Checks API for this fork PR; treat CI green as unconfirmed from API evidence.

Hygiene

  • Branch: fix/hook-path-analyze-command — one feature commit + merge-from-main (harmless).
  • git diff --check and bidi control scan: clean.

Automated multi-tool digest (pr-tri-review). Verify findings before acting.

'.cmd',
'.bat',
'.exe',
'.ps1',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

P2 [code-read].ps1 is treated as a positive PATH hit, but setup.ts resolveGitnexusBin() explicitly rejects .ps1-only installs and falls back to npx (setup.test.ts:302). On Windows npm-11 layouts with only gitnexus.ps1, this hint says `gitnexus analyze` while MCP setup still uses npx.

Fix: On win32, accept .cmd|.bat|.exe only (mirror setup.ts:69); drop .ps1 from positive detection. Same block is copy-pasted at gitnexus-antigravity-hook.cjs:222 and gitnexus-claude-plugin/hooks/gitnexus-hook.js:200.

return false;
}

function buildAnalyzeCommand(hadEmbeddings) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

P2 [code-read] — This file now has two PATH strategies: buildAnalyzeCommand uses the new fs PATH walk (L188–227), while runGitNexusCli still uses where/which + hardcoded gitnexus.cmd (L250–271). They can disagree on edge cases (PATHEXT, non-spawnable shims).

Fix: Share one launcher-resolution helper for both the stale hint and CLI spawn paths.

@magyargergo

Copy link
Copy Markdown
Collaborator

Thanks for tackling #1938, @sainiranjannallam — the diagnosis and the PATH-isolation test work here are genuinely good. This has been overtaken by events, though: #1938 was already fixed on main by #1945 (#1939), which merged after this PR's last sync from main (hence the current conflicts).

#1945 centralized invocation selection into hooks/claude/resolve-analyze-cmd.cjsformatAnalyzeCommand(), which all three hooks call. Its resolveInvocationMode() returns gitnexus as step 1 when a launcher is on PATH, so the hint already emits gitnexus analyze. Running the hooks under controlled PATH:

Scenario main this PR
global gitnexus on PATH gitnexus analyze gitnexus analyze — identical
npm 11 + pnpm, no global gitnexus (the #1938 repro box) pnpm …dlx gitnexus@latest analyze npx gitnexus analyze

The second row is the blocker: buildAnalyzeCommand here has no npm/pnpm awareness and falls back to bare npx gitnexus — the npm-11 arborist "node.target is null" crash path #1938 itself calls "actively harmful." Merging would regress the mitigation #1945 shipped, for the exact environment that filed the issue. It also treats .ps1-only shims (un-spawnable without a shell) as on-PATH, which main's where/PATHEXT matching avoids.

Closing as superseded by #1945. Your work isn't lost: the one genuinely new idea — a pure-Node PATH scan for when which/where itself is unreachable (your "Option A") — plus the spawn-free e2e test are carried forward in #1980, layered on top of the centralized resolver instead of replacing it. Thanks again 🙏

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 #1660: PostToolUse hook still hardcodes npx gitnexus in 1.6.5 — proposed fix never shipped

2 participants