Recompute filtered review summaries#22
Merged
rs545837 merged 1 commit intoMay 22, 2026
Merged
Conversation
|
@Iron-Ham is attempting to deploy a commit to the rs545837's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
inspect review
Triage: 20 entities analyzed | 0 critical, 0 high, 9 medium, 11 low
Verdict: standard_review
Findings (4)
- [low] In
crates/inspect-cli/src/commands/file.rs, thenormalize_pathfunction incorrectly handlesParentDircomponents when the parts vector is empty. The conditionparts.last().is_some_and(|part| part != "..")will be false when parts is empty, causing ".." to be pushed. This means a path like "../file.txt" will normalize to "../file.txt" instead of being properly handled, which could lead to incorrect path matching. - [low] In
crates/inspect-core/src/analyze.rs, therefresh_result_summariesfunction filtersdependency_edgesto only include edges where both endpoints are inremaining_ids, but then reassigns this filtered list back toresult.dependency_edges. However, the originalresult.dependency_edgesis borrowed immutably during iteration, and this creates a potential logic error where the dependency_edges field is being replaced with a filtered version. Ifretain_entity_reviewsis called multiple times, the second call will work with already-filtered edges rather than the original full set, leading to incorrect edge filtering. - [low] In
crates/inspect-cli/src/commands/file.rs, thenormalize_pathfunction has a logic error when handlingParentDircomponents. The conditionparts.last().is_some_and(|part| part != "..")will incorrectly pop a part when the last part is ".." and should instead push another "..". This breaks path normalization for paths like "a/../b/../c" where multiple parent directory references exist. - [low] In
crates/inspect-cli/src/commands/file.rs, thenormalize_pathfunction doesn't handle the case wherepartsis empty when encountering aParentDir. Whenparts.is_empty()is true,parts.last().is_some_and(...)returns false, causing ".." to be pushed. However, the condition should explicitly check if parts is empty OR if the last part is "..", not use the negation of the first condition as a fallback for the second.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
8e3ac50 to
63fa935
Compare
There was a problem hiding this comment.
inspect review
Triage: 23 entities analyzed | 0 critical, 0 high, 9 medium, 14 low
Verdict: standard_review
Findings (1)
- [low] In
crates/inspect-cli/src/commands/file.rs, thenormalize_pathfunction has a logic error when handlingParentDircomponents. The conditionparts.last().is_some_and(|part| part != "..")will pop a part even when the last part is ".." ifis_some_andreturns false, because the else branch pushes ".." regardless. This means consecutive ".." components won't be preserved correctly.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
63fa935 to
55245f4
Compare
There was a problem hiding this comment.
inspect review
Triage: 23 entities analyzed | 0 critical, 0 high, 9 medium, 14 low
Verdict: standard_review
Findings (3)
- [low] In
file.rs::normalize_path, the logic for handlingParentDircomponents is incorrect. When checking if the last part is not "..", it pops from the stack, but this doesn't correctly handle cases where we're already at the root or have consumed all normal components. The conditionparts.last().is_some_and(|part| part != "..")will pop a normal component when encountering.., but ifpartsis empty, it should add..to the result. However, the else branch adds..when the last part IS..or when parts is empty, which is correct. But the pop happens when last part is NOT.., meaning we have a normal component to cancel out. This logic appears correct on closer inspection. - [low] In
file.rs::canonicalize_existing_path, there's a potential infinite loop or incorrect path construction. The function walks up the directory tree by settingcursor = parent, but ifparentis None (at root), the while loop conditionwhile let (Some(parent), Some(name))will exit. However, the logic buildssuffixby prepending components, which could result in an incorrect final path if the loop exits early without finding a canonicalizable parent. - [low] In
file.rs::repo_relative_path, when the path cannot be stripped from the repo prefix, it falls back tonormalize_path(path)which returns a String. However, this normalized path might be relative to the current directory, not the repo root, leading to incorrect file matching. For example, if input is "/other/path/file.rs" and repo is "/tmp/repo", it will return "other/path/file.rs" which won't match entity reviews that have paths relative to the repo.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
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.
Summary
Closes #13
Test plan