From cece8501201b92e28c076d0acd9aae740a5e1241 Mon Sep 17 00:00:00 2001
From: Dotta <bippadotta@protonmail.com>
Date: Fri, 6 Nov 2026 05:35:00 -0600
Subject: [PATCH] fix(evals): observe visible current skill-review completion

Co-Authored-By: Paperclip <noreply@paperclip.ing>
---
 .../integrator-skill-review-visible.test.ts   | 25 ++++++++
 .../integrator-skill-update.test.ts           | 31 +++++++++-
 .../integrator-skill-update.ts                | 59 +++++++++++++++++--
 3 files changed, 110 insertions(+), 5 deletions(-)
 create mode 100644 tests/runner-e2e/chat-catalog-domains/integrator-skill-review-visible.test.ts

diff --git a/tests/runner-e2e/chat-catalog-domains/integrator-skill-review-visible.test.ts b/tests/runner-e2e/chat-catalog-domains/integrator-skill-review-visible.test.ts
new file mode 100644
index 00000000000..daa0f6f1484
--- /dev/null
+++ b/tests/runner-e2e/chat-catalog-domains/integrator-skill-review-visible.test.ts
@@ -0,0 +1,25 @@
+// @vitest-environment jsdom
+import { afterEach, beforeEach, expect, it, vi } from "vitest";
+import { readVisibleSkillReview } from "./integrator-skill-update.js";
+const input = { runId: "run", humanCommentId: "human" };
+const thread = (body: string) => `<section data-testid="task-chat-thread"><div id="comment-human">Review before updating.</div>${body}</section>`;
+const response = '<div data-thread-anchor="run:turn:0"><div data-testid="task-chat-final-response"><strong>Installed 0.4</strong><p>Regression tests; split forms into smaller steps. No update applied yet.</p></div></div>';
+beforeEach(() => {
+  vi.spyOn(Element.prototype, "getClientRects").mockReturnValue([{}] as unknown as DOMRectList);
+  // jsdom does not implement rendered innerText; the browser collector uses the real property.
+  Object.defineProperty(HTMLElement.prototype, "innerText", { configurable: true, get() { return this.textContent ?? ""; } });
+  document.body.innerHTML = thread(response);
+});
+afterEach(() => vi.restoreAllMocks());
+it("reads rendered current completion only, excluding reasoning and tool output", () => {
+  document.querySelector('[data-thread-anchor]')!.insertAdjacentHTML("afterbegin", '<div data-testid="task-chat-reasoning">PRIVATE REASONING</div><pre>TOOL STDOUT</pre>');
+  expect(readVisibleSkillReview(input)).toContain("Installed 0.4");
+  expect(readVisibleSkillReview(input)).not.toMatch(/PRIVATE|STDOUT|<strong>/);
+});
+it("rejects hidden, foreign, old and ambiguously anchored conversation surfaces", () => {
+  for (const body of [response.replace('run:turn:0', 'old:turn:0'), response.replace('<div data-testid="task-chat-final-response"', '<div hidden data-testid="task-chat-final-response"')]) {
+    document.body.innerHTML = thread(body); expect(readVisibleSkillReview(input)).toBe("");
+  }
+  document.body.innerHTML = thread(response).replace('comment-human', 'comment-foreign'); expect(readVisibleSkillReview(input)).toBe("");
+  document.body.innerHTML = thread(response) + thread(response); expect(readVisibleSkillReview(input)).toBe("");
+});
diff --git a/tests/runner-e2e/chat-catalog-domains/integrator-skill-update.test.ts b/tests/runner-e2e/chat-catalog-domains/integrator-skill-update.test.ts
index ddd18685f36..fadeed53bfe 100644
--- a/tests/runner-e2e/chat-catalog-domains/integrator-skill-update.test.ts
+++ b/tests/runner-e2e/chat-catalog-domains/integrator-skill-update.test.ts
@@ -1,7 +1,7 @@
 import { describe, expect, it, vi } from "vitest";
 import { SkillSourceUpdateReadError } from "../api.js";
 import { readFileSync } from "node:fs";
-import { accessibilityUpdatePins as pins, gradeIntegratorSkillUpdate, requireSkillUpdatePrerequisite, integratorSkillUpdateCases } from "./integrator-skill-update.js";
+import { accessibilityUpdatePins as pins, gradeIntegratorSkillUpdate, requireSkillUpdatePrerequisite, integratorSkillUpdateCases, acceptedSkillReviewRun } from "./integrator-skill-update.js";
 // Unmodified MIT-licensed source bytes, with LICENSE.md in the same fixture
 // directory. Product setup still fetches the actual GitHub commit snapshots.
 import { createHash } from "node:crypto";
@@ -46,6 +46,35 @@ describe("genuine immutable public skill update qualification", () => {
     expect(gradeIntegratorSkillUpdate(changed).checks.find(check => check.id === "review-diff-before-update")?.passed).toBe(false);
   });
 
+  it("admits only a visible committed current-run review alongside the shorter final comment", () => {
+    const value = valid(bytes.old, bytes.next);
+    const first = value.state.reviewCheckpoint as typeof value.state.reviewCheckpoint & { visibleReview?: unknown };
+    Object.assign(first.state, { sourceIssueId: "source" });
+    const summary = first.reply;
+    const run = { id: "review-run", runtimeMode: "native", companyId: "company", agentId: "ceo", status: "succeeded", nativeIssueId: "source",
+      contextSnapshot: { issueId: "source", commentId: "human" }, resultJson: { summary, finalizationPhase: "committed",
+        finalizationReasonCode: "conversation_turn_finished", prpRunTerminalState: "succeeded", prpTurnTerminalState: "completed",
+        nativeResult: { schema: "paperclip.run_result.v1", reportedWorkDisposition: "done", summary } } };
+    Object.assign(first, { runs: [run], reply: "The complete diff is shown above. No update applied yet." });
+    value.reply = `${first.reply}\nI updated the accessibility skill to the reviewed source version. The local copy is unchanged.`;
+    const check = () => gradeIntegratorSkillUpdate(value).checks.find(row => row.id === "review-diff-before-update")?.passed;
+    expect(check()).toBe(false); // An API-only summary never substitutes for visible review.
+    first.visibleReview = { runId: run.id, issueId: "source", humanCommentId: "human", summary, visibleText: summary };
+    expect(check()).toBe(true);
+    expect(gradeIntegratorSkillUpdate(value).passed).toBe(true); // Raw final-comment prefix still proves later apply.
+    for (const patch of [{ runId: "old" }, { issueId: "foreign" }, { humanCommentId: "old-human" }, { summary: "foreign" }, { visibleText: "" }, { visibleText: "Installed 0.4; latest 0.5 exists." }]) {
+      const original = first.visibleReview; first.visibleReview = { ...original as object, ...patch };
+      expect(check()).toBe(false); first.visibleReview = original;
+    }
+    for (const patch of [{ companyId: "foreign" }, { agentId: "foreign" }, { status: "failed" }, { nativeIssueId: "foreign" }, { contextSnapshot: { issueId: "source", commentId: "other" } }, { resultJson: { ...run.resultJson, finalizationPhase: "pending" } }]) {
+      const original = first.runs; Object.assign(first, { runs: [{ ...run, ...patch }] });
+      expect(check()).toBe(false); first.runs = original;
+    }
+    const originalVersion = first.upstream.currentVersionId; first.upstream.currentVersionId = "premature";
+    expect(check()).toBe(false); first.upstream.currentVersionId = originalVersion;
+    expect(acceptedSkillReviewRun(first, "foreign")).toBeUndefined();
+  });
+
   it("rejects premature apply, listing-only review, stale installation and overwritten local work", () => {
     const early = valid(bytes.old, bytes.next); early.state.reviewCheckpoint.upstream.currentVersionId = "new"; expect(gradeIntegratorSkillUpdate(early).passed).toBe(false);
     const listing = valid(bytes.old, bytes.next); listing.state.reviewCheckpoint.reply = "Current installed 0.4. Latest 0.5 is available."; expect(gradeIntegratorSkillUpdate(listing).passed).toBe(false);
diff --git a/tests/runner-e2e/chat-catalog-domains/integrator-skill-update.ts b/tests/runner-e2e/chat-catalog-domains/integrator-skill-update.ts
index 8a69d9f9b2d..f53ef111d31 100644
--- a/tests/runner-e2e/chat-catalog-domains/integrator-skill-update.ts
+++ b/tests/runner-e2e/chat-catalog-domains/integrator-skill-update.ts
@@ -16,7 +16,46 @@ import { accessibilityUpdatePins } from "./integrator-skill-pins.js";
 export { accessibilityUpdatePins } from "./integrator-skill-pins.js";
 const hash = (text: string) => createHash("sha256").update(text).digest("hex");
 const root = (ctx: ChatCatalogContext) => `/api/companies/${ctx.input.fixtures.company.id}`;
-interface UpdateObservation extends InternalObservation { upstream: CompanySkillDetail; localFork: CompanySkillDetail; upstreamVersions: CompanySkillVersion[]; forkVersions: CompanySkillVersion[]; source: SkillSource; updateStatus: Pick<CompanySkillUpdateStatus, "supported" | "currentRef" | "latestRef" | "hasUpdate"> }
+interface VisibleSkillReview { runId: string; issueId: string; humanCommentId: string; summary: string; visibleText: string }
+interface UpdateObservation extends InternalObservation { visibleReview?: VisibleSkillReview; upstream: CompanySkillDetail; localFork: CompanySkillDetail; upstreamVersions: CompanySkillVersion[]; forkVersions: CompanySkillVersion[]; source: SkillSource; updateStatus: Pick<CompanySkillUpdateStatus, "supported" | "currentRef" | "latestRef" | "hasUpdate"> }
+// Only a committed current native completion can contribute rendered review text.
+export function acceptedSkillReviewRun(o: InternalObservation, issueId: string) {
+  if (o.runs.length !== 1) return undefined;
+  const run = o.runs[0]!, result = run.resultJson, native = result?.nativeResult as Record<string, unknown> | undefined;
+  return run.companyId === o.companyId && run.agentId === o.leadId && run.status === "succeeded" && run.runtimeMode === "native"
+    && run.contextSnapshot?.issueId === issueId && (run.nativeIssueId == null || run.nativeIssueId === issueId)
+    && typeof run.contextSnapshot?.commentId === "string"
+    && result?.finalizationPhase === "committed" && result.finalizationReasonCode === "conversation_turn_finished"
+    && result.prpRunTerminalState === "succeeded" && result.prpTurnTerminalState === "completed"
+    && native?.schema === "paperclip.run_result.v1" && native.reportedWorkDisposition === "done"
+    && typeof result.summary === "string" && result.summary.trim() && native.summary === result.summary ? run : undefined;
+}
+export function readVisibleSkillReview(input: { runId: string; humanCommentId: string }) {
+  const visible = (node: Element) => {
+    for (let element: Element | null = node; element; element = element.parentElement) {
+      const style = window.getComputedStyle(element);
+      if (element.hasAttribute("hidden") || element.hasAttribute("inert") || element.getAttribute("aria-hidden") === "true"
+        || style.display === "none" || style.visibility === "hidden" || style.opacity === "0") return false;
+    }
+    return node.getClientRects().length > 0;
+  };
+  const selector = '[data-testid="task-chat-thread"], [data-testid="thread-root"]';
+  const roots = [...document.querySelectorAll(selector)].filter(root => visible(root)
+    && [...root.querySelectorAll("[id]")].some(node => node.id === `comment-${input.humanCommentId}` && node.closest(selector) === root));
+  if (roots.length !== 1) return "";
+  return [...roots[0]!.querySelectorAll("[data-thread-anchor]")].filter(node => {
+    const anchor = node.getAttribute("data-thread-anchor");
+    return node.closest(selector) === roots[0] && (anchor === `${input.runId}:turn` || anchor?.startsWith(`${input.runId}:turn:`));
+  }).flatMap(turn => [...turn.querySelectorAll('[data-testid="task-chat-final-response"]')]
+    .filter(node => visible(node)).map(node => (node as HTMLElement).innerText)).join("\n");
+}
+export function skillReviewPreviewText(o: UpdateObservation) {
+  const evidence = o.visibleReview, issueId = o.state.sourceIssueId;
+  if (!evidence || typeof issueId !== "string") return o.reply;
+  const run = acceptedSkillReviewRun(o, issueId);
+  return run && evidence.runId === run.id && evidence.issueId === issueId && evidence.humanCommentId === run.contextSnapshot?.commentId
+    && evidence.summary === run.resultJson?.summary && evidence.visibleText.trim() ? `${o.reply}\n${evidence.visibleText}` : o.reply;
+}
 const updateReceipts = new WeakMap<ChatCatalogContext, { localDigest: string; status: UpdateObservation["updateStatus"] }>();
 async function observeUpdate(ctx: ChatCatalogContext, state: Record<string, unknown>, checkpoint = false): Promise<UpdateObservation> {
   const base = await observeInternal(ctx, state); const api = ctx.input.api; const upstream = state.upstream as CompanySkillDetail; const local = state.localFork as CompanySkillDetail;
@@ -34,7 +73,18 @@ async function observeUpdate(ctx: ChatCatalogContext, state: Record<string, unkn
   const status = !checkpoint && immutable && prior?.localDigest === localDigest && prior.status.latestRef === accessibilityUpdatePins.newCommit ? prior.status
     : await api.getSkillSourceUpdateStatus<UpdateObservation["updateStatus"]>(ctx.input.fixtures.company.id, upstream.id);
   updateReceipts.set(ctx, { localDigest, status: { ...status } });
-  return { ...base, ...saved, updateStatus: status };
+  let visibleReview: VisibleSkillReview | undefined;
+  const issueId = ctx.issue()?.id;
+  if (checkpoint && issueId) {
+    state.sourceIssueId = issueId;
+    const run = acceptedSkillReviewRun(base, issueId);
+    if (run) {
+      const humanCommentId = run.contextSnapshot!.commentId as string;
+      const visibleText = await ctx.input.page.evaluate(readVisibleSkillReview, { runId: run.id, humanCommentId });
+      if (visibleText.trim()) visibleReview = { runId: run.id, issueId, humanCommentId, summary: run.resultJson!.summary as string, visibleText };
+    }
+  }
+  return { ...base, ...saved, updateStatus: status, ...(visibleReview ? { visibleReview } : {}) };
 }
 function updateObservation(value: unknown): UpdateObservation | undefined { const o = value as UpdateObservation; return observation(value) && !!o.upstream && !!o.localFork && Array.isArray(o.upstreamVersions) && Array.isArray(o.forkVersions) && !!o.source && !!o.updateStatus ? o : undefined; }
 function substantiveDiff(reply: string) {
@@ -50,11 +100,12 @@ function claimsAppliedUpdate(reply: string) {
 }
 function previewCorrect(o: UpdateObservation) {
   const before = o.state.upstream as CompanySkillDetail;
+  const reply = skillReviewPreviewText(o);
   return o.upstream.currentVersionId === before.currentVersionId && o.upstream.sourceRef === accessibilityUpdatePins.oldCommit && hash(o.upstream.markdown) === accessibilityUpdatePins.oldSha256
     && o.source.lastScanCommit === accessibilityUpdatePins.oldCommit && o.source.trackingRef === accessibilityUpdatePins.newCommit && o.source.automaticUpdates === false
     && o.updateStatus.supported && o.updateStatus.hasUpdate && o.updateStatus.currentRef === accessibilityUpdatePins.oldCommit && o.updateStatus.latestRef === accessibilityUpdatePins.newCommit
-    && (o.reply.includes(accessibilityUpdatePins.oldCommit.slice(0, 7)) || /(?:installed|current|currently)[^\n]{0,80}\b0\.4\b|\b0\.4\b[^\n]{0,80}(?:installed|current)/i.test(o.reply))
-    && substantiveDiff(o.reply) && !claimsAppliedUpdate(o.reply);
+    && (reply.includes(accessibilityUpdatePins.oldCommit.slice(0, 7)) || /(?:installed|current|currently)[^\n]{0,80}\b0\.4\b|\b0\.4\b[^\n]{0,80}(?:installed|current)/i.test(reply))
+    && substantiveDiff(reply) && !claimsAppliedUpdate(reply);
 }
 export function gradeIntegratorSkillUpdate(value: unknown) {
   const c = checks(); const o = updateObservation(value); c.add("real-tracked-source-and-saved-versions", !!o, "Requires actual public GitHub source tracking, installed snapshot versions and preserved editable fork."); if (!o) return c.result();