ci(ci): gate localStorage allowlist growth via burndown budget script#1589
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a budgeted CI check for the localStorage allowlist: a JSON budget file, a Node CLI that parses ChangeslocalStorage Allowlist Budget Enforcement
Sequence DiagramsequenceDiagram
participant CI as CI Environment
participant Script as check-localstorage-allowlist
participant ESLint as eslint.config.js
participant Budget as localstorage-allowlist-budget.json
participant Exit as Process Exit
CI->>Script: Invoke via npm script
Script->>ESLint: Load and extract rule ignores
ESLint-->>Script: sergeant-design/no-raw-local-storage ignores block
Script->>Script: Count production allowlist entries (exclude test paths & comments)
Script->>Budget: Load budget JSON (or use LS_ALLOWLIST_BUDGET)
Budget-->>Script: {production: 19, rationale: "..."}
Script->>Script: Validate budget schema (finite number, rationale ≥ 8 chars)
alt Budget exceeded or parse/validation error
Script->>Exit: Exit code 1 (print diagnostics)
else Budget OK
Script->>Exit: Exit code 0 (print summary & rationale)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 6/10 reviews remaining, refill in 21 minutes and 48 seconds. Comment |
This was referenced May 4, 2026
Skords-01
force-pushed
the
devin/1777863626-localstorage-budget
branch
from
0081d9a to
c731079
Compare
⏱️ CI Pipeline Duration ReportBased on the last 50 successful runs on the default branch. Overall Pipeline
Trend (last 20 runs): Per-Job Breakdown
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
package.json (1)
26-26: Consider making the allowlist gate a separate required CI step.With Line 26 chained by
&&,pnpm lint:localstorage-allowlistis skipped whenever an earlier lint step fails. A standalone required job keeps this governance signal continuously visible even during unrelated lint regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 26, The package.json "lint" script currently chains "pnpm lint:localstorage-allowlist" so it is skipped if any earlier lint step fails; remove "pnpm lint:localstorage-allowlist" from the "lint" script and instead add a dedicated npm script (e.g., "check:localstorage-allowlist" or "lint:localstorage-allowlist:ci") that runs "pnpm lint:localstorage-allowlist", and update CI configuration to run that new script as its own required job; ensure you reference the original script name "lint" and the allowlist script "lint:localstorage-allowlist" when making these changes so the gate always executes independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/check-localstorage-allowlist.mjs`:
- Around line 71-94: The current discovery logic uses hard-coded window limits
(the backward loop using Math.max(0, i - 80) that assigns ruleLine and the
forward loop using Math.min(lines.length, ruleLine + 60) that finds
ignoresStart), which can miss blocks; change both scans to search until the file
boundaries (or until a clear block delimiter) instead of limiting to 80/60
lines: remove the Math.max(..., i - 80) constraint and the Math.min(...,
ruleLine + 60) constraint so the backward loop scans to index 0 and the forward
loop scans to lines.length (or stop when you hit the next rule separator),
keeping the same checks for lines[j].includes("files:") &&
lines[j].includes('"apps/web/src/') and lines[i].includes("ignores:")
respectively.
- Around line 205-208: When an environment override LS_ALLOWLIST_BUDGET
(envBudget) is present but invalid the code currently silently falls back;
change this to fail fast by validating envBudget and throwing/terminating if
invalid: parse envBudget to Number (const n = Number(envBudget)), check
Number.isFinite(n) and n >= 0; if valid set budget = Math.floor(n) (as now),
otherwise throw an Error or call process.exit(1) with a clear message mentioning
LS_ALLOWLIST_BUDGET and the invalid value so CI fails fast; update the block
that references envBudget, n and budget to implement this behavior.
---
Nitpick comments:
In `@package.json`:
- Line 26: The package.json "lint" script currently chains "pnpm
lint:localstorage-allowlist" so it is skipped if any earlier lint step fails;
remove "pnpm lint:localstorage-allowlist" from the "lint" script and instead add
a dedicated npm script (e.g., "check:localstorage-allowlist" or
"lint:localstorage-allowlist:ci") that runs "pnpm lint:localstorage-allowlist",
and update CI configuration to run that new script as its own required job;
ensure you reference the original script name "lint" and the allowlist script
"lint:localstorage-allowlist" when making these changes so the gate always
executes independently.
🪄 Autofix (Beta)
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: 7366e0bf-89f5-4335-96b9-f9af5c6030a4
📒 Files selected for processing (6)
.tech-debt/localstorage-allowlist-budget.jsondocs/diagnostics/2026-05-03-web-deep-dive/00-overview.mddocs/diagnostics/2026-05-03-web-deep-dive/02-architecture-and-state.mdpackage.jsonscripts/__tests__/check-localstorage-allowlist.test.mjsscripts/check-localstorage-allowlist.mjs
Comment on lines
+71
to
+94
| for (let j = i; j >= Math.max(0, i - 80); j--) { | ||
| if ( | ||
| lines[j].includes("files:") && | ||
| lines[j].includes('"apps/web/src/') | ||
| ) { | ||
| ruleLine = j; | ||
| break; | ||
| } | ||
| } | ||
| if (ruleLine !== -1) break; | ||
| } | ||
| } | ||
|
|
||
| if (ruleLine === -1) return null; | ||
|
|
||
| // Walk forward to the `ignores:` line. | ||
| let ignoresStart = -1; | ||
| for (let i = ruleLine; i < Math.min(lines.length, ruleLine + 60); i++) { | ||
| if (lines[i].includes("ignores:")) { | ||
| ignoresStart = i; | ||
| break; | ||
| } | ||
| } | ||
| if (ignoresStart === -1) return null; |
There was a problem hiding this comment.
Remove brittle line-window limits in rule discovery.
Line 71 and Line 88 hard-cap scans to 80/60 lines. If eslint.config.js gets extra comments or formatting churn, the script can fail to locate a valid block and break CI incorrectly.
Suggested patch
- for (let j = i; j >= Math.max(0, i - 80); j--) {
+ for (let j = i; j >= 0; j--) {
if (
lines[j].includes("files:") &&
lines[j].includes('"apps/web/src/')
) {
ruleLine = j;
break;
}
}
@@
- for (let i = ruleLine; i < Math.min(lines.length, ruleLine + 60); i++) {
+ for (let i = ruleLine; i < lines.length; i++) {
if (lines[i].includes("ignores:")) {
ignoresStart = i;
break;
}
}📝 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.
Suggested change
| for (let j = i; j >= Math.max(0, i - 80); j--) { | |
| if ( | |
| lines[j].includes("files:") && | |
| lines[j].includes('"apps/web/src/') | |
| ) { | |
| ruleLine = j; | |
| break; | |
| } | |
| } | |
| if (ruleLine !== -1) break; | |
| } | |
| } | |
| if (ruleLine === -1) return null; | |
| // Walk forward to the `ignores:` line. | |
| let ignoresStart = -1; | |
| for (let i = ruleLine; i < Math.min(lines.length, ruleLine + 60); i++) { | |
| if (lines[i].includes("ignores:")) { | |
| ignoresStart = i; | |
| break; | |
| } | |
| } | |
| if (ignoresStart === -1) return null; | |
| for (let j = i; j >= 0; j--) { | |
| if ( | |
| lines[j].includes("files:") && | |
| lines[j].includes('"apps/web/src/') | |
| ) { | |
| ruleLine = j; | |
| break; | |
| } | |
| } | |
| if (ruleLine !== -1) break; | |
| } | |
| } | |
| if (ruleLine === -1) return null; | |
| // Walk forward to the `ignores:` line. | |
| let ignoresStart = -1; | |
| for (let i = ruleLine; i < lines.length; i++) { | |
| if (lines[i].includes("ignores:")) { | |
| ignoresStart = i; | |
| break; | |
| } | |
| } | |
| if (ignoresStart === -1) return null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/check-localstorage-allowlist.mjs` around lines 71 - 94, The current
discovery logic uses hard-coded window limits (the backward loop using
Math.max(0, i - 80) that assigns ruleLine and the forward loop using
Math.min(lines.length, ruleLine + 60) that finds ignoresStart), which can miss
blocks; change both scans to search until the file boundaries (or until a clear
block delimiter) instead of limiting to 80/60 lines: remove the Math.max(..., i
- 80) constraint and the Math.min(..., ruleLine + 60) constraint so the backward
loop scans to index 0 and the forward loop scans to lines.length (or stop when
you hit the next rule separator), keeping the same checks for
lines[j].includes("files:") && lines[j].includes('"apps/web/src/') and
lines[i].includes("ignores:") respectively.
Comment on lines
+205
to
+208
| if (envBudget !== undefined && envBudget !== null && envBudget !== "") { | ||
| const n = Number(envBudget); | ||
| if (Number.isFinite(n) && n >= 0) budget = Math.floor(n); | ||
| } |
There was a problem hiding this comment.
Fail fast when LS_ALLOWLIST_BUDGET is provided but invalid.
Line 205–208 currently ignores invalid overrides and silently falls back to file budget. That makes CI misconfiguration hard to detect.
Suggested patch
if (envBudget !== undefined && envBudget !== null && envBudget !== "") {
const n = Number(envBudget);
- if (Number.isFinite(n) && n >= 0) budget = Math.floor(n);
+ if (!Number.isFinite(n) || n < 0) {
+ return {
+ ok: false,
+ count,
+ budget: null,
+ reason: "LS_ALLOWLIST_BUDGET must be a non-negative finite number",
+ };
+ }
+ budget = Math.floor(n);
}📝 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.
Suggested change
| if (envBudget !== undefined && envBudget !== null && envBudget !== "") { | |
| const n = Number(envBudget); | |
| if (Number.isFinite(n) && n >= 0) budget = Math.floor(n); | |
| } | |
| if (envBudget !== undefined && envBudget !== null && envBudget !== "") { | |
| const n = Number(envBudget); | |
| if (!Number.isFinite(n) || n < 0) { | |
| return { | |
| ok: false, | |
| count, | |
| budget: null, | |
| reason: "LS_ALLOWLIST_BUDGET must be a non-negative finite number", | |
| }; | |
| } | |
| budget = Math.floor(n); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/check-localstorage-allowlist.mjs` around lines 205 - 208, When an
environment override LS_ALLOWLIST_BUDGET (envBudget) is present but invalid the
code currently silently falls back; change this to fail fast by validating
envBudget and throwing/terminating if invalid: parse envBudget to Number (const
n = Number(envBudget)), check Number.isFinite(n) and n >= 0; if valid set budget
= Math.floor(n) (as now), otherwise throw an Error or call process.exit(1) with
a clear message mentioning LS_ALLOWLIST_BUDGET and the invalid value so CI fails
fast; update the block that references envBudget, n and budget to implement this
behavior.
Adds scripts/check-localstorage-allowlist.mjs which parses the apps/web no-raw-local-storage ESLint allowlist, counts production entries (test fixtures excluded), and fails CI when the count exceeds .tech-debt/localstorage-allowlist-budget.json. Wired into pnpm lint via lint:localstorage-allowlist. Implements item 6 of diagnostic 2026-05-03-web-deep-dive section 2.2. 12 unit tests cover the parser plus budget validator.
Skords-01
force-pushed
the
devin/1777863626-localstorage-budget
branch
from
c731079 to
7796a79
Compare
This was referenced May 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes item 6 of the web-deep-dive roadmap (
docs/diagnostics/2026-05-03-web-deep-dive/02-architecture-and-state.md§2.2, score 2.00). The 19-entryno-raw-local-storageallowlist вeslint.config.jscould silently grow because nothing was watching it — every new entry is a future Safari Private Mode / quota crash. This PR adds a CI guard that turns the allowlist into a tracked metric.What ships:
scripts/check-localstorage-allowlist.mjs— parseseslint.config.js, locates theapps/web/srcignores block forno-raw-local-storage, counts production entries (test fixtures excluded), and fails if the count exceeds.tech-debt/localstorage-allowlist-budget.json..tech-debt/localstorage-allowlist-budget.json—{ production: 19, rationale: "..." }. Mandatory ≥8-charrationaleso a budget bump always carries an explanation in git blame.scripts/__tests__/check-localstorage-allowlist.test.mjs— 12node --testtests covering parser extraction, comment stripping, test-fixture exclusion, and budget validation (positive numbers, mandatory rationale).package.json— addslint:localstorage-allowlistand wires it into the top-levelpnpm lintpipeline alongside the other governance gates.docs/diagnostics/2026-05-03-web-deep-dive/02-architecture-and-state.md§2.2 now points to the running KPI instead of a TODO.Behaviour: dropping an entry forces an explicit
productiondecrement +rationaleupdate in the same PR (script intentionally tolerates undershoot so lockstep audits are simple). Adding an entry without bumping the budget breaks CI.Governing Skill
sergeant-deploy-and-observabilitysergeant-monorepo-boundariesPlaybook
Verification
pnpm lintis currently broken onmainitself (PR #1572 bumped eslint to 10.x, buteslint-plugin-react@7.37.5still calls the removedcontext.getFilename()API — thecheckjob is red on the most recent green-merged commit). This PR's own surface (the new mjs script + tests) is unaffected and runs clean on Node 20.20.2; CI will fail for the pre-existingeslint-plugin-reactreason regardless.Additional checks:
Docs and Governance
AGENTS.mdneeded an update.Updated docs:
docs/diagnostics/2026-05-03-web-deep-dive/02-architecture-and-state.md§2.2 — now references the live CI metric.docs/diagnostics/2026-05-03-web-deep-dive/00-overview.md— roadmap row Claude/review project structure l2 ke1 #6 marked done.Risk and Rollout
pnpm lintreturns to its prior behaviour with no fallout.Hard Rule #15
AGENTS.mdbefore coding.--no-verify.--no-verifyrationale: identical to PR #1588 — the local Husky pre-commit hook fails becauseeslint-plugin-react@7.37.5is incompatible witheslint@10.3.0. Same infra breakage that's red onmain.Reviewer Notes
productionnumber and update therationalein the same PR.Summary by cubic
Adds a CI guard that enforces a burndown budget for the
localStorageESLint allowlist inapps/web. The lint job fails if production entries exceed the budget, pinning the allowlist as a tracked metric.scripts/check-localstorage-allowlist.mjsparseseslint.config.js, counts production entries (tests ignored), and enforces the budget..tech-debt/localstorage-allowlist-budget.jsonsetsproduction: 19with a requiredrationale.package.jsonaddslint:localstorage-allowlistand runs it inpnpm lint.Written for commit 7796a79. Summary will update on new commits.
Summary by CodeRabbit
Chores
Tests
Documentation