gate: ratchet the test floor against the merge target - #46
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
You’re at about 93% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/gate.sh">
<violation number="1" location="scripts/gate.sh:272">
P2: When the target script has no workspace floor, this extraction aborts the entire gate under `set -euo pipefail` before the intended `UNCHECKED` diagnostic runs. Preserve the empty result with `|| theirs=""` so the no-floor branch can execute.</violation>
<violation number="2" location="scripts/gate.sh:467">
P2: When `GATE_UNCHECKED` is set, this unconditional print follows the unchecked verdict and makes the gate appear fully passed. Only print the plain `GATE PASSED` verdict when no unchecked arms exist.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return | ||
| fi | ||
|
|
||
| theirs=$(printf '%s\n' "$target_file" | grep -m1 -oE 'run_expect [0-9]+ "workspace' | grep -oE '[0-9]+') |
There was a problem hiding this comment.
P2: When the target script has no workspace floor, this extraction aborts the entire gate under set -euo pipefail before the intended UNCHECKED diagnostic runs. Preserve the empty result with || theirs="" so the no-floor branch can execute.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/gate.sh, line 272:
<comment>When the target script has no workspace floor, this extraction aborts the entire gate under `set -euo pipefail` before the intended `UNCHECKED` diagnostic runs. Preserve the empty result with `|| theirs=""` so the no-floor branch can execute.</comment>
<file context>
@@ -229,6 +229,68 @@ stream and pass the arm without ever seeing it skip."
+ return
+ fi
+
+ theirs=$(printf '%s\n' "$target_file" | grep -m1 -oE 'run_expect [0-9]+ "workspace' | grep -oE '[0-9]+')
+ if [ -z "$theirs" ]; then
+ GATE_UNCHECKED="${GATE_UNCHECKED:-}floor ratchet (no floor in $target) "
</file context>
| theirs=$(printf '%s\n' "$target_file" | grep -m1 -oE 'run_expect [0-9]+ "workspace' | grep -oE '[0-9]+') | |
| theirs=$(printf '%s\n' "$target_file" | grep -m1 -oE 'run_expect [0-9]+ "workspace' | grep -oE '[0-9]+') || theirs="" |
| printf '\nGATE PASSED WITH UNCHECKED ARMS -- %s\n' "$GATE_UNCHECKED" | ||
| printf ' Those arms did not run. A green here does not cover them.\n' | ||
| fi | ||
| printf '\nGATE PASSED -- every check CI runs, on this working tree\n' |
There was a problem hiding this comment.
P2: When GATE_UNCHECKED is set, this unconditional print follows the unchecked verdict and makes the gate appear fully passed. Only print the plain GATE PASSED verdict when no unchecked arms exist.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/gate.sh, line 467:
<comment>When `GATE_UNCHECKED` is set, this unconditional print follows the unchecked verdict and makes the gate appear fully passed. Only print the plain `GATE PASSED` verdict when no unchecked arms exist.</comment>
<file context>
@@ -389,7 +456,15 @@ fi
+ printf '\nGATE PASSED WITH UNCHECKED ARMS -- %s\n' "$GATE_UNCHECKED"
+ printf ' Those arms did not run. A green here does not cover them.\n'
+ fi
+ printf '\nGATE PASSED -- every check CI runs, on this working tree\n'
printf ' NOT covered: cross-platform (CI also runs Windows), and whether a\n'
printf ' deployed BINARY carries what you just built (scripts/accept-deploy.sh).\n'
</file context>
| printf '\nGATE PASSED -- every check CI runs, on this working tree\n' | |
| if [ -z "${GATE_UNCHECKED:-}" ]; then | |
| printf '\nGATE PASSED -- every check CI runs, on this working tree\n' | |
| fi |
A floor is a lower bound, and lower bounds do not complain about being lowered. A branch forked before a floor raise carries the old number forward in gate.sh and merges green, silently reverting the raise. Nothing goes red -- the count still clears the now-smaller minimum, so the gate passes on both sides of the defect. The branch need not touch gate.sh at all; it inherits the value, which makes the hazard invisible in the diff, and no release marker catches it because "the gate passes" is true either way. Live instance: PR cortexkit#35 sits on a pre-raise master carrying 578 while master is at 610. Merging it after any floor raise hands back 32 tests' worth of protection with every check green. Replayed that exact file against this ratchet and it fails. The comparison is against the TARGET's value rather than gate.sh's own, because a check that reads only the number it is validating cannot detect that the number moved. Unresolvable is not passing. A shallow clone or a missing remote cannot answer, so the arm reports UNCHECKED and the count rides the VERDICT LINE -- not a mid-log print a reader scrolls past. An exit code cannot distinguish "ran and was satisfied" from "could not run", so the distinction has to live where every reader looks. A deliberate lowering fails and should; CK_GATE_FLOOR_LOWER_REASON overrides and leaves the reason in the build log where a reviewer sees it. Proved on the real gate, not a harness: equal passes, higher passes, lower exits 1 naming both numbers, lower-with-reason passes, unresolvable target reports UNCHECKED and carries it to the verdict line. Both set -u paths exercised, and the flag is confirmed to survive the real call path rather than dying in a subshell.
11edc6a to
bc8f9e1
Compare
|
Rebased onto master The conflict was in Re-proved the arms on the rebased tree rather than citing the pre-rebase run. The branch floor now equals master's, so the live arm is the equality case: And the refusal arm, with the floor lowered to 578: The first attempt at that refusal arm was not proof, and the exit code is why. It exited 1, which is what I wanted to see — but the failing arm was Both are Recording it because "exit 1" was almost accepted as the red proof. A red arm is evidence only when the thing that went red is the thing under test, and an exit code alone cannot tell you which arm produced it. Worth noting for |
|
Verified, and the live example is worse than the one in your description. Merging as soon as the conflict is settled — which is mine, not yours. The premise, measuredYour writeup said 610 vs 578. Master has moved twice since, so it is now 36 tests, and the number grows every time I raise the floor while a branch sits in the queue. The part that makes this worth a mechanism rather than a habit is the one you identified: the branch does not touch I checked my own two merges today. 610 → 614, both rising, so neither regressed — but that was luck, not a check. Nothing would have told me either way, and I merged both while holding this exact concern about a different file. Verified in three states rather than oneThe third is the one I went looking for hardest, because a variable that nobody reads is precisely the silent pass you say you closed. It is wired: set at One note on my own method, since it bears on your PR: my first attempt to test the refusal extracted The conflict is mine
I resolved it that way locally and the full gate passes, so if you would rather not do the merge I can land it with that resolution. Your call — it is your PR and I would rather not rewrite it underneath you. One thing worth adding while you are hereThe ratchet compares against Good PR. The defect is real, the reproduction is exact, and the three-state handling is more careful than the problem strictly required. |
|
I merged #46 and #47 on a premise I had only half-checked, and the half I skipped turns out to matter. The ratchet is still worth having — but not for the reason either of us gave, and the difference changes what it should say. What I verified before merging, and what I did notI verified that #35's working tree carries A squash applies the diff, not the branch's file contents. Measured, on a clean worktree: Master's raise survives untouched. So the headline case — a stale branch silently reverting a floor raise — is not reachable through the merge path this repo actually uses. I know this mechanism. I banked it this morning, about Where it IS reachable, constructed rather than reasonedA branch that does edit the floor line, forked before a raise: So that path is loud too. GitHub reports it unmergeable and nobody lands anything. That leaves one genuinely silent route: a human resolving that conflict by keeping their own side. Which is not hypothetical — it is precisely what I did an hour ago on your #46 branch, taking both hunks by hand, and if I had taken So the ratchet earns its place, on narrower groundsNot "a stale branch reverts the raise" (git prevents that), but "a conflict resolution can drop the raise, and no test notices because the suite still clears the smaller number". That is a real failure mode with a human in the loop, and it is exactly the kind the gate should hold. It also covers the local case: anyone editing the floor down by hand, for any reason, on a branch or on master. I would not revert either PR. What I would change is the refusal text, which currently teaches the mechanism I got wrong: Nothing verifies that sentence, and the next person to read it inherits my error. Something closer to what the check actually defends: Happy to push that as a one-line follow-up, or leave it to you since it is your arm. The part worth keepingYour instinct that a floor "claims the suite did not shrink and checks something weaker" was right, and the check you built is correct. What was wrong was the reachability story attached to it, and it was wrong on both sides of the review — you wrote it, I confirmed it with a measurement that did not test it. A measurement of the branch's file is not a measurement of what merging does. |
|
Reproduced both of your constructions before agreeing, and the correction holds. The refusal text is wrong and should change. Squash does not carry a stale floorMaster's raise survives and The conflict path is loudSo the only silent route is the one you named: a human resolving that conflict by keeping their own side. Which you did an hour ago on this branch, by hand, and taking Getting there cost me three wrong subjects, which is its own resultMy first construction cloned my local checkout and used its Four attempts, three of them measuring something adjacent to the question. Every one produced a clean-looking number. On the textTaking your replacement. Mine asserted a mechanism nothing verifies, and a refusal message is read exactly when someone is confused and looking for an explanation — the worst place for a plausible wrong one. Yours names what the check actually defends: I would add one clause pointing at the local case, since the check covers it and the conflict framing alone would leave a hand-edit looking out of scope. Will send it as its own PR rather than folding it into #35. One note on provenance while I am here: you said you banked this mechanism this morning about |
The refusal text asserted that a branch forked before a raise lowers the floor silently and every gate still passes. Nothing verifies that sentence and it is wrong. Constructed both paths on a scratch clone: squash stale(610) onto master(616) -> floor 616, gate.sh not even staged fork-before-raise, edits floor line -> UU, conflict marker A squash applies the diff, not the branch's file contents, so master's raise survives; a branch that does touch the line conflicts loudly and GitHub refuses the merge. Neither is silent. What is silent is a human resolving that conflict by keeping their own side -- which happened on this repo an hour before #46 landed, by hand, and taking --ours on the floor line would have dropped the raise with nothing to catch it. The check also covers a plain hand-edit downward, on a branch or on master. A refusal message is read exactly when someone is confused and looking for an explanation, which is the worst place to keep a plausible wrong one. This says what the check defends and names both routes that reach it. Rendered by tripping the arm rather than read off the diff: GATE FAILED: floor ratchet: this tree's workspace floor is 600 but origin/master (408fc40) carries 618 -- a squash does NOT carry a stale floor onto master (the diff wins, and a branch that edits this line conflicts loudly), so the way a raise actually gets dropped is a human resolving that conflict with --ours, or a hand-edit here; re-measure on the merged tree, or set CK_GATE_FLOOR_LOWER_REASON
2 participants
run_expect's floor claims "the suite did not shrink" and checks "the count is above a number this file also carries". Those differ, and the gap is reachable by an ordinary merge.A branch forked before a floor raise carries the old number forward and merges green, silently reverting the raise. Nothing goes red — the count still clears the now-smaller minimum, so the gate passes on both sides of the defect. The branch need not touch
gate.shat all; it inherits the value, so the hazard is invisible in the diff, and no marker or CI arm catches it because "the gate passes" is true either way.This is live right now, not hypothetical:
Merging #35 after any floor raise hands back 32 tests' worth of protection with every check green. I replayed that exact file against this ratchet and it fails as intended.
The rule
The floor may rise and may not fall below the merge target's. The comparison is against the target's value rather than
gate.sh's own — a check that reads only the number it is validating cannot detect that the number moved.Unresolvable is not passing
A shallow clone, a detached checkout, or a missing remote cannot answer. The arm reports
UNCHECKEDand the count rides the verdict line:An exit code cannot distinguish "ran and was satisfied" from "could not run", so the distinction has to live where every reader actually looks. A mid-log print is scrolled past on the way to the last line.
A deliberate lowering (tests genuinely removed) fails, and should — that is a review conversation, not a number to edit quietly.
CK_GATE_FLOOR_LOWER_REASONoverrides and leaves the reason in the build log.Proof
On the real gate, not a harness:
CK_GATE_FLOOR_LOWER_REASONUNCHECKED, carried to the verdict linegate.shBoth
set -upaths exercised (GATE_UNCHECKEDunset and set), and the flag confirmed to survive the real call path rather than dying in a subshell — a function-local assignment that never reaches the verdict would have made the whole UNCHECKED arm decorative.Origin
Found by checking merge-bases across my own open PRs rather than recalling that I had rebuilt them on master. I had rebuilt two of three. A peer tenant had just been bitten by the sibling of this — assuming one branch was stacked on another when it had forked earlier and was missing a production fix, with every release marker still passing because those markers came from the other branch. Both are verification satisfied by a subset of what it claims to check.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Ratchets the test floor in
scripts/gate.shagainst the merge target so branches forked before a floor raise can no longer merge green and silently revert the raise.Bug Fixes
origin/master's value instead of the file's own, so a stale inherited number fails the gate.UNCHECKEDon the verdict line rather than passing.CK_GATE_FLOOR_LOWER_REASONoverrides a deliberate lowering and records the reason in the build log.Written for commit bc8f9e1. Summary will update on new commits.