From 5486a9c9bc89541f38e010767a2d78b99f9ad024 Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Sun, 11 Oct 2026 13:50:29 +0000 Subject: [PATCH] docs(memory-bank): record Decision 26, the lessons of the piped AppliesTo, and the acceptance - Decision 26: Add and Remove for access and audit declare no default parameter set, what that changes for a piped object, and what stays silent (a value that PowerShell cannot convert to a scope is ignored and the object is handled as one without AppliesTo). - Debugging insights: a piped property selects a parameter set only when the cmdlet has no default set whose mandatory parameters the object satisfies; PowerShell resets only the parameters that the previous piped object bound, so a field that the cmdlet writes itself reaches the next object; probe the route that the documentation shows and each form of a value, not the one that you tried; a sentence about what a test proves is a claim too. - Active context and progress: the state after the fix, three independent reviews and two checks, the acceptance of the final package in the labs, and the open items; the remaining-path inventory is that of 2112ee3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- .memory-bank/activeContext.md | 85 ++++++++++--------- .memory-bank/debugging-insights.md | 71 +++++++++++----- ...026-no-default-parameter-set-add-remove.md | 49 +++++++++++ .memory-bank/progress.md | 36 ++++---- .memory-bank/systemPatterns.md | 8 +- 5 files changed, 167 insertions(+), 82 deletions(-) create mode 100644 .memory-bank/decisions/0026-no-default-parameter-set-add-remove.md diff --git a/.memory-bank/activeContext.md b/.memory-bank/activeContext.md index 6f3359f..7a62b21 100644 --- a/.memory-bank/activeContext.md +++ b/.memory-bank/activeContext.md @@ -1,6 +1,6 @@ --- status: current -last-verified: 2026-10-10 +last-verified: 2026-10-11 owner: active-agent source: current task evidence --- @@ -9,20 +9,22 @@ source: current task evidence ## Current focus -State at 2026-10-11 00:55 UTC: Handoff 5 (the coverage unknowns) is done -locally on `ai/coverage-unknowns`, from `origin/master` `6696f68`; nothing is -pushed. The report is `Tests/Coverage/Quality-Gate-Unknowns-2026-10-10.md` -with nine CSV tables. rc7 (`fa0701b`) was measured at 3,198/3,649 sequence -points (87.64%); the live suite (first lab, three matrix cells) and the unit -suite on a domain member add only 9 sequence and 4 explicit branch points, all -of `fdd7a8b`. Two `tr-TR` defects are fixed test-first (`419cfb3`, an identity -compared with a name; `0028ccd`, `fbe0289`, `8a625c8`, `Get-ChildItem2 --Filter`, found by the independent review), 79 `It` blocks in four files are -added (final commit `61e936e`, built once at `8a625c8`: 1,459 cases per -configuration, 3,258/3,664 points, 88.92%; the matrix suite passes on Windows -Server 2019, 2022, 2025 and Windows 11 in 16 configurations), and a defect is -open (hard-link names under a root that is not the volume root). The -maintainer decides whether the fixes and the tests enter a new candidate. +State at 2026-10-11 13:50 UTC: Handoff 5 (the coverage unknowns) is done +locally on `ai/coverage-unknowns`, from `origin/master` `6696f68`; the +maintainer pushed the branch at `2b97422`, and the later commits are local. +The report is `Tests/Coverage/Quality-Gate-Unknowns-2026-10-10.md` with nine +CSV tables. rc7 (`fa0701b`) was measured at 3,198/3,649 sequence points. Three +defects are fixed test-first: two `tr-TR` comparisons (`419cfb3`; `0028ccd`, +`fbe0289`, `8a625c8`) and the piped `AppliesTo` that a path dropped (`93ed2c7`, +`9783d2a`: no default parameter set for Add and Remove of access and audit, +Decision 26); one defect is open (hard-link names under a root that is not the +volume root). 101 new `It` blocks; at `2112ee3` (product code built once at +`9783d2a`): 1,492 cases per configuration, 3,266/3,668 points. The final +package passed in the labs (first lab 245/0/1 per edition; the suite on the file +servers and the client, 16 configurations; the three cells 229/0/2 per +edition), every end state clean. Three independent reviews and two checks read +the work, with no Blocker. The maintainer decides whether the fixes and the +tests enter a new candidate. State at 2026-10-10 15:45 UTC: the maintainer integrated the release-gate stack into `master` (`6dd799f`, CI green): #116 (rc7, `8a6be9f`), #120 @@ -116,30 +118,31 @@ acceptance is the maintainer's. passed. A dry run of `Run-MatrixSequence.ps1 -Version 5.0.0-rc6` showed that the published-package path works. Labs at 07:50 UTC on 2026-10-10: no fixture or probe residue; `OSWin11E` shuts itself down an hour after its start. -- Handoff 5, the coverage unknowns, 2026-10-10 (report +- Handoff 5, the coverage unknowns, 2026-10-10 to 11 (report `Tests/Coverage/Quality-Gate-Unknowns-2026-10-10.md`; raw evidence in the session folder `816cf47d-2f80-4eba-8ac9-41702c49560b\files\qg-unknowns` with a hash manifest). rc7 measured at `fa0701b` (four frozen configurations, AltCover): 3,198/3,649 sequence points (87.64%), 1,090/1,168 explicit branch points; 232 unvisited methods, all classified. Live suite on - an instrumented copy (first lab and three cells, counts equal to the - acceptance) and unit suite on `OSFile25`: together +9 sequence and +4 - explicit branch points (the code of `fdd7a8b`); the cells add nothing to - the first lab; no test or probe reaches the audit ownership retry or the - audit read denial. The new tests add 45 points (getters of pipeline - parameters; the paths report was wrong that no caller invokes a getter); - 7 of 64 parameter sets closed, 51 partly, 1 open, 4 left to the maintainer. - Cultures `de-DE` and `tr-TR`: two defects, fixed (`419cfb3`; `0028ccd`, - `fbe0289`, `8a625c8`); the second was found by the one independent review - (`code-review`: four Major findings, all resolved; the later delta had no - second review). Final measurement - `61e936e` (build `8a625c8`): 1,459 cases per configuration, 3,258/3,664 - points, the same 187 unvisited methods; first lab 245/0/1 per edition on - that build; the 22 files of the matrix suite 1,156/0 in 16 configurations - on `OSFile19/22/25` and `OSWin11`; both labs clean at 00:43Z. The tool's - branch summary percentage is not read (it adds a point per method); the - explicit branch points are 1,090/1,168. Server Core, ARM64, a localized - system, and non-Windows servers are out of scope (Decision 25). + an instrumented copy (first lab and three cells) and unit suite on + `OSFile25`: together +9 sequence and +4 explicit branch points (the code of + `fdd7a8b`); no test or probe reaches the audit ownership retry or the audit + read denial. 101 new `It` blocks; 7 of 64 parameter sets closed, 51 partly, + 1 open, 4 left to the maintainer. Cultures `de-DE` and `tr-TR`: two defects, + fixed. The piped + `AppliesTo` (Decision 26, debugging insights): fixed in `93ed2c7`; the + independent reviews then found a carry-over of the scope between piped + objects (`9783d2a`; rc7 had it for descriptor objects) and two false + sentences of the documents about empty values (`7bc7925`, `2112ee3`). Final + commit `2112ee3`, product code of `9783d2a` (its help file differs from the + package that the labs accepted): 1,492 cases per configuration, 3,266/3,668 + points, 183 unvisited methods, all classified. The labs accepted the package + (`ap7`, `ap8`): first lab 245/0/1 per edition; the 22 files on + `OSFile19/22/25` and `OSWin11` 1,189/0 in 16 configurations; the three cells + 229/0/2 per edition; all end states clean at 13:45Z. The first start of the + cells did nothing: a `powershell.exe` that PowerShell 7 starts inherits its + `PSModulePath` (lab README). Server Core, ARM64, a localized system, and + non-Windows servers are out of scope (Decision 25). - Raw evidence is outside git: the session folder `4b12e2f4-d4c7-4a5d-883a-ddb7421c4848\files` (`qg-paths`, the matrix runs, the night log) and @@ -179,11 +182,11 @@ acceptance is the maintainer's. acceptance). The help could also say that a local standard user who asks about a domain account still gets "Access is denied". 6. Handoff 5 is done (Evidence). The maintainer decides whether the fixes - `419cfb3`, `0028ccd`, `fbe0289`, `8a625c8` and the tests enter a new - candidate (rc8), which needs the matrix cells and the first lab on its - bytes, and he decides the open items of the report: hard-link names under - another root (a defect with a reproduction), a piped `AppliesTo` property - that a path drops (cause: the default set; the report has a model of the - fix), 12 getters of pipeline parameters, the 51 partly closed sets, and the - audit paths. + (`419cfb3`, `0028ccd`, `fbe0289`, `8a625c8`, `93ed2c7`, `9783d2a`) and the + tests enter a new candidate (rc8), which needs the matrix cells and the + first lab on its own bytes, and he decides the open items of the report: + hard-link names under another root (a defect with a reproduction), the + silent default scope for a piped `AppliesTo` that PowerShell cannot convert + (a design change), 8 getters of pipeline parameters, the 51 partly closed + sets, and the audit paths. 7. Do not release stable 5.0.0 or equate a percentage with gate closure. diff --git a/.memory-bank/debugging-insights.md b/.memory-bank/debugging-insights.md index a8467ed..eab0c73 100644 --- a/.memory-bank/debugging-insights.md +++ b/.memory-bank/debugging-insights.md @@ -14,12 +14,13 @@ fails without it (Decision 16). ## Culture-sensitive comparison (`tr-TR`) - With `tr-TR`, `I` and `i` are different letters, so a comparison that takes - the culture of the thread drops or misses items: `string.Equals` without a - comparer, a case-insensitive `Regex`, a `WildcardPattern` without - `CultureInvariant`. Handoff 5 found two defects of this class: - `IdentityReference2.Equals(object)` called with a name (now - `OrdinalIgnoreCase`) and `Get-ChildItem2 -Filter` (`index*` did not list - `Index.txt`). + the culture of the thread drops or misses items: `ToLower` or `ToUpper`, + `Compare`, `StartsWith`, `EndsWith`, or `IndexOf(string)` without a + `StringComparison`, a case-insensitive `Regex`, a `WildcardPattern` without + `CultureInvariant` (`string.Equals(a, b)` and `==` are ordinal and safe). + Handoff 5 found two defects of this class: `IdentityReference2.Equals(object)` + called with a name, which lowercased both sides (now `OrdinalIgnoreCase`), + and `Get-ChildItem2 -Filter` (`index*` did not list `Index.txt`). - A scan of the C# is not enough: AlphaFS 2.2 builds a culture-sensitive regex from a search pattern. `Get-ChildItem2` therefore enumerates under `InvariantCultureScope` (around the creation of the enumerator and each @@ -29,9 +30,12 @@ fails without it (Decision 16). only the .NET method call failed. Probe a claim before it enters the changelog or a report (the first wording named `-eq` and was wrong). - A test sets the culture of the thread, restores it in `finally`, names the - rows that differ (`index*`, `*.INI`), keeps a control (`INDEX*`), and is - proved red on the previous build and green on the new one in all four - configurations (`Tests/ItemCmdlets.Tests.ps1`, `Tests/ObjectApis.Tests.ps1`). + rows that differ (`index*`, `*.INI`), keeps a control (`INDEX*`), reads the + culture in the next pipeline stage for each item and once after the call (a + cmdlet that does not restore it, or writes an item inside the scope, fails + both: two mutations proved it), and is proved red on the previous build and + green on the new one in all four configurations + (`Tests/ItemCmdlets.Tests.ps1`, `Tests/ObjectApis.Tests.ps1`). ## A test oracle that is wrong on a domain member @@ -45,19 +49,44 @@ fails without it (Decision 16). ## A piped property and the parameter set - An object with a property `AppliesTo`, piped into `Add-` or - `Remove-NTFSAccess` or `-NTFSAudit` with a path, binds the default - `PathComplex` set and drops the property; with a descriptor it binds - `SDSimple`. A test that means the Simple set names `-AppliesTo` and asserts - the scope it gets (a probe that tells the sets uses an entry of the default - scope: removing `ThisFolderOnly` from it leaves the `InheritOnly` part). -- The cause is `DefaultParameterSetName = "PathComplex"`: when a piped object + `Remove-NTFSAccess` or `-NTFSAudit` with a path, bound the default + `PathComplex` set and lost the property. Fixed in `93ed2c7`: the four cmdlets + declare no default set, so the property selects `PathSimple`, an object with + `AppliesTo` and `InheritanceFlags` fails with `AmbiguousParameterSet`, and the + tests pipe an object of each shape (red on the previous build, green on the + new one). A probe that tells the sets uses an entry of the default scope: + removing `ThisFolderOnly` from it leaves the `InheritOnly` part. +- PowerShell restores only the parameters that the previous piped object + bound. A field that the cmdlet writes itself stays for the next object: the + scope that `ProcessRecord` converted from `AppliesTo` into the field behind + `-InheritanceFlags` went to the next object without a scope. `93ed2c7` + shipped that, the third review found it (rc7 had it for piped + `-SecurityDescriptor` objects), and `9783d2a` keeps the flags of a record in + locals. A pipeline test pipes two or more objects of different shape in one + call (`Objects with different scopes in one pipeline`, both ParameterSets + files); one object per call cannot see it. +- Probe the route that the documentation shows. `Import-Csv` of a file gives an + empty string for a blank cell, `ConvertFrom-Csv` of strings gives `$null` for + a trailing blank cell, and `Import-Csv` gives `$null` for a column that a row + lacks. The binder treats them differently: `$null` fails to bind, and a value + that it cannot convert to the enum (an empty string, a typo) is ignored, so + the object is handled as one without the property. The first statement and + the first tests used the convenient route, and the reviewer found the false + claim; the corrected claim was then too narrow once more (a value that + converts binds, whatever its form; a flag next to an unusable value wins). + Write the claim from a probe of each form, not from the one that you tried. +- A sentence that says what a test proves is a claim too: a title that says + "and change nothing" needs an assertion that can see the change (the audit + cmdlets write the SACL, not the DACL), and "cannot go red" needs a run. +- The cause was `DefaultParameterSetName = "PathComplex"`: when a piped object satisfies the mandatory parameters of the default set, PowerShell settles on it and never tries a parameter that belongs to another set. A stand-alone model with the four sets and no default binds `PathSimple` for an object with `AppliesTo`, `PathComplex` for one without, and an error for one with - `AppliesTo` and `InheritanceFlags`; named calls resolve as before - (`probes\p-parameterset-binding*.ps1` in the Handoff 5 evidence). The default - dates from 4.2.6, when `AppliesTo` was optional; `4b1abc9` made it mandatory - in the Simple sets, so the default no longer decides a named call. Build a - model of the parameter sets before you change an attribute, and test the real - cmdlet afterwards. + `AppliesTo` and `InheritanceFlags` (`probes\p-parameterset-binding*.ps1` in + the Handoff 5 evidence; the model binds `PathComplex` for a piped descriptor + where the real cmdlets bind `SDSimple`); the real cmdlets agreed for paths + (`probes\p-proto-appliesto.ps1`). The default dated from 4.2.6, when + `AppliesTo` was optional; `4b1abc9` made it mandatory in the Simple sets, so + the default no longer decided a named call. Build a model of the parameter + sets before you change an attribute, and test the real cmdlet afterwards. diff --git a/.memory-bank/decisions/0026-no-default-parameter-set-add-remove.md b/.memory-bank/decisions/0026-no-default-parameter-set-add-remove.md new file mode 100644 index 0000000..839d789 --- /dev/null +++ b/.memory-bank/decisions/0026-no-default-parameter-set-add-remove.md @@ -0,0 +1,49 @@ +--- +status: accepted +date: 2026-10-11 +last-verified: 2026-10-11 +owner: shared +source: maintainer decision of 2026-10-11 +--- + +# Decision 26: Add and Remove for access and audit declare no default parameter set + +- Choice: `Add-NTFSAccess`, `Remove-NTFSAccess`, `Add-NTFSAudit`, and + `Remove-NTFSAudit` declare no `DefaultParameterSetName` (maintainer, + 2026-10-11; `93ed2c7`). An object piped with a path binds `PathSimple` when + it has `AppliesTo`, and `PathComplex` when it has `InheritanceFlags`, + `PropagationFlags`, or neither (the default scope). An object that has + `AppliesTo` and one of the flags fails with `AmbiguousParameterSet`, like the + same named parameters. +- Rationale: With `PathComplex` as the default, an object that satisfied its + mandatory parameters made the binder settle on it, and a piped `AppliesTo` + was dropped without a message (a silent change of the scope). `4b1abc9` had + made `AppliesTo` mandatory in the Simple sets, so the default no longer + decided any named call. +- Consequence: a script that piped objects with `AppliesTo` and the flags + together got the flags before and gets an error now; so does an object that + has both `Path` and `SecurityDescriptor`, where the path won. An + `AppliesTo` that is `$null` fails to bind for that object (a CSV row with + fewer fields than its header has `$null` there); a call with neither + `-Path` nor `-SecurityDescriptor` fails with `AmbiguousParameterSet` instead + of a missing-parameter error (CHANGELOG, Fixed). A value that PowerShell + cannot convert to a scope (the blank cell that `Import-Csv` reads, a typo) + is ignored silently, and the object is handled as one without `AppliesTo`: + its own flags, or the default scope when it has none. The binder swallows a + value that it cannot convert, so failing loudly needs a design change + (report, open item 3). Named calls, + `-SecurityDescriptor` calls without `AppliesTo`, and + `Get-NTFSAccess | Add-NTFSAccess -Path` do not change. The syntax lists + `PathSimple` first, because PowerShell orders the sets by declaration. +- Follow-up: the third independent review found that `ProcessRecord` wrote + the scope converted from `AppliesTo` into the fields behind the flag + parameters, which PowerShell does not reset between piped objects. The flags + of a record are locals since `9783d2a`; rc7 had the defect for piped + `-SecurityDescriptor` objects. The reviewer's checks found that "an empty + `AppliesTo` fails to bind" was true for `$null` only (`7bc7925`), and that + the sentence about unconvertible values was too narrow (`2112ee3`). +- Evidence: report `Tests/Coverage/Quality-Gate-Unknowns-2026-10-10.md` + (decision 23; the section on the fix of the piped `AppliesTo`), the tests in + `Tests/ParameterSets.Access.Tests.ps1` and `Tests/ParameterSets.Audit.Tests.ps1` + (red before, green after, in four configurations), and + `.memory-bank/debugging-insights.md`. diff --git a/.memory-bank/progress.md b/.memory-bank/progress.md index 489036c..8a170c6 100644 --- a/.memory-bank/progress.md +++ b/.memory-bank/progress.md @@ -1,6 +1,6 @@ --- status: current -last-verified: 2026-10-10 +last-verified: 2026-10-11 owner: active-agent source: repository and validation evidence --- @@ -99,17 +99,21 @@ After 5.0.0, archive in favor of WindowsAccessControl (Decision 18). 9 245 / 0 / 1 per edition; every end state CLEAN. The counts equal those of the final local candidate. Record: `Tests/Lab/Acceptance-2026-10-10-published-rc7.md`. -- 2026-10-10: Handoff 5, the coverage unknowns (local, `ai/coverage-unknowns`, - nothing pushed). rc7 measured at `fa0701b`: 3,198/3,649 sequence points; - the live suite and the suite on a domain member add 9 sequence and 4 - explicit branch points (`fdd7a8b`); two `tr-TR` defects fixed test-first - (`419cfb3`; `0028ccd`, `fbe0289`, `8a625c8`, the second found by the - independent review); 79 `It` blocks added (final `61e936e`, built at - `8a625c8`: 1,459 cases per configuration, 3,258/3,664 points); the getter - rule of the paths report corrected; the first lab accepted on the fixed - builds and the matrix suite passing on Windows Server 2019, 2022, 2025 and - Windows 11 (1,156 cases, 16 configurations). Open: hard-link names under - another root, a piped `AppliesTo` for a path. Report: +- 2026-10-10 to 2026-10-11: Handoff 5, the coverage unknowns (local, + `ai/coverage-unknowns`; the maintainer pushed `2b97422`, later commits are + local). rc7 measured at `fa0701b`: 3,198/3,649 sequence points; the live + suite and the suite on a domain member add 9 sequence and 4 explicit branch + points (`fdd7a8b`); two `tr-TR` defects fixed test-first (`419cfb3`; + `0028ccd`, `fbe0289`, `8a625c8`); 101 `It` blocks added. The piped + `AppliesTo` that a path dropped is fixed (`93ed2c7`, `9783d2a`, Decision 26) + after three independent reviews and two checks: a carry-over of the scope + between piped objects, and false sentences about empty values (`7bc7925`, + `2112ee3`). Final `2112ee3`, product code built once at `9783d2a`: 1,492 + cases per configuration, 3,266/3,668 points. The labs accepted the package: + first lab 245/0/1 per edition, the suite 1,189/0 in 16 configurations on + four machines, three cells 229/0/2 per edition, every end state clean. Open: + hard-link names under another root, the silent default scope of an + unconvertible piped `AppliesTo`. Report: `Tests/Coverage/Quality-Gate-Unknowns-2026-10-10.md`. ## Stable capabilities @@ -146,10 +150,10 @@ After 5.0.0, archive in favor of WindowsAccessControl (Decision 18). 6. Optional maintainer cleanup: obsolete AppVeyor/Read the Docs access, wiki editing restrictions, `test/transfer`, and old lab checkpoints when no longer needed. No remote changes or snapshot restores here. -7. Remaining-path inventory at the final frozen commit of Handoff 5 - (`61e936e`): 406 unvisited sequence points in 187 methods, all classified - (`Tests/Coverage`): 167 explained from source with evidence, 20 open (8: - `FileSecurity` conversions, `RemoveAll` account filters; 12 getters of +7. Remaining-path inventory at the final measured commit of Handoff 5 + (`2112ee3`): 402 unvisited sequence points in 183 methods, all classified + (`Tests/Coverage`): 167 explained from source with evidence, 16 open (8: + `FileSecurity` conversions, `RemoveAll` account filters; 8 getters of pipeline parameters that no test binds by property name). Other open decisions: lazy path overloads, abandoned `PrivilegeEnabler`, dot patterns of `Get-ChildItem2 -Filter`, 17 diff --git a/.memory-bank/systemPatterns.md b/.memory-bank/systemPatterns.md index d3a6107..fdfbfd2 100644 --- a/.memory-bank/systemPatterns.md +++ b/.memory-bank/systemPatterns.md @@ -50,6 +50,7 @@ Read only task-relevant records; the index controls routing. | 23 | [Non-Windows file servers before 5.0.0, #34 (proposed)](decisions/0023-non-windows-file-servers.md) | | 24 | [The operating-system matrix lab (proposed)](decisions/0024-os-matrix-lab.md) | | 25 | [Four environments are out of scope for the tests](decisions/0025-out-of-scope-environments.md) | +| 26 | [No default parameter set for Add and Remove (access, audit)](decisions/0026-no-default-parameter-set-add-remove.md) | ## Patterns @@ -93,10 +94,9 @@ Read only task-relevant records; the index controls routing. Get/SetFileSecurity preserves stored DACLs; rights oracles use S4U tokens. - A suite that is green on the host and CI misses defects that need a domain member or another token (the matrix found three): also run a domain member, - other builds, and a basic user. A failure that follows cell order, not - version, points to stale account state (Decision 24): alternate the cells. - Oracles differ there too: `Get-Acl -Audit` reads an item without audit - entries as SACL-protected, so a test reads the SACL alone (.NET). + other builds, and a basic user; a failure that follows cell order points to + stale account state (Decision 24). Oracles differ there too: `Get-Acl -Audit` + reads an item without audit entries as SACL-protected; read the SACL alone. ### CI, publication, and integration