Skip to content

fix(ci): re-measure the files.py log-site census after #7293 - #7492

Merged
iamwhatever merged 1 commit into
mainfrom
fix/log-census-files-slack
Sep 1, 2026
Merged

fix(ci): re-measure the files.py log-site census after #7293#7492
iamwhatever merged 1 commit into
mainfrom
fix/log-census-files-slack

Conversation

@chenmingwei23

Copy link
Copy Markdown
Contributor

1. What is the problem?

main is red. test_security_posture.py::TestGateSideLogRedactorSpelling::test_the_census_holds_no_slack fails:

AssertionError: `_BASELINE_LOG_SITE_CENSUS` is now looser than the code --
lower or drop these: dashboard/handlers/files.py: 1 sites, census says 3

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 Tests shard and cannot reach a
green PR Readiness. Reproduced on pristine origin/main at dd9e002bf, not
only 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 sites
into one _audit_file_send helper. #7293 landed first; #7278 then merged with
the already-stale 3 in its census, and nothing re-measures the census at merge
time -- so main merged red rather than any single PR breaking it. No commit has
touched files.py since #7278 landed, which is what rules out a later
regression.

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_urls pairs: both feed _audit_file_send(...), a local
helper, rather than a logger.* or sel().log_* call in the same function, so
the 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 -- 5
passed. Mutation-verified in BOTH ratchet directions, which is the point of a
two-way ratchet:

Census value Result
3 (main today) test_the_census_holds_no_slack FAILS -- the bug
0 (over-lowered) test_no_new_gate_side_log_line_reads_the_baseline_redactor FAILS: files.py: 1 sites, census says 0
1 (this PR) all 5 pass

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 --update flag; or drop the literal and assert
only 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 main after
each 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.

`_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.
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 43fa74c14181b019533f82f05d7c520eb16b9bbd and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 43fa74c

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 43fa74c14181b019533f82f05d7c520eb16b9bbd: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 43fa74c14181b019533f82f05d7c520eb16b9bbd — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Verified: the scanner (_is_log_write) only matches logger-level methods and log_*-named calls, so the two safe_error redaction sites feeding _audit_file_send are invisible to it post-#7293 — the live count is genuinely 1, and the census entry of 3 was measured on the pre-consolidation tree. The fix is exactly what the failing two-way ratchet prescribes, and the PR pins the value from both directions.

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.

Watch

The 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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 43fa74c14181b019533f82f05d7c520eb16b9bbd — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 43fa74c

Verdict parsed from the review's SHA-scoped output markers for commit 43fa74c14181b019533f82f05d7c520eb16b9bbd.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 43fa74c14181b019533f82f05d7c520eb16b9bbd: <one-sentence reason>

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

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 dd9e002bf. Main has since advanced to c28a2199b. I ran the census tests on a throwaway detached worktree at pristine c28a2199b:

FAILED test_the_census_holds_no_slack - `_BASELINE_LOG_SITE_CENSUS` is now looser
  than the code - lower or drop these: dashboard/handlers/files.py: 1 sites, census says 3
1 failed, 1 passed

Same single entry, same count. Applying this PR's one-line change in that worktree: 2 passed for the census pair, and 5 passed for the whole TestGateSideLogRedactorSpelling class including the teeth test. So the number has not gone stale under the burst merges that landed after this PR was authored.

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. test_the_census_holds_no_slack builds slack over every census entry and reports them all in one assertion message, so a second drifted module would surface in the same failure. On c28a2199b only dashboard/handlers/files.py does. This PR's one line is the complete correction, and widening it would be scope for its own sake.

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 .tsx, one new .test.tsx and four media files -- zero Python. A frontend-only diff cannot change a Python log-site count, which is what establishes main ownership from the outside. Each of those PRs also shows Coverage Gate and PR Readiness red as downstream aggregates, so this one line clears three rows per PR.

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 cancelled/null after being superseded by the next merge -- so the ceiling was never actually observed on them. A push: main run of the ratchet suite would have caught both this and #7499 at the moment they landed.

@iamwhatever iamwhatever left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@iamwhatever
iamwhatever merged commit ecab0ba into main Sep 1, 2026
68 checks passed
@iamwhatever
iamwhatever deleted the fix/log-census-files-slack branch September 1, 2026 05:07
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 1, 2026

@dwu96 dwu96 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

atomsbaza added a commit to atomsbaza/KiroCrew that referenced this pull request Sep 1, 2026
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.
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.

4 participants