Skip to content

fix(cli): propagate a failed one-shot turn's exit code on the non-quiet route - #110446

Closed
LBHermes wants to merge 1 commit into
NousResearch:mainfrom
LBHermes:fix/kanban-worker-exit-code
Closed

LBHermes wants to merge 1 commit into
NousResearch:mainfrom
LBHermes:fix/kanban-worker-exit-code

Conversation

@LBHermes

Copy link
Copy Markdown

What does this PR do?

hermes chat -q — the non-quiet one-shot route — exits 0 on a failed turn. The
kanban dispatcher spawns every non-goal_mode worker on exactly that route
(hermes -p <profile> --cli --accept-hooks chat -q "work kanban task <id>"; -Q is
appended only when task.goal_mode), so a worker whose turn died mid-flight on a provider
failure was booked as "worker exited cleanly (rc=0) without calling
kanban_complete/kanban_block — protocol violation"
: the real cause is hidden, the failure
is charged to the worker's protocol-violation budget, and the retry re-runs the whole task
from zero.

The exit-code contract already existed — but only in the -Q branch (rc=1 on a failed
turn, EX_TEMPFAIL (75) for a kanban rate-limit/billing wall). This PR extracts that
existing decision into one helper and has both one-shot routes use it.

Related Issue

No issue is closed by this PR. Same defect class, open and not authored by me: #91177, #101800.

Duplicate audit (repo PR checklist item)

The template asks me to search for existing PRs, so here is the honest result: this defect
has a long open queue, and I am not claiming to be first. Open PRs on the same route:
#101238, #107944, #104351, #98969, #100771, #107493, #97623, #102318, #86345, #91206
(plus the dispatcher-side classifiers #107432, #109468). Several carry the duplicate label.

Why this one may still be the cheapest to land — and I am happy to close it in favour of
whichever you prefer, or to fold its evidence into your pick:

  • Smallest footprint. It sits on the Footprint Ladder rung 1 (extend existing code):
    it reuses the decision that is already in cli.py and introduces no new module, no new
    env var, no new config key. fix(cli): non-interactive chat -q reports failures in its exit code #101238 adds hermes_cli/single_query_exit.py (+82 lines);
    several others add a second copy of the decision.
  • Fixes the whole class, not just the quota arm. The helper returns 0 / 1 / 75 for both
    routes, so a provider transport death, a non-retryable 4xx and a quota wall all stop
    exiting 0 — not only the rate_limit/billing branch.
  • Rebased on current main with the reproduction re-captured on it (ee4452991d), and it
    is the only one in the queue carrying a fleet-level impact measurement (below).

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • cli.py
    • extract _single_query_exit_code(result) from the -Q branch; the quiet route now calls it
      (behaviour unchanged), so there is one decision instead of two.
    • non-quiet route: after chat() + _print_exit_summary(), exit with
      _single_query_exit_code(cli._last_turn_result); 1 when no turn result exists at all
      (credentials / agent-init failure), matching the quiet route.
  • hermes_cli/cli_chat_turn_mixin.py — chat() retains the turn result as
    self._last_turn_result (class default None), so the one-shot caller can read it.
  • tests/hermes_cli/test_single_query_exit_code.py (new) — invariant tests for the shared
    decision and for the non-quiet route's exit code.
  • Two existing tests pinned the non-quiet branch as returning, which the route no longer
    does; both keep their original intent and now assert the exit code too:
    • tests/hermes_cli/test_single_query_session_finalize.py::test_human_single_query_main_finalizes_after_query
      — session still finalized, now asserts SystemExit.code == 0 on success.
    • tests/hermes_cli/test_chat_q_exit_clear.py::test_single_query_main_skips_clear_on_exit_summary
      — exit summary still not cleared, now asserts SystemExit.code == 0 on success.
      Their stubs also retain a turn result, because production chat() does.

Line-level cause (current main, ee4452991d)

  • cli.py:4442-4443 — the non-quiet branch calls cli.chat(...) then
    cli._print_exit_summary(clear_screen=False) and falls out of the try/finally into
    main(): no exit code is computed at all, so the process exits 0 whatever the turn
    result was.
  • cli.py:4114-4126 — the same contract implemented only in _run_quiet_single_query.
  • hermes_cli/kanban_db_dispatch.py:2127-2131 — -Q is appended only when task.goal_mode,
    so every normal worker takes the broken route.

How to Test

All of the following is fresh output from this branch, produced with the repo's own runner
(scripts/run_tests.sh) against the shared venv, TZ=UTC, temp HERMES_HOME.

1. Red on base, green patched

tests/hermes_cli/test_single_query_exit_code.py on main @ ee4452991d (fix not applied):

=== Summary: 1 files, 0 tests passed, 5 failed (100% complete) in 2.2s ===
E   AssertionError: a failed non-quiet one-shot turn exited None: rc=0 is read by the
    kanban dispatcher as a protocol violation, hiding provider crashes
E   assert None == 1
E   AssertionError: a successful one-shot turn must exit 0, got None

The non-quiet route produced no exit code at all — the failure is not "wrong code", it is
"no code". Same file on this branch:

=== Summary: 1 files, 5 tests passed, 0 failed (100% complete) in 0.8s ===

2. End-to-end A/B on the real CLI route

Same command shape, same host, same provider error (-m definitely-not-a-real-model-xyz
→ non-retryable 404 inside a single-query turn), base tree vs patched tree, exit code captured
from the process:

case tree command shape rc
failed turn, non-quiet base ee4452991d --cli --accept-hooks chat -q "…" 0 ← the bug
failed turn, non-quiet patched 06fc56b21e --cli --accept-hooks chat -q "…" 1
success, non-quiet base ee4452991d --cli --accept-hooks chat -q "…" 0
success, non-quiet patched 06fc56b21e --cli --accept-hooks chat -q "…" 0
failed turn, -Q base ee4452991d … chat -q "…" -Q 1
failed turn, -Q patched 06fc56b21e … chat -q "…" -Q 1 (unchanged)

The quota-wall arm (75 / EX_TEMPFAIL, HERMES_KANBAN_TASK set) is pinned by unit test
rather than this probe: the probe's failure mode is a 404, not a 429/billing wall.

3. No regressions

scripts/run_tests.sh tests/hermes_cli/ on this branch (1088 files):

=== Summary: 1088 files, 11257 tests passed, 109 failed, 144 skipped (100% complete) in 543.3s ===

That directory has pre-existing failures on this host (environment: no managed uv, network
and PTY-dependent files), so the honest check is the same failing file set run on both trees
— base ee4452991d vs this branch, identical inputs, identical counts:

base    (ee4452991d):      24 files, 168 tests passed, 108 failed, 15 skipped
patched (06fc56b21e):      24 files, 168 tests passed, 108 failed, 15 skipped
per-file pass/fail counts: identical for all 24 files (0 differences)

So the branch introduces zero additional failures; every remaining failure in that
directory is pre-existing on main in this environment.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(cli): …)
  • I searched for existing PRs — see the duplicate audit above
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the test suite for the affected areas (scripts/run_tests.sh tests/hermes_cli/, 1088 files) — no new failures vs base, see §3
  • I've added tests for my changes (invariant tests, proven red on base)
  • I've tested on my platform: Ubuntu 24.04 (Linux), Python 3.11.15

Documentation & Housekeeping

  • N/A — no user-facing docs, config keys, cli-config.yaml.example, tool schema or
    architecture change: the exit code is already the documented contract on the -Q route
  • Cross-platform impact considered: no OS-specific code path, no new subprocess/argv shape
  • For New Skills — N/A, deleted

Screenshots / Logs

[red-on-base]

=== Summary: 1 files, 0 tests passed, 5 failed (100% complete) in 2.2s (8 workers) ===
tests/hermes_cli/test_single_query_exit_code.py::test_failed_turn_is_nonzero_off_kanban FAILED
tests/hermes_cli/test_single_query_exit_code.py::test_clean_turn_is_zero FAILED
tests/hermes_cli/test_single_query_exit_code.py::test_kanban_quota_wall_keeps_tempfail_sentinel FAILED
tests/hermes_cli/test_single_query_exit_code.py::test_nonquiet_one_shot_propagates_failed_turn FAILED
tests/hermes_cli/test_single_query_exit_code.py::test_nonquiet_one_shot_still_exits_zero_on_success FAILED

[green-patched]

=== Summary: 1 files, 5 tests passed, 0 failed (100% complete) in 0.8s (8 workers) ===

[regression]

# whole directory, this branch
=== Summary: 1088 files, 11257 tests passed, 109 failed, 144 skipped (100% complete) in 543.3s ===

# the 24 files that fail here, base vs patched (identical inputs)
base    ee4452991d:  24 files, 168 passed, 108 failed, 15 skipped
patched 06fc56b21e:  24 files, 168 passed, 108 failed, 15 skipped

Why this matters beyond one board (measured, not estimated)

Across the reporting board's whole history there are 42 runs recorded as clean-exit
protocol violations (2026-07-27 → 2026-09-13; harvey 16, sloane 12, craig 5, vance 4, mira 4,
reid 1). Matching each run's own [start, end+3min] window against that profile's retained
agent.log* finds a terminal provider failure inside the run's own window for 13 of the 42
— e.g. API call failed after 3 retries — Upstream error from Together: Stream error: h2 protocol error and Gemini 503/429, plus one credit wall and one broken pipe.

13/42 is a lower bound, not a rate: profile log retention reaches back only to roughly
Aug 10 – Sep 8 while the run set starts in July, so the July/August runs are unmatchable by
construction. The mechanism is structural either way — on this route every terminal turn
failure exits 0 and lands in the protocol-violation bucket.

After this fix the same event is booked as pid <n> exited with code 1 (outcome crashed,
with the real error text), and a quota wall exits 75, which the dispatcher already releases
without counting a failure.

…et route

The exit-code contract (rc=1 on a failed turn, EX_TEMPFAIL for a kanban
rate-limit/billing wall) existed only in the -Q branch. The kanban dispatcher
spawns workers as `hermes -p <profile> --cli chat -q ...` (no -Q unless
goal_mode), so a worker whose turn died on a provider failure exited 0 while
still 'running' and the dispatcher booked it as a protocol violation
(clean exit without kanban_complete/kanban_block) — hiding provider crashes
and quota walls behind a task-failure label and burning the violation retry
budget on retries that rerun the whole task.

- extract `_single_query_exit_code(result)`; both one-shot routes use it
- chat() retains the turn result as `cli._last_turn_result`
- the non-quiet route exits with it (init/credential failure => 1)

Tests: `tests/hermes_cli/test_single_query_exit_code.py` pins the shared
decision and the non-quiet route's exit code (red on base: the route produced
no exit code at all). Two existing tests
(`test_human_single_query_main_finalizes_after_query`,
`test_single_query_main_skips_clear_on_exit_summary`) pinned the route as
*returning*; their stubs now retain a turn result and the tests keep their
original intent (session finalized / exit summary not cleared) while asserting
the route exits 0 on success.

Verified end-to-end on this host: base exits 0 / patched exits 1 for the same
failed turn; success still 0; -Q unchanged (1).
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation duplicate This issue or pull request already exists labels Sep 14, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Duplicate of #101238 for triage tracking: same defect (non-quiet chat -q exits 0 on a failed turn) and same fix class (shared 0/1/75 exit decision for both one-shot routes). This PR is the smaller-footprint variant, so a maintainer may still prefer it; leaving both open for a human pick. Cluster also includes #86345, #98969, #107944, #104351, #107493, #102318, #91206.

@teknium1

Copy link
Copy Markdown
Collaborator

Landed on main at 1b2a75f via #112500, which salvaged #110917 (@dmelkk-secondbrain) — the same fix as this PR: chat -q kanban workers now share the -Q exit contract (0 success, 1 failure, 75 = EX_TEMPFAIL on a rate-limit/billing wall → rate_limited, requeued without a failure tick), gated on HERMES_KANBAN_TASK. Kanban workers on a quota wall are no longer booked as protocol violations.

Credit where due: the earliest PR fixing this exact fall-through was #97623 by @rodrigogs (Aug 29), and this cluster grew to ten PRs before one landed — my sweep should have picked up the earliest, and I have noted the precedence on the merged PR. Closing this as implemented on main; thank you for the fix.

The unconditional non-zero for a human's chat -q did not land — behaviour change for existing scripts, design call tracked under #111770.

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

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants