feat(approvals): t-paliad-216 SuggestChanges service method

ApprovalService.SuggestChanges is the fourth approval action — in one transaction: 1. Validates the OLD pending row (caller satisfies canApprove, lifecycle in update/complete only, counter differs from old.payload OR note is non-empty). 2. Closes the OLD row as 'changes_requested' with decision_note + counter_payload + decided_by + decided_at + decision_kind. 3. Reverts the entity from old.pre_image (reuses applyRevert — same code path Reject runs). 4. Runs the deadlock check for the NEW row (excluding the suggesting caller; original requester is no longer excluded). 5. Re-applies the counter_payload to the entity row (via applyEntityUpdate, mirroring the write-then-approve write). 6. INSERTs a NEW pending approval_requests row authored by the caller with previous_request_id pointing back at the OLD row. 7. Marks the entity pending + pending_request_id → new row. 8. Emits two project_events: *_approval_changes_suggested + a fresh *_approval_requested for the new row. 4-Augen still holds: the suggesting caller is the new row's requested_by, so self-approval on the new row is blocked by the standard 3-layer guard. The ORIGINAL requester is no longer the requested_by of the new row — if their profession satisfies the required_role they can now approve the counter themselves. Adds: - const RequestStatusChangesRequested = "changes_requested" - var ErrSuggestionRequiresChange = "suggestion requires counter diff or note" - var ErrSuggestionLifecycleInvalid = "suggest is only valid for update/complete" - models.ApprovalRequest.CounterPayload + PreviousRequestID - Per-row read paths (getRequestForUpdate, approvalRequestViewColumns) populate the new columns.
2026-05-20 09:48:25 +02:00
parent d8acbd613c
commit 705e1a2e79
3 changed files with 290 additions and 13 deletions
--- a/internal/models/models.go
+++ b/internal/models/models.go
@@ -805,6 +805,15 @@ type ApprovalRequest struct {
 	// alongside 👀 with a sparkle ✨ on the eye-pill surface.
 	RequesterKind  string          `db:"requester_kind"  json:"requester_kind"`
 	AgentTurnID    *uuid.UUID      `db:"agent_turn_id"   json:"agent_turn_id,omitempty"`
-	CreatedAt      time.Time       `db:"created_at"      json:"created_at"`
-	UpdatedAt      time.Time       `db:"updated_at"      json:"updated_at"`
+	// CounterPayload carries the approver's edited values on a
+	// changes_requested row (mig 103, t-paliad-216). NULL for every
+	// other status. Frontend renders it as a diff against the OLD
+	// payload to show "approver suggested X→Y on the following fields".
+	CounterPayload NullableJSON `db:"counter_payload" json:"counter_payload,omitempty"`
+	// PreviousRequestID is the back-pointer from a row spawned by
+	// SuggestChanges to the prior changes_requested row that birthed it
+	// (mig 103, t-paliad-216). NULL on first-attempt rows.
+	PreviousRequestID *uuid.UUID `db:"previous_request_id" json:"previous_request_id,omitempty"`
+	CreatedAt         time.Time  `db:"created_at"          json:"created_at"`
+	UpdatedAt         time.Time  `db:"updated_at"          json:"updated_at"`
 }
--- a/internal/services/approval_levels.go
+++ b/internal/services/approval_levels.go
@@ -61,11 +61,12 @@ const (

 // RequestStatus values on paliad.approval_requests.status.
 const (
-	RequestStatusPending    = "pending"
-	RequestStatusApproved   = "approved"
-	RequestStatusRejected   = "rejected"
-	RequestStatusRevoked    = "revoked"
-	RequestStatusSuperseded = "superseded"
+	RequestStatusPending          = "pending"
+	RequestStatusApproved         = "approved"
+	RequestStatusRejected         = "rejected"
+	RequestStatusRevoked          = "revoked"
+	RequestStatusSuperseded       = "superseded"
+	RequestStatusChangesRequested = "changes_requested"
 )

 // DecisionKind discriminates 'peer' (normal in-team sign-off) from
@@ -158,12 +159,14 @@ func IsValidResponsibility(r string) bool {
 //	ErrRequestNotPending       -> 409
 //	ErrUnknownEntityType       -> 500 (programming error)
 var (
-	ErrSelfApproval        = errors.New("self-approval blocked")
-	ErrNoQualifiedApprover = errors.New("no qualified approver available")
-	ErrConcurrentPending   = errors.New("entity already has a pending approval request")
-	ErrNotApprover         = errors.New("not authorized to approve this request")
-	ErrRequestNotPending   = errors.New("request is not pending")
-	ErrUnknownEntityType   = errors.New("unknown entity type")
+	ErrSelfApproval               = errors.New("self-approval blocked")
+	ErrNoQualifiedApprover        = errors.New("no qualified approver available")
+	ErrConcurrentPending          = errors.New("entity already has a pending approval request")
+	ErrNotApprover                = errors.New("not authorized to approve this request")
+	ErrRequestNotPending          = errors.New("request is not pending")
+	ErrUnknownEntityType          = errors.New("unknown entity type")
+	ErrSuggestionRequiresChange   = errors.New("suggestion requires a counter_payload diff or a note")
+	ErrSuggestionLifecycleInvalid = errors.New("suggest-changes is only valid for update / complete lifecycles")
 )

 // PendingApprovalError wraps ErrConcurrentPending with the in-flight
--- a/internal/services/approval_service.go
+++ b/internal/services/approval_service.go
@@ -35,6 +35,7 @@ package services
 //      pool, so the deadlock path can't be silently bypassed.

 import (
+	"bytes"
 	"context"
 	"database/sql"
 	"encoding/json"
@@ -363,6 +364,267 @@ func (s *ApprovalService) Revoke(ctx context.Context, requestID, callerID uuid.U
 	return s.decide(ctx, requestID, callerID, RequestStatusRevoked, "")
 }

+// SuggestChanges is the fourth approval action (t-paliad-216). The caller
+// proposes a counter-payload + optional free-text note; in one transaction
+// we close the old request as 'changes_requested', revert the entity from
+// pre_image, then immediately spawn a NEW 'pending' approval_request
+// authored by the caller carrying counter_payload as the new payload. The
+// new row enters the normal pending flow — anyone eligible (including the
+// original requester) can approve, reject, or suggest changes back on it.
+// 4-Augen still holds: the suggesting caller is now the new row's
+// requested_by, so self-approval is blocked by the standard 3-layer guard.
+//
+// Authorization is the same as Approve/Reject on the OLD row (canApprove).
+// The new row's deadlock check (qualified-approver-exists-other-than-
+// caller) runs before the new INSERT so we never spawn an unapprovable
+// request.
+//
+// counterPayload must differ from the old row's payload OR a non-empty
+// note must be present — a no-op suggestion (same values, no note) is
+// indistinguishable from "I have no opinion" and is rejected with
+// ErrSuggestionRequiresChange. counterPayload field shape is the same
+// allowlist used by Submit*/applyRevert (the date-bearing columns per
+// entity_type); unknown keys are silently dropped at apply time.
+//
+// SuggestChanges is only valid for lifecycle in (update, complete). For
+// create the original entity would be deleted by applyRevert, leaving no
+// row to apply a counter to. For delete the original is "remove this
+// entity" — a counter-proposal would be a different lifecycle entirely.
+// Both return ErrSuggestionLifecycleInvalid; the caller (handler) maps
+// it to 400.
+//
+// Returns the new request ID on success.
+func (s *ApprovalService) SuggestChanges(ctx context.Context, requestID, callerID uuid.UUID, counterPayload map[string]any, note string) (*uuid.UUID, error) {
+	trimmedNote := strings.TrimSpace(note)
+
+	tx, err := s.db.BeginTxx(ctx, nil)
+	if err != nil {
+		return nil, fmt.Errorf("begin tx: %w", err)
+	}
+	defer tx.Rollback() //nolint:errcheck
+
+	old, err := s.getRequestForUpdate(ctx, tx, requestID)
+	if err != nil {
+		return nil, err
+	}
+	if old.Status != RequestStatusPending {
+		return nil, fmt.Errorf("%w: status=%s", ErrRequestNotPending, old.Status)
+	}
+	if old.LifecycleEvent != LifecycleUpdate && old.LifecycleEvent != LifecycleComplete {
+		return nil, fmt.Errorf("%w: lifecycle=%s", ErrSuggestionLifecycleInvalid, old.LifecycleEvent)
+	}
+
+	// No-op guard: counter must differ from old.payload OR note must be present.
+	payloadDiffers, err := payloadsDiffer(old.Payload, counterPayload)
+	if err != nil {
+		return nil, err
+	}
+	if !payloadDiffers && trimmedNote == "" {
+		return nil, ErrSuggestionRequiresChange
+	}
+
+	// Authorization on the OLD row: caller must satisfy canApprove (same
+	// gate as Approve/Reject). Self-approval blocks here too.
+	decisionKind, err := s.canApprove(ctx, tx, callerID, old)
+	if err != nil {
+		return nil, err
+	}
+
+	now := time.Now().UTC()
+	counterJSON, err := marshalJSONOrNull(counterPayload)
+	if err != nil {
+		return nil, fmt.Errorf("marshal counter_payload: %w", err)
+	}
+
+	// Validate counter has at least one allowlisted field for the entity
+	// type — otherwise the entity-update below would be a no-op and the
+	// new row would just resubmit the SAME values, which is a degenerate
+	// case we should reject cleanly. Only run this check when the
+	// payload "differs" (i.e. caller actually provided something).
+	if payloadDiffers {
+		if _, _, err := buildRevertSetClauses(old.EntityType, counterPayload); err != nil {
+			// ErrUnknownEntityType wraps "empty pre_image for X" when no
+			// allowlisted key is present. Rebrand as suggestion-input
+			// failure for the handler's 400 mapping.
+			return nil, fmt.Errorf("%w: %v", ErrSuggestionRequiresChange, err)
+		}
+	}
+
+	// 1. Close the OLD row as changes_requested.
+	var noteArg any
+	if trimmedNote != "" {
+		noteArg = trimmedNote
+	}
+	updateOldSQL := `UPDATE paliad.approval_requests
+	    SET status = $1, decided_by = $2, decided_at = $3, decision_kind = $4,
+	        decision_note = $5, counter_payload = $6, updated_at = $3
+	  WHERE id = $7`
+	if _, err := tx.ExecContext(ctx, updateOldSQL,
+		RequestStatusChangesRequested, callerID, now, decisionKind,
+		noteArg, counterJSON, requestID); err != nil {
+		return nil, fmt.Errorf("close old request: %w", err)
+	}
+
+	// 2. Revert the entity from old.pre_image (same as Reject).
+	if err := s.applyRevert(ctx, tx, old); err != nil {
+		return nil, err
+	}
+
+	// 3. Deadlock check on the NEW row: someone other than the caller
+	//    must be qualified to approve. Original requester is no longer
+	//    excluded (they're a regular team member now from the new row's
+	//    POV), so they count if their role is sufficient.
+	ok, err := s.hasQualifiedApprover(ctx, tx, old.ProjectID, callerID, old.RequiredRole)
+	if err != nil {
+		return nil, err
+	}
+	if !ok {
+		return nil, fmt.Errorf("%w: required role %q", ErrNoQualifiedApprover, old.RequiredRole)
+	}
+
+	// 4. Re-apply the counter_payload to the entity row (write-then-approve).
+	//    Reuses buildRevertSetClauses (date-allowlist translation). Always
+	//    runs because we validated payloadDiffers + a valid set of keys
+	//    above; even when only a note was provided (payloadDiffers=false),
+	//    the original payload is re-applied for symmetry with Submit*.
+	applyPayload := counterPayload
+	if !payloadDiffers {
+		// Counter is identical to original — resubmit the same values as
+		// the new row's payload so the standard Submit* shape holds.
+		if err := json.Unmarshal(old.Payload, &applyPayload); err != nil {
+			return nil, fmt.Errorf("unmarshal original payload: %w", err)
+		}
+	}
+	if err := s.applyEntityUpdate(ctx, tx, old.EntityType, old.EntityID, applyPayload); err != nil {
+		return nil, err
+	}
+
+	// 5. INSERT the NEW pending row, authored by the caller, with
+	//    previous_request_id pointing back at the old row.
+	newID := uuid.New()
+	applyPayloadJSON, err := marshalJSONOrNull(applyPayload)
+	if err != nil {
+		return nil, fmt.Errorf("marshal new payload: %w", err)
+	}
+	insertNewSQL := `INSERT INTO paliad.approval_requests
+	    (id, project_id, entity_type, entity_id, lifecycle_event,
+	     pre_image, payload, requested_by, required_role, status,
+	     requester_kind, agent_turn_id, previous_request_id)
+	  VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, 'pending', 'user', NULL, $10)`
+	if _, err := tx.ExecContext(ctx, insertNewSQL,
+		newID, old.ProjectID, old.EntityType, old.EntityID, old.LifecycleEvent,
+		[]byte(old.PreImage), applyPayloadJSON, callerID, old.RequiredRole,
+		requestID); err != nil {
+		return nil, fmt.Errorf("insert new approval_request: %w", err)
+	}
+
+	// 6. Mark the entity pending pointing at the new row.
+	updateEntitySQL := fmt.Sprintf(`UPDATE paliad.%s
+	    SET approval_status = 'pending', pending_request_id = $1, updated_at = now()
+	  WHERE id = $2 AND approval_status IN ('approved','legacy')`,
+		entityTableName(old.EntityType))
+	res, err := tx.ExecContext(ctx, updateEntitySQL, newID, old.EntityID)
+	if err != nil {
+		return nil, fmt.Errorf("mark entity pending: %w", err)
+	}
+	rows, _ := res.RowsAffected()
+	if rows != 1 {
+		return nil, ErrConcurrentPending
+	}
+
+	// 7. Emit *_approval_changes_suggested for the OLD row's transition.
+	suggestedEvent := approvalEventType(old.EntityType, "changes_suggested")
+	suggestedDesc := approvalDescription("changes_suggested", old.RequiredRole, old.LifecycleEvent)
+	suggestedMeta := map[string]any{
+		"approval_request_id": requestID.String(),
+		"new_request_id":      newID.String(),
+		"lifecycle_event":     old.LifecycleEvent,
+		"decision_kind":       decisionKind,
+		old.EntityType + "_id": old.EntityID.String(),
+	}
+	if trimmedNote != "" {
+		suggestedMeta["decision_note"] = trimmedNote
+	}
+	if counterJSON != nil {
+		suggestedMeta["counter_payload"] = json.RawMessage(counterJSON)
+	}
+	if err := insertProjectEventWithMeta(ctx, tx, old.ProjectID, callerID, suggestedEvent, suggestedEvent, suggestedDesc, suggestedMeta); err != nil {
+		return nil, err
+	}
+
+	// 8. Emit *_approval_requested for the NEW row (same shape as Submit*).
+	requestedEvent := approvalEventType(old.EntityType, "requested")
+	requestedDesc := approvalDescription("requested", old.RequiredRole, old.LifecycleEvent)
+	requestedMeta := map[string]any{
+		"approval_request_id":  newID.String(),
+		"previous_request_id":  requestID.String(),
+		"lifecycle_event":      old.LifecycleEvent,
+		"required_role":        old.RequiredRole,
+		"requester_kind":       "user",
+		old.EntityType + "_id": old.EntityID.String(),
+	}
+	if err := insertProjectEventWithMeta(ctx, tx, old.ProjectID, callerID, requestedEvent, requestedEvent, requestedDesc, requestedMeta); err != nil {
+		return nil, err
+	}
+
+	if err := tx.Commit(); err != nil {
+		return nil, fmt.Errorf("commit: %w", err)
+	}
+	return &newID, nil
+}
+
+// applyEntityUpdate writes the allowlisted fields from payload onto the
+// entity row. Mirrors the write side of write-then-approve (which lives in
+// DeadlineService / AppointmentService for the user-driven path) — used
+// by SuggestChanges to apply an approver's counter-proposal back onto the
+// entity inside the same tx. Reuses buildRevertSetClauses for the
+// jsonb-key-to-SQL-SET translation so the allowlist is one source of
+// truth.
+func (s *ApprovalService) applyEntityUpdate(ctx context.Context, tx *sqlx.Tx, entityType string, entityID uuid.UUID, payload map[string]any) error {
+	if len(payload) == 0 {
+		return fmt.Errorf("%w: empty payload", ErrSuggestionRequiresChange)
+	}
+	setClauses, args, err := buildRevertSetClauses(entityType, payload)
+	if err != nil {
+		return err
+	}
+	setClauses = append(setClauses, "updated_at = now()")
+	args = append(args, entityID)
+	q := fmt.Sprintf(`UPDATE paliad.%s SET %s WHERE id = $%d`,
+		entityTableName(entityType), strings.Join(setClauses, ", "), len(args))
+	if _, err := tx.ExecContext(ctx, q, args...); err != nil {
+		return fmt.Errorf("apply counter payload to entity: %w", err)
+	}
+	return nil
+}
+
+// payloadsDiffer returns true iff the candidate counter map decodes to a
+// value that differs from the old row's payload jsonb. Used by
+// SuggestChanges to detect "no-op suggestion". Both NULL or both empty
+// map = identical → false. Comparison is by canonical re-marshal so
+// jsonb-key-ordering doesn't poison the equality check.
+func payloadsDiffer(old models.NullableJSON, candidate map[string]any) (bool, error) {
+	if len(candidate) == 0 && len(old) == 0 {
+		return false, nil
+	}
+	if len(candidate) == 0 || len(old) == 0 {
+		return true, nil
+	}
+	var oldMap map[string]any
+	if err := json.Unmarshal(old, &oldMap); err != nil {
+		return false, fmt.Errorf("unmarshal old payload: %w", err)
+	}
+	oldCanonical, err := json.Marshal(oldMap)
+	if err != nil {
+		return false, fmt.Errorf("re-marshal old payload: %w", err)
+	}
+	candCanonical, err := json.Marshal(candidate)
+	if err != nil {
+		return false, fmt.Errorf("marshal candidate payload: %w", err)
+	}
+	return !bytes.Equal(oldCanonical, candCanonical), nil
+}
+
 // decide is the shared kernel for Approve / Reject / Revoke. The decision
 // kind is derived from the (caller, request) relationship and the requested
 // final status:
@@ -692,6 +954,8 @@ func (s *ApprovalService) getRequestForUpdate(ctx context.Context, tx *sqlx.Tx,
 	q := `SELECT id, project_id, entity_type, entity_id, lifecycle_event,
 	             pre_image, payload, requested_by, requested_at, required_role,
 	             status, decided_by, decided_at, decision_kind, decision_note,
+	             requester_kind, agent_turn_id,
+	             counter_payload, previous_request_id,
 	             created_at, updated_at
 	        FROM paliad.approval_requests
 	       WHERE id = $1
@@ -875,6 +1139,7 @@ const approvalRequestViewColumns = `
    ar.pre_image, ar.payload, ar.requested_by, ar.requested_at, ar.required_role,
    ar.status, ar.decided_by, ar.decided_at, ar.decision_kind, ar.decision_note,
    ar.requester_kind, ar.agent_turn_id,
+    ar.counter_payload, ar.previous_request_id,
    ar.created_at, ar.updated_at,
    p.title AS project_title,
    CASE WHEN ar.entity_type = 'deadline'    THEN d.title