docs(adr): specify Level 3 content capture activation and sandbox env denylist - #5947
docs(adr): specify Level 3 content capture activation and sandbox env denylist#5947dhshah13 wants to merge 5 commits into
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by QodoDefine Level 3 content capture activation contract and sandbox telemetry denylist
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Site previewPreview: https://a26c2f8a-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
dab0780 to
6752847
Compare
|
Both findings addressed in c7d43c3. Allow-list underspecified: correct — ADR 0082 governs mint workflow refs, so it cannot supply matching semantics for an OTLP endpoint allow-list, and leaning on it left a security gate ambiguous. ADR 0084 now defines them directly: comma-separated Reasoning-capture contradiction: correct that a reader of ADR 0050 alone would still expect reasoning text. Since 0050 is accepted and its Decision cannot be rewritten, its annotation now records the narrowing explicitly — Level 3 serves the LLM-judge case with prompts, messages, and tool activity, and extending scope to reasoning requires a new ADR. On the alternatives raised in the summary: full-URL allow-listing is worth revisiting if host-level granularity proves insufficient, but host-level matches the governance unit — sign-off is about which backend holds the content, not which path on it. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings (7 items, all MEDIUM severity) — see inline comments for details.
| activation semantics unspecified: which configuration enables it, what happens | ||
| on partial configuration, and where content may flow. Content is the | ||
| highest-sensitivity telemetry fullsend emits (proprietary source, PII, tool | ||
| output), and the sibling agentic-ci harness demonstrated the failure mode this |
There was a problem hiding this comment.
[MEDIUM] Unverified "sibling agentic-ci harness" incident cited as established fact
The Context section states as settled fact: "the sibling agentic-ci harness demonstrated the failure mode this gap invites: content capture enabled unconditionally by default." No other ADR, problem doc, or design doc in this repo documents or references this incident — the only other "agentic-ci" hits in the repo are unrelated UI-rendering comparisons. This is an external, unverified claim about another project's behavior used to justify the strictness of this contract, with no citation a reader could check.
Suggestion: Either link to where this was actually observed/discussed (an issue, incident writeup, or the agentic-ci repo itself), or soften the claim to something reviewable, e.g. "a comparable harness has shipped content capture enabled unconditionally by default, which is the failure mode this contract exists to prevent," with a source link.
| rather than degrading silently: | ||
|
|
||
| 1. **Operator opt-in (env):** `OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT` | ||
| set to `true` (or the equivalent `span_only`); `false` or unset is off, |
There was a problem hiding this comment.
[MEDIUM] Unverified span_only value for OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT
This states the operator opt-in env var accepts true "(or the equivalent span_only)" as if that is an already-established literal value per the upstream OTel GenAI semantic conventions v1.37.0 pinned by this ADR. The OTel GenAI instrumentation content-capture design documents a multi-mode enum (span-only/event-only/span-and-event style values), but that doesn't confirm this exact literal string ("span_only", this casing/spelling) is what this specific env var accepts — naming is known to vary by language/instrumentation and the spec is still evolving. ADR 0050 (which this builds on) only ever documented true/unset. The same claim is repeated verbatim in the operator guide, and this ADR states "any other value is a hard error," so an incorrect assumption here directly affects implementation behavior.
Suggestion: Before merging, verify against the actual OTel GenAI semantic-conventions spec/instrumentation source (pinned at v1.37.0) exactly which literal values this env var accepts (case and spelling). If span_only is not correct, drop the parenthetical or fix it; if correct, cite the source so implementers don't have to re-derive it.
| sensitivity varies by agent and repository, and the harness file's | ||
| CODEOWNERS review is the consent mechanism — which is why this knob lives | ||
| on the harness rather than `config.yaml` under | ||
| [ADR 0080](0080-config-yaml-vs-agent-env-var-scope.md)'s placement rule. |
There was a problem hiding this comment.
[MEDIUM] Citing ADR 0080's placement rule for a harness field shape ADR 0080 never defines
Condition 2 introduces telemetry.content_capture: true as a new structured harness field and justifies its placement by invoking "ADR 0080's placement rule." ADR 0080 draws a binary line between exactly two surfaces: a plain config.yaml field with no {AGENT}_ prefix (pipeline/dispatch policy), or an {AGENT}_-prefixed env var delivered via env.runner/env.sandbox per ADR 0049 (single-agent behavior tuning). telemetry.content_capture: true is neither — it's a structured, non-prefixed, non-env-var harness key, a third shape ADR 0080's decision text doesn't address (there's also no existing Telemetry/ContentCapture field precedent to fall back on). Citing ADR 0080 as though it already sanctions this field's exact placement stretches a rule that only covers two specific mechanisms.
Suggestion: Either scope the claim down ("a new harness field shape not covered by ADR 0080's two surfaces; chosen because...") and justify it directly, or use an {AGENT}_CONTENT_CAPTURE-style env var in env.runner/env.sandbox instead, which would let this condition ride the existing ADR 0080/0049 placement rule without stretching it.
| Content flows over OTLP export only. `run-telemetry.jsonl` keeps its | ||
| documented metadata-only contract, enforced by an attribute allow-list at the | ||
| file exporter. Captured content is assembled post-iteration from the | ||
| transcript and harness inputs into OTel GenAI semantic-convention aggregated |
There was a problem hiding this comment.
[MEDIUM] Full multi-turn content aggregated into one span attribute may hit backend/collector limits
The Decision commits to assembling "system prompts, user messages, per-turn assistant text, tool arguments, and tool results" (potentially file contents / command output) into a single OTel GenAI semantic-convention aggregated attribute per per-iteration agent span, for a whole run's content. Span attribute values are commonly size-limited by collectors/backends, and aggregating an entire multi-turn transcript's tool output into one attribute is exactly the kind of point where such a limit could silently truncate or drop content, or exceed backend ingestion limits. This isn't addressed anywhere in the ADR, and unlike ADR 0050 there's no "Deferred to implementation" list to park it in.
Suggestion: Note explicitly in the ADR (or add a deferred-to-implementation item) how the design handles per-backend attribute size limits — e.g., truncation policy, splitting across multiple attributes, or falling back to span events/log records if a size ceiling is hit — so implementers don't discover this only after building against the aggregated-attribute shape.
|
|
||
| - Content cannot flow by accident: no single variable, file, or workflow | ||
| edit enables capture, and every implementation PR merges inert. | ||
| - The non-production restriction on content is mechanically enforced — the |
There was a problem hiding this comment.
[MEDIUM] Dangling "non-production restriction" reference never established in the Decision
This Consequences bullet states "The non-production restriction on content is mechanically enforced — the allow-list, not prose, decides where content may go..." but no production/non-production distinction is defined anywhere in this ADR's Context or Decision ("production" doesn't appear outside this one bullet). The Decision only describes three conditions (operator env opt-in, harness consent, allow-listed endpoint host) with no production/non-production framing. A reader relying on Consequences would believe there's a policy restricting capture to non-production backends that the Decision never actually establishes.
Suggestion: Either add an explicit statement in the Decision establishing what "non-production" means (e.g., the allow-list is expected to hold only non-production endpoints, enforced via required sign-off) or reword this bullet to drop the "non-production" framing: "Content cannot flow to an arbitrary destination — the allow-list, not prose, decides where content may go, and every entry requires a recorded sign-off."
| @@ -128,27 +128,39 @@ export OTEL_EXPORTER_OTLP_TRACES_HEADERS="authorization=Basic%20${CREDS_B64},x-m | |||
| ## Enabling content capture (Level 3) | |||
There was a problem hiding this comment.
[MEDIUM] "Guide-link policy" cited to justify omitting ADR links does not exist and contradicts current practice
The PR description says this rewritten Level 3 section has "no ADR links, per the guide-link policy." No such policy is documented anywhere in this repo — not in docs/contributing/adrs.md, not in the writing-adrs skill, not in AGENTS.md. It also contradicts existing practice: docs/guides/infrastructure/layered-config-reference.md, docs/guides/dev/e2e-testing.md, docs/guides/dev/testing-workflows.md, docs/guides/dev/behaviour-testing.md, and docs/guides/user/jira-integration.md all link directly to specific ADRs by number and path. This reads as an invented convention asserted as an established rule.
Suggestion: Either link ADR 0084/0085 from this Level 3 section (matching the convention used elsewhere in docs/guides/), or, if there's an intended distinction between reference guides (which link ADRs) and operator/how-to guides (which don't), document that rule in docs/contributing/ so it can be checked in future reviews instead of asserted per-PR.
| ## Annotations | ||
|
|
||
| **2026-08-05:** [ADR 0084](0084-level-3-content-capture-activation-contract.md) | ||
| applies the secret redactor to Level 3 span content as an output-side |
There was a problem hiding this comment.
[MEDIUM] Annotation claims a consistency the ADR 0084 mechanism doesn't clearly establish
This annotation says applying the secret redactor to Level 3 content is "consistent with this ADR's framing: access control remains the security boundary, and a redaction hit is a breach signal, not the defense." But this ADR's actual mechanism (per its Consequences: "Credential detection in JSONL files surfaces isolation breaches... as incidents rather than silently redacting them") is binary and non-transformative — on a hit the entire JSONL is suppressed, content is never sanitized and shipped. ADR 0084's Level 3 mechanism reads differently: content "passes through secret redaction at assembly" and "the export path strips any content attribute lacking the redaction marker" — implying redaction actively gates what leaves over OTLP to an external, allow-listed-but-still-third-party endpoint. That makes the redactor function as a preventive control on the egress path, not merely a detector paired with access-control-as-the-real-boundary — arguably the opposite framing from this ADR. ADR 0084 also never states precisely what happens to content on a hit (drop the flagged attribute vs. mask-and-still-export), so this "consistent framing" claim can't actually be verified against ADR 0084's own text.
Suggestion: Either (a) have ADR 0084 specify precisely what happens to content on a redaction hit (drop just the flagged attribute vs. mask-and-still-export), or (b) soften this annotation to acknowledge that Level 3's export-to-external-backend context genuinely requires redaction to act as a defense, not merely a signal, since there's no post-hoc suppression option once content has left over OTLP the way there is for a stored JSONL file.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
The activation contract's structure is sound — three gates with distinct owners, fail-closed, single fullsend-owned export pipeline. But verifying ADR 0085 against the current fullsend-ai/agents harnesses shows the denylist as written breaks every existing agent, and two of its guarantees rest on unstated assumptions. Four comments inline; the first is blocking-level, the rest are wording/spec-gap fixes.
|
|
||
| - **Keys:** deny `OTEL_*`, `CLAUDE_CODE_*`, and `TRACEPARENT` among | ||
| `env.sandbox` keys, including forge-merged harness bases. | ||
| - **Destinations:** deny `host_files` destinations that the sandbox sources |
There was a problem hiding this comment.
As written, this denylist hard-fails every existing harness in fullsend-ai/agents, and the migration path it assumes is circular:
- Every agent harness mounts env files into the denied destination:
common/env/gcp-vertex.env → /sandbox/workspace/.env.d/gcp-vertex.env,env/<agent>.env → .env.d/<agent>.env. Denying.env.d/*outright is a day-one pre-flight failure for all of them. - The natural migration is
env.sandboxkeys (ADR 0055's delivery) — butgcp-vertex.envexists to setCLAUDE_CODE_USE_VERTEX=1, and theCLAUDE_CODE_*prefix is denied there too. The variable that selects the inference backend is left with no valid configuration surface at all. The Consequences section says harnesses "must migrate to the supported configuration surfaces" but for this key no such surface exists. - Prefix-matching also confiscates legitimate use:
CLAUDE_CODE_ENABLE_TELEMETRY+ metrics export to a user's own collector is a documented runtime feature (cost tracking), unrelated to Level 3 content.
Suggestion: deny exact keys that carry content/export-routing risk (OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT, OTEL_EXPORTER_OTLP_*, TRACEPARENT), and move runtime-provider/operational config behind fullsend-owned first-class knobs (e.g. runtime.provider: vertex, a telemetry: block) that the runner translates into env from trusted code — shipped before the deny takes effect. Otherwise this ADR contradicts the platform's own env delivery mechanism the moment it lands.
|
|
||
| - Closes a live gap: a harness can no longer enable the runtime's native | ||
| content telemetry or redirect its export with no fullsend gate involved. | ||
| - ADR 0084's activation contract cannot be bypassed from inside the sandbox. |
There was a problem hiding this comment.
This consequence is conditional on an assumption the ADR never states: a trusted sandbox image. image: is an unvalidated string in the harness schema — no allowlist, no digest pinning, and allowed_remote_resources covers composition URLs, not container images. A harness pointing at a custom image can bake OTEL_* into the image env, pre-populate .env.d/, or ship a doctored .claude/settings.json — all below this pre-flight's line of sight, making the bypass-from-inside claim false as written.
Either state the assumption as a consequence ("guarantees are conditional on a platform-trusted sandbox image; image trust is tracked separately") or add the missing control (image reference allowlist and/or digest pinning). Without one of those, the denylist's real value — collapsing the attack surface to a few loud, reviewable artifacts (image ref, policy hosts) instead of many quiet env lines — should be what this bullet claims, not impossibility.
| 3. **Allowlisted destination:** the resolved OTLP traces endpoint's host — | ||
| `host` or `host:port`, matched exactly and case-insensitively, no | ||
| wildcards and no scheme or path matching — must appear in the | ||
| comma-separated `FULLSEND_CONTENT_CAPTURE_ALLOWED_ENDPOINTS` org |
There was a problem hiding this comment.
The org-variable gate has no owner in per-repo mode, and the ADR doesn't say what happens there. GitHub org variables don't exist for personal-account repos, and the repo-scoped CLI subcommands (correctly) can't manage org settings. So either Level 3 is silently impossible for per-repo installs (allow-list permanently empty), or an implementation falls back to a repo variable — which collapses gates 2 and 3 into the same owner and quietly deletes the dual-consent property this contract is built on.
Please state the per-repo story explicitly: either "Level 3 requires org-mode installation" as a listed consequence, or name who the second, non-repo owner is in per-repo mode. Also worth requiring allow-list entries to reference their governance sign-off record (rather than leaving the link to convention), since a bare host:port gives the approving admin nothing to evaluate.
|
|
||
| Content flows over OTLP export only. `run-telemetry.jsonl` keeps its | ||
| documented metadata-only contract, enforced by an attribute allow-list at the | ||
| file exporter. Captured content is assembled post-iteration from the |
There was a problem hiding this comment.
This sentence carries the design's most important invariant, and it's easy to miss mid-paragraph: content is assembled runner-side from the transcript fullsend already extracts — the runtime's native in-sandbox telemetry is never enabled, and ADR 0085 makes it unconstructible. Two careful readers of this PR independently first misread Level 3 as enabling Claude Code's own content capture inside the sandbox.
Suggest promoting it to a headline statement in the Decision (e.g. "The agent runtime's native content telemetry is never enabled; Level 3 re-exports the existing transcript through fullsend's own pipeline") so the single-pipeline property — no second exporter, no redaction bypass, no trace-identity drift — reads as the contract's foundation rather than an implementation detail.
Nine findings from review, all accepted: - Cite the agentic-ci content-on-by-default claim to its public source instead of asserting it as unreferenced fact. - State that the capture variable's value set is fullsend-defined — the upstream conventions require opt-in but do not standardize values. - Stop citing ADR 0080's placement rule for a harness-field shape it does not cover; justify the surface directly (per-agent review semantics). - Define what happens on a redaction hit (mask and record a finding; encoding evasion drops the part whole) and state per-kind size budgets with backend-limit validation as a pilot precondition. - Drop the dangling non-production framing from Consequences. - State the per-repo story: the allow-list is an organization variable so its owner is distinct from CODEOWNERS; personal-account repositories cannot enable Level 3. Entries record a sign-off reference. - Promote the single-pipeline invariant to a headline Decision statement: the runtime's native content telemetry is never enabled. - ADR 0085: replace prefix denial with an enumerated denied set (content flags, OTEL_EXPORTER_*, trace identity) so provider selection and metrics-only telemetry via .env.d keep working — the previous text hard-failed every existing harness with no migration surface for CLAUDE_CODE_USE_VERTEX. .env.d mounting stays supported; its content is scanned for denied assignments. State the trusted-image assumption. - ADR 0021 annotation: acknowledge redaction acts as a preventive egress control at Level 3, not only a breach signal. - Guide: link ADRs 0084/0085 matching other guides' practice. Reviewed-by findings: waynesun09 on PR fullsend-ai#5947. Signed-off-by: Dharit Shah <dhshah@redhat.com>
|
All findings addressed in 4311fa0. Point by point: agentic-ci claim — cited to its public source:
ADR 0080 stretch — accepted. The claim is scoped down: a structured harness field is a third shape 0080's two surfaces don't cover, chosen because neither an org-wide Attribute size limits — the Decision now states per-kind size budgets with structure-preserving truncation markers, and names backend/collector limit validation as a pilot precondition tracked in #5948. Dangling non-production reference — reworded per your suggestion; the prod/non-prod sequencing lives in #5948's preconditions, not this ADR. Guide-link policy — you're right that no such documented policy exists; I inferred it from f058f7d and overstated it. The Level 3 section now links ADRs 0084/0085, matching layered-config-reference and the other guides. (If maintainers do want a link-free rule for operator guides, that belongs in docs/contributing/ as you say — not asserted per-PR.) 0021 annotation — accepted, and 0084 now specifies hit behavior: a hit masks the matched value and records a security finding; encoding evasion drops the affected part whole. The annotation now acknowledges the real difference: on the OTLP egress path there is no post-hoc suppression, so redaction acts as a preventive control there, in addition to 0021's breach-signal role. Access control at the backend remains the primary boundary. 0085 circular migration — your strongest catch; the prefix denial was wrong. The Decision now denies an enumerated set only: the content-capture flags (exact keys), Trusted-image assumption — stated as a consequence: guarantees are conditional on a platform-trusted image; the denylist's value is collapsing the attack surface to a few loud, reviewable artifacts; image trust (allowlisting/digest pinning) is a separate control. Per-repo owner — stated in the Decision: the allow-list is an organization variable precisely so its owner is distinct from the harness's CODEOWNERS; personal-account repositories have no second owner and cannot enable Level 3. Allow-list entries record a sign-off reference alongside the change. Single-pipeline invariant — promoted to a headline Decision statement: "The agent runtime's native content telemetry is never enabled" — Level 3 re-exports the transcript fullsend already extracts through its own pipeline, and ADR 0085 makes the in-sandbox alternative unconstructible. ADR 0084 is now 91 content lines — over the 80 target, all of it review-driven substance. If that reads as more than one decision, the pre-agreed fallback is splitting activation/gates from content-shape/redaction into a sibling ADR; the composite is defended first. |
… denylist ADR 0050 defined Level 3 (content in spans, explicit opt-in) but left the activation semantics unspecified. ADR 0084 records the activation contract: dual consent (operator env opt-in + per-agent harness consent) plus an allow-listed OTLP destination whose entries record governance sign-off, fail-closed on unsatisfiable capture requests, content over OTLP export only, redaction with findings, no reasoning capture. ADR 0085 extends the sandbox env denylist to OTEL_/CLAUDE_CODE_/TRACEPARENT and host_files destinations so the contract cannot be bypassed from inside the sandbox. Living docs updated per the writing-adrs skill: annotations on ADRs 0050 and 0021, the operator guide's Level 3 section rewritten to the contract, an architecture.md Decided entry, and a problem-doc cross-reference. Signed-off-by: Dharit Shah <dhshah@redhat.com>
…cope narrowing Review feedback on the Level 3 activation contract: - ADR 0082's allow-list governs mint workflow refs, so it cannot supply matching semantics for an OTLP endpoint allow-list. Define them in ADR 0084 instead: comma-separated host or host:port, matched exactly and case-insensitively, no wildcards — a wildcard entry would let an unreviewed subdomain receive content. ADR 0082 stays as precedent for the operational shape only. - ADR 0050's Level 3 section still promises reasoning text to LLM-judge scorers. Extend its annotation to record that ADR 0084 narrows the captured scope, so a reader of ADR 0050 alone is not misled about what content Level 3 exports. Signed-off-by: Dharit Shah <dhshah@redhat.com>
Nine findings from review, all accepted: - Cite the agentic-ci content-on-by-default claim to its public source instead of asserting it as unreferenced fact. - State that the capture variable's value set is fullsend-defined — the upstream conventions require opt-in but do not standardize values. - Stop citing ADR 0080's placement rule for a harness-field shape it does not cover; justify the surface directly (per-agent review semantics). - Define what happens on a redaction hit (mask and record a finding; encoding evasion drops the part whole) and state per-kind size budgets with backend-limit validation as a pilot precondition. - Drop the dangling non-production framing from Consequences. - State the per-repo story: the allow-list is an organization variable so its owner is distinct from CODEOWNERS; personal-account repositories cannot enable Level 3. Entries record a sign-off reference. - Promote the single-pipeline invariant to a headline Decision statement: the runtime's native content telemetry is never enabled. - ADR 0085: replace prefix denial with an enumerated denied set (content flags, OTEL_EXPORTER_*, trace identity) so provider selection and metrics-only telemetry via .env.d keep working — the previous text hard-failed every existing harness with no migration surface for CLAUDE_CODE_USE_VERTEX. .env.d mounting stays supported; its content is scanned for denied assignments. State the trusted-image assumption. - ADR 0021 annotation: acknowledge redaction acts as a preventive egress control at Level 3, not only a breach signal. - Guide: link ADRs 0084/0085 matching other guides' practice. Reviewed-by findings: waynesun09 on PR fullsend-ai#5947. Signed-off-by: Dharit Shah <dhshah@redhat.com>
Compression only — every review-driven point survives with identical meaning. The prior revision wrote each fix with a self-defending rationale clause; ADR style states the decision with one clause of why. Signed-off-by: Dharit Shah <dhshah@redhat.com>
6927679 to
69c0e15
Compare
|
@rh-hemartin can you take a look? |
rh-hemartin
left a comment
rh-hemartin
left a comment
There was a problem hiding this comment.
I'm not sure about forcing users to create their own harness to send traces, I get the point of making sure they opt-in very explictly, but couldn't we move that to the config.yaml?
|
Sending traces never requires a harness: Level 1 is zero-config and Level 2 is A top-level config.yaml field is the wrong shape for that: 0084's premise is That last point is the principle: capture consent should bind to the exact What is genuinely missing is the recipe. Consenting for a first-party agent # .fullsend/config.yaml
agents:
- name: code
source: harness/code-capture.yaml# .fullsend/harness/code-capture.yaml
base: https://raw.githubusercontent.com/fullsend-ai/agents/<sha>/harness/code.yaml#sha256=<hash>
telemetry:
content_capture: trueI'll add that to the operator guide in this PR, reword 0084's "three |
Review follow-up to fullsend-ai#5947 (rh-hemartin): sending traces never requires a harness — only Level 3 content consent does. Make that cost honest and the path concrete: - ADR 0084: state that telemetry.content_capture never inherits through base: composition (the allowed_remote_resources exemption), so an upstream harness cannot pre-consent for repos that compose from it. - ADR 0084: reword the enablement consequence — a stock install has no harness files, so consent costs an agents: registration plus a pinned base: overlay, not "three configuration steps". - Operator guide: show the two-file consent recipe and why the pin is the consent object.
|
I would ask this on the public forum and debate this on the collaborators meeting. I won't be there but I'm fine with any decision. That being said I think this is too much, users has been complaining about complexity, so having them create a harness to enable finer telemetry looks like the wrong direction. |
|
Agreed on the complexity — and it goes further than stated: the pinned overlay is not a one-time cost. Every upstream agent release needs a pin bump, so consenting today also takes the agent off the Proposal, in config.yaml as you suggested: telemetry:
content_capture: [code]One reviewed field, no harness authoring. One rule keeps the failure direction safe: a listed name must resolve to a config-registered Rides along in the respin: the field never resolves from |
There was a problem hiding this comment.
Thanks for pushing this forward. A few thoughts from the eval / “what’s on the trace” side.
What’s landing for me: gating where content can leave feels right. Operator env + harness consent + org allow-list so a single flip can’t route conversations to whatever OTLP host is configured. ADR 0085’s enumerated denylist also looks useful on its own even if Level 3 takes a bit.
Where I’m less sure: mostly for eval measurements (#6036): that work treats the trace as the record of what happened (run-telemetry.jsonl, and the same data over OTLP when configured). Eval scores are computed from that record afterward (using run-telemetry.jsonl to remain vendor-agnostic, OTLP if a user chooses to score traces using a specific vendor). If Level 3 content is OTLP-only and drops reasoning, anything that wants to score content has to couple to a specific backend instead of the portable local contract. I’d rather when L3 is on, the full useful payload (including reasoning) show up on the trace — file and export — still gated by who can see the artifact / job / allow-listed endpoint. ADR 0021 already puts full reasoning in transcript JSONL under that access model; I don’t think we want a thinner second shape on OTEL.
On fail-closed before sandbox: I’m with you on not silently shipping content somewhere bad. Aborting the whole agent feels heavier than how Level 2 treats a dead OTLP endpoint (ADR 0050). Some options that still protect routing:
- Degrade to metadata-only (run continues; no content export)
- Hard-fail only the content export path, not sandbox creation
- Keep fail-closed only for “we’re about to attach content to an export,” not “abort the agent”
| Level 3 activates only when three independently-owned conditions all hold. | ||
| While any of them is absent or off, runs stay metadata-only with no error. | ||
| A run that affirmatively requests capture — conditions 1 and 2 both on — | ||
| but cannot satisfy the rest of the contract fails before sandbox creation |
There was a problem hiding this comment.
I’m with you on not silently shipping content to a bad destination. I’m less sold that incomplete L3 config should abort the agent. Level 2 already fail-opens on a dead OTLP endpoint (ADR 0050). Could we instead:
(a) degrade to metadata-only
(b) hard-fail only content export
(c) fail-closed only at “about to attach content,” not before sandbox?
There was a problem hiding this comment.
The dead-endpoint comparison lands on the wrong axis: every abort in 0084 is a static config check — an allowlisted-but-dead endpoint under L3 degrades exactly as L2 does today (retry, drop, stderr; run unaffected). The concern survives on two other axes, though: main warns-and-continues even on static
telemetry misconfig (internal/telemetry/telemetry.go:159-161), and the allowlist gate couples run availability to an org-side owner — removing an allowlist entry (legitimate revocation) aborts every consenting repo's runs until their config changes.
Respin direction: split by cause. Same-repo contradictions — consent on with the secret redactor disabled, invalid opt-in value — stay pre-sandbox hard errors: locally introduced, locally fixable, genuinely contradictory. Org-side allowlist absence or revocation degrades to metadata-only with a mandatory machine-readable marker (span attribute + workflow annotation) — your (b), loud, keeping the no-silent-degrade invariant. (c) buys nothing: all gates are static and pre-sandbox-evaluable, so deferring the check just burns a sandbox.
| [ADR 0085](0085-sandbox-environment-variable-denylist.md) makes the | ||
| in-sandbox alternative unconstructible. | ||
|
|
||
| Content flows over OTLP export only; `run-telemetry.jsonl` keeps its |
There was a problem hiding this comment.
On main the file and OTLP are two exporters of the same SDK spans (#4510, dev guide). The JSONL is the local/artifact view when you aren’t looking at the backend. Making L3 OTLP-only breaks that “same trace, two views” story and means eval-measure (#6036) can’t see content on the portable primary-fact artifact without coupling to a specific backend. Prefer: when L3 is on, both exporters get the content (still gated by allow-list + who can see artifact/job/endpoint).
There was a problem hiding this comment.
Confirmed the premise: one provider, two processors (internal/telemetry/telemetry.go:153,166), the file exporter copies attributes verbatim, and today's only asymmetry runs the other way — file OTLP, via the unsampled-parent suppression. 0084 as written inverts that for the first time, and #6036 reads run-telemetry.jsonl as the primary fact, so the coupling concern is real.
One wrinkle in "both exporters get the content (still gated by allow-list)": if file content requires the endpoint allow-list, an OTLP gate controls a file that never leaves the runner — and L1-only installs still can't eval content. Respin direction: two lanes. Content assembled once, redacted at assembly, byte-identical to both sinks, strip-unmarked backstop at both exporters; the file lane activates on conditions 1+2 (+ redactor), the OTLP lane additionally requires the allowlist. File-lane exposure is bounded by the artifact bundle that already carries the full redacted transcript — with conditions 1+2 it is strictly more gated than the transcript is today. The trade-off to state explicitly: per-repo/personal installs get file-lane content but never egress — the dual-owner gate stays exactly where egress creates a new boundary.
| activation semantics unspecified: which configuration enables it, what happens | ||
| on partial configuration, and where content may flow. Content is the | ||
| highest-sensitivity telemetry fullsend emits (proprietary source, PII, tool | ||
| output), and a comparable harness ships the failure mode this gap invites: |
There was a problem hiding this comment.
Why are you adding this statement about agentic-ci here? Are you using it as a cautionary tail showing an example of what not to do?
There was a problem hiding this comment.
Yes — cautionary example: content capture enabled unconditionally is the failure mode the contract exists to prevent, and the earlier review round asked that the claim be checkable, hence the link.
| issue). Redaction runs at assembly: a hit masks the value and records a | ||
| security finding, encoding evasion drops the affected part whole, and export | ||
| strips any content attribute lacking the redaction marker. | ||
| Reasoning/thinking text is not captured — a deliberate narrowing of |
There was a problem hiding this comment.
I’d drop this narrowing. ADR 0021 already stores full reasoning in transcript JSONL; the boundary is who can download the artifact / see the run — not a thinner OTEL shape. For eval / LLM-judge use (ADR 0050 Level 3), reasoning is part of what makes the trace useful. Same access model as the rest of the content when L3 is enabled.
There was a problem hiding this comment.
For the file lane, agreed — and the exclusion's own justification doesn't hold: ADR 0021 is the only ADR about reasoning and it decided default-exposed with access control as the boundary, and the same artifact bundle already carries full reasoning transcripts. "Treats most conservatively" comes out in the respin.
For OTLP, the 0021 annotation's point stands: once content leaves there is no post-hoc suppression. So rather than silent inclusion, reasoning gets a named consent bit — the consent shape distinguishes messages from reasoning, and the allowlist sign-off records whether reasoning-bearing traces may egress. That covers the eval case (reasoning on the portable artifact) without making external retention of reasoning an unnamed rider.
| (dual consent plus an allow-listed destination, fail-closed). The environment | ||
| variable named here is honored with the value semantics defined there. | ||
| ADR 0084 also narrows the captured scope: reasoning/thinking text is **not** | ||
| captured, so the LLM-judge use case named above is served only by prompts, |
There was a problem hiding this comment.
Same as the reasoning note on 0084 — please don’t bake the exclusion into 0050 via annotation until we’ve settled whether L3 is the full useful record or a prompts-only export. Ties to eval measurements (#6036) needing contentful primary facts.
There was a problem hiding this comment.
Per the reasoning thread on 0084: the exclusion comes out — reasoning rides the file lane with L3, with a named consent bit for OTLP egress. The 0050 annotation gets rewritten accordingly in the respin.
| While any setting is absent or off, runs stay metadata-only with no error. | ||
| A run that requests capture (settings 1 and 2 on) but cannot satisfy the | ||
| rest — endpoint host not allow-listed, secret redactor disabled — fails | ||
| before the sandbox is created: content is never silently enabled, and a |
There was a problem hiding this comment.
Same ask as on ADR 0084: degrade / fail content-export-only rather than abort the agent. Keep the “never silently enable content” invariant.
There was a problem hiding this comment.
Same resolution as the 0084 thread: cause-split — hard error on same-repo contradictions, loud metadata-only degrade on org-side allowlist absence or revocation.
|
|
||
| When active, spans include system prompts, user messages, per-turn assistant | ||
| text, tool arguments, and tool results, all passed through secret redaction | ||
| before export. Reasoning/thinking text is not captured. Content flows only |
There was a problem hiding this comment.
This fights the span-hierarchy note just below that “exported spans and the local file are two views of the same trace.” If L3 splits them, that sentence stops being true. Suggest aligning with: L3 on → same content on both views; access = artifact / job / allow-listed endpoint.
There was a problem hiding this comment.
The two-lane resolution on the 0084 thread keeps that sentence true: content is assembled once, and both views are byte-identical wherever their lane's gates pass.
There was a problem hiding this comment.
Reject both ADRs as scoped. Recommend dropping the activation-contract/denylist apparatus entirely and replacing it with a one-line boundary statement in the security threat model doc.
ADR 0084 — reject
The three-gate "independently owned conditions" premise doesn't hold under this repo's own architecture:
- Gates 1 and 2 collapse to one actor. Operator env opt-in (gate 1, delivered as CI-native infra plumbing) and harness consent (gate 2, CODEOWNERS-gated) are both reachable by whoever has repo-write access to workflow/harness config. There's no separation between them.
- Gate 3's ownership model is broken. It's specified as "an organization variable" whose "owner is distinct from the harness's CODEOWNERS," with personal-account repos structurally excluded. But ADR 0044 (Accepted) deprecated per-org install mode specifically to drop the
admin:orgscope requirement — per-repo is the sole supported model, deliberately scoped torepo+workflowonly. An "org variable" gate reintroduces exactly the elevated-scope dependency 0044 removed, and for personal-account repos it's not a degraded gate, it's a permanent wall. The ADR also cites ADR 0082 as precedent for this gate's "operational shape," but 0082's actual analog (WORKFLOW_HOST_REPOS) is a mint-operator-owned env var, not a GitHub-org-owned one — different owner, different access model. As written, gate 3 has no mechanism that's both (a) actually distinct from repo-write and (b) consistent with the per-repo-only architecture. - The agentic-ci citation is weak support. It's cited as a cautionary precedent, but agentic-ci is converging into fullsend, not an adversarial input — the citation shows a self-inflicted failure mode is possible, not that fullsend faces it from an external actor.
Net: the activation contract asserts a security property — "content cannot flow to an arbitrary destination because a distinctly-owned gate approved it" — that doesn't structurally exist for either of fullsend's two install populations (personal: gate 3 unreachable; org: gate 3 collapses to the same actor as gates 1/2 whenever repo-write and org-admin coincide, which is common).
ADR 0085 — reject, don't respin
Independent of 0084, this doesn't hold up on its own terms either:
OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENTis not a Claude Code variable. Checked against Claude Code's telemetry docs — it isn't documented anywhere. It reads like the generic upstream OTel GenAI semantic-convention name, not something the runtime actually reads. Denying it blocks nothing.- The real denied-set is incomplete, and the gaps are the dangerous ones. Claude Code's actual content-capture vars are
OTEL_LOG_USER_PROMPTS,OTEL_LOG_ASSISTANT_RESPONSES,OTEL_LOG_TOOL_CONTENT. The Decision's denied set covers the first and third but omitsOTEL_LOG_ASSISTANT_RESPONSESentirely. Worse: it also omitsOTEL_LOG_RAW_API_BODIES, which dumps the full Anthropic Messages API request/response — entire conversation history — either inline or, viafile:<dir>, as untruncated files written to disk. That's a bigger, uncovered hole than anything the denylist currently blocks. - This is a whack-a-mole problem against infrastructure fullsend doesn't own. Claude Code's telemetry surface grows over time (the docs version-tag several of these vars as recent additions). A denylist that has to track an upstream product's evolving env surface starts behind and stays behind — this isn't a one-time gap, it's a standing maintenance liability, and it just missed two vars on the first pass.
- It defends against an actor who doesn't need this path anyway. The actor who could set a denied var via
env.sandbox/.env.dis the same repo-write/harness-owning actor who already has broader capability inside the sandbox — a pre/post scriptcurl, a custom skill that POSTs conversation content, anything executable. Closing five specific env var names doesn't reduce that actor's capability to exfiltrate; it just removes one path among many that were already open to someone at that trust level.
What to add instead
No new ADR. Add a scoping sentence to docs/problems/security-threat-model.md (or wherever the pipeline's guarantees are documented): fullsend's content-handling guarantees apply to its own extraction/redaction pipeline only; Claude Code's native OTel instrumentation is out of scope and left to the harness/operator's own env, same as any other in-sandbox capability.
Note there's nothing to gate today in the first place: telemetry.content_capture doesn't exist anywhere in the current harness schema — internal/harness/harness.go has no Telemetry field, and content_capture has zero hits in the repo. Fullsend's pipeline captures no content today because the capability isn't implemented, not because a settable field defaults to off. If a capture toggle is ever added, its default-off behavior is a one-line implementation fact — it doesn't need this contract to justify it.
|
Closing — not respinning. The core read holds: the contract asserts owner separation that doesn't 0085's denied set is wrong on the facts in both directions. Level 3 is unimplemented, so nothing regresses by not deciding activation |
|
🤖 Finished Retro · ❌ Failure · Started 7:05 PM UTC · Completed 7:05 PM UTC Commit: |
|
/fs-retro |
|
🤖 Finished Retro · ❌ Failure · Started 7:13 PM UTC · Completed 7:13 PM UTC Commit: |
4 participants
Summary
ADR 0050 defined Level 3 — prompt/completion content in spans — as an explicit opt-in but left the activation semantics unspecified: which configuration enables it, what happens on partial configuration, and where content may flow. Two ADRs close that gap:
OTEL_*/CLAUDE_CODE_*/TRACEPARENTkeys andhost_filesdestinations, as pre-flight hard errors, so the activation contract cannot be bypassed from inside the sandbox. Closes a live gap independent of Level 3.Living documents updated in the same PR per the writing-adrs skill: dated annotations on ADRs 0050 and 0021, the operator guide's Level 3 section rewritten to the contract (no ADR links, per the guide-link policy), an
architecture.mdDecided entry, and a cross-reference in the operational-observability problem doc.ADR numbers verified free at commit time against main and in-flight PRs via
skills/renumber-adr/scripts/inflight-adr-numbers.sh.Implementation is tracked separately (issues to follow); every implementation PR merges inert until the contract's preconditions are met. Relates to #2862 and the Level 3 line item in ADR 0050.
Test plan
make lint— ADR statuses/numbers/frontmatter, docs link scope, and markdown link linters all pass🤖 Generated with Claude Code