mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
fix(evals): observe visible current skill-review completion
Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
DottaandPaperclip committed
1 parent
b6dec3af08
commit
cece850120
3 files changed
+110
-5
No files matched your search
@@ -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("");
|
||||
});
|
||||
@@ -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);
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in new issue
Block a user