Skip to content

Commit 1a27107

Browse files
itsderek23claude
andcommitted
Fetch missing base SHA; ground summary on counts; preflight skip-install
Three fixes found while running releasebot against PR paperclipai#4432: - ensureShaReachable now falls back to fetching the SHA directly from origin when it's not in pull/<n>/head. This covers the case where origin/master has advanced past the PR base since the last local fetch. - Summary prompt now receives a structured Facts block with exact per-side pass/fail counts and visual-review verdict counts, with an explicit instruction not to invert them. Fixes a hallucination where the summary claimed "Playwright failed on the after side" when the after side had zero failures. - --skip-install now preflights cli/node_modules/tsx/dist/cli.mjs in both worktrees and exits early with a clear message, instead of failing mid-boot with MODULE_NOT_FOUND. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 1501f99 commit 1a27107

3 files changed

Lines changed: 38 additions & 4 deletions

File tree

tools/releasebot/src/cli.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,18 @@ async function main(): Promise<void> {
8484
await pnpmInstall(afterWt);
8585
} else {
8686
log("skipping pnpm install (--skip-install)");
87+
for (const wt of [beforeWt, afterWt]) {
88+
const tsxCli = path.join(wt, "cli/node_modules/tsx/dist/cli.mjs");
89+
try {
90+
await fs.stat(tsxCli);
91+
} catch {
92+
console.error(
93+
`--skip-install passed but ${wt} is missing node_modules (checked ${path.relative(repoRoot, tsxCli)}).`,
94+
);
95+
console.error("Re-run without --skip-install to install dependencies in the fresh worktree.");
96+
process.exit(2);
97+
}
98+
}
8799
}
88100

89101
log("gathering source context around diff hunks...");

tools/releasebot/src/pr.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,17 @@ export async function fetchPrDiff(prNumber: number): Promise<string> {
4040
export async function ensureShaReachable(sha: string, prNumber: number): Promise<void> {
4141
try {
4242
await execFile("git", ["cat-file", "-e", sha]);
43+
return;
4344
} catch {
45+
// not local yet
46+
}
47+
try {
4448
await execFile("git", ["fetch", "origin", `pull/${prNumber}/head`]);
49+
await execFile("git", ["cat-file", "-e", sha]);
50+
return;
51+
} catch {
52+
// pull/<n>/head covers the head commit and its ancestors, but not a base SHA
53+
// on origin/<baseBranch> that has moved since the last fetch. Fall through.
4554
}
55+
await execFile("git", ["fetch", "origin", sha]);
4656
}

tools/releasebot/src/review.ts

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@ const SUMMARY_SYSTEM = `Write a <=400-character single-paragraph run summary for
2525
2626
The visual review (per-step verdicts: pass/intentional_change/fail) is the source of truth for whether this PR ships cleanly. Anchor the summary on those verdicts.
2727
28-
The Playwright assertion layer is a secondary signal. After-side assertion failures are worth mentioning only if the visual review also flagged the step as fail. Before-side assertion failures are EXPECTED when the test asserts on copy the PR introduced — do not flag them as regressions.
28+
The Playwright assertion layer is a secondary signal. The user message gives you exact pass/fail counts for each side as "Facts". Treat those numbers as ground truth — do NOT invert, embellish, or restate them inaccurately. If "after_fails" is 0, the after side did not fail. Before-side assertion failures are EXPECTED when the test asserts on copy the PR introduced — do not flag them as regressions.
29+
30+
After-side assertion failures are worth mentioning only if the visual review also flagged the step as fail.
2931
3032
Start with the headline outcome (how many steps passed / were intentional_change / failed per the visual review), then what the visual review actually observed, then any caveat worth a reviewer's attention. Natural user-facing language — no code identifiers, no bulleted lists. End on a complete sentence. Return plain text only.`;
3133

@@ -145,13 +147,23 @@ async function summarize(
145147
before: SideResult,
146148
after: SideResult,
147149
): Promise<string> {
150+
const beforeFails = before.steps.filter((s) => s.status === "fail").length;
148151
const afterFails = after.steps.filter((s) => s.status === "fail").length;
152+
const visualCounts = {
153+
pass: steps.filter((s) => s.verdict === "pass").length,
154+
intentional_change: steps.filter((s) => s.verdict === "intentional_change").length,
155+
fail: steps.filter((s) => s.verdict === "fail").length,
156+
};
149157
const content = [
150158
`PR #${pr.number}: ${pr.title}`,
151159
`Body: ${pr.body.slice(0, 600)}`,
152-
`Step-level visual review (source of truth): ${JSON.stringify(steps)}`,
153-
`Playwright assertion failures on after side (secondary signal): ${afterFails}/${after.steps.length}`,
154-
].join("\n\n");
160+
"Facts (ground truth — quote these numbers, do not invert):",
161+
` total_steps: ${steps.length}`,
162+
` visual_review: ${visualCounts.pass} pass, ${visualCounts.intentional_change} intentional_change, ${visualCounts.fail} fail`,
163+
` before_fails: ${beforeFails}/${before.steps.length} (expected — PR introduces new copy/routes)`,
164+
` after_fails: ${afterFails}/${after.steps.length}`,
165+
`Per-step visual review detail: ${JSON.stringify(steps)}`,
166+
].join("\n");
155167
const resp = await client.messages.create({
156168
model: MODEL,
157169
max_tokens: 400,

0 commit comments

Comments
 (0)