From c87278dee427291bd71714de2c137d1a12adbdfc Mon Sep 17 00:00:00 2001 From: David Meister Date: Tue, 11 Aug 2026 08:15:11 +0000 Subject: [PATCH 1/2] fix(usage-gate): fail closed when usage cannot be read (#273) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unreadable endpoint with no fallback reading now pauses the tick (exit 10, one skip row) instead of running it. The pause reason — 'PAUSE: no usage reading — endpoint unreachable and no fallback reading set — cannot pace blind, failing closed' — shares no wording with the pace or ceiling pauses, so a blind pause is diagnosable from a single log or skip-row line. --force remains the operator's one-tick way past it, like any other POLICY pause. Prose describing the retired inert behavior is updated in the runners, cron.env.example, the gate's doc comments and the integration-test preamble; the unit test flips to pin the Pause verdict and its reason shape. Co-Authored-By: Claude Fable 5 --- campaign-run.sh | 5 +- cron.env.example | 5 +- pr-review-report-rs/src/main.rs | 70 ++++++++++++-------- pr-review-report-rs/tests/usage_gate_skip.rs | 11 +-- review-run.sh | 5 +- 5 files changed, 57 insertions(+), 39 deletions(-) diff --git a/campaign-run.sh b/campaign-run.sh index 635dd256..484daf28 100755 --- a/campaign-run.sh +++ b/campaign-run.sh @@ -193,8 +193,9 @@ fi # --- weekly-budget pace gate: skip this tick when usage is over the ceiling or inside the BAU # headroom band under the linear burn toward the reset — the crons hold ~USAGE_HEADROOM_PCT points # BEHIND pace so interactive work keeps standing budget (#158). `usage-gate` reads -# /api/oauth/usage itself; exit 10 means PAUSE (record one skip row, exit 0). It is INERT when it -# cannot read usage and no fallback is set — it prints OK and we run. Any OTHER non-zero exit is a +# /api/oauth/usage itself; exit 10 means PAUSE (record one skip row, exit 0). It FAILS CLOSED when +# it cannot read usage and no fallback is set (#273) — that is a PAUSE too, its reason naming the +# read failure so the skip row is diagnosable as an endpoint problem. Any OTHER non-zero exit is a # config REFUSAL (the retired USAGE_SLACK_PCT still set: exit 2, reason on stderr, captured into # the log): the tick must not run on config the gate refused to read, so propagate the failure — a # refusal is neither a run nor a pause, and it writes NO row. --- diff --git a/cron.env.example b/cron.env.example index 26a736d1..21e66f7c 100644 --- a/cron.env.example +++ b/cron.env.example @@ -63,7 +63,8 @@ PR_ASSIGNEE="your-github-handle" # one line in the log — the same posture the gate takes toward USAGE_SLACK_PCT above. # # FALLBACK ONLY — used when the endpoint is unreachable, e.g. the token expired or the API moved. -# Not the normal path. With neither the endpoint nor a fallback the gate is INERT and the crons -# RUN: it exists to pace spending, not to become a new way for the pipeline to stall. +# Not the normal path. With neither the endpoint nor a fallback the gate FAILS CLOSED and the +# crons PAUSE (#273): the gate never paces blind, and the pause's reason names the read failure +# so a stalled stretch reads as an endpoint problem from the log. `--force` runs one tick past it. # USAGE_USED_PCT="60" # % of the weekly budget used, per your last /usage reading # USAGE_RESET_AT="2026-07-19T00:00:00Z" # when the weekly budget resets to 0 (ISO-8601 UTC) diff --git a/pr-review-report-rs/src/main.rs b/pr-review-report-rs/src/main.rs index f6235b8d..84e3f94a 100644 --- a/pr-review-report-rs/src/main.rs +++ b/pr-review-report-rs/src/main.rs @@ -10051,12 +10051,14 @@ fn linear_pct(now_ms: i64, reset_ms: i64) -> f64 { /// Order matters: the CEILING is checked before the PACE, and applies whatever the pace — being /// under a linear burn is no help once the weekly budget is nearly spent. /// -/// `None` reading means the endpoint could not be read and no fallback is set. That is INERT — -/// it prints OK and the crons RUN. This is deliberate and must stay: an earlier version of this -/// gate could not read usage and made the operator paste percentages in by hand, which silently -/// paused both crons for 22 consecutive ticks when a reading went stale at 80%. The gate exists -/// to pace spending, not to become a new way for the pipeline to stall — so a malformed response, -/// an expired token, a network failure and an unparseable date all land here, never on Pause. +/// `None` reading means the endpoint could not be read and no fallback is set. That FAILS +/// CLOSED — the tick pauses (#273): a gate that runs only when it cannot check is a policy +/// enforced everywhere except where it is blind, and one endpoint outage would wave through +/// exactly the ticks the pace check exists to hold. A malformed response, an expired token, a +/// network failure and an unparseable date all land here, and all pause. The reason names the +/// read failure and shares no wording with the pace or ceiling pauses, so a blind pause is +/// diagnosable from its single log/skip-row line; `--force` is the operator's way past it, as +/// past any POLICY pause. fn usage_gate_decide( reading: Option<&UsageReading>, now_ms: i64, @@ -10064,8 +10066,10 @@ fn usage_gate_decide( ceiling: f64, ) -> UsageVerdict { let Some(r) = reading else { - return UsageVerdict::Run( - "OK: usage endpoint unreachable and no fallback reading set — gate inert".to_string(), + return UsageVerdict::Pause( + "PAUSE: no usage reading — endpoint unreachable and no fallback reading set — \ + cannot pace blind, failing closed" + .to_string(), ); }; let (used, source) = (r.used, r.source); @@ -10135,7 +10139,7 @@ fn stale_slack_refusal(slack_var: Option<&str>) -> Option { /// Pull the OAuth bearer token out of the credentials file (`.claudeAiOauth.accessToken`). /// Every failure — absent file, bad JSON, missing key, empty value — is `None`, which routes to -/// the fallback-or-inert path rather than an error. +/// the fallback reading or, absent one, the fail-closed pause, rather than an error. fn oauth_token(creds: &str) -> Option { let doc: Value = serde_json::from_str(&std::fs::read_to_string(creds).ok()?).ok()?; let tok = doc.get("claudeAiOauth")?.get("accessToken")?.as_str()?; @@ -10164,10 +10168,10 @@ fn oauth_token(creds: &str) -> Option { /// 84 files) `five_hour` reached `rejected` in 5 events, all inside one run — which still /// produced a `result`. The window self-clears well inside the gap between ticks. The /// seven-day ceiling is what actually stops work, and this gate already holds it. -/// 3. **A second pause condition contradicts this gate's design commitment.** Every failure path -/// here is deliberately INERT (see [`usage_gate_decide`]) because the repo has an incident on -/// record where the gate silently paused both crons for 22 consecutive ticks. Adding a -/// faster-cycling reason to pause is the same hazard, with 4.8 chances a day to fire. +/// 3. **A second pause condition multiplies stalls.** The gate already pauses whenever it +/// cannot read usage (fail-closed, [`usage_gate_decide`]), and the repo has an incident on +/// record where a gate paused both crons for 22 consecutive ticks. A faster-cycling reason +/// to pause is 4.8 more chances a day to stall the pipeline on window noise. /// 4. **The units differ and would fail silently.** `utilization` on a `rate_limit_event` is a /// FRACTION in 0..=1 (`0.91`); [`UsageReading::used`] here is a PERCENT in 0..=100 compared /// against a default ceiling of 90. Wiring them together without converting would compare @@ -10195,8 +10199,8 @@ fn parse_seven_day(raw: &str) -> Option { /// The operator's last manual reading, used only when the endpoint gave nothing. /// /// A `USAGE_RESET_AT` that is set but unparseable discards the whole reading (returns `None`, -/// i.e. inert) rather than pacing with no reset — matching the shell version, where the date -/// parse and the percent parse shared one error path. +/// i.e. the fail-closed pause) rather than pacing with no reset — matching the shell version, +/// where the date parse and the percent parse shared one error path. fn fallback_reading(used_pct: &str, reset_at: &str) -> Option { if used_pct.trim().is_empty() { return None; @@ -10218,8 +10222,9 @@ fn fallback_reading(used_pct: &str, reset_at: &str) -> Option { }) } -/// One HTTPS GET for the usage document. Any failure is `None` — see [`usage_gate_decide`] for -/// why nothing here may escalate to a pause. +/// One HTTPS GET for the usage document. Any failure is `None` — every transport failure +/// collapses into the one no-reading state [`usage_gate_decide`] pauses on, so no error detail +/// decides anything here. fn fetch_usage(url: &str, token: &str) -> Option { ureq::get(url) .header("Authorization", &format!("Bearer {token}")) @@ -10306,19 +10311,28 @@ mod usage_gate_tests { } const RESET: &str = "2026-07-19T00:00:00Z"; - // ---- inert: the property that must never regress to fail-closed -------------------------- + // ---- fail closed: no reading is a pause, never a run (#273) ------------------------------- - // No endpoint and no fallback => RUN. An earlier gate that could not read usage paused both - // crons for 22 consecutive ticks; this gate paces spending, it does not stall the pipeline. + // No endpoint and no fallback => PAUSE. The gate must not run blind: an unreadable endpoint + // once let a scheduled tick through at 39% used against a 21% pace — the one tick the gate + // could not measure was the one it waved through. The reason must be distinguishable from a + // pace pause at a glance, so a paused-because-blind stretch reads as an endpoint problem from + // any single log line. Kills `UsageVerdict::Pause` -> `UsageVerdict::Run` in the `None` arm. #[test] - fn no_reading_is_inert_and_runs() { + fn no_reading_fails_closed_and_pauses() { let v = usage_gate_decide(None, ms("2026-07-15T00:00:00Z"), 5.0, 90.0); - assert_eq!(v.code(), 0); - assert!(v.reason().starts_with("OK:"), "{}", v.reason()); - assert!(v.reason().contains("gate inert"), "{}", v.reason()); + assert_eq!(v.code(), 10, "{}", v.reason()); + assert!(v.reason().starts_with("PAUSE:"), "{}", v.reason()); + assert!(v.reason().contains("no usage reading"), "{}", v.reason()); + assert!(v.reason().contains("failing closed"), "{}", v.reason()); + assert!( + !v.reason().contains("linear-by-now") && !v.reason().contains("ceiling"), + "a blind pause must not read as a pace or ceiling pause: {}", + v.reason() + ); } - // Every unreadable-usage shape collapses to None, i.e. to the inert path above — never a pause. + // Every unreadable-usage shape collapses to None, i.e. to the fail-closed pause above. #[test] fn unreadable_usage_shapes_never_produce_a_reading() { assert!(parse_seven_day("").is_none(), "empty body"); @@ -10634,11 +10648,11 @@ mod usage_gate_tests { #[test] fn fallback_is_used_only_when_set_and_says_so() { - assert!(fallback_reading("", "").is_none(), "unset => inert"); - assert!(fallback_reading(" ", "").is_none(), "blank => inert"); + assert!(fallback_reading("", "").is_none(), "unset => no reading"); + assert!(fallback_reading(" ", "").is_none(), "blank => no reading"); assert!( fallback_reading("not-a-number", "").is_none(), - "unparseable pct => inert" + "unparseable pct => no reading" ); // Set but with an unparseable reset: discard the whole reading rather than pace blind. assert!( diff --git a/pr-review-report-rs/tests/usage_gate_skip.rs b/pr-review-report-rs/tests/usage_gate_skip.rs index 91f76329..f6a87e71 100644 --- a/pr-review-report-rs/tests/usage_gate_skip.rs +++ b/pr-review-report-rs/tests/usage_gate_skip.rs @@ -10,9 +10,9 @@ //! //! What is stubbed, and why each one is not the thing under test: //! -//! * `usage-gate` — its verdict is the fixture's INPUT. The gate is inert when it cannot read -//! usage (it prints OK and the tick runs), so a test that let the real gate decide would -//! exercise the inert path and never the pause the force exists for. +//! * `usage-gate` — its verdict is the fixture's INPUT. The real gate's answer depends on the +//! box the test runs on (credentials, network, the live usage number), so a test that let it +//! decide would assert on whatever the box happens to say, not on the verdict the case needs. //! * `preflight` — a probe of the BOX (gh's token scopes, whether nix can realise a Solidity //! shell). Its answer is about the machine the test happens to run on, not about forcing. //! * `claude` and `jq` — only in the two whole-run fixtures. `claude` stands in for the model @@ -435,8 +435,9 @@ fn a_refused_vetter_tick_aborts_loudly_and_writes_no_row() { // config nobody validated). Neither yields to anything. // // So the tests come in pairs: a policy stop is driven forced AND scheduled, and a correctness stop -// is driven forced and must still stop. Every one drives a gate that positively decided to PAUSE — -// the inert path (gate cannot read usage, prints OK, tick runs) never exercises the force at all. +// is driven forced and must still stop. Every one drives a gate that decided to PAUSE — exit 10, +// whether from the pace check or from failing closed on an unreadable endpoint, is the one exit +// the force overrides. // --------------------------------------------------------------------------------------------- /// The feature itself: a forced tick runs past the PAUSE, and the row it leaves says so. diff --git a/review-run.sh b/review-run.sh index 0cf79a05..61ba7412 100755 --- a/review-run.sh +++ b/review-run.sh @@ -179,8 +179,9 @@ fi # --- weekly-budget pace gate: skip this tick when usage is over the ceiling or inside the BAU # headroom band under the linear burn toward the reset — the crons hold ~USAGE_HEADROOM_PCT points # BEHIND pace so interactive work keeps standing budget (#158). `usage-gate` reads -# /api/oauth/usage itself; exit 10 means PAUSE (record one skip row, exit 0). It is INERT when it -# cannot read usage and no fallback is set — it prints OK and we run. Any OTHER non-zero exit is a +# /api/oauth/usage itself; exit 10 means PAUSE (record one skip row, exit 0). It FAILS CLOSED when +# it cannot read usage and no fallback is set (#273) — that is a PAUSE too, its reason naming the +# read failure so the skip row is diagnosable as an endpoint problem. Any OTHER non-zero exit is a # config REFUSAL (the retired USAGE_SLACK_PCT still set: exit 2, reason on stderr, captured into # the log): the tick must not run on config the gate refused to read, so propagate the failure — a # refusal is neither a run nor a pause, and it writes NO row. --- From a8d906efbe6e7db5c159b542ff6dcc39d9eec646 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Wed, 12 Aug 2026 06:09:07 +0000 Subject: [PATCH 2/2] test(usage-gate): pin the complete fail-closed pause reason CodeRabbit on #274: fragment asserts let wording drift past the test and never proved the variant. Assert UsageVerdict::Pause and the exact reason. Co-Authored-By: Claude Fable 5 --- pr-review-report-rs/src/main.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/pr-review-report-rs/src/main.rs b/pr-review-report-rs/src/main.rs index 84e3f94a..70aa2ffa 100644 --- a/pr-review-report-rs/src/main.rs +++ b/pr-review-report-rs/src/main.rs @@ -10321,14 +10321,15 @@ mod usage_gate_tests { #[test] fn no_reading_fails_closed_and_pauses() { let v = usage_gate_decide(None, ms("2026-07-15T00:00:00Z"), 5.0, 90.0); + assert!(matches!(v, UsageVerdict::Pause(_)), "{}", v.reason()); assert_eq!(v.code(), 10, "{}", v.reason()); - assert!(v.reason().starts_with("PAUSE:"), "{}", v.reason()); - assert!(v.reason().contains("no usage reading"), "{}", v.reason()); - assert!(v.reason().contains("failing closed"), "{}", v.reason()); - assert!( - !v.reason().contains("linear-by-now") && !v.reason().contains("ceiling"), - "a blind pause must not read as a pace or ceiling pause: {}", - v.reason() + // The complete reason, pinned: it must name the read failure and share no wording with + // the pace ("linear-by-now") or ceiling pauses, so a blind pause is diagnosable from a + // single log line. + assert_eq!( + v.reason(), + "PAUSE: no usage reading — endpoint unreachable and no fallback reading set — \ + cannot pace blind, failing closed" ); }