Skip to content

feat(planning): interview watcher stop, restatement diff and page polish - #5722

Merged
kyle-sexton merged 9 commits into
mainfrom
feat/5569-interview-remaining-gaps
Oct 1, 2026
Merged

kyle-sexton merged 9 commits into
mainfrom
feat/5569-interview-remaining-gaps

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Refs: #5569

Summary

Fourth pass on #5569, the planning interview page and watcher. Ships the largest coherent batch still open on origin/main. Remaining batches are filed as child issues (#5712, #5713, #5714, #5715, #5716, #5723), each starting Refs #5569. Item 8's disable (it reverses the Answer-anyway contract from #4547) is the owner's call and is left out.

Fix

  • round.sh stop ends the data dir's watch.sh through the lease pid; a poll refused after stop exits 3. Windows Git Bash is not covered here: it is tracked in planning: confirm round.sh stop leaves no watch.sh running on Windows Git Bash #5723, which needs a Windows host and an operator run.
  • apply warns when no watcher holds the lease; add defaults to the interview stage; status reports drifted seeded rounds and repair-rounds fixes them.
  • Page: restatement notice outranks notes, persistent changed-restatement banner, Confirm line diff against the newest confirmed rev, #1/(a)/option N in a note offers "Accept with note?", Needs-you toast, finish kept across reloads, and rail, activity and notes polish (items 16 f-r, 15.3 and 15.5).
  • note-reply --needs-answer pins a Claude question as a loose end.
  • planning 0.56.2 to 0.57.0 with a changelog entry.

Verification

  • bash plugins/planning/surface/surface.test.sh: PASS=451 FAIL=0 SKIP=1 (htmlhint not installed)
  • python3 -m unittest discover in plugins/planning/surface: OK (skipped=1)
  • scripts/check-changelog-parity.sh --check, --check-order, --check-bump origin/main: pass
  • scripts/validate-plugins.sh: all manifests validated
  • origin/main merged cleanly.

Related

Issue #5569; prior PRs #5622, #5639, #5652. Child issues: #5712, #5713, #5714, #5715, #5716, #5723.

🤖 Generated with Claude Code

kyle-sexton and others added 7 commits October 1, 2026 09:01
…pair drifted seeded rounds

Refs: #5569

- round.sh apply prints "no watcher armed; N unhandled events" to stderr when no watcher holds the lease; it still writes and exits 0.
- add without a stage takes the newest question's stage and warns, naming the stage and round.
- status lists each seeded question whose round differs from its ledger round cell, and repair-rounds rewrites only those.
- export-brief reads a ledger-only row's named fields, so its defer-until text is unescaped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs: #5569

- watch.sh sends its pid with each poll; the server keeps it in the lease and /api/state shows it.
- round.sh stop sends that pid SIGTERM after the finish, only when its command line is a watch.sh for this data dir; nothing is signalled on Windows.
- watch.sh exits 3 at once on a failed poll once stop has removed the env file, instead of retrying WAIT_FAILS times.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…notes, toast new Needs-you items

Refs: #5569

- A restatement still to confirm outranks a later note in the notice, a persistent banner says it changed, and the Confirm block shows a line diff against the newest confirmed rev.
- An own or ask note that names one card label (Rec, #1, option 2, (a)) asks "Did you mean Accept with note?" before it is saved as typed.
- Confirm shows a line saying it ticks no commitment, that unticked ones become named risks, with a link to the commitment list.
- New Needs-you items raise a dismissible toast, including interview complete; a browser notification is offered only on a click, and the tab-title count counts only Needs-you items.
- The finish text is kept in localStorage, so a tab that cannot reach the server still shows it; a resumed interview drops it.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…in Claude questions as loose ends

Rail preview skips hold bookkeeping, the id column is fixed width, the Pending
filter covers every hold and a filtered-empty rail says so, Ungrouped leads when
it holds the lowest ids, the meter title states the archived count, and the rail
cut at 820 px is marked. Option titles take two lines and the answer grid is one
column below 1200 px. Activity no longer relists ids its text links, the thread
no longer repeats the latest Claude reply, and a Claude asked line carries the
question. A note posted with no reply target reads "Claude posted in Notes" and
the CLI says "Note posted". note-reply --needs-answer pins the note as the first
Loose end with a reply box. A save while a delivered event is unhandled says
"Queued, Claude is busy".

Refs: #5569

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t and page polish

Refs: #5569

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts:
#	plugins/planning/.claude-plugin/plugin.json
#	plugins/planning/CHANGELOG.md
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 1, 2026 16:36
@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-01T16:41:02.727113Z e8b7e8b 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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e8b7e8b408

ℹ️ 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".

Comment thread plugins/planning/surface/index.html Outdated
Comment thread plugins/planning/surface/index.html Outdated
…ed for another data dir

A Claude note flagged needs-answer stays a loose end until a note from the user follows it,
not only while it is the last Notes line. A finish cached under the shared origin key is
replaced when its session differs from the one the server reports, and cleared when the
server reports no finish.

Refs: #5569

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 19s —— View job


Security review (/review:security-review)

  • Gather context (PR diff, changed files)
  • Review changed files for security-relevant surface
  • Check instruction-surface deletions against protected classes
  • Post findings

No security issues found.

Scope: all 17 changed files (gh pr diff against the PR head), with focus on server.py, round.py, watch.sh, exporters.py, and the client-side JS in index.html.

What I checked:

  • New watcher-kill path (round.py end_watcher, called from cmd_stop): round.sh stop now sends SIGTERM to the PID recorded in the lease. The PID is attacker-reachable in principle (/api/wait?...&pid=$$ sets it), but /api/wait is gated by token_ok() (server.py:1365) and end_watcher verifies /proc/<pid>/cmdline (or ps) contains both watch.sh and the data dir's name before signaling — no shell interpolation, list-form subprocess.run, no arbitrary-process kill. This stays inside the existing trust boundary (token holder is already fully trusted / has filesystem control via round.py).
  • Client-side rendering of all new UI (restatement diff banner, needs-answer "loose end" reply box, Needs-you toast, browser Notification calls, cached-finish restore): every new string built with setHtml/innerHTML runs through the existing esc()/inline()/md() escaping helpers before insertion; nothing bypasses them. Notification(..., {body: text}) takes plain text, not HTML, so no injection path there either.
  • localStorage finish cache (iv2:finish): the two UI/data-integrity bugs Codex flagged in its automated review (needs-answer note losing its pin, and a finish cached for one data dir bleeding into another on port reuse) are correctness issues, not security vulnerabilities — no cross-origin or cross-session data exposure, since localStorage is already scoped to the origin/port. Both appear already addressed in the latest commit (b8a828a).
  • Round-drift / repair-rounds and apply's no-watcher warning: pure local file read/write, no new untrusted-input parsing beyond what parse_named already handled (and the new call site wraps it in try/except SystemExit, degrading safely rather than crashing).
  • Instruction-surface check: no CLAUDE.md/AGENTS.md/rules files/skill or agent bodies are touched by this PR; the one skill-context doc changed (skills/interview/context/surface.md) only adds documentation for new behavior, it doesn't remove or narrow any guardrail.

No CRITICAL, IMPORTANT, or SUGGESTION findings to report.

@claude

claude Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 47s —— View job


Code review complete

  • Gather context (PR body, diff, prior Codex review comments)
  • Run /review:code-review against this PR
  • Post findings as inline PR comments
  • Update summary comment with overview

Scope: git diff origin/main...HEAD (17 files, +626/-84) via gh pr diff 5722, focused on the code-review lane's criteria (correctness, maintainability, test coverage; security is left to the dedicated security lane).

On the two prior Codex findings (needs-answer note losing its pin when an ordinary note follows; a finish cached under the origin-wide iv2:finish key leaking across data dirs on the same port): both are already fixed in the latest commit, b8a828a.

  • plugins/planning/surface/index.html now tracks the pinned ask by scanning for the last needsAnswer note and checking whether any by: "you" note follows it (askAt/ask in the loose-ends builder), rather than requiring it to be the thread's last entry.
  • finLoad()/finSave() now compare kept.session !== S.session before reusing or clearing the cached finish, so a finish from another data dir on the same port is dropped instead of shown.
  • Both fixes are covered by new assertions in plugins/planning/surface/tests/ui_journeys.js (the "a later ordinary Claude note leaves the flagged question pinned" and "a finish kept for another data dir is replaced by this one's" cases), which I read but did not execute myself (author-claimed verification, per the PR's Verification section, is not independently re-run here).

Rest of the diff: round.py's new end_watcher/watcher_lease/cmd_repair_rounds, watch.sh's clean-stop detection, server.py's lease pid plumbing, and exporters.py's round_drift all read correctly against their accompanying tests (test_round.py, test_exporters.py, watch.test.sh). The watcher-kill path only signals a PID after confirming its /proc (or ps) command line contains both watch.sh and the data dir name, so a stale or attacker-influenced lease PID can't cause an arbitrary kill. The new interview-page logic (refOf/resolveRef label-matching dialog, reDiffHtml/confirmedRev, renderReBanner, needToast/notifyHidden) checked out against its exercising UI-journey assertions.

No additional high-signal findings — nothing here rose to a "a careful senior reviewer would block or flag" bar beyond what Codex already caught and the author already fixed. No inline comments posted.

@kyle-sexton
kyle-sexton merged commit 864fb61 into main Oct 1, 2026
20 checks passed
@kyle-sexton
kyle-sexton deleted the feat/5569-interview-remaining-gaps branch October 1, 2026 17:04
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.

1 participant