checkout_pr: refuse unconditionally on dirty working tree (#808)
* checkout_pr: refuse unconditionally on dirty working tree drop the live-HEAD comparison from the guard introduced in #796. any checkout_pr call with staged or unstaged changes now throws, even when HEAD is already on pr-N. no stashing, no idempotent escape hatch. motivation is the zed-industries/cloud (2026-05-18) incident: shared-cwd subagents make "carry edits along" semantics dangerous, and the HEAD-equality predicate let a re-checkout silently inherit working-tree state from a sibling agent. forcing commit/discard before any PR-context operation eliminates the entire carry-forward failure class. error names the PR number, lists dirty paths, and tells the agent to commit/push/restore/clean before retrying. * improve dirty-tree error: precise discard commands copilot caught two sloppy bits in the error string: - "push" alone does not clean a dirty tree (needs commit first) - bare `git clean` is a no-op without `-fd` reword to "commit (then push if needed), or discard with `git restore --staged --worktree .` / `git clean -fd`" so the guidance is actually actionable. * checkout_pr: initial-branch invariant setupGit captures `toolState.initialBranch` at run start via live `git rev-parse --abbrev-ref HEAD`. checkout_pr refuses unless current HEAD matches the run-entry branch or the target `pr-N` (idempotent same-PR re-checkout). uses live rev-parse, not toolState.issueNumber (poisonable per the PR #796 review). refusal error names the current branch, target PR, recovery path (`git checkout <initialBranch>` with the literal branch name), and explicitly states routing around via the `git` tool is not sanctioned. closes the zed-industries/cloud (2026-05-18) shape where a subagent parked HEAD on someone else's `pr-X` and the orchestrator's next checkout_pr inherited that position. * reviewfrog: enforce canonical diff + pre-commit halt; align Build dispatch extend REVIEWER_SYSTEM_PROMPT with two prepended HARD CONSTRAINTS: - first action MUST be `git diff origin/<base>` (single-rev, captures uncommitted). no other diff first; no checkout_pr; no alt-ref fetches; no branch listing; no `gh pr list`. - empty canonical diff + claimed-changes dispatch ⇒ reply exactly with `no changes detected — likely pre-commit Build self-review; orchestrator should commit then re-dispatch` and stop. do not guess PR numbers (the zed thrash that ended in `checkout_pr({2582})`). reshape Build mode reviewfrog dispatch step around a verbatim template that names: (a) the situation is pre-commit, (b) canonical diff command, (c) halt-on-empty-diff rule. orchestrator side now says the same thing as the reviewer's baked-in prompt. delegation-discipline bullets and orchestrator-evaluation guidance kept intact. * checkout_pr: handle detached-HEAD entry in initial-branch invariant pullfrog incremental review caught a defense-in-depth gap: `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. with the previous string-typed `initialBranch`, both the captured value and the live probe would equal `"HEAD"` on any detached state, trivially satisfying the invariant — including a subagent doing `git checkout --detach <sha>`. discriminate the captured HEAD: probe `git symbolic-ref --short HEAD` first (works on named branches), fall back to `git rev-parse HEAD` (SHA) on detached entry. store as `{ kind: "branch"; name } | { kind: "detached"; sha }`. checkout_pr runs the identical probe at call time and compares like-with-like (branch name vs branch name, SHA vs SHA). refusal error renders both heads via a small `describeHead` helper and chooses the right `git checkout` recovery target (branch name or SHA). no inline-discriminant `as` casts — uses a top-level `headsEqual` that narrows via the discriminator.
This commit is contained in:
Colin McDonnell
committed by
pullfrog[bot]
pullfrog[bot]
parent
d3b5340583
commit
01e4daa0b5
@@ -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:
|
||||
|
||||
@@ -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/<base>\` (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/<base>\` 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 ` +
|
||||
|
||||
+54
-10
@@ -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<number, Promise<CheckoutPrResult>>();
|
||||
|
||||
type InitialHead = NonNullable<ToolContext["toolState"]["initialHead"]>;
|
||||
|
||||
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<CheckoutPrResult> => {
|
||||
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}`) {
|
||||
// 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, push, or discard them before switching. dirty paths:\n${dirty}`
|
||||
`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} 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.`
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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/<base-branch>\` (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: <branch> (off <base>)
|
||||
Canonical diff command: git diff origin/<base>
|
||||
|
||||
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
|
||||
<YOUR TASK content>
|
||||
|
||||
## Build-phase failures
|
||||
<tight summary — what broke, root cause, the fix — or "no build-phase failures">
|
||||
\`\`\`
|
||||
|
||||
Follow the template with the diff content (\`git diff origin/<base-branch>\`, 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.
|
||||
|
||||
@@ -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
|
||||
*/
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -221,5 +221,37 @@ export async function setupGit(params: SetupGitParams): Promise<void> {
|
||||
// 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 <sha>`) 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 };
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user