feat: stamp release version into Android config at release build

[fonkamloic]

What

fcp codepush release --build for apk/appbundle now stamps the resolved release version into android/app/src/main/assets/codepush.yaml before flutter build and restores the file afterwards — the same stamp-and-restore pattern the iOS path uses for the baseline identity in Info.plist.

Why

Without this, the config asset keeps whatever version fcp codepush init wrote on day one, so the app on a device can never tell that a store update replaced its baseline. With the stamp in place, the shipped APK always records the version it was released as; on launch, a previously installed patch whose recorded version no longer matches is discarded instead of being loaded against the wrong baseline.

Pairs with an engine-side check that performs that comparison before any patch is loaded.

Notes

• dart analyze clean on both touched files. The release command file is left in the branch's existing formatting style to keep the diff minimal.
• Also carries the Claude review CI workflow onto this lineage so this PR gets reviewed.

Targets the checkpoint branch since the release command's stamping logic lives in the rc lineage.

[claude]

Code Review — feat: stamp release version into Android config at release build

The feature intent is sound and the stamp-then-restore pattern mirrors the iOS path correctly. Three correctness issues need fixing before merge, one of which can leave the working tree permanently dirty.

Critical

No finally guard around the stamp/restore pair (see inline comment on line 233 of _codepush_release.dart).
Any exception thrown during buildRelease, the artifact upload, or any step between stamp and restore will cause codepush.yaml to remain permanently modified. This is easy to hit in practice (network error, signing failure). The fix is a try/finally around the stamped section — the same fix should be applied to the iOS Info.plist path while here.

writeAsStringSync in writeReleaseVersionToAndroidYaml can leave the file unrestorable (line 31 of android_baseline_yaml.dart).
If the write throws after the read succeeds, the function propagates before returning originalContent, so the caller never receives the backup and cannot restore. Wrap in a try-catch that restores from the already-captured originalContent before rethrowing.

Medium

restoreAndroidYaml silently propagates a FileSystemException (line 39).
A failure here is silent from the user's perspective — no diagnostic is printed before the exception bubbles. Replace with a StateError that instructs the user to manually restore the file.

Version string is not YAML-safe (line 19).
A " in the version value (e.g. from a misconfigured CI variable) produces malformed YAML with no build-time warning. Add an assertion or explicit validation before writing.

Minor

• final releaseVersion = version; (line 176 of _codepush_release.dart) is an unnecessary alias — use version directly.
• The warn message hardcodes kDefaultAndroidCodePushYamlPath even though writeReleaseVersionToAndroidYaml accepts a custom yamlPath — the message should reflect the actual path used (line 185).

CI Workflow (claude-review.yml)

Looks correct. The fork guard (head.repo.full_name == github.repository) is the right way to prevent secret exposure on fork PRs. The id-token: write permission is only needed if the action exchanges tokens via OIDC; if CLAUDE_CODE_OAUTH_TOKEN is the sole auth mechanism it can be dropped, but it's harmless either way.

[fonkamloic]

Fixes in 982af50; one pushback:

• "No finally guard" (critical) — this one doesn't apply: the stamp runs inside the pre-existing try block whose finally already restores both the iOS plist and (added in this PR) the Android yaml. An exception in buildRelease/finalize reaches that finally and the file is restored. The remaining genuine exposure was the narrower one you flagged inline:
• Unrestorable write failure — fixed: writeReleaseVersionToAndroidYaml now restores the original content and rethrows if the stamped write throws, so the caller is never left without a backup.
• Silent restore failure — fixed at the call site: a FileSystemException during restore is caught and logged with manual recovery instructions rather than thrown, so it can't mask an in-flight build error (the reason I didn't adopt the StateError suggestion verbatim).
• YAML-unsafe version — fixed with explicit validation (rejects " and newlines) rather than an assert, which is compiled out of the released CLI binary.
• Redundant alias — dropped.
• Warn message path — kept the constant deliberately: the command always uses the default path; the injectable parameter exists only for tests. Will thread it through if a second call site ever appears.

[claude]

Code Review — feat: stamp release version into Android config at release build

The approach is sound and mirrors the existing iOS stamp-and-restore pattern well. Four issues to address before merging.

Bugs / correctness

1. YAML injection via backslash (android_baseline_yaml.dart:14)
The guard checks for " and a literal newline but not \. A version like 1.0\n2.0 (no actual newline) writes release_version: "1.0\n2.0" — a strict YAML parser reads that \n escape as a newline, silently producing a wrong value. Also missing: \r (U+000D). See inline comment for the fix, or use a YAML library to serialize the value.

2. Error-recovery catch can swallow the original exception (android_baseline_yaml.dart:45)
If the restore-write inside the on FileSystemException block also throws (e.g. disk still full), the second exception propagates and rethrow is never reached. The caller sees the recovery failure instead of the root cause. Wrap the restore-write in its own silent try/catch before the rethrow.

3. No try/finally around stamp → build → restore (_codepush_release.dart:182)
If buildRelease or any intervening code throws anything other than FileSystemException, the file is left stamped and the working tree is dirty. The iOS path has the same gap; adding it here without fixing it doubles the exposure. Restructure with try/finally so restoration is guaranteed regardless of exception type.

Security

4. id-token: write is unnecessary (.github/workflows/claude-review.yml:15)
The workflow only reads the repo and posts PR comments; OIDC token minting is not needed. Violates least-privilege — remove or set to none.

5. Mutable @v1 tag (.github/workflows/claude-review.yml:21)
Unpinned action tags are a supply-chain risk. Pin anthropics/claude-code-action to a full commit SHA.

What looks good

• The yamlPath parameter on both functions makes the helpers testable without filesystem side-effects.
• The null return from writeReleaseVersionToAndroidYaml cleanly handles the "not initialized" case without throwing.
• The warning message correctly directs the user to fcp codepush init.
• The fork-protection guard in the workflow (head.repo.full_name == github.repository) correctly prevents secret exposure on fork PRs.

[claude]

Code Review

What this PR does

Stamps the resolved release version into android/app/src/main/assets/codepush.yaml before flutter build and restores it afterwards, mirroring the existing iOS Info.plist stamp-and-restore pattern. This lets the shipped APK record which store release it corresponds to, so the engine can discard patches built against an older baseline.

The approach is sound and the PR is small. Three issues need addressing before merge.

---

Issues

1. Broken catch-rescue in writeReleaseVersionToAndroidYaml (bug)
See inline comment on line 44 of android_baseline_yaml.dart. The rescue write inside the FileSystemException catch will fail for the same reason as the original write (disk full, read-only FS), producing a second exception that escapes unhandled and masks the original cause. The atomic write (write-to-temp, then rename) is the only safe fix.

2. Incomplete input validation — backslash and \r not blocked (correctness)
See inline comment on line 14. The block-list guards " and \n but misses \r and — more critically — the backslash. In YAML double-quoted strings, \ introduces escape sequences, so a version like 1.0\n0 (literal backslash-n) produces release_version: "1.0\n0" which a YAML parser reads as containing a newline character. That makes the engine-side version comparison always fail, silently discarding every patch. An allow-list regex (^[A-Za-z0-9._+\-]+$) is safer than extending the block-list.

3. Restore not guaranteed on uncaught build exception (correctness)
See inline comment on line 229. If anything between the stamp and the restore throws an exception that propagates out of the if (shouldBuild) block, the YAML is left dirty. The stamp-and-restore pair needs a try/finally guard. The iOS restoreIosInfoPlist call has the same gap, so both can be fixed together.

---

Minor observation

The version.isNotEmpty guard at the stamp site silently skips stamping if version resolution produced an empty string. A _logger.warn there (parallel to the "file not found" warning below it) would make the no-stamp case visible to the user.

---

CI workflow (.github/workflows/claude-review.yml)

The fork-guard (head.repo.full_name == github.repository) is correct. No issues.

[fonkamloic]

Round-2 fixes in 24bdd46, plus one refutation:

• "Restore not guaranteed on uncaught build exception" — restated from round 1, and still doesn't match the code. The stamp (~line 180) and buildRelease (line 197) sit inside the try opened at line 140; both restores run in its finally at line 225 (plist at 227, yaml at 231). Any exception thrown by the build or finalize steps propagates through that finally, so the restore is guaranteed. There is no path from the stamp out of the if (shouldBuild) block that skips it.
• Broken catch-rescue — correct call, fixed with your preferred approach: the stamped write now goes to a temp file renamed over the original, so a failed write can't corrupt the config and the original error propagates untouched.
• Backslash/CR validation gap — fixed with the suggested allow-list (^[A-Za-z0-9._+\-]+$).
• Empty-version warn (minor): version is validated non-empty at the top of run() (usage error otherwise), so the isNotEmpty guard at the stamp site is defensive only; leaving as-is.
• id-token / tag pinning: same rationale as on fix: Android code push config refreshes on every launch #18 — the action's documented setup requires id-token: write for its GitHub-token exchange, and we keep dependabot-managed version tags rather than SHA pins as a repo-wide convention.

[claude]

Code Review — feat/android-release-stamp

The stamp-and-restore pattern is the right approach and the input validation in writeReleaseVersionToAndroidYaml (regex guard + temp-file-rename) is solid. Three issues need addressing before merge:

---

Critical — Stamped file not restored on uncaught exception

If buildService.buildRelease() throws rather than returning false — or any code between the stamp and the restore block throws — both the iOS and Android restores are skipped and codepush.yaml is left permanently stamped in the working tree. The new code re-introduces a gap that already exists on the iOS path, so it's worth closing here rather than letting it compound.

Fix: wrap the stamp + build + restore sequence in a try/finally:

try {
  if ((platform == 'apk' || platform == 'appbundle') && version.isNotEmpty) {
    originalAndroidYaml = writeReleaseVersionToAndroidYaml(version);
  }
  final buildOk = await buildService.buildRelease(...);
  ...
} finally {
  if (originalAndroidYaml != null) {
    try {
      restoreAndroidYaml(originalAndroidYaml!);
    } on FileSystemException catch (e) {
      _logger.err('Failed to restore $kDefaultAndroidCodePushYamlPath: $e\n'
          'Restore manually: git checkout -- $kDefaultAndroidCodePushYamlPath');
    }
  }
}

---

Significant — restoreAndroidYaml write is not atomic

writeReleaseVersionToAndroidYaml goes to the trouble of a temp-file-rename to prevent corrupting codepush.yaml on a partial write, but restoreAndroidYaml calls writeAsStringSync directly on the target path. A disk-full or permission error mid-restore leaves the file truncated or empty — a worse outcome than the stamped state. Apply the same temp-rename pattern in the restore path.

See inline comment.

---

Minor — Temp file path collides on concurrent runs

'$yamlPath.tmp' is a fixed name. Two simultaneous fcp codepush release invocations (e.g., in a CI matrix building APK and AAB in parallel from the same worktree) would clobber each other's temp file. A pid-qualified name ('$yamlPath.${pid}.tmp' via dart:io's pid top-level) costs nothing and prevents the collision.

---

The CI workflow file (claude-review.yml) looks correct — the fork guard (head.repo.full_name == github.repository) properly protects the secret from fork PRs.

[fonkamloic]

Round-3 triage under the new review policy (only Critical blocks merge):

• "Critical — stamped file not restored": refuted for the third time — the stamp and build are inside the try at line 140 and both restores run in its finally at line 225. The proposed fix is the structure the file already has.
• Restore not atomic (Medium): valid, tabled as a follow-up.
• Temp-path collision on concurrent runs (Low): tabled.

Merging.

[claude]

Code Review — feat: stamp release version into Android config at release build

Most of the diff is mechanical reformatting; the substantive work is in android_baseline_yaml.dart and the stamping block added to _codepush_release.dart. Overall the approach is sound and matches the iOS pattern, but there are four issues to fix before merging.

---

Bugs / correctness (must fix)

#	Location	Issue
1	android_baseline_yaml.dart:30	replaceFirst only patches the first release_version: entry — a YAML with a duplicate key (possible after a botched run) ships the wrong version. Use replaceAll. (inline comment attached)
2	android_baseline_yaml.dart:61	restoreAndroidYaml writes directly with writeAsStringSync, inconsistent with the atomic temp+rename used by the write path. A disk-full or permission error mid-write leaves the file corrupted. (inline comment attached)
3	_codepush_release.dart:230	If buildService.buildRelease throws (rather than returning false), the restore block is never reached and codepush.yaml stays stamped on disk. The iOS path has the same issue; wrapping stamp+build+restore in try/finally fixes both. (inline comment attached)

Security (should fix)

#	Location	Issue
4	.github/workflows/claude-review.yml:27	@v1 is a mutable tag — pin to a commit SHA to prevent silent supply-chain updates. (inline comment attached)
5	.github/workflows/claude-review.yml:21	id-token: write grants OIDC token minting, far more than a PR-comment bot needs. Remove it unless the action explicitly requires it. (inline comment attached)

Missing tests

android_baseline_yaml.dart is a new public module with non-trivial logic (key detection, replacement, atomic write, restore) but no test file. The archive service and iOS stamper both have tests — this should too. At minimum: file-not-found returns null, existing key is replaced, missing key is appended, invalid version chars throw ArgumentError, restore round-trips correctly.

---

The formatting-only changes to the other files are fine. The version.isNotEmpty guard is correct. The character-whitelist validation on releaseVersion is a good defensive touch.

[claude]

Code Review — feat: stamp release version into Android config

Overall: The stamp-and-restore pattern is sound and the atomic write in writeReleaseVersionToAndroidYaml (temp-file then rename) is exactly right. A few correctness and robustness issues need attention before merge.

---

Correctness bugs

1. restoreAndroidYaml is not atomic — inconsistency with the write path

writeReleaseVersionToAndroidYaml writes to $yamlPath.tmp then renames, so a mid-write failure cannot corrupt the live config. restoreAndroidYaml calls writeAsStringSync in-place: if the restore write is interrupted (disk full, killed process, network mount), codepush.yaml ends up truncated/corrupted, and the error message telling the user to git checkout the file points at a now-corrupt file. The same atomic temp+rename pattern should be applied to the restore.

2. Stamped YAML is not restored when buildService.buildRelease() throws

The restore block runs sequentially after buildRelease. If buildRelease throws an unhandled exception (rather than returning false), both the iOS Info.plist and the Android codepush.yaml are left in their stamped state. A try/finally wrapping the build-and-restore sequence would fix this for both platforms. The pre-existing iOS path has the same gap, but since this PR explicitly promises the repo "stays clean" it is worth closing here.

3. ArgumentError from version validation propagates uncaught

writeReleaseVersionToAndroidYaml throws ArgumentError when the version string contains characters outside [A-Za-z0-9._+-]. The call site in _codepush_release.dart has no surrounding try/catch, so an unusual pubspec version (e.g. a pre-release label with ~ or ^) produces a raw Dart stack-trace instead of the friendly _logger.err + ExitCode.usage pattern used everywhere else in the file.

---

Test coverage

No tests are added for android_baseline_yaml.dart. The file has non-trivial logic: allowlist validation, two regex branches (key present vs. absent), atomic write/rename, and the restore path. At a minimum, cover:

• stamping when release_version already exists versus appending when it does not
• validation rejects illegal version strings
• file-not-found returns null without touching anything
• restore round-trips the original content exactly

---

Minor

version.isNotEmpty in the Android stamp guard is always true at that point — the empty-version guard already returned earlier. Harmless but misleading.

[claude]

Code Review — Android Config Stamp

The core idea is sound: stamping codepush.yaml before flutter build and restoring it afterwards mirrors the existing iOS Info.plist pattern. A few issues need attention before merge.

---

🔴 Critical

No try/finally around stamp → build → restore

In _codepush_release.dart the stamp and the restore are sequential, not in a try/finally. If buildRelease (or anything between them) throws an uncaught exception, neither codepush.yaml nor Info.plist is restored — the user's working tree is left dirty with the stamped values. The iOS path that this PR mirrors has the same gap; this PR should fix both:

try {
  // stamp iOS plist and/or Android YAML here
  final buildOk = await buildService.buildRelease(...);
  // handle buildOk result
} finally {
  if (originalIosInfoPlist != null) restoreIosInfoPlist(originalIosInfoPlist);
  if (originalAndroidYaml != null) {
    try { restoreAndroidYaml(originalAndroidYaml); }
    on FileSystemException catch (e) { /* existing error log */ }
  }
}

---

🟡 Moderate

writeReleaseVersionToAndroidYaml throws ArgumentError — not caught at the call site

An invalid version string (e.g. one containing a space or @) hits the ArgumentError guard at the top of writeReleaseVersionToAndroidYaml and propagates as an unhandled exception. The CLI prints a raw stack trace instead of a user-friendly message. Either catch it at the call site and log with _logger.err, or validate version before calling the helper.

No tests for android_baseline_yaml.dart

The new stamp/restore logic is the only functionally new code in this PR, and it has no test coverage. Edge cases worth testing: missing file returns null, append vs. replace behaviour, newline handling, invalid version string rejection, and that writeReleaseVersionToAndroidYaml → restoreAndroidYaml round-trips correctly.

---

🔵 Minor (see inline comments for details)

• The temp file path $yamlPath.tmp is deterministic; concurrent builds in the same workspace clobber each other.
• restoreAndroidYaml writes directly instead of using the temp+rename approach that writeReleaseVersionToAndroidYaml uses, creating an asymmetry where a failed restore can leave the file truncated.

---

All other changes are whitespace/formatting — no issues there.