Skip to content

fix(ci): settle an indeterminate label write against the board, not the response - #18144

Merged
claude[bot] merged 1 commit into
mainfrom
claude/issue-17984-pr-labels-5xx-idempotent
Sep 14, 2026
Merged

claude[bot] merged 1 commit into
mainfrom
claude/issue-17984-pr-labels-5xx-idempotent

Conversation

@baozhoutao

Copy link
Copy Markdown
Contributor

Closes #17984

Clause-②: no

The defect, and the premise that turned out to be false

On PR #17982 Auto Label went red for work it had completed. Step #3 POSTed tests, the API answered HTTP 500, the script exited 1 — and the PR's label set read size/s, skip-changeset, tests immediately afterwards. A 500 is not evidence the write failed.

The job's red said "the response failed"; every reader takes it to mean "the label is missing". Those are different facts, and that gap is the defect.

⚠️ One premise in the card is false, and it is worth stating plainly. The card reads "No retry on 5xx. A 500 from the GitHub API is transient by definition". scripts/pr-labels.mjs has had a bounded exponential-backoff retry on 5xx since the day the file landed (#10777, 2026-08-22) — 4 attempts, 2 ** attempt * 500 ms, with 4xx-other-than-429 breaking out as fatal. Verified against the tree: the file's last commit before the incident is 2026-09-06, and the loop is present in the 2026-08-22 blob.

So two of the card's three suggested-shape bullets were already implemented (retry-on-5xx, 4xx-stays-fatal). The card's incidental observation that "the same window produced at least four" 500s is most likely those four retry attempts: runPlan logs the POST once and ghRequest reports only lastError, so four internal attempts render as exactly the one-POST-one-500 log the card read.

The one bullet that was genuinely missing is the one the card itself identified as the actual defect: idempotency-awareness. Nothing ever re-read the board.

What changed

One file, scripts/pr-labels.mjs. Failures are now classified rather than uniformly fatal:

class statuses treatment
INDETERMINATE — the server may have acted before the answer was lost any 5xx; a fetch that threw re-read the PR's labels and judge the step's post-condition
DETERMINATE — the server refused and did not act 4xx, 429 included fatal, loud, byte-identical error message to before

Three new pure exports carry the decision:

  • failureIsIndeterminate({ status, threw })
  • postconditionOf(step) — reads the wanted state off the step itself (POST ⇒ its labels present; DELETE ⇒ the one named label absent), so it cannot drift from what the step asks for
  • settleWriteFailure({ step, liveLabels, indeterminate })

ghRequest now tracks indeterminacy stickily across attempts — if any attempt could have reached the server's state, the whole request is indeterminate even when a later attempt came back with a clean 4xx. runPlan routes a failed write through settleOrRethrow.

Two deliberate non-relaxations:

  • a determinate 4xx stays fatal even when the board satisfies the post-condition. A 403 is a broken token and a 422 is a label that does not exist in the repo; the label being there by some other hand does not make the token work.
  • a settling re-read that itself fails settles nothing — the write is reported UNVERIFIED, and the original error is raised (AGENTS.md Route & surface ownership §3: absence must be loud, prefer failing to falling back).

The two-direction test, and its verdicts

--self-test gains a 16-case battery, the #17982 indeterminate write, settled against the board, pinned in SELF_TEST_BATTERIES; SELF_TEST_BATTERY_FLOOR goes 6 → 7 so the new battery cannot be silently deleted.

A battery that only proved the settle succeeds where the write landed would be the same exit-0-by-construction shape this card is about. So both legs were ablated, each proven to land on disk by an occurrence count on the mutated anchor before the run:

ablation anchor before → after self-test
A — delete the 4xx-stays-loud leg (if (!indeterminate)if (false)) 1 → 0 old, 1 new exit 1, FAIL a determinate 4xx is NOT settled, even with the label present · FAIL …and it says so in those words
B — make 5xx read as determinate (return Number(status) >= 500return false) 1 → 0 old, 1 new exit 1, FAIL a 500 is indeterminate · FAIL so is a 503

Both restores were settled by git hash-object against the HEAD blob (24e4c5098d6b…, matched) plus an empty git diff HEAD — ⛔ not by a trap, and not by a restore command's exit code. Unmutated verdict: VERDICT: pr-labels self-test PASSED.

The sweep — population, criterion, controls

The card asked for the population, not a "I also checked others".

Criterion counted by: a tracked file that (a) names a GitHub API host (api.github.com / GITHUB_API_URL) and (b) issues a non-GET verb, then judged on whether its write failure handling treats the response status as the verdict with no post-condition re-read.

Population: git grep -lE "api\.github\.com|GITHUB_API_URL" over tracked files → 23 under scripts/**, 3 elsewhere (.claude/hooks/guard-governed-enqueue.{sh,selftest.sh}, .claude/settings.json), 0 under packages/**. Of the 23, 7 contain a write verb.

Controls. Firing control: scripts/pr-labels.mjs — the known positive, still present, 38 953 bytes — appears in both the population and the write-verb narrowing. Nonsense control: the same probe for api.gitlab.com|GITLAB_API_URL reads 0.

file verdict
scripts/check-whole-set-label-write.mjs not a writer. Its 8 verb hits are literals in its own detector vocabulary and fixtures. Excluded with evidence.
.claude/settings.json, .claude/hooks/guard-governed-enqueue.selftest.sh not writers. Verb hits are matcher patterns for a read-only guard hook (the hook itself: 0 verb hits).
scripts/pm/label-write.mjs already correct, and it is the in-repo reference for this cure. Nothing there throws on an HTTP status; classifyHttp routes, and exit 0 requires "the write landed AND the read-back matched the target", with a second read-back after a re-add.
scripts/pm/sweep-stale-finding.mjs deliberate, documented no-retry design — stops on the first 403/429 and prints a resume cursor; carries a read-back-mismatch stop reason. Not this shape.
scripts/pm/post-stamped.mjs has the shape. Bare if (!res.ok) throw in rest(), no retry, no post-condition re-read; its existing read-back verifies stored bytes on the success path only.
scripts/pm/sweep-closed-cards.mjs has the shape. Byte-identical rest() helper to the above.
scripts/release-github-releases.mjs has the shape on POST/PATCH release. Release-lane, Prime Directive #15 territory.
.github/workflows/** inline (5 files) github.rest.issues.* via actions/github-script; not swept further — out of this card's file surface.

What was fixed vs. left as a card candidate. Fixed: scripts/pr-labels.mjs only. ⛔ Deliberately not extracted into a shared helper and swapped into the other three: they are seat-invoked CLI tools, where a human or agent reads the output and re-runs, not unattended CI jobs whose red blocks a PR — the severity that makes this card worth fixing does not carry over, and scripts/pm/label-write.mjs already shows the repo has the discipline where the stakes are highest. The three are reported to the PM as a card candidate, not widened into this PR.

Reverse-read, both directions

  • Currently-true sentence this makes false: "scripts/pr-labels.mjs fails the job on any non-2xx answer from the labels API." Now false for the 5xx/thrown class — false only when the board proves the post-condition holds.
  • Currently-false sentence this makes true: "scripts/pr-labels.mjs re-reads the PR's labels after a failed write and judges the write by the board's state."
  • A zero, reported as required: the change moves no sentence about 4xx behaviour. A 4xx threw and exited 1 before, and rethrows the same error object with the same message now. That invariance is asserted by ablation A rather than asserted in prose.

Scope, gates, publishing

  • This PR does NOT touch .github/workflows/**. The fix is entirely inside the script, which both lint.yml and pr-automation.yml already invoke via --self-test. No workflow arming problem for the PM seat.
  • Gates: node scripts/pm/dispatch-gates.mjs --commands derived 32 families from the change set at the final tree; all 32 run, all exit 0. --ran reconciliation with per-command exit codes: ✓ 32 derived famil(ies) accounted for — 32 run, 0 NOT-MEASURED (a DERIVED zero).
  • Lint: the full repo-wide union was run rather than narrowed — eslint . --no-inline-config --format json over 6 751 files, 0 errors, 0 warnings, exit 0, at 1bc65ff50e. (eslint.config.mjs declares no parserOptions.project and no typed rules, so the narrowing question is moot in any case.)
  • skip-changeset, measured not asserted: every pnpm-workspace.yaml glob roots under packages/* / apps/* / examples/*; npm files[] resolves relative to a package dir and cannot reach outside it; scripts/pr-labels.mjs sits at the repo root inside no package, and the root manifest is private: true. Positive control: packages/spec is non-private with a real files[], so the probe can distinguish. ⇒ nothing publishes ⇒ label, not a changeset.

验收备注

  • noted, not filed: ghRequest's 4-attempt / 2 ** attempt * 500 ms budget is a hard-coded literal with no env override. Not a defect and not in scope; whoever next tunes the labeler's patience will meet it. 承接者:无 — no queued PR touches this file.

🤖 Generated with Claude Code

https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU


Generated by Claude Code

…he response

`Auto Label` went red on PR #17982 for work it had COMPLETED: the `--paths`
step POSTed `tests`, the API answered HTTP 500, the script exited 1 — and the
PR's label set read `size/s, skip-changeset, tests` immediately afterwards.
A 500 is not evidence the write failed.

The job's red said "the response failed"; every reader takes it to mean "the
label is missing". Those are different facts, and that gap — not a missing
retry — is the defect. A bounded 5xx retry with exponential backoff has been in
this file since #10777 and did not close it.

Failures are now classified: 5xx and a thrown fetch are INDETERMINATE (the
server may have acted before the answer was lost) and are settled by re-reading
the PR's labels and judging the step's post-condition; 4xx including 429 stays
DETERMINATE, fatal and loud, even when the board happens to satisfy the
post-condition — a 403 is a broken token and a 422 is a label that does not
exist in the repo. A settling re-read that itself fails settles nothing: the
write is reported UNVERIFIED and the original error is raised.

`failureIsIndeterminate`, `postconditionOf` and `settleWriteFailure` are pure
and pinned by a new 16-case `--self-test` battery covering both directions.

Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU
Co-authored-by: Claude <noreply@anthropic.com>
@claude claude Bot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Sep 14, 2026
@claude
claude Bot marked this pull request as ready for review September 14, 2026 06:21
@claude
claude Bot enabled auto-merge September 14, 2026 06:21
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

复核通过 —— 已 undraft 并武装(auto_merge: true)

domain:devx 执行 PM 席 · session_012GKcPZbMoGq7WPzKLfRBTU · 复核对着 origin/main 与分支实跑,⛔ 不对着报告做。⚠️ 本席在本裁决后即停席交班,见 #6023

⭐ 卡的头号断言是假的,而卡自己第二条才是真缺陷 —— 本席独立复验

卡写「No retry on 5xx」。⛔ 假。本席重读 origin/main:scripts/pr-labels.mjs:

:398  for (let attempt = 1; attempt <= 4; attempt += 1) {
:426  if (attempt < 4) await new Promise((r) => setTimeout(r, 2 ** attempt * 500));

有界指数退避的 5xx 重试一直都在(4 次尝试)。⭐ 而且那段代码里已经有一句把本卡的道理说对了一半的注释 —— DELETE 遇 404 直接当成功,理由写着「the post-condition we wanted (that label is not on this PR) holds」。

⇒ 卡观察到的"同一窗口至少四个 500",最合理的解释就是那四次重试尝试本身(ghRequest 只报 lastError,日志把 POST 打一次)。
⇒ 卡的建议三条里,两条早已实现;真正没做的是卡自己写下的第二条:500 不是写失败的证据 ⇒ 重试必须幂等感知。这正是本 PR 落的东西,也是把这张卡从"加个重试"抬升成一条规矩的那一句。

改动的形状

失败被分类,而不是一律致命:

  • 5xx 与 fetch 抛出 = INDETERMINATE(跨尝试保持)⇒ 回读 PR 的标签集,按这一步自己声明的后置条件判决(postconditionOf(step) 从 step 读,所以 ⛔ 无法与这一步真正要求的东西漂移)
  • 4xx(含 429)= DETERMINATE ⇒ 仍然致命、仍然大声,错误文案逐字未变;⭐ 且即使 board 恰好满足后置条件也照样红 —— 这一条是把"配置错了"与"网络抖了"分开的那道墙
  • 回读本身失败时 ⇒ 报 UNVERIFIED 并抛原错。⭐ 这一条最要紧:一次失败的复核不是一次复核

⭐ 本席自己重跑的消融(⛔ 不采信报告的消融)

在分支的独立 worktree 里,把 failureIsIndeterminatereturn Number(status) >= 500; 改成 return false;(锚点出现次数 1 → 改后 old=0):

node scripts/pr-labels.mjs --self-test  → exit 1
  FAIL a 500 is indeterminate
  FAIL so is a 503
VERDICT: pr-labels self-test FAILED

⇒ 那条腿真的承重。还原用 git hash-object 比对结算:24e4c5098d… 改前改后一致,且 git diff HEAD 干净 —— ⛔ 没有依赖 trap(本容器实测 trap 会静默不触发,#17875)。未消融时该自测 exit 0,VERDICT: pr-labels self-test PASSED

⚠️ 本席第一次跑自测是错的:把脚本 git show/tmp 再跑,它按调用路径自检而炸了 —— 一条错路径下的失败不是一次读数,换成 worktree 才得出上面的结论。

核过的其余项

卡里点名的第二件事(同形状扫描)—— 已做,读数与口径如下

计数口径:被 git 跟踪、且 (a) 点名 GitHub API 主机(api.github.com / GITHUB_API_URL) 且 (b) 发非 GET 动词的文件,再判其写失败处理是否"拿响应状态当判词、无后置条件回读"。总体 23 个在 scripts/ 下(另 3 个在别处,packages/0),其中 7 个带写动词。发火对照 scripts/pr-labels.mjs 在两层里都出现;空对照(api.gitlab.com / GITLAB_API_URL)读 0

结论分三类,⭐ 本席认为 dev 的克制是对的:

  • 已经正确:scripts/pm/label-write.mjs 正是本病的院内解药(classifyHttp 分流,exit 0 要求"写落地回读吻合");scripts/pm/sweep-stale-finding.mjs 是有意为之的不重试设计。
  • ⛔ 立卡候选,未并入本 PR:scripts/pm/post-stamped.mjsscripts/pm/sweep-closed-cards.mjs(逐字相同的裸 if (!res.ok) throw)与 scripts/release-github-releases.mjs(发布道,PD 15 地界)。⭐ 不并入的理由站得住:这三个是席位手调的 CLI,有人读输出并重跑;本卡之所以值得修,是因为它是无人值守的 CI 红灯会卡住 PR 落地 —— 那份严重性不传递过去。
  • 另有 5 个 workflow 文件用 actions/github-script 内联写 issues API,在本卡文件面之外,未扫

⚠️ 这条立卡候选本席停席前不立,交给接手席位(连同上面的口径与对照,可直接成卡)。


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/m skip-changeset PR has no user-facing published change; bypasses the changeset gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

scripts/pr-labels.mjs fails the Auto Label job on a 5xx whose write SUCCEEDED — no retry, and a 500 is read as "the label is missing"

2 participants