feat: add question-render fenced markers and Claude transformer#2186
feat: add question-render fenced markers and Claude transformer#2186Rohan-1920 wants to merge 1 commit intogithub:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a Claude-specific post-processing layer that converts fenced “question table” blocks in generated SKILL.md files into structured JSON payloads intended for Claude’s native question UI, while also adjusting skill placeholder resolution logic for cross-platform script variants.
Changes:
- Added HTML comment fence markers around question tables in
clarify.mdandchecklist.mdtemplates. - Introduced
src/specify_cli/core/question_transformer.pyto detect fenced blocks, parse markdown tables, and emit a JSON payload block. - Updated Claude integration setup to run the transformer during skill installation; updated
{SCRIPT}/{AGENT_SCRIPT}placeholder resolution fallback logic.
Show a summary per file
| File | Description |
|---|---|
| tests/test_question_transformer.py | New unit tests for table parsing and fenced-block transformation behavior |
| tests/integrations/test_integration_claude.py | New integration tests asserting Claude skills contain transformed JSON and no raw markers |
| templates/commands/clarify.md | Adds begin/end markers around the clarify question table example |
| templates/commands/checklist.md | Adds begin/end markers around the checklist question table example |
| src/specify_cli/integrations/claude/init.py | Runs question block transformation when generating Claude SKILL.md files |
| src/specify_cli/core/question_transformer.py | New transformer/parser implementation for fenced question blocks |
| src/specify_cli/core/init.py | Adds core package initializer |
| src/specify_cli/agents.py | Updates script variant fallback selection for {SCRIPT} / {AGENT_SCRIPT} placeholders |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
src/specify_cli/agents.py:366
fallback_orderis only built wheninit_opts['script']is not in {"sh","ps"}. If the user explicitly setsscript: ps(orsh) but that variant is missing fromscriptsoragent_scripts, both{SCRIPT}/{AGENT_SCRIPT}can fail to resolve because the later fallback loop has an emptyfallback_order. Build a fallback list whenever the chosen variant isn’t available (or always include the other available variants), so placeholder resolution works even with a mismatched/partial set of script variants.
script_variant = init_opts.get("script")
fallback_order: list[str] = []
if script_variant not in {"sh", "ps"}:
# Build fallback order: prefer the variant present in BOTH scripts and
# agent_scripts so that {SCRIPT} and {AGENT_SCRIPT} resolve consistently.
# On Windows the OS default is "ps", but if only "sh" is available in
# agent_scripts we must still resolve both placeholders from "sh".
default_variant = "ps" if platform.system().lower().startswith("win") else "sh"
secondary_variant = "sh" if default_variant == "ps" else "ps"
# Prefer a variant that satisfies both scripts AND agent_scripts.
both_variants = set(scripts) & set(agent_scripts)
if both_variants:
for v in (default_variant, secondary_variant):
if v in both_variants:
fallback_order.append(v)
for v in sorted(both_variants):
if v not in fallback_order:
fallback_order.append(v)
# Then add remaining variants from scripts / agent_scripts.
for v in (default_variant, secondary_variant):
if v not in fallback_order and (v in scripts or v in agent_scripts):
fallback_order.append(v)
for key in list(scripts) + list(agent_scripts):
if key not in fallback_order:
fallback_order.append(key)
script_variant = fallback_order[0] if fallback_order else None
# Resolve script_command: try script_variant first, then walk fallback_order.
# This ensures sh-only extensions work on Windows (where default is "ps").
script_command = scripts.get(script_variant) if script_variant else None
if not script_command:
for _variant in fallback_order:
candidate = scripts.get(_variant)
if candidate:
script_command = candidate
break
if script_command:
script_command = script_command.replace("{ARGS}", "$ARGUMENTS")
body = body.replace("{SCRIPT}", script_command)
# Resolve agent_script_command: same cross-platform fallback.
agent_script_command = agent_scripts.get(script_variant) if script_variant else None
if not agent_script_command:
for _variant in fallback_order:
candidate = agent_scripts.get(_variant)
if candidate:
agent_script_command = candidate
break
if agent_script_command:
agent_script_command = agent_script_command.replace("{ARGS}", "$ARGUMENTS")
body = body.replace("{AGENT_SCRIPT}", agent_script_command)
src/specify_cli/core/question_transformer.py:135
- Checklist vs clarify detection uses a simple substring check for
| Candidate |inside the fenced block. This can mis-detect if the word “Candidate” appears in a clarify table description, and it’s sensitive to header capitalization/spacing. Consider detecting schema from the parsed header cell count/names (e.g., first non-empty table row) rather than a raw substring search.
def _replace(match: re.Match) -> str:
block = match.group(1)
is_checklist = "| Candidate |" in block or "|Candidate|" in block
options = parse_checklist(block) if is_checklist else parse_clarify(block)
return _build_payload(options)
- Files reviewed: 8/8 changed files
- Comments generated: 7
|
|
||
| def test_no_markers_returns_identical(self): | ||
| text = "hello world, no markers here" | ||
| assert transform_question_block(text) is not text or transform_question_block(text) == text |
There was a problem hiding this comment.
test_no_markers_returns_identical is effectively a tautology: it passes even if the transformer returns different content, because the is not text branch makes the assertion true. Replace with a single meaningful assertion (e.g., equality, and optionally identity if you truly require same object).
| assert transform_question_block(text) is not text or transform_question_block(text) == text | |
| assert transform_question_block(text) == text |
| @@ -0,0 +1,317 @@ | |||
| """Tests for src/specify_cli/core/question_transformer.py""" | |||
|
|
|||
| import pytest | |||
There was a problem hiding this comment.
pytest is imported but never used in this test module. Please remove the unused import to keep the test file clean (and to avoid unused-import noise if linting is expanded to tests later).
| import pytest |
| for cells in _parse_table_rows(block): | ||
| if len(cells) < 2: | ||
| continue | ||
| label = cells[0] | ||
| description = cells[1] | ||
| if label in seen_labels: | ||
| continue | ||
| seen_labels.add(label) | ||
| entry = {"label": label, "description": description} | ||
| if _RECOMMENDED_RE.search(description): | ||
| if recommended is None: | ||
| recommended = entry | ||
| else: | ||
| options.append(entry) |
There was a problem hiding this comment.
parse_clarify() drops any second (or later) row whose description matches the recommended regex: once recommended is set, subsequent recommended-matching options are silently discarded instead of being kept in the options list. Keep all options, but reorder so the first recommended one is first.
| <!-- speckit:question-render:begin --> | ||
| | Option | Description | | ||
| |--------|-------------| | ||
| | A | <Option A description> | | ||
| | B | <Option B description> | | ||
| | C | <Option C description> (add D/E as needed up to 5) | | ||
| | Short | Provide a different short answer (<=5 words) (Include only if free-form alternative is appropriate) | | ||
| <!-- speckit:question-render:end --> |
There was a problem hiding this comment.
The Claude-only template markers are currently unindented even though the surrounding content is indented as part of the nested list. This can break the Markdown list structure for non-Claude agents (where markers remain) and make the rendered prompt harder to read. Consider indenting the markers to match the table indentation, and update the transformer regex to allow optional leading whitespace before the marker lines.
| <!-- speckit:question-render:begin --> | |
| | Option | Description | | |
| |--------|-------------| | |
| | A | <Option A description> | | |
| | B | <Option B description> | | |
| | C | <Option C description> (add D/E as needed up to 5) | | |
| | Short | Provide a different short answer (<=5 words) (Include only if free-form alternative is appropriate) | | |
| <!-- speckit:question-render:end --> | |
| <!-- speckit:question-render:begin --> | |
| | Option | Description | | |
| |--------|-------------| | |
| | A | <Option A description> | | |
| | B | <Option B description> | | |
| | C | <Option C description> (add D/E as needed up to 5) | | |
| | Short | Provide a different short answer (<=5 words) (Include only if free-form alternative is appropriate) | | |
| <!-- speckit:question-render:end --> |
| - Format as: `**Recommended:** Option [X] - <reasoning>` | ||
| - Then render all options as a Markdown table: | ||
|
|
||
| <!-- speckit:question-render:begin --> | ||
| | Option | Description | | ||
| |--------|-------------| | ||
| | A | <Option A description> | | ||
| | B | <Option B description> | | ||
| | C | <Option C description> (add D/E as needed up to 5) | | ||
| | Short | Provide a different short answer (<=5 words) (Include only if free-form alternative is appropriate) | | ||
| <!-- speckit:question-render:end --> |
There was a problem hiding this comment.
The transformer’s “recommended option first” behavior depends on a Recommended —/ - marker inside a table row description, but the template instructs the recommended line to be emitted above the table (**Recommended:** Option [X] ...) and that line is outside the fenced block. As a result, the transformer cannot actually infer which option is recommended from the template output. Either include the recommended line within the fenced block, or extend the transformer to also parse the recommended selection from adjacent text.
| <!-- speckit:question-render:begin --> | ||
| | Option | Candidate | Why It Matters | | ||
| |--------|-----------|----------------| | ||
| | A | <Candidate A> | <Why it matters> | | ||
| | B | <Candidate B> | <Why it matters> | | ||
| <!-- speckit:question-render:end --> |
There was a problem hiding this comment.
Same as clarify.md: the begin/end marker lines are outdented relative to the surrounding nested list/table example. This risks breaking Markdown formatting for non-Claude agents and also ties correctness to markers being at column 1. Prefer indenting markers consistently and making the transformer accept leading whitespace.
| <!-- speckit:question-render:begin --> | |
| | Option | Candidate | Why It Matters | | |
| |--------|-----------|----------------| | |
| | A | <Candidate A> | <Why it matters> | | |
| | B | <Candidate B> | <Why it matters> | | |
| <!-- speckit:question-render:end --> | |
| <!-- speckit:question-render:begin --> | |
| | Option | Candidate | Why It Matters | | |
| |--------|-----------|----------------| | |
| | A | <Candidate A> | <Why it matters> | | |
| | B | <Candidate B> | <Why it matters> | | |
| <!-- speckit:question-render:end --> |
| r"<!-- speckit:question-render:begin -->\s*\n(.*?)\n\s*<!-- speckit:question-render:end -->", | ||
| re.DOTALL, |
There was a problem hiding this comment.
_FENCE_RE requires the begin/end markers to start with <!-- at column 1 (no leading whitespace). If the templates keep these markers inside nested lists (which typically require indentation), indented markers won’t match and the transform will silently not run. Consider allowing optional leading whitespace before both markers (and possibly making the newline before the end marker optional).
This issue also appears on line 131 of the same file.
| r"<!-- speckit:question-render:begin -->\s*\n(.*?)\n\s*<!-- speckit:question-render:end -->", | |
| re.DOTALL, | |
| r"^\s*<!-- speckit:question-render:begin -->\s*\n(.*?)(?:\n)?^\s*<!-- speckit:question-render:end -->", | |
| re.DOTALL | re.MULTILINE, |
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please deliver this as a community preset. See https://github.com/github/spec-kit/blob/main/presets/README.md?tab=readme-ov-file#presets--customize-existing-workflows
🚀 PR: Add Claude-native AskUserQuestion Rendering Pipeline
Overview
AskUserQuestionpayloadsChanges
Templates
Added fenced markers to isolate question-rendering blocks in:
templates/commands/clarify.mdtemplates/commands/checklist.mdMarkers used:
<!-- speckit:question-render:begin --><!-- speckit:question-render:end -->No changes to existing logic or structure
Core Module
New file:
src/specify_cli/core/question_transformer.pyDetects fenced question blocks
Parses:
Option | Description(clarify)Option | Candidate | Why It Matters(checklist)Converts into
AskUserQuestionpayloadExtra behavior:
Claude Integration
src/specify_cli/integrations/claude/__init__.pyAgents Fix
src/specify_cli/agents.py{SCRIPT}/{AGENT_SCRIPT}resolution logicTests
Unit Tests
tests/test_question_transformer.pyIntegration Tests
tests/integrations/test_integration_claude.pyBackward Compatibility
Result
AskUserQuestionUI with structured selection and fallback option