Skip to content

feat(rees): detect resolved duplication via before/after comparison - #4760

Merged
JSONbored merged 2 commits into
mainfrom
feat/duplication-delta-analyzer
Jul 11, 2026
Merged

feat(rees): detect resolved duplication via before/after comparison#4760
JSONbored merged 2 commits into
mainfrom
feat/duplication-delta-analyzer

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Confirmed: what duplication (duplication-scan.ts) actually does today

Read review-enrichment/src/analyzers/duplication-scan.ts in full before writing any new code (per this
issue's mandatory first step). Note the real file is duplication-scan.ts, not duplication.ts as this
issue's description assumed.

Confirmed behavior matches the issue's working assumption:

  • scanDuplication extracts only ADDED significant lines from each changed file's patch
    (extractAddedBlocks), never anything from the file's own past.
  • It fetches the repo's git tree at headSha (current state only) and compares those added blocks against
    candidate files elsewhere in the repo — explicitly excluding the changed files themselves
    (if (changedPaths.has(entry.path) || isExcludedPath(entry.path)) continue;).
  • It has no notion of "before" at all: no call to reconstruct prior content, no comparison of a file against
    its own history. It is a pure "does this new code already exist somewhere else in the repo" detector.
  • Conclusion: it cannot see, and was never designed to see, duplication a PR removes/consolidates. That is
    a real, unfilled gap this PR closes.

What this PR adds

A new duplicationDelta analyzer (review-enrichment/src/analyzers/duplication-delta.ts) that detects the
reverse signal: a duplicate block pair that existed in a changed file's pre-PR content and is no longer
both present after (e.g. two near-identical functions consolidated into one).

  • Uses the shared reconstructOldContent primitive (Generalize reconstructOldContent into a shared before-content capability on AnalysisContext #4739) to recover each changed file's pre-PR text from
    its post-PR content + patch.
  • Reuses duplication-scan.ts's own chunk-normalization + suffix-automaton longest-shared-run matcher (now
    exported: isSourceExt, isExcludedPath, normalizeFileBlocks, buildMatchIndex, longestSharedRun) —
    same MIN_RUN threshold, same "what counts as a duplicate" definition — instead of a second, differently
    tuned algorithm.
  • Finds all internally-duplicated block pairs in the reconstructed OLD content, then greedily assigns
    each old block to an unclaimed matching block in the NEW content. This matters: a naive "does this old
    block's text exist anywhere in NEW" check would let both old copies of an identical pair "see" the single
    surviving occurrence and falsely conclude nothing changed. Greedy assignment lets only as many old blocks
    survive as there are still-distinct occurrences in NEW.
  • Reports per-file, per-block deltas (file, the old line that's now gone, the old line of its duplicate
    partner, matched-line count) — never a bare total, never code content — so the sibling complexity-delta
    sub-issue (Aggregate deterministic structural-improvement sub-score #4742, not yet started) has structured findings to aggregate.
  • Scope (v1): per file only. A duplicate pair split across two different files is not detected — noted
    clearly in the analyzer's own docs/notes rather than silently approximated. Left as a documented, scoped
    follow-up rather than adding a second repo-wide tree crawl to this PR.
  • Matches the existing AnalyzerDescriptor contract exactly (category quality, cost github-light,
    requires: ["files","github-token","head-sha"], its own render in the same modern per-descriptor style as
    coverageDelta/callerImpact).
  • No regression to duplication's existing new-duplication detection — it is untouched behaviorally; only
    five previously-private helpers gained an export keyword (verified via the full existing duplication-scan
    test suite passing unchanged).

Also fixed while building this

While mirroring the sibling analyzers' repo-slug validation pattern (doc-comment-drift.ts /
exhaustiveness-drift.ts / codeowners.ts), found that a bare /^[A-Za-z0-9._-]+$/ check does not reject
a segment of exactly ".." — every character in ".." is individually allowed by that character class, only
a first-character requirement catches it (which is why codeowners.ts's own regex is stricter). Since this
new file needed its own slug guard anyway, it uses the stricter, codeowners.ts-style pattern from the start.
Not fixed in the two pre-existing files (out of scope for this PR — see follow-up below).

Follow-ups flagged, not done here (deliberately out of scope for this PR)

  1. Shared bounded-content-fetch helper. doc-comment-drift.ts and exhaustiveness-drift.ts each hand-roll
    an identical private readBoundedText + inline fetch (no timeout, no circuit breaker) instead of the
    existing shared external-fetch.ts#boundedFetchText (which this PR's new analyzer uses, mirroring
    codeowners.ts). A small, mechanical follow-up PR — the same shape as refactor(rees): promote reconstructOldContent into a shared analyzer helper #4752's own promotion of
    reconstructOldContent — could migrate both to the shared helper.
  2. The same repo-slug validation gap in doc-comment-drift.ts / exhaustiveness-drift.ts noted above.
  3. Root REES_ANALYZER_NAMES allowlist (src/review/enrichment-analyzer-names.ts) does not include
    duplicationDelta. This is a separate, decoupled operator-config allowlist (.gittensory.yml
    review.enrichment.<name> toggles) — not required for the analyzer to run (defaultEnabled: true in its
    own descriptor is authoritative for that), and nothing enforces the two lists staying in sync. Left as a
    deliberate decision for whoever wires up per-repo toggling of this specific analyzer.

Gate status

  • npm run rees:test: green (1260/1260 — includes build + sourcemap validation +
    generate-analyzer-metadata.mjs --check + the full existing suite — no regressions).
  • Root npm run typecheck: green.
  • Full branch coverage on the new logic (31 new tests), including reconstructOldContent returning null
    (unreconstructable patch) and "" (wholly-new file) as distinct test cases per its truthiness-check
    contract.

Closes #4741
Part of epic #4737

Adds a duplicationDelta analyzer that flags a duplicate block pair which
existed in a changed file's pre-PR content and is no longer both present
after (e.g. two near-identical functions consolidated into one) — the
reverse of the existing duplication analyzer, which only detects NEW
duplication a PR introduces against the rest of the repo. Confirmed by
reading duplication-scan.ts directly before building on it (see PR
description).

Uses reconstructOldContent to recover each changed file's pre-PR text,
then greedily assigns each old block to an unclaimed matching block in
the new content so a duplicate-count reduction is detected correctly
(rather than every old copy independently matching the same surviving
text). Reuses duplication-scan.ts's own chunk-normalization and
suffix-automaton matcher, now exported, so "what counts as a duplicate"
stays identical between the add- and remove-detectors.

Also fixes a repo-slug validation gap surfaced while mirroring the
sibling analyzers' patterns: a bare [A-Za-z0-9._-]+ check accepts ".."
as a whole segment (every character is individually allowed), so this
analyzer requires an alphanumeric first character instead.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 11, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui 1d8d6f4 Commit Preview URL

Branch Preview URL
Jul 11 2026, 12:23 AM

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 11, 2026
@loopover-orb

loopover-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-11 00:25:26 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR adds a new `duplicationDelta` analyzer that detects duplicate-block pairs that existed in a changed file's reconstructed pre-PR content but are no longer both present post-PR (i.e., resolved/consolidated duplication) — a real, previously-unfilled gap relative to `duplication-scan.ts`, which only flags added lines duplicating content elsewhere in the repo at head. It correctly reuses `duplication-scan.ts`'s exported chunk-normalization and suffix-automaton primitives instead of a second differently-tuned matcher, and wires the new analyzer consistently across registry.ts, analyzer-metadata.json, rees-analyzers.ts, .env.example, and render.ts. The test suite drives real `reconstructOldContent`-based fixtures (contiguous deletions, multi-copy consolidation, replacement) rather than fabricated impossible states, and covers abort/fail-safe paths, MAX_FINDINGS capping, and both the `null` vs `""` reconstruction outcomes distinctly. The one real design wrinkle is `assignSurvivors`'s greedy (non-optimal, order-dependent) old→new block assignment, which can misreport a still-present duplicate as resolved in specific multi-candidate scenarios — an acknowledged v1 heuristic limitation, not a crash or data-integrity defect.

Nits — 6 non-blocking
  • `assignSurvivors` (duplication-delta.ts) uses greedy first-match assignment in file order rather than optimal bipartite matching, so an OLD block whose only viable NEW slot is 'stolen' by an earlier OLD block that had other options can be misreported as resolved-duplication when it actually still exists — worth a code comment acknowledging this false-positive mode explicitly, or a test pinning current behavior as intentional.
  • Deep nesting flagged at duplication-delta.ts:151 (depth 5) — consider extracting the inner double loop (over old blocks × new indices) into a small helper for readability.
  • The analyzer's docs/limits (minRun 8, maxFiles 20, maxFindings 25, maxBlocksPerFile 150) are hand-duplicated across registry.ts, analyzer-metadata.json, and rees-analyzers.ts with raw numeric literals in each — easy for the three copies to drift out of sync on a future tuning change.
  • `api.github.com` is hardcoded in duplication-delta.ts:46, though this mirrors the existing convention in duplication-scan.ts/codeowners.ts rather than introducing a new pattern.
  • The external history check notes linked issue Real duplication-delta analyzer (before/after comparison) #4741 coverage as 'partial' — worth confirming in the PR thread that the explicitly-deferred cross-file duplication-removal case is tracked as a follow-up issue rather than left implicit.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #4741
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 (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 48 registered-repo PR(s), 40 merged, 275 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 48 PR(s), 275 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 48 PR(s), 275 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
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.
Visual preview
Route Viewport Before (production) After (this PR's preview) Diff
/ desktop before / after /
/ mobile before / (mobile) after / (mobile)

Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy.

🟩 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

The greedy, order-dependent old-to-new block assignment can rarely
under-report a still-present duplicate as resolved in multi-candidate
scenarios. Advisory-only, never a correctness/data-integrity issue,
but worth stating explicitly rather than leaving implicit.
@JSONbored
JSONbored merged commit 2c8c200 into main Jul 11, 2026
11 checks passed
@JSONbored
JSONbored deleted the feat/duplication-delta-analyzer branch July 11, 2026 00:25
loopover-orb Bot pushed a commit that referenced this pull request Jul 11, 2026
… onto boundedFetchText (#4759) (#4821)

doc-comment-drift.ts, exhaustiveness-drift.ts, and complexity-delta.ts each
carried their own private "fetch a file at headSha, bounded/streamed read
capped at 1MB" helper -- near-byte-identical copies of the same logic, none
with a timeout or circuit breaker. duplication-delta.ts (#4741/#4760) already
migrated onto the more mature boundedFetchText (external-fetch.ts): a typed
ok/failure result, a per-endpoint-category circuit breaker, a configurable
timeout, and byte-size capping. Mirror that same fetchFileAtHead call
pattern -- including the options.analysis.fetchText fallback for when an
AnalysisContext is available -- in all three instead of leaving three more
hand-rolled copies to drift further.

doc-comment-drift.ts also switches to the shared githubHeaders() helper in
place of its own inline auth headers, closing the one pre-existing
inconsistency among the three (the other two already used it).

Pure fetch-mechanism migration -- verified byte-faithful via each analyzer's
existing test suite passing unchanged, plus one new test per file exercising
the added options.analysis branch (mirroring duplication-delta.test.ts's own
coverage of that path).
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. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

Real duplication-delta analyzer (before/after comparison)

1 participant