fix(runners): --force overrides policy stops, not correctness stops - #248
Conversation
Reverses the kill-switch half of #245 on the user's ruling: "force needs to force". The first observation run the feature exists for printed `SKIP: DISABLED flag present` and did nothing. The original reasoning — a force that walks through the kill switch turns the one unambiguous off-switch into a suggestion — was wrong about who the switch protects against. DISABLED stops the CRON. A human typing --force at a terminal is the switch's own owner deliberately overriding their own stop, and refusing them is friction and nothing else. A scheduled tick still honours the switch exactly as before, which is the switch's whole job. The rule is now stated as ONE PROPERTY in both runners and the README, not as a list of exceptions — the list is what made this wrong the first time: --force overrides POLICY stops. It never overrides CORRECTNESS stops. POLICY (yields, and records that it yielded): the usage-gate PAUSE, and the kill switch. CORRECTNESS (never yields): the flock, because two runs corrupt each other's clones and GitHub state, and a gate config REFUSAL, because the tick would run on config nobody validated. The marker becomes PARALLEL ARRAYS, because one forced run can now walk past two stops: `forced` holds each stop's kind and `forceReason` holds that stop's own line, verbatim, paired by position. `run-metrics` refuses a mismatched pair rather than truncating. `--forced-run` carries the fact a human started the run, on its own, so a force that met NO stop still marks its row `"forced": []` — that is the ordinary case, and keying the marker on "something was overridden" would draw it as a paced tick. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both were real gaps found by the mutation pass, not hypotheticals:
* dropping `requires = "forced_run"` from `--forced` survived, because the
only assertion covering it passed BOTH flags — and `--force-reason`'s own
requirement refused that pair on its own. `--forced` alone is now asserted
separately.
* the positional length check had no test at all, so truncating a mismatched
pair — a stop silently named against another stop's line — passed
everything. Driven through `run_metrics_mode` with 2 kinds and 1 reason,
with the matched pair beside it so the guard is the LENGTHS.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Walkthrough
ChangesForced execution and metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Operator
participant campaign-run.sh
participant usage-gate
participant pr-review-report
Operator->>campaign-run.sh: Invoke --force
campaign-run.sh->>usage-gate: Check usage gate
usage-gate-->>campaign-run.sh: Pause or success
campaign-run.sh->>campaign-run.sh: Record overridden policy stops
campaign-run.sh->>pr-review-report: Submit forced metadata
pr-review-report-->>Operator: Stream run output and write JSON row
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
3040-3063: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRoute the documented force operation through
pr-review-report.Lines 3048-3049 provide direct runnable
campaign-run/review-runcommands. Shipped runnable fenced lines must go throughpr-review-report; prose may mention raw commands only to forbid them. Add or extend apr-review-reportsubcommand for the two permitted runner entry points, update the fenced examples, and update the related prose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 3040 - 3063, The documented force-run examples currently invoke campaign-run and review-run directly; route both permitted operations through the pr-review-report subcommand instead. Add or extend pr-review-report to dispatch these two runner entry points with --force, update the fenced commands and related prose to describe that wrapper, and retain raw runner commands only when explicitly documenting them as forbidden.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@README.md`:
- Around line 3040-3063: The documented force-run examples currently invoke
campaign-run and review-run directly; route both permitted operations through
the pr-review-report subcommand instead. Add or extend pr-review-report to
dispatch these two runner entry points with --force, update the fenced commands
and related prose to describe that wrapper, and retain raw runner commands only
when explicitly documenting them as forbidden.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cba41174-298b-4777-bfdd-8a58450db252
📒 Files selected for processing (5)
README.mdcampaign-run.shpr-review-report-rs/src/main.rspr-review-report-rs/tests/usage_gate_skip.rsreview-run.sh
|
Reviewed ab4c751: pass Verified on the head blob, both runners:
Rulings-conformance: checked against CLAUDE.md's rulings section and every ruling stated for this work.
Note on ordering: rainlanguage/rain-org-health#171 carries the dashboard side of the array marker and is not merged yet. Until it lands a forced row renders as "forced past the gate" rather than naming each stop — a graceful degrade, not a break, since CI: all checks pass; the single non-pass is |
Refs #245
This reverses the kill-switch decision made in #246, on the user's ruling. The first observation run that feature exists for printed
SKIP: DISABLED flag presentand exited 0 without running. The ruling:246's stated reasoning — "a force that walked through it would turn the one unambiguous off-switch into a suggestion" — was wrong about who the switch protects against, and it was wrong in a specific way worth naming: it came from writing a LIST of things not to bypass. A list invites adding one more item that sounds right in isolation. So the list is gone.
The rule, as one property
--force?DISABLED/review-DISABLED--forceis its own owner overriding their own stopThat property is now what the comments in both runners and the README section say, and it is what the test module's own header says. Adding a stop means deciding which side it is on, once.
A scheduled tick still honours the kill switch exactly as before — the switch's whole job is stopping the cron, and the cron passes no argument. That is a named test per role, not an implication.
The marker shape
One forced run can now walk past two stops, so a single value cannot carry it.
forcedandforceReasonbecome parallel arrays — same pair, same meaning, now plural:Index i of each is one stop: its kind, and that stop's own line verbatim.
run-metricsrefuses a mismatched pair rather than truncating — a stop silently paired with the wrong line is well-formed, wrong, and invisible until someone reads the dashboard weeks later."forced": []is a real state, and it is the ordinary one. A human starts a run 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 — so--forced-runrecords the human, and the arrays record what they had to override. Keying the marker on "something was overridden" would draw exactly that run as a paced tick, which is the corruption the field exists to prevent.Dashboard: rainlanguage/rain-org-health#169 is merged and reads this field. The shape changed, so it has a matching PR: rainlanguage/rain-org-health#171, which renders every overridden stop with its own line and handles both the empty list and 169's single-value rows. No forced row exists in live data yet, so nothing renders wrong in the meantime.
QA
force_does_not_bypass_the_{producer,vetter}_kill_switch→force_overrides_the_{producer,vetter}_kill_switch, now asserting the run proceeds, both stops land in the marker in the order met, each with its own line, and the log saysFORCED pastwith noSKIP:. New —a_scheduled_{producer,vetter}_tick_still_honours_the_kill_switch(the half that must not regress: exit 0, no row,SKIP:, noFORCED), anda_forced_{producer,vetter}_run_that_met_no_stop_is_still_marked("forced": [], present not absent). Rust —skip_row_tests::a_forced_row_carries_every_stop_it_ran_past_and_each_stops_line_verbatim,skip_row_tests::a_positional_pair_of_different_lengths_is_refused,cli_tests::run_metrics_trace. Fails on base: implementation reverted to the merged scripts (git checkout main -- campaign-run.sh review-run.sh), tests kept — 6 of 24 fail: the twoforce_overrides_*, the two*_met_no_stop_*, and botha_forced_*_run_past_a_pause_runs_marked_and_streamed(the marker is arrays now). The twoa_scheduled_*_still_honours_*tests pass on base, which is exactly right — that is the behaviour being preserved, and mutant N2 is what makes them load-bearing.force_overrides_*. N2 kill switch yields to a scheduled tick →a_scheduled_*_still_honours_*+the_*_kill_switch_outranks_the_force_variable. N3/N4 an override goes unrecorded →force_overrides_*/a_forced_*_past_a_pause_*. N5 reason is the kind, not the stop's line →a_forced_*_past_a_pause_*. N6 a forced run that met no stop is left unmarked →a_forced_*_met_no_stop_*. N7 only the first override recorded (single value, not a list) →force_overrides_*. N8 flock made overridable →force_does_not_bypass_the_*_lock. N9 gate refusal made overridable →force_does_not_bypass_a_*_gate_refusal. N10CRON_FORCErefusal dropped →a_*_force_variable_is_refused_*. N11 positional length check removed →a_positional_pair_of_different_lengths_is_refused. N12 reasons dropped from the stamp →a_forced_row_carries_every_stop_*. N13--forced-runno longer conflicts with--skipped→cli_tests::run_metrics_trace. N14--forcedno longer requires--forced-run→cli_tests::run_metrics_trace. N11 and N14 survived the first pass and are the reason two tests exist: N14 survived because the only assertion covering it passed both flags, and--force-reason's own requirement refused that pair on its own, so--forcedalone is now asserted separately; N11 survived because the length check had no test at all.skipped/skipReasoncontract it mirrors; the empty case comes from the ordinary forced run, not from the blocked one.--forcepast the kill switch, logged in the same shape as the gate override; (b) a scheduled tick unchanged; (c) the two inverted tests renamed to state the new property; (d)the_*_kill_switch_outranks_the_force_variablekept meaningful; (e) flock and refusal tests untouched and not weakened; (f) a marker recording what was overridden; (g) the rule stated as a property in comments and README. Covered a–g. (d) holds and is tested: a staleCRON_FORCEis the absent human repeating for ever, the opposite of a human at a terminal, so the switch still outranks it — that test passes unchanged and N2 kills any code that would break it. (e) verified: the flock and refusal tests are byte-identical, and N8/N9 fail without them.RefsnotCloses: Force a one-off manual producer/vetter run past the usage gate, for observation #245 was closed by feat(runners): --force one manual run past the usage-gate pause #246; this amends that work.Suite: 1087 unit + 24 gate + the rest, all green.
shellcheck --external-sourcesclean on both runners. CI's ownpre-commit run --all-files(rainix pinned bundle) passes every hook.