From 591867e72d94fc01addcb9a8d97e8ededc79e9ba Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 11:25:35 +1000 Subject: [PATCH 01/18] feat(lib): add resolveDefaultBranch helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sync helper that reads `git symbolic-ref --short refs/remotes/origin/HEAD` to find the workspace's integration branch, falling back to `main` when origin/HEAD is unset, dangling, or the workspace isn't a git repo. Mirror of the proven async pattern at packages/vscode/src/commands/view-diff.ts:261-272. Foundation for upcoming consult fixes: #777 (Defect B Layer 1 hardcodes `main` as the merge-base source in two CLI sites) and #784 (two-dot diff semantics stack on top of the wrong base to over-include base-branch commits in the reviewer's "scope"). This commit is pure addition — no call sites changed yet. --- .../src/__tests__/default-branch.test.ts | 74 +++++++++++++++++++ packages/codev/src/lib/default-branch.ts | 25 +++++++ 2 files changed, 99 insertions(+) create mode 100644 packages/codev/src/__tests__/default-branch.test.ts create mode 100644 packages/codev/src/lib/default-branch.ts diff --git a/packages/codev/src/__tests__/default-branch.test.ts b/packages/codev/src/__tests__/default-branch.test.ts new file mode 100644 index 00000000..8d89ea0a --- /dev/null +++ b/packages/codev/src/__tests__/default-branch.test.ts @@ -0,0 +1,74 @@ +/** + * Unit tests for resolveDefaultBranch — the integration-branch helper used + * by consult to anchor merge-base lookups. + * + * Covers issues #777 (Defect B Layer 1) and #784: on repos whose default + * branch isn't `main`, consult was hardcoding `main` and producing phantom + * scope-creep verdicts. The helper reads `origin/HEAD` to find the real + * integration branch, falling back to `main` when the ref is unset/dangling. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { execSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import * as os from 'node:os'; + +import { resolveDefaultBranch } from '../lib/default-branch.js'; + +describe('resolveDefaultBranch', () => { + let tmpDir: string; + let originDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'default-branch-')); + originDir = fs.mkdtempSync(path.join(os.tmpdir(), 'default-branch-origin-')); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + fs.rmSync(originDir, { recursive: true, force: true }); + }); + + function initWithOriginHead(defaultBranchName: string): void { + // Bare origin so we can set its HEAD without messing with checkouts. + execSync(`git init --bare -b ${defaultBranchName} "${originDir}"`); + + execSync(`git init -b ${defaultBranchName}`, { cwd: tmpDir }); + execSync('git config user.email "test@test.com"', { cwd: tmpDir }); + execSync('git config user.name "Test"', { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, 'README.md'), '# test'); + execSync('git add README.md', { cwd: tmpDir }); + execSync('git commit -m "initial"', { cwd: tmpDir }); + execSync(`git remote add origin "${originDir}"`, { cwd: tmpDir }); + execSync(`git push origin ${defaultBranchName}`, { cwd: tmpDir }); + execSync(`git remote set-head origin ${defaultBranchName}`, { cwd: tmpDir }); + } + + it('returns the configured default branch when origin/HEAD is set', () => { + initWithOriginHead('ci'); + expect(resolveDefaultBranch(tmpDir)).toBe('ci'); + }); + + it('returns "main" when origin/HEAD is unset (no remote)', () => { + execSync('git init -b main', { cwd: tmpDir }); + execSync('git config user.email "test@test.com"', { cwd: tmpDir }); + execSync('git config user.name "Test"', { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, 'README.md'), '# test'); + execSync('git add README.md', { cwd: tmpDir }); + execSync('git commit -m "initial"', { cwd: tmpDir }); + expect(resolveDefaultBranch(tmpDir)).toBe('main'); + }); + + it('returns "main" when the workspace is not a git repo', () => { + expect(resolveDefaultBranch(tmpDir)).toBe('main'); + }); + + it('returns "main" when origin/HEAD points to a deleted remote', () => { + initWithOriginHead('develop'); + // Remove the origin remote — `refs/remotes/origin/HEAD` goes with it, + // so symbolic-ref fails and the helper falls back. + execSync('git remote remove origin', { cwd: tmpDir }); + expect(resolveDefaultBranch(tmpDir)).toBe('main'); + }); +}); diff --git a/packages/codev/src/lib/default-branch.ts b/packages/codev/src/lib/default-branch.ts new file mode 100644 index 00000000..8f33f3d5 --- /dev/null +++ b/packages/codev/src/lib/default-branch.ts @@ -0,0 +1,25 @@ +/** + * Resolve the integration branch for a workspace via `origin/HEAD`, + * falling back to `'main'` when the symbolic-ref is unset, dangling, or + * the remote doesn't exist. + * + * Sync mirror of the async pattern at + * `packages/vscode/src/commands/view-diff.ts:261-272`; consult is sync + * everywhere so the helper is too. + */ + +import { execSync } from 'node:child_process'; + +export function resolveDefaultBranch(workspaceRoot: string): string { + try { + const stdout = execSync( + 'git symbolic-ref --short refs/remotes/origin/HEAD', + { cwd: workspaceRoot, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'ignore'] }, + ); + const ref = stdout.trim().replace(/^origin\//, ''); + if (ref) return ref; + } catch { + // origin/HEAD not set, no remote, or dangling — fall through. + } + return 'main'; +} From a88813688f12c3e0aff87a5145795e7e532c6ec7 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 11:27:02 +1000 Subject: [PATCH 02/18] [Bugfix #777][Bugfix #784] fix(consult): resolve default branch + use three-dot diff semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #784 (consult: impl-review uses two-dot ..HEAD diff semantics). Partial fix for #777 (Defect B Layer 1 + Defect C). ## Root cause `consult --type impl` produced phantom "scope creep" verdicts on PRs against non-`main` integration branches and on branches behind their base. Two stacking mechanisms in consult/index.ts: 1. Hardcoded `main` as merge-base source (two sites). On a repo whose integration branch is `ci`, the diff fed to the reviewer swept in every `ci`-only commit and attributed them to the builder. 2. Two-dot diff operator at the architect-mode emit + a "Do NOT rely on git diffs" prompt directive that pushed reviewers to compute their own diff (defaulting to two-dot in agent training data). Two-dot reverse-includes base-branch commits since the branch base. ## Fix - Replace literal `'main'` at lines 989 and 1297 with `resolveDefaultBranch(workspaceRoot)` from the new lib helper. - Switch the architect-mode emit from `..origin/` to `...origin/` (three-dot). Semantically equivalent when the LHS is the merge-base SHA, but reads as the canonical PR-diff semantics and stays correct if the LHS shape ever changes. - Replace the "Do NOT rely on git diffs" prompt line with a positive scope-anchor: the file list is the canonical scope (three-dot diff against the PR's base, equivalent to GitHub's PR view); reviewers should not flag files outside it. - Append a three-dot directive to the "explore the filesystem" fallback so reviewers computing their own diff still get the right operator. - Wrap the architect-mode merge-base call in try/catch (pre-existing latent bug — uncaught crash on dangling refs). On failure, the reviewer drops into the empty-changedFiles fallback rather than killing the whole command. ## What this does NOT fix - Layer 2 (protocol-doc instructions that literally say `git diff main` to executing agents). Separate commit. - Defect A (workspace-rooted spec/plan resolution). Separate commit. ## Preserved The bugfix #280 contract — single-arg `git diff ` in `getDiffStat` is still working-tree diff, so uncommitted changes in builder worktrees keep showing up. All 49 existing consult-related tests pass. --- packages/codev/src/commands/consult/index.ts | 34 +++++++++++++++++--- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/packages/codev/src/commands/consult/index.ts b/packages/codev/src/commands/consult/index.ts index fcda82b4..20d4c5aa 100644 --- a/packages/codev/src/commands/consult/index.ts +++ b/packages/codev/src/commands/consult/index.ts @@ -15,6 +15,7 @@ import chalk from 'chalk'; import { query as claudeQuery } from '@anthropic-ai/claude-agent-sdk'; import { Codex } from '@openai/codex-sdk'; import { readCodevFile, findWorkspaceRoot } from '../../lib/skeleton.js'; +import { resolveDefaultBranch } from '../../lib/default-branch.js'; import { getResolver } from '../porch/artifacts.js'; import { MetricsDB } from './metrics.js'; import { extractUsage, extractReviewText, type SDKResultLike, type UsageData } from './usage-extractor.js'; @@ -986,7 +987,8 @@ function buildImplQuery( let diffStat = ''; let changedFiles: string[] = []; try { - const ref = diffRef ?? execSync('git merge-base HEAD main', { cwd: workspaceRoot, encoding: 'utf-8' }).trim(); + const defaultBranch = resolveDefaultBranch(workspaceRoot); + const ref = diffRef ?? execSync(`git merge-base HEAD ${defaultBranch}`, { cwd: workspaceRoot, encoding: 'utf-8' }).trim(); const result = getDiffStat(workspaceRoot, ref); diffStat = result.stat; changedFiles = result.files; @@ -1025,9 +1027,15 @@ function buildImplQuery( query += `\n\n## How to Review\n`; query += `**Read the changed files from disk** to review their actual content. You have full filesystem access.\n`; query += `For each file listed above, read it and evaluate the implementation against the spec/plan.\n`; - query += `Do NOT rely on git diffs to determine the current state of code — diffs miss uncommitted changes in worktrees.\n`; + query += `\n### Scope is the file list above\n`; + query += `The files above are the canonical scope of this PR (three-dot diff against the PR's base, equivalent to GitHub's PR view). `; + query += `If this PR targets an integration branch, the file list reflects the diff against that integration branch — not necessarily \`main\`. `; + query += `Do not flag files outside this list, even if you see other changes in the worktree. `; + query += `If you compute a diff yourself, use \`git diff ...HEAD\` (three-dot) — never two-dot, which over-includes commits the base branch picked up since this branch was created.\n`; } else { - query += `\n## Instructions\n\nExplore the filesystem to find and review the implementation changes.\n`; + query += `\n## Instructions\n\n`; + query += `Explore the filesystem to find and review the implementation changes. `; + query += `If you compute a diff yourself, use \`git diff ...HEAD\` (three-dot, anchored at the merge-base) — never two-dot, which over-includes commits the base branch picked up since this branch was created.\n`; } query += ` @@ -1294,13 +1302,29 @@ function resolveArchitectQuery(workspaceRoot: string, type: string, options: Con } catch { // May already be fetched } - const mergeBase = execSync(`git merge-base main origin/${pr.headRefName}`, { cwd: workspaceRoot, encoding: 'utf-8' }).trim(); + const defaultBranch = resolveDefaultBranch(workspaceRoot); + let diffRef: string | undefined; + try { + const mergeBase = execSync( + `git merge-base ${defaultBranch} origin/${pr.headRefName}`, + { cwd: workspaceRoot, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'] }, + ).trim(); + // Three-dot is the canonical PR-diff semantics (symmetric difference, + // anchored at the merge-base). Semantically equivalent to two-dot when + // the LHS is already the merge-base SHA, but disambiguates in code review + // and stays correct if the LHS shape ever changes. + diffRef = `${mergeBase}...origin/${pr.headRefName}`; + } catch { + // Merge-base lookup failed (dangling origin/HEAD, default branch not + // locally present, etc.). Let buildImplQuery hit its empty-changedFiles + // fallback rather than crashing the whole command. + } const spec = findSpecContent(workspaceRoot, issueId); const plan = findPlanContent(workspaceRoot, issueId); console.error(`Project: ${issueId} (PR #${pr.number}, branch: ${pr.headRefName})`); if (spec) console.error(`Spec: ${spec.label}`); if (plan) console.error(`Plan: ${plan.label}`); - return buildImplQuery(workspaceRoot, spec, plan, options.planPhase, `${mergeBase}..origin/${pr.headRefName}`); + return buildImplQuery(workspaceRoot, spec, plan, options.planPhase, diffRef); } case 'pr': { From 67bed2667807df2dc3589f98d542c82d1626caac Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 11:30:16 +1000 Subject: [PATCH 03/18] [Bugfix #777] feat(consult): --branch flag and PR-default ref resolver for spec/plan artifacts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #777 Defect A: consult --type {spec,plan,impl} previously resolved spec/plan content from the invoking process's workspace root (architect on the integration branch), not from the builder's branch under review. When the architect ran consult to supply a missing review, reviewers received the stale on-integration-branch artifact and REQUEST_CHANGES'd on findings the reworked artifact had already corrected. ## Changes - New GitRefResolver in commands/porch/artifacts.ts. Same ArtifactResolver interface as LocalResolver/CliResolver; reads via `git ls-tree` + `git show :`. Best-effort `git fetch origin ` at construction for `origin/`-prefixed refs. - New `--branch ` CLI flag on `consult`. Reads spec/plan from that ref instead of the local workspace. - resolveArtifactSource() picks the resolver for architect-mode consults: --branch wins; PR-for-issue defaults to origin/; otherwise falls back to the local resolver with a stderr warning. The architect-mode impl path likewise reads spec/plan from origin/ by default so they match the diff source. - findSpecContent/findPlanContent grow an optional `resolver` param. ## Behavior change Existing impl-review invocations now read spec/plan from origin/ by default instead of the architect's checked-out working tree. This is the intended behavior — the reworked artifact on the builder's branch is the one under review. --- packages/codev/src/cli.ts | 2 + packages/codev/src/commands/consult/index.ts | 96 ++++++++++--- .../codev/src/commands/porch/artifacts.ts | 132 ++++++++++++++++++ 3 files changed, 213 insertions(+), 17 deletions(-) diff --git a/packages/codev/src/cli.ts b/packages/codev/src/cli.ts index 3fa45663..0c095472 100644 --- a/packages/codev/src/cli.ts +++ b/packages/codev/src/cli.ts @@ -176,6 +176,7 @@ program .option('--protocol ', 'Protocol name: spir, aspir, air, bugfix, pir, maintain') .option('-t, --type ', 'Review type: spec, plan, impl, pr, phase, integration') .option('--issue ', 'Issue number (required from architect context)') + .option('--branch ', 'Read spec/plan from this git ref instead of the local workspace (e.g. `origin/builder/777-foo` or `builder/777-foo`). Defaults to the PR\'s head branch when --issue resolves to a PR.') .option('--output ', 'Write consultation output to file (used by porch)') .option('--plan-phase ', 'Scope review to a specific plan phase (used by porch)') .option('--context ', 'Context file with previous iteration feedback (used by porch)') @@ -214,6 +215,7 @@ program protocol: options.protocol, type: options.type, issue: options.issue, + branch: options.branch, output: options.output, planPhase: options.planPhase, context: options.context, diff --git a/packages/codev/src/commands/consult/index.ts b/packages/codev/src/commands/consult/index.ts index 20d4c5aa..50d35910 100644 --- a/packages/codev/src/commands/consult/index.ts +++ b/packages/codev/src/commands/consult/index.ts @@ -16,7 +16,7 @@ import { query as claudeQuery } from '@anthropic-ai/claude-agent-sdk'; import { Codex } from '@openai/codex-sdk'; import { readCodevFile, findWorkspaceRoot } from '../../lib/skeleton.js'; import { resolveDefaultBranch } from '../../lib/default-branch.js'; -import { getResolver } from '../porch/artifacts.js'; +import { getResolver, GitRefResolver, type ArtifactResolver } from '../porch/artifacts.js'; import { MetricsDB } from './metrics.js'; import { extractUsage, extractReviewText, type SDKResultLike, type UsageData } from './usage-extractor.js'; import { executeForgeCommandSync } from '../../lib/forge.js'; @@ -66,6 +66,10 @@ export interface ConsultOptions { protocol?: string; type?: string; issue?: string; + // Read spec/plan from this git ref instead of the local workspace. + // Defaults to the PR's headRefName when --issue resolves to a PR. + // Closes #777 Defect A. + branch?: string; // Porch flags output?: string; planPhase?: string; @@ -204,24 +208,31 @@ function loadDotenv(workspaceRoot: string): void { /** * Find spec content by project ID using the artifact resolver. * Returns a ContentRef with content and label, or null if not found. + * + * Accepts an explicit `resolver` to support reading from a git ref (#777 + * Defect A) instead of the architect's local workspace. When omitted, + * falls back to the workspace-root resolver from `.codev/config.json`. */ -function findSpecContent(workspaceRoot: string, id: string): ContentRef | null { - const resolver = getResolver(workspaceRoot); - const content = resolver.getSpecContent(id, ''); +function findSpecContent(workspaceRoot: string, id: string, resolver?: ArtifactResolver): ContentRef | null { + const r = resolver ?? getResolver(workspaceRoot); + const content = r.getSpecContent(id, ''); if (!content) return null; - const label = resolver.findSpecBaseName(id, '') ?? id; + const label = r.findSpecBaseName(id, '') ?? id; return { content, label }; } /** * Find plan content by project ID using the artifact resolver. * Returns a ContentRef with content and label, or null if not found. + * + * Accepts an explicit `resolver` to support reading from a git ref (#777 + * Defect A) instead of the architect's local workspace. */ -function findPlanContent(workspaceRoot: string, id: string): ContentRef | null { - const resolver = getResolver(workspaceRoot); - const content = resolver.getPlanContent(id, ''); +function findPlanContent(workspaceRoot: string, id: string, resolver?: ArtifactResolver): ContentRef | null { + const r = resolver ?? getResolver(workspaceRoot); + const content = r.getPlanContent(id, ''); if (!content) return null; - const baseName = resolver.findSpecBaseName(id, '') ?? id; + const baseName = r.findSpecBaseName(id, '') ?? id; return { content, label: baseName }; } @@ -1258,6 +1269,48 @@ function resolveBuilderQuery(workspaceRoot: string, type: string, options: Consu } } +/** + * Pick the artifact resolver for an architect-mode consult. + * + * Closes #777 Defect A. Resolution order: + * 1. Explicit `--branch ` → read from that ref. + * 2. PR exists for the issue → read from `origin/`. This is + * the routine "architect supplying missing consult" case. + * 3. Neither → fall back to the local workspace, with a warning so the + * architect knows the verdict may target a stale artifact. + */ +function resolveArtifactSource( + workspaceRoot: string, + issueId: string, + branchOption: string | undefined, +): { resolver: ArtifactResolver; sourceLabel: string } { + if (branchOption) { + return { + resolver: new GitRefResolver(workspaceRoot, branchOption), + sourceLabel: `--branch ${branchOption}`, + }; + } + + try { + const pr = findPRForIssue(workspaceRoot, issueId); + const ref = `origin/${pr.headRefName}`; + return { + resolver: new GitRefResolver(workspaceRoot, ref), + sourceLabel: `${ref} (PR #${pr.number})`, + }; + } catch { + console.error( + `Warning: no PR found for issue #${issueId} and no --branch given; ` + + `reading spec/plan from local workspace. Verdicts may not reflect ` + + `the in-progress version.`, + ); + return { + resolver: getResolver(workspaceRoot), + sourceLabel: 'local workspace', + }; + } +} + /** * Resolve query for architect context (requires --issue) */ @@ -1277,18 +1330,22 @@ function resolveArchitectQuery(workspaceRoot: string, type: string, options: Con switch (type) { case 'spec': { - const spec = findSpecContent(workspaceRoot, issueId); - if (!spec) throw new Error(`Spec ${issueId} not found`); - const plan = findPlanContent(workspaceRoot, issueId); + const { resolver, sourceLabel } = resolveArtifactSource(workspaceRoot, issueId, options.branch); + const spec = findSpecContent(workspaceRoot, issueId, resolver); + if (!spec) throw new Error(`Spec ${issueId} not found at ${sourceLabel}`); + const plan = findPlanContent(workspaceRoot, issueId, resolver); + console.error(`Source: ${sourceLabel}`); console.error(`Spec: ${spec.label}`); if (plan) console.error(`Plan: ${plan.label}`); return buildSpecQuery(spec, plan); } case 'plan': { - const plan = findPlanContent(workspaceRoot, issueId); - if (!plan) throw new Error(`Plan ${issueId} not found`); - const spec = findSpecContent(workspaceRoot, issueId); + const { resolver, sourceLabel } = resolveArtifactSource(workspaceRoot, issueId, options.branch); + const plan = findPlanContent(workspaceRoot, issueId, resolver); + if (!plan) throw new Error(`Plan ${issueId} not found at ${sourceLabel}`); + const spec = findSpecContent(workspaceRoot, issueId, resolver); + console.error(`Source: ${sourceLabel}`); console.error(`Plan: ${plan.label}`); if (spec) console.error(`Spec: ${spec.label}`); return buildPlanQuery(plan, spec); @@ -1319,9 +1376,14 @@ function resolveArchitectQuery(workspaceRoot: string, type: string, options: Con // locally present, etc.). Let buildImplQuery hit its empty-changedFiles // fallback rather than crashing the whole command. } - const spec = findSpecContent(workspaceRoot, issueId); - const plan = findPlanContent(workspaceRoot, issueId); + // Read spec/plan from the PR's branch by default so they match the + // diff source (#777 Defect A). --branch overrides the PR default. + const ref = options.branch ?? `origin/${pr.headRefName}`; + const resolver = new GitRefResolver(workspaceRoot, ref); + const spec = findSpecContent(workspaceRoot, issueId, resolver); + const plan = findPlanContent(workspaceRoot, issueId, resolver); console.error(`Project: ${issueId} (PR #${pr.number}, branch: ${pr.headRefName})`); + console.error(`Source: ${ref}`); if (spec) console.error(`Spec: ${spec.label}`); if (plan) console.error(`Plan: ${plan.label}`); return buildImplQuery(workspaceRoot, spec, plan, options.planPhase, diffRef); diff --git a/packages/codev/src/commands/porch/artifacts.ts b/packages/codev/src/commands/porch/artifacts.ts index 7145ec59..76839c76 100644 --- a/packages/codev/src/commands/porch/artifacts.ts +++ b/packages/codev/src/commands/porch/artifacts.ts @@ -354,6 +354,138 @@ export class CliResolver implements ArtifactResolver { } } +// ============================================================================= +// Git Ref Resolver (reads codev/ artifacts at a specific git ref) +// ============================================================================= + +/** + * Reads codev/specs/, codev/plans/, codev/reviews/, and + * codev/projects//plan.md as they exist on a specific git ref (e.g. + * `origin/builder/777-foo`) — not from the architect's checked-out + * working tree. + * + * Closes #777 Defect A: when an architect runs `consult --type {spec,plan}` + * from the integration-branch checkout to supply a missing review, the + * routine had been reading the stale on-integration-branch artifact instead + * of the builder-branch version under review. This resolver fixes that by + * reading directly from the ref the reviewer is meant to evaluate. + * + * Best-effort `git fetch origin ` is attempted once at construction + * for refs that include an `origin/` prefix; failure is silent so the + * resolver also works with refs that are already locally present + * (including bare branch names like `builder/777-foo`). + */ +export class GitRefResolver implements ArtifactResolver { + constructor( + private workspaceRoot: string, + private ref: string, + ) { + if (ref.startsWith('origin/')) { + const branch = ref.slice('origin/'.length); + try { + execFileSync('git', ['fetch', 'origin', branch], { + cwd: workspaceRoot, + stdio: ['ignore', 'pipe', 'pipe'], + }); + } catch { + // Ref may already be fetched, the remote may be unreachable, or the + // branch may not exist remotely — the subsequent `git show` will + // surface the real failure. + } + } + } + + private listFiles(dir: string): string[] { + try { + const stdout = execFileSync( + 'git', + ['ls-tree', '--name-only', '-r', this.ref, '--', dir], + { cwd: this.workspaceRoot, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'ignore'] }, + ); + return stdout.split('\n').filter(Boolean).map(line => path.basename(line)); + } catch { + return []; + } + } + + private showFile(filePath: string): string | null { + try { + const stdout = execFileSync( + 'git', + ['show', `${this.ref}:${filePath}`], + { + cwd: this.workspaceRoot, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'ignore'], + maxBuffer: 32 * 1024 * 1024, + }, + ); + return stdout; + } catch { + return null; + } + } + + findSpecBaseName(projectId: string, _title: string): string | null { + const files = this.listFiles('codev/specs'); + const specFile = files.find(f => f.endsWith('.md') && matchesProjectId(f, projectId)); + return specFile ? specFile.replace(/\.md$/, '') : null; + } + + getSpecContent(projectId: string, title: string): string | null { + const baseName = this.findSpecBaseName(projectId, title); + if (!baseName) return null; + return this.showFile(`codev/specs/${baseName}.md`); + } + + getPlanContent(projectId: string, _title: string): string | null { + // New location: codev/projects/-/plan.md + try { + const stdout = execFileSync( + 'git', + ['ls-tree', '--name-only', this.ref, '--', 'codev/projects/'], + { cwd: this.workspaceRoot, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'ignore'] }, + ); + const projDirs = stdout.split('\n').filter(Boolean).map(line => path.basename(line)); + const projDir = projDirs.find(d => matchesProjectId(d, projectId)); + if (projDir) { + const content = this.showFile(`codev/projects/${projDir}/plan.md`); + if (content !== null) return content; + } + } catch { /* continue to legacy */ } + + // Legacy location: codev/plans/-*.md + const files = this.listFiles('codev/plans'); + const planFile = files.find(f => f.endsWith('.md') && matchesProjectId(f, projectId)); + if (!planFile) return null; + return this.showFile(`codev/plans/${planFile}`); + } + + getReviewContent(projectId: string, _title: string): string | null { + const files = this.listFiles('codev/reviews'); + const reviewFile = files.find(f => f.endsWith('.md') && matchesProjectId(f, projectId)); + if (!reviewFile) return null; + return this.showFile(`codev/reviews/${reviewFile}`); + } + + hasPreApproval(artifactGlob: string): boolean { + // Strip the leading codev/ root from the glob (e.g. `codev/specs/0073-*.md` + // → `specs/0073-*.md`) and resolve via the appropriate getter. + const typeMatch = artifactGlob.match(/\b(specs|plans|reviews)\b/); + const prefixedMatch = artifactGlob.match(/(?:specs|plans|reviews)\/([a-z]+(?:-[a-z]+)*-\d+)/i); + const numericMatch = artifactGlob.match(/(?:specs|plans|reviews)\/0*(\d+)/); + const projectId = prefixedMatch?.[1] ?? numericMatch?.[1]; + if (!projectId || !typeMatch) return false; + + let content: string | null = null; + if (typeMatch[1] === 'plans') content = this.getPlanContent(projectId, ''); + else if (typeMatch[1] === 'reviews') content = this.getReviewContent(projectId, ''); + else content = this.getSpecContent(projectId, ''); + + return content !== null && isPreApprovedContent(content); + } +} + // ============================================================================= // Factory // ============================================================================= From 22d27e22684e2222ee37324baa304cf631953fd2 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 11:33:44 +1000 Subject: [PATCH 04/18] [Bugfix #777] docs(protocols): use integration branch instead of literal "main" in non-PIR protocols MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #777 Defect B Layer 2 for the non-PIR protocols. PIR was already cleaned in 01f733d3; this commit covers SPIR, ASPIR, AIR, BUGFIX, MAINTAIN. ## Why this needs both trees `codev-skeleton/` is the template that ships to consumer projects via `codev init` / `codev adopt`. Fixing only `codev/protocols/` doesn't propagate to downstream repos. Both trees move together. ## Categories of fix - **Review questions** (5 consult-types/pr-review.md files × 2 trees): "Is the branch up to date with main?" → "...up to date with its base (the integration branch the PR targets)?". On non-main-default repos this question previously always answered "no" and consult typically suggested rebase to main — the wrong action. - **SPIR architect convention** (spir/protocol.md, codev/ only — the skeleton copy already lacked these lines): "Commit to `main` so builder worktrees include the artifact" → "Commit to the integration branch...". - **SPIR/ASPIR builder-prompt literal commands** (skeleton only — codev/ copies omit the Multi-PR Mechanics section): replace literal `git fetch origin main` / `git checkout main` / "Pull main into your worktree" with `` placeholders the builder resolves at runtime. - **BUGFIX protocol commands** (codev/ only — the skeleton copy is already clean): "Verify the fix is on main" → "...on the integration branch"; `git diff --stat main` → `git diff --stat "$(git symbolic-ref --short refs/remotes/origin/HEAD | sed 's|^origin/||')"` so the snippet stays copy-pasteable. ## Verification `grep -rn --include="*.md" -E "(git diff main|git merge-base.*main| git rebase.*main|origin/main|up to date with main|Commit to.*main| push to main|merge to main|checkout main|fetch.*\borigin main\b| Pull main)" codev/protocols codev-skeleton/protocols` returns zero hits in non-PIR protocols after this commit. --- codev-skeleton/protocols/air/consult-types/pr-review.md | 2 +- codev-skeleton/protocols/aspir/builder-prompt.md | 6 +++--- codev-skeleton/protocols/aspir/consult-types/pr-review.md | 2 +- codev-skeleton/protocols/bugfix/consult-types/pr-review.md | 2 +- .../protocols/maintain/consult-types/pr-review.md | 2 +- codev-skeleton/protocols/spir/builder-prompt.md | 6 +++--- codev-skeleton/protocols/spir/consult-types/pr-review.md | 2 +- codev/protocols/air/consult-types/pr-review.md | 2 +- codev/protocols/aspir/consult-types/pr-review.md | 2 +- codev/protocols/bugfix/consult-types/pr-review.md | 2 +- codev/protocols/bugfix/protocol.md | 6 +++--- codev/protocols/maintain/consult-types/pr-review.md | 2 +- codev/protocols/spir/consult-types/pr-review.md | 2 +- codev/protocols/spir/protocol.md | 4 ++-- 14 files changed, 21 insertions(+), 21 deletions(-) diff --git a/codev-skeleton/protocols/air/consult-types/pr-review.md b/codev-skeleton/protocols/air/consult-types/pr-review.md index 8c0f6c55..e291b9c2 100644 --- a/codev-skeleton/protocols/air/consult-types/pr-review.md +++ b/codev-skeleton/protocols/air/consult-types/pr-review.md @@ -34,7 +34,7 @@ If the baked decisions themselves contain contradictions, do not pick one — `R 5. **PR Quality** - Does the PR link to the issue? - Is the PR body review section informative? - - Is the branch up to date with main? + - Is the branch up to date with its base (the integration branch the PR targets)? ## Verdict Format diff --git a/codev-skeleton/protocols/aspir/builder-prompt.md b/codev-skeleton/protocols/aspir/builder-prompt.md index afc46a22..4af70ce4 100644 --- a/codev-skeleton/protocols/aspir/builder-prompt.md +++ b/codev-skeleton/protocols/aspir/builder-prompt.md @@ -74,10 +74,10 @@ The architect MAY request a PR at any point — for spec review, mid-implementat Your worktree is persistent — it survives across PR merges. When the architect asks for sequential PRs (e.g., to slice a large spec into shippable pieces), use this loop: 1. Cut a branch, open a PR, wait for merge -2. After merge: `git fetch origin main && git checkout -b origin/main` +2. After merge: `git fetch origin && git checkout -b origin/` — where `` is the branch the architect targets PRs at (usually `main`; check the open PR's `baseRefName` if unsure) 3. Continue to the next slice, open another PR -**Important**: Do NOT run `git checkout main` — git worktrees cannot check out a branch that's checked out elsewhere. Always branch off `origin/main` via fetch. +**Important**: Do NOT run `git checkout ` — git worktrees cannot check out a branch that's checked out elsewhere. Always branch off `origin/` via fetch. Record PRs: `porch done {{project_id}} --pr --branch ` Record merges: `porch done {{project_id}} --merged ` @@ -85,7 +85,7 @@ Record merges: `porch done {{project_id}} --merged ` ## Verify Phase After the final PR merges, the project enters the **verify** phase. You stay alive through verify: -1. Pull main into your worktree +1. Pull the integration branch into your worktree 2. Run `porch done {{project_id}}` to signal verification is ready 3. The architect approves `verify-approval` when satisfied diff --git a/codev-skeleton/protocols/aspir/consult-types/pr-review.md b/codev-skeleton/protocols/aspir/consult-types/pr-review.md index 048c23f1..7d2ef518 100644 --- a/codev-skeleton/protocols/aspir/consult-types/pr-review.md +++ b/codev-skeleton/protocols/aspir/consult-types/pr-review.md @@ -28,7 +28,7 @@ You are performing a final self-check during the Review phase. The builder has c - Are any new APIs documented? 5. **PR Readiness** - - Is the branch up to date with main? + - Is the branch up to date with its base (the integration branch the PR targets)? - Are commits atomic and well-described? - Is the change diff reasonable in size? diff --git a/codev-skeleton/protocols/bugfix/consult-types/pr-review.md b/codev-skeleton/protocols/bugfix/consult-types/pr-review.md index af7133cf..b617a909 100644 --- a/codev-skeleton/protocols/bugfix/consult-types/pr-review.md +++ b/codev-skeleton/protocols/bugfix/consult-types/pr-review.md @@ -41,7 +41,7 @@ There is **no `codev/specs/`, `codev/plans/`, or `codev/reviews/` file** for a B 6. **PR Hygiene** - Commits use the BUGFIX format: `Fix #: ...` or `[Bugfix #] ...` (**not** `[Spec NNNN][Phase]`). - - Branch is up to date with main (or close enough for clean merge). + - Branch is up to date with its base (or close enough for clean merge). - PR is linked to the issue. ## Out of Scope (Do NOT request changes for) diff --git a/codev-skeleton/protocols/maintain/consult-types/pr-review.md b/codev-skeleton/protocols/maintain/consult-types/pr-review.md index 048c23f1..7d2ef518 100644 --- a/codev-skeleton/protocols/maintain/consult-types/pr-review.md +++ b/codev-skeleton/protocols/maintain/consult-types/pr-review.md @@ -28,7 +28,7 @@ You are performing a final self-check during the Review phase. The builder has c - Are any new APIs documented? 5. **PR Readiness** - - Is the branch up to date with main? + - Is the branch up to date with its base (the integration branch the PR targets)? - Are commits atomic and well-described? - Is the change diff reasonable in size? diff --git a/codev-skeleton/protocols/spir/builder-prompt.md b/codev-skeleton/protocols/spir/builder-prompt.md index 1e5aa286..149c531e 100644 --- a/codev-skeleton/protocols/spir/builder-prompt.md +++ b/codev-skeleton/protocols/spir/builder-prompt.md @@ -74,11 +74,11 @@ The architect MAY request a PR at any point — for spec review, mid-implementat Your worktree is persistent — it survives across PR merges. When the architect asks for sequential PRs (e.g., to slice a large spec into shippable pieces), use this loop: 1. Cut a branch, open a PR, wait for merge -2. After merge: `git fetch origin main && git checkout -b origin/main` +2. After merge: `git fetch origin && git checkout -b origin/` — where `` is the branch the architect targets PRs at (usually `main`; check the open PR's `baseRefName` if unsure) 3. Continue to the next slice, open another PR 4. Repeat -**Important**: Do NOT run `git checkout main` — git worktrees cannot check out a branch that's checked out elsewhere. Always branch off `origin/main` via fetch. +**Important**: Do NOT run `git checkout ` — git worktrees cannot check out a branch that's checked out elsewhere. Always branch off `origin/` via fetch. Record PRs in status.yaml: `porch done {{project_id}} --pr --branch ` Record merges: `porch done {{project_id}} --merged ` @@ -86,7 +86,7 @@ Record merges: `porch done {{project_id}} --merged ` ## Verify Phase After the final PR merges, the project enters the **verify** phase. You stay alive through verify: -1. Pull main into your worktree +1. Pull the integration branch into your worktree 2. Run `porch done {{project_id}}` to signal verification is ready 3. The architect approves `verify-approval` when satisfied diff --git a/codev-skeleton/protocols/spir/consult-types/pr-review.md b/codev-skeleton/protocols/spir/consult-types/pr-review.md index 048c23f1..7d2ef518 100644 --- a/codev-skeleton/protocols/spir/consult-types/pr-review.md +++ b/codev-skeleton/protocols/spir/consult-types/pr-review.md @@ -28,7 +28,7 @@ You are performing a final self-check during the Review phase. The builder has c - Are any new APIs documented? 5. **PR Readiness** - - Is the branch up to date with main? + - Is the branch up to date with its base (the integration branch the PR targets)? - Are commits atomic and well-described? - Is the change diff reasonable in size? diff --git a/codev/protocols/air/consult-types/pr-review.md b/codev/protocols/air/consult-types/pr-review.md index 8c0f6c55..e291b9c2 100644 --- a/codev/protocols/air/consult-types/pr-review.md +++ b/codev/protocols/air/consult-types/pr-review.md @@ -34,7 +34,7 @@ If the baked decisions themselves contain contradictions, do not pick one — `R 5. **PR Quality** - Does the PR link to the issue? - Is the PR body review section informative? - - Is the branch up to date with main? + - Is the branch up to date with its base (the integration branch the PR targets)? ## Verdict Format diff --git a/codev/protocols/aspir/consult-types/pr-review.md b/codev/protocols/aspir/consult-types/pr-review.md index 048c23f1..7d2ef518 100644 --- a/codev/protocols/aspir/consult-types/pr-review.md +++ b/codev/protocols/aspir/consult-types/pr-review.md @@ -28,7 +28,7 @@ You are performing a final self-check during the Review phase. The builder has c - Are any new APIs documented? 5. **PR Readiness** - - Is the branch up to date with main? + - Is the branch up to date with its base (the integration branch the PR targets)? - Are commits atomic and well-described? - Is the change diff reasonable in size? diff --git a/codev/protocols/bugfix/consult-types/pr-review.md b/codev/protocols/bugfix/consult-types/pr-review.md index af7133cf..b617a909 100644 --- a/codev/protocols/bugfix/consult-types/pr-review.md +++ b/codev/protocols/bugfix/consult-types/pr-review.md @@ -41,7 +41,7 @@ There is **no `codev/specs/`, `codev/plans/`, or `codev/reviews/` file** for a B 6. **PR Hygiene** - Commits use the BUGFIX format: `Fix #: ...` or `[Bugfix #] ...` (**not** `[Spec NNNN][Phase]`). - - Branch is up to date with main (or close enough for clean merge). + - Branch is up to date with its base (or close enough for clean merge). - PR is linked to the issue. ## Out of Scope (Do NOT request changes for) diff --git a/codev/protocols/bugfix/protocol.md b/codev/protocols/bugfix/protocol.md index c1ca385a..6813786c 100644 --- a/codev/protocols/bugfix/protocol.md +++ b/codev/protocols/bugfix/protocol.md @@ -278,7 +278,7 @@ git commit -m "[Bugfix #N] Test: Add regression test" git pull ``` -2. Verify the fix is on main: +2. Verify the fix is on the integration branch: ```bash git log --oneline -5 # Should see the bugfix commits ``` @@ -324,7 +324,7 @@ Before marking PR ready, the Builder must verify: The "< 300 LOC" threshold is measured as **net diff** (additions + deletions): ```bash -git diff --stat main | tail -1 +git diff --stat "$(git symbolic-ref --short refs/remotes/origin/HEAD | sed 's|^origin/||')" | tail -1 # Example: "3 files changed, 145 insertions(+), 52 deletions(-)" # Net diff = 145 + 52 = 197 LOC ✓ (under 300) ``` @@ -480,7 +480,7 @@ afx send architect "Merged. Ready for cleanup." # 11. Architect cleans up git pull -git log --oneline -3 # Verify fix is on main +git log --oneline -3 # Verify fix is on the integration branch afx cleanup --issue 42 # → Removes .builders/bugfix-42/ # → Deletes origin/builder/bugfix-42-login-fails-when-userna diff --git a/codev/protocols/maintain/consult-types/pr-review.md b/codev/protocols/maintain/consult-types/pr-review.md index 048c23f1..7d2ef518 100644 --- a/codev/protocols/maintain/consult-types/pr-review.md +++ b/codev/protocols/maintain/consult-types/pr-review.md @@ -28,7 +28,7 @@ You are performing a final self-check during the Review phase. The builder has c - Are any new APIs documented? 5. **PR Readiness** - - Is the branch up to date with main? + - Is the branch up to date with its base (the integration branch the PR targets)? - Are commits atomic and well-described? - Is the change diff reasonable in size? diff --git a/codev/protocols/spir/consult-types/pr-review.md b/codev/protocols/spir/consult-types/pr-review.md index 048c23f1..7d2ef518 100644 --- a/codev/protocols/spir/consult-types/pr-review.md +++ b/codev/protocols/spir/consult-types/pr-review.md @@ -28,7 +28,7 @@ You are performing a final self-check during the Review phase. The builder has c - Are any new APIs documented? 5. **PR Readiness** - - Is the branch up to date with main? + - Is the branch up to date with its base (the integration branch the PR targets)? - Are commits atomic and well-described? - Is the change diff reasonable in size? diff --git a/codev/protocols/spir/protocol.md b/codev/protocols/spir/protocol.md index 3a4f8c11..9cf39237 100644 --- a/codev/protocols/spir/protocol.md +++ b/codev/protocols/spir/protocol.md @@ -145,7 +145,7 @@ When filing an issue for SPIR, you can pin architectural decisions you don't wan 11. Final updates based on second review 12. **COMMIT**: "Final approved specification" - Add YAML frontmatter: `approved: ` and `validated: [gemini, codex, claude]` - - Commit to `main` so builder worktrees include the artifact + - Commit to the integration branch (the branch builders branch off) so builder worktrees include the artifact - Porch will detect this metadata and skip the specify phase automatically 13. **GATE CHECK**: Before proceeding to Plan, verify all `spec_*` items complete - If incomplete: Output "⚠️ BLOCKED" with missing items, stop @@ -240,7 +240,7 @@ When filing an issue for SPIR, you can pin architectural decisions you don't wan 10. Final updates based on second review 11. **COMMIT**: "Final approved plan" - Add YAML frontmatter: `approved: ` and `validated: [gemini, codex, claude]` - - Commit to `main` so builder worktrees include the artifact + - Commit to the integration branch (the branch builders branch off) so builder worktrees include the artifact - Porch will detect this metadata and skip the plan phase automatically 12. **REGISTER PHASES**: For each phase defined in the plan: 13. **GATE CHECK**: Before proceeding to Implement, verify all `plan_*` items complete From 361aa5a424b6147ab04a4a2c250af70524c1f96f Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 11:35:14 +1000 Subject: [PATCH 05/18] test(consult): cover non-main default branch, three-dot scope, and ref-based artifact resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression tests for #777 (Defect B Layer 1, Defect A) and #784. Six cases across two describe blocks: ## #784 three-dot scope correctness on a behind branch Initializes a repo where the integration branch is `ci` (not `main`), sets up origin/HEAD, branches feature off ci, then advances ci with a commit feature does NOT include. Asserts: - resolveDefaultBranch returns `ci` (not main). - Three-dot `...HEAD` produces a clean file list: feature.txt present, upstream-only.txt absent. - Two-dot `ci..HEAD` (the bug we fixed) reverse-includes upstream-only.txt — the canonical "phantom scope creep" file. Anchors the fix and prevents silent regression to two-dot semantics. ## #777 Defect A: GitRefResolver reads from a specific ref Sets up a repo with stale spec/plan on main and a reworked version on `builder/777-feature` (pushed to origin). Asserts: - LocalResolver returns the stale on-main version (anchors the bug). - GitRefResolver(origin/builder/777-feature) returns the reworked builder-branch version (anchors the fix). - GitRefResolver also works with bare local-branch refs. - Returns null for nonexistent project IDs. - findSpecBaseName matches by numeric ID. ## Why this isn't an extension of bugfix-280-consult-diff.test.ts The existing #280 test locks in the single-arg `git diff ` working-tree-diff invariant. The new file covers orthogonal concerns (default branch resolution, three-dot range semantics, ref-based artifact resolution) and would muddy that file's narrow scope. All 87 consult-related tests pass after this commit. --- .../__tests__/non-main-default-branch.test.ts | 178 ++++++++++++++++++ 1 file changed, 178 insertions(+) create mode 100644 packages/codev/src/__tests__/non-main-default-branch.test.ts diff --git a/packages/codev/src/__tests__/non-main-default-branch.test.ts b/packages/codev/src/__tests__/non-main-default-branch.test.ts new file mode 100644 index 00000000..fc379eb4 --- /dev/null +++ b/packages/codev/src/__tests__/non-main-default-branch.test.ts @@ -0,0 +1,178 @@ +/** + * Regression tests for #777 (Defect B Layer 1 + Defect A) and #784: + * consult false-positive diff pollution on non-main-default repos and + * behind-on-rebase branches. + * + * - Three-dot scope correctness: when the integration branch advances + * past the branch base, those upstream commits must NOT appear in the + * reviewer's "scope" file list. + * - GitRefResolver: spec/plan reads from a specific ref, not the + * architect's checked-out working tree. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { execSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import * as os from 'node:os'; + +import { _getDiffStat as getDiffStat } from '../commands/consult/index.js'; +import { resolveDefaultBranch } from '../lib/default-branch.js'; +import { GitRefResolver, LocalResolver } from '../commands/porch/artifacts.js'; + +function shell(cmd: string, cwd: string): void { + execSync(cmd, { cwd, stdio: 'pipe' }); +} + +describe('#784 three-dot scope correctness on a behind branch', () => { + let tmpDir: string; + let originDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'three-dot-')); + originDir = fs.mkdtempSync(path.join(os.tmpdir(), 'three-dot-origin-')); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + fs.rmSync(originDir, { recursive: true, force: true }); + }); + + it('three-dot diff excludes base-only commits when branch is behind', () => { + // Setup: integration branch is `ci`, not `main`. + execSync(`git init --bare -b ci "${originDir}"`); + shell('git init -b ci', tmpDir); + shell('git config user.email "test@test.com"', tmpDir); + shell('git config user.name "Test"', tmpDir); + + fs.writeFileSync(path.join(tmpDir, 'shared.txt'), 'shared'); + shell('git add shared.txt', tmpDir); + shell('git commit -m "initial on ci"', tmpDir); + + shell(`git remote add origin "${originDir}"`, tmpDir); + shell('git push origin ci', tmpDir); + shell('git remote set-head origin ci', tmpDir); + + // Branch off ci into feature. + shell('git checkout -b feature', tmpDir); + + // Add a feature-branch change. + fs.writeFileSync(path.join(tmpDir, 'feature.txt'), 'feature work'); + shell('git add feature.txt', tmpDir); + shell('git commit -m "feature work"', tmpDir); + + // Advance ci with a commit feature does NOT include — the canonical + // "phantom scope creep" file in #784. + shell('git checkout ci', tmpDir); + fs.writeFileSync(path.join(tmpDir, 'upstream-only.txt'), 'ci-only commit'); + shell('git add upstream-only.txt', tmpDir); + shell('git commit -m "advance ci"', tmpDir); + shell('git checkout feature', tmpDir); + + // resolveDefaultBranch should pick up ci, not main. + expect(resolveDefaultBranch(tmpDir)).toBe('ci'); + + // Three-dot range (anchored at merge-base) excludes upstream-only.txt. + const mergeBase = execSync( + `git merge-base HEAD ${resolveDefaultBranch(tmpDir)}`, + { cwd: tmpDir, encoding: 'utf-8' }, + ).trim(); + const threeDot = getDiffStat(tmpDir, `${mergeBase}...HEAD`); + expect(threeDot.files).toContain('feature.txt'); + expect(threeDot.files).not.toContain('upstream-only.txt'); + + // Two-dot from ci's CURRENT tip (the bug we're fixing) would pull in + // upstream-only.txt because it's reachable from HEAD (it isn't) — wait, + // actually the wild bug is the reverse: `git diff ci..HEAD` shows files + // changed between ci's tip and HEAD. Files that are on ci but not on + // feature show up as "removed in feature". Verify the bug behavior to + // anchor the fix. + const twoDot = getDiffStat(tmpDir, 'ci..HEAD'); + // Two-dot against the moving ci tip reverse-includes upstream-only.txt + // (it exists on ci but not on feature → shows as a "scope creep" delete). + expect(twoDot.files).toContain('upstream-only.txt'); + }); +}); + +describe('#777 Defect A: GitRefResolver reads artifacts from a specific ref', () => { + let tmpDir: string; + let originDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitref-')); + originDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitref-origin-')); + + execSync(`git init --bare -b main "${originDir}"`); + shell('git init -b main', tmpDir); + shell('git config user.email "test@test.com"', tmpDir); + shell('git config user.name "Test"', tmpDir); + + fs.mkdirSync(path.join(tmpDir, 'codev', 'specs'), { recursive: true }); + fs.mkdirSync(path.join(tmpDir, 'codev', 'plans'), { recursive: true }); + + // Stale spec on main — what the architect's local checkout sees. + fs.writeFileSync( + path.join(tmpDir, 'codev', 'specs', '777-feature.md'), + '# Stale spec on main', + ); + fs.writeFileSync( + path.join(tmpDir, 'codev', 'plans', '777-feature.md'), + '# Stale plan on main', + ); + shell('git add codev/specs/777-feature.md codev/plans/777-feature.md', tmpDir); + shell('git commit -m "initial spec/plan on main"', tmpDir); + + shell(`git remote add origin "${originDir}"`, tmpDir); + shell('git push origin main', tmpDir); + + // Builder branch with the reworked artifact. + shell('git checkout -b builder/777-feature', tmpDir); + fs.writeFileSync( + path.join(tmpDir, 'codev', 'specs', '777-feature.md'), + '# Reworked spec on builder branch', + ); + fs.writeFileSync( + path.join(tmpDir, 'codev', 'plans', '777-feature.md'), + '# Reworked plan on builder branch', + ); + shell('git add codev/specs/777-feature.md codev/plans/777-feature.md', tmpDir); + shell('git commit -m "rework spec/plan"', tmpDir); + shell('git push origin builder/777-feature', tmpDir); + + // Architect's local checkout sits on main — the stale version. + shell('git checkout main', tmpDir); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + fs.rmSync(originDir, { recursive: true, force: true }); + }); + + it('LocalResolver returns the stale on-main version (the bug)', () => { + const local = new LocalResolver(tmpDir); + expect(local.getSpecContent('777', '')).toBe('# Stale spec on main'); + expect(local.getPlanContent('777', '')).toBe('# Stale plan on main'); + }); + + it('GitRefResolver returns the reworked builder-branch version (the fix)', () => { + const ref = new GitRefResolver(tmpDir, 'origin/builder/777-feature'); + expect(ref.getSpecContent('777', '')).toBe('# Reworked spec on builder branch'); + expect(ref.getPlanContent('777', '')).toBe('# Reworked plan on builder branch'); + }); + + it('GitRefResolver also works with a local branch ref (no origin/ prefix)', () => { + const ref = new GitRefResolver(tmpDir, 'builder/777-feature'); + expect(ref.getSpecContent('777', '')).toBe('# Reworked spec on builder branch'); + }); + + it('GitRefResolver returns null when the ref does not contain the artifact', () => { + const ref = new GitRefResolver(tmpDir, 'origin/builder/777-feature'); + expect(ref.getSpecContent('999', '')).toBeNull(); + expect(ref.getPlanContent('999', '')).toBeNull(); + }); + + it('GitRefResolver findSpecBaseName matches by numeric ID', () => { + const ref = new GitRefResolver(tmpDir, 'origin/builder/777-feature'); + expect(ref.findSpecBaseName('777', '')).toBe('777-feature'); + }); +}); From 9da29fba33b2b44e937a4f9e187ad3cc1caaccb4 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 11:37:01 +1000 Subject: [PATCH 06/18] =?UTF-8?q?[Bugfix=20#777]=20test(baked-decisions):?= =?UTF-8?q?=20refresh=20AIR=20pr-review=20baseline=20after=20main=20?= =?UTF-8?q?=E2=86=92=20integration-branch=20rename?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Layer 2 protocol-doc edit in the previous commit corrected line 37 of codev/protocols/air/consult-types/pr-review.md ("Is the branch up to date with main?" → "...with its base..."), which broke the Spec 746 pure-addition diff invariant for this one baseline. The baked-decisions test (Spec 746) is designed to ensure the *baked-decisions paragraph* is a pure addition — not to lock every pre-existing line of the prompt forever. Updating the baseline to match the corrected line preserves the test's actual intent. No other Spec 746 baselines were affected; only AIR has a pr-review.md.baseline (the others are skeleton mirrors with baselineName: null). --- .../__tests__/fixtures/baselines/air-pr-review.md.baseline | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/codev/src/agent-farm/__tests__/fixtures/baselines/air-pr-review.md.baseline b/packages/codev/src/agent-farm/__tests__/fixtures/baselines/air-pr-review.md.baseline index 91590393..1522bcbd 100644 --- a/packages/codev/src/agent-farm/__tests__/fixtures/baselines/air-pr-review.md.baseline +++ b/packages/codev/src/agent-farm/__tests__/fixtures/baselines/air-pr-review.md.baseline @@ -28,7 +28,7 @@ You are performing a review of a pull request created under the AIR protocol. Th 5. **PR Quality** - Does the PR link to the issue? - Is the PR body review section informative? - - Is the branch up to date with main? + - Is the branch up to date with its base (the integration branch the PR targets)? ## Verdict Format From 2e48467a0664e9110a5b7a1c50e619242384b826 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 15:39:25 +1000 Subject: [PATCH 07/18] [Bugfix #777] fix(bugfix-protocol): default-branch shell substitution falls back to main when origin/HEAD unset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmap-3 review finding (Gemini + Codex + Claude all agreed): the single-line `git diff --stat "$(git symbolic-ref ...)"` form has no fallback when origin/HEAD is unset. `git symbolic-ref` writes to stderr and outputs nothing → substitution becomes the empty string → `git diff --stat ""` silently compares working tree to the *index* (not to the integration branch), producing a confusing wrong answer rather than an obvious error. Fix matches the canonical pattern PIR already uses (codev/protocols/pir/prompts/implement.md:116, review.md:41): resolve DEFAULT_BRANCH once with `|| echo main` fallback, then use the variable. Two-step form also makes the snippet more readable. --- codev/protocols/bugfix/protocol.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codev/protocols/bugfix/protocol.md b/codev/protocols/bugfix/protocol.md index 6813786c..c2aa60f3 100644 --- a/codev/protocols/bugfix/protocol.md +++ b/codev/protocols/bugfix/protocol.md @@ -324,7 +324,8 @@ Before marking PR ready, the Builder must verify: The "< 300 LOC" threshold is measured as **net diff** (additions + deletions): ```bash -git diff --stat "$(git symbolic-ref --short refs/remotes/origin/HEAD | sed 's|^origin/||')" | tail -1 +DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || echo main) +git diff --stat "$DEFAULT_BRANCH" | tail -1 # Example: "3 files changed, 145 insertions(+), 52 deletions(-)" # Net diff = 145 + 52 = 197 LOC ✓ (under 300) ``` From fa16f0f53c9258269a343a63b8d264dd6c8eb757 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 15:41:13 +1000 Subject: [PATCH 08/18] [Bugfix #777] fix(consult): scope architect impl diff against PR.baseRefName, not repo default branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmap-3 review finding (Codex, headline): after the earlier `main` → `resolveDefaultBranch()` fix, architect-mode impl still diffed against the *repo's* default branch — not the PR's actual base. When a PR targets a non-default integration branch (e.g. the PR is into `ci` on a repo whose origin/HEAD is `main`), the merge-base calculation pulls in every commit between `ci` and `main`, attributing them to the builder. Same #777 false-positive class one layer deeper. ## Changes - Forge pr-search concept now returns `baseRefName` alongside `headRefName`. Single-line gh-flag widening; pr-view and pr-diff already used baseRefName via other call paths. - findPRForIssue return type widens to include baseRefName. - Architect-mode impl now: 1. Fetches both origin/baseRefName and origin/headRefName (each silently no-op if already fetched). 2. Merge-bases against `origin/` — the PR's actual base, not the repo's default. 3. On merge-base failure, falls back to three-dot against the base ref directly (`origin/...origin/`), which lets git compute the merge-base inline. 4. On total failure (base ref not resolvable), throws an explicit error rather than silently degrading. ## D2: replace silent merge-base catch with explicit error cmap-3 (Gemini, escalated to fatal): the prior silent catch let `diffRef` stay undefined, dropping into buildImplQuery's empty-changedFiles "explore the filesystem" path. In architect context, "the filesystem" means the architect's checked-out tree — typically the integration branch, not the PR. Reviewers happily emitted APPROVE/REQUEST_CHANGES verdicts on the wrong code. New behavior: try explicit merge-base → three-dot against base ref → throw with actionable error message. Never silently swallow. ## D4: document --branch as artifact-only Help text now clarifies that --branch only changes spec/plan source, not the impl diff scope. The diff is always the PR's head→base. Larger unification (--branch also drives diff scope) is deferred — that's a design choice, not a bug fix. ## Console output The architect-mode `Project:` line now includes the base ref (`Project: 777 (PR #123, builder/777-foo → ci)`) so the architect sees at-a-glance which integration branch the PR targets. --- .../codev/scripts/forge/github/pr-search.sh | 4 +- packages/codev/src/cli.ts | 2 +- packages/codev/src/commands/consult/index.ts | 69 ++++++++++++++----- 3 files changed, 55 insertions(+), 20 deletions(-) diff --git a/packages/codev/scripts/forge/github/pr-search.sh b/packages/codev/scripts/forge/github/pr-search.sh index 5286aadb..cbb84cc5 100755 --- a/packages/codev/scripts/forge/github/pr-search.sh +++ b/packages/codev/scripts/forge/github/pr-search.sh @@ -1,5 +1,5 @@ #!/bin/sh # Forge concept: pr-search (GitHub via gh CLI) # Input: CODEV_SEARCH_QUERY -# Output: JSON [{number, headRefName}] -exec gh pr list --search "$CODEV_SEARCH_QUERY" --json number,headRefName +# Output: JSON [{number, headRefName, baseRefName}] +exec gh pr list --search "$CODEV_SEARCH_QUERY" --json number,headRefName,baseRefName diff --git a/packages/codev/src/cli.ts b/packages/codev/src/cli.ts index 0c095472..4d0ced32 100644 --- a/packages/codev/src/cli.ts +++ b/packages/codev/src/cli.ts @@ -176,7 +176,7 @@ program .option('--protocol ', 'Protocol name: spir, aspir, air, bugfix, pir, maintain') .option('-t, --type ', 'Review type: spec, plan, impl, pr, phase, integration') .option('--issue ', 'Issue number (required from architect context)') - .option('--branch ', 'Read spec/plan from this git ref instead of the local workspace (e.g. `origin/builder/777-foo` or `builder/777-foo`). Defaults to the PR\'s head branch when --issue resolves to a PR.') + .option('--branch ', 'Read spec/plan artifacts from this git ref instead of the local workspace (e.g. `origin/builder/777-foo` or `builder/777-foo`). Defaults to the PR\'s head branch when --issue resolves to a PR. Note: this only changes the artifact source — for --type impl, the diff scope is always the PR\'s head→base, not the --branch ref.') .option('--output ', 'Write consultation output to file (used by porch)') .option('--plan-phase ', 'Scope review to a specific plan phase (used by porch)') .option('--context ', 'Context file with previous iteration feedback (used by porch)') diff --git a/packages/codev/src/commands/consult/index.ts b/packages/codev/src/commands/consult/index.ts index 50d35910..31a4231c 100644 --- a/packages/codev/src/commands/consult/index.ts +++ b/packages/codev/src/commands/consult/index.ts @@ -1189,13 +1189,19 @@ function findPRForCurrentBranch(workspaceRoot: string): string { /** * Find PR number for a given issue number (architect mode) via pr-search forge concept. + * + * Returns the PR's `baseRefName` alongside `headRefName` so the architect-mode + * impl path can compute the merge-base against the PR's *actual* base, not the + * repo's default branch. This matters when the PR targets a non-default + * integration branch — same #777 false-positive class as Layer 1, one layer + * deeper. Found by cmap-3 review. */ -function findPRForIssue(workspaceRoot: string, issueId: string): { number: number; headRefName: string } { +function findPRForIssue(workspaceRoot: string, issueId: string): { number: number; headRefName: string; baseRefName: string } { const result = executeForgeCommandSync('pr-search', { CODEV_SEARCH_QUERY: issueId, }, { cwd: workspaceRoot }); - const prs = Array.isArray(result) ? result as Array<{ number: number; headRefName: string }> : []; + const prs = Array.isArray(result) ? result as Array<{ number: number; headRefName: string; baseRefName: string }> : []; if (prs.length === 0 || !prs[0]?.number) { throw new Error(`No PR found for issue #${issueId}`); } @@ -1353,36 +1359,65 @@ function resolveArchitectQuery(workspaceRoot: string, type: string, options: Con case 'impl': { const pr = findPRForIssue(workspaceRoot, issueId); - // Fetch the branch and diff from merge-base + // Fetch both the PR head and its base so merge-base + diff have local + // refs to work with. Head fetch failures are non-fatal (may already be + // fetched); base fetch failures are also non-fatal because the base + // tracking ref usually exists already. try { execSync(`git fetch origin ${pr.headRefName}`, { cwd: workspaceRoot, stdio: 'pipe' }); - } catch { - // May already be fetched - } - const defaultBranch = resolveDefaultBranch(workspaceRoot); - let diffRef: string | undefined; + } catch { /* may already be fetched */ } + try { + execSync(`git fetch origin ${pr.baseRefName}`, { cwd: workspaceRoot, stdio: 'pipe' }); + } catch { /* may already be fetched */ } + + // Use the PR's actual base (not the repo's default branch) as the + // merge-base counterparty. cmap-3 finding: when a PR targets a + // non-default integration branch, defaultBranch was the wrong anchor + // and produced phantom scope-creep verdicts of the same shape as the + // hardcoded-`main` bug — one layer deeper. + let diffRef: string; try { const mergeBase = execSync( - `git merge-base ${defaultBranch} origin/${pr.headRefName}`, + `git merge-base origin/${pr.baseRefName} origin/${pr.headRefName}`, { cwd: workspaceRoot, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'] }, ).trim(); // Three-dot is the canonical PR-diff semantics (symmetric difference, - // anchored at the merge-base). Semantically equivalent to two-dot when - // the LHS is already the merge-base SHA, but disambiguates in code review - // and stays correct if the LHS shape ever changes. + // anchored at the merge-base). Equivalent to two-dot when the LHS is + // already the merge-base SHA, but disambiguates intent. diffRef = `${mergeBase}...origin/${pr.headRefName}`; } catch { - // Merge-base lookup failed (dangling origin/HEAD, default branch not - // locally present, etc.). Let buildImplQuery hit its empty-changedFiles - // fallback rather than crashing the whole command. + // Explicit merge-base failed (e.g. base ref not fetched, no common + // history). Fall back to three-dot against the base ref directly, + // which lets git compute the merge-base inline. + try { + execSync( + `git rev-parse --verify origin/${pr.baseRefName}`, + { cwd: workspaceRoot, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'] }, + ); + diffRef = `origin/${pr.baseRefName}...origin/${pr.headRefName}`; + } catch (err) { + // Both attempts failed — degrading silently would let reviewers + // emit verdicts against the architect's local checkout (typically + // the integration branch), producing bogus APPROVE/REQUEST_CHANGES + // verdicts on the wrong code. Crash explicitly instead. + // cmap-3 finding (Gemini). + throw new Error( + `Cannot compute diff scope for PR #${pr.number} (${pr.headRefName} → ${pr.baseRefName}). ` + + `Ensure both refs are fetched: \`git fetch origin ${pr.baseRefName} ${pr.headRefName}\`. ` + + `Underlying error: ${err instanceof Error ? err.message : String(err)}`, + ); + } } + // Read spec/plan from the PR's branch by default so they match the - // diff source (#777 Defect A). --branch overrides the PR default. + // diff source (#777 Defect A). --branch overrides the PR default; the + // diff scope itself is always the PR's head→base (--branch does not + // change diff scope, only artifact source — cmap-3 finding). const ref = options.branch ?? `origin/${pr.headRefName}`; const resolver = new GitRefResolver(workspaceRoot, ref); const spec = findSpecContent(workspaceRoot, issueId, resolver); const plan = findPlanContent(workspaceRoot, issueId, resolver); - console.error(`Project: ${issueId} (PR #${pr.number}, branch: ${pr.headRefName})`); + console.error(`Project: ${issueId} (PR #${pr.number}, ${pr.headRefName} → ${pr.baseRefName})`); console.error(`Source: ${ref}`); if (spec) console.error(`Spec: ${spec.label}`); if (plan) console.error(`Plan: ${plan.label}`); From 90be0e67aab1d5f1959755f70264dd7054ebc4db Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 15:43:26 +1000 Subject: [PATCH 09/18] test(consult): cover PR.baseRefName != repo default as merge-base anchor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmap-3 follow-up test for the D3 fix. Sets up a repo where: - origin/HEAD → main (repo default) - ci is cut off main; ci advances independently with ci-only.txt - main also advances with main-only.txt - builder/feature is cut off ci (post-ci-only) and adds feature.txt - A PR from builder/feature targets ci (not main) Two assertions anchor the fix vs the bug: - Fix: `git merge-base origin/ci origin/builder/feature` → ci's tip; three-dot scope = [feature.txt] only. No ci-only.txt, no main-only.txt — the canonical PR-style diff. - Bug: `git merge-base origin/main origin/builder/feature` → initial commit (before ci forked); three-dot scope includes ci-only.txt as phantom "scope creep" attributed to the builder. This makes the D3 correctness gap visible at the test level, not just in the wild on non-default integration branches. --- .../__tests__/non-main-default-branch.test.ts | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/packages/codev/src/__tests__/non-main-default-branch.test.ts b/packages/codev/src/__tests__/non-main-default-branch.test.ts index fc379eb4..2b759dc0 100644 --- a/packages/codev/src/__tests__/non-main-default-branch.test.ts +++ b/packages/codev/src/__tests__/non-main-default-branch.test.ts @@ -176,3 +176,88 @@ describe('#777 Defect A: GitRefResolver reads artifacts from a specific ref', () expect(ref.findSpecBaseName('777', '')).toBe('777-feature'); }); }); + +describe('#777 architect impl: diff scope anchors on PR.baseRefName, not repo default', () => { + // cmap-3 Codex finding (D3): when a PR targets a non-default integration + // branch, the impl-review must compute its scope against the PR's actual + // base — not the repo's `origin/HEAD`. This test exercises the merge-base + // arithmetic directly to confirm the right anchor is picked. + let tmpDir: string; + let originDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'baseref-')); + originDir = fs.mkdtempSync(path.join(os.tmpdir(), 'baseref-origin-')); + + // origin/HEAD → main (repo default), but the PR will target `ci`. + execSync(`git init --bare -b main "${originDir}"`); + shell('git init -b main', tmpDir); + shell('git config user.email "test@test.com"', tmpDir); + shell('git config user.name "Test"', tmpDir); + fs.writeFileSync(path.join(tmpDir, 'shared.txt'), 'shared'); + shell('git add shared.txt', tmpDir); + shell('git commit -m "initial on main"', tmpDir); + shell(`git remote add origin "${originDir}"`, tmpDir); + shell('git push origin main', tmpDir); + shell('git remote set-head origin main', tmpDir); + + // Cut `ci` off main. ci then advances with its own commit (this is what + // makes the merge-bases diverge below — if ci stayed at initial, both + // anchors compute the same SHA). + shell('git checkout -b ci', tmpDir); + fs.writeFileSync(path.join(tmpDir, 'ci-only.txt'), 'ci-only commit'); + shell('git add ci-only.txt', tmpDir); + shell('git commit -m "ci-only work"', tmpDir); + shell('git push origin ci', tmpDir); + + // Main also advances independently. + shell('git checkout main', tmpDir); + fs.writeFileSync(path.join(tmpDir, 'main-only.txt'), 'main-only commit'); + shell('git add main-only.txt', tmpDir); + shell('git commit -m "advance main"', tmpDir); + shell('git push origin main', tmpDir); + + // Builder cuts feature off ci (after ci-only) and adds its own work. + shell('git checkout ci', tmpDir); + shell('git checkout -b builder/feature', tmpDir); + fs.writeFileSync(path.join(tmpDir, 'feature.txt'), 'feature work'); + shell('git add feature.txt', tmpDir); + shell('git commit -m "feature work"', tmpDir); + shell('git push origin builder/feature', tmpDir); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + fs.rmSync(originDir, { recursive: true, force: true }); + }); + + it('merge-base against origin/ (ci) excludes ci-only commits — the fix', () => { + // The fix: compute against the PR's actual base (origin/ci), not the + // repo default (main). The merge-base is the ci-after-ci-only commit, + // so the three-dot scope is exactly feature.txt — no ci-only.txt, + // no main-only.txt. + const mergeBaseCorrect = execSync( + 'git merge-base origin/ci origin/builder/feature', + { cwd: tmpDir, encoding: 'utf-8' }, + ).trim(); + const correct = getDiffStat(tmpDir, `${mergeBaseCorrect}...origin/builder/feature`); + expect(correct.files).toContain('feature.txt'); + expect(correct.files).not.toContain('ci-only.txt'); + expect(correct.files).not.toContain('main-only.txt'); + }); + + it('merge-base against main (repo default) sweeps in ci-only commits — the bug', () => { + // Pre-fix behavior anchor: when the merge-base uses the repo default + // (main) for a PR that actually targets `ci`, the common ancestor falls + // back to the initial commit (before ci forked). The three-dot diff + // then includes the ci-only commit as "scope creep" attributed to the + // builder, even though ci-only.txt is on the base branch. + const mergeBaseWrong = execSync( + 'git merge-base origin/main origin/builder/feature', + { cwd: tmpDir, encoding: 'utf-8' }, + ).trim(); + const wrong = getDiffStat(tmpDir, `${mergeBaseWrong}...origin/builder/feature`); + expect(wrong.files).toContain('feature.txt'); + expect(wrong.files).toContain('ci-only.txt'); + }); +}); From 9b0ee17a74ea4b542ee1b2f388c9351027401f76 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 15:54:22 +1000 Subject: [PATCH 10/18] [Bugfix #777] fix(consult): surface fetch failures instead of silently using stale refs The two silent `catch { /* may already be fetched */ }` blocks introduced by this branch (one in resolveArchitectQuery's impl case, one in GitRefResolver's constructor) hid a class of failure mode that recreates the false-positive bug we're fixing: auth, network, or forge failures would leave us computing diffs / reading artifacts against stale local tracking refs without any signal that something was off. Replace both with structured catches that: - Stay silent on the already-cached / already-fetched case (the intended common case). - Emit a stderr warning when fetch actually failed, including the underlying stderr from git. The warning is explicit about the consequence ("Stale refs may produce misleading diffs/reviews"). - Let the subsequent `git merge-base` / `git rev-parse --verify` / `git show` surface missing-ref failures as before. Scope is intentionally narrow: only the two silent catches this branch introduced. The pre-existing silent catches elsewhere in consult/index.ts and artifacts.ts (LocalResolver fallbacks, metrics plumbing, builder-side diff fallback) are not touched. cmap-3 follow-up: Codex flagged both spots explicitly ("Swallowing fetch failure: too quiet" and "auth/network/forge failures can degrade into reviewing stale local artifacts without making that obvious"). --- packages/codev/src/commands/consult/index.ts | 25 ++++++++++++------- .../codev/src/commands/porch/artifacts.ts | 15 ++++++++--- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/packages/codev/src/commands/consult/index.ts b/packages/codev/src/commands/consult/index.ts index 31a4231c..6684d426 100644 --- a/packages/codev/src/commands/consult/index.ts +++ b/packages/codev/src/commands/consult/index.ts @@ -1360,15 +1360,22 @@ function resolveArchitectQuery(workspaceRoot: string, type: string, options: Con case 'impl': { const pr = findPRForIssue(workspaceRoot, issueId); // Fetch both the PR head and its base so merge-base + diff have local - // refs to work with. Head fetch failures are non-fatal (may already be - // fetched); base fetch failures are also non-fatal because the base - // tracking ref usually exists already. - try { - execSync(`git fetch origin ${pr.headRefName}`, { cwd: workspaceRoot, stdio: 'pipe' }); - } catch { /* may already be fetched */ } - try { - execSync(`git fetch origin ${pr.baseRefName}`, { cwd: workspaceRoot, stdio: 'pipe' }); - } catch { /* may already be fetched */ } + // refs to work with. Fetch failures are non-fatal in the + // already-cached case, but auth/network failures can leave us with + // stale local tracking refs — surface them so the architect knows the + // diff may be misleading. + for (const ref of [pr.headRefName, pr.baseRefName]) { + try { + execSync(`git fetch origin ${ref}`, { cwd: workspaceRoot, stdio: ['ignore', 'pipe', 'pipe'] }); + } catch (err) { + const stderr = err instanceof Error && 'stderr' in err ? String((err as { stderr: unknown }).stderr).trim() : ''; + console.error( + `Warning: \`git fetch origin ${ref}\` failed; proceeding with any locally-cached copy. ` + + `Stale refs may produce misleading diffs.` + + (stderr ? ` Underlying: ${stderr}` : '') + ); + } + } // Use the PR's actual base (not the repo's default branch) as the // merge-base counterparty. cmap-3 finding: when a PR targets a diff --git a/packages/codev/src/commands/porch/artifacts.ts b/packages/codev/src/commands/porch/artifacts.ts index 76839c76..55dd7c45 100644 --- a/packages/codev/src/commands/porch/artifacts.ts +++ b/packages/codev/src/commands/porch/artifacts.ts @@ -387,10 +387,17 @@ export class GitRefResolver implements ArtifactResolver { cwd: workspaceRoot, stdio: ['ignore', 'pipe', 'pipe'], }); - } catch { - // Ref may already be fetched, the remote may be unreachable, or the - // branch may not exist remotely — the subsequent `git show` will - // surface the real failure. + } catch (err) { + // Distinguish "already fetched / offline with cached copy" (silently + // OK) from "fetch actually failed for an unexpected reason" (visible). + // The subsequent `git show` surfaces missing-ref failures, but it + // can't tell stale-vs-fresh apart — that's what this warning is for. + const stderr = err instanceof Error && 'stderr' in err ? String((err as { stderr: unknown }).stderr).trim() : ''; + console.error( + `Warning: \`git fetch origin ${branch}\` failed; reading ${ref} from any locally-cached copy. ` + + `Stale refs may produce misleading reviews.` + + (stderr ? ` Underlying: ${stderr}` : '') + ); } } } From b1840efdd3330f53dbc6556435a8d1b0a2c397de Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 16:01:16 +1000 Subject: [PATCH 11/18] [Bugfix #777] refactor(consult): collapse redundant merge-base, use execFileSync for ref interpolation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two cleanups to the architect-mode impl diff-resolution that the D3 commit had left as residue. ## Drop the redundant explicit merge-base call `git diff A...B` is documented as `git diff $(git merge-base A B) B` — git computes the merge-base internally. So the prior code's strategy-1 path (explicit `git merge-base origin/ origin/` → embed as `${SHA}...origin/`) produced *identical* output to the strategy-2 path (just `origin/...origin/` directly). The first call was dead weight: an extra exec, an extra fallback rung, no functional benefit. Collapse to one path: verify the base ref resolves locally, then emit three-dot directly. On failure, throw with the same actionable message — never silently degrade. ## Switch shell interpolation to execFileSync argv-mode `pr.baseRefName` / `pr.headRefName` come from `gh pr list --json`. Git's check-ref-format only forbids a small subset of characters (space, `~`, `^`, `:`, `?`, `*`, `[`, `..`, `@{`, control, backslash). It permits `;`, `$`, `(`, `)`, `<`, `>`, `&`, `|`, `` ` ``, etc — all shell metacharacters. GitHub likely filters these at the UI layer, but defense-in-depth: don't shell-interpolate ref names. Affected sites in this branch: - `git fetch origin ` (head + base, in the for-loop). - `git rev-parse --verify origin/`. - `getDiffStat`'s `git diff --stat ` and `git diff --name-only ` — `ref` is now sometimes the three-dot range `origin/...origin/`, so it carries the same risk. All switched to `execFileSync('git', [args...])`. `GitRefResolver`'s constructor already used this pattern; the architect-mode impl path now matches. ## Test mock updates `consult.test.ts` mocked `node:child_process` with only `execSync` exported. Added `execFileSync` to the mock; updated the three `getDiffStat` tests (bugfix #240 regression suite) to mock `execFileSync` with the argv signature instead. All 2996 tests pass. --- packages/codev/src/__tests__/consult.test.ts | 52 ++++++------- packages/codev/src/commands/consult/index.ts | 79 +++++++++----------- 2 files changed, 58 insertions(+), 73 deletions(-) diff --git a/packages/codev/src/__tests__/consult.test.ts b/packages/codev/src/__tests__/consult.test.ts index 06634297..8d63dd2e 100644 --- a/packages/codev/src/__tests__/consult.test.ts +++ b/packages/codev/src/__tests__/consult.test.ts @@ -25,6 +25,7 @@ vi.mock('node:child_process', () => ({ } return Buffer.from(''); }), + execFileSync: vi.fn(() => Buffer.from('')), })); // Mock Claude Agent SDK @@ -940,18 +941,15 @@ describe('consult command', () => { it('getDiffStat should call git diff --stat and --name-only', async () => { vi.resetModules(); - const { execSync } = await import('node:child_process'); - vi.mocked(execSync).mockImplementation((cmd: string) => { - if (typeof cmd === 'string' && cmd.includes('--stat')) { - return Buffer.from(' src/app.ts | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n'); - } - if (typeof cmd === 'string' && cmd.includes('--name-only')) { - return Buffer.from('src/app.ts\n'); + const { execFileSync } = await import('node:child_process'); + vi.mocked(execFileSync).mockImplementation((_file: string, args?: readonly string[]) => { + if (args?.includes('--stat')) { + return ' src/app.ts | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n'; } - if (cmd.includes('which')) { - return Buffer.from('/usr/bin/command'); + if (args?.includes('--name-only')) { + return 'src/app.ts\n'; } - return Buffer.from(''); + return ''; }); const { _getDiffStat } = await import('../commands/consult/index.js'); @@ -964,23 +962,20 @@ describe('consult command', () => { it('getDiffStat should handle multiple files', async () => { vi.resetModules(); - const { execSync } = await import('node:child_process'); - vi.mocked(execSync).mockImplementation((cmd: string) => { - if (typeof cmd === 'string' && cmd.includes('--stat')) { - return Buffer.from( + const { execFileSync } = await import('node:child_process'); + vi.mocked(execFileSync).mockImplementation((_file: string, args?: readonly string[]) => { + if (args?.includes('--stat')) { + return ( ' .claude/settings.json | 5 +++++\n' + ' src/app/widget.tsx | 20 ++++++++++++++------\n' + ' src/middleware.ts | 15 ++++++++++++---\n' + ' 3 files changed, 32 insertions(+), 9 deletions(-)\n' ); } - if (typeof cmd === 'string' && cmd.includes('--name-only')) { - return Buffer.from('.claude/settings.json\nsrc/app/widget.tsx\nsrc/middleware.ts\n'); + if (args?.includes('--name-only')) { + return '.claude/settings.json\nsrc/app/widget.tsx\nsrc/middleware.ts\n'; } - if (cmd.includes('which')) { - return Buffer.from('/usr/bin/command'); - } - return Buffer.from(''); + return ''; }); const { _getDiffStat } = await import('../commands/consult/index.js'); @@ -1000,24 +995,21 @@ describe('consult command', () => { // the actual files from disk, eliminating truncation entirely. vi.resetModules(); - const { execSync } = await import('node:child_process'); - vi.mocked(execSync).mockImplementation((cmd: string) => { - if (typeof cmd === 'string' && cmd.includes('--stat')) { - return Buffer.from(' 50 files changed, 10000 insertions(+), 5000 deletions(-)\n'); + const { execFileSync } = await import('node:child_process'); + vi.mocked(execFileSync).mockImplementation((_file: string, args?: readonly string[]) => { + if (args?.includes('--stat')) { + return ' 50 files changed, 10000 insertions(+), 5000 deletions(-)\n'; } - if (typeof cmd === 'string' && cmd.includes('--name-only')) { + if (args?.includes('--name-only')) { // 50 files spanning the full alphabet const files = Array.from({ length: 50 }, (_, i) => i < 10 ? `.claude/file${i}.json` : i < 20 ? `codev/specs/${i}.md` : `src/app/component${i}.tsx` ); - return Buffer.from(files.join('\n') + '\n'); - } - if (cmd.includes('which')) { - return Buffer.from('/usr/bin/command'); + return files.join('\n') + '\n'; } - return Buffer.from(''); + return ''; }); const { _getDiffStat } = await import('../commands/consult/index.js'); diff --git a/packages/codev/src/commands/consult/index.ts b/packages/codev/src/commands/consult/index.ts index 6684d426..843f1e7d 100644 --- a/packages/codev/src/commands/consult/index.ts +++ b/packages/codev/src/commands/consult/index.ts @@ -9,7 +9,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; -import { spawn, execSync } from 'node:child_process'; +import { spawn, execSync, execFileSync } from 'node:child_process'; import { tmpdir } from 'node:os'; import chalk from 'chalk'; import { query as claudeQuery } from '@anthropic-ai/claude-agent-sdk'; @@ -779,10 +779,13 @@ async function runConsultation( /** * Get a compact diff stat summary and list of changed files. + * + * `ref` is passed as a single argv element so branch names with shell + * metacharacters can't break out of the command (#777 cmap-3 follow-up). */ function getDiffStat(workspaceRoot: string, ref: string): { stat: string; files: string[] } { - const stat = execSync(`git diff --stat ${ref}`, { cwd: workspaceRoot, encoding: 'utf-8' }).toString(); - const nameOnly = execSync(`git diff --name-only ${ref}`, { cwd: workspaceRoot, encoding: 'utf-8' }).toString(); + const stat = execFileSync('git', ['diff', '--stat', ref], { cwd: workspaceRoot, encoding: 'utf-8' }); + const nameOnly = execFileSync('git', ['diff', '--name-only', ref], { cwd: workspaceRoot, encoding: 'utf-8' }); const files = nameOnly.trim().split('\n').filter(Boolean); return { stat, files }; } @@ -1359,14 +1362,14 @@ function resolveArchitectQuery(workspaceRoot: string, type: string, options: Con case 'impl': { const pr = findPRForIssue(workspaceRoot, issueId); - // Fetch both the PR head and its base so merge-base + diff have local - // refs to work with. Fetch failures are non-fatal in the - // already-cached case, but auth/network failures can leave us with - // stale local tracking refs — surface them so the architect knows the - // diff may be misleading. + // Fetch both the PR head and its base so the diff has local refs to + // work with. Fetch failures are non-fatal in the already-cached case, + // but auth/network failures can leave us with stale local tracking + // refs — surface them so the architect knows the diff may be + // misleading. for (const ref of [pr.headRefName, pr.baseRefName]) { try { - execSync(`git fetch origin ${ref}`, { cwd: workspaceRoot, stdio: ['ignore', 'pipe', 'pipe'] }); + execFileSync('git', ['fetch', 'origin', ref], { cwd: workspaceRoot, stdio: ['ignore', 'pipe', 'pipe'] }); } catch (err) { const stderr = err instanceof Error && 'stderr' in err ? String((err as { stderr: unknown }).stderr).trim() : ''; console.error( @@ -1378,42 +1381,32 @@ function resolveArchitectQuery(workspaceRoot: string, type: string, options: Con } // Use the PR's actual base (not the repo's default branch) as the - // merge-base counterparty. cmap-3 finding: when a PR targets a - // non-default integration branch, defaultBranch was the wrong anchor - // and produced phantom scope-creep verdicts of the same shape as the - // hardcoded-`main` bug — one layer deeper. + // merge-base anchor. cmap-3 finding: when a PR targets a non-default + // integration branch, defaultBranch was the wrong anchor and produced + // phantom scope-creep verdicts of the same shape as the hardcoded- + // `main` bug — one layer deeper. + // + // Three-dot in `git diff A...B` is documented as `git diff + // $(git merge-base A B) B` — git computes the merge-base internally, + // so an explicit `git merge-base` call would be redundant. We just + // verify the base ref is locally resolvable and let the three-dot + // form do the rest. If verification fails, crash explicitly rather + // than silently degrade to reviewing the architect's checked-out + // tree (cmap-3 Gemini finding). let diffRef: string; try { - const mergeBase = execSync( - `git merge-base origin/${pr.baseRefName} origin/${pr.headRefName}`, - { cwd: workspaceRoot, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'] }, - ).trim(); - // Three-dot is the canonical PR-diff semantics (symmetric difference, - // anchored at the merge-base). Equivalent to two-dot when the LHS is - // already the merge-base SHA, but disambiguates intent. - diffRef = `${mergeBase}...origin/${pr.headRefName}`; - } catch { - // Explicit merge-base failed (e.g. base ref not fetched, no common - // history). Fall back to three-dot against the base ref directly, - // which lets git compute the merge-base inline. - try { - execSync( - `git rev-parse --verify origin/${pr.baseRefName}`, - { cwd: workspaceRoot, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'] }, - ); - diffRef = `origin/${pr.baseRefName}...origin/${pr.headRefName}`; - } catch (err) { - // Both attempts failed — degrading silently would let reviewers - // emit verdicts against the architect's local checkout (typically - // the integration branch), producing bogus APPROVE/REQUEST_CHANGES - // verdicts on the wrong code. Crash explicitly instead. - // cmap-3 finding (Gemini). - throw new Error( - `Cannot compute diff scope for PR #${pr.number} (${pr.headRefName} → ${pr.baseRefName}). ` + - `Ensure both refs are fetched: \`git fetch origin ${pr.baseRefName} ${pr.headRefName}\`. ` + - `Underlying error: ${err instanceof Error ? err.message : String(err)}`, - ); - } + execFileSync('git', ['rev-parse', '--verify', `origin/${pr.baseRefName}`], { + cwd: workspaceRoot, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + diffRef = `origin/${pr.baseRefName}...origin/${pr.headRefName}`; + } catch (err) { + throw new Error( + `Cannot compute diff scope for PR #${pr.number} (${pr.headRefName} → ${pr.baseRefName}). ` + + `Ensure both refs are fetched: \`git fetch origin ${pr.baseRefName} ${pr.headRefName}\`. ` + + `Underlying error: ${err instanceof Error ? err.message : String(err)}`, + ); } // Read spec/plan from the PR's branch by default so they match the From f68f24e7ba37532ac826da0c773227a62fc003de Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 16:10:18 +1000 Subject: [PATCH 12/18] [Bugfix #777] fix(protocols): default-branch resolution falls through to "main" correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pattern shipped in earlier commits — both my D1 fix (2e48467a, BUGFIX) and the pre-existing PIR snippets — had a latent bug: `|| echo main` fires only when the *last* command in the pipeline exits non-zero. `sed` happily exits 0 on empty input, so when `git symbolic-ref` fails: $(git symbolic-ref ... 2>/dev/null | sed 's|^origin/||' || echo main) └─ symbolic-ref fails ─┘ └─ sed succeeds on empty input ─┘ pipeline exit = 0 (sed's), `|| echo main` does NOT fire, $(...) substitutes to '' Result: DEFAULT_BRANCH=""; `git diff --stat ""` silently compares working tree to the index instead of erroring. Exactly the class of silent wrong-answer this whole branch is trying to eliminate. ## The fix Use POSIX parameter expansion `${VAR:-default}` against the captured variable instead of relying on the pipeline's exit status: DEFAULT_BRANCH=$(git symbolic-ref ... 2>/dev/null | sed 's|^origin/||') DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} `${VAR:-main}` fires when VAR is unset OR null/empty, which is the condition we actually care about. Independent of pipeline exit codes. ## Empirical verification ``` $ git symbolic-ref --delete refs/remotes/origin/HEAD $ DEFAULT_BRANCH=$(git symbolic-ref ... 2>/dev/null | sed 's|^origin/||' || echo main) $ echo "'$DEFAULT_BRANCH'" '' # old broken form $ DEFAULT_BRANCH=$(git symbolic-ref ... 2>/dev/null | sed 's|^origin/||'); DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} $ echo "'$DEFAULT_BRANCH'" 'main' # new form ``` ## Scope Five sites across both trees: - codev/protocols/bugfix/protocol.md (my D1 commit; uses the inline `${DEFAULT_BRANCH:-main}` form because it's single-use) - codev/protocols/pir/prompts/implement.md (pre-existing PIR snippet — same latent bug, pulled along because the user explicitly broadened scope to "fix all three sites") - codev/protocols/pir/prompts/review.md - codev-skeleton/protocols/pir/prompts/implement.md - codev-skeleton/protocols/pir/prompts/review.md PIR snippets use the two-statement form with `;` separator since $DEFAULT_BRANCH is referenced later in the prompt and needs to be a guaranteed-set variable. ## When origin/HEAD is actually unset Most common in adopting projects (codev-skeleton ships these snippets downstream): CI runners that `git remote add origin ` instead of cloning, repos cloned by pre-2.20 git, repos after a default-branch rename that never ran `git remote set-head origin --auto`. Codev's own architect+builder flow inherits origin/HEAD correctly, so the bug rarely surfaces in our daily use — but it's the kind of silent wrong-answer that erodes trust when it does. --- codev-skeleton/protocols/pir/prompts/implement.md | 2 +- codev-skeleton/protocols/pir/prompts/review.md | 2 +- codev/protocols/bugfix/protocol.md | 4 ++-- codev/protocols/pir/prompts/implement.md | 2 +- codev/protocols/pir/prompts/review.md | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/codev-skeleton/protocols/pir/prompts/implement.md b/codev-skeleton/protocols/pir/prompts/implement.md index b6eef25e..cc31736c 100644 --- a/codev-skeleton/protocols/pir/prompts/implement.md +++ b/codev-skeleton/protocols/pir/prompts/implement.md @@ -113,7 +113,7 @@ When the gate goes pending, output a short prose summary in the pane to orient t > **What changed**: 2–3 sentence summary. > -> **Files**: `git diff --stat "$DEFAULT_BRANCH"` style list — paths and +/-. (Resolve `$DEFAULT_BRANCH` once per session: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || echo main)`.) +> **Files**: `git diff --stat "$DEFAULT_BRANCH"` style list — paths and +/-. (Resolve `$DEFAULT_BRANCH` once per session: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); DEFAULT_BRANCH=${DEFAULT_BRANCH:-main}`.) > > **Test results**: `npm run build` ✓, `npm test` ✓ (X tests, Y new). > diff --git a/codev-skeleton/protocols/pir/prompts/review.md b/codev-skeleton/protocols/pir/prompts/review.md index 499363fb..6e4dd27d 100644 --- a/codev-skeleton/protocols/pir/prompts/review.md +++ b/codev-skeleton/protocols/pir/prompts/review.md @@ -38,7 +38,7 @@ Fixes #{{issue.number}} ## Files Changed -Output of `git diff --stat "$DEFAULT_BRANCH"`, formatted as a list (resolve once: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || echo main)`): +Output of `git diff --stat "$DEFAULT_BRANCH"`, formatted as a list (resolve once: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); DEFAULT_BRANCH=${DEFAULT_BRANCH:-main}`): - `path/to/file.ts` (+12 / -3) - `path/to/new-file.ts` (+45 / -0) diff --git a/codev/protocols/bugfix/protocol.md b/codev/protocols/bugfix/protocol.md index c2aa60f3..532e32e8 100644 --- a/codev/protocols/bugfix/protocol.md +++ b/codev/protocols/bugfix/protocol.md @@ -324,8 +324,8 @@ Before marking PR ready, the Builder must verify: The "< 300 LOC" threshold is measured as **net diff** (additions + deletions): ```bash -DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || echo main) -git diff --stat "$DEFAULT_BRANCH" | tail -1 +DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||') +git diff --stat "${DEFAULT_BRANCH:-main}" | tail -1 # Example: "3 files changed, 145 insertions(+), 52 deletions(-)" # Net diff = 145 + 52 = 197 LOC ✓ (under 300) ``` diff --git a/codev/protocols/pir/prompts/implement.md b/codev/protocols/pir/prompts/implement.md index b6eef25e..cc31736c 100644 --- a/codev/protocols/pir/prompts/implement.md +++ b/codev/protocols/pir/prompts/implement.md @@ -113,7 +113,7 @@ When the gate goes pending, output a short prose summary in the pane to orient t > **What changed**: 2–3 sentence summary. > -> **Files**: `git diff --stat "$DEFAULT_BRANCH"` style list — paths and +/-. (Resolve `$DEFAULT_BRANCH` once per session: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || echo main)`.) +> **Files**: `git diff --stat "$DEFAULT_BRANCH"` style list — paths and +/-. (Resolve `$DEFAULT_BRANCH` once per session: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); DEFAULT_BRANCH=${DEFAULT_BRANCH:-main}`.) > > **Test results**: `npm run build` ✓, `npm test` ✓ (X tests, Y new). > diff --git a/codev/protocols/pir/prompts/review.md b/codev/protocols/pir/prompts/review.md index 499363fb..6e4dd27d 100644 --- a/codev/protocols/pir/prompts/review.md +++ b/codev/protocols/pir/prompts/review.md @@ -38,7 +38,7 @@ Fixes #{{issue.number}} ## Files Changed -Output of `git diff --stat "$DEFAULT_BRANCH"`, formatted as a list (resolve once: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || echo main)`): +Output of `git diff --stat "$DEFAULT_BRANCH"`, formatted as a list (resolve once: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); DEFAULT_BRANCH=${DEFAULT_BRANCH:-main}`): - `path/to/file.ts` (+12 / -3) - `path/to/new-file.ts` (+45 / -0) From 5f7deb4448839157691923f30b587103358b5d45 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 16:15:42 +1000 Subject: [PATCH 13/18] [Bugfix #777] fix(consult): tolerate stale pr-search.sh overrides without baseRefName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This branch updates the bundled `pr-search.sh` forge concept to return `baseRefName` alongside `number` and `headRefName`, and the new CLI relies on that field for architect-mode impl diff scoping (#777 D3 cmap-3 fix). The forge script ships in the same npm package as the CLI, so under normal install/update they move together. But codev's tier-resolution system lets a project override forge scripts via `.codev/scripts/forge/github/pr-search.sh`. If a project has such an override that pre-dates the baseRefName addition and they upgrade the codev CLI without refreshing the override, the new CLI would get `undefined` for `pr.baseRefName` and crash in the impl path. This is the one genuine cross-version coupling this branch introduces. The fix is purely defensive: detect missing baseRefName, substitute the repo's default branch (via the helper this branch already added), and warn loudly with a pointer to where the authoritative version lives. Failure mode without the fallback: cryptic crash like `fatal: ambiguous argument 'origin/undefined'` deep in the merge-base resolution. Failure mode with the fallback: visible stderr warning + behavior that matches the pre-baseRefName code path (use repo default as base). Easy to discover, easy to fix. Not adding a test for this — `findPRForIssue` isn't exported and the codepath is short enough that adding test scaffolding would cost more than it pays. The behavior is also a strict superset of "crashes on undefined" so it can't regress observably. --- packages/codev/src/commands/consult/index.ts | 23 ++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/packages/codev/src/commands/consult/index.ts b/packages/codev/src/commands/consult/index.ts index 843f1e7d..0270439f 100644 --- a/packages/codev/src/commands/consult/index.ts +++ b/packages/codev/src/commands/consult/index.ts @@ -1198,18 +1198,37 @@ function findPRForCurrentBranch(workspaceRoot: string): string { * repo's default branch. This matters when the PR targets a non-default * integration branch — same #777 false-positive class as Layer 1, one layer * deeper. Found by cmap-3 review. + * + * Defensive fallback: if a project ships its own `pr-search.sh` override at + * `.codev/scripts/forge/github/pr-search.sh` that pre-dates the baseRefName + * addition, the JSON won't include it. Rather than crashing on a stale + * override (which is the kind of thing users only discover at the worst + * possible moment), substitute the repo's default branch and warn loudly. */ function findPRForIssue(workspaceRoot: string, issueId: string): { number: number; headRefName: string; baseRefName: string } { const result = executeForgeCommandSync('pr-search', { CODEV_SEARCH_QUERY: issueId, }, { cwd: workspaceRoot }); - const prs = Array.isArray(result) ? result as Array<{ number: number; headRefName: string; baseRefName: string }> : []; + const prs = Array.isArray(result) ? result as Array<{ number: number; headRefName: string; baseRefName?: string }> : []; if (prs.length === 0 || !prs[0]?.number) { throw new Error(`No PR found for issue #${issueId}`); } - return prs[0]; + const pr = prs[0]; + if (!pr.baseRefName) { + const defaultBranch = resolveDefaultBranch(workspaceRoot); + console.error( + `Warning: forge pr-search did not return baseRefName for PR #${pr.number}; ` + + `falling back to repo default branch \`${defaultBranch}\`. ` + + `This usually means a stale \`pr-search.sh\` override exists at ` + + `.codev/scripts/forge/github/pr-search.sh — refresh it (see the bundled version under ` + + `\`packages/codev/scripts/forge/github/pr-search.sh\` for the current shape).` + ); + return { number: pr.number, headRefName: pr.headRefName, baseRefName: defaultBranch }; + } + + return { number: pr.number, headRefName: pr.headRefName, baseRefName: pr.baseRefName }; } /** From cbdd129a0a93040a9d1eb2b509db75f628d11e64 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 16:32:29 +1000 Subject: [PATCH 14/18] [Bugfix #777] fix(consult): verify both PR refs up front; convert last ref-interpolated execSync; refresh GitRefResolver docstring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmap-3 round-2 follow-ups. Three findings: ## Verify head ref alongside base ref (Codex finding) The prior code rev-parse-verified `origin/${pr.baseRefName}` but not `origin/${pr.headRefName}` before building the three-dot diff range. If the head ref was missing/stale (fetch warned but proceeded), `getDiffStat` later failed inside `buildImplQuery`, whose pre-existing try/catch swallowed the error and degraded silently to "explore the filesystem" against the architect's local checkout — contradicting the explicit-crash comment. Loop verification over both refs so the failure surfaces here with the actionable "fetch both refs" message. ## Convert the last branch-introduced ref-interpolated execSync (Codex + Claude nit) `buildImplQuery` at the builder-side fallback still had `execSync(\`git merge-base HEAD ${defaultBranch}\`)`. The `defaultBranch` value comes from `resolveDefaultBranch()` (introduced by this branch), so it's branch-introduced ref-interpolation that the rest of this branch already converted to execFileSync argv-mode. Closes the consistency loop. ## Refresh GitRefResolver docstring (Codex finding) The class-level docstring still said fetch failure was "silent", which became inaccurate after commit 9b0ee17a added the stderr warning. Updated to describe the actual two-tier behavior: already-cached case stays silent, real failures warn. --- packages/codev/src/commands/consult/index.ts | 20 +++++++++++++------ .../codev/src/commands/porch/artifacts.ts | 9 ++++++--- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/packages/codev/src/commands/consult/index.ts b/packages/codev/src/commands/consult/index.ts index 0270439f..b8a6ba8e 100644 --- a/packages/codev/src/commands/consult/index.ts +++ b/packages/codev/src/commands/consult/index.ts @@ -1002,7 +1002,7 @@ function buildImplQuery( let changedFiles: string[] = []; try { const defaultBranch = resolveDefaultBranch(workspaceRoot); - const ref = diffRef ?? execSync(`git merge-base HEAD ${defaultBranch}`, { cwd: workspaceRoot, encoding: 'utf-8' }).trim(); + const ref = diffRef ?? execFileSync('git', ['merge-base', 'HEAD', defaultBranch], { cwd: workspaceRoot, encoding: 'utf-8' }).trim(); const result = getDiffStat(workspaceRoot, ref); diffStat = result.stat; changedFiles = result.files; @@ -1412,13 +1412,21 @@ function resolveArchitectQuery(workspaceRoot: string, type: string, options: Con // form do the rest. If verification fails, crash explicitly rather // than silently degrade to reviewing the architect's checked-out // tree (cmap-3 Gemini finding). + // Verify both refs up front. Without verifying head, getDiffStat would + // fail later inside buildImplQuery, which swallows diff errors and drops + // the reviewer into "explore the filesystem" — silently degrading the + // architect-mode review against whatever's checked out locally. Verify + // both so the failure surfaces here with an actionable message. + // cmap-3 round-2 finding (Codex). let diffRef: string; try { - execFileSync('git', ['rev-parse', '--verify', `origin/${pr.baseRefName}`], { - cwd: workspaceRoot, - encoding: 'utf-8', - stdio: ['ignore', 'pipe', 'pipe'], - }); + for (const refName of [pr.baseRefName, pr.headRefName]) { + execFileSync('git', ['rev-parse', '--verify', `origin/${refName}`], { + cwd: workspaceRoot, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + } diffRef = `origin/${pr.baseRefName}...origin/${pr.headRefName}`; } catch (err) { throw new Error( diff --git a/packages/codev/src/commands/porch/artifacts.ts b/packages/codev/src/commands/porch/artifacts.ts index 55dd7c45..7bd091ed 100644 --- a/packages/codev/src/commands/porch/artifacts.ts +++ b/packages/codev/src/commands/porch/artifacts.ts @@ -371,9 +371,12 @@ export class CliResolver implements ArtifactResolver { * reading directly from the ref the reviewer is meant to evaluate. * * Best-effort `git fetch origin ` is attempted once at construction - * for refs that include an `origin/` prefix; failure is silent so the - * resolver also works with refs that are already locally present - * (including bare branch names like `builder/777-foo`). + * for refs that include an `origin/` prefix. Fetch failure for the + * already-cached / offline case stays silent (the subsequent `git show` + * surfaces missing-ref errors), but auth / network / unknown failures emit + * a stderr warning since stale local refs could otherwise silently corrupt + * the review. Refs without an `origin/` prefix (e.g. local branches like + * `builder/777-foo`) skip the fetch entirely. */ export class GitRefResolver implements ArtifactResolver { constructor( From 5390eecc2d01fa964687cfd1aac0aa01ad70b608 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 16:32:44 +1000 Subject: [PATCH 15/18] =?UTF-8?q?[Bugfix=20#777]=20fix(protocols):=20compl?= =?UTF-8?q?ete=20the=20||=20echo=20main=20=E2=86=92=20${VAR:-main}=20fix?= =?UTF-8?q?=20in=20PIR=20(sites=20missed=20in=20f68f24e7)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmap-3 round-2 catch (Codex + Claude both flagged). The earlier shell-bug fix in commit f68f24e7 only caught two of the eight PIR sites — the inline doc strings at implement.md:116 and review.md:41. It missed the code-block snippets and the resumption-check one-liner: - pir/builder-prompt.md:85 (resumption after Claude session crash) - pir/prompts/implement.md:23 (initial DEFAULT_BRANCH resolution) - pir/prompts/review.md:252 (rebase-on-conflicts snippet) All three exist in both `codev/protocols/` and `codev-skeleton/protocols/` trees, so 6 files total in this commit. These all had the same latent shell bug f68f24e7 documented: `sed` exits 0 on empty input, so the pipeline exit code never reflects `git symbolic-ref`'s failure, the `|| echo main` doesn't fire, and $DEFAULT_BRANCH ends up empty. Applied the same `${DEFAULT_BRANCH:-main}` fix consistent with the canonical pattern. Audit grep across codev/protocols/ + codev-skeleton/protocols/ for `echo main` now returns zero hits. --- codev-skeleton/protocols/pir/builder-prompt.md | 2 +- codev-skeleton/protocols/pir/prompts/implement.md | 3 ++- codev-skeleton/protocols/pir/prompts/review.md | 3 ++- codev/protocols/pir/builder-prompt.md | 2 +- codev/protocols/pir/prompts/implement.md | 3 ++- codev/protocols/pir/prompts/review.md | 3 ++- 6 files changed, 10 insertions(+), 6 deletions(-) diff --git a/codev-skeleton/protocols/pir/builder-prompt.md b/codev-skeleton/protocols/pir/builder-prompt.md index 5ce94aa8..4d181f71 100644 --- a/codev-skeleton/protocols/pir/builder-prompt.md +++ b/codev-skeleton/protocols/pir/builder-prompt.md @@ -82,7 +82,7 @@ If you encounter **pre-existing flaky tests** (intermittent failures unrelated t If your Claude session crashes mid-flow, Tower's `while true` loop will relaunch you with the same prompt. On startup: 1. Run `porch next {{project_id}}` to learn what phase you're in -2. If `gate_pending`: read the latest plan file (plan-approval) or `git diff "$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || echo main)"` (dev-approval) plus any new GitHub issue comments; check `afx send` queue. Decide whether to revise or just announce you're back. +2. If `gate_pending`: read the latest plan file (plan-approval) or `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); git diff "${DEFAULT_BRANCH:-main}"` (dev-approval) plus any new GitHub issue comments; check `afx send` queue. Decide whether to revise or just announce you're back. 3. Otherwise: pick up where you left off ## Getting Started diff --git a/codev-skeleton/protocols/pir/prompts/implement.md b/codev-skeleton/protocols/pir/prompts/implement.md index cc31736c..0956822d 100644 --- a/codev-skeleton/protocols/pir/prompts/implement.md +++ b/codev-skeleton/protocols/pir/prompts/implement.md @@ -20,7 +20,8 @@ Run `porch next {{project_id}}`. If the response is `gate_pending` on `dev-appro 1. Resolve your repo's default branch (falls back to `main` if `origin/HEAD` isn't set): ```bash - DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || echo main) + DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||') + DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} ``` 2. Check for feedback: diff --git a/codev-skeleton/protocols/pir/prompts/review.md b/codev-skeleton/protocols/pir/prompts/review.md index 6e4dd27d..a522cdae 100644 --- a/codev-skeleton/protocols/pir/prompts/review.md +++ b/codev-skeleton/protocols/pir/prompts/review.md @@ -249,7 +249,8 @@ Together with the `--pr` record from step 4a and the `--merged` record from step **If the PR cannot be created (e.g., merge conflicts with the default branch):** - Rebase on the default branch: ```bash - DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || echo main) + DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||') + DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} git fetch origin "$DEFAULT_BRANCH" && git rebase "origin/$DEFAULT_BRANCH" ``` - Resolve conflicts (do NOT use destructive shortcuts) diff --git a/codev/protocols/pir/builder-prompt.md b/codev/protocols/pir/builder-prompt.md index 5ce94aa8..4d181f71 100644 --- a/codev/protocols/pir/builder-prompt.md +++ b/codev/protocols/pir/builder-prompt.md @@ -82,7 +82,7 @@ If you encounter **pre-existing flaky tests** (intermittent failures unrelated t If your Claude session crashes mid-flow, Tower's `while true` loop will relaunch you with the same prompt. On startup: 1. Run `porch next {{project_id}}` to learn what phase you're in -2. If `gate_pending`: read the latest plan file (plan-approval) or `git diff "$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || echo main)"` (dev-approval) plus any new GitHub issue comments; check `afx send` queue. Decide whether to revise or just announce you're back. +2. If `gate_pending`: read the latest plan file (plan-approval) or `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); git diff "${DEFAULT_BRANCH:-main}"` (dev-approval) plus any new GitHub issue comments; check `afx send` queue. Decide whether to revise or just announce you're back. 3. Otherwise: pick up where you left off ## Getting Started diff --git a/codev/protocols/pir/prompts/implement.md b/codev/protocols/pir/prompts/implement.md index cc31736c..0956822d 100644 --- a/codev/protocols/pir/prompts/implement.md +++ b/codev/protocols/pir/prompts/implement.md @@ -20,7 +20,8 @@ Run `porch next {{project_id}}`. If the response is `gate_pending` on `dev-appro 1. Resolve your repo's default branch (falls back to `main` if `origin/HEAD` isn't set): ```bash - DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || echo main) + DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||') + DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} ``` 2. Check for feedback: diff --git a/codev/protocols/pir/prompts/review.md b/codev/protocols/pir/prompts/review.md index 6e4dd27d..a522cdae 100644 --- a/codev/protocols/pir/prompts/review.md +++ b/codev/protocols/pir/prompts/review.md @@ -249,7 +249,8 @@ Together with the `--pr` record from step 4a and the `--merged` record from step **If the PR cannot be created (e.g., merge conflicts with the default branch):** - Rebase on the default branch: ```bash - DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || echo main) + DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||') + DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} git fetch origin "$DEFAULT_BRANCH" && git rebase "origin/$DEFAULT_BRANCH" ``` - Resolve conflicts (do NOT use destructive shortcuts) From c6144d7202cf23f09c88703259af02d378e288e5 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 16:45:06 +1000 Subject: [PATCH 16/18] [Bugfix #777][Bugfix #784] fix(protocols): anchor diff invocations at merge-base, not the moving base branch tip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmap-3 round-3 blocking finding (Codex). The variable-resolution fix from f68f24e7+5390eecc made `$DEFAULT_BRANCH` correctly resolve, but the diff invocations themselves still used the moving branch name: git diff "$DEFAULT_BRANCH" git diff --stat "$DEFAULT_BRANCH" Single-arg `git diff ` compares the ref's *current tip* to the working tree. When the base branch has advanced past the merge-base (typical for long-lived branches or non-`main` integration branches that absorb daily commits), this includes: ✓ Changes the builder made (committed + uncommitted) ✗ Changes the base branch absorbed since branching, shown as "reverted in worktree" — exactly the phantom scope-creep class #784 / #777 Defect C fixed in the consult CLI. Same bug, different layer. Builders following these PIR prompts run the bad diff *themselves* in their terminal — bypassing the consult CLI's correct merge-base resolution. ## Fix Anchor at the merge-base via `$(git merge-base "$DEFAULT_BRANCH" HEAD)`, matching what the consult CLI's builder-side path does internally at buildImplQuery (line 1007 post-cbdd129a): const ref = diffRef ?? execFileSync('git', ['merge-base', 'HEAD', defaultBranch], ...).trim(); For the multi-line setup blocks (pir/prompts/implement.md, BUGFIX protocol.md 300-LOC measurement), introduce a `MERGE_BASE` variable alongside `DEFAULT_BRANCH` and use it in subsequent diff invocations. For single-use inline contexts (pir/builder-prompt.md, the inline parentheticals in implement.md:117 and review.md:41), inline the `$(git merge-base ...)` call. Preserves the bugfix #280 contract: single-arg `git diff ` still compares against the working tree, so uncommitted changes show up. What's excluded is upstream churn — which was never the builder's work and shouldn't have been in the diff in the first place. ## Sites PIR (×2 trees, 6 files): builder-prompt.md, prompts/implement.md (setup block at line 22-26 + inline at 117), prompts/review.md. BUGFIX (codev/ only — skeleton's bugfix/protocol.md is the older shorter form that pre-dates the 300-LOC section): the LOC counter at line 327-331 now uses merge-base anchoring so commits the base picked up after branching aren't counted toward the 300-LOC budget. ## Audit `grep -rn 'git diff[^|]*"\$DEFAULT_BRANCH"' codev/protocols/ codev-skeleton/protocols/` after this commit: zero hits. Only the new `git merge-base "$DEFAULT_BRANCH" HEAD` wrappers appear, which is the correct form. --- codev-skeleton/protocols/pir/builder-prompt.md | 2 +- codev-skeleton/protocols/pir/prompts/implement.md | 7 ++++--- codev-skeleton/protocols/pir/prompts/review.md | 2 +- codev/protocols/bugfix/protocol.md | 5 ++++- codev/protocols/pir/builder-prompt.md | 2 +- codev/protocols/pir/prompts/implement.md | 7 ++++--- codev/protocols/pir/prompts/review.md | 2 +- 7 files changed, 16 insertions(+), 11 deletions(-) diff --git a/codev-skeleton/protocols/pir/builder-prompt.md b/codev-skeleton/protocols/pir/builder-prompt.md index 4d181f71..0edd1d99 100644 --- a/codev-skeleton/protocols/pir/builder-prompt.md +++ b/codev-skeleton/protocols/pir/builder-prompt.md @@ -82,7 +82,7 @@ If you encounter **pre-existing flaky tests** (intermittent failures unrelated t If your Claude session crashes mid-flow, Tower's `while true` loop will relaunch you with the same prompt. On startup: 1. Run `porch next {{project_id}}` to learn what phase you're in -2. If `gate_pending`: read the latest plan file (plan-approval) or `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); git diff "${DEFAULT_BRANCH:-main}"` (dev-approval) plus any new GitHub issue comments; check `afx send` queue. Decide whether to revise or just announce you're back. +2. If `gate_pending`: read the latest plan file (plan-approval) or `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); git diff "$(git merge-base "${DEFAULT_BRANCH:-main}" HEAD)"` (dev-approval) plus any new GitHub issue comments; check `afx send` queue. Decide whether to revise or just announce you're back. 3. Otherwise: pick up where you left off ## Getting Started diff --git a/codev-skeleton/protocols/pir/prompts/implement.md b/codev-skeleton/protocols/pir/prompts/implement.md index 0956822d..90e74423 100644 --- a/codev-skeleton/protocols/pir/prompts/implement.md +++ b/codev-skeleton/protocols/pir/prompts/implement.md @@ -17,15 +17,16 @@ Implement the approved plan, write tests, and pause at the `dev-approval` gate s Run `porch next {{project_id}}`. If the response is `gate_pending` on `dev-approval`, the code is already written and you're awaiting review. In that case: -1. Resolve your repo's default branch (falls back to `main` if `origin/HEAD` isn't set): +1. Resolve your repo's default branch and the merge-base. The merge-base anchors the diff at the branch's fork point, so commits the base branch picked up *after* you branched don't show up as phantom "scope creep". (`DEFAULT_BRANCH` falls back to `main` if `origin/HEAD` isn't set.) ```bash DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||') DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} + MERGE_BASE=$(git merge-base "$DEFAULT_BRANCH" HEAD) ``` 2. Check for feedback: - - `git diff "$DEFAULT_BRANCH"` — has the reviewer made any direct edits to your code? + - `git diff "$MERGE_BASE"` — has the reviewer made any direct edits to your code? - `gh issue view {{issue.number}} --comments` - `afx send` queue messages 3. If feedback requires code changes: make them, re-run build + tests, recommit. @@ -114,7 +115,7 @@ When the gate goes pending, output a short prose summary in the pane to orient t > **What changed**: 2–3 sentence summary. > -> **Files**: `git diff --stat "$DEFAULT_BRANCH"` style list — paths and +/-. (Resolve `$DEFAULT_BRANCH` once per session: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); DEFAULT_BRANCH=${DEFAULT_BRANCH:-main}`.) +> **Files**: `git diff --stat "$MERGE_BASE"` style list — paths and +/-. (Resolve `$MERGE_BASE` once per session: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); DEFAULT_BRANCH=${DEFAULT_BRANCH:-main}; MERGE_BASE=$(git merge-base "$DEFAULT_BRANCH" HEAD)`. Anchoring at the merge-base excludes commits the base branch picked up after you branched.) > > **Test results**: `npm run build` ✓, `npm test` ✓ (X tests, Y new). > diff --git a/codev-skeleton/protocols/pir/prompts/review.md b/codev-skeleton/protocols/pir/prompts/review.md index a522cdae..29aa6750 100644 --- a/codev-skeleton/protocols/pir/prompts/review.md +++ b/codev-skeleton/protocols/pir/prompts/review.md @@ -38,7 +38,7 @@ Fixes #{{issue.number}} ## Files Changed -Output of `git diff --stat "$DEFAULT_BRANCH"`, formatted as a list (resolve once: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); DEFAULT_BRANCH=${DEFAULT_BRANCH:-main}`): +Output of `git diff --stat "$MERGE_BASE"`, formatted as a list (resolve once: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); DEFAULT_BRANCH=${DEFAULT_BRANCH:-main}; MERGE_BASE=$(git merge-base "$DEFAULT_BRANCH" HEAD)`. Anchoring at the merge-base keeps the file list scoped to *your* changes, not commits the base branch absorbed after you branched.): - `path/to/file.ts` (+12 / -3) - `path/to/new-file.ts` (+45 / -0) diff --git a/codev/protocols/bugfix/protocol.md b/codev/protocols/bugfix/protocol.md index 532e32e8..f9d4b4bb 100644 --- a/codev/protocols/bugfix/protocol.md +++ b/codev/protocols/bugfix/protocol.md @@ -325,7 +325,10 @@ The "< 300 LOC" threshold is measured as **net diff** (additions + deletions): ```bash DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||') -git diff --stat "${DEFAULT_BRANCH:-main}" | tail -1 +DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} +# Anchor at the merge-base so commits the base branch picked up after you +# branched aren't counted toward this PR's LOC budget. +git diff --stat "$(git merge-base "$DEFAULT_BRANCH" HEAD)" | tail -1 # Example: "3 files changed, 145 insertions(+), 52 deletions(-)" # Net diff = 145 + 52 = 197 LOC ✓ (under 300) ``` diff --git a/codev/protocols/pir/builder-prompt.md b/codev/protocols/pir/builder-prompt.md index 4d181f71..0edd1d99 100644 --- a/codev/protocols/pir/builder-prompt.md +++ b/codev/protocols/pir/builder-prompt.md @@ -82,7 +82,7 @@ If you encounter **pre-existing flaky tests** (intermittent failures unrelated t If your Claude session crashes mid-flow, Tower's `while true` loop will relaunch you with the same prompt. On startup: 1. Run `porch next {{project_id}}` to learn what phase you're in -2. If `gate_pending`: read the latest plan file (plan-approval) or `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); git diff "${DEFAULT_BRANCH:-main}"` (dev-approval) plus any new GitHub issue comments; check `afx send` queue. Decide whether to revise or just announce you're back. +2. If `gate_pending`: read the latest plan file (plan-approval) or `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); git diff "$(git merge-base "${DEFAULT_BRANCH:-main}" HEAD)"` (dev-approval) plus any new GitHub issue comments; check `afx send` queue. Decide whether to revise or just announce you're back. 3. Otherwise: pick up where you left off ## Getting Started diff --git a/codev/protocols/pir/prompts/implement.md b/codev/protocols/pir/prompts/implement.md index 0956822d..90e74423 100644 --- a/codev/protocols/pir/prompts/implement.md +++ b/codev/protocols/pir/prompts/implement.md @@ -17,15 +17,16 @@ Implement the approved plan, write tests, and pause at the `dev-approval` gate s Run `porch next {{project_id}}`. If the response is `gate_pending` on `dev-approval`, the code is already written and you're awaiting review. In that case: -1. Resolve your repo's default branch (falls back to `main` if `origin/HEAD` isn't set): +1. Resolve your repo's default branch and the merge-base. The merge-base anchors the diff at the branch's fork point, so commits the base branch picked up *after* you branched don't show up as phantom "scope creep". (`DEFAULT_BRANCH` falls back to `main` if `origin/HEAD` isn't set.) ```bash DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||') DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} + MERGE_BASE=$(git merge-base "$DEFAULT_BRANCH" HEAD) ``` 2. Check for feedback: - - `git diff "$DEFAULT_BRANCH"` — has the reviewer made any direct edits to your code? + - `git diff "$MERGE_BASE"` — has the reviewer made any direct edits to your code? - `gh issue view {{issue.number}} --comments` - `afx send` queue messages 3. If feedback requires code changes: make them, re-run build + tests, recommit. @@ -114,7 +115,7 @@ When the gate goes pending, output a short prose summary in the pane to orient t > **What changed**: 2–3 sentence summary. > -> **Files**: `git diff --stat "$DEFAULT_BRANCH"` style list — paths and +/-. (Resolve `$DEFAULT_BRANCH` once per session: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); DEFAULT_BRANCH=${DEFAULT_BRANCH:-main}`.) +> **Files**: `git diff --stat "$MERGE_BASE"` style list — paths and +/-. (Resolve `$MERGE_BASE` once per session: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); DEFAULT_BRANCH=${DEFAULT_BRANCH:-main}; MERGE_BASE=$(git merge-base "$DEFAULT_BRANCH" HEAD)`. Anchoring at the merge-base excludes commits the base branch picked up after you branched.) > > **Test results**: `npm run build` ✓, `npm test` ✓ (X tests, Y new). > diff --git a/codev/protocols/pir/prompts/review.md b/codev/protocols/pir/prompts/review.md index a522cdae..29aa6750 100644 --- a/codev/protocols/pir/prompts/review.md +++ b/codev/protocols/pir/prompts/review.md @@ -38,7 +38,7 @@ Fixes #{{issue.number}} ## Files Changed -Output of `git diff --stat "$DEFAULT_BRANCH"`, formatted as a list (resolve once: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); DEFAULT_BRANCH=${DEFAULT_BRANCH:-main}`): +Output of `git diff --stat "$MERGE_BASE"`, formatted as a list (resolve once: `DEFAULT_BRANCH=$(git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||'); DEFAULT_BRANCH=${DEFAULT_BRANCH:-main}; MERGE_BASE=$(git merge-base "$DEFAULT_BRANCH" HEAD)`. Anchoring at the merge-base keeps the file list scoped to *your* changes, not commits the base branch absorbed after you branched.): - `path/to/file.ts` (+12 / -3) - `path/to/new-file.ts` (+45 / -0) From fa270cf3f195f632424b9cedcc374e85a22db443 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 17:42:34 +1000 Subject: [PATCH 17/18] test(protocols): add regression-guard audit for #777 / #784 prompt patterns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asserts three pattern classes are absent from every .md file under codev/protocols/ and codev-skeleton/protocols/, with file:line:text reporting on failure so a future author who re-introduces one gets a pointer to the canonical fix. ## What it guards 1. **`git diff "$DEFAULT_BRANCH"` (two-tree against moving tip — #784).** Single-arg `git diff ` compares the moving branch tip to the working tree, pulling in upstream churn. Use `$(git merge-base "$DEFAULT_BRANCH" HEAD)` or a pre-resolved `$MERGE_BASE` instead. Regex carefully scoped so it does NOT match the legitimate `git diff "$(git merge-base ...)"` form. 2. **`|| echo main` (shell-bug fallback that never fires).** `sed` exits 0 on empty input, so when `git symbolic-ref` fails the pipeline's exit status is sed's (0), `||` doesn't fire, and `DEFAULT_BRANCH` ends up empty. Use `${DEFAULT_BRANCH:-main}` parameter expansion instead. 3. **`git diff main` (hardcoded — #777 Defect B Layer 2).** The integration branch isn't always `main`. Resolve dynamically. Word boundary `\b` avoids false positives like `main goal`, `maintain`, `mainline`. ## What it doesn't guard This is a regression test, not a correctness oracle. It only catches the three patterns this branch fought. New bug shapes need new assertions. Discussed as the cheaper alternative to the larger "extract a `consult --print-diff` flag for protocol prompts to share" refactor. ~50 LOC + <100ms test cost vs. ~150 LOC of CLI plumbing — and the ~10 prompt sites it guards are already correct, so the higher-cost refactor doesn't pay for itself. --- .../__tests__/protocol-prompt-audit.test.ts | 110 ++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 packages/codev/src/__tests__/protocol-prompt-audit.test.ts diff --git a/packages/codev/src/__tests__/protocol-prompt-audit.test.ts b/packages/codev/src/__tests__/protocol-prompt-audit.test.ts new file mode 100644 index 00000000..8413549c --- /dev/null +++ b/packages/codev/src/__tests__/protocol-prompt-audit.test.ts @@ -0,0 +1,110 @@ +/** + * Protocol prompt audit — regression guards for cluesmith/codev#777 and #784. + * + * This branch fixed three specific bug classes in the protocol .md files that + * builder agents read and execute. Each `it()` block below grep-asserts that + * the pattern is gone and stays gone. If a future protocol author re-introduces + * one of these, the test fails with the offending file, line, and text. + * + * Not a correctness oracle — only catches the three bug classes named here. + * New bug shapes will need their own assertions. + */ + +import { describe, it, expect } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); +const repoRoot = path.resolve(__dirname, '../../../..'); + +function walkMarkdown(dir: string): string[] { + if (!fs.existsSync(dir)) return []; + const out: string[] = []; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + out.push(...walkMarkdown(full)); + } else if (entry.isFile() && entry.name.endsWith('.md')) { + out.push(full); + } + } + return out; +} + +interface Violation { + file: string; + line: number; + text: string; +} + +function findViolations(files: string[], pattern: RegExp): Violation[] { + const hits: Violation[] = []; + for (const file of files) { + const lines = fs.readFileSync(file, 'utf-8').split('\n'); + lines.forEach((line, idx) => { + if (pattern.test(line)) { + hits.push({ + file: path.relative(repoRoot, file), + line: idx + 1, + text: line.trim(), + }); + } + }); + } + return hits; +} + +function formatViolations(violations: Violation[]): string { + if (violations.length === 0) return ''; + return ( + `\nFound ${violations.length} violation(s):\n` + + violations.map(v => ` ${v.file}:${v.line}\n ${v.text}`).join('\n') + ); +} + +describe('protocol prompt audit (#777 / #784 regression guards)', () => { + const protocolDirs = [ + path.join(repoRoot, 'codev/protocols'), + path.join(repoRoot, 'codev-skeleton/protocols'), + ]; + const files = protocolDirs.flatMap(walkMarkdown); + + it('finds protocol .md files to audit', () => { + // Sanity check — if this fails the regex tests below would silently pass. + expect(files.length).toBeGreaterThan(0); + }); + + it('no `git diff "$DEFAULT_BRANCH"` (two-tree diff against the moving base tip — #784)', () => { + // Matches `git diff [--stat] "$DEFAULT_BRANCH"` or `"${DEFAULT_BRANCH:-main}"` + // when the variable is the *bare* argument — NOT wrapped in + // `$(git merge-base ...)`. Single-arg `git diff ` compares the moving + // branch tip against the working tree, pulling in upstream churn the + // builder didn't author. Use `$(git merge-base "$DEFAULT_BRANCH" HEAD)` or + // a pre-resolved `$MERGE_BASE` variable instead. + const bad = /git diff(\s+--stat)?\s+"\$(\{DEFAULT_BRANCH(:-[^}]*)?\}|DEFAULT_BRANCH)"/; + const violations = findViolations(files, bad); + expect(violations, formatViolations(violations)).toEqual([]); + }); + + it('no `|| echo main` shell-bug pattern (sed-swallows-pipeline-exit-code)', () => { + // sed exits 0 on empty input, so when `git symbolic-ref` fails the + // pipeline's exit status is sed's (0), and `|| echo main` never fires. + // DEFAULT_BRANCH ends up empty rather than `main`. Use + // `${DEFAULT_BRANCH:-main}` parameter expansion instead. + const bad = /\|\|\s+echo\s+main/; + const violations = findViolations(files, bad); + expect(violations, formatViolations(violations)).toEqual([]); + }); + + it('no hardcoded `git diff main` (#777 Defect B Layer 2)', () => { + // The integration branch isn't always `main` (e.g. `ci`, `develop`, + // `trunk`). Resolve it dynamically via `git symbolic-ref --short + // refs/remotes/origin/HEAD` with a fallback. Excludes false-positive + // matches like "main goal", "maintain", etc. by requiring a word boundary. + const bad = /git diff(\s+--stat)?\s+main\b/; + const violations = findViolations(files, bad); + expect(violations, formatViolations(violations)).toEqual([]); + }); +}); From 76bafa01d84f64b7a726f580c936c500859714c5 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Fri, 22 May 2026 19:45:44 +1000 Subject: [PATCH 18/18] docs(consult-types/pr-review): add reviewer-scope rule against flagging quoted git diff prose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Observed in the wild: while exercising this branch's fixes downstream, Codex flagged the two-dot syntax of a `git diff` *example quoted in review-file prose* as a defect. The justification cited "the repo's review instructions" — but no such instruction existed. Codex pattern-matched the two-dot/three-dot concern (legitimate for diffs the reviewer *computes itself*, per #784) onto a documentation string that wasn't a command at all. The fix is a one-bullet scope rule shipped to every protocol's pr-review.md, so the rule lands regardless of which protocol's prompt gets rendered. Codex doesn't see protocol identity at review time, so the rule has to be near-clone across all six. ## Bullet shipped For SPIR / ASPIR / AIR / MAINTAIN — new `## Scope` section before `## Verdict Format`, matching PIR's existing section header: - **DO NOT** flag the syntax of `git diff` examples that appear in review-file prose (e.g., `git diff ci..HEAD` inside a "Files Changed" caption or "How to Test Locally" section). Quoted diff syntax is documentation, not a command. Apply two-dot/three-dot scrutiny only to diffs you compute yourself. For PIR — added to the existing `## Scope` section's DO NOT list. For BUGFIX — added (in DO NOT form) to the existing `## Out of Scope (Do NOT request changes for)` section. ## Why this is a follow-up not a defect fix This is calibration for over-pattern-matching, not a codev bug. The CLI and protocol prompts in this branch are already correct. The new rule just tells the reviewer agent how to apply the two-dot/three-dot lens — to its own computations, not to quoted documentation strings. ## Audit / regression `grep -rln "DO NOT.*flag the syntax of .git diff. examples"` → 12 files (6 protocols × 2 trees), as expected. `pnpm --filter @cluesmith/codev test src/__tests__/protocol-prompt-audit.test.ts` still passes (the bullet doesn't trip any of the three guards: the `ci..HEAD` example doesn't contain `main` or `$DEFAULT_BRANCH`). --- codev-skeleton/protocols/air/consult-types/pr-review.md | 4 ++++ codev-skeleton/protocols/aspir/consult-types/pr-review.md | 4 ++++ codev-skeleton/protocols/bugfix/consult-types/pr-review.md | 1 + codev-skeleton/protocols/maintain/consult-types/pr-review.md | 4 ++++ codev-skeleton/protocols/pir/consult-types/pr-review.md | 1 + codev-skeleton/protocols/spir/consult-types/pr-review.md | 4 ++++ codev/protocols/air/consult-types/pr-review.md | 4 ++++ codev/protocols/aspir/consult-types/pr-review.md | 4 ++++ codev/protocols/bugfix/consult-types/pr-review.md | 1 + codev/protocols/maintain/consult-types/pr-review.md | 4 ++++ codev/protocols/pir/consult-types/pr-review.md | 1 + codev/protocols/spir/consult-types/pr-review.md | 4 ++++ 12 files changed, 36 insertions(+) diff --git a/codev-skeleton/protocols/air/consult-types/pr-review.md b/codev-skeleton/protocols/air/consult-types/pr-review.md index e291b9c2..0d7856f3 100644 --- a/codev-skeleton/protocols/air/consult-types/pr-review.md +++ b/codev-skeleton/protocols/air/consult-types/pr-review.md @@ -36,6 +36,10 @@ If the baked decisions themselves contain contradictions, do not pick one — `R - Is the PR body review section informative? - Is the branch up to date with its base (the integration branch the PR targets)? +## Scope + +- **DO NOT** flag the syntax of `git diff` examples that appear in review-file prose (e.g., `git diff ci..HEAD` inside a "Files Changed" caption or "How to Test Locally" section). Quoted diff syntax is documentation, not a command. Apply two-dot/three-dot scrutiny only to diffs you compute yourself. + ## Verdict Format After your review, provide your verdict in exactly this format: diff --git a/codev-skeleton/protocols/aspir/consult-types/pr-review.md b/codev-skeleton/protocols/aspir/consult-types/pr-review.md index 7d2ef518..837cdea3 100644 --- a/codev-skeleton/protocols/aspir/consult-types/pr-review.md +++ b/codev-skeleton/protocols/aspir/consult-types/pr-review.md @@ -32,6 +32,10 @@ You are performing a final self-check during the Review phase. The builder has c - Are commits atomic and well-described? - Is the change diff reasonable in size? +## Scope + +- **DO NOT** flag the syntax of `git diff` examples that appear in review-file prose (e.g., `git diff ci..HEAD` inside a "Files Changed" caption or "How to Test Locally" section). Quoted diff syntax is documentation, not a command. Apply two-dot/three-dot scrutiny only to diffs you compute yourself. + ## Verdict Format After your review, provide your verdict in exactly this format: diff --git a/codev-skeleton/protocols/bugfix/consult-types/pr-review.md b/codev-skeleton/protocols/bugfix/consult-types/pr-review.md index b617a909..efdb7a17 100644 --- a/codev-skeleton/protocols/bugfix/consult-types/pr-review.md +++ b/codev-skeleton/protocols/bugfix/consult-types/pr-review.md @@ -54,6 +54,7 @@ The following are **not** part of the BUGFIX protocol and must **not** be cited - Commit format `[Spec NNNN][Phase]` — BUGFIX intentionally uses `Fix #N:` / `[Bugfix #N]`. - `status.yaml` fields like `build_complete: false` — porch manages `status.yaml`; the builder is **forbidden** from editing it directly. Treat porch state as informational, not a fixable issue. - Phase-scoping concerns — BUGFIX is a single-phase protocol; there are no plan phases to scope against. +- **DO NOT** flag the syntax of `git diff` examples that appear in review-file prose (e.g., `git diff ci..HEAD` inside a "Files Changed" caption). Quoted diff syntax is documentation, not a command. Apply two-dot/three-dot scrutiny only to diffs you compute yourself. ## Verdict Format diff --git a/codev-skeleton/protocols/maintain/consult-types/pr-review.md b/codev-skeleton/protocols/maintain/consult-types/pr-review.md index 7d2ef518..837cdea3 100644 --- a/codev-skeleton/protocols/maintain/consult-types/pr-review.md +++ b/codev-skeleton/protocols/maintain/consult-types/pr-review.md @@ -32,6 +32,10 @@ You are performing a final self-check during the Review phase. The builder has c - Are commits atomic and well-described? - Is the change diff reasonable in size? +## Scope + +- **DO NOT** flag the syntax of `git diff` examples that appear in review-file prose (e.g., `git diff ci..HEAD` inside a "Files Changed" caption or "How to Test Locally" section). Quoted diff syntax is documentation, not a command. Apply two-dot/three-dot scrutiny only to diffs you compute yourself. + ## Verdict Format After your review, provide your verdict in exactly this format: diff --git a/codev-skeleton/protocols/pir/consult-types/pr-review.md b/codev-skeleton/protocols/pir/consult-types/pr-review.md index 3a9032fd..cf49c3a0 100644 --- a/codev-skeleton/protocols/pir/consult-types/pr-review.md +++ b/codev-skeleton/protocols/pir/consult-types/pr-review.md @@ -57,6 +57,7 @@ KEY_ISSUES: - **DO** flag obvious problems the human reviewer at the gate might have missed - **DO NOT** redesign the approach — that was settled at `plan-approval` and validated at `dev-approval` - **DO NOT** demand changes the human reviewer already accepted at the `dev-approval` gate (the human ran the code and approved it; you didn't) +- **DO NOT** flag the syntax of `git diff` examples that appear in review-file prose (e.g., `git diff ci..HEAD` inside a "Files Changed" caption or "How to Test Locally" section). Quoted diff syntax is documentation, not a command. Apply two-dot/three-dot scrutiny only to diffs you compute yourself. ## Notes diff --git a/codev-skeleton/protocols/spir/consult-types/pr-review.md b/codev-skeleton/protocols/spir/consult-types/pr-review.md index 7d2ef518..837cdea3 100644 --- a/codev-skeleton/protocols/spir/consult-types/pr-review.md +++ b/codev-skeleton/protocols/spir/consult-types/pr-review.md @@ -32,6 +32,10 @@ You are performing a final self-check during the Review phase. The builder has c - Are commits atomic and well-described? - Is the change diff reasonable in size? +## Scope + +- **DO NOT** flag the syntax of `git diff` examples that appear in review-file prose (e.g., `git diff ci..HEAD` inside a "Files Changed" caption or "How to Test Locally" section). Quoted diff syntax is documentation, not a command. Apply two-dot/three-dot scrutiny only to diffs you compute yourself. + ## Verdict Format After your review, provide your verdict in exactly this format: diff --git a/codev/protocols/air/consult-types/pr-review.md b/codev/protocols/air/consult-types/pr-review.md index e291b9c2..0d7856f3 100644 --- a/codev/protocols/air/consult-types/pr-review.md +++ b/codev/protocols/air/consult-types/pr-review.md @@ -36,6 +36,10 @@ If the baked decisions themselves contain contradictions, do not pick one — `R - Is the PR body review section informative? - Is the branch up to date with its base (the integration branch the PR targets)? +## Scope + +- **DO NOT** flag the syntax of `git diff` examples that appear in review-file prose (e.g., `git diff ci..HEAD` inside a "Files Changed" caption or "How to Test Locally" section). Quoted diff syntax is documentation, not a command. Apply two-dot/three-dot scrutiny only to diffs you compute yourself. + ## Verdict Format After your review, provide your verdict in exactly this format: diff --git a/codev/protocols/aspir/consult-types/pr-review.md b/codev/protocols/aspir/consult-types/pr-review.md index 7d2ef518..837cdea3 100644 --- a/codev/protocols/aspir/consult-types/pr-review.md +++ b/codev/protocols/aspir/consult-types/pr-review.md @@ -32,6 +32,10 @@ You are performing a final self-check during the Review phase. The builder has c - Are commits atomic and well-described? - Is the change diff reasonable in size? +## Scope + +- **DO NOT** flag the syntax of `git diff` examples that appear in review-file prose (e.g., `git diff ci..HEAD` inside a "Files Changed" caption or "How to Test Locally" section). Quoted diff syntax is documentation, not a command. Apply two-dot/three-dot scrutiny only to diffs you compute yourself. + ## Verdict Format After your review, provide your verdict in exactly this format: diff --git a/codev/protocols/bugfix/consult-types/pr-review.md b/codev/protocols/bugfix/consult-types/pr-review.md index b617a909..efdb7a17 100644 --- a/codev/protocols/bugfix/consult-types/pr-review.md +++ b/codev/protocols/bugfix/consult-types/pr-review.md @@ -54,6 +54,7 @@ The following are **not** part of the BUGFIX protocol and must **not** be cited - Commit format `[Spec NNNN][Phase]` — BUGFIX intentionally uses `Fix #N:` / `[Bugfix #N]`. - `status.yaml` fields like `build_complete: false` — porch manages `status.yaml`; the builder is **forbidden** from editing it directly. Treat porch state as informational, not a fixable issue. - Phase-scoping concerns — BUGFIX is a single-phase protocol; there are no plan phases to scope against. +- **DO NOT** flag the syntax of `git diff` examples that appear in review-file prose (e.g., `git diff ci..HEAD` inside a "Files Changed" caption). Quoted diff syntax is documentation, not a command. Apply two-dot/three-dot scrutiny only to diffs you compute yourself. ## Verdict Format diff --git a/codev/protocols/maintain/consult-types/pr-review.md b/codev/protocols/maintain/consult-types/pr-review.md index 7d2ef518..837cdea3 100644 --- a/codev/protocols/maintain/consult-types/pr-review.md +++ b/codev/protocols/maintain/consult-types/pr-review.md @@ -32,6 +32,10 @@ You are performing a final self-check during the Review phase. The builder has c - Are commits atomic and well-described? - Is the change diff reasonable in size? +## Scope + +- **DO NOT** flag the syntax of `git diff` examples that appear in review-file prose (e.g., `git diff ci..HEAD` inside a "Files Changed" caption or "How to Test Locally" section). Quoted diff syntax is documentation, not a command. Apply two-dot/three-dot scrutiny only to diffs you compute yourself. + ## Verdict Format After your review, provide your verdict in exactly this format: diff --git a/codev/protocols/pir/consult-types/pr-review.md b/codev/protocols/pir/consult-types/pr-review.md index 3a9032fd..cf49c3a0 100644 --- a/codev/protocols/pir/consult-types/pr-review.md +++ b/codev/protocols/pir/consult-types/pr-review.md @@ -57,6 +57,7 @@ KEY_ISSUES: - **DO** flag obvious problems the human reviewer at the gate might have missed - **DO NOT** redesign the approach — that was settled at `plan-approval` and validated at `dev-approval` - **DO NOT** demand changes the human reviewer already accepted at the `dev-approval` gate (the human ran the code and approved it; you didn't) +- **DO NOT** flag the syntax of `git diff` examples that appear in review-file prose (e.g., `git diff ci..HEAD` inside a "Files Changed" caption or "How to Test Locally" section). Quoted diff syntax is documentation, not a command. Apply two-dot/three-dot scrutiny only to diffs you compute yourself. ## Notes diff --git a/codev/protocols/spir/consult-types/pr-review.md b/codev/protocols/spir/consult-types/pr-review.md index 7d2ef518..837cdea3 100644 --- a/codev/protocols/spir/consult-types/pr-review.md +++ b/codev/protocols/spir/consult-types/pr-review.md @@ -32,6 +32,10 @@ You are performing a final self-check during the Review phase. The builder has c - Are commits atomic and well-described? - Is the change diff reasonable in size? +## Scope + +- **DO NOT** flag the syntax of `git diff` examples that appear in review-file prose (e.g., `git diff ci..HEAD` inside a "Files Changed" caption or "How to Test Locally" section). Quoted diff syntax is documentation, not a command. Apply two-dot/three-dot scrutiny only to diffs you compute yourself. + ## Verdict Format After your review, provide your verdict in exactly this format: