Skip to content

feat(enrichment): categorize the full binary inventory as assets - #3558

Closed
davion-knight wants to merge 1 commit into
JSONbored:mainfrom
davion-knight:feat/categorize-binary-inventory-v2
Closed

feat(enrichment): categorize the full binary inventory as assets#3558
davion-knight wants to merge 1 commit into
JSONbored:mainfrom
davion-knight:feat/categorize-binary-inventory-v2

Conversation

@davion-knight

Copy link
Copy Markdown
Contributor

Summary

categorizeFile (in review-enrichment/src/analysis-context.ts) assigns each changed file a category used for review scheduling/scope. Its asset check used a narrow hand-listed set of binary extensions — png/jpg/jpeg/gif/webp/pdf/zip/gz/zst — so every other binary the shared inventory already knows (mp4, mov, webm, woff2, ttf, wasm, node, safetensors, gguf, onnx, …) fell through to source, mislabeling a media / font / model-weight change as a source change.

This delegates the check to the shared binary-extension inventory:

  • Consistency anchor: BINARY_EXT_RE (from analyzers/binary-extensions) is the same inventory asset-weight and provenance already use; this replaces the duplicated list with it, matching the feat(enrichment): unify binary extension inventory for asset-weight and provenance #3252 unification (and the same consolidation direction as the merged feat(enrichment): skip all binary + lockfile files in history analyzers #3359).
  • .svg preserved: .svg is text (deliberately not in the binary inventory) but is still an asset for categorization, so it is kept explicitly.
  • Purely additive: only binaries that were previously source become asset; every one of the original 9 extensions is in the inventory, so no file that was asset changes, and no other category is affected. The full existing test suite (1034 tests) passes unchanged, confirming nothing relied on a binary being source.

No linked issue

This is a self-contained classification-coverage fix + de-duplication touching only review-enrichment/. It broadens the asset category to the shared inventory and removes a hand-maintained list; no new external behavior beyond correctly categorizing more binaries. It mirrors the accepted no-issue precedent for the same kind of enrichment change (e.g. the merged #3329 and #3359).

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • No linked issue — see the No linked issue section above.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage — no src/** lines changed (this change is under review-enrichment/, which Codecov does not measure), so codecov/patch has no diff to gate; suite is green.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New behavior has tests — a case in review-enrichment/test/analysis-context.test.ts drives createAnalysisContext and asserts .png (legacy), .mp4/.woff2/.wasm/.safetensors (newly covered) categorize as asset, .svg stays asset, and .ts stays source. npm run rees:test passes (1034 tests; analyzer-metadata check clean).

Validated green against the full GitHub CI validate-code check set — actionlint, db:migrations:check, db:schema-drift:check, cf-typegen:check, selfhost:validate-observability, typecheck, test:coverage, test:workers, build:mcp, test:mcp-pack, build:miner, rees:test, ui:openapi:check, ui:openapi:settings-parity, ui:version-audit, ui:lint, ui:typecheck, ui:test, ui:build. Branch rebased on latest main.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • No auth, cookie, CORS, GitHub App, Cloudflare, or session changes (N/A — internal categorization only).
  • No API/OpenAPI/MCP behavior change (N/A).
  • No UI changes (N/A — review-enrichment helper).
  • No visible UI change, so no UI Evidence section is required.
  • No docs/changelog changes needed.

Notes

Analogues followed end-to-end: the existing category tests in analysis-context.test.ts (the .zst asset, long-form docs, and case-insensitive lockfile cases), and the merged consolidation PRs #3329 and #3359. The .svg special-case preserves the pre-existing behavior exactly (svg was already an asset here); only genuinely-binary extensions are newly recognized.

categorizeFile only recognized a narrow hand-listed set of binary extensions
(png/jpg/jpeg/gif/webp/pdf/zip/gz/zst) as the `asset` category, so every other
binary the shared inventory already knows — mp4, mov, webm, woff2, wasm, node,
safetensors, gguf, onnx, and the rest — fell through to `source`, mislabeling
a media/font/model-weight change as a source change.

Delegate the check to the shared binary-extension inventory
(`BINARY_EXT_RE` from analyzers/binary-extensions, the same source asset-weight
and provenance use) instead of a duplicated list, matching the JSONbored#3252
unification. `.svg` is text (deliberately not in the binary inventory) but is
still an asset for categorization, so it is kept explicitly. Purely additive —
only binaries that were previously `source` become `asset`; no other category
changes.
@davion-knight
davion-knight requested a review from JSONbored as a code owner July 5, 2026 16:58
@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

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 Gittensory review result - reject/close recommended

Review updated: 2026-07-05 17:01:26 UTC

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

🛑 Suggested Action - Reject/Close

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This replaces a hand-maintained 9-extension binary allowlist in categorizeFile() (review-enrichment/src/analysis-context.ts:452-459) with a test against the shared BINARY_EXT_RE inventory already used by asset-weight/provenance, keeping .svg as an explicit asset since it's text and excluded from that inventory. The change is purely additive (widens `asset` classification to cover mp4/woff2/wasm/safetensors/etc. that previously fell through to `source`) and cannot affect files already classified earlier by the manifest/lockfile/workflow/config/docs checks, which all run first. The added test exercises the new cases plus a pre-existing case-sensitivity case (uppercase .ZST) that continues to pass per the green CI, confirming BINARY_EXT_RE matching is case-insensitive as required.

Nits — 6 non-blocking
  • review-enrichment/src/analysis-context.ts:455 — confirm BINARY_EXT_RE isn't declared with the global (`g`) flag, since `.test()` is now called repeatedly per file inside the `changedFiles.map()` in createAnalysisContext; a global regex would carry `lastIndex` state across calls and could intermittently misclassify files depending on call order (not observable from this diff alone).
  • The file crosses the 400-line long-file threshold per the external size-check brief — not a blocker, but worth a follow-up split of categorizeFile's growing branch list if more categories get added.
  • The inline comment block above the asset check (analysis-context.ts:450-452) is a bit verbose relative to the file's other category comments; could be tightened to one line consistent with the docs-extension comment above it.
  • Consider asserting in a test that every one of the original 9 extensions (png/jpg/jpeg/gif/webp/pdf/zip/gz/zst) is indeed present in BINARY_EXT_RE, so a future edit to the shared inventory can't silently narrow it and regress asset classification without a failing test.
  • Since this consolidates with feat(enrichment): unify binary extension inventory for asset-weight and provenance #3252/feat(enrichment): skip all binary + lockfile files in history analyzers #3359 precedent, a one-line comment pointing to the canonical BINARY_EXT_RE definition location (review-enrichment/src/analyzers/binary-extensions.ts) would help future readers avoid re-introducing a duplicated list.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.

Why this is blocked

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 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: 39 registered-repo PR(s), 28 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor davion-knight; Gittensor profile; 39 PR(s), 0 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: davion-knight
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: Rust
  • Official Gittensor activity: 39 PR(s), 0 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

@loopover-orb

loopover-orb Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Gittensory is closing this pull request on the maintainer's behalf (No linked issue detected; Maintainer requires a linked issue). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

@loopover-orb loopover-orb Bot closed this Jul 5, 2026
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant