Repository navigation
fix(cli): a rate-limited kanban worker was recorded as a protocol violation (exit 75 only existed on the -Q path) - #97623
Conversation
|
For the record — not a duplicate of #85083 at the mechanism level (no shared files; merges cleanly in either order), though both close the same user-visible bug:
Difference in coverage: after #85083, workers spawned by the dispatcher are safe by construction, but any kanban worker invoked outside |
ac98288 to
b8171f1
Compare
Port onto the post-split layout: the one-shot exits now live in _run_quiet_single_query (quiet branch) and _run_single_query_mode (human branch) instead of inline in main(), and the turn verdict is parked on the CLI by hermes_cli/cli_chat_turn_mixin.py, where the agent thread's result dict is actually in hand after the split. tests/cli/test_kanban_worker_quota_wall_exit.py: 8 passed; guards repinned to the two one-shot runners (exactly one helper call each, two module-wide). Mutation-checked: all 8 fail with the fix stashed. Neigh bors green: test_chat_q_exit_clear + test_single_query_session_finalize (9), test_cli_quiet_stdout_leak + test_kanban_db (37).
b8171f1 to
ca8440c
Compare
|
Landed on main at 1b2a75f via #112500, which salvaged #110917 (@dmelkk-secondbrain) — the same fix as this PR: 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. Yours was first. Sorry the salvage picked a later sibling; the landed diff is the same mechanism as yours (shared exit mapping + |
Three cards on a live board sat blocked with
worker exited cleanly (rc=0) without calling kanban_complete or kanban_block — protocol violation. Nine runs between them, every worker log ending the same way:The worker did nothing wrong. The provider walled it off, and the exit status said "clean exit" — so the dispatcher, which holds a pid and not a transcript, read the only channel it has and concluded the worker had skipped the protocol. Each one counted a failure until the card was blocked.
The contract already existed, on one of two paths
KANBAN_RATE_LIMIT_EXIT_CODE(75,EX_TEMPFAIL) means "quota wall, not a task error"; the reap classifier maps it to arate_limitedexit and releases the task back toreadywithout incrementing the failure counter, so a long quota window cannot trip the circuit breaker.run_conversationalready returnsfailure_reason(agent/conversation_loop.py) for exactly this purpose.That consumer lives in the
-Qbranch of the one-shot path (_run_quiet_single_queryincli.py). Workers are spawned with-Qonly in goal mode (if task.goal_modein_worker_argv, used by_default_spawn), so an ordinary card —goal_mode=0, the common case — runs the human-facing branch of_run_single_query_modeinstead:cli.chat(...),_print_exit_summary(), return. No exit-code logic at all, so the process exits 0. All three blocked cards hadgoal_mode=0.What changes
_kanban_quota_wall_exit_code(result)and both one-shot runners (_run_quiet_single_queryand_run_single_query_mode) ask it. It returns the sentinel only for a kanban worker whose turn failed on a quota-wall reason, andNoneotherwise — leaving each caller's own 0/1 contract untouched, so non-kanban one-shot runs behave exactly as before.chat()(inhermes_cli/cli_chat_turn_mixin.py) parks the turn's result on the CLI as_last_turn_result. It returns the response text, and the human path needsfailure_reason; the assignment sits where the agent thread's dict is already in hand, beside the existingconversation_historyupdate.upstream_rate_limitis deliberately not a quota-wall reason. The tuple is the one the-Qpath has always used, and a per-model 429 from an aggregator can be durable for that model — requeueing on it would spin. Widening it is a separate decision with its own risk.Relationship to #85083
Not a mechanism-level duplicate (no shared files; merges cleanly in either order), though both close the same user-visible bug. #85083 makes
_default_spawnalways pass-Q, so dispatcher-spawned workers land on the branch that already exits 75. This PR fixes the exit semantics where they are decided, so any kanban worker running underHERMES_KANBAN_TASKwithout-Q(manual run, other tooling, a future spawn path) also exits 75 instead of 0. Keeping both is harmless.Verification
8 new tests in
tests/cli/test_kanban_worker_quota_wall_exit.py(all pass on the rebased branch; neighbourstests/cli/test_chat_q_exit_clear.py,tests/cli/test_single_query_session_finalize.py,tests/hermes_cli/test_kanban_db.py,tests/hermes_cli/test_kanban_worker_spawn_toolsets.pygreen; ruff clean on the changed files).Mutation-verified: 5 mutants, 0 survivors — dropping the human-path call; dropping the
_last_turn_resultassignment (which would leave the fix in place and inert); dropping the worker-only env guard; dropping the reason check; restoring the inline tuple in the-Qbranch. Each is killed by a different assertion.The negative assertion is the one that matters most: a
server_error,timeout,auth_permanentorupstream_rate_limitfailure must not be laundered into a throttle, or the breaker stops seeing failures the task itself caused.Two tests read the source rather than run it. Since the
cli.pysplit, the two one-shot exits live in_run_quiet_single_queryand_run_single_query_mode, which still build a whole CLI before reaching them, so there is no honest way to exercise them in-process — and what is worth pinning is structural anyway: that the reasons are named in exactly one place, and that both runners ask the helper (exactly one call each, two module-wide). The producer side of this contract had no test at all before, which is how the asymmetry survived.