fix(ci): re-measure the files.py log-site census after #7293 - #7492
Conversation
`_BASELINE_LOG_SITE_CENSUS` records 3 gate-side baseline-redactor sites for `dashboard/handlers/files.py`; the scanner finds 1, so `test_the_census_holds_no_slack` fails on `main`. The census entry was measured on a tree that predated #7293, which consolidated that module's many `_sel().log_tool_invocation(...)` calls into one `_audit_file_send` helper. #7293 landed first and #7278 brought the already-stale number in with it; nothing re-measures the census at merge, so main merged red. Lower the entry to the count the scanner reports, which is what the failing assertion prescribes. No source change: the two surviving pair-idiom sites in that module feed `_audit_file_send`, not a logger or audit call directly, so the scanner does not count them.
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSDesign-level review of Verified: the scanner ( Design-Verdict: PASS Correct one-line unbreak of a red main: the census records reality, is mutation-pinned both ways, and the structural authoring-vs-merge-time defect is explicitly deferred to maintainers. WatchThe recurrence risk the PR's §5 names is real — any merge racing a census-touching PR re-creates this red-main class; the "print a copy-pasteable corrected dict on failure" option is cheap and worth a maintainer picking up soon. [DESIGN-REVIEWED] 43fa74c |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
|
Independent corroboration from a second session that root-caused the same red while shepherding five rebased PRs. I was about to open a duplicate micro-PR for this and stood down; posting the measurements instead, because two of them cover ground this PR does not. 1. The value still holds at a newer main. This PR reproduces on Same single entry, same count. Applying this PR's one-line change in that worktree: 2. There is no sibling drift to fold in. Worth stating explicitly, because it is easy to assume a burst of merges caused several baseline drifts at once. 3. Blast radius, for whoever prioritises this. The red is not confined to one shard row. I confirmed it on five PRs with mutually disjoint diffs (#7299, #7266, #7295, #7285, #7238), including #7295 whose entire diff is one Two adjacent notes, both filed rather than folded here:
The section-5 diagnosis matches what I found independently: nothing re-measures a scan-derived baseline at merge time, and both offending commits' CI runs came back |
iamwhatever
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: test (1 file). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: test-only re-measure of the _BASELINE_LOG_SITE_CENSUS ratchet entry for dashboard/handlers/files.py (3 -> 1) after #7293 removed two gate-side baseline log sites; the census is two-sided (growth fails, and a baseline looser than the code also fails), so lowering the number tightens the ratchet and cannot relax it.
dwu96
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: test (1 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: test-only one-line update to the _BASELINE_LOG_SITE_CENSUS ratchet table in test/test_security_posture.py, lowering dashboard/handlers/files.py from 3 to 1 after #7293 converted those log sites - a strictly tightening ratchet edit with no runtime code touched.
bolichen97
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: test (1 file). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: test-files-only re-measure of the gate-side log-site census after #7293 removed two sites in dashboard/handlers/files.py; the ratchet is double-directional (it fails on growth AND on a baseline looser than the code), so lowering 3 to 1 is the tightening the gate itself demands.
kirodotdev#7283 _BASELINE_LOG_SITE_CENSUS has no entry for dashboard/handlers/memory.py; kirodotdev#7279/kirodotdev#7283 added two gate-side baseline-redactor sites there (pip ensurepip bootstrap + faiss-cpu install stderr, logged via logger.warning in the dashboard process) after the census was measured, so test_no_new_gate_side_log_line_reads_the_baseline_redactor fails. Raise the census to 2 per the kirodotdev#7492 re-measure pattern. No source change.
1. What is the problem?
mainis red.test_security_posture.py::TestGateSideLogRedactorSpelling::test_the_census_holds_no_slackfails:The census is a two-way ratchet: one test forbids a module GROWING past its
recorded number, the sibling forbids the record staying ABOVE the code (dead
slots a future baseline line could slip into silently). The second one is
failing.
2. Why this issue matters to the user
It is not a flake and it does not clear on a retry, so every open PR on the
repository carries one permanently red
Backend Testsshard and cannot reach agreen
PR Readiness. Reproduced on pristineorigin/mainatdd9e002bf, notonly on a feature branch.
3. How our fix solves it
The number was measured on a tree that predated #7293, which consolidated
dashboard/handlers/files.py's many_sel().log_tool_invocation(...)call sitesinto one
_audit_file_sendhelper. #7293 landed first; #7278 then merged withthe already-stale
3in its census, and nothing re-measures the census at mergetime -- so main merged red rather than any single PR breaking it. No commit has
touched
files.pysince #7278 landed, which is what rules out a laterregression.
Lower the entry to the count the scanner reports, which is exactly what the
failing assertion prescribes ("lower or drop these"). One line, no source
change.
Why 1 and not 2, since the module still contains two
redact_credentials/redact_exfiltration_urlspairs: both feed_audit_file_send(...), a localhelper, rather than a
logger.*orsel().log_*call in the same function, sothe scanner's matcher does not count them. The security posture is unchanged
either way -- this PR moves a recorded number to match code that already exists.
4. What tests we did
pytest test/test_security_posture.py::TestGateSideLogRedactorSpelling-- 5passed. Mutation-verified in BOTH ratchet directions, which is the point of a
two-way ratchet:
3(main today)test_the_census_holds_no_slackFAILS -- the bug0(over-lowered)test_no_new_gate_side_log_line_reads_the_baseline_redactorFAILS:files.py: 1 sites, census says 01(this PR)So the value is pinned from above and below, not merely made green.
5. Any other suggestions on the work
The real defect is structural and outlives this line: a scan-derived baseline
committed as a literal is measured at authoring time and consumed at merge time,
so any PR that lands in between silently invalidates it. The growth direction is
safe (a stale-high number only ever under-reports), but the slack direction turns
an unrelated merge into a red main. Options, cheapest first: have the census test
print a copy-pasteable corrected dict on failure; or generate the census into a
committed snapshot file with a
--updateflag; or drop the literal and assertonly the growth direction, accepting the dead slots the slack test exists to
prevent. Worth one maintainer decision rather than a re-measure PR each time.
Pattern harvest
Rule candidate: review-prompt -- when a PR adds or edits a committed baseline
that was DERIVED by scanning the tree (a census, a count ratchet, a lint
allowlist with counts), ask whether it was re-measured against the merge base
rather than the authoring base. This class cannot be caught by reviewing the PR
in isolation, because the number is correct when written and wrong when merged.
Rule candidate: lint -- a CI job that recomputes such a baseline on
mainaftereach merge and opens (or fails loudly with) the corrected value would have caught
this within one merge instead of leaving main red for every open PR to trip over.