Repository navigation
fix(terminal): make the foreground-timeout and disk-warning caps config keys - #575
Merged
Merged
Conversation
…ig keys
`TERMINAL_MAX_FOREGROUND_TIMEOUT` and `TERMINAL_DISK_WARNING_GB` were
env-var-ONLY. Both are behavioral limits, and AGENTS.md is explicit that .env
is for secrets while behavioral settings belong in config.yaml. Every other
`terminal.*` knob already follows that rule via TERMINAL_CONFIG_ENV_MAP.
Adds `terminal.max_foreground_timeout` (default 600) and
`terminal.disk_warning_gb` (default 500.0) — same values as the historical env
defaults, so no behavior changes for anyone who does not set them. The env vars
keep working; the map bridges config -> env for child processes exactly as it
does for backend, cwd, timeout, and the rest.
THE NON-OBVIOUS HALF — why the one-line map entry is not enough
`FOREGROUND_MAX_TIMEOUT` is a module constant evaluated at IMPORT time, but the
config->env bridge (`_ensure_terminal_env_bridged`) runs on FIRST TOOL USE —
after import. So adding the key to the map alone produces a knob that parses,
validates, shows up in `hermes config`, and does nothing: enforcement would keep
comparing against the pre-bridge value.
The enforcement path now calls `_foreground_max_timeout()` / `_disk_warning_gb()`,
which re-read the env after the bridge has run. The import-time constants remain
as the fallback default.
`TERMINAL_SCHEMA` is a module constant, so the cap quoted in its `timeout`
description is still the import-time number. That is cosmetic and deliberately
left alone — enforcement is what matters, and it is correct. The docstring says
so, so nobody "fixes" it by reintroducing the frozen read.
TESTS (7, RED-proven)
Reverting the enforcement site to the frozen constant — i.e. the naive
map-entry-only fix — turns the source-contract test RED and names the problem.
That test exists because an arithmetic assertion passes either way in a default
environment, where the constant and the accessor hold the same value.
E2E through the real bridge with a real config.yaml in a temp HERMES_HOME:
before bridge: 600 500.0
after bridge: 2400 12.5
BASELINE
tests/hermes_cli/ + tests/tools/ -k "terminal or config":
branch 16 failed, 1120 passed
fork/main 16 failed, 1113 passed <- identical failure set
Zero new failures; the 16 are pre-existing on main and untouched by this diff.
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.
Two behavioral limits were env-var-only
TERMINAL_MAX_FOREGROUND_TIMEOUTandTERMINAL_DISK_WARNING_GBcould only be set via environment variables. AGENTS.md is explicit:Every other
terminal.*knob already follows that rule throughTERMINAL_CONFIG_ENV_MAP. These two were missed.Adds
terminal.max_foreground_timeout(default600) andterminal.disk_warning_gb(default500.0) — the same values as the historical env defaults, so nothing changes for anyone who doesn't set them. The env vars keep working; the map bridges config → env for child processes.The non-obvious half — why a one-line map entry is NOT enough
FOREGROUND_MAX_TIMEOUTis a module constant evaluated at import time. The config→env bridge (_ensure_terminal_env_bridged) runs on first tool use — after import.So adding the key to the map alone produces a knob that parses, validates, appears in
hermes config… and does nothing. Enforcement would keep comparing against the pre-bridge value.Enforcement now calls
_foreground_max_timeout()/_disk_warning_gb(), which re-read the env after the bridge runs. The import-time constants stay as the fallback.TERMINAL_SCHEMAis also a module constant, so the cap quoted in itstimeoutdescription is still the import-time number. That's cosmetic and deliberately left — enforcement is what matters, and the docstring says so, so nobody "fixes" it by reintroducing the frozen read.Tests — 7, RED-proven
Reverting the enforcement site to the frozen constant (the naive map-entry-only fix) turns the source-contract test RED and names the problem. That test exists because an arithmetic assertion passes either way in a default environment, where the constant and the accessor hold the same value.
E2E through the real bridge, real
config.yamlin a tempHERMES_HOME:Baseline
Zero new failures. The 16 are pre-existing on main and untouched by this diff; the +7 are this PR's tests.