diff --git a/README.md b/README.md index 58e3762b..250ad182 100644 --- a/README.md +++ b/README.md @@ -3062,28 +3062,54 @@ quietly do nothing. An unrecognised argument is refused for the same reason: a typo'd flag that fell through would skip on the very pause it was typed to run past, and say nothing about why. -What `--force` does **not** bypass, each deliberately: +What `--force` reaches is **one property**, not a list. The list is what got +this wrong the first time: the version first shipped refused the kill switch, +and the first observation run it was built for printed +`SKIP: DISABLED flag present` and did nothing. -| not bypassed | why | -| ------------------------- | ------------------------------------------------------------------------------------------------------------------------------------ | -| `DISABLED` | a deliberate human stop; walking through it makes the one unambiguous off-switch a suggestion | -| the flock | two runs of a role collide on the same clones and the same GitHub state — the answer to "I want to watch a run" is to watch that one | -| a gate config **refusal** | any non-zero gate exit that is not 10 means the gate could not read its config; forcing past it runs on config nobody validated | +> `--force` overrides **policy** stops. It never overrides **correctness** +> stops. -The row a forced run leaves is marked, in the same shape the skip row uses: +A **policy** stop is the pipeline choosing not to spend right now. The human at +the terminal owns that choice and may make it differently for one run: + +| policy stop — yields to `--force` | why the human owns it | +| --------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------- | +| the usage-gate **PAUSE** | holding budget back from a tick nobody is watching is exactly right, and exactly wrong for a watched one | +| `DISABLED` / `review-DISABLED` | the switch stops the **cron**; the human typing `--force` is its own owner overriding their own stop, which is not what the switch protects against | + +A **correctness** stop is the run being unable to do its job properly no matter +who asked. No argument reaches these: + +| correctness stop — never yields | why no one owns it | +| ------------------------------- | ---------------------------------------------------------------------------------------------------------------- | +| the flock | two runs of a role corrupt each other's clones and GitHub state — not a choice about spending | +| a gate config **refusal** | any non-zero gate exit that is not 10: the gate could not validate its config, so the tick would run unvalidated | + +Every override is recorded. The row a forced run leaves carries, in the same +shape the skip row uses but **plural**, because one run can walk past both: ```json -"forced": "usage-gate", "forceReason": "" +"forced": ["disabled", "usage-gate"], "forceReason": ["DISABLED flag present", ""] ``` -Both fields are **absent** — not null — on every scheduled row, and `forced` and -`skipped` can never appear together (they are opposites: a tick the pipeline -declined to run against one it was told to run anyway; `run-metrics` refuses -both at parse). `metrics/runs.jsonl` is what the dashboard draws its run series -from, so a forced run that looked like a paced tick would show budget being -spent on a schedule nobody followed. The marker rides **beside** `outcome`, -never replacing it — forcing changes what the row says about how the run was -started, never how the run is judged. +The two arrays are **parallel**: index _i_ of `forced` is a stop's kind and +index _i_ of `forceReason` is that stop's own line. `run-metrics` refuses a +mismatched pair rather than truncating, because a stop silently paired with the +wrong line is only visible on the dashboard weeks later. + +**Empty is a real state, and it is the common one.** A forced run that met no +stop still carries `"forced": []` — what the row says first is that the schedule +did not start this, and that is true whether or not anything was in the way. +Keying the marker on "something was overridden" would draw exactly that run as a +paced tick. Both fields are **absent** — not null — on every scheduled row, and +`forced` and `skipped` can never appear together (they are opposites: a tick the +pipeline declined to run against one it was told to run anyway; `run-metrics` +refuses both at parse). `metrics/runs.jsonl` is what the dashboard draws its run +series from, so a forced run that looked like a paced tick would show budget +being spent on a schedule nobody followed. The marker rides **beside** +`outcome`, never replacing it — forcing changes what the row says about how the +run was started, never how the run is judged. ## Tooling failures are run failures, not verdict caveats diff --git a/campaign-run.sh b/campaign-run.sh index 47d470ea..635dd256 100755 --- a/campaign-run.sh +++ b/campaign-run.sh @@ -7,7 +7,9 @@ # WATCH: tail -f campaign.log (distilled trail) # tail -f "$(ls -t runs/*.jsonl | head -1)" (full live trace) # RUN NOW: ./campaign-run.sh -# FORCE: ./campaign-run.sh --force (one run, past a usage-gate PAUSE, streamed to stdout) +# FORCE: ./campaign-run.sh --force (one run, past every POLICY stop — the usage-gate +# PAUSE and DISABLED — streamed to stdout. Never past a +# CORRECTNESS stop: the lock, or a gate config refusal.) # # Deployment-specific values live in ./cron.env (gitignored; copy from cron.env.example). # Guardrails: curated allowlist (campaign-settings.json) + the prompt forbids merge/deploy/ @@ -80,10 +82,9 @@ RUNDIR="$DIR/runs" # review-verdicts.jsonl -- are retired. GitHub is the source of truth. # --- one-off manual FORCE (#245) --------------------------------------------------------------- -# `--force` is the only argument this runner takes, and it authorises exactly ONE bypass: the -# weekly-budget pace gate's PAUSE (usage-gate exit 10). It exists because the gate cannot tell a -# deliberate human-initiated observation run from a scheduled tick, and holding budget back from a -# tick nobody is watching is right where running a watched one is not. +# `--force` is the only argument this runner takes. It exists because nothing else can tell a +# deliberate human-initiated observation run from a scheduled tick, and the pipeline's pacing +# decisions are all written for the tick. # # CRON_DIR= nix run git+file://#campaign-run -- --force # @@ -96,14 +97,29 @@ RUNDIR="$DIR/runs" # left switched on, where a force in cron.env would silently force every scheduled tick for ever. # CRON_FORCE is refused below so that door is shut rather than merely unused. # -# What --force does NOT bypass, each for its own reason: -# * the DISABLED kill switch — a deliberate human stop; a force that walked through it would turn -# the one unambiguous off-switch into a suggestion; -# * the flock — two runs of a role collide on the same clones and the same GitHub state, so the -# answer to "I want to watch a run" is to watch the one already going; -# * a gate config REFUSAL (any non-zero exit that is not 10) — a refusal means the gate could not -# read its config, and running past that is running on config nobody validated, which is a -# different thing entirely from running past a budget ceiling on purpose. +# What `--force` overrides is ONE PROPERTY, not a list — the list is what got this wrong the first +# time (#245 shipped a force that refused the kill switch, and the first observation run it was +# built for printed `SKIP: DISABLED flag present` and did nothing): +# +# --force overrides POLICY stops. It never overrides CORRECTNESS stops. +# +# A POLICY stop is the pipeline choosing not to spend right now. The human at the terminal owns that +# choice and is allowed to make it differently for one run: the usage-gate PAUSE (holding budget +# back from a tick nobody is watching is exactly right, and exactly wrong for a watched one), and the +# DISABLED kill switch (it exists to stop the CRON; the human typing --force is that switch's own +# owner deliberately overriding their own stop, which is not what the switch protects against). +# +# A CORRECTNESS stop is the run being unable to do its job properly no matter who asked. No argument +# reaches these: +# * the flock — two runs of a role collide on the same clones and the same GitHub state. That is +# not a policy choice about spending, it is two processes corrupting each other's work. +# * a usage-gate config REFUSAL (any non-zero exit that is not 10) — the gate could not validate +# its config, so the tick would run on config nobody checked. Forcing past a ceiling on purpose +# and running on unvalidated config are not the same act. +# +# Every override is RECORDED: each one appends its kind and the stop's own line to the run's +# metrics/runs.jsonl row (see the FORCE stamp below), so the dashboard reads what was overridden +# rather than merely that something was. FORCE=0 while [ "$#" -gt 0 ]; do case "$1" in @@ -120,6 +136,14 @@ while [ "$#" -gt 0 ]; do esac done +# What this run actually OVERRODE, appended to as each policy stop is walked past. Two parallel +# arrays because a stop's kind and that stop's own line are one fact in two parts, exactly as +# `skipped`/`skipReason` are — and there can be more than one, since a single forced run can walk +# past both the kill switch and a gate pause. Empty is a real state and NOT the same as absent: a +# forced run that met no stop at all still has to say a human started it (see the FORCE stamp). +FORCED_KINDS=() +FORCED_REASONS=() + # --- where this run's trail goes --------------------------------------------------------------- # A scheduled tick has no terminal, so its trail is appended to $LOG and nowhere else. A FORCED run # is being WATCHED — live observability is the whole reason it exists — so the same bytes also @@ -132,11 +156,20 @@ else fi # --- kill switch --- -# NOT bypassed by --force (#245): this file is a deliberate stop with a human behind it, and a force -# that walked through it would turn the one unambiguous off-switch into a suggestion. +# A POLICY stop, so --force overrides it (#245 as ruled: "force needs to force"). This file stops +# the CRON. The human typing --force is the switch's own owner, at a terminal, deliberately +# overriding their own stop for one run — which is not what the switch protects against, and a +# force that refused them would be friction and nothing else. A SCHEDULED tick still honours it +# exactly as it always has, which is the whole reason the file exists. if [ -f "$DIR/DISABLED" ]; then - echo "$(date -u +%FT%TZ) SKIP: DISABLED flag present" | _log - exit 0 + if [ "$FORCE" -eq 1 ]; then + echo "$(date -u +%FT%TZ) FORCED past the DISABLED kill switch (--force)" | _log + FORCED_KINDS+=(disabled) + FORCED_REASONS+=("DISABLED flag present") + else + echo "$(date -u +%FT%TZ) SKIP: DISABLED flag present" | _log + exit 0 + fi fi # The stale-setting guard, in the posture `usage-gate` already takes toward the retired @@ -174,6 +207,8 @@ if [ "$_ugrc" -eq 10 ]; then # force lives INSIDE the exit-10 branch on purpose — a refusal (below) can never reach it, so # `--force` cannot be the thing that runs the pipeline on config the gate would not read. echo "$(date -u +%FT%TZ) FORCED past the usage-gate PAUSE (--force): $_ug" | _log + FORCED_KINDS+=(usage-gate) + FORCED_REASONS+=("$_ug") else # A paused tick still writes its metrics/runs.jsonl row (#160): the dashboard reads runs from # that file, and a pause that wrote nothing rendered as a dead stretch indistinguishable from a @@ -200,13 +235,21 @@ fi # --- the FORCE stamp every row this run writes carries (#245) ---------------------------------- # A forced run is not a paced tick, and a runs.jsonl row that cannot say so puts budget on the -# dashboard's run series against a schedule that was never followed. Built ONCE, here, from the -# gate's own line — including an OK line, because what makes a row forced is the human who typed -# `--force`, not what the gate happened to decide. Empty for a scheduled tick, which is what keeps -# every existing row byte-identical. +# dashboard's run series against a schedule that was never followed. Built ONCE, here, after the +# last stop a force can walk past, so it carries what this run ACTUALLY overrode rather than what a +# force is allowed to override. +# +# `--forced-run` is the fact that a human started this, and it is passed whenever `--force` was — +# INCLUDING when nothing was in the way, which is the ordinary case once the crons are running +# again. Each `--forced/--force-reason` pair is one stop that was actually walked past. So an empty +# stamp still says "not scheduled", and a consumer reading the kinds learns exactly which stops +# yielded. Absent entirely for a scheduled tick, which is what keeps every other row byte-identical. FORCED_FLAGS=() if [ "$FORCE" -eq 1 ]; then - FORCED_FLAGS=(--forced usage-gate --force-reason "$_ug") + FORCED_FLAGS=(--forced-run) + for _i in "${!FORCED_KINDS[@]}"; do + FORCED_FLAGS+=(--forced "${FORCED_KINDS[$_i]}" --force-reason "${FORCED_REASONS[$_i]}") + done fi # --- single-run lock (non-blocking: skip this tick if a prior run is still going) --- diff --git a/pr-review-report-rs/src/main.rs b/pr-review-report-rs/src/main.rs index f59b540b..6620ceb2 100644 --- a/pr-review-report-rs/src/main.rs +++ b/pr-review-report-rs/src/main.rs @@ -5467,6 +5467,17 @@ struct RunIdentity<'a> { model: Option<&'a str>, } +/// What a `--force`d run overrode, as the runner observed it. +/// +/// The kinds and the reasons are PARALLEL: index i of each is one stop. Both empty is the ordinary +/// forced run — a human started it and nothing was in the way — and is why this is a struct rather +/// than a non-empty list: the fact being recorded is "a human started this", with the overrides as +/// detail on top of it, not the other way round. +struct ForceStamp<'a> { + kinds: &'a [String], + reasons: &'a [String], +} + /// Stamp a record with the run's identity. /// /// Only the fields the caller actually supplied are added, so a bare `run-metrics ` keeps @@ -5912,8 +5923,21 @@ fn run_metrics_mode( preflight_missing: &[String], infra_path: Option<&str>, skip: Option<(&str, &str)>, - forced: Option<(&str, &str)>, + forced: Option, ) -> i32 { + // The pairing is positional, so a caller that supplies a different number of each has written a + // row where a stop names the wrong line — silently, and only visible on the dashboard weeks + // later. Refused here rather than truncated to the shorter of the two. + if let Some(f) = &forced { + if f.kinds.len() != f.reasons.len() { + eprintln!( + "error: --forced and --force-reason are paired by position; got {} and {}", + f.kinds.len(), + f.reasons.len() + ); + return 2; + } + } let content = match std::fs::read_to_string(path) { Ok(c) => c, Err(e) => { @@ -5969,7 +5993,7 @@ fn final_record( preflight_missing: &[String], infra: &InfraRecord, skip: Option<(&str, &str)>, - forced: Option<(&str, &str)>, + forced: Option, spend: &SpendRecord, ) -> Value { let mut doc = serde_json::json!({ @@ -6061,14 +6085,18 @@ fn final_record( } // The FORCE stamp (#245): the mirror image of the skip stamp above, and present for the same // reason. `metrics/runs.jsonl` is where the dashboard's run series comes from, and a run a human - // forced past the pace gate is not a tick the schedule produced — drawn as one, it shows budget - // being spent on a pace nobody followed. ABSENT, not null, on every scheduled row, so a consumer - // keys on the field existing at all and every pre-#245 record stays byte-compatible. - // `forceReason` is the gate's own line at the moment of the override, verbatim — including an OK - // line, because what makes a row forced is the human who typed `--force`, not the gate's verdict. - if let (Some(obj), Some((gate, reason))) = (doc.as_object_mut(), forced) { - obj.insert("forced".into(), serde_json::json!(gate)); - obj.insert("forceReason".into(), serde_json::json!(reason)); + // started by hand is not a tick the schedule produced — drawn as one, it shows budget being + // spent on a pace nobody followed. ABSENT, not null, on every scheduled row, so a consumer keys + // on the field existing at all and every pre-#245 record stays byte-compatible. + // + // ARRAYS, because a forced run can walk past more than one POLICY stop — the kill switch and a + // gate pause are both overridable and both get recorded. They are parallel: index i of `forced` + // is the stop's kind and index i of `forceReason` is that stop's own line, verbatim. EMPTY is a + // real and common state, not a missing one: a force that met no stop still says a human started + // the run, which is the first thing the row has to carry. + if let (Some(obj), Some(f)) = (doc.as_object_mut(), forced) { + obj.insert("forced".into(), serde_json::json!(f.kinds)); + obj.insert("forceReason".into(), serde_json::json!(f.reasons)); } doc } @@ -34586,17 +34614,23 @@ enum Cmd { /// the two arrive together or not at all. #[arg(long, requires = "skipped")] skip_reason: Option, - /// The pre-model gate a human deliberately RAN PAST for this one invocation (`usage-gate`, - /// #245). Present means the tick was not paced — somebody typed `--force` at the runner — - /// so the row must not be read as one the schedule produced. It is the OPPOSITE of - /// `skipped`: a skip is a tick the pipeline chose not to run, a force is one it was told to - /// run anyway, and the two can never appear on the same row. - #[arg(long, requires = "force_reason", conflicts_with = "skipped")] - forced: Option, - /// The forced gate's own output line, verbatim, recorded as `forceReason` beside `forced` — - /// the two arrive together or not at all. - #[arg(long, requires = "forced")] - force_reason: Option, + /// A human started this run with `--force` (#245). This alone is what makes the row a + /// forced one: it is the OPPOSITE of `skipped` — a skip is a tick the pipeline chose not to + /// run, a force is one it was told to run anyway — so the two can never share a row. + /// + /// Passed even when the force met NO stop, which is the ordinary case: what the row has to + /// say first is that the schedule did not start this, and that is true whether or not + /// anything was in the way. + #[arg(long, conflicts_with = "skipped")] + forced_run: bool, + /// One POLICY stop this run actually walked past (`disabled`, `usage-gate`). Repeatable — + /// a single forced run can override both — and paired BY POSITION with `--force-reason`. + #[arg(long, requires = "forced_run")] + forced: Vec, + /// The matching stop's own output line, verbatim, recorded as `forceReason` beside + /// `forced`. One per `--forced`, in the same order: they are one fact in two parts. + #[arg(long, requires = "forced_run")] + force_reason: Vec, }, /// Resolve every external binary the HARNESS needs at read time, plus each capability asked /// for. Exit 12 if any is unsatisfied. @@ -38458,6 +38492,7 @@ fn main() { infra, skipped, skip_reason, + forced_run, forced, force_reason, } => run_metrics_mode( @@ -38471,7 +38506,10 @@ fn main() { &preflight_missing, infra.as_deref(), skipped.as_deref().zip(skip_reason.as_deref()), - forced.as_deref().zip(force_reason.as_deref()), + forced_run.then(|| ForceStamp { + kinds: &forced, + reasons: &force_reason, + }), ), Cmd::Preflight { gh_auth, sol_shell } => preflight_mode(gh_auth, sol_shell), Cmd::SolToolchain { dir } => sol_toolchain_mode(&dir), @@ -41919,8 +41957,8 @@ mod startup_split_tests { #[cfg(test)] mod skip_row_tests { use super::{ - classify_outcome, final_record, InfraRecord, RunIdentity, RunMetrics, SpendRecord, - ToolingReport, TraceOutcome, STAGE_FINAL, + classify_outcome, final_record, run_metrics_mode, ForceStamp, InfraRecord, RunIdentity, + RunMetrics, SpendRecord, ToolingReport, TraceOutcome, STAGE_FINAL, }; /// The gate's real ceiling-pause line, verbatim — em-dash, percent signs and all — because the @@ -42000,13 +42038,17 @@ mod skip_row_tests { } /// The FORCE row (#245), the skip row's mirror image. `metrics/runs.jsonl` is what the - /// dashboard draws its run series from, so a run a human forced past the pace gate has to be + /// dashboard draws its run series from, so a run a human started by hand has to be /// distinguishable from a tick the schedule produced — otherwise the series shows budget spent - /// on a pace nobody followed. The gate's line is carried verbatim for the same reason the skip - /// row carries it: this row is the only durable copy of what the gate said at the moment of - /// the override. - #[test] - fn a_forced_row_carries_the_gate_it_ran_past_and_that_gates_line_verbatim() { + /// on a pace nobody followed. Each stop's line is carried verbatim for the same reason the skip + /// row carries the gate's: this row is the only durable copy of what that stop said at the + /// moment it was overridden. + #[test] + fn a_forced_row_carries_every_stop_it_ran_past_and_each_stops_line_verbatim() { + // BOTH policy stops, which is the case a single value could never carry: a run that walked + // past the kill switch AND a gate pause is a different fact from one that walked past either. + let kinds = ["disabled".to_string(), "usage-gate".to_string()]; + let reasons = ["DISABLED flag present".to_string(), PAUSE_LINE.to_string()]; let doc = final_record( "/runs/20260731T090001Z.jsonl", &RunMetrics::default(), @@ -42016,13 +42058,17 @@ mod skip_row_tests { &[], &InfraRecord::default(), None, - Some(("usage-gate", PAUSE_LINE)), + Some(ForceStamp { + kinds: &kinds, + reasons: &reasons, + }), &SpendRecord::default(), ); - assert_eq!(doc["forced"], "usage-gate"); + assert_eq!(doc["forced"], serde_json::json!(["disabled", "usage-gate"])); assert_eq!( - doc["forceReason"], PAUSE_LINE, - "the reason must be verbatim" + doc["forceReason"], + serde_json::json!(["DISABLED flag present", PAUSE_LINE]), + "each stop's own line, verbatim, positionally paired with its kind" ); assert!( doc.get("skipped").is_none(), @@ -42036,11 +42082,68 @@ mod skip_row_tests { assert_eq!(doc["runId"], "20260731T090001Z"); } + /// A mismatched pair is REFUSED, not truncated. + /// + /// The kinds and the reasons are paired by POSITION, so a caller that supplies a different + /// number of each has written a row where a stop names another stop's line. Truncating to the + /// shorter of the two would produce a well-formed row that is silently wrong, and the only + /// place it would ever show up is the dashboard, weeks later. + #[test] + fn a_positional_pair_of_different_lengths_is_refused() { + let trace = std::env::temp_dir().join(format!("force-pair-{}.jsonl", std::process::id())); + std::fs::write(&trace, "").expect("write empty trace"); + let path = trace.to_str().expect("utf-8 temp path"); + let id = RunIdentity { + run_id: None, + role: None, + model: None, + }; + let kinds = ["disabled".to_string(), "usage-gate".to_string()]; + let one = [PAUSE_LINE.to_string()]; + assert_eq!( + run_metrics_mode( + path, + &id, + None, + &[], + None, + None, + Some(ForceStamp { + kinds: &kinds, + reasons: &one, + }), + ), + 2, + "two kinds and one reason must abort, not truncate" + ); + // …and the matched pair it was nearly mistaken for still succeeds, so what the guard tests + // is the LENGTHS and not merely the presence of more than one override. + let two = ["DISABLED flag present".to_string(), PAUSE_LINE.to_string()]; + assert_eq!( + run_metrics_mode( + path, + &id, + None, + &[], + None, + None, + Some(ForceStamp { + kinds: &kinds, + reasons: &two, + }), + ), + 0 + ); + let _ = std::fs::remove_file(&trace); + } + /// A forced run that ends badly is still judged on the run. The marker rides beside the /// outcome rather than replacing it — otherwise "forced" would become a way to hide a failure, /// and the one thing a forced run exists for is to be watched honestly. #[test] fn forcing_does_not_launder_the_outcome() { + let kinds = ["usage-gate".to_string()]; + let reasons = [PAUSE_LINE.to_string()]; let doc = final_record( "/t.jsonl", &RunMetrics::default(), @@ -42050,10 +42153,13 @@ mod skip_row_tests { &["pdftoppm".to_string()], &InfraRecord::default(), None, - Some(("usage-gate", PAUSE_LINE)), + Some(ForceStamp { + kinds: &kinds, + reasons: &reasons, + }), &SpendRecord::default(), ); - assert_eq!(doc["forced"], "usage-gate"); + assert_eq!(doc["forced"], serde_json::json!(["usage-gate"])); assert_eq!(doc["outcome"], "tooling-failure"); assert_eq!(doc["exitCode"], 12); assert_eq!(doc["missingTools"], serde_json::json!(["pdftoppm"])); @@ -48916,8 +49022,9 @@ mod cli_tests { infra: None, skipped: None, skip_reason: None, - forced: None, - force_reason: None, + forced_run: false, + forced: vec![], + force_reason: vec![], } ); // The form the runners now use in place of the `| jq '. + {…}'` pipe. @@ -48945,8 +49052,9 @@ mod cli_tests { infra: None, skipped: None, skip_reason: None, - forced: None, - force_reason: None, + forced_run: false, + forced: vec![], + force_reason: vec![], } ); // The abort form: `preflight` found nothing to render with, so the model never started. @@ -48970,8 +49078,9 @@ mod cli_tests { infra: None, skipped: None, skip_reason: None, - forced: None, - force_reason: None, + forced_run: false, + forced: vec![], + force_reason: vec![], } ); // The SKIP form (#160): the usage-gate paused the tick, the runners record the row with @@ -49007,8 +49116,9 @@ mod cli_tests { "PAUSE: 91% of the weekly budget used (endpoint) — at/over the 90% ceiling" .to_string() ), - forced: None, - force_reason: None, + forced_run: false, + forced: vec![], + force_reason: vec![], } ); // The two skip flags arrive together or not at all: a gate with no reason would emit a @@ -49045,6 +49155,11 @@ mod cli_tests { "claude-fable-5", "--exit-code", "0", + "--forced-run", + "--forced", + "disabled", + "--force-reason", + "DISABLED flag present", "--forced", "usage-gate", "--force-reason", @@ -49060,19 +49175,59 @@ mod cli_tests { infra: None, skipped: None, skip_reason: None, - forced: Some("usage-gate".to_string()), - force_reason: Some( + forced_run: true, + forced: vec!["disabled".to_string(), "usage-gate".to_string()], + force_reason: vec![ + "DISABLED flag present".to_string(), "PAUSE: 91% of the weekly budget used (endpoint) — at/over the 90% ceiling" - .to_string() - ), + .to_string(), + ], } ); - // Paired at parse for the same reason the skip flags are: a forced row that cannot say what - // the gate said is a marker with no evidence behind it. + // The ORDINARY forced run: a human started it and nothing was in the way. It still parses, + // and still says the schedule did not start it — which is what `--forced-run` carries on + // its own. Lose this case and the marker comes to mean "something blocked me" instead of + // "a human started me". + assert_eq!( + parse(&["prr", "run-metrics", "/t.jsonl", "--forced-run"]), + Cmd::RunMetrics { + trace: "/t.jsonl".to_string(), + run_id: None, + role: None, + model: None, + exit_code: None, + preflight_missing: vec![], + infra: None, + skipped: None, + skip_reason: None, + forced_run: true, + forced: vec![], + force_reason: vec![], + } + ); + // An override with nothing to be an override OF: refused at parse, so a runner edit that + // dropped `--forced-run` fails loudly rather than writing a row that reads as scheduled + // while naming stops it walked past. + assert!( + Cli::try_parse_from([ + "prr", + "run-metrics", + "/t.jsonl", + "--forced", + "usage-gate", + "--force-reason", + "PAUSE: x" + ]) + .is_err(), + "--forced and --force-reason without --forced-run must be refused" + ); + // EACH flag carries its own `requires`. Asserted separately because the pair above is + // refused as long as EITHER one does — so deleting `forced`'s requirement would leave that + // assertion passing while `--forced` alone silently parsed into a row nothing marks. assert!( Cli::try_parse_from(["prr", "run-metrics", "/t.jsonl", "--forced", "usage-gate"]) .is_err(), - "--forced without --force-reason must be refused" + "--forced alone must be refused" ); assert!( Cli::try_parse_from([ @@ -49083,7 +49238,7 @@ mod cli_tests { "PAUSE: x" ]) .is_err(), - "--force-reason without --forced must be refused" + "--force-reason without --forced-run must be refused" ); // SKIPPED and FORCED are opposites — a tick the pipeline declined to run against one it was // told to run anyway — so a row can never be both. Refused at parse, where a runner edit @@ -49097,10 +49252,7 @@ mod cli_tests { "usage-gate", "--skip-reason", "PAUSE: x", - "--forced", - "usage-gate", - "--force-reason", - "PAUSE: x" + "--forced-run" ]) .is_err(), "a row cannot be both skipped and forced" diff --git a/pr-review-report-rs/tests/usage_gate_skip.rs b/pr-review-report-rs/tests/usage_gate_skip.rs index 8a8e2a73..7c8b0132 100644 --- a/pr-review-report-rs/tests/usage_gate_skip.rs +++ b/pr-review-report-rs/tests/usage_gate_skip.rs @@ -414,10 +414,20 @@ fn a_refused_vetter_tick_aborts_loudly_and_writes_no_row() { // --------------------------------------------------------------------------------------------- // The one-off manual force (#245). // -// The whole design is in what `--force` does and does NOT bypass, so each guarantee gets a test -// that fails if the bypass widens by one step. Every one of them 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. +// One property decides every test below, and it is a property rather than a list because the list +// is what got this wrong the first time: +// +// --force overrides POLICY stops. It never overrides CORRECTNESS stops. +// +// POLICY — the pipeline choosing not to spend right now, a choice the human at the terminal owns: +// the usage-gate PAUSE, and the kill switch. Each yields, and each records that it yielded. +// CORRECTNESS — the run being unable to do its job properly no matter who asked: the flock (two +// processes corrupting each other's clones and GitHub state) and a gate config REFUSAL (running on +// 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. // --------------------------------------------------------------------------------------------- /// The feature itself: a forced tick runs past the PAUSE, and the row it leaves says so. @@ -440,11 +450,13 @@ fn a_forced_run_past_a_pause_runs_marked_and_streamed(role: &'static Role, name: ); let row = f.final_row(); assert_eq!( - row["forced"], "usage-gate", - "the row must name the gate the run was forced past" + row["forced"], + serde_json::json!(["usage-gate"]), + "the row names the stop this run walked past, and only that one" ); assert_eq!( - row["forceReason"], PAUSE_LINE, + row["forceReason"], + serde_json::json!([PAUSE_LINE]), "the gate's own line, verbatim — this row is the only durable copy of what it said" ); assert!( @@ -490,6 +502,54 @@ fn a_forced_vetter_run_past_a_pause_runs_marked_and_streamed() { a_forced_run_past_a_pause_runs_marked_and_streamed(&VETTER, "vetter-forced"); } +/// The ORDINARY forced run, once the crons are back: a human starts one to watch it and NOTHING is +/// in the way. The row still has to say the schedule did not start it — that is the first thing the +/// marker carries — with an empty override list, because nothing was overridden. +/// +/// This is the case that decides the marker's shape. Key the marker on "something was overridden" +/// and this run reads as a paced tick on the dashboard, which is the corruption the field exists to +/// prevent. +fn a_forced_run_that_met_no_stop_is_still_marked(role: &'static Role, name: &str) { + if !runner_runtime_available() { + return; + } + let Some(f) = Fixture::new(role, name, 0, OK_LINE).map(Fixture::with_model) else { + return; + }; + let out = f.tick_forced(); + assert!( + out.status.success(), + "the forced run completes: {}", + String::from_utf8_lossy(&out.stderr) + ); + let row = f.final_row(); + assert_eq!( + row["forced"], + serde_json::json!([]), + "present and EMPTY: a human started it, and nothing was in the way" + ); + assert_eq!(row["forceReason"], serde_json::json!([])); + assert!( + row.get("forced").is_some(), + "present is the whole point — absent is what a scheduled tick looks like" + ); + // And it is still a watched run. + assert!( + String::from_utf8_lossy(&out.stdout).contains("run START"), + "a forced run streams whether or not it overrode anything" + ); +} + +#[test] +fn a_forced_producer_run_that_met_no_stop_is_still_marked() { + a_forced_run_that_met_no_stop_is_still_marked(&PRODUCER, "producer-forced-clear"); +} + +#[test] +fn a_forced_vetter_run_that_met_no_stop_is_still_marked() { + a_forced_run_that_met_no_stop_is_still_marked(&VETTER, "vetter-forced-clear"); +} + /// The other side of the same contract, and the reason the marker is worth anything: an ordinary /// tick the gate let through is byte-identical to what it always was — no force fields, and not /// one byte on stdout, which is what keeps `metrics/runs.jsonl` a record of the SCHEDULE. @@ -533,7 +593,7 @@ fn an_unforced_vetter_run_is_unmarked_and_silent() { an_unforced_run_is_unmarked_and_silent(&VETTER, "vetter-unforced"); } -/// GUARANTEE 1 — the force reaches exit 10 and nothing else. +/// CORRECTNESS STOP — a gate config REFUSAL never yields, forced or not. /// /// A config REFUSAL means the gate could not read its config; forcing past that would run the /// pipeline on config nobody validated, which is a different thing entirely from running past a @@ -569,43 +629,103 @@ fn force_does_not_bypass_a_vetter_gate_refusal() { force_does_not_bypass_a_gate_refusal(&VETTER, "vetter-force-refuse"); } -/// GUARANTEE 2 — the DISABLED kill switch outranks the force. +/// POLICY STOP — the kill switch yields to `--force`, and says so on the row. +/// +/// This REVERSES what #245 first shipped, on the user's ruling ("force needs to force") after the +/// first observation run the feature exists for printed `SKIP: DISABLED flag present` and did +/// nothing. The file stops the CRON; the human typing `--force` is the switch's own owner +/// overriding their own stop for one run, which is not what the switch protects against. /// -/// The file is a deliberate stop with a human behind it. A force that walked through it would turn -/// the one unambiguous off-switch into a suggestion. The gate is set to PAUSE so that a force which -/// merely ran the gate early — rather than obeying the switch — would still be caught. -fn force_does_not_bypass_the_kill_switch(role: &'static Role, name: &str) { +/// Both policy stops are set at once — the switch AND a gate PAUSE — because a force that walked +/// past one and stalled on the other is the exact half-done state the ruling is about, and because +/// it is the case only an array-shaped marker can record. +fn force_overrides_the_kill_switch(role: &'static Role, name: &str) { + if !runner_runtime_available() { + return; + } let Some(f) = Fixture::new(role, name, 10, PAUSE_LINE).map(Fixture::with_model) else { return; }; f.write_install(role.disabled, ""); let out = f.tick_forced(); + assert!( + out.status.success(), + "the forced run must complete: {}", + String::from_utf8_lossy(&out.stderr) + ); + let row = f.final_row(); + assert_eq!( + row["forced"], + serde_json::json!(["disabled", "usage-gate"]), + "both stops it walked past, in the order it met them" + ); + assert_eq!( + row["forceReason"][0], + serde_json::json!(format!("{} flag present", role.disabled)), + "the kill switch's own line, paired with its kind by position" + ); + assert_eq!( + row["forceReason"][1], + serde_json::json!(PAUSE_LINE), + "and the gate's, verbatim" + ); + let log = f.log(); + assert!( + log.contains("FORCED past the") && log.contains(role.disabled), + "the override is logged in the same shape the gate override is: {log}" + ); + assert!(!log.contains("SKIP:"), "and nothing skipped: {log}"); +} + +#[test] +fn force_overrides_the_producer_kill_switch() { + force_overrides_the_kill_switch(&PRODUCER, "producer-force-disabled"); +} + +#[test] +fn force_overrides_the_vetter_kill_switch() { + force_overrides_the_kill_switch(&VETTER, "vetter-force-disabled"); +} + +/// …and a SCHEDULED tick still honours it, exactly as it always has. +/// +/// This is the half that must not regress, and it is what makes the override a HUMAN's override +/// rather than a hole: the switch's whole job is stopping the cron, and the cron passes no +/// argument. The gate is set to PAUSE so a runner that had merely reordered its stops — checking +/// the gate first — would be caught here rather than passing by coincidence. +fn a_scheduled_tick_still_honours_the_kill_switch(role: &'static Role, name: &str) { + let Some(f) = Fixture::new(role, name, 10, PAUSE_LINE).map(Fixture::with_model) else { + return; + }; + f.write_install(role.disabled, ""); + let out = f.tick(); assert!( out.status.success(), "a disabled runner exits 0: the stop is deliberate, not an error" ); assert!( !f.runs_jsonl().exists(), - "a disabled runner writes no row at all — forced or not, it never got as far as the gate" + "and writes no row at all — not even the gate's skip row, which it never reached" ); + let log = f.log(); assert!( - f.log().contains("SKIP:") && f.log().contains(role.disabled), - "the log names the kill switch: {}", - f.log() + log.contains("SKIP:") && log.contains(role.disabled), + "the log names the kill switch: {log}" ); + assert!(!log.contains("FORCED"), "nothing was forced: {log}"); } #[test] -fn force_does_not_bypass_the_producer_kill_switch() { - force_does_not_bypass_the_kill_switch(&PRODUCER, "producer-force-disabled"); +fn a_scheduled_producer_tick_still_honours_the_kill_switch() { + a_scheduled_tick_still_honours_the_kill_switch(&PRODUCER, "producer-scheduled-disabled"); } #[test] -fn force_does_not_bypass_the_vetter_kill_switch() { - force_does_not_bypass_the_kill_switch(&VETTER, "vetter-force-disabled"); +fn a_scheduled_vetter_tick_still_honours_the_kill_switch() { + a_scheduled_tick_still_honours_the_kill_switch(&VETTER, "vetter-scheduled-disabled"); } -/// GUARANTEE 3 — the flock outranks the force. +/// CORRECTNESS STOP — the flock never yields, forced or not. /// /// Two runs of one role collide on the same clones and the same GitHub state, so the answer to "I /// want to watch a run" is to watch the one already going. This also carries the positive half of @@ -652,13 +772,17 @@ fn force_does_not_bypass_the_vetter_lock() { force_does_not_bypass_the_lock(&VETTER, "vetter-force-lock"); } -/// GUARANTEE 4 — the force cannot be left switched on. +/// The force cannot be left switched on. /// /// `CRON_FORCE` is the obvious wrong guess, and it is the shape that would be dangerous: a variable /// in `cron.env` forces EVERY scheduled tick, silently and for ever. So it is REFUSED rather than /// ignored — the posture `usage-gate` already takes toward the retired `USAGE_SLACK_PCT` — and /// refused whether or not `--force` was also passed, because the point is that the setting must /// surface, not that this particular invocation was legitimate. +/// +/// This is why a variable can never stand in for the argument even now that the kill switch yields: +/// what the switch yields to is a HUMAN AT A TERMINAL, and a setting in a file is the opposite of +/// that — it is the absent human, repeating for ever. fn a_force_variable_is_refused_however_it_arrives(role: &'static Role, name: &str) { let Some(f) = Fixture::new(role, name, 10, PAUSE_LINE).map(Fixture::with_model) else { return; diff --git a/review-run.sh b/review-run.sh index cec9af57..1a101e42 100755 --- a/review-run.sh +++ b/review-run.sh @@ -8,7 +8,9 @@ # DISABLE: touch review-DISABLED (independent of the producer cron's DISABLED) # WATCH: tail -f review.log # RUN NOW: ./review-run.sh -# FORCE: ./review-run.sh --force (one run, past a usage-gate PAUSE, streamed to stdout) +# FORCE: ./review-run.sh --force (one run, past every POLICY stop — the usage-gate PAUSE +# and review-DISABLED — streamed to stdout. Never past a +# CORRECTNESS stop: the lock, or a gate config refusal.) # Deployment values come from ./cron.env (PR_ASSIGNEE, optional REVIEW_MODEL/REVIEW_MAXTIME/REVIEW_KEEP_RUNS). # Packaged as a flake output (`packages.review-run`), so nix builds PATH from the flake's locked @@ -71,9 +73,7 @@ RUNDIR="$DIR/review-runs" # --- one-off manual FORCE (#245) --------------------------------------------------------------- # Identical in shape and in wording to campaign-run.sh's, because a force that works on the producer -# and silently does nothing on the vetter is worse than no force at all. `--force` is the only -# argument this runner takes and it authorises exactly ONE bypass: the weekly-budget pace gate's -# PAUSE (usage-gate exit 10). +# and silently does nothing on the vetter is worse than no force at all. # # CRON_DIR= nix run git+file://#review-run -- --force # @@ -86,10 +86,29 @@ RUNDIR="$DIR/review-runs" # left switched on, where a force in cron.env would silently force every scheduled tick for ever. # CRON_FORCE is refused below so that door is shut rather than merely unused. # -# It does NOT bypass the review-DISABLED kill switch (a deliberate human stop), the flock (two -# vetter runs collide on the same checkouts and the same GitHub state), or a gate config REFUSAL -# (any non-zero exit that is not 10 — the gate could not read its config, and running past that is -# running on config nobody validated). +# What `--force` overrides is ONE PROPERTY, not a list — the list is what got this wrong the first +# time (#245 shipped a force that refused the kill switch, and the first observation run it was +# built for printed `SKIP: DISABLED flag present` and did nothing): +# +# --force overrides POLICY stops. It never overrides CORRECTNESS stops. +# +# A POLICY stop is the pipeline choosing not to spend right now. The human at the terminal owns that +# choice and is allowed to make it differently for one run: the usage-gate PAUSE (holding budget +# back from a tick nobody is watching is exactly right, and exactly wrong for a watched one), and the +# review-DISABLED kill switch (it exists to stop the CRON; the human typing --force is that switch's own +# owner deliberately overriding their own stop, which is not what the switch protects against). +# +# A CORRECTNESS stop is the run being unable to do its job properly no matter who asked. No argument +# reaches these: +# * the flock — two runs of a role collide on the same clones and the same GitHub state. That is +# not a policy choice about spending, it is two processes corrupting each other's work. +# * a usage-gate config REFUSAL (any non-zero exit that is not 10) — the gate could not validate +# its config, so the tick would run on config nobody checked. Forcing past a ceiling on purpose +# and running on unvalidated config are not the same act. +# +# Every override is RECORDED: each one appends its kind and the stop's own line to the run's +# metrics/runs.jsonl row (see the FORCE stamp below), so the dashboard reads what was overridden +# rather than merely that something was. FORCE=0 while [ "$#" -gt 0 ]; do case "$1" in @@ -106,6 +125,14 @@ while [ "$#" -gt 0 ]; do esac done +# What this run actually OVERRODE, appended to as each policy stop is walked past. Two parallel +# arrays because a stop's kind and that stop's own line are one fact in two parts, exactly as +# `skipped`/`skipReason` are — and there can be more than one, since a single forced run can walk +# past both the kill switch and a gate pause. Empty is a real state and NOT the same as absent: a +# forced run that met no stop at all still has to say a human started it (see the FORCE stamp). +FORCED_KINDS=() +FORCED_REASONS=() + # --- where this run's trail goes --------------------------------------------------------------- # A scheduled tick has no terminal, so its trail is appended to $LOG and nowhere else. A FORCED run # is being WATCHED — live observability is the whole reason it exists — so the same bytes also @@ -118,10 +145,20 @@ else fi # --- kill switch (independent of the producer cron) --- -# NOT bypassed by --force (#245): this file is a deliberate stop with a human behind it, and a force -# that walked through it would turn the one unambiguous off-switch into a suggestion. +# A POLICY stop, so --force overrides it (#245 as ruled: "force needs to force"). This file stops +# the CRON. The human typing --force is the switch's own owner, at a terminal, deliberately +# overriding their own stop for one run — which is not what the switch protects against, and a +# force that refused them would be friction and nothing else. A SCHEDULED tick still honours it +# exactly as it always has, which is the whole reason the file exists. if [ -f "$DIR/review-DISABLED" ]; then - echo "$(date -u +%FT%TZ) SKIP: review-DISABLED flag present" | _log; exit 0 + if [ "$FORCE" -eq 1 ]; then + echo "$(date -u +%FT%TZ) FORCED past the review-DISABLED kill switch (--force)" | _log + FORCED_KINDS+=(disabled) + FORCED_REASONS+=("review-DISABLED flag present") + else + echo "$(date -u +%FT%TZ) SKIP: review-DISABLED flag present" | _log + exit 0 + fi fi # The stale-setting guard, in the posture `usage-gate` already takes toward the retired @@ -155,6 +192,8 @@ if [ "$_ugrc" -eq 10 ]; then # log above, verbatim; the only difference is that this tick does not become a skip row. The # force lives INSIDE the exit-10 branch on purpose — a refusal (below) can never reach it. echo "$(date -u +%FT%TZ) FORCED past the usage-gate PAUSE (--force): $_ug" | _log + FORCED_KINDS+=(usage-gate) + FORCED_REASONS+=("$_ug") else # A paused tick still writes its metrics/runs.jsonl row (#160) — same shape and same reasoning # as campaign-run.sh: an empty trace so the record's shape still comes from `run-metrics`, the @@ -178,13 +217,21 @@ fi # --- the FORCE stamp every row this run writes carries (#245) ---------------------------------- # A forced run is not a paced tick, and a runs.jsonl row that cannot say so puts budget on the -# dashboard's run series against a schedule that was never followed. Built ONCE, here, from the -# gate's own line — including an OK line, because what makes a row forced is the human who typed -# `--force`, not what the gate happened to decide. Empty for a scheduled tick, which is what keeps -# every existing row byte-identical. +# dashboard's run series against a schedule that was never followed. Built ONCE, here, after the +# last stop a force can walk past, so it carries what this run ACTUALLY overrode rather than what a +# force is allowed to override. +# +# `--forced-run` is the fact that a human started this, and it is passed whenever `--force` was — +# INCLUDING when nothing was in the way, which is the ordinary case once the crons are running +# again. Each `--forced/--force-reason` pair is one stop that was actually walked past. So an empty +# stamp still says "not scheduled", and a consumer reading the kinds learns exactly which stops +# yielded. Absent entirely for a scheduled tick, which is what keeps every other row byte-identical. FORCED_FLAGS=() if [ "$FORCE" -eq 1 ]; then - FORCED_FLAGS=(--forced usage-gate --force-reason "$_ug") + FORCED_FLAGS=(--forced-run) + for _i in "${!FORCED_KINDS[@]}"; do + FORCED_FLAGS+=(--forced "${FORCED_KINDS[$_i]}" --force-reason "${FORCED_REASONS[$_i]}") + done fi # --- single-run lock (non-blocking) ---