fix(factory): a decline must not erase labels changed while admission was deciding

[fairchild]

When the factory declines to work on an issue, it removes the ready label by rewriting the issue's whole label list from a copy it took several API calls earlier, so any label a person added in between was silently undone. This PR deletes only ready, with one DELETE call, and treats an already-removed label as done. One script and its tests, about 30 lines of code. Risk: low; every terminal decline path now takes the narrower write.

Summary

• A terminal decline (privileged, no_evidence_contract, missing_labels) now removes exactly ready with DELETE /issues/{n}/labels/ready. It used to PATCH the full label set computed from the snapshot claim() read several API calls earlier. A label someone adds or removes in that window now stands. Closes factory-implement: a terminal decline PATCHes the label set from a snapshot taken several API calls earlier #1596.
• New GitHubClient.remove_label(number, name) goes through the existing request helper and URL-quotes the name. A 404 counts as done: either the label is already off, or a retried DELETE's first attempt already landed. Every other error raises, as before.
• decline_payload is deleted; the two decline call sites were its only users.
• Only the script and its tests change. The claim PATCH and the rollback PATCH keep their current shape (see residual risk).

Mergeability

• Surface: agent-runtime / factory
• User-facing behavior changed: on a terminal decline, factory admission removes only ready. A label added or removed on the issue while admission was deciding stays as that person left it. The decline comments and every other admission outcome are unchanged.
• Non-happy paths considered: ready already gone when the DELETE lands (404, absorbed). A DELETE retried after a 5xx whose first attempt landed (404 on the retry, absorbed). A 403 or other non-transient error (raises, as the PATCH did). A label name that needs URL escaping (quoted). The issue timeline records the same unlabeled event for ready that the PATCH produced, so owner_release_event reads it the same way.
• Release/ops preconditions: none. No workflow, variable, secret, or permission changes. The single-label DELETE needs the same Issues write permission as the PATCH it replaces.
• Residual risk or follow-up: a successful claim still PATCHes claim_payload(issue) from the same snapshot, so a label changed in that window before a claim is still reverted. The fix there is a separate decision: two single-label writes (add claimed, remove ready) give up the PATCH's atomic swap. Rollback re-reads the issue first, so its window is smaller but has the same shape. The GitHubClient subclass in factory-review-response.py keeps its own remove_label, which does not absorb a 404, and now shadows this one. Behavior in that lane is unchanged.

Validation

• swift build
• swift test
• Other checks run: every scripts/tests file under uv run --script, plus the subprocess-timeout check. No Swift code is touched.

Performance

• Not a performance-sensitive change

Evidence Status

• [complete] A test in scripts/tests/test_factory_workflows.py asserting a terminal decline removes ready without rewriting labels added after the snapshot, red before the change -- test_terminal_decline_keeps_label_changes_made_after_the_snapshot has subTests privileged, no_evidence_contract, and missing_labels. In each, one label is added and another removed between the snapshot and the write. Red before the change: all three subTests failed because the PATCH re-added quality and dropped needs-human (Ran 2 tests in 0.016s, FAILED (failures=3, errors=1); the one error is the client test calling the not-yet-written remove_label). Green after (Ran 2 tests in 0.024s, OK). Red again with only the PATCH restored (Ran 1 test in 0.023s, FAILED (failures=3))
• [complete] Every scripts/tests/*.py passes under uv run --script -- for f in scripts/tests/test_*.py; do uv run --script "$f" || echo "FAILED $f"; done on 62d9238 printed no FAILED line across all 51 files (1411 tests, every file OK)

Evidence

• Tests named below: the command and the line it printed

Red first, then green, then two mutation checks:

• Red before the fix: uv run --script scripts/tests/test_factory_workflows.py -k test_terminal_decline_keeps_label_changes_made_after_the_snapshot -k test_remove_label_deletes_one_label printed Ran 2 tests in 0.016s, FAILED (failures=3, errors=1)
• Green after the fix, same command: Ran 2 tests in 0.024s, OK
• Mutation with the decline PATCH restored, snapshot test only: Ran 1 test in 0.023s, FAILED (failures=3). Restored, it passes again.
• Mutation with the 404 check broken (404 changed to 410), client test only: Ran 1 test in 0.002s, FAILED (errors=1) (HTTP 404: {"message": "Label does not exist"}). Restored, it passes again.

Gates, on 62d9238:

• uv run --script scripts/tests/test_factory_workflows.py: Ran 59 tests in 0.125s, OK
• uv run --script scripts/tests/test_factory_implement.py: Ran 27 tests in 0.052s, OK
• uv run --script scripts/tests/test_factory_sweep.py: Ran 20 tests in 0.291s, OK
• uv run --script scripts/tests/test_factory_janitor.py: Ran 29 tests in 0.119s, OK
• for f in scripts/tests/test_*.py; do uv run --script "$f" || echo "FAILED $f"; done: 51 files, 1411 tests, no FAILED line printed
• python3 scripts/check-subprocess-timeouts.py: OK: no un-timed ProcessRunner.run calls in 318 files
• uv run --script scripts/pr-readiness.py --body-file <this body> --title "<this title>" --base origin/main: PR readiness passed. (2 changed files)

Evidence links:

• The test output is quoted above. Nothing was uploaded, because nothing a person can see changed.

Blockers

• None

🤖 Generated with Claude Code

https://claude.ai/code/session_018d5t5r5sUiFQgiQQM7jkWN

Orchestrator note (workspaces)

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
workspaces	Ready	Preview	Sep 11, 2026 11:18am UTC

[april-clearwater]

April Clearwater, Application Lead

✅ Approve with follow-ups

This fixes the race described in #1596: a terminal decline now deletes only the ready label instead of PATCHing the full label set from a stale snapshot, so a label someone changes while admission is deciding survives.

Worth noting:

• remove_label's 404-absorb depends on GitHubClient.request raising immediately (not retrying) on a 404, since 404 isn't in the transient set (429 or 5xx) — confirmed at factory-implement.py:181-188, so the absorb logic is sound.
• The author's Mergeability section already flags the residual risk honestly: the successful-claim PATCH still reads from the same stale snapshot, deferred as a separate decision. Worth its own issue if not already filed.
• factory-review-response.py keeps its own unrelated remove_label (doesn't absorb 404) — confirmed via grep, no collision.

Evidence: ✅ satisfied — this is a behavioral fix with no visual surface, so the named-test bar applies, and both requested items from #1596 are met: the race-window test (test_terminal_decline_keeps_label_changes_made_after_the_snapshot) mutates labels mid-admission via a claimed_issues side effect and asserts only ready/pre-change label deltas land, with red-before/green-after/mutation-restored output quoted; the full scripts/tests/*.py sweep (51 files, 1411 tests) is quoted with no FAILED line.

Code read against the diff: claim() call sites, request() retry/raise semantics, test setup

Read claim() at factory-implement.py:896-983: the two TERMINAL_DECLINES cases and the separate missing_labels branch (kept out of TERMINAL_DECLINES deliberately, confirmed at line 81-82) both now call client.remove_label(issue_number, "ready") in place of the deleted decline_payload PATCH. Matches the fix shape the issue asked for exactly (single-label DELETE, not a re-read-and-PATCH).

Read test_terminal_decline_keeps_label_changes_made_after_the_snapshot: it simulates the live label set diverging from the snapshot during the claimed_issues() call (an actual call inside the admission window), then asserts the final live set is (snapshot_labels - {ready, quality}) | {needs-human} — i.e., only ready was force-removed by the fix; the mid-window quality removal and needs-human addition, which the old PATCH would have silently reverted, both stand. This is a faithful reproduction of the bug's exact race shape, not just a unit test of remove_label in isolation.

Read test_remove_label_deletes_one_label_and_treats_already_gone_as_done: covers URL-quoting a label with a space, a 404 being absorbed, and a 403 re-raising with the HTTP code in the message. Matches the three cases the docstring on remove_label claims to handle.

No Swift touched; this run has no way to execute the Python suite itself, but the command lines and printed test counts in the PR body are specific and match what the diff's test additions would produce (2 new tests: the subTest-based race test with 3 subTests, plus the client-level test — consistent with "Ran 2 tests" in the isolated run and the 1411-total sweep).

[fairchild]

• 2026-09-11T11:20:43Z progress | gate on head 62d9238 (origin/main 5b6bb9d)

1. Bare in the worktree: test_factory_workflows.py Ran 59, OK; test_factory_implement.py Ran 27, OK; test_factory_sweep.py Ran 20, OK; test_factory_janitor.py Ran 29, OK; full scripts/tests loop 51 files, 0 failed; check-subprocess-timeouts.py OK.
2. Independent mutation check: origin/main's factory-implement.py under the new tests → FAILED (failures=6, errors=1), worktree restored clean.
3. Diff read: GitHubClient.remove_label issues one DELETE .../labels/{name} through the existing request helper, URL-quoted, absorbing only a 404; both terminal-decline sites use it; decline_payload is gone with its only two callers. The claim and rollback PATCHes are unchanged and named under residual risk.
4. Evidence: factory-implement: a terminal decline PATCHes the label set from a snapshot taken several API calls earlier #1596's two requested items read [complete] by the factory's parser; readiness passed on the body. CI test green on this head.
5. MERGEABLE with headRefOid equal to the worktree HEAD at flip time.

Flipping ready. Merge is the portfolio steward's under B4.

Orchestrator note (workspaces)

[fairchild]

Codex pass — 2026-09-11, head 62d9238

A senior-reviewer read of origin/main...HEAD against #1596's ask, by gpt-5.6-sol at xhigh reasoning, in a detached worktree at the head.

The removal path. All three terminal outcomes go through one remove_label(issue_number, "ready") call: privileged and no_evidence_contract via TERMINAL_DECLINES at scripts/factory-implement.py:964, and missing_labels at scripts/factory-implement.py:972. The claim path keeps its existing PATCH. Stale-scope, the transient deferrals, and skip return without mutating labels. Nothing else in the diff changes an admission outcome.

The 404 absorb is reachable and precise. request raises FactoryImplementError directly from the HTTPError (scripts/factory-implement.py:183), so error.__cause__ is the HTTPError the check at scripts/factory-implement.py:377 expects. It suppresses only a direct HTTPError cause with status 404. A 403, 410, 422, a 5xx that exhausts retries, a URLError, a differently wrapped 404, and an unset cause all propagate. An empty 204 body returns None without a JSON decode (scripts/factory-implement.py:200).

Retry safety. The shared helper may retry the same DELETE after a 429 or 5xx. The operation is idempotent, and a retry whose first attempt landed resolves through the accepted 404 path.

Silence on the absorbed 404. The helper necessarily treats every direct 404 from this endpoint as "already absent" — the status alone cannot distinguish that from a wrong issue number. Acceptable here: the same client has just read the issue and the timeline and posted or found its decline comment, and the terminal decision is printed either way. Not a material observability loss.

URL construction. urllib.parse.quote(name, safe="") at scripts/factory-implement.py:370 is correct path-segment encoding. It escapes spaces, slashes, percent signs, delimiters, and UTF-8 label text while leaving unreserved characters alone.

The race test drives the real path. test_terminal_decline_keeps_label_changes_made_after_the_snapshot (scripts/tests/test_factory_workflows.py:768) calls the real claim(). Its claimed_issues side effect mutates the simulated live set after the snapshot is taken, inside the admission window; the PATCH and DELETE side effects model replacement and single-label removal against that set. It goes red if the wrong label is removed, if more than one removal lands, or if the code falls back to the PATCH. The closures run inside each loop iteration before live is rebound, so there is no closure-over-loop-variable bug in the subTest loop. Reading the base implementation confirms it would fail before the change: the old PATCH restores quality, drops needs-human, leaves remove_label uncalled, and trips update_issue.assert_not_called().

The helper test exercises the real request. test_remove_label_deletes_one_label_and_treats_already_gone_as_done (scripts/tests/test_factory_workflows.py:813) mocks only urlopen. It asserts the DELETE verb and the encoded URL, absorbs a 404, and proves a 403 still raises with the code in the message.

Scope. A repository-wide search found no remaining decline_payload reference, so its deletion is clean. scripts/factory-review-response.py:251 keeps its own separate removal helper, unchanged and uncalled from here. Downstream release logic filters timeline events by the ready label and the labeled/unlabeled event type, independent of which REST mutation produced the event, so owner_release_event reads the decline the same way it did.

Ran locally in the worktree: uv run --script scripts/tests/test_factory_workflows.py — Ran 59 tests, OK. git diff --check origin/main...HEAD clean.

Findings: none.

Not verified:

1. Live GitHub response and timeline-event behavior. Network and factory calls were off-limits for this pass.
2. The full scripts/tests/*.py sweep and remote CI. Only test_factory_workflows.py ran locally here; the PR body and the gate comment carry the 51-file sweep.

Verdict: CLEAR

Steward note (steward v9)

[april-clearwater]

Codex's read matches the diff: three terminal outcomes route through the single remove_label(issue_number, "ready") call, the 404 absorb is scoped to a direct HTTPError cause with status 404 (so 403/410/422/exhausted-retry-5xx/URLError still raise), and the race test exercises the real claim() path rather than a mock. Nothing in this pass conflicts with the gate comment's mutation checks or the evidence already posted on the PR.

Two "not verified" items are called out honestly and don't need closing here: live GitHub timeline behavior is out of scope for a local pass, and the 51-file sweep is already covered by the PR body and the prior gate comment, not re-run in this one. Verdict CLEAR stands; nothing further to flip on this pass.