Skip to content

docs: strip grill changelogs + cast refs from docs/plans (#Phase2 WI-3) #485

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
primetimetank21 merged 1 commit into from
Jun 13, 2026
+12 −244
Merged

docs: strip grill changelogs + cast refs from docs/plans (#Phase2 WI-3) #485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 1 addition & 39 deletions docs/plans/441-profile-path.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,44 +8,6 @@ updated: 2026-06-13

# Plan: #441 -- Profile path fix on OneDrive/KFM systems

## v5 Changes (revision 5)

| # | Hole | Reviewer | Sev | Patch |
|---|------|---------|-----|-------|
| H1 | `Set-Content` in foreach body missing `-Encoding ASCII` | F-1 | HIGH | Added `-Encoding ASCII` to orphan-strip `Set-Content` (matches production line 28) |
| H2 | GG-7 exit-1 leaves stale `$LASTEXITCODE`; contaminates success-path tests | F-3 | MEDIUM | Section 5 header: each test resets `$global:LASTEXITCODE = 0` before mock redefinition |
| H3 | `TestDrive` in GG-4 contradicts Section 3 D2 (Pester rejected as scope creep) | F-2 + C-2 | MEDIUM | Replaced `TestDrive` with `Join-Path $env:TEMP "gg-test-441-$(New-Guid)"` temp-path language in GG-4; temp-dir cleanup sentence added to Section 5 header |
| H4 | GG-7 exe unspecified; false green on PS5.1-only runner | C-1 | MEDIUM | GG-7 row: `$HostExe = 'powershell'` (guaranteed on Windows); note that `'pwsh'` would mask the not-installed early-exit |
| H5 | `$ps51Fallback`/`$ps7Fallback` undefined inside `Write-PowerShellProfile` under `Set-StrictMode -Version Latest` | A-1 | MEDIUM | Two `$local:` definitions added at top of `Write-PowerShellProfile` in Section 4 (mirror production lines 17-19) |

## v5.2 Changes (revision 5.2)

| # | Hole | Reviewer | Sev | Patch |
|---|------|---------|-----|-------|
| JN-1 | `$local:ps51Fallback`/`$local:ps7Fallback` inside `Write-PowerShellProfile` shadow calling scope; test-scope assignment inoperable; GG-1/GG-4/GG-5 would write to real `$HOME` paths | JN-1 | MEDIUM | Parameterized `Write-PowerShellProfile` with `-Ps51Fallback`/`-Ps7Fallback` (defaults = production lines 17-18); tests pass temp paths as named parameters; Section 3 v5.2-D1 added; Section 5 GG-1/GG-4/GG-5 updated |
| JN-2 | C-2/C-3 PS7+ skip uses `Write-Host`; increments pass counter instead of skip counter on PS7+ CI | JN-2 / NF-3v4 | LOW | `Write-Host 'SKIP C-2: ...'` -> `Write-Warning '[SKIPPED] C-2: ...'` in Section 3 v3-D4; warning stream is visually distinct in PS output; no Pester dependency (D2 preserved) |

## v5.1 Changes (revision 5.1)

- F-4: Orphan-strip regex matches production line 27 (`\r?\n` prefix added; `.+?` -> `.*?`)
- F-5: `$local:beginMarker`/`$local:endMarker` defined in `Write-PowerShellProfile` (mirrors production lines 12-13)

---

## v4 Changes (revision 4)

| # | Griller | Sev | Patch |
|---|---------|-----|-------|
| P1 | Pluto | BLOCKING | Section 4 foreach loop body filled -- explicit strip regex + log; no stub comment |
| P2 | Pluto | BLOCKING | Section 4 algorithm wrapped explicitly in `Write-PowerShellProfile`; comment explains dot-source safety |
| P3 | Chip | HIGH | GG-7 mock calls `& $env:ComSpec /c "exit 1"` -- propagates `$LASTEXITCODE = 1` via native-command global semantics |
| P4 | Chip | SS-2 | Section 3 v3-D4 guard: `skip` replaced with `if/Write-Host/return`; guard + `$PROFILE = $path` moved INSIDE Test-Scenario body |
| P5 | Chip | MEDIUM | Section 5 header: mock isolation pattern documented -- mock redefined before each GG test; Test-Scenario child-scope model stated |
| P6 | Chip | MEDIUM | GG-4 row: both mock calls return same `$oneDrivePath`; dedup -> 1 entry; both legacy paths confirmed orphaned |
| P7 | Doc | cosmetic | Section 3 v3-D4: `$PROFILE` is conceptually (not technically) read-only, per MS Learn |

---

## 1. Problem

`profile.ps1` hardcodes `$HOME\Documents\...` as the profile path. On OneDrive KFM systems, PowerShell's actual `$PROFILE` resolves to `OneDrive\Documents\...`. The dev-setup block is written to a file that is never sourced; aliases silently fail to appear in new terminals.
Expand Down Expand Up @@ -98,7 +60,7 @@ Decision: Check `$LASTEXITCODE -ne 0` after `Invoke-HostQuery`. `& $Exe` exits n

Decision: GG-4 seeds BOTH legacy paths simultaneously and asserts BOTH stripped. One-path test cannot catch a loop-break bug after the first match.

**v3-D4. C-2/C-3 guarded in PS7+** *(updated v4 -- Doc/Chip)*
**v3-D4. C-2/C-3 guarded in PS7+**

Decision: Guard C-2 and C-3 with proper skip. The `$PROFILE` automatic variable is conceptually read-only in PowerShell (per Microsoft Learn); assigning to it is unsupported and may cause issues in test contexts. Full refactor deferred; behavior these tests cover is superseded by this PR. Concrete mechanism: move the `$PROFILE = $path` assignment INSIDE the Test-Scenario body (it was erroneously outside as setup code), then open with `if ($PSVersionTable.PSVersion.Major -ge 7) { Write-Warning '[SKIPPED] C-2: PS7+ -- $PROFILE conceptually read-only; covered by GG tests'; return }`. This ensures the assignment never fires on PS7+ and the skip reason is logged.

Expand Down
37 changes: 6 additions & 31 deletions docs/plans/451-pwsh-parity-gaps.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ Test count increases from 6 (T1-T6) to 9 (T1-T7, T_C, T_D), achieving parity wit

---

## CI Integration (v2 addition -- resolves Mickey BLOCKING-1/BLOCKING-2, Goofy Finding-1)
## CI Integration

The `validate-ps51` job (`.github/workflows/validate.yml` lines 287-369) does NOT currently run
`test_sprint_end_labels_pwsh.ps1`. The `validate-powershell` job (line 282-283) runs only the
Expand Down Expand Up @@ -79,7 +79,7 @@ is a prerequisite to close the CI gap and is explicitly in-scope for this issue.
- T_C / T_D: No setup, direct Invoke-ScriptRun on bare script
- T7: Minimal test state via New-TestEnv (empty issues/prs, copy launcher logic)

**T7 detail (v2 -- replaces vague "assert shebang valid", resolves Mickey MAJOR-2):**
**T7 detail:**
- Read launcher file via `[System.IO.File]::ReadAllBytes()` (binary, no encoding transform)
- Assert no 0x0D (CR) bytes present (primary regression invariant)
- Assert file starts with bytes 0x23 0x21 (the ASCII codes for `#!`) -- this is the shebang
Expand Down Expand Up @@ -117,7 +117,7 @@ if ($bytes[0] -ne 0x23 -or $bytes[1] -ne 0x21) {
**T_C / T_D (LOW):** Pure argument parsing, no side effects, no fixtures.

- Risk: Error message wording is coupled to implementation strings in `scripts/sprint-end-labels.ps1`
and `scripts/sprint-end-labels.sh`. (v2 decision -- resolves Mickey MAJOR-1 / Goofy T_C/T_D note)
and `scripts/sprint-end-labels.sh`.
- **Decision:** Use substring/regex match assertions (not full-string equality). T_C asserts
exit code != 0 only (mirrors bash Test C which checks exit code = 2, no message assertion).
T_D asserts exit code != 0 AND output contains `"release:shipped-"` (mirrors bash Test D).
Expand All @@ -131,7 +131,7 @@ if ($bytes[0] -ne 0x23 -or $bytes[1] -ne 0x21) {
**T7 (MEDIUM):** Byte-level assertion requires precise encoding handling.
- Risk: `[System.IO.File]::ReadAllBytes()` MUST be used; `Get-Content` applies encoding and
may hide CR bytes
- Mitigation: Use established ASCII safety pattern from chip/history.md
- Mitigation: Use the established ASCII safety pattern
- Dependencies: None new (reuses New-TestEnv)

---
Expand All @@ -149,7 +149,7 @@ preventing silent re-introduction of the bug.
1. **PS 5.1 coverage:** RESOLVED. validate-ps51 does NOT currently run this test file. Plan
includes a 2-step YAML addition to close the gap (see CI Integration section above).

2. **Launcher byte determinism:** RESOLVED (by Goofy). Launcher content is session-deterministic
2. **Launcher byte determinism:** RESOLVED. Launcher content is session-deterministic
(same $PowerShellPath within a run) but not identical across independent CI runs. T7 is a
regression test, not an idempotency test -- this is acceptable. See T7 detail above.

Expand All @@ -175,7 +175,7 @@ preventing silent re-introduction of the bug.

---

## Out-of-Scope Tracked Item (v3 addition -- resolves Goofy Round 2 caveat)
## Out-of-Scope Tracked Item

**`$IsWindows` PS 5.1 hazard in `New-TestEnv` (line 320 of test file):**

Expand All @@ -201,28 +201,3 @@ Implementer does NOT need to fix this during #451 -- it is tracked and will be a
in #461.

---

## Revision History

- **v1 (2026-05-28T02:56:01-04:00):** Initial plan committed to `.squad/decisions/451-vertical-slice.md`.
Three parity gaps identified (T_C, T_D, T7). Open questions deferred to grill.
- **v2 (2026-05-27T23:12:03-04:00):** Addressed grill panel findings (Mickey/Goofy/Jiminy).
- BLOCKING (Mickey/Goofy): Added CI Integration section; test wired into validate-ps51 job
(YAML step at line 369+); TODO comment at line 285 marked for removal.
- MAJOR (Mickey #4): Replaced vague "assert shebang valid" with precise two-assertion spec:
no 0x0D bytes (CR regression) + file starts with 0x23 0x21 (#! shebang bytes); both
assertions required and rationale documented.
- MAJOR (Mickey #3): T_C/T_D assertion strategy defined as substring/regex match; T_C is
exit-code-only; T_D uses "release:shipped-" substring; error-message contract documented
in Risk Assessment and Done Criteria.
- MEDIUM (Jiminy): Plan moved from `.squad/decisions/451-vertical-slice.md` to
`docs/plans/451-pwsh-parity-gaps.md` per PR #441 precedent.
- **v3 (2026-05-27T23:47:00-04:00):** Addressed Round 2 grill panel (Mickey/Goofy/Jiminy).
- Mickey APPROVE: All four R1 findings confirmed resolved. Done Criteria at lines 174/172
already cover both Mickey implementation-phase notes (error-message contract in PR
description; TODO removal). No plan text changes needed.
- Goofy APPROVE-WITH-MINOR-CAVEATS: `$IsWindows` PS 5.1 hazard at New-TestEnv line 320
noted. Decision: out of scope for #451 (pre-existing code, not introduced by this slice).
Filed follow-up issue #461. See "Out-of-Scope Tracked Item" section above.
- Jiminy CLEAN: Commit trailer formatting noted. v3 commit uses blank-line-separated
Co-authored-by trailer per git interpret-trailers convention.
Loading
Loading