Repository navigation
integration: multi-worker stack (do not merge — CI only) - #4185
Closed
qingyun-wu wants to merge 100 commits into
Closed
qingyun-wu wants to merge 100 commits into
qingyun-wu wants to merge 100 commits into
Conversation
…s it `restart.sh` stops the watcher (`pkill -f "watch-tasks"`) and that is correct — it is src/-backed and should not survive a code change. What was wrong is the silence afterwards. Every other entry in STOP_PATTERNS is relaunched below or by startup.sh. The watcher cannot be: it is armed by the AGENT through the Monitor tool (schedule-crons step 1.5), so no shell can restore it, and `src/startup.sh` carries no `watch-tasks` reference at all. The caller was told the restart succeeded while its own task intake was gone. Measured 2026-09-11 on this host: an owner-authorized restart stopped the watcher on a live core. It surfaced only because the Monitor emitted an exit-144 notification — a harness artifact, not something this script provides. A shell-launched watcher would have died with no signal. Scope, stated precisely because the first draft of this claim was too strong: health-check DOES catch it (`watcher not running (no PID sentinel) — tasks/ will not be drained`), but on the next proactive pass, not at the moment of the kill. This closes the window between the two, it does not add a missing detector. Both arms verified to FAIL in the broken state: removing the warning -> rc=1; moving it above the pkill (where it would describe a still-running watcher) -> rc=1; restored -> rc=0. The test also pins the premise — that startup.sh has no watch-tasks path — so this stops being right by accident if that changes. Stand: Echo Act IV Mini
…xit code The watcher keeps only the exit code of a task-event handler. Five times tonight a handler run was recorded as exit 1 while the handler's own log showed a delivery and, once instrumented, a zero return from its last line — so the non-zero status arises after the handler's code and before the runner reads $?, and nothing recorded it. The runner now appends the handler's stderr and one `RUNNER rc=<n> <task>` line per run to <workspace>/logs/task-event-handler-runner.log.
…g it fails The existing suite scans source tokens and line order. qingyun-wu measured what that cannot see: wrapping the three warning echoes in `if false; then ... fi` leaves all six of its checks passing, so the test cannot detect removal of the behavior it promises to protect. This one builds an isolated copy, stubs every process-touching command with one that announces itself on stdout, runs restart.sh to completion, and asserts on what was PRINTED — the warning, the re-arm command it names, and that both follow the kill in execution order rather than in the file. Arm 3 is the reviewer's own control: the same `if false` perturbation, asserted to FAIL. It carries a guard of its own, because on a tree that never had the warning "nothing to disable" and "disabled" produce identical output — without the applied-perturbation check the control would certify a missing feature. Measured, same tree, warning disabled: structural suite: all ok (rc 0 — the defect) this suite: DISABLED-WARNING CONTROL fails the assertion (rc 1) On main, where the warning does not exist, this suite fails 4 of 6. Stand: Echo Act IV Mini
…r as the handler's own failure A redirection on the handler command is opened before the handler runs, so a log path that cannot be opened skipped the handler and reported its own error as exit 1 — a required Team task would then be failed without ever reaching its handler. The log is now opened on a separate descriptor first; when that fails the handler runs with the inherited stderr, as before.
…reads its rc receipt first The dispatch drain reaps a running marker whose worker pid is gone. It read "no deliverable result" as "died before producing one" and handed the task back with a synthetic exit 1. A routing handler finishes by writing a delivery sentinel and never a result, so whenever the drain ran between the runner's exit and the fifo's HANDLER_DONE being consumed — every burst — a successful run was marked failed: five owner-facing failure replies and several duplicate fallbacks on 2026-09-10, with the handler's own log showing a zero return each time. The runner now writes settled/<task>.rc before it signals; the drain reads that receipt first and treats only a MISSING receipt as death, where a deliverable result remains the one sign of success.
…er-held task is not unprocessed Three seams, all generic — any multi-instance install needs them, and none of them names or imports a pool. - watch-tasks-stream.sh: SUTANDO_TASKS_DIR names the inbox (an instance whose queue is not <ws>/tasks/ no longer has to be launched with an argument), and SUTANDO_WORKSPACE_DIR names the workspace. The second exists because of the first: once the inbox is <ws>/deliveries/<id>, its parent is deliveries/, so claims and fallback receipts would otherwise be written inside the inbox tree. - check-pending-tasks.sh: a task a peer already holds — <id>.txt or <id>.accepted in a delivery folder other than deliveries/core/ — is not this session's unprocessed work. deliveries/core/ is the core's own inbox, so a sentinel there still blocks; otherwise the Stop guard switches itself off for everything the core accepted. Tested with no handler and no pool module present: the watcher tests run a private copy against a stubbed scripts/sutando-config.sh and a stub handler in a temp tree, and the hook test builds its own workspace and asserts the resolver landed there before writing. Both pin the direction case, and both fail against the parent commit. These let the worker pool ship as an optional skill on top rather than as core changes.
…uncher `spawn()` created an identity, a folder and a tmux session, then sent a bare watcher into the pane: a worker reported `started` with no runtime bound to its recorded session id — a folder somebody reads, nobody answers. It now runs the core's launcher (`src/agent/start-cli.sh`) under per-worker env: SUTANDO_INSTANCE_ID, SUTANDO_TASKS_DIR (the delivery folder), the tmux socket/session, and SUTANDO_CLAUDE_SESSION_ID, so the id in the lineage record IS the CLI session; the agent starts its own watcher exactly as the core does. The runtime defaults to the configured core runtime. The launcher gains a worker mode: no remote-control/chrome surfaces, the worker env forwarded into the session, the core-only shutdown gates skipped. The watcher honours SUTANDO_TASKS_DIR so `/startup` needs no argument change. `current_track.py` and its tests are main's (#4111); the earlier head had reverted that fix while vendoring.
…when its env is unset Pins both sides of the gate. Unset, the real tmux path launches `claude --name sutando-core --remote-control Sutando --chrome …` and the watcher creates and watches `<workspace>/tasks`; set, the pane runs `--name sutando-worker-<id> --session-id <uuid>` with neither owner surface, and the watcher watches the delivery folder and never touches `tasks/`. The two unset cases pass against main as well, which is the guarantee.
…ates late cleanup The watcher is a process group (bash, fswatch, a sleep loop). Killing the leader alone left children writing into the private tree while the temp directory was being removed, so on Linux the two watcher cases errored with "Directory not empty" at cleanup; macOS happened not to race. Start the watcher in its own session, kill the group, and ignore a straggler at cleanup — the assertions read the folders before teardown either way.
…ot from its inbox The watcher derives its workspace as the parent of the folder it watches. A worker watches <ws>/deliveries/<id>, so its results, claims, fallbacks and — once the per-instance sentinel lands — its liveness sentinel were written under deliveries/, where no reader enumerates. The spawner now names the workspace in the worker's environment and the watcher honours it; a core launch, which watches <ws>/tasks, is unchanged.
…po's interpreter Two review findings, both real, both reproduced before fixing. The premise assertion `! grep -q "watch-tasks" startup.sh` claimed "nothing in startup.sh re-arms the watcher" and actually asserted "the literal does not appear". Measured in both directions: inline startup.sh's sentinel path (a no-op refactor, and the form a shipped engine build carries) FAIL <- fires on nothing inject a real re-arm as __w="watch-""tasks-stream.sh" ok <- silent on the regression It cried regression at a cosmetic edit and passed a genuine one. Removed rather than replaced with a second proxy: a sound version must RUN startup.sh, and `set -e` plus ~82 env dependencies make that a partial run whose silence is not evidence — the same absence-from-truncation this suite exists to avoid. What IS soundly checkable is the warning's own claim about its own file, so the behavioural suite now asserts restart.sh starts no watcher itself. That run reaches startup.sh (arm 1 pins it), so the absence is a completed observation. Separately the harness used `command -v python3 || echo /usr/bin/python3`, which reaches the CLT stub `scripts/python-binary.sh` exists to avoid — and restart.sh itself sources that helper at :14-18, so the test contradicted the file under test. It now resolves through the helper and FAILS LOUDLY when no interpreter is available, rather than silently skipping the control. no-op refactor structural rc=0 (was 1 — false alarm gone) warning disabled behavioural rc=1 (real control still fires) Stand: Echo Act IV Mini
…sion writes the shared workspace
`CORE_ENV_ARGS` in src/agent/claude/cli/start-cli.sh is an explicit tmux
allowlist with no general forwarding loop — the file says why twice in its own
comments ("tmux takes the server env, not this shell's"). `spawn_worker.plan()`
sets SUTANDO_WORKSPACE_DIR, but the name appeared 0 times in the launcher, so it
never crossed into the worker's session. The watcher then fell back to
`dirname(inbox)`, and a worker's results/ and state/ landed under
<ws>/deliveries/ — where no drain reads, silently and with no error. Exactly the
outcome spawn_worker.py's own comment says the design prevents.
Repro (launcher's --print-core-env probe under the plan's env, then the
watcher's resolution block fed from what the probe forwarded):
parent 5080eaf
SUTANDO_WORKSPACE_DIR forwarded? NO (control: SUTANDO_TASKS_DIR? YES)
WORKSPACE_DIR=<ws>/deliveries
RESULTS_DIR=<ws>/deliveries/results <- no drain reads this
HEAD
SUTANDO_WORKSPACE_DIR forwarded? YES
WORKSPACE_DIR=<ws>
RESULTS_DIR=<ws>/results
Fix: one allowlist entry, in the same env-gated shape as the line above it, so an
install that sets no workspace env forwards none and resolves as before.
Test: tests/start-cli-worker-env-forwarded.test.sh, in the style of its sibling
start-cli-proxy-routing.test.sh (same --print-core-env probe). Its central check
is derived, not a hand-list: the required set is the INTERSECTION of the keys
`spawn_worker.plan()` sets and the keys `src/watch-tasks-stream.sh` reads from
env, so the next variable added on either side is covered without editing the
test. It also asserts the behaviour (RESULTS_DIR resolves into the shared
workspace, not under deliveries/) rather than the allowlist's spelling, and pins
the no-worker-env case. 7 checks green at HEAD; the same file at 5080eaf fails
4 of them, naming MISSING: SUTANDO_WORKSPACE_DIR.
Reported with a live witness: this test killed both pool workers' task watchers
and bounced web-client and the credential proxy on a reviewer's machine.
build() mkdir bin -> perturbation `|| return 1` -> stubs written
^ returns HERE, leaving bin/ EMPTY
run() PATH="$sb/bin:$PATH" -> falls through to the real /usr/bin/pkill
caller ran arm 3 unconditionally, recording perturbed=1 and continuing
So an unusable interpreter — the exact case the previous commit added a check
for — meant every `pkill -f`, `launchctl kickstart -k` and drain loop in
restart.sh executed for real, in restart.sh's order.
Four changes, each closing a different link:
1. Stubs are written BEFORE the perturbation, so an aborted build cannot leave
an empty bin/.
2. run() REFUSES unless bin/pkill and src/startup.sh exist, and says so.
3. PATH is stub-only. Not `:$PATH`, and not `:/bin` either — /bin carries the
real launchctl. The interpreter and the read-only utilities restart.sh needs
are symlinked INTO bin/ and named explicitly.
4. Arm 3 runs only when perturbed=0.
Verified:
run() on an unbuilt sandbox REFUSED-UNBUILT-SANDBOX
pkill/pgrep/launchctl/lsof/
osascript/killall/tmux under the
stub-only PATH none reachable (bash, hash cleared)
positive control a stub IS reachable
The reachability check had to be re-run under bash: zsh's `command -v` answered
from a hash table populated before the PATH changed and reported pkill reachable
when it was not.
Stand: Echo Act IV Mini
… stale Spawning a worker was three steps: `spawn_worker.py` for the identity, a hand-edited `bindings.json`, and a `python -c` to recompile the roster. Nothing in the tree calls `compile_roster` at all, so the third step existed only in whoever remembered it — and missing it is silent, because the worker runs, holds an identity, and is unaddressable: the router reads the roster and nothing else. That had already happened when this was written. Two worker records sat in `state/workers/`, one in the roster; the other had no delivery folder and a dead session, and a task addressed to it resolved to "not on the roster" and went to the core. The command composes what exists and decides nothing new. It refuses before any side effect rather than after — a worker is four side effects, and a failure found afterwards leaves some behind. A worker cannot run it: lifecycle is the core's. Records with no roster entry are reported, never adopted, because inventing a state for one would decide something the core owns. Controls: drop the recompile and 5 of 10 tests fail; move the caller-identity refusal after the spawn and its test fails. 10 pass at HEAD; the delivery, roster and launcher suites are unchanged and green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e intent `_REPO` locates `src/` for the imports and defaults `--repo`; it is a repo root, not a workspace, and the workspace still resolves through `resolve_workspace()`. The resolution lint cannot tell those apart from the expression alone, so it refused the new line. It ships a per-line pragma for exactly this, which states the intent rather than rewording the expression to slip past the pattern — and it exempts one line, leaving every other line in the file visible. Verified with CI's own invocation (`BASE_REF=<base> --diff`), whose exit code is the check: rc=1 before, rc=0 after. The bare run passes either way, because it tolerates pre-existing resolutions — which is why this was invisible locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The coverage gate measures `scripts/`, and the file sat at 91% against a 95% bar. The eight unmeasured lines were not incidental: they were the missing-repo refusal, the refused spawn, the worker-created-but-roster-failed path, and the `--json` output — the behaviours the PR body claims, none of them exercised. The third is the one worth a test. A worker exists and routing cannot see it, so exiting 0 would tell the caller it is usable; the test pins the non-zero exit and that stderr names the worker. Measured: 91% -> 100% on the file, 14 tests. Controls: turn that exit 1 into a 0 and its test fails; drop the missing-repo refusal and its test fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y tasks The broker turns each picker button into a normal task addressed to this instance: `source: worker-picker`, an id prefixed `worker-add-` or `worker-pin-`, and one sentence of English. Nothing here consumed them, so the '+' button reached the broker and stopped. Two rules keep recovering that intent honest. The ROOM is taken from the `channel_id` header rather than the sentence, because the header is what the gateway stamped and the sentence is prose. And `source` must be that header: prose saying "worker picker" grants nothing, or anyone able to send a message could create workers on this host. It returns intent and does not act, so a misparse cannot spawn anything. An unrecognised sentence from a genuine picker task returns None rather than a guess — a wrong intent here creates a worker or re-routes a room. The test texts are the broker's own, copied from its handlers, so a wording change upstream fails a test instead of silently parsing to nothing. Controls: drop the source gate and the two forgery tests fail; prefer the sentence's room over the header and its test fails. 14 tests, 100%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A new module under src/ has to be regenerated into the index, or the map goes stale and CI says so. Generated, not hand-edited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t header only, never from prose `parse_task_file` used `parse_task_headers_lenient`, which scans every line: a `source: worker-picker` written in the task BODY was accepted as the sole authorization, and the four room-scoped branches fell back to the room named in the sentence whenever `channel_id` was absent. Both contradicted the module's own two stated rules. The strict parser (`parse_task_headers`) stops at `task:`, so no body line can reach `parse` as a header, and a room-scoped button with no stamped room is now refused with a named reason instead of guessing from the sentence. Refs #4120
… state file Main's gateway bridge has no pool advertisement at all: it never POSTs /v1/workers and its profile PUT does not exist, so a production core running this bridge shows the broker no workers and the picker has nothing to draw. The bridge now reads state/pool-advertisement.json, written by the pool side from the roster (#4119). Its "workers" value is POSTed verbatim on boot and on every mtime change; its "profile_workers" map rides on the profile card. The broker REPLACES the profile document, so a card sent without a workers field would erase the map the picker reads — the map therefore ships on every put, and the file's mtime is part of the change key so a roster rewrite re-puts even when the map serialises identically. No file, a malformed file, or a non-dict value reads as "no pool": nothing is pushed and nothing raises, because both pushes run inside the task loop. Both pushes carry the same failure contract as the rest of the loop's best-effort relays: 404 defers an hour (a broker without the endpoint), any other error 5m, never an exception.
…an unsupported endpoint earns the backoff Three findings from @keweichen's review, confirmed by @john-the-dev, all reproduced at f3b1fa4 before the fix. [P1] An unavailable advertisement caused a destructive profile replacement. _read_pool_advertisement collapsed missing, malformed, non-object and one-sided records into (0.0, {}), so _build_agent_profile's .get returned None, the card shipped without `workers`, and the broker — which REPLACES the profile document — erased the map the picker was drawing. The push-on-change guard did not suppress it: an unavailable read drops the mtime to 0.0 AND removes the workers block, so the key was guaranteed to differ from the last good push. The guard fired exactly on the transition that erased the map. Availability is now a tri-state and None is the only "cannot read it" value. Both halves are validated from one read, so a one-sided record is unavailable rather than half-available; a valid record whose maps are empty stays AVAILABLE and ships, because "the pool is empty" is a fact the picker must be told. Unavailable skips both pushes, logs once per edge, and leaves the broker holding the last state we sent. _build_agent_profile now takes the map as an argument, so it cannot be reached with one nobody validated. reviewer's capture, parent f3b1fa4 -> HEAD valid put=True ['display','host','workers'] True ['display','host','workers'] unchanged-ctl put=False False corrupt put=True ['display','host'] False restored put=True ['display','host','workers'] True ['display','host','workers'] missing put=True ['display','host'] False put-workers-presence [True, False, True, False] [True, True] one-sided record [('POST','/v1/workers',False), ('PUT','…/profile',False)] [] [P1] The advertisement's producer is not in this stack. src/pool_advertise.py lives on #4119 (feat/advertise-pool-to-broker), whose merge-base with this branch is this PR's own base c73f8e9 — so it is not an ancestor and `git grep pool-advertisement` at HEAD finds only this reader and its tests. Rather than copy the producer in, the reader's contract is now explicit and testable without it: the file is read-if-present, and absence pushes NOTHING — not even an identity-only card, which is what made an install with no producer destructive. That is main's behaviour (main has no profile PUT at all), so an absent producer is a no-op rather than a regression. Sequencing is the owner's call and is raised on the PR. [P2] Every HTTPError took the one-hour unsupported-endpoint backoff, contrary to both docstrings. Only 404/405/501 — the broker saying the route does not exist here — now defers an hour; every other status is a live endpoint failing and takes the 5m retry, and the log line says which. code parent HEAD 404 3600s 3600s 500 3600s 300s 503 3600s 300s 401 3600s 300s URLError control 300s 300s Also moves the two 15s pushes behind _retry_pending_publications() per @john-the-dev's standing ordering note — a best-effort relay must not sit between a pending result and its next delivery attempt. /usr/bin/python3 -m pytest tests/test_pool_advertisement_push.py -q 15 passed in 0.19s (9 of them fail at f3b1fa4; all 15 at c73f8e9) /usr/bin/python3 -m pytest tests -q 130 passed in 7.17s bash scripts/review-checks.sh --diff /tmp/f4162.diff review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)
…lback Reading intent from an English sentence works and is brittle: a wording change upstream breaks it, and the tests only make that break loud. A stamped command removes the guessing. So the header is read first — `picker_command` plus JSON `picker_args`, from ABOVE `task:` like `requested_worker`, so a body cannot forge one — and the sentence stays as the fallback for a broker that stamps nothing. Nothing is lost: the same task keeps working before and after the broker learns to stamp. Two refusals rather than a guess. An unknown verb reports itself as unsupported instead of falling through to a sentence written for a different one, and args we cannot parse refuse too: the prose beside them may say the same thing, but nothing proves it. The room still comes from `channel_id`, even when the args name one. Controls: let an unknown command fall through and its test fails; let malformed args fall through and its test fails. 20 tests, 100%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ed task path, and is validated against its schema `picker_command`/`picker_args` were outside KNOWN_HEADER_KEYS, so every parser classified them as body and dropped them — from above `task:` as well as below. A legitimate stamped command parsed to None through `parse_task_file`, the gateway writer never emitted the fields, and `serialize_task_last` raised on them. Only the unit tests, which hand `parse()` a dictionary no producer can build, ever saw the feature work. Both keys now live in the vocabulary (canonical + vendored copy, with the two TypeScript guard mirrors in lockstep), and in the gateway writer AHEAD of `task:` — alongside `source` and `channel_id`, which the reader needs in the same pre-body region now that it uses the safe parser. `picker_args` is re-serialized as JSON so an object never lands as a Python repr. Valid JSON is no longer a valid command: args must be an object, `workers` a non-empty list of names (a bare string was iterated into one worker per character), `dedicated` a real boolean, `label` a string, and the room comes from `channel_id` alone — `picker_args.room` is never read. Each refusal names the rule it broke, and a present-but-empty command refuses instead of falling through to whatever sentence is in the body. Refs #4121
The picker in the AG2 Space client is built and shipped; it renders nothing because the instance never advertises its pool. It reads two things by two routes — `POST /v1/workers` for each row's status, and the agent profile's `workers` map for each row's label and runtime — and sending one without the other renders half a row: statuses named by raw 32-hex ids, or names attached to nothing. So both bodies are built here from a single roster read. Translation, not passthrough: the broker's fields are the affinity era's (`live_cores`, `dead_cores`), the roster is this design's. `recovering` maps to neither list on purpose — the broker renders an unlisted-but-known id as "available", which is what a restarting worker is. `retired` is advertised in neither body, because any id with metadata stays on the picker forever. Builds only; it does not send. Transport, credentials and retry belong to whatever already talks to the broker. Controls: drop the retired filter and its test fails; call `recovering` dead and its test fails. 13 tests, 100% of the new file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… allowlist
Four red checks, one cause each of two tests; the coverage and typecheck jobs
only gate on them.
`docs/src-map.md` was stale: a new module under `src/` has to be regenerated
into the index. Ran the generator rather than hand-editing it.
The state-paths test flagged `state = (row or {}).get("state")` as a
runtime-state path reference. It is the ROSTER's own field — live, recovering,
abandoned, retired — and the module reads no path at all: it takes the roster
through `pool_roster`, which owns resolution. That is what the test's allowlist
is for, so the entry carries its justification like every other one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e can send it
The two broker bodies were only ever produced on demand; nothing durable told
the gateway bridge what to send, so on boot it replayed a frozen pool snapshot
and a profile card without workers, which emptied the picker on production.
write_advertisement writes state/pool-advertisement.json ({ts, workers = the
POST /v1/workers body, profile_workers = the profile card's workers field})
whole-or-not, and create-worker writes it right after the roster compile. The
bridge reads that file on boot and on change; a workspace without a pool has
no file and the bridge sends nothing, as before.
…the picker shows them
The broker answers /v1/workers?room= from the snapshot's bindings map and the
client draws the 'bound to <worker>' chip on an existing chat only when that
row says pinned. The snapshot carried live/dead workers and no bindings, so a
room the owner had pinned showed nothing. Each roster binding becomes
{instance, instances: [instance], pinned: true}, the row shape the broker reads.
…ds holds it `_defer` wrote ONE marker store. With `state/pool-route-retry` obstructed the run still exited 0, so the watcher released its claim, `retry_pass` — which enumerates markers — never saw the task, and the Stop hook then reported a worker-addressed payload to the live core as unprocessed work. A transient state-write failure could therefore re-expose a worker's task to the core, and a later restart could deliver it to the worker too. The marker now has a second store under `tasks/.pool-route-retry`, which shares no component with the first below the workspace and is hidden from every `tasks/*.txt` glob. `retry_pass` enumerates both, so one obstructed path no longer takes automatic retry offline — recovery needs no repair and no restart. When NO store takes the marker the run exits UNSETTLED (5) instead of 0: 0 would settle work no pass can find, and any other non-zero takes the watcher's fallback disposition and hands a worker's task to the core. `finish_handler_task` maps that one code to "keep the claim, publish nothing". The Stop hook stops spelling the marker path and asks the handler (`--parked`, 0/1/2 like `--held`). A store obstructed by a file is a NEGATIVE — nothing can be stored under it — so only a genuinely unreadable store is UNKNOWN, and the hook does not go blind for every task when one path breaks. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… names it spawn_worker points the worker's watcher at `deliveries/<worker>`; the router writes a ZERO-BYTE sentinel there; the watcher emitted that basename and handed that path to the handler. `pool_delivery.read_payload` could resolve the body, but no launcher or watcher path called it — so a launched worker was handed 0 bytes where its task was (probe: payload_bytes=61, watched_file_bytes=0). The resolution belongs in the watcher, at the one place an inbox event becomes a task path, and the KIND of inbox is injected by the adapter that named it (`SUTANDO_INBOX_KIND`, set by spawn_worker). The watcher never infers "this looks like a delivery folder" from a path, and the sentinel->payload grammar stays pool_delivery's: a new `payload` verb answers it for callers outside python. Putting the mapping in the launchers instead would copy that grammar into every runtime, which is the duplication the boundary rules forbid. The Codex path additionally lost the shared workspace: the spawner set SUTANDO_WORKSPACE_DIR but no SUTANDO_RESULTS_DIR, the launcher forwarded neither to the notifier, and task-notifier derived results, state, claims and logs from the inbox's parent — `deliveries/results`, which no bridge drains (matches=False). Both launchers now forward the workspace/results/inbox-kind seams, and the notifier resolves every one of those paths from the workspace. spawn_worker composes neither folder itself any more: `pool_delivery` owns the deliveries and results directories, which is also what the state-paths adoption guard asks for. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… the redirect is recorded @yixuan-ag2's nit at pool_router.py:15 — the last sentence of the module docstring was a fragment, so the rule it states ("re-aiming work at a reachable worker is how a task reaches someone the owner never addressed") read as nonsense. The docstring now also says where an unknown target is surfaced, which is the fact that turned that reviewer's "silent mis-route" reading around. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…bprocess The `payload` verb and the handler's usage error were exercised only through `subprocess.run`, which scores zero coverage on the code it ran — so ~15 added lines read as untested to the diff-coverage gate while being covered in fact. The same answers are now asserted in-process as well, alongside the two branches nothing reached at all: a marker store that cannot be enumerated, and a run given neither a task file nor a verb. Local, over the four suites that touch the changed modules: pool_delivery 91% -> 98%, pool_route_handler 95% -> 96%; every remaining miss is a pre-existing line outside this PR's diff. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…omment cap The third line restated what the two above it already said; every other section marker in this file is exactly two lines. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…refuses `os.replace` made publication atomic for the READER; it did nothing for two writers that each loaded the roster, each computed `dropped` against it, and each published. Both decisions were valid against the same predecessor, both allocated version 2, and the second publication re-aimed the first writer's room at the core — the exact silent loss the shrink guard refuses sequentially. The whole sequence — predecessor read, shrink check, version allocation, publication — now runs under an exclusive sidecar lock on `<roster>.lock`, and a caller that read the roster BEFORE calling names that version as `expect_version` and is refused if the roster moved since. The lock is `src/record_lock.py`, and `worker_identity._appending` — the existing production-writer lock this repo already exercises for concurrency — now delegates to it, so a record with a read-decide-write section has one implementation to reason about rather than a second copy added here. Reported by keweichen in review of #4176.
…t it attributes Two halves of the same contract, both raised on #4175. The reader took ANY filesystem object at the flag path as a finish — a directory there produced a confident `metadata.worker_id`, while `pool_delivery.residue` (`.is_file()`) read the same tree as unfinished. What counts as a flag is now one predicate, `pool_delivery.is_done_flag`, asked by both; malformed state raises so the gateway abstains and logs instead of letting the other layout's claimant win by default. The flag also had no production writer: a constructor and two readers, with the only creations in test fixtures. `pool_delivery.mark_done` is that writer — temp file + rename in the same directory, recipient and task id bounded by the pool's own grammar. The worker-side path that publishes a result into `results/` calls it first, so a result the drain can see always has its attribution beside it; the reverse is what leaves a reply with nobody's name on it, and unlike a flag with no result it cannot be recovered after delivery. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lti-worker # Conflicts: # src/pool_delivery.py
Diff coverage named `src/pool_delivery.py` lines 142-144 and 324-328: the rename-failure cleanup and the `mark-done` argv the watcher calls. The watcher reaches that argv through a subprocess, which exercises it and measures nothing, so both are driven in-process here — the failure path with a directory squatting the flag's own name, which is also the reader's malformed-state case. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…n/multi-worker # Conflicts: # src/check-pending-tasks.sh # src/pool_delivery.py # src/watch-tasks-stream.sh # tests/check-pending-tasks-worker-held.test.sh
# Conflicts: # src/watch-tasks-stream.sh
The merge order with heads, the four env seams a worker is driven by, and the create-worker entry point — so the stack can be tried without reading fifteen PRs.
Contributor
Coverage Gate❌ Test suite failed under instrumentation — coverage not measurable. See the job log. |
qingyun-wu
commented
Sep 11, 2026
qingyun-wu
left a comment
qingyun-wu
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed head 8ba463f3f0a3c620e72f9b5a28aac2ea32422843.
Changes requested for deployment; retain the explicit do-not-merge/CI-only designation.
I checked the 84-file cumulative diff and integration resolutions: 71 final blobs match tracked component heads exactly; 13 files compose components or add integration documentation. The two reported suite failures reproduce locally: delivery runs 89 tests with 7 stale held_by_worker errors; marker-failure recovery misses settle_handler_outcome, so the ordinary-failure positive assertion fails while the no-output assertion passes vacuously. They remain component/integration work, not new duplicate findings.
New runtime evidence in this combination: #4175's flag-before-result writer can lose an undelivered reply. Running the production publish_terminal_failure with only result mktemp failing, followed by the production sweep, gives:
publish_rc=1
flag=True result=False residue=finished
sweep.retired=[task-repro]
sentinel_after=None payload=True result_after=False
The success control retains a real result. This happens in both #4175 alone and this integration tree; repair the publication/recovery contract in that component and refresh the integration. The standalone crash test calling this state “recoverable” does not exercise the destructive sweep. Existing worker bootstrap blockers are also inherited; this is not safe to recommend as a working pool deployment merely because the merges resolved.
Worst case is silently losing a task reply after a publication failure, plus duplicate core/worker ceremony from the inherited bootstrap issues. Current hosted node/shell and Python checks were failing (CLA green); local added-path/root/comment checks passed, with a failing forbidden-path control. No live deployment or contributor branch changes performed.
Collaborator
Author
|
Closed at the owner's direction (2026-09-12 03:45Z). This was the CI-only integration of the pre-#4206 stack; it will be rebuilt from the consolidated branch. Branch preserved. Sutando core (qingyun-001) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Do not merge. This branch is for USE and CI only.
integration/multi-workeris the fifteen open multi-worker PRs merged together so aperson can run the whole design before the stack lands, and so CI exercises the heads
in combination. Every line belongs to one of the PRs below — merging this would bury
fifteen reviewable diffs in one commit. Review and land the PRs; then delete this branch.
Base:
origin/main@0b4b8f163. Head:8ba463f3f.Merge table
src/pool_delivery.pymain(): both sides added a subcommand. Union —choicescarriespayloadandmark-done,nargs="?"kept (the--held-only form needs it), both command bodies kept with their ownreturn 0.•
src/check-pending-tasks.sh— #4167'sheld_by_other_instancewording and message, plus #4110's parked clause and itspool_route_handler.py --parkedcall (the only surviving line of #4110's hunk, as expected). File is byte-identical to #4167's plus that clause.•
src/pool_delivery.py— tookCORE_RECIPIENT/self_recipient()/held_by_other_instance()and thed.name == meskip;--heldhelp text from #4167; argparsechoiceskept the union from row 8.•
src/watch-tasks-stream.sh— two comment-only conflicts, #4167's wording.•
tests/check-pending-tasks-worker-held.test.sh(add/add) — #4167's 13-case suite, which strictly covers #4110's cases, plus #4110's case 5 re-added as case 14 (the directpool_delivery --heldassertion, retargeted to #4167'sPEER/TASK_IDnames). No test dropped.src/watch-tasks-stream.shfinish_handler_task(): #4165 extracted the settle policy intosettle_handler_outcome(); #4167/#4164 had added anHANDLER_UNSETTLED_RCbranch to the inline copy. Resolved at the shared owner — took #4165's one-line delegation and moved the unsettled branch intosettle_handler_outcome(), so both call sites (drain and shutdown) get it. Duplicating it at the call site would have been the second copy.docs/src-map.mdwas regenerated (python3 scripts/gen-src-map.py) and came back unchanged — nointegration commit needed for it.
bash -n/python3 -m py_compileclean on every touched fileafter every merge.
Hermetic suites (34 run, on this host)
32 pass. Two failures, both stack findings — no source was patched to make them green:
1.
tests/pool-delivery.test.py— 7 errors,AttributeError: module 'pool_delivery' has no attribute 'held_by_worker'(Ran 89 tests,FAILED (errors=7)). #4110 addedheld_by_worker()+TestHeldByWorker; #4167 renamed the function toheld_by_other_instance()and added its own tests. Git auto-merged the test file (no conflict), so #4110's stale class survives against a symbol #4167 removed. Neither PR fails alone. #4110 needs a rebase onto #4167 that dropsTestHeldByWorker— #4167'sTestHeldByOtherInstancealready covers every case in it.2.
tests/pool-route-marker-failure-recovery.test.sh—FAIL an ordinary handler failure still falls back to the core(watcher printed:— empty). Its harness liftsfinish_handler_taskout ofsrc/watch-tasks-stream.shwithawkandevals it under stubs. Since #4165 that function is a one-line delegation tosettle_handler_outcome, which the awk range does not capture, so the eval hits an undefined command (stderr is2>/dev/null'd) and prints nothing. Worse than the red line: the case above it,an unsettled run keeps the claim and tells no core, asserts on the absence of output and so now passes vacuously. The harness must liftsettle_handler_outcometoo (or call it directly) — otherwise the suite measures nothing after #4165 lands.Not run, per this host's standing safety rule against live-action tests:
tests/spawn-worker-runtime-and-sentinel.test.py(executessrc/agent/start-cli.sh),tests/start-cli-worker-bootstrap.test.py(launches the real launcher on a tmux socket),tests/start-cli-worker-env-forwarded.test.sh(executesstart-cli.sh --print-core-env),tests/restart-warns-watcher-stopped-behavior.test.sh,tests/restart-stops-only-own-watcher.test.sh.CI on this PR covers them.
Gate
Signed: Sutando core (qingyun-001)