Conversation
|
Task completed. Now I have all the context needed for a thorough review. Here's my analysis of PR #776: PR Review: Add Trae opsx command adapter (#776)Overall AssessmentThis is a clean, well-structured PR that follows established project conventions. The adapter pattern makes adding new tool support straightforward, and this PR takes good advantage of that. There are a few items worth addressing. Issue 1: Missing trailing newline in
|
| Priority | Item | Location |
|---|---|---|
| Medium | Missing unit tests in adapters.test.ts |
test/core/command-generation/adapters.test.ts |
| Low | Missing trailing newline in .gitignore |
.gitignore |
| Low | Missing JSDoc comments | src/core/command-generation/adapters/trae.ts |
| Low | Weakened test assertion in update.test.ts |
test/core/update.test.ts:1485-1491 |
| Nit | Confirm name field format matches Trae's expectations |
src/core/command-generation/adapters/trae.ts:27 |
Powered by 1Code
📝 WalkthroughWalkthroughAdds a Trae ToolCommandAdapter that emits OPSX-style command files at Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant InitCmd as InitCommand
participant Registry as AdapterRegistry
participant Adapter as traeAdapter
participant FS as FileSystem
InitCmd->>Registry: request adapter for tool 'trae'
Registry-->>InitCmd: returns traeAdapter
InitCmd->>Adapter: formatFile(CommandContent)
Adapter->>Adapter: escape YAML values / format tags (internal)
Adapter-->>InitCmd: formatted Markdown with frontmatter
InitCmd->>FS: write file at ".trae/commands/opsx-<id>.md"
FS-->>InitCmd: write confirmation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/core/update.test.ts (1)
1488-1491: This test can silently skip the adapterless behavior it intends to validate.When no adapterless tool exists, Line 1489 passes and the test returns early, so the cleanup assertion path is never exercised. Consider making this scenario deterministic (e.g., mock one known tool as adapterless in this test) so the behavior is always validated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/update.test.ts` around lines 1488 - 1491, The test currently returns early when adapterlessTool?.skillsDir is falsy, so it can silently skip validating adapterless cleanup; instead make the scenario deterministic by ensuring an adapterless tool is present for this test (e.g., mock the tool list or the lookup that yields adapterlessTool to include a known tool flagged as adapterless with a non-empty skillsDir), remove the early return path so the cleanup assertions always run, and update the assertions to operate on that mocked adapterlessTool (references: adapterlessTool and skillsDir in update.test.ts) so the adapterless cleanup behavior is reliably exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/command-generation/adapters/trae.ts`:
- Around line 5-8: The YAML quoting logic detects carriage returns (needsQuoting
uses /\r/ in the regex) but only escapes newlines; update the escaping in the
block that builds escaped (the variable escaped derived from value) to also
replace '\r' with '\\r' (i.e., add .replace(/\r/g, '\\r') in the chain) so
CRLF-origin text doesn't produce malformed frontmatter; keep the existing
backslash, double-quote and newline replacements and return the quoted string as
before.
---
Nitpick comments:
In `@test/core/update.test.ts`:
- Around line 1488-1491: The test currently returns early when
adapterlessTool?.skillsDir is falsy, so it can silently skip validating
adapterless cleanup; instead make the scenario deterministic by ensuring an
adapterless tool is present for this test (e.g., mock the tool list or the
lookup that yields adapterlessTool to include a known tool flagged as
adapterless with a non-empty skillsDir), remove the early return path so the
cleanup assertions always run, and update the assertions to operate on that
mocked adapterlessTool (references: adapterlessTool and skillsDir in
update.test.ts) so the adapterless cleanup behavior is reliably exercised.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignoredocs/commands.mddocs/supported-tools.mdsrc/core/command-generation/adapters/index.tssrc/core/command-generation/adapters/trae.tssrc/core/command-generation/registry.tstest/core/init.test.tstest/core/update.test.ts
| const needsQuoting = /[:\n\r#{}[\],&*!|>'"%@`]|^\s|\s$/.test(value); | ||
| if (needsQuoting) { | ||
| const escaped = value.replace(/\\/g, '\\\\').replace(/"/g, '\\"').replace(/\n/g, '\\n'); | ||
| return `"${escaped}"`; |
There was a problem hiding this comment.
Escape \r as well when quoting YAML values.
Line 5 treats carriage returns as special, but Line 7 only escapes \n. Escaping \r too avoids malformed/fragile frontmatter on CRLF-origin text.
💡 Proposed fix
- const escaped = value.replace(/\\/g, '\\\\').replace(/"/g, '\\"').replace(/\n/g, '\\n');
+ const escaped = value
+ .replace(/\\/g, '\\\\')
+ .replace(/"/g, '\\"')
+ .replace(/\r/g, '\\r')
+ .replace(/\n/g, '\\n');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const needsQuoting = /[:\n\r#{}[\],&*!|>'"%@`]|^\s|\s$/.test(value); | |
| if (needsQuoting) { | |
| const escaped = value.replace(/\\/g, '\\\\').replace(/"/g, '\\"').replace(/\n/g, '\\n'); | |
| return `"${escaped}"`; | |
| const needsQuoting = /[:\n\r#{}[\],&*!|>'"%@`]|^\s|\s$/.test(value); | |
| if (needsQuoting) { | |
| const escaped = value | |
| .replace(/\\/g, '\\\\') | |
| .replace(/"/g, '\\"') | |
| .replace(/\r/g, '\\r') | |
| .replace(/\n/g, '\\n'); | |
| return `"${escaped}"`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/command-generation/adapters/trae.ts` around lines 5 - 8, The YAML
quoting logic detects carriage returns (needsQuoting uses /\r/ in the regex) but
only escapes newlines; update the escaping in the block that builds escaped (the
variable escaped derived from value) to also replace '\r' with '\\r' (i.e., add
.replace(/\r/g, '\\r') in the chain) so CRLF-origin text doesn't produce
malformed frontmatter; keep the existing backslash, double-quote and newline
replacements and return the quoted string as before.
Greptile SummaryAdded command adapter for Trae to generate Major Changes
Minor Observations
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Start[OpenSpec Command Generation] --> Registry[CommandAdapterRegistry]
Registry --> Init[Static Initialization Block]
Init --> RegTrae[Register traeAdapter]
Init --> RegOther[Register Other Adapters]
User[User runs openspec init/update] --> GetAdapter[CommandAdapterRegistry.get'trae']
GetAdapter --> TraeAdapter[traeAdapter]
TraeAdapter --> GetPath[getFilePath]
GetPath --> Path[".trae/commands/opsx-explore.md"]
TraeAdapter --> Format[formatFile]
Format --> Escape[escapeYamlValue]
Format --> FormatTags[formatTagsArray]
Content[CommandContent] --> Format
Format --> Output["---\nname: /opsx-explore\ndescription: ...\ncategory: ...\ntags: [...]\n---\n\nbody"]
Output --> Write[Write to .trae/commands/]
style TraeAdapter fill:#90EE90
style RegTrae fill:#90EE90
style Output fill:#FFE4B5
Last reviewed commit: 4689733 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/command-generation/adapters.test.ts (1)
589-616: Consider adding YAML escaping tests for description and category.The
traeAdapterusesescapeYamlValuefor description and category fields, but unlikepiAdapter(lines 551-567), there are no tests verifying this escaping behavior. Adding tests for special characters (colons, quotes, newlines) would ensure the escaping logic works correctly.💡 Example tests to add
it('should escape YAML special characters in description', () => { const contentWithSpecialChars: CommandContent = { ...sampleContent, description: 'Fix: regression in "auth" feature', }; const output = traeAdapter.formatFile(contentWithSpecialChars); expect(output).toContain('description: "Fix: regression in \\"auth\\" feature"'); }); it('should escape newlines in description', () => { const contentWithNewline: CommandContent = { ...sampleContent, description: 'Line 1\nLine 2', }; const output = traeAdapter.formatFile(contentWithNewline); expect(output).toContain('description: "Line 1\\nLine 2"'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/command-generation/adapters.test.ts` around lines 589 - 616, Add tests to verify YAML-escaping behavior for description and category in the traeAdapter tests: create CommandContent variants with special characters (e.g., colons, quotes) and with newlines, call traeAdapter.formatFile, and assert the output contains the escaped strings (relying on escapeYamlValue behavior) for both description and category; target the traeAdapter.formatFile test block in test/core/command-generation/adapters.test.ts so the new cases mirror the piAdapter escaping tests but exercise traeAdapter.formatFile and its handling of CommandContent fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/core/command-generation/adapters.test.ts`:
- Around line 589-616: Add tests to verify YAML-escaping behavior for
description and category in the traeAdapter tests: create CommandContent
variants with special characters (e.g., colons, quotes) and with newlines, call
traeAdapter.formatFile, and assert the output contains the escaped strings
(relying on escapeYamlValue behavior) for both description and category; target
the traeAdapter.formatFile test block in
test/core/command-generation/adapters.test.ts so the new cases mirror the
piAdapter escaping tests but exercise traeAdapter.formatFile and its handling of
CommandContent fields.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignoresrc/core/command-generation/adapters/trae.tstest/core/command-generation/adapters.test.tstest/core/update.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- test/core/update.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/update.ts (1)
443-445: Factor the Trae legacy-path construction into one helper.The flat Trae path literal now exists in three places here and another place in
src/core/init.ts. That makes the cleanup logic easy to desync from the adapter or from any future legacy-path change. A small shared helper for Trae legacy paths would keep init/update aligned and avoid more path drift.Also applies to: 484-486, 693-695
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/update.ts` around lines 443 - 445, Create a single helper (e.g., buildTraeLegacyPath or getTraeLegacyPath) that encapsulates the Trae legacy path construction using the inputs used here (toolId, projectPath, workflow) and return null when toolId !== 'trae'; replace the inline literal/logic in update.ts (the legacyTraePath assignment), the other duplicated spots (the blocks around lines referenced as 484-486 and 693-695), and the similar use in init.ts to call this helper so all places share one canonical path implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/init.ts`:
- Around line 557-566: The cleanup currently only deletes legacy flat files
(.trae/commands/opsx-${workflow}.md) when tool.value === 'trae'; add deletion of
the adapter-resolved files at .trae/commands/opsx/${commandId}.md as produced by
src/core/command-generation/adapters/trae.ts (where commandId is the identifier
used there) so stale adapter files are removed when workflows drop out; update
the loop that iterates ALL_WORKFLOWS (and the block guarded by tool.value ===
'trae') to also construct and unlink the adapter path
(.trae/commands/opsx/${commandId}.md) alongside the existing legacyCommandFile,
keeping the legacy flat filename removal as an extra step.
---
Nitpick comments:
In `@src/core/update.ts`:
- Around line 443-445: Create a single helper (e.g., buildTraeLegacyPath or
getTraeLegacyPath) that encapsulates the Trae legacy path construction using the
inputs used here (toolId, projectPath, workflow) and return null when toolId !==
'trae'; replace the inline literal/logic in update.ts (the legacyTraePath
assignment), the other duplicated spots (the blocks around lines referenced as
484-486 and 693-695), and the similar use in init.ts to call this helper so all
places share one canonical path implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e0fae79-6634-4279-b857-6b74159a75ad
📒 Files selected for processing (3)
src/core/command-generation/adapters/trae.tssrc/core/init.tssrc/core/update.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/command-generation/adapters/trae.ts
| if (tool.value === 'trae') { | ||
| for (const workflow of ALL_WORKFLOWS) { | ||
| const legacyCommandFile = path.join(projectPath, '.trae', 'commands', `opsx-${workflow}.md`); | ||
| try { | ||
| if (fs.existsSync(legacyCommandFile)) { | ||
| await fs.promises.unlink(legacyCommandFile); | ||
| } | ||
| } catch {} | ||
| } | ||
| } |
There was a problem hiding this comment.
Delete the adapter-resolved Trae files here too.
Lines 559-563 only purge .trae/commands/opsx-${workflow}.md, but src/core/command-generation/adapters/trae.ts:35-40 currently resolves Trae commands to .trae/commands/opsx/${commandId}.md. Since init does not remove deselected command files while commands stay enabled, stale Trae commands will survive refreshes for workflows that drop out of the active profile. Purge the adapter path here as well, and keep the flat filename only as an extra legacy location if you still need backward cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/init.ts` around lines 557 - 566, The cleanup currently only deletes
legacy flat files (.trae/commands/opsx-${workflow}.md) when tool.value ===
'trae'; add deletion of the adapter-resolved files at
.trae/commands/opsx/${commandId}.md as produced by
src/core/command-generation/adapters/trae.ts (where commandId is the identifier
used there) so stale adapter files are removed when workflows drop out; update
the loop that iterates ALL_WORKFLOWS (and the block guarded by tool.value ===
'trae') to also construct and unlink the adapter path
(.trae/commands/opsx/${commandId}.md) alongside the existing legacyCommandFile,
keeping the legacy flat filename removal as an extra step.
1 participant
Summary
Add Trae opsx command adapter so OpenSpec generates .trae/commands/opsx-.md .
Update docs to reflect Trae command support and syntax.
Add/adjust tests for Trae command generation and adapterless tool handling.
Context
Newer versions of Trae can read commands from .trae/commands . This change aligns Trae with other tools that use command adapters.
Testing
pnpm test
Summary by CodeRabbit
New Features
Documentation
Tests
Chores / Bug Fixes