diff --git a/apps/board/AGENTS.md b/apps/board/AGENTS.md index 3238a1e31d..710ccfdcae 100644 --- a/apps/board/AGENTS.md +++ b/apps/board/AGENTS.md @@ -47,8 +47,12 @@ resolves run to MR in the background; the request path never waits on it). (review first round, review re-review, respond plan, respond post), each rebuilt from a real gate's shape with invented data (`gate-stories.fixtures.ts`). Keep it to gates a real run asks; a variant -belongs in a DOM test, not a new story. Invented names only (the repo is -public). +belongs in a DOM test, not a new story. The one exception is the +`Review question: …` stories, one per state of asking the review pane about +a finding (the ask link, the box open, sending, asked, a 409, the pane +gone, a parked gate, the revised strip), kept so that UI can be worked on +in Storybook; their play steps click into each state. Invented names only +(the repo is public). ## Respond rounds diff --git a/apps/board/docs/api.md b/apps/board/docs/api.md index 4f749fee24..8c9b74b49a 100644 --- a/apps/board/docs/api.md +++ b/apps/board/docs/api.md @@ -33,12 +33,13 @@ board is not peering at all. ## Launching agents (local-only) -| endpoint | body | -| --------------- | ----------------------------------------------------------------------------------- | -| `POST /review` | `{ mrUrl, iid }`. `400` when `reviewCwd` is unset | -| `POST /respond` | same shape, for your own MR | -| `POST /doctor` | same shape, to repair mechanical breakage | -| `POST /drafts` | approve or dismiss a held doctor draft. The only path from a draft to a GitLab note | +| endpoint | body | +| ------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `POST /review` | `{ mrUrl, iid }`. `400` when `reviewCwd` is unset | +| `POST /respond` | same shape, for your own MR | +| `POST /doctor` | same shape, to repair mechanical breakage | +| `POST /drafts` | approve or dismiss a held doctor draft. The only path from a draft to a GitLab note | +| `POST /review/finding-question` | `{ mrUrl, iid, gateId, findingId, question, tabId? }`. Types `/board:review-finding-question ""` into the pane running the review's Claude session (a pane at its gate form gets one Escape, then the line again) and focuses it. On a parked gate with no pane running the session, resumes the review with the line as its prompt instead. `400` for a finding id outside `[A-Za-z0-9._:-]` or an empty question or one over 2000 characters; `409` when the gate is not the MR's open or parked review gate, the gate does not list the finding, no pane runs the session of an open gate, the resume tab is already open, or the pane will not take the line | A launched agent's lifecycle status never arrives over HTTP. The status CLIs publish it on the rt daemon's event bus (`board/agent-status/`) and the diff --git a/apps/board/skills-src/review-finding-question/SKILL.md b/apps/board/skills-src/review-finding-question/SKILL.md new file mode 100644 index 0000000000..75bb5cac86 --- /dev/null +++ b/apps/board/skills-src/review-finding-question/SKILL.md @@ -0,0 +1,140 @@ +--- +name: board:review-finding-question +description: >- + Use when the human asks about one finding of the review this session wrote, + invoked in a board review pane as "/board:review-finding-question + """ while the session holds at its review-post gate. + Not for any other session. +disable-model-invocation: true +--- + +# Review finding question + +The human is deciding on your review gate and has a question about one +finding: is it real, is it overstated, what does the data say. You answer +with evidence, then they choose to revise the finding or leave it. They +finish the review on the board, never in this pane. + +## Arguments + +` ""`, commas between them optional. The +gate is the review-post gate this session opened; the finding is an `id` in +your report json (the `.json` sibling of your `--report`). Your `--state` +and `--status-bin` are the ones this session was launched with. + +This skill runs only inside the board review session that wrote that +report. If this session has no `/board:review` launch, or the finding id is +not in the report json, say so in one line and stop. + +The line can also arrive as the first message of a resume: a gate the board +parked after a long wait closed this pane, and a question about one of its +findings resumes the session with it. The gate is then `parked`, not `open`, +and no wait loop is running; everything below holds the same. + +## The rule for the whole conversation + +Until the human says to revise, you change nothing: no edit to the report, +no gate opened or answered, no post to the MR, no commit, no database +write. Databases only through rt's sdm verbs (the `rt:sdm-connect` skill), +only `*-read-only` resources, QA unless the human names prod in this +conversation. Posting and the verdict are the human's, on the board. + +## 1. Restate, then explore through a helper + +Restate in two lines: the finding (tier, title, `file:line`) and the +question. Then dispatch one explorer with the Agent tool +(`subagent_type: Explore`, which has no Edit or Write, so read-only is +enforced; `model: sonnet`, `effort: medium`) and wait for it. Its brief is: + +- the finding, verbatim from the report json, and the MR url; +- the human's question, verbatim; +- what it may use: the code (`mr_view`, `gitlab_get`, or a worktree + through the `rt:worktree` skill, disposed of before it returns), logs, + New Relic, and QA databases through rt's sdm verbs on `*-read-only` + resources; prod only when the human named it; +- that it changes nothing anywhere and returns: what it checked (each query + or search, verbatim), what it found, what it could not check, and its + verdict. + +Each follow-up question that needs new digging gets its own explorer with +the same brief and the new question. A question you can answer from what +you already hold, answer directly. + +## 2. Report in this shape, then ask for the outcome as a form + +``` +Verdict: holds | narrower than written | broader than written | does not hold +Evidence: +- → (one line each, queries verbatim) +Not checked: +Proposed change: +``` + +**REQUIRED SUB-SKILL:** Use mattstack:wrap-up to ask for the outcome. The +form has one Decisions question, "What should happen to ?", +with exactly these options, the one matching your verdict first and +labelled `(Recommended)`: + +- **Revise and re-raise the gate**: apply the proposed change. +- **Leave it as is**: the finding stands. +- **Keep asking**: the human has more to ask. + +Recommend "Revise" when the proposed change is not "none", otherwise +"Leave it as is". Act on the answer: §3, §4, or wait for the next +question. A typed reply instead of a pick is a follow-up question: answer +it, report again in the same shape, and ask again. + +## 3. Revise (only on the human's go-ahead) + +1. Show the revised finding next to the original: tier, anchor, body, + comment. Keep its `id` and every field you were not asked to change. + For "drop", say so. Then ask with mattstack:wrap-up: one Decisions + question, "Apply this revision to ?", options **Apply it + (Recommended)**, **Change the wording**, **Cancel**. Apply only on + "Apply it"; "Change the wording" takes their edit and asks again; + "Cancel" goes to §4. +2. Edit the finding in the report json and the report markdown, keeping + them in step (tier counts and summary lines that name it included). +3. Rebuild the gate's open file exactly as this session built the first + one (the same scripts, extras and arguments). The old gate and its wait + loop stay as they are until this succeeds: if it fails, say what + failed, restore nothing by hand, and keep holding at the old gate. +4. Check the old gate is still waiting on the human: + ` gate wait --max-ms 1000`. Only `{"status": + "pending"}` goes on. An `answers` line means the human already decided + on the board: re-raise nothing, tell them the review was answered + before the revision landed and that it is in the report only, then + carry on with that answer as the wrapper's gate step does (a running + wait loop delivers the same answer: act on it once). A failure (the + gate closed) is the same: re-raise nothing and say so. +5. If a background wait loop is running on this gate, stop it with + TaskStop (load it with ToolSearch when it is deferred). +6. Run the same `open-gate.sh review-post + `. The new gate supersedes the old one, a parked one too. +7. Start one background ` gate wait --max-ms 90000` + loop on the new gate and end the turn holding at it. Do not show a pane + form: the human finishes on the board. +8. Tell the human: the finding is revised, the gate is re-raised, finish + the review on the board. + +A `closed (superseded)` result for the old gate id later is your own +re-raise: ignore it and keep holding at the new gate. If the open fails, +say what failed, restore nothing by hand, and leave the old gate's state as +the wrapper's gate step would. + +## 4. Leave it as is + +Change nothing. Tell the human the finding stands and to finish on the +board. If this gate's wait loop is no longer running, start one, then end +the turn holding at the gate. + +## Red flags + +| Thought | Do instead | +|---|---| +| "I'll just grep and query myself" | Dispatch the explorer; your context holds the review. | +| "Prod read-only would settle it" | Offer it; run it only when the human names prod. | +| "They want it gone, I'll answer the gate as comment" | Revise, re-raise, send them to the board. | +| "The old gate closed, the run is over" | That close is your re-raise. Hold at the new gate. | +| "The revision is ready, re-raise it now" | Check the old gate is still pending first; a late re-raise reopens a posted review. | +| "I'll list the options in text, it's quicker" | The outcome is a mattstack:wrap-up form; the pick is the record. | diff --git a/apps/board/skills/review-finding-question/SKILL.md b/apps/board/skills/review-finding-question/SKILL.md new file mode 100644 index 0000000000..fc76a3f902 --- /dev/null +++ b/apps/board/skills/review-finding-question/SKILL.md @@ -0,0 +1,143 @@ +--- +name: board:review-finding-question +description: >- + Use when the human asks about one finding of the review this session wrote, + invoked in a board review pane as "/board:review-finding-question + """ while the session holds at its review-post gate. + Not for any other session. +disable-model-invocation: true +--- + + + + +# Review finding question + +The human is deciding on your review gate and has a question about one +finding: is it real, is it overstated, what does the data say. You answer +with evidence, then they choose to revise the finding or leave it. They +finish the review on the board, never in this pane. + +## Arguments + +` ""`, commas between them optional. The +gate is the review-post gate this session opened; the finding is an `id` in +your report json (the `.json` sibling of your `--report`). Your `--state` +and `--status-bin` are the ones this session was launched with. + +This skill runs only inside the board review session that wrote that +report. If this session has no `/board:review` launch, or the finding id is +not in the report json, say so in one line and stop. + +The line can also arrive as the first message of a resume: a gate the board +parked after a long wait closed this pane, and a question about one of its +findings resumes the session with it. The gate is then `parked`, not `open`, +and no wait loop is running; everything below holds the same. + +## The rule for the whole conversation + +Until the human says to revise, you change nothing: no edit to the report, +no gate opened or answered, no post to the MR, no commit, no database +write. Databases only through rt's sdm verbs (the `rt:sdm-connect` skill), +only `*-read-only` resources, QA unless the human names prod in this +conversation. Posting and the verdict are the human's, on the board. + +## 1. Restate, then explore through a helper + +Restate in two lines: the finding (tier, title, `file:line`) and the +question. Then dispatch one explorer with the Agent tool +(`subagent_type: Explore`, which has no Edit or Write, so read-only is +enforced; `model: sonnet`, `effort: medium`) and wait for it. Its brief is: + +- the finding, verbatim from the report json, and the MR url; +- the human's question, verbatim; +- what it may use: the code (`mr_view`, `gitlab_get`, or a worktree + through the `rt:worktree` skill, disposed of before it returns), logs, + New Relic, and QA databases through rt's sdm verbs on `*-read-only` + resources; prod only when the human named it; +- that it changes nothing anywhere and returns: what it checked (each query + or search, verbatim), what it found, what it could not check, and its + verdict. + +Each follow-up question that needs new digging gets its own explorer with +the same brief and the new question. A question you can answer from what +you already hold, answer directly. + +## 2. Report in this shape, then ask for the outcome as a form + +``` +Verdict: holds | narrower than written | broader than written | does not hold +Evidence: +- → (one line each, queries verbatim) +Not checked: +Proposed change: +``` + +**REQUIRED SUB-SKILL:** Use mattstack:wrap-up to ask for the outcome. The +form has one Decisions question, "What should happen to ?", +with exactly these options, the one matching your verdict first and +labelled `(Recommended)`: + +- **Revise and re-raise the gate**: apply the proposed change. +- **Leave it as is**: the finding stands. +- **Keep asking**: the human has more to ask. + +Recommend "Revise" when the proposed change is not "none", otherwise +"Leave it as is". Act on the answer: §3, §4, or wait for the next +question. A typed reply instead of a pick is a follow-up question: answer +it, report again in the same shape, and ask again. + +## 3. Revise (only on the human's go-ahead) + +1. Show the revised finding next to the original: tier, anchor, body, + comment. Keep its `id` and every field you were not asked to change. + For "drop", say so. Then ask with mattstack:wrap-up: one Decisions + question, "Apply this revision to ?", options **Apply it + (Recommended)**, **Change the wording**, **Cancel**. Apply only on + "Apply it"; "Change the wording" takes their edit and asks again; + "Cancel" goes to §4. +2. Edit the finding in the report json and the report markdown, keeping + them in step (tier counts and summary lines that name it included). +3. Rebuild the gate's open file exactly as this session built the first + one (the same scripts, extras and arguments). The old gate and its wait + loop stay as they are until this succeeds: if it fails, say what + failed, restore nothing by hand, and keep holding at the old gate. +4. Check the old gate is still waiting on the human: + ` gate wait --max-ms 1000`. Only `{"status": + "pending"}` goes on. An `answers` line means the human already decided + on the board: re-raise nothing, tell them the review was answered + before the revision landed and that it is in the report only, then + carry on with that answer as the wrapper's gate step does (a running + wait loop delivers the same answer: act on it once). A failure (the + gate closed) is the same: re-raise nothing and say so. +5. If a background wait loop is running on this gate, stop it with + TaskStop (load it with ToolSearch when it is deferred). +6. Run the same `open-gate.sh review-post + `. The new gate supersedes the old one, a parked one too. +7. Start one background ` gate wait --max-ms 90000` + loop on the new gate and end the turn holding at it. Do not show a pane + form: the human finishes on the board. +8. Tell the human: the finding is revised, the gate is re-raised, finish + the review on the board. + +A `closed (superseded)` result for the old gate id later is your own +re-raise: ignore it and keep holding at the new gate. If the open fails, +say what failed, restore nothing by hand, and leave the old gate's state as +the wrapper's gate step would. + +## 4. Leave it as is + +Change nothing. Tell the human the finding stands and to finish on the +board. If this gate's wait loop is no longer running, start one, then end +the turn holding at the gate. + +## Red flags + +| Thought | Do instead | +|---|---| +| "I'll just grep and query myself" | Dispatch the explorer; your context holds the review. | +| "Prod read-only would settle it" | Offer it; run it only when the human names prod. | +| "They want it gone, I'll answer the gate as comment" | Revise, re-raise, send them to the board. | +| "The old gate closed, the run is over" | That close is your re-raise. Hold at the new gate. | +| "The revision is ready, re-raise it now" | Check the old gate is still pending first; a late re-raise reopens a posted review. | +| "I'll list the options in text, it's quicker" | The outcome is a mattstack:wrap-up form; the pick is the record. | diff --git a/apps/board/skills/review-finding-question/compiled.json b/apps/board/skills/review-finding-question/compiled.json new file mode 100644 index 0000000000..c1055692a3 --- /dev/null +++ b/apps/board/skills/review-finding-question/compiled.json @@ -0,0 +1,8 @@ +{ + "id": "skills:board:review-finding-question", + "dir": "skills/review-finding-question", + "covers": [ + "skills:board:review-finding-question" + ], + "keys": {} +} diff --git a/apps/board/src/__tests__/finding-question.test.ts b/apps/board/src/__tests__/finding-question.test.ts new file mode 100644 index 0000000000..837c8fdcfa --- /dev/null +++ b/apps/board/src/__tests__/finding-question.test.ts @@ -0,0 +1,482 @@ +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { describe, expect, test } from 'bun:test'; + +import type { + GateRow as FacilityGateRow, + PaneSendResult, +} from '@mattstack/rt-client'; +import { + ESCAPE_SETTLE_MS, + findingQuestionText, + handleFindingQuestion, + NO_REVIEW_PANE, + PANE_AT_OTHER_PROMPT, + quoteQuestion, + REVIEW_RESUMING, + type FindingQuestionIo, +} from '../finding-question.ts'; + +const MR = 'https://gitlab.example.com/acme/widgets/-/merge_requests/31'; + +// Screens captured from real panes, shared with the daemon's detector tests. +const daemonFixture = (name: string) => + readFileSync( + join( + import.meta.dir, + '..', + '..', + '..', + '..', + 'lib', + 'daemon', + '__tests__', + 'fixtures', + name + ), + 'utf8' + ); +const FORM_SCREEN = daemonFixture('question-form-single.txt'); +const PERMISSION_SCREEN = [ + '⏺ Bash(rm -rf build)', + '', + ' Do you want to proceed?', + ' ❯ 1. Yes', + ' 2. No, and tell Claude what to do differently (esc)', + '', +].join('\n'); + +function gateRow(over: Partial = {}): FacilityGateRow { + return { + id: 'g-review', + subject: `mr:${MR}`, + kind: 'review-post', + questions: [ + { + id: 'findings-1', + label: 'Post which findings?', + multi: true, + options: [ + { value: 'f1', label: 'one' }, + { value: 'f2', label: 'two' }, + ], + }, + { + id: 'outcome', + label: 'Verdict', + multi: false, + options: ['approve', 'comment'], + }, + ], + meta: null, + status: 'open', + answer: null, + openedAt: 1, + parkedAt: null, + closedAt: null, + closedReason: null, + agent: null, + pane: null, + nudge: null, + delivery: null, + released: false, + supersededBy: null, + owner: null, + escalatedAt: null, + consumedAt: null, + ...over, + } as FacilityGateRow; +} + +const sent = ( + delivered: PaneSendResult['delivered'], + reason?: string, + paneId = 'w1:p1' +): { ok: true; data: PaneSendResult } => ({ + ok: true, + data: { paneId, delivered, ...(reason ? { reason } : {}) }, +}); + +/** A recording io: `replies` answers each paneSend in turn. */ +function io( + over: Partial = {}, + replies: Array>> = [ + sent('accepted'), + ] +) { + const calls: string[] = []; + const texts: string[] = []; + const sessions: Array = []; + const focused: Array<{ paneId: string; tabId?: string }> = []; + const resumed: Array<{ mrUrl: string; prompt: string }> = []; + const value: FindingQuestionIo = { + reviewGate: () => gateRow(), + reviewPane: () => ({ + tabId: 'w1:t1', + sessionId: 'sess-1', + agentId: 'agent-1', + }), + sessionPane: async () => 'w1:p1', + resumeReview: async (mrUrl, prompt) => { + calls.push('resume'); + resumed.push({ mrUrl, prompt }); + return { focusedExisting: false, paneId: 'w5:p2', tabId: 'w5:t2' }; + }, + paneSend: async ({ paneId, text, selfSession }) => { + calls.push(`send:${paneId}`); + texts.push(text); + sessions.push(selfSession); + return replies.shift() ?? sent('refused', 'no more replies'); + }, + readScreen: async paneId => { + calls.push(`read:${paneId}`); + return FORM_SCREEN; + }, + sendEscape: async paneId => { + calls.push(`escape:${paneId}`); + }, + sleep: async ms => { + calls.push(`sleep:${ms}`); + }, + focusPane: async pane => { + calls.push('focus'); + focused.push(pane); + }, + ...over, + }; + return { io: value, calls, texts, sessions, focused, resumed }; +} + +const body = (over: Record = {}) => ({ + mrUrl: MR, + iid: 31, + gateId: 'g-review', + findingId: 'f2', + question: 'Is this really Important?', + ...over, +}); + +describe('quoting the question', () => { + test('collapses every run of whitespace, newlines included, to one space', () => { + expect(quoteQuestion(' why\n\n is this\tcritical? ')).toBe( + '"why is this critical?"' + ); + }); + + test('escapes double quotes', () => { + expect(quoteQuestion('does "retry" mean the loop?')).toBe( + '"does \\"retry\\" mean the loop?"' + ); + }); + + test('escapes backslashes before quotes, so a trailing one cannot eat the closing quote', () => { + expect(quoteQuestion('is C:\\temp\\')).toBe('"is C:\\\\temp\\\\"'); + expect(quoteQuestion('a \\"b\\" c')).toBe('"a \\\\\\"b\\\\\\" c"'); + }); + + test('the pane line names the skill, the gate and the finding', () => { + expect(findingQuestionText('g-review', 'f2', 'a\nb "c"')).toBe( + '/board:review-finding-question g-review f2 "a b \\"c\\""' + ); + }); +}); + +describe('handleFindingQuestion refusals', () => { + test.each([ + ['a finding id with a space', { findingId: 'f 2' }], + ['a finding id with a quote', { findingId: 'f"2' }], + ['an empty question', { question: ' \n ' }], + ['a question over 2000 characters', { question: 'x'.repeat(2001) }], + ['a body missing the gate', { gateId: undefined }], + ])('400 for %s', async (_name, over) => { + const { io: deps, calls } = io(); + const res = await handleFindingQuestion(body(over), deps); + expect(res.status).toBe(400); + expect(calls).toEqual([]); + }); + + test("409 when the gate is not this MR's open review gate", async () => { + for (const reviewGate of [ + () => undefined, + () => gateRow({ id: 'g-other' }), + () => gateRow({ status: 'answered' }), + () => gateRow({ status: 'closed' }), + ]) { + const { io: deps, calls } = io({ reviewGate }); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(409); + expect(calls).toEqual([]); + } + }); + + test("409 when the gate's findings do not list the finding", async () => { + const { io: deps, calls } = io(); + const res = await handleFindingQuestion(body({ findingId: 'f9' }), deps); + expect(res.status).toBe(409); + expect(calls).toEqual([]); + }); + + test('409 with the resume hint when the review has no session on file', async () => { + const { io: deps, calls } = io({ reviewPane: () => ({ tabId: 'w1:t1' }) }); + const res = await handleFindingQuestion(body(), deps); + expect(res).toEqual({ status: 409, body: NO_REVIEW_PANE }); + expect(NO_REVIEW_PANE).toBe( + "the review pane isn't running; resume the review from its row first" + ); + expect(calls).toEqual([]); + }); + + test('409 with the resume hint, typing nothing, when no pane runs the session', async () => { + const { io: deps, calls } = io({ sessionPane: async () => null }); + const res = await handleFindingQuestion(body(), deps); + expect(res).toEqual({ status: 409, body: NO_REVIEW_PANE }); + expect(calls).toEqual([]); + }); + + test('409, typing nothing, when rt cannot say where the session runs', async () => { + const { io: deps, calls } = io({ + sessionPane: async () => { + throw new Error('daemon unreachable'); + }, + }); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(409); + expect(res.body).toContain('daemon unreachable'); + expect(calls).toEqual([]); + }); + + test('409 carrying the reason when the pane refuses for anything else', async () => { + const { io: deps, calls } = io({}, [sent('refused', 'pane is gone')]); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(409); + expect(res.body).toContain('pane is gone'); + expect(calls).toEqual(['send:w1:p1']); + }); + + test('409 when rt cannot deliver at all', async () => { + const { io: deps } = io({}, [{ ok: false, error: 'daemon unreachable' }]); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(409); + expect(res.body).toContain('daemon unreachable'); + }); +}); + +describe('handleFindingQuestion delivery', () => { + test('types the line into the review pane and focuses it', async () => { + const { io: deps, calls, texts, focused } = io(); + const res = await handleFindingQuestion( + body({ question: 'Is "this"\nreally Important?' }), + deps + ); + expect(res).toEqual({ status: 200, body: { ok: true } }); + expect(texts).toEqual([ + '/board:review-finding-question g-review f2 "Is \\"this\\" really Important?"', + ]); + expect(calls).toEqual(['send:w1:p1', 'focus']); + expect(focused).toEqual([{ paneId: 'w1:p1', tabId: 'w1:t1' }]); + }); + + test('a queued delivery is success too', async () => { + const { io: deps } = io({}, [sent('queued')]); + expect((await handleFindingQuestion(body(), deps)).status).toBe(200); + }); + + test("the body's tabId wins over the review state's", async () => { + const { io: deps, focused } = io(); + await handleFindingQuestion(body({ tabId: 'w2:t9' }), deps); + expect(focused).toEqual([{ paneId: 'w1:p1', tabId: 'w2:t9' }]); + }); + + test('a pane at the gate form gets one Escape, a pause, then the line again', async () => { + const { + io: deps, + calls, + texts, + } = io({}, [sent('refused', 'at a prompt'), sent('accepted')]); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(200); + expect(calls).toEqual([ + 'send:w1:p1', + 'read:w1:p1', + 'escape:w1:p1', + `sleep:${ESCAPE_SETTLE_MS}`, + 'send:w1:p1', + 'focus', + ]); + expect(texts[0]).toBe(texts[1]); + expect(ESCAPE_SETTLE_MS).toBe(600); + }); + + test('a pane blocked on any other prompt gets no Escape and a 409', async () => { + const { io: deps, calls } = io( + { readScreen: async () => PERMISSION_SCREEN }, + [sent('refused', 'at a prompt')] + ); + const res = await handleFindingQuestion(body(), deps); + expect(res).toEqual({ status: 409, body: PANE_AT_OTHER_PROMPT }); + expect(PANE_AT_OTHER_PROMPT).toBe( + 'the review pane is waiting on a prompt; answer it there first' + ); + expect(calls).toEqual(['send:w1:p1']); + }); + + test('a blocked pane whose screen cannot be read gets no Escape either', async () => { + const errors: string[] = []; + const { io: deps, calls } = io( + { + readScreen: async () => { + throw new Error('herdr down'); + }, + logError: m => errors.push(m), + }, + [sent('refused', 'at a prompt')] + ); + const res = await handleFindingQuestion(body(), deps); + expect(res).toEqual({ status: 409, body: PANE_AT_OTHER_PROMPT }); + expect(calls).toEqual(['send:w1:p1']); + expect(errors[0]).toContain('herdr down'); + }); + + test('only one retry: a pane still at a prompt is a 409', async () => { + const { io: deps, calls } = io({}, [ + sent('refused', 'at a prompt'), + sent('refused', 'at a prompt'), + ]); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(409); + expect(res.body).toContain('at a prompt'); + expect(calls.filter(c => c.startsWith('send:')).length).toBe(2); + expect(calls).not.toContain('focus'); + }); + + test("the send names the review's session, and the pane it resolved gets the Escape and the focus", async () => { + const { + io: deps, + calls, + sessions, + focused, + } = io({}, [ + sent('refused', 'at a prompt', 'w3:p7'), + sent('accepted', undefined, 'w3:p7'), + ]); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(200); + expect(sessions).toEqual(['sess-1', 'sess-1']); + expect(calls).toEqual([ + 'send:w1:p1', + 'read:w3:p7', + 'escape:w3:p7', + `sleep:${ESCAPE_SETTLE_MS}`, + 'send:w3:p7', + 'focus', + ]); + expect(focused).toEqual([{ paneId: 'w3:p7', tabId: 'w1:t1' }]); + }); + + test('an Escape herdr fails to send is a 409, and the line is not sent again', async () => { + const { io: deps, calls } = io( + { + sendEscape: async () => { + throw new Error('herdr down'); + }, + }, + [sent('refused', 'at a prompt')] + ); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(409); + expect(res.body).toContain('herdr down'); + expect(calls).toEqual(['send:w1:p1', 'read:w1:p1']); + }); + + test('a pane that is gone is the resume hint', async () => { + const { io: deps, calls } = io({}, [sent('refused', 'not a claude pane')]); + const res = await handleFindingQuestion(body(), deps); + expect(res).toEqual({ status: 409, body: NO_REVIEW_PANE }); + expect(calls).toEqual(['send:w1:p1']); + }); + + test('a focus that fails does not lose the delivered question', async () => { + const { io: deps } = io({ + focusPane: async () => { + throw new Error('herdr down'); + }, + }); + expect(await handleFindingQuestion(body(), deps)).toEqual({ + status: 200, + body: { ok: true }, + }); + }); +}); + +describe('handleFindingQuestion on a parked gate', () => { + const parked = () => gateRow({ status: 'parked' }); + + test('resumes the session with the line as its prompt and focuses the new pane', async () => { + const { + io: deps, + calls, + resumed, + focused, + } = io({ + reviewGate: parked, + sessionPane: async () => null, + }); + const res = await handleFindingQuestion(body(), deps); + expect(res).toEqual({ status: 200, body: { ok: true, resumed: true } }); + expect(resumed).toEqual([ + { + mrUrl: MR, + prompt: + '/board:review-finding-question g-review f2 "Is this really Important?"', + }, + ]); + expect(calls).toEqual(['resume', 'focus']); + expect(focused).toEqual([{ paneId: 'w5:p2', tabId: 'w5:t2' }]); + }); + + test('a resume tab already open is only focused, so the question is a 409', async () => { + const { io: deps, calls } = io({ + reviewGate: parked, + sessionPane: async () => null, + resumeReview: async () => ({ + focusedExisting: true, + paneId: '', + tabId: '', + }), + }); + const res = await handleFindingQuestion(body(), deps); + expect(res).toEqual({ status: 409, body: REVIEW_RESUMING }); + expect(calls).toEqual([]); + }); + + test('a resume that fails is a 409 carrying why', async () => { + const { io: deps } = io({ + reviewGate: parked, + sessionPane: async () => null, + resumeReview: async () => { + throw new Error('agent not found'); + }, + }); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(409); + expect(res.body).toContain('agent not found'); + }); + + test('no agent on file to resume is the resume hint', async () => { + const { io: deps, calls } = io({ + reviewGate: parked, + reviewPane: () => ({ sessionId: 'sess-1' }), + sessionPane: async () => null, + }); + const res = await handleFindingQuestion(body(), deps); + expect(res).toEqual({ status: 409, body: NO_REVIEW_PANE }); + expect(calls).toEqual([]); + }); + + test('a session already running again takes the line in its pane', async () => { + const { io: deps, calls } = io({ reviewGate: parked }); + const res = await handleFindingQuestion(body(), deps); + expect(res).toEqual({ status: 200, body: { ok: true } }); + expect(calls).toEqual(['send:w1:p1', 'focus']); + }); +}); diff --git a/apps/board/src/__tests__/gates-cache.test.ts b/apps/board/src/__tests__/gates-cache.test.ts index 732ffa938a..5c6e06fcd9 100644 --- a/apps/board/src/__tests__/gates-cache.test.ts +++ b/apps/board/src/__tests__/gates-cache.test.ts @@ -7,6 +7,7 @@ import { cachedExecution, GateCache, isRowAnswerable, + REPLACED_KEPT, } from '../gates/cache.ts'; const SUBJECT_A = 'mr:https://gitlab.com/acme/webapp/-/merge_requests/4821'; @@ -1227,3 +1228,190 @@ describe('attachGates owner', () => { expect(mr!.gates[0]!.owner).toBe('human'); }); }); + +describe('GateCache.supersededBy', () => { + const opened = (id: string) => ({ + topic: `gate/opened/${id}`, + payload: { + id, + subject: SUBJECT_A, + kind: 'review-post', + questions: [{ id: 'q1', label: 'Ship it?', options: ['yes', 'no'] }], + }, + }); + + test("the supersede rule's frames keep the old gate as the new one's predecessor", () => { + const cache = new GateCache(); + cache.applyEvent(opened('g-old')); + cache.applyEvent(opened('g-new')); + cache.applyEvent({ + topic: 'gate/closed/g-old', + payload: { + id: 'g-old', + subject: SUBJECT_A, + kind: 'review-post', + reason: 'superseded', + supersededBy: 'g-new', + }, + }); + expect(cache.get(SUBJECT_A, 'review-post')?.id).toBe('g-new'); + expect(cache.rows().map(r => r.id)).toEqual(['g-new']); + const prior = cache.supersededBy('g-new'); + expect(prior?.id).toBe('g-old'); + expect(prior?.status).toBe('closed'); + expect(prior?.closedReason).toBe('superseded'); + }); + + test('a replaced gate that was never closed as superseded is no predecessor', () => { + const cache = new GateCache(); + cache.applyEvent(opened('g-old')); + cache.applyEvent(opened('g-new')); + expect(cache.supersededBy('g-new')).toBeUndefined(); + }); + + test('a reconcile listing both rounds, newest first, keeps the predecessor', () => { + const cache = new GateCache(); + cache.reconcile([ + row({ id: 'g-new', openedAt: 2000 }), + row({ + id: 'g-old', + openedAt: 1000, + status: 'closed', + closedReason: 'superseded', + supersededBy: 'g-new', + }), + ]); + expect(cache.get(SUBJECT_A, 'review-post')?.id).toBe('g-new'); + expect(cache.supersededBy('g-new')?.id).toBe('g-old'); + }); + + test('a reconcile listing a whole chain keeps every link, in any order', () => { + const cache = new GateCache(); + cache.reconcile([ + row({ + id: 'g-older', + openedAt: 500, + status: 'closed', + closedReason: 'superseded', + supersededBy: 'g-old', + }), + row({ id: 'g-new', openedAt: 2000 }), + row({ + id: 'g-old', + openedAt: 1000, + status: 'closed', + closedReason: 'superseded', + supersededBy: 'g-new', + }), + ]); + expect(cache.supersededBy('g-new')?.id).toBe('g-old'); + expect(cache.supersededBy('g-old')?.id).toBe('g-older'); + }); + + test('an older round nothing superseded is dropped', () => { + const cache = new GateCache(); + cache.reconcile([ + row({ id: 'g-new', openedAt: 2000 }), + row({ id: 'g-old', openedAt: 1000, status: 'answered' }), + ]); + expect(cache.supersededBy('g-new')).toBeUndefined(); + expect(cache.replacements()).toEqual({}); + }); +}); + +describe('GateCache.replacements', () => { + const opened = (id: string) => ({ + topic: `gate/opened/${id}`, + payload: { + id, + subject: SUBJECT_A, + kind: 'review-post', + questions: [{ id: 'q1', label: 'Ship it?', options: ['yes', 'no'] }], + }, + }); + + test('maps each superseded gate to the gate that replaced it', () => { + const cache = new GateCache(); + cache.reconcile([ + row({ id: 'g-new', openedAt: 2000 }), + row({ + id: 'g-old', + openedAt: 1000, + status: 'closed', + closedReason: 'superseded', + supersededBy: 'g-new', + }), + row({ id: 'p-new', kind: 'respond-plan', openedAt: 2000 }), + row({ + id: 'p-old', + kind: 'respond-plan', + openedAt: 1000, + status: 'closed', + closedReason: 'superseded', + supersededBy: 'p-new', + }), + ]); + expect(cache.replacements()).toEqual({ + 'g-old': 'g-new', + 'p-old': 'p-new', + }); + }); + + test('two re-raises in a row map both old gates to the live one', () => { + const cache = new GateCache(); + const closed = (id: string, by: string) => ({ + topic: `gate/closed/${id}`, + payload: { + id, + subject: SUBJECT_A, + kind: 'review-post', + reason: 'superseded', + supersededBy: by, + }, + }); + cache.applyEvent(opened('g-1')); + cache.applyEvent(opened('g-2')); + cache.applyEvent(closed('g-1', 'g-2')); + cache.applyEvent(opened('g-3')); + cache.applyEvent(closed('g-2', 'g-3')); + expect(cache.replacements()).toEqual({ 'g-1': 'g-3', 'g-2': 'g-3' }); + expect(cache.supersededBy('g-3')?.id).toBe('g-2'); + expect(cache.supersededBy('g-2')?.id).toBe('g-1'); + }); + + test(`keeps only the newest ${REPLACED_KEPT} replaced gates per subject and kind`, () => { + const cache = new GateCache(); + const count = REPLACED_KEPT + 2; + cache.reconcile([ + row({ id: `g-${count}`, openedAt: count * 1000 }), + ...Array.from({ length: count - 1 }, (_, i) => + row({ + id: `g-${i + 1}`, + openedAt: (i + 1) * 1000, + status: 'closed', + closedReason: 'superseded', + supersededBy: `g-${i + 2}`, + }) + ), + ]); + const kept = Object.keys(cache.replacements()).sort(); + expect(kept.length).toBe(REPLACED_KEPT); + expect(kept).not.toContain('g-1'); + expect(new Set(Object.values(cache.replacements()))).toEqual( + new Set([`g-${count}`]) + ); + }); + + test('a replaced gate that is not closed with a successor is left out', () => { + const cache = new GateCache(); + cache.applyEvent(opened('g-old')); + cache.applyEvent(opened('g-new')); + expect(cache.replacements()).toEqual({}); + }); + + test('is empty for a cache that replaced nothing', () => { + const cache = new GateCache(); + cache.applyRow(row()); + expect(cache.replacements()).toEqual({}); + }); +}); diff --git a/apps/board/src/__tests__/herdr.test.ts b/apps/board/src/__tests__/herdr.test.ts index fb3cbf1602..6fd0aea382 100644 --- a/apps/board/src/__tests__/herdr.test.ts +++ b/apps/board/src/__tests__/herdr.test.ts @@ -22,6 +22,7 @@ import { parseLaunchNote, parseTabCreate, parseWorkspaceCreate, + readPaneScreen, reopenPrompt, respondPrompt, reviewPrompt, @@ -1444,3 +1445,15 @@ describe('sendPaneText', () => { ]); }); }); + +describe('readPaneScreen', () => { + test('reads the visible screen with `pane read --source visible`', async () => { + const calls: string[][] = []; + const runner: HerdrRunner = async args => { + calls.push(args); + return 'screen text'; + }; + expect(await readPaneScreen('w1:p1', runner)).toBe('screen text'); + expect(calls).toEqual([['pane', 'read', 'w1:p1', '--source', 'visible']]); + }); +}); diff --git a/apps/board/src/__tests__/question-form.test.ts b/apps/board/src/__tests__/question-form.test.ts new file mode 100644 index 0000000000..f66096d98e --- /dev/null +++ b/apps/board/src/__tests__/question-form.test.ts @@ -0,0 +1,56 @@ +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { describe, expect, test } from 'bun:test'; + +import { hasQuestionForm as daemonHasQuestionForm } from '../../../../lib/daemon/question-form.ts'; +import { hasQuestionForm } from '../question-form.ts'; + +// The board's copy of the daemon's AskUserQuestion detector must agree with +// it on every screen: the daemon's captured fixtures and the shapes its own +// tests build from them. +const FIXTURES = join( + import.meta.dir, + '..', + '..', + '..', + '..', + 'lib', + 'daemon', + '__tests__', + 'fixtures' +); +const fixture = (name: string) => readFileSync(join(FIXTURES, name), 'utf8'); + +const single = fixture('question-form-single.txt'); +const screens: Array<[string, string, boolean]> = [ + ['a single-question form', single, true], + ['a multi-question form', fixture('question-form-multi.txt'), true], + ['an inline-rendered form', fixture('question-form-inline.txt'), true], + ['an idle prompt', fixture('pane-footer-1-shell.txt'), false], + [ + 'a form the session moved past', + `${single}\n⏺ User answered Claude's questions:\n`, + false, + ], + [ + 'the vim-mode footer', + single.replace( + 'Enter to select · ↑/↓ to navigate · Esc to cancel', + '-- INSERT -- · Enter to select · ↑/↓ to navigate' + ), + true, + ], + [ + 'transcript text quoting the footer', + '⏺ quoting:\n Enter to select · ↑/↓ to navigate · Esc to cancel\n❯ \n', + false, + ], + ['an empty screen', '', false], +]; + +describe('hasQuestionForm (port of lib/daemon/question-form.ts)', () => { + test.each(screens)('%s', (_name, screen, form) => { + expect(hasQuestionForm(screen)).toBe(form); + expect(daemonHasQuestionForm(screen)).toBe(form); + }); +}); diff --git a/apps/board/src/__tests__/review-revision.test.ts b/apps/board/src/__tests__/review-revision.test.ts new file mode 100644 index 0000000000..ad27b29d6b --- /dev/null +++ b/apps/board/src/__tests__/review-revision.test.ts @@ -0,0 +1,189 @@ +import { describe, expect, test } from 'bun:test'; + +import type { GateRow as FacilityGateRow } from '@mattstack/rt-client'; +import type { FindingEntry } from '../client/board/gate-ctx.ts'; +import { GateCache } from '../gates/cache.ts'; +import { + findingChanged, + gateFindings, + reviewRevisionFor, +} from '../review-revision.ts'; + +const MR = 'https://gitlab.example.com/acme/widgets/-/merge_requests/31'; +const SUBJECT = `mr:${MR}`; + +const F1: FindingEntry = { + id: 'f1', + severity: 'important', + title: 'Retry loop never backs off', + body: 'The loop retries at once.', + file: 'lib/retry.ts:12', + comment: 'Could this back off?', +}; +const F2: FindingEntry = { + id: 'f2', + severity: 'minor', + title: 'Stale doc comment', + body: 'The comment names the old flag.', + file: 'lib/flags.ts:4', + comment: 'Nit: the comment is stale.', +}; + +/** A review-post facility row whose findings@1 joins its options. */ +function reviewRow( + id: string, + findings: FindingEntry[], + over: Partial = {} +): FacilityGateRow { + const counts = { critical: 0, important: 0, minor: 0 }; + for (const f of findings) counts[f.severity]++; + return { + id, + subject: SUBJECT, + kind: 'review-post', + questions: [ + { + id: 'findings-1', + label: 'Post which findings?', + multi: true, + context: JSON.stringify({ 'gate-ctx': 'findings@1', findings }), + options: findings.map(f => ({ value: f.id, label: f.title })), + }, + { + id: 'outcome', + label: 'Verdict', + multi: false, + options: [ + { value: 'approve', label: 'approve' }, + { value: 'comment', label: 'comment' }, + ], + }, + ], + context: JSON.stringify({ + 'gate-ctx': 'review@1', + readiness: 'with-fixes', + summary: 'Two findings.', + findings: counts, + }), + meta: null, + status: 'open', + answer: null, + openedAt: 2000, + parkedAt: null, + closedAt: null, + closedReason: null, + agent: null, + pane: null, + nudge: null, + delivery: null, + released: false, + supersededBy: null, + owner: null, + escalatedAt: null, + consumedAt: null, + ...over, + } as FacilityGateRow; +} + +const superseded = (id: string, findings: FindingEntry[], by: string) => + reviewRow(id, findings, { + openedAt: 1000, + status: 'closed', + closedReason: 'superseded', + supersededBy: by, + }); + +function cacheWith(...rows: FacilityGateRow[]): GateCache { + const cache = new GateCache(); + cache.reconcile(rows); + return cache; +} + +describe('gateFindings', () => { + test("reads a review sheet's findings by id", () => { + expect([...gateFindings(reviewRow('g', [F1, F2]))!.keys()]).toEqual([ + 'f1', + 'f2', + ]); + }); + + test('a row the sheet cannot read has none', () => { + expect(gateFindings(reviewRow('g', [F1], { context: null }))).toBeNull(); + }); +}); + +describe('findingChanged', () => { + test.each([ + ['severity', { severity: 'minor' as const }], + ['anchor', { file: 'lib/retry.ts:14' }], + ['title', { title: 'Retry loop spins' }], + ['body', { body: 'Reworded.' }], + ['fix', { fix: 'Back off exponentially.' }], + ['comment', { comment: 'Other draft.' }], + ])('a different %s is a revision', (_name, change) => { + expect(findingChanged(F1, { ...F1, ...change })).toBe(true); + }); + + test('the same finding is not', () => { + expect(findingChanged(F1, { ...F1 })).toBe(false); + }); +}); + +describe('reviewRevisionFor', () => { + test("names each finding the open gate changed, with the superseded gate's original", () => { + const revisedF1 = { ...F1, severity: 'minor' as const, body: 'Narrower.' }; + const cache = cacheWith( + reviewRow('g-new', [revisedF1, F2]), + superseded('g-old', [F1, F2], 'g-new') + ); + expect(reviewRevisionFor(cache, MR)).toEqual({ + gateId: 'g-new', + previousGateId: 'g-old', + previousFindingIds: ['f1', 'f2'], + revised: { f1: { original: F1 } }, + }); + }); + + test('a dropped finding has no card, so it is left out', () => { + const cache = cacheWith( + reviewRow('g-new', [F1]), + superseded('g-old', [F1, F2], 'g-new') + ); + expect(reviewRevisionFor(cache, MR)?.revised).toEqual({}); + }); + + test("a predecessor older than the lane's run is another run's gate", () => { + const cache = cacheWith( + reviewRow('g-new', [{ ...F2, id: 'f1' }]), + superseded('g-old', [F1], 'g-new') + ); + expect(reviewRevisionFor(cache, MR, 1500)).toBeUndefined(); + expect(reviewRevisionFor(cache, MR, 1000)?.previousGateId).toBe('g-old'); + }); + + test('no predecessor in the cache (a board restart) means no revision', () => { + const cache = cacheWith(reviewRow('g-new', [F1, F2])); + expect(reviewRevisionFor(cache, MR)).toBeUndefined(); + }); + + test('an answered gate is not on screen to mark', () => { + const cache = cacheWith( + reviewRow('g-new', [{ ...F1, title: 'New' }], { status: 'answered' }), + superseded('g-old', [F1], 'g-new') + ); + expect(reviewRevisionFor(cache, MR)).toBeUndefined(); + }); + + test('a re-raise that changed nothing still names its predecessor', () => { + const cache = cacheWith( + reviewRow('g-new', [F1, F2]), + superseded('g-old', [F1, F2], 'g-new') + ); + expect(reviewRevisionFor(cache, MR)).toEqual({ + gateId: 'g-new', + previousGateId: 'g-old', + previousFindingIds: ['f1', 'f2'], + revised: {}, + }); + }); +}); diff --git a/apps/board/src/client/board/Board.tsx b/apps/board/src/client/board/Board.tsx index 59b8fc2ce4..9d5274bf2f 100644 --- a/apps/board/src/client/board/Board.tsx +++ b/apps/board/src/client/board/Board.tsx @@ -128,6 +128,7 @@ import { useOptimisticLifecycle, } from './hooks.ts'; import { assignMemberLooks } from './invadr-colors.ts'; +import { FINDING_QUESTION, findingQuestionFields } from './launch-flow.ts'; import { MemberInvadr, MemberLooksProvider } from './MemberInvadr.tsx'; import { mrRef, onGitHub } from './MrLinks.tsx'; import { NEED_LABEL, NEED_ORDER, needOf } from './needs-me.ts'; @@ -676,6 +677,32 @@ export function Board() { failureMessage: resumeRespondFailureMessage, }); + const questionAction = useLaunchAction({ + axis: null, + ...FINDING_QUESTION, + optimistic: optimisticLifecycle, + addToast, + startToast, + reload: load, + }); + // Resolves whether the review pane took the question, so the sheet can + // say so under the finding. + const askQuestion = useCallback( + async ( + mr: BoardMRWithReview, + gateId: string, + findingId: string, + question: string + ) => { + const result = await questionAction( + mr, + findingQuestionFields(gateId, findingId, question, mr.review?.tabId) + ); + return Boolean(result?.ok); + }, + [questionAction] + ); + // Each flow's payload lives here once: the row menu, the bulk menu, the // status line's verbs and the decision queue all launch through it. const launch = useCallback( @@ -1476,7 +1503,12 @@ export function Board() { if (gate.status === 'answered') ids.add(gate.gateId); return ids; }, [data]); - const queue = useDecisionQueue(queueEntries, answeredGateIds); + const gateReplacements = data?.gateReplacements; + const queue = useDecisionQueue( + queueEntries, + answeredGateIds, + gateReplacements + ); const activeGateId = queue.active?.gate.gateId ?? null; const retireActiveGate = () => { if (activeGateId === null) return; @@ -2234,6 +2266,7 @@ export function Board() { onContinue={retireActiveGate} onLostChange={lost => queue.hold(lost ? activeGateId : null)} self={seat} + onAskQuestion={askQuestion} readOnly={gateReadOnlyReason( queue.active.gate, queue.active.mr, diff --git a/apps/board/src/client/board/DecisionQueueModal.tsx b/apps/board/src/client/board/DecisionQueueModal.tsx index b8357c2a41..7a7070319f 100644 --- a/apps/board/src/client/board/DecisionQueueModal.tsx +++ b/apps/board/src/client/board/DecisionQueueModal.tsx @@ -5,6 +5,7 @@ import { domainForKind, type GateDomain, } from '@mattstack/gate-kit'; +import { clearGateDraft, readGateDraft } from '@mattstack/gate-kit/react'; import { Button, CHECK_ICON, Chip, Icon } from '@mattstack/tui-kit'; import type { GateRow } from '../../gates/store.ts'; import { gateContext } from '../../gates/wait-meta.ts'; @@ -18,8 +19,13 @@ import { GateSheet, type GateSheetQueue } from './GateSheet.tsx'; import { mrRef } from './MrLinks.tsx'; import { postPicks } from './respond-post.ts'; import { RespondSheetBody } from './RespondSheet.tsx'; +import { + reviewFindingIds, + reviewPredecessor, + seedReviewDraft, +} from './review-draft-seed.ts'; import { isReviewSheetGate, paneContext } from './review-gate.ts'; -import { ReviewGateSheet } from './ReviewGateSheet.tsx'; +import { ReviewGateSheet, type AskQuestion } from './ReviewGateSheet.tsx'; import { paneReason, PaneSheetBody, StageSheetBody } from './StageSheet.tsx'; /** One pip per queued gate. */ @@ -68,6 +74,7 @@ function DecisionQueueModal({ onLostChange, readOnly: seatReadOnly, self, + onAskQuestion, }: { gate: GateRow & { executor?: ExecutorState }; /** Absent for a non-MR gate (queueExtras) -- the strip and face below @@ -98,6 +105,9 @@ function DecisionQueueModal({ readOnly?: string | null; /** The seat's username, for the avatar beside "you" in a sheet. */ self?: string | null; + /** Types a question about one review finding into the review pane, + through the board's launch flow; resolves whether the pane took it. */ + onAskQuestion?: AskQuestion; }) { const paneGone = gate.executor === 'gone'; const context = gateContext(gate); @@ -127,7 +137,30 @@ function DecisionQueueModal({ const isReviewSheet = useMemo(() => isReviewSheetGate(gate), [gate]); const answered = gate.status === 'answered'; const actionable = gate.status === 'open' || gate.status === 'parked'; - const form = useGateForm(gate, onAnswered, { readOnly: Boolean(readOnly) }); + // A review gate the agent re-raised after revising a finding starts from + // the draft the reviewer left on the gate it superseded. + const predecessor = isReviewSheet + ? reviewPredecessor(gate, mr?.reviewRevision) + : undefined; + const form = useGateForm(gate, onAnswered, { + readOnly: Boolean(readOnly), + ...(predecessor + ? { + seed: () => + seedReviewDraft(readGateDraft(predecessor.previousGateId), { + findingIds: reviewFindingIds(gate), + previousFindingIds: predecessor.previousFindingIds, + revisedIds: Object.keys(predecessor.revised), + }), + } + : {}), + }); + // The seed was read on the first render and this gate saves its own draft + // from then on, so the superseded gate's is done with. + const previousGateId = predecessor?.previousGateId; + useEffect(() => { + if (previousGateId) clearGateDraft(previousGateId); + }, [previousGateId]); const deliveryStuck = answered && gate.delivery?.outcome === 'stuck'; const executionUnassigned = answered && gate.execution === 'unassigned'; const reason = gate.kind === 'pane-attention' ? paneReason(gate) : undefined; @@ -217,6 +250,8 @@ function DecisionQueueModal({ void, - { readOnly = false }: { readOnly?: boolean } = {} + { + readOnly = false, + seed, + }: { readOnly?: boolean; seed?: () => GateDraft | null } = {} ) { const actionable = gate.status === 'open' || gate.status === 'parked'; const { - initial: draft, + initial: own, save: saveDraft, clear: clearDraft, } = useGateDraft(gate.gateId, actionable); + const [draft] = useState( + () => own ?? (actionable && seed ? seed() : null) + ); const preset = useMemo( () => actionable && !readOnly diff --git a/apps/board/src/client/board/Gates.stories.tsx b/apps/board/src/client/board/Gates.stories.tsx index 22bb4a2876..f4a99c0a61 100644 --- a/apps/board/src/client/board/Gates.stories.tsx +++ b/apps/board/src/client/board/Gates.stories.tsx @@ -1,6 +1,8 @@ import { useEffect, type ReactNode } from 'react'; import type { Meta, StoryObj } from '@storybook/react-vite'; +import { expect, userEvent, waitFor, within } from 'storybook/test'; +import { ToastHost, useToasts } from '@mattstack/tui-kit'; import { registerTheme, SoribashiProvider } from '@mattstack/tui-kit/provider'; import { tuiTheme } from '@mattstack/tui-kit/theme'; @@ -8,12 +10,20 @@ import '@mattstack/tui-kit/theme.css'; import '@mattstack/tui-kit/canvas.css'; import '../../style.css'; +import type { ActionResult } from '../api.ts'; +import type { ExecutorState } from '../types.ts'; import { DecisionQueueModal } from './DecisionQueueModal.tsx'; import { DiffSourceContext, noDiffSource } from './diff-source.ts'; import * as G from './gate-stories.fixtures.ts'; import { assignMemberLooks } from './invadr-colors.ts'; +import { + FINDING_QUESTION, + findingQuestionFields, + runLaunchFlow, +} from './launch-flow.ts'; import { MemberLooksProvider } from './MemberInvadr.tsx'; import { boardPeople, PeopleProvider } from './people.ts'; +import type { AskQuestion } from './ReviewGateSheet.tsx'; /** * The review and respond gates as the board opens them, one story per gate @@ -117,7 +127,18 @@ const STORY_LOOKS = assignMemberLooks( [...G.PEOPLE].map(([username]) => username) ); -function Face({ entry }: { entry: G.StoryGate }) { +/** A story gate, optionally with the reconciler's view of its pane. */ +type FaceEntry = G.StoryGate & { + gate: G.StoryGate['gate'] & { executor?: ExecutorState }; +}; + +function Face({ + entry, + onAskQuestion, +}: { + entry: FaceEntry; + onAskQuestion?: AskQuestion; +}) { const people = boardPeople({ allMembers: [...G.PEOPLE].map(([username, name]) => ({ username, name })), mrs: [entry.mr], @@ -139,6 +160,7 @@ function Face({ entry }: { entry: G.StoryGate }) { onAnswered={noop} onContinue={noop} self="renee" + onAskQuestion={onAskQuestion} /> @@ -194,3 +216,186 @@ export const ReviewCodeLoading: Story = { /> ), }; + +/** The review sheet's questions to the review pane, sent through the + board's own launch flow (`runLaunchFlow` with `FINDING_QUESTION`), so the + toasts read as they do on the board. `reply` stands in for the route: + a promise that never settles leaves the question sending. */ +function QuestionFace({ + entry, + reply, +}: { + entry: FaceEntry; + reply: () => Promise; +}) { + const { toasts, addToast, startToast } = useToasts(); + const ask: AskQuestion = async (mr, gateId, findingId, question) => { + const result = await runLaunchFlow( + { + ...FINDING_QUESTION, + post: reply, + setQueued: noop, + rollback: noop, + addToast, + startToast, + reload: noop, + }, + mr, + findingQuestionFields(gateId, findingId, question) + ); + return Boolean(result?.ok); + }; + return ( + <> + + + + ); +} + +const answered = + (status: number, text = ''): (() => Promise) => + async () => ({ + ok: status === 200, + status, + body: status === 200 ? {} : null, + text, + }); +const pending = () => new Promise(() => {}); + +/** The first round with the review pane holding at its gate. */ +const live: FaceEntry = G.reviewFirstRound; +/** The same gate with its pane gone, as the reconciler reports it. */ +const paneGone: FaceEntry = { + ...live, + gate: { ...live.gate, executor: 'gone' }, +}; +/** The same gate parked after a long wait: the board closed its pane. */ +const parked: FaceEntry = { + ...live, + gate: { ...live.gate, status: 'parked', executor: 'gone' }, +}; + +const QUESTION = + 'Is a stale tile really worse than none here? The map shows the last good forecast on purpose.'; + +/** Opens the first finding's question box and types `QUESTION` into it. */ +async function openQuestion(canvasElement: HTMLElement) { + const body = within(canvasElement.ownerDocument.body); + const [link] = await body.findAllByRole('button', { + name: /: ask a question$/, + }); + await userEvent.click(link!); + const box = await body.findByRole('textbox', { name: /: your question$/ }); + await userEvent.type(box, QUESTION); + return body; +} + +async function sendQuestion(canvasElement: HTMLElement) { + const body = await openQuestion(canvasElement); + await userEvent.click( + body.getByRole('button', { name: 'send to the review pane' }) + ); + return body; +} + +/** Every drafted finding, ticked or not, offers "ask a question" at the + end of its card head. */ +export const QuestionAskLink: Story = { + name: 'Review question: ask link', + decorators: [ReviewReport], + render: () => , +}; + +/** The question box under the first finding's head, with a question in + it. Cmd/Ctrl+Enter sends it, Escape cancels it. */ +export const QuestionBoxOpen: Story = { + name: 'Review question: box open', + decorators: [ReviewReport], + render: () => , + play: async ({ canvasElement }) => { + await openQuestion(canvasElement); + }, +}; + +/** The question on its way: the box holds, the board toasts "asking in + the review pane", and every other finding's link waits. The route never + answers here, so it stays sending. */ +export const QuestionSending: Story = { + name: 'Review question: sending', + decorators: [ReviewReport], + render: () => , + play: async ({ canvasElement }) => { + const body = await sendQuestion(canvasElement); + await waitFor(() => + expect( + body.getByRole('textbox', { name: /: your question$/ }) + ).toBeDisabled() + ); + }, +}; + +/** The review pane took the question: the area says so and offers the + pane. */ +export const QuestionAsked: Story = { + name: 'Review question: asked', + decorators: [ReviewReport], + render: () => , + play: async ({ canvasElement }) => { + const body = await sendQuestion(canvasElement); + await body.findByText('asked in the review pane'); + }, +}; + +/** The route refused (409): the toast names its reason, and the box keeps + the question to try again. */ +export const QuestionRefused: Story = { + name: 'Review question: 409 refused', + decorators: [ReviewReport], + render: () => ( + + ), + play: async ({ canvasElement }) => { + const body = await sendQuestion(canvasElement); + await body.findByText(/question failed for !826 \(409\)/); + await waitFor(() => + expect( + body.getByRole('textbox', { name: /: your question$/ }) + ).toBeEnabled() + ); + }, +}; + +/** An open gate whose review pane is gone: every link stays, disabled, + with the reason as its tooltip ("the review pane isn't running; resume + the review from its row first"). */ +export const QuestionPaneGone: Story = { + name: 'Review question: pane gone', + decorators: [ReviewReport], + render: () => , +}; + +/** A parked gate: its pane was closed on park, but the link stays live, + since asking resumes the review's session with the question. */ +export const QuestionParked: Story = { + name: 'Review question: parked gate', + decorators: [ReviewReport], + render: () => , + play: async ({ canvasElement }) => { + await openQuestion(canvasElement); + }, +}; + +/** The gate the reviewer re-raised after narrowing f2: its card carries + the revised strip, with the original behind "show original". */ +export const QuestionRevised: Story = { + name: 'Review question: revised strip', + decorators: [ReviewReport], + render: () => , +}; diff --git a/apps/board/src/client/board/ReviewDraftCard.tsx b/apps/board/src/client/board/ReviewDraftCard.tsx index 9aab1bcc9f..f5f36639b8 100644 --- a/apps/board/src/client/board/ReviewDraftCard.tsx +++ b/apps/board/src/client/board/ReviewDraftCard.tsx @@ -36,10 +36,12 @@ import { ArrowOutGlyph, ChevronGlyph, FileGlyph, + QuestionGlyph, UndoGlyph, } from './icons.tsx'; import { CommentAuthor } from './PersonLead.tsx'; import { commentKey, hasDiffLine, summaryKey } from './review-drafts.ts'; +import { SEVERITY_LABEL } from './review-gate.ts'; /** A diff that fails to load or render leaves the file header standing. */ class DiffBoundary extends Component< @@ -660,9 +662,215 @@ function CommentSection({ ); } +/** A `file:line` anchor split into its path and the line part after the + last colon (a single line or a range such as `117–119`). */ +function splitAnchor(file: string | undefined): { + path: string; + lines?: string; +} { + if (file === undefined) return { path: '' }; + const m = /^(.*):(\d[^:]*)$/.exec(file); + return m ? { path: m[1]!, lines: m[2]! } : { path: file }; +} + +/** What a revision changed, in the strip's order: the severity, the anchor + (`lines a → b` on the same file, the whole anchor when the file moved), + whether the title or body was reworded, then the fix and the drafted + comment. */ +export function revisionSummary( + original: FindingEntry, + revised: FindingEntry +): string[] { + const parts: string[] = []; + if (original.severity !== revised.severity) + parts.push( + `${SEVERITY_LABEL[original.severity]} → ${SEVERITY_LABEL[revised.severity]}` + ); + if (original.file !== revised.file) { + const a = splitAnchor(original.file); + const b = splitAnchor(revised.file); + parts.push( + a.path === b.path && a.lines !== undefined && b.lines !== undefined + ? `lines ${a.lines} → ${b.lines}` + : `${original.file ?? 'no file'} → ${revised.file ?? 'no file'}` + ); + } + if (original.title !== revised.title || original.body !== revised.body) + parts.push('finding reworded'); + if (original.fix !== revised.fix) parts.push('fix changed'); + if (original.comment !== revised.comment) parts.push('comment rewritten'); + return parts; +} + +/** A finding the review agent revised in its own pane and re-raised the + gate with: what changed, with the original's title and body behind a + toggle. */ +function RevisedStrip({ + original, + revised, +}: { + original: FindingEntry; + revised: FindingEntry; +}) { + const [open, setOpen] = useState(false); + const parts = [ + 'Revised in the review pane', + ...revisionSummary(original, revised), + ]; + return ( +
+
+ + {parts.map(part => ( + {part} + ))} + + +
+ {open && ( +
+

{original.title}

+ {original.body} +
+ )} +
+ ); +} + +/** One finding's question to the review pane: `idle` offers "ask a + question", `open` is the area being written, `sending` is the request in + flight, `asked` is a question the pane took. `blocked` is set while + another finding's question is sending: one question at a time. + `unavailable` is why no question can be asked at all (the review pane is + gone): the link stays, disabled, with the reason as its tooltip. */ +export interface FindingAsk { + state: 'idle' | 'open' | 'sending' | 'asked'; + blocked: boolean; + unavailable?: string; + onOpen: () => void; + onCancel: () => void; + onSend: (question: string) => void; + /** Brings the review pane forward; absent when the sheet cannot. */ + onFocusPane?: () => void; +} + +export const ASK_PLACEHOLDER = 'What do you want to know about this finding?'; + +/** A finding's "ask a question" link, at the end of the card head. */ +export function AskLink({ title, ask }: { title: string; ask: FindingAsk }) { + return ( + + ); +} + +/** The area under the card head where the question is written and sent to + the review pane, then where the sheet says the pane took it. */ +function AskArea({ title, ask }: { title: string; ask: FindingAsk }) { + const [text, setText] = useState(''); + // A question the pane took is done with; asking again starts empty. + useEffect(() => { + if (ask.state === 'asked') setText(''); + }, [ask.state]); + if (ask.state === 'asked') + return ( +
+ + asked in the review pane + + {ask.onFocusPane && ( + + )} +
+ ); + const sending = ask.state === 'sending'; + const empty = text.trim() === ''; + return ( +
+
+