Repository navigation
fix(review): avoid repeated ancestor ACL checks during owner sweep - #2595
Closed
aiorellana502 wants to merge 1 commit into
Closed
aiorellana502 wants to merge 1 commit into
aiorellana502 wants to merge 1 commit into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
Thanks for the work on this PR. After reviewing the open backlog, we're closing it because the change is already covered on |
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
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.
Linked issue
Fixes #2269
PR type
Summary
readOwnerre-ranassertCandidateOwnerParentfor every ancestor on every marker during the startup orphan sweep, even though the same ancestor chain is shared by every marker in a single sweep pass. On Windows this meant ~31powershell/icaclssubprocess spawns, adding roughly 60s to startup.Changes
lib/review-candidate-view-owner.ts: the sweep now validates each ancestor replacement boundary once per sweep pass instead of once per marker, while still validating every owner marker individually and still revalidating boundaries during destructive cleanup. The reuse is sweep-local only — no process-lifetime cache, no change to the per-marker trust decision.tests/review-candidate-view.test.ts: two new regression tests — one confirming ancestors are validated once while every marker is still checked, one confirming rejected boundaries and owner markers are still preserved correctly after the reuse.Test plan
node --experimental-strip-types --test --test-name-pattern="Windows orphan sweep" tests/review-candidate-view.test.ts) — 2/2.tests/review-candidate-view.test.ts: not clean, independent of this change. The file has pre-existing failures/slowness from real Windows ACL (icacls/whoami) subprocess tests unrelated to this fix; three of those were confirmed to reproduce identically onupstream/mainwith no source changes, the rest were not individually classified. Posted on the issue before starting (Startup takes ~60 s on Windows: readOwner re-runs assertCandidateOwnerParent per owner, spawning ~31 powershell/icacls probes #2269 (comment)) and repeating that disclosure here rather than claiming full-suite verification.AI Assistance