fix(build_context): use forward-slash component paths (cross-platform)#87
Conversation
build_context emitted OS-native path separators for relative component paths, producing backslashes on Windows (e.g. `references\guide.md`). These paths are used as dict keys (components / file_cache / component_metadata) and as SARIF physicalLocation URIs, which are meant to be portable forward-slash paths — so on Windows the report locations were non-portable and downstream lookups (and the test suite) mismatched. Use Path.as_posix() so component paths are forward-slash on every OS, in build_context and the handful of test helpers that mirror the same relative_to logic. Behaviour-preserving on Linux/macOS (str() already yields forward slashes). Fixes NVIDIA#86 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Ram Dwivedi <abhiram.dwivedi@yahoo.com>
|
@keshprad — flagging a conflict with #83 before either PR lands. Both PRs make identical changes to the same 6 files:
#83 is a superset — it also adds Suggested resolution: if #83 is merged first, this PR (#87) can be closed as redundant. If you prefer to keep the scopes separate (path fix vs. encoding fix), I'm happy to rebase #87 after #83 lands and drop the now-duplicate hunks. |
rng1995
left a comment
There was a problem hiding this comment.
Correct cross-platform fix. Approving.
Using Path.as_posix() for relative component paths gives forward slashes on every OS, which is exactly what's needed since these values are used as dict keys (components / file_cache / component_metadata) and as SARIF physicalLocation URIs — backslashes on Windows broke both portability and path-keyed lookups. It's behavior-preserving on Linux/macOS. Nice touch fixing the mirrored relative_to logic in the test helpers too so the latent bug can't resurface, and the explanatory comments are helpful.
Minor / optional (non-blocking): PR #101 makes the same change to the same lines — only one can merge cleanly, so coordinate to close whichever is the duplicate. This one's the better version (it includes the comments).
Fixes #86
What
build_contextemitted OS-native path separators for relative component paths — backslashes on Windows (e.g.references\guide.md). These paths are used as dict keys (components/file_cache/component_metadata) and as SARIFphysicalLocationURIs, which are meant to be portable, forward-slash, URI-style paths. So on Windows the generated report locations were non-portable, downstream path-keyed lookups mismatched, and the unit/integration suite failed (11 tests acrosstest_build_context.pyand the semantic-analyzer fixtures).How
Use
Path.as_posix()instead ofstr(Path)so component paths are forward-slash on every OS:src/skillspector/nodes/build_context.py(the actual fix).relative_tologic (_build_file_cachein the semantic-analyzer tests, plus the two MCP fixture helpers — those have flat fixtures today, fixed for consistency so the latent bug can't resurface).Behaviour-preserving on Linux/macOS, where
str(Path)already yields forward slashes.Test
Full suite green on Windows (was 11 failing):
pytestpasses; ruff clean. No new dependencies; one-line idiom change.🤖 Generated with Claude Code