feat: baseline diff + quality gate (Own.NET Auditor phase 2) - #8
Conversation
A debt ratchet — fail CI on NEW findings only, never the ~72k accepted legacy. - report/baseline.py: diff a saved baseline against current findings into new/fixed/net, and a severity-aware gate. Finding identity is the SAME stable, line-independent fingerprint the SARIF exporter uses (rule + path + normalized message, per-occurrence), so the gate and GitHub's code-scanning alerts agree on what's "the same finding". Whole-corpus set-difference, not the fix-arm's distance-based diff_findings (which is for a single fix's before/after). - report/diff_cli.py: --save-baseline writes a compact, commit-friendly record (fingerprint + rule/path/cat/tool, no long messages); the gate writes diff.json + diff.md and exits 2 when a NEW finding is at/above --gate-level (default warning; --report-only never fails). A line shift or an identifier/count change in the message is not counted as new. - 7 tests (new/fixed, line-shift-not-new, duplicate multiset, gate by level, compact-record round-trip, CLI save+gate), green under -O. Verified on the real 72,569-finding corpus: self-diff = 0/0 PASS; a simulated run (+1 error, -1) = 1 new / 1 fixed / net 0 but gate FAILS (new error-level debt). Baseline is a user-managed artifact (created on the stand), gitignored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds fingerprint-based baseline diffing for audit findings, a CLI to save or compare baselines, JSON and Markdown diff outputs, severity gating, updated docs, an ignore rule for the baseline artifact, and tests for the diff and CLI flow. ChangesOwn.NET Auditor baseline diff and gate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@report/diff_cli.py`:
- Around line 26-28: The `_load_findings` helper currently lets a missing or
malformed `--current` file raise uncaught `FileNotFoundError` or `KeyError`,
unlike the baseline path handling in the CLI. Update `_load_findings` in
`diff_cli.py` to validate the file load and the presence of the "findings" key,
then surface a clean user-facing error and exit consistently with the existing
graceful failure path used by the main CLI logic.
🪄 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: 0104bea4-537f-483d-b222-d484705ba7c0
📒 Files selected for processing (5)
.gitignoredocs/own-net-auditor.mdreport/baseline.pyreport/diff_cli.pyreport/tests/test_baseline.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff2b0afc3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| base = _fingerprint(f) | ||
| k = seen.get(base, 0) | ||
| seen[base] = k + 1 | ||
| out[base if k == 0 else f"{base}/{k}"] = f |
There was a problem hiding this comment.
In occurrence_fingerprints, findings with the same rule/path/normalized message are reduced to the indistinguishable keys base, base/1, etc. When a PR fixes one legacy occurrence and introduces a different occurrence with the same fingerprint in the same file, the duplicate count stays unchanged, so diff() reports no new finding and the quality gate passes even though new debt was added. This affects repeated diagnostics with identical messages, including warning/error categories, so the baseline needs an additional stable per-occurrence discriminator or a location-aware duplicate match instead of comparing only the generated suffix set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex You've correctly described the mechanics, but I'm going to push back on the fix, because the proposed remedy is information-theoretically impossible without breaking the gate's core property.
The scenario — fix one occurrence and add another with the same rule + path + normalized message** in the same file — produces two findings that are *literally indistinguishable*: same rule, same file, same normalized message, differing only in line. The **only** available per-occurrence discriminator is the line number. But the fingerprint is deliberately line-**independent**, because a debt gate that keys on line registers every finding below an inserted line as fixed+newon any PR that adds lines — i.e. ordinary code movement floods the gate with false "new debt" and it fails on essentially every PR. That churn is the single biggest failure mode of naive line-based diff gates, and avoiding it is the whole reason for the line-independent identity (it's also exactly the trade-off GitHub code scanning makes withpartialFingerprints`). A "fix-one-add-one-identical" and a "move-one" are the same event under this identity — there is no signal to separate them.
So the gate tracks the count per (rule, path, message) bucket, which is the meaningful debt measure: 3→4 in a bucket is caught as 1 new (base/3 ∉ baseline); 3→2 is caught as 1 fixed; 3→3 means the file still has the same number of the same diagnostic — actionable state unchanged, no net new debt. The swap you describe leaves the bucket count at 3; the file is no better and no worse for that diagnostic.
I'd argue that's the correct behaviour for a count ratchet, so I'm keeping the line-independent identity by default. If strict location-level ratcheting is wanted (accepting the move-churn), that's a clean --line-aware opt-in I can add later — but it shouldn't be the default. Marking 👎 on the suggested change, with thanks for the precise write-up.
Generated by Claude Code
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
_load_findings now catches OSError/ValueError/KeyError and exits 2 with a message, symmetric with the missing-baseline path — CI gets an error, not a traceback (CodeRabbit). Test test_cli_missing_current_fails_cleanly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
There was a problem hiding this comment.
🧹 Nitpick comments (2)
report/diff_cli.py (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueChain the re-raised
SystemExitto silence B904.Ruff flags raising inside an
exceptclause without explicit chaining. Usefrom None(the original message is already surfaced via the printede) to keep the traceback clean.♻️ Proposed tweak
except (OSError, ValueError, KeyError) as e: print(f"error: cannot read findings from {path!r}: {e}", file=sys.stderr) - raise SystemExit(2) + raise SystemExit(2) from NoneSource: Linters/SAST tools
report/tests/test_baseline.py (1)
126-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: avoid the hardcoded
/tmp/x.jsonbaseline path.The test is correct —
_load_findingsfails on the missing--currentbefore the baseline is ever read, so exit 2 is exercised as intended. The hardcoded/tmppath (flagged by Ruff S108) is harmless here since it's never opened, but using a non-existent path undertempfile.gettempdir()would silence the linter and be portable across OSes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@report/tests/test_baseline.py` around lines 126 - 133, The test in test_cli_missing_current_fails_cleanly uses a hardcoded /tmp baseline path, which triggers the Ruff S108 warning even though it is never opened. Update the diff_cli.main call in this test to use a non-existent path built from tempfile.gettempdir() instead, so the fixture remains portable and the lint warning is avoided while still exercising the missing --current exit path.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@report/tests/test_baseline.py`:
- Around line 126-133: The test in test_cli_missing_current_fails_cleanly uses a
hardcoded /tmp baseline path, which triggers the Ruff S108 warning even though
it is never opened. Update the diff_cli.main call in this test to use a
non-existent path built from tempfile.gettempdir() instead, so the fixture
remains portable and the lint warning is avoided while still exercising the
missing --current exit path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 00eb720b-5703-4d98-87eb-80905ea4f260
📒 Files selected for processing (2)
report/diff_cli.pyreport/tests/test_baseline.py
- diff_cli: raise SystemExit(2) from None (the original error is already in the printed message), silencing Ruff B904. - test: build the non-existent paths from tempfile.gettempdir() instead of a hardcoded /tmp, silencing Ruff S108 and staying portable. Both trivial; tests 8/8 (incl. -O). CodeRabbit nitpicks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
|
@coderabbitai both nitpicks addressed in b18b453:
Both trivial; tests 8/8 (incl. Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
🐇✅ |
2 participants
Фаза 2 дорожной карты — debt-ратчет
Гейт «валим только за новый мусор», не за ~72k принятого легаси. Продолжает Фазу 1 (SARIF-экспортёр) из #7.
report/baseline.py— diff + gateСравнивает сохранённый baseline с текущими находками →
new / fixed / net+ severity-aware гейт.Идентичность находки — тот же стабильный fingerprint, что у SARIF-экспортёра (
rule + path + нормализованное сообщение, per-occurrence disambiguated). Поэтому:Это set-difference по всему корпусу — намеренно не
diff_findingsиз fix-arm (тот distance-based, для before/after одного применённого фикса на одном дереве, а не для сравнения двух прогонов аудита).report/diff_cli.py— CLI--save-baselineпишет компактный record (fingerprint + rule/path/cat/tool, без длинных сообщений);diff.json+diff.md;--gate-level note|warning|error(дефолтwarning);--report-onlyникогда не валит сборку.Семантика ратчета
Гейт смотрит на новизну, а не на net: можно починить одну находку и внести другую (net 0) — сборка справедливо упадёт за новую.
Test plan
Зелёные под
python3иpython3 -O.Проверено на реальных 72 569 находок: self-diff = 0 new / 0 fixed → PASS (exit 0); сэмулированный прогон (+1
CA2000error, −1 находка) = 1 new / 1 fixed / net +0, но гейт FAIL (exit 2) — новый долг error-уровня валит сборку.Baseline (~13MB на 72k) — пользовательский артефакт, создаётся на стенде, в
.gitignore. Докаdocs/own-net-auditor.md§3 — Фаза 2 помечена ✅.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
diff.jsonanddiff.md, supports saving a baseline, and returns a non-zero exit code when the gate fails (unless report-only mode is used)..gitignoreto exclude the user-managed baseline artifact.