fix(consult): correct diff scope on non-default integration branches#821
Merged
waleedkadous merged 18 commits intoMay 22, 2026
Merged
Conversation
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.
… three-dot diff semantics Closes #784 (consult: impl-review uses two-dot <base>..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 `<merge-base>..origin/<head>` to `<merge-base>...origin/<head>` (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 <mergeBase>` in `getDiffStat` is still working-tree diff, so uncommitted changes in builder worktrees keep showing up. All 49 existing consult-related tests pass.
…r for spec/plan artifacts 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 <ref>:<path>`. Best-effort `git fetch origin <branch>` at construction for `origin/`-prefixed refs. - New `--branch <ref>` 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/<headRefName>; otherwise falls back to the local resolver with a stderr warning. The architect-mode impl path likewise reads spec/plan from origin/<headRefName> 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/<headRefName> 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.
…ral "main" in non-PIR protocols Closes #777 Defect B Layer 2 for the non-PIR protocols. PIR was already cleaned in 01f733d; 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 `<integration-branch>` 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.
…f-based artifact resolution 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 `<merge-base>...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 <mergeBase>` 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.
…fter main → integration-branch rename
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).
… falls back to main when origin/HEAD unset 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.
…RefName, not repo default branch 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/<baseRefName>` — 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/<base>...origin/<head>`), 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.
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.
…y 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").
…xecFileSync for ref interpolation
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/<base>
origin/<head>` → embed as `${SHA}...origin/<head>`) produced
*identical* output to the strategy-2 path (just
`origin/<base>...origin/<head>` 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 <ref>` (head + base, in the for-loop).
- `git rev-parse --verify origin/<baseRefName>`.
- `getDiffStat`'s `git diff --stat <ref>` and `git diff --name-only
<ref>` — `ref` is now sometimes the three-dot range
`origin/<base>...origin/<head>`, 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.
… to "main" correctly The pattern shipped in earlier commits — both my D1 fix (2e48467, 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 <url>` 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.
…hout baseRefName 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.
…t ref-interpolated execSync; refresh GitRefResolver docstring
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 9b0ee17 added the stderr
warning. Updated to describe the actual two-tier behavior:
already-cached case stays silent, real failures warn.
…} fix in PIR (sites missed in f68f24e) cmap-3 round-2 catch (Codex + Claude both flagged). The earlier shell-bug fix in commit f68f24e 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 f68f24e 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.
… merge-base, not the moving base branch tip cmap-3 round-3 blocking finding (Codex). The variable-resolution fix from f68f24e+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 <ref>` 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 <merge-base-SHA>` 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.
…tterns 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 <ref>` 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.
…ng quoted git diff prose 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`).
waleedkadous
approved these changes
May 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #777 and #784 — three stacking false-positive mechanisms in
consult --type implthat produced phantom "scope creep" verdicts on PRs against non-mainintegration branches and on branches behind their base.consult/index.tshardcoded'main'as the merge-base source at two sites. On repos whoseorigin/HEADis e.g.ci, the diff swept everyci-only commit into the builder's "scope". Fix: newresolveDefaultBranch(workspaceRoot)helper readsgit symbolic-ref --short refs/remotes/origin/HEADwith amainfallback, wired into both sites.<base>..HEADdiff semantics; over-includes when branch is behind on rebases #784 ≡ consult --type {spec,plan,impl} resolves artifact from invoking workspace (not the builder's branch); impl-review diff base hardcoded to main #777 Defect C) — architect-mode impl emitted a two-dot range; the prompt told reviewersDo NOT rely on git diffs(pushing them to compute their own two-dot diff); the merge-base failure path dropped to an unanchored "explore the filesystem". Fix: three-dot, scope-anchor prompt that names the file list as canonical, three-dot directive in the fallback, explicit crash (not silent degrade) on merge-base failure.findSpecContent/findPlanContentread from the architect's invoking workspace, not the builder's branch. Architect-supplied consults on a missing review returned the stale on-integration-branch artifact, so reviewers REQUEST_CHANGES'd findings the reworked artifact had already corrected. Fix: newGitRefResolverclass reads viagit show <ref>:<path>; new--branch <ref>CLI flag; default-resolver picksorigin/<headRefName>when a PR exists for the issue.Also closes the same bug class one layer deeper (caught by cmap-3 review):
pr.baseRefName(not the repo default) — when a PR targets a non-default integration branch, the previous fix was still wrong.codev/protocols/+codev-skeleton/protocols/no longer tell agents to literally executegit diff main,git checkout main, "Commit to `main`", etc.git diff "\$DEFAULT_BRANCH"(two-tree against moving tip) — anchored at\$(git merge-base "\$DEFAULT_BRANCH" HEAD)to match consult's internal logic.... | sed '...' || echo mainnever fires because sed exits 0 on empty input; switched to\${DEFAULT_BRANCH:-main}parameter expansion across all sites (verified empirically).pr-search.shforge override that pre-datesbaseRefNameno longer crashes the CLI — falls back toresolveDefaultBranchwith a loud warning.Defensive hardening
execSynccalls converted toexecFileSyncargv-mode (closes a small ref-name injection surface — git'scheck-ref-formatpermits;,\$,&, etc.).protocol-prompt-audit.test.tsasserts the three pattern classes (git diff "\$DEFAULT_BRANCH",|| echo main, hardcodedgit diff main) stay out of every protocol .md file.Test plan
pnpm --filter @cluesmith/codev test— 3000 pass / 13 skippedpnpm --filter @cluesmith/codev buildgrep -rn -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/protocolsgrep -rn "echo main" codev/protocols codev-skeleton/protocols— zero hitsmainintegration branch:consult --type impl --issue <N>against a real PR —--debugoutput shows no literalgit merge-base HEAD main, and the "Changed Files" list matchesgh pr diff <N> --name-onlyconsult --type spec --issue <N>and confirm stderr reportsSource: origin/<headRefName>and the spec content matches the builder branch's version