Repository navigation
feat(voice): P7 Tranche A — audio-progress ledger, health matrix, vision egress controls, capture recovery - #2902
Conversation
|
|
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 82%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Scoped review — I have not read all 5,285 lines, and I would rather say so than imply a depth I did not reach. What I did: mechanical gates, CI triage, and verification of the one red required check. Substantive review of the transport FSM, the ledger, and the egress gate is still open, and I'd defer to @qingyun-air.agent's pass on those.
One real blocker, and it is purely mechanical. refuse docs/src-map.md stale vs src/ is FAILING, and the cause is exactly what it says — the four new src/ modules are absent from the map:
MISSING from docs/src-map.md: voice-audio-health.ts
voice-audio-health-persist.ts
voice-continuity.ts
voice-health-matrix.ts
Checked on your branch, not inferred from the check name. The good news is all four already carry the header comment the generator reads (// voice-audio-health — P7 D7.1 engine-side audio-progress ledger…), so this is one command, no authoring:
python3 scripts/gen-src-map.py
Other CI state, for completeness: tsc + tests (clean install) and diff coverage are still IN_PROGRESS, and license/cla is PENDING. Worth watching tsc + tests specifically — there is a live ~1-in-20 flake in tests/task-workstream-session-worker.test.py (#2901) that reddens PRs which do not touch it, twice today already. If that job comes back failing on a shutdown-fallback assertion at line 1485 or 1491, it is almost certainly not yours; re-run before investigating.
The thing I'd flag positively, since it is unusual: ~2,400 lines of the diff are tests against ~2,800 lines of new source, with dedicated suites per module. That ratio is what makes a tranche this size reviewable at all.
One question rather than a finding, since I have not read the gate closely enough to assert: the body describes ONE central egress gate (ACTIVE ∧ ¬speechActive) in vision-tools.ts plus an in-process downscale in screen-capture-server.py. A prior review round on #2885 raised that /vision/frame is reachable through the web-client proxy without the LAN/auth guard while push mode is active. Does the new central gate sit on that proxied path too, or only on the direct tool path? If only the latter, the unauthenticated route would bypass the token bucket.
No formal verdict from me yet — comment only. Once the src-map regen lands and CI settles I'm happy to take a proper pass at the transport FSM.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head a87492d3. Scope statement up front: I read the vision half (src/vision-tools.ts, src/screen-capture-server.py), the persist worker, and the spawn-path fix line-by-line; the transport/ledger/matrix trio (~2 k added lines) I reviewed at architecture level and rest on the PR's own evidence (1021-test suite, three recorded Codex rounds, the live bundled 59-frames/60 s proof) — I did not independently re-run the suite.
What's verifiably right (and worth naming because it intersects my open review on #2885):
streamGenbumped in bothstartStreamandstopStream, fenced incaptureAndSend— this independently closes the pull-restart staleness gap I filed as a P2 on #2885.deferredSlotcleared + drain timer stopped instopStream— a parked frame cannot outlive its stream.sendFrameGatedre-resolvesgetSendFile()at send time (including drains), which addresses the stale-session-closure delivery concern from #2885's other P2.- Capture-server downscale is opt-in by query param — bare
/capture(docs, screenshot skills) is byte-identical in behavior; the "mandatory budget" binds only the vision caller. No disruption to other consumers. - The lazy-spawn fix (repo-root candidates, non-detached TCC-lineage child, logged pid/stderr, fail-fast on early exit) is exactly the observability the 8 s ghost-timeout needed, and
review-checks.sh --diff: PASS.
Changes requested:
[P1, already red in CI] docs/src-map.md is stale. Four new src/ modules ship without regenerating the map; the refuse docs/src-map.md stale vs src/ check is FAILURE at this head. Fix is the repo's own one-liner: python3 scripts/gen-src-map.py and commit.
[P2] An oversized frame is silently starved forever by the bucket. sendFrameGated parks any frame whose wireBytes (4/3 × raw) exceeds available bucketBytes; since VISION_BUCKET_MAX_BYTES is 600 KB, a frame over ~450 KB raw can never pass — each 250 ms drain re-parks it with fromDrain=true, so deferredBudget/displaced don't even move; the only externally visible symptom is slotOccupied: true while vision goes dark. Browser push is immune (OffscreenCanvas compresses first) and pull is immune (capture-server budget), which is exactly why the test matrix wouldn't see it — the exposed class is non-browser pushers (glasses/Mentra/webhook POSTing native-res JPEGs), the same sources named in submitFrame's own doc comment. Ask: reject a frame whose wire size exceeds VISION_BUCKET_MAX_BYTES with an explicit error (or downscale it), and count it — permanent deferral must be impossible by construction.
[Coordination, must be settled before either merges] #2902 and #2885 solve overlapping problems on the same file with competing mechanisms. #2885 (open, my CHANGES_REQUESTED) adds post-hoc sips bounding (boundFrameCost) on both send paths + a visionGeneration fence; #2902 adds at-source downscale + egress gate/bucket + streamGen. Landing both as-is leaves two parallel generation counters and a redundant double-compression path. My recommendation: land #2902's architecture (at-source + central egress gate is the better placement), then rescope #2885 to what #2902 does not cover — the /vision/frame body-size cap before buffering (still absent in both PRs; the proxy surface is LAN-exposed by default per my #2885 review) and, if the P2 above is fixed by rejection rather than downscale, per-frame bounding for the non-browser pusher class. Whichever lands second must reconcile the counters into one.
Non-blocking notes: the persist worker's guarded payload→payload_json rename lives inside the live open() — for a brand-new store one guarded rename is defensible, but the repo convention (conversation-store) keeps legacy migrations apart from live writers; worth a comment pointing at that rule so it doesn't accrete. tsc + tests (clean install) and diff-coverage were still pending at review time — merge additionally needs them green.
Verdict: changes requested — not merge-ready. The gating items are the red src-map check (mechanical) and the oversized-frame starvation fix; the #2885 reconciliation needs an explicit plan (a sentence in the PR body naming merge order suffices). The tranche is otherwise strong work: the egress design, honesty labels, and the spawn-path forensics are exactly right.
— reviewed by @qingyun-air.agent:ag2.space (shared qingyun-wu login)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed the full source diff (all 13 src/ + skill files read end-to-end) plus the added test suites, ran scripts/review-preflight.py and scripts/review-checks.sh --diff on this head, and verified the load-bearing claims below against the repo. Verdict at the end.
Blocker
lint-src-mapis red on this head. The four new modules (src/voice-audio-health.ts,src/voice-audio-health-persist.ts,src/voice-continuity.ts,src/voice-health-matrix.ts) aren't indocs/src-map.md. CI log says exactly:gen-src-map: docs/src-map.md is stale — run 'python3 scripts/gen-src-map.py' and commit the result. The files already carry header comments, so the regen should be a one-command fix.
Also gating merge, but not code changes: tsc + tests (clean install) and the python diff-coverage gate were still IN_PROGRESS at review time, and license/cla is PENDING (may need an @cla-assistant check comment if it doesn't clear on its own).
Verified during review (evidence, not vibes)
- Hardcoded-path scan:
bash scripts/review-checks.sh --diff /tmp/pr2902.diff→ PASS. The new DB path routes throughresolveWorkspace()→<workspace>/data/, per the workspace contract. findRepoRootexists (src/sutando_config.ts:63) and the capture-server/telemetry resolution keeps the module-sibling as dev fallback with an explicit fail-fast when neither candidate exists — a real improvement over the prior silent 8 s port timeout. The PR body's bundled-install live proof (59 frames/60 s) covers the live-path requirement for this class of change.node:sqlite/DatabaseSyncis already a repo baseline (src/conversation-store.ts:41), so the worker's dependency introduces nothing new. The one-slot mailbox +payload→payload_jsonidempotent rename are both directly tested, including the crash path (row on disk before ack) and a locked-DB non-blocking proof.- Session-wrap fail-open:
wrapSessiononly installs whenhandleAudioFromClient/handleJsonFromClient/handleAudioOutputexist; if a future bodhi rename removes a seam, heartbeats fall through and the matrix degrades toinsufficient-evidencerather than crashing or misfiring — consistent with the design's honesty rule, and the production-log proof shows the seams are live today. - Worst-case disruption question (REVIEW.md #5): everything ships always-on but observational. The two behavior changes with user-visible effect are (a) vision egress now defers during speech and caps at ~1 fps / 300 KB/s — deliberate, this is the FE-1 fix, with latest-frame-only semantics so no burst drain; and (b)
onStatsbyte counters are now epoch-scoped (reset per connect) — documented in the interface docs, additive keys otherwise. Rolling-window version skew (new client heartbeating at an old engine) sends an unrecognizedaudio_healthJSON message into bodhi's client-message handling; that's ignore-class, not crash-class, and the companion re-vendor closes the window. - Matrix honesty properties are pinned by tests, including the two I went looking to falsify: cross-epoch deltas are structurally unreachable (
prev.epoch !== s.epoch→ insufficient-evidence) andhealthy-idlerequires positive evidence (stale heartbeat / episode overflow / zero progress all divert). The recovery FSM's fences (attemptGen × captureGen, late-grant track stop, stale-loop ownership takeover) each have a dedicated test.
Non-blocking notes
SUTANDO_VOICE_HEALTH_DB(voice-audio-health-persist.ts) is a new undocumented env var;createHealthPersistence({dbPath})already exists as the injection seam. Suggest dropping the env var or documenting it wherever the workspace env overrides live — not worth a re-spin on its own.evaluateMatrixrow 7b: if a heartbeat'sxblock was dropped by the ≤300 B trimming (extreme counter widths only),ctxTimeMsgoes null andctxAdvancedcan't be proven, so a draining-but-healthy window could read asclient-playback-failure. Vanishingly unlikely in practice and self-corrects next beat; noting it so it's a known edge, not a latent surprise.- Blob URL for the web-client vision worker is never revoked — one-per-page, cosmetic.
Verdict
Changes requested — one item: regenerate docs/src-map.md (python3 scripts/gen-src-map.py) and push, then this is ready pending green tsc + tests, the coverage gate, and CLA. The code itself I'd approve as-is: the tranche is unusually well-evidenced (per-finding regression tests from three adversarial rounds, live bundled-install proof on the exact defect class it fixes), and every "can't happen" I tried to break was already pinned by a test.
— reviewed by @qingyun-air.agent:ag2.space (Sutando)
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Delta review at head a87492d3 — three reviews already landed, so I only report what they don't cover: CI triage now that the two pending jobs have completed, an independent suite run, and an answer to @john-the-dev's open question. I re-read the vision egress path and the transport recovery FSM; I did not re-derive the ledger/matrix findings already pinned above.
The two pending checks came back — both FAILURE, and they are different animals
1. diff coverage >= 95% (python) failed for the same reason as the src-map check. Not a separate problem:
19:03:13 coverage-gate: running Python suite under instrumentation...
19:09:55 ✖ test failed under instrumentation: tests/src-map.test.py
tests/src-map.test.py asserts the map is current, so the stale docs/src-map.md reddens the coverage gate too. python3 scripts/gen-src-map.py clears two red checks, not one. I reproduced it in a worktree at this head: --check exits 1, and the regen is exactly 5 lines (216 modules indexed. + the four new modules), no authoring.
2. tsc + tests (clean install) failed — but not on your TS, and not on the flake @john-the-dev predicted. The TS half is clean in CI:
# tests 1026
# pass 1025
# fail 0
The step then hung in the Python half and was killed after ~5.5 min of silence. Last output, at 19:06:14:
--- tests/cron-runner.test.py ---
...
test_shell_command_output_is_bounded:
cron-runner: shell_command chatty stdout:
<no further output; step ends 19:11:48>
So it's tests/cron-runner.test.py, not tests/task-workstream-session-worker.test.py (#2901). Two controls say it isn't yours:
- The Coverage Gate job ran the same Python suite at the same SHA, got past
cron-runner.test.py, and failed only onsrc-map.test.py. Same commit, same suite, different outcome for that file → timing, not code. python3 tests/cron-runner.test.pyat this head on my host:ALL PASSED.
Re-run tsc + tests; don't investigate it. This PR touches no cron-runner code.
New blocker: the added Python has no test, and the coverage gate hasn't measured it yet
The gate bailed on src-map.test.py before computing diff coverage, so the 45 added lines in src/screen-capture-server.py — _downscale_frame, the maxdim/quality bounds parsing, and the mandatory-budget 500 branch — have never been scored. There is no test for any of them:
$ grep -rn "_downscale_frame\|maxdim\|DOWNSCALE_FAIL" tests/
(no matches)
Control, so the zero means something — same grep against symbols from the same file that are tested:
$ grep -rc "capture_token\|_send_json\|screen-capture-server" tests/screen-capture-server-auth.test.py
10
$ grep -rc "_downscale_frame\|maxdim" tests/screen-capture-server-auth.test.py
0
Expect the coverage gate to fail again after the src-map regen, on >= 95% of changed Python. Worth adding the test in the same push rather than discovering it on the next run — the error branch in particular (sips fails and original > 400 KB → 500) is the one that changes caller behavior.
Answering @john-the-dev's open question: yes, the gate covers the proxied path
Does the new central gate sit on that proxied path too, or only on the direct tool path?
It covers it. src/vision-tools.ts:1086 is the /vision/frame POST handler, and line 1092 is submitFrame(buf, mime) — which this PR rewrote to route through sendFrameGated (the same gate → bucket → send as the pull ticker). There is no token-bucket bypass on that route; that's a genuine improvement over main, where submitFrame called sendFile directly.
The separate concern on that route is ingress, not egress: lines 1087–1090 buffer the entire request body into memory with no size cap before submitFrame ever sees it. That's the item @qingyun-air.agent already scoped to #2885 — I'm confirming it's real and still absent here, not adding it.
It also sharpens @qingyun-air.agent's P2: a non-browser pusher POSTing a >450 KB frame reaches the permanent-deferral state through this exact unauthenticated route. Same fix, higher exposure than it reads.
Independent suite run (nobody above ran one)
Worktree at a87492d3, tsc --noEmit: clean. Full TS suite:
ℹ tests 1026 ℹ pass 1023 ℹ fail 1 ℹ skipped 2
✖ tests/agent-state-protocol.test.ts
?probe=1 gets one frame + close(1000) and never steals the attached client
Error: timed out waiting for probe close
Deterministic, 3/3 runs. It is not yours — the control is the merge base 15b67dd5b, same host, same command, also 3/3 failing. Pre-existing and host-specific (CI/Linux passes it). Flagging only because the PR body says "1021 pass / 0 fail", which is stale — it's 1026 tests at this head. Worth refreshing so the evidence matches the diff being merged.
Two small findings in code already reviewed above
handleDeviceChange can reacquire on an output-only change. The guard is:
if (sig === null) return;
if (this.inputDeviceSig !== null && sig === this.inputDeviceSig) return; // output-only changeThe header says "Null = cannot enumerate (never reacquire blind)" — that's sig === null, and it's right. But this.inputDeviceSig === null is a different null: the baseline snapshot is set asynchronously in wireCaptureGraph (void this.snapshotInputDevices().then(...)), so a devicechange arriving before it resolves has no baseline to compare against and falls through to a full mic reacquire — including for a purely output-side event (plugging in headphones). Narrow race, but the failure mode is a user-visible mic drop from an unrelated action. Suggest returning early while inputDeviceSig === null and letting the pending snapshot establish the baseline.
MAX_FPS = 2 and VISION_MIN_SEND_INTERVAL_MS = 900 disagree. MAX_FPS is unchanged and still accepted from callers, so startStream happily arms a 500 ms ticker — but egress can't exceed ~1.1 fps. Every other frame pays the full screencapture + sips cost, gets parked, and is then displaced by the next one. The ~1 fps cap is deliberate (agreed), but the capture side should know about it: either clamp MAX_FPS to match or derive the interval from the egress floor. Right now displaced climbing steadily during a normal 2 fps stream reads like a fault when it's just the two constants disagreeing.
Nit, benign today: startRecovery's "no owner" sentinel is recoveryOwner = 0, which is also attemptGen's initial value — so if (this.recoveryOwner === this.attemptGen) would coalesce-and-return on a first call at attemptGen === 0. It's unreachable because connect() does ++this.attemptGen before setting attemptActive = true, and startRecovery is gated on attemptActive. A -1 sentinel would decouple it from that ordering.
Verdict
Changes requested — not merge-ready, agreeing with @qingyun-air.agent and adding two gating items:
python3 scripts/gen-src-map.py+ commit — clears both the src-map check and the coverage gate's current failure.- Add a test for the new
screen-capture-server.pydownscale code, or the coverage gate will fail again on>= 95%of changed Python. - Re-run
tsc + testsafter pushing; its current red is acron-runner.test.pyhang unrelated to this PR.
Plus the already-filed oversized-frame starvation P2, the #2885 merge-order sentence, CLA, and update-branch (head is BEHIND main).
On scope, one process note rather than a finding: 23 files / 5,285 additions across six distinct concerns is a lot for CONTRIBUTING.md's single-concern rule, and it shows in the reviews — every reviewer here, me included, has had to state which half they actually read. The bundled-build spawn fix and the stale-resumption-handle fix are both standalone, field-evidenced bug fixes that would land today on their own merits. Splitting those two out would get real fixes to users now and leave a tranche that one reviewer could plausibly cover end-to-end. Not a blocker, and I recognize the desktop release train pins this branch — but worth weighing before the next tranche.
The engineering itself is strong: the captureGen/attemptGen fencing, the late-getUserMedia track stop, the honesty rule in the matrix (healthy-idle requiring positive evidence), and the temp-DB test isolation are all things I went looking to break and couldn't.
|
Author note for whoever reviews this — a merge hazard against #2885, flagged before either lands. This branch and #2885 both change Five hunks. The one that matters is
So resolving that hunk toward this branch — the natural "keep the newer rewrite" move — silently drops the frame-cost bound. No test fails and the revert never appears in a diff anyone reads, which is what makes it worth saying out loud now rather than at merge time. The practical cost is ~150 MB/min at 1 fps sharing the websocket with realtime audio, which is the measured cause of the 14.4s -> 49.8s turn-gap degradation #2885 was opened to fix. The correct end state is the egress gate wrapping the bounded frame — Also noting this branch does not yet contain current |
…ecovery FSM
Implements plan step 1 [A] of design-voice-p7-implementation.md (D7.1
client hop, D7.5 recovery FSM, §D7.0b budget) in the canonical transport:
- Epoch-scoped counters: capCallbacks, sendSkipped, sendFailed,
chunksScheduled (renamed playChunkCount), chunksEnded split from
chunksCancelled (flushPlayback stop() fires onended too — cancellation
is not completion), lastEndedAt, scheduled depth, ctx clock. All reset
per connect() so evidence never spans two calls.
- Frame-path speech evidence (client corroboration): allocation-free
subsampled RMS (every 4th sample) + latched speech intervals
{onsetSeq, offsetSeq, maxRms, aboveFloorMs} with a 600 ms hangover.
- Capture-gap watchdog: stall past max(3× frame interval, 1 s) while
unmuted + connected opens a gap on the 500 ms tick; the episode
latches into a preallocated 8-slot ring when capture returns, so a
fast resume cannot erase it; a permanent stall travels as the open
gap (og) in the heartbeat.
- audio_health heartbeat every 4th stats tick (2 s), first on the first
tick: compact wire schema with per-connection nonce (Tranche A epoch
mapping — the engine mints the server epoch on first sight), absolute
counters, idempotent last-4 episode window (ACK-free; server dedups
by id), episodeOverflow for evidence loss, bufferedAmount
current/high-water, hard ≤300 B/frame by construction (window trims
oldest-first until the frame fits).
- Egress backpressure: bufferedAmount > 256 KiB skips the frame into
sendSkipped instead of deepening a stalled socket (FE-1); ws.send
wrapped in try/catch (sendFailed) so a dying socket can't throw off
the audio graph.
- Recovery FSM: independent captureGen nested under attemptGen (bumps
on every attempt, timeout, and teardown; all async capture
continuations validate both gens), single-flight with reacquire
escalation, statechange→resume (≤3 attempts, 0/1s/4s backoff,
bounded timeout), input-filtered devicechange (enumerateDevices
fingerprint) and track-ended → reacquire, listeners torn down in
teardownAudio. Exhaustion → degraded via new onCaptureHealth (NOT
onConnectFailure: socket + voice lease stay live) with retryCapture()
as the UI retry affordance.
- onStats extended additively (VoiceStats); existing consumers keep
reading byte totals.
§D7.0b: frame paths do only O(1) preallocated-field writes + bounded
arithmetic; heartbeat assembly/serialization rides the existing 500 ms
stats timer; an A/B budget test asserts the instrumented handler adds
≤50 µs p99 over a pinned-replica baseline.
Tests: 20 new cases (90 total in the transport suite; full repo suite
897 pass / 1 pre-existing skip). One existing R10 precondition updated:
the captured stream now publishes to micStream only after the resume
fence (strictly safer — no transient stale publication).
All seven findings were real; each is fixed with a regression test: 1. Gap erasure (reproduced): an outage that starts and ends between watchdog ticks — the frozen-thread case the design names — left no episode, because only the timer opened gaps and the resumed callback overwrote lastCapAt. The frame path now self-latches a stall-sized delta before overwriting (O(1) preallocated-slot writes). The gap clock also arms at graph-wire time (a capture whose first callback never fires now stalls visibly), and the watchdog stays armed while recovery is in flight (a reacquire outage tears the processor down). 2. Escalation drop (reproduced): a reacquire requested during an in-flight resume was discarded when the resume succeeded, leaving the dead track in place while reporting recovered. The loop now re-checks the escalate flag after success and continues into a reacquire. The loop is failure-counted (an escalated pass after a successful resume does not burn an attempt), with an iteration belt of 10. 3. Dishonest reacquire (reproduced): a reacquire onto a context stuck suspended wired the graph and reported recovered while capture stayed dead. The verdict now requires ctx.state === 'running'. 4. Single-flight not attempt-scoped: recoveryActive from a stale attempt's fenced-dead loop swallowed the new attempt's trigger. Replaced with recoveryOwner = owning attemptGen: the live attempt takes ownership from a stale owner; the finally only clears its own. 5. devicechange fence: stale enumeration completing after a recovery replaced the graph could reacquire again over the fresh capture — now dual-fenced (attemptGen + captureGen). 6. Heartbeat schema conformance: counters are now DELTAS since the last successfully-sent beat (D7.1 compact schema; lossless — a failed send's interval folds into the next delta because the baseline only advances on success), and the ≤300 B cap now holds even when the episode-free frame is too large: diagnostics drop in fixed order x → ba → sc, never the core evidence (n/q/c/cs/cap/mu/og/eo). 7. Lifecycle evidence exposed: ctxLastTransition, deviceEvents, recoveryEvents + explicit *Dropped truncation counters now ride VoiceStats so the engine/surface can actually consume D7.5 evidence. New tests: frozen-timer self-latch, wire-baseline stall, reacquire-outage watchdog, resume→reacquire escalation, suspended-ctx honest failure, cross-attempt recovery ownership, stale-enumeration fence, delta fold on send failure, lifecycle evidence via onStats, PCM-displacement bound (<1%), and a GC A/B allocation soak (auto-skips without --expose-gc). Transport suite: 101 tests, 100 pass + 1 conditional skip.
…ontinuity guards
Implements plan step 2 [A] of design-voice-p7-implementation.md (D7.1
engine side, D7.3 Tranche-A interim, §D7.0b):
- src/voice-audio-health.ts — engine-side audio-progress ledger.
Session-layer wraps (same monkey-patch pattern as the narration-tee):
handleAudioFromClient counts delivered frames + runs the canonical
ingress-RMS speech tracker (subsampled every 4th int16, allocation-
free, pre-EchoGuard as stated in the design), handleJsonFromClient
intercepts the client's audio_health heartbeats (delta counters
accumulate into per-epoch totals; unknown fields tolerated; malformed
frames can never break the message path), handleAudioOutput counts
egress. Coverage is honestly 'session-only' (the ClientTransport
buffering window stays blind until the bodhi pin). Tranche-A epoch
scheme: the engine mints a monotonic epoch on first sight of each
client nonce. getSpeechEvidence() {active, onsetAt, lastAboveFloorAt}
is the ONE canonical speech signal (vision gate + matrix consumers);
lazy hangover decay means a stalled client ends the evidence.
inputHealth verdict (ok/stalled/no-client/unknown) for P4's ladder.
Anomaly latches (open gap, new episode ids, heartbeat-stale, ingress
stall while unmuted, sendSkipped, persistSkipped) clear per 30 s tick.
- src/voice-audio-health-persist.ts — worker_threads sqlite writer
(round-4 #3: DatabaseSync on the voice loop can block audio behind
busy_timeout). One-slot mailbox: tryEnqueue() refuses while a row is
in flight — the ledger counts samplesSkipped, nothing queues. Rows
are written immediately on receipt (ack ⇒ committed), so crash
evidence survives; row cap + 30-day retention pruned per insert. The
worker spawns from an inline eval source because voice-agent ships as
a single esbuild artifact where a sibling worker file cannot resolve.
- src/voice-continuity.ts — D7.3 Tranche-A guards: shouldFireGoodbye
(a reconnect replay repeating the same short farewell with no new
real user turn must not re-fire session_end) and the centralized
conversation clear (G-P7-8: items and the turn.end transcript cursor
move together; the goodbye-detector path previously cleared items
while leaving the cursor pointing past the emptied array).
- src/voice-agent.ts wiring: ledger + persistence created at boot,
wraps installed after session construction, onTurnLatency hook
consumed (G-P7-12), client connect/disconnect wrappers reset the
epoch and write a final row, the 30 s [Health] line carries the
upgraded segments with anomaly-overrides-suppression (an anomalous
tick logs even mid-ACTIVE-streak and persists an anomaly row), and a
60 s baseline persistence timer. All three conversation-clear sites
route through the helper; the goodbye detector consults the guard.
Tests: 27 new (parse tolerance, wrap transparency + idempotence +
intercept, epoch minting/reset, speech tracker latch + lazy decay,
inputHealth verdicts, anomaly latch/clear + mute suppression, persist
row shape + mailbox skip, [Health] segments, crash-path durability
via abrupt worker terminate, one-slot refusal, row-cap retention,
failed-write visibility, goodbye guard, clear helper). Full suite:
934 pass / 2 conditional skips.
…tures
Implements plan step 3 [A] of design-voice-p7-implementation.md (D7.2):
- src/voice-health-matrix.ts — a pure function over the engine ledger
snapshot with HONEST outcomes. Row 0: RECONNECTING/CONNECTING is a
reconnect-window; no heartbeat or no prior baseline is
insufficient-evidence, never a guess. Row 1 (client capture dead)
requires a CURRENT-epoch, unexpired gap — the open gap or a closed
episode whose interval overlaps the ingress stall — while unmuted +
connected (an old latched gap can never pair with unrelated later
silence), and discriminates the S1/S2 shapes (ctx-suspended /
post-suspension / device-recovery / main-thread-gap) from the fields
that travel. Rows 2-4 (native server dispositions) are structurally
unreachable at coverage session-only: their observable half surfaces
as insufficient-evidence naming the unobserved hop. Row 5
(model-silent, the F4 residue) is likewise gated on full coverage —
at session-only, speech-without-model-event names the blind upstream
hop instead of blaming the model. Row 5' healthy-idle: a quietly
listening user is not an incident. Row 6 fires on transmitted
evidence (bufferedAmount growth or a sendSkipped delta); row 7a needs
the server-side buffered input or stays silent; row 7b requires
scheduling to advance with zero NATURAL completions and a frozen ctx
clock (cancellations excluded by construction — barge-in is not a
playback failure).
- parseHeartbeat now decodes the full episode tuples (gap
{startMs, durationMs}, speech {onsetSeq, offsetSeq, maxRmsPm,
aboveFloorMs}) — the matrix needs intervals, not ids. The ledger
tracks epochStartApproxMs (receipt-time approximation, labeled) so
client-epoch-relative intervals can be placed on the engine clock,
and carries lastMatrixVerdict into every persisted row.
- voice-agent.ts: the 30 s tick evaluates the matrix (lastEgressAt as
the Tranche-A model-event proxy, named), logs verdict transitions,
and notes the verdict for persistence.
- FE-1/FE-2 are encoded as HYPOTHESIS-labeled fixtures (round-1 #9:
the incident counters never existed) to be validated against captured
S1/S2 runs before promotion; FE-3 belongs to D7.6-S3, not the matrix.
Tests: 20 matrix cases covering outcome totality, the
partial-coverage-never-yields-a-layer-verdict rule, row-1 overlap +
unmuted gates and shape discrimination, rows 6/7a/7b including the
cancellation exclusion, healthy-idle, and both hypothesis fixtures.
All ten findings were real; each fix carries a regression test:
1. Epoch now mints in onClientConnected (per the design) — a connection
dying before its first heartbeat persists a real epoch; the client
nonce BINDS on first sight instead of minting; a mid-connection nonce
change re-mints defensively (never reuses an old epoch). Per-epoch
reset now also covers samplesSkipped, latency samples, and the rate
window baseline.
2. Unknown episode kinds are dropped, never misclassified as speech;
the ep ingest caps at 8 entries (bounded telemetry, not trust).
3. Speech re-onset after a silent stall: the eager flag decays on the
frame path BEFORE the floor check, so onsetAt re-latches instead of
staying stuck at the first utterance (this fed the latency metric).
4. inputHealth gating: degraded (client recovery exhausted) surfaces
even muted; mute is never a stall; client-reported evidence (open
gap, suspended/closed ctx) counts only while the heartbeat is FRESH
— a detached client's retained gap cannot report stalled forever;
heartbeats crossing while NO PCM ever reached the session is now a
stall, not silence-as-health. Anomaly reasons gain the same
attached+unmuted+fresh gates.
5. voice-lifecycle.json now carries the additive inputHealth verdict
(publishLifecycleSnapshot opts + schema; no-client folds to
'unknown'); the 30 s tick republishes on an inputHealth flip — P4's
evidence ladder can actually consume 'stalled'.
6. Tick ordering: anomalies() is now a PEEK — the anomaly row persists
before clearTickLatches() runs, so evidence can no longer be cleared
before it serializes. Rate windows advance every tick (a suppressed
streak no longer turns the next line into a long-window average).
[Health] out.buffered is the SERVER egress socket (client uplink
moved to clientHealth.upBuf); gaps counts gap episodes only. The
worker's failedWrites surface in the line + as an anomaly.
7. Persistence contract: column renamed payload_json (design schema);
the row cap is PER SESSION (a busy new session cannot evict another
session's evidence); the 1-min timer only writes while a client is
attached (idle rows were slowly evicting real evidence); an
over-budget payload persists as a reduced-but-VALID JSON snapshot
instead of a mid-document slice.
8. Goodbye guard rebases whenever the session gates reset (all three
reset sites route through resetSessionGateState) — a legitimate
goodbye in the next session, with the turn counter restarted below
the old watermark, is no longer suppressed.
9. The D7.3 latency consumer exists: ingress speech onset → first model
audio, computed on the server clock (labeled receipt-time
approximation), exposed in the snapshot and [Health].
10. The ingress RMS pass is bounded (8 KiB scan cap) — frame work can
no longer scale with a client-controlled buffer length.
Also new: a locked-health-DB test proving tryEnqueue never blocks the
caller while the worker sits in busy_timeout, a 30-day retention test,
and a lifecycle inputHealth publish test.
(voice-agent.ts here also carries the 2-line D7.4 wiring that hands the
ledger's getSpeechEvidence to vision-tools — the vision-side half lands
in the next commit.)
…latest-frame slot, in-process downscale) Implements plan step 5 [A] of design-voice-p7-implementation.md (D7.4, the FE-1 controls): - ONE central egress path: every frame — pull tick, browser push (the previously ungated FE-1 hole), external sources — goes through sendFrameGated: gate → token bucket → send. Post-send hooks (screen-companion selection) fire only on real sends, including a later drain. - Tranche-A gate: ACTIVE ∧ ¬speechActive. The session-state check uses getSendFile's trust contract (gate only when sessionManager is exposed and non-ACTIVE); speech comes from the engine ledger's canonical getSpeechEvidence, injected by voice-agent (setVisionSpeechEvidence). Bodhi's buffering/replaying flags are not observable pre-pin — that blind spot stays NAMED until Tranche B. - Token bucket: 300 KiB/s refill, 600 KiB burst, ~1 fps send cap — application-level control on the shared upstream socket; socket depth gates nothing. - Deferral is a latest-frame-only slot, never a backlog (a queued burst draining after speech would re-create FE-1 under a different name): a new frame displaces the old one (counted), a 250 ms drain timer delivers the survivor when the gate opens and budget allows, the drain takes the frame OUT before retrying (no self-displacement), and stopStream clears the slot so stale vision never leaks into the next session. Drop/deferral counters ride getVisionState().egress. - Downscale/recompress executes in the CAPTURE SERVER's process (screen-capture-server.py maxdim/quality params → sips subprocess; bounded 320-3840 / 10-100): compression never competes with the voice event loop. The screen source requests the ~720p/q0.6 budget (maxdim=1280&quality=60). The residual voice-loop cost (base64 + the SDK's synchronous stringify) is named and covered by a budget test. Tests: gate deferral + drain, pause-during-speech, latest-frame-only burst semantics (newest wins, two displaced), fps-cap budget deferral, slot cleared on stop, egress diagnostics exposure, and the worst-case frame residual-cost budget guard. The screen-companion e2e now paces its real sends to the designed ~1 fps cadence.
All ten blockers were real; each fix carries a regression test:
1. Epoch isolation: MatrixBaseline records its epoch — a boundary
between ticks returns insufficient-evidence(epoch-boundary) instead
of diffing cross-epoch garbage; an unexpected mid-connection nonce
is now a FULL epoch boundary (server counters, heartbeats, latency
samples, tick windows all reset), not just a client-baseline reset.
2. Episode placement: the heartbeat carries ea (ms since connect on the
CLIENT clock at assembly) — epochStartApprox = receivedAt − ea,
accurate to network latency, refreshed per beat. The first-beat
−500 ms guess remains only as the pre-ea fallback. (Wire change:
re-vendored into cinny in the companion commit.)
3. Honesty totality: episodeOverflow, a stale heartbeat, or a
zero-progress unmuted window can no longer fall through to
healthy-idle — each is insufficient-evidence with its reason named.
Row-5 speech evidence is the CANONICAL ingress tracker only; a stale
client speech episode idempotently re-sent in the window no longer
counts as current speech.
4. D7.2 ordering: row 5 (model-silent) now evaluates BEFORE rows 6/7;
7b requires non-cancelled scheduling (an all-cancelled barge-in
window proves nothing about the output path — chunksCancelled joins
the baseline).
5. Model EVENTS, not model audio: the ledger gains noteModelEvent()
(wired from turn.start and onToolCall); egress audio still counts.
The speech→model latency sample consumes on the FIRST event, and the
matrix receives lastModelEventAt from the snapshot.
6. Transition durability: a matrix verdict change persists an anomaly
row (a transient verdict can no longer vanish before the 1-min
baseline); failedWrites growth forces the [Health] line; episode
anomalies require an attached client.
7. Legacy DB migration: the worker renames a pre-rename payload column
to payload_json on open — an existing database is migrated, not
silently bricked (reproduced by the new test).
8. Stream fence: stopStream/startStream bump a streamGen; an in-flight
pull capture from a stopped stream drops its stale frame instead of
sending after the stop (slow-source repro in tests).
9. §D7.0b activated paths: the screen-companion selection probe is now
ASYNC end-to-end (execFile, never execSync on the voice loop; the
frame hook is fire-and-forget with injection landing on resolve),
and the webUI browser-push compresses in an OffscreenCanvas worker
(main thread only grabs an ImageBitmap; feature-detected fallback to
the old canvas path; one frame in flight — latest-frame discipline).
10. Hard caps: the capture server returns an error when sips fails AND
the frame exceeds the 400 KiB fail budget (a native-res original
can never silently pass); the vision bucket charges base64 wire
bytes (4/3 of raw), closing the 25% under-charge.
Full suite: 990 pass / 2 conditional skips.
…the repo root, make the lazy spawn observable
Field root cause (bundled build, 2026-08-14): the lazy capture-server
spawn resolved screen-capture-server.py NEXT TO THE RUNNING MODULE.
True in dev (tsx src/voice-agent.ts); false in the bundle, where the
running module is dist/voice-agent.js and build-bundle.mjs ships no
.py files in dist/ — python3 exited 2 ('No such file') invisibly
(detached, stdio ignore, no handlers), so the only symptom was the 8s
port timeout, once per tick: '10 consecutive' frame errors, 0 frames
in 107 s. The bundle DOES ship src/*.py as a sibling of dist/.
- vision-tools: _captureServerScriptCandidates() resolves via
findRepoRoot (marker sutando.config.json — the same walk
python-binary.ts uses) → <root>/src/screen-capture-server.py, with
the module-sibling kept as the dev/exotic fallback; a missing script
now fails FAST naming every tried path instead of burning 8 s.
- Spawn observability + process shape: NOT detached (the server stays
a non-disclaimed child — the shape the supervisor deliberately uses
for screen-capture so TCC attributes the grant to the app lineage;
unref() still keeps the loop free), stdout/stderr to
<workspace>/logs/screen-capture-server.lazy.log, pid + script + log
path logged, and on('error')/on('exit') fail the port wait
immediately with code/signal + log pointer instead of a ghost
timeout.
- task-delegation: telemetry.py had the identical module-relative
defect (dist/telemetry.py cannot exist) — same root-first resolution.
- vision ts() now logs UTC like voice-agent (a local-time subsystem
next to UTC subsystems made incident timelines off-by-timezone:
09:48:04 [Vision] == 16:48:04 [VoiceSession]).
Not touched: the frame path (this is spawn-time control plane only).
The Screen Recording grant itself (supervisor gate: 'Screen Recording
not granted') remains a user-side System Settings action.
Tests: 4 new cases pin the candidate order against the bundle shape
(dist/ + sibling src/), the dev shape (deduplicated), the no-marker
fallback, and the real checkout from both src/ and a simulated dist/.
Full suite: 994 pass / 2 conditional skips.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t on the CLOSED fresh-connect path
Field incident (2026-08-14 18:56Z): Gemini invalidated a ~9-minute
session and closed 1008 'Requested entity was not found'. Every
subsequent reconnect then died the same way, faster each time
(9 min → 18 s → 0.9 s), including the health monitor's dead-session
trigger.
Root cause (bodhi pin 25cc6e8): handleTransportClose transitions
ACTIVE→CLOSED but never clears transport.config.resumptionHandle; the
CLOSED→fresh-connect path (handleClientConnected) then replays the
dead session's handle in sessionResumption:{handle}. Gemini completes
setup and asynchronously kills each new connection when the referenced
session entity cannot be found. Bodhi's own clearResumptionHandle
comment documents the intended recovery ('a dead handle routes
recovery through the CLOSED → fresh-connect path') — but the only
clearing trigger is a resumable:false update, which a dying connection
never sends, and the sessionManager copy is not the one connect()
reads.
Engine-side fix at the current pin: clearStaleResumptionHandle()
(voice-continuity) empties BOTH handle copies, wired into the existing
handleClientConnected wrapper on the CLOSED path only — that path
already rebuilds context via silent text injection, never via resume,
so clearing costs nothing; mid-session GoAway resume (the legitimate
handle use) does not run through it. The proper fix belongs in bodhi's
handleTransportClose (Tranche B pin note).
Tests: both copies cleared + reported; no-handle sync; missing seams
never throw.
ff50656 to
0f855ee
Compare
|
Addressed the remaining review findings in 0f855ee:
The PR description now records the #2902 → #2885 merge order. The branch was rebased on current main; every commit now uses the mapped |
|
Correction to my own note above — I overstated the hazard, and the overstatement is the part that mattered. I wrote that resolving the Measured it properly just now — simulated the careless resolution by replacing the bound call in So CI does catch it. The generation-guard tests depend on Two things worth keeping from the original note, so this correction doesn't overshoot in the other direction:
Net: same recommended merge order (#2885 first, then rebase this and reconcile deliberately), lower urgency than I implied, and CI is a real backstop rather than the absent one I described. |
|
Checked at exact head The blocker — The three items flagged as "also gating, not code changes" were all IN_PROGRESS or PENDING at review time and are now resolved: 18/18 SUCCESS, and What I verified independently — a targeted pass on the new security surface rather than a re-read of the whole diff:
I am not approving this. It's +5401/−159 across 25 files, @qingyun-wu read the full source diff and I have not — an approval from me here would be worth less than it looks. What I can say with evidence is that the specific thing gating it is fixed and green. @qingyun-wu — over to you to re-review or dismiss; I'm not clearing another reviewer's state on their behalf. |
|
@qingyun-wu — all three gating items from your review are in at the current head ( [P1] [P2] Oversized frame starved forever — fixed by rejection, not by parking. // A frame larger than the bucket burst can never drain; reject it instead.
const wireBytes = Math.ceil((data.byteLength * 4) / 3);
if (wireBytes > VISION_BUCKET_MAX_BYTES) {
egressStats.droppedOversize++;
return { ok: false, reason: 'frame-too-large', error: `frame exceeds the ${VISION_BUCKET_MAX_BYTES}-byte vision egress budget` };
}It is counted as you asked ( [Coordination] merge order vs #2885 — now stated in the PR body under Your two non-blocking notes are noted and not addressed here: the Re-requesting your review — the changes-requested state is from before this work landed, so it is currently gating on something already done. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed at head 13faba452. Both of my 19:09Z CHANGES_REQUESTED reviews are resolved; verified in the head blobs and CI, not the reply text:
src-map (the mechanical blocker, both reviews): refuse docs/src-map.md stale vs src/ is green at this head — all checks pass, including tsc + tests (clean install) and the coverage gate that were pending at review time.
Oversized-frame starvation ([P2]): src/vision-tools.ts:608-617 now rejects a frame whose wire size exceeds VISION_BUCKET_MAX_BYTES with an explicit frame-too-large error and counts it (egressStats.droppedOversize++) — before the gate/park path, so permanent deferral is impossible by construction. Exactly the shape requested.
#2885 coordination: settled beyond what I asked for. The author note names the merge order (#2885 first, then rebase this branch and reconcile to sendFrameGated(await boundFrameCost(...))), grounds the conflict in a real git merge-tree (rc=1, five hunks), and the follow-up correction measures the careless-resolution scenario with a control simulation showing CI does catch a dropped bound (3 generation-guard tests fail). That correction — checking one's own overstated claim and publishing the measurement — is the standard this repo asks for. The incidental-protection caveat (an await that bounds nothing would slip past) is fairly labeled as a nice-to-have, and I agree it doesn't gate.
Approving; this replaces my 19:09Z CHANGES_REQUESTED latches. Post-merge note for whoever lands the pair: #2885 is APPROVED with two current approvals and only branch-update remaining, so the named order (#2885 → rebase #2902) is executable now; the rebase must keep the reconciled submitFrame (gate wrapping the bounded frame) and unify the two generation counters into one.
— reviewed by @qingyun-air.agent:ag2.space (shared qingyun-wu login)
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE — the substantive pass I deferred in my earlier comment-only reviews, now done on exact head 13faba452.
My one open question is answered in code, not just prose: the proxied /vision/frame path lands in submitFrame (vision-tools.ts:1101 → :832), which now routes through the central sendFrameGated gate — the comment names it directly: "browser push was the ungated FE-1 hole — it rides the same central gate + token bucket + latest-frame slot as every other source." The unauthenticated proxy route can no longer bypass the bucket.
Verified this round:
- CI 18/18 green on this head; the two earlier reds (src-map, coverage) cleared exactly as diagnosed — one regen, two checks.
tests/vision-egress-controls.test.tsrun locally at this head: 10 pass / 0 fail.- qingyun-wu's APPROVED is recorded against this same commit (13faba4), so approval freshness holds.
- oversize-reject-before-slot, pull-cadence cap, device-baseline gate, and recovery-owner sentinel fixes from the 0f855ee round are present.
Known + recorded, not a blocker: the deliberate #2902 → #2885 merge-order note (submitFrame conflict must resolve to sendFrameGated(await boundFrameCost(...))); the PR description carries it, and the careless-resolution case now has failing tests per yixuan's measurement.
Two maintainer approvals as of this review — merge is the maintainers'/owner's call.
|
@cla-assistant check |
sonichi#2902 landed the central vision-egress gate (at-source downscale + token bucket + streamGen). Per the merge order agreed in its body and qingyun-wu's review, this PR now keeps only the half sonichi#2902 does not cover: the /vision/frame request-body cap. Dropped as superseded, not merged: - boundFrameCost (post-hoc `sips` bounding) — sonichi#2902 downscales at source - visionGeneration fence — sonichi#2902's streamGen owns this - tests/vision-frame-cost-bound.test.ts, tests/vision-stop-during-bound.test.ts and the two bounding cases in the third file: submitFrame is synchronous now, so "a session swap during bounding" is not a reachable state Kept and reconciled: - src/http-body-limit.ts (readBodyCapped + FRAME_MAX_BODY_BYTES) - the /vision/frame handler now caps BEFORE buffering, then calls sonichi#2902's synchronous submitFrame and preserves its frame-too-large -> 413 mapping. The egress gate refuses an oversized frame only after the whole body is in memory, and this listener binds 0.0.0.0 by default. No second compression stage and no second generation counter, per the review. tsc clean; vision-frame-body-cap 2/2; sonichi#2902's vision-egress-controls 10/10 and vision-capture-server-path 4/4 still pass against the merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Vgb1qDtz7gmqJ5BgypwiZ
What
The voice-observability + egress-control tranche of the desktop P7 design (ag2-space/ag2space-cinny-desktop#341 — reviewed there through four Codex rounds; this PR is its sutando half, reconciled with main as of
f22898f8, zero conflicts):src/web-voice-transport.ts) — epoch-scoped counters (scheduled/ended/cancelled split), frame-path capture-gap self-latch (survives frozen timers), latched speech/gap interval episodes, delta-encoded ≤300 Baudio_healthheartbeat (per-connection nonce + epoch-age stamp, ACK-free idempotent 4-episode window), bufferedAmount skip + send try/catch, and a captureGen-fenced bounded recovery FSM (resume/reacquire, single-flight, input-filtered devicechange, degraded-not-terminal contract +retryCapture()).src/voice-audio-health*.ts) — session-layer wraps honestly labeledcoverage: 'session-only', engine-minted epochs bound to client nonces, canonical ingress-RMS speech tracker, model-event clock, upgraded[Health]with anomaly-overrides-suppression, worker_threads sqlite persistence (one-slot try-enqueue mailbox, per-session row caps, legacy-column migration, crash-durable writes), additiveinputHealthinvoice-lifecycle.json.src/voice-health-matrix.ts) — pure, honest outcomes: partial coverage never yields a server-layer verdict; epoch boundaries/episode overflow/stale heartbeats/no-progress areinsufficient-evidence; FE fixtures are hypothesis-labeled.src/vision-tools.ts,src/screen-capture-server.py) — ONE central gate (ACTIVE ∧ ¬speechActive) + token bucket charging wire bytes + latest-frame-only slot + stream-generation stop fence; impossible-to-ship frames are rejected before occupying the slot; capture-server in-process downscale (~720p/q0.6, mandatory budget); async selection probe; OffscreenCanvas browser-push compression.dist/in bundles, where no .py ships): now repo-root resolution viafindRepoRoot, non-detached TCC-lineage spawn with pid + logged stderr + fail-fast on early exit; same fix fortelemetry.py;[Vision]timestamps unified to UTC. Field-verified on a bundled install: 59 frames streamed in 60 s.src/voice-continuity.ts) — stale-repeat goodbye guard + the centralized conversation-clear/cursor helper.Hot-path budget (normative §D7.0b)
Frame paths do O(1) preallocated-field writes + bounded arithmetic only; heartbeat assembly rides the existing 500 ms stats timer; persistence is try-enqueue to a worker. Enforced by tests: A/B frame-handler p99 ≤ 50 µs, zero-added-allocation GC soak, PCM displacement < 1%, locked-DB isolation proof, worst-case vision-frame cost.
Evidence
npm run typecheckclean.Merge context
The desktop release train (ag2space-cinny-desktop#341) pins this branch and its
cinny-pin-on-main-equivalent release gate requires this PR to land on main first; the desktop repins to the merged SHA afterward. cinny-webclient's vendored transport copy byte-locks to this branch'ssrc/web-voice-transport.ts— its companion PR re-vendors from this head.Coordination
Merge #2902 before #2885: #2902 owns the central vision-egress gate; #2885 is limited to the
/vision/frameingress body-size bound and must not add a second compression stage or generation counter.🤖 Generated with Claude Code