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