From 11063a60e0deb2cd34873483b2ae14ea5a2a21de Mon Sep 17 00:00:00 2001 From: kigland Date: Wed, 7 Oct 2026 23:34:19 +0800 Subject: [PATCH] fix regression review merge-base guidance --- gitnexus/src/cli/ai-context.ts | 46 +++++++++---- gitnexus/src/cli/analyze.ts | 2 +- gitnexus/test/unit/ai-context.test.ts | 99 ++++++++++++++++++++++----- 3 files changed, 116 insertions(+), 31 deletions(-) diff --git a/gitnexus/src/cli/ai-context.ts b/gitnexus/src/cli/ai-context.ts index 89bf912f0..f2f1c52fb 100644 --- a/gitnexus/src/cli/ai-context.ts +++ b/gitnexus/src/cli/ai-context.ts @@ -117,6 +117,11 @@ export function markdownSafeBranch(branch: string): string { return branch.replace(/`/g, ''); } +function regressionReviewGuidance(defaultBranch: string, runner: string): string { + const target = JSON.stringify(markdownSafeBranch(defaultBranch)); + return `For regression review of target branch ${target}, pass the merge-base SHA resolved by \`gitnexus-review\`: \`detect_changes({scope: "compare", base_ref: ""})\` or \`${runner} detect-changes --scope compare --base-ref "" --repo .\`.`; +} + /** Options for {@link generateGitNexusContent} (collapsed from positional * params, #2188 review — six `undefined`s to reach `hasPdg` was the smell). */ export interface GitNexusContentOptions { @@ -129,9 +134,9 @@ export interface GitNexusContentOptions { * the available runner (global `gitnexus` → `pnpm dlx` → `bunx` → `npx`) at * call time. */ runnerPath?: string; - /** Default branch for the regression-compare example (#243). Configurable so - * projects on `develop`/`master`/etc. don't get `base_ref: "main"` rewritten - * back over their fix on every analyze. The value is embedded inside a + /** Default target branch for the regression-review guidance (#243, #3509). + * This is a target hint only: compare scope must receive the exact merge-base + * SHA, never the moving branch tip. The value is embedded inside a * Markdown inline-code span: validateBranchName rejects backticks upstream, * and `markdownSafeBranch` strips any remaining backtick here as defense in * depth, so JSON.stringify's quote/escape handling is sufficient and the @@ -232,7 +237,7 @@ This project is indexed by GitNexus as **${projectName}**${noStats ? '' : ` (${s ? ` For unified PDG impact, add \`mode: "pdg"\` with optional \`line: \` — it returns statement-level \`affectedStatements\` over CDG + REACHING_DEF and inter-procedural symbols in \`interproceduralByDepth\`/\`byDepth\`; no-layer/degraded PDG results are UNKNOWN-risk notes (\`--pdg\` layer). CLI equivalent: \`${runner} impact "symbolName" --direction upstream --mode pdg --line --repo .\`.` : '' } -- **MUST analyze graph changes before committing.** Use \`detect_changes({scope: "all"})\` (MCP) or \`${runner} detect-changes --scope all --repo .\` (CLI fallback). \`partial: true\` or \`truncated: true\` is not a clean check — a zero means unseen, not unaffected; re-run it. For regression review: \`detect_changes({scope: "compare", base_ref: ${JSON.stringify(markdownSafeBranch(defaultBranch))}})\` or \`${runner} detect-changes --scope compare --base-ref ${JSON.stringify(markdownSafeBranch(defaultBranch))} --repo .\`. +- **MUST analyze graph changes before committing.** Use \`detect_changes({scope: "all"})\` (MCP) or \`${runner} detect-changes --scope all --repo .\` (CLI fallback). \`partial: true\` or \`truncated: true\` is not a clean check — a zero means unseen, not unaffected; re-run it. ${regressionReviewGuidance(defaultBranch, runner)} - MUST warn on HIGH/CRITICAL \`risk\` pre-edit; never use \`riskSharedAxes\` to waive a HIGH/CRITICAL \`risk\` warning. Compare File/symbol: MCP File omits axes; Graph-RAG expands File. - **MUST treat \`risk: UNKNOWN\` as unresolved, not as low.** An empty caller set is not evidence the symbol is unused — it can also mean the callers are not resolvable by the index (plain-object property access, dynamic dispatch, cross-language calls). \`impact\` pairs \`UNKNOWN\` with a \`riskNote\` saying so. Confirm with a text search before treating the symbol as safe to change or delete; do not proceed on the strength of a zero. - **MUST use \`query({search_query: "concept"})\` for concepts/flows, \`context({name: "symbolName"})\` for a named symbol, or \`impact\` for blast radius, on read-only callers, dependencies, imports, or execution flow.** Graph first; text search only for empty/\`UNKNOWN\`/literals.${ @@ -726,8 +731,8 @@ export async function generateAIContextFiles( } /** - * Refresh only the `base_ref: "..."` value inside the GitNexus block of an - * already-generated AGENTS.md / CLAUDE.md, in place (#1996 tri-review P2). + * Refresh the regression-review target inside the GitNexus block of an + * already-generated AGENTS.md / CLAUDE.md, in place (#1996 P2, #3509). * * The `alreadyUpToDate` analyze fast path returns before the normal * {@link generateAIContextFiles} call, so a changed `.gitnexusrc` defaultBranch @@ -736,8 +741,10 @@ export async function generateAIContextFiles( * block — including community-skill rows written by a prior `--skills` run — * rather than regenerating (which would drop those rows on a no-`--skills` run). * - * Best-effort: missing files, a missing/blank block, or a block with no - * `base_ref` line (e.g. a user-trimmed keep block) are silently skipped. Writes + * Legacy blocks that passed the branch tip as `base_ref` are migrated to the + * merge-base guidance at the same time. Best-effort: missing files, a + * missing/blank block, or a block with no generated regression sentence are + * silently skipped. Writes * only when the value actually changes, so a routine up-to-date run is a no-op. */ export async function refreshBaseRefLine( @@ -746,7 +753,7 @@ export async function refreshBaseRefLine( options?: { skipAgentsMd?: boolean }, ): Promise<{ files: string[] }> { if (options?.skipAgentsMd) return { files: [] }; - const replacement = `base_ref: ${JSON.stringify(markdownSafeBranch(defaultBranch))}`; + const targetReplacement = `For regression review of target branch ${JSON.stringify(markdownSafeBranch(defaultBranch))}`; const updated: string[] = []; for (const name of ['AGENTS.md', 'CLAUDE.md']) { const filePath = path.join(repoPath, name); @@ -763,9 +770,24 @@ export async function refreshBaseRefLine( if (endIdx === -1 || endIdx <= startIdx) continue; const blockEnd = endIdx + GITNEXUS_END_MARKER.length; const block = content.substring(startIdx, blockEnd); - // Only the generated regression example carries a base_ref line, and only - // one per block; replace its quoted value while leaving the rest untouched. - const newBlock = block.replace(/base_ref: "(?:[^"\\]|\\.)*"/, replacement); + // Preserve the runner emitted with the block. Its location follows the + // configured storage path, so assuming `.gitnexus/run.cjs` would break the + // CLI fallback when migrating guidance generated for an external store. + const legacyRunner = + block.match(/or `([^`\n]+?) detect-changes --scope compare --base-ref\b/)?.[1] ?? + '.gitnexus/run.cjs'; + const mergeBaseGuidance = regressionReviewGuidance(defaultBranch, legacyRunner); + let newBlock = block.replace( + /For regression review of target branch "(?:[^"\\]|\\.)*"/, + targetReplacement, + ); + if (newBlock === block) { + // Migrate the legacy generated sentence. + newBlock = block.replace( + /For regression review:(?=[^\n]*base_ref:)[^\n]*/, + mergeBaseGuidance, + ); + } if (newBlock === block) continue; // no base_ref line present, or already current const newContent = content.substring(0, startIdx) + newBlock + content.substring(blockEnd); try { diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index 00e9a9d62..483b889de 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -1663,7 +1663,7 @@ const analyzeCommandImpl = async ( } if (baseRefRefreshed.length > 0) { console.log( - ` Updated base_ref to "${resolvedDefaultBranch}" in ${baseRefRefreshed.join(', ')}\n`, + ` Updated regression-review target to "${resolvedDefaultBranch}" in ${baseRefRefreshed.join(', ')}\n`, ); } // #2639: opt-in self-commit of any AGENTS.md/CLAUDE.md churn from this diff --git a/gitnexus/test/unit/ai-context.test.ts b/gitnexus/test/unit/ai-context.test.ts index c411d7ec5..36d22a93d 100644 --- a/gitnexus/test/unit/ai-context.test.ts +++ b/gitnexus/test/unit/ai-context.test.ts @@ -175,8 +175,11 @@ describe('generateAIContextFiles', () => { ); expect(content).toContain('detect_changes({scope: "all"})'); expect(content).toContain('node .gitnexus/run.cjs detect-changes --scope all --repo .'); - expect(content).toContain('detect_changes({scope: "compare", base_ref: "develop"})'); - expect(content).toContain('--scope compare --base-ref "develop" --repo .'); + expect(content).toContain('target branch "develop"'); + expect(content).toContain( + 'detect_changes({scope: "compare", base_ref: ""})', + ); + expect(content).toContain('--scope compare --base-ref "" --repo .'); expect(content).toContain('Never substitute grep for graph analysis'); expect(content).not.toContain('gitnexus impact --target'); } @@ -1458,19 +1461,22 @@ Indexed as **placeholder** (1 symbols, 1 relationships, 1 execution flows). Cust }); // ────────────────────────────────────────────────────────────────── - // Configurable default branch in the regression example (#243) + // Configurable review target and exact merge-base guidance (#243, #3509) // ────────────────────────────────────────────────────────────────── - it('generated regression-compare example uses the configured default branch (#243)', () => { + it('uses the configured branch only as a review target, never as base_ref (#243, #3509)', () => { const stats = { nodes: 50, edges: 100, processes: 5 }; const develop = generateGitNexusContent('P', stats, { defaultBranch: 'develop' }); - expect(develop).toContain('base_ref: "develop"'); - expect(develop).not.toContain('base_ref: "main"'); + expect(develop).toContain('target branch "develop"'); + expect(develop).toContain('base_ref: ""'); + expect(develop).not.toContain('base_ref: "develop"'); }); - it('defaults the regression-compare example to "main" when no branch is configured (#243)', () => { + it('defaults the review target to main without comparing to the main tip (#243, #3509)', () => { const content = generateGitNexusContent('P', { nodes: 50, edges: 100, processes: 5 }); - expect(content).toContain('base_ref: "main"'); + expect(content).toContain('target branch "main"'); + expect(content).toContain('base_ref: ""'); + expect(content).not.toContain('base_ref: "main"'); }); it('references MCP tools by their registered (unprefixed) names (#2059)', () => { @@ -1492,15 +1498,15 @@ Indexed as **placeholder** (1 symbols, 1 relationships, 1 execution flows). Cust // A branch name with a double-quote must be JSON-escaped, not concatenated // raw, so it stays inside the inline code span. const content = generateGitNexusContent('P', { nodes: 1 }, { defaultBranch: 'we"ird' }); - expect(content).toContain('base_ref: "we\\"ird"'); - expect(content).toContain('--base-ref "we\\"ird" --repo .'); + expect(content).toContain('target branch "we\\"ird"'); + expect(content).toContain('--base-ref "" --repo .'); }); it('a backtick branch cannot break the generated Markdown code span (#1996 P1)', () => { // The branch is embedded inside a backtick inline-code span; a stray // backtick would close it early. markdownSafeBranch strips it at the sink. const content = generateGitNexusContent('P', { nodes: 1 }, { defaultBranch: 'main`evil' }); - const line = content.split('\n').find((l) => l.includes('base_ref'))!; + const line = content.split('\n').find((l) => l.includes('target branch'))!; // Even backtick count ⇒ every span is balanced (the regression line opens // and closes exactly one). expect((line.match(/`/g) || []).length % 2).toBe(0); @@ -1508,7 +1514,7 @@ Indexed as **placeholder** (1 symbols, 1 relationships, 1 execution flows). Cust expect(markdownSafeBranch('a`b`c')).toBe('abc'); }); - it('refreshBaseRefLine updates base_ref in place, preserving the rest of the block (#1996 P2)', async () => { + it('refreshBaseRefLine migrates legacy guidance and preserves the rest of the block (#1996 P2, #3509)', async () => { const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-baseref-')); try { // Seed a realistic block: a configured base_ref "main" plus a community @@ -1518,7 +1524,7 @@ Indexed as **placeholder** (1 symbols, 1 relationships, 1 execution flows). Cust # GitNexus — Code Intelligence -- run \`detect_changes({scope: "compare", base_ref: "main"})\`. +For regression review: \`detect_changes({scope: "compare", base_ref: "main"})\` or \`.gitnexus/run.cjs detect-changes --scope compare --base-ref "main" --repo .\`. | Task | Read this skill file | |------|---------------------| @@ -1534,7 +1540,9 @@ Indexed as **placeholder** (1 symbols, 1 relationships, 1 execution flows). Cust for (const f of ['AGENTS.md', 'CLAUDE.md']) { const after = await fs.readFile(path.join(dir, f), 'utf-8'); - expect(after).toContain('base_ref: "develop"'); + expect(after).toContain('target branch "develop"'); + expect(after).toContain('base_ref: ""'); + expect(after).not.toContain('base_ref: "develop"'); expect(after).not.toContain('base_ref: "main"'); // The community-skill row (and everything else) is preserved. expect(after).toContain('.claude/skills/gitnexus-area-auth/SKILL.md'); @@ -1548,14 +1556,14 @@ Indexed as **placeholder** (1 symbols, 1 relationships, 1 execution flows). Cust const skipped = await refreshBaseRefLine(dir, 'master', { skipAgentsMd: true }); expect(skipped.files).toEqual([]); expect(await fs.readFile(path.join(dir, 'AGENTS.md'), 'utf-8')).toContain( - 'base_ref: "develop"', + 'target branch "develop"', ); } finally { await fs.rm(dir, { recursive: true, force: true }); } }); - it('refreshBaseRefLine is a no-op when there is no base_ref line or no file (#1996 P2)', async () => { + it('refreshBaseRefLine is a no-op when there is no generated review line or no file (#1996 P2)', async () => { const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-baseref-noop-')); try { // No AGENTS.md/CLAUDE.md at all → no files updated, no throw. @@ -1569,6 +1577,60 @@ Indexed as **P**. Custom. await fs.writeFile(path.join(dir, 'CLAUDE.md'), seed, 'utf-8'); expect((await refreshBaseRefLine(dir, 'develop')).files).toEqual([]); expect(await fs.readFile(path.join(dir, 'CLAUDE.md'), 'utf-8')).toBe(seed); + + const custom = ` +Custom target branch "main" and base_ref: "main" notes. + +`; + await fs.writeFile(path.join(dir, 'AGENTS.md'), custom, 'utf-8'); + expect((await refreshBaseRefLine(dir, 'develop')).files).toEqual([]); + expect(await fs.readFile(path.join(dir, 'AGENTS.md'), 'utf-8')).toBe(custom); + + // A user-authored compare instruction is not generated migration state, + // even though it contains the same tool call and base_ref shape. + const compareInstruction = ` +Before release, run \`detect_changes({scope: "compare", base_ref: "main"})\` and inspect every returned flow. + +`; + await fs.writeFile(path.join(dir, 'CLAUDE.md'), compareInstruction, 'utf-8'); + expect((await refreshBaseRefLine(dir, 'develop')).files).toEqual([]); + expect(await fs.readFile(path.join(dir, 'CLAUDE.md'), 'utf-8')).toBe(compareInstruction); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); + + it('refreshBaseRefLine updates current merge-base guidance idempotently (#3509)', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-review-target-')); + try { + const seed = generateGitNexusContent('P', { nodes: 1 }, { defaultBranch: 'main' }); + await fs.writeFile(path.join(dir, 'CLAUDE.md'), seed, 'utf-8'); + + expect((await refreshBaseRefLine(dir, 'develop')).files).toEqual(['CLAUDE.md']); + const after = await fs.readFile(path.join(dir, 'CLAUDE.md'), 'utf-8'); + expect(after).toContain('target branch "develop"'); + expect(after).toContain('base_ref: ""'); + expect(after).not.toContain('base_ref: "develop"'); + expect((await refreshBaseRefLine(dir, 'develop')).files).toEqual([]); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); + + it('preserves a custom runner path while migrating legacy guidance (#3509)', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-review-runner-')); + try { + const seed = ` +For regression review: \`detect_changes({scope: "compare", base_ref: "main"})\` or \`.external-index/run.cjs detect-changes --scope compare --base-ref "main" --repo .\`. + +`; + await fs.writeFile(path.join(dir, 'CLAUDE.md'), seed, 'utf-8'); + + expect((await refreshBaseRefLine(dir, 'develop')).files).toEqual(['CLAUDE.md']); + const after = await fs.readFile(path.join(dir, 'CLAUDE.md'), 'utf-8'); + expect(after).toContain('`.external-index/run.cjs detect-changes'); + expect(after).not.toContain('`.gitnexus/run.cjs detect-changes'); + expect(after).toContain('base_ref: ""'); } finally { await fs.rm(dir, { recursive: true, force: true }); } @@ -1585,8 +1647,9 @@ Indexed as **P**. Custom. }); for (const f of ['CLAUDE.md', 'AGENTS.md']) { const content = await fs.readFile(path.join(subDir, f), 'utf-8'); - expect(content).toContain('base_ref: "release/1.0"'); - expect(content).not.toContain('base_ref: "main"'); + expect(content).toContain('target branch "release/1.0"'); + expect(content).toContain('base_ref: ""'); + expect(content).not.toContain('base_ref: "release/1.0"'); } } finally { await fs.rm(subDir, { recursive: true, force: true });