Complete temp deletion rights and identity acceptance - #44
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 24 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
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 |
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 1 minor |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f93ae011e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Initialize the native fixture in isolation and make parent ACL setup failure-terminating; clarify the documented disappearance behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds NTFS ACL and native identity acceptance tests for temp deletion, with supporting documentation and no runtime changes.
Changes:
- Adds ACL, deletion-failure, substitution, disappearance, and identity tests.
- Documents deletion rights, sharing behavior, and ReFS limitations.
- Updates release evidence and changelog.
File summaries
| File | Description |
|---|---|
src/Tests/Unit/TempDeletionRights.Tests.ps1 |
Adds ACL and native identity regression coverage. |
docs/safety-and-confirmation.md |
Documents deletion-rights behavior. |
docs/release-plan-1.0.md |
Records validation evidence and remaining gates. |
docs/issue-29-delete-rights.md |
Provides acceptance mapping and native contract details. |
CHANGELOG.md |
Summarizes the issue #29 evidence packet. |
Review details
Suppressed comments (2)
docs/issue-29-delete-rights.md:51
- This sentence overstates the command's error behavior: both cleanup commands intentionally convert a missing-candidate/open failure with native codes 2/3/53/123 into
FilesSkippedwithout writing an error, as required by the disappearance contract. Qualify the statement to non-missing failures and mention that disappearance is reconciled as a skip so it agrees with the mapping below.
path between validation and disposition. Any native open, identity, metadata,
or disposition failure throws and is reported through the command error stream.
src/Tests/Unit/TempDeletionRights.Tests.ps1:87
- This ACL mutation is allowed to fail non-terminatingly, so the test can continue with only the file-level DELETE deny applied. In that case
OpenForDeletionstill returns access denied and the assertions pass without ever exercising the parentFILE_DELETE_CHILDdenial that the fixture and documentation claim to cover. Make the fixture setup terminating (and keep the explicit rule verification) so a failed parent ACL application cannot produce a false pass.
Set-Acl -LiteralPath $TempRoot -AclObject $RootAcl
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
SamErde
left a comment
SamErde
left a comment
There was a problem hiding this comment.
Independent model review by GPT-6 Astra, high effort, separate from GPT-5.6 Sol, high effort implementation. This COMMENT is posted under the PR-author account and does not represent a separate human-maintainer approval. The final rebase and shared-document delta will receive a bounded follow-up review before merge.
PR #44 bounded follow-up review
APPROVE the local committed head 7ce8c0f6796617c47c046a4d4c90ca2ed86d4a96. No material findings. This follows the independent review of 5f93ae011e1117eb1030b814ca6e059829708f8f; it is not approval of an unseen subsequent rebase or documentation integration.
Reviewed the complete delta, limited to TempDeletionRights.Tests.ps1 and docs/issue-29-delete-rights.md. Executable module code is unchanged.
- Test lines 287-338 now exercise equality through every one of the 16 identifier bytes, independently varying each position. They also distinguish volume serials, accept equal cloned arrays, and verify that metadata changes do not alter the intended volume-plus-file-ID identity contract. Equal identities retain equal hashes. These checks directly address the upper-64-bit regression-coverage request.
- Test lines 22-24 initialize the native type in the file's own
BeforeAll, with a runtime platform check. This removes dependence on a different test or an external runner initializing the type. - ACL setup and readback use
-ErrorAction Stop; the added assertion verifies both fileDeleteand parentDeleteSubdirectoriesAndFilesdeny rules before either negative command case. The existing native-open check still proves access is genuinely denied. - Test lines 135-165 attempt both ACL restorations independently, always restore TEMP/TMP and dispose the token identity, and then surface any restoration error. Fixture mutation remains limited to unique TestDrive paths. Positive read denial and real command outcomes remain asserted.
- The documentation now correctly distinguishes intentional missing-candidate skips from surfaced native failures. Historical dirty-tree evidence remains labeled as such; it is not passed off as immutable final-head evidence.
Independent standalone validation
Used Invoke-Pr44StandaloneReview.ps1 from the evidence directory. Each fresh process imported only pinned Pester 5.7.1 before invoking the test file. The runner asserted that TheCleaners.NativeFileInterop was absent before the run; it did not dot-source or invoke the initializer.
| Runtime | Commit / tree | Results |
|---|---|---|
| PowerShell 7.6.6 | 7ce8c0f6796617c47c046a4d4c90ca2ed86d4a96, clean |
12 passed; 0 failed, skipped, not run |
| Windows PowerShell 5.1.26100.9444 | Same commit, clean | 12 passed; 0 failed, skipped, not run |
Both ran on Windows 10.0.26200.0. Test SHA-256: D439B0D1FC4EDC0AB2EB4F11FCA87DD6A523C5667F6411169E2105E90581C082. Machine-readable reports: pr44-astra-7ce8c0f-standalone-ps7.json / .xml and pr44-astra-7ce8c0f-standalone-ps51.json / .xml. The reviewed delta passes git diff --check.
No repository edits, external posts, volume provisioning, or live-data cleaner execution were performed. The final rebase/shared-document delta still needs bounded review and exact final-head hosted checks/artifact verification. Deferred ReFS and broader product/lab gates remain unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ce8c0f679
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
The read-denial test does not yet perform an actual content read.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
docs/release-plan-1.0.md:57
- The release-ledger totals do not match the checked-in test structure: this file has five
Itblocks under each of the two command cases (10) plus two native-identityItblocks (12 new tests), andDocumentationContract.Tests.ps1adds four more. Please report 16/16 (12+4), not 15/15, and update the 11-case wording consistently.
docs/issue-29-delete-rights.md:172
- The closing sentence incorrectly calls the two 11-test XML reports the exact evidence for this packet, while the current test file has 12 focused cases and the preceding review checkpoint explicitly records 12/12. Keep those 11-test reports as historical evidence only, or point this sentence to the current 12-test/final integration evidence.
evidence for the final working copy. The two 11-test XML reports above were
regenerated after normalization and are the exact local evidence for this
packet.
docs/release-plan-1.0.md:55
- This new packet is marked implemented in draft, but the earlier
Current non-lab delivery sequencestill says the #29 fixture/contract criteria “remain unexecuted” (line 38). That leaves the release ledger contradictory; update the earlier Remaining status or explicitly label that sentence as historical before this packet.
**Implemented in draft:** [Issue #29's deletion-rights packet](issue-29-delete-rights.md) adds 11 cases covering both temp commands with real NTFS read-denied/delete-allowed and delete-denied ACLs, errors/Stop/reconciliation, disappearance/directory substitution, 128-bit native identity and invalid-handle fail-closed behavior. No executable runtime change was required.
src/Tests/Unit/TempDeletionRights.Tests.ps1:122
- This block only calls
File.Open; it never invokesRead/ReadByte, so it proves that requesting a read handle is denied, not that an actual content read is denied. The evidence table claims a realFileStreamread is tested; consume at least one byte after opening so the fixture covers the stated acceptance criterion.
$ReadStream = [System.IO.File]::Open(
$CandidatePath,
[System.IO.FileMode]::Open,
[System.IO.FileAccess]::Read,
[System.IO.FileShare]::ReadWrite
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
7ce8c0f to
f7da103
Compare
SamErde
left a comment
SamErde
left a comment
There was a problem hiding this comment.
Independent model review: GPT-6 Astra, high effort, reviewing Sol high's implementation. Verdict: APPROVE exact head f7da103; no material findings. This is posted as COMMENT because the authenticated PR author account cannot provide a separate human approval.
Verified the clean tree, ancestry on merged PR #43 (0829d07), full rebase delta and final PR diff. The changelog preserves both packets; native/public runtime and the deletion-rights tests are identical to approved pre-rebase 7ce8c0f. Dedicated test SHA-256 remains D439B0D1FC4EDC0AB2EB4F11FCA87DD6A523C5667F6411169E2105E90581C082.
The ledger, recovery prompt and evidence table correctly distinguish merged #42/#43, draft #44, historical checkpoints, current 12-case suite and pending hosted/merged-source gates. Inspected clean rebased a1f3b2a reports: 16/16 including four docs contracts on PS7.6.6 and Windows PS5.1.26100.9444 with Pester5.7.1, zero failures/skips/not-run. My independent fresh-process standalone tests at unchanged 7ce8c0f passed 12/12 on both runtimes with no native type initialized beforehand. Final PR whitespace passes.
CodeRabbit's quota-skipped review remains incomplete, not an approval. Exact-head hosted checks/artifact inspection and the final paginated thread audit remain integrator gates. ReFS execution, product labs and final release acceptance remain deferred.
There was a problem hiding this comment.
🔵 Needs a closer look
Conflicting validation evidence and specialized Windows ACL/identity scope require human verification.
Review details
Suppressed comments (3)
docs/issue-29-delete-rights.md:17
ParentIdentityis not captured for every candidate:Get-TheCleanersTempPlanonly populates it when-RemoveEmptyDirectoryis enabled (src/TheCleaners/Private/Get-TheCleanersTempPlan.ps1:204-210), and all cases in this packet omit that switch. Please qualify this contract so the note does not claim the normal file-removal path captures parent identity.
Both temp commands discover a candidate and capture its volume serial number,
128-bit file ID, attributes, logical length, UTC last-write time, and parent
identity. After `ShouldProcess`, the command opens the current literal path with
docs/issue-29-delete-rights.md:123
- The PR description still reports
51ef...as the exact clean validation with 15/15 tests (11 new plus four documentation contracts), but this added suite contains 12 cases (five per command plus two identity cases) and this evidence records a different clean commit,a1f..., with 16/16. Please synchronize the PR description and evidence so the acceptance record identifies one final result unambiguously.
Exact clean rebased commit `a1f3b2a3e3d66c331de31c7457a17e733cab3582`
passed **16/16** tests: 12 deletion-rights cases plus four documentation
contracts, under PowerShell **7.6.6** and Windows PowerShell
**5.1.26100.9444**, with pinned Pester **5.7.1** on Windows
**10.0.26200.0**. Both runs had zero failures, skips and not-run tests. Retained
docs/release-plan-1.0.md:66
- The ledger now records the final rebased head
a1f3b2a3e3d66c331de31c7457a17e733cab3582with 12 focused cases and 16/16, while the PR description still identifies51ef3783f8f89c15b870042579c09a6073e816f3and 15/15 (11 new cases) as the exact validation. This leaves reviewers with conflicting final evidence; update the PR description/checklist to cite the current final head and count, and retain51ef...only as historical evidence.
Exact clean rebased commit `a1f3b2a3e3d66c331de31c7457a17e733cab3582` passed **16/16** tests (12 new cases plus four documentation contracts) with Pester **5.7.1** under PowerShell **7.6.6** and Windows PowerShell **5.1.26100.9444** on Windows **10.0.26200.0**, zero failures/skips/not-run. Independent Astra high review of pre-rebase head `7ce8c0f6796617c47c046a4d4c90ca2ed86d4a96` also passed 12/12 standalone cases in fresh processes on both hosts with no native type initialized beforehand. Historical integration `51ef3783f8f89c15b870042579c09a6073e816f3` passed 15/15 before the all-byte and standalone refinements. Official Microsoft API documentation determines the ReFS identity design; ReFS runtime tests remain explicitly unexecuted TC-003/004 acceptance. The packet documents why the existing stricter `FILE_SHARE_READ` contract supersedes the issue comment's proposed write/delete sharing, including the attribute-only timestamp exception. Issues #30 and #28 are merged; final-head hosted validation and final review of this packet remain required before closure. Broader product/release acceptance remains separate.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
SamErde
left a comment
SamErde
left a comment
There was a problem hiding this comment.
Independent Astra high follow-up: APPROVE exact head abe5f42, no findings. The only delta from approved f7da103 is the native-access table value, now written as separate DELETE and FILE_READ_ATTRIBUTES code spans. This preserves the flag meaning and avoids an extra table cell in Markdown renderers. The strict Zensical build passed and the emitted row contains exactly three complete cells. Tests and runtime sources remain unchanged; the previous independent review and exact-source local evidence retain their stated scope. Fresh hosted checks and final thread audit remain pending.
There was a problem hiding this comment.
🔵 Needs a closer look
An unresolved moderate test-gap finding remains, and the native ACL/identity scope warrants human review.
Review details
Suppressed comments (1)
src/Tests/Unit/TempDeletionRights.Tests.ps1:124
- This helper only opens a stream with
FileAccess.Readand then returns; it never performs aRead, so the fixture does not prove the stated requirement that an actual content read is denied. Read at least one byte after opening so the test distinguishes a denied content operation from an open-only permission check.
)
return $null
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Final readiness evidence for exact head
Copilot's final suppressed read-access suggestion was independently checked by Astra high and is a false positive. Merged-source builds/deployment will be verified separately after merge. NTFS fixture closure does not establish ReFS, actual-root, Windows client/server or IIS/Exchange product acceptance. |
|
Post-merge verification is complete for Build 35340908362 independently passed the merged-source matrix: PS7.4.20/7.5.11/7.6.6 each 359 unit + 4 integration, Windows PS5.1.26100.33296 363 combined, zero failures/skips/not-run, 88.05% PS7 coverage. All eleven artifact-wrapper digests, source/runtime reports and NUnit cases, ACL/local-feed evidence and repeated/cross-runtime archives were inspected. Package SHA-256 remains Analyzer 35340908372 passed. Documentation deployment 35340908280 and an independent live check verified all 39 files plus required navigation against the retained manifest. Deployment commit: The separate final documentation sweep is underway. Broader product, actual-root, ReFS and release acceptance remain deferred/open. |
Type of Change
Issue
Closes #29, including its expanded owner-comment scope. The replacement/retention criteria are supplied by merged #42; this PR verifies the combined source in its final matrix.
Release-plan packet
TC-003/004: deterministic deletion-rights and native-identity acceptance. ReFS runtime and broader Windows/product/release gates stay open.
Description
Add 12 native NTFS ACL/identity cases for both temp commands. Prove an actual file-content read is denied while authorized deletion succeeds; deny both file DELETE and parent FILE_DELETE_CHILD to test real deletion failures, PermissionDenied, Stop and counters. Verify read-denied disappearance/directory substitution, equality across all 16 identity bytes and volume identity, and invalid-handle fail-closed behavior. Initialize native interop in the standalone suite, verify ACL setup with terminating errors, and restore fixture ACLs, environment variables and handles.
Document why the existing stricter FILE_SHARE_READ contract is retained instead of the issue comment's proposed write/delete sharing: data-writer/rename exclusion protects the validated object, while attribute-only timestamp changes remain possible and explicitly documented by #30. Determine ReFS identity semantics from Microsoft's primary API contract and the implementation's full 128-bit comparison/fail-closed handling. NTFS results are not represented as ReFS execution. No runtime behavior is changed.
Update safety guidance, changelog, release ledger and the issue evidence note. All original acceptance criteria are mapped to evidence; final product or stable-release acceptance is not implied.
Validation
Exact final head
abe5f4275b2d5b9a7a510aa0759413dbcf91a727passed build 35339431249: PS7.4.20/7.5.11/7.6.6 each passed 359 unit + 4 integration tests; Windows PS5.1.26100.33296 passed 363 combined tests, with zero failures/skips/not-run. PS7 coverage is 88.05% (1,636/1,858). Independent inspection verified all eleven artifact-wrapper digests, exact source/runtime JSON and NUnit results, ACL and isolated local-feed rehearsal evidence, and identical repeated/cross-runtime archives. Package: 19 files, 224,558 bytes, SHA-2565153966aaef9f194fdf60fe8793989f4693dcd3310b10c4223d6208865d0135f. All final-head workflow/check-run/status results pass.Local evidence remains separately scoped: clean rebased
a1f3b2a3e3d66c331de31c7457a17e733cab3582passed 16/16 (12 new plus four docs contracts) on PowerShell 7.6.6 and Windows PowerShell 5.1.26100.9444, Pester 5.7.1, Windows 10.0.26200.0, zero failed/skipped/not-run. Independent review passed 12/12 standalone cases on each host at clean pre-rebase7ce8c0f, with native interop absent before invocation. Strict Zensical 0.0.62, parser and full PR-range whitespace checks pass. The corrected native-access table was also checked in rendered HTML.Implementation: GPT-5.6 Sol/high. Independent review: GPT-6 Astra/high, selected for the native ACL/identity and ReFS scope; approval of the exact final head is posted as a model-review COMMENT, not a separate human approval. Copilot completed its final review. CodeRabbit reported a review quota limit; that review is recorded as incomplete, not approval.
Checklist
AGENTS.md, release plan and expanded issue comments.