Skip to content

fix(queue): consolidate duplicate mapWithConcurrency helpers#6720

Closed
RealDiligent wants to merge 2 commits into
JSONbored:mainfrom
RealDiligent:feat/consolidate-map-with-concurrency-6602
Closed

fix(queue): consolidate duplicate mapWithConcurrency helpers#6720
RealDiligent wants to merge 2 commits into
JSONbored:mainfrom
RealDiligent:feat/consolidate-map-with-concurrency-6602

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

  • Delegate mapWithConcurrencyLimit and mapPatchLessSecretScanFilesWithConcurrency to the canonical mapWithConcurrency helper so the bounded-concurrency worker-pool loop lives in one place.
  • Preserve existing export names and call signatures; no behavior change for callers.

Closes #6602

Test plan

  • Existing patchless-secret-scan / focus-manifest / processors suites pass
  • Codecov patch gate green on changed lines

Delegate focus-manifest and patchless-secret-scan wrappers to the canonical map-with-concurrency helper so the worker-pool loop cannot drift.

Closes JSONbored#6602

Co-authored-by: Cursor <cursoragent@cursor.com>
@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 16, 2026 23:55
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@3f6591d). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6720   +/-   ##
=======================================
  Coverage        ?   95.87%           
=======================================
  Files           ?      599           
  Lines           ?    47536           
  Branches        ?    15070           
=======================================
  Hits            ?    45575           
  Misses          ?     1163           
  Partials        ?      798           
Flag Coverage Δ
shard-1 43.86% <100.00%> (?)
shard-2 36.92% <100.00%> (?)
shard-3 32.37% <0.00%> (?)
shard-4 34.24% <50.00%> (?)
shard-5 31.43% <50.00%> (?)
shard-6 45.78% <50.00%> (?)

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

Files with missing lines Coverage Δ
src/api/routes.ts 94.62% <ø> (ø)
src/queue/patchless-secret-scan.ts 100.00% <100.00%> (ø)
src/signals/focus-manifest-loader.ts 95.76% <100.00%> (ø)

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

loopover-orb Bot commented Jul 17, 2026

Copy link
Copy Markdown

Caution

🛑 LoopOver review result - reject/close recommended

Review updated: 2026-07-17 00:07:36 UTC

3 files · 1 AI reviewer · 1 blocker · CI green · clean

🛑 Suggested Action - Reject/Close

Review summary
This PR replaces the two duplicated bounded-concurrency loop implementations in src/queue/patchless-secret-scan.ts and src/signals/focus-manifest-loader.ts with calls to a shared mapWithConcurrency helper, preserving the original 3-arg (items, limit, mapper) call shape and export names — a reasonable, narrowly-scoped consolidation if the canonical helper (not shown in this diff) implements the same nextIndex-cursor semantics. However, the diff also silently drops two unrelated imports (buildFindingTaxonomyDocument, buildEnrichmentAnalyzersTaxonomyDocument) from src/api/routes.ts with no explanation in the PR description, and CI shows validate/validate-code and every validate-tests shard failing on this commit, which is consistent with those symbols still being referenced elsewhere in routes.ts.

Blockers

  • src/api/routes.ts:194-196 drops the buildFindingTaxonomyDocument and buildEnrichmentAnalyzersTaxonomyDocument imports with zero mention in the PR description (which only claims a mapWithConcurrency consolidation) — if either symbol is still referenced later in routes.ts this is a ReferenceError at runtime/build time, and the CI FAILED status on validate/validate-code/all validate-tests shards is consistent with exactly that breakage; confirm no remaining call sites before merging or drop this unrelated hunk from the PR.
Nits — 5 non-blocking
  • The canonical mapWithConcurrency implementation isn't shown in this diff — verify it preserves the exact same 'first-available-slot claims next index' semantics as the two removed implementations (no behavior drift for error propagation or ordering).
  • PR description says 'preserve existing export names and call signatures' but doesn't mention the routes.ts import removal at all, making the diff harder to review against its own stated scope.
  • Split the routes.ts import removal into its own PR (or add a sentence in the description explaining why it's bundled here) so reviewers can verify it independently of the concurrency consolidation.
  • Add or point to a unit test asserting mapWithConcurrency's canonical implementation matches the removed loops' ordering/error-propagation behavor, since this refactor has no test-path changes per the engine classifier.
  • Code changes lack test evidence — Add focused regression tests or explain why existing coverage is sufficient.

Why this is blocked

  • src/api/routes.ts:194-196 drops the buildFindingTaxonomyDocument and buildEnrichmentAnalyzersTaxonomyDocument imports with zero mention in the PR description (which only claims a mapWithConcurrency consolidation) — if either symbol is still referenced later in routes.ts this is a ReferenceError at runtime/build time, and the CI FAILED status on validate/validate-code/all validate-tests shards is consistent with exactly that breakage; confirm no remaining call sites before merging or drop this unrelated hunk from the PR.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. src/api/routes.ts:194-196 drops the buildFindingTaxonomyDocument and buildEnrichmentAnalyzersTaxonomyDocument imports with zero mention in the PR description \(which only claims a mapWithConcurrency consolidation\) — if either symbol is still referenced later in routes.ts this is a ReferenceError at runtime/build time, and the CI FAILED status on validate/validate-code/all validate-tests shards is consistent with exactly that breakage; confirm no remaining call sites before merging or drop this unrelated hunk from the PR.

Decision drivers

  • ❌ Code review — 1 blocker (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #6602
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ❌ 8/20 High 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: 35 registered-repo PR(s), 10 merged, 33 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 35 PR(s), 33 issue(s).
Improvement ℹ️ None detected risk: low · value: none · LLM: moderate
Review context
  • Author: RealDiligent
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, Ruby, Svelte, TypeScript, Markdown, MDX
  • Official Gittensor activity: 35 PR(s), 33 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Add a concise scope and risk note.
  • Then work through the remaining 1 step in the Signals table above.
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

@RealDiligent

Copy link
Copy Markdown
Contributor Author

Pushed a follow-up that removes duplicate buildFindingTaxonomyDocument / buildEnrichmentAnalyzersTaxonomyDocument imports in src/api/routes.ts (broken on current main after overlapping taxonomy REST merges). Needed so validate-code can pass for this PR.

Overlapping merges left the same two imports twice; TypeScript fails validate-code on main without this.

Co-authored-by: Cursor <cursoragent@cursor.com>
@loopover-orb

loopover-orb Bot commented Jul 17, 2026

Copy link
Copy Markdown

LoopOver is closing this pull request on the maintainer's behalf (AI reviewers agree on a likely critical defect: src/api/routes.ts:194-196 drops the buildFindingTaxonomyDocument and buildEnrichmentAnalyzersTaxonomyDocument imports with zero mention in the PR description (which only claims a mapWithConcurrency consolidation) — if either symbol is still referenced later in routes.ts this is a ReferenceError at runtime/build time, and the CI FAILED status on validate/validate-code/all validate-tests shards is consistent with exactly that breakage; confirm no remaining call sites before merging or drop this unrelated hunk from the PR.). 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 17, 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.

Consolidate duplicate bounded-concurrency map helpers in src/queue and src/signals

1 participant