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 4 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://44b79836-site.fullsend-ai.workers.dev Commit: |
… 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>
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
…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>
|
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. |
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>
2 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