diff --git a/agents/honker/work/queue-194/BUILD.md b/agents/honker/work/queue-194/BUILD.md index d14c8a702..c2693a4ce 100644 --- a/agents/honker/work/queue-194/BUILD.md +++ b/agents/honker/work/queue-194/BUILD.md @@ -791,3 +791,77 @@ What the rebase exposed, fixed on b4: under a temp data root and `testerRequest`, as `tester-socket.test.mjs` and `sso.test.mjs` do. - Drift: c3d88840c..eb017d77f overlaps the manifest only on `docs/plans/DEFERRED.md` (merge-tree clean); the requirements tests changed their own counts, `host-run.test.mjs` took the #453 T27 fix. + +### b6: Webby's b5 review (webby-0094) and Sage's rulings (sage-2241, sage-2245), 2026-10-21T23:50Z + +`honker/row194-b6` is on b5 (dfabbbe8a). It covers B1, N1-N4, and notes for N5-N8. B2 (S6) waits for +row 191's `stopRun`. No test has run on b6 yet. Every run waits for Sage's CLEAR. + +B1, restart on every committed lifecycle verb: +- `runtime.mjs` adds `restart: !live` to the answer of every instance-writer verb: `instances.set`, + `veto.set`, `instance.hire` and `instance.fire` (`INSTANCE_VERBS`). Before, only `instances.set` carried + it. Limits, routes and plugins writes don't go through the instance writer, and they keep their own + answers. +- The CLI's veto output and the Console's veto receipt print the restart line. `/api/veto` treats an answer + without a boolean `restart` as a bad shape (agents.test.mjs, calls 6 to 7). +- The stuck-firing sequence: a keep the bus copy hasn't loaded (a refused reload) passed the broker's + checks, step A stopped the runs, and step B refused `instance-kept`, leaving the instance firing. The + broker now calls `instance.fireCheck` before step A. That runs the writer's `fireChecks` against the + file with no write, so the fire refuses before it stops anything. `fire` runs the same `fireChecks` at + step B. +- Tests: in reload.test, after a refused reload, veto, hire and fire answer restart true. A fire of a + name kept since then refuses `instance-kept` with the file unchanged, the run still held and no + `instance-firing`. After an unkeep, it fires. chief-of-staff T10 covers a fireCheck refusal that stops + nothing. The CLI stub-bus test covers a veto with RESTART_NEEDED. +- Mutants for the gate: (a) put back the `instances.set`-only restart line, which reload.test's new test, + the CLI veto test and the Console tests should kill; (b) drop the `instance.fireCheck` call, which the + stuck-sequence test and T10 should kill. + +N1: a fire of a delisted name now revokes its services, with no write: `delisted: false`, `transaction: +null` and the revoke receipt. A repeat revoke is safe. Test in lifecycle-revoke.test.mjs. + +N2: `createdCredentials` drops HOST_SERVICE, as `checkRoles` does (instance-settings.test.mjs). + +N3: I extended the check, as row 213 did for creative instances. `keptHumanOnly` (launch-settings.mjs) is +called in the limits writer against the file. It refuses a re-point of a `launch.models` entry a kept +instance's model key names, and a change of a kept instance's own key, for every actor but the human (the +tester included, as with `creativeHumanOnly`). That makes a shared-key re-point also refused for agents +whenever any kept instance uses that key; the chief can still add or change keys no kept instance names. +The broker passes `instance-kept` from the writer for every verb. Test: "B7 kept instance" in +launch-settings.test.mjs. + +N4: not small, so it's deferred. B1's file check removes the reachable case Webby named. A step B that +keeps refusing (a revoke that fails every time, or a file changed between the check and the write) +still strands the instance. The DEFERRED.md entry names a human-only verb and its test (sage-2241: never +an agent verb). + +N5 (Console `busStatus` codes and the `unknown-request` polling): follow-up, not in b6. The body is right +and the status is 502. Neither is harmful. + +N6 (CLI terminate signals every registry run of the instance): this is broader than V3's text +(DESIGN.md:118-120), which names the verb's runs plus spawned-but-unbound ones. A run the chief launched +after the verb is ended too, and its claim wasn't revoked at the verb. I haven't changed it in b6, +because the launcher's signal pass is what S6 rewires onto `stopRun`. When S6 lands, either the pass +narrows to the verb's runs plus unbound registry runs, or the design's next revision says terminate ends +every run of the instance at the take. Sage's call. + +N7 (a delete that closes `outcome-unknown` after commit can't be retried; DESIGN.md:173 says a second +delete retries step 3): follow-up. The window is narrow. + +N8: DESIGN.md:625 still lists "SIGKILL escalation" under "Not in the row". That's r2 text, and D2.2 r4 +(the fire stop, T9a, M51-M55) replaced it. The approved design isn't edited. D2.2 governs. Line 128's "no +SIGKILL escalation" is V3 terminate and still holds (TERM only). + +B2, S6 (sage-2241, sage-2245): 194 lands after or with row 191 in the 10-24 window. The per-run stop +belongs to row 191. Rocko and I agreed its shape in +`agents/rocko/work/queue-191/STOP-RUN-CONTRACT.md` (sha256 dd9bead59372cb3394fc15060e87a824c458ee8e0e45a2b519141ba94c965bf8): +- Call: `stopRun({root, business, run, reason, owner})` from `processing-control/src/runtimes.mjs`. +- Answer: `{run, outcome, pid, signals, receipt}`, where outcome is stopped, killed, alive, + identity-changed or unknown. +- Grace: fixed 5 s TERM and 2 s observe. Identity is checked again before each signal. +- The first owner joins, with one immutable receipt. +- It writes nothing to the bus and needs no claim. + +S6 maps killed and alive to `lifecycle.end.killed` and `lifecycle.end.alive`, and V3 terminate stays TERM +only. It wires onto Rocko's implementation commit (`rocko/row191-stoprun-v1`) when that arrives. S6 +doesn't use the root-wide halt sweep. diff --git a/docs/plans/DEFERRED.md b/docs/plans/DEFERRED.md index 41752848f..09c674ba4 100644 --- a/docs/plans/DEFERRED.md +++ b/docs/plans/DEFERRED.md @@ -384,6 +384,22 @@ at every gate. Started 2026-09-12 during the control board MVP. (instances.set launch: false) but neither fire nor delete removes a creative entry (sage-2166). The row covers fire and delete together; Sage files it. (2026-10-21) +- **A fire stuck between step A and step B has no human escape.** (Webby + N4 on row 194 b5, webby-0094; Sage's ruling sage-2241, #1692.) The + broker's chief fire records `fire.stopped` at step A, and `#firing` + refuses every launch and take of the instance until a `fire.done`. The + events are in the store, so a bus restart doesn't clear them. Row 194 + b6 runs the writer's checks against the file before step A + (`instance.fireCheck`), so a keep the bus hasn't loaded no longer + strands an instance. A step B that keeps refusing still does: a + credential revoke that fails every time, or a file a human changed + between the check and the write. Each repeat fire runs step A again + and refuses at B. The fix is a human verb, never an agent one, that + records the end of the firing (a `fire.done`, or a new + `fire.abandoned` that `#firing` also counts) with a reason. Its test: + a fire whose step B refuses leaves the instance `instance-firing`, the + verb as the chief or the tester refuses, and the verb as the human + clears it so the instance launches again. (2026-10-21) ## Queue diff --git a/packages/bus/src/broker.mjs b/packages/bus/src/broker.mjs index f3d66d1b2..41c89d48a 100644 --- a/packages/bus/src/broker.mjs +++ b/packages/bus/src/broker.mjs @@ -2591,6 +2591,9 @@ export class Broker { // The last listed name refuses before step A stops anything, as the writer would at step B. const listed = b.launch?.instances ?? []; if (listed.length === 1 && listed[0] === args.name) fail('invalid-request'); + // webby-0094 B1: the same checks against the file. A keep this copy hasn't loaded (a reload the + // credentials refused) would otherwise pass here, and step B's refusal would leave I firing. + write('instance.fireCheck'); // Step A (webby-0011 B2): the store side first, so a delisted agent never keeps a live process. // V3's revoke, cancel and end, with kill: true for the launcher's escalation. const stopped = this.#store.transaction(() => { @@ -2655,7 +2658,9 @@ export class Broker { if (e.code?.startsWith("organization-")) fail(e.code); if (e.code === "invalid-plugins" || e.code === "unknown-plugin") fail(e.code); // Row 194: the writer's own copy of the hire and fire checks, against the file as it stands. - if ((verb === "instance.hire" || verb === "instance.fire") && ["definition-not-hireable", "instance-kept"].includes(e.code)) fail(e.code); + if ((verb === "instance.hire" || verb === "instance.fire") && e.code === "definition-not-hireable") fail(e.code); + // Row 194 (B7, webby-0094 N3): the limits writer's keep check, against the file, as well. + if (e.code === "instance-kept") fail(e.code); // Row 194 (brief item 8): a fire's or a delete's revoke failed, and nothing was written. if ((verb === "instance.fire" || verb === "instances.set") && e.code === "credential-revoke-failed") fail(e.code); // Row 65: the route checks every write of an instance's route vars runs. diff --git a/packages/bus/src/runtime.mjs b/packages/bus/src/runtime.mjs index 1f3c55ddd..9600eb28c 100644 --- a/packages/bus/src/runtime.mjs +++ b/packages/bus/src/runtime.mjs @@ -28,6 +28,9 @@ import { inspectInterpreter, recordInterpreterStart } from './interpreter.mjs'; import { createPeerReader } from './peercred.mjs'; import { createUsage } from './usage.mjs'; import { createLimits } from './limits.mjs'; + +// Row 194 (D3): the verbs the instance writer commits, whose answers carry restart. +const INSTANCE_VERBS = new Set(['instances.set', 'veto.set', 'instance.hire', 'instance.fire']); // Trusted host API. S1 supplies resolved definitions, S6 supplies launch records and ends them. // Human limits writes use the shared organization writer behind the verified transport. // `tasks` is S3's adapter factory: ({broker, credentials, businesses}) => {handle, timeout, close}. @@ -265,6 +268,8 @@ export async function startBroker({ if(verb==='routes')return settings.routes(id); // Row 194 (#1692, V5): a delete's checks at the verb, with no write and no reload. if(verb==='instances.check')return instances.checkDelete(id,args.instance); + // Row 194 b6 (webby-0094 B1): a fire's checks against the file, before step A. + if(verb==='instance.fireCheck')return instances.checkFire(id,args.name); let result; if(verb==='limits.set')result=withTokens(id,settings.set(id,args,actor)); else if(verb==='instances.set')result=instances.set(id,args,actor); @@ -279,7 +284,10 @@ export async function startBroker({ // Row 194 (D3 item 2): one reload, after every verb that committed, then the limit checks. const live=reload(id); limits.refresh(id).catch(e=>limitLog({event:'limit-refresh-failed',business:id,message:e.message})); - return verb==='instances.set'?{...result,restart:!live}:result; + // Row 194 b6 (webby-0094 B1, sage-2241): every instance-writer verb says whether the reload + // took, so a refused one (credentials, roots, ...) shows the restart line for a keep or a + // hire too. The other writers' answers carry no restart field. + return INSTANCE_VERBS.has(verb)?{...result,restart:!live}:result; }:null, // Row 194 (D2.3): the role files as they stand, for the hire checks. roleFile: organization ? (name) => loadRole(organization.rolesDir, name) : null, diff --git a/packages/bus/tests/chief-of-staff.test.mjs b/packages/bus/tests/chief-of-staff.test.mjs index a5dc3c988..64123b221 100644 --- a/packages/bus/tests/chief-of-staff.test.mjs +++ b/packages/bus/tests/chief-of-staff.test.mjs @@ -777,7 +777,7 @@ test('row 194 T11: on a bus with two businesses, the chief of one hires and fire assert.equal(organization.length, 0); b.request(chief, { verb: 'instance.hire', args: HIRE }); b.request(chief, { verb: 'instance.fire', args: FIRE }); - assert.deepEqual(organization.map((c) => [c[0], c[1]]), [['demo', 'instance.hire'], ['demo', 'instance.fire']]); + assert.deepEqual(organization.map((c) => [c[0], c[1]]), [['demo', 'instance.hire'], ['demo', 'instance.fireCheck'], ['demo', 'instance.fire']]); assert.equal(store.get("SELECT count(*) AS n FROM events WHERE business='other'").n, 0); }); @@ -821,7 +821,7 @@ test('row 194 T8 (sage-1471): a chief whose authority lacks role.hire or role.fi // The other action still goes through. const [other, otherArgs] = missing === 'role.hire' ? ['instance.fire', FIRE] : ['instance.hire', HIRE]; call('chief', other, otherArgs); - assert.equal(organization.length, 1, missing); + assert.equal(organization.filter(([, verb]) => verb !== 'instance.fireCheck').length, 1, missing); } }); @@ -909,9 +909,9 @@ test('row 194 T9: fire refusals come before step A and leave the claim, the requ test('row 194 T9c: a writer failure at B leaves the instance firing, and the repeat delists and logs once', (t) => { const { call, b, caps, organization, allowed, writer } = hireSetup(t); - writer.error = new Error('disk'); + writer.during = ([, verb]) => { if (verb === 'instance.fire') throw new Error('disk'); }; assert.throws(() => call('chief', 'instance.fire', FIRE), /outcome-unknown/); - writer.error = null; + writer.during = null; assert.equal(allowed('fire.stopped').length, 1); assert.equal(allowed('role.fire').length, 0); // W-N1: while it fires, no launch of the instance starts by any path. @@ -931,7 +931,7 @@ test('row 194 T9c: a writer failure at B leaves the instance firing, and the rep test('row 194 T9c: a failure at C leaves the fire unlogged and the instance firing, and the repeat logs it', (t) => { const { call, organization, allowed, store, writer } = hireSetup(t); // The delist commits, then the step C transaction aborts. - writer.during = () => store.run("CREATE TRIGGER fail_c BEFORE INSERT ON events WHEN json_extract(new.body,'$.action')='fire.done' BEGIN SELECT RAISE(ABORT, 'c failed'); END"); + writer.during = ([, verb]) => verb === 'instance.fire' && store.run("CREATE TRIGGER fail_c BEFORE INSERT ON events WHEN json_extract(new.body,'$.action')='fire.done' BEGIN SELECT RAISE(ABORT, 'c failed'); END"); assert.throws(() => call('chief', 'instance.fire', FIRE)); writer.during = null; store.run('DROP TRIGGER fail_c'); @@ -967,6 +967,19 @@ test('row 194 T10: a hire or fire of a kept name refuses instance-kept before th assert.equal(allowed('role.hire').length + allowed('role.fire').length, 0); }); +// webby-0094 B1: the file's keep, which this copy hasn't loaded, refuses at the fire check, before +// step A revokes, ends or marks anything, so the instance isn't left firing. +test('row 194 T10: a fire the writer\'s check refuses from the file stops nothing and leaves the instance launchable', (t) => { + const { call, organization, lastClaim, allowed, writer } = hireSetup(t); + writer.during = ([, verb]) => { if (verb === 'instance.fireCheck') throw Object.assign(new Error('instance-kept'), { code: 'instance-kept' }); }; + assert.throws(() => call('chief', 'instance.fire', FIRE), /instance-kept/); + writer.during = null; + assert.deepEqual(organization.map(([, verb]) => verb), ['instance.fireCheck']); + assert.equal(lastClaim('coder').op, 'claim'); + assert.equal(allowed('fire.stopped').length + allowed('lifecycle.end').length + allowed('role.fire').length, 0); + assert.ok(call('human', 'instance.launch', { instance: 'coder' }).request); +}); + test('row 194 T18: hire and fire take only their own keys, and a refused call writes nothing', (t) => { const { call, organization, allowed } = hireSetup(t); for (const extra of [{ keep: ['x'] }, { launch: {} }, { revision: 'a'.repeat(64) }, { force: true }]) { diff --git a/packages/bus/tests/lifecycle-revoke.test.mjs b/packages/bus/tests/lifecycle-revoke.test.mjs index 8b8c7ccec..7f20a3802 100644 --- a/packages/bus/tests/lifecycle-revoke.test.mjs +++ b/packages/bus/tests/lifecycle-revoke.test.mjs @@ -91,6 +91,25 @@ test('T20 fire: a revoke that fails stops the fire with the file unchanged; one assert.deepEqual(readdirSync(f.agentDir('coder')), []); }); +// webby-0094 N1: a fire of an instance already delisted (here a hand edit) still revokes its tokens, +// with no write, and the instance is no longer firing. +test('T20 fire: an instance already delisted still has its tokens revoked and the file is left alone', async (t) => { + const f = await setup(t); + f.ask('credentials.issue', { agent: 'rev-2', service: 'gitea', token: TOKEN, date: '2099-01-01' }); + assert.ok(existsSync(join(f.agentDir('rev-2'), 'gitea'))); + const doc = JSON.parse(f.bytes()); + doc.launch.instances = doc.launch.instances.filter((name) => name !== 'rev-2'); + writeFileSync(f.file, JSON.stringify(doc, null, 2) + '\n', { mode: 0o600 }); + const before = f.bytes(); + const fired = f.fire('rev-2'); + assert.equal(fired.delisted, false); + assert.equal(fired.transaction, null); + assert.deepEqual(fired.credentials, { revoked: ['gitea', 'vikunja'], outsideStore: [] }); + assert.equal(f.bytes(), before); + assert.deepEqual(readdirSync(f.agentDir('rev-2')), []); + assert.deepEqual(f.status('rev-2'), { gitea: 'revoked', vikunja: 'revoked' }); +}); + test('T20 fire: with no entry in the running bus, a token on disk refuses and an empty store goes on', async (t) => { const f = await setup(t, { drop: ['rev-2'] }); f.plant('rev-2', 'gitea'); diff --git a/packages/bus/tests/reload.test.mjs b/packages/bus/tests/reload.test.mjs index e7f04196a..091b37b26 100644 --- a/packages/bus/tests/reload.test.mjs +++ b/packages/bus/tests/reload.test.mjs @@ -57,7 +57,8 @@ test('T14 a create goes live with no restart: the new instance binds at once and test('T14 every committed verb reloads once, limits.set, routes.set, plugins.set and veto.set included, and a refused one does not', async (t) => { const f = await setup(t); - f.ask('limits.set', { revision: f.revision(), limits: { tokens: { instances: { coder: { limit: 100 } } } } }); + // Only the instance writer's verbs carry restart (webby-0094 B1); the other writers' answers don't. + assert.equal(Object.hasOwn(f.ask('limits.set', { revision: f.revision(), limits: { tokens: { instances: { coder: { limit: 100 } } } } }), 'restart'), false); assert.equal(f.reloads.length, 1); assert.equal(f.reloads[0].launch.limits.tokens.instances.coder.limit, 100); f.ask('routes.set', { revision: f.revision(), routes: { coder: { harness: 'claude-code', model: 'claude-sonnet-5-5', thinking: 'low', fallback: null } } }); @@ -66,7 +67,7 @@ test('T14 every committed verb reloads once, limits.set, routes.set, plugins.set f.ask('plugins.set', { revision: f.revision(), plugins: { harness: ['claude-code', 'pi'] } }); assert.equal(f.reloads.length, 3); assert.deepEqual(f.reloads[2].plugins.harness, ['claude-code', 'pi']); - f.ask('veto.set', { revision: f.revision(), op: 'keep', name: 'coder' }); + assert.equal(f.ask('veto.set', { revision: f.revision(), op: 'keep', name: 'coder' }).restart, false); assert.equal(f.reloads.length, 4); assert.deepEqual(f.reloads[3].launch.keep, ['coder']); assert.equal(code(() => f.ask('instances.set', { revision: f.revision(), instance: 'ghost', op: 'change', vars: {} })), 'organization-unknown-instance'); @@ -142,6 +143,44 @@ test('T19 a credentialed create answers restart true; issue works only after a r assert.equal(reloads[0].roles['rev-2'].vars.model, 'claude-sonnet-5-5'); }); +// Row 194 b6 (webby-0094 B1, sage-2241): a credentialed create leaves every later reload refused, +// so each instance-writer verb after it answers restart true: the keep, the chief's hire and fire. +// Webby's sequence: a keep the running copy never loaded still refuses the chief's fire before step +// A, from the file, so the kept instance keeps its claim and isn't left firing. +test('T14 after a refused reload veto.set, instance.hire and instance.fire answer restart true, and a fire of a name kept since refuses before step A', async (t) => { + const f = await setup(t); + const roleFile = join(f.rolesDir, 'chief-of-staff.json'); + writeFileSync(roleFile, JSON.stringify({ ...JSON.parse(readFileSync(roleFile, 'utf8')), hireable: ['chief-information-security-officer'] }, null, 2)); + mkdirSync(join(f.root, 'secrets', 'agents'), { recursive: true, mode: 0o700 }); + const chief = bind(f.bus, 'chief'); + f.bus.broker.request(chief, { verb: 'role.claim' }); + const asChief = (verb, args) => f.bus.broker.request(chief, { verb, args }); + const reviewer = bind(f.bus, 'reviewer'); + f.bus.broker.request(reviewer, { verb: 'role.claim' }); + const run = f.ask('instances').instances.reviewer.running; + assert.equal(run.length, 1); + + const added = f.ask('instances.set', { revision: f.revision(), instance: 'rev-2', op: 'add', definition: 'reviewer', tracker: { bot: 'bot-demo-rev-2', botId: 40 } }); + assert.equal(added.restart, true); + const hired = asChief('instance.hire', { name: 'sec-2', definition: 'chief-information-security-officer', reason: 'a security lead' }); + assert.equal(hired.restart, true); + const kept = f.ask('veto.set', { revision: f.revision(), op: 'keep', name: 'reviewer' }); + assert.equal(kept.restart, true); + assert.equal(f.reloads.length, 0, 'the running copy still has reviewer unkept'); + + const before = readFileSync(f.file, 'utf8'); + assert.equal(code(() => asChief('instance.fire', { name: 'reviewer', reason: 'the work is done' })), 'instance-kept'); + assert.equal(readFileSync(f.file, 'utf8'), before); + assert.deepEqual(f.ask('instances').instances.reviewer.running, run, 'step A never ran: the claim is held'); + assert.notEqual(code(() => f.ask('instance.launch', { instance: 'reviewer' })), 'instance-firing'); + + const unkept = f.ask('veto.set', { revision: f.revision(), op: 'unkeep', name: 'reviewer' }); + assert.equal(unkept.restart, true); + const fired = asChief('instance.fire', { name: 'reviewer', reason: 'the work is done' }); + assert.equal(fired.restart, true); + assert.equal(JSON.parse(readFileSync(f.file, 'utf8')).launch.instances.includes('reviewer'), false); +}); + test('T14 a file whose project root differs from the boot copy is not reloaded, because the Credentials store protects the boot roots', async (t) => { const f = await setup(t); const doc = JSON.parse(readFileSync(f.file, 'utf8')); diff --git a/packages/business/src/instance-settings.mjs b/packages/business/src/instance-settings.mjs index 49a0dabb6..14356282c 100644 --- a/packages/business/src/instance-settings.mjs +++ b/packages/business/src/instance-settings.mjs @@ -14,7 +14,7 @@ import { resolveInstance } from "./resolve.mjs"; import { loadRole } from "./role.mjs"; import { refuse } from "./errors.mjs"; import { isPlainObject, canonicalJson } from "./util.mjs"; -import { CREATIVE_DEFINITIONS, END_PROTECTED, ID_PATTERN } from "./vocabulary.mjs"; +import { CREATIVE_DEFINITIONS, END_PROTECTED, HOST_SERVICE, ID_PATTERN } from "./vocabulary.mjs"; const SET_KEYS = ["revision", "instance", "op", "definition", "vars", "launch", "tracker"]; const VETO_KEYS = ["revision", "op", "name"]; @@ -97,7 +97,8 @@ function definitionFor(rolesDir, name, { credentials = false } = {}) { // needs a bus restart, then row 199's issue. A vikunja definition needs args.tracker, an existing // Vikunja bot the human names. The add makes no Vikunja account. Any other definition refuses it. function createdCredentials(old, instance, definition, args) { - const services = definition.credentials.map(c => c.service); + // webby-0094 N2: a host grant is no token reference (business.mjs checkRoles drops it too). + const services = definition.credentials.map(c => c.service).filter(service => service !== HOST_SERVICE); const out = {}; if (services.includes("vikunja")) { if (!Object.hasOwn(args, "tracker")) { @@ -326,26 +327,44 @@ export function instanceSettings({ dir, rolesDir, system, levels = () => null, r // fire after a failed step C goes on to consume (webby-0011 B2). The checks repeat against the file. function fire(business, args, { actor, context }) { const { document: old, revision } = writer.read(business); - if (!old.launch) throw new OrganizationError("organization-launch-not-configured"); const { name } = args; - if (!id(name) || !Object.hasOwn(old.roles, name)) throw new OrganizationError("organization-unknown-instance"); - if ((old.launch.keep ?? []).includes(name)) throw new OrganizationError("instance-kept"); - if (END_PROTECTED.includes(old.roles[name].definition)) throw new OrganizationError("organization-definition-refused"); - if (old.launch.by === name) throw new OrganizationError("organization-instance-referenced", `${name} is launch.by`); - if (!old.launch.instances.includes(name)) return { ...read(business), transaction: null, delisted: false }; - if (old.launch.instances.length === 1) { - refuse(`${name} is the last name in launch.instances; a business needs one instance the chief may launch`); + fireChecks(old, name); + // Item 8: fire revokes each service of the entry and keeps the store directory (sage-1593). + const services = Object.keys(old.roles[name].credentials ?? {}); + // webby-0094 N1: an instance already delisted (a hand edit, or a repeat after step B) still has + // its tokens revoked, with no write. A revoke is safe to repeat. + if (!old.launch.instances.includes(name)) { + const revoked = revoke ? revoke(business, name, "fire", services) : null; + return { ...read(business), transaction: null, delisted: false, ...(revoked ? { credentials: revoked } : {}) }; } const next = structuredClone(old); next.launch.instances = old.launch.instances.filter(entry => entry !== name); - // Item 8: fire revokes each service of the entry and keeps the store directory (sage-1593). - const services = Object.keys(old.roles[name].credentials ?? {}); let revoked = null; const beforeWrite = revoke ? () => { revoked = revoke(business, name, "fire", services); } : null; const result = commit(business, old, next, { revision, actor, context, beforeWrite }); return { ...result, delisted: true, ...(revoked ? { credentials: revoked } : {}) }; } + // Row 194 b6 (webby-0094 B1): the broker runs fire's checks against the file before step A stops + // anything, so a keep its own copy hasn't loaded (a refused reload) refuses there and not at + // step B, which would leave the instance firing. No write. + function checkFire(business, name) { + const { document, revision } = writer.read(business); + fireChecks(document, name); + return { revision }; + } + + function fireChecks(old, name) { + if (!old.launch) throw new OrganizationError("organization-launch-not-configured"); + if (!id(name) || !Object.hasOwn(old.roles, name)) throw new OrganizationError("organization-unknown-instance"); + if ((old.launch.keep ?? []).includes(name)) throw new OrganizationError("instance-kept"); + if (END_PROTECTED.includes(old.roles[name].definition)) throw new OrganizationError("organization-definition-refused"); + if (old.launch.by === name) throw new OrganizationError("organization-instance-referenced", `${name} is launch.by`); + if (old.launch.instances.includes(name) && old.launch.instances.length === 1) { + refuse(`${name} is the last name in launch.instances; a business needs one instance the chief may launch`); + } + } + // The checks every write runs, then beforeWrite (a fire's or a delete's revoke), then the write. A // check that refuses revokes nothing, and a revoke that throws writes nothing. function commit(business, old, next, { revision, actor, context, beforeWrite = null }) { @@ -386,5 +405,5 @@ export function instanceSettings({ dir, rolesDir, system, levels = () => null, r return { ...read(business), transaction: receipt.transaction, restart: false }; } - return { writer, read, set, checkDelete, veto, hire, fire }; + return { writer, read, set, checkDelete, checkFire, veto, hire, fire }; } diff --git a/packages/business/src/launch-settings.mjs b/packages/business/src/launch-settings.mjs index ac1baa183..2d1c46ed1 100644 --- a/packages/business/src/launch-settings.mjs +++ b/packages/business/src/launch-settings.mjs @@ -16,6 +16,16 @@ export function creativeHumanOnly(document, actor, instance, definition) { } } +// Row 194 (D2.4 B7, webby-0094 N3): only the human changes a kept instance's model, through the +// instance's own key or by re-pointing the launch.models entry that key names (the chief and the +// tester are refused, as row 213 refuses them a creative instance's). The check reads the file, so a +// keep the broker's copy hasn't loaded holds too. +function keptHumanOnly(document, actor, instance) { + if (actor !== document.human && (document.launch?.keep ?? []).includes(instance)) { + throw new OrganizationError("instance-kept", `${instance} is kept; only the human changes its model`); + } +} + // Row 155 (#1649, Jason's ruling Q169): the fields of an assistant instance's entry the human and // the chief of staff may change. holder and credentials stay hand edits. const ASSISTANT_FIELDS = ["vars", "tracker", "definition"]; @@ -184,7 +194,10 @@ export function launchSettings({ dir, rolesDir, system, sessions = () => [], now if (typeof key !== "string") continue; const was = own(old.document.launch.models, key) ? canonicalJson(old.document.launch.models[key]) : null; const is = own(args.models, key) ? canonicalJson(args.models[key]) : null; - if (was !== is) creativeHumanOnly(old.document, actor, instance, entry.definition); + if (was !== is) { + creativeHumanOnly(old.document, actor, instance, entry.definition); + keptHumanOnly(old.document, actor, instance); + } } } next.launch.models = args.models; @@ -199,6 +212,7 @@ export function launchSettings({ dir, rolesDir, system, sessions = () => [], now if (CREATIVE_DEFINITIONS.includes(next.roles[instance].definition)) { throw new OrganizationError("organization-creative-instance", `${instance}'s model changes through instances.set or routes.set`); } + keptHumanOnly(old.document, actor, instance); next.roles[instance].vars = { ...next.roles[instance].vars, model: key }; } } diff --git a/packages/business/tests/instance-settings.test.mjs b/packages/business/tests/instance-settings.test.mjs index 27f9a5ebb..ad6a93d6b 100644 --- a/packages/business/tests/instance-settings.test.mjs +++ b/packages/business/tests/instance-settings.test.mjs @@ -180,6 +180,20 @@ test("T19 add of a credentialed definition writes store references dated today, assert.equal(f.settings.read("acme").definitions.reviewer.accepted, false); }); +// webby-0094 N2: a definition that also declares the host service gets store references for its +// token services only. host is a grant, not a reference, and an "undefined" date key refused the add. +test("T19 add of a definition that declares host writes references for its token services only", () => { + const f = fixture(); + const file = join(f.rolesDir, "reviewer.json"); + const role = JSON.parse(readFileSync(file, "utf8")); + role.credentials.push({ service: "host" }); + writeFileSync(file, JSON.stringify(role)); + const today = new Date().toISOString().slice(0, 10); + f.settings.set("acme", { revision: f.settings.read("acme").revision, instance: "reviewer-2", op: "add", definition: "reviewer", tracker: { bot: "bot-acme-reviewer-2", botId: 40 } }, "jason"); + assert.deepEqual(JSON.parse(readFileSync(f.file, "utf8")).roles["reviewer-2"].credentials, + { gitea: { store: true, rotateBy: today }, vikunja: { store: true, expires: today } }); +}); + test("row 165 refuses a definition change to or from one that declares credentials", () => { const f = fixture(doc => { doc.roles["sec-1"] = { definition: "security-reviewer" }; }); refuses(f, { instance: "sec-1", op: "change", definition: "coder" }, "organization-credentials-out-of-scope"); diff --git a/packages/business/tests/launch-settings.test.mjs b/packages/business/tests/launch-settings.test.mjs index 7a8b88fd4..92813fdb4 100644 --- a/packages/business/tests/launch-settings.test.mjs +++ b/packages/business/tests/launch-settings.test.mjs @@ -142,6 +142,27 @@ test('row 65 a file with harness vars and no plugins.harness loads and its views const moved=settings.set('acme',{revision:view.revision,instances:{coder:'opus'}},'jason');assert.equal(moved.instances.coder.modelKey,'opus'); assert.throws(()=>settings.setRoutes('acme',{revision:moved.revision,routes:{coder:{harness:'pi',model:'opus',thinking:'low',fallback:null}}},'jason'),code('harness-off')); }); +// Row 194 (D2.4 B7, webby-0094 N3): a kept instance's model is the human's, through its own key in +// instances or a models key it names, checked against the file. A key no kept instance names stays +// the chief's. +test('B7 kept instance: the models keys it names and the instances key are the human\'s',async()=>{ + const f=fixture();const doc=structuredClone(f.doc);doc.launch.keep=['coder'];writeJson(f.file,doc); + const g={...f,doc};const {settings}=await routeSettings(g);const file=()=>readFileSync(f.file,'utf8'); + const chief='chief-of-staff.chief';let {revision}=settings.read('acme'); + const models=structuredClone(settings.read('acme').models); + const key=doc.roles.coder.vars.model; + const moved={...models,[key]:{provider:'proxy',model:'claude-sonnet-5-5'}}; + for(const actor of [chief,'tester']){ + const bytes=file(); + assert.throws(()=>settings.set('acme',{revision,models:moved},actor),code('instance-kept'),actor); + assert.throws(()=>settings.set('acme',{revision,instances:{coder:'opus'}},actor),code('instance-kept'),actor); + assert.equal(file(),bytes,actor); + } + ({revision}=settings.set('acme',{revision,models:{...models,spare:{provider:'proxy',model:'claude-opus-5-5'}}},chief)); + ({revision}=settings.set('acme',{revision,models:{...settings.read('acme').models,[key]:moved[key]}},'jason')); + settings.set('acme',{revision,instances:{coder:'opus'}},'jason'); + assert.equal(JSON.parse(file()).roles.coder.vars.model,'opus'); +}); // Row 213 (C2-08, #1714, M10): a creative instance is the human's. The chief of staff (and the tester) // can't reach its model through the models table, the instances key or a route; the human can, // except through the instances key, which refuses a creative instance for everyone. diff --git a/packages/cli/src/instances.mjs b/packages/cli/src/instances.mjs index a7a006595..32d7abd39 100644 --- a/packages/cli/src/instances.mjs +++ b/packages/cli/src/instances.mjs @@ -232,7 +232,8 @@ export async function instancesCommand(args, io, callFor) { if (lifecycle) { const out = await call(lifecycle.verb, lifecycle.args); if (sub === "veto") { - return io.stdout.write(`business ${out.business}: veto ${lifecycle.args.op} ${lifecycle.args.name}\nrevision ${out.revision}\ntransaction ${out.transaction}\n`); + // Row 194 b6 (webby-0094 B1): a veto says whether the reload took, as instances.set does. + return io.stdout.write(`business ${out.business}: veto ${lifecycle.args.op} ${lifecycle.args.name}\nrevision ${out.revision}\ntransaction ${out.transaction}\n${out.restart ? RESTART_NEEDED : NO_RESTART}\n`); } const op = parsed.positional[1]; if (op === "terminate") return io.stdout.write(terminateLines(business, lifecycle.args.instance, out)); diff --git a/packages/cli/tests/instances.test.mjs b/packages/cli/tests/instances.test.mjs index 2798661be..7db02cb88 100644 --- a/packages/cli/tests/instances.test.mjs +++ b/packages/cli/tests/instances.test.mjs @@ -102,6 +102,10 @@ test("row 194 CLI: a bus whose boot credential references differ from the file a assert.doesNotMatch(RESTART_NEEDED, /Mos|Jason/, "product text names no seat (sage-0931)"); assert.doesNotMatch(added.out, /--restart|run `mosaic bus/, "the CLI names the restart and doesn't offer to run it"); assert.throws(() => s.bus.broker.bindLaunch({ business: "acme", role: "sec-1", run: "run-1", harness: "pi" }), { code: "unknown-role" }); + // Row 194 b6 (webby-0094 B1): a veto on the same bus prints the restart line too. + const kept = await s.mosaic(["veto", "acme", "keep", "sec-1", "--revision", await s.revision()]); + assert.equal(kept.code, 0, kept.err); + assert.equal(kept.out.split("\n").at(-2), RESTART_NEEDED); }); test("row 165 CLI: instance set passes the vars file and --no-launch, and a vars-only change needs no restart", async (t) => { @@ -185,7 +189,8 @@ test("row 194 CLI V6: veto keep, unkeep, nohire and hire write through veto.set, assert.equal(kept.code, 0, kept.err); assert.deepEqual(s.calls.at(-1), { business: "acme", verb: "veto.set", args: { revision: s.calls.at(-1).args.revision, op: "keep", name: "coder" } }); const entry = s.log().at(-1); - assert.deepEqual(kept.out.split("\n"), ["business acme: veto keep coder", `revision ${await s.revision()}`, `transaction ${entry.transaction}`, ""]); + assert.deepEqual(kept.out.split("\n"), ["business acme: veto keep coder", `revision ${await s.revision()}`, `transaction ${entry.transaction}`, + "no restart: the launcher reads the change at its next launch check", ""]); assert.equal(entry.actor, "jason"); const nohire = await s.mosaic(["veto", "acme", "nohire", "merge-gate", "--revision", await s.revision()]); assert.equal(nohire.code, 0, nohire.err); diff --git a/packages/webui/src/public/agents.js b/packages/webui/src/public/agents.js index 756666aaa..f463e02c7 100644 --- a/packages/webui/src/public/agents.js +++ b/packages/webui/src/public/agents.js @@ -85,7 +85,8 @@ if (answer === null) return; const { ok, result } = answer; if (!ok) { receipt = refused(result); redraw(); return; } - if (url === '/api/veto') { receipt = `${{ keep: 'Kept', unkeep: 'Unkept', nohire: 'No hire for', hire: 'Hiring allowed for' }[action]} ${name}.`; reread(); return; } + // Row 194 b6 (webby-0094 B1): a veto says whether the reload took, as a create does. + if (url === '/api/veto') { receipt = `${{ keep: 'Kept', unkeep: 'Unkept', nohire: 'No hire for', hire: 'Hiring allowed for' }[action]} ${name}. ${result.restart ? RESTART_TEXT : NO_RESTART_TEXT}`; reread(); return; } if (!REQUEST_ID.test(result?.request ?? '')) { receipt = UNKNOWN; redraw(); return; } if (action === 'end') delete reasons[name]; pending = { request: result.request, action, instance: name, state: 'open', tries: 0 }; diff --git a/packages/webui/src/serve.mjs b/packages/webui/src/serve.mjs index 8a491ae7e..6a9a6ca24 100644 --- a/packages/webui/src/serve.mjs +++ b/packages/webui/src/serve.mjs @@ -424,7 +424,7 @@ const AGENT_ROUTES = { '/api/agent/terminate': { kind: 'terminate', verb: 'instance.end', keys: ['instance', 'reason'], tester: true, done: r => REQUEST_ID.test(r?.request ?? '') && ids(r.runs) && ids(r.cancelled) && ids(r.taken) }, '/api/veto': { kind: 'veto', verb: 'veto.set', keys: ['revision', 'op', 'name'], tester: false, - done: r => /^[a-f0-9]{64}$/.test(r?.revision ?? '') && r.instances && typeof r.instances === 'object' && !Array.isArray(r.instances) }, + done: r => /^[a-f0-9]{64}$/.test(r?.revision ?? '') && typeof r.restart === 'boolean' && r.instances && typeof r.instances === 'object' && !Array.isArray(r.instances) }, }; async function agentSend(req, res, { route, agentWrite, preview, log }) { let target = null; diff --git a/packages/webui/tests/agents-browser.test.mjs b/packages/webui/tests/agents-browser.test.mjs index 61aed40e2..c6bb10012 100644 --- a/packages/webui/tests/agents-browser.test.mjs +++ b/packages/webui/tests/agents-browser.test.mjs @@ -110,7 +110,7 @@ test('row 194 T16 Settings Agents: unavailable read-only, launch follows its req assert.equal(await b.evaluate('reads.length'),before); await b.evaluate(`replies.push([200,{...${JSON.stringify(view)},transaction:'t-2',restart:false}]);true`); await click('[data-agent-action="keep"][data-agent-name="coder"]'); - await wait('document.querySelector("#agents-receipt")?.textContent==="Kept coder."'); + await wait('document.querySelector("#agents-receipt")?.textContent==="Kept coder. No restart: the launcher reads the change at its next launch check."'); assert.deepEqual(await b.evaluate('posts.at(-1)'),['/api/veto',{revision:REV,op:'keep',name:'coder'}]); // An unknown outcome says so and is not retried. await b.evaluate(`replies.push([503,{error:'outcome-unknown',sent:'unknown'}]);true`); diff --git a/packages/webui/tests/agents.test.mjs b/packages/webui/tests/agents.test.mjs index ca7cd87bf..8e90989f8 100644 --- a/packages/webui/tests/agents.test.mjs +++ b/packages/webui/tests/agents.test.mjs @@ -141,12 +141,14 @@ test('row 194 T16: a bus refusal names its reason and nothing changed; an unknow ['/api/agent/launch', { instance: 'coder' }, { request: 'not-an-id' }], ['/api/agent/terminate', { instance: 'coder' }, { request: ID, runs: 'r-1', cancelled: [], taken: [] }], ['/api/veto', { revision: REV, op: 'nohire', name: 'coder' }, { revision: REV }], + // Row 194 b6 (webby-0094 B1): a veto answer without restart can't say whether the reload took. + ['/api/veto', { revision: REV, op: 'nohire', name: 'coder' }, { ...read(), revision: REV, transaction: 't-1' }], ]) { answer = () => bad; const r = await send(s, path, body); assert.deepEqual([r.status, r.body.error, r.body.sent], [503, 'outcome-unknown', 'unknown'], path); } - assert.equal(s.calls.length, 6, 'one call per POST, nothing retried'); + assert.equal(s.calls.length, 7, 'one call per POST, nothing retried'); }); test('row 194 T16: the request read takes one id, adds the refusal reason and refuses an unexpected shape', async t => {