Skip to content

Commit 7fa67da

Browse files
fix(pm): fleet-write lands a pull mutation only when its answer shows the requested state (#20974)
Fixes #20826 Clause-②: no The relay judged a pull-request GraphQL row landed on HTTP 200 with `data` and no `errors`, whatever the returned pull showed. `enablePullRequestAutoMerge` answering `autoMergeRequest: null` on a pull it had neither armed nor queued printed `✓ … auto-merge off`, and the run concluded `success`. This PR has the op table declare what landed means for each pull mutation. The executor now reports a FAILED action (exit 5, later actions not attempted), naming what the answer showed, whenever the state is missing. That is how a REST 4xx is already reported. ## What was measured before choosing the predicate All readings are from the relay's own run logs in this repository's Actions (`fleet-write.yml`). They were read through the job-log API. 1. **The card's "GraphQL `errors` are not surfaced" is false as stated.** At the relay head those runs executed (`79114850f`), `requestLanded()` already fails any answer that carries a non-empty `errors` array. Runs 36694856488 and 36695124034 each printed `✓ action 1 automerge_enable: POST /graphql enablePullRequestAutoMerge -> HTTP 200 · #2491 · auto-merge off`. So both answers were 200 with `data.enablePullRequestAutoMerge.pullRequest.autoMergeRequest: null` and no `errors` entry. The raw bodies are not in the logs; only the formatted row is printed. The defect is the honesty half alone. 2. **The shape is shared.** `pr_ready`, `pr_draft`, `automerge_enable` and `automerge_disable` are the four ops whose descriptor carries a `pull`. All four went through the same 200-plus-data-means-landed test. `transfer` already judged its answer against the target, and the REST ops are unchanged. 3. **On a merge-queue base, arming a green pull answers with no `autoMergeRequest`, and that landing worked.** Two relay landings on this repository show it: - Run 36786665458: `pr_ready` then `automerge_enable` on objectstack PR 20952 printed `auto-merge off`. The PR timeline has `added_to_merge_queue` by the fleet bot at 22:38:08Z, inside that mutation's request window. There is no `auto_merge_enabled` event, and the pull merged. - Run 36780440589 (PR 20942) shows the same shape. So the naive predicate "`autoMergeRequest` non-null or fail" would have failed working landings. Relay landings that armed before the checks settled show the other shape: run 36784403107 (PR 20948) printed `auto-merge MERGE` for a `SQUASH` request, followed by `auto_merge_enabled` and a later `added_to_merge_queue`. GitHub's own input documentation says "when merging with a merge queue any input value for merge method is ignored", so the merge method is never judged. ## The change - `ops.mjs` has a new `PR_LANDED_STATE` entry per pull mutation, carried on its descriptor as `landed`: - `pr_ready` lands on `isDraft: false`, and `pr_draft` on `isDraft: true`. - `automerge_enable` lands on an `autoMergeRequest`, or on `isInMergeQueue: true`. - `automerge_disable` lands on `autoMergeRequest: null`. It deliberately does not also require the pull to be out of the queue: disabling does not dequeue, and failing there would stop the stroke's later `pr_draft`, the second half of the disarm AGENTS.md prescribes. Its row prints `in the merge queue` when the answer says so. - Both auto-merge mutations now also select `isInMergeQueue` and `isMergeQueueEnabled` (`PullRequest`, both `Boolean!`). - The first is the queue half of the arm's landing test. - The second says in the row whether the base has a queue, which the cloud diagnosis below needs. - `gh pr merge` reads both fields to take its merge-queue path, and it runs under a workflow token that holds only `contents` and `pull-requests`. That is the evidence that the relay's token (the same two, plus `issues` and `metadata`) can read them. - `execute.mjs`: `requestLanded()` judges every pull mutation against its `landed` entry. A pull descriptor with no entry, or an answer with no `pullRequest`, is never a landing. The new `pullStateText()` prints every selected field in the row, on success and on failure. - The card's interim suggestion was a header note in `ops.mjs` saying cloud arms need the maintainer's click. That note is not needed: an arm that does not land now fails loudly, and its row names what came back. The reported shape now reads: ```text FAILED: GraphQL: HTTP 200 with no errors, but the answer shows #2491 · auto-merge off · not in the merge queue · its base has no merge queue — the op asked for auto-merge armed (an autoMergeRequest) or the pull in the merge queue (isInMergeQueue true) ``` ## Verification (at `f620d1ce4`) - `node scripts/pm/fleet-write/execute.mjs --self-test`: exit 0, `71 cases pass across 11 batteries`. Before the change it was 59 across 10. The new battery pins 12 cases under a raised floor of 11 batteries: - the reported shape fails, names what came back, and stops the stroke; - an armed arm on a base with no queue lands; - the enqueued-at-once merge-queue shape lands; - a `MERGE`-for-`SQUASH` arm lands; - the mirror rows for `automerge_disable`, `pr_ready` and `pr_draft`; - an answer with no `pullRequest`, and a descriptor with no entry, are never landings; - the table pin: every pull mutation carries its entry, and its query selects every field that entry reads. - Reverse verification, all run from the committed state through `scripts/ablation-replace.mjs`. Each mutation was proven on disk, and each restore was proven by blob equal to HEAD with `git diff HEAD` empty: - dropping the landed check in `requestLanded()` turned 5 of 71 red (the reported shape, the stop, the disable, and both flips); - dropping the queue half of the arm predicate turned 1 of 71 red (the merge-queue case); - dropping `isInMergeQueue` from the arm's selection turned 1 of 71 red (the table pin). - Named and derived gates, all exit 0: `pnpm check:pm-fleet-write-execute`, `check:pm-fleet-write-validate`, `check:pm-fleet-write-dispatch`, `node scripts/check-self-test-wired.mjs`, `node scripts/check-scripts-symbol-anchors.mjs`, `pnpm check:nul-bytes`, plus every family `node scripts/pm/dispatch-gates.mjs --commands` derives for this diff (listed in the report on the card). ## Acceptance notes - NOT MEASURED, family relay-live-answer. What the enqueued-at-once answer carries in `isInMergeQueue`, and whether the fleet token reads that field, cannot be measured before merge. The relay is the only GraphQL route with that token, and GraphQL is closed to agent sessions. The evidence for `true`: the `added_to_merge_queue` event lands inside the mutation's own request window, with the mutation's own actor, on both measured landings, and the payload is resolved after the mutation. The first live objectstack landing after merge measures it: its row reads `in the merge queue`. If it ever reads `not in the merge queue` on a pull the timeline shows queued, the relay reports FAILED on a queued pull. That is loud, not silent, and the timeline stays the authoritative reading, so do not re-arm on the row alone. - The cause on cloud stays open, and nothing about it is encoded here. It is carried in the card report's open question, together with the reads that settle it. --- _Generated by [Claude Code](https://claude.ai/code/session_01JAhu8u8QfBvRjVZDox7CP9)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 05be352 commit 7fa67da

2 files changed

Lines changed: 134 additions & 17 deletions

File tree

‎scripts/pm/fleet-write/execute.mjs‎

Lines changed: 79 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,11 @@
4141
* Every action is validated AGAIN here (defence in depth: the executor trusts
4242
* the validator's rule, not the step that ran it), then its requests are
4343
* issued in order with the App token: REST calls straight from the op table;
44-
* the pull-request GraphQL ops resolve the pull's node id with one GET first;
45-
* a `transfer` reads the issue (refusing a pull request, or a card that no
44+
* the pull-request GraphQL ops resolve the pull's node id with one GET first,
45+
* and land only when the answer's `pullRequest` SHOWS the state the op table
46+
* declares for them (`PR_LANDED_STATE` in `ops.mjs`) — a 200 with `data` and
47+
* no `errors` whose pull is not in that state is a FAILED action naming what
48+
* the answer showed, exactly as a REST 4xx is; a `transfer` reads the issue (refusing a pull request, or a card that no
4649
* longer answers from the source — already moved) and the target repository,
4750
* then sends `transferIssue` with both node ids and accepts only an answer
4851
* that places the card on the target. A transfer that fails prints the
@@ -85,7 +88,7 @@ import { isEntrypoint } from '../../invoked-as.mjs';
8588
import { scrub } from '../fleet-token.mjs';
8689
import { classifyHttp } from '../label-write.mjs';
8790
import { EXIT_WRITE_PACE_REFUSED, isWriteMethod, noteResponse, paceWrite, releaseWriteLease } from '../write-pace.mjs';
88-
import { OPS, PERMISSIONS, transferRemedy } from './ops.mjs';
91+
import { OPS, PERMISSIONS, PR_LANDED_STATE, transferRemedy } from './ops.mjs';
8992
import { PAYLOAD_ENV, refusalText, tokenRepositoriesOf, validatePayload } from './validate.mjs';
9093

9194
const SELF_PATH = fileURLToPath(import.meta.url);
@@ -131,7 +134,12 @@ export function repoOfIssue(json) {
131134
return m ? m[1] : null;
132135
}
133136

134-
/** Did this answer land the request? A 404 on a directed label DELETE is the label already being gone. */
137+
/**
138+
* Did this answer land the request? A 404 on a directed label DELETE is the label already being gone.
139+
* A pull-request mutation lands only when its answer SHOWS the state the op table declares for it
140+
* (`PR_LANDED_STATE`, carried on the descriptor as `landed`) — a 200 with `data` and no `errors` is
141+
* the platform accepting the request, not the pull being in that state.
142+
*/
135143
export function requestLanded(req, { status, json } = {}) {
136144
if (req.graphql) {
137145
if (status !== 200 || !json || typeof json !== 'object') return { ok: false, why: `HTTP ${status}` };
@@ -144,24 +152,41 @@ export function requestLanded(req, { status, json } = {}) {
144152
if (!moved || !Number.isInteger(moved.number)) return { ok: false, why: 'GraphQL: the answer carries no transferred issue' };
145153
if (String(where ?? '').toLowerCase() !== req.graphql.target_repo.toLowerCase()) return { ok: false, why: `GraphQL: the answer places the issue on ${where ?? 'no repository'}, not ${req.graphql.target_repo}` };
146154
}
155+
if (req.graphql.pull !== undefined) {
156+
const landed = req.graphql.landed;
157+
if (!landed || typeof landed.holds !== 'function') return { ok: false, why: `the op table declares no landed state for ${req.graphql.mutation}, so its answer cannot be judged — a 200 alone is not a landing` };
158+
const pr = json.data[req.graphql.mutation]?.pullRequest;
159+
if (!pr || typeof pr !== 'object') return { ok: false, why: 'GraphQL: the answer carries no pullRequest' };
160+
if (!landed.holds(pr)) return { ok: false, why: `GraphQL: HTTP 200 with no errors, but the answer shows ${pullStateText(pr)} — the op asked for ${landed.wants}` };
161+
}
147162
return { ok: true, why: '' };
148163
}
149164
if (status >= 200 && status < 300) return { ok: true, why: '' };
150165
if (status === 404 && req.idempotent404) return { ok: true, why: 'already absent', idempotent: true };
151166
return { ok: false, why: `HTTP ${status}${typeof json?.message === 'string' ? ` — ${json.message}` : ''}` };
152167
}
153168

169+
/**
170+
* A pull request's state as a mutation answered it — every field the row selected, so a failure names
171+
* what came back and a landing names how (armed, or already in the merge queue).
172+
*/
173+
export function pullStateText(pr) {
174+
const bits = [`#${pr.number}`];
175+
if (typeof pr.isDraft === 'boolean') bits.push(pr.isDraft ? 'draft' : 'ready');
176+
if ('autoMergeRequest' in pr) bits.push(pr.autoMergeRequest ? `auto-merge ${pr.autoMergeRequest.mergeMethod ?? ''}`.trim() : 'auto-merge off');
177+
if (typeof pr.isInMergeQueue === 'boolean') bits.push(pr.isInMergeQueue ? 'in the merge queue' : 'not in the merge queue');
178+
if (typeof pr.isMergeQueueEnabled === 'boolean') bits.push(pr.isMergeQueueEnabled ? 'its base has a merge queue' : 'its base has no merge queue');
179+
return bits.join(' · ');
180+
}
181+
154182
/** The one thing a seat needs from an answer: the id / number / url of what was written. */
155183
export function resultOf(req, json) {
156184
if (req.graphql) {
157185
const moved = json?.data?.[req.graphql.mutation]?.issue;
158186
if (moved) return [`#${moved.number}`, moved.url, moved.repository?.nameWithOwner ? `(now on ${moved.repository.nameWithOwner})` : ''].filter(Boolean).join(' ');
159187
const pr = json?.data?.[req.graphql.mutation]?.pullRequest;
160188
if (!pr) return 'ok';
161-
const bits = [`#${pr.number}`];
162-
if (typeof pr.isDraft === 'boolean') bits.push(pr.isDraft ? 'draft' : 'ready');
163-
if ('autoMergeRequest' in pr) bits.push(pr.autoMergeRequest ? `auto-merge ${pr.autoMergeRequest.mergeMethod ?? ''}`.trim() : 'auto-merge off');
164-
return bits.join(' · ');
189+
return pullStateText(pr);
165190
}
166191
if (Array.isArray(json)) return `${json.length} label(s) now on the issue`;
167192
if (!json || typeof json !== 'object') return 'ok';
@@ -362,13 +387,14 @@ const SELF_TEST_BATTERIES = Object.freeze({
362387
'the run: actions in order, stop at the first failure, later actions never attempted': 6,
363388
'the idempotent removal: a 404 on a directed label DELETE is success': 2,
364389
'the GraphQL ops: the node id first, then the mutation; an errors array is a failure': 5,
390+
'the PR-state landing: a 200 lands only when the answer SHOWS the state the op asked for — armed or queued for auto-merge, off for its disable, ready and draft for the flips': 12,
365391
'the summary: request, sender and role, session, target, one row per request': 5,
366392
'redaction: the token reaches no summary line, log line or error': 3,
367393
'the wiring: both halves around every write verb, on the roster': 4,
368394
'the CLI: environment inputs, the payload refusal, the exit ladder': 7,
369395
'the transfer: the sender gated on BOTH repositories, both node ids first, a pull or a moved card refused before the mutation, landing only on the target, the remedy on failure': 11,
370396
});
371-
const SELF_TEST_BATTERY_FLOOR = 10;
397+
const SELF_TEST_BATTERY_FLOOR = 11;
372398
const UNATTRIBUTED_BATTERY = '(unattributed)';
373399

374400
const batteryCases = new Map();
@@ -530,6 +556,49 @@ export async function selfTest() {
530556
t('a pull whose node id cannot be read is a failed action with no mutation sent', [noNode.exit, noNode.seen.some((s) => s.call === 'POST /graphql')], [EXIT_ACTION_FAILED, false]);
531557
}
532558

559+
// ── the PR-state landing ────────────────────────────────────────────────
560+
battery('the PR-state landing: a 200 lands only when the answer SHOWS the state the op asked for — armed or queued for auto-merge, off for its disable, ready and draft for the flips');
561+
{
562+
const pull = { [`GET /repos/${REPO}/pulls/21`]: { status: 200, json: { number: 21, node_id: 'PR_kwDO21' } } };
563+
const answer = (mutation, pr) => ({ ...allowed, ...pull, 'POST /graphql': { status: 200, json: { data: { [mutation]: { pullRequest: { number: 21, ...pr } } } } } });
564+
const enable = (pr, actions = [{ op: 'automerge_enable', pull: 21 }]) => run(base(actions), answer('enablePullRequestAutoMerge', pr), { file: paceFile });
565+
566+
// The shape the card measured: 200, no errors, nothing armed, nothing queued — and a later action behind it.
567+
const unarmed = await run(base([{ op: 'automerge_enable', pull: 21 }, { op: 'comment', issue: 21, body: 'after' }]), {
568+
...answer('enablePullRequestAutoMerge', { autoMergeRequest: null, isInMergeQueue: false, isMergeQueueEnabled: false }),
569+
[`POST /repos/${REPO}/issues/21/comments`]: { status: 201, json: { id: 9 } },
570+
});
571+
t('⛔ automerge_enable answered 200 with no errors, no autoMergeRequest and the pull not queued is a FAILED action (exit 5)', [unarmed.exit, unarmed.rows[0]?.status], [EXIT_ACTION_FAILED, 200]);
572+
t('…whose row names what the answer showed and what was asked for', [unarmed.rows[0]?.result.startsWith('FAILED'), ['auto-merge off', 'not in the merge queue', 'its base has no merge queue', 'auto-merge armed'].every((s) => unarmed.rows[0]?.result.includes(s))], [true, true], unarmed.rows[0]?.result);
573+
t('…and the stroke stops there: the later action is never attempted, and the summary says so', [writes(unarmed.seen).map((s) => s.call), unarmed.summary.includes('stopped at action 1 (`automerge_enable`)')], [['POST /graphql'], true]);
574+
575+
const armed = await enable({ autoMergeRequest: { enabledAt: 'now', mergeMethod: 'SQUASH' }, isInMergeQueue: false, isMergeQueueEnabled: false });
576+
t('the happy path still lands: armed on a base with no merge queue', [armed.exit, armed.rows[0]?.result], [EXIT_OK, '#21 · auto-merge SQUASH · not in the merge queue · its base has no merge queue']);
577+
// Measured on this repository's queue: a pull whose checks are green is enqueued at once and the answer carries no autoMergeRequest.
578+
const queued = await enable({ autoMergeRequest: null, isInMergeQueue: true, isMergeQueueEnabled: true });
579+
t('a green pull enqueued at once on a merge-queue base lands, though the answer carries no autoMergeRequest', [queued.exit, queued.rows[0]?.result], [EXIT_OK, '#21 · auto-merge off · in the merge queue · its base has a merge queue']);
580+
const queueArmed = await enable({ autoMergeRequest: { enabledAt: 'now', mergeMethod: 'MERGE' }, isInMergeQueue: false, isMergeQueueEnabled: true });
581+
t('⛔ the merge method is never judged: a merge-queue base answers MERGE for a SQUASH request, and that arm lands', [queueArmed.exit, queueArmed.rows[0]?.result], [EXIT_OK, '#21 · auto-merge MERGE · not in the merge queue · its base has a merge queue']);
582+
583+
const stillArmed = await run(base([{ op: 'automerge_disable', pull: 21 }]), answer('disablePullRequestAutoMerge', { autoMergeRequest: { enabledAt: 'then' }, isInMergeQueue: false, isMergeQueueEnabled: true }));
584+
t('automerge_disable whose answer still carries an autoMergeRequest is a FAILED action', [stillArmed.exit, stillArmed.rows[0]?.result.includes('auto-merge off (autoMergeRequest null)')], [EXIT_ACTION_FAILED, true], stillArmed.rows[0]?.result);
585+
const disarmedQueued = await run(base([{ op: 'automerge_disable', pull: 21 }]), answer('disablePullRequestAutoMerge', { autoMergeRequest: null, isInMergeQueue: true, isMergeQueueEnabled: true }), { file: paceFile });
586+
t('automerge_disable lands on auto-merge off even for a queued pull — the row says it is still in the merge queue', [disarmedQueued.exit, disarmedQueued.rows[0]?.result], [EXIT_OK, '#21 · auto-merge off · in the merge queue · its base has a merge queue']);
587+
588+
const stillDraft = await run(base([{ op: 'pr_ready', pull: 21 }]), answer('markPullRequestReadyForReview', { isDraft: true }));
589+
const stillReady = await run(base([{ op: 'pr_draft', pull: 21 }]), answer('convertPullRequestToDraft', { isDraft: false }));
590+
t('pr_ready answered isDraft true, and pr_draft answered isDraft false, are FAILED actions naming the state', [stillDraft.exit, stillDraft.rows[0]?.result.includes('#21 · draft'), stillReady.exit, stillReady.rows[0]?.result.includes('#21 · ready')], [EXIT_ACTION_FAILED, true, EXIT_ACTION_FAILED, true]);
591+
const noPull = await run(base([{ op: 'automerge_enable', pull: 21 }]), { ...allowed, ...pull, 'POST /graphql': { status: 200, json: { data: { enablePullRequestAutoMerge: null } } } });
592+
t('an answer that carries no pullRequest is a FAILED action, never a landing', [noPull.exit, noPull.rows[0]?.result.includes('carries no pullRequest')], [EXIT_ACTION_FAILED, true]);
593+
594+
const bare = { verb: 'POST', path: '/graphql', graphql: { mutation: 'enablePullRequestAutoMerge', query: 'q', pull: 21 } };
595+
t('⛔ a pull mutation whose descriptor declares no landed state is never judged landed', requestLanded(bare, { status: 200, json: { data: { enablePullRequestAutoMerge: { pullRequest: { number: 21, autoMergeRequest: { mergeMethod: 'SQUASH' } } } } } }).ok, false);
596+
// The table half: every pull mutation the op table can issue carries its entry, and selects every field that entry reads.
597+
const pullRows = Object.keys(OPS).flatMap((op) => OPS[op].requests({ op, pull: 21, issue: 21, comment_id: 1, target_repo: 'x', title: 't', body: 'b', head: 'h', base: 'main', labels: ['l'], assignees: ['u'], reviewers: ['r'] }, REPO).filter((r) => r.graphql?.pull !== undefined).map((r) => [op, r.graphql]));
598+
const selects = (query, field) => new RegExp(`\\b${field}\\b`).test(query.slice(query.indexOf('pullRequest {')));
599+
t('every pull mutation in the op table carries its PR_LANDED_STATE entry, and its query selects every field that entry reads', [pullRows.map(([op]) => op).sort(), pullRows.every(([op, g]) => g.landed === PR_LANDED_STATE[op] && g.landed.fields.every((f) => selects(g.query, f)))], [Object.keys(PR_LANDED_STATE).sort(), true]);
600+
}
601+
533602
// ── the transfer ────────────────────────────────────────────────────────
534603
battery('the transfer: the sender gated on BOTH repositories, both node ids first, a pull or a moved card refused before the mutation, landing only on the target, the remedy on failure');
535604
{
@@ -664,7 +733,7 @@ export async function selfTest() {
664733
console.log(
665734
`✓ fleet-write/execute self-test: ${cases.length} cases pass across ${declared.length} batteries — the sender gate from the target repo's answer, ` +
666735
'every op as the request the table declares, stop at the first failure with later actions untouched, the idempotent label DELETE, the GraphQL ' +
667-
'ops behind a node-id read, a transfer gated on both repositories and landing only on its target, one summary row per request, and a known ' +
736+
'ops behind a node-id read, a pull mutation landing only when its answer shows the state it asked for (armed or queued, off, ready, draft), a transfer gated on both repositories and landing only on its target, one summary row per request, and a known ' +
668737
'token that came back out of NO summary, log or error.',
669738
);
670739
selfTestReachedVerdict = true;

0 commit comments

Comments
 (0)