From 648d6a3a51305decfcd2e90ef5369241f0e868cf Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 1 Nov 2026 16:09:36 -0800 Subject: [PATCH] sitesmith self-review: never present a claimed fix that sent no ops An empty patch whose message claims a change ("Fixed the hero ...") is now status 'claimed-no-ops' with a warning note: "Sitesmith's reply describes a fix, but it sent no changes, so nothing on your page was changed." The structured render_mismatch flag from newer backends is read first; the existing wording check still covers older replies. Co-Authored-By: Claude Opus 5.5 --- craft/src/panels/sitesmith/SitesmithModal.tsx | 2 +- craft/src/panels/sitesmith/selfReview.test.ts | 39 ++++++++++++++++++- craft/src/panels/sitesmith/selfReview.ts | 26 ++++++++++++- craft/src/types/sitesmith.ts | 3 +- 4 files changed, 66 insertions(+), 4 deletions(-) diff --git a/craft/src/panels/sitesmith/SitesmithModal.tsx b/craft/src/panels/sitesmith/SitesmithModal.tsx index 5cc6b57..82e7ca5 100644 --- a/craft/src/panels/sitesmith/SitesmithModal.tsx +++ b/craft/src/panels/sitesmith/SitesmithModal.tsx @@ -453,7 +453,7 @@ export const SitesmithModal: React.FC = ({ onClose, target }) => { {reviewNote && (
{reviewNote.note}
diff --git a/craft/src/panels/sitesmith/selfReview.test.ts b/craft/src/panels/sitesmith/selfReview.test.ts index c8a344f..2d0cf7f 100644 --- a/craft/src/panels/sitesmith/selfReview.test.ts +++ b/craft/src/panels/sitesmith/selfReview.test.ts @@ -1,5 +1,5 @@ import { describe, test, expect, vi } from 'vitest'; -import { runSelfReview, saysRenderMismatch, REVIEW_MESSAGE, type SelfReviewDeps } from './selfReview'; +import { runSelfReview, saysRenderMismatch, claimsAFix, REVIEW_MESSAGE, type SelfReviewDeps } from './selfReview'; const blob = () => new Blob([new Uint8Array(3000)], { type: 'image/jpeg' }); const deps = (o: Partial = {}): SelfReviewDeps => ({ @@ -120,4 +120,41 @@ describe('runSelfReview', () => { await runSelfReview(d); expect((d.capture as any).mock.calls[0][0]).toContain('/api/a'); }); + // Live eval 2026-11-01 (ols-home-firefox): the model said "Fixed the hero" but sent ops: []. + const CLAIMS = [ + 'Fixed the hero: it rendered on a near-white background, so the headline, subtitle and outline button were almost invisible. It now sits on the navy brand colour with white text. The rest of the page reads fine, so I left it alone.', + 'I darkened the hero overlay and increased the card padding.', + 'Changed the CTA button to the accent colour.', + ]; + test.each(CLAIMS)('a reply that claims a fix but sends no ops is reported honestly, never as fixed (%#)', async (message) => { + const d = deps({ review: vi.fn(async () => ({ ok: true as const, response: { type: 'patch' as const, message, ops: [] } })) }); + const out = await runSelfReview(d); + expect(out.status).toBe('claimed-no-ops'); + expect(out.note).toMatch(/sent no changes/); + expect(out.note).toMatch(/nothing on your page was changed/i); + expect(d.applyResult).not.toHaveBeenCalled(); + }); + test('claim detection stays narrow: "no fixes needed" style replies are not claims', () => { + for (const m of [ + 'No fixes needed: contrast, spacing, alignment and the mobile-stacking column setup all look right. The hero photo is still a grey placeholder, so upload a team photo to replace it.', + 'The page already looks right, so no changes. Text contrast holds on the dark bands and the red band, the show cards are even, spacing follows one rhythm, and the icons and form render properly.', + 'Nothing needed to be changed.', + 'I have not changed anything; it looks right.', + 'I made no changes because everything reads well.', + 'Changed nothing, the page looks right.', + '', + ]) expect(claimsAFix(m)).toBe(false); + }); + test('structured render_mismatch flag wins over wording (and an old reply without it still works)', async () => { + const d = deps({ review: vi.fn(async () => ({ ok: true as const, response: { type: 'patch' as const, message: 'Screenshot mismatch: the hero is navy on the page I built.', ops: [], render_mismatch: true } })) }); + const out = await runSelfReview(d); + expect(out.status).toBe('mismatch'); + expect(out.note).toMatch(/screenshot/i); + const d2 = deps({ review: vi.fn(async () => ({ ok: true as const, response: { type: 'patch' as const, message: 'Looks right.', ops: [] } })) }); + expect((await runSelfReview(d2)).status).toBe('fine'); + }); + test('render_mismatch with ops still applies the ops (the flag never blocks real fixes)', async () => { + const d = deps({ review: vi.fn(async () => ({ ok: true as const, response: { type: 'patch' as const, message: 'm', ops: [{ op: 'delete_node' as const, node_id: 'n' }], render_mismatch: true } })) }); + expect((await runSelfReview(d)).status).toBe('fixed'); + }); }); diff --git a/craft/src/panels/sitesmith/selfReview.ts b/craft/src/panels/sitesmith/selfReview.ts index cbf2023..3dc73ea 100644 --- a/craft/src/panels/sitesmith/selfReview.ts +++ b/craft/src/panels/sitesmith/selfReview.ts @@ -37,9 +37,10 @@ export interface SelfReviewDeps { * - `fine`: the review ran on the right page and Sitesmith changed nothing. * - `mismatch`: Sitesmith said the screenshot was not the page it built, so it changed nothing. * - `not-applied`: Sitesmith proposed changes but none of them could be applied. + * - `claimed-no-ops`: Sitesmith's words claim a fix but it sent no changes (nothing changed; say so). * - `skipped`: the review did not run, or failed (the note says why). */ -export type SelfReviewStatus = 'fixed' | 'fine' | 'mismatch' | 'not-applied' | 'skipped'; +export type SelfReviewStatus = 'fixed' | 'fine' | 'mismatch' | 'not-applied' | 'claimed-no-ops' | 'skipped'; export interface SelfReviewOutcome { status: SelfReviewStatus; note: string } const skip = (why: string): SelfReviewOutcome => ({ status: 'skipped', note: `Self-review skipped: ${why}. Your page was built as normal.` }); @@ -62,6 +63,22 @@ export function saysRenderMismatch(message: string | undefined | null): boolean return !!message && MISMATCH_RES.some((re) => re.test(message)); } +/** + * The model's words claim it changed something ("Fixed the hero", "I darkened the overlay"). Only + * consulted when it sent NO ops, so a claim can never be shown as a fix that did not happen + * (eval 2026-11-01). Judged per clause; a negation before the verb in the same clause ("no + * changes", "nothing needed to be changed", "have not changed") is not a claim. + */ +const CLAIM_VERB = /\b(?:fixed|changed|updated|adjusted|moved|increased|decreased|reduced|darkened|lightened|recolou?red|swapped|replaced|switched|tightened|enlarged|added|removed|corrected|improved|raised|lowered|gave|made)\b|\bnow\s+(?:sits|uses|has|reads|shows|is|are)\b/i; +const NEGATION = /\b(?:no|not|nothing|never|without|n't|none)\b|n't\b/i; +export function claimsAFix(message: string | undefined | null): boolean { + if (!message) return false; + return message.split(/[.!?;:\n]+/).some((clause) => { + const m = CLAIM_VERB.exec(clause); + return !!m && !NEGATION.test(clause.slice(0, m.index)) && !/^\s+(?:no|nothing|none)\b/i.test(clause.slice(m.index + m[0].length)); + }); +} + const quote = (m: string | undefined | null): string => { const t = (m ?? '').trim().replace(/\s+/g, ' '); if (!t) return ''; @@ -90,9 +107,16 @@ export async function runSelfReview(d: SelfReviewDeps): Promise