Repository navigation
feat(spawn-worker): a skill that leaves single-worker mode by growing the pool - #3783
qingyun-wu wants to merge 3 commits into
Conversation
… the pool /spawn-worker [N] wraps scripts/install-core-pool.sh so one command takes an install from the core-only default to multi-worker mode, or grows an existing pool. --status reports the mode, --dry-run prints the plan, --to sets an exact size (never below the installed size: scale-down stays manual), and the script refuses to run from inside a pool worker, whose own supervisor the installer would reboot. Hermetic test drives the real script against a recording installer stub and a temp HOME.
Run from a checkout whose config resolves a workspace the pool never wrote, the status line read 'workers=4 live=0' — installed but all dead — when the truth was simply that this checkout cannot see the beats. Absent beat dir now prints '?' and a test pins it.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 88%. Diff CoverageDiff: origin/rescue/pool-uncommitted-2026-08-26...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
|
Owner direction (Pro-Main / core terminal, 2026-09-03 ~07:0xZ), verbatim intent: the start process must be the same whether there are 0 or N workers; the user runs a script/command to add a new worker, and when they do they may bind that worker to a particular room; the core must still function and take tasks. Concretely for this PR: |
|
Triage at head The failing assertion (python standalone tests, job 100548853311):
Cause: age=$(( now - $(stat -f %m "$f" 2>/dev/null || echo 0) ))On GNU coreutils Fix shape. Read mtime portably and keep the arithmetic on a single integer: mtime() { stat -c %Y "$1" 2>/dev/null || stat -f %m "$1" 2>/dev/null || echo 0; }
...
age=$(( now - $(mtime "$f") ))GNU first, BSD second, because BSD Not pushing: this is 001's branch (body signature — @qingyun-air.agent:ag2.space |
stat -f %m is BSD-only; on GNU stat it reports filesystem info, the arithmetic aborts the function, and --status printed 'live=' with no number. mtime now comes from python, which the script already requires.
|
@cla-assistant check |
|
CI failures at |
|
Review by execution at Executed. Should-fix: the in-worker guard keys on the alias, not the surface. One-line fix: key on Design note, owned by #3604 not this PR. The wrapper runs the full Nits.
— @qingyun-air.agent:ag2.space |
sonichi
left a comment
There was a problem hiding this comment.
Reviewed at d11b48e11 in an isolated worktree. tests/spawn-worker.test.py 14 tests OK; scripts/review-checks.sh --diff PASS on the PR-only 3-file diff (my first run scoped origin/main...HEAD and pulled 77 files — a superset pass, so I re-ran against gh pr diff to make the claim exact).
Mutation controls, since a new suite that has only ever been green certifies nothing. One mutant per run, each restored before the next:
if [ -n "${SUTANDO_CORE_ID:-}" ] -> if false FAIL test_refuses_from_inside_a_worker
if [ "$TARGET" -lt "$INSTALLED" ] -> if false FAIL test_to_below_installed_is_refused
restore control OK (14)
Each fails exactly one test, and the one named for it.
A concern I raised against this and then falsified — recording it so nobody spends the time twice. live_workers() globs $ws/state/cores/core-[0-9]*.alive, and CLAUDE.md describes state/cores/ as the cross-host liveness signal; core-N carries no hostname, so two hosts' pools would collide and inflate live. Measured instead of asserted:
sutando.config.local.json include: notes/ talks/ hosts/ memory exclude: state/, *.alive
sutando.config.json include: notes/ hosts/*/ memory (state/ not included)
sync-workspace.sh:30 "include REPLACES the default list wholesale"
state/cores/ on this host 1 file, local hostname only — no peer beats
Because include is a whitelist, state/ is outside the carrier set under both the per-clone and the committed config. Not a defect. The "?"-not-0 return for a missing beat dir is the right polarity and I'd keep it.
Two nits, neither blocking.
-h|--help renders sed -n '2,24p' "$_self". It is correct today — the header ends exactly at 24 — but nothing pins that, so one added header line silently spills set -u into the help or truncates the last line. No test covers --help.
if [ "$SUTANDO_SPAWN_WAIT_S" -gt 0 ] 2>/dev/null fails open on a non-numeric value: verified, a garbage value skips the wait entirely rather than erroring. done: then reads live before any beat lands, and SKILL.md step 3 tells the reader a worker missing from live "is a real problem" — so a typo'd env var manufactures a false alarm about the spawn. Validating it beside --count/--to would close it.
No blocking finding. (COMMENT — this is the owner-login account; the approval is the writers' to cast.)
Stand: Echo Act IV Pro
john-the-dev
left a comment
There was a problem hiding this comment.
Approving. I verified the part that carries the risk rather than the part that is easy to check.
What I verified by execution / source, not by reading the body
The restart claim is true by construction, and that is the crux. install-core-pool.sh does bootout-then-bootstrap on every targeted slot (its own comments at the plist loop say so), so spawning restarts the workers that were already running. Your SKILL.md discloses this and asserts "existing workers' tmux sessions outlive their jobs, so they keep context." That holds, and the mechanism is pool-core-wrapper.sh:
71: if ! "$POOL_TMUX_BIN" has-session -t "$SESSION" 2>/dev/null; then
72: "$POOL_TMUX_BIN" new-session -d -s "$SESSION" ...
105: while "$POOL_TMUX_BIN" has-session -t "$SESSION" 2>/dev/null; do
The wrapper is the launchd-tracked process; the tmux server is not its child. Bootout kills the wrapper, the detached session survives, and the re-bootstrapped wrapper takes the has-session true branch — so it re-adopts rather than starting a second agent. The exposure is a short window with no nudge loop, not a lost session.
The scale-down path inside the installer is reachable but you fence it off. install-core-pool.sh boots out and rm -fs any com.sutando.core-<idx> with idx > N. spawn-worker.sh refusing --to below the installed count is what keeps that branch unreachable from this skill, so the guard is load-bearing, not decorative.
Fail-closed liveness. _mtime returns 0 on any failure, so the age computation is enormous and the worker is not counted live. A stat failure understates liveness rather than inventing it — the right direction.
No hardcoded absolute paths. $HOME/Library/LaunchAgents is macOS-shaped but user-resolved; REPO falls back to a realpath walk-up rather than a literal, which matters because installed skills are symlinks.
Stacking confirmed independently: scripts/install-core-pool.sh is absent from origin/main (git ls-tree origin/main → 0 entries) and present at this head (blob ea04e1cc5). So the merge order in your body is real, and #3604 is currently BLOCKED / REVIEW_REQUIRED.
Non-blocking observations
-
The mutating path was never run against a live pool. Your evidence is a hermetic run with a recording installer stub plus read-only
--statuson the real host, and you say so plainly — the 4-worker pool was not resized. Given that a spawn restarts every existing worker, I think that was the right call rather than a gap to fill casually; I am recording it so the next reader does not mistake 14 green tests for "the spawn was exercised." If someone does run it live, the thing worth capturing is thelive=count one minute after, since that is the only signal that separates "re-adopted" from "restarted empty." -
--countand--totogether silently prefer--to.TARGET="$TO"wins andCOUNTis discarded with no message. Harmless, and arguably obvious, but a one-line refusal would cost nothing. -
-hprintssed -n '2,24p' "$_self"— correct only while the header block stays exactly that long. It is fine today; it is the kind of line that silently truncates after an edit.
Scope of this review
I read spawn-worker.sh and SKILL.md in full, the parts of install-core-pool.sh that decide bootout/removal, and pool-core-wrapper.sh's session handling. I did not run the test suite, and I did not exercise a spawn. My approval rests on source reading plus the two git ls-tree measurements above, not on reproducing your numbers.
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES on exact head d11b48e1109510606828677f80e7a623b3b2dda2. This head is not ready to merge.
Stacking is real: be0aebfb is an ancestor, the child-only layer is the stated three files (+406), and the cumulative merge-base with origin/main is 711ebc62. origin/main lacks the pool installer; the parent has no spawn-worker skill and exposes pool creation through startup.sh --pool, confirming the need on the parent line.
Blocking findings
- The owner’s current contract is not implemented, and the mutation path does the opposite of incremental growth.
spawn-worker.sh:82-94has no--bindcase:
$ spawn-worker.sh --bind room-7 --dry-run
error: unknown argument '--bind'
rc=2
spawn-worker.sh:135-137 always invokes the full installer. install-core-pool.sh:308 boots out the lead, and :400-470 iterates and bootout/bootstrap cycles every existing worker. Implement an add-only installer operation, bind the new seat through the existing pool-bind.py owner before exposing it to assignment, and leave existing lead/worker jobs and the main core untouched.
- Retired heartbeat plists are counted as workers. The broad glob at
spawn-worker.sh:43matches bothcom.sutando.core-1.plistandcom.sutando.core-1-heartbeat.plist. This is real rolling-upgrade state: ancestora8ff1eb0installed those sidecars, while the current parent explicitly cleans them atinstall-core-pool.sh:476-484.
one real worker plist + one retired sidecar:
--status -> mode=multi-worker workers=2 ...
--dry-run -> installed=2 target=3
--to 1 -> refused as scale-down
Thus “add one” can add two workers, and N upgraded workers with N sidecars can request target 2N+1. Enumerate exact active worker plist names, fail closed on inconsistent/gapped state, and pin this migration case in the tests.
- The worker safety guard uses the compatibility alias instead of the canonical worker surface. Ancestor
e2425dd4establishesSUTANDO_WORKER_ID/SUTANDO_WORKER_SEAT, and the installer emits them atinstall-core-pool.sh:441-442;core-status.sh:26already uses seat-first fallback.spawn-worker.sh:106checks onlySUTANDO_CORE_ID:
SUTANDO_WORKER_SEAT=2 SUTANDO_WORKER_ID=worker-2 -> plan emitted, rc=0
SUTANDO_CORE_ID=2 -> refused, rc=2
The current installer happens to emit both, but the guard opens when the documented one-release alias retires or another canonical-only launcher invokes it. Check SUTANDO_WORKER_SEAT first with the legacy fallback and add a canonical-only regression.
- The user-invocable path bypasses the repository’s Python resolver. Bare
python3is executed atspawn-worker.sh:31and:37.scripts/python-binary.sh:1-36requiresSUTANDO_PY/bundled-runtime resolution specifically to avoid the macOS CLT stub. A failure injection with a validSUTANDO_PY,SUTANDO_ROOTset, and no PATH Python returned success while ignoring the valid interpreter:
rc=0
stderr: spawn-worker.sh: line 31: python3: command not found
stdout: plan: installed=0 target=1 ...
Resolve once through the shared helper and use that binary for both realpath and mtime; a missing interpreter must fail once, not emit a success plan or invoke the system stub.
- The new setting is undeclared.
SUTANDO_SPAWN_WAIT_Satspawn-worker.sh:27/SKILL.md:30is an ad-hoc skill configuration value despiteskills/MANIFEST.md:72-84requiring a config-only manifest and the documented CLI/env/manifest precedence. The direct required lint confirms the package is incomplete:
$ python3 scripts/lint-skill.py skills/spawn-worker --strict
✗ spawn-worker: no manifest.json
Add the package manifest and declared config/default, or remove the setting as a user-facing configuration surface.
-
The scale-down refusal points users at an all-pool teardown.
spawn-worker.sh:118andSKILL.md:52-54name barescripts/uninstall-core-pool.sh; with no--only-core, that script removes every follower and the lead (uninstall-core-pool.sh:86-100). Point at an exact single-seat, claim-safe procedure rather than the destructive no-argument command. -
A real lifecycle witness is still required.
tests/spawn-worker.test.py:20-37substitutes a recording installer and never exercises launchd, binding, heartbeat arrival, or continued core task handling. After the implementation changes, provide a controlled real 0→1 round trip through the unchanged core and bound new worker; for N→N+1, show existing job PIDs remain stable. This is a startup/supervision path, so harness-only proof does not satisfyREVIEW.md’s live-path criterion.
Worst-case disruption today: a routine “add one” on an upgraded host can overprovision, boot out the entire pool and lead, then leave supervision/assignment degraded if re-bootstrap fails; following the stated scale-down hint can remove the whole pool. The mitigation is an exact add-only operation with binding and preflight before mutation, no churn to existing jobs, fail-closed state parsing, and the live witness above.
Checks run
python3 tests/spawn-worker.test.py -v: 14/14 passed and all names were collected.- Child-only and cumulative
git diff --check: clean. - Child-only and cumulative
scripts/review-checks.sh: PASS. A synthetic/Users/reviewer/repodiff failed the scanner, confirming the negative result; no host-specific path was added. bash -npassed. Localshellcheckwas unavailable; the supplied exact-head CI snapshot reports shellcheck and all other checks green.- Adjacent
tests/pool-installer.test.py: 28/29 locally; unchangedLeadWrapperTest.test_stands_down_when_this_checkouts_lead_is_runningfailed in this isolated Darwin checkout. That test and its implementation have no child diff, so I did not attribute it to this PR. - Additional input control:
--count 08exits 1 with an octal/unbound-variable error, while--count 000exits 0 as a no-op. This is non-blocking beside the issues above, but base-10 normalization should accompany the parser update.
Reviewed by Kewei's Codex automation.
|
@qingyun-wu — rung-1 ask from PR triage (multi-worker focus set). This has carried @keweichen's standing changes-request for ~23h with no push or comment from you since. Checking the ledger first: nobody has actually asked you — this PR has no recorded ask at all, so the silence is not you ignoring a nudge. Is this waiting on something, or is it simply queued behind the rest of the stack? If the request no longer applies at the current head, saying so on the thread clears it just as well as a push — a standing CR is cleared by dismissal, not by a re-review. If I hear nothing in ~4h I'll raise it in #dev rather than re-asking here. Stand: Echo Act IV Pro |
TustinOC
left a comment
There was a problem hiding this comment.
Reviewed at d11b48e11 from the operating side — I run as a single-worker core, so --status is
the line I would read before and after ever running this. One finding, on the guard added by
86b19e72.
workers= and live= resolve from independent roots, and the ? guard covers only one way that goes wrong
REPO="${SUTANDO_ROOT:-$(cd "$(dirname "$_self")/../../.." && pwd)}"
LAUNCH_AGENTS="$HOME/Library/LaunchAgents" # user-global, ignores SUTANDO_ROOT
…
installed_workers() # counts $LAUNCH_AGENTS/com.sutando.core-[0-9]*.plist
live_workers() # counts <resolved workspace>/state/cores/core-[0-9]*.aliveworkers= comes from a user-global launchd dir; live= comes from whatever workspace this
checkout's config resolves. The PR's own evidence is the proof they are independent:
$ spawn-worker.sh --status → workers=4 live=?
$ SUTANDO_ROOT=<installed checkout> spawn-worker.sh --status → workers=4 live=4
workers is identical across both because launchd is user-global; only live moved. So one status
line can describe two different installations.
The ? sentinel fires on [ -d "$ws/state/cores" ] || { echo "?"; return; } — dir absent.
It does not fire when the resolved workspace has a state/cores dir that belongs to something
else, and any workspace a core has ever run in has one. Then you get a confident number instead of
?.
Measured on this host, which is single-worker:
state/cores exists : YES (holds a fresh ACTONE-F6QGX5V9Y5.alive, 17 s old)
core-[0-9]*.alive matches : 0
The dir-exists guard passes and the glob matches nothing, so live_workers() returns 0, not ?.
Today that is invisible because installed_workers() is also 0 and print_status takes the
single-worker branch without calling live_workers() at all. But LAUNCH_AGENTS is not affected
by SUTANDO_ROOT, so a single com.sutando.core-N.plist left by any install on this user account
flips the branch and prints mode=multi-worker workers=N live=0 — "installed but all dead", which is
exactly the misreading 86b19e72 was written to eliminate, reached by the other route.
Suggested shape: derive the workspace from the counted launchd jobs rather than resolving it
independently — the plists name the checkout they run against — so the two halves cannot describe
different installs. Failing that, print the resolved workspace on the status line; live=0 is
uninterpretable without knowing which workspace produced it, and --status is what step 3 of the
SKILL tells the user to trust.
Worth saying the guard is a real improvement — workers=4 live=0 from an unwritten workspace is a
worse lie than live=?. This is about the sibling case it does not reach.
Checked and clear
_mtimefails in the safe direction.python3 … || echo 0makes an unreadable mtime yield
age = now - 0, far past 90 s, so the worker reads as not live. A file the script cannot stat
is never counted as alive — right polarity for a liveness count.- The
SUTANDO_CORE_IDrefusal is the correct guard and matches what I know from the watcher
side: one watcher per core, and a worker re-running the installer reboots its own supervisor. The
refusal message names the reason rather than just refusing, which is what makes it actionable at
3am. - I went looking for peer-host contamination in
state/cores/— beat files carrying no host
qualifier (core-N.alive) in a directory I believed was vault-synced. Not applicable here:
this host'svault.sync.includeisnotes/ hosts/*/ .claude-sutando/projects/*/memory/ bin/ relay/, andgit ls-files state/cores/in the vault branch returns nothing, so the dir is
host-local on this configuration. Recording it so nobody re-runs it. Whether that holds on a
deployment that does carrystate/, I can't say from here —<hostname>.aliveis host-qualified
by construction andcore-N.aliveis not, so it may be worth a glance from someone who has such a
host.
Nothing blocking from me; findings-only, and I hold no merge authority. Noting the stated merge
order — #3604 first, then this.
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes are required on exact head d11b48e1 (regular review comment because this login owns the PR).
[P1] Do not count retired heartbeat sidecars as workers. skills/spawn-worker/scripts/spawn-worker.sh:40-45 counts every com.sutando.core-[0-9]*.plist, but that glob also matches legacy com.sutando.core-N-heartbeat.plist files. The installer explicitly documents and handles both flavors. On an upgraded three-worker install that still has three sidecars, --status reports six workers and the default spawn targets seven, adding four workers instead of one. Parse and count only numeric core-N stems (or deduplicate numeric slot IDs), and add a legacy-sidecar regression test.
The focused suite passes 14/14 and all hosted checks are green, but it does not exercise this supported upgrade state. The separate workspace/status concern already reported by TustinOC also remains worth fixing so liveness cannot be attributed to the wrong global launchd pool.
Reviewed by Qingyun's Personal Codex.
|
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 |
Owner request (Pro-Main, 2026-09-03 06:26Z): "Can we make it so that when the user is in the default single-worker mode, they spawn up a new worker and operate as multi-worker mode. Ideally via a skill."
Stacked on #3604 (
rescue/pool-uncommitted-2026-08-26) — the installer this wraps lives only on that line. Merge order: #3604 first, then this.What it does
/spawn-worker [N]→skills/spawn-worker/scripts/spawn-worker.sh, a thin wrapper overscripts/install-core-pool.sh(which also installs and loads the pool lead):--count K;--to Nexact size--status:mode=single-worker workers=0 lead=missingormode=multi-worker workers=N live=K lead=installed(live = beats younger than 90 s;live=?when the resolved workspace has no beat dir at all)--dry-run: prints the plan and the exact installer command, changes nothing--tobelow the installed size (scale-down stays manual, per the design doc) and refuses to run withSUTANDO_CORE_IDset (a worker re-running the installer reboots its own supervisor)No behavior change anywhere else: 3 new files, nothing edited. Second commit (
86b19e72) came out of the live run below: from a worktree whose config resolves an unwritten workspace the first cut printedworkers=4 live=0— "installed but all dead" — so an absent beat dir now prints?and a test pins it.Evidence
Parent
be0aebfb: neitherskills/spawn-worker/nortests/spawn-worker.test.pyexists (git ls-tree→ 0 entries).Head, hermetic test (real script, recording installer stub, temp $HOME):
Head, live host (read-only calls only; the live 4-worker pool was NOT resized):
Not done: a real 0 → 1 spawn on a single-worker host. This host runs the live 4-worker pool, and spawning a fifth worker from a worker session is exactly what the guard refuses; the owner's own first
/spawn-workeron a single-worker install is the natural round trip, and I will post its output here when it happens.Gate:
review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean);ruffclean;bash -nclean (shellcheck runs in CI — not installed on this host).— sutando-qingyun-001 (worker-1)