Table Pipe Escaping

[FrankRay78]

Why

The at-a-glance table that #281 added to /speckit.reviewissue is a Markdown table. A literal | in a gap title or consequence cell silently splits the row into the wrong columns. It shows as a misaligned table, not an error. The @claude review on #284 caught this after merge.

What changes

• Every | in a table cell's content is written \|, including inside inline code. Gap titles may still contain |, and a title cell that differs only by the escape still counts as verbatim. Nothing outside the table is escaped.
• The pre-post self-check gains a clause for unescaped pipes in the table.

Non-obvious things a reviewer should know

• Inline code needs the escape too. GitHub splits a row into columns before it renders code spans. Rendered via gh api markdown (gfm): `A | B` in a cell lost the third column, while `A \| B` renders as A | B.
• Re-runs check only the row they change. When a re-run edits an existing review, the pipe check covers just that row. The re-run rules require every other row to pass through byte-for-byte, and migrating already-posted comments is out of scope. This came from the /verify review.
• No CI gate. The rule governs how a review comment is composed, which nothing in the repo runs, so it stays a self-check, as the issue's confirmed decisions say.

Related

Closes #286

• Make a /speckit.reviewissue comment triageable, and stop the category sweep inflating it #281 / Triageable Review #284 — added the table.

🤖 Generated with Claude Code

https://claude.ai/code/session_016g3xHRn9ChW4mcjvduzB2U

[FrankRay78]

@claude Review this pull request. Analyse the code changes and provide feedback covering:

• Bugs or correctness issues
• Security concerns (including any flagged by static analysis)
• Adherence to the project conventions in CLAUDE.md
• Test coverage — do the tests adequately cover the new behaviour?
• Any spec/test-plan mismatches

End the review with a recommendation whether to merge the PR, and if not, what you suggest needs addressing first.

[claude]

Claude finished @FrankRay78's task in 2m 29s —— View job

---

Review: Table Pipe Escaping (#288)

• Gather context (diff, file, related issue)
• Analyse correctness / bugs
• Check security concerns
• Check CLAUDE.md / constitution adherence
• Check test coverage
• Check spec/test-plan mismatches
• Post final review with recommendation

---

Scope: this changes only .claude/commands/speckit.reviewissue.md (prompt instructions for an internal spec-kit slash command) — no application code (NetPace.Core/NetPace.Console) or tests are touched.

Bugs / correctness

• The added escaping rule (line 215) and the self-check clause (line 223) are internally consistent and close the actual gap: "column dividers stay bare" correctly disambiguates structural | from content |, and the note that a title cell "differs from the gap only by that escape and still counts as verbatim" pre-empts a real contradiction with the pre-existing "matches the gap's title verbatim" rule one sentence earlier.
• The claim that GFM splits table columns on raw text before inline/code-span rendering (so `A \| B` renders as A | B rather than showing the backslash) is not obvious from the CommonMark spec alone — normally backslash escapes don't apply inside code spans. The commit message documents this was verified against the real renderer (gh api markdown, mode=gfm), which is the right way to validate a claim like this rather than trusting it by inspection.
• The follow-up commit (e0a5580) correctly scopes the check to the row changed in a refine run, which stays consistent with the pre-existing step 6 rule that every other row "must come through byte-for-byte" (line 293) — no new contradiction introduced there.
• No correctness issues found in the diff itself.

Security

• None applicable — this is prompt text with no executable code, no shell interpolation, no new external inputs.

CLAUDE.md / constitution adherence

• Principle I (TDD carve-out for non-production changes): correctly applied. This isn't C# production code, so the RED/GREEN evidence is the real tool's output, not a bespoke test — the original commit (965c498) documents RED (gh api markdown splitting the unescaped rows) and GREEN (escaped forms rendering correctly) in the commit message, per the constitution's config/tooling carve-out.
• No hard-wrapped markdown: the new text is appended into the paragraph's existing single line, consistent with the project's soft-wrap convention.
• CLI docs (README/USER_GUIDE): not applicable — this changes an internal Claude command prompt, not a NetPace CLI option.
• Change Intent Record: arguably borderline under docs/conventions/change-intent-records.md ("working around a limitation the code itself won't make obvious"), but the prompt text itself spells out the why (GitHub's split-before-render behaviour) inline, and the PR body's "Non-obvious things" section covers the same ground — a separate CIR would be redundant for a change this small. Not a blocker.

Test coverage

• N/A in the traditional sense — there's no test suite that exercises .claude/commands/*.md prompt text, and the PR body correctly says so ("No CI gate... [the rule] stays a self-check"). The verification method (real GitHub renderer via gh api markdown) is the appropriate substitute per Principle I, and it's documented in the commit rather than just asserted.

Spec/test-plan mismatches

• No spec.md/test-plan.md exists for issue Escaping rule for a literal pipe in an at-a-glance table cell #286 (this was a direct fix, not run through the full spec-kit flow), so there's nothing to cross-check for AC-to-test traceability. No mismatch found.

---

Recommendation: Merge

Small, well-scoped, behaviourally verified against the real renderer, and consistent with the surrounding prompt's conventions. Nothing here needs to be addressed before merging.
· branch feature/286-table-pipe-escaping