Repository navigation
audit declares when its dependency half is not the --head version - #28
Conversation
audit's two halves read from different places: history takes the commits named by --base/--head, deps takes the manifest checked out in --repo. Measured on the rustls run, a clone parked on main produced 284 dependency facts for a range whose tag holds 366, and nothing in the output said so. The fix declares rather than corrects: a dependency entry under criteria-not- evaluated, with a machine-readable code (worktree-not-head, or manifest-uncommitted for an edited-but-uncommitted manifest, which a commit comparison cannot see). Choosing declaration over reading the blob at --head was deliberate — the evidence pointers would stop naming files a reader can re-run. A declared gap cannot fail a run the history half answered, so the exit-code contract is unchanged. Tests build real repositories and assert both the presence and the absence of the declaration; the English string is pinned separately, because an unexercised half of a t() call is still production code.
|
Sorry @modusensus, you've used your own review budget of 250,000 diff characters for the last 7 days. You can request another review in 5 days and 3 hours by commenting |
Reviewer's GuideThe PR makes Sequence diagram for audit dependency version declarationssequenceDiagram
participant CLI as audit CLI
participant Git as Git repository
participant Deps as Dependency collector
participant Report as Audit report
CLI->>Git: revParse(toplevel, HEAD)
CLI->>Deps: collectDependencyFacts(...)
Deps-->>CLI: dependency facts and subject.lockfile
CLI->>Git: git(status --porcelain -- manifest)
alt worktree HEAD differs from resolved --head
CLI->>Report: add dependency code worktree-not-head
end
alt manifest has uncommitted changes
CLI->>Report: add dependency code manifest-uncommitted
end
CLI->>Report: retain measured facts and unchanged exit code
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@sourcery-ai review |
|
Sorry @modusensus, you've used your own review budget of 250,000 diff characters for the last 7 days. You can request another review in 5 days and 3 hours by commenting |
…out sha An independent review of the first version found the criterion wrong in both directions, and both cases reproduce against the real CLI: - a PR merge commit whose package-lock.json is byte-identical to --head's got worktree-not-head on every run. Not cosmetic: with a non-empty notEvaluated no gate may pass (fact-contract rule 1), so every finding citing a dependency fact was forced to not_evaluated, and the remedy the skill teaches — check out an end of the range — is impossible on a merge ref. - a lockfile that is gitignored and was never committed produced dependency facts with no declaration at all, even though its content belongs to no commit. The criterion is now content: git ls-tree decides whether the measured manifest exists at --head, git diff --name-only -- <head> decides whether it matches, and either way the entry is one code (manifest-not-head) rather than two. Using git diff also keeps core.autocrlf checkouts from reporting a difference that exists only in line endings. A probe that fails is declared as manifest-unverified: a git error is not evidence of a match, which is the direction the allowFailure version got backwards. The decision moved into an exported pure function so the fail-closed branch is reachable from a test; nothing else here can make git fail on demand. Suite: 246 tests, 245 pass, 0 fail, 1 skipped.
audit's two halves read from different places.historytakes the commits named by--base/--head;depstakes the manifest currently checked out at the repository root. Nothing in the output said so — measured on the rustls run, a clone parked onmainyielded 284 dependency facts for a range whose tag holds 366, and the report looked entirely normal.This is the CLI follow-up to the rustls case study, where the split was documented (
docs/case-study-rust.md§3, plus an AGENTS.md pitfall row). The maintainer chose declaration over re-measurement; this PR is that decision.What it does
One new entry in the
dependencyhalf ofcoverage.notEvaluated, carrying a machine-readablecode:codemanifest-not-head--head, or does not exist there at allmanifest-unverifiedExit codes are unchanged, deliberately:
audit's rule is that a missing or partial criterion cannot fail a run the history half answered (src/cli.js:524), and a declared gap is that kind of entry. Declaring also does not suppress the dependency facts that were measured — they stay in the report, and now say what version they describe.The criterion is content, and that was the review's finding
The first commit compared commit shas (
HEADvs--head) and added a separategit statuscheck for uncommitted edits. An independent review of that version found it wrong in both directions; both cases reproduce against the real CLI:B is not cosmetic. With
notEvaluatednon-empty, no gate may concludepass(fact-contract rule 1), so every finding citing a dependency fact was forced tonot_evaluated— and the remedy the skill teaches, "check out an end of the range", is impossible on a merge ref. A is the opposite failure: content that belongs to no commit, measured silently, reading as clean.Content comparison fixes both with one criterion:
git ls-treeanswers whether the measured manifest exists at--head,git diff --name-only <head> -- <path>answers whether it matches. Usinggit diffalso keepscore.autocrlfcheckouts from reporting a difference that exists only in line endings — the phantom-diff failure this repository already records forLICENSE.manifest-unverifiedexists because the earlier version passedallowFailureto both git calls, so a git error produced silence: the direction that inverts "missing data is never clean".Why declare rather than read the blob at
--headReading
git show <head>:Cargo.lockwould make the facts genuinely range-pinned, but every dependency fact's evidence pointer is apath:linea reader is told to re-run (euthyna deps --lockfile '<path>' --dep '<name>'). Pointing at a blob would break that channel — the file on disk would not contain what the report claims. Making it honest meant changing the evidence format across the contract, both skill editions and bothdocs/copies. That is a much larger change than the failure it fixes.Tests
Ten new/rewritten, building real git repositories as the suite already does:
--head→ declared, names the commit, keeps the facts, does not change the exit code--head(gitignored, never committed) → declared--head→ declares nothing, with assertions that the fixture's two manifests really are identical and really are different commits--head→ declares nothing--headcontent → declared--headomitted → declares nothing--lang enand the Chinese one does notThe limit flagged in the first version of this PR is now closed by measurement, not argument: the merge-commit and matching-manifest guards were written against the sha implementation and failed (that was their RED). Two "asserts nothing is declared" tests that fail under the wrong criterion cannot be vacuous.
docs/fact-contract.mdanddocs/fact-contract-zh.mdgain rule 4 (codeis machine-readable, appears only on--json, never a verdict); bothfact-producers.md§七 bullets and the AGENTS.md pitfall row describe the content criterion and say plainly that a declaration is not a measurement. Both language pairs were edited together, sotest/docs-pair.test.jsandtest/skill-mirror.test.jsare what prove the two languages did not drift.Suite
node --test→ tests 246 / pass 245 / fail 0 / skipped 1 (the deliberate non-Windows case).Summary by Sourcery
Declare dependency-version gaps in
auditby comparing the measured manifest's content with--headand fail closed when that verification is unavailable.New Features:
auditis absent from or differs in content from the--headversion, using machine-readablemanifest-not-headandmanifest-unverifiedcodes while retaining measured dependency facts.Bug Fixes:
Enhancements:
--headwhile remaining compatible with working-tree evidence paths and line-ending normalization.Documentation:
codecontract for JSON-only not-evaluated entries in English and Chinese fact-contract documentation.Tests: