Repository navigation
test: harden Windows persistence teardown - #33
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17526cf0ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $initializations=New-CcodInstallDirectory -Transaction $FileTransaction -Parent $state -Leaf 'install-initializations' -CreateIfMissing | ||
| $baseline=New-CcodInstallDirectory -Transaction $FileTransaction -Parent $initializations -Leaf $RuntimeId -CreateIfMissing | ||
| $stateRoot=Join-Path $InstallRoot 'state' | ||
| $values=Initialize-CcodState -StateRoot $stateRoot -NodeCandidates $NodeCandidates -CandidateCompatibleOptIn $CandidateCompatibleOptIn |
There was a problem hiding this comment.
During an ordinary upgrade or a fresh-install race, this call re-enters Initialize-CcodState using only the pathname even though the surrounding function has already acquired a pinned $state capability. Initialize-CcodState then follows state\lifecycle by pathname while creating lifecycle\receipts and reading the lifecycle request, so a preexisting or concurrently created junction at that child can redirect initialization and subsequent state operations outside the install root; the create-only records written to $baseline do not protect the operational files. Pass the transaction into initialization and open/write the operational state hierarchy through its pinned directory capabilities.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
Two critical and five moderate findings remain unresolved, and fresh full Persistence/npm verification is still pending.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This draft PR hardens Windows persistence teardown and expands installer, TrayHost, uninstall, and release validation.
Changes:
- Strengthens allowlisted temporary test-tree cleanup and persistence coverage.
- Adds deterministic installer manifests, destination inventories, and release-note tooling.
- Improves portable publishing, uninstall finalization, TrayHost lifecycle handling, and CI validation.
Open findings:
Install-CodexRemote-fix.ps1:82— moderate, 1 vote: preserve the history as aHashSetbefore comparison.build/InstallerPackage.psm1:99,130— critical, 1 vote: reject boolean values and require contract integer types.src/persistence/modules/PortableRelease.psm1:358— moderate, 1 vote: safely roll back or quarantine the published directory after validation failure.src/trayhost/TrayTerminalReceiptSink.cs:77— moderate, 1 vote: enqueue and count receipts before signaling.tests/persistence/TestSupport.ps1:83— moderate, 3 votes: retry only known transient filesystem failures with re-enumeration.tools/New-GitHubReleaseNotes.ps1:25— moderate, 1 vote: enable strict invalid-UTF-8 rejection.tools/New-InstallerDestinationInventory.ps1:141— critical, 1 vote: inspect and reject a reparse-point source root.
File summaries
| File | Change |
|---|---|
Uninstall-CodexControlOtherDevices.ps1 |
Installed uninstall finalization flow |
tools/Test-CleanReleaseRunner.psm1 |
Clean-runner module wrapper |
tools/New-InstallerPayloadManifest.ps1 |
Deterministic payload manifest generation |
tools/New-InstallerDestinationInventory.ps1 |
Setup destination inventory |
tools/New-GitHubReleaseNotes.ps1 |
Release-note extraction |
tools/Invoke-GitHubDraftRelease.psm1 |
Draft-release module wrapper |
tests/Validate.ps1 |
Validation orchestration |
tests/trayhost/TrayHostParentClientSelfTest.cs |
Parent-client tests |
tests/trayhost/TrayHostCorrelationTestBridge.cs |
TrayHost test bridge |
tests/trayhost/Invoke-TrayHostSelfTest.ps1 |
TrayHost test runner |
tests/PersistenceSelfTest.ps1 |
Persistence test runner |
tests/persistence/TrayHostClient.SelfTest.ps1 |
TrayHost client tests |
tests/persistence/TestSupport.SelfTest.ps1 |
Cleanup tests |
tests/persistence/TestSupport.ps1 |
Temporary test-tree cleanup |
tests/persistence/StateStore.SelfTest.ps1 |
State-store tests |
tests/persistence/SessionEngine.SelfTest.ps1 |
Session-engine tests |
tests/persistence/ProcessControl.SelfTest.ps1 |
Process-control tests |
tests/persistence/PersistenceIO.SelfTest.ps1 |
Persistence I/O tests |
tests/persistence/ManualWrappers.SelfTest.ps1 |
Wrapper tests |
tests/persistence/LegacyReleaseFixture.ps1 |
Release fixtures |
src/trayhost/WindowsTrayHostRuntime.cs |
Windows TrayHost runtime |
src/trayhost/TrayTerminalReceiptSink.cs |
Durable receipt sink |
src/trayhost/TrayHostParentClient.cs |
Parent/child lifecycle |
src/trayhost/TrayHostChildSession.cs |
Child session handling |
src/trayhost/TrayHostApplication.cs |
TrayHost application lifecycle |
src/trayhost/Program.cs |
TrayHost entrypoint |
src/trayhost/NativeMethods.cs |
Native bindings |
src/trayhost/HostTransport.cs |
Host transport |
src/trayhost/CodexRemote.TrayHost.manifest |
TrayHost manifest |
src/trayhost/AssemblyInfo.cs |
TrayHost assembly metadata |
src/portable/CodexRemote.Portable.manifest |
Portable manifest |
src/portable/AssemblyInfo.cs |
Portable assembly metadata |
src/persistence/modules/UiPreferences.psm1 |
UI preference preservation |
src/persistence/modules/StateStore.psm1 |
State initialization |
src/persistence/modules/SessionEngine.psm1 |
Session execution |
src/persistence/modules/ProcessControl.psm1 |
Process-tree handling |
src/persistence/modules/PortableRelease.psm1 |
Portable payload publishing |
src/persistence/modules/PersistenceIO.psm1 |
Strict JSON I/O |
README.zh-CN.md |
Chinese release documentation |
README.md |
Release documentation |
package.json |
Project metadata |
Install-CodexRemote-fix.ps1 |
Portable installer |
docs/superpowers/specs/2026-08-28-v2522-install-runtime-reliability-design.md |
Runtime reliability design |
docs/superpowers/specs/2026-08-28-v2522-async-tray-receipts-design.md |
Async receipt design |
docs/superpowers/plans/2026-09-03-v2522-runtime-v7-closure.md |
Runtime closure plan |
docs/superpowers/plans/2026-09-03-v2522-cross-process-retry-closure.md |
Retry closure plan |
docs/superpowers/plans/2026-09-02-v2522-rollback-lease-ordering.md |
Lease ordering plan |
docs/superpowers/plans/2026-09-02-v2522-legacy-upgrade-safe-reclamation.md |
Upgrade reclamation plan |
CHANGELOG.md |
Release changelog |
.superpowers/sdd/2026-09-03-v2522-runtime-v7-closure/runtime-v7-report.md |
Runtime report |
.superpowers/sdd/2026-09-03-v2522-runtime-v7-closure/progress.md |
Runtime progress |
.superpowers/sdd/2026-09-03-v2522-runtime-v7-closure/implementation-brief.md |
Runtime implementation brief |
.superpowers/sdd/2026-09-03-v2522-cross-process-retry-closure/task-2-fix1-brief.md |
Retry task brief |
.superpowers/sdd/2026-09-03-v2522-cross-process-retry-closure/task-1-report.md |
Retry task report |
.superpowers/sdd/2026-09-03-v2522-cross-process-retry-closure/task-1-fix1-brief.md |
Retry fix brief |
.superpowers/sdd/2026-09-03-v2522-cross-process-retry-closure/progress.md |
Retry progress |
.superpowers/sdd/2026-09-03-v2522-cross-process-retry-closure/implementation-brief.md |
Retry implementation brief |
.superpowers/sdd/2026-09-02-v2522-rollback-lease-ordering/task-1-report.md |
Rollback report |
.superpowers/sdd/2026-09-02-v2522-legacy-upgrade-safe-reclamation/task-3-report.md |
Upgrade task report |
.superpowers/sdd/2026-09-02-v2522-legacy-upgrade-safe-reclamation/progress.md |
Upgrade progress |
.superpowers/sdd/2026-09-02-v2522-legacy-upgrade-safe-reclamation/final-fix-report.md |
Upgrade final report |
.superpowers/sdd/2026-09-02-v2522-durable-product-cleanup/task-2-report.md |
Cleanup task report |
.superpowers/sdd/2026-09-02-v2522-durable-product-cleanup/final-fix-report.md |
Cleanup final report |
.superpowers/sdd/2026-08-29-v2522-immutable-generation/task-6-fix-round-verification.md |
Generation verification |
.superpowers/sdd/2026-08-29-v2522-immutable-generation/task-5-fix1-report.md |
Generation fix report |
.superpowers/sdd/2026-08-29-v2522-immutable-generation/task-5-fix1-brief.md |
Generation fix brief |
.superpowers/sdd/2026-08-29-v2522-immutable-generation/task-5-brief.md |
Generation task brief |
.superpowers/sdd/2026-08-28-v2522-install-runtime-reliability/task-3-report.md |
Runtime task report |
.github/workflows/ci.yml |
CI workflow |
Review details
Suppressed comments (2)
Install-CodexRemote-fix.ps1:82
- This assignment receives the function's pipeline-enumerated
HashSet: an empty history becomes$null, while multiple keys become an array, but the later comparison calls$before.Contains(...)as if it were always a set. Materialize an ordinalHashSet[string]here (or make the helper useWrite-Output -NoEnumerate) before comparing post-scan detections.
build/InstallerPackage.psm1:130 - This duplicate package-record validation has the same
ValueTypeacceptance: JSONtruecan become a declared length of1and pass the package seal if the corresponding ZIP entry is one byte. Keep the package manifest schema strict by requiring an integer type here, not merely a value type.
- Files reviewed: 68/137 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| foreach($record in $records){ | ||
| if($record-isnot[pscustomobject]-or(@($record.PSObject.Properties.Name)-join',')-cne'path,length,sha256'-or | ||
| -not(Test-CcodInstallerPackageRelativePath ([string]$record.path))-or | ||
| $record.length-isnot[ValueType]-or[decimal]$record.length-ne[decimal][int64]$record.length-or[int64]$record.length-lt 0-or[int64]$record.length-gt$script:CcodInstallerPackageEntryMaximumBytes-or |
Comment on lines
+141
to
+143
| if (-not [IO.Directory]::Exists($sourceRoot)) { throw "Recursive [Files] source root is missing: $sourceRoot" } | ||
| $sourcePrefix = $sourceRoot.TrimEnd('\') + '\' | ||
| foreach ($entry in @(Get-ChildItem -LiteralPath $sourceRoot -Force -Recurse -ErrorAction Stop)) { |
Comment on lines
+83
to
+84
| } catch [IO.FileNotFoundException] { } | ||
| catch [IO.DirectoryNotFoundException] { } |
Owner
Author
|
CI failure diagnosis: run 34781536555 is bound to 17526cf and fails at the repository validation step because InstalledLifecycleHarness still uses ambient global process/CIM fixtures under the Windows runner. The focused StaticProbeWorker suite now passes locally after its teardown fix; the next patch will address the remaining InstalledLifecycle observation boundary before rerunning CI. |
Owner
Author
|
Pushed f9dc476: UninstallBootstrap test-owned teardowns now use the restricted cleanup helper, including OpenChild handle release before cleanup. Focused UninstallBootstrap passed 47 cases. Please rerun/check CI for the new head. |
added 14 commits
September 19, 2026 15:21
Windows hosted runners expose two ambient conditions that the installed lifecycle observation boundary and the native child handshakes cannot tolerate: - %TEMP% is the 8.3 short name C:\Users\RUNNER~1\AppData\Local\Temp, while the observation boundary requires canonical long-form paths and therefore rejected every fixture root built from the alias. - The console code page is UTF-8, so Windows PowerShell 5.1 derives Console.InputEncoding from it and Process.StandardInput emits a UTF-8 byte-order mark that corrupts the first JSON token a child reads. Windows PowerShell 5.1 has no ProcessStartInfo.StandardInputEncoding. Add shared helpers in tests/persistence/TestSupport.ps1 that canonicalize the temp root and that read and write child stdio as explicit BOM-free UTF-8 bytes, then use them for every fixture root and every child handoff. Evidence: the installed lifecycle harness and the TestSupport suite both pass locally under a reproduced runner condition (an 8.3 temp alias plus code page 65001); both previously failed with CCOD_INTEGRATION_OBSERVATION_UNAVAILABLE and 'Invalid JSON primitive: .'. On the runner the installed lifecycle harness now passes and a pre-existing unrelated InstallFileTransaction case is the next failure.
Hosted windows-latest runners expose ambient conditions that the repository test gates did not tolerate. Each defect below was reproduced from a runner log and fixed on the test side, without weakening a product security check. Temp roots: %TEMP% is the 8.3 short name C:\Users\RUNNER~1\AppData\Local\Temp while the installed lifecycle observation boundary requires canonical long-form paths, so every fixture root built from the alias was rejected with CCOD_INTEGRATION_OBSERVATION_UNAVAILABLE. Child stdin: the runner console page is UTF-8, so Windows PowerShell 5.1 derives Console.InputEncoding from it and Process.StandardInput emits a byte-order mark that corrupts the first JSON token a child reads. Windows PowerShell 5.1 has no ProcessStartInfo.StandardInputEncoding, so both sides now use explicit BOM-free UTF-8. Inherited owner: the runner runs elevated, so a new directory inherits owner S-1-5-32-544 (Administrators) rather than the token user. The legacy migration plan recovery predicate compared the owner against the token user and therefore rejected safe inherited evidence. It now accepts the token's default owner for inherited evidence and still requires the token user for exact evidence; a foreign owner is still rejected. Line endings: build\CodexControlOtherDevices.iss has no pinned line endings, so a Windows checkout is CRLF while the line-anchored Source: pattern expects a trailing [^\r\n]+$. Normalize the text before asserting content. Module path: npm and Actions start these Windows PowerShell 5.1 suites from a PowerShell 7 shell, whose PSModulePath cannot resolve inbox cmdlets such as Get-ExecutionPolicy. Restore the Desktop module paths exactly as tests/Validate.ps1 already does. The CI job limit is raised because the 43 sequential persistence suites measured about 33 minutes on a hosted runner, which the previous 20 minute limit could not cover.
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.
Summary
Verification
Release status