mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-11 05:11:49 +01:00
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 491dfa1c6d.
## 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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
4f8dd0e25a
commit
d2832a5a0b
4 files changed
+178
-3
No files matched your search
@@ -377,7 +377,8 @@ fn decode_rich_event(value: &Value) -> Result<AcpxEventPayload, LocalRunnerError
|
||||
value => 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<Value, LocalRunnerError> {
|
||||
let original = value.clone();
|
||||
let Some(question_set) = value.as_object_mut() else {
|
||||
|
||||
@@ -1947,13 +1947,17 @@ fn preserve_bounded_display_content(
|
||||
original: &Value,
|
||||
mut sanitized: Value,
|
||||
) -> Result<Value, DurableRunnerError> {
|
||||
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");
|
||||
}
|
||||
+83
@@ -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);
|
||||
}
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user