diff --git a/craft/src/hooks/useSitesmith.test.ts b/craft/src/hooks/useSitesmith.test.ts index 897f633..5139f45 100644 --- a/craft/src/hooks/useSitesmith.test.ts +++ b/craft/src/hooks/useSitesmith.test.ts @@ -141,6 +141,17 @@ describe('useSitesmith start+poll', () => { expect(calls.some((c) => c.url.includes('action=send'))).toBe(false); }); + test('r4: site_build and review_ops pass through typed; malformed ones are ignored', async () => { + script(OKSTART, [done({ ok: true, response: { type: 'ask', question: 'q' }, site_build: { planned: ['Home', 'About'], built: ['Home'], missing: ['About'] }, review_ops: { proposed: 3, kept: 0 } })]); + const res = await sendFor(30000, 'hi', 'canvas'); + expect(res.siteBuild).toEqual({ planned: ['Home', 'About'], built: ['Home'], missing: ['About'] }); + expect(res.reviewOps).toEqual({ proposed: 3, kept: 0 }); + script(OKSTART, [done({ ok: true, response: { type: 'ask', question: 'q' }, site_build: { planned: 'x', built: [], missing: [1] }, review_ops: { proposed: 1, kept: 5 } })]); + const bad = await sendFor(30000, 'hi', 'canvas'); + expect(bad.siteBuild).toBeUndefined(); + expect(bad.reviewOps).toBeUndefined(); + }); + test('polls back off from ~1s towards a 5s cap', async () => { script(OKSTART, [running()]); vi.useFakeTimers(); diff --git a/craft/src/hooks/useSitesmith.ts b/craft/src/hooks/useSitesmith.ts index 169819e..fb278e6 100644 --- a/craft/src/hooks/useSitesmith.ts +++ b/craft/src/hooks/useSitesmith.ts @@ -159,6 +159,11 @@ function asSendResult(res: any): SendResult { if (sb && typeof sb === 'object' && names(sb.planned) && names(sb.built) && names(sb.missing)) { out.siteBuild = { planned: names(sb.planned)!, built: names(sb.built)!, missing: names(sb.missing)! }; } + const ro = res.review_ops; + const count = (v: unknown) => (typeof v === 'number' && Number.isInteger(v) && v >= 0 && v <= 1000 ? v : null); + if (ro && typeof ro === 'object' && count(ro.proposed) !== null && count(ro.kept) !== null && ro.kept <= ro.proposed) { + out.reviewOps = { proposed: ro.proposed, kept: ro.kept }; + } return out; } return failure(toSitesmithStatus(res?.status, 'ERROR'), sensibleMessage(res) ?? 'Sitesmith could not complete that request. Please try again.'); diff --git a/craft/src/panels/sitesmith/SitesmithModal.tsx b/craft/src/panels/sitesmith/SitesmithModal.tsx index 44903be..c0dd204 100644 --- a/craft/src/panels/sitesmith/SitesmithModal.tsx +++ b/craft/src/panels/sitesmith/SitesmithModal.tsx @@ -487,7 +487,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 774041c..fc541a8 100644 --- a/craft/src/panels/sitesmith/selfReview.test.ts +++ b/craft/src/panels/sitesmith/selfReview.test.ts @@ -170,3 +170,46 @@ describe('runSelfReview', () => { expect((await runSelfReview(d)).status).toBe('fixed'); }); }); + +// ---- round 4 item 6: honest wording when the server's safety checks dropped the review's changes +describe('runSelfReview: changes dropped by the safety checks', () => { + const LIVE = 'Image band narrowed and the copy moved up so the hero reads first.'; + const reply = (ops: any[], reviewOps?: { proposed: number; kept: number }, message = LIVE) => + vi.fn(async () => ({ ok: true as const, response: { type: 'patch' as const, message, ops }, ...(reviewOps ? { reviewOps } : {}) })); + test('all proposed changes dropped: never "made no changes. Sitesmith said: "', async () => { + const out = await runSelfReview(deps({ review: reply([], { proposed: 3, kept: 0 }) })); + expect(out.status).toBe('dropped'); + expect(out.note).toContain('Sitesmith proposed 3 changes, but none passed the safety checks, so nothing on your page was changed.'); + expect(out.note).not.toMatch(/made no changes|Sitesmith said:/); + expect(out.note).toContain(`What it proposed (not applied): "${LIVE}"`); + }); + test('singular', async () => { + const out = await runSelfReview(deps({ review: reply([], { proposed: 1, kept: 0 }) })); + expect(out.note).toContain('Sitesmith proposed 1 change, but it did not pass the safety checks'); + }); + test('some dropped, the rest applied: says how many were dropped, and that the description may include them', async () => { + const ops = [{ op: 'update_props' as const, node_id: 'a', props: { style: { color: '#000' } } }]; + const out = await runSelfReview(deps({ review: reply(ops, { proposed: 3, kept: 1 }), applyResult: vi.fn(async () => ({ ok: true, changed: 1, ops: 1 })) })); + expect(out.status).toBe('fixed'); + expect(out.note).toContain('Sitesmith proposed 3 changes; 2 did not pass the safety checks and were dropped.'); + expect(out.note).toContain(`Sitesmith's description (it may include the dropped changes): "${LIVE}"`); + }); + test('nothing dropped: unchanged wording (fine / fixed)', async () => { + const fine = await runSelfReview(deps({ review: reply([], { proposed: 0, kept: 0 }, 'Looks right.') })); + expect(fine.status).toBe('fine'); + expect(fine.note).toMatch(/made no changes\. Sitesmith said: "Looks right\."/); + const old = await runSelfReview(deps({ review: reply([], undefined, 'Looks right.') })); // older backend + expect(old.status).toBe('fine'); + }); + test('ops sent but none applied in the editor: the quote is labelled as not applied', async () => { + const out = await runSelfReview(deps({ applyResult: vi.fn(async () => ({ ok: true, changed: 0, ops: 1 })) })); + expect(out.status).toBe('not-applied'); + expect(out.note).toMatch(/What it proposed \(not applied\): "m"/); + expect(out.note).not.toMatch(/Sitesmith said:/); + }); + test('claimed a fix but sent no ops: the quote is labelled as not applied', async () => { + const out = await runSelfReview(deps({ review: reply([], { proposed: 0, kept: 0 }, 'Fixed the hero contrast.') })); + expect(out.status).toBe('claimed-no-ops'); + expect(out.note).toContain('Its reply (nothing was applied): "Fixed the hero contrast."'); + }); +}); diff --git a/craft/src/panels/sitesmith/selfReview.ts b/craft/src/panels/sitesmith/selfReview.ts index 16db384..13c8ec1 100644 --- a/craft/src/panels/sitesmith/selfReview.ts +++ b/craft/src/panels/sitesmith/selfReview.ts @@ -38,9 +38,12 @@ export interface SelfReviewDeps { * - `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). + * - `dropped`: Sitesmith proposed changes but the server's safety checks removed all of them (round 4). * - `skipped`: the review did not run, or failed (the note says why). + * The model's message is quoted as "Sitesmith said" only when it describes what actually happened; + * whenever nothing it proposed was applied, the quote is labelled as not applied. */ -export type SelfReviewStatus = 'fixed' | 'fine' | 'mismatch' | 'not-applied' | 'claimed-no-ops' | 'skipped'; +export type SelfReviewStatus = 'fixed' | 'fine' | 'mismatch' | 'not-applied' | 'claimed-no-ops' | 'dropped' | 'skipped'; export interface SelfReviewOutcome { status: SelfReviewStatus; note: string; @@ -84,11 +87,13 @@ export function claimsAFix(message: string | undefined | null): boolean { }); } -const quote = (m: string | undefined | null): string => { +const quote = (m: string | undefined | null, label = 'Sitesmith said'): string => { const t = (m ?? '').trim().replace(/\s+/g, ' '); if (!t) return ''; - return ` Sitesmith said: "${t.length > 280 ? t.slice(0, 277) + '...' : t}"`; + return ` ${label}: "${t.length > 280 ? t.slice(0, 277) + '...' : t}"`; }; +const NOT_APPLIED = 'What it proposed (not applied)'; +const changes = (n: number) => `${n} ${n === 1 ? 'change' : 'changes'}`; /** One review round, never more. Never throws; any failure is a non-blocking note. */ export async function runSelfReview(d: SelfReviewDeps): Promise { @@ -121,6 +126,12 @@ async function reviewOnce(d: SelfReviewDeps, onShot: (c: NonNullable ro.kept ? ro.proposed - ro.kept : 0; + if (r.type === 'patch' && r.ops.length === 0 && ro && ro.proposed > 0) { + return { status: 'dropped', note: `Self-review: Sitesmith proposed ${changes(ro.proposed)}, but ${ro.proposed === 1 ? 'it did' : 'none'} ${ro.proposed === 1 ? 'not pass' : 'passed'} the safety checks, so nothing on your page was changed.${quote(msg, NOT_APPLIED)}` }; + } if (r.type === 'patch' && r.ops.length === 0) { // Structured flag first (newer backends); the wording check keeps older replies working. if (r.render_mismatch === true) { @@ -130,19 +141,20 @@ async function reviewOnce(d: SelfReviewDeps, onShot: (c: NonNullable