Cap additional-repo clones in verify mode

[schenksj]

Cap additional-repo clones in verify mode

Adds an upper bound on the cross-repo clone work a verify run can be driven to do, configurable through [verify] max_additional_repos. This came out of the cross-model /vulnhunt evaluation (eval_rep/). It was developed test-first with the existing suite kept green, and updated for review feedback.

The branch has been brought up to date with main by a merge (08bebf6). The earlier config.py paren prerequisite is now a no-op because main already has the same fix (#13), and its redundant TestModuleImports test class has been removed (7169182).

CANON-37: Bound additional-repo clones (CWE-400)

In vulnhunter-agent/agent/verify.py, _process_clone_request used to clone one repo per resolvable cross-repo reference with no overall cap; the only guards were per-hint dedup and host/alias gating. The reference list comes from issue and comment text, so a run with many distinct references could clone an unbounded number of repos.

• Cap on clone attempts, not retained clones (1653ab1, d8c62a7). config.verify.max_additional_repos (default 10) caps how many clones are attempted. The budget is charged when a reference clears the cheap pre-checks, so references that resolve but don't exist (and fail to clone) still count against it. Without that, they could drive unbounded failing 300s clones.
• No hidden pre-truncation (c8278f6). Thanks to @AmirF194 for catching this. An interim MAX_EXTRACTED_SOURCES = 10 truncation in verify_refs._coerce_sources has been removed. It ran before hint resolution and dedup and ignored the config, so any configured value above 10 had no effect, and junk or unresolvable references could crowd out real ones. The attempt cap in _process_clone_request is now the single control.
• Duplicate mentions don't burn the budget (ccadd73). References are deduped on the resolved repo's identity (verify_resolve.repo_identity: host/owner/repo, normalizing alias vs URL, .git, trailing slash, case, and https/ssh/scp forms) before the budget is charged. Repeat mentions of a repo whose clone failed are still recorded for R6.
• Over-cap references are recorded, not silently dropped (98a9163). Resolvable references past the cap go into state.ignored_hints, so the R6 annotation tells the skill the verifier had no local access to them. The single cap warning now counts only the distinct repos actually skipped.
• Config (32040b5, baeb4fe). load_config rejects negative values with a ValueError; 0 disables cross-repo cloning. The knob is documented in agent/config.example.toml. The default now lives only on VerifyConfig, and the duplicate verify.MAX_ADDITIONAL_REPOS constant has been removed.

Tests (tests/test_verify.py, tests/test_config.py, tests/test_verify_resolve.py):

• test_process_clone_request_caps_additional_repos and ..._caps_failed_clone_attempts: the attempt cap holds for both successful and failing clones.
• test_preflight_configured_cap_above_ten_reaches_clone_step: with the cap set to 20, all 15 resolvable references reach the clone step. This goes through the real extractor coercion path with only the LLM faked.
• test_preflight_honours_configured_cap_below_default and test_preflight_cap_zero_disables_cross_repo_cloning: the configured value is what reaches _process_clone_request.
• test_process_clone_request_duplicate_mentions_do_not_burn_budget: 10 mentions of org/a in different spellings plus 1 of org/b, with a cap of 2, clone both in 2 attempts. ..._duplicate_of_failed_clone_is_ignored covers repeats of a failed clone.
• test_process_clone_request_records_over_cap_hints_as_ignored: over-cap references end up in ignored_hints, and the warning reports the accurate skip count.
• test_verify_max_additional_repos_*: default, TOML parse, 0 allowed, negative rejected.
• test_repo_identity_* and test_clone_cap_default_has_single_source_of_truth.

Verification

• vulnhunter-agent (pytest --cov, as in CI): 1159 passed, 1 skipped, 89% coverage. That is 20 new tests over main's 1139.
• Each review fix was written test-first. The new tests failed (RED) before the fix and passed (GREEN) after. The exceptions are the config parse/threading tests, which document wiring that already existed; the negative-value validation test was RED first.

🤖 Generated with Claude Code

[schenksj]

Adversarial review + fixes applied (pushed d8c62a7, ceabd22).

Review found the clone cap gated on len(state.additional_repos) — i.e. retained successful clones — while resolve_repo_hint accepts any allow-listed-host URL with no existence check. So an attacker packing many github.com/org/nonexistent-{i} refs triggers a full shallow_clone attempt each (300s timeout), each fails → routed to ignored_hints without incrementing the count, so the cap never trips and the clone-attempt work (the substance of CANON-37) stayed unbounded.

Fixes:

• Cap now bounds clone attempts, not retained repos — an attempts counter increments once an entry clears the cheap pre-checks and is about to be cloned; break at the cap. Cheap early-skips don't consume budget.
• Config-driven — added VerifyConfig.max_additional_repos (default 10), replacing the hardcoded constant.
• Upstream bound — _coerce_sources now truncates the extracted source list (MAX_EXTRACTED_SOURCES=10).
• New discriminating test — test_process_clone_request_caps_failed_clone_attempts: 53 nonexistent sources all raising ResolveError; asserts attempts ≤ cap. RED against the prior code (53 attempts), GREEN after.

Full vulnhunter-agent suite: 1112 passed, 1 skipped.

[AmirF194]

I cloned the PR head (ceabd22d) and read the full clone-request path end to end, config.py through verify.py through verify_refs.py.

vulnhunter-agent/agent/verify_refs.py:56 adds MAX_EXTRACTED_SOURCES = 10 as a bare module constant, and _coerce_sources (line 171, called from extract_cross_repo_references at line 168) truncates the extractor's output to it before _process_clone_request ever runs. extract_cross_repo_references does receive config as a parameter (line 119), but never passes it into _coerce_sources, whose signature only takes parsed. So this truncation never reads config.verify.max_additional_repos.

The result: config.verify.max_additional_repos (the new configurable knob this PR adds in config.py, correctly threaded into _process_clone_request's attempts >= max_additional_repos check at verify.py:1036) only ever narrows the effective cap below 10, never widens it above 10. An operator who sets max_additional_repos: 20 in config still gets at most 10 references out of the pre-flight extractor, because verify_refs.py's hardcoded cap runs first and unconditionally. _process_clone_request's own cap is correctly config-driven and well tested (test_process_clone_request_caps_additional_repos, test_process_clone_request_caps_failed_clone_attempts), but nothing in this PR's diff or in tests/test_verify_refs.py exercises MAX_EXTRACTED_SOURCES against a non-default config value, so this direction of the knob being inert has no test surfacing it.

Simplest fix: drop the pre-truncation in verify_refs.py entirely. _process_clone_request already bounds clone attempts correctly per CANON-37 regardless of how many references the extractor returns, so this second, unconfigurable cap doesn't add protection, it just silently overrides the configured value whenever it's set above 10.

[schenksj]

Thanks @AmirF194 — great catch, and you were right. MAX_EXTRACTED_SOURCES = 10 truncated the extractor output before hint resolution and dedup, so max_additional_repos > 10 was inert, and junk/unresolvable references could also crowd out real ones.

Fixed as you suggested in c8278f6: the pre-truncation is gone, and the config-driven attempt cap in _process_clone_request is now the single control. New test test_preflight_configured_cap_above_ten_reaches_clone_step runs the real extractor with cap 20 / 15 refs and confirms all 15 reach the clone step.

While in there, a follow-up review turned up related gaps, now also fixed:

• ccadd73 — repeat mentions of the same repo (URL / alias / org/repo spellings) are deduped by repo identity before charging the clone budget, so they can't starve distinct repos.
• 98a9163 — references skipped by the cap are recorded in ignored_hints, so the R6 annotation flags that the verifier lacked access to them.
• 32040b5 / baeb4fe — [verify] max_additional_repos is validated (>= 0, 0 disables), documented in config.example.toml, and has a single source of truth for its default.

vulnhunter-agent: 1159 passed, 1 skipped (merged with current main).

[CLAassistant]

All committers have signed the CLA.

[emeth-]

This is a solid improvement on main, which has no cap on clone work. Counting per attempt is the right design, and dropping the pre-truncation resolves @AmirF194's point. Dedup also fixes a main quirk: a failing variant of a repo that has already cloned no longer ends up in R6. The full suite passes locally (1163 passed, 1 skipped).

One fix I'd like before merge: a failed clone of one spelling blocks every other spelling of that repo.

I know this is intentional (test_process_clone_request_duplicate_of_failed_clone_is_ignored). The trouble is that clone failures depend on the URL, not the repo. Transport, credentials and port all vary by spelling, while repo_identity merges them.

Example: suppose git@github.com:acme/shared-lib.git (on a runner with no GitHub SSH key) or https://github.com:9999/acme/shared-lib comes before https://github.com/acme/shared-lib or the shared-lib alias in the extractor's output. Then the later spellings are never tried. The repo never reaches ADDITIONAL_REPOS, so phase 2 can't follow the fix into it, and claims citing those spellings are rejected under R6. On main, they clone. A teammate pasting the SSH URL is enough to trigger this, and a single comment with a bogus port triggers it on purpose.

Suggested fix: only skip a duplicate if an earlier spelling succeeded or was skipped by the cap. If it failed, let the next spelling try, charging the budget as usual. I tried this locally with three states (ok / failed / capped), and the only test that fails is the one above. The trade-off is that repeated failing spellings each use up budget, and a bogus port can take a full clone timeout. That's still bounded by the cap.

Optional, in the same place: once a spelling clones, drop that repo's earlier failed spellings from ignored_hints. R6 matches case-insensitive substrings, so a leftover failed https://github.com/Acme/shared-lib would still reject claims citing https://github.com/acme/shared-lib. That also happens on main.

Follow-ups (fine as a separate PR):

1. Crowd-out. The budget is spent in the extractor's output order, so enough distinct resolvable references ahead of a real one push it past the cap, and that repo is never cloned. A commenter on main can already get a claim rejected under R6 with one unresolvable hint; what's new here is losing the repo itself. Ideas: process alias-resolved hints first, or exempt them from the budget, in which case they also need to bypass the capped-duplicate skip. Separately, the R6 and Limitations wording ("could not resolve … to a clonable URL", "no clonable URL provided") is wrong for cap-skipped hints. It appears in verify_extract.render_comments_file, R6 in comment_rules.md, step 0.6 of phase0_preflight.md, and phase2_verify.md.
2. repo_identity lowercases the path and drops the port. So two distinct repos that both clone successfully can merge into one identity, for example on a case-sensitive host or a second port. The second is then dropped with no R6 entry. I'd suggest keeping the port (normalizing default ports) and lowercasing the path only for hosts known to be case-insensitive, such as the configured GitHub host.
3. Remaining bound. The worst case is still max_additional_repos × clone_timeout_seconds (about 50 minutes at the defaults). An overall deadline would close that.