feat(bitbucket): add Bitbucket adapter (Cloud + Data Center) - #50
Conversation
Add the BitbucketBotType iota const between GitLab and Signal, its
String() case ("bitbucket"), and the root re-export, so the
platform-agnostic layer can identify a Bitbucket bot.
Add a webhook adapter for Bitbucket, serving both Cloud (REST 2.0 via ktrysmt/go-bitbucket) and Data Center (REST 1.0 via net/http) from one package, selected by Config.BaseURL. Cloud and Data Center share only the webhook framing (X-Event-Key routing, X-Hub-Signature HMAC-SHA256 over the raw body); the divergence in event keys, payload shapes, reply bodies and self-identity lives behind an unexported flavor interface so server.go routes on a flavor-computed category and never branches on flavor. - Replies as pull-request and issue comments; issue comments are Cloud only (Data Center has no issue tracker), so a workspace/repo#N channel id is rejected on Data Center. Threading is native on both flavors via a comment's parent.id, so the adapter implements core.ThreadedSender. - Auth is an Atlassian API token (HTTP Basic) or a repository/project/ workspace access token (Bearer); app passwords are never offered. Self identity for the reply-loop guard resolves via GET /2.0/user in Cloud API-token mode and is required (Config.Self) in access-token mode and on Data Center, where no whoami exists. - OnPullRequest and OnPush callbacks route those deliveries on the same endpoint; nil callbacks ack and drop. Every authentic delivery acks 200 except the bad-signature 401; handlers must be idempotent (Bitbucket Cloud retries a timed-out delivery). - Reactions are permanently omitted: Bitbucket has no comment reactions. Ports the internal/gitlab lifecycle (per-connection semaphore, drain, detached dispatch context) guard for guard. Public bitbucket/ facade with CloudClient/RawEvent/Addr accessors plus import guards; isolation_deps_test proves ktrysmt is confined to the bitbucket build closure.
Sweep every platform enumeration in BotType iota order (between GitLab and Signal): CLAUDE.md architecture, botbooter package doc, README (tagline, features, run list, constructor table, threading, reactions, credentials, roadmap) and _docs/platforms.md (intro, official-docs list, a full Bitbucket section covering both flavors and both auth modes, the threaded- replies table). Add a bitbucket case to _examples/basic.
|
✅ Review posted — 1 finding(s).
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe PR adds Bitbucket Cloud and Data Center support, including configuration, webhook verification, event parsing, comment delivery, threading, callbacks, lifecycle management, public wrappers, tests, examples, and documentation. ChangesBitbucket adapter
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Bitbucket
participant WebhookServer
participant MessageParser
participant Callback
Bitbucket->>WebhookServer: Send signed webhook
WebhookServer->>WebhookServer: Validate signature and limits
WebhookServer->>MessageParser: Parse event
MessageParser-->>WebhookServer: Return normalized event
WebhookServer->>Callback: Dispatch configured callback
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![botbooter-test[bot]](https://avatars.githubusercontent.com/u/197033?s=60&v=4){kind=small}
There was a problem hiding this comment.
🗂️ Old review — superseded
Verdict: comment · Grade: 8/10
To reach a higher grade: Add ktrysmt/go-bitbucket to the root imports_guard_test.go and the sibling */imports_test.go banned lists, matching how go-github and gitlab client-go were rolled into every guard.
📝 Summary of changes
Adds a Bitbucket adapter (Cloud + Data Center in one package, selected at runtime by Config.BaseURL) mirroring the GitHub/GitLab webhook siblings: HMAC-SHA256 X-Hub-Signature verification over the raw body, per-connection dispatch/read semaphores, drain-on-shutdown, detached dispatch context, core.ThreadedSender via parent.id, and a flavor interface that keeps Cloud/DC divergence out of the lifecycle code. Auth is API-token (Basic) or access-token (Bearer), with Config.Self required where whoami is unavailable. The change is well-structured, thoroughly tested (signature rejections, self-drop, cap/semaphore shedding, callbacks, reconnect, drain deadline, channel-id hardening including ~USERNAME DC personal repos), and the docs/enumeration sweep across CLAUDE.md, README.md, _docs/platforms.md, and botbooter.go is complete and in iota order.
I reviewed the signature/auth path, self-resolution lifecycle, channel-id parsing, threading precedence, and the concurrency of the semaphore/inflight/drain machinery; all are consistent with the established sibling adapters and I found no correctness, security, or concurrency defect. The documented ktrysmt-context and Cloud-whoami goroutine ceilings are acceptable and clearly annotated.
The one gap is a consistency issue in the direct-import test guards: unlike every prior platform SDK, ktrysmt/go-bitbucket was not added to the sibling */imports_test.go banned lists nor to the root imports_guard_test.go. Actual isolation is still proven transitively by isolation_deps_test.go, so this is low severity, but it leaves the fast direct-import guards non-exhaustive for the new SDK.
🤖 AI prompt to fix all 1 finding(s) (review before running)
Fix 1 issue(s) found during code review of lao/botbooter (PR #50).
--- Issue 1 ---
File: isolation_deps_test.go:77 (side RIGHT)
Severity: low
Issue: New ktrysmt SDK added to transitive guard but not to the direct-import guards
The transitive `isolation_deps_test.go` rows are correctly updated to require `ktrysmt/go-bitbucket` present in the bitbucket closure and absent everywhere else. However, the direct-import guards were not correspondingly updated: the root `imports_guard_test.go` banned list and every sibling `*/imports_test.go` (slack, discord, telegram, teams, github, gitlab, signal, cli, whatsapp) still omit `ktrysmt/go-bitbucket`.
This breaks the established convention — when GitHub and GitLab landed, `google/go-github`, `bradleyfalzon/ghinstallation`, and `gitlab-org/api/client-go` were added to every sibling guard (see `slack/imports_test.go`, which bans all three). The direct guards are the fast first line of defense (no toolchain needed), and `goListDeps` skips the transitive test entirely when `go` is not on PATH, so a stray `ktrysmt` import into, e.g., the SDK-free root `botbooter.go` could go uncaught in that environment.
Suggestion: add `"ktrysmt/go-bitbucket"` (or the `github.com/ktrysmt/go-bitbucket` substring) to the banned list in root `imports_guard_test.go` and in each non-bitbucket `*/imports_test.go`, mirroring the treatment of the other platform SDKs.
Apply minimal, correct fixes that resolve these issues. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, /auto-fix to have me attempt the fixes, or /clean to remove my comments.
| // confined to the bitbucket closure and never leak into another package | ||
| // (Data Center replies use plain net/http, so even a Bitbucket consumer's | ||
| // binary carries it only through the Cloud path). | ||
| ktrysmtSDK = "github.com/ktrysmt/go-bitbucket" |
There was a problem hiding this comment.
[low] New ktrysmt SDK added to transitive guard but not to the direct-import guards
The transitive isolation_deps_test.go rows are correctly updated to require ktrysmt/go-bitbucket present in the bitbucket closure and absent everywhere else. However, the direct-import guards were not correspondingly updated: the root imports_guard_test.go banned list and every sibling */imports_test.go (slack, discord, telegram, teams, github, gitlab, signal, cli, whatsapp) still omit ktrysmt/go-bitbucket.
This breaks the established convention — when GitHub and GitLab landed, google/go-github, bradleyfalzon/ghinstallation, and gitlab-org/api/client-go were added to every sibling guard (see slack/imports_test.go, which bans all three). The direct guards are the fast first line of defense (no toolchain needed), and goListDeps skips the transitive test entirely when go is not on PATH, so a stray ktrysmt import into, e.g., the SDK-free root botbooter.go could go uncaught in that environment.
Suggestion: add "ktrysmt/go-bitbucket" (or the github.com/ktrysmt/go-bitbucket substring) to the banned list in root imports_guard_test.go and in each non-bitbucket */imports_test.go, mirroring the treatment of the other platform SDKs.
🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #50).
File: isolation_deps_test.go:77 (side RIGHT)
Severity: low
Issue: New ktrysmt SDK added to transitive guard but not to the direct-import guards
The transitive `isolation_deps_test.go` rows are correctly updated to require `ktrysmt/go-bitbucket` present in the bitbucket closure and absent everywhere else. However, the direct-import guards were not correspondingly updated: the root `imports_guard_test.go` banned list and every sibling `*/imports_test.go` (slack, discord, telegram, teams, github, gitlab, signal, cli, whatsapp) still omit `ktrysmt/go-bitbucket`.
This breaks the established convention — when GitHub and GitLab landed, `google/go-github`, `bradleyfalzon/ghinstallation`, and `gitlab-org/api/client-go` were added to every sibling guard (see `slack/imports_test.go`, which bans all three). The direct guards are the fast first line of defense (no toolchain needed), and `goListDeps` skips the transitive test entirely when `go` is not on PATH, so a stray `ktrysmt` import into, e.g., the SDK-free root `botbooter.go` could go uncaught in that environment.
Suggestion: add `"ktrysmt/go-bitbucket"` (or the `github.com/ktrysmt/go-bitbucket` substring) to the banned list in root `imports_guard_test.go` and in each non-bitbucket `*/imports_test.go`, mirroring the treatment of the other platform SDKs.
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
internal/bitbucket/bitbucket_test.go (2)
80-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover both large-request callback paths.
requestByteLimitraises the limit when eitherOnPullRequestorOnPushis set. This test covers onlyOnPush, so a regression in the pull-request branch would pass. Add anOnPullRequestcase and assertlargeRequestBytes. The corresponding implementation is ininternal/bitbucket/bitbucket.goLines 222-227.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bitbucket/bitbucket_test.go` around lines 80 - 90, Extend TestRequestByteLimitScales to cover the OnPullRequest callback path in addition to OnPush. Create an adapter configuration with OnPullRequest set and assert its maxRequestBytes equals largeRequestBytes, while preserving the existing small-cap and OnPush assertions.
37-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover Data Center access-token construction.
Add a valid case for Data Center with
AccessTokenandSelf. The current table does not exercise this distinctdataCenter && accessModepath, so a regression in Data Center Bearer authentication could pass unnoticed. The constructor selects these branches ininternal/bitbucket/bitbucket.goLines 275-339.Suggested test case
{name: "DataCenter", cfg: Config{Secret: "s", Addr: ":0", Email: "e", APIToken: "t", BaseURL: "https://bb.example.com", Self: "botuser"}}, + {name: "DataCenterAccessToken", cfg: Config{Secret: "s", Addr: ":0", AccessToken: "a", BaseURL: "https://bb.example.com", Self: "botuser"}},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bitbucket/bitbucket_test.go` around lines 37 - 53, Add a valid Data Center test case to the TestNewValid table using Config with Secret, Addr, AccessToken, BaseURL, and Self populated, while preserving the existing Cloud and Data Center cases so the New constructor’s Data Center access-token branch is exercised.bitbucket/imports_test.go (1)
13-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding the WhatsApp and Teams SDK markers to the banned list.
The banned list omits
go.mau.fi/whatsmeow,modernc.org/sqlite, andgolang-jwt. The module-level closure test already asserts these are absent from the bitbucket row, so this is a completeness nit for the direct-import guard only.♻️ Proposed addition
asserts.CheckBannedImports(t, ".", - []string{"discordgo", "slack-go/slack", "go-telegram/bot", "google/go-github", "bradleyfalzon/ghinstallation", "gitlab-org/api/client-go"}, "bitbucket") + []string{"discordgo", "slack-go/slack", "go-telegram/bot", "google/go-github", "bradleyfalzon/ghinstallation", "gitlab-org/api/client-go", "whatsmeow", "modernc.org/sqlite", "golang-jwt"}, "bitbucket")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bitbucket/imports_test.go` around lines 13 - 16, Extend the banned import markers in TestBitbucketImportsNoForeignSDK to include go.mau.fi/whatsmeow, modernc.org/sqlite, and golang-jwt, matching the module-level closure test’s exclusions while preserving the existing bitbucket marker.internal/bitbucket/message_test.go (1)
33-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a Cloud timestamp case with fractional seconds and an explicit offset.
Cloud sends
created_onvalues such as2026-08-03T10:00:00.123456+00:00. The current fixture uses a whole-secondZform only. A case with fractional seconds and a numeric offset locks in thatparseRFC3339does not silently fall back to the zero time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bitbucket/message_test.go` around lines 33 - 64, Extend TestCloudParseComment with a timestamp case using fractional seconds and an explicit numeric offset, such as 2026-08-03T10:00:00.123456+00:00, and assert parseComment preserves the expected instant and fractional precision. Reuse the existing PullRequest fixture/assertions so the test verifies parseRFC3339 handles this format without falling back to zero time.internal/bitbucket/send.go (1)
186-196: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider draining the response body on the success path.
The success path closes the body without reading it. An undrained body prevents connection reuse for the next comment call. A short
io.Copy(io.Discard, io.LimitReader(resp.Body, N))before returning keeps the keep-alive connection usable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bitbucket/send.go` around lines 186 - 196, The successful response path in the pull-request comment request should drain a bounded amount of resp.Body before closing it so the HTTP connection remains reusable. Update the flow around the existing defer and status check, using io.Copy with io.Discard and a limited reader before returning nil, while preserving the current non-2xx snippet handling.internal/bitbucket/send_test.go (1)
63-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
capvariable and consider guardingcapture.
capshadows the builtincapinsiderecordingServerand in every caller. Rename it torecorgotfor clarity.
capturefields are written on the server goroutine and read on the test goroutine. The single request/response per test orders these accesses today. If a future test issues concurrent requests,-racewill report a data race. A mutex or an accessor makes the helper safe for reuse.♻️ Proposed rename
-func recordingServer(t *testing.T, status int) (*httptest.Server, *capture) { - t.Helper() - cap := &capture{} - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - cap.method = r.Method - cap.path = r.URL.Path - raw, _ := io.ReadAll(r.Body) - _ = json.Unmarshal(raw, &cap.body) +func recordingServer(t *testing.T, status int) (*httptest.Server, *capture) { + t.Helper() + got := &capture{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got.method = r.Method + got.path = r.URL.Path + raw, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(raw, &got.body) w.WriteHeader(status) _, _ = w.Write([]byte(`{"id":1}`)) })) t.Cleanup(srv.Close) - return srv, cap + return srv, got }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bitbucket/send_test.go` around lines 63 - 76, Update recordingServer to rename the cap variable and its returned value to a non-shadowing name such as rec, including all callers. Protect capture field writes and reads with a mutex or synchronized accessor so the helper remains race-safe when requests are concurrent.internal/bitbucket/coverage_test.go (2)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
discardLoggerin the sibling test files.
server_test.gorepeatsslog.New(slog.NewTextHandler(io.Discard, nil))indiscardDeps,newCloudAdapter, and several tests. All files share thebitbucketpackage, so they can calldiscardLogger()instead. This removes the duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bitbucket/coverage_test.go` at line 33, Replace repeated slog.New(slog.NewTextHandler(io.Discard, nil)) constructions in server_test.go, including discardDeps, newCloudAdapter, and the affected tests, with calls to the existing discardLogger helper. Keep the logging behavior unchanged and reuse the shared helper across the bitbucket package.
103-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the handler sleep to keep the test fast.
httptest.Server.Closewaits for outstanding handlers. The handler sleeps 500ms, so this test always costs about 500ms even thoughresolveSelffails at once. A shorter sleep, or a handler that waits onr.Context().Done(), keeps the assertion and removes the delay.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bitbucket/coverage_test.go` around lines 103 - 114, Reduce the 500ms delay in the httptest handler within TestResolveSelfCloudCancelled, preferably by waiting for r.Context().Done() so the cancelled request exits promptly, while preserving the existing response and cancellation assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@_docs/platforms.md`:
- Line 776: Update the direct subsections under the `## Bitbucket` heading,
including `Step 1`, `Step 2`, `Step 3`, and `No response?`, from `####` to `###`
so the heading hierarchy satisfies MD001; apply the same correction to the
additional occurrences identified in the comment.
- Line 5: Update _docs/platforms.md line 5 to distinguish Bitbucket Cloud and
Data Center credential requirements instead of grouping all Bitbucket
deployments under Cloud. Update README.md lines 21 and 101 to state that
pull-request comments are supported by both flavors, while issue comments are
supported only on Bitbucket Cloud.
In `@CLAUDE.md`:
- Line 36: In the GitLab section’s documentation, replace every occurrence of
“noteable” with the correctly spelled “notable,” including compound references
such as noteable type and noteable filter; leave the surrounding behavior and
terminology unchanged.
In `@internal/bitbucket/bitbucket_test.go`:
- Around line 16-35: Update newAdapter to reject AccessToken combined with
either Email or APIToken, not only when both Basic-auth fields are present;
retain the existing mutual-exclusion behavior and add TestNewValidation
regression cases for AccessToken with only Email and only APIToken, setting Self
so authentication validation is reached.
---
Nitpick comments:
In `@bitbucket/imports_test.go`:
- Around line 13-16: Extend the banned import markers in
TestBitbucketImportsNoForeignSDK to include go.mau.fi/whatsmeow,
modernc.org/sqlite, and golang-jwt, matching the module-level closure test’s
exclusions while preserving the existing bitbucket marker.
In `@internal/bitbucket/bitbucket_test.go`:
- Around line 80-90: Extend TestRequestByteLimitScales to cover the
OnPullRequest callback path in addition to OnPush. Create an adapter
configuration with OnPullRequest set and assert its maxRequestBytes equals
largeRequestBytes, while preserving the existing small-cap and OnPush
assertions.
- Around line 37-53: Add a valid Data Center test case to the TestNewValid table
using Config with Secret, Addr, AccessToken, BaseURL, and Self populated, while
preserving the existing Cloud and Data Center cases so the New constructor’s
Data Center access-token branch is exercised.
In `@internal/bitbucket/coverage_test.go`:
- Line 33: Replace repeated slog.New(slog.NewTextHandler(io.Discard, nil))
constructions in server_test.go, including discardDeps, newCloudAdapter, and the
affected tests, with calls to the existing discardLogger helper. Keep the
logging behavior unchanged and reuse the shared helper across the bitbucket
package.
- Around line 103-114: Reduce the 500ms delay in the httptest handler within
TestResolveSelfCloudCancelled, preferably by waiting for r.Context().Done() so
the cancelled request exits promptly, while preserving the existing response and
cancellation assertion.
In `@internal/bitbucket/message_test.go`:
- Around line 33-64: Extend TestCloudParseComment with a timestamp case using
fractional seconds and an explicit numeric offset, such as
2026-08-03T10:00:00.123456+00:00, and assert parseComment preserves the expected
instant and fractional precision. Reuse the existing PullRequest
fixture/assertions so the test verifies parseRFC3339 handles this format without
falling back to zero time.
In `@internal/bitbucket/send_test.go`:
- Around line 63-76: Update recordingServer to rename the cap variable and its
returned value to a non-shadowing name such as rec, including all callers.
Protect capture field writes and reads with a mutex or synchronized accessor so
the helper remains race-safe when requests are concurrent.
In `@internal/bitbucket/send.go`:
- Around line 186-196: The successful response path in the pull-request comment
request should drain a bounded amount of resp.Body before closing it so the HTTP
connection remains reusable. Update the flow around the existing defer and
status check, using io.Copy with io.Discard and a limited reader before
returning nil, while preserving the current non-2xx snippet handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f7d8454-9525-4c59-a07f-a24a2950fa09
⛔ Files ignored due to path filters (2)
_examples/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
CLAUDE.mdREADME.md_docs/platforms.md_examples/basic/platforms.go_examples/go.modbitbucket/bitbucket.gobitbucket/imports_test.gobitbucket/wrapper_test.gobotbooter.gogo.modinternal/bitbucket/bitbucket.gointernal/bitbucket/bitbucket_test.gointernal/bitbucket/coverage_test.gointernal/bitbucket/message.gointernal/bitbucket/message_test.gointernal/bitbucket/send.gointernal/bitbucket/send_test.gointernal/bitbucket/server.gointernal/bitbucket/server_test.gointernal/core/core.gointernal/core/core_test.goisolation_deps_test.go
| Cloud, or its **user slug** on Data Center — it is how the adapter drops its own | ||
| comments to avoid reply loops. | ||
|
|
||
| #### Step 1, Create the webhook |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the heading hierarchy under ## Bitbucket.
## Bitbucket is followed directly by #### Step 1, #### Step 2, #### Step 3, and #### No response?. Use ### for these direct subsections, or add an intervening ### parent. The current structure triggers markdownlint MD001.
Also applies to: 792-792, 816-816, 868-868
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 776-776: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@_docs/platforms.md` at line 776, Update the direct subsections under the `##
Bitbucket` heading, including `Step 1`, `Step 2`, `Step 3`, and `No response?`,
from `####` to `###` so the heading hierarchy satisfies MD001; apply the same
correction to the additional occurrences identified in the comment.
Source: Linters/SAST tools
| - **`internal/{cli,slack,discord,telegram,whatsapp/cloud,whatsapp/whatsmeow,teams,github,gitlab,signal}`** — one `core.Adapter` implementation each. Each exposes `New(...)` returning a `*core.Bot` built via `core.New(botType, adapter)`, plus package-level accessors (`slack.Client`, `discord.Session`, …) that recover the concrete adapter from a `*Bot` via `core.AdapterAs[T]` and hand back the raw client. WhatsApp Cloud API, Teams, GitHub and GitLab are webhook adapters: they run their own HTTP server (`Connect` binds a listener; `Disconnect` shuts it down + drains in-flight dispatch) and reply over REST. Teams authenticates every inbound request against the Bot Connector JWKS (its only third-party dep, `golang-jwt/jwt/v5`, used solely for signature/claim verification) and routes replies via an adapter-side `conversationID→serviceUrl` map populated on inbound Activities. GitHub verifies `issue_comment` webhooks and replies via the REST API using `google/go-github`, authenticating with either a PAT or a GitHub App (`bradleyfalzon/ghinstallation`), both under one `Config`. Two optional `Config` callbacks (the whatsmeow-`QRCallback` pattern — GitHub-only events, so no core ingress path) route further webhook deliveries on the same endpoint: `OnPullRequest` (actions opened/reopened/synchronize only, self/bot authors dropped via `isSelfOrBotUser`) and `OnPush` (unfiltered); nil callbacks keep those deliveries acked-and-dropped, and both run on drain-covered dispatch goroutines. | ||
| - **`internal/{cli,slack,discord,telegram,whatsapp/cloud,whatsapp/whatsmeow,teams,github,gitlab,bitbucket,signal}`** — one `core.Adapter` implementation each. Each exposes `New(...)` returning a `*core.Bot` built via `core.New(botType, adapter)`, plus package-level accessors (`slack.Client`, `discord.Session`, …) that recover the concrete adapter from a `*Bot` via `core.AdapterAs[T]` and hand back the raw client. WhatsApp Cloud API, Teams, GitHub, GitLab and Bitbucket are webhook adapters: they run their own HTTP server (`Connect` binds a listener; `Disconnect` shuts it down + drains in-flight dispatch) and reply over REST. Teams authenticates every inbound request against the Bot Connector JWKS (its only third-party dep, `golang-jwt/jwt/v5`, used solely for signature/claim verification) and routes replies via an adapter-side `conversationID→serviceUrl` map populated on inbound Activities. GitHub verifies `issue_comment` webhooks and replies via the REST API using `google/go-github`, authenticating with either a PAT or a GitHub App (`bradleyfalzon/ghinstallation`), both under one `Config`. Two optional `Config` callbacks (the whatsmeow-`QRCallback` pattern — GitHub-only events, so no core ingress path) route further webhook deliveries on the same endpoint: `OnPullRequest` (actions opened/reopened/synchronize only, self/bot authors dropped via `isSelfOrBotUser`) and `OnPush` (unfiltered); nil callbacks keep those deliveries acked-and-dropped, and both run on drain-covered dispatch goroutines. | ||
|
|
||
| GitLab mirrors GitHub's shape with GitLab-native seams: it authenticates each delivery by comparing the `X-Gitlab-Token` header (constant-time) against `Config.Secret` **before** reading the body — no body HMAC, unlike GitHub — and replies via `gitlab.com/gitlab-org/api/client-go` notes. Auth is a single access-token mode (personal/project/group token, sent as `Private-Token`), so there is no ambiguous-auth error and no ghinstallation; self-identity is resolved via `GET /user` at Connect (fatal on failure) and the bot's own notes are dropped by author id (GitLab note webhooks carry no reliable bot flag, so arbitrary *other* bots are not filtered — the Slack situation). `client-go`'s `ParseWebhook` splits a Note Hook by noteable type into typed `IssueCommentEvent`/`MergeCommentEvent` (commit/snippet notes and system/edited notes are acked-and-dropped); the `Confidential Note Hook` trigger is shaped identically and routes through the same handler, but GitLab picks that trigger when the note is internal **or** its noteable is confidential, so internal notes are dropped on the note's own `object_attributes.internal` flag — GitLab only ships it since **18.6**, client-go's typed events do not expose it, so `internalNote` side-parses the raw body — with the noteable as the second discriminator for a payload that arrives without the flag (on that trigger, a non-confidential issue or any MR, since MRs cannot be confidential, means the note is internal by elimination). A note on a confidential issue dispatches (the plain-note reply inherits the issue's audience); an internal note is acked-and-dropped, since `Send` cannot answer in kind — and it is the one filtered drop that logs (`Debug`). **Below 18.6 the flag is absent**, leaving only the noteable filter, which cannot see past a confidential issue: an internal note there is answered with a plain note, widening its audience. The note's pre-rename `confidential` spelling was never in a note payload at all (the only `confidential` key describes the noteable), so do not "restore" it as a fallback — it would drop ordinary comments on confidential issues. `ChannelID` is GitLab-native `group/project#iid` (issue) or `group/project!iid` (merge request). **Every authentic delivery acks 200** — including dropped, unreadable (over the body cap or truncated) and concurrency-shed ones; the unreadable/unparseable and concurrency-shed ones are logged as warnings, while filtered drops (system/internal/edited/self notes, commit and snippet notes, nil-callback and non-matching events) are silent: GitLab never re-delivers a failed webhook and counts *consecutive* failures (4xx, 5xx, timeouts and other HTTP errors alike; one success resets the count) toward auto-disabling the hook, temporarily first and then permanently — the current thresholds live in `_docs/platforms.md` only, so they are corrected in one place — so a non-200 loses the delivery *and* suppresses later ones. Only the bad-token 401 is non-200, deliberately. **No re-delivery is not exactly-once**, though: GitLab does not retry a failed delivery indefinitely, but the same payload can still arrive twice — a delivery that times out on our side (GitLab gives up after ~10s and records a failure even though the note already dispatched) or a manual *Resend* from the hook's *Recent events* tab. So handlers must be **idempotent**: never assume a delivered note is received exactly once. **This is a real divergence from the GitHub sibling** (which answers 400/503 there) — do not "sync" it back. The parity callbacks are `OnMergeRequest` (actions open/reopen/update, self author dropped) and `OnPush` (unfiltered). Reactions are a deliberate v1 omission (GitLab has a native Emoji webhook, so the correct future design is webhook ingress, not the REST poller GitHub needs — see the Reactions section). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the spelling in the GitLab section.
Replace each noteable occurrence on Line 36 with notable.
🧰 Tools
🪛 LanguageTool
[grammar] ~36-~36: Ensure spelling is correct
Context: ... the flag is absent**, leaving only the noteable filter, which cannot see past a confide...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` at line 36, In the GitLab section’s documentation, replace every
occurrence of “noteable” with the correctly spelled “notable,” including
compound references such as noteable type and noteable filter; leave the
surrounding behavior and terminology unchanged.
Source: Linters/SAST tools
…ield newAdapter only flagged ambiguous auth when BOTH Email and APIToken were set alongside AccessToken. Setting an access token plus just one Basic field (Email or APIToken) silently fell through to access-token mode, ignoring the stray field. Reject any access token paired with either Basic field, and add regression cases plus a Data Center access-token valid case and the OnPullRequest cap-scaling case.
…reusable The Data Center reply path closed resp.Body on 2xx without reading it, which prevents the keep-alive connection from being reused. Drain a bounded amount before returning.
The transitive isolation_deps_test already proves ktrysmt is confined to the bitbucket closure, but the fast direct-import guards omitted it. Add it to the root guard and every non-bitbucket platform guard, matching how go-github and client-go are banned everywhere.
…ra cases - recordingServer's capture renamed off the 'cap' builtin and guarded by a mutex so field access is race-safe. - Reuse the discardLogger helper across server_test instead of repeating slog.New(slog.NewTextHandler(io.Discard, nil)). - Add a fractional-second + explicit-offset timestamp case (parseRFC3339 must preserve sub-second precision), and make the cancelled-probe test block on the request context instead of a fixed 500ms sleep.
… support Pull-request comments work on both flavors; issue comments are Cloud only. Distinguish Bitbucket's credential requirement (a token against Cloud or a self-hosted Data Center instance) from the cloud-only webhook platforms.
|
/loop |
|
🔁 Review-fix loop started — target 9/10, up to 3 fix round(s). |
|
✅ No issues found.
|
There was a problem hiding this comment.
Verdict: approve · Grade: 9/10
To reach a higher grade: Nothing blocking; the only residual risk is the external assumption that Bitbucket Cloud emits X-Hub-Signature when a secret is configured, which cannot be verified from the diff.
📝 Summary of changes
Overview
Adds a Bitbucket platform adapter (Cloud + Data Center in one package, selected at runtime by Config.BaseURL), following the established GitHub/GitLab webhook-adapter shape. The flavor divergence (event keys, payload shapes, reply bodies, self-identity) is hidden behind an unexported flavor interface so server.go never branches on flavor — a clean design that keeps the lifecycle code flavor-independent.
The change spans: the internal adapter (internal/bitbucket/{bitbucket,message,send,server}.go + thorough tests), the public wrapper (bitbucket/), a new BotType const + String() case, the isolation-deps and import-guard sweep across every sibling package, and a complete documentation sweep (CLAUDE.md, botbooter.go doc, README.md, _docs/platforms.md, _examples/basic).
Assessment
This is a high-quality, well-scoped addition. The lifecycle scaffolding (per-connection dispatch semaphore snapshotted under the lock, readSem/body-cap pre-auth buffering bound, detached dispatch context, drain-before-cancel, identity-compared ctx-watch teardown) is ported guard-for-guard from the GitLab sibling and correct. Auth-mode validation, Self-required logic for access-token/Data-Center, channel-id parsing with validSegment traversal hardening (including the ~USERNAME personal-repo case), constant-time signature check, self-drop reply-loop guard keyed on actor.uuid/actor.slug, and threading via parent.id are all implemented consistently and matched by tests. The isolation-deps additions correctly assert ktrysmt/go-bitbucket is confined to the bitbucket closure and absent everywhere else, and every sibling import guard bans it.
I could not find a concrete correctness, security, or concurrency defect. One external-fact dependency carries residual risk that I cannot verify from the diff: the whole Cloud path depends on Bitbucket Cloud emitting X-Hub-Signature: sha256=… when a webhook secret is set (the author states this was confirmed against current Atlassian docs). If that assumption were wrong the adapter would 401 every Cloud delivery, but there is no evidence in the code that it is incorrect, so I am not raising it as a finding.
Documentation is thorough and the platform enumeration sweep is complete and in the correct BotType iota order.
✅ No issues found.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, /auto-fix to have me attempt the fixes, or /clean to remove my comments.
|
🔁 Review-fix loop finished — target 9/10.
|
1 participant
What
Adds Bitbucket as a botbooter platform: a consumer writes one bot and runs it on Bitbucket pull-request and issue comments the same way it already runs on GitHub
issue_commentand GitLab Note Hook deliveries.Cloud + Data Center in one package, selected at runtime by
Config.BaseURL(empty → Cloud REST 2.0 viaktrysmt/go-bitbucket; a value → Data Center REST 1.0 via plainnet/http, no second dependency). This is not the two-flavor WhatsApp import-path split — the flavor divergence (event keys, payload shapes, reply bodies, self-identity) lives behind an unexportedflavorinterface, soserver.goroutes on a flavor-computed category and never branches on flavor.Design
email:token) or a repository/project/workspace access token (Bearer). App passwords are never offered — removed platform-wide 2026-07-28. Exactly one mode is required.X-Hub-SignatureHMAC-SHA256 over the raw body (same header/algorithm on both flavors), verified constant-time (hmac.Equal). The body is read before auth (GitHub's posture, since the signature covers it).actor.uuidon Cloud (account_idis GDPR-unreliable) andactor.slugon Data Center.Config.Selfis resolved viaGET /2.0/userin Cloud API-token mode and required in access-token mode and on Data Center (that endpoint 401s for access tokens; DC has no whoami).parent.idon both flavors → implementscore.ThreadedSender.workspace/repo#Nis rejected on DC. Comment edits are ignored (separate unhandled key). Reactions are permanently omitted — Bitbucket has none.OnPullRequest/OnPushcallbacks route those deliveries on the same endpoint; nil callbacks ack-and-drop. Every authentic delivery acks200except the bad-signature401, and handlers must be idempotent (Bitbucket Cloud retries a timed-out delivery).workspace/repo!N(PR),workspace/repo#N(Cloud issue),PROJECTKEY/repo!N/~USERNAME/repo!N(Data Center, incl. personal repos).The lifecycle (per-connection semaphore snapshot, drain, detached dispatch context,
Addr/Pathnormalization) is ported frominternal/gitlabguard-for-guard — intentional duplication perCLAUDE.md, not hoisted into a shared helper.Verification
make allclean:go vet(incl._examples), golangci-lint v2 (0 issues),go test -race ./....internal/bitbucketcoverage 97.2% (≥ the GitLab sibling's 97.0%); public facade 100%.isolation_deps_testprovesktrysmt/go-bitbucketis in thebitbucketbuild closure and absent from every other platform (and no other SDK leaks intobitbucket).~USERNAME) were unaddressable — is fixed (~admitted invalidSegment, traversal guarantee preserved).X-Hub-Signature: sha256=…when a webhook secret is configured.Docs
Full platform-enumeration sweep in
BotTypeiota order (between GitLab and Signal):CLAUDE.md,botbooterpackage doc,README.md,_docs/platforms.md(new Bitbucket section covering both flavors, both auth modes, theConfig.Selfrequirement and why, the idempotency warning, and the no-issue-tracker / no-reactions omissions)._examples/basicgains abitbucketcase.Summary by CodeRabbit
New Features
Documentation