[yixuan-ag2]

READY — activation PR, the last of the sutando voice train. Part of the voice reliability & onboarding plan (desktop repo: docs/impl-plan-voice-reliability-and-onboarding.md, PR group E). Its parents #2690 + #2691 are merged; this branch is rebased onto main (plus the update-branch merge), so the diff is exactly the activation change (7 files, +281/−12: pin, doubly-gated marker + acquisition-token binding, probe acceptance test, gate-branch tests).

What changed, and why?

The activation switch for probe isolation, in one atomic change:

1. bodhi pin bump: bodhi-realtime-agent → sonichi main 25cc6e8 (the post-Fix phone greeting hallucination #27 merge SHA, dist-rebuilt). This brings the probe/verify/takeover connection roles server-side: a ?probe=1 upgrade is intercepted before client attachment and can never steal a live call; ?takeover=1/?verify=1 get their 4409/4410/4411 semantics.
2. Capability marker: voice-agent publishes state/voice-agent.capabilities.json {"probeIsolation": true, "at": …, "pid": <publisher>, "lockId": "vl1-<uuid4>"} once at startup (atomic temp+rename, failure-silent, same single-writer pattern as the lifecycle snapshot), doubly gated: on the same probeState feature-detect that wires the capability (a mis-resolved bodhi — failed/cached install, hand-edited pin — never gets an advertising marker) and on having the acquisition token (no token → no publication; lockId is a required parameter, so this repo cannot emit an unbound marker — fail closed at the writer, not in another repository). Both dormant branches log. This is the flag the desktop supervisor gates its ws-upgrade probe battery, exit-7 holder probes, and single-shot verification probes on.

Pin and marker land together on purpose: the marker is the activation switch, so it must never precede the capability. Until this PR merges (and the desktop pin picks it up), every desktop probe path stays dormant/L1 exactly as shipped in phase 1.

Downgrade defense (review findings, fixed in 06e954a8 + 0011ff60): the marker file is a one-way latch — older builds neither write nor delete it — so a bare true would keep the desktop reader armed after an engine rollback below the capable pin. And pid alone is not stale-proof: OS pids are reused, so a later run can land on the stale marker's pid. The marker therefore binds to its acquisition: acquireVoiceLock surfaces the structured lock's per-acquisition lockId (vl1-<uuid4>, the token voice-lock.py already writes — the same defense class its release path uses via startTimeMs, with exact string equality instead of clock tolerance), and the marker records it alongside pid. Publication runs strictly after the lock win (a loser exits 7 first).

The paired desktop reader (the other half, reviewable there): ag2-space/ag2space-cinny-desktop#293 — 39b25ac (pid binding) + 6e67ca8 (lockId binding) in engine/backend-supervisor.mjs (voiceProbeIsolationSupported). It requires marker.pid and marker.lockId to equal the current structured lock holder's, plus holder liveness, and fails closed on anything else (marker without token, mismatch, dead holder, legacy/unreadable lock). Since this repo now skips publication when the token is missing, the reader's token requirement is a second fence — it still guards marker files left behind by older builds. That PR pins this branch as its engine/sutando submodule, so marker and reader ship as one atomic pair.

What files / sections should reviewers look at first?

• package.json / package-lock.json — the pin.
• src/voice-agent-state.ts publishCapabilitiesMarker (+ path helper) and its startup call in src/voice-agent.ts.
• src/voice-lock.ts acquireVoiceLock — surfaces the acquisition lockId the marker binds to.
• tests/agent-state-protocol.test.ts — marker shape (strict, token-bound + unbound contract), atomicity, failure-silence; tests/voice-lock-acquire.test.ts — token surfacing.

Feature/documentation consistency

N/A — the capability marker + probe gating are specified in the desktop repo's design/impl plan (group E); no sutando docs/ canonical document covers the supervisor probe contract.

What tests did you run?

Re-run at ccf6aec5 (on top of current main incl. the update-branch merge): tests/agent-state-protocol.test.ts + tests/web-voice-transport.test.ts + tests/voice-lock-acquire.test.ts 96/96 — including the group-E acceptance test: a ?probe=1 upgrade against a spawned agent with a real client attached gets exactly one agent.state frame + close(1000), the client's socket stays OPEN and the lifecycle still counts it attached, and the published marker is verified BOUND in vivo (marker.pid/marker.lockId equal the structured lock file's — the gate's TRUE branch). The gate's FALSE branch is proven on the real caller too: a spawned agent with the SUTANDO_TEST_MODE-only detect-override seam reaches its first frame (wiring init ran past the marker site) and publishes no marker, with the dormant-branch log asserted — deleting the gate now fails the suite loudly. Marker unit rows assert the token-bound shape strictly; the unbound shape is unrepresentable (lockId required). npx tsc --noEmit clean; npm run build:bundle OK; npm ci validates the lock; installed bodhi dist confirmed to carry the role code (4409/probe markers present). The dist-rebuilt bodhi pin was E2E-verified in the desktop packaged-app pass (busy/takeover live, evidence on #2690/#27). package-lock.json was rebuilt from main + the pin change only, so its diff is exactly spec/resolved/integrity (an earlier regen had dropped six unrelated "peer": true flags — npm-version churn, now reverted).

Stacked PR?

No longer — parents merged. #2684/#2689/#2690/#2691 were squash-merged, so the original branch's diff did not auto-collapse; the branch was rebased (the two group-E commits cherry-picked onto current main, content byte-identical — the transport blob on this branch matches main's exactly). Readiness checklist (all satisfied):

• feat(transport): single-client isolation — probe/verify/takeover connection roles in ClientTransport bodhi_realtime_agent#27 merged → pin repointed to the post-merge sonichi main SHA (25cc6e8), lock regenerated
• feat(voice-transport): fail-fast client hardening + canonical transport consolidation (voice plan WS1 Steps 15–18) #2690 + feat(credentials): S1 voice-source truth table — voicePreference + logout quarantine across every consumer (voice plan WS2 Steps 3–6) #2691 merged
• re-run: agent-state + transport suites, tsc, build:bundle

Hardcoded-path check

No absolute paths in added lines; test fixtures use mkdtemp.

🤖 Generated with Claude Code

[yixuan-ag2]

Un-draft checklist progress: bodhi #27 merged (merge commit) — the pin now points at the canonical sonichi main SHA 25cc6e8 with the lock regenerated and the installed dist re-verified to carry the role code (5de70d6e). Remaining before un-draft: #2690 + #2691 merge (this branch contains both; its diff then collapses to the pin + capability marker).

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[yixuan-ag2]

Rebased onto current main and ready for review. The parent PRs (#2684/#2689/#2690/#2691) were squash-merged, so this branch's old merge-base kept re-showing the whole train in the diff. Fixed by cherry-picking the two group-E commits onto current main (force-pushed as d02bc70f). Verification on the rebased head: diff is exactly the activation change (5 files, +68/−9); npm ci validates the lock; agent-state + transport suites 92/92; tsc --noEmit clean; build:bundle OK; installed bodhi dist confirmed to carry the probe/4409 role code. Content is byte-identical to the pre-rebase branch (transport blob matches main's exactly).

[yixuan-ag2]

Review verified and addressed in 06e954a8 (regular push, no force):

One-way latch → confirmed real, fixed in both halves. Verified against the code: publishCapabilitiesMarker only ever wrote {probeIsolation: true, at}, nothing deletes or invalidates it, and the desktop reader (voiceProbeIsolationSupported) accepted bare probeIsolation === true — so a stale marker after an engine rollback would keep the probe battery armed against a non-isolating build, exactly as described.

Fix shape (pid binding rather than recording the pin SHA — the reader has no way to judge which SHAs are capable, but it does know who currently holds the agent's PID lock):

• Here: the marker records the publisher's pid. Publication runs strictly after acquirePidLock() — a lock loser exits 7 before reaching it — so marker.pid equals the live lock holder's pid exactly when the marker describes the running build.
• Desktop half (paired via the group-H activation PR, ag2-space/ag2space-cinny-desktop#293, commit 39b25ac): the reader now requires marker.pid === current lock holder's pid and holder liveness; the exit-7 grace path threads its already-validated holder pid. Marker without pid, pid mismatch, dead holder, or unreadable lock all fail closed (dormant).

All four pairing quadrants fail safe: old reader+old engine (no marker) → dormant; new reader+old engine → dormant; old reader+new engine → armed against a capable build; new reader+new engine → armed via pid match. The rollback case (stale marker, rolled-back engine) → pid mismatch → dormant. New negative-path test rows on the desktop side pin each of these.

Lockfile nit → confirmed, eliminated rather than re-described. The lock is now rebuilt from main + the pin change only; its diff is exactly spec/resolved/integrity (+3/−2). The six dropped "peer": true flags (npm-version churn from the regen) are restored, and npm ci validates the result.

Re-run at 06e954a8: suites 92/92, tsc clean, build:bundle OK. Body updated to match (marker shape, accurate diff description).

[yixuan-ag2]

Verified and addressed in 0011ff60 (fast-forward push on top of the update-branch merge):

Confirmed — pid equality alone is not stale-proof. OS pids are reused, so after a rollback to a non-isolating build a later run can land on the stale marker's pid, and the reader's pid check would incorrectly arm the probe battery.

Fix — bind to the acquisition, not the process id. The structured lock already carries the right unique startup token: lockId (vl1-<uuid4>, unique per acquisition — the same defense class voice-lock.py's release path uses via startTimeMs, but with exact string equality instead of clock-tolerance semantics; the acquire helper already emits the full record on success, it was just being discarded):

• acquireVoiceLock now surfaces lockId from the helper's {"ok": true, "lock": record} output (best-effort: unparseable → acquisition still succeeds, marker just stays unbound).
• voice-agent threads it into the marker: {probeIsolation, at, pid, lockId}. A marker without a token never arms the reader (fail closed).
• The paired desktop reader (ag2-space/ag2space-cinny-desktop#293, 6e67ca8) requires marker.pid and marker.lockId to equal the current structured lock holder's (legacy locks carry no token → fail closed), plus holder liveness. The exit-7 path threads its validated holder's lockId alongside the pid. A dedicated PID-REUSE test row pins the exact scenario: pid matches the live holder, token differs → dormant.

Why lockId over marker-side startTimeMs: the reader compares against the lock file, so a lock-originated token round-trips with exact equality — a self-measured start time would reintroduce the tolerance-matching the python helper needs for ps-derived times.

Suites at 0011ff60: 95/95 (incl. new voice-lock-acquire token-surfacing rows + the unbound-marker contract row), tsc clean, build:bundle OK, npm ci validates the lock. Body updated (marker shape, diff description).

[john-the-dev]

Reviewed at head 0011ff60 (Rui asked me + @sutando-qingyun-001 to look). One blocker: CI is red.

tsc + tests (clean install) fails — 846/847 pass, and the one failure is this PR's own newly-added integration subtest (tests/agent-state-protocol.test.ts, modified +40/-0):

not ok - agent.state emission (integration, spawned agent) → subtest immediate frame on connect; transition frame on injected terminal close; lifecycle file tracks it
error: 'timed out waiting for lifecycle snapshot upstream=failed'

The spawned-agent lifecycle snapshot never reaches upstream=failed within the timeout. Please confirm whether that's a real gap in the probe-isolation/terminal-close path (the actual thing this PR changes) or just a too-tight timeout / CI-flaky spawn — and push a green run either way. A required check must be green before this can be approved.

Also: the branch is BEHIND main — update-branch after the fix so it's re-tested against current main (note the src-map-stale trap if the merge touches generated files).

Happy to re-review as soon as CI is green. The change is scoped (voice-agent-state / voice-lock + tests) and I'll do a full read + local test run on the green head.

[yixuan-ag2]

All three points addressed at fa996a2f:

1. Desktop reader — the other half exists and is paired. Fair flag (the "strict probeIsolation === true" line was the original body; it was updated in round 1). The desktop gate (ag2-space/ag2space-cinny-desktop#293, commits 39b25ac pid + 6e67ca8 lockId) now reads, in engine/backend-supervisor.mjs:

const doc = JSON.parse(fs.readFileSync(p, 'utf8'));
if (!doc || doc.probeIsolation !== true || !Number.isInteger(doc.pid)) return false;
if (typeof doc.lockId !== 'string' || !doc.lockId) return false;
let holderPid = lockPid, holderLockId = lockId;          // exit-7 threads its validated holder
if (!Number.isInteger(holderPid) || typeof holderLockId !== 'string' || !holderLockId) {
  const lock = readLock(ws);                              // voice-lock.py read
  if (!lock || lock.kind !== 'structured') return false;  // legacy: no token to bind
  holderPid = lock.pid; holderLockId = lock.lockId;
}
if (!Number.isInteger(holderPid) || holderPid !== doc.pid) return false;
if (typeof holderLockId !== 'string' || holderLockId !== doc.lockId) return false;
return pidAlive(holderPid);

Marker and reader ship as one atomic pair via that PR's engine/sutando pin; a dedicated PID-REUSE test row (pid matches the live holder, token differs → dormant) pins the exact scenario. All pairing quadrants fail closed.

2. CI red — root-caused, and it's not these commits. The diff provably doesn't touch the emit/lifecycle path (the failing wait is on the lifecycle FILE; the failed/quota FRAME was observed, so emitAgentState ran; publishLifecycleSnapshot is untouched). It's a pre-existing race in the test itself, latent since it landed in #2689: the fake key's own startup auth close arrives through the same wrapped onClose seam and can land after the injected quota close, legitimately overwriting the file's category (R8 makes reason/category changes within failed meaningful transitions — recordTerminalClassification → emit → publish runs for both). The file holds only the LAST state, so the 150 ms poll for quota misses forever once auth lands. Captured live under parallel load — at timeout the file read {"upstream":"failed","category":"auth"} while the quota frame sat in the frames array; reproduced 2/10 sequential, ~1/9 parallel. Fix: the file wait now asserts the real invariant — mirrors the latest failed classification — while the quota mapping stays pinned end-to-end by the FRAME assertions. 15/15 parallel runs green post-fix. Main is exposed to the same flake on every run; the fix rides here (happy to split it out to a standalone PR if you'd rather land it on main independently).

3. BEHIND — updated. origin/main (63821b79) merged in (4011422f), test fix on top (fa996a2f); checks re-running at the new head. Suites locally: 95/95, tsc clean.

[yixuan-ag2]

Re-reviewed at head fa996a2f (asked in Sutando Triage). Prior blocker is cleared; one new blocker, both non-blocking notes below.

CI (john-the-dev's blocker at 0011ff60): resolved. All 17 checks green, including the one that was failing — tsc + tests (clean install) passes in 12m12s. bash scripts/review-checks.sh --diff → PASS (hardcoded-paths clean).

The core claim holds, and I verified it in the activated path rather than from the diff. In the installed dist at this pin, dist/index.js:2088 intercepts params.get("probe") === "1" as the first branch of the upgrade handler and returns; handleProbe (:2119) sends one frame and ws.close(1000) without ever reading or writing this.client. So "a ?probe=1 upgrade can never steal a live client slot" is true end-to-end, not just asserted. Two supporting halves also check out: scripts/voice-lock.py:333 emits {"ok": True, "lock": record} with lockId: "vl1-<uuid4>" (:295), exactly the shape acquireVoiceLock parses — so the marker is bound in practice, not silently unbound; and running the literal detect expression against the pinned module gives String(VoiceSession).includes('probeState') === true, so the wiring at src/voice-agent.ts:881 actually activates.

Nice tightening worth naming: the pin moves from short ffec0cd to a full 40-char sha and package-lock.json now carries an integrity hash for bodhi. That closes a real supply-chain gap.

---

Blocker: the behavior this PR activates has no test in this repo

tests/agent-state-protocol.test.ts:26 still reads:

NOTE (Step 11 seam): the pinned bodhi has no ?probe=1 interception yet — probeState is passed behind a feature-detect in voice-agent.ts and the probe-frame + live-call acceptance tests activate with the bodhi pin bump (impl plan PR group E).

This PR is group E and is the pin bump, so that note is now false — and the tests it schedules for this PR aren't here. The diff adds marker-shape, unbound-marker, failure-silence, and lockId-surfacing tests (all good), but nothing opens a ?probe=1 upgrade while a real client is attached and asserts the real client keeps its slot. That is the failure mode the whole change exists to prevent, and REVIEW.md criterion 3 is explicit that happy-path green isn't proof.

I confirmed the mechanism by hand (above), so I am not claiming it's broken — I'm saying the suite doesn't hold it. The spawned-agent harness in this same file already stands up a real agent, so the test looks cheap. To clear: add that acceptance test, or say the plan moved it and update the stale note either way.

Non-blocking: the activation marker isn't gated on the capability it advertises

publishCapabilitiesMarker at src/voice-agent.ts:962 is unconditional, while bodhiSupportsProbeState is computed at :858 — same function, ~100 lines earlier, in scope. The docblock at src/voice-agent-state.ts:191 states the invariant plainly:

the marker is the activation switch, so it must never precede the capability

Right now that's asserted by shipping-discipline (pin and code travel together) rather than enforced by the code, even though the signal is sitting right there. It's correct today. It stops being correct the moment npm resolves a bodhi other than the lockfile's — failed or cached install, or a hand-edited package.json — and then the marker advertises probeIsolation: true to a build that will route probes down the normal attach path.

Worst-case disruption (criterion 5), named: a probe against a non-isolating build lands in handleUpgrade's role logic and can preempt or reject the live client — i.e. voice drops mid-conversation on a machine whose desktop supervisor is probing on a timer. Mitigation is one line: if (bodhiSupportsProbeState) publishCapabilitiesMarker(...). Cheap enough that I'd take it now rather than rely on the invariant staying true.

Non-blocking: unverifiable-from-here, and worth stating rather than assuming

The consumer half is not in this repo. grep -rn "probeIsolation" across the tree returns only this writer and its tests; src/Sutando/main.swift does not read voice-agent.capabilities.json. So the safety argument in the docblock — the reader "requires BOTH equalities plus holder liveness and fails closed otherwise" — can't be checked from this PR. Given the marker is a one-way latch with no deleter (nothing unlinks it on exit, which I did confirm), that reader is the entire stale-marker defense after a rollback. Please link the reader commit so the pairing is reviewable; the lockId design reads sound to me, I just can't confirm the other side enforces it.

Nit: a test relaxation is riding along

The lifecycle assertion moves from pinning category === 'quota' to mirroring the latest failed frame. The reasoning in the comment is sound and the new assertion is guarded (!!lastFailed), so it isn't vacuous — but it's a flake fix bundled into an activation PR, which CONTRIBUTING's single-concern rule would rather see as its own commit.

---

Verdict: changes requested — one blocker (no test exercises probe-vs-live-client), and I'd fold in the one-line marker gate while touching it. Everything else I checked passes, and the prior CI blocker is genuinely resolved.

Note on form: I'm authenticated as the PR author's own account, so GitHub won't let me file this as a formal REQUEST_CHANGES — this is a COMMENT review and must not be counted toward the two required approvals.

[yixuan-ag2]

Round-4 review addressed at d81f714e (two single-concern commits; thanks for the dist-level verification — handleProbe never touching this.client and the emitted lock record shape are exactly the load-bearing facts):

Blocker → cleared: the promised acceptance test now exists and passes (a505c121). Against a spawned agent with a real client attached, a ?probe=1 upgrade gets exactly one agent.state frame + close(1000); the client's socket stays OPEN and voice-lifecycle.json still counts it attached 500 ms after the probe closes — the steal path asserted end-to-end, offline, using the same harness. The stale Step-11 note is rewritten to say the test is ACTIVE here. Bonus teeth: the same spawn verifies the marker in vivo — marker.pid/marker.lockId equal the structured lock file's, so the acquisition binding is proven against a real agent, not just unit fixtures.

Marker gate → taken as suggested (d81f714e): if (bodhiSupportsProbeState) publishCapabilitiesMarker(...) — the same feature-detect that wires probeState, so a mis-resolved bodhi (failed/cached install, hand-edited pin) never gets an advertising marker; the dormant branch logs. The voice-agent-state.ts docblock now says the invariant is enforced by the caller, not by shipping discipline.

Reader links → in the body now: ag2-space/ag2space-cinny-desktop#293, commits 39b25ac (pid) + 6e67ca8 (lockId) — voiceProbeIsolationSupported in engine/backend-supervisor.mjs requires both equalities against the current structured lock plus holder liveness, fails closed otherwise, and that PR pins this branch as its engine/sutando submodule (repinned to d81f714e just now), so the pair ships atomically.

Single-concern nit: the lifecycle-assertion flake fix is already its own commit (fa996a2f, test-only) rather than folded into the activation commits; the offer to lift it into a standalone main PR stands if you'd rather take it independently.

Suites at d81f714e: 96/96 (agent-state 24 incl. the new acceptance row, transport 70, lock-acquire 2), tsc clean, build:bundle OK.

[john-the-dev]

APPROVE — reviewed at head d81f714e (Rui asked me to review). My earlier blocker (the agent.state emission integration subtest timing out on upstream=failed) is resolved: the fix gates capability-marker publication on the bodhiSupportsProbeState feature-detect, and CI is now green at this head (all checks incl tsc + tests (clean install) — 0 failures; I relied on that hermetic clean-install run for execution).

The design is sound and security-minded:

• Feature-detect gate: the marker ({probeIsolation:true}) is published only when the running bodhi actually ships probe/verify/takeover roles — a build without probe isolation (npm resolved a different bodhi, cached install, hand-edited pin) never advertises the capability. Marker-follows-capability ordering is enforced, not assumed.
• PID-reuse + rollback defense: the marker binds pid + lockId (the per-acquisition vl1-<uuid4> surfaced from the structured lock). A stale true after an engine rollback can't keep arming the reader, and pid-alone isn't trusted (pids are reused). The desktop reader requires both equalities + holder liveness and fails closed otherwise.
• Safe I/O: atomic temp+rename, failure-silent — a marker write can never take the voice path down (probes just stay dormant); an unparseable token leaves the marker unbound → reader fails closed.

Tests cover the marker shape/atomicity, the unbound (no-token) path, failure-silence, lockId surfacing from voice-lock, and the probe-frame + live-call acceptance (probe upgrade against a spawned agent with a real client attached → one frame + clean close, client keeps its slot). bodhi pin bump and the activation marker land together on purpose. Clean, well-tested activation switch.

[yixuan-ag2]

Re-reviewed the delta at d81f714e (fa996a2f..d81f714e, two commits). My blocker is cleared. Both points addressed, and I checked the substance rather than the commit messages.

Blocker — probe-vs-live-client acceptance test: genuinely closed. a505c121 adds a test that exercises the actual failure mode rather than a happy path: it attaches a real client, confirms it holds the slot, fires a ?probe=1 upgrade, then asserts the probe got close(1000) plus one agent.state frame and that the live client's socket is still OPEN with voice-lifecycle.json still reporting clientAttached: true. A steal would have dropped that socket and flipped the lifecycle — so the assertion is on the negative that matters. It also pins the marker end-to-end (marker.pid/marker.lockId equal the structured lock's), which is the desktop reader's stale-marker contract.

Green isn't enough on its own, so I checked the test actually executes rather than being skipped — in the tsc + tests (clean install) log for d81f714e:

ok 3 - ?probe=1 gets one frame + close(1000) and never steals the attached client
# pass 848   # fail 0   # skipped 0

17/17 checks SUCCESS. The stale header note at the top of that file is also correctly rewritten — it no longer claims the pinned bodhi lacks ?probe=1 interception.

Marker gate: done, and better than what I suggested. d81f714e wraps publication in if (bodhiSupportsProbeState), so the invariant in the docblock ("the marker must never precede the capability") is now enforced by the code rather than asserted by shipping discipline. The else branch logging capability marker NOT published (probes stay dormant) is a genuine improvement on my suggestion — it makes the dormant state observable instead of silent, which matters because a missing marker and a marker nobody reads look identical from the outside.

Two small things, neither blocking:

1. The gate's dormant branch has no test. The unit tests cover publishCapabilitiesMarker directly and the new acceptance test covers the published path, but nothing asserts that an incapable bodhi withholds the marker — and that withholding is the new safety property. Stubbing the detect is awkward, so I'd accept this as-is; noting it so it's a decision rather than an oversight.
2. Still unanswered from my last pass: the reader half isn't in this repo (grep -rn "probeIsolation" finds only the writer and its tests; main.swift doesn't read the marker). With no deleter for a one-way latch, that reader is the entire stale-marker defense after a rollback. A link to the reader commit would make the pairing reviewable. Also unchanged: the lifecycle-assertion relaxation is still a flake fix riding along in an activation PR.

Verdict: my changes-requested is withdrawn — this is merge-ready from my side. Everything I flagged is either fixed or explicitly parked, CI is fully green, and the behavior the PR activates is now held by a test that reproduces the failure it prevents.

Same caveat as before: I'm authenticated as the PR author's own account, so GitHub won't let me file a formal APPROVE — this is a COMMENT and must not be counted toward the two required approvals.

[yixuan-ag2]

Round-5 addressed at ccf6aec5 — both points taken, and the design question changed the contract for the better:

Design question → adopted: no unbound markers, ever. You're right that it read as an oversight — there is no consumer that benefits from an unbound marker, and the defense shouldn't live only across a repo boundary. lockId is now a required parameter of publishCapabilitiesMarker (the unbound shape is unrepresentable in this repo), and the caller skips publication entirely when the acquisition token is missing, with its own log line. Fail closed at the writer; a marker on disk is always fully bound. The desktop reader's token requirement remains as a second fence — it still guards files left behind by older builds after a rollback.

Gap → closed: both gate branches now regress loudly, on the real caller. New integration row: spawn the agent with a SUTANDO_TEST_MODE-only detect-override seam (SUTANDO_TEST_FORCE_NO_PROBE_STATE=1, same pattern as the injected-close seam), wait for its first agent.state frame — which proves the wiring init ran past the marker site — then assert voice-agent.capabilities.json does not exist and the dormant-branch log line appeared. Your exact regression probe ("delete the if (bodhiSupportsProbeState) guard") now fails the suite: the marker file materializes and the log assertion trips. The TRUE branch stays pinned by the probe acceptance test's in-vivo binding assertions (marker.pid/marker.lockId vs the lock file).

CI note: correct at the time — tsc + tests (clean install) was still running when you looked; it finished green at d81f714e (all 16 runs) and is re-running now at ccf6aec5.

Suites at ccf6aec5: 96/96 (agent-state 24 — the removed unbound-marker unit row is replaced by the caller-level FALSE-branch row — transport 70, lock-acquire 2), tsc clean, build:bundle OK. Desktop #293's engine pin repointed to ccf6aec5 so the pair stays atomic. Body updated (doubly-gated marker, second-fence framing, test description).

[yixuan-ag2]

Round 5 at ccf6aec5: both remaining notes closed, nothing left from my side.

Unbound markers are now unconstructable, not merely unread. Making lockId a required parameter and gating publication on bodhiSupportsProbeState && lockId moves the guarantee from "the desktop reader will reject it" to "this repo never writes one" — fail closed at the writer. That's a better answer than the one I asked for: my note was to link the reader so the pairing could be reviewed, and this removes the need to trust a boundary I can't see from here. The reader's token requirement becomes a second fence rather than the only one.

Also the right call to delete the publishes UNBOUND row and leave a comment saying why the case can no longer exist, instead of leaving a test asserting a shape the type system now forbids.

The gate's dormant branch is genuinely covered. The new integration test asserts three things, and the third is the one that makes it real: no marker on disk, the dormant log line fired, and the agent still reached a first agent.state frame — which distinguishes "reached the marker site and chose not to publish" from "died before getting there." A weaker test would conflate those and pass for the wrong reason.

I checked the seam rather than taking the comment's word for it: SUTANDO_TEST_MODE is pre-existing (7 uses in voice-agent.ts, two of them other fault-injection seams), so "same pattern as the injected-close seam" holds. Its blast radius is fail-closed by construction — forcing the detect off only withholds the marker and leaves probes dormant, so a stray env var in production degrades rather than exposes.

Verification, since green alone doesn't prove a new row executed. 17/17 SUCCESS, and in the tsc + tests (clean install) log for ccf6aec5:

ok 3 - ?probe=1 gets one frame + close(1000) and never steals the attached client
ok 4 - publishes NO capability marker when bodhi lacks probeState
# pass 848   # fail 0   # skipped 0

848 on both this head and d81f714e — consistent with one row removed and one added, so the count isn't hiding a silently-skipped test.

Verdict: merge-ready from my side, no open requests. Every point I raised across the three rounds is either fixed or explicitly parked with a reason. The only thing I never got is the reader-commit link, and the writer-side gate makes that a nice-to-have rather than a gap.

Unchanged caveat: all three of my reviews here are COMMENTs — I authenticate as the PR author's own account, so GitHub won't accept a formal APPROVE. None of them count toward the two required approvals; this still needs two maintainers.

[bassilkhilo-ag2]

Reviewed at current head ccf6aec5. This is a well-built, thoroughly-verified PR — functionally it's in good shape and I'm not re-litigating the 5 prior review rounds.

Independent verification (functional):

• npx tsc --noEmit clean.
• npx tsx --test --test-force-exit over tests/agent-state-protocol.test.ts + tests/voice-lock-acquire.test.ts + tests/web-voice-transport.test.ts: 96/96 pass.
• Confirmed publishCapabilitiesMarker's lockId param is non-optional (opts: { lockId: string; ... }) — the "no unbound markers, ever" design change from round 5 is real, not just claimed.
• Confirmed the acceptance test (tests/agent-state-protocol.test.ts:639-707) actually asserts marker.lockId === lock.lockId against a real acquisition, not just the shape.

Blocker: comment-policy, CLAUDE.md:29 ("at most 2 lines... no narration, no incident history, no references to PRs, issues, people, or other systems"). This PR adds several comment blocks far past that cap, none caught across 5 review rounds:

• src/voice-agent-state.ts:189-216 — 28-line JSDoc on publishCapabilitiesMarker, narrating the full design rationale (rollback defense, pid-reuse, review history).
• src/voice-agent.ts:963-978 — 15-line block above the marker-publish call, and it literally says (review) and (review round 5) — exactly the kind of process/discussion reference the rule excludes.
• src/voice-agent.ts:855-860 — 5-line test-seam comment.
• src/voice-agent.ts:212-215 — 3-line comment on voiceLockId = res.lockId.
• src/voice-lock.ts:118-123 — 6-line comment in acquireVoiceLock.

The reasoning in these blocks is good and worth keeping — it just belongs in the PR body (which already has it, near-verbatim, in "Downgrade defense") and the commit messages, not in the source. Suggest condensing each to the 1-2 line invariant (e.g. for the marker docstring: "Publishes the group-E capability marker the desktop supervisor gates probes on. lockId is required — an unbound marker has no consumer, so publication is skipped without one.") and moving the rest out.

No functional blocker. This is the only thing standing between this and a clean approval.

[yixuan-ag2]

Comment-policy blocker addressed at b8fe50b0 (rebased onto the latest update-branch merge; suites re-run there: 96/96, tsc clean).

Verified the rule first — CLAUDE.md line 29 says exactly what you quoted — and all five flagged blocks are condensed to ≤2-line invariants with zero review/PR/process references:

• publishCapabilitiesMarker docblock (28 → 2 lines): "Publishes the group-E capability marker the desktop supervisor gates probes on; pid+lockId bind it to this acquisition, and lockId is required — an unbound marker has no consumer."
• Marker-call gate (15 → 2): "The marker must never advertise a capability the resolved bodhi lacks, and never publish unbound (no token → no marker): a marker on disk is always real and bound."
• Detect test-seam (5 → 2), voiceLockId assignment (3 → 2), acquireVoiceLock parse (5 → 2) — the last one also dropped a claim your round-5 change had made stale ("leaves the marker unbound"; nothing publishes unbound anymore).

The displaced rationale already lives near-verbatim in the PR body's Downgrade defense section and in the four commit messages, per the rule's own destination. The pre-existing 6-line feature-detect comment above the seam is main's code, left untouched to keep the diff single-concern.

Desktop #293's engine pin repointed to b8fe50b0 in step.

[github-actions]

@cla-assistant check

[bassilkhilo-ag2]

Approving at head `c1ca1a1b` (rebased onto main, content unchanged from `b8fe50b0`).

The comment-policy fix I flagged is confirmed correct: all 5 over-cap blocks (the 28-line marker-publish JSDoc, the 15-line block that literally cited "(review)"/"(review round 5)", plus 3 shorter ones across `voice-agent.ts` and `voice-lock.ts`) are now condensed to the 2-line CLAUDE.md cap. Verified independently in a fresh clone: comment-only diff (no non-comment lines changed), no remaining review/reviewer references in added lines, 96/96 tests pass (`agent-state-protocol` + `voice-lock-acquire` + `web-voice-transport`), `tsc --noEmit` clean. All 17 hosted checks green, including `tsc + tests (clean install)` which just landed.

No open blocker remains. Functional review from my prior pass stands — this fix touched only comments.

[liususan091219]

Verdict: approve-equivalent. Formal record of my review (three passes in Discord #dev; verdict given at head fa996a2f, delta fa996a2f..c1ca1a1b re-read before submitting). Filed post-merge as a COMMENT review: an APPROVE is 👍-gated on this repo and its merge-enabling weight is moot on a merged PR — the record is what matters.

What I verified at fa996a2f:

• The CI failure (agent.state emission (integration, spawned agent) timeout) was a pre-existing race, not introduced by this PR. The old assertion pinned category === 'quota' on the lifecycle file, but the file holds only the latest failed state, and the fake key's own startup auth close can land after the injected quota close through the same wrapped onClose seam — the file then legitimately reads failed/auth. The fix asserts the file's actual invariant (latest failed classification) while the quota mapping stays pinned on the frame assertions, where ordering is preserved. Evidenced, not guessed: the file was captured reading failed/auth at timeout with the quota frame still present in the frames array. Test tightened to the real contract, not weakened.

Post-verdict delta (d81f714e, ccf6aec5, a505c121, b8fe50b0), re-read for this submission:

• Marker publication is now gated on the same probeState feature-detect that wires the capability — a mis-resolved bodhi never gets an advertising marker (d81f714e).
• lockId is now required; publication is skipped when unbound, so a marker on disk is always fully bound to its acquisition — fail closed at the writer, not in another repository (ccf6aec5), with a SUTANDO_TEST_MODE seam proving the FALSE branch against a spawned agent (a505c121 activates the acceptance test).
• b8fe50b0 is comment-cap style only.

Remaining note (cross-repo, unverifiable from this repo): whether the desktop supervisor's reader honours the pid/lockId binding or still checks only probeIsolation === true. ccf6aec5 shrinks the exposure — no unbound marker can exist any more — but reader-side confirmation lives in the desktop repo.

[liususan091219]

Formal APPROVE for the record (post-merge, at the owner's request; full evidence in my COMMENT review — three passes in #dev, verdict at head fa996a2f, delta fa996a2f..c1ca1a1b re-read). Summary: the CI timeout was a pre-existing race, not this PR — the test fix tightens the assertion to the lifecycle file's real invariant rather than weakening it; the post-verdict commits gate the capability marker on the probeState feature-detect and make lockId required (no unbound marker can exist — fail closed at the writer). Remaining cross-repo note: reader-side pid/lockId enforcement lives in the desktop repo, unverifiable from here.