Skip to content

Fix Windows path separators and console encoding#83

Merged
rng1995 merged 2 commits into
NVIDIA:mainfrom
dc995:fix/windows-path-and-console-encoding
Jun 24, 2026
Merged

Fix Windows path separators and console encoding#83
rng1995 merged 2 commits into
NVIDIA:mainfrom
dc995:fix/windows-path-and-console-encoding

Conversation

@dc995

@dc995 dc995 commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes three Windows-specific portability bugs discovered while running the test suite and CLI on Windows.

1. Path separators in �uild_context (product bug)

�uild_context emitted component and file-cache paths using the OS-native separator (e.g. scripts\helper.py on Windows) instead of forward slashes. This broke SARIF output and any consumer/test expecting POSIX-style paths. Fixed by normalizing relative paths with Path.as_posix().

2. Console encoding crash in the CLI (product bug)

The terminal report crashed on Windows consoles (cp1252) with UnicodeEncodeError when rendering box-drawing characters/icons. Fixed by reconfiguring stdout/stderr to UTF-8 (errors="replace") at CLI startup.

3. Test helpers (test-only)

Five test helpers built fixture file-caches with str(path), producing backslash keys on Windows. Normalized them with .as_posix() to match the product behavior.

Validation

  • Unit tests: 600 passed, 11 skipped (was 11 failing on Windows before this fix)
  • Lint (ruff check), format (ruff format --check), build (python -m build): all pass
  • Scans across dir/single-file/zip inputs and terminal/json/markdown/sarif outputs: all work; SARIF validates against 2.1.0; exit codes correct (1 risk>50, 0 safe, 2 error)
  • Integration tests (run against a real gpt-4o endpoint): the previously credential-blocked tests pass; a small number of semantic-judgment tests are nondeterministic across runs (model calibration), independent of this change

Notes

Changes are surgical: path-separator normalization and stdout encoding only. No behavior change to analysis logic.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

build_context emitted component/file-cache paths using the OS-native
separator (e.g. scripts\helper.py on Windows), which broke SARIF output
and tests that expect POSIX-style paths. Normalize relative paths with
Path.as_posix() so analysis output is identical across platforms. Apply
the same �s_posix() normalization to the five test helpers that build
file caches from fixture directories.

The CLI terminal report also crashed on Windows consoles (cp1252) with
UnicodeEncodeError when rendering box-drawing characters and icons.
Reconfigure stdout/stderr to UTF-8 (errors="replace") at startup so the
report renders without crashing on any platform.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@AbhiramDwivedi

Copy link
Copy Markdown
Contributor

@keshprad — heads up: this PR has a content overlap with #87.

Both PRs fix the same str(rel)rel.as_posix() issue across the same 6 files (build_context.py + 5 test helpers). This PR (#83) is a superset — it additionally fixes Windows UTF-8 console encoding in cli.py.

If this PR lands first, #87 becomes a full conflict on those 6 files and can be closed. Flagging so you don't have to resolve a merge conflict on review.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Clean, well-scoped cross-platform fixes:

  • Normalizing relative paths with Path.as_posix() in build_context._walk_skill_files (and the matching test helpers) is the right fix — it guarantees POSIX-style component/file-cache keys regardless of OS, which keeps SARIF output and downstream pattern matching consistent on Windows.
  • _ensure_utf8_streams() defensively reconfigures stdout/stderr to UTF-8 with errors="replace", guards for streams without a reconfigure method, and swallows only ValueError/OSError — a reasonable way to avoid UnicodeEncodeError on legacy Windows consoles.

Minor / optional (non-blocking):

  • _ensure_utf8_streams() runs unconditionally at import on every platform, switching the error handler to replace even where the console is already UTF-8. That's harmless for a report-rendering CLI, but if you want strict behaviour preserved on POSIX you could gate it on sys.platform == "win32" or on a detected non-UTF-8 encoding.

Nice work verifying across input/output formats and confirming the previously-failing Windows tests now pass.

@rng1995

rng1995 commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

@dc995 - Please resolve the conflicts, minor issues and merge the PR.

…console-encoding

# Conflicts:
#	src/skillspector/nodes/build_context.py
#	tests/nodes/analyzers/test_semantic_developer_intent.py
#	tests/nodes/analyzers/test_semantic_security_discovery.py
#	tests/nodes/test_semantic_quality_policy.py
#	tests/test_mcp_least_privilege.py
#	tests/test_mcp_tool_poisoning.py
@dc995

dc995 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

@rng1995 Done — this is ready to merge on your end:

  • Conflicts resolved. They were all the str(rel) ->
    el.as_posix() lines, which fix(build_context): use forward-slash component paths (cross-platform) #87 landed independently in the meantime. I kept main's (already-merged) version, so the path-normalization is now identical to what's on main and this PR's unique remaining contribution is the cli.py UTF-8 console fix.
  • Minor (non-blocking) note addressed. _ensure_utf8_streams() now skips streams that are already UTF-8, so strict encoding behaviour is preserved on POSIX and it only reconfigures where needed (e.g. legacy Windows consoles).
  • Validation: ruff check + ruff format --check clean; the 120 tests across the affected modules (build_context / mcp / semantic) pass.

PR shows MERGEABLE / CLEAN / approved. I don't have merge rights on the repo, so it's over to a maintainer — please squash-merge when you're happy. (No action needed on #87; it's already merged.)

@rng1995

rng1995 commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

@dc995 - You got another conflict to resolve. Please resolve it and I will merge the PR. Thanks!

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Automated SkillSpector Review]

Re-review after rebase — re-confirming approval.

Since my approval the path-normalization (Path.as_posix()) landed independently via #87, so this PR's unique remaining source change is the cli.py UTF-8 console fix — and it now addresses my prior non-blocking nit by skipping streams that are already UTF-8 (preserving strict behavior on POSIX, reconfiguring only legacy consoles). It still guards for a missing reconfigure and swallows only ValueError/OSError. The large -403 in the diff is just uv.lock regeneration. No concerns. Approving.

@rng1995
rng1995 merged commit ac6b41b into NVIDIA:main Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants