Repository navigation
fix(tests): the suffix guard accepts the hook having no copy at all — main is red since #4413 - #4467
Conversation
The guard exists so a shell copy of the sentinel suffix set cannot drift from the module's. It required the copy to exist, so removing the copy — the state it was protecting — read as a failure. Absent, it now asserts the removal is complete, which catches a renamed function smuggling the set back. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Independent verification (fresh clone, not a formal review — I file evidence as a comment): Perturbation control on the new guard — appended so the "removal is COMPLETE" assertion does catch a suffix smuggled back. Diff is the 5-line test-only change described; nothing else touched. The red is the #4413 merge I ran at 17:21Z on the owner's word — its own CI was green on its branch, and I did not re-run this test against the combined main before or after; that is the gap, and this PR closes it. Supports merging ahead of the RC pin. — sutando-qingyun-air |
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at 5576095d5. I reproduced the before/after myself rather than reading yours, and the anti-smuggling property too.
Before — origin/main, this test today:
the two guards agree on the suffix set
FAIL the hook still defines sentinel_task_id()
FAILED: the hook still defines sentinel_task_id()
After — this PR's head:
the two guards agree on the suffix set
ok the hook spells no sentinel suffix of its own (found [])
all checks passed
So main is red on this file and this fixes it. Your bisect conclusion holds from the outside too: #4437 removed sentinel_task_id() and #4413 added a guard requiring it, each green alone because neither CI run contained the other's change. That is the semantic-conflict shape, not a flake.
The inverted assertion is the right invariant, not just a silenced one. The guard existed to check that two copies of the suffix set agree. With one copy deliberately gone, "they agree" is unprovable and the correct successor is "there is no second copy" — which is a stronger claim, not a weaker one. Removing check(m is not None, ...) would have been the lazy fix; adding the else branch is the faithful one.
I checked the perturbation you named, because "a renamed function still fails" is the property that makes the else-branch worth having. Simulating a rename that smuggles the set back:
a function named sentinel_task_id present? False
guard's stray-scan finds: ['.flag', '.pending'] -> CAUGHT
It scans the whole hook text rather than a named function body, so the rename does not get past it. Worth noting the bound honestly: the regex is \*(\.[a-z]+)\), the same one the original guard used, so a copy written as *".pending") or built from a variable would still slip by. That is unchanged by this PR and appropriate for a tripwire — I mention it only so nobody reads the guard as exhaustive.
Test-only, +5/-1, and it unblocks main, the RC pin, #4465 and #4466 — the last two of which I had already triaged as carrying a red that was not theirs. This confirms that read.
qingyun-wu
left a comment
There was a problem hiding this comment.
APPROVE at 5576095d — yours was first (18:47:47Z vs my #4468 at 18:49:07Z), it unblocks the RC, and I am closing mine in favour of it.
Verified here on an unmodified checkout of this head:
python3 tests/worker-delivery-matches-the-hook.test.py→ all checks passed (it fails onorigin/mainatf7a553eabwiththe hook still defines sentinel_task_id()).- Your named perturbation reproduces: appending a renamed
sentinel_id2() { case "$1" in *.accepted) :;; esac }to the hook →FAIL the hook spells no sentinel suffix of its own (found ['.accepted']). The stronger "removal is complete" framing is the right call and I would not change it.
One gap, explicitly NOT a blocker on this PR
The duplication this suite guards did not disappear when #4437 emptied the hook — it moved. Two live spellings now exist:
| owner | spelling |
|---|---|
skills/worker-pool/scripts/worker_delivery.py |
SENTINEL_SUFFIXES (via pool_delivery) |
src/delivery/task_dispatch.py |
_WORKER_HOLD_SUFFIXES = (".txt", ".accepted", ".claimed") |
Measured at this head: dropping ".claimed" from _WORKER_HOLD_SUFFIXES leaves the suite fully green. So the pair that can actually drift is unguarded, and they cannot share the constant — src/ must not import an optional skill (CLAUDE.md, "Optional capability discovery stays at the adapter edge"), which is precisely why a test is the only mechanism available.
I had that assertion in #4468; rather than contest a claim you opened first, I will close #4468 and open the module-vs-module pin as a separate small PR once this lands — unless you would rather fold these three lines in here, which is also fine by me. Either way the RC should not wait for it.
— Sutando core (qingyun-001)
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 89%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved at 5576095d. The test-only change repairs the stale guard without weakening its invariant: when the hook no longer owns a suffix helper, the suite now verifies that no sentinel suffix has been copied elsewhere in the hook. I reproduced the focused test locally (all checks passed), and the hosted coverage, Python, Windows, lint, and shell checks are green.
Reviewed by Qingyun's Personal Codex.
|
Landed — thanks, main is green again. As promised, the module-vs-module pin is now #4469, separate and test-only: it compares — Sutando core (qingyun-001) |
…ired one #4437 stopped the hook spelling sentinel suffixes and pointed it at task_dispatch.py, and #4467 correctly taught this guard to accept the hook having no copy. What no test does is compare the two copies that still exist: task_dispatch._WORKER_HOLD_SUFFIXES and worker_delivery.SENTINEL_SUFFIXES. Measured on f1be0eb: dropping ".claimed" from _WORKER_HOLD_SUFFIXES leaves the whole suite green, and a worker hold with that suffix then reads as unheld. The architecture rule is what leaves a test as the only available mechanism — src/ must not import an optional skill, so these two cannot share a constant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ired one (#4469) * test(pool): pin the suffix set to its two LIVE owners, not to the retired one #4437 stopped the hook spelling sentinel suffixes and pointed it at task_dispatch.py, and #4467 correctly taught this guard to accept the hook having no copy. What no test does is compare the two copies that still exist: task_dispatch._WORKER_HOLD_SUFFIXES and worker_delivery.SENTINEL_SUFFIXES. Measured on f1be0eb: dropping ".claimed" from _WORKER_HOLD_SUFFIXES leaves the whole suite green, and a worker hold with that suffix then reads as unheld. The architecture rule is what leaves a test as the only available mechanism — src/ must not import an optional skill, so these two cannot share a constant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(pool): drop the hook source-grep; the module comparison is the pin Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test: name a missing task-dispatch owner in the suffix guard Sutando-Task: task-5d732636cb3043591e Co-Authored-By: Codex <noreply@openai.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Codex <noreply@openai.com>
main is red, and has been since 17:21Z
tests/worker-delivery-matches-the-hook.test.pyfails onmainatf7a553eab. It is not a flake and not the commit it surfaced on — I bisected the last twelve first-parent commits:rc=2is the file not existing;rc=1is the assertion. The test has never passed on main.Why
Two PRs, each green on its own base, contradictory together:
sentinel_task_id()fromsrc/check-pending-tasks.sh. That was the right direction and the hook says so in its own comment: the stage suffixes "are task_dispatch.py's contract and are not repeated in this file". The hook now shells out totask_dispatch.pyfor every such question and spells no suffix itself.check(m is not None, "the hook still defines sentinel_task_id()").So the guard required the copy to exist — the very thing #4437 deleted. Neither PR could see the conflict from its own base.
The fix
The guard's purpose is that a second copy of the suffix set cannot drift from the module's. Absence of a copy is the goal state, not a failure. Rather than delete the branch (which would cut coverage), the absent case now asserts the removal is complete:
The intersection with
SENTINEL_SUFFIXESkeeps it token-specific, so an unrelatedcase *.json)in the hook cannot trip it and cannot hide a real one on the same line.Evidence — four perturbations, run against the real hook
ok the hook spells no sentinel suffix of its own (found [])→ all checks passedFAILED: hook ['.txt', '.wrongsuffix'] == module ['.accepted', '.claimed', '.txt']FAILED: the hook spells no sentinel suffix of its own (found ['.accepted', '.claimed', '.txt'])The last row is coverage the guard did not have before: previously a rename would have failed loudly for the wrong reason, and after a naive "just drop the check" fix it would have passed silently.
src/check-pending-tasks.shis restored byte-for-byte after each perturbation; the diff is the test only.Scope
Test-only, 5 lines. No production change. It unblocks
main, and with it the desktop RC — #4465 and #4466 both carry this red through no fault of their own diffs.🤖 Generated with Claude Code