From a0afbc88c9858b319ae5be810839cb6263b26039 Mon Sep 17 00:00:00 2001 From: d3cker Date: Thu, 17 Sep 2026 14:05:41 +0200 Subject: [PATCH] Retry delayed PR head updates after checkpointed pushes --- CHANGELOG.md | 5 ++++ docs/advanced.md | 19 +++++++++++---- docs/architecture.md | 2 ++ docs/bot-workflow.md | 48 +++++++++++++++++++++++++++---------- docs/runtime.md | 8 +++++++ src/dispatcher.ts | 13 ++++++---- src/github.ts | 15 +++++++++--- test/core.test.ts | 35 ++++++++++++++++++++++++++- test/pr-description.test.ts | 40 ++++++++++++++++++++++++++----- 9 files changed, 155 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6960db9..ab37b22 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,11 @@ include the full version, for example `## 0.7.0-beta.1`. ### Fixed +- Retry PR head propagation after a successful push when the remote branch still + matches the verified commit. Preserve a durable push checkpoint across restarts, + avoid repeating acknowledged pushes, and distinguish closed PRs from changed + branches while preserving manual description edits. + - Keep repository inventory RPC responses valid JSON when an owner has no runtime snapshots. Isolate setup-test registries so validation never adds fixture repositories to the operator's inventory. diff --git a/docs/advanced.md b/docs/advanced.md index 60b9ef5..67b443a 100644 --- a/docs/advanced.md +++ b/docs/advanced.md @@ -239,8 +239,10 @@ The monitor reports task maintenance until closure completes. Task state stores `completion` (final public text or an explicit unavailable reason, session, round, then verified commit and checks), `initialCompletion`, -and `publishedBody` (the last acknowledged managed section). New rounds clear -only the current completion. Older verifying/publishing tasks recover missing +`publishedBody` (the last acknowledged managed section), and `pushedCommit` (the +successfully pushed verified SHA). New rounds clear the current completion and +push checkpoint while retaining the original report and published body. +Older verifying/publishing tasks recover missing summaries from their saved sessions; this does not rerun the model. Missing or empty successful reports are marked unavailable, while transient reads retry. Already completed or closed tasks are not bulk rewritten on upgrade. @@ -251,13 +253,22 @@ replaced; unknown unmarked text is retained with the new section appended, since it might contain manual edits. A later legacy round may not have enough saved information to identify its old acknowledgement exactly. -An edited/removed managed section, changed PR head, closed follow-up PR, or oversized +After push, GitHub's PR head can temporarily lag behind its branch ref. Before +description reconciliation and again before PATCH, the plugin reads the remote +branch ref. If it matches the verified SHA but the PR head does not, publication +enters `retry_wait` and retries with the normal backoff, up to `maxAttempts`. +Retries reuse the saved report and verified commit, skipping a push already +recorded in `pushedCommit`, including after a restart. Exhausted retries become +`failed`; inspect the reported SHAs and use `/restartworkflow` after resolving the +problem. An unacknowledged push still uses normal non-force push reconciliation. + +An edited/removed managed section, changed remote branch, closed follow-up PR, or oversized body blocks at `publishing`. Preserve your notes outside the markers and restore the previous managed section from `publishedBody` in the task checkpoint (or PR edit history), then use the normal workflow retry. Do not delete the queue or restart implementation just to retry a description update. If a PATCH succeeded but its response was lost, matching desired content is accepted without another -write. Body and head are reread before PATCH; edits after that final read cannot +write. Body, PR head and branch ref are reread before PATCH; edits after those reads cannot be atomically excluded by this implementation. Each rendered report is limited to 22,000 UTF-8 bytes with an explicit truncation diff --git a/docs/architecture.md b/docs/architecture.md index a3346e9..a2356e4 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -35,6 +35,8 @@ loads a generic scheduler, a GitHub dispatcher, and a terminal UI component. changes, bind the report to the verified commit, then push and create or reconcile the PR with that report and separate dispatcher checks. Generate its title only if creating a PR without an already-saved title. + Checkpoint successful pushes. Retry a lagging PR head only while the remote + branch matches the verified commit; block actual branch changes or closure. 6. After publication, process queued authorized issue comments as new rounds on the same worktree and branch, with a new main session and the existing open PR. After cancellation, use a fresh local branch/worktree from the published head, diff --git a/docs/bot-workflow.md b/docs/bot-workflow.md index 9de4885..b9918d5 100644 --- a/docs/bot-workflow.md +++ b/docs/bot-workflow.md @@ -201,6 +201,8 @@ flowchart TD V -->|Failed check or Git consistency guard| VB[verifying / blocked] VB -->|Operator retries saved stage| V P --> Done[pr_opened / done with PR and publishedAt] + P -->|Branch matches, PR head not yet updated| Lag[publishing / retry_wait within attempt limit] + Lag -->|Backoff elapsed, reuse saved push| P P -->|Publication failure| PB[Retain publishing phase and apply error policy] PB -->|Eligible retry| P Done -->|Pending authorized feedback| Round[Increment round, move feedback and reset per-round state] @@ -449,7 +451,9 @@ flowchart TD Find --> Follow{Follow-up round?} Follow -->|Yes| Open{Existing PR open?} Open -->|No| Block - Open -->|Yes| Push[Validate origin, workspace, saved HEAD and clean tree, push exact SHA] + Open -->|Yes| Pushed{Saved pushedCommit matches verified SHA?} + Pushed -->|Yes| Description + Pushed -->|No| Push[Validate origin, workspace, saved HEAD and clean tree, push exact SHA] Follow -->|No| Exists{PR already exists?} Exists -->|Yes| Closed{PR closed?} Closed -->|Yes| Done[Record PR and publication time, pr_opened / done] @@ -457,11 +461,17 @@ flowchart TD Exists -->|No| Title[Generate title only if no saved prTitle] Title --> Issue{Issue still open?} Issue -->|No| Block - Issue -->|Yes| PushNew[Validate origin and workspace, push exact verified SHA] - PushNew --> Create[Create or reconcile signed PR against pinned base] - Create --> Description[Read open PR at verified SHA, reconcile managed description] - Push --> Description - Description -->|Edited managed block, changed head or oversized body| Block + Issue -->|Yes| NewPushed{Saved pushedCommit matches verified SHA?} + NewPushed -->|Yes| Create + NewPushed -->|No| PushNew[Validate origin and workspace, push exact verified SHA] + PushNew --> SaveNew[Persist pushedCommit] + SaveNew --> Create[Create or reconcile signed PR against pinned base] + Create --> Description[Read open PR and remote branch, reconcile managed description] + Push --> SavePush[Persist pushedCommit] + SavePush --> Description + Description -->|Branch matches, PR head differs| Pending[Retry saved publication with backoff and attempt limit] + Pending --> Retry + Description -->|Closed PR, changed branch, edited managed block or oversized body| Block Description -->|Unchanged or update succeeds| Acknowledge[Persist published body checkpoint] Acknowledge --> Done Failure[Other command, model or transport error] --> Policy[Keep current phase and apply retry policy in section 8] @@ -473,9 +483,11 @@ flowchart TD ``` Resuming `running` validates the saved session first; retrying `verifying` runs -checks again. Retrying `publishing` uses the saved verified SHA and requires the -worktree still to match it, rather than rerunning checks implicitly. An already -pushed branch does not by itself make a task complete. +checks again. Retrying `publishing` uses the saved verified SHA without rerunning +checks implicitly. If a push is still required, the worktree must still match +that SHA. After a checkpointed push, description reconciliation checks GitHub's +branch and PR instead; it does not publish later local edits. An already pushed +branch does not by itself make a task complete. The configured checks are command argument arrays. A failing configured check produces `blocked`. With no configured checks, only Git consistency checks run; @@ -498,9 +510,21 @@ follow-ups. Dispatcher checks appear separately from agent-reported tests, follo by `Closes #N`, session, round and verified commit. Push uses `COMMIT:refs/heads/TASK_BRANCH` without force. The first publication reconciles an existing branch PR without another push; follow-ups require an open PR and push -the new verified commit before updating its description. The title is retained. +the new verified commit before updating its description. A successful push saves +`pushedCommit`; retries skip that push when it matches the verified SHA, including +after an owner restart. New rounds clear this checkpoint. If push succeeded but +its response or checkpoint was lost, normal non-force push reconciliation still +applies. The title is retained. An already closed first-round PR is recorded without editing its description. +Before reconciling the description and again before PATCH, compare the remote +branch ref with the verified SHA. If the branch matches but the PR head is stale, +`PullHeadPending` uses the normal backoff and `maxAttempts` policy at `publishing`. +It does not rerun implementation, verification, or a checkpointed push. A closed +PR or different branch SHA blocks with a distinct error; a stale PR view is not +permission to overwrite a changed branch. The guard also checks the branch when +the PR view already reports the expected SHA. + A stable HTML marker pair encloses the bot-managed description. Notes outside it are preserved; an edited or removed managed section blocks publication rather than overwriting it. The last acknowledged body is checkpointed, so a lost update @@ -666,9 +690,9 @@ flowchart TD Result -->|WaitingForAnswer| Wait[waiting, or ready if answer already arrived] Result -->|SessionStopped| Stop[running / blocked, sessionStopped true] Result -->|Other Blocked, description conflict or GitHub 401, 404, 422| Block[blocked at saved phase] - Result -->|Other failure below attempt limit| Retry[retry_wait at saved phase] + Result -->|PR head propagation or other failure below attempt limit| Retry[retry_wait at saved phase] Retry -->|nextAt elapsed| Work - Result -->|Other failure at limit| Fail[failed at saved phase] + Result -->|PR head propagation or other failure at limit| Fail[failed at saved phase] Stop --> Probe[On available worker pass, probe due saved session without unresolved question] Legacy[Recognized legacy timeout or outcome block] --> Probe Probe --> Complete{Matching location and task marker, succeeded outcome and valid final assistant?} diff --git a/docs/runtime.md b/docs/runtime.md index 75dd565..fe68166 100644 --- a/docs/runtime.md +++ b/docs/runtime.md @@ -431,6 +431,14 @@ durable as before. ## PR descriptions +After pushing a verified commit, GitHub may briefly show the previous commit in +the PR. If the remote branch already matches the verified commit, the bot waits +and automatically retries publication within its configured attempt limit. It +keeps the saved report and does not repeat implementation or a checkpointed push. +A changed remote branch or closed PR produces a separate blocking error. If the +propagation retries are exhausted, inspect the error and use `/restartworkflow`; +see [description recovery](advanced.md#pr-description-recovery). + The PR contains the agent's final completion summary from the successful session, with Markdown preserved, followed by dispatcher verification, the issue reference, session, round and verified commit. The initial issue acknowledgement is not a diff --git a/src/dispatcher.ts b/src/dispatcher.ts index b4f8414..11726fb 100644 --- a/src/dispatcher.ts +++ b/src/dispatcher.ts @@ -44,7 +44,7 @@ export const Task = z.object({ source: z.enum(["issue", "comment"]).optional(), feedback: z.array(Comment).optional(), pendingFeedback: z.array(Comment).optional(), commentCursor: z.number().optional(), previousSessionID: z.string().optional(), - checks: z.array(z.string()).optional(), commit: z.string().optional(), + checks: z.array(z.string()).optional(), commit: z.string().optional(), pushedCommit: z.string().optional(), publishedAt: z.number().optional(), merged: z.boolean().optional(), mergeError: z.string().optional(), mergeNextAt: z.number().optional(), completion: CompletionSummary.optional(), initialCompletion: CompletionSummary.optional(), publishedBody: z.string().optional(), @@ -268,7 +268,7 @@ export class Dispatcher { } : {}), round: (finished.round ?? 1) + 1, feedback: finished.pendingFeedback, pendingFeedback: [], previousSessionID: finished.sessionID, phase: "queued", status: "ready", attempts: 0, nextAt: this.now(), analysis: undefined, commentID: undefined, analysisDecision: undefined, analysisDialogue: undefined, question: undefined, - sessionID: undefined, sessionReady: false, promptAttempted: false, sessionStopped: undefined, recovery: undefined, completion: undefined, checks: undefined, commit: undefined, error: undefined }); + sessionID: undefined, sessionReady: false, promptAttempted: false, sessionStopped: undefined, recovery: undefined, completion: undefined, checks: undefined, commit: undefined, pushedCommit: undefined, error: undefined }); await this.store.save(this.queue); }); const resumable = this.queue.tasks.filter(t => ["ready", "retry_wait"].includes(t.status)); @@ -355,11 +355,16 @@ export class Dispatcher { const body = renderDescription(task.key, task.issue.number, task.initialCompletion!, task.completion!); let pr = await this.github.findPull(task.repo, task.branch); if (followup && (!pr || pr.state !== "open")) throw new Blocked("The original PR is no longer open; changes remain in the worktree"); - if (followup && pr) await this.executor.push(task, repo); + const push = async (repository: Repository) => { + if (task.pushedCommit === task.commit) return; + await this.executor.push(task, repository); + await this.update(task, { pushedCommit: task.commit }); + }; + if (followup && pr) await push(repo); if (!pr) { if (!task.prTitle) await this.update(task, { prTitle: await this.executor.title(task) }); if ((await this.github.issue(task.repo, task.issue.number)).state !== "open") throw new Blocked("Issue closed before PR publication"); - await this.executor.push(task, repo); + await push(repo); pr = await this.github.ensurePull(task.repo, task.branch, repo.baseBranch, task.prTitle!, body); } if (pr.state === "open") { diff --git a/src/github.ts b/src/github.ts index 0e16bc4..6e9a489 100644 --- a/src/github.ts +++ b/src/github.ts @@ -16,6 +16,7 @@ export type Pull = z.infer; export class GithubError extends Error { constructor(readonly status: number, readonly retryAt?: number) { super(`GitHub HTTP ${status}`); } } +export class PullHeadPending extends Error {} export class Github { private login?: string; constructor(private token: string, private signal: AbortSignal, private fetcher: typeof fetch = fetch, private signature?: string) {} @@ -71,10 +72,17 @@ export class Github { return Pull.parse(await this.request(`/repos/${repo}/pulls`, "POST", { head: branch, base, title, body: signed })); } async updatePullBody(repo: string, number: number, commit: string, key: string, body: string, previous?: string, legacy?: string) { - const schema = z.object({ state: z.string(), head: z.object({ sha: z.string() }), body: z.string().nullable() }); + const schema = z.object({ state: z.string(), head: z.object({ sha: z.string(), ref: z.string() }), body: z.string().nullable() }); const read = async () => schema.parse(await this.request(`/repos/${repo}/pulls/${number}`)); + const requireHead = async (pr: z.infer) => { + if (pr.state !== "open") throw new DescriptionConflict(`PR #${number} is closed. Inspect it before retrying publication.`); + const ref = z.object({ object: z.object({ sha: z.string() }) }).parse( + await this.request(`/repos/${repo}/git/ref/heads/${encodeURIComponent(pr.head.ref)}`)); + if (ref.object.sha !== commit) throw new DescriptionConflict(`PR #${number} branch changed: expected ${commit}, found ${ref.object.sha}. Inspect it before retrying publication.`); + if (pr.head.sha !== commit) throw new PullHeadPending(`Waiting for GitHub PR #${number} to reflect pushed commit ${commit}; its branch matches, but the PR reports ${pr.head.sha}. Publication will retry automatically within the configured attempt limit.`); + }; const current = await read(); - if (current.state !== "open" || current.head.sha !== commit) throw new DescriptionConflict("PR is closed or its head differs from the verified commit. Inspect it before retrying publication."); + await requireHead(current); const signedLegacy = legacy === undefined ? undefined : await this.signed(legacy); let next = mergeDescription(current.body ?? "", key, body, previous, signedLegacy); // Sign a new body or an exact legacy replacement; retain existing signatures elsewhere. @@ -82,7 +90,8 @@ export class Github { assertDescriptionSize(next); if (next === current.body) return; const fresh = await read(); - if (fresh.state !== "open" || fresh.head.sha !== commit || fresh.body !== current.body) throw new DescriptionConflict("PR changed while its description was being prepared. Retry after inspecting concurrent edits."); + await requireHead(fresh); + if (fresh.body !== current.body) throw new DescriptionConflict("PR description changed while its update was being prepared. Retry after inspecting concurrent edits."); await this.request(`/repos/${repo}/pulls/${number}`, "PATCH", { body: next }); } async mergeApproved(repo: string, number: number, commit: string, since: number, authors: string[], options: GithubOptions["autoMerge"]): Promise { diff --git a/test/core.test.ts b/test/core.test.ts index e2a572d..a3ee5fa 100644 --- a/test/core.test.ts +++ b/test/core.test.ts @@ -8,7 +8,7 @@ import { GithubOptions, Job, matchRoute } from "../src/config.js"; import { Scheduler } from "../src/scheduler.js"; import { Dispatcher, Blocked, Queue, type Executor, type GithubPort } from "../src/dispatcher.js"; import { JsonStore, acquire, type Store } from "../src/state.js"; -import { Github, GithubError, type Issue, type Comment } from "../src/github.js"; +import { Github, GithubError, PullHeadPending, type Issue, type Comment } from "../src/github.js"; import type { Plugin } from "@opencode/plugin"; import { OpenCodeExecutor } from "../src/executor.js"; @@ -909,6 +909,39 @@ test("completion is persisted before verification, bound to its commit and reuse assert.equal(f.events.filter(e => e === "run").length, 1); }); +test("publication retries a lagging PR after owner restart without rerunning work or pushing again", async () => { + const f = fixture(), d = f.make(); await d.init(); await d.scan(); await d.tick(); + f.github.findPull = async () => ({ number: 2, html_url: "https://github.com/owner/repo/pull/2", state: "open" }); + f.github.comments = async () => [{ id: 43, body: "Fix the tests", user: { login: "alice" } }]; + let updates = 0; + f.github.updatePullBody = async () => { if (++updates === 1) throw new PullHeadPending("GitHub PR still reports the previous commit"); }; + f.executor.verify = async () => { f.events.push("verify"); return { checks: ["passed"], commit: "round-two" }; }; + await d.scan(); await d.tick(); + const saved = d.status()[0]!; + assert.equal(saved.status, "retry_wait"); assert.equal(saved.phase, "publishing"); + assert.equal(saved.pushedCommit, "round-two"); assert.equal(saved.completion?.commit, "round-two"); + assert.equal(saved.publishedHead?.commit, "sha"); + const events = [...f.events]; + // Exercise the durable schema, not just the in-memory task object. + f.store.data = Queue.parse(JSON.parse(JSON.stringify(f.store.data))); + f.advance(); const restarted = f.make(); await restarted.init(); await restarted.tick(); + assert.equal(restarted.status()[0]?.status, "done"); assert.equal(restarted.status()[0]?.publishedHead?.commit, "round-two"); + assert.equal(updates, 2); assert.deepEqual(f.events, events); +}); + +test("PR propagation retries stop at the configured attempt limit while retaining the pushed commit", async () => { + const f = fixture(), d = f.make(); + f.github.updatePullBody = async () => { throw new PullHeadPending("GitHub PR has not caught up"); }; + await d.init(); await d.scan(); await d.tick(); + f.github.findPull = async () => ({ number: 2, html_url: "https://github.com/owner/repo/pull/2", state: "open" }); + for (let attempt = 1; attempt < options.maxAttempts; attempt++) { f.advance(); await d.tick(); } + const saved = d.status()[0]!; + assert.equal(saved.status, "failed"); assert.equal(saved.phase, "publishing"); + assert.equal(saved.attempts, options.maxAttempts); assert.equal(saved.pushedCommit, "sha"); + assert.equal(f.events.filter(e => e === "push").length, 1); + assert.equal(f.events.filter(e => e === "run").length, 1); +}); + test("follow-up publication keeps the initial report and updates the same PR from the new session only after push", async () => { const f = fixture(); const bodies: string[] = []; f.executor.run = async (t, checkpoint) => { await checkpoint({ sessionID: `ses_round_${t.round ?? 1}` }); }; diff --git a/test/pr-description.test.ts b/test/pr-description.test.ts index 623645e..3f44ae8 100644 --- a/test/pr-description.test.ts +++ b/test/pr-description.test.ts @@ -1,7 +1,7 @@ import test from "node:test"; import assert from "node:assert/strict"; import { finalReport, renderDescription, mergeDescription, descriptionMarkers, assertDescriptionSize, DescriptionConflict } from "../src/pr-description.js"; -import { Github } from "../src/github.js"; +import { Github, PullHeadPending } from "../src/github.js"; const key = "owner/repo#1"; const first = { sessionID: "ses_first", round: 1, text: "## Summary\n\nImplemented **controls**.\n\n24/24 tests passed.", commit: "one", checks: ["npm test — passed"] }; @@ -59,21 +59,25 @@ test("long reports are bounded and marked as truncated without exposing manageme }); function githubFixture() { - let body = `Notes above\n${original}\nNotes below`, sha = "two", state = "open", lost = false, concurrent = false, reads = 0; + let body = `Notes above\n${original}\nNotes below`, sha = "two", branch = "two", state = "open", lost = false, concurrent = false, reads = 0; + let onSecondRead = () => {}; const patches: Record[] = []; const fetcher: typeof fetch = async (url, init) => { if (String(url).endsWith("/user")) return Response.json({ login: "bot" }); + if (String(url).endsWith("/git/ref/heads/automation%2Fissue-1")) return Response.json({ object: { sha: branch } }); if (init?.method === "PATCH") { const patch = JSON.parse(String(init.body)); patches.push(patch); body = patch.body; if (lost) { lost = false; throw new Error("lost response after PATCH"); } return Response.json({}); } reads++; + if (reads === 2) onSecondRead(); if (concurrent && reads === 2) body += "\nConcurrent note"; - return Response.json({ body, state, head: { sha } }); + return Response.json({ body, state, head: { sha, ref: "automation/issue-1" } }); }; return { github: new Github("token", new AbortController().signal, fetcher), patches, body: () => body, - lose: () => { lost = true; }, race: () => { concurrent = true; }, head: (value: string) => { sha = value; }, close: () => { state = "closed"; } }; + lose: () => { lost = true; }, race: () => { concurrent = true; }, head: (value: string) => { sha = value; }, + branch: (value: string) => { branch = value; }, secondRead: (fn: () => void) => { onSecondRead = fn; }, close: () => { state = "closed"; } }; } test("GitHub description reconciliation retries a lost PATCH response without changing title or manual notes", async () => { @@ -84,11 +88,35 @@ test("GitHub description reconciliation retries a lost PATCH response without ch assert.equal(f.body(), `Notes above\n${updated}\nNotes below`); }); -test("GitHub refuses stale heads, closed PRs and a detected concurrent description edit", async () => { +test("GitHub refuses changed branches, closed PRs and a detected concurrent description edit", async () => { for (const scenario of ["head", "closed", "race"]) { const f = githubFixture(); - if (scenario === "head") f.head("unexpected"); else if (scenario === "closed") f.close(); else f.race(); + if (scenario === "head") f.branch("unexpected"); else if (scenario === "closed") f.close(); else f.race(); await assert.rejects(f.github.updatePullBody("owner/repo", 2, "two", key, updated, original), DescriptionConflict); assert.equal(f.patches.length, 0); } }); + +test("a lagging PR head retries only while the remote branch matches the verified commit", async () => { + const f = githubFixture(); f.head("one"); + await assert.rejects(f.github.updatePullBody("owner/repo", 2, "two", key, updated, original), error => { + assert.ok(error instanceof PullHeadPending); + assert.match(error.message, /reflect pushed commit two/); assert.match(error.message, /reports one/); + return true; + }); + assert.equal(f.patches.length, 0); + f.head("two"); + await f.github.updatePullBody("owner/repo", 2, "two", key, updated, original); + assert.equal(f.patches.length, 1); assert.equal(f.body(), `Notes above\n${updated}\nNotes below`); +}); + +test("both PR and branch checks run again before PATCH without confusing real conflicts with propagation", async () => { + for (const scenario of ["lag", "branch", "closed"]) { + const f = githubFixture(); + f.secondRead(() => { + if (scenario === "lag") f.head("one"); else if (scenario === "branch") f.branch("someone-else"); else f.close(); + }); + await assert.rejects(f.github.updatePullBody("owner/repo", 2, "two", key, updated, original), scenario === "lag" ? PullHeadPending : DescriptionConflict); + assert.equal(f.patches.length, 0); + } +});