refactor(messages): extract content helpers (4 of 8) - #1901
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**⚙️ CodeRabbit configuration file
Files:
**/*⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🪛 ast-grep (0.44.1)src/utils/messages/content.ts[warning] 20-25: Do not use variable for regular expressions (regexp-non-literal-typescript) [warning] 30-30: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns. (regexp-from-variable) [warning] 31-31: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns. (regexp-from-variable) 🪛 OpenGrep (1.23.0)src/utils/messages/content.ts[ERROR] 34-34: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead. (coderabbit.command-injection.exec-js) [ERROR] 44-44: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead. (coderabbit.command-injection.exec-js) [ERROR] 50-50: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead. (coderabbit.command-injection.exec-js) 🔇 Additional comments (5)
📝 WalkthroughWalkthroughMessage content helpers were moved into ChangesMessage content module extraction
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/messages/content.test.ts (1)
1-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the remaining moved helpers.
isEmptyMessageTextandgetAssistantMessageTextare moved but have no tests, andtextForResubmit's non-bash paths (slash-command extraction viaCOMMAND_NAME_TAG, and the plain-text fallback throughstripIdeContextTags) aren't exercised either. Since this PR's purpose is to lock in existing semantics during extraction, filling these gaps now is cheap insurance against regressions in later parts of the de-monolith migration.As per path instructions, "Review tests for meaningful coverage of the changed behavior... Block when risky runtime changes lack focused regression coverage."
♻️ Example additional tests
test('isEmptyMessageText treats stripped-tag-only and sentinel text as empty', () => { expect(isEmptyMessageText('<context>hidden</context>')).toBe(true) expect(isEmptyMessageText('(no content)')).toBe(true) expect(isEmptyMessageText('hello')).toBe(false) }) test('getAssistantMessageText joins text blocks for assistant messages', () => { const message = { type: 'assistant', message: { content: [{ type: 'text', text: 'a' }, { type: 'text', text: 'b' }] }, } as never expect(getAssistantMessageText(message)).toBe('a\nb') }) test('textForResubmit extracts slash commands and falls back to plain text', () => { const cmdMessage = createUserMessage({ content: '<command-name>review</command-name><command-args>pr</command-args>', }) expect(textForResubmit(cmdMessage)).toEqual({ text: 'review pr', mode: 'prompt' }) const plainMessage = createUserMessage({ content: 'just text' }) expect(textForResubmit(plainMessage)).toEqual({ text: 'just text', mode: 'prompt' }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/messages/content.test.ts` around lines 1 - 45, Add regression tests for the moved helpers that currently lack coverage: `isEmptyMessageText` should be verified for stripped-tag-only and sentinel inputs, and `getAssistantMessageText` should be checked to join assistant text blocks correctly. Also extend `textForResubmit` tests to cover the non-bash paths by exercising `COMMAND_NAME_TAG` slash-command extraction and the plain-text fallback through `stripIdeContextTags`, using the existing `createUserMessage`, `extractTag`, `stripPromptXMLTags`, and `textForResubmit` symbols to locate the relevant behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/utils/messages/content.test.ts`:
- Around line 1-45: Add regression tests for the moved helpers that currently
lack coverage: `isEmptyMessageText` should be verified for stripped-tag-only and
sentinel inputs, and `getAssistantMessageText` should be checked to join
assistant text blocks correctly. Also extend `textForResubmit` tests to cover
the non-bash paths by exercising `COMMAND_NAME_TAG` slash-command extraction and
the plain-text fallback through `stripIdeContextTags`, using the existing
`createUserMessage`, `extractTag`, `stripPromptXMLTags`, and `textForResubmit`
symbols to locate the relevant behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52afa24e-9d01-405b-8eec-9392dba4d03c
📒 Files selected for processing (3)
src/utils/messages.tssrc/utils/messages/content.test.tssrc/utils/messages/content.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports in source and test files.
Files:
src/utils/messages/content.tssrc/utils/messages.tssrc/utils/messages/content.test.ts
src/{commands,components,services,tools,utils,integrations,entrypoints,tasks}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer the existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Files:
src/utils/messages/content.tssrc/utils/messages.tssrc/utils/messages/content.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
In the files you touch, preserve the existing code style.
Files:
src/utils/messages/content.tssrc/utils/messages.tssrc/utils/messages/content.test.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/messages/content.tssrc/utils/messages.tssrc/utils/messages/content.test.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/utils/messages/content.tssrc/utils/messages.tssrc/utils/messages/content.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/messages/content.test.ts
🪛 ast-grep (0.44.1)
src/utils/messages/content.ts
[warning] 30-30: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(<${escapedTag}(?:\\s+[^>]*?)?>, 'gi')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 31-31: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(<\\/${escapedTag}>, 'gi')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 20-25: Do not use variable for regular expressions
Context: new RegExp(
<${escapedTag}(?:\\s+[^>]*)?> + // Opening tag with optional attributes
'([\s\S]*?)' + // Content (non-greedy match)
<\\/${escapedTag}>, // Closing tag
'gi',
)
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
🪛 OpenGrep (1.23.0)
src/utils/messages/content.ts
[ERROR] 34-34: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 44-44: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 50-50: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (3)
src/utils/messages/content.ts (2)
9-63: Static analysis ReDoS/command-injection flags are false positives here.The ast-grep ReDoS warnings (lines 21-25, 30-31) and OpenGrep "command injection" hits (lines 34/44/50) don't apply:
escapedTagis passed throughescapeRegExpbefore being interpolated, so it's not attacker-controlled regex syntax, and there's nochild_processcall anywhere in this file (the OpenGrep hit is matching.exec()on theRegExpobjects, notchild_process.exec). No change needed.Source: Linters/SAST tools
1-148: LGTM!src/utils/messages.ts (1)
649-658: LGTM!Re-export list matches
content.ts's exports one-for-one; existing import paths frommessages.tscontinue to work unchanged.
techbrewboss
left a comment
techbrewboss
left a comment
There was a problem hiding this comment.
Review summary
Part 4 of 8 of the messages.ts de-monolith — extract text/content helpers into src/utils/messages/content.ts with re-exports. Approve.
Compared each moved symbol against main: bodies are byte-identical. Public surface preserved; no cycle. CI green.
Findings
src/utils/messages.ts— leftover unused importsstripIdeContextTagsandescapeRegExpafter the move (same nit class as #1899).src/utils/messages.tsre-export — double-quoted path; prefer'./messages/content.js'.src/utils/messages/content.ts— extra blank line (cosmetic).src/utils/messages/content.test.ts— partial coverage of moved helpers; fine for a verbatim extract, optional follow-up.
Validation
- Body comparison vs
main: identical bun test src/utils/messages/content.test.ts— passbun run typecheck— passbun run security:pr-scan— clean- GitHub checks — all SUCCESS
Maliciousness / Risk
None — pure refactor, no supply-chain or trust-boundary changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/messages/content.test.ts`:
- Around line 68-84: The test for getAssistantMessageText is using an
unnecessary cache-busting dynamic import, which looks like leftover debug
behavior. Replace the import in content.test.ts with the same static import
pattern used by the other helpers in this file, and keep the test focused on
asserting the message text behavior from getAssistantMessageText without forcing
a fresh module load.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 64bf2e9b-351f-47bf-9e35-7d624e4882c8
📒 Files selected for processing (3)
src/utils/messages.tssrc/utils/messages/content.test.tssrc/utils/messages/content.ts
💤 Files with no reviewable changes (1)
- src/utils/messages/content.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/utils/messages/content.test.tssrc/utils/messages.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/utils/messages/content.test.tssrc/utils/messages.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/messages/content.test.tssrc/utils/messages.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/messages/content.test.ts
🔇 Additional comments (2)
src/utils/messages.ts (1)
647-656: LGTM! Prior nit (quote style) and unused-import cleanup (stripIdeContextTags,escapeRegExp) both appear addressed per the line-range-change-details.src/utils/messages/content.test.ts (1)
1-66: LGTM!Also applies to: 86-91
f0fdbf2 to
8a32ec2
Compare
8a32ec2 to
a83b94d
Compare
* refactor(messages): extract content helpers * test(messages): cover extracted content helpers * test(messages): statically import content helper * test(compact): preserve assistant text helper semantics
T1 cherry-pick landed 8 upstream commits (Twigpine#1908 shim, Twigpine#1916 diff, Twigpine#1917 hunks, Twigpine#1913 powershell, Twigpine#1905 plan mode, Twigpine#1906 API cleanup, Twigpine#1901 content, Twigpine#1932 gitdiff cap). The following typecheck fixes were needed because OpenCC has stricter types than upstream (per `cherry-pick-test-localization-not-shipped`): 1. descriptors.ts:64 -- `enableToolStreaming?: true` widened to `boolean` so upstream's `enableToolStreaming: false` (in shim 2259c80) typechecks 2. apiTransform.ts:33 -- `message.uuid ?? ''` (UserMessage.uuid is optional in OpenCC Message type; upstream's UserMessage.uuid is required) 3. apiTransform.ts:225-261 -- `stripCallerFieldFromAssistantMessage` early-returns for string content; Upstream assumed `content` was array 4. content.ts:77-119 -- guard for OpenCC's optional `Message.message` and widened param types to accept `UserMessage` (uuid is `string | undefined`) 5. compact.test.ts:363 -- preserves OpenCC's existing `getAssistantMessageText` mock instead of upstream's "_realMessagesModule" (per user directive) Verification: - bun run typecheck: 0 errors (was 8 with cherry-picks+before-fix; 0 at baseline pre-T1) - bun test (T1 areas): 272 pass / 21 fail (21 fails are pre-existing GLM-5.2/estimateMessageTokens) - bun test (full): 4858 pass / 131 fail (same as baseline pre-T1; no regression) No rebrand or provider-policy changes needed; cherry-picks are 3-Provider-clean.
3 participants
Summary
Part 4 of 8 in the messages.ts de-monolith migration.
This standalone PR extracts text/content helpers from src/utils/messages.ts into src/utils/messages/content.ts. The public messages.ts surface continues to re-export the moved APIs so existing imports remain valid while this migration lands one PR at a time.
Associated test coverage included:
Related active PR impact
No direct active PR overlap found for this text-helper slice. #1614 still directly touches src/utils/messages.ts, but this PR avoids the normalization/tool-id areas that #1614 edits.
Validation
Summary by CodeRabbit