fix(enrichment): fail closed on dependency scan truncation

[JSONbored]

Motivation

• The dependency-scanner silently truncated attacker-controlled inputs (manifest count, patch lines, dependency queries) which could hide vulnerable packages by processing only the first items and returning an apparently-successful empty result. This leads to a fail-open security-analysis bypass.
• The orchestrator treats any normal return as ok, so a silent truncation produced misleading analyzer status and an incomplete brief; truncation must be explicit and surfaced as a degraded analyzer.

Description

• Added DependencyScanTruncatedError and made the scanner fail closed by throwing when the manifest-file cap is exceeded, when a single file exceeds the per-file patch-line cap, or when the number of extracted dependency changes exceeds the dependency-query cap instead of silently slicing them.
• Replaced silent early break/slice behavior in extractDependencyChanges and scanDependencies with explicit checks that throw DependencyScanTruncatedError on over-cap inputs.
• Kept orchestrator behavior unchanged so thrown truncation errors are caught and result in analyzerStatus = "degraded" and partial = true in the brief.
• Added regression tests in review-enrichment/test/enrichment.test.ts covering dependency-query bypass, manifest/patch-line truncation, that truncated scans cause degraded briefs, and that abort-signal forwarding still works when not truncated.

Testing

• Ran unit tests for the review-enrichment package with cd review-enrichment && npm test, and all tests passed (41 tests, 0 failures).
• Ran the local gate npm run test:ci; it was invoked but failed early due to external tooling setup (actionlint fallback required network/DNS access), so full gate could not complete in this environment.
• Ran npm audit --audit-level=moderate in this environment and the audit endpoint returned a 403 (external registry limitation), so that check could not complete here.

---

Codex Task

[superagent-security]

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

[JSONbored]

Closing — this reverses a deliberate policy I shipped hours earlier in #1537 (104ac4b0), and it's a net availability regression. #1537 intentionally bifurcated the dependency scan: a timeout → degraded/partial, but an over-cap input → bounded best-effort (split("\n", maxPatchLinesPerFile) scans the first N lines; .slice(maxDependencyQueries) caps the OSV fan-out). This PR throws DependencyScanTruncatedError on any over-cap input, so a single oversized-but-benign manifest patch (>500 lines — entirely plausible for a big lockfile or dep-bump diff) now discards the entire dependency scan and drops every CVE finding, instead of scanning the first 500 lines. It also rewrites #1537's own pinning tests (the "caps manifest files and patch lines" assertion of ["first"] becomes assert.throws) with no issue link and no mention of #1537. Fail-open-bounded vs fail-closed-throw is a real maintainer tradeoff — and ideally a configurable one — but it's a considered decision to make against #1537, not a same-day one-shot reversal.

[loopover-orb]

Tip

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

✅ Gittensory review — safe to merge

2 files · 1 AI reviewers · no blockers · readiness 48/100 · CI green · unknown

✅ Approved — safe to merge

Review summary
The PR converts three silent-truncation paths in the dependency scanner — per-manifest-file cap, per-file patch-line cap, and per-scan query cap — from silent break/slice into explicit `DependencyScanTruncatedError` throws, which the existing orchestrator catch already surfaces as `analyzerStatus = 'degraded'` / `partial = true`. The logic is correct at all three sites: thresholds are preserved (manifests throw on file 21, patches throw at 501 lines, queries throw at 26 changes vs the 25-query default), and the four new test cases drive real production code paths — not fabricated states — covering each truncation axis, the orchestrator degraded-brief path, and abort-signal forwarding in the non-truncated case. The removed silent-cap test is correctly replaced by the fail-closed equivalents.

Signal	Result	Evidence
Code review	✅ No blockers	1 reviewers, synthesized
Linked issue	⚠️ Missing	No linked issue or no-issue rationale found.
Related work	⚠️ 1 scoped overlap	Top overlaps are listed below; lower-confidence bulk is hidden.
Review load	❌ 8/20	Readiness component derived from cached public PR metadata and labels; size label size:S.
Validation evidence	❌ 5/25	Cached preflight status is hold.
Open PR queue	❌ 3/10	26 open PR(s), 9 likely reviewable, 17 unlinked.
Contributor context	✅ Confirmed Gittensor contributor	JSONbored; Gittensor profile; 80 PR(s), 312 issue(s).
Gate result	✅ Passing	No configured blocker found.

Nits — 5 non-blocking

• dependency-scan.ts: `DependencyScanTruncatedError` embeds `reason` only in the message string (`dependency_scan_truncated:${reason}`); callers wanting to discriminate by cause for telemetry or logging must parse the message. Adding `readonly reason: string` to the class and assigning it in the constructor costs one line and makes branching on truncation cause safe.
• enrichment.test.ts (`buildBrief` degraded-brief test): `globalThis.fetch = okFetch([])` is set but the `DependencyScanTruncatedError` throw in `scanDependencies` fires before `fetch` is ever called — the stub is dead code in this test path. Harmless but misleading; drop the stub assignment or replace the test fixture with a comment explaining it is never reached.
• enrichment.test.ts (`manifest and patch-line truncation fail closed`): the patch-line sub-test hard-codes `501` lines against the live `MAX_PATCH_LINES_PER_FILE = 500` default. If the constant is bumped later, this sub-test silently becomes a boundary test for a different threshold. The companion manifest sub-test correctly uses `{ maxManifestFiles: 1 }` to decouple mechanism from production value — apply the same pattern here with `{ maxPatchLinesPerFile: N }` and a patch of `N + 1` lines.
• Add `readonly reason: string` to `DependencyScanTruncatedError` (set in the constructor alongside the message) so orchestrator logging and future telemetry can discriminate `manifest_file_limit` vs `patch_line_limit` vs `dependency_query_limit` without message parsing.
• In the patch-line sub-test inside `'manifest and patch-line truncation fail closed'`, replace the hard-coded 501/500 pair with an explicit limit option (e.g. `{ maxPatchLinesPerFile: 2 }` and a 3-line patch), matching how the manifest sub-test already handles it — this keeps the test stable against future constant changes.

Review context

• Author: JSONbored
• Role context: owner (maintainer lane)
• Public audience mode: oss maintainer
• Lane context: Repository registration is not available in the local Gittensory cache.
• Public profile languages: not available
• Official Gittensor activity: 80 PR(s), 312 issue(s).
• Related work: Titles/paths share 5 meaningful terms. (PR #1581)

Contributor next steps

• Treat this as maintainer-lane context rather than normal contributor-lane activity.
• Explain no-issue PR.
• Review top overlaps.
• Add scope summary.
• Fix blocker.
• Expect slower review.
• Refresh registry data or choose a registered active repo.
• Link the issue being solved, or explicitly explain why this is a no-issue PR.
• Check active issues and PRs before submitting.

Signal definitions

• Related work = same linked issue, overlapping active PRs, or title/path similarity.
• Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
• Open PR queue = repo-wide review pressure; it is not a PR quality failure.
• Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

Review details

Generated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative.

The PR converts three silent-truncation paths in the dependency scanner — per-manifest-file cap, per-file patch-line cap, and per-scan query cap — from silent break/slice into explicit `DependencyScanTruncatedError` throws, which the existing orchestrator catch already surfaces as `analyzerStatus = 'degraded'` / `partial = true`. The logic is correct at all three sites: thresholds are preserved (manifests throw on file 21, patches throw at 501 lines, queries throw at 26 changes vs the 25-query default), and the four new test cases drive real production code paths — not fabricated states — covering each truncation axis, the orchestrator degraded-brief path, and abort-signal forwarding in the non-truncated case. The removed silent-cap test is correctly replaced by the fail-closed equivalents.

Nits (5)

• dependency-scan.ts: `DependencyScanTruncatedError` embeds `reason` only in the message string (`dependency_scan_truncated:${reason}`); callers wanting to discriminate by cause for telemetry or logging must parse the message. Adding `readonly reason: string` to the class and assigning it in the constructor costs one line and makes branching on truncation cause safe.
• enrichment.test.ts (`buildBrief` degraded-brief test): `globalThis.fetch = okFetch([])` is set but the `DependencyScanTruncatedError` throw in `scanDependencies` fires before `fetch` is ever called — the stub is dead code in this test path. Harmless but misleading; drop the stub assignment or replace the test fixture with a comment explaining it is never reached.
• enrichment.test.ts (`manifest and patch-line truncation fail closed`): the patch-line sub-test hard-codes `501` lines against the live `MAX_PATCH_LINES_PER_FILE = 500` default. If the constant is bumped later, this sub-test silently becomes a boundary test for a different threshold. The companion manifest sub-test correctly uses `{ maxManifestFiles: 1 }` to decouple mechanism from production value — apply the same pattern here with `{ maxPatchLinesPerFile: N }` and a patch of `N + 1` lines.
• Add `readonly reason: string` to `DependencyScanTruncatedError` (set in the constructor alongside the message) so orchestrator logging and future telemetry can discriminate `manifest_file_limit` vs `patch_line_limit` vs `dependency_query_limit` without message parsing.
• In the patch-line sub-test inside `'manifest and patch-line truncation fail closed'`, replace the hard-coded 501/500 pair with an explicit limit option (e.g. `{ maxPatchLinesPerFile: 2 }` and a 3-line patch), matching how the manifest sub-test already handles it — this keeps the test stable against future constant changes.

_{🟩 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