BitBucket support - plan & discussion#16
Draft
awrobel-gd wants to merge 2 commits into
Draft
Conversation
…r resolution, and repository management. Introduce GitProvider abstraction for handling both GitHub and BitBucket, ensuring seamless integration and token sanitization across services. Update relevant tests and documentation to reflect new functionality.
7583647 to
3343fdb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review notes
Assessment vs. the plan
PR16 does not ship the plan in docs/plans/workspace-repos/bitbucket-support.md. It ships a much narrower, separate plan the PR itself introduces — docs/plans/workspace-repos/bitbucket-pr1-commit-and-read.md — which explicitly scopes down to "credentials + runtime" and "auto-provisioning" only, and explicitly defers the full GitProvider ABC/registry package, CI/CD, and Server/DC to later PRs. That narrowing is reasonable and disclosed, but it means the two questions have to be answered against what actually shipped:
Can you commit code and get P10Y scores for a BitBucket workspace right now? No — five separate bugs block this, several of them in the common/default path:
Does it set up for the future CI/CD (GHA→Pipelines) swap? Not well. create_generation_session_repos.py's main() threads ~12 separate if is_bitbucket: ... else: ... branches through owner selection, token selection, validation, and prints, rather than dispatching through the GitHostStrategy/registry the sibling full plan calls for. git_provider.py already models per-provider facts (default_git_user, sanitization_patterns) in a dataclass — but this script re-derives per-provider strings and flags ad hoc anyway. Bolting a DeployStrategy for BB Pipelines onto this shape means repeating the same two-way branch pattern in 3+ more places (workflow_steps.py, agents_claude_code.py, tool_usage.py) instead of registering one new strategy — a larger, riskier lift than if the minimal registry existed now.
Findings below, most severe first.
⏺ Code review(review · 6 findings)
● backend/app/services/github_auth.py:50 [correctness] — Default-workspace-pool auth resolution checks/returns raw
git_user_name_defaultinstead of the newactive_git_user_defaultfallback, breaking BitBucket's no-username default-pool flow.● backend/scripts/init_firestore.py:71 [correctness] —
WorkspaceConfig.p10y_repository_idis typed non-nullableintand raises in__post_init__, but BitBucket'semit_workspace_config()now deliberately writesnullfor that field.● backend/app/workflows/multi_workspace_estimation_p10y.py:105 [correctness] — Workspaces with
p10y_repository_id=None(every BitBucket-provisioned workspace, permanently) are silently and permanently skipped from P10Y estimation with no backfill mechanism anywhere in the repo.●backend/app/core/app_lifecycle.py:[correctness]—
_init_platform_secretsonly catchesValueError, butresolve_active_git_provider(now called unconditionally from_load_from_env) raisesGitProviderResolutionError, bypassing the memory-mode graceful-degradation fallback and hard-crashing startup.●backend/app/services/startup_validation.py:176[correctness] — FR-5 startup health check hardcodes
GITHUB_TOKEN_DEFAULT/GIT_USER_NAME_DEFAULTas required for non-k8s firestore/emulator deployments, with no BitBucket alternative, so a valid BitBucket-only deployment is reported unhealthy.●backend/scripts/create_generation_session_repo[altitude— Provider dispatch in
main()is ~12 separateif is_bitbucket: ... else: ...branches instead of routing through theGitHostStrategy/registry the sibling full plan (docs/plans/workspace-repos/bitbucket-support.md) calls for, which will make the upcoming CI/CD (GHA→Pipelines) work harder, not easier.These six findings answer both parts of the question directly:
Commit + P10Y MVP: not yet working. Five distinct, independently-triangulated bugs block it — the default-pool auth path breaks for BitBucket's no-username flow, the provisioning script emits JSON that crashes init_firestore.py, workspaces that do land in Firestore are permanently excluded from P10Y scoring with no backfill path, server startup can hard-crash in memory-mode dev with the new unhandled GitProviderResolutionError, and the health check misreports a valid BitBucket deployment as unhealthy.
Future CI/CD swap: not well-positioned. The provisioning script dispatches on is_bitbucket booleans threaded through ~12 branch points rather than the GitHostStrategy/registry pattern the full plan calls for, so each future CI/CD site (workflow_steps.py, agents_claude_code.py, tool_usage.py) will need its own hand-copied branch instead of one new strategy registration.