Repository navigation
feat(harness): allow per-run model override via VULNHUNT_HARNESS_MODEL - #34
liamthuynh wants to merge 3 commits into
Conversation
The scanning/judging model was a hardcoded constant, so trying a different model for one batch or benchmark run meant editing tracked source — easy to commit by accident, and awkward when two runs need different models. Read it from VULNHUNT_HARNESS_MODEL instead, keeping claude-opus-4-8 as the default. The name is scoped to the harness the way VULNHUNT_AGENT_CONFIG is scoped to the agent, so it stays clear of the agent's VULNHUNT_<SECTION>_<KEY> namespace. Reusing that component's VULNHUNT_ANTHROPIC_MODEL would not work anyway: it may hold a Bedrock inference-profile ID, which the `claude --model` flag the harness shells out to does not accept. Blank and whitespace-only values are treated as unset. os.environ.get() falls back only on an absent key, so an exported-but-empty variable would otherwise have produced `claude --model ""` in scan.py and broken the existing non-empty assertion in test_config.py. The tests restore module state through a fixture rather than a `finally` inside the test body. monkeypatch undoes itself in fixture teardown, which runs after the body, so an in-body restoring reload re-reads the override and leaves it baked into config.MODEL for every later test. The README now names the two traps a downgraded run walks into: /vulnhunt stops on non-Opus models at Step 0, and scan artifacts carry no model tag, so a cheap trial run is silently reused by a later full run on both resume paths. Validated: harness suite 169 passed, coverage steady at 95%; re-run with the variable unset, empty, whitespace-only, and set to a real model id.
|
|
schenksj
left a comment
schenksj
left a comment
There was a problem hiding this comment.
Thanks @liamthuynh, this is a great contribution! Being able to override the model for one run without editing config.py is really useful. The details are handled well: blank values fall back to the default, the value is stripped, the comment explains why this doesn't reuse the agent's VULNHUNT_<SECTION>_<KEY> namespace, and the test fixture restores the environment so the override can't carry into other tests.
I'm requesting changes for one issue. With an env var, switching models is now easy, which makes an existing labeling problem in the benchmark easy to hit.
The benchmark report can name the wrong model. benchmark/run.py:427 runs state["model"] = MODEL on every run, before any phase executes. phase_scan skips any target that is already marked scanned or has valid results, and tally.py:102 writes state["model"] into BENCHMARK_REPORT.md. So:
VULNHUNT_HARNESS_MODEL=<cheap-model> python -m local_harness.benchmark.run(trial run)python -m local_harness.benchmark.run(no env var, no--force-rescan)
Step 2 rescans nothing. It re-judges and tallies the trial's findings, and the report says they came from claude-opus-4-8. --tally-only mislabels results the same way.
That line is older than this PR. Before, though, changing the model meant editing a source file. Now it's one environment variable, so this PR is a good place to fix it. Either of these would work:
- only set
state["model"]when a scan actually runs, or - record the model on each target when it is scanned, and have the tally report the model(s) that actually produced the results (or flag a mix).
After that, please update the README caveat to match (inline comment below).
Otherwise this looks ready. The README commands and flags (--force-rescan, --re-clone) match the CLI, and the config change itself is correct. Thanks again!
| - Scan artifacts are not tagged with the model that produced them, and both | ||
| resume paths skip any target that already has results. A cheap trial run will |
There was a problem hiding this comment.
Calling out the resume behaviour here is helpful, but "not tagged with the model that produced them" isn't quite accurate. state.json and BENCHMARK_REPORT.md are tagged with a model: whichever one was set on the most recent run (benchmark/run.py:427), not the one that produced the results. That's worse than no tag, because the report confidently names the wrong model.
Once the labeling is fixed (see the review summary), could you update this bullet to describe the new behaviour? If you decide to leave it unfixed, please state plainly that the report shows the current run's model even when the results were reused.
There was a problem hiding this comment.
Fixed. The model label now updates only when scans run, so reused results keep their saved label. I updated the README bullet to reflect this.
| # `claude --model ""`. Read once at import, so it must be set before the process | ||
| # starts. | ||
| DEFAULT_MODEL = "claude-opus-4-8" | ||
| MODEL = os.environ.get("VULNHUNT_HARNESS_MODEL", "").strip() or DEFAULT_MODEL |
There was a problem hiding this comment.
The override logic looks good, and stripping the value plus falling back on blank is the right call.
One thing to fix with it: in the benchmark, MODEL is also used as a label, not just passed to claude --model. benchmark/run.py:427 copies it into state["model"] even when every target is skipped, so after this change a run without the env var will relabel results from an earlier override run. Details are in the review summary.
There was a problem hiding this comment.
Fixed. state["model"] now updates only when scans run. Resumed runs and --tally-only keep the saved label, with regression tests covering both.
3 participants
While setting up a batch scan I hit the fact that the harness hardcodes the
scanning/judging model in
local_harness/config.py, so trying a different modelfor one batch or benchmark run means editing tracked source, easy to commit by
accident, and awkward when two runs need different models.
This reads it from
VULNHUNT_HARNESS_MODELinstead, keepingclaude-opus-4-8as the default.
Naming. Scoped to the harness the way
VULNHUNT_AGENT_CONFIGis scoped tothe agent, so it stays clear of the agent's
VULNHUNT_<SECTION>_<KEY>namespace.Reusing that component's
VULNHUNT_ANTHROPIC_MODELwould not work anyway: perthe agent README it may hold a Bedrock inference-profile ID
(
us.anthropic.claude-...), which theclaude --modelflag the harness shellsout to does not accept.
Blank values. Treated as unset.
os.environ.get()falls back only on anabsent key, so an exported-but-empty variable would otherwise have produced
claude --model ""inscan.pyand broken the existing non-empty assertion intest_config.py.Test state. The new tests restore module state through a fixture rather than
a
finallyinside the test body.monkeypatchundoes itself in fixtureteardown, which runs after the body, so an in-body restoring reload re-reads
the override and leaves it baked into
config.MODELfor every later test.Docs. The README now names the two traps a downgraded run walks into:
/vulnhuntstops on non-Opus models at Step 0, and scan artifacts carry no modeltag, so a cheap trial run is silently reused by a later full run on both resume
paths (
--force-rescanor a fresh clone are needed to actually re-scan).Validation
pytest --cov=local_harness→ 169 passed (was 165), coverage steady at 95%.VULNHUNT_HARNESS_MODELunset, empty, whitespace-only, andset to a real model id.