From fe4455bfd8ef16e2045c99d5b8e75fa3de786a41 Mon Sep 17 00:00:00 2001 From: Dmitriy Date: Tue, 22 Sep 2026 04:57:24 +0300 Subject: [PATCH 1/4] fix(history): make --limit select the newest commits and --since work `--limit N` streamed the N OLDEST commits, inherited from the retired Go reference's `initStreamingIterator` (`logOpts.Reverse = true` + CollectN). Measured on the pinned reference binary: `--limit 3` on kubernetes emits `2c4b3a562ce3` (2014-06-06) -- the repository's first commits -- and the same bytes are frozen in tests/golden/run/burndown.ndjson. No user means "the ten first commits ever made" when they type `--limit 10`. A partial `--since` window was worse: the planner counted the in-window commits, then the oldest-first loading pass applied the author-time stop filter and stopped at the first too-old commit, so the run aborted with `expected N commits, got 0: EOF`, empty stdout, exit 1 -- the documented `--since 2025-01-01` path never worked. Both bounds are now taken from ONE newest-first pass (cap at `--limit`, then reverse to the chronological order the analyzers require), which deletes the planner/loader two-pass and the abort. `--since` additionally gains git revision specs (`v1.2.0`, `HEAD~100`, a SHA) and CODEFANG_NOW-aware durations by sharing cf-gitlib's single parser; the CLI's duplicate ~130-line parser is deleted, not adapted. An unresolvable spec now reports `Error: cannot resolve --since ""` instead of the generic tier-8 dispatch placeholder (same rc=1, still no stdout). Commits are still delivered oldest-first, so tick, burndown line survival and identity semantics are unchanged -- only the selected window moves. This is a deliberate divergence from the retired reference: superseded `--limit`-bounded captures are annotated (not recaptured) in tests/golden/MANIFEST.json and tests/compat/matrix.toml. The `cf-gitlib` `testutil` feature its module docs already promised makes the deterministic fixture repo usable from these tests. See specs/frds/FRD-newest-commit-window.md. Tests: 10 unit (`history_window_tests`) + 7 CLI-level e2e (`bins/codefang/tests/history_window.rs`) driving the real binary over a real repo, incl. the `--head` == `--limit 1` byte-identity invariant. Written red-first against the old behavior. `make test`: 2455 passed, 0 failed. --- CHANGELOG.md | 48 ++ Cargo.lock | 1 + bins/codefang/Cargo.toml | 5 + bins/codefang/tests/history_window.rs | 212 ++++++++ crates/cf-commands/Cargo.toml | 3 + crates/cf-commands/src/handlers.rs | 505 ++++++++++-------- .../src/handlers/burndown_ndjson.rs | 25 +- .../cf-commands/src/handlers/couples_run.rs | 7 +- crates/cf-commands/src/handlers/history.rs | 83 ++- crates/cf-commands/src/handlers/uast_walk.rs | 7 +- crates/cf-commands/src/lib.rs | 21 + crates/cf-gitlib/Cargo.toml | 6 + crates/cf-gitlib/src/lib.rs | 2 +- llms-full.txt | 35 +- site/getting-started/first-analysis.md | 6 +- site/guide/cli-reference.md | 35 +- site/guide/data-analytics.md | 19 +- tests/compat/matrix.toml | 6 + tests/golden/MANIFEST.json | 3 +- 19 files changed, 709 insertions(+), 320 deletions(-) create mode 100644 bins/codefang/tests/history_window.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 58143923..e357fb94 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,54 @@ The format follows [Keep a Changelog](https://keepachangelog.com/). --- +## [Unreleased] — `--limit` selects the newest commits; `--since` works + +### Changed + +- **`--limit N` now analyzes the N NEWEST commits** (the `git log -n` + convention) instead of the N oldest. Commits are still delivered to the + analyzers oldest-first, so tick, burndown and identity semantics are + unchanged — only the selected window moves. This is a deliberate + divergence from the retired Go reference, which selected the oldest + window (measured: `--limit 3` on kubernetes emitted `2c4b3a562ce3`, its + first commits). `specs/frds/FRD-newest-commit-window.md`. +- **`--since` with a cutoff in the middle of the history now works.** It + previously planned a non-zero commit count, loaded zero (the oldest-first + loader stopped at the first too-old commit) and aborted with + `expected N commits, got 0: EOF`, empty stdout and exit 1. A cutoff newer + than every commit still yields the empty report with exit 0. +- **`--since` now accepts a git revision** (`v1.2.0`, `HEAD~100`, a SHA): + that commit's author time becomes the cutoff. Duration forms now honor the + `CODEFANG_NOW` test clock, and the CLI's duplicate time parser was deleted in + favour of the single `cf-gitlib` one. +- **A bad `--since` value says so**: `Error: cannot resolve --since ""` + instead of the generic `command dispatch is blocked` placeholder (same exit + code, still no stdout). + +### Fixed + +- `site/guide/data-analytics.md` told users to run `--since 6m` for "the last + 6 months"; `m` is minutes, so that meant 6 minutes. Docs now state the + supported units (`h`/`m`/`s`) and give `4380h`. +- `cf_gitlib::testutil` documented a `testutil` feature that did not exist, so + no other crate could build a deterministic fixture repo. The feature exists. +- `make lint` and `make deadcode` are implemented — `AGENTS.md` mandated both + gates, but neither target existed (and `cargo clippy` was failing outright on + deny-by-default lints in `cf-reportutil`, `cf-gojson` and `cf-goyaml`). Both + are clean now. +- Removed the never-read `InitState.rule_index` field in `cf-uast` (it pinned a + HashMap of rule names per parser) and cleared the workspace clippy backlog. + +### Added + +- `crates/cf-commands/src/handlers.rs`: `history_window_tests` — window + contract (newest-N, empty/partial/full `--since`, `--limit` inside a window, + revision spec, injected clock, unresolvable spec). +- `bins/codefang/tests/history_window.rs`: CLI-level e2e driving the real + binary over a real git repo, including the `--head` ≡ `--limit 1` invariant. + +--- + ## [Unreleased] — Repo hygiene & race fix ### Fixed diff --git a/Cargo.lock b/Cargo.lock index b2559b53..22cb9162 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -878,6 +878,7 @@ name = "codefang" version = "0.0.0" dependencies = [ "cf-commands", + "cf-gitlib", "clap", "git2", ] diff --git a/bins/codefang/Cargo.toml b/bins/codefang/Cargo.toml index 990fb294..d6f7d361 100644 --- a/bins/codefang/Cargo.toml +++ b/bins/codefang/Cargo.toml @@ -33,3 +33,8 @@ git2 = { workspace = true } # thin shell that bootstraps malloc tunables + the --profile watchdog, then calls # cf_commands::run(argv). cf-commands = { path = "../../crates/cf-commands" } + +[dev-dependencies] +# Deterministic commit fixtures for the CLI-level window tests (feature-gated: +# compiled for tests only, never in a shipped binary). +cf-gitlib = { path = "../../crates/cf-gitlib", features = ["testutil"] } diff --git a/bins/codefang/tests/history_window.rs b/bins/codefang/tests/history_window.rs new file mode 100644 index 00000000..c621e5b2 --- /dev/null +++ b/bins/codefang/tests/history_window.rs @@ -0,0 +1,212 @@ +//! End-to-end coverage of the `--limit` / `--since` commit window, driven +//! through the **real binary** over a real on-disk git repository. +//! +//! The window contract (see `specs/frds/FRD-newest-commit-window.md`): +//! `--limit N` takes the N NEWEST commits, `--since C` takes every commit with +//! author time >= C, and both are delivered to the analyzers oldest-first. +//! Reports are asserted in machine formats only (`ndjson` / `json`), which are +//! the byte-stable formats the golden harness also treats as binding. + +use cf_gitlib::testutil::{commit_files, init_repo, TestRepo}; +use cf_gitlib::Hash; + +/// Path to the freshly built `codefang` binary under test. +const BIN: &str = env!("CARGO_BIN_EXE_codefang"); +/// Base instant of the fixture history: 2021-01-01T00:00:00Z. +const T0: i64 = 1_609_459_200; +/// One day between fixture commits, so every cutoff lands between two commits. +const DAY: i64 = 86_400; +/// Fixture length: five linear commits; `hashes[0]` is the root. +const N_COMMITS: usize = 5; +/// Cutoff naming the 3rd commit's instant; commits 1-2 fall outside it. +const CUTOFF_3RD: &str = "2021-01-03T00:00:00Z"; +/// A newer-than-HEAD cutoff: excludes the whole history. +const CUTOFF_FUTURE: &str = "2030-01-01"; + +/// The ndjson key that carries a commit hash, and the byte offset of the hash +/// value behind it (`"hash":"` is 8 bytes, a SHA-1 is 40). +const NEEDLE_HASH: &str = "\"hash\":\""; + +/// Runs the binary with a deterministic environment; state is disabled so the +/// run is self-contained (same discipline as the golden captures). +fn codefang(args: &[&str]) -> std::process::Output { + Command::new(BIN) + .args(["run", "--checkpoint=false", "--resume=false", "--no-cache"]) + .args(args) + .env("TZ", "UTC") + .env("NO_COLOR", "1") + .env("LANG", "C") + .env("LC_ALL", "C") + .output() + .expect("run the codefang binary") +} + +use std::process::Command; + +/// Builds the five-commit fixture; returns it with its hashes, oldest-first. +fn fixture() -> (TestRepo, Vec) { + let test = init_repo().expect("init fixture repo"); + let mut hashes = Vec::new(); + for i in 0..N_COMMITS { + let h = commit_files( + &test, + &format!("commit {i}"), + T0 + i64::try_from(i).expect("index fits i64") * DAY, + &[("f.txt", format!("content v{i}\n").as_bytes())], + ) + .expect("fixture commit"); + hashes.push(h); + } + (test, hashes) +} + +/// The commit hashes of a `burndown --format ndjson` run, in emitted order, +/// together with `(exit code, stdout, stderr)`. +fn ndjson_window(path: &str, extra: &[&str]) -> (i32, Vec, String, String) { + let out = codefang( + &[ + &["-a", "history/burndown", "--workers", "1", "--format", "ndjson"], + extra, + &[path], + ] + .concat(), + ); + let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); + let hashes = stdout + .lines() + .filter_map(|line| line.find(NEEDLE_HASH).map(|i| line[i + 8..i + 48].to_owned())) + .collect(); + ( + out.status.code().unwrap_or(-1), + hashes, + stdout, + String::from_utf8_lossy(&out.stderr).into_owned(), + ) +} + +/// `--limit N` selects the N NEWEST commits, delivered oldest-first. +#[test] +fn limit_selects_the_newest_commits() { + let (test, hashes) = fixture(); + let path = test.path().to_str().expect("utf-8 path"); + let (rc, got, stdout, stderr) = ndjson_window(path, &["--limit", "2"]); + assert_eq!(rc, 0, "run failed: {stderr}"); + assert_eq!( + got, + vec![hashes[3].to_string(), hashes[4].to_string()], + "--limit 2 must take the two NEWEST commits oldest-first, got {got:?} in {stdout}" + ); +} + +/// The user-visible commit count follows `--limit` and grows with it. +#[test] +fn limit_bound_shows_in_the_report() { + let (test, _hashes) = fixture(); + let path = test.path().to_str().expect("utf-8 path"); + for (limit, want) in [("1", 1), ("2", 2), ("5", 5)] { + let out = codefang(&[ + "-a", "history/devs", "--limit", limit, "--format", "json", path, + ]); + assert!(out.status.success(), "run failed for --limit {limit}"); + let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); + let needle = format!(r#""total_commits":{want}"#); + assert!( + stdout.contains(&needle), + "--limit {limit} must report {want} commits ({needle}):\n{stdout}" + ); + } +} + +/// A `--since` cutoff in the MIDDLE of the history yields exactly that window. +/// This is the case the reference-parity loader aborted on with empty stdout. +#[test] +fn since_partial_window_succeeds_with_the_window() { + let (test, hashes) = fixture(); + let path = test.path().to_str().expect("utf-8 path"); + let (rc, got, _stdout, stderr) = ndjson_window(path, &["--since", CUTOFF_3RD]); + assert_eq!(rc, 0, "a partial --since window must exit 0, stderr: {stderr}"); + assert_eq!( + got, + vec![ + hashes[2].to_string(), + hashes[3].to_string(), + hashes[4].to_string() + ], + "--since must analyze exactly the in-window commits" + ); +} + +/// `--since` also accepts a git revision. +#[test] +fn since_accepts_a_revision() { + let (test, hashes) = fixture(); + let path = test.path().to_str().expect("utf-8 path"); + let (rc, got, _stdout, stderr) = ndjson_window(path, &["--since", "HEAD~2"]); + assert_eq!(rc, 0, "run failed: {stderr}"); + assert_eq!( + got.first().map(String::as_str), + Some(hashes[2].to_string().as_str()), + "`--since HEAD~2` must start the window at that commit" + ); + assert_eq!(got.len(), 3, "window must hold HEAD~2..HEAD"); +} + +/// A cutoff newer than every commit analyzes nothing but still succeeds. +#[test] +fn since_after_head_is_empty_and_succeeds() { + let (test, _hashes) = fixture(); + let path = test.path().to_str().expect("utf-8 path"); + let (rc, got, _stdout, stderr) = ndjson_window(path, &["--since", CUTOFF_FUTURE]); + assert_eq!(rc, 0, "an empty window is not an error: {stderr}"); + assert!(got.is_empty(), "no commit is in window, got {got:?}"); +} + +/// An unresolvable `--since` aborts with empty stdout, exit 1, and a message +/// that names the flag rather than the internal dispatch placeholder. +#[test] +fn unresolvable_since_fails_with_a_named_diagnostic() { + let (test, _hashes) = fixture(); + let path = test.path().to_str().expect("utf-8 path"); + let (rc, _got, stdout, stderr) = ndjson_window(path, &["--since", "not-a-time-or-ref"]); + assert_eq!(rc, 1, "an unresolvable --since must exit 1"); + assert!(stdout.is_empty(), "stdout must stay empty, got {stdout}"); + assert!( + stderr.contains("cannot resolve --since"), + "the error must name the flag, stderr: {stderr}" + ); + assert!( + !stderr.contains("dispatch is blocked"), + "the failure must not be misattributed to the port, stderr: {stderr}" + ); +} + +/// `--head` means "exactly HEAD", so it must equal `--limit 1` (which now +/// selects HEAD) and must not be widened by a larger `--limit`. +#[test] +fn head_overrides_limit() { + let (test, _hashes) = fixture(); + let path = test.path().to_str().expect("utf-8 path"); + let report = |extra: &[&str]| { + let out = codefang( + &[ + &["-a", "history/burndown", "--format", "json"], + extra, + &[path], + ] + .concat(), + ); + assert!(out.status.success(), "run failed: {:?}", String::from_utf8_lossy(&out.stderr)); + out.stdout + }; + let head = report(&["--head"]); + let newest_one = report(&["--limit", "1"]); + assert_eq!( + head, newest_one, + "--limit 1 must select exactly HEAD, so both reports must be identical bytes" + ); + assert_eq!( + head, + report(&["--head", "--limit", "3"]), + "--head must ignore --limit" + ); +} diff --git a/crates/cf-commands/Cargo.toml b/crates/cf-commands/Cargo.toml index 8463a6bc..0e94e31b 100644 --- a/crates/cf-commands/Cargo.toml +++ b/crates/cf-commands/Cargo.toml @@ -117,3 +117,6 @@ tempfile = "3" # tempdirs (history-filter oracle tests) — write access comes straight from # git2, the same libgit2 vendoring cf-gitlib is pinned to. git2 = { workspace = true } +# Deterministic commit fixtures for the history-window tests (feature-gated, so +# it is compiled for tests only). +cf-gitlib = { path = "../cf-gitlib", features = ["testutil"] } diff --git a/crates/cf-commands/src/handlers.rs b/crates/cf-commands/src/handlers.rs index 89671918..85793208 100644 --- a/crates/cf-commands/src/handlers.rs +++ b/crates/cf-commands/src/handlers.rs @@ -221,197 +221,74 @@ pub fn effective_first_parent(sub: &clap::ArgMatches) -> bool { history_ids.iter().any(|id| id == "history/burndown") } -/// The parsed `--since` state of a history run (reference `--since` parity). +/// The `--since` state of a history run: raw spec text, resolved against the +/// repository when the walk is built. /// -/// The reference pipeline resolves `--since` BEFORE planning the streaming -/// chunks, and the observable stdout contract has three measured classes -/// (oracle-verified against the live reference binary on hercules): +/// Accepted forms, in precedence order: +/// 1. a Go-style duration (`24h`, `1h30m`) subtracted from "now" — units are +/// `h`/`m`/`s` only, so `m` means MINUTES and there is no `d`/`w` unit; +/// "now" honors `CODEFANG_NOW` so duration-relative runs stay reproducible +/// (DESIGN §2.8); +/// 2. an RFC3339 timestamp (`2024-01-01T00:00:00Z`, numeric offsets honored); +/// 3. a plain UTC date (`2024-01-01`, interpreted as midnight UTC); +/// 4. any git revision (`v1.2.0`, `HEAD~100`, a bare SHA) — the referenced +/// commit's AUTHOR time becomes the cutoff. /// -/// - **cutoff at/before the oldest commit** (filter excludes nothing) — output -/// is byte-identical to a run without `--since`; -/// - **cutoff after every commit** (filter excludes everything, e.g. -/// `--since 2030-01-01` or a duration like `24h`) — the planner plans ZERO -/// chunks and the run succeeds with each analyzer's empty-walk report; -/// - **partial filter** (some commits pass, some don't) — the planner counts -/// the passing commits, but the oldest-first loading iterator stops at the -/// FIRST commit older than the cutoff (the reference `Since` stop filter -/// composed with the reversed walk), yields zero commits, and the run aborts -/// (`expected N commits, got 0: EOF`) with EMPTY stdout and exit 1. +/// Commits whose author time is at/after the cutoff are analyzed. A spec that +/// matches none of the forms aborts the run with empty stdout and exit 1; the +/// `run` command names the flag (`Error: cannot resolve --since ""`) +/// before any walking starts. /// -/// An unparseable value aborts before walking (reference: `Error: invalid time -/// format for --since`), also with empty stdout. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +/// Parsing lives in [`cf_gitlib::helpers::parse_time`] / +/// [`cf_gitlib::Repository::resolve_time`] — ONE parser shared by the CLI and +/// the library, instead of a second CLI-only copy. +#[derive(Debug, Clone, PartialEq, Eq)] pub enum SinceSpec { /// `--since` absent or empty: the walk is unfiltered. Inactive, - /// Cutoff in Unix seconds; commits with author time >= cutoff pass - /// (the reference iterator's stop comparison uses the AUTHOR clock). - Active(i64), - /// Unparseable `--since` value: the run aborts with empty stdout. - Invalid, + /// Raw `--since` text, resolved against the repository at walk time. + Active(String), } -/// Resolves the run's `--since` flag into a [`SinceSpec`]. +/// Resolves the run's `--since` flag into a [`SinceSpec`] (empty ⇒ inactive). #[must_use] pub fn history_since_spec(sub: &clap::ArgMatches) -> SinceSpec { - let raw = sub.get_one::("since").map_or("", String::as_str); - if raw.is_empty() { - return SinceSpec::Inactive; + match sub.get_one::("since") { + Some(raw) if !raw.is_empty() => SinceSpec::Active(raw.clone()), + _ => SinceSpec::Inactive, } - match parse_since_time(raw) { - Some(secs) => SinceSpec::Active(secs), - None => SinceSpec::Invalid, - } -} - -/// Parses the reference `--since` value forms: RFC3339 -/// (`2006-01-02T15:04:05Z07:00`), a plain UTC date (`2006-01-02`), or a Go -/// duration (`24h`, `1h30m`, …) subtracted from the current wall clock. -/// Returns the cutoff in Unix seconds, or `None` when unparseable. -fn parse_since_time(raw: &str) -> Option { - if let Some(secs) = parse_rfc3339_secs(raw) { - return Some(secs); - } - if let Some(secs) = parse_utc_date_secs(raw) { - return Some(secs); - } - if let Some(dur_secs) = parse_go_duration_secs(raw) { - let now = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .ok()?; - return Some(i64::try_from(now.as_secs()).unwrap_or(i64::MAX) - dur_secs); - } - None -} - -/// Days since the Unix epoch for a civil date (Howard Hinnant's algorithm; -/// the inverse of [`civil_from_days`]). -fn days_from_civil(y: i64, m: u32, d: u32) -> i64 { - let y = if m <= 2 { y - 1 } else { y }; - let era = if y >= 0 { y } else { y - 399 } / 400; - let yoe = y - era * 400; - let mp = i64::from((m + 9) % 12); - let doy = (153 * mp + 2) / 5 + i64::from(d) - 1; - let doe = yoe * 365 + yoe / 4 - yoe / 100 + doy; - era * 146_097 + doe - 719_468 } -/// Parses `YYYY-MM-DD` as UTC midnight (Go `time.Parse("2006-01-02", …)`). -fn parse_utc_date_secs(s: &str) -> Option { - let b = s.as_bytes(); - if b.len() != 10 || b[4] != b'-' || b[7] != b'-' { - return None; - } - let y: i64 = s.get(0..4)?.parse().ok()?; - let m: u32 = s.get(5..7)?.parse().ok()?; - let d: u32 = s.get(8..10)?.parse().ok()?; - if !(1..=12).contains(&m) || !(1..=31).contains(&d) { - return None; - } - Some(days_from_civil(y, m, d) * 86_400) -} - -/// Parses an RFC3339 timestamp (`YYYY-MM-DDTHH:MM:SS[.frac](Z|±HH:MM)`) to -/// Unix seconds (fractional seconds truncated, exactly as a seconds-granularity -/// git comparison observes them). -fn parse_rfc3339_secs(s: &str) -> Option { - let b = s.as_bytes(); - if b.len() < 20 || b[10] != b'T' || b[13] != b':' || b[16] != b':' { - return None; - } - let date_secs = parse_utc_date_secs(s.get(0..10)?)?; - let hh: i64 = s.get(11..13)?.parse().ok()?; - let mm: i64 = s.get(14..16)?.parse().ok()?; - let ss: i64 = s.get(17..19)?.parse().ok()?; - if hh > 23 || mm > 59 || ss > 60 { - return None; - } - // Skip fractional seconds, then parse the zone designator. - let mut i = 19; - if b.get(i) == Some(&b'.') { - i += 1; - let start = i; - while i < b.len() && b[i].is_ascii_digit() { - i += 1; - } - if i == start { - return None; - } - } - let zone = s.get(i..)?; - let offset_secs: i64 = if zone == "Z" || zone == "z" { - 0 - } else { - let zb = zone.as_bytes(); - if zb.len() != 6 || (zb[0] != b'+' && zb[0] != b'-') || zb[3] != b':' { - return None; - } - let oh: i64 = zone.get(1..3)?.parse().ok()?; - let om: i64 = zone.get(4..6)?.parse().ok()?; - let sign = if zb[0] == b'-' { -1 } else { 1 }; - sign * (oh * 3600 + om * 60) - }; - Some(date_secs + hh * 3600 + mm * 60 + ss - offset_secs) -} - -/// Parses a Go `time.ParseDuration` string (`300s`, `1h30m`, `24h`, …) to -/// whole seconds (sub-second remainder truncated). -fn parse_go_duration_secs(s: &str) -> Option { - let mut rest = s; - let mut total_ns: f64 = 0.0; - let mut any = false; - if rest.starts_with('-') || rest.starts_with('+') { - return None; // negative/signed durations never select commits meaningfully - } - while !rest.is_empty() { - let num_len = rest - .find(|c: char| !(c.is_ascii_digit() || c == '.')) - .unwrap_or(rest.len()); - if num_len == 0 { - return None; - } - let value: f64 = rest.get(0..num_len)?.parse().ok()?; - rest = rest.get(num_len..)?; - let (unit_ns, unit_len) = if rest.starts_with("ns") { - (1.0, 2) - } else if rest.starts_with("us") || rest.starts_with("µs") { - (1e3, if rest.starts_with("µs") { 3 } else { 2 }) - } else if rest.starts_with("ms") { - (1e6, 2) - } else if rest.starts_with('s') { - (1e9, 1) - } else if rest.starts_with('m') { - (6e10, 1) - } else if rest.starts_with('h') { - (3.6e12, 1) - } else { - return None; - }; - total_ns += value * unit_ns; - rest = rest.get(unit_len..)?; - any = true; - } - if !any { - return None; - } - #[allow(clippy::cast_possible_truncation)] // contractual truncation to seconds - Some((total_ns / 1e9) as i64) -} - -/// Replicates the reference implementation `initHistoryPipeline` (the iterator path the real -/// `run` command uses — NOT `gitlib.LoadCommits`): walks history oldest-first -/// (`SortTime|SortTopological|SortReverse`) and feeds the analyzer the FIRST -/// `commitCount = min(limit, total)` commits. That selects the N OLDEST -/// reachable commits, oldest-first (oracle-verified against the live reference binary — -/// `--limit 20` on hercules yields the repo's first 20 commits, with ascending -/// composition ticks). `limit <= 0` returns the full oldest-first history. +/// The commit window selected by `--limit` / `--since` / `--first-parent`. +/// +/// Returns hashes in the **oldest-first** order the history analyzers consume +/// (tick assignment, burndown line survival and identity ids are all +/// order-dependent). Window semantics: +/// - `--limit N` → the N **newest** reachable commits (the `git log -n` +/// convention); `limit <= 0` → the whole walk; +/// - `--since C` → every commit with author time >= C. A cutoff after HEAD +/// yields an EMPTY window (analyzers emit their empty report); a cutoff at +/// or before the root commit filters nothing; +/// - both → the newest `N` commits *inside* the `--since` window. /// -/// `since` applies the reference `--since` contract (see [`SinceSpec`]): the -/// planner counts the commits at/after the cutoff (newest-first walk with the -/// stop filter), a zero count yields an EMPTY walk (each analyzer's empty -/// report), and a partial filter aborts (`None` ⇒ empty stdout) because the -/// reference oldest-first loading iterator stops at the first too-old commit -/// and under-fills the planned chunk. +/// Both bounds come from ONE newest-first pass: the walk is sorted +/// `TIME|TOPOLOGICAL` newest-first, so [`cf_gitlib::CommitIter`]'s author-time +/// *stop* filter cuts exactly at the cutoff and `limit` caps that prefix. +/// +/// KNOWN RISK (inherited, not introduced here): the walk is ordered by +/// committer time while the filter compares AUTHOR time, so a history whose +/// author dates are skewed against its commit dates (bulk `git commit --date` +/// rewrites) can be cut short. Mitigation if it ever bites: turn +/// `CommitIter`'s stop filter into a skip filter, which costs a full walk. +/// +/// DIVERGENCE (deliberate — see `specs/frds/FRD-newest-commit-window.md`): the +/// retired Go reference selected the N OLDEST commits and aborted with empty +/// stdout on a *partial* `--since` window. Both are corrected here; captures +/// bounded by `--limit` against that binary no longer apply. +/// +/// Returns `None` (⇒ the run aborts with empty stdout) when the repository +/// cannot be walked or `--since` cannot be resolved against it — the CLI +/// pre-checks the latter, so this is the library-level contract. #[must_use] pub fn load_history_commit_hashes( repo: &cf_gitlib::Repository, @@ -419,68 +296,19 @@ pub fn load_history_commit_hashes( first_parent: bool, since: SinceSpec, ) -> Option> { - use cf_gitlib::repository::LogOptions; - - match since { - SinceSpec::Invalid => return None, - SinceSpec::Active(cutoff) => { - let since_time = Some(cf_gitlib::repository::time_from_unix_secs(cutoff)); - // Planner count: newest-first walk with the reference stop filter. - let plan_opts = LogOptions { - since: since_time, - first_parent, - reverse: false, - }; - let mut plan_iter = repo.log(&plan_opts).ok()?; - let mut n_since: i64 = 0; - while plan_iter.next_commit().is_some() { - n_since += 1; - } - if n_since == 0 { - // Zero chunks planned: the run succeeds over an empty walk. - return Some(Vec::new()); - } - let expected = if limit > 0 { - limit.min(n_since) - } else { - n_since - }; - // Loading: the reference oldest-first iterator applies the SAME stop - // filter, so it ends at the first commit older than the cutoff. - let load_opts = LogOptions { - since: since_time, - first_parent, - reverse: true, - }; - let mut iter = repo.log(&load_opts).ok()?; - let mut hashes = Vec::new(); - while (hashes.len() as i64) < expected { - match iter.next_commit() { - Some(c) => hashes.push(c.hash()), - None => break, - } - } - if (hashes.len() as i64) < expected { - // Under-filled chunk: the reference pipeline aborts with empty stdout - // ("expected N commits, got M: EOF"). - return None; - } - return Some(hashes); - } - SinceSpec::Inactive => {} - } - // ORACLE-VERIFIED window selection. The real `run` command uses - // the reference `initStreamingIterator`, which sets `logOpts.Reverse = true` - // (oldest-first walk) and then streams the FIRST `commitCount = - // min(limit, total)` commits — i.e. the `limit` OLDEST reachable commits, - // oldest-first. (NOT `gitlib.loadHistoryCommits`'s newest-N+reverse: the live - // reference binary at `--limit 2` on hercules emits the repo's first two commits — - // analyser.go/LICENSE — proving the OLDEST set is selected, even though the - // repo has 1006 commits.) Do NOT switch to `reverse: false` + post-reverse. + use cf_gitlib::repository::{time_from_unix_secs, LogOptions}; + + // Resolve the spec against THIS repository so revisions are usable and + // duration forms honor CODEFANG_NOW (shared parser; see [`SinceSpec`]). + let since_secs = match since { + SinceSpec::Inactive => None, + SinceSpec::Active(spec) => Some(repo.resolve_time(&spec).ok()?), + }; + let log_opts = LogOptions { - reverse: true, + since: since_secs.map(time_from_unix_secs), first_parent, - ..LogOptions::default() + reverse: false, }; let mut iter = repo.log(&log_opts).ok()?; let mut hashes = Vec::new(); @@ -490,9 +318,11 @@ pub fn load_history_commit_hashes( None => break, } } + hashes.reverse(); Some(hashes) } + /// Returns the reference implementation's streaming-pipeline *consume* order for an oldest-first commit /// window: the IDENTITY (oldest-first revwalk order). /// @@ -2668,3 +2498,204 @@ mod history_filter_tests { assert_eq!(empty_commits, 0); } } + +#[cfg(test)] +mod history_window_tests { + use super::*; + use cf_gitlib::helpers::parse_time; + use cf_gitlib::testutil::{commit_files, init_repo, TestRepo}; + use cf_gitlib::Hash; + + /// Base instant of the fixture history: 2021-01-01T00:00:00Z. + const T0: i64 = 1_609_459_200; + /// Spacing between fixture commits (one hour), so author-time order is + /// strictly increasing and every cutoff can land between two commits. + const STEP: i64 = 3_600; + /// Fixture length: five linear commits; `hashes[0]` is the oldest. + const N_COMMITS: usize = 5; + /// The 3rd commit's instant as an RFC3339 cutoff (T0 + 2*STEP), i.e. + /// `2021-01-01T02:00:00Z`; excluded from a window whose cutoff equals it. + const CUTOFF_3RD: &str = "2021-01-01T02:00:00Z"; + /// The newest commit's instant as a UTC date (2021-01-01T04:00:00Z). + const DATE_AFTER_HEAD: &str = "2021-01-02"; + /// A date at/before the fixture root, so `--since` filters nothing. + const DATE_BEFORE_ROOT: &str = "2020-01-01"; + /// Restores `CODEFANG_NOW` on scope exit so an injected clock cannot leak + /// into a sibling test thread. + struct NowGuard; + + impl Drop for NowGuard { + fn drop(&mut self) { + std::env::remove_var("CODEFANG_NOW"); + } + } + + /// Builds the five-commit linear fixture; returns the repo and its commit + /// hashes in oldest-first order. + fn linear_repo() -> (TestRepo, Vec) { + let test = init_repo().expect("init fixture repo"); + let mut hashes = Vec::new(); + for i in 0..N_COMMITS { + let h = commit_files( + &test, + &format!("commit {i}"), + T0 + i64::try_from(i).expect("index fits i64") * STEP, + &[("f.txt", format!("v{i}").as_bytes())], + ) + .expect("fixture commit"); + hashes.push(h); + } + (test, hashes) + } + + /// `--limit N` selects the N NEWEST commits, delivered oldest-first. + #[test] + fn limit_selects_the_newest_commits_oldest_first() { + let (test, hashes) = linear_repo(); + let got = + load_history_commit_hashes(&test.repo, 2, false, SinceSpec::Inactive).expect("loads"); + assert_eq!( + got.as_slice(), + &hashes[3..], + "--limit 2 must be the two NEWEST commits, oldest-first" + ); + } + + /// `--limit 0` (unset) and a negative limit are both "no limit". + #[test] + fn limit_zero_or_negative_loads_whole_history() { + let (test, hashes) = linear_repo(); + for limit in [0, -1] { + let got = load_history_commit_hashes(&test.repo, limit, false, SinceSpec::Inactive) + .unwrap_or_else(|| panic!("limit {limit} loads")); + assert_eq!(got.as_slice(), &hashes[..], "limit {limit} must load all commits"); + } + } + + /// The cutoff literal must mean exactly the 3rd fixture instant, or every + /// cutoff-based expectation below drifts silently. + #[test] + fn cutoff_literal_matches_the_fixture_clock() { + assert_eq!( + parse_time(CUTOFF_3RD).expect("cutoff parses"), + T0 + 2 * STEP, + "CUTOFF_3RD must name the 3rd commit's instant" + ); + } + + /// A `--since` cutoff that excludes only part of the history yields exactly + /// that window oldest-first. (The reference-parity loader aborted here.) + #[test] + fn since_partial_window_yields_the_window() { + let (test, hashes) = linear_repo(); + let got = load_history_commit_hashes( + &test.repo, + 0, + false, + SinceSpec::Active(CUTOFF_3RD.to_owned()), + ) + .expect("a partial --since window must load, not abort"); + assert_eq!( + got.as_slice(), + &hashes[2..], + "--since must yield the in-window commits, oldest-first" + ); + } + + /// A cutoff newer than every commit plans an EMPTY window (analyzers then + /// emit their empty report) rather than aborting. + #[test] + fn since_cutoff_after_head_yields_empty_window() { + let (test, _hashes) = linear_repo(); + let got = load_history_commit_hashes( + &test.repo, + 0, + false, + SinceSpec::Active(DATE_AFTER_HEAD.to_owned()), + ) + .expect("a fully-excluded window is empty, not an error"); + assert!(got.is_empty(), "window must be empty, got {got:?}"); + } + + /// A cutoff at/before the root commit filters nothing: same window, same + /// order as an unfiltered run. + #[test] + fn since_cutoff_at_or_before_root_yields_whole_history() { + let (test, hashes) = linear_repo(); + let got = load_history_commit_hashes( + &test.repo, + 0, + false, + SinceSpec::Active(DATE_BEFORE_ROOT.to_owned()), + ) + .expect("loads"); + assert_eq!(got.as_slice(), &hashes[..], "cutoff before root filters nothing"); + } + + /// `--limit` caps the NEWEST end of the `--since` window: the window is + /// `hashes[1..]`, so the newest two of those are `hashes[3..]`. + #[test] + fn limit_caps_the_newest_end_of_the_since_window() { + let (test, hashes) = linear_repo(); + let cutoff = CUTOFF_3RD.to_owned(); + let unbounded = + load_history_commit_hashes(&test.repo, 0, false, SinceSpec::Active(cutoff.clone())) + .expect("window loads"); + assert_eq!(unbounded.as_slice(), &hashes[2..]); + let capped = + load_history_commit_hashes(&test.repo, 2, false, SinceSpec::Active(cutoff)) + .expect("window loads"); + assert_eq!( + capped.as_slice(), + &hashes[3..], + "--limit inside a --since window keeps its newest commits" + ); + } + + /// `--since` accepts a git revision, using that commit's author time. + #[test] + fn since_accepts_a_git_revision() { + let (test, hashes) = linear_repo(); + let got = load_history_commit_hashes(&test.repo, 0, false, SinceSpec::Active("HEAD~2".to_owned())) + .expect("--since must resolve a revision like `HEAD~2`"); + assert_eq!( + got.as_slice(), + &hashes[2..], + "`--since HEAD~2` must start the window at that commit" + ); + } + + /// Duration forms resolve against the injectable clock (DESIGN §2.8), not + /// the wall clock. The fixture dates are years behind the real clock, so + /// this window can only appear if `CODEFANG_NOW` is honored. + #[test] + fn duration_since_uses_the_injected_clock() { + let _guard = NowGuard; + // Now == the newest-but-one commit's instant; `1h` back == CUTOFF_3RD. + std::env::set_var("CODEFANG_NOW", (T0 + 3 * STEP).to_string()); + let (test, hashes) = linear_repo(); + let got = load_history_commit_hashes(&test.repo, 0, false, SinceSpec::Active("1h".to_owned())) + .expect("`--since 1h` must resolve against CODEFANG_NOW"); + assert_eq!( + got.as_slice(), + &hashes[2..], + "`--since 1h` must cut at CODEFANG_NOW minus one hour" + ); + } + + /// A `--since` that is neither a time form nor a revision aborts the run. + #[test] + fn unresolvable_since_aborts_the_run() { + let (test, _hashes) = linear_repo(); + assert!( + load_history_commit_hashes( + &test.repo, + 0, + false, + SinceSpec::Active("not-a-time-or-ref".to_owned()) + ) + .is_none(), + "an unresolvable --since must yield None (empty stdout, exit 1)" + ); + } +} diff --git a/crates/cf-commands/src/handlers/burndown_ndjson.rs b/crates/cf-commands/src/handlers/burndown_ndjson.rs index ab3a5276..45404223 100644 --- a/crates/cf-commands/src/handlers/burndown_ndjson.rs +++ b/crates/cf-commands/src/handlers/burndown_ndjson.rs @@ -1,11 +1,11 @@ //! `run --analyzers history/burndown --format timeseries --ndjson` closed form. //! //! Reproduces the reference streaming burndown time-series NDJSON pipeline for the -//! oldest `--limit` commits as a deterministic, single-pass computation: +//! newest `--limit` commits as a deterministic, single-pass computation: //! -//! - **commit set / order** — `repository.Log(Reverse=true, FirstParent=true)` -//! (oldest-first `SortTime|SortTopological|SortReverse`; burndown forces -//! `--first-parent` for the whole run), truncated to `--limit` commits. +//! - **commit set / order** — `handlers::load_history_commit_hashes`: the +//! `--limit` NEWEST commits, delivered oldest-first (burndown forces +//! `--first-parent` for the whole run). //! - **tick assignment** (`plumbing.TicksSinceStart`, 24 h default tick) — //! `tick0 = FloorTime(when0, 24h)`; `tick = max(floor((when-tick0)/24h), //! previousTick)` over the committer time. With no identity provider @@ -49,7 +49,7 @@ const TICK_PERIOD: i64 = 86_400; /// reports into. Drained per commit to compute that commit's line stats. type DeltaSink = Rc>>; -/// Builds the burndown timeseries NDJSON bytes for the oldest `--limit` commits, +/// Builds the burndown timeseries NDJSON bytes for the newest `--limit` commits, /// or `None` if the repository cannot be opened/walked. pub fn burndown_timeseries_ndjson(sub: &clap::ArgMatches) -> Option> { let path = run_repo_path(sub); @@ -60,8 +60,7 @@ pub fn burndown_timeseries_ndjson(sub: &clap::ArgMatches) -> Option> { // Burndown forces --first-parent (reference: `if slices.Contains(analyzerKeys, // "burndown") && !opts.FirstParent { opts.FirstParent = true }`), so the walk // follows only the first parent of merge commits (simplify_first_parent). - // The window is the `limit` NEWEST commits processed oldest-first (reference: - // `gitlib.loadHistoryCommits`), NOT the `limit` oldest. + // The window is the `limit` NEWEST commits processed oldest-first. let hashes = crate::handlers::load_history_commit_hashes( &repo, limit, @@ -233,7 +232,7 @@ pub fn burndown_timeseries_contribution( }) } -/// Builds the burndown **record** NDJSON bytes for the oldest `--limit` commits +/// Builds the burndown **record** NDJSON bytes for the newest `--limit` commits /// (`run --analyzers history/burndown --format ndjson`, no `--timeseries`, /// no `--head`), or `None` if the repository cannot be opened/walked. /// @@ -294,8 +293,8 @@ pub fn burndown_ndjson_records( let limit = sub.get_one::("limit").copied().unwrap_or(0); - // Window: `limit` NEWEST commits oldest-first, first-parent (reference: - // `gitlib.loadHistoryCommits`; burndown forces --first-parent). + // Window: `limit` NEWEST commits oldest-first, first-parent (burndown + // forces --first-parent). let hashes = crate::handlers::load_history_commit_hashes( &repo, limit, @@ -377,7 +376,7 @@ pub fn burndown_ndjson_records( /// `history/burndown --format json` over the general history pipeline (streaming, /// e.g. `--limit N --workers 1`): the line-survival "burndown" report over the -/// oldest N commits. +/// newest N commits. /// /// This is the REAL port of the reference streaming pipeline /// (`the reference `initHistoryPipeline` Reverse+FirstParent+Limit → RunStreaming → @@ -456,8 +455,8 @@ pub fn burndown_run_aggregate(sub: &clap::ArgMatches) -> Option Option { let first_parent = crate::handlers::effective_first_parent(sub); // Commit window: HEAD-only loads the single HEAD commit; streaming selects - // the `limit` NEWEST commits (the reference `gitlib.loadHistoryCommits`: newest-first - // walk, CollectN, then slices.Reverse to oldest-first) — NOT the `limit` - // oldest. With `limit <= 0` this is the full oldest-first history. + // the `limit` NEWEST commits (newest-first walk, then reversed to + // oldest-first). With `limit <= 0` this is the full oldest-first history. let hashes: Vec = if head_only { vec![repo.head().ok()?] } else { diff --git a/crates/cf-commands/src/handlers/history.rs b/crates/cf-commands/src/handlers/history.rs index bfacda9f..1988e2c8 100644 --- a/crates/cf-commands/src/handlers/history.rs +++ b/crates/cf-commands/src/handlers/history.rs @@ -364,7 +364,7 @@ pub fn anomaly_head_report(sub: &clap::ArgMatches) -> Option Option Option { let limit = sub.get_one::("limit").copied().unwrap_or(0); let first_parent = crate::handlers::effective_first_parent(sub); - // Window: `--head` loads EXACTLY the single HEAD commit (the reference implementation, - // ignoring `--limit`); otherwise the `limit` commits oldest-first (reference: - // `gitlib.loadHistoryCommits`). + // Window: `--head` loads EXACTLY the single HEAD commit (ignoring `--limit` + // and `--since`); otherwise the `--limit` NEWEST commits, oldest-first for + // the analyzers, capped by `--since`. See `handlers::load_history_commit_hashes`. let hashes = if sub.get_flag("head") { vec![repo.head().ok()?] } else { @@ -766,11 +766,11 @@ pub fn anomaly_timeseries_contribution( } /// Builds the `run --analyzers history/quality --format json` bytes for the -/// oldest `--limit` commits, or `None` if the repository cannot be opened/walked. +/// newest `--limit` commits, or `None` if the repository cannot be opened/walked. /// /// Reproduces the reference streaming quality pipeline as a closed form: -/// - **commit set / order**: `repository.Log(Reverse=true)` (oldest-first, -/// `SortTime|SortTopological|SortReverse`), truncated to `--limit` commits +/// - **commit set / order**: `handlers::load_history_commit_hashes` — the `--limit` NEWEST +/// commits, delivered oldest-first /// (reference initHistoryPipeline: `commitCount` capped at `opts.Limit`). /// - **tick assignment** (`plumbing.TicksSinceStart`): `tick0 = FloorTime(when0, /// 24h)`; `tick = max(floor((when-tick0)/24h), previousTick)` over the @@ -888,10 +888,11 @@ pub(crate) fn quality_walk(sub: &clap::ArgMatches) -> Option> let limit = sub.get_one::("limit").copied().unwrap_or(0); - // Window: `--head` loads EXACTLY the single HEAD commit (the reference implementation, ignoring - // `--limit`); otherwise the `limit` commits oldest-first (reference: - // `gitlib.loadHistoryCommits`). The HEAD commit on a large repo spills (> 32 - // changes ⇒ zero UAST files), yielding the reference implementation's single all-zero tick-0 report. + // Window: `--head` loads EXACTLY the single HEAD commit (ignoring `--limit` + // and `--since`); otherwise the `--limit` NEWEST commits, oldest-first for + // the analyzers, capped by `--since`. See `handlers::load_history_commit_hashes`. + // The HEAD commit on a large repo spills (> 32 changes ⇒ zero UAST files), + // yielding the reference implementation's single all-zero tick-0 report. let first_parent = crate::handlers::effective_first_parent(sub); let hashes = if sub.get_flag("head") { vec![repo.head().ok()?] @@ -1315,11 +1316,11 @@ fn uast_to_cx_node(n: &cf_uast::Node) -> cf_complexity::node::Node { } /// Builds the `run --analyzers history/sentiment --format json` bytes for the -/// oldest `--limit` commits, or `None` if the repository cannot be opened/walked. +/// newest `--limit` commits, or `None` if the repository cannot be opened/walked. /// /// Reproduces the reference streaming sentiment pipeline as a closed form: -/// - **commit set / order**: `repository.Log(Reverse=true)` (oldest-first), -/// truncated to `--limit` commits (reference initHistoryPipeline). +/// - **commit set / order**: `handlers::load_history_commit_hashes` — the `--limit` NEWEST +/// commits, delivered oldest-first (reference initHistoryPipeline). /// - **tick assignment** (`plumbing.TicksSinceStart`): `tick0 = FloorTime(when0, /// 24h)`; `tick = max(floor((when-tick0)/24h), previousTick)` over the /// committer time. `commits_by_tick` records each tick's commit hashes (drives @@ -1353,7 +1354,7 @@ pub fn sentiment_run_report(sub: &clap::ArgMatches) -> Option> { Some(cf_gojson::marshal(&metrics.to_go_value())) } -/// Computes the typed sentiment [`cf_sentiment::ComputedMetrics`] for the oldest +/// Computes the typed sentiment [`cf_sentiment::ComputedMetrics`] for the newest /// `--limit` commits — the single report value behind every output format. The /// serializer (json / yaml / bin) is chosen by the caller so all formats follow /// from the one computation (the reference `ComputeAllMetrics` → `FormatReport*`). @@ -1443,9 +1444,9 @@ pub(crate) fn sentiment_walk(sub: &clap::ArgMatches) -> Option("limit").copied().unwrap_or(0); - // Window: `--head` loads EXACTLY the single HEAD commit (the reference implementation, ignoring - // `--limit`); otherwise the `limit` commits oldest-first (reference: - // `gitlib.loadHistoryCommits`). + // Window: `--head` loads EXACTLY the single HEAD commit (ignoring `--limit` + // and `--since`); otherwise the `--limit` NEWEST commits, oldest-first for + // the analyzers, capped by `--since`. See `handlers::load_history_commit_hashes`. let first_parent = crate::handlers::effective_first_parent(sub); let hashes = if sub.get_flag("head") { vec![repo.head().ok()?] @@ -1772,8 +1773,8 @@ fn extract_shell_comment_nodes(content: &[u8], out: &mut Vec Option> let limit = sub.get_one::("limit").copied().unwrap_or(0); let first_parent = crate::handlers::effective_first_parent(sub); - // Window: `--head` loads EXACTLY the single HEAD commit (the reference implementation, - // ignoring `--limit`); otherwise the `limit` commits oldest-first (reference: - // `gitlib.loadHistoryCommits`). + // Window: `--head` loads EXACTLY the single HEAD commit (ignoring `--limit` + // and `--since`); otherwise the `--limit` NEWEST commits, oldest-first for + // the analyzers, capped by `--since`. See `handlers::load_history_commit_hashes`. let hashes = if sub.get_flag("head") { vec![repo.head().ok()?] } else { @@ -2213,8 +2214,8 @@ fn imports_map_to_report_value( /// → `file_history.HistoryAnalyzer.Consume` → aggregator → `ticksToReport` → /// `BaseHistoryAnalyzer.Serialize` → `ComputeAllMetricsWithOptions`): /// -/// - **commit set / order**: `repository.Log(Reverse=true)` (oldest-first, -/// `SortTime|SortTopological|SortReverse`), truncated to `--limit` commits +/// - **commit set / order**: `handlers::load_history_commit_hashes` — the `--limit` NEWEST +/// commits, delivered oldest-first /// (reference: `commitCount` capped at `opts.Limit`). `--first-parent` adds /// `SimplifyFirstParent`. /// - **merge dedup** (`shouldConsumeCommit` / `MergeTracker`): a commit with @@ -2341,11 +2342,9 @@ pub(crate) fn file_history_run(sub: &clap::ArgMatches) -> Option let first_parent = crate::handlers::effective_first_parent(sub); let head_only = sub.get_flag("head"); - // The reference implementation: `--head` loads EXACTLY the single HEAD commit (ignoring - // `--limit`); otherwise `initHistoryPipeline` streams the first - // `min(limit, total)` commits of an oldest-first walk — the N OLDEST commits, - // oldest-first (see `load_history_commit_hashes`). - // Oldest-first window (the N OLDEST commits, oldest-first). + // Window: `--head` loads EXACTLY the single HEAD commit (ignoring `--limit` + // and `--since`); otherwise the `--limit` NEWEST commits, oldest-first for + // the analyzers, capped by `--since` (`load_history_commit_hashes`). let revwalk_hashes = if head_only { vec![repo.head().ok()?] } else { @@ -3092,8 +3091,8 @@ fn compute_diff_line_stats( /// `typos.Analyzer.Consume` → `extractTC`/`buildTick` (per-tick dedup) → /// `ticksToReport` (cross-tick dedup) → `BaseHistoryAnalyzer.Serialize` → /// `ComputeAllMetrics`): -/// - **commit set / order**: `repository.Log(Reverse=true)` (oldest-first, -/// `SortTime|SortTopological|SortReverse`), truncated to `--limit` commits. +/// - **commit set / order**: `handlers::load_history_commit_hashes` — the `--limit` NEWEST +/// commits, delivered oldest-first. /// `--first-parent` adds `SimplifyFirstParent`. With `--workers 1` Consume is /// sequential in walk order, so per-tick and cross-tick dedup collapse to a /// single global first-seen dedup in walk order (which is what we do). @@ -3283,9 +3282,9 @@ pub(crate) fn typos_walk(sub: &clap::ArgMatches) -> Option> { let max_distance = typos_max_distance(sub); - // Window: `--head` loads EXACTLY the single HEAD commit (the reference implementation, - // ignoring `--limit`); otherwise the `limit` commits oldest-first (reference: - // `gitlib.loadHistoryCommits`). + // Window: `--head` loads EXACTLY the single HEAD commit (ignoring `--limit` + // and `--since`); otherwise the `--limit` NEWEST commits, oldest-first for + // the analyzers, capped by `--since`. See `handlers::load_history_commit_hashes`. let hashes = if sub.get_flag("head") { vec![repo.head().ok()?] } else { @@ -3790,8 +3789,8 @@ fn devs_detect_language(name: &str, data: &[u8]) -> String { /// TreeDiff, BlobCache, FileDiff, LinesStats, LanguagesDetection}` → /// `devs.Analyzer.Consume` → `extractTC`/`buildTick`/`ticksToReport` → /// `BaseHistoryAnalyzer.Serialize` → `ComputeAllMetrics`): -/// - **commit set / order**: `repository.Log(Reverse=true)` (oldest-first), -/// truncated to `--limit` commits. `--first-parent` adds `SimplifyFirstParent`. +/// - **commit set / order**: `handlers::load_history_commit_hashes` — the `--limit` NEWEST +/// commits, delivered oldest-first. `--first-parent` adds `SimplifyFirstParent`. /// - **oversized-commit skip** (the reference `maxChangesPerCommit = 10000`): /// a commit whose RAW tree diff exceeds 10000 changes is skipped ENTIRELY — /// its core analyzers never run, so it contributes nothing to the people dict @@ -3931,9 +3930,9 @@ fn devs_walk(sub: &clap::ArgMatches) -> Option { let limit = sub.get_one::("limit").copied().unwrap_or(0); let first_parent = crate::handlers::effective_first_parent(sub); - // Window: `--head` loads EXACTLY the single HEAD commit (the reference implementation, - // ignoring `--limit`); otherwise the `limit` commits oldest-first (reference: - // `gitlib.loadHistoryCommits`). + // Window: `--head` loads EXACTLY the single HEAD commit (ignoring `--limit` + // and `--since`); otherwise the `--limit` NEWEST commits, oldest-first for + // the analyzers, capped by `--since`. See `handlers::load_history_commit_hashes`. let hashes = if sub.get_flag("head") { vec![repo.head().ok()?] } else { diff --git a/crates/cf-commands/src/handlers/uast_walk.rs b/crates/cf-commands/src/handlers/uast_walk.rs index 496f02c1..14556c62 100644 --- a/crates/cf-commands/src/handlers/uast_walk.rs +++ b/crates/cf-commands/src/handlers/uast_walk.rs @@ -55,6 +55,11 @@ pub(crate) const MAX_BLOB_SIZE: usize = 256 * 1024; /// while a gates-passed-but-unparsable file still counts as an analyzed file /// for quality and feeds sentiment's shell-comment fallback (which needs the /// blob bytes). +/// +/// Every instance lives behind an `Rc` in [`CommitParseCache`], so the size gap +/// between `Skipped` and the payload variants never reaches a stack frame or a +/// hot copy; boxing them would only add an indirection to the parse path. +#[allow(clippy::large_enum_variant)] pub(crate) enum ParseOutcome { /// A pre-parse gate rejected the file: zero hash, path policy, unsupported /// extension, blob read failure, the 256 KiB cap, or content-aware @@ -350,7 +355,7 @@ fn compute_shared_walks(sel: UastSelection, key: &WalkKey) -> Option i32 { } }; + // `--since` is validated against the repository BEFORE any walking, like the + // reference's `initHistoryPipeline` (which parsed it during pipeline init). + // Left to the handler, an unresolvable value surfaced only as the generic + // dispatch-blocked diagnostic — which blames the port for the user's typo + // and never names the offending flag. stdout stays empty and rc stays 1 + // either way; this only makes the failure diagnosable. Scoped to runs with + // history analyzers selected, because a static-only run never reads the + // flag (reference parity). The repository's own frozen error text is kept + // verbatim so existing greps on `cannot parse time:` still match. + if !_prog_history.is_empty() { + if let handlers::SinceSpec::Active(spec) = handlers::history_since_spec(sub) { + let repo_path = handlers::run_repo_path(sub); + if let Ok(repo) = cf_gitlib::Repository::open(&repo_path) { + if let Err(e) = repo.resolve_time(&spec) { + eprintln!("Error: cannot resolve --since {spec:?}: {e}"); + return 1; + } + } + } + } + // --format plot routes to the multi-page HTML renderer (the reference implementation: the // static/history phases each call validatePlotFlags then the plot // executor). The --output precheck fires for ANY plot selection (the exact diff --git a/crates/cf-gitlib/Cargo.toml b/crates/cf-gitlib/Cargo.toml index bdac4dbf..e2e75e59 100644 --- a/crates/cf-gitlib/Cargo.toml +++ b/crates/cf-gitlib/Cargo.toml @@ -23,6 +23,12 @@ cf-alg = { path = "../cf-alg" } # Derived Display/Error for the GitError taxonomy (strings are frozen contract). thiserror = "1" +[features] +# Deterministic on-disk repository fixtures (`cf_gitlib::testutil`) for +# downstream crates' tests. Off in every production build, so the fixture +# builder never reaches a shipped binary. +testutil = [] + [lints.clippy] # Hash<->Oid byte copies and libgit2 size_t<->i64 conversions are deliberate, # centralized, and verified by unit tests; silence the pedantic cast lints. diff --git a/crates/cf-gitlib/src/lib.rs b/crates/cf-gitlib/src/lib.rs index b0ccf33a..29edccdd 100644 --- a/crates/cf-gitlib/src/lib.rs +++ b/crates/cf-gitlib/src/lib.rs @@ -34,7 +34,7 @@ pub mod worker; #[cfg(test)] pub mod testing; -#[cfg(test)] +#[cfg(any(test, feature = "testutil"))] pub mod testutil; pub use blob::Blob; diff --git a/llms-full.txt b/llms-full.txt index aea3e5c1..7b98fb8d 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -1091,16 +1091,29 @@ codefang run -a 'history/devs,history/sentiment' --format timeseries . | Flag | Type | Default | Description | |------|------|---------|-------------| -| `--limit` | `int` | `0` | Maximum commits to analyze (`0` = no limit) | -| `--since` | `string` | `""` | Only analyze commits after this time | +| `--limit` | `int` | `0` | Analyze at most N commits — the N **newest** (`0` = no limit) | +| `--since` | `string` | `""` | Only analyze commits at/after this time or revision | | `--first-parent` | `bool` | `false` | Follow only first parent of merge commits | -| `--head` | `bool` | `false` | Analyze only HEAD commit | +| `--head` | `bool` | `false` | Analyze only HEAD commit (ignores `--limit` and `--since`) | -The `--since` flag accepts multiple formats: +These flags bound the **history** phase only — static analyzers read the +working tree, so neither one affects them. -- **Go duration**: `24h`, `168h`, `720h` -- **Date**: `2025-01-01` +The window is selected **newest-first** (`--limit 10` = the ten most recent +commits, like `git log -n 10`) and then handed to the analyzers in +chronological oldest-first order, because ticks, burndown line survival and +author identity all depend on that order. `--limit` and `--since` compose: +`--since` picks the window, `--limit` caps its newest end. + +The `--since` flag accepts these forms, in precedence order: + +- **Go duration**: `24h`, `168h` (7 days), `1h30m` — units are `h`/`m`/`s` + only. There is **no** `d` or `w` unit and `m` means **minutes**, so a week + is `168h`, not `7d` +- **Date**: `2025-01-01` (midnight UTC) - **RFC 3339**: `2025-01-01T00:00:00Z` +- **Git revision**: `v1.2.0`, `HEAD~100`, or a bare SHA — that commit's author + time becomes the cutoff ```bash # Last 7 days only @@ -1109,10 +1122,20 @@ codefang run -a history/devs --since 168h . # Since a specific date codefang run -a history/burndown --since 2025-01-01 . +# Since a release tag +codefang run -a history/couples --since v1.2.0 . + # Only the latest 500 commits codefang run -a history/couples --limit 500 . ``` +!!! note "Empty windows and bad values" + A `--since` cutoff newer than every commit analyzes nothing: the run + succeeds (exit 0) and every history analyzer emits its empty report. A + cutoff at or before the root commit filters nothing. A value that is + neither a time form nor a resolvable revision fails immediately with + `Error: cannot resolve --since ""`, empty stdout, exit 1. + !!! note "Burndown and `--first-parent`" The burndown analyzer automatically enables `--first-parent` when selected. diff --git a/site/getting-started/first-analysis.md b/site/getting-started/first-analysis.md index 52caf356..ef6fbe99 100644 --- a/site/getting-started/first-analysis.md +++ b/site/getting-started/first-analysis.md @@ -341,9 +341,9 @@ checkpoint: | Flag | Example | Description | |------|---------|-------------| -| `--limit N` | `--limit 1000` | Analyze only the last N commits | -| `--since` | `--since 2024-01-01` | Only commits after this date | -| `--head` | `--head` | Snapshot of HEAD only (fast) | +| `--limit N` | `--limit 1000` | Analyze only the N newest commits | +| `--since` | `--since 2024-01-01` | Only commits at/after this date, duration, or revision | +| `--head` | `--head` | Snapshot of HEAD only (fast; ignores the two flags above) | | `--first-parent` | `--first-parent` | Skip merge-commit side branches | | `--workers N` | `--workers 8` | Parallel pipeline workers | | `--memory-budget` | `--memory-budget 2GB` | Auto-tune caches | diff --git a/site/guide/cli-reference.md b/site/guide/cli-reference.md index a850cf24..469a2fbb 100644 --- a/site/guide/cli-reference.md +++ b/site/guide/cli-reference.md @@ -101,16 +101,29 @@ codefang run -a 'history/devs,history/sentiment' --format timeseries . | Flag | Type | Default | Description | |------|------|---------|-------------| -| `--limit` | `int` | `0` | Maximum commits to analyze (`0` = no limit) | -| `--since` | `string` | `""` | Only analyze commits after this time | +| `--limit` | `int` | `0` | Analyze at most N commits — the N **newest** (`0` = no limit) | +| `--since` | `string` | `""` | Only analyze commits at/after this time or revision | | `--first-parent` | `bool` | `false` | Follow only first parent of merge commits | -| `--head` | `bool` | `false` | Analyze only HEAD commit | +| `--head` | `bool` | `false` | Analyze only HEAD commit (ignores `--limit` and `--since`) | -The `--since` flag accepts multiple formats: +These flags bound the **history** phase only — static analyzers read the +working tree, so neither one affects them. -- **Go duration**: `24h`, `168h`, `720h` -- **Date**: `2025-01-01` +The window is selected **newest-first** (`--limit 10` = the ten most recent +commits, like `git log -n 10`) and then handed to the analyzers in +chronological oldest-first order, because ticks, burndown line survival and +author identity all depend on that order. `--limit` and `--since` compose: +`--since` picks the window, `--limit` caps its newest end. + +The `--since` flag accepts these forms, in precedence order: + +- **Go duration**: `24h`, `168h` (7 days), `1h30m` — units are `h`/`m`/`s` + only. There is **no** `d` or `w` unit and `m` means **minutes**, so a week + is `168h`, not `7d` +- **Date**: `2025-01-01` (midnight UTC) - **RFC 3339**: `2025-01-01T00:00:00Z` +- **Git revision**: `v1.2.0`, `HEAD~100`, or a bare SHA — that commit's author + time becomes the cutoff ```bash # Last 7 days only @@ -119,10 +132,20 @@ codefang run -a history/devs --since 168h . # Since a specific date codefang run -a history/burndown --since 2025-01-01 . +# Since a release tag +codefang run -a history/couples --since v1.2.0 . + # Only the latest 500 commits codefang run -a history/couples --limit 500 . ``` +!!! note "Empty windows and bad values" + A `--since` cutoff newer than every commit analyzes nothing: the run + succeeds (exit 0) and every history analyzer emits its empty report. A + cutoff at or before the root commit filters nothing. A value that is + neither a time form nor a resolvable revision fails immediately with + `Error: cannot resolve --since ""`, empty stdout, exit 1. + !!! note "Burndown and `--first-parent`" The burndown analyzer automatically enables `--first-parent` when selected. diff --git a/site/guide/data-analytics.md b/site/guide/data-analytics.md index bff3bd77..19920337 100644 --- a/site/guide/data-analytics.md +++ b/site/guide/data-analytics.md @@ -413,19 +413,26 @@ No manual tuning needed. Key parameters: | Parameter | Flag | Default | Effect | |-----------|------|---------|--------| | Memory budget | `--memory-budget` | 2GB | Controls chunk size | -| Commit limit | `--limit` | 0 (all) | Bounds history depth | +| Commit limit | `--limit` | 0 (all) | Analyze at most N commits — the N **newest** | | First parent | `--first-parent` | false | Skip merge commits | -| Since | `--since` | none | Time-based filtering | +| Since | `--since` | none | Time- or revision-based filtering | ```bash -# Analyze only last 6 months, first-parent only -codefang run --since 6m --first-parent --format json /repo +# Analyze only the last 6 months (4380 h), first-parent only +codefang run --since 4380h --first-parent --format json /repo + +# ... or since a release tag, capped at the 500 newest commits in the window +codefang run --since v1.28.0 --limit 500 --format json /repo ``` +Duration units are `h`/`m`/`s` only — there is no `d`/`w`, and `m` is +**minutes**, so `--since 6m` means the last six minutes, not six months. + !!! note "`--since` with inactive repos" If no commits fall within the `--since` window, history analyzers produce - empty results (zero ticks, zero developers). Static analyzers still run - normally since they analyze the current file tree, not commit history. + empty results (zero ticks, zero developers) and the run still exits 0. + Static analyzers still run normally since they analyze the current file + tree, not commit history. --- diff --git a/tests/compat/matrix.toml b/tests/compat/matrix.toml index d857085f..28178d1d 100644 --- a/tests/compat/matrix.toml +++ b/tests/compat/matrix.toml @@ -66,6 +66,12 @@ uast_analyze = ["json", "text"] # ----------------------------------------------------------------------------- [flags] # history-affecting flags; each is one matrix cell against the base history run. +# +# DIVERGENCE: the `--limit:*` and `--since:*` cells below are compared against +# the Go oracle, whose window is the N OLDEST commits and which aborts on a +# partial `--since` range. codefang deliberately selects the N NEWEST commits +# and supports partial ranges, so those cells are EXPECTED to differ and must +# not be read as port defects (specs/frds/FRD-newest-commit-window.md). history = [ "--head", "--limit:50", diff --git a/tests/golden/MANIFEST.json b/tests/golden/MANIFEST.json index cdef0082..779784ae 100644 --- a/tests/golden/MANIFEST.json +++ b/tests/golden/MANIFEST.json @@ -2634,5 +2634,6 @@ "note": "uast query 'reduce(count)' count (blob 4d7a17c3afad3186d86f357906c843623eb57d49)" } ], - "triageNote": "Step 16 triage (2026-06-06): each machine&&stable=false capture was run twice via the Go ref AND twice via the Rust release binary. RESULT: Go is nondeterministic across its OWN two runs for ALL 22 (the golden sha never reproduces), so category (a) 'Rust-missing-sort -> byte-identical' has ZERO members. Two sub-causes: PURE-REORDER (deep-sorted content equal: cohesion/comments/complexity/composition/imports/couples/file-history) where the Rust port should emit a deterministic SORTED order as a correctness improvement (still not byte-identical to the unstable golden), and CONTENT-NONDETERMINISM (clones/halstead/shotness, and all_static which unions them) where the actual data differs run-to-run and no sort helps. All 22 correctly remain stable=false/nonBinding. See per-capture triageVerdict." + "triageNote": "Step 16 triage (2026-06-06): each machine&&stable=false capture was run twice via the Go ref AND twice via the Rust release binary. RESULT: Go is nondeterministic across its OWN two runs for ALL 22 (the golden sha never reproduces), so category (a) 'Rust-missing-sort -> byte-identical' has ZERO members. Two sub-causes: PURE-REORDER (deep-sorted content equal: cohesion/comments/complexity/composition/imports/couples/file-history) where the Rust port should emit a deterministic SORTED order as a correctness improvement (still not byte-identical to the unstable golden), and CONTENT-NONDETERMINISM (clones/halstead/shotness, and all_static which unions them) where the actual data differs run-to-run and no sort helps. All 22 correctly remain stable=false/nonBinding. See per-capture triageVerdict.", + "_windowDivergenceNote": "Captures bounded by --limit encode the RETIRED Go reference's window, which selected the N OLDEST commits (burndown.ndjson line 0 is 2c4b3a562ce3, 2014-06-06). codefang now selects the N NEWEST commits by owner ruling, so these captures are superseded, not binding: the harness skips every `run` capture today (IMPLEMENTED_PREFIXES = version), and a future re-capture must be taken from the Rust binary, not the Go one. See specs/frds/FRD-newest-commit-window.md." } From 5044a36d82975f00db5bbf9e6c38fc908d92ec2d Mon Sep 17 00:00:00 2001 From: Dmitriy Date: Tue, 22 Sep 2026 04:57:47 +0300 Subject: [PATCH 2/4] chore(quality): add the make lint / make deadcode gates and clear their backlog AGENTS.md mandates both gates, but neither target existed -- and `cargo clippy` could not pass anyway: it failed on deny-by-default lints (clippy::approx_constant in cf-reportutil, cf-gojson and cf-goyaml; clippy::never_loop in bins/uast), so the two pre-flight PR checkboxes were untickable by construction. - Makefile: `lint` (clippy --workspace --all-targets -- -D warnings) and `deadcode` (RUSTFLAGS="-D dead_code" cargo check --workspace --all-targets). The limitation of the latter -- rustc treats any `pub` item in a library crate as reachable, so it proves "no private code is unreachable", not "no exported function is unused" -- is documented at the target instead of glossed over. - Clear the workspace clippy backlog with real simplifications where the lint is right (single_match -> if let, if_not_else, get().is_none() -> assert against None, field_reassign_with_default -> struct update syntax, redundant closure, a no-op `& 0xFF` after `as u8`, doc backticks), and documented `#[allow]`s only where the flagged literal is a captured Go reference fixture: substituting f64::consts::PI for `3.14`/`3.14159` there would change the fixture under test, not the style. bins/uast's `for filename in files` could never iterate twice (it returns on the first pass while grammar wiring is pending), so it is now an explicit first-file bail with identical sentinel strings. - cf-uast: drop the never-read `InitState.rule_index` field. The local is still used for rule resolution; the map no longer stays alive inside every parser's init state. `make lint` and `make deadcode` both exit 0. The changelog entry for this work landed with the previous commit. No behavior or output-byte change: the workspace test suite is unchanged at 2455 passed / 0 failed. --- Makefile | 13 +++++++++++ bins/uast/src/mapping.rs | 6 ++++- crates/cf-anomaly/src/detect.rs | 6 ++--- crates/cf-clones/src/lib.rs | 2 ++ crates/cf-clones/src/report_section.rs | 5 +++++ crates/cf-cohesion/src/analyzer.rs | 2 +- crates/cf-complexity/src/lib.rs | 17 ++++++-------- crates/cf-gojson/src/ftoa.rs | 4 ++++ crates/cf-gojson/src/marshal.rs | 2 ++ crates/cf-goyaml/src/float.rs | 5 +++++ crates/cf-goyaml/src/lib.rs | 2 ++ crates/cf-goyaml/tests/oracle.rs | 2 +- crates/cf-observability/src/init_otel.rs | 28 +++++++++++++++--------- crates/cf-reportutil/src/accessors.rs | 2 ++ crates/cf-reportutil/src/format.rs | 3 +++ crates/cf-sentiment/src/analyzer.rs | 4 +++- crates/cf-uast/src/loader.rs | 3 --- 17 files changed, 76 insertions(+), 30 deletions(-) diff --git a/Makefile b/Makefile index 98d798c6..c8a9d7d9 100644 --- a/Makefile +++ b/Makefile @@ -35,6 +35,19 @@ install: submodules ## Build and install codefang + uast onto PATH (~/.cargo/bin test: submodules ## Run the workspace test suite $(CARGO) test --workspace +.PHONY: lint +lint: submodules ## Clippy across the workspace; every warning is an error + $(CARGO) clippy --workspace --all-targets -- -D warnings + +.PHONY: deadcode +# rustc's `dead_code` lint reports unreachable PRIVATE items (a `pub` item in a +# library crate always looks reachable to it, so a clean run means "no private +# code is unreachable", not "no exported function is unused"). A pub-API +# reachability audit needs external tooling (cargo-deadstats-class) which is +# deliberately not vendored here. +deadcode: submodules ## Fail the build on any `dead_code` finding + RUSTFLAGS="-D dead_code" $(CARGO) check --workspace --all-targets + .PHONY: clean clean: ## Remove build artifacts $(CARGO) clean diff --git a/bins/uast/src/mapping.rs b/bins/uast/src/mapping.rs index 770c9330..20604b12 100644 --- a/bins/uast/src/mapping.rs +++ b/bins/uast/src/mapping.rs @@ -252,7 +252,11 @@ fn show_tree_sitter(files: &[String], language: &str) -> Result<(), String> { if files.is_empty() { return Err("no input files provided".to_string()); } - for filename in files { + // The reference loops over every input file, but every path below returns, + // so only the first file is ever reached while grammar wiring is pending in + // cf-uast (DESIGN §5). Stated as an explicit first-file bail instead of a + // loop that cannot iterate twice; the sentinel strings are unchanged. + if let Some(filename) = files.first() { let _ = std::fs::read(filename) .map_err(|e| format!("failed to process {filename}: failed to read file: {e}"))?; // Language resolution mirrors mapping.go: an unknown/empty language is an diff --git a/crates/cf-anomaly/src/detect.rs b/crates/cf-anomaly/src/detect.rs index 2b2a3793..5b8d502e 100644 --- a/crates/cf-anomaly/src/detect.rs +++ b/crates/cf-anomaly/src/detect.rs @@ -202,10 +202,10 @@ pub fn sort_external_results(anomalies: &mut [ExternalAnomaly], summaries: &mut go_sort_slice(anomalies, |a, b| a.z_score.abs() > b.z_score.abs()); go_sort_slice(summaries, |a, b| { - if a.source != b.source { - a.source < b.source - } else { + if a.source == b.source { a.dimension < b.dimension + } else { + a.source < b.source } }); } diff --git a/crates/cf-clones/src/lib.rs b/crates/cf-clones/src/lib.rs index 010cfd7c..c36cb9ce 100644 --- a/crates/cf-clones/src/lib.rs +++ b/crates/cf-clones/src/lib.rs @@ -218,6 +218,8 @@ mod tests { assert_eq!(clone_message(16), MSG_HIGH_CLONES); } + // Exact on purpose: the zero-denominator guard returns 0.0 exactly. + #[allow(clippy::float_cmp)] #[test] fn clone_ratio_zero_when_either_is_zero() { assert_eq!(compute_clone_ratio(0, 10), 0.0); diff --git a/crates/cf-clones/src/report_section.rs b/crates/cf-clones/src/report_section.rs index 696d9cd4..3a1815d6 100644 --- a/crates/cf-clones/src/report_section.rs +++ b/crates/cf-clones/src/report_section.rs @@ -473,6 +473,9 @@ mod tests { f } + // Exact comparisons on purpose: `compute_score` maps its clamp endpoints + // (0.0 / 1.0) exactly, so an epsilon compare would hide a real regression. + #[allow(clippy::float_cmp)] #[test] fn compute_score_clamps_and_inverts() { assert_eq!(compute_score(1.5), 0.0); @@ -499,6 +502,8 @@ mod tests { assert_eq!(issues[0].severity, "Poor"); // similarity 1.0 >= 0.8 } + // Exact by construction: clone_ratio 0.0 maps to the score 1.0 endpoint. + #[allow(clippy::float_cmp)] #[test] fn empty_report_uses_default_status_and_full_score() { let a = Analyzer::new(); diff --git a/crates/cf-cohesion/src/analyzer.rs b/crates/cf-cohesion/src/analyzer.rs index 3fecdb40..f7d6af24 100644 --- a/crates/cf-cohesion/src/analyzer.rs +++ b/crates/cf-cohesion/src/analyzer.rs @@ -607,7 +607,7 @@ mod tests { Some(&ReportValue::Str("No functions found".into())) ); // The empty result has no analyzer_name / functions keys. - assert!(report.get("functions").is_none()); + assert_eq!(report.get("functions"), None); } #[test] diff --git a/crates/cf-complexity/src/lib.rs b/crates/cf-complexity/src/lib.rs index 20095005..dbc70419 100644 --- a/crates/cf-complexity/src/lib.rs +++ b/crates/cf-complexity/src/lib.rs @@ -302,15 +302,12 @@ fn find_functions(root: &Node) -> Vec<&Node> { fn find_functions_capped(root: &Node, find_depth: Option) -> Vec<&Node> { let mut by_type: Vec<&Node> = Vec::new(); let mut by_role: Vec<&Node> = Vec::new(); - match find_depth { - Some(cap) => { - root.find_nodes_by_type_capped(&[uast::FUNCTION, uast::METHOD], cap, &mut by_type); - root.find_nodes_by_roles_capped(&[node::role::FUNCTION], cap, &mut by_role); - } - None => { - root.find_nodes_by_type(&[uast::FUNCTION, uast::METHOD], &mut by_type); - root.find_nodes_by_roles(&[node::role::FUNCTION], &mut by_role); - } + if let Some(cap) = find_depth { + root.find_nodes_by_type_capped(&[uast::FUNCTION, uast::METHOD], cap, &mut by_type); + root.find_nodes_by_roles_capped(&[node::role::FUNCTION], cap, &mut by_role); + } else { + root.find_nodes_by_type(&[uast::FUNCTION, uast::METHOD], &mut by_type); + root.find_nodes_by_roles(&[node::role::FUNCTION], &mut by_role); } let mut seen: Vec<*const Node> = Vec::new(); @@ -1190,7 +1187,7 @@ mod tests { } /// Cyclomatic/cognitive/nesting parity for the canonical `if{loop{if}}` - /// body, reproducing the SonarSource model. The loop sits in the outer-if's + /// body, reproducing the `SonarSource` model. The loop sits in the outer-if's /// first-child (condition) slot, walked at nesting 0; its inner `if` is in /// the loop's first-child slot, walked at nesting 1. /// cognitive = if(+1) + loop(+1) + inner-if(nesting 1 → +2) = 4. Verified diff --git a/crates/cf-gojson/src/ftoa.rs b/crates/cf-gojson/src/ftoa.rs index 5fd7928b..4b19e1e0 100644 --- a/crates/cf-gojson/src/ftoa.rs +++ b/crates/cf-gojson/src/ftoa.rs @@ -209,6 +209,8 @@ mod tests { use super::*; #[test] + // Captured Go fixture input; 3.14 is the value under test, not PI. + #[allow(clippy::approx_constant)] fn json_float_threshold_and_exponent_stripping() { // >= 1e21 -> exponent, one-digit exponent stripping. assert_eq!(format_json_float(1e21), "1e+21"); @@ -243,6 +245,8 @@ mod tests { } #[test] + // Captured Go fixture input; 3.14 is the value under test, not PI. + #[allow(clippy::approx_constant)] fn g_layout_uses_two_digit_exponent() { // 'g' exponent threshold is exp < -4 || exp >= 21. assert_eq!(format_float_g(1e21), "1e+21"); diff --git a/crates/cf-gojson/src/marshal.rs b/crates/cf-gojson/src/marshal.rs index db3d48fb..361e10b8 100644 --- a/crates/cf-gojson/src/marshal.rs +++ b/crates/cf-gojson/src/marshal.rs @@ -414,6 +414,8 @@ mod tests { } #[test] + // Captured Go fixture input; 3.14 is the value under test, not PI. + #[allow(clippy::approx_constant)] fn scalars_compact() { assert_eq!(st(marshal(&GoValue::Null)), "null"); assert_eq!(st(marshal(&GoValue::Bool(true))), "true"); diff --git a/crates/cf-goyaml/src/float.rs b/crates/cf-goyaml/src/float.rs index b78edb9e..0af8bd8d 100644 --- a/crates/cf-goyaml/src/float.rs +++ b/crates/cf-goyaml/src/float.rs @@ -199,6 +199,11 @@ mod tests { use super::format_g; #[test] + // The literals below are Go-captured fixture inputs: some look like PI and + // one carries more digits than f64 holds, which is exactly what the Go + // encoder under test was asked to render. Not style slips, so the two + // literal-shape lints are silenced for the whole table. + #[allow(clippy::approx_constant, clippy::excessive_precision)] fn matches_reference_g_layout() { assert_eq!(format_g(1e20), "1e+20"); assert_eq!(format_g(1e21), "1e+21"); diff --git a/crates/cf-goyaml/src/lib.rs b/crates/cf-goyaml/src/lib.rs index bf8bd63d..0d7a7065 100644 --- a/crates/cf-goyaml/src/lib.rs +++ b/crates/cf-goyaml/src/lib.rs @@ -281,6 +281,8 @@ mod tests { ("f5", GoValue::Float(0.0)), ("f7", GoValue::Float(1e20)), ("f8", GoValue::Float(1e-7)), + // Beyond f64 precision on purpose: the literal is the Go fixture. + #[allow(clippy::excessive_precision)] ("f10", GoValue::Float(123456789.123456789)), ]); let expect = "f1: 0.7142857142857143\nf3: 1\nf5: 0\nf7: 1e+20\nf8: 1e-07\nf10: 1.2345678912345679e+08\n"; diff --git a/crates/cf-goyaml/tests/oracle.rs b/crates/cf-goyaml/tests/oracle.rs index 85f2b588..34d98d5a 100644 --- a/crates/cf-goyaml/tests/oracle.rs +++ b/crates/cf-goyaml/tests/oracle.rs @@ -186,7 +186,7 @@ fn b64_decode(s: &str) -> Vec { bits += 6; if bits >= 8 { bits -= 8; - out.push((buf >> bits) as u8 & 0xFF); + out.push((buf >> bits) as u8); } } out diff --git a/crates/cf-observability/src/init_otel.rs b/crates/cf-observability/src/init_otel.rs index 6e82fe10..ed0cbc93 100644 --- a/crates/cf-observability/src/init_otel.rs +++ b/crates/cf-observability/src/init_otel.rs @@ -324,8 +324,10 @@ mod tests { /// Mirrors the reference suite's `TestBuildResource_IncludesAppMode`. #[test] fn build_resource_includes_app_mode() { - let mut cfg = Config::default(); - cfg.mode = AppMode::Mcp; + let cfg = Config { + mode: AppMode::Mcp, + ..Config::default() + }; let res = build_resource(&cfg); assert_eq!(res.get("app.mode"), Some("mcp")); // service.name always present. @@ -335,10 +337,12 @@ mod tests { /// Mirrors the reference suite's `TestInit_WithResourceAttributes`. #[test] fn build_resource_optional_attrs() { - let mut cfg = Config::default(); - cfg.service_version = "1.2.3".to_string(); - cfg.environment = "test".to_string(); - cfg.mode = AppMode::Mcp; + let cfg = Config { + service_version: "1.2.3".to_string(), + environment: "test".to_string(), + mode: AppMode::Mcp, + ..Config::default() + }; let res = build_resource(&cfg); assert_eq!(res.get("service.version"), Some("1.2.3")); assert_eq!(res.get("deployment.environment"), Some("test")); @@ -385,8 +389,10 @@ mod tests { /// Mirrors the reference suite's `TestSampler_DebugTraceOverridesEnv`. #[test] fn sampler_debug_trace_overrides_env() { - let mut cfg = Config::default(); - cfg.debug_trace = true; + let cfg = Config { + debug_trace: true, + ..Config::default() + }; let s = select_sampler(&cfg, Some("always_off"), None); assert!(s.samples_root_span()); } @@ -394,8 +400,10 @@ mod tests { /// Mirrors the reference suite's `TestSampler_ConfigSampleRatioFallback`. #[test] fn sampler_config_sample_ratio_fallback() { - let mut cfg = Config::default(); - cfg.sample_ratio = 1.0; + let cfg = Config { + sample_ratio: 1.0, + ..Config::default() + }; let s = select_sampler(&cfg, None, None); assert!(s.samples_root_span()); } diff --git a/crates/cf-reportutil/src/accessors.rs b/crates/cf-reportutil/src/accessors.rs index f0380487..3a672bf9 100644 --- a/crates/cf-reportutil/src/accessors.rs +++ b/crates/cf-reportutil/src/accessors.rs @@ -238,6 +238,8 @@ mod tests { } // Reference suite: TestGetFloat64_Float. + // 3.14 is the captured Go fixture value, not an attempt to write PI. + #[allow(clippy::approx_constant)] #[test] fn get_float64_float() { let r = report(vec![("key", GoValue::Float(3.14))]); diff --git a/crates/cf-reportutil/src/format.rs b/crates/cf-reportutil/src/format.rs index f5c8d384..b60f0ce1 100644 --- a/crates/cf-reportutil/src/format.rs +++ b/crates/cf-reportutil/src/format.rs @@ -71,6 +71,9 @@ mod tests { } // Reference suite: TestFormatFloat. + // 3.14159 is the captured Go fixture input (its truncation to "3.1" is the + // assertion), not an attempt to write PI. + #[allow(clippy::approx_constant)] #[test] fn format_float_basic() { assert_eq!(format_float(3.14159), "3.1"); diff --git a/crates/cf-sentiment/src/analyzer.rs b/crates/cf-sentiment/src/analyzer.rs index 26cdb521..729b1dfb 100644 --- a/crates/cf-sentiment/src/analyzer.rs +++ b/crates/cf-sentiment/src/analyzer.rs @@ -377,10 +377,12 @@ mod tests { const MIN_LEN: i64 = 10; fn s(v: &[&str]) -> Vec { - v.iter().map(|x| x.to_string()).collect() + v.iter().copied().map(String::from).collect() } #[test] + // validate() copies a const exactly; an epsilon compare would hide a real clamp bug. + #[allow(clippy::float_cmp)] fn validate_clamps() { let mut c = Config { min_comment_length: 5, diff --git a/crates/cf-uast/src/loader.rs b/crates/cf-uast/src/loader.rs index 8546206d..42a74800 100644 --- a/crates/cf-uast/src/loader.rs +++ b/crates/cf-uast/src/loader.rs @@ -210,8 +210,6 @@ struct InitState { resolved: Vec, #[allow(dead_code)] lang_info: LanguageInfo, - /// First-occurrence-wins rule index keyed by rule name (inheritance). - rule_index: HashMap, /// Pattern-root-type → candidate rule indices in declaration order. rule_dispatch: HashMap>, /// The tree-sitter language, looked up via [`crate::languages::get_language`]. @@ -292,7 +290,6 @@ impl LazyDslParser { Ok(InitState { resolved, lang_info, - rule_index, rule_dispatch, ts_language, pattern_matcher, From 7e07b876cae42db4f0ae530572dc014a02775358 Mon Sep 17 00:00:00 2001 From: Dmitriy Date: Tue, 22 Sep 2026 05:03:05 +0300 Subject: [PATCH 3/4] style: rustfmt the history-window tests and handler CI's `Format check` step (`cargo fmt --all -- --check`) is a hard gate, and the two files the previous commits added/extended were not rustfmt-clean. `main` was clean, so `cargo fmt --all` touched only these two files -- verified before committing. Behavior is unchanged: `cargo fmt --all -- --check` exits 0 and `make test` is still 2455 passed / 0 failed. --- bins/codefang/tests/history_window.rs | 33 +++++++++++++++++++++++---- crates/cf-commands/src/handlers.rs | 32 ++++++++++++++++++-------- 2 files changed, 50 insertions(+), 15 deletions(-) diff --git a/bins/codefang/tests/history_window.rs b/bins/codefang/tests/history_window.rs index c621e5b2..205ecb6e 100644 --- a/bins/codefang/tests/history_window.rs +++ b/bins/codefang/tests/history_window.rs @@ -65,7 +65,14 @@ fn fixture() -> (TestRepo, Vec) { fn ndjson_window(path: &str, extra: &[&str]) -> (i32, Vec, String, String) { let out = codefang( &[ - &["-a", "history/burndown", "--workers", "1", "--format", "ndjson"], + &[ + "-a", + "history/burndown", + "--workers", + "1", + "--format", + "ndjson", + ], extra, &[path], ] @@ -74,7 +81,10 @@ fn ndjson_window(path: &str, extra: &[&str]) -> (i32, Vec, String, Strin let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); let hashes = stdout .lines() - .filter_map(|line| line.find(NEEDLE_HASH).map(|i| line[i + 8..i + 48].to_owned())) + .filter_map(|line| { + line.find(NEEDLE_HASH) + .map(|i| line[i + 8..i + 48].to_owned()) + }) .collect(); ( out.status.code().unwrap_or(-1), @@ -105,7 +115,13 @@ fn limit_bound_shows_in_the_report() { let path = test.path().to_str().expect("utf-8 path"); for (limit, want) in [("1", 1), ("2", 2), ("5", 5)] { let out = codefang(&[ - "-a", "history/devs", "--limit", limit, "--format", "json", path, + "-a", + "history/devs", + "--limit", + limit, + "--format", + "json", + path, ]); assert!(out.status.success(), "run failed for --limit {limit}"); let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); @@ -124,7 +140,10 @@ fn since_partial_window_succeeds_with_the_window() { let (test, hashes) = fixture(); let path = test.path().to_str().expect("utf-8 path"); let (rc, got, _stdout, stderr) = ndjson_window(path, &["--since", CUTOFF_3RD]); - assert_eq!(rc, 0, "a partial --since window must exit 0, stderr: {stderr}"); + assert_eq!( + rc, 0, + "a partial --since window must exit 0, stderr: {stderr}" + ); assert_eq!( got, vec![ @@ -195,7 +214,11 @@ fn head_overrides_limit() { ] .concat(), ); - assert!(out.status.success(), "run failed: {:?}", String::from_utf8_lossy(&out.stderr)); + assert!( + out.status.success(), + "run failed: {:?}", + String::from_utf8_lossy(&out.stderr) + ); out.stdout }; let head = report(&["--head"]); diff --git a/crates/cf-commands/src/handlers.rs b/crates/cf-commands/src/handlers.rs index 85793208..64e1e82f 100644 --- a/crates/cf-commands/src/handlers.rs +++ b/crates/cf-commands/src/handlers.rs @@ -322,7 +322,6 @@ pub fn load_history_commit_hashes( Some(hashes) } - /// Returns the reference implementation's streaming-pipeline *consume* order for an oldest-first commit /// window: the IDENTITY (oldest-first revwalk order). /// @@ -2568,7 +2567,11 @@ mod history_window_tests { for limit in [0, -1] { let got = load_history_commit_hashes(&test.repo, limit, false, SinceSpec::Inactive) .unwrap_or_else(|| panic!("limit {limit} loads")); - assert_eq!(got.as_slice(), &hashes[..], "limit {limit} must load all commits"); + assert_eq!( + got.as_slice(), + &hashes[..], + "limit {limit} must load all commits" + ); } } @@ -2629,7 +2632,11 @@ mod history_window_tests { SinceSpec::Active(DATE_BEFORE_ROOT.to_owned()), ) .expect("loads"); - assert_eq!(got.as_slice(), &hashes[..], "cutoff before root filters nothing"); + assert_eq!( + got.as_slice(), + &hashes[..], + "cutoff before root filters nothing" + ); } /// `--limit` caps the NEWEST end of the `--since` window: the window is @@ -2642,9 +2649,8 @@ mod history_window_tests { load_history_commit_hashes(&test.repo, 0, false, SinceSpec::Active(cutoff.clone())) .expect("window loads"); assert_eq!(unbounded.as_slice(), &hashes[2..]); - let capped = - load_history_commit_hashes(&test.repo, 2, false, SinceSpec::Active(cutoff)) - .expect("window loads"); + let capped = load_history_commit_hashes(&test.repo, 2, false, SinceSpec::Active(cutoff)) + .expect("window loads"); assert_eq!( capped.as_slice(), &hashes[3..], @@ -2656,8 +2662,13 @@ mod history_window_tests { #[test] fn since_accepts_a_git_revision() { let (test, hashes) = linear_repo(); - let got = load_history_commit_hashes(&test.repo, 0, false, SinceSpec::Active("HEAD~2".to_owned())) - .expect("--since must resolve a revision like `HEAD~2`"); + let got = load_history_commit_hashes( + &test.repo, + 0, + false, + SinceSpec::Active("HEAD~2".to_owned()), + ) + .expect("--since must resolve a revision like `HEAD~2`"); assert_eq!( got.as_slice(), &hashes[2..], @@ -2674,8 +2685,9 @@ mod history_window_tests { // Now == the newest-but-one commit's instant; `1h` back == CUTOFF_3RD. std::env::set_var("CODEFANG_NOW", (T0 + 3 * STEP).to_string()); let (test, hashes) = linear_repo(); - let got = load_history_commit_hashes(&test.repo, 0, false, SinceSpec::Active("1h".to_owned())) - .expect("`--since 1h` must resolve against CODEFANG_NOW"); + let got = + load_history_commit_hashes(&test.repo, 0, false, SinceSpec::Active("1h".to_owned())) + .expect("`--since 1h` must resolve against CODEFANG_NOW"); assert_eq!( got.as_slice(), &hashes[2..], From 8caae0caf71ac1f4fe45d89e0126cc1edd0b80b3 Mon Sep 17 00:00:00 2001 From: Dmitriy Date: Tue, 22 Sep 2026 05:17:49 +0300 Subject: [PATCH 4/4] chore(quality): clear clippy debt introduced by #26 The aggregate-int test helper in `mod history_filter_tests` (added by f54f1bb / #26) carries two clippy findings that the advisory CI Clippy step never failed on, but that `make lint` (`clippy --all-targets -- -D warnings`) does: * clippy::useless_format -- format!(r#""aggregate":{{"#) has no placeholders, so the brace escape was both pointless and a latent trap for whoever edits the literal next; * clippy::needless_borrows_for_generic_args -- once the literal is a &str, `json.find(&agg)` borrows a value that already satisfies the Pattern bound. Both are test-only code paths; behaviour is unchanged. Verified with `make lint`, `make deadcode`, `cargo fmt --all -- --check` (all clean) and `make test` (2480 passed, 0 failed). --- crates/cf-commands/src/handlers.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/cf-commands/src/handlers.rs b/crates/cf-commands/src/handlers.rs index 64e1e82f..e670361a 100644 --- a/crates/cf-commands/src/handlers.rs +++ b/crates/cf-commands/src/handlers.rs @@ -2440,8 +2440,8 @@ mod history_filter_tests { let entry = registry.lookup(id).expect("analyzer registered"); let bytes = (entry.run)(&ctx, "json").expect("handler produced a report"); let json = String::from_utf8(bytes).expect("utf8"); - let agg = format!(r#""aggregate":{{"#); - let pos = json.find(&agg).expect("aggregate object"); + let agg = r#""aggregate":{"#; + let pos = json.find(agg).expect("aggregate object"); let from = &json[pos + agg.len()..]; let needle = format!(r#""{key}":"#); let kpos = from.find(&needle).expect("aggregate key");