feat(diagnostics): add the managed-transport failure contract - #8725
feat(diagnostics): add the managed-transport failure contract#8725JulienAu wants to merge 11 commits into
Conversation
First slice of the correlated outbound diagnostics facility: a shared managed_transport_failure event schema with seven connection-ordered phases, an allowlist-only builder, a safe error-cause-chain walker that drops free-text messages, credential and query stripping for endpoint references, a bounded content-type-aware error-body snippet, a strict response-header allowlist, deterministic phase classification over the undici and socket vocabulary, and failure-only key=value emission with a generated correlation id. A non-MCP webhook consumer test proves the contract is reusable beyond the first integration. Negative tests cover every forbidden field class. Refs NVIDIA#7957 Signed-off-by: JulienAu <16043912+JulienAu@users.noreply.github.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:
📝 WalkthroughWalkthroughAdds a managed transport diagnostics module. It defines sanitized failure records, classifies transport phases, builds traceable failure events, emits stable output, and tests managed transport and webhook usage. ChangesManaged transport diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WebhookConsumer
participant classifyTransportPhase
participant buildManagedTransportFailure
participant emitManagedTransportFailure
participant stderr
WebhookConsumer->>classifyTransportPhase: transport failure context
classifyTransportPhase-->>WebhookConsumer: failure phase
WebhookConsumer->>buildManagedTransportFailure: failure input
buildManagedTransportFailure-->>WebhookConsumer: sanitized failure event
WebhookConsumer->>emitManagedTransportFailure: failure event
emitManagedTransportFailure->>stderr: stable diagnostic lines
Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/diagnostics/managed-transport.ts`:
- Around line 94-106: Update the cause-chain traversal loop in the managed
transport serializer to count every visited object independently of
chain.length, including empty cause objects. Stop traversal after
MAX_CAUSE_DEPTH hops while preserving the existing seen-cycle protection and
retained SafeTransportCause entries.
- Around line 147-171: Extend classifyTransportPhase with explicit
proxy-connection context and check it before the existing app-connect error-code
matching, returning "proxy_connect" for proxy failures. Update callers to
provide that context when applicable, and add coverage for proxied timeout or
refusal while preserving direct connection classification as "app_connect".
- Around line 118-123: In src/lib/diagnostics/managed-transport.ts:118-123,
replace raw allowlisted response-header values with bounded classifications or
presence indicators, retaining identifier values only when they match a safe
grammar; reject or encode whitespace and control characters. In
src/lib/diagnostics/managed-transport.ts:99-103, constrain or omit arbitrary
cause fields such as name, code, and syscall before they reach diagnostics. In
src/lib/diagnostics/managed-transport.ts:236-238, ensure dynamic values passed
to push cannot introduce additional key=value fields or log lines, and add
adversarial coverage for tokens and line breaks in allowed headers and cause
values.
🪄 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: 0236d64f-a048-4015-ba55-e6c7d59f54dc
📒 Files selected for processing (2)
src/lib/diagnostics/managed-transport.test.tssrc/lib/diagnostics/managed-transport.ts
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None 1 optional E2E recommendation
Blockers
|
The advisor's PRA-1 blocker is right: the formatter wrote allowlisted header values and required string fields as raw key=value text, so a delimiter- or credential-bearing upstream value could forge a second record or leak. Route every emitted string through encodeLogField, which redacts through the shared trace sanitizer, bounds length, and JSON-quotes any value carrying a control character, whitespace, quote, or separator. Add a formatter test with newline, CRLF, forged-field, and credential-shaped values proving one-record, no-leak output. Refs NVIDIA#7957 Signed-off-by: JulienAu <16043912+JulienAu@users.noreply.github.com>
|
Fixed the PRA-1 blocker in 0daaa57. Every emitted string field now goes through a single Added a formatter test that builds the event with a newline-plus- |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/diagnostics/managed-transport.ts`:
- Around line 288-290: Update the errorBodySnippet formatting in the managed
transport failure emitter to pass event.errorBodySnippet through the shared
encodeLogField policy instead of JSON.stringify, preserving the existing
conditional and output field. Add a direct-emitter test using a
credential-shaped oversized snippet that verifies redaction and the
256-character limit.
🪄 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: baf7a18b-3c70-483e-8472-833ab4d83468
📒 Files selected for processing (2)
src/lib/diagnostics/managed-transport.test.tssrc/lib/diagnostics/managed-transport.ts
Addresses the advisor's PRA-2 blocker: redaction happened only in the line formatter, so an alternate consumer serializing the built event could disclose a credential embedded in an operation, route, consumer name, or allowlisted header. Redact every untrusted string field in buildManagedTransportFailure so the returned object is safe by construction; emission encoding stays as defense in depth. Also select the first cause-chain code for the top-level cause_code so a wrapped transport error surfaces its network code (advisor PRA-1 correctness). Refs NVIDIA#7957 Signed-off-by: JulienAu <16043912+JulienAu@users.noreply.github.com>
|
Addressed both findings from the latest advisor pass in 007583c:
16 tests pass in normal and shuffled order; lint, typecheck, and repository checks are green. The terminology decisions need no change. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/diagnostics/managed-transport.ts`:
- Around line 203-225: Update buildManagedTransportFailure to pass an explicitly
supplied input.traceId through redactField before returning it, while preserving
generateTransportTraceId() for missing IDs. Extend the built-object
serialization test to verify caller-provided trace IDs are redacted.
🪄 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: e7d12b6e-8b68-4e9c-9f99-7b3dcd77d8f0
📒 Files selected for processing (2)
src/lib/diagnostics/managed-transport.test.tssrc/lib/diagnostics/managed-transport.ts
Addresses the advisor's PRA-3 blocker and trace-id warning: copied error names, codes, and syscalls are now bounded identifier tokens (anything longer or carrying other characters becomes <invalid>), and a supplied trace id is kept only when it matches the safe id shape, falling back to a generated one otherwise. The built event stays safe to serialize regardless of what an upstream library or caller puts in error metadata. Builder tests cover credential-shaped and delimiter-bearing cause fields and trace ids. Refs NVIDIA#7957 Signed-off-by: JulienAu <16043912+JulienAu@users.noreply.github.com>
|
Two things. First, the advisor's PRA-3 blocker and the trace-id and cause-code findings are addressed in 3106eba: copied error names/codes/syscalls are now bounded identifier tokens, a supplied trace id is validated at build time, and Second, and more important: the advisor's architecture warning led me to The two are not identical: the staged patch is JS injected into the OpenClaw sandbox bundle (it cannot import I'd rather you decide the shape than force one. Options as I see them:
Which do you prefer? |
wscurran
added
area: networking
|
Maintainer architecture decision required The author identified that merged PR #8204 already defines and ships the canonical OpenClaw managed-transport diagnostic event, with a different field set, phase vocabulary, and event naming. This PR proposes a separate in-process TypeScript contract for NemoClaw consumers. Two materially different directions remain:
The PR is not conclusively superseded: #8204 covers the injected OpenClaw runtime path, while this branch targets in-process NemoClaw consumers. Closing it now would require an architecture and scope judgment. I am therefore leaving it open, assigning the release target, and deferring implementation, security sign-off, approval, and merge until a maintainer chooses the contract direction. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/diagnostics/managed-transport.test.ts (2)
57-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact cause-chain depth bound.
Line 65 asserts
toBeLessThanOrEqual(8). That assertion also passes if the chain collapses to zero or one entry, so a regression that drops causes stays hidden.MAX_CAUSE_DEPTHis a bounded-output guarantee. Assert the exact length, and assert the retained entries.♻️ Proposed change
let deep: Record<string, unknown> = { name: "leaf" }; for (let index = 0; index < 20; index += 1) deep = { name: `n${index}`, cause: deep }; - expect(safeCauseChain(deep).length).toBeLessThanOrEqual(8); + const chain = safeCauseChain(deep); + expect(chain).toHaveLength(8); + expect(chain[0]).toEqual({ name: "n19" }); + expect(chain[7]).toEqual({ name: "n12" });Confirm that
MAX_CAUSE_DEPTHis 8 insrc/lib/diagnostics/managed-transport.tsbefore applying the diff.🤖 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/diagnostics/managed-transport.test.ts` around lines 57 - 66, Update the deep-chain assertions in the test "guards against cause cycles and unbounded depth" to verify exactly MAX_CAUSE_DEPTH entries, confirming that MAX_CAUSE_DEPTH is 8 in managed-transport.ts first. Assert the retained entries as well, preserving the expected newest-to-oldest cause order so regressions that drop causes are detected.
291-301: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that
phase=appears exactly once.The comment on line 296 states that the genuine
phasefield "appears once as a real top-level field". The assertion on line 297 only proves presence. If the encoder stopped quoting the injected operation value, the line would contain bothphase=response_headersand a forgedphase=policy, and the test would still pass. This is the central claim of the log-injection test.♻️ Proposed change
- expect(line).toContain("phase=response_headers"); + expect(line).toContain("phase=response_headers"); + expect(line.match(/(?:^| )phase=/g)).toHaveLength(1); + expect(line).not.toContain(" phase=policy");🤖 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/diagnostics/managed-transport.test.ts` around lines 291 - 301, Update the assertions in the managed transport test around the `phase=response_headers` check to verify that the `phase=` token occurs exactly once in the encoded line, while preserving the existing checks for the quoted operation and request ID values.Source: Path instructions
🤖 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/diagnostics/managed-transport.test.ts`:
- Around line 236-269: Remove the duplicate schema assertions in the
emitManagedTransportFailure test, or update them to match the shipped
managed_transport_failure helper’s contract. Use the runtime helper’s emitted
fields and event format as the source of truth, eliminating unsupported fields
such as phase, trace_id, and cause_chain from this test.
---
Nitpick comments:
In `@src/lib/diagnostics/managed-transport.test.ts`:
- Around line 57-66: Update the deep-chain assertions in the test "guards
against cause cycles and unbounded depth" to verify exactly MAX_CAUSE_DEPTH
entries, confirming that MAX_CAUSE_DEPTH is 8 in managed-transport.ts first.
Assert the retained entries as well, preserving the expected newest-to-oldest
cause order so regressions that drop causes are detected.
- Around line 291-301: Update the assertions in the managed transport test
around the `phase=response_headers` check to verify that the `phase=` token
occurs exactly once in the encoded line, while preserving the existing checks
for the quoted operation and request ID values.
🪄 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: 1af6305e-5243-4642-bd0c-cb0e42baf028
📒 Files selected for processing (2)
src/lib/diagnostics/managed-transport.test.tssrc/lib/diagnostics/managed-transport.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/diagnostics/managed-transport.ts
…ippet Addresses both advisor warnings on the managed-transport failure contract. PRA-1: buildManagedTransportFailure accepted an error body with no condition on the HTTP status, while boundedErrorBodySnippet documents non-2xx capture only. A caller could therefore attach the body of a successful response to failure diagnostics and retain response content the module promises not to keep. The snippet is now carried only when httpStatus is present and outside 200-299. An absent status is not treated as a failure status either: a transport error that never produced a response has no body to capture. PRA-2: the emitter serialized errorBodySnippet with JSON.stringify while every other field went through encodeLogField. JSON.stringify quotes and escapes, so it prevented record forging, but it neither redacts nor bounds. An event object constructed directly rather than through the builder could therefore carry a credential-bearing snippet straight into diagnostic output, bypassing the build-time redaction boundary. The snippet now goes through encodeLogField like everything else. encodeLogField takes an optional bound so the snippet keeps its documented 512-character limit rather than being truncated to the 256-character field limit. Two regression tests, both verified to fail without their fix: a builder test asserting a 200 response with a textual body yields no snippet and does not serialize its content, and an emitter test passing a hand-constructed event whose snippet carries a bearer token and a forged event line, asserting the credential is absent, the record count is unchanged, and no newline escapes. 20 tests pass in normal and shuffled order. Biome check and the src typecheck are clean; the two pre-existing banner.ts errors about an unbuilt nemoclaw/dist artifact reproduce identically on an unmodified tree. Signed-off-by: JulienAu <16043912+JulienAu@users.noreply.github.com>
|
Both advisor warnings from the latest pass are addressed in e14b0c4. Neither blocked, but one is a real gap in the redaction boundary, so I would rather not leave it open while the architecture question is pending. PRA-2 (security). The emitter serialized PRA-1 (correctness). Both regression tests were verified to fail without their fix, not merely to pass with it: reverting either change alone turns exactly one of them red. The builder test asserts a 200 response with a textual body yields no snippet and does not serialize its content; the emitter test passes a hand-constructed event whose snippet carries a bearer token and a forged event line, then asserts the credential is absent, the record count is unchanged, and no newline escapes. 20 tests pass in normal and shuffled order. Biome check and the On the architecture decision itself: nothing here presumes an outcome. The encoding policy and the body-capture restriction hold whichever direction you choose, including a rework onto the canonical schema from #8204, since both are properties of how values are emitted rather than of the field set. Still happy to close this as superseded if the in-process surface is not wanted yet. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Security review completed for revision No blocking findings were identified across the repository's nine review categories. The follow-up keeps supplied correlation IDs only when they match the allowed shape and the shared sanitizer leaves them unchanged; credential-shaped values are replaced before the event can be serialized. Explicit proxy-connection context now maps to The change adds no credential custody, authentication, file-permission, subprocess, dependency, SSRF, or network-policy surface. Focused validation passed: 21 tests, CLI typecheck, repository hooks, and secret scanning. The documentation writer found no user-facing documentation change because this module still has no production importer. |
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Architecture/product-scope blocker on latest PR commit 4783135b5.
This adds a second managed-transport diagnostic contract while the shipped OpenClaw helper from #8204 remains the production source of truth with incompatible event fields and phase vocabulary. The new 349-line module also has no production importer, so merging it now creates two contracts without completing a migration.
Please choose one canonical direction before landing this slice: either adapt and contract-test the shipped schema for in-process consumers, or migrate the existing production helper and consumer to this schema in the same change. The maintainer architecture comment on this PR records the same unresolved choice; the security review passing does not resolve that product decision.
|
The canonical-direction question from the architecture review now has a concrete answer in #8964: the contract adopts the dist patch's documented wire vocabulary ( |
|
Additional verified security finding on latest PR commit
The endpoint sanitizer must remove text through the final This security finding is independent of the existing architecture and product-scope blocker. Resolving it does not select the canonical contract direction. |
|
Large-change flag: this revision adds 779 lines. The canonical-schema decision remains unresolved. The latest security review also confirmed that endpoint values containing multiple |
6 participants
Summary
First slice of #7957: the reusable failure-only diagnostic contract for managed outbound transports. It defines the
managed_transport_failureevent schema with the seven connection-ordered phases from the issue, builds events through an allowlist-only constructor so authorization material, cookies, tokens, query strings, request bodies, and session identifier values can never reach an event, and emits stablekey=valuelines with a generated correlation id on failure only. A non-MCP webhook consumer test proves the contract is reusable beyond the first integration. The OpenClaw remote MCP dist-patch integration and the CLI/docs surface follow as the next two slices per the plan on the issue.Related Issue
Refs #7957 (sequential PR 1 of 3; claimed on the issue, prior interested commenter confirmed they are not working on it)
Changes
src/lib/diagnostics/managed-transport.ts:ManagedTransportFailureschema and phase type;buildManagedTransportFailure(allowlist-only; requirement: the issue's redaction constraints; consumers: the slice-2 MCP dist patch plus any managed outbound transport; a direct change is impossible because no shared contract exists today; protected by the negative tests below);safeCauseChain(keeps name/code/errno/syscall/port, deliberately drops free-text messages because undici embeds URLs and header values in them, cycle-guarded, depth-bounded);safeTargetRef(credential/query/fragment stripping for URL and non-URL endpoint strings; the URL parse path is restricted to http/https becausenew URL("user:secret@host")parses with schemeuser:and would leak the credential);pickSafeResponseHeaders(server, via, x-request-id, x-envoy-response-flags only);boundedErrorBodySnippet(512-char cap, textual content types only, defense-in-depth through the existingsanitizeTraceAttributes);classifyTransportPhase(deterministic over the undici/socket vocabulary);emitManagedTransportFailure(failure-only, injectable writer). Placement follows the issue's "observability/networking layer" wording; glad to move it besidetrace.tsif preferred, per the open question on the issue.src/lib/diagnostics/managed-transport.test.ts: 14 deterministic tests including one negative test per forbidden field class (authorization header, cookie, token in query string, URL userinfo, session identifier value), a log-injection test proving delimiter- and credential-bearing values cannot forge records, and the non-MCP example consumer required by acceptance criterion 6.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX 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 unavailable — rannpm run validate:pr; every hook passes except hadolint, which reports a pre-existing info-level finding (Dockerfile:1543 SC2015) that is byte-identical on unmodifiedmain; this PR changes no Dockerfilenpx vitest run --project cli src/lib/diagnostics/managed-transport.test.ts; 21 tests pass. CLI typecheck, repository hooks, source-shape checks, test-size checks, and secret scanning pass.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: JulienAu 16043912+JulienAu@users.noreply.github.com
Summary by CodeRabbit
Reliability
Security
Bug Fixes