fix(responses): redact tracing data

[hanakannzashi]

Summary

• remove customer-supplied and content-derived values from Responses tracing, including rendered errors, request/response digests, response deltas, tool arguments, tool/MCP names and labels, and available-tool sets
• retain permitted operational metadata where available: IDs, model IDs, HTTP status, stable error category, counts/lengths, latency, and token metrics
• add source-level privacy regression coverage for the current Responses paths, including the request-body-hash middleware

API impact

Surface	Behavior
Public routes, request schemas, successful response behavior, and persistence/history policy	Unchanged. This PR does not add, remove, or retarget an API route.
Legacy Responses input_file fetch failure	Keeps the existing 5xx class and error envelope, but replaces provider-derived detail with a stable generic message so sensitive upstream detail is not reflected to clients.
Attestation/signature records	Unchanged. They remain separate from application tracing and retain digest/signature material rather than raw request or response content.

Compatibility boundary

This is primarily a logging/privacy change. Apart from the intentionally sanitized legacy input_file failure message above, it does not alter external API routing, successful response behavior, response persistence/history policy, or attestation/signature retention and access policy.

Request and response digests are content-derived sensitive metadata and must not enter tracing. Existing attestation records remain separate from application logs and contain digest/signature material rather than raw request or response content.

Validation

• cargo fmt --all -- --check
• git diff --check
• cargo test -p api --test response_logging_privacy (2 passed)
• cargo test -p services citation_tracker --lib (13 passed)
• strict Clippy for the affected API and services targets
• complete GitHub CI passed on the current PR head

Closes #938
Part of #934

[claude]

Review: fix(responses): redact tracing data

Direction is right, and the mechanical if let Err(e) → .is_err() conversions are all behavior-preserving (I checked the two that changed nesting — reasoning_item_emitted && … at service.rs:686 and !stream_error && … at service.rs:725 — both short-circuit correctly). No prior review threads on this PR to build on.

Findings group into two buckets: the new regression test has holes that let the exact pattern this PR removes come back, and a few redactions went past what CLAUDE.md actually forbids.

🔴 The privacy test cannot catch inline {e} capture — the main pattern this PR removes

contains_error_rendering gates every prefix through is_outside_string, but {e} / {e:?} / {error} only ever occur inside the format-string literal. The "{" branch is therefore dead code, and this sails through:

tracing::error!("Failed to store response signature: {e}");  // test passes

The [", e)", ", e,", ", error)", ", error,"] fallback is also narrow — , e.to_string(), , err, , &e all slip past. Suggest scanning the message literal separately for {e}-style captures (i.e. require in-string for the { prefix), and matching , <ident> generally rather than a fixed list of two names.

🔴 RESPONSE_LOG_SOURCES is hardcoded — new files are silently unchecked

include_str! over a hand-maintained list covers all 17 files today, but a new tools/code_interpreter.rs gets zero coverage and the test still goes green. For a guard whose whole job is catching newly added logs, that is the failure mode that matters. Walk the tree instead:

// concat!(env!("CARGO_MANIFEST_DIR"), "/../services/src/responses") + read_dir recursion

🔴 tracing_invocations swallows unparseable invocations

The paren matcher returns None inside filter_map when it cannot balance (raw strings r#"…"#, or a " inside a // comment within the macro). Those invocations vanish from the check with no signal. Collect parse failures and assert on them rather than dropping them.

🟠 Attestation failures lost response_id (IDs are explicitly permitted)

crates/api/src/routes/responses.rs:400 and :672 — rid / response_id are both in scope:

tracing::error!("Failed to store response signature");

An unsigned response in a TEE is a security-relevant event and is now uncorrelatable. CLAUDE.md lists response_id under "OK to log" — please add response_id = %rid. Same for debug!("Create response request") at :261; api_key.api_key.id was permitted.

🟠 Internal errors dropped entirely instead of categorized

service.rs:339 (also dropped file_id), :1469, :2528, :3369, :3392 now log a bare string. These are S3 / provider / JoinError failures with no customer content, and this PR just introduced log_category() for exactly this purpose. With neither category nor variant name, a 5xx incident is undebuggable from prod logs:

tracing::error!(error_category = e.log_category(), "Image edit request failed");

mcp.rs:496 — debug!("Executing MCP tool") with neither label nor tool name is similarly inert; consider a hashed/short server identifier.

🟠 The scrubbed error text is still returned to the client

service.rs:339-342 redacts the log, then returns:

Err(errors::ResponseError::InternalError(format!("Failed to fetch file content: {e}")))

which errors.rs:164 serializes into the HTTP body as "Internal server error: {msg}". If that string is unsafe for internal logs, it should not be in a client-visible error body either (client logs and proxies capture those). Pick one: keep the detail in logs and genericize the client message (the pattern already used for image gen), or scrub both. brave.rs got this right — worth applying consistently.

Notes

• log_tool_error(&self, _tool_type: &str, …) at executor.rs:261 — the param is now unused; drop it from the signature and the :359 call site.
• item_id = %self.tool_call_id (executor.rs:76) labels a tool-call id as an item id; slightly misleading field name.
• No unused-binding fallout from the removed fields — available_tool_names and inferred_name are still used elsewhere. This review is static: cargo was not runnable in my environment, so I did not independently re-run the validation commands from the PR description.

⚠️ Issues found. The three test findings are the ones worth fixing before merge, since that test is what is meant to keep this class of leak from returning.

[ironloopai]

🧭 IronLoop Run · Review

This comment updates in place as the Run moves through its stages.

🟩 Final result · Completed

🟨 Queued → 🟦 Working → 🟦 Posting results → 🟩 Completed

_{Automatic trigger · attempt 1 of 3 · completed in 54s}

IronLoop completed the review and posted it to GitHub.

🔗 Result

Open submitted review →

Run details

Run: d4d4f415-000a-4474-b457-d274206765b0
Base: main at 95b3c8c
Head: codex/issue-938-privacy-main at fb58945
Created: 2026-08-19 04:49 UTC
Updated: 2026-08-19 04:50 UTC

[ironloopai]

🔍 IronLoop review

🟢 No actionable findings

No additional findings beyond the issues already raised in the captured review feedback.

Validation

• ✅ Static review — Inspected every changed area for correctness, privacy, error handling, and observability; no distinct additional defect was found.
• ✅ Patch integrity — The change contains no whitespace or conflict-marker errors.

Review details

• Run: d4d4f415-000a-4474-b457-d274206765b0
• Workflow: Review
• Attempts: 1

[hanakannzashi]

Addressed the privacy review feedback in fbd1060: Responses tracing coverage now recursively scans the implementation tree and safely handles raw strings/comments; input-file retrieval failures return a stable client-safe error; and attestation/operational failures retain only permitted IDs and stable categories. Validation is included in the latest commit.

[think-in-universe]

@ironloopai review

[think-in-universe]

@claude review

[ironloopai]

Review · Status

🟩 Completed

IronLoop completed the review and posted it to GitHub.

Result

Open submitted review →

Run details

• Run: c87ceda4-68c9-43be-a289-16ca6233097d
• Base: main at 54b4a7c
• Head: codex/issue-938-privacy-main at 60dac34
• Created: 2026-08-21 14:34 UTC
• Updated: 2026-08-21 14:43 UTC

_{Manual command by think-in-universe · attempt 1 of 3 · completed in 9m 16s}

[ironloopai]

Review · Summary

Found two gaps in the new tracing privacy regression guard; the changed production tracing calls otherwise use safe operational metadata.

Findings: 🟠 Medium 2

_{Code-specific findings are attached to the diff.}

Validation

• ✅ Tracing redaction inspection — Changed production tracing calls were statically verified to use IDs, model IDs, status metadata, counts, and stable error categories instead of raw content, digests, tool data, or rendered errors.

Review details

• Run: c87ceda4-68c9-43be-a289-16ca6233097d
• Attempts: 1

[claude]

Review: fix(responses): redact tracing data (round 2)

Re-reviewed at 60dac34 against my earlier review on this PR. The three 🔴 test findings are genuinely fixed:

• ✅ Inline {e} / {e:?} capture is now caught — format_message_contains_capture + format_string_contains_capture scan inside the message literal and handle {{ escaping.
• ✅ Source list is no longer hardcoded — collect_rust_sources walks services/src/responses recursively, so new files are covered automatically.
• ✅ Parse failures no longer vanish — tracing_invocations returns Result and the test panics on unparseable input, with raw-string / comment / char-literal handling.

Also confirmed fixed: response_id restored on both attestation paths in routes/responses.rs, input_file_fetch_error() genericizes the client-visible body, log_tool_errors dead tool_type param dropped, item_id → tool_call_id rename. I traced every if let Err(e) → .is_err() conversion, including the two that changed nesting (reasoning_item_emitted && …, !stream_error && …) — all behavior-preserving.

Two new findings, then unresolved carry-overs.

---

🔴 The guard still misses the create_err / update_err naming class — removed by this same diff

crates/api/tests/response_logging_privacy.rs:502 — PROHIBITED_RENDERED_IDENTIFIERS is a four-name list (e, err, error, hash), and both contains_positional_identifier (:406) and format_string_contains_capture (:390) match on exact identifier boundaries. So all of this passes the test:

tracing::warn!("Failed to store failed response item: {}", create_err);   // green
tracing::warn!("Failed to update response status: {update_err}");         // green
tracing::error!("Provider call failed: {cause}");                         // green

For , create_err) the candidate after the comma starts with c, so starts_with("err") is false; {create_err} does not contain the literal {err. Those first two are service.rs:1314 and :1327 on main — i.e. two of the lines this PR deletes are lines the regression guard cannot catch coming back.

Since the invariant these files now hold is "structured fields only, no rendered error values", inverting the rule is both simpler and closed:

Flag any positional format argument or inline {ident} capture inside a tracing macro in the scanned files, with a small explicit allowlist for constants. Every compliant call in the post-PR tree already uses field = value plus a literal message, so this should be green today — and it subsumes the denylist instead of requiring you to guess future variable names.

🟠 status is logged as both a u16 and a &str in the same crate

New in this diff, crates/services/src/responses/service.rs:

status = error_cause.http_status_code(),   // :807  -> u16 (HTTP status)
status = e.http_status_code(),             // :1302 -> u16 (HTTP status)
status = "failed",                         // :1251, :1418, :1439 -> &str (persisted response status)

Meanwhile routes/responses.rs:429 and service.rs:246 use status_code for the same u16. In Datadog/OTel a facet that receives both an integer and a string hits a type conflict and gets dropped or silently unindexed — which breaks exactly the status_code:5xx triage this PR is trying to preserve. Suggest status_code = <u16> everywhere, and response_status = "failed" for the persisted status.

---

🟠 Carry-over: 5xx triage is still mostly a single bucket

log_category() is the right primitive, but ResponseError::InternalError(_) => "internal_error" collapses 37 construction sites in services/src/responses — S3 fetch, DB insert/update, serialization, provider calls — into one category. Combined with the image paths, which now discard the provider error entirely in favour of a constant:

.map_err(|_| {
    tracing::error!(error_category = "image_edit_provider_failure", "Image edit request failed");
    …
})

a prod 5xx yields error_category=internal_error and nothing else. request_error_category (brave.rs:46) and web_search_error_category (tools/mod.rs:23) already demonstrate the shape that works: a &'static str derived from the error kind, never the message. Carrying a category: &'static str on InternalError (or an inner enum) would make this actionable without touching customer data. Not blocking, but it is the difference between the logs being redacted and the logs being useless.

🟠 Carry-over: MCP logs are now content-free

mcp.rs:351 debug!("Connecting to MCP server (no cache)") and :496 debug!(tool_type = "mcp", "Executing MCP tool") — dropping the customer-supplied server_label is correct, but tool_type = "mcp" is a constant, so neither line can distinguish which server or which tool. A stable non-reversible identifier (truncated SHA-256 of the label, or the server index within the request) keeps correlation without exposing the label.

Notes

• brave.rs:276 / :425 — the deleted comment ("serde_json::Error Display contains category and location only, not the body") was accurate; serde_json::Error renders category + line/column, never the payload. Replacing it with the constant "invalid_json" loses the parse location for no privacy gain — and {e} is still embedded in the WebSearchError two lines below, so the value was not actually scrubbed, only the log was.
• service.rs:336 — the UTF-8 branch dropped the identifier entirely while its sibling Err branch kept file_id = %file_uuid. IDs are explicitly permitted by CLAUDE.md; worth keeping for symmetry. Same for warn!("Failed to fetch organization system prompt") at :2356, which lost org_id.
• The description says "does not alter external API behavior", which is not quite right — the client-visible body for input-file failures changed from "Internal server error: Failed to fetch file content: {e}" to a fixed string. That change is correct and was requested last round; just worth stating so a contract-test diff is not a surprise.
• macro_open_paren (:185) does not cover debug_span! / info_span! / error_span!. None exist in the scanned tree today, but they would be invisible if added.
• Verified statically that the post-PR tree has no remaining error = %e, %err, {e}, or , e) inside a tracing macro in the scanned paths; that tool_name = WEB_SEARCH_TOOL_NAME (web_search.rs:307, web_context_search.rs:289) is a constant and correctly not matched by PROHIBITED_DERIVED_FIELDS; and that tag_name at service.rs:2836-2858 is gated by REASONING_TAGS.contains(…), so it is an allowlisted constant rather than model output. cargo was not runnable in this environment, so I did not re-run the validation commands from the description.

⚠️ Issues found. The test gap is the one I would fix before merge — same reasoning as last round: that guard is the only thing standing between this cleanup and a slow regression, and right now it cannot see the two variable names this very diff removes.

[hanakannzashi]

Follow-up on the current review: 3e648b6 tightens the source-level Responses tracing guard so all field forms are fail-closed, all standard span macros and Rust macro delimiters are inspected, and regressions cover the previously unguarded aliases/syntax. It also separates numeric HTTP status_code from lifecycle response_status, preventing mixed telemetry types. No API payload behavior changed.