test(build-context): write fixture content with the newlines the assertions pin by kevin9327 · Pull Request #505 · NVIDIA/SkillSpector

kevin9327 · 2026-09-09T04:49:16Z

Problem

Five cases in tests/nodes/test_build_context.py assert cached content exactly, including the
trailing newline:

assert result["file_cache"].get("references/guide.md") == "# Reference guide\n"
assert result["file_cache"].get("scripts/run.py") == "print(1)\n"
assert result["local_file_cache"][".env"] == "TOKEN=not-reported\n"
assert result["raw_file_cache"][".env"] == b"TOKEN=not-reported\n"

but the fixtures are written with Path.write_text and no newline argument:

(root / "references" / "guide.md").write_text("# Reference guide\n", encoding="utf-8")

Text mode translates "\n" to os.linesep, so on Windows the fixture lands on disk as CRLF.
build_context reads it back faithfully and returns CRLF, and the case fails against content
the test itself wrote. The raw_file_cache assertion above makes the intent explicit — it pins
the bytes — so the fixture, not the product, is what is wrong here.

_write_real_oms_signature has the same problem in both directions:

target.write_text(_OMS_FIXTURE.read_text(encoding="utf-8"), encoding="utf-8")

read_text applies universal newlines and write_text applies the reverse translation, so the
pinned signature is newline-translated twice on the way to the temporary directory. Its case
then compares the cache against nested.read_text(...), which normalises again, and the two
never line up.

Fix

_write_real_oms_signature copies bytes. That is what a pinned signature fixture needs
regardless of platform, and it removes both translations.
The writes whose content is asserted exactly pass newline="\n", so the fixture matches what
the assertion pins. (Path.write_text has accepted newline since 3.10; this project
requires >=3.12.)

Writes whose content is not asserted verbatim are left alone, so the diff stays at the defect.
On a platform where os.linesep is already "\n" nothing changes, so CI behaviour is
identical.

Reproduction

Windows 11, Python 3.12.10, against unmodified main (704bc95):

$ python -m pytest -p no:randomly tests/nodes/test_build_context.py \
    -k "real_directory_with_skill_md or reads_directory_with_windows_secure_open or \
        stops_at_progressing_shared_deadline or scans_nested_oms_signature or \
        inventories_hidden_file_for_local_analysis" -q --no-header

E       AssertionError: assert '# Reference guide\r\n' == '# Reference guide\n'
E       AssertionError: assert 'print(1)\r\n' == 'print(1)\n'
E       AssertionError: assert {'first.txt': 'first\r\n'} == {'first.txt': 'first\n'}
E       assert '{"mediaType"...d":""}]}}\r\n' == '{"mediaType"...yid":""}]}}\n'
E       AssertionError: assert 'TOKEN=not-reported\r\n' == 'TOKEN=not-reported\n'

With this change, same command:

.....                                                                    [100%]
5 passed, 75 deselected in 5.09s

What must still hold

tests/nodes/test_build_context.py in full — five cases move from failing to passing and
nothing else changes state:

	failed	passed	skipped
before	12	66	2
after	7	71	2

The seven that remain are unrelated to newline handling and are not touched here.

ruff check tests/ — All checks passed!
ruff format --check tests/ — 126 files already formatted

Diff is 1 file changed, 9 insertions(+), 6 deletions(-).

…rtions pin Several cases assert cached content byte for byte, down to a trailing "\n", but build their fixtures with Path.write_text and no newline argument. Text mode translates "\n" to os.linesep, so on Windows the fixture is written with CRLF while the assertion pins LF, and the cases fail on content the test itself wrote. _write_real_oms_signature had the same problem in both directions: it read the pinned signature as text and wrote it back as text, so the copy was newline-translated twice. Copy its bytes instead, which is what a signature fixture needs anyway. Pass newline="\n" on the writes whose content is asserted exactly. Signed-off-by: kevin9327 <[email protected]>

MohammedAlkindi · 2026-09-10T07:48:45Z

Verified on Windows 11. This one fixes four failures outright, and the three test PRs together take the file from 12 failures to 2.

tests/nodes/test_build_context.py, same venv, -p no:randomly:

ref	failed	passed	skipped
`main` `69dcdfb`	12	66	2
+ this PR	8	70	2
+ #501 + #502 + this PR	2	70	8

The four this PR turns green, and they pass rather than skip:

test_build_context_real_directory_with_skill_md
test_build_context_reads_directory_with_windows_secure_open
test_build_context_inventories_hidden_file_for_local_analysis
test_file_cache_stops_at_progressing_shared_deadline_with_affected_suffix

For context on the other two, since they are hard to see from a Linux runner: #501 accounts for five symlink cases that fail here with OSError: [WinError 1314] A required privilege is not held by the client, and #502 for one FIFO case. Those become platform skips. Only this PR produces actual passes.

The two still failing on the stack are test_build_context_scans_nested_oms_signature and test_build_context_reports_read_error_without_fake_empty_content. Both fail identically on main, so they are outside all three PRs.

Environment: Windows 11, uv-managed CPython 3.14.0, uv pip install -e ., no elevation and Developer Mode off, which is why the symlink cases cannot run at all here.

…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 <[email protected]> Signed-off-by: SanHsien <[email protected]>

rng1995

[SkillSpector Review]

Reviewed head 6b04e9136792554bd02f4eaf95388f22dc9eebc6 — APPROVE.

The fixtures now write the exact bytes their assertions pin, and the OMS signature is copied byte-for-byte rather than normalized through text I/O. The changes are scoped to byte-exact test setup and are compatible with the supported Python versions. I found no required changes.

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

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 <[email protected]> Signed-off-by: SanHsien <[email protected]>

MohammedAlkindi mentioned this pull request Sep 10, 2026

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

Merged

rng1995 approved these changes Sep 12, 2026

View reviewed changes

rng1995 added 2 commits September 15, 2026 21:09

Merge branch 'main' into fix/build-context-lf-fixtures

58735cf

Merge branch 'main' into fix/build-context-lf-fixtures

323e3b8

rng1995 enabled auto-merge (squash) September 16, 2026 19:20

Merge branch 'main' into fix/build-context-lf-fixtures

e6a0a9f

rng1995 disabled auto-merge September 16, 2026 19:46

rng1995 merged commit 091f795 into NVIDIA:main Sep 16, 2026
5 checks passed

rng1995 mentioned this pull request Sep 16, 2026

release: SkillSpector 2.12.0 #550

Merged

BrewTestBot mentioned this pull request Sep 23, 2026

skillspector 2.12.0 Homebrew/homebrew-core#313189

Merged

1 task

Navigation Menu

Repository navigation

test(build-context): write fixture content with the newlines the assertions pin - #505

test(build-context): write fixture content with the newlines the assertions pin#505
rng1995 merged 4 commits into
NVIDIA:mainfrom
kevin9327:fix/build-context-lf-fixtures

kevin9327 commented Sep 9, 2026

Uh oh!

MohammedAlkindi commented Sep 10, 2026

Uh oh!

rng1995 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kevin9327 commented Sep 9, 2026

Problem

Fix

Reproduction

What must still hold

Uh oh!

MohammedAlkindi commented Sep 10, 2026

Uh oh!

rng1995 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants