Repository navigation
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an opt-in, hidden Voice Gateway for OpenClaw. The gateway validates private credentials, binds to loopback, creates bounded voice sessions, streams sanitized responses over HTTP, and shuts down cleanly. Tests cover security, lifecycle, protocol handling, CLI wiring, documentation, and end-to-end behavior. ChangesVoice Gateway
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime
participant InternalCLI
participant VoiceGatewayAction
participant VoiceGatewayHTTP
participant VoiceSessionService
participant OpenClawVoiceAgentClient
Runtime->>InternalCLI: start hidden voice gateway command
InternalCLI->>VoiceGatewayAction: pass trusted configuration
VoiceGatewayAction->>VoiceGatewayHTTP: create loopback server
Runtime->>VoiceGatewayHTTP: authenticate and create session
VoiceGatewayHTTP->>VoiceSessionService: create grant-bound session
Runtime->>VoiceGatewayHTTP: submit committed turn
VoiceGatewayHTTP->>VoiceSessionService: start authorized turn
VoiceSessionService->>OpenClawVoiceAgentClient: invoke chat turn
OpenClawVoiceAgentClient-->>VoiceSessionService: filtered ordered events
VoiceSessionService-->>VoiceGatewayHTTP: sanitized response events
VoiceGatewayHTTP-->>Runtime: NDJSON stream
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-8427.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 0eb1881 in the TypeScript / code-coverage/cliThe overall coverage in commit 0eb1881 in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 4 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (9)
src/lib/domain/voice/session-service.ts (2)
269-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
startTurndeclaresPromise<void>but throws synchronously on every validation failure.Lines 276-283 throw
VoiceSessionErrorbefore the function returns a promise. The current callers are safe:src/lib/adapters/http/voice-gateway.tswraps theawaitin atry, and the unit test at line 136 asserts the synchronous throw withexpect(() => ...).toThrowError.The hazard is the mixed contract. A future caller that writes
startTurn(...).catch(handler)without a surroundingtryreceives an uncaught exception instead of a rejection.Mark the method
asyncso all failures surface as rejections, and update the unit-test assertions toawait expect(...).rejects. If you prefer to keep the synchronous throw, document it on the method so the contract is explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/domain/voice/session-service.ts` around lines 269 - 297, Update startTurn to be async so validation failures from authorize and requireBoundedString, along with duplicate or active-turn checks, are returned as promise rejections consistent with its Promise<void> contract. Update the affected unit-test assertions to await expect(...).rejects rather than asserting synchronous throws.
146-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDo not use
labelas a behavior switch.
labelis a descriptive tag for every caller except"text". At line 155 it silently becomes a control flag that enables blank rejection. If a later change renames the literal at line 278, the whitespace check stops running. No type error and no other test fails.Make the rule explicit in the signature.
♻️ Proposed refactor
-function requireBoundedString(value: unknown, label: string, maxBytes: number): string { +function requireBoundedString( + value: unknown, + _label: string, + maxBytes: number, + options: { rejectBlank?: boolean } = {}, +): string { if ( typeof value !== "string" || value.length === 0 || Buffer.byteLength(value) > maxBytes || /[\u0000-\u0008\u000b\u000c\u000e-\u001f\u007f]/u.test(value) ) { throw new VoiceSessionError("invalid_request", 400); } - if (label === "text" && value.trim().length === 0) { + if (options.rejectBlank && value.trim().length === 0) { throw new VoiceSessionError("invalid_request", 400); } return value; }Then update the call site at line 278:
const text = requireBoundedString(textValue, "text", VOICE_MAX_TURN_TEXT_BYTES, { rejectBlank: true, });As per coding guidelines: "Prefix intentionally unused variables with
_."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/domain/voice/session-service.ts` around lines 146 - 159, Update requireBoundedString to accept an explicit options parameter, such as rejectBlank, instead of using the label value to control whitespace validation. Apply the blank-rejection check from that option, update the text call site to pass rejectBlank: true, and prefix any intentionally unused label or parameter with an underscore.Source: Coding guidelines
src/lib/adapters/fs/private-credential.ts (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why the walk stops at the first foreign-owned ancestor.
Line 27 ends the check at the first directory that the current user does not own. This skips the group/other-writable check for that directory and every directory above it. The behavior is correct for root-owned system paths such as
/tmp, which are world-writable with the sticky bit. The reason is not visible in the code. Add a short comment so a later change does not remove thebreakor reorder it before the symlink check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/adapters/fs/private-credential.ts` at line 27, In the ancestor-walking logic around the uid ownership check, add a concise comment explaining that encountering the first foreign-owned directory intentionally stops traversal before applying group/other-writable checks, preserving safe handling of root-owned paths such as sticky-bit /tmp; keep the break after the symlink check and do not change the behavior.test/voice-gateway-integration.test.ts (1)
176-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the invocation index by method rather than by position.
invocations[0]andinvocations[1]depend on the exact order and count of frames the client sends. If the client adds a handshake or keepalive request, the indexes shift and the assertion fails or silently checks the wrong frame. Record the method alongside the params inFakeOpenClawSocket.send, then select thechat.sendentry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/voice-gateway-integration.test.ts` around lines 176 - 177, Update FakeOpenClawSocket.send to record each invocation’s method alongside its params, then locate the chat.send entry by method before asserting its scopes, message, and deliver fields. Remove the positional invocations[0]/invocations[1] assumptions while preserving the existing expectations.src/lib/adapters/http/voice-gateway.ts (1)
155-238: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting the request handler into route functions.
The handler mixes routing, admission authentication, body validation, session creation, session deletion, and turn streaming in one async function. Extracting
handleCreateSession,handleDeleteSession, andhandleTurnkeeps each unit small and matches the low-complexity guideline for TypeScript sources.As per coding guidelines: "Keep function complexity low."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/adapters/http/voice-gateway.ts` around lines 155 - 238, Split the inline request handler into focused route functions: extract session creation into handleCreateSession, deletion into handleDeleteSession, and streaming into handleTurn, preserving their existing authentication, validation, session-service, response, and error behavior. Keep the main handler responsible only for path dispatch and shared error handling, and reuse the existing routing symbols and dependencies rather than changing the API contract.Source: Coding guidelines
test/internal-commands-docs.test.ts (1)
102-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect the OpenClaw reference by name.
renderedCommandReferencescurrently placescommands.openclaw.generated.mdxat index 0 and appendsreferences.slice(1). Find the entry bynameinstead of relying on this ordering, then assert absence for the remaining entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/internal-commands-docs.test.ts` around lines 102 - 110, Update the test around renderedCommandReferences to locate the OpenClaw reference by its name rather than assuming it is at index 0. Assert the experimental voice gateway invocation is present in that named reference, then check that all other references do not contain it.Source: Path instructions
src/lib/adapters/openclaw/voice-agent-client.test.ts (2)
233-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the oversize payload from the exported limit.
1_100_000encodes an unstated relationship toVOICE_MAX_RESPONSE_TEXT_BYTES: one frame must stay under the limit and two must exceed it, and both must stay underMAX_NATIVE_FRAME_BYTES. If the domain constant changes, this test keeps passing for a different reason.Import
VOICE_MAX_RESPONSE_TEXT_BYTESand compute the size, for example"x".repeat(Math.ceil(VOICE_MAX_RESPONSE_TEXT_BYTES * 0.6)). The two-frame overflow intent then stays correct when the limit moves.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/adapters/openclaw/voice-agent-client.test.ts` around lines 233 - 251, Update the oversized payload setup in the test’s queueMicrotask message loop to import and derive each deltaText length from VOICE_MAX_RESPONSE_TEXT_BYTES, using a value that keeps one frame below the limit while two frames exceed it and remain below MAX_NATIVE_FRAME_BYTES. Remove the hardcoded 1_100_000 size.Source: Path instructions
143-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the terminal-frame assertion.
expect(onText).not.toHaveBeenCalled()passes for two independent reasons: the post-terminal delta is ignored, and thefinalframe carries no accumulated text, so line 135 ofvoice-agent-client.tsskips the callback. The assertion cannot distinguish them.Emit a delta before the
finalframe and assert thatonTextis called exactly once with only the pre-terminal text. The test then fails if a post-terminal frame ever leaks through.This fixture also covers only the queued-drain path, because both chat frames arrive before the
chat.sendresponse sets the run id. The post-terminal protection for unqueued frames relies on listener removal infinish. Consider a second case that delivers the late delta after the run id is known.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/adapters/openclaw/voice-agent-client.test.ts` around lines 143 - 199, Strengthen the terminal-frame test around OpenClawVoiceAgentClient.invoke by adding a pre-terminal delta with text, then assert onText is called exactly once with only that text after the final frame; retain the late delta to verify it is ignored. Add a separate scenario that delivers the post-terminal delta after the chat.send response establishes the run id, covering the listener-removal path in finish in addition to the queued-drain path.Source: Path instructions
src/lib/adapters/openclaw/voice-agent-client.ts (1)
73-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffExtract the frame handlers from
invoke.
invokespans about 140 lines and holds six mutable turn variables plus four nested closures. The coding guidelines require low function complexity for TypeScript files. ExtractconsumeChatFrameandonMessageinto a small private turn-reader type that ownsexpectedRunId,lastSequence,projectedText,terminal, and the queue.invokethen wires listeners and sendschat.send.This is a readability change only. Defer it if you prefer to keep the experimental path stable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/adapters/openclaw/voice-agent-client.ts` around lines 73 - 83, Refactor invoke in OpenClawVoiceAgentClient by extracting consumeChatFrame and onMessage into a small private turn-reader type that owns expectedRunId, lastSequence, projectedText, terminal, queuedChatFrames, and queuedChatBytes. Keep invoke responsible only for connecting, wiring listeners, and sending chat.send, while preserving the existing frame-processing behavior and experimental path semantics.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/adapters/fs/private-credential.test.ts`:
- Around line 62-63: Remove the unused process.env.HOME assignment and
originalHome save from the test setup, and delete the corresponding conditional
restore block after readPrivateCredentialFile. Keep the test linear and retain
only necessary temporary-directory cleanup.
In `@src/lib/adapters/http/voice-gateway.ts`:
- Around line 128-129: Replace the request.once("aborted", ...) listener with a
request.once("close", ...) listener in the request body handling flow,
preserving the existing VoiceHttpError completion behavior and the separate
error listener.
- Around line 198-222: Update the response close handler near deliveryOpen and
streamStarted so disconnectTurn(voiceSessionId) runs only when the response has
started streaming and is not writableFinished. Keep rejected or otherwise
unstarted requests from invoking sessionService.disconnectTurn, while preserving
cleanup for authorized started turns.
In `@src/lib/adapters/openclaw/voice-agent-client.test.ts`:
- Around line 13-25: Remove the method-based conditionals from the fake socket
send implementations at the base socket and the TerminalSocket and
OversizedSocket overrides. Add a method-keyed responder map to FakeSocket that
handles the shared connect response, then have each subclass extend that map
with only its own chat.send responder using the existing response and fixture
helpers; preserve message recording and response dispatch behavior while
eliminating the duplicated connect responders.
In `@src/lib/adapters/openclaw/voice-agent-client.ts`:
- Around line 329-334: Handle already-aborted signals in both VoiceAgentClient
sites: in request at src/lib/adapters/openclaw/voice-agent-client.ts#L329-L334,
reject with agent_unavailable before registering the abort listener or calling
socket.send; in connect at
src/lib/adapters/openclaw/voice-agent-client.ts#L306-L306, invoke onAbort()
immediately when signal.aborted is true rather than only registering the
listener.
- Around line 195-208: In the chat.send continuation, add the existing settled
guard immediately before assigning expectedRunId and invoking
request.onRun(runId). Return without updating state or calling onRun when the
turn has already finished; leave the queuedChatFrames drain behavior unchanged.
- Around line 272-299: Update the connect handshake in the challenge handler to
preserve the challenge nonce and timestamp, compute the required protocol-v4
device signature, and include both nonce and signature under
connect.params.device alongside auth.token. Update the related test assertion so
it validates the populated device object instead of requiring connect.params to
omit device.
In `@src/lib/domain/voice/session-service.test.ts`:
- Line 51: Add a separate test that constructs VoiceSessionService without
injecting randomGrant, then creates two sessions through the public API and
closes the first binding between them. Assert each default grant matches the
43-character base64url format and that the grants differ; keep the existing
harness test focused on its injected stub rather than treating its literal
length as production-generator coverage.
- Around line 159-201: Move fake-timer cleanup for the tests “rejects a grant
for another or expired voice session (`#8378`)” and “bounds a running turn with a
stable content-free failure (`#8378`)” into the file’s afterEach hook, ensuring
vi.useRealTimers() runs even when assertions fail. Remove the trailing
vi.useRealTimers() calls from both test bodies while preserving their existing
fake-timer behavior.
In `@src/lib/domain/voice/session-service.ts`:
- Around line 395-409: Unify terminal-reason selection in the session flow by
adding a shared failure-reason helper next to emitTerminal, with
active.expiryReason taking precedence, followed by the response-limit condition,
then active.timedOut. Use this helper in both the success and catch paths so
invoke outcomes emit the same reason, and replace the duplicate
responseBytes/responseLimitExceeded checks with the shared condition while
preserving terminal emission behavior.
- Around line 262-267: Update disconnectTurn to require and validate the
request’s grant before aborting the matching active session, using the existing
authorization mechanism and returning without throwing when authorization fails.
Preserve the current session and voiceSessionId checks, and ensure the gateway
passes the grant through the close-handler cleanup path.
In `@test/voice-gateway-integration.test.ts`:
- Around line 43-52: The test file contains two conditionals that violate the CI
guardrail. In test/voice-gateway-integration.test.ts lines 43-52, update
FakeOpenClawSocket.send to select chat fixture events through a ternary-backed
chatFixtureEvents(sessionKey) helper and iterate the resulting array, which is
empty for non-chat.send methods; in lines 168-171, replace the
response.text.delta conditional with an events.filter(...).map(...) pipeline
when building runtimeOutput.
---
Nitpick comments:
In `@src/lib/adapters/fs/private-credential.ts`:
- Line 27: In the ancestor-walking logic around the uid ownership check, add a
concise comment explaining that encountering the first foreign-owned directory
intentionally stops traversal before applying group/other-writable checks,
preserving safe handling of root-owned paths such as sticky-bit /tmp; keep the
break after the symlink check and do not change the behavior.
In `@src/lib/adapters/http/voice-gateway.ts`:
- Around line 155-238: Split the inline request handler into focused route
functions: extract session creation into handleCreateSession, deletion into
handleDeleteSession, and streaming into handleTurn, preserving their existing
authentication, validation, session-service, response, and error behavior. Keep
the main handler responsible only for path dispatch and shared error handling,
and reuse the existing routing symbols and dependencies rather than changing the
API contract.
In `@src/lib/adapters/openclaw/voice-agent-client.test.ts`:
- Around line 233-251: Update the oversized payload setup in the test’s
queueMicrotask message loop to import and derive each deltaText length from
VOICE_MAX_RESPONSE_TEXT_BYTES, using a value that keeps one frame below the
limit while two frames exceed it and remain below MAX_NATIVE_FRAME_BYTES. Remove
the hardcoded 1_100_000 size.
- Around line 143-199: Strengthen the terminal-frame test around
OpenClawVoiceAgentClient.invoke by adding a pre-terminal delta with text, then
assert onText is called exactly once with only that text after the final frame;
retain the late delta to verify it is ignored. Add a separate scenario that
delivers the post-terminal delta after the chat.send response establishes the
run id, covering the listener-removal path in finish in addition to the
queued-drain path.
In `@src/lib/adapters/openclaw/voice-agent-client.ts`:
- Around line 73-83: Refactor invoke in OpenClawVoiceAgentClient by extracting
consumeChatFrame and onMessage into a small private turn-reader type that owns
expectedRunId, lastSequence, projectedText, terminal, queuedChatFrames, and
queuedChatBytes. Keep invoke responsible only for connecting, wiring listeners,
and sending chat.send, while preserving the existing frame-processing behavior
and experimental path semantics.
In `@src/lib/domain/voice/session-service.ts`:
- Around line 269-297: Update startTurn to be async so validation failures from
authorize and requireBoundedString, along with duplicate or active-turn checks,
are returned as promise rejections consistent with its Promise<void> contract.
Update the affected unit-test assertions to await expect(...).rejects rather
than asserting synchronous throws.
- Around line 146-159: Update requireBoundedString to accept an explicit options
parameter, such as rejectBlank, instead of using the label value to control
whitespace validation. Apply the blank-rejection check from that option, update
the text call site to pass rejectBlank: true, and prefix any intentionally
unused label or parameter with an underscore.
In `@test/internal-commands-docs.test.ts`:
- Around line 102-110: Update the test around renderedCommandReferences to
locate the OpenClaw reference by its name rather than assuming it is at index 0.
Assert the experimental voice gateway invocation is present in that named
reference, then check that all other references do not contain it.
In `@test/voice-gateway-integration.test.ts`:
- Around line 176-177: Update FakeOpenClawSocket.send to record each
invocation’s method alongside its params, then locate the chat.send entry by
method before asserting its scopes, message, and deliver fields. Remove the
positional invocations[0]/invocations[1] assumptions while preserving the
existing expectations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6b0491f2-114c-4bc5-b998-a27e4c474ab4
📒 Files selected for processing (18)
ci/source-architecture-budget.jsondocs/reference/commands.mdxsrc/commands/internal/README.mdsrc/commands/internal/voice-gateway/serve.tssrc/lib/actions/voice-gateway/serve.test.tssrc/lib/actions/voice-gateway/serve.tssrc/lib/adapters/fs/private-credential.test.tssrc/lib/adapters/fs/private-credential.tssrc/lib/adapters/http/voice-gateway.test.tssrc/lib/adapters/http/voice-gateway.tssrc/lib/adapters/openclaw/voice-agent-client.test.tssrc/lib/adapters/openclaw/voice-agent-client.tssrc/lib/domain/voice/session-service.test.tssrc/lib/domain/voice/session-service.tstest/internal-cli.test.tstest/internal-commands-docs.test.tstest/package-contract/cli/oclif-metadata.test.tstest/voice-gateway-integration.test.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Summary
Add the first experimental voice-gateway capability slice: one authenticated local runtime can create an expiring session and stream one committed text turn through a NemoClaw-owned OpenClaw boundary. The service remains hidden, foreground-only, loopback-only, OpenClaw-specific, and outside supported VoiceClaw or general voice claims.
Related Issue
Fixes #8378
Parent: #8275
Changes
nemoclaw internal voice-gateway serverouting behind the exactNEMOCLAW_EXPERIMENTAL_VOICE_GATEWAY=1gate.operator.read/operator.write, bounded inputs/queues/responses/deadlines, exact session/run correlation, duplicate and overlap rejection, cleanup, and content-free diagnostics.Type of Change
Quality Gates
0eb18814bf378105a96259277d2aad77b6127a10with no findings after credential-path, OpenClaw protocol, queue, response, duration, and backpressure hardening.Documentation Writer Review
docs-updateddocs/reference/commands.mdx;src/commands/internal/README.mdDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailableorigin/main.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Focused behavior tests,npm run build:cli,npm run typecheck:cli,npm run checks:repository,npm run validate:pr,npm run docs, productionnpm audit, andgit diff --checkpassed. Full live runtime validation remains for CI/E2E selection.npm run docsbuilds without warnings (doc changes only) — build passed with 0 errors; two existing Fern warnings remain.Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
New Features
Documentation
Tests