Repository navigation
fix(terminal): the advertised foreground cap must match the enforced one - #577
Merged
Merged
Conversation
#575 made `terminal.max_foreground_timeout` a config key and routed ENFORCEMENT through `_foreground_max_timeout()`. I left the schema's quoted cap alone and called it cosmetic. It is not. MEASURED (2026-08-11, cap set to 3600 in config.yaml) at IMPORT -> constant: 600 schema BEFORE bridge: 600 enforced AFTER bridge: 3600 schema AFTER bridge: 600 <- the model is told 600 `TERMINAL_SCHEMA` is built at import; `_ensure_terminal_env_bridged` runs on first tool use, AFTER import. So the description froze the pre-bridge value. WHY IT MATTERS The description is the ONLY thing the model reads when choosing a timeout. A schema that understates the cap silently holds the model to the old limit and makes the config key look inert even though enforcement honours it — the user raises the cap, nothing appears to change. Drift the other way is worse: an overstated cap invites a timeout that is then rejected. FIX * `_timeout_param_description(cap)` — one builder, used by both the schema literal and the refresh, so the two copies of the text cannot drift. * `_refresh_terminal_schema_limits()` re-renders it, called from the bridge's `finally` so a partial or failed bridge still leaves schema and enforcement agreeing. * Mutates the dict IN PLACE: the registry holds a reference to that same object, so rebinding the name would leave the registered copy stale. A test asserts `registry.get_entry("terminal").schema is TERMINAL_SCHEMA` rather than trusting it. Also fixes a pre-existing HERMETICITY bug found by this work: `test_default_value_is_600` read the live `FOREGROUND_MAX_TIMEOUT`, so it failed on UNMODIFIED fork/main for anyone who had set the config key (the bridge exports the env var process-wide and it leaks into the test run). Verified failing on a clean `--detach fork/main` worktree before touching it. It now pops the variable and reloads to assert the actual default. TESTS (6, RED-proven) Reverting to the frozen description turns 3 red. Covers a raised cap, a LOWERED cap, refresh-through-the-registry, the bridge end-to-end, and the `finally` path when the bridge raises. E2E, the exact reproduction: at import -> schema: 600 | enforced: 600 after bridge -> schema: 3600 | enforced: 3600 AGREE tests/tools -k terminal: 195 passed, 5 skipped, 0 failed.
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
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.
I called this cosmetic. It isn't.
#575 made
terminal.max_foreground_timeouta config key and routed enforcement through_foreground_max_timeout(). I left the schema's quoted cap frozen and deferred it as cosmetic.Measured with the cap set to 3600 in config.yaml:
TERMINAL_SCHEMAis built at import;_ensure_terminal_env_bridgedruns on first tool use — after import.Why it actually matters
The description is the only thing the model reads when choosing a timeout. A schema that understates the cap silently holds the model to the old limit and makes the config key look inert even though enforcement honours it — the user raises the cap and nothing appears to change.
Drift the other way is worse: an overstated cap invites a timeout that then gets rejected.
Fix
_timeout_param_description(cap)— one builder, used by both the schema literal and the refresh, so the two copies of the text can't drift._refresh_terminal_schema_limits()— called from the bridge'sfinally, so even a partial or failed bridge leaves schema and enforcement agreeing.registry.get_entry("terminal").schema is TERMINAL_SCHEMArather than trusting it.Also: a pre-existing hermeticity bug
test_default_value_is_600read the liveFOREGROUND_MAX_TIMEOUT, so it failed on unmodifiedfork/mainfor anyone who had set the config key — the bridge exports the env var process-wide and it leaks into the test run.Verified failing on a clean
--detach fork/mainworktree before touching it, so this is a real inherited bug, not something my diff caused. It now pops the variable and reloads to assert the actual default.Tests — 6, RED-proven
Reverting to the frozen description turns 3 red. Covers a raised cap, a lowered cap, refresh-through-the-registry, the bridge end-to-end, and the
finallypath when the bridge raises.E2E — the exact reproduction:
tests/tools -k terminal: 195 passed, 5 skipped, 0 failed.