test(permissions): budget the Windows ACL tests for the no-native path

[Yigtwxx]

What Problem This Solves

new-primitives.test.ts > secure file reads > reads from a validated Windows ACL and owner times out at 15s on Windows CI. It has failed that way on #82, #85 and #87, always on one Node version while the other passes on the identical commit. The affected surface is test reliability for the Windows permissions fallback — nothing ships differently.

The cause is that the Node N check job never runs pnpm native:build, so these tests run the command fallback rather than the native readOwnerAndDacl(). Instrumenting defaultPermissionExec shows the cost:

SPAWNS inspect=2 total=4 ms=546
LOG ["powershell.exe -NoLogo -NoProfile -NonInteractive -Enco",
     "icacls.exe C:\...\secret.json",
     "powershell.exe -NoLogo -NoProfile -NonInteractive -Enco",
     "icacls.exe C:\...\secret.json"]

Each inspectPathPermissions() spawns powershell.exe for the owner query (inspectWindowsOwner) plus icacls.exe for the ACL. Each test pays six process spawns: two icacls in secureWindowsTestFile, then two inspecting calls at two spawns each.

Why This Change Was Made

This is a timing budget for a spawn-bound test, not a slow assertion, so the budget is the thing that is wrong. Both Windows ACL tests do identical work and both move from 15s to 60s.

Cold process starts dominate on a Windows runner. The same failing run showed the machine five to ten times slower than local across unrelated suites — api-coverage at 6833ms and archive at 5306ms — and a test whose cost is almost entirely CreateProcess scales worse than wall-clock I/O does. Locally these tests take 561ms.

Non-goals: this does not change the library. permissions.ts still prefers inspectWindowsPermissionsNative(), which performs no spawns, and only falls back to the commands when no binding is loaded. Jobs that do build the binding never pay this cost.

User Impact

None for package consumers. Test-only change.

Evidence

Local run of the affected file, all green:

Test Files  1 passed (1)
     Tests  59 passed | 11 skipped (70)

Full local verification: vitest run 487 passed / 183 skipped, tsc --noEmit clean, scripts/check-file-size.mjs and scripts/check-fs-boundary-primitives.mjs both exit 0.

The spawn counts above come from temporarily instrumenting defaultPermissionExec to record each command; that instrumentation is not part of this diff.

Separate finding, not fixed here

defaultPermissionExec in src/permissions.ts calls execFileAsync with no timeout, and so does secureWindowsTestFile. A wedged icacls.exe or powershell.exe would hang inspectPathPermissions() indefinitely for a consumer, not just in tests. That is a real robustness gap but it is a behaviour change to public API, so I have left it out of a test-only PR. Happy to open it separately if you want it bounded.

• Tests added or updated when behavior changed
• Security and compatibility impact considered
• CHANGELOG.md updated when release-relevant — test-only, nothing ships
• No credentials, private paths, private hosts, or sensitive contents included

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed August 2, 2026, 5:04 PM ET / 21:04 UTC.

ClawSweeper review

What this changes

The PR raises the timeout for two Windows-only secure-read ACL tests from 15 seconds to 60 seconds when the command-based permissions fallback is exercised.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

This is a focused, correct test-only mitigation for repeated Windows command-fallback timeout failures. Keep it open: the linked broader replacement changes public fallback behavior and has not yet supplied the Windows proof needed to make it a safe superseding landing path.

Priority: P3
Reviewed head: 8c3a71d65bddb0cbfc059627bfd86d58298a877c
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	The patch is focused and evidence-backed; the remaining blocker is a maintainer choice about whether the broader replacement should supersede it.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (terminal): The PR body contains after-change terminal output for the affected test file and broader validation, consistent with the test-only scope.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The PR body contains after-change terminal output for the affected test file and broader validation, consistent with the test-only scope.
Evidence reviewed	5 items	Focused branch diff: The branch changes only the explicit timeout values on the two Windows-only ACL secure-read tests, from 15 seconds to 60 seconds. Current fallback test shape: Each affected test configures the Windows file ACL, calls permission inspection directly, then calls secure read and checks its returned permission record; the test therefore performs repeated inspection work. Current implementation routing: Current main imports the native Windows permission reader and routes Windows inspection through native inspection before the owner and ACL command helpers; the command fallback is an intentional security-sensitive path.
Findings	None	None.
Security	None	None.

How this fits together

Secure file reads verify ownership and access controls before returning secret-like file contents. On Windows, verification prefers a native binding when available and otherwise uses system ACL commands; these tests cover the resulting secure-read behavior.

flowchart LR
  A[Windows secure-read test] --> B[ACL-protected test file]
  B --> C[Permission inspection]
  C --> D{Native binding available?}
  D -->|No| E[PowerShell and icacls fallback]
  D -->|Yes| F[Native ACL reader]
  E --> G[Verified secure read]
  F --> G

Loading

Decision needed

Question	Recommendation
Should the repository land this isolated Windows test-budget adjustment now, or wait for the broader command-bounding replacement to receive real Windows proof and then retire this branch?	Land the focused budget: Merge the two-line test-only timeout increase now while keeping production permission behavior unchanged.

Why: The two paths overlap on Windows fallback test cost, but the replacement also changes production command-failure behavior and is not yet proven on the affected platform.

Before merge

• Resolve merge risk (P2) - Waiting for the broader replacement without Windows proof would leave the known timeout-prone test budget unresolved, while merging that replacement carries additional public fallback-behavior risk not present in this PR.
• Resolve merge risk (P2) - The reported Ubuntu root-containment failure is outside these Windows-gated timeout lines and should be triaged independently rather than treated as a defect introduced by this branch.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Changed test budgets	2 changed, 0 added, 0 removed	The branch is confined to the timeout budgets of two Windows-only tests.

Merge-risk options

Maintainer options:

1. Decide the mitigation before merge
   Either land this narrow timeout adjustment as an immediate CI-stability mitigation, or obtain real Windows proof for the broader replacement and close this PR only after that replacement demonstrably preserves the same test reliability benefit.
2. Pause or close
   Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Either land this narrow timeout adjustment as an immediate CI-stability mitigation, or obtain real Windows proof for the broader replacement and close this PR only after that replacement demonstrably preserves the same test reliability benefit.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR changes existing Windows test budgets rather than an end-user behavior. The supplied terminal output reports the affected test file passes after the adjustment.

Is this the best way to solve the issue?

Unclear: the timeout increase is the narrowest mitigation for the stated CI symptom, but the broader replacement could be preferable if it gains Windows proof and safely removes the repeated command work.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 2477f5681f68.

Labels

Label justifications:

• P3: This is a low-risk CI test-reliability adjustment with no direct package-runtime behavior change.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body contains after-change terminal output for the affected test file and broader validation, consistent with the test-only scope.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR body contains after-change terminal output for the affected test file and broader validation, consistent with the test-only scope.

Evidence

What I checked:

• Focused branch diff: The branch changes only the explicit timeout values on the two Windows-only ACL secure-read tests, from 15 seconds to 60 seconds. (test/new-primitives.test.ts:242, 8c3a71d65bdd)
• Current fallback test shape: Each affected test configures the Windows file ACL, calls permission inspection directly, then calls secure read and checks its returned permission record; the test therefore performs repeated inspection work. (test/new-primitives.test.ts:217, 2477f5681f68)
• Current implementation routing: Current main imports the native Windows permission reader and routes Windows inspection through native inspection before the owner and ACL command helpers; the command fallback is an intentional security-sensitive path. (src/permissions.ts:151, 2477f5681f68)
• Feature provenance: Blame attributes both affected test blocks and their original 15-second budgets to the v0.5.1 release commit, tying the test surface to the current Windows ACL/owner verification feature. (test/new-primitives.test.ts:217, 16e1bd489ae8)
• Replacement is not yet a safe close target: The linked open replacement explicitly says Windows timing evidence will come from its CI run while also changing production command deadlines and failure handling. That is meaningful overlap, but not proof-positive supersession of this isolated test-only PR.

Likely related people:

• Peter Steinberger: Git blame assigns the affected Windows ACL secure-read tests to the v0.5.1 release commit authored by Peter Steinberger, and the linked broader replacement is also attributed to him in the supplied review context. (role: Windows permissions feature introducer and recent area contributor; confidence: high; commits: 16e1bd489ae8, 2477f5681f68; files: test/new-primitives.test.ts, src/permissions.ts, CHANGELOG.md)

Rank-up moves

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

• Choose whether to merge this isolated mitigation or first require real Windows proof from the broader replacement.

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 (2 earlier review cycles)

• reviewed 2026-08-02T19:14:45.858Z sha 8c3a71d :: needs maintainer review before merge. :: none
• reviewed 2026-08-02T19:44:56.559Z sha 8c3a71d :: needs maintainer review before merge. :: none

[Yigtwxx]

Node 24 check (ubuntu-latest) is red on this branch. It is not from this diff — and I do not think it should be written off as noise either.

This PR changes two numbers, 15_000 to 60_000, on tests guarded by it.runIf(process.platform === "win32"). On ubuntu those tests do not run, so the timeout value has no effect there.

What failed:

AssertionError: expected [ 'logs', 'secret.txt' ] to deeply equal [ 'secret.txt' ]
 ❯ test/write-boundary-bypass.test.ts:336:51

That is does not create directories outside the root when copyIn fallback races a parent symlink swap. The assertion found a logs directory outside the root, which is the exact escape the test exists to catch: the swapper replaces data with a symlink to layout.outside while copyIn("data/logs/2026/07/app.log") runs, and one of the 400 attempts appears to have created the parent chain through the swapped symlink. The test sets configureFsSafeNative({ mode: "off" }), so this is the command/JS fallback rather than the native path.

Caveats on how far I can take that claim:

• One observation. I checked recent main runs and the ubuntu failure there was json.test.ts:329 (expected 9 to be greater than 10), a different test, so this is not obviously the same known-flaky family.
• I could not reproduce it. The boundary-bypass tests are runIf(process.platform !== "win32") and I am on Windows, so I have no way to run the racing case locally or to tell an intermittent real escape from a harness artifact.

Flagging rather than diagnosing, since a genuine intermittent containment escape in the non-native fallback would matter well beyond this PR. Anyone with Linux CI can hit it far more cheaply than I can from here.

[steipete]

Superseded by #89, merged as 64f7742 — thank you, your diagnosis was the whole basis for it.

I didn't take the timeout increase, but only because your own analysis pointed at something better. You correctly identified that the Node N check job never runs pnpm native:build, so those tests exercise the command fallback and pay six process spawns each. Rather than widen the budget to fit that cost, #89 removes a redundant inspection so the affected tests drop to four spawns, and keeps ordinary Windows CI on the real command fallback — that path is real shipping code and deserves the coverage it was getting by accident.

The more important part is the finding you flagged and deliberately left out of a test-only PR: defaultPermissionExec calling execFileAsync with no timeout. You were right that it is a real robustness gap, and right that it was bigger than the flake. A wedged icacls.exe or powershell.exe would hang inspectPathPermissions() indefinitely for any consumer. It is now bounded to 10 seconds and fails closed — an owner query that cannot complete yields source: "unknown", the ACL query is skipped, and readSecureFile() refuses with permission-unverified rather than returning a permissive answer.

The affected test went from 15-second timeouts to about four seconds on the merged branch. Thanks for chasing the cause instead of just the symptom, and for scoping the PR honestly rather than smuggling the API change in with it.