test(organization): pin route refusal variable names (#1873)

packages/organization/src/agent-levels.mjs

@@ -41,34 +41,6 @@ function noBoundKeys(entry, at) {
   }
 }
 
-// A var value and every leaf it holds, as a var check's message quotes them (JSON.stringify).
-function quoted(value, out = []) {
-  if (Array.isArray(value)) value.forEach((v) => quoted(v, out));
-  else if (isPlainObject(value)) Object.values(value).forEach((v) => quoted(v, out));
-  if (value !== undefined) out.push(JSON.stringify(value));
-  return out;
-}
-
-// checkVars, but a refusal never carries a value. Var checks quote the value they refuse, a value
-// can be a secret pasted in the wrong place, and these refusals reach the Console. Each key is
-// rechecked alone to name the one at fault; a message that quotes its value is replaced by one
-// that names the key only. A rule across keys (the fallback set) names keys only already.
-function checkVarsNamingKeys(vars, layer, where) {
-  try { return checkVars(vars, layer, where); }
-  catch (error) {
-    if (error?.name !== "BusinessError" || !isPlainObject(vars)) throw error;
-    for (const [key, value] of Object.entries(vars)) {
-      try { checkVars({ [key]: value }, layer, where); }
-      catch (one) {
-        if (one?.name !== "BusinessError") throw one;
-        if (quoted(value).some((text) => one.message.includes(text))) refuse(`${where}: variable ${key} has a value it can't take (the value is not shown)`);
-        throw one;
-      }
-    }
-    throw error;
-  }
-}
-
 // The root file's document. `where` names it in messages, never with a host path.
 export function checkRootAgents(document, { rolesDir, where = "root agents" } = {}) {
   requireObject(document, where);
@@ -94,7 +66,7 @@ export function checkRootAgents(document, { rolesDir, where = "root agents" } =
     agents[name] = {
       definition: entry.definition,
       title: definition.title,
-      vars: checkVarsNamingKeys(entry.vars ?? {}, "agent", `${at}.vars`),
+      vars: checkVars(entry.vars ?? {}, "agent", `${at}.vars`),
       excludeOrgs: entry.excludeOrgs === undefined ? [] : requireDistinctList(entry.excludeOrgs, `${at}.excludeOrgs`, (id) => requireId(id, `${at}.excludeOrgs entry`)),
     };
   }
@@ -149,7 +121,7 @@ export function checkProjectAgents(agents, { rolesDir, where }) {
     if (Object.hasOwn(entry, "definition")) {
       const definition = definitionOf(entry.definition, at, rolesDir, cache);
       if (entry.definition === CHIEF || entry.definition === ASSISTANT) refuse(`${at}: ${entry.definition} can't be a project agent`);
-      out[name] = { form: "define", definition: entry.definition, title: definition.title, vars: checkVarsNamingKeys(entry.vars ?? {}, "agent", `${at}.vars`) };
+      out[name] = { form: "define", definition: entry.definition, title: definition.title, vars: checkVars(entry.vars ?? {}, "agent", `${at}.vars`) };
       continue;
     }
     if (!isPlainObject(entry.vars) || Object.keys(entry.vars).length === 0) refuse(`${at}: an entry defines (definition), narrows (vars) or excludes (exclude)`);
@@ -157,7 +129,7 @@ export function checkProjectAgents(agents, { rolesDir, where }) {
     for (const key of Object.keys(entry.vars)) {
       if (!INTERSECT.includes(key)) refuse(`${at}.vars: a project can only narrow ${INTERSECT.join(", ")}, not ${JSON.stringify(key)}`);
     }
-    out[name] = { form: "narrow", vars: checkVarsNamingKeys(entry.vars, "project", `${at}.vars`) };
+    out[name] = { form: "narrow", vars: checkVars(entry.vars, "project", `${at}.vars`) };
   }
   return deepFreeze(out);
 }

packages/organization/src/launch-settings.mjs

@@ -128,7 +128,7 @@ export function checkRoutes({ proposed, old = null, levels, catalogue = null })
     for (const [harness, model, thinking] of ROUTES) {
       // A missing model refuses no-route at launch, not here.
       if (proposed.launch && Object.hasOwn(vars, model) && !Object.hasOwn(proposed.launch.models, vars[model])) {
-        throw new OrganizationError("unknown-model", `${instance} ${model} ${JSON.stringify(vars[model] ?? null)} isn't a launch.models key`);
+        throw new OrganizationError("unknown-model", `${instance} variable ${model} isn't a launch.models key`);
       }
       const currentEntry = proposed.launch?.models?.[vars[model]];
       const previousEntry = old?.launch?.models?.[before[model]];
@@ -149,12 +149,12 @@ export function checkRoutes({ proposed, old = null, levels, catalogue = null })
       }
       if (!Object.hasOwn(vars, harness) || (vars[harness] === before[harness] && vars[thinking] === before[thinking])) continue;
       const name = vars[harness];
-      if (!on.includes(name)) throw new OrganizationError("harness-off", `${instance} ${harness} ${name} isn't switched on in plugins.harness`);
+      if (!on.includes(name)) throw new OrganizationError("harness-off", `${instance} variable ${harness} isn't switched on in plugins.harness`);
       if (vars[thinking] === undefined) continue;
       const applies = levels(name);
-      if (!Array.isArray(applies)) throw new OrganizationError("harness-refused", `harness module ${name} didn't load, so its thinking levels are unknown`);
+      if (!Array.isArray(applies)) throw new OrganizationError("harness-refused", `${instance} variable ${harness}: its harness module didn't load, so its thinking levels are unknown`);
       if (!applies.includes(vars[thinking])) {
-        throw new OrganizationError("thinking-unsupported", `harness ${name} doesn't apply thinking ${vars[thinking]} (${instance} ${thinking}); it applies ${applies.join(", ")}`);
+        throw new OrganizationError("thinking-unsupported", `${instance} variable ${thinking}: its harness doesn't apply this thinking level; it applies ${applies.join(", ")}`);
       }
     }
   }
@@ -374,7 +374,7 @@ export function launchSettings({ dir, rolesDir, system, sessions = () => [], now
         const entry = offered ? { provider: offered.provider, model: offered.model, module: offered.module, upstream: offered.upstream } : null;
         if (!existing && !offered && catalogue?.reason === 'model-key-collision') throw new OrganizationError('model-key-collision');
         if (!existing && !offered && catalogue?.status === 'unavailable') throw new OrganizationError('model-catalogue-unavailable');
-        if (!existing && !offered) throw new OrganizationError('unknown-model', `${instance} model isn't a launch.models key`);
+        if (!existing && !offered) throw new OrganizationError('unknown-model', `${instance} variable ${prefix}model isn't a launch.models key`);
         if (existing && offered && canonicalJson(existing) !== canonicalJson(entry) && !unchanged) throw new OrganizationError('model-key-collision');
         if (!existing || existing.module) {
           if (unchanged) continue;

packages/organization/src/organization-config.mjs

@@ -205,7 +205,7 @@ function checkLaunch(launch, roles, definitions, file) {
   for (const name of Object.keys(limits.tokens?.daily ?? {})) {
     if (name !== account?.name) refuse(`${where}.limits.tokens.daily names ${name}, which isn't ${where}.account.name`);
   }
-  for(const [instance,role] of Object.entries(roles))if(role.vars.model!==undefined&&!Object.hasOwn(models,role.vars.model))refuse(`${where}: instance ${instance} selects unknown model ${role.vars.model}`);
+  for(const [instance,role] of Object.entries(roles))if(role.vars.model!==undefined&&!Object.hasOwn(models,role.vars.model))refuse(`${where}: instance ${instance} variable model selects an unknown model`);
   // No upper bound: limits have no ceiling in code (Q150).
   const threadIdleSeconds = launch.threadIdleSeconds === undefined ? THREAD_IDLE_SECONDS : requirePositiveInt(launch.threadIdleSeconds, `${where}.threadIdleSeconds`);
   // Row 194 (#1692, D2.4, D2.5): the human's per-agent forbid. keep names declared instances the

packages/organization/src/vars.mjs

@@ -16,7 +16,7 @@
 
 import { refuse } from "./errors.mjs";
 import { ACTIONS, NETWORKS, TOOLS } from "./vocabulary.mjs";
-import { isPlainObject, requireDistinctList, requirePositiveInt, requireString } from "./util.mjs";
+import { isPlainObject, requireString } from "./util.mjs";
 import { NAME as MODULE_NAME } from "../../plugins/src/kinds.mjs";
 
 export const LAYERS = Object.freeze(["system", "business", "project", "agent"]);
@@ -29,7 +29,7 @@ function string(max = 200) {
 
 function oneOf(values) {
   return (v, key) => {
-    if (!values.includes(v)) refuse(`variable ${key} must be one of: ${values.join(", ")} (got ${JSON.stringify(v)})`);
+    if (!values.includes(v)) refuse(`variable ${key} must be one of: ${values.join(", ")}`);
     return v;
   };
 }
@@ -37,13 +37,13 @@ function oneOf(values) {
 // A harness plug-in module's name, in the loader's pattern (row 65, REQ-PLUG-2). Whether the
 // module exists or is switched on is checked at launch and at a write, never at load.
 function moduleName(v, key) {
-  if (typeof v !== "string" || !MODULE_NAME.test(v)) refuse(`variable ${key} must be a harness module name (got ${JSON.stringify(v)})`);
+  if (typeof v !== "string" || !MODULE_NAME.test(v)) refuse(`variable ${key} must be a harness module name`);
   return v;
 }
 
 function integer(min) {
   return (v, key) => {
-    if (!Number.isSafeInteger(v) || v < min) refuse(`variable ${key} must be an integer of at least ${min} (got ${JSON.stringify(v)})`);
+    if (!Number.isSafeInteger(v) || v < min) refuse(`variable ${key} must be an integer of at least ${min}`);
     return v;
   };
 }
@@ -55,7 +55,7 @@ function baseUrl(v, key) {
   try {
     url = new URL(v);
   } catch {
-    refuse(`variable ${key} must be an http or https URL (got ${JSON.stringify(v)})`);
+    refuse(`variable ${key} must be an http or https URL`);
   }
   if (!["http:", "https:"].includes(url.protocol) || url.username || url.password || url.search || url.hash || v.endsWith("/")) {
     refuse(`variable ${key} must be an http or https base URL with no credentials, query, fragment or trailing slash`);
@@ -70,21 +70,31 @@ function searxngUrl(v, key) {
   baseUrl(v, key);
   const url = new URL(v);
   const loopback = ["127.0.0.1", "localhost", "[::1]"].includes(url.hostname);
-  if (url.protocol === "http:" && !loopback) refuse(`variable ${key} must be https, or http on loopback (got ${JSON.stringify(v)})`);
+  if (url.protocol === "http:" && !loopback) refuse(`variable ${key} must be https, or http on loopback`);
   return v;
 }
 
 function branch(v, key) {
-  if (typeof v !== "string" || !BRANCH.test(v)) refuse(`variable ${key} must be a git branch name (got ${JSON.stringify(v)})`);
+  if (typeof v !== "string" || !BRANCH.test(v)) refuse(`variable ${key} must be a git branch name`);
   return v;
 }
 
 function list(check, { nonEmpty = true } = {}) {
-  return (v, key) => requireDistinctList(v, `variable ${key}`, (item) => check(item, `${key} entry`), { nonEmpty });
+  return (v, key) => {
+    if (!Array.isArray(v)) refuse(`variable ${key} must be an array`);
+    if (nonEmpty && v.length === 0) refuse(`variable ${key} must not be empty`);
+    const seen = new Set();
+    for (const item of v) {
+      check(item, `${key} entry`);
+      if (seen.has(item)) refuse(`duplicate entry in variable ${key}`);
+      seen.add(item);
+    }
+    return [...seen];
+  };
 }
 
 function boolean(v, key) {
-  if (typeof v !== "boolean") refuse(`variable ${key} must be true or false (got ${JSON.stringify(v)})`);
+  if (typeof v !== "boolean") refuse(`variable ${key} must be true or false`);
   return v;
 }
 
@@ -125,7 +135,10 @@ export const REGISTRY = Object.freeze({
   "bus.tester": def(["business"], boolean, { default: false }),
 
   // Project.
-  "tracker.project": def(["project"], (v, key) => requirePositiveInt(v, `variable ${key}`)),
+  "tracker.project": def(["project"], (v, key) => {
+    if (!Number.isSafeInteger(v) || v < 1) refuse(`variable ${key} must be a positive integer`);
+    return v;
+  }),
   // On: the tracker project is the root of a tree, and sync reads its descendants too.
   "tracker.descendants": def(["project"], boolean, { default: false }),
   "git.workingBranch": def(["project"], branch),
@@ -161,16 +174,17 @@ export const REGISTRY = Object.freeze({
   // Limits narrow the role definition's ceilings. limits.authority is an
   // allowlist: a role action it doesn't name becomes gated.
   "limits.tools": def(["business", "project", "agent"], list((v, key) => {
-    if (!TOOLS.includes(v)) refuse(`variable ${key} names an unsupported tool: ${JSON.stringify(v)}`);
+    if (!TOOLS.includes(v)) refuse(`variable ${key} names an unsupported tool`);
   }, { nonEmpty: false }), { merge: "intersect" }),
   "limits.network": def(["business", "project", "agent"], oneOf(NETWORKS), { merge: "intersect" }),
   "limits.authority": def(["business", "project", "agent"], list((v, key) => {
-    if (!ACTIONS.includes(v)) refuse(`variable ${key} names an unknown action: ${JSON.stringify(v)}`);
+    if (!ACTIONS.includes(v)) refuse(`variable ${key} names an unknown action`);
   }, { nonEmpty: false }), { merge: "intersect" }),
 });
 
 // Check one layer's vars object. `where` names the file and path for the
-// message. Returns a frozen copy with checked values.
+// message. Returns a frozen copy with checked values. Validators name the key
+// and the rule, never a value: refusals can reach the Console and the trail.
 export function checkVars(vars, layer, where) {
   if (!LAYERS.includes(layer)) throw new Error(`unknown layer ${layer}`);
   if (!isPlainObject(vars)) refuse(`${where} must be a JSON object`);

packages/organization/tests/agent-levels.test.mjs

@@ -123,7 +123,7 @@ test("malformed root definitions refuse", () => {
 
 test("a refused var names its key, never its value", () => {
   const secret = "sk-secret-marker-7731";
-  const hides = (fn, key) => assert.throws(fn, (error) => error.name === "BusinessError" && error.message.includes(`variable ${key} has a value it can't take`) && !error.message.includes(secret));
+  const hides = (fn, key) => assert.throws(fn, (error) => error.name === "BusinessError" && error.message.includes(`variable ${key}`) && !error.message.includes(secret));
   hides(() => root({ x: { definition: "reviewer", vars: { thinking: secret } } }), "thinking");
   hides(() => root({ x: { definition: "reviewer", vars: { model: "opus-5.5", harness: `${secret}!` } } }), "harness");
   hides(() => root({ x: { definition: "reviewer", vars: { "limits.tools": ["read", secret] } } }), "limits.tools");

packages/organization/tests/organization-config.test.mjs

@@ -251,7 +251,7 @@ test("names that are Object.prototype properties don't count as declared", () =>
   refuses(() => check((d) => { d.arbiters.delivery = "constructor"; }), /arbiters\.delivery names constructor/);
   refuses(() => check((d) => { d.launch.by = "constructor"; }), /launch\.by names constructor/);
   refuses(() => check((d) => { d.launch.instances = ["constructor"]; }), /instances names constructor/);
-  refuses(() => check((d) => { d.roles.pm.vars.model = "constructor"; }), /unknown model constructor/);
+  refuses(() => check((d) => { d.roles.pm.vars.model = "constructor"; }), /variable model selects an unknown model/);
   refuses(() => check((d) => { d.roles.pm.definition = "constructor"; }), /not found/, 4);
   const labels = check((d) => { d.tracker.labels = JSON.parse('{"__proto__": 5}'); }).tracker.labels;
   assert.equal(Object.getPrototypeOf(labels), Object.prototype);

packages/organization/tests/var-redaction.test.mjs

@@ -0,0 +1,140 @@
+import test from "node:test";
+import assert from "node:assert/strict";
+import { join } from "node:path";
+import {
+  BusinessError, checkVars, loadBusiness, loadProject, projectFilePath, systemVars,
+} from "../src/index.mjs";
+import { businessDoc, REPO_ROLES, tmp, writeJson } from "./helpers.mjs";
+import { checkRoutes, launchSettings } from "../src/launch-settings.mjs";
+import { OrganizationError } from "../src/organization.mjs";
+
+const SECRET = "sk-test-NOT-A-SECRET-1234";
+
+function hidesValue(fn, key, reason) {
+  assert.throws(fn, (error) => {
+    assert.ok(error instanceof BusinessError);
+    assert.equal(error.name, "BusinessError");
+    assert.equal(error.exitCode, 2);
+    assert.ok(error.message.includes(`variable ${key}`), error.message);
+    if (reason) assert.match(error.message, reason);
+    assert.ok(!error.message.includes(SECRET), "refusal disclosed a var value");
+    assert.ok(!String(error).includes(SECRET), "rendered error disclosed a var value");
+    return true;
+  });
+}
+
+for (const [scope, key, value] of [
+  ["business", "tracker.kind", SECRET],
+  ["agent", "thinking", SECRET],
+  ["agent", "model", SECRET],
+]) {
+  test(`organization file ${scope} ${key} refusal names its key and hides its value`, () => {
+    const root = tmp();
+    const doc = businessDoc(root);
+    if (scope === "agent") doc.roles.coder.vars[key] = value;
+    else doc.vars[key] = value;
+    writeJson(join(root, "businesses/acme.json"), doc);
+    hidesValue(() => loadBusiness("acme", { dir: root, rolesDir: REPO_ROLES }), key, key === "model" ? /unknown model/ : /one of/);
+  });
+}
+
+for (const scope of ["project", "role"]) {
+  test(`project file ${scope} var refusal names its key and hides its value`, () => {
+    const root = tmp();
+    const vars = { "tracker.project": SECRET };
+    writeJson(projectFilePath(root), {
+      projectVersion: 1, id: "stack",
+      ...(scope === "project" ? { vars } : { roles: { coder: { vars } } }),
+    });
+    hidesValue(() => loadProject(root), "tracker.project", /positive integer/);
+  });
+}
+
+test("systemVars accepts user values and refuses malformed ones without quoting them", () => {
+  const config = {
+    environment: "development", dataRoot: "/d",
+    execution: { backend: "docker", provider: "zai", model: "m", adapter: "pi" },
+  };
+  for (const key of ["environment", "dataRoot", "execution.backend", "execution.provider", "execution.model", "execution.adapter"]) {
+    const bad = structuredClone(config);
+    const value = { pasted: SECRET };
+    if (key.startsWith("execution.")) bad.execution[key.slice("execution.".length)] = value;
+    else bad[key] = value;
+    hidesValue(() => systemVars(bad), key);
+  }
+});
+
+// Exercise every validator that used to quote a scalar, a nested leaf or a duplicate.
+// Removing any of their redactions must fail here even if a caller sanitizes its error.
+for (const [key, layer, value, reason] of [
+  ["tracker.kind", "business", SECRET, /one of/],
+  ["harness", "agent", `${SECRET}!`, /harness module name/],
+  ["fallback.harness", "agent", `${SECRET}!`, /harness module name/],
+  ["thinking", "agent", { pasted: [SECRET] }, /one of/],
+  ["fallback.thinking", "agent", SECRET, /one of/],
+  ["tracker.reconcileMinutes", "business", SECRET, /integer of at least 1/],
+  ["tracker.pollSeconds", "project", SECRET, /integer of at least 10/],
+  ["tracker.baseUrl", "business", SECRET, /http or https URL/],
+  ["gitea.baseUrl", "business", SECRET, /http or https URL/],
+  ["web.searxng", "business", `http://${SECRET}.example`, /https, or http on loopback/],
+  ["git.workingBranch", "project", `${SECRET}!`, /git branch name/],
+  ["git.protectedBranches", "project", [SECRET, `${SECRET}!`], /git branch name/],
+  ["git.protectedBranches", "project", [SECRET, SECRET], /duplicate entry/],
+  ["suites", "project", [SECRET, SECRET], /duplicate entry/],
+  ["tracker.descendants", "project", SECRET, /true or false/],
+  ["bus.tester", "business", SECRET, /true or false/],
+  ["tracker.project", "project", SECRET, /positive integer/],
+  ["limits.tools", "agent", ["read", SECRET], /unsupported tool/],
+  ["limits.authority", "agent", ["message.send", SECRET], /unknown action/],
+  ["limits.network", "agent", SECRET, /one of/],
+]) {
+  test(`checkVars hides refused ${key}: ${reason.source}`, () => {
+    hidesValue(() => checkVars({ [key]: value }, layer, "fixture vars"), key, reason);
+  });
+}
+
+for (const prefix of ["", "fallback."]) {
+  test(`route writer unknown ${prefix}model names its key and hides its value`, () => {
+    const root = tmp();
+    const doc = businessDoc(root);
+    writeJson(join(root, "businesses/acme.json"), doc);
+    const settings = launchSettings({ dir: root, rolesDir: REPO_ROLES });
+    const route = { harness: "pi", model: Object.keys(doc.launch.models)[0], thinking: "high", fallback: null };
+    if (prefix) route.fallback = { harness: "pi", model: SECRET, thinking: "high" };
+    else route.model = SECRET;
+    assert.throws(() => settings.setRoutes("acme", {
+      revision: settings.writer.read("acme").revision, routes: { coder: route },
+    }, "jason"), error => {
+      assert.ok(error instanceof OrganizationError);
+      assert.equal(error.code, "unknown-model");
+      assert.ok(!String(error).includes(SECRET));
+      assert.ok(error.message.includes(`variable ${prefix}model`), error.message);
+      return true;
+    });
+  });
+
+  for (const [field, code] of [
+    ["model", "unknown-model"], ["harness", "harness-off"],
+    ["harness", "harness-refused"], ["thinking", "thinking-unsupported"],
+  ]) {
+    test(`route check ${prefix}${field} ${code} names its key and hides its value`, () => {
+      const vars = prefix ? {
+        harness: "pi", model: "listed", thinking: "high",
+        "fallback.harness": "pi", "fallback.model": "listed", "fallback.thinking": "high",
+      } : { harness: "pi", model: "listed", thinking: "high" };
+      const key = `${prefix}${field}`;
+      vars[key] = SECRET;
+      const proposed = {
+        roles: { coder: { vars } }, launch: { models: { listed: {} } },
+        plugins: { harness: code === "harness-refused" ? ["pi", SECRET] : ["pi"] },
+      };
+      assert.throws(() => checkRoutes({ proposed, levels: name => name === SECRET ? null : ["high"] }), error => {
+        assert.ok(error instanceof OrganizationError);
+        assert.equal(error.code, code);
+        assert.ok(!String(error).includes(SECRET), "route refusal disclosed a var value");
+        assert.ok(error.message.includes(`variable ${key}`), error.message);
+        return true;
+      });
+    });
+  }
+}

packages/webui/tests/agent-levels-route.test.mjs

@@ -88,6 +88,6 @@ test('a refused root file hides the root level only, and says why without a host
   writeFileSync(join(dir, 'agents.json'), JSON.stringify({ agentsVersion: 1, agents: { x: { definition: 'reviewer', vars: { thinking: 'sk-secret-marker-7731' } } } }));
   const badVar = await call({ owner: true, session: {}, organizations: { members }, levels: { ...levels, loadRoot: () => loadRootAgents(dir, { rolesDir }) } });
   assert.equal(badVar.body.root.state, 'invalid');
-  assert.match(badVar.body.root.reason, /variable thinking has a value it can't take/);
+  assert.match(badVar.body.root.reason, /variable thinking must be one of/);
   assert.doesNotMatch(JSON.stringify(badVar.body), /secret-marker/);
 });