Trace exporter follow-ups from the PR #244 review

The review of PR #244 turned up these, and none of them blocked the merge, so they're collected here so they don't get lost. Each one was confirmed by running it during the review.

Exporter (packages/guardian/src/otel-trace-exporter.ts):

A failed output stays off for the life of the process. isolated() never resets dead, so a collector that restarts for longer than 2 s ends endpoint export for good, with one stderr line. Retry with a backoff and count the dropped spans.
The span starts before validation, so a request that fails validation can set the session and request ids on a span. Re-posting a real pair without a JSON-RPC id put a guardian_error event on that pair, which marks the real decision as not enforced. Either mark spans from invalid envelopes or keep their ids under separate keys.
ACS_OTEL_FILE follows a symlink, and an existing file keeps its mode (0644 stays 0644). Open with O_NOFOLLOW and tighten the mode, or refuse.
ACS_OTEL_ENDPOINT accepts plain http:// to a remote host, and the sample collector's receiver has no auth. Warn on a non-loopback http endpoint and document TLS and headers.

Host adapter (packages/host-adapter/):

Audit entries carry no tool name, so converter spans lack gen_ai.tool.name, which the mapping requires on step spans. The sample trace for issue [RefImpl] Commit a sample OTLP trace from the reference Guardian, and test the exporter against it #266 has to account for that until it's fixed.
An unparseable Guardian response is filed under kind unknown, so it converts to other instead of malformed_response.
A malformed ask was denied with no audit entry, although issue [Bug]: acs.decision trace event cannot carry reason_codes, policy_references, cited_provenance_ids, or chain_hash, so replay from the trace alone is not possible #228 says the Observed Agent records every decision failure.
The Claude Code shim sends no request_id_ref on steps/toolCallResult and no capability, so host-driven traces lack both.

Repo:

Dependabot has no bun ecosystem, so nothing proposes updates for the new @opentelemetry pins. That needs a .github/ change, which a project lead approves.

Two more from the PR #275 review, both for the exporter in PR #244 once PR #275 merges:

otel-trace-exporter.ts still says acs.decision.failure "is not in otel-mapping.json yet" and that the mapping has no attribute for the request id. PR Carry the decision's replay facts and decision failures on Trace #275 adds both, so read the failure event name and step_span_attributes from the mapping and drop those comments.
DECISION_DETAIL_NAMES exists so the exporter works before and after PR Carry the decision's replay facts and decision failures on Trace #275. Once it merges, the mapping covers those names, and the list can go.

The agt job (bun) is also not a required check on integration, so a red run there does not stop a merge.

@rocklambros

@rocklambros a few more from my pass on #244 that aren't in the list yet (checked by running it):

Extending your first item: FailureClass (otel-trace-exporter.ts:126) is also missing signature_invalid and handshake_failure from Carry the decision's replay facts and decision failures on Trace #275, so the class list should come from the mapping too.
The converter labels its spans service.name=acs-reference-guardian (audit-to-otlp.ts:202), though the host reported them. Since Carry the decision's replay facts and decision failures on Trace #275 asks readers to cross-check separately produced streams, it should have its own name.
The converter skips handshake failures; after Carry the decision's replay facts and decision failures on Trace #275 they should become handshake_failure on the acs.session span.
main.ts:78-79 uses process.once, so a second SIGTERM during a slow shutdown loses the last batch, and nothing tests the signal path.
Spans beyond the SDK's 2048-span queue are dropped with only a diag warning nobody installed.
audit-to-otlp.test.ts:242 takes ~19 s locally against bun's 5 s default (CI is green).
guardian/README.md:67 says the policy-version attributes are absent; they're emitted as [""].
Once fix(agt): implement ACS-Core wire safeguards #229 lands: its allow-by-default hooks aren't in methods_evaluated, so they get no span at all (otel-trace-exporter.ts:751). They need a span plus a marker, e.g. a not_evaluated reason code, so they don't read as policy allows.

More from a deeper pass on #244, checked against 8058752, most by running it. The four that matter before the lab handoff are in #276.

A hung file output is never marked dead, and pending writes grow without bound (otel-trace-exporter.ts:546-608).
close() drops in-flight connections, but the span still records a normal decision while the host records unreachable (server.ts:491).
Steps open at shutdown get other; when the host already timed out, timeout is the right class (otel-trace-exporter.ts:873-880).
The converter skips a host-side -32005 entry, so the earlier decision for that pair reads as enforced (audit-to-otlp.ts:257-260).
Tests: required attributes are only checked in the source table (a wrong acs.exit_status field passes), which [RefImpl] Commit a sample OTLP trace from the reference Guardian, and test the exporter against it #266's sample test will cover. Also, the "empty array is absent" rule is never exercised, the off-by-default test (:1042) can't fail, there's no end-to-end join test, and the converter's posture and parsing paths are unasserted.
Nits: payload_bytes counts decoded text (server.ts:577); the shared status sentence never matches between the two writers; elapsed_ms can equal timeout_ms; prototype-named kinds slip through (audit-to-otlp.ts:86); oversized error codes overflow int64; the same input and output path corrupts the audit log; README:85 should say "after the agent session has ended"; OTEL_BSP_* variables still apply in the exporter.

Thanks for collecting these. I wrote the exporter and the converter, so I’d like to take this issue end to end, in three PRs.

Failed outputs. The permanent dead flag, the hung file write and the silent drops past the span queue are one problem: an output fails or stalls, and nothing recovers or counts it. I’ll replace dead with a per-output breaker. It retries with backoff, caps pending writes, and keeps a dropped-span count that’s reported on recovery and at shutdown.
Hardening. Ids from invalid envelopes go under separate keys rather than marking the span, so the default join can’t pick them up. ACS_OTEL_FILE opens with O_NOFOLLOW and refuses a file wider than 0600. The http warning, the signal path and the shutdown class go here too.
Converter, tests and README, from Bar’s list.

Some of it has to wait. Reading names and classes from the mapping needs #275; the hook spans need #229. I’m leaving the host-adapter items until #229 lands, Dependabot to a lead, and the required-attribute check to #266.

Two questions. close() dropping in-flight connections overlaps with #112: should I wait for it, or fix it on the exporter side now? And is a stderr line enough for the dropped-span count, or do you want it in the trace?

From the review of #277 (checked by running it):

Before the handoff: README "Run it again" should say to stop the collector before rm -rf .acs. With it running, spans go to the deleted file with no error.
keepLoopbackOffProxy (otel-trace-exporter.ts:598) creates no_proxy when only NO_PROXY exists, and Bun reads no_proxy first, so a user's existing exemptions stop applying. Append to the variables that exist instead.

Commit a sample trace and a walkthrough anyone can run, and fix the c…

Keep a user's proxy exemptions, warn when loopback would reach a prox…

@sabasiddique1

@sabasiddique1, thank you for taking this on. You wrote the exporter and the converter, so you're the right person for it. I've assigned issue #274 to you and to me.

Some things changed since your comment, so here is the current state.

What is already done or unblocked

PR Carry the decision's replay facts and decision failures on Trace #275 merged two hours before your comment. You don't need to wait for it, and the items you deferred because of it are ready now.
PR Commit a sample trace and a walkthrough anyone can run, and fix the collector setup #277 and PR Keep a user's proxy exemptions, warn when loopback would reach a proxy, and stop everything before a rerun #279 already did these, so you don't need to do them:
- the converter's own service.name
- OTEL_BSP_*
- the converter refusing an existing output
- the README lines about policy versions and "after the agent session has ended"
- "Run it again"
- the NO_PROXY fix
The required-attribute check exists in test/sample-trace.test.ts.
Two items need repository admin rights, so I'll do them, not you: Dependabot for Bun (PR Give the AGT reference implementation Dependabot coverage #126), and making the agt check required.

Your three PRs, as three sub-issues

I split issue #274 into sub-issues, so each of your PRs can close its own part and issue #274 stays open until all of them are done.

Issue [RefImpl] Recover a failed or stalled trace output and count the spans it drops #282: failed and stalled outputs, and the dropped-span count. Start here. The reviewer is @artmaro.
Issue [RefImpl] Harden the trace exporter's file and endpoint handling, and the spans it vouches for #283: file and endpoint hardening, and spans the Guardian can vouch for. Start this after your first PR is in review. The reviewer is me.
Issue [RefImpl] Read the trace exporter's names from the mapping, and close its test gaps #284: names from the mapping, and the test gaps. The reviewer is @bar-capsule.

Issue #285 holds the host-adapter items. They wait for PR #229, and they are not yours yet.

Each sub-issue lists its scope and the tests that prove it. Please read them before you start. Some details changed after a review, especially these two:

The breaker must have a limit on memory. Spans in the batch queue are removed before export runs, so a retry needs its own buffer. If you keep one, limit it by count and by bytes, drop the oldest first, count what you drop, and never drain it during shutdown. One large request can make a span of about 0.5 MB, so issue [RefImpl] Recover a failed or stalled trace output and count the spans it drops #282 also asks for a byte limit per span.
For file permissions, tighten an existing file to 0600 instead of refusing it. PR Batch Guardian log writes and drain them on shutdown #112 keeps existing file modes, and a refusal would turn off the trace in the walkthrough.

Your questions

Question 1: should you wait for PR #112 before fixing close()?

No, don't wait. The fix is one change in server.ts, around line 491. Change server.stop(true) to server.stop(), inside the existing Promise.race. We tested it, and the in-flight response is delivered. Please send it as a small separate PR with a regression test, and don't change the exporter for this. I'll add a note on PR #112 that its rebase must keep the Promise.race.

Question 2: is a stderr line enough for the dropped-span count?

Yes, for now. Report the count per output:

at the first drop;
at most once per minute while drops continue;
when the output recovers;
in the shutdown line.

The trace is not the right place yet, for two reasons. The mapping has no attribute for this, so adding one is a Spec change. And when an output is down, a span about it can't reach that output either. If we want the gap inside the trace later, that is a proposal for the Spec leads.

How to write each PR

In the section "Which issue does this implement", write two lines: Closes #282 (or the sub-issue for that PR) and Refs #274.
Do not write Closes #274, because that would close the whole umbrella issue.
Keep each PR small: one sub-issue per PR, and about 400 changed lines of source, not counting tests. If a PR grows past that, split it.
Please open each PR as a draft early, so the reviewer can check the direction before you write all the code.
Please run the timer tests on Bun 1.3.11 as well, because CI uses that version.

One request

Please leave a short comment on issue #282. GitHub only lets me assign you to an issue after you have commented on it.

If anything here is unclear, ask on the sub-issue. I'll answer there.

@rocklambros

Thanks @rocklambros, this is clear. Noted on the bounded retry buffer and on tightening to 0600 rather than refusing. I’ll send the close() fix as its own small PR with a regression test, and start #282 alongside it.

Address Ariel's review of the trace exporter (#289)

Navigation Menu

Repository navigation

Trace exporter follow-ups from the PR #244 review #274

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Trace exporter follow-ups from the PR #244 review #274

Description

Activity

rocklambros commented on Oct 14, 2026

bar-capsule commented on Oct 15, 2026

bar-capsule commented on Oct 15, 2026

sabasiddique1 commented on Oct 15, 2026

bar-capsule commented on Oct 15, 2026

rocklambros commented on Oct 15, 2026

sabasiddique1 commented on Oct 15, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions