Skip to content

Commit 588e29b

Browse files
committed
feat(hub): triage reviewer feedback and forward approved items
1 parent 7f8bd7d commit 588e29b

18 files changed

Lines changed: 4507 additions & 714 deletions

‎apps/hub/src/artifacts/approval-rounds.test.ts‎

Lines changed: 261 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -625,6 +625,231 @@ describe("approval package zip and approval rounds", () => {
625625
);
626626
});
627627

628+
test("extracts triage bullets without messaging the coordinator, and only mails the coordinator when the owner incorporates an item", async () => {
629+
const mailedRequests: Array<{
630+
url: string;
631+
cookie: string | null;
632+
body: { content: string };
633+
}> = [];
634+
const { store } = createFakeStore({ coordinatorRunId: "wfr_coord_99" });
635+
const service = createApprovalRoundService(store);
636+
const app = createTestApp(service, OWNER, async (request) => {
637+
mailedRequests.push({
638+
url: new URL(request.url).pathname,
639+
cookie: request.headers.get("cookie"),
640+
body: (await request.json()) as { content: string },
641+
});
642+
return new Response(JSON.stringify({ ok: true }), { status: 200 });
643+
});
644+
645+
const created = await service.start(OWNER, "proj_1", {
646+
packageVersion: 1,
647+
expiry: "7d",
648+
reviewers: [{ name: "Dana Reviewer", persona: "Finance VP" }],
649+
});
650+
const reviewerId = created.reviewers[0]!.id;
651+
652+
// 1. Recording feedback splits multi-bullet overall notes and per-slide notes into triage items without mailing the coordinator
653+
const decisionRes = await app.request(
654+
`/proj_1/approval-rounds/${created.roundId}/reviewers/${reviewerId}/decision`,
655+
{
656+
method: "POST",
657+
headers: {
658+
"content-type": "application/json",
659+
cookie: "session=owner_cookie",
660+
},
661+
body: JSON.stringify({
662+
outcome: "request_changes",
663+
note: "- Add sensitivity table for Q3.\n- Clarify cloud vendor commits.",
664+
slideNotes: [
665+
{
666+
slide: 4,
667+
text: "Break out contractor headcount by month.",
668+
},
669+
],
670+
}),
671+
},
672+
);
673+
expect(decisionRes.status).toBe(200);
674+
const afterDecision = (await decisionRes.json()) as {
675+
triage: Array<{
676+
itemId: string;
677+
reviewerName: string;
678+
persona: string;
679+
slide?: number;
680+
text: string;
681+
status: string;
682+
reply?: string;
683+
}>;
684+
};
685+
expect(afterDecision.triage).toHaveLength(3);
686+
expect(afterDecision.triage.map((item) => item.text)).toEqual([
687+
"Add sensitivity table for Q3.",
688+
"Clarify cloud vendor commits.",
689+
"Break out contractor headcount by month.",
690+
]);
691+
expect(afterDecision.triage[2]?.slide).toBe(4);
692+
expect(afterDecision.triage.map((item) => item.status)).toEqual([
693+
"pending",
694+
"pending",
695+
"pending",
696+
]);
697+
expect(mailedRequests).toHaveLength(0);
698+
699+
const [firstItem, secondItem, slideItem] = afterDecision.triage;
700+
701+
// 2. Dismissing an item updates triage state without messaging the coordinator
702+
const dismissRes = await app.request(
703+
`/proj_1/approval-rounds/${created.roundId}/triage/${firstItem!.itemId}`,
704+
{
705+
method: "POST",
706+
headers: {
707+
"content-type": "application/json",
708+
cookie: "session=owner_cookie",
709+
},
710+
body: JSON.stringify({ action: "dismiss" }),
711+
},
712+
);
713+
expect(dismissRes.status).toBe(200);
714+
const afterDismiss = (await dismissRes.json()) as typeof afterDecision;
715+
expect(
716+
afterDismiss.triage.find((item) => item.itemId === firstItem!.itemId)
717+
?.status,
718+
).toBe("dismissed");
719+
expect(mailedRequests).toHaveLength(0);
720+
721+
// 3. Replying to an item records the owner's reply without messaging the coordinator
722+
const replyRes = await app.request(
723+
`/proj_1/approval-rounds/${created.roundId}/triage/${secondItem!.itemId}`,
724+
{
725+
method: "POST",
726+
headers: {
727+
"content-type": "application/json",
728+
cookie: "session=owner_cookie",
729+
},
730+
body: JSON.stringify({
731+
action: "reply",
732+
reply: "Handled in the appendix contract schedule.",
733+
}),
734+
},
735+
);
736+
expect(replyRes.status).toBe(200);
737+
const afterReply = (await replyRes.json()) as typeof afterDecision;
738+
const replied = afterReply.triage.find(
739+
(item) => item.itemId === secondItem!.itemId,
740+
);
741+
expect(replied?.status).toBe("replied");
742+
expect(replied?.reply).toBe("Handled in the appendix contract schedule.");
743+
expect(mailedRequests).toHaveLength(0);
744+
745+
// 4. Incorporating an item marks it incorporated and mails the coordinator as the owner with the delimited external block
746+
const incorporateRes = await app.request(
747+
`/proj_1/approval-rounds/${created.roundId}/triage/${slideItem!.itemId}`,
748+
{
749+
method: "POST",
750+
headers: {
751+
"content-type": "application/json",
752+
cookie: "session=owner_cookie",
753+
},
754+
body: JSON.stringify({ action: "incorporate" }),
755+
},
756+
);
757+
expect(incorporateRes.status).toBe(200);
758+
const afterIncorporate =
759+
(await incorporateRes.json()) as typeof afterDecision;
760+
expect(
761+
afterIncorporate.triage.find((item) => item.itemId === slideItem!.itemId)
762+
?.status,
763+
).toBe("incorporated");
764+
765+
expect(mailedRequests).toHaveLength(1);
766+
expect(mailedRequests[0]?.url).toBe(
767+
"/api/tenants/tenant_proj_1/workflows/runs/wfr_coord_99/mail",
768+
);
769+
expect(mailedRequests[0]?.cookie).toBe("session=owner_cookie");
770+
expect(mailedRequests[0]?.body.content).toBe(
771+
[
772+
"Please revise the project to incorporate this reviewer feedback:",
773+
"",
774+
"--- reviewer feedback (external, package: v1, audience: Executive committee, persona: Finance VP, reviewer: Dana Reviewer, decision: request_changes, slide: slide 4) ---",
775+
"Break out contractor headcount by month.",
776+
"--- end reviewer feedback ---",
777+
].join("\n"),
778+
);
779+
780+
// 5. Re-triaging an already-incorporated item fails with 409 triage_item_already_incorporated and sends no second coordinator message
781+
const secondIncorporateRes = await app.request(
782+
`/proj_1/approval-rounds/${created.roundId}/triage/${slideItem!.itemId}`,
783+
{
784+
method: "POST",
785+
headers: {
786+
"content-type": "application/json",
787+
cookie: "session=owner_cookie",
788+
},
789+
body: JSON.stringify({ action: "incorporate" }),
790+
},
791+
);
792+
expect(secondIncorporateRes.status).toBe(409);
793+
expect(await secondIncorporateRes.json()).toEqual({
794+
error: "triage_item_already_incorporated",
795+
roundId: created.roundId,
796+
itemId: slideItem!.itemId,
797+
});
798+
expect(mailedRequests).toHaveLength(1);
799+
800+
// 6. Amending the reviewer's decision preserves already-triaged items without duplicating them as pending, while adding newly added bullets as pending
801+
const amendRes = await app.request(
802+
`/proj_1/approval-rounds/${created.roundId}/reviewers/${reviewerId}/decision`,
803+
{
804+
method: "POST",
805+
headers: {
806+
"content-type": "application/json",
807+
cookie: "session=owner_cookie",
808+
},
809+
body: JSON.stringify({
810+
outcome: "request_changes",
811+
note: "- Add sensitivity table for Q3.\n- Include multi-year ARR ramp.",
812+
slideNotes: [
813+
{
814+
slide: 4,
815+
text: "Break out contractor headcount by month.",
816+
},
817+
],
818+
}),
819+
},
820+
);
821+
expect(amendRes.status).toBe(200);
822+
const afterAmend = (await amendRes.json()) as typeof afterDecision;
823+
expect(
824+
afterAmend.triage.map((item) => ({
825+
text: item.text,
826+
slide: item.slide,
827+
status: item.status,
828+
})),
829+
).toEqual([
830+
{
831+
text: "Add sensitivity table for Q3.",
832+
slide: undefined,
833+
status: "dismissed",
834+
},
835+
{
836+
text: "Clarify cloud vendor commits.",
837+
slide: undefined,
838+
status: "replied",
839+
},
840+
{
841+
text: "Break out contractor headcount by month.",
842+
slide: 4,
843+
status: "incorporated",
844+
},
845+
{
846+
text: "Include multi-year ARR ramp.",
847+
slide: undefined,
848+
status: "pending",
849+
},
850+
]);
851+
});
852+
628853
test("saves reviewers marked saveToWorkspace at the workspace level and returns workspaceReviewers on list", async () => {
629854
const { store } = createFakeStore();
630855
const service = createApprovalRoundService(store);
@@ -688,10 +913,15 @@ describe("approval package zip and approval rounds", () => {
688913
expect(rounds.size).toBe(0);
689914
});
690915

691-
test("preserves both reviewers' decisions when recorded concurrently", async () => {
916+
test("preserves both reviewers' decisions when recorded concurrently and prevents concurrent double-incorporate from mailing twice", async () => {
917+
let mailCount = 0;
692918
const { store } = createFakeStore({ coordinatorRunId: "wfr_coord_99" });
693919
const service = createApprovalRoundService(store);
694-
const app = createTestApp(service, OWNER);
920+
const app = createTestApp(service, OWNER, async () => {
921+
mailCount += 1;
922+
await new Promise<void>((resolve) => queueMicrotask(resolve));
923+
return new Response(JSON.stringify({ ok: true }), { status: 200 });
924+
});
695925

696926
const created = await service.start(OWNER, "proj_1", {
697927
packageVersion: 1,
@@ -739,5 +969,34 @@ describe("approval package zip and approval rounds", () => {
739969
round.reviewers.find((r) => r.id === marcus!.id)?.decision?.outcome,
740970
).toBe("approve");
741971
expect(round.triage).toHaveLength(2);
972+
973+
const targetItem = round.triage[0]!;
974+
const [inc1, inc2] = await Promise.all([
975+
app.request(
976+
`/proj_1/approval-rounds/${created.roundId}/triage/${targetItem.itemId}`,
977+
{
978+
method: "POST",
979+
headers: {
980+
"content-type": "application/json",
981+
cookie: "session=owner_cookie",
982+
},
983+
body: JSON.stringify({ action: "incorporate" }),
984+
},
985+
),
986+
app.request(
987+
`/proj_1/approval-rounds/${created.roundId}/triage/${targetItem.itemId}`,
988+
{
989+
method: "POST",
990+
headers: {
991+
"content-type": "application/json",
992+
cookie: "session=owner_cookie",
993+
},
994+
body: JSON.stringify({ action: "incorporate" }),
995+
},
996+
),
997+
]);
998+
const statuses = [inc1.status, inc2.status].sort();
999+
expect(statuses).toEqual([200, 409]);
1000+
expect(mailCount).toBe(1);
7421001
});
7431002
});

0 commit comments

Comments
 (0)