fix(qwen): generate Markdown commands instead of deprecated TOML format#1191
fix(qwen): generate Markdown commands instead of deprecated TOML format#1191mc856 wants to merge 2 commits into
Conversation
Qwen Code deprecated TOML custom commands in favor of Markdown files with YAML frontmatter. Update the Qwen adapter to emit .qwen/commands/opsx-<id>.md and register the old opsx-*.toml files as legacy artifacts so they are cleaned up on update. Closes Fission-AI#838 Generated with Claude (Cowork) using claude-fable-5; tested with the full vitest suite (1663 tests passing).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR migrates Qwen command generation from deprecated TOML format to Markdown with YAML frontmatter, aligning with Qwen Code's current recommendations. The adapter gains a YAML-safe value escaper, legacy TOML artifacts are marked for cleanup detection, and tests are updated to validate the new format and proper character escaping. ChangesQwen Markdown Command Format Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 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/core/command-generation/adapters/qwen.ts`:
- Around line 18-27: The escapeYamlValue function correctly detects values
needing quotes but doesn't escape carriage returns; update the escaping step in
escapeYamlValue to also replace '\r' with '\\r' (in addition to existing
backslash, double-quote, and newline escapes) so double-quoted YAML strings
conform to YAML 1.2; modify the replace chain inside escapeYamlValue
accordingly.
🪄 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: CHILL
Plan: Pro
Run ID: c1c9ab7b-d581-4492-929b-bc9c6e993f95
📒 Files selected for processing (7)
.changeset/qwen-markdown-commands.mddocs/supported-tools.mdsrc/core/command-generation/adapters/qwen.tssrc/core/legacy-cleanup.tstest/core/command-generation/adapters.test.tstest/core/legacy-cleanup.test.tstest/core/update.test.ts
| function escapeYamlValue(value: string): string { | ||
| // Check if value needs quoting (contains special YAML characters or starts/ends with whitespace) | ||
| const needsQuoting = /[:\n\r#{}[\],&*!|>'"%@`]|^\s|\s$/.test(value); | ||
| if (needsQuoting) { | ||
| // Use double quotes and escape internal double quotes and backslashes | ||
| const escaped = value.replace(/\\/g, '\\\\').replace(/"/g, '\\"').replace(/\n/g, '\\n'); | ||
| return `"${escaped}"`; | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
Add carriage return escape sequence for complete YAML compliance.
The regex on line 20 correctly detects \r as requiring quotes, but line 23 doesn't escape it. Per the YAML 1.2 specification, carriage returns inside double-quoted strings must be escaped. While carriage returns are rare in description strings, adding the escape ensures valid YAML output.
🛡️ Proposed fix to add carriage return escaping
// Use double quotes and escape internal double quotes and backslashes
- const escaped = value.replace(/\\/g, '\\\\').replace(/"/g, '\\"').replace(/\n/g, '\\n');
+ const escaped = value.replace(/\\/g, '\\\\').replace(/"/g, '\\"').replace(/\n/g, '\\n').replace(/\r/g, '\\r');
return `"${escaped}"`;📝 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.
| function escapeYamlValue(value: string): string { | |
| // Check if value needs quoting (contains special YAML characters or starts/ends with whitespace) | |
| const needsQuoting = /[:\n\r#{}[\],&*!|>'"%@`]|^\s|\s$/.test(value); | |
| if (needsQuoting) { | |
| // Use double quotes and escape internal double quotes and backslashes | |
| const escaped = value.replace(/\\/g, '\\\\').replace(/"/g, '\\"').replace(/\n/g, '\\n'); | |
| return `"${escaped}"`; | |
| } | |
| return value; | |
| } | |
| function escapeYamlValue(value: string): string { | |
| // Check if value needs quoting (contains special YAML characters or starts/ends with whitespace) | |
| const needsQuoting = /[:\n\r#{}[\],&*!|>'"%@`]|^\s|\s$/.test(value); | |
| if (needsQuoting) { | |
| // Use double quotes and escape internal double quotes and backslashes | |
| const escaped = value.replace(/\\/g, '\\\\').replace(/"/g, '\\"').replace(/\n/g, '\\n').replace(/\r/g, '\\r'); | |
| return `"${escaped}"`; | |
| } | |
| return value; | |
| } |
🤖 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/core/command-generation/adapters/qwen.ts` around lines 18 - 27, The
escapeYamlValue function correctly detects values needing quotes but doesn't
escape carriage returns; update the escaping step in escapeYamlValue to also
replace '\r' with '\\r' (in addition to existing backslash, double-quote, and
newline escapes) so double-quoted YAML strings conform to YAML 1.2; modify the
replace chain inside escapeYamlValue accordingly.
There was a problem hiding this comment.
Good catch on the detect/escape asymmetry. This function is an intentional verbatim copy of the existing escapeYamlValue in pi.ts and claude.ts (current convention is one copy per adapter), so the same gap exists in both siblings. Since descriptions come from internal command templates, a literal CR can't occur in practice. Rather than diverging this copy from the other two or expanding this PR's scope, I'd prefer a follow-up PR that extracts escapeYamlValue into a shared util and adds \r escaping in one place. Happy to fix it here instead if maintainers prefer.
There was a problem hiding this comment.
@mc856, that context makes sense — the detect/escape asymmetry is a pre-existing gap shared across all three adapters, and fixing it consistently via a shared utility is the right approach. Happy to leave this PR as-is.
Would you like me to open a follow-up GitHub issue to track:
- Extracting
escapeYamlValueinto a shared YAML utility - Adding
\r → \\rescaping in that shared location - Updating
pi.ts,claude.ts, andqwen.tsto consume it
Just let me know and I'll file it.
✏️ Learnings added
Learnt from: mc856
Repo: Fission-AI/OpenSpec PR: 1191
File: src/core/command-generation/adapters/qwen.ts:18-27
Timestamp: 2026-06-09T19:17:10.070Z
Learning: In the OpenSpec repository, `escapeYamlValue` is intentionally duplicated per adapter file (e.g., `src/core/command-generation/adapters/pi.ts`, `claude.ts`, `qwen.ts`) — one copy per adapter is the current convention. There is a known detect/escape asymmetry where `\r` is detected as needing quoting but is not escaped in the replace chain; this gap exists in all three siblings and is tracked for a future refactor that would extract the helper into a shared util.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
gh issue create \
--repo Fission-AI/OpenSpec \
--title "refactor(adapters): extract escapeYamlValue into shared util and fix \\r escape gap" \
--assignee mc856 \
--body "## Summary
The \`escapeYamlValue\` helper is currently duplicated verbatim across three adapter files:
- \`src/core/command-generation/adapters/pi.ts\`
- \`src/core/command-generation/adapters/claude.ts\`
- \`src/core/command-generation/adapters/qwen.ts\`
There is also a known detect/escape asymmetry: \`\\r\` (carriage return) is correctly detected as requiring YAML quoting (via the regex), but is **not** escaped in the double-quoted string replace chain. This gap exists consistently across all three siblings.
## Proposed Work
1. Extract \`escapeYamlValue\` into a shared utility (e.g., \`src/core/command-generation/utils/yaml.ts\` or similar).
2. Add \`\r → \\\\r\` escaping in the shared implementation so double-quoted YAML strings fully conform to YAML 1.2.
3. Update \`pi.ts\`, \`claude.ts\`, and \`qwen.ts\` to import and use the shared helper instead of their local copies.
4. Add or update unit tests for the shared utility to cover the \`\\r\` case.
## Context
- Discussed in PR `#1191` (comment: https://github.com/Fission-AI/OpenSpec/pull/1191#discussion_r3383171623)
- The \`\\r\` fix was intentionally deferred from PR `#1191` to avoid diverging the qwen adapter copy from its siblings before a proper shared-util refactor.
**Requested by:** `@mc856`
"Length of output: 3193
🏁 Script executed:
gh issue create \
--repo Fission-AI/OpenSpec \
--title "refactor(adapters): extract escapeYamlValue into shared util and fix \\r escape gap" \
--body "## Summary
The \`escapeYamlValue\` helper is currently duplicated verbatim across three adapter files:
- \`src/core/command-generation/adapters/pi.ts\`
- \`src/core/command-generation/adapters/claude.ts\`
- \`src/core/command-generation/adapters/qwen.ts\`
There is also a known detect/escape asymmetry: \`\\r\` (carriage return) is correctly detected as requiring YAML quoting (via the regex), but is **not** escaped in the double-quoted string replace chain. This gap exists consistently across all three siblings.
## Proposed Work
1. Extract \`escapeYamlValue\` into a shared utility (e.g., \`src/core/command-generation/utils/yaml.ts\` or similar).
2. Add \`\r → \\\\r\` escaping in the shared implementation so double-quoted YAML strings fully conform to YAML 1.2.
3. Update \`pi.ts\`, \`claude.ts\`, and \`qwen.ts\` to import and use the shared helper instead of their local copies.
4. Add or update unit tests for the shared utility to cover the \`\\r\` case.
## Context
- Discussed in PR `#1191` (comment: https://github.com/Fission-AI/OpenSpec/pull/1191#discussion_r3383171623)
- The \`\\r\` fix was intentionally deferred from PR `#1191` to avoid diverging the qwen adapter copy from its siblings before a proper shared-util refactor.
**Requested by:** `@mc856`
"Length of output: 1527
@mc856, the follow-up issue has been filed: #1205
It tracks:
- Extracting
escapeYamlValueinto a shared utility (e.g.,src/core/command-generation/utils/yaml.ts) - Adding
\r → \\rescaping in that shared implementation for YAML 1.2 compliance - Updating
pi.ts,claude.ts, andqwen.tsto consume the shared helper - Adding/updating unit tests to cover the
\rcase
Note: the integration couldn't set the assignee automatically — you may want to self-assign it via the GitHub UI.
🧬 Code Graph Analysis Results
Relevant code snippets to understand src/core/command-generation/adapters/qwen.ts and its inputs/outputs:
src/core/command-generation/types.ts(lines 12-25)
export interface CommandContent {
/** Command identifier (e.g., 'explore', 'apply', 'new') */
id: string;
/** Human-readable name (e.g., 'OpenSpec Explore') */
name: string;
/** Brief description of command purpose */
description: string;
/** Grouping category (e.g., 'Workflow') */
category: string;
/** Array of tag strings */
tags: string[];
/** The command instruction content (body text) */
body: string;
}src/core/command-generation/types.ts(lines 32-48)
export interface ToolCommandAdapter {
/** Tool identifier matching AIToolOption.value (e.g., 'claude', 'cursor') */
toolId: string;
/**
* Returns the file path for a command.
* `@param` commandId - The command identifier (e.g., 'explore')
* `@returns` Path from project root (e.g., '.claude/commands/opsx/explore.md').
* May be absolute for tools with global-scoped prompts (e.g., Codex).
*/
getFilePath(commandId: string): string;
/**
* Formats the complete file content including frontmatter.
* `@param` content - The tool-agnostic command content
* `@returns` Complete file content ready to write
*/
formatFile(content: CommandContent): string;
}🧠 Learnings used
Learnt from: mc856
Repo: Fission-AI/OpenSpec PR: 1191
File: src/core/command-generation/adapters/qwen.ts:18-27
Timestamp: 2026-06-09T19:17:10.070Z
Learning: In this repo, `escapeYamlValue` is intentionally duplicated in each adapter file (one implementation per adapter) rather than extracted to a shared util. When reviewing adapter files under `src/core/command-generation/adapters/`, do not require deduplication/refactoring of `escapeYamlValue` across siblings. Also note the known detect/escape asymmetry for `\r` (it is detected as needing quoting but is not escaped in the current replace chain) and that this behavior currently exists consistently across the adapter siblings; treat this as an existing, tracked gap and don’t insist on fixing it unless the change is part of the planned refactor and updated consistently for all adapters.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks, this is the right fix shape. I checked the Qwen docs, current adapter, legacy cleanup, docs, and tests, and the migration to Markdown commands plus TOML cleanup lines up.
One tiny follow-up: registry.test.ts still lists qwen with non-YAML tools, but the adapter/update coverage here makes that non-blocking.
|
Thanks for the review! Good point on |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-approved latest head 930de0c. I verified the follow-up is the requested registry-test change removing qwen from the non-YAML-frontmatter allowlist, and reran the focused adapter/registry/legacy-cleanup/update tests in a temp worktree: 252 tests passed. The shared escape helper cleanup can stay a non-blocking follow-up.
2 participants
Summary
Qwen Code has deprecated TOML custom commands in favor of Markdown files with YAML frontmatter (docs). This PR updates the Qwen adapter to emit
.qwen/commands/opsx-<id>.mdand registers the oldopsx-*.tomlfiles as legacy artifacts so they are cleaned up onupdate.Closes #838
Changes
qwenadapter now generates Markdown withdescriptionfrontmatter (YAML values escaped, same as thepi/claudeadapters)legacy-cleanupgains a.qwen/commands/opsx-*.tomlpattern, following the junie/opencode precedent.mdfiles are not flagged as legacydocs/supported-tools.mdand changeset updatedDesign decisions
{{args}}, but the old TOML adapter relied on Qwen's default argument appending, so this PR intentionally preserves that behavior for parity. Explicit{{args}}injection could be a follow-up..qwen/commands/opsx-*.tomlfiles are cleaned up via the same legacy-cleanup mechanism used for the junie/opencode migrations (interactive confirm onupdate, auto with--force). Non-interactive runs without--forcewarn and leave the old files in place, matching existing behavior.Testing
npx vitest run— full suite passes (1664 tests)npx eslintclean on changed filesGenerated with Claude (Cowork) using claude-fable-5.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests