From 4f9d595c7362036ebcc890c287f54d4335a1fcc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Wed, 10 Jun 2026 08:38:37 +0100 Subject: [PATCH 1/2] fix(docker): ship runtime-needed published assets (hooks/, skills/) into the image (#2130) (#2132) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(docker): copy hooks/ into Dockerfile.cli runtime stage (#2130) `gitnexus analyze` inside the official image (akonlabs/gitnexus, ghcr.io/abhigyanpatwari/gitnexus) crashed at startup with: Error: Cannot find module '../../hooks/claude/resolve-analyze-cmd.cjs' Require stack: - /app/gitnexus/dist/cli/resolve-invocation.js `dist/cli/resolve-invocation.js` does `createRequire(import.meta.url)('../../hooks/claude/resolve-analyze-cmd.cjs')` at module load (it is the single source of truth for the npm-11 npx-crash invocation decision, #1939), and `analyze.ts` statically imports it. The Dockerfile.cli runtime stage copied dist/node_modules/package.json/the duckdb script/vendor but never `hooks/`, so the require throws before the command does any work. `hooks/` is in package.json `files`, so npm already ships it — Docker was the only distribution dropping it. Fix: copy `hooks/` into the runtime stage, mirroring what npm publishes. Also add `test/unit/dockerfile-runtime-asset-parity.test.ts`: a regression guard that derives every out-of-dist `require()`/`createRequire()` target from source and asserts each is a runtime-stage `COPY`. Scoped to the require family (not `fs.access`/`new URL`), so it locks the #2130 class without false-flagging the intentionally-omitted, gracefully-degrading `web/` and `skills/` assets. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(docker): also ship skills/ into the runtime image Follow-up to the hooks/ fix: `skills/` is another published runtime asset (in package.json `files`) the Docker image dropped. The CLI reads the bundled SKILL.md templates from `/skills/` for `gitnexus analyze --skills` (ai-context skill generation) and `gitnexus setup`/`uninstall` (installing skills into editor configs). Unlike the hooks/ require(), these reads degrade SILENTLY when the dir is absent — `--skills` writes minimal placeholder content (ai-context.ts), `setup` installs zero skills (setup.ts readdir → []) — so the image looked fine but produced wrong output. Copy `skills/` so the image is fully usable for all CLI tooling. `web/` (also in `files`) is intentionally NOT shipped: this image never builds gitnexus-web (the builder doesn't copy it, build.js logs "skipping web UI"), so it is API-only by design — the UI is the separate Dockerfile.web image / hosted app. The duckdb script is the only runtime asset needed from scripts/, so that stays a single-file copy. Extends the runtime-asset-parity guard with an explicit skills/ assertion. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(test): correct stale docstring that listed skills/ as not copied The 2nd commit on this branch added a skills/ COPY + an it('copies skills/…') assertion, but the top-of-file docstring still grouped skills/ with web/ as 'intentionally not copied / out of scope'. Drop skills/ from that sentence and note it is shipped (and covered by its own test). web/ remains the sole fs-accessed-but-uncopied example. Documentation-only; assertions unchanged. * fix(test): make runtime-stage detection case-insensitive on AS Docker accepts a lowercase `as runtime`; the parity guard's stage-detection regex was case-sensitive on `AS`, so a future Dockerfile reformat would empty the parsed COPY set and trip the named assertions. Add the /i flag. * fix(test): stop runtime-stage COPY parsing at the next FROM runtimeStageCopiedSources scanned from the runtime FROM to EOF. Bound the scan to the runtime stage (start after its FROM, break on the next FROM) so a build stage added after runtime can't have its COPY lines misattributed. No-op today (runtime is the last stage); the copied set is unchanged. * fix(test): assert at least one runtime COPY is parsed (no vacuous pass) If the runtime FROM or the /app/gitnexus/ source prefix ever stops matching, the copied set goes empty and the parity assertion passes vacuously. Add an explicit copied.length>0 guard so that failure mode is loud and named. * fix(test): strip line comments before require-scanning requiredExternalAssets() regex-scanned raw source, so a future doc-comment such as a commented-out require('../../web/x') in a shallow src file would resolve outside dist/ and spuriously fail the parity guard. Strip // line comments first. Block comments are deliberately not stripped (a naive block strip mangles slash-star inside string/glob literals). Verified the real-tree scanner output is byte-identical with and without the strip, and resolve-invocation.ts's multi-line createRequire is still detected. (Also swaps a stray non-ASCII glyph in the prior commit's comment for ASCII.) * fix(test): account for aliased + computed module-load requires (fail-closed) The parity scanner only matched string-literal require/createRequire, so it missed module-load requires via aliased createRequire bindings and computed paths — and already failed to see community-processor.ts's `_require(leidenPath)` -> vendor/leiden, making the "every out-of-dist asset" claim untrue. Broaden the scan: - Discover per-file createRequire bindings (requireCJS, _require, …) and match their literal-arg calls; keep the createRequire(...)('…') IIFE form. - Detect COMPUTED (non-literal) requires and gate them on MODULE-LOAD position (brace-depth 0), so the four in-function computed requires that target node_modules/package.json (optional-grammars, native-check, capabilities, parse-cache) are correctly out of charter and ignored. A module-load computed require must be vetted in KNOWN_COMPUTED_REQUIRES (seed: community-processor -> vendor/leiden) or the test FAILS CLOSED for manual review. - Allowlist entries are coverage-checked via isCovered, never trusted: a new test removes the `vendor` COPY from a fixture and asserts leiden surfaces as uncovered (so deleting a COPY can't silently pass — the #2130 class). - Exclude `.resolve(...)` (a path lookup, not a load). - Upgrade the comment stripper to a string-aware pass that removes line AND block comments without mangling slash-star inside string/glob literals — the computed branch needs JSDoc requires (e.g. javascript/index.ts) gone, and the literal scan output stays byte-identical. Honest claim wording: the 4th test now says coverage = resolvable + vetted module-load requires, unrecognized computed requires fail for review. Adds unit tests for fail-closed, aliased-literal, and in-function-ignored paths. * fix(test): also scan shipped .cjs/.mjs assets for sibling requires The guard only scanned src/**/*.ts, so hand-written shipped runtime files were invisible — and they DO require siblings: hooks/claude/gitnexus-hook.cjs and hooks/antigravity/gitnexus-antigravity-hook.cjs each require('./hook-lock.cjs'), './hook-db-lock-probe.cjs', './resolve-analyze-cmd.cjs'. Add a second pass over shipped .cjs/.mjs assets (the runtime COPY set minus dep/data roots), resolving each relative require against the asset's OWN package-relative dir and checking COPY coverage — by prefix, NOT on-disk existence: the antigravity hook's './hook-lock.cjs' resolves to hooks/antigravity/hook-lock.cjs (which doesn't physically exist; hook-lock.cjs lives under hooks/claude) yet is covered by the whole-hooks COPY. All 6 shipped sibling requires resolve under the hooks COPY. * fix(docker): move hooks/skills COPYs past the DuckDB FTS RUN The hooks/ and skills/ COPYs sat between the vendor COPY and the DuckDB FTS-extension install RUN, so any edit to hook/skill content invalidated that RUN's cache layer — which performs a one-time network INSTALL of the extension (~tens of seconds per affected build). The COPYs have no input dependency on the DuckDB step; relocate them to after it (before USER node) so stable infrastructure layers are not rebuilt on hook/skill churn. Image contents are unchanged. The runtime-asset-parity guard still detects both (its scan covers the whole runtime stage), and the two are consolidated under one comment. --------- Co-authored-by: Claude Opus 4.8 (1M context) --- Dockerfile.cli | 16 + .../dockerfile-runtime-asset-parity.test.ts | 445 ++++++++++++++++++ 2 files changed, 461 insertions(+) create mode 100644 gitnexus/test/unit/dockerfile-runtime-asset-parity.test.ts diff --git a/Dockerfile.cli b/Dockerfile.cli index b5ec3390c..633d23f5d 100644 --- a/Dockerfile.cli +++ b/Dockerfile.cli @@ -100,6 +100,22 @@ ENV HOME=/home/node \ RUN su node -s /bin/sh -c "HOME=/home/node node /app/gitnexus/scripts/install-duckdb-extension.mjs fts" \ && su node -s /bin/sh -c "HOME=/home/node node /app/gitnexus/scripts/install-duckdb-extension.mjs fts --verify-only" +# Published runtime assets (in package.json `files`). Placed AFTER the DuckDB +# FTS-extension RUN above so editing hook/skill content does not invalidate that +# network-fetching cache layer; they have no input dependency on it. +# `hooks/`: dist/cli/resolve-invocation.js does +# `require('../../hooks/claude/resolve-analyze-cmd.cjs')` at module load — the +# single source of truth for the npm-11 npx-crash invocation decision (#1939). +# Without it, `gitnexus analyze` inside the image crashes with MODULE_NOT_FOUND +# before it does any work (#2130). `skills/`: the CLI reads the bundled SKILL.md +# templates from `/skills/` for `gitnexus analyze --skills` and `gitnexus +# setup`/`uninstall`; absent, those degrade silently (placeholder content / zero +# skills installed). (The web UI bundle `web/`, also in `files`, is deliberately +# NOT shipped: this builder never builds gitnexus-web, so the image is API-only; +# the UI is the separate Dockerfile.web image / hosted app.) +COPY --from=builder --chown=node:node /app/gitnexus/hooks ./gitnexus/hooks +COPY --from=builder --chown=node:node /app/gitnexus/skills ./gitnexus/skills + USER node # The web UI defaults to http://localhost:4747 - keep that contract. diff --git a/gitnexus/test/unit/dockerfile-runtime-asset-parity.test.ts b/gitnexus/test/unit/dockerfile-runtime-asset-parity.test.ts new file mode 100644 index 000000000..226b57d3f --- /dev/null +++ b/gitnexus/test/unit/dockerfile-runtime-asset-parity.test.ts @@ -0,0 +1,445 @@ +import { describe, it, expect } from 'vitest'; +import { readFileSync, readdirSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** + * Regression guard for #2130. + * + * `Dockerfile.cli`'s runtime stage hand-copies a SUBSET of the package's + * published assets (`package.json` `files` = dist, hooks, scripts, skills, + * vendor, web) out of the builder. npm ships all of `files`, but the Docker + * image copies only what it thinks it needs — so when compiled `dist/**` gains a + * `require()`/`createRequire()` into a sibling directory that the runtime stage + * does NOT copy, the image crashes with `MODULE_NOT_FOUND` at module load while + * the npm package keeps working. That is exactly #2130: `dist/cli/ + * resolve-invocation.js` does `require('../../hooks/claude/resolve-analyze-cmd.cjs')` + * (statically imported by `analyze.ts`), but the runtime stage never copied + * `hooks/`, so `gitnexus analyze` inside the image died before doing any work. + * + * This test derives, from the SOURCE tree, every out-of-`dist` asset that + * compiled code `require()`s AT MODULE LOAD, then asserts each one is covered by + * a runtime-stage `COPY --from=builder`. It is deliberately scoped to + * `require`/`createRequire` (hard module resolution — a missing target throws): + * an asset reached only via `fs.access`/`fs.readFile`/`new URL(...)` (e.g. `web/`) + * degrades gracefully when absent and is intentionally not copied, so it is out of + * scope here. `skills/` is also fs-accessed but IS shipped (covered by its own + * `it('copies skills/…')` below), because the CLI must stay fully usable. + */ + +const UNIT_DIR = path.dirname(fileURLToPath(import.meta.url)); +const GITNEXUS_ROOT = path.resolve(UNIT_DIR, '..', '..'); +const REPO_ROOT = path.resolve(GITNEXUS_ROOT, '..'); +const SRC_DIR = path.join(GITNEXUS_ROOT, 'src'); +const DOCKERFILE = path.join(REPO_ROOT, 'Dockerfile.cli'); + +const toPosix = (p: string): string => p.split(path.sep).join('/'); + +/** + * Source paths (relative to the gitnexus package root) copied into the image by + * the RUNTIME stage of Dockerfile.cli — e.g. `hooks`, + * `scripts/install-duckdb-extension.mjs`. The builder stage's full-tree + * `COPY gitnexus ./gitnexus` is ignored on purpose: it would mask every gap. + */ +function runtimeStageCopiedSources(dockerfile: string): string[] { + const lines = dockerfile.split('\n'); + // `i` flag: Docker accepts lowercase `as`, so a future reformat to + // `FROM … as runtime` must not silently lose the stage (which would empty the + // copied set and trip the named assertions below). + const runtimeStart = lines.findIndex((l) => /^FROM\s.*\bAS\s+runtime\b/i.test(l)); + expect(runtimeStart, 'Dockerfile.cli must declare a `... AS runtime` stage').toBeGreaterThan(-1); + const sources: string[] = []; + // Scan only the runtime stage: start after its FROM and stop at the next + // stage boundary, so COPY lines from any stage added AFTER runtime are never + // misattributed to it. + for (const line of lines.slice(runtimeStart + 1)) { + if (/^FROM\b/.test(line)) break; + if (!/^COPY\s+--from=builder\b/.test(line)) continue; + // The source operand is the `/app/gitnexus/` token (the dest is + // `./gitnexus/`). There is exactly one per COPY line here. + const m = line.match(/\s\/app\/gitnexus\/(\S+)/); + if (m) sources.push(m[1]); + } + return sources; +} + +const isCovered = (assetPath: string, copied: string[]): boolean => + copied.some((c) => assetPath === c || assetPath.startsWith(c + '/')); + +/** Recursively list non-test `.ts` files under a directory. */ +function listSourceFiles(dir: string): string[] { + const out: string[] = []; + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name === '__tests__' || entry.name === '__mocks__') continue; + out.push(...listSourceFiles(full)); + } else if ( + entry.name.endsWith('.ts') && + !entry.name.endsWith('.test.ts') && + !entry.name.endsWith('.spec.ts') && + !entry.name.endsWith('.d.ts') + ) { + out.push(full); + } + } + return out; +} + +// The `createRequire(...)('../x')` IIFE form (e.g. resolve-invocation.ts). +// Captures the relative specifier (starting with '.'). The built-in +// `require`/`_require` and aliased-binding literal forms are matched separately +// in `scanContent` from the file's discovered require-family identifiers, and +// computed (non-literal) module-load requires are handled there too. Dynamic/ +// static ESM `import` is excluded — TS keeps those inside `dist/`. +const CREATE_REQUIRE_IIFE_RE = /createRequire\([\s\S]*?\)\s*\(\s*['"](\.[^'"]+)['"]\s*\)/g; + +/** + * Strip `//` line comments and block comments so commented-out or documented + * requires (e.g. a JSDoc `require(computedPath)`, or a `// require('../../web/x')`) + * cannot spuriously trip the parity guard — a false-fail for the literal scan + * and a false "unverifiable computed require" for the broadened scan below. + * + * This is a small string-aware pass rather than a naive regex strip: it tracks + * `'`/`"`/`` ` `` string state so a slash-star or `//` INSIDE a string or glob + * literal (e.g. a `node_modules/` glob, a `thrift::x/` template) is never + * mistaken for a comment delimiter and used to mangle real code. Newlines are + * preserved so brace-depth accounting stays meaningful. (Verified: the real-tree + * literal-scan output is byte-identical with and without this pass.) + */ +function stripComments(content: string): string { + let out = ''; + type State = 'code' | 'line' | 'block' | 'sq' | 'dq' | 'tpl'; + let state: State = 'code'; + for (let i = 0; i < content.length; i += 1) { + const c = content[i]; + const c2 = content[i + 1]; + if (state === 'code') { + if (c === '/' && c2 === '/') { + state = 'line'; + i += 1; + } else if (c === '/' && c2 === '*') { + state = 'block'; + i += 1; + } else if (c === "'") { + state = 'sq'; + out += c; + } else if (c === '"') { + state = 'dq'; + out += c; + } else if (c === '`') { + state = 'tpl'; + out += c; + } else { + out += c; + } + } else if (state === 'line') { + if (c === '\n') { + state = 'code'; + out += c; + } + } else if (state === 'block') { + if (c === '*' && c2 === '/') { + state = 'code'; + i += 1; + } else if (c === '\n') { + out += c; + } + } else { + // inside a string/template literal — copy verbatim, honoring escapes + out += c; + if (c === '\\' && i + 1 < content.length) { + out += content[i + 1]; + i += 1; + } else if ( + (state === 'sq' && c === "'") || + (state === 'dq' && c === '"') || + (state === 'tpl' && c === '`') + ) { + state = 'code'; + } + } + } + return out; +} + +/** + * Vetted module-load requires whose target is a COMPUTED (non-literal) path the + * scanner cannot resolve statically. Maps a source file (relative to `src/`) to + * the package-relative asset it loads at module load. The asset is still run + * through the COPY-coverage check like any literal — this allowlist only + * suppresses the "unverifiable computed require" hard-fail; it never exempts the + * asset from `isCovered` (so deleting the covering COPY still fails the guard, + * enforced by the coverage-not-trust test below). + */ +const KNOWN_COMPUTED_REQUIRES: { source: string; asset: string }[] = [ + // community-processor.ts: `const leidenPath = resolve(__dirname,'..','..','..', + // 'vendor','leiden','index.cjs'); const leiden = _require(leidenPath);` + { source: 'core/ingestion/community-processor.ts', asset: 'vendor/leiden/index.cjs' }, +]; + +const escapeRegExp = (s: string): string => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + +/** + * Identifiers that behave like `require` in a file: the built-ins plus any + * `const X = createRequire(...)` binding (e.g. `requireCJS`, `_require`). + */ +function requireFamilyIds(content: string): string[] { + const ids = new Set(['require', '_require']); + for (const m of content.matchAll( + /\b(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*createRequire\s*\(/g, + )) { + ids.add(m[1]); + } + return [...ids]; +} + +/** Net `{` minus `}` before `index`; 0 means the call sits at module top-level. */ +function braceDepthBefore(content: string, index: number): number { + let depth = 0; + for (let i = 0; i < index; i += 1) { + const ch = content[i]; + if (ch === '{') depth += 1; + else if (ch === '}') depth -= 1; + } + return depth; +} + +interface RequireScan { + assets: { asset: string; source: string }[]; + unresolvedComputed: { source: string; arg: string }[]; +} + +/** + * What one source file require()s OUTSIDE `dist/` at module load: + * - `assets`: statically-resolvable relative literals (built-in `require`/ + * `_require`, aliased `createRequire` bindings, and the `createRequire(...) + * ('…')` IIFE) plus the resolved asset of each vetted `KNOWN_COMPUTED_REQUIRES` + * entry. + * - `unresolvedComputed`: MODULE-LOAD (brace-depth 0) requires with a computed / + * non-literal arg that are NOT in the allowlist — the guard fails closed on + * these for manual review. + * The module-load gate applies ONLY to the computed branch: in-function computed + * requires (e.g. `_require(g.pkg)`, `requireCJS(c)`) target `node_modules`/ + * `package.json`, are out of the "module-load" charter, and are ignored. Literal + * requires stay ungated — an in-function `require('../x')` still resolves to the + * same `dist`-relative target, so gating them would only risk dropping real + * coverage. Residual limit: a truly dynamic require whose target is assembled + * across functions / from config is caught only via the fail-closed gate. + */ +function scanContent(relUnderSrc: string, rawContent: string): RequireScan { + const content = stripComments(rawContent); + const distDir = path.posix.join('dist', path.posix.dirname(relUnderSrc)); + const assets: { asset: string; source: string }[] = []; + const unresolvedComputed: { source: string; arg: string }[] = []; + + const addResolved = (spec: string): void => { + const resolved = path.posix.normalize(path.posix.join(distDir, spec)); + if (resolved === 'dist' || resolved.startsWith('dist/')) return; // internal + assets.push({ asset: resolved, source: relUnderSrc }); + }; + + // (a) Literal relative requires: createRequire IIFE … + for (const m of content.matchAll(CREATE_REQUIRE_IIFE_RE)) addResolved(m[1]); + // … plus built-ins and aliased createRequire bindings called with a literal + // (`require('../x')`, `_require('../x')`, `requireCJS('../x')`). + const idAlt = requireFamilyIds(content).map(escapeRegExp).join('|'); + const aliasLiteralRe = new RegExp( + `(?.resolve(...)` is excluded — `\s*\(` must follow the identifier, but + // `.resolve(` sits between, so it never matches (it is a path lookup, not a load). + const computedRe = new RegExp(`(? k.source === relUnderSrc); + if (known) assets.push({ asset: known.asset, source: relUnderSrc }); + else unresolvedComputed.push({ source: relUnderSrc, arg: m[1].trim() }); + } + + return { assets, unresolvedComputed }; +} + +/** + * Aggregate {@link scanContent} over the whole `src/` tree. `src/.ts` + * compiles to `dist/.js`, so a specifier resolved against `dist/` + * reproduces the runtime layout exactly. + */ +function requiredExternalAssets(): RequireScan { + const assets: { asset: string; source: string }[] = []; + const unresolvedComputed: { source: string; arg: string }[] = []; + for (const file of listSourceFiles(SRC_DIR)) { + const relUnderSrc = toPosix(path.relative(SRC_DIR, file)); + const scan = scanContent(relUnderSrc, readFileSync(file, 'utf-8')); + assets.push(...scan.assets); + unresolvedComputed.push(...scan.unresolvedComputed); + } + return { assets, unresolvedComputed }; +} + +/** + * Hand-written shipped assets the runtime image executes directly (hook and + * installer `.cjs`/`.mjs`), derived from the runtime COPY set minus dependency/ + * data roots (`node_modules`, `vendor`, `dist`, `skills`, `package.json`). + * Unlike `src/**` these are not compiled, so they require siblings at their OWN + * package-relative location. + */ +function shippedAssetFiles(copiedSources: string[]): string[] { + const SKIP = new Set(['dist', 'node_modules', 'vendor', 'skills', 'web', 'package.json']); + const out: string[] = []; + const walk = (absDir: string, relDir: string): void => { + for (const e of readdirSync(absDir, { withFileTypes: true })) { + const abs = path.join(absDir, e.name); + const rel = relDir ? `${relDir}/${e.name}` : e.name; + if (e.isDirectory()) walk(abs, rel); + else if (/\.(cjs|mjs|js)$/.test(e.name)) out.push(rel); + } + }; + for (const entry of new Set(copiedSources)) { + if (SKIP.has(entry)) continue; + if (/\.(cjs|mjs|js)$/.test(entry)) { + out.push(entry); // a single shipped script (e.g. scripts/install-duckdb-extension.mjs) + } else { + walk(path.join(GITNEXUS_ROOT, entry), entry); // a directory of shipped assets (e.g. hooks) + } + } + return out; +} + +/** + * Relative `require()`s of shipped `.cjs`/`.mjs` assets, resolved against the + * asset's OWN package-relative dir (not the `dist` mapping). Coverage is checked + * by COPY prefix, never on-disk existence: `hooks/antigravity/*.cjs` does + * `require('./hook-lock.cjs')`, which resolves to `hooks/antigravity/hook-lock.cjs` + * — a path that need not physically exist but IS covered by the whole-`hooks` COPY. + */ +function shippedAssetRequiredAssets(copiedSources: string[]): { asset: string; source: string }[] { + const found: { asset: string; source: string }[] = []; + for (const rel of shippedAssetFiles(copiedSources)) { + const baseDir = path.posix.dirname(rel); + const content = stripComments(readFileSync(path.join(GITNEXUS_ROOT, rel), 'utf-8')); + for (const m of content.matchAll(/\brequire\s*\(\s*['"](\.[^'"]+)['"]\s*\)/g)) { + found.push({ asset: path.posix.normalize(path.posix.join(baseDir, m[1])), source: rel }); + } + } + return found; +} + +describe('Dockerfile.cli runtime-stage asset parity (#2130)', () => { + const dockerfile = readFileSync(DOCKERFILE, 'utf-8'); + const copied = runtimeStageCopiedSources(dockerfile); + + it('parses at least one runtime-stage COPY (guards against a vacuous pass)', () => { + // If the runtime `FROM` or the `/app/gitnexus/` source prefix ever stops + // matching, `copied` goes empty and the parity assertion below would pass + // vacuously (an empty copied set yields zero uncovered assets). Fail loud. + expect(copied.length, 'runtime stage must contain COPY --from=builder lines').toBeGreaterThan( + 0, + ); + }); + + it('copies hooks/ — resolve-invocation.ts require()s it at module load (#2130)', () => { + // The exact regression: without this COPY, `gitnexus analyze` crashes inside + // the image with `Cannot find module '../../hooks/claude/resolve-analyze-cmd.cjs'`. + expect(copied).toContain('hooks'); + }); + + it('copies skills/ — CLI reads the bundled SKILL.md templates at runtime', () => { + // Degradation class (not a crash): `gitnexus analyze --skills` (ai-context.ts) + // and `gitnexus setup`/`uninstall` read `/skills/*.md`. Absent, they + // silently emit placeholder content / install nothing. The image ships it to + // stay fully usable as a CLI. `web/` (also in `files`) is intentionally NOT + // shipped — this image never builds gitnexus-web, so it is API-only. + expect(copied).toContain('skills'); + }); + + it('sanity-checks the scanner sees both literal and vetted-computed module-load deps', () => { + const assets = requiredExternalAssets().assets.map((a) => a.asset); + // literal IIFE require (resolve-invocation.ts → hooks) … + expect(assets).toContain('hooks/claude/resolve-analyze-cmd.cjs'); + // … and the vetted COMPUTED require (community-processor.ts → vendor/leiden). + expect(assets).toContain('vendor/leiden/index.cjs'); + }); + + it('copies every out-of-dist asset reached by a resolvable or vetted module-load require', () => { + // Coverage = statically-resolvable relative literals (built-in / aliased / + // IIFE createRequire) + vetted KNOWN_COMPUTED_REQUIRES; any UNRECOGNIZED + // module-load computed require fails closed for manual review. Truly dynamic + // requires (target assembled across functions / from config) are caught only + // via that fail-closed gate, never statically resolved. + const { assets, unresolvedComputed } = requiredExternalAssets(); + expect( + unresolvedComputed, + `Unverifiable module-load computed require(s) — statically confirm each target is ` + + `COPY'd into the image and add it to KNOWN_COMPUTED_REQUIRES:\n` + + unresolvedComputed.map((u) => ` - src/${u.source}: require(${u.arg})`).join('\n'), + ).toEqual([]); + const uncovered = assets.filter(({ asset }) => !isCovered(asset, copied)); + expect( + uncovered, + `Dockerfile.cli runtime stage is missing COPY lines for module-load require() targets ` + + `outside dist/. Each will crash with MODULE_NOT_FOUND inside the image (cf. #2130). ` + + `Add a \`COPY --from=builder /app/gitnexus/ ./gitnexus/\`:\n` + + uncovered.map((u) => ` - ${u.asset} (required by src/${u.source})`).join('\n'), + ).toEqual([]); + }); + + it('coverage-checks allowlisted computed requires instead of trusting them', () => { + // vendor/leiden is contributed by KNOWN_COMPUTED_REQUIRES. Removing the + // `vendor` COPY must make it surface as uncovered — proving the allowlist + // suppresses only the unresolvable hard-fail, NOT the COPY check (else + // deleting a COPY would silently pass, recreating the #2130 class). + const assets = requiredExternalAssets().assets.map((a) => a.asset); + expect(assets).toContain('vendor/leiden/index.cjs'); + const copiedWithoutVendor = copied.filter((c) => c !== 'vendor'); + expect(isCovered('vendor/leiden/index.cjs', copied)).toBe(true); + expect(isCovered('vendor/leiden/index.cjs', copiedWithoutVendor)).toBe(false); + }); + + it('fails closed on an unrecognized module-load computed require', () => { + const scan = scanContent( + 'fake/widget.ts', + 'const r = createRequire(import.meta.url);\nconst mod = r(somethingComputed);\n', + ); + expect(scan.unresolvedComputed).toHaveLength(1); + expect(scan.unresolvedComputed[0]).toMatchObject({ + source: 'fake/widget.ts', + arg: 'somethingComputed', + }); + }); + + it('resolves aliased createRequire literals and ignores in-function computed requires', () => { + // Aliased binding with a relative literal → resolved like require('../x'). + const aliased = scanContent( + 'cli/widget.ts', + "const requireCJS = createRequire(import.meta.url);\nconst x = requireCJS('../../hooks/z.cjs');\n", + ); + expect(aliased.assets.map((a) => a.asset)).toContain('hooks/z.cjs'); + expect(aliased.unresolvedComputed).toEqual([]); + // A computed require INSIDE a function is out of the module-load charter. + const inFn = scanContent('cli/widget.ts', 'function f(pkg) {\n return require(pkg);\n}\n'); + expect(inFn.unresolvedComputed).toEqual([]); + expect(inFn.assets).toEqual([]); + }); + + it('covers sibling requires of shipped .cjs/.mjs assets (coverage, not existence)', () => { + const shipped = shippedAssetRequiredAssets(copied); + // Sanity: the hand-written hook .cjs sibling requires are actually scanned + // (e.g. gitnexus-hook.cjs → ./hook-lock.cjs); guards against a silent no-op. + expect(shipped.length).toBeGreaterThan(0); + const uncovered = shipped.filter(({ asset }) => !isCovered(asset, copied)); + expect( + uncovered, + `Shipped .cjs/.mjs assets require siblings not COPY'd into the image:\n` + + uncovered.map((u) => ` - ${u.asset} (required by ${u.source})`).join('\n'), + ).toEqual([]); + // The antigravity hook's `require('./hook-lock.cjs')` resolves to a path that + // does NOT physically exist (hook-lock.cjs lives under hooks/claude), yet is + // covered by the whole-`hooks` COPY — coverage-check, not existence-check. + expect(isCovered('hooks/antigravity/hook-lock.cjs', copied)).toBe(true); + }); +}); From 292f26ece350fed7dcaf692abc27787278228438 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Wed, 10 Jun 2026 09:09:41 +0100 Subject: [PATCH 2/2] fix(hooks): silence MCP-owned-DB augment skip for strict hook runners (#1913) (#2134) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(hooks): silence MCP-owned-DB augment skip for strict hook runners The PreToolUse augment-skip path wrote `[GitNexus] augment skipped: MCP server owns DB` to stderr unconditionally on a normal (non-error) skip. Strict hook runners that validate hook output (e.g. Codex `PreToolUse`) treat that as noisy / "invalid pre-tool-use JSON output". Gate the diagnostic behind GITNEXUS_DEBUG via a shared `isDebugEnabled()` helper, so normal skips are silent by default (empty stdout AND stderr, exit 0) and the reason stays recoverable with `GITNEXUS_DEBUG=1`. Applied consistently to all three hand-maintained hook copies (claude, antigravity, claude-plugin). Tests: - Unit (claude CJS + plugin): assert default-silent and debug-on behavior for the MCP-owned-DB skip and for the fail-closed (lsof ETIMEDOUT) skip that routes through the same gated line; the owner-detection tests run with GITNEXUS_DEBUG=1 so the skip discriminator stays observable. - e2e (antigravity): the antigravity adapter shares the identical gated skip but only runs from its install dir, so cover it through the install pipeline with a faked DB-owner probe (strict empty-stdout/stderr + debug-on). Promote the fake-probe helpers (createHookToolDir / hookEnv, plus a module-private writeExecutable) into shared hook-test-helpers so unit + e2e reuse them. Fixes #1913 Co-Authored-By: Claude Opus 4.8 (1M context) * fix(hooks): unify GITNEXUS_DEBUG gating in main() catch handlers The main() catch-handler in all three hook copies still gated its crash log on truthy `if (process.env.GITNEXUS_DEBUG)`, while the skip diagnostic the #1913 fix added is gated on the strict `isDebugEnabled()` helper (=== '1' || === 'true'). That split meant GITNEXUS_DEBUG=0 or =false suppressed the skip line yet still enabled crash logging — two conflicting contract signals in the same file. Switch the three catch handlers to isDebugEnabled() so GITNEXUS_DEBUG has one strict meaning everywhere: exactly '1' or 'true' enables all diagnostics; everything else (incl. '0', 'false', empty, unset) is silent. Add boundary tests asserting the MCP-owner skip stays silent with GITNEXUS_DEBUG='0' and 'false' (CJS + Plugin), pinning the strict contract. Refs #1913 Co-Authored-By: Claude Opus 4.8 (1M context) * fix(hooks): gate antigravity stale-index hint stderr behind GITNEXUS_DEBUG The antigravity AfterTool handler mirrored the stale-index hint to stderr unconditionally on a normal (non-error) success path — the last ungated stderr write of the class issue #1913 targets, and a divergence from the claude hook, which never mirrors this hint to stderr. Gate the stderr mirror behind isDebugEnabled(). The hint still reaches the agent via additionalContext (stdout JSON) — parts.push(hint) stays unconditional — so there is no functional loss; only the by-default terminal mirror moves behind GITNEXUS_DEBUG=1. This knowingly changes the #1730 terminal-mirror behavior in favor of strict-runner cleanliness and parity with the claude adapter. Split the e2e assertion into a default-silent test (hint in additionalContext, absent from stderr) and a GITNEXUS_DEBUG=1 test (hint mirrored to stderr). Refs #1913 Co-Authored-By: Claude Opus 4.8 (1M context) * docs(hooks): document GITNEXUS_DEBUG=1 for hook diagnostics GITNEXUS_DEBUG was documented only in the cursor integration README, so the diagnostic escape hatch for the Claude Code / Antigravity hooks was undiscoverable. Operators hitting a silent hook skip (MCP server owns the DB, fail-closed probe timeout, or an already-current index) had no documented way to surface the reason. Add a Troubleshooting subsection explaining that the hooks stay silent on normal skip paths for strict runners, that GITNEXUS_DEBUG=1 surfaces the reason on stderr, and that only '1'/'true' enable diagnostics (stdout JSON the agent consumes is unaffected). Refs #1913 Co-Authored-By: Claude Opus 4.8 (1M context) * test(hooks): update setup-antigravity unit test for gated stale-index hint U2 (7995e921) gated the antigravity stale-index hint stderr mirror behind GITNEXUS_DEBUG, but a second test — setup-antigravity.test.ts's "AfterTool emits stale-index hint" — also asserted the hint on stderr by default and was missed (it lives outside the two files validated locally; the full CI matrix caught it). Update it to the U2 contract: assert the hint via additionalContext with stderr silent by default, plus a GITNEXUS_DEBUG=1 run asserting the terminal mirror reappears. Refs #1913 Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- gitnexus-claude-plugin/hooks/gitnexus-hook.js | 21 +- gitnexus/README.md | 20 ++ .../antigravity/gitnexus-antigravity-hook.cjs | 28 ++- gitnexus/hooks/claude/gitnexus-hook.cjs | 21 +- .../integration/antigravity-hook-e2e.test.ts | 122 +++++++++- gitnexus/test/unit/hooks.test.ts | 213 ++++++++++++------ gitnexus/test/unit/setup-antigravity.test.ts | 45 ++-- gitnexus/test/utils/hook-test-helpers.ts | 65 ++++++ 8 files changed, 439 insertions(+), 96 deletions(-) diff --git a/gitnexus-claude-plugin/hooks/gitnexus-hook.js b/gitnexus-claude-plugin/hooks/gitnexus-hook.js index e3c62c769..c3ec2ecf5 100644 --- a/gitnexus-claude-plugin/hooks/gitnexus-hook.js +++ b/gitnexus-claude-plugin/hooks/gitnexus-hook.js @@ -110,10 +110,20 @@ function hasGitNexusServerOwner(gitNexusDir) { return hasGitNexusDbLockedByGitNexusServer(path.join(gitNexusDir, 'lbug'), process.pid); } +/** + * Whether opt-in diagnostics should be written to the hook's stderr. Strict + * hook runners (e.g. Codex `PreToolUse`) validate hook output, so normal, + * non-error skip paths must stay silent unless the operator explicitly asks + * for diagnostics via GITNEXUS_DEBUG. See issue #1913. + */ +function isDebugEnabled() { + return process.env.GITNEXUS_DEBUG === '1' || process.env.GITNEXUS_DEBUG === 'true'; +} + function extractAugmentContext(stderr) { const output = (stderr || '').trim(); const marker = output.indexOf('[GitNexus]'); - const debug = process.env.GITNEXUS_DEBUG === '1' || process.env.GITNEXUS_DEBUG === 'true'; + const debug = isDebugEnabled(); if (debug && output.length > 0) { // Emit the FULL discarded prefix (everything before the marker, or all of // it when no marker is present) so suppressed diagnostics — LadybugDB lock @@ -267,7 +277,12 @@ function handlePreToolUse(input) { const pattern = extractPattern(toolName, toolInput); if (!pattern || pattern.length < 3) return; if (hasGitNexusServerOwner(gitNexusDir)) { - process.stderr.write('[GitNexus] augment skipped: MCP server owns DB\n'); + // Normal skip path: the MCP server owns the DB, so the CLI augment would + // contend on the lock. Stay silent for strict hook runners (issue #1913); + // surface the reason only when diagnostics are explicitly requested. + if (isDebugEnabled()) { + process.stderr.write('[GitNexus] augment skipped: MCP server owns DB\n'); + } return; } @@ -366,7 +381,7 @@ function main() { const handler = handlers[input.hook_event_name || '']; if (handler) handler(input); } catch (err) { - if (process.env.GITNEXUS_DEBUG) { + if (isDebugEnabled()) { console.error('GitNexus hook error:', (err.message || '').slice(0, 200)); } } diff --git a/gitnexus/README.md b/gitnexus/README.md index d6ea20641..4f5b27781 100644 --- a/gitnexus/README.md +++ b/gitnexus/README.md @@ -436,6 +436,26 @@ After scope resolution, analyze prunes inert block-local value symbols (a functi Programmatic callers can pass `keepLocalValueSymbols: true` in `PipelineOptions` instead of setting the env var. +### Hook augmentation/notifications are silently skipped + +The Claude Code / Antigravity hooks intentionally stay **silent** on normal skip +paths so strict hook runners (e.g. Codex `PreToolUse`) never see unexpected +output. A search may not be augmented — or a stale-index reminder may not appear +on stderr — when the GitNexus MCP server owns the repo DB, when the DB-lock probe +times out and fails closed, or when the index is already current. + +To see why a hook skipped, set `GITNEXUS_DEBUG=1` and re-run the action — the hook +writes the reason (e.g. `[GitNexus] augment skipped: MCP server owns DB`) and the +stale-index hint to its stderr: + +```bash +GITNEXUS_DEBUG=1 # surfaces hook skip/diagnostic reasons on stderr +``` + +Only `GITNEXUS_DEBUG=1` and `GITNEXUS_DEBUG=true` enable diagnostics; every other +value (including `0` and `false`) is treated as off. Diagnostics go to stderr +only — the hook's structured stdout (the JSON the agent consumes) is unaffected. + ## Privacy - All processing happens locally on your machine diff --git a/gitnexus/hooks/antigravity/gitnexus-antigravity-hook.cjs b/gitnexus/hooks/antigravity/gitnexus-antigravity-hook.cjs index bbfccb92e..0d837fb2c 100755 --- a/gitnexus/hooks/antigravity/gitnexus-antigravity-hook.cjs +++ b/gitnexus/hooks/antigravity/gitnexus-antigravity-hook.cjs @@ -91,10 +91,20 @@ function hasGitNexusServerOwner(gitNexusDir) { return hasGitNexusDbLockedByGitNexusServer(path.join(gitNexusDir, 'lbug'), process.pid); } +/** + * Whether opt-in diagnostics should be written to the hook's stderr. Strict + * hook runners validate hook output, so normal, non-error skip paths must stay + * silent unless the operator explicitly asks for diagnostics via GITNEXUS_DEBUG. + * See issue #1913. + */ +function isDebugEnabled() { + return process.env.GITNEXUS_DEBUG === '1' || process.env.GITNEXUS_DEBUG === 'true'; +} + function extractAugmentContext(stderr) { const output = (stderr || '').trim(); const marker = output.indexOf('[GitNexus]'); - const debug = process.env.GITNEXUS_DEBUG === '1' || process.env.GITNEXUS_DEBUG === 'true'; + const debug = isDebugEnabled(); if (debug && output.length > 0) { // Emit the FULL discarded prefix (everything before the marker, or all of // it when no marker is present) so suppressed diagnostics — LadybugDB lock @@ -258,8 +268,14 @@ function buildAfterToolContext(input) { if (/\bgit\s+(commit|merge|rebase|cherry-pick|pull)(\s|$)/.test(command)) { const hint = buildStaleIndexHint(gitNexusDir, cwd); if (hint) { - process.stderr.write(`${hint}\n`); + // The hint always reaches the agent via additionalContext (parts). Mirror + // it to stderr (for terminal users) only under GITNEXUS_DEBUG, so strict + // hook runners see no unexpected output on this normal path (#1913). The + // claude hook never mirrored this to stderr — this aligns the two adapters. parts.push(hint); + if (isDebugEnabled()) { + process.stderr.write(`${hint}\n`); + } } } } @@ -269,7 +285,11 @@ function buildAfterToolContext(input) { function runAugment(gitNexusDir, cwd, pattern) { if (hasGitNexusServerOwner(gitNexusDir)) { - process.stderr.write('[GitNexus] augment skipped: MCP server owns DB\n'); + // Normal skip path: the MCP server owns the DB. Stay silent for strict + // hook runners (issue #1913); surface the reason only under GITNEXUS_DEBUG. + if (isDebugEnabled()) { + process.stderr.write('[GitNexus] augment skipped: MCP server owns DB\n'); + } return ''; } const release = acquireHookSlot(gitNexusDir); @@ -338,7 +358,7 @@ function main() { const handler = handlers[input.hook_event_name || '']; if (handler) handler(input); } catch (err) { - if (process.env.GITNEXUS_DEBUG) { + if (isDebugEnabled()) { console.error('GitNexus antigravity hook error:', (err.message || '').slice(0, 200)); } } diff --git a/gitnexus/hooks/claude/gitnexus-hook.cjs b/gitnexus/hooks/claude/gitnexus-hook.cjs index 8bfa49381..40d0b08df 100755 --- a/gitnexus/hooks/claude/gitnexus-hook.cjs +++ b/gitnexus/hooks/claude/gitnexus-hook.cjs @@ -110,10 +110,20 @@ function hasGitNexusServerOwner(gitNexusDir) { return hasGitNexusDbLockedByGitNexusServer(path.join(gitNexusDir, 'lbug'), process.pid); } +/** + * Whether opt-in diagnostics should be written to the hook's stderr. Strict + * hook runners (e.g. Codex `PreToolUse`) validate hook output, so normal, + * non-error skip paths must stay silent unless the operator explicitly asks + * for diagnostics via GITNEXUS_DEBUG. See issue #1913. + */ +function isDebugEnabled() { + return process.env.GITNEXUS_DEBUG === '1' || process.env.GITNEXUS_DEBUG === 'true'; +} + function extractAugmentContext(stderr) { const output = (stderr || '').trim(); const marker = output.indexOf('[GitNexus]'); - const debug = process.env.GITNEXUS_DEBUG === '1' || process.env.GITNEXUS_DEBUG === 'true'; + const debug = isDebugEnabled(); if (debug && output.length > 0) { // Emit the FULL discarded prefix (everything before the marker, or all of // it when no marker is present) so suppressed diagnostics — KuzuDB lock @@ -250,7 +260,12 @@ function handlePreToolUse(input) { const pattern = extractPattern(toolName, toolInput); if (!pattern || pattern.length < 3) return; if (hasGitNexusServerOwner(gitNexusDir)) { - process.stderr.write('[GitNexus] augment skipped: MCP server owns DB\n'); + // Normal skip path: the MCP server owns the DB, so the CLI augment would + // contend on the lock. Stay silent for strict hook runners (issue #1913); + // surface the reason only when diagnostics are explicitly requested. + if (isDebugEnabled()) { + process.stderr.write('[GitNexus] augment skipped: MCP server owns DB\n'); + } return; } @@ -361,7 +376,7 @@ function main() { const handler = handlers[input.hook_event_name || '']; if (handler) handler(input); } catch (err) { - if (process.env.GITNEXUS_DEBUG) { + if (isDebugEnabled()) { console.error('GitNexus hook error:', (err.message || '').slice(0, 200)); } } diff --git a/gitnexus/test/integration/antigravity-hook-e2e.test.ts b/gitnexus/test/integration/antigravity-hook-e2e.test.ts index 33b2ca56f..a4a9d1f01 100644 --- a/gitnexus/test/integration/antigravity-hook-e2e.test.ts +++ b/gitnexus/test/integration/antigravity-hook-e2e.test.ts @@ -26,6 +26,8 @@ import { runHook, parseHookOutput, createGitNexusPathEntry, + createHookToolDir, + hookEnv, envWithPath, } from '../utils/hook-test-helpers.js'; import { setupCommand } from '../../src/cli/setup.js'; @@ -101,7 +103,10 @@ afterAll(async () => { describe('antigravity hook adapter e2e', () => { describe('AfterTool — stale-index hint after git mutations', () => { - it('emits the hint via both additionalContext and stderr after a successful git commit', () => { + // #1913: by default the hint reaches the agent via additionalContext (stdout + // JSON) but is NOT mirrored to stderr, so strict hook runners see no + // unexpected output on this normal (non-error) path. + it('emits the hint via additionalContext and stays silent on stderr by default', () => { fs.writeFileSync( path.join(gitNexusDir, 'meta.json'), JSON.stringify({ lastCommit: 'a'.repeat(40), stats: {} }), @@ -117,7 +122,7 @@ describe('antigravity hook adapter e2e', () => { cwd: tmpDir, }, tmpDir, - { env: { ...process.env, GITNEXUS_INVOCATION: 'npx' } }, + { env: { ...process.env, GITNEXUS_INVOCATION: 'npx', GITNEXUS_DEBUG: '' } }, ); const output = parseHookOutput(result.stdout); @@ -125,9 +130,33 @@ describe('antigravity hook adapter e2e', () => { expect(output!.hookEventName).toBe('AfterTool'); expect(output!.additionalContext).toContain('index is stale'); expect(output!.additionalContext).toContain('npx gitnexus@latest analyze'); + // Strict-runner contract: the hint is NOT mirrored to stderr by default. + expect(result.stderr).not.toContain('[GitNexus] index is stale'); + }); - // Mirror to stderr so terminal users see the hint even when the agent - // discards additionalContext + // #1913: the terminal-mirror remains available for operators who opt in. + it('mirrors the hint to stderr for terminal users only under GITNEXUS_DEBUG=1', () => { + fs.writeFileSync( + path.join(gitNexusDir, 'meta.json'), + JSON.stringify({ lastCommit: 'a'.repeat(40), stats: {} }), + ); + + const result = runHook( + installedHook, + { + hook_event_name: 'AfterTool', + tool_name: 'run_shell_command', + tool_input: { command: 'git commit -m "test"' }, + tool_response: { llmContent: '[committed]' }, + cwd: tmpDir, + }, + tmpDir, + { env: { ...process.env, GITNEXUS_INVOCATION: 'npx', GITNEXUS_DEBUG: '1' } }, + ); + + const output = parseHookOutput(result.stdout); + expect(output).not.toBeNull(); + expect(output!.additionalContext).toContain('index is stale'); expect(result.stderr).toContain('[GitNexus] index is stale'); }); @@ -359,6 +388,91 @@ describe('antigravity hook adapter e2e', () => { }); }); + // Issue #1913: when a GitNexus MCP server owns the repo DB, runAugment() must + // SKIP — silently by default so strict hook runners never see unexpected + // output, and surface the reason only under GITNEXUS_DEBUG=1. The Claude/Plugin + // copies are covered in test/unit/hooks.test.ts; the antigravity adapter shares + // the identical gated skip and is exercised here through the install pipeline + // (its lock/probe helpers only resolve from the install dir). A faked lsof/ps + + // an empty `lbug` lock force hasGitNexusServerOwner() => true; a marker-writing + // fake CLI proves augment never ran. + describe.skipIf(process.platform === 'win32')( + 'AfterTool — augment skipped when MCP server owns the DB (#1913)', + () => { + const OWNER_PROBE = { + lsofOutput: '12345\n', + psOutput: 'node /tmp/node_modules/.bin/gitnexus mcp\n', + }; + + it('stays SILENT by default (no augment ran, no stderr noise, exit 0)', () => { + const markerPath = path.join(os.tmpdir(), `antigravity-skip-silent-${process.pid}`); + const lbugPath = path.join(gitNexusDir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + fs.rmSync(markerPath, { force: true }); + const binDir = createHookToolDir({ ...OWNER_PROBE, gitnexusMarkerPath: markerPath }); + try { + const result = runHook( + installedHook, + { + hook_event_name: 'AfterTool', + tool_name: 'search_file_content', + tool_input: { pattern: 'validateUser' }, + tool_response: { llmContent: '...' }, + cwd: tmpDir, + }, + tmpDir, + { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '' } }, + ); + + expect(result.status).toBe(0); + // Strict-runner contract: completely silent — empty stdout AND stderr + // (matches the unit suite's assertion strength for the claude/plugin copies). + expect(result.stdout.trim()).toBe(''); + expect(result.stderr.trim()).toBe(''); + // Marker absent ⇒ the CLI never ran (augment short-circuited at the owner + // check). The paired GITNEXUS_DEBUG=1 test below positively proves the skip + // was the owner path (it asserts the owner-skip diagnostic on stderr). + expect(fs.existsSync(markerPath)).toBe(false); + } finally { + fs.rmSync(lbugPath, { force: true }); + fs.rmSync(markerPath, { force: true }); + fs.rmSync(binDir, { recursive: true, force: true }); + } + }); + + it('surfaces the skip reason on stderr only under GITNEXUS_DEBUG=1', () => { + const markerPath = path.join(os.tmpdir(), `antigravity-skip-debug-${process.pid}`); + const lbugPath = path.join(gitNexusDir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + fs.rmSync(markerPath, { force: true }); + const binDir = createHookToolDir({ ...OWNER_PROBE, gitnexusMarkerPath: markerPath }); + try { + const result = runHook( + installedHook, + { + hook_event_name: 'AfterTool', + tool_name: 'search_file_content', + tool_input: { pattern: 'validateUser' }, + tool_response: { llmContent: '...' }, + cwd: tmpDir, + }, + tmpDir, + { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '1' } }, + ); + + expect(result.status).toBe(0); + expect(parseHookOutput(result.stdout)).toBeNull(); + expect(result.stderr).toContain('[GitNexus] augment skipped: MCP server owns DB'); + expect(fs.existsSync(markerPath)).toBe(false); + } finally { + fs.rmSync(lbugPath, { force: true }); + fs.rmSync(markerPath, { force: true }); + fs.rmSync(binDir, { recursive: true, force: true }); + } + }); + }, + ); + describe('cwd validation', () => { it('rejects relative cwd silently', () => { const result = runHook(installedHook, { diff --git a/gitnexus/test/unit/hooks.test.ts b/gitnexus/test/unit/hooks.test.ts index ef8f9afef..54193d837 100644 --- a/gitnexus/test/unit/hooks.test.ts +++ b/gitnexus/test/unit/hooks.test.ts @@ -22,7 +22,12 @@ import { spawnSync } from 'child_process'; import fs from 'fs'; import path from 'path'; import os from 'os'; -import { runHook, parseHookOutput } from '../utils/hook-test-helpers.js'; +import { + runHook, + parseHookOutput, + createHookToolDir, + hookEnv, +} from '../utils/hook-test-helpers.js'; // ─── Paths to both hook variants ──────────────────────────────────── @@ -145,61 +150,8 @@ function createGlobalRegistry(homeDir: string, marker: 'both' | 'registry' | 're } } -function writeExecutable(filePath: string, content: string) { - fs.writeFileSync(filePath, content, { mode: 0o755 }); -} - -function createHookToolDir(options: { - gitnexusStderr?: string; - gitnexusMarkerPath?: string; - lsofOutput?: string; - lsofOutputLines?: string[]; - psOutput?: string; - psOutputByPid?: Record; - lsofSleepMs?: number; -}) { - const binDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-hook-bin-')); - const gitnexusStderr = JSON.stringify(options.gitnexusStderr ?? ''); - const markerPath = JSON.stringify(options.gitnexusMarkerPath ?? ''); - - const fakeGitNexus = `#!/usr/bin/env node\nconst fs = require('fs');\nconst marker = ${markerPath};\nif (marker) fs.writeFileSync(marker, 'called');\nprocess.stderr.write(${gitnexusStderr});\n`; - writeExecutable(path.join(binDir, 'gitnexus'), fakeGitNexus); - writeExecutable(path.join(binDir, 'gitnexus-cli.js'), fakeGitNexus); - - const lsofOutput = - options.lsofOutputLines != null - ? options.lsofOutputLines.join('\n') + (options.lsofOutputLines.length ? '\n' : '') - : (options.lsofOutput ?? ''); - const lsofBody = - options.lsofSleepMs != null - ? `#!/usr/bin/env node\nsetTimeout(() => {}, ${Number(options.lsofSleepMs)});\n` - : `#!/usr/bin/env node\nprocess.stdout.write(${JSON.stringify(lsofOutput)});\nprocess.exit(0);\n`; - writeExecutable(path.join(binDir, 'lsof'), lsofBody); - - const psBody = - options.psOutputByPid != null - ? `#!/usr/bin/env node -const byPid = ${JSON.stringify(options.psOutputByPid)}; -const args = process.argv; -const p = args[args.indexOf('-p') + 1]; -process.stdout.write(byPid[p] ?? ''); -process.exit(0); -` - : `#!/usr/bin/env node\nprocess.stdout.write(${JSON.stringify(options.psOutput ?? '')});\nprocess.exit(0);\n`; - writeExecutable(path.join(binDir, 'ps'), psBody); - - return binDir; -} - -function hookEnv(binDir: string) { - return { - ...process.env, - PATH: `${binDir}${path.delimiter}${process.env.PATH || ''}`, - GITNEXUS_HOOK_CLI_PATH: path.join(binDir, 'gitnexus-cli.js'), - GITNEXUS_HOOK_LSOF_PATH: path.join(binDir, 'lsof'), - GITNEXUS_HOOK_PS_PATH: path.join(binDir, 'ps'), - }; -} +// createHookToolDir / hookEnv live in ../utils/hook-test-helpers so the antigravity +// e2e suite can reuse the same DB-owner-probe fakes. // ─── Both hook files should exist ─────────────────────────────────── @@ -972,8 +924,13 @@ describe('PreToolUse augmentation filtering (integration)', () => { } }); + // Issue #1913: the MCP-owned-DB skip is a NORMAL (non-error) path, so by + // default it must stay completely silent — empty stdout AND empty stderr, + // exit 0 — so strict hook runners (e.g. Codex `PreToolUse`) never see + // unexpected output. GITNEXUS_DEBUG is forced off to keep the assertion + // deterministic regardless of the ambient environment. it.skipIf(process.platform === 'win32')( - `${label}: skips augment when a GitNexus MCP process owns the repo DB`, + `${label}: skips augment SILENTLY when a GitNexus MCP process owns the repo DB`, () => { const markerPath = path.join(os.tmpdir(), `gitnexus-hook-called-${process.pid}-${label}`); const lbugPath = path.join(gitNexusDir, 'lbug'); @@ -994,25 +951,118 @@ describe('PreToolUse augmentation filtering (integration)', () => { cwd: tmpDir, }, undefined, - { env: hookEnv(binDir) }, + { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '' } }, ); expect(result.stdout.trim()).toBe(''); + expect(result.stderr.trim()).toBe(''); expect(result.status).toBe(0); - expect(result.stderr).toContain('[GitNexus] augment skipped'); expect(fs.existsSync(markerPath)).toBe(false); } finally { + fs.rmSync(lbugPath, { force: true }); fs.rmSync(markerPath, { force: true }); fs.rmSync(binDir, { recursive: true, force: true }); } }, ); + + // Issue #1913: the skip reason remains recoverable for operators who opt in + // via GITNEXUS_DEBUG=1 — stdout stays empty (no augment ran), the diagnostic + // appears on stderr. + it.skipIf(process.platform === 'win32')( + `${label}: surfaces the MCP-owner skip reason only under GITNEXUS_DEBUG`, + () => { + const markerPath = path.join(os.tmpdir(), `gitnexus-hook-dbg-${process.pid}-${label}`); + const lbugPath = path.join(gitNexusDir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + fs.rmSync(markerPath, { force: true }); + const binDir = createHookToolDir({ + gitnexusMarkerPath: markerPath, + lsofOutput: '12345\n', + psOutput: 'node /tmp/node_modules/.bin/gitnexus mcp\n', + }); + try { + const result = runHook( + hookPath, + { + hook_event_name: 'PreToolUse', + tool_name: 'Grep', + tool_input: { pattern: 'validateUser' }, + cwd: tmpDir, + }, + undefined, + { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '1' } }, + ); + + expect(result.stdout.trim()).toBe(''); + expect(result.status).toBe(0); + expect(result.stderr).toContain('[GitNexus] augment skipped: MCP server owns DB'); + expect(fs.existsSync(markerPath)).toBe(false); + } finally { + fs.rmSync(lbugPath, { force: true }); + fs.rmSync(markerPath, { force: true }); + fs.rmSync(binDir, { recursive: true, force: true }); + } + }, + ); + + // #1913: the GITNEXUS_DEBUG contract is strict — ONLY '1' and 'true' enable + // diagnostics. Pin that non-canonical truthy-looking values ('0', 'false') + // are treated as OFF, so the skip stays silent. A truthy-gated reader would + // have emitted on these; this guards the unified strict gate (incl. the + // main() catch handler) across the claude/plugin copies. + for (const debugValue of ['0', 'false']) { + it.skipIf(process.platform === 'win32')( + `${label}: MCP-owner skip stays SILENT with GITNEXUS_DEBUG='${debugValue}' (strict contract)`, + () => { + const markerPath = path.join( + os.tmpdir(), + `gitnexus-hook-dbg-${debugValue}-${process.pid}-${label}`, + ); + const lbugPath = path.join(gitNexusDir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + fs.rmSync(markerPath, { force: true }); + const binDir = createHookToolDir({ + gitnexusMarkerPath: markerPath, + lsofOutput: '12345\n', + psOutput: 'node /tmp/node_modules/.bin/gitnexus mcp\n', + }); + try { + const result = runHook( + hookPath, + { + hook_event_name: 'PreToolUse', + tool_name: 'Grep', + tool_input: { pattern: 'validateUser' }, + cwd: tmpDir, + }, + undefined, + { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: debugValue } }, + ); + + expect(result.stdout.trim()).toBe(''); + expect(result.stderr.trim()).toBe(''); + expect(result.status).toBe(0); + expect(fs.existsSync(markerPath)).toBe(false); + } finally { + fs.rmSync(lbugPath, { force: true }); + fs.rmSync(markerPath, { force: true }); + fs.rmSync(binDir, { recursive: true, force: true }); + } + }, + ); + } } }); describe.skipIf(process.platform === 'win32')( 'Ladybug DB owner guard — production-shaped ps + failure modes (#1493)', () => { + // These tests assert owner *detection*: a positive skip is signalled by the + // `[GitNexus] augment skipped` diagnostic. Since #1913 made that diagnostic + // debug-gated (silent by default for strict hook runners), they run with + // GITNEXUS_DEBUG=1 so the discriminator remains observable. Default-silence + // itself is covered by the 'augmentation filtering' describe above. for (const [label, hookPath] of [ ['CJS', CJS_HOOK], ['Plugin', PLUGIN_HOOK], @@ -1037,7 +1087,7 @@ describe.skipIf(process.platform === 'win32')( cwd: tmpDir, }, undefined, - { env: hookEnv(binDir) }, + { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '1' } }, ); expect(result.stdout.trim()).toBe(''); expect(result.status).toBe(0); @@ -1101,7 +1151,7 @@ describe.skipIf(process.platform === 'win32')( cwd: tmpDir, }, undefined, - { env: hookEnv(binDir) }, + { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '1' } }, ); expect(result.stdout.trim()).toBe(''); expect(result.status).toBe(0); @@ -1169,7 +1219,7 @@ describe.skipIf(process.platform === 'win32')( cwd: tmpDir, }, undefined, - { env: hookEnv(binDir) }, + { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '1' } }, ); expect(result.stdout.trim()).toBe(''); expect(result.status).toBe(0); @@ -1181,6 +1231,43 @@ describe.skipIf(process.platform === 'win32')( } }); + // #1913: the fail-closed (probe-timeout) skip routes through the SAME gated + // line as the MCP-owner skip, so it too must be silent by default. Symmetric + // counterpart to the debug-on test above, so a regression that ungated the + // ETIMEDOUT path specifically would still be caught. + it(`${label}: ETIMEDOUT lsof → augment skipped SILENTLY by default`, () => { + const markerPath = path.join(os.tmpdir(), `gn-hook-etime-silent-${process.pid}-${label}`); + const lbugPath = path.join(gitNexusDir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + fs.rmSync(markerPath, { force: true }); + const binDir = createHookToolDir({ + gitnexusMarkerPath: markerPath, + lsofSleepMs: 5000, + psOutput: '', + }); + try { + const result = runHook( + hookPath, + { + hook_event_name: 'PreToolUse', + tool_name: 'Grep', + tool_input: { pattern: 'validateUser' }, + cwd: tmpDir, + }, + undefined, + { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '' } }, + ); + expect(result.stdout.trim()).toBe(''); + expect(result.stderr.trim()).toBe(''); + expect(result.status).toBe(0); + expect(fs.existsSync(markerPath)).toBe(false); + } finally { + fs.rmSync(lbugPath, { force: true }); + fs.rmSync(markerPath, { force: true }); + fs.rmSync(binDir, { recursive: true, force: true }); + } + }); + it(`${label}: non-GitNexus ps line → augment runs`, () => { const markerPath = path.join(os.tmpdir(), `gn-hook-other-${process.pid}-${label}`); const lbugPath = path.join(gitNexusDir, 'lbug'); @@ -1237,7 +1324,7 @@ describe.skipIf(process.platform === 'win32')( cwd: tmpDir, }, undefined, - { env: hookEnv(binDir) }, + { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '1' } }, ); expect(result.stdout.trim()).toBe(''); expect(result.status).toBe(0); diff --git a/gitnexus/test/unit/setup-antigravity.test.ts b/gitnexus/test/unit/setup-antigravity.test.ts index 73a1cb305..42c58bb47 100644 --- a/gitnexus/test/unit/setup-antigravity.test.ts +++ b/gitnexus/test/unit/setup-antigravity.test.ts @@ -428,29 +428,36 @@ describe('gitnexus-antigravity-hook adapter', () => { 'utf-8', ); - const { stdout, stderr } = runAdapter( - adapter, - { - hook_event_name: 'AfterTool', - tool_name: 'run_shell_command', - tool_input: { command: 'git commit -m "x"' }, - tool_response: { llmContent: '[committed]' }, - cwd: workdir, - }, - workdir, - // Force a deterministic invocation mode: the emitted analyze command - // varies by what's installed on each CI runner (gitnexus/pnpm/npx), and - // only the `gitnexus` mode yields the bare `gitnexus analyze` form. - { GITNEXUS_INVOCATION: 'gitnexus' }, - ); - - // Hint surfaces both via the agent-visible channel and stderr (terminal). - expect(stderr).toMatch(/\[GitNexus\] index is stale/); - expect(stderr).toMatch(/gitnexus analyze/); + const input = { + hook_event_name: 'AfterTool', + tool_name: 'run_shell_command', + tool_input: { command: 'git commit -m "x"' }, + tool_response: { llmContent: '[committed]' }, + cwd: workdir, + }; + // Force a deterministic invocation mode: the emitted analyze command varies + // by what's installed on each CI runner (gitnexus/pnpm/npx); only the + // `gitnexus` mode yields the bare `gitnexus analyze` form. + const { stdout, stderr } = runAdapter(adapter, input, workdir, { + GITNEXUS_INVOCATION: 'gitnexus', + GITNEXUS_DEBUG: '', + }); + // #1913: by default the hint reaches the agent via additionalContext (stdout + // JSON) but is NOT mirrored to stderr, so strict hook runners stay clean. const parsed = JSON.parse(stdout); expect(parsed.hookSpecificOutput.hookEventName).toBe('AfterTool'); expect(parsed.hookSpecificOutput.additionalContext).toMatch(/index is stale/); + expect(parsed.hookSpecificOutput.additionalContext).toMatch(/gitnexus analyze/); + expect(stderr).not.toMatch(/\[GitNexus\] index is stale/); + + // The terminal mirror remains available under GITNEXUS_DEBUG=1. + const debug = runAdapter(adapter, input, workdir, { + GITNEXUS_INVOCATION: 'gitnexus', + GITNEXUS_DEBUG: '1', + }); + expect(debug.stderr).toMatch(/\[GitNexus\] index is stale/); + expect(debug.stderr).toMatch(/gitnexus analyze/); }); it('AfterTool skips augment when the tool failed', async () => { diff --git a/gitnexus/test/utils/hook-test-helpers.ts b/gitnexus/test/utils/hook-test-helpers.ts index f659a5834..1baa94ea6 100644 --- a/gitnexus/test/utils/hook-test-helpers.ts +++ b/gitnexus/test/utils/hook-test-helpers.ts @@ -72,6 +72,71 @@ function hasGitNexusLauncher(dir: string): boolean { }); } +// ─── Fake tool dir for the DB-owner probe (shared by unit + e2e) ──── +// +// Builds a temp bin dir holding fake `gitnexus`, `lsof`, and `ps` executables so +// a hook spawned with hookEnv(binDir) sees a deterministic DB-owner probe result +// (and a marker-writing fake CLI) without touching the real process table. + +// Module-private: only createHookToolDir writes these fakes; callers use the +// higher-level createHookToolDir, never writeExecutable directly. +function writeExecutable(filePath: string, content: string) { + fs.writeFileSync(filePath, content, { mode: 0o755 }); +} + +export function createHookToolDir(options: { + gitnexusStderr?: string; + gitnexusMarkerPath?: string; + lsofOutput?: string; + lsofOutputLines?: string[]; + psOutput?: string; + psOutputByPid?: Record; + lsofSleepMs?: number; +}) { + const binDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-hook-bin-')); + const gitnexusStderr = JSON.stringify(options.gitnexusStderr ?? ''); + const markerPath = JSON.stringify(options.gitnexusMarkerPath ?? ''); + + const fakeGitNexus = `#!/usr/bin/env node\nconst fs = require('fs');\nconst marker = ${markerPath};\nif (marker) fs.writeFileSync(marker, 'called');\nprocess.stderr.write(${gitnexusStderr});\n`; + writeExecutable(path.join(binDir, 'gitnexus'), fakeGitNexus); + writeExecutable(path.join(binDir, 'gitnexus-cli.js'), fakeGitNexus); + + const lsofOutput = + options.lsofOutputLines != null + ? options.lsofOutputLines.join('\n') + (options.lsofOutputLines.length ? '\n' : '') + : (options.lsofOutput ?? ''); + const lsofBody = + options.lsofSleepMs != null + ? `#!/usr/bin/env node\nsetTimeout(() => {}, ${Number(options.lsofSleepMs)});\n` + : `#!/usr/bin/env node\nprocess.stdout.write(${JSON.stringify(lsofOutput)});\nprocess.exit(0);\n`; + writeExecutable(path.join(binDir, 'lsof'), lsofBody); + + const psBody = + options.psOutputByPid != null + ? `#!/usr/bin/env node +const byPid = ${JSON.stringify(options.psOutputByPid)}; +const args = process.argv; +const p = args[args.indexOf('-p') + 1]; +process.stdout.write(byPid[p] ?? ''); +process.exit(0); +` + : `#!/usr/bin/env node\nprocess.stdout.write(${JSON.stringify(options.psOutput ?? '')});\nprocess.exit(0);\n`; + writeExecutable(path.join(binDir, 'ps'), psBody); + + return binDir; +} + +/** A full env that points a spawned hook at the fake tool dir from createHookToolDir. */ +export function hookEnv(binDir: string) { + return { + ...process.env, + PATH: `${binDir}${path.delimiter}${process.env.PATH || ''}`, + GITNEXUS_HOOK_CLI_PATH: path.join(binDir, 'gitnexus-cli.js'), + GITNEXUS_HOOK_LSOF_PATH: path.join(binDir, 'lsof'), + GITNEXUS_HOOK_PS_PATH: path.join(binDir, 'ps'), + }; +} + /** * The current PATH with every dir that contains a `gitnexus` launcher removed, so * a test box that already has gitnexus installed cannot make the assertion pass