Skip to content

feat(planning): accept-all-and-check action on the interview page routed to audit-answers - #5536

Open
kyle-sexton wants to merge 13 commits into
mainfrom
feat/5472-accept-all-audit
Open

kyle-sexton wants to merge 13 commits into
mainfrom
feat/5472-accept-all-audit

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Closes #5472

Summary

The interview page gains an "Accept all and have agents check them" action on each round. It posts one accept-audit event; the interview skill routes that event to /planning:audit-answers. The page holds no validation logic. Follows the owner decision on #4653 (Option B).

Fix

  • Server: POST /api/answer {kind:'accept-audit', alt:<round id>, items:[{id,contentRev}]} writes one accept-audit event, then one ordinary accept per eligible item (auditSeq links them). Ineligible or changed items are returned in skipped; nothing accepted returns 409.
  • Schema, exporters: an accept tied to an audit exports as accepted: REC; note: pending agent validation.
  • Page: round sections carry the button and dialog; per-question undo still works. A question carrying a typed note is left out of the dialog (the event carries no notes); accept it alone to keep the note.
  • Skill: context/surface.md adds the accept-audit event row, says to handle its seqs, and routes it to /planning:audit-answers.
  • planning 0.48.0 with a CHANGELOG entry.

Verification

  • python3 surface tests: test_server 151 (1 skipped), test_exporters 106, test_schema 17, test_round 92: pass.
  • plugins/planning/surface/surface.test.sh: PASS=296 FAIL=0 SKIP=1 (htmlhint not installed).
  • plugins/planning/tests/interview-defenses.test.sh: PASS=156 FAIL=0 (includes new routing pins).
  • scripts/check-changelog-parity.sh --check --check-order: pass.
  • scripts/validate-plugins.sh: pass.

Related

Refs #4653 (owner decision 2026-09-29, Option B). #5480 records the decision in docs.

🤖 Generated with Claude Code

kyle-sexton and others added 7 commits September 29, 2026 22:33
One accept-audit event carries a round id in alt and the accepted
questions as id and contentRev pairs. The server accepts each eligible
item, skips a changed or ineligible one, writes the accept-audit event
followed by one accept per accepted item carrying auditSeq, and answers
with the accepted ids and the skipped ids with a reason. Exporters write
an accept with auditSeq with the note "pending agent validation" so it
never reads as a plain hand accept; commitments stay unconfirmed.

Refs #5472

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

A round section with opened recommended questions gets a second button
beside Accept all. Its dialog says agents will check the accepted answers
and commitments stay unconfirmed. Confirming posts one accept-audit event
with the round and the id and contentRev pairs, toasts the server's
accepted and skipped result, and refreshes. UI checks cover the button's
presence and absence and the single event.

Refs #5472

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

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

The accept-audit event carries no per-question text, so the dialog showed
a typed note that was never sent and then cleared its draft. Questions
carrying a note are now left out of the audit dialog, like challenged ones,
and can be accepted one at a time to keep the note. surface.md says to
handle the accept-audit seq and its accepts, and the dead accept-audit
kind is removed from loose_ends.

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

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review September 30, 2026 03:46
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 30, 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-09-30T03:50:58.999645Z 70a728d 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: 70a728d15a

ℹ️ 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/skills/interview/context/surface.md Outdated
Scope the accept-audit routing to the accepted questions only, so the audit
skill does not fill or validate open branches outside the selected round.

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

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review — /review:security-review

  • Gather context (PR diff, body, review comments)
  • Run /review:security-review for this PR (CI security lane)
  • Post findings

Scope reviewed: the 14 changed files, focused on the new accept-audit code path — plugins/planning/surface/server.py (check_accept_audit, split_accept_audit, the fan-out loop in Hub.record), plugins/planning/surface/schema/event.schema.json, plugins/planning/surface/exporters.py (pending_validation), and plugins/planning/surface/index.html.

No security findings.

Notes from the review, for context (none rise to a reportable finding):

  • Auth/trust boundary is unchanged: do_POST still gates every /api/answer request (including accept-audit) on origin_ok() and token_ok() before dispatch — server.py#L1361-L1366. The new kind doesn't open a new unauthenticated path.
  • split_accept_audit (server.py#L280-L312) and the fan-out (server.py#L940-L947) all execute inside the existing self.cond lock, so there's no TOCTOU window between the eligibility check and the write.
  • The server does not cross-check that an accept-audit event's items actually belong to the round named in alt — it only re-validates each item's own eligibility (open, has a recommendation, not held/waiting, contentRev matches, prerequisites decided). That's a data-integrity nuance, not a privilege escalation: this is a single-token, single-user local tool, and a token holder can already accept any eligible question individually via the existing plain accept kind, so grouping under accept-audit grants no new capability.
  • The previously flagged Codex P1 ("audit-answers consumes the whole interview, not just the selected round") is addressed in this same PR: context/surface.md's routing row now says to run /planning:audit-answers "on the event's items only," and audit-answers/SKILL.md Step 1 adds "When the caller names the question ids to audit... do not fill or validate any other open branch." This is an instruction-level (skill-prompt) fix rather than code-enforced, consistent with how this plugin's skills already work.
  • event.schema.json's new items array is additionalProperties: false with required id/contentRev, and check_accept_audit independently re-validates shape/types server-side, so a malformed or oversized payload (capped by the existing 64 KB body limit) is rejected with 400 before any write.
  • index.html's new dialog path (acceptAllDialog(..., audit)) renders item ids/titles through the existing esc() escaping, no new unescaped interpolation.
  • No instruction-surface deletions in this diff (context/surface.md and SKILL.md only gain rows/sentences; nothing removed or narrowed).

🤖 Generated with Claude Code

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review (/review:code-review)

  • Gather context (PR body, diff via gh pr diff, review skill)
  • Review changed files against /review:code-review criteria
  • Verify the earlier Codex P1 finding against current HEAD
  • Post summary

Scope: all 14 changed files (plugins/planning/surface/server.py, exporters.py, index.html, schema/event.schema.json, skills/audit-answers/SKILL.md, skills/interview/context/surface.md, tests, CHANGELOG/version).

Findings

No blocking correctness or maintainability issues found. Highlights of what I checked:

  • server.py accept-audit path (check_accept_audit, split_accept_audit, record): validation rejects malformed alt/items (missing, empty, wrong types, duplicate ids, extra keys) before touching the lock; eligibility correctly excludes closed/archived/held/no-recommendation/undecided-prerequisite questions and rejects a stale contentRev; a same-batch prerequisite (not yet in r["responses"]) correctly does not count as decided (matches test_4_a_prerequisite_decided_only_in_the_same_batch_does_not_count); content_rev only bumps from the fanned-out accept events, not the accept-audit event itself, matching the undo/re-accept test.
  • exporters.py pending_validation: correctly keys off the resolved decision's own seq matching an event with auditSeq, so a later decision or an imported terminal answer (no seq) reads as its own rather than inheriting the pending note — matches test_a_later_decision_or_terminal_answer_reads_as_its_own.
  • index.html: the new "Accept all and have agents check them" button is scoped to Rounds sections only (s.round && !s.gid), sends no per-item notes, and correctly leaves out any question carrying a typed note (held filter) — consistent with the dialog copy and ui_b.js's new assertions.
  • Scoping to the round — the Codex review on this PR flagged (against an earlier commit, 70a728d) that /planning:audit-answers didn't consume the event's items, so it could fill and collapse questions outside the selected round. Current HEAD already fixes this: skills/audit-answers/SKILL.md#L51 ("When the caller names the question ids to audit ... the answer set is exactly those answers: do not fill or validate any other open branch") and skills/interview/context/surface.md#L133 (routes on "the event's items only, so questions outside the round stay open"). No further action needed here.

Verification

The sandbox here blocks python3/test execution (approval required, none available in this unattended run), so I could not independently run test_server.py / test_exporters.py / test_schema.py / the shell test suites. The PR body's reported pass counts are author-claimed and unverified by me — static review of the diff is consistent with those results (test additions align with the implementation's actual branching), but I did not execute them.

No inline comments to post — nothing met the high-signal bar for a blocking finding.

State the caller-named id scope under Preconditions so the pinned Step 1
section stays byte-identical.

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

Copy link
Copy Markdown
Contributor Author

Correction to the earlier reply: the id-list scope is stated under Preconditions in audit-answers/SKILL.md (Step 1 is pinned by interview-defenses.test.sh and stays unchanged), fixed in the latest push.

kyle-sexton and others added 3 commits September 30, 2026 01:40
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review — /review:security-review

  • Gather context (PR diff, body, review comments)
  • Run /review:security-review for this PR (CI security lane)
  • Post findings

No security findings.

Scope and re-verification note: the task's HEAD SHA (edf1e2d1) is an ancestor of the branch tip in this checkout (3e264441); the commits between them touch only plugins/planning/CHANGELOG.md, a duplicate-key fix in plugin.json, and interview-defenses.test.sh — no security-relevant surface. Network access (git fetch, gh) was unavailable in this sandbox, so I reviewed against the local checkout rather than re-fetching; the diff between edf1e2d1 and the checked-out tip is fully accounted for above, so this is not a scope gap.

I independently re-read the core accept-audit path rather than relying solely on the prior review posted on this PR (2026-09-30T05:27:20Z):

  • Auth boundary unchanged — do_POST still gates every /api/answer request on origin_ok() and token_ok() (server.py#L1362-L1366) before any kind, including accept-audit, is dispatched.
  • check_accept_audit (server.py#L583-L606) strictly validates shape: alt must be a non-empty string, items a non-empty list of exactly {id: str, contentRev: int} with no duplicate ids — malformed or oversized input (capped by the existing body-size limit) is rejected with 400 before the lock is taken.
  • split_accept_audit (server.py#L609-L641) and the fan-out in Hub.record (server.py#L939-L944, fan-out loop at L1001-L1010) run inside self.cond, so there's no TOCTOU window between eligibility check and write. Re-checked: alt (the round id) is stored on the event but not used to filter items — a token holder could name items from a different round in the same accept-audit call. This is the same non-finding the prior review noted: it's a data-integrity nuance, not a privilege escalation, since a plain per-item accept already grants that same capability to any authenticated caller (single-token, single-user local tool).
  • event.schema.json — items[] is additionalProperties: false with required id/contentRev; matches the server-side validation, no schema/server drift.
  • Round-scoping fix (the earlier Codex P1) — confirmed present at this head: audit-answers/SKILL.md's Preconditions section and context/surface.md's routing row both instruct /planning:audit-answers to treat the caller-named items as the entire answer set, leaving other branches open. This is an instruction-level control, consistent with how this plugin already works, not a code-enforced one.
  • No instruction-surface deletions in this diff (only additive rows/sentences in SKILL.md and surface.md).

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 4m 51s —— View job


Code review (/review:code-review)

  • Gather context (PR body, diff via gh pr diff 5536, review skill)
  • Invoke /review:code-review and follow it exactly
  • Review changed files against the lane's criteria
  • Post findings

Findings

No blocking correctness or maintainability issues found in this pass over the current diff (gh pr diff 5536, all 14 changed files). This is an independent pass, not a restatement of the two prior automated reviews already on this PR.

What I checked, beyond re-confirming the previously-flagged Codex P1 (round-scoping) is fixed on this HEAD in both context/surface.md:61 and SKILL.md:39:

  • server.py — check_accept_audit validates shape before the lock (malformed bodies write nothing); split_accept_audit's eligibility precedence (changed checked before ineligible) matches TestAcceptAudit.test_6; the same-batch-prerequisite-doesn't-count behavior matches test_4; the accept-audit event and its fanned-out accept events are built and appended inside the single self.cond lock, so there's no TOCTOU window and no seq collision between the batch's own accepts.
  • exporters.py pending_validation (line 475) — keys off rec["seq"] matching an event carrying auditSeq. Confirmed held_terminal() (exporters.py:428) never sets a seq key, so a terminal/imported answer reliably falls through to "reads as its own" rather than risking a false match, consistent with test_a_later_decision_or_terminal_answer_reads_as_its_own.
  • index.html acceptAllDialog(pool, where, audit) — the audit path holds out any question carrying any note (carried(q)), not just a "Challenge:" note, and sends no text for any item, matching "the event carries no notes"; dlgOk is disabled when every eligible item is held out, so an all-noted round can't post an empty items array.
  • event.schema.json / test_schema.py — items is additionalProperties: false with required id/contentRev, matching the server's independent re-validation.

Verification

Static review only — no test runner was available in this pass, so I did not execute test_server.py / test_exporters.py / test_schema.py or the shell suites. The PR body's pass counts, and the verification claims in the two prior automated review comments on this PR, are author/tool-claimed and were not independently re-run by me here.

Branch: feat/5472-accept-all-audit

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review — /review:security-review

  • Gather context (PR diff, body, review comments)
  • Run /review:security-review for this PR (CI security lane)
  • Post findings

No security findings.

Skip-gate check: this head (3e26444) already has a completed security review on this PR, run against 9a03178 (2026-09-30T05:27:20Z). I diffed 9a03178..3e26444 restricted to the PR's 14 changed files to confirm it still applies:

  • plugins/planning/surface/server.py, exporters.py, index.html, schema/event.schema.json, skills/interview/context/surface.md, README.md, .claude-plugin/plugin.json, and all four test files — byte-identical in that range. The accept-audit code path (check_accept_audit, split_accept_audit, the lock-held fan-out in Hub.record) that the prior review examined is unchanged.
  • plugins/planning/skills/audit-answers/SKILL.md — the one substantive change: the existing "when the caller names the question ids to audit... do not fill or validate any other open branch" sentence moved from under the ### Step 1 heading to under ## Preconditions (current: skills/audit-answers/SKILL.md#L47). Same text, no wording change — this is the fix kyle-sexton's comment described for the earlier Codex P1 (round-scoping), which the prior review had already verified was addressed. Purely an instruction-surface relocation, not a deletion or narrowing, so the instruction-surface-deletion lens doesn't apply.
  • plugins/planning/tests/interview-defenses.test.sh — two new pin assertions pinning that exact surface.md routing sentence and the accept-audit table row (current: tests/interview-defenses.test.sh#L869-L872), test-only, no security surface.
  • plugins/planning/CHANGELOG.md — an added 0.48.0 entry plus unrelated 0.47.9/0.47.10 entries pulled in by merging origin/main; no code.

No new security-relevant surface since the prior clean review. Nothing to add to that review's conclusions (auth/trust boundary unchanged, fan-out executes inside the lock so no TOCTOU, schema rejects malformed payloads server-side, index.html renders via existing esc()).

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

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review (/review:code-review)

  • Gather context (PR body, diff via gh pr diff, review skill)
  • Invoke /review:code-review and follow it exactly
  • Review changed files against the lane's criteria
  • Post findings

Skip gate: this head already has a covering review

This lane already posted a clean code review on this PR at
2026-09-30T06:19:55Z
against the branch tip at that time (3e26444). I diffed 3e26444..12d48ef (the current HEAD)
restricted to this PR's 14 changed files to check whether that review still applies:

plugins/planning/CHANGELOG.md                          |  6 ++++++
plugins/planning/skills/interview/context/surface.md   | 16 ++++++++--------
plugins/planning/surface/README.md                     |  5 +++--
3 files changed, 17 insertions(+), 10 deletions(-)

The other 11 changed files, including the accept-audit implementation
(server.py, exporters.py, index.html, schema/event.schema.json,
skills/audit-answers/SKILL.md) and all four test files, are byte-identical
in that range.

The three files that did change are a merge-in of an unrelated main change
(wake.sh, #5462) plus its CHANGELOG entry:

  • surface.md — every changed line is inside the "wake" section documenting
    the new wake.sh wrapper for the watch loop; the accept-audit event-table
    row and its routing sentence
    (surface.md#L133)
    are untouched by this range.
  • README.md — same wake.sh documentation, no accept-audit surface.
  • CHANGELOG.md — adds the 0.47.11 (wake.sh) entry pulled in from main;
    the 0.48.0 (accept-audit) entry already reviewed is unchanged.

No new review surface for this lane since the last clean pass. Nothing to add
to that review's findings (validation ordering in check_accept_audit/
split_accept_audit, the lock-held fan-out, pending_validation's seq
keying, the round-scoping fix in audit-answers/SKILL.md's Preconditions and
surface.md's routing row — all previously verified and unchanged here).

Branch: feat/5472-accept-all-audit

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review — /review:security-review

  • Gather context (PR diff, body, review comments)
  • Run /review:security-review for this PR (CI security lane)
  • Post findings

No security findings.

Skip-gate check: this head (12d48ef2) already has a completed, clean security review on this PR, run against 3e26444 (2026-09-30T06:21:47Z), which itself confirmed no security-relevant drift back to 9a03178 (the first clean review). I diffed 3e26444..12d48ef, restricted to this PR's 14 changed files, to confirm it still applies:

  • plugins/planning/surface/server.py, exporters.py, index.html, schema/event.schema.json, skills/audit-answers/SKILL.md, all four test files, and .claude-plugin/plugin.json — byte-identical in that range. The accept-audit code path (check_accept_audit, split_accept_audit, the lock-held fan-out in Hub.record) that the prior reviews examined is unchanged.
  • plugins/planning/skills/interview/context/surface.md — current: L45-L118 changed, but the diff is entirely origin/main merge content from an unrelated PR (docs(planning): add wake.sh, a short wrapper for the interview wake command #5489, wake.sh): it collapses the two-command wake (round.sh apply && watch.sh) into one wake.sh wrapper and updates the permission-rule wording to match. It does not touch the accept-audit event row or the audit-routing sentence this PR added — those are unchanged (still at the same "route the event's items only" instruction confirmed clean in the prior review). Not an instruction-surface deletion: the &&-stops-on-refusal guarantee and the permission-rule guidance are restated for the new script, not removed or narrowed.
  • plugins/planning/surface/README.md — same wake.sh merge content, doc-only, no security surface.
  • plugins/planning/CHANGELOG.md — an added 0.47.11 entry (the wake.sh PR) pulled in by merging origin/main; no code.

No new security-relevant surface since the prior clean reviews. Nothing to add to their conclusions (auth/trust boundary unchanged, fan-out executes inside the lock so no TOCTOU, schema rejects malformed payloads server-side, index.html renders via existing esc(), round-scoping fix present in SKILL.md Preconditions).

Branch: feat/5472-accept-all-audit ·

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.

feat(planning): accept-all-and-check action on the interview page routed to audit-answers

1 participant