[mctl-agents]

Implements accepted proposal mctl-api/issue-700-governance-define-authority-and-decision.

Spec: https://github.com/mctlhq/mctl-gitops/tree/main/platform-gitops/agents-state/mctl-api/proposals/issue-700-governance-define-authority-and-decision/

Generated by mctl-agents Tier 2 implementer. Review carefully — this PR was opened automatically and the spec may have gaps.

Closes #700

[claude]

Claude finished @mctl-agents[bot]'s task in 8m 7s —— View job

---

Claude Code Review

• Read pinned diff (2632664...dbfc02c, 16 files, +1369/-70)
• Review internal/governance core logic
• Review API handlers + router wiring
• Review internal/workitems store changes
• Review migration, config, openapi, docs
• Build / vet / test — go build and go test were not permitted in this run, so nothing was compiled or executed. Findings below are from static review only.
• Post inline findings and verdict

Verdict: changes requested — 1 P1, 2 P2, 4 P3. Findings are posted as inline comments bound to dbfc02c.

P1 — pre-existing approval rows can never be approved

internal/workitems/action_approvals.go:145-149 adds risk_level, side_effect, intent_source, policy_decision, policy_reasons with empty defaults and no backfill. guardDecision (internal/api/handlers_approval_governance.go:88) then computes context_complete=false for every row that already exists and refuses approve with 422 approval_context_incomplete.

This also hits POST /api/v1/agents/dev-loop/{workflow_id}/approve: decideDevLoopApproval finds the pending mctl-agents-approve grant, so the noGrant legacy-payload fallback is never reached. Every in-flight human-approval gate is stuck until its TTL (up to 7 days) runs out, and there is no env var to relax the gate. The patch handles legacy rows carefully for fresh_until and decided_context_digest — the same treatment is missing for context completeness. A backfill of state='pending' rows from the registered policy / registry, or grandfathering rows created before the column, would close it.

P2 — completeness gate outranks the terminal-state refusals

guardDecision runs before DecideActionApproval, where the self-decision, expired and already-decided checks live. For a row with an incomplete context the answer is 422 approval_context_incomplete instead of 409 approval_expired / 409 approval_already_decided / 403 approval_self_decision, with no details.reapproval_required, and the documented invariant "the request stays pending" is false for those rows.

P2 — no handler-level test for revalidation

revalidateApproval adds five 409 codes plus a 503 to POST .../consume and to spendApprovalGrant. The internal/governance unit tests cover Revalidate in isolation, but nothing asserts the status/code mapping, details.reapproval_required, that the row stays approved after a refusal, or the revalidation audit param. The store's new fresh_until CAS predicate and the min(expires_at, now+FreshFor) clamp are also untested.

P3

• governance.go:30 — "the first six mean reapproval_required" contradicts reapproval: false for ErrContextIncomplete/ErrContextInvalid.
• governance.go:383 — skipped_legacy_mode is recorded even when CheckHumanPrincipal ran and passed; Result.PolicyStatus and LiveAuthority.Mode are set but never read.
• handlers_approval_governance.go:52 — devLoopPolicy silently yields an empty version for a missing rule, surfacing as 409 approval_intent_mismatch.
• handlers_action_approvals.go:102 / router.go:168 — audit risk override conflates action risk with operation risk; Options.Clock is unwired, leaving two clocks on the freshness path.

What looks right

Length-prefixed canonical digest (no field-separator ambiguity, and the policy_reasons regex excludes ,); fail-closed ErrAuthorityUnavailable on a missing or non-human-checking principal store; ordered revalidation with a test pinning the order; risk taken from the live registry so a client cannot downgrade a registered operation; fresh_until enforced both in the CAS predicate and as a post-hoc refusal; context kept to references, digests and enums with no args or free text; boot-time validation of ACTION_APPROVAL_MAX_DECISION_AGE; and both ConsumeActionApproval call sites revalidate.
· branch feat/agents-issue-700-governance-define-authority-and-decision

[claude]

P1 — The new context columns are added with DEFAULT /{} and never backfilled, so every approval row that already exists has side_effect=, intent_source=, policy_decision= and policy_reasons={}. guardDecision then computes context_complete=false for all of them and refuses approve with 422 approval_context_incomplete (internal/api/handlers_approval_governance.go:88).

Failure scenario: a DevLoop is waiting on a pending mctl-agents-approve grant created before this deploy. After the deploy a platform admin calls POST /api/v1/agents/dev-loop/{workflow_id}/approve; decideDevLoopApproval finds the pending grant, guardDecision returns 422 with missing_fields: [side_effect, intent_source, policy_decision, policy.reasons], and because the grant is pending the noGrant legacy-payload fallback is never reached. The grant can only be denied or left to expire, so every in-flight human-approval gate is stuck until its TTL (up to 7 days) runs out. There is no env var to relax the completeness gate.

Suggestion: backfill in the same migration block — for state=pending rows, set side_effect/policy_decision/policy_reasons from the registered policy for policy_rule_id and intent_source from coalesce(work_item_id, execution_id) — or grandfather rows whose created_at predates the column. Same class of back-compat handling the patch already does for fresh_until and decided_context_digest.

[claude]

P2 — guardDecision now runs before DecideActionApproval, which is where the self-decision, expired and already-decided checks live (internal/workitems/action_approvals.go:655-666). For any request with an incomplete context the completeness refusal therefore wins over the real reason.

Failure scenario: a pending approval created before this change (empty side_effect etc.) expires. An admin calls POST /api/v1/action-approvals/{id}/decision with approve. Before this PR the answer was 409 approval_expired with details.reapproval_required=true; now it is 422 approval_context_incomplete, which carries no reapproval_required, and both docs/decision-context-contract.md (quote: the request stays pending) and the openapi text are wrong for that row — it is expired, not pending. The same applies to an already approved/denied/consumed row (422 instead of 409 approval_already_decided) and to a requester self-decision (422 instead of 403 approval_self_decision), so a client branching on the code picks the wrong recovery.

Fix: gate on a.State == workitems.ApprovalPending (and a.RequestedBy != principalOf(user)) before the completeness check, so the terminal-state refusals keep precedence.

[claude]

P2 — No test exercises revalidateApproval through an HTTP handler, yet it is new refusal behaviour on POST /api/v1/action-approvals/{id}/consume and on spendApprovalGrant (handlers_approval_grant.go:191): five new 409 codes plus a 503, each of which must leave the approval unspent.

The internal/governance unit tests cover Revalidate in isolation, but nothing asserts the wiring: that approval_authority_revoked / approval_decider_disabled / approval_stale / approval_policy_drift come back as 409 with details.reapproval_required=true, that approval_authority_unavailable comes back as 503, that the row is still approved (not consumed) after a refusal, and that revalidation lands in the consume audit params. handlers_approval_governance_test.go only covers the two F6 decide paths.

Two store-level behaviours are untested too:

• the new CAS predicate AND (fresh_until IS NULL OR fresh_until > $3) in ConsumeActionApproval and the ErrApprovalStale fallback at action_approvals.go:728;
• the fresh_until = min(expires_at, now+FreshFor) clamp in DecideActionApproval — in particular that a FreshFor longer than the remaining TTL does not push fresh_until past expires_at.

The Options.Policies, Options.Clock and Options.ApprovalMaxDecisionAge injection points were added for exactly this, so these are cheap to write against workItemsEnv.

[claude]

P3 — This comment says "the first six mean the approval must be re-requested (reapproval_required)", but actionApprovalRefusals marks ErrContextIncomplete and ErrContextInvalid as reapproval: false (internal/api/handlers_action_approvals.go:114-115), which matches docs/decision-context-contract.md. Since the contract tells callers to key on details.reapproval_required rather than the code text, the comment should say "the four refusals from ErrContextChanged to ErrDeciderDisabled" (plus ErrPolicyDrift) instead of "the first six".

[claude]

P3 — In shadow/legacy mode with a recorded decided_by_principal_id, CheckHumanPrincipal has just run and passed, yet Revalidation is overwritten with skipped_legacy_mode. Only step 4 (ADMIN_PRINCIPALS membership) was skipped. The consume audit then records revalidation=skipped_legacy_mode for a spend where the decider was in fact re-verified as an active human, understating what was checked. Consider a distinct value (e.g. admin_check_skipped) so the audit distinguishes "nothing was re-checked" (line 365, no principal at all) from "the human check ran, membership did not".

Related, in the same function: Result.PolicyStatus is set but never read by any caller — revalidateApproval only forwards res.Revalidation — so a spend allowed under an unregistered policy rule (strict mode off) leaves no trace in the audit. And LiveAuthority.Mode (line 322) is assigned in revalidateApproval but never read anywhere; either audit it or drop the field.

[claude]

P3 — devLoopPolicy returns a silently empty version when the rule is absent from h.opts.Policies (map index on a missing key yields the zero Policy). The value feeds IntentHash in bindApprovalGrant, so a Policies set that omits dev-loop.human-approval produces a hash computed over policy_version="" and the grant spend fails with 409 approval_intent_mismatch — a confusing symptom for a configuration mistake. Falling back to governance.Policies[DevLoopPolicyRule].Version (or logging when the lookup misses) would fail loudly instead.

[claude]

P3 — Two small things in the audit changes:

1. The risk override makes risk_level on an action_approval.create entry the risk of the approved action rather than of the API operation that produced the entry (the call site passes RiskLow). Creating a pending request is a low-risk write whatever the action is; reporting it as high conflates the two and makes the audit risk column less useful for filtering. Consider recording the action risk under a separate param (e.g. action_risk_level) and leaving risk_level as the operation risk.

2. params["policy_version"] = a.PolicyVersion is unconditional while every neighbouring optional field is guarded by a non-empty check, so legacy rows emit policy_version: "".

[claude]

P3 — Clock is never wired in cmd/api/main.go, so there are now two independent clocks on the freshness path: Revalidate compares fresh_until against h.approvalNow() (this field, else time.Now) while Store.ConsumeActionApproval compares it against Store.now(). A test that injects one but not the other sees the two disagree, and the handler check can pass while the store CAS rejects (or vice versa). Worth either threading the store clock into Options.Clock or dropping the field and reading the store clock.

[claude]

Has P1/P2 findings, changes requested: 1 P1, 2 P2, 4 P3.

P1: the new decision-context columns are added with empty defaults and never backfilled, so every approval row that predates the deploy fails the context_complete gate and can never be approved (422 approval_context_incomplete). That includes POST /api/v1/agents/dev-loop/{workflow_id}/approve, where a pending grant bypasses the legacy-payload fallback, so in-flight human-approval gates are stuck until their TTL expires. No kill switch exists to relax the gate.

P2: guardDecision runs before the store state checks, so an expired / already-decided / self-decision request with an incomplete context answers 422 instead of 409/403 and loses details.reapproval_required, contradicting the contract doc. P2: revalidateApproval is new refusal behaviour on consume and on the grant spend (five 409 codes plus a 503) with no handler-level test, and the new fresh_until CAS predicate and the min(expires_at, now+FreshFor) clamp are untested at the store level.

The design itself is sound — length-prefixed digest, fail-closed authority check, ordered revalidation, and the legacy-row handling for fresh_until/decided_context_digest is careful. The gap is that the same legacy-row care was not applied to context completeness.

Note: go build and go test were not available in this run, so compilation and the new tests were not executed; findings are from static review only.

[github-actions]

Antigravity (agy) review

agy returned a result this workflow could not use; see the workflow log. Treated as a failed review.

[mctl-agents]

@claude review

[claude]

Claude finished @mashkovd's task in 7m 29s —— View job

---

Claude Code Review

• Read pinned diff (841d2e7...05bc250, 17 files, +1558/-74)
• Check whether the previous round's findings were addressed (fa980ef)
• Review internal/governance core logic
• Review API handlers + router wiring
• Review internal/workitems store + migration backfill
• Review config, helm, openapi, docs
• Build / vet / test — go build, go vet and go test were not permitted in this run, so nothing was compiled or executed. Findings are from static review only.
• Post inline findings and verdict

Verdict: changes requested — 0 P1, 2 P2, 3 P3. Findings are posted as inline comments bound to 05bc250.

Previous round's findings: fixed

fa980ef addresses all three blockers.

• P1 (unbackfilled context columns) — the migration now backfills side_effect, policy_decision, policy_reasons and intent_source for pre-governance rows. The legacy marker is sound: execution_id is a required field, so a new row always has an intent_source, which makes WHERE intent_source = '' both correct and idempotent. decided_context_digest = '' correctly protects rows decided after the deploy.
• P2 (gate precedence) — DecideActionApproval now runs guardDecision only when cur.State == ApprovalPending && cur.RequestedBy != principalOf(user), so the 409/403 terminal refusals keep precedence, with TestF6_ContextGateDoesNotMaskTerminalRefusal pinning it.
• P2 (missing tests) — TestF3_ConsumeRevalidatesBeforeSpend covers all five revalidation refusals through the HTTP handler, asserting status, code, details.reapproval_required and that the row stays approved. TestActionApprovalFreshUntil covers the min(expires_at, now+FreshFor) clamp, the nil case and the CAS; TestActionApprovalSchemaBackfillsLegacyContext covers the backfill and its idempotency.

P2 — consume revalidates before the store's state checks

internal/api/handlers_action_approvals.go:531 — the same precedence bug that was just fixed on the decide path, now on consume. In TENANT_AUTHZ_MODE=principal, consuming a still-pending approval hits case prn == "" && live.PrincipalMode (a pending row has no decided_by_principal_id) and answers 409 approval_authority_revoked with details.reapproval_required=true instead of 409 approval_not_approved. A caller doing what the contract doc tells it to — key on that flag — throws away a live pending approval. Terminal rows are masked too: a consumed row past fresh_until answers approval_stale rather than approval_consumed, losing the replay signal. Fix is the mirror of the decide fix: revalidate only when the state is approved.

P2 — create accepts a context that can never be completed

internal/api/handlers_action_approvals.go:317 — FillCreateContext defaults side_effect/policy_decision/policy_reasons only for a policy_rule_id present in governance.Policies, which has exactly one entry (dev-loop.human-approval), while openapi documents all three as Optional; defaults from the registered policy. Any other rule gets 201 and then a permanent 422 approval_context_incomplete on every approve — no PATCH, the backfill deliberately skips it, no kill switch. The test helper approvalBody had to add all four fields for exactly this reason. Refusing at create, where the client can still fix it, makes the contract enforceable instead of producing dead rows.

P3

• Backfill: risk_level is not backfilled, so a legacy row for an action_kind outside the operation registry still reports missing_fields: [risk_level]; and the WHERE has no state predicate, so denied/consumed/executed rows are rewritten too.
• decideDevLoopApproval (handlers_dev_loop.go:648) did not get the self-decision half of the new precedence guard — not reachable today, but the two decide paths now disagree.
• The previous round's P3s are all still open: the "first six" comment contradicting reapproval: false, unread Result.PolicyStatus and LiveAuthority.Mode, skipped_legacy_mode recorded after CheckHumanPrincipal passed, silently empty devLoopPolicy version, unwired Options.Clock leaving two clocks on the freshness path, and the audit risk override.

What looks right

Length-prefixed canonical digest; fail-closed ErrAuthorityUnavailable; ordered revalidation with the order pinned by a test; risk taken from the live registry so a client cannot downgrade a registered operation; fresh_until enforced both in the CAS predicate and as a post-hoc refusal with min(expires_at, …) clamping; context limited to references, digests and enums; boot-time validation of ACTION_APPROVAL_MAX_DECISION_AGE that refuses a non-positive value; both spend sites revalidated; and helm/openapi/docs carry the new env vars, codes and details shape.
· branch feat/agents-issue-700-governance-define-authority-and-decision

[claude]

P2 — revalidateApproval runs before Store.ConsumeActionApproval, so a revalidation refusal outranks the store's state checks (approval_not_approved, approval_denied, approval_consumed, approval_expired). This is the same precedence bug that was just fixed on the decide path in fa980ef, now on the consume path.

Failure scenario, TENANT_AUTHZ_MODE=principal: the requesting service polls POST /api/v1/action-approvals/{id}/consume on an approval that is still pending. visibleActionApproval returns the pending row whatever its state, then Revalidate reaches case prn == "" && live.PrincipalMode (internal/governance/governance.go:365) — a pending row has no decided_by_principal_id — and answers 409 approval_authority_revoked with details.reapproval_required=true. The caller, which docs/decision-context-contract.md tells to key on that flag, abandons a perfectly live pending approval and asks a human for a new one. The correct answer is 409 approval_not_approved, which carries reapproval_required: false.

Same class for terminal rows: an already-consumed approval whose fresh_until has passed answers approval_stale instead of approval_consumed, losing the replay signal; a denied row whose denier has since been disabled answers approval_decider_disabled instead of approval_denied.

Fix: if cur.State == workitems.ApprovalApproved { reval, ok = h.revalidateApproval(...) }, mirroring the cur.State == workitems.ApprovalPending guard added at line 481. Revalidation only has meaning for an approved row.

[claude]

P2 — Create persists a row whose decision context can never be completed, and the only signal is a 422 at approve time. FillCreateContext fills side_effect, policy_decision and policy_reasons only if p, ok := ps[intent.PolicyRuleID]; ok (internal/governance/governance.go:286), and governance.Policies has exactly one entry, dev-loop.human-approval. risk_level is filled only for an action kind in the operation registry. Nothing checks completeness here, and validateContext only validates non-empty values.

Failure scenario: a service principal posts a valid request with policy_rule_id: "merge-needs-human" and no side_effect/policy_decision/policy_reasons — all three documented as Optional; defaults from the registered policy in internal/openapi/openapi.yaml:4437-4441. It gets 201 and a pending aar_. Every subsequent approve is then 422 approval_context_incomplete with missing_fields: [side_effect, policy_decision, policy.reasons], forever: there is no PATCH, the new backfill at internal/workitems/action_approvals.go:169 deliberately skips it (intent_source is non-empty), and no env var relaxes the gate. The only exits are deny or waiting out expires_at. Before this PR the same request approved fine, so an unchanged client is silently broken at the point a human tries to act.

This repo's own tests show the shape of the problem: approvalBody in handlers_action_approvals_test.go:30-36 had to add all four fields with the comment "github.merge_pr is no registered operation and merge-needs-human no registered policy, so the client supplies what a reviewer must be shown". That is a reasonable contract — but it should be enforced where it can still be fixed. Refuse at create with 422 approval_context_incomplete (details.missing_fields) when DecisionContextOf is not complete, rather than accepting a 201 that becomes a dead row.

[claude]

P3 — Two gaps in the backfill (which does fix the earlier P1 for the common case: execution_id is required, so intent_source is a sound legacy marker and the UPDATE is idempotent).

1. risk_level is not backfilled. RiskOf recovers it only for an action_kind in the operation registry; a legacy pending row for any other kind still reports missing_fields: [risk_level] and stays unapprovable. mctl-agents-approve is registered (internal/operations/registry.go:939), so the DevLoop path is covered, but the generic create endpoint takes an arbitrary action_kind. A WHEN risk_level = THEN medium arm would close the same hole the other four columns got.

2. The WHERE clause has no state predicate, so the UPDATE also rewrites denied, consumed and executed rows, asserting side_effect=write and policy_decision=require_approval about side effects that already happened and were never classified. policy_reasons=[legacy.pre-governance] does mark them, which keeps it honest, but the P1 only needed state IN (pending, approved) — narrowing it keeps the durable record of past decisions untouched.

Worth a line in docs/decision-context-contract.md either way: a reviewer looking at a backfilled row sees side_effect: write with no indication it is a placeholder unless they know what legacy.pre-governance means.

[claude]

P3 — This call site did not get the precedence guard that DecideActionApproval gained in fa980ef. The state is pending by construction here, so the terminal-state half does not apply, but the self-decision half does: if the caller is also newest.RequestedBy, an incomplete context answers 422 approval_context_incomplete instead of 403 approval_self_decision. Not reachable today (create is isDirectService-only and the approve route is admin-only), but it makes the two decide paths disagree about precedence, and a future caller that can both request and approve would hit it. Extracting the cur.State == workitems.ApprovalPending && cur.RequestedBy != principalOf(user) condition into guardDecision itself would keep both sites in step.

[claude]

P3 (carried over from the previous round, still open) — "The first six mean the approval must be re-requested" contradicts actionApprovalRefusals, where ErrContextIncomplete and ErrContextInvalid are reapproval: false (internal/api/handlers_action_approvals.go:117-118) in agreement with docs/decision-context-contract.md. The six that do set the flag are ErrContextChanged, ErrApprovalStale, ErrAuthorityRevoked, ErrDeciderDisabled, ErrPolicyDrift and workitems.ErrApprovalExpired — not a contiguous prefix of this block. Naming them, or dropping the count, would stop the comment from contradicting the code.

Also still open from the previous round, all P3 and non-blocking:

• Result.PolicyStatus is set by Revalidate but read by nobody, so a spend allowed under an unregistered rule with ACTION_APPROVAL_REQUIRE_REGISTERED_POLICY off leaves no trace in the consume audit; LiveAuthority.Mode is assigned in revalidateApproval and never read.
• Revalidation is overwritten with skipped_legacy_mode at line 383 even though CheckHumanPrincipal just ran and passed — only the ADMIN_PRINCIPALS membership step was skipped.
• devLoopPolicy (internal/api/handlers_approval_governance.go:50) yields an empty version for a Policies set missing dev-loop.human-approval, surfacing later as 409 approval_intent_mismatch.
• Options.Clock is still unwired in cmd/api/main.go, so handler freshness and the store CAS read two clocks; TestF3_ConsumeRevalidatesBeforeSpend/stale now depends on the handler winning that race.
• auditActionApproval overrides the call site's operation risk with the approved action's risk, and params["policy_version"] is unconditional while every neighbouring optional field is guarded.

[claude]

Has P1/P2 findings, changes requested: 0 P1, 2 P2, 3 P3.

The previous round's P1 (unbackfilled context columns) and both P2s (gate precedence on decide, missing handler/store tests for revalidation and fresh_until) are addressed in fa980ef. The backfill marker is sound — execution_id is required, so an empty intent_source really does identify a pre-governance row and the UPDATE is idempotent — and the new tests cover the five revalidation refusals end to end, the min(expires_at, now+FreshFor) clamp, the CAS, and that a refusal leaves the row approved.

Two P2s remain, both residue of the same precedence/fail-fast question:

• ConsumeActionApproval revalidates before the store's state checks, so in TENANT_AUTHZ_MODE=principal a consume of a still-pending approval answers 409 approval_authority_revoked with reapproval_required=true (no decided_by_principal_id on a pending row) instead of 409 approval_not_approved. A caller following the documented rule of keying on that flag discards a live approval. Terminal rows are similarly masked (approval_stale instead of approval_consumed). The fix is the mirror of the one just applied to decide: only revalidate when the state is approved.
• Create accepts an incomplete decision context and persists an unapprovable row. FillCreateContext defaults side_effect/policy_decision/policy_reasons only for a rule in governance.Policies, which holds one entry, yet openapi documents all three as optional. Any other policy_rule_id yields 201 followed by a permanent 422 at approve, with no PATCH, no backfill and no kill switch. Refusing at create with approval_context_incomplete makes the contract enforceable where it can still be met.

P3: the backfill leaves risk_level empty for an unregistered action_kind and rewrites terminal rows; decideDevLoopApproval did not get the self-decision half of the new precedence guard; the previous round's five P3 nits are still open (the "first six" comment, unread PolicyStatus/Mode, skipped_legacy_mode overstated, silent empty devLoopPolicy version, unwired Options.Clock, audit risk override).

Note: go build, go vet and go test were not permitted in this run, so nothing was compiled or executed; findings are from static review of 841d2e7...05bc250 only.

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-high · commit 05bc250.

Finding 1: Calling revalidateApproval on non-approved requests returns approval_authority_revoked with reapproval_required: true

• Severity: P1
• File: internal/api/handlers_action_approvals.go (approx. line 531)
• Anchor: + reval, ok := h.revalidateApproval(w, r, user, cur)
• Concrete Failure Scenario:
  In ConsumeActionApproval, h.revalidateApproval is called unconditionally on cur before checking whether the request is in workitems.ApprovalApproved state.
  When a client attempts to consume a pending approval request (cur.State == workitems.ApprovalPending), cur.DecidedByPrincipalID is empty (""). In TENANT_AUTHZ_MODE=principal, governance.Revalidate checks step 3:

  case prn == "" && live.PrincipalMode:
      return res, fmt.Errorf("%w: %s records no decider principal", ErrAuthorityRevoked, a.ID)

  It returns ErrAuthorityRevoked, which writeActionApprovalError renders as HTTP 409 approval_authority_revoked with details: {"reapproval_required": true}.
  This masks the expected approval_not_approved status. Because the contract instructs clients to key on details.reapproval_required as an unrecoverable failure requiring a new request, clients polling or racing to consume a pending approval will abort and abandon the pending request rather than waiting for an administrator's decision.
  Similarly, for approvals in terminal states (e.g. consumed, expired, or denied), calling revalidateApproval before checking state masks the terminal response (e.g. returning approval_stale with reapproval_required: true on an already-consumed request whose decision age exceeded fresh_until, instead of approval_consumed).
• Proposed Fix:
  Only run h.revalidateApproval when cur.State == workitems.ApprovalApproved. If cur.State != workitems.ApprovalApproved, skip revalidateApproval and proceed directly to h.opts.WorkItems.ConsumeActionApproval (or return the appropriate store refusal) so that approval_not_approved, approval_consumed, approval_expired, and approval_denied preserve precedence.

---

Finding 2: Database backfill migration omits risk_level, permanently blocking approval of pending legacy requests

• Severity: P2
• File: internal/workitems/action_approvals.go (approx. line 169)
• Anchor: +UPDATE action_approval_requests SET
• Concrete Failure Scenario:
  The schema migration adds the decision context columns with empty defaults (ALTER TABLE action_approval_requests ADD COLUMN IF NOT EXISTS risk_level TEXT NOT NULL DEFAULT '';) and runs a backfill intended to ensure that:

  -- Backfill rows written before the decision-context columns existed, so a
  -- pending legacy approval is not stuck at approval_context_incomplete.

  However, the UPDATE query only backfills side_effect, policy_decision, policy_reasons, and intent_source:

  UPDATE action_approval_requests SET
      side_effect = CASE WHEN side_effect = '' THEN 'write' ELSE side_effect END,
      policy_decision = CASE WHEN policy_decision = '' THEN 'require_approval' ELSE policy_decision END,
      policy_reasons = CASE WHEN cardinality(policy_reasons) = 0
          THEN ARRAY['legacy.pre-governance'] ELSE policy_reasons END,
      intent_source = CASE
          WHEN intent_source <> '' THEN intent_source
          WHEN work_item_id <> '' THEN 'work_item:' || work_item_id
          ELSE 'execution:' || execution_id END
  WHERE intent_source = '' AND decided_context_digest = '';

  risk_level is omitted. Existing legacy rows therefore retain risk_level = ''.
  For any pre-existing action approval request whose action_kind is not registered in operations.Registry (such as github.merge_pr), governance.RiskOf returns "". When an administrator attempts to approve the pending legacy request, governance.DecisionContextOf marks risk_level as missing (ContextComplete = false), and guardDecision rejects the approval with HTTP 422 approval_context_incomplete (missing_fields: ["risk_level"]). Because there is no endpoint to modify an existing request's decision context, pending legacy approvals for unregistered action kinds cannot be approved and remain permanently stuck.
• Proposed Fix:
  Include risk_level in the backfill UPDATE statement:

  UPDATE action_approval_requests SET
      risk_level = CASE WHEN risk_level = '' THEN 'medium' ELSE risk_level END,
      side_effect = CASE WHEN side_effect = '' THEN 'write' ELSE side_effect END,
      ...

---

Finding 3: Missing test coverage for decision_context in ListActionApprovals

• Severity: P3
• File: internal/api/handlers_action_approvals.go (approx. line 442)
• Anchor: + out[i] = approvalWithContext{ActionApprovalRequest: &list[i], DecisionContext: h.decisionContext(&list[i])}
• Concrete Failure Scenario:
  ListActionApprovals was updated to project list items into approvalWithContext so that decision_context is served alongside each listed approval. There is no test visible in this diff verifying that GET /api/v1/action-approvals returns the expected decision_context structure for listed items.
• Proposed Fix:
  Add a test in internal/api/handlers_action_approvals_test.go or internal/api/handlers_approval_governance_test.go that calls GET /api/v1/action-approvals and asserts decision_context fields on listed entries.

[claude]

Claude finished @mashkovd's task in 5m 9s —— View job

---

Claude Code Review

• Read pinned diff (841d2e7...f7b249c, 20 files, +1578/-74)
• Re-check the two open P2s from the previous round
• Review internal/governance core logic
• Review API handlers + router wiring
• Review internal/workitems store changes + migration backfill
• Build / vet / test — go build, go vet, go test and gofmt were all refused in this run, so nothing was compiled or executed. Findings are from static review only.
• Post inline findings and verdict

Verdict: changes requested — 0 P1, 2 P2, 3 P3. Findings are posted as inline comments bound to f7b249c.

What this round changed

f7b249c adds the missing decision_refused audit entry for the decide pre-read and repairs four test fixtures that the gate had broken (loginPrincipals now implements CheckHumanPrincipal, the grant envs get a registry and a client-supplied risk level). Previous round's P1 (unbackfilled context columns) and the decide-path precedence P2 remain fixed; the revalidation and store tests from fa980ef hold up.

P2 — consume revalidates before the store's state checks

internal/api/handlers_action_approvals.go:536. DecideActionApproval got the state guard at line 488; ConsumeActionApproval did not. In TENANT_AUTHZ_MODE=principal, consuming a still-pending approval hits case prn == "" && live.PrincipalMode (governance.go:360) and answers 409 approval_authority_revoked with details.reapproval_required=true, instead of the store's 409 approval_not_approved (action_approvals.go:774). docs/decision-context-contract.md tells callers to key on that flag, so the caller throws away a live approval. openapi.yaml:4612 still documents approval_not_approved (pending) for this case, so spec and code disagree. Terminal rows are masked the same way (approval_stale over approval_consumed). Fix is the mirror of the decide guard: revalidate only when cur.State == workitems.ApprovalApproved.

P2 — create accepts an incomplete context and persists an unapprovable row

internal/api/handlers_action_approvals.go:314. FillCreateContext defaults side_effect/policy_decision/policy_reasons only for a rule present in governance.Policies, which holds exactly one entry, yet openapi.yaml:4437-4441 documents all three as optional. Any other policy_rule_id gets 201 and then a permanent 422 at approve — no PATCH, the backfill deliberately skips the row, no kill switch. Retrying create does not recover it either: the context fields are not covered by intent_hash, so the same idempotency key returns the old incomplete row (action_approvals.go:517). Refusing at create with approval_context_incomplete enforces the contract where it can still be met.

P3

• Backfill (internal/workitems/action_approvals.go:178) leaves risk_level empty for an unregistered action_kind, and has no state predicate, so it rewrites denied/consumed/executed rows too.
• decideDevLoopApproval (handlers_dev_loop.go:648) did not get the self-decision half of the precedence guard.
• Carried nits on governance.go:30: the "first six" comment contradicts actionApprovalRefusals; Result.PolicyStatus and LiveAuthority.Mode are written and never read; skipped_legacy_mode overstates what was skipped at line 383; devLoopPolicy yields a silent empty version; Options.Clock is still unwired in cmd/api/main.go, leaving two clocks on the freshness path; the audit risk override conflates action risk with operation risk and policy_version is emitted unguarded.

What looks right

Length-prefixed canonical digest with a policy_reasons regex that excludes ,; fail-closed ErrAuthorityUnavailable on a non-human-checking principal store; ordered revalidation with a test pinning the order; risk taken from the live registry so a client cannot downgrade a registered operation; fresh_until enforced both in the CAS predicate and post-hoc, with the min(expires_at, now+FreshFor) clamp tested; context limited to references, digests and enums; boot-time validation of ACTION_APPROVAL_MAX_DECISION_AGE; and both ConsumeActionApproval call sites revalidate.
· branch feat/agents-issue-700-governance-define-authority-and-decision

[claude]

P2 (carried from the previous round, still open) — revalidateApproval runs before Store.ConsumeActionApproval, so a revalidation refusal outranks the store's own state checks. DecideActionApproval got the precedence guard in this PR (line 488, cur.State == workitems.ApprovalPending && cur.RequestedBy != principalOf(user)); the consume path did not.

Failure scenario, TENANT_AUTHZ_MODE=principal: the requesting service calls POST /api/v1/action-approvals/{id}/consume on an approval that is still pending. visibleActionApproval (line 528) returns the row whatever its state, then Revalidate hits case prn == "" && live.PrincipalMode (internal/governance/governance.go:360) — a pending row has no decided_by_principal_id — and answers 409 approval_authority_revoked with details.reapproval_required=true. The store would have answered 409 approval_not_approved (internal/workitems/action_approvals.go:774-775), which carries reapproval_required: false. docs/decision-context-contract.md tells callers to key on that flag, so the caller discards a live pending approval and asks a human for a new one.

internal/openapi/openapi.yaml:4612 still documents the pending case as approval_not_approved (pending), so the spec and the code disagree for exactly this row.

Terminal rows are masked the same way: an already-consumed approval whose fresh_until has passed answers approval_stale instead of approval_consumed (losing the replay signal), and a denied row whose denier has since been disabled answers approval_decider_disabled instead of approval_denied. The grant path is partly shielded — prepareApprovalGrant refuses a row with no human decider — but a consumed or denied grant row reaches spendApprovalGrant (internal/api/handlers_approval_grant.go:197) and is masked there too.

Fix: mirror the decide guard — only revalidate an approved row.

reval := governance.Result{}
if cur.State == workitems.ApprovalApproved {
    var ok bool
    if reval, ok = h.revalidateApproval(w, r, user, cur); !ok {
        return
    }
}

Revalidation only has meaning for an approved row; every other state has a store refusal that is strictly more accurate.

[claude]

P2 (carried from the previous round, still open) — create persists a row whose decision context can never be completed, and the only signal is a 422 at approve time.

FillCreateContext fills side_effect, policy_decision and policy_reasons only if p, ok := ps[intent.PolicyRuleID]; ok (internal/governance/governance.go:295), and governance.Policies holds exactly one entry, dev-loop.human-approval. risk_level is filled only for an action_kind in the operation registry. Nothing checks completeness here; validateContext only validates non-empty values.

Failure scenario: a service principal posts a request with policy_rule_id: "merge-needs-human" and no side_effect/policy_decision/policy_reasons — all three documented as Optional; defaults from the registered policy at internal/openapi/openapi.yaml:4437-4441. It gets 201 and a pending aar_. Every subsequent approve is 422 approval_context_incomplete with missing_fields: [side_effect, policy_decision, policy.reasons], permanently: there is no PATCH, the new backfill at internal/workitems/action_approvals.go:169 deliberately skips the row (intent_source is non-empty), and no env var relaxes the gate.

Retrying create does not recover it either: the decision-context fields are not part of IntentHash, so a retry with the same idempotency key and a corrected context matches on existing.IntentHash == hash (internal/workitems/action_approvals.go:517) and silently returns the old, incomplete row with created=false. Recovery needs a deny plus a brand-new idempotency key — i.e. a client change. Before this PR the same request approved fine, so an unchanged client is silently broken at the point a human tries to act.

TestF6_IncompleteContextCannotBeApproved codifies exactly this shape, and approvalBody in handlers_action_approvals_test.go:30-36 had to add all four fields with the comment "github.merge_pr is no registered operation and merge-needs-human no registered policy, so the client supplies what a reviewer must be shown". That contract is reasonable — it should just be enforced where it can still be met.

Fix: after FillCreateContext, refuse at create when the resulting context is not complete, with 422 approval_context_incomplete and details.missing_fields, rather than accepting a 201 that becomes a dead row. The handler already has everything it needs — build the prospective workitems.ActionApprovalRequest (or factor the mandatory-field loop out of DecisionContextOf) and check ContextComplete.

[claude]

P3 — the backfill does close the previous round's P1 for the common case (execution_id is required, so an empty intent_source really is a sound pre-governance marker, and the UPDATE is idempotent). Two gaps remain.

1. risk_level is not backfilled. governance.RiskOf recovers it only for an action_kind in the operation registry; a legacy pending row for any other kind still reports missing_fields: [risk_level] and stays unapprovable. mctl-agents-approve is registered, so the DevLoop path is covered, but the generic create endpoint accepts an arbitrary action_kind. A WHEN risk_level = '' THEN 'medium' arm would close the same hole the other four columns got.

2. The WHERE has no state predicate, so the UPDATE also rewrites denied, consumed and executed rows, asserting side_effect=write and policy_decision=require_approval about side effects that already happened and were never classified. policy_reasons=[legacy.pre-governance] does mark them, which keeps it honest, but the P1 only needed state IN ('pending','approved'); narrowing it leaves the durable record of past decisions untouched.

Either way, worth a line in docs/decision-context-contract.md: a reviewer looking at a backfilled row sees side_effect: write with no indication it is a placeholder unless they already know what legacy.pre-governance means.

[claude]

P3 (carried, still open) — "The first six mean the approval must be re-requested" contradicts actionApprovalRefusals, where ErrContextIncomplete and ErrContextInvalid are reapproval: false (internal/api/handlers_action_approvals.go:118-119), in agreement with docs/decision-context-contract.md. The six that do set the flag are ErrContextChanged, ErrApprovalStale, ErrAuthorityRevoked, ErrDeciderDisabled, ErrPolicyDrift and workitems.ErrApprovalExpired — not a contiguous prefix of this block. Naming them, or dropping the count, would stop the comment from contradicting the code.

The rest of the open P3s from the previous rounds, all non-blocking, grouped here rather than reposted individually:

• Result.PolicyStatus (line 395) is set but read by nobody — revalidateApproval only forwards res.Revalidation — so a spend allowed under an unregistered rule with ACTION_APPROVAL_REQUIRE_REGISTERED_POLICY off leaves no trace in the consume audit. LiveAuthority.Mode (line 322) is assigned in revalidateApproval and never read anywhere; either audit it or drop the field.
• Line 383: Revalidation is overwritten with skipped_legacy_mode even though CheckHumanPrincipal just ran and passed — only the ADMIN_PRINCIPALS membership step was skipped. A distinct value (e.g. admin_check_skipped) would let the audit distinguish "nothing was re-checked" (line 365, no principal at all) from "the human check ran, membership did not".
• devLoopPolicy (internal/api/handlers_approval_governance.go:52) yields an empty version for a Policies set missing dev-loop.human-approval — map index on a missing key gives the zero Policy. That value feeds IntentHash in bindApprovalGrant, so the grant spend fails later with 409 approval_intent_mismatch, a confusing symptom for a configuration mistake.
• Options.Clock is still unwired in cmd/api/main.go, so the handler freshness check (h.approvalNow()) and the store CAS (Store.now()) read two independent clocks. TestF3_ConsumeRevalidatesBeforeSpend/stale only passes because the handler wins that race; a test that injects the store clock instead would see them disagree.
• auditActionApproval (handlers_action_approvals.go:182) overrides the call site's operation risk with the approved action's risk, so an action_approval.create entry reports high for what is a low-risk write; and params["policy_version"] is unconditional while every neighbouring optional field is guarded by a non-empty check, so legacy rows emit policy_version: "".

[claude]

P3 (carried, still open) — this call site did not get the precedence guard DecideActionApproval gained in fa980ef. The state is pending by construction here, so the terminal-state half does not apply, but the self-decision half does: if the caller is also newest.RequestedBy, an incomplete context answers 422 approval_context_incomplete instead of 403 approval_self_decision. Not reachable today (create is isDirectService-only and the approve route is admin-only), but it leaves the two decide paths disagreeing about precedence. Folding a.State == workitems.ApprovalPending && a.RequestedBy != principalOf(user) into guardDecision itself would keep both sites in step and remove the duplicated condition at handlers_action_approvals.go:488.

[claude]

Has P1/P2 findings, changes requested: 0 P1, 2 P2, 3 P3.

This round (f7b249c) adds the decide pre-read audit entry and fixes four test fixtures. The previous round's P1 (unbackfilled context columns) and the decide-path precedence P2 stay fixed, and the revalidation/store tests added in fa980ef hold up. Both remaining P2s are the ones carried from the last round, unchanged.

• Consume revalidates before the store's state checks (handlers_action_approvals.go:536). The decide path got the state guard; consume did not. In TENANT_AUTHZ_MODE=principal, consuming a still-pending approval answers 409 approval_authority_revoked with reapproval_required=true — a pending row has no decided_by_principal_id — instead of 409 approval_not_approved. A caller following the documented rule of keying on that flag discards a live approval; openapi.yaml:4612 still documents approval_not_approved (pending) for this case. Terminal rows are masked too (approval_stale over approval_consumed). The fix is the mirror of the one applied to decide: only revalidate when the state is approved.
• Create accepts an incomplete decision context and persists an unapprovable row (handlers_action_approvals.go:314). FillCreateContext defaults side_effect/policy_decision/policy_reasons only for a rule in governance.Policies, which holds one entry, while openapi documents all three as optional. Any other policy_rule_id yields 201 followed by a permanent 422 at approve. Retrying create does not recover it either: the context fields are not covered by intent_hash, so the same idempotency key returns the old incomplete row. Refusing at create with approval_context_incomplete enforces the contract where it can still be met.

P3: the backfill leaves risk_level empty for an unregistered action_kind and also rewrites terminal rows; decideDevLoopApproval did not get the self-decision half of the precedence guard; the earlier nits are still open (the "first six" comment, unread PolicyStatus/Mode, overstated skipped_legacy_mode, silent empty devLoopPolicy version, unwired Options.Clock, audit risk override).

What holds up well: the length-prefixed canonical digest with a policy_reasons regex that excludes ,; fail-closed ErrAuthorityUnavailable; ordered revalidation pinned by a test; risk taken from the live registry so a client cannot downgrade a registered operation; fresh_until enforced both in the CAS predicate and post-hoc; context limited to references, digests and enums; and boot-time validation of ACTION_APPROVAL_MAX_DECISION_AGE.

Note: go build, go vet, go test and gofmt were all refused in this run, so nothing was compiled or executed; findings are from static review of 841d2e7...f7b249c only.

[github-actions]

Antigravity (agy) review

agy returned a result this workflow could not use; see the workflow log. Treated as a failed review.

[claude]

Claude finished @mashkovd's task in 6m 26s —— View job

---

Claude Code Review

• Read pinned diff (841d2e7...5ad100d, 20 files, +1738/-83)
• Re-check the two open P2s (consume precedence, create completeness)
• Review internal/governance core logic
• Review API handlers + router wiring
• Review internal/workitems store changes + migration backfill
• Check every HTTP create fixture against the new create gate
• Build / vet / test — go build, go vet and go test were all refused in this run, so nothing was compiled or executed. Findings are from static review only.
• Post inline findings and verdict

Verdict: approved — 0 P1, 0 P2, 3 P3. Findings are posted as inline comments bound to 5ad100d.

Both previous P2s are fixed, and in the right shape

• Consume precedence. workitems.ConsumeStateRefusal is now one exported function used by the store itself and by both spend paths (ConsumeActionApproval handler at handlers_action_approvals.go:544, spendApprovalGrant at handlers_approval_grant.go:199) before revalidation. A pending row answers 409 approval_not_approved instead of approval_authority_revoked with reapproval_required=true; consumed/executed/denied/expired rows keep their own codes. TestF3_ConsumeStateRefusalPrecedesRevalidation drives pending, consumed and executed through the handler in TENANT_AUTHZ_MODE=principal with the decider's authority removed, asserting the code, the absence of reapproval_required, that the row stays pending, and that an executed row stays final. Code and openapi.yaml:4612 agree again.
• Create completeness. CreateContext.Missing() plus the refusal at handlers_action_approvals.go:325 stops an unapprovable row from being persisted; the 422 carries details.missing_fields, and openapi's field descriptions and new 422 response match. TestF6_IncompleteContextIsRefusedAtCreate covers the refusal, the exact missing_fields, and that the same idempotency key with the context supplied is a fresh 201 — not a replay of a stored incomplete row. TestF6_IncompleteContextCannotBeApproved now builds its row through the store, so the approve-time gate stays covered for pre-governance rows.

The DevLoop path is unaffected: mctl-agents-approve is registered (RiskMedium) and the grant's intent hash is rebuilt from dev-loop.human-approval/v1, which is in governance.Policies, so both the registry and the policy defaults fill. Create does now refuse what it used to accept for any other policy_rule_id — the intended contract change, but a client-visible break with no kill switch, so worth confirming no producer outside the DevLoop flow creates approvals under an unregistered rule.

P3 (inline)

• governance.go:285 — Missing() duplicates the mandatory-field list in DecisionContextOf, with nothing pinning the two together; a field added there without an arm here silently reopens the dead-row hole this commit closes. risk_level is required here but still not backfilled.
• handlers_action_approvals.go:325 — the gate outranks the store's own invalid_request / secret_in_text / intent_hash_mismatch refusals, and precedes the idempotency lookup, so a retry under a stored key that omits a context field now 422s instead of replaying.
• docs/decision-context-contract.md:17 — not updated for the create-time 422 or the new state-refusal-first ordering, so this normative doc now disagrees with the openapi spec it describes.

P3 carried from earlier rounds, still open (not reposted)

Backfill omits risk_level and has no state predicate; decideDevLoopApproval lacks the self-decision half of the precedence guard; the "first six" comment at governance.go:30 contradicts actionApprovalRefusals; Result.PolicyStatus and LiveAuthority.Mode are written and never read; skipped_legacy_mode overstates what was skipped at governance.go:404; devLoopPolicy yields a silent empty version; Options.Clock is still unwired in cmd/api/main.go, leaving two clocks on the freshness path; the audit risk override conflates action risk with operation risk and policy_version is emitted unguarded.

What holds up

Length-prefixed canonical digest with a policy_reasons regex that excludes ,; fail-closed ErrAuthorityUnavailable on a principal store that cannot check a kind (the production principals.Resolver does implement CheckHumanPrincipal, so the common path does not 503); ordered revalidation with the order pinned by a test; risk taken from the live registry so a client cannot downgrade a registered operation; fresh_until enforced in the CAS predicate and post-hoc with the min(expires_at, now+FreshFor) clamp tested; closed enum vocabularies validated in the store; context limited to references, digests and enums; boot-time validation of ACTION_APPROVAL_MAX_DECISION_AGE.
· branch feat/agents-issue-700-governance-define-authority-and-decision

[claude]

P3 — Missing() is a second, independent copy of the mandatory-field list that DecisionContextOf uses for ContextComplete (governance.go:234-252). The two agree today only by inspection: Missing() checks four fields, DecisionContextOf nine plus policy.reasons, and the gap is covered by reasoning stated in the doc comment (intent_source follows from the required execution_id; target/args_digest/intent_hash/policy.rule_id/policy.version are refused by ActionApprovalInput.validate).

That reasoning holds, but nothing pins it. A field added to the DecisionContextOf loop without a matching arm here silently reopens exactly the hole this commit closes: a 201 at create followed by a permanent 422 at approve, with no PATCH and a backfill that deliberately skips the row. A test that creates through the handler and asserts decision_context.context_complete == true on the 201 body would pin the invariant from the outside; deriving both lists from one table would remove the duplication altogether.

Related, same function: risk_level is required here but still not backfilled by the migration, so a pre-governance pending row whose action_kind is outside the operation registry remains unapprovable — new rows are now guarded, legacy ones are not.

[claude]

P3 — the completeness gate runs before Store.CreateActionApproval, so it now outranks that method's own validation. A body that is both malformed and context-incomplete answers 422 approval_context_incomplete rather than the more actionable refusal: a missing target or args_digest would be 400 invalid_request, a credential in target 400 secret_in_text, an expires_at outside the window 400, an intent_hash that does not match 400 intent_hash_mismatch. Same shape as the precedence problem fixed on decide and consume in this PR, just benign here — the client has to fix both fields either way. Running the gate after in.validate() (or validating the intent fields first) would keep the most specific refusal.

One behaviour worth confirming is intended: the gate also precedes the idempotency lookup, so a retry under an already-stored key that omits a context field now gets 422 instead of the 200 replay of the stored row. The context fields are not part of intent_hash, so before this commit such a retry replayed fine. A client that reconstructs the full body each time is unaffected; one that retries from a stored intent only is not.

[claude]

P3 — this contract doc is the normative description of the behaviour and it no longer matches the code after 5ad100d. internal/openapi/openapi.yaml:4448 was updated; this file was not.

• The mandatory context is now enforced at create (422 approval_context_incomplete, details.missing_fields, nothing stored), not only at approve. This section and the refusal table at line 81 (approval_context_incomplete (decide)) still describe the approve gate as the only one, and lines 24/27 still call side_effect, policy.decision and policy.reasons "request, else the registered policy" without saying that an unregistered rule makes them required, nor that risk_level is required for an unregistered action_kind.
• The spend ordering at lines 55-65 lists the revalidation steps as the first thing a consume does. The state refusal (workitems.ConsumeStateRefusal) now precedes step 1 on both spend paths, which is the point of this commit: a pending row answers approval_not_approved and a spent one approval_consumed, never approval_authority_revoked. Worth being step 0 in that list, since the doc is what tells callers to key on details.reapproval_required.

[claude]

No P1/P2 findings (3 P3 posted inline, plus carried nits listed below). Good to merge.

Both P2s from the previous round are fixed in 5ad100d, and the fixes are the right shape.

• Consume precedence. workitems.ConsumeStateRefusal is now a single exported function used by the store itself and by both spend paths (ConsumeActionApproval handler, spendApprovalGrant) before revalidation. A pending row answers 409 approval_not_approved rather than approval_authority_revoked with reapproval_required=true, and a consumed/executed/denied/expired row keeps its own code. TestF3_ConsumeStateRefusalPrecedesRevalidation drives pending, consumed and executed through the handler in TENANT_AUTHZ_MODE=principal with the decider's authority removed, asserting the code and that reapproval_required is absent; it also checks the row stays pending and that an executed row stays final. Code and openapi.yaml:4612 agree again.
• Create completeness. CreateContext.Missing() plus the refusal at handlers_action_approvals.go:325 means an unapprovable row is no longer persisted; the 422 carries details.missing_fields and the openapi field descriptions and the new 422 response were updated to match. TestF6_IncompleteContextIsRefusedAtCreate covers the refusal, the exact missing_fields, and that the same idempotency key with the context supplied is a fresh 201 rather than a replay of a stored incomplete row. TestF6_IncompleteContextCannotBeApproved now builds its row through the store, so the approve-time gate is still covered for pre-governance rows.

The DevLoop path is unaffected: mctl-agents-approve is registered (RiskMedium) and the grant's intent hash is rebuilt from dev-loop.human-approval/v1, which is in governance.Policies, so both the registry and the policy defaults fill. Create does now refuse what it used to accept for any other policy_rule_id, which is the intended contract change but is a client-visible break with no kill switch — worth confirming no producer outside the DevLoop flow creates approvals under an unregistered rule.

P3, posted inline: Missing() duplicates the mandatory-field list in DecisionContextOf with nothing pinning the two together; the create gate outranks the store's own invalid_request/secret_in_text refusals and the idempotency replay; docs/decision-context-contract.md was not updated for the create-time 422 or the new state-refusal-first ordering, so it now disagrees with the openapi spec it describes.

P3 carried from earlier rounds, still open and not reposted: the backfill omits risk_level and has no state predicate; decideDevLoopApproval lacks the self-decision half of the precedence guard; the "first six" comment at governance.go:30 contradicts actionApprovalRefusals; Result.PolicyStatus and LiveAuthority.Mode are written and never read; skipped_legacy_mode overstates what was skipped at governance.go:404; devLoopPolicy yields a silent empty version; Options.Clock is still unwired in cmd/api/main.go, leaving two clocks on the freshness path; and the audit risk override conflates action risk with operation risk while policy_version is emitted unguarded.

Note: go build, go vet and go test were all refused in this run, so nothing was compiled or executed. Findings are from static review of 841d2e7...5ad100d only; I did check the new create gate against every HTTP create fixture in internal/api and they all supply (or default) a complete context.

[github-actions]

Antigravity (agy) review

agy returned a result this workflow could not use; see the workflow log. Treated as a failed review.