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.
This commit is contained in:
jason.woltje committed
1 parent
079ced93f3
commit
1ba3288bec
7 files changed
+86
-17
No files matched your search
@@ -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();
|
||||
|
||||
@@ -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));
|
||||
|
||||
@@ -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/);
|
||||
|
||||
@@ -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 => {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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, []);
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in new issue
Block a user