2026-06-13 03:51:44 +00:00
3 changed files with 173 additions and 9 deletions
@@ -546,11 +546,18 @@ func (s *RequestStore) AddMessage(ctx context.Context, id, authorType, authorID,
 		return "", fmt.Errorf("request: add message: %w", err)
 	}

-	// If the author is the recipient (the one being asked), this message is a
-	// "please clarify" — flip to info_requested so the requester is prompted.
+	// A message from anyone OTHER than the requester is a "please clarify" back
+	// TO the requester — flip to info_requested so the requester is prompted.
+	// We key off "not the requester" rather than "is the recipient" on purpose:
+	// an agent→user request is stored with an EMPTY recipient_id (the generic
+	// "the user"), but the canvas posts the reply with a concrete author_id
+	// (the session user_id, or the "admin" placeholder). A strict
+	// authorID==RecipientID match would never hold for that common case, so the
+	// flip (and the requester notification below) would silently never fire.
 	// Only flip a non-terminal request; a closed request keeps its terminal
 	// status even if a late note is appended.
-	if authorType == req.RecipientType && authorID == req.RecipientID {
+	authoredByRequester := authorType == req.RequesterType && authorID == req.RequesterID
+	if !authoredByRequester {
 		if _, err := s.db.ExecContext(ctx, `
 			UPDATE requests SET status = 'info_requested', updated_at = now()
 			WHERE id = $1 AND status IN ('pending', 'info_requested')
@@ -572,12 +579,15 @@ func (s *RequestStore) AddMessage(ctx context.Context, id, authorType, authorID,
 		}
 	}

-	// More-Info from the recipient must reach a requester AGENT as a real
+	// More-Info from the other party must reach a requester AGENT as a real
 	// turn (same rationale as the Respond notification — CTO 2026-06-11).
+	// Same "not the requester" gate as the flip above: the user's reply on an
+	// agent→user request carries a concrete author_id while recipient_id is
+	// empty, so we must NOT require authorID==RecipientID here. We only skip
+	// when the requester authored the message (it would be notifying itself).
 	// Keyed per message so a multi-round clarification thread delivers each
 	// ask; the requester replies with add_request_message.
-	if authorType == req.RecipientType && authorID == req.RecipientID &&
-		req.RequesterType == "agent" && req.RequesterID != "" {
+	if !authoredByRequester && req.RequesterType == "agent" && req.RequesterID != "" {
 		s.notifyRequesterAgent(ctx, req,
 			"request-message:"+messageID,
 			fmt.Sprintf("More info requested on your %s request %q (id %s): %s\nReply with add_request_message.",
@@ -1010,6 +1010,90 @@ func TestRequests_AddMessage_MoreInfo_NotifiesRequesterAgent(t *testing.T) {
 	}
 }

+// TestRequests_AddMessage_MoreInfo_GenericUserRecipient_NotifiesRequesterAgent
+// is the REAL production path: an agent→user request is stored with an EMPTY
+// recipient_id (the generic "the user"), but the canvas posts the More-Info
+// reply with a CONCRETE author_id (the session user_id, or the "admin"
+// placeholder when no session). The old gate (authorID == RecipientID) was
+// "admin" == "" → false, so neither the info_requested flip NOR the requester
+// notification ever fired — the agent silently never received the thread.
+// The fix keys off "not the requester", so this must now flip + notify.
+func TestRequests_AddMessage_MoreInfo_GenericUserRecipient_NotifiesRequesterAgent(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	handler := NewRequestsHandler(newTestBroadcaster())
+	captured := interceptRequestNotify(t)
+
+	// recipient_id is EMPTY (generic user); author_id is the "admin" placeholder.
+	mock.ExpectQuery("FROM requests WHERE id").
+		WithArgs("req-7").
+		WillReturnRows(oneRequestRow("req-7", "approval", "ws-agent-1", "user", "", "pending"))
+	mock.ExpectQuery("INSERT INTO request_messages").
+		WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("msg-7"))
+	// Must STILL flip to info_requested even though author_id != recipient_id.
+	mock.ExpectExec("UPDATE requests SET status = 'info_requested'").
+		WillReturnResult(sqlmock.NewResult(0, 1))
+	mock.ExpectExec("INSERT INTO structure_events").
+		WillReturnResult(sqlmock.NewResult(0, 1))
+
+	w := httptest.NewRecorder()
+	c, _ := gin.CreateTestContext(w)
+	c.Params = gin.Params{{Key: "requestId", Value: "req-7"}}
+	c.Request = httptest.NewRequest("POST", "/", bytesNewBufferStringHelper(`{"author_type":"user","author_id":"admin","body":"which secret did you mean?"}`))
+	c.Request.Header.Set("Content-Type", "application/json")
+
+	handler.AddMessage(c)
+
+	if w.Code != http.StatusCreated {
+		t.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String())
+	}
+	if len(*captured) != 1 {
+		t.Fatalf("expected 1 requester notification (generic-user recipient), got %d", len(*captured))
+	}
+	n := (*captured)[0]
+	if n["workspace_id"] != "ws-agent-1" || n["idem"] != "request-message:msg-7" {
+		t.Errorf("notification misrouted: %+v", n)
+	}
+	if !strings.Contains(n["body"], "which secret did you mean?") {
+		t.Errorf("notification body missing ask: %s", n["body"])
+	}
+}
+
+// TestRequests_AddMessage_RequesterReply_NoFlipNoNotify: when the REQUESTER
+// agent itself appends a message (replying to the clarification), it must NOT
+// flip to info_requested and must NOT notify itself.
+func TestRequests_AddMessage_RequesterReply_NoFlipNoNotify(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	handler := NewRequestsHandler(newTestBroadcaster())
+	captured := interceptRequestNotify(t)
+
+	// Requester is agent ws-agent-1; the author IS that requester.
+	mock.ExpectQuery("FROM requests WHERE id").
+		WithArgs("req-8").
+		WillReturnRows(oneRequestRow("req-8", "task", "ws-agent-1", "user", "", "info_requested"))
+	mock.ExpectQuery("INSERT INTO request_messages").
+		WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("msg-8"))
+	// No status-flip UPDATE expected (requester-authored).
+	mock.ExpectExec("INSERT INTO structure_events").
+		WillReturnResult(sqlmock.NewResult(0, 1))
+
+	w := httptest.NewRecorder()
+	c, _ := gin.CreateTestContext(w)
+	c.Params = gin.Params{{Key: "requestId", Value: "req-8"}}
+	c.Request = httptest.NewRequest("POST", "/", bytesNewBufferStringHelper(`{"author_type":"agent","author_id":"ws-agent-1","body":"I meant the staging DB secret."}`))
+	c.Request.Header.Set("Content-Type", "application/json")
+
+	handler.AddMessage(c)
+
+	if w.Code != http.StatusCreated {
+		t.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String())
+	}
+	if len(*captured) != 0 {
+		t.Fatalf("requester-authored message must NOT self-notify; got %d", len(*captured))
+	}
+}
+
 // TestRequests_Respond_NoNotifyForUserRequester: a request raised by a USER
 // must not enqueue an agent notification on respond.
 func TestRequests_Respond_NoNotifyForUserRequester(t *testing.T) {
@@ -110,8 +110,12 @@ func mintWorkspaceToken(t *testing.T, cfg stagingCfg, host, adminToken, orgID, w
 	return tok
 }

-// raiseRequest POSTs a task/approval to recipient=user as the workspace.
-func raiseRequest(t *testing.T, host, wsToken, orgID, wsID, kind, title string) {
+// raiseRequest POSTs a task/approval to recipient=user as the workspace and
+// returns the new request_id. Note recipient_id is EMPTY — the generic "the
+// user" — which is exactly what makes the More-Info reply path non-trivial
+// (the canvas posts the reply with a concrete author_id, see the More-Info
+// e2e below).
+func raiseRequest(t *testing.T, host, wsToken, orgID, wsID, kind, title string) string {
 	t.Helper()
 	url := "https://" + host + "/workspaces/" + wsID + "/requests"
 	body := fmt.Sprintf(
@@ -122,9 +126,75 @@ func raiseRequest(t *testing.T, host, wsToken, orgID, wsID, kind, title string)
 	if status != http.StatusCreated && status != http.StatusOK {
 		t.Fatalf("raise %s request (workspace token, wsAuth): HTTP %d: %s", kind, status, resp)
 	}
-	if id := jsonField(resp, "request_id"); id == "" {
+	id := jsonField(resp, "request_id")
+	if id == "" {
 		t.Fatalf("raise %s request: no request_id: %s", kind, resp)
 	}
+	return id
+}
+
+// TestWorkspaceRequestMoreInfoFlipsToInfoRequested is the live proof of the
+// "agent doesn't receive the More-Info thread" fix. An agent→user request is
+// stored with an EMPTY recipient_id; the canvas posts the user's clarification
+// reply with a CONCRETE author_id ("admin"/session user). The OLD gate
+// (authorID == RecipientID) was "admin" == "" → false, so the request never
+// flipped to info_requested and the requester agent was never notified. The
+// fixed gate keys off "not the requester", so the same POST must now flip the
+// status to info_requested — the server-observable proof that the gate fired
+// (and therefore that the requester-agent A2A notification was enqueued).
+func TestWorkspaceRequestMoreInfoFlipsToInfoRequested(t *testing.T) {
+	cfg := requireStagingEnv(t)
+	slug := fmt.Sprintf("e2e-moreinfo-%d", time.Now().Unix()%100000000)
+	t.Logf("more-info: slug=%s", slug)
+
+	orgID := adminCreateOrg(t, cfg, slug)
+	t.Cleanup(func() { adminDeleteTenant(t, cfg, slug) })
+	adminToken := tenantAdminToken(t, cfg, slug)
+	tenantHost := slug + "." + cfg.subdomainSuffix
+	waitForHTTP(t, tenantHost, http.StatusOK, 10*time.Minute, "tenant /health ready")
+
+	wsID := tenantCreateWorkspace(t, cfg, tenantHost, adminToken, orgID)
+	wsToken := mintWorkspaceToken(t, cfg, tenantHost, adminToken, orgID, wsID)
+
+	// Agent raises an approval to the generic user (recipient_id="").
+	apprTitle := "e2e-moreinfo approval " + slug
+	reqID := raiseRequest(t, tenantHost, wsToken, orgID, wsID, "approval", apprTitle)
+	t.Logf("raised request %s", reqID)
+
+	// The user posts a More-Info reply via the admin-gated canvas path with a
+	// concrete author_id (exactly what RequestsInbox.tsx sends: author_type
+	// "user", author_id = session user_id or the "admin" placeholder).
+	msgURL := "https://" + tenantHost + "/requests/" + reqID + "/messages"
+	msgBody := `{"author_type":"user","author_id":"admin","body":"which environment do you mean?"}`
+	status, resp := doTenantJSON(t, "POST", msgURL, adminToken, orgID, msgBody)
+	if status != http.StatusCreated && status != http.StatusOK {
+		t.Fatalf("post more-info message (admin path): HTTP %d: %s", status, resp)
+	}
+
+	// GET the request and assert the status flipped to info_requested. Under the
+	// pre-fix gate it would still read "pending".
+	getURL := "https://" + tenantHost + "/requests/" + reqID
+	deadline := time.Now().Add(30 * time.Second)
+	var lastStatus string
+	for time.Now().Before(deadline) {
+		code, body := doTenantJSON(t, "GET", getURL, adminToken, orgID, "")
+		if code == http.StatusOK {
+			var rt struct {
+				Request struct {
+					Status string `json:"status"`
+				} `json:"request"`
+			}
+			if json.Unmarshal([]byte(body), &rt) == nil {
+				lastStatus = rt.Request.Status
+				if lastStatus == "info_requested" {
+					t.Logf("request %s flipped to info_requested after user More-Info ✓", reqID)
+					return
+				}
+			}
+		}
+		time.Sleep(2 * time.Second)
+	}
+	t.Fatalf("request %s never flipped to info_requested (last status %q) — More-Info gate did not fire", reqID, lastStatus)
 }

 // requirePending polls GET /requests/pending?kind= until `title` appears.