diff --git a/AGENTS.md b/AGENTS.md index eec780c..2ccb30f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -54,6 +54,16 @@ any self-hosted Forgejo Actions runner because `runs-on` is parameterized via short LRU of `(source_event_id, target_object_id)` pairs catches the case where the marker was stripped. Both layers must stay — they cover different failure modes. + - **Plane CE v1.3.1 strips HTML comments from `description_html`** during + ProseMirror sanitization (verified against plane.stern.ca — POST body + `

x

\n\n` round-trips as `

x

\n\n
`). + Comment bodies (`comment_html`) preserve the marker; work item + descriptions do not. The marker check is therefore non-functional on + inbound `work_item.*` echoes. The durable defence is the + `external_source` round-trip: the bridge stamps + `external_source="forge:owner/repo"` on every forge→plane create, and + `handlePlaneWorkItemCreated` skips any inbound `work_item.created` + whose `WorkItem.ExternalSource` starts with `"forge:"`. See PFB-27. - **HMAC verification happens at the HTTP handler boundary**, before the body is parsed. Constant-time compare. Reject on missing header. - **Stateless process.** No on-disk state, no DB. Config + env vars in, HTTP diff --git a/CHANGELOG.md b/CHANGELOG.md index 93a4df3..263fc95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,28 @@ All notable changes to plane-forge-bridge are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.1.4] — 2026-05-24 + +### Fixed + +- **Plane → forge: drop work_item.created echoes via external_source + (PFB-27).** The bridge stamps `external_source="forge:owner/repo"` + on every forge→plane create; Plane echoes that back on its own + `work_item.created` webhook. The HTML-comment loop-break marker is + supposed to catch the echo, but Plane CE v1.3.1 strips HTML comments + out of `description_html` during ProseMirror sanitization (verified + against plane.stern.ca — `

x

\n\n` round-trips as + `

x

\n\n
`; comment bodies preserve the marker). With + no marker on the echo, `handlePlaneWorkItemCreated` would create a + duplicate forge issue — the failure mode PFB-25 exposed in + production. The handler now gates on `WorkItem.ExternalSource` + starting with the bridge's `"forge:"` prefix and skips with + `reasonPlaneCreatedEchoExternalSource`. Non-bridge external sources + (e.g. an operator importing from GitHub) are unaffected. AGENTS.md + updated to document the ProseMirror sanitization finding so the next + feature work doesn't reach for the marker as the durable + description-side defence. + ## [0.1.3] — 2026-05-24 ### Fixed @@ -78,6 +100,7 @@ Loop-break: HTML-comment marker + in-memory LRU. Single static binary, distroless runtime, stateless. CI matrix tests against real Forgejo 15 and Gitea 1.22 service containers end-to-end on every PR. +[0.1.4]: https://github.com/hstern/plane-forge-bridge/compare/v0.1.3...v0.1.4 [0.1.3]: https://github.com/hstern/plane-forge-bridge/compare/v0.1.2...v0.1.3 [0.1.2]: https://github.com/hstern/plane-forge-bridge/compare/v0.1.1...v0.1.2 [0.1.1]: https://github.com/hstern/plane-forge-bridge/compare/v0.1.0...v0.1.1 diff --git a/internal/sync/planeworkitem.go b/internal/sync/planeworkitem.go index acc514d..f921e36 100644 --- a/internal/sync/planeworkitem.go +++ b/internal/sync/planeworkitem.go @@ -37,8 +37,32 @@ const ( // loud so operators can correlate the deferral with the rollout // state. reasonPlaneIssueWriterMissing = "plane → forge issue writes deferred — forge client build does not implement ForgeIssueWriter" + + // reasonPlaneCreatedEchoExternalSource is returned when a plane + // work_item.created webhook arrives for a work item that already + // carries a bridge-stamped external_source ("forge:owner/repo"). The + // bridge sets external_source / external_id on every forge → plane + // create, so a created webhook with that pair set is necessarily the + // echo of our own write — mirroring it back to forge would duplicate + // the original forge issue. + // + // This is the durable backstop from PFB-27. The HTML-comment marker + // is the bridge's primary loop-break defence, but Plane CE v1.3.1 + // strips HTML comments from description_html during ProseMirror + // sanitization (verified against plane.stern.ca), so the marker + // never reaches the webhook payload for work item descriptions. + // Plane preserves the marker in comment_html, so comment echoes + // still rely on the marker. + reasonPlaneCreatedEchoExternalSource = "plane work_item.created echoes a bridge-originated work item (external_source matches forge: prefix)" ) +// externalSourceForgePrefix is the prefix the bridge stamps on every +// forge → plane create. Matching it on an inbound work_item.created +// webhook is the durable signal that the work item is our own echo — +// the marker check can't see it (Plane sanitizes HTML comments out of +// description_html). See PFB-27. +const externalSourceForgePrefix = "forge:" + // HandlePlaneWorkItem translates a plane work_item.* event into a forge // issue create or update. This is the inverse of HandleForgeIssue and // closes the bidirectional loop for issues (step 10 of the build order). @@ -138,11 +162,34 @@ func (e *Engine) HandlePlaneWorkItem(ctx context.Context, evt *plane.Event) (*Ou } // handlePlaneWorkItemCreated implements the EventWorkItemCreated branch. -// CreateIssue is unconditional in v1 (no reverse lookup); the caller's -// loop-break LRU is the only defence against a re-delivered create -// producing a duplicate forge issue. The LRU is the server's job and -// fires before we get here, so v1 ships without the redundant lookup. +// +// First gate: external_source check. The bridge stamps +// external_source="forge:owner/repo" on every forge → plane create +// (see externalRef). A plane work_item.created event whose WorkItem +// carries that prefix is necessarily our own echo — mirroring it back +// would duplicate the original forge issue. The check is the durable +// backstop from PFB-27 (the marker-stripping window the inbound +// HTML-comment marker can't cover). +// +// Second gate (unchanged): forge writer capability check. +// +// CreateIssue is otherwise unconditional in v1 — no reverse lookup, +// no LRU consultation here. func (e *Engine) handlePlaneWorkItemCreated(ctx context.Context, evt *plane.Event, link *mapping.Link) (*Outcome, error) { + if strings.HasPrefix(evt.WorkItem.ExternalSource, externalSourceForgePrefix) { + e.Log.Info("dropping plane work_item.created echo (external_source matches bridge prefix)", + "plane_project", evt.WorkItem.Project, + "work_item", evt.WorkItem.ID, + "external_source", evt.WorkItem.ExternalSource, + "external_id", evt.WorkItem.ExternalID, + "delivery", evt.DeliveryID) + return &Outcome{ + Action: ActionSkipped, + Reason: reasonPlaneCreatedEchoExternalSource, + Link: link, + }, nil + } + writer, ok := e.ForgeClient.(ForgeIssueWriter) if !ok { e.Log.Warn("plane work_item.created received but ForgeClient cannot write issues", diff --git a/internal/sync/planeworkitem_test.go b/internal/sync/planeworkitem_test.go index 6f87acb..057c60a 100644 --- a/internal/sync/planeworkitem_test.go +++ b/internal/sync/planeworkitem_test.go @@ -170,6 +170,82 @@ func TestHandlePlaneWorkItem_AssigneeResolvedFromForgeEmail(t *testing.T) { } } +// TestHandlePlaneWorkItem_CreatedEchoSkipsByExternalSource is the +// regression test for PFB-27. The bridge stamps external_source on +// every forge → plane create; Plane echoes that back on its own +// work_item.created webhook. The HTML-comment marker the bridge also +// stamps would normally catch this, but Plane CE v1.3.1 strips HTML +// comments out of description_html during sanitization (preserves +// them in comment_html — verified against plane.stern.ca). The +// external_source check is the durable defence. +// +// This test fails if anyone removes the external_source gate. +func TestHandlePlaneWorkItem_CreatedEchoSkipsByExternalSource(t *testing.T) { + t.Parallel() + e, _, fc := newPlaneWorkItemTestEngine(t) + + evt := mkPlaneWorkItemEvent(plane.EventWorkItemCreated) + // The work item carries the bridge's external_source — this is our own + // echo. The marker on the description is gone (Plane stripped it during + // sanitization), so the external_source check is what saves us. + evt.WorkItem.ExternalSource = "forge:acme/widgets" + evt.WorkItem.ExternalID = "42" + + out, err := e.HandlePlaneWorkItem(context.Background(), evt) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if out.Action != ActionSkipped { + t.Fatalf("Action=%v reason=%q, want ActionSkipped", out.Action, out.Reason) + } + if out.Reason != reasonPlaneCreatedEchoExternalSource { + t.Errorf("Reason=%q, want %q", out.Reason, reasonPlaneCreatedEchoExternalSource) + } + if out.Link == nil { + t.Errorf("Link should be set on the echo skip — the project link was matched") + } + if len(fc.IssueCreates) != 0 { + t.Errorf("IssueCreates=%d on echo path — must be 0 (duplicate avoidance)", + len(fc.IssueCreates)) + } +} + +// TestHandlePlaneWorkItem_CreatedNonBridgeExternalSourceProceeds +// pins that external_source pointing at a non-bridge system (e.g. +// "github:foo/bar" if an operator imports issues from GitHub) does +// NOT trigger the echo skip — only the "forge:" prefix is the bridge +// signal. Without this guard the bridge would silently drop legitimate +// non-bridge work items. +func TestHandlePlaneWorkItem_CreatedNonBridgeExternalSourceProceeds(t *testing.T) { + t.Parallel() + e, pc, fc := newPlaneWorkItemTestEngine(t) + pc.ListProjectLabelsFunc = func(_ context.Context, _ string) ([]plane.Label, error) { + return []plane.Label{ + {ID: "label-uuid-bug", Name: "bug"}, + {ID: "label-uuid-help", Name: "help wanted"}, + }, nil + } + fc.ListRepoLabelsFunc = func(_ context.Context, _, _ string) ([]forge.Label, error) { + return []forge.Label{{ID: 11, Name: "bug"}, {ID: 22, Name: "help wanted"}}, nil + } + + evt := mkPlaneWorkItemEvent(plane.EventWorkItemCreated) + evt.WorkItem.ExternalSource = "github:acme/widgets" + evt.WorkItem.ExternalID = "99" + + out, err := e.HandlePlaneWorkItem(context.Background(), evt) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if out.Action != ActionCreated { + t.Fatalf("Action=%v reason=%q, want ActionCreated (non-bridge source should mirror)", + out.Action, out.Reason) + } + if len(fc.IssueCreates) != 1 { + t.Errorf("IssueCreates=%d, want 1", len(fc.IssueCreates)) + } +} + func TestHandlePlaneWorkItem_NoLinkSkips(t *testing.T) { t.Parallel() e, _, fc := newPlaneWorkItemTestEngine(t)