[afourniernv]

Summary

Join inspected HTTP responses, OpenShell security records and supervisor request
traces using the existing OpenShell-generated request ID. Include request roots
and children for stages that actually run, following the corrected review.

Related Issue

Closes #5393. Broader tracking: #5210. The tracing work previously split into
#5471 is included here; operator sampling configuration remains separate in #5362.

Changes

• Reuse the middleware UUID for the protected X-OpenShell-Request-ID receipt,
  OCSF metadata.correlation_uid, and the exported log attribute.
• Emit one primary HTTP Activity record when final response headers are
  accepted, with an early-failure fallback and distinct late exceptions.
• Add supervisor-owned request spans and actual stage children. Link workload
  traceparent as untrusted context, never as the supervisor's parent. Ignore
  invalid/duplicate context and do not adopt workload tracestate.
• Keep receipts, records and enforcement independent of tracing/export. Do not
  add denial-only spans for inspected requests. End HTTP tracing before an
  upgraded WebSocket session.
• Preserve raw HTTP bytes and receipt protections while borrowing unchanged
  response heads. Use typed, batched outcome updates and shared tracing fixtures.
  Name request-write/flush timing openshell.upstream.send.
• Integrate HTTP hooks from main f37687a2d. Preserve streamed-upload stage
  lifetimes, delayed empty-head/final-byte completion, generation checks and
  canonical receipts on the new response writer and failure paths.
• Finalize the existing request span after its owned stages, so an earlier
  header-log entry does not freeze its exported end time before the response.

One additive Rust OcsfEvent::set_correlation_uid method; tracing helpers are
internal. No new configuration, gateway RPC, protobuf/FFI change or Relay
dependency. A receipt joins records; it is not authorization or proof of
completed delivery. L4 and uninspected passthrough traffic are outside this
receipt contract. Upstream propagation remains separate.

Testing

• ad6040b80: 1,697 network, 156 middleware, 187 OCSF and 70 OpenTelemetry
  tests passed (2,110 total); two existing network tests ignored.
• Raw-header byte/borrowing tests, protected receipts, nested middleware RPC
  parentage, sampled/unsampled links, actual stage coverage and WebSocket timing.
• Full mise run pre-commit, including strict workspace and standalone
  Clippy, formatting, license and lockfile checks.
• Four focused integration guards, including actual DUPLEX upload, delayed
  empty-body response, typed failure and all six protected-header authorities.
• Independent review of the cleanup and current-main integration found no
  remaining actionable production issue.
• Completion-timing regression fails before the fix and passes afterward;
  parent/child end times stay nested without sleeps or new spans.
• Final integrated stack 6b81f8784: full mise run test passed unchanged
  (9,189 Rust tests, 94 auxiliary tests, Python 211 and TypeScript 206).
• Ten live native Hermes/Relay workflows using matching-source binaries:
  12 turns, 37 model calls, 276 native spans and 45 receipt/OCSF joins. Protected
  fixtures and authenticated read-only GitHub were exercised. Two workflows
  retained receipts and OCSF with supervisor trace export disabled.

Live validation is cumulative on the final stack, not isolated-layer E2E.
Initial network timeouts, an unchanged macOS VM cleanup failure and incomplete
agent attempts remain in the evidence; isolated and final full reruns passed.
W3C links join separately owned traces, without rewriting parentage or claiming
one stitched tree. Full repository mise run ci is not claimed.

Checklist

• Conventional Commits and DCO sign-offs.
• Crate and published correlation documentation updated.
• Existing review history preserved; no force push.

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[sftimeless]

Hey, thanks for working on this.

The separation between correlation context and authorization identity is important.

I've been testing Keel as an external pre-execution authorization service through OpenShell's Supervisor Middleware, and this connects directly to an issue we encountered during our v0.1.3 qualification.

In that experiment, the adapter could associate RequestContext.request_id with a signed Keel permit, but the corresponding supervisor OCSF event did not carry that identifier. We documented the ALLOW/DENY examples in #4557.

I see this PR introduces OcsfEvent::set_correlation_uid and the OpenTelemetry correlation helpers, while leaving live HTTP enforcement and middleware integration for subsequent work.

For that next integration step, would the intended approach be to propagate the supervisor-generated RequestContext.request_id into metadata.correlation_uid on the actual middleware enforcement events, as proposed in #4557?

That would let an external authorization provider join:

• Its signed authorization decision and permit ID
• The middleware request ID
• OpenShell's observed enforcement result

It would also be useful to preserve that identifier across request and response evaluation, including denials and middleware failures.

I'm not suggesting that correlation alone proves exact-request binding or downstream effects. Those remain separate guarantees.

Happy to help validate the eventual runtime integration against our existing sanitized test cases if helpful.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[krishicks]

Written by Claude Code, reviewed by @krishicks

Superseded by this comment. The request spans can stay in this PR; what matters is that no span exists only because of a denial, and that nothing depends on spans being exported. The question below about the request ID join without a sampled span still stands.

A question about span volume. Every inspected request gets openshell.http.request and openshell.policy.evaluate, and the queued layers add more INFO children. Supervisors use the SDK's default parentbased_always_on sampler, because OTEL_TRACES_SAMPLER doesn't reach them, and an unsampled workload traceparent can't suppress these spans. So every inspected request is exported. Inspected denials also move from supervisor.egress.deny, which #4789 bounded with per-destination and process-wide budgets, to request spans with no bound.

We see supervisor traces as a tool for latency and errors, sampled like any other service, rather than a full record of every request. OCSF already provides that record, and request rates belong in metrics. Sampling for supervisors is tracked in #5362, and supervisor metrics in #5364. In the meantime, we'd like these spans not to assume they're always exported. In particular, could you confirm that the request ID join through metadata.correlation_uid and the response header works with no sampled span? Also, when supervisor.egress.deny gives way to request spans, the request spans should stay inside whatever bound #4789 provides until sampling exists.

[github-actions]

Compatibility review (advisory)

New or updated breaking changes; see the full report for all findings.

1. api-behavior — No explicit risk acceptance found

Successful HTTP FORWARD requests without an L7 route now emit a fallback HTTP Activity record instead of the existing Allowed/Success record with the requested path and process.

• Impact: OCSF consumers filtering action=Allowed or attributing requests through actor.process lose successful requests and their original request details.
• Action: Populate the correlated FORWARD record with the existing action, status, path, process and endpoint fields before response-head emission.
• PR: feat(observability): correlate HTTP receipts, OCSF and request traces #5214

2. api-behavior — No explicit risk acceptance found

x-openshell-request-id previously passed through HTTP forwarding and allowed middleware mutations; the candidate strips incoming values, replaces upstream values and rejects mutations.

• Impact: Existing middleware writing or removing this header fails closed by default, while applications using it for end-to-end request correlation lose their identifiers.
• Action: Keep legacy header handling on v0.1.8 and make protected receipts opt-in, documenting a separate header for application-owned correlation.
• PR: feat(observability): correlate HTTP receipts, OCSF and request traces #5214

3. api-behavior — No explicit risk acceptance found

Inspected HTTP denials replace bounded supervisor.egress.deny spans with unbounded openshell.http.request and policy spans that can inherit workload traceparent.

• Impact: Existing deny-span queries and root-trace joins miss or regroup denials, while retry traffic bypasses the documented span budgets.
• Action: Keep the legacy deny-span name, parent behavior and budgets as the v0.1.8 default, making replacement tracing opt-in with migration guidance.
• PR: feat(observability): correlate HTTP receipts, OCSF and request traces #5214

Review details

• Execution: complete
• Candidate: PR #5214 (37a7bef8c689b4e3ea34b2f839fa2bd70f306472)
• Baseline: 9321d1632a41cd7337db38ff982164bff9120d08 (9321d1632a41cd7337db38ff982164bff9120d08)
• Reviewed: rust-sdk, python-sdk, go-sdk, typescript-sdk, api-behavior, cli, configuration, policy, helm, state

Advisory review; publication gates are unchanged.

Branch check run

[krishicks]

Written by Claude Code, reviewed by @krishicks

Reviewed at 7e8f7ae. Each point starts with what a user would see; the cause and file references follow. Line numbers refer to that commit.

Audit records

1. A SIEM shows requests the supervisor blocked as allowed. A request blocked by a middleware failure (403), admission exhaustion (503), a WebSocket preflight error (502), or a credential preparation failure (502) gets one HTTP Activity record that says Allowed, with no failure status.
   • Cause: emit_primary reclassifies the record only when the outcome is denied (l7/correlation.rs:212-220). The platform_error and upstream_error outcomes keep the allow record (relay.rs:900-904, relay.rs:1823-1855).
2. Allowed plain-HTTP proxy requests no longer say which process made them. On main, every allowed FORWARD request without L7 inspection is recorded with its path, binary, PID, and cmdline. With this PR, it gets a placeholder record: "HTTP request ended before a response was selected", URL /, no action, no process.
   • Cause: the correlation is created up front (proxy.rs:5921-5926), so the empty slot is emitted at the response head. The real allow record arrives after the relay (proxy.rs:7228) and is dropped (correlation.rs:174).
3. Requests rejected before policy runs are reported as policy denials. An Upgrade refusal, a malformed request target, or an invalid GraphQL or JSON-RPC body produces "HTTP request blocked after policy admission", URL /, and a span with policy.decision=deny. Policy never ran on these requests, so an operator investigating policy is sent to the wrong place.
   • Cause: these paths emit only a Network Activity record, so the placeholder record is emitted and relabelled as a denial (relay.rs:1310-1335, relay.rs:1550-1665, relay.rs:3344-3348).
4. Some requests still produce extra HTTP Activity records that can't be joined by request ID, and one produces two records that disagree. This happens to any request with response middleware, a request-middleware denial, or a token-grant refusal. A token-grant refusal gives one record saying Denied and another saying Allowed with a 502. Denials on these paths also open a separate supervisor.egress.deny span next to the request span.
   • Cause: these paths still emit through telemetry::emit_decision without correlation_uid (http_response.rs:698-1251, middleware.rs:1023, token_grant_injection.rs:344). The request ID is already available at the call sites (relay.rs:1810, relay.rs:2655, relay.rs:3138).
5. When credential resolution or MCP reinspection rejects a request, the record no longer shows which rule matched. The surviving record loses the request path, policy rule, and reason. A problem after the response headers are sent produces no correlated record at all.
   • Cause: emit() replaces the primary record with any later HTTP Activity, and silently drops one that arrives after the head is written (correlation.rs:166-176).

Trust and sampling

6. A sandboxed workload decides which trace ID appears on supervisor audit records. If it sends a sampled traceparent, the HTTP Activity record's trace.uid is the workload's trace ID. A workload could point its denials at an unrelated trace. Once #5362 adds a sampling ratio, it could also force its request spans to be exported regardless of that ratio.
   • Cause: a sampled workload context becomes the parent of openshell.http.request (propagation.rs:169-174). #4789 documents that workload headers never supply supervisor correlation, and #2640 keeps workload context in a separate field.
   • Linking sampled context the way unsampled context is already linked would keep the connection to the agent's trace without either effect.

Behavior changes beyond correlation

These change what clients see. Could they move to a separate PR, or at least be listed in the PR description?

7. Agent HTTP clients stop retrying, so a routine connection close becomes a failed request. When an upstream closes an idle keep-alive connection, main closes the client connection. Go, urllib3, and curl retry that automatically. With this PR the client gets a 502 and surfaces the error.
   • Cause: a synthetic 502 on upstream EOF before headers (http_response.rs:100-110).
8. Some upstream APIs that work today stop working. For example, a server that sends HTTP/1.1 200 without a reason phrase now has its response rejected, and the agent sees the connection close. The audit record still reports a 502.
   • Cause: new strict response-head validation that applies whether or not a request is correlated (http_response.rs:425-440). httparse accepts a missing reason phrase.
9. Agents get a 500 response where the connection used to close. This happens on a missing credential or a stale credential generation.
   • Cause: relay.rs:1064-1080, proxy.rs:6851-6880.

Spans

10. Traces don't show where an inspected request's time went. Only policy evaluation appears under openshell.http.request. Middleware calls, credential preparation, and the upstream dial are not children of the request span, and remote middleware receives the connection's trace context instead of the request's.
11. WebSocket requests show up as a single span lasting the whole session. The root span stays open until the upgraded connection closes.
12. The span marks an error but not which stage failed. record_error_stage ignores its stage argument (correlation.rs:162).

Tests

Most of the record problems above would have been caught by a test asserting exactly one HTTP Activity per request. The dispatch tests use .find() instead. These paths have no coverage:

• placeholder records and rejections before policy runs
• the content of a non-L7 FORWARD record
• token-grant refusal
• request-middleware denial
• upstream EOF before headers
• a second request on a keep-alive connection, which should get a new ID

Process

The branch conflicts with main.

[krishicks]

This is what the robot drew when I asked for a before/after with this PR:

[krishicks]

Written by Claude Code, reviewed by @krishicks

Superseded by this comment. The request spans don't need to wait for #5362, and the diagram below is out of date.

A follow-up connecting the review with my earlier comment on span volume. The compatibility review's item 3 raises the same concern: span volume. Review point 10 asks for a child span per stage, which adds spans. Nothing bounds that until #5362 adds supervisor sampling, and the #4789 deny-span limiter isn't the right tool, since #5362 removes it.

Could the request spans land with or after #5362? This PR would keep the request ID, correlation_uid, the response header, and the primary record. Those don't depend on spans. That also needs the request ID join to work with no sampled span, which the earlier comment asked you to confirm.

Then the after diagram would look like this:

[HarryMoss]

Drafted with Codex; reviewed and approved by @HarryMoss.

Hi @afourniernv, I’ve read Kris’s latest review and the proposed split between the correlation core and request spans.

My local #2640 connection-startup prototype is based on #5214 at 7e8f7aec and touches the shared CONNECT/proxy and telemetry paths. I’ll keep the L4 span work local pending #5362.

Once you’ve settled the split, which revision should I track for those shared paths so I can avoid duplicating your fixes?

Context: #2640 (comment)

[krishicks]

Drafted by 🤖, signed off by @krishicks

A correction to my last comment: the request spans don't need to wait for #5362. They can stay in this PR, along with per-stage children (review point 10). The concern is spans that exist only because a denial happened. A request span that records every outcome is the right shape.

Two requirements apply whether or not sampling exists:

• No span exists only because of a denial. On inspected paths, that means no supervisor.egress.deny span next to the request span for middleware denials, response-middleware blocks, or token-grant refusals (review point 4).
• Nothing depends on spans being exported. Records, enforcement, and the request ID join have to work the same whether a request's span is sampled or not.

The updated diagram:

[HarryMoss]

Drafted by Codex; checked by @HarryMoss.

Thanks @afourniernv for the update. I reviewed b595d70 against @krishicks’s latest guidance. The supervisor-owned roots, untrusted workload links and sampling-independent receipt joins address those design points.

Thirty-five focused controls passed locally. Two additional regression tests reproduced cases that may be worth checking:

• A response declaring Content-Length: 4 but sending one byte correctly produces a relay error, but the request span finishes as allowed without error status. Could the terminal trace reflect the transfer failure while preserving the primary record’s admission meaning?
• A body-credential refusal returns a 403 receipt and correlated HTTP Activity, but the companion Network Activity and credential finding lack metadata.correlation_uid. Looking up the receipt misses the finding containing the specific refusal reason. Could those companions use the existing request correlation scope?
  I also confirmed conflicts with main’s middleware changes in feat(middleware): introduce improved HTTP request/response hooks for the middleware #4359. I can share the small regression tests if useful.

[github-actions]

Compatibility review (advisory)

New or updated breaking changes; see the full report for all findings.

1. api-behavior — No explicit risk acceptance found

Inspected HTTP requests previously emitted per-middleware HTTP Activity records; consolidation now drops routine invocation records and their details.

• Impact: Existing OCSF consumers lose middleware attribution, transformation flags, and response-stage outcomes needed to audit successful processing.
• Action: Preserve per-invocation records and fields on v0.1.9; defer consolidation to a minor release with an actionable audit-query migration.
• PR: feat(observability): correlate HTTP receipts, OCSF and request traces #5214

3. api-behavior — No explicit risk acceptance found

Inspected denials replace bounded supervisor.egress.deny spans and policy attributes with openshell.http.request spans outside the legacy limits.

• Impact: Existing denial-span queries miss inspected denials, and collector capacity assumptions based on documented destination and process budgets no longer hold.
• Action: Preserve legacy denial-span names, attributes, and bounds as patch defaults, or defer replacement to a minor release with query and capacity migration guidance.
• PR: feat(observability): correlate HTTP receipts, OCSF and request traces #5214

Review details

• Execution: complete
• Candidate: PR #5214 (0bef4c745c4e8064e4229c99567b9c81a8a011ad)
• Baseline: f37687a2deb0ffb92122c785da835ca091aa5e55 (f37687a2deb0ffb92122c785da835ca091aa5e55)
• Reviewed: rust-sdk, python-sdk, typescript-sdk, api-behavior, cli, configuration, policy, helm, state

Advisory review; publication gates are unchanged.

Branch check run

[HarryMoss]

Drafted by Codex; @HarryMoss reviewed

Thanks @afourniernv for the updates. I reran the two regressions from my earlier review on ad6040b8. Twenty-nine focused existing controls passed; both added regressions still reproduce:
Thanks @afourniernv for the updates. I reran the two regressions from my earlier review on ad6040b8. Thirty-nine focused existing controls passed, including the real FORWARD sampling/export matrix and WebSocket trace-lifetime checks; both added regressions still reproduce:

• The truncated Content-Length response returns the expected relay error, but its request span finishes as allowed, with no error status.
• The body-credential refusal returns its 403 receipt, but the companion Network Activity and credential finding still lack metadata.correlation_uid.

I have the small test-only patch ready to share. This is local macOS evidence, rather than a fresh deployed validation.

The compatibility advisory also matches what I see in the successful middleware merge path: routine invocation details are dropped rather than retained on the primary record. Would you prefer retaining those audit details, or documenting that loss as an explicit compatibility decision? That question follows Kris's guidance that audit records and joins work independently of trace export.