feat(observability): add managed outbound transport diagnostics - #8204
Conversation
Emit a redacted, failure-only diagnostic that states which phase of a managed outbound request failed, and wrap the remote Streamable HTTP MCP fetch boundary as the first consumer. Correlation with OpenShell audit events is by endpoint and time; the pinned OpenShell OCSF objects carry no request-scoped identifier. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds a fail-closed OpenClaw patch for sandbox-gated, failure-only diagnostics on remote Streamable HTTP MCP requests. It integrates the script into image builds, adds validation coverage, and documents diagnostic fields, privacy limits, and audit correlation. ChangesManaged transport diagnostics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OpenClawBundleMcp
participant StreamableHTTPFetchBoundary
participant RemoteMCPServer
participant SandboxStderr
OpenClawBundleMcp->>StreamableHTTPFetchBoundary: invoke unchanged fetch
StreamableHTTPFetchBoundary->>RemoteMCPServer: send Streamable HTTP request
RemoteMCPServer-->>StreamableHTTPFetchBoundary: return response or transport error
StreamableHTTPFetchBoundary->>SandboxStderr: emit redacted failure diagnostic
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 |
laitingsheng
added
area: observability
|
🌿 Preview your docs: https://nvidia-preview-pr-8204.docs.buildwithfern.com/nemoclaw |
laitingsheng
added
enhancement
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 19d55c5 in the TypeScript / code-coverage/cliThe overall coverage in commit 19d55c5 in the Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
7 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 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: 7
🤖 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 `@docs/security/openclaw-2026.7.1-dependency-review.md`:
- Line 451: Align nemoClawMtdErrorBody and the corresponding shared-module
implementation so the final emitted error_body actually honors the documented
2048-byte bound after redaction; update nemoClawMtdRedact or the truncation
order as needed, and keep the documentation consistent with the resulting output
limit.
In `@scripts/patch-openclaw-managed-transport-diagnostics.mts`:
- Around line 168-174: Update the diagnostic flow around nemoClawMtdErrorBody
and nemoClawMtdEmit so the failing response is emitted and returned without
awaiting an unbounded body read. Bound the cloned response text read with a
timeout, clear the timer when the read settles, and attach the body only when it
completes within the limit; expose setTimeout and clearTimeout in the VM test
context so the helper is testable without keeping the process alive.
In `@src/lib/observability/managed-transport-correlation.ts`:
- Around line 48-50: Update the correlation logic around extractDeniedEndpoint
and the mode reporting near the correlation result so an absent failure.target
fails closed by returning no matched lines, or explicitly uses a time_only mode
when that behavior is intended. Preserve endpoint filtering for present targets,
and add coverage for an absent target with unrelated audit entries.
- Line 39: Update the correlation window handling in the managed transport
correlation implementation to reject or clamp non-finite, negative, and
oversized windowMs values, then enforce deterministic maximums for both retained
matching lines and their total formatted byte size across the collection and
formatting flow around the constructor and lines 44-58/80-83. Ensure truncation
is predictable and add coverage for high-volume matching audit input.
In `@src/lib/observability/managed-transport.ts`:
- Around line 186-187: Ensure error-body truncation uses one effective bound
after redaction: in src/lib/observability/managed-transport.ts lines 186-187,
update captureErrorBody to redact before applying MAX_ERROR_BODY_LENGTH; in
scripts/patch-openclaw-managed-transport-diagnostics.mts line 129, apply the
same order in nemoClawMtdErrorBody and adjust nemoClawMtdRedact at line 66 if
2048 is the intended body bound; update
docs/security/openclaw-2026.7.1-dependency-review.md line 451 to state the bound
enforced by the resulting code.
- Around line 291-293: The managed_transport_failure event uses inconsistent
response-header key spelling; standardize all emitters and documentation on
underscore-separated keys. In
src/lib/observability/managed-transport.ts:291-293, retain the
hyphen-to-underscore transform and add coverage for x-envoy-response-flags; in
scripts/patch-openclaw-managed-transport-diagnostics.mts:145-148, apply the same
transform in nemoClawMtdEmit; update the expected spelling in
test/openclaw-managed-transport-diagnostics-patch.test.ts:177-178 and revise the
sample and field list in docs/reference/troubleshoot-mcp-servers.mdx:111-112 and
:130.
In `@test/openclaw-managed-transport-diagnostics-patch.test.ts`:
- Around line 80-88: Update emittedEvent to remove its conditional branch while
preserving the existing key-value parsing behavior: only valid lines with a
separator after the first character should contribute entries. Use a linear
transformation such as filtering parsed entries before constructing the returned
Record, without adding another if statement.
🪄 Autofix (Beta)
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: 824a4669-c3c0-48b2-b10d-53b81f5fde19
📒 Files selected for processing (16)
Dockerfileci/full-e2e-cold-path-calibration.jsonci/source-architecture-budget.jsondocs/reference/troubleshoot-mcp-servers.mdxdocs/security/openclaw-2026.7.1-dependency-review.mdscripts/patch-openclaw-managed-transport-diagnostics.mtssrc/lib/actions/sandbox/exec-policy-hint-detection.tssrc/lib/observability/managed-transport-correlation.test.tssrc/lib/observability/managed-transport-correlation.tssrc/lib/observability/managed-transport.test.tssrc/lib/observability/managed-transport.tssrc/lib/sandbox/build-context.tstest/openclaw-dependency-review.test.tstest/openclaw-final-image-layout.test.tstest/openclaw-managed-transport-diagnostics-patch.test.tstest/sandbox-build-context.test.ts
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Reviewed commit 46a5728f8. The current failure path can block the caller indefinitely while awaiting the response error body. Bound that read by time as well as size, and ensure diagnostic collection cannot delay the original transport failure without a fixed limit.
The correlation path also needs a fixed line/byte evidence budget and must not correlate every audit event when target is absent. The shared and patched emitters must use one response-header field spelling, and the documented error-body bound must match the emitted value after redaction. Add tests for timeout, oversized audit windows, absent targets, and schema parity. Resolve the repository growth-guard failure without raising a budget for avoidable test control flow.
These are availability, evidence-integrity, and contract defects in the new supported diagnostic surface, so they block approval. CI is running after the maintainer update from main. The independent documentation-writer and sensitive-path reviews remain blocked and must cover the final commit.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/sandbox-build-context.test.ts (1)
135-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated managed-startup runtime-source path list into one constant.
The same 8-entry list of
src/lib/*paths appears three times: inwriteBuildContextFixture, inexpectStagedManagedStartupRuntimeSources, and inline in the test at lines 522-531. If a future change adds or removes one of these files in the Dockerfile COPY set, an update to only one or two of these arrays produces a false-passing test instead of a caught mismatch.Define one shared constant array (for example,
MANAGED_STARTUP_RUNTIME_SOURCES) near the top of the file and reuse it at each of these three call sites.♻️ Proposed refactor sketch
+const MANAGED_STARTUP_RUNTIME_SOURCES = [ + path.join("core", "json-types.ts"), + path.join("core", "ports.ts"), + path.join("onboard", "managed-bootstrap", "envelope.ts"), + path.join("onboard", "managed-bootstrap", "image-runtime.ts"), + path.join("onboard", "managed-startup", "image-runtime.ts"), + path.join("security", "credential-hash.ts"), + path.join("state", "paths.ts"), + path.join("state", "state-root.ts"), +]; + function writeBuildContextFixture(sourceRoot: string) { // ... - for (const relativePath of [ - path.join("core", "json-types.ts"), - path.join("core", "ports.ts"), - path.join("onboard", "managed-bootstrap", "envelope.ts"), - path.join("onboard", "managed-bootstrap", "image-runtime.ts"), - path.join("onboard", "managed-startup", "image-runtime.ts"), - path.join("security", "credential-hash.ts"), - path.join("state", "paths.ts"), - path.join("state", "state-root.ts"), - ]) { + for (const relativePath of MANAGED_STARTUP_RUNTIME_SOURCES) { writeFixture(path.join("src", "lib", relativePath)); }Apply the same substitution (with the
path.join("src", "lib", ...)prefix added where needed) at the other two call sites.Also applies to: 276-293, 522-533
🤖 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/sandbox-build-context.test.ts` around lines 135 - 146, Define a shared constant for the eight managed-startup runtime source paths near the top of the test file, then reuse it in writeBuildContextFixture, expectStagedManagedStartupRuntimeSources, and the inline test around the managed-startup runtime sources. Add the src/lib prefix at call sites that require full paths, ensuring all three locations derive their expectations from the same list.
🤖 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.
Nitpick comments:
In `@test/sandbox-build-context.test.ts`:
- Around line 135-146: Define a shared constant for the eight managed-startup
runtime source paths near the top of the test file, then reuse it in
writeBuildContextFixture, expectStagedManagedStartupRuntimeSources, and the
inline test around the managed-startup runtime sources. Add the src/lib prefix
at call sites that require full paths, ensuring all three locations derive their
expectations from the same list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8e513b0d-9ce4-4ed5-869f-cfb5515726ff
📒 Files selected for processing (5)
Dockerfileci/source-architecture-budget.jsonsrc/lib/sandbox/build-context.tstest/openclaw-final-image-layout.test.tstest/sandbox-build-context.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/sandbox/build-context.ts
- ci/source-architecture-budget.json
- test/openclaw-final-image-layout.test.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/openclaw-managed-transport-diagnostics-patch.test.ts (1)
100-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider replacing the fixed sleep with a polling wait.
settleDiagnosticswaits a fixed wall-clock duration. The diagnostic task is detached by design, so a wait is needed. A fixed duration couples the tests to real timer scheduling on shared CI runners.Line 291 waits 300 ms against the 250 ms body timeout. That margin is 50 ms. A loaded runner can exceed it and produce a flaky failure at line 292.
A poll that resolves as soon as
stderris non-empty removes the margin problem and also shortens the fast cases at lines 183, 267, 360, 380, and 396.♻️ Proposed polling helper
-function settleDiagnostics(delayMs = 20): Promise<void> { - return new Promise((resolve) => setTimeout(resolve, delayMs)); -} +async function settleDiagnostics(stderr: string[], timeoutMs = 1000): Promise<void> { + const deadline = Date.now() + timeoutMs; + while (stderr.length === 0 && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 5)); + } +}Call sites then pass the harness
stderrarray, for exampleawait settleDiagnostics(stderr);. Note that thewhileloop adds noifstatement, so the conditional guardrail stays satisfied.🤖 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/openclaw-managed-transport-diagnostics-patch.test.ts` around lines 100 - 102, Replace the fixed-delay implementation of settleDiagnostics with polling that accepts the harness stderr array and resolves as soon as stderr is non-empty, while retaining a short asynchronous delay between polls. Update all settleDiagnostics call sites to pass stderr, including the waits around lines 183, 267, 291, 360, 380, and 396, so the tests no longer depend on a fixed wall-clock margin.
🤖 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 `@docs/security/openclaw-2026.7.1-dependency-review.md`:
- Around line 472-473: Update the phase classification documentation to include
thrown failures with a response_headers classification, specifically the
UND_ERR_HEADERS_TIMEOUT path handled by nemoClawMtdPhase. Clarify that this case
represents a headers timeout where no response headers arrived and http_status
is absent, while retaining the existing non-2xx response_headers description.
---
Nitpick comments:
In `@test/openclaw-managed-transport-diagnostics-patch.test.ts`:
- Around line 100-102: Replace the fixed-delay implementation of
settleDiagnostics with polling that accepts the harness stderr array and
resolves as soon as stderr is non-empty, while retaining a short asynchronous
delay between polls. Update all settleDiagnostics call sites to pass stderr,
including the waits around lines 183, 267, 291, 360, 380, and 396, so the tests
no longer depend on a fixed wall-clock margin.
🪄 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: 46a5670e-ed2a-495a-9686-a656850b8915
📒 Files selected for processing (12)
Dockerfileci/source-architecture-budget.jsondocs/reference/troubleshoot-mcp-servers.mdxdocs/security/openclaw-2026.7.1-dependency-review.mdscripts/patch-openclaw-managed-transport-diagnostics.mtssrc/lib/observability/managed-transport.test.tssrc/lib/observability/managed-transport.tssrc/lib/sandbox/build-context.tstest/openclaw-dependency-review.test.tstest/openclaw-final-image-layout.test.tstest/openclaw-managed-transport-diagnostics-patch.test.tstest/sandbox-build-context.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- test/openclaw-final-image-layout.test.ts
- src/lib/sandbox/build-context.ts
- ci/source-architecture-budget.json
- src/lib/observability/managed-transport.ts
- Dockerfile
- src/lib/observability/managed-transport.test.ts
- test/openclaw-dependency-review.test.ts
- docs/reference/troubleshoot-mcp-servers.mdx
- test/sandbox-build-context.test.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head da3ace3058ac56ba07b3705bd7280eb267a611dc. The requested availability and evidence-contract changes are present: non-2xx body sampling is detached from the caller and bounded by a 250 ms absolute timeout plus a 2,048-byte read cap; the redacted UTF-8 value is capped at 2,048 bytes before JSON encoding; the unused unbounded audit-correlation utility and its absent-target behavior are removed; both emitters and docs use underscore-separated response-header fields; and tests cover the hanging body, multibyte/credential bound, shared schema, and header-timeout distinction. The avoidable test conditional was removed without increasing its guardrail budget.
I found no remaining blocker in the current diff. Required exact-head CI and automated review are still running and remain authoritative; this approval does not waive a terminal non-success check.
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head 5716022c72a021cbac3a0c5c6298b22228afa697. The follow-up makes the route field accurately report configuration evidence (proxy_configured/unknown) without claiming that a particular request traversed the proxy, updates the operator/security documentation to match, and removes the unused duplicate source schema so the injected shipped helper remains the tested runtime source of truth. The previously reviewed time/byte bounds, redaction, response-preservation, and failure-only behavior remain intact. Approved; exact-head CI and remaining maintainer review state still apply.
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Addressed at 19d55c5: response-body diagnostics are detached and bounded to 250 ms/2,048 bytes with redaction before the output bound; the unsafe unused correlation path was removed; response-header fields and documentation are consistent; focused regression coverage, growth guardrails, exact-head security/documentation review, both advisor lanes, required CI, cloud-onboard, and full-e2e all pass. Dismissing the obsolete changes request submitted on 46a5728.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
5 participants
Summary
OpenClaw 2026.7.1 reduces sandboxed remote Streamable HTTP MCP failures to errors such as
fetch failed. This change adds a sandbox-only, failure-only diagnostic at the actual OpenClawfetchboundary. It records the managed route, safe endpoint, failure phase through response headers, bounded redacted evidence, session presence, and a local diagnostic identifier without retrying or changing the request.This is the right interim solution for the observable fetch boundary: NemoClaw does not own the upstream source, and the repository already uses exact-version, fail-closed compiled-dist patches for reviewed OpenClaw compatibility. It is not the complete solution to #7957. The boundary cannot see the JSON-RPC operation or a failure while the caller consumes a successful 2xx body, and pinned OpenShell cannot carry the same request identifier in its audit event.
Related Issue
Partially addresses #7957.
Full resolution remains blocked by NVIDIA/OpenShell#2508. OpenShell 0.0.85 does not expose a request-scoped identifier in the available OCSF request or response objects, so application and audit events still require endpoint-and-time correlation.
Changes
scripts/patch-openclaw-managed-transport-diagnostics.mts, an exact-shape, fail-closed patch for the remote Streamable HTTPfetchboundary. The sibling SSE boundary remains unchanged.access_token,refresh_token, andclient_secret, safe response-header values, error bodies, and cause messages.transport_phaseclassification, and a distinct local 32-character hexadecimaldiagnostic_idfor each emitted diagnostic.The patch is intentionally scoped to OpenClaw 2026.7.1 and fails the image build if the reviewed compiled shape changes. Remove it when an audited OpenClaw release provides equivalent phase-classified, redacted fetch diagnostics.
Type of Change
Quality Gates
Documentation Writer Review
docs-updated. Independent review passed at the recorded head; code behavior, failure fallback, evidence bounds, route configuration evidence, exact-helper source of truth,transport_phaseanddiagnostic_idsemantics, header-timeout behavior, and correlation guidance agree with the documentation.docs/reference/troubleshoot-mcp-servers.mdx,docs/security/openclaw-2026.7.1-dependency-review.md;npm run docscompleted with 0 errors and 2 existing Fern warnings; OpenClaw agent variant includes the section and Hermes/Deep Agents variants exclude it; final focused follow-up tests passed 37/37.Verification
Signed-off-by:line and every commit appears as Verified in GitHubpre-commit,commit-msg, andpre-pushhooks passednpm run test:changedexited 0;npm run build:clipassed;npm run typecheck:clipassed; normal repository checks passed in hooksnpm run docsbuilds without errorsAdditional package verification applied the current MCP reliability patch and this diagnostic patch to the exact
openclaw@2026.7.1distribution, then passed diagnostic--auditandnode --check.Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Documentation
Bug Fixes