perf(global-graph): batch repository updates - #3444
Conversation
ROHIT8759
commented
Sep 9, 2026
ROHIT8759
commented
…global-add-many
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Replaces global_add with global_add_many, which processes a batch of (path, tag) sources in one manifest load — deduplicating externals across the whole batch, computing a batch-total cross_repo_calls, and rejecting duplicate tags or missing source files up front. Adds an on_error policy ("abort" vs "skip") so a failing source can either halt the batch or be recorded as failed while the rest proceed, with per-repo changes staged and rolled back on error rather than partially applied. Extends graphify global add to accept multiple graph paths plus a --keep-going flag, still inferring each repo tag from the path (or requiring --as for a single graph), and prints per-source added/skipped/failed lines.
Worth a look
- Load/size-check errors occur after prune, so error rollback path (except) does not restore pruned nodes on skip —
graphify/global_graph.py· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- on_error='skip' failure leaves repo's stale nodes deleted but manifest unchanged, corrupting graph vs manifest consistency —
graphify/global_graph.py· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Rollback removes nodes by data['repo'] but prefixed nodes may not carry repo attr —
graphify/global_graph.py:217· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- on_error='skip' persists partial mutations for failed repo without full rollback —
graphify/global_graph.py:240· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- --as can consume --keep-going as the repo tag —
graphify/cli.py:3122· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 333 functions depend on the 124 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 125 callees - new:
global_add_many()— 18 callers, 8 callees - new:
_stale_graph_sources()— 7 callers, 6 callees - new:
_run_hook_guard()— 4 callers, 8 callees - new:
global_remove()— 5 callers, 5 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees - new:
test_global_add_many_replacement_failure_preserves_original()— 0 callers, 6 callees
Verification — 333 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 279 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
25 of 263 test file(s) selected (10%) via static blast radius.
tests/test_affected_cli.py— impacttests/test_agents_platform.py— impacttests/test_cli_global_add_many.py— changed-testtests/test_codebuddy.py— impacttests/test_devin.py— impacttests/test_explain_cli.py— impacttests/test_extract_cli.py— impacttests/test_global_graph.py— impact, changed-testtests/test_god_nodes_cli.py— impacttests/test_hollow_chunks_arm_shrink_guard.py— impacttests/test_hook_guard_token_match.py— impacttests/test_hook_out_of_project_paths.py— impacttests/test_hook_strict.py— impacttests/test_incomplete_build_guard.py— impacttests/test_install.py— impacttests/test_install_references.py— impacttests/test_merge_chunks_validation.py— impacttests/test_multigraph_diagnostics.py— impacttests/test_no_dedup_flag.py— impacttests/test_partial_cache.py— impacttests/test_path_cli.py— impacttests/test_query_cli.py— impacttests/test_query_induced_edges.py— impacttests/test_stale_prune.py— impacttests/test_unverified_semantic_shrink.py— impact
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
Could not verify: Could not verify global\_add.
The verifier did not have enough to check global\_add, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `source_path` is annotated `Path` — outside the synthesizable primitive/collection set
· 3 grounded finding(s) anchored inline below; 4 more finding(s) on lines outside this diff (see the check run).
|
|
||
| def global_add(source_path: Path, repo_tag: str) -> dict: | ||
| """Add or update a project graph in the global graph. | ||
| def global_add_many( |
There was a problem hiding this comment.
global_add_many()
fans out to 8 callees (efferent coupling); 18 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| return global_add_many([(source_path, repo_tag)])[0] | ||
|
|
||
|
|
||
| def global_remove(repo_tag: str) -> int: |
There was a problem hiding this comment.
global_remove()
high coupling complexity (Ca·Ce = 25).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| # It does not scale with the number of repositories. | ||
| assert scan_count == 2 | ||
|
|
||
| def test_global_add_many_replacement_failure_preserves_original(tmp_path): |
There was a problem hiding this comment.
test_global_add_many_replacement_failure_preserves_original()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
There was a problem hiding this comment.
🟡 Changes recommended
The batch-merge rollback and persistence logic has edge cases that can leave partial mutations or write to disk even when nothing was applied, and the new CLI subprocess tests currently aren’t isolated from the user’s real ~/.graphify state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a batched global-graph update pathway so multiple repository graphs can be merged into the global graph with a single load → resolve → save cycle, addressing the repeated O(K²)-style work described in #3438.
Changes:
- Added
global_add_many()as the primary batch API and refactoredglobal_add()to delegate to it. - Reworked the merge flow to initialize external-label and repo-node indexes once per batch, and to support rollback on apply-time failures.
- Updated the CLI to accept multiple graph paths and
--keep-going, and added regression tests for batch equivalence and CLI behavior.
File summaries
| File | Description |
|---|---|
graphify/global_graph.py |
Adds global_add_many() batching + rollback logic and delegates global_add() to it. |
graphify/cli.py |
Extends graphify global add to accept multiple graphs and --keep-going mapped to on_error="skip". |
tests/test_global_graph.py |
Adds extensive regression coverage for batching, skip/abort, replacement, rollback, and cross-repo call resolution. |
tests/test_cli_global_add_many.py |
Adds subprocess-based CLI tests for single/multi add, --as validation, and inferred-tag errors. |
Review details
Suppressed comments (5)
tests/test_cli_global_add_many.py:22
- This subprocess-based CLI test currently writes to the real user HOME (~/.graphify) because graphify.global_graph uses Path.home(); set HOME (and PYTHONPATH) in the subprocess env to isolate test state under tmp_path.
res = subprocess.run([sys.executable, "-m", "graphify", "global", "add", str(g), "--as", "myrepo"], capture_output=True, text=True)
tests/test_cli_global_add_many.py:37
- This subprocess-based CLI test currently writes to the real user HOME (~/.graphify) because graphify.global_graph uses Path.home(); set HOME (and PYTHONPATH) in the subprocess env to isolate test state under tmp_path.
res = subprocess.run([sys.executable, "-m", "graphify", "global", "add", str(g1), str(g2)], capture_output=True, text=True)
tests/test_cli_global_add_many.py:53
- This subprocess-based CLI test currently writes to the real user HOME (~/.graphify) because graphify.global_graph uses Path.home(); set HOME (and PYTHONPATH) in the subprocess env to isolate test state under tmp_path.
res = subprocess.run([sys.executable, "-m", "graphify", "global", "add", str(g1), str(g2), "--keep-going"], capture_output=True, text=True)
tests/test_cli_global_add_many.py:64
- This subprocess-based CLI test should set HOME (and PYTHONPATH) in the subprocess env to avoid depending on or mutating ~/.graphify on the machine running the test suite.
res = subprocess.run([sys.executable, "-m", "graphify", "global", "add", str(g1), str(g2), "--as", "myrepo"], capture_output=True, text=True)
tests/test_cli_global_add_many.py:77
- This subprocess invocation sets PYTHONPATH but not HOME; setting HOME to tmp_path isolates graphify's global graph location (Path.home()/.graphify) so the test doesn't depend on or mutate the user's real ~/.graphify.
env = os.environ.copy()
env["PYTHONPATH"] = str(Path.cwd())
res = subprocess.run([sys.executable, "-m", "graphify", "global", "add", "graph.json"], cwd=tmp_path, env=env, capture_output=True, text=True)
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| nodes_to_add = [(n, d) for n, d in prefixed.nodes(data=True) if n not in remap] | ||
| edges_to_add = [] | ||
| for u, v, data in prefixed.edges(data=True): | ||
| u = remap.get(u, u) | ||
| v = remap.get(v, v) | ||
| if u != v: | ||
| edges_to_add.append((u, v, data)) | ||
|
|
||
| # --- APPLY --- | ||
| for node, data in nodes_to_add: | ||
| G.add_node(node, **data) | ||
| if not data.get("source_file") and data.get("label"): | ||
| external_labels[data["label"]] = node | ||
|
|
||
| for u, v, data in edges_to_add: | ||
| G.add_edge(u, v, **data) | ||
|
|
||
| except Exception as apply_error: | ||
| G.remove_nodes_from([n for n, d in G.nodes(data=True) if d.get("repo") == repo_tag]) | ||
| G.add_nodes_from(to_remove_nodes) | ||
| G.add_edges_from(to_remove_edges) | ||
| external_labels.clear() | ||
| external_labels.update(old_external_labels) | ||
| raise apply_error |
| cross_repo_calls = link_cross_repo_member_calls(G) | ||
| _save_global_graph(G) | ||
|
|
||
| manifest["repos"][repo_tag] = { | ||
| "added_at": datetime.now(timezone.utc).isoformat(), | ||
| "source_path": str(source_path.resolve()), | ||
| "node_count": added, | ||
| "edge_count": prefixed.number_of_edges(), | ||
| "source_hash": src_hash, | ||
| } | ||
| _save_global_graph(G) | ||
| _save_manifest(manifest) |
|
|
||
| res = subprocess.run([sys.executable, "-m", "graphify", "global", "add", str(g)], capture_output=True, text=True) |
| # The node for X in repoB should have repo label repoB, because the old one was from repoA and was deleted. | ||
| # Wait, external nodes in prefixed graph don't necessarily get repo tag prefix. Actually they do get repo in prefix_graph_for_global. | ||
| # The node from repoA should be deleted. |
| # Should exactly be 2 scans (1 for our upfront index, 1 for cross_repo_calls pass)! | ||
| # It does not scale with the number of repositories. | ||
| assert scan_count == 2 |
