From 17bdbeb0cc379a1c1ac74adc9edfc8cc03c7892e Mon Sep 17 00:00:00 2001 From: Dotta Date: Wed, 4 Nov 2026 17:14:08 -0600 Subject: [PATCH] fix(evals): preserve independent hire revision observations Apply only the reviewed delta from 1590aa72a11af5b2bc3334f63e399947d8655329 to 2490177cb302e2ddee7eb226cb17e266ecc64e33; the original implementation and future v4 metadata are already integrated. Co-Authored-By: Paperclip --- .../identity-hire-review-readiness.test.ts | 40 ++++++++++++++++--- .../identity-hire-revision.test.ts | 36 ++++++++++++++++- .../identity-hire-revision.ts | 11 +++-- 3 files changed, 77 insertions(+), 10 deletions(-) diff --git a/tests/runner-e2e/chat-catalog-domains/identity-hire-review-readiness.test.ts b/tests/runner-e2e/chat-catalog-domains/identity-hire-review-readiness.test.ts index 6c1e7449707..5bad5b087e7 100644 --- a/tests/runner-e2e/chat-catalog-domains/identity-hire-review-readiness.test.ts +++ b/tests/runner-e2e/chat-catalog-domains/identity-hire-review-readiness.test.ts @@ -1,6 +1,8 @@ import { afterEach, expect, it, vi } from "vitest"; import type { ChatCatalogContext } from "../chat-catalog-contract.js"; const clock = vi.hoisted(() => ({ now: 30_000, deadlines: [] as number[] })); +const observations = vi.hoisted(() => ({ read: vi.fn() })); +vi.mock("./identity-support.js", async importOriginal => ({ ...await importOriginal(), observeIdentity: observations.read })); vi.mock("../codex-subscription-fixture.js", () => ({ codexSubscriptionAgentConfig: (value: unknown) => value })); vi.mock("@playwright/test", () => ({ expect: (node: { visible?: () => boolean; text?: () => string }) => ({ toBeVisible: async () => { if (!node.visible?.()) throw Error("Actual pending proposal is not visible"); }, @@ -17,8 +19,8 @@ vi.mock("../api.js", () => ({ pollUntil: async (options: { deadlineAt: number; l throw Error("Controlled original observation expired"); } })); import { hireRevisionIdentityCases } from "./identity-hire-revision.js"; -afterEach(() => { vi.restoreAllMocks(); document.body.replaceChildren(); }); -type Options = { deadline?: number | "missing"; readyAt?: number; href?: string; wrongComment?: boolean; foreignSource?: boolean; wrongActor?: boolean; wrongNativeComment?: boolean; wrongModel?: boolean; notPending?: boolean; lateDom?: boolean }; +afterEach(() => { vi.restoreAllMocks(); observations.read.mockReset(); document.body.replaceChildren(); }); +type Options = { deadline?: number | "missing"; readyAt?: number; href?: string; wrongComment?: boolean; foreignSource?: boolean; wrongActor?: boolean; wrongNativeComment?: boolean; wrongModel?: boolean; notPending?: boolean; lateDom?: boolean; progress?: boolean }; function fixture(options: Options = {}) { clock.now = 30_000; clock.deadlines.splice(0); vi.spyOn(Date, "now").mockImplementation(() => clock.now); vi.spyOn(Element.prototype, "getClientRects").mockImplementation(() => [{ width: 10, height: 10 }] as unknown as DOMRectList); @@ -27,10 +29,11 @@ function fixture(options: Options = {}) { const readyAt = options.readyAt ?? 210_000, href = options.href ?? "/approvals/approval"; const run = { id: "native", companyId: "company", agentId: options.wrongActor ? "other" : "ceo", runtimeMode: "native", status: "succeeded", contextSnapshot: { issueId: source, commentId: options.wrongNativeComment ? "other" : "human" } }; const comments = [{ id: "human", companyId: "company", issueId: source, authorUserId: "person", body: query }, - { id: "reply", companyId: "company", issueId: source, authorAgentId: "ceo", createdByRunId: "native", body: "Revised pending proposal." }]; + ...(options.progress ? [{ id: "progress", companyId: "company", issueId: source, authorAgentId: "ceo", createdByRunId: "native", body: "I am checking this proposal before revising it." }] : []), + { id: "reply", companyId: "company", issueId: source, authorAgentId: "ceo", createdByRunId: "native", body: "Revised to requested-model, pending proposal." }]; function sync() { - document.body.innerHTML = '
'; - if (clock.now >= readyAt) { const a = document.createElement("a"); a.setAttribute("href", href); a.textContent = "pending approval"; document.querySelector('div:last-child')!.append(a); } + document.body.innerHTML = '
' + (options.progress ? '
' : '') + '
'; + if (clock.now >= readyAt) { const a = document.createElement("a"); a.setAttribute("href", href); a.textContent = "pending approval"; document.querySelector('div:last-child')!.append(a); if (options.progress) document.getElementById("comment-progress")!.append(a.cloneNode(true)); } } type Nodes = { nodes: Element[]; locator(selector: string): Nodes; first(): Nodes; filter(options: { has: Nodes }): Nodes; evaluateAll(read: (nodes: Element[]) => T): Promise; click(): Promise; text(): string }; function nodes(list: Element[]): Nodes { return { nodes: list, locator: selector => nodes(list.flatMap(n => Array.from(n.querySelectorAll(selector)))), first: () => nodes(list.slice(0,1)), @@ -70,3 +73,30 @@ it("accepts the same owned company-prefixed ordinary approval route", async () = it.each([{ wrongModel: true }, { notPending: true }])("does not turn review visibility into a wrong model/approved proposal PASS %j", async options => { const f = fixture({ readyAt:30_000, ...options }); await expect(hireRevisionIdentityCases[0]!.act!(f.ctx,f.state)).rejects.toThrow(); }); + +it("captures the model-specific bound revision note when progress also renders the same proposal link", async () => { + const f = fixture({ readyAt: 30_000, progress: true }); + await hireRevisionIdentityCases[0]!.act!(f.ctx, f.state); + expect(f.state).toMatchObject({ revisionHistoryComment: { id: "reply", body: "Revised to requested-model, pending proposal." }, + revisionHistoryLink: { commentId: "reply", href: "/approvals/approval" } }); +}); + +it("rereads the actual source comment independently of the captured history receipt", async () => { + const captured = { id: "reply", issueId: "source", companyId: "company", createdByRunId: "native", authorAgentId: "ceo", body: "Revised model, pending approval." }; + const observed = structuredClone(captured), child = { id: "child-comment", issueId: "child", body: "Child history." }; + const state = { sourceIssueId: "source", originalApproval: { id: "approval", payload: { adapterConfig: { model: "old" } } }, revisionHistoryComment: captured }; + observations.read.mockResolvedValue({ comments: [child], state }); + const get = vi.fn(async (url: string) => { + if (url === "/api/approvals/approval") return { id: "approval", payload: { adapterConfig: { model: "new" } } }; + if (url === "/api/approvals/approval/comments") return []; + if (url === "/api/issues/source/comments?order=asc") return [observed]; + throw Error("Unexpected observation read"); + }); + const ctx = { input: { api: { get } } } as unknown as ChatCatalogContext; + const result = await hireRevisionIdentityCases[0]!.observe!(ctx, state) as { comments: typeof captured[] }; + expect(get).toHaveBeenCalledWith("/api/issues/source/comments?order=asc"); + expect(result.comments).toEqual([child, captured]); + expect(result.comments[1]).not.toBe(captured); + observed.body = "Changed independent observation"; + expect(captured.body).toBe("Revised model, pending approval."); +}); diff --git a/tests/runner-e2e/chat-catalog-domains/identity-hire-revision.test.ts b/tests/runner-e2e/chat-catalog-domains/identity-hire-revision.test.ts index 83c81d0fcb9..1603d81727d 100644 --- a/tests/runner-e2e/chat-catalog-domains/identity-hire-revision.test.ts +++ b/tests/runner-e2e/chat-catalog-domains/identity-hire-revision.test.ts @@ -20,16 +20,43 @@ function validChatRevision(): IdentityObservation { o.runs = [{ id: "native", companyId: "acorn", agentId: "ceo", status: "succeeded", runtimeMode: "native", nativeIssueId: "source", resultJson: { finalizationPhase: "committed" }, contextSnapshot: { source: "issue.comment", issueId: "source", commentId: "human", paperclipWake: { comments: [{ id: "human", issueId: "source", body: query, author: { type: "user", id: "alex" } }] } } }]; - o.state.revisionHistoryComment = { id: "bound-reply", companyId: "acorn", issueId: "source", authorAgentId: "ceo", authorUserId: null, createdByRunId: "native", body: o.reply }; + const comment = { id: "bound-reply", companyId: "acorn", issueId: "source", authorAgentId: "ceo", authorUserId: null, createdByRunId: "native", body: o.reply }; + o.comments = [structuredClone(comment)]; + o.state.revisionHistoryComment = structuredClone(comment); + Object.assign(o.company, { issuePrefix: "ACORN" }); o.renderedLinkEvidence = [{ threadTestId: "task-chat-thread", source: "reply", href: "/approvals/approval", conversationId: "source", anchorCommentId: "bound-reply", commentId: "bound-reply" }]; - o.state.revisionHistoryLink = o.renderedLinkEvidence[0]; + o.state.revisionHistoryLink = structuredClone(o.renderedLinkEvidence[0]); return o; } describe("formal hire remains a proposal after cost revision", () => { it("accepts retained original history and the exact bound native revision reply without an extra approval comment", () => { expect(gradeHireRevision(validChatRevision()).passed).toBe(true); }); + it("accepts a bound revision reply alongside progress from the same run", () => { + const o = validChatRevision(); + const progress = { ...structuredClone(o.comments[0]!), id: "progress", body: "I am checking the revised proposal." }; + o.comments.unshift(progress); + o.reply = o.comments.map(row => row.body).join("\n\n"); + expect(gradeHireRevision(o).passed).toBe(true); + }); + it("accepts an earlier bound revision note when the final message only asks for board review", () => { + const o = validChatRevision(); + o.comments.push({ ...structuredClone(o.comments[0]!), id: "final", body: "It remains pending board approval." }); + o.reply = o.comments.map(row => row.body).join("\n\n"); + expect(gradeHireRevision(o).passed).toBe(true); + }); + it("keeps captured comment and link receipts separate from independently observed evidence", () => { + const o = validChatRevision(); + expect(o.state.revisionHistoryComment).not.toBe(o.comments[0]); + expect(o.state.revisionHistoryLink).not.toBe(o.renderedLinkEvidence![0]); + const observed = structuredClone({ comments: o.comments, links: o.renderedLinkEvidence }); + (o.state.revisionHistoryComment as { body: string }).body = "Changed captured receipt only"; + (o.state.revisionHistoryLink as { href: string }).href = "/ACORN/approvals/approval"; + expect({ comments: o.comments, links: o.renderedLinkEvidence }).toEqual(observed); + expect(gradeHireRevision(o).passed).toBe(false); + }); it.each([ + ["unobserved captured message", (o: IdentityObservation) => { o.comments = []; }], ["unbound prose", (o: IdentityObservation) => { delete o.state.revisionHistoryComment; }], ["wrong comment company", (o: IdentityObservation) => { (o.state.revisionHistoryComment as { companyId: string }).companyId = "other"; }], ["wrong comment source", (o: IdentityObservation) => { (o.state.revisionHistoryComment as { issueId: string }).issueId = "other"; }], @@ -42,6 +69,11 @@ describe("formal hire remains a proposal after cost revision", () => { ["foreign run source", (o: IdentityObservation) => { o.runs[0]!.nativeIssueId = "other"; }], ["wrong human wake", (o: IdentityObservation) => { o.runs[0]!.contextSnapshot!.commentId = "other"; }], ["unobserved rendered link", (o: IdentityObservation) => { o.renderedLinkEvidence = []; }], + ["different canonical receipt href", (o: IdentityObservation) => { (o.state.revisionHistoryLink as { href: string }).href = "/ACORN/approvals/approval"; }], + ["wrong captured link source", (o: IdentityObservation) => { (o.state.revisionHistoryLink as { source: string }).source = "project-created-card"; }], + ["wrong captured link conversation", (o: IdentityObservation) => { (o.state.revisionHistoryLink as { conversationId: string }).conversationId = "other"; }], + ["wrong captured link comment", (o: IdentityObservation) => { (o.state.revisionHistoryLink as { commentId: string }).commentId = "other"; }], + ["changed captured message", (o: IdentityObservation) => { (o.state.revisionHistoryComment as { body: string }).body += " Unobserved receipt text"; }], ["wrong proposal link", (o: IdentityObservation) => { (o.state.revisionHistoryLink as { href: string }).href = "/approvals/other"; }], ["foreign link origin", (o: IdentityObservation) => { (o.state.revisionHistoryLink as { href: string }).href = "https://other.test/approvals/approval"; }], ["missing original history", (o: IdentityObservation) => { o.state.approvalComments = []; }], diff --git a/tests/runner-e2e/chat-catalog-domains/identity-hire-revision.ts b/tests/runner-e2e/chat-catalog-domains/identity-hire-revision.ts index d6d14ca5bc1..c6571ee4d87 100644 --- a/tests/runner-e2e/chat-catalog-domains/identity-hire-revision.ts +++ b/tests/runner-e2e/chat-catalog-domains/identity-hire-revision.ts @@ -14,12 +14,14 @@ function hasBoundRevisionHistory(o: IdentityObservation, approval: Approval | un const sourceId = o.state.sourceIssueId; if (!approval || !comment || !link || typeof sourceId !== "string" || o.runs.length !== 1 || o.humanQueries.length !== 1 || comment.companyId !== o.companyId || comment.issueId !== sourceId || comment.authorAgentId !== o.leadId || comment.authorUserId - || typeof comment.body !== "string" || comment.body !== o.reply || typeof o.state.cheaperModel !== "string" + || typeof comment.body !== "string" || typeof o.state.cheaperModel !== "string" || !comment.body.includes(o.state.cheaperModel)) return false; const run = o.runs[0]!; if (run.id !== comment.createdByRunId || run.companyId !== o.companyId || run.agentId !== o.leadId || run.runtimeMode !== "native" || run.status !== "succeeded" || run.resultJson?.finalizationPhase !== "committed" || run.nativeIssueId !== sourceId || run.contextSnapshot?.source !== "issue.comment" || run.contextSnapshot.issueId !== sourceId) return false; + if (!o.comments.some(row => row.id === comment.id && row.companyId === o.companyId && row.issueId === sourceId + && row.authorAgentId === o.leadId && !row.authorUserId && row.createdByRunId === run.id && row.body === comment.body)) return false; const wake = run.contextSnapshot.paperclipWake as { comments?: Array<{ id?: string; issueId?: string; body?: string; author?: { type?: string; id?: string } }> } | undefined; if (!wake?.comments?.some(row => row.id === run.contextSnapshot?.commentId && row.issueId === sourceId && row.author?.type === "user" && row.author.id === o.state.humanId && row.body?.trim() === o.humanQueries[0]!.trim())) return false; @@ -104,7 +106,8 @@ export const hireRevisionIdentityCases: readonly ChatCatalogCase[] = [identityDe && run.contextSnapshot?.issueId === sourceId && humans.length === 1 && run.contextSnapshot?.commentId === humans[0]?.id; const link = bound && run ? links.find(row => { if (row.source !== "reply" || row.conversationId !== sourceId || !comments.some(comment => comment.id === row.commentId - && comment.authorAgentId === run.agentId && comment.createdByRunId === run.id)) return false; + && comment.authorAgentId === run.agentId && comment.createdByRunId === run.id + && comment.body?.includes(String(state.cheaperModel)))) return false; try { const url = new URL(row.href, ctx.input.api.baseURL); return url.origin === new URL(ctx.input.api.baseURL).origin && [ `/approvals/${id}`, `/${ctx.input.fixtures.company.issuePrefix}/approvals/${id}` ].includes(url.pathname); } catch { return false; } @@ -132,6 +135,8 @@ export const hireRevisionIdentityCases: readonly ChatCatalogCase[] = [identityDe state.approvalComments = await ctx.input.api.get(`/api/approvals/${id}/comments`); const stripModel = (payload: Record) => { const adapter = payload.adapterConfig as Record; return { ...payload, adapterConfig: { ...adapter, model: undefined } }; }; state.originalOtherFieldsPreserved = JSON.stringify(stripModel((state.originalApproval as Approval).payload)) === JSON.stringify(stripModel(current.payload)); - return observeIdentity(ctx, state); + const sourceComments = await ctx.input.api.get(`/api/issues/${String(state.sourceIssueId)}/comments?order=asc`); + const observation = await observeIdentity(ctx, state); + return { ...observation, comments: [...observation.comments.filter(row => row.issueId !== state.sourceIssueId), ...sourceComments] }; }, })];