From 65269d93919f6758917c9cf4d367bd179b5c4325 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Wed, 15 Apr 2026 00:23:46 -0700 Subject: [PATCH 1/2] fix: #93 category_routing + #105 X-RateLimit headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #93 and #105. #93 — add research/plugins/template/channels entries to org.yaml category_routing defaults. Without them, evolution crons firing with these categories found no target and their audit summaries silently dropped at PM. Routes each back to the role that generated it so the author acts on their own findings. #105 — emit X-RateLimit-Limit / -Remaining / -Reset on every response (allowed and throttled) and Retry-After on 429s per RFC 6585. 2 tests cover both paths. Clients and monitoring tools can now back off proactively instead of polling into 429 walls. Co-Authored-By: Claude Opus 4.6 (1M context) --- org-templates/molecule-dev/org.yaml | 10 +++ platform/internal/middleware/ratelimit.go | 25 ++++++- .../internal/middleware/ratelimit_test.go | 72 +++++++++++++++++++ 3 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 platform/internal/middleware/ratelimit_test.go diff --git a/org-templates/molecule-dev/org.yaml b/org-templates/molecule-dev/org.yaml index 66b0e134b..ca79f8f7f 100644 --- a/org-templates/molecule-dev/org.yaml +++ b/org-templates/molecule-dev/org.yaml @@ -53,6 +53,16 @@ defaults: performance: [Backend Engineer] docs: [Documentation Specialist] mixed: [Dev Lead] + # Evolution-cron categories (#93): these four are fired by hourly + # self-review schedules (Research Lead, Technical Researcher, Dev Lead, + # DevOps Engineer). Routing them to the same role that generated them + # is a safe default — it converts the summary into a delegation back + # to the author so they act on their own findings. Override per-org + # if you want a different fan-out. + research: [Research Lead] + plugins: [Technical Researcher] + template: [Dev Lead] + channels: [DevOps Engineer] # workspace_dir: not set by default — each agent gets an isolated Docker volume # Set per-workspace to bind-mount a host directory as /workspace diff --git a/platform/internal/middleware/ratelimit.go b/platform/internal/middleware/ratelimit.go index 7b4a8eba4..0e6077626 100644 --- a/platform/internal/middleware/ratelimit.go +++ b/platform/internal/middleware/ratelimit.go @@ -4,6 +4,7 @@ package middleware import ( "context" "net/http" + "strconv" "sync" "time" @@ -71,11 +72,33 @@ func (rl *RateLimiter) Middleware() gin.HandlerFunc { b.lastReset = time.Now() } + // Issue #105 — advertise the current bucket state so clients and + // monitoring tools can back off proactively. Headers are set on every + // response (both allowed and throttled) so they're observable against + // any endpoint — /health, /metrics, and every /workspaces/* route. + // + // The `reset` value is seconds until the current bucket refills, + // matching the RFC 6585 Retry-After spec for 429 responses and the + // de-facto X-RateLimit-Reset convention (GitHub, Stripe, etc.). + remaining := b.tokens - 1 + if remaining < 0 { + remaining = 0 + } + resetSeconds := int(time.Until(b.lastReset.Add(rl.interval)).Seconds()) + if resetSeconds < 0 { + resetSeconds = 0 + } + c.Header("X-RateLimit-Limit", strconv.Itoa(rl.rate)) + c.Header("X-RateLimit-Remaining", strconv.Itoa(remaining)) + c.Header("X-RateLimit-Reset", strconv.Itoa(resetSeconds)) + if b.tokens <= 0 { rl.mu.Unlock() + // Retry-After is the canonical 429 signal per RFC 6585. + c.Header("Retry-After", strconv.Itoa(resetSeconds)) c.JSON(http.StatusTooManyRequests, gin.H{ "error": "rate limit exceeded", - "retry_after": rl.interval.Seconds(), + "retry_after": resetSeconds, }) c.Abort() return diff --git a/platform/internal/middleware/ratelimit_test.go b/platform/internal/middleware/ratelimit_test.go new file mode 100644 index 000000000..4f17ce562 --- /dev/null +++ b/platform/internal/middleware/ratelimit_test.go @@ -0,0 +1,72 @@ +package middleware + +import ( + "context" + "net/http" + "net/http/httptest" + "strconv" + "testing" + "time" + + "github.com/gin-gonic/gin" +) + +// newTestLimiter spins up a tiny limiter with a 2-token/5s budget so tests can +// exhaust + recover without real-time delays. +func newTestLimiter(t *testing.T) (*RateLimiter, *gin.Engine) { + t.Helper() + gin.SetMode(gin.TestMode) + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + rl := NewRateLimiter(2, 5*time.Second, ctx) + r := gin.New() + r.Use(rl.Middleware()) + r.GET("/x", func(c *gin.Context) { c.String(http.StatusOK, "ok") }) + return rl, r +} + +// TestRateLimit_HeadersPresentOnAllowedRequest covers issue #105 — every +// response (not just 429s) must carry the X-RateLimit-* triplet so clients +// can back off proactively. +func TestRateLimit_HeadersPresentOnAllowedRequest(t *testing.T) { + _, r := newTestLimiter(t) + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/x", nil)) + + if got := w.Header().Get("X-RateLimit-Limit"); got != "2" { + t.Errorf("X-RateLimit-Limit = %q, want 2", got) + } + if got := w.Header().Get("X-RateLimit-Remaining"); got != "1" { + t.Errorf("X-RateLimit-Remaining = %q, want 1", got) + } + reset, err := strconv.Atoi(w.Header().Get("X-RateLimit-Reset")) + if err != nil || reset < 0 || reset > 5 { + t.Errorf("X-RateLimit-Reset = %q, want 0-5", w.Header().Get("X-RateLimit-Reset")) + } +} + +// TestRateLimit_RetryAfterOn429 — throttled responses must carry Retry-After +// per RFC 6585, so curl/fetch clients back off the exact required window. +func TestRateLimit_RetryAfterOn429(t *testing.T) { + _, r := newTestLimiter(t) + // Burn through both tokens. + for i := 0; i < 2; i++ { + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/x", nil)) + if w.Code != http.StatusOK { + t.Fatalf("request %d: want 200, got %d", i+1, w.Code) + } + } + // Third should 429. + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/x", nil)) + if w.Code != http.StatusTooManyRequests { + t.Fatalf("3rd request: want 429, got %d", w.Code) + } + if got := w.Header().Get("Retry-After"); got == "" { + t.Error("missing Retry-After header on 429") + } + if got := w.Header().Get("X-RateLimit-Remaining"); got != "0" { + t.Errorf("X-RateLimit-Remaining = %q on 429, want 0", got) + } +} From 97cbfc08da62914208c00e010b9464f704480d08 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Wed, 15 Apr 2026 00:25:49 -0700 Subject: [PATCH 2/2] =?UTF-8?q?feat(webhooks):=20#101=20=E2=80=94=20workfl?= =?UTF-8?q?ow=5Frun=20event=20=E2=86=92=20DevOps=20A2A?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #101 layer 1: buildGitHubA2APayload now handles workflow_run events, routing failed CI runs to a workspace via the existing X-Molecule-Workspace-ID / webhook path. Only completed runs with a failure/cancelled/timed_out conclusion fan out — success/skipped/neutral are dropped via errIgnoredGitHubAction. Surface message is human-readable + includes the run URL so DevOps can jump straight to the failing job. Metadata carries the full run context (workflow_name, run_id, run_number, conclusion, head_branch, head_sha, run_url, trigger_event) for programmatic handling. 4 new tests cover the failure path, success skip, non-completed action skip, and short-SHA edge case. Layer 2 (org.yaml wiring for DevOps workspace + GITHUB_WEBHOOK_SECRET docs) stays as a follow-up PR. Co-Authored-By: Claude Opus 4.6 (1M context) --- platform/internal/handlers/webhooks.go | 67 ++++++++++++++ .../handlers/webhooks_workflow_test.go | 89 +++++++++++++++++++ 2 files changed, 156 insertions(+) create mode 100644 platform/internal/handlers/webhooks_workflow_test.go diff --git a/platform/internal/handlers/webhooks.go b/platform/internal/handlers/webhooks.go index e63370fd2..259c62f03 100644 --- a/platform/internal/handlers/webhooks.go +++ b/platform/internal/handlers/webhooks.go @@ -155,6 +155,29 @@ type githubPRReviewCommentEvent struct { Comment githubComment `json:"comment"` } +// githubWorkflowRun captures the subset of GitHub's `workflow_run` event we +// route to workspaces (#101). Full schema is ~50 fields; we only need the +// handful that tell DevOps "which CI job failed, where, and how to get there." +type githubWorkflowRun struct { + ID int64 `json:"id"` + Name string `json:"name"` // workflow name, e.g. "CI" + Event string `json:"event"` // push / pull_request / etc. + Status string `json:"status"` // queued / in_progress / completed + Conclusion string `json:"conclusion"` // success / failure / cancelled / timed_out + HeadBranch string `json:"head_branch"` + HeadSHA string `json:"head_sha"` + HTMLURL string `json:"html_url"` + RunNumber int `json:"run_number"` +} + +type githubWorkflowRunEvent struct { + WorkspaceID string `json:"workspace_id"` + Action string `json:"action"` // requested / in_progress / completed + Repository githubRepository `json:"repository"` + Sender githubSender `json:"sender"` + WorkflowRun githubWorkflowRun `json:"workflow_run"` +} + func buildGitHubA2APayload(eventType, deliveryID string, rawBody []byte) (string, map[string]interface{}, error) { switch eventType { case "issue_comment": @@ -209,6 +232,50 @@ func buildGitHubA2APayload(eventType, deliveryID string, rawBody []byte) (string "pull_request_num": payload.PullRequest.Number, "comment_url": payload.Comment.HTMLURL, }), nil + case "workflow_run": + // #101 — CI-break notifications for DevOps Engineer. Only surface + // *completed* runs with a non-success conclusion; queued / in_progress + // are noise. A success completion is dropped too (explicit filter + // rather than `errIgnoredGitHubAction` so the behaviour is visible + // in the switch). + var payload githubWorkflowRunEvent + if err := json.Unmarshal(rawBody, &payload); err != nil { + return "", nil, fmt.Errorf("invalid workflow_run payload: %w", err) + } + if payload.Action != "completed" { + return payload.WorkspaceID, nil, errIgnoredGitHubAction + } + if payload.WorkflowRun.Conclusion == "success" || payload.WorkflowRun.Conclusion == "skipped" || payload.WorkflowRun.Conclusion == "neutral" { + return payload.WorkspaceID, nil, errIgnoredGitHubAction + } + text := fmt.Sprintf( + "GitHub CI break — workflow '%s' run #%d %s on %s@%s\nTriggered by: %s (%s)\nRepo: %s\nRun URL: %s", + payload.WorkflowRun.Name, + payload.WorkflowRun.RunNumber, + payload.WorkflowRun.Conclusion, + payload.WorkflowRun.HeadBranch, + payload.WorkflowRun.HeadSHA[:min(7, len(payload.WorkflowRun.HeadSHA))], + payload.Sender.Login, + payload.WorkflowRun.Event, + payload.Repository.FullName, + payload.WorkflowRun.HTMLURL, + ) + return payload.WorkspaceID, newGitHubMessagePayload(text, map[string]interface{}{ + "source": "github", + "event": eventType, + "action": payload.Action, + "delivery_id": deliveryID, + "repository": payload.Repository.FullName, + "sender": payload.Sender.Login, + "workflow_name": payload.WorkflowRun.Name, + "run_id": payload.WorkflowRun.ID, + "run_number": payload.WorkflowRun.RunNumber, + "conclusion": payload.WorkflowRun.Conclusion, + "head_branch": payload.WorkflowRun.HeadBranch, + "head_sha": payload.WorkflowRun.HeadSHA, + "run_url": payload.WorkflowRun.HTMLURL, + "trigger_event": payload.WorkflowRun.Event, + }), nil default: return "", nil, errUnsupportedGitHubEvent } diff --git a/platform/internal/handlers/webhooks_workflow_test.go b/platform/internal/handlers/webhooks_workflow_test.go new file mode 100644 index 000000000..5c3419b03 --- /dev/null +++ b/platform/internal/handlers/webhooks_workflow_test.go @@ -0,0 +1,89 @@ +package handlers + +import ( + "encoding/json" + "strings" + "testing" +) + +// Tests the workflow_run → DevOps A2A routing added for #101. + +func TestBuildGitHubA2APayload_WorkflowRunFailure(t *testing.T) { + raw := []byte(`{ + "workspace_id": "ws-devops", + "action": "completed", + "repository": {"full_name": "Molecule-AI/molecule-monorepo"}, + "sender": {"login": "hongming"}, + "workflow_run": { + "id": 123456, + "name": "CI", + "event": "pull_request", + "status": "completed", + "conclusion": "failure", + "head_branch": "fix/thing", + "head_sha": "deadbeef1234567", + "html_url": "https://github.com/Molecule-AI/molecule-monorepo/actions/runs/123456", + "run_number": 42 + } + }`) + + wsID, payload, err := buildGitHubA2APayload("workflow_run", "delivery-abc", raw) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if wsID != "ws-devops" { + t.Errorf("workspace id: got %q want ws-devops", wsID) + } + + body, _ := json.Marshal(payload) + text := string(body) + for _, needle := range []string{"failure", "CI", "run #42", "fix/thing", "deadbee", "Molecule-AI/molecule-monorepo"} { + if !strings.Contains(text, needle) { + t.Errorf("missing %q in payload: %s", needle, text) + } + } +} + +func TestBuildGitHubA2APayload_WorkflowRunSuccessIgnored(t *testing.T) { + raw := []byte(`{ + "workspace_id": "ws-devops", + "action": "completed", + "repository": {"full_name": "x/y"}, + "sender": {"login": "u"}, + "workflow_run": {"name": "CI", "status": "completed", "conclusion": "success", "head_sha": "abcdef1"} + }`) + _, _, err := buildGitHubA2APayload("workflow_run", "d1", raw) + if err != errIgnoredGitHubAction { + t.Errorf("success run should be ignored; got err=%v", err) + } +} + +func TestBuildGitHubA2APayload_WorkflowRunNonCompletedIgnored(t *testing.T) { + raw := []byte(`{ + "workspace_id": "ws-devops", + "action": "requested", + "repository": {"full_name": "x/y"}, + "sender": {"login": "u"}, + "workflow_run": {"name": "CI", "status": "in_progress", "conclusion": "", "head_sha": "abc"} + }`) + _, _, err := buildGitHubA2APayload("workflow_run", "d2", raw) + if err != errIgnoredGitHubAction { + t.Errorf("non-completed action should be ignored; got err=%v", err) + } +} + +// Short-SHA truncation used to crash when head_sha was < 7 chars — the +// `min(7, len)` guard covers that edge case. +func TestBuildGitHubA2APayload_WorkflowRunShortSHA(t *testing.T) { + raw := []byte(`{ + "workspace_id": "ws-devops", + "action": "completed", + "repository": {"full_name": "x/y"}, + "sender": {"login": "u"}, + "workflow_run": {"name": "CI", "status": "completed", "conclusion": "failure", "head_sha": "abc", "run_number": 1} + }`) + _, _, err := buildGitHubA2APayload("workflow_run", "d3", raw) + if err != nil { + t.Errorf("short-sha path: %v", err) + } +}