test(gc-ratchet): fail open per cell and re-pin the baseline (#7554) - #7609
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe GC ratchet now collects artifact, probe, and cell defects. Structural validation can defer non-fatal defects until measurement and final checking. Evaluation demotes affected rows while continuing unaffected measurements. Strict validation remains the default. ChangesGC ratchet validation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant main
participant inspect_artifact
participant evaluate
participant Report
CI->>main: validate --scope structural
main->>inspect_artifact: inspect artifact
inspect_artifact-->>main: return scoped defects
main->>evaluate: run measurements
evaluate->>Report: record failures and unfit rows
CI->>main: run check
main-->>CI: final ratchet verdict
Possibly related issues
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
tests/test_gc_ratchet.py (1)
908-920: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the negative half of "only that probe".
The test asserts that
01_proberows are non-gating and that some02_otherfailure exists. It does not assert that02_otherrows stayed gating, and it does not assert that the02_otherfailure is the planted regression rather than any other message. Both additions keep the test honest if the demotion set is ever computed too widely.💚 Proposed strengthening
rows, failures = evaluate(baseline, current, profile="shared_ci") for row in rows: if row.probe == "01_probe": self.assertFalse(row.gating, f"{row.metric} on an unfit probe must be demoted") + self.assertTrue( + any(row.gating for row in rows if row.probe == "02_other"), + "the fit probe must keep its gating cells", + ) self.assertTrue( - any("02_other" in failure for failure in _hard(failures)), + any("02_other: copied_objects" in failure for failure in _hard(failures)), "the fit probe must still be able to fail the job", )🤖 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 `@tests/test_gc_ratchet.py` around lines 908 - 920, Strengthen test_an_unfit_probe_demotes_only_that_probe by asserting that rows for the fit probe 02_other remain gating, and verify the hard failure specifically identifies the planted copied_objects regression rather than merely containing the probe name. Keep the existing assertions that 01_probe is demoted and that the fit probe can fail the job.benchmarks/gc_ratchet/gc_ratchet.py (5)
1490-1502: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe heading names cells, but the list also contains probe-scoped defects.
unfitholds every non-fatal defect, which includesprobescope. A probe pinned without an oracle diff appears under "Pinned cells that could not be gated". Name both scopes in the heading.Separately,
renderre-derives the defect list thatevaluatealready computed for the same run. Passing the list fromevaluateintorenderwould remove the implicit coupling, at the cost of a signature change thattests/test_gc_ratchet.pyLine 548 also uses.♻️ Proposed heading fix
- "### Pinned cells that could not be gated (baseline defects)", + "### Pinned probes and cells that could not be gated (baseline defects)",🤖 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 `@benchmarks/gc_ratchet/gc_ratchet.py` around lines 1490 - 1502, Update the baseline-defect heading in render to mention both pinned cells and probes, since unfit includes non-fatal defects from either scope. Also pass the defect list already computed by evaluate into render, updating the render signature and its call site in tests/test_gc_ratchet.py, instead of re-deriving defects for the same run.
1269-1271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
describe()so pin-time errors name the defective cell.
messagefor acelldefect does not contain the probe or metric name; the location lives only indefect.where.assemblereports this exception to a maintainer, so the current text readsspread 6768 when pinned, ...with no indication of which of the 144 cells failed.cmd_validatealready printsdescribe()for the same defects, so this is also an inconsistency between the two strict paths.♻️ Proposed change
defects = inspect_artifact(artifact) if defects: - raise RatchetError("; ".join(defect.message for defect in defects)) + raise RatchetError("; ".join(defect.describe() for defect in defects))
tests/test_gc_ratchet.py::test_pinning_an_unfit_artifact_is_still_refusedasserts on"bit-identity", whichdescribe()still contains.🤖 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 `@benchmarks/gc_ratchet/gc_ratchet.py` around lines 1269 - 1271, Update the defect aggregation in the artifact pinning path to use each defect’s describe() output instead of defect.message when raising RatchetError. Preserve the existing semicolon-separated formatting so errors identify the defective cell’s probe or metric, matching cmd_validate behavior.
1678-1691: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the "unfit cell(s)" wording and the trailing success line.
Two accuracy problems in the operator-facing output:
unfitcontains bothcell- andprobe-scoped defects, so "unfit cell(s)" mislabels a probe pinned without an oracle diff. Lines 1680 and 1687 both use it.- Under
--scope structuralwith deferred defects, Line 1691 printsis structurally validdirectly after the deferral warning. The two lines read as contradictory in a CI log.♻️ Proposed wording fix
if unfit and args.scope == "all": print( - f"gc-ratchet: {args.artifact} has {len(unfit)} unfit cell(s); " + f"gc-ratchet: {args.artifact} has {len(unfit)} unfit probe(s)/cell(s); " "re-pin them or record a probe_overrides entry", file=sys.stderr, ) return 1 if unfit: print( - f"gc-ratchet: {len(unfit)} unfit cell(s) deferred to `check` " + f"gc-ratchet: {len(unfit)} unfit probe(s)/cell(s) deferred to `check` " "(--scope structural); they will fail the job there, after the probes run", file=sys.stderr, ) - print(f"gc-ratchet: {args.artifact} is structurally valid") + print(f"gc-ratchet: {args.artifact} is structurally valid; `check` will fail on the deferred defects") + else: + print(f"gc-ratchet: {args.artifact} is structurally valid")🤖 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 `@benchmarks/gc_ratchet/gc_ratchet.py` around lines 1678 - 1691, Update the operator-facing messages in the unfit handling flow to refer to generic unfit item(s) rather than unfit cell(s), covering both the all-scope error and structural-scope deferral warning. Suppress or adjust the final success message when unfit defects are deferred under structural scope so it does not claim the artifact is structurally valid; retain the success line for genuinely valid results.
1429-1434: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDistinguish the two quarantined statuses by words, not by case.
UNFIT (pinned cell unusable)andunfit (pinned cell unusable)differ only in letter case. In a 144-row table, that is the only signal that a demoted cell also breached its band. A distinct wording makes the breach visible.♻️ Optional wording change
if breach and quarantined: - status = "UNFIT (pinned cell unusable)" + status = "UNFIT + breached (pinned cell unusable)" elif breach: status = "REGRESSION" if tolerance.gating else "drift (informational)" elif quarantined: - status = "unfit (pinned cell unusable)" + status = "unfit (pinned cell unusable)"If you change the first label, keep the substring
unfitout of it or updatetests/test_gc_ratchet.pyLine 888, which assertsassertIn("unfit", row.status).🤖 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 `@benchmarks/gc_ratchet/gc_ratchet.py` around lines 1429 - 1434, Update the status values in the breach/quarantined branching near status assignment so the case where both breach and quarantined are true uses wording distinct from the quarantined-only status by more than capitalization. Preserve the existing status semantics and ensure the revised breach-and-quarantined label does not contain the substring “unfit” unless the related test expectation is updated.
196-202: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider binding
scopetoSCOPES.
scopeis typed asstr, andSCOPESis declared but never used for validation or typing. A typo such as"cells"would produce a silently non-fatal defect withwhere == "artifact". All current constructors are internal, so this is only a robustness gap.♻️ Optional: type the field against the declared scopes
- scope: str + scope: Literal["cell", "probe", "artifact"] message: str probe: str | None = None metric: str | None = NoneThis requires
from typing import Literalin the import block.🤖 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 `@benchmarks/gc_ratchet/gc_ratchet.py` around lines 196 - 202, Bind the scope field in the relevant configuration or result class to the declared SCOPES values, preferably by typing it with a Literal derived from "cell", "probe", and "artifact" and importing Literal as needed. Ensure invalid scope values cannot silently fall through to the artifact behavior, while preserving the existing SCOPES ordering.
🤖 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.
Inline comments:
In @.github/workflows/gc-ratchet.yml:
- Around line 91-101: Add the gc-ratchet workflow’s check context to the
repository’s default-branch required status checks alongside the workflow
change, ensuring the deferred defect verdict can block merges. Update the
branch-protection configuration or automation that defines required contexts,
using the existing gc-ratchet status-check name and preserving the gate’s
non-canceling, non-continue-on-error behavior.
In `@benchmarks/gc_ratchet/gc_ratchet.py`:
- Around line 1678-1691: Update operator-facing labels in
benchmarks/gc_ratchet/gc_ratchet.py: in the messages near lines 1678-1691,
replace “unfit cell(s)” with “unfit probe(s)/cell(s)” in both the --scope all
and deferral paths; also update the report heading near lines 1490-1502 from
“Pinned cells that could not be gated” to name both probes and cells.
- Around line 1186-1212: Update inspect_artifact’s probe and metric validation
to require each probe entry and each metric entry to be a Mapping before calling
get or accessing its contents. Report invalid entries through artifact_defect,
mark the artifact unreadable, and continue classification so malformed artifacts
produce the existing fatal artifact result and RatchetError CLI handling instead
of raising AttributeError.
In `@benchmarks/gc_ratchet/README.md`:
- Around line 291-307: The README’s new artifact-validation section conflicts
with existing incident details and implementation behavior. Update the outage
duration to match the two-day account, revise the bit-identity rule reference
from validate_artifact to inspect_artifact, and clarify the cell scope row to
state that defects apply only when gated_anywhere(...) is true, excluding cells
disabled by probe_overrides.
---
Nitpick comments:
In `@benchmarks/gc_ratchet/gc_ratchet.py`:
- Around line 1490-1502: Update the baseline-defect heading in render to mention
both pinned cells and probes, since unfit includes non-fatal defects from either
scope. Also pass the defect list already computed by evaluate into render,
updating the render signature and its call site in tests/test_gc_ratchet.py,
instead of re-deriving defects for the same run.
- Around line 1269-1271: Update the defect aggregation in the artifact pinning
path to use each defect’s describe() output instead of defect.message when
raising RatchetError. Preserve the existing semicolon-separated formatting so
errors identify the defective cell’s probe or metric, matching cmd_validate
behavior.
- Around line 1678-1691: Update the operator-facing messages in the unfit
handling flow to refer to generic unfit item(s) rather than unfit cell(s),
covering both the all-scope error and structural-scope deferral warning.
Suppress or adjust the final success message when unfit defects are deferred
under structural scope so it does not claim the artifact is structurally valid;
retain the success line for genuinely valid results.
- Around line 1429-1434: Update the status values in the breach/quarantined
branching near status assignment so the case where both breach and quarantined
are true uses wording distinct from the quarantined-only status by more than
capitalization. Preserve the existing status semantics and ensure the revised
breach-and-quarantined label does not contain the substring “unfit” unless the
related test expectation is updated.
- Around line 196-202: Bind the scope field in the relevant configuration or
result class to the declared SCOPES values, preferably by typing it with a
Literal derived from "cell", "probe", and "artifact" and importing Literal as
needed. Ensure invalid scope values cannot silently fall through to the artifact
behavior, while preserving the existing SCOPES ordering.
In `@tests/test_gc_ratchet.py`:
- Around line 908-920: Strengthen test_an_unfit_probe_demotes_only_that_probe by
asserting that rows for the fit probe 02_other remain gating, and verify the
hard failure specifically identifies the planted copied_objects regression
rather than merely containing the probe name. Keep the existing assertions that
01_probe is demoted and that the fit probe can fail the job.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cca1b93e-fdc6-4bc7-a0af-816e11720ee2
📒 Files selected for processing (5)
.github/workflows/gc-ratchet.ymlbenchmarks/gc_ratchet/README.mdbenchmarks/gc_ratchet/baseline/gc-ratchet-v1.jsonbenchmarks/gc_ratchet/gc_ratchet.pytests/test_gc_ratchet.py
| # Under `structural` a per-cell defect is printed loudly and deferred; the | ||
| # `check` step below re-derives the same defect list and fails on it, after | ||
| # the probes have run and with the full table attached. So this cannot | ||
| # wave a defect through to a green job — `check` is where the verdict is, | ||
| # and tests/test_gc_ratchet.py's | ||
| # `test_structural_preflight_defers_every_defect_it_waves_through` asserts | ||
| # that coupling one planted defect shape at a time. | ||
| - name: Harness unit tests and artifact validation | ||
| run: | | ||
| python3 -m unittest discover -s tests -p 'test_gc_ratchet.py' -v | ||
| python3 benchmarks/gc_ratchet/gc_ratchet.py validate | ||
| python3 benchmarks/gc_ratchet/gc_ratchet.py validate --scope structural |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make gc-ratchet a required context before this deferral lands.
This change moves the verdict for per-cell defects from the preflight step to the check step, and the comment on Lines 91-97 states that check is where the verdict is. The PR objectives list "making gc-ratchet a required context" as a follow-up, so the job is not required today. A deferred defect therefore produces a red check step in a job that cannot block a merge. That converts the deferral into effective suppression for merge purposes, which is the outcome the comment says the design prevents.
Add gc-ratchet to the branch-protection required contexts in the same change, not as a follow-up.
Run the following script to confirm the current protection settings:
#!/bin/bash
# Description: List required status checks on the default branch.
branch="$(gh repo view --json defaultBranchRef --jq '.defaultBranchRef.name')"
repo="$(gh repo view --json nameWithOwner --jq '.nameWithOwner')"
gh api "repos/$repo/branches/$branch/protection/required_status_checks" --jq '.contexts, .checks' 2>&1 || \
echo "no required_status_checks configured or insufficient token scope"As per coding guidelines: "A CI gate must not use continue-on-error, must be included in required branch-protection contexts, must avoid unconditional cancellation of main runs, and must assert that the behavior it measures actually executed."
🤖 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 @.github/workflows/gc-ratchet.yml around lines 91 - 101, Add the gc-ratchet
workflow’s check context to the repository’s default-branch required status
checks alongside the workflow change, ensuring the deferred defect verdict can
block merges. Update the branch-protection configuration or automation that
defines required contexts, using the existing gc-ratchet status-check name and
preserving the gate’s non-canceling, non-continue-on-error behavior.
Source: Coding guidelines
| for name, entry in probes.items(): | ||
| metrics = entry.get("metrics") | ||
| if not isinstance(metrics, Mapping): | ||
| raise RatchetError(f"{name}: no metrics recorded") | ||
| artifact_defect(f"{name}: no metrics recorded") | ||
| continue | ||
|
|
||
| # Integrity of the recorded numbers. A missing metric or a summary that | ||
| # disagrees with its own samples is tampering or corruption, not | ||
| # unfitness: it stays fatal, because a partially-trusted artifact is not | ||
| # a thing this gate should ever compare against. | ||
| unreadable = False | ||
| for metric in ALL_METRICS: | ||
| if metric not in metrics: | ||
| raise RatchetError(f"{name}: baseline is missing {metric}") | ||
| artifact_defect(f"{name}: baseline is missing {metric}") | ||
| unreadable = True | ||
| continue | ||
| recorded = metrics[metric] | ||
| samples = recorded.get("samples") | ||
| if not isinstance(samples, list) or len(samples) < 2: | ||
| raise RatchetError(f"{name}: {metric} has too few samples") | ||
| artifact_defect(f"{name}: {metric} has too few samples") | ||
| unreadable = True | ||
| continue | ||
| if recorded != distribution(samples): | ||
| raise RatchetError(f"{name}: {metric} summary is inconsistent with its samples") | ||
| artifact_defect(f"{name}: {metric} summary is inconsistent with its samples") | ||
| unreadable = True | ||
| if unreadable: | ||
| continue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against non-Mapping probe and metric entries.
inspect_artifact validates that probes and metrics are Mappings, but it does not validate their values. If a metric entry is a scalar, recorded.get("samples") at Line 1203 raises AttributeError. If a probe entry is a scalar, entry.get("metrics") at Line 1187 raises AttributeError. The function is the classifier for malformed artifacts, so it should report these as fatal artifact defects instead of crashing. A crash also bypasses the RatchetError handler in main, so the CLI prints a traceback instead of the gc-ratchet error: message and its exit code 2.
🛡️ Proposed fix
for name, entry in probes.items():
+ if not isinstance(entry, Mapping):
+ artifact_defect(f"{name}: probe entry is not an object")
+ continue
metrics = entry.get("metrics")
if not isinstance(metrics, Mapping):
artifact_defect(f"{name}: no metrics recorded")
continue
@@
recorded = metrics[metric]
+ if not isinstance(recorded, Mapping):
+ artifact_defect(f"{name}: {metric} is not a recorded distribution")
+ unreadable = True
+ continue
samples = recorded.get("samples")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for name, entry in probes.items(): | |
| metrics = entry.get("metrics") | |
| if not isinstance(metrics, Mapping): | |
| raise RatchetError(f"{name}: no metrics recorded") | |
| artifact_defect(f"{name}: no metrics recorded") | |
| continue | |
| # Integrity of the recorded numbers. A missing metric or a summary that | |
| # disagrees with its own samples is tampering or corruption, not | |
| # unfitness: it stays fatal, because a partially-trusted artifact is not | |
| # a thing this gate should ever compare against. | |
| unreadable = False | |
| for metric in ALL_METRICS: | |
| if metric not in metrics: | |
| raise RatchetError(f"{name}: baseline is missing {metric}") | |
| artifact_defect(f"{name}: baseline is missing {metric}") | |
| unreadable = True | |
| continue | |
| recorded = metrics[metric] | |
| samples = recorded.get("samples") | |
| if not isinstance(samples, list) or len(samples) < 2: | |
| raise RatchetError(f"{name}: {metric} has too few samples") | |
| artifact_defect(f"{name}: {metric} has too few samples") | |
| unreadable = True | |
| continue | |
| if recorded != distribution(samples): | |
| raise RatchetError(f"{name}: {metric} summary is inconsistent with its samples") | |
| artifact_defect(f"{name}: {metric} summary is inconsistent with its samples") | |
| unreadable = True | |
| if unreadable: | |
| continue | |
| for name, entry in probes.items(): | |
| if not isinstance(entry, Mapping): | |
| artifact_defect(f"{name}: probe entry is not an object") | |
| continue | |
| metrics = entry.get("metrics") | |
| if not isinstance(metrics, Mapping): | |
| artifact_defect(f"{name}: no metrics recorded") | |
| continue | |
| # Integrity of the recorded numbers. A missing metric or a summary that | |
| # disagrees with its own samples is tampering or corruption, not | |
| # unfitness: it stays fatal, because a partially-trusted artifact is not | |
| # a thing this gate should ever compare against. | |
| unreadable = False | |
| for metric in ALL_METRICS: | |
| if metric not in metrics: | |
| artifact_defect(f"{name}: baseline is missing {metric}") | |
| unreadable = True | |
| continue | |
| recorded = metrics[metric] | |
| if not isinstance(recorded, Mapping): | |
| artifact_defect(f"{name}: {metric} is not a recorded distribution") | |
| unreadable = True | |
| continue | |
| samples = recorded.get("samples") | |
| if not isinstance(samples, list) or len(samples) < 2: | |
| artifact_defect(f"{name}: {metric} has too few samples") | |
| unreadable = True | |
| continue | |
| if recorded != distribution(samples): | |
| artifact_defect(f"{name}: {metric} summary is inconsistent with its samples") | |
| unreadable = True | |
| if unreadable: | |
| continue |
🤖 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 `@benchmarks/gc_ratchet/gc_ratchet.py` around lines 1186 - 1212, Update
inspect_artifact’s probe and metric validation to require each probe entry and
each metric entry to be a Mapping before calling get or accessing its contents.
Report invalid entries through artifact_defect, mark the artifact unreadable,
and continue classification so malformed artifacts produce the existing fatal
artifact result and RatchetError CLI handling instead of raising AttributeError.
| if unfit and args.scope == "all": | ||
| print( | ||
| f"gc-ratchet: {args.artifact} has {len(unfit)} unfit cell(s); " | ||
| "re-pin them or record a probe_overrides entry", | ||
| file=sys.stderr, | ||
| ) | ||
| return 1 | ||
| if unfit: | ||
| print( | ||
| f"gc-ratchet: {len(unfit)} unfit cell(s) deferred to `check` " | ||
| "(--scope structural); they will fail the job there, after the probes run", | ||
| file=sys.stderr, | ||
| ) | ||
| print(f"gc-ratchet: {args.artifact} is structurally valid") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Operator-facing text calls every non-fatal defect a "cell". The non-fatal set contains both cell- and probe-scoped defects, but two output paths name only cells. A probe pinned without an oracle diff is then reported as an unfit cell.
benchmarks/gc_ratchet/gc_ratchet.py#L1678-L1691: change "unfit cell(s)" to "unfit probe(s)/cell(s)" in both the--scope allmessage and the deferral message.benchmarks/gc_ratchet/gc_ratchet.py#L1490-L1502: change the report heading "Pinned cells that could not be gated" to name probes and cells.
📍 Affects 1 file
benchmarks/gc_ratchet/gc_ratchet.py#L1678-L1691(this comment)benchmarks/gc_ratchet/gc_ratchet.py#L1490-L1502
🤖 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 `@benchmarks/gc_ratchet/gc_ratchet.py` around lines 1678 - 1691, Update
operator-facing labels in benchmarks/gc_ratchet/gc_ratchet.py: in the messages
near lines 1678-1691, replace “unfit cell(s)” with “unfit probe(s)/cell(s)” in
both the --scope all and deferral paths; also update the report heading near
lines 1490-1502 from “Pinned cells that could not be gated” to name both probes
and cells.
| ## A defect in the artifact costs one cell, not the whole gate (#7554) | ||
|
|
||
| Artifact validation used to abort on the first problem it found, and it runs | ||
| *before* the measurement step. So one cell — `12_large_live_set.heap_used_bytes`, | ||
| spread 6,768 bytes — meant none of the twelve probes executed on any branch for | ||
| three days. Two GC pacing changes (#7594, #7596) merged inside that window and | ||
| each had to hand-run a both-arms A/B in place of the gate. The claim that caused | ||
| it was about **one cell**; nothing about it voided the other 143 or made the | ||
| probes unrunnable. | ||
|
|
||
| Defects now carry a scope, and the scope is the blast radius: | ||
|
|
||
| | scope | examples | what it voids | | ||
| |---|---|---| | ||
| | `artifact` | wrong schema, missing metric, a summary that disagrees with its own samples | everything — still fatal, still in preflight | | ||
| | `probe` | pinned without an oracle diff, pinned with no collection | that probe's rows | | ||
| | `cell` | spread ≠ 0 on a metric whose band's premise is bit-identity | that one cell | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the new section with the existing #7554 text.
Three accuracy points:
- Line 295 states the outage lasted three days. Line 204 of the same file states two days for the same incident. Make the two agree.
- Line 249 states that the bit-identity rule "now lives in
validate_artifact". This change moved the rule intoinspect_artifact;validate_artifactonly raises on the defects thatinspect_artifactcollects. Update that sentence so the reader looks in the right function. - The
cellrow on Line 307 omits the gating condition.inspect_artifactraises a cell defect only whengated_anywhere(...)is true, so a cell already excluded byprobe_overridesproduces no defect. Adding that clause prevents a reader from concluding that a recorded override still fails the artifact.
🤖 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 `@benchmarks/gc_ratchet/README.md` around lines 291 - 307, The README’s new
artifact-validation section conflicts with existing incident details and
implementation behavior. Update the outage duration to match the two-day
account, revise the bit-identity rule reference from validate_artifact to
inspect_artifact, and clarify the cell scope row to state that defects apply
only when gated_anywhere(...) is true, excluding cells disabled by
probe_overrides.
…gate (#7554) An artifact defect used to abort validation on the first problem, and the artifact-validation step runs BEFORE the measurement step. So one cell — 12_large_live_set.heap_used_bytes, spread 6,768 bytes — meant none of the twelve probes executed on any branch for three days, while two GC pacing changes (#7594, #7596) merged with hand-run both-arms A/Bs standing in for the gate. The defect was a claim about ONE cell. Nothing about it voided the other 143, and nothing about it made the probes unrunnable. Defects now carry a scope. `artifact` (unreadable, tampered, missing metric) stays fatal and stays in preflight. `probe` (pinned without an oracle diff, or with no collection) and `cell` (contradicts the bit-identity premise of its own band) demote their subject out of the gating family and are reported as failures — so `check` still measures everything, still evaluates the other cells, and still names a regression elsewhere in the matrix, while the defect itself keeps the job red. `validate --scope structural` (what CI preflight now runs) fails only on the fatal kind. It cannot suppress: `check` re-derives the same list and fails on it, and a test asserts that coupling per planted defect shape. `assemble` is unchanged — pin time still refuses any defect outright, so this cannot be used to freeze a new unfit artifact.
…ost (#7554) gc-ratchet had not been green on main since 2026-08-01T05:39Z — 179 consecutive red main runs. The 2026-08-05 window where it could not reach its probes at all (#7554, fixed by #7557) was an episode inside that, not the whole of it: after #7557 restored measurement the job stayed red against a 0.5.1280 artifact that no longer described the collector. Re-pinned at origin/main 26b9c9d (0.5.1346) on perry-macos — the same Mac mini and the same rustc/cargo/clang the 2026-08-05 pin used, so this is like-for-like. All 12 probes oracle-pass; heap_used_bytes spread 0 on eleven and 864 B on 12_large_live_set. Full per-cell attribution is in the artifact's own `notes`. Three of the four moved groups are explained: - 03/04's copy and promote counters collapsing 40–99.8% is #7594 + #7596 doing what they said (less futile promotion). Recorded caveat: 03's promoted_* now pin at 0, where the allowance floor and the liveness assert both go quiet. - 02 +2.77% and 05 +16.44% retention are conservative-scan false roots, not retention. `classify` on this host gives 05 precise 5,329,880 — byte-identical to what #7571 measured at both ends of its window — and 02 precise 9,416,632, BELOW the number this baseline previously recorded. That is #7559's answer, reproduced rather than assumed. The fourth is flagged, not explained: 12_large_live_set.wall_ms 3,056 -> 3,471 ms (+13.58%), two non-overlapping 7-sample clusters on one host, while 06 and 11 got 9.6% and 28.4% faster. #7596 reported -7.4% on that cell, so by its own evidence this is not #7596. It is gated on pinned_host only. #7596's accepted 12_large_live_set.heap_total_bytes +36% did NOT reproduce here (110,100,480 -> 110,100,480, +0.00%), so nothing was re-pinned for it.
2657ba1 to
8a04f53
Compare
Audit before merge — verified, merged as v0.5.1348The premise correction is the finding. I briefed this as "preflight fails, Sabotage re-verified: silencing the demoted-defect reporting (turning Two attribution results deserve the record:
The honest failure is worth more than the sabotage table: the agent's first Gates re-run here: harness suite green, validate structurally valid on the Owner action queued: promote |
1 participant
Closes the #7554 repair: makes an artifact defect cost one cell instead of the
whole gate, re-pins the baseline on the pinned host at current
main, and showsthe restored gate going red and green on demand.
The premise moved — what is actually broken
#7554 says the ratchet dies in preflight. That was true, and #7557 already
fixed it. Preflight passes at
maintoday (63 unit tests +validate, exit 0,verified locally). The failure is now one step later and one week longer:
gc-ratchetonmain30686263856)mainruns, 15 cancelled, 0 greencheck, against a0.5.1280artifact that no longer describes the collectorSo the gate has produced no actionable verdict for a week, and because
gc-ratchetis not a required context (see below) none of it blockedanything. #7594 and #7596 both substituted hand-run both-arms A/Bs.
1. Fail open per cell, fail closed on the verdict
Artifact validation aborted on the first defect, and it runs before the
measurement step. One cell —
12_large_live_set.heap_used_bytes, spread 6,768bytes — therefore meant none of the twelve probes executed on any branch for
three days. The claim that caused it was about one cell; nothing about it
voided the other 143 or made the probes unrunnable. That blast radius was never
chosen, it was inherited from raising an exception.
Defects now carry a scope:
artifactprobecellprobe/celldefects demote their subject out of the gating family and arereported as failures.
checkstill measures all twelve probes, still evaluatesthe rest of the matrix, and still names a regression elsewhere — while the defect
keeps the job red.
CI preflight now runs
validate --scope structural, which fails only on thefatal kind.
assembleis deliberately unchanged: pinning still refuses anydefect, so this cannot be used to freeze a new unfit artifact.
Why this is not suppression.
checkre-derives the same defect list andfails on every entry, and a test asserts that coupling per planted defect shape.
Sabotage evidence below.
2. Baseline re-pinned at
main26b9c9d (0.5.1346)Measured on
perry-macos(Mac mini M1, 8 GB) — the same host and the samerustc/cargo/clang as the 2026-08-05 pin, so like-for-like. Load 2.38/1.9/1.7 at
capture (the previous pin was taken at 2.01/3.2/7.57). All 12 probes
oracle-pass;
heap_used_bytesspread 0 on eleven probes and 864 B on12_large_live_set. Full attribution is written into the artifact's ownnotes.Cells that moved
(a) Explained — collection pacing (#7594, #7596).
03_cross_gen_writes03_cross_gen_writes03_cross_gen_writes03_cross_gen_writes04_dead_after_deep_stack04_dead_after_deep_stack04_dead_after_deep_stack04_dead_after_deep_stackRecorded rather than glossed:
03'spromoted_*now pin at 0, where theallowance floor (16 objects / 64 KiB) covers the whole range and the liveness
assertion fires only when the baseline median is > 0. That cell no longer carries
signal in either direction. It is the price of pinning a counter at zero.
(b) Explained — measurement, not retention (#7558, #7571).
classifyprecise02_survivor_promotion05_closure_capture05's precise retention is the exact figure #7571 measured at both ends ofits 74-commit window; the conservative residue went 1 block → 2 (2,097,080 B)
while real retention did not move at all.
02's precise reading is lower thanthe number this artifact previously recorded as that probe's retention, so real
retention cannot have grown. This is #7559's answer, reproduced independently on
the pinned host rather than assumed.
(c) FLAGGED — not explained by any merged, documented decision.
12_large_live_set.wall_ms3,056 → 3,471 ms (+13.58%). Two non-overlapping7-sample clusters (3,047–3,061 vs 3,466–3,476), same host, same toolchain, same
protocol — while
06_string_retentionand11_collect_at_depthgot 9.6% and28.4% faster over the same window. #7596 reported −7.4% on this very cell
in its own both-arms A/B, so by that PR's own evidence this is not #7596. Gated
under
pinned_hostonly, so it does not block CI. Pinned here so the rest of thematrix can gate again; it wants a bisect over 0.5.1280..0.5.1346.
Note on the diff size
baseline/gc-ratchet-v1.jsonis+684 / −3,057, but 2,374 of those deletedlines are the
suiteblock (abenchmarks/compare.shrun recorded alongsidethe previous pin), now
null. The probe set is unchanged and every top-level keyis still present.
compare.shneeds a full checkout and a Node/Bun toolchain;this pin measured shipped binaries on the mini, so the suite was not run. The
driver supports exactly this via
--no-suite, and the omission is recorded in theartifact's
notesrather than left for a reader to notice. Re-running it later isa re-pin, not a patch.
(d) Did not reproduce. #7596's merge audit accepted
12_large_live_set.heap_total_bytes+36% (95.4 → 130.0 MB) and deferred there-pin to this repair. Under the harness protocol on this host that cell is
110,100,480 → 110,100,480, +0.00% — and neither endpoint of the +36% figure
matches this artifact's reading. Nothing was re-pinned on account of it.
3. Sabotage evidence (real exit codes)
Harness mechanism — two ways to break it, both caught:
evaluateto abort on any defect (the #7554 collapse)failures(fail-open → suppression)Preflight scoping, on an artifact carrying a planted 6,768-byte spread:
validate --scope allvalidate --scope structuralcheck— 144 rows still evaluated, unfit cell demoted, and an unrelated plantedfreed_bytes−50% regression still named--scope structural(integrity, not fitness)That third row is the whole point: under the old behaviour that
freed_bytesregression was invisible, because nothing ran.
The re-pinned gate itself, end to end on the pinned host — sabotaging the
collector, not the JSON:
checkexitpinned_hostshared_ciPERRY_GEN_GC=0(non-generational)PERRY_GEN_GC_EVACUATE=0, which moved 0 cells across all 12 probes and leftthe gate green — measured, not inferred: a cell-by-cell diff of the two
measurements reports
cells that moved: 0. So the knob changes nothing thisratchet observes, and "passes with evacuation policy disabled" has never been
evidence about this suite. (Plausibly because it gates the C4b policy
evacuation of tenured objects rather than the copying minor that
copied_objectscounts — but that attribution is a reading of the code, not something these runs
establish.) That is the
#6942/#7024 pattern (a knob that disables something other than what it is
believed to disable) and it is worth its own look; per CLAUDE.md's kill-policy an
off-state nothing exercises should not survive. It also means "passes with
evacuation policy disabled" has never been evidence about this suite.
4. CI workflow audit — the four hazards
continue-on-errorif: always()only on artifact uploadlint, cargo-test, parity, compile-smoke, api-docs-drift, security-audit, conformance-smoke-complete.gc-ratchetis absent. 179 red main runs blocked nothing.push(#7205); main runs queue and completecheckassertsminor_cycles > 0andcopied_objects > 0rather than inferring from bands, and thePERRY_GEN_GC=0arm above proves it fires. Caveat:03.promoted_*pinned at 0 is a cell that can no longer fail (called out in §2a).Hazard 2 needs admin action and I have not attempted it. Promoting
gc-ratchetto a required context is an admin-only branch-protection change, andCLAUDE.md's corollary applies: promote after a green
mainrun, not before, orevery open PR is blocked. This PR is the prerequisite — it is what makes a green
mainrun possible for the first time since 2026-08-01.Validation
CI has a deep runner backlog (
mainpush runs from 22:07Z onward are stillqueued), so the evidence here is local and I am saying so plainly:
python3 -m unittest discover -s tests -p 'test_gc_ratchet.py'— 71 passed(63 pre-existing + 8 new), plus both sabotages above
gc_ratchet.py validate --scope allon the new artifact — exit 0gc_ratchet.py checkon the pinned host, both profiles — exit 0raw_handle_debt.py998 = baseline ·addr_class_inventory.pypass ·class_id_collisions.pypass ·check_file_size.shpass ·cargo fmt --all -- --checkcleancargo test -p perry-runtimeis not implicated; themeasured collector is exactly
origin/main26b9c9d59Follow-ups (not in this PR)
gc-ratchetto a required context after its first greenmainrun.12_large_live_set.wall_ms+13.58% — filed as gc: 12_large_live_set is 13.6% slower at 0.5.1346 than 0.5.1280 with every collector counter flat #7610, bisect 0.5.1280..0.5.1346.PERRY_GEN_GC_EVACUATE=0is inert on this suite — filed as gc: PERRY_GEN_GC_EVACUATE=0 moves zero cells on all 12 gc-ratchet probes — the knob's off-state is unexercised #7611;delete the off-state or give it an arm that exercises it (CLAUDE.md kill-policy).
heap_used_byteshas a 1 MiB noise quantum and a127 KiB band on some probes, so re-pinning only resets the clock. test(gc-ratchet): classify a retention breach instead of guessing at it (#7559) #7571's own
conclusion is that the fix is to measure a different quantity — gate the
classifyprecise reading instead of the conservative one. That is ametric-family redesign and a maintainer call, deliberately not smuggled in
here.