Skip to content

fix(rees): consolidate diff-header detection onto shared isDiffFileHeaderLine#2705

Merged
JSONbored merged 1 commit into
JSONbored:mainfrom
glorydavid03023:fix/rees-diff-header-consolidation
Jul 3, 2026
Merged

fix(rees): consolidate diff-header detection onto shared isDiffFileHeaderLine#2705
JSONbored merged 1 commit into
JSONbored:mainfrom
glorydavid03023:fix/rees-diff-header-consolidation

Conversation

@glorydavid03023

Copy link
Copy Markdown
Contributor

What

Two rees analyzers — dependency-scan and lockfile-drift — still used the anchored inline guard

line.startsWith("+++ ") || line.startsWith("---")

to skip unified-diff file headers, instead of the shared isDiffFileHeaderLine helper that the other patch-scanning analyzers (diff-lines, heavy-dependency, history) already delegate to.

Why it matters

That anchored guard misclassifies added/removed content whose text begins with ++/-- as a file header: git renders an added line whose content is ++ x as the diff line +++ x, and --- likewise. The shared helper deliberately keys on the header's path form (+++ a/, +++ b/, +++ /dev/null) precisely to avoid this — it's documented in diff-lines.ts and covered by diff-lines.test.ts.

In lockfile-drift this is an observable off-by-one. Its line-number generator advances the new-file counter per emitted line; a +++-content line wrongly hit by the old guard is continued without advancing the counter, so every drift finding after it is reported one line too low:

OLD guard  → version reported at line 11   (wrong)
new helper → version reported at line 12   (correct)

In dependency-scan the anchored guard is behavior-equivalent for the structured manifests it parses (npm/PyPI/Go lines never begin ++/--), but it's the same drifted copy, so it's consolidated here for a single source of truth — completing the migration to isDiffFileHeaderLine.

Tests

  • lockfile-drift.test.ts: new regression asserting the corrected line number (12) across an intervening ++-content added line — fails on the old guard, passes now.
  • dependency-scan.test.ts (new file): guards that real --- a/… / +++ b/… headers are skipped while the real version bump is still extracted.

Full node --test suite green except the two pre-existing upload-sourcemaps tests that require Sentry-CLI locally (unrelated; pass in CI).

…aderLine

dependency-scan and lockfile-drift still used the anchored `startsWith("+++ ")`
/ `startsWith("---")` guard that the shared isDiffFileHeaderLine helper replaced
in the other patch-scanning analyzers. That guard misclassifies added/removed
CONTENT whose text begins with `++`/`--` (git renders content `++ x` as the diff
line `+++ x`) as a unified-diff file header.

In lockfile-drift's line-number generator this is a real off-by-one: a wrongly
skipped `+++`-content added line does not advance the new-file counter, so every
drift finding after it is reported one line too low. In dependency-scan the
anchored guard is behavior-equivalent for the structured manifests it parses but
is consolidated here for a single source of truth. Both now delegate to
isDiffFileHeaderLine, which matches only real `+++ a/`/`b/`/`/dev/null` headers.

Adds a lockfile-drift regression pinning the corrected line number across a
++-content added line, and a dependency-scan guard that real file headers are
skipped while the version bump is still extracted.
@loopover-orb

loopover-orb Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-03 07:35:31 UTC

4 files · 1 AI reviewer · no blockers · readiness 73/100 · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
The change consolidates both analyzers onto the shared diff-header discriminator and fixes the demonstrated lockfile line-counter bug for `+++`/`---` content that is not path-shaped. The implementation is small, imports are present in the post-change files, and the new regression exercises the reachable off-by-one path in `extractLockfileChanges`. I do not see a must-fix defect in the visible diff.

Nits — 5 non-blocking
  • nit: `review-enrichment/test/dependency-scan.test.ts:6` only proves real headers are still skipped, but it does not exercise the bug class this consolidation is about for dependency parsing; add a path-shaped or non-header `+++`/`---` content line if you want this test to guard the shared-discriminator behavior rather than just the happy path.
  • nit: `review-enrichment/test/lockfile-drift.test.ts:31` uses a non-ASCII dash in the synthetic patch/comment while the surrounding tests are ASCII-heavy; this is harmless but avoidable churn in a parser test.
  • Add a dependency-scan regression with an added manifest line that starts with `+++ ` but is not a real `+++ a/` or `+++ b/` header, even if today’s supported manifest parsers mostly make that behavior-equivalent.
  • Keep parser regression fixtures ASCII unless the Unicode character is itself part of the behavior under test.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ❌ 5/25 Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review.
Contributor workload ✅ 10/10 Author activity: 232 registered-repo PR(s), 144 merged, 11 issue(s).
Contributor context ✅ Confirmed Gittensor contributor glorydavid03023; Gittensor profile; 232 PR(s), 11 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: glorydavid03023
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: JavaScript, Python, TypeScript, Rust, C++, Kotlin, MDX
  • Official Gittensor activity: 232 PR(s), 11 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Explain no-issue PR.
  • Await review-lane availability.
  • Refresh registry data or choose a registered active repo.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@JSONbored
JSONbored merged commit 57c459a into JSONbored:main Jul 3, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Development

Successfully merging this pull request may close these issues.

2 participants