fix(kanban): propagate subcommand exit codes from CLI dispatch - #85
Conversation
hermes_cli/main.py dispatch called args.func(args) and discarded the return value, so hermes kanban <subcmd> always exited 0 even on handled failures (unknown task/board -> kanban_command returns 1/2). That broke the card-drop receiver's unknown-card -> 404 mapping, which keys on proc.returncode != 0, producing a false 200 for a bogus card_id. Propagate an int return via sys.exit at the single dispatch site; isinstance guard leaves handlers returning None on the implicit exit-0 path. Fixes both kanban and project (both return exit codes). Patch note: ~/.hermes/plans/hermes-patches/fix-kanban-cli-exit-codes.md
|
Claude finished @exiao's task in 1m 15s —— View job PR Review: propagate subcommand exit codes
LGTM. Minimal, correctly-layered fix. Propagating the handler return value as a shell exit code at the single general dispatch site is the right place, and the guard is now correct. Bool concern is resolved. The updated guard Handler audit confirms safety. Every
Notes (non-blocking):
No prompt-cache, role-alternation, narrow-waist, env-var, or profile-safety concerns — this is pure CLI process-exit plumbing. 0 blocking, 0 minor. |
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-import |
1 |
First entries
tests/hermes_cli/test_kanban_cli_exit_code.py:18: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
✅ Fixed issues: none
Unchanged: 6190 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
There was a problem hiding this comment.
Code Review
This pull request ensures that subcommand shell-style exit codes are correctly propagated by checking if the handler returns an integer and calling sys.exit(rc). It also adds comprehensive regression tests to verify this behavior. The review feedback correctly points out that because bool is a subclass of int in Python, isinstance(rc, int) will evaluate to True for boolean return values, which could lead to unexpected exit codes (e.g., sys.exit(True) exiting with status code 1). It is recommended to use type(rc) is int instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0fb9ca7cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
LGTM — minimal, correct fix. Propagating the handler return value as a shell exit code at the single general dispatch site is the right layer, and the isinstance(rc, int) guard preserves the implicit exit-0 path for None-returning handlers. Audited every func-registered handler (bundled + plugin): none return a bool (the isinstance(True, int) gotcha) or an int-as-data — they return None or genuine exit codes, so the change is safe and even propagates codes for more commands than the PR body lists. The subprocess test correctly exercises the real main() exit path. One non-blocking note: this also broadens the contract for third-party plugin CLI handlers that might return True on success. 0 blocking.
bool is a subclass of int, so a handler returning a success/failure flag (e.g. a plugin CLI command via PluginContext.register_cli_command) would be treated as a shell exit code -- sys.exit(True) exits 1, inverting the signal. Guard the dispatch with `not isinstance(rc, bool)` so only genuine int exit codes propagate; bools and None keep the implicit exit-0 path. Addresses gemini-code-assist and Codex P2 review findings on #85.
There was a problem hiding this comment.
LGTM — minimal, correctly-layered fix. Propagating the handler return as a shell exit code at the single general dispatch site is right, and the updated guard isinstance(rc, int) and not isinstance(rc, bool) resolves the bool-subclass concern so plugin handlers returning success/failure flags keep the implicit exit-0 path. Audited every func-registered handler: all return None or genuine exit codes (cmd_migrate/cmd_migrate_xai are now correctly propagated too as a bonus), none return int-as-data. The parametrized subprocess test exercises the real main() dispatch for both True/False. 0 blocking.
Problem
hermes kanban <subcommand>always exited0, even on handled failures. The argparse dispatch inhermes_cli/main.pycalledargs.func(args)and discarded the return value.cmd_kanban→kanban_commandreturns1(e.g.ValueErroron an unknown task) or2, butmain()threw the code away, so the process still exited0.Repro (pre-fix):
Impact: the kanban card-drop receiver (PR #84) maps unknown-card →
404only whenproc.returncode != 0. Because the CLI exited0, an inline follow-up posted to a boguscard_idreturned a false200 {"commented":true}while the comment actually no-op'd. Found during the Diligence E2E boot.Fix
Class-level, single site — the general dispatch in
main():isinstance(rc, int)guard leaves handlers returningNoneon the implicit exit-0 path (no behavior change for them).func-registered handler returns a bool or an int-as-data — onlycmd_kanban,cmd_project, andcmd_whatsapp_cloudreturn ints, all shell exit codes. So this also fixeshermes project.cmd_securityalready doessys.exit(int(code or 0))).Tests
New
tests/hermes_cli/test_kanban_cli_exit_code.pyruns the realhermes kanbanas a subprocess against a throwawayHERMES_HOME(an in-process handler call would bypass the discarding bug). Asserts unknown task → non-zero, unknown board → non-zero, successful list → 0.Ground-truth: 2 fail pre-fix, all 3 pass post-fix.
Scope note
The receiver-side integration test (real
hermes kanban comment→ 404) belongs on the #84 branch (feat/kanban-card-drop-receiver, unmerged), not onlive-config— coupling two unmerged branches would be wrong. Once #84 lands it can drop itsreturncode=1stub for a real-CLI assertion; this core fix is what makes that possible.Patch note:
~/.hermes/plans/hermes-patches/fix-kanban-cli-exit-codes.md