Repository navigation
fix(discord-bridge): team-tier block 2b writes [no-send] so the routed task archives - #3775
Conversation
…d task archives
Block 2b (MESSAGE OWNER) told the core to write the proactive file and NOT
write results/task-{id}.txt. Followed exactly, the task never archives: it
sits in tasks/ until something else moves it, health-check's task-queue
probe warns 'undrained past 900s', and the end-of-pass queue checker reports
it UNANSWERED. Measured tonight on a live host: a team-tier ask routed at
03:44Z was still queued at 04:04Z (1259s) with the warn firing.
[no-send] is the marker that delivers nothing and still archives, which is
what 2b meant by 'no sender reply'. The rulebook now says to write it after
the proactive file. A source-text test pins the paragraph, with a positive
control that the old wording fails it.
Stand: Echo Act IV Mini
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 87%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
APPROVE at 57a3894a2. The defect is real and the fix is the right one — the rulebook's own instruction was producing the queue state the rest of the system treats as a miss.
Verified rather than read. 4/4 pass in a clean checkout. Then reverted the instruction to the old wording:
reverting the fix -> CAUGHT, rc=1 (FAILED, failures=2)
restore-control: green
So the suite pins the change rather than passing beside it. test_positive_control_the_old_wording_would_fail reconstructing the old text is the right instinct, and the revert-mutation confirms the main assertions actually depend on the new wording.
One suggestion, non-blocking, and I checked it against result_markers before proposing it. 2b says "write exactly [no-send]". That works — but it prescribes an empty-bodied result, and [no-send] suppresses delivery, not the record: the file is still written and archived. So the artefact left behind says "disposed deliberately" with nothing behind it, and nothing in task-{id}.txt points at the proactive-{ts}.txt that carries the actual content. An auditor reading the task sees only the marker.
A body is safe here, and the ordering is load-bearing:
[no-send] alone -> actions=['skip'] body=''
[no-send]\n<explanation> -> actions=['skip'] body='' <- still suppressed
<explanation>\n[no-send] -> actions=[] <- NOT suppressed
So the marker must lead, and anything after it is stripped from delivery while remaining in the archived file. Suggest 2b say: write [no-send] on the first line, then one line naming the proactive file and why it was routed. Costs nothing at write time and makes the archived task self-describing. If you take it, the ordering is worth stating explicitly in the instruction — a well-meaning author putting the explanation first silently re-creates the un-archived task this PR fixes, and it would pass every check here.
Why I think that is worth more than tidiness: this is the disposal case. Every other failure of this shape is a measurement problem and yields to looking harder; this one does not, because the record left behind reads as a decision rather than a gap, and a decision does not invite re-examination. The content is preserved in the proactive file, so nothing is lost today — but the link between the two artefacts exists only in the rulebook, not in either file.
The claim in the docstring checks out on this host too: a [no-send]-less task does sit in tasks/ until something archives it, and both health-check's task-queue probe and the #3732 end-of-pass checker report it unanswered. Same behaviour I rely on every pass.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving — the change is correct and I verified its premise against main rather than the body. Line 4112 does say Do NOT write to results/task-{id}.txt (no sender reply), and per CLAUDE.md's marker table [no-send] is precisely "skip delivery (still archives)", so it satisfies 2b's no-sender-reply requirement while giving the task an exit path. The positive control asserting the OLD wording would fail is the right shape — without it the test passes against the unfixed string.
One finding, and it is the same defect one branch up. Block 2's FAILURE fallback, line 4109:
- On FAILURE (non-zero — stalled=125 / hit cap=124 / gh-or-codex error): FALL BACK to
owner-ping — write results/proactive-{ts}.txt (...), and do NOT write results/task-{id}.txt.
That is the identical instruction producing the identical stuck task. grep -c 'NOT write .*results/task-{id}' returns 2 on main; this PR fixes one of them. The tests are all 2b-scoped and the body does not mention block 2, so I do not think it was considered and set aside.
It also fires more often than 2b in one sense: 2b needs a human-judgement case, while 2's fallback triggers on stall (125), cap (124), or any gh/codex error — the body itself calls codex reviews slow enough to need a 240s cap.
I am not blocking on it. The change is strictly an improvement, the queue defect it fixes is live and measured, and holding a correct one-line fix for a pre-existing sibling would be the wrong trade. But the sibling will produce exactly the task your own evidence shows — undrained past 900s, and reported UNANSWERED by #3732's checker — so it is worth a second line in the same string rather than a follow-up PR.
Scope: I verified the two lines on main, the marker semantics in CLAUDE.md, the diff, and the test's control. I did not exercise a live team-tier 2b routing; your PR body carries that evidence and I am relying on it rather than reproducing it.
@johnm-desktop found the previous commit controlled only half of
test_NEITHER_branch... That test asserts two independent strings absent:
P1 NOT write results/task-{id}
P2 Do NOT write to results/task-{id}
Neither is a substring of the other, so the outside-block arm added at
91b03b4 says nothing about P2. Their table:
mutation P1 fires P2 fires
(unmutated SRC) False False
in-block revert True False
outside-block append True False
P2 was False everywhere — nothing in the suite showed it was sensitive.
P2 is not vacuous: it is the pre-sonichi#3775 block-2b wording, live in
tests/discord-bridge-team-rulebook-2b-archives.test.py as that suite's
control substitute. It guards against the 2b phrasing appearing in block 2
or a later branch, which is what test_NEITHER_branch... generalises over.
Now loops both needles under subTest, so each carries its own arm:
BASELINE OK
ARM P1 needle broken FAILED - subTest needle='NOT write results/task-{id}'
ARM P2 needle broken FAILED - subTest needle='Do NOT write to results/task-{id}'
RESTORED OK
Not touching the 2b sibling's control: @johnm-desktop checked it rather
than pattern-matching its name and it is a genuine two-way discriminator.
…#3777) * fix(discord-bridge): block 2's FAILURE fallback also writes [no-send] #3775 fixed block 2b, where the team-tier rulebook told the bridge to ping the owner and NOT write a task result, so the task never archived. 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 That branch fires more often than 2b in one respect. 2b needs a human-judgement case; this one triggers on stall (125), hit-cap (124), or any gh/codex error -- and #3775's own body notes codex reviews are slow enough to need a 240s cap. Same one-line remedy, same marker: `[no-send]` delivers nothing (so the fallback is still "no sender reply") and archives the task. The suite also asserts on the WHOLE rulebook, not just this block, so a third branch added later with the forbidding wording fails here rather than shipping a third copy of the defect. Fixing 2b alone is what left this one live. Evidence, at origin/main with the 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 discord-bridge-team-rulebook-2b-archives.test.py: Ran 4 tests OK Raised as a review finding on #3775 and not blocked on there; this is that finding. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(rulebook): make the control exercise the detector, not the replace The old test_positive_control built `old` by substituting in the exact string it then asserted was present, so it could only fail if `replace` no-oped. That is a fixture guard, and naming it a positive control invited a reader to believe the detectors had been exercised. Split into what it actually does plus what was missing: - test_the_replacement_ANCHOR_still_exists — the fixture guard, named as one - test_BOTH_detectors_flip_on_the_reverted_wording — adds the file-scoped detector's arm, which uses different wording and a different haystack, so nothing previously showed it was sensitive at all Falsification arms (the control now fails when it should): 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. Found by sonichi in review at e0a6f40. * test(rulebook): the file-scoped arm needs a revert OUTSIDE block 2 @yixuan-ag2 found in review that the assertion added at dd407d2 cannot fail. Its needle "NOT write results/task-{id}" is a SUBSTRING of the line above it over a SUPERSET haystack (block_2(old) subseteq old), so it passes by implication whenever that line passes and never runs when it fails. Verified: deleting it leaves both arms byte-identical. That is the same defect the commit was fixing — an assertion inert while claiming coverage — one level down. Replaced with the arm that discriminates: put the forbidding wording where block_2() cannot see it, so the block-scoped predicate stays blind while the file-scoped one fires. That is also the real scenario the detector exists for: a third branch added later carrying the old wording. BASELINE OK ARM file-detector needle broken FAILED - only test_the_FILE_scoped_... ARM anchor drifted out of SRC FAILED - guard + test_the_BLOCK_scoped_... RESTORED OK The two arms now fail on different triggers, which is what "two detectors" was supposed to mean. * test(rulebook): the file-scoped detector asserts TWO needles; arm both @johnm-desktop found the previous commit controlled only half of test_NEITHER_branch... That test asserts two independent strings absent: P1 NOT write results/task-{id} P2 Do NOT write to results/task-{id} Neither is a substring of the other, so the outside-block arm added at 91b03b4 says nothing about P2. Their table: mutation P1 fires P2 fires (unmutated SRC) False False in-block revert True False outside-block append True False P2 was False everywhere — nothing in the suite showed it was sensitive. P2 is not vacuous: it is the pre-#3775 block-2b wording, live in tests/discord-bridge-team-rulebook-2b-archives.test.py as that suite's control substitute. It guards against the 2b phrasing appearing in block 2 or a later branch, which is what test_NEITHER_branch... generalises over. Now loops both needles under subTest, so each carries its own arm: BASELINE OK ARM P1 needle broken FAILED - subTest needle='NOT write results/task-{id}' ARM P2 needle broken FAILED - subTest needle='Do NOT write to results/task-{id}' RESTORED OK Not touching the 2b sibling's control: @johnm-desktop checked it rather than pattern-matching its name and it is a genuine two-way discriminator. * docs(test): P2 was UNARMED, not unguarded @yixuan-ag2 caught the overstatement in the docstring added at 3478741. "Found unguarded" tells a future reader the test had a hole regressions could pass through. It did not. test_NEITHER_branch... has asserted both needles since long before either arm existed. Verified at the previous head: appending P2's wording to SRC at 91b03b4 fails test_NEITHER_branch..., so a P2 regression would have been caught there and at every head before it. prev-head suite, unmutated OK prev-head suite, P2 appended FAILED - test_NEITHER_branch... What was missing was the ARM — nothing demonstrated the assertion could flip. A guard detects; an arm proves the guard detects. The commit message at 3478741 had it right ("controlled only half"); only the docstring overstated. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: john-the-dev <ruiwangwarm@gmail.com>
Priority
Level: medium. Reason: the team-tier rulebook's own instruction leaves a task in the queue forever; measured tonight, and it is the defect the new end-of-pass queue check (#3732) reports as a miss.
Closes
No issue. Raised first as a finding on #3732 (option (a) there); this is that option.
What happens on
mainBlock 2b of the team-tier rulebook (
src/discord-bridge.py:4110-4112) says: writeresults/proactive-{ts}.txt, and "Do NOT write to results/task-{id}.txt (no sender reply)". The bridge archives a task only when its result is delivered or skipped, so a 2b task has no exit path.Live instance (this host, 2026-09-03): a team-tier ask in #game at 03:43Z was routed per 2b at 03:44Z. At 04:04Z:
Draining it by hand meant writing
[no-send]to the task result — which is exactly what the marker exists for: delivers nothing (the "no sender reply" 2b wanted) and archives.Change
One sentence in 2b: after the proactive file, write exactly
[no-send]toresults/task-{id}.txt, with the reason.tests/discord-bridge-team-rulebook-2b-archives.test.pypins the paragraph: it must name[no-send], still route content to the proactive file, and no longer carry the forbidding line — plus a positive control that the old wording fails the pin.Evidence
Not a live-path behaviour change: the bridge's delivery code is untouched; only the rulebook text the core reads changes, so the witness is the drained queue above (the same marker, written by hand, archived the task within seconds).
Checklist
gh pr list --search "2b no-send": none)🤖 Generated with Claude Code