Skip to content

fix: align Windows archive safety across coverage - #116

Merged
steipete merged 3 commits into
mainfrom
ci/coverage-matrix
Aug 4, 2026
Merged

fix: align Windows archive safety across coverage#116
steipete merged 3 commits into
mainfrom
ci/coverage-matrix

Conversation

@steipete

@steipete steipete commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • collect V8 coverage once on Linux, macOS, and Windows with Node 22
  • canonicalize platform-specific absolute source paths and require all three artifacts before merging
  • generate final text, JSON, HTML, and LCOV reports from the merged map, then enforce the thresholds from vitest.config.ts
  • keep the native-first Windows permission contract while making synthetic executor tests explicitly select native mode off
  • reject NTFS alternate data stream archive entry names before extraction and fix one-code-unit native Windows rename and hardlink buffers

Windows 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.ts and permissions-exec.test.ts explicitly select native mode off and 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:stream while 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-path errors 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 from vitest.config.ts to the merged map.

The final merged report measures 93.62% lines, 92.47% statements, 90.79% functions, and 87.84% branches.

Proof

  • Local pnpm check: 100 files passed, 1,034 tests passed, 61 platform or optional skips.
  • Local pnpm test:security: 70 tests passed.
  • Local native crate tests: 18 passed; native-enabled coverage collection: 101 files passed, 1,112 tests passed, 23 skips.
  • Local three-input merge dry run generated all report formats and enforced configured thresholds.
  • actionlint .github/workflows/coverage.yml, Rust formatting, and git diff --check passed.
  • Codex autoreview ran on the permission repair, Windows archive fix, and complete branch; TruffleHog was clean and no P0/P1 accepted or actionable findings remained.
  • Full platform, native, Cargo, package-smoke, and workflow CI passed on exact head 35b82ff9ca49fa25691cdf6fd183820923a10d90.
  • Linux, macOS, and Windows coverage collection plus the merged threshold report passed.
  • Benchmarks passed.

@steipete
steipete requested a review from a team as a code owner August 3, 2026 06:42
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Aug 3, 2026
@clawsweeper

clawsweeper Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 4, 2026, 7:04 PM ET / 23:04 UTC.

ClawSweeper review

What this changes

The 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
Reviewed head: 35b82ff9ca49fa25691cdf6fd183820923a10d90

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch is focused and consistent with the current archive contract, but real Windows proof remains a merge gate.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: The body reports local macOS and dry-run results, but no linked exact-head Windows collection, native archive result, or merged-report artifact is available; attach redacted runner proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The body reports local macOS and dry-run results, but no linked exact-head Windows collection, native archive result, or merged-report artifact is available; attach redacted runner proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 4 items Current Windows contract: Current main’s portability test explicitly expects ADS entry names to fail with raw ENOENT on Windows, establishing the backend-dependent behavior this PR changes to entry-path rejection.
Archive validation boundary: Current main validates entry paths before native extraction, so the proposed early Windows validation applies consistently to the extraction boundary.
Policy provenance: The earlier archive-hardening commit deliberately recorded ADS behavior without introducing a portable rejection policy; this PR supplies the Windows-specific policy and regression coverage.
Findings None None.
Security None None.

How this fits together

Archive 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]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The body reports local macOS and dry-run results, but no linked exact-head Windows collection, native archive result, or merged-report artifact is available; attach redacted runner proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - The Windows ADS change deliberately replaces a raw ENOENT with the public entry-path error contract, so Windows consumer compatibility needs exact-head proof.
  • Resolve merge risk (P1) - The new artifact merge and threshold job has no completed exact-head Windows collection or merged-report artifact yet.
  • Resolve merge risk (P1) - This security-boundary change remains a draft until both JavaScript and native archive paths are demonstrated on Windows.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope 13 files; 267 added, 34 removed The patch couples a Windows security-contract correction with CI reporting, so both runtime and workflow evidence matter.
Coverage topology 3 collection legs, 1 merge job All three platform artifacts are required before the new threshold gate can represent cross-platform coverage.

Merge-risk options

Maintainer options:

  1. Add exact-head Windows proof (recommended)
    Attach redacted Windows runner output or artifacts showing the ADS cases, native binding path, and successful three-input merged coverage report before merge.
  2. Keep the draft paused
    Leave this PR in draft state until the Windows matrix and merged report can verify the changed archive contract.

Technical review

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

Labels

Label changes:

  • add merge-risk: 🚨 compatibility: Windows archive consumers currently observe ENOENT for ADS names, while this patch documents and returns entry-path instead.

Label justifications:

  • P3: This is low-urgency hardening and CI measurement work, with no demonstrated current user outage.
  • merge-risk: 🚨 compatibility: Windows archive consumers currently observe ENOENT for ADS names, while this patch documents and returns entry-path instead.
  • merge-risk: 🚨 security-boundary: The patch changes validation of attacker-controlled archive entry names before filesystem extraction.
  • merge-risk: 🚨 automation: The PR replaces one coverage job with a cross-platform artifact collection and merge workflow.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The body reports local macOS and dry-run results, but no linked exact-head Windows collection, native archive result, or merged-report artifact is available; attach redacted runner proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Current Windows contract: Current main’s portability test explicitly expects ADS entry names to fail with raw ENOENT on Windows, establishing the backend-dependent behavior this PR changes to entry-path rejection. (test/archive-property-fuzz.test.ts:244, d4634144acc3)
  • Archive validation boundary: Current main validates entry paths before native extraction, so the proposed early Windows validation applies consistently to the extraction boundary. (src/archive-native.ts:106, d4634144acc3)
  • Policy provenance: The earlier archive-hardening commit deliberately recorded ADS behavior without introducing a portable rejection policy; this PR supplies the Windows-specific policy and regression coverage. (test/archive-property-fuzz.test.ts:132, ed06666a28eb)
  • Exact PR object unavailable locally: The checkout’s promisor remote could not resolve GitHub while materializing the PR head; current-main source and supplied exact-head patch data were still inspected.

Likely related people:

  • steipete: Authored the current main archive-hardening commit that introduced the ADS portability contract and related generated-input coverage. (role: archive safety policy author and recent area contributor; confidence: high; commits: ed06666a28eb, d4634144acc3; files: src/archive-entry.ts, test/archive-property-fuzz.test.ts, test/helpers/property.ts)
  • Vincent Koc: Authored the current main Windows test-worker stabilization that owns the platform-specific Vitest configuration adjacent to this coverage change. (role: recent Windows CI contributor; confidence: medium; commits: fe009ad3422a; files: vitest.config.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Attach redacted exact-head Windows logs or artifacts for ADS rejection and the merged three-platform coverage report.
  • Update the PR body after proof is available; a maintainer can request a fresh review if needed.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (4 earlier review cycles)
  • reviewed 2026-08-03T06:46:39.152Z sha 7cb285b :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-03T07:02:16.317Z sha b9c1e13 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-03T10:34:01.092Z sha b9c1e13 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-03T11:57:01.383Z sha b9c1e13 :: needs real behavior proof before merge. :: [P1] Resolve the Windows contract before requiring the matrix

@steipete
steipete marked this pull request as draft August 3, 2026 06:58
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 3, 2026
@steipete
steipete force-pushed the ci/coverage-matrix branch from b9c1e13 to e6af38a Compare August 4, 2026 22:31
@steipete steipete changed the title ci: merge coverage across platforms fix: align Windows archive safety across coverage Aug 4, 2026
@steipete
steipete marked this pull request as ready for review August 4, 2026 23:04
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. label Aug 4, 2026
@steipete
steipete merged commit ec5519f into main Aug 4, 2026
29 of 30 checks passed
@steipete
steipete deleted the ci/coverage-matrix branch August 4, 2026 23:06
@steipete

steipete commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Landed as ec5519f8eecf16e8c06088fedbaebf424c8b57df.

Verification performed on exact head 35b82ff9ca49fa25691cdf6fd183820923a10d90:

  • Local pnpm check: 100 test files passed, 1,034 tests passed, 61 platform or optional skips.
  • Local pnpm test:security: 70 tests passed.
  • Local native crate tests: 18 passed. Native-enabled coverage collection passed with 101 files, 1,112 tests, and 23 skips.
  • A local three-input merge dry run generated all report formats and enforced the thresholds from vitest.config.ts.
  • actionlint .github/workflows/coverage.yml, Rust formatting, and git diff --check passed.
  • Codex autoreview covered the permission-test isolation, Windows archive safety fix, and complete branch. TruffleHog was clean; the final security review reported no accepted or actionable P0/P1 findings.
  • Full CI passed across Node 22 and 24 on Linux, macOS, and Windows; native checks passed on Linux, macOS, Windows, and musl; Cargo audit/clippy, package smoke, workflow lint, benchmarks, and CodeQL passed: https://github.com/openclaw/fs-safe/actions/runs/30958473070
  • All three native-enabled coverage collectors and the merged threshold report passed: https://github.com/openclaw/fs-safe/actions/runs/30958473065
  • Merged coverage: 93.62% lines, 92.47% statements, 90.79% functions, and 87.84% branches.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant