mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-14 04:08:05 +00:00
fix regression review merge-base guidance
This commit is contained in:
kigland
parent
191a954bf6
commit
11063a60e0
3 changed files with 116 additions and 31 deletions
|
|
@ -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: "<merge-base SHA>"})\` or \`${runner} detect-changes --scope compare --base-ref "<merge-base SHA>" --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: <N>\` — 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 <N> --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 {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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: "<merge-base SHA>"})',
|
||||
);
|
||||
expect(content).toContain('--scope compare --base-ref "<merge-base SHA>" --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: "<merge-base SHA>"');
|
||||
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: "<merge-base SHA>"');
|
||||
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 "<merge-base SHA>" --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:start -->
|
||||
# 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: "<merge-base SHA>"');
|
||||
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 = `<!-- gitnexus:start -->
|
||||
Custom target branch "main" and base_ref: "main" notes.
|
||||
<!-- gitnexus:end -->
|
||||
`;
|
||||
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 = `<!-- gitnexus:start -->
|
||||
Before release, run \`detect_changes({scope: "compare", base_ref: "main"})\` and inspect every returned flow.
|
||||
<!-- gitnexus:end -->
|
||||
`;
|
||||
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: "<merge-base SHA>"');
|
||||
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 = `<!-- gitnexus:start -->
|
||||
For regression review: \`detect_changes({scope: "compare", base_ref: "main"})\` or \`.external-index/run.cjs detect-changes --scope compare --base-ref "main" --repo .\`.
|
||||
<!-- gitnexus:end -->
|
||||
`;
|
||||
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: "<merge-base SHA>"');
|
||||
} 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: "<merge-base SHA>"');
|
||||
expect(content).not.toContain('base_ref: "release/1.0"');
|
||||
}
|
||||
} finally {
|
||||
await fs.rm(subDir, { recursive: true, force: true });
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue