Repository navigation
fix(review): normalise an absolute in-workspace symlink target before the '..' check (#1157) - #1207
Merged
bingran-you merged 1 commit intoOct 29, 2026
Conversation
5 tasks
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.
Fixes #1157.
Problem.
review.evidence._link_targetchecked an absolute symlink target after stripping the workspace prefix but before normalising it. A link the agent made as/root/analysis/scripts/../figuresresolves inside the workspace, yet_relative_pathrejected its..part and the whole capture failed withEvidenceError: Unsafe evidence path: 'analysis/scripts/../figures', so a verified rollout was never reviewed. Relative targets were already normalised first.Change
src/benchflow/review/evidence.py: normalise the absolute target afterrelative_to(workspace)(posixpath.normpath). A result that is..or starts with../raises "Evidence symlink escapes workspace", as an absolute target outside the workspace already does; anything else is checked and returned relative to the link's parent, as before.tests/test_review_evidence.py: a unit table for_link_target(in-workspace..kept,.kept, plain absolute unchanged, two escapes refused), an end-to-end capture of an absolute in-workspace..link throughcapture_workspace,prepare_workspace_uploadandinstall_uploaded_workspace, and a negative control for a link that climbs out after normalising.CHANGELOG.md: a Fixed entry.Validation
Unsafe evidence path); with the fix all pass.tests/test_review_evidence.py,test_review_evidence_runtime.py,test_acceptance_evidence.py: 137 passed. Full suite with an empty HOME: 6599 passed, 4 failed; the same 4 fail on main's code here (nouvon the test PATH, no playwright browser, two host-specific fixtures).ruff checkandruff format --checkclean.Not in this PR: release/0.8's
kept_link(PR #1130) needs the same normalisation, otherwise it drops such a link assymlink_escape.