Skip to content

fix(quality): let --update remove baseline entries that already fit the cap - #8589

Closed
MumuTW wants to merge 4 commits into
diegosouzapw:release/v3.8.49from
MumuTW:fix/file-size-update-cannot-remove-at-cap
Closed

MumuTW wants to merge 4 commits into
diegosouzapw:release/v3.8.49from
MumuTW:fix/file-size-update-cannot-remove-at-cap

Conversation

@MumuTW

@MumuTW MumuTW commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Fixes the --update gap reported in #8584. Two files: the gate script and its unit tests. No baseline data change.

The bug

check:file-size --update deletes a baseline entry only from inside the improvement branch:

if (violations.length === 0 && improvements.length) {
  for (const [file, loc] of improvements) {      // improvements = loc < frozen[file]
    if (loc <= cap) delete frozen[file];
    else frozen[file] = loc;
  }
}

improvements requires loc < frozen[file]. So an entry sitting at exactly its frozen value, while already within the 800-line new-file cap, never reaches the delete — it is not a violation, not an improvement, and there is no third branch. It stays in the baseline permanently no matter how far under the cap it is.

Three real cases on release/v3.8.49 @ 4053e2314 survived a full --update run for exactly this reason:

entry LOC frozen new-file cap
…/providers/[id]/ProviderDetailPageClient.tsx 798 798 800
…/providers/[id]/hooks/useProviderModels.ts 155 155 800
…/providers/[id]/hooks/useProviderSettings.ts 264 264 800

A 155-line file cannot leave a baseline whose purpose is to track files over 800 lines.

The fix

evaluateFileSizes now returns a third bucket, redundant — entries that are in frozen, are neither a violation nor an improvement, and satisfy loc <= cap:

if (loc > frozen[file]) violations.push(…);
else if (loc < frozen[file]) improvements.push([file, loc]);
else if (loc <= cap) redundant.push(file);        // ← new

The --update path deletes those alongside the shrinks, and the same treatment is applied to the testFrozen/testCap half, which had the identical gap.

The non-update gate path is untouched. redundant entries were already passing and still pass — no PR's pass/fail result changes anywhere. Only --update behaves differently, and only by removing entries that had become meaningless.

TDD

7 new tests written first and confirmed failing, then the fix:

before:  # tests 16 / # pass 9 / # fail 7
after:   # tests 16 / # pass 16 / # fail 0

The 9 pre-existing tests pass unchanged throughout. The new cases pin the boundaries rather than just the happy path:

  • at baseline and within cap → redundant
  • at baseline but still over cap → not redundant (must stay frozen)
  • exactly at the cap → redundant (cap is inclusive, matching loc > cap for new files)
  • shrank → stays an improvement, not double-counted as redundant
  • grew → violation, never redundant
  • the three real #8584 entries above, asserted by path
  • the test-file path (testFrozen/testCap) too

End-to-end verification

Running --update against the real baseline with the fix applied:

[file-size] baseline ratcheado: 74 arquivo(s) encolheram, 3 entrada(s) redundante(s) removida(s)

17 entries removed instead of 14, 0 caps raised, and all three residual entries confirmed gone by diffing the pre/post JSON key-by-key.

That baseline diff is deliberately not in this commit — banking the data is #8585's job, and mixing a logic change with 145 lines of JSON churn would make both harder to review.

Interaction with #8585

Independent, different files, no conflict, either merge order works:

Neither is blocking the other. Both are green-before-green-after — no base-red is being repaired here.

Scope note

I did not touch check-complexity-ratchets.mjs. It has no frozen map and no per-file removal concept — it ratchets two scalar counts — so this class of bug doesn't exist there.

Refs #8584

…he cap

`check:file-size --update` could only ever delete an entry from inside the
improvement branch (`loc < frozen[file]`). An entry sitting at EXACTLY its
frozen value while already within the 800-line new-file cap therefore never
qualified for removal, however far under the cap it was — it stayed in the
baseline permanently.

Three real cases on release/v3.8.49 @ 4053e23 survived a full `--update`
run for this reason:

  src/app/(dashboard)/dashboard/providers/[id]/ProviderDetailPageClient.tsx  798
  src/app/(dashboard)/dashboard/providers/[id]/hooks/useProviderModels.ts    155
  src/app/(dashboard)/dashboard/providers/[id]/hooks/useProviderSettings.ts  264

`evaluateFileSizes` now reports a third bucket, `redundant` — in `frozen`,
not a violation, not an improvement, and `loc <= cap` — and the `--update`
path deletes those entries alongside the shrinks. Same treatment for the
testFrozen/testCap half, which had the identical gap.

The non-update gate path is untouched: `redundant` entries were already
passing and still pass, so no PR's result changes. Only `--update` behaves
differently.

TDD: 7 new failing tests first (16 total in tests/unit/check-file-size.test.ts,
9 pre-existing all still passing), then the fix, then 16/16 green. End-to-end
on the real baseline, `--update` now reports "74 arquivo(s) encolheram, 3
entrada(s) redundante(s) removida(s)" and removes 17 entries instead of 14,
with 0 caps raised. That baseline diff is deliberately NOT in this commit —
banking the data belongs to diegosouzapw#8585.

Refs diegosouzapw#8584
@MumuTW
MumuTW force-pushed the fix/file-size-update-cannot-remove-at-cap branch from e781fd0 to 4198fdd Compare July 26, 2026 09:32
MumuTW added 2 commits July 26, 2026 17:50
Same tip fix as diegosouzapw#8657 so Merge integrity is green without waiting for
that PR to land. Regenerated via generate-agent-skills --apply.
@diegosouzapw

Copy link
Copy Markdown
Owner

Hi @MumuTW — closing this in favor of #8657, which carries the same cli-backup-sync SKILL regeneration with the --status flag removed. The two diffs overlap on skills/cli-backup-sync/SKILL.md, so merging both is not viable; #8657 is the keep. Closing as superseded.

@diegosouzapw

Copy link
Copy Markdown
Owner

Superseded by #8657 (same SKILL regeneration).

@diegosouzapw

Copy link
Copy Markdown
Owner

@MumuTW — Desculpas, fechei este PR por engano durante uma rodada de review-prs (eu tinha metadata stale e o confundi com outro PR seu, o #8657, que de fato regenera cli-backup-sync SKILL.md). Seus dois PRs (#8589 e #8657) tratam de coisas distintas e não têm conflito — este aqui é fix(quality): let --update remove baseline entries that already fit the cap, que parece valioso (fecha o gap #8584 de entradas stuck-as-at-cap).

Você prefere:

  • (A) Eu reabri-lo e rolar a revisão de verdade (leio o diff direito e posto o feedback correto); ou
  • (B) Você abre um PR novo com base nele (git diff origin/release/v3.8.49..gh pr/8589 > /tmp.patch + reabrir)?

Por favor responda se quiser prosseguir. O fechamento foi meu erro de orquestração — o PR em si continua válido.

@diegosouzapw

Copy link
Copy Markdown
Owner

@MumuTW — re: my earlier comment apologizing for mistakenly closing this PR (the orchestration round read stale metadata and conflated #8589 with #8657). On re-reading the diff, this is a clean, focused fix: check:file-size --update now removes baseline entries that already fit the new cap (closes #8584). Approved as merge-ready (★3).

/merge-prs will pick it up once you're comfortable reopening — or you can git push the same branch to a fresh PR since the head is unchanged. Apologies again for the churn.

@MumuTW

MumuTW commented Jul 27, 2026 •

Copy link
Copy Markdown
Contributor Author

@diegosouzapw Reopen was blocked for this account after the head branch was temporarily missing (API: Could not open the pull request). The same head (MumuTW:fix/file-size-update-cannot-remove-at-cap @ 75b9382fbe) is now open again as replacement #8741.

Please treat #8741 as the continue-from for the ★3 merge-ready verdict on this PR. Closing this one as superseded by #8741 is fine.

@diegosouzapw diegosouzapw mentioned this pull request Jul 28, 2026
@MumuTW
MumuTW deleted the fix/file-size-update-cannot-remove-at-cap branch September 5, 2026 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants