Skip to content

fix(workflows): init step must not replace init's own error with SystemExit: 1 - #4530

Open
jawwad-ali wants to merge 2 commits into
github:mainfrom
jawwad-ali:fix/init-step-error-masking
Open

jawwad-ali wants to merge 2 commits into
github:mainfrom
jawwad-ali:fix/init-step-error-masking

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

InitStep._run_init appends 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 exists 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 fires on routine errors too.

init prints its diagnostics through Rich to stdout, so result.stderr is empty. The synthesized detail therefore becomes the entire stderr, which preempts execute's fallback two frames later:

error=(
    stderr.strip()
    or stdout.strip()          # <-- never reached
    or f"specify init exited with code {exit_code}."
),

Reproduction on current main (c173bf1)

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"

So the workflow author is told SystemExit: 1 while the list of valid integrations sits unread in stdout. The same applies to any ordinary init failure — a project: directory that already exists, an unusable script type.

It also leaks into workflow data: steps.<id>.output.stderr is the string "SystemExit: 1", so a downstream step reading it gets the sentinel rather than a diagnosis.

Fix

Exclude only SystemExit, preserving the branch for genuine crashes:

if (
    result.exit_code != 0
    and result.exception is not None
    and not isinstance(result.exception, SystemExit)
):

After the fix the same input reports init's own message, and a RuntimeError escaping the runner still yields RuntimeError: boom inside init.

Verification

  • Fail-before / pass-after: 1 new-vs-baseline failure with the source reverted to upstream/main → 16 passed with the fix.
  • A second test pins the branch's original purpose — an unexpected RuntimeError still surfaces. It passes both before and after by design: it guards preserved behaviour rather than proving the fix.
  • Scoped regression on tests/test_workflows.py: 20 failed / 959 passed vs a clean-main baseline of 20 failed / 957 passed — no new failures (the 20 are the known Windows os.replace flakiness in that file).
  • uvx ruff@0.15.0 check src tests → clean

Behaviour change, disclosed: error and output.stderr for a failing init step change from "SystemExit: 1" to init's own captured output (which includes its banner, since init writes to stdout). Successful steps are untouched, and no existing test asserted the old sentinel.


Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current main.

🤖 Generated with Claude Code

…temExit: 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) <noreply@anthropic.com>
@jawwad-ali
jawwad-ali requested a review from mnriem as a code owner September 11, 2026 15:59
@mnriem mnriem added the triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review label Sep 12, 2026
@mnriem
mnriem requested a balanced review from Copilot September 16, 2026 11:41
@mnriem

mnriem commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

The error-selection fix looks sound: ordinary CLI exits no longer obscure init’s own diagnostic, while unexpected exceptions still retain their details.

Please correct the behavior description: for a stdout-only failure on modern Click, result.error picks up the diagnostic through its stdout fallback, while output.stderr remains empty. The patch does not move that diagnostic into stderr. This is a description correction, not a request to change stream handling.

Drafted for @mnriem with assistance from GitHub Copilot (model: GPT-6 Astra; interactive comment drafting).

@mnriem mnriem added the author-awaiting Waiting on author response label Sep 16, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Routine failures still leave output.stderr empty instead of exposing init’s diagnostic as described.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Prevents routine init failures from being replaced by SystemExit: 1.

Changes:

  • Excludes SystemExit from unexpected-crash reporting.
  • Adds regression and crash-preservation tests.
  • Reported verification was not rerun during review.
File summaries
File Description
src/specify_cli/workflows/steps/init/__init__.py Refines failure handling.
tests/test_workflows.py Tests routine failures and unexpected crashes.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.


💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/specify_cli/workflows/steps/init/__init__.py

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback and resolve conflicts

@mnriem mnriem added author-needs-rebase Branch conflicts with main — rebase/resolve before merge author-awaiting Waiting on author response and removed author-awaiting Waiting on author response labels Sep 22, 2026
Addresses review feedback: dropping the synthesized "SystemExit: 1" fixed
`result.error` but left `steps.<id>.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.<id>.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) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation addresses the regression and tests both corrected and preserved behavior, though verification relied on the provided results.

Review effort: Balanced
Findings: None

Resolved since last review (1)

@mnriem

mnriem commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

Please resolve conflicts

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-awaiting Waiting on author response author-needs-rebase Branch conflicts with main — rebase/resolve before merge triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants