Skip to content

fix(review): lockfile-tamper-risk scanner loses cross-hunk version/integrity correlation, causing false positives #8351

Description

@JSONbored

Context

src/review/lockfile-tamper.ts's scanPackageLockPatch (lines ~115-224) scans a package-lock.json unified diff for tamper risk: a resolved/integrity change WITHOUT the same lockfile entry's own "version" field genuinely changing. When an entry's own "node_modules/<pkg>": { header line isn't visible anywhere in the diff (git's default 3-line context doesn't guarantee it survives — the documented #7778 case), the scanner buckets the change under a synthetic "${path}#unattributed-N" key (activeUnknownKey, declared line 133-134).

The bug: activeUnknownKey is reset to null on any depth-0 closing brace (lines 179-187), not just at the boundary of the specific entry it belongs to. When the SAME lockfile entry's version bump and its resolved/integrity change happen to land in two separate hunks of the same patch (a realistic occurrence — git only gives ~3 lines of context, and an intervening } between the two changed fields is enough to reset tracking), each hunk gets a brand-new unattributed bucket with its own unknownEntrySeq. Bucket #1 records the version change; bucket #2 records the resolved/integrity change — but they never merge, so bucket #2's resolvedOrIntegrityChanged && !versionChanged check (line 246) is wrongly true even though the same entry legitimately bumped both fields together. This fires a false lockfile_tamper_risk finding on a routine npm install/npm update.

The 527-line existing test suite (test/unit/lockfile-tamper.test.ts) thoroughly covers the single-hunk unattributed case (#7778) but has no test for this cross-hunk scenario.

Requirements

Deliverables

  • Updated scanPackageLockPatch in src/review/lockfile-tamper.ts that correlates unattributed version/resolved/integrity signals for the same package name across hunk boundaries within one file's patch.
  • A new regression test in test/unit/lockfile-tamper.test.ts reproducing the cross-hunk scenario: a "version" add/remove pair in one hunk, and a "resolved"/"integrity" add/remove pair for the SAME package in a separate hunk (separated by an intervening depth-0 }), both with no visible entry header — asserting NO lockfile_tamper_risk finding fires.
  • A second regression test confirming a genuine tamper (integrity changed, version NOT changed) split across two hunks for the SAME package still correctly fires the finding.

Test Coverage Requirements

Standard 99%+ Codecov patch coverage, branch-counted, on every changed line/branch in lockfile-tamper.ts per this repo's codecov.yml (src/** is covered; only_pulls: true, threshold: 0%). Add both a positive case (still flags a real cross-hunk tamper) and a negative case (no longer false-flags a legitimate cross-hunk version+integrity bump) — this is a correctness fix, so both the old wrong behavior and the new correct behavior must be explicitly asserted, not just covered for line count.

Expected Outcome

A legitimate npm lockfile update whose version bump and resolved/integrity change land in different diff hunks (realistic any time an entry has enough intervening unchanged fields to exceed git's 3-line context) no longer produces a false lockfile_tamper_risk finding, while a genuine tamper (integrity changed without a real version bump) is still caught even when split across hunks.

Links & Resources

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions