stack/agents/sage/work/setspark-approvers/NOTES.md

# SetSpark required_approvers fix: review notes (revision 2)

Repository: /mnt/storage/src/shared-signals (jetrich/shared-signals), branch main.
Base commit: 492548af3b48d039023b5ece2e7943eb6d15556f. The tree was clean before the work.
Nothing was committed, pushed, branched, stashed or reset. Sage commits after review.

This revision answers Rocko's review of the first packet (fix.patch e4dfc1e5..., report
/mnt/storage/src/mosaic-stack/agents/rocko/work/setspark-approvers-review-2026-09-26.md,
sha256 706e9ac1...). The next section lists what changed since then; the rest describes the
patch as a whole.

Packet files:

- fix.patch: output of `git -C /mnt/storage/src/shared-signals diff` against the base
  (416 lines). It replaces the e4dfc1e5 patch; it is not an increment on top of it.
- manifest.sha256: sha256 of each changed file after the patch, paths relative to the
  repository root. The patch was applied to a fresh export of the base commit and
  `sha256sum -c manifest.sha256` passed for all three files.

## Changes since e4dfc1e5

Rocko's blocking finding was reproduced before the fix. Rows written by the base code
(a 17-approver Proposed decision with an open request, and a 17-approver decision sealed as
Accepted) let e4dfc1e5 do three things:

- (a) open a new request with 17 approvers;
- (b) collect 17 approvals through the old request and seal the decision as Accepted;
- (c) supersede the sealed legacy decision when a valid successor was accepted.

Changes in `service.py`:

1. New `approvers_ready(approvers)`. It returns True only when the list passes
   `check_required_approvers` in its Accepted form: 1 to 16 distinct `discord:<id>`
   entries and no pending markers. It calls the same helper, so there is still one rule
   and no second copy of it.
2. `_open_approval_request` now requires `approvers_ready` on the stored decision list.
   e4dfc1e5 checked only that every entry was a Discord id, which let 17 entries and
   duplicates through. Error: 422 `approvers_not_ready`, fixed message.
3. `_add_approval` now requires `approvers_ready` on both the request's stored copy and the
   decision's current list. The check runs after the existing `not_open`, `request_stale`
   and `import_pending` checks and before `not_approver` and any write, so a refusal writes
   no approval, no status change and no request state change. Error: 422
   `approvers_not_ready`. Seal to Accepted happens only inside `_add_approval`, after this
   check, so this one guard is also the seal guard. I did not add a second check at the
   seal write: it would be the same predicate on the same row in the same transaction, so
   it could never fire, and no test could prove it.
4. `_link_supersession` now refuses when the old decision's stored list is not ready
   (the rule for Superseded). Both supersession routes go through it: the `supersede`
   verb, and `_add_approval` when the accepted decision carries `supersedes`. Error: 422
   `invalid_record`, because the write is to a decision, not a request. The message names
   the old decision's id and states the rule; it does not include approver values. When
   the route is `_add_approval`, the refusal rolls back the completing approval and the
   successor's seal together.

Other changes:

- The approvers_not_ready message now states the full readiness rule.
- `approvers_not_ready` is the code for open_approval_request and add_approval, because
  both are about collecting approvals. `invalid_record` is the code for supersession,
  because the refused write is the decision's status.
- README: the open_approval_request, add_approval and supersede rows describe the new
  checks. A new paragraph explains why stored rows are rechecked. The approvers_not_ready
  error row covers add_approval.
- README: the second SQL query now uses the full readiness rule (length 1 to 16,
  distinct, Discord ids only), not just "not a Discord id".
- README: a paragraph now says that an Accepted row with a broken list cannot be
  corrected through the API, because the immutability trigger freezes it, and so cannot be
  superseded. See "Open question for the owner" below.
- Tests: three upgrade regressions and two helpers that plant pre-rule rows (described
  below).
- Tests: `assert_invalid_without_echo` now also checks `json.dumps(body())`,
  `repr(exc)` and `str(exc)`, not only the message.
- NOTES: resolve_links was wrongly described as revalidating through `_validate`. It calls
  `MODELS[rtype].model_validate`, which type-checks the record and does not call the
  approver helper. It never writes approvers, and finalize_import later runs `_validate`
  on the final record.

## What changed and why (whole patch)

The defect: setspark-api accepted any distinct nonempty strings as a decision's
`required_approvers`. A decision such as DEC-009, stored with bare names, could never be
approved, because add_approval compares the Discord author id against that list.

`stack/api/setspark_api/service.py`:

- `check_required_approvers(approvers, status)` applies the rule:
  - the list has 1 to 16 entries;
  - each entry is `discord:<id>` matched with the existing `DISCORD_ID_RE` (fullmatch), or
    `pending:<text>` whose text is nonempty after `strip()`;
  - entries are distinct;
  - pending markers are refused when status is Accepted or Superseded.

  Every failure is 422 `invalid_record` with a fixed message that states the rule and
  never includes the submitted value.
- `DISCORD_ID_RE` was already `discord:[0-9]{17,22}`, with ASCII `[0-9]` rather than `\d`.
  It needed no tightening and was not changed. It is the same pattern `validate_vault.py`
  uses at line 167, so the API and the validator share one digit rule.
- `_validate` for decisions calls the helper in place of the old check (`evidence` plus
  distinct). `_validate` covers create, import, update (the merged record) and
  finalize_import.
- `approvers_ready` guards open_approval_request, add_approval (and so the seal) and
  supersession, as described above.
- The older check in `_import_approvals` (Discord ids required for an Accepted or
  Superseded import) was left in place. It is now redundant with the helper but harmless,
  and removing it was out of scope.

`tools/tests/test_setspark_api.py`: eleven new ServiceTests methods, a `BAD_APPROVERS` table,
two id helpers and two legacy-row helpers (listed below).

`stack/api/README.md`:

- A "Required approvers" section states the rule and where each part is enforced.
- Two read-only SQL queries: decisions whose stored list breaks the rule, and open approval
  requests whose list is not ready. DEC-009 is named as the known case.
- `approvers_not_ready` is added to the error table.
- The affected verb rows are updated.

No data migration was written. No database was touched except throwaway test databases.

Pending markers follow `validate_vault.py` lines 160-169 exactly, as the coordinator ruled.
The validator requires Discord ids only when status is Accepted or Superseded. It does not
restrict Proposed or Rejected, so the API allows pending markers on Proposed and Rejected
and refuses them on Accepted and Superseded.

docs/RECORDS.md line 55 says "While Proposed, required_approvers may contain explicit
pending markers". A Rejected decision can only be reached from Proposed and keeps its list,
so allowing pending markers on Rejected matches the validator without contradicting the
convention. validate_vault.py, RECORDS.md and the decision template are unchanged.

`stack/api/openapi.json` is unchanged. It is generated from routes, and no route, request
model or response model changed. `python -m setspark_api.app --openapi` output was compared
byte for byte with the committed file and is identical.

## Write paths and coverage

Every code path that writes a decision's approvers or status, or an approval request:

| Path | Writes | Guarded by | Test |
|---|---|---|---|
| create (`_create`, `_insert`) | decisions | `_validate` then helper | create refuses 13 bad forms; create accepts ids, 17 and 22 digits, a pending marker, 16 entries |
| update (`_update`, `_write`) | decisions | `_validate` on the merged record | update refuses 13 bad forms, revision and list unchanged; valid pending update works; title-only update of a legacy row refused; update that fixes the list works; a Rejected legacy row can be fixed (checked in the SQL script) |
| import (`_import`, `_insert`, sealed status UPDATE) | decisions | `_validate`, and `_import_approvals` before the sealed status write | import refuses 13 bad forms and stores nothing; Accepted import with a pending marker refused |
| finalize_import | import_pending flag | `_validate` on the stored row | stored legacy row refused at finalize |
| open_approval_request | approval_requests (copies the list) | `approvers_ready` on the decision | pending marker refused, no row written, opens after the fix; upgrade test (a): 17 ids, duplicate ids, bare names, pending marker |
| add_approval, including the seal to Accepted | approvals, decisions, approval_requests | `approvers_ready` on request copy and decision, before any write | upgrade test (b): 17-id legacy request and mixed pending/id legacy request refused, nothing written; after the list is fixed the old request goes stale and a new one accepts |
| supersession (`supersede` verb, successor acceptance) | decisions | `approvers_ready` on the old decision in `_link_supersession` | upgrade test (c): both routes refused, successor stays Proposed, legacy row unchanged |

Paths confirmed not to write approvers or status:

- resolve_links writes link columns only. It type-checks with `model_validate`, not
  `_validate`, and finalize_import runs `_validate` afterwards.
- bind_approval_message writes message and channel ids only.
- `stack/mirror` reads the DB and runs validate_vault.py; it never writes the DB.
- `stack/migrate` (ApiSink) writes only through `/v1/import` and `/v1/import/finalize`.
- `stack/api/setspark_api/migrate.py` handles schema and counters only.
- REST and MCP both enter through `Service.execute`.

The only three writes that set Accepted or Superseded are:

- the import seal (guarded by `_validate` and `_import_approvals`);
- the add_approval seal;
- `_link_supersession`.

## Compatibility

- Vault: DEC-001 to DEC-008 are all Proposed, each with two `pending:` markers. All pass the
  helper (asserted in the migration-shaped test).
- The existing digest-equality test imports all eight real DEC files through the API and
  still passes.
- `python3 tools/validate_vault.py`: PASS on 45 structured records.
- The API rule is a strict subset of the validator's rule, so the mirror cannot publish a
  record the validator refuses. The validator accepts bare names on a Proposed decision and
  the API does not. No current vault record has that shape.
- Migration: an import in the vault shape that ApiSink sends (Proposed, pending markers,
  `approvals: []`) still imports and finalizes.
- Stored legacy Proposed or Rejected rows: any update is refused until the same update
  corrects `required_approvers`. Opening a request or approving is refused until then.
  After the correction, an open request from before it goes stale on the next add_approval,
  because the list is digest-covered and the version bumps.
- The ordinary valid approval path is unchanged. All existing approval, acceptance and
  supersession tests pass.
- Error message change: the old invalid_record text "Decision requires distinct, explicit
  required_approvers" is replaced. `git grep` in shared-signals and in mosaic-stack (which
  holds the Discord connector) found no caller that matches on the old text or on
  approvers_not_ready.

## Open question for the owner

An Accepted decision sealed before the rule with a broken list cannot be corrected: the
immutability trigger freezes `required_approvers` on Accepted rows. Under this revision it
also cannot be superseded.

The only such shape the base code could produce is more than 16 distinct Discord ids. The
old code needed every approver to approve, so bare names or pending markers could never
seal. An import also required Discord ids.

The README's first query lists any such row. Whether one exists in production is unknown.
If one does, unblocking it needs an owner decision, for example a one-off reviewed data
correction, which this change deliberately does not include.

## Tests and fail-without-fix evidence

New tests in `tools/tests/test_setspark_api.py` (ServiceTests). Every refusal is checked
with `assert_invalid_without_echo`: status and code, and no submitted value in the
message, body JSON, repr or str.

1. `test_approvers_create_refuses_every_malformed_form_without_echo`: 13 subTests. The cases
   are bare name, bare id, 16 digits, 23 digits, Arabic-Indic digits, fullwidth digits,
   trailing newline, `Discord:` prefix case, empty pending text, blank pending text,
   duplicate, 0 entries and 17 entries.
2. `test_approvers_create_accepts_ids_pending_markers_and_bounds`: one id, 17 and 22 digit
   ids, a pending marker on a Proposed create, and 16 entries.
3. `test_approvers_update_refused_the_same_way`: the 13 cases, each on a fresh decision,
   with revision and list unchanged. Then a valid pending update bumps proposal_version.
4. `test_approvers_import_refused_the_same_way`: the 13 cases, each on an unused id, with
   nothing stored. Then an Accepted import with a pending marker is refused.
5. `test_approvers_pending_markers_follow_the_validator_status_rule`: calls the helper
   directly for all four statuses.
6. `test_approvers_open_request_refused_until_every_approver_is_a_discord_id`: a pending
   marker gets approvers_not_ready with no row written; after an update to two ids, the
   request opens.
7. `test_approvers_migration_shaped_import_with_pending_markers_still_finalizes`: an import
   in the ApiSink shape imports and finalizes, and then open is refused. Every real vault
   DEC file passes the helper.
8. `test_approvers_stored_legacy_row_is_refused_at_finalize_update_and_open`: a
   DEC-009-shaped row is refused at finalize, at open and on a title-only update. A
   correcting update succeeds.
9. `test_upgrade_legacy_decision_cannot_open_a_request`, upgrade part (a): 4 subTests
   (17 ids, duplicate ids, bare names, pending marker). Each gets approvers_not_ready and
   no request row is written.
10. `test_upgrade_legacy_request_cannot_collect_approvals_or_seal`, upgrade part (b):
    2 subTests (17 ids, mixed pending and id).
    - A legacy decision with a legacy open, bound request.
    - The first approval is refused with approvers_not_ready. No approval row is written,
      the request stays open and the decision stays Proposed.
    - A correcting update then makes the old request return request_stale, and a new
      request accepts normally.
11. `test_upgrade_legacy_accepted_decision_cannot_be_superseded`, upgrade part (c).
    - A legacy Accepted decision with 17 ids.
    - A valid successor's completing approval is refused with invalid_record. The
      successor stays Proposed with only its first approval.
    - With the successor planted as Accepted, the `supersede` verb is also refused.
    - The legacy row stays Accepted, with no superseded_by and its list unchanged.

How the upgrade tests build legacy state: two helpers write it in the test database only.
They do not load the base code.

- `plant_legacy_decision` creates a valid decision, rewrites its list by SQL and recomputes
  `proposal_digest` with `decision_digest`, so version and digest stay consistent.
- `plant_legacy_request` inserts the open, bound request row the base
  open_approval_request would have written.

To confirm that the planted state matches real pre-fix behaviour, I also ran Rocko's
scenario with the real base code, out of tree, in /tmp/ssapi-upgrade-repro.py:

- It loads service.py from commit 492548af with `git show`.
- On a fresh database it uses the base code to create a 17-approver decision with an open
  request, a 17-approver decision sealed Accepted through 17 approvals, and a mixed
  pending/id decision with an open request.
- It then drives the working-tree service on the same database.

| Scenario step | with e4dfc1e5 | with this revision |
|---|---|---|
| (a) open a new request on the 17-approver decision | allowed, 17 approvers | 422 approvers_not_ready |
| (b) approvals through the old 17-approver request | allowed; after 17, accepted, status Accepted | 422 approvers_not_ready on the first |
| (b) approval through the old mixed request | allowed | 422 approvers_not_ready |
| (c) successor acceptance superseding the sealed legacy decision | allowed; legacy became Superseded | 422 invalid_record; legacy stays Accepted, superseded_by none |

Proofs by guard: I ran the API module (57 tests) on a fresh database once per service.py
variant, and restored the fixed file afterwards. `cmp` against a saved copy confirmed the
restore.

| service.py variant | Result | Failing new checks |
|---|---|---|
| base (whole fix reverted) | failures=43, errors=1 | create/update/import 11 of 13 cases each; tests 6, 7, 8, 11; all 4 cases of test 9; both cases of test 10; test 5 errors (no helper) |
| e4dfc1e5 (the reviewed packet) | failures=5 | test 9: 17 ids and duplicate ids; test 10: both cases; test 11 |
| this revision without the `_validate` change | failures=34 | create/update/import 11 cases each; test 8 |
| this revision without any open_approval_request check | failures=7 | tests 6, 7, 8; all 4 cases of test 9 |
| this revision with open reverted to the e4dfc1e5 format-only check | failures=2 | test 9: 17 ids and duplicate ids |
| this revision without the add_approval guard | failures=2 | test 10: both cases |
| this revision without the supersession guard | failures=1 | test 11 |
| this revision | 57 run, OK | none |

Each new guard has a test that fails when only that guard is removed.

Some checks pass without the fix. They are not proofs:

- The duplicate and 0-entry cases in tests 1, 3 and 4. The old check already refused them.
- Test 2 and the final valid update in test 3, which are positive guards.
- The Accepted-with-pending import in test 4. The old `_import_approvals` already refused
  it.
- The import and finalize half of test 7.
- The bare-names and pending cases of test 9 under e4dfc1e5, which already refused them.

## README SQL verification

I extracted both queries from the README and ran them on a throwaway pgserver database
(PostgreSQL 16.2, datctype en_US.UTF-8). The seeded rows:

- decisions: 59 approver lists under each of 4 statuses, 236 rows. The lists include every
  Python whitespace character alone as pending text, non-arrays, non-string elements,
  duplicates, and 0, 16 and 17 entries.
- approval_requests: the same 59 lists in each of the open, closed and approved states,
  177 rows.

Rows were seeded with `session_replication_role = replica`. Each row's SQL verdict was
compared with `check_required_approvers` for decisions and with `approvers_ready` for open
requests.

| Table | Flagged by Python | Flagged by SQL | Mismatches |
|---|---|---|---|
| decisions | 208 | 208 | 0 |
| requests | 54 | 54 | 0 |

The request query flags 54 now, against 52 under e4dfc1e5's narrower Discord-only query.
The two extra rows are the 17-entry and duplicate lists.

The pending-text test spells out the exact set of characters that `str.isspace()`
accepts, not `[:space:]`, so its result does not depend on the database locale.

The same script confirms that a Rejected legacy row can be corrected through `update`.
It is /tmp/ssapi-sqlcheck.py, outside both repositories.

## Commands and counts

```
python3 -m unittest discover -s tools/tests -v
    system Python 3.12.8, no API deps: Ran 131, OK (skipped=1; the API module skips itself)
SETSPARK_TEST_DATABASE_URL=<fresh pgserver db> /tmp/ssapi-venv/bin/python -m unittest discover -s tools/tests -v
    Ran 188, OK, three consecutive runs on three fresh databases
    (API module alone: 57 tests; base had 46, so 11 are new; e4dfc1e5 had 54)
python3 tools/validate_vault.py
    PASS: 45 structured records, exit 0
(cd stack/api && python -m setspark_api.app --openapi | cmp - openapi.json)
    identical
git diff --check
    clean
```

Environment: the venv /tmp/ssapi-venv has psycopg 3.3.6, fastapi 0.141.1 and pgserver.
/tmp/ssapi-fresh-db.py creates an empty database on a local embedded server and prints its
URL. All added lines in the diff are ASCII; the non-ASCII test inputs are written as
`\u` escapes.

## What could not be verified

- The suite normally runs against a `postgres:17` container. Docker was off limits, so every
  database run used pgserver's embedded PostgreSQL 16.2 through
  `SETSPARK_TEST_DATABASE_URL`. Nothing in the change depends on a 17-only feature, but it
  has not been run on 17.
- The live SetSpark database was not touched, and the README queries were not run on
  production. Still unknown:
  - whether DEC-009 is the only affected decision;
  - whether any Accepted row holds more than 16 approvers (see the open question);
  - whether any open request holds a list that is not ready.
- The deployed service was not exercised. The change takes effect when Sage's reviewed
  commit is deployed.
- An existing test, `test_supersede_reciprocity_and_acyclicity`, failed once in an early run
  of the first packet with duplicate_evidence or already_approved. The helper `approve()`
  draws a random source URL index in 0 to 89999, so two draws can collide. It did not
  recur in any later run, including every run for this revision. It is an existing flake
  and was left alone.