fix(adapters): escape YAML frontmatter values consistently across all command adapters - #1447
Conversation
… command adapters
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesShared YAML serialization now consistently quotes and escapes frontmatter values, including tag arrays. Command-generation adapters use the shared helpers, with tests covering YAML round trips. Archive workflows now require inline synchronization and main-spec verification before archiving. YAML frontmatter generation
Archive workflow synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
🤖 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 `@test/core/command-generation/adapters.test.ts`:
- Around line 18-21: Update the YAML adapter matrix in the adapter tests to
include kiroAdapter and lingmaAdapter alongside the existing YAML-emitting
adapters. Keep kilocodeAdapter excluded from yamlAdapters because its dedicated
coverage verifies that it does not emit YAML frontmatter.
- Around line 977-997: Update escapeYamlValue, used by the adapter formatFile
flow, to quote values that YAML would interpret as non-string scalars, including
true, null, and numeric values, and to exclude indicator-prefixed text such as
“- starts here” from plain scalars. Extend the existing YAML frontmatter tests
with round-trip cases asserting these values parse back as the original strings.
🪄 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 Plus
Run ID: 5b1b1ec2-de80-4b21-8370-9916276171d4
📒 Files selected for processing (23)
src/core/command-generation/adapters/amazon-q.tssrc/core/command-generation/adapters/antigravity.tssrc/core/command-generation/adapters/auggie.tssrc/core/command-generation/adapters/bob.tssrc/core/command-generation/adapters/claude.tssrc/core/command-generation/adapters/codebuddy.tssrc/core/command-generation/adapters/continue.tssrc/core/command-generation/adapters/costrict.tssrc/core/command-generation/adapters/crush.tssrc/core/command-generation/adapters/factory.tssrc/core/command-generation/adapters/github-copilot.tssrc/core/command-generation/adapters/iflow.tssrc/core/command-generation/adapters/junie.tssrc/core/command-generation/adapters/kiro.tssrc/core/command-generation/adapters/lingma.tssrc/core/command-generation/adapters/opencode.tssrc/core/command-generation/adapters/qoder.tssrc/core/command-generation/adapters/qwen.tssrc/core/command-generation/adapters/trae.tssrc/core/command-generation/adapters/windsurf.tssrc/core/command-generation/adapters/zcode.tssrc/core/command-generation/yaml.tstest/core/command-generation/adapters.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The affected encoder code is unchanged from the reproduced hold: values beginning with a block indicator can produce invalid YAML, while true, null, and numeric-looking strings round-trip as other scalar types. Please serialize every frontmatter string safely and add table-driven round-trip coverage across the changed adapters, including Kiro and Lingma.
Code review — verdict: good to mergeI was initially wary of this one — 23 files touching 20 adapters is exactly the shape that quietly changes output for every user. So I didn't review the diff; I generated the full artifact tree for all 21 affected tools on 1. Real need — yes, and it's bigger than the description suggests
---
name: OPSX: Apply
description: "Implement tasks from an OpenSpec change (Experimental)"
argument-hint: "[command arguments]"
---
Swept across every generated frontmatter file in the tree: So every command file OpenSpec generates for CodeBuddy, Crush, Lingma, and Qoder has broken frontmatter today. That's a shipping bug affecting four integrations, and it's invisible unless you actually parse the output — which is presumably why it survived this long. Good find. 2. Does it work — yes, all 24 fixed, nothing else broken0 unparseable files after. And critically, the fix doesn't just make things parse — it makes them parse to the right values. 3. Breaking changes — none semantically, and I verified this rather than assumingThis was my main concern, because the diff also removes quotes in places (several adapters previously always-quoted Zero semantic changes across 222 files, and zero body changes. Every quoting difference is stylistic —
The one thing to be aware of: on the next 4. Scope — correctly surgical, and net-negative complexity
On the testsFull suite on this branch: 112 files / 2275 tests, all passing, matching your reported numbers. For transparency: my first run showed 3 failures, but they did not reproduce on a clean re-run (that run took 332s vs. 45s, so it was almost certainly resource contention on my machine from parallel worktree builds, not this PR). I'm calling it green. The one modified existing expectation is correct and worth stating explicitly: - expect(output).toContain('description: \n');
+ expect(output).toContain('description: ""');That's not a weakened test — a bare Nit (optional, no action needed)
Harmless and still correct, but it's the source of most of the cosmetic churn above. Narrowing the regex would reduce the diff users see. That regex is pre-existing on Nice work — this is a well-targeted fix to a genuinely broken output path. |
97cbe7b to
2b63f0f
Compare
…nd table-driven adapter coverage
|
Updated YAML frontmatter serialization so all string values are safely double-quoted and preserve their type across edge cases. Added round-trip coverage for all 24 YAML adapters, including dedicated Kiro and Lingma tests. All 960 command-generation tests pass. |
…nous, and verified
|
Addressed review feedback in commit |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/command-generation/adapters.test.ts (1)
897-905: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale test rationale: "quoting is triggered by another special char" no longer applies.
This test's title/comment describes the old conditional-quoting behavior (backslash alone didn't trigger quoting, only
:did). SinceescapeYamlValuenow always double-quotes and escapes every value unconditionally, the premise is outdated and may confuse future readers about why this assertion holds.📝 Suggested comment update
- it('should escape backslashes when quoting is triggered by another special char', () => { - // Backslash alone does not trigger quoting, but once quoting is on (via ':') - // every backslash must be doubled. Locks the replace(/\\/g, '\\\\') branch. + it('should escape backslashes in description', () => { + // Every value is now unconditionally double-quoted, so backslashes must + // always be doubled. Locks the replace(/\\/g, '\\\\') branch. const output = zcodeAdapter.formatFile({🤖 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 `@test/core/command-generation/adapters.test.ts` around lines 897 - 905, Update the test title and inline comments in the backslash-escaping case within the relevant adapter test to reflect that values are now always double-quoted and backslashes are escaped unconditionally. Keep the existing input and assertion unchanged.
🤖 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.
Nitpick comments:
In `@test/core/command-generation/adapters.test.ts`:
- Around line 897-905: Update the test title and inline comments in the
backslash-escaping case within the relevant adapter test to reflect that values
are now always double-quoted and backslashes are escaped unconditionally. Keep
the existing input and assertion unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 14af19e8-fa20-4755-ba9e-7bed75fba36d
📒 Files selected for processing (9)
src/core/command-generation/adapters/continue.tssrc/core/command-generation/adapters/cursor.tssrc/core/command-generation/adapters/iflow.tssrc/core/command-generation/yaml.tstest/commands/artifact-workflow.test.tstest/core/command-generation/adapters.test.tstest/core/command-generation/generator.test.tstest/core/command-generation/yaml.test.tstest/core/init.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/command-generation/adapters/continue.ts
- src/core/command-generation/adapters/iflow.ts
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 `@skills/openspec-bulk-archive-change/SKILL.md`:
- Around line 132-146: Honor recorded conflict-resolution exclusions by making
delta-spec sync and verification conditional: route excluded, unimplemented
deltas through an explicit “sync skipped” path and allow their archive without
requiring verification, while retaining sync and verification for included
deltas. Apply this in skills/openspec-bulk-archive-change/SKILL.md (132-146),
both corresponding sections of
src/core/templates/workflows/bulk-archive-change.ts (134-146 and 405-417), and
update test/core/templates/skill-templates-parity.test.ts (253-277) to assert
the rendered instructions preserve the skipped-sync path and omit verification
requirements for excluded deltas.
🪄 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 Plus
Run ID: ac99c1df-b10d-4443-a904-b41d07d59e88
📒 Files selected for processing (4)
skills/openspec-bulk-archive-change/SKILL.mdsrc/core/templates/workflows/archive-change.tssrc/core/templates/workflows/bulk-archive-change.tstest/core/templates/skill-templates-parity.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The YAML hold is resolved at 206522d: all 24 YAML adapters now round-trip the scalar edge cases as strings, root-aware inline sync/verification is covered, and build, lint, focused tests, full tests, and generated-skill parity are clean. One blocker remains in bulk archive: step 5 can exclude an unimplemented delta from sync, but step 8 still syncs and verifies every delta unconditionally; please carry the recorded inclusion/exclusion set into execution, sync and verify only included deltas, explicitly report excluded ones as “sync skipped,” and cover the mixed included/excluded path in both generated variants.
|
Addressed the remaining bulk-archive blocker in b3b037b. Both generated variants now carry the per-delta inclusion/exclusion decisions into execution. Only included deltas are synced and verified; excluded deltas are reported explicitly as sync skipped. Added mixed included/excluded regression coverage for both generated variants. |
Resolve conflicts between this branch's bulk-archive rework and main's runtime-context/artifact-rules guidance (Fission-AI#1062) by unioning both sides: - archive-change: keep main's specs-instruction fetch gate; keep this branch's /opsx:sync naming in the opsx command variant. - bulk-archive: step 8a syncs only includedDeltas AND carries main's specs-rule snapshot + artifact-rule scoping bullets; guidelines lists keep both sides' bullets. - Regenerated skills/ and parity hashes rather than hand-merging them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Resolved the conflicts with What conflicted: only the archive/bulk-archive templates and their parity hashes. How it was resolved — union, nothing dropped from either side:
Verified: no |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed at 62c7eab. The YAML serializers round-trip edge-case strings across all adapters, and bulk archive now carries per-delta included/excluded decisions through inline sync, verification, and sync-skipped reporting in both generated variants while preserving current runtime guidance. 1,094 focused tests, build, lint, generated-skill parity, the cross-platform suite, audit, dependency review, and release tracking all pass.
… close test gaps Adversarial review of the merged branch turned up four gaps: - Bulk archive tells the sync workflow to ignore `excludedDeltas`, but main's sync-specs calls `existingOutputPaths` the "complete list" of delta specs. An agent following both would sync the delta the caller withheld, step 8b would not catch it (it verifies only included deltas), and the run would still report `sync skipped`. Sync now honors a caller-supplied subset, mirroring the inline rule-snapshot handoff main already added. - escapeYamlValue left C0/DEL/C1 control characters raw. The repo's own parser accepts them, so tests passed while stricter parsers used by other tools reject the document. Emit them as \xHH. - The adapter matrix was a hand-maintained list driving only `description`, so raw interpolation in lingma's name/category/tags — and any newly registered adapter — passed green. It now derives from the registry and drives every string field. - Four bulk-archive template lines were guarded only by golden hashes, which this repo regenerates as routine. Also corrects the escapeYamlValue docstring, which still described the pre-PR conditional-quoting behavior, and adds the missing changeset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Ran four adversarial review passes over the merged branch (YAML correctness, template integrity, mutation testing, downstream blast radius). Fixes pushed in 62bb846. One real functional defect, introduced by the merge. Step 8a tells the sync workflow to sync only Escaping hardened one class further. Two test gaps that mattered:
Also corrected the Left alone deliberately, as pre-existing and outside this PR:
Full suite 3113 passing, lint and |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed at 62bb846. The latest fixes correctly preserve caller-supplied delta subsets in both sync variants, escape C0/DEL/C1 control characters for strict YAML parsers, and derive adapter/string-field coverage from the registry. Build, lint, 1,112 focused tests, strict spec validation, generated-skill parity, cross-platform CI, audit, dependency review, and release tracking all pass.
A second adversarial pass found the previous fix incomplete. Narrowing
step 3 ("Find delta specs") left step 4 — the loop that actually applies
the changes — still reading "for each capability delta spec path
returned by the CLI". An agent treating step 3 as descriptive and step 4
as operative re-widens to the full list and syncs the delta bulk archive
withheld: the original defect, one step further down the template.
Step 4 now iterates the step-3 selection, and the parity test pins both
the new wording and the absence of the old.
Also from that pass:
- Generalize the carve-out beyond archive. It was conditioned on
"archive invoked this workflow inline", so a user asking /opsx:sync to
sync one delta read as an instruction to ignore them.
- Define the two undefined edges: a named path outside
existingOutputPaths, and an empty named list. Both stop and report
rather than proceeding on a guess.
- Drive control characters through the adapter matrix. It drove none, so
the escaping this suite exists to prove had no adapter-level coverage
and the raw-CR assertion could never fail. Verified live by mutation.
- Give contentDerivedFields two markers that differ in length and shape.
Same-shaped markers render identically for a length- or slice-derived
field, which would drop it from every assertion silently.
Drops the `not.toContain('complete list of delta spec files')`
assertion: it banned one exact synonym while any reword of the same
conflicting instruction passed, so it read as coverage without being it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Second adversarial round, on the previous round's own fixes rather than the original diff. Pushed as 5bfd574. The earlier fix was incomplete, in the same way and for the same reason. Narrowing step 3 ("Find delta specs") and the guardrails left step 4 — the loop that actually applies the changes — still reading "For each capability delta spec path returned by the CLI." An agent that reads step 3 as descriptive and step 4 as operative re-widens to the full list and syncs the delta bulk archive withheld. Same wrong outcome as the original defect, one step further down the template. Step 4 now iterates the step-3 selection, and the parity test pins both the new wording and the absence of the old, verified by mutation. Two more from that pass:
The control-character escaping was independently validated against js-yaml, PyYAML and the repo's Test hardening, both found by mutating the previous round's tests:
Exhaustive mutation of the rebuilt matrix — every adapter × every content-derived field × all cases — found 0 survivors, plus 14 in-tree mutations all killed. I also dropped the Non-blocking, not fixed here: sync's success output has no place to list withheld paths, so a caller can't cross-check what was skipped; and 3257 tests passing, lint and |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed at 5bfd574. The sync workflow now carries the step-3 delta selection into the operative step-4 loop, stops on out-of-scope or empty named subsets, and supports caller narrowing beyond archive without widening back to the CLI list. The adapter matrix now exercises control characters and uses shape/length-distinct markers. Build, lint, 1,256 focused tests, strict spec validation, generated-skill parity, cross-platform CI, audit, dependency review, and release tracking all pass.
3 participants
Description
Centralize and enforce YAML frontmatter value escaping across all tool command adapters.
Root Cause
Command adapters emitting YAML frontmatter (
amazon-q,antigravity,auggie,bob,codebuddy,continue,costrict,crush,factory,github-copilot,iflow,junie,kiro,lingma,opencode,qoder,qwen,trae,zcode) either omitted string escaping or re-implemented local un-escaped/duplicate variants ofescapeYamlValue. When command descriptions, names, categories, or tags contained YAML special characters (e.g. colons:, quotes", newlines\n, carriage returns\r, or brackets[]), generated command files contained invalid YAML frontmatter that failed YAML parsers.Fix Details
escapeYamlValueandformatTagsArrayfromsrc/core/command-generation/yaml.tsas the single authoritative helper module.escapeYamlValueto return""when given an empty string so empty fields produce valid YAML scalars instead of unquoted trailing whitespace.amazon-q,antigravity,auggie,bob,codebuddy,continue,costrict,crush,factory,github-copilot,iflow,junie,kiro,lingma,opencode,qoder,qwen,trae,windsurf,zcode) to import and use sharedescapeYamlValueandformatTagsArrayhelpers.test/core/command-generation/adapters.test.tsverifying that frontmatter emitted across all YAML adapters parses cleanly withyaml.parse.Verification
pnpm test(112 test files passed, 2275 tests passed)pnpm run build(Clean build completion)pnpm run lint(0 ESLint errors)pnpm tsc --noEmit(0 type errors)git diff --check(Clean formatting, no trailing whitespace)Summary by CodeRabbit
Bug Fixes
""and tag frontmatter values are emitted in a consistent YAML-safe array format.Tests
Documentation