fix: handle OS-level absolute paths in module resolver#30
Merged
Conversation
The module resolver was incorrectly treating OS-level absolute paths (like /Users/...) as project-relative paths (like /module), causing path duplication when resolving entry points. This fix distinguishes between the two by checking if the absolute path has multiple path segments. The issue occurred because: 1. The resolver checked for absolute paths with fs.existsSync() 2. If the file didn't exist or the check failed, it fell through to the project-relative handler 3. The project-relative handler (resolveAbsolute) would strip the leading '/' and concatenate with baseUrl, causing duplication The fix checks if an absolute path has multiple segments (e.g., /Users/foo/bar vs /module) to determine if it's an OS-level absolute path or a project-relative path. Fixes the issue where 'somon run hello_world.som' would fail with a duplicated path error.
Slashmsu
force-pushed
the
claude/add-greeting-function-01CQtfagG9P8iXVRbbpKLKDp
branch
from
4a2d877 to
a5b5190
Compare
|
✅ Tests completed on Node.js 20.x: success |
1 similar comment
|
✅ Tests completed on Node.js 20.x: success |
Adds a test that verifies the fix for absolute path resolution in the module resolver. The test ensures that running 'somon run' with an absolute path (like /Users/.../hello_world.som) works correctly without path duplication. The test uses the user's example code: - Creates a greeting function that takes a name parameter - Returns a template string with 'Салом' (Hello in Tajik) - Calls the function and prints the result This test verifies that the module resolver correctly distinguishes between OS-level absolute paths (/Users/...) and project-relative paths (/module).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
✅ Tests completed on Node.js 20.x: success |
added 2 commits
November 17, 2025 15:03
The previous implementation incorrectly classified project-relative absolute imports (like /lib/utils/math.som) as OS-level paths, causing resolution failures. This commit adds a robust check to distinguish between: 1. OS-level absolute paths: /Users/..., C:\Users\..., /home/..., /tmp/... 2. Project-relative paths: /lib/utils, /module The new isOsLevelAbsolutePath() method checks: - If the path starts with the project's baseUrl (OS path within project) - If the path matches common OS directory patterns (Unix: /home/, /Users/, /var/, /tmp/, /opt/, /usr/, /etc/; Windows: C:\, D:\, etc.) - Otherwise, treats it as project-relative Added comprehensive tests: - Project-relative absolute imports with multiple segments (/lib/utils/math) - OS-level absolute paths (full filesystem paths) - Ensures both resolve correctly without path duplication Addresses code review feedback from chatgpt-codex-connector.
Addresses SonarQube security warning about using publicly writable directories like /tmp/. The isOsLevelAbsolutePath() method only performs READ-ONLY path string classification and does not perform any file operations. Key security points documented: - Method only does path pattern matching, no file I/O - Actual file operations happen in ModuleLoader with proper validation - Path classification prevents path traversal attacks by ensuring project-relative paths don't escape the baseUrl - /tmp/ pattern is needed for test suite temp files (read-only classification) This is a false positive - no files are created or modified in publicly writable directories by this code.
|
✅ Tests completed on Node.js 20.x: success |
Add NOSONAR suppression comment to the unixOsPrefixes array to silence the security warning about publicly writable directories like /tmp/. This is a confirmed false positive because: - The array contains path PATTERNS for string matching only - No file operations (create, write, read) are performed on these paths - The patterns are used to classify whether an absolute path is OS-level or project-relative for module resolution - Actual file I/O happens separately in ModuleLoader with proper validation SonarQube's static analysis cannot distinguish between: 1. Unsafe: Creating/writing files in /tmp/ (security risk) 2. Safe: Pattern matching against '/tmp/' string (this code) The NOSONAR comment is justified and documents the reason for suppression.
|
|
✅ Tests completed on Node.js 20.x: success |
1 similar comment
|
✅ Tests completed on Node.js 20.x: success |
ggulpari
approved these changes
Nov 17, 2025
ggulpari
deleted the
claude/add-greeting-function-01CQtfagG9P8iXVRbbpKLKDp
branch
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
2 participants
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.
The module resolver was incorrectly treating OS-level absolute paths (like /Users/...) as project-relative paths (like /module), causing path duplication when resolving entry points. This fix distinguishes between the two by checking if the absolute path has multiple path segments.
The issue occurred because:
The fix checks if an absolute path has multiple segments (e.g., /Users/foo/bar vs /module) to determine if it's an OS-level absolute path or a project-relative path.
Fixes the issue where 'somon run hello_world.som' would fail with a duplicated path error.
🎉 Pull Request
Description
Type of Change
to not work as expected)
Related Issue
Fixes #(issue number)
Changes Made
Testing
npm test)npm run lint)npm run type-check)Checklist
Additional Context
📋 License Information
SomonScript is open source software licensed under the MIT License.
By submitting this pull request, you agree that your contributions will be
licensed under the MIT License.
For contribution guidelines, please review:
guidelines
Thank you for contributing to SomonScript! 🚀