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.