Skip to content

feat(review): deterministic changed-file classifier (#2143) - #3547

Merged
JSONbored merged 1 commit into
JSONbored:mainfrom
nickmopen:feat/changed-files-classify
Jul 5, 2026
Merged

feat(review): deterministic changed-file classifier (#2143)#3547
JSONbored merged 1 commit into
JSONbored:mainfrom
nickmopen:feat/changed-files-classify

Conversation

@nickmopen

Copy link
Copy Markdown
Contributor

Closes #2143 (part of #1957).

Adds a pure classifyChangedFile(path) that maps a changed file to one of five review buckets — source / test / docs / config / generated — so the changed-files summary table (and future analytics) group deterministically. Pure + path-only — no diff content, no IO — composing the existing hardened path-matchers.

Design

  • Fixed precedence generated > test > docs > config > source (documented), so a file matching several buckets — a generated test file, a lockfile, a vendored fixture — always resolves to the higher-precedence class.
  • Reuses the hardened matchers: isGeneratedFile / isVendoredFile / isLockfile / isMinifiedFile (→ generated), isTestFile / isTestPath, isDocsFile, isConfigFile.
  • Deliberately distinct from path-matchers.ts's existing 10-way classifyChangedFile: that one has a different category set and precedence (it ranks config above test), so this review classifier composes its own precedence rather than folding it.

Tests

One file per class, the precedence conflict (a vendored test file → generated), lockfile/vendored/minified/generated → generated, and unknown → source. Each path also asserts its underlying matcher (covering both arms).

Test Files  1 passed (1)
     Tests  7 passed (7)

Typecheck clean. Pure function + tests only — no rendering (keeps the #1957 render slice small).

Add a pure `classifyChangedFile(path)` that maps a changed file to one of five
review buckets — source / test / docs / config / generated — so the changed-files
summary (and future analytics) group deterministically. Pure + path-only (no diff
content, no IO); composes the existing hardened path-matchers.

- src/review/changed-files-classify.ts: fixed precedence generated > test > docs >
  config > source (documented), so a file matching several buckets — a generated test
  file, a lockfile, a vendored fixture — always resolves to the higher-precedence
  class. Reuses isGeneratedFile/isVendoredFile/isLockfile/isMinifiedFile,
  isTestFile/isTestPath, isDocsFile, isConfigFile.
- Distinct from path-matchers' 10-way `classifyChangedFile` (different category set
  AND precedence: that one ranks config above test), so it can't just fold it.
- test/unit/changed-files-classify.test.ts: one file per class, precedence conflict
  (vendored test file → generated), lockfile/vendored/minified/generated → generated,
  unknown → source; asserts the underlying matcher for each path.

Part of JSONbored#1957. Pure function + tests only — no rendering.
@nickmopen
nickmopen requested a review from JSONbored as a code owner July 5, 2026 16:39
@superagent-security

Copy link
Copy Markdown
Contributor

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

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

loopover-orb Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-05 16:44:18 UTC

2 files · 1 AI reviewer · no blockers · readiness 80/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

Review summary
Adds a small, pure, path-only classifier (`classifyChangedFile` in src/review/changed-files-classify.ts) that buckets changed files into source/test/docs/config/generated using a fixed precedence, built entirely by composing existing hardened matchers (isGeneratedFile, isVendoredFile, isLockfile, isMinifiedFile, isTestFile, isTestPath, isDocsFile, isConfigFile). The logic is straightforward and matches the documented precedence (generated > test > docs > config > source), and the tests exercise every bucket plus the stated precedence conflict (vendored+test → generated) and the unknown→source fallback. This PR intentionally ships no caller wiring (deferred to the #1957 render slice), so it's inert on its own but internally consistent and correctly closes #2143.

Nits — 5 non-blocking
  • This module exports a function named `classifyChangedFile` that is identical in name but different in signature/precedence from the existing `classifyChangedFile` in src/signals/path-matchers.ts — despite the code comment explaining the distinction, this is a real footgun for future imports/refactors (e.g. an IDE auto-import picking the wrong one); consider a more distinguishing name like `classifyReviewFileBucket`.
  • The module relies on two separate test-detection matchers (isTestFile from local-branch and isTestPath from test-evidence) ORed together — worth a one-line comment on why both are needed here (are they not equivalent/superset of each other?) so a future reader doesn't 'simplify' it down to one.
  • No caller wires this classifier into the changed-files summary table yet, so it's currently dead code from the app's perspective — acceptable per the PR description (deferred to a later PR) but worth flagging in review so it isn't lost.
  • Rename the new export to avoid collision with src/signals/path-matchers.ts's classifyChangedFile, or re-export it under a distinct alias where both might be imported together.
  • Add a short inline comment at the isTestFile || isTestPath line clarifying why both matchers are needed (e.g. one covers naming conventions the other doesn't) to preempt future dedup attempts.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #2143
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 ❌ 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: 121 registered-repo PR(s), 81 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor nickmopen; Gittensor profile; 121 PR(s), 0 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: nickmopen
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: TypeScript
  • Official Gittensor activity: 121 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Await review-lane availability.
  • Refresh registry data or choose a registered active repo.
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

@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.10%. Comparing base (f47f0b3) to head (aa00f97).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3547   +/-   ##
=======================================
  Coverage   93.10%   93.10%           
=======================================
  Files         302      303    +1     
  Lines       31496    31501    +5     
  Branches    11502    11506    +4     
=======================================
+ Hits        29325    29330    +5     
  Misses       1517     1517           
  Partials      654      654           
Files with missing lines Coverage Δ
src/review/changed-files-classify.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored merged commit 626b580 into JSONbored:main Jul 5, 2026
8 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.

feat(review): deterministic changed-file classifier (source/test/docs/config/generated)

2 participants