refactor(text): consolidate cleanup owners#99718
Conversation
43081df to
80e2d79
Compare
|
Codex review: needs maintainer review before merge. Reviewed July 3, 2026, 8:29 PM ET / 00:29 UTC. Summary PR surface: Source -13, Tests +12. Total -1 across 9 files. Reproducibility: not applicable. This PR is a no-user-visible-behavior refactor rather than a bug report. The review compared current main, the PR diff, callers, adjacent tests, live PR status, and GitHub path history. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsBest possible solution: Land the owner-boundary refactor through normal maintainer review if maintainers accept the cleanup, keeping the deprecated SDK compatibility export intact. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR is a no-user-visible-behavior refactor rather than a bug report. The review compared current main, the PR diff, callers, adjacent tests, live PR status, and GitHub path history. Is this the best way to solve the issue? Yes: moving the copied ANSI grammar into terminal-core and runtime scaffolding removal out of channel presentation sanitization is the narrow owner-boundary fix, while preserving the deprecated SDK re-export avoids a compatibility break. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 24dba4b44a5b. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source -13, Tests +12. Total -1 across 9 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
@clawsweeper re-review Exact-head CI is now complete: 69 successful checks, zero pending/failing, merge state clean. The PR evidence also clarifies that the Testbox-through-Crabbox wrapper exited 0 and the backing lifecycle job was cancelled only when the successful one-shot lease was stopped. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
What Problem This Solves
ANSI/control-sequence cleanup and outbound protocol-marker removal had overlapping “text sanitizer” shapes under unrelated owners. The agent runtime carried a copied ANSI implementation outside terminal-core, while outbound runtime framing lived inside the channel-oriented plain-text/HTML sanitizer, making future consolidation likely to mix distinct contracts.
Why This Change Was Made
Move the historical agent ANSI grammar unchanged into terminal-core as a narrow helper while preserving the existing, narrower
stripAnsicontract. Move outbound prompt/runtime framing removal verbatim into a focused protocol module, leaving channel presentation conversion insanitize-text.ts; retain the shipped deprecated Plugin SDK compatibility export. Provider payload cleanup, Unicode policy, Markdown, TUI index mapping, and channel-specific transforms remain separate.User Impact
No user-visible behavior change. Agent shell/tool output keeps the same escape grammar, and outbound delivery keeps the same marker ordering, whitespace, malformed-delimiter, Unicode, prompt-wrapper, and tool-call cleanup behavior.
Evidence
80e2d79dce41615fdb419befe02e5c5881a60039.tbx_01kwn6928zmm02dmhknpyptqhfexited 0 aftercheck:changed,pnpm build, andpnpm check:import-cycles; 0 runtime value cycles. The backing Actions Testbox lifecycle job may show cancelled because the successful one-shot wrapper stopped the delegated lease after completion.