Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces validate_vendoring_roots to prevent vendoring symlinks from escaping private roots, refines the TOML require block replacement regex to stop at any TOML table, and fixes a regex backtracking issue in trace prefix rewriting. It also adds comprehensive unit tests and fixtures. The review feedback suggests improving robustness by removing strict=True from path.resolve() to gracefully handle dangling symlinks, and replacing Path.is_relative_to with a Python 3.8-compatible alternative to ensure backward compatibility.
| for name in directories + files: | ||
| path = Path(current) / name | ||
| if path.is_symlink(): | ||
| target = path.resolve(strict=True) |
There was a problem hiding this comment.
Using strict=True in path.resolve(strict=True) will raise a FileNotFoundError if a broken or dangling symlink is encountered. It is common for repositories or vendored packages to contain dangling symlinks (e.g., in test fixtures or unused files). If this happens, the script will crash with a traceback instead of handling it gracefully.\n\nUsing strict=False (which is the default for resolve()) allows the path to be resolved as much as possible. If the resolved path points outside the allowed roots, it will still be correctly caught and raise the descriptive ValueError. If it points to a nonexistent path inside the allowed roots, it will be safely allowed.
| target = path.resolve(strict=True) | |
| target = path.resolve() |
| path = Path(current) / name | ||
| if path.is_symlink(): | ||
| target = path.resolve(strict=True) | ||
| if not any(target.is_relative_to(parent) for parent in allowed): |
There was a problem hiding this comment.
Path.is_relative_to was introduced in Python 3.9. If this script is executed in an environment running Python 3.8 or older (which is still common in some CI/CD pipelines or older LTS distributions), it will raise an AttributeError at runtime.\n\nTo ensure compatibility with Python 3.8 and older, we can replace target.is_relative_to(parent) with an equivalent check using parent == target or parent in target.parents.
| if not any(target.is_relative_to(parent) for parent in allowed): | |
| if not any(parent == target or parent in target.parents for parent in allowed): |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3875 +/- ##
=======================================
Coverage 92.94% 92.94%
=======================================
Files 16 16
Lines 2466 2466
=======================================
Hits 2292 2292
Misses 174 174 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Vendoring must change dependency locations while retaining the declarations which consume them. Stop each TOML require block at the next table so that library and executable declarations survive. Add fixtures from the pinned aesop and importGraph packages to cover those boundaries. Reject symlinks outside the private archive roots before rewriting any file. Keep path components separate in trace matching so a long absolute path cannot trigger exponential regular-expression backtracking. These changes let native Lake traces remain usable after archive relocation. gherrit-pr-id: Gxizdpw52bljtxjfng3ervgk5xnca6mow Agent-authored-by: AI agent acting on joshlf's behalf
a1106d8 to
b593edb
Compare
Vendoring must change dependency locations while retaining the declarations
which consume them. Stop each TOML require block at the next table so that
library and executable declarations survive. Add fixtures from the pinned
aesop and importGraph packages to cover those boundaries.
Reject symlinks outside the private archive roots before rewriting any
file. Keep path components separate in trace matching so a long absolute
path cannot trigger exponential regular-expression backtracking. These
changes let native Lake traces remain usable after archive relocation.
Authored by an AI agent acting on joshlf's behalf.
Latest Update: v12 — Compare vs v11
📚 Full Patch History
Links show the diff between the row version and the column version.
⬇️ Download this PR
Branch
git fetch origin refs/heads/Gxizdpw52bljtxjfng3ervgk5xnca6mow && git checkout -b pr-Gxizdpw52bljtxjfng3ervgk5xnca6mow FETCH_HEADCheckout
git fetch origin refs/heads/Gxizdpw52bljtxjfng3ervgk5xnca6mow && git checkout FETCH_HEADCherry Pick
git fetch origin refs/heads/Gxizdpw52bljtxjfng3ervgk5xnca6mow && git cherry-pick FETCH_HEADPull
Stacked PRs enabled by GHerrit.