diff --git a/agents/rocko/work/setspark-approvers-review-2026-09-26.md b/agents/rocko/work/setspark-approvers-review-2026-09-26.md new file mode 100644 index 00000000..ef25a58b --- /dev/null +++ b/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. diff --git a/agents/rocko/work/setspark-approvers-review-r2-2026-09-26.md b/agents/rocko/work/setspark-approvers-review-r2-2026-09-26.md new file mode 100644 index 00000000..251b4adb --- /dev/null +++ b/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. diff --git a/agents/sage/work/setspark-approvers/NOTES.md b/agents/sage/work/setspark-approvers/NOTES.md new file mode 100644 index 00000000..2aade7d6 --- /dev/null +++ b/agents/sage/work/setspark-approvers/NOTES.md @@ -0,0 +1,362 @@ +# 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:` + 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:` matched with the existing `DISCORD_ID_RE` (fullmatch), or + `pending:` 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= /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. diff --git a/agents/sage/work/setspark-approvers/fix.patch b/agents/sage/work/setspark-approvers/fix.patch new file mode 100644 index 00000000..60e8a8e6 --- /dev/null +++ b/agents/sage/work/setspark-approvers/fix.patch @@ -0,0 +1,416 @@ +diff --git a/stack/api/README.md b/stack/api/README.md +index 475fb5f..d23ba36 100644 +--- a/stack/api/README.md ++++ b/stack/api/README.md +@@ -74,7 +74,7 @@ verified key id; it is never used for authorization. + + | Verb | REST | MCP tool | Notes | + |---|---|---|---| +-| create | `POST /v1/records` `{record_type, record}` | `create` | Allocates the next ID from `counters`; work items default Open/Later; decisions start Proposed, version 1 | ++| create | `POST /v1/records` `{record_type, record}` | `create` | Allocates the next ID from `counters`; work items default Open/Later; decisions start Proposed, version 1. Decisions must satisfy the required approvers rule below | + | update | `PATCH /v1/records/{id}` `{revision, fields}` | `update` | Revision CAS. Mismatch: 409 with `current_revision` and `changed_fields` (from audit). Digest-covered edits to a Proposed decision bump `proposal_version` | + | import | `POST /v1/import` | `import` | Privileged. Keeps the source `id`, moves the counter past it, skips link existence checks unless `check_links` is true. Accepted/Superseded sources must carry reproducible approvals | + | resolve_links | `POST /v1/import/resolve-links` `{items: [{record_type, id, links}]}` | `resolve_links` | Migration link pass, only for records still marked `import_pending` (set by `import`, cleared by `finalize_import`); anything else is `links_locked`. Sets `business`, `project`, `work_item`, `supersedes`, `superseded_by`, `related` in one transaction, verifying targets exist and cardinality (`related` is a list of ids, every other link a single id), then type-checks the resulting record, without bumping `revision`. One bad link fails the batch. Decisions with any approval, an `accepted_snapshot`, or status Accepted/Superseded are refused (`links_locked`). A link change on a Proposed decision recomputes `proposal_digest` (no version bump: no approval can exist yet) | +@@ -83,11 +83,11 @@ verified key id; it is never used for authorization. + | list | `GET /v1/records?record_type=..&=..` (`type=` also accepted) | `list` | Equality filters on column-backed properties, `limit` (max 500), `offset` | + | resolve_id | `GET /v1/resolve?q=` | `resolve_id` | Exact ID or title substring | + | get_counters | `GET /v1/counters` | `get_counters` | Read verb: `{counters: {prefix: next}}` for every ID prefix, for the migration verify step against the frozen registry | +-| open_approval_request | `POST /v1/approval-requests` `{decision_id, proposal_version, proposal_digest}` | same | Model verb, before any Discord message exists. Version and digest must be current (else `request_stale`). Returns `request_id`, `required_approvers` as Discord user ids, `state: open` | ++| open_approval_request | `POST /v1/approval-requests` `{decision_id, proposal_version, proposal_digest}` | same | Model verb, before any Discord message exists. Version and digest must be current (else `request_stale`). Refused with `approvers_not_ready` unless the stored list is ready: 1 to 16 distinct `discord:` entries (replace pending markers or legacy entries through `update` first). Returns `request_id`, `required_approvers` as Discord user ids, `state: open` | + | bind_approval_message | `POST /v1/approval-requests/{request_id}/message` `{message_id, channel_id}` | same | Connector only (sage). Key `::bind`; replay returns the same result; `message_id` unique; rebinding is `already_bound` | +-| add_approval | `POST /v1/approvals` `{request_id, kind, author_id, message_id, source_url, statement, bound_message_id?, at?}` | same | Connector only (sage). Key `::approval`. `kind` is `button` or `reply`. A button event must have `message_id` equal to the bound message (`bound_message_id` is ignored). A reply event must carry its own `message_id`, `bound_message_id` equal to the bound message, and a `statement` that trimmed and case-folded is exactly `approve` (`not_affirmative`). Acceptance also verifies the decision's supersession links: it must not carry `superseded_by`, and any `supersedes` must point at an Accepted decision not superseded by anyone else (`invalid_supersession`). Checks in order: request open (`not_open`); decision still Proposed at the request's version and digest, otherwise the request is closed and `request_stale` returned; author in required approvers (`not_approver`); not already approved (`already_approved`); `message_id` is the bound message or `bound_message_id` equals it (`message_mismatch`); `source_url` unique. Acceptance, `accepted_snapshot`, request `state: approved` and reciprocal supersession all commit with the completing approval | ++| add_approval | `POST /v1/approvals` `{request_id, kind, author_id, message_id, source_url, statement, bound_message_id?, at?}` | same | Connector only (sage). Key `::approval`. `kind` is `button` or `reply`. A button event must have `message_id` equal to the bound message (`bound_message_id` is ignored). A reply event must carry its own `message_id`, `bound_message_id` equal to the bound message, and a `statement` that trimmed and case-folded is exactly `approve` (`not_affirmative`). Acceptance also verifies the decision's supersession links: it must not carry `superseded_by`, and any `supersedes` must point at an Accepted decision not superseded by anyone else (`invalid_supersession`). Checks in order: request open (`not_open`); decision still Proposed at the request's version and digest, otherwise the request is closed and `request_stale` returned; both the request's stored approvers and the decision's are ready (`approvers_not_ready`, nothing written); author in required approvers (`not_approver`); not already approved (`already_approved`); `message_id` is the bound message or `bound_message_id` equals it (`message_mismatch`); `source_url` unique. Acceptance, `accepted_snapshot`, request `state: approved` and reciprocal supersession all commit with the completing approval | + | get_approval_request | `GET /v1/approval-requests/{request_id}` | same | `state` open/approved/closed, bound `channel_id`/`message_id`, `approvals` (approver, at, message_id, source_url) | +-| supersede | `POST /v1/supersede` `{decision_id, successor_id}` | same | Successor must be Accepted and created with `supersedes = decision_id`; marks the old decision Superseded with `superseded_by`; acyclic. The predecessor must be Accepted (`predecessor_not_accepted`), checked both when the successor is created with `supersedes` and again when it is accepted | ++| supersede | `POST /v1/supersede` `{decision_id, successor_id}` | same | Successor must be Accepted and created with `supersedes = decision_id`; marks the old decision Superseded with `superseded_by`; acyclic. An old decision whose stored approvers break the rule for Superseded is refused (`invalid_record`), here and when a successor's acceptance would supersede it. The predecessor must be Accepted (`predecessor_not_accepted`), checked both when the successor is created with `supersedes` and again when it is accepted | + | create_document | `POST /v1/documents` `{collection, title, text, source?}` | same | Outline, allowlisted collections only. `source` must be one non-empty line: any character `str.splitlines` splits on is refused (`invalid_source`). The `documents.search` reconciliation is paginated to exhaustion before a document is considered absent; a failing page raises `outline_error` and keeps the pending row. One connection holds a per-key session advisory lock across the pending row, the search by `Source:` header line, the create and the receipt, so concurrent calls with the same key serialise and never both reach Outline; replay reconciles instead of creating twice | + + Records are exchanged in frontmatter shape (`title`, `business`, `work_item`, `proposal_body`, +@@ -106,6 +106,73 @@ acyclicity, referenced records must exist (create/update). The database adds ins + triggers on `approvals` and `audit`, and a trigger that refuses any change to digest-covered + columns or the snapshot of an Accepted or Superseded decision. + ++### Required approvers ++ ++A decision's `required_approvers` is a list of 1 to 16 distinct strings. Each entry is either ++`discord:` (17 to 22 ASCII digits, the same pattern as `tools/validate_vault.py`) or ++`pending:` whose text is not empty after Python `str.strip()`. Pending markers follow the ++records convention as the validator applies it: allowed on Proposed and Rejected decisions, ++refused on Accepted and Superseded ones. One helper (`service.check_required_approvers`) ++enforces this on create, update, import and finalize_import. Update checks the merged record, ++so an update to a stored decision that breaks the rule is refused until the same update also ++corrects the list. A failure is 422 `invalid_record`; the message states the rule and never ++echoes the value. ++ ++Stored rows are rechecked wherever approvals are collected or a decision is sealed, because rows ++and approval requests written before this rule can hold any list. `open_approval_request` and ++`add_approval` refuse with `approvers_not_ready` unless the list is ready: the rule in its ++Accepted form, so 1 to 16 distinct Discord ids and no pending markers. `add_approval` checks both ++the request's copy and the decision's list before writing anything, which also guards the seal ++to Accepted. Superseding a decision (`supersede`, or the acceptance of a successor that ++`supersedes` it) is refused with `invalid_record` when the old decision's stored list breaks the ++rule for Superseded. ++ ++Rows written before this rule may break it; DEC-009's bare names are the known case. No ++migration rewrites them. A Proposed or Rejected row is corrected through `update`. An Accepted ++row is frozen by the immutability trigger, so one with a broken list cannot be corrected through ++the API and cannot be superseded; the first query lists any such row for an owner decision. ++These read-only queries list the affected rows: ++ ++```sql ++-- decisions whose required_approvers break the rule ++SELECT d.id, d.status, d.proposal_version ++FROM setspark.decisions d ++WHERE CASE ++ WHEN jsonb_typeof(d.required_approvers) <> 'array' THEN true ++ WHEN jsonb_array_length(d.required_approvers) NOT BETWEEN 1 AND 16 THEN true ++ ELSE EXISTS (SELECT 1 FROM jsonb_array_elements(d.required_approvers) AS e(v) ++ WHERE jsonb_typeof(e.v) <> 'string' ++ OR NOT ((e.v #>> '{}') ~ '^discord:[0-9]{17,22}$' ++ OR (d.status NOT IN ('Accepted', 'Superseded') ++ AND (e.v #>> '{}') ~ '^pending:.*[^\u0009-\u000d\u001c-\u0020\u0085\u00a0\u1680\u2000-\u200a\u2028\u2029\u202f\u205f\u3000]'))) ++ OR (SELECT count(DISTINCT e.v) FROM jsonb_array_elements(d.required_approvers) AS e(v)) ++ <> jsonb_array_length(d.required_approvers) ++ END ++ORDER BY d.id; ++ ++-- open approval requests whose approvers are not ready (not 1 to 16 distinct Discord ids) ++SELECT r.id, r.decision_id, r.proposal_version ++FROM setspark.approval_requests r ++WHERE r.state = 'open' ++ AND CASE ++ WHEN jsonb_typeof(r.required_approvers) <> 'array' THEN true ++ WHEN jsonb_array_length(r.required_approvers) NOT BETWEEN 1 AND 16 THEN true ++ ELSE EXISTS (SELECT 1 FROM jsonb_array_elements(r.required_approvers) AS e(v) ++ WHERE jsonb_typeof(e.v) <> 'string' ++ OR (e.v #>> '{}') !~ '^discord:[0-9]{17,22}$') ++ OR (SELECT count(DISTINCT e.v) FROM jsonb_array_elements(r.required_approvers) AS e(v)) ++ <> jsonb_array_length(r.required_approvers) ++ END ++ORDER BY r.id; ++``` ++ ++The pending test in the first query spells out the characters Python `str.strip()` removes ++instead of using `[:space:]`, whose meaning depends on the database locale. The second query ++lists every open request that `add_approval` now refuses, which includes requests holding ++pending markers, entries of neither form, duplicates, and more than 16 entries. Correcting the ++decision's approvers through `update` bumps `proposal_version` (the list is digest-covered), so ++the next `add_approval` against the old request closes it with `request_stale`. ++ + Digest: SHA-256 over canonical JSON `{properties, body}` (sorted keys, compact separators, + Unicode preserved), properties excluding `status`, `approvals`, `updated`, `superseded_by` and + every database-added column; body is the stripped Markdown. `setspark_api/digest.py` is a copy +@@ -149,6 +216,7 @@ Every failure body is exactly `{code, message}`; 409 `stale_revision` adds `curr + | 422 | idempotency_mismatch | Same key, different request | + | 422 | invalid_record, invalid_link, invalid_filter, invalid_source | Validation. Records are type-checked (pydantic, per record type, unknown properties allowed) before any SQL; messages name fields and error kinds only | + | 422 | import_pending | open_approval_request or add_approval on a decision whose import is not finalized | ++| 422 | approvers_not_ready | open_approval_request or add_approval while the stored required approvers (the decision's, or the request's copy) are not 1 to 16 distinct `discord:` entries, for example a pending marker or a list stored before the rule; nothing is written and no value is echoed | + | 422 | reserved_property | A caller supplied a service-owned flag (`import_pending`) in a record or update fields | + | 422 | links_locked | resolve_links on a record that is not `import_pending`, or on a decision that already has approval evidence | + | 422 | invalid_supersession | At acceptance: the decision carries `superseded_by`, or `supersedes` is not an Accepted decision free to be superseded. At finalize_import: a supersession link target is missing, not reciprocal, of the wrong status, or forms a cycle; the record stays `import_pending` | +diff --git a/stack/api/setspark_api/service.py b/stack/api/setspark_api/service.py +index 5fc1152..9d8aacd 100644 +--- a/stack/api/setspark_api/service.py ++++ b/stack/api/setspark_api/service.py +@@ -153,6 +153,50 @@ def evidence(v): + return isinstance(v, list) and bool(v) and all(nonempty(x) for x in v) + + ++APPROVERS_MAX = 16 ++PENDING_PREFIX = 'pending:' ++SEALED_STATUS = ('Accepted', 'Superseded') ++APPROVERS_RULE = ('required_approvers must be a list of 1 to %d distinct entries, each discord: ' ++ '(17 to 22 ASCII digits) or pending:' % APPROVERS_MAX) ++ ++ ++def is_discord_approver(v): ++ return isinstance(v, str) and DISCORD_ID_RE.fullmatch(v) is not None ++ ++ ++def is_pending_approver(v): ++ return isinstance(v, str) and v.startswith(PENDING_PREFIX) and nonempty(v[len(PENDING_PREFIX):]) ++ ++ ++def check_required_approvers(approvers, status): ++ """The single required_approvers rule, applied wherever a decision is validated (create, update, import, ++ finalize_import). Pending markers follow tools/validate_vault.py: allowed until the decision is Accepted or ++ Superseded, which need stable Discord IDs. Messages state the rule and never echo the submitted values.""" ++ if not isinstance(approvers, list) or not 1 <= len(approvers) <= APPROVERS_MAX: ++ raise ApiError(422, 'invalid_record', APPROVERS_RULE) ++ if not all(is_discord_approver(x) or is_pending_approver(x) for x in approvers): ++ raise ApiError(422, 'invalid_record', APPROVERS_RULE) ++ if len(set(approvers)) != len(approvers): ++ raise ApiError(422, 'invalid_record', 'required_approvers must be distinct') ++ if status in SEALED_STATUS and not all(is_discord_approver(x) for x in approvers): ++ raise ApiError(422, 'invalid_record', 'Accepted and Superseded decisions require discord: approvers, not pending markers') ++ ++ ++def approvers_ready(approvers): ++ """True when a stored list may collect approvals or be sealed: the full rule in its Accepted/Superseded form (1 to 16 ++ distinct discord: entries, no pending markers). Stored rows are rechecked because rows written before ++ this rule, and approval requests copied from them, can hold any list.""" ++ try: ++ check_required_approvers(approvers, SEALED_STATUS[0]) ++ except ApiError: ++ return False ++ return True ++ ++ ++APPROVERS_NOT_READY = ('Every required approver must be a distinct discord:, 1 to %d of them, before approvals can be ' ++ 'collected; correct the decision through update first' % APPROVERS_MAX) ++ ++ + def jsonable(value): + return json.loads(json.dumps(value, default=str)) + +@@ -350,8 +394,7 @@ class Service: + raise ApiError(422, 'invalid_record', 'status must be Proposed, Accepted, Rejected or Superseded') + if not nonempty(rec.get('work_item')): + raise ApiError(422, 'invalid_record', 'Decision requires originating work_item') +- if not evidence(rec.get('required_approvers')) or len(set(rec['required_approvers'])) != len(rec['required_approvers']): +- raise ApiError(422, 'invalid_record', 'Decision requires distinct, explicit required_approvers') ++ check_required_approvers(rec.get('required_approvers'), rec['status']) + pv = rec.get('proposal_version', 1) + if type(pv) is not int or pv < 1: + raise ApiError(422, 'invalid_record', 'proposal_version must be a positive integer') +@@ -585,6 +628,8 @@ class Service: + raise ApiError(422, 'import_pending', '%s is an unfinished import; finalize_import first' % row['id']) + if payload.get('proposal_version') != row['proposal_version'] or payload.get('proposal_digest') != row['proposal_digest']: + raise ApiError(422, 'request_stale', 'The decision is at version %d with a different digest; get it again' % row['proposal_version']) ++ if not approvers_ready(row['required_approvers']): ++ raise ApiError(422, 'approvers_not_ready', APPROVERS_NOT_READY) + req = conn.execute('''INSERT INTO approval_requests (decision_id, proposal_version, proposal_digest, required_approvers) + VALUES (%s, %s, %s, %s) RETURNING *''', (row['id'], row['proposal_version'], row['proposal_digest'], Jsonb(row['required_approvers']))).fetchone() + out = self._request_view(conn, req) +@@ -642,6 +687,10 @@ class Service: + raise ApiError(422, 'request_stale', 'Request %d was for version %d; the proposal is now version %d and %s. Request closed.' % (req['id'], req['proposal_version'], row['proposal_version'], row['status']), commit=close) + if row['import_pending']: + raise ApiError(422, 'import_pending', '%s is an unfinished import; finalize_import first' % row['id']) ++ # Before any write: this guards the approval row and the seal to Accepted below. The request's copy and the ++ # decision's list are both checked, since either may predate the rule. ++ if not approvers_ready(req['required_approvers']) or not approvers_ready(row['required_approvers']): ++ raise ApiError(422, 'approvers_not_ready', APPROVERS_NOT_READY) + if approver not in req['required_approvers']: + raise ApiError(422, 'not_approver', 'Author is not one of the required approvers') + if conn.execute('SELECT 1 FROM approvals WHERE request_id = %s AND approver = %s', (req['id'], approver)).fetchone(): +@@ -821,6 +870,9 @@ class Service: + cursor = nxt['supersedes_id'] if nxt else None + if old['status'] == 'Superseded' and old['superseded_by_id'] == new_id: + return old ++ if not approvers_ready(old['required_approvers']): # an Accepted row sealed before the rule; the trigger keeps it as is ++ raise ApiError(422, 'invalid_record', '%s has required_approvers that break the rule for a Superseded decision, so it cannot ' ++ 'be superseded' % old_id) + before = self._to_record('decision', old) + newold = self._write(conn, 'decision', old_id, {'status': 'Superseded', 'superseded_by_id': new_id}) + self._audit(conn, auth, payload, key, 'decision', old_id, newold['revision'], 'supersede', before, self._to_record('decision', newold)) +diff --git a/tools/tests/test_setspark_api.py b/tools/tests/test_setspark_api.py +index 0d5e445..4f1e134 100644 +--- a/tools/tests/test_setspark_api.py ++++ b/tools/tests/test_setspark_api.py +@@ -895,6 +895,206 @@ class ServiceTests(unittest.TestCase): + self.finalize(pid) + self.assertNotIn('import_pending', self.svc.execute(self.coord, 'get', {'id': pid})[1]) + ++ # ----- required_approvers rule ------------------------------------------------ ++ def assert_invalid_without_echo(self, cm, approvers, code='invalid_record'): ++ self.assertEqual((cm.exception.status, cm.exception.code), (422, code)) ++ shown = (cm.exception.message, json.dumps(cm.exception.body(), ensure_ascii=False), repr(cm.exception), str(cm.exception)) ++ for v in approvers: ++ token = v.split(':', 1)[-1].strip() if isinstance(v, str) else '' ++ if token: ++ for text in shown: ++ self.assertNotIn(token, text) ++ ++ def far_decision_id(self): ++ """An unused DEC id well above the counter, so a later create cannot collide with it.""" ++ return 'DEC-%d' % (self.sql("SELECT next FROM counters WHERE prefix = 'DEC'")[0]['next'] + 5000) ++ ++ def test_approvers_create_refuses_every_malformed_form_without_echo(self): ++ for label, approvers in BAD_APPROVERS: ++ with self.subTest(case=label): ++ with self.assertRaises(ApiError) as cm: ++ self.svc.execute(self.coord, 'create', {'record_type': 'decision', 'record': { ++ 'title': 'Bad approvers', 'work_item': self.work, 'required_approvers': approvers, 'proposal_body': 'x'}}, self.key()) ++ self.assert_invalid_without_echo(cm, approvers) ++ ++ def test_approvers_create_accepts_ids_pending_markers_and_bounds(self): ++ for label, approvers in (('one id', [JASON]), ('17 and 22 digit ids', [_discord(17), _discord(22)]), ++ ('pending marker', ['pending:Fixture approver A', JASON]), ('16 entries', [_fake_approver(i) for i in range(16)])): ++ with self.subTest(case=label): ++ self.assertEqual(self.decision(approvers=approvers)['required_approvers'], approvers) ++ ++ def test_approvers_update_refused_the_same_way(self): ++ for label, approvers in BAD_APPROVERS: ++ with self.subTest(case=label): ++ dec = self.decision() # one decision per case, so each case is judged on its own ++ with self.assertRaises(ApiError) as cm: ++ self.svc.execute(self.coord, 'update', {'id': dec['id'], 'revision': dec['revision'], 'fields': {'required_approvers': approvers}}, self.key()) ++ self.assert_invalid_without_echo(cm, approvers) ++ _, now = self.svc.execute(self.coord, 'get', {'id': dec['id']}) ++ self.assertEqual((now['revision'], now['required_approvers']), (dec['revision'], [JASON, CARMEN])) ++ ok = self.decision() ++ _, upd = self.svc.execute(self.coord, 'update', {'id': ok['id'], 'revision': ok['revision'], 'fields': {'required_approvers': [JASON, 'pending:Fixture approver B']}}, self.key()) ++ self.assertEqual(upd['proposal_version'], 2) ++ ++ def test_approvers_import_refused_the_same_way(self): ++ for label, approvers in BAD_APPROVERS: ++ with self.subTest(case=label): ++ rid = self.far_decision_id() # unused id per case, so each case is judged on its own ++ with self.assertRaises(ApiError) as cm: ++ self.import_decision(rid, 'Proposed', approvers=approvers) ++ self.assert_invalid_without_echo(cm, approvers) ++ with self.assertRaises(ApiError) as cm: ++ self.svc.execute(self.coord, 'get', {'id': rid}) ++ self.assertEqual(cm.exception.code, 'not_found') ++ with self.assertRaises(ApiError) as cm: # a pending marker cannot be sealed ++ self.import_decision(self.far_decision_id(), 'Accepted', approvers=[JASON, 'pending:Fixture approver B']) ++ self.assert_invalid_without_echo(cm, ['pending:Fixture approver B']) ++ ++ def test_approvers_pending_markers_follow_the_validator_status_rule(self): ++ mixed = [JASON, 'pending:Fixture approver B'] ++ for status in ('Proposed', 'Rejected'): ++ service_mod.check_required_approvers(mixed, status) ++ for status in ('Accepted', 'Superseded'): ++ with self.subTest(status=status): ++ with self.assertRaises(ApiError) as cm: ++ service_mod.check_required_approvers(mixed, status) ++ self.assert_invalid_without_echo(cm, mixed) ++ service_mod.check_required_approvers([JASON, CARMEN], status) ++ ++ def test_approvers_open_request_refused_until_every_approver_is_a_discord_id(self): ++ dec = self.decision(approvers=[JASON, 'pending:Fixture approver B']) ++ with self.assertRaises(ApiError) as cm: ++ self.svc.execute(self.sage, 'open_approval_request', {'decision_id': dec['id'], 'proposal_version': dec['proposal_version'], 'proposal_digest': dec['proposal_digest']}, self.key()) ++ self.assert_invalid_without_echo(cm, [JASON, 'pending:Fixture approver B'], code='approvers_not_ready') ++ self.assertEqual(self.sql('SELECT count(*) AS n FROM approval_requests WHERE decision_id = %s', dec['id'])[0]['n'], 0) ++ self.svc.execute(self.coord, 'update', {'id': dec['id'], 'revision': dec['revision'], 'fields': {'required_approvers': [JASON, CARMEN]}}, self.key()) ++ self.assertEqual(self.open_request(dec['id'])['required_approvers'], [JASON_ID, CARMEN_ID]) ++ ++ def test_approvers_migration_shaped_import_with_pending_markers_still_finalizes(self): ++ from validate_vault import parse ++ pending = ['pending:Fixture approver A verified approval', 'pending:Fixture approver B verified identity and approval'] ++ rid = self.far_decision_id() ++ dec = self.import_decision(rid, 'Proposed', approvers=pending, approvals=[]) # vault frontmatter shape, as ApiSink sends it ++ self.assertEqual((dec['required_approvers'], dec['import_pending']), (pending, True)) ++ self.assertEqual(self.finalize(rid)['items'][0]['import_pending'], False) ++ with self.assertRaises(ApiError) as cm: ++ self.svc.execute(self.sage, 'open_approval_request', {'decision_id': rid, 'proposal_version': 1, 'proposal_digest': dec['proposal_digest']}, self.key()) ++ self.assertEqual(cm.exception.code, 'approvers_not_ready') ++ for path in sorted((ROOT / 'vault' / 'Decisions').glob('DEC-*.md')): # every current vault decision satisfies the rule ++ meta, _ = parse(path.read_text(encoding='utf-8')) ++ service_mod.check_required_approvers(meta['required_approvers'], meta['status']) ++ ++ def test_approvers_stored_legacy_row_is_refused_at_finalize_update_and_open(self): ++ rid = self.far_decision_id() ++ dec = self.import_decision(rid, 'Proposed', approvers=['pending:Fixture approver A']) ++ legacy = ['fixture-name-a', 'fixture-name-b'] # the DEC-009 shape: bare names written before this rule ++ self.sql('UPDATE decisions SET required_approvers = %s::jsonb WHERE id = %s', json.dumps(legacy), rid) ++ with self.assertRaises(ApiError) as cm: ++ self.finalize(rid) ++ self.assert_invalid_without_echo(cm, legacy) ++ self.sql('UPDATE decisions SET import_pending = false WHERE id = %s', rid) ++ with self.assertRaises(ApiError) as cm: ++ self.svc.execute(self.sage, 'open_approval_request', {'decision_id': rid, 'proposal_version': 1, 'proposal_digest': dec['proposal_digest']}, self.key()) ++ self.assert_invalid_without_echo(cm, legacy, code='approvers_not_ready') ++ with self.assertRaises(ApiError) as cm: # an unrelated edit cannot carry the bad list forward ++ self.svc.execute(self.coord, 'update', {'id': rid, 'revision': 1, 'fields': {'title': 'Renamed legacy'}}, self.key()) ++ self.assert_invalid_without_echo(cm, legacy) ++ _, fixed = self.svc.execute(self.coord, 'update', {'id': rid, 'revision': 1, 'fields': {'required_approvers': [JASON, CARMEN]}}, self.key()) ++ self.assertEqual((fixed['required_approvers'], fixed['proposal_version']), ([JASON, CARMEN], 2)) ++ ++ ++ # ----- upgrade: rows stored before the approver rule ------------------------------------------------------- ++ def plant_legacy_decision(self, approvers, status='Proposed', **extra): ++ """A decision whose stored list predates the rule, as the base code could write it. Created valid, then rewritten ++ in the test database only, with the digest recomputed so the stored version and digest stay consistent.""" ++ from setspark_api.digest import decision_digest ++ dec = self.decision(**extra) ++ row = self.sql('UPDATE decisions SET required_approvers = %s::jsonb WHERE id = %s RETURNING *', json.dumps(approvers), dec['id'])[0] ++ self.sql('UPDATE decisions SET proposal_digest = %s, status = %s WHERE id = %s', decision_digest(row), status, dec['id']) ++ return self.svc.execute(self.coord, 'get', {'id': dec['id']})[1] ++ ++ def plant_legacy_request(self, dec): ++ """An open, bound approval request copied from a legacy decision, as the base open_approval_request wrote it.""" ++ row = self.sql('''INSERT INTO approval_requests (decision_id, proposal_version, proposal_digest, required_approvers, message_id, channel_id) ++ VALUES (%s, %s, %s, %s::jsonb, %s, %s) RETURNING *''', dec['id'], dec['proposal_version'], dec['proposal_digest'], ++ json.dumps(dec['required_approvers']), uuid.uuid4().hex, '200000000000000000')[0] ++ return {'request_id': row['id'], 'message_id': row['message_id']} ++ ++ def test_upgrade_legacy_decision_cannot_open_a_request(self): ++ for label, approvers in (('17 ids', [_fake_approver(i) for i in range(17)]), ('duplicate ids', [JASON, JASON]), ++ ('bare names', ['fixture-name-a', 'fixture-name-b']), ('pending marker', [JASON, 'pending:Fixture approver B'])): ++ with self.subTest(case=label): ++ dec = self.plant_legacy_decision(approvers) ++ with self.assertRaises(ApiError) as cm: ++ self.svc.execute(self.sage, 'open_approval_request', {'decision_id': dec['id'], 'proposal_version': dec['proposal_version'], ++ 'proposal_digest': dec['proposal_digest']}, self.key()) ++ self.assert_invalid_without_echo(cm, approvers, code='approvers_not_ready') ++ self.assertEqual(self.sql('SELECT count(*) AS n FROM approval_requests WHERE decision_id = %s', dec['id'])[0]['n'], 0) ++ ++ def test_upgrade_legacy_request_cannot_collect_approvals_or_seal(self): ++ for label, approvers in (('17 ids', [_fake_approver(i) for i in range(17)]), ('mixed pending and id', [JASON, 'pending:Fixture approver B'])): ++ with self.subTest(case=label): ++ dec = self.plant_legacy_decision(approvers) ++ req = self.plant_legacy_request(dec) ++ with self.assertRaises(ApiError) as cm: ++ self.approve(req, approvers[0]) ++ self.assert_invalid_without_echo(cm, approvers, code='approvers_not_ready') ++ self.assertEqual(self.sql('SELECT count(*) AS n FROM approvals WHERE decision_id = %s', dec['id'])[0]['n'], 0) ++ self.assertEqual(self.sql('SELECT state FROM approval_requests WHERE id = %s', req['request_id'])[0]['state'], 'open') ++ self.assertEqual(self.svc.execute(self.coord, 'get', {'id': dec['id']})[1]['status'], 'Proposed') ++ # the way out: correcting the list bumps the version, so the old request goes stale and closes ++ _, fixed = self.svc.execute(self.coord, 'update', {'id': dec['id'], 'revision': dec['revision'], 'fields': {'required_approvers': [JASON, CARMEN]}}, self.key()) ++ self.assertEqual(fixed['proposal_version'], dec['proposal_version'] + 1) ++ with self.assertRaises(ApiError) as cm: ++ self.approve(req, JASON) ++ self.assertEqual(cm.exception.code, 'request_stale') ++ self.assertEqual(self.accept(dec['id'])['status'], 'Accepted') ++ ++ def test_upgrade_legacy_accepted_decision_cannot_be_superseded(self): ++ legacy = [_fake_approver(i) for i in range(17)] ++ old = self.plant_legacy_decision(legacy, status='Accepted') ++ succ = self.decision(supersedes=old['id']) ++ req = self.open_request(succ['id']) ++ self.approve(req, JASON) ++ with self.assertRaises(ApiError) as cm: # the completing approval would seal succ and supersede old ++ self.approve(req, CARMEN) ++ self.assert_invalid_without_echo(cm, legacy) ++ self.assertEqual(self.svc.execute(self.coord, 'get', {'id': succ['id']})[1]['status'], 'Proposed') ++ self.assertEqual(self.sql('SELECT count(*) AS n FROM approvals WHERE decision_id = %s', succ['id'])[0]['n'], 1) ++ # the supersede verb, with a successor that is already Accepted (planted, as a pre-rule acceptance could leave it) ++ self.sql("UPDATE decisions SET status = 'Accepted' WHERE id = %s", succ['id']) ++ with self.assertRaises(ApiError) as cm: ++ self.svc.execute(self.coord, 'supersede', {'decision_id': old['id'], 'successor_id': succ['id']}, self.key()) ++ self.assert_invalid_without_echo(cm, legacy) ++ now = self.svc.execute(self.coord, 'get', {'id': old['id']})[1] ++ self.assertEqual((now['status'], now.get('superseded_by'), now['required_approvers']), ('Accepted', None, legacy)) ++ ++ ++def _discord(digits): ++ """discord:, obviously fake: 1, zeros, 1.""" ++ return 'discord:1' + '0' * (digits - 2) + '1' ++ ++ ++def _fake_approver(i): ++ return 'discord:%d' % (100000000000000001 + i) ++ ++ ++BAD_APPROVERS = ( ++ ('bare name', ['fixture-name-a']), ++ ('bare id', ['100000000000000001']), ++ ('16 digits', [_discord(16)]), ++ ('23 digits', [_discord(23)]), ++ ('arabic-indic digits', ['discord:' + '\u0661' * 18]), ++ ('fullwidth digits', ['discord:' + '\uff11' * 18]), ++ ('trailing newline', [JASON + '\n']), ++ ('prefix case', ['Discord:100000000000000001']), ++ ('empty pending text', ['pending:']), ++ ('blank pending text', ['pending: ']), ++ ('duplicate', [JASON, JASON]), ++ ('zero entries', []), ++ ('17 entries', [_fake_approver(i) for i in range(17)]), ++) ++ + + class HttpTests(unittest.TestCase): + @classmethod diff --git a/agents/sage/work/setspark-approvers/manifest.sha256 b/agents/sage/work/setspark-approvers/manifest.sha256 new file mode 100644 index 00000000..7c51df22 --- /dev/null +++ b/agents/sage/work/setspark-approvers/manifest.sha256 @@ -0,0 +1,3 @@ +14d3efbe22ba3f36032d137181bbc525ccd56d8f957cb919d687d0eacce48cbc stack/api/README.md +66f76782b3448e32e69803982f4cb294f498afbee4d9495eeee81cff88482f26 stack/api/setspark_api/service.py +552cac8c1b90eade48188482be2931dd82ae1ab8140457195f99481968129356 tools/tests/test_setspark_api.py diff --git a/docs/SESSIONS.md b/docs/SESSIONS.md index d08de9b0..b5c15d46 100644 --- a/docs/SESSIONS.md +++ b/docs/SESSIONS.md @@ -412,3 +412,5 @@ are never rewritten or removed; corrections are new entries. 2026-09-26T23:13:44Z | Sage (T3 Claude Code, thread 1ef1e4f8) | SetSpark approver owner, CHAT-03 charter | Jason delegated both. SetSpark fix: Sage subagent in shared-signals, Rocko reviews, Sage commits; compatibility stop on pending markers resolved as lead decision 21. CHAT-03 brief assigned to Dewey (decision 22). A1/A2 split recorded as decision 20. QUEUE row 5 updated. 2026-09-26T23:25:08Z | Filbert (T3 Claude Code, thread 9cb9731e) | Queue A1 (#1508) code review, manifest 4319695a | changes requested: R1 matrix refusal tests (8.14), R2 review.issue null freeze, R3 evidence round; five Sage conditions met; review agents/filbert/work/queue-a1-review-2026-09-26.md 6933b885 2026-09-26T23:25:52Z | Sage (T3 Claude Code, thread 1ef1e4f8) | #1508 queue A1 review rulings | Filbert changes requested (6933b885), five conditions met. Ruled review issue (explicit --issue when several) and set piece/gate privileged only, lead decision 23; N13 to DEFERRED and Darkwing. +2026-09-26T23:37:23Z | Filbert (T3 Claude Code, thread 9cb9731e) | CHAT-03 brief R1 review (#1507 row 5), BRIEF.md 5dd447f7 | changes requested: B1 R3-1 premise wrong for Pi 0.85.1, B2 foreign-writer detection has no entry ids, B3 schema gaps (unavailable event, turnProof queue text), B4 no live-root refusal, B5 increment order and done gate, B6 claim two-key and reserved gaps; review agents/filbert/work/chat-03-brief-review-2026-09-26.md ec00544e +2026-09-26T23:52:04Z | Sage (T3 Claude Code, thread 1ef1e4f8) | SetSpark approver fix in shared-signals | Sage subagent wrote it, Rocko R1 revise (706e9ac1), R2 approve (c190814b); suites 188/131, vault 45 PASS; committed and pushed shared-signals cc74d92. Deploy to VM 1022 and production survey go to Jason. Lead decision 24. diff --git a/docs/plans/2026-09-26_lead-decisions.md b/docs/plans/2026-09-26_lead-decisions.md index 2c78af36..858bb6d9 100644 --- a/docs/plans/2026-09-26_lead-decisions.md +++ b/docs/plans/2026-09-26_lead-decisions.md @@ -279,3 +279,17 @@ which stay with him. Each item names who decided it and what happened. Filbert's N13 (the nested `node --test` in `test-foundation.sh` and `test-discord.sh`) is added to the DEFERRED entry. It goes to Darkwing as a small reviewed item after the A1 delta, before A2. +24. **SetSpark approver fix landed in shared-signals.** Rocko approved R2 + (41e735f4, review c190814b) after R1 found that rows stored before the + fix could still open requests, collect approvals, seal and be + superseded. Sage reran the suites on the checkout (188 with a test + database, 131 without, vault 45 PASS) and committed it to shared-signals + main as cc74d92 under the Sage identity, following that repo's + AGENTS.md. It was pushed. Correction to item 21's reasoning about + legacy Accepted rows with more than 16 approvers: I called them unlikely + because sealing one needed more than 16 live approvals. Rocko points out + that the old coordinator import could seal from supplied evidence, so + imported sealed rows need the same survey. The ruling stands: they stay + refused, and a one-off correction goes to Jason if the README query + finds any. Deploying to VM 1022 and running that query on production + are host actions for Jason. diff --git a/docs/plans/DEFERRED.md b/docs/plans/DEFERRED.md index 6d3d90b6..67bc617b 100644 --- a/docs/plans/DEFERRED.md +++ b/docs/plans/DEFERRED.md @@ -139,6 +139,10 @@ at every gate. Started 2026-09-12 during the control board MVP. The rule keeps the sanctioned `pending:` markers. The reasons and the DEC-009 disposition are in lead decisions item 21. Deploying to VM 1022 is still a gated host action. + 2026-09-26 23:50Z: fixed in source, shared-signals cc74d92 (Rocko + approved R2, lead decision 24). The SetSpark lead confirmed SetSpark + does not depend on free-text approvers. Still open: the deploy to VM 1022 + and running the README survey query on production. Both are Jason's. - **Index-export suite runs leave Docker networks behind.** Each export directory becomes a compose project, and its `_default` network stays after the run. On 2026-09-26 the host ran out of address pools. The