From 465da4227adebabedad23c8ed686fd65fd7477f1 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 9 Nov 2026 05:06:43 -0600 Subject: [PATCH] fix(runner): drop a tool target path that breaks a line so the controller can acknowledge its event (#20238) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The Paperclip Runner turns provider tool calls into durable PRP events. The controller acknowledges each event only after it validates and commits it. > - The controller's event schema forbids line terminators in a safe path such as `tool.execution` `target`. > - The ACPX sidecar and the Rust runner accepted a location path that contained a newline. Claude sometimes emits a malformed Write call that puts the file content into the path. > - The controller refused that event without an acknowledgement. The runner redelivered the same bytes, and the run stopped making progress for 14 minutes (2,848 reconnects). > - This pull request makes both producers drop such a path, so the event carries a null target and the controller accepts it. > - The benefit is that a malformed provider path can no longer wedge a run's event delivery. ## Linked Issues or Issue Description **What happened?** In restart-chaos wave seed 9807 (claude, R1, local), the run stopped receiving events after runner source seq 33. The runner reconnected 2,848 times in 14 minutes. Offline replay of the saved runner outbox through the controller's `validatePrpEvent` showed that seq 34 fails `/payload/target` (the safePath pattern), because the target is `packages/shared/src/chaos/slug-2c5c3c8f.ts\nexport function chaosSlug…`. Seq 35, 39 and 40 (the same tool call) fail the same way. **Expected behavior** The runner never emits an event that the controller's schema refuses. A malformed tool path becomes a null target, and the tool event still arrives. **Steps to reproduce** 1. Have an ACPX provider send a tool call whose location path contains a newline (for example, a Write call with the content in the path). 2. See that the runner emits `tool.execution.progressed` with that target. 3. See that the controller closes the connection without an acknowledgement on every redelivery. **Paperclip version or commit** master at 3963d38560 ## What Changed - `runner-core/src/tool_file_changes.rs`: new `is_single_line_path`, which refuses control characters and U+2028/U+2029 (the characters ECMAScript `.` does not match). `normalize_relative` uses it, so file-change paths are covered too. - `runner-core/src/provider_events.rs`: `safe_acpx_location` uses the same check (it replaces the NUL-only check). - `src/drivers/acpx/safe-locations.ts`: the sidecar drops a location whose path contains a control character or U+2028/U+2029. - Tests: a Rust test covers the observed path, `\r`, U+2028, U+2029 and tab, plus a full ACPX tool-call normalization with a broken location and file change. A TS sidecar test covers the same paths. The controller-side detection and reason capture for a refused event is in paperclipai/paperclip#20235 (Phase 5). This PR does not touch the controller. ## Verification - Red before the fix: the Rust test `drops_a_location_whose_path_breaks_a_line` and the TS test `drops a path that breaks a line…` both failed. Both pass after it. - `npx vitest run src/cli/acpx-sidecar-locations.test.ts src/cli/acpx-runtime-sidecar.test.ts src/drivers/acpx` in `packages/paperclip-runner`: 1,818 passed. - `PAPERCLIP_TYPECHECK_LOW_MEMORY=1 pnpm typecheck` in `packages/paperclip-runner`: passes. - `cargo test --lib` in `runner-core`: 598 passed. Four `task_memory::tests::monitor_*` tests fail on this macOS host in isolation too. That file is unchanged here (last changed in #18801), and the CI Rust job is the authority. ## Risks - Low. Only a path that the controller already refuses is dropped. A valid POSIX filename with a tab is now dropped as a display target too, which is consistent with the existing ACPX rich-event check (`char::is_control`). - Previous-release story: a runner outbox already saved with such an event (from an older runner) still holds it. That case needs the controller detector in paperclipai/paperclip#20235, which fails the run closed instead of looping. No saved state, schema or pack contract changes here. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, via Claude Code with tool use and code execution. ## 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 --- .../crates/runner-core/src/provider_events.rs | 44 ++++++++++++++++++- .../runner-core/src/tool_file_changes.rs | 9 ++++ .../src/cli/acpx-sidecar-locations.test.ts | 22 ++++++++++ .../src/drivers/acpx/safe-locations.ts | 2 +- 4 files changed, 75 insertions(+), 2 deletions(-) diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/provider_events.rs b/packages/paperclip-runner/runner/crates/runner-core/src/provider_events.rs index b143ab4fa63..b8c1eb73cc9 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/provider_events.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/provider_events.rs @@ -2349,7 +2349,7 @@ fn safe_acpx_location(value: Option<&Value>, allow_create_target: bool) -> Value .unwrap_or_default(); if raw_path.is_empty() || raw_path.starts_with('/') - || raw_path.contains('\0') + || !crate::tool_file_changes::is_single_line_path(raw_path) || raw_path.split('/').any(|segment| segment == "..") { return Value::Null; @@ -2868,6 +2868,48 @@ mod tests { } } + #[test] + fn drops_a_location_whose_path_breaks_a_line() { + for location in [ + "packages/shared/src/chaos/slug.ts\nexport function chaosSlug() {}", + "src/a.ts\rsrc/b.ts", + "src/a.ts\u{2028}tail", + "src/a.ts\u{2029}tail", + "src/a\tb.ts", + ] { + assert_eq!( + safe_acpx_location( + Some(&json!({ + "path": location, + "pathBoundary": "paperclip.workspace_relative_display.v2", + "pathAttestation": "paperclip.workspace_create_target.v1" + })), + true, + ), + Value::Null, + "location {location:?}", + ); + } + let events = normalize_acpx_runtime_event( + AcpxRuntimeEventKind::ToolCall, + &json!({ + "type": "tool_call", "toolCallId": "toolu_write", "tag": "tool_call_update", + "status": "in_progress", "title": "Write", + "locations": [{ + "path": "packages/shared/src/chaos/slug.ts\nexport function chaosSlug() {}", + "pathBoundary": "paperclip.workspace_relative_display.v2" + }], + "fileChanges": [{"path": "src/a.ts\nsrc/b.ts", "operation": "modify"}], + }), + Some("edit"), + "fallback", + "turn-1", + 0, + ); + assert_eq!(events[0].payload["target"], Value::Null); + assert!(events[0].payload.get("fileChanges").is_none()); + } + #[test] fn preserves_valid_posix_display_characters() { for location in [ diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/tool_file_changes.rs b/packages/paperclip-runner/runner/crates/runner-core/src/tool_file_changes.rs index 4477ca7dd02..636c8c9c394 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/tool_file_changes.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/tool_file_changes.rs @@ -49,7 +49,16 @@ fn within_call_budget(mut files: Vec) -> Vec { static TURN_DIFF_REVISION: AtomicU64 = AtomicU64::new(0); +pub(crate) fn is_single_line_path(value: &str) -> bool { + !value + .chars() + .any(|character| character.is_control() || matches!(character, '\u{2028}' | '\u{2029}')) +} + fn normalize_relative(candidate: &str) -> Option { + if !is_single_line_path(candidate) { + return None; + } let mut parts: Vec<&str> = Vec::new(); for part in candidate.split('/') { match part { diff --git a/packages/paperclip-runner/src/cli/acpx-sidecar-locations.test.ts b/packages/paperclip-runner/src/cli/acpx-sidecar-locations.test.ts index 605dc7cb10c..229be36cfdb 100644 --- a/packages/paperclip-runner/src/cli/acpx-sidecar-locations.test.ts +++ b/packages/paperclip-runner/src/cli/acpx-sidecar-locations.test.ts @@ -13,6 +13,28 @@ import { describe, expect, it } from "vitest"; import { safeAcpxLocations } from "./acpx-sidecar-locations.js"; describe("ACPX sidecar locations", () => { + it("drops a path that breaks a line, so the runner never emits a target the controller rejects", () => { + const workspace = mkdtempSync(join(tmpdir(), "paperclip-acpx-locations-")); + try { + expect( + safeAcpxLocations( + [ + { path: 'packages/shared/src/slug.ts\nexport const a = 1;' }, + { path: "src/a.ts\rsrc/b.ts" }, + { path: "src/a.ts\u2028tail" }, + { path: "src/a.ts\u2029tail" }, + { path: "src/ok.ts" }, + ], + workspace, + "edit", + "Write", + ).map((location) => location.path), + ).toEqual(["src/ok.ts"]); + } finally { + rmSync(workspace, { recursive: true, force: true }); + } + }); + it.runIf(sep === "/")( "preserves valid host-relative display names without admitting escape", () => { diff --git a/packages/paperclip-runner/src/drivers/acpx/safe-locations.ts b/packages/paperclip-runner/src/drivers/acpx/safe-locations.ts index 75897b6eb82..67fb705b9ac 100644 --- a/packages/paperclip-runner/src/drivers/acpx/safe-locations.ts +++ b/packages/paperclip-runner/src/drivers/acpx/safe-locations.ts @@ -42,7 +42,7 @@ export function safeAcpxLocations( return (locations ?? []).slice(0, 2_000).flatMap((location) => { const candidate = record(location); const rawPath = typeof candidate.path === "string" ? candidate.path : ""; - if (!rawPath || rawPath.includes("\0")) return []; + if (!rawPath || /[\p{Cc}\u2028\u2029]/u.test(rawPath)) return []; const absolute = isAbsolute(rawPath) ? resolve(rawPath) : resolve(cwd, rawPath);