Conversation
|
CI on this PR is red, and it is red for the right reason. Recording it rather than working around it. GitHub sets That is the gate doing its job. It refuses to report PASS when the comparison was never attempted, which is the property the whole change exists to establish. #234 merges first, then this goes green on a re-run. No code change is needed here and none should be made — the fix for "the gate cannot compare" is to give it something to compare against, not to teach it to pass anyway. Worth stating plainly because it is the honest correction to my own PR description: I wrote that splitting the landing means "the base already has floors, so the ratchet works on its first run". That is true only after #234 lands. Until then this PR is correctly blocked, and the split still does what it was for — there is no bootstrap exemption anywhere in the code, and the condition resolves by merging in order rather than by an authorized bypass. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 4 files
Confidence score: 3/5
- In
scripts/check-test-count-floors.ts, the COUNT path compares against the branch floor when it exceeds the merge-target floor, so counts that satisfy the intended merge-target gate can be rejected; compare COUNT results withtargetFloorswhile retaining the separate ratchet and lowering checks. - In
.github/workflows/ci.yml, exit code 2 (UNCHECKED) is treated like exit code 1 (FAIL) because the step lackscontinue-on-error, so base-ref or stamp validation failures can fail the entire check job instead of being reported distinctly; handle the two verdicts separately in the workflow. - In
scripts/check-test-count-floors.ts, zero-test measurements are rejected before COUNT evaluation, preventing valid non-negative counts from being checked; allow zero for measured counts while keeping floor-file values strictly positive, and add coverage for that path inscripts/check-test-count-floors.test.ts. - Early verdicts in
scripts/check-test-count-floors.tsreportpackages=core,opencode,pieven when no counts were evaluated, and the related test case does not actually exercise an empty scope becauseparseFloors({})fails first; derive pre-measurement scope from evaluated counts (nonewhen empty) and rename the test to cover invalid input explicitly.
You’re at about 94% 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/check-test-count-floors.test.ts">
<violation number="1" location="scripts/check-test-count-floors.test.ts:258">
P2: This input never exercises the empty-scope case: `parseFloors` rejects `{}` before evaluation, and the invalid-input handler happens to satisfy the same `FAIL packages=none` substring. Rename this as an invalid-input test and assert the rejection detail, or provide a valid way to produce an empty scope.</violation>
</file>
<file name="scripts/check-test-count-floors.ts">
<violation number="1" location="scripts/check-test-count-floors.ts:66">
P3: When a package has zero measured tests, `--counts` rejects the valid measurement before COUNT runs. Allow non-negative values for measured counts while keeping floor-file values strictly positive.</violation>
<violation number="2" location="scripts/check-test-count-floors.ts:309">
P2: When base-ref or stamp validation exits before measurement, UNCHECKED reports `packages=core,opencode,pi` even though no counts were evaluated. Derive every early verdict scope from evaluated counts, using `none` before measurement.</violation>
<violation number="3" location="scripts/check-test-count-floors.ts:373">
P2: When the branch floor exceeds the merge-target floor, this COUNT arm rejects counts that satisfy the merge-target gate. Compare counts with `targetFloors` as specified, while retaining the separate ratchet and lowering handling.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:25">
P2: The gate distinguishes UNCHECKED (exit 2) from FAIL (exit 1), but this step has no `continue-on-error`, so both exit codes fail the check job identically. Whenever `${{ github.event.pull_request.base.sha }}` cannot be resolved locally (e.g., base branch force-pushed between the event and checkout, or the floor file not present at `base.sha`), the script prints VERDICT: UNCHECKED and exits 2, turning the whole CI job red and blocking the merge just like a real floor violation — with no distinguishing status.</violation>
</file>
Architecture diagram
sequenceDiagram
participant CI as CI Workflow
participant Gate as check-test-count-floors.ts
participant Git as Git Repository
participant FloorFile as test-count-floors.json
participant Marker as lowering marker file
participant Tests as Package Test Suites
Note over CI,Tests: Test-Count Floor Gate with Merge-Target Ratchet
CI->>Git: fetch-depth: 0 (full clone)
CI->>Gate: bun run check-test-count-floors.ts --base-ref ${{ github.event.pull_request.base.sha }}
Gate->>FloorFile: Read branch floor file
alt Branch floor file missing
Gate-->>CI: FAIL packages=none (NONCOMPLIANT SOURCE, exit 1)
else Branch floor file exists
Gate->>Gate: Parse & validate floor file
alt No --base-ref provided
Gate-->>CI: UNCHECKED (merge target ref not provided, exit 2)
else --base-ref provided
Gate->>Git: git show {baseRef}:.ci/test-count-floors.json
alt Merge target floor unresolvable
Gate-->>CI: UNCHECKED (could not resolve merge target, exit 2)
else Merge target floor resolved
Gate->>Gate: Parse & validate merge target floors
Gate->>Gate: Validate measurement stamp (ancestor check via git merge-base)
alt Measurement head not an ancestor
Gate-->>CI: UNCHECKED (stale/foreign measurement, exit 2)
else Measurement valid
Gate->>Tests: Measure test counts (core, opencode, pi)
Tests-->>Gate: Measured counts per package
alt Measured count < branch floor
Gate-->>CI: FAIL (count violation, exit 1)
else Counts satisfy branch floors
Gate->>Gate: Compare branch floor vs merge target floor
alt Branch floor < merge target floor (ratchet violation)
Gate->>Marker: Check for lowering marker file
alt Marker missing or invalid
Gate-->>CI: FAIL (ratchet violation, exit 1)
else Marker valid with matching from/to
Gate-->>CI: PASS (deliberate lowering authorized, exit 0)
end
else Branch floor >= merge target floor
Gate-->>CI: PASS (test counts and floor ratchet satisfied, exit 0)
end
end
end
end
end
end
Note over Gate,CI: Verdict includes evaluated package scope:
Note over Gate,CI: PASS/FAIL/UNCHECKED packages=core,opencode,pi or packages=none
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| test('does not pass when no packages are evaluated', async () => { | ||
| const floors = { core: 10, opencode: 20, pi: 30 } | ||
| const cwd = await makeStaleBranch(floors, floors) | ||
| const result = runGateWithCounts(cwd, '{}') |
There was a problem hiding this comment.
P2: This input never exercises the empty-scope case: parseFloors rejects {} before evaluation, and the invalid-input handler happens to satisfy the same FAIL packages=none substring. Rename this as an invalid-input test and assert the rejection detail, or provide a valid way to produce an empty scope.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/check-test-count-floors.test.ts, line 258:
<comment>This input never exercises the empty-scope case: `parseFloors` rejects `{}` before evaluation, and the invalid-input handler happens to satisfy the same `FAIL packages=none` substring. Rename this as an invalid-input test and assert the rejection detail, or provide a valid way to produce an empty scope.</comment>
<file context>
@@ -0,0 +1,281 @@
+test('does not pass when no packages are evaluated', async () => {
+ const floors = { core: 10, opencode: 20, pi: 30 }
+ const cwd = await makeStaleBranch(floors, floors)
+ const result = runGateWithCounts(cwd, '{}')
+
+ expect(result.exitCode).toBe(1)
</file context>
| console.error( | ||
| verdict( | ||
| 'UNCHECKED', | ||
| packageScope(branchDocument.floors), |
There was a problem hiding this comment.
P2: When base-ref or stamp validation exits before measurement, UNCHECKED reports packages=core,opencode,pi even though no counts were evaluated. Derive every early verdict scope from evaluated counts, using none before measurement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/check-test-count-floors.ts, line 309:
<comment>When base-ref or stamp validation exits before measurement, UNCHECKED reports `packages=core,opencode,pi` even though no counts were evaluated. Derive every early verdict scope from evaluated counts, using `none` before measurement.</comment>
<file context>
@@ -0,0 +1,426 @@
+ console.error(
+ verdict(
+ 'UNCHECKED',
+ packageScope(branchDocument.floors),
+ 'merge target ref was not provided',
+ ),
</file context>
| const targetFloors = targetDocument.floors | ||
| const failures: string[] = [] | ||
| for (const packageName of packageNames) { | ||
| if (counts[packageName] < branchFloors[packageName]) { |
There was a problem hiding this comment.
P2: When the branch floor exceeds the merge-target floor, this COUNT arm rejects counts that satisfy the merge-target gate. Compare counts with targetFloors as specified, while retaining the separate ratchet and lowering handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/check-test-count-floors.ts, line 373:
<comment>When the branch floor exceeds the merge-target floor, this COUNT arm rejects counts that satisfy the merge-target gate. Compare counts with `targetFloors` as specified, while retaining the separate ratchet and lowering handling.</comment>
<file context>
@@ -0,0 +1,426 @@
+ const targetFloors = targetDocument.floors
+ const failures: string[] = []
+ for (const packageName of packageNames) {
+ if (counts[packageName] < branchFloors[packageName]) {
+ failures.push(
+ `COUNT: ${packageName} measured ${counts[packageName]} < branch floor ${branchFloors[packageName]}`,
</file context>
| - run: bun run types | ||
| - run: bun run build | ||
| - run: bun test scripts/check-test-count-floors.test.ts | ||
| - run: bun scripts/check-test-count-floors.ts --base-ref "${{ github.event.pull_request.base.sha }}" |
There was a problem hiding this comment.
P2: The gate distinguishes UNCHECKED (exit 2) from FAIL (exit 1), but this step has no continue-on-error, so both exit codes fail the check job identically. Whenever ${{ github.event.pull_request.base.sha }} cannot be resolved locally (e.g., base branch force-pushed between the event and checkout, or the floor file not present at base.sha), the script prints VERDICT: UNCHECKED and exits 2, turning the whole CI job red and blocking the merge just like a real floor violation — with no distinguishing status.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci.yml, line 25:
<comment>The gate distinguishes UNCHECKED (exit 2) from FAIL (exit 1), but this step has no `continue-on-error`, so both exit codes fail the check job identically. Whenever `${{ github.event.pull_request.base.sha }}` cannot be resolved locally (e.g., base branch force-pushed between the event and checkout, or the floor file not present at `base.sha`), the script prints VERDICT: UNCHECKED and exits 2, turning the whole CI job red and blocking the merge just like a real floor violation — with no distinguishing status.</comment>
<file context>
@@ -15,10 +15,14 @@ jobs:
- run: bun run types
- run: bun run build
+ - run: bun test scripts/check-test-count-floors.test.ts
+ - run: bun scripts/check-test-count-floors.ts --base-ref "${{ github.event.pull_request.base.sha }}"
- run: bun run --cwd packages/opencode smoke:tui
env:
</file context>
| if ( | ||
| typeof value !== 'number' || | ||
| !Number.isSafeInteger(value) || | ||
| value <= 0 |
There was a problem hiding this comment.
P3: When a package has zero measured tests, --counts rejects the valid measurement before COUNT runs. Allow non-negative values for measured counts while keeping floor-file values strictly positive.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/check-test-count-floors.ts, line 66:
<comment>When a package has zero measured tests, `--counts` rejects the valid measurement before COUNT runs. Allow non-negative values for measured counts while keeping floor-file values strictly positive.</comment>
<file context>
@@ -0,0 +1,426 @@
+ if (
+ typeof value !== 'number' ||
+ !Number.isSafeInteger(value) ||
+ value <= 0
+ ) {
+ fail(`${source}.${packageName} must be a positive integer`)
</file context>
|
Marked draft until #234 lands, because a red check here reads as "broken" when the accurate signal is "not mergeable yet". The red is the gate refusing to report PASS on a comparison it could not make — #234 is green and independent. Once it merges I will re-run this and un-draft. |
8fb753b to
fd004ad
Compare
1 participant
Stacked on #234 — merge that first, or this compares against a base with no floors and reports UNCHECKED.
What this closes
We have no test-count floor at all. The
checkgate isbuild && core && opencode && pi, so a suite can shrink to a single test and stay green.That is not hypothetical. In August, 153 tests in
packages/corewere stranded — present, correct, and executed by no gate (#171). It was caught because a test total rose by fewer than the tests added and the arithmetic did not work. Nothing mechanical would catch a recurrence.Three enforcements, kept distinguishable
The ratchet is the part that is easy to get wrong. A bare number bumped by whoever measures is carryable backward by a stale branch: a branch forked before a bump merges green and lowers the bound, and nothing goes red, because a floor is a lower bound and lower bounds do not complain about being lowered. So the gate compares against the merge target's value rather than its own.
fetch-depth: 0is load-bearing — a shallow clone cannot resolve the merge target's floor, and without it the UNCHECKED arm would be the only arm that ever fired.Three verdict states, not two
Missing branch config is a noncompliant source contract → FAIL, rebase and adopt. Inability to inspect the merge target → UNCHECKED. Collapsing those would make UNCHECKED a catch-all that quietly absorbs a real violation.
One defect found by asking what
github.event.pull_request.base.shadoes on a non-PR event:if (!value || …)treated a quoted--base-ref ""as a missing flag and crashed as FAIL/1 rather than UNCHECKED/2. Every direct push would have hard-failed the gate — and failed as a violation verdict, asserting floors were breached when the comparison was never attempted. A gate that cries violation when it cannot run gets disabled. The suite was green with that defect present, because no arm passed an empty flag, only a missing one. Now pinned.Verdicts carry their scope
The package set is derived from the counts actually evaluated, not a hardcoded list. An empty scope renders
packages=noneand never as a pass. A green that does not state its scope can be read as covering more than it does — if a package were dropped from the checked set tomorrow, a scope-free verdict line would look identical.Proof arms
Arm (g) is the one that matters: floors bumped on main, a branch forked before the bump carrying the old numbers. Without it the other arms only prove the script runs.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds a test-count floor gate with a merge-target ratchet so a test suite can no longer shrink to a single test and stay green. It records initial floors for
core,opencode, andpiand measures counts itself by running each package's tests.Behavior
fetch-depth: 0so the merge target's floor can be resolved.Written for commit fd004ad. Summary will update on new commits.