From d16a1687f965ceca641a32f779b37f4b36f065a5 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 4 Oct 2026 17:50:11 +0000 Subject: [PATCH] E2: one make-pdf resolver for readiness and every $P block on env-var hosts make-pdf exported $P from its readiness block, which Codex and the other env-var hosts lose between blocks. binaryAssignment gains 'make-pdf': the MAKE-PDF SETUP readiness check and the runtime prelude both use it, so every block that runs $P derives the same make-pdf/dist/pdf binary, with MAKE_PDF_BIN (dev builds) winning when it is executable. Claude output keeps its repo-local-first probe; only the false 'subsequent blocks' comment changed. Root plus P=... fits the 400-byte prelude budget; the fixed MAKE_PDF_OVERRIDE segment is the one exemption, named in runtime-root.ts and the conformance test. --- scripts/resolvers/make-pdf.ts | 27 +++++----- scripts/resolvers/runtime-root.ts | 37 +++++++++---- test/env-host-fence-conformance.test.ts | 70 +++++++++++++++++++++---- 3 files changed, 100 insertions(+), 34 deletions(-) diff --git a/scripts/resolvers/make-pdf.ts b/scripts/resolvers/make-pdf.ts index 55729b0cc..52216d92e 100644 --- a/scripts/resolvers/make-pdf.ts +++ b/scripts/resolvers/make-pdf.ts @@ -1,29 +1,26 @@ -import { type TemplateContext, toShellPath } from './types'; +import { type TemplateContext } from './types'; +import { binaryAssignment } from './runtime-root'; /** - * {{MAKE_PDF_SETUP}} — emits the shell preamble that resolves $P to the - * make-pdf binary. Mirrors generateDesignSetup. + * {{MAKE_PDF_SETUP}} — emits the readiness check that resolves $P to the + * make-pdf binary (make-pdf/dist/pdf). Mirrors generateDesignSetup: the + * assignment comes from binaryAssignment(ctx, 'make-pdf'), the same resolver + * the runtime prelude puts into every later block that uses $P on env-var + * hosts, so readiness and use always name the same binary. * - * $P = make-pdf/dist/pdf. - * - * Resolution order: - * 1. Local skill root: $_ROOT/{localSkillRoot}/make-pdf/dist/pdf - * 2. Global: ~/{globalRoot}/make-pdf/dist/pdf - * 3. Env override (MAKE_PDF_BIN) — for contributor dev builds + * Resolution order: MAKE_PDF_BIN (contributor dev builds), then the + * repo-local install, then the global install (env-var hosts: $GSTACK_ROOT, + * which the prelude resolves repo-local first). */ export function generateMakePdfSetup(ctx: TemplateContext): string { return `## MAKE-PDF SETUP (run this check BEFORE any make-pdf command) \`\`\`bash -_ROOT=$(git rev-parse --show-toplevel 2>/dev/null) -P="" -[ -n "$MAKE_PDF_BIN" ] && [ -x "$MAKE_PDF_BIN" ] && P="$MAKE_PDF_BIN" -[ -z "$P" ] && [ -n "$_ROOT" ] && [ -x "$_ROOT/${ctx.paths.localSkillRoot}/make-pdf/dist/pdf" ] && P="$_ROOT/${ctx.paths.localSkillRoot}/make-pdf/dist/pdf" -[ -z "$P" ] && P="${toShellPath(ctx.paths.skillRoot)}/make-pdf/dist/pdf" +${binaryAssignment(ctx, 'make-pdf')} if [ -x "$P" ]; then echo "MAKE_PDF_READY: $P" alias _p_="$P" # shellcheck alias helper (not exported) - export P # available as $P in subsequent blocks within the same skill invocation + export P # env-var hosts re-derive $P in every later block (runtime prelude) else echo "MAKE_PDF_NOT_AVAILABLE (run './setup' in the gstack repo to build it)" fi diff --git a/scripts/resolvers/runtime-root.ts b/scripts/resolvers/runtime-root.ts index e9e6a507e..31df6b168 100644 --- a/scripts/resolvers/runtime-root.ts +++ b/scripts/resolvers/runtime-root.ts @@ -2,8 +2,8 @@ * Runtime root for env-var hosts (C1, #1159): the one owner of how generated * bash finds gstack on Codex and every other `usesEnvVars` host. * - * Those hosts run each fenced block in a fresh shell, so `GSTACK_ROOT`, `B` - * and `D` set by an earlier block are gone. The preamble's root resolution and + * Those hosts run each fenced block in a fresh shell, so `GSTACK_ROOT`, `B`, + * `D` and `P` set by an earlier block are gone. The preamble's root resolution and * the per-fence prelude come from `runtimeRootPrelude()`; `insertRuntimePreludes()` * is the single post-render pass (gen-skill-docs, before rewriteInstallRoot) * that puts it into every fence that uses these variables without assigning @@ -19,6 +19,13 @@ import { toShellPath, type TemplateContext } from './types'; /** Per-fence byte budget for the prelude (T-ENG1), asserted by the INV-3 test. */ export const PRELUDE_BYTE_BUDGET = 400; +/** + * make-pdf's dev-build override (E2), appended to `P`'s assignment. It is the + * one fixed segment outside PRELUDE_BYTE_BUDGET: root plus `P=...` fits the + * budget like `B`/`D` do, and the override is needed in every block that runs + * `$P` so readiness and use name the same binary. + */ +export const MAKE_PDF_OVERRIDE = ';[ -x "${MAKE_PDF_BIN:-}" ]&&P=$MAKE_PDF_BIN||:'; const ROOT_VARS = /\$\{?GSTACK_(?:ROOT|BIN|BROWSE|DESIGN|MAKE_PDF)\b/; const assigns = (name: string) => new RegExp(`(?:^|[\\s;&|(])${name}=`, 'm'); @@ -48,13 +55,23 @@ export function runtimeRootPrelude(ctx: TemplateContext): string { } /** - * Lines that assign `B` (browse) or `D` (design). BROWSE SETUP, DESIGN SETUP and - * the prelude all use this, so a later fence runs the binary setup checked. - * Env-var hosts derive from `GSTACK_ROOT`, which the prelude already points at - * the repo-local install when one exists; other hosts keep their - * repo-local-first probe. + * Lines that assign `B` (browse), `D` (design) or `P` (make-pdf). BROWSE SETUP, + * DESIGN SETUP, MAKE-PDF SETUP and the prelude all use this, so a later fence + * runs the binary setup checked. Env-var hosts derive from `GSTACK_ROOT`, which + * the prelude already points at the repo-local install when one exists; other + * hosts keep their repo-local-first probe. make-pdf's binary is + * `make-pdf/dist/pdf`, and `MAKE_PDF_BIN` (a contributor dev build) wins on + * every host. */ -export function binaryAssignment(ctx: TemplateContext, tool: 'browse' | 'design'): string { +export function binaryAssignment(ctx: TemplateContext, tool: 'browse' | 'design' | 'make-pdf'): string { + if (tool === 'make-pdf') { + if (getHostConfig(ctx.host).usesEnvVars) return `P=$GSTACK_ROOT/make-pdf/dist/pdf${MAKE_PDF_OVERRIDE}`; + return `_ROOT=$(git rev-parse --show-toplevel 2>/dev/null) +P="" +[ -n "$MAKE_PDF_BIN" ] && [ -x "$MAKE_PDF_BIN" ] && P="$MAKE_PDF_BIN" +[ -z "$P" ] && [ -n "$_ROOT" ] && [ -x "$_ROOT/${ctx.paths.localSkillRoot}/make-pdf/dist/pdf" ] && P="$_ROOT/${ctx.paths.localSkillRoot}/make-pdf/dist/pdf" +[ -z "$P" ] && P="${toShellPath(ctx.paths.skillRoot)}/make-pdf/dist/pdf"`; + } const v = tool === 'browse' ? 'B' : 'D'; const dir = tool === 'browse' ? ctx.paths.browseDir : ctx.paths.designDir; if (getHostConfig(ctx.host).usesEnvVars) return `${v}=$GSTACK_ROOT/${tool}/dist/${tool}`; @@ -69,8 +86,9 @@ export function fencePrelude(ctx: TemplateContext, body: string): string { if (!getHostConfig(ctx.host).usesEnvVars) return ''; const needB = uses('B').test(body) && !assigns('B').test(body); const needD = uses('D').test(body) && !assigns('D').test(body); + const needP = uses('P').test(body) && !assigns('P').test(body); if (assigns('GSTACK_ROOT').test(body)) return ''; - const needRoot = needB || needD || ROOT_VARS.test(body); + const needRoot = needB || needD || needP || ROOT_VARS.test(body); const derived = Object.entries(DERIVED) .filter(([name]) => uses(`GSTACK_${name}`).test(body) && !assigns(`GSTACK_${name}`).test(body)) .map(([name, dir]) => `GSTACK_${name}=$GSTACK_ROOT/${dir}`); @@ -79,6 +97,7 @@ export function fencePrelude(ctx: TemplateContext, body: string): string { derived.join(' '), needB ? binaryAssignment(ctx, 'browse') : '', needD ? binaryAssignment(ctx, 'design') : '', + needP ? binaryAssignment(ctx, 'make-pdf') : '', ].filter(Boolean).join('\n'); } diff --git a/test/env-host-fence-conformance.test.ts b/test/env-host-fence-conformance.test.ts index 872f3d304..78b37d98a 100644 --- a/test/env-host-fence-conformance.test.ts +++ b/test/env-host-fence-conformance.test.ts @@ -16,13 +16,14 @@ import * as os from 'os'; import * as path from 'path'; import { ALL_HOST_CONFIGS } from '../hosts'; import { runGeneration } from '../scripts/gen-skill-docs'; -import { binaryAssignment, fencePrelude, insertRuntimePreludes, PRELUDE_BYTE_BUDGET, runtimeRootPrelude } from '../scripts/resolvers/runtime-root'; +import { generateMakePdfSetup } from '../scripts/resolvers/make-pdf'; +import { binaryAssignment, fencePrelude, insertRuntimePreludes, MAKE_PDF_OVERRIDE, PRELUDE_BYTE_BUDGET, runtimeRootPrelude } from '../scripts/resolvers/runtime-root'; import { HOST_PATHS, type TemplateContext } from '../scripts/resolvers/types'; import { lintFence, normalizePlaceholders } from './helpers/generated-bash-lint'; const ENV_HOSTS = ALL_HOST_CONFIGS.filter(h => h.usesEnvVars); const FORBIDDEN = /^(?:\/bin\/|\/browse|\/design|\/gstack-)/; -const PRELUDE_LINE = /^\[ -d "\$\{GSTACK_ROOT:-\/-\}\/bin" \]|^(?:GSTACK_(?:BIN|BROWSE|DESIGN|MAKE_PDF)=\$GSTACK_ROOT\/\S+ ?)+$|^[BD]=\$GSTACK_ROOT\//; +const PRELUDE_LINE = /^\[ -d "\$\{GSTACK_ROOT:-\/-\}\/bin" \]|^(?:GSTACK_(?:BIN|BROWSE|DESIGN|MAKE_PDF)=\$GSTACK_ROOT\/\S+ ?)+$|^[BDP]=\$GSTACK_ROOT\//; const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-env-fences-')); const renderDir = path.join(tmp, 'render'); @@ -70,8 +71,10 @@ function fenceProblems(fence: Fence): string[] { if (/\$\{?GSTACK_(?:ROOT|BIN|BROWSE|DESIGN|MAKE_PDF)\b/.test(fence.body) && !assigned('GSTACK_ROOT')) problems.push(`${where} uses GSTACK_* without resolving GSTACK_ROOT`); if (/\$\{?B\b/.test(fence.body) && !assigned('B')) problems.push(`${where} uses $B without deriving it`); if (/\$\{?D\b/.test(fence.body) && !assigned('D')) problems.push(`${where} uses $D without deriving it`); + if (/\$\{?P\b/.test(fence.body) && !/(?:^|[\s;&|(])P=/m.test(fence.body)) problems.push(`${where} uses $P without deriving it`); if (fence.body.includes('gstack: no install found')) { - const bytes = Buffer.byteLength(fence.body.split('\n').filter(l => PRELUDE_LINE.test(l)).join('\n') + '\n'); + // MAKE_PDF_OVERRIDE is the one fixed segment outside the budget (E2). + const bytes = Buffer.byteLength(fence.body.split('\n').filter(l => PRELUDE_LINE.test(l)).join('\n').replace(MAKE_PDF_OVERRIDE, '') + '\n'); if (bytes > PRELUDE_BYTE_BUDGET) problems.push(`${where} prelude is ${bytes} bytes (budget ${PRELUDE_BYTE_BUDGET})`); if (fence.body.split('gstack: no install found').length !== 2) problems.push(`${where} carries the prelude more than once`); } @@ -84,7 +87,7 @@ function sh(script: string, cwd: string, env: Record) { return spawnSync('bash', ['-uc', script], { cwd, encoding: 'utf8', timeout: 10_000, env: { PATH: process.env.PATH ?? '/usr/bin:/bin', ...env } }); } function mkroot(dir: string) { - for (const sub of ['bin', 'lib', 'browse/dist', 'design/dist']) fs.mkdirSync(path.join(dir, sub), { recursive: true }); + for (const sub of ['bin', 'lib', 'browse/dist', 'design/dist', 'make-pdf/dist']) fs.mkdirSync(path.join(dir, sub), { recursive: true }); return dir; } @@ -95,13 +98,13 @@ beforeAll(async () => { afterAll(() => fs.rmSync(tmp, { recursive: true, force: true })); describe('C1: every env-var host fence resolves its own runtime paths', () => { - test('each fence that uses $GSTACK_*, $B or $D assigns them in the same fence, within the byte budget', () => { + test('each fence that uses $GSTACK_*, $B, $D or $P assigns them in the same fence, within the byte budget', () => { const problems: string[] = []; let checked = 0; for (const host of ENV_HOSTS) { for (const file of renderedDocs(renderDir, host.hostSubdir)) { for (const fence of bashFences(fs.readFileSync(file, 'utf8'), path.relative(renderDir, file))) { - if (!/\$\{?(?:GSTACK_(?:ROOT|BIN|BROWSE|DESIGN|MAKE_PDF)|B|D)\b/.test(fence.body)) continue; + if (!/\$\{?(?:GSTACK_(?:ROOT|BIN|BROWSE|DESIGN|MAKE_PDF)|B|D|P)\b/.test(fence.body)) continue; checked++; problems.push(...fenceProblems(fence)); } @@ -234,6 +237,7 @@ describe('INV-3: every rendered fence parses, runs its prelude and passes the li for (const host of ENV_HOSTS) { const root = mkroot(path.join(home, host.name === 'codex' ? '.codex/skills/gstack' : host.globalRoot)); for (const tool of ['browse', 'design']) fs.writeFileSync(path.join(root, tool, 'dist', tool), `#!/bin/sh\necho "STUB_${tool}"\n`, { mode: 0o755 }); + fs.writeFileSync(path.join(root, 'make-pdf', 'dist', 'pdf'), '#!/bin/sh\necho "STUB_pdf"\n', { mode: 0o755 }); const preludes = new Set(); for (const file of renderedDocs(renderDir, host.hostSubdir)) { for (const fence of bashFences(fs.readFileSync(file, 'utf8'))) { @@ -244,8 +248,8 @@ describe('INV-3: every rendered fence parses, runs its prelude and passes the li expect(preludes.size).toBeGreaterThan(0); for (const prelude of preludes) { ran++; - const vars = [...new Set([...prelude.matchAll(/(?:^|\s)(GSTACK_[A-Z_]+|[BD])=/gm)].map(m => m[1]))]; - const consumers = vars.filter(v => v === 'B' || v === 'D').map(v => `"$${v}"`).join('\n'); + const vars = [...new Set([...prelude.matchAll(/(?:^|\s)(GSTACK_[A-Z_]+|[BDP])=/gm)].map(m => m[1]))]; + const consumers = vars.filter(v => v === 'B' || v === 'D' || v === 'P').map(v => `"$${v}"`).join('\n'); const script = `${prelude}\n${vars.map(v => `printf '%s=%s\\n' ${v} "$${v}"`).join('\n')}\n${consumers}`; const r = spawnSync('env', ['-i', `HOME=${home}`, 'PATH=/usr/bin:/bin', 'bash', '-uc', script], { cwd: w, encoding: 'utf8', timeout: 10_000 }); if (r.status !== 0) { problems.push(`${host.name}: exit ${r.status}: ${r.stderr.trim()}\n${prelude}`); continue; } @@ -253,8 +257,8 @@ describe('INV-3: every rendered fence parses, runs its prelude and passes the li const value = line.slice(line.indexOf('=') + 1); if (!value || FORBIDDEN.test(value)) problems.push(`${host.name}: ${line}`); } - for (const v of vars.filter(x => x === 'B' || x === 'D')) { - if (!r.stdout.includes(`STUB_${v === 'B' ? 'browse' : 'design'}`)) problems.push(`${host.name}: $${v} did not run the stub binary`); + for (const v of vars.filter(x => x === 'B' || x === 'D' || x === 'P')) { + if (!r.stdout.includes(`STUB_${{ B: 'browse', D: 'design', P: 'pdf' }[v]}`)) problems.push(`${host.name}: $${v} did not run the stub binary`); } } } @@ -302,3 +306,49 @@ describe('INV-3: negative controls (planted bad fences fail each check)', () => expect(fenceProblems({ file: 'x', line: 1, body: twice })).toContain('x:1 carries the prelude more than once'); }); }); + +// E2: make-pdf's $P comes from one resolver (binaryAssignment 'make-pdf') for +// the readiness check and every later block, honoring MAKE_PDF_BIN. +describe('E2: make-pdf $P on env-var hosts', () => { + const pdfStub = (file: string, label: string) => { + fs.mkdirSync(path.dirname(file), { recursive: true }); + fs.writeFileSync(file, `#!/bin/sh\necho "${label} $*"\n`, { mode: 0o755 }); + }; + + test('readiness and a later block resolve the same binary, MAKE_PDF_BIN wins, paths with spaces work', () => { + const w = fs.mkdtempSync(path.join(tmp, 'pdf with spaces-')); + const home = path.join(w, 'home dir'); + const root = mkroot(path.join(w, 'gstack root')); + pdfStub(path.join(root, 'make-pdf', 'dist', 'pdf'), 'ROOT_PDF'); + const dev = path.join(w, 'dev build', 'pdf'); + pdfStub(dev, 'DEV_PDF'); + const c = { ...ctx('codex'), skillName: 'make-pdf' }; + const readiness = bashFences(insertRuntimePreludes(generateMakePdfSetup(c), c))[0].body; + const later = fencePrelude(c, '"$P" generate letter.md') + '\n"$P" generate letter.md'; + const env = (extra: Record = {}) => + ['-i', `HOME=${home}`, 'PATH=/usr/bin:/bin', `GSTACK_ROOT=${root}`, ...Object.entries(extra).map(([k, v]) => `${k}=${v}`)]; + const run = (script: string, extra?: Record) => + spawnSync('env', [...env(extra), 'bash', '-uc', script], { cwd: w, encoding: 'utf8', timeout: 10_000 }); + + let r = run(readiness); + expect(r.stdout, r.stderr).toContain(`MAKE_PDF_READY: ${root}/make-pdf/dist/pdf`); + r = run(later); + expect(r.stdout.trim(), r.stderr).toBe('ROOT_PDF generate letter.md'); + + r = run(readiness, { MAKE_PDF_BIN: dev }); + expect(r.stdout, r.stderr).toContain(`MAKE_PDF_READY: ${dev}`); + r = run(later, { MAKE_PDF_BIN: dev }); + expect(r.stdout.trim(), r.stderr).toBe('DEV_PDF generate letter.md'); + + // A MAKE_PDF_BIN that is not executable falls back to the install, in both places. + r = run(later, { MAKE_PDF_BIN: path.join(w, 'missing pdf') }); + expect(r.stdout.trim(), r.stderr).toBe('ROOT_PDF generate letter.md'); + }); + + test('Claude keeps its repo-local-first probe with MAKE_PDF_BIN first; only the comment changed', () => { + const block = bashFences(generateMakePdfSetup({ ...ctx('claude'), skillName: 'make-pdf' }))[0].body; + expect(block).toContain(binaryAssignment(ctx('claude'), 'make-pdf')); + expect(block.indexOf('MAKE_PDF_BIN')).toBeLessThan(block.indexOf('_ROOT/')); + expect(block).not.toContain('available as $P in subsequent blocks'); + }); +});