Skip to content

feat(breg): accept an optional reason on approve_request and apply_request - #1112

Merged
jeremi merged 3 commits into
mainfrom
feat/breg-approve-apply-reason
Sep 17, 2026
Merged

jeremi merged 3 commits into
mainfrom
feat/breg-approve-apply-reason

Conversation

@jeremi

@jeremi jeremi commented Sep 16, 2026

Copy link
Copy Markdown
Member

Closes #1109.

What changes

A favourable decision had nowhere to put remarks: approve_request and apply_request refused a reason, so remarks could only be captured on rejection or return. Both actions now accept the same optional, bounded reason (≤ 4096 Unicode characters, NUL refused, empty string allowed) that reject_request and request_revision already accept. A body without reason stays valid, so nothing changes for callers that send none.

  • Server (registry-breg): RequestActionBody::Approve/Apply carry reason: Option<String>, parsed by parse_reasoned_proposal_action. The approve reason is recorded on the decision entry exactly as a rejection's is. The apply reason is recorded on the application receipt (ApplicationReceipt gains reason/reason_present, registry_request_applications gains the columns) — apply is an execution step with no decision entry, and an application only ever exists for the current proposal version, so data.request.application is the complete read surface for it.
  • Disclosure and retention parity: data.request.application carries reasonPresent always and reason under the same readableRequestFields: [reason] rule; anonymous profiles never see text. Request-detail erasure removes apply-reason text while keeping presence, and the retention report gains applicationReasons beside decisionReasons.
  • Idempotency (issue point 3, unchanged rule): the reason joins the canonical action binding, so replaying under one key must repeat the same text; a changed reason is a conflict. Lifecycle webhook events carry reasonPresent/reason on approve and apply transitions — including an approve whose proposal applies in the same transaction (emitted on the terminal applied state).
  • Client (registry-breg-client + Node/Python bindings): with_reason accepts approve and apply; recovery (recovery_action_body) replays a reasoned body verbatim; the retained/erased application decoders expose reason_present/reason. The byte-exact metadata gate now expects the reason property on all four action schemas the server publishes — same-release clients and servers stay in lockstep, and an older client refuses newer metadata (fail closed) rather than guessing.
  • Casework: the BReg adapter forwards reasons on every promoted decision and apply instead of answering SourceAdapterError::ReasonUnsupported. The problem code stays registered for source adapters that do refuse reasons; its published row and products/casework/README.md no longer claim BReg refuses it.
  • Generated artifacts: the four change-request OpenAPI baselines regenerated with their documented generators; check-generated.sh matches all nine baselines.
  • Docs: breg-api.mdx action section, the Casework decision explanation, the review-breg-changes-in-casework tutorial (records a remark on the favourable decision), and the client READMEs.

Decisions on the issue's open questions

  1. Apply in scope? Yes — implemented as proposed (title and numbered proposal both include it). It is recorded on the application receipt rather than a decision entry, since apply creates no decision; that is the smallest mechanism with the same disclosure and erasure semantics.
  2. review.stages[].requireReason? Not added — optional covers the stated institutional need; required can follow a concrete ask.
  3. Field name? Kept reason on the wire; the docs describe it as the approver's/applier's recorded remark, not only a refusal explanation.

Security-sensitive notes

This touches disclosure and data minimization: reason text on favourable decisions is guarded by the existing readableRequestFields profile rule (presence always, text only when permitted, never for anonymous profiles) and erased by operator_erase retention with presence retained — verified by new assertions for the hidden-profile and erasure paths. The DB CHECK constraints enforce the bounds and the reason/presence pairing in both tables. No new audit surface: the reason was already covered for reject/return; approve/apply reuse it.

Verification

  • cargo fmt --check; cargo clippy --all-targets -D warnings for the changed crates; full-workspace cargo check --all-targets.
  • cargo test for registry-breg, registry-breg-client, registry-casework-breg, plus registry-casework*, registry-bregctl — all pass.
  • Real PostgreSQL (PostGIS, CI-pinned image): postgres_change_requests (including reworked reviewer-reason journeys: reasoned approve decision read-back, reasoned apply with disclosure/hidden/erasure checks and idempotent replay, event payloads), postgres_request_events, postgres_request_read_retention, postgres_request_upgrade_retention, postgres_request_receipts, postgres_mutation, postgres_webhook_outbox — all pass. The DB CHECK-constraint change is exercised by these suites (an in-place upgrade re-adds the constraint at startup).
  • Node binding: npm ci && npm run build:debug && npm test && npm run check:types (33 pass). Python binding: extension-module build + 27 unittests pass.
  • Product checks: products/breg/scripts/check-contracts.sh, check-client-contract.sh, check-generated.sh (9/9 baselines match), products/casework/scripts/check-checkpoint.sh.
  • Docs site: npm run generate, npm test (601 pass), npm run check.

A favourable decision had nowhere to put remarks: approve_request and
apply_request refused a reason, so a reviewer could only record text on
rejection or return. Approve and apply bodies now accept the same
optional bounded reason reject and request-revision already accept.

The approve reason is recorded on the decision entry exactly as a
rejection's is. The apply reason is recorded on the application
receipt, since apply is an execution step with no decision entry: it is
disclosed in data.request.application under the same
readableRequestFields reason rule, with reasonPresent always present,
and request-detail erasure removes the text while keeping presence. The
retention report gains an applicationReasons counter beside
decisionReasons.

The reason joins the canonical idempotency binding, so replay under one
key must repeat the same text. Lifecycle webhook events carry
reasonPresent and the reason on approve and apply transitions, and an
approve that applies in the same transaction keeps its reason on the
terminal applied event.

BRegLifecycleAction::with_reason accepts approve and apply, recovery
replays a reasoned body verbatim, and the Casework BReg adapter
forwards reasons instead of answering ReasonUnsupported; the problem
code remains registered for sources that do refuse one. The strict
client metadata gate now expects the reason property on all four
action schemas the server publishes, so clients and servers of one
release stay in lockstep and an older client refuses newer metadata
rather than guessing.

Closes #1109.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Allow a reason on an approve that leaves the request submitted. The request
lifecycle outbox projection guard accepted an approve reason only on
("approve", "approved"), so a reasoned approve at a non-final stage, or one
that did not complete its stage quorum, failed the whole action with HTTP 400
request.invalid.

Carry the applier's reason presence into erased terminal request metadata.
After operator_erase on an applied request,
data.request.application.reasonPresent came back null instead of true, so
erasure dropped the presence flag the retention rule preserves.

Cover both through the real PostgreSQL HTTP router, including the first
coverage for RequestDetailErasure::application_reasons.

Correct the decisions CHECK-constraint test, which still asserted the database
refuses an approve carrying a reason after this branch widened the constraint.

Update the change-request examples, the events-and-webhooks reference, and the
API action table, which still named only reject and request_revision.

Security review notes (data minimization): the applier reason keeps the same
disclosure gate as a decision reason. may_disclose_decision_reasons still
governs the text, reasonPresent is disclosed unconditionally, and erasure nulls
the retained text while leaving the flag set. The idempotency binding hashes
the reason into canonical_request_digest, so no reason text is stored in
cleartext there.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
@jeremi
jeremi merged commit 4c5a4f4 into main Sep 17, 2026
48 of 82 checks passed
@jeremi
jeremi deleted the feat/breg-approve-apply-reason branch September 17, 2026 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BReg: accept an optional reason on approve_request and apply_request

1 participant