Skip to content

fix(history): --limit selects the newest commits; --since windows work - #28

Merged
dmytrogajewski merged 4 commits into
mainfrom
fix/newest-commit-window
Sep 24, 2026
Merged

dmytrogajewski merged 4 commits into
mainfrom
fix/newest-commit-window

Conversation

@dmytrogajewski

@dmytrogajewski dmytrogajewski commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Scope

Two history-window bugs, both measured rather than theorized.

  1. --limit N analyzed the N oldest commits — inherited from the retired Go
    reference's initStreamingIterator (logOpts.Reverse = true + CollectN).
    Verified two independent ways:
    • live reference binary (build/bin/codefang): --limit 3 on kubernetes emits
      2c4b3a562ce3 (2014-06-06) — the repository's first commits;
    • tests/golden/run/burndown.ndjson (Go-captured at --limit 10) line 0 is
      that same commit.
  2. A partial --since window aborted the run. The planner counted the
    in-window commits, then the oldest-first loading pass applied the author-time
    stop filter and halted at the first too-old commit, so the run died with
    expected N commits, got 0: EOF — empty stdout, exit 1. The primary
    documented path (--since 2025-01-01) had never worked.

Both bounds now come from one newest-first pass (cap at --limit, then
reverse to the chronological order analyzers need), which deletes the
planner/loader two-pass and the under-fill abort. --since additionally accepts
git revisions (v1.2.0, HEAD~100, a SHA) and its durations now honor
CODEFANG_NOW, because the CLI's duplicate ~130-line time parser was deleted in
favour of cf-gitlib's single one.

Also fixed, on the same path:

  • An unresolvable --since said command dispatch is blocked on cf-commands (tier 8) — blaming the port for the user's typo. It now says
    Error: cannot resolve --since "<value>" (same rc=1, still no stdout).
  • site/guide/data-analytics.md told users --since 6m meant six months;
    m is minutes. Docs now state the supported units (h/m/s, no d/w)
    and use 4380h.
  • cf_gitlib::testutil documented a testutil cargo feature that did not exist,
    so no other crate could build a deterministic fixture repo. It exists now.
  • make lint / make deadcode did not exist although AGENTS.md and this
    PR template mandate them — and cargo clippy failed outright on deny-by-default
    lints. Second commit adds both targets and clears the backlog (incl. a dead
    InitState.rule_index field in cf-uast). Rebased onto fix(static+history): flag/language filters for every analyzer; root-cause oracle timeout #26, the gate caught two
    further lints in the test helper fix(static+history): flag/language filters for every analyzer; root-cause oracle timeout #26 added (clippy::useless_format,
    clippy::needless_borrows_for_generic_args) that the advisory CI Clippy step had
    never reported; 8caae0c clears them.

Measured behavior (built binary, 5-commit fixture dated 2021-01-01…05):

Invocation Before After
--limit 2 the two oldest commits 74d76e4b,f5846e20 — newest two, oldest-first
--since 2021-01-03 empty stdout, exit 1 3 commits, exit 0
--since <sha> / HEAD~2 aborted window starts at that commit
--since 2030-01-01 0 lines, exit 0 unchanged
--since nonsense exit 1, misleading tier-8 error exit 1, Error: cannot resolve --since "nonsense"
--head vs --limit 1 — byte-identical burndown json

Test matrix

Written red first against the old behavior, then green.

  • Unit — crates/cf-commands/src/handlers.rs::history_window_tests (10):
    newest-N selection; --limit 0/negative = unlimited; partial window; empty
    window (cutoff after HEAD); cutoff at/before root; --limit capping the newest
    end of a --since window; revision spec; CODEFANG_NOW-injected duration;
    unresolvable spec aborts; a self-check tying the cutoff literal to the fixture
    clock so expectations cannot drift.
  • CLI e2e — bins/codefang/tests/history_window.rs (7): real binary over a
    real on-disk repo, machine formats only (the ones the golden harness treats as
    binding): newest-N, user-visible total_commits growth with --limit, partial
    --since exits 0 with the right hashes, --since <sha>, empty window exits 0,
    bad --since → rc 1 + empty stdout + flag named in stderr, and the
    --head ≡ --limit 1 byte-identity invariant (which also proves --head
    ignores --limit).

Gates, re-measured after the rebase onto f54f1bb (#26): make test
2480 passed / 0 failed across 164 result groups, make lint (=
clippy --workspace --all-targets -- -D warnings) exit 0, make deadcode exit 0,
cargo fmt --all -- --check exit 0. No flakes across repeated runs.

Link to FRD/BUG

specs/frds/FRD-newest-commit-window.md (includes the measured before/after
evidence and the residuality analysis). specs/ is gitignored in this repo, so
the FRD is not in the diff — content is reproduced in this description.

Breaking changes

Yes — output-changing for --limit and --since runs, by owner ruling:

  • --limit N now analyzes the N newest commits (was: N oldest). This is a
    deliberate divergence from the retired Go reference; the golden harness is
    unaffected (it skips every run capture today — IMPLEMENTED_PREFIXES is
    version only), so no bytes were rewritten under tests/golden/. Instead,
    tests/golden/MANIFEST.json and tests/compat/matrix.toml now record that
    --limit/--since cells encode the superseded reference and must not be read
    as port defects. A future re-capture must come from the Rust binary.
  • --since in the middle of a history now produces a report instead of exiting 1.
  • Commit order fed to analyzers is unchanged (oldest-first), so ticks, burndown
    line survival and identity ids behave as before; --head, --first-parent and
    all static analyzers are untouched.
  • The one signature change is SinceSpec::Active(i64) → Active(String) (a raw
    spec string, resolved once by cf_gitlib::Repository::resolve_time). It has
    three production sites — history_since_spec, the cutoff resolution in
    load_history_commit_hashes, and the new pre-dispatch check — all inside
    cf-commands; every other mention is this PR's tests.

Risks / known limits

  • The walk orders by committer time while --since compares author time, so a
    history with bulk git commit --date rewrites can be cut short. Inherited from
    the reference, documented at the function; mitigation is a skip-filter (costs a
    full walk).
  • Duration units stay h/m/s (Go ParseDuration has no d/w), so
    --since -24h now resolves to a future cutoff (empty window) instead of
    erroring, and 1.5h is rejected.
  • make deadcode proves no private code is unreachable; a pub-API
    reachability audit needs external tooling and is out of scope.

Rollback — revert the four commits
(git revert --no-commit fe4455b^..8caae0c && git commit, newest-first by design);
no data/migration/format change is involved. The testutil feature
is additive and off by default, so it is inert for shipped binaries.


Before opening this PR

  • All tests pass locally (make test) — 2480 passed, 0 failed (post-rebase)
  • Linting is clean (make lint) — added by this PR, exit 0
  • Dead code analysis passes (make deadcode) — added by this PR, exit 0
  • Commit messages follow conventional commits
  • Documentation is updated if needed (site/guide/cli-reference.md,
    site/guide/data-analytics.md, site/getting-started/first-analysis.md,
    llms-full.txt, CHANGELOG.md)

CI status

CI is executing for this PR now: push 8caae0c triggered CI run
35678954520
and Deploy Documentation run
35678954461
(Build Documentation, markdownlint, Vale, cspell, lychee all pass). Earlier in this
PR gh pr checks 28 reported no checks at all, and the newest run anywhere in
the repo predated #24/#25 — a repo-wide runner/Actions stall, not something caused
by this branch. That earlier state is why the local gates below were measured
instead of deferred to CI.

Local gates, all re-run after the rebase: cargo fmt --all -- --check exit 0,
make lint exit 0, make deadcode exit 0, make test 2480 passed / 0 failed.
The Test job outcome is recorded in the issue comments once it reports.

Note for maintainers: CI's Clippy step is continue-on-error: true, and that
advisory setting is exactly why the two #26 lints above reached main unnoticed.
The backlog is now empty, so it can become a blocking gate — deliberately left out
of this PR, as that is repo policy rather than part of the fix.

@dmytrogajewski

Copy link
Copy Markdown
Contributor Author

Post-open corrections & one observation

rustfmt follow-up (d6396d8). My first two commits were not rustfmt-clean, and CI's
Format check step (cargo fmt --all -- --check) is a hard gate — my PR body's
pre-flight list omitted it. cargo fmt --all touched only the two files this PR
adds/extends (bins/codefang/tests/history_window.rs, crates/cf-commands/src/handlers.rs),
which confirms main was already clean; --check now exits 0 and make test is
still 2455 passed / 0 failed.

Clippy is now eligible to stop being advisory. ci.yml marks Clippy
continue-on-error: true because "the workspace still carries pre-existing clippy
debt (e.g. clippy::never_loop, clippy::approx_constant in test code) … switch to
blocking once the debt is cleared." The second commit cleared that debt —
cargo clippy --workspace --all-targets -- -D warnings exits 0 locally — and this PR
adds make lint so it stays cleared. I deliberately did not flip
continue-on-error in this PR, since making CI blocking is a policy call for
maintainers; say the word and I will drop it into a follow-up commit here.

Not touched on purpose: third_party/libgit2 (dirty before this branch) and
.claude/workflows/generalize-loop.js (untracked before this branch) are excluded
from all three commits.

`--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 "<value>"` 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.
…ir 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.
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.
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).
@dmytrogajewski
dmytrogajewski force-pushed the fix/newest-commit-window branch from d6396d8 to 8caae0c Compare September 22, 2026 02:17
@dmytrogajewski

Copy link
Copy Markdown
Contributor Author

Rebased onto main (f54f1bb, #26)

main absorbed #26 while this branch was open, which is why the PR briefly showed
CONFLICTING. Force-pushed the rebased branch (d6396d8...8caae0c); the four
commits are now fe4455b, 5044a36, 7e07b87, 8caae0c.

Three conflicts, each resolved by keeping both sides — nothing from #26 was
dropped:

File Resolution
crates/cf-commands/Cargo.toml kept #26's git2 dev-dep and this PR's cf-gitlib (testutil) dev-dep
crates/cf-commands/src/lib.rs kept both validation blocks; pre-dispatch order is now static filter → history filter → --since
crates/cf-commands/src/handlers.rs kept both tail test modules (history_filter_tests / dispatch_flag_tests from #26, history_window_tests from this PR)

One finding the new make lint gate caught in #26

clippy --workspace --all-targets -- -D warnings fails on two lints in the
aggregate-int test helper that #26 added (format!(r#""aggregate":{{"#) has no
placeholders, and the follow-on json.find(&agg) borrows a value that already
satisfies Pattern). CI's Clippy step is continue-on-error: true, so nothing
ever reported them. 8caae0c fixes both; test-only, behavior unchanged.

Now that the backlog is clear, the Clippy step could stop being advisory — left as
a maintainer decision, since it is repo policy rather than part of this fix.

Post-rebase gates (re-measured, not carried over)

make test 2480 passed / 0 failed across 164 result groups (2455 before the
rebase; #26 added the rest), make lint exit 0, make deadcode exit 0,
cargo fmt --all -- --check exit 0. Focused suites re-run because #26 touched
history.rs/uast_walk.rs: history_window_tests 10/10,
bins/codefang/tests/history_window.rs 7/7.

@dmytrogajewski

Copy link
Copy Markdown
Contributor Author

CI is green on 8caae0c

CI run 35678954520
Test job: success in 7m21s, every step green — notably

Step Outcome
Format check (cargo fmt --all -- --check) success — hard gate
Clippy (--workspace --all-targets -- -D warnings) success
Test (cargo test --workspace) success

So the workspace is clippy-clean on the pinned toolchain on CI's own runner, not
just on this machine: CI executes the same -D warnings command and passes it. That
is the evidence for the note in the description — continue-on-error: true on that
step is no longer load-bearing, and flipping it off would have caught the two #26
lints before they landed.

Deploy Documentation run
35678954461:
Build Documentation, markdownlint, lychee pass; Vale and cspell (advisory) pass.
Deploy to GitHub Pages is skipped, as expected for a pull_request event.

PR state: mergeable=MERGEABLE, mergeStateStatus=CLEAN — ready for review.

@dmytrogajewski
dmytrogajewski merged commit 71c84d9 into main Sep 24, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants