Skip to content

refactor(text): consolidate cleanup owners#99718

Merged
RomneyDa merged 1 commit into
mainfrom
refactor/text-cleanup-consolidation
Jul 4, 2026
Merged

refactor(text): consolidate cleanup owners#99718
RomneyDa merged 1 commit into
mainfrom
refactor/text-cleanup-consolidation

Conversation

@RomneyDa

@RomneyDa RomneyDa commented Jul 4, 2026

Copy link
Copy Markdown
Member

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 stripAnsi contract. Move outbound prompt/runtime framing removal verbatim into a focused protocol module, leaving channel presentation conversion in sanitize-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

  • Exact head: 80e2d79dce41615fdb419befe02e5c5881a60039.
  • Focused worktree-safe proof after the final amendment: 6 files / 179 tests passed across terminal-core, outbound delivery, and agent caller shards.
  • Exact-head PR CI: 69 checks succeeded, zero pending/failing; merge state clean.
  • Testbox-through-Crabbox wrapper proof before the comment-only amendment: tbx_01kwn6928zmm02dmhknpyptqhf exited 0 after check:changed, pnpm build, and pnpm 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.
  • Fresh exact-head commit autoreview: no accepted/actionable findings; correctness 0.97.
  • ClawSweeper exact-head review: platinum patch quality, explicitly confirms this is the best maintainable fix; final refresh requested after CI completed.
  • Source/tests total: net -1 line; deleted the copied 60-line agent ANSI owner.
  • Live overlap search found no open ANSI/control/runtime-scaffolding consolidation PRs or issues.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: M maintainer Maintainer-authored PR labels Jul 4, 2026
@RomneyDa
RomneyDa force-pushed the refactor/text-cleanup-consolidation branch from 43081df to 80e2d79 Compare July 4, 2026 00:08
@clawsweeper

clawsweeper Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 3, 2026, 8:29 PM ET / 00:29 UTC.

Summary
The PR moves the copied agent ANSI stripping grammar into terminal-core and splits outbound runtime-scaffolding stripping from channel text sanitization while keeping the deprecated sanitizer re-export.

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.

  • Compatibility Export: 1 retained. The deprecated plugin-sdk/infra-runtime path still receives stripInternalRuntimeScaffolding through sanitize-text, so this refactor does not remove that shipped compatibility surface.

Stored data model
Persistent data-model change detected: migration/backfill/repair: src/infra/outbound/protocol-scaffolding.ts, migration/backfill/repair: src/infra/outbound/sanitize-text.ts, serialized state: src/agents/sessions/bash-executor.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🌊 off-meta tidepool
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Next step before merge

  • Protected maintainer/member PR should stay open for normal maintainer merge judgment; no automated repair is needed.

Security
Cleared: No concrete security or supply-chain regression found; the diff moves existing sanitizer logic and tests without workflows, lockfiles, dependencies, scripts, permissions, or secret handling changes.

Review details

Best 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 changes

Label changes:

  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🌊 off-meta tidepool and patch quality is 🦞 diamond lobster.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

  • P3: This is a low-risk internal cleanup/refactor with no user-visible behavior change and no blocking review finding.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🌊 off-meta tidepool and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate does not apply because this is a member-authored, maintainer-labeled internal refactor; focused tests and exact-head CI remain useful supporting evidence.
Evidence reviewed

PR surface:

Source -13, Tests +12. Total -1 across 9 files.

View PR surface stats
Area Files Added Removed Net
Source 7 150 163 -13
Tests 2 13 1 +12
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 9 163 164 -1

What I checked:

  • Repository policy and scoped guides applied: The root policy and scoped agents/outbound guides were read; the review applied the protected maintainer/member PR handling and the repository preference for moving cleanup ownership to the right boundary. (AGENTS.md:24, 24dba4b44a5b)
  • Current main has the duplicated cleanup ownership the PR targets: Current main keeps a narrow terminal-core stripAnsi helper, a copied agent ANSI stripping helper, and outbound runtime-scaffolding removal inside sanitize-text, matching the PR's stated consolidation target. (src/agents/utils/ansi.ts:46, 24dba4b44a5b)
  • PR preserves the broader agent ANSI grammar in terminal-core: At PR head, terminal-core adds stripAnsiSequences with the same compatibility grammar shape and non-string TypeError behavior as the deleted agent helper, while leaving the existing narrower stripAnsi contract in place. (packages/terminal-core/src/ansi.ts:53, 80e2d79dce41)
  • PR keeps protocol stripping separate and compatibility exported: At PR head, stripInternalRuntimeScaffolding lives in protocol-scaffolding and sanitize-text re-exports it for the deprecated plugin-sdk/infra-runtime compatibility barrel. (src/infra/outbound/sanitize-text.ts:5, 80e2d79dce41)
  • Callers stay on the same cleanup order: The agent shell and render call sites still decode or collect text, strip ANSI/control sequences, sanitize binary output, and normalize carriage returns; outbound delivery still strips runtime scaffolding before channel normalization. (src/agents/sessions/bash-executor.ts:101, 80e2d79dce41)
  • Adjacent tests cover the moved behavior: Existing outbound tests cover runtime tags, internal delimiters, prompt-data wrappers, stray markers, and tool-call stripping; the PR diff adds terminal-core coverage for the moved agent ANSI grammar. (src/infra/outbound/sanitize-text.test.ts:120, 80e2d79dce41)

Likely related people:

  • steipete: GitHub path history shows repeated current-main work on outbound runtime-scaffolding stripping, generic outbound delivery, plugin SDK infra-runtime compatibility, and the agent runtime internalization that introduced the copied ANSI helper. (role: feature-history owner; confidence: high; commits: c2d31a5e59c1, 856592cf001b, bb46b79d3c14; files: src/infra/outbound/sanitize-text.ts, src/infra/outbound/deliver.ts, src/agents/utils/ansi.ts)
  • yyzquwu: GitHub path history shows recent ANSI scanner work in terminal-core, which is the new home for the moved compatibility grammar. (role: recent terminal-core contributor; confidence: medium; commits: 205ab8d4bd7a; files: packages/terminal-core/src/ansi.ts)
  • vincentkoc: GitHub path history shows recent refactors of session tool output rendering, one of the agent call sites updated to consume the terminal-core ANSI helper. (role: recent agent session contributor; confidence: medium; commits: 32494c7aced4, 336494c86373; files: src/agents/sessions/tools/render-utils.ts)
  • RomneyDa: Beyond authoring this PR, GitHub path history shows current-main adjacent outbound delivery refactoring on the same delivery module touched by the cleanup. (role: recent area contributor; confidence: medium; commits: 5fa20826553c, 80e2d79dce41; files: src/infra/outbound/deliver.ts, packages/terminal-core/src/ansi.ts, src/agents/sessions/bash-executor.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels Jul 4, 2026
@RomneyDa

RomneyDa commented Jul 4, 2026

Copy link
Copy Markdown
Member Author

@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.

@clawsweeper

clawsweeper Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 4, 2026
@RomneyDa
RomneyDa merged commit 50479c2 into main Jul 4, 2026
136 of 142 checks passed
@RomneyDa
RomneyDa deleted the refactor/text-cleanup-consolidation branch July 4, 2026 00:57
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling maintainer Maintainer-authored PR P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant