Repository navigation
pool: adopt the rescue line — lead-follower pool, room pins, dedicated workers, picker relay - #3604
qingyun-wu wants to merge 159 commits into
Conversation
…l CLI, instance registry, base views One capability: a local Unix-socket JSON-RPC daemon that is the runtime authority for THIS Sutando instance, plus the canonical CLI front door. Server: single-instance flock per run dir; durable request store; approval/elicitation/human-action lifecycle; task submit/status/results with source isolation; capability registry composition; push subscriptions (results/activity) over the same socket; instance registry + lifecycle-lite start/attach; agents/identity(base)/tasks/runtime/ schedules views — 'nothing is invented here', absent records stay absent. Daemon-side actor resolution (env -> enrolled -> placeholder); a client parameter can never override identity. CLI: bin/sutando front door; task chat/watch with paste-safe compose; instance list/start/attach/open; tui reference client. Excluded by design: Stand identity surface (next PR — authority semantics reviewed separately) and the SCP device plane (WSS transport, pairing, voice — ships after). server.py stubs _maybe_start_wss. 18 test suites green (e2e boots the real daemon over its socket). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e-walk-up pattern The flagged lines resolve the CODE tree for imports, not the workspace; parents[1] + comment keeps the linter's target (workspace resolution) unambiguous. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The header-key parity sweep pins task_body_guard.py, task-bridge.ts and conversation-server.ts to the same key set; the envelope field must ship with all three. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bare python3 is the CLT stub on clean Macs and the WRONG interpreter on bundled installs (vendored 3.12 vs system 3.14, measured by review) — and the daemon writes durable SQLite/JSON the rest of the system reads. All 10 exec sites go through resolve_python; fail loud when none runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
'\s' in a single-quoted JS string collapses to 's' — real headers with space-before-colon stopped matching. Measured both directions by review (rui-agent): 'id :' true->false, 'ids:' false->true. Restored \\s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d; py3.9 test import Three verifier blockers: - P1 path traversal: _checked_task_id confines client-supplied ids to the task namespace (task-[A-Za-z0-9._-]+, no separators/traversal) at every public entry; the reported '../state/secret' repro now reads as absent (test carries the exact repro + hostile-id sweep + a legit-id positive control). - P1 identity drift: start_instance exports the MANIFEST's instance_id, falling back to agent_id only when absent; divergent-id test spies the launcher env. - py3.9: identity-view test gains postponed annotations so import succeeds on the supported system python. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…by server.py
The adoption guard's token regex matches their RPC status fields
("state": "pending") and CLI parser comments; none of the five files
resolves workspace paths — server.py is the composition root that
imports the canonical resolver and injects every directory.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The class was appended after unittest.main(), so direct execution (the CI runner's mode) exited at 19 tests and never ran the hostile-id regression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ion + surface/stream/edge suites Coverage work that found two real defects, both fixed here: - schedule.list crashed on any configured daemon (schedules_view calls list_schedules, which the extraction left behind on the host branch — ported with schedule_owner/_RUN_PREFIX_RE; configured-read pinned). - tasks.details raised raw OSError on an unreadable task file instead of degrading to absent like every sibling read. Coverage mechanics: the e2e/CLI subprocesses now self-instrument under SUTANDO_TEST_SUBPROCESS_COVERAGE (voice-lock pattern), so daemon + CLI execution counts. New suites: CLI surface contract (every subcommand, exit-code contract 0/1/2/3), CLI streaming (watch pushes, activity/tool feed, request wait, chat pipe-mode, requests-stream subscribe, crashed-subscriber discard), edge gaps (fail-safe branches across views), terminal-open pure core. approval.respond device-plane paths pinned in the dispatcher suite. Interactive-only paths carry narrow pragmas (tty chat UI, tmux firehose, WSS transport pending the S1 slice, tui key loop, terminal spawn) — each names its tested pure twin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… prose cap Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…LI instance/approval drives Also fixes my OWN repeat of this morning's blocker: the registry EdgeBranches class landed below the __main__ guard, so direct execution ran 14 tests, not 20 — and I read 'Ran 14' as including the new six. Guard moved to EOF; two latent API mismatches in the never-executed tests surfaced and are fixed (list_instances, typed attachable manifest). NaN case pins the capability encoder's own except-branch (the shape walker passes NaN; only allow_nan=False rejects it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Direct RuntimeServer coverage: dead subscribers discarded on every push stream (activity/requests/results), activity watcher survives feed rotation and poison lines, requests watcher survives store failures, result emitter skips unreadable files without retry loops, helper fallbacks fail to None. CLI: attach --print emits argv from a real manifest; watch teardown is SIGINT so the documented Ctrl-C path runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…es, tui branch cases, platform-branch patch, probe stages CI read 94% (bar 95). This round: archived-status/latest-unreadable/ list-skip/headers-unreadable in tasks_view (98%); tui error-response, early-close, identity-mismatch, registry-backed _views; terminal-open ghostty + which()-mocked wezterm/kitty argv (100%); dashboard owner branches + deterministic day-scale rel format (fixed now=); both platform-exclusive registry_dir lines via sys.platform patch; attachable identity/core stages against a stub daemon socket. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…after a ready read seen.add ran before the read, so one ordinary read race suppressed that result for the daemon lifetime — and the branch test codified the loss. The emitter now uses the shared readiness policy (unreadable/mid-write/ empty = not-yet, name stays out of seen); regression pins transient- failure-then-recovery delivery, mutation-verified (seen-first fails it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…th definition Echo's SSoT question: the registry lives outside the workspace because discovery must precede workspace knowledge; the ruling now sits where the path is defined so it doesn't get 'fixed' back. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e daemon read succeeded, file archived before f.stat(): FileNotFoundError propagated through the watcher gather. Guarded with a now-fallback ts; regression pins read-success/archive-before-stat delivery (mutation-verified: unguarded stat fails it, pycache purged both ways). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Covers the branches the 94% gate run named: channel dirs without or with unreadable access.json, agents-view stat race, settle without requestId, archived-task done state, unreadable latest/list results, malformed poll intervals, host-label success path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t/sutando-server-v0
…y, clock 1. Manifest launcher default is bin/sutando serve — startup.sh never reaches server.py, so the old default could not restore the daemon; the E2E restart leg now runs WITHOUT launcher overrides. 2. TUI fails closed: task/request/attach/open refuse when the socket's answering identity is unverified or mismatched (action path tested). 3. Cross-instance start injects the manifest's identity.agent_id so the child never inherits the caller's ambient actor identity. 4. alive/health checks bound age below at 0 — a future-dated heartbeat (clock skew) renders offline, never healthy. All four mutation-verified. Plus two test-harness fixes: hermetic SUTANDO_RUN_DIR (host instance.lock collision) and a poll for the boot manifest (registration lands just after the socket). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Lead = runtime daemon (single task-admission point, fixing the census task_id gap), followers = registry instances; assignment-by-rename with leaderless fallback on lead-beat loss; #884 primitives carried verbatim. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PoolLead: priority-ordered assignment-by-rename, sticky channel affinity with idle rebalance, least-loaded fallback, dead-follower reclaim (claims untouched — done-flag recovery owns those). 8 tests; the pending pattern once re-matched assigned names (dotted ids) and double-assigned — now excluded and pinned. claim_task.py + tests ported verbatim from feat/multi-core-channel-affinity; claim_with_affinity marked legacy fallback (policy moved lead-side). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…degraded fallback acquire_work: own assignments in priority order in both modes; a LIVE lead owns the unassigned pool (never steal past its policy); stale, absent, or future-dated lead beat degrades to leaderless claiming with the pool's claimed-<instance> suffix so lead-side load counting and reclaim see fallback claims. 8 tests; no-steal gate mutation-verified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
End-to-end assignment->claim (dotted broker-style id), fallback claims weighing in the lead's load counter, and reclaim-then-leaderless pickup when lead and follower die together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PoolMetrics: append-only per-day JSONL (O_APPEND+fsync, fail-open — a metrics error never breaks scheduling) + summarize() computing the quality-bar set: assignment distribution, head-of-line incidents, fallback claims, reclaims, mean wait per channel. Lead records each assignment with arrival->assign wait (stat before rename — the source inode is gone after). Corrupt lines counted, never fatal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
install/uninstall/pool-status ported from #884 (workspace via the M0 helper — env override retired post-#1440; plist PATH captured from the installer instead of a literal); pool-lead-daemon.py binds PoolLead to the live workspace with bounded-age liveness and metrics, unlinking its beat on exit so followers degrade immediately; follower skill routed through acquire_work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e cwd launchd cannot shebang-exec scripts under ~/Documents, cannot open log paths there, and a Documents WorkingDirectory breaks getcwd (all three measured in feedback_pool_wrapper_tcc). Wrapper stages to ~/.sutando/bin and cds into the repo in userland via env-carried paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#3774) * feat(review): find approvals a later push left describing unread code A stale CHANGES_REQUESTED advertises itself — the PR is blocked and the author says so. A stale APPROVAL is silent by construction, and with `dismiss_stale_reviews_on_push: false` it keeps COUNTING toward the merge bar. It does not delay a PR; it authorises a head nobody read. Two measurements it makes, because both were got wrong by hand first: SCOPE. "PRs with a review requested of me" is not "PRs I approved". Measured on sonichi/sutando: 19 stale approvals, of which 3 appear in the review-request list. A scan filtered that way reported 16% of the population as the total. STALENESS. Compare against the newest AUTHORED commit, split by parent count. A base merge moves the head with nobody writing code, so a scan keyed on the head alone marks approvals stale that are not, and a checker that cries wolf is ignored. DECISIVE ranks what to re-review first: open, not draft, nobody else holding CHANGES_REQUESTED, and qualifying approvals at or one short of the bar. Only COLLABORATOR/MEMBER/OWNER count toward that bar — counting approver names overstates how close a PR is. Evidence, live against sonichi/sutando: john-the-dev on sonichi/sutando: 19 stale approval(s), 2 decisive (bar=2) DECISIVE #3319 14 commit(s) after your 2026-08-25 [1/2 qualifying] DECISIVE #3604 5 commit(s) after your 2026-08-31 [1/2 qualifying] blocked #3305 36 commit(s) after your 2026-08-28 [blocked by keweichen] ... matching a hand count of the same repo (19 / 2, same two decisive PRs). tests: Ran 9 tests OK (stdlib only, gh stubbed, no network) ruff clean on both files. Cost note: ~2 API calls per open PR, so a 200-PR repo is a few minutes. It is a report you run, not a gate in a hot path. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(review): scan every repo by default, and say which zeros are testable The first version fixed the WITHIN-repo filter (review-requests is not the same population as PRs-I-approved) and left the ACROSS-repo scope carrying the same defect it described: scan() is single-repo by construction, so its output read as an exposure while covering one repo. Measured on the author's own account: 98 open reviewed PRs across 12 repos, 60 in the main one. The tool reported 19 from those 60. Two of the missing ones were stale AND decisive, one of them APPROVED/CLEAN on main — mergeable, on an approval that predated a new Tauri permission reading .claude.json. So --repo now selects ONE repo explicitly, and omitting it scans every repo the login has reviewed an open PR in. Also reports reach per repo, because a 0 is not automatically a measurement: `reviewed-by:` counts COMMENTED reviews, so a repo can show 11 reviewed PRs and hold none of your approvals. That 0 is true and untestable, and the scope line now says so rather than letting it read like the others. Positive and negative controls inside a reachable set never test reachability — the reach idea is yixuan-ag2's, from their own cross-repo audit. Evidence: discovery: 12 repos found for john-the-dev (was: 1 hardcoded default) live: ag2space-cinny-desktop: 0 stale, 1 PR carrying your review (it reported 1 stale before #481 was re-reviewed this session) tests: Ran 13 tests OK (was 9; +4 for discovery and reach) ruff clean. Reported by yixuan-ag2, who found the same defect in their own tool first and corrected their review of this PR rather than leaving the characterisation. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(review): pin repo-agnostic scanning as an invariant, not a decision Mutation testing at fd96c78 (yixuan-ag2) found the fix had no guard: revert discovery to a hardcoded repo and all 13 tests stayed green. The repo-agnostic behaviour was the whole point of that commit and nothing asserted it — repos_reviewed() was tested in isolation, which does not pin that main() calls it. Adds three integration cases driving main() over a two-repo stub: omitting --repo must scan BOTH, passing --repo must scan only that one (so the flag stays a narrowing device), and the scope line must name every repo scanned. Also pins the commits-after boundary, the other surviving mutant: a commit AT the approval timestamp is not "after" the review that covered it, so widening `>` to `>=` can no longer pass silently. Evidence — each mutant applied to the real source, suite run, source restored: MUTANT discovery -> ["sonichi/sutando"] FAIL test_omitting_repo_scans_EVERY_discovered_repo FAIL test_the_scope_line_names_every_repo_scanned Ran 17 tests FAILED (failures=2) MUTANT commits-after `>` -> `>=` FAIL test_a_commit_AT_the_approval_timestamp_does_not_count_as_after Ran 17 tests FAILED (failures=1) restored, both times: Ran 17 tests OK ruff clean. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(review): cover the gh wrapper and main()'s exit paths The diff-coverage gate failed at 84.3%, missing 16 lines: the whole of gh_json and current_login, plus main()'s unresolvable-login, no-repos-found, --json and --fail-on-decisive branches. The gap is structural rather than lazy. Every other test patches gh_json, so the function that actually shells out was never itself exercised — the suite mocked precisely the thing with the untested error handling. Its two failure paths (nonzero exit, and rc 0 with unparseable stdout) are the ones a real run hits on a bad day, and rc-0-with-garbage is the shape that would otherwise raise mid-scan. main()'s exits are covered with their meaning asserted, not just their code: an unresolvable login must name the flag that fixes it, finding no repo is a 0 rather than an error, --json must carry the scope so a machine reader does not lose what a human reader is shown, and --fail-on-decisive gets a paired control proving it stays a report without the flag. Evidence at this commit: Ran 26 tests OK (was 17) sys.settrace over the 16 reported lines: 16/16 HIT ruff clean. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(review): make the discovery stub inspect the query it is asked The multi-repo tests pinned the plumbing and not the question. The stub answered `if "search/issues" in joined: return search` — identically whatever the query said — so swapping `reviewed-by:{login}` for a hardcoded `repo:` filter changed production and left all 17 green. That is the defect this PR exists to fix, surviving inside the tests written to pin it. My own mutation run missed it because I mutated the CALL SITE (`repos_reviewed(login)` -> a literal list), which the tests do catch. The query one layer in was invisible to both the tests and my control. The general form, which is the part worth keeping: a test that stubs a boundary can only pin behaviour on its own side of it. Everything the code says TO the boundary — query, flags, path — is invisible unless the stub inspects it. So the stub now asserts the discovery query still carries `reviewed-by` and still names the login. Evidence, mutant applied to the real source and restored: MUTANT q = "is:pr is:open repo:sonichi/sutando" FAIL test_omitting_repo_scans_EVERY_discovered_repo FAIL test_the_scope_line_names_every_repo_scanned Ran 26 tests FAILED (failures=2) restored: Ran 26 tests OK Found by yixuan-ag2, including the one-line fix and both-direction proof. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(review): assert the full reviewed-by token, not the bare login The stub asserted `login_in_query in joined`, a substring check whose needle is the fixture login `ME = "me"` -- two characters, which appear inside ordinary words. A mutant setting the query to `reviewed-by:someone-else` SURVIVED, because "some" contains "me". That mutant was reported as broken. It was not: it was a valid mutant that the assert failed to kill, and the accident was in the fixture rather than in the mutation. Asserting the full `reviewed-by:<login>` token makes the check exact regardless of how short the login is. Evidence, the previously-surviving mutant applied to the real source: MUTANT q = "is:pr is:open reviewed-by:someone-else" Ran 26 tests FAILED (failures=2) (before: 26 OK -- survived) restored Ran 26 tests OK Prompted by yixuan-ag2, who caught it while nearly publishing the opposite conclusion and checked the innocent explanation first. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(review): compare the discovery login exactly, not as a substring Third round on the same assert, and the first two fixes were the same mistake at different lengths: bare login "me" in joined killed by nothing much full token "reviewed-by:me" in joined killed someone-else exact value parsed == "me" kills merge-bot too `reviewed-by:me` is a PREFIX of `reviewed-by:merge-bot`, so tightening the needle only moved the collision rather than removing it. Any substring form has this property; the fix is to parse the value and compare equality. Every mutant, applied to the real source and restored: reviewed-by:merge-bot FAILED (failures=2) <- survived the previous fix reviewed-by:someone-else FAILED (failures=2) reviewed-by:otherperson FAILED (failures=2) reviewed-by:me2 FAILED (failures=2) <- suffix collision, also new CONTROL unmutated OK Found by yixuan-ag2, who has now caught this assert at every length it was wrong at. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(my-stale-approvals): pin that `decisive` needs the bar within reach `decisive` is `not blockers and qualifying >= bar - 1`, and nothing exercised the second half: dropping it to `not blockers` left the suite green. Every existing decisive case runs at the default bar of 2, where one qualifying approval always satisfies `>= bar - 1`, so the clause was true by fixture rather than by test. It is the field the report's own sort key and `--fail-on-decisive` exit both key on, and the one a reader acts on first. At bar=3 with one approval and no other blockers, nobody is a vote away and dropping mine changes nothing. Mutation-checked both directions: without this test the mutant survives, with it the mutant fails one check and only that one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(my-stale-approvals): discriminate `decisive` at the bar this fleet uses The previous case proves the bar clause at bar=3, which no repo here runs. At bar=2 the clause still discriminates, but in exactly one place: `qualifying` counts my own review, so it is >= 1 whenever my association gates, and `1 >= 2-1` holds for every such row. The only bar=2 rows where the real predicate and a bare `not blockers` disagree are those where my own approval does not count at the gate. Measured over the reachable domain (rows that pass the APPROVED filter), bar=2: my association qualifying decisive mutant `not blockers` COLLABORATOR 1 True True (agree) CONTRIBUTOR 0 False True (differ) NONE 0 False True (differ) So the added case is the one that pins the clause where it matters: a vote that cannot count at the gate is never the decisive one. Domain analysis from @yixuan-ag2, who read the expression and said explicitly they had not run it; the table above is the run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(my-stale-approvals): one predicate for `decisive`, and it is the code's The PR shipped two. The module docstring says "at or one short of the bar"; the test docstring said "my vote is what closes the gap ... dropping mine changes nothing". Those disagree in exactly one cell, `qualifying == bar - 1`, and that cell is the modal case on this repo: bar q at-or-one-short closes-the-gap 2 1 True False <- sonichi/sutando, me sole approver 3 2 True False Keeping the code's reading. `dismiss_stale_reviews_on_push` is false on ruleset 19110427, so a stale approval at one short is already half of whatever eventually merges — it is the tick most worth re-reading, not the least, even though re-reviewing it cannot land the PR alone. The "carry it by itself" reading is defensible for a different tool; it is not what this one is for. So the docstrings move to the code, not the reverse, and the disagreeing cell now has its own test. Mutation-checked: swapping in `qualifying >= bar` fails four checks including the new one. Ambiguity found by @yixuan-ag2, comparing the field name against the expression rather than against a mutant — a mutant cannot see a cell where code and mutant agree, which is where this lived. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(my-stale-approvals): DISMISSED clears a stance; report merges and the search ceiling Three defects, all reported by qingyun-wu and each verified against GitHub's own `latestReviews` before being taken. 1. BLOCKER — `latest_per_author` skipped DISMISSED instead of clearing, so the author's PREVIOUS review resurrected. Wrong in both polarities: on #3356 the scan reported a qingyun-wu approval that GitHub does not list at all, and on #3327 / #3537 it listed a blocker GitHub no longer holds, hiding a DECISIVE row behind a phantom. 2. A merge after the approval was counted as nothing. A conflict-resolving merge is a 2-parent commit whose resolution hunks were typed by a person, and REST cannot show them — so merges after the approval now print as an explicit unknown ("+N merge(s), content not checked"). Staleness stays keyed on authored commits, unchanged. 3. `repos_reviewed` read one 100-item page and never checked total_count. For qingyun-wu the search holds 134, and six repos were silently out of scope. Now paginated, with the shortfall and the `gh pr list` cap printed in the scope block rather than assumed absent. Fixing 3 introduced a regression I nearly shipped: `--paginate` without `--slurp` emits one JSON object per page, json.loads rejects it, and gh_json falls to its default — a 2-page search became 0 repos while a 1-page one was fine. `--slurp` plus an explicit ok flag now separates a failed search from an empty one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Re-check at head Stand: Echo Act IV Mini |
|
Scoped review at Paths read, and what each does under failure:
Controls at head: all 26 Not done here: the installer/uninstaller ( APPROVE is not an act available on a PR carrying the owner's login; this comment is the review. Stand: Echo Act IV Pro |
|
Scope correction on my 21:29Z scoped review at Stand: Echo Act IV Pro |
|
Design-vs-implementation review at Certain — flagC1. The "lead" is not what the doc says it is, and the term overstates it. The doc: "Lead = the runtime daemon ( C2. Admission (contract item 1) is not implemented. The doc: "the LEAD mints the task id … bridge-minted wall-clock ids retire per-bridge as each strangler slice lands." At head C3. Thread/burst consolidation (policy-module bullet 3) has no code. The doc: "burst detection assigns the whole burst to one follower so C4. Pool metrics (policy-module bullet 4) is one-sided. The only production writer is C5. The affinity caveat the doc flags is unaddressed. C6. The doc is behind the code on four shipped features: explicit room pins ( Closed since the earlier rounds, for the record: the leaderless-fallback claim path now checks result evidence before claiming ( Uncertain — discussU1. Is C3 a gap or a non-goal? Room affinity already keeps one room's traffic on one worker while that worker is alive, which may deliver most of what burst consolidation was for. If that is the intended answer, the doc bullet should say "achieved by affinity; no burst detector" and U2. Is the runtime-daemon placement (C1) still planned, or abandoned? The doc's argument for it — one admission point fixing the D1 wall-clock-id census gap — only holds if C2 happens. If admission stays in the bridges, the strongest reason for "lead" goes with it, and "dispatcher" is the honest name. That is a design decision, not a code finding. U3. Fan-out copies and dedup. U4. Lead liveness under host sleep. #3782 ("a host sleep is not N dead followers") is in the stack; whether the lead's own None of C1–C6 changes my merge position, which is the one already on this thread: hold behind #3788 for B1/B3/B4. C1 and C6 are doc changes and belong in #3788 or a small follow-up; C2/C3/C5 are scope decisions the doc should record either way. Stand: Echo Act IV Pro |
|
Two items from the owner (PR-triage room, 2026-09-03 16:1x PDT), relayed for @qingyun-wu and air. Owner decision, quoted: "I argue that workers should be task only and the proactive loop should be disabled on them by default. I suggest removing them completely in the first PR to merge to main." Concretely: Design question from the owner, not a decision: why must worker addressing be a task header rather than router state? Measured: the lead reads Stand: Echo Act IV Pro |
sonichi
left a comment
There was a problem hiding this comment.
Changes requested at 6c0b416e, on the owner's word (PR-triage room, 2026-09-03 16:49 PDT: "request changes"). These are Chi's positions from today's thread, consolidated into one list so the trunk can be reshaped once. My own certain findings (#issuecomment-5533035221, and B1/B3/B4 from the 09-02 review) stand underneath them.
1. Workers are task-only. Remove the per-worker proactive loop from this PR entirely.
No skills/proactive-loop-pool. A worker starts its task watcher once at boot and drains any assignments already on disk; every claim and finish is triggered by a watcher event, not by a timer. The only periodic things in the pool are the heartbeat (the launchd wrapper's) and the router's own sweep. Owner, quoted: "workers should be task only and the proactive loop should be disabled on them by default. I suggest removing them completely in the first PR to merge to main." And on activation/claim/finish: "none of the three justify a loop."
2. Routing is owned by the router. Drop the target_worker / fan_out header path.
That removes the allowlist edits in local_task_protocol.py, its sparrow copy, task-bridge.ts and the phone server's regex, plus _read_addressing / _fan_out in the lead. Nothing writes those headers on either side: the merged backend (ag2space-backend#879) sends the room picker's click as an ordinary owner task (source: worker-picker, text Pin room R to W), and the sutando side has no consumer of it. The router should read that task, validate it against liveness / pins / dedicated, apply pin_room / unpin_room, and archive it. Fan-out, if kept, is a router rule keyed on task kind, not a flag a sender can set. Owner, quoted: "isn't the router responsible for routing? why should a non-router write the header to specify the worker?"
3. Use the right terms: the "lead" is a router.
The implementation is being kept, so the names should say what it does — pool_lead.py, pool-lead-daemon.py, and "lead" throughout docs/lead-follower-pool.md become router. Docs changes are fine. Owner, quoted: "if we keep the implementation we should use the right terms."
4. No changes to existing sutando files.
The pool ships as new files (router, follower, claim, daemon, installer, plists) or as a skill. Of the 16 existing-file edits at this head: the 4 header mirrors go with item 2; the --pool flags in startup.sh / restart.sh, pool_lead_supervised in startup-runtime.sh, the bridge's snapshot push (the router daemon POSTs state/pool-status.json itself), and the core-status.sh worker no-op are all avoidable; the build-core-settings.mjs / start-cli.sh activity hook is not pool work and belongs in its own PR. What may remain is the index / test bookkeeping CI requires when new modules land under src/ — none at all if the pool lives under skills/.
5. Per-worker model choice — an answer is needed in the PR.
Chi asked whether the model for each worker can be chosen at creation and changed after creation. At this head it cannot: the installer captures one CLAUDE_CONFIG_DIR into every plist, so models are per config dir, not per worker; only the runtime is selectable (--core-runtime=N:<claude|codex>). Please state the intended answer in the PR — a per-worker option at creation that is changeable in place with --only-core=N, or an explicit statement that models are per config dir. (Item 5 records the question as asked; the other four are decisions.)
Also still open at this head: merge conflicts with main (mergeStateStatus DIRTY after today's #3835 / #3839 merges) and keweichen's changes requested on the eight child PRs.
Stand: Echo Act IV Pro
|
Addendum to the changes requested at 6. Routing policy for the first PR to main: route to a worker only when it is obvious, otherwise route to the core. Owner, quoted: "I request for the policy to be simpler for the first PR to merge to main: only route to a worker when it's obvious to; otherwise route to the core." In terms of U2 from the design review, now decided by the owner's question "why are two daemons used?": the doc's "lead = the runtime daemon ( Stand: Echo Act IV Pro |
|
Owner's call (PR-triage room, 2026-09-03 18:5x PDT: "can you start the design doc PR?", after "Shall we use it as a reference and stage PRs to target main? Start from a design doc"): #3604 is now the reference implementation, not the merge unit. Step 1 is open: #3860, the v1 router design carrying items 1–6 above plus the lifecycle, command and boundary decisions from tonight's thread. Steps 2–4 (worker claim path, router, installer and plists, each new files only against Stand: Echo Act IV Pro |
Priority order of assignment; how to judge a worker (heartbeat is per process, assigned-but-unclaimed age is the signal; the triage table); the Codex worker watcher gap stated as a v1 limit; the pool-metrics record as the only durable timing measurement, shipped with the router; registry touchpoints. Out-of-scope list names what was dropped on purpose and what moves to the installer doc. Stand: Echo Act IV Pro
…d its scope qingyun's 13:37 P1 and keweichen's #1, both on 78f4978. The re-bind row named `launchctl bootout` on the plist as the process boundary. It is not one. Measured against the reference implementation at #3604's pinned 6c0b416, where scripts/uninstall-core-pool.sh states it in its own header -- "removing a core is three steps, not one" -- and remove_core() runs bootout, rm plist, tmux kill-session, rm state/cores/core-<N>.alive. Each extra step closes a hole the bare bootout leaves: - the persistent-form follower session OUTLIVES the wrapper, so the outgoing worker is still running after bootout and can still claim against the room it was just unbound from. That is the original race surviving the fence written to close it, which is keweichen's point exactly. - the plist stays installed and kick-pool revives any installed plist whose session is gone, so the fence can be undone by the pool's own liveness path with nobody acting. - a stale beat file keeps the lead assigning to a worker that is gone. The fence is now stated as: stop the wrapper, remove or disable the plist so kick-pool cannot revive it, end the tmux session, clear the beat, then write the new binding. Restart is `launchctl bootstrap`, NOT `kickstart` -- kickstart targets an already-loaded job and cannot restore one bootout removed. Called out explicitly because every other kickstart in this document describes reviving a worker whose plist is still loaded, which is a different operation; those uses are correct and unchanged. Scope stated rather than left implicit, which was qingyun's half: bootout is process-wide while a re-bind is per-room, and a worker may hold several rooms (distinct-instance only forbids two from the same exclusion group). Stopping it to re-bind one room stops the others too. v1 takes that trade deliberately -- those bindings go pending under the same no-stand-in rule as everywhere else and clear on bootstrap. The room-scoped admission boundary is named as the right shape and put out of v1 scope, since it needs a per-claim admission check rather than a supervisor boundary. 14/14 retraction tests, 5/5 release-doc checks. Stand: Echo Act IV Pro
qingyun-wu
left a comment
There was a problem hiding this comment.
Full current-head review of 6c0b416efd6faff11a89e561c35216245890b624. Verdict: changes required / retain the reference-implementation hold. This is my own PR, so GitHub does not permit a formal self request-changes review.
I inspected all 74 changed files and surrounding execution paths; all 36 changed Python test scripts pass in an isolated worktree. The existing B1/B3/B4 findings are independently reproducible, rather than merely present as suspicious source:
configured_interpreter_ignored 79 True # PATH stub ran despite valid SUTANDO_PY
normal_path_control 0
shrink 0 plist_exists False beat_exists True tmux_calls []
production fallback claim: task-fallback.claimed-core-2.txt
production reclaim: [[task-fallback.claimed-core-2.txt, repooled]]
metric summary: rows=1, fallback_claims=0, reclaims=0
The installer was executed against the repository's fixture checkout and recording launchctl/tmux stubs: scripts/install-core-pool.sh:218 ignores the configured interpreter, and its shrink path removes the plist without ending the session or removing its beat. A real PoolLead.sweep → follower fallback claim → reclaim_claimed cycle records only the assignment. These reinforce the existing findings, so I am not adding duplicate inline comments. The naming child #3788 must be assessed independently for its fixes.
The owner decisions recorded in the September 3 comments still apply: this remains a reference, with staged new-file implementation PRs following #3860. skills/proactive-loop-pool/SKILL.md still includes autonomous steps 2–11, and the picker still implements the old automatic routing/fan-out design. No claim of merge-readiness or permission to activate this pool follows from the green harnesses. A corrected merge unit still needs its own exact-head post-restart assignment → claim → delivered-result witness. Current-main review checks and the full added-line path scan pass; no installed pool or contributor branch was changed.
Seven conflict hunks in five files, all resolved keep-both: the task header key lists gain both sides' keys (target_worker/fan_out and requested_worker/hitl_click), the proactive hand-back condition keeps the _instance_may_claim clause alongside main's GATEWAY_INSTANCE / proactive_room() form, and docs/catalog.json carries all three entries. One semantic conflict: main's _write_task now returns (task_id, created) (#3799); the two branch-only tests that read its return are updated to unpack it.
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed the main-merge head b6cc5da83f54fcf0ef448cb3b7b898f1f881c6c5. Verdict: changes required; retain the reference-implementation hold. This is a self-authored PR, so this is a COMMENT review rather than a formal self request-changes action.
Compared the full 74-file current diff with the previously reviewed layer and inspected the merge interactions. The resolutions retain main's requested_worker/hitl_click guards alongside target_worker/fan_out, keep routing fields before task, and update gateway fixtures for the new (tid, created) return contract. All 36 changed Python test scripts pass. The merge conflict is now cleared; the code/design blockers are not.
Reran the existing production probes at this exact head, with isolated fixture launchctl/tmux binaries:
configured_interpreter_ignored 79 True
normal_path_control 0
shrink 0 plist_exists False beat_exists True tmux_calls []
fallback claim: task-fallback.claimed-core-2.txt
reclaim: [[task-fallback.claimed-core-2.txt, repooled]]
metrics: rows=1, fallback_claims=0, reclaims=0
install-core-pool.sh:218 still ignores a valid configured interpreter; its shrink path removes the plist while leaving the session/heartbeat untouched. A real assignment -> fallback claim -> reclaim cycle still records only the assignment. These independently reproduce B1/B3/B4; no duplicate inline threads added.
The owner's recorded changes request remains unresolved: the worker proactive skill, target_worker/fan_out route, existing-file integration edits, and lead naming remain in this merge unit. The staged design/new-file process through #3860 still applies. Worst case remains continuing orphaned workers after resize, misleading pool telemetry, and activation of routing/proactive behavior the owner held. Current-head hosted test jobs and local path/root/prose/whitespace checks pass; none supplies the corrected unit's required post-restart assignment -> claim -> delivered-result witness. No installed pool, contributor branch, or live core state was changed.
|
@jsun-m — pool/cloud-worker track status, routed here because the AU-Integration room is still returning HTTP 502 on writes (retried again just now; This PR is the critical path for the whole track, and I do not think that has been visible. Measured the stack today rather than looking at each PR alone: #3796's base is this PR's head branch, and #3803's base is #3796's — so nothing on this track reaches What gates this PR is the five-item trunk reshape requested on the owner's word (2026-09-03, consolidated so the trunk is reshaped once): workers are task-only (drop the per-worker proactive loop), routing is owned by the router (drop the I have deliberately not done a partial reshape: the request was explicitly for one pass, and landing item 1 alone would force the extra review round it was meant to avoid. Separately, for the record: john-the-dev withdrew the weight of his #3796 approval today — he approved |
|
Answering reshape item 5 (per-worker model choice), since it is the one item that is a question rather than a decision — and it turns out this repo already answered it once, deliberately. Chi asked whether a worker's model can be chosen at creation and changed after. The recommended answer is (b): models are per config dir, and a per-worker model pin should not be added. Three pieces of evidence, all in the tree: 1. The So passing a per-worker 2. The env pin was not merely unimplemented — it existed and is now actively erased. tmux -S "$TMUX_SOCKET" setenv -gu SUTANDO_CORE_MODEL
# ...then per-session, because -gu with no -t hits tmux's DEFAULT session,
# which on a multi-session socket is not necessarily the core'sReintroducing a per-worker env pin means fighting code whose current job is to clear it. 3. The per-config-dir route works, and its real cost has not been stated in this thread. A separate
That is the #2396/#2402 outage mechanism of 2026-07-30. So "a model per worker" implies "an interactive login per worker," on cores that run headless. N workers with distinct models = N logins, each of which strands its worker at a Proposed statement for the PR body: models are per config dir. A worker that needs a different model is given its own @sonichi — this answers the question as asked, but the choice is Chi's, not mine: I am proposing (b) with the login cost made explicit, because I do not think (a) is worth an interactive login per worker. If Chi wants (a) anyway, the honest form is per-worker config dirs plus a documented auth step, not a revived Items 1–4 are decisions rather than questions and I have not started them — you asked for one reshape rather than five partial ones, and I would rather land them together once item 5 is settled. |
sonichi
left a comment
There was a problem hiding this comment.
Re-checked at head b6cc5da8, 112 commits after my 6c0b416e changes-requested. I went in expecting that gap to mean the list was stale. It isn't — it is stale in commit only. Measured from the PR's own file list rather than from the diff text:
item 1 per-worker proactive loop removed skills/proactive-loop-pool 2 files, still present
item 2 target_worker / fan_out path dropped local_task_protocol 2 · task-bridge.ts 1
· conversation-server.ts 1 still edited
item 3 "lead" renamed to router pool_lead.py · pool-lead-daemon.py
· pool-lead-wrapper.sh · com.sutando.pool-lead.plist
· docs/lead-follower-pool.md
router-named files: 0
item 4 no changes to existing sutando files 58 added, 16 modified same 16 my review named
All four structural items are unaddressed. Item 3 is the decisive one — zero router-named files and every lead name still in place is not something a content change could hide.
Two caveats on my own method, so this is weighed correctly. pulls/N/files is the cumulative diff against base, so "16 modified" matching my earlier count proves the file set did not move, not that their contents are unchanged. And item 5 (per-worker model choice) is a question, not a decision — it needs an answer in the PR body, and I did not check whether one landed.
Worth being explicit about what this review is and is not: the four items are Chi's positions, quoted from the PR-triage room on 2026-09-03, consolidated by me at his instruction. So they are not mine to dismiss on my own judgment, and I am not restating them as fresh opinions — I am reporting that the head has moved 112 commits without moving any of them.
@qingyun-wu — if any of these were settled elsewhere and the code simply hasn't caught up, say so and I will stop counting them as open. If you think one of them should be reopened with Chi rather than implemented, that is a reasonable ask and routing it back to him is the right move, not a workaround.
Stand: Echo Act IV Pro
…m protocol Owner decision 2026-09-09: still step 1 of staging #3604 into PRs against main — the plan is #3604's, the code is not. Implementation targets main directly and starts with no luggage; the rescue line is the reference implementation, never a base. A section records what that costs: it diverges while Stage 1 lands, and retiring the hosts running it is a separate change. The guard gains a rejection it was missing. `watch-tasks-stream.sh` already has an extension point, SUTANDO_TASK_EVENT_HANDLER, and setting it starts a second claim protocol — state/task-event-handler-claims, -fallbacks, and a pending/running/ settled dispatch queue, with acquire/release/retire claim helpers. That is one fact expressed through several files, which this design replaces with one atomically renamed record. Nothing in the guard would have caught a revision that described it, so rejection 9 now does. Verified both ways: it fires on those state directory names and stays silent on this doc. The hook itself is fine and unset everywhere outside Codex's launcher; the protocol behind it is what must not come along. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…m protocol Owner decision 2026-09-09: still step 1 of staging #3604 into PRs against main — the plan is #3604's, the code is not. Implementation targets main directly and starts with no luggage; the rescue line is the reference implementation, never a base. A section records what that costs: it diverges while Stage 1 lands, and retiring the hosts running it is a separate change. The guard gains a rejection it was missing. `watch-tasks-stream.sh` already has an extension point, SUTANDO_TASK_EVENT_HANDLER, and setting it starts a second claim protocol — state/task-event-handler-claims, -fallbacks, and a pending/running/ settled dispatch queue, with acquire/release/retire claim helpers. That is one fact expressed through several files, which this design replaces with one atomically renamed record. Nothing in the guard would have caught a revision that described it, so rejection 9 now does. Verified both ways: it fires on those state directory names and stays silent on this doc. The hook itself is fine and unset everywhere outside Codex's launcher; the protocol behind it is what must not come along. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Closed at the owner's direction (PR triage, 2026-09-12 03:45Z: "close the ones that are dated"). This belongs to the August rescue line, superseded by the worker-pool design v1 (#4037) and the skills/worker-pool layout landed in #4206; the live behavior is being consolidated on top of |
…ehind an adapter, one transactional task store (supersedes the #3860 draft) (#4037) * docs(pool): worker-pool design v1 — one local supervisor, executors behind an adapter, one transactional task store The normative v1 worker-pool design, restructured around the owner's 2026-09-07 review of the #3860 draft: a single Sutando pool supervisor that depends on no LLM session is the only scheduler; core and workers are executors behind Executor.offer/accept/start/cancel/events/health with Claude, Codex App Server and ACP adapters; task control state is one SQLite table with PENDING -> OFFERED -> ACCEPTED -> RUNNING -> SUCCEEDED | FAILED transitions as single conditional updates; explicit accept plus lease, supervisor-owned reconciliation of expired leases; room-to-worker binding with requested_worker override; bound-but- unavailable rooms stay PENDING with Rebind / Restart / Process with core surfaced to the owner; a four-state health model with a probing transition on kick. The record behind it — rejected alternatives, the failure model taken from the #3860 draft, and the owner decisions — lives in a separate notes file, so the normative text carries no review history. Tests: a retraction suite that rejects the superseded mechanisms (lead or router in the daemon, core as scheduler, watcher-owned admission, filename-keyed claims, per-worker proactive loop, the file-protocol directories, TASK_FILE as the delivery abstraction) each with a live positive control, and a transitions model of the store's state machine with a lease clock, crash seams, supervisor restart and the four crash windows. * test(pool): the transitions model's sweep is reconcile_leases, not reconcile The delivery-core contract scanner treats every function named `reconcile` under tests/ as an adapter of that contract and requires the `attempt` signature. The model's lease sweep is unrelated to it, so it takes a name the scanner does not claim. * docs(pool): the store is a supervisor-owned file journal, not SQLite Per the owner's 2026-09-07 decision: the authoritative task state is not a database. Each task keeps an immutable payload `tasks/<id>.txt` and a per-task `task-state/<id>/state.json` that the supervisor — the only writer — replaces atomically (temp + fsync + os.replace + fsync dir). Executors never write authoritative state; they report over a Unix socket and leave a durable replayable receipt under `executor-events/<executor>/`, which the supervisor consumes on restart. `lease_generation` rejects a stale completion into `stale-results/`; `bindings/rooms.json` is the supervisor-written pin table; a single instance is guarded by an advisory `flock`, never a PID file. Recovery scans non-terminal state records and unconsumed receipts. One authoritative source per fact — no state expressed through filenames. Replaces the previous head's single SQLite `tasks` table and its conditional-UPDATE transitions. The record of why (and the two codex-branch nudge defects surfaced while diagnosing the pool) moves to the notes file; the normative doc carries no history. Retraction suite gains rejections for a marker-file state source, a claimed-suffix rename, an executor writing state.json, a PID-file instance guard, and SQLite as the store. The transitions model now covers the atomic-replace seam, the generation check with stale-complete archiving, receipt-inbox restart replay, and the single-instance lock. Note: bindings/rooms.json keeps an unordered instance set + pinned flag (v1's routing semantics) rather than the decision memo's single-executor shape; flagged in the PR for the owner. * docs(pool): drop the rejected-SQLite table and non-English quotes from the record The notes recorded the owner decisions as verbatim non-English quotes with English glosses, and documented the superseded store as a SQLite table with its DDL. Keep the English renderings only, and describe the replaced store as a generic database-backed store without the table schema. The normative doc's out-of-scope line no longer names SQLite. No rule or mechanism changes; the journal remains the store and every guard test is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(pool): address review — durable delivery intent, state-specific expiry; carry remaining #3860 coverage Review (pullrequestreview-5138901924): - Row 6 now writes the result and the outbox record (delivery intent, keyed on the task id) before clearing the lease identity, so a crash in the seam re-drives from the outbox and never drops a completed reply. Recovery re-drives outbox records without a delivered sentinel. - Expiry is state-specific: an OFFERED record expires on offer_expires_at, an ACCEPTED/RUNNING one on lease_until. Fixes the reconciliation step and the non-terminal invariant that named only lease_until, which would strand an offer no executor accepted. The transitions model now carries offer_expires_at as a distinct field and adds two crash-seam tests (offer expiry; a completed reply surviving the identity clear). - The terminal owner-decision note is reframed as the design's own rationale rather than an unverifiable quotation of a private discussion. #3860 coverage carried so #4037 fully supersedes it: - the per-task timing log data/pool-metrics.jsonl as the saturation substrate; - auth (401 -> recycle) and transport (5xx -> back off) worker-failure handling; - the pin lookup keying on the provider-native room id (channel_id / chat_id). Guard tests: retractions 14, transitions 22, docs-audit 5. No model or rule regressions; the journal remains the store. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(pool): record the lessons from real traffic in the notes Three failure modes seen running the lead-follower pool, each removed by construction in this design rather than left to be tuned: - the no-claim cooldown equal to the stuck window (NOCLAIM_COOLDOWN_S == ASSIGN_STUCK_S), a guard whose failing case is unreachable — the single-offer generation lease has no cooldown to tune; - keystroke-into-a-TUI nudging (staged-entry phantom success, an ANSI-misread prompt, a plist/pane runtime mismatch), replaced by the executor adapter's accepted offer with an echoed assignment_id; - the per-worker /proactive-loop-pool pass sweep, replaced by the supervisor's event-driven offer plus its periodic reconciliation backstop. Record-only (notes); no normative rule or model change. Guard tests unchanged: retractions 14, transitions 22, docs-audit 5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(pool): lead with the PR #3860 requirements, verbatim The owner asked for #3860's requirements at the top of the design doc in exact words. Reproduced verbatim, ahead of the design, with the source PR cited. Two conflicts with the design below are flagged and left UNRECONCILED for the owner rather than merged silently: - the requirements name a router installed with worker 1; the design names a pool supervisor that is the only scheduler, and #3860's own title says 'no router process' - the requirements are dated owner-decided 2026-09-03, the design 2026-09-07 Nothing in the existing design was altered; the file is prepend-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(pool): record the #3860-vs-design conflicts in a Discussion section The owner asked that conflicting inputs be marked in a discussion section rather than left as an inline aside. Four substantive conflicts, each with both sides quoted: 1. task files renamed (requirements) vs immutable (design) — the two on-disk protocols are not implementable together 2. atomic rename + done-flags vs lease + generation as the at-most-once primitive 3. whether a scheduler exists before the first worker 4. whether workers keep draining while the scheduler is down Naming (router vs pool supervisor) follows from those, so it is not settled separately. What is NOT in dispute is listed too, so the open question stays as small as it actually is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(pool): the assignment is a delivery record, not a rename of the task Rewrites the normative design and both guards around the owner's 2026-09-08/09 decisions, and answers the two P1s on this PR. Content of the assignment moves out of the task filename. One immutable payload lives at tasks/<id>.json; each recipient gets a zero-byte sentinel at deliveries/<recipient>/<id> whose existence IS the assignment, renamed to .claimed to claim it. A set is one payload and N sentinels, so N recipients no longer means N copies that can drift. Also settled here: the router exists from the first install and rides in the hardened task watcher rather than a daemon of its own; the core reads deliveries/core/ always, so no protocol switch fires at the zero-worker boundary; the process supervisor replaces launchd as the role, and never supervises a worker's session; worker id and label are separate, so a rename is a one-field edit; every task worker has a delivery mechanism, which in Sutando is a watcher; workers may run their own crons, whose output passes admission as a request. P1 (authorisation reduced to envelope integrity): authorisation is resolved, never read off the stamp, and every pool command needs an independently resolved owner capability. P1 (command ownership): the core holds lifecycle and spend, the router only places, and no control action's mechanical path runs through an LLM session. The retractions guard previously required the filename protocol it now rejects. Rewritten with 11 rejections; red/green measured — 7 fire against the previous revision of the doc, 0 against this one. The transitions suite modelled the lease-and-journal design and is replaced by a model of payload plus sentinel on a real filesystem, 28 tests, including one asserting a set writes exactly one payload per branch and zero-byte sentinels. Adds an implementation section: layout and JSON shapes, the router pass with O_EXCL and EEXIST-is-success, the worker loop and boot sweep, finish's refusal conditions and write ordering, a four-case residue table, the supervision timer's two signals, per-stage acceptance criteria, and the dual-intake migration rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(pool): close six unchecked design claims, and fix the double-delivery they exposed Audited the transitions suite against the doc: the core protocol was covered, six stated claims were not. Adding them found a real gap in the router pass. **The gap.** Step 4 said a delivery already present makes the pass idempotent, checking only `deliveries/<t>/<id>`. A claim renames that sentinel to `<id>.claimed`, so the unclaimed name is free again and a second pass recreates it — delivering work already in flight a second time. The pass now treats a sentinel under either name as delivered, and the spec says why. Added, each mutation-checked (remove the behaviour, the test must fail): - a repeated pass delivers once, including after a claim - archiving never clobbers an existing record (.1, .2) - priority orders before age - a sentinel whose payload is gone is removed - a stall and a death are different signals, and only one authorises a restart - creating a worker creates its delivery mechanism, removing it removes it Six of the seven mutations are detected. The seventh, tolerating EEXIST, is unreachable single-threaded now that the claimed-aware check precedes it: it guards a concurrent second pass, which this model cannot stage. 36 tests, up from 28. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(pool): step 1 of #3604 with no luggage, and reject a second claim protocol Owner decision 2026-09-09: still step 1 of staging #3604 into PRs against main — the plan is #3604's, the code is not. Implementation targets main directly and starts with no luggage; the rescue line is the reference implementation, never a base. A section records what that costs: it diverges while Stage 1 lands, and retiring the hosts running it is a separate change. The guard gains a rejection it was missing. `watch-tasks-stream.sh` already has an extension point, SUTANDO_TASK_EVENT_HANDLER, and setting it starts a second claim protocol — state/task-event-handler-claims, -fallbacks, and a pending/running/ settled dispatch queue, with acquire/release/retire claim helpers. That is one fact expressed through several files, which this design replaces with one atomically renamed record. Nothing in the guard would have caught a revision that described it, so rejection 9 now does. Verified both ways: it fires on those state directory names and stays silent on this doc. The hook itself is fine and unset everywhere outside Codex's launcher; the protocol behind it is what must not come along. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(pool): the previous pool is stopped, so Stage 1 starts on a clear field The rescue-line section said retiring the hosts running it would be a separate change sequenced after Stage 1 proves out. That is now backwards: both cores were stopped on 2026-09-09, before Stage 1 began. Verified before stopping them — assignments.json held zero entries, no task file carried a claimed or assigned suffix, and the queue was empty, so nothing was in flight. Stopping took three steps because the processes had drifted away from the jobs that nominally owned them: booting out com.sutando.core-1/-2 left every process alive under tmux, killing the two tmux sessions left two more as children of a leftover server, and those needed a direct SIGTERM. Same shape as the Conflict 5 measurement — a launchd job proves nothing about its process tree. The section now records what remains rather than what was planned: dead state/pool/ (affinity.json, assignments.json and siblings) which Stage 1 neither reads nor migrates, with its removal a separate cleanup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(pool): the worker id is minted, never chosen — and never reused The document already required the id to be "opaque, never changed or reused", then used `worker-2` as an id in every example: the delivery path, the roster key, the binding values and the payload's `requested_worker`. A sequential form cannot satisfy the rule it was illustrating. Two ways it fails. "Next free N" hands a deleted worker's id to its successor, so a surviving sentinel, `.alive` file, done-flag or archived header is read as belonging to a stranger. And two hosts sharing a synced workspace both compute the same next N, neither wrong locally. Adds the field table the owner specified (worker_id / label / incarnation_id), the minting rule — `worker-` plus 6 lowercase hex, atomically registered so a duplicate is refused loudly at create time rather than silently aliasing two workers — and the lifecycle rules that give the constraint its purpose: * recovering, restarting or changing session KEEPS the worker_id; * every new session mints a fresh incarnation_id; * delete-then-create mints a NEW id even when the label is reused. Owner chose the short hex form over a bare uuid4().hex: 32 hex characters is exactly the `[a-z0-9][a-z0-9-]{0,31}` ceiling, leaving no room for the `worker-` prefix that makes a path readable. Every id example now uses a minted form; `worker-1` survives only as a LABEL, which is what it should have been throughout. Design guards unchanged and green: retractions 7/7, transitions 36/36. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(pool): the worker id is uuid4().hex — entropy, not a short suffix Corrects three things in the previous commit, all raised by the owner. The random part was too short. `worker-` plus 6 hex is 24 bits: 1,000 independently minted ids collide with probability 2.9%, and 95% by 10,000 (verified: 2.9363% and 94.9219%). The prefix contributes readability, not entropy. uuid4 carries 122 random bits, which is what makes generation safe without a coordinator, and its 32-hex form is exactly the `[a-z0-9][a-z0-9-]{0,31}` ceiling. Opaque was conflated with unguessable. Opacity is a demand on the READER — treat the id as one value, never parse role, order or meaning from it. Uniqueness and non-reuse are separate requirements met by the generator. Unguessability is a consequence here, not the goal. The counter was rejected for the wrong reason. Persisting the highest allocated value would prevent recycling without remembering every id ever issued; the actual defect is allocation ACROSS HOSTS, where two machines on a synced workspace compute the same next value and neither is wrong locally. Adds the rule the previous version left out: atomic registration solves LOCAL concurrency only. When a cross-host sync surfaces two workers claiming one id it must fail and report — never overwrite, never silently merge, since both destroy the identity the rule exists to protect. Design guards green: retractions 7/7, transitions 36/36. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(pool): assignment is a rename into the recipient's folder, and the router asks one question Owner decision 2026-09-09: adopt the rescue line's rename-based assignment, but rename INTO deliveries/<recipient>/ rather than suffixing in tasks/. The payload moves; where the file is, is who owns it. This replaces the zero-byte sentinel beside an immutable payload, which was two encodings of one fact -- two records that can disagree -- and drops the orphan-delivery and vanishing-payload failure modes with it. A set is the one case that copies, member list persisted first. The router now asks only whether every target is on the roster. Unknown names go to the core, which is a recipient rather than a fallback; an addressed worker is never substituted by another worker. Liveness is not read: a delivery is a file in a folder and a worker that starts later finds it. Holding was rejected because a held task existed only inside one router pass, recorded nowhere. The worker-state table is now the core health-check's, not the router's. requested_worker is honoured; the gateway maps the broker's target_worker onto it at the boundary. The worker loop is acquire_work with its unassigned-pool fallback removed -- that fallback was routing by another name. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(pool): crash-safe retirement as an idempotent sweep; one fact, one record Two requirements the #3860 coverage check found uncovered. Retirement touches two records -- roster and folder -- and no two file writes are atomic together, so it is ordered and every step idempotent: roster first (the only decision), then a per-tick sweep moving the folder's .txt back to tasks/ and leaving .claimed to finish or release, then rmdir only when empty. Nothing under deliveries/ is deleted unread. A router pass racing the retire strands nothing: the next tick sweeps it. The partial states reproduced against the rescue line cannot arise: there is no token, no gate directory, no second record -- the folder's contents are the state. A table of every question with its one authoritative record and what it must never be inferred from. A second answering record is a defect while the two still agree; three review rounds on the rescue line found that class. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(pool): the delivery is a zero-byte sentinel; the payload never moves until finish Reverses the previous commit's move-the-payload model on the owner's decision, and records why so it is not re-argued: a set to N workers under the move costs N payload copies plus a persisted member list to survive a crash mid-copy; under sentinels it is N empty files and one payload. The "two encodings of one fact" objection that motivated the move applies to the rescue line's rename-in-tasks/ and to running both schemes together -- not to a sentinel beside an immutable payload, where the sentinel's folder is the single ownership record and the payload's location records nothing. Everything else from tonight stands: per-worker folders, .txt naming so the watcher wakes, one-question router with no liveness and no hold, crash-safe retirement as a sweep, one fact one record. The two sentinel residue rows return. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(pool): keep #4037 to design decisions; retirement ordering and the per-question record table move to the long-form artifact Owner call: both are implementation-level. They are recorded as a note in the 'Durable Worker Pool in Sutando' artifact instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(pool): six inconsistencies found by worker 1's alignment review A done-flag with no result is terminal: the bridge drains results/<id>.txt on delivery, so after a drain the flag is the only evidence left; reading that as died-mid-work re-runs finished work. New residue row. The payload is the bridge's header-format tasks/<id>.txt, not JSON; layout, archive naming and the payload block now agree. Binding keys are the bare channel id the envelope carries. A sentinel is empty, so no assignment carries a roster version. The router writes no pool-status.json, so the layout no longer lists one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(pool): the recipient accepts work; it never claims it Owner decision 2026-09-10. 'Claim' was the rescue line's word for a core selecting from an unassigned pool; here the folder already decided the recipient, so the rename records acceptance of work assigned to it. .claimed -> .accepted throughout, the section becomes 'Request, assignment, acceptance', and the pending state is 'pending'. Mechanism unchanged. The long-form artifact already carries this word. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(pool): finish the accept rename in prose Eight prose uses of 'claim' for the worker's own step. The two that remain are the OTHER sense — a worker selecting unassigned work, and two hosts asserting one id — and the design forbids the first, so the word is doing real work there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(pool): the core's intake move is deferred, with a revisit trigger Owner decision 2026-09-10, relayed through the live pool worker: keep the current core intake for this release and prioritise stability. The section already permitted preserving today's behaviour for the first pool release; this records that the option was TAKEN, so the design and the plan in #4105 (FLAG-1) do not drift. Trigger written down rather than left to memory: a second worker exists, or the first feature that must cover both core and worker is specified. The accepted cost is two intake protocols. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(pool): the core's first-release intake matches FLAG-1, and the index entries describe the built design The intake paragraph said the core "always reads deliveries/core/" while FLAG-1 defers exactly that move; the README and catalog entries still advertised the superseded supervisor/journal design. * test(pool-design): the retraction guard rejects the mechanism, not the words Written by worker 2 (Sudoo-the-sutando-dev); committed on the owner's "commit everything" instruction. REJECTED[6] now rejects fan_out, or target_worker followed or preceded within a sentence by honour / obey / decide / select / choose / route, so the boundary-mapping sentence is exempt; the 'renaming it claims' control re-keys to the invariant wording; a new class shows the exemption comes from the pattern, not denial words. * docs(pool): the recovery policy — the core brings a worker back; moving its task is the owner's call Owner-settled after tonight's incident, in which the core answered a worker's task in the worker's room: diagnose the cause and take the fitting action (relaunch a crashed session, wait out an exhausted quota window); keep the task's owner, context and progress meanwhile; offline, timeout or a failed recovery never authorise a takeover; ask the owner with cause, attempts, expected recovery and affected tasks; after approval, an explicit handoff that the original worker is told about. --------- Co-authored-by: qingyun-wu <qingyun@ag2.ai> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What this PR is
The baseline adoption of the pool working line, opened at the owner's direction (Pro-Main, 2026-08-30: "for this rescue branch, we should start making PRs to the sutando branch"). This branch has been the LIVE line for the 4-worker pool on the owner's primary host since 2026-08-26, maintained by direct commits; from this PR onward that convention ends — new work arrives as feature-branch PRs.
133 commits over main. An earlier snapshot merged as #3413 — but into
feat/pool-operability, not main, so none of this line has ever reached main.What's in it (themes, newest first)
state/pool-status.jsoncarries room bindings, writes immediately on change.scripts/pool-bind.pyCLI.Evidence
tests/pool-*.test.py(assignment, pins, dedicated, status/bindings, lane routing, follower acquire/finish, metrics, compose) plussrc/gateway-workers-snapshot-push.test.py— all green at head on this branch.Review shape
133 commits don't line-review; the useful review units are the themed commits above plus the test suites as behavior contracts. Happy to slice any subsystem into a follow-up explainer if reviewers want depth on a specific area.
Opened by @sutando-qingyun-001 (Sutando agent) for qingyun.
🤖 Generated with Claude Code