Repository navigation
Conversation
…tory Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deploying contributing-docs with
|
| Latest commit: |
389da1c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e1d9e6c2.contributing-docs.pages.dev |
| Branch Preview URL: | https://docs-adr-0035-product-reposi.contributing-docs.pages.dev |
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR adds one file, Code Review DetailsNo new findings. One thread is still unresolved but outdated: the inline-SVG diagram thread. The SVG has since been removed and no diagram remains, so nothing needs to change. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 1000 560" fontFamily="ui-sans-serif, system-ui, sans-serif"> | ||
| <text x="120" y="70" fill="#1a1a1a" fontSize="30">SDK</text> | ||
| <text x="472" y="66" fill="#888" fontSize="26">NPM</text> | ||
| <text x="800" y="70" fill="#1a1a1a" fontSize="30">Clients</text> | ||
|
|
||
| <line x1="500" y1="110" x2="500" y2="490" fill="none" stroke="#888" strokeWidth="2" /> | ||
|
|
||
| <g fill="none" stroke="#1a1a1a" strokeWidth="3" strokeLinecap="round"> | ||
| <path d="M70,120 L290,120 C360,120 380,300 450,300" /> | ||
| <path d="M70,165 L240,165 C330,165 370,300 450,300" /> | ||
| <path d="M70,210 L190,210 C300,210 350,300 450,300" /> | ||
| <path d="M70,480 L290,480 C360,480 380,300 450,300" /> | ||
| <path d="M70,435 L240,435 C330,435 370,300 450,300" /> | ||
| <path d="M70,390 L190,390 C300,390 350,300 450,300" /> | ||
| <path d="M70,300 L450,300" /> | ||
| <path d="M550,300 C650,300 700,210 810,210 L930,210" /> | ||
| <path d="M550,300 C630,300 660,165 760,165 L930,165" /> | ||
| <path d="M550,300 C620,300 640,120 710,120 L930,120" /> | ||
| <path d="M550,300 C650,300 700,390 810,390 L930,390" /> | ||
| <path d="M550,300 C630,300 660,435 760,435 L930,435" /> | ||
| <path d="M550,300 C620,300 640,480 710,480 L930,480" /> | ||
| <path d="M550,300 L930,300" /> | ||
| </g> | ||
|
|
||
| <path d="M450,300 L550,300" fill="none" stroke="#1a1a1a" strokeWidth="3" strokeLinecap="round" strokeDasharray="2 14" /> | ||
| </svg> |
There was a problem hiding this comment.
♻️ DEBT: Diagram is inline SVG rather than Mermaid, and carries no perspective caption
Details and fix
The diagram standard adopted in ADR-0033 states rule 1 as "Diagrams are Mermaid source text, nothing else" and rule 4 as "Every diagram carries a perspective caption: audience, intent, and scope", attached as an italic blockquote below the diagram. This is the only inline <svg> in docs/; the two remaining SVGs are the autofill assets that ADR-0033 records as a tracked deviation until replacements ship, and this site is meant to be the standard's reference implementation.
There is also a concrete rendering consequence: the strokes and text are hardcoded to #1a1a1a (and #888). docusaurus.config.js sets no colorMode override, so the default dark-mode toggle is available, and in dark mode near-black lines on the dark background make the diagram effectively invisible. Mermaid inherits the site theme automatically, and markdown.mermaid is already enabled.
A flowchart LR with the contributing teams fanning into a single publish node and out to the consuming clients would carry the same "serialized pipeline" message, in theme-aware, diffable source.
There was a problem hiding this comment.
Needs to be converted to mermaid.
There was a problem hiding this comment.
Can't, I'll probably end up removing it
There was a problem hiding this comment.
I don't believe we are currently in a place where we can efficiently deal with separate repositories for products. The SM SDK constantly lags behind "sdk-internal" and without process changes where "core" is forced to be stable with longer upgrade cycles we're just moving the pain from sdk-internal <-> clients to sdk-core <-> sdk-product.
If we start banning breaking changes in sdk-core, and have it adopt a regular release cadence, and we support past + current release with bug fixes then that could work. But if sdk-core continues with frequent breaking changes you still have to play catch-up. And looking at the current velocity I would expect the turmoil in "core" to continue.
Anything preventing us from taking along core into clients? It already hosts the SM clients, and there is no near term plan to change that. Rather we would be forced to put the SM SDK there anyway to maintain the web experience.
True, and
is exactly how the proposal addresses this:
but maybe the text doesn't fully elaborate on exactly what this means and the consequences. I'll update the doc!
Revisiting the data it seems like you have a point here. According to some new data I had Claude assemble there are significant changes in the core crates. The difference is that we would be minimizing the amount of teams creating these breaks to platform/core teams. Having a stable API for crypto can also be seen as desirable.
Not technically, no. My main reason for recommending the product-repository approach was mainly to more closely follow MSA architecture in general. This gathers closely related code and allows separation of code that is unrelated, organizing code by verticals instead of horizontals if you will. Of course, the initial proposal I brought up was |
|
Since If all the platform teams are alright enrolling in a release cadence, commit to stable APIs and maintain multiple parallel versions with bugfixes then I have no objection. |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
coroiu
changed the title
|
@Hinton I've taken peoples feedback into account and revised the ADR to propose the original monorepo approach which we had all agreed on. I left product repo as a considered but rejected alternative 👍 Thank you for you feedback!
I think there's still a chance we might to exactly this at some point in the future (the ADR was exactly this commitment) but like you say, today it's not a strict requirement, so no need to take on more work that necessary |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MGibson1
left a comment
MGibson1
left a comment
There was a problem hiding this comment.
I've long been an advocate of monorepos. I think this ADR brings solid data to the table that this will help the web clients, but I do worry about the mobile clients, still
it would be nice to understand if similar headaches exist there while considering this.
They do to a certain degree. Mobile is still disconnected from regular engineering teams though and I'm somewhat concerned what coupling SDK changes would mean. In many ways they are better suited for a stable release schedule with backported bugfixes. I think web clients are less suited for that because a 1 week or 2 week delay would be to disruptive. |
Hinton
left a comment
Hinton
left a comment
There was a problem hiding this comment.
Overall positive to this. Mobile is somewhat concerning, what would bringing that into our repository look like. Would we be willing to accept breaking changes to the SDK requirering mobile updates?
| <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 1000 560" fontFamily="ui-sans-serif, system-ui, sans-serif"> | ||
| <text x="120" y="70" fill="#1a1a1a" fontSize="30">SDK</text> | ||
| <text x="472" y="66" fill="#888" fontSize="26">NPM</text> | ||
| <text x="800" y="70" fill="#1a1a1a" fontSize="30">Clients</text> | ||
|
|
||
| <line x1="500" y1="110" x2="500" y2="490" fill="none" stroke="#888" strokeWidth="2" /> | ||
|
|
||
| <g fill="none" stroke="#1a1a1a" strokeWidth="3" strokeLinecap="round"> | ||
| <path d="M70,120 L290,120 C360,120 380,300 450,300" /> | ||
| <path d="M70,165 L240,165 C330,165 370,300 450,300" /> | ||
| <path d="M70,210 L190,210 C300,210 350,300 450,300" /> | ||
| <path d="M70,480 L290,480 C360,480 380,300 450,300" /> | ||
| <path d="M70,435 L240,435 C330,435 370,300 450,300" /> | ||
| <path d="M70,390 L190,390 C300,390 350,300 450,300" /> | ||
| <path d="M70,300 L450,300" /> | ||
| <path d="M550,300 C650,300 700,210 810,210 L930,210" /> | ||
| <path d="M550,300 C630,300 660,165 760,165 L930,165" /> | ||
| <path d="M550,300 C620,300 640,120 710,120 L930,120" /> | ||
| <path d="M550,300 C650,300 700,390 810,390 L930,390" /> | ||
| <path d="M550,300 C630,300 660,435 760,435 L930,435" /> | ||
| <path d="M550,300 C620,300 640,480 710,480 L930,480" /> | ||
| <path d="M550,300 L930,300" /> | ||
| </g> | ||
|
|
||
| <path d="M450,300 L550,300" fill="none" stroke="#1a1a1a" strokeWidth="3" strokeLinecap="round" strokeDasharray="2 14" /> | ||
| </svg> |
There was a problem hiding this comment.
Needs to be converted to mermaid.
|
Note that this needs renumbering now. |
…cope Add versioned SDK releases as a considered option, decide that android and iOS stay out of the monorepo with commitments to improve the mobile boundary, scope the positive consequence to the 18 clients events, and tighten wording. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
coroiu
changed the title
Event counts misrepresent impact, which is better measured in person-hours. The inline SVG does not follow the Mermaid diagram standard. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
To be honest, when we first talked about this I was neutral, but at this point I am (slightly) opposed to a mono repo, at least for the reasons stated in this ADR. It does not solve the problem for mobile or external consumers of our SDK and possibly makes the problem worse by making breaking changes easier / more acceptable within clients.
Further, the problem we had in the first place is simply much less frequent now that the AI team has added the auto-fix workflow. Breaking changes are fairly automatically resolved, so it's not really an issue? On the other end we have a large effort we are doing here, for an unclear outcome.
| A 12-month measurement (2025-09-01 to 2026-08-31, read from merged PRs and git history) shows the | ||
| shape of the problem: | ||
|
|
||
| - **68 forced downstream breaking events**: clients 18, android 28, iOS 22. The earlier six months |
There was a problem hiding this comment.
Question: Now that we have the LLM-auto-fix workflow in place, does this argument still hold?
Mots changes are auto-resolved. cc @SaintPatrck
There was a problem hiding this comment.
I spoke with @vvolkgang regarding how this affects mobile and the impression I got was that, while AI does help with minor things, there are still a lot of changes that require human intervention, especially breaking changes that only show up during runtime. As for clients, do we have an AI job there now?
There was a problem hiding this comment.
As for clients, do we have an AI job there now?
Yes, we had it sucecssfully auto-resolve some things already.
There was a problem hiding this comment.
Ok nice, I haven't seen it yet and the ADR has taking way to long to get published. I think I'll have to evaluate how it performs before I can comment
|
|
||
| - **68 forced downstream breaking events**: clients 18, android 28, iOS 22. The earlier six months | ||
| had 25; the recent six months had 43, about 1.7x, with August the peak at 16. | ||
| - **About 20% of merges to `sdk-internal` main are breaking**: 149 of 754. Distinct authors per |
There was a problem hiding this comment.
Why are they breaking? Have we investigated that?
To me that suggests the interfaces are bad, which is an issue regardless of the PR management.
There was a problem hiding this comment.
The SDK owns the data model, and new features modify the data model. At the moment that causes hard breakages since the conversion is not lossy as that may introduce data loss instead it forces the consumer to decide what to do with the change.
| (and a Maven package for android, a Swift package for iOS). Every breaking change in the SDK has to | ||
| travel through that publish step before any consumer can adopt the change that follows it. That | ||
| turns delivery into a single serialized pipeline: many teams write to the SDK, everything funnels | ||
| through one published version, and many teams consume it on the other side. One team's unfinished |
There was a problem hiding this comment.
One team's unfinished downstream fix holds up everyone waiting for a later SDK change
Does this still hold true with the auto-fix workflow treated by the AI team?
There was a problem hiding this comment.
Yes, codeowners. With timezones that's upwards of a 24h delay assuming the owning team prioritizes review.
| downstream fix holds up everyone waiting for a later SDK change. | ||
|
|
||
| Put another way: every breaking change in the SDK adds to a queue of fixes that then have to merge | ||
| on the clients side in the same order they merged in the SDK. If Team A merges a breaking change |
There was a problem hiding this comment.
I want to note, that in one of our slack channels, Brian fairly clearly stated that the SDK's purpose in not only internal teams (our mobile clients and clients), but external users too. This ADR currently ignores this fully, and the solution does not help those users.
The fact is that we make too many breaking changes, because our interfaces are designed ad-hoc for our clients rather than being designed from an "i want to design a stable SDK" perspective.
There was a problem hiding this comment.
I agree and also want the SDK to be the basis for something we eventually release as a "public SDK" (I'm not sure if this is actually a company goal yet though), but I don't think an "everything SDK" is how that will look, and that it will instead be released as product SDKs. As an analogy, you don't have a "Google everything API" or "Atlassian API" instead you get APIs for Google Maps, Google Search, Jira, Confluence, etc. Either way, where the code lives does not dictate what we can and can not release
The fact is that we make too many breaking changes, because our interfaces are designed ad-hoc for our clients rather than being designed from an "i want to design a stable SDK" perspective.
Yes, and I think this follows the same principles as the current server APIs, we have an internal API for our clients and we have a public API that follows separate rules when it comes to breaking changes. I envision the SDK doing the same, but I don't want to say "we can't move fast and break stuff because we have a public API that nobody currently uses"
There was a problem hiding this comment.
it will instead be released as product SDKs
Maybe we should then split the SDK into individual product SDKs ?
Or rather, since PM remains the main offering that have multiple clients, sdk-intenal would only contain PM client, while SM, PAM, (AC?) would either fork or completely separate out.
For "platform" stuff, including crypto, we could have a versioned, stable release, that many product SDKs could consume.
There was a problem hiding this comment.
There was a problem hiding this comment.
An external facing SDK is not currently on any roadmap. Such an SDK would have a very different release cadence and would most likely need to be a wrapper to facilitate LTS.
| surface changes (struct and field reshapes, renames, unions, new APIs), and the clients coupling | ||
| points are mostly vault, tools, admin-console, and importer code. | ||
|
|
||
| The reason to act is not the size of the problem today, it is the trend. The main driver is upstream |
There was a problem hiding this comment.
Right, so here we are saying "We will move sdk to clients, because we recognize we are making breaking changes, and will make more of them, so to make breaking changes less impactful, everything living in one merge is better".
I want to warn about this stance. It does not consider mobile, and external consumers of the SDK. Worse, it encourages the breaking change pattern by making breaking changes more comfortable to the team implementing the feature, without regard for external consumers, or our mobile team.
|
|
||
| ## Considered options | ||
|
|
||
| - **Do nothing, improve iteratively:** keep the repos separate and reduce breaks at the edges (SDK |
There was a problem hiding this comment.
Have we done a survey as to why breaking changes are breaking? Was the interface designed wrong in the first place? Is this just regular product development?
Can we address this by having guidelines as to what stable APIs should look like?
There was a problem hiding this comment.
Yes, I have https://bitwarden.atlassian.net/wiki/spaces/~62955327d9eae9006f348ae9/pages/3289186311/Breaking+changes+across+repositories and https://bitwarden.atlassian.net/wiki/spaces/~62955327d9eae9006f348ae9/pages/3340435458/SDK+breaking-change+event+log+raw+data as the basis for this PR.
Can we address this by having guidelines as to what stable APIs should look like?
You can go further than guidelines and block breaking changes in CI, at least the ones the static types can detect. That's the "Ban breaking changes" option. However you'd be paying for it by having to carry a backwards-compatible surface, and some changes can't be made additive at all. For example, cipher blob encryption (bitwarden/sdk-internal#1122) made Cipher.name optional, because blob-encrypted ciphers keep the name inside the encrypted blob. It broke clients, android, and iOS, and there was no additive way to do it. Changes like that would still need an explicit, coordinated exception.
You can also re-design interfaces and how objects are constructed, I mention this under Reduce breaks by surface design in the confluence page above:
Shape the SDK's exported surface so it is hard to break: let the SDK own construction (builders/constructors) so added fields don't break callers, make new fields optional by default, and minimise the fine-grained types clients have to construct or exhaustively match. Most breaks today come from clients hand-building SDK input structs.
but by introducing builders you are weakening static typing usefulness:
Oscar: I think another gotcha here is the lack of compile errors, meaning it’s easy to miss setting the field somewhere.
Andreas: Yeah, I feel like whatever we do we’re just pushing the slider back and forth between static safety and dynamic flexibility
There was a problem hiding this comment.
❓ The analysis in those docs explains the proximate causes of breaking changes. I wonder whether there's a root cause that we're missing?
- Are folks making mistakes because they're unfamiliar with Rust?
- Or perhaps because they don't know it's a breaking change?
- Or is our culture surrounding breaking changes too permissive? Dismissive?
- Is this an up-front design issue?
- Is it due to vulns or structural bugs?
| The SDK stays its own project inside `clients`, with its own crates, ownership, and published | ||
| packages for consumers outside the repository. | ||
|
|
||
| The monorepo covers the SDK and the clients. Android and iOS stay in their own repositories and keep |
There was a problem hiding this comment.
As per above, this will not solve the issue for iOS/Android and possibly will make it worse.
|
|
||
| ### Positive consequences | ||
|
|
||
| - Breaking changes between the SDK and the clients stop being cross-repo events. A change to the SDK |
There was a problem hiding this comment.
I would consider this bad framing / false. Yes, one client set is prevented from breaking. External clients and mobile will still break, and if we don't change the fact that we are making breaking changes, all we are doing here is enabling more frequent breaking changes, which is worse for both mobile and external consumers compared to now.
There was a problem hiding this comment.
With this stance, I would not consider that we can offer the SDK as something external customers can use.
There was a problem hiding this comment.
clients here being the clients in the clients repo, not in the general sense. Yeah, it's confusing, I should probably change this
There was a problem hiding this comment.
Even if this referred to the clients repo, i would hold the same stance of framing this as a benefit being false framing, specifically because the benfit directly means us encouraging more breaking changes by making them easier.
What I'm saying is that this "benefit" is not a benefit but a downside, when viewed in context.
There was a problem hiding this comment.
The ADR is long and verbose. That is good because it's an important decision because it's moving a key part of our code into another. However, the comprehension fatigue is legitimate. Please consider my notes and any other places where anything is duplicated or overstated.
Otherwise, I support the effort and think this is a great idea for the team and our code health long term!
| through one published version, and many teams consume it on the other side. One team's unfinished | ||
| downstream fix holds up everyone waiting for a later SDK change. | ||
|
|
||
| Put another way: every breaking change in the SDK adds to a queue of fixes that then have to merge |
There was a problem hiding this comment.
🎨/♻️ Please consider refactoring the following sentence because it reads as throat clearing.
Put another way: every breaking change in the SDK adds to a queue of fixes that then have to merge
to
Every breaking change in the SDK adds to a queue of fixes that then have to merge
|
|
||
| - **68 forced downstream breaking events**: clients 18, android 28, iOS 22. The earlier six months | ||
| had 25; the recent six months had 43, about 1.7x, with August the peak at 16. | ||
| - **About 20% of merges to `sdk-internal` main are breaking**: 149 of 754. Distinct authors per |
There was a problem hiding this comment.
🎨 / ♻️ Please consider changing this sentence/phrase:
month grew from about 14 to about 29 over the year. More contributors means more breaking changes.
to
month grew from about 14 to about 29 over the year, alongside the growth in breaking merges.
| adaptation effort and the branch sitting open between re-bumps, not by waiting for a reviewer, and | ||
| per-break approval time cannot be cleanly separated from it. The cost across all three repos is | ||
| coordination and adaptation, not code review. | ||
| - Keep in mind that this is absolute time and the real cost is person-hours across the blocked |
There was a problem hiding this comment.
🎨 / ♻️ Please consider changing this sentence/phrase:
Keep in mind that this is absolute time and the real cost is person-hours across the blocked
to
This is absolute time. The real cost is person-hours across the blocked
| per-break approval time cannot be cleanly separated from it. The cost across all three repos is | ||
| coordination and adaptation, not code review. | ||
| - Keep in mind that this is absolute time and the real cost is person-hours across the blocked | ||
| teams. On repositories like `clients` that can easily mean an order of magnitude more |
There was a problem hiding this comment.
🎨 Please consider changing this sentence/phrase:
On repositories like clients that can easily mean an order of magnitude more
We need a count of people or teams blocked per break on clients to back "an order of magnitude," or perhaps we drop the magnitude and say the cost multiplies by the number of blocked people. 🤔
| ### Monorepo | ||
|
|
||
| Fold the whole SDK into the `clients` repository, the way `jslib` was folded in before. A change to | ||
| the SDK lands in the same commit as the consumer updates it requires, and main is never broken, |
There was a problem hiding this comment.
🎨 Please consider changing this sentence/phrase:
the SDK lands in the same commit as the consumer updates it requires, and main is never broken,
to
the SDK lands in the same commit as the consumer updates it requires, and main keeps building,
| as versioned packages that other consumers, such as the server, depend on; only the in-repo clients | ||
| drop the version boundary. The cost is a large one-time migration and a bigger repository. | ||
|
|
||
| ## Decision outcome |
There was a problem hiding this comment.
♻️ We have been working lately to be more concise with the decision as simply the choice.
Please read through this and consider what's restated that can be removed; or needs to be moved to the plan; or belongs in the Jira/Confluence document that is the actual work. If that Jira/Confluence section doesn't exist, then one suggestion is to add that to the PR description as a follow-up things that will happen if the ADR is accepted.
There was a problem hiding this comment.
Please see the last 4-5 ADRs that have been merged and accepted (starting with maybe the diagraming one) for potential ideas about what the direction has been as of late with this section in particular.
| - Look into automatically generated mobile fixes together with the mobile team. These will not be a | ||
| merge requirement: requiring a working or merged mobile PR before an SDK change can merge would | ||
| bring back the cross-repo blocker this decision removes. | ||
| - Work closer with the mobile team, and address problems at the boundary as we run into them. |
There was a problem hiding this comment.
❓ Who is working closer with the mobile team?
| send access, which is application logic, so like the client apps it is a consumer of the product, | ||
| not a shared crate. Its feasibility is unaffected by this decision. | ||
|
|
||
| The upfront migration is real work, but it is one-time work, and we have done a version of it before |
There was a problem hiding this comment.
♻️/🎨 This paragraph reads as a repeat of several places; mostly in the positive and negative consequences. Let's consider removing it.
|
@quexten Thank you for you feedback! I think it is obvious that you critical to the approach that the ADR currently recommends (and it's great that you are sharing that), but I'd be curious to also get your feedback on what you would prefer instead. Have you looked at the other alternatives in the ADR? Do any of them match your preferences, or is there an alternative that I've missed? |
|
@coroiu So, to solve this issue my first question is: What are the specific types of breaking changes we are seeing? Why was it necessary to perform a breaking change in the first place? (Given you cite statistics most likely gotten from a claude eval over the codebase, it should be easy to have those auto-categorized). Are these just a temporary issue due to us moving things into the SDK, or is this a persistent pattern? For example, when considering moving vault item crypto to the SDK, yes that is breaking, but it is only breaking now where the migration traffic is high. That won't be an issue a few months from now. So, as for my preference:
this is close, but I don't think simply banning is effective. You have to make it easy for people to succeed. Evidently, people just don't know how to write APIs right now that are stable. How do we help them? By giving them 1. examples of how to design an API that doesn't break / is stable in contrib docs / on existing clients 2. adding that as part of the claude code review. This should be based on the patterns that we have observed to be breaking.
This is close too, and closer to my preference. As part of adding an API we should always ask: "Who are the possible consumers, how will they consume it, and is it likely that this will change in the future?" If the answer is "this will change" that needs to be either planned for, or we need to re-think the API. Then, we should have way fewer breaking changes added. This would help both external consumers and mobile. I will say that mobile at least already uses a bunch of API surface that is not stable, so we will have breaking changes moving them off of it. The same applies to clients. |
|
|
||
| ### Plan | ||
|
|
||
| - Fold `sdk-internal` into the `clients` repository, following the `jslib` precedent, so the SDK and |
There was a problem hiding this comment.
Copying out of DM; this is missing a few steps:
- How are skills handled?
- How are claude reviews handled? Do we have one reviewer per sub project somehow?
- The context is too much and too different to fit across the whole repo
There was a problem hiding this comment.
Definitely a good point to look into!
When it comes to the context, we could have different reviewers for each language package, but having one reviewer for the entire thing also has it's benefits, like giving the agent the opportunity to review the entire feature from start to finish
@coroiu it very much is. I will note that most of the used API surface however, is considered deprecated (even if not yet marked as such). We are working on exposing only the stable crypto APIs, but as so often it's a prioritization thing... |
quexten
left a comment
quexten
left a comment
There was a problem hiding this comment.
To make my objection clear on the grounds that this change in my opinion (and other KM members opinion) makes the situation actively directly worse, by encouraging breaking changes and making them easier, I'm changing my review from "comment" to "Request changes".
My recommendation of stabilizing changes plus @mzieniukbw recommendation of making people prepare PR's here: https://bitwarden.slack.com/archives/C04LE162PHA/p1791803147344229?thread_ts=1791793761.811589&cid=C04LE162PHA apply.
| - **Versioned SDK releases:** keep the repos separate, but release the SDK as its own deliverable | ||
| every two weeks (semver, a changelog of breaking changes) instead of publishing on every merge to | ||
| main, and let consumers upgrade when it suits them. | ||
| - **Product repository:** split the SDK crates. Move the application-logic crates into a product |
There was a problem hiding this comment.
Pretty much all of these options are much lighter and reversible compared to the monorepo, which is irreversible. Should we find out that, as claimed in other comments, the monorepo does not have the intended effect, how do we revert that descision? Can we first try other, lighter options, and measuring their success?
| A 12-month measurement (2025-09-01 to 2026-08-31, read from merged PRs and git history) shows the | ||
| shape of the problem: | ||
|
|
||
| - **68 forced downstream breaking events**: clients 18, android 28, iOS 22. The earlier six months |
There was a problem hiding this comment.
Copying from slack:
- What is the nature of these breaking changes?
I.e trivial or hard to resolve? - Were they instantly resolved?
- How long was development actually stalled by these?
Next, most of these. are on mobile, with clients just being a minority share. 18 out of 68. This ADR only addresses clients.
| - One-time migration cost, with a precedent (`jslib`), against a coordination cost that otherwise | ||
| grows with the number of contributing teams. | ||
|
|
||
| ### Negative consequences |
There was a problem hiding this comment.
🤔 A few additional negative consequences:
- Rust's build output is very large (multiple gigabytes) and people with many working copies will have a copy of all build artifacts in every repository.
- Rust has long build times. People with a worktree-based workflows (including claude code run with
--worktree <desc>) will experience on every worktree. - This encourages people to make breaking changes to the client interfaces by lowering its cost. We may see the rate of breaking changes to client interfaces increase as a result (The Jevons paradox).
| - **The server's dependency on the SDK.** `server` currently depends on `sdk-internal` for | ||
| cryptographic and policy code. Folding the SDK into `clients` should not turn that into a |
There was a problem hiding this comment.
I don't think the SDK has been added to server yet. If it is, it's not used for policy code. (I did mention this as a future possibility, but no more than that.)
mzieniukbw
left a comment
mzieniukbw
left a comment
There was a problem hiding this comment.
The objective is questionable. The solution proposed does not address the problem. It degrades our productivity. In fact, the problem itself is questionable, since it is not a measure of the impact on the teams.
We have not tried enough and have not improved our current processes much, to be able to say, that, what we have is not working and have to look for the alternative, like the ADR proposes.
|
|
||
| We have tried to manage this without changing the structure. The breaking-change detection workflow | ||
| builds the latest `clients` `main` against SDK PRs so a breaking change can arrive with its fix PR | ||
| ready. It has not been enough: breaking changes have gone up, not down, even with the workflow in |
There was a problem hiding this comment.
Breaking changes is bad metric. Rather it should be, whether the team got impacted by the breaking change. In all clients, SDK update is a separate PR that does not immediately break main. AI's auto-fix only made it easier to handle.
Unless i am missing the point here, i don't feel like "breaking changes" should be treated as a root cause problem that we need to fix.
| A 12-month measurement (2025-09-01 to 2026-08-31, read from merged PRs and git history) shows the | ||
| shape of the problem: | ||
|
|
||
| - **68 forced downstream breaking events**: clients 18, android 28, iOS 22. The earlier six months |
There was a problem hiding this comment.
This is a problem with lack of any enforcements, rather than a problem that requires solving.
| within 14 days; 143 of 149 within 3 days. A consumer picking up a new version does not adopt one | ||
| break, it adopts a bundle of unrelated breaks at once. | ||
| - **Nothing gates the publish.** npm and Maven publish on every merge to main. The breaking-change | ||
| check runs on PRs, comments, and adds a `breaking-change` label, but never fails the job (a |
There was a problem hiding this comment.
I often found that the clients itself did not build due to issue not related to SDK change and added breaking-change label either way.
Have we filtered those out ?
| branch, with median branch windows of about 80 and 165 hours, but that window is dominated by | ||
| adaptation effort and the branch sitting open between re-bumps, not by waiting for a reviewer, and | ||
| per-break approval time cannot be cleanly separated from it. The cost across all three repos is | ||
| coordination and adaptation, not code review. |
There was a problem hiding this comment.
but that window is dominated by adaptation effort and the branch sitting open between re-bumps
The cost across all three repos is coordination and adaptation, not code review.
With AI auto-fix on mobile and AI driven development, the adaptation and code review efforts merged together.
I think we should re-consider this ADR entirely, since the amount of adaptation effort with AI auto-fix likely reduced from hours to minutes (adopt to all clients), which feels like the main driver of the ADR (effort / impact on teams).
| - **The churn is in the application layer.** Breaks across the dataset are dominated by SDK-owned | ||
| surface changes (struct and field reshapes, renames, unions, new APIs), and the clients coupling | ||
| points are mostly vault, tools, admin-console, and importer code. |
There was a problem hiding this comment.
Once mobile clients fully adopt the SDK on higher level designed SDK APIs, rather than low level Uniffi split structs and functions (mostly due to mTLS issue), the amount of breaking changes would reduce significantly.
Since this is upcoming effort planned by Platform team for this year, i would recommend to take an iterative approach: migrate to higher level SDK APIs in mobile clients and re-evaluate again in mid 2027.
We might find that 80% of a breaking changes for mobile were hidden behind this limitation.
| between them to keep in sync. A change to the SDK and the client code that uses it becomes one | ||
| change that only merges if it builds. The cross-repo breaking change stops being a separate thing to |
There was a problem hiding this comment.
A change to the SDK and the client code that uses it becomes one change that only merges if it builds.
This negatively impacts the PR, since both SDK and TS changes might have to bundled together. This would severely increase PR sizes and time required for a team to adopt them due to increased amount of dependent teams.
Teams might get discouraged by this (like KM team), and effectively will split the SDK and TS code, so neither breaks each other.
This is effectively the same as banning breaking changes in practise.
| as versioned packages that other consumers, such as the server, depend on; only the in-repo clients | ||
| drop the version boundary. The cost is a large one-time migration and a bigger repository. |
There was a problem hiding this comment.
only the in-repo clients drop the version boundary
Rust is known to be relatively slow to compile, hence additional 10-15 minutes compilation time every single time SDK changes is huge cost. In practice, that's every day. This will result in significant wait time to run lint, run tests or run client. All depends on he WASM build, which is not light.
9 participants
🎟️ Tracking
No Jira ticket. This ADR came out of a cross-repo breaking-changes investigation.
📔 Objective
Adds ADR 0047 (status: Proposed): move the SDK into the
clientsmonorepo, so the SDK and the clients build together and a change to the SDK lands in the same commit as the client updates it requires.The ADR records the problem (breaking changes serialized through the npm/Maven publish pipeline, worsening as more teams contribute to the SDK), the options considered (do nothing, additive-only, coordinated merge gate, versioned SDK releases, product repository, monorepo), and the decision with its consequences and plan (including keeping android and iOS out of the monorepo, with work to improve the mobile boundary), and follow-ups (the server's dependency on the SDK). The product repository is recorded as a considered alternative we can revisit if a second repository is ever warranted.
Proposed for discussion.
If you prefer you can also look at the hand-written version/starting point
Input for ADR
Ever since we introduced more teams into the SDK we've had a continuous challenge: Breaking changes.
Developers want to move fast, and the best way of doing that is using breaking changes, because of this
we've continuously tried to find ways of enabling this. An example of this is the breaking changes detection
workflow which coordinates with the
clientsmonorepo to build the latestmainclientscommit againstSDK PRs (this actually started with a purely static analysis of the TypeScript types on the SDK repo alone).
The goal of this workflow was to encourage and guide developers to have fix PRs for the breaking changes reviewed and ready for merge. Unfortunately, it hasn't achieved this to the degree that we were hoping for as we've seen breaking changes increase even with the workflow. I argue that not doing anything is not an option, as the situation will continue to deteriorate as more and more features and development moves to the SDK.
What is the problem with breaking changes?
The issues with breaking changes can be traced back to the SDK living in a separate repository and pulled into the clients as a published NPM package because of how it collapses the flow into a serialized delivery pipeline. In other words, it creates a bottleneck.
<style> .rail { fill: none; stroke: #1a1a1a; stroke-width: 3; stroke-linecap: round; } .dash { fill: none; stroke: #1a1a1a; stroke-width: 3; stroke-linecap: round; stroke-dasharray: 2 14; } .npm { fill: none; stroke: #888; stroke-width: 2; } .label { fill: #1a1a1a; font-size: 30px; } .npmlabel { fill: #888; font-size: 26px; } </style>SDK
NPM
Clients
Think of this in another way, every breaking change in the SDK adds to a queue of fixes that then need to be merged on the clients repo, in the same order as they were merged in the SDK. So if Team A merges a breaking change into the SDK, then every other team needs to wait for the corresponding clients fix until they can start pulling in SDK features merged after the breaking change.
I did some research by asking Claude to look at the previous 12 months. When reading it it's important to note that, while research may show a median breaking change on clients only taking a couple of hours to fix, that is a couple of hours where nobody else can merge. It's one thing to look at it from the perspective of absolute time, and another from the perspective of total person-hours affected. Here is what the research found: [claude: insert your research and conclusions here].
Side-note on Server - SDK breaking changes
These also present a challenge because of how we automatically generate API bindings from server commits.
However, this relationship is a bit different and can be split into two parts:
Compile time breaks
These are the ones that are most similar to the SDK - Clients breaks. If a breaking change is merged on the server then
a fix needs to be merged on the SDK before any additional changes after the break can be pulled in. The research shows [claude: insert research here (these are not as common)].
Run time breaks
These come from the fact that running SDK instances are not physically tied to a server version (SDK is client side, Server is server), meaning that the SDK needs to combine static type safety with the ability to handle unknown data coming from an unknown server version. This is fundamentally different from SDK - Client where the SDK is physically bundled with each client at compile time. We need to follow-up on this separately, but it is just a matter of implementing better support for these scenarios, and not an architectural re-design.
Alternatives considered
Coordinated merge gate (cross-repo/meta-repo)
Keep the repos separate, but link a breaking sdk-internal change to its clients/mobile
fixesand merge them together as one gated change set, essentially a distributed atomic merge. This would probably be implemented as an extension of the automations we already have.Advantages
Disadvantages
package.jsonversionBan breaking changes
Exactly what it says, ban breaking changes and force every update to be backwards compatible.
Advantages
Disadvantages
Do nothing - improve what we can
Continue allowing breaking changes, don't move any repositories, don't add any gates.
This actually turns into a collection of smaller solutions, some examples:
Advantages
Disadvantages
Monorepo
The solution that was chosen for
jslib: move the SDK into theclientsrepo.Advantages
Disadvantages
Product repository
Similar to the monorepo but taking Bitwarden's ambition of becoming a true multi-product company into account. The core idea here is: Move closely related code together to address the most frequent breaking changes but keep the core infrastructure separate and available for other products. There is a fundamental assumption being made here: Products will not need to share application logic at the SDK level.
Advantages
clientsrepo, e.g. Secrets Manager.Disadvantages
Solution chosen: Product repository
This solution combines a few key traits that makes it stand out as the best solution: Fastest development time, smallest maintenance cost. It's a one time effort, one that we've done before with
jslib, which results in an environment that lets us write any type of changes as often as we want without having to worry about compatibility.Some additional points [claude: try to merge these into flowing text with the section above]:
Open question
mobilenow? Or leave it as a standalone repo for now?Follow ups