fix(presets): escape installed preset metadata in Rich output - #3826
Open
Noor-ul-ain001 wants to merge 1 commit into
Open
fix(presets): escape installed preset metadata in Rich output#3826Noor-ul-ain001 wants to merge 1 commit into
Noor-ul-ain001 wants to merge 1 commit into
Conversation
`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 github#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>
Contributor
There was a problem hiding this comment.
Pull request overview
Escapes user-controlled preset metadata so Rich renders it literally instead of swallowing tags or crashing.
Changes:
- Escapes installed preset and catalog metadata.
- Safely renders resolve paths, sources, errors, and strategy labels.
- Adds Rich-markup regression tests.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/presets/_commands.py |
Escapes untrusted Rich output. |
tests/test_presets.py |
Adds installed-preset markup tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Medium
Comment on lines
+474
to
+475
| catalog_tags_str = _escape_markup(", ".join(str(t) for t in catalog_tags)) | ||
| console.print(f" Tags: {catalog_tags_str}") |
Comment on lines
+12284
to
+12288
| 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) |
mnriem
requested changes
Jul 28, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
preset.ymlis user-editable, but the installed-preset display paths interpolate its fields straight intoconsole.print, where Rich parses[...]as a style tag.#3773 escaped the catalog branch of these commands; the local branch was left behind — so the same field renders correctly from a catalog and incorrectly once installed.
Silent data loss. Description
Does [stuff] nicely:Hard crash. An unbalanced tag like
Broken [/red] tagraisesrich.errors.MarkupError, aborting with a traceback and exit 1 — the preset can't be inspected at all:preset resolvewas unescaped throughout, and it echoes its own argument, so this crashes on user input alone with no preset installed:Separately, the composition chain's literal
[{strategy_label}]was itself consumed as a style tag, so every chain line printed a blank label:Fix
Route the untrusted fields through the already-imported
_escape_markup:preset list(installed branch) — name, id, version, descriptionpreset info(installed branch) — name, id, version, description, author, tags, repository, license, and the per-template descriptionpreset info(catalog branch) — the tags join the earlier sweep missedpreset resolve— thetemplate_nameargument, resolved paths, layer sources, composition-error message\[, matching the step-graph line inworkflow infoNo behaviour change beyond rendering: fields now display verbatim instead of being swallowed or crashing.
Tests
TestInstalledPresetRichMarkupintests/test_presets.py— 5 tests covering markup fields in list/info, the template description line, the unbalanced-tag crash, thepreset resolveargument crash, and the composition strategy labels.Test-the-test: with the source fix stashed, all 5 fail (4 on markup assertions, 1 on the swallowed template description); with it applied, all 5 pass.
pytest tests/test_presets.py→ 475 passed, 7 failed, 1 error, matching the pre-existing baseline on cleanmainfor this Windows box (symlink tests requiring elevation + one unrelated GHES-asset error). None are in the touched paths.🤖 Generated with Claude Code