test(windows): cover each quota repeat outcome at both catch seams (#219) - #338
Merged
Merged
Conversation
Protected run 35146928092 failed closed with repeat=persistent after an access denial in directory-enumeration-depth-3-plus. That label could not separate a second UnauthorizedAccessException from an exhausted traversal-wide entry budget (QuotaEntryBoundException) or a changed IOException, because both repeat paths in the supervisor classified every non-missing second exception with a bare catch. The frozen supervisor source already maps both sites through one helper, ClassifyPersistentQuotaRepeat, which reports persistent-<category> using the fixed first-failure categories (persistent-entry-bound, persistent-access-denied, persistent-io and its HRESULT variants, persistent-arithmetic-overflow, persistent-unexpected). The lock pins that source, so it is unchanged here. What was missing was coverage through the production seams and a current description of the labels: - Drive ReadQuotaOperation and ReadDirectorySnapshotOperation with one injected second failure at a time (second denial, entry-bound, generic and HRESULT-classified I/O, both missing types, unexpected) and require the matching label after exactly two calls, with the first category preserved and no private text. - Read a real two-entry directory through ReadBoundedDirectorySnapshot with a one-entry budget and require entry-bound on both attempts, with no injection, since MaximumQuotaEntries is one budget per traversal. - Document the persistent-<category> labels in docs/windows-quota-repeat-outcome.md alongside the retired transient label, and refresh the diagnostics test's byte and SHA-256 row. No resource limit, quota ceiling, retry count, identity, permission, or dependency changes. Refs #219 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Adds diagnostic coverage for Windows quota repeat classification across both production seams and documents the resulting labels.
Changes:
- Adds injected repeat-outcome matrix and real entry-budget regression coverage.
- Documents repeat categories and protected-run context.
- Refreshes conformance metadata for the updated test script.
| File | Description |
|---|---|
| scripts/windows-quota-diagnostics.test.ps1 | Updated as part of this pull request. |
| docs/windows-quota-repeat-outcome.md | Updated as part of this pull request. |
| docs/phase1-conformance.md | Updated as part of this pull request. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…rified status Address the two Copilot review threads on PR #338. The both-seam matrix omitted persistent-arithmetic-overflow and the classified I/O labels for file-not-found, path-not-found, lock-violation, name-too-long and invalid-directory, so a seam-specific wiring regression for those outcomes could not be caught; the earlier loop only calls ClassifyPersistentQuotaRepeat directly. Inject one exception per remaining normalized label: an OverflowException and plain IOException instances carrying each reviewed Win32 HRESULT. A plain IOException with the file-not-found HRESULT is not a FileNotFoundException, so it must reach the classifier rather than the missing catch; the matrix now proves that at both production seams. The status line in docs/windows-quota-repeat-outcome.md said the matrix and spent-budget check had not been executed. That is no longer true: the managed file passed on macOS (PowerShell 7.6.6, .NET 10) and the windows-supervisor-behavior job passed on windows-2025 in run 35561671518. State that, and keep what remains unproven separate: which cause fired in protected run 35146928092, which only a fresh protected run reporting a persistent-<category> label can show. Refresh the diagnostics test's byte and SHA-256 row. Refs #219 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.
What
Diagnostic coverage for the Windows resource-quota monitor's repeat classification, for #219.
The diagnosis posted on #219 (from OpenCoven/sdk#38) proposed splitting the bare
catchthat mapped every second failure torepeat=persistent. On currentmainthat split already exists: since2cb98c0(#326) bothReadDirectorySnapshotOperationandClassifyQuotaReadRepeatcall the sharedClassifyPersistentQuotaRepeat, which emitspersistent-entry-bound,persistent-access-denied,persistent-io[-<hresult class>],persistent-arithmetic-overflowandpersistent-unexpected. The production supervisor source is unchanged by this PR; the frozen blob pinned inphase1-conformance.lock.jsonstill matches.What was missing was proof. This PR adds it:
scripts/windows-quota-diagnostics.test.ps1: a matrix that drives both seams (entry-attributesanddirectory-enumeration-depth-3-plus) with one injected second failure at a time and asserts the exact repeat label, that the first category staysaccess-denied, that exactly two attempts were made, and that no private path text leaks. A no-injection case reads a real two-entry directory with a one-entry budget and assertsCategory=entry-bound,Repeat=persistent-entry-bound, the exact mechanism described on the issue (the entry budget is per traversal, not per directory).docs/windows-quota-repeat-outcome.md: documents run 35146928092, whypersistentalone could not identify the cause, and everypersistent-<category>label.docs/phase1-conformance.md: refreshed byte/SHA-256 row for the diagnostics test, whichsrc/client-v1-conformance-workflow.test.tsenforces.Verification
pwsh -NoLogo -NoProfile -NonInteractive -File scripts/windows-quota-diagnostics.test.ps1on macOS (PowerShell 7.6.6, .NET 10): exit 0, including the new lines "Second denial, exhausted entry budget, other I/O failures, and missing paths keep distinct repeat labels at both seams" and "A spent traversal entry budget reports entry-bound on the first attempt and its repeat".dotnet buildof the probe C# block: 0 warnings, 0 errors.vitest run src/client-v1-conformance-workflow.test.ts: 91 passed, 19 skipped.ci:fulllabel requested sowindows-supervisor-behaviorexercises the suite onwindows-2025.Not claimed
No Windows reproduction, no repair of the protected failure, no acceptance change. Which of the three causes fired in run 35146928092 is still unknown; the next protected run will say
persistent-entry-bound,persistent-access-deniedorpersistent-io-*instead ofpersistent. If the SDK validator fixturetests/fixtures/chat-debee-windows-supervisor.cs.brpredates2cb98c0, that rebinding is a separate follow-up on OpenCoven/sdk#38.🤖 Generated with Claude Code