fix: align Windows archive safety across coverage - #116
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed August 4, 2026, 7:04 PM ET / 23:04 UTC. ClawSweeper reviewWhat this changesThe PR rejects Windows archive alternate-data-stream names before extraction, isolates fallback permission tests, and merges Linux, macOS, and Windows coverage into one thresholded report. Merge readiness⛔ Blocked until stronger real behavior proof is added - 4 items remain This PR remains necessary and open: the prior Windows contract concern is addressed in the proposed patch, but the draft lacks exact-head Windows and merged-report proof. The local partial checkout could inspect current main but could not materialize the PR object, so patch details are assessed from the supplied exact-head diff. Priority: P3 Review scores
Verification
How this fits togetherArchive extraction validates untrusted entry paths before either JavaScript or native backends write into a confined destination. The coverage workflow runs platform-dependent tests, uploads their maps, and merges them into the repository’s configured coverage reports. flowchart LR
A[Archive entry names] --> B[Path validation]
B --> C[Windows ADS decision]
C --> D[JavaScript or native extraction]
E[Linux macOS Windows tests] --> F[Coverage artifacts]
F --> G[Merged coverage report]
G --> H[Configured thresholds]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the focused policy and coverage change only after exact-head Windows evidence shows both archive backends reject ADS names as entry-path and the three-artifact report passes configured thresholds. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main’s Windows portability test defines ADS entries as raw ENOENT, and the proposed validation changes that path before either extraction backend writes. Is this the best way to solve the issue? Yes, conditionally: early Windows-only ADS rejection is the narrow fail-closed fix and is documented, but its exact-head native and workflow behavior still needs Windows proof. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d4634144acc3. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
b9c1e13 to
e6af38a
Compare
|
Landed as Verification performed on exact head
The first complete Windows coverage attempt was valuable: it exposed native acceptance of NTFS alternate data stream archive names and a short one-code-unit rename buffer. Both were fixed rather than suppressing the coverage cases. The native-first Windows permission contract remains unchanged. |
Summary
vitest.config.tsoffWindows Permission Test Isolation
The first matrix attempt exposed twelve Windows tests that injected a fallback command executor while a real host binding was present. Current implementation, documentation, and the dedicated native regression consistently define verified native owner and DACL facts as authoritative.
The repair leaves production precedence unchanged. Synthetic fallback tests in
new-primitives.test.tsandpermissions-exec.test.tsexplicitly select native modeoffand reset the global native configuration after each test. The dedicated native-first regression remains unchanged, and the coverage job still builds and exercises the host binding.Windows Archive Safety
The first complete three-platform run then exposed two real native Windows boundary defects. Native archive extraction accepted NTFS alternate data stream names such as
file:streamwhile the JavaScript backend failed later with raw filesystem errors, and one-UTF-16-code-unit native rename targets could pass a buffer shorter than the complete Windows information structure.TypeScript now rejects colon-bearing archive path segments as typed
entry-patherrors on Windows before either backend writes. POSIX colon names remain valid. The native rename and hardlink primitives now provide at least the complete fixed information structure, with Windows tests covering one-code-unit ASCII and Unicode targets.Coverage Design
One coverage leg per operating system is deliberate. Platform branches are selected by the host, while the normal CI matrix already proves Node 22 and 24. The report job refuses to run without all three artifacts, normalizes source paths to repository-relative
src/paths, generates the standard report formats, and applies the numeric thresholds fromvitest.config.tsto the merged map.The final merged report measures 93.62% lines, 92.47% statements, 90.79% functions, and 87.84% branches.
Proof
pnpm check: 100 files passed, 1,034 tests passed, 61 platform or optional skips.pnpm test:security: 70 tests passed.actionlint .github/workflows/coverage.yml, Rust formatting, andgit diff --checkpassed.35b82ff9ca49fa25691cdf6fd183820923a10d90.