From 1ba3288bec540d4ff18e0974617e88c4e5d47939 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Thu, 12 Nov 2026 17:03:56 -0600 Subject: [PATCH] fix(packs): SP4 review round, --as collisions, step roles, the board route and repeat applies (#1873) - An --as target that lands on another pack item's name refuses invalid-request (definitions and workflows); the preview no longer shows two adds for one path. - A workflow step role naming one of the pack's definitions follows its --as rename, so an inline definition renamed at preview applies. - GET /api/board stays the Board read; only POST /api/board is the pack board write. - Tombstones key on pack id, origin and project: a saved template reusing a shipped id is another pack. - An apply that adds nothing and proposes what the last apply proposed writes nothing, so retries do not fill the 256 packs entries. - mosaic org pack list refuses inside an agent run like the other words. - Tests pin coder-1's credential lines, skills.notEnabled and the member's unsuffixed pack.apply and board.set refusals; the CONSOLE_VERBS pins in two bus tests name the five pack verbs. --- packages/bus/src/runtime.mjs | 28 ++++++++--- packages/bus/tests/console-decide.test.mjs | 4 +- packages/bus/tests/packs.test.mjs | 55 +++++++++++++++++++-- packages/bus/tests/request-console.test.mjs | 4 +- packages/cli/src/org.mjs | 4 +- packages/cli/tests/org-packs.test.mjs | 4 +- packages/webui/src/serve.mjs | 4 +- 7 files changed, 86 insertions(+), 17 deletions(-) diff --git a/packages/bus/src/runtime.mjs b/packages/bus/src/runtime.mjs index bc1dab5fa..c81035198 100644 --- a/packages/bus/src/runtime.mjs +++ b/packages/bus/src/runtime.mjs @@ -345,9 +345,11 @@ function packOps({ writer, rolesDir, dataRoot, system, registryFor, assertClean, if (!chief) return null; return { ...chief, ...resolveInstance({ system, business: loaded, instance: doc.launch.by }).limits }; } - // Each earlier packs entry of this id (and, for a project pack, this project) lists what it added; - // an item absent now that one of them added was removed on purpose, so it stays removed. - const tombstones = (doc, id, project) => new Set((doc.packs ?? []).filter(e => e.id === id && (e.project ?? null) === project).flatMap(e => e.added ?? [])); + // Each earlier packs entry of this id and origin (and, for a project pack, this project) lists what + // it added; an item absent now that one of them added was removed on purpose, so it stays removed. + // A saved template may reuse a shipped id, so the origin is part of the key. + const sameApply = (e, id, origin, project) => e.id === id && e.origin === origin && (e.project ?? null) === project; + const tombstones = (doc, id, origin, project) => new Set((doc.packs ?? []).filter(e => sameApply(e, id, origin, project)).flatMap(e => e.added ?? [])); async function build(id, args, actor, context) { const { document: doc, revision } = writer.read(id); const found = await loadPack(args.pack, actor); @@ -363,8 +365,14 @@ function packOps({ writer, rolesDir, dataRoot, system, registryFor, assertClean, const as = renames(args.as); for (const name of as.definitions.keys()) if (!Object.hasOwn(pack.definitions, name)) bad(); for (const name of as.workflows.keys()) if (!Object.hasOwn(pack.workflows, name)) bad(); + // Two pack items of one kind never land on one name: a rename onto another item's own name + // would show both as add while the later one replaced the earlier in the plan. + for (const kind of ['definitions', 'workflows']) { + const targets = Object.keys(pack[kind]).map(name => as[kind].get(name) ?? name); + if (new Set(targets).size !== targets.length) bad(); + } scan(pack); - const gone = tombstones(doc, pack.id, project); + const gone = tombstones(doc, pack.id, origin, project); const scope = project === null ? doc : doc.projects[project]; const prefix = project === null ? '' : `projects.${project}.`; const chief = chiefOf(doc, `${id}.json`); @@ -408,7 +416,9 @@ function packOps({ writer, rolesDir, dataRoot, system, registryFor, assertClean, for (const [name, value] of Object.entries(pack.workflows)) { const target = as.workflows.get(name) ?? name; // The step ids are the pack's own; the organization's workflow keeps title, state and roles. - const workflow = { description: value.description, steps: value.steps.map(s => ({ title: s.title, state: s.state, roles: [...s.roles] })) }; + // A role naming one of this pack's definitions follows that definition's --as rename. + const role = r => Object.hasOwn(pack.definitions, r) ? as.definitions.get(r) ?? r : r; + const workflow = { description: value.description, steps: value.steps.map(s => ({ title: s.title, state: s.state, roles: s.roles.map(role) })) }; const path = `${prefix}workflows.${target}`; const current = scope.workflows?.[target]; let mark; @@ -458,7 +468,13 @@ function packOps({ writer, rolesDir, dataRoot, system, registryFor, assertClean, if (typeof args.digest !== 'string') bad(); const preview = await build(id, args, actor, context); if (preview.digest !== args.digest) throw new BusError('pack-plan-stale'); - return { ...write(id, preview.plan, preview.revision, actor, {}), pack: preview.pack.id }; + // An apply that adds nothing and proposes what the last apply of this pack proposed writes + // nothing, so a retry loop does not fill the packs list to its cap. The digest pins the revision. + const { plan } = preview; + const last = (writer.read(id).document.packs ?? []).filter(e => sameApply(e, plan.packs.id, plan.packs.origin, plan.packs.project)).at(-1); + if (Object.keys(plan).every(k => k === 'packs') && last && canonicalJson(last.proposedHires ?? []) === canonicalJson(plan.packs.proposedHires)) + return { revision: preview.revision, added: [], pack: preview.pack.id }; + return { ...write(id, plan, preview.revision, actor, {}), pack: preview.pack.id }; }, 'board.set': (id, args, actor) => { if (args.board !== null && !isObject(args.board)) bad(); diff --git a/packages/bus/tests/console-decide.test.mjs b/packages/bus/tests/console-decide.test.mjs index ded008212..edc179812 100644 --- a/packages/bus/tests/console-decide.test.mjs +++ b/packages/bus/tests/console-decide.test.mjs @@ -90,7 +90,9 @@ function setup(t) { // no unsuffixed verb is ever a console verb. test('N4: owner Console verbs have an explicit immutable whitelist and exclude decision.resolve', () => { 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']); + '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']); 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/packs.test.mjs b/packages/bus/tests/packs.test.mjs index 7ab8a2240..afe37667e 100644 --- a/packages/bus/tests/packs.test.mjs +++ b/packages/bus/tests/packs.test.mjs @@ -78,7 +78,7 @@ const variant = (id, patch = () => {}) => { const p = structuredClone(codingPack const marks = (preview) => Object.fromEntries(preview.items.map(i => [i.path, i.mark])); test('SP4 preview writes nothing; apply writes the add items and one packs entry, raises no decision, and a second apply is all same', async t => { - const { call, file, path, decisions } = await harness(t); + const { call, file, path, decisions, root, rolesDir } = await harness(t); const before = readFileSync(path); const decided = JSON.stringify(decisions()); const preview = await call('pack.preview.console', { pack: 'coding' }); @@ -89,6 +89,16 @@ test('SP4 preview writes nothing; apply writes the add items and one packs entry // The chief is known, so every proposed hire lists its flagged and credential lines. assert.equal(preview.chief, true); for (const h of preview.hires) { assert.ok(Array.isArray(h.flagged)); assert.ok(Array.isArray(h.credentialLines)); } + // coder-1's credential lines name the services the coder template reaches. + const coderServices = JSON.parse(readFileSync(join(rolesDir, 'coder.json'), 'utf8')).credentials.map(c => c.service).sort(); + assert.ok(coderServices.length > 0); + const lines = preview.hires.find(h => h.name === 'coder-1').credentialLines; + assert.ok(lines.length >= coderServices.length); + assert.deepEqual([...new Set(lines.map(l => l.service))].sort(), [...new Set(coderServices)]); + // On an empty data root every pack skill is not enabled; an enabled one drops out. + assert.deepEqual(preview.skills.notEnabled, codingPack().skills); + mkdirSync(join(root, 'data', 'skills-enabled', 'ms-sdlc'), { recursive: true }); + assert.deepEqual((await call('pack.preview.console', { pack: 'coding' })).skills.notEnabled, codingPack().skills.filter(n => n !== 'ms-sdlc')); assert.match(preview.digest, /^[0-9a-f]{64}$/); const out = await call('pack.apply.console', { pack: 'coding', digest: preview.digest }); @@ -113,6 +123,14 @@ test('SP4 preview writes nothing; apply writes the add items and one packs entry await call('pack.apply.console', { pack: 'coding', digest: again.digest }); assert.deepEqual(file().definitions, after.definitions); assert.deepEqual(file().workflows, after.workflows); + // An apply that adds nothing and proposes what the last apply proposed writes nothing at all. + const settled = readFileSync(path); + const third = await call('pack.preview.console', { pack: 'coding' }); + const out3 = await call('pack.apply.console', { pack: 'coding', digest: third.digest }); + assert.deepEqual(out3.added, []); + assert.equal(out3.revision, third.revision); + assert.deepEqual(readFileSync(path), settled); + assert.equal(file().packs.length, 2); }); test('SP4 a stale digest refuses pack-plan-stale and writes nothing', async t => { @@ -126,7 +144,7 @@ test('SP4 a stale digest refuses pack-plan-stale and writes nothing', async t => }); test('SP4 an edited definition is kept with its diff; a removed item stays removed; another pack naming it adds it', async t => { - const { call, human, file, revision, install } = await harness(t); + const { call, human, file, revision, install, save } = await harness(t); const first = await call('pack.preview.console', { pack: 'coding' }); await call('pack.apply.console', { pack: 'coding', digest: first.digest }); @@ -165,6 +183,12 @@ test('SP4 an edited definition is kept with its diff; a removed item stays remov assert.equal(o.board, 'add'); assert.equal(o['workflows.change'], 'add'); assert.equal(o['definitions.coder'], 'kept'); + // A saved template reusing the shipped id is another pack: the shipped apply's removals don't bind it. + save('jason', 'mine', variant('coding')); + const mine = await call('pack.preview.console', { pack: 'saved:mine' }); + assert.equal(mine.pack.origin, 'saved'); + assert.equal(marks(mine)['definitions.reviewer'], 'add'); + assert.equal(marks(mine).board, 'add'); }); test('SP4 --as renames a definition or workflow and pins the hire to the new name; a bad rename refuses', async t => { @@ -178,10 +202,32 @@ test('SP4 --as renames a definition or workflow and pins the hire to the new nam assert.ok(file().definitions.builder); assert.equal(file().definitions.coder, undefined); assert.ok(file().workflows.ship); - for (const as of [['definitions.nobody=x'], ['definitions.coder=a', 'definitions.coder=b'], ['definitions.coder=x', 'definitions.pm=x'], ['roles.coder=x'], 'definitions.coder=x']) + // A rename onto another pack item's own name would show both as add while one replaced the other. + for (const as of [['definitions.nobody=x'], ['definitions.coder=a', 'definitions.coder=b'], ['definitions.coder=x', 'definitions.pm=x'], ['roles.coder=x'], 'definitions.coder=x', ['definitions.coder=reviewer']]) await assert.rejects(call('pack.preview.console', { pack: 'coding', as }), /invalid-request/); }); +test('SP4 --as onto another workflow of the pack refuses; a renamed inline definition renames the workflow roles naming it', async t => { + const { call, file, install, rolesDir } = await harness(t); + install(variant('twoflow', p => { p.workflows.other = structuredClone(p.workflows.change); })); + await assert.rejects(call('pack.preview.console', { pack: 'twoflow', as: ['workflows.change=other'] }), /invalid-request/); + const { entryFromTemplate, readTemplate } = await import('../../organization/src/definitions.mjs'); + const { rev: _rev, ...scribe } = entryFromTemplate(readTemplate(rolesDir, 'coder')); + install(variant('scribekit', p => { + p.definitions = { scribe }; + p.hires = []; + p.workflows = { notes: { description: 'Notes.', steps: [{ id: 'write', title: 'Write it', state: 'todo', roles: ['scribe', 'human'] }] } }; + })); + const as = ['definitions.scribe=writer']; + const preview = await call('pack.preview.console', { pack: 'scribekit', as }); + assert.equal(marks(preview)['definitions.writer'], 'add'); + assert.deepEqual(preview.plan.workflows.notes.steps[0].roles, ['writer', 'human']); + await call('pack.apply.console', { pack: 'scribekit', as, digest: preview.digest }); + assert.ok(file().definitions.writer); + assert.equal(file().definitions.scribe, undefined); + assert.deepEqual(file().workflows.notes.steps[0].roles, ['writer', 'human']); +}); + test('SP4 an instance by the hire name drops the proposal; launch.noHire marks it', async t => { const { call } = await harness(t, { mutate: doc => { doc.roles['coder-1'] = { ...structuredClone(doc.roles.reviewer), definition: 'coder', tracker: { bot: 'bot-demo-coder-1', botId: 7 } }; @@ -229,7 +275,8 @@ test('SP4 who may call: the owner only; the tester, a member, an agent and a con assert.throws(() => b.request(tester, { verb: 'workflow.set', args: { revision: revision(), name: 'x', workflow: null } }), /tester-refused/); assert.throws(() => b.request(tester, { verb: 'pack.preview', args: { pack: 'coding' } }), /tester-refused/); const member = b.bindHireConsole({ business: 'demo', actor: 'alice', subject: { provider: 'test', iss: 'test', sub: 'alice' }, rights: ['org.hire', 'board.read'] }); - for (const [verb, args] of [['pack.apply.console', { pack: 'coding', digest: 'a'.repeat(64) }], ['board.set.console', { revision: revision(), board: null }], ['workflow.set.console', { revision: revision(), name: 'x', workflow: null }], ['packs.console', {}]]) + for (const [verb, args] of [['pack.apply.console', { pack: 'coding', digest: 'a'.repeat(64) }], ['board.set.console', { revision: revision(), board: null }], ['workflow.set.console', { revision: revision(), name: 'x', workflow: null }], ['packs.console', {}], + ['pack.apply', { pack: 'coding', digest: 'a'.repeat(64) }], ['board.set', { revision: revision(), board: null }]]) assert.throws(() => b.request(member, { verb, args }), /human-required/); const agent = b.bindLaunch({ business: 'demo', role: 'coder', run: 'coder-run', harness: 'pi', address: 'coder-run' }); assert.throws(() => b.request(agent, { verb: 'pack.apply', args: { pack: 'coding', digest: 'a'.repeat(64) } }), /human-required/); diff --git a/packages/bus/tests/request-console.test.mjs b/packages/bus/tests/request-console.test.mjs index a0d4928ce..178e0bff5 100644 --- a/packages/bus/tests/request-console.test.mjs +++ b/packages/bus/tests/request-console.test.mjs @@ -91,7 +91,9 @@ const ask = (x, state, extra = {}) => x.call('pm', 'request.question', { request test('the console verb list includes the artifact read and existing owner controls', () => { 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']); + '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']); }); test('only the signed-in owner capability reaches the console verbs and it reaches nothing else', async t => { diff --git a/packages/cli/src/org.mjs b/packages/cli/src/org.mjs index 67c04d6fe..8f92c6d6b 100644 --- a/packages/cli/src/org.mjs +++ b/packages/cli/src/org.mjs @@ -76,8 +76,8 @@ async function packsCommand(args, io, deps) { const allowed = (...keys) => { for (const k of [...Object.keys(values), ...(as.length ? ['--as'] : [])]) if (!keys.includes(k)) throw new CliError(`unexpected ${k}`, 4); }; const project = values['--project'] === undefined ? {} : { project: values['--project'] }; const [word, sub, id, name, file] = pos; - // Every word but pack list is a bus call as the human: an agent run refuses before a file is read. - if (!(word === 'pack' && sub === 'list')) refuseInsideAgent(io.env); + // Every word is the human's: an agent run refuses before a file is read, a pack is loaded or the bus is called. + refuseAgentRun(io); let verb, request, revised = false; if (word === 'pack' && sub === 'list' && pos.length === 2) { allowed(); verb = 'packs'; request = {}; } else if (word === 'pack' && ['preview', 'apply'].includes(sub) && pos.length === 4) { diff --git a/packages/cli/tests/org-packs.test.mjs b/packages/cli/tests/org-packs.test.mjs index 866dfe854..bd309fd14 100644 --- a/packages/cli/tests/org-packs.test.mjs +++ b/packages/cli/tests/org-packs.test.mjs @@ -85,13 +85,13 @@ test('SP4 mosaic org workflow remove takes out only that workflow; board set cha await assert.rejects(s.run(['workflow', 'rename', 'acme', 'two']), (e) => e.exitCode === 4 && e.message === PACKS_USAGE); }); -test('SP4 an agent shell refuses every pack, board and workflow write before it reads a file or calls the bus', async (t) => { +test('SP4 an agent shell refuses every pack, board and workflow word, pack list too, before it reads a file or calls the bus', async (t) => { const s = await setup(t); const agent = { ...s.f.env, MOSAIC_RUN_ID: 'run-1' }; const spec = join(s.root, 'flow.json'); writeFileSync(spec, JSON.stringify(FLOW('one'))); const before = readFileSync(s.file); - for (const args of [['pack', 'preview', 'acme', 'coding'], ['pack', 'apply', 'acme', 'coding', '--digest', 'a'.repeat(64)], ['board', 'set', 'acme', spec], ['workflow', 'set', 'acme', 'one', spec], ['workflow', 'remove', 'acme', 'one']]) { + for (const args of [['pack', 'list'], ['pack', 'preview', 'acme', 'coding'], ['pack', 'apply', 'acme', 'coding', '--digest', 'a'.repeat(64)], ['board', 'set', 'acme', spec], ['workflow', 'set', 'acme', 'one', spec], ['workflow', 'remove', 'acme', 'one']]) { await assert.rejects(s.run(args, agent), (e) => e.exitCode === 3 && /human shell/.test(e.message), args.join(' ')); } assert.deepEqual(s.calls, []); diff --git a/packages/webui/src/serve.mjs b/packages/webui/src/serve.mjs index bb70dd86d..bb23c7e9e 100644 --- a/packages/webui/src/serve.mjs +++ b/packages/webui/src/serve.mjs @@ -933,7 +933,9 @@ export async function startServer({ host = '127.0.0.1', port = 7330, board = DEF // alone stays open on a loopback Host for the status and release gates (Q-C). if (sso !== null && !named && !(path === '/healthz' && req.method === 'GET')) return json(res, 403, { error: 'public Host required' }); // No CORS. JSON content type and same-origin checks keep browser forms out. - const decisionPath = DECISION_ROUTE.test(path), requestPath = REQUEST_ROUTE.test(path), remotePath = PROJECT_REMOTE_ROUTE.test(path), systemTypesPath = SYSTEM_TYPES_ROUTE.test(path), autonomyPath = AUTONOMY_ROUTE.test(path), packsPath = PACKS_ROUTE.test(path); + const decisionPath = DECISION_ROUTE.test(path), requestPath = REQUEST_ROUTE.test(path), remotePath = PROJECT_REMOTE_ROUTE.test(path), systemTypesPath = SYSTEM_TYPES_ROUTE.test(path), autonomyPath = AUTONOMY_ROUTE.test(path), + // GET /api/board stays the Board read; only POST /api/board is the pack route's board write. + packsPath = PACKS_ROUTE.test(path) && (path !== '/api/board' || req.method === 'POST'); if (req.headers.origin && req.headers.origin !== (named ? (sso ? (localOrigin ?? sso.publicOrigin) : `https://${preview.host}`) : `http://${req.headers.host}`)) return decisionPath ? decisionRefuse(res, 'cross-origin') : requestPath ? requestRefuse(res, 'cross-origin') : json(res, 403, { error: 'cross-origin request refused' }); let signer = preview, ssoSession = null;