diff --git a/apps/hub/src/app.ts b/apps/hub/src/app.ts index 27de5b1a..7645707b 100644 --- a/apps/hub/src/app.ts +++ b/apps/hub/src/app.ts @@ -102,6 +102,10 @@ import { requestErrorMetadata } from "./request-error.ts"; import { createLiveAppProxy } from "./live-app/proxy.ts"; import { createLiveAppRunner } from "./live-app/runner.ts"; import { mountWebUi } from "./web-ui.ts"; +import { + coordinatorAskReader, + createDeclineNote, +} from "./projects/decline-note.ts"; import { version } from "../package.json"; const log = getLogger(["corbits-builder", "hub"]); @@ -460,6 +464,13 @@ export async function createHubRuntime( repoStore: agentRepoStore.repoStore, }), ); + // Only a signed-in member of the tenant reaches the note; Interchange's route reuses the + // resolved tenant. + app.use( + "/api/tenants/:tenantId/approvals/:approvalId/reject", + createResolveTenant({ db: handle.db }), + createDeclineNote({ coordinatorAsk: coordinatorAskReader(handle.db) }), + ); mountHubRoutes(app, { db: handle.db, sidecarRouter, diff --git a/apps/hub/src/config/decline.ts b/apps/hub/src/config/decline.ts new file mode 100644 index 00000000..91510b99 --- /dev/null +++ b/apps/hub/src/config/decline.ts @@ -0,0 +1,23 @@ +// Interchange answers a declined call with "denied by approver: " and the decline's message. The +// hub writes that message for the coordinator's asks, so it reads the person's answer rather than +// a failed call. Stored declines keep this wording, and the conversation finds the person's words +// by it, so rewording it shows earlier declines' whole note as the person's message. +const DECLINE_LEAD = "The person has chosen to decline this tool call."; +const FEEDBACK_LEAD = DECLINE_LEAD + "\n\nTheir feedback is: "; +const NO_FEEDBACK = + DECLINE_LEAD + + " They left no feedback. Ask them what they would like to do next."; + +/** The message a coordinator ask's decline carries, around the person's words when they gave some. */ +export function declineNote(words: string | undefined): string { + const said = words?.trim(); + return said === undefined || said === "" ? NO_FEEDBACK : FEEDBACK_LEAD + said; +} + +/** The person's own words in a decline's message: none for a bare decline, all of a message the hub did not write. */ +export function declinedWords(message: string | undefined): string | undefined { + if (message === undefined || message === NO_FEEDBACK) return undefined; + return message.startsWith(FEEDBACK_LEAD) + ? message.slice(FEEDBACK_LEAD.length) + : message; +} diff --git a/apps/hub/src/mail/conversation-turns.test.ts b/apps/hub/src/mail/conversation-turns.test.ts index b82b2029..1f8cf3eb 100644 --- a/apps/hub/src/mail/conversation-turns.test.ts +++ b/apps/hub/src/mail/conversation-turns.test.ts @@ -3,6 +3,7 @@ import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { $ } from "bun"; +import { declineNote } from "../config/decline.ts"; import { resumeMessage } from "../config/resume.ts"; import { specialistEndedMessage } from "../config/specialist-ended.ts"; import { readDurableConversationTurns } from "./conversation-turns.ts"; @@ -105,6 +106,35 @@ test("resume turns read each approval decision as a plain line", async () => { ]); }); +test("a coordinator ask's decline reads back as the person's words alone", async () => { + const dir = await runRepo([ + { + from: "signal@local", + text: JSON.stringify({ + outcome: "rejected", + message: declineNote("Make it shorter\n\nand warmer"), + }), + }, + { + from: "signal@local", + text: JSON.stringify({ + outcome: "rejected", + message: declineNote(undefined), + }), + }, + ]); + expect(await personTexts(dir, "note")).toEqual([ + { role: "person", text: "Make it shorter\n\nand warmer" }, + ]); + expect(await personTexts(dir, "decision")).toEqual([ + { + role: "person", + text: "Rejected a request: Make it shorter\n\nand warmer", + }, + { role: "person", text: "Rejected a request." }, + ]); +}); + test("a person's words read back with the line breaks they typed", async () => { const typed = 'I dont get this one bit\n\nOn "Constraints" v1:\n\n> Use $5 per month'; diff --git a/apps/hub/src/mail/conversation-turns.ts b/apps/hub/src/mail/conversation-turns.ts index 5093447e..fccc9d38 100644 --- a/apps/hub/src/mail/conversation-turns.ts +++ b/apps/hub/src/mail/conversation-turns.ts @@ -21,6 +21,7 @@ import { withoutResumeContext, } from "../config/resume.ts"; import { personTurnText } from "../config/specialist-ended.ts"; +import { declinedWords } from "../config/decline.ts"; import { readCoordinatorMailThreads } from "./conversation-threads.ts"; import { withCommittedAgentState } from "./committed-agent-state.ts"; import type { CoordinatorRepoDirSource } from "./coordinator-question.ts"; @@ -78,9 +79,13 @@ function approvalText( ): string | undefined { const decision = ApprovalDecision(parseJson(body)); if (decision instanceof type.errors) return undefined; + const words = declinedWords(decision.message); return decisions === "note" - ? decision.message - : approvalDecisionLine(decision); + ? words + : approvalDecisionLine({ + outcome: decision.outcome, + ...(words === undefined ? {} : { message: words }), + }); } // A missing or corrupt turn list is empty; it does not throw. diff --git a/apps/hub/src/projects/decline-note.test.ts b/apps/hub/src/projects/decline-note.test.ts new file mode 100644 index 00000000..15cbcf4d --- /dev/null +++ b/apps/hub/src/projects/decline-note.test.ts @@ -0,0 +1,52 @@ +import { expect, test } from "bun:test"; +import { Hono } from "hono"; +import { declineNote } from "../config/decline.ts"; +import { createDeclineNote } from "./decline-note.ts"; + +// What Interchange's reject route would receive. +async function rejected(approvalId: string, body: unknown) { + const app = new Hono(); + app.use( + "/api/tenants/:tenantId/approvals/:approvalId/reject", + createDeclineNote({ + coordinatorAsk: async (tenantId, id) => + tenantId === "tenant_1" && id === "appr_brief", + }), + ); + app.post("/api/tenants/:tenantId/approvals/:approvalId/reject", async (c) => + c.json(await c.req.json()), + ); + const response = await app.request( + `/api/tenants/tenant_1/approvals/${approvalId}/reject`, + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }, + ); + return response.json(); +} + +test("a coordinator ask's decline carries the person's words in the hub's note", async () => { + expect( + await rejected("appr_brief", { scope: "once", message: "Make it shorter" }), + ).toEqual({ scope: "once", message: declineNote("Make it shorter") }); +}); + +test("a coordinator ask's decline without words asks the coordinator to find out what next", async () => { + expect(await rejected("appr_brief", {})).toEqual({ + message: declineNote(undefined), + }); +}); + +test("a decline of another run's ask passes through unchanged", async () => { + expect(await rejected("appr_packages", { message: "Not that one" })).toEqual({ + message: "Not that one", + }); +}); + +test("a decline whose message Interchange would refuse passes through unchanged", async () => { + expect(await rejected("appr_brief", { message: 42 })).toEqual({ + message: 42, + }); +}); diff --git a/apps/hub/src/projects/decline-note.ts b/apps/hub/src/projects/decline-note.ts new file mode 100644 index 00000000..a50c2645 --- /dev/null +++ b/apps/hub/src/projects/decline-note.ts @@ -0,0 +1,69 @@ +import { and, eq } from "drizzle-orm"; +import type { MiddlewareHandler } from "hono"; +import type { DB } from "@intx/db"; +import { approval } from "@intx/db/schema"; +import { getLogger } from "@intx/log"; +import { declineNote } from "../config/decline.ts"; +import { projectCoordinator } from "../db/schema.ts"; +import { requestErrorMetadata } from "../request-error.ts"; +import { readJson } from "../lib/json.ts"; +import { isRecord } from "../mail/conversation-turns.ts"; + +const log = getLogger(["corbits-builder", "hub", "decline-note"]); + +/** Whether an approval is one of a project coordinator's own asks. */ +export function coordinatorAskReader(db: DB["db"]) { + return async (tenantId: string, approvalId: string): Promise => { + const [asked] = await db + .select({ runId: approval.runId }) + .from(approval) + .innerJoin( + projectCoordinator, + eq(projectCoordinator.runId, approval.runId), + ) + .where(and(eq(approval.id, approvalId), eq(approval.tenantId, tenantId))) + .limit(1); + return asked !== undefined; + }; +} + +/** + * A decline of one of a coordinator's asks carries the hub's note around the person's words, so + * the coordinator reads it as their answer. Other declines, such as the coder's package asks, and + * a body Interchange's own validation should refuse, pass through unchanged. + */ +export function createDeclineNote(deps: { + coordinatorAsk: (tenantId: string, approvalId: string) => Promise; +}): MiddlewareHandler { + return async (c, next) => { + const tenantId = c.req.param("tenantId"); + const approvalId = c.req.param("approvalId"); + if (c.req.method !== "POST" || !tenantId || !approvalId) return next(); + // A note that cannot be written must not cost the person their decision. + let body: Record; + let words: string | undefined; + try { + const read = await readJson(c.req.raw.clone()); + if (!isRecord(read)) return next(); + const message = read["message"]; + if (message !== undefined && typeof message !== "string") return next(); + if (!(await deps.coordinatorAsk(tenantId, approvalId))) return next(); + body = read; + words = message; + } catch (error) { + log.error("Decline note unavailable", { + approvalId, + ...requestErrorMetadata(error), + }); + return next(); + } + const headers = new Headers(c.req.raw.headers); + headers.delete("content-length"); + c.req.raw = new Request(c.req.raw, { + method: "POST", + headers, + body: JSON.stringify({ ...body, message: declineNote(words) }), + }); + return next(); + }; +} diff --git a/apps/web/src/config.ts b/apps/web/src/config.ts index a9335714..d805b74f 100644 --- a/apps/web/src/config.ts +++ b/apps/web/src/config.ts @@ -776,9 +776,8 @@ export const wording = { approve: "Yes, go ahead", notYet: "Not yet", decline: "Decline", - /** The composer's placeholder while an approval is open: a message leaves the card open. */ - composerPlaceholder: - "Ask a question, or choose Not yet to request changes…", + /** The composer's placeholder while an approval is open: a message declines it, with these words as the reason. */ + composerPlaceholder: "Say what you'd like changed, or ask a question…", /** A whole message that approves the one open card, after a countdown the person can undo. */ typedYes: { affirmations: [ diff --git a/apps/web/src/pages/workspace/index.tsx b/apps/web/src/pages/workspace/index.tsx index 0bab9f5e..836e32bb 100644 --- a/apps/web/src/pages/workspace/index.tsx +++ b/apps/web/src/pages/workspace/index.tsx @@ -69,7 +69,8 @@ import { restoreProject, startProjectCoordinator, } from "../../api/projects.ts"; -import type { Wait } from "../../api/types.ts"; +import type { ChatMessage, Wait } from "../../api/types.ts"; +import { deliveredText } from "@corbits-builder/schemas/conversation"; import { keys } from "../../queries/keys.ts"; import { COMPOSER_BOX_CLASS, CONV_CLASS, PANES_CLASS } from "./pane-classes.ts"; import { questionsOpen, RoomThread, turnItemId } from "./transcript.tsx"; @@ -651,9 +652,13 @@ export function StageWorkspace({ return documents[key]; }; + const notSent = "That message was not sent."; + const mail = async (text: string) => + (await sendMessage(projectId, text)).message; const deliverMail = async ( body: string, clearComposer = false, + post: (text: string) => Promise = mail, ): Promise => { const text = body.trim(); if (text.length === 0 || sending) return false; @@ -672,13 +677,12 @@ export function StageWorkspace({ setSent((previous) => [...previous, pending]); if (clearComposer) setValue(""); try { - const { message, receipt } = await sendMessage(projectId, text); + const message = await post(text); setSent((previous) => previous.map((item) => item === pending ? { ...pending, ...message } : item, ), ); - void receipt; reloadRef.current(); onChanged(); return true; @@ -687,10 +691,7 @@ export function StageWorkspace({ setSent((previous) => previous.filter((item) => item !== pending)); if (clearComposer) setValue((current) => (current === "" ? text : current)); - const message = await humanFaultWithCode( - cause, - "That message was not sent.", - ); + const message = await humanFaultWithCode(cause, notSent); setSendFailed(true); setSendFault(message); toast.error(message); @@ -827,12 +828,14 @@ export function StageWorkspace({ setMobilePane("artifacts"); queueMicrotask(() => documentPaneRef.current?.focus()); }; - // Resolves to the fault the person sees, or null once the decision stands. - const onDecideApproval = async (decision: { + // "settled" is a 409: someone already decided the ask, or its run moved on; the page reads + // where it stands. + const saveDecision = async (decision: { approvalId: string; choice: "yes" | "always" | "no"; - }): Promise => { - const notSaved = "That decision was not saved."; + message?: string; + }): Promise<"saved" | "settled" | { fault: string }> => { + const notSaved = { fault: "That decision was not saved." }; const cachedWaits = queryClient.getQueryData( keys.openDecisions.of(projectId), @@ -865,6 +868,9 @@ export function StageWorkspace({ ? "approve" : "reject", ...(decision.choice === "always" ? { scope: "always" as const } : {}), + ...(decision.message === undefined + ? {} + : { message: decision.message }), }); if (decided !== undefined) settle(decided, { @@ -876,21 +882,51 @@ export function StageWorkspace({ }); setGateDone((value) => value + 1); onChanged(); - return null; + return "saved"; } catch (cause: unknown) { - // 409: someone already decided it (another tab or collaborator); show where it stands. if (cause instanceof ProductApiError && cause.response.status === 409) { setGateDone((value) => value + 1); onChanged(); - return null; + return "settled"; } - const fault = await humanFaultWithCode(cause, notSaved); + const fault = await humanFaultWithCode(cause, notSaved.fault); setApprovalError(fault); - return fault; + return { fault }; } finally { setApprovalBusy(false); } }; + // Resolves to the fault the person sees, or null once the decision stands. + const onDecideApproval = async (decision: { + approvalId: string; + choice: "yes" | "always" | "no"; + }): Promise => { + const saved = await saveDecision(decision); + return typeof saved === "object" ? saved.fault : null; + }; + // The person's words go with the decline as their reason; mailed instead when the ask could + // not take them, so they are never lost and a failed mail says why. + const declineWith = + (approvalId: string) => + async (text: string): Promise => { + const saved = await saveDecision({ + approvalId, + choice: "no", + message: deliveredText(text), + }); + if (saved === "settled") return mail(text); + if (typeof saved === "object") { + // The composer reports the fault; the card keeps no second copy of it. + setApprovalError(null); + throw new Error(saved.fault); + } + return { + id: `declined:${approvalId}`, + author: "me", + body: text, + at: new Date().toISOString(), + }; + }; const openApproval = pendingApprovals.find( (approval) => approval.packages === undefined, ); @@ -917,6 +953,8 @@ export function StageWorkspace({ openApprovalId: currentOpen?.approvalId, decide: onDecideApproval, deliver: () => deliverMail(message), + decline: (approvalId) => + deliverMail(message, false, declineWith(approvalId)), notSent: wording.approaches.notSent, }); }; @@ -930,6 +968,8 @@ export function StageWorkspace({ openApprovalId: currentOpen?.approvalId, decide: onDecideApproval, deliver: () => deliverMail(body, clearComposer), + decline: (approvalId) => + deliverMail(body, clearComposer, declineWith(approvalId)), }); }; const sendFiles = async () => { @@ -970,10 +1010,18 @@ export function StageWorkspace({ if (role === undefined || caution === role) return sendNow(); setCaution(role); }; + // The composer is disabled while the decline saves, so the cursor goes there once it can take it. + const [changesAsked, setChangesAsked] = useState(false); const requestChanges = () => { setMobilePane("conversation"); - composerRef.current?.focus(); + setChangesAsked(true); }; + const composerDisabled = sending || approvalBusy || providerMissing; + useEffect(() => { + if (!changesAsked || composerDisabled) return; + setChangesAsked(false); + composerRef.current?.focus(); + }, [changesAsked, composerDisabled]); const unbound = snapshot !== undefined && events === null; const startCoordinator = async () => { @@ -1395,7 +1443,7 @@ export function StageWorkspace({ onSubmit={send} canSubmit={value.trim() !== "" || files.length > 0} status={sending || approvalBusy ? "submitted" : "ready"} - disabled={sending || approvalBusy || providerMissing} + disabled={composerDisabled} > {files.length === 0 ? null : ( diff --git a/apps/web/src/pages/workspace/typed-yes.test.ts b/apps/web/src/pages/workspace/typed-yes.test.ts index ea880b9c..6e34a349 100644 --- a/apps/web/src/pages/workspace/typed-yes.test.ts +++ b/apps/web/src/pages/workspace/typed-yes.test.ts @@ -44,6 +44,27 @@ test("sending a message while a coordinator approval is open denies it first", a expect(steps).toEqual(["decide:appr-design:no", "deliver"]); }); +test("a message sent while a coordinator approval is open is the decline's reason, not separate mail", async () => { + const steps: string[] = []; + const sent = await deliverWithOpenApproval({ + openApprovalId: "appr-brief", + decide: async () => { + steps.push("decide"); + return null; + }, + deliver: async () => { + steps.push("deliver"); + return true; + }, + decline: async (approvalId) => { + steps.push(`decline:${approvalId}`); + return true; + }, + }); + expect(sent).toBe(true); + expect(steps).toEqual(["decline:appr-brief"]); +}); + test("sending a message when no coordinator approval is open delivers directly", async () => { const steps: string[] = []; const sent = await deliverWithOpenApproval({ @@ -78,7 +99,7 @@ test("a failed denial stops before delivering the message", async () => { expect(steps).toEqual(["decide"]); }); -test("choosing an approach approves a matching ask without denying it and denies an unrelated open ask", async () => { +test("choosing an approach approves a matching ask, and declines an unrelated open ask with the choice as its reason", async () => { const matchingSteps: string[] = []; const matchingFault = await chooseApproachWithOpenApproval({ askedApprovalId: "appr-approaches", @@ -91,6 +112,10 @@ test("choosing an approach approves a matching ask without denying it and denies matchingSteps.push("deliver"); return true; }, + decline: async (approvalId) => { + matchingSteps.push(`decline:${approvalId}`); + return true; + }, notSent: "not sent", }); expect(matchingFault).toBeNull(); @@ -108,10 +133,14 @@ test("choosing an approach approves a matching ask without denying it and denies unrelatedSteps.push("deliver"); return true; }, + decline: async (approvalId) => { + unrelatedSteps.push(`decline:${approvalId}`); + return true; + }, notSent: "not sent", }); expect(unrelatedFault).toBeNull(); - expect(unrelatedSteps).toEqual(["decide:appr-design:no", "deliver"]); + expect(unrelatedSteps).toEqual(["decline:appr-design"]); }); test("sending attached files while a coordinator approval is open denies it before attaching materials", async () => { diff --git a/apps/web/src/pages/workspace/typed-yes.tsx b/apps/web/src/pages/workspace/typed-yes.tsx index 3ed7054a..fc95220c 100644 --- a/apps/web/src/pages/workspace/typed-yes.tsx +++ b/apps/web/src/pages/workspace/typed-yes.tsx @@ -99,13 +99,16 @@ export function useTypedYes({ } /** - * Sends a message while a coordinator approval card may be open: denies the open card first - * so the parked coordinator wakes to read the message. + * Sends a message while a coordinator approval card may be open. The parked coordinator wakes + * only once the card is declined: `decline` sends the message as the decline's reason, so the + * coordinator reads the person's words as their answer; without it the card is declined + * first and the message follows. */ export async function deliverWithOpenApproval({ openApprovalId, decide, deliver, + decline, }: { openApprovalId: string | undefined; decide: (decision: { @@ -113,7 +116,10 @@ export async function deliverWithOpenApproval({ choice: "yes" | "always" | "no"; }) => Promise; deliver: () => Promise; + decline?: (approvalId: string) => Promise; }): Promise { + if (openApprovalId !== undefined && decline !== undefined) + return decline(openApprovalId); if (openApprovalId !== undefined) { const fault = await decide({ approvalId: openApprovalId, choice: "no" }); if (fault !== null) return false; @@ -123,13 +129,15 @@ export async function deliverWithOpenApproval({ /** * Picks an approach while a coordinator approval card may be open: approves when the open - * card asks about the chosen version, or denies any other open coordinator card first. + * card asks about the chosen version, or declines any other open coordinator card with the + * choice as the person's reason. */ export async function chooseApproachWithOpenApproval({ askedApprovalId, openApprovalId, decide, deliver, + decline, notSent, }: { askedApprovalId: string | undefined; @@ -139,15 +147,14 @@ export async function chooseApproachWithOpenApproval({ choice: "yes" | "always" | "no"; }) => Promise; deliver: () => Promise; + decline: (approvalId: string) => Promise; notSent: string; }): Promise { if (askedApprovalId !== undefined) { const fault = await decide({ approvalId: askedApprovalId, choice: "yes" }); if (fault !== null) return fault; - } else if (openApprovalId !== undefined) { - const fault = await decide({ approvalId: openApprovalId, choice: "no" }); - if (fault !== null) return fault; - } + } else if (openApprovalId !== undefined) + return (await decline(openApprovalId)) ? null : notSent; return (await deliver()) ? null : notSent; } diff --git a/test/fixtures/pipeline/runs/coordinator-1/exchanges/13/expect.json b/test/fixtures/pipeline/runs/coordinator-1/exchanges/13/expect.json index d1709e04..84c6a0ac 100644 --- a/test/fixtures/pipeline/runs/coordinator-1/exchanges/13/expect.json +++ b/test/fixtures/pipeline/runs/coordinator-1/exchanges/13/expect.json @@ -15,7 +15,7 @@ "callId": "coord_approve_approaches_v1", "toolName": "request_approval", "isError": true, - "contains": "I think Approach B is the best" + "contains": "Their feedback is: I think Approach B is the best" } ] }