Skip to content

fix(tests): the suffix guard follows the policy to its second owner - #4468

Closed
qingyun-wu wants to merge 1 commit into
mainfrom
fix/suffix-guard-follows-the-policy
Closed

qingyun-wu wants to merge 1 commit into
mainfrom
fix/suffix-guard-follows-the-policy

Conversation

@qingyun-wu

Copy link
Copy Markdown
Collaborator

main is red, and this is why

tests/worker-delivery-matches-the-hook.test.py asserts the Stop hook still defines sentinel_task_id(). #4437 deliberately removed it when the hook started shelling out to src/delivery/task_dispatch.py. Measured on origin/main (f7a553eab) just now, unmodified checkout:

$ python3 tests/worker-delivery-matches-the-hook.test.py
  FAIL the hook still defines sentinel_task_id()
FAILED: the hook still defines sentinel_task_id()

Two consecutive main CI runs are red on it (f7a553eab, 82d3b7093), so every PR whose python standalone tests job runs inherits the failure — that is the red on #4466 as well, which is otherwise clean (@john-the-dev reached the same conclusion from the import graph; this is the measurement).

The duplication did not disappear, it moved

The suite exists because the suffix set is spelled in more than one place and "the copy nobody re-reads is the one that hands a worker's task back to the core". After #4437 the hook holds no copy — but a new one appeared:

owner spelling
skills/worker-pool/scripts/worker_delivery.py SENTINEL_SUFFIXES (from pool_delivery)
src/delivery/task_dispatch.py _WORKER_HOLD_SUFFIXES = (".txt", ".accepted", ".claimed")

They agree today (['.accepted', '.claimed', '.txt'] both). They cannot share the constant: src/ must not import an optional skill (CLAUDE.md, "Optional capability discovery stays at the adapter edge"), so this suite is the only thing keeping them in step — and it was pinned to the hook, which no longer participates.

What the guard asserts now

  • the hook spells no sentinel suffix of its own (it delegates);
  • the hook asks task_dispatch.py owned-by for ownership;
  • the two module spellings are the same set.

Mutation evidence

Each assertion mutated separately, reverted by reverse replace:

mutation result
add case "$1" in *.accepted) :;; esac to the hook FAIL the hook spells no sentinel suffix of its own (found ['.accepted'])
break the task_dispatch.py owned-by call FAIL the hook asks task_dispatch.py owned-by for ownership
drop ".claimed" from _WORKER_HOLD_SUFFIXES FAIL task_dispatch ['.accepted', '.txt'] == worker_delivery ['.accepted', '.claimed', '.txt']

A first, looser form of the delegation check ("task_dispatch.py" in hook) survived its mutation, because the hook names the module three times — it is anchored on the owned-by call for that reason, and the table above is the re-run.

After: all checks passed, 23 checks, same file. review-checks.sh --diff PASS; lint-workspace-resolution.sh --diff clean.

Test-only; no production behaviour changes. Single concern, no bundled refactor.

🤖 Generated with Claude Code

main is red: `tests/worker-delivery-matches-the-hook.test.py` asserts the Stop
hook still defines `sentinel_task_id()`, which #4437 deliberately removed when
the hook started shelling out to `src/delivery/task_dispatch.py`. Two
consecutive main runs fail on it (f7a553e, 82d3b70), so every PR whose
`python standalone tests` job runs inherits the red.

The duplication the suite guards did not disappear — it moved. The suffix set
now has two live spellings: `worker_delivery.SENTINEL_SUFFIXES` (from
`pool_delivery`) and `task_dispatch._WORKER_HOLD_SUFFIXES = (".txt",
".accepted", ".claimed")`. `src/` must not import an optional skill, so they
cannot share the constant and this suite is the only thing keeping them in
step. Pinning the hook instead of the module left the real pair unguarded.

The guard now asserts what is true of the shipped design:
  - the hook spells no sentinel suffix of its own (it delegates),
  - the hook asks `task_dispatch.py owned-by` for ownership,
  - the two module spellings are the same set.

Mutation-verified, each assertion separately and reverted by reverse replace:
  - add `*.accepted)` to the hook  -> FAIL "spells no sentinel suffix (found ['.accepted'])"
  - break `task_dispatch.py owned-by` -> FAIL "asks task_dispatch.py owned-by"
  - drop ".claimed" from `_WORKER_HOLD_SUFFIXES` -> FAIL "['.accepted', '.txt'] == [...]"
A first, looser form of the delegation check (`"task_dispatch.py" in hook`)
survived its mutation because the hook names the module three times; it is
anchored on the `owned-by` call for that reason.

Before: `FAILED: the hook still defines sentinel_task_id()` at f7a553e.
After:  `all checks passed`, 23 checks, same file.

Test-only; no production behaviour changes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Closing in favour of #4467, which was opened 80 seconds earlier (18:47:47Z vs 18:49:07Z) and fixes the same red — first PR opened wins the claim.

Verified #4467 green at 5576095d and approved it there. The one thing this branch had that #4467 does not is the module-vs-module pin: dropping ".claimed" from task_dispatch._WORKER_HOLD_SUFFIXES leaves #4467's suite fully green, so the two live spellings (worker_delivery.SENTINEL_SUFFIXES and _WORKER_HOLD_SUFFIXES) are unguarded. I have named it on #4467 as a non-blocking follow-up and will open it separately once that lands, so the RC is not held up.

Branch kept for that follow-up.

@qingyun-wu qingyun-wu closed this Sep 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Gate

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

Diff Coverage

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

No lines with coverage information in this diff.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant