diff --git a/agents/rocko/work/row25-setspark-fix-review-2026-09-26.md b/agents/rocko/work/row25-setspark-fix-review-2026-09-26.md new file mode 100644 index 00000000..baae7dda --- /dev/null +++ b/agents/rocko/work/row25-setspark-fix-review-2026-09-26.md @@ -0,0 +1,59 @@ +# Row 25 SetSpark configuration fix review + +Verdict: **approve**. Rocko for Sage, 2026-09-26. + +Packet SHA-256: +`4dec189844f0f77f3fcfca1bf97731f015f41535e97d0b6a47635311900ba7a5`. +The complete staged diff hashes to that same value. The four candidate +working files have no unstaged difference from the index. + +No blocking findings. + +1. **The trimmed object is correct for the extension boundary.** + resolveToolRoots now emits exactly baseUrl, keyFile, principal and + timeoutMs. cli.mjs JSON-serializes toolRoots into TOOLS_ENV. The actual + extension at packages/discord/extension/tools.mjs parses that JSON, + calls loadToolsConfig and only then creates the tool set. + loadSetsparkConfig restores the fixed 262144-byte cap. enabledToolNames + only needs SetSpark presence before this validation; it does not consume + the omitted cap. The check display only consumes baseUrl/principal. + Other inspected SetSpark consumers use the validated binding or + extension config, not the trimmed wire object. + +2. **The connector retains the response cap.** cli.mjs now constructs + createSetsparkApi from binding.tools.setspark, the full validated object. + That API delegates to callApi, whose response-size comparison therefore + receives maxResponseBytes. The factory itself performs no key read or + HTTP call. Principal use in connector/context still comes from the + validated binding. Absent SetSpark produces no API and no record verbs. + +3. **SetSpark check-time validation reads metadata only.** + loadSetsparkConfig calls checkPrivateFile, which uses lstatSync to check + symlink, regular-file, mode and nonzero size. Key contents are read in + readKey, reached by callApi at request time, not by check or preparation. + Check prints service origin and principal, not the key contents. Failure + diagnostics can name the key path; that is distinct from printing the + secret. The existing check command DOES read the separate Discord bot + token and contact Discord; this approval's metadata-only statement is + specifically about the SetSpark key. I did not run a live check or read + either credential. + +4. **The regression test proves the repaired serialization path.** + It starts from validateBinding, passes through resolveToolRoots and a + real JSON round trip into loadToolsConfig, asserts every SetSpark verb, + deep-compares the reconstructed config including the cap, and checks + absence behavior. This catches both dropping the field and passing the + internal cap into the strict input schema. It does not execute CLI + startup or directly regression-test the CLI factory argument; that + one-line wiring is verified by source inspection here. That test scope + is sufficient for this small fix and is not a full live-service claim. + +Independent verification: +`timeout 40s node --test packages/discord/tests/setspark.test.mjs` +returned 12 passed, zero failed, exit 0. Sage's eight-suite index-export +results remain author evidence, not an independent rerun. README now +matches principal being required and timeoutMs being optional. + +Only this report was written. No source/index edits, commit, restart, +credential access or external service call. Approval applies to the pinned +diff; Sage owns integration and the authorized restart. diff --git a/packages/discord/README.md b/packages/discord/README.md index d716e695..4703fa86 100644 --- a/packages/discord/README.md +++ b/packages/discord/README.md @@ -139,7 +139,7 @@ is `src/binding.mjs`. | `engine` | `provider`, `model`, `thinking` for pi | | `limits` | `turnsPerDay` (200), `turnTimeoutSeconds` (180), `replyChunkChars` (1900), `inboundMaxChars` (4000) | | `context.files[]` | files appended to pi's system prompt in order, repository-relative and inside the repository (no absolute paths, `..` or symlinks); the Discord block is added after them | -| `tools` | optional. `roots[]` of `{name, path, write?, git?}`: absolute directories the seat may read through `list_dir`, `read_file` and `search`; a root with `"write": true` may also be written through `write_file` and `edit_file`; a writable root that is a git work tree may carry `git` `{branch, identity, tokenFile, author, protocol?}` and gains `git_status`, `git_commit`, `git_pull` and `git_push` (`protocol: "vault"` adds `reserve_id`); `maxFileBytes` (262144), `maxCallsPerTurn` (8); `web` (optional) `{searxng, maxFetchBytes}` enables `web_fetch` and `web_search` through the named SearXNG instance (https, or http on loopback; `maxFetchBytes` 1048576); `setspark` (optional) `{baseUrl, keyFile, principal?}` names the SetSpark record service (https origin, or http on loopback; key file absolute, 0600, read per call, never printed) and turns on connector-verified approvals. Absent means no tools and a pi launch with `--no-tools`. A root may not be `/`, the home directory, a symlink, a path with a dot-prefixed segment, or anything inside or above the data root | +| `tools` | optional. `roots[]` of `{name, path, write?, git?}`: absolute directories the seat may read through `list_dir`, `read_file` and `search`; a root with `"write": true` may also be written through `write_file` and `edit_file`; a writable root that is a git work tree may carry `git` `{branch, identity, tokenFile, author, protocol?}` and gains `git_status`, `git_commit`, `git_pull` and `git_push` (`protocol: "vault"` adds `reserve_id`); `maxFileBytes` (262144), `maxCallsPerTurn` (8); `web` (optional) `{searxng, maxFetchBytes}` enables `web_fetch` and `web_search` through the named SearXNG instance (https, or http on loopback; `maxFetchBytes` 1048576); `setspark` (optional) `{baseUrl, keyFile, principal, timeoutMs?}` names the SetSpark record service (https origin, or http on loopback; key file absolute, 0600, read per call, never printed) and turns on connector-verified approvals. Absent means no tools and a pi launch with `--no-tools`. A root may not be `/`, the home directory, a symlink, a path with a dot-prefixed segment, or anything inside or above the data root | Unknown keys, missing fields, wrong types, empty allowlists, a user channel that is not listed and a bot listed as a user all refuse with exit 2. A diff --git a/packages/discord/src/binding.mjs b/packages/discord/src/binding.mjs index 587452fd..c9349b81 100644 --- a/packages/discord/src/binding.mjs +++ b/packages/discord/src/binding.mjs @@ -360,5 +360,8 @@ export function resolveToolRoots(binding, { dataRoot }) { if (real === data || real.startsWith(data + sep) || data.startsWith(real + sep)) throw new DiscordError(`tool root ${r.name} overlaps the data root: ${r.path}`); return { name: r.name, path: real, write: r.write, ...(r.git ? { git: { branch: r.git.branch, identity: r.git.identity, tokenFile: r.git.tokenFile, author: `${r.git.author.name} <${r.git.author.email}>`, ...(r.git.protocol ? { protocol: r.git.protocol } : {}) } } : {}) }; }); - return { roots, maxFileBytes: binding.tools.maxFileBytes, maxCallsPerTurn: binding.tools.maxCallsPerTurn, ...(binding.tools.web ? { web: { searxng: binding.tools.web.searxng, maxFetchBytes: binding.tools.web.maxFetchBytes } } : {}) }; + // setspark carries only the keys loadSetsparkConfig accepts; the extension + // re-validates it and adds the response cap itself. + const ss = binding.tools.setspark; + return { roots, maxFileBytes: binding.tools.maxFileBytes, maxCallsPerTurn: binding.tools.maxCallsPerTurn, ...(binding.tools.web ? { web: { searxng: binding.tools.web.searxng, maxFetchBytes: binding.tools.web.maxFetchBytes } } : {}), ...(ss ? { setspark: { baseUrl: ss.baseUrl, keyFile: ss.keyFile, principal: ss.principal, timeoutMs: ss.timeoutMs } } : {}) }; } diff --git a/packages/discord/src/cli.mjs b/packages/discord/src/cli.mjs index 93d18805..045f4918 100755 --- a/packages/discord/src/cli.mjs +++ b/packages/discord/src/cli.mjs @@ -214,7 +214,7 @@ async function run(opts) { // The connector's own SetSpark client (bind and approvals) uses the same // key as the model's verbs; without a setspark key it has none, and an // approval request from the model is refused. - const api = toolRoots && toolRoots.setspark ? createSetsparkApi(toolRoots.setspark) : null; + const api = binding.tools && binding.tools.setspark ? createSetsparkApi(binding.tools.setspark) : null; const connector = createConnector({ binding, journalDir, rest, gateway, engine, api, log: warn }); let shuttingDown = false; let exitCode = 0; diff --git a/packages/discord/tests/setspark.test.mjs b/packages/discord/tests/setspark.test.mjs index fc613011..f88dfb23 100644 --- a/packages/discord/tests/setspark.test.mjs +++ b/packages/discord/tests/setspark.test.mjs @@ -12,7 +12,7 @@ import { loadSetsparkConfig, readKey, idempotencyKey, connectorKey, callApi, renderRefusal, createSetsparkApi, renderRecord, } from "../src/setspark.mjs"; import { loadToolsConfig, createToolSet, enabledToolNames } from "../src/tools.mjs"; -import { validateBinding } from "../src/binding.mjs"; +import { validateBinding, resolveToolRoots } from "../src/binding.mjs"; import { makeRoot, rawBinding } from "./helpers.mjs"; const KEY_A = "ssk_" + "a".repeat(40); @@ -120,6 +120,24 @@ test("setspark config: reaches the tools config and the binding as a fixed key", assert.throws(() => validateBinding(rawBinding({ tools: { roots: [{ name: "docs", path: docs }], setspark: { baseUrl: "https://api.setspark.io", keyFile: kf, principal: "sage", nope: 1 } } })), /unknown key/); }); +test("setspark config: the binding's key survives resolveToolRoots and the engine's JSON hand-off to the extension", () => { + const root = makeRoot(); + const docs = join(root, "docs"); + mkdirSync(docs); + const dataRoot = join(root, "data"); + mkdirSync(dataRoot); + const kf = keyFile(root); + const b = validateBinding(rawBinding({ tools: { roots: [{ name: "docs", path: docs }], setspark: { baseUrl: "https://api.setspark.io", keyFile: kf, principal: "sage", timeoutMs: 5000 } } })); + const resolved = resolveToolRoots(b, { dataRoot }); + assert.deepEqual(resolved.setspark, { baseUrl: "https://api.setspark.io", keyFile: kf, principal: "sage", timeoutMs: 5000 }); + for (const name of SETSPARK_TOOL_NAMES) assert.ok(enabledToolNames(resolved).includes(name), `${name} reaches pi's --tools list`); + const ext = loadToolsConfig(JSON.parse(JSON.stringify(resolved))); + assert.deepEqual(ext.setspark, b.tools.setspark, "the extension rebuilds the same config, response cap included"); + const plain = resolveToolRoots(validateBinding(rawBinding({ tools: { roots: [{ name: "docs", path: docs }] } })), { dataRoot }); + assert.equal("setspark" in plain, false); + assert.ok(!enabledToolNames(plain).some((n) => SETSPARK_TOOL_NAMES.includes(n))); +}); + test("setspark keys: read per call, one printable token per file, rotation without a restart", async () => { const root = makeRoot(); const c = config(root);