Express the source-tool read filter as a predicate - #120
Merged
Conversation
Solidity must never be withheld from the agent's source surface: any .sol can turn out to be part of the verification target, since the conf's `packages` remappings resolve into vendored dependency trees and a stock Foundry layout keeps real contracts in lib/ and test/. Stating that as a regex means a lookahead guarding an alternation, and stating "this directory at any depth" means (?:.*/)?d/.* — neither survives being read. Replace FS_FORBIDDEN_READ with fs_forbidden_read, a predicate over the path, and name the four rule classes it distinguishes: directories withheld whole, prover report directories, directories whose non-Solidity content is withheld, and machine-generated files. Withheld-whole is tested before the Solidity carve-out, which is what makes prover working directories the deliberate exception. The filter's type widens to graphcore's GlobalExcludeArg at every site it flows through, so --forbidden-read keeps taking a regex from the command line while the default is now the predicate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
check_submodule_pin.py requires the two to agree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jtoman
approved these changes
Aug 4, 2026
Comment on lines
+94
to
+98
| return any( | ||
| stem.endswith(sep + marker) | ||
| for sep in _BUNDLE_SEPARATORS | ||
| for marker in _BUNDLE_MARKERS | ||
| ) |
Contributor
There was a problem hiding this comment.
worth computing this cartesian product ahead of time imo.
Contributor
Author
There was a problem hiding this comment.
Claude answers: done in 6f7e236. Built the six pairings once at import and let str.endswith take the tuple, which also collapses the helper to a single expression:
_BUNDLE_STEM_SUFFIXES = tuple(
sep + marker for sep in (".", "-", "_") for marker in ("min", "bundle")
)
# ('.min', '.bundle', '-min', '-bundle', '_min', '_bundle')
def _is_generated_bundle(path: PurePath) -> bool:
return path.suffix == ".js" and path.stem.endswith(_BUNDLE_STEM_SUFFIXES)Tests unchanged and still green.
str.endswith takes a tuple, so the separator/marker pairings can be built once at import and matched in a single call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
shellygr
marked this pull request as ready for review
August 4, 2026 10:40
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ericeil
added a commit
that referenced
this pull request
Aug 4, 2026
master (#120) replaced the FS_FORBIDDEN_READ regex with the fs_forbidden_read predicate, which collided with this branch's SolidityIdentifier -> SourceIdentifier rename on the same two lines of the SourceFields construction in pipeline/cli.py. Took both: the neutral identifier and the predicate. The ecosystem seam merged clean but not correct — the SOLIDITY facet imported the constant master deleted. It now holds the predicate, and Language.default_forbidden_read widens from str to str | Callable[[PurePath], bool], the two shapes graphcore's GlobalExcludeArg accepts. RUST_FORBIDDEN_READ stays a pattern: nothing in a Cargo layout needs carving back out of an excluded directory, which is what forced Solidity to a predicate. graphcore moves to c8b3ae5 (master's pin), a descendant of this branch's 932cf73 that carries the forbidden_read predicate support the above depends on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses @jtoman's review on #119. Same behavioural goal, expressed the way he asked for
rather than as a regex. #119 is left open for now; this branch is the replacement for its
composer/spec/util.pychange.Stacked on Certora/graphcore#29 — the
graphcoresubmodule here is pinned to thatPR's head commit, which is not merged yet. Needs a re-pin to the merge commit before this
leaves draft.
Why
The rule the pipeline needs is "withhold these trees, but never withhold Solidity".
As a regex that is a lookahead guarding an alternation, plus
(?:.*/)?d/.*for everydirectory that can nest:
graphcore's own type-alias comment already said why this is the wrong tool —
"node_modules" in p.partsversus(?:.*/)?node_modules/.*.Change
FS_FORBIDDEN_READ(a regex string) becomesfs_forbidden_read, a predicate over thepath, and the four rule classes get names instead of being inlined into one alternation:
_WITHHELD_WHOLE_DIRS.git,.certora_internal, any depth_REPORT_DIR_PREFIXemv-*, root only_NON_SOLIDITY_DIRSnode_modules,lib,test,dist, any depth.json/.map/.dat,*.min.js,*.bundle.jsWithheld-whole is tested before the carve-out — that ordering is what makes prover
working directories the deliberate exception to "never withhold Solidity". Their
.solisa verbatim copy of a contract already readable at its canonical path (each
certoraRunmaterializes another
inputs/.certora_sources/**), and the analyzer reaches a specificreport's copy through a VFS scoped to that report.
The filter's type widens to
GlobalExcludeArgeverywhere it flows, so--forbidden-readstill takes a regex from the command line while the default is the predicate.
Testing
tests/test_fs_forbidden_read.pyis carried over from #119 unchanged except for itsharness — it now calls the predicate instead of
re.fullmatch. The assertions are thebehavioural spec, so 6/6 green is the evidence that dropping the regex changed nothing it
covered.
Measured against the source bundle of the dev run that motivated this:
.solgrep_filescall that overflowedDeliberate behaviour deltas vs
master, beyond the intended ones:.gitignorebecomes readable. The old(?:.*/)?\.git.*matched it by prefix; thepredicate matches
.gitas an exact path component. Real.gitcontent is stillwithheld here and by graphcore's floor.
x_bundle.jsbecomes withheld (new generated-file rule); a hand-writtenbundle.jsstaysreadable.
Also: 15 passed across
test_fs_forbidden_read.py+test_design_doc_finder.py(the otherFS_FORBIDDEN_READconsumer), andpyright composer/ analyzer sanity_analyzer certora_autosetupis clean — 0 errors, with one pre-existingFormTwarning incomposer/pipeline/core.pyunchanged.🤖 Generated with Claude Code