fix(gates): the two file-size gates disagreed about how to report a finding, and one skipped files it could not read (#17377) by mrveiss · Pull Request #17884 · mrveiss/AutoBot-AI

mrveiss · 2026-10-03T00:48:14Z

#17377 stays OPEN after this PR, and the wording here is deliberate: a closing keyword next to that number would auto-close it on merge, and GitHub does not read negation (#16464 was closed by a body saying it was not closing it). This PR is a precursor, not the delivery. The issue asks for one engine with both gates as thin configuration over it, a rewritten injection seam in the 590-line ratchet test, and a lowered duplication pin. None of that is here. What is here is the precursor: the two gates reported the same findings differently, and #17377's second criterion — "identical verdict strings for over-ceiling, under-ceiling, compliant, unlisted-oversized and unreadable, verified case by case" — cannot be verified against two gates that disagree about where output goes, at what level, and whether a finding is reported once or twice. Which criteria remain open is listed at the bottom.

Thinking Path

Six drifts, found by reading both gates side by side rather than by a failing test — none of them had one.

The one that mattered most was not a cosmetic difference. _scan_tracked_files in the Python gate did if line_count is None: continue — a silent skip. unmeasured() existed and stated the contract (exit 0 is only entitled to mean within the limit, and a file that was never opened has not earned that), and main honoured it, but the scan path did not. So --audit-ceilings could report "all live and at size" having never read a broken symlink, a bad mode, or a non-UTF-8 file. A clean verdict from a file that was never opened, which is the same failure shape as a guard that reports clean having examined nothing.

Fixing that surfaced a pre-existing double-report in both gates: a file reported as unmeasured was then reported again by the KNOWN_LARGE - seen pass as moved-or-deleted. Two stories about one file, and the second one is wrong — it exists, it simply cannot be read. seen and reached answer different questions, and the fix turns on keeping them different: seen answers "did the walk find this path", so the vanished-entry pass must not fire; reached stays exclusive, so an unmeasured file still cannot prop up the reach floor in run_audit.

I nearly got this wrong in the other direction. python_file_size_ratchet_test.py:511 contradicted my first version of the fix, and rather than dismissing it I read it — it was right, and my change would have suppressed a real report.

Where conformance went in each direction matters for review: it is not "shell conformed to python". Shell adopted Python's stream (stdout → stderr), level (INFO → ERROR, on both paths including the commit path), and reach-breach handling (early return → collected, so a breach no longer suppresses the violations already found). Python adopted shell's KNOWN_LARGE guidance in verdict — advice against grandfathering belongs in the gate that has a KNOWN_LARGE. Each direction is per item, decided by which gate was right.

One thing found after the rebase, and it is the kind CI finds for you if you let it. #17882 merged while this branch was held, taking python_file_size_ratchet_test.py to 597 lines; this branch's hunk in the same file took it to 603, past the 600 hard limit, in a file with no KNOWN_LARGE entry. Two of my own commits, each correct alone, colliding on a file at its cap — the rebase was clean, the size was not. Not fixed by recording an entry for it, which would be grandfathering a test file to fit a docstring: the full-message assertion moved into the parametrised file, which has room and covers both gates, so a sentence both gates emit is pinned once over both.

And the em dash was my drift, not theirs. My new Python sentence used ASCII --; both gates use em dashes throughout their messages (21 and 13 occurrences). The ASCII one was the inconsistency this PR exists to remove.

What Changed

file	change
`scripts/check_python_file_size.py`	the silent `continue` becomes a reported `unmeasured` finding; `_report_unmeasured` extracted carrying both rationales; gained shell's `KNOWN_LARGE` guidance in `verdict`, with the shared em dash
`scripts/check_shell_file_size.py`	findings to stderr not stdout; ERROR not INFO on both paths including the commit path; a reach breach is collected rather than returning early, so it no longer suppresses the violations already found; same `_report_unmeasured` extraction
`repo_tests/gate_reporting_parity_17377_test.py`	new. Parametrised over both gates: stderr, the INFO level, seen-but-not-reached, unmeasured-reported-once, reach-breach-does-not-suppress, commit-path level, the full split-it message, the wording of all seven message branches, and that each gate's `RATCHET_REL` names a file holding entries. Logger isolation is fixture-scoped, so every test inherits it
`repo_tests/python_file_size_ratchet_test.py`	the unlisted-oversized assertion returns to its pre-branch 3-line prefix form, since the full wording is now pinned for both gates in the file above. Net ±0 lines against merged `main`

_scan_tracked_files had grown 21 → 37 (python) and 27 → 35 (shell) because I inlined two rationales; the extraction brings them to 22 and 21, longest function in each file 24 and 23.

Review round: both MEDIUMs taken rather than deferred, and what that found

Review reported the level unpinned (LOW), a handler leak, and two MEDIUMs where behaviour was aligned but not pinned. I took the MEDIUMs here rather than deferring them to the consolidation, because this PR's stated subject is reporting parity — "aligned and pinned" while three message branches drift is a claim exceeding what is enforced, which is the defect this branch exists to remove.

Pinning each text per branch would have been the obvious fix and the weaker one. The new test compares the two gates' messages with everything legitimately per-gate abstracted away — paths, ceilings, the floor constant, SELF_REL, RATCHET_REL — across all seven branches: over-ceiling, under-ceiling, now-compliant, unlisted-oversized, unmeasured, vanished-entry, reach-breach. A future rewording passes only if both gates are reworded together, which is the actual claim being made.

It found a defect nobody had reported. RATCHET_REL in the Python gate named repo_tests/python_file_size_ratchet_test.py, whose RATCHET_BASELINE is a single re-export line (:76) — the entries live in python_file_size_ratchet_baseline.py. So "lower the matching RATCHET_BASELINE entry in " sent a developer to a file with nothing to edit: the wrong-copy failure #17872 exists to prevent, emitted by the gate itself. The wording test cannot see it, because it normalises RATCHET_REL away precisely because that value legitimately differs per gate — so the pointer gets its own test: the named file must define the mapping, which a re-export does not, and must hold at least as many entries as the hook.

Also three serial commas rather than the one reported — the now-compliant, under-ceiling and vanished-entry messages. Systematic rather than random.

The (#14236) citation went the other way, deliberately. It is dropped from Python's over-ceiling message rather than added to shell's: the shell gate's own header cites #14236 as the origin of the Python ratchet ("so .py files cannot grow"), so putting it in a shell-file message would attribute that gate's rule to the wrong issue. The provenance stays in Python's module docstring.

The level, and why it was quiet. Findings are logged at ERROR and survive a default logger, so dropping logger.setLevel(logging.INFO) loses only the informational "all live and at size" summary, under logging's lastResort WARNING floor. The Python gate had a test for that and the shell gate had none — for one of the two gates the line was unguarded. Now asserted for both.

The handler leak. configure_logging() attaches a stderr handler and sets INFO on a logger that logging.getLogger(name) returns as the same object for every load, and nothing removed it. The isolation went into the fixture, so every consuming test inherits it instead of each remembering, restoring handlers and calling setLevel — not a plain attribute write, which would leave logging's effective-level cache stale.

One item I did not take, and the reviewer agreed: the commit-path structural difference is outside this PR's stated subject and belongs to #17377's consolidation.

Verification

Local, repo_tests/ and --check-style gate runs only — no application code run.

repo_tests/{gate_reporting_parity_17377,python_file_size_ratchet,shell_file_size_ratchet,ceiling_record_parity_17872}_test.py
  67 passed
both gates run read-only: exit 0
python_file_size_ratchet_test.py: 597 lines (hard limit 600)

The cross-gate test is the evidence that the two gates now agree, and it was built to fail: with the gate fixes reverted it reports 7 failed, 3 passed — and all three survivors are [python], so which copy was right per item is visible in which tests survive rather than asserted in prose.

Mutation-verified, each mutation confirmed applied by grep -c before its result was trusted:

mutation	result
python gate's split-it sentence reverted	1 failed `[python]`
shell gate's em dash back to ASCII `--`	1 failed `[shell]`
`seen.add` dropped from `_report_unmeasured`	4 failed — both gates, both the seen-not-reached and reported-once tests
`problems.append(unmeasured(rel))` dropped	4 failed — both gates
`RATCHET_REL` reverted to the test file	1 failed — the pointer test (the wording test cannot see this)
one serial comma re-added	1 failed — that branch
`setLevel(INFO)` dropped	1 failed — the level test
the reach hint stripped from the shell gate	1 failed — the reach-breach branch
all restored	78 passed, both gates exit 0, longest function in either gate 23 lines

#17377's criteria that remain OPEN after this PR

One engine implements the ratchet; both gates thin configuration over it.
python_file_size_ratchet_test.py injects through the engine's seam rather than patching module attributes, each rewritten test shown to fail against a deliberately broken engine.
The duplication-guard pin lowered by the consolidation.
The shell gate's deliberate divergence (autobot-infrastructure/ in scope for .sh, excluded for .py) survives the consolidation — untouched here, its test still passes.

Partially served: the identical-behaviour criterion now has a harness. The cross-gate file is parametrised over both gates, so after consolidation the same tests assert the engine once per configuration instead of being rewritten.

Single-issue rationale

This stands alone because it touches the two gate scripts that #17377's consolidation will rewrite wholesale, and the same-file rule puts those in one PR under one owner — batching an unrelated issue in would create exactly the collision that rule exists to prevent. It also edits python_file_size_ratchet_test.py, which sits 3 lines under its 600-line hard limit, so unrelated work cannot ride along without pushing it over. The one other branch I hold (#13841, voice/TTS) shares no file with this and would not have reduced the CI suite count by riding here.

Model Used

Claude Opus 5 (1M context)

…inding (#17377) Drift fixes only; consolidation is deliberately a separate PR -- see the last section for why that order is load-bearing rather than cautious. The defect: the audit path skipped files it could not read ---------------------------------------------------------- `check_python_file_size.py`'s `_scan_tracked_files` did `if line_count is None: continue`, so `--audit-ceilings` could report "all live and at size" having never opened a broken symlink, a bad mode or a non-UTF-8 file. `unmeasured()` existed and stated the contract -- exit 0 is only entitled to mean *within the limit*, and a file never opened has not earned that (#14975) -- but was wired only into `main`. The fix had reached one of two callers. Narrower than first reported, and the correction matters: GRANDFATHERED files were already reported, by accident, through the wrong message. A KNOWN_LARGE entry absent from `seen` is reported as having moved or been deleted -- true for a vanished file, false for one sitting right there unreadable. `seen` and `reached` are not the same question ---------------------------------------------- seen did the walk FIND this path -- yes: it exists, it would not open reached was it RULED ON -- no Reporting the file without adding it to `seen` gave two findings for one file, one of them false. Counting it in `reached` would let an unreadable file prop up the floor check. It now joins `seen` and not `reached`: one accurate finding, floor intact. The shell gate had this bug already, same line, fixed with it. Four more drift items, direction per item ----------------------------------------- shell stream stdout -> stderr (python was right) shell findings level INFO -> ERROR, BOTH paths (python was right) shell reach breach early return -> collected (python was right) python verdict gains the KNOWN_LARGE guidance (SHELL was right) The level is not a style choice: logging's `lastResort` emits WARNING and above only, so INFO findings vanish on any path that never called `configure_logging` -- the gate then exits non-zero and says nothing. `check_paths` was the second INFO site and is the COMMIT path, so the finding most at risk of vanishing was the one that blocks a push. `verdict` goes the other way: the advice not to grandfather a new file belongs in the gate that HAS a KNOWN_LARGE, and python lacked it. That changed the message `test_unlisted_files_keep_the_plain_limit` pins, so the expectation is updated in full rather than loosened to a substring -- it is the only test pinning it. Also restored `unmeasured`'s rationale in the shell copy, reduced to one line there. Lost in the copy, not superseded. Tested across BOTH gates, and proven by reverting the fix --------------------------------------------------------- New `repo_tests/gate_reporting_parity_17377_test.py` asserts the shared contract against both gates, parameterised, because every one of these differences was invisible precisely because each gate's tests only looked at that gate. with the fixes: 10 passed gates reverted: 7 failed, 3 passed The three still passing are all `[python]` -- the cases where python was already correct -- so the "python was right" / "shell was right" split is evidenced rather than asserted. 80 passed across all four affected suites. Why consolidation is a separate PR ---------------------------------- The natural prior is backwards, and that is the strongest reason for this order. "The Python gate is the mature one, the shell gate is the copy" is what anyone reaching for a shared helper would assume -- and across this batch the SHELL copy was the correct one four times out of five: unmeasured reported on the audit path shell right verdict's KNOWN_LARGE guidance shell right the seen/reached double-report shell right (python had it too) KNOWN_LARGE == RATCHET_BASELINE equality shell right (#17872) log level, stream, reach-breach shape python right So consolidating on the prior picks the wrong copy more often than the right one, and each wrong pick looks deliberate afterwards. That is not an argument about tidiness order; it is the difference between a helper that preserves behaviour and one that silently adopts four defects. Two of the six items go the other way, so there is no canonical gate to extract from: either copy lifted wholesale carries two defects. A shared helper would have made one gate's behaviour the other's BY ACCIDENT and erased the evidence they ever disagreed, and the surviving copy would look deliberate. Twelve shared function names with exactly one byte-identical body is structural duplication, not textual, so extracting the identical half leaves two copies of one algorithm that no longer even look alike. #17402 is NOT in this PR: investigating it showed its premise does not hold. Its reach counter IS read, by a fixture test over every flagged shape that fails when the detector's subject narrows. Closed as not-a-defect with that evidence. Refs #17377, #14975, #17353

…ile under 600 (#17377) Found by the overlap check before pushing, not by CI. #17882 landing changed the answer: it took `python_file_size_ratchet_test.py` to 597 lines, and this branch's hunk in the same file took it to 603 -- past the 600 hard limit, in a file with no KNOWN_LARGE entry. Two of my own commits, each correct alone, colliding on a file at its cap. The rebase was clean; the size was not. Not fixed by recording an entry for the file, which would be grandfathering a test file to fit a docstring. The full-message assertion moved to `gate_reporting_parity_17377_test.py`, which has room AND is parametrised over both gates -- so a sentence both gates emit is now pinned once over both rather than once in the Python-only file. The capped file keeps a 3-line prefix assertion, which is what it had before this branch touched it. The dash was my drift, not theirs --------------------------------- My new Python sentence used ASCII "--" while the shell gate used an em dash. Both gates use em dashes throughout their messages (21 and 13 occurrences), so the ASCII one was the inconsistency this PR exists to remove. The two sentences are now textually identical apart from each gate's own SELF_REL, and the assertion pins the dash: reverting the shell gate to "--" fails [shell]. _scan_tracked_files was over the standard, and it was my doing -------------------------------------------------------------- This branch had grown it from 21 to 37 lines (python) and 27 to 35 (shell) -- the 31-50 "consider refactoring" band, and both grew because I inlined two rationales. Extracted `_report_unmeasured(rel, seen, problems)` in both gates, carrying the rationale with it, since "classify a file we could not measure" is one idea: _scan_tracked_files 37 -> 22 (python) longest function in the file: 24 35 -> 21 (shell) 23 Mutation-verified, each mutation confirmed applied by grep first: python gate's sentence reverted -> 1 failed [python] shell gate's dash back to ASCII -> 1 failed [shell] seen.add dropped from the helper -> 4 failed (both gates, both tests) problems.append(unmeasured) dropped -> 4 failed (both gates) restored -> 67 passed, both gates exit 0

coderabbitai · 2026-10-03T00:48:50Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 28 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 0d41b8e5-9aa0-41bf-b26b-c85a8970fc19

📥 Commits

Reviewing files that changed from the base of the PR and between a26655b and f0e6704.

📒 Files selected for processing (3)

repo_tests/gate_reporting_parity_17377_test.py
scripts/check_python_file_size.py
scripts/check_shell_file_size.py

📝 Walkthrough

Walkthrough

The Python and shell file-size gates now report unreadable files without counting them as reached. The shell gate reports accumulated findings together. The Python gate adds guidance for oversized files. New parity tests cover reporting and guidance.

Changes

File-size gate reporting

Layer / File(s)	Summary
Unreadable-file tracking `scripts/check_python_file_size.py`, `scripts/check_shell_file_size.py`, `repo_tests/gate_reporting_parity_17377_test.py`	Both gates mark unreadable paths as seen, report them as findings, and exclude them from the reached count. Tests check unreadable tracked and grandfathered paths.
Finding and logging behaviour `scripts/check_shell_file_size.py`, `repo_tests/gate_reporting_parity_17377_test.py`	The shell gate logs accumulated findings at error level and returns failure when problems exist. Tests check stderr logging, reach-floor findings, and unreadable commit-path arguments.
Oversized-file guidance `scripts/check_python_file_size.py`, `repo_tests/gate_reporting_parity_17377_test.py`, `repo_tests/python_file_size_ratchet_test.py`	The Python gate tells contributors not to add oversized files to `KNOWN_LARGE`. Tests check the guidance and allow the verdict to include additional text.

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to a2665

The gates appear mergeable with bounded test-coverage risk: the new tests could miss duplicate grandfathered-file findings or a regression in finding severity.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 90.91% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 4 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: aligning reporting between the two file-size gates and handling unreadable files. It is longer than ideal but remains specific and understandable.

✨ Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 229072cf-cdd6-4301-b198-27b14d8dcf4d

📥 Commits

Reviewing files that changed from the base of the PR and between 8a70979 and a26655b.

📒 Files selected for processing (4)

repo_tests/gate_reporting_parity_17377_test.py
repo_tests/python_file_size_ratchet_test.py
scripts/check_python_file_size.py
scripts/check_shell_file_size.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

github-actions · 2026-10-03T00:55:05Z

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

…tence (#17377) Review found the level unpinned and two message branches still drifting. Both are fixed, and taking the MEDIUMs rather than deferring them was the call: the PR's stated subject is reporting parity, so "aligned and pinned" while three branches drift is a claim exceeding what is enforced -- the defect this branch exists to remove. Pinning text per branch would have been the obvious fix and the weaker one. The new test instead compares the two gates' messages with everything legitimately per-gate abstracted away -- paths, ceilings, the floor constant, SELF_REL, RATCHET_REL -- over all seven branches: over-ceiling, under-ceiling, now-compliant, unlisted-oversized, unmeasured, vanished-entry, reach-breach. That is #17377's identical-behaviour criterion stated as an assertion instead of prose, and a future rewording passes only if BOTH gates are reworded together. What it found that nobody had reported -------------------------------------- `RATCHET_REL` in the python gate named `python_file_size_ratchet_test.py`, whose `RATCHET_BASELINE` is a single re-export line (`:76`); the entries live in `python_file_size_ratchet_baseline.py`. So "lower the matching RATCHET_BASELINE entry in <that file>" sent the developer to a file with nothing to edit -- the wrong-copy failure #17872 exists to prevent, emitted by the gate itself. The wording test cannot see it, because it normalises RATCHET_REL away precisely because it legitimately differs per gate, so that pointer gets its own test: the file named must DEFINE the mapping, which a re-export does not, and must hold at least as many entries as the hook. Also three serial commas, not the one reported: python wrote "in {SELF_REL}, and its RATCHET_BASELINE" in the now-compliant, under-ceiling and vanished-entry messages where shell wrote no comma. Systematic, not random -- one author's habit against another's. The two review items, and what I did with each --------------------------------------------- * The level: `test_findings_go_to_stderr` pinned the stream only, so dropping `logger.setLevel(logging.INFO)` passed every test here. Findings are ERROR and survive a default logger, so what vanishes is the informational "all live and at size" summary under lastResort's WARNING floor. The python gate had a test for that and **the shell gate had none**, so for one of the two gates the line was unguarded. Now asserted for both. * Test pollution: `configure_logging()` adds a stderr handler and sets INFO on a logger that `logging.getLogger(name)` returns as the SAME object for every load, and nothing removed it. Isolation went into the FIXTURE so every consuming test inherits it, restoring handlers and calling `setLevel` -- not a plain attribute write, which would leave logging's effective-level cache stale. The `(#14236)` citation went the other way, deliberately: it is dropped from python's over-ceiling message rather than added to shell's. The shell gate's own header cites #14236 as the origin of the *python* ratchet ("so `.py` files cannot grow"), so putting it in a shell-file message would attribute that gate's rule to the wrong issue. The provenance stays in python's module docstring, where a reader looking for history goes. Mutation-verified, each confirmed applied by grep before its result was trusted: RATCHET_REL reverted to the test file -> 1 failed (the pointer test) one serial comma re-added -> 1 failed (that branch) setLevel(INFO) dropped -> 1 failed (the level test) the reach hint stripped from shell -> 1 failed (reach-breach branch) restored -> 78 passed, both gates exit 0 Longest function in either gate: 23 lines. `python_file_size_ratchet_test.py` unchanged at 597.

…NING (#17377) Both review threads were right, and both are the same defect as the code they test. The double-report test reimplemented the pass it was testing -------------------------------------------------------------- `test_an_unmeasured_grandfathered_file_is_reported_once` called `_scan_tracked_files` and then computed the `KNOWN_LARGE - seen` pass ITSELF, so it asserted that my reconstruction reports once. A wiring change inside `audit_ceilings` -- the function whose double-report this branch fixes -- would have left it green. The test for the fix had the shape the fix exists to remove. It now drives `audit_ceilings()` with a real non-UTF-8 probe, which is the reachable case: the file EXISTS so the walk sees it, and cannot be decoded so it is never ruled on. `repo_root` and the tracked-files function are injected, the probe is the only KNOWN_LARGE key, and the assertions are that exactly one finding comes back, that it is the unmeasured one, and that it does NOT also say "moved or was deleted". A merely missing path is the other case and keeps its own test. >= WARNING accepted the regression this batch removed ----------------------------------------------------- Both level assertions read `min(levelno) >= logging.WARNING`. The contract is ERROR -- and WARNING is exactly what the shell gate used before this batch -- so the assertions would have passed a revert of one of the three alignments the PR claims. Now `{r.levelno for r in caplog.records} == {logging.ERROR}` on both the audit and commit paths. Mutation-verified: findings logged at WARNING in both gates -> 3 failed seen.add dropped from _report_unmeasured -> 4 failed (both gates, both tests) restored -> 23 passed The second mutation is worth recording because my first attempt at it was vacuous and `grep -c` did not catch that. I mutated `seen.add(normalise(rel))` at 8-space indent -- the MEASURED path's copy inside the loop -- not the 4-space one in `_report_unmeasured`. A replacement happened, grep confirmed it, and the suite stayed green because no test here exercises the measured path. Confirming that a mutation applied is not confirming that the INTENDED mutation applied; the line has to be identified by its enclosing function, not by its text.

mrveiss · 2026-10-03T07:23:24Z

Correction: the "gates run read-only: exit 0" line in this PR's body was a vacuous measurement

Found while starting #17377's engine extraction, by chasing a number I could not explain. Correcting it here because it is a claim about evidence, and it appears in three of my PRs tonight — #17882, #17884 and #17887.

What I ran:

python scripts/check_python_file_size.py   ->  exit 0

What that does: main(argv) checks for --audit-ceilings, and without it loops over argv to size the files named on the command line. With no arguments there is nothing to loop over, so it configures logging and returns 0. It audits nothing and cannot fail. Exit 0 from a no-op, reported as the gate being satisfied.

What the real invocation says, on the same tree:

python scripts/check_python_file_size.py --audit-ceilings
exit 0
python-file-size ceilings: 6007 files scanned, 479 grandfathered, all live and at size.

So the conclusion was true — the tree is clean, 6007 scanned and 479 grandfathered — and the evidence I gave for it was worthless. The same distinction I drew about a CI-occupancy claim earlier tonight: a true conclusion and a supported one are different, and the instrument has to be the thing that says so.

Nothing in the change itself is affected. The ratchet and parity tests were always the real evidence here and they ran properly: repo_tests/python_file_size_ratchet_test.py 31 passed, and this PR's own test files as reported. The gate line was a redundant check that turned out to be measuring nothing, not the check the change rested on.

How it hid: --audit-ceilings is the auditing path and the bare invocation is the commit path, which takes filenames. Running a file-size gate with no filenames is asking it about no files. The tell was available and I did not take it — I read "exit 0" as "the gate is happy" rather than asking what it had been given to look at.

…tion (#16502) `_build_delete_session_response` has always set `knowledge_context_cleanup` (#16490). `SessionDeleteData` did not declare it and carries no `extra: allow`, so Pydantic's default `extra="ignore"` dropped the key at the step `response_model=DataResponse[SessionDeleteData]` runs. The caller never learned whether the chat-knowledge context was removed, while the other four cleanups were reported. The handler was never wrong, which is why this survived: a test over its return value passes throughout the defect. Only the SERIALIZED shape shows it. Making room first, because the ceiling is lowered and never raised. `schemas_chat.py` sat at exactly 732. The five chat-folder models are the most cohesive group in it and serve `api/chat_folders.py` alone -- nothing else imports them -- so they move to `api/schemas_chat_folders.py` at the cost of one import line at the single call site. 732 to 702, and both recorded ceilings are lowered together: `repo_tests/` and `scripts/` disagreeing is what #17884 was about. NOT re-exported from `schemas_chat`. A re-export leaves two names for one object -- harmless for a class, fatal for anything a test patches -- and would keep the lines removing them was the point of. `FolderData` and `FolderListData` have no consumer anywhere in the tree. Moved rather than deleted: unwired code is completed or recorded, not swept. `KnowledgeContextCleanupResult` mirrors exactly what `_cleanup_chat_knowledge_context` returns -- `context_deleted` and `file_associations_removed`, and no `cleanup_error`, unlike its siblings, because that producer has no error branch to report. Declaring a field nothing sets would be inventing a contract. Two tests, both asserting the serialized shape. The contrast case is the load-bearing one: without it, "the key is present" is satisfiable by a model that always emits it regardless of input, which hides the opposite defect -- a caller told the context was handled when nothing ran. Mutation-proved under the sanctioned venv: removing the field fails both tests by name; restored, 8 pass. Size ratchet 31 pass. Move verified by AST: all five classes present in the new module, none left behind, and all three touched files compile. Closes #16502

mrveiss added 2 commits October 3, 2026 03:41

coderabbitai Bot reviewed Oct 3, 2026

View reviewed changes

Comment thread repo_tests/gate_reporting_parity_17377_test.py Outdated

Comment thread repo_tests/gate_reporting_parity_17377_test.py Outdated

mrveiss added 2 commits October 3, 2026 04:11

mrveiss merged commit 370a347 into main Oct 3, 2026
66 checks passed

mrveiss deleted the issue-17377-gate-drift branch October 3, 2026 01:45

Navigation Menu

Repository navigation

fix(gates): the two file-size gates disagreed about how to report a finding, and one skipped files it could not read (#17377) - #17884

fix(gates): the two file-size gates disagreed about how to report a finding, and one skipped files it could not read (#17377)#17884
mrveiss merged 4 commits into
mainfrom
issue-17377-gate-drift

mrveiss commented Oct 3, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 3, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Oct 3, 2026

Uh oh!

Uh oh!

mrveiss commented Oct 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mrveiss commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thinking Path

What Changed

Review round: both MEDIUMs taken rather than deferred, and what that found

Verification

#17377's criteria that remain OPEN after this PR

Single-issue rationale

Model Used

Uh oh!

coderabbitai Bot commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Oct 3, 2026

✅ SSOT Configuration Compliance: Passing

Uh oh!

Uh oh!

mrveiss commented Oct 3, 2026

Correction: the "gates run read-only: exit 0" line in this PR's body was a vacuous measurement

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mrveiss commented Oct 3, 2026 •

edited

Loading

coderabbitai Bot commented Oct 3, 2026 •

edited

Loading