Skip to content

fix(planning): kill the server by native pid in the watch clean-stop test on Windows - #5748

Merged
kyle-sexton merged 1 commit into
mainfrom
fix/5723-watch-test-taskkill
Oct 1, 2026
Merged

kyle-sexton merged 1 commit into
mainfrom
fix/5723-watch-test-taskkill

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Closes #5723

Summary

On Windows Git Bash, round.sh stop already ends the watcher through the refused-poll path: watch.sh exits 3 with "the interview server was stopped (round.sh stop): not re-arming" and no watch.sh or curl long-poll is left (confirmed on the issue). The one failing case in watch.test.sh was a test defect: case (o) stopped the server with Git Bash kill "$spid", but $spid is a native Windows pid, so kill printed "No such process" and the server kept answering.

Fix

  • plugins/planning/surface/watch.test.sh, case (o): stop the server with taskkill //F //PID "$spid" when uname -s is MINGW*, MSYS* or CYGWIN*, and kill "$spid" elsewhere.
  • plugins/planning/surface/README.md: replace "That path has not been exercised on Windows Git Bash." with the verified behavior.
  • plugins/planning/.claude-plugin/plugin.json 0.58.3 to 0.58.4, with a CHANGELOG.md entry.
  • No digest-pinned section of the interview SKILL.md, context/loop.md or evals.json is touched.

Verification

Linux (WSL, this branch), bash plugins/planning/surface/watch.test.sh:

ok: round.sh stop stops the server
ok: round.sh stop ended the data dir's watch.sh
ok: watch.sh exits 3 at once on a refused poll after a clean stop
ok: exits 2 when the server is gone
PASS=27 FAIL=0

Windows, melo-desk-001, native Git Bash (MSYS_NT-10.0-26200, GNU bash 5.3.15, Git for Windows 2.55.0), a copy of this branch's plugins/planning under C:\Users\KyleSexton\claude-lane-sandbox\, launched through WSL interop with bash.exe plugins/planning/surface/watch.test.sh:

ok: round.sh stop stops the server
ok: round.sh stop ended the data dir's watch.sh
ok: watch.sh exits 3 at once on a refused poll after a clean stop
ok: exits 2 when the server is gone
PASS=27 FAIL=0

Before this change the same host gave PASS=26 FAIL=1 with "FAIL: watch.sh kept retrying after a clean stop" (issue comment 5940259605). No process from the run was left behind. Also green: plugins/planning/tests/interview-defenses.test.sh (PASS=165 FAIL=0), scripts/check-changelog-parity.sh (--check, --check-bump origin/main, --check-order), markdownlint-cli2 on the two touched .md files.

Related

🤖 Generated with Claude Code

…test on Windows

Case (o) of watch.test.sh stopped the server with Git Bash `kill`, which does not know the
native Windows pid the server records, so the server kept answering and the case failed on
Windows Git Bash while watch.sh itself was correct. The case now uses `taskkill //F //PID` on
MINGW*, MSYS* and CYGWIN* and `kill` elsewhere. watch.test.sh passes 27 of 27 on Linux and on
Windows Git Bash, and the surface README states the verified Windows behavior.

Closes #5723

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 1, 2026 21:10
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-01T21:14:25.756384Z 8be2f09 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 31s —— View job


Code review complete ✅

  • Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
  • Understand the request (ran /review:code-review per custom instructions)
  • Review changed files for correctness/maintainability
  • Post findings

Summary

Reviewed the diff (git diff origin/main...HEAD) across all 4 changed files:
plugins/planning/surface/watch.test.sh, plugins/planning/surface/README.md,
plugins/planning/.claude-plugin/plugin.json, plugins/planning/CHANGELOG.md.

No blocking findings. The fix is narrowly scoped and correct:

  • watch.test.sh case (o) switches to taskkill //F //PID "$spid" on MINGW*/MSYS*/CYGWIN* and keeps kill "$spid" elsewhere. $spid is read from the server's .interview-session.json pid field — a native Windows PID — which Git Bash's kill genuinely cannot signal, unlike this file's other kill "$pid" calls (wpid/mpid/cpid/apid), which are all bash job-control pids ($! of a backgrounded watch.sh/subshell) and remain correctly handled by plain kill. I checked every other kill/pid-capture site in the file (lines 48, 124, 198, 278, 326, 335, 374, 385-387, 398-405, 430-433) to confirm this is the only spot reading a native server PID, so the fix's scope (touching only case (o)) is correct and there's no parallel instance of the same bug elsewhere in the suite.
  • The //F //PID double-slash form is the correct MSYS escaping to stop Git Bash's path-mangling from rewriting /F//PID into Windows paths before taskkill.exe sees them.
  • README wording and the new CHANGELOG.md entry accurately describe the fixed behavior and match the plugin.json version bump (0.58.3 → 0.58.4) and existing changelog formatting/conventions.
  • The author's "Verification" section (WSL + Windows Git Bash runs) is author-claimed and I did not independently re-run it.

No security, architecture, or maintainability concerns in this diff.

@claude

claude Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 40s —— View job


Security review complete

  • Load /review:security-review instructions
  • Gather PR diff (gh pr diff 5748, origin/main...HEAD)
  • Review changed files for security-relevant surface
  • Post findings or clean-bill summary

No security issues found.

This PR touches only test-harness and documentation surface:

  • plugins/planning/surface/watch.test.sh — switches the clean-stop test case to taskkill //F //PID "$spid" on MINGW*/MSYS*/CYGWIN*, keeping kill "$spid" elsewhere. $spid is parsed from the data dir's own .interview-session.json, written moments earlier by the very same test run, not from user input or an external/untrusted source, so there's no injection surface in the taskkill/kill invocation.
  • plugins/planning/surface/README.md — documentation wording update describing verified Windows stop behavior; not a rules/instruction file, and it doesn't remove or soften any guardrail, so the instruction-surface-deletion check doesn't apply.
  • plugins/planning/.claude-plugin/plugin.json / CHANGELOG.md — version bump and changelog entry only.

No authn/authz, injection, secret-handling, or GitHub Actions concerns in scope for this diff.

@kyle-sexton
kyle-sexton merged commit 387447d into main Oct 1, 2026
23 of 34 checks passed
@kyle-sexton
kyle-sexton deleted the fix/5723-watch-test-taskkill branch October 1, 2026 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

planning: confirm round.sh stop leaves no watch.sh running on Windows Git Bash

1 participant