feat(cli): unified cross-platform patch numbering#3738
Conversation
Make `POST /v1/apps/{appId}/patches` idempotent on a user-supplied
correlation key so cross-platform builds resolve to one patch row
instead of one per platform.
Customer outcome: a developer who shipped "patch 3" sees the same
number on iOS and Android — in their analytics, in-app, and in their
release notes. The dominant CI flow (iOS bot publishes, Android bot
publishes minutes later, no shared state between them) collapses to
one patch as long as both bots pass the same `--patch-id`.
## Protocol — shorebird_code_push_protocol
- `CreatePatchRequest` gains optional `clientPatchId` (wire field
`client_patch_id`). `toJson` always emits the key (null when unset)
so server parsing doesn't have to special-case absence.
- `CreatePatchResponse` (new field) echoes `clientPatchId` back so
callers can verify whether an idempotent re-use occurred.
- `ReleasePatch` carries `clientPatchId` so existing GET endpoints
surface the correlation key for admin/console rendering.
## Client — shorebird_code_push_client
- `CodePushClient.createPatch` accepts and threads `clientPatchId`,
and now returns `CreatePatchResponse` instead of `Patch` so callers
can read the echoed key.
- Empty-string `clientPatchId` is normalized to null at the client
boundary — a caller that passes an unexpanded template variable or
empty flag shouldn't land on the idempotent path keyed on `''` and
inherit a stranger's patch. Done at the client rather than the
protocol so the protocol layer remains a faithful round-trip.
## CLI — shorebird_cli
- New `--patch-id=<string>` flag on `shorebird patch`. Common usage:
`--patch-id=\${{ github.sha }}` or any stable token
(`hotfix-login`, a CI run ID, etc.).
- Multi-platform invocations (`--platforms=ios,android`) auto-generate
a UUID and share it across every per-platform fan-out — single-
invocation is just cross-invocation-with-an-internal-ID.
- Append-after-promotion confirmation: when an idempotent hit lands
on a patch already promoted to stable, the new platform's artifacts
go live to that platform's stable users immediately. In interactive
mode the CLI prompts before continuing; in CI it proceeds silently
(the iOS-publishes-then-Android-completes flow is the canonical
use case and prompting would deadlock).
- Suppresses the prompt when the same run promoted the patch (e.g.
the first platform of a single `--platforms ios,android` invocation
— the user already opted in by passing both platforms together).
- Success line: `✅ Published Patch 3 (iOS, Android)` instead of one
log per platform; degenerate cases where platforms land on
different numbers each get their own line.
## Backwards compatibility
- Missing `client_patch_id` deserializes to null. Old clients that
don't set the field fall through the existing per-call create path
on the server.
- Old servers that don't recognize the field ignore it on
deserialization; the CLI flag becomes a no-op until the server
side ships, with no error surface.
This is the protocol + CLI half of unified patch numbering. The
server-side migration and idempotent createPatch live in the parent
repo and are gated behind the same `client_patch_id` correlation key.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
CI was failing on `dart format --set-exit-if-changed`. The cascade literals in the unified-success log tests exceeded 80 columns and need to be split across multiple lines.
|
|
||
| final List<ReleasePatch> existing; | ||
| try { | ||
| existing = await codePushClient.getPatches( |
There was a problem hiding this comment.
bdero's review bot:
💡 The dominant CI flow is "every run has a unique SHA" — so every publishPatch is a genuinely new patch row, but we still fetch the full patch list on every one of them just to confirm "not already on stable." Two server round-trips per platform instead of one, scaling with release-patch-count on the read path.
The cleanest fix lives server-side: have CreatePatchResponse return a created: bool (or echo back the patch's current channel) so the CLI can skip the getPatches lookup when the response says we created a new row. Worth raising with the server PR while it's still in flight.
| return; | ||
| } | ||
|
|
||
| final priorState = existing.firstWhereOrNull((p) => p.id == patch.id); |
There was a problem hiding this comment.
bdero's review bot:
💡 firstWhereOrNull((p) => p.id == patch.id) over the full list — if getPatches ever returns a windowed/recent slice (or starts to), an older patch's promotion state silently reads as "not on stable" and the courtesy prompt vanishes. Not sure if getPatches paginates today — worth confirming, and if there's a "fetch one patch by id" endpoint we should prefer that.
| String? _resolveClientPatchId() { | ||
| final explicit = results['patch-id'] as String?; | ||
| if (explicit != null && explicit.isNotEmpty) return explicit; | ||
| if (results.releaseTypes.length > 1) return const Uuid().v4(); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
bdero's review bot:
💡 if (explicit != null && explicit.isNotEmpty) return explicit; treats --patch-id="" and --patch-id (omitted) the same. The failure mode: someone writes --patch-id=${{ env.PATCH_SHA }} in their CI template, the env var is undefined, the flag arrives as "", and on a multi-platform invocation we silently generate a UUID — the iOS bot and the Android bot never converge because each generates its own. Loud failure when the flag is present but empty would surface this on the first broken CI run instead of after someone notices the patches aren't unifying.
| }) async { | ||
| // Coerce empty to null. A caller that passes an unexpanded template | ||
| // variable or empty flag would otherwise land on the idempotent path | ||
| // keyed on `''` and inherit a stranger's patch. |
There was a problem hiding this comment.
bdero's review bot:
💡 The PR description argues normalization belongs at the client boundary (so the protocol layer stays a faithful round-trip), which I agree with — but then _resolveClientPatchId in patch_command.dart also normalizes empty-to-null at the command layer. Pick one. If the "error on --patch-id=""" suggestion lands, the command-layer normalization goes away naturally and this stays the only normalizer.
| final platforms = | ||
| byPatchNumber[number]!.map((p) => p.displayName).toList()..sort(); | ||
| logger.success( | ||
| '\n✅ Published Patch $number (${platforms.join(', ')})', |
There was a problem hiding this comment.
bdero's review bot:
🧹 Lost the ! — was Published Patch X!, now Published Patch X (Android, iOS). The ! matches the celebratory tone of other logger.success lines in the CLI. Published Patch 3 (Android, iOS)! keeps it consistent.
| /// Used to suppress the append-after-promotion warning for the second | ||
| /// platform of a single `--platforms ios,android` invocation, where the | ||
| /// "already on stable" state was created moments earlier by this same run. | ||
| final Set<int> _patchesPromotedThisRun = {}; |
There was a problem hiding this comment.
bdero's review bot:
🧹 The name reads as if it tracks global state — it actually tracks "promotions made by this wrapper instance," which has command-scoped lifetime via the scoped ref. A field comment noting the scope (and that cross-invocation reuse correctly prompts because the set starts empty) would save the next reader the trace.
| // RFC4122 v4: 8-4-4-4-12 hex with version nibble == 4. Asserted | ||
| // as a single raw string to keep the regex semantics obvious; | ||
| // splitting it earned a "use raw string" / "adjacent strings" | ||
| // ping-pong from the linter. |
There was a problem hiding this comment.
bdero's review bot:
🧹 The "linter ping-pong" comment is more interesting than the regex it explains. Either trust the raw string and drop the comment, or hoist the regex to a top-level final RegExp _v4 = RegExp(r'...'); with a one-line "RFC 4122 v4" note — both quieter than the current four-line explanation of why-the-lint-fought-us.
| /// Returns null only for single-platform invocations with no `--patch-id`, | ||
| /// preserving the legacy behavior where the server allocates a fresh patch | ||
| /// number per call. | ||
| late final String? clientPatchId = _resolveClientPatchId(); |
There was a problem hiding this comment.
bdero's review bot:
🧹 late final String? clientPatchId = _resolveClientPatchId(); works, but _resolveClientPatchId() reads results[...] and calls Uuid().v4(), which means the order of first access decides which Uuid instance is materialized. Tests have to go through runWithOverrides for argResults to be wired before the first access — they do — but a getter (or a method returning a memoized value) reads more obviously than late final-with-side-effect.
|
Overall makes sense & approach seems good to me. Bot has some good looking suggestions. My human comment keeps getting dropped when submitting these for some reason 🤷 |
- Reject `--patch-id=` (present but empty) outright in run(). Typical cause is an unexpanded CI template variable; silently coalescing to null would mean a multi-platform invocation generates a fresh UUID per bot and iOS/Android never converge. - Consolidate empty-string normalization at the client boundary; remove the redundant `isNotEmpty` check from `_resolveClientPatchId` so the single normalizer lives in `CodePushClient.createPatch`. - Restore the `!` on the aggregated success log to match the celebratory tone of other `logger.success` lines. - Rename the doc comment on `_patchesPromotedThisRun` to make its command-scoped lifetime explicit and note that cross-invocation reuse still prompts (the set starts empty per command). - Extract the RFC 4122 v4 UUID regex into a top-level constant and drop the linter-ping-pong commentary that obscured it. - Add a `late final` doc note explaining the resolve-on-first-read contract and the `argResults`-must-be-wired prerequisite.
Address review feedback on PR #3738 (round-trip optimization). The CLI's append-after-promotion prompt previously meant two server round-trips per `publishPatch`: createPatch, then getPatches to check whether the returned patch was already on stable. Every CI run with a unique SHA — the dominant flow — paid that cost even though every createPatch result is a freshly inserted row with no channel yet. Add `channel` to `CreatePatchResponse` so the server can report the patch's current promotion state inline. The CLI then: - Skips the getPatches lookup entirely when channel is null (fresh insert) or non-stable. - Only prompts when `patch.channel == "stable"`, which can only happen on an idempotent hit (server only populates the field when there's something to report). ## Backwards compatibility Older servers omit the field; `channel` deserializes to null and the prompt simply doesn't fire — append-after-promotion stays allowed per the design. Older clients that don't read the field ignore it on deserialization. The server-side wiring (and the openapi.yaml schema update) live in the parent repo alongside `PostgresDatabase.createPatch`.
`dart format` wraps the regex literal across multiple lines and drops the `lines_longer_than_80_chars` ignore comment along with it.
2 participants
Summary
Make
POST /v1/apps/{appId}/patchesidempotent on a user-supplied correlation key so cross-platform builds resolve to one patch row instead of one per platform.Customer outcome: a developer who shipped "patch 3" sees the same number on iOS and Android — in their analytics, in-app, and in their release notes. The dominant CI flow (iOS bot publishes, Android bot publishes minutes later, no shared state between them) collapses to one patch as long as both bots pass the same
--patch-id.Protocol —
shorebird_code_push_protocolCreatePatchRequestgains optionalclientPatchId(wire fieldclient_patch_id).toJsonalways emits the key (null when unset) so server parsing doesn't have to special-case absence.CreatePatchResponse(new field) echoesclientPatchIdback so callers can verify whether an idempotent re-use occurred.ReleasePatchcarriesclientPatchIdso existing GET endpoints surface the correlation key for admin/console rendering.Client —
shorebird_code_push_clientCodePushClient.createPatchaccepts and threadsclientPatchId, and now returnsCreatePatchResponseinstead ofPatchso callers can read the echoed key.clientPatchIdis normalized to null at the client boundary — a caller that passes an unexpanded template variable or empty flag shouldn't land on the idempotent path keyed on''and inherit a stranger's patch. Done at the client rather than the protocol so the protocol layer remains a faithful round-trip.CLI —
shorebird_cli--patch-id=<string>flag onshorebird patch. Common usage:--patch-id=${{ github.sha }}or any stable token (hotfix-login, a CI run ID, etc.).--platforms=ios,android) auto-generate a UUID and share it across every per-platform fan-out — single-invocation is just cross-invocation-with-an-internal-ID.--platforms ios,androidinvocation — the user already opted in by passing both platforms together).✅ Published Patch 3 (iOS, Android)instead of one log per platform; degenerate cases where platforms land on different numbers each get their own line.Backwards compatibility
client_patch_iddeserializes to null. Old clients that don't set the field fall through the existing per-call create path on the server.This is the protocol + CLI half of unified patch numbering. The server-side migration and idempotent
createPatchlive in the private parent repo and are gated behind the sameclient_patch_idcorrelation key.Test plan
dart test packages/shorebird_code_push_protocoldart test packages/shorebird_code_push_clientdart test packages/shorebird_clishorebird patch android --patch-id=test-shaagainst a server with the matching server-side change deployedshorebird patch --platforms=ios,androidend-to-end (single invocation; auto-generated UUID)shorebird patchcalls minutes apart with the same--patch-id