From e5f368b8c0cb9a22c52cebe3abd47c4cc9ea452e Mon Sep 17 00:00:00 2001 From: Tauan Binato <11513929+tauanbinato@users.noreply.github.com> Date: Sun, 4 Oct 2026 00:04:23 -0300 Subject: [PATCH] Keep fingerprints stable across merges, selections and renames A repeat is identified by its copies, a file outline by its path with its members matched by similarity, and a custom hunk by the definition around it and its own run of changed lines. A finding of a renamed file is also known under its old path. Baseline entries written before still accept their findings through the fingerprints they had then, and the next baseline write rewrites them, keeping reasons and notes. SARIF carries both fingerprints for a release. --- CHANGELOG.md | 9 + jevgate-baseline.json | 140 +++++ site/src/custom-questions.md | 2 +- site/src/output.md | 4 +- .../src/rules/maintainability/shared-logic.md | 6 +- src/baseline.rs | 496 +++++++----------- src/baseline/entries.rs | 145 +++++ src/baseline/listing.rs | 181 +++++++ src/changes.rs | 44 +- src/hook/events.rs | 3 +- src/hook/review.rs | 6 +- src/mcp/tools.rs | 4 +- src/sarif.rs | 21 +- src/schema/mod.rs | 2 +- src/schema/report.rs | 43 +- src/tests/mod.rs | 1 + src/units/compose/comments.rs | 8 +- src/units/compose/identity.rs | 94 ++++ src/units/compose/mod.rs | 37 +- src/units/compose/redundant.rs | 8 +- src/units/custom/examples.rs | 2 +- src/units/custom/hunks.rs | 206 ++++++-- src/units/custom/items.rs | 42 +- src/units/custom/mod.rs | 8 +- src/units/duplicates.rs | 52 +- src/units/grouping.rs | 2 + src/units/mod.rs | 25 +- src/units/outcome/mod.rs | 4 +- src/units/outline.rs | 12 +- src/units/plan/mod.rs | 16 +- src/units/tests/changed.rs | 58 +- src/units/tests/custom.rs | 40 +- src/units/tests/fingerprints.rs | 226 ++++++++ src/units/tests/mod.rs | 10 + src/units/tests/organization.rs | 22 + src/units/wording/maintainability.rs | 2 +- 36 files changed, 1536 insertions(+), 445 deletions(-) create mode 100644 src/baseline/entries.rs create mode 100644 src/baseline/listing.rs create mode 100644 src/units/compose/identity.rs create mode 100644 src/units/tests/fingerprints.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e31734..c57e268 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver ## [Unreleased] +Fingerprints stay the same across merges, stacked branches and renames, so a dismissed finding is not raised again because main moved underneath it or a check selected other files. An upgraded baseline keeps accepting every finding it accepted, and its next write moves each entry to its new fingerprint with its reason and note: expect one large `jevgate-baseline.json` diff. + +- A shared-logic finding is identified by its copies, each function that holds one by path and name, and a copy outside a function by its path and statements: no longer by which file a check selected, which pair represents a group or how far the repeated window reaches. A baseline accepts a repeat while every copy is one an accepted repeat names, so a copy that goes leaves it accepted and a new copy that joins raises it again. +- A file-organization finding is identified by its file's path, and a baseline accepts it while at least 80% of its member names are the ones accepted; a file that grew a lot is asked about again. +- A custom `hunk` question asks about each run of changed lines on its own, as a diff without context lines has it, identified by the definition JevGate's parser finds around it and the lines it changes, not Git's hunk header: a change nearby, such as a parent branch's, or a function added above leaves it as it was. +- With `--base` and in an agent's turn, a finding of a file the change renamed is also accepted under its old path. +- Baseline entries of shared-logic and file-organization findings record their `members`. An entry written by 0.35 or earlier still accepts its finding through the fingerprint it had then; `jevgate baseline`, `--merge` and `baseline mark` rewrite it to the new one. The JSON report gives such a finding's earlier fingerprint as `fingerprint_v1`, its fingerprints under a renamed file's old path as `aliases`, and its `members`. +- SARIF results carry `jevgateFingerprint/v2` beside `jevgateFingerprint/v1`, which holds the earlier fingerprint where it changed, so code-scanning alerts stay the same alerts across the upgrade. GitLab Code Quality reports and MCP ids use the new fingerprint. + ## [0.35.0] - 2026-10-03 A repeat found by a `--base` check or an agent's turn points at the change's own copy and names the copies it left untouched, so a change fixes its own copy without rewriting code it never touched. Fingerprints, and what fails the gate, are unchanged. diff --git a/jevgate-baseline.json b/jevgate-baseline.json index 40cd928..8437552 100644 --- a/jevgate-baseline.json +++ b/jevgate-baseline.json @@ -427,6 +427,16 @@ "message": "This file may do several separate kinds of work, such as separate features, layers or integrations.", "reason": "wrong" }, + { + "fingerprint": "5dc95562dd420d856dd0a09a3e6a44afcd14cef3aabe34d0dd9ca73d8f66e7a3", + "rule": "maintainability/file-organization", + "path": "src/baseline.rs", + "line": 19, + "strength": "review", + "message": "This file may do several separate kinds of work, such as separate features, layers or integrations.", + "reason": "intended", + "note": "the baseline file's reads and writes; this PR moves matching and listing to baseline/entries.rs and baseline/listing.rs" + }, { "fingerprint": "89c1deedd5fe8507aad0a494fda37caae01d5a80be6d0590ca591092f0592dbe", "rule": "maintainability/file-organization", @@ -437,6 +447,17 @@ "reason": "wrong", "note": "every function reads or writes jevgate-baseline.json; marked wrong on main before" }, + { + "fingerprint": "d0d9af103ceb80928d4f43721e3c4abb763c76ad1babc37d992e36cbd11df897", + "rule": "maintainability/shared-logic", + "path": "src/baseline.rs", + "line": 358, + "unit": "`last_check` (src/baseline.rs:190) and `mark` (src/baseline.rs:358)", + "strength": "review", + "message": "`mark` (src/baseline.rs:358), which this change touched, may repeat logic that copies it left untouched also hold: `last_check` (src/baseline.rs:190, in a file this change edits).", + "reason": "wrong", + "note": "one read_latest call, optional for a mark and required for a write" + }, { "fingerprint": "b5974a3e8b0774e52416c53f1003281dc460cd55853041125750b311c3d71376", "rule": "maintainability/function-simplification", @@ -802,6 +823,17 @@ "message": "`question_tables` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", "reason": "intended" }, + { + "fingerprint": "40804709e7f925e3b46f2579ce4fcd2d9568214edf862e73c2a55caf656ebdac", + "rule": "maintainability/function-simplification", + "path": "src/hook/events.rs", + "line": 311, + "unit": "Hook::told", + "strength": "review", + "message": "`Hook::told` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", + "reason": "intended", + "note": "as on main; this PR changes one partition condition" + }, { "fingerprint": "417b704fd1318a0cca1d3dea37b13407764d0e5707b579e27ac017b7c880c0ae", "rule": "maintainability/function-simplification", @@ -1235,6 +1267,17 @@ "reason": "intended", "note": "as on main; this PR only adds the new empty untouched field" }, + { + "fingerprint": "758ba90e8cd677dd4f595cec6954a046c60fb0906f3195f62feb41173cccd4cf", + "rule": "maintainability/function-simplification", + "path": "src/units/compose/comments.rs", + "line": 44, + "unit": "comment_findings", + "strength": "review", + "message": "`comment_findings` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", + "reason": "intended", + "note": "as on main; this PR only names the identity and adds rename aliases" + }, { "fingerprint": "0426cf29bc82927d5ba0d17e12226e080d2c849266ad2a8909303cd6fc633099", "rule": "maintainability/function-simplification", @@ -1244,6 +1287,28 @@ "message": "`counted_status` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", "reason": "wrong" }, + { + "fingerprint": "04ff31dddb13b208c6d96a23cf0c263172b25248218f99a754d6a0d105697687", + "rule": "maintainability/shared-logic", + "path": "src/units/compose/mod.rs", + "line": 350, + "unit": "`undecided_unit` (src/units/compose/mod.rs:350) and `finding` (src/units/compose/mod.rs:787)", + "strength": "review", + "message": "`undecided_unit` (src/units/compose/mod.rs:350) and `finding` (src/units/compose/mod.rs:787) may repeat one piece of logic, so a change to it would have to be made in each place.", + "reason": "wrong", + "note": "a two-arm match on the custom question in two different outputs" + }, + { + "fingerprint": "64555705f5133a7b8372d67a8333eed3393f9e853c91b19b698db3cca8ddaa36", + "rule": "maintainability/shared-logic", + "path": "src/units/compose/mod.rs", + "line": 374, + "unit": "`finding` (src/units/compose/mod.rs:782), `group_finding` (src/units/compose/redundant.rs:127) and 1 more copy", + "strength": "review", + "message": "`undecided_unit` (src/units/compose/mod.rs:374), which this change touched, may repeat logic that copies it left untouched also hold: `finding` (src/units/compose/mod.rs:782, in a file this change edits); `group_finding` (src/units/compose/redundant.rs:127, in a file this change edits).", + "reason": "wrong", + "note": "one first-line expression in three struct literals" + }, { "fingerprint": "839519bcf4ed992fc3927e179fb57cb1d5fd5ea1f68d08f85c9d4d1f8f254db9", "rule": "maintainability/function-simplification", @@ -1273,6 +1338,28 @@ "reason": "intended", "note": "as on main; this PR only adds the new empty untouched field" }, + { + "fingerprint": "e4ab21fc2ce573a1a85c439f886b5e57b8ab8deb8aa200e826f94fd179bcb545", + "rule": "maintainability/function-simplification", + "path": "src/units/compose/mod.rs", + "line": 667, + "unit": "finding", + "strength": "review", + "message": "`finding` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", + "reason": "intended", + "note": "as on main; this PR only takes the fingerprint and identity from known()" + }, + { + "fingerprint": "819b49424c2d2fa8601c98ce04e340aba0f82f13576dd277fa02159ca1a5ad22", + "rule": "maintainability/function-simplification", + "path": "src/units/custom/hunks.rs", + "line": 189, + "unit": "split", + "strength": "review", + "message": "`split` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", + "reason": "wrong", + "note": "a short loop over the parts lines_now, runs and in_order compute" + }, { "fingerprint": "7b9c45a11bf97f35d12cf672422f6bb47560176fa5a5a91cd0618f98fed57922", "rule": "maintainability/shared-logic", @@ -1282,6 +1369,17 @@ "message": "`whole` (src/units/custom/items.rs:170), `plan_unit` (src/units/drift.rs:384) and 6 more copies may repeat one piece of logic, so a change to it would have to be made in each place.", "reason": "wrong" }, + { + "fingerprint": "9fd482e579cb61d310999ba136e44333c58e7af38d6bcf3893deeec372679199", + "rule": "maintainability/shared-logic", + "path": "src/units/custom/items.rs", + "line": 199, + "unit": "`comments` (src/units/custom/items.rs:116), `sections` (src/units/custom/items.rs:150) and 1 more copy", + "strength": "review", + "message": "`changed` (src/units/custom/items.rs:199), which this change touched, may repeat logic that copies it left untouched also hold: `comments` (src/units/custom/items.rs:116, in a file this change edits); `sections` (src/units/custom/items.rs:150, in a file this change edits).", + "reason": "wrong", + "note": "each kind zips its own items with their ids; the items differ" + }, { "fingerprint": "63f373d699dc0e64490e490447be4f918b909ade305fbbc5f0e524a216cb144c", "rule": "maintainability/function-simplification", @@ -1428,6 +1526,17 @@ "message": "`rule_outcome` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", "reason": "intended" }, + { + "fingerprint": "d4621f0ec7cfb7d2c46fa373015502193d63cca30378530ca13848ad34a6c5de", + "rule": "maintainability/function-simplification", + "path": "src/units/outcome/mod.rs", + "line": 258, + "unit": "rule_outcome", + "strength": "review", + "message": "`rule_outcome` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", + "reason": "intended", + "note": "as on main; this PR only adds a field to a pattern" + }, { "fingerprint": "0d67b1174cf848d492e1869b020379d045170a9ed8cd1a70111ef6573d3274d1", "rule": "maintainability/function-simplification", @@ -1466,6 +1575,17 @@ "message": "`parsed_scope` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", "reason": "intended" }, + { + "fingerprint": "ed7dae0e4679174d2696ad4d986f61fe7a0c86883018ec54cd2b606308b79a00", + "rule": "maintainability/function-simplification", + "path": "src/units/plan/mod.rs", + "line": 98, + "unit": "plan", + "strength": "review", + "message": "`plan` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", + "reason": "intended", + "note": "as on main; this PR only adds the note_renames call" + }, { "fingerprint": "fa6491af668dc8dc248218b34a7b9c7e88fd633565412eacf3d670dd6b91f4ab", "rule": "maintainability/function-simplification", @@ -1638,6 +1758,16 @@ "message": "This test file may test several separate subjects.", "reason": "wrong" }, + { + "fingerprint": "fb70b4fbd170ce7111d30aeedbb9490409aa0df579c613eb63d97e8fae913a26", + "rule": "maintainability/file-organization", + "path": "src/units/tests/custom.rs", + "line": 6, + "strength": "review", + "message": "This test file may test several separate subjects.", + "reason": "intended", + "note": "the custom question tests, one file as for every rule; this PR adds a rename test" + }, { "fingerprint": "b2e04f6d34e41c1ca8e363f836ca25f0515e2d01a5f17a06d56fdeab751fca7c", "rule": "maintainability/file-organization", @@ -1656,6 +1786,16 @@ "message": "A constant of this file may fix a value worth a look: one that differs between environments or singles out one record.", "reason": "intended" }, + { + "fingerprint": "0d6b6454a9185e789a8d4165909e449868ca000d3ec7c849157abcaa97b756ce", + "rule": "maintainability/file-organization", + "path": "src/units/tests/mod.rs", + "line": 34, + "strength": "review", + "message": "This test file may test several separate subjects.", + "reason": "intended", + "note": "the unit tests' shared helpers; this PR adds accept_all" + }, { "fingerprint": "76f152756739bb3b1c167f2ef55d71da86cf3e59f62983af21b177e3986b2231", "rule": "maintainability/file-organization", diff --git a/site/src/custom-questions.md b/site/src/custom-questions.md index 34cbd50..4931d7c 100644 --- a/site/src/custom-questions.md +++ b/site/src/custom-questions.md @@ -79,7 +79,7 @@ function auditOrder(req: Request) { } ``` -A finding keeps its fingerprint through unrelated edits, as the built-in rules' do: a function's by its name and code, a hunk's by what it changes and where. A file's is its path and text, so a baselined finding of a `file` question covers the file as it was: once the file is edited, the question is asked of it again, as it is of an edited function. +A finding keeps its fingerprint through unrelated edits, as the built-in rules' do: a function's by its name and code, a hunk's by the lines it changes and the definition around them. Each run of changed lines is a hunk of its own, shown with up to three unchanged lines on each side, so a change nearby, such as a parent branch's, never joins it. A file's is its path and text, so a baselined finding of a `file` question covers the file as it was: once the file is edited, the question is asked of it again, as it is of an edited function. ## Writing a question diff --git a/site/src/output.md b/site/src/output.md index 3c99303..a3c48b8 100644 --- a/site/src/output.md +++ b/site/src/output.md @@ -40,7 +40,9 @@ PORT = 4222 The report keeps the finding with its reason, it never fails the gate, and `jevgate baseline` leaves it out, so deleting the comment brings it back. -`jevgate baseline` can record why each finding was accepted: `intended` (right, and meant to be so), `later` (right, to fix later) or `wrong` (mistaken), with `--reason` or `jevgate baseline mark`. `baseline mark --note "#192"` keeps a one-line note with the reason, such as the issue a `later` finding will be fixed in. Reasons and notes survive later rewrites of the baseline. `jevgate baseline list --reason later --format md` prints the marks as a checklist for a cleanup issue (`text` and `json` too), and `jevgate baseline stats` reports each rule's share of findings marked wrong: labels from daily use, not the model's own probabilities. +`jevgate baseline` can record why each finding was accepted: `intended` (right, and meant to be so), `later` (right, to fix later) or `wrong` (mistaken), with `--reason` or `jevgate baseline mark`. `baseline mark --note "#192"` keeps a one-line note with the reason, such as the issue a `later` finding will be fixed in. Reasons and notes survive later rewrites of the baseline. + +A baseline matches findings by fingerprint, which a finding keeps through edits elsewhere, merges and changes to which files a check selects: a function's is its path, name and code, a [repeat](rules/maintainability/shared-logic.md#fingerprints)'s its copies, and a file outline's its path, with the baseline accepting it while at least 80% of its member names are the ones accepted (the entry's `members`). With `--base` and in an agent's turn, a finding of a file the change renamed is also accepted under its old path (`aliases` in the JSON report). An entry written by JevGate 0.35 or earlier still accepts its finding through the fingerprint it had then (`fingerprint_v1`), until `jevgate baseline`, `--merge` or `baseline mark` rewrites it to the new one with its reason and note. SARIF results carry both as `jevgateFingerprint/v1` and `jevgateFingerprint/v2`. `jevgate baseline list --reason later --format md` prints the marks as a checklist for a cleanup issue (`text` and `json` too), and `jevgate baseline stats` reports each rule's share of findings marked wrong: labels from daily use, not the model's own probabilities. ## Guards diff --git a/site/src/rules/maintainability/shared-logic.md b/site/src/rules/maintainability/shared-logic.md index 8524a20..3d4e4fa 100644 --- a/site/src/rules/maintainability/shared-logic.md +++ b/site/src/rules/maintainability/shared-logic.md @@ -10,7 +10,11 @@ Copies are compared within a package and across packages linked by a local depen ## Copies a change left untouched -With `--base`, and in an agent's turn, a repeat is asked about when the change touched at least one copy. When it left some copies untouched, the finding points at the change's own copy and names each untouched one, saying whether its file is one the change edits; the JSON report lists them under `untouched`, with `file_changed`. The change fixes its own copy, for example by reusing an untouched one; when sharing the logic would rewrite the untouched copies, it marks the finding `later --note "#issue"` and leaves them. The agent hook holds the end of a turn for the change's copy until it is fixed or marked, never for the untouched ones. The finding keeps the fingerprint its repeat has in a check of whole files, so a baseline accepts it in both. +With `--base`, and in an agent's turn, a repeat is asked about when the change touched at least one copy. When it left some copies untouched, the finding points at the change's own copy and names each untouched one, saying whether its file is one the change edits; the JSON report lists them under `untouched`, with `file_changed`. The change fixes its own copy, for example by reusing an untouched one; when sharing the logic would rewrite the untouched copies, it marks the finding `later --note "#issue"` and leaves them. The agent hook holds the end of a turn for the change's copy until it is fixed or marked, never for the untouched ones. A baseline that accepts the repeat from a check of whole files accepts it here too. + +## Fingerprints + +A repeat is identified by its copies: each function that holds one, by path and name, and a copy outside a function by its path and statements. Its fingerprint is the same whichever file a check selects or reports it in and whichever window of those functions repeats. A baseline accepts it while every copy is one an accepted repeat names: a copy that goes leaves it accepted, and a new copy that joins raises it again. ## How it is measured diff --git a/src/baseline.rs b/src/baseline.rs index 1b3c750..775f66e 100644 --- a/src/baseline.rs +++ b/src/baseline.rs @@ -1,16 +1,17 @@ //! The baseline of accepted findings: writing it from the last check, marking //! why findings were accepted, with an optional note, listing the marks, and -//! counting their reasons per rule. +//! counting their reasons per rule. Every write rewrites the entries a +//! finding of the last check matched through a fingerprint it had before +//! (`entries`), so a baseline moves to the current fingerprints keeping +//! each reason and note. use crate::{ options::Disposition, - schema::{Report, Scope, Strength}, + schema::{Finding, Report, Scope, Strength}, }; use anyhow::{Context, Result, ensure}; +use entries::{Entries, Found}; use serde::{Deserialize, Serialize}; -use std::{ - collections::{BTreeMap, BTreeSet}, - path::Path, -}; +use std::{collections::BTreeSet, path::Path}; pub const BASELINE_FILE: &str = "jevgate-baseline.json"; @@ -41,8 +42,17 @@ struct Accepted { /// will be fixed in. #[serde(default, skip_serializing_if = "Option::is_none")] note: Option, + /// What a finding whose fingerprint changed is matched by: a repeat's + /// copies, an outline's member names. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + members: Vec, } +mod entries; +mod listing; + +pub use listing::{list, list_markdown, list_text, stats, stats_table}; + /// A note is one line of at most this many characters. pub const NOTE_CHARS: usize = 200; @@ -75,6 +85,11 @@ fn read_baseline(root: &Path) -> Result> { parse(&text).map(Some) } +/// The entries of a baseline read: none without one. +fn entries(baseline: Result>) -> Result> { + Ok(baseline?.map_or_else(Vec::new, |b| b.findings)) +} + /// The baseline in Git tree `tree`, such as an agent turn's start. fn baseline_in(root: &Path, tree: &str) -> Result> { let path = Path::new(BASELINE_FILE); @@ -94,28 +109,25 @@ pub(crate) fn parses(text: &str) -> bool { parse(text).is_ok() } -/// Mark findings whose fingerprints the baseline accepted: the committed -/// one, or, within an agent's turn, the one in Git tree `as_of`, the turn's -/// start, and the entries the turn added that dismiss a finding with a -/// reason (see [`dismissed_since`]). +/// Mark the findings the baseline accepts (`Entries::find`): the +/// committed one, or, within an agent's turn, the one in Git tree `as_of`, +/// the turn's start, and the entries the turn added that dismiss a finding +/// with a reason (see [`Dismissals`]). pub fn apply(root: &Path, report: &mut Report, as_of: Option<&str>) -> Result<()> { - let accepted: BTreeSet = match as_of { + let accepts: Box bool> = match as_of { Some(tree) => { - let then = baseline_in(root, tree)?.map_or_else(Vec::new, |b| b.findings); - let dismissed = dismissed_since(root, tree).unwrap_or_default(); - then.into_iter() - .map(|f| f.fingerprint) - .chain(dismissed.into_keys()) - .collect() + let dismissals = Dismissals::since(root, tree)?; + Box::new(move |finding| { + dismissals.then.find(finding).is_some() || dismissals.of(finding).is_some() + }) + } + None => { + let entries = Entries::new(entries(read_baseline(root))?); + Box::new(move |finding| entries.find(finding).is_some()) } - None => read_baseline(root)? - .map_or_else(Vec::new, |b| b.findings) - .into_iter() - .map(|f| f.fingerprint) - .collect(), }; for finding in report.files.iter_mut().flat_map(|f| &mut f.findings) { - finding.baselined = accepted.contains(&finding.fingerprint); + finding.baselined = accepts(finding); } Ok(()) } @@ -127,32 +139,41 @@ pub struct Dismissal { pub note: Option, } -/// The findings the baseline dismisses with a reason that its version in Git -/// tree `since` did not accept, by fingerprint: what an agent dismissed with -/// `baseline mark` during a turn that began at `since`. Accepting a finding -/// is otherwise the person's call, so an entry without a reason counts from -/// the next turn; a person audits the reasons with `baseline stats`. -pub fn dismissed_since(root: &Path, since: &str) -> Result> { - let held: BTreeSet = baseline_in(root, since)? - .map_or_else(Vec::new, |b| b.findings) - .into_iter() - .map(|f| f.fingerprint) - .collect(); - Ok(read_baseline(root)? - .map_or_else(Vec::new, |b| b.findings) - .into_iter() - .filter(|f| !held.contains(&f.fingerprint)) - .filter_map(|f| { - let reason = f.reason?; - Some(( - f.fingerprint, - Dismissal { - reason, - note: f.note, - }, - )) +/// The baseline as an agent's turn began and as it is now, which tell the +/// findings the agent dismissed with `baseline mark` during the turn. +pub struct Dismissals { + then: Entries, + now: Entries, +} + +impl Dismissals { + /// The baseline in Git tree `since`, the turn's start, and the + /// committed one; a baseline the turn left unreadable dismisses nothing. + pub fn since(root: &Path, since: &str) -> Result { + let then = entries(baseline_in(root, since))?; + let now = entries(read_baseline(root)).unwrap_or_default(); + Ok(Self { + then: Entries::new(then), + now: Entries::new(now), }) - .collect()) + } + + /// The dismissal of `finding` with a reason that the baseline as the + /// turn began did not accept. Accepting a finding is otherwise the + /// person's call, so an entry without a reason counts from the next + /// turn; a person audits the reasons with `baseline stats`. An entry a + /// write rewrote to the finding's current fingerprint was accepted + /// before, through the fingerprint it had then. + pub fn of(&self, finding: &Finding) -> Option { + if self.then.find(finding).is_some() { + return None; + } + let entry = self.now.get(self.now.find(finding)?); + Some(Dismissal { + reason: entry.reason?, + note: entry.note.clone(), + }) + } } /// What `jevgate baseline` wrote: the file, the findings it accepted from the @@ -188,18 +209,36 @@ fn last_check(root: &Path, merge: bool) -> Result { /// are replaced by what the check found. A check of changed lines judged only /// what its change touched, so the entries of the files it checked stay too, /// and only deleted files' entries go. A finding accepted before keeps its -/// reason and note; the others get `reason`. +/// reason and note, at its current fingerprint; the others get `reason`. pub fn write(root: &Path, merge: bool, reason: Option) -> Result { let report = last_check(root, merge)?; - let mut findings = to_accept(&report, reason); - let accepted = findings.len(); - let previous = read_baseline(root)?; - if let Some(previous) = &previous { - keep_marks(&mut findings, previous); + let previous = Entries::new(entries(read_baseline(root))?); + let mut findings = Vec::new(); + // Entries a finding now stands for, which a merge does not keep. + let mut replaced = BTreeSet::new(); + for (path, finding) in accepted_findings(&report) { + let entry = accepted(path, finding, reason); + findings.push(match previous.find(finding) { + Some(found) => { + if !matches!(found, Found::Copies(_)) { + replaced.insert(found.at()); + } + kept(entry, previous.get(found)) + } + None => entry, + }); } - let earlier = match previous { - Some(previous) if merge => uncovered(&report, previous), - _ => Vec::new(), + let accepted = findings.len(); + let earlier = if merge { + let kept = previous + .entries + .into_iter() + .enumerate() + .filter(|(at, _)| !replaced.contains(at)) + .map(|(_, entry)| entry); + uncovered(&report, kept) + } else { + Vec::new() }; let kept = earlier.len(); findings.extend(earlier); @@ -219,51 +258,45 @@ pub fn write(root: &Path, merge: bool, reason: Option) -> Result) -> Vec { - report - .files - .iter() - .flat_map(|file| { - file.findings - .iter() - .filter(|f| f.suppressed.is_none()) - .map(|f| Accepted { - fingerprint: f.fingerprint.clone(), - rule: f.rule.clone(), - path: file.path.clone(), - line: Some(f.line), - unit: f.symbol.clone(), - strength: Some(f.strength), - message: f.message.clone(), - reason, - note: None, - }) - }) - .collect() +/// The check's findings to accept, with their files' paths. A suppressed +/// finding is accepted where its comment is; removing the comment brings +/// it back. +fn accepted_findings(report: &Report) -> impl Iterator { + report.files.iter().flat_map(|file| { + file.findings + .iter() + .filter(|f| f.suppressed.is_none()) + .map(|f| (file.path.as_path(), f)) + }) } -/// Give each finding accepted before the reason it was accepted with, and -/// its note. -fn keep_marks(findings: &mut [Accepted], previous: &Baseline) { - let marks: BTreeMap<&str, &Accepted> = previous - .findings - .iter() - .filter(|f| f.reason.is_some() || f.note.is_some()) - .map(|f| (f.fingerprint.as_str(), f)) - .collect(); - for finding in findings { - if let Some(earlier) = marks.get(finding.fingerprint.as_str()) { - finding.reason = earlier.reason.or(finding.reason); - finding.note.clone_from(&earlier.note); - } +/// The entry that accepts `finding`, in the file at `path`, with `reason`. +fn accepted(path: &Path, finding: &Finding, reason: Option) -> Accepted { + Accepted { + fingerprint: finding.fingerprint.clone(), + rule: finding.rule.clone(), + path: path.to_path_buf(), + line: Some(finding.line), + unit: finding.symbol.clone(), + strength: Some(finding.strength), + message: finding.message.clone(), + reason, + note: None, + members: finding.identity.members.clone(), } } +/// `entry` with the reason and note of the `earlier` one it replaces; its +/// own reason when that one had none. +fn kept(mut entry: Accepted, earlier: &Accepted) -> Accepted { + entry.reason = earlier.reason.or(entry.reason); + entry.note.clone_from(&earlier.note); + entry +} + /// The earlier entries a merge keeps: those of files the check did not /// cover. A check of changed lines covers only the files it deleted. -fn uncovered(report: &Report, previous: Baseline) -> Vec { +fn uncovered(report: &Report, previous: impl Iterator) -> Vec { let whole = report.scope == Scope::WholeFiles; let covered: BTreeSet<&Path> = report .files @@ -273,8 +306,6 @@ fn uncovered(report: &Report, previous: Baseline) -> Vec { .chain(report.deleted_files.iter().map(|p| p.as_path())) .collect(); previous - .findings - .into_iter() .filter(|f| !covered.contains(f.path.as_path())) .collect() } @@ -324,19 +355,14 @@ pub fn mark(root: &Path, mark: &Mark<'_>) -> Result { let named = |finding: &Accepted, paths: bool| { selected(&finding.rule, mark.rules) && targets.iter().any(|t| names(t, finding, paths)) }; - let mut marked = 0; - for finding in &mut baseline.findings { - if named(finding, true) { - finding.reason = Some(mark.reason); - if let Some(note) = &mark.note { - finding.note.clone_from(note); - } - marked += 1; - } + let report = crate::storage::read_latest(root).ok(); + if let Some(report) = &report { + rewrite(&mut baseline, report); } - if let Ok(report) = crate::storage::read_latest(root) { + let mut marked = mark_entries(&mut baseline.findings, mark, |f| named(f, true)); + if let Some(report) = &report { let note = mark.note.clone().flatten(); - marked += add_dismissed(&mut baseline, &report, (mark.reason, note), |f| { + marked += add_dismissed(&mut baseline, report, (mark.reason, note), |f| { named(f, false) }); } @@ -349,6 +375,24 @@ pub fn mark(root: &Path, mark: &Mark<'_>) -> Result { Ok(marked) } +/// Record the mark's reason, and its note when given, on the entries +/// `named` picks; returns how many. +fn mark_entries( + findings: &mut [Accepted], + mark: &Mark<'_>, + named: impl Fn(&Accepted) -> bool, +) -> usize { + let mut marked = 0; + for finding in findings.iter_mut().filter(|f| named(f)) { + finding.reason = Some(mark.reason); + if let Some(note) = &mark.note { + finding.note.clone_from(note); + } + marked += 1; + } + marked +} + /// Whether a finding of `rule` is among `rules`, rule keys: every rule when /// there are none. fn selected(rule: &str, rules: &[&str]) -> bool { @@ -359,41 +403,69 @@ fn selected(rule: &str, rules: &[&str]) -> bool { rules.is_empty() || key.is_some_and(|key| rules.contains(&key)) } +/// Rewrite each entry a finding of `report` matched through a fingerprint +/// it had before to the finding's current fingerprint, keeping the entry's +/// reason and note. +fn rewrite(baseline: &mut Baseline, report: &Report) { + let entries = Entries::new(std::mem::take(&mut baseline.findings)); + let mut replaced = BTreeSet::new(); + let mut rewritten = Vec::new(); + for (path, finding) in accepted_findings(report) { + if let Some(Found::Earlier(at)) = entries.find(finding) { + rewritten.push(kept(accepted(path, finding, None), &entries.entries[at])); + replaced.insert(at); + } + } + baseline.findings = entries + .entries + .into_iter() + .enumerate() + .filter(|(at, _)| !replaced.contains(at)) + .map(|(_, entry)| entry) + .chain(rewritten) + .collect(); + in_file_order(&mut baseline.findings); +} + /// Adds the findings of `report` that `named` picks and `baseline` does not -/// hold yet, dismissed for the reason with the note, and says how many it -/// added. +/// accept yet, dismissed for the reason with the note, and says how many it +/// added. One replaces an entry with its fingerprint that no longer +/// accepts it, as an outline's whose members changed too much. fn add_dismissed( baseline: &mut Baseline, report: &Report, (reason, note): (Disposition, Option), named: impl Fn(&Accepted) -> bool, ) -> usize { - let held: BTreeSet = baseline - .findings - .iter() - .map(|f| f.fingerprint.clone()) - .collect(); - let mut dismissed: Vec = to_accept(report, Some(reason)) - .into_iter() - .filter(|f| !held.contains(&f.fingerprint) && named(f)) - .map(|f| Accepted { + let held = Entries::new(std::mem::take(&mut baseline.findings)); + let mut dismissed: Vec = accepted_findings(report) + .filter(|(_, finding)| held.find(finding).is_none()) + .map(|(path, finding)| Accepted { note: note.clone(), - ..f + ..accepted(path, finding, Some(reason)) }) + .filter(|f| named(f)) .collect(); in_file_order(&mut dismissed); let added = dismissed.len(); - baseline.findings.extend(dismissed); - in_file_order(&mut baseline.findings); + let replaced: BTreeSet<&str> = dismissed.iter().map(|f| f.fingerprint.as_str()).collect(); + let mut findings: Vec = held + .entries + .into_iter() + .filter(|f| !replaced.contains(f.fingerprint.as_str())) + .collect(); + findings.extend(dismissed); + in_file_order(&mut findings); + baseline.findings = findings; added } /// The order the baseline file keeps its entries in, by path, then -/// fingerprint, each finding once. +/// fingerprint, each finding once: the first entry with its fingerprint. fn in_file_order(findings: &mut Vec) { + let mut seen = BTreeSet::new(); + findings.retain(|f| seen.insert(f.fingerprint.clone())); findings.sort_by(|a, b| (&a.path, &a.fingerprint).cmp(&(&b.path, &b.fingerprint))); - // A fingerprint covers the path, so a finding's copies are adjacent. - findings.dedup_by(|a, b| a.fingerprint == b.fingerprint); } /// Whether a `mark` target names a finding: by fingerprint prefix or @@ -411,177 +483,3 @@ fn names(target: &str, finding: &Accepted, paths: bool) -> bool { let target = target.trim_end_matches('/'); paths && !target.is_empty() && finding.path.starts_with(target) } - -/// An accepted finding as `baseline list` prints it. -#[derive(Debug, Serialize, PartialEq)] -pub struct Listed { - pub path: std::path::PathBuf, - pub line: Option, - pub reason: Option, - pub rule: String, - pub unit: Option, - pub fingerprint: String, - pub note: Option, - pub message: String, -} - -/// The fingerprint prefix the listing prints: as many characters as -/// `baseline mark` needs to name a finding. -const LISTED_PREFIX: usize = 8; - -/// The accepted findings with one of `reasons` (any, or none, when empty) -/// whose rule is among `rules` (every rule when empty), by path, then line. -pub fn list(root: &Path, reasons: &[Disposition], rules: &[&str]) -> Result> { - let baseline = read_baseline(root)? - .with_context(|| format!("No {BASELINE_FILE}; run jevgate baseline first"))?; - let mut listed: Vec = baseline - .findings - .into_iter() - .filter(|f| reasons.is_empty() || f.reason.is_some_and(|r| reasons.contains(&r))) - .filter(|f| selected(&f.rule, rules)) - .map(|f| Listed { - path: f.path, - line: f.line, - reason: f.reason, - rule: f.rule, - unit: f.unit, - fingerprint: f.fingerprint, - note: f.note, - message: f.message, - }) - .collect(); - listed - .sort_by(|a, b| (&a.path, a.line, &a.fingerprint).cmp(&(&b.path, b.line, &b.fingerprint))); - Ok(listed) -} - -impl Listed { - fn location(&self) -> String { - match self.line { - Some(line) => format!("{}:{line}", self.path.display()), - None => self.path.display().to_string(), - } - } - - fn reason(&self) -> String { - self.reason - .map_or_else(|| "no reason".into(), |r| crate::output::label(&r)) - } - - fn id(&self) -> &str { - self.fingerprint - .get(..LISTED_PREFIX) - .unwrap_or(&self.fingerprint) - } -} - -/// The listing for people: location, reason, rule and fingerprint prefix -/// in aligned columns, then the unit and the note. -pub fn list_text(listed: &[Listed]) -> String { - let rows: Vec<[String; 4]> = listed - .iter() - .map(|l| [l.location(), l.reason(), l.rule.clone(), l.id().to_string()]) - .collect(); - let widths: Vec = (0..4) - .map(|column| { - rows.iter() - .map(|row| row[column].chars().count()) - .max() - .unwrap_or(0) - }) - .collect(); - rows.iter() - .zip(listed) - .map(|(row, l)| { - let mut cells: Vec = row - .iter() - .zip(&widths) - .map(|(cell, &width)| format!("{cell:>() - .join("\n") -} - -/// The listing as a Markdown checklist to paste into a cleanup issue. -pub fn list_markdown(listed: &[Listed]) -> String { - // A unit named with its own code spans stays as it is. - let code = |text: &str| { - if text.contains('`') { - text.to_string() - } else { - format!("`{text}`") - } - }; - listed - .iter() - .map(|l| { - let mut line = format!("- [ ] {} {}", code(&l.location()), l.rule); - if let Some(unit) = &l.unit { - line.push_str(&format!(" {}", code(unit))); - } - line.push_str(&format!(" ({}, {})", l.reason(), code(l.id()))); - if let Some(note) = &l.note { - line.push_str(&format!(": {note}")); - } - line - }) - .collect::>() - .join("\n") -} - -/// Accepted findings of one rule by reason. -#[derive(Debug, Default, Serialize, PartialEq)] -pub struct ReasonCounts { - pub accepted: usize, - pub intended: usize, - pub later: usize, - pub wrong: usize, - pub without_reason: usize, - /// `wrong` among the findings with a reason; none when no finding has one. - pub wrong_rate: Option, -} - -/// Accepted findings by rule ID and reason. -pub fn stats(root: &Path) -> Result> { - let baseline = read_baseline(root)? - .with_context(|| format!("No {BASELINE_FILE}; run jevgate baseline first"))?; - let mut counts = BTreeMap::::new(); - for finding in &baseline.findings { - let count = counts.entry(finding.rule.clone()).or_default(); - count.accepted += 1; - *match finding.reason { - Some(Disposition::Intended) => &mut count.intended, - Some(Disposition::Later) => &mut count.later, - Some(Disposition::Wrong) => &mut count.wrong, - None => &mut count.without_reason, - } += 1; - } - for count in counts.values_mut() { - let reasoned = count.accepted - count.without_reason; - count.wrong_rate = (reasoned > 0).then(|| count.wrong as f64 / reasoned as f64); - } - Ok(counts) -} - -/// The stats as a table for people. -pub fn stats_table(counts: &BTreeMap) -> String { - let width = counts.keys().map(String::len).max().unwrap_or(0).max(4); - let mut lines = vec![format!( - "{:8} {:>8} {:>6} {:>6} {:>9} {:>6}", - "rule", "accepted", "intended", "later", "wrong", "no reason", "wrong%" - )]; - for (rule, c) in counts { - let rate = c - .wrong_rate - .map_or("-".to_string(), |r| format!("{:.0}%", r * 100.0)); - lines.push(format!( - "{rule:8} {:>8} {:>6} {:>6} {:>9} {rate:>6}", - c.accepted, c.intended, c.later, c.wrong, c.without_reason - )); - } - lines.join("\n") -} diff --git a/src/baseline/entries.rs b/src/baseline/entries.rs new file mode 100644 index 0000000..f884d66 --- /dev/null +++ b/src/baseline/entries.rs @@ -0,0 +1,145 @@ +//! Which baseline entry accepts a finding: the one with its fingerprint, or +//! one written before its fingerprint changed, through the fingerprints it +//! had then or the members it is made of. +use super::Accepted; +use crate::{catalog, schema::Finding}; +use std::collections::{BTreeMap, BTreeSet}; + +/// An outline whose member names overlap an accepted outline's this much +/// (shared names over all names) is the one accepted: a file that grew a +/// little stays accepted, and one that grew a lot is asked about again. +const SIMILAR_OUTLINE: f64 = 0.8; + +/// A baseline's entries, indexed to find the one that accepts a finding. +pub(super) struct Entries { + pub entries: Vec, + by_fingerprint: BTreeMap, + /// Shared-logic entries by each copy they name. + by_copy: BTreeMap>, +} + +/// How an entry accepts a finding. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(super) enum Found { + /// By the finding's fingerprint. + Own(usize), + /// By a fingerprint the finding had before, under JevGate 0.35 or + /// before its file was renamed: the next baseline write rewrites it. + Earlier(usize), + /// A repeat whose every copy shared-logic entries accept, as when a + /// copy was removed or a check saw only some copies; the first of them. + Copies(usize), +} + +impl Found { + pub fn at(self) -> usize { + match self { + Self::Own(at) | Self::Earlier(at) | Self::Copies(at) => at, + } + } +} + +impl Entries { + pub fn new(entries: Vec) -> Self { + let mut by_fingerprint = BTreeMap::new(); + let mut by_copy = BTreeMap::>::new(); + let shared = catalog::id(catalog::SHARED_LOGIC); + for (at, entry) in entries.iter().enumerate() { + by_fingerprint + .entry(entry.fingerprint.clone()) + .or_insert(at); + if entry.rule == shared { + for member in &entry.members { + by_copy.entry(member.clone()).or_default().push(at); + } + } + } + Self { + entries, + by_fingerprint, + by_copy, + } + } + + /// The entry that accepts `finding`, if one does. An outline's entry + /// accepts it only while their member names are similar; a repeat is + /// accepted while every copy is one an accepted repeat names, and asked + /// about again when a copy no entry names joins it. + pub fn find(&self, finding: &Finding) -> Option { + if let Some(&at) = self.by_fingerprint.get(&finding.fingerprint) + && similar(&self.entries[at].members, &finding.identity.members) + { + return Some(Found::Own(at)); + } + if let Some(at) = finding + .identity + .earlier() + .find_map(|earlier| self.by_fingerprint.get(earlier)) + { + return Some(Found::Earlier(*at)); + } + self.copies(finding).map(Found::Copies) + } + + pub fn get(&self, found: Found) -> &Accepted { + &self.entries[found.at()] + } + + /// The first shared-logic entry naming a copy of `finding`, when the + /// entries naming its copies name all of them. + fn copies(&self, finding: &Finding) -> Option { + let members = &finding.identity.members; + if finding.rule != catalog::id(catalog::SHARED_LOGIC) || members.is_empty() { + return None; + } + let naming: BTreeSet = members + .iter() + .filter_map(|member| self.by_copy.get(member)) + .flatten() + .copied() + .collect(); + let named: BTreeSet<&String> = naming + .iter() + .flat_map(|&at| &self.entries[at].members) + .collect(); + members + .iter() + .all(|member| named.contains(member)) + .then(|| naming.first().copied()) + .flatten() + } +} + +/// Whether an entry's members and a finding's are similar enough for the +/// entry to accept it: always when either has none, as every finding but +/// an outline or a repeat, whose fingerprint already names its copies. +fn similar(accepted: &[String], found: &[String]) -> bool { + if accepted.is_empty() || found.is_empty() { + return true; + } + let accepted: BTreeSet<&String> = accepted.iter().collect(); + let found: BTreeSet<&String> = found.iter().collect(); + let shared = accepted.intersection(&found).count(); + let all = accepted.union(&found).count(); + shared as f64 >= SIMILAR_OUTLINE * all as f64 +} + +#[cfg(test)] +mod tests { + use super::similar; + + fn names(names: &[&str]) -> Vec { + names.iter().map(|name| name.to_string()).collect() + } + + #[test] + fn an_outline_stays_similar_until_a_fifth_of_its_names_change() { + let before = names(&["a", "b", "c", "d", "e", "f", "g", "h"]); + let mut grown = before.clone(); + grown.push("i".into()); + assert!(similar(&before, &grown), "8 of 9 names shared"); + grown.extend(names(&["j", "k"])); + assert!(!similar(&before, &grown), "8 of 11 names shared"); + assert!(similar(&[], &grown), "an entry without members"); + } +} diff --git a/src/baseline/listing.rs b/src/baseline/listing.rs new file mode 100644 index 0000000..f3356a8 --- /dev/null +++ b/src/baseline/listing.rs @@ -0,0 +1,181 @@ +//! The baseline's entries as `baseline list` and `baseline stats` print +//! them: by path with their reasons and notes, and counted by rule and reason. +use super::{BASELINE_FILE, read_baseline, selected}; +use crate::options::Disposition; +use anyhow::{Context, Result}; +use serde::Serialize; +use std::{collections::BTreeMap, path::Path}; + +/// An accepted finding as `baseline list` prints it. +#[derive(Debug, Serialize, PartialEq)] +pub struct Listed { + pub path: std::path::PathBuf, + pub line: Option, + pub reason: Option, + pub rule: String, + pub unit: Option, + pub fingerprint: String, + pub note: Option, + pub message: String, +} + +/// The fingerprint prefix the listing prints: as many characters as +/// `baseline mark` needs to name a finding. +const LISTED_PREFIX: usize = 8; + +/// The accepted findings with one of `reasons` (any, or none, when empty) +/// whose rule is among `rules` (every rule when empty), by path, then line. +pub fn list(root: &Path, reasons: &[Disposition], rules: &[&str]) -> Result> { + let baseline = read_baseline(root)? + .with_context(|| format!("No {BASELINE_FILE}; run jevgate baseline first"))?; + let mut listed: Vec = baseline + .findings + .into_iter() + .filter(|f| reasons.is_empty() || f.reason.is_some_and(|r| reasons.contains(&r))) + .filter(|f| selected(&f.rule, rules)) + .map(|f| Listed { + path: f.path, + line: f.line, + reason: f.reason, + rule: f.rule, + unit: f.unit, + fingerprint: f.fingerprint, + note: f.note, + message: f.message, + }) + .collect(); + listed + .sort_by(|a, b| (&a.path, a.line, &a.fingerprint).cmp(&(&b.path, b.line, &b.fingerprint))); + Ok(listed) +} + +impl Listed { + fn location(&self) -> String { + match self.line { + Some(line) => format!("{}:{line}", self.path.display()), + None => self.path.display().to_string(), + } + } + + fn reason(&self) -> String { + self.reason + .map_or_else(|| "no reason".into(), |r| crate::output::label(&r)) + } + + fn id(&self) -> &str { + self.fingerprint + .get(..LISTED_PREFIX) + .unwrap_or(&self.fingerprint) + } +} + +/// The listing for people: location, reason, rule and fingerprint prefix +/// in aligned columns, then the unit and the note. +pub fn list_text(listed: &[Listed]) -> String { + let rows: Vec<[String; 4]> = listed + .iter() + .map(|l| [l.location(), l.reason(), l.rule.clone(), l.id().to_string()]) + .collect(); + let widths: Vec = (0..4) + .map(|column| { + rows.iter() + .map(|row| row[column].chars().count()) + .max() + .unwrap_or(0) + }) + .collect(); + rows.iter() + .zip(listed) + .map(|(row, l)| { + let mut cells: Vec = row + .iter() + .zip(&widths) + .map(|(cell, &width)| format!("{cell:>() + .join("\n") +} + +/// The listing as a Markdown checklist to paste into a cleanup issue. +pub fn list_markdown(listed: &[Listed]) -> String { + // A unit named with its own code spans stays as it is. + let code = |text: &str| { + if text.contains('`') { + text.to_string() + } else { + format!("`{text}`") + } + }; + listed + .iter() + .map(|l| { + let mut line = format!("- [ ] {} {}", code(&l.location()), l.rule); + if let Some(unit) = &l.unit { + line.push_str(&format!(" {}", code(unit))); + } + line.push_str(&format!(" ({}, {})", l.reason(), code(l.id()))); + if let Some(note) = &l.note { + line.push_str(&format!(": {note}")); + } + line + }) + .collect::>() + .join("\n") +} + +/// Accepted findings of one rule by reason. +#[derive(Debug, Default, Serialize, PartialEq)] +pub struct ReasonCounts { + pub accepted: usize, + pub intended: usize, + pub later: usize, + pub wrong: usize, + pub without_reason: usize, + /// `wrong` among the findings with a reason; none when no finding has one. + pub wrong_rate: Option, +} + +/// Accepted findings by rule ID and reason. +pub fn stats(root: &Path) -> Result> { + let baseline = read_baseline(root)? + .with_context(|| format!("No {BASELINE_FILE}; run jevgate baseline first"))?; + let mut counts = BTreeMap::::new(); + for finding in &baseline.findings { + let count = counts.entry(finding.rule.clone()).or_default(); + count.accepted += 1; + *match finding.reason { + Some(Disposition::Intended) => &mut count.intended, + Some(Disposition::Later) => &mut count.later, + Some(Disposition::Wrong) => &mut count.wrong, + None => &mut count.without_reason, + } += 1; + } + for count in counts.values_mut() { + let reasoned = count.accepted - count.without_reason; + count.wrong_rate = (reasoned > 0).then(|| count.wrong as f64 / reasoned as f64); + } + Ok(counts) +} + +/// The stats as a table for people. +pub fn stats_table(counts: &BTreeMap) -> String { + let width = counts.keys().map(String::len).max().unwrap_or(0).max(4); + let mut lines = vec![format!( + "{:8} {:>8} {:>6} {:>6} {:>9} {:>6}", + "rule", "accepted", "intended", "later", "wrong", "no reason", "wrong%" + )]; + for (rule, c) in counts { + let rate = c + .wrong_rate + .map_or("-".to_string(), |r| format!("{:.0}%", r * 100.0)); + lines.push(format!( + "{rule:8} {:>8} {:>6} {:>6} {:>9} {rate:>6}", + c.accepted, c.intended, c.later, c.wrong, c.without_reason + )); + } + lines.join("\n") +} diff --git a/src/changes.rs b/src/changes.rs index 1e0427a..8ec87f5 100644 --- a/src/changes.rs +++ b/src/changes.rs @@ -1,4 +1,7 @@ -//! Finding lineage between snapshots, by fingerprint: introduced, persistent or resolved. +//! Finding lineage between snapshots, by fingerprint: introduced, persistent +//! or resolved. A finding is also known by the fingerprints it had before +//! (`Identity::earlier`), so a snapshot taken before they changed or before +//! its file was renamed still holds it. use crate::schema::{Change, Report, Scope, Status}; use std::{ collections::{BTreeMap, BTreeSet}, @@ -68,7 +71,9 @@ fn lineage(previous: &Report, report: &Report) -> Vec { let current: BTreeSet<&str> = report .files .iter() - .flat_map(|f| f.findings.iter().map(|x| x.fingerprint.as_str())) + .flat_map(|f| &f.findings) + .flat_map(|x| x.fingerprints()) + .map(String::as_str) .collect(); let judged = judged_paths(report); for (fingerprint, (path, rule)) in before { @@ -107,7 +112,10 @@ fn current_changes( let mut changes = Vec::new(); for file in &report.files { for finding in &file.findings { - let (state, reason) = if before.contains_key(finding.fingerprint.as_str()) { + let seen = finding + .fingerprints() + .any(|fingerprint| before.contains_key(fingerprint.as_str())); + let (state, reason) = if seen { ("persistent", "The same finding remains") } else if comparable { ("introduced", "New since the previous snapshot") @@ -167,6 +175,7 @@ mod tests { precision: None, preview: None, untouched: Vec::new(), + identity: Default::default(), } } @@ -190,16 +199,20 @@ mod tests { old.files[0].findings = vec![finding("kept"), finding("fixed")]; let mut new = old.clone(); new.generation = 2; + let mut renamed = finding("now"); + renamed.identity.aliases = vec!["fixed".into()]; new.files[0].findings = vec![finding("kept"), finding("added")]; + let states = |new: &Report| -> BTreeMap { + new.changes + .iter() + .map(|c| (c.fingerprint.clone(), c.state.clone())) + .collect() + }; compare(Some(&old), &mut new); - let states: BTreeMap<_, _> = new - .changes - .iter() - .map(|c| (c.fingerprint.as_str(), c.state.as_str())) - .collect(); - assert_eq!(states["kept"], "persistent"); - assert_eq!(states["added"], "introduced"); - assert_eq!(states["fixed"], "resolved"); + let states_now = states(&new); + assert_eq!(states_now["kept"], "persistent"); + assert_eq!(states_now["added"], "introduced"); + assert_eq!(states_now["fixed"], "resolved"); new.files[0].status = Status::Error; compare(Some(&old), &mut new); assert!( @@ -207,6 +220,15 @@ mod tests { .iter() .any(|c| c.fingerprint == "fixed" && c.state == "non-comparable") ); + new.files[0].status = Status::Review; + new.files[0].findings.push(renamed); + compare(Some(&old), &mut new); + let states_now = states(&new); + assert_eq!( + (states_now["now"].as_str(), states_now.get("fixed")), + ("persistent", None), + "a finding is the one it was before its fingerprint changed" + ); compare(None, &mut new); assert!(new.changes.iter().all(|c| c.state == "baseline")); } diff --git a/src/hook/events.rs b/src/hook/events.rs index 6ef6832..f0a5cd7 100644 --- a/src/hook/events.rs +++ b/src/hook/events.rs @@ -313,10 +313,11 @@ impl<'a> Hook<'a> { turn.as_ref() .is_some_and(|t| t.reported.iter().any(|r| r == id)) }; + // Told under a fingerprint it had before its file was renamed, too. let (known, new): (Vec<_>, Vec<_>) = checked .flagged .into_iter() - .partition(|f| reported(&f.finding.fingerprint)); + .partition(|f| f.finding.fingerprints().any(|id| reported(id))); let undecided: Vec<_> = checked .undecided .iter() diff --git a/src/hook/review.rs b/src/hook/review.rs index 7145384..633fe28 100644 --- a/src/hook/review.rs +++ b/src/hook/review.rs @@ -378,14 +378,16 @@ fn run( /// The findings of `report` the agent dismissed with a reason since the /// turn began at `start`, which the report's gate already accepts. fn dismissed(root: &Path, report: &Report, start: &str) -> Vec { - let dismissals = crate::baseline::dismissed_since(root, start).unwrap_or_default(); + let Ok(dismissals) = crate::baseline::Dismissals::since(root, start) else { + return Vec::new(); + }; report .files .iter() .flat_map(|file| file.findings.iter().map(move |f| (file, f))) .filter(|(_, f)| f.baselined) .filter_map(|(file, f)| { - let dismissal = dismissals.get(&f.fingerprint)?; + let dismissal = dismissals.of(f)?; Some(Dismissed { path: file.path.clone(), line: f.line, diff --git a/src/mcp/tools.rs b/src/mcp/tools.rs index 8c8e79e..03d2661 100644 --- a/src/mcp/tools.rs +++ b/src/mcp/tools.rs @@ -145,7 +145,7 @@ fn finding_schema() -> Value { json!({ "type": "object", "properties": { - "id": {"type": "string", "description": "The finding's fingerprint (rule, path and unit identity), as the baseline and SARIF record it; stable across unrelated edits"}, + "id": {"type": "string", "description": "The finding's fingerprint (rule, unit identity and, except for shared logic, path), as the baseline and SARIF record it; stable across unrelated edits and merges"}, "path": {"type": "string"}, "line": {"type": "integer"}, "end_line": {"type": "integer"}, @@ -176,7 +176,7 @@ fn verify_schema() -> Value { "type": "object", "description": "A unit whose answers stayed undecided: never a finding, and failing the gate only where jevgate.toml puts `uncertain` among its rule's levels", "properties": { - "id": {"type": "string", "description": "The unit's fingerprint (rule, path and unit identity), made as a finding's is; stable across unrelated edits"}, + "id": {"type": "string", "description": "The unit's fingerprint (rule, unit identity and, except for shared logic, path), made as a finding's is; stable across unrelated edits and merges"}, "path": {"type": "string"}, "line": {"type": "integer"}, "end_line": {"type": "integer"}, diff --git a/src/sarif.rs b/src/sarif.rs index de1dbed..31b1913 100644 --- a/src/sarif.rs +++ b/src/sarif.rs @@ -155,7 +155,12 @@ fn result(path: &Path, finding: &Finding, rule_index: Option) -> Value { "artifactLocation": artifact(path), "region": {"startLine": finding.line.max(1), "endLine": end.max(1)}, }}], - "partialFingerprints": {"jevgateFingerprint/v1": finding.fingerprint}, + // Both for a release, so an alert raised under the fingerprint + // JevGate 0.35 gave a finding stays the same alert. + "partialFingerprints": { + "jevgateFingerprint/v1": finding.identity.v1.as_ref().unwrap_or(&finding.fingerprint), + "jevgateFingerprint/v2": finding.fingerprint, + }, "properties": { "strength": output::label(&finding.strength), "probability": finding.concern_probability, @@ -208,7 +213,8 @@ mod tests { #[test] fn results_name_their_rule_level_location_and_fingerprint() { let args = crate::tests::args(); - let review = counted(Strength::Review, Gating::Fails); + let mut review = counted(Strength::Review, Gating::Fails); + review.identity.v1 = Some("v1-fingerprint".into()); let measuring = counted(Strength::Review, Gating::Measuring); let path = Path::new("src/a,b.rs"); let log = document(&report(&args), &[(path, &review), (path, &measuring)], &[]); @@ -248,7 +254,16 @@ mod tests { .unwrap() .ends_with("Not yet measured.\n\nNext step: Share one | implementation") ); - assert!(first["partialFingerprints"]["jevgateFingerprint/v1"].is_string()); + assert_eq!( + first["partialFingerprints"], + json!({"jevgateFingerprint/v1": "v1-fingerprint", "jevgateFingerprint/v2": review.fingerprint}), + "an alert raised under the earlier fingerprint stays the same alert" + ); + let unchanged = &results[1]["partialFingerprints"]; + assert_eq!( + unchanged["jevgateFingerprint/v1"], + unchanged["jevgateFingerprint/v2"] + ); } /// The reporting descriptor of rule `id` in a log without results. diff --git a/src/schema/mod.rs b/src/schema/mod.rs index bb04f0c..b442901 100644 --- a/src/schema/mod.rs +++ b/src/schema/mod.rs @@ -9,7 +9,7 @@ pub use report::*; pub const RUBRIC: &str = "jevgate-units-v1"; /// Changes how saved answers become a status. Included in the report identity /// and not in the judgment cache, so unchanged questions are not sent again. -pub const COMPOSITION: &str = "unit-composition-v14"; +pub const COMPOSITION: &str = "unit-composition-v15"; pub const SCHEMA_VERSION: u32 = 2; #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] diff --git a/src/schema/report.rs b/src/schema/report.rs index bad0626..dd71a0d 100644 --- a/src/schema/report.rs +++ b/src/schema/report.rs @@ -169,8 +169,12 @@ pub struct Finding { /// The weakness a security finding names, such as "CWE-89 SQL injection". #[serde(default, skip_serializing_if = "Option::is_none")] pub category: Option, - /// Rule, path, unit and normalized evidence; stable across unrelated edits. + /// Rule, path, unit and normalized evidence; stable across unrelated + /// edits, merges and changes to which files a check selects. pub fingerprint: String, + /// The fingerprints a baseline written earlier may hold for it. + #[serde(flatten)] + pub identity: Identity, /// Probability times the log of the lines involved; orders findings. pub rank: f64, #[serde(default)] @@ -201,6 +205,38 @@ pub struct Finding { pub untouched: Vec, } +/// What else a finding is known by: the fingerprints a baseline written +/// before may hold for it, and the members a baseline matches by. +#[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq)] +pub struct Identity { + /// The fingerprint JevGate 0.35 and earlier gave it, when it differs: + /// a shared-logic, file-organization or changed-hunk finding's. A + /// baseline entry still holding it accepts the finding until the next + /// baseline write rewrites the entry. + #[serde( + default, + rename = "fingerprint_v1", + skip_serializing_if = "Option::is_none" + )] + pub v1: Option, + /// Its fingerprints under the path its file had before a rename the + /// change made, which a baseline written before the rename holds. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub aliases: Vec, + /// What a baseline entry matches it by when its fingerprint differs: a + /// shared-logic finding's copies as `path::function`, or `path#hash` + /// outside a function, and a file-organization finding's member names. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub members: Vec, +} + +impl Identity { + /// Every fingerprint a baseline written earlier may hold for it. + pub fn earlier(&self) -> impl Iterator { + self.v1.iter().chain(&self.aliases) + } +} + /// A copy of a shared-logic finding that its change did not touch. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct Untouched { @@ -220,6 +256,11 @@ impl Finding { pub fn fails_gate(&self) -> bool { self.gate == Some(Gating::Fails) } + + /// Its fingerprint, then those it had before (`Identity::earlier`). + pub fn fingerprints(&self) -> impl Iterator { + std::iter::once(&self.fingerprint).chain(self.identity.earlier()) + } } /// How the gate counted a new finding, at the level in force for its rule diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 6585091..e3978f7 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -107,6 +107,7 @@ pub(super) fn finding(strength: crate::schema::Strength) -> crate::schema::Findi precision: None, preview: None, untouched: Vec::new(), + identity: Default::default(), } } diff --git a/src/units/compose/comments.rs b/src/units/compose/comments.rs index 056ce37..3962bb2 100644 --- a/src/units/compose/comments.rs +++ b/src/units/compose/comments.rs @@ -72,6 +72,7 @@ pub(super) fn comment_findings( let identities: Vec<&str> = std::iter::once(owner) .chain(entries.iter().map(|(unit, ..)| unit.identity.as_str())) .collect(); + let identity = crate::units::identity(&identities); Finding { rule: catalog::id(catalog::COMMENTS).into(), strength, @@ -85,11 +86,7 @@ pub(super) fn comment_findings( locations, quote: entries[0].0.quote.clone(), category: None, - fingerprint: fingerprint( - catalog::COMMENTS, - plan, - &crate::units::identity(&identities), - ), + fingerprint: fingerprint(catalog::COMMENTS, plan, &identity), rank: rank(p, lines), baselined: false, suppressed: None, @@ -97,6 +94,7 @@ pub(super) fn comment_findings( precision: None, preview: None, untouched: Vec::new(), + identity: renamed_only(catalog::COMMENTS, plan, &identity), } }) .collect() diff --git a/src/units/compose/identity.rs b/src/units/compose/identity.rs new file mode 100644 index 0000000..8c548f9 --- /dev/null +++ b/src/units/compose/identity.rs @@ -0,0 +1,94 @@ +//! What identifies a finding across edits, merges, selections and +//! renames: its fingerprint, the earlier ones a baseline may hold, and the +//! members a baseline matches by. +use super::*; + +pub(super) fn fingerprint(rule: &str, plan: &FilePlan, identity: &str) -> String { + hashed(rule, &plan.path, identity) +} + +fn hashed(rule: &str, path: &Path, identity: &str) -> String { + let separator = crate::schema::HASH_SEPARATOR; + hash(format!("{rule}{separator}{}{separator}{identity}", path.display()).as_bytes()) +} + +/// The fingerprints of a unit identified by its rule, its file's path and +/// `identity` under the path a rename moved the file from. +fn renamed(rule: &str, plan: &FilePlan, identity: &str) -> Vec { + plan.previous + .iter() + .map(|before| hashed(rule, before, identity)) + .collect() +} + +/// What a finding identified by its rule, path and `identity` is also +/// known by: its fingerprints under the path a rename moved its file from. +pub(super) fn renamed_only(rule: &str, plan: &FilePlan, identity: &str) -> Identity { + Identity { + aliases: renamed(rule, plan, identity), + ..Default::default() + } +} + +/// A unit's fingerprint and what else its findings are known by. Most +/// units are identified by their path and content. A repeat is identified +/// by its copies alone, wherever it is reported; a file's outline by its +/// path, its members matched by similarity. Each of these, and a changed +/// hunk, also keeps the fingerprint JevGate 0.35 gave it, which a baseline +/// written then holds, and every unit of a renamed file its fingerprints +/// under the old path. +pub(super) fn known(plan: &FilePlan, unit: &UnitPlan) -> (String, Identity) { + let rule = unit.rule; + let (v1, members) = match &unit.detail { + Detail::Pair { members, v1 } => (Some(v1.clone()), members.clone()), + Detail::Outline { members, .. } => (Some(identity_of(members)), members.clone()), + Detail::Custom(_, v1) => (v1.clone(), Vec::new()), + _ => (None, Vec::new()), + }; + let (now, mut aliases) = match &unit.detail { + Detail::Pair { members, .. } => (copies(rule, members), moved_copies(rule, plan, members)), + _ => ( + fingerprint(rule, plan, &unit.identity), + renamed(rule, plan, &unit.identity), + ), + }; + aliases.extend(v1.iter().flat_map(|v1| renamed(rule, plan, v1))); + let v1 = v1.map(|v1| fingerprint(rule, plan, &v1)); + ( + now, + Identity { + v1, + aliases, + members, + }, + ) +} + +fn identity_of(members: &[String]) -> String { + crate::units::identity(&members.iter().map(String::as_str).collect::>()) +} + +/// A repeat's fingerprint: its rule and copies, whichever file reports it. +fn copies(rule: &str, members: &[String]) -> String { + hashed(rule, Path::new(""), &identity_of(members)) +} + +/// A repeat's fingerprint with the copies in a renamed file under the path +/// it had before. +fn moved_copies(rule: &str, plan: &FilePlan, members: &[String]) -> Vec { + let Some(before) = &plan.previous else { + return Vec::new(); + }; + let path = plan.path.to_string_lossy(); + let mut moved: Vec = members + .iter() + .map(|member| match member.strip_prefix(&*path) { + Some(rest) if rest.starts_with("::") || rest.starts_with('#') => { + format!("{}{rest}", before.display()) + } + _ => member.clone(), + }) + .collect(); + moved.sort(); + vec![copies(rule, &moved)] +} diff --git a/src/units/compose/mod.rs b/src/units/compose/mod.rs index 788204c..c1f21bb 100644 --- a/src/units/compose/mod.rs +++ b/src/units/compose/mod.rs @@ -21,15 +21,20 @@ use super::{ use crate::{ catalog, schema::{ - Answer, Dimension, Finding, Judgment, Pass, Status, Strength, Undecided, UnitCounts, hash, + Answer, Dimension, Finding, Identity, Judgment, Pass, Status, Strength, Undecided, + UnitCounts, hash, }, }; -use std::collections::{BTreeMap, BTreeSet}; +use std::{ + collections::{BTreeMap, BTreeSet}, + path::Path, +}; mod answers; mod caps; mod comments; mod due; +mod identity; mod located; mod open; mod redundant; @@ -40,6 +45,7 @@ pub use due::{ finished_plans, uncertain_units, unconfirmed_units, unkinded_units, unlocated_units, unqueried_units, unsettled, untraced_units, }; +use identity::*; use located::*; use open::Quotes; use redundant::*; @@ -342,7 +348,7 @@ fn undecided_unit( let mut questions: Vec = open .iter() .map(|q| match &unit.detail { - Detail::Custom(question) => question.question.clone(), + Detail::Custom(question, _) => question.question.clone(), _ => question_label(q).to_string(), }) .collect(); @@ -367,7 +373,7 @@ fn undecided_unit( values, line: unit.locations.first().map_or(1, |l| l.start_line), questions, - fingerprint: fingerprint(unit.rule, plan, &unit.identity), + fingerprint: known(plan, unit).0, locations: unit.locations.clone(), open: quotes.open(unit, &open, answers), } @@ -376,7 +382,7 @@ fn undecided_unit( /// The questions whose answers left a unit undecided: a custom unit is /// asked its one question. fn open_questions(unit: &UnitPlan, answers: &Answers<'_>) -> Vec<&'static str> { - if let Detail::Custom(_) = unit.detail { + if let Detail::Custom(..) = unit.detail { return answers .get(super::answers::CUSTOM) .map(|_| super::answers::CUSTOM) @@ -628,17 +634,6 @@ fn basis(noun: &str, count: &UnitCounts) -> String { parts.join(" ") } -fn fingerprint(rule: &str, plan: &FilePlan, identity: &str) -> String { - let separator = crate::schema::HASH_SEPARATOR; - hash( - format!( - "{rule}{separator}{}{separator}{identity}", - plan.path.display() - ) - .as_bytes(), - ) -} - fn rank(probability: f64, lines: usize) -> f64 { probability * (1.0 + lines as f64).ln() } @@ -698,7 +693,7 @@ fn finding( symbol = None; look_wording(unit) } - Detail::Function { .. } | Detail::Pair | Detail::Values { .. } => look_wording(unit), + Detail::Function { .. } | Detail::Pair { .. } | Detail::Values { .. } => look_wording(unit), Detail::Security { sites, messages, .. } => { @@ -756,7 +751,7 @@ fn finding( comment_wording(name, &[(&unit.locations[0], reason)], strength) } Detail::Test { .. } => test_wording(name, strength, answers), - Detail::Custom(question) => { + Detail::Custom(question, _) => { if matches!( question.unit, crate::custom::Kind::File | crate::custom::Kind::Hunk @@ -780,6 +775,7 @@ fn finding( if let Some(block) = block { locations.insert(0, block.location.clone()); } + let (fingerprint, identity) = known(plan, unit); Finding { rule: catalog::id(unit.rule).into(), strength, @@ -789,14 +785,14 @@ fn finding( action: action.into(), symbol, rule_version: match &unit.detail { - Detail::Custom(question) => question.version.clone(), + Detail::Custom(question, _) => question.version.clone(), _ => catalog::rule_version(unit.rule).into(), }, concern_probability: p, locations, quote: unit.quote.clone(), category, - fingerprint: fingerprint(unit.rule, plan, &unit.identity), + fingerprint, rank: rank(p, lines), baselined: false, suppressed: None, @@ -804,5 +800,6 @@ fn finding( precision: None, preview: None, untouched: Vec::new(), + identity, } } diff --git a/src/units/compose/redundant.rs b/src/units/compose/redundant.rs index 4e96729..69bbf9f 100644 --- a/src/units/compose/redundant.rs +++ b/src/units/compose/redundant.rs @@ -120,6 +120,7 @@ pub(super) fn group_finding(plan: &FilePlan, cluster: Cluster<'_>) -> Finding { let identity: Vec<&str> = std::iter::once(subject.as_str()) .chain(tests.iter().map(|t| t.as_str())) .collect(); + let identity = crate::units::identity(&identity); Finding { rule: catalog::id(catalog::TEST_REDUNDANCY).into(), strength: Strength::Consider, @@ -137,11 +138,7 @@ pub(super) fn group_finding(plan: &FilePlan, cluster: Cluster<'_>) -> Finding { locations, quote: None, category: None, - fingerprint: fingerprint( - catalog::TEST_REDUNDANCY, - plan, - &crate::units::identity(&identity), - ), + fingerprint: fingerprint(catalog::TEST_REDUNDANCY, plan, &identity), rank: rank(p, lines), baselined: false, suppressed: None, @@ -149,5 +146,6 @@ pub(super) fn group_finding(plan: &FilePlan, cluster: Cluster<'_>) -> Finding { precision: None, preview: None, untouched: Vec::new(), + identity: renamed_only(catalog::TEST_REDUNDANCY, plan, &identity), } } diff --git a/src/units/custom/examples.rs b/src/units/custom/examples.rs index 278504f..c39aaae 100644 --- a/src/units/custom/examples.rs +++ b/src/units/custom/examples.rs @@ -136,7 +136,7 @@ fn items_of(kind: Kind, file: &FileContext<'_>) -> Result> { Ok(match kind { Kind::Section => items::sections(file), Kind::File => vec![items::whole(file)], - Kind::Hunk => items::changed(file, &hunks::example(file.source)), + Kind::Hunk => items::changed(file, &hunks::example(file.source), &[]), Kind::Function | Kind::Comment | Kind::Test => { let parsed = crate::analysis::units::parse(file.path, file.source)?; ensure!( diff --git a/src/units/custom/hunks.rs b/src/units/custom/hunks.rs index fb669b7..380b7d0 100644 --- a/src/units/custom/hunks.rs +++ b/src/units/custom/hunks.rs @@ -1,6 +1,8 @@ //! The changed hunks of a file since `--base`, read from Git, for custom //! questions about what a change adds or alters. A hunk works in any -//! language: it needs a diff, not a parser. +//! language: it needs a diff, not a parser. Each run of changed lines is +//! its own hunk, as a diff without context lines has it, so a change +//! nearby, such as a parent branch's, never joins it. use std::{ collections::BTreeMap, path::{Path, PathBuf}, @@ -10,6 +12,10 @@ use std::{ /// file, is asked about in parts, each well inside one request. const MAX_LINES: usize = 80; +/// Unchanged lines a hunk shows on each side of its changes, as Git's own +/// diff does. +const CONTEXT: usize = 3; + /// One changed hunk, or part of one. #[derive(Clone, Debug, PartialEq)] pub(super) struct Hunk { @@ -23,6 +29,10 @@ pub(super) struct Hunk { pub context: Option, /// Only its added and removed lines, which identify it wherever it moves. pub changed: String, + /// The added and removed lines of the part of Git's hunk it was found + /// in, which identified it in JevGate 0.35 and earlier, when the runs + /// of changed lines within a few lines of each other were one hunk. + pub joined: String, } impl Hunk { @@ -93,7 +103,11 @@ impl Changes { let Ok(diff) = crate::revision::git(&self.root, &args) else { return added(source); }; - let hunks = parse(&String::from_utf8_lossy(&diff), source.lines().count()); + let hunks = parse( + &String::from_utf8_lossy(&diff), + source.lines().count(), + true, + ); if hunks.is_empty() && previous.is_none() { added(source) } else { @@ -103,16 +117,17 @@ impl Changes { } /// The hunks of one file's unified diff, parts of at most `MAX_LINES` -/// lines each; `lines` is how many lines the file has now. An empty line +/// lines each; `lines` is how many lines the file has now, and with +/// `apart`, each run of changed lines is a hunk of its own. An empty line /// inside a hunk is a blank line of context, as Git prints one when /// `diff.suppressBlankEmpty` is set. -pub(super) fn parse(diff: &str, lines: usize) -> Vec { +pub(super) fn parse(diff: &str, lines: usize, apart: bool) -> Vec { let mut hunks = Vec::new(); let mut open: Option<(usize, Option, Vec<&str>)> = None; for line in diff.lines() { if let Some(header) = line.strip_prefix("@@ ") { if let Some(hunk) = open.take() { - split(hunk, lines, &mut hunks); + split(hunk, (lines, apart), &mut hunks); } open = header_start(header).map(|(start, context)| (start, context, Vec::new())); } else if let Some((_, _, body)) = open.as_mut() { @@ -121,24 +136,25 @@ pub(super) fn parse(diff: &str, lines: usize) -> Vec { None => body.push(" "), // "\ No newline at end of file" Some(b'\\') => {} - _ => split(open.take().unwrap(), lines, &mut hunks), + _ => split(open.take().unwrap(), (lines, apart), &mut hunks), } } } if let Some(hunk) = open { - split(hunk, lines, &mut hunks); + split(hunk, (lines, apart), &mut hunks); } hunks } /// The hunks of an example change written by hand: diff lines under `@@` -/// headers, or with none, one change from line 1. An empty line is an -/// unchanged blank line, since editors strip the space a diff gives it. +/// headers, or with none, one change from line 1, each hunk whole as its +/// author wrote it. An empty line is an unchanged blank line, since +/// editors strip the space a diff gives it. pub(super) fn example(diff: &str) -> Vec { let headed = diff.lines().any(|line| line.starts_with("@@ ")); let header = if headed { "" } else { "@@ -1 +1 @@\n" }; // No line count to keep within: the example is all the file there is. - parse(&format!("{header}{diff}"), usize::MAX) + parse(&format!("{header}{diff}"), usize::MAX, false) } /// A new file's text as hunks that add every line. @@ -146,7 +162,7 @@ fn added(source: &str) -> Vec { let body: Vec = source.lines().map(|line| format!("+{line}")).collect(); let body: Vec<&str> = body.iter().map(String::as_str).collect(); let mut hunks = Vec::new(); - split((1, None, body), source.lines().count(), &mut hunks); + split((1, None, body), (source.lines().count(), true), &mut hunks); hunks } @@ -167,42 +183,112 @@ fn header_start(header: &str) -> Option<(usize, Option)> { } /// A hunk's body in parts of at most `MAX_LINES` lines, each with the lines -/// it changes; a part holding only context is left out. +/// it changes; a part holding only context is left out. With `apart`, each +/// run of changed lines is a part, with up to `CONTEXT` unchanged lines on +/// each side; without, the body is cut into parts in order. fn split( (start, context, body): (usize, Option, Vec<&str>), - lines: usize, + (lines, apart): (usize, bool), hunks: &mut Vec, ) { let last_line = lines.max(1); - // The line of the file as it is now that the next context or added line - // is, and where a removed line was. - let mut next = start.max(1); - for part in body.chunks(MAX_LINES) { - let mut changed = Vec::new(); - let mut range: Option<(usize, usize)> = None; - for line in part { - let kind = line.as_bytes()[0]; - if kind == b'+' || kind == b'-' { - range = Some(range.map_or((next, next), |(first, last)| (first, last.max(next)))); - changed.push(*line); - } - if kind != b'-' { - next += 1; - } - } - let Some((first, last)) = range else { + let at = lines_now(start, &body); + // The parts JevGate 0.35 cut a body into, whose changes identified them. + let joined: Vec = body.chunks(MAX_LINES).map(changes).collect(); + let parts = if apart { + runs(&body) + } else { + in_order(body.len()) + }; + for (from, first, last, to) in parts { + let changed: Vec = (first..last).filter(|&i| changes_line(body[i])).collect(); + let (Some(&a), Some(&b)) = (changed.first(), changed.last()) else { continue; }; hunks.push(Hunk { - start: first.min(last_line), - end: last.min(last_line), - diff: part.join("\n"), + start: at[a].min(last_line), + end: at[b].min(last_line), + diff: body[from..to].join("\n"), context: context.clone(), - changed: changed.join("\n"), + changed: changes(&body[first..last]), + joined: joined[a / MAX_LINES].clone(), }); } } +/// The line of the file as it is now that each line of a hunk's body +/// starting at line `start` is, and where a removed line was. +fn lines_now(start: usize, body: &[&str]) -> Vec { + let mut next = start.max(1); + body.iter() + .map(|line| { + let at = next; + if !line.starts_with('-') { + next += 1; + } + at + }) + .collect() +} + +/// The added and removed lines of `part`. +fn changes(part: &[&str]) -> String { + part.iter() + .filter(|line| changes_line(line)) + .copied() + .collect::>() + .join("\n") +} + +/// A body of `len` lines in parts of at most `MAX_LINES`, in order, as +/// [`runs`] gives them. +fn in_order(len: usize) -> Vec<(usize, usize, usize, usize)> { + (0..len) + .step_by(MAX_LINES) + .map(|first| { + let last = (first + MAX_LINES).min(len); + (first, first, last, last) + }) + .collect() +} + +/// Whether a diff body line is added or removed. +fn changes_line(line: &str) -> bool { + line.starts_with('+') || line.starts_with('-') +} + +/// Each run of changed lines in a hunk's body, in parts of at most +/// `MAX_LINES`, as indexes `(from, first, last, to)`: the part is +/// `first..last`, and its diff `from..to` adds the unchanged lines around it, +/// up to `CONTEXT` on each side and never another run's. +fn runs(body: &[&str]) -> Vec<(usize, usize, usize, usize)> { + let unchanged = |i: &usize| !changes_line(body[*i]); + let mut parts = Vec::new(); + let mut i = 0; + while i < body.len() { + if !changes_line(body[i]) { + i += 1; + continue; + } + let end = (i..body.len()).find(|j| unchanged(j)).unwrap_or(body.len()); + for first in (i..end).step_by(MAX_LINES) { + let last = (first + MAX_LINES).min(end); + let before = (first.saturating_sub(CONTEXT)..first) + .rev() + .take_while(unchanged) + .last() + .unwrap_or(first); + let after = (last..(last + CONTEXT).min(body.len())) + .take_while(unchanged) + .last() + .map_or(last, |j| j + 1); + parts.push((before, first, last, after)); + } + i = end; + } + parts +} + #[cfg(test)] mod tests { use super::*; @@ -210,7 +296,7 @@ mod tests { #[test] fn a_diff_splits_into_hunks_at_the_lines_they_change() { let diff = "diff --git a/src/a.rs b/src/a.rs\nindex 1..2 100644\n--- a/src/a.rs\n+++ b/src/a.rs\n@@ -10,4 +10,5 @@ fn charge(order: &Order) {\n let total = order.total();\n- log(total);\n+ log(order.body());\n+ audit(total);\n send(total);\n@@ -40,2 +41,1 @@\n keep();\n-drop();\n\\ No newline at end of file\n"; - let hunks = parse(diff, 60); + let hunks = parse(diff, 60, true); assert_eq!(hunks.len(), 2); assert_eq!((hunks[0].start, hunks[0].end), (11, 12)); assert_eq!( @@ -227,21 +313,63 @@ mod tests { (42, 42, "42".to_string()), "a removal counts at the line that now follows it" ); - assert_eq!(parse(diff, 41)[1].start, 41, "at most the file's last line"); - assert!(parse("Binary files a/x.png and b/x.png differ\n", 1).is_empty()); + assert_eq!( + parse(diff, 41, true)[1].start, + 41, + "at most the file's last line" + ); + assert!(parse("Binary files a/x.png and b/x.png differ\n", 1, true).is_empty()); } #[test] fn a_blank_line_of_context_printed_empty_stays_in_its_hunk() { // As Git prints it with `diff.suppressBlankEmpty` set. let diff = "@@ -1,5 +1,5 @@ def charge(order):\n def charge(order):\n- x = 1\n+ x = 2\n\n- y = 2\n+ y = log(order.body)\n return x + y\n"; - let hunks = parse(diff, 5); + let hunks = parse(diff, 5, false); assert_eq!(hunks.len(), 1); assert_eq!((hunks[0].start, hunks[0].end), (2, 4)); assert!(hunks[0].changed.ends_with("+ y = log(order.body)")); assert!(hunks[0].diff.contains("+ x = 2\n \n- y = 2")); } + /// A change a line away from another, such as a parent branch's, is a + /// hunk of its own, so its identity is the same whether or not the + /// other is in the diff. + #[test] + fn each_run_of_changed_lines_is_a_hunk_with_the_unchanged_lines_around_it() { + let diff = "@@ -1,7 +1,7 @@ def charge(order):\n def charge(order):\n- x = 1\n+ x = 2\n z = 0\n- y = 2\n+ y = log(order.body)\n return x + y\n pass\n"; + let hunks = parse(diff, 7, true); + let found: Vec<_> = hunks + .iter() + .map(|h| (h.start, h.end, h.changed.as_str())) + .collect(); + assert_eq!( + found, + [ + (2, 2, "- x = 1\n+ x = 2"), + (4, 4, "- y = 2\n+ y = log(order.body)") + ] + ); + assert_eq!( + hunks[0].diff, + " def charge(order):\n- x = 1\n+ x = 2\n z = 0" + ); + assert_eq!( + hunks[1].diff, + " z = 0\n- y = 2\n+ y = log(order.body)\n return x + y\n pass" + ); + let alone = parse( + "@@ -3,3 +3,3 @@\n z = 0\n- y = 2\n+ y = log(order.body)\n return x + y\n", + 7, + true, + ); + assert_eq!(alone[0].changed, hunks[1].changed); + assert_eq!( + hunks[0].joined, hunks[1].joined, + "both were one hunk before" + ); + } + #[test] fn a_long_hunk_or_new_file_is_asked_in_parts_without_context_only_parts() { let source: String = (1..=170).map(|n| format!("line {n}\n")).collect(); @@ -255,7 +383,7 @@ mod tests { let mut body = vec!["+new"]; body.extend(std::iter::repeat_n(" same", MAX_LINES)); let mut hunks = Vec::new(); - split((5, None, body), 200, &mut hunks); + split((5, None, body), (200, true), &mut hunks); assert_eq!(hunks.len(), 1, "the part of context alone is left out"); assert_eq!((hunks[0].start, hunks[0].end), (5, 5)); } diff --git a/src/units/custom/items.rs b/src/units/custom/items.rs index 6738af4..e3ce826 100644 --- a/src/units/custom/items.rs +++ b/src/units/custom/items.rs @@ -30,6 +30,8 @@ pub(super) struct Item { pub lines: usize, /// What its findings' fingerprints keep across unrelated edits. pub identity: String, + /// The identity JevGate 0.35 and earlier gave it, when it differs. + pub v1: Option, pub quote: Option, /// Where runs of packed items end, as the built-in stage keys them: a /// definition's name, a comment's owner, a heading, a hunk's content. @@ -60,6 +62,7 @@ pub(super) fn functions( reach: (line_of(file.source, unit.span.start), unit.end_line), lines: unit.lines(), identity: identity(&[&unit.name, &compact(source)]), + v1: None, quote: None, run: unit.name.clone(), } @@ -84,6 +87,7 @@ pub(super) fn tests(file: &FileContext<'_>, cases: &[TestCase]) -> Vec { reach: (line_of(file.source, case.span.start), case.end_line), lines: case.end_line + 1 - case.line, identity: identity(&[&case.name, &compact(source)]), + v1: None, quote: None, run: case.name.clone(), } @@ -122,6 +126,7 @@ pub(super) fn comments( reach: (comment.line, comment.end_line), lines: comment.end_line + 1 - comment.line, identity: identity(&[owner, &compact(&comment.text)]), + v1: None, quote: Some(comment.text.clone()), run: owner.to_string(), }) @@ -153,6 +158,7 @@ pub(super) fn sections(file: &FileContext<'_>) -> Vec { reach: (section.start_line, section.end_line), lines: section.end_line + 1 - section.start_line, identity: identity(&[§ion.heading, &compact(§ion.text)]), + v1: None, quote: None, run: section.heading.clone(), name, @@ -176,17 +182,21 @@ pub(super) fn whole(file: &FileContext<'_>) -> Item { reach: (1, lines), lines, identity: identity(&["file", &compact(file.source)]), + v1: None, quote: None, run: String::new(), } } -/// Changed hunks. A hunk's findings are identified by the definition Git -/// names for it and what it changes, without line numbers, so they follow -/// the change when lines above it move. -pub(super) fn changed(file: &FileContext<'_>, hunks: &[Hunk]) -> Vec { +/// Changed hunks. A hunk's findings are identified by the definition +/// JevGate's parser finds around it and the lines it changes, without line +/// numbers, so they follow the change when lines above it move or a change +/// nearby comes and goes; a second hunk with the same changes in the same +/// definition is told apart by its order. +pub(super) fn changed(file: &FileContext<'_>, hunks: &[Hunk], units: &[Unit]) -> Vec { let labels: Vec = hunks.iter().map(Hunk::lines).collect(); let ids = unique_ids("hunk", labels.iter().map(String::as_str)); + let mut seen = std::collections::BTreeMap::::new(); hunks .iter() .zip(labels) @@ -196,10 +206,12 @@ pub(super) fn changed(file: &FileContext<'_>, hunks: &[Hunk]) -> Vec { if let Some(context) = &hunk.context { state["in"] = json!(context); } - let changed = identity(&[ - hunk.context.as_deref().unwrap_or_default(), - &compact(&hunk.changed), - ]); + let mut changed = identity(&[enclosing(units, hunk), &compact(&hunk.changed)]); + let earlier = seen.entry(changed.clone()).or_default(); + *earlier += 1; + if *earlier > 1 { + changed = identity(&[&changed, &earlier.to_string()]); + } Item { state, name: if hunk.start == hunk.end { @@ -212,9 +224,23 @@ pub(super) fn changed(file: &FileContext<'_>, hunks: &[Hunk]) -> Vec { reach: (hunk.start, hunk.end), lines: hunk.end + 1 - hunk.start, identity: changed.clone(), + v1: Some(identity(&[ + hunk.context.as_deref().unwrap_or_default(), + &compact(&hunk.joined), + ])), quote: None, run: changed, } }) .collect() } + +/// The name of the innermost definition around a hunk's changed lines, +/// empty at the top level. +fn enclosing<'u>(units: &'u [Unit], hunk: &Hunk) -> &'u str { + units + .iter() + .filter(|u| u.line <= hunk.start && hunk.end <= u.end_line) + .min_by_key(|u| u.span.len()) + .map_or("", |u| u.name.as_str()) +} diff --git a/src/units/custom/mod.rs b/src/units/custom/mod.rs index e2329bf..b3e9393 100644 --- a/src/units/custom/mod.rs +++ b/src/units/custom/mod.rs @@ -213,7 +213,11 @@ impl Planner { (Kind::File, _) => Some(vec![items::whole(file)]), (Kind::Hunk, _) => { let hunks = self.changes.as_ref()?.hunks(file.path, file.source); - Some(items::changed(file, &hunks)) + let units = match offered { + Offered::Code(code) => &code.parsed.units[..], + _ => &[], + }; + Some(items::changed(file, &hunks, units)) } _ => None, } @@ -301,7 +305,7 @@ fn unit(question: &'static Question, item: &Item) -> UnitPlan { quote: item.quote.clone(), lines: item.lines, identity: item.identity.clone(), - detail: Detail::Custom(question), + detail: Detail::Custom(question, item.v1.clone()), recheck: None, } } diff --git a/src/units/duplicates.rs b/src/units/duplicates.rs index 3434efe..02fb7de 100644 --- a/src/units/duplicates.rs +++ b/src/units/duplicates.rs @@ -35,6 +35,7 @@ pub(super) fn plan( ); let sent = file.push_fitting(build(file, pair, hashes, &id), requests); let presence = Presence::judged_if(sent); + let members = members(pair); out.units.push(UnitPlan { rule: SHARED_LOGIC, name: match pair.copies.len() { @@ -61,25 +62,46 @@ pub(super) fn plan( }, id, presence, - locations: [&pair.a, &pair.b] - .into_iter() - .chain(&pair.copies) - .map(location) - .collect(), + locations: sites(pair).map(location).collect(), quote: Some(pair.a.quote.clone()), lines: pair.a.end_line + 1 - pair.a.start_line, - identity: identity(&[ - pair.a.function.as_deref().unwrap_or(""), - &pair.b.path.to_string_lossy(), - pair.b.function.as_deref().unwrap_or(""), - &pair.normalized, - ]), - detail: Detail::Pair, + identity: identity(&members.iter().map(String::as_str).collect::>()), + detail: Detail::Pair { + members, + v1: identity(&[ + pair.a.function.as_deref().unwrap_or(""), + &pair.b.path.to_string_lossy(), + pair.b.function.as_deref().unwrap_or(""), + &pair.normalized, + ]), + }, recheck: None, }); } } +/// The copies a pair's finding is identified by, in order: each function +/// as `path::function`, whichever window of it repeats and whichever pair +/// represents the group, and a copy outside a function as `path#hash` of +/// the repeated statements. Neither the file that owns the finding nor +/// which files a check selected changes them. +fn members(pair: &Pair) -> Vec { + let mut members: Vec = sites(pair) + .map(|site| match &site.function { + Some(function) => format!("{}::{function}", site.path.display()), + None => format!("{}#{}", site.path.display(), pair.normalized), + }) + .collect(); + members.sort(); + members.dedup(); + members +} + +/// A pair's copies: the two it was found from, then the group's others. +fn sites(pair: &Pair) -> impl Iterator { + [&pair.a, &pair.b].into_iter().chain(&pair.copies) +} + fn site_label(site: &Site) -> String { match &site.function { Some(function) => format!("`{function}` ({}:{})", site.path.display(), site.start_line), @@ -119,11 +141,7 @@ fn build( LOOK, Pass::First, ); - let sites: Vec<&Site> = [&pair.a, &pair.b] - .into_iter() - .chain(&pair.copies) - .take(SHOWN_SITES) - .collect(); + let sites: Vec<&Site> = sites(pair).take(SHOWN_SITES).collect(); let state = json!({"sites": sites.iter().map(|s| site_state(s)).collect::>()}); let mut sources = vec![(file.path, file.source_hash)]; for site in &sites { diff --git a/src/units/grouping.rs b/src/units/grouping.rs index e5687f7..0f46658 100644 --- a/src/units/grouping.rs +++ b/src/units/grouping.rs @@ -80,6 +80,7 @@ fn group_finished_plans(files: &mut [FileResult]) -> BTreeSet { "Delete the finished plans, or move them out of the living documentation".into(); primary.locations.extend(locations); primary.fingerprint = directory_fingerprint(primary, &directory); + primary.identity = Default::default(); } changed } @@ -237,6 +238,7 @@ fn head_finding(finding: &mut Finding, head: &Section, names: &[String], locatio .unwrap_or_default(); finding.category = Some(REPEATED_SECTION.into()); finding.fingerprint = section_fingerprint(finding, &head.0, &heading); + finding.identity = Default::default(); } /// A section a finding names: its path and first line. diff --git a/src/units/mod.rs b/src/units/mod.rs index 9955206..94f89fb 100644 --- a/src/units/mod.rs +++ b/src/units/mod.rs @@ -181,10 +181,17 @@ pub enum Detail { /// split question raises a review or consider. locate: Option, }, - /// A file's outline: a test file's cases, or an application's members. - Outline { tests: bool }, + /// A file's outline: a test file's cases, or an application's members, + /// whose names a baseline matches a changed outline by. + Outline { tests: bool, members: Vec }, /// A group of repeated snippets, its first site owned by the file. - Pair, + Pair { + /// Each copy as `path::function`, or `path#hash` outside a + /// function, in order: what identifies the finding. + members: Vec, + /// The identity JevGate 0.35 and earlier gave it. + v1: String, + }, /// A function and the literal values it uses. Values { values: Vec }, /// A comment of application code and the unit it documents or sits in. @@ -272,8 +279,9 @@ pub enum Detail { /// first answer says it asserts internal details. confirm: Option, }, - /// A unit a custom question asks about. - Custom(&'static crate::custom::Question), + /// A unit a custom question asks about, and for a changed hunk the + /// identity JevGate 0.35 and earlier gave it. + Custom(&'static crate::custom::Question, Option), TestPair { names: [String; 2], subject: String, @@ -345,7 +353,7 @@ impl UnitPlan { check.iter().chain(settle).collect() } Detail::Test { confirm } | Detail::TestPair { confirm, .. } => confirm.iter().collect(), - Detail::Pair + Detail::Pair { .. } | Detail::Outline { .. } | Detail::Values { .. } | Detail::Constants { .. } @@ -355,7 +363,7 @@ impl UnitPlan { | Detail::Access(_) | Detail::Job { .. } | Detail::Law - | Detail::Custom(_) => Vec::new(), + | Detail::Custom(..) => Vec::new(), }; self.recheck.iter().chain(planned) } @@ -374,6 +382,9 @@ pub struct FilePlan { pub questions: Vec<&'static crate::custom::Question>, /// What syntax errors left out of this file's units, as the report names it. pub left_out: Vec, + /// The path the file had before a rename the checked change made: its + /// findings are also known by their fingerprints under it. + pub previous: Option, } impl UnitPlan { diff --git a/src/units/outcome/mod.rs b/src/units/outcome/mod.rs index 2500fc2..2aa84fc 100644 --- a/src/units/outcome/mod.rs +++ b/src/units/outcome/mod.rs @@ -257,7 +257,7 @@ pub(super) fn unit_outcome(unit: &UnitPlan, answers: &Answers<'_>) -> Outcome { /// The outcome of a unit's answers under its rule's policy. fn rule_outcome(unit: &UnitPlan, answers: &Answers<'_>) -> Option { let get = |q: &str| answers.get(q).copied(); - if let Detail::Custom(question) = &unit.detail { + if let Detail::Custom(question, _) = &unit.detail { return get(super::answers::CUSTOM).map(|answer| custom_outcome(question, answer)); } match unit.rule { @@ -356,7 +356,7 @@ fn in_examples(unit: &UnitPlan, outcome: Outcome) -> Outcome { .iter() .all(|l| crate::analysis::clones::example_code(&l.path)) && !unit.locations.is_empty() - && !matches!(unit.detail, Detail::Custom(_)); + && !matches!(unit.detail, Detail::Custom(..)); match outcome { Outcome::Review(p) | Outcome::Consider(p) if example diff --git a/src/units/outline.rs b/src/units/outline.rs index 85e9ba3..b148895 100644 --- a/src/units/outline.rs +++ b/src/units/outline.rs @@ -6,8 +6,7 @@ //! findings; on files labeled for splitting, the look-here question flagged //! 6 of 9 against 5 of 54 kept. use super::{ - Detail, FileContext, FilePlan, Planned, Presence, Questions, UnitPlan, identity, outcome::LOOK, - questions, + Detail, FileContext, FilePlan, Planned, Presence, Questions, UnitPlan, outcome::LOOK, questions, }; use crate::{ analysis::{ @@ -174,8 +173,13 @@ fn plan_outline( locations: vec![file.location(first, last, None)], quote: None, lines: file.source.lines().count(), - identity: identity(&names), - detail: Detail::Outline { tests }, + // A file has one outline, so its path identifies it, and a + // baseline matches its members by similarity. + identity: String::new(), + detail: Detail::Outline { + tests, + members: names.iter().map(|name| name.to_string()).collect(), + }, recheck: None, }); if fits && !small { diff --git a/src/units/plan/mod.rs b/src/units/plan/mod.rs index 1c094bd..600a5f5 100644 --- a/src/units/plan/mod.rs +++ b/src/units/plan/mod.rs @@ -159,6 +159,7 @@ pub fn plan( custom.text(&context, source, &mut file, &mut result.requests); result.files.insert(owner, file); } + note_renames(inputs, &mut result); keep_changed(inputs, &mut result); // Text addressed to a reviewer is asked about once the requests that // send it are known: with `--base`, those the change touched. @@ -220,7 +221,7 @@ fn chosen_when_planned(unit: &UnitPlan) -> bool { | Detail::Document { .. } | Detail::Stale { .. } | Detail::Plan { .. } - | Detail::Custom(_) + | Detail::Custom(..) ) } @@ -239,6 +240,19 @@ fn skip_left_out(scope: &Scope<'_>, result: &mut Plan) { } } +/// The path each file a change renamed had before, which its findings are +/// also known by. +fn note_renames(inputs: &[Input], result: &mut Plan) { + for (&owner, file) in &mut result.files { + file.previous = inputs[owner] + .changed + .as_ref() + .and_then(|change| change.previous()) + .filter(|before| *before != file.path) + .map(Path::to_path_buf); + } +} + /// Each GitHub Actions workflow file's jobs. fn plan_workflows(scope: &Scope<'_>, args: &CheckArgs, budget: Limits<'_>, result: &mut Plan) { for &owner in &scope.configuration { diff --git a/src/units/tests/changed.rs b/src/units/tests/changed.rs index 9079003..2ebeffc 100644 --- a/src/units/tests/changed.rs +++ b/src/units/tests/changed.rs @@ -4,7 +4,7 @@ use super::*; use std::path::Path; /// A check of what changed since `base`, for `rules`. -fn since(base: &str, rules: &[&str]) -> CheckArgs { +pub(super) fn since(base: &str, rules: &[&str]) -> CheckArgs { let mut options = args(); options.rules = rules.iter().map(|r| r.to_string()).collect(); options.base = Some(base.into()); @@ -227,7 +227,7 @@ fn a_copy_pair_is_asked_when_either_copy_changed() { /// `LOAD` loading `what` from `field`, then a tail that is no copy, /// edited beside the copy when `tail_edited`, and with the copy's default /// changed when `copy_edited`: a literal, so the copies still match. -fn load_copy( +pub(super) fn load_copy( what: &str, field: &str, tail: String, @@ -249,7 +249,7 @@ fn load_copy( } /// The shared-logic findings of `report`, by the file each is in. -fn copies_found(report: &Report) -> Vec<(&Path, &crate::schema::Finding)> { +pub(super) fn copies_found(report: &Report) -> Vec<(&Path, &crate::schema::Finding)> { report .files .iter() @@ -710,10 +710,56 @@ unit = "hunk" "def charge(order):\n x = 2\n\n y = log(order.body)\n return x + y\n", ); let (_, plan) = planned(&project, &custom_since_head(toml)); - assert_eq!(unit_names(&plan, "m.py"), ["lines 2–4"]); - let diff = &plan.requests[0].request["state"]["hunks"][0]["diff"]; + // Each run of changed lines is its own hunk. + assert_eq!(unit_names(&plan, "m.py"), ["line 2", "line 4"]); + let diff = &plan.requests[0].request["state"]["hunks"][1]["diff"]; assert!( - diff.as_str().unwrap().contains("+ y = log(order.body)"), + diff.as_str() + .unwrap() + .starts_with(" \n- y = 2\n+ y = log(order.body)"), "{diff}" ); } + +/// A hunk is identified by the definition around it and its own changed +/// lines: a change a line away, such as a parent branch's, or a function +/// added above leaves it as it was. +#[test] +fn a_hunk_keeps_its_identity_when_a_change_nearby_or_a_function_above_comes_and_goes() { + let toml = r#" +[[question]] +id = "no-body-logs" +question = "Does this change log a request body?" +unit = "hunk" +"#; + let project = Project::new(); + let base = "def charge(order):\n x = 1\n z = 0\n y = 2\n return x + y\n"; + project.write("m.py", base); + project.commit_all(); + let hunks = |source: &str| { + project.write("m.py", source); + let (_, plan) = planned(&project, &custom_since_head(toml)); + file_plan(&plan, "m.py") + .units + .iter() + .map(|unit| (unit.identity.clone(), unit.detail.clone())) + .collect::>() + }; + let logged = base.replace("y = 2", "y = log(order.body)"); + let alone = hunks(&logged); + assert_eq!(alone.len(), 1); + let v1 = |detail: &Detail| match detail { + Detail::Custom(_, v1) => v1.clone(), + _ => None, + }; + let nearby = hunks(&logged.replace("x = 1", "x = 2")); + assert_eq!(nearby.len(), 2, "two runs of changed lines"); + assert_eq!(nearby[1].0, alone[0].0); + assert_ne!( + v1(&nearby[1].1), + v1(&alone[0].1), + "JevGate 0.35 joined the two into one hunk" + ); + let above = hunks(&format!("def audit():\n pass\n\n\n{logged}")); + assert_eq!(above.last().unwrap().0, alone[0].0); +} diff --git a/src/units/tests/custom.rs b/src/units/tests/custom.rs index edd5277..f380780 100644 --- a/src/units/tests/custom.rs +++ b/src/units/tests/custom.rs @@ -3,8 +3,8 @@ use super::*; use crate::config::ConfigContext; /// Answers every custom question `yes` and every built-in one clear. -struct Custom { - yes: f64, +pub(super) struct Custom { + pub yes: f64, } impl crate::transport::Evaluator for Custom { @@ -56,12 +56,12 @@ fn custom_units<'p>(plan: &'p Plan, name: &str) -> Vec<&'p UnitPlan> { file_plan(plan, name) .units .iter() - .filter(|u| matches!(u.detail, Detail::Custom(_))) + .filter(|u| matches!(u.detail, Detail::Custom(..))) .collect() } /// The findings of a custom rule across a report. -fn findings_of<'r>(report: &'r Report, rule: &str) -> Vec<&'r crate::schema::Finding> { +pub(super) fn findings_of<'r>(report: &'r Report, rule: &str) -> Vec<&'r crate::schema::Finding> { report .files .iter() @@ -463,6 +463,38 @@ unit = "file" ); } +/// A whole file's finding is identified by its path and text, and a check +/// of a change that renamed the file finds it under the old path too. +#[test] +fn a_renamed_files_finding_stays_accepted_through_its_old_path() { + let toml = r#" +[[question]] +id = "no-secrets" +question = "Does this file hold a hardcoded secret?" +unit = "file" +"#; + let project = Project::new(); + project.write("lib.rs", &function("charge")); + project.commit_all(); + let report = run( + &project, + &configured(toml, &["custom"]), + &mut Custom { yes: 0.95 }, + ); + accept_all(&project, &report); + project.git(&["mv", "lib.rs", "billing.rs"]); + let mut moved = configured(toml, &["custom"]); + moved.base = Some("HEAD".into()); + let report = run(&project, &moved, &mut Custom { yes: 0.95 }); + let finding = findings_of(&report, "custom/no-secrets")[0]; + assert_eq!( + finding.locations[0].path, + std::path::Path::new("billing.rs") + ); + assert_eq!(finding.identity.aliases.len(), 1); + assert!(finding.baselined, "accepted under lib.rs"); +} + #[test] fn test_comment_and_section_questions_ride_in_their_built_in_requests() { let toml = r#" diff --git a/src/units/tests/fingerprints.rs b/src/units/tests/fingerprints.rs new file mode 100644 index 0000000..8395d76 --- /dev/null +++ b/src/units/tests/fingerprints.rs @@ -0,0 +1,226 @@ +//! A repeat's fingerprint, which names its copies, and a baseline written by +//! JevGate 0.35, which still accepts findings through the fingerprints they +//! had then until the next baseline write rewrites it. +use super::{ + changed::{copies_found, load_copy}, + duplicates::LOAD, + *, +}; +use crate::{baseline, options::Disposition}; +use std::path::Path; + +/// Shared-logic and function-simplification arguments, for the whole project. +fn whole() -> CheckArgs { + let mut options = args(); + options.rules = vec![ + catalog::SHARED_LOGIC.into(), + catalog::FUNCTION_SIMPLIFICATION.into(), + ]; + options +} + +/// `LOAD` in `a.rs` and its copy loading a team in `b.rs`. +fn copied() -> Project { + let project = Project::new(); + project.write("a.rs", LOAD); + project.write("b.rs", ©("load_team", "\"title\"")); + project +} + +fn copy(what: &str, field: &str) -> String { + load_copy(what, field, String::new(), (false, false)) +} + +/// `jevgate baseline mark REASON TARGET`. +fn mark(project: &Project, reason: Disposition, target: String) -> usize { + baseline::mark( + &project.0, + &baseline::Mark { + reason, + note: None, + targets: &[target], + rules: &[], + }, + ) + .unwrap() +} + +fn target(path: &Path, finding: &crate::schema::Finding) -> String { + format!("{}:{}", path.display(), finding.line) +} + +#[test] +fn a_repeat_has_one_fingerprint_whichever_copy_a_check_selects() { + let project = copied(); + let all = run(&project, &whole(), &mut scripted(2)); + let found = copies_found(&all); + assert_eq!(found.len(), 1, "{found:?}"); + let (owner, repeat) = found[0]; + assert_eq!(owner, Path::new("a.rs")); + assert_eq!( + repeat.identity.members, + ["a.rs::load_user", "b.rs::load_team"] + ); + // A check of `b.rs` reading `a.rs` as context owns the repeat in `b.rs`. + let mut from_b = whole(); + from_b.paths = vec!["b.rs".into()]; + from_b.context = vec!["a.rs".into()]; + let report = run(&project, &from_b, &mut scripted(2)); + let found = copies_found(&report); + assert_eq!(found.len(), 1, "{found:?}"); + assert_eq!(found[0].0, Path::new("b.rs")); + assert_eq!(found[0].1.fingerprint, repeat.fingerprint); + assert_ne!( + found[0].1.identity.v1, repeat.identity.v1, + "JevGate 0.35 identified it by the copy the check selected" + ); +} + +/// A repeat stays accepted while every copy is one an accepted repeat +/// names, and is asked about again when a new copy joins it. +#[test] +fn a_repeat_a_new_copy_joins_is_asked_about_again_and_one_a_copy_leaves_is_not() { + let project = copied(); + accept_all(&project, &run(&project, &whole(), &mut scripted(2))); + project.write("c.rs", ©("load_org", "\"label\"")); + let report = run(&project, &whole(), &mut scripted(2)); + let found = copies_found(&report); + assert_eq!(found.len(), 1, "{found:?}"); + assert_eq!(found[0].1.identity.members.len(), 3); + assert!(!found[0].1.baselined, "a copy no entry names joined"); + crate::storage::Store::open(&project.0) + .unwrap() + .publish(&report) + .unwrap(); + assert_eq!( + mark( + &project, + Disposition::Intended, + target(found[0].0, found[0].1) + ), + 1 + ); + std::fs::remove_file(project.0.join("b.rs")).unwrap(); + let report = run(&project, &whole(), &mut scripted(2)); + let found = copies_found(&report); + assert_eq!( + found[0].1.identity.members, + ["a.rs::load_user", "c.rs::load_org"] + ); + assert!(found[0].1.baselined, "both copies are accepted ones"); +} + +/// The repeat of [`copied`] and a long function in `c.rs`, committed, with +/// a baseline as JevGate 0.35 wrote it: the repeat accepted under the +/// fingerprint it had then, for later, with a note. Returns the check's +/// report. +fn upgraded() -> (Project, Report) { + let project = copied(); + project.write("c.rs", &long_function("f")); + let report = run(&project, &whole(), &mut scripted(2)); + let (_, repeat) = copies_found(&report)[0]; + let v1 = repeat.identity.v1.clone().unwrap(); + assert_ne!(v1, repeat.fingerprint); + let old = json!({ + "version": 1, + "created_at": 1, + "findings": [{ + "fingerprint": v1, + "rule": repeat.rule, + "path": "a.rs", + "line": repeat.line, + "message": "Old wording.", + "reason": "later", + "note": "#12", + }], + }); + project.write( + baseline::BASELINE_FILE, + &serde_json::to_string_pretty(&old).unwrap(), + ); + project.commit_all(); + let report = run(&project, &whole(), &mut scripted(2)); + crate::storage::Store::open(&project.0) + .unwrap() + .publish(&report) + .unwrap(); + (project, report) +} + +/// The fingerprint, reason and note of each entry of the baseline. +fn entries(project: &Project) -> Vec<(String, Option, Option)> { + baseline::list(&project.0, &[], &[]) + .unwrap() + .into_iter() + .map(|l| (l.fingerprint, l.reason, l.note)) + .collect() +} + +#[test] +fn an_old_baseline_accepts_through_earlier_fingerprints_until_a_write_rewrites_it() { + let (project, report) = upgraded(); + let (_, repeat) = copies_found(&report)[0]; + assert!(repeat.baselined, "accepted through its v1 fingerprint"); + let function = report + .files + .iter() + .find(|f| f.path == Path::new("c.rs")) + .map(|f| &f.findings[0]) + .unwrap(); + assert!(!function.baselined); + // Marking another finding rewrites the old entry, keeping its reason + // and note; the turn's dismissals are the mark alone. + assert_eq!( + mark( + &project, + Disposition::Wrong, + target(Path::new("c.rs"), function) + ), + 1 + ); + assert_eq!( + entries(&project), + [ + ( + repeat.fingerprint.clone(), + Some(Disposition::Later), + Some("#12".into()) + ), + (function.fingerprint.clone(), Some(Disposition::Wrong), None), + ] + ); + let dismissals = baseline::Dismissals::since(&project.0, "HEAD").unwrap(); + assert_eq!(dismissals.of(repeat), None, "accepted as the turn began"); + assert_eq!( + dismissals.of(function).map(|d| d.reason), + Some(Disposition::Wrong) + ); + assert_eq!( + crate::gate::exit_code(&run(&project, &whole(), &mut scripted(2))), + 0 + ); +} + +#[test] +fn writing_the_baseline_over_an_old_one_moves_each_entry_to_its_current_fingerprint() { + for merge in [false, true] { + let (project, report) = upgraded(); + let (_, repeat) = copies_found(&report)[0]; + baseline::write(&project.0, merge, None).unwrap(); + let rewritten = entries(&project); + assert_eq!(rewritten.len(), 2, "merge {merge}: no v1 entry is kept"); + assert!( + rewritten.contains(&( + repeat.fingerprint.clone(), + Some(Disposition::Later), + Some("#12".into()) + )), + "merge {merge}: {rewritten:?}" + ); + let text = std::fs::read_to_string(project.0.join(baseline::BASELINE_FILE)).unwrap(); + assert!( + text.contains("\"members\""), + "a repeat's entry names its copies" + ); + } +} diff --git a/src/units/tests/mod.rs b/src/units/tests/mod.rs index ef789c1..e365559 100644 --- a/src/units/tests/mod.rs +++ b/src/units/tests/mod.rs @@ -7,6 +7,7 @@ mod django; mod documentation; mod duplicates; mod examples; +mod fingerprints; mod functions; mod handlers; mod hardcoded; @@ -48,6 +49,15 @@ fn planned(project: &Project, options: &CheckArgs) -> (Vec, Plan) { } /// A `lib.rs` holding `count` judged functions `f0`, `f1`… +/// Accept every finding of `report`, as `jevgate baseline` does after the check. +fn accept_all(project: &Project, report: &Report) { + crate::storage::Store::open(&project.0) + .unwrap() + .publish(report) + .unwrap(); + crate::baseline::write(&project.0, false, None).unwrap(); +} + fn functions_project(count: usize) -> Project { let project = Project::new(); let source: String = (0..count).map(|i| function(&format!("f{i}"))).collect(); diff --git a/src/units/tests/organization.rs b/src/units/tests/organization.rs index 36c7d88..c024a53 100644 --- a/src/units/tests/organization.rs +++ b/src/units/tests/organization.rs @@ -53,6 +53,28 @@ fn a_file_the_look_question_flags_is_one_review_for_an_agent_to_verify() { } } +/// An outline is identified by its file's path, and an accepted one +/// accepts the file while most of its members are the ones accepted. +#[test] +fn an_accepted_outline_stays_accepted_while_its_members_stay_similar() { + let (project, options) = organized("lib.rs", &two_concerns()); + let first = run(&project, &options, &mut scripted(2)); + accept_all(&project, &first); + let grown = format!("{}{}", two_concerns(), function("warm16")); + project.write("lib.rs", &grown); + let report = run(&project, &options, &mut scripted(2)); + let finding = &report.files[0].findings[0]; + assert!(finding.baselined, "34 of its 35 members accepted"); + assert_eq!(finding.fingerprint, first.files[0].findings[0].fingerprint); + let more: String = (17..27).map(|i| function(&format!("warm{i}"))).collect(); + project.write("lib.rs", &format!("{grown}{more}")); + let report = run(&project, &options, &mut scripted(2)); + assert!( + !report.files[0].findings[0].baselined, + "34 of its 45 members accepted: asked about again" + ); +} + #[test] fn a_function_another_file_passes_by_path_names_that_file_as_its_user() { let project = Project::new(); diff --git a/src/units/wording/maintainability.rs b/src/units/wording/maintainability.rs index 46cafcb..aee456e 100644 --- a/src/units/wording/maintainability.rs +++ b/src/units/wording/maintainability.rs @@ -90,7 +90,7 @@ pub(in crate::units) fn look_wording(unit: &crate::units::UnitPlan) -> Wording { "This file may do several separate kinds of work, such as separate features, layers or integrations.".into(), "Move each separate part to its own module, or dismiss this finding with a reason", ), - Detail::Pair => ( + Detail::Pair { .. } => ( format!("{name} may repeat one piece of logic, so a change to it would have to be made in each place."), "Keep the logic in one shared function, or dismiss this finding with a reason if the copies must stay separate", ),