From 045865918afca96899a4aee2981b66bff6883a00 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Sat, 8 Aug 2026 19:26:43 -0700 Subject: [PATCH] Held for review: agent placeholder substitution, reviewer recapture contract, base-directory script form (#544) * Resolve {{scripts_path}} in the agent bodies Codex ships Three code paths emit an agent body: the degraded fallback reference, the .toml nested inside the skill for Codex, and the native agent file. Only the nested .toml skipped placeholder substitution and rule-marker stripping, so the codex and .agents dists shipped `node {{scripts_path}}/embed-prompt.mjs` verbatim in the asset producer, and every caller had to substitute the token itself at load time. All three now render through renderAgentBody(), and the new regression test asserts a runnable embed-prompt command on each emitted surface plus a synthetic agent proving markers and placeholders resolve in the nested .toml. Prepared by an AI agent (Claude Code) under pbakaus's instruction. Co-Authored-By: Claude Fable 5 * Give the finish reviewer's screenshots one fixed address The Input Contract asked for "desktop and mobile screenshot paths captured by the parent" and named none, so each session invented a filename and the verdict pass went looking for a recapture that was never written there. Two reviewer passes burned on that in the eval runs. The parent now captures and recaptures to .impeccable/review/desktop.png and .impeccable/review/mobile.png, and the reviewer reads those two first, treating a brief-named path as the fallback for a parent that wrote elsewhere. Prepared by an AI agent (Claude Code) under pbakaus's instruction. Co-Authored-By: Claude Fable 5 * Lead Setup with the base directory the runtime reports The rendered claude and codex skills opened with `node .claude/skills/impeccable/scripts/context.mjs`, a project-relative path that resolves in this repo and in nothing a user installs: a personal or plugin install puts the scripts outside the project entirely. The working form was already in the text, parenthesized, after the one that fails. Setup now leads with `node /scripts/context.mjs` and says once that the base directory resolves every scripts-path command in the skill and its references, leaving the project-relative path as the fallback for runtimes that report no base directory. Prepared by an AI agent (Claude Code) under pbakaus's instruction. Co-Authored-By: Claude Fable 5 * Answer the Copilot review: brittle model assertion, missing review dir Assert that {{model}} resolved rather than that it resolved to "GPT", which belongs to PROVIDER_PLACEHOLDERS and can change without touching what the test guards. And have the parent create .impeccable/review/ when the harness does not, so a fresh project's first capture has somewhere to land. Prepared by an AI agent (Claude Code) under pbakaus's instruction. Co-Authored-By: Claude Fable 5 * Make the review-screenshot contract directory-based, not web-viewport-named Two amendments to the recapture contract from review feedback: 1. The canonical location is the directory .impeccable/review/, one file per captured viewport; desktop.png and mobile.png are the web case, not the contract. Baking web-viewport names into the reviewer's spec would have hardened a web assumption into paths that a native (ios/android/adaptive) build cannot honestly write. 2. Precedence restored to explicit-beats-convention: paths the calling brief names are authoritative when the files exist; the canonical directory is where the reviewer looks when the brief names none or a named path is missing. This avoids stale canonical files from an earlier run silently winning over fresh explicit paths. The observed failure (the verdict round inventing a round-stamped filename) stays fixed: recapture happens over the same files, and invented filenames are still called out as pointing at nothing. Assisted-by: Claude Code --------- Co-authored-by: Claude Fable 5 --- scripts/lib/transformers/factory.js | 29 +++++--- skill/SKILL.src.md | 2 +- skill/agents/impeccable-finish-reviewer.md | 4 +- skill/reference/new-work.md | 2 +- tests/build.test.js | 84 ++++++++++++++++++++++ 5 files changed, 107 insertions(+), 14 deletions(-) diff --git a/scripts/lib/transformers/factory.js b/scripts/lib/transformers/factory.js index bf792d8bc..195e1b159 100644 --- a/scripts/lib/transformers/factory.js +++ b/scripts/lib/transformers/factory.js @@ -176,6 +176,22 @@ function buildCursorAgent(agent, body) { return `${generateYamlFrontmatter(frontmatter)}\n${body.trim()}\n`; } +/** + * Render an agent's markdown body for one provider. + * + * Every surface that ships an agent body (the degraded fallback reference, the + * Codex .toml nested inside the skill, and the native agent file) goes through + * here, so all three resolve provider blocks, {{placeholders}}, rule markers, + * and {{scripts_path}} the same way. The nested Codex .toml used to skip the + * last two and shipped `node {{scripts_path}}/embed-prompt.mjs` literally. + */ +function renderAgentBody(agent, { providerTags, placeholderKey, allSkillNames, scriptsPath }) { + let body = compileProviderBlocks(agent.body, providerTags); + body = replacePlaceholders(body, placeholderKey, [], allSkillNames); + body = stripRuleMarkers(body); + return body.replace(/\{\{scripts_path\}\}/g, scriptsPath); +} + function buildAgentFile(config, agent, body) { if (config.agentFormat === 'codex-toml') { return { @@ -330,10 +346,7 @@ export function createTransformer(config) { ensureDir(degradedDir); for (const agent of skill.agents) { const role = agent.name.replace(/^impeccable-/, ''); - let body = compileProviderBlocks(agent.body, providerTags); - body = replacePlaceholders(body, placeholderKey, [], allSkillNames); - body = stripRuleMarkers(body); - body = body.replace(/\{\{scripts_path\}\}/g, scriptsPath); + const body = renderAgentBody(agent, { providerTags, placeholderKey, allSkillNames, scriptsPath }); const content = `${DEGRADED_PREAMBLE}\n\n${body.replace(/^\s+/, '')}`; writeFile(path.join(degradedDir, `${role}.md`), content); refCount++; @@ -358,8 +371,7 @@ export function createTransformer(config) { if (CODEX_SKILL_PROVIDERS.has(provider)) { for (const agent of skill.agents || []) { if (agent.providers && !agent.providers.includes('codex')) continue; - let agentBody = compileProviderBlocks(agent.body, providerTags); - agentBody = replacePlaceholders(agentBody, placeholderKey, [], allSkillNames); + const agentBody = renderAgentBody(agent, { providerTags, placeholderKey, allSkillNames, scriptsPath }); const filename = `${agent.codexName || agent.name.replace(/-/g, '_')}.toml`; ensureDir(path.join(skillDir, 'agents')); writeFile(path.join(skillDir, 'agents', filename), buildCodexAgent(agent, agentBody)); @@ -375,10 +387,7 @@ export function createTransformer(config) { // Agents can declare `providers: ` to limit which harnesses // they emit to. Default (no field) ships everywhere with agentFormat. if (agent.providers && !agent.providers.includes(provider)) continue; - let body = compileProviderBlocks(agent.body, providerTags); - body = replacePlaceholders(body, placeholderKey, [], allSkillNames); - body = stripRuleMarkers(body); - body = body.replace(/\{\{scripts_path\}\}/g, scriptsPath); + const body = renderAgentBody(agent, { providerTags, placeholderKey, allSkillNames, scriptsPath }); const agentFile = buildAgentFile(config, agent, body); if (!agentFile) continue; ensureDir(agentsDir); diff --git a/skill/SKILL.src.md b/skill/SKILL.src.md index 59e56f09c..004a1b03e 100644 --- a/skill/SKILL.src.md +++ b/skill/SKILL.src.md @@ -18,7 +18,7 @@ Core principles: ## Setup -1. Run `node {{scripts_path}}/context.mjs` once per session (if the runtime shows this skill's loaded base directory, run `node /scripts/context.mjs`; keep cwd at the user's project). Pass a named source file or route as `--target `. It loads PRODUCT.md, DESIGN.md, the matching surface brief, and native-platform guidance when applicable; follow its directives and do not rerun it. +1. Run `node /scripts/context.mjs` once per session, where `` is the loaded base directory the runtime reports for this skill; keep cwd at the user's project. That base directory resolves every `node {{scripts_path}}/...` command in this skill and its references, and `{{scripts_path}}` is the fallback only when the runtime reports no base directory. Pass a named source file or route as `--target `. It loads PRODUCT.md, DESIGN.md, the matching surface brief, and native-platform guidance when applicable; follow its directives and do not rerun it. 2. Before acting, load the one playbook that owns the request: the Commands table's reference for an explicit or clearly implied sub-command, or [reference/new-work.md](reference/new-work.md) for a new surface or replacement visual world. Then inspect the target and at least one representative source of incumbent visual truth (tokens, theme, CSS, component, or asset) before editing. 3. After analysis and direction are resolved, load [reference/craft-floor.md](reference/craft-floor.md) immediately before editing UI. It carries the quality floor, the absolute bans, and the reflexes no detector catches. Do not load it for planning-only work. diff --git a/skill/agents/impeccable-finish-reviewer.md b/skill/agents/impeccable-finish-reviewer.md index 588c1545e..ed7e82fc7 100644 --- a/skill/agents/impeccable-finish-reviewer.md +++ b/skill/agents/impeccable-finish-reviewer.md @@ -22,7 +22,7 @@ A hard turn ceiling ends the run without warning; a run that ends before the fiv ## Input Contract -Expect: the original request; the confirmed user answers; the artifact path(s); desktop and mobile screenshot paths captured by the parent; the direction contract (THESIS, OWN-WORLD, STORY, FIRST VIEWPORT, FORM); PRODUCT.md path; existing hook or detector findings; the chosen world's QUALITY BAR card paths and, on a comp-led build, the approved comp path (a code-led build has no approved comp; it passes the chosen decision comp as a separate critique-reference input, labeled as such, and nothing in this file that binds “the approved comp” binds it); and the skill's `reference/craft-floor.md` path. When the harness can view images, open the screenshots, the comp, and the card first, and inventory the comp's salient elements in your own words before reading the direction contract or any builder-authored summary: a review anchored on the contract inherits whatever the builder's abstraction dropped. +Expect: the original request; the confirmed user answers; the artifact path(s); the screenshots the parent captured, which live in `.impeccable/review/` (on the web, `desktop.png` and `mobile.png`); a screenshot path the calling brief names is authoritative when the file exists, and `.impeccable/review/` is where to look when the brief names none or a named path is missing, never a filename you invent; the direction contract (THESIS, OWN-WORLD, STORY, FIRST VIEWPORT, FORM); PRODUCT.md path; existing hook or detector findings; the chosen world's QUALITY BAR card paths and, on a comp-led build, the approved comp path (a code-led build has no approved comp; it passes the chosen decision comp as a separate critique-reference input, labeled as such, and nothing in this file that binds “the approved comp” binds it); and the skill's `reference/craft-floor.md` path. When the harness can view images, open the screenshots, the comp, and the card first, and inventory the comp's salient elements in your own words before reading the direction contract or any builder-authored summary: a review anchored on the contract inherits whatever the builder's abstraction dropped. ## Checks, in order @@ -45,4 +45,4 @@ Return the disposition line first, then exactly five sections: `persistence` (pa ## Verdict Pass -When the parent returns with post-fix recaptures, you are scoring, not re-hunting. The parent's narration of what was fixed is not evidence; a claimed fix you cannot see in the recaptures is unresolved. For each material fix from your review, one line: resolved, partial, or unresolved, tied to what the new screenshots visibly show; a fix answered mechanically, positions moved but the quality the finding named still absent, is partial at best. Then name at most three regressions the fix batch itself introduced, judged by the same matrix rules, and nothing else; no new hunt, no new checks. Return exactly two sections: `verdict` (the scored list) and `remaining` (what stays open, or "clear"), and end with the disposition line recomputed against what remains open; unresolved or partial material findings can never recompute to ship. +When the parent returns with post-fix recaptures, you are scoring, not re-hunting. The parent recaptures over the same screenshot files you read in the review round, so re-read those exact paths for this round; a round-stamped filename you invent points at nothing. The parent's narration of what was fixed is not evidence; a claimed fix you cannot see in the recaptures is unresolved. For each material fix from your review, one line: resolved, partial, or unresolved, tied to what the new screenshots visibly show; a fix answered mechanically, positions moved but the quality the finding named still absent, is partial at best. Then name at most three regressions the fix batch itself introduced, judged by the same matrix rules, and nothing else; no new hunt, no new checks. Return exactly two sections: `verdict` (the scored list) and `remaining` (what stays open, or "clear"), and end with the disposition line recomputed against what remains open; unresolved or partial material findings can never recompute to ship. diff --git a/skill/reference/new-work.md b/skill/reference/new-work.md index adddcc12e..d745fdf5d 100644 --- a/skill/reference/new-work.md +++ b/skill/reference/new-work.md @@ -109,6 +109,6 @@ Preserve semantics, accessibility, performance, responsiveness, project conventi Inspect desktop and mobile in one batched screenshot round, critique the render against the user's request and the direction contract, fix material gaps, and confirm with one final round; two rounds is the ceiling, and fixes batch between them rather than earning per-tweak screenshots. When an approved comp exists, the critique is a side-by-side: view the comp region and the build region together, the hero and each section as its own crop at legible scale, never one full-page thumbnail, which hides exactly the failures that matter, crude controls, wrong lettering character, flattened material, behind a superficially similar section order. On a Persuade surface, verify the mode did its job: a first-time visitor should know what this is, why it matters, and what to do within seconds, in the form's own vocabulary. -After the second inspection round the build thread's polishing is over: no further defect hunts, micro-edit scripts, or rebuilds here; whatever remains ships through the handoffs, where a fresh context does the finding better and cheaper. Where this harness runs no design hook, run `node {{scripts_path}}/detect.mjs --json` on the changed targets once here, fix what is mechanical, and pass the remaining findings to the reviewer; a hookless build that skips this ships every tell the hook exists to catch. Capture desktop and mobile screenshots to files, then spawn the shipped finish reviewer, `impeccable-finish-reviewer` (`impeccable_finish_reviewer` in codex; `/impeccable-finish-reviewer` in Cursor; on GitHub Copilot say "Use the impeccable-finish-reviewer agent"), with the original request, confirmed answers, the artifact path, the screenshot paths, its direction contract, existing hook findings, the QUALITY BAR card and approved comp paths (on a code-led build there is no approved comp; the chosen decision comp rides in that slot as the critique reference, named as such), and the craft-floor reference path. The reviewer has no browser; screenshots you fail to pass are checks it cannot run. Never read the shipped agents' definition files before spawning; the harness loads them at spawn, and you owe only the input packet. Wait on any agent with one long timeout rather than a loop of short polls, and spend the wait on the next independent step. Verify its return carries the five contract sections; on an empty or thrashed return, respawn once with the same inputs before doing anything else. This review never runs inside the build thread and never inherits it: spawn the reviewer fresh, with no forked conversation history (`fork_turns: 0` in codex); a reviewer that inherits your transcript inherits your framing, your optimism, and your abstractions, and everything it needs travels in the inputs above. Only a harness whose tool surface has no subagent capability at all substitutes a fresh in-thread pass after stepping fully out of the build context, run from [degraded/finish-reviewer.md](degraded/finish-reviewer.md), and a substituted or failed-and-replaced review is disclosed in one line at finish, never silently. When the reviewer's first material fix is a rebuild directive, fidelity failed wholesale rather than in patches, so skip the fix batch and execute the rebuild immediately: re-derive the named regions, produce the named assets, and send the result back for a verdict, telling the user what is happening rather than asking permission to fix a failure. The user is consulted only when a second rebuild directive arrives, both verdicts on the table, or when rebuilding would discard content the user approved. Otherwise apply the material fixes in one batch, rebuild once, and recapture the same viewports. A recapture measures positions, loading, and overflow; it cannot measure whether a fix reached the quality the finding named, so send the recaptured screenshots back to the same reviewer for a verdict scoring every material fix resolved, partial, or unresolved (through the harness's agent continuation; without one, run the scoring fresh from [degraded/finish-reviewer.md](degraded/finish-reviewer.md)'s Verdict Pass). Fixes scored partial or unresolved get another batch, recapture, and verdict. Two rounds is the budget an unattended run ends at; an attended session's ceiling belongs to the user, so when the second verdict still lists open items, put the table in front of them and let them choose between shipping as it stands and funding another round. Whoever is deciding, stop the moment a round resolves nothing, and the reviewer's findings are the only list you work from, never your own re-opened hunt. Report the final verdict table to the user as it stands, open items included, under the reviewer's own disposition word: a table with open material findings is never announced as a pass, and never under a softer label than the reviewer wrote. Do not run a second detector. +After the second inspection round the build thread's polishing is over: no further defect hunts, micro-edit scripts, or rebuilds here; whatever remains ships through the handoffs, where a fresh context does the finding better and cheaper. Where this harness runs no design hook, run `node {{scripts_path}}/detect.mjs --json` on the changed targets once here, fix what is mechanical, and pass the remaining findings to the reviewer; a hookless build that skips this ships every tell the hook exists to catch. Capture the screenshots into `.impeccable/review/`, one file per captured viewport (on the web, `desktop.png` and `mobile.png`), creating that directory when the harness does not; the paths you pass the reviewer are its spec, and that directory is where it looks when a passed path is missing. Then spawn the shipped finish reviewer, `impeccable-finish-reviewer` (`impeccable_finish_reviewer` in codex; `/impeccable-finish-reviewer` in Cursor; on GitHub Copilot say "Use the impeccable-finish-reviewer agent"), with the original request, confirmed answers, the artifact path, the screenshot paths, its direction contract, existing hook findings, the QUALITY BAR card and approved comp paths (on a code-led build there is no approved comp; the chosen decision comp rides in that slot as the critique reference, named as such), and the craft-floor reference path. The reviewer has no browser; screenshots you fail to pass are checks it cannot run. Never read the shipped agents' definition files before spawning; the harness loads them at spawn, and you owe only the input packet. Wait on any agent with one long timeout rather than a loop of short polls, and spend the wait on the next independent step. Verify its return carries the five contract sections; on an empty or thrashed return, respawn once with the same inputs before doing anything else. This review never runs inside the build thread and never inherits it: spawn the reviewer fresh, with no forked conversation history (`fork_turns: 0` in codex); a reviewer that inherits your transcript inherits your framing, your optimism, and your abstractions, and everything it needs travels in the inputs above. Only a harness whose tool surface has no subagent capability at all substitutes a fresh in-thread pass after stepping fully out of the build context, run from [degraded/finish-reviewer.md](degraded/finish-reviewer.md), and a substituted or failed-and-replaced review is disclosed in one line at finish, never silently. When the reviewer's first material fix is a rebuild directive, fidelity failed wholesale rather than in patches, so skip the fix batch and execute the rebuild immediately: re-derive the named regions, produce the named assets, and send the result back for a verdict, telling the user what is happening rather than asking permission to fix a failure. The user is consulted only when a second rebuild directive arrives, both verdicts on the table, or when rebuilding would discard content the user approved. Otherwise apply the material fixes in one batch, rebuild once, and recapture the same viewports over the same files. A recapture measures positions, loading, and overflow; it cannot measure whether a fix reached the quality the finding named, so send the recaptured screenshots back to the same reviewer for a verdict scoring every material fix resolved, partial, or unresolved (through the harness's agent continuation; without one, run the scoring fresh from [degraded/finish-reviewer.md](degraded/finish-reviewer.md)'s Verdict Pass). Fixes scored partial or unresolved get another batch, recapture, and verdict. Two rounds is the budget an unattended run ends at; an attended session's ceiling belongs to the user, so when the second verdict still lists open items, put the table in front of them and let them choose between shipping as it stands and funding another round. Whoever is deciding, stop the moment a round resolves nothing, and the reviewer's findings are the only list you work from, never your own re-opened hunt. Report the final verdict table to the user as it stands, open items included, under the reviewer's own disposition word: a table with open material findings is never announced as a pass, and never under a softer label than the reviewer wrote. Do not run a second detector. Then spawn the shipped documenter, `impeccable-documenter` (`impeccable_documenter` in codex), with the project root, the artifact path, the direction contract, PRODUCT.md, the [document.md](document.md) reference path, and the boundary to write at; it records DESIGN.md and the sidecar from the built world, ground truth over intention; without subagents the pass runs from [degraded/documenter.md](degraded/documenter.md). A clean detector pass is not finished; finished is the contract kept, the comp honored, the review closed, and the system recorded. diff --git a/tests/build.test.js b/tests/build.test.js index 44a21dd8d..6f6865237 100644 --- a/tests/build.test.js +++ b/tests/build.test.js @@ -647,3 +647,87 @@ describe('Cursor subagent generation', () => { expect(assetProducer).toContain('.cursor/skills/impeccable/scripts'); }); }); + +// Regression guard for the gap that shipped literal `{{scripts_path}}` inside +// the Codex dists' nested agent .toml: three separate code paths emit an agent +// body, and one of them skipped placeholder substitution and rule-marker +// stripping. Assert every surface, not just the one that was broken. +describe('agent bodies resolve placeholders on every surface that ships them', () => { + const ROOT = process.cwd(); + const AGENT_TEST_DIR = path.join(ROOT, 'test-tmp-agent-placeholders'); + const DIST = path.join(AGENT_TEST_DIR, 'dist'); + + // [emitted file, the scripts path that provider installs to] + const SURFACES = [ + // Nested Codex .toml: the skill install is the whole delivery for these. + ['codex/.codex/skills/impeccable/agents/impeccable_asset_producer.toml', '.codex/skills/impeccable/scripts'], + ['agents/.agents/skills/impeccable/agents/impeccable_asset_producer.toml', '.agents/skills/impeccable/scripts'], + // Native agent files. + ['claude-code/.claude/agents/impeccable-asset-producer.md', '.claude/skills/impeccable/scripts'], + ['github/.github/agents/impeccable-asset-producer.agent.md', '.github/skills/impeccable/scripts'], + ['grok/.grok/agents/impeccable-asset-producer.md', '.grok/skills/impeccable/scripts'], + // Degraded fallback reference generated from the same agent definition. + ['codex/.codex/skills/impeccable/reference/degraded/asset-producer.md', '.codex/skills/impeccable/scripts'], + ]; + + beforeEach(() => { + if (fs.existsSync(AGENT_TEST_DIR)) fs.rmSync(AGENT_TEST_DIR, { recursive: true, force: true }); + fs.mkdirSync(AGENT_TEST_DIR, { recursive: true }); + const { skills } = utils.readSourceFiles(ROOT); + transformers.transformCodex(skills, DIST); + transformers.transformAgents(skills, DIST); + transformers.transformClaudeCode(skills, DIST); + transformers.transformGitHub(skills, DIST); + transformers.transformGrok(skills, DIST); + }); + + afterEach(() => { + if (fs.existsSync(AGENT_TEST_DIR)) fs.rmSync(AGENT_TEST_DIR, { recursive: true, force: true }); + }); + + test('the asset producer ships a runnable embed-prompt command, never the raw token', () => { + for (const [relPath, scriptsPath] of SURFACES) { + const content = fs.readFileSync(path.join(DIST, relPath), 'utf-8'); + expect(content).toContain(`node ${scriptsPath}/embed-prompt.mjs`); + expect(content).not.toContain('{{scripts_path}}'); + } + }); + + test('no emitted agent body carries an unresolved placeholder or a rule marker', () => { + const synthetic = { + name: 'impeccable', + description: 'synthetic', + body: 'Synthetic skill body.', + agents: [ + { + name: 'impeccable-synthetic', + codexName: 'impeccable_synthetic', + description: 'synthetic agent', + body: 'Run `node {{scripts_path}}/embed-prompt.mjs` and ask {{model}}. ', + }, + ], + }; + const synthDist = path.join(AGENT_TEST_DIR, 'synth'); + transformers.transformCodex([synthetic], synthDist); + transformers.transformClaudeCode([synthetic], synthDist); + + const emitted = [ + 'codex/.codex/skills/impeccable/agents/impeccable_synthetic.toml', + 'codex/.codex/skills/impeccable/reference/degraded/synthetic.md', + 'claude-code/.claude/agents/impeccable-synthetic.md', + ]; + for (const relPath of emitted) { + const content = fs.readFileSync(path.join(synthDist, relPath), 'utf-8'); + expect(content).not.toContain('{{'); + expect(content).not.toMatch(/