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.
Use AST-based hashing for semantic file change detection #259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Use AST-based hashing for semantic file change detection #259
Changes from all commits
e94b21de1841856092a9fd2d3134ed23aa2File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DiscoveredFile.file_hashreads viaself.path.exists()/read_bytes(), butpathis often stored relative to the project root (seeset_relative_pathusage infrom_pathandabsolute_path). If the current working directory isn’t the project root, this branch can incorrectly treat an existing file as missing and fall back toget_blake_hash(b""). Prefer usingself.absolute_pathfor the existence/read checks here (and when passing the path intocompute_semantic_file_hash) to ensure consistent behavior regardless of CWD.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot
Please correct the following two issues:
Let's also catch a narrower error than BaseException, like the pyO3 error specifically. We need to be careful because when Codeweaver moves to a monorepo there will be situations where pyO3 isn't a transitive dependency.
Offhand, I know watchfiles and rignore use it, but those are engine dependencies. Some providers use it (DuckDuckGo via primp, for example). We may need to make the catch or caught exception dependent on whether we have pyO3 in the environment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed both issues in ed23aa2:
file_hashnow usesself.absolute_pathfor existence checks and reads, matching the pattern already used insize,contents, andraw_contents.Narrowed exception handling in
_compute_ast_hash:except Exceptioncatches all standard Python errors (includingImportErrorifast_grep_pyis missing)except BaseExceptiononly fires for non-Exceptionsubclasses; checksgetattr(type(exc), "__module__", None) == "pyo3_runtime"to identifyPanicExceptionby module name, re-raising everything else (GeneratorExit, etc.)This works in environments without pyo3 since the
ImportErrorfromast_grep_pyfalls into the firstexcept Exceptionhandler. The pyo3 check only activates when a Rust-side panic actually occurs — andPanicExceptioncan't be imported directly (it's a Rust-embedded type not registered insys.modules), hence the module name check.Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.