chore: enforce max-lines suppression ratchet#107315
Conversation
openclaw-barnacle
Bot
added
docs
clawsweeper
Bot
added
proof: sufficient
|
Codex review: needs changes before merge. Reviewed July 14, 2026, 9:09 AM ET / 13:09 UTC. Summary Reproducibility: yes. from source and routing evidence: a PR that only changes a fast-only governed TypeScript file can push it over 700 effective lines without a suppression, causing the suppression ratchet to pass while the full lint shard is skipped. This review did not execute the fixture because the checkout is read-only. Review metrics: 2 noteworthy metrics.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the suppression-only debt ratchet, but make every governed fast path execute the actual max-lines check on changed files and add a fixture proving an unsuppressed fast-only file over budget fails; retain authenticated merge-tree validation and the documented release-gate refresh path. Do we have a high-confidence way to reproduce the issue? Yes, from source and routing evidence: a PR that only changes a fast-only governed TypeScript file can push it over 700 effective lines without a suppression, causing the suppression ratchet to pass while the full lint shard is skipped. This review did not execute the fixture because the checkout is read-only. Is this the best way to solve the issue? No, not yet: replacing the old LOC comparison with a suppression ratchet is the better direction, but it is only complete when all governed CI routes also run the underlying max-lines rule. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against a34bed409d49. Label changesLabel changes:
Label justifications:
Evidence reviewedAcceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5690d3fea3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const checksFastCoreTasks = | ||
| runNode && !frozenTarget | ||
| ? [ |
There was a problem hiding this comment.
When manual CI is dispatched from one ref with target_ref pointing at a different branch/tag/SHA (the documented gh workflow run ci.yml --ref main -f target_ref=<branch-or-sha> path in docs/ci.md), checkoutRevision !== workflowRevision, so frozenTarget is true and this condition omits the checks-fast-max-lines-ratchet task entirely. That makes the later RATCHET_MANUAL_TARGET_SHA merge-base logic unreachable for exactly those target-ref runs, allowing baseline growth or new suppressions in release-candidate/manual validations to pass without the new ratchet.
Useful? React with 👍 / 👎.
|
Land-ready at Proof:
Known proof gaps: separate aggregate CodeQL and precise OpenGrep checks surface existing code because EOF grandfather comments make those files part of the diff. CodeQL flags the existing process-local HMAC as a password hash; OpenGrep flags six existing Discord test literals. No executable security code or scanner suppression changed. |
|
Merged via squash.
|
1 participant
What Problem This Solves
Resolves a maintainer tooling problem where changed-file line-count gates created refactor and generated-file friction while still leaving no durable control over new lint suppressions.
Why This Change Was Made
This replaces changed-file LOC comparisons with a suppression-baseline ratchet:
src/,ui/src/,packages/, andextensions/.This is lower friction than the prior changed-file LOC gate: renames and refactors pass unless they add a new suppression, generated files never enter the policy, and existing debt does not block unrelated work. Maintainer confirmation: this is the explicitly requested replacement design; landing remains gated on exact-head hosted CI.
AI-assisted: yes (Codex). I understand and reviewed the implementation.
User Impact
No end-user behavior change. Contributors get predictable file-size enforcement: split new oversized files instead of adding a suppression, while existing debt can only decrease.
Evidence
e92c4b3c8db5036605a7781688aa4a805a87a35cmain.tbx_01kxgjn5art64s9f07zdn5s5we: 6 focused files / 385 tests, test types, the prior 1,232-entry ratchet, and full governed-tree Oxlint passed on the preceding clean rebase.git diff --checkThe separate precise OpenGrep scan reports six existing Discord test literals because the baseline comments make those files part of the PR diff. The aggregate CodeQL check reports an existing process-local HMAC as a password hash for the same reason. No scanner suppression or runtime change is included.