fix(artifacts/spec): tolerate YAML-style SPEC drift (#7)#10
Conversation
The SPEC.md parser was strict — exactly six `## ` H2 sections in canonical order, bullets-only bodies, no other content shapes — with zero variant tolerance and no pre-parse adapter. The DEFINE-phase artifact is the very first artifact in the SDLC, so any drift between the BA persona's actual output and the strict schema crashes the run before PLAN. Same bug class as #5 (HYPOTHESES YAML) and #3 (SOURCE_CHECK REF-NONE) — both of which surfaced during the recent friend demo. Two layers of root cause + fix: 1. Persona: src/agents/defaults/ba.md described the protocol in prose ("produce the complete SPEC.md draft in the canonical format") but carried no concrete schema example. With no anchor, the BA persona could default to YAML — top-level `goals:` / `users:` / `acceptance_criteria:` keys with indented `- bullet` list values — exactly the drift pattern the Scientist persona exhibited for HYPOTHESES.md before #5. Add a "Canonical schemas (read before emitting)" section with explicit wrong / right examples and the locked rules (six section spellings, bullets-only bodies, the empty open-questions sentinel, H1 form). 2. Parser: src/artifacts/spec.ts had no pre-parse adapter. Add `adaptYamlStyleSpec(raw)` that pre-rewrites column-0 YAML keys (case-insensitive, with snake_case / camelCase / kebab-case aliases) followed by indented `- bullet` lines or inline flow lists into canonical `## Heading\n\n- bullet` Markdown sections. Mixed format input (some YAML, some canonical) is supported — only recognised YAML keys at column 0 are rewritten. Recognised aliases (case folded): goals, users, constraints, acceptance / acceptance_criteria / acceptanceCriteria / acceptance-criteria, open_questions / openQuestions / open-questions, non_goals / nonGoals / non-goals / explicit_non_goals / explicitNonGoals / explicit-non-goals. Discipline boundary: the adapter fires only on lines at column 0 that match a recognised key. Canonical `## Heading` sections, the `# SPEC` H1, indented content, and unknown keys all pass through verbatim. The strict parser still owns final validation; an unrecognised top-level YAML key falls through to spec_unexpected_content. Tests: 13 new regression cases covering (a) YAML→canonical rewrite of every supported alias, (b) inline flow-list values, (c) snake_case / camelCase / kebab-case key normalisation, (d) preservation of bullet text, (e) mixed-format input, (f) end-to-end pure-YAML parsing, (g) round-trip serialize → reparse to canonical, (h) unchanged output for canonical input, (i) still-rejects-missing-section after rewrite, (j) still-rejects-unknown-key after rewrite, (k) accepts the empty open-questions sentinel via YAML. Full offline suite: 2181 pass / 0 fail / 1 skip (gated live xAI). Note: this fix is proactive defense — the DEFINE-phase YAML drift has not been observed in the wild yet, but the bug class is the same as #3 and #5 and the BA persona's prompt had the same pre-#5 vagueness. Better to ship the two-layer protection before the next demo than after. Closes #7
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR addresses demo-crash risk by adding YAML-style tolerance to the SPEC artifact parser. It includes prompt guidance for canonical Markdown format, a pre-parse adapter that rewrites YAML keys to Markdown sections, and comprehensive test coverage validating both input formats. ChangesSPEC YAML Tolerance
Sequence DiagramsequenceDiagram
participant Agent as BA Agent
participant Parser as parseSpec()
participant Adapter as adaptYamlStyleSpec()
participant Validator as Strict Parser
participant Artifact as SpecArtifact
Agent->>Parser: emit YAML-style SPEC
Parser->>Adapter: raw YAML input
Adapter->>Adapter: detect top-level keys<br/>(goals:, acceptance_criteria:, etc.)
Adapter->>Adapter: rewrite keys to<br/>## Markdown headings
Adapter->>Adapter: normalize key aliases<br/>(snake_case/camelCase/kebab-case)
Adapter->>Adapter: convert lists to<br/>dash bullets
Adapter->>Parser: return canonical Markdown
Parser->>Validator: canonical # SPEC text
Validator->>Validator: strict H1, six H2,<br/>bullets-only validation
Validator->>Artifact: ✓ typed SpecArtifact
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 33 minutes and 47 seconds.Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a tolerance layer for SPEC.md files generated by LLMs in YAML format. It adds documentation, a pre-parsing adapter that converts YAML-style keys and lists into canonical Markdown H2 sections, and extensive tests. The review feedback identifies several improvement opportunities: stripping the Byte Order Mark (BOM) before adaptation to prevent regex mismatches, broadening the initial format probe to support bare inline values, and refining the comma-splitting logic for inline lists to handle quoted values containing commas.
| const adapted = adaptYamlStyleSpec(raw) | ||
| const text = adapted.startsWith(BOM) ? adapted.slice(BOM.length) : adapted |
There was a problem hiding this comment.
The BOM should be stripped before calling adaptYamlStyleSpec. If the input starts with a BOM and the first section is in YAML style (e.g., if the # SPEC title is missing or if the adapter were to be used on other artifacts), the YAML_SPEC_KEY_PROBE and the line-by-line matching will fail to recognize the first key because of the leading BOM character. Stripping it first ensures the adapter works correctly regardless of the file encoding.
const textWithoutBom = raw.startsWith(BOM) ? raw.slice(BOM.length) : raw\n const text = adaptYamlStyleSpec(textWithoutBom)| // by a colon, with no leading `## ` Markdown heading marker. Matches every | ||
| // alias spelling in YAML_SPEC_KEY_MAP via a single regex so we can short | ||
| // circuit when the input is already canonical Markdown. | ||
| const YAML_SPEC_KEY_PROBE = /^(?:goals|users|constraints|acceptance(?:[ _-]?criteria)?|open[ _-]?questions|(?:explicit[ _-])?non[ _-]?goals):\s*(?:\[.*\])?\s*$/im |
There was a problem hiding this comment.
The YAML_SPEC_KEY_PROBE regex is too restrictive to support "comma-separated bare values" as mentioned in the parseInlineList comments (line 127). Currently, it only matches lines that are either just the key (for indented lists) or the key followed by a bracketed flow list. A bare value like goals: my goal will fail the probe, causing the adapter to return early and the strict parser to fail. Consider broadening the probe to allow non-bracketed inline content.
| const YAML_SPEC_KEY_PROBE = /^(?:goals|users|constraints|acceptance(?:[ _-]?criteria)?|open[ _-]?questions|(?:explicit[ _-])?non[ _-]?goals):\s*(?:\[.*\])?\s*$/im | |
| const YAML_SPEC_KEY_PROBE = /^(?:goals|users|constraints|acceptance(?:[ _-]?criteria)?|open[ _-]?questions|(?:explicit[ _-])?non[ _-]?goals):\s*(?:\[.*\]|[^\s\[].*)?\s*$/im |
| const flow = trimmed.match(/^\[(.*)\]$/) | ||
| const inner = flow !== null ? flow[1]! : trimmed | ||
| return inner | ||
| .split(',') |
There was a problem hiding this comment.
The split(',') approach in parseInlineList is naive and will incorrectly split values that contain commas, even if they are enclosed in quotes (e.g., [ "Goal 1, with comma", "Goal 2" ]). While this is a tolerance layer for LLM output, LLM-generated content frequently contains commas within descriptive bullets. Consider a more robust parsing approach if complex values are expected.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 971af9c106
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (indented === null) { | ||
| // Indented continuation that isn't a bullet — drop it; the strict | ||
| // parser would have rejected the same content anyway. | ||
| i++ | ||
| continue |
There was a problem hiding this comment.
When a YAML-style section contains an indented continuation line (for example - first line followed by second line), this branch drops the continuation and still returns a valid SPEC. In parseSpec this means DEFINE can now succeed with silently truncated requirements, whereas previously the draft would fail validation and trigger a repair turn. This is a data-loss regression for any wrapped/multiline YAML bullets, and it can change acceptance criteria or constraints without surfacing an error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds proactive tolerance for YAML-style drift in the DEFINE-phase SPEC.md artifact, aligning with the established “prompt guidance + parser adapter + regression tests” pattern used in prior artifact validators.
Changes:
- Updated BA persona instructions with explicit canonical
SPEC.mdschema rules and wrong/right examples. - Added
adaptYamlStyleSpec(raw)to rewrite recognized YAML-style section keys/lists into canonical Markdown sections before strict parsing. - Expanded SPEC artifact test coverage with new YAML-tolerance regression cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/artifacts/spec.ts |
Adds YAML-to-canonical Markdown pre-parser adapter and wires it into parseSpec() before strict validation. |
tests/artifacts-spec.test.ts |
Adds regression tests for YAML-style input adaptation and end-to-end parsing/round-tripping. |
src/agents/defaults/ba.md |
Adds canonical schema guidance block with concrete wrong/right output examples and locked formatting rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SPEC.md is **plain Markdown with `# SPEC` as the H1 and exactly six `## ` H2 sections, each containing only dash bullets and blank lines**. It is NOT YAML. Do not emit YAML keys (`goals:`, `users:`, `acceptance_criteria:`) with indented list values — the parser rejects them. | ||
|
|
||
| Wrong (YAML-style — parser rejects): |
| const indented = cont.match(/^[ \t]+-\s*(.*)$/) | ||
| if (indented === null) { | ||
| // Indented continuation that isn't a bullet — drop it; the strict | ||
| // parser would have rejected the same content anyway. | ||
| i++ | ||
| continue | ||
| } |
| // Probe: column-0 line starting with a recognised SPEC section key followed | ||
| // by a colon, with no leading `## ` Markdown heading marker. Matches every | ||
| // alias spelling in YAML_SPEC_KEY_MAP via a single regex so we can short | ||
| // circuit when the input is already canonical Markdown. | ||
| const YAML_SPEC_KEY_PROBE = /^(?:goals|users|constraints|acceptance(?:[ _-]?criteria)?|open[ _-]?questions|(?:explicit[ _-])?non[ _-]?goals):\s*(?:\[.*\])?\s*$/im |
| // Accepts `[a, b, c]` flow-style YAML or comma-separated bare values. | ||
| const trimmed = value.trim() | ||
| if (trimmed.length === 0) return [] | ||
| const flow = trimmed.match(/^\[(.*)\]$/) | ||
| const inner = flow !== null ? flow[1]! : trimmed | ||
| return inner | ||
| .split(',') | ||
| .map((s) => s.trim().replace(/^["']|["']$/g, '')) | ||
| .filter((s) => s.length > 0) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/agents/defaults/ba.md`:
- Around line 51-98: Update the two fenced code blocks that begin with "```" and
contain the "# SPEC" example in ba.md to use a language tag by changing the
opening fence to "```markdown" for both occurrences so the blocks are lint-clean
(MD040) and preserve syntax highlighting; locate the exact fenced examples that
show the SPEC block and add the "markdown" tag to their opening fences.
In `@src/artifacts/spec.ts`:
- Around line 113-123: The YAML_SPEC_KEY_PROBE is too narrow and causes
adaptYamlStyleSpec to skip inputs that normalizeSpecKey/YAML_SPEC_KEY_MAP can
handle (e.g., explicitNonGoals:, single-line scalars like "goals: only goal"),
so broaden the probe to match every alias in YAML_SPEC_KEY_MAP (or at least any
key that matches YAML_SPEC_KEY_LINE and whose normalized form exists) before
returning early; update YAML_SPEC_KEY_PROBE (or the early-check logic in
adaptYamlStyleSpec) to either build an alternation from YAML_SPEC_KEY_MAP keys
or to test the line against YAML_SPEC_KEY_LINE then call normalizeSpecKey to
decide whether to proceed, ensuring parseInlineList/parseSpec receive inputs the
adapter claims to support.
- Around line 188-205: In the while-loop that parses indented continuations
(variables lines, cont, and the indented match), do not silently drop cases
where indented === null; instead preserve the original cont line in the adapted
output for the current section or mark the whole section adaptation as invalid
and skip/adapt accordingly; update the branch handling indented === null
(currently just i++ and continue) to either append cont to the current section
buffer (so malformed YAML lines like "note: bad" are retained) or set a flag
(e.g., sectionInvalid) and abort adaptation for that block so the strict parser
will later reject it; ensure this change is applied where the code checks /^[
\t]+-\s*(.*)$/ and adjust subsequent logic that consumes i to keep iteration
consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f651b0a2-5ed7-4c7d-9b25-8789fc80b4a9
📒 Files selected for processing (3)
src/agents/defaults/ba.mdsrc/artifacts/spec.tstests/artifacts-spec.test.ts
| ``` | ||
| # SPEC | ||
|
|
||
| goals: | ||
| - Help a parent name their newborn. | ||
| - Suggest names balanced across given-name and surname pairings. | ||
| users: | ||
| - New parents with a fixed surname. | ||
| constraints: | ||
| - Runs locally on a phone-class device. | ||
| acceptance_criteria: | ||
| - Given a surname, the app produces 5 candidate given names. | ||
| open_questions: | ||
| - Does the parent want gender-neutral suggestions only? | ||
| explicit_non_goals: | ||
| - Not building a name registry. | ||
| ``` | ||
|
|
||
| Right (Markdown H2 sections — parser accepts): | ||
|
|
||
| ``` | ||
| # SPEC | ||
|
|
||
| ## Goals | ||
|
|
||
| - Help a parent name their newborn. | ||
| - Suggest names balanced across given-name and surname pairings. | ||
|
|
||
| ## Users | ||
|
|
||
| - New parents with a fixed surname. | ||
|
|
||
| ## Constraints | ||
|
|
||
| - Runs locally on a phone-class device. | ||
|
|
||
| ## Acceptance criteria | ||
|
|
||
| - Given a surname, the app produces 5 candidate given names. | ||
|
|
||
| ## Open questions | ||
|
|
||
| - Does the parent want gender-neutral suggestions only? | ||
|
|
||
| ## Explicit non-goals | ||
|
|
||
| - Not building a name registry. | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced examples.
Both new fences are already tripping MD040. Tagging them as markdown keeps the prompt lint-clean and preserves syntax highlighting.
Suggested diff
-```
+```markdown
# SPEC
...- +markdown
SPEC
...
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agents/defaults/ba.md` around lines 51 - 98, Update the two fenced code
blocks that begin with "```" and contain the "# SPEC" example in ba.md to use a
language tag by changing the opening fence to "```markdown" for both occurrences
so the blocks are lint-clean (MD040) and preserve syntax highlighting; locate
the exact fenced examples that show the SPEC block and add the "markdown" tag to
their opening fences.
Codex review (verdict fix-first) flagged two semantic-corruption risks
that violate the "rewrite shape, not semantics" discipline boundary —
exactly the bug class the parser-tolerance work is fixing. Plus three
fix-soon items.
Block-push 1 — quote-aware flow-list split:
`parseInlineList` used naive `inner.split(',')` which split inside
quoted scalars: `goals: ["a, b", c]` produced `['a', 'b', 'c']`,
turning one author-quoted scalar into two accepted bullets. Add
`splitTopLevelCommas` that respects single- and double-quoted strings
so `"a, b"` and `'a, b'` survive as single items. Trailing commas now
drop the empty trailing item.
Block-push 2 — folded YAML continuation lines preserved, not dropped:
The previous `if (indented === null) { i++; continue }` silently
dropped indented-but-not-bullet lines. A YAML list with a folded
multi-line scalar (`- First line\n continuation`) lost the
continuation text — silent author-content deletion. Fix: append the
trimmed continuation onto the previous bullet so the author's intent
survives the rewrite. If there is no previous bullet (unusual; YAML
key with continuation but no leading `-`), push the text as its own
bullet rather than dropping.
Fix-soon 1 — probe/map asymmetry on `non goals`:
`YAML_SPEC_KEY_PROBE` matched `non goals:` via `non[ _-]?goals` but
`YAML_SPEC_KEY_MAP` lacked the `'non goals'` entry. The unknown-key
fallthrough was safe (strict parser rejects), but the probe and map
should agree. Add `'non goals': 'Explicit non-goals'`.
Fix-soon 2 — narrow flow-list test coverage:
Add regression tests for quoted commas (single + double quotes),
trailing commas, YAML continuation folding, the `non goals` alias,
and BOM-prefixed input with YAML sections.
Nit — ba.md persona prompt phrasing:
The "Wrong (... — parser rejects):" heading was technically false
after this PR adds parser tolerance. Rephrase to "Wrong (YAML-style —
emit canonical Markdown instead):" + "Right (Markdown H2 sections —
canonical contract):" so the prompt remains strict about author
intent without mis-stating implementation behaviour.
Tests: 6 new regression cases covering each block-push and fix-soon
finding. Full offline suite: 2187 pass / 0 fail / 1 skip.
Codex review trail: /tmp/codex_review_pr10.md
PR #10's Codex review surfaced two block-push semantic-corruption risks in `adaptYamlStyleSpec` that violate the "rewrite shape, not semantics" discipline boundary. The same code shape was copied into `adaptYamlStylePlan` and `adaptYamlStyleSourceCheck` in this PR's initial commit, so both bugs propagate here. Mirror the spec.ts fixes verbatim: 1. Quote-aware top-level comma splitter — `splitTopLevelCommasPlan` / `splitTopLevelCommasSourceCheck` replace naive `inner.split(',')` so quoted scalars containing commas (`["a, b", c]`) survive as single items. Trailing commas drop the empty trailing element. 2. Folded YAML continuation lines preserved — the previous `if (indented === null) { i++; continue }` silently dropped indented-but-not-bullet lines (folded multi-line scalars). Replace with: append the trimmed continuation onto the previous bullet so author content survives the rewrite. If there is no previous bullet (unusual; YAML key with continuation but no leading `-`), push the text as its own bullet rather than dropping. 3. lead.md persona phrasing — match the ba.md update in PR #10. Replace "They are NOT YAML. ... the parsers reject both forms." with the Codex-suggested phrasing: "The canonical contract is Markdown — emit Markdown, not YAML. The parsers include a narrow YAML-tolerance fallback for accidental section-level drift, but you must produce canonical Markdown by default. Nested `- id: T-NNN` / `- id: SC-NNN` block-style entries are rejected outright by the strict parser." This stays strict about author intent without mis-stating implementation behaviour. Tests: 4 new regression cases (2 PLAN + 2 SOURCE_CHECK): - Quote-aware flow-list split for PLAN.md goals. - Folded YAML continuation for PLAN.md goals. - Quote-aware flow-list split for SOURCE_CHECK.md Coverage. - Folded YAML continuation for SOURCE_CHECK.md Open questions. Full offline suite: 2192 pass / 0 fail / 1 skip. This commit lands BEFORE Codex review of PR #11, applying PR #10's review lessons preemptively because the bug class is shared.
…n PR #10 Codex round-2 review (verdict fix-first) flagged two new semantic-corruption risks not surfaced by round-1 plus two fix-soon items. Round-1 closures all confirmed except for two partial cases. Block-push 1 — escape-aware comma splitter: `splitTopLevelCommas` toggled quote state on every `"`, so an escaped double quote inside a flow scalar (`["say \"yes, now\"", second]`) closed the quote state at `\"` and treated the next comma as a top-level delimiter — three bullets emerged from one author-quoted scalar. Add a backslash-escape path that preserves the next character verbatim and skips quote toggling. Honors `\"` in double-quoted scalars and `\'` in single-quoted scalars. Block-push 2 — nested YAML rejected, not flattened: The previous `if (indented === null)` branch folded any indented-but-not-bullet line onto the previous bullet, including indented `key: value` (nested map) and deeper-indent `- bullet` (nested list). The bullet regex matched `-` at any indentation, so `goals:\n - first\n nested:\n - sub1` flattened a nested map + list into the canonical first bullet — silent author-content corruption, exactly the bug class issue #7 is fixing. Restructure the bullet-collection loop with three rejection conditions: 1. Indented `key:` (no leading `-`) → nested map → abort rewrite. 2. Indented `- bullet` at deeper indent than the first bullet → nested list → abort rewrite. 3. Otherwise indented non-bullet text → folded continuation → append to previous bullet (preserved from round-1 fix). When any rejection condition fires, the YAML block's collected lines are emitted verbatim instead of the canonical heading/bullets so the strict parser surfaces the unrewritten YAML structure. Fix-soon 1 — explicit/nongoals probe asymmetry: Probe was `(?:explicit[ _-])?non[ _-]?goals` — required a separator after `explicit`, so `explicitnongoals:` skipped the adapter even though `explicitnongoals` is in YAML_SPEC_KEY_MAP. Make the separator optional: `(?:explicit[ _-]?)?non[ _-]?goals`. Closes the probe-vs-map asymmetry; closed regression added. Fix-soon 2 — round-trip stability with comma-containing scalar: Add `parseSpec → serializeSpec → parseSpec` coverage for a goal like `a, b` so canonical `- a, b` stability is protected against future regression in the splitter or serializer. Tests: 6 new regression cases covering each round-2 finding (2 escape cases for double + single quoted scalars, 2 nested-rejection cases for nested map + nested list, 1 explicitnongoals probe match, 1 comma-scalar round-trip). Full offline suite: 2193 pass / 0 fail / 1 skip. Codex round-2 review trail: /tmp/codex_review_pr10_round2.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99e9d8866f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const inner = flow !== null ? flow[1]! : trimmed | ||
| return splitTopLevelCommas(inner) | ||
| .map((s) => s.trim().replace(/^["']|["']$/g, '')) |
There was a problem hiding this comment.
Treating every non-flow inline value as a comma-delimited list silently changes SPEC meaning: goals: Improve speed, reliability is valid YAML for one scalar, but this code rewrites it into two bullets (Improve speed and reliability). That introduces data corruption in DEFINE (requirements are altered without any validation error), which is worse than the prior fail-fast behavior.
Useful? React with 👍 / 👎.
| // The `[ _-]?` after `explicit` is optional so the probe matches | ||
| // concatenated forms like `explicitnongoals` and `explicitNonGoals` | ||
| // (which the map already accepts) — closes a round-2 fix-soon asymmetry. | ||
| const YAML_SPEC_KEY_PROBE = /^(?:goals|users|constraints|acceptance(?:[ _-]?criteria)?|open[ _-]?questions|(?:explicit[ _-]?)?non[ _-]?goals):\s*(?:\[.*\])?\s*$/im |
There was a problem hiding this comment.
The pre-check only matches keys with an empty value or bracketed flow list, so YAML documents written as inline scalars (for example goals: Goal text for each section) bypass adaptation entirely and still fail strict parsing with missing-section/unexpected-content errors. This is inconsistent with the adapter logic that already handles single bare inline values, and leaves a common YAML drift form un-tolerated.
Useful? React with 👍 / 👎.
PR #10 round-2 Codex review (verdict fix-first) found two new block-push items in spec.ts not surfaced by round-1: escape-aware splitter + nested-YAML rejection. The same code shape exists in plan.ts and source-check.ts, so the fixes mirror here. Plus PR #11's own round-1 review (verdict push) flagged one fix-soon and one nit worth closing now rather than as a follow-up. Mirror of PR #10 round-2 block-push fixes: 1. Escape-aware comma splitter — `splitTopLevelCommasPlan` and `splitTopLevelCommasSourceCheck` get the backslash-escape path so `"\"yes, now\""` no longer toggles quote state at the inner `\"` and splits at the wrong comma. 2. Nested YAML rejection — both adapters' bullet-collection loops now detect nested maps (indented `key:` with no leading `-`) and nested lists (deeper-indent `- bullet`) and abort the rewrite of the YAML block, emitting collected lines verbatim so the strict parser surfaces the unrewritten structure rather than flattening nesting into bullets. Closures of PR #11 round-1 findings: 3. PLAN probe/map alignment (fix-soon) — `YAML_PLAN_KEY_PROBE` previously used `out[ _-]?of[ _-]?scope` and `open[ _-]?questions` which match mixed-separator forms (`out_of-scope:`, `out-ofscope:`) that aren't keys in `YAML_PLAN_KEY_MAP`. Replace with explicit alternation matching exactly the four canonical forms each so the probe and map are fully aligned. 4. Persona prompt phrasing (nit) — `lead.md` example labels said "Wrong (YAML-style — parser rejects)" but the parser tolerates section-level YAML; only nested block forms are rejected. Replace with "Wrong (YAML-style — emit canonical Markdown instead; section-level keys hit the narrow tolerance fallback, and the nested `- id: ...` block form is rejected outright)" and "Right (Markdown sections + H3 ... blocks — canonical contract)" to match ba.md's pattern and avoid mis-stating implementation. NOT addressed in this commit (deferred per Codex's recommendation): - DRY-at-3x extraction of the shared splitTopLevelCommas / parse- inline-list helpers. Codex flagged this as a fix-soon but recommended deferring until both PR #10 and PR #11 land so the shape is stable and no PR has to coordinate with another's review cycle. Will land as a follow-up `refactor(artifacts): extract shared YAML-tolerance helpers` commit on main once both merge. Tests: 4 new regression cases (2 PLAN + 2 SOURCE_CHECK): - Quote-escape regression for PLAN goals. - Nested-YAML rejection for PLAN goals. - Quote-escape regression for SOURCE_CHECK Coverage. - Nested-YAML rejection for SOURCE_CHECK Coverage. Full offline suite: 2196 pass / 0 fail / 1 skip. Codex review trails: /tmp/codex_review_pr10_round2.md, /tmp/codex_review_pr11.md
Three artifact validators (spec.ts, plan.ts, source-check.ts) carried byte-identical copies of `splitTopLevelCommas` and `parseInlineList` (named with `*Plan` / `*SourceCheck` suffixes for readability). DRY-at-3x threshold per CLAUDE.md global rule. Codex review on PR #11 round-1 flagged this as fix-soon and recommended deferring to a follow-up after PR #10 + PR #11 merged so the helper shape would be stable across all three call sites. Behavior-preserving consolidation: - New `src/artifacts/yaml-tolerance.ts` exports `splitTopLevelCommas` and `parseInlineList` (canonical bodies preserved verbatim). - Each parser drops its local copy and imports `parseInlineList` from the shared module. `splitTopLevelCommas` stays internal-use-only via parseInlineList today; exported for future consumers. Test fixture fix (same commit): `escaped double quote in Coverage flow scalar does not corrupt split` only exercised quoted-comma split — the fixture lacked a real `\"`. Coverage rows are strictly formatted (`T-NNN -> SC-X-NNN`) and cannot carry escaped quotes, so the test now adds an `open_questions:` flow scalar with a real `\"` (matching the PLAN equivalent at tests/artifacts-plan.test.ts:537) and asserts both quoted-comma split (Coverage) and escape handling (Open questions). Renamed to mirror PLAN's `escaped double quote in flow scalar does not corrupt comma split`. Tests: 2221 pass / 0 fail / 1 skip preserved.
Summary
Same two-layer fix pattern as PR #6 (issue #5 HYPOTHESES YAML tolerance), applied proactively to the SPEC validator — the DEFINE-phase artifact and the highest-stakes parser-tolerance gap surfaced by the artifact-validator audit after the friend demo.
src/agents/defaults/ba.mdgets a "Canonical schemas (read before emitting)" block with explicit wrong / right examples + locked SPEC.md rules.src/artifacts/spec.tsgetsadaptYamlStyleSpec(raw)— a narrow pre-parser that rewrites column-0 YAML keys (case-folded, with snake_case / camelCase / kebab-case aliases) plus indented- bulletlists or inline flow lists into canonical## Heading\n\n- bulletMarkdown sections before strict parsing.artifacts-spec.test.ts).Why proactive
The audit triggered by issues #3 and #5 found three more validators in the same risk class — strict Markdown parsers vs. flexible LLM output. SPEC is the highest priority because it's the very first artifact in the SDLC; any drift here crashes the run before PLAN, exactly the "demo crashes immediately" failure mode that hurts first impressions. See issue #7 body for the full risk analysis.
The DEFINE-phase YAML drift has not been observed yet, but:
Test plan
bun run typecheckcleanCloses #7
Summary by CodeRabbit