Skip to content

fix(manager)!: leave managed agents running on a stack stop - #1301

Open
davidfarah2003 wants to merge 17 commits into
mainfrom
fix/964-down-leaves-agents
Open

fix(manager)!: leave managed agents running on a stack stop#1301
davidfarah2003 wants to merge 17 commits into
mainfrom
fix/964-down-leaves-agents

Conversation

@davidfarah2003

@davidfarah2003 davidfarah2003 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Bare cotal down / Manager.stop() no longer reap live managed seats. The previous reap is cotal down --with-agents / stop({ withAgents: true }).

One 2026-08-26 teardown took six live seats, some with uncommitted work. The operator-visible default is now spare: leftover seats are listed (name, mode, pid, agent, cwd, status) as unmanaged OS processes, with cotal down --with-agents as the reap. Spare down always signals the manager; listing is honesty, never a refuse-to-signal. Listing and reap pass { onRefusal: "throw" } into resolveControlTarget so an unreachable broker is catchable instead of process.exit(1), and the stop loop still runs. --with-agents --dry-run prints the seats that would be reaped and mutates nothing. --preserve-state --with-agents remains refused.

Preservation / stopRetainedAgentsOnExit is unchanged. No new manager RPC. --with-agents sends waitForExit: true on each seat stop so proof of exit is not derived from graceful: false. Ordinary cotal stop / cotal_despawn stay acceptance-not-exit.

--preserve-state keeps the default connectOrExit at its three resolveControlTarget sites (retry, prepare, commit). A half-cut that cannot talk to the broker must abort rather than continue; that is not listing honesty.

Listing seats matches recorded roots canonical-root-wise (meshesForRoot). If preflight still deletes the entry as a stale unreachable broker, listing and reap restore it. A failed dependent must keep that record so later cleanup cannot proceed under a live process.

Cluster revision

This PR claims manager cluster document revision 11, measured against origin/main (still revision 10) at review time, not against this branch's merge base. Revision 11 is the optional waitForExit input on despawn / stop: a changed input contract, so a changed described surface, even though the command names are unchanged.

#1302 also needs revision 11, for a different surface change (status output digest). If #1302 merges first, this PR must move to 12 and re-derive its digest against the merged surface. Do not treat 11 as surviving a later merge by number alone.

Named gaps

  • A PTY child may still die when the manager process exits and the PTY master closes. This PR holds handles in-process so GC is not what is measured; process-exit survival is not claimed.
  • A successor reconcileStaticLifecycles can still terminalize durable slots with no live owner. Out of this PR.
  • This listing is cotal down. A failed cotal up teardown SIGTERMs the manager without a seat snapshot.
  • An older manager whose stop() still reaps will still reap on SIGTERM.
  • --with-agents that cannot list seats (no recorded mesh, unreachable control plane, or a non-ok ps) still stops the stack, reaps none, and exits non-zero naming that no seats were reaped. It does not claim a completed reap.

Because those remain, #964 stays open for the process-exit survival and successor-terminalize halves.

Follow-up: #1310 (spare-guard conventional-name allowlist).

Signature-mirror SET

The SET is one suite. A mirror is invisible to an import search, so the claim is the shape search: a local re-declaration of despawnAuthorized(...). That search returns only implementations/manager/smoke/renewal-terminal-race.smoke.ts plus the real method in implementations/manager/src/manager.ts. Twelve manager smokes cast mgr as unknown as { ... }; only that one redeclares despawnAuthorized. requireAuthoritativeExit has the same two-file set. That suite now awaits Promise<{ ok: boolean }>.

CI / smoke:clean

Shard 0 at 138e1a1b5 ran pnpm smoke:clean to CLEAN SMOKE OK (70 passed), including down: listing a failed dependent names the control plane, not a missing record and down: a failed dependent preserves the mesh registry. The shard then failed at pnpm smoke:mutation-fixtures: three find anchors spanned comments (down-failed-dependent-preserves-mesh.json [1], down-with-agents-unreaped.json [0] and [1]). 83ab72640 re-anchors those on unique code-only windows (pnpm smoke:mutation-fixtures local OK, both mutation-proof fixtures 2/2 killed). CI green at 83ab72640 is still unmeasured.

Tests

  • pnpm smoke:manager-stop-reap (authed broker + delivery daemon + real managed PTY seats): spare leaves seat A running and undeprovisioned; a second manager starts on a different root so it is not A's successor; stop({ withAgents: true }) reaps seat B; seat A survives B's reap (18 cells).
  • pnpm smoke:preserve-state: named spare cells plus stop({ withAgents: true }) counterpart. Mutation-proof preserve-state-spare.json: restoring always-reap reddens normal stop leaves managed agents running.
  • pnpm smoke:manager-stop-spare-guard: planted-control walk; live-PTY smokes must not spare-stop except named coverage.
  • pnpm smoke:start-model cell 9b: in-process spare path. Mutation-proof: restoring always-reap reddens spare: a plain stop does not hard-stop the child.
  • pnpm smoke:lease-loss-keeps-serving CONTROL retargeted onto stop({ withAgents: true }); all 4 mutations killed.
  • pnpm smoke:down-target: hermetic --with-agents combination refusals, including --with-agents --dry-run allowed; live-broker unreaped cell (stack stops, exit 1, loud unreaped line); down manager against nats://127.0.0.1:1 still SIGTERMs and does not process.exit. Mutation-proof down-with-agents-unreaped.json (2/2 killed after the code-only re-anchor).
  • pnpm smoke:clean: a failed dependent with a spelled-different recorded root still names the control plane (not a missing record) and keeps the mesh registry. Mutation-proof down-failed-dependent-preserves-mesh.json (2/2 killed after the code-only re-anchor).
  • pnpm smoke:mutation-fixtures: 0 dead / 0 ambiguous / 0 prose-spanning anchors locally at 83ab72640.
  • pnpm smoke:renewal-terminal-race: signature-mirror awaits despawnAuthorized.
  • pnpm smoke:flag-inventory: with-agents:boolean on down.
  • Live-PTY teardowns retargeted to stop({ withAgents: true }) (freeslot, int2-revoke-hold, attach-reconnect/stdin, cli-seat-locality, console-ws-duplex, lifecycle-e2e, manager-multi-live, manager-reconcile-startup, user-spawn F, probes).

Refs #964

Bare Manager.stop() and cotal down no longer reap live seats. The previous
reap is stop({ withAgents: true }) / cotal down --with-agents. A live
manager that cannot list seats is refused. Leftover seats are printed
after a spare down.

Refs #964. A PTY child may still die when the manager process exits and
the PTY master closes; that survival is not claimed here.
@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Review — BLOCK

Head: c560a6a9d6cd0737ab82e2c93f2c095d48fd9a7d (confirmed: git rev-parse HEAD and gh pr view 1301 --json headRefOid both read this sha). Graded in an isolated worktree with an in-process manager; the real up/down verbs were never run.

Verdict: BLOCK — the PR breaks its own test suite at this head.

A stale in-process test still asserts the OLD destructive default, was not updated with the flip, and is not in the diff:

implementations/manager/smoke/preserve-state.smoke.ts:1021  check("normal stop still hard-stops managed agents", handle.stops === 1, ...)
implementations/manager/smoke/preserve-state.smoke.ts:1022  check("normal stop still deprovisions managed agents", deprovisions === 1, ...)

That cell (comment: "Regression: active-mode stop remains the existing destructive shutdown path") calls bare manager.stop() and asserts a reap. Under this PR's own change, active-mode bare stop() now detachManagedAgents()handle.stops === 0, deprovisions === 0. CI at this exact sha confirms it, on Linux and Windows:

  • CI / unit (33980005112): not ok - normal stop still hard-stops managed agents: 0 / ... deprovisions ...: 0PRESERVE-STATE SMOKE FAILED (2 failures).
  • CI / smoke (shard 0/4): same failure; because the shard is fail-fast, ✗ shard 0/4 FAILED at: pnpm smoke:preserve-state and 105 of 132 planned smokes in that shard never ran — much of the security/protocol gate is unmeasured at this head.
  • Windows / required: same two not ok lines.
  • ci-ok/windows-ok aggregates: failure.

preserve-state.smoke.ts is in the manager package test script and was not touched by the PR. This is the single un-updated stop path the flip missed. One-line fix (update the two assertions + the stale comment), but it is a red required gate, so BLOCK.


Independently built reap-path enumeration (built from the other end, then checked against the diff)

Method: grepped every call that ends a managed agent, independent of the diff — deprovision( (positive control: 40+ hits), handle.stop, stopHandle, SIG*, freeSlot, teardownManagedAgents, detachManagedAgents, stopRetainedAgentsOnExit, agents.clear(), reconcileStaticLifecycles. Confirmed the per-agent chokepoint is stopHandle; the only three MASS paths are the branches of Manager.stop(). Each anchor verified to resolve exactly once.

# Path that can end a managed agent Reached by Behavior at this head Correct for the flip?
1 Manager.stop() active, withAgents:trueteardownManagedAgents() cotal down --with-agents (per-seat via manager stop op), stop({withAgents:true}) hard-stop + await exit + deprovision
2 Manager.stop() active, bare → detachManagedAgents() bare cotal down (SIGTERM → manager shutdown handler → mgr.stop() no opts), stop() drop from table, suppressCleanup=true, no stop/deprovision; handles held on this.detached (only pushed, never reaped) ✓ (the fix)
3 Manager.stop() preservation → stopRetainedAgentsOnExit() down --preserve-state, signal after partial preservation stops retained children (unchanged) ✓ (out of scope, unchanged)
4 per-seat stopHandle + freeSlot/deprovision manager stop op (CLI stop --name, --with-agents loop), self-despawn, despawn, pi-crash, process-exit unchanged
5 successor reconcileStaticLifecycles() on boot a successor manager terminalizes durable static slots this instance owns with no adopted agent terminalizes broker footprint of still-running detached seats Disclosed as an out-of-scope named gap (#964 not closed)
6 PTY master close on manager process exit → SIGHUP after bare down detaches, the exiting process closes the pty master child may die Disclosed as a named gap; survival not claimed

No hidden mass-reap path found. teardownManagedAgents, detachManagedAgents, stopRetainedAgentsOnExit each have exactly one caller; no agents.clear() anywhere in manager.ts; this.detached is never drained.


Findings per numbered question

Primary (reap-path completeness): complete for cotal down. The one in-process stop path that was NOT updated for the flip is the preserve-state.smoke.ts active-mode-stop cell above — a genuine missed reap-behavior path, and it is red in CI. That is the enumeration's payoff, not a doc nit.

1. Smoke teardown leakage — real, and no mechanism guards it. Many in-process suites spawn real pty seats and tear down with bare mgr.stop(), which now DETACHES instead of reaping (console-ws-duplex seat echo1, manager-multi-live a1/a2, cli-seat-locality, etc.). The author updated only lifecycle-e2e cell 4, start-model cell 9, and lease-loss CONTROL to pass {withAgents:true}. Mitigation: each smoke is its own process ending in process.exit, which closes the pty master fd → SIGHUP reaps the child, so the practical leak is bounded to the test's own lifetime on this box. But nothing catches a future author who forgets — no lint, no guard; the three were found by hand. Combined with the missed preserve-state cell (found only by CI, not by the author), this is exactly the "grep is not a mechanism" hazard. Recommend a guard (e.g. a teardown that reaps by default, or a lint) before this default flip is trusted at scale.

2. Mutation fixtures — sound. All 6 anchors resolve exactly once at this head; all expectRed cells exist as TEXT. shutdown-seat-exit 2/2: mutation 1 (void failures) reddens cell "…proves every managed child exited…" because exitProofs is never populated (the earlier hard-stop cell stays green, so this is the first red for the right reason); mutation 2 (always-reap) reddens the spare cell via the observable stopped list, and it is the earliest red because cell 9 (withAgents) stays green under it. lease-loss 4/4: the retargeted CONTROL (mutation 4, agents.clear() in the withAgents branch) reddens Cell 0 (h.stops===1) — Cell 0 is the ONLY cell exercising stop({withAgents:true}), so it genuinely proves the stops instrument fires and earns the stops===0 zeros in cells 1-3. No short-circuit-first-cell trap. (Did NOT run mutation-proof, per instruction; graded by reading.)

3. Docs + bundle — clean. pnpm check:docs-voice passes (40 pages). Regenerating docs-bundle.generated.ts leaves git status --porcelain empty (regenerated in the same commit; the bundle carries the new --with-agents text). Docs correctly describe the new default, the leftover-seat listing, the refuse-on-unlistable-manager, and the --preserve-state exclusion.

4. The refusal is reachable, correct, covered. down.ts acknowledgeManagerSeats() runs BEFORE any stop loop and THROWS (refuses, no signal) on: no recorded mesh, control-plane unreachable, or a non-ok ps reply. down-target.smoke.ts covers the combination refusals. This is the load-bearing safety gate and it gates.

5. Design contradiction resolved as "refused loud." down.ts:82 refuses --preserve-state --with-agents; docs/cli.md states the same. No "stays mode 3" remnant. Consistent.

6. --with-agents --dry-run still refused — I agree this is backwards. down.ts:84 refuses it with only "bare-whole-stack only", no concrete safety reason. A dry run of the destructive mode is what a careful operator wants before a mass reap. Finding: either allow it, or state a concrete reason. (Design finding, not a blocker.)

7. No overclaim. PR body states both caveats (PTY child may die on manager-process exit; successor reconcileStaticLifecycles can terminalize durable slots) and says it does NOT close #964. Verified both in code (paths 5-6 above). Changeset/docs claim no persistence the code cannot deliver.

8. flag-inventory updated correctly. with-agents:boolean added to the golden down set, matching the registered flag. (Minor: no per-flag attributing comment, but most golden flags carry none — consistent with the file.)

9. Changeset level justified. minor for the 3 changed packages; pnpm changeset status bumps all 21 fixed-group packages at minor, none at major. Correct for 0.x per AGENTS.md (a breaking change is a minor). PR body carries the ! rationale.


Standards / what I could not check

  • pnpm typecheck: exit 0, Scope: 27 of 28 workspace projects (28th is JS-only @cotal-ai/example-04-frontier-faces, skipped — reported, not called clean).
  • CI at the full 40-char sha: Docs=success, Code Quality=success, CI=failure, Windows=failure, Mutation reproof=in_progress (UNMEASURED). One run per workflow (no supersede ambiguity). cancelled seen on none. The CI/Windows failures are the preserve-state regression above (the BLOCK).
  • The CI / live job also failed, on FAIL: stale shutdown ownership is reclaimed (broker at nats://127.0.0.1:1) — its down-related cells all passed, so this looks like a separate pre-existing/environmental flake NOT caused by this change; I could not fully isolate it without the real verbs, so I flag it as a non-green gate rather than attributing it to the PR.
  • Could not exercise the real cotal down verb (forbidden — walks to the live fleet). What that leaves unverified: the end-to-end CLI signal → manager shutdown handler → detach chain in a live stack, and the --with-agents per-seat CLI reap loop against a live manager. Both are exercised in-process by manager-stop-reaps-agents.smoke.ts (real broker, real pty seats: asserts seat A process alive + creds retained after bare stop; seat B process dead + creds gone after --with-agents; A survives B's reap), which is the correct substitute.

Worktree left clean.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Merge hold at c560a6a9d6cd0737ab82e2c93f2c095d48fd9a7d — consolidated findings

Two independent reviews from different vendors both return BLOCK, and CI is red. Recording the
findings in one place so the lane works from a single artifact rather than a thread, and so none of
this lives only in a session that can end.

The direction of this change is right and is not in question. One reviewer built the reap-path
enumeration independently, from the other end rather than by reading the diff, and reports it
complete: three mass paths (the branches of Manager.stop) plus per-seat stopHandle, with two
out-of-scope gaps disclosed in the body. That was the question I most wanted answered and it came
back clean. What follows is about the paths around the flag, not the flag.

Blocking

1. Two existing suites assert the old default and were not updated. Neither is in this diff.

  • implementations/manager/smoke/preserve-state.smoke.ts:1021-1022 — bare manager.stop(), asserts
    handle.stops === 1 and deprovisions === 1. Live-reproduced: both red, 0 and 0.
  • implementations/auth/smoke/user-spawn.smoke.ts section F — bare await manager.stop(), asserts
    the managed row is deleted, token/sentinel/health files shredded, and the old captured actor token
    returns 401. Live-reproduced: {rowGone:false, filesGone:false} and the old token returns 200.

These need different repairs and the difference matters. preserve-state should be inverted to
assert the new contract and gain a counterpart pair proving stop({ withAgents: true }) still
hard-stops and still deprovisions — without that second pair the suite stops covering the reaping
path entirely, and the PR would ship a flag having deleted the test for the behaviour it gates.
user-spawn section F should switch to stop({ withAgents: true }) and keep both assertions: it
covers credential revocation on teardown, and inverting it would delete coverage of a security
property.

The user-spawn result also raises a product question the body should answer: a bare down no
longer revokes agent credentials. That is coherent, since the agents keep running — but it is a
change an operator should be told about rather than discover.

2. The refusal is unsatisfiable in the case it most matters, and CI proves it.
bin/smoke/ext-live.smoke.ts:435 "stale shutdown ownership is reclaimed" now fails:

✗ no broker answered at nats://127.0.0.1:1 - "main" is registered here but its mesh is not up

Verified with a control, because the message names a broker and therefore reads like an environment
problem: ext-live.smoke.ts is not in this diff, and the live job is success on main at
both f3bad8772 and 4eebbc481. Same fixture, same deliberately-unreachable broker, passes on main,
fails here. The fixture always had an unreachable broker; the cell passed because down did not need
to enumerate seats. It does now.

So: if the broker is gone, unreachable, or was never up, down can no longer stop the manager,
because it cannot enumerate the seats it has decided it must enumerate first. A safety check that
fails closed on unreachability turns every broker outage into an unstoppable manager
, leaving a
by-hand kill as the only route — which is what this work exists to remove.

3. down never issues the detach that the refusal exists to protect. down.ts:171 calls
acknowledgeManagerSeats, :183 calls stopLocalProcess, :386 sends SIGTERM. The only detach is
Manager.detachManagedAgents() at manager.ts:1810, internal to Manager.stop() and reached only
after the signal handler runs. A manager live enough to answer ps but stalled in its event loop
answers the seat query, never runs its handler, and is SIGKILLed after the 15-second escalation. No
detach occurs, the PTY master closes, and the children die — the outcome the flag exists to prevent.
Taken with finding 2: the refusal blocks the case it can least help with, in order to guard an
operation that never runs on that path.

4. --with-agents does not wait for the reap it promises. down.ts:253 treats an
askManager(..., "stop") reply of {ok: true} as completion, but admin opStop calls stopHandle
(which catches runtime stop throws) and then trackStoppedHandle(..., false), freeing the slot
immediately and awaiting exit asynchronously. The reply at manager.ts:4872 is acceptance, not
exit or deprovision acknowledgement
. down then SIGTERMs the manager, whose stop sees an empty
map and cannot await the already-freed seats. No CLI smoke asserts that --with-agents waits for
actual seat exit; a search for that positive behaviour returned zero after controls hit. A throwing
or slow external runtime yields a green per-seat reply while teardown proceeds before the reap is
proven.

Findings 3 and 4 are the same defect on both paths: the safe path refuses without the operation it is
protecting, and the destructive path proceeds without proving the operation it promises.

Non-blocking, wanted before merge

5. The docs overclaim. Several pages state managed agents "stay running" with no PTY-exit or
successor-reconcile qualification. Only the PR body names both; the changeset names the PTY case only.
The docs are what an operator reads.

6. --with-agents --dry-run is refused with no reason given. A dry run of the destructive mode is
what a careful operator wants before mass-reaping. Allow it, or state the concrete reason it cannot be
represented.

7. flag-inventory's golden was updated without an attributing comment, against the file's own
convention, with the new flag placed under the existing --space comment.

8. Nothing mechanically guards a future author who forgets { withAgents: true } in a real-pty
teardown.
Measured: 69 suites construct a Manager, 4 pass the flag explicitly. Every instance in
this PR was hand-found, and the two CI caught are the ones that ASSERT the old contract — suites that
merely relied on reaping for cleanup will not fail, they will leak seats silently. The loud ones are
the lucky ones.

Clean

Mutation fixtures are sound on both reviews: anchors unique, expectRed values are text, and the
retargeted control genuinely earns its zeros. typecheck exits 0 at 27 of 28 (the 28th is JS-only).
Docs bundle regenerated in the same commit and check:docs-voice passes. The minor changeset is
correct for 0.x, and Refs #964 rather than a close is the right call for a partial.

Findings 3 and 4 are design rather than repair. Push new commits on top; do not rebase, so both
reviewers can grade a delta.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Correction to my review — the live failure is caused by this change, not a flake

I was wrong in my earlier comment when I wrote that CI/live "failed but on an unrelated broker-registration flake, not this change." I inferred that from the error text instead of pulling the job log and comparing against main. Retract that line. The live failure is caused by this PR, and it is the most serious finding here — a design flaw, not a red cell.

Evidence (I re-verified each in the code at this head)

The failing cell is bin/smoke/ext-live.smoke.ts:435 ("stale shutdown ownership is reclaimed"):

Error: FAIL: stale shutdown ownership is reclaimed —
  "✗ no broker answered at nats://127.0.0.1:1 - \"main\" is registered here but its mesh is not up..."
  • ext-live.smoke.ts is not in this PR's diff (0 matches) — the fixture did not change.
  • The live job is success on main (f3bad8772, 4eebbc481) and fails at this head, at that exact cell. The change is the only variable.

Why the error text misled me, and why it is not an environment problem

The message names a broker ("no broker answered", "its mesh is not up"), so it reads like the sandbox is missing a broker. But the fixture has always run against a deliberately-unreachable broker at nats://127.0.0.1:1; the cell passed anyway on main. What changed is this PR. down.ts now, on any down that selects the manager component (not only bare down), calls the newly-added acknowledgeManagerSeats (down.ts:170-172, all + lines in this diff):

const managerComp = selected.find((component) => component.name === "manager");
if (managerComp && mayBeRunning(managerComp, contextFor(managerComp))) {
  leftover = await acknowledgeManagerSeats(contextFor(managerComp), Boolean(values["with-agents"]));
}

acknowledgeManagerSeats resolves the control target and asks the manager for a ps snapshot; against a dead broker that throws ("manager is running but its control plane could not be reached ... refusing to signal it"), so cotal down manager now exits non-zero where it used to SIGTERM the manager pid without needing the broker. The message describes the environment; the defect is the new refusal.

Why this is the central design flaw

The refusal is unsatisfiable in exactly the situation it is most needed. If the broker is gone or was never up, down can no longer stop the manager, because it has decided it must first enumerate seats through that broker and cannot. A safety check that fails closed on unreachability turns every broker outage into a manager the operator cannot stop through the product — the same class of incident this PR set out to prevent, inverted.

And it guards an operation that never happens on that path: down.ts:171acknowledgeManagerSeats, :183stopLocalProcess, :386 → SIGTERM. The only detach is Manager.detachManagedAgents() (manager.ts:1810), internal to stop() and reached only after the signal handler runs. down itself never issues a detach op, so the refusal protects nothing on that path while making the stop unreachable.

This strengthens the BLOCK. My verdict stands (BLOCK at c560a6a9d6cd0737ab82e2c93f2c095d48fd9a7d), and it now rests on two independent grounds: the stale preserve-state unit cell (red required gate), and this refusal being unsatisfiable-when-most-needed while guarding a detach down never issues. The refusal needs to degrade to a signal-and-warn (or a stop that does not depend on broker reachability) when the control plane is unreachable, not fail closed.

The rest of my review is unchanged.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Two of the findings above sharpen on inspection, and both sharpen in a useful direction — each turns
out to name its own repair. Verified here against merge-base 5b19238f7, not against a local main
ref (which is 270 commits stale in this checkout and would have given the wrong answer to the last
question below).

Finding 4: the manager already has the mechanism, and this path opts out of it

--with-agents sends the ctl stop per seat, which reaches despawnAuthorized (manager.ts:4868).
That method is not async. It runs two synchronous statements and returns:

this.stopHandle(a, graceful);
this.trackStoppedHandle(a, trackNonAdmin);
return { ok: true, data: { name: a.name, stopped: true, graceful } };

stopHandle (:2554) is void and swallows a runtime throw into console.error at :2562-2564, so
a runtime that fails to stop produces no signal a caller can see. trackStoppedHandle (:2576) takes
a third parameter, requireAuthoritativeExit, defaulting to false — and this path omits it. With
it false, both branches free the slot before exit is observed: :2590 when the handle has no
waitForExit, :2593 immediately before the async await when it does.

So {ok: true, stopped: true} means the stop was requested. down.ts:253 reads it as done.

The useful part: requireAuthoritativeExit is exactly the disposition this flag wants, and it is not
hypothetical. A live caller at :3081 passes true, and on a runtime that cannot prove exit it
records recursive reap cannot prove exit on runtime "<kind>" and does not free the slot. The
recursive reap already refuses to claim an unproven exit. An operator-requested mass reap is the same
situation and should make the same demand.

Neither function is touched by this diff (0 hunks against the merge-base), so this is a pre-existing
semantic the new flag inherits rather than something it introduced. It still blocks, because
--with-agents is the first caller whose contract depends on the distinction.

Finding 8: the leakage is not hypothetical on the machine this runs on

Refined inventory: 24 suites construct a Manager, populate slots and call a bare Manager.stop;
22 contain no withAgents anywhere. Only start-model and lifecycle-e2e are updated. A search
for a shared teardown helper or sentinel found only the two explicit call sites and the implementation
branch — there is no enforcement mechanism of any kind.

Two of the 24 are concrete rather than theoretical, and neither is in this diff:

  • implementations/manager/smoke/cli-seat-locality.smoke.tsruntime: "pty" at :159-160, real
    pty.spawn at :115, teardown at :230-231 is await m1?.stop().catch(() => {}).
  • implementations/manager/smoke/manager-multi-live.smoke.tsruntime: "pty" at :94-95, real
    agents joining, teardown at :142-143 in the same shape.

Both bare-stop, both swallow the result. After this change they leave live PTY children behind, and
the .catch(() => {}) guarantees nothing reports it. Some suites in the set later kill only the
broker, which does not reap a seat. On a CI runner the leak dies with the machine; where these run
next to a live fleet, an orphaned pty child is a process nobody owns.

This does not argue against the change — the default is right. It argues that flipping a default which
69 files depend on needs something mechanical, because the two failures CI caught are the two suites
that happened to assert the old contract. The 22 that merely relied on it will stay green while
leaking.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Finding 5 has two concrete instances outside the five docs pages, and an adjudication

Both verified at this head; neither file is in the diff.

  • README.md:77cotal down # stop everything. This is the first-run public surface
    and it now states the opposite of the default. It is the one docs line I would call unambiguous.
  • docs/embedding.md:197 — tells embedders to wire your own SIGINT/SIGTERM -> mgr.stop().
    Under the new semantics that call leaves agents running, so every embedder following this page
    silently changes behaviour on upgrade without a word here saying so.

Adjudication: the two reviews do not actually disagree about the enumeration

One review reports the reap-path enumeration COMPLETE. The other then found stopManager() in
up.ts, called on foreground up Ctrl-C (:1032) and on broker exit (:1046), which signals the
manager by pidfile with no seat snapshot, no detach and no refusal. That reads like a contradiction
and is not one — the two answer different questions, so both stand:

  • The first enumerated the paths that reap an agent, and that set is complete.
  • The second found a path that kills the manager process, which ends agents indirectly through
    PTY master close — the PR's own documented caveat.

I checked which of the two the new code guards: acknowledgeManagerSeats has exactly one caller,
down.ts:171. stopManager does not route through it. So the protection this PR adds covers
cotal down and nothing else, and up Ctrl-C — the path an operator in a foreground stack uses
constantly — still takes seats with it.

Worth noting for whoever fixes that later, not here: at :1032 the handler does void stopManager(...)
and then child.kill("SIGTERM") on the broker without awaiting it, so manager teardown is still in
flight when its broker dies.

This does not block this PR. It is Refs #964, an honest partial, and widening it to up would
be scope creep on a change that already needs rework. It becomes blocking only if the PR body claims
that stack shutdown in general is protected — so the cheapest resolution is to not make that claim,
and to say plainly that up Ctrl-C is unchanged. Filed separately as #1307 so it is not lost.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

BLOCK — c560a6a9d6cd0737ab82e2c93f2c095d48fd9a7d

I confirmed both the local checkout and PR head at this full SHA immediately before posting. The PR is currently CONFLICTING against main.

Independent reap-path enumeration

I built this before reading the diff. I searched the whole tree for .stop(, handle.stop, deprovision, SIGTERM/SIGINT/SIGKILL and process.kill, teardown helpers, freeSlot, reapChildrenOf, preservation methods, and successor reconciliation. I loosened zero-result patterns and used positive controls.

The resulting paths were:

  1. Named despawn and self-stop through stopHandletrackStoppedHandlefreeSlot → deprovision.
  2. Natural/readiness exit through onAgentExit, followed by recursive child reap.
  3. Parent-exit recursive hard reap with authoritative exit proof.
  4. Pi crash-loop/recovery failure and replacement cleanup.
  5. Spawn/resume session-binding failures and orphaned-provision rollback.
  6. Preservation commit and partial-preservation shutdown.
  7. Active Manager.stop(), both spare and explicit-reap branches.
  8. SIGINT/SIGTERM on cotal supervise.
  9. cotal down seat coordination followed by manager SIGTERM/SIGKILL.
  10. Foreground cotal up signal and broker-exit cleanup through stopManager().
  11. Successor startup/post-resume reconcileStaticLifecycles terminalization.
  12. Runtime-specific PTY/tmux/cmux/orca/herdr termination and exit-proof implementations.

Findings by requested question

  1. Reap-path completeness: BLOCK. Bare down only asks the manager for ps; it never issues or receives acknowledgment for a detach before signaling. The only detach is internal to Manager.stop(), after the SIGTERM handler runs. If the manager answers ps and then stalls, down escalates to SIGKILL after 15 seconds, the detach never runs, and PTY master close can kill the seats. The refusal gate protects a writer that does not exist.

    The destructive path also does not acknowledge the promised reap to completion. --with-agents sends individual admin stop requests, but despawnAuthorized is synchronous: it calls stopHandle, then trackStoppedHandle, then returns success. stopHandle catches and logs runtime stop throws. trackStoppedHandle defaults requireAuthoritativeExit to false, frees the slot immediately, and proves exit asynchronously. A live recursive-reap caller already passes true, so the mechanism exists but this path opts out. down then signals a manager whose map no longer contains those seats, so shutdown cannot await them.

  2. Refusal safety: BLOCK. The control-unreachable and ps-failure refusals are fail-closed, but no test or implementation performs the load-bearing pre-signal detach operation. A successful snapshot is not evidence that the safety transition happened.

  3. Smoke teardown leakage: BLOCK. A whole-tree correlator found 24 suites that construct/populate a Manager and later use a bare stop; 22 contain no explicit withAgents: true. I found no shared teardown helper or sentinel enforcing destructive test cleanup. Two missed old-default assertions were reproduced live:

    • implementations/manager/smoke/preserve-state.smoke.ts: exactly two failures, with stops=0 and deprovisions=0. The repair needs to invert the bare-stop pair and retain/add the explicit-reap counterpart.
    • implementations/auth/smoke/user-spawn.smoke.ts section F: 108 checks pass, then the managed row and files remain and the captured actor token still exchanges with HTTP 200 instead of 401. This teardown must use { withAgents: true } while retaining both revocation assertions. Inverting or deleting them would remove security coverage.
  4. Preservation combination: clear. Code, docs, and the hermetic test consistently refuse --preserve-state --with-agents.

  5. Destructive dry run: BLOCK. --with-agents --dry-run is refused. The PR gives no concrete reason why the destructive mode cannot be previewed.

  6. Overclaim: BLOCK. The PR body honestly names the PTY process-exit and successor-reconcile gaps and says this does not close cotal down reaps every managed agent by default, and there is no flag that stops the stack without them #964. The edited docs repeatedly say managed agents “stay running” without those qualifications. Top-level README.md remains stale with cotal down # stop everything. docs/embedding.md still tells hosts to wire signals directly to mgr.stop() without explaining the changed semantics or PTY process-exit caveat.

    I also found foreground up signal/broker-exit cleanup calling stopManager() directly, outside the new down seat handshake. That is a manager-process shutdown path rather than a direct agent-reap path, so I am not using it as a separate blocker for this PR. It is tracked as cotal up Ctrl-C still reaps managed agents: the new down protection has exactly one caller #1307 and means the PR must not claim general stack-shutdown protection.

  7. Mutation fixtures: clear by inspection. Both shutdown mutations and all four lease-loss mutations have find anchors occurring exactly once in their configured target files. Every per-mutant expectRed is text. The named assertions correspond to the intended guarantees, and the explicit-reap control instruments the zero-stop cells. I did not run mutation-proof because it writes a mutant into the worktree.

  8. Flag inventory: finding. The golden flag set is correct and the smoke passes, but with-agents was added below a comment attributing only the older --space change. It does not follow this file's attributing-comment convention.

  9. Docs generation: mechanically clear. The docs and generated bundle changed in the same commit. pnpm check:docsbundle passed, including check:docs-voice for 40 pages, tool-doc generation, bundle generation, and a zero diff.

  10. Changeset: level correct, justification incomplete. Minor is correct for a breaking 0.x change, and pnpm changeset status confirms fixed-group propagation. The PR body does not explicitly justify the breaking/minor level beyond the ! title.

Checks

I ran pnpm install --frozen-lockfile because this worktree initially had no node_modules.

Passed:

  • pnpm typecheck — Scope 27 of 28; JS-only example 04 skipped as expected
  • pnpm check:docsbundle
  • pnpm changeset status
  • pnpm smoke:flag-inventory
  • pnpm smoke:down-target
  • pnpm smoke:start-model
  • pnpm smoke:lease-loss-keeps-serving
  • pnpm smoke:manager-stop-reap

Failed as described above:

  • pnpm smoke:preserve-state
  • pnpm exec tsx implementations/auth/smoke/user-spawn.smoke.ts

The full-SHA runs API returned five runs: CI and Windows are red; Docs and Code Quality passed; Mutation reproof is still in progress and therefore unmeasured. Because the PR currently conflicts with main, future pull_request synthetic-merge workflows may have zero runs until the conflict is resolved. An empty full-SHA run list must not be read as green.

I did not run cotal up, cotal down, any *-live/:live suite, the dev-tree CLI, pnpm check, or mutation-proof. The actual CLI verb remains unexecuted by design. The evidence above comes from in-process manager paths, direct handler tracing, hermetic command tests, and isolated real-broker suites.

Worktree clean.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Reading this PR's checks: they are present, and they are stale

#1301 is CONFLICTING against main, on one file —
extensions/connector-core/src/docs-bundle.generated.ts — from #1294 landing at f3bad8772. It is
mechanical: merge main in and regenerate with pnpm gen:docsbundle. Do not hand-edit it.

Two consequences worth stating explicitly, because they pull in opposite directions:

The runs currently showing on this PR were built before the base moved, so their conclusions
describe a merge that is no longer the one that would happen. A green among them would prove nothing
about merging today.

That does not make the red ones go away. Every failure cited in the findings above was
independently reproduced at the head by other means, not read off a run:

  • the two preserve-state cells, live-reproduced returning 0 and 0 against expected 1 and 1;
  • user-spawn section F, live-reproduced, including the old actor token returning 200 where the
    cell asserts 401;
  • the ext-live cell, verified from the job log and controlled against main — that file is not in
    this diff, and live is success on main at both f3bad8772 and 4eebbc481.

So: a green here is worthless until the conflict is resolved, and a red here is real.

And after the conflict is resolved, expect no runs at all until it is. While a PR conflicts,
GitHub cannot build the synthetic merge commit and pull_request workflows create zero runs. An
empty check list reads as "zero failures, zero pending", which is indistinguishable from green unless
you enumerate runs at the full 40-character sha and assert each expected workflow is present by name.
That confusion produced a confident false ALL GREEN on a sibling PR today. Check presence and
conclusions as two separate quantities; either one alone fails permissive.

Cotal merge validation added 3 commits September 5, 2026 20:21
Plain Manager.stop leaves managed seats; stop({ withAgents: true }) is the reap. Live-PTY smokes must not spare-stop except named coverage. Spare down always signals and lists seats as honesty, including --with-agents --dry-run.
…gents

# Conflicts:
#	extensions/connector-core/src/docs-bundle.generated.ts
The planted control is matched by regex, never imported, but bin's smoke
tsconfig still typechecks it. Import Manager from the workspace package.
README and embedding.md now name the spare stop default.
@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Re-review of the repaired head — BLOCK (one new red gate), but the prior grounds are resolved

Head: 64b3a9ccea33a25cd0cdb9416b45149e18fd200f (confirmed on both sides). c560a6a9d and origin/main (f3bad8772) are both ancestors — a merge plus one follow-up commit (853fbbffc), not a rebase; my earlier BLOCK stands as the review of c560a6a9d and this grades the delta. Graded in an isolated worktree with an in-process manager; the real up/down verbs were never run. Worktree left clean.

CI presence at the full sha (checked before conclusions): all four required workflows PRESENT by name — Docs success, Windows failure, CI cancelled (unmeasured; exactly one CI run at the sha, so not superseded-ambiguous but still not green), Mutation reproof in_progress (unmeasured), plus Code Quality success.

Verdict: BLOCK — a new self-inflicted typecheck failure, introduced by the follow-up commit.

pnpm typecheck fails at this head (I reproduced it locally, exit 2; Windows/required failed at its Typecheck step with the identical error):

bin typecheck: smoke/fixtures/manager-stop-spare.planted.ts(4,25): error TS2307:
  Cannot find module '../src/manager.js' or its corresponding type declarations.

The new planted-control fixture bin/smoke/fixtures/manager-stop-spare.planted.ts (added by 853fbbffc for the spare-guard) imports ../src/manager.js. That idiom resolves in implementations/manager/smoke/ (→ implementations/manager/src/manager.ts), but this file lives in bin/smoke/fixtures/, where ../src/ is bin/src/ — there is no bin/src/manager.ts. The fixture's comment says "Not imported" (the guard reads it as text via readFileSync, never executes it) — but bin/tsconfig.smoke.json has "include": ["**/*.ts"] and still type-checks it. That tsconfig exists precisely to catch smoke files that reference something which does not resolve, and it did its job. Same class of self-inflicted red as the original preserve-state cell, now in typecheck. Trivial fix (point the import at the manager package, or exclude the fixtures dir, or make the planted control a non-.ts text file), but it is a red required gate.

The prior BLOCK grounds are genuinely fixed — credit where due

  • The stale preserve-state cell is repaired correctly. It now asserts handle.stops === 0 / deprovisions === 0 (the new default) AND a counterpart cell was added asserting stop({ withAgents: true }) still hard-stops (stops === 1) and deprovisions (=== 1). I ran the suite: PRESERVE-STATE SMOKE OK (0 failures). The new preserve-state-spare.json mutation anchors on the unique spare branch; its expectRed cell exists and the counterpart pair stays green, so the spare cell is the first red for the right reason. user-spawn.smoke.ts section F uses { withAgents: true } with assertions kept, not inverted (verified by reading the diff).

  • The refusal removal is the right trade. You asked me to grade the removal, not the refusal. Removing it is correct, and not merely because it fixes the ext-live red: the safety property that actually mattered — "a plain stop does not reap" — lives in Manager.stop()'s internal detachManagedAgents(), reached whether or not the CLI could list seats. The CLI listing was only ever honesty (telling the operator what is left), never the mechanism preventing a reap. So signalling-with-honest-listing keeps the real guarantee and drops a false gate that made an unreachable manager unstoppable. Against my original brief's premise — a flag promising safety it does not deliver is worse than no flag — this flag now promises less and delivers exactly that: bare down leaves agents running (enforced in the manager), and the CLI is honest about what it could and could not enumerate. That is the better bargain.

  • --with-agents --dry-run is now allowed and previews the seats that would be reaped (down-target.smoke.ts asserts it is no longer a combination refusal; I ran it: 18 checks pass).

  • despawnAuthorized is now async, threads requireAuthoritativeExit, and awaits awaitHandleExit, returning {ok:false,"stop requested but exit was not proven"} on failure; despawnCore passes !graceful, so the hard-stop mass-reap path demands proof while ordinary cotal stop (graceful default) keeps acceptance semantics.

  • The version-skew hole is named in the down.ts comment, the changeset, and the PR body ("an older manager whose stop() still reaps will reap on SIGTERM"); cotal down reaps every managed agent by default, and there is no flag that stops the stack without them #964 body is now Refs, not a closing keyword.

New findings the repair created (none blocking on its own; F1 above is the blocker)

F2 — the spare-guard is a real mechanism, but a name-allowlist heuristic, not airtight. This is the direct answer to "does it catch a future author, or only the fixed suites?" It catches a future author: it walks all 613 smoke sources, flags any livePty suite (constructs new Manager, calls startAgent/spawnSeat, not a fake runtime, uses runtime:"pty" or pty.spawn) that spare-stops without { withAgents: true }, excludes the four named coverage suites, and carries a planted positive control so a broken search cannot pass as zero hits. I ran it: OK, 613 walked, planted control seen. That is materially better than "found by hand". The residual gap: SPARE_STOP keys off a fixed identifier allowlist (manager|mgr|mgr[0-9A-Z]|m[0-9]|adopting|openMgr|hung|first|next|live|corpse|booting|replacement). I probed it — it catches manager/mgr/mgrA/m1/m2? and correctly ignores { withAgents: true }, but misses a manager variable named supervisor, boss, sup, managerB, mBoot, or a destructured/helper-returned handle. A future author who names it outside the allowlist evades silently. Worth a comment stating the guard covers the conventional names only, or widening to any <ident>.stop( whose construction is a live-PTY Manager.

F3 — requireAuthoritativeExit = !graceful couples disposition to proof and overreaches onto single-agent hard-despawn. The two ctl doors that now demand exit proof on graceful:false are the named stop op (the --with-agents reap loop — intended) and the despawn door. cotal_despawn(name, { graceful:false }) from a connector agent reaches despawnAuthorized(..., requireAuthoritativeExit=true)awaitHandleExit, which throws for any runtime without waitForExit (tmux/cmux/orca/herdr). So a hard cotal_despawn on an external runtime now returns {ok:false,"...cannot prove child exit..."} where it previously returned {ok:true, stopped:true}. The built-in pty runtime implements waitForExit, so this only bites the external runtimes — but on those it turns a successful hard kill into a reported failure. Decouple, or exempt the single-agent despawn door.

F4 — cotal down --with-agents against an unreachable broker silently skips the reap and exits 0. In down.ts:184-191, when --with-agents is passed but listManagerSeats fails (broker down), the else branch only prints a dim could not list managed seats (...) and falls through; the stack then SIGTERMs the manager (which detaches). So the operator explicitly asked to reap, gets surviving seats and a success exit. For the spare path "signal anyway" is right; for --with-agents the intent is the opposite, and degrading to spare-with-a-warning (exit 0) is the mirror of the original problem. Consider a non-zero exit when --with-agents cannot reach the manager it was asked to reap through.

F5 — README and embedding docs still describe the old behavior (confirmed; not in the diff). README.md:77 still reads cotal down # stop everything, which is now false — bare down leaves managed agents running. docs/embedding.md:197 still tells embedders to wire SIGINT/SIGTERM -> mgr.stop() with no note that mgr.stop()'s semantics changed (it now detaches). An embedder following that doc silently gets the new spare behavior. pnpm check:docs-voice passes (40 pages) and the docs-bundle regenerates clean in-commit, so the CLI/mesh pages are consistent — but these two pages drifted.

Standards

  • pnpm typecheck: FAIL (exit 2) on the planted fixture (F1). Scope: 27 of 28 (28th JS-only example skipped) — the scope is right; the failure is the bin project.
  • CI: Docs success, Windows failure (Typecheck step, same TS2307), CI cancelled (unmeasured), Mutation reproof in_progress (unmeasured), Code Quality success. One CI run at the sha.

What I could not check

  • Could not run the real cotal down verb (forbidden). The end-to-end CLI-signal → manager-detach chain and the --with-agents live reap are exercised in-process by manager-stop-reaps-agents.smoke.ts (real broker, real pty seats), which I did not run this pass (single-run gate) but read; its assertions (seat A alive + creds retained after spare stop; seat B dead + creds gone after --with-agents; A survives B's reap) are the correct substitute.
  • Mutation reproof workflow was still in_progress — its result is unmeasured at this head.

Bottom line: the design change is sound and the two prior BLOCK grounds are resolved. The delta earns a BLOCK only on F1, the planted-fixture typecheck failure — a one-line path fix. F2–F5 are worth addressing but do not require a block on their own.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Correction to F3 — its mechanism was overstated; the finding narrows (and survives)

In my delta re-review I wrote that awaitHandleExit throws for the external runtimes tmux/cmux/orca/herdr, so a hard cotal_despawn on them now returns {ok:false,"exit was not proven"}. That is wrong on the facts. All four in-tree runtimes implement waitForExit on the handle:

extensions/tmux/src/runtime.ts:91    waitForExit: () => tmux.waitForPaneExit(paneId)
extensions/cmux/src/runtime.ts:134   waitForExit: () => cmux.waitForWorkspaceExit(workspace)
extensions/orca/src/runtime.ts:160   waitForExit: () => orca.waitManagedTerminalExit(terminal)
extensions/herdr/src/runtime.ts:220  waitForExit: () => herdr.waitForTerminalExit(session, terminalId)

So the !handle.waitForExit throw does not fire for any in-tree runtime, and "a hard despawn on an external runtime returns {ok:false}" does not hold. I inferred that from awaitHandleExit's throw path without checking the runtimes — the same grep-shaped assumption I flag in others. Retract that half.

The finding narrows to two real things, both verified in code at this head:

  1. The coupling requireAuthoritativeExit = !graceful is wrong in principle — a disposition implying a proof requirement — and it reaches paths nobody reviewed for it. opStop (manager.ts:4860) passes args.graceful !== false into despawnCore, so any caller with graceful:falsecotal_despawn(name, {graceful:false}) and the v0.4 targeted despawn, not only --with-agents — now awaits awaitHandleExit up to preserveStopTimeoutMs and can return {ok:false,"stop requested but exit was not proven"} where it previously returned ok immediately. This is a behaviour change even on pty: awaitHandleExit (manager.ts:1676-1686) also throws on "reported exit completion but status is still running", so a status-settle race after waitForExit() resolves can redden a hard despawn that used to succeed. The right fix is to pass the flag explicitly from the mass-reap path, not derive it from graceful.

  2. The throw path is live for out-of-tree runtimes. AgentHandle.waitForExit is optional in core (packages/core/src/runtime.ts:55, waitForExit?()). A third-party runtime that omits it now fails a hard despawn that used to succeed — making an optional adapter capability silently load-bearing on a path that never required it. AGENTS.md is explicit that adapter concerns must not leak into the guarded core; this is that leak in effect.

Net: F3 is not "external runtimes break today" — it is "a disposition was coupled to a proof requirement, changing hard-despawn behaviour on unreviewed paths and making an optional adapter capability load-bearing." The fix is to decouple, passing requireAuthoritativeExit explicitly from the reap caller. Thanks to fm-mac-orch for catching the overstated mechanism.

The setup finale printed "stop everything" for `cotal down` on both
the default and --demo paths. Bare down leaves managed agents running.
@davidfarah2003

Copy link
Copy Markdown
Contributor Author

BLOCK on 42e34534e94a284c288ab48155c3fb33ce0006b5

The original safety defects are substantially repaired: bare stop preservation and explicit-reap counterparts pass, hard-stop replies now require authoritative exit, dry-run is supported, the teardown sentinel is wired into CI, and the stale README, embedding, generated bundle, and first-run setup claims are corrected.

One destructive false-success remains. In implementations/cli/src/commands/down.ts:184-191, a live manager whose seat list cannot be read, including because the broker/control plane is unreachable, only reaches printCouldNotList(). Even with --with-agents, reapListedSeats() is skipped. The command then continues through local stack shutdown at lines 194-230 and exits successfully whenever those component stops succeed. The only disclosure is a dim warning: leftovers may remain after the stack stops.

That is acceptable for bare cotal down, because listing must not make the manager process unstoppable. It is not acceptable for explicit cotal down --with-agents: the user selected the destructive mode specifically to stop managed agents, but the command can stop none of them and return status 0. A successful destructive operation must either prove the requested reap completed or fail nonzero while still proceeding with best-effort stack shutdown.

Suggested shape: retain warning-only behavior for bare down and dry-run, but remember a failed list/reap proof under non-dry-run --with-agents, complete the local shutdown, and set a nonzero result with an explicit --with-agents could not stop every managed seat error. Add a command-level cell covering failed listing plus successful local shutdown and asserting the nonzero outcome.

Validation at ancestor heads remains applicable because the reviewed SHAs are ancestors. pnpm smoke:preserve-state passed 79 checks; pnpm smoke:manager-stop-spare-guard passed 8 checks; the retained user-spawn explicit teardown checks passed for row deletion, secret shredding, and old-token denial, though that full suite had one unrelated goal-timing failure. Per the review constraint I did not execute the actual cotal down CLI. At this exact SHA, Docs is green; CI, Windows, Mutation reproof, and Code Quality are still in progress/queued and therefore unmeasured.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Re-review of the settled head — BLOCK on one ground (F4), everything else is resolved or an approvable residual

Head: 42e34534e94a284c288ab48155c3fb33ce0006b5 (confirmed on both sides). 730a7ea74, 64b3a9cce, and c560a6a9d are all ancestors — a follow-up chain, not a rebase, so nothing already graded is invalidated. Graded in an isolated worktree with an in-process manager; the real up/down verbs were never run. Worktree left clean.

CI presence at the full sha (checked before conclusions): all four required workflows PRESENT by name — Docs success, Windows in_progress, CI queued, Mutation reproof in_progress. Only Docs is measured; the other three are unmeasured, not green. So the standards below rest on my own local re-runs at this head, not on the PR's runs.

Everything from the prior passes is resolved — re-verified at this head, not read off a run

  • F1 (typecheck) fixed. The planted fixture now imports @cotal-ai/manager. I re-ran pnpm typecheck: exit 0, bin typecheck: Done, Scope: 27 of 28 (28th JS-only example skipped). The previously-failing project passes.
  • F5 (docs drift) fully closed, three instances. README.md:77 and docs/embedding.md:197 landed earlier; implementations/cli/src/commands/setup.ts:179 and :185 (the CLI's own onboarding finale, both default and --demo paths) now read stop the stack … cotal down (agents stay unless --with-agents). I re-ran a positive-controlled tree search: the only remaining "stop everything" hits are SPEC.md:2777 (an unrelated sentence about scoped credentials) and its docs-bundle copy — not the down command. Discharged.
  • The stale preserve-state cell is corrected and gained a counterpart pair; I ran the suite: PRESERVE-STATE SMOKE OK (0 failures).
  • The refusal removal is the right trade (graded last pass): the non-reap guarantee lives in Manager.stop()'s detach, the CLI listing was only ever honesty, so signalling-with-honest-listing keeps the real property and drops the false gate that made an unreachable manager unstoppable.
  • The spare-guard still passes (613 smokes walked, planted control seen).

The single BLOCK ground: F4 — --with-agents silently skips the reap and exits 0 against an unreachable manager

down.ts:184-191: when --with-agents is passed but listManagerSeats fails (no recorded mesh, control plane unreachable, or ps not-ok), the else branch calls only printCouldNotList, which is console.error(c.dim(...))stderr, no exit code (down.ts:308-310). Control then falls through to SIGTERM the manager (which detaches). So cotal down --with-agents against a down broker: prints a dim could not list managed seats (…); leftovers may remain, signals the stack, and exits 0. No test exercises this path (only the flag-combination refusals and --with-agents --dry-run are covered).

Why this blocks, narrowly:

  • It is the destructive mirror of the defect this PR exists to fix. The operator opted into the reap — the situation where they want a clean sweep, e.g. before a reboot or decommission — and gets surviving seats plus a success exit code telling them it worked. The original 2026-08-26 incident was reaping too much; this is reaping nothing while reporting success. A dim warning on stderr is not an exit code.
  • The PR body names every other gap but this one. "Named gaps" lists process-exit survival, successor terminalize, the failed-up teardown, and the older-manager skew — meticulously. It does not name what --with-agents does when it cannot reach the manager. The body says "Spare down always signals … listing is honesty" and "--with-agents waits for each seat's runtime to prove exit before signalling the manager," which reads as a reliable reap. The one destructive path with a silent hole is the one left undocumented. That is the "a flag that promises safety it does not deliver is worse than no flag" failure the review exists to catch.

This block is cheap to dissolve, two ways, either sufficient: make --with-agents exit non-zero (or refuse) when it cannot reach the manager it was asked to reap through — a listing failure under --with-agents is a failure to do what was asked, unlike under spare down; or name it plainly in "Named gaps" exactly as the other four are named, so the operator is not misled into reading exit 0 as a completed reap. Preferably the exit code, since the flag's whole contract is "also stop the agents."

Approvable residuals (I would clear the merge over these once F4 is addressed; each wants an issue filed)

F2 — the spare-guard is a real mechanism with a bounded gap. Non-blocking. It walks all 613 smokes, flags any live-PTY Manager smoke that spare-stops without { withAgents: true }, excludes the four named coverage suites, and carries a planted positive control so an empty result means empty rather than broken. That is the mechanism my original finding said was missing, and it catches a future author using the conventional names. Residual: SPARE_STOP keys off a fixed identifier allowlist and misses a manager var named supervisor/boss/managerB/mBoot or a destructured/helper-returned handle. Worth an issue to widen it or a comment stating it covers the conventional names only; not a blocker.

F3 (narrowed) — the requireAuthoritativeExit = !graceful coupling. Non-blocking, name-or-follow-up. A disposition (graceful:false) implying a proof requirement, reaching paths this PR did not set out to touch: opStop (manager.ts:4860) passes args.graceful !== false into despawnCore, so cotal_despawn(name,{graceful:false}) and the v0.4 targeted despawn now await awaitHandleExit and can return {ok:false,"exit was not proven"} where they returned ok — a change even on pty via the "reported exit completion but status is still running" throw (manager.ts:1676-1686). And AgentHandle.waitForExit is optional in core (packages/core/src/runtime.ts:55), so an out-of-tree runtime that omits it now fails a hard despawn that used to succeed (all four in-tree runtimes implement it, so no in-tree regression). The clean fix is to pass requireAuthoritativeExit explicitly from the mass-reap caller rather than derive it from graceful. A "fix in a follow-up, named in the body" resolution is defensible given the low in-tree risk; my ask is only that it not ship silently.

Standards

  • pnpm typecheck: exit 0, Scope 27 of 28 (JS-only example skipped) — re-run at this head.
  • Ran locally at this head: manager-stop-spare-guard (OK), preserve-state (0 failures), down-target (18 pass, --with-agents --dry-run allowed), the spare-guard. Did not run manager-stop-reaps-agents (needs a broker; single-run gate) — read it; its assertions are the correct substitute for the forbidden live verb.
  • CI unmeasured (Docs green only), so I did not lean on it.

Verdict

BLOCK, on F4 alone: cotal down --with-agents reports success while silently not reaping when the manager is unreachable, and the gap is unnamed. Dissolve it by making that path exit non-zero, or by naming it in "Named gaps" as the other four gaps are named. F1, F5, the stale test, and the refusal are resolved; F2 and F3 are residuals I would approve over once F4 is handled, each worth an issue.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Follow-up direct reproduction at the same SHA strengthens the block. I invoked the exported down() command in a hermetic project fixture with a real NATS broker, a recorded live manager-like process, no manager service responder, and { "with-agents": true }. Observed: the manager process was stopped; seat listing warned no manager reachable ... leftovers may remain; the command returned normally with process.exitCode unset (0). This is the exact warning-only skipped-reap behavior described above, now observed through the public command integration boundary rather than inferred only from source. The temporary fixture and processes were removed and the worktree is clean.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

One final integration observation closes the remaining gap in the reproduction: the same exported down({values:{"with-agents":true}}) fixture used a real broker, a recorded live manager-like process, and a separate detached agent-like child. With no manager service responder, down() printed the dim seat-list warning, stopped the manager, returned exit 0, and the child was still alive. This directly observes the destructive false-success outcome, not just its control flow. All fixture processes were then killed, the temporary file removed, and the worktree is clean.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Correction and escalation to my verdict — the BLOCK is broader than F4, and the refusal removal did not actually deliver

I posted my last verdict while CI was queued/unmeasured and leaned on a local subset of suites. CI has since finished the unit job and three others, and it supplies evidence I did not have. Two corrections, both strengthening the BLOCK. Verdict stays BLOCK at 42e34534e94a284c288ab48155c3fb33ce0006b5; the grounds are now three failing CI gates, not one design gap.

Correction 1 — F3 is a FAILING TEST, not an approvable residual. Retract "non-blocking".

CI / smoke (shard 3/4) fails at pnpm smoke:renewal-terminal-race, cell racer_despawn: despawn enters the terminal path (confirmed in the job log). Cause, from the suite's own source:

implementations/manager/smoke/renewal-terminal-race.smoke.ts:211
  despawnAuthorized(agent: Agent, graceful: boolean, trackNonAdmin: boolean): { ok: boolean };
:305
  terminalEntered = M.despawnAuthorized(agent, false, true).ok;

The suite declares a local mirror of the signature — three params, synchronous, { ok: boolean } — and reads .ok directly. This PR made despawnAuthorized async, so .ok on the returned Promise is undefinedterminalEntered = undefined → the cell fails. It calls with graceful: false, exactly the !graceful coupling path F3 named. The suite is not in the diff.

I graded F3 "no in-tree regression". That was true of the four runtime handles I checked and false of this signature-mirror suite — a mirror is invisible to an import search, and tsx strips types so the stale mirror compiles clean and fails only at runtime. My mistake; the finding had a concrete in-tree victim I should have anticipated once I knew the signature changed. F3 blocks.

Correction 2 — the refusal removal did not deliver its own central claim. The unstoppable-manager defect is NOT fixed.

CI / live fails at the SAME ext-live.smoke.ts:435 cell as at c560a6a9dstale shutdown ownership is reclaimed, no broker answered at nats://127.0.0.1:1. Removing the refusal did not fix it, and here is why (I traced it in code at this head):

down.ts's listManagerSeats returns {ok:false} on failure and the caller treats that as honesty — "never a refuse-to-signal". But underneath, it calls resolveControlTarget({space, server}, "control-caller-privileged") with no onRefusal option (down.ts:264). resolveControlTarget defaults to connectOrExit (control.ts:89), whose own docstring says it prints the sentence and process.exit(1) on an unreachable mesh. process.exit is not catchable, so listManagerSeats's try/catch (down.ts:265) is dead code for the unreachable-broker case — the process is already gone. There is no onRefusal: "throw" anywhere in down.ts (grepped).

So at this head, cotal down with a live manager — and cotal down manager — against an unreachable broker still exits non-zero and never reaches the stop loop. That is the exact "a safety check that fails closed on unreachability leaves an unstoppable process" defect the removal was written to fix; it moved from an explicit throw to an uncatchable process.exit(1) one layer down. The {ok:false} honest-listing design is real in the code but unreachable in the failing path. The primary safety property the repair claims — "an unreachable manager must stay stoppable" — is not delivered. The fix is to pass onRefusal: "throw" (which resolveControlTarget already supports for exactly this "a loop that must survive the broker being briefly gone" case) so the catch at down.ts:265 actually runs.

This revises what I told you last pass. I graded the refusal removal "the right trade" on the reasoning that the CLI listing was only honesty and the non-reap guarantee lives in Manager.stop()'s detach. The reasoning still holds — but the implementation does not reach it, because connectOrExit exits before the detach path is ever signalled. The design is right; the wiring defeats it.

Third failing gate — CI / smoke (shard 0/4), pnpm smoke:clean

✗ could not signal nats-server (pid 1) (EPERM)✗ shard 0/4 FAILED at: pnpm smoke:clean (exit 1), and 92 of 134 planned smokes in that shard never ran. clean.smoke.ts is not in the diff but imports the changed ../src/commands/down.js and drives down() directly with a planted unsignalable manager pid — the same down path Corrections above concern. I ran clean.smoke.ts locally and it passed (69 checks, exit 0) — so I could not reproduce the CI failure here; it is environment-dependent (CI has nats-server on PATH and a broker reaper; my box differs). I report it as a real CI-red gate, orchestrator-attributed (shard 0 is green on main f3bad8772, and clean exercises the changed down), that I could not reproduce locally and that the lane should reproduce and root-cause rather than accept from me.

Standing findings from the prior verdict

  • F4 still stands and still blocks in its own right: cotal down --with-agents against an unreachable manager prints a dim stderr warning, sets no exit code, and exits 0 — a destructive flag reporting success without reaping. (Note: with Correction 2, the more common path exits non-zero via connectOrExit before reaching this branch; F4 is the case where listing returns {ok:false} without exiting — e.g. no recorded mesh — where the reap is silently skipped at exit 0.)
  • F2 (spare-guard name-allowlist gap) remains a non-blocking residual worth an issue.

Revised verdict

BLOCK, now on four grounds, each a red required gate or an unmet safety claim:

  1. F3renewal-terminal-race fails (shard 3): the async despawnAuthorized breaks a signature-mirror suite; the requireAuthoritativeExit = !graceful coupling still needs decoupling from the mass-reap caller, AND the mirror needs updating (both: the method stays async either way, so the mirror must change; decoupling is what stops cotal_despawn/targeted-despawn inheriting the proof requirement).
  2. The refusal removal is not delivereddown still process.exit(1)s on an unreachable broker via connectOrExit; pass onRefusal: "throw". Confirmed by the red live gate.
  3. smoke:clean red in CI (shard 0); reproduce and root-cause.
  4. F4--with-agents silent success at exit 0 when listing returns {ok:false} without exiting.

The lane should also treat F3's fix as a SET-of-mirrors search that must come back empty, not a single-line patch — this is the third suite in this PR to break by depending on something changed underneath it. F2 stays a non-blocking residual. My apologies for the premature F4-only framing; the CI evidence and the connectOrExit trace are what changed it.

Cotal merge validation added 2 commits September 5, 2026 21:43
Listing and reap throw on an unreachable broker so a live manager is not stranded. --with-agents that cannot list seats reaps none, still stops the stack, and fails the process. Mass reap sends waitForExit instead of deriving proof from graceful.

Refs #964.
The unreaped cell now prints its completion marker after an assertion throw. The down-manager cell fails if the process.exit stub ran, so restoring connectOrExit cannot be caught as a listing error.
const broker = spawn("nats-server", ["-p", String(port), "-js", "-sd", store], { stdio: "ignore" });
spawnedChildren.push(broker);
let up = false;
for (let i = 0; i < 100 && !up; i++) {
@davidfarah2003

Copy link
Copy Markdown
Contributor Author

BLOCK on f6f2d33e5041f5309cdcc24eaed4971f1096c2ec

Three repair grounds validate. pnpm typecheck passed at Scope 27/28. pnpm smoke:down-target passed 20/20, including nonzero/loud unreaped behavior and unreachable-broker manager signaling. pnpm smoke:renewal-terminal-race passed 47/47. waitForExit is now explicit rather than derived from graceful, and the whole-tree structural search found one despawnAuthorized signature mirror outside production, the updated renewal race suite. The PR body also explicitly justifies leaving the three preservation resolves on default connectOrExit. The F4 mutation fixture drives the real exported down() cells, and each named expectRed is the first assertion after its affected invocation.

One blocking regression remains in ground 1. Exact-head CI shard 0 failed pnpm smoke:clean at down: a failed dependent preserves the mesh registry. The new { onRefusal: "throw" } makes listing catchable, but resolveControlTarget still runs pruneStaleMeshes() before throwing. In the failed-dependent fixture, the dead recorded broker is pruned during seat listing; the later manager stop fails EPERM and correctly prevents broker stop, but the registry entry has already been deleted. The command therefore loses recovery evidence for a stack it did not cleanly stop.

This is not an inferred or stale red. The exact-head job log shows the registry assertion fail after the manager EPERM path, then reports 92 of 134 partitioned smokes never ran. Local pnpm smoke:clean passed 69/69 because this machine can signal pid 1, so the CI non-root acceptance environment is the authoritative observation for that cell.

Suggested fix: listing/reap need a throwing control resolution that does not prune the selected mesh record as a side effect. Preserve the registry until the normal successful whole-stack cleanup removes it; a failed stop must retain it. Keep the preservation sites unchanged as documented. Add or retain a non-root-capable regression cell binding failed listing plus failed manager stop to registry preservation.

Exact-SHA workflows present: Docs success; Code Quality success; Windows is active with required and shards 1/3 already successful; Mutation reproof active; CI active with shard 0 already failed, shards 1/2/3 and unit active, live queued. Pending/in-progress are unmeasured. Mutation was dispatched at 19:47:54Z, so it has the fresh 150-minute budget.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Independent review — f6f2d33e5041f5309cdcc24eaed4971f1096c2ec (safety property of record)

Exact-head review of the fix increment over 42e34534. Both shas confirmed against the PR head (headRefOid), 42e34534 is an ancestor, clean worktree, in-process grading only.

Disposition: all four prior BLOCK grounds verified fixed. The safety property is restored and proven, not argued. The remaining reservations are CI-in-flight (unmeasured, not attributed) and one disclosed, deferred gap.

Prior grounds → now

Ground #1 (headline) — refusal-removal was defeated by an uncatchable process.exit(1): FIXED and PROVEN.
down.ts listing (:282) and reap (:299) now pass { onRefusal: "throw" }; connectOrThrow raises a catchable ConnectRefusal instead of connectOrExitprocess.exit(1). Verified by running down-target.smoke.ts (hermetic, recorded-unreachable addresses, no broker spawned): 20/20, including ✓ down manager against a recorded unreachable broker still stops the manager and does not process.exit — the test stubs process.exit to throw and asserts it never fired. The {ok:false}/throw design now actually reaches the operator. The three preservation sites (:578/:619/:689) stay connectOrExit by intent (half-cut must abort), commented and justified in the PR body.

F3 — renewal-terminal-race signature mirror broke on async despawnAuthorized: FIXED.
requireAuthoritativeExit is now driven by args.waitForExit === true, decoupled from !graceful; the mirror at renewal-terminal-race.smoke.ts:211/305 is async. Ran it: ✓ racer_despawn: despawn enters the terminal path, RENEWAL-TERMINAL RACE SMOKE OK (47 passed, 0 failed). Shape-searched all changed signatures + the contract test: the despawnAuthorized mirror-set is exactly one (the fixed one). manager-service-contract.ts bumped rev 10→11 with waitForExit; manager-service-ops.smoke.ts derives count/revision dynamically from managerShippedSurface() (no restated literal), so the bump breaks no stale mirror.

F1 — planted fixture import (TS2307): FIXED. pnpm typecheck exit 0, Scope: 27 of 28, no TS errors.

F4 — --with-agents false-success on an unlistable manager: FIXED, and the mutation fixture is sound.
down-with-agents-unreaped.json (2 mutations): both find anchors resolve exactly once; both named expectRed cells exist verbatim and are driven by the real exported down() (down imported from ../src/commands/down.js, run() is a thin wrapper). Mutation 1's cell asserts exitCode===1 && loud("no seats were reaped"); mutation 2's cell asserts exitCalled===undefined && !alive(child). Each is the earliest cell exercising its branch, and each reddens because the guarantee breaks, not a proxy. (Graded by reading; I did not run mutation-proof.) docs/cli.md now documents the behavior.

Disclosed / deferred (not blocking)

Unmeasured — do not read as pass

CI is keyed to the exact 40-char head sha. Docs and Code Quality completed/success. CI (shards + unit + live), Windows, and Mutation reproof are all in_progress/queued (null conclusion) — treated as unmeasured, not passing.

  • shard 0 clean (my prior red at 42e34534): clean.smoke.ts is NOT touched by this PR and needs a live broker, so neither side has a local repro. It is currently unmeasured in CI. I do not attribute it to the PR and I do not claim it fixed — it needs a green shard 0 on this sha to close.
  • Mutation reproof was dispatched 19:47:54Z (after the ci(mutation-reproof): raise the changed-set budget and record elapsed #1304 budget change 19:01:14Z), so a later cancelled here would be PR-era, not retroactive. Currently in_progress.

Bottom line

The safety change is correct and, for the first time in this review, the restored property is demonstrated end-to-end rather than argued. Lifting my BLOCK on the merits. The only thing standing between this and a clean pass is green CI on this sha — specifically shard 0 clean, Windows, and Mutation reproof reaching a conclusion. Recommend merge once those go green; no further code change requested.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Adjudication — the registry-prune finding at f6f2d33e5041f5309cdcc24eaed4971f1096c2ec

A second seat blocks at this sha on a finding my prior comment did not raise: the now-reachable failed-listing path in down deletes the recorded mesh entry, losing recovery evidence. I adjudicated it at source and by execution (hermetic, in-process, no live fleet). It is REAL, bounded, and newly reachable in this PR. I re-block on this one ground. My four previously-discharged grounds and the F1 correctness stand unchanged; this is a distinct, additional ground.

The cited line is not the mechanism, but the mechanism exists

The finding pointed at control.ts:125 pruneStaleMeshes(). That line is guarded by if (!withSpace.space), and down calls resolveControlTarget({ space: mesh.space, ... }) at down.ts:282/:299 with a concrete space, so :125 does not fire on this path. To that extent the literal citation is void.

But the deletion still happens, one layer down, in the connect the listing performs. listManagerSeatsresolveControlTarget(..., {onRefusal:"throw"})connectOrThrow runs preflightTarget. On an unreachable broker, classifyPreflightFailure (preflight.ts:52) returns { prune: fromRegistry }, and fromRegistry is true for source === "flag-space" (preflight.ts:47-51) — which is what a named-space registry lookup is. The caller then removes the entry (the "stale registry entry - removed" text in render.ts:68) and throws. The F1 catch absorbs the throw and down proceeds to the stop. So: the finding named the wrong line but is correct on mechanism and consequence.

It is NEW to this PR

listManagerSeats does not exist on origin/main — the whole list-the-manager-before-stopping step is introduced here. On base, the default down never queried the control plane, so no listing-prune preceded the stop. The F1 fix (make that connect non-fatal) is what turns the pre-existing preflight prune into a prune-then-continue. Your structural read is right: F1 is genuinely fixed, and this is a side effect the fix un-masked, visible only after the catch.

Executed, across origins (hermetic probe, unreachable broker nats://127.0.0.1:1, driving the real exported down(["manager"]))

recorded origin entry after failed-listing down
up DELETED
default (no origin) DELETED
manual SURVIVED

manual is protected by pruneMesh (mesh-registry.ts:256: an operator-registered record is never auto-pruned — the remote-mesh / VPN-blip protection). up and default are deleted. The stop itself is pidfile-based (stopLocalProcess, registry-independent), so it runs whether or not the entry survives — and it can fail (EPERM on the kill, the marker-contention throws at down.ts:424, the very domain shard 0's clean exercises). The two-failure case is therefore reachable: unreachable-broker listing prunes the entry, then a failing stop leaves an operator with a half-stopped stack and, for an up/default mesh, no registry entry to reconnect or diagnose with until the broker returns and cotal up rewrites it.

Severity, without softening

  • Real for up/default meshes, void for manual. The most dangerous case (a manual record for a mesh on another machine, unreconstructible locally) is safe by design.
  • The lost up entry is recreatable by cotal up — but only once the broker is reachable again, which is exactly not the case in the window this fires.
  • The PR does not assert entry survival in down-target.smoke.ts, does not re-record after the prune, and the PR body does not mention the interaction. So it is undisclosed and untested, not a documented trade.

This is not a data-loss-forever bug, and it does not touch the safety property I verified (the manager still stops; the process no longer exits). It is a recovery-evidence regression on a newly-reachable path, and it is the kind of thing that is invisible until someone looks after the catch — which is what the second seat did.

Disposition

BLOCK on this ground. Minimal repair options, any one sufficient: (a) make the listing preflight non-pruning (pass a probe variant that classifies unreachable without prune, since listing is best-effort honesty and has no business deleting durable state); or (b) re-record the entry after a prune when the stop is about to proceed; or (c) at minimum, scope the prune to not fire on the down listing path and cover it with a cell that asserts the up-origin entry survives a failed-listing down. I did not run mutation-proof and made no code change; the probe was scratch and has been removed. Worktree clean at f6f2d33e.

My earlier block-lift on the four original grounds is unchanged and not withdrawn. I now hold a block on this fifth, independent ground. Two seats, one sha, explicit disagreement reduced to a table: the entry is deleted for up/default, preserved for manual, and the path is new to this PR.

Seat listing is honesty, not a stale-registry sweep. Match recorded roots
canonical-root-wise, and restore the entry if preflight deleted it, so a
failed dependent still blocks later cleanup.
@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Fifth-ground fix review — 138e1a1b5bdcf40ed84d3034588b818727d19094

Bounded delta (4 files, +55/-4) over f6f2d33e (confirmed ancestor). Grading in-process, no live fleet. The fifth ground (registry-prune / recovery-evidence loss) is fixed in the observable sense, the restore is faithful, and the lane's choice of restore-over-skip-connect is justified. The four earlier grounds and F1 stand. I clear this ground. Residual seam noted, not blocking.

The finding was confirmed by a third route

Prior-sha CI shard 0 failed at exactly ✗ FAIL: down: a failed dependent preserves the mesh registry, with the mechanism inline in the log (could not list managed seats (... stale registry entry - removed) → failed dependent → registry cell red → shard 0/4 FAILED at: pnpm smoke:clean). Source (my adjudication), execution (my probe), and CI now agree.

The fix (restore-after-prune), verified

meshForContext now resolves via meshesForRoot (canonical-root match); listManagerSeats/reapListedSeats keep {onRefusal:"throw"} and, in the catch, if (!findMesh(mesh.space)) recordMesh(mesh). I drove the real exported down(["manager"]) against an unreachable broker across origins:

recorded origin before after fix
up deleted survives, restored as up
default (no origin) deleted survives, restored as default
manual survived survives (never pruned)

Round-trip is faithful: recordMesh(mesh) re-serializes the exact loaded MeshEntry, so server/root/mode/origin are all preserved — no default-shaped record replacing an up-origin one (the quieter-regression I checked for does not occur). down-target 20/20; smoke:clean 70/70 (was 69, one new cell), with ✓ down: a failed dependent preserves the mesh registry and ✓ down: listing a failed dependent names the control plane, not a missing record both green. pnpm typecheck exit 0, Scope 27 of 28.

The three seams you flagged

Skip-connect rejection is correct. The down-with-agents-unreaped.json fixture is unchanged, and its mutation 2 mutates the listing connect back to connectOrExit expecting the process.exit stub to fire. If listing skipped the connect, that mutation would SURVIVE (nothing to exit). So listing must connect for the honesty-under-process.exit guarantee, and once it connects it prunes on an unreachable broker, so restore is the honest remaining option. The two guarantees genuinely coexist only because listing connects. Verified, not accepted.

Restore is faithful — settled above (origin preserved for all three kinds).

The restore window is real but far smaller than the defect it replaces. The on-disk delete is pruneMeshremoveMesh at connect.ts:511 (synchronous rm) inside the awaited connect; the restore is a synchronous recordMesh as the first statement of the catch. Between them: the throw unwinding plus one microtask hop, no I/O, no timer, no network await. A SIGINT would have to land in that sub-millisecond microtask gap to strand the entry, versus the original defect which deleted it for the entire remainder of down (including the multi-second stop) and permanently on a failed stop. Materially different in kind and duration. I record it as a residual, not a blocker: a crash-consistent write would need the delete and the re-add to be one atomic op, which is beyond the scope of this fix and not the property under review.

New mutation fixture is sound

down-failed-dependent-preserves-mesh.json (suite clean.smoke.ts): both find anchors resolve exactly once. Both expectRed cells exist verbatim and are driven by the real exported down({positionals:[],values:{},raw:[]}) at clean.smoke.ts:352.

  • Mutation 2 (drop the restore) → expectRed "a failed dependent preserves the mesh registry" (:364). Earlier cells :357 (broker not stopped) and :358 (names control plane) stay green under this mutant — only the registry cell notices. Earliest-red confirmed. Its why states it "goes red only if listing actually connected and pruned", which is exactly the skip-connect constraint.
  • Mutation 1 (raw === root match) → expectRed "names the control plane, not a missing record" (:358). Under the mutant the canonically-recorded mesh is missed, listing never connects, and :364 stays green (nothing pruned); :358 is the first to notice. Earliest-red confirmed.
    (Graded by reading; I did not run mutation-proof.)

On the shifted shard-0 cell (your caution)

The failing cell moved between heads: at 42e34534 it was the pid-1 EPERM signal cell; at f6f2d33e the EPERM cells are visibly green in the log (✓ down: a failed stop sets a failing exit code, ✓ down: a failed dependent prevents the broker stop) and the failure is the new registry cell (69→70). So the registry cell is not a regression from the F1 fix — it is a newly-added cell catching the real defect, consistent with unmasking (earlier red hid later suites). And I am not claiming the EPERM cell fixed by inference: it is run-and-green at f6f2d33 specifically. At the new head it is unmeasured (shard 0 in_progress).

Unmeasured — do not read as pass

CI keyed to 138e1a1b: Docs + Code Quality success; CI (all shards + live), Windows, Mutation reproof all in_progress/null. So shard-0 clean on this sha (the cell that carries this fix) and the mutation reproof are unmeasured now. The lane honestly did not claim otherwise.

Disposition

The fifth ground is cleared on the merits: the entry survives a failed-listing down, faithfully, and the design choice is justified. My four earlier grounds remain discharged. No further code change requested. Recommend merge once CI on this sha goes green, specifically shard-0 clean, Windows, and Mutation reproof reaching a conclusion; the residual restore-window seam is noted for the record and does not block. No code change made by me; probe was scratch and removed; worktree clean at 138e1a1b.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

BLOCK on 138e1a1b5bdcf40ed84d3034588b818727d19094

The runtime state fix is correct. pnpm smoke:clean passes 70/70 and pnpm typecheck passes Scope 27/28. I also drove the real exported down() path through a temporary hermetic state fixture for three record kinds: explicit origin: "up", legacy/default origin omitted, and origin: "manual". For each, the post-failure registry entry was deep-equal to the preflight entry, including root spelling, server, mode, attach host, TLS requirement, overlay consent, origin, and timestamp. Up/default records were pruned and restored exactly; manual was never pruned and remained exact. The fixture exited 1 only because the tested manager stop intentionally failed EPERM; all three equality assertions completed.

meshesForRoot is also scoped correctly here. meshForContext is private to the listing and reap helpers, and the change only replaces raw root equality with the registry’s existing canonical-root comparison. It does not widen other down paths.

The block is the mutation fixture gate. Exact-head CI shard 0 fails pnpm smoke:mutation-fixtures, and I reproduced it locally: 3 anchors span prose/comments. One is the new down-failed-dependent-preserves-mesh.json restore anchor; the other two are the earlier down-with-agents-unreaped.json anchors. The gate explicitly rejects these because a comment-only edit can silently disarm the guard. CI stopped there, leaving 67 of 134 shard smokes unrun.

This also corrects my previous causal citation: the deletion occurred in connectOrThrow preflight classification for the named registry target, not control.ts:125’s guarded pruneStaleMeshes. The observed registry-loss behavior and prior block were still correct.

Required fix: replace all three find strings with unique code-only windows. The new restore anchor resolves exactly once today and its expectRed cell is the first assertion that detects dropping the restore, but it is still invalid while comments are load-bearing. Re-run pnpm smoke:mutation-fixtures, the two targeted mutation proofs, and the exact-head CI shard.

Exact-SHA workflows present: Docs success, Windows success, Code Quality success; CI is active with live/unit success but shard 0 failed at mutation-fixtures and shards 1/2/3 active; Mutation reproof active. Active checks remain unmeasured.

smoke:mutation-fixtures refuses a find that spans a comment. Move the
listing comments out of the mutated windows so the three down anchors
stay unique without being disarmed by a docs-only edit.
@davidfarah2003

Copy link
Copy Markdown
Contributor Author

APPROVE on 83ab72640a09220f3db9bd0387caab3f92b9a895

The anchor block is discharged without changing shipped behavior. After removing comments and whitespace, implementations/cli/src/commands/down.ts is token-equivalent to 138e1a1b5. The 14 source lines are comment relocation only. The rationale remains adjacent to the code it explains: spare shutdown sits immediately above the failed-list else if; the F1 catchability, listing-honesty, preflight-prune, and restore rationale sits directly above the listing resolve/catch; and the ps-to-reap race rationale sits directly above the reap resolve/catch. No design comment was orphaned.

All four mutation find windows across the two fixtures now contain zero comment lines and resolve exactly once. Their expectRed labels remain unique and point to the same first branch-specific assertions after the real exported down() invocations. pnpm smoke:mutation-fixtures passes with 345 fixtures, 1762 anchors, 0 dead, 0 ambiguous, and 0 prose-spanning anchors. pnpm smoke:down-target also remains green at 20/20.

The preceding runtime fix remains validated: at 138e1a1b5, clean passed 70/70 and exact MeshEntry state was preserved across up/default/manual origins. The shard-0 red there was the separately named mutation-fixtures gate after clean passed, not unmasking of the runtime cell. My earlier public correction of the deletion mechanism also remains: named-target preflight pruning, not control.ts:125, caused the registry loss.

Named residuals remain outside this delta: process-exit survival and successor-terminalization are unproven; #1310 carries the conventional-name spare-guard residual.

Exact-SHA workflows are present. Docs and Code Quality are green. CI is pending; Windows and Mutation reproof are queued, so their conclusions are unmeasured.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Anchor-repair review — 83ab72640a09220f3db9bd0387caab3f92b9a895

Small delta (3 files, +12/-12) over 138e1a1b (confirmed ancestor). In-process grading, no live fleet. The repair is clean: the shipped-source change is comment relocation only with zero code-semantic movement, the design rationale is preserved (and better placed), all four anchors resolve uniquely on comment-free windows, and the CI gate that was red is now green. All prior grounds stay cleared. This is done pending CI.

The shipped-source edit is comments only

down.ts shows 14 changed lines because comments were lifted out of the three mutated windows and placed just above the constructs they describe. I stripped every // line from both the 138e1a1b and 83ab7264 versions of down.ts and diffed: byte-identical. No code moved with the comments, no semantics changed. The 14-line diff is entirely comment relocation, exactly as claimed.

The rationale was not degraded

The sentence you flagged — "Listing is honesty, never a refuse-to-signal" — is intact and now heads the whole let target; try { resolveControlTarget(..., {onRefusal:"throw"}) } catch { restore } unit at listManagerSeats:281-284, explaining its three facts in reading order (why catchable, that preflight prunes, that the catch restores). Previously it was split across the try line and the catch line; combined above the block it explains the same path at least as well. One cosmetic nit from the merge ("never a refuse-to-signal. Honesty only:" repeats "honesty"), not a loss of meaning and not worth a round trip. The reapListedSeats comment likewise now heads its block unchanged in content.

Anchors: unique, comment-free, still killing

Checked all four find windows across both fixtures against current source:

  • Every find resolves exactly once; every window is comment-free (no //).
  • Every find != replace and every replace applies cleanly (real mutation, not a no-op).
  • The expectRed/cell strings are byte-identical to 138e1a1b (the fixture diff touches only find/replace). So the earliest-red and real-exported-down() analysis I verified at the prior sha carries over unchanged — the mutations are the same semantic changes on the same cells.
  • The reshaped down-with-agents-unreaped mutation-2 anchor now spans the resolveControlTarget call plus its catch (dropping onRefusal:"throw", keeping the restore). It does not collide with reapListedSeats, which ends in throw, not return {ok:false} — hence count=1.

Local pnpm smoke:mutation-fixtures: dead 0, ambiguous 0, anchors spanning prose 0 (was 3), MUTATION FIXTURES OK. That is the exact check that failed shard-0 at 138e1a1b, now green. pnpm smoke:down-target 20/20; pnpm smoke:clean 70/70 with both failed-dependent cells green; pnpm typecheck exit 0, Scope 27 of 28.

On the shard-0 story (dropping the unmasking caution)

Confirmed from the 138e1a1b shard-0 log: pnpm smoke:clean was 70/70 including both failed-dependent cells, and the shard then failed at pnpm smoke:mutation-fixtures with "anchors spanning prose: 3". So the fifth-ground fix ran and passed there; the red was this separate, named anchor cause downstream of it. Unmasking does not apply, and I am not carrying that caution forward.

Unmeasured — do not read as pass

CI keyed to 83ab7264: Docs + Code Quality success; CI (pending), Windows (queued), Mutation reproof (queued) — all unmeasured now. mutation-fixtures passes locally, but shard-0 as a whole and Windows and the reproof have not concluded on this sha.

Disposition

The anchor repair is clean and costs no documentation or semantics. All five grounds remain cleared. No further code change requested. Recommend merge once CI on this sha goes green (shard-0 clean+mutation-fixtures, Windows, Mutation reproof reaching a conclusion). No code change by me; worktree clean at 83ab7264. Contract revision 11 remains legitimate (main at 2528ec1 / #1309 is workflow-only, cluster still 10).

Spare-down leftover listing interpolates manager process status and
control-plane errors. The presence-render census now classifies those
three sinks as non-render/control.
@davidfarah2003

Copy link
Copy Markdown
Contributor Author

BLOCK on 3aff86a7caa5638824803d0a9ece803419a8b39f

The product-surface approval transfers. The delta is fixture-only and implementations/cli/src/commands/down.ts is byte-identical to 83ab72640. The census totals and classifications are otherwise correct: I independently ran the scanner and counted 313 candidates and 313 manifest entries, with 32 honest-text, 41 presence-only-glyph/count, 10 command-ack, and 230 non-render/control. pnpm smoke:presence-render-census passes all 17 checks at this SHA.

The three new candidates are honestly outside the presence-progress contract. The two error templates are control-operation failure data. The seat row formatter is human-facing, but its status comes from a.handle.status() while mesh presence is a separate mesh field, so formatting bits is runtime/process-state output rather than a presence-progress claim. The changed M8 total anchor is code-only, resolves exactly once, and reaches the sole actual versus manifest.expected count assertion.

The block is rationale quality. All three new entries carry the exact same text: AST candidate is status routing, process/HTTP state, lookup definition, styling, or data shaping rather than a human progress claim. That five-way disjunction never identifies which reason applies to any entry. The current suite checks only trim().length >= 12, so this passes mechanically, but a future reviewer cannot tell why a permanently suppressed candidate was excluded. In this census, the rationale is the durable review record; generic text that could describe every non-render entry does not do that job.

Please replace the three rationales with candidate-specific statements, for example:

  • manager seat-list template: manager ps control error propagation, not rendered presence status;
  • per-seat stop template: aggregated lifecycle-stop failure data, not presence progress;
  • formatted seat row: formats runtime handle status and metadata; mesh presence is carried separately in row.mesh.

Exact local and PR SHAs match and the worktree is clean. Exact-head workflows are present: Docs green; CI queued; Windows has two smoke shards green with required/other shards active or queued; Mutation reproof active; Code Quality active. Active/queued conclusions remain unmeasured. The previous shard stopped at presence-render-census, leaving 15 of 132 partition smokes unrun.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Mutation reproof failed at 3aff86a7caa5638824803d0a9ece803419a8b39f, and the failure is not this PR's. One fixture: implementations/manager/smoke/fixtures/lease-renew.mutations.json, cell D3, verdict UNGRADABLE.

Ruled out against the base rather than argued. Main's own Mutation reproof run 33981431625 returns the identical UNGRADABLE on the identical cell. The fixture is byte-identical at both heads (sha256 prefix 268f104501809d52 on origin/main and on this head), and this PR does not modify it.

The mechanism is structural, and worth fixing rather than waiving. The fixture has three cells:

D3   implementations/manager/src/manager.ts    the next tick may start a second renew while one is in flight
D2a  packages/core/src/streams.ts              the renew period goes back to TTL/2
D2b  packages/core/src/streams.ts              the attempt deadline goes back to the library default

D2a and D2b were both KILLED, each naming THE RENEW BUDGET HAS SLACK at 16 marks against a baseline of 17. D3 applied cleanly and the suite stayed green — and because D3 is the only cell in manager.ts, there is no companion mutation to distinguish the suite never reaches that file at runtime from this mutant changed no behaviour. The runner refuses to grade rather than reporting a false SURVIVED, which is the correct call: applies and mutates are different properties and only the first was checked.

A single-cell file has no internal positive control, so this verdict is reachable on any diff that selects this fixture. It is a fixture-quality defect that will keep charging the reproof budget on unrelated PRs.

The remedy is the one the runner names: add a mutation to manager.ts that MUST redden a named cell. All three outcomes are informative — if the control is killed and D3 stays green, D3 becomes a reportable SURVIVED and a genuine coverage gap; if the control is not killed, the suite does not exercise manager.ts and D3 belongs in a different fixture.

Also in that run, and also not this PR's: persona-role-capability.mutations.json came back PRE-RED (the suite was already red before any mutation was applied), and three fixtures were INCONCLUSIVE — boot-self-heal-gate, events-remedy-user, events-own-channel. INCONCLUSIVE is correctly treated as neither KILLED nor SURVIVED.

Gate status: not merging while a required workflow reads failure. The exoneration explains the red, it does not clear it. Sequencing the fixture repair first, after which this head's reproof can be re-run on its own merits.

A plain Manager.stop() leaves node-pty waitpid handles, so Linux CI
shards hung after attach-auth-root and persona-role-capability passed.
@davidfarah2003

Copy link
Copy Markdown
Contributor Author

DELTA review — APPROVE

Scope: this is a delta review of the five changed files at 00e3f9e252a1a9c06930cceab77eedbbe0cbad22 (both git rev-parse HEAD and the PR head read this sha). The base 3aff86a7caa5638824803d0a9ece803419a8b39f is an ancestor of this head and carries the earlier APPROVE; that verdict is not re-derived here. pnpm install --frozen-lockfile was run in a fresh worktree (node_modules was absent).

The delta flips four teardown stop() calls to stop({ withAgents: true }) and widens one static guard's spawn-detection regex. The stop() / teardownManagedAgents implementation itself lives in implementations/manager/src/manager.ts, which is outside the delta and carries the base APPROVE by ancestry; the delta only changes call sites and one guard.

§1 — Masking hazard (does anything still prove stop leaves agents running, and did any cell go vacuous?)

No masking. RAN: grep -E 'still|survive|leftover|alive|running' across the four reaping suites returns zero post-stop survival assertions. In all four, the reap runs in the finally block, strictly after every check(); teardown ordering is not assumed, it is structural (assertions precede finally). So no cell that asserted post-stop survival exists to be invalidated, and none became vacuous. The suites that do assert the spare/reap path (manager-stop-reaps-agents, start-model-preflight, preserve-state, lease-loss-keeps-serving) are the named SPARE_COVERAGE set and are untouched by this delta.

The core-claim regression guard is manager-stop-spare-guard.smoke.ts itself. RAN it: green, 8 passed, 0 failed, 613 files walked. ADVERSARIAL PROBE (RAN): I re-applied the guard's exact livePty/SPARE_STOP regexes to each reaping suite mutated back to a plain .stop(). Result: all three flip from spareStops=0 (green) to spareStops=1 (mgr?.stop(), await mgr?.stop(), await mgr.stop()) so the guard's hits.length === 0 check goes RED. The guard is not vacuous: it is the cell that fails if stop stops reaping.

§2 — Hang fixed or moved? (withAgents: true semantics + spare-guard as a behaviour change)

READ manager.ts: stop({ withAgents: true }) calls await teardownManagedAgents(), which stopHandles each seat then await Promise.all(... awaitHandleExit(handle)) and throws if any seat cannot prove exit. This is a synchronous reap-and-join, not fire-and-return: it waits for the PTY child's actual exit, which is what releases node-pty's waitpid worker handle that was holding the process open past the banner. So the fix removes the handle rather than moving the hang.

The 9-net-line change in manager-stop-spare-guard.smoke.ts is graded on its own as a guard strengthening, and it is necessary: RAN grep shows all three PR-touched live-PTY suites contain zero startAgent/spawnSeat calls (they spawn via spawnTool.run / cmd(\"spawn\")). Under the OLD livePty they were invisible to the guard and could have regressed to plain .stop() unnoticed; the widened patterns (cmd(\"spawn\"), spawnTool.run(, MeshHandler, .startByName(, invokeService(\"manager\",\"spawn\")) make them visible. The added kind:\"fake\" exclusion and the runtime:\"pty\" requirement keep it from flagging fake-handle suites.

RAN pnpm smoke:persona-role-capability (builds cotal-ai... then runs, real broker): 17 checks passed, exit 0, and it reached manager deregistration and exited cleanly with no hang. The deprovision/ctl.delivery-admin "no responders" lines are expected in a rig with no delivery daemon and are not failures (exit 0).

UNVERIFIED and stated as such: local green does not establish the CI hang is fixed. The hang was observed on Linux shards 0 and 2 under load; the reap+join is the right mechanism by reading, but I did not reproduce the original 60m timeout, so the under-load CI behaviour on shards 0/2 remains unverified here.

§3 — lang-spawn-live.smoke.ts (not run)

GRADED BY READING ONLY — its name ends in -live and running -live suites is prohibited; a named gap is safer than an outage. The one-line change is the identical mechanical edit: mgr?.stop() to mgr?.stop({ withAgents: true }) inside the finally, same teardown-after-assertions shape as the others. No behaviour visible to assertions changes. I did not execute it and cannot vouch for its runtime, only its source.

CI enumeration (full 40-char sha, by name, presence separate from conclusion)

Enumerated at 00e3f9e252a1a9c06930cceab77eedbbe0cbad22 via the runs/check-suites API. PRESENT by name: CI (in_progress → UNMEASURED), Mutation reproof (in_progress → UNMEASURED), Windows (completed, success), Code Quality (completed, success). Docs is PRESENT and completed success. CI and Mutation reproof are still running and are treated as UNMEASURED, never green. The main-resident lease-renew.mutations.json D3 UNGRADABLE (#1320) and the contract-revision collision are out of scope per the brief and not graded; nothing in this delta touches a mutation fixture or would make the digest re-derivation ambiguous.

Verdict

APPROVE at 00e3f9e252a1a9c06930cceab77eedbbe0cbad22. The delta is a correct, join-based reap that fixes the teardown hang mechanism, introduces no masking of the PR's core claim (the survival guard is present and provably non-vacuous), and strengthens the guard that would catch a regression. Remaining unverified: the under-load CI hang on Linux shards 0/2 (CI + Mutation reproof still in_progress), and lang-spawn-live graded by reading only.

Cotal merge validation added 3 commits September 6, 2026 05:48
A source-checkout attach without COTAL_SKIP_CONNECTOR_SEED now refuses
the operator-global seed store instead of writing payloads, so the
seed-skip mutant stayed WRONG-RED on a green named cell.
Resolve the four-file conflict set against 74f7eeb:
- regenerate the docs bundle so it stamps 0.46.0
- manager cluster document revision 13 for optional waitForExit
- recount the presence-render census and keep the M8 mutation at -1
Regenerate the docs bundle against d6a61b5.
Contract revision and presence-render census are unchanged.
@davidfarah2003

Copy link
Copy Markdown
Contributor Author

BLOCK on 7b88fb3bfe82b3642037d885df03fc8b50573f37.

packages/workspace/smoke/fixtures/presence-render-sinks.json still gives the three new down.ts census entries the identical five-way disjunction, "status routing, process/HTTP state, lookup definition, styling, or data shaping." It does not identify which reason applies to any entry. The suite at packages/workspace/smoke/presence-render-census.smoke.ts:313 only checks rationale.trim().length >= 12, so this durable review record is satisfiable without documenting the classification. Replace these with entry-specific reasons. From source, the seat-list string is a manager ps error, the per-seat string is a stop-command failure, and bits.join formats a leftover/would-reap seat inventory row whose optional status is manager process state, not observed work progress.

The three previously unreviewed-file fixes otherwise pass my review.

Ran:

  • The real persona smoke directly, without its build wrapper. It exited 0 with 17 checks.
  • A temporary copy with a forced throw immediately inside the actual try. Its real finally completed and the process exited 1. The worktree was clean afterward.
  • An exact-anchor script. The rewritten attach mutation find matched once.
  • A regex inclusion check. Both former write-path strings still matched, and the source-checkout refusal changed from old=false to new=true, so the forbidden set strictly grew.

Read:

  • The attach-auth suite, mutation fixture, and actual seed-writer refusal source.
  • manager-stop-reaps-agents.smoke.ts:256, which positively requires plain Manager.stop() to leave the live seat running. It would fail if default stop reaped again. Its explicit { withAgents: true } phase separately requires process death and credential removal.
  • manager-stop-spare-guard.smoke.ts. Its livePty inclusion gate is an undisclosed scanner limitation because it requires literal new Manager(...) plus a fixed spawn-shape list, but I am not adding a second block without independently demonstrating a skipped violating file.

Not run: attach-auth-root, mutation-proof, any build, any -live suite, or any stack start/stop command.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Acceptance-loop addendum for 7b88fb3bfe82b3642037d885df03fc8b50573f37: I ran the real project gate pnpm smoke:presence-render-census. It reported 315 candidates (32 honest-text, 41 presence-only-glyph/count, 10 command-ack, 232 non-render/control) and PRESENCE-RENDER-CENSUS: 17 checks passed while the three new down.ts rationales remain identical generic five-way disjunctions. This directly demonstrates the blocker: the actual acceptance gate accepts an inadequate durable rationale record. The command left the worktree clean. Broader CI acceptance was not run because the review brief forbids builds, pnpm check, mutation-proof, stack commands, and -live suites.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Whole-result evidence audit, still bound to live clean HEAD 7b88fb3bfe82b3642037d885df03fc8b50573f37:

  1. Persona both directions: the actual smoke public integration path (MeshAgent tool specs to manager service to PTY runtime on an ephemeral authenticated broker) ran successfully with 17 checks and exit 0. A copied-source forced-throw probe exercised the actual try/finally/process.exit(code) control and exited 1. The failure direction is synthetic because intentionally breaking the tracked tree or running mutation-proof was prohibited.
  2. Attach control: exact fixture anchor cardinality ran against the real fixture/source and returned 1. Regex set inclusion retained both former forbidden outputs while newly matching the actual source-checkout refusal. The full attach integration suite was not run because its registered command includes forbidden builds. Mutation-proof was explicitly prohibited.
  3. Census finding: the real public project command pnpm smoke:presence-render-census ran and passed 17 checks with the exact 315/32/41/10/232 counts while the three generic rationales remained. This is acceptance-aligned proof of the block.
  4. Central stop contract: read-only because its suite starts a delivery daemon, broker, managers, and PTY seats. The positive cell at line 256 would fail if plain stop reaped. The explicit withAgents phase separately requires a dead process and removed credentials. Running it was outside the brief safety constraints.
  5. No -live suite, build, install, mutation-proof, pnpm check, cotal up, or cotal down was run. These are named constraints, not inferred greens.

Result: BLOCK is unchanged. The three target-file fixes have the strongest safe evidence available, and the census defect is reproduced through its real acceptance command.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Cold review — adversarial coverage-integrity lens. Verdict: BLOCK at 7b88fb3bfe82b3642037d885df03fc8b50573f37.

Graded SHA verified: git rev-parse HEAD == gh pr view 1301 --json headRefOid == 7b88fb3bfe82b3642037d885df03fc8b50573f37. The refold from the prior head carries no first-party change; its diff is only #1315/#1316 main content plus the regenerated docs bundle, and none of the three focus files appear in it.

Blocking finding

Undocumented livePty inclusion gate in bin/smoke/manager-stop-spare-guard.smoke.ts silently shrinks coverage.
I replicated the shipped gate as a local Node probe over the tree: 619 smoke files walked, 74 contain new Manager(, but only 26 pass livePty() and are examined. ~48 files that construct new Manager(...) with a live PTY runtime are skipped silently because their spawn call is not one of the seven enumerated shapes. Several of the skipped files are not fakes and carry the exact banned spare-.stop() form the guard exists to catch (for example manager-deregister.smoke.ts, which does real new Manager({... runtime:"pty"}) + .start() + plain .stop()). The planted control sits downstream of this filter — it passes livePty only because the fixture is shaped to — so it proves the detector live for one shape and inherits the blind spot. The guard therefore prints green with a healthy-looking 26-file count while its coverage silently erodes; an all-clear indistinguishable from no-data is not a check.

The receiver-name boundary in the same file is a fair contrast: it is documented in-file and measurement confirms it, so it is not a defect. The difference between a declared boundary and a silent filter is the whole finding.

Repair (no census/validator/count change): either drop the spawn-shape prefilter so any file with new Manager( + a live PTY runtime is scanned, or document the shape boundary in-file the way the receiver-name list is documented. A disclosed boundary would clear this block.

Items verified clean

  • Census / M8 anchor: In packages/workspace/smoke/fixtures/presence-render-sinks.json the anchor "total": 315 occurs exactly once and the mutant 314 zero times. The entries array length is 315 and the breakdown 32+41+10+232=315 matches. presence-render-census.smoke.ts recounts via a live AST scan (actual.total = candidates.length) and deep-equals the manifest, so anchor and recount agree and the mutation would fire a real red.
  • Central hazard / auto-merged manager.ts claim survival (read-only): on this SHA the withAgents? gate is present; stop() reaps only when withAgents === true (a plain Manager.stop() does not reap); the manager throws rather than degrades when a runtime cannot prove child exit and when a seat reports exit but is still running; teardown awaits and throws "could not prove every seat exited"; down.ts sends waitForExit: true under a bounded timeout. The control cell "a plain Manager.stop() leaves the live managed seat running" positively requires the seat still alive, so it would fail if default stop reaped again, and no spare-vs-reap cell went vacuous. The behaviour the PR exists to add is intact.

Method / RAN vs READ

  • RAN: the livePty-gate replication probe and the census anchor/recount probe (local Node only, no stack, no broker).
  • READ (not run): manager.ts, down.ts, the stop/spare contract cells, and lang-spawn-live.smoke.ts (name ends -live).
  • Not run, by constraint: mutation-proof, any build, pnpm check, any -live suite, the stop/persona/attach integration suites, and any stack start/stop.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Numeric correction to my BLOCK, reproduced first-hand and accepted. My earlier "~48 skipped" figure was 74 minus 26 — the count of skipped files containing new Manager(, not the count that also carry a live PTY runtime and are not kind:"fake". Re-sweeping with the shipped livePty/SPARE_STOP predicates extracted from the guard at 7b88fb3: the qualified skipped-live-PTY set is ~32 files, of which 29 carry the exact banned spare-.stop() form the guard exists to catch. That is the number to quote — 29 live instances the guard cannot see, because livePty throws each file out before the SPARE_STOP regex (which would match their receivers) ever runs. The receiver-name list is disclosed and correct; the livePty gate is the undisclosed part that hides these. Verdict unchanged: BLOCK at 7b88fb3. Repair accepted as DOCUMENT-not-widen, on the condition the in-file comment carries the measured magnitude (literal new Manager( + one of seven spawn shapes; admits 26 of 74; skips ~32 live-PTY files of which 29 carry the banned form); a vague caveat would not clear the block, a measured disclosure does. The widening and the per-file leak question for those 29 belong in a follow-up issue, same #964 leftover-PTY-leak class.

…s census reasons

Keep the spawn-shape gate but print every skipped live-PTY spare-stop, pin manager-deregister as the real-file control, and floor the examined set so a later shrink reds. Two already-examined teardowns from main pass withAgents so the visible hits check is green. Census totals and the M8 anchor are unchanged.
Cotal merge validation added 2 commits September 6, 2026 09:24
Fold 5e635a2 (#1329 mutation reproof gate fixtures) so main is an ancestor.
A new spawn-shape-skipped live-PTY spare-stop now reds instead of printing and exiting green. A stale frozen path reds so the 29-file baseline can only shrink.
@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Cold review — adversarial coverage-integrity lens. Verdict: APPROVE at d614f8e4251882c07ff6634865860197ad1b47ac. My prior BLOCK on the livePty gate is discharged. Graded this head fresh (verified git rev-parse HEAD == gh pr view 1301 --json headRefOid); no prior verdict carried.

The livePty coverage-integrity block is resolved

The defect I named last round was silence, not narrowness: the guard printed a healthy examined-file count while a new live-PTY spare-stop would pass unseen. This head keeps the spawn-shape gate but converts the skipped set into a frozen, gated inventory (FROZEN_DROPPED, exactly 29 entries), so the silence is fixed at the mechanism.

I confirmed the ratchet gates rather than merely reports — the exact trap from the previous fix — by running the guard and then proving both gates fire by hand (my own edits, tree restored clean afterward):

  • Ran the guard suite directly (it reads smoke sources off disk; no stack, build, or -live): green, 18 passed, 26 examined of 74, manager-deregister named as a dropped-with-candidate exclusion and not hidden on SPARE_COVERAGE.
  • N1 — added a spare manager.stop() to persona-show-auth.smoke.ts (a currently-clean skipped live-PTY file, no prior stop, not on the frozen list). The suite went red on the exact cell "a new dropped-with-candidate-stop path is not on the frozen smoke: spare-stop guard skips 29 live-PTY teardowns by spawn shape #1343 inventory", naming the file. That is the previously-silent hole, now loud.
  • N2 — repaired persona-announce's spare stop to { withAgents: true } while leaving its frozen row. The suite went red on "frozen smoke: spare-stop guard skips 29 live-PTY teardowns by spawn shape #1343 dropped inventory has no stale paths", so a repaired file cannot linger on the inventory and the baseline can only shrink.

Both mutation-fixture entries name these exact cells with a first-red target. The in-file comment states the 29 files are "not SPARE_COVERAGE and are not safe", owned by #1343, with audit/admit/repair instructions. That is the magnitude-carrying disclosure I required, made into a check rather than a reassuring sentence. The receiver-name boundary remains documented and correct.

Item C is fixed at source, not by allowlist: SPARE_COVERAGE is unchanged at its original four entries and the two live-teardown fixes use { withAgents: true }; neither filename appears anywhere in the guard.

Invariants confirmed intact at this head

  • Census: entries array length 315, breakdown 32/41/10/232, anchor "total": 315 cardinality 1 (mutant 314 absent).
  • manager.ts withAgents gate present (a plain Manager.stop() does not reap); the control cell manager-stop-reaps-agents.smoke.ts:256 is non-vacuous.

Method / RAN vs READ

  • RAN: the guard suite and the N1/N2 hand-mutations (all local file reads; no stack or broker).
  • READ: manager.ts, down.ts, the census fixture, the mutation fixture, the contract.
  • Not run, by constraint: the mutation-proof harness, any build, pnpm check, any -live suite, the integration stop/persona/attach suites, and cotal up/down.
  • CI at this head was queued/pending; I did not verify it and do not treat it as green.

Net: the block is cleared. Verdict is APPROVE at d614f8e4251882c07ff6634865860197ad1b47ac, contingent only on CI landing green (shard-2 guard result and the Mutation reproof), which is the merge gate's to confirm.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

APPROVE d614f8e4251882c07ff6634865860197ad1b47ac, with CI still pending and therefore not asserted green.

My rationale BLOCK is cleared. The three PR-added down.ts entries now give distinct reasons: manager ps control-path failure, aggregated per-seat lifecycle-stop failure, and leftover/would-reap inventory formatting with manager-reported process status. The other seven generic down.ts rationales are byte-for-byte present on current main 5e635a22c and are outside this repair/PR addition. Census remains 315/32/41/10/232, validator unchanged, and the M8 "total": 315 find anchor occurs once.

I also independently graded the repaired livePty ratchet.

Ran:

  • Real pnpm smoke:manager-stop-spare-guard: 18/18 passed, examined 26/74, printed all 29 dropped-with-candidate-stop rows, and named manager-deregister.
  • Source-derived inventory probe: FROZEN=29, current dropped set=29, unique=29, with no new or stale paths.
  • Isolated scratch mutations without modifying the worktree: N1 exited 1 first-red on a new dropped-with-candidate-stop path...; N2 exited 1 first-red on frozen #1343 dropped inventory has no stale paths.
  • Isolated floor probe: raising EXAMINED_FLOOR from 26 to 27 exited 1 on the measured-floor cell.
  • Mutation fixture cardinality and anchors: 2 selected fixtures; both find anchors resolve once.

Read/static:

  • SPARE_COVERAGE remains four entries and manager-deregister is not hidden there.
  • Contract revision 13, manager stop's opts?.withAgents === true gate, the positive plain-stop survival control at line 256, and bundle version 0.46.0 remain.
  • Both previously targeted smoke teardowns use { withAgents: true }; neither filename appears in the guard.
  • test(manager,connector-core): make the mutation reproof gate grade what it claims #1329's fixture-fix merge and named files are present by ancestry.

Not run: mutation-proof, builds, install, pnpm check, any -live suite, cotal up/down, or stack-starting integration suites.

CI observed, not graded green: docs passes; Analyze, Windows, smoke shards including shard 2, unit, live, and changed are pending. The merge gate must still require shard-2 guard output and changed's mutation selection/fatal verdicts to complete within 145 minutes.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

APPROVE at d614f8e4251882c07ff6634865860197ad1b47ac.

Both prior blocks are cleared.

Ran

  • pnpm smoke:manager-stop-spare-guard: 18 passed, 0 failed. It reported 26 of 74 new Manager( files examined and explicitly named 29 dropped live-PTY candidate-stop files.
  • An independent in-memory execution of the shipped predicates over the real source population and the two fixture mutations. Baseline: frozen 29 equals dropped 29, with no new or stale paths. N1 adds persona-show-auth.smoke.ts to newDropped, making a new dropped-with-candidate-stop path... false. N2 makes persona-announce.smoke.ts stale, making frozen #1343 dropped inventory has no stale paths false. This confirms the ratchet gates rather than merely prints.
  • The manager-deregister control in both directions. Under the old livePty-only criterion it is false; under the repaired examined-or-named criterion it is true, and the file carries five banned stops.
  • Sentinel direction checks: shrinking examined 26 to 25 or new-Manager population 74 to 73 makes the corresponding check false.
  • Mutation fixture checks: 2 mutations selected structurally, each find anchor resolves exactly once, each replacement is absent, and the reproof corpus explicitly discovers *.mutations.json.

Read and checked

  • The three PR-added down.ts rationales are now entry-specific: manager ps failure, aggregated per-seat stop failure, and leftover/would-reap inventory formatting. The seven other generic down.ts entries are present on current main and outside this repair delta, so I do not extend this PR's block to them.
  • Census expected values remain 315/32/41/10/232. The configured M8 find anchor resolves exactly once and its replacement is absent.
  • The final post-fold repair delta is only the guard plus its mutation fixture. The named test(manager,connector-core): make the mutation reproof gate grade what it claims #1329 fixture files and ancestry are present.

Not run
Mutation-proof, builds, pnpm check, any -live suite, stack-starting suites, cotal up, or cotal down.

CI
Docs and Windows soak are green. Shard 2, changed, and the remaining exact-head jobs are pending, so they are UNMEASURED rather than green.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant