fix(discord): row 25 approvers are user names, never Discord ids in tool text (#1509)
Jason's live check after the 20:58Z restart posted no Approve button. The
Discord Sage wrote DEC-009's required_approvers as names; the SetSpark
service stores approvers as discord:<id> and accepted the names, and the
connector correctly refused the approval request ("bad approver id").
- binding.mjs derives setspark.approvers from the binding's users (name to
id); a binding-set approvers key and duplicate names are refused. With
setspark set, a user id or name change refuses the reload (pi's approvers
are fixed at start).
- setspark.mjs: record_create/record_update map required_approvers names to
discord:<id> and refuse unknown names, ids, duplicates and non-lists
before any request, without echoing the value. hideIds turns mentions,
discord: values and standalone 17-20 digit runs into the user's name or
"unknown user" in every verb's text and refusal, including the service
message and code before they are cut. The connector's approval request
keeps the bare ids.
- tests: boundary test over nested, keyed, numeric, mention and cut ids;
a local contract fixture from create through validateRequest, with the
old name-stored shape still refused.
Rocko: R1 revise, R2 revise, R3 approve (81379830..., report da75219f...).
Suites on an index export: 24/90/43/17/14/15/63/18; Discord node tests 173/173.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
Claude Opus 5.5
@@ -0,0 +1,78 @@
|
||||
# Row 25 approver names R2 — review
|
||||
|
||||
Verdict: **revise**, one remaining medium finding. Rocko, 2026-09-26.
|
||||
|
||||
Packet and staged diff verified as
|
||||
`7da44a5975e6261dc524431df2a3d3f9093e064d26d67e9b1f5926467d9c00f9`.
|
||||
|
||||
## 1. Medium — two error paths still cut identities before sanitization
|
||||
|
||||
The fix correctly hides service messages before their 400-character cut,
|
||||
and hides successful output deeply before its field/render limits. But:
|
||||
|
||||
- callApi still takes `err.code.slice(0, 64)` before renderRefusal hides
|
||||
identities (setspark.mjs line 265).
|
||||
- needObject and record_list's filter-name refusal still interpolate
|
||||
`k.slice(0, 32)` before renderRefusal (lines 342 and 427).
|
||||
|
||||
**Executed reproductions:** with synthetic id `100000000000000100`:
|
||||
|
||||
1. A local fake service returns HTTP 422 with code consisting of 49 x's,
|
||||
a space and the id. The 64-character cut retains 14 digits. Final text:
|
||||
|
||||
```text
|
||||
refused: the record service refused the request (code xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx 10000000000000)
|
||||
refused
|
||||
```
|
||||
|
||||
2. record_create receives an invalid property name consisting of 17 x's,
|
||||
a space and the id. No request is sent. Final refusal:
|
||||
|
||||
```text
|
||||
refused: the call's arguments are not valid
|
||||
record has a property name that is not allowed: xxxxxxxxxxxxxxxxx 10000000000000
|
||||
```
|
||||
|
||||
The filter-name diagnostic has the same cut-before-hide pattern. These
|
||||
14-digit fragments survive because hideIds correctly targets complete
|
||||
17–20 digit tokens. This is the same truncation problem R2 explicitly fixes
|
||||
for service messages and tests for 12–16 digit remnants; the other early
|
||||
cuts were missed.
|
||||
|
||||
**Fix:** sanitize the service code before its length cap, while retaining
|
||||
any raw code needed privately for reason classification. For invalid
|
||||
property/filter names, prefer a fixed or position-based diagnostic that
|
||||
does not echo the invalid value, as already done for approvers. Alternatively
|
||||
sanitize the complete value before slicing. Add regression cases for both
|
||||
code and property/filter-name cut boundaries. Do not widen the final regex
|
||||
to redact arbitrary shorter numbers; fix the operation order.
|
||||
|
||||
## Previous findings and requested checks
|
||||
|
||||
The rejected-approver echo is closed. All eight success renderers share the
|
||||
new boundary, including resolve/document output. Embedded mentions, nested
|
||||
keys/values and numeric values are handled before successful output is cut.
|
||||
The deliberate longer-token exception is documented; this is a text-format
|
||||
rule, not semantic recognition of every possible identity encoding.
|
||||
|
||||
The contract fixture now connects name-based create, stored discord:<id>,
|
||||
bare-id approval view and validateRequest. It also retains the malformed
|
||||
legacy-name refusal. The raw connector request remains intact; sanitization
|
||||
operates on the rendered copy. That separation is correct. Existing bad
|
||||
records still need a separately authorized correction.
|
||||
|
||||
The restart requirement for changed name/id mappings remains acceptable:
|
||||
Pi's map is fixed at startup and hot-reloading only the connector would
|
||||
split authority views. Channel changes remain reloadable.
|
||||
|
||||
## Verification
|
||||
|
||||
`timeout 30s node --test packages/discord/tests/setspark.test.mjs`:
|
||||
16 passed, zero failed, exit 0. Both additional failures above were executed
|
||||
independently, using a disposable local HTTP server and fake key for the
|
||||
service case and a direct pre-send refusal for the property-name case.
|
||||
Temporary fixtures were removed. No live token or service was accessed.
|
||||
|
||||
Only this report was written in the repository. No source/index edits,
|
||||
commit, push or restart. This remaining finding should be fixed before the
|
||||
approval requested for R2.
|
||||
@@ -0,0 +1,40 @@
|
||||
# Row 25 approver names R3 — review
|
||||
|
||||
Verdict: **approve**. Rocko for Sage, 2026-09-26.
|
||||
|
||||
Packet and complete staged diff both verified as
|
||||
`81379830715f48ff3c8e8dac1c1556925aa6b599f5ffc8a0ae231eb7a3b1c2a9`.
|
||||
The reviewed source/test working files match the index. Compared R3 with
|
||||
the pinned R2 packet: changes are limited to the three truncation fixes
|
||||
and their test fixtures/assertions.
|
||||
|
||||
The remaining R2 finding is closed. Service error codes now pass through
|
||||
hideIds before the 64-character limit. Invalid record-property and filter
|
||||
names pass through it before the 32-character limit. Their null config
|
||||
correctly yields unknown user rather than echoing an identity. The final
|
||||
render/refusal passes remain, and the regex was not broadened to hide
|
||||
arbitrary shorter numbers. The new tests exercise all three early-cut
|
||||
paths and check the resulting tool text for full ids and digit remnants.
|
||||
|
||||
The other slice sites do not reopen this finding: list/match slices limit
|
||||
array counts; successful text limits follow hideDeep; service message cuts
|
||||
follow hideIds. The approver-name config validation diagnostic is a startup
|
||||
configuration error, not a model-facing tool return. The documented
|
||||
longer-token exception remains the scope of the text-hiding rule.
|
||||
|
||||
The previous name mapping, fixed-config reload restriction, raw internal
|
||||
approval request and connected service-contract fixture remain acceptable.
|
||||
This approval does not repair existing malformed records automatically or
|
||||
prove a live service round trip; Jason's planned acceptance check supplies
|
||||
that evidence after the authorized integration/restart.
|
||||
|
||||
Independent verification:
|
||||
`timeout 30s node --test packages/discord/tests/setspark.test.mjs`
|
||||
returned 16 passed, zero failed, exit 0. This includes boundary and approval
|
||||
contract fixtures. Sage's 173-test run, index-export suites and live check
|
||||
are author evidence, not independent reruns. The disclosed Docker pool
|
||||
failure and cleanup do not change the candidate reviewed here.
|
||||
|
||||
Only this report was written. No source/index edits, commit, push, live
|
||||
credential access, Docker action or restart. Approval applies to the pinned
|
||||
R3 diff; Sage owns the authorized integration and restart.
|
||||
@@ -0,0 +1,104 @@
|
||||
# Row 25 approver names fix — review
|
||||
|
||||
Verdict: **revise**. Rocko for Sage, 2026-09-26.
|
||||
|
||||
Packet and complete staged diff both verified as:
|
||||
`c17e15cd2a7e398e72a1d4c5e69e35f42255facb621bde7aa786163a4f00c9ed`.
|
||||
The name-to-identity write mapping addresses the reported malformed
|
||||
approver failure, but the stated model-visible ID boundary is incomplete.
|
||||
|
||||
## 1. Medium — rejected raw IDs are echoed back to the model
|
||||
|
||||
`approverIds` embeds the rejected value in the bad-argument message.
|
||||
`renderRefusal` prints that message. Executed with a synthetic id:
|
||||
|
||||
```text
|
||||
refused: the call's arguments are not valid
|
||||
required_approvers: "discord:100000000000000100" is not a known user; use names from: jason
|
||||
```
|
||||
|
||||
No request or key read is needed for this reproduction. The existing raw-id
|
||||
test asserts only `ok === false`, not that the returned text omits the id.
|
||||
Server refusal messages, codes and changed_fields are also rendered without
|
||||
this new mapping, so a service rejection that echoes an approver can expose
|
||||
it even when the model supplied only a name.
|
||||
|
||||
**Fix:** never echo a raw identity in the invalid-name diagnostic; provide
|
||||
known names and a fixed reason. Apply the intended model-facing identity
|
||||
policy to all rendered refusal fields, including service error text. Test
|
||||
the final tool text, not only the verb's rejection status.
|
||||
|
||||
## 2. Medium — namesFor matches whole string values only, and only two verbs use it
|
||||
|
||||
The anchored regex converts exact bare or discord-prefixed string values.
|
||||
It does not remove an id inside prose, a Discord mention, an object key,
|
||||
or a numeric JSON value. A local fake service returned a record containing
|
||||
an exact required_approvers value plus nested text and a body mention.
|
||||
`record_get` produced:
|
||||
|
||||
```text
|
||||
nested: {"text":"approver discord:100000000000000100"}
|
||||
required_approvers: jason
|
||||
body:
|
||||
Ask <@100000000000000100>
|
||||
```
|
||||
|
||||
Thus the happy-path assertion for the simple required_approvers field does
|
||||
not establish “no Discord id reaches the model.” `resolve_id` renders match
|
||||
titles without mapping, and create_document renders title/URL without it.
|
||||
Approval view rendering also interpolates service fields directly. Record
|
||||
create/update return terse receipts, so their unconverted raw records are
|
||||
not themselves proof of a current approver-list text leak; assess the final
|
||||
rendered surface instead of merely the intermediate object.
|
||||
|
||||
**Fix:** centralize model-facing rendering/sanitization across all SetSpark
|
||||
success and refusal text, with a defined rule for known identities,
|
||||
unknown identities and embedded Discord mentions/prefixed values. Cover
|
||||
nested fields and keys, numeric identity values, embedded text, resolve
|
||||
results, document receipts and service errors in boundary tests. If the
|
||||
intended promise is narrower than every Discord id in arbitrary service
|
||||
text, state that narrower promise explicitly instead of claiming complete
|
||||
ID concealment. Do not sanitize the connector's internal approval request
|
||||
ids into names: that would break its authorization check.
|
||||
|
||||
## Requested checks that are fine
|
||||
|
||||
**Reload refusal is acceptable.** The derived map is part of tools, which
|
||||
Pi receives once at startup. Allowing name/id edits only on the connector
|
||||
side would split the identity views. Refusing those reloads until restart
|
||||
is conservative and documented; channel-only changes remain reloadable.
|
||||
JSON equality can also refuse user reordering or equivalent map insertion
|
||||
orders, and case-only normalized names may compare unchanged. Those are
|
||||
availability/normalization details, not authority widening. An urgent user
|
||||
removal requires an operator stop/restart under this policy, not an assumed
|
||||
successful hot reload.
|
||||
|
||||
**Keep the bare-id connector path.** Under the stated service contract,
|
||||
create/update sends discord:<id> in record properties and the service's
|
||||
approval-request view returns bare snowflakes. open_approval_request copies
|
||||
those into request.approvers; setsparkDetails carries that request through
|
||||
tool details to the engine/connector, and validateRequest requires 1..16
|
||||
unique bare snowflakes. That is the correct internal representation.
|
||||
Model-facing prose must be handled separately. If the service returns
|
||||
names or prefixed ids instead, refusing is correct; do not weaken
|
||||
validateRequest to make malformed views pass.
|
||||
|
||||
The added tests prove name mapping, rejection before requests, round-trip
|
||||
configuration and reload restrictions. They do not prove the whole
|
||||
create-decision → service view → connector validation/button path against
|
||||
the service normalization contract. Add a local contract fixture connecting
|
||||
those stages, while retaining malformed-view negatives. Existing malformed
|
||||
records such as DEC-009 will not be repaired automatically by this code;
|
||||
a separately authorized update can now express their approvers as names.
|
||||
|
||||
## Evidence and boundaries
|
||||
|
||||
Independent `timeout 30s node --test packages/discord/tests/setspark.test.mjs`:
|
||||
14 passed, zero failed, exit 0. The two leaks above were independently
|
||||
executed with synthetic ids and a temporary local HTTP fixture. Only a
|
||||
throwaway fake key was read; no live credential, service or pane was used.
|
||||
The temporary fixture was removed. Staged diff hash remained unchanged.
|
||||
|
||||
Only this report was written in the repository. No source/index edits,
|
||||
commit, push or restart. The approval/restart gate should remain held for
|
||||
these output-boundary fixes or an explicit narrower requirement.
|
||||
Reference in New Issue
Block a user