fix(evals): retain complete persisted skill-review path

Co-Authored-By: Paperclip <noreply@paperclip.ing>

tests/runner-e2e/chat-catalog-domains/integrator-skill-review-visible.test.ts

@@ -1,6 +1,6 @@
 // @vitest-environment jsdom
 import { afterEach, beforeEach, expect, it, vi } from "vitest";
-import { readVisibleSkillReview, waitForVisibleSkillReview } from "./integrator-skill-update.js";
+import { readVisibleSkillReview, waitForVisibleSkillReview, collectVisibleSkillReviewFallback } 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>';
@@ -29,11 +29,16 @@ it("waits for eventual current response hydration within the original deadline o
   document.body.innerHTML = thread("");
   const evaluate = vi.fn(async () => readVisibleSkillReview(input));
   const ctx = { observationDeadlineAt: 2_000, input: { page: { evaluate } } } as unknown as Parameters<typeof waitForVisibleSkillReview>[0];
-  const pending = waitForVisibleSkillReview(ctx, input);
+  const summary = "Installed 0.4. Regression tests and forms split into small steps.";
+  const base = { reply: "See the complete diff above. No update applied yet.", companyId: "company", leadId: "ceo",
+    runs: [{ id: "run", companyId: "company", agentId: "ceo", status: "succeeded", runtimeMode: "native", 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 } } }] } as unknown as Parameters<typeof collectVisibleSkillReviewFallback>[1];
+  const pending = collectVisibleSkillReviewFallback(ctx, base, "source");
   await vi.advanceTimersByTimeAsync(250);
   document.body.innerHTML = thread(response);
   await vi.advanceTimersByTimeAsync(250);
-  expect(await pending).toContain("Installed 0.4");
+  expect((await pending)?.visibleText).toContain("Installed 0.4");
   expect(evaluate).toHaveBeenCalledTimes(3);
   expect(ctx.observationDeadlineAt).toBe(2_000);
   document.body.innerHTML = thread(response.replace('run:turn:0', 'foreign:turn:0'));

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, acceptedSkillReviewRun } from "./integrator-skill-update.js";
+import { accessibilityUpdatePins as pins, gradeIntegratorSkillUpdate, requireSkillUpdatePrerequisite, integratorSkillUpdateCases, acceptedSkillReviewRun, collectVisibleSkillReviewFallback } 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,15 @@ describe("genuine immutable public skill update qualification", () => {
     expect(gradeIntegratorSkillUpdate(changed).checks.find(check => check.id === "review-diff-before-update")?.passed).toBe(false);
   });
 
+  it("keeps a complete persisted preview on the original path without any DOM read", async () => {
+    const first = valid(bytes.old, bytes.next).state.reviewCheckpoint;
+    const evaluate = vi.fn(async () => { throw new Error("No additional native bubble exists"); });
+    const ctx = { input: { page: { evaluate } } } as unknown as Parameters<typeof collectVisibleSkillReviewFallback>[0];
+    expect(await collectVisibleSkillReviewFallback(ctx, first, "source")).toBeUndefined();
+    expect(evaluate).not.toHaveBeenCalled();
+    expect(gradeIntegratorSkillUpdate(valid(bytes.old, bytes.next)).passed).toBe(true);
+  });
+
   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 };

tests/runner-e2e/chat-catalog-domains/integrator-skill-update.ts

@@ -84,12 +84,7 @@ async function observeUpdate(ctx: ChatCatalogContext, state: Record<string, unkn
   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 waitForVisibleSkillReview(ctx, { runId: run.id, humanCommentId });
-      if (visibleText.trim()) visibleReview = { runId: run.id, issueId, humanCommentId, summary: run.resultJson!.summary as string, visibleText };
-    }
+    visibleReview = await collectVisibleSkillReviewFallback(ctx, base, issueId);
   }
   return { ...base, ...saved, updateStatus: status, ...(visibleReview ? { visibleReview } : {}) };
 }
@@ -105,14 +100,27 @@ function claimsAppliedUpdate(reply: string) {
   return [...reply.matchAll(/\b(?:I (?:have )?(?:updated|applied|installed)|update (?:is |has been )?(?:applied|installed|complete))\b/gi)]
     .some(match => !/^update /i.test(match[0]) || !/\bno\s+$/i.test(reply.slice(0, match.index)));
 }
+function previewNarrativeCorrect(reply: string) {
+  return (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 async function collectVisibleSkillReviewFallback(ctx: ChatCatalogContext, base: InternalObservation, issueId: string): Promise<VisibleSkillReview | undefined> {
+  // A complete persisted review is already the ordinary user-facing answer.
+  // Its deduplicated native bubble need not exist for the original path to pass.
+  if (previewNarrativeCorrect(base.reply)) return undefined;
+  const run = acceptedSkillReviewRun(base, issueId);
+  if (!run) return undefined;
+  const humanCommentId = run.contextSnapshot!.commentId as string;
+  const visibleText = await waitForVisibleSkillReview(ctx, { runId: run.id, humanCommentId });
+  return { runId: run.id, issueId, humanCommentId, summary: run.resultJson!.summary as string, visibleText };
+}
 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
-    && (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);
+    && previewNarrativeCorrect(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();