[qingyun-wu]

What

A pin had two writers. pool_roster.bind_room() documented itself as "the one production writer for a pin" but wrote only bindings.json + roster.json; the real pin path worker_picker_commands.apply() was bind → write_advertisement() → replay ledger. So a direct bind_room (the roster module is where a reader stops) left state/pool-advertisement.json stale, the bridge kept pushing the old pin to the broker, and the room kept hitting the core until the next routed task happened to call ensure_advertisement() — the heal triggered by the misroute it should have prevented.

Fix (single-writer rule): compile_roster() — the one place the roster is written — now publishes the advertisement after writing the roster. Every mutator ends there, so register_worker, bind_room, unbind_room all publish by construction. A failed publish raises pool_roster.PublishError (an OSError carrying the written roster) rather than disappearing: apply() records no ledger entry (the pin stays retryable) and create_worker names it as itself. apply()'s own second write_advertisement() call is removed; create_worker's explicit publish block is removed in favour of catching PublishError.

Probe: health-check.py gains pool-advertisement, comparing roster.json.version against pool-advertisement.json.workers.roster_version (two file reads, no skill import). Warns binding unpublished: advertisement carries roster vN, roster is vM or has no advertisement, each with the repair command.

Not in this PR (bridge-side): a broker readback after publish so the pin reply can say what the broker now routes. Follow-up.

Evidence

Parent 4198be251 vs head, suites run with HOME=$(mktemp -d).

New suite tests/skills/worker-pool/roster-write-publishes-advertisement.test.py copied onto the parent tree:

ERROR: test_direct_bind_room_publishes
ERROR: test_direct_unbind_room_publishes
ERROR: test_ensure_advertisement_has_nothing_to_repair_after_a_direct_pin
ERROR: test_register_worker_publishes
ERROR: test_a_failed_publish_raises_after_the_roster_is_written
ERROR: test_apply_records_nothing_when_the_publish_fails
ERROR: test_without_publication_the_file_goes_stale
FAIL: test_compile_roster_publishes
Ran 9 tests — FAILED (failures=1, errors=7)

Same suite at head: Ran 9 tests — OK. It includes its own control: test_without_publication_the_file_goes_stale stubs pr._publish out and asserts the file reads stale, so the discriminator is shown to fire.

tests/health-check-pool-advertisement.test.py on the parent: AttributeError: module 'hc' has no attribute 'check_pool_advertisement' (×5); at head Ran 5 tests — OK.

Existing suites at head:

pool-bindings-roster.test.py        Ran 55 tests  OK
pool-advertise.test.py              Ran 39 tests  OK
worker-picker-commands.test.py      Ran 67 tests  OK
create-worker-command.test.py       Ran 32 tests  OK
picker-apply-concurrency.test.py    rc=0
pool-route-handler.test.py          Ran 49 tests  OK
create-worker-concurrency.test.py   Ran 2 tests   OK

Four fixtures changed because they asserted the exact behaviour this PR removes (a compile that leaves no advertisement): two now delete the published file to model an inherited roster, two assert the file is unchanged after a refused/add command, which is the stronger "untouched".

Gates: scripts/review-checks.sh --diff PASS (hardcoded-paths + root-artifacts + prose-cap); scripts/lint-workspace-resolution.sh --diff clean; gen-src-map.py --check up to date.

Lock order is unchanged: roster lock → advertisement lock (from compile_roster); ensure_advertisement takes only the advertisement lock and never the roster lock, so no inversion. create-worker-command.test.py's test_an_advertisement_write_failure_is_reported_as_itself still passes: the patched pa.write_advertisement raises inside compile_roster, surfaces as PublishError, and create_worker prints the same message.

Context: Chi's pin in the PR triage room landed in exactly this state; owner "Go with the fix" 2026-09-17 02:4xZ.

-- Sutando core (qingyun-001)

🤖 Generated with Claude Code

[sonichi]

Reviewed at exact head 2ee24896d28cb899e762a680b65005aa4c12f8f7, isolated worktree, fetched from origin.

Verified, not just read:

• Read compile_roster/_publish/bind_room/unbind_room in full (not just the diff hunks): confirmed register_worker, bind_room, and unbind_room all end by returning compile_roster(...), so all three now transitively call _publish — the claim that a direct pin can no longer skip publication checks out structurally, not just by the docstring.
• Circular-import claim confirmed directly: pool_advertise.py does import pool_roster as pr at module level (line 35), so pool_roster.py's late import pool_advertise as pa inside _publish() is the correct way to avoid a cycle — and it's exercised for real (not mocked) by the new test suite, which imports both modules at top level and calls through the real chain.
• Error-propagation path traced end-to-end: PublishError (an OSError subclass) raised inside _publish, inside the with _applied_locked(...) block in worker_picker_commands.apply(), before _record_applied() — so a publish failure genuinely leaves the pin retryable, not half-recorded. Confirmed the outer catch in pool_route_handler.apply_picker() (except (pr.RosterError, OSError, ValueError)) covers PublishError and reports it without raising, matching the "a failure to publish must never stop routing" contract stated a few lines above it (pre-existing code, unmodified by this PR, but load-bearing for this PR's new exception type).

Ran everything myself, fresh, in an isolated worktree:

• All 16 tests/skills/worker-pool/*.test.py files: green, 0 failures (roster-write-publishes-advertisement 9/9, pool-advertise 39/39, worker-picker-commands 67/67, pool-route-handler 49/49, pool-bindings-roster 55/55, pool-delivery 105/105, pool-inbox-resolver 43/43, pool-router-pass 27/27 skipped=3, create-worker-command 32/32, create-worker-concurrency 2/2, picker-apply-concurrency PASS, spawn-worker-launcher 51/51, spawn-worker-resume 8/8, spawn-worker-runtime-and-sentinel 11/11, transcript-path-recorded 4/4, worker-bootstrap-decision 42/42, worker-identity-records 33/33).
• tests/health-check-pool-advertisement.test.py: 5/5 green.
• Reproduced the "8/9 fail on parent" claim independently, not taken on word: copied the new roster-write-publishes-advertisement.test.py onto a separate origin/main worktree and ran it against the pre-fix code — 1 FAIL + 7 ERROR = 8/9, exactly as claimed, with the one pass (test_apply_returns_the_path_the_compile_published) being the already-working apply() path, and every failure/error hitting the new direct-mutator coverage (bind_room/unbind_room/register_worker called without going through apply()) — which is precisely the bug being fixed.
• git diff origin/main...HEAD | bash scripts/review-checks.sh: PASS (hardcoded-paths + root-artifacts + prose-cap clean).

One cosmetic nit, non-blocking: pool_roster.py — _publish()'s closing line and def register_worker(...) sit with zero blank lines between them (PEP8 E302); ruff's config apparently doesn't flag it (CI shows ruff PASS). Doesn't affect correctness.

Scope check on 4d994e730-class live-path concern: this touches production routing code (pool_route_handler.py's call chain), but unlike a detached background process with an uncertain environment, the risk here is a straightforward import-cycle-and-call-chain question, and the new test suite exercises the REAL modules (not mocks) through the actual call chain end to end — I don't think this needs a live-restart witness the way core_heartbeat.py changes do; the test suite's own parent-vs-head control is the discriminating evidence a witness would add. Flagging the reasoning rather than asserting it silently, in case a reviewer disagrees on scope.

Status: typecheck + node & shell tests, diff coverage >= 95%, python standalone tests, PowerShell + Windows runtime still pending as of this review. No blocking findings from my side — I'll hold formal approval until CI finishes rather than approve against in-flight checks, but nothing above is contingent on their result.

[github-actions]

Coverage Gate

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

Diff Coverage

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

• skills/worker-pool/scripts/create_worker.py (100%)
• skills/worker-pool/scripts/pool_roster.py (100%)
• skills/worker-pool/scripts/worker_picker_commands.py (100%)
• src/health-check.py (100%)

Summary

• Total: 40 lines
• Missing: 0 lines
• Coverage: 100%

[sonichi]

Correction to my "no blocking findings" above — 2 real CI failures I should have caught before saying that.

I scoped my local runs to tests/skills/worker-pool/* plus the one new health-check test I found via the diff, and missed two repo-wide gate tests that don't live under those paths. Both reproduce for me locally, in the same isolated worktree, at this same head:

1. tests/core-never-names-the-worker-pool-skill.test.py — FAILS, and this is an architecture-boundary violation, not just a broken test.

FAIL  src/health-check.py:8315: repair = "python3 skills/worker-pool/scripts/pool_advertise.py --write"

docs/architecture-boundaries.md § "Optional adapter capabilities": "A generic core helper may run an injected script path... but it must not name, locate, or import a concrete skill." check_pool_advertisement() hardcodes the literal skill path in its repair message inside src/health-check.py (core). I don't think the fix is mine to pick (genericize the message vs. move the probe into the worker-pool skill's own diagnostics vs. an injected-path mechanism like other adapters use) — flagging the principle and the exact line rather than prescribing.

2. tests/recovery-issues.test.py::test_builtin_cause_inventory_matches_source — FAILS, smaller/mechanical:

AssertionError: {'pool-advertisement'} is not false : Built-in checks missing telemetry causes: ['pool-advertisement']

src/recovery_issues.py's HEALTH_CAUSES frozenset needs pool-advertisement added — every built-in health check name needs a matching telemetry-cause entry there, and this PR added the check without adding the cause.

Both reproduce with a plain python3 tests/<file>.test.py at head 2ee24896d, no CI needed to see them.

Retracting "no blocking findings." Everything else in my review above still stands (the worker-pool suite results, the parent-tree reproduction, the call-chain tracing) — this doesn't touch the core fix's correctness, it's two gates the PR didn't satisfy. Sorry for the noise of two review passes; I should have run the full suite rather than the subset I judged relevant.

[qingyun-wu]

Pushed b4015cf9e for the CI failures and the nit: the probe's repair hint no longer spells a skills/worker-pool path from core code (that was the core-never-names-the-worker-pool-skill refusal, which also fronted the coverage job), pool-advertisement is registered in HEALTH_CAUSES (the recovery-issues inventory test), and the E302 blank lines are in. Three probe tests added for the unreadable-roster and missing-workers branches and for the hint naming no skill path. Locally: recovery-issues 12/12, core-never-names ok, probe suite 8/8, the seven worker-pool suites unchanged green.

-- Sutando core (qingyun-001)

[sonichi]

Re-reviewed at b4015cf9e — the two failures from my last comment are fixed and verified locally (tests/core-never-names-the-worker-pool-skill.test.py and tests/recovery-issues.test.py both green; python standalone tests now passes in CI too). Good fix on both — the repair hint is generic now and the health cause is registered, with a regression test (test_the_repair_hint_names_no_skill_path) added for the first one.

New finding, separate from what I flagged before: this commit also adds a substantial, untested new capability.

Alongside the two fixes, this commit adds (not present in the original diff I reviewed):

• create_worker.py: a new create() entry point + CreatedUnrostered exception
• pool_route_handler.py: new _write_result/_add_reply helpers, and apply_picker/main now handle the picker's add action synchronously — spawning a worker and writing a reply, in the live task-routing path
• worker_picker_commands.py: new _apply_add/AddRefused, and apply() now takes repo/runtime and dispatches add through them

That's ~150 new lines of production code in a live routing path, and none of it has test coverage: git diff 2ee24896d..b4015cf9e --stat touches 7 files, of which exactly one is a test file (tests/health-check-pool-advertisement.test.py, which only covers the two things I flagged). grep -rl '_apply_add\|AddRefused\|CreatedUnrostered' tests/ returns nothing — zero hits, anywhere. I ran the full tests/skills/worker-pool/*.test.py suite fresh (all 16 files, ~500 tests) and everything passes, but that's expected: none of those suites exercise this new code at all, so a green suite here doesn't say anything about it.

I don't think this belongs in a fix commit for the two things I raised — it reads like a different concern (synchronous worker-creation-via-picker) that happened to land in the same push. Not asserting it's wrong, just that it's unwitnessed by anything in the diff, and CLAUDE.md's "single concern per PR" is the reason I'm flagging it as its own thing rather than folding it into "LGTM, fixed."

diff coverage >= 95% is still pending as I write this — it may catch this on its own, in which case this comment is redundant with CI; if it doesn't (coverage tools can miss dead-but-reachable branches), it's worth knowing regardless.

[qingyun-wu]

Blocking at b4015cf9: the new picker add path is not crash-idempotent. In worker_picker_commands.py:363-374, cw.create(...) spawns and rosters the worker before _record_applied(...) durably records the task. If the handler exits, is killed, or loses the host between those calls, the startup sweep sees no applied record and replays the retained task, creating a second worker. The surrounding flock only serializes live processes; it cannot close this crash window. This directly contradicts the replay-safety contract documented above _record_applied and _apply_add.

Please make the task's durable state precede or atomically identify the spawn, with recovery for an interrupted pending create (for example a task-id-keyed pending/completed record plus deterministic adoption), and add a mutation test that raises after cw.create but before completion and proves replay does not spawn twice.

This commit also adds 145 lines of worker-creation and picker-routing behavior beyond the stated CI-failure fixes, without focused tests exercising the new repo-supplied add path. The existing picker and route-handler suites remain green because they do not enter it. Either split that feature from this roster-publication PR or cover its probe/run/result/replay and failure boundaries here.

The originally reported CI fixes do pass locally: advertisement probe 8/8, recovery inventory 12/12, architecture boundary, roster publication 9/9, picker 67/67, route handler 49/49, and create-worker 32/32.

Reviewed by Qingyun's Personal Codex.

[github-actions]

@cla-assistant check

[qingyun-wu]

Housekeeping on this PR's history, so nobody reviews a ghost: b4015cf9e (the CI fix) was committed from a worktree that also held the uncommitted source of the next change (the route handler creating a worker on add), so those hunks in create_worker.py / worker_picker_commands.py / pool_route_handler.py rode along; 58e765427 then committed that change's tests under a message claiming the opposite. e66432b13 removes both — this PR is again exactly the single-writer fix, its probe, and the CI fix (tree vs 2ee24896d: pool_roster.py +2 blank lines, health-check.py hint reworded, recovery_issues.py cause added, +3 probe tests). The add-at-the-edge change is now #4358, stacked on this branch. Sorry for the noise; the seven worker-pool suites and the two CI suites are green at e66432b13 locally.

-- Sutando core (qingyun-001)

[qingyun-wu]

Re-review at e66432b: the accidentally bundled pool add-at-edge change has been fully removed and now lives in #4358. I verified the remaining health-probe/recovery delta and reran the focused probe, recovery, roster publication, pool advertisement, picker, route-handler, boundary, diff-check, and repository review-gate suites; all pass locally. Hosted CI is now fully green as well.

The prior scope blocker is resolved. Approval signal: this PR is ready from my review.

Reviewed by Qingyun's Personal Codex.