From aa067f721d90467066bf96692e1ee446fc69a4ab Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 11 Sep 2026 20:59:11 +0500 Subject: [PATCH 1/2] fix(workflows): init step must not replace init's own error with "SystemExit: 1" `InitStep._run_init` appended the runner's exception to stderr: if result.exit_code != 0 and result.exception is not None: detail = f"{type(result.exception).__name__}: {result.exception}" stderr = f"{stderr}\n{detail}".strip() if stderr else detail That branch is written for an unexpected crash, but `typer.Exit(n)` -- how `specify init` reports every ordinary failure -- surfaces through `CliRunner` as `result.exception = SystemExit(n)`, so it fired on routine errors too. `init` prints its diagnostics through Rich to stdout, so `result.stderr` is empty and the synthesized detail became the ENTIRE stderr, preempting `execute`'s fallback: error=(stderr.strip() or stdout.strip() or f"specify init exited ...") `stdout.strip()` -- which holds the real message -- was never reached. Reproduced on main with an ordinary typo in `integration:` (which `InitStep.validate` does not value-check): validate : [] status : failed | exit_code: 1 result.error : 'SystemExit: 1' output.stderr: 'SystemExit: 1' stdout holds : "... lingma, muse, omp, opencode, pi, qodercli, qwen, rovodev, shai, tabnine, trae, vibe, zcode, zed" Now excludes only `SystemExit`, so an ordinary non-zero exit falls through to init's own output while a genuine crash still reports its exception. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflows/steps/init/__init__.py | 16 ++++- tests/test_workflows.py | 64 +++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/steps/init/__init__.py b/src/specify_cli/workflows/steps/init/__init__.py index 270badc4fc..a49b1c8c17 100644 --- a/src/specify_cli/workflows/steps/init/__init__.py +++ b/src/specify_cli/workflows/steps/init/__init__.py @@ -295,7 +295,21 @@ def _run_init( # steps..output.stderr for error details. stderr = stdout if result.exit_code != 0 else "" - if result.exit_code != 0 and result.exception is not None: + # Record the exception only for an UNEXPECTED crash. ``typer.Exit(n)`` + # -- how ``specify init`` reports every ordinary failure -- surfaces + # through ``CliRunner`` as ``result.exception = SystemExit(n)``, so this + # branch used to fire on routine errors too. ``init`` prints its + # diagnostics through Rich to stdout, leaving ``result.stderr`` empty, + # so the synthesized "SystemExit: 1" became the whole of ``stderr`` and + # preempted ``execute``'s ``stderr.strip() or stdout.strip()`` fallback. + # Every failing init step then reported ``error: 'SystemExit: 1'`` while + # init's real message -- e.g. the list of valid integrations for a + # typo'd ``integration:`` -- sat unread in stdout. + if ( + result.exit_code != 0 + and result.exception is not None + and not isinstance(result.exception, SystemExit) + ): detail = f"{type(result.exception).__name__}: {result.exception}" stderr = f"{stderr}\n{detail}".strip() if stderr else detail diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 2c7141e954..443275d109 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2594,6 +2594,70 @@ def test_invalid_integration_fails(self, tmp_path): assert result.output["exit_code"] != 0 assert result.error is not None + def test_failed_init_surfaces_inits_own_message(self, tmp_path): + """A failing init step must report init's diagnostics, not 'SystemExit: 1'. + + `typer.Exit(n)` — how `specify init` reports every ordinary failure — + surfaces through `CliRunner` as `result.exception = SystemExit(n)`, so + the unexpected-crash branch fired on routine errors too. `init` prints + through Rich to stdout, leaving `result.stderr` empty, so the + synthesized "SystemExit: 1" became the whole of stderr and preempted + the `stderr.strip() or stdout.strip()` fallback — stranding the real + message (here, the list of valid integrations) in stdout. + """ + from specify_cli.workflows.steps.init import InitStep + from specify_cli.workflows.base import StepContext, StepStatus + + result = InitStep().execute( + { + "id": "bootstrap", + "here": True, + "integration": "no-such-agent", + "script": "sh", + }, + StepContext(project_root=str(tmp_path)), + ) + + assert result.status == StepStatus.FAILED + assert result.error is not None + assert result.error.strip() != "SystemExit: 1" + assert result.output["stderr"].strip() != "SystemExit: 1" + # The real diagnostic reaches the caller. + collapsed = " ".join(result.error.split()) + assert "no-such-agent" in collapsed or "Unknown" in collapsed, collapsed + + def test_unexpected_crash_still_reports_its_exception(self, monkeypatch, tmp_path): + """The branch's original purpose is preserved for a genuine crash. + + Only `SystemExit` is now excluded; any other exception escaping the + runner must still be surfaced, since nothing else would describe it. + """ + from specify_cli.workflows.steps.init import InitStep + import typer.testing + + class _Result: + exit_code = 1 + output = "" + stderr = "" + exception = RuntimeError("boom inside init") + + class _Runner: + def __init__(self, *a, **k): + pass + + def invoke(self, *a, **k): + return _Result() + + monkeypatch.setattr(typer.testing, "CliRunner", _Runner) + + from specify_cli.workflows.base import StepContext + + _code, _stdout, stderr = InitStep()._run_init( + ["init", "demo"], StepContext(project_root=str(tmp_path)) + ) + + assert "RuntimeError: boom inside init" in stderr + def test_non_empty_current_dir_without_force_fails_fast(self, tmp_path): from specify_cli.workflows.steps.init import InitStep from specify_cli.workflows.base import StepContext, StepStatus From 40a3edec8de09625e6f68814ff83fdc17b11ff7f Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Wed, 23 Sep 2026 22:58:06 +0500 Subject: [PATCH 2/2] fix(workflows): carry init's output into stderr for an ordinary failure Addresses review feedback: dropping the synthesized "SystemExit: 1" fixed `result.error` but left `steps..output.stderr` EMPTY, so a downstream step reading that field still got no diagnostic -- contrary to this PR's disclosed behaviour and to the contract stated a few lines above ("treat stdout as stderr so workflows can consistently read steps..output.stderr for error details"). Under click >= 8.2 the streams are separate and `init` prints through Rich to stdout, so `result.stderr` is genuinely empty for a routine failure. The captured output is now carried across for an ordinary non-zero exit with no real stderr, matching what the older-Click branch already does: before: output.stderr = '' after : output.stderr = "... rovodev, shai, tabnine, trae, vibe, zcode, zed" The unexpected-crash branch is unchanged, so a genuine exception still reports its detail. The regression test was also too weak: asserting only "not the old sentinel" is satisfied by an empty string. It now asserts `output.stderr` is non-empty AND contains init's diagnostic. Mutation-verified -- removing the carry fails it with "output.stderr must not be empty for a failed init". Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/workflows/steps/init/__init__.py | 11 +++++++++++ tests/test_workflows.py | 14 ++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/workflows/steps/init/__init__.py b/src/specify_cli/workflows/steps/init/__init__.py index a49b1c8c17..5dc04484a9 100644 --- a/src/specify_cli/workflows/steps/init/__init__.py +++ b/src/specify_cli/workflows/steps/init/__init__.py @@ -312,6 +312,17 @@ def _run_init( ): detail = f"{type(result.exception).__name__}: {result.exception}" stderr = f"{stderr}\n{detail}".strip() if stderr else detail + elif result.exit_code != 0 and not stderr.strip(): + # Ordinary failure with no real stderr: under click >= 8.2 the + # streams are separate and ``init`` prints its diagnostics through + # Rich to stdout, so ``result.stderr`` is genuinely empty. Merely + # dropping the synthesized "SystemExit: 1" would leave + # ``steps..output.stderr`` empty -- still no diagnostic for a + # downstream step to read, and still contrary to the contract above + # ("treat stdout as stderr so workflows can consistently read + # steps..output.stderr for error details"). Carry the captured + # output across, matching what the older-Click branch already does. + stderr = stdout.strip() return (result.exit_code, stdout, stderr) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 443275d109..29ba2e2536 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2621,11 +2621,21 @@ def test_failed_init_surfaces_inits_own_message(self, tmp_path): assert result.status == StepStatus.FAILED assert result.error is not None assert result.error.strip() != "SystemExit: 1" - assert result.output["stderr"].strip() != "SystemExit: 1" - # The real diagnostic reaches the caller. + + # The real diagnostic reaches the caller... collapsed = " ".join(result.error.split()) assert "no-such-agent" in collapsed or "Unknown" in collapsed, collapsed + # ...and reaches a downstream step reading steps..output.stderr. + # Asserting only "not the old sentinel" was too weak: an EMPTY stderr + # satisfies that while still carrying no diagnostic at all, which is + # exactly what dropping the synthesized detail left behind under + # click >= 8.2 (separate streams, init prints through Rich to stdout). + stderr = " ".join(result.output["stderr"].split()) + assert stderr, "output.stderr must not be empty for a failed init" + assert stderr != "SystemExit: 1" + assert "no-such-agent" in stderr or "Unknown" in stderr, stderr + def test_unexpected_crash_still_reports_its_exception(self, monkeypatch, tmp_path): """The branch's original purpose is preserved for a genuine crash.