diff --git a/internal/plane/parse.go b/internal/plane/parse.go index 7625472..cb8fe3c 100644 --- a/internal/plane/parse.go +++ b/internal/plane/parse.go @@ -99,11 +99,11 @@ func VerifyAndParse(secret string, headers http.Header, body []byte) (*Event, er func workItemKind(action string) (EventKind, error) { switch action { - case actionCreate: + case actionCreate, actionCreated: return EventWorkItemCreated, nil - case actionUpdate: + case actionUpdate, actionUpdated: return EventWorkItemUpdated, nil - case actionDelete: + case actionDelete, actionDeleted: return EventWorkItemDeleted, nil default: return "", fmt.Errorf("%w: issue action %q", ErrUnsupportedEvent, action) @@ -112,11 +112,11 @@ func workItemKind(action string) (EventKind, error) { func commentKind(action string) (EventKind, error) { switch action { - case actionCreate: + case actionCreate, actionCreated: return EventCommentCreated, nil - case actionUpdate: + case actionUpdate, actionUpdated: return EventCommentUpdated, nil - case actionDelete: + case actionDelete, actionDeleted: return EventCommentDeleted, nil default: return "", fmt.Errorf("%w: issue_comment action %q", ErrUnsupportedEvent, action) diff --git a/internal/plane/parse_test.go b/internal/plane/parse_test.go index f441b2f..a719055 100644 --- a/internal/plane/parse_test.go +++ b/internal/plane/parse_test.go @@ -5,6 +5,7 @@ import ( "net/http" "os" "path/filepath" + "strings" "testing" ) @@ -265,3 +266,71 @@ func TestNewClient(t *testing.T) { t.Errorf("client fields wrong: %+v", c) } } + +// TestParse_RealPlaneActionVerbsAccepted pins the past-tense action +// verbs that real Plane (v1.3.1+) actually sends. The bridge originally +// shipped recognising only present-tense create/update/delete; every +// real Plane delivery hit ErrUnsupportedEvent and silently 204'd. See +// GH#10 (PFB-22) for the full reproduction. +// +// This test fails if anyone tightens the parser back to present-tense +// only. +func TestParse_RealPlaneActionVerbsAccepted(t *testing.T) { + t.Parallel() + + const realPlanePayload = `{ + "event": "issue", + "action": "updated", + "webhook_id": "3eda452e-0000-0000-0000-000000000001", + "workspace_id": "00000000-0000-0000-0000-000000000aaa", + "data": { + "id": "33333333-3333-3333-3333-333333333333", + "name": "real plane payload", + "project": "77777777-7777-7777-7777-777777777777", + "workspace": "00000000-0000-0000-0000-000000000aaa", + "sequence_id": 1 + } + }` + + cases := []struct { + event string + action string + wantKind EventKind + }{ + {planeEventIssue, "created", EventWorkItemCreated}, + {planeEventIssue, "updated", EventWorkItemUpdated}, + {planeEventIssue, "deleted", EventWorkItemDeleted}, + // Present-tense aliases — kept so hand-rolled clients / older + // Plane code paths that emit "create"/"update"/"delete" still + // work. Removing these would be a silent behavior change. + {planeEventIssue, "create", EventWorkItemCreated}, + {planeEventIssue, "update", EventWorkItemUpdated}, + {planeEventIssue, "delete", EventWorkItemDeleted}, + {planeEventIssueComment, "created", EventCommentCreated}, + {planeEventIssueComment, "updated", EventCommentUpdated}, + {planeEventIssueComment, "deleted", EventCommentDeleted}, + {planeEventIssueComment, "create", EventCommentCreated}, + {planeEventIssueComment, "update", EventCommentUpdated}, + {planeEventIssueComment, "delete", EventCommentDeleted}, + } + + for _, tc := range cases { + t.Run(tc.event+"_"+tc.action, func(t *testing.T) { + t.Parallel() + body := []byte(strings.ReplaceAll( + strings.ReplaceAll(realPlanePayload, `"event": "issue"`, `"event": "`+tc.event+`"`), + `"action": "updated"`, `"action": "`+tc.action+`"`, + )) + h := http.Header{} + h.Set(HeaderEvent, tc.event) + h.Set(HeaderDelivery, "plane-action-verb-test") + ev, err := Parse(h, body) + if err != nil { + t.Fatalf("Parse failed for %s/%s: %v", tc.event, tc.action, err) + } + if ev.Kind != tc.wantKind { + t.Errorf("Kind = %q, want %q", ev.Kind, tc.wantKind) + } + }) + } +} diff --git a/internal/plane/testdata/comment_created.json b/internal/plane/testdata/comment_created.json index 9d05970..e708538 100644 --- a/internal/plane/testdata/comment_created.json +++ b/internal/plane/testdata/comment_created.json @@ -1,6 +1,6 @@ { "event": "issue_comment", - "action": "create", + "action": "created", "webhook_id": "11111111-1111-1111-1111-111111111111", "workspace_id": "22222222-2222-2222-2222-222222222222", "data": { diff --git a/internal/plane/testdata/comment_updated.json b/internal/plane/testdata/comment_updated.json index aff7731..63b8472 100644 --- a/internal/plane/testdata/comment_updated.json +++ b/internal/plane/testdata/comment_updated.json @@ -1,6 +1,6 @@ { "event": "issue_comment", - "action": "update", + "action": "updated", "webhook_id": "11111111-1111-1111-1111-111111111111", "workspace_id": "22222222-2222-2222-2222-222222222222", "data": { diff --git a/internal/plane/testdata/work_item_created.json b/internal/plane/testdata/work_item_created.json index 8794bb7..aaaba7e 100644 --- a/internal/plane/testdata/work_item_created.json +++ b/internal/plane/testdata/work_item_created.json @@ -1,6 +1,6 @@ { "event": "issue", - "action": "create", + "action": "created", "webhook_id": "11111111-1111-1111-1111-111111111111", "workspace_id": "22222222-2222-2222-2222-222222222222", "data": { diff --git a/internal/plane/testdata/work_item_deleted.json b/internal/plane/testdata/work_item_deleted.json index 64ef247..0ac0c29 100644 --- a/internal/plane/testdata/work_item_deleted.json +++ b/internal/plane/testdata/work_item_deleted.json @@ -1,6 +1,6 @@ { "event": "issue", - "action": "delete", + "action": "deleted", "webhook_id": "11111111-1111-1111-1111-111111111111", "workspace_id": "22222222-2222-2222-2222-222222222222", "data": { diff --git a/internal/plane/testdata/work_item_updated.json b/internal/plane/testdata/work_item_updated.json index 3b9c5d0..381bce2 100644 --- a/internal/plane/testdata/work_item_updated.json +++ b/internal/plane/testdata/work_item_updated.json @@ -1,6 +1,6 @@ { "event": "issue", - "action": "update", + "action": "updated", "webhook_id": "11111111-1111-1111-1111-111111111111", "workspace_id": "22222222-2222-2222-2222-222222222222", "data": { diff --git a/internal/plane/types.go b/internal/plane/types.go index 982fccc..23a208a 100644 --- a/internal/plane/types.go +++ b/internal/plane/types.go @@ -33,11 +33,19 @@ const ( planeEventIssueComment = "issue_comment" ) -// Plane "action" values in the body envelope. +// Plane "action" values in the body envelope. Real Plane (verified +// against v1.3.1's webhook_task.py) sends past-tense verbs +// ("created"/"updated"/"deleted"), matching the GitHub/GitLab webhook +// convention. The present-tense form ("create"/"update"/"delete") shows +// up in older / non-bgtask code paths and in some hand-rolled clients, +// so the parser accepts both — the past-tense form is canonical. const ( - actionCreate = "create" - actionUpdate = "update" - actionDelete = "delete" + actionCreate = "create" + actionUpdate = "update" + actionDelete = "delete" + actionCreated = "created" + actionUpdated = "updated" + actionDeleted = "deleted" ) // Actor is the (often partial) record of who triggered the event. Plane diff --git a/internal/server/server.go b/internal/server/server.go index 900702f..049d757 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -368,14 +368,27 @@ func (s *Server) respondVerifyParseErr(w http.ResponseWriter, r *http.Request, s case errors.Is(err, forge.ErrMissingEventHeader), errors.Is(err, plane.ErrMissingEventHeader): http.Error(w, "missing event header", http.StatusBadRequest) - case errors.Is(err, forge.ErrUnsupportedEvent), - errors.Is(err, plane.ErrUnsupportedEvent): - // Acknowledge events we don't handle so the forge stops retrying. + case errors.Is(err, forge.ErrUnsupportedEvent): + // Forges legitimately fan out events the operator didn't subscribe + // to (e.g. push events from a repo whose webhook also covers + // issues). Log at DEBUG so production isn't flooded. s.log.LogAttrs(r.Context(), slog.LevelDebug, "webhook event ignored", slog.String("side", side), slog.String("err", err.Error()), ) w.WriteHeader(http.StatusNoContent) + case errors.Is(err, plane.ErrUnsupportedEvent): + // Plane only sends event types the operator subscribed to, so + // any unsupported event here means a contract mismatch with + // real Plane (e.g. an action-verb spelling we don't recognise). + // Log at INFO so it surfaces at the default log level — silent + // 204s on a wholly-broken integration are a brutal failure + // mode. See GH#10 (PFB-22). + s.log.LogAttrs(r.Context(), slog.LevelInfo, "plane webhook event ignored", + slog.String("side", side), + slog.String("err", err.Error()), + ) + w.WriteHeader(http.StatusNoContent) case errors.Is(err, forge.ErrMalformedPayload), errors.Is(err, plane.ErrMalformedPayload): s.log.LogAttrs(r.Context(), slog.LevelWarn, "webhook payload malformed",