Repository navigation
fix(capture): retain valid UTF8 output at the byte limit - #1006
Conversation
Signed-off-by: Rudy Celekli <47457359+rudycelekli@users.noreply.github.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
PR SummaryLow Risk Overview Unchanged: exact cap at true EOF, incomplete bytes at EOF without further discard, interior invalid UTF-8, overlong/surrogate/out-of-range prefixes still yield empty output with the prior strict rules. The 64 KiB budget, truncation flags, and process/manifest plumbing are untouched. Adds Swift Testing regressions for aligned caps, split writes, multibyte prefixes (é/€/🙂), invalid sequences, and manifest stream byteCount alignment. Reviewed by Cursor Bugbot for commit b328b49. Bugbot is set up for automated code reviews on this repo. Configure here. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed October 7, 2026, 7:24 PM ET / 23:24 UTC. ClawSweeper reviewWhat this changesPreserve valid capture-action stdout and stderr when the 64 KiB retention limit splits a UTF-8 character, with regression coverage for truncation and malformed input. Merge readiness✅ Ready for maintainer review This PR remains necessary: current main and v4.9.0 still exhibit the decoding defect. The focused repair has relevant native before/after evidence, and no actionable correctness finding remains. Priority: P2 Review scores
Verification
How this fits togetherPeekaboo’s capture-action process runner collects bounded output from a child command. That text feeds the command result and the manifest’s stream byte counts and hashes. flowchart LR
A[Child command] --> B[Stdout and stderr pipes]
B --> C[64 KiB retention limit]
C --> D[Strict UTF-8 decoding]
D --> E[Recover incomplete trailing character when truncated]
E --> F[Command result]
F --> G[Manifest stream receipts]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Retain the complete valid UTF-8 prefix within the existing byte budget while preserving strict malformed-input handling and accurate stream receipts. Do we have a high-confidence way to reproduce the issue? Yes: current main retains the first 65,536 bytes and strictly decodes them, so splitting a multibyte character discards the valid prefix. Source inspection establishes the path; the contributor also reports native before/after reproduction. Is this the best way to solve the issue? Yes: the repair is confined to truncated output and validates both the incomplete suffix and retained prefix, preserving existing malformed-input and true-EOF behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 60b541e8f340. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
Landed as 1bf9581. Thanks @rudycelekli! Verification: I read the recovery logic against the edge cases. Recovery only runs when bytes were actually truncated; the trailing scalar is completed with the minimal valid continuation (E0 needs A0, F0 needs 90), so overlong, surrogate and out-of-range prefixes still fail strictly; and the valid prefix must decode on its own. Codex autoreview (P0–P2) was scoped-clean at b328b49. Hosted CI was green on that head (CLI, Core, Tachikoma, macOS app builds, SwiftFormat, SwiftLint, CodeQL). The new |
A Peekaboo CLI binary invoked under any name not ending in "peekaboo" (a copy such as peekaboo-before, or a symlink such as pb -> peekaboo) rejected every command with "Unknown command '<its own path>'", because CommanderRuntimeRouter dropped argv[0] only when it hasSuffix("peekaboo"). The CLI entry points now take full argv and always drop exactly the executable element; the same heuristic is removed from agent default-subcommand normalization and result-envelope classification. A caller audit found only two tail-only callers (both tests), now passing an executable name.
Also adds the changelog credits for #1006 and #1007.
Tests: CommanderRuntimeArgvTests plus expanded resolution/classification tests cover absolute paths, pb, peekaboo-4.9, peekaboo-before and the peekaboo forms (141 assertion failures against the old code); focused swift test 78 tests in 7 suites; manual copied-binary and real pb symlink --version/see --help; Codex autoreview scoped-clean; hosted CI and CodeQL green.
2 participants
Summary
Preserve capture-action child stdout/stderr when the existing 64 KiB retention limit splits a UTF8 scalar. Previously, strict decoding of the incomplete retained bytes discarded the entire valid prefix. The command result lost the text and its manifest stream receipt recorded byteCount 0 with the empty-content digest.
Recover only a valid prefix whose incomplete trailing scalar could be completed, and only when bytes were actually discarded. Keep the byte budget and truncation flags. Exact-limit true EOF, incomplete true EOF, interior binary data, overlong encodings, surrogate prefixes and out-of-range prefixes retain strict behavior. Process dispatch, generation identity, signal handling, cleanup, capture permissions and artifact custody are unchanged.
Native production evidence
A complete unchanged CLI build at main
159412696passed. The native consumer links 1,853 actual production CLI/dependency objects and calls the real process runner plusCaptureActionManifestWriter.stream; it contains no copied producer or manifest implementation. Six actual owned Perl child processes exit0 with real generation identity and process-group cleanup. Both stream receipts are serialized and checked against independently hashed retained text.Before: valid split 2/3/4-byte output loses both stdout and stderr and produces byteCount 0/empty-content digest. ASCII and aligned multibyte controls retain 65,536 bytes. After: all six controls pass; the split cases retain 65,535, 65,535 and 65,533 bytes with the correct nonempty digests and truncation flags.
This executes the actual process/manifest graph without a desktop capture. Host macOS 26.0 is below the documented 26.1 UI baseline; no UI, Bridge compatibility, screen capture or permission escalation is claimed.
Verification
pnpm run lint --no-cachepasses: 112 existing warnings, zero serious violations. The changed paths pass strict lint with zero violations. An earlier extra full--strictcheck promoted those unchanged warnings to errors; it is preserved separately and is not the declared gate.b328b49925ddf086348a7717f26861192cc2138c: ordinary macOS CI all six jobs; CodeQL all three languages, including actual Swift builds/analysis; supplemental release validation all four groups. The full-safe job actually ran unchangedpnpm run test:safe, reporting 110 core tests and 2,266 CLI tests passed, and verified canonical workspace locks. The other groups ran their complete package suites. No workflows or permissions were changed.AI assistance was used; the frozen two-file source patch and native evidence received independent review before signed/DCO preparation. No release or installed binary promotion.