diff --git a/packages/bus/src/broker.mjs b/packages/bus/src/broker.mjs index 239af6921..d131f2a9d 100644 --- a/packages/bus/src/broker.mjs +++ b/packages/bus/src/broker.mjs @@ -96,7 +96,7 @@ const taskRef = (x) => typeof x === 'string' && /^vikunja:[1-9][0-9]*\/[1-9][0-9 // A git commit id, SHA-1 or SHA-256. const oid = (x) => typeof x === 'string' && /^[0-9a-f]{40}([0-9a-f]{24})?$/.test(x); // Row 123 (#1617): the one list of organization-file keys limits.set and organization.set accept. -const ORGANIZATION_KEYS = ['revision', 'limits', 'models', 'instances']; +export const ORGANIZATION_KEYS = ['revision', 'limits', 'models', 'instances']; // Row 123 (#1617): role.end never reaches the assistant (the user's seat) or the caller's own run. const END_PROTECTED = ['assistant']; const VERDICTS = ['approve', 'changes']; diff --git a/packages/business/README.md b/packages/business/README.md index 1d3802b87..f0d2d7097 100644 --- a/packages/business/README.md +++ b/packages/business/README.md @@ -127,7 +127,8 @@ assistant and never talks to the user. It holds `message.send` and `organization.change` (row 123): it ends a run with a reason and no decision (`role.revoke` stays gated for every role), and it changes the organization file through the broker's `organization.set`, limited to the keys the human's -`limits.set` takes. It has no credentials and the same read-only tools. It is the only shipped role that holds `role.launch`, so +`limits.set` takes; the harness's `end_run` and `change_organization` tools +call them (row 132). It has no credentials and the same read-only tools. It is the only shipped role that holds `role.launch`, so it is the only role `launch.by` can name. The PM holds no `role.launch` (REQ-LAUNCH-1); it asks the chief of staff for a session. diff --git a/packages/harness/README.md b/packages/harness/README.md index 4d2d6fb96..f675eb020 100644 --- a/packages/harness/README.md +++ b/packages/harness/README.md @@ -88,6 +88,19 @@ file outside the workspace, which the gate refuses to read (row 89, #1583; tasks, 50 at a time, without descriptions. `get_task` takes a `task_ref` and returns that task in full. `packages/bus/README.md` has the reply. +The chief of staff's two grants from row 123 have tools (row 132, #1628), +so its manifest lists both in `typed` and nothing in `untyped`: + +- `end_run` (role.end holders) sends `role.revoke` with `role` and a + `reason` of 1 to 500 characters, never a `decision`. The broker refuses + the caller's own run and the assistant's with `role-end-refused`. A + `role.end` holder's prompt names the tool in place of the "only the human + stops a run" line; every other role keeps that line. +- `change_organization` (organization.change holders) sends + `organization.set` with `revision` and the keys that verb takes, `limits`, + `models` and `instances`. `ORGANIZATION_KEYS` in `tools.mjs` is the + broker's list; a test fails when the two differ. + The review loop (row 93, #1587) adds four tools: - `push_working` (git.push.working holders with a clone) pushes a local diff --git a/packages/harness/src/bundle.mjs b/packages/harness/src/bundle.mjs index 95dbb1ebe..3dfa70389 100644 --- a/packages/harness/src/bundle.mjs +++ b/packages/harness/src/bundle.mjs @@ -88,11 +88,13 @@ function prompt({ contract, resolved, session, tools, untyped }) { `- Bus tools: ${tools.map((t) => t.name).join(", ")}. The broker decides every call; a refusal names its reason.`, "- Each message to you arrives as one turn. Your final answer goes back to the sender as the reply, so end each turn with the answer itself.", "- You never hold a token. You act on the tracker, the forge and other roles only through the bus tools.", - // Row 90 (#1584): PM fault 10 (#228 c91). - "- Only the human stops a run, with `mosaic stop`. If you think a run should stop, raise a role.revoke decision with blocking false and the role instance as target, naming the run and saying why.", ]; - // Row 93 (#1587): the review loop, for the roles that have its tools. const named = new Set(tools.map((t) => t.name)); + // Row 90 (#1584): PM fault 10 (#228 c91). Row 132 (#1628): a role holding role.end has end_run. + lines.push(named.has("end_run") + ? "- You end another role's run with end_run, naming the role instance and a reason of 1 to 500 characters; the reason is required and goes into the log. You can't end your own run or the assistant's. For anything the broker refuses, raise a role.revoke decision with blocking false and the role instance as target, naming the run and saying why; the human decides, and `mosaic stop` is theirs." + : "- Only the human stops a run, with `mosaic stop`. If you think a run should stop, raise a role.revoke decision with blocking false and the role instance as target, naming the run and saying why."); + // Row 93 (#1587): the review loop, for the roles that have its tools. if (named.has("push_working")) lines.push("- Review loop: commit on a branch of your clone, push_working it, then request_review with the commit it returned. Work that isn't pushed can't be reviewed, and a task with pushed work closes only on an approve verdict for its latest push. After a changes verdict, commit the fixes, push again and ask again."); if (named.has("fetch_review")) diff --git a/packages/harness/src/tools.mjs b/packages/harness/src/tools.mjs index d544befb1..7badb1613 100644 --- a/packages/harness/src/tools.mjs +++ b/packages/harness/src/tools.mjs @@ -22,6 +22,9 @@ const TASK_REF = str("Task reference, as list_tasks shows it"); // doesn't load the broker. const TASK_PAGE = 50; const TASK_PAGE_MAX = 200; +// The keys the broker's `organization.set` accepts (ORGANIZATION_KEYS in packages/bus/src/broker.mjs; +// a test holds the two lists equal). Kept here so the session's tool server doesn't load the broker. +export const ORGANIZATION_KEYS = Object.freeze(["revision", "limits", "models", "instances"]); const COMMON = { task_ref: TASK_REF, decision: str("Id of a resolved decision that approves this call; needed when the action is cross-role or gated for you"), @@ -132,6 +135,22 @@ export function typedTools({ resolved, arbiter = false, tracker = false, project decision: COMMON.decision, }, ["task_ref", "request", "verdict", "body"])); } + // Row 132 (#1628, REQ-ROLE-2, REQ-COS-1, REQ-LIMIT-1): the chief of staff's two grants from row 123. + // end_run sends `reason` and never a decision; the broker refuses the caller's own run and the assistant's. + if (held.has("role.end")) { + add("end_run", "role.revoke", "role.end", "End another role's run. A reason is required, 1 to 500 characters, and goes into the log. You can't end your own run or the assistant's; the broker refuses with role-end-refused.", obj({ + role: str("Role instance whose run to end"), + reason: str("Why the run ends, 1 to 500 characters", { minLength: 1, maxLength: 500 }), + }, ["role", "reason"])); + } + if (held.has("organization.change")) { + add("change_organization", "organization.set", "organization.change", "Change the organization file: the launch limits, the model table, and the model each instance runs. revision is the file's current revision; the broker refuses a stale one with revision-conflict. Send only the keys you change; each replaces what the file holds for it.", obj({ + revision: str("The organization file's current revision", { pattern: "^[a-f0-9]{64}$" }), + limits: { type: "object", description: "The launch limits, replacing the file's launch limits" }, + models: { type: "object", description: "The model table, replacing the file's model table" }, + instances: { type: "object", description: "Role instance to model key, for the instances that change", additionalProperties: { type: "string" } }, + }, ["revision"])); + } add("raise_decision", "decision.raise", null, "Ask for a decision before a gated or cross-role action, or when you need a choice made. The answer arrives as a message.", obj({ action: str("The action the decision is about", { enum: [...ACTIONS] }), question: str("The question, with enough context to answer it and the reason for your recommendation"), diff --git a/packages/harness/tests/bundle.test.mjs b/packages/harness/tests/bundle.test.mjs index 0cd90dd70..64e50ae5d 100644 --- a/packages/harness/tests/bundle.test.mjs +++ b/packages/harness/tests/bundle.test.mjs @@ -76,11 +76,38 @@ test("a chief-of-staff bundle lists launch among the bus tools and the manifest' const b = build(t, r); const prompt = readFileSync(b.files.prompt, "utf8"); assert.match(prompt, /Role instance: chief \(definition chief-of-staff\)\. Run: run-1\./); - assert.match(prompt, /Bus tools: send_message, launch, raise_decision, /); + assert.match(prompt, /Bus tools: send_message, launch, end_run, change_organization, raise_decision, /); assert.ok(b.manifest.typed.includes("launch")); assert.deepEqual(JSON.parse(readFileSync(b.files.policy, "utf8")).needs, []); }); +// Row 132 (#1628, REQ-ROLE-2, REQ-COS-1): the 3d re-pin window checks this manifest: both tools typed, nothing untyped. +test("a chief-of-staff bundle lists end_run and change_organization as typed and nothing untyped, for either harness", (t) => { + for (const harness of ["pi", "claude-code"]) { + const r = resolvedFor(t, "chief", (d) => (d.roles.chief.vars = { harness, model: "claude-opus-5-5" })); + const b = build(t, r); + const prefix = harness === "pi" ? "" : "mcp__mosaic__"; + assert.deepEqual(b.manifest.typed, ["send_message", "launch", "end_run", "change_organization", "raise_decision", "list_agents", "inbox", "trail", "list_tasks", "get_task"].map((n) => prefix + n), harness); + assert.deepEqual(b.manifest.untyped, [], harness); + assert.match(readFileSync(b.files.prompt, "utf8"), /Bus tools: send_message, launch, end_run, change_organization, raise_decision, /); + assert.doesNotMatch(readFileSync(b.files.prompt, "utf8"), /Actions you hold that have no bus tool yet/); + } +}); + +// Row 132 (#1628): a role holding role.end is told about end_run; every other role keeps the human-only text. +test("the stop line names end_run for a role.end holder, and every other role keeps today's text", (t) => { + const line = (b) => readFileSync(b.files.prompt, "utf8").split("\n").filter((l) => /\brole\.revoke\b/.test(l) && /run/.test(l)); + const human = "- Only the human stops a run, with `mosaic stop`. If you think a run should stop, raise a role.revoke decision with blocking false and the role instance as target, naming the run and saying why."; + const chief = line(build(t, resolvedFor(t, "chief", (d) => (d.roles.chief.vars = { harness: "pi", model: "claude-opus-5-5" })))); + assert.equal(chief.length, 1); + assert.notEqual(chief[0], human); + assert.match(chief[0], /^- You end another role's run with end_run, naming the role instance and a reason of 1 to 500 characters; the reason is required/); + assert.match(chief[0], /You can't end your own run or the assistant's\./); + assert.match(chief[0], /For anything the broker refuses, raise a role\.revoke decision with blocking false/); + assert.doesNotMatch(chief[0], /Only the human stops a run/); + for (const instance of ["pm", "coder"]) assert.deepEqual(line(build(t, resolvedFor(t, instance, (d) => (d.roles[instance].vars = { ...d.roles[instance].vars, harness: "pi", model: "claude-opus-5-5" })))), [human], instance); +}); + test("a claude-code bundle adds the wrapped gate hook and the MCP config", (t) => { const r = resolvedFor(t, "coder", (d) => (d.roles.coder.vars = { harness: "claude-code", model: "claude-sonnet-5-5" })); const b = build(t, r); diff --git a/packages/harness/tests/runner.test.mjs b/packages/harness/tests/runner.test.mjs index 784bd674e..4e5da7f17 100644 --- a/packages/harness/tests/runner.test.mjs +++ b/packages/harness/tests/runner.test.mjs @@ -14,7 +14,8 @@ import { Store } from "../../bus/src/store.mjs"; import { Broker, BusError } from "../../bus/src/broker.mjs"; import { serve } from "../../bus/src/server.mjs"; import { EXIT, founderCheck, stateBlock, turnRequest } from "../src/runner.mjs"; -import { scratch } from "./helpers.mjs"; +import { typedTools } from "../src/tools.mjs"; +import { resolvedFor, scratch } from "./helpers.mjs"; const RUNNER = new URL("../src/runner.mjs", import.meta.url).pathname; const FAKE = new URL("./fixtures/fake-adapter.mjs", import.meta.url).pathname; @@ -27,6 +28,9 @@ const businesses = { roles: { pm: { authority: { withinRole: ["message.send", "role.launch"], crossRole: [] } }, coder: { authority: { withinRole: ["message.send"], crossRole: [] } }, + // Row 132 (#1628): the chief of staff's grants and the assistant it can't end. + chief: { definition: "chief-of-staff", authority: { withinRole: ["message.send", "role.end", "organization.change"], crossRole: [] } }, + assistant: { definition: "assistant", authority: { withinRole: ["message.send"], crossRole: [] } }, }, }, }; @@ -85,10 +89,10 @@ test("stateBlock names the launch state with its time and event, and the open ta assert.match(capped, /Open tasks: 9, the first 1 of cap 1\./); }); -async function setup(t, { session = {}, policy = {}, tools } = {}) { +async function setup(t, { session = {}, policy = {}, tools, as = "coder", organization = null } = {}) { const dir = scratch(t); const store = new Store(dir); - const broker = new Broker({ store, businesses }); + const broker = new Broker({ store, businesses, organization }); const brokerSocket = join(dir, "bus.sock"); const server = await serve({ broker, path: brokerSocket }); t.after(async () => { @@ -120,8 +124,8 @@ async function setup(t, { session = {}, policy = {}, tools } = {}) { t.after(() => launchServer.close()); writeFileSync(join(runDir, "session.json"), JSON.stringify({ business: "demo", - instance: "coder", - run: "coder-run", + instance: as, + run: `${as}-run`, launchedBy: "pm", harness: "pi", provider: "fake", @@ -138,7 +142,7 @@ async function setup(t, { session = {}, policy = {}, tools } = {}) { turnTimeout: 20, ...session, })); - const cap = broker.bindLaunch({ business: "demo", role: "coder", run: "coder-run", harness: "pi" }); + const cap = broker.bindLaunch({ business: "demo", role: as, run: `${as}-run`, harness: "pi" }); const pm = broker.bindLaunch({ business: "demo", role: "pm", run: "pm-run", harness: "pi" }); const call = (c, verb, args = {}) => broker.request(c, { verb, args }); call(pm, "role.claim"); @@ -368,6 +372,47 @@ test("typed tools carry the runner's capability; launch goes to the host's launc assert.deepEqual(ctx.launches, [{ cap: ctx.cap, instance: "coder" }, { cap: ctx.cap, instance: "reviewer" }]); }); +// Row 132 (#1628, REQ-ROLE-2, REQ-COS-1, REQ-LIMIT-1): the chief's two tools reach the broker by their verbs +// with the arguments the session gave, and a refusal comes back with the broker's code. +test("end_run and change_organization reach the broker with the tool's arguments, and refusals keep their codes", async (t) => { + const written = []; + const organization = (...args) => { + written.push(args); + if (args[2].revision === "e".repeat(64)) throw Object.assign(new Error("stale"), { code: "revision-conflict" }); + return { revision: "b".repeat(64) }; + }; + const tools = typedTools({ resolved: resolvedFor(t, "chief") }).filter((x) => ["end_run", "change_organization"].includes(x.name)); + assert.deepEqual(tools.map((x) => x.name), ["end_run", "change_organization"]); + const ctx = await setup(t, { as: "chief", organization, tools, policy: { typed: tools.map((x) => x.name) } }); + ctx.inbox = []; + ctx.call(ctx.broker.bindLaunch({ business: "demo", role: "coder", run: "coder-run", harness: "pi" }), "role.claim"); + const r = startRunner(ctx.runDir, ctx.cap); + t.after(() => r.child.kill("SIGKILL")); + const send = (name, args) => ctx.call(ctx.pm, "message.send", { to: "chief", body: `TOOL ${name} ${JSON.stringify(args)}` }); + const change = { revision: "a".repeat(64), limits: { total: 3 } }; + send("end_run", { role: "coder", reason: "stuck on the same test for an hour" }); + send("end_run", { role: "chief", reason: "my own run" }); + send("end_run", { role: "assistant", reason: "the user's seat" }); + send("change_organization", change); + send("change_organization", { ...change, surprise: 1 }); + send("change_organization", { ...change, revision: "e".repeat(64) }); + await until(() => replies(ctx)() && ctx.inbox.length >= 6); + assert.deepEqual(ctx.inbox.map((m) => m.body), [ + '{"revoked":true}', + "refused: role-end-refused", + "refused: role-end-refused", + '{"revision":"' + "b".repeat(64) + '"}', + "refused: invalid-request", + "refused: revision-conflict", + ]); + assert.deepEqual({ ...ctx.store.get("SELECT op, reason, by FROM role_claims WHERE role='coder' ORDER BY rowid DESC LIMIT 1") }, { op: "revoke", reason: "stuck on the same test for an hour", by: "chief-run" }); + // The writer saw the tool's arguments and the chief's name; the unknown key never reached it. + assert.deepEqual(written.map((w) => [w[0], w[1], w[2], w[3]]), [ + ["demo", "limits.set", change, "chief-of-staff.chief"], + ["demo", "limits.set", { ...change, revision: "e".repeat(64) }, "chief-of-staff.chief"], + ]); +}); + test("a RESULT gets no automatic reply; failed turns reply with the reason", async (t) => { const ctx = await setup(t, { session: { turnTimeout: 1 } }); ctx.inbox = []; diff --git a/packages/harness/tests/tools.test.mjs b/packages/harness/tests/tools.test.mjs index 582fd6385..51b5d7f8e 100644 --- a/packages/harness/tests/tools.test.mjs +++ b/packages/harness/tests/tools.test.mjs @@ -1,7 +1,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { callTool, REPLY_CAP, replyText, typedTools, untypedActions } from "../src/tools.mjs"; -import { TASK_PAGE, TASK_PAGE_MAX } from "../../bus/src/broker.mjs"; +import { callTool, ORGANIZATION_KEYS, REPLY_CAP, replyText, typedTools, untypedActions } from "../src/tools.mjs"; +import { ORGANIZATION_KEYS as BROKER_ORGANIZATION_KEYS, TASK_PAGE, TASK_PAGE_MAX } from "../../bus/src/broker.mjs"; import { fakeToolSocket, resolvedFor, scratch } from "./helpers.mjs"; const names = (tools) => tools.map((t) => t.name); @@ -29,17 +29,53 @@ test("the PM gets its task verbs and the reads, and no launch", (t) => { } }); -// Row 119 (#1613): the chief of staff holds launch and message.send; row 123 adds role.end and organization.change (no typed tools). -test("the chief of staff gets launch and send_message, and no task verbs", (t) => { +// Row 119 (#1613): the chief of staff holds launch and message.send; row 123 adds role.end and organization.change, which row 132 gives tools. +test("the chief of staff gets launch, send_message, end_run and change_organization, and no task verbs", (t) => { const chief = resolvedFor(t, "chief"); const tools = typedTools({ resolved: chief, tracker: true }); - assert.deepEqual(names(tools), ["send_message", "launch", "raise_decision", "list_agents", "inbox", "trail", "list_tasks", "get_task"]); + assert.deepEqual(names(tools), ["send_message", "launch", "end_run", "change_organization", "raise_decision", "list_agents", "inbox", "trail", "list_tasks", "get_task"]); const launch = tools.find((x) => x.name === "launch"); assert.equal(launch.verb, "launch"); assert.equal(launch.action, "role.launch"); assert.deepEqual(launch.parameters.properties.instance.enum, ["coder", "reviewer"]); - // Row 123 (#1617, REQ-COS-1): the chief also holds role.end and organization.change on the bus; no typed tool yet. - assert.deepEqual(untypedActions(chief, tools), ["role.end", "organization.change"]); + // Row 132 (#1628): role.end and organization.change have their tools, so nothing the chief holds is untyped. + assert.deepEqual(untypedActions(chief, tools), []); +}); + +// Row 132 (#1628, REQ-ROLE-2, REQ-COS-1, REQ-LIMIT-1): each tool maps to the broker verb and action row 123 added. +test("end_run sends role and reason, no decision; change_organization sends revision and the organization keys", (t) => { + const tools = typedTools({ resolved: resolvedFor(t, "chief"), tracker: true }); + const end = tools.find((x) => x.name === "end_run"); + assert.deepEqual([end.verb, end.action], ["role.revoke", "role.end"]); + assert.deepEqual(Object.keys(end.parameters.properties), ["role", "reason"]); + assert.deepEqual(end.parameters.required, ["role", "reason"]); + assert.deepEqual([end.parameters.properties.reason.minLength, end.parameters.properties.reason.maxLength], [1, 500]); + assert.equal(end.parameters.additionalProperties, false); + assert.match(end.description, /can't end your own run or the assistant's/); + const change = tools.find((x) => x.name === "change_organization"); + assert.deepEqual([change.verb, change.action], ["organization.set", "organization.change"]); + assert.deepEqual(Object.keys(change.parameters.properties), [...ORGANIZATION_KEYS]); + assert.deepEqual(change.parameters.required, ["revision"]); + assert.equal(change.parameters.additionalProperties, false); + assert.equal(change.parameters.properties.revision.pattern, "^[a-f0-9]{64}$"); +}); + +// Row 132 (#1628): the key list is the broker's. +test("the organization keys change_organization takes are the broker's ORGANIZATION_KEYS", () => { + assert.deepEqual([...ORGANIZATION_KEYS], BROKER_ORGANIZATION_KEYS); +}); + +// Row 132 (#1628): the grants are the offer; a role without them gets neither tool. +test("a role without role.end or organization.change gets neither tool, and each tool follows its own grant", (t) => { + for (const instance of ["pm", "coder", "reviewer"]) { + const tools = typedTools({ resolved: resolvedFor(t, instance), arbiter: true, tracker: true }); + assert.deepEqual(names(tools).filter((n) => n === "end_run" || n === "change_organization"), [], instance); + } + const assistant = resolvedFor(t, "assistant", (d) => (d.roles.assistant = { definition: "assistant", holder: "mos" })); + assert.deepEqual(names(typedTools({ resolved: assistant, tracker: true })).filter((n) => n === "end_run" || n === "change_organization"), []); + const only = (grant) => resolvedFor(t, "chief", (d) => (d.roles.chief.vars = { ...d.roles.chief.vars, "limits.authority": ["message.send", grant] })); + assert.deepEqual(names(typedTools({ resolved: only("role.end") })).filter((n) => n === "end_run" || n === "change_organization"), ["end_run"]); + assert.deepEqual(names(typedTools({ resolved: only("organization.change") })).filter((n) => n === "end_run" || n === "change_organization"), ["change_organization"]); }); // Row 119 (#1613): the assistant holds message.send alone.