Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate findings affect path safety, cleanup reliability, workflow coverage, and runner compatibility.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
This PR stabilizes the Windows portable extraction path and adds packaging, taskbar, relaunch, and concurrency smoke coverage.
Changes:
- Configures and tests a fixed portable unpack directory.
- Adds PowerShell lifecycle checks and a Windows Actions workflow.
- Updates ADRs, runbooks, E2E plans, and documentation mirrors.
| File | Summary |
|---|---|
scripts/windows-portable-e2e.ps1 |
Adds lifecycle checks. Critical (2 votes): canonicalize the unpack path before containment validation. Moderate (2 votes): stop discovered PI-Desktop.exe children during cleanup. Moderate (1 vote): surface final cleanup failures. Moderate (1 vote): replace the fixed sleep with bounded readiness polling. |
scripts/README.md |
Documents the Windows portable smoke tooling. |
docs/zh-CN/spec/08-meta/decisions-log.md |
Mirrors the D364 decision update. |
docs/zh-CN/spec/06-delivery/06-release-runbook.md |
Mirrors portable release behavior. |
docs/zh-CN/spec/06-delivery/04-e2e-test-plan.md |
Mirrors E2E-211 coverage. |
docs/spec/08-meta/decisions-log.md |
Updates the D364 decision record. |
docs/spec/06-delivery/06-release-runbook.md |
Documents portable runtime behavior. |
docs/spec/06-delivery/04-e2e-test-plan.md |
Expands E2E-211 coverage. |
docs/adr/0197-windows-portable-exe.md |
Records portable lifecycle consequences. |
apps/desktop/test/auto-update.test.mjs |
Verifies the packaging configuration. |
apps/desktop/package.json |
Sets the stable portable extraction directory. |
.github/workflows/windows-portable-e2e.yml |
Critical (1 vote): hosted runners may lack an interactive taskbar session; use a capable runner or separate the assertion. Moderate (1 vote): assert the NSIS artifact and latest.yml as required by E2E-211. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| runs-on: windows-latest | ||
| timeout-minutes: 60 |
| $portablePath | ||
| } | ||
| $tempRoot = (Resolve-Path -LiteralPath $env:TEMP).Path | ||
| $unpackRoot = Join-Path $tempRoot $UnpackDirName |
| foreach ($run in $runs) { | ||
| Stop-Process -Id $run.Id -Force -ErrorAction SilentlyContinue | ||
| } |
|
Updated in 05d178c:
Validation:
|
vastsa
left a comment
vastsa
left a comment
There was a problem hiding this comment.
Thanks for the Windows portable lifecycle work. The current Windows portable E2E is still a landing blocker: run 35524266283 fails with Portable app did not start at C:\Users\runneradmin\AppData\Local\Temp\PI-Desktop-Portable\PI-Desktop.exe. Please fix or classify that startup failure and rerun the workflow.
There is also a concurrency safety concern: the fixed unpack directory is shared by concurrent portable launches, so one run can clean or overwrite files used by another process. Please make the unpack directory ownership-safe (or serialize it with an explicit lock) and cover the concurrent case before merging.
|
I verified that the taskbar/portable lifecycle problem in #663 is real, but the current PR head is not landing-safe. The required Windows job still fails in run 35524266283 with “Portable app did not start at C:\Users\runneradmin\AppData\Local\Temp\PI-Desktop-Portable\PI-Desktop.exe”. The current approach also still uses a shared fixed unpack directory, which can let concurrent runs clean up or overwrite one another. Because the packaging/taskbar lifecycle is not proven by the required E2E and the root cause is not safely closed, I am closing this PR. Issue #663 remains open. |
No description provided.