diff --git a/capabilities/cap.implement.generate-direction.yaml b/capabilities/cap.implement.generate-direction.yaml index cd693b8..1f5335c 100644 --- a/capabilities/cap.implement.generate-direction.yaml +++ b/capabilities/cap.implement.generate-direction.yaml @@ -68,6 +68,7 @@ workflows: candidate: { type: object } initialState: preparing initialContext: + assets_ok: false renderable: false candidate: {} reason: "" @@ -232,7 +233,7 @@ workflows: verify_gate: transitions: ok: - target: done + target: checking_assets actor: deterministic guards: - { kind: expr, expr: "$.context.renderable == true" } @@ -242,6 +243,40 @@ workflows: guards: - { kind: expr, expr: "$.context.renderable != true" } + # ASSET REFERENCES (#4 made local assets legal; nothing checked them). + # A candidate that references files which are not there renders as a page + # full of broken-image icons while parsing perfectly — observed live, and + # carried four generations toward a human before anyone noticed. A network + # reference is the same class: it renders here and fails everywhere else, + # including in the headless screenshotter. + checking_assets: + goal: Confirm every referenced asset is a local file that exists. + transitions: + check: + target: assets_gate + actor: deterministic + executor: + kind: script + subject: verify.assets-resolve + args: + - "{{ $.workflow.input.out_dir }}/candidate-{{ $.workflow.input.candidate_id }}.html" + output: + assets_ok: "$.output.json.assets_ok" + reason: "$.output.json.reason" + + assets_gate: + transitions: + ok: + target: done + actor: deterministic + guards: + - { kind: expr, expr: "$.context.assets_ok == true" } + broken: + target: failed + actor: deterministic + guards: + - { kind: expr, expr: "$.context.assets_ok != true" } + done: terminal: true outcome: success diff --git a/capabilities/cap.implement.review-and-revise.yaml b/capabilities/cap.implement.review-and-revise.yaml index 98f6ee2..94a3870 100644 --- a/capabilities/cap.implement.review-and-revise.yaml +++ b/capabilities/cap.implement.review-and-revise.yaml @@ -70,6 +70,7 @@ workflows: initialState: preparing initialContext: revised: false + assets_ok: false copy_preserved: false renderable: false snapshot: "" @@ -175,6 +176,41 @@ workflows: "no change needed" is a legitimate, useful outcome. design brief: {{ $.workflow.input.content_path }} + # THE REPORT IS THE PRODUCT OF THIS PASS. Without a schema the model + # answered `{summary: "Saw the rendered page, found gaps, rewrote + # candidate-v3.html", details: "See internal monologue."}` — a + # successful review that told the human nothing. What a screenshot + # buys is the OBSERVATION ("the plate is cropped through the child's + # face", "the setup list is unreadable at 320px"); nothing else in + # the pipeline can produce it, so it is required, not requested. + inputSchema: + type: object + required: [observations, changes, verdict] + properties: + verdict: + type: string + enum: [revised, no_change_needed] + observations: + type: array + default: [] + items: + type: object + required: [saw, severity] + properties: + # What was ON SCREEN — not what the markup intended. + saw: { type: string } + severity: { type: string, enum: [blocking, notable, minor] } + # Where: "full width", "320px", "dark mode". + where: { type: string } + changes: + type: array + default: [] + items: + type: object + required: [changed, why] + properties: + changed: { type: string } + why: { type: string } output: review_report: "$.output" @@ -231,18 +267,51 @@ workflows: copy_gate: transitions: preserved: - target: done + target: checking_assets actor: deterministic guards: - { kind: expr, expr: "$.context.copy_preserved == true" } - output: - revised: true altered: target: restoring actor: deterministic guards: - { kind: expr, expr: "$.context.copy_preserved != true" } + # THE SECOND GUARD. The copy check correctly permits removing an image — + # a `src` is not copy — so a reviewer shown broken-image icons deleted all + # eight of them and passed every gate as a successful revision. Passing the + # snapshot as the baseline turns "every reference resolves" into "and you + # did not resolve them by deleting the artwork". + checking_assets: + goal: Confirm the revision did not strip the page's imagery. + transitions: + check: + target: assets_gate + actor: deterministic + executor: + kind: script + subject: verify.assets-resolve + args: + - "$.context.candidate" + - "$.context.snapshot" + output: + assets_ok: "$.output.json.assets_ok" + reason: "$.output.json.reason" + + assets_gate: + transitions: + ok: + target: done + actor: deterministic + guards: + - { kind: expr, expr: "$.context.assets_ok == true" } + output: + revised: true + stripped: + target: restoring + actor: deterministic + guards: + - { kind: expr, expr: "$.context.assets_ok != true" } # A bad revision must never cost the human a candidate they might have # chosen. Put the pre-review file back and report why. restoring: diff --git a/scripts-library/assets.test.mjs b/scripts-library/assets.test.mjs new file mode 100644 index 0000000..8440dc0 --- /dev/null +++ b/scripts-library/assets.test.mjs @@ -0,0 +1,241 @@ +// assets.test.mjs — a referenced asset must actually be there, and must be local. +// +// #4 made local assets legal and the pack could check neither of the two rules +// that came with them. Both failed live: +// +// * The brief that named the artwork lost its filename list to a copy/paste +// slip. The generator invented plausible names from the story titles +// (`art/dragon-under-stairs.png`) and wrote eight tags pointing at +// nothing. The candidate parsed, passed verify.candidate-renderable, scored, +// and was on its way to a human as a legitimate direction. Four generations +// went by; a person looking at the pixels caught it, not the pipeline. +// +// * "Renders with the network unplugged" is stated in both prompt layers and +// verified nowhere. A CDN font would look perfect on the machine that made +// it and degrade silently everywhere else. +// +// Same discipline the pack already applies to writes: the claim is not trusted, +// deterministic code re-opens the artifact, and a described-but-absent thing is +// a failure rather than a shrug. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync, writeFileSync, mkdtempSync, mkdirSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; +import { execFileSync } from "node:child_process"; + +const HERE = dirname(fileURLToPath(import.meta.url)); + +function bodyRunner(yamlName) { + const raw = readFileSync(join(HERE, yamlName), "utf8"); + const lines = raw.split("\n"); + const i = lines.findIndex((l) => /^\s*body:\s*\|\s*$/.test(l)); + assert.ok(i >= 0, `no 'body: |' block in ${yamlName}`); + const indent = lines[i].match(/^(\s*)/)[1].length + 2; + const out = []; + for (let k = i + 1; k < lines.length; k++) { + const l = lines[k]; + if (l.trim() === "") { out.push(""); continue; } + if (l.match(/^(\s*)/)[1].length < indent) break; + out.push(l.slice(indent)); + } + const file = join(mkdtempSync(join(tmpdir(), "as-")), yamlName.replace(/\.yaml$/, ".js")); + writeFileSync(file, out.join("\n")); + return file; +} + +const CHECK = bodyRunner("verify.assets-resolve.yaml"); + +// Build a candidate directory with a real art/ folder, then check some markup. +function check(bodyHtml, { assets = ["art/plate-one.png"] } = {}) { + const dir = mkdtempSync(join(tmpdir(), "as-run-")); + for (const a of assets) { + mkdirSync(join(dir, dirname(a)), { recursive: true }); + writeFileSync(join(dir, a), "PNGDATA"); + } + const artifact = join(dir, "candidate-c1.html"); + writeFileSync(artifact, `${bodyHtml}`); + return JSON.parse(execFileSync("node", [CHECK, artifact], { encoding: "utf8" })); +} + +// --- the happy path --------------------------------------------------------- + +test("a candidate referencing files that exist passes", () => { + const out = check(`a plate`); + assert.equal(out.assets_ok, true); + assert.equal(out.checked, 1); +}); + +test("CSS url() references are checked too, not just markup", () => { + const out = check(`
`); + assert.equal(out.assets_ok, true); + assert.equal(out.checked, 1); +}); + +test("srcset candidates are split off their descriptors before resolving", () => { + const out = check( + ``, + { assets: ["art/plate-one.png", "art/plate-two.png"] }, + ); + assert.equal(out.assets_ok, true); +}); + +test("a query string or fragment does not make a real file look missing", () => { + const out = check(``); + assert.equal(out.assets_ok, true); +}); + +// --- the failure observed live --------------------------------------------- + +test("invented filenames are BROKEN_ASSET_REFERENCE", () => { + const out = check(` + the dragon + gran + a real one`); + assert.equal(out.assets_ok, false); + assert.equal(out.reason, "BROKEN_ASSET_REFERENCE"); + assert.equal(out.missing_count, 2); + assert.ok(out.missing.includes("art/dragon-under-stairs.png")); + assert.ok(!out.missing.includes("art/plate-one.png"), "the file that exists is not reported"); +}); + +test("a root-relative path is broken — a self-contained page has no document root", () => { + const out = check(``); + assert.equal(out.assets_ok, false); + assert.equal(out.reason, "BROKEN_ASSET_REFERENCE"); +}); + +// --- the network rule, finally enforced ------------------------------------- + +test("a CDN stylesheet is NETWORK_ASSET_REFERENCE", () => { + const out = check(``); + assert.equal(out.assets_ok, false); + assert.equal(out.reason, "NETWORK_ASSET_REFERENCE"); + assert.equal(out.network_count, 1); +}); + +test("a protocol-relative URL is a network reference too", () => { + const out = check(``); + assert.equal(out.assets_ok, false); + assert.equal(out.reason, "NETWORK_ASSET_REFERENCE"); +}); + +test("a remote webfont in CSS is caught", () => { + const out = check(``); + assert.equal(out.assets_ok, false); + assert.equal(out.network_count, 1); +}); + +// Network is the stricter rule and the one that fails on someone else's +// machine, so it names the reason when both are present. +test("network takes precedence over broken when both are present", () => { + const out = check(``); + assert.equal(out.reason, "NETWORK_ASSET_REFERENCE"); + assert.equal(out.missing_count, 1, "the broken one is still reported"); + assert.equal(out.network_count, 1); +}); + +// --- things that are not file references at all ------------------------------ + +test("data: URIs, anchors and mailto are ignored, not resolved", () => { + const out = check(` + + skip + mail`); + assert.equal(out.assets_ok, true); + assert.equal(out.checked, 0, "none of these are files to look for"); +}); + +test("an inline needs no assets and passes cleanly", () => { + const out = check(``); + assert.equal(out.assets_ok, true); +}); + +test("an unreadable artifact is a hard error, not a silent pass", () => { + assert.throws(() => + execFileSync("node", [CHECK, join(tmpdir(), "no-such-candidate.html")], { encoding: "utf8", stdio: "pipe" }), + ); +}); + +// --------------------------------------------------------------------------- +// RETENTION — the second argument. Added after a visual reviewer, shown a page +// of broken-image icons, deleted all eight of them and passed every gate as a +// successful revision. "Every reference resolves" is trivially satisfied by a +// page with no references at all. +// --------------------------------------------------------------------------- + +// Build a before/after pair in ONE directory, so both resolve against the same +// art/ folder exactly as a candidate and its pre-review snapshot do. +function checkRetention(beforeHtml, afterHtml, { assets = ["art/plate-one.png"] } = {}) { + const dir = mkdtempSync(join(tmpdir(), "as-ret-")); + for (const a of assets) { + mkdirSync(join(dir, dirname(a)), { recursive: true }); + writeFileSync(join(dir, a), "PNGDATA"); + } + const before = join(dir, "candidate-c1.pre-review.html"); + const after = join(dir, "candidate-c1.html"); + writeFileSync(before, `${beforeHtml}`); + writeFileSync(after, `${afterHtml}`); + return JSON.parse(execFileSync("node", [CHECK, after, before], { encoding: "utf8" })); +} + +test("a revision that deletes ALL imagery is REVISION_STRIPPED_ASSETS", () => { + const out = checkRetention( + ``, + `

Hello, Rosa

`, + { assets: ["art/plate-one.png", "art/plate-two.png"] }, + ); + assert.equal(out.assets_ok, false); + assert.equal(out.reason, "REVISION_STRIPPED_ASSETS"); + assert.equal(out.had_assets, 2); + assert.equal(out.has_assets, 0); +}); + +// Deliberately narrow. Consolidating five plates into four is a design +// judgement and none of a validator's business; a rule that misfires on +// legitimate work gets turned off. +test("using FEWER images is a design judgement, not a violation", () => { + const out = checkRetention( + ``, + ``, + { assets: ["art/plate-one.png", "art/plate-two.png"] }, + ); + assert.equal(out.assets_ok, true); +}); + +test("swapping which plate is used is fine", () => { + const out = checkRetention( + ``, + ``, + { assets: ["art/plate-one.png", "art/plate-two.png"] }, + ); + assert.equal(out.assets_ok, true); +}); + +test("a page that never had imagery is not forced to acquire some", () => { + const out = checkRetention(`

words only

`, `

words only, restyled

`); + assert.equal(out.assets_ok, true); +}); + +test("moving artwork from into a CSS background still counts as retained", () => { + const out = checkRetention( + ``, + `
`, + ); + assert.equal(out.assets_ok, true); +}); + +// A broken page is already failing for a better reason; retention must not +// mask BROKEN_ASSET_REFERENCE. +test("a broken reference still reports as broken, not as stripped", () => { + const out = checkRetention(``, ``); + assert.equal(out.assets_ok, false); + assert.equal(out.reason, "BROKEN_ASSET_REFERENCE"); +}); + +test("with no baseline argument, retention is not checked at all", () => { + const out = check(`

no images here

`); + assert.equal(out.assets_ok, true, "single-argument behaviour is unchanged"); +}); diff --git a/scripts-library/self-review.test.mjs b/scripts-library/self-review.test.mjs index 74a8400..3e1f1de 100644 --- a/scripts-library/self-review.test.mjs +++ b/scripts-library/self-review.test.mjs @@ -161,3 +161,47 @@ test("restore degrades instead of throwing when there is no snapshot", () => { assert.equal(out.restored, false); assert.match(out.message, /no snapshot/); }); + +// --------------------------------------------------------------------------- +// THE REPORT CONTRACT. The first live review returned: +// { summary: "Saw the rendered page, found gaps, rewrote candidate-v3.html.", +// details: "See internal monologue." } +// — a successful pass that told the human nothing. The observation is the ONLY +// thing a screenshot buys that no CSS parser can produce; a bare +// `review_report: "$.output"` let the model decline to provide it. These pin +// the schema so the value cannot quietly evaporate again. +// --------------------------------------------------------------------------- + +const CAP = readFileSync( + join(HERE, "..", "capabilities", "cap.implement.review-and-revise.yaml"), + "utf8", +).replace(/\s+/g, " "); + +test("the review step REQUIRES observations, changes and a verdict", () => { + assert.match(CAP, /required: \[observations, changes, verdict\]/); +}); + +test("an observation must say what was SEEN and how bad it is", () => { + assert.match(CAP, /required: \[saw, severity\]/); + assert.match(CAP, /enum: \[blocking, notable, minor\]/); +}); + +test("a change must carry its reason, not just the edit", () => { + assert.match(CAP, /required: \[changed, why\]/); +}); + +// "The page is already good" has to be expressible, or every pass invents work. +test("no_change_needed is a legitimate verdict", () => { + assert.match(CAP, /enum: \[revised, no_change_needed\]/); +}); + +test("the revision is gated on asset retention, not only on copy", () => { + assert.match(CAP, /checking_assets/); + assert.match(CAP, /subject: verify\.assets-resolve/); + // The snapshot is the baseline — without it the check is vacuous. + assert.match(CAP, /args: - "\$\.context\.candidate" - "\$\.context\.snapshot"/); +}); + +test("a stripped-asset revision routes to restore, not to done", () => { + assert.match(CAP, /stripped: target: restoring/); +}); diff --git a/scripts-library/verify.assets-resolve.yaml b/scripts-library/verify.assets-resolve.yaml new file mode 100644 index 0000000..a40e318 --- /dev/null +++ b/scripts-library/verify.assets-resolve.yaml @@ -0,0 +1,174 @@ +# scripts-library/verify.assets-resolve.yaml +# +# Make the two asset rules DETERMINISTIC instead of merely instructed. +# +# Allowing local assets (#4) traded one failure mode for two, and the pack could +# detect neither: +# +# 1. A REFERENCE TO A FILE THAT IS NOT THERE. Observed live: the operator's +# brief named the artwork but a copy/paste slip dropped the filename list, +# so the generator invented plausible names from the story titles — +# `art/dragon-under-stairs.png`, `art/gran-1953.png` — and wrote eight +# tags pointing at nothing. The candidate parsed as HTML, passed +# verify.candidate-renderable, scored, and was carried toward the human as +# a legitimate direction. Four generations were spent before anyone noticed, +# and only because a human looked at the pixels. +# +# 2. A NETWORK REFERENCE. "Renders with the network unplugged" is stated in +# both prompt layers and checked by nothing. A CDN stylesheet or a Google +# Fonts link would render beautifully on the machine that made it and +# degrade silently anywhere else — including in the headless screenshotter. +# +# Both are the pack's own discipline applied to the newest surface: the agent's +# claim is not trusted, deterministic code re-opens the artifact, and a +# described-but-absent thing is a failure rather than a shrug. +# +# WHAT IS SCANNED: src / srcset / href / poster on any element, CSS url(...) in +# both inline