fix(agent): harden policy storage transactions - #1978
Open
Benoît Cortier (CBenoit) wants to merge 20 commits into
Open
fix(agent): harden policy storage transactions#1978Benoît Cortier (CBenoit) wants to merge 20 commits into
Benoît Cortier (CBenoit) wants to merge 20 commits into
Conversation
Benoît Cortier (CBenoit)
changed the base branch from
cbenoit-plan-agent-policy-endpoint
to
cbenoit-policy-caller-hardening
September 8, 2026 01:49
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 01:50
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Default-path migration, directory bootstrap, and probe races can violate availability and preservation guarantees.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens Windows policy persistence against filesystem races and interrupted writes.
Changes:
- Adds handle-relative transactional create/replace and crash recovery.
- Secures and validates default/custom policy directories.
- Expands filesystem security and concurrency tests.
File summaries
| File | Description |
|---|---|
policy_store/windows.rs |
Implements secure Windows storage primitives and recovery. |
policy_store/mod.rs |
Integrates transactional persistence into policy management. |
policy_security.rs |
Adds managed-policy ACL, identity, and ancestor checks. |
Review details
- Files reviewed: 2/3 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Benoît Cortier (CBenoit)
force-pushed
the
cbenoit-policy-storage-hardening
branch
from
September 8, 2026 04:31
7190ec5 to
cddee20
Compare
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 04:45
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Failed probes can self-trigger repeated reloads, and legacy installations lose write capability without an in-band migration path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/4 changed files
- Comments generated: 5
- Review effort level: Balanced
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 05:08
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Create transactions miss an ancestor-security check, and custom-path publication and authoritative state reporting have correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
crates/now-package-broker/src/policy_security.rs:309
- The retained hosting-directory handle for configured paths omits
FILE_TRAVERSE, but this exact handle is later passed asFILE_RENAME_INFO.RootDirectorybyatomic_createandatomic_replace. Windows requires traverse access on that root handle; meanwhile the capability probe uses a separate handle that does requestFILE_TRAVERSE, so a custom path can be reportedWritableand then every handle-relative publication fails. IncludeFILE_TRAVERSEin these retained handles.
crates/now-package-broker/src/policy_store/mod.rs:130 - This authoritative re-observation discards its current write capability and read-only reason. The success path later copies those fields from the pre-write observation, so an ACL change after the post-write check but before this read can be observed as read-only while the response still advertises
Writable. Carry the authoritative observation's capability fields into the persisted result and success snapshot.
- Files reviewed: 4/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 05:29
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Crash recovery can discard the only verified policy copy, while unsafe probes can misclassify state or leave storage persistently read-only.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 2
- Review effort level: Balanced
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 13:25
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Pre-marker crashes remain unrecoverable, and watcher gaps can temporarily leave stale policy active.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 3
- Review effort level: Balanced
Benoît Cortier (CBenoit)
force-pushed
the
cbenoit-policy-storage-hardening
branch
from
September 8, 2026 13:47
e5aba80 to
8cb1f4b
Compare
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 13:55
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Transaction security digests are architecture-dependent, which can break crash recovery across 32-bit and 64-bit builds.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Preserve external policy changes by binding writes to retained file and directory identities and publishing with handle-relative no-replace renames. Recover interrupted replacements from durable verified markers and create the default policy tree under retained, reparse-safe ancestors with strict ACL and filesystem capability checks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep unsafe path shapes distinct from unsupported file formats after replacing the path-based storage backend. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve legacy policy continuity while making managed-store selection sticky and observable across startup and watcher races. Harden default-directory bootstrap, capability probing, and unsupported-filesystem reporting without weakening retained-handle publication or recovery. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Thread the original configured path through hardened writes and use it for authoritative post-write observation so self-generated watcher events keep stable store tokens. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Deny write and delete sharing during ordinary policy observations so pre-existing mutable handles cannot alter content after ACL verification. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Clarify managed and legacy default paths, bound failed capability probes, suppress probe watcher churn, and preserve the installer-owned migration boundary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reopen probe leaves relative to the retained directory and compare file identities after a no-replace collision. This avoids rejecting equivalent Windows path representations while still detecting retargeted entries. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 17:15
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A dangling reparse leaf can be incorrectly reported as missing when directory verification fails.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Inspect policy leaves without following reparses when their containing directory cannot be verified. Existing or ambiguous entries remain invalid, while only genuine absence is reported missing and non-writable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 17:33
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Custom policy paths use a noncanonical watcher target, potentially delaying reloads until fallback polling.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/now-package-broker/src/policy_store/mod.rs:296
- Custom paths now watch the original configuration string instead of the canonical path captured by observation. This code explicitly accepts 8.3 parent aliases, but
windows_paths_equalcannot equate a short alias with the long path that a Windows notification may report, so an external policy change can be missed until the 30-second poll. Preserve the previous behavior by watching the snapshot's canonical path.
- Files reviewed: 4/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Watch the verified canonical custom policy path so alternate configured path representations do not delay reloads. Refresh complete watcher sets without gaps while preserving the original configured path for secure writes and authoritative re-observation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 18:01
View session
Keep only the live watcher handles during canonical path refresh. Replacement sets still register completely before assignment, so failed refreshes preserve the active monitoring set without unused path bookkeeping. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 18:19
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Managed-policy selection can be lost across restart, reactivating a stale legacy policy.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Create and verify durable protected evidence before managed default policy activation succeeds. Restarts then remain bound to managed storage after its final policy disappears, without treating an untouched empty managed directory as authoritative over legacy policy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 18:46
View session
Document the boundary between verified canonical paths used for storage and watching and the original configured path retained for authoritative re-observation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 19:03
View session
Use a versioned zero-length marker whose protected CREATE_NEW operation is already semantically complete. Existing valid markers remain authoritative, while invalid collisions are preserved and fail closed without staging or cleanup assumptions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 19:34
View session
Benoît Cortier (CBenoit)
marked this pull request as ready for review
September 8, 2026 19:49
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.
Hardens managed policy persistence so writes remain bound to the exact file and directory state that was observed.
Retains the policy target and every directory component from root to leaf, rejects reparse ancestors and leaves, and publishes with handle-relative no-replace renames so concurrent administrator changes are preserved. Durable marker and tombstone state provides deterministic crash recovery without discarding the only valid policy copy.
Creates the default
Devolutions\PackageBrokertree component by component with protected ACLs, permits only compatible create-only grants on the shared vendor directory, and verifies custom NTFS/ReFS storage with nondestructive capability probes.The three-file implementation contains 2,544 production and 1,097 test lines in
policy_store/windows.rs, 532 production and 293 test/support lines inpolicy_store/mod.rs, and 1,133 production and 790 test lines inpolicy_security.rs.Stacked on #1977 and #1975. Extracted from #1963. Predecessor fixes and overall stack readiness remain tracked separately.