Skip to content

[codex] sanitize ACP native event diagnostics#3417

Merged
juliusmarminge merged 1 commit into
mainfrom
codex/sanitize-acp-native-logs
Jun 20, 2026
Merged

[codex] sanitize ACP native event diagnostics#3417
juliusmarminge merged 1 commit into
mainfrom
codex/sanitize-acp-native-logs

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Jun 20, 2026

Copy link
Copy Markdown
Member

Summary

  • replace raw ACP request/result/protocol payload logging with bounded type, size, method, status, and failure-tag diagnostics
  • stop serializing nested causes and filesystem/serialization defects into provider log warnings
  • preserve logger interruption semantics while keeping best-effort writer defects non-fatal
  • add a reusable bounded errorTag helper for untyped failures

Validation

  • vp test packages/shared/src/observability.test.ts apps/server/src/provider/acp/AcpNativeLogging.test.ts apps/server/src/provider/Layers/EventNdjsonLogger.test.ts (18 tests)
  • vp check (passes with existing warnings)
  • vp run typecheck

Note

Medium Risk
Changes what gets persisted in provider observability logs (less detail for debugging) but reduces leakage of session/protocol secrets; write-path interrupt behavior is explicitly preserved.

Overview
Provider observability logs no longer carry raw ACP or filesystem failure details. Native request/protocol records and NDJSON writer warnings are reduced to bounded metadata (payload shape, byte lengths, validated method names, errorTag, reasonCount) instead of full payloads, Cause.pretty output, or nested error objects.

ACP native logging adds summarizePayload / formatProtocolLogPayload so secrets in prompts, tokens, and wire JSON are not written to thread logs. Write failures are handled with catchCause: interruptions propagate; other defects log structural tags only. EventNdjsonLogger applies the same errorTag pattern for serialization and I/O warnings.

packages/shared exports errorTag with structural-tag validation (replacing inline tag/name extraction used by causeErrorTag). Tests assert secrets stay out of serialized log output and interruption semantics are preserved.

Reviewed by Cursor Bugbot for commit d3567de. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Sanitize ACP native event diagnostics by replacing raw error objects with bounded structural tags

  • Adds errorTag to observability.ts that converts arbitrary error values into bounded structural tag strings (1–128 chars, [A-Za-z][A-Za-z0-9._:/-]*), used by causeErrorTag.
  • Updates EventNdjsonLogger.ts to log errorTag(error) instead of raw error objects on serialization, sink init, flush, and directory creation failures.
  • Updates AcpNativeLogging.ts to summarize request/protocol payloads (type, size, field count) and emit errorTag/reasonCount instead of raw payloads or Cause.pretty output.
  • Adds interruption preservation in makeAcpNativeLoggerFactory: interruptions propagate via Effect.interrupt; non-interrupt failures emit a bounded warning.
  • Behavioral Change: log records for ACP events and ndjson writer failures no longer contain raw error objects or payload content — only structural tags and size metadata.

Macroscope summarized d3567de.

Co-authored-by: codex <codex@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 758577ed-e710-437c-bd6c-46066c72949b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/sanitize-acp-native-logs

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jun 20, 2026
@macroscopeapp

macroscopeapp Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved

This PR sanitizes ACP native event diagnostics to prevent sensitive data from being logged, replacing raw error objects and payloads with structural summaries (type names, sizes, counts). The changes are well-tested and limited to logging output, with the author being the primary maintainer of the affected files.

You can customize Macroscope's approvability policy. Learn more.

@juliusmarminge
juliusmarminge merged commit d53237c into main Jun 20, 2026
16 checks passed
@juliusmarminge
juliusmarminge deleted the codex/sanitize-acp-native-logs branch June 20, 2026 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant