-
Notifications
You must be signed in to change notification settings - Fork 747
[Bug] - Incorrectly handled absolute paths #1064
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?
Conversation
🦋 Changeset detectedLatest commit: 451105a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 044fc37498
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # For paths outside of the context directory, return the full resolved path in POSIX format | ||
| if src.startswith(".."): | ||
| return to_posix_path(os.path.normpath(os.path.join(file_context_path, src))) | ||
| return src |
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.
Python SDK path mismatch for relative context with .. paths
Medium Severity
The Python SDK's rewrite_src function uses os.path.normpath(os.path.join()) for .. paths, which preserves relative paths if file_context_path is relative. The JS SDK uses path.resolve() which always returns an absolute path. This causes a mismatch when file_context_path is relative: the Dockerfile COPY instruction would reference a relative path like config.txt while the tar archive stores the file at a fully resolved path like home/user/config.txt. The build would fail because the paths don't match. Using os.path.abspath instead of os.path.normpath(os.path.join()) would align the behavior with the JS SDK.
| if len(posix_path) >= 2 and posix_path[1] == ":": | ||
| posix_path = posix_path[2:] | ||
| # Strip leading slash | ||
| return posix_path.lstrip("/") |
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.
Python lstrip strips all slashes vs JS single slash
Low Severity
The Python SDK's to_posix_path function uses lstrip("/") which removes all leading slashes, while the JS SDK uses replace(/^\//, '') which removes only one. For paths with multiple leading slashes (like UNC paths //server/share after backslash conversion), Python would return server/share while JS would return /server/share. This inconsistency could cause different tar archive structures between the SDKs when handling network paths or malformed paths with multiple leading slashes.
Add handling for absolute paths (/home/users/mish/file.txt) and up dirs (../../file.txt)
Note
Adds robust path handling for file COPY/upload across JS and Python SDKs, enabling absolute (/) and parent-directory (../) sources with correct POSIX normalization and tar packaging.
rewriteSrcandtoPosixPath; updatesgetAllFilesInPathto resolve absolute/..paths and normalize separatorsmodern-tarwith gzip stream; updatestarFileStream/tarFileStreamUploadand renames params tofilePathfilePath; uses it for hashing, uploads, and logs in both SDKs..sourcesrewriteSrc,tarFileStream, and build flows using absolute pathsmodern-tar, movetarto dev)Written by Cursor Bugbot for commit 451105a. This will update automatically on new commits. Configure here.