From d2832a5a0b0f00655e3738fc00179db3de648614 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Tue, 10 Nov 2026 11:48:27 -0800 Subject: [PATCH] fix(runner): keep redacted display events inside their schema bounds (#20607) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agent runs execute through Paperclip Runner. Its daemon, runnerd, stores provider events durably before the server receives them > - Before storing rich display events, such as tool output and provider notices, runnerd redacts credential-shaped values. It then checks the result against the event's protocol schema > - Redaction can make a string longer. A field already at its schema limit then goes over the limit, runnerd rejects the event, and the daemon exits > - The event is replayed after each restart, so every replacement runner exits the same way. The run loops until something outside it stops the run > - This pull request trims a field that redaction pushed past its schema limit, in both places that redact rich display events, so the event is accepted and the runner keeps working > - The benefit is that long tool output that contains a secret no longer crashes the runner ## Linked Issues or Issue Description **What happened?** An agent run read GitHub Actions output and, after a context compaction, entered a loop. About every 14 seconds runnerd started, restored its durable PRP session, and exited with `paperclip-runnerd: redacted display activity exceeds its schema bounds`. This continued for 15 minutes (62 launches) until a server restart ended the run. Every reattach after that failed the same way, so the task was blocked. **Expected behavior** Redaction never makes a valid display event invalid. runnerd stores the redacted event and continues. **Steps to reproduce** 1. Produce a tool output longer than 64 KiB that contains a credential-shaped pair, for example a GitHub Actions log line `GITHUB_TOKEN: ***`. The ACPX sidecar keeps its last 64 KiB, which is exactly the `toolExecution.output` limit of 65,536 characters. 2. runnerd redacts the pair to `GITHUB_TOKEN: [REDACTED]`, which is longer, so `output` now has more than 65,536 characters. 3. `preserve_bounded_display_content` returns `redacted display activity exceeds its schema bounds`, and runnerd exits. A probe test with 64 KiB of output that contains `GITHUB_TOKEN: ***`, `token=abc`, `Authorization: Bearer x`, or `password: hunter2` gets exactly this error on `master`. **Paperclip version or commit** `master` at 491dfa1c6d5. ## What Changed - `acpx_event_payload.rs`: new `fit_redacted_display_to_length_bounds`. - It asks the rich-event validator which fields fail `maxLength`, and trims each one to its limit. - `output` keeps its tail, like the sidecar's own truncation, and sets `outputTruncated: true` on tool execution events. Other fields keep their start. - Fields within their limits do not change. A payload that still fails its schema for another reason (type, pattern, or enum) is rejected as before. - Both places that redact rich display events call it before their post-redaction schema check: - `decode_rich_event`, for ACPX `runtime.rich_event` payloads; - `preserve_bounded_display_content` in `durable/state.rs`, before durable storage. - A new `tests/acpx_rich_events.rs` test decodes a 60,000-character `plan.proposed` (Claude ExitPlanMode) with `token=abc`, and a 65,536-character tool output, through `decode_acpx_event`. - Three new tests in `durable/tests/redaction_display_bounds.rs`, included by the `durable/state.rs` test module: - A full-length tool output stays within its bound for each of the four credential shapes. It is the tail of the redacted text, no secret remains, `outputTruncated` is set, and the stored event passes its schema. - A provider notice `summary` at its 4,000-character limit keeps its start and gets no `outputTruncated` field. - A short tool output that redaction makes longer, but still within its limit, is not trimmed. ## Verification - `cargo fmt --all -- --check`: clean. - `cargo check --locked --workspace`: passes, with no new warnings. - `cargo test -p paperclip-runner-core --lib -- redaction_ durable_display_content keeps_a_long_plan governance_display`: 16 pass, including the three new tests. - `cargo test -p paperclip-runner-core --test acpx_rich_events --test acpx_event_payload`: 6 and 9 pass. Without the decoder call, the new decoder test fails with `ACPX rich event redaction exceeded its schema bounds`. - Red check: before the fix, the probe described above failed with `redacted display activity exceeds its schema bounds` for all four credential shapes. - Full `cargo test -p paperclip-runner-core --lib`: 620 pass and 2 fail. The 2 failures are `codex_provider::tests::fenced_receipt_output_reports_capacity_without_dropping_known_frames` and `fenced_tool_rejection_keeps_identity_and_real_delivery_strict`. They fail only under the full parallel run. In isolation they pass on `master` and pass 3 of 3 times with this change. ## Risks - Low. Only payloads that runnerd rejects today change, and those payloads stop the runner. A trimmed display field loses some characters at the limit. The tail of tool output is kept and `outputTruncated` is set. - Redaction still runs before trimming. Trimming only removes characters, so it cannot expose a secret. - Companion pull request #20608 bounds the transport's restore loop, so any other event that runnerd rejects fails the run quickly instead of looping. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`) via Claude Code, with tool use (shell, file editing, cargo builds and tests, read-only log analysis). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- .../runner-core/src/acpx_event_payload.rs | 42 +++++++++- .../crates/runner-core/src/durable/state.rs | 10 ++- .../durable/tests/redaction_display_bounds.rs | 83 +++++++++++++++++++ .../runner-core/tests/acpx_rich_events.rs | 46 ++++++++++ 4 files changed, 178 insertions(+), 3 deletions(-) create mode 100644 packages/paperclip-runner/runner/crates/runner-core/src/durable/tests/redaction_display_bounds.rs diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_event_payload.rs b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_event_payload.rs index 77f5f913c3e..b455854db11 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_event_payload.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_event_payload.rs @@ -377,7 +377,8 @@ fn decode_rich_event(value: &Value) -> Result value.clone(), } } - let payload = sanitize_display(payload); + let mut payload = sanitize_display(payload); + fit_redacted_display_to_length_bounds(event_type, &mut payload); if !validator.is_valid(&payload) { return Err(LocalRunnerError::invalid( "ACPX rich event redaction exceeded its schema bounds", @@ -411,6 +412,45 @@ pub(crate) fn has_bounded_rich_display_shape(event_type: &str, payload: &Value) }) } +fn rich_display_length_overflows(event_type: &str, payload: &Value) -> Vec<(String, usize)> { + let Some(schema) = rich_event_schema(event_type) else { + return Vec::new(); + }; + let Some(validator) = rich_event_validators() + .ok() + .and_then(|validators| validators.get(schema)) + else { + return Vec::new(); + }; + validator + .iter_errors(payload) + .filter_map(|error| match error.kind() { + jsonschema::error::ValidationErrorKind::MaxLength { limit } => usize::try_from(*limit) + .ok() + .map(|limit| (error.instance_path().as_str().to_owned(), limit)), + _ => None, + }) + .collect() +} + +pub(crate) fn fit_redacted_display_to_length_bounds(event_type: &str, payload: &mut Value) { + let mut output_truncated = false; + for (pointer, limit) in rich_display_length_overflows(event_type, payload) { + if let Some(Value::String(text)) = payload.pointer_mut(&pointer) { + let excess = text.chars().count().saturating_sub(limit); + *text = if pointer == "/output" { + output_truncated = true; + text.chars().skip(excess).collect() + } else { + text.chars().take(limit).collect() + }; + } + } + if output_truncated && event_type.starts_with("tool.execution.") { + payload["outputTruncated"] = Value::Bool(true); + } +} + fn sanitize_question_set(mut value: Value) -> Result { let original = value.clone(); let Some(question_set) = value.as_object_mut() else { diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/durable/state.rs b/packages/paperclip-runner/runner/crates/runner-core/src/durable/state.rs index e2765c5bb08..58fffdfb2e4 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/durable/state.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/durable/state.rs @@ -1947,13 +1947,17 @@ fn preserve_bounded_display_content( original: &Value, mut sanitized: Value, ) -> Result { - use crate::acpx_event_payload::{has_bounded_rich_display_shape, validate_question_set}; + use crate::acpx_event_payload::{ + fit_redacted_display_to_length_bounds, has_bounded_rich_display_shape, + validate_question_set, + }; // Rich display payloads are already closed and byte-bounded by their exact // protocol schemas. Applying a diagnostic preview cap again would silently // change complete output into a truncated artifact without its metadata. if has_bounded_rich_display_shape(event_type, original) { - let preserved = sanitize_bounded_display_value(original); + let mut preserved = sanitize_bounded_display_value(original); + fit_redacted_display_to_length_bounds(event_type, &mut preserved); if !has_bounded_rich_display_shape(event_type, &preserved) { return Err(DurableRunnerError::invalid( "redacted display activity exceeds its schema bounds", @@ -4677,4 +4681,6 @@ mod tests { .is_err()); assert!(state.outbox.is_empty()); } + + include!("tests/redaction_display_bounds.rs"); } diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/durable/tests/redaction_display_bounds.rs b/packages/paperclip-runner/runner/crates/runner-core/src/durable/tests/redaction_display_bounds.rs new file mode 100644 index 00000000000..74bb4ee48ca --- /dev/null +++ b/packages/paperclip-runner/runner/crates/runner-core/src/durable/tests/redaction_display_bounds.rs @@ -0,0 +1,83 @@ +#[test] +fn redaction_growth_keeps_a_full_tool_output_inside_its_schema_bound() { + let mut config = config(PathBuf::from("unused")); + config.max_outbox_bytes = 1024 * 1024; + config.max_frame_bytes = 256 * 1024; + for (needle, secret) in [ + ("GITHUB_TOKEN: ***", "***"), + ("token=abc", "token=abc"), + ("Authorization: Bearer x", "Bearer x"), + ("password: hunter2", "hunter2"), + ] { + let mut state = DurableState::new(&config); + let line = format!("{}\n{needle}\n", "x".repeat(70)); + let text: String = line + .repeat(65_536 / line.len() + 1) + .chars() + .take(65_536) + .collect(); + state + .enqueue_event(&config, "tool.execution.completed", EventPriority::P1, json!({ + "schema":"paperclip.tool.execution.v1","executionId":"tool-1","transport":"builtin","operation":"execute","status":"completed", + "output":text,"outputBytes":200_000,"outputTruncated":false,"outputDigest":format!("sha256:{}", "a".repeat(64)) + })) + .unwrap_or_else(|error| panic!("{needle} was rejected: {error:?}")); + let stored = state.outbox[0] + .envelope + .pointer("/payload/payload") + .unwrap(); + let output = stored["output"].as_str().unwrap(); + let redacted = redact_sensitive_text_values(&text); + let expected: String = redacted + .chars() + .skip(redacted.chars().count() - 65_536) + .collect(); + assert_eq!(output, expected, "{needle}"); + assert!(!output.contains(secret), "{needle}"); + assert_eq!(stored["outputTruncated"], true, "{needle}"); + assert!(crate::acpx_event_payload::has_bounded_rich_display_shape( + "tool.execution.completed", + stored + )); + } +} + +#[test] +fn redaction_growth_keeps_a_bounded_notice_summary_inside_its_schema_bound() { + let config = config(PathBuf::from("unused")); + let mut state = DurableState::new(&config); + let summary = format!("token=abc {}", "y".repeat(3_990)); + assert_eq!(summary.chars().count(), 4_000); + state + .enqueue_event(&config, "provider.notice.recorded", EventPriority::P1, json!({ + "schema":"paperclip.provider.notice.v1","noticeId":"notice-1","severity":"info","category":"provider.status", + "scope":"turn","recoverable":true,"userActionable":false,"summary":summary + })) + .unwrap(); + let stored = state.outbox[0] + .envelope + .pointer("/payload/payload") + .unwrap(); + let stored_summary = stored["summary"].as_str().unwrap(); + assert_eq!(stored_summary.chars().count(), 4_000); + assert!(stored_summary.starts_with("token=[REDACTED] y")); + assert!(stored.get("outputTruncated").is_none()); +} + +#[test] +fn redaction_within_the_bound_leaves_short_tool_output_whole() { + let config = config(PathBuf::from("unused")); + let mut state = DurableState::new(&config); + state + .enqueue_event(&config, "tool.execution.completed", EventPriority::P1, json!({ + "schema":"paperclip.tool.execution.v1","executionId":"tool-1","transport":"builtin","operation":"execute","status":"completed", + "output":"before\ntoken=abc\nafter","outputBytes":22,"outputTruncated":false,"outputDigest":null + })) + .unwrap(); + let stored = state.outbox[0] + .envelope + .pointer("/payload/payload") + .unwrap(); + assert_eq!(stored["output"], "before\ntoken=[REDACTED]\nafter"); + assert_eq!(stored["outputTruncated"], false); +} diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_rich_events.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_rich_events.rs index dc570b69a41..d577d255489 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_rich_events.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_rich_events.rs @@ -179,3 +179,49 @@ fn full_plan_presentation_survives_utf8_redaction_without_diagnostic_truncation( .contains("196 KiB") ); } + +#[test] +fn redaction_keeps_full_length_rich_display_inside_its_schema_bounds() { + let markdown: String = format!("# Plan\ntoken=abc\n{}", "- step\n".repeat(10_000)) + .chars() + .take(60_000) + .collect(); + let line = format!("{}\nGITHUB_TOKEN: ***\n", "x".repeat(70)); + let output: String = line + .repeat(65_536 / line.len() + 1) + .chars() + .take(65_536) + .collect(); + for (event_type, payload, field, limit) in [ + ( + "plan.proposed", + json!({"schema":"paperclip.plan.proposed.v1","proposalId":"proposal-1","status":"completed","markdown":markdown,"source":"claude_exit_plan_mode"}), + "markdown", + 60_000, + ), + ( + "tool.execution.completed", + json!({"schema":"paperclip.tool.execution.v1","executionId":"tool-1","transport":"builtin","operation":"execute","status":"completed","output":output,"outputBytes":200_000,"outputTruncated":false,"outputDigest":null}), + "output", + 65_536, + ), + ] { + let decoded = decode_acpx_event(&scope(), &event(event_type, payload)) + .unwrap_or_else(|error| panic!("{event_type} was rejected: {error:?}")); + let AcpxEventPayload::RichActivity { payload, .. } = decoded else { + panic!("{event_type} did not decode as rich activity"); + }; + let text = payload[field].as_str().unwrap(); + assert_eq!(text.chars().count(), limit, "{event_type}"); + assert!(text.contains("[REDACTED]"), "{event_type}"); + assert!( + !text.contains("token=abc") && !text.contains("TOKEN: ***"), + "{event_type}" + ); + if field == "markdown" { + assert!(text.starts_with("# Plan\ntoken=[REDACTED]\n")); + } else { + assert_eq!(payload["outputTruncated"], true); + } + } +}