mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(presets): escape installed preset metadata in Rich output (#3826)
* fix(presets): escape installed preset metadata in Rich output `preset.yml` is user-editable, but the installed-preset display paths interpolated its fields straight into `console.print`, where Rich parses `[...]` as a style tag. PR #3773 escaped the *catalog* branch of these commands; the local branch was left behind, so the same field rendered correctly from a catalog and incorrectly once installed. Two failure modes: - Silent data loss: a description `Does [stuff] nicely` renders as `Does nicely`. - Hard crash: an unbalanced tag such as `Broken [/red] tag` raises `rich.errors.MarkupError`, aborting `preset list`/`preset info` with a traceback and exit code 1 — the preset cannot be inspected at all. Escaped the installed branch of `preset list` (name/id/version/ description) and `preset info` (name/id/version/description/author/tags/ repository/license plus the per-template description), and the catalog branch's tags join that the earlier sweep missed. `preset resolve` was unescaped throughout: it echoes its own `template_name` argument, so `preset resolve 'no[/red]such'` crashed on user input alone. Also escaped the resolved paths, layer sources, and composition-error message. Separately, the composition chain's `[{strategy_label}]` was consumed as a style tag, so every chain line printed a blank label instead of `[base]`/`[append]`. Escaped the literal bracket as `\[`, matching the step-graph line in `workflow info`. Regression tests in `TestInstalledPresetRichMarkup` cover all five behaviours; each fails before this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(presets): cover catalog tags and resolve escapes Addresses Copilot review feedback on #3826: two escapes added by the previous commit had no regression assertion, so they could be reverted with the suite still green. - `test_info_escapes_catalog_markup` asserted every catalog field except `tags`; the new tag assertion only exercised an installed preset. Assert the rendered tags join in the catalog branch too. - The escapes on `preset resolve`'s resolved path, layer source, and composition-error message were untested. Add three cases patching `PresetResolver` to feed markup through the top-layer line, the no-layer `resolve_with_source` fallback, and a markup-bearing `resolve_content` exception. Test-the-test: with `_commands.py` reverted to the pre-fix revision, 9 of the 10 markup tests fail (was 5); with the fix applied all 10 pass. A closing tag cannot be embedded in the mocked path — `Path` treats the `/` as a separator — so the path assertion uses an opening tag for the swallowing case and the unbalanced tag rides on the adjacent `source` field on the same line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code (model: claude-opus-5, under direct human supervision) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Noor ul ain
@@ -59,8 +59,11 @@ def preset_list():
|
||||
for pack in installed:
|
||||
status = "[green]enabled[/green]" if pack.get("enabled", True) else "[red]disabled[/red]"
|
||||
pri = pack.get('priority', 10)
|
||||
console.print(f" [bold]{pack['name']}[/bold] ({pack['id']}) v{pack['version']} — {status} — priority {pri}")
|
||||
console.print(f" {pack['description']}")
|
||||
name = _escape_markup(str(pack['name']))
|
||||
pack_id = _escape_markup(str(pack['id']))
|
||||
version = _escape_markup(str(pack['version']))
|
||||
console.print(f" [bold]{name}[/bold] ({pack_id}) v{version} — {status} — priority {pri}")
|
||||
console.print(f" {_escape_markup(str(pack['description']))}")
|
||||
tags = pack.get("tags", [])
|
||||
if isinstance(tags, list) and tags:
|
||||
tags_str = _escape_markup(", ".join(str(t) for t in tags))
|
||||
@@ -317,13 +320,20 @@ def preset_resolve(
|
||||
project_root = _require_specify_project()
|
||||
resolver = PresetResolver(project_root)
|
||||
layers = resolver.collect_all_layers(template_name)
|
||||
safe_template_name = _escape_markup(str(template_name))
|
||||
|
||||
if layers:
|
||||
# Use the highest-priority layer for display because the final output
|
||||
# may be composed and may not map to resolve_with_source()'s single path.
|
||||
display_layer = layers[0]
|
||||
console.print(f" [bold]{template_name}[/bold]: {display_layer['path']}")
|
||||
console.print(f" [dim](top layer from: {display_layer['source']})[/dim]")
|
||||
console.print(
|
||||
f" [bold]{safe_template_name}[/bold]: "
|
||||
f"{_escape_markup(str(display_layer['path']))}"
|
||||
)
|
||||
console.print(
|
||||
f" [dim](top layer from: "
|
||||
f"{_escape_markup(str(display_layer['source']))})[/dim]"
|
||||
)
|
||||
|
||||
has_composition = (
|
||||
layers[0]["strategy"] != "replace"
|
||||
@@ -335,7 +345,10 @@ def preset_resolve(
|
||||
composed = resolver.resolve_content(template_name)
|
||||
except Exception as exc:
|
||||
composed = None
|
||||
console.print(f" [yellow]Warning: composition error: {exc}[/yellow]")
|
||||
console.print(
|
||||
f" [yellow]Warning: composition error: "
|
||||
f"{_escape_markup(str(exc))}[/yellow]"
|
||||
)
|
||||
if composed is None:
|
||||
console.print(" [yellow]Warning: composition cannot produce output (no base layer with 'replace' strategy)[/yellow]")
|
||||
else:
|
||||
@@ -358,15 +371,27 @@ def preset_resolve(
|
||||
strategy_label = layer["strategy"]
|
||||
if strategy_label == "replace" and i == 0:
|
||||
strategy_label = "base"
|
||||
console.print(f" {i + 1}. [{strategy_label}] {layer['source']} → {layer['path']}")
|
||||
# Escape the literal bracket (\[) so Rich renders `[<strategy>]`
|
||||
# instead of parsing it as a style tag and swallowing the label,
|
||||
# mirroring `workflow info`'s step-graph line.
|
||||
console.print(
|
||||
f" {i + 1}. \\[{_escape_markup(str(strategy_label))}] "
|
||||
f"{_escape_markup(str(layer['source']))} → "
|
||||
f"{_escape_markup(str(layer['path']))}"
|
||||
)
|
||||
else:
|
||||
# No layers found — fall back to resolve_with_source for non-composition cases
|
||||
result = resolver.resolve_with_source(template_name)
|
||||
if result:
|
||||
console.print(f" [bold]{template_name}[/bold]: {result['path']}")
|
||||
console.print(f" [dim](from: {result['source']})[/dim]")
|
||||
console.print(
|
||||
f" [bold]{safe_template_name}[/bold]: "
|
||||
f"{_escape_markup(str(result['path']))}"
|
||||
)
|
||||
console.print(
|
||||
f" [dim](from: {_escape_markup(str(result['source']))})[/dim]"
|
||||
)
|
||||
else:
|
||||
console.print(f" [yellow]{template_name}[/yellow]: not found")
|
||||
console.print(f" [yellow]{safe_template_name}[/yellow]: not found")
|
||||
console.print(" [dim]No template with this name exists in the resolution stack[/dim]")
|
||||
|
||||
|
||||
@@ -386,24 +411,32 @@ def preset_info(
|
||||
local_pack = manager.get_pack(preset_id)
|
||||
|
||||
if local_pack:
|
||||
console.print(f"\n[bold cyan]Preset: {local_pack.name}[/bold cyan]\n")
|
||||
console.print(f" ID: {local_pack.id}")
|
||||
console.print(f" Version: {local_pack.version}")
|
||||
console.print(f" Description: {local_pack.description}")
|
||||
console.print(
|
||||
f"\n[bold cyan]Preset: {_escape_markup(str(local_pack.name))}[/bold cyan]\n"
|
||||
)
|
||||
console.print(f" ID: {_escape_markup(str(local_pack.id))}")
|
||||
console.print(f" Version: {_escape_markup(str(local_pack.version))}")
|
||||
console.print(
|
||||
f" Description: {_escape_markup(str(local_pack.description))}"
|
||||
)
|
||||
if local_pack.author:
|
||||
console.print(f" Author: {local_pack.author}")
|
||||
console.print(f" Author: {_escape_markup(str(local_pack.author))}")
|
||||
local_tags = local_pack.tags
|
||||
if isinstance(local_tags, list) and local_tags:
|
||||
console.print(f" Tags: {', '.join(str(t) for t in local_tags)}")
|
||||
tags_str = _escape_markup(", ".join(str(t) for t in local_tags))
|
||||
console.print(f" Tags: {tags_str}")
|
||||
console.print(f" Templates: {len(local_pack.templates)}")
|
||||
for tmpl in local_pack.templates:
|
||||
console.print(f" - {tmpl['name']} ({tmpl['type']}): {tmpl.get('description', '')}")
|
||||
tmpl_name = _escape_markup(str(tmpl['name']))
|
||||
tmpl_type = _escape_markup(str(tmpl['type']))
|
||||
tmpl_desc = _escape_markup(str(tmpl.get('description', '')))
|
||||
console.print(f" - {tmpl_name} ({tmpl_type}): {tmpl_desc}")
|
||||
repo = local_pack.data.get("preset", {}).get("repository")
|
||||
if repo:
|
||||
console.print(f" Repository: {repo}")
|
||||
console.print(f" Repository: {_escape_markup(str(repo))}")
|
||||
license_val = local_pack.data.get("preset", {}).get("license")
|
||||
if license_val:
|
||||
console.print(f" License: {license_val}")
|
||||
console.print(f" License: {_escape_markup(str(license_val))}")
|
||||
console.print("\n [green]Status: installed[/green]")
|
||||
# Get priority from registry
|
||||
pack_metadata = manager.registry.get(preset_id)
|
||||
@@ -438,7 +471,8 @@ def preset_info(
|
||||
)
|
||||
catalog_tags = pack_info.get("tags", [])
|
||||
if isinstance(catalog_tags, list) and catalog_tags:
|
||||
console.print(f" Tags: {', '.join(str(t) for t in catalog_tags)}")
|
||||
catalog_tags_str = _escape_markup(", ".join(str(t) for t in catalog_tags))
|
||||
console.print(f" Tags: {catalog_tags_str}")
|
||||
if pack_info.get("repository"):
|
||||
console.print(
|
||||
f" Repository: {_escape_markup(str(pack_info['repository']))}"
|
||||
|
||||
@@ -12209,3 +12209,213 @@ class TestPresetCatalogRichMarkup:
|
||||
):
|
||||
value = self.MARKUP_PRESET[field]
|
||||
assert value in output
|
||||
# Tags are joined into a single line, so assert on the rendered join.
|
||||
assert ", ".join(self.MARKUP_PRESET["tags"]) in output
|
||||
|
||||
|
||||
class TestInstalledPresetRichMarkup:
|
||||
"""Locally installed preset metadata must render as literal text.
|
||||
|
||||
``preset.yml`` is user-editable, so its fields can contain ``[...]``.
|
||||
``TestPresetCatalogRichMarkup`` covers the catalog branch of these
|
||||
commands; the installed-preset branch of ``preset list``/``preset info``
|
||||
and all of ``preset resolve`` were left unescaped, so a field like
|
||||
``Does [stuff] nicely`` silently rendered as ``Does nicely`` and an
|
||||
unbalanced tag such as ``[/red]`` raised ``rich.errors.MarkupError``,
|
||||
aborting the command with a traceback.
|
||||
"""
|
||||
|
||||
MARKUP_FIELDS = {
|
||||
"name": "[green]Markup Name[/green]",
|
||||
"version": "1.0.0",
|
||||
"description": "[yellow]Markup Description[/yellow]",
|
||||
"author": "[magenta]Markup Author[/magenta]",
|
||||
"repository": "[bold]Markup Repository[/bold]",
|
||||
"license": "[cyan]Markup License[/cyan]",
|
||||
}
|
||||
|
||||
def _install(self, temp_dir, project_dir, preset_overrides=None, strategy=None,
|
||||
pack_id="markup-pack", priority=10, tmpl_description=None):
|
||||
"""Install a preset from a directory built with the given manifest fields."""
|
||||
from specify_cli.presets import PresetManager
|
||||
|
||||
src = temp_dir / f"src-{pack_id}"
|
||||
(src / "templates").mkdir(parents=True)
|
||||
(src / "templates" / "spec-template.md").write_text("# tmpl\n")
|
||||
|
||||
preset_section = {
|
||||
"id": pack_id,
|
||||
"name": pack_id,
|
||||
"version": "1.0.0",
|
||||
"description": "plain description",
|
||||
}
|
||||
preset_section.update(preset_overrides or {})
|
||||
tmpl = {
|
||||
"type": "template",
|
||||
"name": "spec-template",
|
||||
"file": "templates/spec-template.md",
|
||||
}
|
||||
if tmpl_description is not None:
|
||||
tmpl["description"] = tmpl_description
|
||||
if strategy:
|
||||
tmpl["strategy"] = strategy
|
||||
(src / "preset.yml").write_text(yaml.dump({
|
||||
"schema_version": "1.0",
|
||||
"preset": preset_section,
|
||||
"requires": {"speckit_version": ">=0.0.1"},
|
||||
"provides": {"templates": [tmpl]},
|
||||
"tags": ["[italic]markup-tag[/italic]"],
|
||||
}))
|
||||
|
||||
manager = PresetManager(project_dir)
|
||||
manager.install_from_directory(src, "9.9.9", priority)
|
||||
return manager
|
||||
|
||||
def _invoke(self, project_dir, args):
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
with patch.object(Path, "cwd", return_value=project_dir):
|
||||
return CliRunner().invoke(app, args)
|
||||
|
||||
def test_list_and_info_escape_installed_markup(self, temp_dir, project_dir):
|
||||
"""Every ``preset.yml`` field must survive verbatim in list/info output."""
|
||||
self._install(temp_dir, project_dir, preset_overrides=self.MARKUP_FIELDS)
|
||||
|
||||
for args in (["preset", "list"], ["preset", "info", "markup-pack"]):
|
||||
result = self._invoke(project_dir, args)
|
||||
assert result.exit_code == 0, result.output
|
||||
output = " ".join(strip_ansi(result.output).split())
|
||||
# `preset list` does not render repository/license.
|
||||
fields = ("name", "description") if args[1] == "list" else self.MARKUP_FIELDS
|
||||
for field in fields:
|
||||
assert self.MARKUP_FIELDS[field] in output, (field, args, output)
|
||||
assert "[italic]markup-tag[/italic]" in output, (args, output)
|
||||
|
||||
def test_info_does_not_swallow_template_description(self, temp_dir, project_dir):
|
||||
"""The per-template line in ``preset info`` must escape the template description.
|
||||
|
||||
``name``/``type`` are format-restricted by manifest validation, but
|
||||
``description`` is free-form, so it is the field that can carry markup.
|
||||
"""
|
||||
self._install(
|
||||
temp_dir,
|
||||
project_dir,
|
||||
tmpl_description="Template [desc] here",
|
||||
)
|
||||
result = self._invoke(project_dir, ["preset", "info", "markup-pack"])
|
||||
assert result.exit_code == 0, result.output
|
||||
output = " ".join(strip_ansi(result.output).split())
|
||||
assert "spec-template (template): Template [desc] here" in output, output
|
||||
|
||||
def test_unbalanced_markup_does_not_crash_list_or_info(self, temp_dir, project_dir):
|
||||
"""An unbalanced tag must not raise MarkupError and abort the command."""
|
||||
self._install(
|
||||
temp_dir,
|
||||
project_dir,
|
||||
preset_overrides={"description": "Broken [/red] tag"},
|
||||
)
|
||||
|
||||
for args in (["preset", "list"], ["preset", "info", "markup-pack"]):
|
||||
result = self._invoke(project_dir, args)
|
||||
assert result.exit_code == 0, (args, result.output, result.exception)
|
||||
assert "Broken [/red] tag" in strip_ansi(result.output)
|
||||
|
||||
def test_resolve_escapes_template_name(self, project_dir):
|
||||
"""``preset resolve`` echoes its argument; an unbalanced tag must not crash."""
|
||||
result = self._invoke(project_dir, ["preset", "resolve", "no[/red]such"])
|
||||
assert result.exit_code == 0, (result.output, result.exception)
|
||||
assert "no[/red]such" in strip_ansi(result.output)
|
||||
|
||||
def test_resolve_escapes_layer_path_and_source(self, project_dir):
|
||||
"""The top-layer path/source lines must render markup literally.
|
||||
|
||||
A preset can be installed from any directory, so the resolved path can
|
||||
contain ``[...]``; the layer source carries the pack id and version.
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
from specify_cli.presets import PresetResolver
|
||||
|
||||
# A closing tag cannot live inside a path segment: `Path` treats its
|
||||
# `/` as a separator on POSIX and rewrites it to `\` on Windows. The
|
||||
# opening tag covers the swallowing case for the path; the unbalanced
|
||||
# closing tag rides on `source`, which is a plain string.
|
||||
layer = {
|
||||
"path": Path("/tmp/[red]dir/spec-template.md"),
|
||||
"source": "pack [/red] v1.0.0",
|
||||
"strategy": "replace",
|
||||
}
|
||||
with patch.object(PresetResolver, "collect_all_layers", return_value=[layer]):
|
||||
result = self._invoke(project_dir, ["preset", "resolve", "spec-template"])
|
||||
|
||||
assert result.exit_code == 0, (result.output, result.exception)
|
||||
output = " ".join(strip_ansi(result.output).split())
|
||||
assert "[red]dir" in output, output
|
||||
assert "pack [/red] v1.0.0" in output, output
|
||||
|
||||
def test_resolve_escapes_fallback_path_and_source(self, project_dir):
|
||||
"""The no-layer fallback branch must escape ``resolve_with_source`` output."""
|
||||
from unittest.mock import patch
|
||||
from specify_cli.presets import PresetResolver
|
||||
|
||||
with patch.object(
|
||||
PresetResolver, "collect_all_layers", return_value=[]
|
||||
), patch.object(
|
||||
PresetResolver,
|
||||
"resolve_with_source",
|
||||
return_value={
|
||||
"path": "/tmp/[blue]fallback[/blue]/spec-template.md",
|
||||
"source": "fallback [/red] source",
|
||||
},
|
||||
):
|
||||
result = self._invoke(project_dir, ["preset", "resolve", "spec-template"])
|
||||
|
||||
assert result.exit_code == 0, (result.output, result.exception)
|
||||
output = " ".join(strip_ansi(result.output).split())
|
||||
assert "[blue]fallback[/blue]" in output, output
|
||||
assert "fallback [/red] source" in output, output
|
||||
|
||||
def test_resolve_escapes_composition_error(self, project_dir):
|
||||
"""A composition exception message must not be parsed as markup."""
|
||||
from unittest.mock import patch
|
||||
from specify_cli.presets import PresetResolver
|
||||
|
||||
layers = [
|
||||
{
|
||||
"path": Path("/tmp/top/spec-template.md"),
|
||||
"source": "top-pack v1.0.0",
|
||||
"strategy": "append",
|
||||
},
|
||||
{
|
||||
"path": Path("/tmp/base/spec-template.md"),
|
||||
"source": "base-pack v1.0.0",
|
||||
"strategy": "append",
|
||||
},
|
||||
]
|
||||
with patch.object(
|
||||
PresetResolver, "collect_all_layers", return_value=layers
|
||||
), patch.object(
|
||||
PresetResolver,
|
||||
"resolve_content",
|
||||
side_effect=RuntimeError("compose failed: [/red] bad layer"),
|
||||
):
|
||||
result = self._invoke(project_dir, ["preset", "resolve", "spec-template"])
|
||||
|
||||
assert result.exit_code == 0, (result.output, result.exception)
|
||||
output = " ".join(strip_ansi(result.output).split())
|
||||
assert "compose failed: [/red] bad layer" in output, output
|
||||
|
||||
def test_resolve_renders_composition_strategy_labels(self, temp_dir, project_dir):
|
||||
"""The composition chain's ``[<strategy>]`` label must not be eaten as a tag."""
|
||||
self._install(temp_dir, project_dir, strategy="replace",
|
||||
pack_id="base-pack", priority=20)
|
||||
self._install(temp_dir, project_dir, strategy="append",
|
||||
pack_id="app-pack", priority=5)
|
||||
|
||||
result = self._invoke(project_dir, ["preset", "resolve", "spec-template"])
|
||||
assert result.exit_code == 0, (result.output, result.exception)
|
||||
output = strip_ansi(result.output)
|
||||
assert "Composition chain" in output, output
|
||||
assert "[base]" in output, output
|
||||
assert "[append]" in output, output
|
||||
|
||||
Reference in New Issue
Block a user