From 13560660ae929695a161f44dadee787ea583d68c Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Thu, 12 Nov 2026 19:50:45 -0600 Subject: [PATCH] fix(packs): SP8 review round, single-pass placeholders, CLI hints, pins (#1873) - template-save: one alternation pass replaces every literal, so an instance named root, org or agent never rewrites a placeholder already written in. checkPack's refusal message is passed on as the reason. A write or fsync failure removes the partial file it created. - broker #templateFail drops a reason the credential check refuses. - cli template words: humanTransport throws a CliError named by the bus code; a known code now gains its hint (the test transport throws the same CliError). - template-http: the header notes serve.mjs strips /api/orgs/. - pins: the two .console verbs in console-decide and request-console; the packages/business/src re-export for template-save.mjs. - tests: placeholder collision, a held-value key masked as [key], and a save after a real pack.apply.console keeping {id, version}. --- packages/bus/src/broker.mjs | 7 +++- packages/bus/tests/console-decide.test.mjs | 4 +- packages/bus/tests/request-console.test.mjs | 4 +- packages/bus/tests/templates.test.mjs | 26 +++++++++++++ packages/business/src/template-save.mjs | 2 + packages/cli/src/org.mjs | 8 ++-- packages/cli/tests/org-templates.test.mjs | 5 ++- packages/organization/src/template-save.mjs | 39 +++++++++++++------ .../organization/tests/template-save.test.mjs | 11 ++++++ packages/webui/src/template-http.mjs | 3 ++ 10 files changed, 90 insertions(+), 19 deletions(-) create mode 100644 packages/business/src/template-save.mjs diff --git a/packages/bus/src/broker.mjs b/packages/bus/src/broker.mjs index a351d1fbc..4df950a9a 100644 --- a/packages/bus/src/broker.mjs +++ b/packages/bus/src/broker.mjs @@ -7046,7 +7046,12 @@ export class Broker { // #1873 SP8: a template refusal keeps its code and its reason (a path, never the text); anything // else is read as a pack refusal. #templateFail(e) { - if (e?.name === 'TemplateError' && TEMPLATE_CODES.includes(e.code)) throw new BusError(e.code, typeof e.reason === 'string' ? e.reason.slice(0, 300) : undefined); + if (e?.name === 'TemplateError' && TEMPLATE_CODES.includes(e.code)) { + // A reason names a field path or a definition name; one that would quote a held value is dropped. + let reason = typeof e.reason === 'string' ? e.reason.slice(0, 300) : undefined; + if (reason !== undefined) try { this.#secretCheck(reason); } catch { reason = undefined; } + throw new BusError(e.code, reason); + } this.#packFail(e); } // The bus code for an organization writer's refusal. Anything the writer didn't refuse by name is diff --git a/packages/bus/tests/console-decide.test.mjs b/packages/bus/tests/console-decide.test.mjs index edc179812..3496f4e78 100644 --- a/packages/bus/tests/console-decide.test.mjs +++ b/packages/bus/tests/console-decide.test.mjs @@ -92,7 +92,9 @@ test('N4: owner Console verbs have an explicit immutable whitelist and exclude d assert.deepEqual([...CONSOLE_VERBS], ['system-types.console', 'project.remote.get.console', 'project.remote.set.console', 'autonomy.get.console', 'autonomy.set.console', 'autonomy.review.console', 'decision.confirm.console', 'decision.reverse.console', 'request.workspace.console', 'request.artifact.console', 'decision.preview.console', 'decision.resolve.console', 'hire.preview.console', 'hire.resolve.console', 'hire.start.console', 'hire.templates.console', 'prd.revoke.console', 'request.mode.console', 'request.draft.console', 'request.get.console', 'request.projects.console', 'request.answer.console', 'request.attach.console', 'request.cancel.console', // Starter packs SP4 (#1873): the owner's pack list and preview, and the three pack writes. - 'packs.console', 'pack.preview.console', 'pack.apply.console', 'board.set.console', 'workflow.set.console']); + 'packs.console', 'pack.preview.console', 'pack.apply.console', 'board.set.console', 'workflow.set.console', + // Starter packs SP8 (#1873): the owner saves and lists templates. + 'template.save.console', 'template.list.console']); assert.ok(CONSOLE_VERBS.every(verb => verb.endsWith('.console'))); assert.equal(CONSOLE_VERBS.includes('decision.resolve'), false); assert.ok(Object.isFrozen(CONSOLE_VERBS)); diff --git a/packages/bus/tests/request-console.test.mjs b/packages/bus/tests/request-console.test.mjs index 178e0bff5..a185712d3 100644 --- a/packages/bus/tests/request-console.test.mjs +++ b/packages/bus/tests/request-console.test.mjs @@ -93,7 +93,9 @@ test('the console verb list includes the artifact read and existing owner contro assert.deepEqual([...CONSOLE_VERBS], ['system-types.console', 'project.remote.get.console', 'project.remote.set.console', 'autonomy.get.console', 'autonomy.set.console', 'autonomy.review.console', 'decision.confirm.console', 'decision.reverse.console', 'request.workspace.console', 'request.artifact.console', 'decision.preview.console', 'decision.resolve.console', 'hire.preview.console', 'hire.resolve.console', 'hire.start.console', 'hire.templates.console', 'prd.revoke.console', 'request.mode.console', 'request.draft.console', 'request.get.console', 'request.projects.console', 'request.answer.console', 'request.attach.console', 'request.cancel.console', // Starter packs SP4 (#1873): the owner's pack list and preview, and the three pack writes. - 'packs.console', 'pack.preview.console', 'pack.apply.console', 'board.set.console', 'workflow.set.console']); + 'packs.console', 'pack.preview.console', 'pack.apply.console', 'board.set.console', 'workflow.set.console', + // Starter packs SP8 (#1873): the owner saves and lists templates. + 'template.save.console', 'template.list.console']); }); test('only the signed-in owner capability reaches the console verbs and it reaches nothing else', async t => { diff --git a/packages/bus/tests/templates.test.mjs b/packages/bus/tests/templates.test.mjs index a372ce825..7d576500d 100644 --- a/packages/bus/tests/templates.test.mjs +++ b/packages/bus/tests/templates.test.mjs @@ -124,6 +124,32 @@ test('SP8 the real credential store: a held value in kept text refuses template- assert.ok(!existsSync(join(root, 'templates'))); }); +test('SP8 a key that holds a held value: the refusal reason masks it as [key], never the value', async t => { + // A definition keyed by one never loads (definition-holds-credential); a workflow keyed by one is + // what save sees. The broker also drops any reason its credential check refuses. + const workflow = { description: 'Ship it.', steps: [{ title: 'Write', state: 'todo', roles: ['writer'] }] }; + const { call, root } = await harness(t, { mutate: doc => { doc.workflows = { [TOKEN]: workflow }; } }); + await assert.rejects(call('template.save.console', { name: 'leaky', preview: true }), (e) => { + assert.equal(e.code, 'template-secret-suspected'); + assert.equal(e.reason, 'workflows[key]'); + assert.ok(!String(e.message).includes(TOKEN) && !JSON.stringify(e).includes(TOKEN)); + return true; + }); + assert.ok(!existsSync(join(root, 'templates'))); +}); + +test('SP8 a save after a real pack.apply.console keeps each packs entry as {id, version}', async t => { + const { call, saved } = await harness(t); + const packs = await call('packs.console'); + const shipped = packs.packs.find(p => p.origin === 'shipped'); + assert.ok(shipped, JSON.stringify(packs.packs.map(p => [p.id, p.origin]))); + const pack = await call('pack.preview.console', { pack: shipped.id }); + await call('pack.apply.console', { pack: shipped.id, digest: pack.digest }); + await save(call, 'after-apply'); + const kept = JSON.parse(readFileSync(saved('after-apply'), 'utf8')).packs; + assert.deepEqual(kept, [{ id: shipped.id, version: pack.pack.version }]); +}); + test('SP8 argument checks: a second save, a .. name, an unknown key, preview with a digest, and a stale digest', async t => { const { call, saved } = await harness(t); const { preview } = await save(call, 'demo-shape'); diff --git a/packages/business/src/template-save.mjs b/packages/business/src/template-save.mjs new file mode 100644 index 000000000..56271b7f0 --- /dev/null +++ b/packages/business/src/template-save.mjs @@ -0,0 +1,2 @@ +// Old path of packages/organization/src/template-save.mjs (rename design r2, stage B). +export * from "../../organization/src/template-save.mjs"; diff --git a/packages/cli/src/org.mjs b/packages/cli/src/org.mjs index 95821b0ba..57bbcd531 100644 --- a/packages/cli/src/org.mjs +++ b/packages/cli/src/org.mjs @@ -149,9 +149,11 @@ async function templateCommand(args, io, deps) { let result; try { result = await packCall(id, io, deps)(verb, request); } catch (e) { - if (typeof e?.code !== 'string' || e.code === '' || e instanceof CliError) throw e; - const why = TEMPLATE_REFUSALS[e.code] ?? PACK_REFUSALS[e.code]; - const error = new CliError(why ? `${e.code}: ${why}` : e.code, busExit(e.code)); + // humanTransport throws a CliError named by the bus code alone: a known code gains its hint. + const why = typeof e?.code !== 'string' ? undefined + : Object.hasOwn(TEMPLATE_REFUSALS, e.code) ? TEMPLATE_REFUSALS[e.code] : Object.hasOwn(PACK_REFUSALS, e.code) ? PACK_REFUSALS[e.code] : undefined; + if (!why) throw e; + const error = new CliError(`${e.code}: ${why}`, busExit(e.code)); error.code = e.code; throw error; } diff --git a/packages/cli/tests/org-templates.test.mjs b/packages/cli/tests/org-templates.test.mjs index fd4a94f07..8d438eba3 100644 --- a/packages/cli/tests/org-templates.test.mjs +++ b/packages/cli/tests/org-templates.test.mjs @@ -8,6 +8,8 @@ import { join } from 'node:path'; import { main } from '../src/cli.mjs'; import { ORG_USAGE, TEMPLATE_USAGE } from '../src/org.mjs'; import { bootConfig, loadSystem } from '../src/config.mjs'; +import { CliError } from '../src/errors.mjs'; +import { busExit } from '../src/transport.mjs'; import { BusError } from '../../bus/src/broker.mjs'; import { startBroker } from '../../bus/src/runtime.mjs'; import { fixture, io, tmp } from './helpers.mjs'; @@ -24,7 +26,8 @@ async function setup(t) { const deps = { system: { dataRoot: f.dataRoot }, transport: () => async (verb, args = {}) => { calls.push([verb, Object.keys(args).sort()]); try { return structuredClone(await b.broker.request(human, { verb, args })); } - catch (e) { if (!(e instanceof BusError)) throw e; throw Object.assign(new Error(e.code), { code: e.code }); } + // What humanTransport throws for a bus refusal: a CliError named by the code alone. + catch (e) { if (!(e instanceof BusError)) throw e; throw Object.assign(new CliError(e.code, busExit(e.code)), { code: e.code }); } } }; const run = async (args, env = f.env) => { const x = io(env); await main(['org', ...args], x, deps); return JSON.parse(x.out.text); }; const saved = (name) => join(boot.organization.dir, 'templates', f.doc.human, `${name}.json`); diff --git a/packages/organization/src/template-save.mjs b/packages/organization/src/template-save.mjs index 712cfcbd2..4214fa1c9 100644 --- a/packages/organization/src/template-save.mjs +++ b/packages/organization/src/template-save.mjs @@ -19,7 +19,7 @@ // A refusal is a TemplateError: a bus code and a reason that names a path or a checked name only, // never a value from the document and never a host path. -import { closeSync, constants, fchmodSync, fsyncSync, lstatSync, mkdirSync, openSync, readdirSync, writeSync } from "node:fs"; +import { closeSync, constants, fchmodSync, fsyncSync, lstatSync, mkdirSync, openSync, readdirSync, unlinkSync, writeSync } from "node:fs"; import { join } from "node:path"; import { ORG_PREFIX, checkDefinition, readTemplate } from "./definitions.mjs"; import { sha256 } from "./util.mjs"; @@ -122,19 +122,26 @@ function literals(document, { rolesDir, cache }) { for (const project of Object.values(document.projects ?? {})) for (const name of Object.keys(project?.agents ?? {})) agents.add(name); for (const name of agents) if (!role(name)) add(name, "{agent}"); out.sort((a, b) => b.text.length - a.text.length); - return out.map((l) => ({ ...l, pattern: new RegExp(`(? { - counts[placeholder] = (counts[placeholder] ?? 0) + 1; - return placeholder; - }); + if (list.length === 0) return text; + let entry = combined.get(list); + if (!entry) { + const by = new Map(list.map((l) => [l.text, l.placeholder])); + entry = { by, pattern: new RegExp(`(? escape(l.text)).join("|")})(?![${WORD}])`, "g") }; + combined.set(list, entry); } - return out; + return text.replace(entry.pattern, (match) => { + const placeholder = entry.by.get(match); + counts[placeholder] = (counts[placeholder] ?? 0) + 1; + return placeholder; + }); } // ---- Capture ---- @@ -368,7 +375,9 @@ export function templatePreview({ document, name, project, rolesDir, assertClean try { checked = checkPack(template, { origin: "saved", rolesDir }); } catch (error) { - if (error?.name === "PluginError") refuse(/^starter-pack-[a-z-]+$/.test(error.code ?? "") ? error.code : "starter-pack-invalid"); + if (error?.name === "PluginError") { + refuse(/^starter-pack-[a-z-]+$/.test(error.code ?? "") ? error.code : "starter-pack-invalid", typeof error.message === "string" ? error.message.slice(0, 300) : undefined); + } throw error; } const json = `${JSON.stringify(checked, null, 2)}\n`; @@ -429,8 +438,14 @@ export function templateSave({ document, name, project, digest, rolesDir, assert let offset = 0; while (offset < bytes.length) offset += writeSync(fd, bytes, offset); fsyncSync(fd); - } finally { + } catch (error) { + // A partial file would hold the name: remove the file this call created, then report the failure. closeSync(fd); + fd = undefined; + try { unlinkSync(file); } catch { /* already gone */ } + throw error; + } finally { + if (fd !== undefined) closeSync(fd); } return { name, scope: preview.scope, ...(project === undefined ? {} : { project }), digest }; } diff --git a/packages/organization/tests/template-save.test.mjs b/packages/organization/tests/template-save.test.mjs index 6fd82103f..9348c6efa 100644 --- a/packages/organization/tests/template-save.test.mjs +++ b/packages/organization/tests/template-save.test.mjs @@ -113,6 +113,17 @@ test("SP8 placeholders replace the org and project ids, the owner, agent names a assert.deepEqual(at("definitions.writer.instructions"), ["{agent}", "{org}", "{owner}", "{project}", "{root}"]); }); +test("SP8 an instance named root, org or agent never rewrites a placeholder already written in", (t) => { + const { doc, rolesDir } = fixture(t); + doc.roles.root = { definition: "org:writer" }; + doc.roles.org = { definition: "org:writer" }; + doc.definitions.writer.instructions = "Work for acme with root on stack. Ask org, then jason."; + const preview = templatePreview({ document: doc, name: "team-shape", rolesDir, assertClean }); + assert.equal(preview.template.definitions.writer.instructions, "Work for {org} with {agent} on {project}. Ask {agent}, then {owner}."); + const at = preview.replacements.filter((r) => r.path === "definitions.writer.instructions").map((r) => r.placeholder).sort(); + assert.deepEqual(at, ["{agent}", "{org}", "{owner}", "{project}"]); +}); + test("SP8 the preview lists every kept free-text field with its saved text", (t) => { const { doc, rolesDir } = fixture(t); const preview = templatePreview({ document: doc, name: "acme-shape", rolesDir, assertClean }); diff --git a/packages/webui/src/template-http.mjs b/packages/webui/src/template-http.mjs index d31e39e37..d0cca1232 100644 --- a/packages/webui/src/template-http.mjs +++ b/packages/webui/src/template-http.mjs @@ -2,6 +2,9 @@ // template under their own account, and lists the templates they saved. Owner Console transports only; // no caller-supplied business, account, subject or origin reaches the bus. // +// serve.mjs strips /api/orgs/ before the route is matched, so the paths below reach this module as +// /api/templates and /api/templates/preview. +// // GET /api/orgs//templates {templates, writable}: template.list.console, the owner's saved templates // POST /api/orgs//templates/preview {name, project?}: template.save.console with preview true. Writes nothing; // answers the template JSON, every free-text field and the digest