fix(extract): reject invalid CLI arguments - #2026
Conversation
5f293e1 to
ec6a901
Compare
| print(f"error: path not found: {target}", file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
| backend: str | None = None |
There was a problem hiding this comment.
The argument parsing logic is easy to follow. Would using argparse simplify maintenance as the number of CLI options continues to grow?
There was a problem hiding this comment.
Thanks. argparse could make sense as a broader CLI refactor. This PR is intentionally focused on validating the existing extract arguments.
| "cli_cargo": False, | ||
| "cli_allow_partial": False, | ||
| "no_cluster": False, | ||
| "dedup_llm": False, |
There was a problem hiding this comment.
Nice validation for the target path. Should we also validate that out_dir is writable before proceeding to avoid failing later in the execution?
There was a problem hiding this comment.
Good point. I would handle this in a follow-up by catching the actual OSError, since a pre-check can become stale.
| code_only: bool = parsed["code_only"] | ||
| no_gitignore: bool = parsed["no_gitignore"] | ||
| global_repo_tag: str | None = parsed["global_repo_tag"] | ||
| cli_max_workers: int | None = parsed["cli_max_workers"] |
There was a problem hiding this comment.
I like the dedicated _parse_int() helper. Would it make sense to follow the same pattern for float(args[i + 1]) (e.g. --exclude-hubs) to keep error handling consistent across numeric arguments?
There was a problem hiding this comment.
Yes. This PR already routes --exclude-hubs through _parse_percentile() for both spaced and = forms, with consistent conversion and range errors.
ec6a901 to
dd9ec20
Compare
![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. 6 change(s) tested, no difference found (not proven).
Graphify review — findings
Rewrites ARCHITECTURE.md to match the actual code: real entry-point signatures per module (detect, extract/collect_files, build, cluster, analyze helpers, report.generate, export.to_*, wiki, etc.), a note that tests/test_architecture_doc.py imports every symbol so the table can't drift, and a new section on calling extract() with a path list and explicit root. Updates CHANGELOG.md with 0.9.47/0.9.48 release notes. The changed test/symbol list reflects those doc-import checks and the corresponding extractor/build/serve/watch behavior covered by the release fixes.
Worth a look
- Project license changed from MIT to Apache-2.0 —
LICENSE:1· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Swift noise labels are applied globally across all languages —
graphify/analyze.py:23· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- _derive_prune_root can produce empty-string root as valid —
graphify/build.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- _prune_match side-effect pollutes matched-entries during root-derivation probe —
graphify/build.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- build_merge default directed behavior changed for existing graphs —
graphify/build.py· 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 — 9484 functions depend on the 8673 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 474 callers, 42 callees - new:
_rebuild_code()— 98 callers, 50 callees - new:
build_from_json()— 185 callers, 18 callees - new:
detect()— 108 callers, 15 callees - new:
deduplicate_entities()— 63 callers, 21 callees - new:
build_merge()— 46 callers, 14 callees - new:
save_semantic_cache()— 55 callers, 11 callees - new:
_extract_generic()— 18 callers, 24 callees - …and 198 more — each is listed as a finding
Verification — 9484 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit b2cd362 (diverged from this PR's base — delta is approximate).
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: 9484 function(s) in the blast radius were not formally verified this run
Formal verification
Verification did not run: Verification did not run for affected\_nodes.
The verification could not execute (an environment/toolchain issue, not a statement about the code).
Guarantee: No guarantee, the check itself did not complete.
Note: Detail: exec: NameError
No difference found (not proven): No behavior difference found in format\_affected (not a proof).
The verifier ran both versions of format\_affected on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify resolve\_seed.
The verifier did not have enough to check resolve\_seed, 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 `root` is annotated `Path | None` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_query\_subgraph\_tokens (not a proof).
The verifier ran both versions of \_query\_subgraph\_tokens on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify run\_benchmark.
The verifier did not have enough to check run\_benchmark, 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: non-vacuity: domain too small (only 2 distinct inputs exercised, need 3) — 'no divergence' would be near-vacuous
Could not verify: Could not verify \_abs\_identity.
The verifier did not have enough to check \_abs\_identity, 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: non-vacuity: domain too small (only 2 distinct inputs exercised, need 3) — 'no divergence' would be near-vacuous
Could not verify: Could not verify build.
The verifier did not have enough to check build, 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 9 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly AttributeError — names the real obstacle, not a sampling gap)
Could not verify: Could not verify build\_from\_json.
The verifier did not have enough to check build\_from\_json, 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 6 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly NameError — names the real obstacle, not a sampling gap)
Could not verify: Could not verify build\_merge.
The verifier did not have enough to check build\_merge, 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 `graph_path` is annotated `str | Path | None` — outside the synthesizable primitive/collection set
Could not verify: Could not verify graph\_has\_legacy\_ids.
The verifier did not have enough to check graph\_has\_legacy\_ids, 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 `root` is annotated `str | Path | None` — outside the synthesizable primitive/collection set
Could not verify: Could not verify \_infer\_merge\_root.
The verifier did not have enough to check \_infer\_merge\_root, 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 `graph_path` is annotated `Path` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_norm\_source\_file (not a proof).
The verifier ran both versions of \_norm\_source\_file on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
No difference found (not proven): No behavior difference found in \_normalize\_hyperedge\_members (not a proof).
The verifier ran both versions of \_normalize\_hyperedge\_members on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify prefix\_graph\_for\_global.
The verifier did not have enough to check prefix\_graph\_for\_global, 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 115 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous
No difference found (not proven): No behavior difference found in \_semantic\_id\_remap (not a proof).
The verifier ran both versions of \_semantic\_id\_remap on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify cache\_dir.
The verifier did not have enough to check cache\_dir, 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 `root` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify cached\_word\_count.
The verifier did not have enough to check cached\_word\_count, 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 `path` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify check\_semantic\_cache.
The verifier did not have enough to check check\_semantic\_cache, 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 `root` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify \_ensure\_stat\_index.
The verifier did not have enough to check \_ensure\_stat\_index, 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 `root` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify file\_hash.
The verifier did not have enough to check file\_hash, 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 `path` is annotated `Path` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_flush\_stat\_index (not a proof).
The verifier ran both versions of \_flush\_stat\_index on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify load\_cached.
The verifier did not have enough to check load\_cached, 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 `path` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify \_relativize\_source\_files\_in.
The verifier did not have enough to check \_relativize\_source\_files\_in, 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 `root` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify save\_cached.
The verifier did not have enough to check save\_cached, 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 `path` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify save\_semantic\_cache.
The verifier did not have enough to check save\_semantic\_cache, 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 `root` is annotated `Path` — outside the synthesizable primitive/collection set
· 20 grounded finding(s) anchored inline below; 165 more finding(s) on lines outside this diff (see the check run); 21 additional anchorable finding(s) not shown (cap).
|
|
||
|
|
||
| def resolve_seed(graph: nx.Graph, query: str) -> str | None: | ||
| def resolve_seed(graph: nx.Graph, query: str, root: Path | None = None) -> str | None: |
There was a problem hiding this comment.
resolve_seed()
11 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| return meta.get("mcp_kind") in _MCP_GLOBAL_ID_KINDS | ||
|
|
||
|
|
||
| def graph_has_legacy_ids(nodes: list, root: str | Path | None = None, sample: int = 300) -> bool: |
There was a problem hiding this comment.
graph_has_legacy_ids()
fans out to 8 callees (efferent coupling); 10 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| ) | ||
|
|
||
|
|
||
| def merge_raw_extraction( |
There was a problem hiding this comment.
merge_raw_extraction()
fans out to 9 callees (efferent coupling); 6 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| return new | ||
|
|
||
|
|
||
| def build_merge( |
There was a problem hiding this comment.
build_merge()
fans out to 14 callees (efferent coupling); 46 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
|
|
||
| def generate_call_table_rows(nodes: list, section_edges: list, lang: str) -> str: | ||
| """Generate call table row scaffolding for a section's nodes.""" | ||
| def generate_call_table_rows( |
There was a problem hiding this comment.
generate_call_table_rows()
fans out to 6 callees (efferent coupling); 8 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| t = _text(node) | ||
| return t.lstrip(":").lstrip("#:") | ||
|
|
||
| def _handle_defpackage(node) -> None: |
There was a problem hiding this comment.
_handle_defpackage()
fans out to 7 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| idx += 1 | ||
| return children[idx:] | ||
|
|
||
| def _handle_def_form(node, def_keyword: str) -> None: |
There was a problem hiding this comment.
_handle_def_form()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| return False | ||
| return sym.startswith("def") or sym.startswith("define-") | ||
|
|
||
| def _process_form(top) -> bool: |
There was a problem hiding this comment.
_process_form()
fans out to 10 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| return None | ||
|
|
||
|
|
||
| def _resolve_csharp_type_references( |
There was a problem hiding this comment.
_resolve_csharp_type_references()
fans out to 10 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| salt = hashlib.sha1(name.encode("utf-8"), usedforsecurity=False).hexdigest()[:6] | ||
| return _make_id(plain_nid, salt) | ||
|
|
||
| def walk(node) -> None: |
There was a problem hiding this comment.
walk()
fans out to 8 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
3617f15 to
f5f66b6
Compare
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 4 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Refactors dispatch_command's extract argument parsing from a long hand-rolled if/elif chain into a data-driven option_specs table with a shared loop handling store/flag/append actions and = syntax. Adds strict validation—rejecting unknown options, unexpected positionals, empty values, and value-less flags—plus a _parse_percentile converter for --exclude-hubs, and collects results into a parsed dict instead of individual locals. Expands test_extract_cli_args to cover the new append semantics, empty-value rejection, unknown-option handling, and syntax-before-path-existence ordering.
Worth a look
- Remote repository targets are rejected before clone handling —
graphify/cli.py:3096· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Dangling undefined assertion makes new test fail —
tests/test_extract_cli_args.py:417· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Postgres-only extract still resolves a missing path —
graphify/cli.py:3096· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- target computed from target_arg before checking whether target_arg is defined when only --postgres DSN given —
graphify/cli.py:3096· 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 — 252 functions depend on the 66 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 117 callees - new:
_stale_graph_sources()— 7 callers, 6 callees - new:
_run_hook_guard()— 4 callers, 7 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 252 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: 205 function(s) in the blast radius were not formally verified this run
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)
· 4 more finding(s) on lines outside this diff (see the check run).
2 participants
Summary
graphify extractarguments from one declarative option schema--exclude-hubsrange during parsing instead of returning a conversion traceback--no-dedup, repeated-option semantics, and intentional empty PostgreSQL DSNs while omitting unknown option values from error outputBefore and after
Before: invalid extract arguments could be ignored and a graph could still be written.
After: invalid syntax exits with code 2 before path validation or output work begins.
Result
Testing
python -m pytest tests/test_extract_cli_args.py tests/test_no_dedup_flag.py -q --tb=short(110 passed)python -m pytest tests/test_extract_cli.py tests/test_extract_code_only_cli.py -q --tb=short(41 passed)python -m pytest tests/ -q --tb=short(4985 passed, 11 skipped)python -m ruff check .python -m tools.skillgen --check(134 artifacts match)git diff --checkgraphify update .Closes #2025.
Related to #2004.