fix(windows): account private directories as isolated identity - #221
Conversation
|
Heads-up: #223 landed on main as d20045f and advanced the frozen harness pin to |
There was a problem hiding this comment.
🟡 Changes recommended
A moderate disposal-admission issue remains in scripts/windows-job-supervisor.cs (2 votes).
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Repairs Windows quota accounting for isolated owner-only directories using validated isolated-user tokens, AppData security initialization, and expanded lifetime/regression coverage.
Changes:
- Adds isolated-token quota scans and strict path handling.
- Adds AppData initialization and native reader/lifetime regressions.
- Refreshes workflow bindings, lock metadata, and documentation.
File summaries
| File | Description |
|---|---|
src/phase1-conformance-lock.test.ts |
Updates frozen authority expectations. |
src/client-v1-conformance-workflow.test.ts |
Updates workflow assertions and metadata coverage. |
scripts/windows-quota-lifetime.test.ps1 |
Tests deferred monitor cleanup. |
scripts/windows-quota-isolated-reader.test.ps1 |
Adds native reader and boundary regressions. |
scripts/windows-job-supervisor.test.ps1 |
Runs Windows quota and lifetime regressions. |
scripts/windows-job-supervisor.cs |
Implements isolated quota reads and lifetime handling. Moderate finding (2 votes): mark disposal before quarantine to prevent new reads during disposal. |
scripts/windows-identity-cleanup-diagnostics.test.ps1 |
Initializes cleanup diagnostics for lifetime state. |
phase1-conformance.lock.json |
Pins the revised harness source and hashes. |
docs/windows-quota-reproduction.md |
Documents reader behavior and regressions. |
docs/roadmap.md |
Updates Windows quota repair status. |
docs/phase1-conformance.md |
Records repair, validation, and authority metadata. |
.github/workflows/client-v1-conformance.yml |
Embeds the updated supervisor implementation. |
Review details
- Files reviewed: 12/12 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.
| SafeAccessTokenHandle retiredQuotaToken; | ||
| lock (quotaTokenSync) | ||
| { | ||
| disposed = true; | ||
| retiredQuotaToken = quotaToken; | ||
| quotaToken = null; |
Owner-only producer directories require quota reads under the isolated identity, while ancestor metadata above its root may be unreadable. Native run 34627213499 confirmed that boundary: ancestor index 3 denied attributes, but direct target reads returned one 1,024-byte file. A subsequent root-level control isolated a separate initialization omission: only
profile/AppDatadenied enumeration; its parent, children and other fixed directories were readable (run 34632027669).This validates fixed ancestors as the supervisor, then expands and measures only patterns constrained to the isolated root under the validated isolated-user token. Outside-root and ambiguous patterns fail closed. Descendant denial is never retried as the supervisor. The omitted AppData directory now enters the existing security initialization and validation loop with the same trustees and masks as its parent and children.
Each synchronous read retains a noninheritable token duplicate through completion, including after account disablement. Identity disposal rejects new reads. A monitor surviving the existing bounded wait retains its state until completion.
Main's #214 and #223 changes are integrated without dropping either repair. Native regressions cover root-equality, descendant/wildcard accounting, outside/ambiguous patterns, unreadable private descendants, actual terminal production, post-disable reads, identity restoration and reads held across disposal. The integrated native result remains required.
Frozen source:
eda879fa0da04e76289fa977e3e853d9d8696df7, tree4f0386723bc7742f4ee69beba3b38a454987a018. Binding head:d3007b0b313378f2018e7ecd3cc3b3d2a957c010. Merge with an actual merge commit to retain frozen-source ancestry.Validation: 794 normal tests passed (63 skips), and 92 heavy lock tests passed (1 skip). Typecheck, lint, PowerShell/C# compilation, quota/lifetime/cleanup regressions and metadata checks passed. All 25 authority files match the frozen tree. Independent reviews found no blockers in the reader boundary, initialization correction or integrated cleanup delta. Native run 34636950444 passed the reader, root/byte, post-disable/disposal, private-ACL and integrated cleanup regressions. A later existing directory-quota failure case failed at isolated-process termination during quarantine; the original log omitted the Win32 error code. Test-only head
5f4707b39de9f6096e7577ff659dc288cf22d9adadds that bounded code. Run 34638791505 is queued; keep draft until full native CI passes. No termination checks were relaxed.Advances #219. SDK publication must bind the actual verified Chat merge, followed by both validator-scope rotations and fresh protected validation. Protected acceptance remains outstanding. Chat and active worktrees are preserved.