Check that declared source paths exist during component analysis - #202
Check that declared source paths exist during component analysis#202shellygr wants to merge 1 commit into
Conversation
The path fields on the source-carrying model types are written by the component-analysis agent and were never checked against the filesystem. A path that resolves to nothing travelled through name_to_path, the transitive closure and extra_files before AutoSetup's parse_contract_files raised on it, an hour and several phases after it was written. validate_solidity_connectivity now takes the project root and checks each declared path in its existing pass over components, so a submission wrong in both its graph and its paths comes back as one message. Cache hits are held to the same check, since the analysis cache key covers neither the source tree nor the rules in force. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| if fs_forbidden_read(PurePath(declared)): | ||
| return _PathFault.WITHHELD |
There was a problem hiding this comment.
I'm confused how this happened?
| found: dict[str, list[str]] = {name: [] for name in names} | ||
| for dirpath, dirnames, filenames in os.walk(root): | ||
| rel_dir = Path(dirpath).relative_to(root) | ||
| dirnames[:] = [d for d in dirnames if not fs_withheld_subtree(rel_dir / d)] |
There was a problem hiding this comment.
I was today years old when I learned you could control the recursion behavior of os.walk by just mutating dirnames. This language is insane dude.
| name = PurePosixPath(declared).name | ||
| candidates = same_named[name] | ||
| tail_matches = [c for c in candidates if c.endswith("/" + declared)] |
There was a problem hiding this comment.
Wait, I'm confused. This is basically finding candidates where the LLM clearly forgot a leading directory?
| return _PathFault.DIRECTORY | ||
| if not candidate.is_file(): | ||
| return _PathFault.MISSING | ||
| if fs_forbidden_read(PurePath(declared)): |
There was a problem hiding this comment.
FWIW, we should be using the forbidden_read on SourceFields which makes up the input to the pipeline. fs_forbidden_read is just the default (that we happen to use always, so its worth considering whether we need to keep forbidden_read as a separate part of the input)
| async def run_component_analysis[T: BaseApplication]( | ||
| ty: type[T], | ||
| child_ctxt: WorkflowContext[T], | ||
| input: SystemDoc | SourceCode | None, |
There was a problem hiding this comment.
FWIW, this is the input field which holds the source field to use.
| extra_input: list[str | dict], | ||
| expected_main_id: SourceIdentifier | None = None, | ||
| *, | ||
| project_root: Path | None, |
There was a problem hiding this comment.
doesn't this duplicate the data available on SourceCode? When will this be non-none and input !is SourecCode?
|
|
||
| ``project_root`` is the tree the validator resolves any source paths the model declares | ||
| against; it is required (not defaulted) so a new call site has to say what the model's paths | ||
| mean, and is ``None`` only when the run has no source tree. |
There was a problem hiding this comment.
which is also the semantics (if you will) of input == SystemDoc or input == None. I think we're duplicating data.
2 participants
What
The
pathfields onSourceExplicitContract,SourceExternalActorandExistingFromSourcearewritten by the component-analysis agent. Nothing checked them against the filesystem.
validate_solidity_connectivitycovered names, duplicates and the interaction graph, and it wastyped over
BaseApplicationwith no project root to check anything against.So a declared path that resolves to nothing travels a long way before anyone notices. It goes
through
run_setup_part1'sname_to_path, into the transitive closure, intoextra_filesinrun_autosetup_phase, and finally into the AutoSetup subprocess argv, whereparse_contract_filesraises
File ... does not exist. By then an hour has passed and several analysis and bug-analysisphases have run.
The shape that prompted this: a repository whose build project sits one or more directories below
the repository root. Paths are project-root-relative, so every declared path has to carry that
leading directory. A path assembled from an import statement or from a build tool's source
directory instead of from the agent's own file-tool output loses it, and every contract in the
submission loses it the same way.
How
The validator takes a third
project_root: Path | Noneand checks the declared paths inside itsexisting single pass over components. Path complaints join the same accumulated message as the
graph ones, so a submission that is wrong in both ways is corrected in one retry instead of two.
project_rootthreads throughEcosystem.validate_analysis, which follows the existinglocate_mainprecedent of handing ecosystem callables the run's facts. Solana and Soroban take anignored parameter; nothing in their models carries a source path.
Containment is lexical, meaning relative and never stepping up out of the root. Resolving instead
would follow symlinks, and the forbidden-read predicate deliberately keeps
.solreadable underlib/andnode_modules/, which are routinely symlinked. The agent is entitled to name a filethere and has no way to restate it as a non-symlinked path, so a resolving check would reject it
with no way to comply. There is a test pinning that.
The error says where to look instead. One walk of the tree collects readable files carrying the
declared name, narrowed to those whose path ends with the declared path, so the dropped-directory
case names exactly one real file rather than reporting "not found". One walk covers every bad path
in the submission, and wholly withheld directories are pruned during descent rather than filtered
out afterwards, because this runs while the agent waits on its retry.
Cache hits get the same check.
root_cache_keyissha256(project_root | doc_hash | relative_path | contract_name), which covers neither the sourcetree nor the rules in force, and the cloud namespaces the cache per user and repository. Without
this, a model written before the check existed is replayed unvalidated on every rerun and the gate
never fires at all. An entry that fails validation is stale, so it is re-derived and the existing
cache_putreplaces it.The prompt and the
pathfield descriptions now state the convention the validator enforces. Theyreject nothing on their own; they say what the check requires.
Scope note for reviewers
SourceExternalActor.pathis checked too, which is wider than the failure above. It is the sameclass of unchecked agent-written string and it reaches the summarizer prompt. The error offers
"omit the path instead" as the remedy, and an actor with no path is passed over silently
downstream, so a repository that passes today can start reporting a validation error. That is
intended, not a regression.
Testing
tests/test_solidity_component_paths.pyis new: 17 tests over acceptance, a path missing itsleading directory (asserting the hint names the one real file), several files sharing a name, no
file of that name anywhere, a directory, absolute and escaping paths, a path under a withheld
directory, a symlinked dependency being accepted, several bad paths arriving in one message, graph
errors and path errors arriving together, the external actor with and without a path,
project_root=None, greenfield, andFromSourceApplicationwhere the existing contract is checkedand the fresh one is not. Two more cover the cache branch in both directions.
tests/test_analysis_input_phrase.pygained rendering assertions for the two path bullets and theomit-the-path clause.
tests/test_pipeline_overlap.pynow asserts the driver forwards a realproject root.
Each new test was checked by breaking the code it covers and confirming it fails.
Targeted run: 91 passed.
pyright composer/ analyzer sanity_analyzer certora_autosetup: 0 errors.