From b5c1f855b0cd646457fe907e1eb84b04713f3be2 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Wed, 29 Jul 2026 02:19:52 +0500 Subject: [PATCH 1/2] fix(presets): escape installed preset metadata in Rich output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- src/specify_cli/presets/_commands.py | 72 +++++++++++---- tests/test_presets.py | 130 +++++++++++++++++++++++++++ 2 files changed, 183 insertions(+), 19 deletions(-) diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index ef4bf898dc..437c8f3ffb 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -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 `[]` + # 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']))}" diff --git a/tests/test_presets.py b/tests/test_presets.py index 8a8ff464d7..ea0216ee0d 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -12170,3 +12170,133 @@ def test_info_escapes_catalog_markup(self, project_dir): ): value = self.MARKUP_PRESET[field] assert value 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_renders_composition_strategy_labels(self, temp_dir, project_dir): + """The composition chain's ``[]`` 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 From eb667ecaedde5e13e64f8b147f43ab1a0408c83d Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Wed, 29 Jul 2026 10:36:54 +0500 Subject: [PATCH 2/2] test(presets): cover catalog tags and resolve escapes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Assisted-by: Claude Code (model: claude-opus-5, under direct human supervision) --- tests/test_presets.py | 80 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/tests/test_presets.py b/tests/test_presets.py index ea0216ee0d..9f838abdce 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -12170,6 +12170,8 @@ def test_info_escapes_catalog_markup(self, project_dir): ): 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: @@ -12287,6 +12289,84 @@ def test_resolve_escapes_template_name(self, project_dir): 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 ``[]`` label must not be eaten as a tag.""" self._install(temp_dir, project_dir, strategy="replace",