Repository navigation
fix(config): name the offending rules item when a list is malformed - #1984
Conversation
A rule item containing an unquoted ": " is valid-looking YAML but parses as a mapping, so the artifact's whole rule set is dropped. The warning named only the artifact, so the bad item had to be found by bisecting the list by hand. Name every offending index with the shape YAML produced there, and point at the quoting fix when a mapping is the cause. Parsing behavior is unchanged.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: Fission-AI/OpenSpec/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughArtifact rule warnings now describe invalid YAML values and identify each non-string list item by index and shape. For a mapping item, the warning includes a quoting hint. Tests cover malformed items and valid sibling rules. ChangesArtifact rule diagnostics
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change improves malformed-rule warnings without changing which artifacts load; valid sibling rules remain available. No merge-blocking concern is evident, subject to normal checks. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 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 |
|
Maintainer validation on 0c47e15:
Ready for maintainer approval and merge. |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Approved at 0c47e15. The diagnostic now identifies every malformed rules item by zero-based index and parsed YAML shape, adds the useful quoting fix for mappings, and leaves parsing behavior unchanged. The focused project-config suite passes locally: 57 tests.
…ommands Rebased onto main, where Fission-AI#1984 already names the offending rules item but keeps dropping the whole artifact. This PR's per-item parsing supersedes it, so the Fission-AI#1984 tests now assert the per-item result and its leftover helpers are removed. Hardening for configs every command already tolerates: - An empty or comment-only config.yaml parses to null. It was reported as "not a valid YAML object" and failed `validate --all`; it is now treated like a missing config. - A key left empty in YAML (`rules:` with its entries commented out, a bare `-` item, `context:`) and empty-string rules or guidance lose nothing the author wrote, so they are warnings (fail only under --strict), not errors. - Unknown top-level keys are reported only by inspectProjectConfig (validate). readProjectConfig keeps ignoring them silently, so other commands do not start printing new stderr warnings. Documents the config check under `openspec validate` in docs/cli.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4 participants
Summary
A rule item containing an unquoted
": "is valid-looking YAML, but YAML parses it as a mapping. The list then stops being an array of strings, the artifact's entire rule set is dropped, and the only signal is a warning on stderr that names the artifact but not the item — so the bad entry has to be found by bisecting the list by hand.This takes the first of the three suggestions in #1891 and makes the warning point at the exact item and how to fix it:
Every offending index is named with the shape YAML produced there (
a mapping,a number,a nested list,null), so a list with more than one bad item doesn't need re-bisecting either.Parsing behavior is unchanged — the malformed artifact's rules are still dropped wholesale, and well-formed sibling artifacts are still loaded. Only the warning text is more precise, so this is a diagnostic fix and not a change of contract. Deciding whether
validateshould surface config problems too, and whether a single bad item should still take out its siblings, are the other two suggestions from #1891; those are behavior changes and belong in their own PR after the maintainers weigh in.Closes #1891
Changes
src/core/project-config.ts— newdescribeRulesShapeError()builds the warning anddescribeYamlType()names a value's YAML shape. The existingconsole.warncall site is unchanged apart from delegating to the helper, so the original sentence is still a prefix of every message.test/core/project-config.test.ts— two regression tests: the reported mapping case (asserting the drop behavior is unchanged alongside the new index/shape/hint in the message), and a mixed list with two mappings and a number. The pre-existing "filter out invalid rules" test gained one assertion for the non-list case..changeset/tidy-moons-tap.md— patch changeset.Testing
pnpm exec tsc --noEmit,pnpm lint, andpnpm buildare clean.test/core/project-config.test.tspasses at 57/57.I reproduced the report against a built CLI first, and confirmed the advice is correct: on the issue's exact
config.yaml,new changeprinted the improved warning namingrules.proposal[0], and after quoting that scalar the same config loads all 3 rules with no warning.Full
pnpm teston this branch: 5937 passed, 4 failed. All 4 are unrelated to this change:test/package-install-scripts.test.ts(3) — runnpm installagainst the network. Confirmed failing identically on a cleanmainwith my changes stashed.test/commands/artifact-workflow.test.ts(1) andtest/commands/store-references.test.ts(1) — 10s timeouts under full-suite parallel load. Both pass in isolation with this branch built.AI Disclosure
Written with OpenCode (
opencode/space-bunny-free). I ran the gate above and verified the before/after against a real CLI build rather than only through the unit tests.Summary by CodeRabbit
:can prevent an item from being interpreted as a mapping.