[mashkovd]

Summary

Two additions to action approvals:

Approval provenance. A grant-authorized mctl-agents-approve now passes approval_ref, the id of the action approval it spends.

• prepareApprovalGrant sets it, next to approver and expected_content_digest.
• Like those, it is never taken from a request body: a body naming approval_ref gets 400 actor_not_accepted and a denied audit entry on every path (grant, human admin, service).
• Registry: an optional param with pattern ^aar_[A-Za-z0-9_-]{1,124}$.

Single-use execution mark. A consume records that a side effect was authorized, not that it ran. The new POST /api/v1/action-approvals/{id}/execute with body {target, artifact_hash} moves a consumed request to executed and records executed_at.

• It is one compare-and-set, guarded by the requester, the stored target and a non-empty stored artifact hash. It succeeds once however many callers race.
• Auth is the same as consume: requesting service only, and another service's request answers 404.
• Refusals are 409 approval_not_consumed, approval_executed, approval_denied and approval_target_mismatch, each audited as action_approval.execute_refused.
• executed is final: it never expires, it is not a live delegation grant, and consuming it again answers approval_consumed.
• The migration adds executed_at and widens the state CHECK in place, only when it does not already admit executed, so a restart does nothing. I checked it on a database with the old constraint and an existing consumed row.

Deploy order

mctl-gitops#2004 has to be live first. Every mctl-agents-approve submit now carries an approval_ref argument (empty off the grant path, from the defaults), and the workflow template has to declare it.

Test plan

• go test ./... -p 1: 40 packages, all pass.
• New handlers_action_approval_execute_test.go:
  • happy path, with GET, list and audit;
  • second mark, and marks on pending, approved and denied requests;
  • wrong target and wrong artifact leave the state unchanged;
  • malformed bodies;
  • every non-service caller is refused;
  • another service's request answers 404;
  • 8 concurrent marks give exactly one 200.
• Store tests, including the unbound-artifact case and concurrency.
• Body approval_ref refusal tested on the grant, human-admin and service paths.
• Each guard was removed in turn (state, artifact, target, requester, body refusal, server-side assignment, handler service check), and each time the matching tests failed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HuqYdXZiB63R9bmak1DWDN

[claude]

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

---

Review posted — 1 P2, 2 P3

• Read the pinned diff (2632664...b9af9bf, 18 files, +761/−31)
• Review store layer (internal/workitems/action_approvals.go) and migration
• Review handlers, auth and body-refusal paths
• Review OpenAPI / registry / docs consistency
• Post findings and verdict (CHANGES_REQUESTED, bound to b9af9bf)

Findings

P2 — approval_ref is missing OmitWhenEmpty: true (internal/operations/registry.go:966)
Declared optional with Default: "", so ApplyDefaults (registry.go:343) fills it and buildArgoParams (executor.go:497) forwards it on every mctl-agents-approve submit, not only grant-authorized ones. That is the exact hazard the OmitWhenEmpty doc comment at registry.go:79-88 describes, and the reason traceparent, agent_image/agent_version and every devLoopCorrelationParams entry set it. It is also what makes the mctl-gitops#2004 dependency a hard, non-reversible ordering: a rollback of that gitops change breaks all DevLoop approvals again.

P3 — approval_executed missing from the dev-loop approve route's documented 409 set (internal/api/handlers_dev_loop.go:678)
Adding executed to closedApprovalCodes is right, but openapi.yaml:3099-3100, openapi.yaml:3152 and the approval_consumed/approval_expired row in docs/principals.md:468 still list only denied/consumed/expired.

P3 — the sha256:<64 hex> guard narrows /execute to DevLoop-shaped approvals (internal/api/handlers_action_approvals.go:482)
Create accepts any artifact hash (checkText(..., false), no pattern) and the store only requires non-empty + equal, so an approval whose bound artifact is not a content digest can never be marked executed — 400 invalid_request instead of a typed 409. Harmless today; worth either relaxing to non-empty or documenting the restriction.

What holds up

Both compare-and-sets are correctly guarded and their loser paths are read-back-only, so the single-use claim survives the race (the losing racers deterministically read executed, which is why the racers-1 audit assertion is sound). executed was threaded through every state switch I could find: effectiveState, ValidApprovalState, the list filter's default branch, ConsumeActionApproval, ActionApprovalGrant and closedApprovalCodes. The body approval_ref refusal is correctly placed after StripUndeclared — where the now-declared key survives rather than being silently dropped — and covers the grant, human-admin and service paths with an audit entry on each. The in-place CHECK widening is guarded and idempotent across concurrent replica restarts (the second replica re-drops and re-adds under the same name). Test coverage is unusually complete, including the unbound-artifact case and 8-way concurrency at both the store and handler layers.

Not verified

go build, go vet and go test all required approval in this sandbox, so the verdict is from static review only. The new handler and store tests also skip without TEST_DATABASE_URL. I did confirm by inspection that every test helper the new files use (workItemsEnv.connStr/tenant/router/user, auditOps, auditRefusals, code, the four auth.New*User constructors) exists.
· branch feat/approval-grant-provenance

[claude]

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

P2: approval_ref is declared without OmitWhenEmpty, so ApplyDefaults + buildArgoParams send approval_ref="" on every mctl-agents-approve submit, not only grant-authorized ones. That is the hazard registry.go:79-88 documents, and it is what turns the mctl-gitops#2004 dependency into a hard, non-reversible deploy ordering. One field removes it; the grant path is unaffected.

P3: the new approval_executed code is reachable from the dev-loop approve route but missing from that route's documented 409 set (openapi.yaml:3099-3100, :3152, docs/principals.md:468); and the handler's sha256:<64 hex> guard on artifact_hash is stricter than the store/create contract, making /execute DevLoop-only without saying so.

The rest holds up. Both compare-and-sets are correctly guarded and the loser paths are read-back-only, so the single-use claim survives the race; executed was threaded through every state switch I could find (effectiveState, ValidApprovalState, the list filter's default branch, ConsumeActionApproval, ActionApprovalGrant, closedApprovalCodes); the body approval_ref refusal sits after StripUndeclared where the now-declared key survives, and covers grant, admin and service paths with an audit entry each; the in-place CHECK widening is guarded and idempotent across replica restarts. Test coverage is unusually complete, including the unbound-artifact case and 8-way concurrency at both layers.

Note: I could not run go build, go vet or go test in this sandbox (the Go toolchain required approval), so the verdict is from static review only.

[claude]

Claude finished @mashkovd's task in 2m 55s —— View job

---

Re-review of a669cda — approved, 0 P1/P2/P3

• Read the incremental commit (b9af9bf..a669cda)
• Verify the P2 (approval_ref / OmitWhenEmpty) is resolved
• Verify the two P3s (409 code set, artifact-hash guard)
• Re-check store layer, handlers and migration for new issues
• Post findings and verdict (APPROVE, bound to a669cda)

All three findings resolved

P2 — approval_ref now OmitWhenEmpty: true (internal/operations/registry.go:968)
ApplyDefaults (registry.go:330-339) deletes the key when it is absent or empty, so buildArgoParams no longer forwards approval_ref="" off the grant path. The grant path is unaffected: prepareApprovalGrant sets a non-empty aar_ id at handlers_write.go:139, before ApplyDefaults at :354, and ValidateInput still applies approvalRefPattern.

The body refusal is unchanged in behaviour, which is the part worth confirming: it is a presence check on the stripped input at handlers_write.go:132, running before ApplyDefaults, so a body sending approval_ref: "" is still refused rather than silently deleted by the new OmitWhenEmpty branch. TestApprovalGrant_BodyApprovalRefIsRefused covers "" explicitly. The off-the-grant-path assertion in handlers_write_approver_test.go now checks the key is absent and extends to the service principal as well as the human admin — and auth.NewServiceUser() is platformAdmin, so the 202 that test expects is right.

P3 — 409 code set approval_executed added to openapi.yaml:3099-3100, openapi.yaml:3152 and the typed-refusal table in docs/principals.md.

P3 — artifact-hash guard replaced by a non-empty check, which now tracks the store contract (checkText(..., MaxApprovalFieldBytes, true) plus equality against a non-empty stored hash). A non-digest bound artifact gets a typed 409 approval_target_mismatch instead of 400 invalid_request; OpenAPI relaxed to minLength: 1, and the refusal test moved the sha256:diff case from the 400 group to the 409 group. strings is still used in that file, so no unused import.

Still holds from the previous pass

Both compare-and-sets are correctly guarded and their loser paths are read-back-only; executed is threaded through effectiveState, ValidApprovalState, the list filter, ConsumeActionApproval, ActionApprovalGrant and closedApprovalCodes; the in-place CHECK widening is guarded and idempotent. No new issues in the incremental commit.

One stale doc note (non-blocking)

The PR body's Deploy order section no longer describes the code: with OmitWhenEmpty, only grant-authorized submits carry approval_ref. mctl-gitops#2004 is still needed before the grant path works, but it no longer gates ordinary human-admin or service approvals, and a rollback is survivable.

Not verified

go build, go vet and go test all required approval in this sandbox, so the verdict is from static review only. The new handler and store tests also skip without TEST_DATABASE_URL.
· branch feat/approval-grant-provenance

[claude]

No P1/P2 findings (0 P3). Good to merge.

All three findings from the review of b9af9bf are resolved in a669cda:

• P2 approval_ref / OmitWhenEmpty: the ParameterDef now sets OmitWhenEmpty: true, so ApplyDefaults (registry.go:330-339) deletes the key when absent or empty and buildArgoParams no longer forwards approval_ref="" off the grant path. The grant path is unaffected — prepareApprovalGrant sets a non-empty aar_ id before ApplyDefaults runs (handlers_write.go:139 then :354), and ValidateInput still checks the pattern. The body refusal is unchanged in behaviour: it is a presence check on the stripped input at handlers_write.go:132, before ApplyDefaults, so a body sending approval_ref: "" is still refused rather than silently deleted — TestApprovalGrant_BodyApprovalRefIsRefused covers that value explicitly. handlers_write_approver_test.go now asserts absence (not "") for human admin and service alike.
• P3 409 code set: approval_executed added to openapi.yaml:3099-3100, openapi.yaml:3152 and the typed-refusal table in docs/principals.md.
• P3 artifact-hash guard: the handler's sha256:<64 hex> check is replaced by a non-empty check, which tracks the store contract (checkText(..., MaxApprovalFieldBytes, true) plus equality against a non-empty stored hash), so a non-digest bound artifact now gets a typed 409 approval_target_mismatch instead of 400. OpenAPI relaxed to minLength: 1 and the refusal test moved the sha256:diff case from the 400 group to the 409 group; strings is still used in that file.

Everything else from the previous pass still holds: both compare-and-sets are correctly guarded with read-back-only loser paths, executed is threaded through effectiveState, ValidApprovalState, the list filter, ConsumeActionApproval, ActionApprovalGrant and closedApprovalCodes, and the in-place CHECK widening is guarded and idempotent.

Note: the PR body's "Deploy order" section is now stale — with OmitWhenEmpty only grant-authorized submits carry approval_ref, so mctl-gitops#2004 is still needed for the grant path but no longer gates ordinary approvals, and a rollback is survivable.

Note: go build, go vet and go test all required approval in this sandbox, so this verdict is from static review only.

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-high · commit b9af9bf.

Findings

Finding 1 (P2)

• Severity: P2
• File: internal/api/handlers_action_approval_execute_test.go:211
• Anchor: Anchor: + if _, err := pool.Exec(context.Background(), INSERT INTO action_approval_requests`
• Failure Scenario: In PostgreSQL, when INSERT INTO <table> SELECT ... omits an explicit target column list, PostgreSQL implicitly targets all columns defined on the table in ordinal position. The action_approval_requests table has 30 columns (20 original table columns, 9 columns added via preceding ALTER TABLE statements, and executed_at added in this pull request). Because the SELECT query specifies only 20 expressions, PostgreSQL raises ERROR: INSERT has more target columns than expressions (SQLSTATE 42601). As a result, pool.Exec returns an error, triggering t.Fatal(err) and unconditionally failing TestActionApprovals_OnlyTheRequestingServiceMarksExecuted.
• Proposed Fix: Explicitly specify the target column list in the INSERT INTO clause:

  	if _, err := pool.Exec(context.Background(), `INSERT INTO action_approval_requests (
  		id, execution_id, action_kind, target, args_digest, policy_rule_id, policy_version,
  		artifact_hash, work_item_id, idempotency_key, requested_by, intent_hash,
  		state, expires_at, created_at, decided_by, decided_at, reason, consumed_at, schema_version)
  		SELECT $2, execution_id, action_kind, target, args_digest, policy_rule_id, policy_version,
  		       artifact_hash, work_item_id, idempotency_key || '-other', 'service:someone-else', intent_hash,
  		       state, expires_at, created_at, decided_by, decided_at, reason, consumed_at, schema_version
  		FROM action_approval_requests WHERE id=$1`, id, other); err != nil {
  		t.Fatal(err)
  	}

---

Finding 2 (P3)

• Severity: P3
• File: internal/api/handlers_dev_loop.go:671
• Anchor: Anchor: + workitems.ApprovalExecuted: aarCodeExecuted,
• Failure Scenario: No test visible in this diff verifies that when a DevLoop's action approval is in state ApprovalExecuted, decideDevLoopApproval correctly maps it via closedApprovalCodes and returns HTTP 409 Conflict with code approval_executed.
• Proposed Fix: Add a test in the dev loop approval test suite verifying that attempting to decide an approval already in the executed state responds with HTTP 409 and code approval_executed.

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-high · commit a669cda.

Findings

[P2] Missing column list in test INSERT query causes SQL error 42601

• File: internal/api/handlers_action_approval_execute_test.go:212
• Anchor: Anchor: + if _, err := pool.Exec(context.Background(), INSERT INTO action_approval_requests`
• Concrete failure scenario:
  In TestActionApprovals_OnlyTheRequestingServiceMarksExecuted, the test sets up an approval request belonging to another service by running INSERT INTO action_approval_requests SELECT $2, ... without an explicit target column list. The action_approval_requests table has 30 columns (20 base columns + 9 principal/actor columns + 1 executed_at column added in this PR). When the target column list is omitted from an INSERT INTO table SELECT ... query, PostgreSQL expects the SELECT query to supply values for all 30 columns. Because the query only supplies 20 expressions, PostgreSQL fails query planning with ERROR: INSERT has more target columns than expressions (SQLSTATE 42601). pool.Exec returns this error and t.Fatal(err) aborts the test suite.
• Proposed fix:
  Explicitly specify the 20 target columns in the INSERT INTO action_approval_requests (...) clause so PostgreSQL can assign default/null values to the omitted columns:

  if _, err := pool.Exec(context.Background(), `INSERT INTO action_approval_requests (
  		id, execution_id, action_kind, target, args_digest, policy_rule_id, policy_version,
  		artifact_hash, work_item_id, idempotency_key, requested_by, intent_hash,
  		state, expires_at, created_at, decided_by, decided_at, reason, consumed_at, schema_version)
  	SELECT $2, execution_id, action_kind, target, args_digest, policy_rule_id, policy_version,
  	       artifact_hash, work_item_id, idempotency_key || '-other', 'service:someone-else', intent_hash,
  	       state, expires_at, created_at, decided_by, decided_at, reason, consumed_at, schema_version
  	FROM action_approval_requests WHERE id=$1`, id, other); err != nil {
  	t.Fatal(err)
  }

---

[P3] No test visible in this diff for executed DevLoop approval rejection

• File: internal/api/handlers_dev_loop.go:671
• Anchor: Anchor: + workitems.ApprovalExecuted: aarCodeExecuted,
• Concrete failure scenario:
  workitems.ApprovalExecuted was added to closedApprovalCodes so that decideDevLoopApproval rejects approval attempts with 409 approval_executed when the newest approval for the DevLoop has already been marked executed. However, no test visible in this diff covers this route branch when handling an executed grant.
• Proposed fix:
  Add a test case in the DevLoop approval tests ensuring that calling the approve route on a DevLoop workflow whose latest approval is in the executed state responds with HTTP 409 and error code approval_executed.

[mashkovd]

Re agy P2 (missing column list in the test INSERT ... SELECT): not a defect. PostgreSQL's INSERT takes the first N columns in declared order when the query supplies only N expressions and no column list is given, and fills the rest with their defaults. From the docs: "If no list of column names is given at all, the default is all the columns of the table in their declared order; or the first N column names, if there are only N columns supplied by the VALUES clause or query." The 20 expressions line up with the first 20 declared columns, and the added columns all come after them. TestActionApprovals_OnlyTheRequestingServiceMarksExecuted passes in this PR's test job on a669cda and locally on Postgres 16. Leaving it as is.