Compile-check generated harnesses before accepting them - #129
Conversation
The harness generation prompt promises the agent that its harnesses are type-checked on delivery and that compiler errors come back for repair, but the check behind that promise never ran: it was gated on `if False`, because invoking bare `solc` on the harness files cannot resolve a project's remappings, include paths or compiler settings. A harness that does not compile is therefore only discovered by AutoSetup, one phase later, where it is misread as a compilation problem to work around rather than a source error to repair -- and the run dies with everything downstream of it discarded. Check them with the project's own build instead: write the candidates to a scratch directory at the same depth as `certora/harnesses` (so their relative imports resolve identically) and run `forge build --json` on them, which reports diagnostics with a severity field and reuses the artifacts the build phase already produced. Error-severity diagnostics are handed back to the agent against the paths it knows; the scratch directory does not outlive the check. Projects with no foundry build, or a forge that never gets as far as a report, are accepted unchecked as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| _logger.info("Harness compile check skipped: needs forge and a foundry project") | ||
| return None | ||
| check_dir = root / _HARNESS_CHECK_DIR | ||
| try: |
There was a problem hiding this comment.
shouldn't you wrap this call with materialize?
There was a problem hiding this comment.
Claude answers: yes — done in e51d98f, and thank you, it was covering a real gap rather than just being tidier.
The gap: I was writing only the delivered harnesses. The VFS can also hold files the agent wrote but did not deliver — a shared base contract the harnesses import, for instance — and those were missing from the build, so a correct harness could be rejected for an import that does not resolve. Materializing gets the agent's actual filesystem view, which is what we want to compile.
It also lets a fair amount of machinery go: the harnesses keep their certora/harnesses paths, so the scratch directory, its same-depth requirement (so ../../src/... imports resolved) and its cleanup all disappear, and the diagnostics no longer need rewriting before they go back to the agent.
Measured on the project this came from (353MB, 12,286 files):
| materialize | ~6.5s |
| forge build after it | 1.4s (rejecting) / 3.2s (accepting) |
Notably forge reuses the copied cache/ and out/ — its cache keys are project-relative, so they survive the copy and no dependency graph is recompiled. Both cases re-verified end to end through a materialized copy: the original harness set is rejected with the full should be marked as abstract diagnostic, the repaired set is accepted.
One risk worth naming: the copy lands in the container's temp space, and it scales with the project. This one is 353MB; a project whose out/ carries build_info can be several GB, and the copy happens once per repair attempt. If that turns out to bite, the narrower fix is to materialize only what the VFS holds plus what the harnesses transitively import, rather than the whole tree.
Building the delivered files in a scratch directory inside the live project checks something subtly different from what the agent produced: the VFS may hold files it wrote but did not deliver -- a shared base contract the harnesses import, say -- and those would be missing from the build, failing harnesses that are in fact correct. It also puts a directory into the project tree for the duration of the check. Materialize the VFS instead and build there. The harnesses keep their `certora/harnesses` paths, so the diagnostics need no rewriting and the scratch directory, its depth-matching requirement and its cleanup all go away. Cost on a 353MB / 12k-file project: ~6.5s to materialize, and forge reuses the copied build cache rather than recompiling the dependency graph. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| try: | ||
| report = json.loads(forge_json) | ||
| except json.JSONDecodeError: | ||
| return None | ||
| if not isinstance(report, dict): | ||
| return None | ||
| diagnostics = report.get("errors", []) | ||
| return [ | ||
| d.get("formattedMessage") or d.get("message", "") | ||
| for d in diagnostics if d.get("severity") == "error" | ||
| ] | ||
|
|
There was a problem hiding this comment.
pydantic schema is cheap and easier to read imo
There was a problem hiding this comment.
Claude answers: agreed, done in 5ab7839 — the parsing is now a two-model schema and the digging is gone:
class ForgeDiagnostic(BaseModel):
severity: str
message: str = ""
formatted_message: str = Field(default="", alias="formattedMessage")
@property
def rendered(self) -> str:
return self.formatted_message or self.message
class ForgeReport(BaseModel):
errors: list[ForgeDiagnostic] = Field(default_factory=list)
@property
def compile_errors(self) -> list[str]:
return [d.rendered for d in self.errors if d.severity == "error"]Call site is ForgeReport.model_validate_json(stdout) plus one except ValidationError. That also collapses three separate "not a report" returns (JSON decode error, non-dict, missing key) into the one place, and the two facts worth knowing about this format — that errors carries warnings too, and that the exit code says nothing because forge exits 0 either way — now sit on the models rather than in a comment above the .get chain.
| proc = subprocess.run( | ||
| [forge, "build", "--json", *sorted(harness_paths)], | ||
| cwd=root, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=_HARNESS_CHECK_TIMEOUT_S, | ||
| ) |
There was a problem hiding this comment.
shouldn't this be async? you'll need to use an async result tool (there's one floating around you can use...) but that should be fine...
There was a problem hiding this comment.
Claude answers: it should, and it now is — 5ab7839. You were right that it mattered: both halves of this check block, the project copy as much as the compile, and it sat on the event loop for the whole of each attempt.
Found the one you meant — AsyncResultTool in composer/spec/natspec/async_result.py, as used by interface_gen/stub_gen. The validator is a coroutine now:
class HarnessResultTool(AsyncResultTool[HarnessAgentResult], WithInjectedState[GenerationState]):
@override
async def validate_result(self, res: HarnessAgentResult) -> str | None:
...
async with _materialized(mat, self.state) as project_dir:
compile_errors = await _compile_check(project_dir, harness_paths)WithInjectedState on top of it because the validator needs the VFS — for the existence checks and for the materialization. That combination had no precedent in the tree, so I checked what the model actually sees rather than assuming: the tool's args are ['value'], state and tool_call_id are injected and hidden, a rejection returns the message without setting result, and an acceptance stores the validated model. There is a test pinning that now, since a silent change there would break the phase.
The copy goes through asyncio.to_thread around the accessor's context manager — the same shape materializing_project uses in composer/spec/source/prover.py for the prover's project copies — and forge runs via create_subprocess_exec with wait_for for the timeout.
Re-verified end to end against the source of the run this came from, through a materialized copy: the original harness set is rejected with the full diagnostic (materialize 4.3s, forge 1.8s), the repaired set is accepted (3.9s, 15.6s — a cold compile of all 16). Suite 511 passed, pyright clean.
Two review points from @jtoman: The check ran blocking work — the project copy and the forge build — inside the agent's tool call, stalling the event loop for the duration. Move the result tool to `AsyncResultTool` so the validator is a coroutine, materialize through `asyncio.to_thread` (the pattern `materializing_project` already uses for the prover's project copies), and run forge through `asyncio.create_subprocess_exec`. The forge report was read with nested `dict.get` calls. Give it a pydantic schema instead: `ForgeReport.compile_errors` is now the whole of the parsing, and output that isn't a report raises `ValidationError` where the digging used to return None from three separate places. The tool's own schema is unchanged from the model's side — `value` is all it sees, with the state injected — which a test now pins, along with acceptance storing the result and rejection returning the message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jtoman
left a comment
There was a problem hiding this comment.
one nit about finding a new home for a class you're using, and a suggestion for a future API improvement, neither of which block the landing of this PR. ship it
| stack = ExitStack() | ||
| project_dir = await asyncio.to_thread(stack.enter_context, accessor.materialize(state)) |
There was a problem hiding this comment.
this is common enough now, and the materialize is slow enough, we should really add an accessor.amaterialize. This VFS API was designed before we went full async...
There was a problem hiding this comment.
I'll make an issue..
| from langgraph.graph import MessagesState | ||
|
|
||
| from composer.prover.core import ProverOptions | ||
| from composer.spec.natspec.async_result import AsyncResultTool |
There was a problem hiding this comment.
worth moving out of this very specific package imo.
…ting them) into eric/rust Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem
harness_generation_prompt.j2tells the agent:That check never ran. It sat behind
if False: # this doesn't work(with a secondand Falseinside it) ingenerate_harnesses.result_validator, because the check it guarded was a baresolc <files>invocation, which cannot resolve a project's remappings, include paths, evm-version or via-ir settings.So an uncompilable harness is delivered unchallenged, and is only discovered one phase later by AutoSetup, which reads it as a compilation problem to work around rather than a source error to repair. A dev run hit this: the harness for an abstract contract was missing implementations that live in a sibling mixin, AutoSetup spent four
certoraRunpasses on irrelevant workarounds plus an import patch, and the run then died — after the pipeline had gone on to spend roughly two thirds of its output tokens on analysis phases whose results were discarded with it.Change
Check the candidates with the project's own build instead:
forge build --jsonthere on the delivered harnesses, which reuses the copied build cache instead of recompiling the dependency graphAsyncResultTool, the copy goes throughasyncio.to_thread(asmaterializing_projectdoes for the prover), and forge runs viacreate_subprocess_execcertora/harnessespaths, so nothing needs rewritingforge build --jsonexits 0 whether or not the sources compiled, so the report'sseverityfield is the signal, not the exit code. A project with no foundry build, noforge, or a forge that never got as far as a report is accepted unchecked, as today.Also drops
HarnessAgentResult.solidity_compilerand the prompt paragraph requesting it: the removedsolcinvocation was its only consumer, and the version now comes from the project's own build config. No tape or fixture references the field.Validation
uv run --no-sync python -m pytest tests/ -m "not expensive" -q→ 511 passed;pyright→ 0 errorsshould be marked as abstractdiagnostic (including everyMissing implementationnote), the repaired set is acceptedCompanions
Two independent PRs address the same failure from the other two directions: prompt guidance so the agent does not extend a non-deployable contract in the first place, and a terminal AutoSetup detection so the error is never mistaken for a workaround-able one.
Companion PRs: #130 #131. They are independent — any order, any subset.