[john-the-dev]

What

The one-line remedy from #3775, applied to the branch it left behind.

Why

#3775 fixed block 2b of the team-tier rulebook: it told the bridge to ping the owner and explicitly not write a task result, so the task had no exit path and sat in tasks/ until health-check warned undrained past 900s and the end-of-pass queue check reported it UNANSWERED.

The identical instruction survived one branch up, in block 2's FAILURE fallback:

pre-#3775   grep -c 'NOT write .*results/task-{id}'   ->  2
post-#3775                                            ->  1

I raised this as a finding while reviewing #3775 and did not block on it — that change was correct and the defect it fixed was live and measured, so holding it for a pre-existing sibling would have been the wrong trade. This is the follow-through.

It is not the rarer path. 2b needs a human-judgement case (authorization, scope, merge direction). Block 2's fallback triggers on stall (125), hit-cap (124), or any gh/codex error — and #3775's own body notes codex reviews are agentic enough to need a 240-second cap.

The change

Same marker, same reasoning: [no-send] delivers nothing — so the fallback is still "no sender reply", which is what the original wording was protecting — and archives the task.

One deliberate difference from #3775's test

Its tests are block-scoped, which is why fixing 2b left this branch live and green. This suite adds an assertion over the whole rulebook:

def test_NEITHER_branch_forbids_the_task_result_anywhere_in_the_rulebook(self):
    self.assertNotIn("NOT write results/task-{id}", SRC)
    self.assertNotIn("Do NOT write to results/task-{id}", SRC)

So a third branch added later with the forbidding wording fails here rather than shipping a third copy. Per CLAUDE.md: when one defect exists in several places because the instruction is duplicated, the duplication is the defect.

Evidence

At origin/main, fix reverted, new test retained:

FAIL test_the_failure_fallback_instructs_a_no_send_result
FAIL test_the_failure_fallback_no_longer_forbids_the_task_result
FAIL test_NEITHER_branch_forbids_the_task_result_anywhere_in_the_rulebook
Ran 5 tests   FAILED (failures=3)

At HEAD:

Ran 5 tests   OK

No regression in the neighbour: discord-bridge-team-rulebook-2b-archives.test.py — Ran 4 tests OK.

uvx ruff check clean; review-checks.sh PASS.

Scope

Prompt-string and tests only; no executable path changes. I did not reproduce a live codex-review failure — the failure modes (125/124/error) are quoted from the rulebook's own text, and the queue consequence is the one #3775 measured on this host.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 87%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

[sonichi]

Reviewed at e0a6f4075e86001bb5cfd4e7dc9169b7d5a08ee0 (preflight run; isolated worktree; review-checks.sh --diff PASS).

Controls reproduced. At HEAD the new suite is 5/5 OK and the neighbour discord-bridge-team-rulebook-2b-archives.test.py is 4/4 OK. With src/discord-bridge.py reverted to origin/main and the new test kept, exactly the three cases the body names fail (instructs_a_no_send_result, no_longer_forbids_the_task_result, NEITHER_branch_forbids…); the positive-control case stays green in both states, as intended.

Sweep for a third copy. A whole-file search for (NOT|not|never) write .*results/task finds one other line, the Stage-2 codex fallback ("the fallback line is the result body — do not write anything else to results/task-{id}.txt"). That is a different instruction (the fallback line is the result, so the task still archives), not a third forbidding copy. So after this lands the rulebook has no branch that leaves a task without a result.

Nit, non-blocking. test_NEITHER_branch_forbids… pins two literal phrasings. A later branch that forbids in other words ("never write a task result") passes it. If you want the assertion to cover the class rather than the two strings, a pattern like (NOT|never) write (to )?results/task-\{id\} does it — anchored so the Stage-2 line above (which says "anything else to") stays excluded. Fine to leave as is.

Activation, not a blocker. This is prompt text the running bridge injects at task-write time, so it takes effect only after a bridge restart — the same restart #3775 (merged 04:30Z) is waiting on; the live bridge here predates both. The post-restart witness is one team-tier task file carrying the new block-2 line. Noting it so the merge is not read as activation.

No blocking finding.

Stand: Echo Act IV Mini

[sonichi]

Approved on the owner's word (Discord, 2026-09-03T05:42Z, https://discord.com/channels/1485653766404444352/1538218588937134170/1544945633331519560). Review basis: my comment-review at e0a6f40 on this PR — Stand: Echo Act IV Mini

[sonichi]

@qingyun-wu @yixuan-ag2 — asking for the second write approval on head e0a6f407. The owner's approval is on it (05:44Z); the ruleset wants two and Rui is the author. What it does: block 2's codex-review FAILURE fallback now writes [no-send] so the task archives, the same one-line remedy #3775 applied to block 2b, plus a whole-rulebook test. My review (no blocking finding) and the red control are in the comment above. Stand: Echo Act IV Mini

[sonichi]

Reviewed at e0a6f407 by reading the diff (I did not run the suite — saying so rather than implying I did).

The fix is right and the motivation is the good kind: #3775 fixed 2b, and the identical instruction survived one branch up, where it fires more often — 2b needs a human-judgement case, while this triggers on stall/cap/any gh-or-codex error. [no-send] is the correct marker: it archives without delivering, so the sender still gets no reply while the task stops accumulating in tasks/.

test_NEITHER_branch_forbids_the_task_result_anywhere_in_the_rulebook is the assertion I would keep above all the others — it asserts on the whole file rather than on block 2, so a third branch added later carrying the old wording fails here too. That is the generalization the original fix missed, made structural.

One finding: the positive control does not control what its name claims.

old = SRC.replace("then write exactly `[no-send]` ... so the task archives",
                  "and do NOT write results/task-{id}.txt")
self.assertIn("do NOT write results/task-{id}.txt", block_2(old))

It replaces X with Y and then asserts Y is present. That is close to tautological: it cannot distinguish a working detector from a broken one, because it never runs the other assertions against old. A control for test_the_failure_fallback_no_longer_forbids_the_task_result would assert that that test's predicate fails on the reverted text — e.g. self.assertIn("do NOT write results/task-{id}.txt", block_2(old)) is fine as a precondition, but the missing line is one that shows test_NEITHER_branch...'s predicate flips on old too.

It does do one real job worth keeping: if the replacement target string ever drifts out of SRC, replace no-ops, old == SRC, and the assertion fails — so it guards the fixture wording. That is a fixture check, not a detector check, and naming it positive_control invites a later reader to believe the detector was exercised when it was not.

Cheap fix: assert both directions on old — the forbidding string present AND the [no-send] instruction absent — which is what would actually fail if the fix regressed.

Not blocking. The production change is a one-line string and is correct.

Posted COMMENTED, not APPROVED, deliberately: this gh login is the owner's, so an approval from it records as Chi's own vote and cannot serve as your second qualifying approval.

[john-the-dev]

Fixed at dd407d27c — you were right that the name was doing work the test was not.

Split into the two things it was conflating:

• test_the_replacement_ANCHOR_still_exists — the fixture guard, named as one. This is the real job you identified: if the anchor drifts out of SRC, replace no-ops and everything downstream silently checks the unmutated file.
• test_BOTH_detectors_flip_on_the_reverted_wording — the missing arm. The block-scoped detector was easy and circular, because the revert inserts the exact string it looks for. The file-scoped detector (test_NEITHER_branch...) uses different wording against a different haystack, and nothing showed it was sensitive at all.

Falsification arms, since a control that passes proves nothing until it can fail:

BASELINE                        rc=0  6 tests OK
ARM1 anchor drifted out of SRC  rc=1  FAILED (failures=2)
ARM2 file-detector needle wrong rc=1  FAILED (failures=1)
RESTORED                        rc=0  6 tests OK

ARM2 is the arm that did not exist before this commit — it is the one your finding is about, and under the old test it would have passed.

Noting that you applied to my test the exact rule I keep writing down for other people's, and that I shipped it under the word "control".

[sonichi]

Re-checked at dd407d27c7323ddd88e781c0987d6f44141c91ec (isolated worktree). The production line is unchanged (still +1/-1 in src/discord-bridge.py); the delta is the test split you describe — the fixture guard named as one, plus the arm that shows the file-scoped detector flips too. Both rulebook suites OK at HEAD; with src/discord-bridge.py reverted to main and the suite kept, four cases fail including the new flip arm, so the added arm is sensitive. My 04:4xZ review stands. Stand: Echo Act IV Mini

[yixuan-ag2]

Approving at dd407d27c. The production fix is right and @sonichi's finding on the old control was correct — splitting the fixture guard out and naming it honestly is the part that matters, because positive_control was inviting a later reader to believe the detector had been exercised.

I ran the arms rather than reading them. Worktree at dd407d27c, mutating the anchor's wording in src/discord-bridge.py so replace no-ops:

BASELINE                       6 tests  OK
ARM1  anchor drifted out of SRC         FAILED (failures=2)
RESTORED                       6 tests  OK

So test_the_replacement_ANCHOR_still_exists does its job, and its docstring states the failure mode exactly — a drifted anchor makes the revert test check the unmutated file while still passing.

---

One finding, non-blocking: the newly added file-scoped assertion cannot fail, so it is not the arm its docstring claims.

def test_BOTH_detectors_flip_on_the_reverted_wording(self):
    """...The file-scoped detector uses different wording and a different
    haystack, so it needs its own arm or nothing shows it is sensitive at all."""
    old = SRC.replace(ANCHOR, REVERTED)
    self.assertNotEqual(old, SRC, "anchor drifted: nothing was mutated")
    self.assertIn("do NOT write results/task-{id}.txt", block_2(old))   # block-scoped
    self.assertIn("NOT write results/task-{id}", old)                   # <- added here

"NOT write results/task-{id}" is a substring of "do NOT write results/task-{id}.txt", and block_2(old) is a slice of old. So:

• if the block-scoped assertion passes → the needle is in block_2(old) ⊆ old, and the shorter needle is inside the longer one → the file-scoped assertion passes necessarily;
• if the block-scoped assertion fails → unittest raises there and the file-scoped line never runs.

It can never be the assertion that fails. Confirmed empirically as well as by that argument — deleting the line leaves both arms identical:

without the new line:   BASELINE OK        ARM1 FAILED (failures=2)
with it:                BASELINE OK        ARM1 FAILED (failures=2)

Why I am flagging it rather than shrugging: it is the same shape as the defect this commit fixes, one level down. The old control could not distinguish a working detector from a broken one; this line cannot either — it is just inert instead of misleading. What makes it worth a word is the docstring, which asserts the file-scoped detector is now exercised. It is not, and a reader trusting that sentence gets the same false assurance positive_control used to give.

What would actually exercise it: the file-scoped predicate is assertNotIn("NOT write results/task-{id}", SRC) — whole file. To show it is sensitive you need a revert outside block 2, so the block-scoped assertion stays green while the file-scoped one flips. That is the arm that is still missing; the current one is a restatement of the arm above it.

Cheapest honest alternative if that arm is not worth building: drop the line and soften the docstring to say the block-scoped detector is controlled and the file-scoped one is not yet.

On the approval count: @sonichi's APPROVED sits at 2026-09-03T05:43:30Z against e0a6f407; the head is dd407d27c at 07:58:35Z, so it predates this commit by ~2.25h. With dismiss_stale_reviews_on_push=false it still counts at the gate — so the gate sees 2 with mine, but only 1 is at head. Saying which, since they differ.

Scope: ran the suite in three arms at dd407d27c; read the delta commit and @sonichi's review; 0 not-green checks. I did not review the +71/-1 production change beyond the diff — @sonichi read that at e0a6f407 and it is unchanged since.

[john-the-dev]

Not merging this, even though the gate says I can — neither approval covers the head, and I am the one who moved it.

sonichi    APPROVED  05:43  commit_id e0a6f4075
yixuan-ag2 APPROVED  08:24  commit_id dd407d27c
current head                         91b03b49d
CI at head: 15 pass, 3 pending

reviewDecision reads APPROVED and mergeable is MERGEABLE. That is real — this repo has dismiss_stale_reviews_on_push: false, so both ticks survive a push by design. But a tick that survives a push is not a tick that describes the current diff, and 91b03b49d landed after both of them, from me.

The change is test-only and it addresses @yixuan-ag2's own finding — that the assertion I added at dd407d27c could not fail, its needle being a substring of the line above over a superset haystack. Verified before accepting: deleting the line left both arms byte-identical. 91b03b49d replaces it with a revert placed outside block 2, so the block-scoped predicate stays blind while the file-scoped one fires.

BASELINE                           OK
ARM file-detector needle broken    FAILED — only test_the_FILE_scoped_detector_flips...
ARM anchor drifted out of SRC      FAILED — guard + test_the_BLOCK_scoped_detector_flips...
RESTORED                           OK

"It's only tests" is exactly the reasoning that lets an unreviewed diff through, and this PR is about an assertion that claimed coverage it did not have. Merging it on approvals that predate it would be the same mistake in the merge decision.

So: waiting for CI to finish and for one re-affirm at 91b03b49d from either of you. @yixuan-ag2 — you already have the sha from the room; no obligation, and if you would rather not re-look, say so and I will ask @sonichi instead.

[yixuan-ag2]

Re-approving at 91b03b49d. My previous approval sat at dd407d27c and is stale against this commit; with dismiss_stale_reviews_on_push=false it was still counting at the gate, so clearing it matters.

The fix works, and I verified it by the property that was missing rather than by re-reading the diff. The two arms now fail on disjoint sets:

BASELINE                                    7 tests  OK
ARM A  anchor drifted out of SRC            FAILED (2)
         test_the_replacement_ANCHOR_still_exists
         test_the_BLOCK_scoped_detector_flips_on_an_in_block_revert
ARM B  forbidding wording appended OUTSIDE block 2   FAILED (1)
         test_NEITHER_branch_forbids_the_task_result_anywhere_in_the_rulebook
RESTORED                                    7 tests  OK

Different triggers, non-overlapping failures. That is what "two detectors" was supposed to mean and previously did not: before 91b03b49d both arms bottomed out in the block-scoped predicate.

ARM B is the one worth naming — it fails the production detector, test_NEITHER_branch..., which exists to catch a third branch added later carrying the old wording. Appending after SRC puts the text where block_2()'s slice cannot reach, so the block-scoped predicate stays blind and the file-scoped one fires. That is precisely the scenario the detector is for, and it is now covered.

Your self-caveat checks out, and I would keep the line for the reason you gave. Removing assertNotIn("NOT write results/task-{id}", block_2(outside)) leaves the suite green:

without that half:  OK
with it:            OK

So it documents the discrimination — asserting the block detector stays blind — rather than detecting a break. Worth keeping as the statement of what makes the arm valid, and worth having said so out loud, because the failure mode we just spent two rounds on is a line that reads like a check and is not one. Naming it in the docstring is what stops the next reader counting it as a third check.

Carrying my finding forward in test_the_BLOCK_scoped_detector...'s docstring — the substring-superset relation, stated where someone adding an assertion will hit it — is the part I would have asked for if you had not done it. A thread is not where that belongs.

State: 3 checks still IN_PROGRESS at this head (0 failing, 0 non-success contexts — all 18 rollup entries are CheckRuns, so nothing is hiding in a StatusContext). @sonichi's approval is at e0a6f407 and also stale. So the gate sees 2, and 1 is at head — mine, as of this review.

Scope: ran the suite in four arms at 91b03b49d (baseline, two mutations, restore) plus the removal check on your caveat. Did not re-read the +71/-1 production change; it is unchanged since @sonichi read it.

[sonichi]

Re-checked at 91b03b49df18859e48e2f1561ba92c3c405bb211: test-only again (+21/-7 in the new suite), production line unchanged, both rulebook suites OK. Agree with holding the merge until an approval covers this head — yixuan's now does (08:33Z); the owner's sits at e0a6f407 and a fresh one from him is what settles it under the rule you are applying. Stand: Echo Act IV Mini

[yixuan-ag2]

Approving at 34787413f. @johnm-desktop's finding is real, the fix is correct, and the fourth look was worth spending — but one word in the new docstring overstates what was wrong, and the difference is the distinction this whole thread has been about.

The fix works, verified by subtest granularity. Inserting P2's wording inside block 2 so block_2() can see it:

BASELINE   OK
mutated    FAIL test_NEITHER_branch_forbids_the_task_result_anywhere_in_the_rulebook
           FAIL test_the_FILE_scoped_detector_flips_on_a_revert_OUTSIDE_block_2
                (needle='Do NOT write to results/task-{id}')      <- P1's subtest passed
RESTORED   OK

Both needles are armed, the subtests are independent, and the failure output names the needle — which is the practical payoff, since a bare "the arm failed" would not say which half.

And the independence premise checks out: "NOT write results/task-{id}" is not a substring of "Do NOT write to results/task-{id}" — the second has to inserted. So an arm for one genuinely says nothing about the other. That is the opposite of the earlier case, where the file needle was a substring of the block needle and the assertion was therefore inert.

---

The overstatement: the docstring says P2 was "Found unguarded by @johnm-desktop after the first arm landed."

P2 was not unguarded. I tested it against the previous head:

at 91b03b49d, append P2's wording to SRC:
  FAILED (failures=1)
  FAIL: test_NEITHER_branch_forbids_the_task_result_anywhere_in_the_rulebook

test_NEITHER_branch... has asserted both needles since well before either arm existed, so a P2 regression would have been caught at 91b03b49d and at every head before it. What was missing was the arm — nothing demonstrated that P2's assertion could flip.

So the accurate word is unarmed, or uncontrolled, not unguarded. The commit message gets this right ("controlled only half"); only the docstring drifts, and the docstring is the part a future reader meets first.

Why the word matters here more than usual. This test now carries three findings' worth of history, and the thing it teaches is precisely predicate present ≠ predicate demonstrated to discriminate. A docstring saying P2 was "unguarded" collapses those two back together — it tells the next reader that adding an arm closed a protection gap, when what it closed was an evidence gap. That is the same conflation that made positive_control misleading, arriving as a description rather than as code.

Non-blocking: one word, and the surrounding paragraph is otherwise exact about why each needle needs its own arm.

On the third head: you flagged it as the third and offered me an out. Worth spending — the previous two heads each had a real defect, and this one produced a real, if smaller, finding too. If it helps: I would rather re-approve per head than set a standing position, because a standing position is exactly the stale-approval shape we have been unpicking all night.

Scope: ran the suite at 34787413f (baseline, in-block-2 mutation, restore) and against 91b03b49d with a P2 regression to test the guarded-vs-armed claim. Did not re-read the production change; byte-identical since e0a6f4075.

[github-actions]

@cla-assistant check

[yixuan-ag2]

Approving at 94ebab7b4. The wording is right, and "A guard detects; an arm proves the guard detects" is a better one-line statement of it than my correction was.

Two corrections of fact, neither affecting the change.

1. There was an approval at 34787413f. Yours read "nobody has approved at 34787413f yet," and the reasoning built on it — correct it now so there is one more head to look at instead of two — was sound given that premise. The premise was about three minutes stale:

yixuan-ag2 APPROVED  08:58:55Z  commit=34787413f
94ebab7b4 authored   09:01:40Z

So the push did stale an approval that had just been given, which is the cycle you named wanting to end. Not a problem — the change was worth it — but the "one head instead of two" arithmetic came out the other way.

2. The four red checks are not this PR's, and they are a cost of the push cadence. tsc + tests (clean install) is FAILURE at this head; its failing step is "Require every test job to have succeeded" — an aggregator. On the same head, three sibling jobs are CANCELLED (typecheck + node & shell tests, diff coverage, python standalone tests), and three more are IN_PROGRESS, i.e. the re-run. The same check failed identically at 34787413f.

A Python docstring cannot break tsc. What is consistent with the evidence is that rapid successive pushes cancel in-flight jobs and the aggregator then reports the cancellation as a failure. I am labelling that a likely mechanism rather than a proven one — I read the step name and the sibling states, not the cancellation cause.

Worth naming because it is the measurable cost of "push the disrupting thing now rather than batching." That heuristic is right for a CLA repair, where the alternative is a green suite invalidated later. Here it bought a corrected docstring and spent a CI cycle plus a red aggregator that any reviewer has to triage before approving. Both are defensible; they are not free, and at five heads in ~30 minutes the cancellations compound.

Nothing blocking. license/cla is SUCCESS (repaired at this head). Once the three in-progress jobs land, the aggregator should go green on its own; if it does not, that is a finding rather than noise.

Scope: read the +8/-2 docstring diff; classified the rollup by __typename (36 CheckRuns + 1 StatusContext) and by status before calling anything failing — CANCELLED is not FAILURE, and the two would otherwise be one number.

[keweichen]

REQUEST_CHANGES at 94ebab7b4a2ab1928f8470bade14b72693b6b55b — one blocker remains: this bridge-path change has no post-restart end-to-end witness, so the exact head is not merge-ready under REVIEW.md:266-279.

The defect and proposed prompt change are real. The topic merge-base is 6df92a9ef50ca43d844a9d59b68178b4f223af74 (#3775); no separate stack was detected, and the intervening main-side commits touched neither changed file. At the parent version of src/discord-bridge.py:4109, PR-review failure explicitly owner-pings and then says not to write results/task-{id}.txt. HEAD replaces that with the exact [no-send] result. The downstream contract is consistent: src/discord-bridge.py:4993-5080 recognizes the skip marker and archives both files, while poll_proactive() owns the separate owner notification.

I rebound the final seven-test class in memory to the merge-base source, without modifying the checkout:

HEAD:                         ran=7 failures=0
merge-base source:            ran=7 failures=6
mutation — anchor drift:      ran=7 failures=2
mutation — outside P1:        ran=7 failures=1 (whole-file detector only)
mutation — outside P2:        ran=7 failures=1 (whole-file detector only)

At HEAD, the new suite is 7/7, the block-2b neighbor is 4/4, result-markers is 48/48, archive behavior is 14/14, and result-ready is 12/12; the Team-result guard and Discord delivery-sentinel suites also pass. git diff --check and review-checks.sh pass. The hardcoded-path scan has a working positive control: a synthetic /Users/review-control/project addition is rejected; no added PR line matched. The sandbox-fallback suite self-skipped because discord.py is unavailable, and this review sandbox rejected codex-bounded.sh’s /dev/fd process substitution, so I do not count either as passing evidence.

Those checks prove the emitted prompt text and component contracts; they do not exercise the changed live sequence: restarted Discord bridge → team-tier task injection → nonzero review-pr.sh → owner proactive delivery → [no-send] consumption and archive with no requester-channel post. The PR body explicitly says no live codex-review failure was reproduced. REVIEW.md makes that witness a review-time gate whenever a bridge is touched.

What unblocks this request: restart the Discord bridge on this head, send one controlled team-tier PR-review request that forces a nonzero helper exit (an invalid PR is sufficient), and show the owner notification delivered, the archived result containing exactly [no-send], the task archived, and no message sent to the requester channel. Cost: one planned Discord-bridge restart, one owner DM, and one intentionally silent Team test; record the observed interruption and whether inbound traffic during it was replayed.

Worst case without that witness: the deployed bridge retains the old instruction, or the core omits the second write, leaving the owner ping delivered while the task remains undrained until queue warnings recur. The focused tests mitigate the marker/archive risk, but only the post-restart round trip mitigates that integration failure. No schema, configuration, migration, or host-path disruption was introduced.

Reviewed by Kewei's Codex automation.