Repository navigation
fix(scripts): stop-all 支援單一 pid 檔 - #217
Conversation
Wrap stop-all pid-file enumeration in an array so StrictMode does not emit a Count error when exactly one pid file exists. Validation: parser check; scripts/tests/test-stop-all-single-pid.ps1; npx openspec validate stop-all-single-pid-cleanup --strict.
|
Warning Review limit reached
More reviews will be available in 7 minutes and 57 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a Set-StrictMode bug in scripts/stop-all.ps1 where Get-ChildItem returns a scalar FileInfo (not an array) when scripts/.run/ contains exactly one .pid file, causing .Count to error. This was discovered during post-merge closeout of PR #215 which added governance-service to the deploy/stop lifecycle.
Changes:
- Wraps the
.pidfile enumeration in@(...)to guarantee array semantics for zero, one, and many results. - Adds a focused regression test that creates a single stale
governance-service.pidand verifiesstop-all.ps1exits cleanly without strict-mode errors. - Adds OpenSpec change artifacts (
proposal.md,design.md,spec.md,tasks.md) documenting the fix rationale and verification.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
scripts/stop-all.ps1 |
Wraps Get-ChildItem in @(...) on line 121 to ensure array semantics under strict mode |
scripts/tests/test-stop-all-single-pid.ps1 |
New regression test: creates one stale pid file, runs stop-all, asserts no strict-mode .Count error |
openspec/changes/stop-all-single-pid-cleanup/proposal.md |
OpenSpec proposal documenting the bug origin and fix scope |
openspec/changes/stop-all-single-pid-cleanup/design.md |
Design decision record for the @(...) wrapping approach |
openspec/changes/stop-all-single-pid-cleanup/specs/one-click-deploy-hybrid/spec.md |
Spec delta: shutdown cleanup must handle 0/1/many pid files without strict-mode errors |
openspec/changes/stop-all-single-pid-cleanup/tasks.md |
Task checklist tracking fix implementation and validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function Assert-True { | ||
| param( | ||
| [Parameter(Mandatory = $true)] $Condition, | ||
| [Parameter(Mandatory = $true)][string] $Message | ||
| ) | ||
| if (-not $Condition) { throw "ASSERT FAILED: $Message" } | ||
| } |
![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: f2bd843119
ℹ️ 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".
| Set-Content -LiteralPath $PidFile -Value '999999' -Encoding ASCII | ||
|
|
||
| try { | ||
| $output = & powershell -NoProfile -ExecutionPolicy Bypass -File $StopAll 2>&1 | Out-String |
There was a problem hiding this comment.
When this test is run from a checkout that already has an active scripts\.run\governance-service.pid, it overwrites that PID with 999999 and then invokes the real stop-all.ps1 against the repo’s live .run directory and default ports. That can delete existing pid/log files and stop any workspace processes found by the fallback path, so running the focused regression during an active demo/deploy corrupts the session instead of being a safe test; use a temp sandbox/test run directory or add a test-only override for the run dir/ports before invoking the script.
Useful? React with 👍 / 👎.
| # one-click-deploy-hybrid — Spec Delta (stop-all-single-pid-cleanup) | ||
|
|
||
| ## MODIFIED Requirements | ||
|
|
||
| ### Requirement: Final Summary 可診斷性 |
There was a problem hiding this comment.
This adds a second active OpenSpec delta for one-click-deploy-hybrid while openspec/changes/governance-service-deploy/specs/one-click-deploy-hybrid/spec.md is still active. openspec/AGENTS.md says MUST NOT 平行開兩個改同一 capability 的 active change, so this state violates the NoSuccessorWhilePredecessorOpen gate and leaves reviewers with concurrent deltas against the same capability; fold this cleanup into the existing active change or archive/close the predecessor first.
Useful? React with 👍 / 👎.
|
|
||
| ### Requirement: Final Summary 可診斷性 | ||
|
|
||
| deploy and closeout scripts SHALL keep failure / recovery output diagnosable under strict mode. Shutdown cleanup MUST NOT emit strict-mode collection-shape errors when the runtime directory contains zero, one, or many `.pid` files. |
There was a problem hiding this comment.
The new active OpenSpec text is written in English, but openspec/AGENTS.md requires proposal/design/tasks/spec prose to be in Traditional Chinese while preserving only the parser-required headings. Leaving this delta in English makes the active change inconsistent with the repo’s OpenSpec contract, so translate the prose in the new change artifacts before merging.
Useful? React with 👍 / 👎.
| ## MODIFIED Requirements | ||
|
|
||
| ### Requirement: Final Summary 可診斷性 | ||
|
|
||
| deploy and closeout scripts SHALL keep failure / recovery output diagnosable under strict mode. Shutdown cleanup MUST NOT emit strict-mode collection-shape errors when the runtime directory contains zero, one, or many `.pid` files. |
There was a problem hiding this comment.
Because this delta is under MODIFIED Requirements for the existing Final Summary 可診斷性 requirement, archiving it will replace that requirement’s current text and scenarios. The formal spec already contains the success, failure, build/probe, and null-safe PID/signature scenarios under this requirement, but this delta only keeps the new stop-all scenario, so it would erase the deployed deploy.ps1 Final Summary contract unless the full existing requirement is repeated with the new scenario added.
Useful? React with 👍 / 👎.
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
#229) 把 13 個產品碼已 merge 進 main 的 active change 歸檔為不可變快照， 並將其 spec delta 併入 canonical specs。對齊 #197 收斂規約。 歸檔（archive/<merge-date>-<id>，git 偵測為 R100 純改名、零內容漂移）： a1-m1-closeout(#213) a2-version-diff-selector(#207) conv-coverage-report(#218/#220) conv-prioritize-retry(#221) conv-watch-toggle(#225) conversion-artifact-id-sanitize(#206) governance-service-deploy(#215) minio-fileserver-source(#204) minio-watch-auto-intake(#210) raise-claude-md-line-budget(#199) sessions-terminate(#226) stop-all-single-pid-cleanup(#217) test-deploy-rebuild-workflow(#198) canonical 併入： - 9 個新 capability（純 ADDED → 新建 spec）：a1-m1-closeout、a2-version-diff-selector、 conv-coverage-report、conv-prioritize-retry、conversion-control、conversion-artifact-id-sanitize、 minio-fileserver-source、minio-watch-auto-intake、test-deploy-rebuild-workflow。 - review-session-request-lifecycle：append sessions-terminate 的 ADDED requirement 「Operator 結束 session controlled action」（5 scenario），既有 7 requirement 不動。 - one-click-deploy-hybrid：併入 governance-service-deploy 與 stop-all-single-pid-cleanup 兩 delta，採「合併不取代」保全既有更豐富內容。依 deploy.ps1 現況權威 （4a=governance/4b=conversion/4c=Kit/4d=docker）調和 Phase 4 編號，並修正 canonical 其他兩處陳舊的舊 3 段編號（Mode C 入口 scenario、退出碼 stage 清單補 4d）。 - agent-doc-context-budget：raise-claude-md-line-budget 的 130 行預算已於 #199 併入， 本次為 archive-only。 驗證： - 結構檢查無殘留 ## ADDED/MODIFIED header、每 requirement 皆有 scenario、 43 archive 檔全 R100、git diff --cached --check 無 whitespace。 - 雙 agent 對抗驗證：完整性 PASS（無規範遺失）；一致性初判 FAIL 抓到 2 處 Phase 4 編號矛盾，已修正後複驗。 - 本機 openspec CLI 不可用（結構驗證代替）；openspec validate --strict 由 CI pr-review-agent 執行。 Claude-Session: https://claude.ai/code/session_01JEyNWhEmb3x8oinY3B2v9V Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Validation
Notes