Repository navigation
feat(bitbucket): add Data Center review state writes - #1746
KatalKavya96 wants to merge 1 commit into
Conversation
|
Hi @potiuk, This is the next focused #606 Data Center parity follow-up. It enables the existing standalone review-state commands on Data Center through the participant-status API:
The implementation keeps the existing backend-neutral CLI and normalizers, adds API + CLI coverage for all four operations, and avoids the deprecated approval-specific endpoints. The combined Refs #606 |
github-actions
Bot
added
family:tools
Kaap10
left a comment
Kaap10
left a comment
There was a problem hiding this comment.
Thank you for implementing Bitbucket Data Center pull request approval writes for #606!
Review Summary
The participant status mapping (PUT .../participants/{userSlug} with APPROVED, UNAPPROVED, NEEDS_WORK) is the correct modern REST API approach for Bitbucket Data Center (Server 4.2+). Output normalization and capability documentation updates in README.md and docs/labels-and-capabilities.md are also well-structured.
However, there is a blocker regarding _current_user_slug that will cause review state writes to fail closed on live Data Center instances:
Major Blocker: Schema & Endpoint Mismatch on _current_user_slug
In tools/bitbucket/src/magpie_bitbucket/datacenter.py (lines 361-369):
def _current_user_slug(config: BitbucketConfig) -> str:
"""Return the authenticated Bitbucket Data Center user's URL-safe slug."""
user = get_json(f"{_api_base(config)}/users", config)
slug = user.get("slug")
if not isinstance(slug, str) or not slug.strip():
raise BitbucketError("Bitbucket Data Center current-user response did not include a user slug")
return quote_path(slug)-
Atlassian Data Center REST API v1.0 Contract:
- In Bitbucket Server / Data Center,
GET /rest/api/1.0/usersis the paged directory search endpoint (Page<User>). - The response schema is:
{ "size": 1, "limit": 25, "isLastPage": true, "values": [ { "name": "alice", "displayName": "Alice", "slug": "alice" } ], "start": 0 } - It contains no top-level
slugkey.user.get("slug")evaluates toNone, which will always raise:
BitbucketError: Bitbucket Data Center current-user response did not include a user slug - Furthermore,
/rest/api/1.0/usersreturns server-wide users alphabetically rather than the caller.
- In Bitbucket Server / Data Center,
-
Why Unit Tests Passed:
- In
tools/bitbucket/tests/test_bitbucket.py:3050, the test mock configured a flat dictionary:mock_opener( mock_build_opener, { "slug": "alice", "displayName": "Alice", "name": "alice", }, ... )
- This masked the endpoint mismatch.
- In
-
Remediation Options:
- Extract
X-AUSERNAMEheader: In Bitbucket Server / Data Center, all HTTP responses return theX-AUSERNAME: <username>header identifying the authenticated caller. - Support
config.username: Allow reading the slug fromconfig.username/BITBUCKET_DATACENTER_USER, with fallback toX-AUSERNAME.
- Extract
|
|
||
| def _current_user_slug(config: BitbucketConfig) -> str: | ||
| """Return the authenticated Bitbucket Data Center user's URL-safe slug.""" | ||
| user = get_json(f"{_api_base(config)}/users", config) |
There was a problem hiding this comment.
In Bitbucket Data Center REST API v1.0, GET /rest/api/1.0/users is a paged user-directory search returning {"values": [...], "size": ...} rather than a single user object. Because there is no top-level slug field, user.get("slug") returns None on live instances and raises BitbucketError. Consider extracting the X-AUSERNAME response header or reading config.username.
| match="Data Center pull request approval writes are not supported", | ||
| ): | ||
| datacenter.approve_pull_request(load_config(), "9") | ||
| mock_opener( |
There was a problem hiding this comment.
The mock simulates a flat {"slug": "alice"} object, which masks the paged Page<User> schema returned by /rest/api/1.0/users. Updating the test to mock the real endpoint schema will prevent regressions.
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
The participant-status mapping and the docs look right, but the current-user lookup cannot work against a real Data Center instance, so the two open threads from Kaap10 need addressing before this can merge.
current-user lookup (tools/bitbucket/src/magpie_bitbucket/datacenter.py:363)
user = get_json(f"{_api_base(config)}/users", config)
slug = user.get("slug")
Agreeing with the open thread from Kaap10: GET /rest/api/1.0/users is the paged user-directory search ({"values": [...]}), not a current-user endpoint, so slug is always missing and all four new commands fail on a live Data Center instance. Read the authenticated username from the X-AUSERNAME response header (Bitbucket Server/DC sets it on every authenticated response), or from a configured username, and resolve the slug from that.
Smaller observations
tools/bitbucket/tests/test_bitbucket.py:3050— the mock returns a flat user object, which is why the suite passes; mock the real response shape of whatever endpoint or header the fix uses
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Marking this as changes requested, so the PR's status matches where it stands: the points in my earlier review (#1746 (review)) are still open and need a change, or a reply on their threads. This review adds nothing new to them.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
Summary
pr approvepr unapprovepr request-changespr remove-request-changesType of change
.claude/skills/<name>/)tools/<system>/*.md)tools/*/withpyproject.toml)Implementation
Bitbucket Data Center review state is updated through the pull-request participant resource for the authenticated user.
The command mappings are:
pr approve→APPROVEDpr unapprove→UNAPPROVEDpr request-changes→NEEDS_WORKpr remove-request-changes→UNAPPROVEDThe bridge first resolves the authenticated Data Center user's slug, then updates that user's participant state on the target pull request.
This avoids the deprecated approval-specific endpoints and keeps the existing normalized approval/change-request output contracts unchanged across Cloud and Data Center.
Test plan
tools/bitbucket/tests/test_bitbucket.pysuite passesgit diff --checkpassesScope
This PR adds standalone Data Center review-state mutations only.
The combined:
pr review <id> --verdict {comment,approve,request-changes} --body-file <path>path remains Cloud-only because Data Center review-body + verdict semantics are a separate follow-up.
Data Center decline/reject and merge writes also remain separate follow-ups.
Refs #606