Skip to content

test(build-context): read the nested OMS fixture without newline translation - #518

Merged
rng1995 merged 6 commits into
NVIDIA:mainfrom
MohammedAlkindi:test/oms-signature-newline-comparison
Sep 16, 2026
Merged

rng1995 merged 6 commits into
NVIDIA:mainfrom
MohammedAlkindi:test/oms-signature-newline-comparison

Conversation

@MohammedAlkindi

Copy link
Copy Markdown
Contributor

test_build_context_scans_nested_oms_signature fails on Windows and passes on Linux.

The fixture is written byte-exact, so it keeps its CRLF endings and build_context caches it verbatim. The assertion compared that against Path.read_text(), which applies universal-newline translation and returns LF, so the two never match on Windows.

Reading the fixture back as bytes and decoding matches how it is written. Path.read_text(newline="") would also work, but that parameter is 3.13+ and pyproject.toml sets requires-python = ">=3.12,<3.15".

Measured on Windows 11, uv-managed CPython 3.14.0, -p no:randomly:

before  1 failed   AssertionError: '...}}\r\n' == '...}}\n'
after   1 passed

Verified in the full-file run as well, not only in isolation. Also verified on top of #505, which fixes the write side of the same file: this change is independent of it and both orderings pass.

The other failures in this file on Windows are unrelated. #501, #502 and #503 cover the symlink, FIFO and secure-open cases; the remaining two are timing-sensitive and flake on main as well.

…slation

The signature fixture is written byte-exact, so on Windows it keeps its CRLF
line endings and build_context caches it verbatim. The assertion compared that
against Path.read_text(), which applies universal-newline translation and hands
back LF, so the test failed on Windows while passing on Linux.

Read the fixture back as bytes and decode, matching how the fixture is written.
Path.read_text(newline=...) would also work but is 3.13+, and this project
supports 3.12.

Signed-off-by: Mohammed Alkindi <alkndymhmd692@gmail.com>
@MohammedAlkindi

Copy link
Copy Markdown
Contributor Author

Where this sits relative to the other open Windows test PRs, since it is the last of the set.

With #501, #502, #503, #505 and this one applied together, tests/nodes/test_build_context.py goes from 12 failures on main to 0 on Windows 11. Three runs of the stack: 72 passed, 8 skipped once, and 71 passed, 1 failed twice.

The one intermittent failure is test_dense_directory_discovery_and_cache_complete_with_modest_real_elapsed_time, which is load-sensitive and fails on main too, 1 of 3 runs there. Nothing in this PR touches it.

The 8 skips are the symlink and FIFO cases from #501 and #502, which cannot run without elevation or Developer Mode.

SanHsien added a commit to SanHsien/SkillSpector that referenced this pull request Sep 11, 2026
…VIDIA#524

Raise reviewed_pr_through to 527 and reviewed_issue_through to 524 in
tools/upstream_baseline.json (commit axis unchanged at 69dcdfb). Every
item gets a verdict in docs/DECISIONS.md: NVIDIA#493/NVIDIA#507/NVIDIA#508/NVIDIA#511 verified
via git merge-base --is-ancestor as already included through the
2.11.1/2.11.2 sync (including NVIDIA#521, which merged only into the still-
open NVIDIA#516 stack, not main); the remaining 27 items stay "wait for
upstream merge", none adopted now.

Two items get dedicated comparison notes per docs/DIVERGENCE.md's
static_runner.py and scripts/compare_scan_accuracy.py rows: NVIDIA#522 uses a
different env var name and different default/semantics than this
fork's SKILLSPECTOR_MAX_STATIC_SECONDS, so merging it cannot simply
delete the divergence row and needs a downstream env var migration
first; NVIDIA#490 extends this fork's own upstream PR NVIDIA#486 with a Python
3.14/POSIX edge case the fork's Windows environment does not hit, so
NVIDIA#486 is left untouched pending upstream's own resolution. NVIDIA#501-NVIDIA#505 and
NVIDIA#518 are also flagged as near-verbatim matches to this fork's existing
Windows test divergence rows, worth revisiting for row deletion once
merged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: SanHsien <34234698+SanHsien@users.noreply.github.com>

@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.

[SkillSpector Review]

Reviewed head 3cd47535cdec5116e3259a18f1aa1148da13805b — APPROVE.

The assertion now compares the exact decoded fixture bytes against the byte-faithful cache, avoiding Windows universal-newline normalization while preserving the production contract. This is a focused and portable test correction with no required changes.

Required checks pass, but GitHub currently reports mergeStateStatus=BEHIND; update against current main and re-run required checks before merging.

@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.

[SkillSpector Review]

Re-reviewed current head b6c2d9fc39087df572fe936fba07fe7b484f8e57. The current PR delta remains the byte-faithful fixture assertion; intervening changes in the file come from unrelated main symlink-portability work. read_bytes().decode("utf-8") correctly avoids Windows universal-newline translation while retaining Python 3.12 compatibility. I found no required change.

All required checks pass, but GitHub reports mergeStateStatus=BEHIND; update and revalidate before merging.

@rng1995
rng1995 enabled auto-merge (squash) September 16, 2026 19:20
@rng1995
rng1995 disabled auto-merge September 16, 2026 19:41
@rng1995
rng1995 merged commit d2ce556 into NVIDIA:main Sep 16, 2026
5 checks passed
SanHsien added a commit to SanHsien/SkillSpector that referenced this pull request Sep 17, 2026
Brings the fork up to upstream main c13f70e; the version is still 2.11.2.

The fork history was squashed into one commit on 2026-09-13, so it
shares no merge-base with upstream and git merge refuses. The range
diff was applied with git apply -3 instead. The fork content equals
69dcdfb plus the registered divergences, so conflicts landed only on
those seven files; the other 116 applied cleanly. FORK.md now documents
this procedure.

Divergences, resolved by each row's rule:
- static_runner.py takes upstream NVIDIA#522
  (SKILLSPECTOR_MAX_STATIC_ANALYSIS_SECONDS_PER_ARTIFACT, default 300s).
  The fork's SKILLSPECTOR_MAX_STATIC_SECONDS override and its seven
  tests are removed. Downstream gates must use the upstream name when
  their pin moves.
- test_static_yara.py, test_build_context.py and test_input_handler.py
  take upstream (NVIDIA#501-NVIDIA#505, NVIDIA#518 fix the same Windows issues); 301
  passed on Windows, rows deleted.
- test_security_end_to_end.py: upstream's version still fails
  nine_case on Windows (YARA load and SC8 budgets stay hard-coded), so
  the relaxation helper is re-applied on top; row kept and rewritten.
- .gitignore keeps the fork block; README.md stays Traditional Chinese
  and the upstream README goes to README.en.md.

Two new Windows divergences from new upstream tests:
- tests/unit/test_cli.py: a file name containing a backslash is split
  into two path parts on Windows; skipped by a capability probe added to
  tests/platform_support.py.
- test_json_container_ownership.py: oversized payloads became test ids,
  which pytest copies into PYTEST_CURRENT_TEST, over Windows' 32,767
  character environment limit; short ids added, content unchanged.

Triage: 13 of PRs NVIDIA#528-NVIDIA#580 merged into upstream main and arrive here;
29 stay open (including NVIDIA#550, release 2.12.0). Upstream closed this
fork's PR NVIDIA#486 on 2026-09-15; NVIDIA#490 builds on it and is open.

Verified on Windows in fresh-process batches against this tree:
tests/unit 1563 passed, 29 skipped; tests/nodes 3822 passed, 11
skipped, 4 xfailed (plus test_json_container_ownership 71 passed after
the id fix); remaining tests 182 passed, 16 skipped;
test_security_end_to_end.py 98 passed. ruff check and format clean,
check_divergence OK (10 diverging, 10 registered), check_pin_bounds OK.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: SanHsien <34234698+SanHsien@users.noreply.github.com>
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.

2 participants