Repository navigation
voice: agent.state v1 protocol emitter + lifecycle snapshot (phase 1, stacked on #2684) - #2689
Conversation
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed and verified at head ab6d75ee. The unique agent.state work (v1 protocol emitter, atomic crash-safe voice-lifecycle.json, classifier→protocol mapper, Z3 idle-restore fence) is clean and well-tested: I ran agent-state-protocol.test.ts 20/20 (incl. the cross-process torn-read race, Z3 arm/fence, and the spawned-agent integration driving a real transport close → failed/quota frame + lifecycle tracking) and voice-error-classifier.test.ts 15/15; tsc --noEmit clean. The credential path only reports an opaque credentialGeneration (never mints/derives/logs the actual key), the lifecycle write is atomic temp+rename with unique temp names, and the wire contract is additive + feature-detected behind the not-yet-landed bodhi pin.
Two process notes before merge (important):
- This is a cross-fork stack that physically includes all of #2684's commits, so it must land after #2684 — merging this first would pull the whole parent surface in one shot. (I've separately reviewed + approved #2684; its tests pass and its crash/lock/exit invariants hold.)
- Branch is BEHIND main — needs an update-branch.
Non-blocking nits: the String(VoiceSession).includes('probeState') feature-detect is a little fragile under a minified bundle (fails safe → probe wiring omitted, which is the current pin state); the SUTANDO_TEST_MODE-gated fault-injection seams live in main() (inert in prod, consistent with repo pattern). No blocking code concerns.
|
Child-layer review at head
No blockers on the child layer; with john's approval this is merge-ready after #2684 lands (merge order as the owner listed). — qingyun-sutando (001) |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed the child-only layer (pr2684...head: 7 files, +1301) plus the cumulative stack interaction, per the stacked-PR rule.
What I ran on the child head (ab6d75e, all green)
npx tsx --test tests/agent-state-protocol.test.ts tests/voice-error-classifier.test.ts
-> # pass 35 / # fail 0
# cumulative — the parent's suites still green on THIS head:
tests/voice-lock.test.py -> All voice-lock tests passed.
tests/voice-agent-kill-path-gate.test.py -> voice-agent kill-path gate: clean.
npx tsx --test tests/voice-agent-crash-only.test.ts -> # pass 16 / # fail 0
git diff pr2684...HEAD | bash scripts/review-checks.sh -> PASS (hardcoded-paths clean)
Child-layer read
- No secret on the wire or on disk: the
agent.statev1 frame carriescredentialSource+ an opaquecredentialGenerationonly — I grepped the emitter for key references: zero. The resolver change reports a Rust-minted generation verbatim and never derives anything from the secret. - Snapshot atomicity is right:
publishLifecycleSnapshotwritesstate/voice-lifecycle.jsonvia unique temp name (-tmp-<pid>-<counter>) +renameSync, explicitly diverging fromwriteVoiceState's plainwriteFileSyncbecause a cross-process consumer reads it — the comment says why, and the path comes fromstatusPath()/workspace helpers, no hardcoded paths. - Wire contract discipline: the v1 frame schema is marked verbatim-contract with a version-bump requirement — the right shape for a protocol PR.
- The isolated idle-restore fence (Z3) addresses a real leak (probe wakes upstream after the one-shot idle timer already fired → upstream connected forever) and
agent-state-protocol.test.tsexercises the fence + invalidation paths rather than just the happy path.
Same owner-actionable note as the parent: this head's full CI is also gated on a maintainer's "Approve and run" (fork PR). Merge order stands: #2684 first, then this (its branch already contains the parent; after the parent's squash lands, this branch will need the same rebuild-onto-main treatment #2686 just went through — cherry-pick the child commits, don't rebase the stale stack).
Approving the child layer on the local runs + the reads above.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Static review of the child-only layer (this diff includes #2684's commits; I reviewed #2684 separately and looked here at what 2689 adds on top). Suite not run — anything unverified is called out.
Hardcoded absolute paths: clean — zero hits across added lines.
Credential exposure — checked, and it's done right. A state frame that reports "which credential is loaded" is the obvious place to leak a secret, so I traced credentialGeneration. The resolver's contract is explicit: "This resolver only REPORTS it — never mints, never derives it from the secret", the value is an opaque cg1-<UUID> minted by the Rust host, and legacy credentials without one simply omit the field. So the frame carries an identifier that is useless to an attacker, not a fingerprint of the key. That's the right design and worth keeping that comment exactly as written.
Stacking. Per CONTRIBUTING.md the merge order needs stating on the PR: #2684 lands first, then this is rebased and its full checks re-run. Worth confirming that's the plan, since the cross-fork base means CI here is exercising parent+child together and a green run now is not evidence for the child alone after the parent moves.
Merge-readiness: no blocker found in the child layer from a static read. My reservation is inherited rather than new — I did not run this suite, and #2684's own verification (a real wedge-and-reap round trip) is what I'd want green before either lands. If a maintainer has already done that end-to-end, treat this as a second pair of eyes on the state-frame and credential surface specifically.
One cross-PR note: bodhi sonichi/bodhi_realtime_agent#27 adds a ?probe=1 role that returns a JSON state frame. If the desktop supervisor ends up using that instead of the HTTP __liveness_probe__ curl, it would close the gap I raised on #2684 — where only a curl timeout (rc 28) counts as wedged, so an agent answering a fast HTTP 500 is never reaped. A state frame can express "answering but not usable" in a way an exit code cannot. Worth deciding deliberately which probe is authoritative rather than ending up with both.
Pull request was closed
|
@yixuan-ag2 this is 2-approved + mergeable but a required check ( |
|
Merge position: Stage 2 — after #2684 (merge commit, not squash). Once #2684 lands via merge commit, this PR's diff collapses to its own commits. #2690 and #2691 both stack on this branch and follow it. 🤖 Generated with Claude Code |
|
Double-check after the GitHub outage: the prior required check |
|
Approved the fork workflow runs for the current head |
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
|
Required CI did not pass on head |
Head branch was pushed to by a user without write access
|
Addressed on head 7f67198 by merging the fixed #2684 head into this stacked branch, preserving the required merge-based history. The parent fix lets the existing SUTANDO_TEST_MODE spawned agents run on Linux, which directly resolves this PRs two agent.state integration timeouts and the five inherited crash-only failures. The subprocess coverage fix and 99% voice-lock coverage are inherited as well; current upstream main is included through the parent merge. Local verification: agent-state-protocol.test.ts passes 20/20, including both spawned-agent integrations; tsc is clean; targeted ESLint has zero errors. Required fork workflows are currently action_required and await maintainer approval. |
…dle fence
agent.state v1 protocol (design 1a', impl plan WS1 Step 12, amendments
R8/A9/A10/S3/Z3): a new src/voice-agent-state.ts owns the pure pieces —
createAgentStateProvider assembles {type:'agent.state', v:1, initialized,
upstream: live|idle|connecting|backoff|failed, reason?, category?,
clientAttached, credentialSource?, credentialGeneration?, launchdContract?}
from late-bound getters; voice-agent wires it to bodhi's probeState option
behind a feature-detect (the Step-11 pin lands separately), sends an
immediate frame on every accepted real connection, and repeats on every
upstream transition (session.stateChange subscription + the classifier's
non-retryable close path; reason/category changes within 'failed' count
as transitions).
R8: the failure classification extends voice-error-classifier.ts with a
stable protocol mapper (auth_invalid -> failed/auth/auth-invalid,
quota_exceeded -> failed/quota/quota-exceeded, credits_depleted ->
failed/quota/credits-depleted, model_not_found -> failed/other;
retryable -> null) and persists the last terminal classification for
buildAgentState (cleared on ACTIVE recovery). The startup /credit|quota/
regex stays a logging hint; startup failures route through the same
classifier — no second classifier.
A10/S3: credentialSource comes from the EXISTING resolveCredential result
(voiceApiKey() signature untouched; env maps to 'byok' on the wire).
resolveCredential now also REPORTS an opaque Rust-minted generation —
managed entries' `generation` field or the injected
SUTANDO_VOICE_CREDENTIAL_GENERATION for the voice env tier; legacy keys
stay generationless (field omitted). launchdContract:1 echoed only under
SUTANDO_VOICE_LAUNCHD_CONTRACT=1 (R17).
A9: this emitter is the ONE writer of state/voice-lifecycle.json
{at, clientAttached, initialized, upstream, category, credentialSource,
credentialGeneration}, published on client attach/detach, the initialized
flip, and upstream changes via temp+rename with unique per-write temp
names (writeVoiceState's plain writeFileSync is not the pattern).
Z3: createIsolatedIdleRestore arms an isolated idle-teardown timer on
probe close with no real client attached (restoring the prior idle
state), fenced by any later real connection; until the bodhi pin exposes
a probe/verifier-close hook, the probeState callback is the in-repo arm
point (seam documented).
Tests: tests/agent-state-protocol.test.ts (frame shape/vectors, resolver
generation reporting incl. legacy omission, lifecycle-file atomicity with
a cross-process torn-read race, Z3 arm/fence semantics, and spawned-agent
integration: immediate frame on connect, injected terminal close ->
failed/quota transition frame through the real transport.onClose seam,
lifecycle attach/detach tracking) plus classifier mapper/persistence
cases in tests/voice-error-classifier.test.ts. Existing TS suite green
(772/772) and voice-adjacent python suites pass; the state-paths gate
drove voiceLifecyclePath through the canonical statusPath resolver.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tests Fixes the review nit that 19981-19984 were hardcoded — parallel runners or a stray listener could collide. Each spawn now binds :0 and reads back the assigned port. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fb91559 to
ea8a00c
Compare
|
@cla-assistant check |
|
Stack rebuild complete at |
…e mirrors the LATEST failed classification The integration test pinned category === 'quota' on the lifecycle FILE, but 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 category (R8: reason/category changes within 'failed' are meaningful transitions — recordTerminalClassification → emit → publish runs for both). The file only holds the LAST state, so a 150ms poll for 'quota' misses forever once 'auth' lands. Captured live under parallel load: at timeout the file read failed/auth while the quota frame sat in the frames array; reproduced 2/10 sequential and ~1/9 parallel on the pre-fix assertion. The quota mapping stays pinned end-to-end by the FRAME assertions; the file's real invariant — mirrors the latest failed classification — is what the wait now asserts. 15/15 parallel runs green post-fix. Pre-existing since the test landed (sonichi#2689); surfaced on the PR head run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…marker (activation switch) (#2713) * chore(voice): group E — bodhi probe-isolation pin + capability marker (activation switch) Bumps bodhi-realtime-agent to the PR-27 head (probe/verify/takeover connection roles, dist rebuilt) and publishes the capability marker state/voice-agent.capabilities.json {probeIsolation:true} once at voice-agent startup — the strict-true flag the desktop supervisor's ws-upgrade probe battery and exit-7 holder checks gate on. Pin and marker land in the same change on purpose: the marker is the activation switch and must never precede the capability. NOT READY until sonichi/bodhi_realtime_agent#27 merges — the pin then repoints to the post-merge sonichi main SHA (fork SHA is a draft-only stand-in) and the lock regenerates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(voice): repoint bodhi pin to the post-#27 sonichi main SHA bodhi #27 merged via merge commit — 25cc6e8 is the canonical main head containing the probe/verify/takeover roles + dist rebuild. Replaces the draft-only fork stand-in; lock regenerated; installed dist re-verified to carry the role code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(voice-agent): bind the capability marker to its publishing process (review) The marker was a one-way latch: only ever {probeIsolation: true}, nothing deletes or invalidates it, and older builds don't write it at all. After an engine rollback below the capable pin, the stale true on disk would keep the desktop supervisor's ws-upgrade probe battery armed against a build without probe isolation — a ?probe=1 upgrade could then steal a live call, the exact failure the isolation exists to prevent. Fix: the marker now records pid (the publisher's own). Publication runs strictly after acquirePidLock() — a lock loser exits 7 first — so marker.pid equals the live lock holder's pid exactly when the marker describes the running build. The desktop reader (paired in the group-H activation PR) requires that equality plus holder liveness and fails closed on mismatch. Also: package-lock.json rebuilt from main + the pin change only — the previous regen dropped six unrelated "peer": true flags (npm-version churn); the lock diff is now exactly the bodhi pin (spec/resolved/integrity). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(voice-agent): bind the capability marker to the acquisition token, not just pid (review) pid alone is not stale-marker-proof: OS pids are reused, so after a rollback to a build without probe isolation a later run can land on the stale marker's pid and the reader's pid equality would incorrectly arm the probe battery. The structured lock already carries the right token: lockId (vl1-<uuid4>), unique per acquisition — the same class of defense voice-lock.py uses (startTimeMs matching) against pid reuse on release, with exact string equality instead of clock-tolerance semantics. acquireVoiceLock now surfaces the helper's emitted lock record's lockId; voice-agent threads it into the marker. A marker published without a token (helper output unparseable) is unbound: the desktop reader requires the token and fails closed (dormant). The paired desktop reader requires marker.pid AND marker.lockId to equal the current structured lock holder's, plus liveness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(agent-state): fix a pre-existing race — assert the lifecycle file mirrors the LATEST failed classification The integration test pinned category === 'quota' on the lifecycle FILE, but 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 category (R8: reason/category changes within 'failed' are meaningful transitions — recordTerminalClassification → emit → publish runs for both). The file only holds the LAST state, so a 150ms poll for 'quota' misses forever once 'auth' lands. Captured live under parallel load: at timeout the file read failed/auth while the quota frame sat in the frames array; reproduced 2/10 sequential and ~1/9 parallel on the pre-fix assertion. The quota mapping stays pinned end-to-end by the FRAME assertions; the file's real invariant — mirrors the latest failed classification — is what the wait now asserts. 15/15 parallel runs green post-fix. Pre-existing since the test landed (#2689); surfaced on the PR head run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(agent-state): activate the group-E probe-vs-live-client acceptance test The file's Step-11 note scheduled the probe-frame + live-call acceptance test for the group-E pin bump — which this PR is, making the note stale and the promised test due. Against the pinned bodhi, with a real client attached: a ?probe=1 upgrade is intercepted before client attachment, gets exactly its agent.state frame + close(1000), and the client keeps its slot (socket stays OPEN, lifecycle still counts it attached) — the steal the isolation exists to prevent, asserted end-to-end. The same spawn also proves the capability marker in vivo: published (capable pin) and BOUND to the structured lock — marker.pid and marker.lockId equal the lock file's. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(voice-agent): gate the capability marker on the probeState feature-detect (review) The marker publish was unconditional while bodhiSupportsProbeState sat in scope ~100 lines earlier — the never-precede-the-capability invariant held by shipping discipline alone. It breaks the moment npm resolves a bodhi other than the lockfile's (failed or cached install, hand-edited pin): the marker would advertise probeIsolation to a build that routes probes down the normal attach path, and a supervisor probing on a timer could preempt a live call. Gate publication on the same feature-detect that wires probeState; log the dormant branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(voice-agent): never publish an unbound marker; prove both gate branches (review round 5) Two findings, one contract change: 1. The unbound marker failed closed only in another repository: a missing acquisition token still published {probeIsolation, at, pid}, and the entire defense was the desktop reader requiring the token — a safety property living across a repo boundary on a one-way-latch file. An unbound marker has no consumer that benefits from it, so skip publication instead: lockId is now a REQUIRED parameter of publishCapabilitiesMarker and the caller gates on having one. 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 by older builds). 2. The gate was the safety property and nothing tested it — deleting if (bodhiSupportsProbeState) left the suite green. New integration row spawns the agent with a SUTANDO_TEST_MODE-only detect-override seam (same pattern as the injected-close seam), proves the wiring init ran past the marker site (first frame observed), and asserts NO marker file plus the dormant-branch log line. The gate now regresses loudly, on the real caller, for both branches (the TRUE branch is pinned by the probe acceptance test's in-vivo marker-binding assertions). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * style(voice): condense group-E comments to the 2-line policy cap CLAUDE.md: comments state the constraint or non-obvious reason in at most two lines; design rationale, incident history, and review references live in the PR body and commit messages. Five blocks added across this PR's review rounds exceeded the cap (marker docblock, marker-call gate, detect seam, token assignment, acquire parse) — each condensed to its invariant. The acquire-parse comment also dropped a stale claim: since lockId became required, no unbound marker is ever published. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Qingyun Wu <qingyun0327@gmail.com>
Phase 1 / PR group C-partial of the desktop voice-reliability plan (ag2-space/ag2space-cinny-desktop#281, merged there). Structured per CONTRIBUTING.md ("The PR body should answer").
Stack status
Parent #2684 merged as
1a3733aa. This branch was rebuilt on that merge and currentmain, so the PR diff is now the child-onlyagent.state/lifecycle layer. Rebuild commits:675c8b3d(feature),3daddc22(ephemeral-port test fix), andea8a00c1(regenerated source-map count). Intended next order: this → sonichi/bodhi_realtime_agent#27 → desktop #282.What changed, and why?
The desktop supervisor needs a truthful, streaming view of the agent's state (initialized? upstream live/idle/backoff/failed? which credential loaded?) to supervise without guessing — and the incident showed "port is open" proves nothing. This adds:
src/voice-agent-state.ts—buildAgentState(): versioned frame{type:'agent.state', v:1, initialized, upstream, reason?, category?, clientAttached, credentialSource?, credentialGeneration?, launchdContract?:1}.credentialSourcefrom the existing resolver result (voiceApiKey()signature untouched);credentialGenerationis reported, never minted (opaque, host-supplied);launchdContract:1echoed only under the env marker.src/voice-error-classifier.ts(auth/quota/credits/network →{upstream:'failed', reason, category}), persisting the last terminal classification; deliberately no second classifier.state/voice-lifecycle.json— atomic (temp+rename, unique temp names) crash-safe snapshot on every relevant transition; the desktop's call-aware decisions read this, notvoice-state.json's plainwriteFileSync.What files should reviewers look at first?
src/voice-agent-state.ts, thevoice-error-classifier.tsmapper additions, then thevoice-agent.tswiring (frame emission on connect + upstream transitions; lifecycle publisher;probeStatebehind a feature-detect).What user behavior or bug does this prove?
The supervisor/UI can distinguish "agent up but upstream dead (bad key/quota)" from "healthy idle" from "wedged" — the confusions that made the incident invisible.
Feature/documentation consistency
N/A for
docs/canonical documents:docs/catalog.jsonhas no canonical doc for the agent wire protocol; the protocol's canonical spec is the desktop design doc (docs/design-voice-reliability-and-onboarding.md, 1a′, merged in the desktop repo) which governs this cross-repo feature; module-level docs are in the source headers here.design-credential-capability-resolver.mdis unaffected (this PR reads the resolver's result; resolution semantics unchanged).Before/after evidence
Before: no
agent.stateframe exists (a probe/client learns only "socket opened");voice-lifecycle.jsondoesn't exist; classifier verdicts die in the log.After (bot-verify — suites don't exist at base, green at HEAD):
What tests did you run?
Post-rebuild on current
main:npx --no-install tsx --test tests/agent-state-protocol.test.ts tests/voice-error-classifier.test.ts→ 35/35;python3 tests/src-map.test.py→ 25/25;npx --no-install tsc --noEmit→ clean;git diff origin/main...HEAD | bash scripts/review-checks.sh→ PASS. Integration tests use ephemeral ports (3daddc22).Live-path / voice manual evidence (owner, 2026-08-05)
This branch was the
engine/sutandoref bundled into the desktop phase-1 verification DMG. Voice exercised over the rail mic and the room-header Start Call → Voice dropdown (force-tier menu rendered: T0-a/T0-b local rows, T1–2 direct n/a, T3 relay rows); realworkround trips completed — transcript excerpt in #2684's evidence section (same live sessions,data/conversation.sqlite). Lifecycle transitions observed in the logs:state=ACTIVE client=false→ 60 s idle → transport closed →state=CLOSED(clean idle teardown with the emitter active).Edge cases / non-happy paths checked
Legacy no-generation spawn (fields correctly absent, not undefined); torn-read race on the lifecycle file from a second process; terminal-close during an attached client (failed/quota frame streamed);
probeStatefeature-detect is false at the current bodhi pin (no dead wiring).Migrations, config, rollback risks
Additive protocol + one new state file; older clients ignore unknown frames (documented fallback). No config changes; rollback = revert.
Known gaps / follow-ups
Probe-frame + live-call acceptance tests activate with the bodhi pin bump (PR group E, sonichi/bodhi_realtime_agent#27); documented in the test header.
CLA: signed. 🤖 Generated with Claude Code