fix(bitbucket): treat empty whoami identity as fatal at Connect - #53
Conversation
On Cloud API-token mode, self-identity is resolved via GET /2.0/user. A successful response carrying an empty uuid was stored verbatim, leaving a.self == "". isSelf then never matches, silently disabling the reply-loop guard so the bot's own comments are dispatched. Treat an empty resolved identity like a resolve failure (deps.Done + close listener), matching the GitLab sibling's fatal-on-unresolved-self posture.
|
✅ No issues found.
|
📝 WalkthroughWalkthroughBitbucket connection startup now treats an empty successful self-identity lookup as a fatal configuration error. A test verifies that ChangesBitbucket identity validation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Verdict: approve · Grade: 9/10
📝 Summary of changes
This is a small, well-targeted fix in internal/bitbucket/server.go. On Cloud API-token mode the bot's self-identity is resolved via GET /2.0/user; previously only a non-nil error was treated as fatal, so a successful whoami returning a blank uuid would leave a.self == "", silently disabling the isSelf reply-loop guard. The change treats an empty-but-error-free identity as a fatal resolve failure (surfaced via deps.Done, listener closed), mirroring the GitLab sibling's non-zero selfID requirement. A focused test TestConnectWhoamiEmptyIdentityFatal exercises the new path and asserts the error surfaces via Done as ErrMissingConfig.
The logic is correct: only the Cloud path (cfg.Self == "") reaches the guard, Data Center requires Config.Self at New, and fmt/ErrMissingConfig are already used in this package. The reasoning in the PR description matches the code, and there is no attempt to steer the review. Clean change.
✅ 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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/bitbucket/server_test.go (1)
397-404: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover listener shutdown and clean up the asynchronous connection.
The test verifies
DoneandErrMissingConfig, but it does not verify that the listener is closed and no webhook is served after the fatal result. Use a cancelable context, clean it up after the test, and assert that the bound address does not accept a request afterDone. This also prevents theConnectcontext watcher from remaining blocked oncontext.Background().This covers the listener-closure requirement in the PR objective.
🤖 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/server_test.go` around lines 397 - 404, Update the asynchronous connection test around Connect to use a cancelable context, defer its cleanup, and pass it instead of context.Background() so the context watcher cannot remain blocked. After receiving ErrMissingConfig from done, verify the listener’s bound address no longer accepts webhook requests, while preserving the timeout assertion for Done.
🤖 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.
Nitpick comments:
In `@internal/bitbucket/server_test.go`:
- Around line 397-404: Update the asynchronous connection test around Connect to
use a cancelable context, defer its cleanup, and pass it instead of
context.Background() so the context watcher cannot remain blocked. After
receiving ErrMissingConfig from done, verify the listener’s bound address no
longer accepts webhook requests, while preserving the timeout assertion for
Done.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 42d72000-8e8e-498d-8f16-892d2847ad1c
📒 Files selected for processing (2)
internal/bitbucket/server.gointernal/bitbucket/server_test.go
Addresses the finding in #52.
Problem
On Cloud API-token mode, the bot's self-identity is resolved via
GET /2.0/userat Connect. The code treated only a non-nil error as fatal — a successful whoami returning an emptyuuidwas stored verbatim, leavinga.self == "".isSelfisself != "" && id == self, so an empty self never matches: the bot's own comments are never dropped, opening a reply loop. The GitLab sibling makes an unresolved self fatal by design; this path let an empty-but-error-free result slip through.Fix
Treat an empty resolved identity the same as a resolve failure (
deps.Done+ close the listener) so a bot that cannot recognize itself never starts serving. Data Center is unaffected — it requiresConfig.SelfatNew, so the resolve block never runs.internal/bitbucket/server.go— 3-line guard. New testTestConnectWhoamiEmptyIdentityFatalcovers the empty-identity path.Verification
make allclean (fmt + vet + lint + test-race, incl._examples).🤖 Generated with Claude Code
Summary by CodeRabbit