Files
stack/agents/darkwing/work/slice1-s1/r1-to-r2.diff
T
jason.woltjeandClaude Opus 5.5 d9568cec98 docs(slice1): row 36 S1 round 2 packet (darkwing)
Answers Filbert's round 1 (#1518 comment 26724): launch.by must hold
role.launch, launch is null when limits.authority drops it, a test for
cross-role narrowing, and the business file checked on its open
descriptor.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
2026-10-04 22:55:37 -05:00

249 lines
12 KiB
Diff

--- r1/packages/business/README.md
+++ r2/packages/business/README.md
@@ -69,7 +69,9 @@
it to `<configDir>/businesses/mosaic-stack.json`, put in the bot ids and
token dates from `docs/guides/slice-1-identities.md`, fix the token paths
and run `validate`. The file must belong to you and must not be writable
-by group or other.
+by group or other. The loader checks the owner and mode on the descriptor
+it reads from, so the file can't be swapped between the check and the
+read.
Each role instance names a `definition` (a version 2 role file), an
optional `holder`, its Vikunja bot (`tracker`, required when the definition
@@ -87,9 +89,10 @@
`rotateBy`, prints a warning.
`launch` lets one instance (`by`) start the listed instances, up to `max`
-sessions per model family (Opus 4, Sonnet 4 at most). Only that instance
-keeps `role.launch` as within-role. Every other instance loses it, so for
-them it's gated.
+sessions per model family (Opus 4, Sonnet 4 at most). `by` must name an
+instance whose definition holds `role.launch` within-role; otherwise the
+file refuses. Only that instance keeps `role.launch`. Every other instance
+loses it, so for them it's gated.
### Project file
@@ -143,7 +146,10 @@
`authority.crossRole`), `credentials` (references only), `tracker` (`bot`,
`botId` or null), `launch` (the launch block, or null) and `digest`, the
SHA-256 of the record's canonical JSON. `classify` refuses an action
-outside the vocabulary.
+outside the vocabulary. `launch` is set exactly when `classify(record,
+"role.launch")` is `within`. If `limits.authority` leaves out
+`role.launch` for the launcher, `launch` is null. A launcher should
+still ask `classify`, not read `launch` alone.
Errors are `BusinessError` with `exitCode` 2 (invalid) or 4 (a required
file missing). The CLI adds 3 for a system config problem and 4 for usage.
--- r1/packages/business/src/business.mjs
+++ r2/packages/business/src/business.mjs
@@ -3,7 +3,6 @@
// it and never writes it. A missing or invalid file refuses (lead decision
// 46, 6.1).
-import { lstatSync } from "node:fs";
import { dirname, isAbsolute, join, normalize } from "node:path";
import { homedir } from "node:os";
import { refuse } from "./errors.mjs";
@@ -112,12 +111,16 @@
return { roles: out, definitions };
}
-function checkLaunch(launch, roles, file) {
+function checkLaunch(launch, roles, definitions, file) {
const where = `${file} launch`;
requireObject(launch, where);
rejectUnknownKeys(launch, ["by", "instances", "max"], where);
requireId(launch.by, `${where}.by`);
if (!Object.hasOwn(roles, launch.by)) refuse(`${where}.by names ${launch.by}, which the business file doesn't declare`);
+ const launcher = definitions[roles[launch.by].definition];
+ if (!launcher.authority.withinRole.includes("role.launch")) {
+ refuse(`${where}.by names ${launch.by}, whose role ${launcher.name} doesn't hold role.launch within-role`);
+ }
const instances = requireDistinctList(launch.instances, `${where}.instances`, (name) => {
requireId(name, `${where}.instances entry`);
if (!Object.hasOwn(roles, name)) refuse(`${where}.instances names ${name}, which the business file doesn't declare`);
@@ -180,7 +183,7 @@
refuse(`${file} tracker.sync must use its own bot, not the one roles.${instance} uses`);
}
}
- const launch = document.launch === undefined ? null : checkLaunch(document.launch, roles, file);
+ const launch = document.launch === undefined ? null : checkLaunch(document.launch, roles, definitions, file);
return deepFreeze({
businessVersion: 1, id: document.id, file, human, arbiters, projects, vars, tracker, roles, launch, definitions,
@@ -193,9 +196,9 @@
export function loadBusiness(id, { dir = configDir(), rolesDir }) {
if (!rolesDir) throw new Error("loadBusiness needs rolesDir");
const file = businessFilePath(id, dir);
- const document = readJsonFile(file, "business file");
- const stat = lstatSync(file);
- if (stat.uid !== process.getuid()) refuse(`business file must belong to uid ${process.getuid()}: ${file}`);
- if ((stat.mode & 0o022) !== 0) refuse(`business file must not be writable by group or other (mode ${(stat.mode & 0o777).toString(8)}): ${file}`);
+ const document = readJsonFile(file, "business file", (stat) => {
+ if (stat.uid !== process.getuid()) refuse(`business file must belong to uid ${process.getuid()}: ${file}`);
+ if ((stat.mode & 0o022) !== 0) refuse(`business file must not be writable by group or other (mode ${(stat.mode & 0o777).toString(8)}): ${file}`);
+ });
return validateBusinessDocument(document, file, { rolesDir });
}
--- r1/packages/business/src/resolve.mjs
+++ r2/packages/business/src/resolve.mjs
@@ -68,8 +68,10 @@
crossRole = crossRole.filter((a) => vars["limits.authority"].includes(a));
}
// role.launch needs the business file's launch block naming this
- // instance (addendum A section 8). Without it the verb is gated.
- const launch = business.launch && business.launch.by === instance ? business.launch : null;
+ // instance (addendum A section 8), and the verb must survive
+ // limits.authority. Otherwise the verb is gated and `launch` is null,
+ // so the two never disagree.
+ const launch = business.launch && business.launch.by === instance && withinRole.includes("role.launch") ? business.launch : null;
if (!launch) {
withinRole = withinRole.filter((a) => a !== "role.launch");
crossRole = crossRole.filter((a) => a !== "role.launch");
--- r1/packages/business/src/util.mjs
+++ r2/packages/business/src/util.mjs
@@ -1,4 +1,4 @@
-import { lstatSync, readFileSync } from "node:fs";
+import { closeSync, constants, fstatSync, openSync, readFileSync } from "node:fs";
import { createHash } from "node:crypto";
import { refuse } from "./errors.mjs";
import { ID_PATTERN } from "./vocabulary.mjs";
@@ -59,17 +59,29 @@
}
// Read a JSON file that must be a regular file, not a symbolic link.
-// Missing or not a regular file is 4; unparseable is 2.
-export function readJsonFile(file, what) {
- let stat;
+// Missing or not a regular file is 4; unparseable is 2. One descriptor,
+// opened without following a link, serves every check and the read.
+// `checkStat` sees that descriptor's stat before the read, so the file
+// can't be swapped between the check and the read.
+export function readJsonFile(file, what, checkStat) {
+ let fd;
try {
- stat = lstatSync(file);
- } catch {
- refuse(`${what} not found: ${file}`, 4);
+ fd = openSync(file, constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK);
+ } catch (error) {
+ if (error.code === "ENOENT") refuse(`${what} not found: ${file}`, 4);
+ refuse(`${what} must be a regular, non-symbolic-link file: ${file}`, 4);
+ }
+ let text;
+ try {
+ const stat = fstatSync(fd);
+ if (!stat.isFile()) refuse(`${what} must be a regular, non-symbolic-link file: ${file}`, 4);
+ checkStat?.(stat);
+ text = readFileSync(fd, "utf8");
+ } finally {
+ closeSync(fd);
}
- if (!stat.isFile() || stat.isSymbolicLink()) refuse(`${what} must be a regular, non-symbolic-link file: ${file}`, 4);
try {
- return JSON.parse(readFileSync(file, "utf8"));
+ return JSON.parse(text);
} catch (error) {
refuse(`${what} is not valid JSON (${file}): ${error.message}`);
}
--- r1/packages/business/tests/business.test.mjs
+++ r2/packages/business/tests/business.test.mjs
@@ -1,7 +1,9 @@
import { test } from "node:test";
import assert from "node:assert/strict";
-import { chmodSync, lstatSync, readdirSync, readFileSync, symlinkSync, writeFileSync } from "node:fs";
+import { execFileSync, spawnSync } from "node:child_process";
+import { chmodSync, lstatSync, mkdirSync, readdirSync, readFileSync, symlinkSync, writeFileSync } from "node:fs";
import { join } from "node:path";
+import { pathToFileURL } from "node:url";
import { BusinessError, businessFilePath, configDir, loadBusiness, validateBusinessDocument } from "../src/index.mjs";
import { businessDoc, REPO, REPO_ROLES, rolesCopy, tmp, writeJson } from "./helpers.mjs";
@@ -118,6 +120,7 @@
test("launch", () => {
assert.equal(check((d) => { delete d.launch; }).launch, null);
refuses(() => check((d) => { d.launch.by = "ghost"; }), /launch\.by names ghost/);
+ refuses(() => check((d) => { d.launch.by = "cto"; d.launch.instances = ["coder"]; }), /launch\.by names cto, whose role cto doesn't hold role\.launch within-role/);
refuses(() => check((d) => { d.launch.instances = []; }), /must not be empty/);
refuses(() => check((d) => { d.launch.instances = ["coder", "coder"]; }), /duplicate/);
refuses(() => check((d) => { d.launch.instances = ["ghost"]; }), /instances names ghost/);
@@ -152,9 +155,31 @@
writeFileSync(join(dir, "businesses", "broken.json"), "{ not json");
refuses(() => loadBusiness("broken", { dir, rolesDir: REPO_ROLES }), /not valid JSON/);
+ // The owner and mode checks run on the opened file before the read, so
+ // a group-writable file refuses for its mode even when it won't parse.
+ chmodSync(join(dir, "businesses", "broken.json"), 0o660);
+ refuses(() => loadBusiness("broken", { dir, rolesDir: REPO_ROLES }), /writable by group or other \(mode 660\)/);
assert.throws(() => loadBusiness("acme", { dir }), /needs rolesDir/);
});
+// A directory or a FIFO under the business name refuses without a read.
+// Opening a FIFO without O_NONBLOCK would block the whole process, which
+// node:test can't time out, so that case runs in a child with a timeout.
+test("loadBusiness: not a regular file", () => {
+ const dir = join(tmp(), "config");
+ mkdirSync(join(dir, "businesses", "folder.json"), { recursive: true });
+ refuses(() => loadBusiness("folder", { dir, rolesDir: REPO_ROLES }), /must be a regular, non-symbolic-link file/, 4);
+ execFileSync("mkfifo", [join(dir, "businesses", "pipe.json")]);
+ const index = pathToFileURL(join(REPO, "packages", "business", "src", "index.mjs")).href;
+ const child = spawnSync(process.execPath, ["--input-type=module", "-e", `
+ import { loadBusiness } from ${JSON.stringify(index)};
+ try { loadBusiness("pipe", { dir: ${JSON.stringify(dir)}, rolesDir: ${JSON.stringify(REPO_ROLES)} }); }
+ catch (error) { console.log(error.exitCode, error.message); }
+ `], { encoding: "utf8", timeout: 5000 });
+ assert.equal(child.signal, null, "opening the FIFO waited for a writer");
+ assert.match(child.stdout, /^4 .*must be a regular, non-symbolic-link file/);
+});
+
test("loading writes nothing", () => {
const root = tmp();
const dir = join(root, "config");
--- r1/packages/business/tests/resolve.test.mjs
+++ r2/packages/business/tests/resolve.test.mjs
@@ -97,8 +97,39 @@
const noLaunch = resolve(setup((d) => { delete d.launch; }), "pm");
assert.equal(classify(noLaunch, "role.launch"), "gated");
assert.equal(noLaunch.launch, null);
- const otherLauncher = resolve(setup((d) => { d.launch.by = "cto"; d.launch.instances = ["coder"]; }), "pm");
- assert.equal(classify(otherLauncher, "role.launch"), "gated");
+
+ // A second pm instance is the launcher, so the first one may not launch.
+ const two = setup((d) => {
+ d.roles.pm2 = { ...d.roles.pm, holder: undefined, tracker: { bot: "bot-acme-pm2", botId: 7 } };
+ d.launch.by = "pm2";
+ });
+ assert.equal(classify(resolve(two, "pm"), "role.launch"), "gated");
+ assert.equal(resolve(two, "pm").launch, null);
+ assert.equal(classify(resolve(two, "pm2"), "role.launch"), "within");
+ assert.equal(resolve(two, "pm2").launch.by, "pm2");
+});
+
+test("limits.authority without role.launch leaves the launcher with no launch block", () => {
+ for (const layer of ["agent", "project"]) {
+ const authority = ["task.create", "task.assign", "message.send"];
+ const s = layer === "agent"
+ ? setup((d) => { d.roles.pm.vars["limits.authority"] = authority; })
+ : setup(undefined, { roles: { pm: { vars: { "limits.authority": authority } } } });
+ const pm = resolve(s, "pm");
+ assert.equal(classify(pm, "role.launch"), "gated", layer);
+ assert.equal(pm.launch, null, layer);
+ }
+});
+
+test("limits.authority narrows cross-role actions too", () => {
+ const s = setup((d) => {
+ d.roles.coder.vars["limits.authority"] = ["task.update.assigned", "git.push.working", "task.scope.change"];
+ });
+ const coder = resolve(s, "coder");
+ assert.deepEqual(coder.limits.authority, { withinRole: ["task.update.assigned", "git.push.working"], crossRole: ["task.scope.change"] });
+ assert.equal(classify(coder, "task.reassign"), "gated");
+ assert.equal(classify(coder, "task.scope.change"), "cross");
+ assert.equal(classify(coder, "review.request"), "gated");
});
test("classify", () => {