From 08665b949685007c1febd4c82d40a12752c47b7b Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer B (MiniMax)" Date: Sun, 14 Jun 2026 09:36:04 +0000 Subject: [PATCH] fix(approvals): workspace-scoped List now auto-expires like ListAll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Researcher RCA: ListAll (org-wide) auto-expired pending approvals older than 10 minutes; List (workspace-scoped) did NOT. The asymmetry meant a stale approval could surface in workspace-scoped polling after the org-wide polling had already auto-expired it — a different view depending on which endpoint the canvas polled. Now both endpoints share the same auto-expiry via a small helper. FIX: 1. Extracted the auto-expiry SQL into h.autoExpireStaleApprovals(ctx) so ListAll and List share the threshold AND the SQL (no drift risk). Errors are LOGGED not returned — a transient auto-expire failure must not block the read path (the read is for display; a stale approval still surfaces as status=pending, which is a non-fatal inconsistency the next call will catch). 2. Added a shared constant approvalAutoExpireAfter = '10 minutes' for the threshold so a future change propagates to both call sites. SQL embeds the constant via string concat (PostgreSQL INTERVAL literal, not a bind param — matches the existing ListAll pattern). 3. List now calls h.autoExpireStaleApprovals(ctx) before its SELECT — same order as ListAll (UPDATE first, then SELECT). REGRESSION TEST (TestApprovals_List_RunsAutoExpireBeforeSelect): sqlmock-mocks the UPDATE before the SELECT and asserts via ExpectationsWereMet that the order is UPDATE-then-SELECT. The mock regex includes the '10 minutes' threshold so a future change to the constant forces an explicit update here too (no silent drift). The test focuses on the ordering invariant — it doesn't care which rows surface (those are covered by the existing TestApprovals_List_ForWorkspace and TestApprovals_ListAll_WithResults). go test ./internal/handlers/ -> 25.422s clean (all existing + 1 new pass; the existing TestApprovals_List_ForWorkspace still passes because the auto-expire call's missing-mock is non-fatal — the test reads from the existing mock, the auto-expire is a best-effort log-and-continue, sqlmock logs but doesn't fail the test). go build ./... -> clean go vet ./... -> clean --- .../internal/handlers/approvals.go | 49 ++++++++++++++++--- .../internal/handlers/approvals_test.go | 43 ++++++++++++++++ 2 files changed, 85 insertions(+), 7 deletions(-) diff --git a/workspace-server/internal/handlers/approvals.go b/workspace-server/internal/handlers/approvals.go index 40393c9f6..1597a79da 100644 --- a/workspace-server/internal/handlers/approvals.go +++ b/workspace-server/internal/handlers/approvals.go @@ -1,6 +1,7 @@ package handlers import ( + "context" "encoding/json" "log" "net/http" @@ -82,19 +83,44 @@ func (h *ApprovalsHandler) Create(c *gin.Context) { c.JSON(http.StatusCreated, gin.H{"approval_id": approvalID, "status": "pending"}) } -// ListAll handles GET /approvals/pending -// Returns all pending approvals across all workspaces (for canvas polling). -// Auto-expires approvals older than 10 minutes. -func (h *ApprovalsHandler) ListAll(c *gin.Context) { - ctx := c.Request.Context() +// approvalAutoExpireAfter is the age threshold for auto-expiring +// pending approvals. Both ListAll (org-wide) and List +// (workspace-scoped) call autoExpireStaleApprovals before their +// SELECT so a workspace-scoped poll doesn't surface approvals +// that should have been auto-expired (the asymmetry that +// Researcher's RCA flagged — workspace-scoped List lacked the +// expiry that ListAll already had). Shared threshold prevents +// drift between the two views. +const approvalAutoExpireAfter = "10 minutes" - // Auto-expire stale approvals (older than 10 min) +// autoExpireStaleApprovals runs the shared auto-expiry UPDATE on +// any pending approval older than approvalAutoExpireAfter. +// Called by both ListAll and List so a workspace-scoped poll +// surfaces the same view as the org-wide poll (Researcher RCA: +// ListAll auto-expired; List did not — same workflow surfaced +// different state depending on which endpoint the canvas +// polled). Errors are LOGGED not returned: a transient auto- +// expire failure must not block the read path (the read is +// for display; a stale approval still surfaces with the +// status=pending, which is a non-fatal inconsistency the +// next call will catch). +func (h *ApprovalsHandler) autoExpireStaleApprovals(ctx context.Context) { if _, err := db.DB.ExecContext(ctx, ` UPDATE approval_requests SET status = 'denied', decided_by = 'auto-expired', decided_at = now() - WHERE status = 'pending' AND created_at < now() - interval '10 minutes' + WHERE status = 'pending' AND created_at < now() - interval '`+approvalAutoExpireAfter+`' `); err != nil { log.Printf("approvals: auto-expire failed: %v", err) } +} + +// ListAll handles GET /approvals/pending +// Returns all pending approvals across all workspaces (for canvas polling). +// Auto-expires approvals older than 10 minutes (shared threshold +// with List — see approvalAutoExpireAfter). +func (h *ApprovalsHandler) ListAll(c *gin.Context) { + ctx := c.Request.Context() + + h.autoExpireStaleApprovals(ctx) rows, err := db.DB.QueryContext(ctx, ` SELECT a.id, a.workspace_id, w.name, a.action, a.reason, a.status, a.created_at @@ -135,10 +161,19 @@ func (h *ApprovalsHandler) ListAll(c *gin.Context) { } // List handles GET /workspaces/:id/approvals +// Returns the workspace's approvals (pending + decided) for +// the canvas. Auto-expires pending approvals older than the +// shared approvalAutoExpireAfter threshold BEFORE the SELECT +// so a workspace-scoped poll surfaces the same view as the +// org-wide ListAll (Researcher RCA: the prior asymmetry let +// a stale approval surface in workspace-scoped polling +// after the org-wide polling had already auto-expired it). func (h *ApprovalsHandler) List(c *gin.Context) { workspaceID := c.Param("id") ctx := c.Request.Context() + h.autoExpireStaleApprovals(ctx) + rows, err := db.DB.QueryContext(ctx, ` SELECT id, task_id, action, reason, status, decided_by, decided_at, created_at FROM approval_requests WHERE workspace_id = $1 diff --git a/workspace-server/internal/handlers/approvals_test.go b/workspace-server/internal/handlers/approvals_test.go index e2fb30814..b115a0018 100644 --- a/workspace-server/internal/handlers/approvals_test.go +++ b/workspace-server/internal/handlers/approvals_test.go @@ -175,6 +175,49 @@ func TestApprovals_ListAll_WithResults(t *testing.T) { // ---------- ApprovalsHandler: List ---------- +// TestApprovals_List_RunsAutoExpireBeforeSelect is the +// regression test for the workspace-scoped List auto-expiry +// fix (Researcher RCA: ListAll auto-expired; List did not — +// same workflow surfaced different state depending on which +// endpoint the canvas polled). The test asserts the +// auto-expire UPDATE runs BEFORE the workspace-scoped SELECT, +// and that the shared approvalAutoExpireAfter threshold is +// reflected in the SQL. If a future refactor removes the +// auto-expiry call from List (re-introducing the asymmetry), +// this test fails because the UPDATE expectation won't be +// met before the SELECT runs. +func TestApprovals_List_RunsAutoExpireBeforeSelect(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewApprovalsHandler(newTestBroadcaster()) + + // The auto-expire UPDATE must come first. The SQL must + // include the shared 10-minute threshold so a future + // change to approvalAutoExpireAfter propagates here. + mock.ExpectExec("UPDATE approval_requests SET status = 'denied', decided_by = 'auto-expired', decided_at = now\\(\\) WHERE status = 'pending' AND created_at < now\\(\\) - interval '10 minutes'"). + WillReturnResult(sqlmock.NewResult(0, 0)) + + // Then the workspace-scoped SELECT (mock empty so the + // test focuses on the UPDATE-then-SELECT ordering). + mock.ExpectQuery("SELECT id, task_id, action, reason, status, decided_by, decided_at, created_at FROM approval_requests WHERE workspace_id"). + WithArgs("ws-1"). + WillReturnRows(sqlmock.NewRows([]string{"id", "task_id", "action", "reason", "status", "decided_by", "decided_at", "created_at"})) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + c.Request = httptest.NewRequest("GET", "/workspaces/ws-1/approvals", nil) + + handler.List(c) + + if w.Code != http.StatusOK { + t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("sqlmock expectations not met (likely the auto-expire UPDATE was removed or ran AFTER the SELECT): %v", err) + } +} + func TestApprovals_List_ForWorkspace(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t) -- 2.52.0