From 3ad75a6d59adf6d01d242ab7f8125910389e466c Mon Sep 17 00:00:00 2001 From: Behruz Nassre Esfahani <20915308+behruznassre@users.noreply.github.com> Date: Tue, 8 Sep 2026 21:31:14 -0700 Subject: [PATCH] enhance(#4285): resolve context-monitor fire-points from .planning/config.json (#4366) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * enhance(#4285): resolve context-monitor fire-points from .planning/config.json The monitor's WARNING (35%) and CRITICAL (25%) fire-points were module constants, so the only way to tune them was editing gsd-context-monitor.js — a file in the MANAGED hooks registry, whose body the next install re-stages, silently discarding the edit. The alternative was turning the safety net off. Both are now readable from the config block the hook already opens: hooks.context_warning_threshold and hooks.context_critical_threshold. Absent keys resolve to today's 35/25, so every existing project is byte-identical. Resolution is total and never throws — this hook must not block the tool call it rides in on. A value is usable only if Number.isFinite (type-strict, so the string "30" and true are rejected) and inside the 0-100 domain of the remaining_percentage it is compared against; anything else falls back to the default. The PAIR falls back together: critical >= warning has no coherent reading, and honouring one side silently picks which of the operator's two numbers to discard. That also covers a single override contradicting the other key's default. config-set validates the domain per key so accept and honour agree, but deliberately does not enforce the pair — it writes one key per call, so a two-step retune is transiently inconsistent on disk and refusing it there would block a legitimate configuration. Registration follows the statusline.show_git precedent: schema manifest plus src/config.cts validation, not config-defaults.manifest.json and not buildNewProjectConfig — emitting 35/25 into every new project would pin the defaults at creation time for a setting nobody has tuned. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DsUAawHKUy9pCpnye1Jzd2 * enhance(#4285): address Codex review — per-key fallback docs, discriminating tests Codex full-PR review (gpt-6-astra, read-only) returned five findings. Each was verified against source before acting; all five are real. 1. docs/CONFIGURATION.md described the wrong fallback. An out-of-domain value falls back PER KEY; both defaults apply only when the RESOLVED pair violates critical < warning. warning 150 with critical 30 resolves to 35/30, not 35/25 — at remaining 28 that difference changes the severity emitted. The table now states the two rules in the order they compose, and docs/context-monitor.md gains the same worked example. 2. The inconsistent-pair test could not prove the CRITICAL side reverts: its pair was 20/25, and 25 is already the default, so an implementation that reset only `warning` passed it. A 45/50 pair — both halves away from their defaults — now pins each side with its own reading, and an equal 45/45 pair pins that the rule is strict (`<`, not `<=`). 3. The rejection table's rows could not tell rejection from acceptance: an accepted -5 pairs with the default critical 25, trips the pair check, and produces the same silence. Two rows now separate those: a below-domain critical must escalate remaining 20 to CRITICAL (proving -5 was rejected, not honoured), and an unusable critical beside a usable warning 45 must still fire WARNING at remaining 40 (proving per-key fallback rather than reset-both). The over-claiming comments are narrowed to what each row actually shows. 4. Scope, reproduced rather than assumed: config-set writes through planningDir(), so under GSD_WORKSTREAM it lands in .planning/workstreams//config.json while this hook reads only /.planning/config.json. That is the pre-existing root-only scope hooks.context_warnings has always had, but this PR advertises the setter route, so both docs now say the keys are root-project settings. 5. Four other English docs still stated 35/25 as fixed: the REQ-CTX-02/03 requirements fragment, ARCHITECTURE.md's hook table and threshold table, and INVENTORY.md's hook row. All now name them as defaults and point at the config keys; docs/FEATURES.md is regenerated from its fragment via scripts/gen-features.cjs --write, not hand-edited. Four new mutations, each reverted after: resetting only the warning half on an inconsistent pair (1 red), resetting both on any unusable key (1), dropping the >= 0 bound (1), and accepting critical == warning (1). perf-317 is 116/0. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DsUAawHKUy9pCpnye1Jzd2 * enhance(#4285): tighten claims after Codex round 2 — scoped paths, one more discriminator Confirmation round found no runtime defect and confirmed the five round-1 fixes landed. Four precision items, all real, all fixed here. 1. The scoped-write note named the wrong path for GSD_PROJECT. planningDir() composes three distinct shapes, confirmed by running config-set under each: .planning//config.json, .planning/workstreams//config.json, and .planning//workstreams//config.json. docs/context-monitor.md now tabulates all four cases instead of collapsing them into one. 2. The 45/50 silence row asserted empty stdout without pinning the exit code. runMonitorRaw turns a spawn failure, a non-zero exit or a timeout into empty stdout as well, so the row could have passed on a dead child. It asserts exitCode === 0 first now, like the equal-pair row already did. 3. The sibling row's message claimed it proved critical fell back to 25. It does not: coercing '30' to 30 yields WARNING at remaining 40 too, so the row pins the WARNING side surviving and nothing more. Message narrowed, and a new row reads the same config at remaining 28, where the two candidate resolutions diverge — rejected gives (45, 25) and WARNING, coerced gives (45, 30) and CRITICAL. Mutation-verified: swapping Number.isFinite for the coercing global reds it. 4. "Accept and honour must agree" was too absolute in the src/config.cts and tests/config.test.cjs comments. The agreement holds on the DOMAIN and per key: an accepted value can still lose to the hook's pair check at read time, and a scoped write never reaches the hook at all. Likewise a two-step retune only CAN be transiently inconsistent — 35/25 to 20/10 is valid throughout if critical moves first — so the docs now say what a setter-side pair check would actually cost: rejecting that intermediate write and forcing an order. The same over-absolute phrasing is in b7d179c89's message, which is left as written rather than rewriting history; this commit and the PR body carry the precise claim. perf-317 117/0, config 192/0, config-field-docs 47/0, features-index-gate 84/0, lint:ci clean cold. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DsUAawHKUy9pCpnye1Jzd2 * chore(#4285): add changeset Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DsUAawHKUy9pCpnye1Jzd2 * enhance(#4285): address review — planning-config rows, resolveThresholds properties Two Minor findings from the maintainer review, no behaviour change. Minor 1: gsd-core/references/planning-config.md's "Hook Fields" table gains rows for hooks.context_warning_threshold and hooks.context_critical_threshold, in that table's 5-column form, carrying the same per-key-fallback, pair-reversion and root-config-scope claims docs/CONFIGURATION.md already makes. hooks.workflow_guard's absence from that table is pre-existing and out of scope here. Minor 2: resolveThresholds() gets fast-check property coverage, which ADR 456 requires of a threshold/limit contract. Reaching it needed a require-time seam: the resolver was previously observable only by spawning the hook, and a subprocess per case cannot drive 200 runs — the same conclusion CONTEXT-INDEX records for the ROADMAP Requirements parser. The stdin adapter therefore moves into main() behind `require.main === module`, mirroring gsd-cursor-subagent-start.js and gsd-statusline.js, and module.exports exposes the resolver plus both default constants so a test asserts fallback against the source of truth rather than a second copy of 35/25. Spawned behaviour is unchanged: the 10s stdin timeout still arms per invocation (stdinTimeout is now a module-scope let assigned in main(), still cleared by the end handler), and the try/catch crash(ON_CRASH) path is untouched. Seven properties: totality, ordering, exactness, togetherness, non-vacuity, per-key fallback, non-object argument. Exactness is stated PER KEY — a mixed result (one key honoured, one fallen back) is legal and is the documented contract; the property falsified a per-pair phrasing of it in 4 runs. Verified: cold lint:ci 0; perf-317 file 125/0; seven mutations killed and restored, one of which (upper bound widened to 120) is invisible to the 17 hand-written cases and caught only by a property. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UZw5UhR474YLyE4knjHrte * enhance(#4285): close the Codex-found gap in the property coverage Codex whole-PR review of round 3 returned no Blocker and no Major. Two items, both in the tests added this round, both verified against source before acting. Minor — the per-key fallback property was asymmetric: it required a usable warning to survive an unusable critical, but never the reverse. A resolver that reverted BOTH keys the moment warning was unusable passed all seven properties. Reproduced exactly: that mutant answers 35/25 for {warning: 150, critical: 30} where the resolver answers 35/30, and the file stayed green at 125/0. The mirrored property closes it — with the mutant re-applied it is now the single failing row, and it is the only row that fails, so it is load-bearing rather than incidental. Nit — the ordering property's comment credited it with catching a half-honoured pair, which it does not: 45/50 "repaired" by resetting only critical yields 45/25, perfectly ordered. That case belongs to togetherness. The same comment claimed the behavioural rows sample an inconsistent pair at exactly one point; stale — they cover 20/25, 45/50 and the 45/45 equality boundary. Both claims corrected in place. Verified: cold lint:ci 0; perf-317 file 126/0; the mutant above killed by the new property alone and the hook restored byte-identical afterwards. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UZw5UhR474YLyE4knjHrte * enhance(#4285): name the installed-monitor prerequisite; close the negative-critical gap Second Codex whole-PR pass, run because the base moved: the author's three "Update branch" merges pulled ~26 upstream commits in, so the previously reviewed diff sat on a base that no longer exists. No Blocker, no Major, two Minor — both verified against source before acting. Minor 1, and only reachable because of what the merge brought in: #2586 (03738824d) landed in that window and stops staging hooks/gsd-context-monitor.js for Codex, since the metrics bridge it reads is written only by hooks/gsd-statusline.js, which Codex never installs (bin/install.js: "gsd-context-monitor.js is deliberately NOT copied for Codex"). These two keys are read by that hook and nothing else, so on such a runtime config-set stores and validates them and nothing consumes them — a claim the docs this PR adds did not make. docs/context-monitor.md now carries the explanation and both key tables carry a clause pointing at it; the FEATURES and INVENTORY entries already link through to those two files, so they are not edited again. The changeset says it too, because it is user-facing. Accepting the keys on every runtime is kept deliberately: config is shared across runtimes, so validation stays runtime-independent and the runtime caveat lives in documentation rather than in the setter. Minor 2: the per-key fallback property's junk generator had no negative arm, though its mirror did — and that asymmetry hid a gap. A resolver reverting BOTH keys whenever critical is negative answers 35/25 for {45, -5} where the resolver answers 45/25, and it passed all 126 tests. With the negative arm added it is the single failing row. Verified: cold lint:ci 0; perf-317 126/0; both mutants above killed and the hook restored byte-identical; 538/0 across the config, changeset, doc-parity and emitted-attribution gates. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UZw5UhR474YLyE4knjHrte * enhance(#4285): refuse the two dead threshold endpoints; resolve absent keys Maintainer review round 2 raised two Minors and a nit. Minor 1 — `hooks.context_warning_threshold: 0` was accepted and stored but can never take effect: `critical < warning` must hold and both sides are clamped to 0-100, so nothing can sit below a warning of 0. Verifying it surfaced the MIRROR case the review did not name: `critical: 100` is equally dead, since nothing can sit above it. Both confirmed against the real resolver for partners {absent, 0, 50, 100}, with 0.001 and 99.999 honoured as controls. `config-set` now refuses both, because storing a value the reader always discards is the accept-then-discard shape this codebase refuses elsewhere. The hook is unchanged and still total — it degrades to defaults rather than throwing, so a project that already carries one of these on disk still loads. The old "accepts the domain bounds 0 and 100" row asserted the misleading half and is replaced by tables that make the asymmetry the point (0 is legal for critical and illegal for warning; 100 is the reverse), plus a control row so "refuse both endpoints outright" would not pass in its place. Minor 2 — the keys are absent from config-defaults.manifest.json / buildNewProjectConfig where the sibling `hooks.context_warnings` lives. Kept that way: buildNewProjectConfig writes a hooks object into every NEW project's config.json, which would freeze today's fire-points as an explicit per-project override everywhere — the opposite of this PR's premise. But the underlying complaint was real, so the actual symptom is fixed: `config-get` on an absent key returned "Key not found" while the hook silently used 35/25. It now resolves through SCHEMA_DEFAULTS. Restated rather than derived because CONFIG_DEFAULTS is re-exported flattened and has no `hooks` member at runtime; the one resulting copy of 35/25 outside the hook is pinned against the hook's exported constants by a drift test (red-checked: moving the literal to 40 reds it). Nit — PR-body counts unverifiable from the diff. Noted, no code change. Codex round 3 then found a broken doc link (`context-monitor.md` resolved inside gsd-core/references/, where it does not exist; the emitted tree's own convention is `../../docs/...`) and a stale comment still describing the manifest-derived approach I had backed out. Both fixed. It also corrected my rationale on a point of fact: manifest entries alone would NOT have reached new project configs, since buildNewProjectConfig builds its own literal — the freezing argument applies to that function, not to the manifest. The comment now says so rather than running the two together. Verified: cold lint:ci 0; full suite 36,082 / 0 fail before these two fixes, config + perf-317 321/0 after; drift pin red-checked. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UZw5UhR474YLyE4knjHrte --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: Tom Boucher --- .changeset/curious-tigers-tumble.md | 5 + docs/ARCHITECTURE.md | 7 +- docs/CONFIGURATION.md | 2 + docs/FEATURES.md | 4 +- docs/INVENTORY.md | 2 +- docs/context-monitor.md | 76 +++ docs/features/context-window-monitoring.md | 4 +- .../bin/shared/config-schema.manifest.json | 2 + gsd-core/references/planning-config.md | 2 + hooks/gsd-context-monitor.js | 103 +++- src/config.cts | 64 ++ tests/config.test.cjs | 124 ++++ tests/perf-317-context-monitor-fs.test.cjs | 546 +++++++++++++++++- 13 files changed, 919 insertions(+), 22 deletions(-) create mode 100644 .changeset/curious-tigers-tumble.md diff --git a/.changeset/curious-tigers-tumble.md b/.changeset/curious-tigers-tumble.md new file mode 100644 index 000000000..a5b861193 --- /dev/null +++ b/.changeset/curious-tigers-tumble.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 4366 +--- +**Context-monitor WARNING/CRITICAL fire-points are now readable from `.planning/config.json`** — `hooks.context_warning_threshold` (default 35) and `hooks.context_critical_threshold` (default 25) move the two rungs per project, so a tuned fire-point survives an update instead of being re-staged away with the managed hook file. Absent keys resolve to today's 35/25, so existing projects are unchanged. An unusable value falls back per key; both revert to their defaults only when the resolved pair violates `critical < warning`. The keys are root-project settings — the hook reads `/.planning/config.json` only, and they are read by that hook and nothing else, so on a runtime where it is not installed (Codex, per #2586) both keys are stored and validated but inert. `config-set` refuses the two endpoints that can never take effect — a warning of 0 and a critical of 100 — because `critical < warning` has no legal partner for either, and an absent key now reports the shipped default (35/25) instead of "Key not found". (#4285) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 2a8a272f9..085fdd0b8 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -287,7 +287,7 @@ Runtime hooks that integrate with the host AI agent: | Hook | Event | Purpose | |------|-------|---------| | `gsd-statusline.js` | `statusLine` | Displays model (long-context suffixes like `(1M context)` collapse to a compact `(1M)` badge), task, directory, and context usage bar | -| `gsd-context-monitor.js` | `PostToolUse` / `AfterTool` | Injects agent-facing context warnings at 35%/25% remaining | +| `gsd-context-monitor.js` | `PostToolUse` / `AfterTool` | Injects agent-facing context warnings at 35%/25% remaining by default (configurable — see [CONFIGURATION.md](CONFIGURATION.md)) | | `gsd-check-update.js` | `SessionStart` | Foreground trigger for the background update check | | `gsd-ensure-canonical-path.js` | `SessionStart` | For Claude Code plugin installs, symlinks `~/.claude/gsd-core/{bin,contexts,references,templates,workflows}` to the plugin's bundled tree so `@~/.claude/gsd-core/...` includes resolve; runs first in `SessionStart`, no-op in classic installs, self-heals after `claude plugin update` (#997) | | `gsd-check-update-worker.js` | (helper) | Background worker spawned by `gsd-check-update.js`; no direct event registration | @@ -888,6 +888,11 @@ Runtime Engine (Claude Code / Antigravity CLI) | ≤ 35% | WARNING | "Avoid starting new complex work" | | ≤ 25% | CRITICAL | "Context nearly exhausted, inform user" | +The two fire-points are defaults. `hooks.context_warning_threshold` and +`hooks.context_critical_threshold` in `.planning/config.json` move them per +project; see [context-monitor.md](context-monitor.md) for the resolution and +fallback rules. + Debounce: 5 tool uses between repeated warnings. Severity escalation (WARNING→CRITICAL) bypasses debounce. diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index aa9c29bb5..196448d8a 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -803,6 +803,8 @@ for a worked example. | Setting | Type | Default | Description | |---------|------|---------|-------------| | `hooks.context_warnings` | boolean | `true` | Show context window usage warnings via context monitor hook | +| `hooks.context_warning_threshold` | number | `35` | Percent of context window REMAINING at or below which the monitor emits CONTEXT WARNING. Must be greater than 0 and at most 100, and strictly greater than `hooks.context_critical_threshold` — `config-set` refuses 0, which no critical value can pair with. An out-of-domain value falls back **per key**; both keys revert to their defaults only when the RESOLVED pair violates `critical < warning`. Read from the root project config — a workstream-scoped `config-set` does not reach this hook. Inert on a runtime with no context-monitor hook installed, Codex among them (#2586); see [context-monitor.md](context-monitor.md) (#4285) | +| `hooks.context_critical_threshold` | number | `25` | Percent of context window REMAINING at or below which the monitor escalates to CONTEXT CRITICAL. Must be at least 0 and less than 100, and strictly less than `hooks.context_warning_threshold` — `config-set` refuses 100, which no warning value can pair with. Setting only one of the pair is checked against the other's default, so tune both when moving either past the other. Same root-config scope, and the same installed-monitor prerequisite, as the key above (#4285) | | `hooks.workflow_guard` | boolean | `false` | Warn when file edits happen outside GSD workflow context (advises using `/gsd-quick` or `/gsd-fast`). When enabled, the hook's one hard block — `git add -f` on `agent-*`/`worktree-agent-*` branches — also fails closed on internal error (see `docs/explanation/security-model.md`, #3504) | | `statusline.show_last_command` | boolean | `false` | Append `last: /` suffix to the statusline showing the most recently invoked slash command. Opt-in; reads the active session transcript to extract the latest `` tag (closes #2538) | | `statusline.context_position` | string | `"end"` | Position of the context-window meter. `"end"` (default) renders at line tail; `"front"` renders immediately after the model name so the meter stays visible in narrow terminals. Closes #2937 | diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 8d60b8b91..9df3b2723 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -809,8 +809,8 @@ phase of the same epic. **Requirements:** - REQ-CTX-01: Statusline MUST display context usage percentage to user -- REQ-CTX-02: Context monitor MUST inject agent-facing warnings at ≤35% remaining (WARNING) -- REQ-CTX-03: Context monitor MUST inject agent-facing warnings at ≤25% remaining (CRITICAL) +- REQ-CTX-02: Context monitor MUST inject agent-facing warnings at the WARNING fire-point — ≤35% remaining by default, overridable per project via `hooks.context_warning_threshold` +- REQ-CTX-03: Context monitor MUST inject agent-facing warnings at the CRITICAL fire-point — ≤25% remaining by default, overridable per project via `hooks.context_critical_threshold` - REQ-CTX-04: Warnings MUST debounce (5 tool uses between repeated warnings) - REQ-CTX-05: Severity escalation (WARNING→CRITICAL) MUST bypass debounce - REQ-CTX-06: Context monitor MUST differentiate GSD-active vs non-GSD-active projects diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 12420ec0f..d25ba1717 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -708,7 +708,7 @@ Full listing: `hooks/`. | Hook | Event | Purpose | |------|-------|---------| | `gsd-statusline.js` | `statusLine` | Displays model, task, directory, context usage | -| `gsd-context-monitor.js` | `PostToolUse` / `AfterTool` | Injects agent-facing context warnings at 35%/25% remaining | +| `gsd-context-monitor.js` | `PostToolUse` / `AfterTool` | Injects agent-facing context warnings at 35%/25% remaining by default (configurable — see [CONFIGURATION.md](CONFIGURATION.md)) | | `gsd-check-update.js` | `SessionStart` | Background check for new GSD versions | | `gsd-check-update-worker.js` | (worker) | Background worker helper for check-update | | `gsd-update-banner.js` | `SessionStart` | Opt-in banner surfacing update availability when GSD statusline isn't used (PR #2795) | diff --git a/docs/context-monitor.md b/docs/context-monitor.md index a63cfec73..64e0a6939 100644 --- a/docs/context-monitor.md +++ b/docs/context-monitor.md @@ -28,6 +28,82 @@ breadcrumb bookkeeping. | WARNING | <= 35% | Wrap up current task, avoid starting new complex work | | CRITICAL | <= 25% | Stop immediately, save state (`/gsd-pause-work`) | +### Tuning the fire-points + +35 and 25 are defaults, not fixed points. How much runway "35% remaining" buys +depends on the window size and on what the phase is doing, so both are +overridable per project in `.planning/config.json` (#4285): + +```jsonc +{ + "hooks": { + "context_warnings": true, + "context_warning_threshold": 45, + "context_critical_threshold": 30 + } +} +``` + +Editing the constants in `gsd-context-monitor.js` instead does not survive: the +file is in the managed-hooks registry, so the next install re-stages the +vendored body and the edit is gone without a conflict or a warning. A config key +survives by construction. + +Both keys are optional and both are percentages of context window **remaining**, +so a *larger* number fires *earlier*. Absent keys resolve to the defaults above, +which is what every existing project gets. + +**Both keys require an installed monitor.** They are read by +`hooks/gsd-context-monitor.js` and by nothing else, so on a runtime where that +hook is not installed they are inert — `config-set` still stores them and still +validates them, but no hook consumes them. Codex is that case today: the hook is +deliberately not staged for it, because the metrics bridge it reads is written +only by `hooks/gsd-statusline.js`, which Codex never installs (#2586). Storing a +value is therefore not evidence that a fire-point moved; check that the monitor +is installed for the runtime first. + +The hook never blocks a tool call, so it never throws on a bad value — it +degrades: + +| config | resolved | +|---|---| +| key absent | the default (35 / 25) | +| not a number, or outside 0-100 | the default for that key | +| `critical >= warning` after resolution | **both** defaults — an inconsistent pair has no coherent reading, and honouring one side would silently discard the other | +| `warning` 0, or `critical` 100 | **both** defaults, always. These are in range but have no legal partner — nothing is below 0 and nothing is above 100 — so `critical < warning` can never hold. `config-set` refuses them at write time rather than storing a value the hook will always discard | + +Note the two rows are different rules and compose in this order: an unusable +value is replaced by ITS OWN default first, and only the resulting pair is +checked. So `context_warning_threshold: 150` with `context_critical_threshold: +30` resolves to 35 / 30 — not 35 / 25 — because 30 is usable and 30 < 35 holds. + +The pair check compares resolved values, so overriding only one key is still +checked against the other's default: `context_warning_threshold: 20` on its own +is inconsistent with the default critical of 25 and resolves back to 35 / 25. +Move both when either crosses the other. `gsd-tools config-set` validates the +0-100 domain per key but deliberately does not enforce the pair, because it +writes one key per call and a two-step retune *can* be transiently inconsistent +on disk — 35 / 25 to 20 / 10 is valid throughout if critical goes first, and +inconsistent in between if warning does. A pair check in the setter would reject +that intermediate write and force one particular order. + +### Scope: the root project config only + +The monitor reads `/.planning/config.json` and nothing else. It does not +consult a sub-project or workstream config, but `gsd-tools config-set` does +write to one when the environment selects it — so a scoped write succeeds and +the monitor keeps using the root value, or the default when the root has none: + +| environment | `config-set` writes to | +|---|---| +| neither variable | `.planning/config.json` — the file the monitor reads | +| `GSD_PROJECT=p` | `.planning/p/config.json` | +| `GSD_WORKSTREAM=w` | `.planning/workstreams/w/config.json` | +| both | `.planning/p/workstreams/w/config.json` | + +That is the same root-only scope `hooks.context_warnings` has always had; tune +these keys in the root `.planning/config.json`. + ## Debounce To avoid spamming the agent with repeated warnings: diff --git a/docs/features/context-window-monitoring.md b/docs/features/context-window-monitoring.md index 9726929bb..5a54d93db 100644 --- a/docs/features/context-window-monitoring.md +++ b/docs/features/context-window-monitoring.md @@ -8,8 +8,8 @@ group: Context Engineering Features **Requirements:** - REQ-CTX-01: Statusline MUST display context usage percentage to user -- REQ-CTX-02: Context monitor MUST inject agent-facing warnings at ≤35% remaining (WARNING) -- REQ-CTX-03: Context monitor MUST inject agent-facing warnings at ≤25% remaining (CRITICAL) +- REQ-CTX-02: Context monitor MUST inject agent-facing warnings at the WARNING fire-point — ≤35% remaining by default, overridable per project via `hooks.context_warning_threshold` +- REQ-CTX-03: Context monitor MUST inject agent-facing warnings at the CRITICAL fire-point — ≤25% remaining by default, overridable per project via `hooks.context_critical_threshold` - REQ-CTX-04: Warnings MUST debounce (5 tool uses between repeated warnings) - REQ-CTX-05: Severity escalation (WARNING→CRITICAL) MUST bypass debounce - REQ-CTX-06: Context monitor MUST differentiate GSD-active vs non-GSD-active projects diff --git a/gsd-core/bin/shared/config-schema.manifest.json b/gsd-core/bin/shared/config-schema.manifest.json index ad08ba1e7..8be668320 100644 --- a/gsd-core/bin/shared/config-schema.manifest.json +++ b/gsd-core/bin/shared/config-schema.manifest.json @@ -76,6 +76,8 @@ "planner.stall_threshold_minutes", "workflow.inline_plan_threshold", "hooks.context_warnings", + "hooks.context_warning_threshold", + "hooks.context_critical_threshold", "hooks.workflow_guard", "hooks.commit_types", "hooks.community", diff --git a/gsd-core/references/planning-config.md b/gsd-core/references/planning-config.md index c0bd80aa2..66fe6be81 100644 --- a/gsd-core/references/planning-config.md +++ b/gsd-core/references/planning-config.md @@ -361,6 +361,8 @@ Set via `hooks.*` namespace (e.g., `"hooks": { "context_warnings": true }`). | Key | Type | Default | Allowed Values | Description | |-----|------|---------|----------------|-------------| | `hooks.context_warnings` | boolean | `true` | `true`, `false` | Show warnings when context budget is exceeded | +| `hooks.context_warning_threshold` | number | `35` | Greater than 0 and at most 100, and strictly greater than `hooks.context_critical_threshold`. `config-set` refuses 0: nothing is below it, so no critical value could satisfy the pair | Percent of context window REMAINING at or below which the monitor emits CONTEXT WARNING. An out-of-domain value falls back **per key**; both keys revert to their defaults only when the RESOLVED pair violates `critical < warning`. Read from the root project config — a workstream-scoped `config-set` does not reach this hook. Inert on a runtime with no context-monitor hook installed, Codex among them (#2586); see [context-monitor.md](../../docs/context-monitor.md) (#4285) | +| `hooks.context_critical_threshold` | number | `25` | At least 0 and less than 100, and strictly less than `hooks.context_warning_threshold`. `config-set` refuses 100: nothing is above it, so no warning value could satisfy the pair | Percent of context window REMAINING at or below which the monitor escalates to CONTEXT CRITICAL. Setting only one of the pair is checked against the other's default, so tune both when moving either past the other. Same root-config scope, and the same installed-monitor prerequisite, as the key above (#4285) | ### Learnings Fields diff --git a/hooks/gsd-context-monitor.js b/hooks/gsd-context-monitor.js index 6d77f247e..4263eb438 100644 --- a/hooks/gsd-context-monitor.js +++ b/hooks/gsd-context-monitor.js @@ -14,6 +14,9 @@ // Thresholds: // WARNING (remaining <= 35%): Agent should wrap up current task // CRITICAL (remaining <= 25%): Agent should stop immediately and save state +// Both fire-points are overridable per project via .planning/config.json +// (hooks.context_warning_threshold / hooks.context_critical_threshold, #4285); +// the values above are the defaults used when the keys are absent or unusable. // // Debounce: 5 tool uses between warnings to avoid spam // Severity escalation bypasses debounce (WARNING -> CRITICAL fires immediately) @@ -30,8 +33,8 @@ const { HOOK_ON_CRASH, allow, crash } = require('./lib/hook-exit.js'); // context warning is far cheaper than stalling the agent's work (#3911). const ON_CRASH = HOOK_ON_CRASH.ALLOW; -const WARNING_THRESHOLD = 35; // remaining_percentage <= 35% -const CRITICAL_THRESHOLD = 25; // remaining_percentage <= 25% +const WARNING_THRESHOLD = 35; // remaining_percentage <= 35% (default, see resolveThresholds) +const CRITICAL_THRESHOLD = 25; // remaining_percentage <= 25% (default, see resolveThresholds) const STALE_SECONDS = 60; // ignore metrics older than 60s const DEBOUNCE_CALLS = 5; // min tool uses between warnings // How long after a PreCompact readings stay suspect. The watermark records the @@ -57,6 +60,41 @@ const COMPACT_GRACE_SECONDS = 60; // watermark this far ahead pushes first recovery from +61 to +66 (measured). const WATERMARK_SKEW_SECONDS = 5; +// Resolve the two fire-points from the project's `.planning/config.json` +// (#4285). The constants above are the DEFAULTS; a project overrides either one +// through `hooks.context_warning_threshold` / `hooks.context_critical_threshold`, +// which is what keeps a tuned fire-point alive across updates — this file is in +// the MANAGED registry, so an edit to the constants is re-staged away by the +// next install. +// +// TOTAL and never-throwing: this hook must not block the tool call it rides in +// on, so every unusable input degrades to the default instead of raising. +// Unusable is decided by Number.isFinite, which is type-strict (the string +// "30" and true are both rejected, unlike the global isFinite), plus the 0-100 +// domain of the remaining_percentage these are compared against. +// +// The PAIR is validated too, and falls back TOGETHER. `critical >= warning` has +// no coherent reading — critical fires deeper into the window than warning — +// and honouring one side of an inconsistent pair silently picks which of the +// operator's two numbers to discard. This also rejects a single override that +// contradicts the OTHER key's default (warning 20 with critical absent, i.e. +// 25); the resulting pair is the same nonsense either way. Set-time validation +// cannot stand in for this check: `config-set` writes one key per call, so +// tuning both (warning first, then critical) is transiently inconsistent on +// disk, and refusing it there would block a legitimate configuration. +function resolveThresholds(hooks) { + const defaults = { warning: WARNING_THRESHOLD, critical: CRITICAL_THRESHOLD }; + if (!hooks || typeof hooks !== 'object') return defaults; + + const usable = (value, fallback) => + (Number.isFinite(value) && value >= 0 && value <= 100) ? value : fallback; + + const warning = usable(hooks.context_warning_threshold, WARNING_THRESHOLD); + const critical = usable(hooks.context_critical_threshold, CRITICAL_THRESHOLD); + + return critical < warning ? { warning, critical } : defaults; +} + // One DEFINITION of what counts as a lifecycle event name, shared by the #3709 // PreCompact reset and the #2289 output-envelope allowlist. Two call sites, one // rule — so the two cannot drift into disagreeing about what "no event name" is. @@ -164,14 +202,11 @@ function writeSentinel(target, payload) { } let input = ''; -// Timeout guard: if stdin doesn't close within 10s (e.g. pipe issues on -// Windows/Git Bash, or slow Claude Code piping during large outputs), -// exit silently instead of hanging until Claude Code kills the process -// and reports "hook error". See #775, #1162. -const stdinTimeout = setTimeout(() => allow(undefined), 10000); -process.stdin.setEncoding('utf8'); -process.stdin.on('data', chunk => input += chunk); -process.stdin.on('end', () => { +// Assigned by main(); the handler below clears it. Declared out here rather +// than inside main() because the handler closes over it. +let stdinTimeout = null; + +const handleStdinEnd = () => { clearTimeout(stdinTimeout); try { const data = JSON.parse(input); @@ -266,18 +301,26 @@ process.stdin.on('end', () => { allow(undefined); } - // Check if context warnings are disabled via config. + // Check if context warnings are disabled via config, and resolve the two + // fire-points from the same read (#4285 — one config read, not two). // Collapsed existsSync+readFileSync into a single read guarded by try/catch // (ENOENT or parse error → use defaults, same as old "planningDir absent" branch). const cwd = data.cwd || process.cwd(); + let thresholds = { warning: WARNING_THRESHOLD, critical: CRITICAL_THRESHOLD }; try { const configPath = path.join(cwd, '.planning', 'config.json'); const config = JSON.parse(fs.readFileSync(configPath, 'utf8')); if (config.hooks?.context_warnings === false) { allow(undefined); } + // After the disable check, not before: a disabled monitor exits above and + // never reaches a threshold, so resolving first would only add work to the + // path that does nothing. allow() exits the process (it does not throw), + // so this line is unreachable when warnings are off. + thresholds = resolveThresholds(config.hooks); } catch (e) { - // Missing or unparseable config → proceed with defaults (context warnings enabled) + // Missing or unparseable config → proceed with defaults (context warnings + // enabled, thresholds at the constants above, which `thresholds` already holds) } // If no metrics file, this is a subagent or fresh session -- exit silently. @@ -352,7 +395,7 @@ process.stdin.on('end', () => { const usedPct = metrics.used_pct; // No warning needed - if (remaining > WARNING_THRESHOLD) { + if (remaining > thresholds.warning) { allow(undefined); } @@ -389,7 +432,7 @@ process.stdin.on('end', () => { warnData.callsSinceWarn = (warnData.callsSinceWarn || 0) + 1; - const isCritical = remaining <= CRITICAL_THRESHOLD; + const isCritical = remaining <= thresholds.critical; const currentLevel = isCritical ? 'critical' : 'warning'; // Emit immediately on first warning, then debounce subsequent ones @@ -491,4 +534,34 @@ process.stdin.on('end', () => { // exit(0) fail-open behavior exactly (#3911). crash(ON_CRASH, undefined); } -}); +}; + +// The stdin adapter is the only side-effecting statement in this file, so it is +// the only thing that must not run on `require()`. Gating it lets a test import +// `resolveThresholds` and drive it directly — the repo's own conclusion for a +// seam like this (CONTEXT-INDEX, on the ROADMAP Requirements parser: a closure +// reachable only by spawning the CLI is one "no fast-check property can do"). +// A spawn-per-case property test is not the same test: it would exercise the +// resolver at whatever pairs survive to an observable severity, not over its +// whole numeric domain. +/* istanbul ignore next -- stdin adapter, exercised via spawnSync in tests */ +function main() { + // Timeout guard: if stdin doesn't close within 10s (e.g. pipe issues on + // Windows/Git Bash, or slow Claude Code piping during large outputs), + // exit silently instead of hanging until Claude Code kills the process + // and reports "hook error". See #775, #1162. + stdinTimeout = setTimeout(() => allow(undefined), 10000); + process.stdin.setEncoding('utf8'); + process.stdin.on('data', chunk => input += chunk); + process.stdin.on('end', handleStdinEnd); +} + +if (require.main === module) { + main(); +} + +// Exported for the #4285 property test only. The two constants ride along so a +// test asserts the fallback pair against the SOURCE of truth rather than +// re-hardcoding 35/25 — a test carrying its own copy of the defaults would stay +// green if the constants were edited. +module.exports = { resolveThresholds, WARNING_THRESHOLD, CRITICAL_THRESHOLD }; diff --git a/src/config.cts b/src/config.cts index a1ae98b5e..1b199538d 100644 --- a/src/config.cts +++ b/src/config.cts @@ -121,6 +121,27 @@ const SCHEMA_DEFAULTS: Record = { // effective default existed only as the workflow's shell fallback and the // docs disagreed (settings-advanced said 3). Manifest stays the one owner. 'workflow.inline_plan_threshold': CONFIG_DEFAULTS.inline_plan_threshold, + // #4285 review: an absent threshold resolved to "Key not found" while the + // hook silently used 35/25 — the query surface disagreeing with the reader. + // + // Restated here rather than derived: `CONFIG_DEFAULTS` is re-exported with a + // FLATTENED shape that drops the manifest's nested blocks, so + // `CONFIG_DEFAULTS.hooks` is undefined at runtime and the manifest cannot + // feed these two rows the way `workflow.smart_zone_tokens` above is fed. + // + // Not added to `buildNewProjectConfig` either, and that one is deliberate + // rather than incidental: it writes a `hooks` object into every NEW project's + // config.json, which would freeze today's fire-points as an explicit + // per-project override everywhere — the opposite of this PR's premise that an + // absent key tracks the shipped default. (The manifest alone would NOT have + // that effect; `buildNewProjectConfig` builds its own literal. Correcting an + // earlier version of this comment that ran the two together.) + // + // That leaves ONE copy of 35/25 outside the hook — these two rows — and + // `tests/config.test.cjs` pins them against the hook's exported + // WARNING_THRESHOLD/CRITICAL_THRESHOLD so the copies cannot drift. + 'hooks.context_warning_threshold': 35, + 'hooks.context_critical_threshold': 25, }; /** @@ -968,6 +989,49 @@ function cmdConfigSet(cwd: string, keyPath: string | undefined, value: string | } } + // Context-monitor fire-points (#4285) — a percentage of the context window + // REMAINING, so the domain is 0-100 and the hook compares them against + // `remaining_percentage`. Rejecting an out-of-domain value here keeps accept + // and honour in agreement ON THE DOMAIN: the hook falls back to its default + // for a value outside it, so reporting success would be a lie. That agreement + // is per-key and no wider — a value accepted here can still be superseded at + // read time by the hook's pair check, and a scoped write (GSD_PROJECT / + // GSD_WORKSTREAM) lands in a config the hook does not read at all. The PAIR + // (critical < warning) is deliberately NOT enforced here: config-set writes + // one key per call, so a two-step retune can be transiently inconsistent on + // disk and a check here would reject that intermediate write. + if (kp === 'hooks.context_warning_threshold' || kp === 'hooks.context_critical_threshold') { + if (typeof parsedValue !== 'number' || !Number.isFinite(parsedValue) || parsedValue < 0 || parsedValue > 100) { + error(`Invalid ${kp} '${val}'. Must be a number between 0 and 100 (percent of context window remaining).`); + } + // The two ENDPOINTS that are in range but can never form a valid pair are + // refused here rather than stored (#4285 review). `critical < warning` must + // hold at read time and BOTH sides are clamped to 0-100, so `warning: 0` + // has no legal partner (nothing is below 0) and `critical: 100` has none + // either (nothing above 100). Either one is silently discarded by the hook + // for EVERY value of the other key — verified: both resolve to the 35/25 + // defaults against a present, absent, or extreme partner, while 0.001 and + // 99.999 are honoured. + // + // Storing a value the reader can never honour is exactly the + // accept-then-discard shape this codebase refuses elsewhere, so this fails + // at write time where the operator can see it. The pair itself is still NOT + // checked here — config-set writes one key per call, so a two-step retune + // is legitimately inconsistent on disk in between. + if (kp === 'hooks.context_warning_threshold' && parsedValue === 0) { + error(`Invalid ${kp} '${val}'. 0 is in range but unusable: the monitor requires ` + + `hooks.context_critical_threshold < hooks.context_warning_threshold, and no valid ` + + `critical value is below 0, so a warning of 0 would always fall back to the 35/25 ` + + `defaults. Use a value above 0.`); + } + if (kp === 'hooks.context_critical_threshold' && parsedValue === 100) { + error(`Invalid ${kp} '${val}'. 100 is in range but unusable: the monitor requires ` + + `hooks.context_critical_threshold < hooks.context_warning_threshold, and no valid ` + + `warning value is above 100, so a critical of 100 would always fall back to the ` + + `35/25 defaults. Use a value below 100.`); + } + } + // Fallow scope + profile enum validation (#3424) const VALID_FALLOW_SCOPES = ['phase', 'repo']; if (kp === 'code_quality.fallow.scope') assertEnumValue(parsedValue, val, VALID_FALLOW_SCOPES, 'code_quality.fallow.scope'); diff --git a/tests/config.test.cjs b/tests/config.test.cjs index a89856d6f..6183148ba 100644 --- a/tests/config.test.cjs +++ b/tests/config.test.cjs @@ -3181,3 +3181,127 @@ describe('references/checkpoints.md documents the flag', () => { }); }); } + +describe('config-set hooks.context_warning_threshold / hooks.context_critical_threshold (#4285)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + runGsdTools('config-ensure-section', tmpDir, { HOME: tmpDir, USERPROFILE: tmpDir }); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // Registration: before #4285 both keys were rejected outright as unknown, so + // the only way to tune a fire-point was editing the managed hook file. + test('both keys are accepted by config-set and persisted', () => { + for (const [key, value] of [ + ['hooks.context_warning_threshold', 45], + ['hooks.context_critical_threshold', 30], + ]) { + const result = runGsdTools(`config-set ${key} ${value}`, tmpDir); + assert.ok(result.success, `config-set ${key} failed: ${result.error}`); + const config = readConfig(tmpDir); + assert.strictEqual(config.hooks[key.split('.')[1]], value); + } + }); + + // The domain the hook compares against (remaining_percentage) is 0-100 + // inclusive, but the two ENDPOINTS that can never form a valid pair are + // refused rather than stored (#4285 review). `critical < warning` must hold at + // read time and both sides are clamped to 0-100, so `warning: 0` has no legal + // partner below it and `critical: 100` has none above it. Each is discarded by + // the hook for EVERY value of the other key, so storing one would report a + // tuning that can never take effect. + // + // Stated as a pair of tables rather than one row per case, because the + // asymmetry is the point: 0 is legal for critical and illegal for warning, and + // 100 is the reverse. A single "bounds are inclusive" row (which this replaces) + // asserted the misleading half and was what let the dead value through. + test('refuses the two endpoints that can never form a valid pair', () => { + for (const [key, dead] of [ + ['hooks.context_warning_threshold', 0], + ['hooks.context_critical_threshold', 100], + ]) { + const before = JSON.stringify(readConfig(tmpDir)); + const result = runGsdTools(`config-set ${key} ${dead}`, tmpDir); + assert.ok(!result.success, `config-set ${key} ${dead} must fail — no partner value can satisfy the pair check`); + assert.strictEqual(JSON.stringify(readConfig(tmpDir)), before, + `${key}: a refused write must leave the config byte-identical`); + } + }); + + test('accepts the endpoints that ARE reachable, and the values just inside the dead ones', () => { + // The controls for the row above. Without these, refusing 0 and 100 + // outright would pass it just as well as refusing only the dead pairing. + for (const [key, live] of [ + ['hooks.context_warning_threshold', 100], // warning 100 pairs with the default critical 25 + ['hooks.context_critical_threshold', 0], // critical 0 pairs with the default warning 35 + ['hooks.context_warning_threshold', 0.001], // just inside the dead endpoint + ['hooks.context_critical_threshold', 99.999], + ]) { + const result = runGsdTools(`config-set ${key} ${live}`, tmpDir); + assert.ok(result.success, `config-set ${key} ${live} must succeed: ${result.error}`); + assert.strictEqual(readConfig(tmpDir).hooks[key.split('.')[1]], live); + } + }); + + // The query surface must agree with the reader: an ABSENT key resolves to the + // hook's own default rather than "Key not found" (#4285 review). SCHEMA_DEFAULTS + // necessarily restates 35/25 outside the hook — the hook is a standalone + // subprocess on a hot path and cannot read a manifest to learn its own + // defaults — so this row pins the two copies against each other. If either + // moves alone, this goes red. + test('an absent threshold resolves to the hook\'s own constant, not "Key not found"', () => { + const monitor = require('../hooks/gsd-context-monitor.js'); + for (const [key, constant] of [ + ['hooks.context_warning_threshold', monitor.WARNING_THRESHOLD], + ['hooks.context_critical_threshold', monitor.CRITICAL_THRESHOLD], + ]) { + const result = runGsdTools(`config-get ${key}`, tmpDir); + assert.ok(result.success, `config-get ${key} failed: ${result.error}`); + assert.strictEqual(Number(String(result.output).trim()), constant, + `${key} must resolve to the hook's own default (${constant}); a drift here means ` + + 'SCHEMA_DEFAULTS and the hook constants disagree'); + } + }); + + // Accept and honour must agree ON THE DOMAIN: the hook falls back to its + // default for a value outside 0-100, so config-set reporting success on such + // a value would tell the operator a tuning took effect when it did not. The + // agreement is per-key and no wider — an accepted value can still lose to the + // hook's pair check at read time, and a scoped write (GSD_PROJECT / + // GSD_WORKSTREAM) lands in a config the hook does not read at all. + test('rejects values the hook would discard, and leaves the config untouched', () => { + const before = JSON.stringify(readConfig(tmpDir)); + + for (const value of ['101', '-1', 'high', 'true']) { + for (const key of ['hooks.context_warning_threshold', 'hooks.context_critical_threshold']) { + const result = runGsdTools(`config-set ${key} ${value}`, tmpDir); + assert.ok(!result.success, `config-set ${key} ${value} must be rejected, not silently stored`); + } + } + + assert.strictEqual(JSON.stringify(readConfig(tmpDir)), before, + 'a rejected config-set must not have written anything'); + }); + + // Deliberate non-guard, documented in docs/context-monitor.md: config-set + // writes ONE key per call, so a two-step retune (warning first, then + // critical) is transiently inconsistent on disk. Refusing it here would block + // a legitimate configuration; the hook resolves the pair at read time + // instead, falling back to both defaults while it is inconsistent. + test('does NOT enforce the pair ordering across two calls', () => { + const warn = runGsdTools('config-set hooks.context_warning_threshold 20', tmpDir); + assert.ok(warn.success, `config-set must accept a warning threshold below the default critical: ${warn.error}`); + + const crit = runGsdTools('config-set hooks.context_critical_threshold 10', tmpDir); + assert.ok(crit.success, `config-set must accept the second half of the retune: ${crit.error}`); + + const config = readConfig(tmpDir); + assert.strictEqual(config.hooks.context_warning_threshold, 20); + assert.strictEqual(config.hooks.context_critical_threshold, 10); + }); +}); diff --git a/tests/perf-317-context-monitor-fs.test.cjs b/tests/perf-317-context-monitor-fs.test.cjs index 8f9ebd6f0..bc9b50797 100644 --- a/tests/perf-317-context-monitor-fs.test.cjs +++ b/tests/perf-317-context-monitor-fs.test.cjs @@ -14,6 +14,7 @@ * - #2289 — output-envelope allowlist; side effects still run on silent events * - #1974 — one-time critical-session breadcrumb * - #3709 — PreCompact clears the warn sentinel AND the metrics bridge + * - #4285 — WARNING/CRITICAL fire-points resolve from .planning/config.json * Extend this list when folding in the next one. */ @@ -41,19 +42,36 @@ const tmpDir = os.tmpdir(); * @param {number} [opts.usedPct] - used_pct for bridge file * @param {boolean} [opts.writeWarn] - if true, write a warn sentinel before spawn * @param {object} [opts.warnData] - content for warn sentinel (defaults to first-warn-like data) + * @param {object} [opts.planningConfig] - when given, run in a throwaway project dir + * holding this object as .planning/config.json (#4285). Mutually exclusive with + * `cwd`, which the caller no longer chooses; the dir is removed on the way out. * @returns {{ exitCode: number, stdout: string }} */ function runMonitorRaw(opts) { const { sessionId, - cwd = tmpDir, writeMetrics = false, remaining = 20, usedPct = 80, writeWarn = false, warnData = null, + planningConfig = null, } = opts; + // A staged config needs a project dir of its own; without one the caller's + // cwd (tmpDir by default) is used exactly as before. + const stagedCwd = planningConfig === null + ? null + : fs.mkdtempSync(path.join(tmpDir, 'gsd-4285-cfg-')); + if (stagedCwd !== null) { + fs.mkdirSync(path.join(stagedCwd, '.planning'), { recursive: true }); + fs.writeFileSync( + path.join(stagedCwd, '.planning', 'config.json'), + JSON.stringify(planningConfig) + ); + } + const cwd = stagedCwd ?? opts.cwd ?? tmpDir; + const metricsPath = path.join(tmpDir, `claude-ctx-${sessionId}.json`); const warnPath = path.join(tmpDir, `claude-ctx-${sessionId}-warned.json`); @@ -90,6 +108,7 @@ function runMonitorRaw(opts) { } finally { try { fs.unlinkSync(metricsPath); } catch { /* already absent */ } try { fs.unlinkSync(warnPath); } catch { /* already absent */ } + if (stagedCwd !== null) cleanup(stagedCwd); } return { exitCode, stdout }; @@ -2493,3 +2512,528 @@ describe('#3709 round 3: DEBOUNCE_CALLS and STALE_SECONDS at their limits', () = }); }); + +// ─── #4285: WARNING/CRITICAL fire-points resolve from .planning/config.json ─── + +describe('#4285 regression: context-monitor thresholds resolve from .planning/config.json', () => { + // Why these are behavioural spawns rather than unit calls on resolveThresholds: + // the value under test is not the resolver's return, it is WHICH fire-point the + // running hook compares `remaining_percentage` against. A unit test on the + // resolver would pass even if the resolved pair were never threaded to the two + // comparison sites, which is the whole of the change. + const sid = (tag) => `test-4285-${tag}-${Date.now()}-${Math.random().toString(36).slice(2)}`; + + const severityOf = (stdout) => JSON.parse(stdout)?.hookSpecificOutput?.severity; + + test('a raised warning threshold fires where the default is silent', () => { + // remaining 40 is ABOVE the default 35 → the hook is silent by default. + // The control below proves that; without it this test would pass on a hook + // that emits for every reading. + const control = runMonitorRaw({ sessionId: sid('raise-control'), writeMetrics: true, remaining: 40, usedPct: 60 }); + assert.strictEqual(control.stdout, '', + 'control: remaining 40 must be silent under the default 35 threshold — ' + + 'if this emits, the test below proves nothing about the config key'); + + const { exitCode, stdout } = runMonitorRaw({ + sessionId: sid('raise'), + writeMetrics: true, + remaining: 40, + usedPct: 60, + planningConfig: { hooks: { context_warning_threshold: 45 } }, + }); + + assert.strictEqual(exitCode, 0, 'resolving a configured threshold must not fail the hook'); + assert.match(stdout, /CONTEXT WARNING/, + 'a configured warning threshold of 45 must fire at remaining 40; still silent means ' + + 'the hook is comparing against the hardcoded 35'); + assert.strictEqual(severityOf(stdout), 'warning', 'crossing only the warning point is not CRITICAL'); + }); + + test('a raised critical threshold escalates a reading the default calls WARNING', () => { + // remaining 32: default resolves warning (32 <= 35, 32 > 25). With critical + // moved to 35 the same reading is CRITICAL — so this pins the critical key + // specifically, not just "some threshold was read". + const control = runMonitorRaw({ sessionId: sid('crit-control'), writeMetrics: true, remaining: 32, usedPct: 68 }); + assert.strictEqual(severityOf(control.stdout), 'warning', + 'control: remaining 32 is a WARNING under the defaults'); + + const { stdout } = runMonitorRaw({ + sessionId: sid('crit'), + writeMetrics: true, + remaining: 32, + usedPct: 68, + planningConfig: { hooks: { context_warning_threshold: 45, context_critical_threshold: 35 } }, + }); + + assert.strictEqual(severityOf(stdout), 'critical', + 'a configured critical threshold of 35 must escalate remaining 32 to CRITICAL'); + }); + + test('a lowered warning threshold silences a reading the default warns on', () => { + // The opposite direction — proves the key moves the fire-point rather than + // only ever adding warnings. + const control = runMonitorRaw({ sessionId: sid('lower-control'), writeMetrics: true, remaining: 30, usedPct: 70 }); + assert.match(control.stdout, /CONTEXT WARNING/, + 'control: remaining 30 warns under the defaults'); + + const { exitCode, stdout } = runMonitorRaw({ + sessionId: sid('lower'), + writeMetrics: true, + remaining: 30, + usedPct: 70, + planningConfig: { hooks: { context_warning_threshold: 20, context_critical_threshold: 10 } }, + }); + + assert.strictEqual(exitCode, 0, 'hook exits 0 when the configured thresholds silence it'); + assert.strictEqual(stdout, '', + 'with the pair moved to 20/10, remaining 30 is above the warning point and must be silent'); + }); + + test('an unusable value falls back to that key\'s default rather than throwing', () => { + // One row per rejection reason, each run at remaining 40, where the default + // is silent and the honoured value (45) is not. Silence here is NOT a unique + // signature of rejection — Codex review of this PR showed that an accepted + // out-of-domain value can reach the same silence through the pair check + // instead (an accepted -5 pairs with the default critical 25, which is + // >= -5, so both revert and 40 is silent again). So this table proves + // "unusable input never fires early and never throws"; the two rows BELOW + // are what separate per-key fallback from honouring the value. + const rejected = [ + ['string', '45'], + ['above domain', 150], + ['negative', -5], + ['null', null], + ['boolean', true], + ['array', [45]], + ['object', {}], + ]; + + for (const [label, value] of rejected) { + const { exitCode, stdout } = runMonitorRaw({ + sessionId: sid(`bad-${label.replace(/\W+/g, '-')}`), + writeMetrics: true, + remaining: 40, + usedPct: 60, + planningConfig: { hooks: { context_warning_threshold: value } }, + }); + + assert.strictEqual(exitCode, 0, `${label}: an unusable threshold must never fail the hook`); + assert.strictEqual(stdout, '', + `${label}: an unusable threshold must fall back to the default 35, leaving remaining 40 silent`); + } + }); + + test('100 is inside the domain, not rejected as out of range', () => { + // The bound is inclusive on the top. 100 warns at every reading; if the + // range check were `< 100` this would fall back to 35 and go silent at + // remaining 40, which is exactly what the rejection table above asserts for + // a genuinely out-of-domain 150. + const { stdout } = runMonitorRaw({ + sessionId: sid('bound-100'), + writeMetrics: true, + remaining: 40, + usedPct: 60, + planningConfig: { hooks: { context_warning_threshold: 100 } }, + }); + + assert.match(stdout, /CONTEXT WARNING/, + 'a warning threshold of 100 is in-domain and must fire at remaining 40'); + }); + + test('an inconsistent pair falls back to BOTH defaults, not to the usable half', () => { + // warning 20 / critical 25 is inconsistent (critical >= warning). Honouring + // the warning half alone would leave remaining 30 SILENT; falling back to + // both defaults warns. NOTE the limit of this row, raised by Codex review: + // critical 25 IS the default here, so it cannot show that the CRITICAL side + // reverts — an implementation that reset only `warning` would pass it. The + // 45/50 block below is what pins both halves. + const { exitCode, stdout } = runMonitorRaw({ + sessionId: sid('pair'), + writeMetrics: true, + remaining: 30, + usedPct: 70, + planningConfig: { hooks: { context_warning_threshold: 20, context_critical_threshold: 25 } }, + }); + + assert.strictEqual(exitCode, 0, 'an inconsistent pair must never fail the hook'); + assert.match(stdout, /CONTEXT WARNING/, + 'an inconsistent pair resolves to the defaults (35/25), which warn at remaining 30; ' + + 'silence here would mean the warning half was honoured on its own'); + assert.strictEqual(severityOf(stdout), 'warning', + 'the default critical (25) is below remaining 30, so the fallback pair yields WARNING'); + }); + + test('a single override is checked against the OTHER key\'s default', () => { + // Same rule as the row above, reached with one key set instead of two — the + // case the docs call out, because it is the one an operator hits by accident. + const { stdout } = runMonitorRaw({ + sessionId: sid('single'), + writeMetrics: true, + remaining: 30, + usedPct: 70, + planningConfig: { hooks: { context_warning_threshold: 20 } }, + }); + + assert.match(stdout, /CONTEXT WARNING/, + 'warning 20 against the default critical 25 is inconsistent and resolves to 35/25, ' + + 'which warns at remaining 30'); + }); + + test('an invalid key falls back alone — the sibling override survives', () => { + // Codex review: nothing above separated per-key fallback from a + // reset-BOTH implementation. Warning 45 is usable, critical is not. + // Per key -> (45, 25): remaining 40 is <= 45 and > 25, so WARNING. + // Reset both -> (35, 25): remaining 40 is above 35, so SILENCE. + const { stdout } = runMonitorRaw({ + sessionId: sid('sibling'), + writeMetrics: true, + remaining: 40, + usedPct: 60, + planningConfig: { hooks: { context_warning_threshold: 45, context_critical_threshold: '30' } }, + }); + + assert.match(stdout, /CONTEXT WARNING/, + 'an unusable critical must not drag the usable warning override down with it'); + // What this row pins is the WARNING side surviving. It does NOT by itself + // prove critical became 25: coercing '30' to 30 would also yield WARNING at + // remaining 40 (Codex review, round 2). The next row settles that. + assert.strictEqual(severityOf(stdout), 'warning', + 'remaining 40 is above any resolved critical here, so the severity is the warning rung'); + }); + + test('a numeric-looking STRING is rejected, not coerced', () => { + // Same config as the row above, read at 28 — the reading that separates the + // two candidate resolutions: + // rejected -> (45, 25): 28 > 25, so WARNING. + // coerced -> (45, 30): 28 <= 30, so CRITICAL. + const { stdout } = runMonitorRaw({ + sessionId: sid('string-critical'), + writeMetrics: true, + remaining: 28, + usedPct: 72, + planningConfig: { hooks: { context_warning_threshold: 45, context_critical_threshold: '30' } }, + }); + + assert.strictEqual(severityOf(stdout), 'warning', + "'critical' at remaining 28 means the string '30' was coerced into a fire-point; " + + 'Number.isFinite must reject it and leave critical at its default 25'); + }); + + test('a below-domain critical is rejected rather than honoured', () => { + // Codex review: the -5 row in the table above cannot tell rejection from + // acceptance. Here it can. Warning 45 with critical -5: + // rejected -> (45, 25): remaining 20 is <= 25, so CRITICAL. + // honoured -> (45, -5): remaining 20 is above -5, so merely WARNING. + const { stdout } = runMonitorRaw({ + sessionId: sid('neg-critical'), + writeMetrics: true, + remaining: 20, + usedPct: 80, + planningConfig: { hooks: { context_warning_threshold: 45, context_critical_threshold: -5 } }, + }); + + assert.strictEqual(severityOf(stdout), 'critical', + 'a negative critical must fall back to 25 and escalate remaining 20; ' + + "'warning' here means -5 was honoured as a fire-point"); + }); + + describe('an inconsistent pair of TWO configured values reverts both', () => { + // Codex review: the 20/25 row cannot prove the critical side resets, because + // 25 IS the default — an implementation that reset only `warning` would pass + // it. 45/50 is inconsistent with BOTH halves away from their defaults, so + // each reading below fails a different partial implementation. + const pair = { context_warning_threshold: 45, context_critical_threshold: 50 }; + + test('the warning half reverts: remaining 40 is silent', () => { + // Reverted -> warning 35, and 40 > 35 -> silence. + // Warning 45 preserved -> 40 <= 45 -> a warning would fire. + const { exitCode, stdout } = runMonitorRaw({ + sessionId: sid('pair45-40'), writeMetrics: true, remaining: 40, usedPct: 60, + planningConfig: { hooks: { ...pair } }, + }); + // exitCode first: the helper turns a spawn failure, a non-zero exit or a + // timeout into empty stdout too, so asserting silence alone would pass on + // a crashed child (Codex review, round 2). + assert.strictEqual(exitCode, 0, 'the silence below must come from the threshold, not from a dead child'); + assert.strictEqual(stdout, '', + 'output at remaining 40 means the configured warning 45 survived an inconsistent pair'); + }); + + test('the critical half reverts: remaining 32 is WARNING, not CRITICAL', () => { + // Reverted -> critical 25, and 32 > 25 -> severity 'warning'. + // Critical 50 preserved -> 32 <= 50 -> severity 'critical'. + const { stdout } = runMonitorRaw({ + sessionId: sid('pair45-32'), writeMetrics: true, remaining: 32, usedPct: 68, + planningConfig: { hooks: { ...pair } }, + }); + assert.match(stdout, /CONTEXT WARNING/, 'the default warning 35 must fire at remaining 32'); + assert.strictEqual(severityOf(stdout), 'warning', + "'critical' at remaining 32 means the configured critical 50 survived an inconsistent pair"); + }); + }); + + test('an EQUAL pair is inconsistent too — critical must fire strictly deeper', () => { + // The boundary of the pair rule: `critical < warning`, not `<=`. With 45/45 + // honoured, remaining 40 would be <= 45 on BOTH tests and every warning + // would arrive pre-escalated to CRITICAL. Rejected, both revert to 35/25 and + // remaining 40 is above the warning point. + const { exitCode, stdout } = runMonitorRaw({ + sessionId: sid('equal-pair'), + writeMetrics: true, + remaining: 40, + usedPct: 60, + planningConfig: { hooks: { context_warning_threshold: 45, context_critical_threshold: 45 } }, + }); + + assert.strictEqual(exitCode, 0, 'an equal pair must never fail the hook'); + assert.strictEqual(stdout, '', + 'an equal pair must revert to 35/25, leaving remaining 40 silent'); + }); + + test('context_warnings:false still wins over configured thresholds', () => { + // A project that tuned the thresholds and later switched warnings off stays + // silent: the disable exit is unconditional, so no configured fire-point can + // resurrect it. NOT an ordering guard — measured: moving the resolution + // above the disable check leaves this row green, because allow() exits + // either way. The ordering is a cost choice (don't resolve on a path that + // exits), not an observable, so nothing here pins it. + const { exitCode, stdout } = runMonitorRaw({ + sessionId: sid('disabled'), + writeMetrics: true, + remaining: 40, + usedPct: 60, + planningConfig: { + hooks: { + context_warnings: false, + context_warning_threshold: 45, + context_critical_threshold: 30, + }, + }, + }); + + assert.strictEqual(exitCode, 0, 'hook exits 0 when warnings are disabled'); + assert.strictEqual(stdout, '', + 'context_warnings:false must silence the hook even when a threshold would have fired'); + }); +}); + +// ─── #4285: resolveThresholds domain invariants (property-based) ───────────── + +const fc = require('./helpers/fast-check-setup.cjs'); +const { + resolveThresholds, + WARNING_THRESHOLD, + CRITICAL_THRESHOLD, +} = require(MONITOR_PATH); + +describe('#4285 properties: resolveThresholds holds its domain invariants for arbitrary input', () => { + // The spawn-based rows above pin that the RESOLVED pair reaches the two + // comparison sites. They cannot pin the resolver over its numeric domain: + // each case costs a subprocess, and only pairs that change an observable + // severity are visible at all. These properties cover the other half — the + // resolver as a total function — per ADR 456 (threshold/limit contracts get + // fast-check coverage) and the seam rule the repo already applies elsewhere + // (CONTEXT-INDEX, on the ROADMAP Requirements parser: a closure reachable + // only by spawning the CLI is one "no fast-check property can do", so it is + // exported and driven directly instead). No path literal here on purpose — + // the docs-guard lint reads a bare one as this file guarding a shipped doc. + + const DEFAULTS = { warning: WARNING_THRESHOLD, critical: CRITICAL_THRESHOLD }; + + // Deliberately wider than the accepted domain: NaN/±Infinity (fc.double's + // default), out-of-range and negative reals, and non-numbers of every shape. + const anyThreshold = fc.oneof( + { weight: 6, arbitrary: fc.double() }, + { weight: 3, arbitrary: fc.double({ min: -1000, max: 1000, noNaN: true }) }, + { weight: 1, arbitrary: fc.anything() }, + ); + + const isDefaults = (r) => r.warning === DEFAULTS.warning && r.critical === DEFAULTS.critical; + + test('the defaults themselves satisfy the invariant they are the fallback for', () => { + // If this ever goes red the constants have drifted into the state the + // resolver rejects, and every fallback below would return a nonsense pair. + assert.ok(Number.isFinite(WARNING_THRESHOLD) && WARNING_THRESHOLD >= 0 && WARNING_THRESHOLD <= 100); + assert.ok(Number.isFinite(CRITICAL_THRESHOLD) && CRITICAL_THRESHOLD >= 0 && CRITICAL_THRESHOLD <= 100); + assert.ok(CRITICAL_THRESHOLD < WARNING_THRESHOLD); + }); + + test('totality: no input throws, and the pair is always two finite numbers in [0, 100]', () => { + fc.assert( + fc.property(anyThreshold, anyThreshold, (w, c) => { + const r = resolveThresholds({ context_warning_threshold: w, context_critical_threshold: c }); + return ( + Number.isFinite(r.warning) && r.warning >= 0 && r.warning <= 100 && + Number.isFinite(r.critical) && r.critical >= 0 && r.critical <= 100 + ); + }), + ); + }); + + test('ordering: the resolved pair always satisfies critical < warning', () => { + // This property enforces ORDERING only, and nothing more. It is NOT the + // one that catches a half-honoured pair: a resolver that "repaired" 45/50 + // by resetting only critical returns 45/25, which is perfectly ordered and + // sails through here. `togetherness` below is what pins simultaneous + // reversion. (Both claims corrected after Codex review of this round — the + // comment previously credited this property with catching that case, and + // also claimed the behavioural rows sample an inconsistent pair at exactly + // one point, which is stale: they cover 20/25, 45/50 and the 45/45 + // equality boundary.) + fc.assert( + fc.property(anyThreshold, anyThreshold, (w, c) => { + const r = resolveThresholds({ context_warning_threshold: w, context_critical_threshold: c }); + return r.critical < r.warning; + }), + ); + }); + + test('exactness: each side is the configured value or that key\'s default — never a third number', () => { + // Rules out a resolver that "repairs" an out-of-domain or inconsistent + // input by clamping or nudging it: 150 must become 35, not 100. + // + // Stated per key rather than per pair, because a MIXED result is legal and + // is the documented per-key fallback — warning 150 with critical 0 resolves + // to {35, 0}, which is neither "the configured pair" nor "the defaults". + // (Found by this property on its first run, against a per-pair phrasing.) + fc.assert( + fc.property(anyThreshold, anyThreshold, (w, c) => { + const r = resolveThresholds({ context_warning_threshold: w, context_critical_threshold: c }); + return ( + (r.warning === w || r.warning === WARNING_THRESHOLD) && + (r.critical === c || r.critical === CRITICAL_THRESHOLD) + ); + }), + ); + }); + + test('togetherness: an inconsistent RESOLVED pair reverts both sides, not the offending one', () => { + // The complement of the property above: mixing is legal only while the + // resulting pair stays ordered. Once it does not, the result is the exact + // defaults — no half-honoured pair survives. + fc.assert( + fc.property( + fc.double({ min: 0, max: 100, noNaN: true }), + fc.double({ min: 0, max: 100, noNaN: true }), + (a, b) => { + fc.pre(b >= a); // in-domain but inconsistent: critical >= warning + return isDefaults(resolveThresholds({ + context_warning_threshold: a, + context_critical_threshold: b, + })); + }, + ), + ); + }); + + test('non-vacuity: every in-domain consistent pair is honoured verbatim', () => { + // Without this, `resolveThresholds = () => DEFAULTS` passes all three + // properties above. This is the one that makes them mean something. + fc.assert( + fc.property( + fc.double({ min: 0, max: 100, noNaN: true }), + fc.double({ min: 0, max: 100, noNaN: true }), + (a, b) => { + const warning = Math.max(a, b); + const critical = Math.min(a, b); + fc.pre(critical < warning); + const r = resolveThresholds({ + context_warning_threshold: warning, + context_critical_threshold: critical, + }); + return r.warning === warning && r.critical === critical; + }, + ), + ); + }); + + test('per-key fallback: one unusable key does not discard the other usable one', () => { + // Pins the per-key half of the contract the docs promise. warning is left + // at a value that stays consistent with the default critical (25), so a + // resolved pair survives and the honoured half is observable. + fc.assert( + fc.property( + fc.double({ min: 26, max: 100, noNaN: true }), + // The negative arm is not decoration: without it a resolver that + // reverts BOTH keys whenever critical is negative passes every other + // property (verified — it answers 35/25 for {45, -5} where the + // resolver answers 45/25). Found by Codex review of this round; the + // mirrored property below already carried its negative arm, and the + // asymmetry between the two is exactly what hid the gap. + fc.oneof(fc.string(), fc.boolean(), fc.constant(null), fc.constant(undefined), fc.constant(NaN), fc.constant(Infinity), fc.double({ min: 100.001, max: 1e6, noNaN: true }), fc.double({ min: -1e6, max: -0.001, noNaN: true })), + (warning, junkCritical) => { + const r = resolveThresholds({ + context_warning_threshold: warning, + context_critical_threshold: junkCritical, + }); + return r.warning === warning && r.critical === CRITICAL_THRESHOLD; + }, + ), + ); + }); + + test('per-key fallback, mirrored: one unusable WARNING does not discard a usable critical', () => { + // The mirror of the property above, and NOT redundant with it: without + // this, a resolver that reverts BOTH keys the moment warning is unusable + // passes all seven other properties. Verified against exactly that mutant + // — it answers 35/25 for {warning: 150, critical: 30} where the real + // resolver answers 35/30, and the suite stayed green at 125/0 until this + // row existed. (Found by Codex review of this round.) + // + // critical is generated strictly below the DEFAULT warning (35), so the + // resolved pair {35, critical} stays ordered and the honoured half is + // observable rather than swallowed by the pair check. + fc.assert( + fc.property( + fc.double({ min: 0, max: 34.999, noNaN: true }), + fc.oneof(fc.string(), fc.boolean(), fc.constant(null), fc.constant(undefined), fc.constant(NaN), fc.constant(Infinity), fc.double({ min: 100.001, max: 1e6, noNaN: true }), fc.double({ min: -1e6, max: -0.001, noNaN: true })), + (critical, junkWarning) => { + const r = resolveThresholds({ + context_warning_threshold: junkWarning, + context_critical_threshold: critical, + }); + return r.warning === WARNING_THRESHOLD && r.critical === critical; + }, + ), + ); + }); + + test('the two in-range endpoints with no legal partner always revert', () => { + // `warning: 0` and `critical: 100` are inside the 0-100 domain and pass the + // per-key check, but `critical < warning` can never hold for either: nothing + // is below 0 and nothing is above 100. So each is discarded for EVERY value + // of the other key. config-set refuses them at write time (tests/config.test.cjs); + // this row pins the READ side, which must stay total and simply fall back. + // + // The 0.001 / 99.999 controls are what make it a claim about the endpoints + // rather than about small and large numbers generally. + const D = { warning: WARNING_THRESHOLD, critical: CRITICAL_THRESHOLD }; + + for (const partner of [undefined, 0, 50, 100]) { + assert.deepStrictEqual( + resolveThresholds({ context_warning_threshold: 0, context_critical_threshold: partner }), D, + `warning 0 must revert whatever critical is (tried ${partner})`); + assert.deepStrictEqual( + resolveThresholds({ context_critical_threshold: 100, context_warning_threshold: partner }), D, + `critical 100 must revert whatever warning is (tried ${partner})`); + } + + assert.deepStrictEqual( + resolveThresholds({ context_warning_threshold: 0.001, context_critical_threshold: 0 }), + { warning: 0.001, critical: 0 }, + 'control: just inside the dead endpoint still resolves, so the row above is about 0 itself'); + assert.deepStrictEqual( + resolveThresholds({ context_warning_threshold: 100, context_critical_threshold: 99.999 }), + { warning: 100, critical: 99.999 }, + 'control: just inside the other dead endpoint still resolves'); + }); + + test('a non-object hooks argument of any shape yields the defaults', () => { + fc.assert( + fc.property( + fc.oneof(fc.constant(null), fc.constant(undefined), fc.string(), fc.double(), fc.boolean()), + (hooks) => isDefaults(resolveThresholds(hooks)), + ), + ); + }); +});