CRU-133: recheck round-trip guidance in persona prompt, launch response, tool descriptions - #386
Merged
Merged
Conversation
… launch response, and tool descriptions (CRU-133) Wires the opt-in review round-trip through the three text carriers that agents actually see: - Persona prompt: append a round-trip guidance block to assemblePersonaPrompt when the review is launched with allowRecheck. - dispatch_launch_persona response: when allowRecheck is true, return a block that tells the parent how to drive the loop (get/resolve feedback, submit resolution, poll for round 2). - Tool descriptions: rewrite dispatch_launch_persona, dispatch_resolve_feedback, dispatch_submit_resolution, and dispatch_await_recheck so each carries the constraints the server enforces and tells the agent what to do next. - Rejection messages: update complete_review and submit_resolution errors so a fresh agent can self-correct on the next turn. No backend behavior changes beyond text — state machine and poll logic remain as landed in CRU-131/CRU-132. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address two items from backend-security-review: - dispatch_launch_persona response (allowRecheck=true): replace "poll dispatch_get_feedback until the reviewer's round-2 verdict is complete" — dispatch_get_feedback only returns agent_feedback rows, not persona_review.verdict/status, so a round-2 approve-with-no-new- items would look indistinguishable from "still running." New text describes what the API actually surfaces: watch for items linked via responds_to_feedback_id, and treat "nothing new after a reasonable window" as an approval signal. - dispatch_resolve_feedback description: the "the reviewer will see it in a recheck pass" clause was unconditional, but the tool is also used by parents whose reviews were launched with allowRecheck=false (the default), where no recheck happens. Qualify the clause. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…hints (CRU-133) Close the test-coverage gaps identified in the CRU-133 self-review: - Extract the dispatch_launch_persona response-text assembly into a pure `buildLaunchPersonaResponseText` exported from shared/mcp/server.ts. The handler now delegates to it, so the allowRecheck branching is unit-testable without spinning up the MCP server. New tests in test/mcp-launch-persona-response.test.ts cover: base-only when allowRecheck is false, full driver-loop block when true, required tool references (dispatch_get_feedback, dispatch_resolve_feedback, dispatch_submit_resolution, responds_to_feedback_id), and the "no verdict signal" regression guard for the fix in ee45e96. - Strengthen the open-items and ignored-missing-reason rejection tests in both agent-manager.test.ts and resolution-capture-integration.test.ts to assert the new recovery hint text ("Call dispatch_resolve_feedback", status names, and the reason-explanation phrase). The existing regexes only proved IDs were named. - Lock the ticket-spec copy for the third-completion rejection in agent-manager.test.ts — toBe the exact string, not just a substring, so the rejection message itself (which is guidance) can't drift. No production behavior change beyond the pure-function refactor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dispatch_get_feedback (CRU-133) Live dogfood against the dev stack surfaced a real bug in the CRU-133 text: the persona prompt and the launch-response guidance both tell agents to link round-2 follow-up findings to the originals using `responds_to_feedback_id`, but the MCP contract had no such field. - `dispatch_feedback` zod schema had no `respondsToFeedbackId` input, so reviewers couldn't actually set the linkage — the instruction was inert. - `FeedbackItem` (the type surfaced by `dispatch_get_feedback`) omitted the field, so parents couldn't read it either. The DB column `responds_to_feedback_id` existed and the SELECTs already aliased it to `respondsToFeedbackId`, but neither type exported that name. - Prompt and launch-response used snake_case (`responds_to_feedback_id`) while the rest of the MCP surface is camelCase (`feedbackId`, `personaAgentId`, `mediaRef`). End-to-end fix: - Add `respondsToFeedbackId?: number` to `FeedbackInput` (both `manager.ts` and `mcp/server.ts`). - Add `respondsToFeedbackId: number | null` to `FeedbackItem`. - Add the zod parameter to `dispatch_feedback` and thread it through to `AgentManager.submitFeedback`, which now persists it via the existing DB column. - Update persona prompt and launch-response copy to camelCase `respondsToFeedbackId`, and expand the persona-side explanation so a reviewer understands *why* the link matters (parent can map round-2 follow-ups back to round-1 concerns). - New regression test: `submitFeedback` persists `respondsToFeedbackId` on a round-2 finding. Existing tests updated for the camelCase form. Found via live-dogfood review against the dev stack running this branch (feedback items #1 and #2 from agt_35cac0176460). Co-Authored-By: gpt-5.4 review agent <noreply@anthropic.com>
…recheck round-trip (CRU-133) Live dogfood against the dev stack exposed five concrete gaps in the recheck round-trip that the text-only CRU-133 guidance couldn't close on its own. Fixing all five here so the round-trip can actually run end-to-end without manual intervention. 1. **Parent-side status-watch primitive** — new `dispatch_await_review` MCP tool (parent-only, AGENT_TOOLS) and `AgentManager.awaitReview`. Mirrors `dispatch_await_recheck` for the opposite side of the round-trip. Returns 'pending' with `pollAgainInSeconds` while the reviewer works, 'feedback_ready' on round-1 close for an allowRecheck review, 'complete' when the final round is done (or when a single-pass review closes), or 'cancelled'. Uses the same `pollCadenceSeconds` helper as the reviewer side for consistency, but doesn't auto-cancel (that's owned by the reviewer flow). 2. **Launch-response guidance rewrite** — `buildLaunchPersonaResponseText` now produces a survival kit mirroring the reviewer block: explicit "do not emit a terminal dispatch_event yet," explicit `ScheduleWakeup` mechanism, numbered steps pointing at `dispatch_await_review` instead of polling `dispatch_get_feedback` (which was the original bug — it fires on the first item that lands and misleads agents into thinking the review is done). Also adds an explicit step 3: commit fixes before `dispatch_submit_resolution`, otherwise the reviewer's round-2 diff is empty. 3. **`dispatch_event` added to PERSONA_TOOLS** — standard guidance told personas to emit dispatch events but the tool wasn't exposed, so the first call always errored. Fixed. 4. **Consolidated completion paths** — removed `status: "complete"` from `review_status`'s enum. Completion now goes exclusively through `dispatch_complete_review`. Standard guidance updated to match. The dogfood reviewer called both and the second was rejected with a state-machine error that fresh agents can't self-correct from. 5. **Reviewer round-2 obligation sharpened** — `RECHECK_ROUND_TRIP_GUIDANCE` is now explicit that round 2 has a mandatory close via `dispatch_complete_review`, and that the terminal `dispatch_event` must wait until then. The dogfood reviewer woke from its recheck but exited without submitting a round-2 verdict, leaving the review permanently in `awaiting_recheck`. 6. **`dispatch_submit_resolution` description** — added the commit-before-submit warning so it's visible from the tool surface, not just the launch-response text. Tests: - New `awaitReview` manager describe block covering all five status transitions (pending/reviewing, feedback_ready, complete single-pass, pending/awaiting_recheck, complete round-2, cancelled, and the parent-mismatch / missing-review error paths). - `mcp-launch-persona-response.test.ts` rewritten for the new text: survival-kit sleep mechanism, status-watch pattern, commit-before- submit clause, and regression guards against the old "poll feedback items" pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… Claude-specific sleep hints (CRU-133) Two fixes from user feedback on the first survival-kit pass: 1. **`dispatch_await_review` now supports multi-reviewer parents.** `personaAgentId` is optional; omit it and the server returns whichever of the caller's launched reviewers most needs attention (priority: feedback_ready > cancelled > complete > pending; ties broken by most recently updated). Every non-`no_reviews` response now includes the `review` it refers to so the parent knows which one changed — previously the pending branch returned bare `reviewStatus`/`roundNumber` with no review ID, which was useless when several reviews were in flight. Adds a new `no_reviews` terminal status for parents that haven't launched anything. 2. **Remove Claude-Code-specific sleep hints.** The guidance text (launch response, reviewer persona prompt, dispatch_await_recheck / dispatch_await_review descriptions) previously named `ScheduleWakeup` as the sleep mechanism. Dispatch runs claude / codex / opencode — that's only correct for the Claude path. Now the text says "wait N seconds using whatever sleep mechanism your agent runtime provides" and each runtime can figure out its own primitive. The `pollAgainInSeconds` contract is unchanged; only the prose around it is now runtime-neutral. Tests: existing awaitReview tests updated for the new response shape (review now present on pending/cancelled); new coverage for the null-personaAgentId branches (no_reviews on empty, feedback_ready preference over in-progress, pending when all are reviewing). New assertion in mcp-launch-persona-response.test.ts that the text does NOT contain "ScheduleWakeup" and does describe the sleep mechanism in agent-runtime-neutral terms. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- dispatch_await_review: handle feedbackCount=0 on a round-1 allowRecheck approve so the parent is told to submit_resolution/cancel_recheck instead of looping on an empty dispatch_get_feedback. - awaitReview multi-review path: fold the feedback COUNT into the primary SELECT via a correlated subquery and let classifyReviewForAwait accept a prefetched count, eliminating the N+1 COUNT queries. - submitFeedback: validate respondsToFeedbackId within the existing transaction — reject 400 when unknown, cross-review, or pointing at a round-2 item so the linkage invariant is enforced server-side.
…ound 2 (CRU-133)
End-to-end dogfood on the dev stack surfaced a state-machine bug: the
reviewer agent pinged review_status('reviewing', 'round 2 begin') per
the standard progress-ping guidance while it was actually in round 2.
updatePersonaReviewStatus unconditionally SET status = 'reviewing',
which downgraded the review out of 'awaiting_recheck' back to
'reviewing'. The next dispatch_complete_review call then landed in the
`status === 'reviewing'` branch of completePersonaReview, which sets
nextRoundNumber = 1 — so round_number stuck at 1 even though the
reviewer had just finished round 2. Downstream, dispatch_await_review
saw complete + round_number=1 + allowRecheck=true and would have
returned feedback_ready a second time, sending the parent into a loop
over already-fixed findings.
Two complementary fixes:
1. updatePersonaReviewStatus now leaves the status field untouched
when the review is in a non-working state (complete, cancelled, or
awaiting_recheck). The progress-ping channel only updates the
message. This matches the intent: a reviewer pinging "still
working" during round 2 shouldn't look identical to the round-1
start.
2. completePersonaReview is defensively tolerant of a 'reviewing'
status that sits on top of an existing round-1 resolution. It
infers the next round number from the most recent submitted
resolution rather than trusting the status label alone. If any
future path manages to put the review back into 'reviewing' after
a resolution, the completion still lands as round 2.
Tests: regression covering the review_status clobber (status stays
awaiting_recheck, subsequent complete lands as round 2) and the
defense-in-depth path (forcibly setting status='reviewing' after a
resolution still yields round_number=2 on the next complete).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…QL (CRU-133) PR #386 review comment called out the CASE WHEN in updatePersonaReviewStatus: logic in SQL is hard to unit-test and can't produce contextual errors. Fair — pulled it into TypeScript. - New exported pure helper `resolveProgressPingStatus(requested)` validates the progress-ping status at the tool boundary. Unit-tested in its own file (no DB). Throws a 400 AgentError with the valid-set listed for any bad value. - `updatePersonaReviewStatus` now opens a transaction, SELECTs the current status FOR UPDATE, then branches in code: - `complete` / `cancelled`: rejected with a specific 409 — "Cannot ping review_status on a <status> review" — agents get a clear signal that pings on terminal reviews aren't allowed, instead of a silent no-op. - `awaiting_recheck`: preserves the status label (reviewer is still doing work; round-2 round-trip must stay intact) and just refreshes the message. - any other status: transitions to `reviewing` as before. Tests: two new integration tests that exercise the terminal-state 409 paths (complete and cancelled), plus a dedicated unit-test file for the pure helper. The existing round-2 regression test still passes — the awaiting_recheck preservation moved from SQL to code without changing behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wires the opt-in review round-trip (CRU-127) through the three text carriers that agents actually see at runtime. No backend behavior changes beyond the text — the state machine and poll logic landed in CRU-131/CRU-132 (PRs #381, #382).
assemblePersonaPromptnow accepts anoptions.allowRecheckflag. When the launching review hasallowRecheck: true, the assembled prompt includes a "Recheck round-trip" block that tells the reviewer to stay alive, calldispatch_await_recheck, handle thepending/ready/cancelledbranches, and submit a round‑2 verdict. Absent otherwise. The launch path inserver.tspasses the flag through.dispatch_launch_personaresponse: whenallowRecheckis true, the tool result now includes a parent‑facing guidance block explaining the full driver loop (read feedback → resolve each → submit resolution → poll for round‑2 verdict). Present only when opt‑in.dispatch_launch_persona,dispatch_resolve_feedback,dispatch_submit_resolution, anddispatch_await_recheckdescriptions tightened to carry the constraints the server enforces and tell the agent what to do next.submit_resolutionerrors for open items / missing ignore reasons now name thefeedbackIds and point atdispatch_resolve_feedback. The three "round 2 already" paths (complete_review × 2, submit_resolution × 1) now share the ticket-specified copy: "Round 2 already complete. This review only supports a single round-trip."Acceptance criteria from the ticket:
allowRecheck: true(new tests inpersona-loader.test.ts).dispatch_launch_personaresponse includes the parent guidance block only whenallowRecheck: true.pnpm --filter @dispatch/server checkpasses. Full server test suite (394 tests) passes. Web tsc failures are pre-existing onmain(react-day-picker,@radix-ui/react-popover, implicitanyinactivity-pane.tsx:868) and unrelated to this change.Test plan
allowRecheck=true, inspect the generated reviewer--append-system-prompt— "Recheck round-trip" block is present and mentionsdispatch_await_recheck,pollAgainInSeconds,responds_to_feedback_id.allowRecheck— block is absent.dispatch_submit_resolutionwith an open feedback item — rejection names thefeedbackIdand points atdispatch_resolve_feedback.dispatch_complete_reviewa third time — rejection uses the new "Round 2 already complete." copy.🤖 Generated with Claude Code