spec-kitty / spec-kitty/spec-kitty
accept --allow-fail exits 1 identically to plain failure — flag has no effect on automation-visible outcome
- Dominant language
- Python
- Stars
- 1.6k
- Forks
- 165
- Avg merge
- 14h 52m
- Merged PRs (30d)
- 303
Description
## Summary
`spec-kitty accept --mission --allow-fail` and `spec-kitty accept --mission ` (no flag) produce an **identical exit code (1)** when the mission has outstanding acceptance issues. The flag's own `--help` text ("Return checklist even when issues remain") implies it should let the command succeed, or at least meaningfully change behavior for a caller that gates on exit code — but it doesn't. This makes `--allow-fail` a no-op for its stated purpose in any CI/automation context.
## Root cause
`src/specify_cli/cli/commands/accept.py:717-729` (v3.2.6, commit `b04da00e1`):
```python
if not summary.ok:
if json_output:
print(json.dumps(summary.to_dict(), indent=2))
else:
_print_acceptance_summary(summary)
if not allow_fail:
_safe_emit_error_logged("Outstanding acceptance issues detected")
if not json_output:
console.print(
"\n[red]Outstanding acceptance issues detected. Resolve them "
"before merging or rerun with --allow-fail for a checklist-only "
"report.[/red]"
)
raise typer.Exit(1)
raise typer.Exit(1) # <-- identical exit code to the branch above
```
Both branches raise `typer.Exit(1)`. `typer.Exit` carries only an int exit code — no metadata that could let a caller distinguish these two raises by anything other than process exit code, which is identical either way. The only two observable differences are:
- a suppressed Rich console line
- a suppressed internal telemetry call (`_safe_emit_error_logged`), which writes to `specify_cli.sync.events` — not visible to a shell/CI job checking `$?`.
`_print_acceptance_summary(summary)` (the "checklist") is already printed **unconditionally** above this block, regardless of `--allow-fail` — so the flag's documented purpose ("return checklist even when issues remain") is already default behavior with or without the flag. There is nothing left for the flag to gate.
For comparison, `--mode checklist` (`accept.py:715`) is `raise typer.Exit(0 if summary.ok else 1)` — it also does **not** override a failing summary to exit 0. So currently there is no code path anywhere in `accept` that produces a non-blocking, exit-0 "checklist-only" report when real acceptance issues remain, despite one being clearly implied by `--allow-fail`'s help text.
## History
Not a regression — broken since the flag's introduction in `c0b5731ef` ("Add feature acceptance workflow", 2025-10-22):
```python
if not allow_fail:
console.print("...rerun with --allow-fail for a checklist-only report...")
raise typer.Exit(1)
raise typer.Exit(1)
```
Every subsequent refactor (`a9e334e1d`, `cfe1b6562`, `44b935e6e`, `e36547461`, `64d0eea4c`) carried the identical dual-`Exit(1)` shape forward verbatim.
## Test gap
No test in `tests/` exercises `allow_fail=True` against a **failing** summary (`summary.ok is False`) and asserts an exit code.
- `grep -rn "allow_fail" tests/` — every direct-call test passes `allow_fail=False` (the default).
- The only two CLI tests passing `--allow-fail` (`tests/agent/test_commands.py:244,268`) use a `DummySummary` with `ok = True` **and** `--mode checklist`, which returns via the checklist-mode branch (line 715) before the `allow_fail` block is ever reached. They assert `exit_code == 0` because `summary.ok` is `True`, not because of `--allow-fail`.
- A stray comment in `tests/specify_cli/cli/commands/test_accept_clean_tree.py:346-348` ("`--allow-fail` / `--lenient` paths can reach the commit step with other changes present") is misleading relative to current code — when `summary.ok is False`, both branches raise `Exit(1)` before the commit step, so `--allow-fail` alone cannot reach it today.
## Other consumers
The only internal caller of `allow_fail` besides the CLI option itself is `src/specify_cli/cli/commands/agent/mission_accept_merge.py:127`, which hardcodes `allow_fail=False` ("Agent commands use strict validation") and never exercises the `True` path. No internal caller depends on the current (broken) behavior.
## Reproduction
1. Create/checkout a mission with an outstanding acceptance issue (e.g. an unapproved WP, a failing negative invariant, or an unrelated path-convention warning).
2. `spec-kitty accept --mission ` → exit code 1.
3. `spec-kitty accept --mission --allow-fail` → exit code 1 (identical).
4. `diff` the two stdout captures: identical except one suppressed red console line.
## Suggested fix
Make the `allow_fail=True` arm exit 0 (non-blocking, report-only), matching the documented intent:
```python
if not allow_fail:
...
raise typer.Exit(1)
raise typer.Exit(0) # allow_fail: report-only, don't block automation
```
This is an exit-code contract change and should be a maintainer decision rather than a unilateral patch — please add a regression test asserting the chosen behavior for `allow_fail=True` + `summary.ok=False`, since none currently exists.
---
Found while dogfooding `spec-kitty accept`/`merge` on a real mission in `Priivacy-ai/spec-kitty-saas`, independently verified against a fresh clone of this repo (commit `b04da00e1`, matching the installed v3.2.6 CLI).
Contributor guide
Research direction
Start in src/specify_cli/cli/commands/accept.py around lines 715-729 and trace the failing-summary path for allow_fail. Review the existing acceptance CLI tests, especially tests/agent/test_commands.py and tests/specify_cli/cli/commands/test_accept_clean_tree.py, then add a regression test for allow_fail=True with summary.ok=False. Done means the test captures the maintainer-approved exit-code contract and the full relevant test suite passes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- cli, testing-qa
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 68/100