fix(discord): row 25 SetSpark key reaches pi and the connector (#1509)
resolveToolRoots dropped tools.setspark, so the record verbs never reached pi's --tools list and the connector never built its SetSpark client. The resolved config now carries only baseUrl, keyFile, principal and timeoutMs, the keys the extension's loadSetsparkConfig accepts; the extension restores the response cap. The connector's client uses the validated binding object, which keeps the cap. README: principal is required, timeoutMs is optional. Test (failing first): binding -> resolveToolRoots -> JSON -> loadToolsConfig gives the binding's config, and the verbs reach enabledToolNames. Eight suites green on an index export. Rocko approved (agents/rocko/work/row25-setspark-fix-review-2026-09-26.md, a28df89e). Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
@@ -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.
|
||||
Reference in New Issue
Block a user