From 9559118678f3fa9725cce48a252c1430ddaeddab Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Wed, 29 Apr 2026 05:53:52 -0700 Subject: [PATCH] feat(activity): accept ?since_secs= for time-window filtering (#2268) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The harness runner (scripts/measure-coordinator-task-bounds-runner.sh) calls `/workspaces/:id/activity?since_secs=$A2A_TIMEOUT` to scope a trace to a specific test window. The query param was silently ignored — `ActivityHandler.List` accepted only `type`, `source`, and `limit`, so the runner got the most-recent-100 events regardless of how long ago they happened. Works for fresh-tenant tests where activity_logs is ~empty pre-run, breaks on busy tenants and on tests that exceed 100 events. Adds `since_secs` parsing with three behaviors: - Valid positive int → `AND created_at >= NOW() - make_interval(secs => $N)` on the SQL. Parameterised; values bound via lib/pq, not interpolated. `make_interval(secs => $N)` is required — the `INTERVAL '$N seconds'` literal form rejects placeholder substitution inside the string. - Above 30 days (2_592_000s) → silently clamped to the cap. Defends against a paranoid client triggering a multi-month full-table scan via `since_secs=999999999`. - Negative, zero, or non-integer → 400 with a structured error, NOT silently dropped. Silent drop is exactly the bug this is fixing — a typoed param shouldn't be lost as most-recent-100. Tests cover all four paths: accepted (with arg-binding assertion via sqlmock.WithArgs), clamped at 30 days, invalid rejected (5 sub-cases), and omitted (verifies no extra clause / arg leak via strict WithArgs count). RFC #2251 §V1.0 step 6 (platform-side-transition audit) also depends on this for time-window filtering of activity_logs. Closes #2268 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/handlers/activity.go | 38 +++- .../handlers/activity_since_secs_test.go | 163 ++++++++++++++++++ 2 files changed, 200 insertions(+), 1 deletion(-) create mode 100644 workspace-server/internal/handlers/activity_since_secs_test.go diff --git a/workspace-server/internal/handlers/activity.go b/workspace-server/internal/handlers/activity.go index 57adf78e..0ae9f1b8 100644 --- a/workspace-server/internal/handlers/activity.go +++ b/workspace-server/internal/handlers/activity.go @@ -23,12 +23,22 @@ func NewActivityHandler(b *events.Broadcaster) *ActivityHandler { return &ActivityHandler{broadcaster: b} } -// List handles GET /workspaces/:id/activity?type=&limit= +// List handles GET /workspaces/:id/activity?type=&source=&limit=&since_secs= +// +// since_secs filters to activity_logs.created_at >= NOW() - INTERVAL '$N seconds'. +// Optional, additive — callers that don't pass it get today's behavior (the +// most-recent N events regardless of time). The harness runner +// (scripts/measure-coordinator-task-bounds-runner.sh) uses this to scope a +// trace to a specific test window; RFC #2251 §V1.0 step 6 also depends on it. +// Capped at 30 days (2_592_000s) — anything older has typically been paged +// out anyway, and a defensive ceiling keeps a paranoid client from triggering +// a full-table scan via since_secs=99999999999. Closes #2268. func (h *ActivityHandler) List(c *gin.Context) { workspaceID := c.Param("id") activityType := c.Query("type") source := c.Query("source") // "canvas" = source_id IS NULL, "agent" = source_id IS NOT NULL limitStr := c.DefaultQuery("limit", "100") + sinceSecsStr := c.Query("since_secs") limit := 100 if n, err := strconv.Atoi(limitStr); err == nil && n > 0 { @@ -38,6 +48,23 @@ func (h *ActivityHandler) List(c *gin.Context) { } } + // Parse since_secs. Reject negative or non-integer values rather than + // silently ignoring them — a typoed param shouldn't be lost as + // most-recent-100, that's exactly the bug this fixes. + var sinceSecs int + if sinceSecsStr != "" { + n, err := strconv.Atoi(sinceSecsStr) + if err != nil || n <= 0 { + c.JSON(http.StatusBadRequest, gin.H{"error": "since_secs must be a positive integer"}) + return + } + const maxSinceSecs = 30 * 24 * 60 * 60 // 30 days + if n > maxSinceSecs { + n = maxSinceSecs + } + sinceSecs = n + } + // Build query with optional filters query := `SELECT id, workspace_id, activity_type, source_id, target_id, method, summary, request_body, response_body, tool_trace, duration_ms, status, error_detail, created_at @@ -58,6 +85,15 @@ func (h *ActivityHandler) List(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": "source must be 'canvas' or 'agent'"}) return } + if sinceSecs > 0 { + // Use a parameterized interval so the value is bound, not + // interpolated into the SQL string. `make_interval(secs => $N)` + // avoids the lib/pq quirk where INTERVAL '$N seconds' won't + // substitute a placeholder inside the literal. + query += fmt.Sprintf(" AND created_at >= NOW() - make_interval(secs => $%d)", argIdx) + args = append(args, sinceSecs) + argIdx++ + } query += fmt.Sprintf(" ORDER BY created_at DESC LIMIT $%d", argIdx) args = append(args, limit) diff --git a/workspace-server/internal/handlers/activity_since_secs_test.go b/workspace-server/internal/handlers/activity_since_secs_test.go new file mode 100644 index 00000000..2b0ae511 --- /dev/null +++ b/workspace-server/internal/handlers/activity_since_secs_test.go @@ -0,0 +1,163 @@ +package handlers + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/gin-gonic/gin" +) + +// Tests for the since_secs query parameter on GET /workspaces/:id/activity. +// Closes #2268 — the harness runner was passing this param and it was +// silently ignored, capping the trace at most-recent-100 events. The new +// shape: parse since_secs, add a parameterised `created_at >= NOW() - +// make_interval(secs => $N)` clause, cap at 30 days, reject invalid input +// with 400. + +const activityCols = `id, workspace_id, activity_type, source_id, target_id, method, ` + + `summary, request_body, response_body, tool_trace, duration_ms, status, error_detail, created_at` + +func newActivityRows() *sqlmock.Rows { + cols := []string{ + "id", "workspace_id", "activity_type", "source_id", "target_id", "method", + "summary", "request_body", "response_body", "tool_trace", "duration_ms", "status", "error_detail", "created_at", + } + return sqlmock.NewRows(cols). + AddRow("act-1", "ws-1", "a2a_send", nil, nil, nil, + "sent", nil, nil, nil, nil, "ok", nil, + time.Date(2026, 4, 29, 10, 0, 0, 0, time.UTC)) +} + +// TestActivityHandler_SinceSecs_Accepted verifies that a valid since_secs +// query param adds the make_interval clause to the SQL with the parsed +// value as a bound parameter — exactly what the runner needs to scope a +// trace to a test window. +func TestActivityHandler_SinceSecs_Accepted(t *testing.T) { + mock := setupTestDB(t) + + mock.ExpectQuery("SELECT id, workspace_id, activity_type"). + WithArgs("ws-1", 600, 100). // workspaceID, since_secs, limit + WillReturnRows(newActivityRows()) + + broadcaster := newTestBroadcaster() + handler := NewActivityHandler(broadcaster) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + c.Request = httptest.NewRequest("GET", "/workspaces/ws-1/activity?since_secs=600", nil) + + handler.List(c) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestActivityHandler_SinceSecs_ClampedAt30Days verifies the defensive +// ceiling so a paranoid client can't trigger a multi-month full-table +// scan via since_secs=999999999. +func TestActivityHandler_SinceSecs_ClampedAt30Days(t *testing.T) { + mock := setupTestDB(t) + + const cap30Days = 30 * 24 * 60 * 60 + mock.ExpectQuery("SELECT id, workspace_id, activity_type"). + WithArgs("ws-1", cap30Days, 100). + WillReturnRows(newActivityRows()) + + broadcaster := newTestBroadcaster() + handler := NewActivityHandler(broadcaster) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + c.Request = httptest.NewRequest("GET", "/workspaces/ws-1/activity?since_secs=999999999", nil) + + handler.List(c) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestActivityHandler_SinceSecs_InvalidRejected covers the loud-fail path: +// a typoed param (non-int, zero, negative) returns 400 instead of being +// silently dropped — that's the bug this whole feature is fixing. +func TestActivityHandler_SinceSecs_InvalidRejected(t *testing.T) { + cases := []struct { + name string + val string + }{ + {"non-integer", "abc"}, + {"zero", "0"}, + {"negative", "-1"}, + {"hex-prefix", "0x10"}, + {"float", "60.5"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // No DB call expected; bad input must be caught before the query. + setupTestDB(t) + broadcaster := newTestBroadcaster() + handler := NewActivityHandler(broadcaster) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + c.Request = httptest.NewRequest("GET", + "/workspaces/ws-1/activity?since_secs="+tc.val, nil) + + handler.List(c) + + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400 for %q, got %d: %s", tc.val, w.Code, w.Body.String()) + } + var resp map[string]string + _ = json.Unmarshal(w.Body.Bytes(), &resp) + if resp["error"] == "" { + t.Errorf("expected error message in response body for %q", tc.val) + } + }) + } +} + +// TestActivityHandler_SinceSecs_Omitted verifies backward compat — callers +// that don't pass since_secs see the original behavior (no extra WHERE +// clause, just workspace_id + limit). +func TestActivityHandler_SinceSecs_Omitted(t *testing.T) { + mock := setupTestDB(t) + + // Only workspace_id + limit; the query must NOT include the + // make_interval clause. sqlmock's WithArgs is strict on count, so a + // since_secs leak would surface as "expected 2 args, got 3". + mock.ExpectQuery("SELECT id, workspace_id, activity_type"). + WithArgs("ws-1", 100). + WillReturnRows(newActivityRows()) + + broadcaster := newTestBroadcaster() + handler := NewActivityHandler(broadcaster) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + c.Request = httptest.NewRequest("GET", "/workspaces/ws-1/activity", nil) + + handler.List(c) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +}