docs(records): SetSpark approver fix landed in shared-signals cc74d92, lead decision 24

Packet, Rocko's R1 and R2 reviews, DEFERRED outcome and SESSIONS.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

agents/rocko/work/setspark-approvers-review-2026-09-26.md

@@ -0,0 +1,139 @@
+# Shared Signals required_approvers — independent review
+
+Verdict: **revise**. Rocko for Sage, 2026-09-26.
+
+Pins verified:
+- Base: `492548af3b48d039023b5ece2e7943eb6d15556f`
+- fix.patch: `e4dfc1e56ad7a667e4af94920bb864197c128875dc3a5420b2af5ce65459ab0a`
+- manifest.sha256: `227e87a1d345f6b96beca863ed542265d9b653663221329a330a6544a0627372`
+- NOTES.md: `d2e60e47546b11c0e6ba59608ea3bf610eb9dc15e972785e088eccd7c5f00513`
+
+Reviewed in `/tmp/rocko-setspark-review-BromBE`, a shared clone detached at
+the specified base with the packet applied. All three manifest entries
+matched before testing and afterward. The source checkout was not edited.
+Lead decision 21 supplies the approver ruling; current item 23 is about
+queue A1, not an additional approver rule. That reference mismatch does not
+prevent reviewing the explicit assignment.
+
+## 1. Blocking, medium — legacy requests and direct status writes bypass the new invariant
+
+The helper enforces 1..16 entries, but `_open_approval_request` checks only
+nonempty list and Discord-id format. It does not enforce the upper bound
+or distinctness on stored legacy rows. More importantly, `_add_approval`
+can change a decision to Accepted through `_write` without calling the
+helper. `_link_supersession` similarly changes it to Superseded without
+checking its approvers.
+
+**Executed, using authentic pre-fix writes rather than corrupting SQL:**
+
+1. Load base service.py from commit 492548af as a separate Python module
+   against a fresh embedded Postgres. Create a Proposed decision with 17
+   distinct valid Discord approvers and open/bind its request. The old
+   validator accepts this shape and computes the real proposal digest.
+2. Switch to the patched Service instance, using the same database.
+3. Open a second request for that same decision. It succeeds with 17
+   approvers: `NEW_OPEN_LEGACY17 17`.
+4. Submit all 17 approvals through the **old** request using the patched
+   service. The last response reports `accepted: true, status: Accepted`.
+5. Create and approve a valid successor. The old decision becomes
+   `Superseded`, still with 17 required approvers.
+
+Thus existing records do not merely remain untouched until remediation:
+new approval-request and sealing writes carry forward a list the new
+helper rejects. This is not a claim that those approvals were forged;
+it is a demonstrated bypass of the newly promised validation invariant.
+The 185-test suite does not cover this upgrade scenario.
+
+**Fix:** validate the full stored list before opening a request. Revalidate
+both request and current decision readiness at the approval mutation
+boundary, including requests created before deployment, and enforce the
+sealed-status helper rule before Accepted/Superseded writes. Keep checks
+inside the existing transaction so refusal leaves no new approval, status
+change or partial supersession. Preserve existing evidence and idempotent
+historical receipts; do not introduce an automatic data migration.
+
+**Acceptance:** construct a legacy 17-approver decision/request through
+base behavior, then upgrade in place. New open, add/seal and supersession
+must refuse under a documented fixed error, without values or partial
+writes. Also cover an old mixed pending/id request: readiness cannot be
+assumed just because a request row exists. Keep the ordinary current-valid
+approval path green. Align the open-request diagnostic SQL with the full
+readiness rule if that rule is broadened to include these bounds.
+
+## 2. Not blocking — NOTES overstates resolve_links validation
+
+The packet says resolve_links revalidates through `_validate`. It actually
+calls `MODELS[rtype].model_validate`, which checks structural types and does
+not invoke check_required_approvers. It does not write approvers, and later
+finalize_import does use `_validate`, so this is not another way to import
+an invalid final record through the reviewed path. Correct the claim so the
+path audit does not imply broader enforcement than exists.
+
+## Other requested checks
+
+**Create/update/import/finalize:** their `_validate` calls reach the helper.
+Update validates the merged record and direct update cannot newly set
+Accepted/Superseded. Import validates before its special sealed-status and
+approval-evidence writes. `_import_approvals` already requires stable ids;
+its older pending rejection is a legitimate redundant guard. Current
+migration imports and finalization remain compatible.
+
+**Error-value exposure:** the new helper and readiness errors carry fixed
+messages, no submitted values or extra detail. ApiError initializes its
+Exception args with that safe message; its ordinary repr is safe. REST
+returns body(), and MCP wraps that same body. Pydantic validation is
+converted to field locations/error kinds, not raw input, and database error
+logging here uses exception class names. I found no new approver-value
+leak in these refusal paths. This is not a claim that successful record
+reads/audit snapshots hide stored approvers: those deliberately contain
+record data. The added tests check messages; body/extra/repr and log capture
+would strengthen the explicit no-echo boundary without replacing the
+source inspection.
+
+**README SQL:** independently extracted and executed both queries against
+a separate disposable PostgreSQL 16.2 instance. Tested 296 decision cases
+(74 lists under four statuses) and 74 open-request cases, including all 29
+Python isspace characters, whitespace around nonempty pending text,
+zero-width space, Arabic-Indic/fullwidth digits, trailing newline/space,
+nonarrays, nonstrings, duplicates and 17 entries. No mismatch with the
+Python helper for decisions or the current Discord-only readiness predicate
+for requests. The explicit whitespace class matches strip for these cases.
+The second query is narrower than full list validity, as finding 1 notes.
+No query was run on production.
+
+**Tests that pass without the fix:** NOTES distinguishes duplicate/empty
+list checks, positive compatibility cases and the old sealed-import guard
+from new regression evidence. That is honest. A missing-helper error in
+test 5 proves test sensitivity to removal, not by itself every status
+branch; its direct positive/refusal assertions still exercise those branches.
+The absent upgrade/old-request test is the material gap, not the inclusion
+of existing guards.
+
+**Vault and mirror:** the API is deliberately stricter than the vault
+validator on Proposed/Rejected entries and on cardinality. Current pending
+markers are retained, and the current vault passes. The successful full
+suite includes migration and mirror tests. Existing arbitrary Proposed
+names elsewhere would now require explicit correction before import; no
+such current-vault regression was observed. No data migration is included.
+
+## Independent verification
+
+- System `python3 -m unittest discover -s tools/tests -v`: 131 tests,
+  OK, one API-module skip for missing dependencies.
+- `python3 tools/validate_vault.py`: PASS, 45 structured records.
+- With `/tmp/ssapi-venv/bin/python` and an explicitly supplied fresh local
+  pgserver database: full discovery, 185 tests, OK, exit 0 (39.624 seconds).
+- Separate fresh database: upgrade reproduction above, all three observed
+  bypasses confirmed.
+- Separate fresh database: SQL/Python comparison, zero mismatches.
+- Manifest and `git diff --check` passed in the review copy.
+
+The first attempt to run the extra upgrade fixture reused the suite's DB
+and stopped at duplicate synthetic test-key setup. It made no upgrade
+finding; the successful reproduction used another fresh DB and the base
+service to create the legacy state. Test servers were stopped by pgserver
+cleanup. No Docker, VM 1022, live service, or ~/.config/setspark key file
+was used. PostgreSQL 17 was not exercised.
+
+Only this report was written in the Mosaic checkout. No commit, push,
+deployment or edit to the shared-signals source checkout.

agents/rocko/work/setspark-approvers-review-r2-2026-09-26.md

@@ -0,0 +1,123 @@
+# Shared Signals approvers R2 — independent review
+
+Verdict: **approve**. Rocko for Sage, 2026-09-26.
+
+Verified pins:
+- Base: `492548af3b48d039023b5ece2e7943eb6d15556f`
+- fix.patch: `41e735f4e6a31e6f7011054dcfc819cd89c9b1b6ce8efc8f22742146984c0cec`
+- manifest.sha256: `23c0c52cfb40566ad4d62cc9543390ad919331881036a8d183d673e5d5066788`
+- NOTES.md: `804256cf7569c447eb11d42d2045725faf77e850b9db21ed69a702be7be63c8b`
+
+Reviewed in a fresh shared clone, `/tmp/rocko-setspark-r2-dd5SY9`, detached
+at the base with the full patch applied. All three manifest entries matched
+before and after verification. No blocking finding remains from report
+706e9ac1.
+
+## 1. Approval and sealing coverage is correct
+
+Open uses the full Accepted-form readiness predicate. add_approval checks
+both the stored request copy and current decision list before inserting
+approval evidence. Both records are locked in the transaction; no later
+step changes either list before the status write. This covers its seal.
+
+The other path to Accepted is import. It calls _validate with the final
+requested status before temporarily inserting Proposed, then verifies
+imported approval evidence before writing the final status. Thus its
+Accepted/Superseded import path already reaches the strict helper. Ordinary
+update cannot newly set those sealed statuses and validates the merged
+record. Finalization validates unfinished imports; resolve_links only
+changes links and type-checks, as the corrected NOTES now says.
+
+Both supersession routes call _link_supersession, which refuses a bad
+predecessor before changing it. The already-Superseded/same-successor early
+return performs no new status write; returning existing history is not a
+new invalid seal. Existing idempotent success receipts likewise remain
+historical responses, not new writes.
+
+## 2. Rollback verified with real base-created states
+
+In another fresh embedded database I loaded service.py from commit
+492548af as a separate module. Through that base service, without planting
+invalid decision rows, I created:
+
+- a 17-id Proposed decision with a bound open request;
+- a mixed pending/id Proposed decision with a bound open request;
+- a 17-id Accepted predecessor, completed through all 17 approval calls;
+- an Accepted imported successor pointing to that predecessor, still
+  import_pending (a legitimate intermediate migration state).
+
+After switching to R2 on the same database:
+
+- opening another request on the 17-id decision refused approvers_not_ready;
+- the first approval on each old 17-id/mixed request refused approvers_not_ready;
+- a normal valid successor's completing approval refused invalid_record;
+- the explicit supersede verb using the genuinely imported Accepted
+  successor also refused invalid_record.
+
+For the completing-approval refusal I compared complete rows before/after
+for both decisions, successor approvals, the request and affected audit
+entries. All were identical. The existing first approval remained; the
+second approval, successor seal/snapshot and predecessor update did not
+remain. A failed-command receipt is intentionally committed outside the
+handler savepoint; “nothing half-written” does not mean no failure receipt.
+
+## 3. Nonblocking fixture qualifications
+
+Not every planted row is literally a base-produced state:
+
+- the base validator already rejected duplicate required approvers, so the
+  duplicate case is defensive corrupt-state coverage, not upgrade evidence;
+- plant_legacy_decision(status=Accepted) does not create the approval rows
+  and accepted snapshot that a real base seal produces;
+- the direct-supersede test manually marks a successor Accepted while its
+  predecessor remains Accepted. Normal completing approval would update
+  both atomically, so that exact planted ordinary-approval history is not
+  achievable. A still-pending Accepted import supplies a real reachable
+  state for testing the explicit supersede route instead.
+
+These limitations should be labelled in the test descriptions/NOTES.
+They do not block this candidate: the independent base-code reproduction
+above covers the meaningful upgrade and rollback properties with genuine
+states. The author also reports a separate base-code reproduction.
+
+## 4. Accepted operational consequence, with one correction
+
+The sealed >16-id predecessor is intentionally unrecoverable through normal
+update and cannot be superseded after this change. That is Sage's explicit
+fail-closed ruling; no new correction mechanism is requested here. A valid
+successor can collect partial approvals but cannot complete while linked to
+that predecessor. The transaction keeps its attempted completion atomic.
+
+Do not justify rarity solely by “more than 16 real approvals.” The base
+coordinator import path could seal from supplied, structurally checked
+approval evidence; it does not require 17 live connector interactions or
+independently authenticate the cited Discord messages. Both ordinary seals
+and historical imports therefore belong in the README query's operational
+survey. This does not widen the accepted failure mode, but it weakens the
+proposed reason to assume no affected record exists. A discovered sealed
+case still goes to Jason for the separately reviewed correction, as ruled.
+
+## Verification
+
+- System `python3 -m unittest discover -s tools/tests -v`: 131 tests, OK,
+  one API-module dependency skip.
+- `python3 tools/validate_vault.py`: PASS, 45 structured records.
+- Full discovery with the existing test venv and a fresh explicitly
+  configured embedded Postgres: 188 tests, OK, exit 0 (39.711 seconds).
+- Independent base-code upgrade/rollback experiment: all refusals and
+  unchanged-state assertions passed.
+- Updated README queries independently compared with Python on 296
+  decision cases and 74 open-request cases, including all 29 Python
+  whitespace characters and Unicode digits: zero mismatches. Request SQL
+  now covers the full readiness rule, including bounds and duplicates.
+- Manifest checks and git diff --check passed in the copy.
+
+No new value-echo path was introduced by the guards: errors remain fixed
+text, with a record id in the supersession diagnostic rather than submitted
+approver values. Current vault migration/mirror compatibility remains green.
+PostgreSQL 16.2 was exercised; PostgreSQL 17 and production were not.
+
+Test servers were stopped through pgserver cleanup. No Docker, VM 1022,
+~/.config/setspark key file, live service, or source-checkout edit was used.
+Only this report was written in the Mosaic checkout. No commit, push or
+deployment was performed.