Skip to content

fix(cli): stop commands printing their own exit code as "Error: 1" (#1113) - #1134

Merged
frankbria merged 1 commit into
mainfrom
fix/1113-typer-exit-swallowed
Aug 10, 2026
Merged

fix(cli): stop commands printing their own exit code as "Error: 1" (#1113)#1134
frankbria merged 1 commit into
mainfrom
fix/1113-typer-exit-swallowed

Conversation

@frankbria

Copy link
Copy Markdown
Owner

Closes #1113.

Before / after

$ cf tasks generate
Error: No PRD found.
  cf prd generate              Start AI-guided requirements discovery
  cf prd add <file.md>         Import a PRD you already have
Error: 1                      ← the exit code, printed as a message
$ cf tasks generate
Error: No PRD found.
  cf prd generate              Start AI-guided requirements discovery
  cf prd add <file.md>         Import a PRD you already have
exit=1

Both captured from a real workspace.

Cause

typer.Exit subclasses RuntimeError, so except Exception as e catches a
command's own deliberate exit and prints e — which stringifies to the exit
code. prd_generate already had the except typer.Exit: raise guard; the
others did not.

On the count: the issue says 11, I found 5

An AST scan of app.py finds 5 structurally vulnerable commands —
tasks_generate, templates_apply, schedule_show, schedule_predict,
schedule_bottlenecks. Running every command the issue names against an empty
workspace, only 2 actually reproduce; the other 3 are latent because their
error paths need conditions an empty workspace does not produce, and the rest
either exit 0 or already guard correctly.

I did not try to reconcile the number, because the fix that matters is not a
list of commands.

The scanner is the rule

tests/cli/test_typer_exit_not_swallowed_1113.py fails for any function that
raises typer.Exit inside a try whose broad handler would catch it, naming the
function and line:

these raise typer.Exit inside a try whose broad handler will catch it and print
the exit code as 'Error: <n>'. Add `except typer.Exit: raise` above the catch-all:
  tasks_generate (app.py:2136)

It deliberately inspects only the try body — a raise inside a handler
propagates out of the statement rather than into a sibling handler, so counting
those would produce false positives.

This is what the issue asked for ("preferably a shared helper or a lint rule, so
a new command cannot reintroduce the pattern"). A per-command fix list would go
stale the next time someone adds a command.

Verified non-tautological, two ways

  1. A unit test proves the scanner detects the pattern in a synthetic sample — a
    scanner that cannot fail is worse than no scanner.
  2. Removing one real guard fails three tests, including the user-visible
    output check, not just the scanner.

Acceptance criteria

  • Commands re-raise typer.Exit before the catch-all
  • cf tasks generate with no PRD prints its message and nothing else
  • A test covers that case, asserting Error: 1 is absent and exit code is 1
  • A lint rule so a new command cannot reintroduce it

ruff clean; tests/cli/: 552 passed.

…1113)

typer.Exit subclasses RuntimeError, so a command's catch-all caught its own
deliberate exit and stringified the exit *code* as a message:

    Error: No PRD found.
    Add one first: codeframe prd add <file.md>
    Error: 1

To a new user that reads as a second, unexplained failure immediately after a
message that was otherwise clear and actionable.

Five commands were structurally vulnerable — tasks_generate, templates_apply,
schedule_show, schedule_predict, schedule_bottlenecks — each now re-raising
typer.Exit ahead of its catch-all, the guard prd_generate already had.

The issue lists 11 commands; an AST scan finds 5, and only 2 reproduce with the
inputs available on an empty workspace (the other 3 are latent — their error
paths need different conditions). Rather than argue the count, the scanner is
now the rule: tests/cli/test_typer_exit_not_swallowed_1113.py fails for ANY
function that raises typer.Exit inside a try whose broad handler would catch it,
which is the enforcement the issue asked for in preference to spot fixes. It
names the offending function and line.

The scanner is verified non-tautological two ways: a unit test proving it
detects the pattern in a synthetic sample, and by removing a real guard, which
fails three tests including the user-visible output check.

Verified on a real workspace: `cf tasks generate` with no PRD now prints its
message and nothing else, still exiting 1.
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @frankbria's task in 4m 9s —— View job


Bug-hunting review complete

  • Gather PR context (gh pr view, gh pr diff, existing comments)
  • Read changed files + surrounding code to confirm against real context
  • Analyze the except typer.Exit: raise guards for placement/logic errors
  • Analyze the AST scanner for false positives/negatives & logic errors
  • Post inline findings + summary

Result: no concrete defects found. Summary posted to the PR.

What I verified against the checked-out code:

  • All 5 new guards (app.py:2131, 5831, 5929, 6011, 6175) sit immediately before their except Exception catch-all, behind only FileNotFoundError/ValueError/TaskGenerationError handlers — none of which are in typer.Exit's MRO — so each guard is reachable and necessary.
  • The guards also fix a latent, more severe variant in schedule_predict/schedule_bottlenecks: their raise typer.Exit(0) ("No tasks found") was previously caught and re-raised as Exit(1) — a success→failure exit-code corruption.
  • Scanner blind spots that could cause false negatives (tuple-form broad handlers like except (Exception, …), bare-Name raise Exit(...), except RuntimeError) were each checked and confirmed absent from app.py.
  • The tasks_generate nested try/except Exception: (body @2049) does not raise typer.Exit itself and bare-raises, so it neither needs nor defeats the outer guard.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review

Solid, well-scoped fix. The root cause analysis is correct (typer.Exit subclasses RuntimeError, so a bare except Exception catches and re-stringifies it), the fix mirrors the existing prd_generate guard, and all five except typer.Exit: raise blocks are placed correctly — before the broad handler, so they actually take effect at runtime (verified by reading each of the five sites in app.py, not just the diff).

Finding: the scanner doesn't check handler order

find_unguarded_exits() in tests/cli/test_typer_exit_not_swallowed_1113.py only checks whether a typer.Exit-typed handler exists somewhere in node.handlers:

guarded = any(
    isinstance(name, ast.Attribute) and name.attr == "Exit"
    for handler in node.handlers
    for name in _handler_names(handler)
)

It never checks that this handler appears before the broad one. Python matches except clauses top-to-bottom, first match wins — so a future command that (accidentally) adds except typer.Exit: raise after except Exception as e: would pass this test (the AST sees a qualifying handler) while still swallowing the exit at runtime, reproducing the exact bug #1113 fixed. Given the PR's stated goal is "a new command cannot reintroduce the pattern," this is a real gap in that guarantee — worth a follow-up: compare node.handlers.index(exit_handler) against node.handlers.index(broad_handler) and require the former to come first.

Relatedly, the check only confirms a typer.Exit-typed handler exists, not that its body actually re-raises — except typer.Exit: pass would also satisfy guarded=True while silently swallowing the exit (a worse regression, since the command would fall through instead of exiting 1 at all).

Minor: _BROAD doesn't include RuntimeError

_BROAD = {"Exception", "BaseException"}. The PR's own root-cause explanation is that typer.Exit subclasses RuntimeError — so a hypothetical except RuntimeError as e: console.print(...); raise typer.Exit(1) written as a "catch survivable errors" clause would reproduce the identical bug (typer.Exit is-a RuntimeError) but isn't flagged as broad by this scanner. Not exploitable today (grepped app.py, no bare except RuntimeError), but given the scanner is meant to be the durable enforcement mechanism, it's worth including for the same reason Exception/BaseException are.

Both of the above are gaps in the scanner's robustness, not the current fix — the five sites touched today are correctly ordered and verified to work.

Everything else looks good

  • Comments on each guard are identical and accurate, consistent with the existing prd_generate pattern.
  • Test coverage is strong: a meta-test proving the scanner isn't tautological, plus behavioral assertions on actual CLI output (Error: 1 absence, correct exit code, message presence) for the two commands that reproduce on an empty workspace.
  • The PR body's honesty about the count discrepancy (issue says 11, AST finds 5, only 2 repro) is good practice and matches what I verified by reading the code — no over- or under-claiming.
  • No security or performance concerns; this is CLI-only error-handling control flow.

@github-actions

Copy link
Copy Markdown
Contributor

✅ GLM review: no defects found.

Checked against the checked-out code: the 5 except typer.Exit: raise guards are correctly placed (each immediately before its except Exception catch-all, behind only handlers outside typer.Exit's MRO), and the scanner's plausible false-negative surfaces — tuple-form broad handlers (except (Exception, …)), bare-Name raise Exit(...), and except RuntimeError — are all absent from app.py.

@frankbria
frankbria merged commit 008e67d into main Aug 10, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[P2.29] 11 CLI commands print a bogus 'Error: 1' line after every intentional error exit

1 participant