fix(permissions): bound Windows ACL commands - #89
Merged
Conversation
|
Codex review: needs maintainer review before merge. Reviewed August 2, 2026, 5:10 PM ET / 21:10 UTC. ClawSweeper reviewWhat this changesThe PR gives fallback Windows owner and ACL commands a 10-second deadline, returns an unverified result after owner-query failure, removes duplicate secure-read inspections from Windows tests, and documents fallback CI coverage. Merge readinessThis PR addresses a real unbounded Windows permission-fallback path that remains on current Priority: P2 Review scores
Verification
How this fits togetherWindows permission inspection protects secure file reads when the optional native binding is unavailable. The fallback gathers Windows owner and ACL facts through system commands, classifies access, and secure reads reject files whose ownership or permissions cannot be verified. flowchart LR
A[Secure file read] --> B[Permission inspection]
B --> C{Native binding available?}
C -->|Yes| D[Native ACL facts]
C -->|No| E[Bounded Windows commands]
D --> F[Permission classification]
E --> F
F --> G{Safe and verified?}
G --> H[Read file or reject]
Decision needed
Why: The code and runtime proof support the security and availability benefit, but the chosen deadline changes observable behavior for existing fallback users and therefore needs maintainer compatibility intent. Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the bounded, explicit, fail-closed fallback; merge it once maintainers accept the documented 10-second compatibility boundary and retain separate native and JavaScript-fallback CI coverage. Do we have a high-confidence way to reproduce the issue? Yes. Current main has no deadline in the default Windows permission executor, and the branch adds a deterministic wedged-child regression that exercises the new bounded executor and public inspection behavior. Is this the best way to solve the issue? Yes, subject to the compatibility decision. A shared bounded executor plus an unverified result after owner failure is narrower and safer than retaining an unbounded command path or only relaxing test timeouts. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 2477f5681f68. LabelsLabel 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 (1 earlier review cycle)
|
steipete
force-pushed
the
fix/permissions-exec
branch
from
August 2, 2026 21:43
04efaf1 to
9726e1d
Compare
4 tasks
steipete
added a commit
that referenced
this pull request
Aug 2, 2026
The 10-second bound added in #89 was too tight for the command fallback and fired on contended Windows runners, producing an intermittent permission-unverified failure on unrelated pull requests. That code means "we could not verify", which a consumer may reasonably treat as a security signal, so emitting it because a runner was busy is a false alarm with real consequences. Successful runs of the affected secure-read test measure 1.930s to 5.400s, and one Node 24 leg passed at 9.349s with roughly 651ms of margin. The contended failures on #94 and #95 landed at 10.130s and 10.173s, which is the old bound firing rather than a hang. Thirty seconds is over five times the slowest observed success and about three times the overloaded cutoff, while still terminating a genuinely wedged command rather than letting it hang indefinitely. The deadline stays per process, since each child is the independently wedgable unit and an overall budget would let several slow-but-successful commands compete for one allowance and reproduce the same false result. Fail-closed behavior is unchanged: a real failure or timeout still yields source "unknown", and secure reads still reject it as permission-unverified.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
This supersedes #88. Credit to @Yigtwxx for the careful diagnosis of the missing native build, the six-spawn test shape, and the unbounded production executor.
Why
The timeout increase in #88 treated runner latency rather than the underlying issues. Without the native binding, each permission inspection launches PowerShell for owner facts and icacls for ACL facts. The affected tests inspected once directly and then repeated that work through the secure read. More importantly, the production executor had no deadline, so a wedged system command could hang a caller indefinitely.
The ordinary matrix intentionally remains a fallback test rather than building native everywhere. That preserves real Windows coverage of shipping fallback code. Removing the duplicate inspection cuts each affected test from six real process launches to four, including its two ACL setup commands.
Proof
pnpm check: 648 passed, 25 skippedpnpm test:security: 62 passedgit diff --checkOn macOS, a fake PowerShell executable that sleeps indefinitely left the pre-change public inspection call running beyond 2.00 seconds. With this change,
inspectPathPermissions()returnedsource: "unknown"with an explicit timeout error in 10.07 seconds.Both real Windows fallback jobs are green.
reads from a validated Windows ACL and ownercompleted in 3.962 seconds on Node 22 and 4.367 seconds on Node 24; the extended-path case completed in 439 ms and 430 ms. The deterministic wedged-child regression passed in the same jobs as part of the 157 ms / 155 ms permission-executor suite.