fix(presets): escape installed preset metadata in Rich output - #3826
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>
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
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
Addresses Copilot review feedback on github#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)
|
Thanks — both Copilot comments addressed in eb667ec (tests only, no source change). Catalog tags assertion. Untested
One note on the path assertion: a closing tag can't be embedded in a mocked Test-the-test: with |
|
Thank you! |
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