fix: remove hardcoded 300-flows cap for large repositories#2198
fix: remove hardcoded 300-flows cap for large repositories#2198ChunxueLi wants to merge 4 commits into
Conversation
The dynamicMaxProcesses was capped at 300 via Math.min(300, ...), causing large repositories (280K+ nodes) to lose execution flows. Change: Remove the Math.min(300, ...) cap, keep dynamic calculation. Effect: 280K-node repo: 300 → 1617 flows.
|
Someone is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 14649 tests passed 62 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
I did a focused local pass on the cap behavior. This looks directionally right to me. What I checked:
I also checked the remaining guardrails in Only optional follow-up I would consider is adding a small regression around |
…#2198) Verify that processProcesses honours maxProcesses > 300 without truncation. Addresses the optional follow-up suggested by @koriyoshi2041.
|
Thanks for the thorough local validation @koriyoshi2041! I've added the regression test you mentioned as an optional follow-up. The test ( All 12 tests in the file pass, including the new one. |
|
Thanks for adding that regression test. That covers the gap I had in mind, and the latest CI is green apart from the usual Vercel fork authorization noise. No further concerns from my side. |
|
Friendly ping on this one — CI is all green and @koriyama2041 has already signed off. The regression test for 200 branching entry points (400 candidate processes) has been added. Happy to address any remaining concerns, otherwise it would be great to get this merged so large repositories are no longer truncated. Thanks! |
|
@abhigyanpatwari friendly ping — wondering if you have any thoughts on this one? @koriyama2041 has already signed off and CI is green. If there's a concern about removing the cap entirely for very large repos, I'm happy to rework this as a configurable threshold instead — something like a Alternatively, if you have a different approach in mind, I'd be glad to implement it. Would appreciate any feedback! Contributed by 卫宁健康科技集团 — WiNEX门诊医生站团队 |
magyargergo
left a comment
magyargergo
left a comment
There was a problem hiding this comment.
Review: remove hardcoded 300-flows cap (head f3ae65ee, merge-base 527ea5fc)
The one-line cap removal is correct and safe — verified with graph + source evidence below. Requesting changes on one issue only: the accompanying regression test doesn't actually cover the fix (inline comment).
Why the cap removal is safe
maxProcesses bounds work (the trace loop stops at maxProcesses * 2), but two untouched guards dominate after removal:
findEntryPointsslices to 200 entry points (process-processor.ts"Limit to prevent explosion")traceFromEntryPointcaps atmaxBranching(4) × 3 = 12traces per entry
→ hard ceiling ~2,400 traces / ~24K STEP_IN_PROCESS edges regardless of repo size. The O(n²) subset dedup grows from 600² to at worst 2,400² comparisons — bounded, small absolute cost. All MATCH (p:Process) consumers (wiki graph-queries.ts, local-backend.ts ×4) are LIMIT-bounded, and the embeddings pipeline does not index Process nodes. The "<3000 symbols unaffected" claim checks out (Math.min only bound at symbolCount/10 ≥ 300).
Blast radius (GitNexus, indexed at head)
detect_changes(compare, base=527ea5fc): onlyprocessesPhase/executechanged; 0 affected flows; risk LOW.impact(processesPhase, upstream): 0 dependents — only the pipeline runner invokes the phase; blast radius confined to index-time flow detection.context(processProcesses): exactly two callers —processesPhase.executeand the test file.
Coverage
12/12 tests pass at head, but the new regression test passes with the bug re-introduced too (empirically verified — see inline comment), so the changed line has no guard. Fix that and this is good to merge.
Verdict: REQUEST CHANGES
| // truncated the process index. The cap was removed so dynamicMaxProcesses | ||
| // grows with symbolCount. This test verifies that processProcesses honours a | ||
| // large maxProcesses (>300) without truncation, mirroring the phase-level fix. | ||
| it('honours maxProcesses > 300 (no hardcoded cap, #2198)', async () => { |
There was a problem hiding this comment.
[MEDIUM] This regression test does not cover the fix — it passes with the bug re-introduced.
The test calls processProcesses directly with maxProcesses: 400, but processProcesses never contained the 300 cap — that cap lived only in processesPhase (pipeline-phases/processes.ts:56), which this test never exercises. Its import graph (process-processor.js, graph.js, community-processor.js) excludes the changed file entirely.
Empirically verified: re-adding Math.min(300, ...) to processes.ts at this head → all 12 tests in this file still pass, including this one. So the comment on line 590 ("would be silently capped to 300 before the fix") is false at this call site — processProcesses honoured 400 before the fix as well. Someone re-introducing the cap in the phase would ship with green tests.
Suggested fix (either):
- Extract the sizing into an exported helper, e.g.
computeDynamicMaxProcesses(symbolCount)inprocesses.ts, and assertcomputeDynamicMaxProcesses(3010) > 300/ grows withsymbolCount; or - Test
processesPhase.executewithvi.mock('../process-processor.js')capturing themaxProcessesactually passed for a graph with >3,000 non-File nodes.
| if (n.label !== 'File') symbolCount++; | ||
| }); | ||
| const dynamicMaxProcesses = Math.max(20, Math.min(300, Math.round(symbolCount / 10))); | ||
| const dynamicMaxProcesses = Math.max(20, Math.round(symbolCount / 10)); |
There was a problem hiding this comment.
The removal itself checks out (non-blocking, evidence for the record): work stays bounded after this change because findEntryPoints caps at 200 entry points and traceFromEntryPoint yields ≤ maxBranching × 3 = 12 traces per entry — a hard ceiling of ~2,400 traces no matter how large dynamicMaxProcesses gets. Worst-case cost delta is the O(n²) subset dedup growing from 600² to 2,400² key comparisons, which is fine. Downstream, every MATCH (p:Process) read is LIMIT-bounded and Process nodes aren't embedded, so no consumer scales badly with the higher flow count.
|
Thanks for keeping this updated. I rechecked the current head against current The actual code change looks directionally good to me. Removing the fixed The remaining blocker is the regression test. The new test calls I would update the coverage to exercise the layer that actually changed. Either extract and test a helper such as |
azizur100389
left a comment
azizur100389
left a comment
There was a problem hiding this comment.
Requesting changes on test coverage only.
The code change in gitnexus/src/core/ingestion/pipeline-phases/processes.ts looks correct and bounded by the existing process detection guards.
The regression test in gitnexus/test/unit/process-processor.test.ts does not protect the changed line, though. It calls processProcesses directly with maxProcesses: 400, but the removed Math.min(300, ...) cap lived in processesPhase.execute before processProcesses is called. If that cap were reintroduced, this test would still pass.
Please update the test to cover the real changed path, either by testing processesPhase.execute with a large symbol graph or by extracting/testing a small dynamic max-process calculation helper with symbolCount > 3000.
4 participants
Problem
The
dynamicMaxProcesseswas capped at 300 viaMath.min(300, ...), causing large repositories (280K+ nodes) to lose execution flows.Solution
Remove the
Math.min(300, ...)cap, keep dynamic calculation based on symbol count.Effect
Code Change
Testing
Verified on a 280,951-node Java repository:
Small repositories (< 3000 symbols) are unaffected (dynamic calculation already < 300).
Contributed by 卫宁健康科技集团 — WiNEX门诊医生站团队