feat(kanban): honor kanban.default_max_runtime_seconds as a NULL-cap backstop - #156
Merged
Merged
Conversation
…tasks enforce_max_runtime() only reclaims rows where max_runtime_seconds IS NOT NULL, and nothing supplied a default: a task created without an explicit --max-runtime stored NULL and could run unbounded. On a real board 91% of dev runs (879/970) were uncapped, and one run burned 214.7 hours of Modal sandbox time (~$82, 38% of the fleet's monthly compute) before anything reclaimed it. Add _default_max_runtime_seconds(), mirroring the existing _default_assignee() helper, applied in create_task() only when the caller passed nothing. Precedence: explicit value > config default > uncapped. No change to the enforcement path, no new env var, no schema change. 0/negative is an explicit opt-out rather than a 0-second insta-kill, and any config-load or parse failure resolves to None so the old uncapped behavior is preserved rather than inventing a limit. Verified the new tests FAIL against the unwired code, and end-to-end against a live board: no flag -> 5400, explicit 600 -> 600. Patch note: ~/.hermes/plans/hermes-patches/kanban-default-max-runtime.md
…backstop enforce_max_runtime only reclaims rows where max_runtime_seconds IS NOT NULL, so a task created without --max-runtime was invisible to it. 91% of one lane's runs (879/970 over 30d) had no cap. _default_max_runtime_seconds() reads the config key and create_task() applies it only when the caller passed nothing. Precedence: explicit > config default > uncapped. Supersedes PR #153, which I closed after self-review. Fixes its four findings: - Drops the false '214.7h / $82 Modal burn' story from the function and test docstrings. That row was a leaked run (heartbeat stopped 6.4 min in, closed by manual cleanup 2026-07-22), ~4 cents of real compute. - Uses load_config_readonly() instead of load_config(); this is a pure read on every task creation and load_config() deepcopies for mutating callers. Matches the fd_headroom precedent. New test pins that the resolver does not mutate the shared cache. - Registers the key in config.py next to dispatch_stale_timeout_seconds, defaulting to 0 (no cap). It was previously unregistered, so 'hermes config set' warned it may not be read. - Documents in code that this is a BACKSTOP, not the primary reaper: dispatch_stale_timeout_seconds kills on stale heartbeat and spares long healthy runs, while a wall-clock cap cannot tell hung from slow. Set below your slowest legitimate run and it truncates real work. Patch note: ~/.hermes/plans/hermes-patches/kanban-default-max-runtime.md
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f851be89ab
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60d1157497
ℹ️ 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".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Default runtime cap for kanban tasks
Supersedes #153, which I opened and then closed the same day after self-review ("do not merge as-is"). Eric asked to reopen it; #153's branch was deleted on close, so this is the same work on a fresh branch with all four of its findings fixed.
Problem
enforce_max_runtime()is gated:A task created without an explicit
--max-runtimestores NULL, so that reaper skips it entirely. Measured on a live board: 91% of one lane's runs had no cap (879 of 970 over 30 days).The uncapped cards are the hand-created ones (
created_by=user). The PR-babysit detector already passes--max-runtimeon everything it creates (35m / 40m / 9000s), so this gap is specifically about cards made inline.Fix
_default_max_runtime_seconds()reads the config key;create_task()applies it only when the caller passed nothing.Precedence: explicit value > config default > uncapped.
No change to the enforcement path, no schema change, no new env var (per AGENTS.md, behavioral settings belong in
config.yaml). Ships defaulting to0= off.What changed vs #153
last_heartbeat_atstops 6.4 min in, closed by manual cleanup 2026-07-22, ~4 cents of real compute. Docstrings now cite the defensible 91%-uncapped figureload_config()on a pure-read path that runs on every task creation (it deepcopies, for mutating callers)load_config_readonly(), matching thefd_headroomprecedent atkanban_db.py:2230. New test pins that the resolver never mutates the shared cachehermes config setwarned "not a recognized config key"config.pybesidedispatch_stale_timeout_seconds, default0dispatch_stale_timeout_secondsalready does this, better"On the wrong-mechanism objection (#153 item 4)
It was a fair hit and I'm not pretending otherwise.
dispatch_stale_timeout_secondskills on lack of progress (stale heartbeat); this kills on wall clock.dispatch_stale_timeout_secondsThe stale sweep is the better primary reaper. What it does not cover is the NULL-cap blind spot in
enforce_max_runtime— that's the narrow gap this closes, and both the docstring and the config comment now say exactly that, including the warning that a cap below your slowest legitimate run truncates real work. Concrete example left in the comment: equity-analyst has a legitimate 354-minute completed run, so a 90-minute default would kill it. That's why this ships off (0) rather than with an opinionated default.Still open, not addressed here: the stale sweep has fired exactly once (2026-06-29) despite obvious later candidates. Worth chasing separately — two half-working reapers is worse than one working one.
Verification
Live, not just unit:
tests/cli/test_kanban_default_max_runtime.py— 14 passed. Fulltests/cli -k kanban— 36 passed.HERMES_HOME, key set to 5400: uncapped create → 5400; explicit--max-runtime 600→ 600.None, stored cap →None(old behaviour intact).ruff checkclean on all three files.Decisions
0(off), not 5400. A wall-clock cap that truncates a real 354-min run is worse than no cap. Operators opt in.0/negative = opt-out, not a 0-second insta-kill.None(uncapped), never raises into task creation.Patch note:
~/.hermes/plans/hermes-patches/kanban-default-max-runtime.md