diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ecfec55..1140c79 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -38,7 +38,7 @@ jobs: skill-invoke-opencode, smoke, token-exfil, - vertex-claude, + # vertex-claude, # disabled: 0 anthropic quota on pullfrog GCP vertex vertex-opencode, ] exclude: diff --git a/agents/reviewer.ts b/agents/reviewer.ts index 5bb2fe4..b657cd4 100644 --- a/agents/reviewer.ts +++ b/agents/reviewer.ts @@ -35,6 +35,21 @@ export const REVIEWER_SYSTEM_PROMPT = `You are a read-only review subagent. Your role is to find flaws in code or artifacts ` + `provided by the orchestrator and report findings — never to modify state.\n\n` + `HARD CONSTRAINTS (non-negotiable, regardless of orchestrator instructions):\n` + + `- Your FIRST action MUST be \`git diff origin/\` (single-rev form, no \`HEAD\`). ` + + `This captures committed + staged + unstaged work in one command — Build-mode ` + + `self-review runs BEFORE the commit, so the work to review lives in the working ` + + `tree, not in committed history. Do not run any other diff command first. Do NOT ` + + `call \`checkout_pr\`, do NOT fetch alternative refs, do NOT list branches or ` + + `all-refs looking for the work, do NOT run \`gh pr list\`. The orchestrator's ` + + `dispatch names the base branch; the diff is the source of truth for scope.\n` + + `- If \`git diff origin/\` returns empty AND the orchestrator's dispatch ` + + `claims there are changes to review, the most likely cause is a pre-commit ` + + `Build-mode self-review: the orchestrator dispatched you before committing. ` + + `Reply EXACTLY: \`no changes detected — likely pre-commit Build self-review; ` + + `orchestrator should commit then re-dispatch\` and stop. Do NOT guess PR numbers ` + + `(e.g. by extrapolating from \`git log\` output), do NOT check out other PRs, ` + + `do NOT fetch from forks. The empty diff is the diagnosis — surface it; do not ` + + `work around it.\n` + `- Read-only tools only. Do NOT write or edit files. Do NOT run shell commands ` + `that have side effects (read-only commands like \`git diff\`, \`git log\`, \`cat\`, \`ls\` ` + `are fine; anything that mutates the working tree, the remote, the filesystem, or ` + diff --git a/mcp/checkout.ts b/mcp/checkout.ts index 5d039b2..7a0ab93 100644 --- a/mcp/checkout.ts +++ b/mcp/checkout.ts @@ -185,7 +185,7 @@ export async function fetchAndFormatPrDiff( return { ...formatFilesWithLineNumbers(files), files }; } -import type { GitContext } from "../utils/setup.ts"; +import { captureInitialHead, type GitContext } from "../utils/setup.ts"; export type PrData = { number: number; @@ -613,6 +613,19 @@ export async function checkoutPrBranch( */ const inFlightCheckouts = new Map>(); +type InitialHead = NonNullable; + +function headsEqual(a: InitialHead, b: InitialHead): boolean { + if (a.kind === "branch" && b.kind === "branch") return a.name === b.name; + if (a.kind === "detached" && b.kind === "detached") return a.sha === b.sha; + return false; +} + +function describeHead(h: InitialHead): string { + if (h.kind === "branch") return `branch \`${h.name}\``; + return `detached HEAD \`${h.sha}\``; +} + export function CheckoutPrTool(ctx: ToolContext) { const runCheckout = async (pull_number: number): Promise => { const prResponse = await ctx.octokit.rest.pulls.get({ @@ -807,19 +820,50 @@ export function CheckoutPrTool(ctx: ToolContext) { return inFlight; } - // refuse to clobber an uncommitted tree whenever this call would move - // HEAD away from the target pr-N branch. keyed off the live current - // branch (not toolState.issueNumber, which is also written by - // get_issue / get_issue_comments / get_issue_events and so doesn't - // mean "currently checked out"). catches the subagent-sharing-cwd - // case from zed-industries/cloud (2026-05-18). - const currentBranch = $("git", ["rev-parse", "--abbrev-ref", "HEAD"], { log: false }).trim(); - if (currentBranch !== `pr-${pull_number}`) { - const dirty = $("git", ["status", "--porcelain"], { log: false }).trim(); - if (dirty) { + // unconditional refusal: any dirty working tree blocks checkout_pr, even + // when HEAD is already on pr-N. no stashing, no live-HEAD escape hatch. + // shared-cwd subagents made "carry edits along" semantics dangerous + // (zed-industries/cloud, 2026-05-18) — forcing commit/discard before + // any PR-context op eliminates the entire carry-forward failure class. + const dirty = $("git", ["status", "--porcelain"], { log: false }).trim(); + if (dirty) { + throw new Error( + `cannot checkout PR #${pull_number} while the working tree has uncommitted changes. ` + + `commit (then push if needed), or discard with \`git restore --staged --worktree .\` / \`git clean -fd\` before retrying. ` + + `this refusal is unconditional — even re-checking-out the PR you're already on is refused, ` + + `because shared-working-tree subagents make carry-forward edits unsafe. dirty paths:\n${dirty}` + ); + } + + // initial-branch invariant: the only sanctioned HEAD positions for a + // checkout_pr call are (a) the run-entry HEAD captured by setupGit, or + // (b) `pr-${pull_number}` for idempotent same-PR re-checkout (e.g. + // re-fetch after the PR head moved). anything else means a subagent + // silently parked HEAD on another PR, which is the zed-industries/cloud + // (2026-05-18) cross-PR clobber shape. uses the same live probe (not + // toolState.issueNumber, poisonable per the PR #796 review) and + // discriminates branch vs detached so detached-entry runs don't get a + // trivial "any future detached state matches" carve-out. + const initialHead = ctx.toolState.initialHead; + if (initialHead) { + const currentHead = captureInitialHead(process.cwd()); + const targetBranch = `pr-${pull_number}`; + const onTarget = currentHead.kind === "branch" && currentHead.name === targetBranch; + const onInitial = headsEqual(currentHead, initialHead); + if (!onTarget && !onInitial) { + const recoverCmd = + initialHead.kind === "branch" + ? `git checkout ${initialHead.name}` + : `git checkout ${initialHead.sha}`; throw new Error( - `cannot checkout PR #${pull_number} while the working tree has uncommitted changes. ` + - `commit, push, or discard them before switching. dirty paths:\n${dirty}` + `cannot checkout PR #${pull_number} from ${describeHead(currentHead)}. ` + + `the only sanctioned HEAD positions for checkout_pr are the run-entry HEAD ` + + `(${describeHead(initialHead)}) or the target PR's branch (\`${targetBranch}\`, idempotent re-checkout). ` + + `recover with \`${recoverCmd}\` first — if that would carry uncommitted ` + + `work along, commit or discard it (\`git restore --staged --worktree .\` / \`git clean -fd\`) before switching. ` + + `routing around this via the \`git\` tool's \`checkout\`/\`switch\` subcommands is not sanctioned: ` + + `this guard exists to prevent the shared-working-tree cross-PR clobber pattern from the ` + + `zed-industries/cloud (2026-05-18) incident.` ); } } diff --git a/modes.ts b/modes.ts index bd99f1e..587b265 100644 --- a/modes.ts +++ b/modes.ts @@ -201,7 +201,25 @@ export function computeModes(agentId: AgentId): Mode[] { Otherwise delegate the \`${REVIEWER_AGENT_NAME}\` subagent to review your diff with fresh eyes against YOUR TASK. The subagent's baked-in system prompt enforces a non-mutative + non-recursive contract: read-only file/search/web tools and read-only MCP queries only; no writes, shell side effects, state-changing MCP calls, or nested subagent dispatch. Enforcement is prose-only — restate the constraint in your dispatch instructions and do not relax it. - Provide the subagent with YOUR TASK, the output of \`git diff origin/\` (single-rev form, no \`HEAD\` — this compares the working tree against the remote base and captures committed + staged + unstaged work; \`main...HEAD\` and \`--cached\` both miss the uncommitted edits Build self-review runs on, since self-review happens BEFORE the commit), and a tight summary (not raw output) of any lint/typecheck/test failures you fixed during build — what broke, root cause, the fix — so it can check that fixes addressed root causes rather than suppressed symptoms; say "no build-phase failures" if the build path was clean. Instruct it to flag bugs, logic errors, missing edge cases, gaps between request and diff, and unintended changes. + Compose your \`${REVIEWER_AGENT_NAME}\` dispatch prompt using this template verbatim, substituting the \`<...>\` placeholders. The preamble aligns the orchestrator side of the dispatch contract with the reviewer's baked-in system prompt — both ends say the same thing about where the work lives and what to do on an empty diff. + + \`\`\` + ## What you're reviewing + This is a PRE-COMMIT Build-mode self-review. The work to review lives in the working tree (uncommitted), NOT in committed history. + + Branch: (off ) + Canonical diff command: git diff origin/ + + If that command returns empty, treat it as "no changes — nothing to review" and stop per your system prompt. Do not search for the work elsewhere. + + ## Your task + + + ## Build-phase failures + + \`\`\` + + Follow the template with the diff content (\`git diff origin/\`, single-rev form — \`main...HEAD\` and \`--cached\` both miss the uncommitted edits self-review runs on) and your task brief. Instruct the subagent to flag bugs, logic errors, missing edge cases, gaps between request and diff, and unintended changes. Delegation + research discipline (distilled from \`/anneal\` canonical — these are codified learnings from many review rounds, not theoretical best practices): - Do NOT summarize what you implemented — that biases the subagent toward validating the shape of your solution rather than questioning it. diff --git a/test/crossagent/vertexClaude.ts b/test/crossagent/vertexClaude.ts index 00c285b..7a0221d 100644 --- a/test/crossagent/vertexClaude.ts +++ b/test/crossagent/vertexClaude.ts @@ -1,39 +1,11 @@ -import type { AgentResult, TestRunnerOptions, ValidationCheck } from "../utils.ts"; -import { defineFixture } from "../utils.ts"; - -const fixture = defineFixture( - { - prompt: `Call set_output with "VERTEX CLAUDE SMOKE PASSED".`, - }, - { localOnly: true } -); - -function validator(result: AgentResult): ValidationCheck[] { - const output = result.structuredOutput; - const setOutputCalled = output !== null; - const correctValue = setOutputCalled && /VERTEX CLAUDE SMOKE PASSED/i.test(output); - - return [ - { name: "set_output", passed: setOutputCalled }, - { name: "correct_value", passed: correctValue }, - ]; -} - -export const test: TestRunnerOptions = { - name: "vertex-claude", - agents: ["claude"], - fixture, - validator, - env: { - PULLFROG_DISABLE_SECURITY_INSTRUCTIONS: "1", - PULLFROG_MODEL: "vertex/byok", - VERTEX_MODEL_ID: "claude-opus-4-1@20250805", - VERTEX_LOCATION: "global", - }, - coverage: [ - "action/models.ts", - "action/main.ts", - "action/agents/claude.ts", - "action/utils/{agent,apiKeys,vertex}.ts", - ], -}; +/** + * vertex-claude crossagent smoke — disabled. + * pullfrog GCP project has 0 quota for anthropic claude on vertex. + * re-enable after quota increase. + * + * previous test definition (for restore): + * name: "vertex-claude" + * agents: ["claude"] + * prompt: Call set_output with "VERTEX CLAUDE SMOKE PASSED". + * env: PULLFROG_MODEL=vertex/byok, VERTEX_MODEL_ID=claude-opus-4-1@20250805, VERTEX_LOCATION=global + */ diff --git a/toolState.ts b/toolState.ts index 73e137d..9219f0b 100644 --- a/toolState.ts +++ b/toolState.ts @@ -62,6 +62,19 @@ export interface ToolState { // push destination set by checkout_pr - used as primary source in push_branch // because git config reads can fail in certain environments pushDest?: StoredPushDest; + // HEAD identity captured by setupGit at run start. load-bearing for the + // checkout_pr initial-branch invariant: the only sanctioned HEAD positions + // when calling checkout_pr are the run-entry HEAD or the target `pr-N`. + // blocks the zed-style cross-PR clobber where a subagent left HEAD on + // someone else's `pr-X` and the orchestrator's next checkout_pr inherited + // that position. + // + // discriminated by `kind` because `git rev-parse --abbrev-ref HEAD` returns + // the literal sentinel string `"HEAD"` on detached entry, which is the + // default state from `actions/checkout` on `pull_request` events (it + // checks out the merge commit as a detached SHA). without the kind tag, + // detached-entry runs would trivially accept any future detached state. + initialHead?: { kind: "branch"; name: string } | { kind: "detached"; sha: string }; // issue or PR number (same number space in GitHub) issueNumber?: number; // PR HEAD sha at checkout time — used to detect new commits pushed during a review diff --git a/utils/setup.ts b/utils/setup.ts index 9599958..542d77b 100644 --- a/utils/setup.ts +++ b/utils/setup.ts @@ -221,5 +221,37 @@ export async function setupGit(params: SetupGitParams): Promise { // disable credential helpers to prevent prompts and ensure clean auth state $("git", ["config", "--local", "credential.helper", ""], { cwd: repoDir }); + // pin the run-entry HEAD for the checkout_pr initial-branch invariant; see + // captureInitialHead for the named-branch vs detached split and why it + // matters (zed-industries/cloud 2026-05-18 cross-PR clobber shape). + params.toolState.initialHead = captureInitialHead(repoDir); + log.info("» git authentication configured"); } + +/** + * snapshot the current HEAD as either a branch name (when on a named branch) + * or a literal SHA (when detached). used by setupGit to pin the run-entry + * position and by checkout_pr to compare the live HEAD against it. + * + * splitting the two cases is load-bearing: `git rev-parse --abbrev-ref HEAD` + * returns the sentinel string `"HEAD"` on detached entry — which is the + * default `actions/checkout` state for `pull_request` events. storing that + * raw string would make any future detached state (including a subagent's + * `git checkout --detach `) compare equal. + */ +export function captureInitialHead( + repoDir: string +): { kind: "branch"; name: string } | { kind: "detached"; sha: string } { + try { + const name = $("git", ["symbolic-ref", "--short", "HEAD"], { + cwd: repoDir, + log: false, + }).trim(); + if (name) return { kind: "branch", name }; + } catch { + // detached HEAD — fall through + } + const sha = $("git", ["rev-parse", "HEAD"], { cwd: repoDir, log: false }).trim(); + return { kind: "detached", sha }; +}