Skip to content

feat(planning): add research events and waiting-for-reply cues to the interview page - #5639

Merged
kyle-sexton merged 13 commits into
mainfrom
fix/5569-interview-page-remaining
Oct 1, 2026
Merged

kyle-sexton merged 13 commits into
mainfrom
fix/5569-interview-page-remaining

Conversation

@kyle-sexton

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

Copy link
Copy Markdown
Contributor

Refs: #5569

Summary

Part of #5569, a running log that stays open after this PR. The first pass (#5622) met its early items. This PR adds a further set that was still open on main:

  • The revising chip comes from a question's own unhandled decision (item 2).
  • A bare #N links through meta.repo.
  • The summary's Confirm sits below the What-is-off box (the 22:23Z comment).
  • Research this and Cancel research, with a "Research in progress, started " card (item 7, and the card and Cancel part of item 8).
  • A waiting-for-reply chip, and Accept asks first (the wait rule in item 9).
  • The truncation repro checks, the decide-area height on short screens, the 390px layout, and stale-read guidance in the wake contract.

Not done here:

  • Item 8's disable. The issue asks for Accept, the alternatives and Own answer to be disabled while Claude holds a question. They stay enabled (Answer anyway), which feat(planning): redesign the interview page for the multi-round flow #4547 shipped and context/surface.md documents. Disabling them would reverse that contract, and Cancel research only posts an event that the session must handle, so a disabled page would depend on Claude releasing the hold. The owner decides; nothing on the issue settles it.
  • Batch B (B1 to B6) and batch A items 11 to 19. For example there is no sync-ledger and no export-ledger --diff (B1), and import-ledger still reads "round 5 (sweep S1 to S3)" as round 513 (item 18).
  • The 22:25Z to 22:39Z comments. Wrap-up exports that lose ledger-only rows, glued Q1-Q3 range chips, the offline and finish modal, the "N to confirm" wording, and the source-text-first default.

Fix

  • The revising chip reads "Answer not handled yet". question_states in server.py marks a question from its own delivered, unhandled decision event, so a changed prerequisite shows in the Stale and Waiting-on chips instead of naming an upstream question.
  • The page links a bare #N through meta.repo; round.py warns, without blocking, when text has one and meta.repo is unset.
  • The summary renders the textarea first, then one action row with Confirm and Something's off.
  • research and cancel-research events, a held card reading "Research in progress, started " from waitingSince, and the wake contract re-reading events after a stale-read refusal.
  • A waiting-for-reply chip, and Accept asks for confirmation while Claude has not replied.
  • The decide fieldset caps at 40% of the viewport on short screens; the phone layout wraps the status line, including an unbroken run.
  • Planning bumps to 0.54.0 (minor: new event kinds, meta.repo, waitingSince and two buttons), with the new capability under Added in the changelog. The meta op README line says five keys. origin/main is merged in (the hedged kind and research kinds coexist in the schema and server).

Verification

  • surface.test.sh: PASS=362 FAIL=0 SKIP=1 (htmlhint absent).
  • unittest discover over plugins/planning/surface: 443 tests OK (1 skipped), including a new test for the repo key on the meta op.
  • scripts/run-ruff.sh check and format --check on plugins/planning: clean.
  • check-changelog-parity.sh --check, --check-order, --check-bump origin/main, validate-plugins.sh, typos and markdownlint on the changed Markdown: clean.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 11 commits September 30, 2026 18:36
The page links owner/repo#N to that repo's issue and a bare #N to meta.repo's,
leaving [text](url) links, Qn refs and code spans as written. meta.repo is an
allowed meta key in the file and the meta op. round.py warns, without blocking,
when question or op text carries a bare #N and meta.repo is unset.

Refs: #5569

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…vent

Refs: #5569

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Render the textarea first as a block, then one action row with Confirm and Something's off.

Refs: #5569

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

The page posts a generic `research` event per question and a `cancel-research`
event while Claude holds it. Neither names a plugin; surface.md routes research
to /discovery:research when it resolves, else an inline lookup. A hold records
`waitingSince` so the held card reads "Research in progress, started <time>".
The wake contract now says to re-read the events and restate after a stale-read
refusal instead of passing force.

Refs: #5569

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e Claude has not replied

Refs: #5569

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

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ne layout

Cap the decide fieldset at 40% of the viewport height under 800px tall. At phone width the
question rail no longer scrolls inside itself, the header is compact, and the status line wraps.
Add ui checks for both.

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

Bump planning to 0.53.1 with its changelog entry. At phone width the Claude line wraps
anywhere, so an unbroken run no longer widens the page.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…y docs

The branch adds the research and cancel-research event kinds, meta.repo,
the Research this and Cancel research buttons and waitingSince, so the
release is a minor bump with the new capability under Added. The entry no
longer claims to finish issue 5569, says the revising chip comes from a
question's own unhandled decision, and says the answer controls stay
enabled on a held question. The meta op takes five keys; the README and
the set_meta docstring said four.

Refs: #5569

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@kyle-sexton kyle-sexton changed the title fix(planning): finish the interview page gaps left open in #5569 feat(planning): add research events and waiting-for-reply cues to the interview page Oct 1, 2026
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 1, 2026 01:29
@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-01T01:36:48.992075Z 6a11f78 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 39s —— View job


Security review complete

  • Gather context (PR diff, changed files)
  • Review diff for security-relevant surface
  • Check for instruction-surface deletions (CLAUDE.md/AGENTS.md/rules)
  • Report findings

No security issues found.

Scope reviewed: all 18 changed files (git diff origin/main...HEAD), with focus on the files most likely to carry exploitable surface:

  • index.html — the new bare-#N/owner/repo#N issue-link rendering (link() helper and its two regex call sites). Both the inline [\w.-]+/[\w.-]+ repo capture and the meta.repo value used as a fallback are validated against /^[\w.-]+\/[\w.-]+$/ before being interposed into the built href, and the surrounding text has already passed through esc() (escaping &<>"') before these regexes run, so there's no way to break out of the attribute or inject markup through either path. The new waitingSince/"Research in progress" card and the confirmAccept() dialog use only esc()-wrapped or static strings in their innerHTML.
  • server.py — adding research/cancel-research to REQUESTS doesn't bypass the existing record() validation: both still require id to resolve to a real question in questions.json (confirmed by the new test_08a_research_and_cancel_research_are_accepted_without_a_decision test, which asserts a 400 for an unknown id and an unknown kind). No new unauthenticated surface — /api/answer is still the same token-guarded, localhost-only endpoint, and MAX_BODY/LINE_CAP limits are untouched.
  • round.py — warn_bare_issue_refs()/strings() only walk JSON values to emit a non-blocking warning; the new regexes (BARE_ISSUE_REF, CODE_SPAN) are linear, no backtracking/ReDoS risk. meta.repo is a CLI-only (round.sh/apply), trusted-local-operator input; the client still re-validates it before use as shown above.
  • Schemas (event.schema.json, ops.schema.json, questions.schema.json) — additive only (research/cancel-research kinds, meta.repo, waitingSince), nothing loosened.
  • skills/interview/context/surface.md — documentation changes are additive (new events/fields documented); no standing instruction, guardrail, or rule is deleted, narrowed, or softened, so the instruction-surface-deletion lens doesn't apply here.

This tool remains 127.0.0.1-only and token-guarded per server.py's existing design; nothing in this PR widens that trust boundary.

@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: 6a11f78652

ℹ️ 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
Comment thread plugins/planning/surface/round.py Outdated
…ta.repo

A question held for the user's answer no longer offers Research this, since
posting research would replace that hold. The bare #N warning now fires when
meta.repo is not an owner/repo slug, matching what the page can link.

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 1m 28s —— View job


Security review complete

  • Gather context (git diff origin/main...HEAD, 18 changed files)
  • Review diff for security-relevant surface (injection, authz, trust boundaries, secrets)
  • Check for instruction-surface deletions (CLAUDE.md/AGENTS.md/rules)
  • Report findings

No security issues found.

Scope: the full diff at HEAD 0fe76df (18 files, +270/-45), concentrated on the files carrying executable or trust-relevant surface:

  • index.html — bare #N/owner/repo#N issue-link rendering. The link() helper and both regex call sites constrain repo to [\w.-]+\/[\w.-]+ and n to \d+ before interpolating into the built href/anchor text, and the surrounding text has already passed through esc(). No way to break out of the attribute or inject markup through either path, and S.doc.meta.repo is re-validated client-side against the same pattern even though it originates from a trusted local CLI operator, not a remote actor.
  • index.html — new researchNow() toast. Interpolates the question id into the toast string, but renderToast() wraps the whole message in esc() before innerHTML, so this stays safe.
  • server.py — research/cancel-research added to REQUESTS. Both still flow through record()'s existing qid not in qs check, so an unknown id is still rejected; no new unauthenticated surface, and /api/answer stays the same token-guarded, localhost-only endpoint. MAX_BODY/LINE_CAP are untouched.
  • round.py — warn_bare_issue_refs()/strings(). Linear regexes (BARE_ISSUE_REF, CODE_SPAN, REPO_SLUG), no backtracking/ReDoS risk; this only emits a non-blocking warning and never gates apply. meta.repo is CLI-only, trusted-local-operator input, and the client independently re-validates it before use (see above) — not a double-trust gap.
  • A prior Codex review flagged two issues against an earlier commit (6a11f78): (1) the "Research this" button rendering while a question waits on the user, and (2) warn_bare_issue_refs treating any non-empty meta.repo as configured even when malformed. Both are already fixed on this head: the talk-row logic at index.html#L1032-L1039 now special-cases st === "need" to render neither button, and warn_bare_issue_refs at round.py#L331-L333 now checks REPO_SLUG.fullmatch(...) rather than mere truthiness. Both were code-quality/correctness issues rather than security findings and are not re-reported here.
  • Schemas (event.schema.json, ops.schema.json, questions.schema.json) — additive only (research/cancel-research kinds, meta.repo, waitingSince), nothing loosened.
  • Instruction-surface check — skills/interview/context/surface.md changes are additive documentation (new events/fields/behavior documented); no standing instruction, guardrail, or rule is deleted, narrowed, or softened. docs/conventions/instruction-exception-register/README.md was not present in this checkout to consult directly, but the diff reads as pure addition under the consequence test, so the lens doesn't surface a finding here.

The tool remains 127.0.0.1-only and token-guarded per server.py's existing design; nothing in this PR widens that trust boundary.

@claude

claude Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review complete

  • Gather context (PR diff, changed files)
  • Apply skip gate (not a skip: PR is open, ready for review, meaningful diff)
  • Review changed files against CLAUDE.md / AGENTS.md and code-review lane criteria
  • Post inline findings (none met the bar)
  • Final summary

No new findings at this head (0fe76df).

Both issues Codex flagged on the prior revision (6a11f78) are already fixed by the latest commit, 0fe76df ("hide Research this on user holds and lint malformed meta.repo"):

  • index.html's talk-row now has an explicit st === "need" branch (index.html#L1037) so a question held on the user (waitingBy: "user") no longer renders an enabled "Research this" button.
  • round.py's warn_bare_issue_refs now validates meta.repo against REPO_SLUG (fullmatch on owner/repo) (round.py#L334) instead of just checking truthiness, so https://github.com/o/r or o/r/ still trigger the warning. Covered by test_bare_issue_ref_warns_when_meta_repo_is_not_an_owner_repo_slug.

Traced through the rest of the diff for correctness (not restating the author's own verification, which I did not independently re-run):

  • server.py's question_states rewrite: revising is now computed directly from each question's own events entries (not propagated to children), matching the updated docstring and test_7_revising_marks_the_question_with_its_own_delivered_unhandled_decision. children is still used for the stale-cascade logic a few lines up, so it isn't dead code.
  • record() in server.py correctly leaves research/cancel-research out of DECISIONS, so they don't write to r["responses"][qid] and still 400 on an unknown id — matches test_08a_research_and_cancel_research_are_accepted_without_a_decision.
  • The bare-#N / owner/repo#N linking regexes in index.html's inline() (link() helper) only activate on a validated meta.repo slug and are checked after esc(), consistent with the earlier security pass on this PR.
  • op_wait in round.py stamps and clears waitingSince symmetrically with the other hold fields; the "Research in progress, started …" card only reads it for st === "wait", not "need", which is correct per the design.

Scope was the 18 changed files via git diff origin/main...HEAD. Security-relevant surface was already covered by the dedicated security-review lane on this PR, so it's out of scope here.

@kyle-sexton
kyle-sexton merged commit cd1d0b6 into main Oct 1, 2026
20 checks passed
@kyle-sexton
kyle-sexton deleted the fix/5569-interview-page-remaining branch October 1, 2026 01:58
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