From d60bc63f9ab80cf360d910ee75b6cf1f26f2d5bb Mon Sep 17 00:00:00 2001 From: Multica Eve Date: Tue, 4 Aug 2026 17:53:03 +0800 Subject: [PATCH] Revert "MUL-5516: fix(issues): re-trigger blocked issues when resumed (#6155)" (#6377) This reverts commit 738217c2756efb18c67a5ba1989298120d60a4f2. Re-firing an assigned agent when an Issue leaves `blocked` is not the product behaviour we want: `blocked` is a human-attention state, and clearing it is not by itself a signal to start work. Reverted on product grounds ahead of the v0.4.18 release, not because of a defect. Restores the pre-#6155 behaviour exactly: only `backlog` -> active promotes a parked Issue into a run, and the write/preview trigger probes go back to being separate. Requested by Bohan on MUL-5716. Co-authored-by: Eve Co-authored-by: multica-agent --- server/internal/handler/issue.go | 8 +- server/internal/handler/issue_trigger.go | 25 ++- .../handler/issue_trigger_preview_test.go | 210 +----------------- server/internal/service/issue_trigger.go | 39 ++-- 4 files changed, 39 insertions(+), 243 deletions(-) diff --git a/server/internal/handler/issue.go b/server/internal/handler/issue.go index 7b1fdc4852..7c296a3d73 100644 --- a/server/internal/handler/issue.go +++ b/server/internal/handler/issue.go @@ -2974,7 +2974,7 @@ func (h *Handler) UpdateIssue(w http.ResponseWriter, r *http.Request) { AssigneeChanged: assigneeChanged, StatusChanged: statusChanged, }, - h.issueTriggerProbe(r, actorType, actorID, workspaceID, issue), + h.issueTriggerWriteProbe(r, actorType, issue), ); ok && !req.SuppressRun { h.dispatchIssueRun(r.Context(), issue, trigger, actorType, actorID, req.HandoffNote) } @@ -3067,8 +3067,8 @@ func (h *Handler) validateAssigneePair(ctx context.Context, r *http.Request, wor // shouldEnqueueAgentTask returns true when an issue creation or assignment // should trigger the assigned agent. Backlog issues are skipped — backlog // acts as a parking lot where issues can be pre-assigned without immediately -// triggering execution. Moving out of backlog or blocked is handled separately -// in UpdateIssue. +// triggering execution. Moving out of backlog is handled separately in +// UpdateIssue. func (h *Handler) shouldEnqueueAgentTask(ctx context.Context, issue db.Issue) bool { if issue.Status == "backlog" { return false @@ -3472,7 +3472,7 @@ func (h *Handler) BatchUpdateIssues(w http.ResponseWriter, r *http.Request) { AssigneeChanged: assigneeChanged, StatusChanged: statusChanged, }, - h.issueTriggerProbe(r, actorType, actorID, workspaceID, issue), + h.issueTriggerWriteProbe(r, actorType, issue), ); ok && !req.Updates.SuppressRun { h.dispatchIssueRun(r.Context(), issue, trigger, actorType, actorID, req.Updates.HandoffNote) } diff --git a/server/internal/handler/issue_trigger.go b/server/internal/handler/issue_trigger.go index 12a1c3ce76..5d7c7cfb74 100644 --- a/server/internal/handler/issue_trigger.go +++ b/server/internal/handler/issue_trigger.go @@ -16,10 +16,25 @@ import ( // selection cannot fan out into thousands of readiness probes. const maxPreviewTriggerIssues = 500 -// issueTriggerProbe builds the request-scoped gates shared by preview and the -// real write paths. Status-only updates do not pass through validateAssigneePair, -// so the invocation gate must remain part of the enqueue decision itself. -func (h *Handler) issueTriggerProbe(r *http.Request, actorType, actorID, workspaceID string, issue db.Issue) service.IssueTriggerProbe { +// issueTriggerWriteProbe builds the probe the write paths feed to +// WillEnqueueRun. The private-agent gate is already enforced at the HTTP +// boundary (validateAssigneePair on assign) and inside enqueueSquadLeaderTask +// (canEnqueueSquadLeader), so a write must NOT re-run or sink it — it passes +// allow-all. The self-loop check needs the request's X-Task-ID header. +func (h *Handler) issueTriggerWriteProbe(r *http.Request, actorType string, issue db.Issue) service.IssueTriggerProbe { + return service.IssueTriggerProbe{ + CanAccessAgent: nil, // allow-all; gate lives at the write boundary + IsSelfLoop: func() bool { + return h.isAgentRunningOnIssue(r, actorType, issue) + }, + } +} + +// issueTriggerPreviewProbe mirrors the real write-time gates for the read-only +// preview: the private-agent gate (so preview never leaks a private agent's +// readiness to a member who cannot see it — matching validateAssigneePair / +// canEnqueueSquadLeader) and the same self-loop guard. +func (h *Handler) issueTriggerPreviewProbe(r *http.Request, actorType, actorID, workspaceID string, issue db.Issue) service.IssueTriggerProbe { originatorUserID := h.invokeOriginatorFromRequest(r, actorType, actorID) return service.IssueTriggerProbe{ CanAccessAgent: func(agent db.Agent) bool { @@ -143,7 +158,7 @@ func (h *Handler) PreviewIssueTrigger(w http.ResponseWriter, r *http.Request) { resp := IssueTriggerPreviewResponse{Triggers: make([]IssueTriggerPreviewItem, 0)} appendTrigger := func(issue db.Issue, in service.IssueTriggerInput) { - probe := h.issueTriggerProbe(r, actorType, actorID, workspaceID, issue) + probe := h.issueTriggerPreviewProbe(r, actorType, actorID, workspaceID, issue) if trigger, ok := h.IssueService.WillEnqueueRun(r.Context(), in, probe); ok { resp.Triggers = append(resp.Triggers, IssueTriggerPreviewItem{ IssueID: uuidToString(trigger.IssueID), diff --git a/server/internal/handler/issue_trigger_preview_test.go b/server/internal/handler/issue_trigger_preview_test.go index fcc925987d..22896e5e2d 100644 --- a/server/internal/handler/issue_trigger_preview_test.go +++ b/server/internal/handler/issue_trigger_preview_test.go @@ -24,13 +24,9 @@ func seededReadyAgentID(t *testing.T) string { } func previewIssueTrigger(t *testing.T, body map[string]any) IssueTriggerPreviewResponse { - return previewIssueTriggerAs(t, testUserID, body) -} - -func previewIssueTriggerAs(t *testing.T, userID string, body map[string]any) IssueTriggerPreviewResponse { t.Helper() w := httptest.NewRecorder() - req := newRequestAs(userID, "POST", "/api/issues/preview-trigger?workspace_id="+testWorkspaceID, body) + req := newRequest("POST", "/api/issues/preview-trigger?workspace_id="+testWorkspaceID, body) testHandler.PreviewIssueTrigger(w, req) if w.Code != http.StatusOK { t.Fatalf("PreviewIssueTrigger: expected 200, got %d: %s", w.Code, w.Body.String()) @@ -72,15 +68,6 @@ func taskCountFor(t *testing.T, issueID, agentID string) int { return n } -func assignIssueToAgentForTest(t *testing.T, issueID, agentID string) { - t.Helper() - if _, err := testPool.Exec(context.Background(), ` - UPDATE issue SET assignee_type = 'agent', assignee_id = $2 WHERE id = $1 - `, issueID, agentID); err != nil { - t.Fatalf("assign issue to agent: %v", err) - } -} - // TestPreviewIssueTrigger_CreateAgentVsBacklog covers the create entry point: // an active status with an agent assignee previews one run; the same assignee // parked in backlog previews none. @@ -151,201 +138,6 @@ func TestPreviewIssueTrigger_BatchAggregates(t *testing.T) { } } -// TestBlockedToTodoRestartsSquad verifies that once a squad-assigned issue is -// parked as blocked and its previous run has ended, moving it back to todo must -// preview and enqueue a fresh leader task. -func TestBlockedToTodoRestartsSquad(t *testing.T) { - ctx := context.Background() - leaderID := seededReadyAgentID(t) - - var squadID string - if err := testPool.QueryRow(ctx, ` - INSERT INTO squad (workspace_id, name, description, leader_id, creator_id) - VALUES ($1, $2, '', $3, $4) - RETURNING id - `, testWorkspaceID, "Blocked Recovery Squad", leaderID, testUserID).Scan(&squadID); err != nil { - t.Fatalf("create squad: %v", err) - } - t.Cleanup(func() { _, _ = testPool.Exec(ctx, `DELETE FROM squad WHERE id = $1`, squadID) }) - - issue := createIssueForTest(t, map[string]any{ - "title": "blocked squad recovery", - "status": "blocked", - }) - if _, err := testPool.Exec(ctx, ` - UPDATE issue SET assignee_type = 'squad', assignee_id = $2 WHERE id = $1 - `, issue.ID, squadID); err != nil { - t.Fatalf("assign blocked issue to squad: %v", err) - } - - preview := previewIssueTrigger(t, map[string]any{ - "issue_ids": []string{issue.ID}, - "status": "todo", - }) - if preview.TotalCount != 1 || len(preview.Triggers) != 1 { - t.Fatalf("blocked->todo preview: expected 1 trigger, got %+v", preview) - } - if trigger := preview.Triggers[0]; trigger.AgentID != leaderID || trigger.Source != "status" { - t.Fatalf("blocked->todo preview: wrong trigger %+v", trigger) - } - - w := httptest.NewRecorder() - req := withURLParam(newRequest("PUT", "/api/issues/"+issue.ID, map[string]any{ - "status": "todo", - }), "id", issue.ID) - testHandler.UpdateIssue(w, req) - if w.Code != http.StatusOK { - t.Fatalf("UpdateIssue blocked->todo: %d %s", w.Code, w.Body.String()) - } - if got := taskCountFor(t, issue.ID, leaderID); got != 1 { - t.Fatalf("blocked->todo: expected 1 squad-leader task, got %d", got) - } -} - -func TestBlockedToNonRunnableStatusDoesNotRestart(t *testing.T) { - agentID := seededReadyAgentID(t) - for _, status := range []string{"backlog", "done", "cancelled"} { - t.Run(status, func(t *testing.T) { - issue := createIssueForTest(t, map[string]any{ - "title": "blocked non-runnable " + status, - "status": "blocked", - }) - if _, err := testPool.Exec(context.Background(), ` - UPDATE issue SET assignee_type = 'agent', assignee_id = $2 WHERE id = $1 - `, issue.ID, agentID); err != nil { - t.Fatalf("assign blocked issue: %v", err) - } - - preview := previewIssueTrigger(t, map[string]any{ - "issue_ids": []string{issue.ID}, - "status": status, - }) - if preview.TotalCount != 0 { - t.Fatalf("blocked->%s: expected no trigger, got %+v", status, preview) - } - }) - } -} - -// TestBlockedPrivateAgentResumeRequiresInvokePermission covers status-only -// writes, which keep the existing assignee and therefore do not pass through -// validateAssigneePair. Preview and write must both deny an unrelated member, -// while still applying the requested issue status. -func TestBlockedPrivateAgentResumeRequiresInvokePermission(t *testing.T) { - agentID, _, memberID := privateAgentTestFixture(t) - - for _, status := range []string{"todo", "in_progress", "in_review"} { - t.Run(status, func(t *testing.T) { - issue := createIssueForTest(t, map[string]any{ - "title": "private agent resume denied " + status, - "status": "blocked", - }) - assignIssueToAgentForTest(t, issue.ID, agentID) - - preview := previewIssueTriggerAs(t, memberID, map[string]any{ - "issue_ids": []string{issue.ID}, - "status": status, - }) - if preview.TotalCount != 0 { - t.Fatalf("blocked->%s preview: expected no trigger, got %+v", status, preview) - } - - w := httptest.NewRecorder() - req := withURLParam(newRequestAs(memberID, "PUT", "/api/issues/"+issue.ID, map[string]any{ - "status": status, - }), "id", issue.ID) - testHandler.UpdateIssue(w, req) - if w.Code != http.StatusOK { - t.Fatalf("UpdateIssue blocked->%s: %d %s", status, w.Code, w.Body.String()) - } - var updated IssueResponse - if err := json.NewDecoder(w.Body).Decode(&updated); err != nil { - t.Fatalf("decode blocked->%s response: %v", status, err) - } - if updated.Status != status { - t.Fatalf("blocked->%s status was not applied: got %q", status, updated.Status) - } - if got := taskCountFor(t, issue.ID, agentID); got != 0 { - t.Fatalf("blocked->%s write disagreed with preview: enqueued %d tasks", status, got) - } - }) - } -} - -func TestBatchBlockedPrivateAgentResumeRequiresInvokePermission(t *testing.T) { - agentID, _, memberID := privateAgentTestFixture(t) - issues := []IssueResponse{ - createIssueForTest(t, map[string]any{"title": "private batch resume 1", "status": "blocked"}), - createIssueForTest(t, map[string]any{"title": "private batch resume 2", "status": "blocked"}), - } - issueIDs := make([]string, 0, len(issues)) - for _, issue := range issues { - assignIssueToAgentForTest(t, issue.ID, agentID) - issueIDs = append(issueIDs, issue.ID) - } - - preview := previewIssueTriggerAs(t, memberID, map[string]any{ - "issue_ids": issueIDs, - "status": "todo", - }) - if preview.TotalCount != 0 { - t.Fatalf("batch preview: expected no triggers, got %+v", preview) - } - - w := httptest.NewRecorder() - req := newRequestAs(memberID, "POST", "/api/issues/batch-update", map[string]any{ - "issue_ids": issueIDs, - "updates": map[string]any{"status": "todo"}, - }) - testHandler.BatchUpdateIssues(w, req) - if w.Code != http.StatusOK { - t.Fatalf("BatchUpdateIssues: %d %s", w.Code, w.Body.String()) - } - var batchResp struct { - Updated int `json:"updated"` - } - if err := json.NewDecoder(w.Body).Decode(&batchResp); err != nil { - t.Fatalf("decode batch response: %v", err) - } - if batchResp.Updated != len(issues) { - t.Fatalf("batch status update: expected %d updated, got %d", len(issues), batchResp.Updated) - } - for _, issue := range issues { - if got := taskCountFor(t, issue.ID, agentID); got != 0 { - t.Fatalf("batch write disagreed with preview for issue %s: enqueued %d tasks", issue.ID, got) - } - } -} - -func TestBlockedPrivateAgentResumeAllowsOwner(t *testing.T) { - agentID, ownerID, _ := privateAgentTestFixture(t) - issue := createIssueForTest(t, map[string]any{ - "title": "private agent owner resume", - "status": "blocked", - }) - assignIssueToAgentForTest(t, issue.ID, agentID) - - preview := previewIssueTriggerAs(t, ownerID, map[string]any{ - "issue_ids": []string{issue.ID}, - "status": "todo", - }) - if preview.TotalCount != 1 || len(preview.Triggers) != 1 { - t.Fatalf("owner preview: expected one trigger, got %+v", preview) - } - - w := httptest.NewRecorder() - req := withURLParam(newRequestAs(ownerID, "PUT", "/api/issues/"+issue.ID, map[string]any{ - "status": "todo", - }), "id", issue.ID) - testHandler.UpdateIssue(w, req) - if w.Code != http.StatusOK { - t.Fatalf("UpdateIssue as owner: %d %s", w.Code, w.Body.String()) - } - if got := taskCountFor(t, issue.ID, agentID); got != 1 { - t.Fatalf("owner write disagreed with preview: expected 1 task, got %d", got) - } -} - // TestPreviewIssueTrigger_MatchesWritePath is the core invariant: when preview // says a run will start, the real write path enqueues it; when preview says it // won't, the write path enqueues nothing. diff --git a/server/internal/service/issue_trigger.go b/server/internal/service/issue_trigger.go index e5264621d2..cbf6fd3308 100644 --- a/server/internal/service/issue_trigger.go +++ b/server/internal/service/issue_trigger.go @@ -15,22 +15,24 @@ const ( // RunSourceAssign covers issue creation and assignee changes — the issue // is being handed to an agent/squad. Parks silently on backlog. RunSourceAssign RunEnqueueSource = "assign" - // RunSourceStatus covers activating an already-assigned parked issue by - // moving it out of backlog or blocked into a runnable status. + // RunSourceStatus covers promoting an already-assigned issue out of + // backlog into an active status. RunSourceStatus RunEnqueueSource = "status" ) // IssueTriggerProbe carries the request-scoped checks WillEnqueueRun cannot // resolve from issue state alone. // -// CanAccessAgent is the invocation-permission gate shared by preview and write -// paths. Keeping it in this decision is required for status-only writes, which -// do not revalidate an unchanged assignee. A nil func is treated as allow-all -// for callers that have no request-scoped permission model. +// CanAccessAgent is the private-agent gate. The write paths enforce it at the +// HTTP boundary (validateAssigneePair on assign, canEnqueueSquadLeader inside +// the squad enqueue helper) and therefore pass an allow-all probe so the gate +// is never duplicated or sunk into the service layer. Preview passes the real +// gate so it never leaks a private agent's readiness to a member who cannot +// see it. A nil func is treated as allow-all. // -// IsSelfLoop reports whether activating this parked issue would be the calling -// agent re-triggering its own running task. Only the status source consults it; -// create and assign never do. A nil func means "not a self-loop". +// IsSelfLoop reports whether promoting this issue out of backlog would be the +// calling agent re-triggering its own running task. Only the status source +// consults it; create and assign never do. A nil func means "not a self-loop". type IssueTriggerProbe struct { CanAccessAgent func(agent db.Agent) bool IsSelfLoop func() bool @@ -75,7 +77,7 @@ func allowAllAgents(db.Agent) bool { return true } // CreateAgentTask, guarded by the (issue_id, agent_id) partial unique index // over pending (queued/dispatched) tasks; the pending check below mirrors that // guard, and only the status source needs it: -// - status source (parked → runnable) can re-fire against an assignee that +// - status source (backlog → active) can re-fire against an assignee that // already holds a pending task (e.g. one a @mention raised while the issue // sat in backlog); the check keeps preview from promising a run the unique // index would coalesce away. @@ -102,8 +104,8 @@ func (s *IssueService) WillEnqueueRun(ctx context.Context, in IssueTriggerInput, return IssueRunTrigger{}, false } source = RunSourceAssign - case in.StatusChanged && isParkedIssueStatus(in.PrevStatus) && - isRunnableIssueStatus(issue.Status): + case in.StatusChanged && in.PrevStatus == "backlog" && + issue.Status != "done" && issue.Status != "cancelled": if probe.IsSelfLoop != nil && probe.IsSelfLoop() { return IssueRunTrigger{}, false } @@ -163,19 +165,6 @@ func (s *IssueService) WillEnqueueRun(ctx context.Context, in IssueTriggerInput, return IssueRunTrigger{}, false } -func isParkedIssueStatus(status string) bool { - return status == "backlog" || status == "blocked" -} - -func isRunnableIssueStatus(status string) bool { - switch status { - case "todo", "in_progress", "in_review": - return true - default: - return false - } -} - // hasPendingRun reports whether the agent already holds a queued or dispatched // task for the issue (the (issue_id, agent_id) unique-index slot). Errors fail // closed to "pending" so preview never over-promises a run.