diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a03181..c2004b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,9 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver ## [Unreleased] +- File organization: a long application file (400 lines or more) whose outline, recheck and kind of file raised no finding is asked about its candidate parts, one request per part with the part's source: whether it does a job of its own that a reader would look for apart from the rest, and what it is within the file. A part of 100 lines or more whose answer reaches 0.65 and whose role leans to a job of its own is a consider naming its members. The kind of file cleared long single-type files wholesale: of 50 such files labeled from the code, 15 were worth splitting (a URL scraper inside lobsters' `Story`, a diff engine inside a renderer, a JSON parser inside a protocol module), and asking each part found 4 of them, at the part the labeler named, and no file to keep. The parts are the outline's groups without the links that merged every method of a large class into one group and without links through a helper most members call, with links for neighbours and for names sharing a distinctive word. On the corpus, 11 considers were added and nothing else changed: 9 right and 2 wrong (a demo page's placeholder table, a class's public API), 5 of 7 right outside the files used for tuning and 1 of 1 on the held-out projects; on 9 projects never used before (click, rich, zod, hono, viper, ripgrep, sinatra, jsoup, guzzle), 2 of 2 (Guzzle's `WWW-Authenticate` parser inside `DigestAuth`, ripgrep's `--hyperlink-format` language). File-organization considers were 63% right before. Benchmarks, examples and `scripts` and `docs` directories are not asked (5 of 5 such findings were wrong), nor is a part holding `main`. About $0.08 on the corpus. JevGate's own large files stay clear: their parts read as one job, and what decided them by hand, which other files use a part, is missing for Rust functions passed by name. +- A request whose answer is already cached fits the provider limit, whatever the token calibration says. The calibration (`.jevgate/token-budget.json`) is replaced after each run by the bytes per token of that run's fresh requests, and a request near the limit, such as a recheck that sends a whole file, was sent in one run and not the next: the file's finding changed with no change to its code. On the corpus, 0.23.1 under three calibrations (the last run's, a snapshot's and a stricter one) differed in one consider and five undecided units, and under the stricter one asked 42 thousand new tokens of questions; with this, only three units whose recheck the provider refused as too long still differ, and nothing new is asked. `--refresh` skips the cache, so it plans by the estimate alone. + ## [0.23.1] - 2026-09-27 - Injection: the follow-up that asks a path finding what its paths can hold now also asks a markup finding what its values hold where they enter the markup (already escaped, percent-encoded or serialized as a URL; typed; the program's own; or raw text) and a redirect finding where its targets can lead (a fixed path such as `/admin` first keeps it on the site; `origin + next` with no slash between them does not). Leaning to harmless values, the finding is a note. On the 38 corpus projects with such findings, vaultwarden's `hibp_breach` (a username percent-encoded before the link), its admin login redirect and shiori's login redirect, all labeled wrong, are notes; the 26 markup and 6 redirect findings labeled right put at most 0.22 and 0.44 on the harmless options and stay. About $0.01 on the corpus. diff --git a/docs/classification-cascade.md b/docs/classification-cascade.md index ca473bc..b646555 100644 --- a/docs/classification-cascade.md +++ b/docs/classification-cascade.md @@ -154,6 +154,26 @@ signatures, or one candidate pair. whole gets no recheck, so its undecided first answer is asked the kind from the outline alone; large Java classes and their test files otherwise stayed uncertain. + An application file of 400 lines or more whose outline, recheck and kind + raised no finding is then asked about its candidate parts, one request + per part: the part's members with their source, the file's other members + by signature, whether the part does a job of its own that a reader would + look for apart from the rest, and what it is within the file (a job of + its own, more of what the rest does, helpers the rest uses throughout, or + the file's main job). A part of 100 lines or more whose Noul reaches 0.65 + and whose role leans to a job of its own raises a consider naming its + members. Asked of the whole outline, the split and the kind of file read + a URL scraper inside a Rails model and a diff engine inside a renderer as + one feature: of 50 long files the kind cleared, labeled from the code, + 15 were worth splitting, and asking each part found 4 of them, each at the + part the labeler named, and no file to keep. The parts are the outline's + groups without the links a type's members share when it has more than + twelve (those merged every method of a large class into one group), + without links through a helper most members call, and with a link for + neighbours and for names sharing a distinctive word. Benchmarks, + examples, `scripts` and `docs` directories are not asked (a script runs + its steps top to bottom and a benchmark is often pinned by hash: 5 of 5 + such findings were wrong), nor is a part holding `main`. A test left undecided on whether it re-implements the code or checks only its mocks is asked again with the bodies of the functions it calls and its file's imports, mocks and setup hooks (a part too long is left out, never diff --git a/site/src/how-it-works.md b/site/src/how-it-works.md index bbf95af..02dea1e 100644 --- a/site/src/how-it-works.md +++ b/site/src/how-it-works.md @@ -172,6 +172,26 @@ signatures, or one candidate pair. whole gets no recheck, so its undecided first answer is asked the kind from the outline alone; large Java classes and their test files otherwise stayed uncertain. + An application file of 400 lines or more whose outline, recheck and kind + raised no finding is then asked about its candidate parts, one request + per part: the part's members with their source, the file's other members + by signature, whether the part does a job of its own that a reader would + look for apart from the rest, and what it is within the file (a job of + its own, more of what the rest does, helpers the rest uses throughout, or + the file's main job). A part of 100 lines or more whose Noul reaches 0.65 + and whose role leans to a job of its own raises a consider naming its + members. Asked of the whole outline, the split and the kind of file read + a URL scraper inside a Rails model and a diff engine inside a renderer as + one feature: of 50 long files the kind cleared, labeled from the code, + 15 were worth splitting, and asking each part found 4 of them, each at the + part the labeler named, and no file to keep. The parts are the outline's + groups without the links a type's members share when it has more than + twelve (those merged every method of a large class into one group), + without links through a helper most members call, and with a link for + neighbours and for names sharing a distinctive word. Benchmarks, + examples, `scripts` and `docs` directories are not asked (a script runs + its steps top to bottom and a benchmark is often pinned by hash: 5 of 5 + such findings were wrong), nor is a part holding `main`. A test left undecided on whether it re-implements the code or checks only its mocks is asked again with the bodies of the functions it calls and its file's imports, mocks and setup hooks (a part too long is left out, never diff --git a/src/analysis/groups.rs b/src/analysis/groups.rs index da271a3..bf25b46 100644 --- a/src/analysis/groups.rs +++ b/src/analysis/groups.rs @@ -37,6 +37,145 @@ pub fn groups(units: &[Unit], members: &[usize], imports: &BTreeSet) -> .collect() } +/// Candidate parts of a long file, for the part follow-up: `groups` links +/// every method of a class through their owner, so a scraper inside a +/// model or a codec inside a manager never stood apart. Here methods of a +/// type with more than `PART_OWNER_MEMBERS` members link only through calls +/// and shared names, calls to a helper that more than three in ten members +/// call do not link, and members next to each other or sharing a distinctive +/// word of their names link once more. Scored against the parts that +/// labelers named on 23 files, the best matching part rose from 0.45 to +/// 0.72 (F1 over member lines), and from 5 to 9 of the 15 files to split. +pub fn parts(units: &[Unit], members: &[usize], imports: &BTreeSet) -> Vec> { + clusters(part_weights(units, members, imports)) + .into_iter() + .map(|set| set.into_iter().map(|m| members[m]).collect()) + .collect() +} + +/// A type with more members than this holds parts of its own. +const PART_OWNER_MEMBERS: usize = 12; +/// The share of members calling a helper above which calls to it do not link. +const HUB_SHARE: f64 = 0.3; +/// The share of members whose names a word may appear in and still link them. +const DISTINCT_WORD_SHARE: f64 = 0.25; + +fn part_weights(units: &[Unit], members: &[usize], imports: &BTreeSet) -> Vec> { + let n = members.len(); + let unit = |i: usize| &units[members[i]]; + let names = linking_names(units, members, imports); + let tallies = Tallies::of(units, members); + let calls = |x: &Unit, y: &Unit| { + !tallies.hub(&y.short_name) + && (x.calls.contains(&y.short_name) + || !y.owner.is_empty() && x.calls.contains(&y.owner) && !tallies.hub(&y.owner)) + }; + let mut weights = vec![vec![0u32; n]; n]; + for i in 0..n { + for j in i + 1..n { + let (a, b) = (unit(i), unit(j)); + let mut weight = names[i].intersection(&names[j]).count().min(2) as u32; + if calls(a, b) || calls(b, a) { + weight += CALL_WEIGHT; + } + if !a.owner.is_empty() && a.owner == b.owner && !tallies.large(&a.owner) { + weight += OWNER_WEIGHT; + } + weight += u32::from(j == i + 1) + u32::from(tallies.share_word(i, j)); + weights[i][j] = weight; + weights[j][i] = weight; + } + } + weights +} + +/// What the part links weigh against, counted over a file's members: how +/// many call each name, how many each type owns, and the words of each +/// member's name with how many names hold each word. +struct Tallies<'u> { + members: usize, + called: BTreeMap<&'u str, usize>, + owners: BTreeMap<&'u str, usize>, + words: Vec>, + spread: BTreeMap, +} + +impl<'u> Tallies<'u> { + fn of(units: &'u [Unit], members: &[usize]) -> Self { + let mut tallies = Tallies { + members: members.len(), + called: BTreeMap::new(), + owners: BTreeMap::new(), + words: Vec::with_capacity(members.len()), + spread: BTreeMap::new(), + }; + for unit in members.iter().map(|&m| &units[m]) { + for call in &unit.calls { + *tallies.called.entry(call.as_str()).or_default() += 1; + } + if !unit.owner.is_empty() { + *tallies.owners.entry(unit.owner.as_str()).or_default() += 1; + } + let words = name_words(&unit.short_name); + for word in &words { + *tallies.spread.entry(word.clone()).or_default() += 1; + } + tallies.words.push(words); + } + tallies + } + + /// A helper that more than `HUB_SHARE` of the members call. + fn hub(&self, name: &str) -> bool { + self.called + .get(name) + .is_some_and(|&c| c as f64 > (self.members as f64 * HUB_SHARE).max(3.0)) + } + + /// A type with more than `PART_OWNER_MEMBERS` members. + fn large(&self, owner: &str) -> bool { + self.owners + .get(owner) + .is_some_and(|&c| c > PART_OWNER_MEMBERS) + } + + /// Whether the names of members `i` and `j` share a word that at most + /// `DISTINCT_WORD_SHARE` of the members' names hold. + fn share_word(&self, i: usize, j: usize) -> bool { + let distinct = (self.members as f64 * DISTINCT_WORD_SHARE).max(2.0); + self.words[i] + .intersection(&self.words[j]) + .any(|w| self.spread[w] as f64 <= distinct) + } +} + +/// The lowercase words of a name longer than two letters, split at +/// underscores, punctuation and camel-case humps: `fetchedAttributesHtml` +/// and `fetched_attributes_pdf` share `fetched` and `attributes`. +fn name_words(name: &str) -> BTreeSet { + let mut words = BTreeSet::new(); + let mut word = String::new(); + let mut previous: Option = None; + for c in name.trim_start_matches(['#', '_']).chars() { + let hump = + c.is_uppercase() && previous.is_some_and(|p| p.is_lowercase() || p.is_ascii_digit()); + if !c.is_alphanumeric() || hump { + if word.chars().count() > 2 { + words.insert(std::mem::take(&mut word)); + } + word.clear(); + } + if c.is_alphanumeric() { + word.extend(c.to_lowercase()); + } + previous = Some(c); + } + if word.chars().count() > 2 { + words.insert(word); + } + words +} + /// Group a test file's cases and the support code they share: cases link by /// their innermost suite and by the subjects and helpers they share, and a /// case that calls a helper links to it. Positions `0..cases.len()` are the @@ -307,6 +446,61 @@ mod tests { } } + #[test] + fn a_large_type_is_parted_by_its_calls_not_its_owner() { + let scraper = [ + "fetched_attributes", + "fetched_html", + "fetched_pdf", + "canonical_target", + ]; + let mut source = String::from("struct Story { title: String }\nimpl Story {\n"); + for i in 0..9 { + source.push_str(&format!( + " fn field{i}(&self) -> usize {{ self.title.len() + {i} }}\n" + )); + } + for name in scraper { + let calls: Vec = scraper + .iter() + .filter(|other| **other != name) + .map(|other| format!("self.{other}()")) + .collect(); + source.push_str(&format!( + " fn {name}(&self) -> usize {{ {} }}\n", + calls.join(" + ") + )); + } + source.push_str("}\n"); + let file = super::super::units::parse(Path::new("story.rs"), &source).unwrap(); + let members: Vec = (0..file.units.len()).collect(); + let name = |m: &usize| file.units[*m].short_name.as_str(); + assert!( + groups(&file.units, &members, &file.imports) + .iter() + .any(|g| g.members.iter().any(|m| name(m) == "field0") + && g.members.iter().any(|m| name(m) == scraper[0])), + "a shared owner links the scraper to the fields" + ); + let parts = parts(&file.units, &members, &file.imports); + let scraping = parts + .iter() + .find(|p| p.iter().any(|m| name(m) == scraper[0])) + .unwrap(); + assert_eq!(scraping.iter().map(name).collect::>(), scraper); + } + + #[test] + fn name_words_split_humps_underscores_and_private_marks() { + let words = |name: &str| name_words(name).into_iter().collect::>(); + assert_eq!( + words("fetchedAttributesHtml"), + ["attributes", "fetched", "html"] + ); + assert_eq!(words("#parse_UTF8_value"), ["parse", "utf8", "value"]); + assert_eq!(words("to"), Vec::::new()); + } + #[test] fn constructing_a_class_links_to_its_methods() { let source = "export class GatewayError extends Error {\n constructor(code: string) {\n super(code)\n }\n}\nexport function requireLive(at: number) {\n if (Date.now() >= at) throw new GatewayError('EXPIRED')\n}\n"; diff --git a/src/catalog.rs b/src/catalog.rs index fee3786..7200554 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -287,7 +287,7 @@ pub fn rules() -> Vec { pub fn rule_version(key: &str) -> &'static str { match key { - FILE_ORGANIZATION => "21", + FILE_ORGANIZATION => "22", FUNCTION_SIMPLIFICATION => "16", SHARED_LOGIC => "22", TEST_VALUE => "7", diff --git a/src/evaluate.rs b/src/evaluate.rs index 0043180..994b4dd 100644 --- a/src/evaluate.rs +++ b/src/evaluate.rs @@ -3,7 +3,7 @@ use super::{ options::CheckArgs, schema::{self, FileResult, Report, Status}, storage::Store, - token_budget::TokenBudget, + token_budget::{Limits, TokenBudget}, transport::Evaluator, }; use crate::config::ConfigContext; @@ -129,13 +129,16 @@ fn empty_report(args: &CheckArgs, current: &SnapshotContext<'_>, files: Vec {} Ok(Scheduled::Purpose(request)) => { match cached_purpose(input, args, root, &request, &mut report.files[owner]) { @@ -218,6 +221,7 @@ impl Session<'_> { // Traces judge where a security concern's values come from; rechecks // settle uncertain units; a security check still undecided is asked // where its URL comes from or its output goes, and an outline its kind; + // a long file left without a finding is asked about its parts; // locate follow-ups then point split findings at a block, and a // located value is asked what it is. Each depends on the answers // before it. @@ -227,6 +231,7 @@ impl Session<'_> { crate::units::rechecks, crate::units::settles, crate::units::kinds, + crate::units::parts, crate::units::locates, crate::units::value_kinds, ] { @@ -261,12 +266,17 @@ impl Session<'_> { ) { let mut purpose = Vec::new(); let mut views = BTreeMap::new(); + let root = &self.context.root; + let answered = |request: &serde_json::Value| { + crate::requests::answered(root, self.args, request).is_some() + }; + let limits = Limits::new(&self.budget, &answered); for (owner, file) in report.files.iter_mut().enumerate() { if file.status != Status::Pending { continue; } file.judgments.clear(); - match schedule(&inputs[owner], self.args, &self.budget, file) { + match schedule(&inputs[owner], self.args, limits, file) { Ok(Scheduled::None) => file.cached = false, Ok(Scheduled::Purpose(request)) => { file.cached = true; @@ -549,7 +559,7 @@ fn apply_classification(file: &mut FileResult, class: crate::file_kind::Classifi fn schedule( input: &Input, args: &CheckArgs, - budget: &TokenBudget, + budget: Limits<'_>, file: &mut FileResult, ) -> Result { if file.status != Status::Pending { diff --git a/src/file_kind.rs b/src/file_kind.rs index 03ba15c..96eabfd 100644 --- a/src/file_kind.rs +++ b/src/file_kind.rs @@ -9,7 +9,7 @@ use crate::{ options::CheckArgs, policy, schema::{FileResult, SourceRange, Status}, - token_budget::TokenBudget, + token_budget::Limits, }; use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; @@ -165,7 +165,7 @@ pub fn language(path: &Path) -> &'static str { } } -pub(crate) fn plan(input: &Input, args: &CheckArgs, budget: &TokenBudget) -> Result { +pub(crate) fn plan(input: &Input, args: &CheckArgs, budget: Limits<'_>) -> Result { let format = crate::docs::format::Format::of(&input.result.path).language(); if input.result.role == crate::inventory::INSTRUCTIONS { return Ok(Plan::Ready(document( @@ -640,8 +640,8 @@ fn extension(path: &Path) -> String { #[cfg(test)] mod tests { use super::*; - use crate::schema::Status; use crate::tests::{Project, args, run}; + use crate::{schema::Status, token_budget::TokenBudget}; const MIXED: &str = "fn production(value: &str) -> String {\n value.trim().to_string()\n}\n\n#[cfg(test)]\nmod tests {\n use super::production;\n\n fn helper(value: &str) -> String {\n production(value)\n }\n\n #[test]\n fn checks_production() {\n assert_eq!(helper(\" a \"), \"a\");\n }\n}\n"; @@ -657,7 +657,7 @@ mod tests { let input = crate::inventory::collect(options, &project.context(), &[]) .unwrap() .remove(0); - match plan(&input, options, &TokenBudget::default()).unwrap() { + match plan(&input, options, TokenBudget::default().uncached()).unwrap() { Plan::Ready(view) => view, _ => panic!("expected a gate view"), } @@ -736,7 +736,7 @@ mod tests { .unwrap() .remove(0); assert!(matches!( - plan(&input, &args(), &TokenBudget::default()).unwrap(), + plan(&input, &args(), TokenBudget::default().uncached()).unwrap(), Plan::Purpose(..) )); // Outside a test path, `main` is the program's entry. diff --git a/src/requests.rs b/src/requests.rs index d3c1bc3..8c80916 100644 --- a/src/requests.rs +++ b/src/requests.rs @@ -29,7 +29,7 @@ pub(super) struct Receipt { } /// Request kinds reported in `stages`, in dispatch order. -pub(crate) const STAGES: [&str; 20] = [ +pub(crate) const STAGES: [&str; 21] = [ "file-purpose", "functions", "outline", @@ -43,6 +43,7 @@ pub(crate) const STAGES: [&str; 20] = [ "comments", "security", "trace", + "parts", "settle", "instructions", "docs", diff --git a/src/token_budget.rs b/src/token_budget.rs index d853ced..e1dfb8b 100644 --- a/src/token_budget.rs +++ b/src/token_budget.rs @@ -23,6 +23,43 @@ const MAX_BYTES_PER_TOKEN: f64 = 6.0; /// through that the provider refused as beyond its context. const STRUCTURED_BYTES_PER_TOKEN: f64 = 2.0; +/// The budget as a run applies it to one request: the calibrated estimate, +/// or the answer cache when it already holds that request's answer. The +/// calibration follows the fresh requests of the last run, so a request near +/// the limit fit in one run and not the next: two runs of one release on a +/// pinned project differed in a file's recheck, and so in its finding. A +/// request answered once fits from then on. +#[derive(Clone, Copy)] +pub struct Limits<'a> { + budget: &'a TokenBudget, + answered: &'a dyn Fn(&Value) -> bool, +} + +impl<'a> Limits<'a> { + pub fn new(budget: &'a TokenBudget, answered: &'a dyn Fn(&Value) -> bool) -> Self { + Self { budget, answered } + } + + pub fn fits(&self, request: &Value) -> bool { + self.budget.fits(request) || (self.answered)(request) + } + + pub fn fits_structured(&self, request: &Value) -> bool { + self.budget.fits_structured(request) || (self.answered)(request) + } +} + +#[cfg(test)] +impl TokenBudget { + /// The estimate alone, for plans made without an answer cache. + pub fn uncached(&self) -> Limits<'_> { + fn never(_: &Value) -> bool { + false + } + Limits::new(self, &never) + } +} + /// The bytes-per-token ratio, calibrated from observed `usage.input_tokens` and /// saved in `.jevgate/`. #[derive(Clone, Copy, Debug, serde::Serialize, serde::Deserialize, PartialEq)] diff --git a/src/units/access.rs b/src/units/access.rs index 4f26244..9c8e73c 100644 --- a/src/units/access.rs +++ b/src/units/access.rs @@ -11,7 +11,7 @@ use crate::{ inventory::Input, options::CheckArgs, schema::Pass, - token_budget::TokenBudget, + token_budget::Limits, }; use serde_json::{Value, json}; use std::{ @@ -46,7 +46,7 @@ fn project(path: &Path) -> PathBuf { pub(super) fn plan( files: &[(usize, &Input)], args: &CheckArgs, - budget: &TokenBudget, + budget: Limits<'_>, plans: &mut BTreeMap, requests: &mut Vec, ) { diff --git a/src/units/compose.rs b/src/units/compose.rs deleted file mode 100644 index 992b03a..0000000 --- a/src/units/compose.rs +++ /dev/null @@ -1,1866 +0,0 @@ -//! Pure composition from typed judgments to unit outcomes, rule dimensions, -//! findings and a file status; each unit's outcome comes from `outcome`. -use super::{ - Access, Block, Detail, FilePlan, Presence, UnitPlan, - outcome::{ - Answers, Outcome, at_most_note, benefit, checks, choice, choice_mass, confirmable, - document_split, logs_found, lowered, noul, open, origin_outcome, score, settled_checks, - several_kind, unit_outcome, value_signals, - }, - wording::{Wording, comment_reason, comment_wording}, - wording::{ - doc_pair_wording, document_wording, function_wording, handler_wording, law_wording, - module_wording, outline_wording, pair_wording, plan_wording, privilege_wording, - question_label, section_wording, security_wording, stale_wording, test_pair_wording, - test_wording, values_wording, - }, -}; -use crate::{ - catalog, - schema::{ - Answer, Dimension, Finding, Judgment, Location, Pass, Status, Strength, Undecided, - UnitCounts, hash, - }, -}; -use std::collections::{BTreeMap, BTreeSet}; - -pub struct Composed { - pub dimensions: BTreeMap, - pub findings: Vec, - pub status: Status, -} - -fn answers<'a>(judgments: &'a [Judgment], unit: &str, pass: Pass) -> Answers<'a> { - judgments - .iter() - .filter(|j| j.unit == unit && j.pass == pass) - .map(|j| (j.question.as_str(), &j.answer)) - .collect() -} - -fn security(rule: &str) -> bool { - catalog::SECURITY.contains(&rule) -} - -/// A security unit's first-pass and trace answers, with each recheck answer -/// (the origin or a check, seen with callers) in place of the traced one, -/// unless the traced answer is decisive and the recheck is not: an undecided -/// traced answer is replaced even by an undecided recheck, whose lean saw -/// more evidence. -fn security_answers<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> Answers<'a> { - let mut merged = answers(judgments, &unit.id, Pass::First); - merged.extend(answers(judgments, &unit.id, Pass::Trace)); - let judged = |question: &str, answer: &Answer| match question { - "origin" => origin_outcome(answer), - _ => noul(answer), - }; - for (question, answer) in answers(judgments, &unit.id, Pass::Recheck) { - let traced = merged.get(question).map(|a| judged(question, a)); - if judged(question, answer).decisive() || !traced.is_some_and(Outcome::decisive) { - merged.insert(question, answer); - } - } - // The settle answers sit beside the checks they settle, under their own - // names, and so does what an injection consider's values can hold. - merged.extend(answers(judgments, &unit.id, Pass::Settle)); - merged.extend(answers(judgments, &unit.id, Pass::Locate)); - merged -} - -/// The settle Choices a security unit calls for, not yet asked: those whose -/// checks stay undecided after the trace and recheck while the unit is -/// uncertain, and for a consider or note resting on an undecided check, -/// where its text goes (the finding claims it likely reaches a client) and -/// where code that requests a URL runs, and every Choice for an injection -/// note in Django code; and those asked whenever their checks are not -/// clear, such as what a PHP page joins into HTML. -pub fn unsettled(unit: &UnitPlan, judgments: &[Judgment]) -> BTreeSet<&'static str> { - use crate::units::security::{SETTLES, SettleWhen}; - if unit.presence != Presence::Judged - || !security(unit.rule) - || answers(judgments, &unit.id, Pass::Trace).is_empty() - { - return BTreeSet::new(); - } - let merged = security_answers(unit, judgments); - // Nearly every Django view places request values somewhere, so an - // injection note that no check found ("values from another party …, - // but no check found one placed unhandled") rests on its undecided - // checks, such as a redirect to its own path with an id in it. - let django_note = unit.rule == catalog::INJECTION - && matches!(unit.detail, Detail::Security { django: true, .. }); - let open = |when: SettleWhen| match unit_outcome(unit, &merged) { - Outcome::Uncertain(_) => true, - Outcome::Note(_) if django_note => true, - Outcome::Consider(_) | Outcome::Note(_) => when == SettleWhen::UndecidedOrFinding, - _ => false, - }; - let undecided = |q: &str| { - merged - .get(q) - .is_some_and(|a| matches!(noul(a), Outcome::Uncertain(_))) - }; - let not_clear = |q: &str| merged.get(q).is_some_and(|a| noul(a) != Outcome::Clear); - SETTLES - .iter() - .filter(|kind| kind.rule == unit.rule && !merged.contains_key(kind.question)) - .filter(|kind| match kind.when { - SettleWhen::NotClear => kind.checks.iter().any(|q| not_clear(q)), - when => open(when) && kind.checks.iter().any(|q| undecided(q)), - }) - .map(|kind| kind.question) - .collect() -} - -/// A unit's outcome and the answers it rests on: its first-pass answers with -/// the follow-ups its rule reads beside or in place of them. -fn resolved<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> (Outcome, Answers<'a>) { - let merged = if security(unit.rule) { - security_answers(unit, judgments) - } else if let Some(pass) = beside(unit.rule) { - beside_answers(unit, judgments, pass) - } else if unit.rule == catalog::COMMENTS { - comment_answers(unit, judgments) - } else if unit.rule == catalog::TEST_VALUE { - let merged = test_value_answers(unit, judgments); - let outcome = leaning_test(unit, judgments, unit_outcome(unit, &merged)); - return (outcome, merged); - } else if unit.rule == catalog::TEST_REDUNDANCY { - // Whether each test checks something the other does not, asked of a - // pair that reached a review, sits beside its answers. - let (_, mut merged) = rechecked(unit, judgments); - merged.extend(answers(judgments, &unit.id, Pass::Locate)); - merged - } else { - return rechecked(unit, judgments); - }; - (unit_outcome(unit, &merged), merged) -} - -/// A test whose hollow checks stay undecided once its recheck is asked (or -/// when it has none) leans: below 0.50 it is clear. Labeled from the code, -/// 4 of 43 such tests below 0.50 checked only their mocks or recomputed -/// their expected value (5 counting a test whose one real check is weak), -/// against 10 of 35 at 0.50 or more; 636 of the 792 undecided tests on the -/// corpus lean below. -fn leaning_test(unit: &UnitPlan, judgments: &[Judgment], outcome: Outcome) -> Outcome { - let rechecked = - unit.recheck.is_none() || !answers(judgments, &unit.id, Pass::Recheck).is_empty(); - match outcome { - Outcome::Uncertain(p) - if rechecked - && !crate::policy::probability_at_least(p, crate::policy::LEADING_PROBABILITY) => - { - Outcome::Clear - } - other => other, - } -} - -/// The pass of the follow-ups whose questions sit beside the first answers -/// under their own ids: document section and pair checks, the kind of a -/// large document, and benign-kind value checks. -fn beside(rule: &str) -> Option { - if [ - catalog::DOC_STALENESS, - catalog::DOC_DUPLICATION, - catalog::LARGE_DOCS, - ] - .contains(&rule) - { - Some(Pass::Trace) - } else if [ - catalog::HARDCODED_VALUES, - catalog::AGENT_CONTEXT, - catalog::WORKFLOWS, - ] - .contains(&rule) - { - Some(Pass::Recheck) - } else { - None - } -} - -fn beside_answers<'a>(unit: &UnitPlan, judgments: &'a [Judgment], pass: Pass) -> Answers<'a> { - let mut merged = answers(judgments, &unit.id, Pass::First); - merged.extend(answers(judgments, &unit.id, pass)); - // How a pair's sections relate, or what a section treats its missing - // names as, asked when its checks stay undecided. - merged.extend(answers(judgments, &unit.id, Pass::Settle)); - merged -} - -/// A comment's recheck replaces its first answers when the first stayed open -/// and the recheck decides, or neither decides; the kind of comment, asked -/// when it stays undecided, sits beside them. -fn comment_answers<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> Answers<'a> { - let first = answers(judgments, &unit.id, Pass::First); - let before = unit_outcome(unit, &first); - let recheck = answers(judgments, &unit.id, Pass::Recheck); - let mut merged = if !recheck.is_empty() - && open(unit, &first, before) - && (unit_outcome(unit, &recheck).decisive() || !before.decisive()) - { - recheck - } else { - first - }; - merged.extend(answers(judgments, &unit.id, Pass::Settle)); - merged -} - -/// A test recheck asks the hollow-test questions again with the code under -/// test and the setup; each answer replaces the first one unless only the -/// first is decisive. -fn test_value_answers<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> Answers<'a> { - let mut merged = answers(judgments, &unit.id, Pass::First); - for (question, answer) in answers(judgments, &unit.id, Pass::Recheck) { - let first = merged.get(question).map(|a| noul(a)); - if noul(answer).decisive() || !first.is_some_and(Outcome::decisive) { - merged.insert(question, answer); - } - } - // What its assertions read, asked after an internal-details consider. - merged.extend(answers(judgments, &unit.id, Pass::Locate)); - merged -} - -/// The first-pass outcome, or the recheck's when the first called for one -/// and the recheck decides. -fn rechecked<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> (Outcome, Answers<'a>) { - let first = answers(judgments, &unit.id, Pass::First); - let outcome = unit_outcome(unit, &first); - let mut recheck = answers(judgments, &unit.id, Pass::Recheck); - if unit.rule == catalog::FILE_ORGANIZATION { - // The kind is asked apart from the recheck and read beside its split, - // or beside the first split of a file too long for a recheck. - if unit.recheck.is_none() { - recheck.extend(first.iter().map(|(q, a)| (*q, *a))); - } - recheck.extend(answers(judgments, &unit.id, Pass::Trace)); - } - if open(unit, &first, outcome) && !recheck.is_empty() { - let second = unit_outcome(unit, &recheck); - if second.decisive() { - return (second, recheck); - } - } - (outcome, first) -} - -/// Security units whose finding a confirm Choice of their own follows, not -/// yet asked: an injection finding whose one concern is a path (what its -/// paths can hold), or markup or a redirect unless its values are asked -/// already (what they hold, where they lead), and a sensitive-data finding -/// its log checks raised (when the log line runs). -pub fn unconfirmed_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { - plan.units - .iter() - .filter(|u| u.presence == Presence::Judged) - .filter(|u| answers(judgments, &u.id, Pass::Locate).is_empty()) - .filter(|u| { - let Detail::Security { - checked, logging, .. - } = &u.detail - else { - return false; - }; - let (outcome, resolved) = resolved(u, judgments); - let get = |q: &str| resolved.get(q).copied(); - let kind = confirmable(&get); - matches!(outcome, Outcome::Review(_) | Outcome::Consider(_)) - && (checked.is_some() - && (kind == Some("path") || kind.is_some() && !values_due(outcome, &resolved)) - || logging.is_some() && logs_found(&get)) - }) - .map(|u| u.id.clone()) - .collect() -} - -/// Whether an injection outcome calls for what its values can hold: a -/// consider that rests on the function's parameters, its origin not -/// another party. -fn values_due(outcome: Outcome, resolved: &Answers<'_>) -> bool { - matches!(outcome, Outcome::Consider(_)) - && !resolved - .get("origin") - .is_some_and(|a| matches!(origin_outcome(a), Outcome::Review(_))) -} - -/// Functions whose split question raised a review or consider, and whose -/// block has not been located yet; hardcoded-value functions raised to a -/// review or consider whose value has not been named yet; and the other -/// units whose confirm `locate_due` calls for. -pub fn unlocated_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { - plan.units - .iter() - .filter(|u| u.presence == Presence::Judged) - .filter(|u| answers(judgments, &u.id, Pass::Locate).is_empty()) - .filter(|u| locate_due(u, judgments)) - .map(|u| u.id.clone()) - .collect() -} - -/// Whether a unit's outcome calls for its locate or confirm follow-up. -fn locate_due(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - let (outcome, resolved) = resolved(unit, judgments); - let raised = |o: Option| matches!(o, Some(Outcome::Review(_) | Outcome::Consider(_))); - match &unit.detail { - Detail::Values { - locate: Some(_), .. - } - | Detail::Constants { - locate: Some(_), .. - } => raised(Some(outcome)), - Detail::TestPair { - confirm: Some(_), .. - } => matches!(outcome, Outcome::Review(_)), - // Only the internal-details check raises a test's consider. - Detail::Test { confirm: Some(_) } => matches!(outcome, Outcome::Consider(_)), - // An injection consider rests on the function's parameters unless - // its origin was another party; one that rests on a path is asked - // what its paths can hold instead. - Detail::Security { - confirm: Some(_), .. - } => { - values_due(outcome, &resolved) - && confirmable(&|q| resolved.get(q).copied()) != Some("path") - } - Detail::Function { - locate: Some(_), .. - } => raised(resolved.get("split").map(|a| benefit(a))), - Detail::Document { - locate: Some(_), .. - } => raised( - resolved - .get("split") - .map(|a| document_split(a, resolved.get("kind").copied())), - ), - _ => false, - } -} - -/// Judged units whose first pass stayed undecided, or became a note from a -/// torn function or file-organization answer, and that have no recheck yet; -/// for injection, units whose traced origin stayed unclear or was the -/// function's parameters, so callers can settle it. -pub fn uncertain_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { - plan.units - .iter() - .filter(|u| u.presence == Presence::Judged) - .filter(|u| answers(judgments, &u.id, Pass::Recheck).is_empty()) - .filter(|u| { - if security(u.rule) { - origin_unsettled(u, judgments) - } else if u.rule == catalog::HARDCODED_VALUES { - value_undecided(u, judgments) - } else { - let first = answers(judgments, &u.id, Pass::First); - open(u, &first, unit_outcome(u, &first)) - } - }) - .map(|u| u.id.clone()) - .collect() -} - -/// An injection unit whose checks are not all clear while its traced origin -/// stayed unclear or was the function's parameters. -fn origin_unsettled(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - let merged = security_answers(unit, judgments); - let get = |q: &str| merged.get(q).copied(); - unit.rule == catalog::INJECTION - && !checks(unit.rule, &get).iter().all(|o| *o == Outcome::Clear) - && matches!( - merged.get("origin").map(|a| origin_outcome(a)), - Some(Outcome::Uncertain(_) | Outcome::Consider(_)) - ) -} - -/// A hardcoded-value unit with a question its first pass left undecided. -fn value_undecided(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - let first = answers(judgments, &unit.id, Pass::First); - let get = |q: &str| first.get(q).copied(); - value_signals(&get, &unit.detail, false).is_some_and(|signals| { - signals - .iter() - .any(|(_, o, _)| matches!(o, Outcome::Uncertain(_))) - }) -} - -/// Outlines whose recheck left the split Score undecided, or whose first -/// answer did when the file is too long for a recheck, and large documents -/// whose split Score stayed undecided, whose kind has not been asked yet; -/// section pairs and stale sections whose checks stayed undecided and whose -/// settle has not been asked yet. -pub fn unkinded_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { - plan.units - .iter() - .filter(|u| u.presence == Presence::Judged) - .filter(|u| match u.detail { - Detail::DocPair { .. } | Detail::Stale { .. } => unsettled_check(u, judgments), - Detail::Comment { .. } => unsettled_comment(u, judgments), - _ => unkinded_split(u, judgments), - }) - .map(|u| u.id.clone()) - .collect() -} - -/// An outline or large document not yet asked its kind whose split Score -/// stayed undecided: the recheck's for an outline that has one, else the -/// first. A large document's split finding is asked its kind as well, since -/// its Score reads headings alone, and so is an outline's finding its -/// recheck raised from an undecided first answer. -fn unkinded_split(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - if ![catalog::FILE_ORGANIZATION, catalog::LARGE_DOCS].contains(&unit.rule) - || !answers(judgments, &unit.id, Pass::Trace).is_empty() - { - return false; - } - let pass = if unit.recheck.is_some() && unit.rule != catalog::LARGE_DOCS { - Pass::Recheck - } else { - Pass::First - }; - let document = unit.rule == catalog::LARGE_DOCS; - answers(judgments, &unit.id, pass) - .get("split") - .is_some_and(|a| match benefit(a) { - Outcome::Uncertain(_) => true, - Outcome::Consider(_) | Outcome::Review(_) => document || pass == Pass::Recheck, - _ => false, - }) -} - -/// A section pair or stale section whose checks were asked, stayed -/// undecided, and whose settle has not been asked. -fn unsettled_check(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - !answers(judgments, &unit.id, Pass::Trace).is_empty() - && answers(judgments, &unit.id, Pass::Settle).is_empty() - && matches!(resolved(unit, judgments).0, Outcome::Uncertain(_)) -} - -/// A comment still undecided after its recheck, or without one, whose kind -/// has not been asked. -fn unsettled_comment(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - answers(judgments, &unit.id, Pass::Settle).is_empty() - && (unit.recheck.is_none() || !answers(judgments, &unit.id, Pass::Recheck).is_empty()) - && matches!(resolved(unit, judgments).0, Outcome::Uncertain(_)) -} - -/// Documents whose plan question found a plan whose work Git shows finished. -pub fn finished_plans( - plan: &super::Plan, - files: &[crate::schema::FileResult], -) -> BTreeSet { - plan.files - .iter() - .filter(|(owner, file_plan)| { - file_plan.units.iter().any(|u| { - matches!(u.detail, Detail::Plan { .. }) - && matches!( - resolved(u, &files[**owner].judgments).0, - Outcome::Consider(_) | Outcome::Review(_) - ) - }) - }) - .map(|(_, file_plan)| file_plan.path.clone()) - .collect() -} - -/// Security units whose presence is not clear, to trace. -pub fn untraced_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { - plan.units - .iter() - .filter(|u| u.presence == Presence::Judged && security(u.rule)) - .filter(|u| answers(judgments, &u.id, Pass::Trace).is_empty()) - .filter(|u| { - let first = answers(judgments, &u.id, Pass::First); - let presence: Vec = super::security::presence_questions(u.rule) - .iter() - .filter_map(|q| first.get(q).map(|a| noul(a))) - .collect(); - if presence.is_empty() { - return false; - } - presence.iter().any(|o| *o != Outcome::Clear) - }) - .map(|u| u.id.clone()) - .collect() -} - -pub fn compose(plan: &FilePlan, judgments: &[Judgment]) -> Composed { - let few = few_comment_lines(plan, judgments); - let mut tally = Tally::default(); - for (rule, omitted) in &plan.rules { - tally.counts.entry(rule).or_default().omitted = *omitted; - } - for unit in &plan.units { - tally.add(plan, unit, judgments, &few); - } - let Tally { - mut counts, - concern, - mut findings, - redundant, - commented, - mut undecided, - } = tally; - // A pair of tests in a group of three or more is reported by the group. - let (groups, grouped) = over_tested(plan, &redundant); - // A pair in a group reaches the group's consider; a lone pair is a note. - if let Some(count) = counts.get_mut(catalog::TEST_REDUNDANCY) { - count.note -= grouped.len(); - count.consider += grouped.len(); - } - let mut index = 0; - findings.retain(|_| { - index += 1; - !grouped.contains(&(index - 1)) - }); - findings.extend(groups); - drop_copies_of_redundant_tests(&mut findings); - findings.extend(comment_findings(plan, &commented)); - let dimensions = plan - .rules - .keys() - .map(|rule| { - let count = counts.remove(rule).unwrap_or_default(); - let concern = concern.get(rule).copied().unwrap_or(0.0); - let undecided = undecided.remove(rule).unwrap_or_default(); - (rule.to_string(), dimension(rule, count, concern, undecided)) - }) - .collect(); - // Strongest first, so a note never sits above a review or consider. - findings.sort_by(|a, b| b.strength.cmp(&a.strength).then(b.rank.total_cmp(&a.rank))); - let status = file_status(&dimensions, &findings); - Composed { - dimensions, - findings, - status, - } -} - -/// What a file's units add up to, per rule, before comment findings and -/// redundant tests are grouped. -#[derive(Default)] -struct Tally<'p> { - counts: BTreeMap<&'p str, UnitCounts>, - concern: BTreeMap<&'p str, f64>, - findings: Vec, - redundant: Vec>, - commented: Vec<(&'p UnitPlan, Strength, f64, &'static str)>, - undecided: BTreeMap<&'p str, Vec>, -} - -/// A shared-logic finding whose every copy lies inside tests a redundancy -/// finding already names says the same thing twice: on sqlite-utils, 12 -/// test pairs were reported by both rules. The redundancy finding stays, -/// since it says which test to merge or delete. -fn drop_copies_of_redundant_tests(findings: &mut Vec) { - let tests: Vec = findings - .iter() - .filter(|f| f.rule == catalog::id(catalog::TEST_REDUNDANCY)) - .flat_map(|f| f.locations.iter().cloned()) - .collect(); - let named = |l: &crate::schema::Location| { - tests - .iter() - .any(|t| t.path == l.path && t.start_line <= l.start_line && l.end_line <= t.end_line) - }; - findings.retain(|f| { - f.rule != catalog::id(catalog::SHARED_LOGIC) - || f.locations.is_empty() - || !f.locations.iter().all(named) - }); -} - -/// A redundant test pair, with the index of its finding (a note) when it -/// reached a consider, which a group of three or more tests reports instead. -struct Redundant<'p> { - unit: &'p UnitPlan, - names: &'p [String; 2], - subject: &'p String, - p: f64, - finding: Option, -} - -impl<'p> Tally<'p> { - /// Counts one unit under its rule and keeps what it contributes: a - /// finding, a comment to group, a redundant test pair or an undecided unit. - fn add( - &mut self, - plan: &FilePlan, - unit: &'p UnitPlan, - judgments: &[Judgment], - few: &BTreeSet<&str>, - ) { - let count = self.counts.entry(unit.rule).or_default(); - if !counted_as_judged(unit, judgments, count) { - return; - } - let (outcome, answers) = resolved(unit, judgments); - let outcome = capped(unit, judgments, few, outcome); - // Two tests that check one behavior with different inputs are a note - // on their own; three or more linked by such pairs are grouped into a - // consider below. Labeled by hand on just, express, gson and - // lobsters, lone pairs were mostly style, with few worth merging. - let grouping = outcome; - let outcome = match (&unit.detail, outcome) { - (Detail::TestPair { .. }, Outcome::Consider(p)) => Outcome::Note(p), - _ => outcome, - }; - let top = self.concern.entry(unit.rule).or_default(); - *top = top.max(outcome.concern()); - match strength_of(outcome) { - Some((strength, p)) => { - *match strength { - Strength::Review => &mut count.review, - Strength::Consider => &mut count.consider, - Strength::Note => &mut count.note, - } += 1; - if unit.rule == catalog::COMMENTS { - self.commented.push(( - unit, - strength, - p, - comment_reason(&answers, documented(unit)), - )); - } else { - self.findings - .push(finding(plan, unit, strength, p, &answers, judgments)); - } - } - None if outcome == Outcome::Clear => count.clear += 1, - None => { - count.uncertain += 1; - self.undecided - .entry(unit.rule) - .or_default() - .push(undecided_unit(unit, &answers)); - } - } - if let ( - Detail::TestPair { names, subject, .. }, - Outcome::Review(p) | Outcome::Consider(p), - ) = (&unit.detail, grouping) - { - // A review pair stays its own finding: it says a test adds nothing. - let finding = matches!(grouping, Outcome::Consider(_)).then(|| self.findings.len() - 1); - self.redundant.push(Redundant { - unit, - names, - subject, - p, - finding, - }); - } - } -} - -/// Counts a unit that is too small, needs context or was left unasked under -/// a finished plan, and returns false for it; otherwise counts it as judged. -fn counted_as_judged(unit: &UnitPlan, judgments: &[Judgment], count: &mut UnitCounts) -> bool { - match unit.presence { - Presence::TooSmall => count.too_small += 1, - Presence::NeedsContext => count.needs_context += 1, - // A check left unasked because its document is a finished plan. - Presence::Judged - if matches!(unit.detail, Detail::Stale { .. } | Detail::DocPair { .. }) - && answers(judgments, &unit.id, Pass::Trace).is_empty() => - { - count.covered += 1 - } - Presence::Judged => { - count.judged += 1; - return true; - } - } - false -} - -/// The finding strength and probability of an outcome that raises one. -fn strength_of(outcome: Outcome) -> Option<(Strength, f64)> { - match outcome { - Outcome::Review(p) => Some((Strength::Review, p)), - Outcome::Consider(p) => Some((Strength::Consider, p)), - Outcome::Note(p) => Some((Strength::Note, p)), - _ => None, - } -} - -/// Questions whose undecided answer leaves a unit of the rule undecided; the -/// other questions are weak signals or only matter when decisive. -fn deciding_questions(rule: &str) -> &'static [&'static str] { - match rule { - catalog::FUNCTION_SIMPLIFICATION => &["split", "flatten"], - catalog::FILE_ORGANIZATION => &["split"], - catalog::SHARED_LOGIC => &["same"], - catalog::HARDCODED_VALUES => &["environment", "magic", "special"], - catalog::COMMENTS => &["restates", "verbose", "history", "disabled"], - catalog::TEST_VALUE => &["own_logic", "mock_only"], - catalog::INJECTION => &[ - "interpreted", - "resource", - "origin", - "sql", - "shell", - "code", - "markup", - "path", - "url", - "type", - "redirect", - "deserialize", - "xxe", - ], - catalog::SENSITIVE_DATA => &[ - "logs_secret", - "error_details", - "logs_object_secret", - "exception_to_client", - "environment_to_client", - "handler_leaks", - ], - catalog::UNSAFE_SETTINGS => &[ - "weakened", - "tls", - "hash", - "random", - "cors", - "cookie", - "debug", - "token", - "key", - "csrf", - "literal_secret", - ], - catalog::ACCESS_CONTROL => &[ - "others", - "editable", - "search_path", - "unchecked", - "broad", - "data", - "exposed", - "rows", - "returns_others", - "reach", - "argument_rows", - "operator_only", - ], - catalog::WORKFLOWS => &["outside", "untrusted"], - catalog::LAWS => &["states"], - catalog::LARGE_DOCS => &["split", "history"], - catalog::DOC_STALENESS => &["plan", "relies"], - catalog::DOC_DUPLICATION => &["a_covers", "b_covers", "conflict"], - catalog::AGENT_CONTEXT => &[ - "inferable", - "describes", - "commands", - "generic", - "history", - "enforced", - ], - _ => &["overlap"], - } -} - -/// Candidate values are listed with an undecided unit only when this few, -/// so the entry names what the question was about without repeating the code. -const SHOWN_VALUES: usize = 3; - -/// The unit and its undecided questions; with no answers, why. -fn undecided_unit(unit: &UnitPlan, answers: &Answers<'_>) -> Undecided { - let get = |q: &str| answers.get(q).copied(); - let settled_values = (unit.rule == catalog::HARDCODED_VALUES) - .then(|| value_signals(&get, &unit.detail, true)) - .flatten(); - // Instruction sections and section pairs settle some signals by others. - let settled_sections = match unit.rule { - catalog::AGENT_CONTEXT => super::outcome::section_signals(&get), - catalog::DOC_DUPLICATION => super::outcome::pair_signals(&get), - _ => None, - }; - let mut questions: Vec = match (settled_values, settled_sections) { - (Some(signals), _) => signals - .iter() - .filter(|(_, o, _)| matches!(o, Outcome::Uncertain(_))) - .map(|(q, ..)| question_label(q).to_string()) - .collect(), - (None, Some(signals)) => signals - .iter() - .filter(|(_, o)| matches!(o, Outcome::Uncertain(_))) - .map(|(q, _)| question_label(q).to_string()) - .collect(), - (None, None) => undecided_questions(unit.rule, answers), - }; - if answers.is_empty() { - questions.push("no answer".into()); - } - let values = match &unit.detail { - Detail::Values { values, .. } | Detail::Constants { values, .. } - if values.len() <= SHOWN_VALUES => - { - values.clone() - } - _ => Vec::new(), - }; - Undecided { - unit: match &unit.detail { - Detail::Outline { .. } => "file outline".into(), - // Policies are often named by what they allow, the same on each table. - Detail::Access(Access::Policy { table }) => format!("{} on {table}", unit.name), - _ => unit.name.clone(), - }, - values, - line: unit.locations.first().map_or(1, |l| l.start_line), - questions, - } -} - -/// The deciding questions of a rule whose own answers stayed undecided. -fn undecided_questions(rule: &str, answers: &Answers<'_>) -> Vec { - deciding_questions(rule) - .iter() - .filter(|q| { - answers.get(*q).is_some_and(|a| { - let outcome = match a { - Answer::Noul { .. } => noul(a), - _ if **q == "origin" => origin_outcome(a), - _ if ["data", "rows", "reach"].contains(q) => { - super::outcome::acceptable_levels(a) - } - _ => score(a), - }; - matches!(outcome, Outcome::Uncertain(_)) - }) - }) - .map(|q| question_label(q).to_string()) - .collect() -} - -/// A rule's status is its most severe unit outcome. -fn dimension(rule: &str, count: UnitCounts, concern: f64, undecided: Vec) -> Dimension { - Dimension { - decision_basis: basis(rule, &count), - status: counted_status(&count), - concern_probability: concern, - rule_version: catalog::rule_version(rule).into(), - units: count, - undecided, - } -} - -pub(super) fn counted_status(count: &UnitCounts) -> Status { - if count.review > 0 { - Status::Review - } else if count.consider > 0 { - Status::Consider - } else if count.needs_context > 0 { - Status::NeedsContext - } else if count.uncertain > 0 { - Status::Uncertain - } else if count.note > 0 { - Status::Note - } else if count.clear > 0 { - Status::Clear - } else { - Status::NotApplicable - } -} - -pub(super) fn file_status( - dimensions: &BTreeMap, - findings: &[Finding], -) -> Status { - let any = |status: Status| dimensions.values().any(|d| d.status == status); - if dimensions - .values() - .all(|d| d.status == Status::NotApplicable) - { - Status::NotApplicable - } else if findings.iter().any(|f| f.strength == Strength::Review) { - Status::Review - } else if findings.iter().any(|f| f.strength == Strength::Consider) { - Status::Consider - } else if any(Status::NeedsContext) { - Status::NeedsContext - } else if any(Status::Uncertain) { - Status::Uncertain - } else if !findings.is_empty() { - Status::Note - } else { - Status::Clear - } -} - -fn basis(rule: &str, count: &UnitCounts) -> String { - let noun = match rule { - catalog::FILE_ORGANIZATION => "outline", - catalog::FUNCTION_SIMPLIFICATION => "function", - catalog::SHARED_LOGIC => "candidate pair", - catalog::TEST_VALUE => "test", - catalog::HARDCODED_VALUES => "value unit", - catalog::COMMENTS => "comment", - catalog::INJECTION | catalog::SENSITIVE_DATA | catalog::UNSAFE_SETTINGS => "security unit", - catalog::ACCESS_CONTROL => "access statement", - catalog::WORKFLOWS => "workflow job", - catalog::LAWS => "law", - catalog::AGENT_CONTEXT => "section", - catalog::LARGE_DOCS => "document", - catalog::DOC_STALENESS => "document check", - catalog::DOC_DUPLICATION => "section pair", - _ => "test pair", - }; - let plural = |n: usize| if n == 1 { "" } else { "s" }; - let mut parts = Vec::new(); - if count.judged == 0 && count.needs_context == 0 { - parts.push(format!("No {noun}s to judge.")); - } else { - let mut outcomes = Vec::new(); - for (n, label) in [ - (count.review, "review"), - (count.consider, "consider"), - (count.note, "note"), - (count.clear, "clear"), - (count.uncertain, "uncertain"), - ] { - if n > 0 { - outcomes.push(format!("{n} {label}")); - } - } - parts.push(format!( - "{} {noun}{} judged{}.", - count.judged, - plural(count.judged), - if outcomes.is_empty() { - String::new() - } else { - format!(": {}", outcomes.join(", ")) - } - )); - } - if count.needs_context > 0 { - parts.push(format!( - "{} {noun}{} exceed the request limit and were not sent.", - count.needs_context, - plural(count.needs_context) - )); - } - if count.too_small > 0 { - parts.push(format!( - "{} {noun}{} too small to judge.", - count.too_small, - plural(count.too_small) - )); - } - if count.covered > 0 { - parts.push(format!( - "{} check{} inside finished plans not asked.", - count.covered, - plural(count.covered) - )); - } - if count.omitted > 0 { - parts.push(format!( - "{} candidate{} omitted by caps.", - count.omitted, - plural(count.omitted) - )); - } - 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() -} - -fn finding( - plan: &FilePlan, - unit: &UnitPlan, - strength: Strength, - p: f64, - answers: &Answers<'_>, - judgments: &[Judgment], -) -> Finding { - let name = &unit.name; - let mut locations = unit.locations.clone(); - let mut symbol = Some(name.clone()); - let mut block = None; - let mut category = None; - let (message, action) = match &unit.detail { - Detail::Function { blocks, .. } => { - block = located_block(unit, blocks, judgments, "block") - .filter(|b| !most_of(&b.location, &unit.locations)); - let bend = crate::analysis::bend::file(&plan.path); - function_wording(name, strength, p, answers, (block, bend)) - } - Detail::Outline { - tests, - groups, - members, - .. - } => { - let chosen = outline_groups(answers.get("module").copied(), groups, *members); - symbol = chosen.first().map(|group| group.id.clone()); - if !chosen.is_empty() { - locations = chosen.iter().flat_map(|g| g.locations.clone()).collect(); - } - let several = several_kind(answers.get("split").copied(), answers.get("kind").copied()); - outline_wording(&chosen, *tests, several, strength, p) - } - Detail::Pair { - differences, - within_test, - in_tests, - in_cases, - } => pair_wording( - name, - differences, - (*within_test, *in_tests, *in_cases), - strength, - p, - ), - Detail::Values { .. } | Detail::Constants { .. } => { - let (wording, constant) = values_finding(unit, strength, p, answers, judgments); - if let Some(location) = constant { - symbol = location.symbol.clone(); - locations = vec![location]; - } - wording - } - Detail::Security { - sites, messages, .. - } => { - let (wording, site, named) = - security_finding(unit, (sites, messages), strength, p, answers); - block = site; - category = Some(named); - wording - } - Detail::Section { .. } => section_wording(name, &unit.detail, strength, p, answers), - Detail::Plan { facts } => { - symbol = None; - category = Some(super::grouping::FINISHED_PLAN.into()); - plan_wording(name, facts, p) - } - Detail::Stale { missing, .. } => stale_wording(name, missing, p), - Detail::DocPair { other, .. } => { - let (wording, conflict) = doc_pair_wording(name, other, answers, p); - if conflict { - category = Some("conflict".into()); - } - wording - } - Detail::Document { parts, .. } => { - symbol = None; - block = located_block(unit, parts, judgments, "part"); - document_wording(name, strength, p, answers, block) - } - Detail::Handler { registered } => { - category = Some("CWE-209 error details exposed".into()); - handler_wording(name, registered, strength, p) - } - Detail::Access(access @ (Access::Table | Access::View | Access::Reducer)) => { - let (wording, named) = module_wording(access, name, strength, p, answers); - category = Some(named); - wording - } - Detail::Access(access) => { - let subject = match access { - Access::Policy { table } => format!("Policy `{name}` on `{table}`"), - Access::Definer => format!("SECURITY DEFINER function `{name}`"), - _ => format!("A grant on `{name}`"), - }; - let (wording, named) = privilege_wording(&subject, strength, p, answers); - category = Some(named); - wording - } - Detail::Job { expressions } => { - let (wording, named) = job_wording(name, expressions, strength, p, answers); - category = Some(named); - wording - } - Detail::Comment { .. } => { - let reason = comment_reason(answers, documented(unit)); - comment_wording(name, &[(&unit.locations[0], reason)], strength, p) - } - Detail::Test { .. } => test_wording(name, strength, p, answers), - Detail::Law => law_wording(name, strength, p, answers), - Detail::TestPair { .. } => { - symbol = None; - test_pair_wording(name, strength == Strength::Review, p) - } - }; - let lines = locations - .iter() - .map(|l| l.end_line + 1 - l.start_line) - .sum::() - .max(unit.lines.min(1)); - // The located block comes first, so an agent acts on it; the function follows. - if let Some(block) = block { - locations.insert(0, block.location.clone()); - } - Finding { - rule: catalog::id(unit.rule).into(), - strength, - line: locations.first().map_or(1, |l| l.start_line), - message, - action: action.into(), - symbol, - rule_version: catalog::rule_version(unit.rule).into(), - concern_probability: p, - locations, - quote: unit.quote.clone(), - category, - // Only a special-cased identity groups across files: the same number - // or path can mean different things in different code. - values: located_value(unit, judgments) - .filter(|_| { - matches!( - answers.get("special").map(|a| noul(a)), - Some(Outcome::Review(_) | Outcome::Consider(_)) - ) - }) - .into_iter() - .collect(), - fingerprint: fingerprint(unit.rule, plan, &unit.identity), - rank: rank(p, lines), - baselined: false, - suppressed: None, - } -} - -/// A unit's outcome under the caps its rule and facts put on it: an -/// unnamed or single-use value, a value that only needs a name, security -/// code at a test path or resting on what lies outside the function, a -/// short outline or section, an outline naming no group, and comments too -/// few to act on. -fn capped( - unit: &UnitPlan, - judgments: &[Judgment], - few: &BTreeSet<&str>, - outcome: Outcome, -) -> Outcome { - if unnamed_value(unit, judgments) { - return lowered(lowered(outcome)); - } - if single_use_value(unit, judgments) - || readable_value(unit, judgments) - || same_everywhere(unit, judgments) - || short_outline(unit) - || sectioned_outline(unit) - || small_section(unit) - { - return at_most_note(outcome); - } - if named_value_only(unit, judgments) || bend_outline(unit) { - return at_most_consider(outcome); - } - let lower = test_path_security(unit) - || outside_function(unit, judgments) - || unnamed_outline(unit, judgments) - || few.contains(unit.id.as_str()); - if lower { lowered(outcome) } else { outcome } -} - -/// The kind follow-up of each hardcoded-value finding not yet asked one: -/// what the value is, for a consider that rests on a value's name whose -/// file writes the value again; where the value or constant would differ, -/// for a finding that rests on the environment. Both need the value or -/// constant the locate named. -pub fn unkinded_values( - plan: &FilePlan, - judgments: &[Judgment], -) -> Vec<(serde_json::Value, super::Asked)> { - plan.units - .iter() - .filter(|u| u.presence == Presence::Judged) - .filter_map(|u| { - let asked = answers(judgments, &u.id, Pass::Locate); - if asked.contains_key("value_kind") || asked.contains_key("environment_kind") { - return None; - } - match &u.detail { - Detail::Values { - locate: Some(locate), - repeated, - .. - } => { - let option = located_option(u, judgments, ("value", 'v'))?; - if named_value_only(u, judgments) && repeated.get(option) == Some(&true) { - super::hardcoded::value_kind(locate, option, &u.id) - } else if environment_only(u, judgments) { - super::hardcoded::environment_kind(locate, option, &u.id) - } else { - None - } - } - Detail::Constants { - locate: Some(locate), - .. - } if environment_only(u, judgments) => { - let option = located_constant(u, judgments)?; - super::hardcoded::environment_kind(locate, option, &u.id) - } - _ => None, - } - }) - .collect() -} - -/// Such a consider whose value, asked what it is, clearly reads for itself -/// where it is used: the field or argument it fills or a comment beside it -/// says what it is, or it is an idiom or a hand-tuned number, together at -/// the threshold of a clear located part. Its finding is a note; a value -/// with copies that must change together, or that nothing explains, stays -/// a consider. Leaning toward those kinds was not enough: at 0.50 they took -/// 30 of 46 right considers with 47 of 64 wrong ones. -fn readable_value(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - named_value_only(unit, judgments) - && choice_mass( - answers(judgments, &unit.id, Pass::Locate) - .get("value_kind") - .copied(), - &super::questions::READABLE_VALUES, - ) - .is_some_and(|p| { - crate::policy::probability_at_least(p, crate::policy::LOCATION_PROBABILITY) - }) -} - -/// A finding that rests on the environment whose value or constant, asked -/// where it would differ, needs no configuration at the review threshold: -/// the same in every copy of the program on purpose, a fallback used only -/// when configuration gives none, or code no deployment runs. Its finding -/// is a note. Labeled by hand, that took 17 of 36 wrong reviews and -/// considers and 2 of 17 right ones (a frontend's API host, edited in code -/// three times, and a template author's domain as a fallback); leaning at -/// 0.50 would have taken 25 wrong and 6 right. -fn same_everywhere(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - environment_only(unit, judgments) - && choice_mass( - answers(judgments, &unit.id, Pass::Locate) - .get("environment_kind") - .copied(), - &super::questions::SAME_EVERYWHERE, - ) - .is_some_and(|p| crate::policy::probability_at_least(p, crate::policy::REVIEW_PROBABILITY)) -} - -/// A function's hardcoded-value review or consider whose value was not -/// named: the locate Choice picked none clearly, or there were too many -/// values to offer. Its finding is a note, since a reader cannot tell what -/// to change: one level lower, 8 of lobsters' 10 such considers were wrong. -fn unnamed_value(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - matches!(unit.detail, Detail::Values { .. }) - && located_value(unit, judgments).is_none() - && matches!( - resolved(unit, judgments).0, - Outcome::Review(_) | Outcome::Consider(_) - ) -} - -/// A hardcoded-value review or consider that rests only on whether a value -/// needs a name. Naming a value is a cleanup, so it is at most a consider: -/// labeled by hand, 17 such reviews were right and 18 wrong, most of the -/// wrong ones tuning in game, audio and animation code (a scheduler's -/// 500 ms, a hash seed, a mix gain, a float epsilon). -fn named_value_only(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - matches!(unit.detail, Detail::Values { .. }) && rests_only_on(unit, judgments, "magic") -} - -/// A hardcoded-value review or consider that rests only on whether a value -/// changes between environments: a file's constants always do. -fn environment_only(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - rests_only_on(unit, judgments, "environment") -} - -/// A hardcoded-value review or consider whose raised questions are all -/// `question`. -fn rests_only_on(unit: &UnitPlan, judgments: &[Judgment], question: &str) -> bool { - if !matches!( - unit.detail, - Detail::Values { .. } | Detail::Constants { .. } - ) { - return false; - } - let (outcome, answers) = resolved(unit, judgments); - if !matches!(outcome, Outcome::Review(_) | Outcome::Consider(_)) { - return false; - } - let get = |q: &str| answers.get(q).copied(); - crate::units::outcome::value_signals(&get, &unit.detail, true) - .unwrap_or_default() - .iter() - .filter(|(_, o, _)| matches!(o, Outcome::Review(_) | Outcome::Consider(_))) - .all(|(raised, ..)| *raised == question) -} - -/// Such a finding about a value its file writes once is a note: labeled by -/// hand on 35 projects, those considers were right 19 times in 52, against -/// 34 in 49 for a value its file repeats. A delay given to `setTimeout`, a -/// size given to an attribute or a CSS class reads where it is used; a value -/// written twice can drift apart. -fn single_use_value(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - let Detail::Values { repeated, .. } = &unit.detail else { - return false; - }; - named_value_only(unit, judgments) - && located_option(unit, judgments, ("value", 'v')) - .is_some_and(|i| repeated.get(i) == Some(&false)) -} - -/// Weak-setting checks whose review needs its settle Choice to name what -/// the function itself does, and the option that does: whether a token was -/// verified before the function reads it, or whether a callee or model hook -/// hashes the password it saves, lies outside the function. -const SHOWN_IN_FUNCTION: [(&str, &str, &str); 2] = [ - ("token", "token_use", "turned_off"), - ("hash", "password_handling", "fast_hash"), -]; - -/// An unsafe-settings review named only by checks of `SHOWN_IN_FUNCTION` -/// whose Choice does not name what the function itself does. Labeled by -/// hand, reviews that decoded a token to decide access were right in -/// intentionally vulnerable apps and wrong in three others (a SpacetimeDB -/// module whose host verifies tokens, a SvelteKit hook whose API verifies -/// them, an identity provider's token read over TLS), and reviews for -/// passwords saved as plain text were wrong where a service or an entity's -/// `@BeforeInsert` hook hashed them; turning `verify_signature` off and -/// hashing with MD5 in the function were right. It is one level lower. -fn outside_function(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - if unit.rule != catalog::UNSAFE_SETTINGS { - return false; - } - let (outcome, answers) = resolved(unit, judgments); - if !matches!(outcome, Outcome::Review(_)) { - return false; - } - let get = |q: &str| answers.get(q).copied(); - let named: Vec<&str> = settled_checks(unit.rule, &get) - .into_iter() - .filter(|(_, o)| matches!(o, Outcome::Review(_))) - .map(|(id, _)| id) - .collect(); - let shown = |check: &str| { - SHOWN_IN_FUNCTION - .iter() - .find(|(id, ..)| *id == check) - .is_none_or(|(_, question, option)| { - matches!( - choice(get(question)), - Some((chosen, p)) if chosen == *option - && crate::policy::probability_at_least(p, crate::policy::REVIEW_PROBABILITY) - ) - }) - }; - !named.is_empty() && !named.iter().any(|check| shown(check)) -} - -/// Instruction sections of fewer tokens than this cost a session too little -/// to be worth a consider. -const SECTION_NOTE_TOKENS: usize = 15; - -/// An instruction section of fewer than 15 tokens is a note: labeled by -/// hand, 1 of 10 findings on such sections was right, most of them a title -/// and a "Last updated" line read as a record of past work, against 64 of -/// 68 on larger ones. -fn small_section(unit: &UnitPlan) -> bool { - matches!(unit.detail, Detail::Section { tokens, .. } if tokens < SECTION_NOTE_TOKENS) -} - -/// A security unit of a file at a test path, judged as application code -/// because it holds no tests, such as a test app's settings or a model only -/// tests use: like code that runs only in development, it is one level -/// lower. The dummy apps of devise and clearance and a test model hashing -/// with `password.reverse` were three wrong reviews, the only security -/// reviews or considers at test paths across 103 projects. -fn test_path_security(unit: &UnitPlan) -> bool { - matches!( - unit.detail, - Detail::Security { - test_path: true, - .. - } - ) -} - -/// Why a hardcoded-value finding is below the level its answers reached, -/// with that level. -fn lowered_value(unit: &UnitPlan, judgments: &[Judgment]) -> Option<(Strength, &'static str)> { - let why = if unnamed_value(unit, judgments) { - "No single value stood out, so it is a note." - } else if single_use_value(unit, judgments) { - "It is written once in its file, so it is a note." - } else if readable_value(unit, judgments) { - "It reads for itself where it is used, so it is a note." - } else if same_everywhere(unit, judgments) { - "It likely stays the same wherever the program runs, or is only a fallback, so it is a note." - } else if named_value_only(unit, judgments) - && matches!(resolved(unit, judgments).0, Outcome::Review(_)) - { - "" - } else { - return None; - }; - strength_of(resolved(unit, judgments).0).map(|(s, _)| (s, why)) -} - -/// A review lowered to a consider; other outcomes as they are. -fn at_most_consider(outcome: Outcome) -> Outcome { - match outcome { - Outcome::Review(p) => Outcome::Consider(p), - other => other, - } -} - -/// A file-organization consider that says only that some members could -/// move, naming no group: the module Choice was not asked (one group or -/// none) or spread wider than two groups, and no kind of file decided it. -/// Its finding is a note, since a reader cannot tell which members to move. -/// A review, or a consider the kind decided, says to split the whole file. -fn unnamed_outline(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - let Detail::Outline { - groups, members, .. - } = &unit.detail - else { - return false; - }; - let (outcome, answers) = resolved(unit, judgments); - let get = |q: &str| answers.get(q).copied(); - matches!(outcome, Outcome::Consider(_)) - && several_kind(get("split"), get("kind")).is_none() - && outline_groups(get("module"), groups, *members).is_empty() -} - -/// Files shorter than this many lines read easily whole. -const OUTLINE_NOTE_LINES: usize = 250; - -/// A file-organization finding on a file of fewer than 250 lines is a note: -/// of 32 such findings labeled by hand on 25 projects, 3 were right, while -/// splitting a 138-line module or a 175-line test helper file would only -/// scatter it; 21 of 29 on longer files were right. -fn short_outline(unit: &UnitPlan) -> bool { - matches!(unit.detail, Detail::Outline { .. }) && unit.lines < OUTLINE_NOTE_LINES -} - -/// A split of a Bend 2 file is at most a consider: a language that writes -/// each match arm, binding and effect on a line of its own runs to long -/// files, and on 64 Bend 2 projects 14 of 43 file-organization reviews were -/// right, 8 of 13 on the 41 its floor and sections were tuned on and 6 of -/// 30 on 23 it had never seen. -fn bend_outline(unit: &UnitPlan) -> bool { - matches!(unit.detail, Detail::Outline { .. }) - && unit - .locations - .first() - .is_some_and(|l| crate::analysis::bend::file(&l.path)) -} - -/// Section rules that show a Bend 2 file laid out in titled parts. -const SECTIONS: usize = 2; - -/// A file-organization finding on a Bend 2 file its author laid out in -/// titled sections is a note: the groups proposed from its calls rarely -/// follow those sections, and on 41 Bend 2 projects 7 of 43 such findings -/// were right, against 10 of 17 on files without them. -fn sectioned_outline(unit: &UnitPlan) -> bool { - matches!(unit.detail, Detail::Outline { sections, .. } if sections >= SECTIONS) -} - -/// The group a module Choice picks clearly, or else the two it leans toward -/// when together they reach the location probability: flask's `cli.py` -/// split 0.45 and 0.23 over two of six groups. None when it spreads wider. -/// A group holding three quarters or more of the outline's `members` is -/// left out: moving 14 of a file's 15 members, or 9 of its 11 tests, moves -/// the file rather than splitting it. -fn outline_groups<'g>( - module: Option<&Answer>, - groups: &'g [super::GroupInfo], - members: usize, -) -> Vec<&'g super::GroupInfo> { - let mut chosen = chosen_groups(module, groups); - chosen.retain(|g| !three_quarters(g.names.len(), members)); - chosen -} - -fn chosen_groups<'g>( - module: Option<&Answer>, - groups: &'g [super::GroupInfo], -) -> Vec<&'g super::GroupInfo> { - let find = |id: &str| groups.iter().find(|g| g.id == id); - if let Some((id, _)) = choice(module) { - return find(id).into_iter().collect(); - } - let Some(Answer::Choice { probabilities, .. }) = module else { - return Vec::new(); - }; - let mass: f64 = probabilities.values().sum::().max(f64::MIN_POSITIVE); - let mut ranked: Vec<(&str, f64)> = probabilities - .iter() - .filter(|(id, _)| id.as_str() != "none") - .map(|(id, p)| (id.as_str(), p / mass)) - .collect(); - ranked.sort_by(|a, b| b.1.total_cmp(&a.1)); - match ranked.as_slice() { - [first, second, ..] - if crate::policy::probability_at_least( - first.1 + second.1, - crate::policy::LOCATION_PROBABILITY, - ) => - { - [first.0, second.0].into_iter().filter_map(find).collect() - } - _ => Vec::new(), - } -} - -/// A hardcoded-value finding's wording, naming the value or constant the -/// locate Choice named, and the location of that constant. -fn values_finding( - unit: &UnitPlan, - strength: Strength, - p: f64, - answers: &Answers<'_>, - judgments: &[Judgment], -) -> (Wording, Option) { - let lowered = lowered_value(unit, judgments); - let (message, action) = values_wording( - &unit.name, - &unit.detail, - (strength, lowered.map(|(reached, _)| reached)), - p, - answers, - ); - let why = lowered - .filter(|(_, why)| !why.is_empty()) - .map_or(String::new(), |(_, why)| format!(" {why}")); - if let Some(index) = located_constant(unit, judgments) { - // The finding points at the constant the Choice named. - let location = unit.locations[index].clone(); - let constant = location.symbol.as_deref().unwrap_or(""); - let message = format!("{message} The constant is `{constant}`.{why}"); - return ((message, action), Some(location)); - } - let wording = match located_value(unit, judgments) { - Some(value) => (format!("{message} The value is {value}.{why}"), action), - None => (format!("{message}{why}"), action), - }; - (wording, None) -} - -/// A security finding's wording, the site the Choice named and its -/// category; error details quote the message that carries another error's -/// text. -fn security_finding<'a>( - unit: &UnitPlan, - (sites, messages): (&'a [Block], &[String]), - strength: Strength, - p: f64, - answers: &Answers<'_>, -) -> (Wording, Option<&'a Block>, String) { - let site = - choice(answers.get("site").copied()).and_then(|(id, _)| sites.iter().find(|s| s.id == id)); - let ((message, action), named) = security_wording(unit.rule, &unit.name, strength, p, answers); - // The error message the Choice found carrying another error's text. - let carried = choice(answers.get("messages").copied()) - .and_then(|(id, _)| messages.get(id.strip_prefix('m')?.parse::().ok()?)) - .filter(|_| named.starts_with("CWE-209") && strength != Strength::Note); - let wording = match carried { - Some(text) => (format!("{message} The message is {text}."), action), - None => (message, action), - }; - (wording, site, named) -} - -/// A workflow job's wording and category, listing the expressions of its -/// scripts when outsiders can write them. -fn job_wording( - name: &str, - expressions: &[String], - strength: Strength, - p: f64, - answers: &Answers<'_>, -) -> (Wording, String) { - let ((message, action), named) = - privilege_wording(&format!("Job `{name}`"), strength, p, answers); - let outside = matches!( - answers.get("outside").map(|a| noul(a)), - Some(Outcome::Review(_)) - ); - if !outside { - return ((message, action), named); - } - let listed = expressions - .iter() - .map(|e| format!("`${{{{ {e} }}}}`")) - .collect::>() - .join(", "); - let message = format!("{message} Expressions in its scripts: {listed}."); - ((message, action), named) -} - -/// The position of the constant the locate Choice names, when it is clear. -fn located_constant(unit: &UnitPlan, judgments: &[Judgment]) -> Option { - if !matches!(unit.detail, Detail::Constants { .. }) { - return None; - } - located_option(unit, judgments, ("constant", 'c')).filter(|&i| i < unit.locations.len()) -} - -/// The value a hardcoded-value finding is about, when the locate choice is clear. -fn located_value(unit: &UnitPlan, judgments: &[Judgment]) -> Option { - let Detail::Values { choices, .. } = &unit.detail else { - return None; - }; - choices - .get(located_option(unit, judgments, ("value", 'v'))?) - .cloned() -} - -/// The index of the option `{prefix}N` a clear locate Choice `question` names. -fn located_option( - unit: &UnitPlan, - judgments: &[Judgment], - (question, prefix): (&str, char), -) -> Option { - let located = answers(judgments, &unit.id, Pass::Locate); - let (id, _) = choice(located.get(question).copied())?; - id.strip_prefix(prefix)?.parse().ok() -} - -/// Whether a block spans most of its function, three quarters or more: -/// naming it as the part to extract says no more than the finding does, as -/// lines 206–390 of just's 198-line `Justfile::run` did. -fn most_of(block: &crate::schema::Location, function: &[crate::schema::Location]) -> bool { - let lines = |l: &crate::schema::Location| l.end_line + 1 - l.start_line; - function - .first() - .is_some_and(|f| three_quarters(lines(block), lines(f))) -} - -/// Whether `part` is three quarters or more of `whole`. -fn three_quarters(part: usize, whole: usize) -> bool { - part * 4 >= whole * 3 -} - -/// The block chosen by the locate follow-up `question`, when its choice is clear. -fn located_block<'a>( - unit: &UnitPlan, - blocks: &'a [Block], - judgments: &[Judgment], - question: &str, -) -> Option<&'a Block> { - let located = answers(judgments, &unit.id, Pass::Locate); - let (id, _) = choice(located.get(question).copied())?; - blocks.iter().find(|b| b.id == id) -} - -/// Tests linked by overlapping pairs on one subject, with where they are, -/// the lowest probability of their pairs, and their pairs' findings. -struct Cluster<'a> { - subject: &'a String, - tests: BTreeSet<&'a String>, - locations: Vec, - p: f64, - findings: Vec>, -} - -/// Three or more tests linked by overlapping pairs on one subject: the tests -/// a chain of such pairs connects. Two pairs of one subject that share no -/// test stay two pairs; grouped by subject alone, sinatra's pair of redirect -/// tests and pair of deny tests of `get` read as four overlapping tests. -/// Also returns the indices of the pair findings each group reports. -fn over_tested(plan: &FilePlan, redundant: &[Redundant<'_>]) -> (Vec, BTreeSet) { - let clusters = clusters(redundant); - let grouped = clusters - .iter() - .flat_map(|c| c.findings.iter().flatten().copied()) - .collect(); - let groups = clusters - .into_iter() - .map(|cluster| group_finding(plan, cluster)) - .collect(); - (groups, grouped) -} - -/// The clusters of three or more tests that overlapping pairs connect. -fn clusters<'a>(redundant: &'a [Redundant<'_>]) -> Vec> { - let mut clusters: Vec> = Vec::new(); - for pair in redundant { - let mut joined = Cluster { - subject: pair.subject, - tests: BTreeSet::new(), - locations: Vec::new(), - p: pair.p, - findings: vec![pair.finding], - }; - let mut index = 0; - while index < clusters.len() { - let other = &clusters[index]; - if other.subject == pair.subject && pair.names.iter().any(|n| other.tests.contains(n)) { - let other = clusters.remove(index); - joined.tests.extend(other.tests); - joined.locations.extend(other.locations); - joined.p = joined.p.min(other.p); - joined.findings.extend(other.findings); - } else { - index += 1; - } - } - for (name, location) in pair.names.iter().zip(&pair.unit.locations) { - if joined.tests.insert(name) { - joined.locations.push(location.clone()); - } - } - clusters.push(joined); - } - clusters.sort_by(|a, b| (a.subject, &a.tests).cmp(&(b.subject, &b.tests))); - clusters.retain(|c| c.tests.len() >= 3); - clusters -} - -/// The consider that names a cluster's tests. -fn group_finding(plan: &FilePlan, cluster: Cluster<'_>) -> Finding { - let Cluster { - subject, - tests, - mut locations, - p, - .. - } = cluster; - locations.sort(); - let names: Vec = tests.iter().map(|t| format!("`{t}`")).collect(); - let lines = locations - .iter() - .map(|l| l.end_line + 1 - l.start_line) - .sum(); - let identity: Vec<&str> = std::iter::once(subject.as_str()) - .chain(tests.iter().map(|t| t.as_str())) - .collect(); - Finding { - rule: catalog::id(catalog::TEST_REDUNDANCY).into(), - strength: Strength::Consider, - line: locations.first().map_or(1, |l| l.start_line), - message: format!( - "{} tests of `{subject}` overlap: {} ({p:.2}).", - tests.len(), - names.join(", ") - ), - action: "Consider one parameterized test for these cases".into(), - symbol: Some(subject.clone()), - rule_version: catalog::rule_version(catalog::TEST_REDUNDANCY).into(), - concern_probability: p, - locations, - quote: None, - category: None, - values: Vec::new(), - fingerprint: fingerprint(catalog::TEST_REDUNDANCY, plan, &super::identity(&identity)), - rank: rank(p, lines), - baselined: false, - suppressed: None, - } -} - -fn documented(unit: &UnitPlan) -> bool { - matches!( - unit.detail, - Detail::Comment { - documentation: true, - .. - } - ) -} - -/// Lines a unit's comments may span in all and still be few: a comment or -/// two a reader skips in a moment cost little. -const FEW_COMMENT_LINES: usize = 3; - -/// Comments raised to a consider whose unit's considered comments span -/// fewer than `FEW_COMMENT_LINES` lines in all: their finding is a note. -fn few_comment_lines<'a>(plan: &'a FilePlan, judgments: &[Judgment]) -> BTreeSet<&'a str> { - let mut considered = BTreeMap::<&str, Vec<&UnitPlan>>::new(); - for unit in &plan.units { - if let Detail::Comment { owner, .. } = &unit.detail - && unit.presence == Presence::Judged - && matches!(resolved(unit, judgments).0, Outcome::Consider(_)) - { - considered.entry(owner.as_str()).or_default().push(unit); - } - } - considered - .into_values() - .filter(|units| units.iter().map(|u| u.lines).sum::() < FEW_COMMENT_LINES) - .flatten() - .map(|u| u.id.as_str()) - .collect() -} - -/// One finding per unit and strength for its comments a reader could do -/// without, listing each with what makes it so, at the lowest probability -/// among them. -fn comment_findings( - plan: &FilePlan, - commented: &[(&UnitPlan, Strength, f64, &'static str)], -) -> Vec { - let mut grouped = - BTreeMap::<(&str, Strength), Vec<&(&UnitPlan, Strength, f64, &'static str)>>::new(); - for entry in commented { - let Detail::Comment { owner, .. } = &entry.0.detail else { - continue; - }; - grouped - .entry((owner.as_str(), entry.1)) - .or_default() - .push(entry); - } - grouped - .into_iter() - .map(|((owner, strength), mut entries)| { - entries.sort_by_key(|(unit, ..)| unit.locations[0].start_line); - let p = entries.iter().map(|(_, _, p, _)| *p).fold(1.0, f64::min); - let listed: Vec<(&crate::schema::Location, &'static str)> = entries - .iter() - .map(|(unit, _, _, reason)| (&unit.locations[0], *reason)) - .collect(); - let (message, action) = comment_wording(owner, &listed, strength, p); - let locations: Vec = - listed.iter().map(|(l, _)| (*l).clone()).collect(); - let lines = entries.iter().map(|(unit, ..)| unit.lines).sum(); - let identities: Vec<&str> = std::iter::once(owner) - .chain(entries.iter().map(|(unit, ..)| unit.identity.as_str())) - .collect(); - Finding { - rule: catalog::id(catalog::COMMENTS).into(), - strength, - line: locations[0].start_line, - message, - action: action.into(), - symbol: (owner != super::comments::TOP_LEVEL).then(|| owner.to_string()), - rule_version: catalog::rule_version(catalog::COMMENTS).into(), - concern_probability: p, - locations, - quote: entries[0].0.quote.clone(), - category: None, - values: Vec::new(), - fingerprint: fingerprint(catalog::COMMENTS, plan, &super::identity(&identities)), - rank: rank(p, lines), - baselined: false, - suppressed: None, - } - }) - .collect() -} diff --git a/src/units/compose/answers.rs b/src/units/compose/answers.rs new file mode 100644 index 0000000..a770166 --- /dev/null +++ b/src/units/compose/answers.rs @@ -0,0 +1,187 @@ +//! The answers a unit's outcome rests on: its first pass with the rechecks, +//! traces, settles and locates its rule reads beside or in place of them. +use super::*; + +pub(super) fn answers<'a>(judgments: &'a [Judgment], unit: &str, pass: Pass) -> Answers<'a> { + judgments + .iter() + .filter(|j| j.unit == unit && j.pass == pass) + .map(|j| (j.question.as_str(), &j.answer)) + .collect() +} + +pub(super) fn security(rule: &str) -> bool { + catalog::SECURITY.contains(&rule) +} + +/// A security unit's first-pass and trace answers, with each recheck answer +/// (the origin or a check, seen with callers) in place of the traced one, +/// unless the traced answer is decisive and the recheck is not: an undecided +/// traced answer is replaced even by an undecided recheck, whose lean saw +/// more evidence. +pub(super) fn security_answers<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> Answers<'a> { + let mut merged = answers(judgments, &unit.id, Pass::First); + merged.extend(answers(judgments, &unit.id, Pass::Trace)); + let judged = |question: &str, answer: &Answer| match question { + "origin" => origin_outcome(answer), + _ => noul(answer), + }; + for (question, answer) in answers(judgments, &unit.id, Pass::Recheck) { + let traced = merged.get(question).map(|a| judged(question, a)); + if judged(question, answer).decisive() || !traced.is_some_and(Outcome::decisive) { + merged.insert(question, answer); + } + } + // The settle answers sit beside the checks they settle, under their own + // names, and so does what an injection consider's values can hold. + merged.extend(answers(judgments, &unit.id, Pass::Settle)); + merged.extend(answers(judgments, &unit.id, Pass::Locate)); + merged +} + +/// A unit's outcome and the answers it rests on: its first-pass answers with +/// the follow-ups its rule reads beside or in place of them. +pub(super) fn resolved<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> (Outcome, Answers<'a>) { + let merged = if security(unit.rule) { + security_answers(unit, judgments) + } else if let Some(pass) = beside(unit.rule) { + beside_answers(unit, judgments, pass) + } else if unit.rule == catalog::COMMENTS { + comment_answers(unit, judgments) + } else if unit.rule == catalog::TEST_VALUE { + let merged = test_value_answers(unit, judgments); + let outcome = leaning_test(unit, judgments, unit_outcome(unit, &merged)); + return (outcome, merged); + } else if unit.rule == catalog::FILE_ORGANIZATION { + // Each candidate part's answers, asked of a long file once its + // outline raised no finding, sit beside the answers it rests on. + let (_, mut merged) = rechecked(unit, judgments); + merged.extend(answers(judgments, &unit.id, Pass::Locate)); + merged + } else if unit.rule == catalog::TEST_REDUNDANCY { + // Whether each test checks something the other does not, asked of a + // pair that reached a review, sits beside its answers. + let (_, mut merged) = rechecked(unit, judgments); + merged.extend(answers(judgments, &unit.id, Pass::Locate)); + merged + } else { + return rechecked(unit, judgments); + }; + (unit_outcome(unit, &merged), merged) +} + +/// A test whose hollow checks stay undecided once its recheck is asked (or +/// when it has none) leans: below 0.50 it is clear. Labeled from the code, +/// 4 of 43 such tests below 0.50 checked only their mocks or recomputed +/// their expected value (5 counting a test whose one real check is weak), +/// against 10 of 35 at 0.50 or more; 636 of the 792 undecided tests on the +/// corpus lean below. +pub(super) fn leaning_test(unit: &UnitPlan, judgments: &[Judgment], outcome: Outcome) -> Outcome { + let rechecked = + unit.recheck.is_none() || !answers(judgments, &unit.id, Pass::Recheck).is_empty(); + match outcome { + Outcome::Uncertain(p) + if rechecked + && !crate::policy::probability_at_least(p, crate::policy::LEADING_PROBABILITY) => + { + Outcome::Clear + } + other => other, + } +} + +/// The pass of the follow-ups whose questions sit beside the first answers +/// under their own ids: document section and pair checks, the kind of a +/// large document, and benign-kind value checks. +pub(super) fn beside(rule: &str) -> Option { + if [ + catalog::DOC_STALENESS, + catalog::DOC_DUPLICATION, + catalog::LARGE_DOCS, + ] + .contains(&rule) + { + Some(Pass::Trace) + } else if [ + catalog::HARDCODED_VALUES, + catalog::AGENT_CONTEXT, + catalog::WORKFLOWS, + ] + .contains(&rule) + { + Some(Pass::Recheck) + } else { + None + } +} + +pub(super) fn beside_answers<'a>( + unit: &UnitPlan, + judgments: &'a [Judgment], + pass: Pass, +) -> Answers<'a> { + let mut merged = answers(judgments, &unit.id, Pass::First); + merged.extend(answers(judgments, &unit.id, pass)); + // How a pair's sections relate, or what a section treats its missing + // names as, asked when its checks stay undecided. + merged.extend(answers(judgments, &unit.id, Pass::Settle)); + merged +} + +/// A comment's recheck replaces its first answers when the first stayed open +/// and the recheck decides, or neither decides; the kind of comment, asked +/// when it stays undecided, sits beside them. +pub(super) fn comment_answers<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> Answers<'a> { + let first = answers(judgments, &unit.id, Pass::First); + let before = unit_outcome(unit, &first); + let recheck = answers(judgments, &unit.id, Pass::Recheck); + let mut merged = if !recheck.is_empty() + && open(unit, &first, before) + && (unit_outcome(unit, &recheck).decisive() || !before.decisive()) + { + recheck + } else { + first + }; + merged.extend(answers(judgments, &unit.id, Pass::Settle)); + merged +} + +/// A test recheck asks the hollow-test questions again with the code under +/// test and the setup; each answer replaces the first one unless only the +/// first is decisive. +pub(super) fn test_value_answers<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> Answers<'a> { + let mut merged = answers(judgments, &unit.id, Pass::First); + for (question, answer) in answers(judgments, &unit.id, Pass::Recheck) { + let first = merged.get(question).map(|a| noul(a)); + if noul(answer).decisive() || !first.is_some_and(Outcome::decisive) { + merged.insert(question, answer); + } + } + // What its assertions read, asked after an internal-details consider. + merged.extend(answers(judgments, &unit.id, Pass::Locate)); + merged +} + +/// The first-pass outcome, or the recheck's when the first called for one +/// and the recheck decides. +pub(super) fn rechecked<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> (Outcome, Answers<'a>) { + let first = answers(judgments, &unit.id, Pass::First); + let outcome = unit_outcome(unit, &first); + let mut recheck = answers(judgments, &unit.id, Pass::Recheck); + if unit.rule == catalog::FILE_ORGANIZATION { + // The kind is asked apart from the recheck and read beside its split, + // or beside the first split of a file too long for a recheck. + if unit.recheck.is_none() { + recheck.extend(first.iter().map(|(q, a)| (*q, *a))); + } + recheck.extend(answers(judgments, &unit.id, Pass::Trace)); + } + if open(unit, &first, outcome) && !recheck.is_empty() { + let second = unit_outcome(unit, &recheck); + if second.decisive() { + return (second, recheck); + } + } + (outcome, first) +} diff --git a/src/units/compose/caps.rs b/src/units/compose/caps.rs new file mode 100644 index 0000000..7bef381 --- /dev/null +++ b/src/units/compose/caps.rs @@ -0,0 +1,303 @@ +//! Measured limits on outcomes: the levels a unit's finding may reach once +//! its answers are composed, each set from findings labeled on the corpus. +use super::*; + +/// A unit's outcome under the caps its rule and facts put on it: an +/// unnamed or single-use value, a value that only needs a name, security +/// code at a test path or resting on what lies outside the function, a +/// short outline or section, an outline naming no group, and comments too +/// few to act on. +pub(super) fn capped( + unit: &UnitPlan, + judgments: &[Judgment], + few: &BTreeSet<&str>, + outcome: Outcome, +) -> Outcome { + if unnamed_value(unit, judgments) { + return lowered(lowered(outcome)); + } + if single_use_value(unit, judgments) + || readable_value(unit, judgments) + || same_everywhere(unit, judgments) + || short_outline(unit) + || sectioned_outline(unit) + || small_section(unit) + { + return at_most_note(outcome); + } + if named_value_only(unit, judgments) || bend_outline(unit) { + return at_most_consider(outcome); + } + let lower = test_path_security(unit) + || outside_function(unit, judgments) + || unnamed_outline(unit, judgments) + || few.contains(unit.id.as_str()); + if lower { lowered(outcome) } else { outcome } +} + +/// Such a consider whose value, asked what it is, clearly reads for itself +/// where it is used: the field or argument it fills or a comment beside it +/// says what it is, or it is an idiom or a hand-tuned number, together at +/// the threshold of a clear located part. Its finding is a note; a value +/// with copies that must change together, or that nothing explains, stays +/// a consider. Leaning toward those kinds was not enough: at 0.50 they took +/// 30 of 46 right considers with 47 of 64 wrong ones. +pub(super) fn readable_value(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + named_value_only(unit, judgments) + && choice_mass( + answers(judgments, &unit.id, Pass::Locate) + .get("value_kind") + .copied(), + &crate::units::questions::READABLE_VALUES, + ) + .is_some_and(|p| { + crate::policy::probability_at_least(p, crate::policy::LOCATION_PROBABILITY) + }) +} + +/// A finding that rests on the environment whose value or constant, asked +/// where it would differ, needs no configuration at the review threshold: +/// the same in every copy of the program on purpose, a fallback used only +/// when configuration gives none, or code no deployment runs. Its finding +/// is a note. Labeled by hand, that took 17 of 36 wrong reviews and +/// considers and 2 of 17 right ones (a frontend's API host, edited in code +/// three times, and a template author's domain as a fallback); leaning at +/// 0.50 would have taken 25 wrong and 6 right. +pub(super) fn same_everywhere(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + environment_only(unit, judgments) + && choice_mass( + answers(judgments, &unit.id, Pass::Locate) + .get("environment_kind") + .copied(), + &crate::units::questions::SAME_EVERYWHERE, + ) + .is_some_and(|p| crate::policy::probability_at_least(p, crate::policy::REVIEW_PROBABILITY)) +} + +/// A function's hardcoded-value review or consider whose value was not +/// named: the locate Choice picked none clearly, or there were too many +/// values to offer. Its finding is a note, since a reader cannot tell what +/// to change: one level lower, 8 of lobsters' 10 such considers were wrong. +pub(super) fn unnamed_value(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + matches!(unit.detail, Detail::Values { .. }) + && located_value(unit, judgments).is_none() + && matches!( + resolved(unit, judgments).0, + Outcome::Review(_) | Outcome::Consider(_) + ) +} + +/// A hardcoded-value review or consider that rests only on whether a value +/// needs a name. Naming a value is a cleanup, so it is at most a consider: +/// labeled by hand, 17 such reviews were right and 18 wrong, most of the +/// wrong ones tuning in game, audio and animation code (a scheduler's +/// 500 ms, a hash seed, a mix gain, a float epsilon). +pub(super) fn named_value_only(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + matches!(unit.detail, Detail::Values { .. }) && rests_only_on(unit, judgments, "magic") +} + +/// A hardcoded-value review or consider that rests only on whether a value +/// changes between environments: a file's constants always do. +pub(super) fn environment_only(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + rests_only_on(unit, judgments, "environment") +} + +/// A hardcoded-value review or consider whose raised questions are all +/// `question`. +pub(super) fn rests_only_on(unit: &UnitPlan, judgments: &[Judgment], question: &str) -> bool { + if !matches!( + unit.detail, + Detail::Values { .. } | Detail::Constants { .. } + ) { + return false; + } + let (outcome, answers) = resolved(unit, judgments); + if !matches!(outcome, Outcome::Review(_) | Outcome::Consider(_)) { + return false; + } + let get = |q: &str| answers.get(q).copied(); + crate::units::outcome::value_signals(&get, &unit.detail, true) + .unwrap_or_default() + .iter() + .filter(|(_, o, _)| matches!(o, Outcome::Review(_) | Outcome::Consider(_))) + .all(|(raised, ..)| *raised == question) +} + +/// Such a finding about a value its file writes once is a note: labeled by +/// hand on 35 projects, those considers were right 19 times in 52, against +/// 34 in 49 for a value its file repeats. A delay given to `setTimeout`, a +/// size given to an attribute or a CSS class reads where it is used; a value +/// written twice can drift apart. +pub(super) fn single_use_value(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + let Detail::Values { repeated, .. } = &unit.detail else { + return false; + }; + named_value_only(unit, judgments) + && located_option(unit, judgments, ("value", 'v')) + .is_some_and(|i| repeated.get(i) == Some(&false)) +} + +/// Weak-setting checks whose review needs its settle Choice to name what +/// the function itself does, and the option that does: whether a token was +/// verified before the function reads it, or whether a callee or model hook +/// hashes the password it saves, lies outside the function. +pub(super) const SHOWN_IN_FUNCTION: [(&str, &str, &str); 2] = [ + ("token", "token_use", "turned_off"), + ("hash", "password_handling", "fast_hash"), +]; + +/// An unsafe-settings review named only by checks of `SHOWN_IN_FUNCTION` +/// whose Choice does not name what the function itself does. Labeled by +/// hand, reviews that decoded a token to decide access were right in +/// intentionally vulnerable apps and wrong in three others (a SpacetimeDB +/// module whose host verifies tokens, a SvelteKit hook whose API verifies +/// them, an identity provider's token read over TLS), and reviews for +/// passwords saved as plain text were wrong where a service or an entity's +/// `@BeforeInsert` hook hashed them; turning `verify_signature` off and +/// hashing with MD5 in the function were right. It is one level lower. +pub(super) fn outside_function(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + if unit.rule != catalog::UNSAFE_SETTINGS { + return false; + } + let (outcome, answers) = resolved(unit, judgments); + if !matches!(outcome, Outcome::Review(_)) { + return false; + } + let get = |q: &str| answers.get(q).copied(); + let named: Vec<&str> = settled_checks(unit.rule, &get) + .into_iter() + .filter(|(_, o)| matches!(o, Outcome::Review(_))) + .map(|(id, _)| id) + .collect(); + let shown = |check: &str| { + SHOWN_IN_FUNCTION + .iter() + .find(|(id, ..)| *id == check) + .is_none_or(|(_, question, option)| { + matches!( + choice(get(question)), + Some((chosen, p)) if chosen == *option + && crate::policy::probability_at_least(p, crate::policy::REVIEW_PROBABILITY) + ) + }) + }; + !named.is_empty() && !named.iter().any(|check| shown(check)) +} + +/// Instruction sections of fewer tokens than this cost a session too little +/// to be worth a consider. +pub(super) const SECTION_NOTE_TOKENS: usize = 15; + +/// An instruction section of fewer than 15 tokens is a note: labeled by +/// hand, 1 of 10 findings on such sections was right, most of them a title +/// and a "Last updated" line read as a record of past work, against 64 of +/// 68 on larger ones. +pub(super) fn small_section(unit: &UnitPlan) -> bool { + matches!(unit.detail, Detail::Section { tokens, .. } if tokens < SECTION_NOTE_TOKENS) +} + +/// A security unit of a file at a test path, judged as application code +/// because it holds no tests, such as a test app's settings or a model only +/// tests use: like code that runs only in development, it is one level +/// lower. The dummy apps of devise and clearance and a test model hashing +/// with `password.reverse` were three wrong reviews, the only security +/// reviews or considers at test paths across 103 projects. +pub(super) fn test_path_security(unit: &UnitPlan) -> bool { + matches!( + unit.detail, + Detail::Security { + test_path: true, + .. + } + ) +} + +/// Why a hardcoded-value finding is below the level its answers reached, +/// with that level. +pub(super) fn lowered_value( + unit: &UnitPlan, + judgments: &[Judgment], +) -> Option<(Strength, &'static str)> { + let why = if unnamed_value(unit, judgments) { + "No single value stood out, so it is a note." + } else if single_use_value(unit, judgments) { + "It is written once in its file, so it is a note." + } else if readable_value(unit, judgments) { + "It reads for itself where it is used, so it is a note." + } else if same_everywhere(unit, judgments) { + "It likely stays the same wherever the program runs, or is only a fallback, so it is a note." + } else if named_value_only(unit, judgments) + && matches!(resolved(unit, judgments).0, Outcome::Review(_)) + { + "" + } else { + return None; + }; + strength_of(resolved(unit, judgments).0).map(|(s, _)| (s, why)) +} + +/// A review lowered to a consider; other outcomes as they are. +pub(super) fn at_most_consider(outcome: Outcome) -> Outcome { + match outcome { + Outcome::Review(p) => Outcome::Consider(p), + other => other, + } +} + +/// A file-organization consider that says only that some members could +/// move, naming no group: the module Choice was not asked (one group or +/// none) or spread wider than two groups, and no kind of file decided it. +/// Its finding is a note, since a reader cannot tell which members to move. +/// A review, or a consider the kind decided, says to split the whole file. +pub(super) fn unnamed_outline(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + let Detail::Outline { + groups, + members, + parts, + .. + } = &unit.detail + else { + return false; + }; + let (outcome, answers) = resolved(unit, judgments); + let get = |q: &str| answers.get(q).copied(); + matches!(outcome, Outcome::Consider(_)) + && several_kind(get("split"), get("kind")).is_none() + && outline_groups(get("module"), groups, *members).is_empty() + && deciding_part(&answers, parts).is_none() +} + +/// Files shorter than this many lines read easily whole. +pub(super) const OUTLINE_NOTE_LINES: usize = 250; + +/// A file-organization finding on a file of fewer than 250 lines is a note: +/// of 32 such findings labeled by hand on 25 projects, 3 were right, while +/// splitting a 138-line module or a 175-line test helper file would only +/// scatter it; 21 of 29 on longer files were right. +pub(super) fn short_outline(unit: &UnitPlan) -> bool { + matches!(unit.detail, Detail::Outline { .. }) && unit.lines < OUTLINE_NOTE_LINES +} + +/// A split of a Bend 2 file is at most a consider: a language that writes +/// each match arm, binding and effect on a line of its own runs to long +/// files, and on 64 Bend 2 projects 14 of 43 file-organization reviews were +/// right, 8 of 13 on the 41 its floor and sections were tuned on and 6 of +/// 30 on 23 it had never seen. +pub(super) fn bend_outline(unit: &UnitPlan) -> bool { + matches!(unit.detail, Detail::Outline { .. }) + && unit + .locations + .first() + .is_some_and(|l| crate::analysis::bend::file(&l.path)) +} + +/// Section rules that show a Bend 2 file laid out in titled parts. +pub(super) const SECTIONS: usize = 2; + +/// A file-organization finding on a Bend 2 file its author laid out in +/// titled sections is a note: the groups proposed from its calls rarely +/// follow those sections, and on 41 Bend 2 projects 7 of 43 such findings +/// were right, against 10 of 17 on files without them. +pub(super) fn sectioned_outline(unit: &UnitPlan) -> bool { + matches!(unit.detail, Detail::Outline { sections, .. } if sections >= SECTIONS) +} diff --git a/src/units/compose/comments.rs b/src/units/compose/comments.rs new file mode 100644 index 0000000..96fec33 --- /dev/null +++ b/src/units/compose/comments.rs @@ -0,0 +1,99 @@ +//! Comment findings: a unit's comments to clean up, reported together. +use super::*; + +pub(super) fn documented(unit: &UnitPlan) -> bool { + matches!( + unit.detail, + Detail::Comment { + documentation: true, + .. + } + ) +} + +/// Lines a unit's comments may span in all and still be few: a comment or +/// two a reader skips in a moment cost little. +pub(super) const FEW_COMMENT_LINES: usize = 3; + +/// Comments raised to a consider whose unit's considered comments span +/// fewer than `FEW_COMMENT_LINES` lines in all: their finding is a note. +pub(super) fn few_comment_lines<'a>( + plan: &'a FilePlan, + judgments: &[Judgment], +) -> BTreeSet<&'a str> { + let mut considered = BTreeMap::<&str, Vec<&UnitPlan>>::new(); + for unit in &plan.units { + if let Detail::Comment { owner, .. } = &unit.detail + && unit.presence == Presence::Judged + && matches!(resolved(unit, judgments).0, Outcome::Consider(_)) + { + considered.entry(owner.as_str()).or_default().push(unit); + } + } + considered + .into_values() + .filter(|units| units.iter().map(|u| u.lines).sum::() < FEW_COMMENT_LINES) + .flatten() + .map(|u| u.id.as_str()) + .collect() +} + +/// One finding per unit and strength for its comments a reader could do +/// without, listing each with what makes it so, at the lowest probability +/// among them. +pub(super) fn comment_findings( + plan: &FilePlan, + commented: &[(&UnitPlan, Strength, f64, &'static str)], +) -> Vec { + let mut grouped = + BTreeMap::<(&str, Strength), Vec<&(&UnitPlan, Strength, f64, &'static str)>>::new(); + for entry in commented { + let Detail::Comment { owner, .. } = &entry.0.detail else { + continue; + }; + grouped + .entry((owner.as_str(), entry.1)) + .or_default() + .push(entry); + } + grouped + .into_iter() + .map(|((owner, strength), mut entries)| { + entries.sort_by_key(|(unit, ..)| unit.locations[0].start_line); + let p = entries.iter().map(|(_, _, p, _)| *p).fold(1.0, f64::min); + let listed: Vec<(&crate::schema::Location, &'static str)> = entries + .iter() + .map(|(unit, _, _, reason)| (&unit.locations[0], *reason)) + .collect(); + let (message, action) = comment_wording(owner, &listed, strength, p); + let locations: Vec = + listed.iter().map(|(l, _)| (*l).clone()).collect(); + let lines = entries.iter().map(|(unit, ..)| unit.lines).sum(); + let identities: Vec<&str> = std::iter::once(owner) + .chain(entries.iter().map(|(unit, ..)| unit.identity.as_str())) + .collect(); + Finding { + rule: catalog::id(catalog::COMMENTS).into(), + strength, + line: locations[0].start_line, + message, + action: action.into(), + symbol: (owner != crate::units::comments::TOP_LEVEL).then(|| owner.to_string()), + rule_version: catalog::rule_version(catalog::COMMENTS).into(), + concern_probability: p, + locations, + quote: entries[0].0.quote.clone(), + category: None, + values: Vec::new(), + fingerprint: fingerprint( + catalog::COMMENTS, + plan, + &crate::units::identity(&identities), + ), + rank: rank(p, lines), + baselined: false, + suppressed: None, + } + }) + .collect() +} diff --git a/src/units/compose/due.rs b/src/units/compose/due.rs new file mode 100644 index 0000000..d735bec --- /dev/null +++ b/src/units/compose/due.rs @@ -0,0 +1,352 @@ +//! Which follow-ups a file's recorded answers call for: the units each +//! follow-up stage asks next, in the order the stages run. +use super::*; + +/// The settle Choices a security unit calls for, not yet asked: those whose +/// checks stay undecided after the trace and recheck while the unit is +/// uncertain, and for a consider or note resting on an undecided check, +/// where its text goes (the finding claims it likely reaches a client) and +/// where code that requests a URL runs, and every Choice for an injection +/// note in Django code; and those asked whenever their checks are not +/// clear, such as what a PHP page joins into HTML. +pub fn unsettled(unit: &UnitPlan, judgments: &[Judgment]) -> BTreeSet<&'static str> { + use crate::units::security::{SETTLES, SettleWhen}; + if unit.presence != Presence::Judged + || !security(unit.rule) + || answers(judgments, &unit.id, Pass::Trace).is_empty() + { + return BTreeSet::new(); + } + let merged = security_answers(unit, judgments); + // Nearly every Django view places request values somewhere, so an + // injection note that no check found ("values from another party …, + // but no check found one placed unhandled") rests on its undecided + // checks, such as a redirect to its own path with an id in it. + let django_note = unit.rule == catalog::INJECTION + && matches!(unit.detail, Detail::Security { django: true, .. }); + let open = |when: SettleWhen| match unit_outcome(unit, &merged) { + Outcome::Uncertain(_) => true, + Outcome::Note(_) if django_note => true, + Outcome::Consider(_) | Outcome::Note(_) => when == SettleWhen::UndecidedOrFinding, + _ => false, + }; + let undecided = |q: &str| { + merged + .get(q) + .is_some_and(|a| matches!(noul(a), Outcome::Uncertain(_))) + }; + let not_clear = |q: &str| merged.get(q).is_some_and(|a| noul(a) != Outcome::Clear); + SETTLES + .iter() + .filter(|kind| kind.rule == unit.rule && !merged.contains_key(kind.question)) + .filter(|kind| match kind.when { + SettleWhen::NotClear => kind.checks.iter().any(|q| not_clear(q)), + when => open(when) && kind.checks.iter().any(|q| undecided(q)), + }) + .map(|kind| kind.question) + .collect() +} + +/// Security units whose finding a confirm Choice of their own follows, not +/// yet asked: an injection finding whose one concern is a path (what its +/// paths can hold), or markup or a redirect unless its values are asked +/// already (what they hold, where they lead), and a sensitive-data finding +/// its log checks raised (when the log line runs). +pub fn unconfirmed_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { + plan.units + .iter() + .filter(|u| u.presence == Presence::Judged) + .filter(|u| answers(judgments, &u.id, Pass::Locate).is_empty()) + .filter(|u| { + let Detail::Security { + checked, logging, .. + } = &u.detail + else { + return false; + }; + let (outcome, resolved) = resolved(u, judgments); + let get = |q: &str| resolved.get(q).copied(); + let kind = confirmable(&get); + matches!(outcome, Outcome::Review(_) | Outcome::Consider(_)) + && (checked.is_some() + && (kind == Some("path") || kind.is_some() && !values_due(outcome, &resolved)) + || logging.is_some() && logs_found(&get)) + }) + .map(|u| u.id.clone()) + .collect() +} + +/// Whether an injection outcome calls for what its values can hold: a +/// consider that rests on the function's parameters, its origin not +/// another party. +pub(super) fn values_due(outcome: Outcome, resolved: &Answers<'_>) -> bool { + matches!(outcome, Outcome::Consider(_)) + && !resolved + .get("origin") + .is_some_and(|a| matches!(origin_outcome(a), Outcome::Review(_))) +} + +/// Functions whose split question raised a review or consider, and whose +/// block has not been located yet; hardcoded-value functions raised to a +/// review or consider whose value has not been named yet; and the other +/// units whose confirm `locate_due` calls for. +pub fn unlocated_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { + plan.units + .iter() + .filter(|u| u.presence == Presence::Judged) + .filter(|u| answers(judgments, &u.id, Pass::Locate).is_empty()) + .filter(|u| locate_due(u, judgments)) + .map(|u| u.id.clone()) + .collect() +} + +/// Whether a unit's outcome calls for its locate or confirm follow-up. +pub(super) fn locate_due(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + let (outcome, resolved) = resolved(unit, judgments); + let raised = |o: Option| matches!(o, Some(Outcome::Review(_) | Outcome::Consider(_))); + match &unit.detail { + Detail::Values { + locate: Some(_), .. + } + | Detail::Constants { + locate: Some(_), .. + } => raised(Some(outcome)), + Detail::TestPair { + confirm: Some(_), .. + } => matches!(outcome, Outcome::Review(_)), + // Only the internal-details check raises a test's consider. + Detail::Test { confirm: Some(_) } => matches!(outcome, Outcome::Consider(_)), + // An injection consider rests on the function's parameters unless + // its origin was another party; one that rests on a path is asked + // what its paths can hold instead. + Detail::Security { + confirm: Some(_), .. + } => { + values_due(outcome, &resolved) + && confirmable(&|q| resolved.get(q).copied()) != Some("path") + } + Detail::Function { + locate: Some(_), .. + } => raised(resolved.get("split").map(|a| benefit(a))), + Detail::Document { + locate: Some(_), .. + } => raised( + resolved + .get("split") + .map(|a| document_split(a, resolved.get("kind").copied())), + ), + _ => false, + } +} + +/// Judged units whose first pass stayed undecided, or became a note from a +/// torn function or file-organization answer, and that have no recheck yet; +/// for injection, units whose traced origin stayed unclear or was the +/// function's parameters, so callers can settle it. +pub fn uncertain_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { + plan.units + .iter() + .filter(|u| u.presence == Presence::Judged) + .filter(|u| answers(judgments, &u.id, Pass::Recheck).is_empty()) + .filter(|u| { + if security(u.rule) { + origin_unsettled(u, judgments) + } else if u.rule == catalog::HARDCODED_VALUES { + value_undecided(u, judgments) + } else { + let first = answers(judgments, &u.id, Pass::First); + open(u, &first, unit_outcome(u, &first)) + } + }) + .map(|u| u.id.clone()) + .collect() +} + +/// An injection unit whose checks are not all clear while its traced origin +/// stayed unclear or was the function's parameters. +pub(super) fn origin_unsettled(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + let merged = security_answers(unit, judgments); + let get = |q: &str| merged.get(q).copied(); + unit.rule == catalog::INJECTION + && !checks(unit.rule, &get).iter().all(|o| *o == Outcome::Clear) + && matches!( + merged.get("origin").map(|a| origin_outcome(a)), + Some(Outcome::Uncertain(_) | Outcome::Consider(_)) + ) +} + +/// A hardcoded-value unit with a question its first pass left undecided. +pub(super) fn value_undecided(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + let first = answers(judgments, &unit.id, Pass::First); + let get = |q: &str| first.get(q).copied(); + value_signals(&get, &unit.detail, false).is_some_and(|signals| { + signals + .iter() + .any(|(_, o, _)| matches!(o, Outcome::Uncertain(_))) + }) +} + +/// Outlines whose recheck left the split Score undecided, or whose first +/// answer did when the file is too long for a recheck, and large documents +/// whose split Score stayed undecided, whose kind has not been asked yet; +/// section pairs and stale sections whose checks stayed undecided and whose +/// settle has not been asked yet. +pub fn unkinded_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { + plan.units + .iter() + .filter(|u| u.presence == Presence::Judged) + .filter(|u| match u.detail { + Detail::DocPair { .. } | Detail::Stale { .. } => unsettled_check(u, judgments), + Detail::Comment { .. } => unsettled_comment(u, judgments), + _ => unkinded_split(u, judgments), + }) + .map(|u| u.id.clone()) + .collect() +} + +/// An outline or large document not yet asked its kind whose split Score +/// stayed undecided: the recheck's for an outline that has one, else the +/// first. A large document's split finding is asked its kind as well, since +/// its Score reads headings alone, and so is an outline's finding its +/// recheck raised from an undecided first answer. +pub(super) fn unkinded_split(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + if ![catalog::FILE_ORGANIZATION, catalog::LARGE_DOCS].contains(&unit.rule) + || !answers(judgments, &unit.id, Pass::Trace).is_empty() + { + return false; + } + let pass = if unit.recheck.is_some() && unit.rule != catalog::LARGE_DOCS { + Pass::Recheck + } else { + Pass::First + }; + let document = unit.rule == catalog::LARGE_DOCS; + answers(judgments, &unit.id, pass) + .get("split") + .is_some_and(|a| match benefit(a) { + Outcome::Uncertain(_) => true, + Outcome::Consider(_) | Outcome::Review(_) => document || pass == Pass::Recheck, + _ => false, + }) +} + +/// Outlines of long files left without a finding whose candidate parts are +/// not yet asked; the kind of file, when due, is asked first. +pub fn unparted_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { + plan.units + .iter() + .filter(|u| { + u.presence == Presence::Judged + && matches!(&u.detail, Detail::Outline { parts, .. } if !parts.is_empty()) + && answers(judgments, &u.id, Pass::Locate).is_empty() + && !unkinded_split(u, judgments) + && matches!( + resolved(u, judgments).0, + Outcome::Clear | Outcome::Note(_) | Outcome::Uncertain(_) + ) + }) + .map(|u| u.id.clone()) + .collect() +} + +/// A section pair or stale section whose checks were asked, stayed +/// undecided, and whose settle has not been asked. +pub(super) fn unsettled_check(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + !answers(judgments, &unit.id, Pass::Trace).is_empty() + && answers(judgments, &unit.id, Pass::Settle).is_empty() + && matches!(resolved(unit, judgments).0, Outcome::Uncertain(_)) +} + +/// A comment still undecided after its recheck, or without one, whose kind +/// has not been asked. +pub(super) fn unsettled_comment(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + answers(judgments, &unit.id, Pass::Settle).is_empty() + && (unit.recheck.is_none() || !answers(judgments, &unit.id, Pass::Recheck).is_empty()) + && matches!(resolved(unit, judgments).0, Outcome::Uncertain(_)) +} + +/// Documents whose plan question found a plan whose work Git shows finished. +pub fn finished_plans( + plan: &crate::units::Plan, + files: &[crate::schema::FileResult], +) -> BTreeSet { + plan.files + .iter() + .filter(|(owner, file_plan)| { + file_plan.units.iter().any(|u| { + matches!(u.detail, Detail::Plan { .. }) + && matches!( + resolved(u, &files[**owner].judgments).0, + Outcome::Consider(_) | Outcome::Review(_) + ) + }) + }) + .map(|(_, file_plan)| file_plan.path.clone()) + .collect() +} + +/// Security units whose presence is not clear, to trace. +pub fn untraced_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { + plan.units + .iter() + .filter(|u| u.presence == Presence::Judged && security(u.rule)) + .filter(|u| answers(judgments, &u.id, Pass::Trace).is_empty()) + .filter(|u| { + let first = answers(judgments, &u.id, Pass::First); + let presence: Vec = crate::units::security::presence_questions(u.rule) + .iter() + .filter_map(|q| first.get(q).map(|a| noul(a))) + .collect(); + if presence.is_empty() { + return false; + } + presence.iter().any(|o| *o != Outcome::Clear) + }) + .map(|u| u.id.clone()) + .collect() +} + +/// The kind follow-up of each hardcoded-value finding not yet asked one: +/// what the value is, for a consider that rests on a value's name whose +/// file writes the value again; where the value or constant would differ, +/// for a finding that rests on the environment. Both need the value or +/// constant the locate named. +pub fn unkinded_values( + plan: &FilePlan, + judgments: &[Judgment], +) -> Vec<(serde_json::Value, crate::units::Asked)> { + plan.units + .iter() + .filter(|u| u.presence == Presence::Judged) + .filter_map(|u| { + let asked = answers(judgments, &u.id, Pass::Locate); + if asked.contains_key("value_kind") || asked.contains_key("environment_kind") { + return None; + } + match &u.detail { + Detail::Values { + locate: Some(locate), + repeated, + .. + } => { + let option = located_option(u, judgments, ("value", 'v'))?; + if named_value_only(u, judgments) && repeated.get(option) == Some(&true) { + crate::units::hardcoded::value_kind(locate, option, &u.id) + } else if environment_only(u, judgments) { + crate::units::hardcoded::environment_kind(locate, option, &u.id) + } else { + None + } + } + Detail::Constants { + locate: Some(locate), + .. + } if environment_only(u, judgments) => { + let option = located_constant(u, judgments)?; + crate::units::hardcoded::environment_kind(locate, option, &u.id) + } + _ => None, + } + }) + .collect() +} diff --git a/src/units/compose/located.rs b/src/units/compose/located.rs new file mode 100644 index 0000000..6e0a5c8 --- /dev/null +++ b/src/units/compose/located.rs @@ -0,0 +1,211 @@ +//! Where a finding points and what it names: the block, value, constant, +//! group or part its locate answers chose. +use super::*; + +/// The candidate part that raised an outline's finding: the outline's split +/// and kind raised none, and the part does a job of its own. +pub(super) fn deciding_part<'p>( + answers: &Answers<'_>, + parts: &'p [crate::units::Part], +) -> Option<&'p crate::units::Part> { + let get = |q: &str| answers.get(q).copied(); + let split = organization_outcome(get("split"), get("kind"), &[]); + if matches!(split, Some(Outcome::Review(_) | Outcome::Consider(_))) { + return None; + } + // A part too long to ask is left out of `parts`, so find it by its ids. + let (position, _) = separable_part(&part_answers(&get))?; + let ids = crate::units::outline::PART_QUESTIONS[position]; + parts.iter().find(|part| part.questions == ids) +} + +/// The group a module Choice picks clearly, or else the two it leans toward +/// when together they reach the location probability: flask's `cli.py` +/// split 0.45 and 0.23 over two of six groups. None when it spreads wider. +/// A group holding three quarters or more of the outline's `members` is +/// left out: moving 14 of a file's 15 members, or 9 of its 11 tests, moves +/// the file rather than splitting it. +pub(super) fn outline_groups<'g>( + module: Option<&Answer>, + groups: &'g [crate::units::GroupInfo], + members: usize, +) -> Vec<&'g crate::units::GroupInfo> { + let mut chosen = chosen_groups(module, groups); + chosen.retain(|g| !three_quarters(g.names.len(), members)); + chosen +} + +pub(super) fn chosen_groups<'g>( + module: Option<&Answer>, + groups: &'g [crate::units::GroupInfo], +) -> Vec<&'g crate::units::GroupInfo> { + let find = |id: &str| groups.iter().find(|g| g.id == id); + if let Some((id, _)) = choice(module) { + return find(id).into_iter().collect(); + } + let Some(Answer::Choice { probabilities, .. }) = module else { + return Vec::new(); + }; + let mass: f64 = probabilities.values().sum::().max(f64::MIN_POSITIVE); + let mut ranked: Vec<(&str, f64)> = probabilities + .iter() + .filter(|(id, _)| id.as_str() != "none") + .map(|(id, p)| (id.as_str(), p / mass)) + .collect(); + ranked.sort_by(|a, b| b.1.total_cmp(&a.1)); + match ranked.as_slice() { + [first, second, ..] + if crate::policy::probability_at_least( + first.1 + second.1, + crate::policy::LOCATION_PROBABILITY, + ) => + { + [first.0, second.0].into_iter().filter_map(find).collect() + } + _ => Vec::new(), + } +} + +/// A hardcoded-value finding's wording, naming the value or constant the +/// locate Choice named, and the location of that constant. +pub(super) fn values_finding( + unit: &UnitPlan, + strength: Strength, + p: f64, + answers: &Answers<'_>, + judgments: &[Judgment], +) -> (Wording, Option) { + let lowered = lowered_value(unit, judgments); + let (message, action) = values_wording( + &unit.name, + &unit.detail, + (strength, lowered.map(|(reached, _)| reached)), + p, + answers, + ); + let why = lowered + .filter(|(_, why)| !why.is_empty()) + .map_or(String::new(), |(_, why)| format!(" {why}")); + if let Some(index) = located_constant(unit, judgments) { + // The finding points at the constant the Choice named. + let location = unit.locations[index].clone(); + let constant = location.symbol.as_deref().unwrap_or(""); + let message = format!("{message} The constant is `{constant}`.{why}"); + return ((message, action), Some(location)); + } + let wording = match located_value(unit, judgments) { + Some(value) => (format!("{message} The value is {value}.{why}"), action), + None => (format!("{message}{why}"), action), + }; + (wording, None) +} + +/// A security finding's wording, the site the Choice named and its +/// category; error details quote the message that carries another error's +/// text. +pub(super) fn security_finding<'a>( + unit: &UnitPlan, + (sites, messages): (&'a [Block], &[String]), + strength: Strength, + p: f64, + answers: &Answers<'_>, +) -> (Wording, Option<&'a Block>, String) { + let site = + choice(answers.get("site").copied()).and_then(|(id, _)| sites.iter().find(|s| s.id == id)); + let ((message, action), named) = security_wording(unit.rule, &unit.name, strength, p, answers); + // The error message the Choice found carrying another error's text. + let carried = choice(answers.get("messages").copied()) + .and_then(|(id, _)| messages.get(id.strip_prefix('m')?.parse::().ok()?)) + .filter(|_| named.starts_with("CWE-209") && strength != Strength::Note); + let wording = match carried { + Some(text) => (format!("{message} The message is {text}."), action), + None => (message, action), + }; + (wording, site, named) +} + +/// A workflow job's wording and category, listing the expressions of its +/// scripts when outsiders can write them. +pub(super) fn job_wording( + name: &str, + expressions: &[String], + strength: Strength, + p: f64, + answers: &Answers<'_>, +) -> (Wording, String) { + let ((message, action), named) = + privilege_wording(&format!("Job `{name}`"), strength, p, answers); + let outside = matches!( + answers.get("outside").map(|a| noul(a)), + Some(Outcome::Review(_)) + ); + if !outside { + return ((message, action), named); + } + let listed = expressions + .iter() + .map(|e| format!("`${{{{ {e} }}}}`")) + .collect::>() + .join(", "); + let message = format!("{message} Expressions in its scripts: {listed}."); + ((message, action), named) +} + +/// The position of the constant the locate Choice names, when it is clear. +pub(super) fn located_constant(unit: &UnitPlan, judgments: &[Judgment]) -> Option { + if !matches!(unit.detail, Detail::Constants { .. }) { + return None; + } + located_option(unit, judgments, ("constant", 'c')).filter(|&i| i < unit.locations.len()) +} + +/// The value a hardcoded-value finding is about, when the locate choice is clear. +pub(super) fn located_value(unit: &UnitPlan, judgments: &[Judgment]) -> Option { + let Detail::Values { choices, .. } = &unit.detail else { + return None; + }; + choices + .get(located_option(unit, judgments, ("value", 'v'))?) + .cloned() +} + +/// The index of the option `{prefix}N` a clear locate Choice `question` names. +pub(super) fn located_option( + unit: &UnitPlan, + judgments: &[Judgment], + (question, prefix): (&str, char), +) -> Option { + let located = answers(judgments, &unit.id, Pass::Locate); + let (id, _) = choice(located.get(question).copied())?; + id.strip_prefix(prefix)?.parse().ok() +} + +/// Whether a block spans most of its function, three quarters or more: +/// naming it as the part to extract says no more than the finding does, as +/// lines 206–390 of just's 198-line `Justfile::run` did. +pub(super) fn most_of( + block: &crate::schema::Location, + function: &[crate::schema::Location], +) -> bool { + let lines = |l: &crate::schema::Location| l.end_line + 1 - l.start_line; + function + .first() + .is_some_and(|f| three_quarters(lines(block), lines(f))) +} + +/// Whether `part` is three quarters or more of `whole`. +pub(super) fn three_quarters(part: usize, whole: usize) -> bool { + part * 4 >= whole * 3 +} + +/// The block chosen by the locate follow-up `question`, when its choice is clear. +pub(super) fn located_block<'a>( + unit: &UnitPlan, + blocks: &'a [Block], + judgments: &[Judgment], + question: &str, +) -> Option<&'a Block> { + let located = answers(judgments, &unit.id, Pass::Locate); + let (id, _) = choice(located.get(question).copied())?; + blocks.iter().find(|b| b.id == id) +} diff --git a/src/units/compose/mod.rs b/src/units/compose/mod.rs new file mode 100644 index 0000000..14cf863 --- /dev/null +++ b/src/units/compose/mod.rs @@ -0,0 +1,682 @@ +//! Pure composition from typed judgments to unit outcomes, rule dimensions, +//! findings and a file status; each unit's outcome comes from `outcome`. +//! `answers` gathers what an outcome rests on, `due` picks the units each +//! follow-up stage asks next, `caps` holds the measured limits on a +//! finding's level, `located` where a finding points, and `redundant` and +//! `comments` report tests and comments together. +use super::{ + Access, Block, Detail, FilePlan, Presence, UnitPlan, + outcome::{ + Answers, Outcome, at_most_note, benefit, checks, choice, choice_mass, confirmable, + document_split, logs_found, lowered, noul, open, organization_outcome, origin_outcome, + part_answers, score, separable_part, settled_checks, several_kind, unit_outcome, + value_signals, + }, + wording::{Wording, comment_reason, comment_wording}, + wording::{ + doc_pair_wording, document_wording, function_wording, handler_wording, law_wording, + module_wording, outline_wording, pair_wording, part_wording, plan_wording, + privilege_wording, question_label, section_wording, security_wording, stale_wording, + test_pair_wording, test_wording, values_wording, + }, +}; +use crate::{ + catalog, + schema::{ + Answer, Dimension, Finding, Judgment, Location, Pass, Status, Strength, Undecided, + UnitCounts, hash, + }, +}; +use std::collections::{BTreeMap, BTreeSet}; + +mod answers; +mod caps; +mod comments; +mod due; +mod located; +mod redundant; +use answers::*; +use caps::*; +use comments::*; +pub use due::{ + finished_plans, uncertain_units, unconfirmed_units, unkinded_units, unkinded_values, + unlocated_units, unparted_units, unsettled, untraced_units, +}; +use located::*; +use redundant::*; + +pub struct Composed { + pub dimensions: BTreeMap, + pub findings: Vec, + pub status: Status, +} + +pub fn compose(plan: &FilePlan, judgments: &[Judgment]) -> Composed { + let few = few_comment_lines(plan, judgments); + let mut tally = Tally::default(); + for (rule, omitted) in &plan.rules { + tally.counts.entry(rule).or_default().omitted = *omitted; + } + for unit in &plan.units { + tally.add(plan, unit, judgments, &few); + } + let Tally { + mut counts, + concern, + mut findings, + redundant, + commented, + mut undecided, + } = tally; + // A pair of tests in a group of three or more is reported by the group. + let (groups, grouped) = over_tested(plan, &redundant); + // A pair in a group reaches the group's consider; a lone pair is a note. + if let Some(count) = counts.get_mut(catalog::TEST_REDUNDANCY) { + count.note -= grouped.len(); + count.consider += grouped.len(); + } + let mut index = 0; + findings.retain(|_| { + index += 1; + !grouped.contains(&(index - 1)) + }); + findings.extend(groups); + drop_copies_of_redundant_tests(&mut findings); + findings.extend(comment_findings(plan, &commented)); + let dimensions = plan + .rules + .keys() + .map(|rule| { + let count = counts.remove(rule).unwrap_or_default(); + let concern = concern.get(rule).copied().unwrap_or(0.0); + let undecided = undecided.remove(rule).unwrap_or_default(); + (rule.to_string(), dimension(rule, count, concern, undecided)) + }) + .collect(); + // Strongest first, so a note never sits above a review or consider. + findings.sort_by(|a, b| b.strength.cmp(&a.strength).then(b.rank.total_cmp(&a.rank))); + let status = file_status(&dimensions, &findings); + Composed { + dimensions, + findings, + status, + } +} + +/// What a file's units add up to, per rule, before comment findings and +/// redundant tests are grouped. +#[derive(Default)] +struct Tally<'p> { + counts: BTreeMap<&'p str, UnitCounts>, + concern: BTreeMap<&'p str, f64>, + findings: Vec, + redundant: Vec>, + commented: Vec<(&'p UnitPlan, Strength, f64, &'static str)>, + undecided: BTreeMap<&'p str, Vec>, +} + +impl<'p> Tally<'p> { + /// Counts one unit under its rule and keeps what it contributes: a + /// finding, a comment to group, a redundant test pair or an undecided unit. + fn add( + &mut self, + plan: &FilePlan, + unit: &'p UnitPlan, + judgments: &[Judgment], + few: &BTreeSet<&str>, + ) { + let count = self.counts.entry(unit.rule).or_default(); + if !counted_as_judged(unit, judgments, count) { + return; + } + let (outcome, answers) = resolved(unit, judgments); + let outcome = capped(unit, judgments, few, outcome); + // Two tests that check one behavior with different inputs are a note + // on their own; three or more linked by such pairs are grouped into a + // consider below. Labeled by hand on just, express, gson and + // lobsters, lone pairs were mostly style, with few worth merging. + let grouping = outcome; + let outcome = match (&unit.detail, outcome) { + (Detail::TestPair { .. }, Outcome::Consider(p)) => Outcome::Note(p), + _ => outcome, + }; + let top = self.concern.entry(unit.rule).or_default(); + *top = top.max(outcome.concern()); + match strength_of(outcome) { + Some((strength, p)) => { + *match strength { + Strength::Review => &mut count.review, + Strength::Consider => &mut count.consider, + Strength::Note => &mut count.note, + } += 1; + if unit.rule == catalog::COMMENTS { + self.commented.push(( + unit, + strength, + p, + comment_reason(&answers, documented(unit)), + )); + } else { + self.findings + .push(finding(plan, unit, strength, p, &answers, judgments)); + } + } + None if outcome == Outcome::Clear => count.clear += 1, + None => { + count.uncertain += 1; + self.undecided + .entry(unit.rule) + .or_default() + .push(undecided_unit(unit, &answers)); + } + } + if let ( + Detail::TestPair { names, subject, .. }, + Outcome::Review(p) | Outcome::Consider(p), + ) = (&unit.detail, grouping) + { + // A review pair stays its own finding: it says a test adds nothing. + let finding = matches!(grouping, Outcome::Consider(_)).then(|| self.findings.len() - 1); + self.redundant.push(Redundant { + unit, + names, + subject, + p, + finding, + }); + } + } +} + +/// Counts a unit that is too small, needs context or was left unasked under +/// a finished plan, and returns false for it; otherwise counts it as judged. +fn counted_as_judged(unit: &UnitPlan, judgments: &[Judgment], count: &mut UnitCounts) -> bool { + match unit.presence { + Presence::TooSmall => count.too_small += 1, + Presence::NeedsContext => count.needs_context += 1, + // A check left unasked because its document is a finished plan. + Presence::Judged + if matches!(unit.detail, Detail::Stale { .. } | Detail::DocPair { .. }) + && answers(judgments, &unit.id, Pass::Trace).is_empty() => + { + count.covered += 1 + } + Presence::Judged => { + count.judged += 1; + return true; + } + } + false +} + +/// The finding strength and probability of an outcome that raises one. +fn strength_of(outcome: Outcome) -> Option<(Strength, f64)> { + match outcome { + Outcome::Review(p) => Some((Strength::Review, p)), + Outcome::Consider(p) => Some((Strength::Consider, p)), + Outcome::Note(p) => Some((Strength::Note, p)), + _ => None, + } +} + +/// Questions whose undecided answer leaves a unit of the rule undecided; the +/// other questions are weak signals or only matter when decisive. +fn deciding_questions(rule: &str) -> &'static [&'static str] { + match rule { + catalog::FUNCTION_SIMPLIFICATION => &["split", "flatten"], + catalog::FILE_ORGANIZATION => &["split"], + catalog::SHARED_LOGIC => &["same"], + catalog::HARDCODED_VALUES => &["environment", "magic", "special"], + catalog::COMMENTS => &["restates", "verbose", "history", "disabled"], + catalog::TEST_VALUE => &["own_logic", "mock_only"], + catalog::INJECTION => &[ + "interpreted", + "resource", + "origin", + "sql", + "shell", + "code", + "markup", + "path", + "url", + "type", + "redirect", + "deserialize", + "xxe", + ], + catalog::SENSITIVE_DATA => &[ + "logs_secret", + "error_details", + "logs_object_secret", + "exception_to_client", + "environment_to_client", + "handler_leaks", + ], + catalog::UNSAFE_SETTINGS => &[ + "weakened", + "tls", + "hash", + "random", + "cors", + "cookie", + "debug", + "token", + "key", + "csrf", + "literal_secret", + ], + catalog::ACCESS_CONTROL => &[ + "others", + "editable", + "search_path", + "unchecked", + "broad", + "data", + "exposed", + "rows", + "returns_others", + "reach", + "argument_rows", + "operator_only", + ], + catalog::WORKFLOWS => &["outside", "untrusted"], + catalog::LAWS => &["states"], + catalog::LARGE_DOCS => &["split", "history"], + catalog::DOC_STALENESS => &["plan", "relies"], + catalog::DOC_DUPLICATION => &["a_covers", "b_covers", "conflict"], + catalog::AGENT_CONTEXT => &[ + "inferable", + "describes", + "commands", + "generic", + "history", + "enforced", + ], + _ => &["overlap"], + } +} + +/// Candidate values are listed with an undecided unit only when this few, +/// so the entry names what the question was about without repeating the code. +const SHOWN_VALUES: usize = 3; + +/// The unit and its undecided questions; with no answers, why. +fn undecided_unit(unit: &UnitPlan, answers: &Answers<'_>) -> Undecided { + let get = |q: &str| answers.get(q).copied(); + let settled_values = (unit.rule == catalog::HARDCODED_VALUES) + .then(|| value_signals(&get, &unit.detail, true)) + .flatten(); + // Instruction sections and section pairs settle some signals by others. + let settled_sections = match unit.rule { + catalog::AGENT_CONTEXT => super::outcome::section_signals(&get), + catalog::DOC_DUPLICATION => super::outcome::pair_signals(&get), + _ => None, + }; + let mut questions: Vec = match (settled_values, settled_sections) { + (Some(signals), _) => signals + .iter() + .filter(|(_, o, _)| matches!(o, Outcome::Uncertain(_))) + .map(|(q, ..)| question_label(q).to_string()) + .collect(), + (None, Some(signals)) => signals + .iter() + .filter(|(_, o)| matches!(o, Outcome::Uncertain(_))) + .map(|(q, _)| question_label(q).to_string()) + .collect(), + (None, None) => undecided_questions(unit.rule, answers), + }; + if answers.is_empty() { + questions.push("no answer".into()); + } + let values = match &unit.detail { + Detail::Values { values, .. } | Detail::Constants { values, .. } + if values.len() <= SHOWN_VALUES => + { + values.clone() + } + _ => Vec::new(), + }; + Undecided { + unit: match &unit.detail { + Detail::Outline { .. } => "file outline".into(), + // Policies are often named by what they allow, the same on each table. + Detail::Access(Access::Policy { table }) => format!("{} on {table}", unit.name), + _ => unit.name.clone(), + }, + values, + line: unit.locations.first().map_or(1, |l| l.start_line), + questions, + } +} + +/// The deciding questions of a rule whose own answers stayed undecided. +fn undecided_questions(rule: &str, answers: &Answers<'_>) -> Vec { + deciding_questions(rule) + .iter() + .filter(|q| { + answers.get(*q).is_some_and(|a| { + let outcome = match a { + Answer::Noul { .. } => noul(a), + _ if **q == "origin" => origin_outcome(a), + _ if ["data", "rows", "reach"].contains(q) => { + super::outcome::acceptable_levels(a) + } + _ => score(a), + }; + matches!(outcome, Outcome::Uncertain(_)) + }) + }) + .map(|q| question_label(q).to_string()) + .collect() +} + +/// A rule's status is its most severe unit outcome. +fn dimension(rule: &str, count: UnitCounts, concern: f64, undecided: Vec) -> Dimension { + Dimension { + decision_basis: basis(rule, &count), + status: counted_status(&count), + concern_probability: concern, + rule_version: catalog::rule_version(rule).into(), + units: count, + undecided, + } +} + +pub(super) fn counted_status(count: &UnitCounts) -> Status { + if count.review > 0 { + Status::Review + } else if count.consider > 0 { + Status::Consider + } else if count.needs_context > 0 { + Status::NeedsContext + } else if count.uncertain > 0 { + Status::Uncertain + } else if count.note > 0 { + Status::Note + } else if count.clear > 0 { + Status::Clear + } else { + Status::NotApplicable + } +} + +pub(super) fn file_status( + dimensions: &BTreeMap, + findings: &[Finding], +) -> Status { + let any = |status: Status| dimensions.values().any(|d| d.status == status); + if dimensions + .values() + .all(|d| d.status == Status::NotApplicable) + { + Status::NotApplicable + } else if findings.iter().any(|f| f.strength == Strength::Review) { + Status::Review + } else if findings.iter().any(|f| f.strength == Strength::Consider) { + Status::Consider + } else if any(Status::NeedsContext) { + Status::NeedsContext + } else if any(Status::Uncertain) { + Status::Uncertain + } else if !findings.is_empty() { + Status::Note + } else { + Status::Clear + } +} + +fn basis(rule: &str, count: &UnitCounts) -> String { + let noun = match rule { + catalog::FILE_ORGANIZATION => "outline", + catalog::FUNCTION_SIMPLIFICATION => "function", + catalog::SHARED_LOGIC => "candidate pair", + catalog::TEST_VALUE => "test", + catalog::HARDCODED_VALUES => "value unit", + catalog::COMMENTS => "comment", + catalog::INJECTION | catalog::SENSITIVE_DATA | catalog::UNSAFE_SETTINGS => "security unit", + catalog::ACCESS_CONTROL => "access statement", + catalog::WORKFLOWS => "workflow job", + catalog::LAWS => "law", + catalog::AGENT_CONTEXT => "section", + catalog::LARGE_DOCS => "document", + catalog::DOC_STALENESS => "document check", + catalog::DOC_DUPLICATION => "section pair", + _ => "test pair", + }; + let plural = |n: usize| if n == 1 { "" } else { "s" }; + let mut parts = Vec::new(); + if count.judged == 0 && count.needs_context == 0 { + parts.push(format!("No {noun}s to judge.")); + } else { + let mut outcomes = Vec::new(); + for (n, label) in [ + (count.review, "review"), + (count.consider, "consider"), + (count.note, "note"), + (count.clear, "clear"), + (count.uncertain, "uncertain"), + ] { + if n > 0 { + outcomes.push(format!("{n} {label}")); + } + } + parts.push(format!( + "{} {noun}{} judged{}.", + count.judged, + plural(count.judged), + if outcomes.is_empty() { + String::new() + } else { + format!(": {}", outcomes.join(", ")) + } + )); + } + if count.needs_context > 0 { + parts.push(format!( + "{} {noun}{} exceed the request limit and were not sent.", + count.needs_context, + plural(count.needs_context) + )); + } + if count.too_small > 0 { + parts.push(format!( + "{} {noun}{} too small to judge.", + count.too_small, + plural(count.too_small) + )); + } + if count.covered > 0 { + parts.push(format!( + "{} check{} inside finished plans not asked.", + count.covered, + plural(count.covered) + )); + } + if count.omitted > 0 { + parts.push(format!( + "{} candidate{} omitted by caps.", + count.omitted, + plural(count.omitted) + )); + } + 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() +} + +fn finding( + plan: &FilePlan, + unit: &UnitPlan, + strength: Strength, + p: f64, + answers: &Answers<'_>, + judgments: &[Judgment], +) -> Finding { + let name = &unit.name; + let mut locations = unit.locations.clone(); + let mut symbol = Some(name.clone()); + let mut block = None; + let mut category = None; + let (message, action) = match &unit.detail { + Detail::Function { blocks, .. } => { + block = located_block(unit, blocks, judgments, "block") + .filter(|b| !most_of(&b.location, &unit.locations)); + let bend = crate::analysis::bend::file(&plan.path); + function_wording(name, strength, p, answers, (block, bend)) + } + Detail::Outline { + tests, + groups, + members, + parts, + .. + } => { + if let Some(part) = deciding_part(answers, parts) { + symbol = part.names.first().cloned(); + locations = part.locations.clone(); + part_wording(part, strength, p) + } else { + let chosen = outline_groups(answers.get("module").copied(), groups, *members); + symbol = chosen.first().map(|group| group.id.clone()); + if !chosen.is_empty() { + locations = chosen.iter().flat_map(|g| g.locations.clone()).collect(); + } + let several = + several_kind(answers.get("split").copied(), answers.get("kind").copied()); + outline_wording(&chosen, *tests, several, strength, p) + } + } + Detail::Pair { + differences, + within_test, + in_tests, + in_cases, + } => pair_wording( + name, + differences, + (*within_test, *in_tests, *in_cases), + strength, + p, + ), + Detail::Values { .. } | Detail::Constants { .. } => { + let (wording, constant) = values_finding(unit, strength, p, answers, judgments); + if let Some(location) = constant { + symbol = location.symbol.clone(); + locations = vec![location]; + } + wording + } + Detail::Security { + sites, messages, .. + } => { + let (wording, site, named) = + security_finding(unit, (sites, messages), strength, p, answers); + block = site; + category = Some(named); + wording + } + Detail::Section { .. } => section_wording(name, &unit.detail, strength, p, answers), + Detail::Plan { facts } => { + symbol = None; + category = Some(super::grouping::FINISHED_PLAN.into()); + plan_wording(name, facts, p) + } + Detail::Stale { missing, .. } => stale_wording(name, missing, p), + Detail::DocPair { other, .. } => { + let (wording, conflict) = doc_pair_wording(name, other, answers, p); + if conflict { + category = Some("conflict".into()); + } + wording + } + Detail::Document { parts, .. } => { + symbol = None; + block = located_block(unit, parts, judgments, "part"); + document_wording(name, strength, p, answers, block) + } + Detail::Handler { registered } => { + category = Some("CWE-209 error details exposed".into()); + handler_wording(name, registered, strength, p) + } + Detail::Access(access @ (Access::Table | Access::View | Access::Reducer)) => { + let (wording, named) = module_wording(access, name, strength, p, answers); + category = Some(named); + wording + } + Detail::Access(access) => { + let subject = match access { + Access::Policy { table } => format!("Policy `{name}` on `{table}`"), + Access::Definer => format!("SECURITY DEFINER function `{name}`"), + _ => format!("A grant on `{name}`"), + }; + let (wording, named) = privilege_wording(&subject, strength, p, answers); + category = Some(named); + wording + } + Detail::Job { expressions } => { + let (wording, named) = job_wording(name, expressions, strength, p, answers); + category = Some(named); + wording + } + Detail::Comment { .. } => { + let reason = comment_reason(answers, documented(unit)); + comment_wording(name, &[(&unit.locations[0], reason)], strength, p) + } + Detail::Test { .. } => test_wording(name, strength, p, answers), + Detail::Law => law_wording(name, strength, p, answers), + Detail::TestPair { .. } => { + symbol = None; + test_pair_wording(name, strength == Strength::Review, p) + } + }; + let lines = locations + .iter() + .map(|l| l.end_line + 1 - l.start_line) + .sum::() + .max(unit.lines.min(1)); + // The located block comes first, so an agent acts on it; the function follows. + if let Some(block) = block { + locations.insert(0, block.location.clone()); + } + Finding { + rule: catalog::id(unit.rule).into(), + strength, + line: locations.first().map_or(1, |l| l.start_line), + message, + action: action.into(), + symbol, + rule_version: catalog::rule_version(unit.rule).into(), + concern_probability: p, + locations, + quote: unit.quote.clone(), + category, + // Only a special-cased identity groups across files: the same number + // or path can mean different things in different code. + values: located_value(unit, judgments) + .filter(|_| { + matches!( + answers.get("special").map(|a| noul(a)), + Some(Outcome::Review(_) | Outcome::Consider(_)) + ) + }) + .into_iter() + .collect(), + fingerprint: fingerprint(unit.rule, plan, &unit.identity), + rank: rank(p, lines), + baselined: false, + suppressed: None, + } +} diff --git a/src/units/compose/redundant.rs b/src/units/compose/redundant.rs new file mode 100644 index 0000000..72397b1 --- /dev/null +++ b/src/units/compose/redundant.rs @@ -0,0 +1,148 @@ +//! Redundant tests: pairs linked into groups of three or more, and +//! shared-logic findings that repeat what a redundancy finding says. +use super::*; + +/// A shared-logic finding whose every copy lies inside tests a redundancy +/// finding already names says the same thing twice: on sqlite-utils, 12 +/// test pairs were reported by both rules. The redundancy finding stays, +/// since it says which test to merge or delete. +pub(super) fn drop_copies_of_redundant_tests(findings: &mut Vec) { + let tests: Vec = findings + .iter() + .filter(|f| f.rule == catalog::id(catalog::TEST_REDUNDANCY)) + .flat_map(|f| f.locations.iter().cloned()) + .collect(); + let named = |l: &crate::schema::Location| { + tests + .iter() + .any(|t| t.path == l.path && t.start_line <= l.start_line && l.end_line <= t.end_line) + }; + findings.retain(|f| { + f.rule != catalog::id(catalog::SHARED_LOGIC) + || f.locations.is_empty() + || !f.locations.iter().all(named) + }); +} + +/// A redundant test pair, with the index of its finding (a note) when it +/// reached a consider, which a group of three or more tests reports instead. +pub(super) struct Redundant<'p> { + pub(super) unit: &'p UnitPlan, + pub(super) names: &'p [String; 2], + pub(super) subject: &'p String, + pub(super) p: f64, + pub(super) finding: Option, +} + +/// Tests linked by overlapping pairs on one subject, with where they are, +/// the lowest probability of their pairs, and their pairs' findings. +pub(super) struct Cluster<'a> { + subject: &'a String, + tests: BTreeSet<&'a String>, + locations: Vec, + p: f64, + findings: Vec>, +} + +/// Three or more tests linked by overlapping pairs on one subject: the tests +/// a chain of such pairs connects. Two pairs of one subject that share no +/// test stay two pairs; grouped by subject alone, sinatra's pair of redirect +/// tests and pair of deny tests of `get` read as four overlapping tests. +/// Also returns the indices of the pair findings each group reports. +pub(super) fn over_tested( + plan: &FilePlan, + redundant: &[Redundant<'_>], +) -> (Vec, BTreeSet) { + let clusters = clusters(redundant); + let grouped = clusters + .iter() + .flat_map(|c| c.findings.iter().flatten().copied()) + .collect(); + let groups = clusters + .into_iter() + .map(|cluster| group_finding(plan, cluster)) + .collect(); + (groups, grouped) +} + +/// The clusters of three or more tests that overlapping pairs connect. +pub(super) fn clusters<'a>(redundant: &'a [Redundant<'_>]) -> Vec> { + let mut clusters: Vec> = Vec::new(); + for pair in redundant { + let mut joined = Cluster { + subject: pair.subject, + tests: BTreeSet::new(), + locations: Vec::new(), + p: pair.p, + findings: vec![pair.finding], + }; + let mut index = 0; + while index < clusters.len() { + let other = &clusters[index]; + if other.subject == pair.subject && pair.names.iter().any(|n| other.tests.contains(n)) { + let other = clusters.remove(index); + joined.tests.extend(other.tests); + joined.locations.extend(other.locations); + joined.p = joined.p.min(other.p); + joined.findings.extend(other.findings); + } else { + index += 1; + } + } + for (name, location) in pair.names.iter().zip(&pair.unit.locations) { + if joined.tests.insert(name) { + joined.locations.push(location.clone()); + } + } + clusters.push(joined); + } + clusters.sort_by(|a, b| (a.subject, &a.tests).cmp(&(b.subject, &b.tests))); + clusters.retain(|c| c.tests.len() >= 3); + clusters +} + +/// The consider that names a cluster's tests. +pub(super) fn group_finding(plan: &FilePlan, cluster: Cluster<'_>) -> Finding { + let Cluster { + subject, + tests, + mut locations, + p, + .. + } = cluster; + locations.sort(); + let names: Vec = tests.iter().map(|t| format!("`{t}`")).collect(); + let lines = locations + .iter() + .map(|l| l.end_line + 1 - l.start_line) + .sum(); + let identity: Vec<&str> = std::iter::once(subject.as_str()) + .chain(tests.iter().map(|t| t.as_str())) + .collect(); + Finding { + rule: catalog::id(catalog::TEST_REDUNDANCY).into(), + strength: Strength::Consider, + line: locations.first().map_or(1, |l| l.start_line), + message: format!( + "{} tests of `{subject}` overlap: {} ({p:.2}).", + tests.len(), + names.join(", ") + ), + action: "Consider one parameterized test for these cases".into(), + symbol: Some(subject.clone()), + rule_version: catalog::rule_version(catalog::TEST_REDUNDANCY).into(), + concern_probability: p, + locations, + quote: None, + category: None, + values: Vec::new(), + fingerprint: fingerprint( + catalog::TEST_REDUNDANCY, + plan, + &crate::units::identity(&identity), + ), + rank: rank(p, lines), + baselined: false, + suppressed: None, + } +} diff --git a/src/units/evidence.rs b/src/units/evidence.rs index 99a969d..2e9c36e 100644 --- a/src/units/evidence.rs +++ b/src/units/evidence.rs @@ -1,7 +1,7 @@ //! Request building shared by every planner: one file's facts, the request //! envelope, packing, and stable identities. use super::{Asked, PACK_ITEMS, Questions}; -use crate::{schema::Location, token_budget::TokenBudget}; +use crate::{schema::Location, token_budget::Limits}; use serde_json::{Value, json}; use sha2::{Digest, Sha256}; use std::{collections::BTreeMap, path::Path}; @@ -19,7 +19,7 @@ pub(super) struct FileContext<'a> { pub source: &'a str, pub source_hash: &'a str, pub model: &'a str, - pub budget: &'a TokenBudget, + pub budget: Limits<'a>, /// What a web framework makes of the file, such as a Next.js route /// handler or Server Actions module, sent beside its path. pub framework: Option, diff --git a/src/units/follow_ups.rs b/src/units/follow_ups.rs index bb2f676..567adcf 100644 --- a/src/units/follow_ups.rs +++ b/src/units/follow_ups.rs @@ -1,6 +1,6 @@ //! Follow-up requests that recorded answers call for: traces of security -//! units, rechecks of undecided units, the kind of an outline still undecided -//! and locating split findings. +//! units, rechecks of undecided units, the kind of an outline still undecided, +//! the parts of a long file without a finding and locating split findings. use super::{Detail, FollowUp, Plan, Planned, UnitPlan, compose}; use crate::schema::{FileResult, Judgment, Status}; use std::collections::BTreeSet; @@ -144,6 +144,27 @@ pub fn kinds(plan: &Plan, files: &[FileResult]) -> Vec { }) } +/// Whether each candidate part of a long file does a job of its own, asked +/// once the file's outline, its recheck and its kind raised no finding. +pub fn parts(plan: &Plan, files: &[FileResult]) -> Vec { + let mut planned = Vec::new(); + for (&owner, file_plan) in &plan.files { + let file = &files[owner]; + if file.status == Status::Error { + continue; + } + let selected = compose::unparted_units(file_plan, &file.judgments); + for unit in &file_plan.units { + if let Detail::Outline { parts, .. } = &unit.detail + && selected.contains(&unit.id) + { + planned.extend(parts.iter().map(|part| part.follow_up.planned(owner))); + } + } + } + planned +} + /// The planned follow-up of every selected unit, skipping failed files. fn follow_ups( plan: &Plan, diff --git a/src/units/handlers/mod.rs b/src/units/handlers/mod.rs index a5958c2..a607e3a 100644 --- a/src/units/handlers/mod.rs +++ b/src/units/handlers/mod.rs @@ -20,7 +20,7 @@ use crate::{ catalog::SENSITIVE_DATA, options::CheckArgs, schema::Pass, - token_budget::TokenBudget, + token_budget::Limits, }; use classes::error_classes; use implemented::implemented; @@ -39,7 +39,7 @@ pub(super) fn plan( scope: &Scope<'_>, evidence: &Evidence<'_>, args: &CheckArgs, - budget: &TokenBudget, + budget: Limits<'_>, result: &mut Plan, ) { let handlers = error_handlers(scope, evidence.links); diff --git a/src/units/mod.rs b/src/units/mod.rs index e1f61e4..11b74f2 100644 --- a/src/units/mod.rs +++ b/src/units/mod.rs @@ -33,7 +33,7 @@ mod workflows; use answers::Questions; pub use answers::{Asked, record}; use evidence::{FileContext, compact, identity, pack, pack_runs, request, unique_ids}; -pub use follow_ups::{doc_checks, kinds, locates, rechecks, settles, traces, value_kinds}; +pub use follow_ups::{doc_checks, kinds, locates, parts, rechecks, settles, traces, value_kinds}; use plan::Scope; pub use plan::plan; pub use spacetimedb::spacetimedb_module; @@ -69,6 +69,19 @@ pub struct GroupInfo { pub locations: Vec, } +/// A candidate part of a long file and the follow-up that asks whether it +/// does a job of its own, sent only when the outline raised no finding. +#[derive(Clone, Debug)] +pub struct Part { + /// The ids its answers are recorded under, from `outline::PART_QUESTIONS`. + pub questions: (&'static str, &'static str), + pub names: Vec, + pub locations: Vec, + /// The lines of its members. + pub lines: usize, + pub follow_up: FollowUp, +} + /// A top-level block of a function body, offered when locating a split. #[derive(Clone, Debug)] pub struct Block { @@ -139,6 +152,10 @@ pub enum Detail { sections: usize, /// What kind of file it is, asked after a recheck that stays undecided. kind: Option, + /// The candidate parts of a long application file, each asked + /// whether it does a job of its own once the outline stays without + /// a finding. + parts: Vec, }, Pair { differences: Vec, diff --git a/src/units/outcome/documentation.rs b/src/units/outcome/documentation.rs index 613bdb3..3fdf0ff 100644 --- a/src/units/outcome/documentation.rs +++ b/src/units/outcome/documentation.rs @@ -66,32 +66,11 @@ pub(in crate::units) fn document_outcome<'a>( /// split or a finding, and a collection of unrelated subjects reaching it /// raises an undecided split to a consider. pub(in crate::units) fn document_split(split: &Answer, kind: Option<&Answer>) -> Outcome { - let outcome = benefit(split); - let ( - Outcome::Uncertain(_) | Outcome::Consider(_) | Outcome::Review(_), - Some(Answer::Choice { probabilities, .. }), - ) = (outcome, kind) - else { - return outcome; - }; - let mass: f64 = probabilities.values().sum(); - if mass <= 0.0 { - return outcome; - } - let several: f64 = probabilities - .iter() - .filter(|(kind, _)| { - crate::units::questions::SEVERAL_DOCUMENT_KINDS.contains(&kind.as_str()) - }) - .map(|(_, p)| p / mass) - .sum(); - if at_least(1.0 - several) { - Outcome::Clear - } else if at_least(several) && matches!(outcome, Outcome::Uncertain(_)) { - Outcome::Consider(several) - } else { - outcome - } + weighed_by_kind( + benefit(split), + kind, + &crate::units::questions::SEVERAL_DOCUMENT_KINDS, + ) } /// Each answered question about an instruction section with its outcome. diff --git a/src/units/outcome/maintainability.rs b/src/units/outcome/maintainability.rs index 85f418c..0bd8606 100644 --- a/src/units/outcome/maintainability.rs +++ b/src/units/outcome/maintainability.rs @@ -79,36 +79,53 @@ pub(in crate::units) fn values_outcome<'a>( /// the recheck raised it from an undecided first answer: of the 18 such /// findings the kind cleared on the corpus, 4 were right, and one was the /// proposal to split JevGate's own planner of one unit per rule. +/// +/// An outline left without a finding then reads its candidate parts: a part +/// of a long file that does a job of its own raises a consider (see +/// `separable_part`). pub(in crate::units) fn organization_outcome( split: Option<&Answer>, kind: Option<&Answer>, + parts: &[PartAnswers<'_>], ) -> Option { - let outcome = benefit(split?); - let ( - Outcome::Uncertain(_) | Outcome::Consider(_) | Outcome::Review(_), - Some(Answer::Choice { probabilities, .. }), - ) = (outcome, kind) - else { - return Some(outcome); - }; - let mass: f64 = probabilities.values().sum(); - if mass <= 0.0 { - return Some(outcome); - } - let several: f64 = probabilities - .iter() - .filter(|(kind, _)| questions::SEVERAL_KINDS.contains(&kind.as_str())) - .map(|(_, p)| p / mass) - .sum(); - Some(if at_least(1.0 - several) { - Outcome::Clear - } else if at_least(several) && matches!(outcome, Outcome::Uncertain(_)) { - Outcome::Consider(several) - } else { - outcome + let outcome = weighed_by_kind(benefit(split?), kind, &questions::SEVERAL_KINDS); + Some(match separable_part(parts) { + Some((_, p)) if !matches!(outcome, Outcome::Review(_) | Outcome::Consider(_)) => { + Outcome::Consider(p) + } + _ => outcome, }) } +/// One candidate part's answers: whether it does a job of its own, and what +/// it is within its file. +pub(in crate::units) type PartAnswers<'a> = (Option<&'a Answer>, Option<&'a Answer>); + +/// The candidate part, by position, that does a job of its own, with the +/// probability that it does: its own Noul reaches the location probability +/// and the role Choice leans to a job of its own. The role keeps apart what +/// the Noul alone did not: more of what the rest of the file does, such as a +/// dialect's other queries, read as a job of its own on its own (6 files +/// labeled for splitting against 3 to keep, and no file to keep with the +/// role). Of 50 long files the outline cleared, 15 of them worth splitting +/// by their labels, it raised 4, all worth splitting, each naming the part +/// the labeler named; on the corpus, 9 of its 11 considers were right. +pub(in crate::units) fn separable_part(parts: &[PartAnswers<'_>]) -> Option<(usize, f64)> { + parts + .iter() + .enumerate() + .filter_map(|(position, &(own, role))| { + let Some(Answer::Noul { noul }) = own else { + return None; + }; + let own_job = choice_mass(role, &[questions::OWN_JOB])?; + (probability_at_least(*noul, LOCATION_PROBABILITY) + && probability_at_least(own_job, LEADING_PROBABILITY)) + .then_some((position, *noul)) + }) + .max_by(|a, b| a.1.total_cmp(&b.1).then(b.0.cmp(&a.0))) +} + /// The kind of file that decided an undecided split Score toward a split: /// the likelier of the kinds that serve several features. pub(in crate::units) fn several_kind<'a>( diff --git a/src/units/outcome/mod.rs b/src/units/outcome/mod.rs index d8e96e5..917b3a1 100644 --- a/src/units/outcome/mod.rs +++ b/src/units/outcome/mod.rs @@ -34,8 +34,8 @@ pub(super) use injection::{ RESOURCE_CHECKS, confirmable, harmless, injection_outcome, origin_outcome, }; pub(super) use maintainability::{ - benign_key, function_outcome, organization_outcome, several_kind, shared_outcome, - value_signals, values_outcome, + PartAnswers, benign_key, function_outcome, organization_outcome, separable_part, several_kind, + shared_outcome, value_signals, values_outcome, }; use pairs::doc_pair_outcome; pub(super) use pairs::{disagreement, pair_signals, repeated}; @@ -190,6 +190,48 @@ pub(super) fn choice(answer: Option<&Answer>) -> Option<(&str, f64)> { pub(super) type Answers<'a> = BTreeMap<&'a str, &'a Answer>; +/// A split weighed with the kind of file or document once it is asked: the +/// kinds that serve one feature or subject reaching the threshold clear an +/// undecided split or a finding, and the kinds in `several` reaching it +/// raise an undecided split to a consider. +pub(super) fn weighed_by_kind( + outcome: Outcome, + kind: Option<&Answer>, + several: &[&str], +) -> Outcome { + let ( + Outcome::Uncertain(_) | Outcome::Consider(_) | Outcome::Review(_), + Some(Answer::Choice { probabilities, .. }), + ) = (outcome, kind) + else { + return outcome; + }; + let mass: f64 = probabilities.values().sum(); + if mass <= 0.0 { + return outcome; + } + let share: f64 = probabilities + .iter() + .filter(|(kind, _)| several.contains(&kind.as_str())) + .map(|(_, p)| p / mass) + .sum(); + if at_least(1.0 - share) { + Outcome::Clear + } else if at_least(share) && matches!(outcome, Outcome::Uncertain(_)) { + Outcome::Consider(share) + } else { + outcome + } +} + +/// Each candidate part's answers, in part order. +pub(super) fn part_answers<'a>(get: &impl Fn(&str) -> Option<&'a Answer>) -> Vec> { + super::outline::PART_QUESTIONS + .iter() + .map(|(own, role)| (get(own), get(role))) + .collect() +} + pub(super) fn unit_outcome(unit: &UnitPlan, answers: &Answers<'_>) -> Outcome { let outcome = rule_outcome(unit, answers).unwrap_or(Outcome::Missing); in_examples(unit, outcome) @@ -206,7 +248,8 @@ fn rule_outcome(unit: &UnitPlan, answers: &Answers<'_>) -> Option { // with one caller still helps a reader find a feature of a large file. // A test file's layout is advice, one level lower. catalog::FILE_ORGANIZATION => { - organization_outcome(get("split"), get("kind")).map(|outcome| { + let parts = part_answers(&get); + organization_outcome(get("split"), get("kind"), &parts).map(|outcome| { if matches!(unit.detail, Detail::Outline { tests: true, .. }) { lowered(outcome) } else { diff --git a/src/units/outline.rs b/src/units/outline.rs index 2657dd7..1bb524b 100644 --- a/src/units/outline.rs +++ b/src/units/outline.rs @@ -2,7 +2,7 @@ //! without bodies. A test file's members are its test cases and the support //! code they share. use super::{ - Asked, Detail, FileContext, FilePlan, GroupInfo, Planned, Presence, Questions, UnitPlan, + Asked, Detail, FileContext, FilePlan, GroupInfo, Part, Planned, Presence, Questions, UnitPlan, identity, questions, }; use crate::{ @@ -30,6 +30,23 @@ pub const MIN_FILE_LINES: usize = 100; /// the 13 right ones were on files of 313 member lines or more. pub const MIN_BEND_FILE_LINES: usize = 300; +/// Files shorter than this are not asked about their parts: the labeled +/// splits of files the outline cleared were all on files of 400 lines or more. +const PART_FILE_LINES: usize = 400; +/// A part shorter than this is not asked about: of four parts of 80 to 99 +/// lines found doing a job of their own, two were worth moving. +const PART_LINES: usize = 100; +/// The question ids of each candidate part's follow-up, in part order: a +/// part's answers are recorded beside the outline's own. +pub(super) const PART_QUESTIONS: [(&str, &str); groups::MAX_GROUPS] = [ + ("part1_own", "part1_role"), + ("part2_own", "part2_role"), + ("part3_own", "part3_role"), + ("part4_own", "part4_role"), + ("part5_own", "part5_role"), + ("part6_own", "part6_role"), +]; + /// One listed member: its name, its lines and the state sent for it. struct Member { name: String, @@ -56,7 +73,122 @@ pub(super) fn plan( .collect(); let listed = member_state(file, units, members, callers); let source = application_source(file.source, units, members); - plan_outline(file, false, listed, sets, source, out, requests); + let parts = plan_parts(file, parsed, members); + plan_outline( + file, + false, + listed, + sets, + Beyond { source, parts }, + out, + requests, + ); +} + +/// The candidate parts of a long application file, each with the follow-up +/// that asks whether it does a job of its own: at least `PART_LINES` lines +/// and at most three quarters of the members' lines, since moving most of a +/// file moves the file rather than splitting it. +fn plan_parts(file: &FileContext<'_>, parsed: &FileUnits, members: &[usize]) -> Vec { + if file.source.lines().count() < PART_FILE_LINES + || crate::analysis::bend::file(file.path) + || tooling(file.path) + { + return Vec::new(); + } + let units = &parsed.units; + let total: usize = members.iter().map(|&m| units[m].lines()).sum(); + groups::parts(units, members, &parsed.imports) + .into_iter() + .filter(|part| { + let lines: usize = part.iter().map(|&m| units[m].lines()).sum(); + lines >= PART_LINES + && lines * 4 <= total * 3 + && !part.iter().any(|&m| units[m].short_name == "main") + }) + .zip(PART_QUESTIONS) + .filter_map(|(part, ids)| { + let (request, asked) = part_request(file, units, members, &part, ids); + file.budget.fits(&request).then(|| Part { + questions: ids, + names: part.iter().map(|&m| units[m].name.clone()).collect(), + locations: part + .iter() + .map(|&m| file.location(units[m].line, units[m].end_line, Some(&units[m].name))) + .collect(), + lines: part.iter().map(|&m| units[m].lines()).sum(), + follow_up: (request, asked).into(), + }) + }) + .collect() +} + +/// Benchmarks, examples, scripts and documentation tooling: a script runs +/// its steps top to bottom and a benchmark or experiment is often pinned by +/// hash, so their parts read as stages of one job. On the corpus, 5 part +/// findings in `benchmarks`, `scripts` and `docs` directories were all +/// wrong, and so was the one part holding a script's `main`. +fn tooling(path: &std::path::Path) -> bool { + crate::analysis::clones::benchmark_code(path) + || crate::analysis::clones::example_code(path) + || path.parent().is_some_and(|dir| { + dir.iter().any(|part| { + matches!( + part.to_string_lossy().to_ascii_lowercase().as_str(), + "scripts" | "script" | "docs" | "doc" + ) + }) + }) +} + +/// A part's members with their source, and the file's other members by +/// signature: the part itself, not the whole file, is judged. +fn part_request( + file: &FileContext<'_>, + units: &[Unit], + members: &[usize], + part: &[usize], + (own, role): (&'static str, &'static str), +) -> (Value, Asked) { + let mut questions = Questions::default(); + let own_job = questions::outline_part_own(); + questions.ask( + "own".into(), + own_job, + ID, + FILE_ORGANIZATION, + own, + Pass::Locate, + ); + let kind = questions::outline_part_role(); + questions.ask( + "role".into(), + kind, + ID, + FILE_ORGANIZATION, + role, + Pass::Locate, + ); + let lines: Vec<&str> = file.source.lines().collect(); + let source: Vec = part + .iter() + .map(|&m| lines[units[m].line - 1..units[m].end_line.min(lines.len())].join("\n")) + .collect(); + let rest: Vec = members + .iter() + .filter(|m| !part.contains(m)) + .map(|&m| json!({"name": units[m].name, "signature": units[m].signature})) + .collect(); + let mut state = json!({ + "file": file.plain_state(), + "part": { + "members": part.iter().map(|&m| &units[m].name).collect::>(), + "source": source.join("\n"), + }, + "rest": rest, + }); + state["file"]["lines"] = json!(lines.len()); + file.request("parts", state, questions) } /// A test file's cases, with their suites and subjects, and the helpers, @@ -114,15 +246,18 @@ pub(super) fn plan_tests( ) .collect(); let sets = groups::test_groups(cases, &support); - plan_outline( - file, - true, - listed, - sets, - file.source.to_string(), - out, - requests, - ); + let beyond = Beyond { + source: file.source.to_string(), + parts: Vec::new(), + }; + plan_outline(file, true, listed, sets, beyond, out, requests); +} + +/// What an outline's follow-ups send beyond it: the file's source for the +/// recheck and the kind, and the candidate parts of a long application file. +struct Beyond { + source: String, + parts: Vec, } /// The outline unit and its request; `sets` are groups of positions in `listed`. @@ -131,7 +266,7 @@ fn plan_outline( tests: bool, listed: Vec, sets: Vec>, - source: String, + Beyond { source, parts }: Beyond, out: &mut FilePlan, requests: &mut Vec, ) { @@ -191,6 +326,7 @@ fn plan_outline( .map(|source| outline.request(file, Ask::Kind(source))) .find(|(request, _)| file.budget.fits(request)) .map(Into::into), + parts: if judged { parts } else { Vec::new() }, groups: ids .into_iter() .zip(&sets) diff --git a/src/units/plan/file.rs b/src/units/plan/file.rs index 9cd0498..99d4614 100644 --- a/src/units/plan/file.rs +++ b/src/units/plan/file.rs @@ -11,7 +11,7 @@ use crate::{ file_kind::View, inventory::Input, options::CheckArgs, - token_budget::TokenBudget, + token_budget::Limits, units::{ FileContext, FilePlan, Planned, comments, duplicates, functions, hardcoded, laws, outline, spacetimedb, test_units, @@ -29,7 +29,7 @@ pub(super) fn plan_file( shared: &Shared<'_>, owner: usize, args: &CheckArgs, - budget: &TokenBudget, + budget: Limits<'_>, requests: &mut Vec, ) -> FilePlan { let input = &scope.inputs[owner]; @@ -134,7 +134,7 @@ fn file_context<'a>( input: &'a Input, owner: usize, args: &'a CheckArgs, - budget: &'a TokenBudget, + budget: Limits<'a>, ) -> FileContext<'a> { FileContext { owner, diff --git a/src/units/plan/mod.rs b/src/units/plan/mod.rs index 6d5d86f..2986009 100644 --- a/src/units/plan/mod.rs +++ b/src/units/plan/mod.rs @@ -24,7 +24,7 @@ use crate::{ file_kind::View, inventory::Input, options::CheckArgs, - token_budget::TokenBudget, + token_budget::{Limits, TokenBudget}, }; use std::{ @@ -89,6 +89,9 @@ pub fn plan( budget: &TokenBudget, root: &std::path::Path, ) -> Plan { + let answered = + |request: &serde_json::Value| crate::requests::answered(root, args, request).is_some(); + let budget = Limits::new(budget, &answered); let mut result = Plan::default(); let scope = parsed_scope(inputs, views, &mut result.skipped); let mut shared = Shared::new(&scope, args); @@ -134,7 +137,7 @@ pub fn plan( } /// Each GitHub Actions workflow file's jobs. -fn plan_workflows(scope: &Scope<'_>, args: &CheckArgs, budget: &TokenBudget, result: &mut Plan) { +fn plan_workflows(scope: &Scope<'_>, args: &CheckArgs, budget: Limits<'_>, result: &mut Plan) { for &owner in &scope.configuration { let input = &scope.inputs[owner]; if input.result.role != crate::inventory::WORKFLOW { @@ -164,7 +167,7 @@ fn plan_document( input: &Input, owner: usize, args: &CheckArgs, - budget: &TokenBudget, + budget: Limits<'_>, drift: &drift::Shared<'_>, requests: &mut Vec, ) -> FilePlan { diff --git a/src/units/questions/maintainability.rs b/src/units/questions/maintainability.rs index 56f728b..fc990e6 100644 --- a/src/units/questions/maintainability.rs +++ b/src/units/questions/maintainability.rs @@ -175,6 +175,49 @@ pub fn outline_kind(tests: bool) -> Value { }) } +const PART_NOTE: &str = "`part` holds some members of the file with their source; `rest` lists the file's other members by signature."; + +/// Whether a part of a long file does a job of its own, asked of each +/// candidate part with its source once the outline raised no finding. +/// Asked of the whole outline, the split and the kind of file stayed "one +/// feature" for a scraper inside a model and a diff engine inside a +/// renderer; asked of the part itself, with its code, they stood apart. +pub fn outline_part_own() -> Value { + json!({ + "type": "noul", + "instructions": { + "question": "Does `part` do a job of its own that a reader would look for apart from the rest of the file?", + "note": format!("{PART_NOTE} {EVIDENCE}"), + }, + "criteria": { + "true": "A separate piece of work, such as its own feature, layer, data table, parser, codec or algorithm, that the rest of the file only calls or does not use, and that reads on its own.", + "false": "One piece of the file's single job: its steps, methods or cases share the state and purpose of the rest, and reading it needs the rest of the file.", + }, + }) +} + +/// Kinds of part that do a job of their own; every other kind serves the file's own. +pub const OWN_JOB: &str = "own_job"; + +/// What a candidate part of a long file is within it, asked beside +/// `outline_part_own`: more of what the rest does (a dialect's other +/// queries, a class's other methods) read as a job of its own on its own. +pub fn outline_part_role() -> Value { + json!({ + "type": "choice", + "instructions": { + "question": "Which best describes `part` within this file?", + "note": format!("{PART_NOTE} {EVIDENCE}"), + }, + "criteria": { + OWN_JOB: "A job of its own with its own vocabulary, such as a parser, a scraper, an algorithm, a set of validators, a formatter or a data table, that the rest of the file only calls or does not use.", + "same_kind": "More of what the rest of the file does: other methods of the same class or interface, or other handlers, queries, cases or definitions of the same kind.", + "support": "Small helpers, types or constants that the rest of the file uses throughout.", + "core": "The file's main job, which the other parts serve.", + }, + }) +} + /// Asked only after a finding on a file's constants, to name the constant it /// is about: "one of this file's constants" left a reader to search 14 of /// them for the three URLs holding the author's account name. diff --git a/src/units/questions/mod.rs b/src/units/questions/mod.rs index 32d677a..599e908 100644 --- a/src/units/questions/mod.rs +++ b/src/units/questions/mod.rs @@ -131,6 +131,8 @@ mod tests { outline_module(true, &["G1".into(), "G2".into()]), outline_kind(false), outline_kind(true), + outline_part_own(), + outline_part_role(), duplicate_same(false), duplicate_same(true), duplicate_only_differences(), diff --git a/src/units/tests/mod.rs b/src/units/tests/mod.rs index f4c4b93..9755cca 100644 --- a/src/units/tests/mod.rs +++ b/src/units/tests/mod.rs @@ -33,7 +33,7 @@ fn planned(project: &Project, options: &CheckArgs) -> (Vec, Plan) { .iter() .enumerate() .filter_map( - |(i, input)| match crate::file_kind::plan(input, options, &budget) { + |(i, input)| match crate::file_kind::plan(input, options, budget.uncached()) { Ok(crate::file_kind::Plan::Ready(view)) => Some((i, view)), _ => None, }, diff --git a/src/units/tests/organization.rs b/src/units/tests/organization.rs index 3d9f0b0..b2589f6 100644 --- a/src/units/tests/organization.rs +++ b/src/units/tests/organization.rs @@ -134,6 +134,33 @@ fn an_outline_too_long_for_a_recheck_is_decided_by_its_kind_alone() { assert!(kind.request()["state"]["file"]["source"].is_null()); } +#[test] +fn a_request_answered_once_fits_whatever_the_calibration() { + // About 79 KB: the recheck sends it whole, which the estimate fits at the + // default 3.0 bytes per token and not at 2.0. + let padding = format!("// {}\n", "x".repeat(100)).repeat(700); + let (project, options) = organized("lib.rs", &format!("{}{padding}", two_concerns())); + let first = run(&project, &options, &mut scripted(3)); + assert_eq!(first.stages["recheck"].successful_requests, 1); + let context = project.context(); + let (inputs, mut report) = crate::tests::snapshot(&project, &options); + let store = crate::storage::Store::open(&project.0).unwrap(); + let mut mock = Mock::default(); + let mut session = crate::tests::session(&options, &context, &store, &mut mock); + session.budget = TokenBudget { + bytes_per_token: 2.0, + }; + session.evaluate(&inputs, &mut report).unwrap(); + assert_eq!(mock.calls, 0, "every request comes from the cache"); + assert_eq!(report.stages["recheck"].cache_hits, 1); + let status = |report: &Report| { + report.files[0].dimensions["file_organization"] + .status + .clone() + }; + assert_eq!(status(&report), status(&first)); +} + #[test] fn outlines_carry_member_and_file_sizes() { let (project, options) = rule_project(&two_concerns(), catalog::FILE_ORGANIZATION); @@ -362,3 +389,134 @@ fn a_bend_file_s_split_is_at_most_a_consider_and_a_note_in_titled_sections() { assert_eq!(strength(false), Status::Consider); assert_eq!(strength(true), Status::Note); } + +/// A parser and a renderer of ten functions each, every function naming its +/// part's two types: two parts of over two hundred lines, 470 in all. +fn two_parts() -> String { + parts_of_steps(20) +} + +/// `two_parts` with `steps` statements in each function. +fn parts_of_steps(steps: usize) -> String { + let mut source = String::new(); + for (part, state, item) in [ + ("parse", "Parser", "Token"), + ("render", "Renderer", "Glyph"), + ] { + source.push_str(&format!( + "struct {state} {{ at: usize }}\nstruct {item} {{ size: u8 }}\n" + )); + for i in 0..10 { + source.push_str(&format!( + "fn {part}_{i}(state: &{state}, item: &{item}) -> usize {{\n" + )); + for step in 0..steps { + source.push_str(&format!( + " let v{step} = state.at + item.size as usize + {step};\n" + )); + } + source.push_str(&format!(" v{}\n}}\n", steps - 1)); + } + } + source +} + +/// Answers that clear the outline and find each part a job of its own. +fn parts_of_their_own(own: f64, own_job: f64) -> Scripted { + let mut eval = scripted(0); + let chosen = if own_job >= 0.5 { + "own_job" + } else { + "same_kind" + }; + let role = json!({"type": "choice", "choice": chosen, "confidence": 0.5, + "probabilities": {"own_job": own_job, "same_kind": 1.0 - own_job, "support": 0.0, "core": 0.0}}); + eval.overrides = vec![ + ("own", json!({"type": "noul", "noul": own})), + ("role", role), + ]; + eval +} + +#[test] +fn a_long_file_s_parts_are_planned_with_their_source_and_the_rest_by_signature() { + let (project, options) = organized("src/lib.rs", &two_parts()); + let (_, plan) = planned(&project, &options); + let Detail::Outline { parts, .. } = &plan.files.values().next().unwrap().units[0].detail else { + panic!("an outline"); + }; + let names: Vec> = parts + .iter() + .map(|p| p.names.iter().map(String::as_str).take(3).collect()) + .collect(); + assert_eq!( + names, + [ + ["Parser", "Token", "parse_0"], + ["Renderer", "Glyph", "render_0"] + ] + ); + let request = parts[0].follow_up.request(); + let source = request["state"]["part"]["source"].as_str().unwrap(); + assert!(source.contains("fn parse_9(") && !source.contains("fn render_0(")); + assert_eq!(request["state"]["rest"][0]["name"], "Renderer"); + assert!( + request["state"]["rest"][2]["signature"] + .as_str() + .unwrap() + .contains("render_0") + ); + for (path, source) in [ + ("scripts/lib.rs", two_parts()), + // 370 lines, short enough to read whole. + ("src/short.rs", parts_of_steps(15)), + ] { + let (project, options) = organized(path, &source); + let (_, plan) = planned(&project, &options); + let Detail::Outline { parts, .. } = &plan.files.values().next().unwrap().units[0].detail + else { + panic!("an outline"); + }; + assert!(parts.is_empty(), "{path} is not asked about its parts"); + } +} + +#[test] +fn a_part_of_a_long_file_that_does_a_job_of_its_own_is_a_consider_naming_it() { + let (project, mut options) = organized("src/lib.rs", &two_parts()); + let report = run(&project, &options, &mut parts_of_their_own(0.8, 0.7)); + assert_eq!(report.stages["parts"].successful_requests, 2); + let finding = &report.files[0].findings[0]; + assert_eq!(finding.strength, Strength::Consider); + assert!( + finding.message.starts_with("`Parser`, `Token`, `parse_0`") + && finding.message.contains("do a job of their own"), + "{}", + finding.message + ); + assert_eq!(finding.symbol.as_deref(), Some("Parser")); + assert_eq!(finding.locations.len(), 12); + options.refresh = true; + for (own, own_job) in [(0.6, 0.9), (0.9, 0.4)] { + let report = run(&project, &options, &mut parts_of_their_own(own, own_job)); + assert_eq!( + report.files[0].dimensions["file_organization"].status, + Status::Clear, + "own {own}, own job {own_job}" + ); + } +} + +#[test] +fn an_outline_with_a_finding_is_not_asked_about_its_parts() { + let (project, options) = organized("src/lib.rs", &two_parts()); + let mut eval = parts_of_their_own(0.9, 0.9); + eval.level = 2; + let report = run(&project, &options, &mut eval); + assert!(!report.stages.contains_key("parts")); + assert!( + report.files[0].findings[0] + .message + .contains("several features") + ); +} diff --git a/src/units/tests/pipeline.rs b/src/units/tests/pipeline.rs index f3c2c82..0857284 100644 --- a/src/units/tests/pipeline.rs +++ b/src/units/tests/pipeline.rs @@ -115,7 +115,7 @@ fn packing_and_cache_identity_do_not_depend_on_token_calibration() { let budget = TokenBudget { bytes_per_token }; let views = BTreeMap::from([( 0, - match crate::file_kind::plan(&inputs[0], &options, &budget).unwrap() { + match crate::file_kind::plan(&inputs[0], &options, budget.uncached()).unwrap() { crate::file_kind::Plan::Ready(view) => view, _ => unreachable!(), }, diff --git a/src/units/wording/maintainability.rs b/src/units/wording/maintainability.rs index 13635a1..7471ab9 100644 --- a/src/units/wording/maintainability.rs +++ b/src/units/wording/maintainability.rs @@ -94,21 +94,7 @@ pub(in crate::units) fn outline_wording( }; let shown: Vec = chosen .iter() - .map(|group| { - let names: Vec<_> = group - .names - .iter() - .take(6) - .map(|n| format!("`{n}`")) - .collect(); - let more = group.names.len().saturating_sub(names.len()); - let more = if more > 0 { - format!(" and {more} more") - } else { - String::new() - }; - format!("{} ({}{more})", group.id, names.join(", ")) - }) + .map(|group| format!("{} ({})", group.id, listed_names(&group.names))) .collect(); let detail = if shown.is_empty() { String::new() @@ -156,6 +142,36 @@ pub(in crate::units) fn outline_wording( } } +/// The first six of `names` in backticks, and how many more there are. +fn listed_names(names: &[String]) -> String { + let shown: Vec<_> = names.iter().take(6).map(|n| format!("`{n}`")).collect(); + match names.len().saturating_sub(shown.len()) { + 0 => shown.join(", "), + more => format!("{} and {more} more", shown.join(", ")), + } +} + +/// A part of a long file that does a job of its own apart from the rest. +pub(in crate::units) fn part_wording( + part: &crate::units::Part, + strength: Strength, + p: f64, +) -> Wording { + let part_named = format!("{} ({} lines)", listed_names(&part.names), part.lines); + match strength { + Strength::Note => ( + format!("{part_named} could live in a module of their own ({p:.2})."), + "Optional: move those members when you next change them", + ), + _ => ( + format!( + "{part_named} do a job of their own apart from the rest of this file ({p:.2})." + ), + "Consider moving those members into a module of their own", + ), + } +} + /// `sites` is (within one test, owner in test code, every site in a test case). pub(in crate::units) fn pair_wording( name: &str, diff --git a/src/units/wording/mod.rs b/src/units/wording/mod.rs index 67357cd..d95786b 100644 --- a/src/units/wording/mod.rs +++ b/src/units/wording/mod.rs @@ -20,7 +20,9 @@ pub(super) use documentation::{ comment_reason, comment_wording, doc_pair_wording, document_wording, plan_wording, section_wording, stale_wording, }; -pub(super) use maintainability::{function_wording, outline_wording, pair_wording, values_wording}; +pub(super) use maintainability::{ + function_wording, outline_wording, pair_wording, part_wording, values_wording, +}; pub(super) use security::{handler_wording, module_wording, privilege_wording, security_wording}; pub(super) use test_rules::{law_wording, test_pair_wording, test_wording};