mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(workflows): escape the step-progress line so step ids render (and / stops failing the run) (#3783)
`workflow run` and `workflow resume` both print the step-progress line as
`f" ▸ [{sid}] {label} …"`. Rich parses the bracketed step id as a style tag,
which produces three failures on main:
1. The id is SILENTLY SWALLOWED on every run -- the only identifying content on
the line. `id: greet` prints " ▸ shell …"; "[greet]" is absent.
2. An id that forms a closing tag FAILS THE WHOLE RUN. `validate_workflow`
places no charset restriction on step ids, so `id: "/"` is a valid workflow;
the callback then raises MarkupError, which propagates into execute()'s
handler -> run persisted as `failed` with empty `step_results`, the step
never executed, exit 1 with a Rich internals error.
3. An id that is a real style (`bold`, `red`) is applied as FORMATTING to the
rest of the line.
The unescaped `label` (from `step_config["command"]`) compounds it.
Escape the literal bracket with `\[` and escape both interpolated values, at
both sites. This mirrors the `\[<type>]` step-graph precedent already in this
file (workflow_info). Escaping only the values is NOT sufficient -- the
f-string's own brackets are what Rich consumes.
Verified through the real CLI: ids `greet`/`bold`/`a]b` now render verbatim, and
`id: "/"` goes from a failed run to `Status: completed`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1054,7 +1054,18 @@ def workflow_run(
|
||||
load_custom_steps(project_root)
|
||||
engine = WorkflowEngine(project_root)
|
||||
if not json_output:
|
||||
engine.on_step_start = lambda sid, label: console.print(f" \u25b8 [{sid}] {label} \u2026")
|
||||
# Escape the literal bracket (\[) so Rich renders `[<step id>]` instead
|
||||
# of parsing it as a style tag named after the step id -- which it
|
||||
# silently swallows (losing the only identifying content on the line),
|
||||
# applies as formatting when the id happens to be a real style such as
|
||||
# `bold`, or raises MarkupError when the id forms a closing tag (`/`),
|
||||
# failing the whole run. Escape the interpolated values too, since both
|
||||
# come from workflow YAML. Mirrors the `\[<type>]` step-graph precedent
|
||||
# in workflow_info below.
|
||||
engine.on_step_start = lambda sid, label: console.print(
|
||||
f" \u25b8 \\[{_escape_markup(str(sid))}] "
|
||||
f"{_escape_markup(str(label))} \u2026"
|
||||
)
|
||||
|
||||
err = _error_console(json_output)
|
||||
|
||||
@@ -1176,7 +1187,18 @@ def workflow_resume(
|
||||
load_custom_steps(project_root)
|
||||
engine = WorkflowEngine(project_root)
|
||||
if not json_output:
|
||||
engine.on_step_start = lambda sid, label: console.print(f" \u25b8 [{sid}] {label} \u2026")
|
||||
# Escape the literal bracket (\[) so Rich renders `[<step id>]` instead
|
||||
# of parsing it as a style tag named after the step id -- which it
|
||||
# silently swallows (losing the only identifying content on the line),
|
||||
# applies as formatting when the id happens to be a real style such as
|
||||
# `bold`, or raises MarkupError when the id forms a closing tag (`/`),
|
||||
# failing the whole run. Escape the interpolated values too, since both
|
||||
# come from workflow YAML. Mirrors the `\[<type>]` step-graph precedent
|
||||
# in workflow_info below.
|
||||
engine.on_step_start = lambda sid, label: console.print(
|
||||
f" \u25b8 \\[{_escape_markup(str(sid))}] "
|
||||
f"{_escape_markup(str(label))} \u2026"
|
||||
)
|
||||
|
||||
inputs = _parse_input_values(input_values, json_output=json_output)
|
||||
err = _error_console(json_output)
|
||||
|
||||
@@ -9744,6 +9744,92 @@ steps:
|
||||
assert asset_calls[0][1] == {"Accept": "application/octet-stream"}
|
||||
|
||||
|
||||
class TestWorkflowStepStartProgressLine:
|
||||
"""The `run`/`resume` step-progress line must render the step id literally.
|
||||
|
||||
The line is built as ` ▸ [<id>] <label> …`, so Rich parsed the bracketed id
|
||||
as a style tag: it silently swallowed the id (the only identifying content
|
||||
on the line), applied it as formatting when the id happened to be a real
|
||||
style like `bold`, and raised MarkupError — failing the whole run — when the
|
||||
id formed a closing tag such as `/`. `validate_workflow` places no charset
|
||||
restriction on step ids, so all of these are accepted workflows.
|
||||
"""
|
||||
|
||||
def _write(self, tmp_path, step_id):
|
||||
path = tmp_path / "wf.yml"
|
||||
path.write_text(
|
||||
'schema_version: "1.0"\n'
|
||||
"workflow:\n"
|
||||
' id: "probe-wf"\n'
|
||||
' name: "Probe"\n'
|
||||
' version: "1.0.0"\n'
|
||||
"steps:\n"
|
||||
f' - id: "{step_id}"\n'
|
||||
" type: shell\n"
|
||||
' run: "exit 0"\n',
|
||||
encoding="utf-8",
|
||||
)
|
||||
return path
|
||||
|
||||
@pytest.mark.parametrize("step_id", ["greet", "bold", "a]b"])
|
||||
def test_progress_line_shows_step_id(self, tmp_path, monkeypatch, step_id):
|
||||
from typer.testing import CliRunner
|
||||
from specify_cli import app
|
||||
|
||||
monkeypatch.chdir(tmp_path)
|
||||
result = CliRunner().invoke(
|
||||
app, ["workflow", "run", str(self._write(tmp_path, step_id))]
|
||||
)
|
||||
assert result.exit_code == 0, result.stdout
|
||||
assert f"[{step_id}]" in result.stdout
|
||||
|
||||
def test_step_id_forming_a_closing_tag_does_not_fail_the_run(
|
||||
self, tmp_path, monkeypatch
|
||||
):
|
||||
"""`id: "/"` raised MarkupError from inside the progress callback, which
|
||||
surfaced as a failed run with no step results."""
|
||||
from typer.testing import CliRunner
|
||||
from specify_cli import app
|
||||
|
||||
monkeypatch.chdir(tmp_path)
|
||||
result = CliRunner().invoke(
|
||||
app, ["workflow", "run", str(self._write(tmp_path, "/"))]
|
||||
)
|
||||
assert result.exit_code == 0, result.stdout
|
||||
assert "Status: completed" in result.stdout
|
||||
assert "[/]" in result.stdout
|
||||
|
||||
def test_resume_progress_line_shows_step_id(self, tmp_path, monkeypatch):
|
||||
"""`workflow resume` installs its own copy of the same callback, so it
|
||||
needs independent coverage — a one-line fix would miss the twin."""
|
||||
import json as _json
|
||||
|
||||
from typer.testing import CliRunner
|
||||
from specify_cli import app
|
||||
|
||||
monkeypatch.chdir(tmp_path)
|
||||
path = tmp_path / "wf.yml"
|
||||
path.write_text(
|
||||
'schema_version: "1.0"\n'
|
||||
"workflow:\n"
|
||||
' id: "probe-resume"\n'
|
||||
' name: "Probe"\n'
|
||||
' version: "1.0.0"\n'
|
||||
"steps:\n"
|
||||
" - id: boom\n"
|
||||
" type: shell\n"
|
||||
' run: "exit 1"\n',
|
||||
encoding="utf-8",
|
||||
)
|
||||
runner = CliRunner()
|
||||
first = runner.invoke(app, ["workflow", "run", str(path), "--json"])
|
||||
run_id = _json.loads(first.stdout).get("run_id")
|
||||
assert run_id
|
||||
|
||||
resumed = runner.invoke(app, ["workflow", "resume", run_id])
|
||||
assert "[boom]" in resumed.stdout
|
||||
|
||||
|
||||
class TestWorkflowRunExitCodes:
|
||||
"""CLI-level tests for the run/resume process exit codes."""
|
||||
|
||||
|
||||
Reference in New Issue
Block a user