fix(sitesmith): honest self-review wording when changes were dropped or not applied
Live 2026.11.8: the server's review lockdown stripped every op, and the editor said
"made no changes. Sitesmith said: Image band narrowed..." (a fix that never happened). With
the backend's review_ops {proposed, kept}:
- all dropped -> new status "dropped": "Sitesmith proposed N changes, but none passed the
safety checks, so nothing on your page was changed. What it proposed (not applied): ..."
- some dropped -> the "made fixes" note says how many were dropped and labels the quote as
possibly describing them;
- not-applied / claimed-no-ops quotes are labelled as not applied instead of "Sitesmith said".
"Sitesmith said" is kept only where the quote describes what actually happened.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
274ae9fc33
commit
52b453eecc
6 files changed
+81
-8
No files matched your search
@@ -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();
|
||||
|
||||
@@ -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.');
|
||||
|
||||
@@ -487,7 +487,7 @@ export const SitesmithModal: React.FC<Props> = ({ onClose, target }) => {
|
||||
{reviewNote && (
|
||||
<div
|
||||
role="note"
|
||||
style={{ ...(reviewNote.status === 'mismatch' || reviewNote.status === 'not-applied' || reviewNote.status === 'claimed-no-ops' ? warnBox : noteStyle), marginBottom: 8 }}
|
||||
style={{ ...(reviewNote.status === 'mismatch' || reviewNote.status === 'not-applied' || reviewNote.status === 'claimed-no-ops' || reviewNote.status === 'dropped' ? warnBox : noteStyle), marginBottom: 8 }}
|
||||
data-testid="ss-review-note"
|
||||
data-status={reviewNote.status}
|
||||
>{reviewNote.note}</div>
|
||||
|
||||
@@ -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: <a fix>"', 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."');
|
||||
});
|
||||
});
|
||||
@@ -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<SelfReviewOutcome> {
|
||||
@@ -121,6 +126,12 @@ async function reviewOnce(d: SelfReviewDeps, onShot: (c: NonNullable<SelfReviewO
|
||||
return { status: 'skipped', note: `Self-review: Sitesmith asked a question instead of reviewing the page, so nothing was changed.${quote(r.question)}` };
|
||||
}
|
||||
const msg = 'message' in r ? r.message : '';
|
||||
// newer backends: how many ops the model proposed and how many the review lockdown kept
|
||||
const ro = res.reviewOps;
|
||||
const dropped = ro && ro.proposed > 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<SelfReviewO
|
||||
return { status: 'mismatch', note: `Self-review did not work this time: Sitesmith said the screenshot did not match the page it built, so it changed nothing. Your page is as built; check it yourself.${quote(msg)}` };
|
||||
}
|
||||
if (claimsAFix(msg)) {
|
||||
return { status: 'claimed-no-ops', note: `Self-review: Sitesmith's reply describes a fix, but it sent no changes, so nothing on your page was changed. Check the part it mentions yourself.${quote(msg)}` };
|
||||
return { status: 'claimed-no-ops', note: `Self-review: Sitesmith's reply describes a fix, but it sent no changes, so nothing on your page was changed. Check the part it mentions yourself.${quote(msg, 'Its reply (nothing was applied)')}` };
|
||||
}
|
||||
return { status: 'fine', note: `Self-review: Sitesmith looked at a screenshot of the finished page and made no changes.${quote(msg)}` };
|
||||
}
|
||||
const applied = await d.applyResult(res);
|
||||
if (!applied.ok) return skip(applied.message || 'the fixes could not be applied');
|
||||
if (r.type === 'patch' && applied.changed === 0) {
|
||||
return { status: 'not-applied', note: `Self-review: Sitesmith suggested ${r.ops.length === 1 ? 'a fix' : `${r.ops.length} fixes`}, but none could be applied (the parts it named are not on the page), so nothing changed.${quote(msg)}` };
|
||||
return { status: 'not-applied', note: `Self-review: Sitesmith suggested ${r.ops.length === 1 ? 'a fix' : `${r.ops.length} fixes`}, but none could be applied (the parts it named are not on the page), so nothing changed.${quote(msg, NOT_APPLIED)}` };
|
||||
}
|
||||
const extra = applied.summary ? ` ${applied.summary}.` : '';
|
||||
const partial = r.type === 'patch' && applied.changed !== undefined && applied.changed < r.ops.length
|
||||
? ` ${applied.changed} of ${r.ops.length} suggested fixes were applied; the rest named parts that are not on the page.` : '';
|
||||
return { status: 'fixed', note: `Self-review: Sitesmith looked at a screenshot of the finished page and made fixes.${partial}${extra} Use Undo to go back.${quote(msg)}` };
|
||||
const drop = dropped ? ` Sitesmith proposed ${changes(ro!.proposed)}; ${dropped} did not pass the safety checks and ${dropped === 1 ? 'was' : 'were'} dropped.` : '';
|
||||
return { status: 'fixed', note: `Self-review: Sitesmith looked at a screenshot of the finished page and made fixes.${drop}${partial}${extra} Use Undo to go back.${quote(msg, dropped ? "Sitesmith's description (it may include the dropped changes)" : 'Sitesmith said')}` };
|
||||
} catch (e) {
|
||||
return skip(e instanceof Error && e.message ? e.message.slice(0, 100) : 'something went wrong');
|
||||
} finally {
|
||||
|
||||
@@ -77,6 +77,8 @@ export type SitesmithResponse =
|
||||
|
||||
export interface SendResultOk { ok: true; response: SitesmithResponse; /** Non-blocking server messages (e.g. images skipped). */ notes?: string[];
|
||||
/** Whole-site builds (newer backends): which planned pages were built / missing. */
|
||||
siteBuild?: { planned: string[]; built: string[]; missing: string[] }; }
|
||||
siteBuild?: { planned: string[]; built: string[]; missing: string[] };
|
||||
/** Self-review (newer backends): ops the model proposed vs ops that passed the server's review lockdown. */
|
||||
reviewOps?: { proposed: number; kept: number }; }
|
||||
export interface SendResultErr { ok: false; status: SitesmithStatus; message: string; /** The job this failure belongs to (resumable when we only stopped waiting). */ jobId?: string; }
|
||||
export type SendResult = SendResultOk | SendResultErr;
|
||||
Reference in new issue
Block a user