Skip to content

Commit 8a4e5ea

Browse files
mchwangclaude
andcommitted
Address Copilot round 9 on F2d: one deadline per PR operation, adopt on sight
- Each PR operation (open, lookup, refresh, draft change) runs under one deadline for all of its commands and poll waits (60 s by default), combined with the caller's signal, instead of 30 s per command. - An abandoned opening's PR is adopted as soon as publish sees it, under the state-version guard and without changing the task status, so a non-clear check no longer leaves the PR unrecorded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent f094853 commit 8a4e5ea

5 files changed

Lines changed: 85 additions & 6 deletions

File tree

‎docs/implementation/pull-request-opening.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ Only one publish runs per task at a time; a second one is refused. A publish who
5252

5353
1. **Recover.** If an opening is still `opening`, look for the open PR from the task branch, with the markers of all the task's openings for that branch. If it has this opening's marker, record it as opened; if this was a draft opening, this publish is a draft publish too, and the PR has since been made ready, turn it back into a draft first (a failure keeps the opening owned). The recovered opening ends this publish only when it is this publish's own work: same task state version and same draft mode. Otherwise the PR is recorded and publishing continues, so the main path pushes the current head and brings the PR into the current mode. If it has another opening's marker, this opening created nothing (one open PR per branch): mark it `abandoned` and continue. If none exists, the request may still be in flight or not yet visible. So publish stops with `OpeningUnsettled` until the opening is 10 minutes old (`settleMs`). After that, it marks the opening `abandoned` and continues.
5454
2. **Status and no changes.** Refuse unless the task is running (or in needs human, for a draft). If the task head is its base, open nothing. A running task moves to needs human.
55-
3. **Find the branch PR.** Ask GitHub for the branch's open PR every time, with the markers of the task's opened and abandoned openings for that branch (none if the task has no PR yet). An open PR that carries none of them was not opened by codeboost: publish refuses before anything is pushed. A PR with an opened record's marker but another number is refused too. When the PR's draft state on GitHub differs from the record, the record is corrected here (for example after a draft change whose record was lost), under the task's state-version guard. This comes before the check: an abandoned opening's PR links the issue and has no recorded number, so only its marker shows it is the task's own, and its number is added to the own PRs for the check.
55+
3. **Find the branch PR.** Ask GitHub for the branch's open PR every time, with the markers of the task's opened and abandoned openings for that branch (none if the task has no PR yet). An open PR that carries none of them was not opened by codeboost: publish refuses before anything is pushed. A PR with an opened record's marker but another number is refused too. An abandoned opening whose PR is now visible is adopted here (number, URL and draft state recorded, task status unchanged), whatever the check then says, so the record names every PR the task has on GitHub. When the PR's draft state on GitHub differs from the record, the record is corrected here (for example after a draft change whose record was lost), under the task's state-version guard. This comes before the check: an abandoned opening's PR links the issue and has no recorded number, so only its marker shows it is the task's own, and its number is added to the own PRs for the check.
5656
4. **Check.** Run the check. Record the result, and any status change, in one transaction. That transaction refuses if the task changed during the check.
5757
5. **Push.** When the task already has an open PR, the refresh is recorded first (the push moves that PR's head), then the push runs; otherwise the push runs straight after the check's transaction with no await in between. Push the task head to `codeboost/issue-<issue>-<task slug>-<hash>`. The slug is readable but can collide. The hash is 16 hex characters of SHA-256 over the exact task identity, so two tasks never share a branch.
5858
6. **Re-read.** Right before the GitHub call, the Store confirms that the latest check is clear and that nothing changed since that check: same task state version, same review version (approvals, choices and notes), same snapshot, same head.
@@ -73,6 +73,8 @@ When the check matches (or is unknown) and the task's earlier PR is open and rea
7373

7474
The adapter refuses any answer for a PR that is not open. A PR closed between the lookup and the update is never recorded as the task's review PR.
7575

76+
**Deadlines.** Each PR operation (open, lookup, refresh, draft change) has one deadline for all of its commands and poll waits together: 60 seconds by default (`operationMs`), combined with the caller's signal.
77+
7678
**Transport.** The PR title and description go to `gh api --input -` as a JSON body on stdin (`github/run-with-input.ts`), never as arguments: Linux limits one argument to 128 KiB, and a 60,000-character description of multibyte text is larger. The runner settles only after `gh` has exited, including on a timeout or abort: it sends SIGTERM, then SIGKILL after 5 seconds if `gh` is still running, and if a process `gh` started keeps the output pipes open after `gh` exits, it closes them after 1 second. The already-fixed check uses the same runner, so its 12-second deadline always holds. The already-fixed check also waits for both of its reads to settle before it returns.
7779

7880
**Environment.** Each `gh` process gets only an allowlist of variables: the path, home and locale; GitHub tokens, host and configuration directories; the D-Bus session bus that Linux keyring sign-in uses; and proxy and CA settings (`github/gh-env.ts`). Prompts, the pager and update checks are turned off.

‎github/pull-requests.ts‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ async function draftCall<T>(call: () => Promise<T>): Promise<T> {
4747
* agent-controlled, so a marker-shaped string there never identifies a PR.
4848
*/
4949
export const markerOf = (body: string): string => body.split('\n', 1)[0]!.trim();
50+
/** The longest one open, lookup, refresh or draft change may take in total, whatever the caller's signal. */
51+
export const PR_OPERATION_DEADLINE_MS = 60_000;
5052
/** GitHub updates a PR's head a moment after a push; the read-back waits up to this many polls for the pushed head. */
5153
export const HEAD_POLLS = 5, HEAD_POLL_MS = 500;
5254

@@ -58,12 +60,22 @@ export const HEAD_POLLS = 5, HEAD_POLL_MS = 500;
5860
export class GhPullRequestGateway implements PullRequestGateway {
5961
readonly repository: string;
6062
readonly run: RunGhWithInput;
61-
constructor(config: { repository: string }, run?: RunGhWithInput) {
63+
/** One deadline for a whole operation (every command and poll wait in it), not a fresh allowance per command. */
64+
readonly operationMs: number;
65+
constructor(config: { repository: string; operationMs?: number }, run?: RunGhWithInput) {
6266
if (!REPOSITORY.test(config.repository)) throw new Error('A GitHub repository is required to open pull requests.');
67+
if (config.operationMs !== undefined && (!Number.isSafeInteger(config.operationMs) || config.operationMs < 1)) throw new Error('Invalid operation deadline.');
6368
this.repository = config.repository;
69+
this.operationMs = config.operationMs ?? PR_OPERATION_DEADLINE_MS;
6470
this.run = run ?? ((args, options) => runWithInput('gh', args, { input: options?.input, timeout: 30_000, maxBuffer: 8 * 1024 * 1024, signal: options?.signal, env: ghEnvironment() }));
6571
}
6672

73+
/** The caller's signal combined with this operation's single deadline; every command and wait in it uses the result. */
74+
#bounded(signal?: AbortSignal): AbortSignal {
75+
const deadline = AbortSignal.timeout(this.operationMs);
76+
return signal ? AbortSignal.any([signal, deadline]) : deadline;
77+
}
78+
6779
async #json(args: readonly string[], signal?: AbortSignal, body?: Record<string, unknown>): Promise<unknown> {
6880
const output = await this.run(body ? [...args, '--input', '-'] : args, { signal, ...(body ? { input: JSON.stringify(body) } : {}) });
6981
try { return JSON.parse(output); }
@@ -91,6 +103,7 @@ export class GhPullRequestGateway implements PullRequestGateway {
91103
}
92104

93105
async open(input: OpenPullRequestInput, signal?: AbortSignal): Promise<OpenedPullRequest> {
106+
signal = this.#bounded(signal);
94107
this.#validate(input);
95108
if (markerOf(input.body) !== input.marker) throw new Error('The pull request description must start with its marker.');
96109
const post = () => this.#json(['api', '-X', 'POST', '-H', 'Accept: application/vnd.github+json', `repos/${this.repository}/pulls`], signal,
@@ -104,6 +117,7 @@ export class GhPullRequestGateway implements PullRequestGateway {
104117
}
105118

106119
async findOpened(input: { base: string; headBranch: string; markers: readonly string[] }, signal?: AbortSignal): Promise<(OpenedPullRequest & { marker: string }) | null> {
120+
signal = this.#bounded(signal);
107121
this.#validate(input);
108122
const owner = this.repository.split('/')[0]!;
109123
const query = new URLSearchParams({ state: 'open', head: `${owner}:${input.headBranch}`, base: input.base, per_page: '100' });
@@ -118,6 +132,7 @@ export class GhPullRequestGateway implements PullRequestGateway {
118132
}
119133

120134
async refresh(number: number, input: OpenPullRequestInput & { ready: boolean; headSha?: string }, signal?: AbortSignal): Promise<OpenedPullRequest> {
135+
signal = this.#bounded(signal);
121136
this.#validate(input);
122137
if (!Number.isSafeInteger(number) || number < 1) throw new Error('Invalid pull request number.');
123138
if (markerOf(input.body) !== input.marker) throw new Error('The pull request description must start with its marker.');
@@ -136,6 +151,7 @@ export class GhPullRequestGateway implements PullRequestGateway {
136151

137152
/** The caller has just read the PR as ready, so this changes it straight away and reads the result back once. */
138153
async markDraft(number: number, input: { base: string; headBranch: string; marker: string }, signal?: AbortSignal): Promise<OpenedPullRequest> {
154+
signal = this.#bounded(signal);
139155
this.#validate(input);
140156
if (!Number.isSafeInteger(number) || number < 1) throw new Error('Invalid pull request number.');
141157
let refused: unknown = null;

‎runner/publish.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,9 +95,15 @@ export class PullRequestPublisher {
9595
// before the push could move it.
9696
const live = await this.#pulls.findOpened({ base: this.#config.baseBranch, headBranch: branch, markers: candidates.map(pr => marker(pr.openingId)) }, signal);
9797
signal?.throwIfAborted();
98-
const earlier = live ? candidates.find(pr => marker(pr.openingId) === live.marker)! : undefined;
98+
let earlier = live ? candidates.find(pr => marker(pr.openingId) === live.marker)! : undefined;
9999
// What GitHub shows is the truth for the draft flag: a draft change whose record was lost is repaired here.
100100
let stateVersion = task.stateVersion;
101+
// An abandoned opening's PR that is now visible is adopted at once, whatever the check says next, so the record
102+
// always names every PR the task has on GitHub (for reuse, cancel or cleanup). It does not change the task status.
103+
if (earlier && live && earlier.state === 'abandoned') {
104+
stateVersion = this.#store.adoptOpening(identity, earlier.openingId, live, stateVersion);
105+
earlier = { ...earlier, state: 'opened', number: live.number, url: live.url, draft: live.draft };
106+
}
101107
if (earlier && live && earlier.state === 'opened' && earlier.number === live.number && earlier.draft !== live.draft)
102108
stateVersion = this.#store.recordPullRequestDraft(identity, earlier.openingId, live.number, live.draft, stateVersion);
103109
const own = new Set(prs.filter(pr => pr.number !== null && pr.repository.toLowerCase() === this.#config.repository.toLowerCase()).map(pr => pr.number!));
@@ -117,7 +123,7 @@ export class PullRequestPublisher {
117123
const needsDraft = result.outcome !== 'clear' && earlier && live && !live.draft;
118124
if (needsDraft) this.#store.assertUnchangedSince(identity, { stateVersion, reviewVersion, snapshotId: snapshot.id, draft });
119125
let drafted = null, leftReady: number | undefined;
120-
if (needsDraft) {
126+
if (needsDraft && earlier && live) {
121127
try { drafted = await this.#pulls.markDraft(live.number, { base: this.#config.baseBranch, headBranch: branch, marker: marker(earlier.openingId) }, signal); }
122128
catch (error) { if (!(error instanceof DraftsUnsupported)) throw error; leftReady = live.number; }
123129
}
@@ -136,8 +142,7 @@ export class PullRequestPublisher {
136142
}
137143
// The push is a refresh's first content write (it moves the open PR's head), so the refresh is recorded before it;
138144
// beginRefresh re-reads the task after the draft change's await. A task change during the push cannot strand it.
139-
const stateVersion = this.#store.beginRefresh(identity, { checkId: check.id, openingId: earlier.openingId, headSha: snapshot.head, draft,
140-
...(earlier.state === 'abandoned' ? { adopt: { number: live.number, url: live.url } } : {}) });
145+
const stateVersion = this.#store.beginRefresh(identity, { checkId: check.id, openingId: earlier.openingId, headSha: snapshot.head, draft });
141146
await this.#pusher.push(identity, { head: snapshot.head, branch }, signal);
142147
signal?.throwIfAborted();
143148
// Any failure here, including a draft refusal after the PR was made ready again meanwhile, leaves the update

‎runner/store.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1064,6 +1064,22 @@ export class Store {
10641064
return this.#task(key).status as TaskStatus;
10651065
});
10661066
}
1067+
/**
1068+
* An abandoned opening whose PR became visible is the task's PR after all: record its number, URL and draft state so
1069+
* it can be reused, closed or cleaned up. The task status does not change. Guarded by the state version the caller
1070+
* read; returns the new state version.
1071+
*/
1072+
adoptOpening(identity: PlanIdentity, openingId: string, pr: { number: number; url: string; draft: boolean }, expectedStateVersion: number): number {
1073+
if (!Number.isSafeInteger(pr.number) || pr.number < 1 || typeof pr.url !== 'string') throw new Error('Invalid pull request.');
1074+
const key = identityKey(identity);
1075+
return this.#transaction(() => {
1076+
if (this.#task(key).state_version !== expectedStateVersion) throw new GuardRefusal('Stale task state. Reload before writing.');
1077+
if (this.#run("UPDATE task_pull_requests SET state='opened', number=?, url=?, draft=?, updated_at=? WHERE plan_key=? AND opening_id=? AND state='abandoned'",
1078+
pr.number, pr.url, pr.draft ? 1 : 0, new Date().toISOString(), key, openingId).changes !== 1) throw new GuardRefusal('No abandoned opening with this ID.');
1079+
this.#touch(key);
1080+
return this.#task(key).state_version as number;
1081+
});
1082+
}
10671083
/** The task, its review and its head are exactly as a publish read them before its last await. */
10681084
assertUnchangedSince(identity: PlanIdentity, input: { stateVersion: number; reviewVersion: number; snapshotId: string; draft: boolean }): void {
10691085
const key = identityKey(identity);

‎test/publish.test.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -557,6 +557,32 @@ describe('recovering from an abandoned opening whose PR appears later', () => {
557557
expect(store.taskPullRequests(identity)).toMatchObject([{ openingId: first!.openingId, state: 'opened', number: 100 }, { state: 'abandoned' }]);
558558
expect(third.log.filter(line => line.startsWith('open'))).toEqual([]);
559559
});
560+
it('adopts an abandoned opening\'s PR as soon as it is seen, even when the check then matches', async () => {
561+
const store = runningTask(), live = new Map<string, OpenedPullRequest>(), next = { value: 100 }, hidden = new Set<string>();
562+
const later = { now: () => Date.now() + 10 * 60_000 };
563+
await expect(harness(store, { live, next, openTimesOut: true }).publisher.publish(identity)).rejects.toThrow('timeout');
564+
for (const m of live.keys()) hidden.add(m);
565+
// The opening is abandoned after its settle time; a new POST is refused (the first PR exists), leaving one opening.
566+
await expect(harness(store, { live, next, hidden, config: later }).publisher.publish(identity)).rejects.toThrow(/already exists/);
567+
hidden.clear();
568+
const found: AlreadyFixedResult = { outcome: 'found', baseHead: oid(9), matches: [{ kind: 'closed', by: 'owner/repo#5' }] };
569+
expect(await harness(store, { live, next, results: [found], config: later }).publisher.publish(identity)).toMatchObject({ kind: 'possibly already fixed' });
570+
// The first opening's PR is now recorded with its number and URL, so it can be found and closed later.
571+
expect(store.taskPullRequests(identity)).toMatchObject([{ state: 'opened', number: 100, url: expect.stringContaining('github.com') }, { state: 'abandoned' }]);
572+
});
573+
it('does not adopt when the task changed during the lookup', async () => {
574+
const store = runningTask(), live = new Map<string, OpenedPullRequest>(), next = { value: 100 }, hidden = new Set<string>();
575+
const later = { now: () => Date.now() + 10 * 60_000 };
576+
await expect(harness(store, { live, next, openTimesOut: true }).publisher.publish(identity)).rejects.toThrow('timeout');
577+
for (const m of live.keys()) hidden.add(m);
578+
await expect(harness(store, { live, next, hidden, config: later }).publisher.publish(identity)).rejects.toThrow(/already exists/);
579+
hidden.clear();
580+
// Recovery drops the second opening; then the task changes while the branch lookup runs.
581+
let finds = 0;
582+
const again = harness(store, { live, next, config: later, onFind: () => { if (++finds === 2) store.setAssignment(identity, store.getTask(identity).stateVersion, 'someone-else', 'code'); } });
583+
await expect(again.publisher.publish(identity)).rejects.toThrow(/Stale task state/);
584+
expect(store.taskPullRequests(identity).map(pr => pr.state)).toEqual(['abandoned', 'abandoned']);
585+
});
560586
it('refuses before pushing when the branch PR has the earlier marker but another number', async () => {
561587
const store = runningTask(), live = new Map<string, OpenedPullRequest>(), next = { value: 100 };
562588
store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs human');
@@ -789,6 +815,20 @@ describe('GitHub PR adapter', () => {
789815
await expect(gh.open({ ...input, draft: true })).rejects.toThrow(/as ready, not as a draft/);
790816
expect(await gh.open({ ...input, draft: false })).toMatchObject({ draft: false });
791817
});
818+
it('bounds a whole refresh by one deadline, not a fresh allowance per command or poll', async () => {
819+
let calls = 0, lastSignal: AbortSignal | undefined;
820+
const gh = new GhPullRequestGateway({ repository: 'owner/repo', operationMs: 300 }, async (args, options) => {
821+
calls++; lastSignal = options?.signal;
822+
await new Promise(resolve => setTimeout(resolve, 40));
823+
// GitHub never shows the pushed head, so the refresh would poll 5 times at 500 ms without the deadline.
824+
return args[0] === 'pr' ? '' : JSON.stringify(response({ draft: false, head: { sha: oid(2), ref: 'codeboost/issue-12-task', repo: { full_name: 'owner/repo' } } }));
825+
});
826+
const started = Date.now();
827+
await expect(gh.refresh(7, { ...input, draft: false, ready: true, headSha: oid(3) })).rejects.toThrow();
828+
expect(Date.now() - started).toBeLessThan(1_500);
829+
expect(lastSignal?.aborted).toBe(true);
830+
expect(calls).toBeLessThan(5);
831+
});
792832
it('finds a lost PR only by its marker, and refuses a PR on the branch that codeboost did not open', async () => {
793833
const calls: string[][] = [];
794834
const found = new GhPullRequestGateway({ repository: 'owner/repo' }, async args => { calls.push([...args]); return JSON.stringify([response()]); });

0 commit comments

Comments
 (0)