feat(goals): unbounded turn-budget sentinel + judge kill-switch - #69308
feat(goals): unbounded turn-budget sentinel + judge kill-switch#69308Slimydog21 wants to merge 2 commits into
Conversation
Add a config-gated way to run the /goal loop unbounded: - resolve_goal_max_turns(): single source of truth mapping goals.max_turns (0 / negative int / unbounded-strings) to None (unbounded); positive ints pass through; unset/bool/junk fall back to DEFAULT_MAX_TURNS (20). - goal_judge_enabled(): reads goals.judge_enabled (default true); when false a judge 'done' verdict is coerced to 'continue' in both the main loop and the kanban worker loop, while last_verdict still records the true verdict. - Migrate every read site (cli, gateway, tui_gateway x2) from the lossy int(... or 20) idiom to the resolver so the 0-sentinel is honored. - Budget-skip guard treats None as unbounded (never reaches >=); all max_turns consumers are None-safe (goal_budget_label/_fmt_turns). - Harden GoalState.from_json and GoalManager.set to route through the resolver so a literal 0 collapses to unbounded, never a finite-0 budget. - New locale key gateway.goal.set_unbounded across all 16 locales. - Config defaults + docs (goals.md, configuration.md). Tests: 61 unit + 4 gateway E2E; i18n key-parity suite green.
The rebase onto origin/main picked up the upstream transport_failed return value; the stub must mirror the real 5-tuple (verdict, reason, parse_failed, wait_directive, transport_failed) or evaluate_after_turn's unpack fails.
alt-glitch
added
type/feature
| # else (positive int) is a normal finite budget. | ||
| _UNBOUNDED_MAX_TURNS_STRINGS = {"unbounded", "infinite", "infinity", "none", "no limit", "unlimited"} | ||
|
|
||
|
|
There was a problem hiding this comment.
Why a resolver at all: the five read sites each carried their own copy of int(x) or 20, and Python's 0 or 20 -> 20 made the "unbounded" sentinel unwritable — falsiness was being used to detect absence (the NULL-vs-0 bug one layer up). One resolver is what makes the sentinel mean the same thing at every site.
Note the bool trap two branches down: isinstance(True, int) is True in Python, so without the explicit bool branch a stray max_turns: true in YAML would silently become a 1-turn budget.
| # loop is bounded only by the judge / user / preemption). A positive int | ||
| # is a finite budget. ``0`` never reaches here — resolve_goal_max_turns | ||
| # maps the 0/negative/unbounded-string config sentinel to ``None`` before | ||
| # state is constructed. |
There was a problem hiding this comment.
Why Optional[int] and not a magic large int: 10**6 still bounds — it just lies about intent and produces banners like 3/1000000. None is the only honest representation of "no budget," and every consumer (_fmt_turns, goal_budget_label, the budget check) is None-aware by construction.
| # ``0 >= 0``. ``None`` (unbounded) is preserved by the first branch; | ||
| # positive ints pass through; junk/unrecognised values fall back to | ||
| # ``DEFAULT_MAX_TURNS`` via the resolver. | ||
| max_turns = None if raw_max_turns is None else resolve_goal_max_turns(raw_max_turns) |
There was a problem hiding this comment.
Why route deserialization through the resolver: a hand-edited or legacy state file can carry max_turns: 0. The old int(...) or DEFAULT would resurrect that as 20; a naive int(0) would create a finite-0 budget that instantly fires 0 >= 0. This line makes the finite-0 budget unconstructible from persisted state. (Surfaced in adversarial review as a dormant fail-closed path; hardened here and pinned by regression tests in test_goal_unbounded_budget.py.)
| # reason as a diagnostic in the continuation banner); wait/continue | ||
| # are unaffected. The budget, an explicit user stop, and preemption | ||
| # remain as exits — the judge simply loses its power to terminate. | ||
| if verdict == "done" and not goal_judge_enabled(): |
There was a problem hiding this comment.
Ordering is the invariant: state.last_verdict (two lines up) records the judge's true verdict before this coercion rewrites the local. An operator who disarms the judge keeps full observability into where the judge would have stopped — the judge is demoted from guillotine to advisor, not silenced. The coercion also sits after the 5-tuple unpack so upstream's transport_failed handling below is untouched.
|
|
||
| if state.turns_used >= state.max_turns: | ||
| # Budget exit — skipped entirely when max_turns is None (unbounded): | ||
| # the loop is then bounded only by the judge, the user, or preemption. |
There was a problem hiding this comment.
Why is not None and not truthiness: None (unbounded) must skip this exit entirely — a bare >= would TypeError, and any or-style guard would silently re-impose 20 on an operator's explicit unbounded setting. By the resolver invariant, a literal 0 can never reach this line, so the guard only ever compares a positive int.
| if verdict == "wait": | ||
| verdict = "continue" | ||
| # Judge kill-switch: a disabled judge cannot end the kanban loop. | ||
| if verdict == "done" and not _judge_enabled: |
There was a problem hiding this comment.
Same gate as the main loop — a kill-switch covering one of two loops is a lie. _judge_enabled is captured once per worker run rather than per turn: re-reading config every iteration would pay IO per turn and, worse, let a mid-run config edit produce a loop that honors some done verdicts and coerces others.
Disclosed gap (PR body §6): this path's max_turns normalization above still applies its own ... or DEFAULT collapse; the closing commit is prepared.
|
|
||
| goals_cfg = (load_config() or {}).get("goals") or {} | ||
| return int(goals_cfg.get("max_turns", 20) or 20) | ||
| return resolve_goal_max_turns(goals_cfg.get("max_turns")) |
There was a problem hiding this comment.
Why resolve at command-handling time, not construction: GatewayRunner is long-lived, so resolving per /goal command means an operator's config.yaml edit takes effect on the next goal with no gateway restart. The except below falls to finite 20 on any error — malformed config must fail toward the bounded attractor, never silently grant an unbounded loop.
(Annotation note: this head still says -> int though the resolver can return None; corrected to -> Optional[int] in the prepared follow-up commit.)
|
Reviewer guide — the shortest path through this diff, and where the risk lives. Read order. Start at The invariant chain (each link has an inline comment): resolver ⇒ literal The one ordering that matters: in Where the risk is (disclosed, not hidden):
Process: code and PR body were each adversarially reviewed by a different model than the author. The code round's two findings (dormant fail-closed paths) are hardened here with regression pins; the body round's accepted findings are visible in the body's §1 (fourth termination category), §3 (R5 marked partial), and §7 (worst-case disclosure). |
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real configuration bug: current origin/main still turns goals.max_turns: 0 into 20 at cli.py:10277, gateway/run.py:17808, tui_gateway/server.py:9374, and hermes_cli/goals.py:462.
Problems
hermes_cli/goals.py:797does not fail safe as documented.return bool(value)makes YAMLnull,[], and{}disable the judge; the PR then coercesdonetocontinueathermes_cli/goals.py:1581.- The disclosed kanban gap is still material: the worker normalizes falsy budgets back to the default at
hermes_cli/goals.py:1839on the PR head, so the advertised unbounded budget is not consistent across the stated loops. - The new docs' “only explicit stop” claim omits existing automatic parse/transport-failure pauses (
hermes_cli/goals.py:1645,:1676on the PR head).
Suggested changes
- Whitelist supported
judge_enabledvalues and test null/collection inputs as enabled. - Complete Optional-budget propagation for kanban, or narrow the declared scope.
- Preserve the circuit breakers and revise the docs. Current main moved
DEFAULT_CONFIGtohermes_cli/config_defaults.py(hermes_cli/config.py:935), so carry the new defaults there during salvage.
Automated hermes-sweeper review.
| return value | ||
| if isinstance(value, str): | ||
| return value.strip().lower() not in {"false", "no", "off", "0", "disabled"} | ||
| return bool(value) |
There was a problem hiding this comment.
This is not fail-safe for malformed YAML: null, [], and {} all make bool(value) false, so they disable the judge and cause the done-to-continue coercion below. Accept only explicit supported false encodings; return True for all other types/values, with regression tests for null and collections.
teknium1
added
sweeper:risk-session-state
3 participants
§1 — First principles: what an agent loop is, and what may stop it
An agent is a loop: observe → decide → act → observe. Stripped to its ontology, such a loop has three operator-legible termination conditions:
A fourth category exists but is not operator-legible: involuntary environmental exhaustion — context-window overflow, provider failure, unhandled exception. It is neither set (T2) nor chosen (T3) nor judged (T1); it simply happens to the loop. This PR neither arms nor disarms it; it is noted so the taxonomy is complete rather than flattering.
Two claims follow. First, a turn cap is T2: a governor, not a correctness property — the number 20 encodes a default risk posture, nothing about the task. Its value must therefore be operator-settable, including the value "no cap" (T2 disarmed; T1/T3 remain). Second, T1 must be demotable from guillotine to advisor without going blind — an operator who distrusts a premature "done" still wants the evaluator's signal.
One structural premise, stated rather than smuggled: a real system instantiates this loop at multiple call sites and in multiple concrete implementations (Hermes has a main loop and a kanban worker loop, and historically read the cap in five places). Requirements about consistency across sites below derive from this premise, not from the abstract ontology.
§2 — The defect: conflating absence with falsiness
Every read site computed the budget as a variant of:
This uses falsiness — a property of the value domain — to detect absence — a property of the schema. In Python
0is falsy, so0 or 20 → 20: an operator's explicitly-written zero, the natural spelling of "no cap," is silently reinterpreted as "not provided." It is the classicNULL-vs-0bug, one layer up. The type had no representation for "unbounded," so the sentinel was unwritable: no value in the config language meant what the operator meant. The judge had the dual problem: T1 was always armed — no config could demote the evaluator from guillotine to advisor.§3 — The design, traceable to §1's claims plus the codebase's actual structure
Optional[int];None= unbounded. Not10**6— a magic large int lies about intent and still bounds.GoalState.max_turns: Optional[int]; budget checkis not None-guarded soNonenever reaches>=.or 20— every copy a divergence risk.resolve_goal_max_turns()— single source of truth:0/negative/"unbounded"/"none"/"infinite"→None; positive int passthrough; unset/bool/junk → 20. All sites migrated.state.last_verdict = verdictprecedesif verdict == "done" and not _judge_enabled: verdict = "continue"— in both loops. Feedback as signal, not as guillotine.max_turnson the kanban path: pending — see §6 known limitation.New config surface (both keys strictly opt-in):
goals.max_turns200/ negative / unbounded-string → no cap; positive int unchangedgoals.judge_enabledtruefalse→ judgedonecoerced tocontinuein both loops; true verdict still recorded§4 — Prior art, verified across the four sources of truth
Uncapped-but-governed agent loops are an established pattern. Each citation below does specific work; none is offered as proof that caps are wrong — §1 already established the cap is a legitimate governor whose value must be settable.
while :; do cat PROMPT.md | claude-code ; done" — with the load-bearing constraint: "Ralph can be done with any tool that does not cap tool calls and usage." A non-configurable hard cap is what makes a loop non-Ralph-capable; this PR is precisely the configurability. Companion: everything is a ralph loop — "give it a goal then looping the goal." The pattern has field evidence (on X, embedded in the same page: a Ralph-built MVP delivered at $297 against a $50k contract quote) and independent replication at YC's Agents hackathon (repomirrorhq/repomirror: an uncapped loop produced 1,000+ commits and six ported codebases overnight, filesystem as cross-iteration memory).§5 — Changes
hermes_cli/goals.py—resolve_goal_max_turns();goal_judge_enabled();Optional[int]threaded throughGoalState/GoalManager/ budget guard / labels (goal_budget_label,_fmt_turnsareNone-safe); coercion in both loops after true-verdict recording;from_json+sethardened through the resolver.cli.py,gateway/run.py,gateway/slash_commands.py,hermes_cli/cli_commands_mixin.py,tui_gateway/server.py: all migrated offint(... or 20).hermes_cli/config.py— defaults for the two keys. 16/16 locales — newgateway.goal.set_unboundedkey, full parity. Docs —goals.md,configuration.md.§6 — Evidence
test_goals.py, 4 gateway E2E, 47 i18n key-parity).ruff checkclean on all changed files.check-attribution(theAll required checks passaggregator fails in sympathy): commits are authoredf.nazer@sanabil.com, unmapped to@Slimydog21. Acontributors/emails/mapping commit is prepared and will be added to this PR.from_json/setliteral-0 pins). The body round's accepted findings are reflected in §1's fourth category, R5's partial marking, the reframed Reflexion/Magentic-One citations, and §7's combined-flags disclosure.tests/hermes_cli/sweep: 15 failures in profile/auth/backup test files reproduce identically on a cleanorigin/mainworktree (feature code absent, same 15 fail) — pre-existing Windows-environment failures, unrelated; this diff touches none of those files.Known limitation (follow-up ready): the kanban worker's separate normalization path (
run_kanban_goal_loop/cli._run_kanban_goal_loop_q) still applies its own... or DEFAULTcollapse, re-bounding an unbounded budget to 20 on that path only (R5, partial). Main loop, gateway, CLI, and TUI are covered here; the closing commit (Optional[int] + resolver routing, with tests) is prepared and will be pushed to this PR.§7 — Notes for reviewers
goals.max_turnsunbounded andgoals.judge_enabled: falsetogether removes both automatic exits. Only operator-stop (T3) and involuntary environmental failure remain. There is no absolute system ceiling beneath these two knobs. Operators choosing that combination should read it as "this loop runs until I stop it."