Skip to content

fix(review): skip lockfiles in RAG via the canonical isLockfile set - #8678

Merged
JSONbored merged 1 commit into
JSONbored:mainfrom
shin-core:fix/rag-lockfile-skip-canonical-8649
Jul 25, 2026
Merged

fix(review): skip lockfiles in RAG via the canonical isLockfile set#8678
JSONbored merged 1 commit into
JSONbored:mainfrom
shin-core:fix/rag-lockfile-skip-canonical-8649

Conversation

@shin-core

Copy link
Copy Markdown
Contributor

What & why

Closes #8649.

src/review/rag.ts's SKIP_FILE_RE hand-listed 8 lockfile names plus a generic \.lock$ suffix pattern instead of delegating to the canonical LOCKFILE_NAMES/isLockfile set (packages/loopover-engine/src/signals/path-matchers.ts). The generic suffix coincidentally catches most of the canonical set, but structurally cannot catch lockfile names that don't end in .lock:

  • npm-shrinkwrap.json (a valid npm lockfile format) and packages.lock.json (.NET NuGet) both fail SKIP_FILE_RE and have a .json extension matched by CODE_EXT_RE, so classifyRepoFile returned "code" — chunking those large machine-generated files into the RAG index, burning embedding budget and polluting retrieval.

The adjacent DOC_EXT_RE in the same file already received the equivalent drift fix ("mirror the canonical DOCS set"); the lockfile list never got the same treatment.

The fix

Delegate the lockfile-name check to the canonical isLockfile() in classifyRepoFile, and reduce SKIP_FILE_RE to only the suffix-shaped skips (minified bundles, sourcemaps, the generic .lock suffix, snapshots). Future additions to the canonical LOCKFILE_NAMES set now propagate automatically.

Tests

  • npm-shrinkwrap.json and packages.lock.json (incl. a nested path) now classify "skip" (were "code").
  • No regression: the 8 already-listed lockfiles (package-lock.json, pnpm-lock.yaml, go.sum, yarn.lock, bun.lockb, cargo.lock, poetry.lock, composer.lock) still skip.
  • Verified bug-catching: removing the isLockfile check fails the npm-shrinkwrap.json assertion.

Validation

  • test/unit/rag.test.ts: 111 tests pass; typecheck clean; engine-parity:drift-check OK (this only consumes the engine's isLockfile, changes no twin); 100% of changed lines/branches covered.
  • Branched off current main, mergeable-clean.

…not a hand-rolled list

rag.ts's SKIP_FILE_RE hand-listed 8 lockfile names plus a generic `.lock` suffix,
which structurally can't catch lockfile names that don't end in .lock --
npm-shrinkwrap.json and packages.lock.json (.NET NuGet) both have a .json
extension matched by CODE_EXT_RE, so classifyRepoFile returned "code" and chunked
those large machine-generated files into the RAG index, burning embedding budget.
Delegate the lockfile check to the canonical isLockfile()/LOCKFILE_NAMES set
(path-matchers.ts), mirroring how DOC_EXT_RE already delegates to the canonical
DOCS set, so future canonical additions propagate automatically. The 8
already-skipped names still skip (no regression).
@shin-core
shin-core requested a review from JSONbored as a code owner July 25, 2026 23:21
@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 25, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.66%. Comparing base (ee390f5) to head (81b5323).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8678      +/-   ##
==========================================
- Coverage   93.77%   90.66%   -3.11%     
==========================================
  Files         798       97     -701     
  Lines       79475    22747   -56728     
  Branches    24082     3956   -20126     
==========================================
- Hits        74525    20623   -53902     
+ Misses       3565     1945    -1620     
+ Partials     1385      179    -1206     
Flag Coverage Δ
backend 99.61% <100.00%> (+4.57%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/review/rag.ts 99.61% <100.00%> (ø)

... and 701 files with indirect coverage changes

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

loopover-orb Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-25 23:37:00 UTC

2 files · 1 AI reviewer · no blockers · CI green · unstable

⏸️ Suggested Action - Manual Review

Review summary
This diff swaps `rag.ts`'s hand-rolled 8-name lockfile regex for a call to the canonical `isLockfile()` from `path-matchers.ts`, closing the gap where `npm-shrinkwrap.json` / `packages.lock.json` (which end in `.json`, not `.lock`) fell through to `CODE_EXT_RE` and got indexed as code. `SKIP_FILE_RE` is correctly reduced to only the suffix-shaped skips (minified/map/lock/snap), and `isLockfile(path)` is added as its own OR-branch in `classifyRepoFile` ahead of the doc/code checks, so a lockfile is skipped regardless of its extension. New tests directly assert `npm-shrinkwrap.json`, a nested `npm-shrinkwrap.json`, and `packages.lock.json` now return `"skip"`, and the previously-hardcoded 8 lockfiles are re-verified as a no-regression check.

Nits — 3 non-blocking
  • test/unit/rag.test.ts:91-92's comment ("go.sum stays skipped … SKIP_FILE_RE's lockfile check runs before ALLOW_EXTLESS_RE") is now stale — the lockfile check moved to the separate `isLockfile()` branch in `classifyRepoFile`, not `SKIP_FILE_RE`; worth a one-line update so a future reader isn't misled about where the guard lives.
  • `isLockfile`'s implementation isn't in this diff or the provided file content, so I can't independently verify it does a basename match (vs. requiring an exact full-path match) — the nested-path test (`nested/dir/npm-shrinkwrap.json`) implies basename matching, which lines up with the description, but flagging since I couldn't trace it directly.
  • Consider a follow-up comment/link in `path-matchers.ts` noting `rag.ts` now depends on `isLockfile`'s output shape, so a future rename there doesn't silently reopen fix(orb): RAG indexer's lockfile skip-list misses npm-shrinkwrap.json and packages.lock.json #8649 without a compile error (import breakage would catch a rename, but not a semantic change to what counts as a lockfile).
Flagged checks (non-blocking)
  • Contributor trust — Contributor flagged for review

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8649
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), 23 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor shin-core; Gittensor profile; 48 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The diff delegates lockfile detection to the canonical isLockfile() helper in classifyRepoFile, trims SKIP_FILE_RE to only suffix-shaped patterns, and adds tests confirming npm-shrinkwrap.json, packages.lock.json (including nested paths), and the 8 previously-listed lockfiles all classify as "skip".

Review context
  • Author: shin-core
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: TypeScript, JavaScript, Solidity, Dart, Python, CSS, PHP, Rust
  • Official Gittensor activity: 48 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
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.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

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

Labels

contributor:flagged Contributor flagged for review by trust analysis. gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(orb): RAG indexer's lockfile skip-list misses npm-shrinkwrap.json and packages.lock.json

2 participants