Skip to content

Use public Sentry org/project, env-injected token - #5708

Open
mokagio wants to merge 8 commits into
mainfrom
ainfra-2790-remove-sentryproperties-in-favor-of-public-values-and-env
Open

Use public Sentry org/project, env-injected token#5708
mokagio wants to merge 8 commits into
mainfrom
ainfra-2790-remove-sentryproperties-in-favor-of-public-values-and-env

Conversation

@mokagio

@mokagio mokagio commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

See https://linear.app/a8c/issue/AINFRA-2790. Also, wordpress-mobile/WordPress-Android#23189 for a similar implementation.

  • Inlines the Sentry organization and project names, which do not need to be secret (and cannot be anyway because they can be accessed in the published binary), instead of reading from encrypted sentry.properties.
  • Leaves the DSN as a secret, however, not because it is secret, but just to avoid open source contributors accidentally spamming our production logs
  • Moves reading the API token from .properties to env var, so that it can be kept off any machine that doesn't need it (i.e., ideally CI only)

Notice the pinned mobile-secrets checkout has not changed, that's because the updates here only stop reading properties from secrets.properties, they don't need new ones. So, I thought I'd keep the diff smaller.

AI-generated details

Rationale

sentryOrg, the per-variant sentry*Project slugs, and sentryAuthToken aren't secrets: the org/project slugs are Sentry-public identifiers, and the auth token is only needed by CI to upload ProGuard mappings. Moving them out of secret.properties shrinks what mobile-secrets needs to hold for this repo, matching the pattern already shipped in simplenote-android#1850 and in review in dayone-android.

Intentional tradeoff

Unlike those sibling repos, the Sentry DSN stays in secret.properties unchanged. Pocket Casts Android is open source and hasn't otherwise adopted a8c-secrets yet, so per the paaHJt-ahO-p2 RFC the DSN can't become a public repo constant the way it did there.

Gotchas

  • This assumes SENTRY_AUTH_TOKEN is already available as an env var on this repo's Buildkite agents (it needed no pipeline changes for dayone-android). Watching the release-build CI job's mapping upload step to confirm.
  • Depends on a follow-up mobile-secrets change (not included here, applied separately): delete sentryOrg, sentryAuthToken, sentryAndroidProject, sentryAutomotiveProject, sentryWearProject, sentryTvProject from the pocket-casts-android secret.properties entry. The DSN keys stay.

How to test

./gradlew help (or any task) evaluates all Gradle scripts cleanly; ./gradlew spotlessCheck passes.

@mokagio
mokagio requested a review from a team as a code owner August 7, 2026 04:21
@mokagio
mokagio requested review from Copilot and sztomek and removed request for a team August 7, 2026 04:21
@mokagio mokagio self-assigned this Aug 7, 2026
@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown

Claude finished @mokagio's task in 3m 48s —— View job


Code review

  • Read the diff and gather context
  • Check Sentry config wiring (applyCommonSentryConfiguration, per-module sentry {} blocks)
  • Verify nothing else referenced the removed properties (incl. sentryTvProject)
  • Sanity-check the new fail-fast task hook & configuration cache behaviour
  • Post review

The direction is sound and the change is well scoped. Two inline comments on build.gradle.kts; everything else below is verification notes and context, not blockers.


Findings

1. A blank SENTRY_AUTH_TOKEN slips past the new guardinline · build.gradle.kts:500

providers.environmentVariable(...).isPresent is true when the variable is set to the empty string, which is exactly what a failed secret-injection step produces. The guard passes, authToken = "" reaches the plugin, and the build fails deep in the upload with a Sentry auth error rather than your message. The removed code used isNullOrBlank() for this reason — worth preserving it in both places. Fix this →

2. The guard doesn't cover the source-bundle uploadinline · build.gradle.kts:498

includeSourceContext is driven by the same shouldUploadDebugFiles flag and its task also needs the token, but sentryUploadSourceBundle* doesn't match the uploadSentryProguardMappings prefix. Since task ordering isn't guaranteed, a token-less CI run could surface the plugin's opaque error first. Same comment also notes that prefix-matching on a plugin-internal task name will silently stop matching on a future plugin bump.

3. Confirm the hardcoded slugs against what's in mobile-secrets today (pre-merge check, not a code issue)

org = "a8c" plus pocket-casts-android / pocket-casts-automotive / pocket-casts-wear are now compile-time constants, and a typo only surfaces as a 404 at the mapping-upload step of an actual release build — the slowest possible feedback loop. Worth diffing them character-for-character against the current sentryOrg / sentry*Project values before the follow-up mobile-secrets deletion lands, since after that the old values are gone and you'd be reconstructing them from Sentry's UI.


Verified

  • sentryTvProject removal is safe. tv/build.gradle.kts doesn't apply libs.plugins.sentry, and applyCommonSentryConfiguration() only runs under plugins.withId(sentry) (build.gradle.kts:227), so that property was already dead. No sentry {} block exists in the tv module to update.
  • No other references remain. Grepped the whole tree: nothing outside the deleted lines reads sentryOrg, sentryAuthToken, or any sentry*Project. No docs, gradle.properties defaults, Fastlane lanes, or .buildkite/ scripts mention them.
  • sentryAndroidDsn / sentryAutomotiveDsn / sentryWearDsn / sentryTvDsn are untouched in dependencies.gradle.kts:109-116, matching the stated tradeoff.
  • Local release builds are unaffected. includeProguardMapping is gated on CI, so no upload task is registered off-CI and the new GradleException can't fire on a developer machine. Losing the old println warning costs nothing there, since the project name is no longer configurable.
  • doFirst is the right place for this check, not configuration time. A configuration-time failure would break every CI job that runs with CI=true without -PskipSentryProguardMappingUpload (unit tests, lint) even though they never upload anything. Deferring to task execution is slower to fail on a release build but correct.
  • -PskipSentryProguardMappingUpload=true is already passed by .buildkite/commands/assemble-release-apk.sh:19 and the prototype lane (fastlane/Fastfile:647), so the escape hatch named in the error message is real and in use.
  • Configuration cache (org.gradle.configuration-cache=true) is fine. Reading the token through providers.environmentVariable registers it as a tracked build input, so rotating the token invalidates the entry correctly — strictly better than a raw System.getenv() read. The Provider captured in the doFirst lambda is config-cache serializable.

On the stated gotcha

Nothing in this repo exports SENTRY_AUTH_TOKEN.buildkite/release-builds.yml sets no env: block, and .buildkite/commands/release-build.sh only runs configure_apply for mobile-secrets. So the assumption that it's a global agent env var is load-bearing and untestable from this diff. Note the failure mode changed shape: it used to be a println warning and a release that shipped without mappings; now it's a hard failure late in build_and_upload_to_play_store, after the bundle is built. That's the better outcome, but if the env var isn't there it will block a release rather than degrade it — worth having the skip flag in your back pocket for the first run.

Security / performance

No concerns. Moving the token from an on-disk secret.properties to an env var reduces exposure, and the DSN staying put is the right call for an open-source repo. One thing to be aware of rather than act on: because authToken is resolved at configuration time, the token value gets serialized into .gradle/configuration-cache/ on the agent — relevant only if that directory is ever cached or archived across jobs. No performance impact; the guard adds one doFirst to tasks that only exist on CI.


Review only — no code changes made or pushed.
· Branch: ainfra-2790-remove-sentryproperties-in-favor-of-public-values-and-env

@mokagio mokagio added [Area] Gradle [Type] Tooling Related to the Gradle build scripts and the setup or maintenance of the project build process. labels Aug 7, 2026
@mokagio mokagio added this to the 8.19 milestone Aug 7, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes Sentry Gradle configuration less dependent on secret.properties by hardcoding the public Sentry org/project slugs in build scripts and sourcing the Sentry auth token from an environment variable (for CI-only ProGuard mapping uploads).

Changes:

  • Hardcode Sentry projectName for app, automotive, and wear modules to public slugs.
  • Stop exporting Sentry org/project/token values from secret.properties via dependencies.gradle.kts.
  • Update common Sentry Gradle configuration to read SENTRY_AUTH_TOKEN from the environment and fail the mapping upload task when the token is missing.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
build.gradle.kts Moves Sentry auth token to env-var provider, hardcodes org, and adds a guard to fail upload tasks when token is missing.
dependencies.gradle.kts Removes Sentry org/project/token keys from the secret.properties-backed Gradle properties export.
app/build.gradle.kts Hardcodes Sentry project slug for the main Android app module.
automotive/build.gradle.kts Hardcodes Sentry project slug for the Automotive module.
wear/build.gradle.kts Hardcodes Sentry project slug for the Wear module.

Comment thread build.gradle.kts Outdated
// mapping upload is expected — fail there rather than ship a release we can't deobfuscate.
tasks.matching { it.name.startsWith("uploadSentryProguardMappings") }.configureEach {
doFirst {
if (!sentryAuthToken.isPresent) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provider.isPresent is true for an env var that is set but empty. That's the most likely way this fails in practice — a secret-injection step that resolves to nothing exports SENTRY_AUTH_TOKEN="" rather than leaving it unset. In that case the guard passes, authToken = "" reaches the plugin, and you get an opaque Sentry auth error mid-upload instead of this message.

The code being replaced deliberately used isNullOrBlank() for exactly this reason. Suggest keeping that semantics:

Suggested change
if (!sentryAuthToken.isPresent) {
if (sentryAuthToken.orNull.isNullOrBlank()) {

Worth mirroring on line 480 too, so a blank value never reaches the extension:

authToken = sentryAuthToken.orNull?.takeIf { it.isNotBlank() }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, adopted in 9d74335 — with one adjustment.

Rather than blank-checking at both sites, I normalise once at the source so the bound value and the guard can't drift:

val sentryAuthToken = providers.environmentVariable("SENTRY_AUTH_TOKEN").orNull?.takeIf { it.isNotBlank() }

authToken = sentryAuthToken and if (sentryAuthToken == null) then read the same thing by construction.

One correction to the premise: the code being replaced here didn't use isNullOrBlank() for the token — it was project.findProperty("sentryAuthToken")?.toString(), unchecked. The isNullOrBlank() you're thinking of was in app/build.gradle.kts, guarding a warning println about sentryAndroidProject, and this PR removes it. Doesn't change the conclusion — an empty env var reading as "" rather than absent is exactly the failure mode worth defending against.

Posted by Claude Code (Opus 5) on behalf of @mokagio with approval.

Comment thread build.gradle.kts Outdated

// The upload task only exists when includeProguardMapping is on, so this fires exactly when a
// mapping upload is expected — fail there rather than ship a release we can't deobfuscate.
tasks.matching { it.name.startsWith("uploadSentryProguardMappings") }.configureEach {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small things about the predicate:

  1. includeSourceContext is toggled by the same flag but its upload task (sentryUploadSourceBundle*) isn't matched here. It needs the auth token just as much, and task ordering isn't guaranteed, so a token-less CI run may well fail on the source bundle first — with the plugin's error rather than this one. Broadening the match keeps the friendly message authoritative:

    tasks.matching {
        it.name.startsWith("uploadSentryProguardMappings") || it.name.startsWith("sentryUploadSourceBundle")
    }.configureEach { ... }
  2. Matching on a name prefix couples the guard to a plugin-internal naming convention. If a future io.sentry.android.gradle bump renames the task, this silently stops matching and the guard quietly disappears — the exact failure it exists to prevent. tasks.withType<SentryUploadProguardMappingsTask>() would break the build at compile time instead. Your call on whether the extra import is worth it; if you keep the string match, a comment pointing at the plugin version it was verified against (6.16.0) would help the next person doing the upgrade.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both points valid — fixed in 9d74335 by going one level up the type hierarchy instead of broadening the string match:

tasks.withType<SentryCliExecTask>().configureEach { ... }

Checking the 6.16.0 plugin jar: SentryCliExecTask is the public base of every task that shells out to sentry-cli with the auth token — SentryUploadProguardMappingsTask, UploadSourceBundleTask, and the native-symbols/app-artifact tasks. So one type covers both cases you raised, and a future rename is a compile error rather than a silently vanishing guard.

Your first point checks out: sentryUploadSourceBundle* is registered by UploadSourceBundleTask and gated by the same includeSourceContext flag, so the old prefix genuinely missed it.

Two consequences worth noting:

  • The message is now "upload debug files to Sentry" rather than "upload ProGuard mappings", since the net is wider. The -PskipSentryProguardMappingUpload escape hatch stays accurate — in this repo that one flag gates both includeProguardMapping and includeSourceContext.
  • The extra tasks this now catches are inert here: nothing sets uploadNativeSymbols or app-artifact upload, so those tasks are never registered.

Note that CI cannot exercise this guard: every PR-triggered build passes -PskipSentryProguardMappingUpload=true (assemble-release-apk.sh, and the prototype lane), so the upload tasks are never registered and doFirst never runs. The first real exercise is build_bundle on a beta/release build, which passes no skip property. What CI does confirm is that the build scripts compile, so the typed match resolves against the plugin.

Posted by Claude Code (Opus 5) on behalf of @mokagio with approval.

@wpmobilebot

Copy link
Copy Markdown
Collaborator

App Icon📲 You can test the changes from this Pull Request in 📱 Mobile by scanning the QR code below to install the corresponding build.

App Name📱 Mobile
Build TypePrototype
Build Number9445
Version8.18-rc-1
Application IDau.com.shiftyjelly.pocketcasts
Commite6a053a
Installation URL66449fg0usvn8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

Copy link
Copy Markdown
Collaborator
App Icon📲 You can test the changes from this Pull Request in ⌚ Wear by scanning the QR code below to install the corresponding build.
App Name⌚ Wear
Build TypeDebugProd
Build Number9445
Version8.18-rc-1
Application IDau.com.shiftyjelly.pocketcasts
Commite6a053a
Direct Downloadpocketcasts-wear-prototype-build-pr5708-e6a053a.apk
Installation URL66449fg0usvn8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.
App Icon📲 You can test the changes from this Pull Request in 🚗 Automotive by scanning the QR code below to install the corresponding build.
App Name🚗 Automotive
Build TypeDebugProd
Build Number9445
Version8.18-rc-1
Application IDau.com.shiftyjelly.pocketcasts
Commite6a053a
Direct Downloadpocketcasts-automotive-prototype-build-pr5708-e6a053a.apk
Installation URL66449fg0usvn8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

mokagio added a commit that referenced this pull request Aug 11, 2026
A secret-injection step that resolves to nothing exports an empty
`SENTRY_AUTH_TOKEN` rather than leaving it unset, and `Provider.isPresent` is
`true` for that.
The guard passed, a blank token reached the plugin, and sentry-cli failed
mid-upload with an opaque auth error instead of the actionable message.
Normalising once at the source keeps the bound `authToken` and the guard from
disagreeing.

`includeSourceContext` rides the same flag as `includeProguardMapping` here, so
`sentryUploadSourceBundle*` needs the token just as much, but the old
`uploadSentryProguardMappings` name prefix never matched it — and task ordering
isn't guaranteed, so a token-less CI run could fail there first.
`SentryCliExecTask` is the base of every task that shells out to sentry-cli with
the token, so matching on the type covers both and fails at compile time if a
future plugin bump renames a task.

This diverges from the name-prefix match in the sibling wpandroid and
dayone-android changes.
dayone-android never enables source context, so it isn't exposed; wpandroid does
and still is.

Both points came from review feedback on #5708.

---

Generated with the help of Claude Code, https://claude.ai/code

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Claude finished @mokagio's task in 3m 22s —— View job


Follow-up review — commit 9d74335

  • Diff the new commit against the previously reviewed state
  • Verify both prior findings are addressed
  • Check the new SentryCliExecTask guard for correctness and over-reach
  • Re-check config-cache behaviour and style gates
  • Post review

Both findings from the earlier review are resolved, and the fixes are better than what I suggested. Nothing blocking below — one assumption worth a cheap one-time confirmation, one drive-by nit, and the pre-merge slug check still stands.


Resolved

1. Blank token now treated as missing ✅ — build.gradle.kts:481

val sentryAuthToken = providers.environmentVariable("SENTRY_AUTH_TOKEN").orNull?.takeIf { it.isNotBlank() }

Resolving once and normalising blank → null at the top is cleaner than my suggestion of patching the two sites separately: authToken and the guard now can't disagree by construction, and the comment above it records why blank ≠ unset, which is the part a future reader would otherwise delete.

2. Guard now covers every sentry-cli task, by type ✅ — build.gradle.kts:504

tasks.withType<SentryCliExecTask>() is the stronger version of both halves of my second comment: it picks up sentryUploadSourceBundle* alongside the ProGuard mappings, and a plugin bump that renames or removes the class fails at script-compile time instead of silently un-installing the guard. withType(...).configureEach stays lazy, so no task realisation cost.


Worth confirming (not a blocker)

The comment's "those tasks only exist when the upload flags are on" is an assumption about plugin internals. It's very likely right, and the downside is bounded — if a SentryCliExecTask subtype is registered but disabled through onlyIf, Gradle skips its actions and the doFirst never fires, so only a task that is both registered and scheduled without needing a token could misfire. Still, the failure mode would be a hard GradleException on a CI job that never intended to upload anything, so it's worth buying certainty once:

CI=true ./gradlew :app:assembleRelease --dry-run

That lists exactly which sentryUpload* / sentryCli* tasks get scheduled under the real upload configuration. Worth pairing with the first real release build you're already watching. (I couldn't run it here — Gradle execution and dependency resolution are both unavailable in this sandbox, so I also can't independently confirm io.sentry.android.gradle.tasks.SentryCliExecTask resolves on the buildscript classpath for plugin 6.16.0; the Buildkite build on this commit is the real check for that.)

Nit — CI is still read the coarse way. build.gradle.kts:487 uses System.getenv()["CI"], i.e. a read of the whole environment map, two lines below the new providers.environmentVariable call. Pre-existing and out of scope, but under configuration cache a whole-map read is tracked far more broadly than a single-variable read, and the file now has both idioms side by side. providers.environmentVariable("CI").orNull.toBoolean() would make them consistent and narrow the tracked input. Entirely optional. Fix this →

Still open from the last round: diff the hardcoded slugs against mobile-secrets before the deletion lands. org = "a8c" and pocket-casts-android / pocket-casts-automotive / pocket-casts-wear are compile-time constants now, and a typo only surfaces as a 404 at the upload step of a real release build. Once the follow-up mobile-secrets change removes the old values you'd be reconstructing them from Sentry's UI, so it's cheapest to check character-for-character first.


Re-verified on this commit

  • No new style violations. No line in any of the five changed files exceeds 120 chars (the only two over-length lines in build.gradle.kts are pre-existing, at :287 and :289). I can't run ./gradlew spotlessCheck here, so that's a manual check, not the gate itself.
  • No stragglers. Grepping the tree for sentryOrg, sentryAuthToken and sentry*Project now returns only the new SENTRY_AUTH_TOKEN lines in build.gradle.kts — no docs, gradle.properties defaults, Fastlane lanes or .buildkite/ scripts reference the removed keys.
  • DSN keys untouched, as the description states — dependencies.gradle.kts:108-115 still resolves sentryAndroidDsn / sentryAutomotiveDsn / sentryWearDsn / sentryTvDsn from secret.properties.
  • The escape hatch named in the error message is real and in use.buildkite/commands/assemble-release-apk.sh:19 and fastlane/Fastfile:647 both pass -PskipSentryProguardMappingUpload=true, so the widened guard can't break the prototype or release-APK jobs.
  • doFirst still the right place. The captured value is a plain String?, the lambda references nothing from the script instance, and the check stays out of configuration time — so CI jobs that run with CI=true but never upload (unit tests, lint) are unaffected.
  • Config-cache input tracking is correct. Reading through providers.environmentVariable registers the token as a tracked build input, so rotating it invalidates the entry. The caveat from last time is unchanged: the resolved token is serialised into .gradle/configuration-cache/ on the agent, which only matters if that directory is ever archived or shared across jobs.

Review only — no code changes made or pushed.
· Branch: ainfra-2790-remove-sentryproperties-in-favor-of-public-values-and-env

Comment thread build.gradle.kts Outdated
Comment thread build.gradle.kts Outdated
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Claude finished @mokagio's task in 1m 18s —— View job


Follow-up review — commit eaac1b3

  • Diff the new commit against the previously reviewed state
  • Verify the Sentry guard still behaves as reviewed
  • Re-check style gates and stragglers
  • Post review

eaac1b3 is comments-only. It applies the two empty-suggestion blocks from your own review, deleting the explanatory comment above sentryAuthToken (build.gradle.kts:478) and the one above the tasks.withType<SentryCliExecTask>() block (:497). Nothing else moved — git diff 9d74335..HEAD touches no code, so everything verified on 9d74335 still holds. No new findings.


Verified on this commit

  • Behaviour is byte-identical to the reviewed state. The diff removes 8 comment lines and nothing else; both fixes from the previous round are intact at build.gradle.kts:478 (blank → null normalisation at the source) and :497 (typed match over SentryCliExecTask).
  • No stragglers. Grepping the tree for sentryOrg / sentryAuthToken / sentry*Project returns only the four new SENTRY_AUTH_TOKEN lines in build.gradle.kts.
  • No style violations. No line in the five changed files exceeds 120 chars — the only two over-length lines in build.gradle.kts are the pre-existing buildConfigField calls at :287 and :289. (Gradle can't run in this sandbox, so that's a manual check, not spotlessCheck itself.)

One trade-off note (not a blocker)

With the comments gone, ?.takeIf { it.isNotBlank() } on line 478 is the only surviving trace of why blank is treated as unset. It's a deliberate defence against a secret-injection step that exports SENTRY_AUTH_TOKEN="", and it now reads as removable noise — the plausible regression is a future cleanup shortening it to .orNull, which silently restores the opaque mid-upload failure. Your call entirely; a five-word trailing comment (// blank env var == unset) would pin it without the paragraph you just deleted. Not worth another round on its own.

Still open from earlier rounds (unchanged by this commit)

  • Diff the hardcoded slugs against mobile-secrets before the deletion lands. org = "a8c" and pocket-casts-android / -automotive / -wear are compile-time constants now; a typo only surfaces as a 404 at the upload step of a real release build, and after the follow-up deletion the old values are gone.
  • CI=true ./gradlew :app:assembleRelease --dry-run would confirm which SentryCliExecTask subtypes actually get scheduled under the real upload configuration, closing out the "those tasks only exist when the upload flags are on" assumption.
  • SENTRY_AUTH_TOKEN availability on the agents remains load-bearing and untestable from this diff — no PR-triggered job exercises the guard, since assemble-release-apk.sh:19 and fastlane/Fastfile:647 both pass -PskipSentryProguardMappingUpload=true. First real exercise is build_bundle on a beta/release build.

Your correction on the earlier round stands, for the record: the isNullOrBlank() I referred to was in app/build.gradle.kts guarding the println about sentryAndroidProject, not the token read — the token was findProperty("sentryAuthToken")?.toString(), unchecked. The diff confirms it.


Review only — no code changes made or pushed.
· Branch: ainfra-2790-remove-sentryproperties-in-favor-of-public-values-and-env

@mokagio
mokagio marked this pull request as draft August 11, 2026 06:39
@mokagio
mokagio marked this pull request as ready for review August 13, 2026 05:32
@mokagio
mokagio requested a review from a team as a code owner August 13, 2026 05:32
@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown

Claude finished @mokagio's task in 3m 8s —— View job


Code review — ready-for-review pass (358eddc)

  • Read the full diff against the merge base
  • Verify the new .configure-files/sentry.properties.enc deletion is safe
  • Re-verify the Sentry guard, config-cache behaviour and style gates on the final commit
  • Confirm no stragglers reference the removed keys
  • Post review

The only change since the last round is 358eddc, which deletes an orphaned encrypted file. It checks out. No blocking findings — the two pre-merge checks from earlier rounds are still the only open items, and they're operational rather than code.


358eddc — the .enc deletion is safe

.configure decrypts exactly the entries in files_to_copy, and there is no entry with a sentry.properties destination — the manifest lists only secret.properties, the three google-services.json copies, google-upload-credentials.json, firebase.secrets.json and android.keystore. So configure_apply (called from all thirteen .buildkite/ scripts and Fastfile:424) never decrypted .configure-files/sentry.properties.enc; it was dead weight, and nothing regenerates it. Removing it after this PR is the right sequencing, since the values it held are exactly the ones this PR stops reading.

Two follow-on notes, neither needing action:

  • .gitignore:61 (sentry.properties) is worth keeping even though the encrypted file is gone. sentry-cli and the Gradle plugin both still read a project-root sentry.properties if one exists, so the ignore now guards hand-made local files rather than a generated one. Deleting it would be a small regression in footgun protection.
  • mobile-secrets may still hold android/pocket-casts/sentry.properties. Since it was never referenced by .configure, it's orphaned on that side too — worth sweeping in the same follow-up that deletes the secret.properties keys, while you're already in there.

One observation on the token wiring

sentry-cli reads SENTRY_AUTH_TOKEN from its own environment, and the plugin's exec tasks inherit the agent env. So on a correctly-provisioned agent the upload would succeed even with authToken left entirely unset in the extension — the authToken = sentryAuthToken line is explicitness rather than load-bearing wiring. What is load-bearing is the doFirst guard: it's the only thing that turns "no token" from an opaque sentry-cli failure into a legible one. Reads as correct to me; just worth knowing which half is doing the work if you ever simplify this.

Re-verified on 358eddc

  • Guard is intact and correctly scoped. build.gradle.kts:478 normalises blank → null at the single source, so authToken and the check at :499 can't disagree. tasks.withType<SentryCliExecTask>() at :497 covers the source-bundle upload alongside the ProGuard mappings and fails at script-compile time on a plugin rename.
  • Config cache is fine. org.gradle.configuration-cache=true (gradle.properties:24); the doFirst lambda captures only a String? local and references nothing from the script instance. Reading through providers.environmentVariable keeps the token a tracked, narrowly-scoped build input. Unchanged caveat: the resolved token is serialised into .gradle/configuration-cache/ on the agent — only matters if that directory is ever archived or shared between jobs.
  • No stragglers. Nothing in the tree references sentryOrg, sentryAuthToken or any sentry*Project; no .md file in the repo mentions Sentry at all, so there's no contributor doc to update.
  • DSNs untouched, per the stated tradeoff — dependencies.gradle.kts:109-114 still resolves sentryAndroidDsn / -Automotive / -Wear / -Tv from secret.properties, with the per-app fallback to the Android DSN intact.
  • sentryTvProject removal remains safetv/build.gradle.kts doesn't apply libs.plugins.sentry, so applyCommonSentryConfiguration() (gated by plugins.withId at :228) never runs there.
  • Style. No line in the five changed files exceeds 120 chars; the only two over-length lines in build.gradle.kts are the pre-existing buildConfigField calls at :287 and :289. Gradle can't run in this sandbox, so that's a manual check rather than spotlessCheck itself.

Still open (unchanged — operational, not code)

  1. Diff org = "a8c" and the three project slugs character-for-character against the current mobile-secrets values before the deletion follow-up lands. A typo surfaces only as a 404 at the upload step of a real release build, and afterwards the old values are gone.
  2. SENTRY_AUTH_TOKEN on the agents stays load-bearing and unexercised by PR CI — assemble-release-apk.sh:19 and Fastfile:647 both pass -PskipSentryProguardMappingUpload=true, so no upload task is registered on any PR build. First real exercise is build_bundle on a beta/release build. CI=true ./gradlew :app:assembleRelease --dry-run would also confirm which SentryCliExecTask subtypes get scheduled under the real upload config.

Note the failure mode still changes shape with this PR: a missing token used to mean a println and a release shipped without mappings; now it's a hard failure after the bundle is built. Better outcome, but keep the skip flag handy for the first run.


Review only — no code changes made or pushed.
· Branch: ainfra-2790-remove-sentryproperties-in-favor-of-public-values-and-env

@mokagio
mokagio requested review from ParaskP7 and wzieba August 14, 2026 02:05
@wpmobilebot wpmobilebot modified the milestones: 8.19, 8.20 Aug 17, 2026
@wpmobilebot

Copy link
Copy Markdown
Collaborator

Version 8.19 has now entered code-freeze, so the milestone of this PR has been updated to 8.20.

@iangmaia
iangmaia requested a balanced review from Copilot August 17, 2026 18:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.

Comment thread build.gradle.kts
extensions.getByType(SentryPluginExtension::class.java).apply {
authToken = project.findProperty("sentryAuthToken")?.toString()
org = project.findProperty("sentryOrg")?.toString()
authToken = sentryAuthToken

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this assignment is needed if you already have it above?

You could also keep the fact that it exists or not like val hasSentryAuthToken = providers.environmentVariable("SENTRY_AUTH_TOKEN").map { it.isNotBlank() }.orElse(false) or similar.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adopted the Provider form in 5dad760, but kept the assignment — it's load-bearing.

Checking the 6.16.0 plugin jar: SentryPluginExtension.authToken is objects.property(String::class.java).convention(null as String?). It has no environment default, so without authToken = … the extension property stays absent. SentryCliExecTask then goes the other way — it exports SENTRY_AUTH_TOKEN into the sentry-cli process from that property. And the extension value isn't only used for that: SentryOrgValueSource, SentryTelemetryService, and the source-bundle/native-symbol registration read it too.

So sentry-cli picking the token up "for free" would rely on it happening to be in the Gradle daemon's inherited environment, which is stale whenever the daemon outlives the shell that started it. The explicit assignment makes it deterministic.

Your second point stands on its own though, so I took it:

val sentryAuthToken = providers.environmentVariable("SENTRY_AUTH_TOKEN").filter { it.isNotBlank() }

authToken = sentryAuthToken
//
if (!sentryAuthToken.isPresent) { throw GradleException(…) }

Same single source for the bound value and the guard, but lazy now — a build that never configures Sentry never resolves the variable. I used filter { it.isNotBlank() } rather than map { … }.orElse(false) so the one provider serves both roles; a blank value stays absent instead of reaching the plugin as "".

Verified against the three env states (unset / set-but-empty / set), with configuration cache on: the extension property reads <absent>, <absent>, dummy-token, and the guard throws, throws, passes.

Posted by Claude Code (Opus 5) on behalf of @mokagio with approval.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correcting myself — I've reverted the provider refactor in 3ad15ee, so this is back to the String? val you commented on.

The refactor was a local improvement that made this repo the odd one out. The same migration already merged elsewhere with the eager read:

  • WordPress-Android#23189WordPress/build.gradle:71, def sentryAuthToken = providers.environmentVariable("SENTRY_AUTH_TOKEN").orNull?.trim() ?: null, bound with authToken = sentryAuthToken and guarded with if (sentryAuthToken == null) on SentryCliExecTask. Same shape as here.
  • simplenote-android#1850Simplenote/build.gradle:77, authToken = providers.environmentVariable("SENTRY_AUTH_TOKEN").orNull (no guard).

The point of this migration is one pattern across the fleet, so consistency wins over the marginally nicer form here.

On the assignment itself: it is needed. In the 6.16.0 plugin jar, SentryPluginExtension.authToken is objects.property(String::class.java).convention(null as String?) — no environment default. SentryCliExecTask goes the other way, exporting SENTRY_AUTH_TOKEN into the sentry-cli process from that property, and SentryOrgValueSource / SentryTelemetryService read it too. Without the assignment we'd be relying on the token happening to sit in the Gradle daemon's inherited environment, which is stale whenever the daemon outlives the shell that started it.

Why takeIf { it.isNotBlank() } here rather than WordPress-Android's ?.trim() ?: null: that form relies on Groovy truth, where "" ?: null is null. In Kotlin the elvis only fires on null, so a set-but-empty variable would sail through as "". Verified both against the four env states — unset, empty, whitespace, real — and they agree.

Posted by Claude Code (Opus 5) on behalf of @mokagio with approval.

@mokagio mokagio Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iangmaia real Gio here, not a meat proxy.

You could also keep the fact that it exists or not like val hasSentryAuthToken = providers.environmentVariable("SENTRY_AUTH_TOKEN").map { it.isNotBlank() }.orElse(false) or similar.

I updated the implementation in 829a341 and aligned it with the other apps that already do this.

Not sure why this assignment is needed if you already have it above?

My understanding is that the val sentryAuthToken = assigns the token from the env if any making it available to the other steps. The authToken = sentryAuthToken is what passes it to the Sentry plugin itself.

mokagio added a commit that referenced this pull request Aug 18, 2026
Reading `SENTRY_AUTH_TOKEN` through the Provider API instead of resolving it
eagerly with `.orNull` means a build that never configures Sentry never
resolves it, and the extension property and the task guard are the same
provider rather than two reads that could drift.

Per @iangmaia's review on #5708.
The plugin does not default `authToken` from the environment — its convention
is `null`, and the extension value also feeds telemetry and the org lookup, not
just the `sentry-cli` environment — so the assignment itself stays.

---

Generated with the help of Claude Code, https://claude.ai/code

Co-Authored-By: Claude Code Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown

Claude finished @mokagio's task in 3m 1s —— View job


Follow-up review — commit 5dad760

  • Diff the new commit against the previously reviewed state
  • Check the lazy-provider rewrite for correctness and API availability
  • Verify the guard still behaves as reviewed
  • Re-check stragglers and style gates
  • Post review

5dad760 is a two-line change answering @iangmaia's comment: the token is now a Provider<String> filtered on non-blank, rather than an eagerly-resolved String?. It's correct and I'd call it the better of the two shapes suggested. No blocking findings, no new ones — nothing to inline this round.


The rewrite checks out

val sentryAuthToken = providers.environmentVariable("SENTRY_AUTH_TOKEN").filter { it.isNotBlank() }
  • .filter { it.isNotBlank() } preserves the blank→absent normalisation that the previous round introduced, and keeps it at a single source: authToken (:481) and !sentryAuthToken.isPresent (:499) still can't disagree by construction. This is why it beats the hasSentryAuthToken variant from the review comment — that would have reintroduced two providers derived from the same variable, which is the drift the earlier fix was about.
  • Provider.filter needs Gradle ≥ 8.11; the wrapper is on 9.6.1 (gradle/wrapper/gradle-wrapper.properties:3), so it's available. Worth knowing only because it's the kind of API that silently constrains a wrapper downgrade.
  • Reading the provider inside doFirst is the CC-preferred idiom, not a workaround — providers.environmentVariable is exactly what Gradle recommends over a captured value or a System.getenv() call in a task action. The env var stays a narrowly-scoped tracked input, so rotating the token still invalidates the entry.
  • No behavioural change when the token is absent. Property<String>.set(null) and set(<absent provider>) both leave the property with no value, so what reaches the plugin is identical to the previously-reviewed state — the diff is purely about when the read happens.

Your commit-message reasoning for keeping the assignment matches what I'd concluded independently in the last round: sentry-cli would pick SENTRY_AUTH_TOKEN out of the inherited environment on its own, so the assignment isn't load-bearing for the upload itself — the plugin-side consumers (telemetry / org resolution) are what justify it. I can't confirm that from here (see the sandbox note below), but it's the right call regardless: explicit beats relying on env inheritance through an exec task.

Two small unverifiable-from-here notes

Neither needs action; both are things CI answers for free.

  1. authToken = <provider> assumes authToken is a Property<String>, resolved through Kotlin DSL lazy assignment. Almost certainly true — includeProguardMapping = shouldUploadDebugFiles and org = "a8c" in the same block only work that way — but note the old code assigned a String?, which would also have compiled against a plain var authToken: String?. If the property were not lazy, this commit is the one that wouldn't compile. Build-script compilation on any PR job settles it.
  2. The friendly message can still be pre-empted by input validation, in the one case where the plugin declares the token as a non-@Optional task input: Gradle snapshots inputs before running task actions, so you'd get "no value has been specified for property 'authToken'" instead of your GradleException. This is not a regression — the previous null assignment left the property in the same unset state — and it's only reachable on a real upload build. Flagging it so the outcome isn't a surprise if the first token-less run produces the plugin's error rather than yours.

Re-verified on 5dad760

  • Guard scope unchanged and still correct. tasks.withType<SentryCliExecTask>() (:497) covers sentryUploadSourceBundle* alongside the ProGuard mappings, and a plugin rename becomes a script-compile error rather than a silently vanishing guard.
  • No stragglers. Grepping the tree for sentryOrg / sentryAuthToken / sentry*Project returns only the four SENTRY_AUTH_TOKEN lines in build.gradle.kts. .gitignore:61 still ignores a local sentry.properties, which is worth keeping now that it guards hand-made files rather than a generated one.
  • DSNs untouched, per the stated tradeoff — dependencies.gradle.kts:109-116 still resolves sentryAndroidDsn / -Automotive / -Wear / -Tv from secret.properties, per-app fallback intact.
  • Escape hatch still real and in use.buildkite/commands/assemble-release-apk.sh:19 and fastlane/Fastfile:647 both pass -PskipSentryProguardMappingUpload=true.
  • Style. No line in the six changed files exceeds 120 chars; line :478 lands at 103. The only over-length lines in build.gradle.kts are the pre-existing buildConfigField calls at :290/:292.
  • Dropping my earlier System.getenv()["CI"] nit. It's the established idiom in this file (:251 uses it too), so switching only line :484 would trade one inconsistency for another. Not worth touching in this PR.

Sandbox limits, unchanged from prior rounds: Gradle can't execute and there are no resolved dependencies on disk, so I can't run spotlessCheck, can't inspect the 6.16.0 plugin jar to confirm the authToken property type or SentryCliExecTask's input annotations, and the style/length checks above are manual reads rather than the gate itself.

Still open (operational, unchanged)

  1. Diff org = "a8c" and the three project slugs character-for-character against the current mobile-secrets values before the deletion follow-up lands. A typo surfaces only as a 404 at the upload step of a real release build, and afterwards the old values are gone.
  2. SENTRY_AUTH_TOKEN on the agents stays load-bearing and unexercised by PR CI — every PR-triggered build passes the skip flag, so no upload task is registered and the guard never runs. First real exercise is build_bundle on a beta/release build. CI=true ./gradlew :app:assembleRelease --dry-run would confirm which SentryCliExecTask subtypes actually get scheduled. Keep the skip flag handy for that first run — the failure mode is now a hard failure after the bundle is built, rather than a println and a release shipped without mappings.
  3. mobile-secrets may still hold an orphaned android/pocket-casts/sentry.properties — never referenced by .configure (no files_to_copy entry targets it), so worth sweeping in the same follow-up.

Review only — no code changes made or pushed.
· Branch: ainfra-2790-remove-sentryproperties-in-favor-of-public-values-and-env

@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown

Claude finished @mokagio's task in 2m 12s —— View job


Follow-up review — commit 3ad15ee (revert of 5dad760)

  • Diff the new commit against the previously reviewed state
  • Understand why the lazy-provider change was reverted
  • Re-verify the guard on the reverted code
  • Re-check stragglers and style gates
  • Post review

3ad15ee is a clean revert: git diff 358eddc..HEAD is empty, so the tree is byte-identical to 358eddc — the state I reviewed in full on the ready-for-review pass. Everything verified there still holds. No blocking findings, nothing new to inline.


On the revert itself

The rationale in the commit message is a legitimate one and not mine to second-guess: fleet consistency with simplenote-android#1850 and WordPress-Android#23189 is the point of the migration, and "one pattern across the fleet" beats a locally nicer read when the next person maintaining all three has to recognise the same shape. Worth recording what the eager form costs, since it's small and bounded but non-zero:

val sentryAuthToken = providers.environmentVariable("SENTRY_AUTH_TOKEN").orNull?.takeIf { it.isNotBlank() }
  • The single-source property survives the revert, which is the part that actually mattered. authToken (build.gradle.kts:481) and the guard (:499) still read the same val, so they can't drift, and blank → null is still normalised once at the source. The two findings from the first round remain fixed.
  • The resolved token is captured into the doFirst lambda as a plain String?, so it lands in the serialised task graph under .gradle/configuration-cache/. The provider form kept only the provider spec there (value re-read at execution, hashed into the input fingerprint for invalidation). Same caveat I've flagged every round — only matters if that directory is ever archived or shared across jobs — just slightly more literal now.
  • The env var is resolved during configuration of every app module, whether or not anything will upload. So on an agent where the token is present, rotating it invalidates the CC entry for all builds, not just upload builds. A cache miss, not a correctness issue.
  • ?.takeIf { it.isNotBlank() } is again the only surviving trace of why blank counts as unset, since eaac1b3 removed the comment explaining it. Unchanged from before; noting it only because the plausible future regression is someone shortening it to .orNull, which quietly restores the opaque mid-upload sentry-cli error.

Re-verified on 3ad15ee

  • Guard intact and correctly scoped. tasks.withType<SentryCliExecTask>() (:497) covers sentryUploadSourceBundle* alongside the ProGuard mappings, and a plugin rename becomes a script-compile error rather than a silently vanishing guard. configureEach keeps it lazy.
  • doFirst is still the right placement. The lambda captures only a String? local and references nothing from the script instance, so CI jobs that run with CI=true but never upload (unit tests, lint) are unaffected and the check stays out of configuration time.
  • No stragglers. Grepping the tree for sentryOrg / sentryAuthToken / sentry*Project returns only the three SENTRY_AUTH_TOKEN lines in build.gradle.kts (:478, :481, :499). .gitignore still ignores a local sentry.properties, worth keeping now that it guards hand-made files rather than a generated one.
  • DSNs untouched, per the stated tradeoff — dependencies.gradle.kts still resolves sentryAndroidDsn / -Automotive / -Wear / -Tv from secret.properties, per-app fallback intact.
  • Escape hatch real and in use.buildkite/commands/assemble-release-apk.sh:19 and fastlane/Fastfile:647 both pass -PskipSentryProguardMappingUpload=true.
  • Style. No line in the changed files exceeds 120 chars; :478 is the longest new line at 119. The only over-length lines in build.gradle.kts are the pre-existing buildConfigField calls at :287/:289.

Sandbox limits, unchanged from prior rounds: Gradle can't execute and there are no resolved dependencies on disk, so I can't run spotlessCheck and can't inspect the 6.16.0 plugin jar. Build-script compilation is settled by any PR job — and since this reverts to a tree CI has already built, that's already covered.

Still open (operational, unchanged)

  1. Diff org = "a8c" and the three project slugs character-for-character against the current mobile-secrets values before the deletion follow-up lands. A typo surfaces only as a 404 at the upload step of a real release build, and afterwards the old values are gone.
  2. SENTRY_AUTH_TOKEN on the agents stays load-bearing and unexercised by PR CI — every PR-triggered build passes the skip flag, so no upload task is registered and the guard never runs. First real exercise is build_bundle on a beta/release build; CI=true ./gradlew :app:assembleRelease --dry-run would confirm which SentryCliExecTask subtypes get scheduled. Keep the skip flag handy for that first run: the failure mode is now a hard failure after the bundle is built, rather than a println and a release shipped without mappings.
  3. mobile-secrets may still hold an orphaned android/pocket-casts/sentry.properties — never referenced by .configure (no files_to_copy entry targets it), so worth sweeping in the same follow-up.

Review only — no code changes made or pushed.
· Branch: ainfra-2790-remove-sentryproperties-in-favor-of-public-values-and-env

mokagio and others added 7 commits August 18, 2026 20:34
`sentryOrg`, the per-variant `sentry*Project` slugs, and `sentryAuthToken`
aren't secrets: the org/project slugs are Sentry-public identifiers, and the
auth token is CI-only. Moving them out of `secret.properties` shrinks what
mobile-secrets needs to hold for this repo.

The Sentry DSNs stay in `secret.properties` unchanged. Pocket Casts Android
is open source and hasn't otherwise adopted a8c-secrets yet, so per the
"Public vs. Secrets x Internal vs. External" RFC the DSN can't become a
public repo constant the way it did in simplenote-android/dayone-android.

The mobile-secrets deletion of the six keys above happens separately.

Part of AINFRA-2790.

---

Generated with the help of Claude Code, https://claude.ai/code

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A secret-injection step that resolves to nothing exports an empty
`SENTRY_AUTH_TOKEN` rather than leaving it unset, and `Provider.isPresent` is
`true` for that.
The guard passed, a blank token reached the plugin, and sentry-cli failed
mid-upload with an opaque auth error instead of the actionable message.
Normalising once at the source keeps the bound `authToken` and the guard from
disagreeing.

`includeSourceContext` rides the same flag as `includeProguardMapping` here, so
`sentryUploadSourceBundle*` needs the token just as much, but the old
`uploadSentryProguardMappings` name prefix never matched it — and task ordering
isn't guaranteed, so a token-less CI run could fail there first.
`SentryCliExecTask` is the base of every task that shells out to sentry-cli with
the token, so matching on the type covers both and fails at compile time if a
future plugin bump renames a task.

This diverges from the name-prefix match in the sibling wpandroid and
dayone-android changes.
dayone-android never enables source context, so it isn't exposed; wpandroid does
and still is.

Both points came from review feedback on #5708.

---

Generated with the help of Claude Code, https://claude.ai/code

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>
Reading `SENTRY_AUTH_TOKEN` through the Provider API instead of resolving it
eagerly with `.orNull` means a build that never configures Sentry never
resolves it, and the extension property and the task guard are the same
provider rather than two reads that could drift.

Per @iangmaia's review on #5708.
The plugin does not default `authToken` from the environment — its convention
is `null`, and the extension value also feeds telemetry and the org lookup, not
just the `sentry-cli` environment — so the assignment itself stays.

---

Generated with the help of Claude Code, https://claude.ai/code

Co-Authored-By: Claude Code Opus 5 <noreply@anthropic.com>
This reverts 5dad760, restoring the eager read.

The lazy provider was a local improvement that made this repo the odd one out.
simplenote-android#1850 and WordPress-Android#23189 both shipped the eager
`.orNull` read bound to a `String?` val, and this migration's value is one
pattern across the fleet, not the best pattern per repo.

---

Generated with the help of Claude Code, https://claude.ai/code

Co-Authored-By: Claude Code Opus 5 <noreply@anthropic.com>
A token injected with a trailing newline passed `isNotBlank()` and reached
`sentry-cli` verbatim, failing with the opaque auth error the guard exists to
pre-empt.

Aligns with WordPress-Android#23189 and DayOne-Android#8230, which both trim.
Their `?.trim() ?: null` does not port directly: it relies on Groovy truth,
where `"" ?: null` is `null`. Kotlin's elvis fires only on `null`, so the
literal translation would let a set-but-empty variable through as `""`.

---

Generated with the help of Claude Code, https://claude.ai/code

Co-Authored-By: Claude Code Opus 5 <noreply@anthropic.com>
@mokagio
mokagio force-pushed the ainfra-2790-remove-sentryproperties-in-favor-of-public-values-and-env branch from 3ad15ee to 829a341 Compare August 18, 2026 10:37
@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown

Claude finished @mokagio's task in 2m 21s —— View job


Follow-up review — commit 829a341

  • Diff the new commit against the previously reviewed state
  • Verify the trim semantics against the four env-var states
  • Re-check the guard, stragglers and style gates
  • Post review

829a341 is a one-line change: ?.takeIf { it.isNotBlank() }?.trim()?.ifEmpty { null }. It's a strict improvement over the state I reviewed on 3ad15ee, and it compiles as written. No blocking findings, nothing to inline. One assumption about plugin internals is worth naming, and the plugin has moved to 6.18.0 since the earlier jar inspection.


The change is correct across all four env states

val sentryAuthToken = providers.environmentVariable("SENTRY_AUTH_TOKEN").orNull?.trim()?.ifEmpty { null }
SENTRY_AUTH_TOKEN before (takeIf { isNotBlank() }) after (trim().ifEmpty { null })
unset null null
"" null null
" " null null
"sntrys_…\n" "sntrys_…\n" → opaque CLI auth error "sntrys_…"
"sntrys_…" unchanged unchanged

So the only changed row is the one the commit message targets, and the guard's behaviour on the three empty-ish rows is preserved. A trailing newline is a realistic shape for an injected secret ($(cat secret), a heredoc, some secret-manager CLIs), so this is a real case rather than a theoretical one — and it was the worst one, because it's the only input that passed the guard and then failed deep in the upload, which is precisely what the guard exists to prevent.

?.ifEmpty { null } type-checks, for the record. It resolves to the generic <C, R> C.ifEmpty(() -> R): R where C : CharSequence, C : R — with C = String and the lambda returning Nothing?, R widens to String?, satisfying C : R. Result is String?, same as before, so authToken = sentryAuthToken binds identically. ?.ifBlank { null } would collapse the two calls into one, but after trim() the two forms are indistinguishable and the current spelling reads closer to the sibling repos' ?.trim(). Not worth changing.

The rationale in the commit message about Groovy truth is right. ?.trim() ?: null works in WordPress-Android because Groovy's elvis treats "" as falsy; ported literally to Kotlin the elvis fires only on null, so a set-but-empty variable would bind "". This keeps the fleet's semantics while adapting the spelling to the language — the correct way to be consistent.

One assumption worth naming (not a blocker)

The trim only reaches sentry-cli if the plugin overrides the child process's SENTRY_AUTH_TOKEN from the extension property rather than letting the exec task inherit the agent's raw value. Gradle's Exec environment starts as a copy of the parent env, so a plugin that sets the key wins over inheritance and the trimmed value is what the CLI sees. If instead the plugin only used the property for its own purposes (org resolution, telemetry) and relied on inheritance for the CLI, the untrimmed env var would still reach sentry-cli and this fix would be inert for the newline case — harmless, but not the improvement claimed.

You verified in an earlier round that SentryCliExecTask exports the variable from the property, which is the override case, so this is very likely already settled. Flagging it only because it's the single link that makes this commit do anything, and it's not visible from the diff.

The plugin is now 6.18.0, not 6.16.0

gradle/libs.versions.toml:54 reads sentry-plugin = "6.18.0". That didn't come from this PR — origin/main already has it via the 8.19 release merge (2dc458d) — but it means the jar inspections cited in this PR's discussion (SentryCliExecTask being the public base of the token-using tasks; authToken being objects.property(String).convention(null)) were made against a version the tree no longer resolves. Both facts are very likely stable across a patch bump, and the import io.sentry.android.gradle.tasks.SentryCliExecTask is a compile-time reference, so a rename or removal is a build-script compile failure rather than a silently vanishing guard.

Worth knowing that GitHub Actions doesn't settle that here — the only non-Claude check on this PR is Validate Gradle Wrapper. Buildkite compiling the build scripts is the actual confirmation. Also note authToken's convention(null) is the fact that makes the authToken = sentryAuthToken assignment load-bearing (per your reply to @iangmaia); if a future bump gave it an environment convention, the assignment would become redundant rather than wrong.

Re-verified on 829a341

  • Single source preserved. authToken (:481) and the guard (:499) still read the same val, so they can't drift — the fix from the first review round survives its third rewrite, which is the property that matters most here.
  • Guard scope unchanged. tasks.withType<SentryCliExecTask>().configureEach (:497) covers sentryUploadSourceBundle* alongside the ProGuard mappings; configureEach keeps it lazy, and the doFirst lambda captures only a String? local, so nothing from the script instance is dragged into the serialised task graph.
  • No stragglers. Grepping the tree for sentryOrg / sentryAuthToken / sentry*Project returns only the four SENTRY_AUTH_TOKEN lines in build.gradle.kts (:478, :481, :499, :501). No .buildkite/ script, Fastlane lane, gradle.properties default or doc references the removed keys.
  • DSNs untouched, per the stated tradeoff — dependencies.gradle.kts still resolves sentryAndroidDsn / -Automotive / -Wear / -Tv from secret.properties, per-app fallback intact.
  • Escape hatch real and in use.buildkite/commands/assemble-release-apk.sh:19 and fastlane/Fastfile:647 both pass -PskipSentryProguardMappingUpload=true.
  • Style. Line :478 is 109 chars, under the 120 limit. The only over-length lines in build.gradle.kts remain the pre-existing buildConfigField calls at :287/:289.
  • Config cache. Unchanged from 3ad15ee: the token resolves at configuration time and lands in .gradle/configuration-cache/ as a literal, which matters only if that directory is ever archived or shared across jobs. The trim doesn't affect input tracking — providers.environmentVariable still registers the read.

Sandbox limits, unchanged: Gradle can't execute and there are no resolved dependencies on disk, so I can't run spotlessCheck or inspect the 6.18.0 plugin jar. The length and grep checks above are manual reads, not the gates themselves.

Still open (operational, unchanged from prior rounds)

  1. Diff org = "a8c" and the three project slugs character-for-character against the current mobile-secrets values before the deletion follow-up lands. A typo surfaces only as a 404 at the upload step of a real release build, and afterwards the old values are gone.
  2. SENTRY_AUTH_TOKEN on the agents stays load-bearing and unexercised by PR CI — every PR-triggered build passes the skip flag, so no upload task is registered and the guard never runs. First real exercise is build_bundle on a beta/release build; CI=true ./gradlew :app:assembleRelease --dry-run would confirm which SentryCliExecTask subtypes get scheduled. Keep the skip flag handy for that first run: the failure mode is now a hard failure after the bundle is built, rather than a println and a release shipped without mappings.
  3. mobile-secrets may still hold an orphaned android/pocket-casts/sentry.properties — never referenced by .configure, so worth sweeping in the same follow-up.

Review only — no code changes made or pushed.
· Branch: ainfra-2790-remove-sentryproperties-in-favor-of-public-values-and-env

The guard rejects a set-but-empty variable as well as an unset one, but the
message named only the unset case, sending anyone who hit it looking for a
variable that is in fact there.

Matches DayOne-Android#8230 and WordPress-Android#23189.

---

Generated with the help of Claude Code, https://claude.ai/code

Co-Authored-By: Claude Code Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown

Claude finished @mokagio's task in 1m 12s —— View job


Follow-up review — commit f981a32

  • Diff the new commit against the previously reviewed state
  • Verify the error message and guard on the final commit
  • Re-check stragglers and style gates
  • Post review

f981a32 changes one string: "SENTRY_AUTH_TOKEN is not set.""SENTRY_AUTH_TOKEN is not set (or is blank)." No behavioural change; git diff 829a341..HEAD touches nothing else. Approving in spirit — no findings, nothing to inline.


The message now matches the guard

This closes the last small gap between what the code does and what it says. As of 829a341 the guard fires on three distinct env states, only one of which the old message described:

SENTRY_AUTH_TOKEN sentryAuthToken old message new message
unset null accurate accurate
"" null "is not set" — misleading accurate
" " null "is not set" — misleading accurate

The middle two rows are the realistic CI failures — a secret-injection step that resolves to nothing exports the variable empty rather than leaving it unset. Someone debugging that would have run echo $SENTRY_AUTH_TOKEN, seen a line of output, and concluded the message was wrong about their environment. Now it points at the actual cause.

It also partly compensates for what eaac1b3 removed: with the explanatory comment above line 478 gone, this message is the only place in the repo that records blank-is-treated-as-unset. Not a substitute for a code comment — a reader of ?.trim()?.ifEmpty { null } won't see the string 23 lines below — but the semantics are at least written down somewhere now.

Line 501 lands at 116 chars, under the 120 limit; the only over-length lines in the file remain the pre-existing buildConfigField calls at :287/:289.

Re-verified on f981a32

  • Single source intact. authToken (build.gradle.kts:481) and the guard (:499) still read the same val from :478, so they can't drift. That's the fix from the first review round, now surviving four rewrites of the surrounding expression.
  • Guard scope unchanged. tasks.withType<SentryCliExecTask>().configureEach (:497) covers sentryUploadSourceBundle* alongside the ProGuard mappings; configureEach keeps it lazy, and the doFirst lambda captures only a String? local.
  • No stragglers. Grepping the tree for sentryOrg / sentryAuthToken / sentry*Project returns only the four SENTRY_AUTH_TOKEN lines in build.gradle.kts (:478, :481, :499, :501).
  • Escape hatch named in the message is real and in use.buildkite/commands/assemble-release-apk.sh:19 and fastlane/Fastfile:647 both pass -PskipSentryProguardMappingUpload=true. Worth noting the message names the ProGuard-specific flag while the guard now covers source-bundle uploads too; that's still correct in this repo, since that one flag gates both includeProguardMapping and includeSourceContext (:486:487).
  • DSNs untouched, per the stated tradeoff — dependencies.gradle.kts still resolves sentryAndroidDsn / -Automotive / -Wear / -Tv from secret.properties.
  • Diff is still 6 files, +19/−24, matching the description.
  • Plugin is 6.18.0 (gradle/libs.versions.toml:54), not the 6.16.0 the jar inspections in this thread were made against — that came from main via the 8.19 merge, not from this PR. Both facts cited (SentryCliExecTask as the public base; authToken having a null convention) are very likely stable across a patch bump, and the import makes a rename a script-compile error rather than a silent no-op.

Sandbox limits, unchanged: Gradle can't execute and there are no resolved dependencies on disk, so I can't run spotlessCheck or inspect the 6.18.0 plugin jar. The length and grep checks above are manual reads, not the gates themselves. Note the only non-Claude check on this PR is Validate Gradle Wrapper — Buildkite compiling the build scripts is the real confirmation that the typed match resolves.

Still open (operational, unchanged from prior rounds)

  1. Diff org = "a8c" and the three project slugs character-for-character against the current mobile-secrets values before the deletion follow-up lands. A typo surfaces only as a 404 at the upload step of a real release build, and afterwards the old values are gone.
  2. SENTRY_AUTH_TOKEN on the agents stays load-bearing and unexercised by PR CI — every PR-triggered build passes the skip flag, so no upload task is registered and this message can't be reached. First real exercise is build_bundle on a beta/release build; CI=true ./gradlew :app:assembleRelease --dry-run would confirm which SentryCliExecTask subtypes get scheduled. Keep the skip flag handy for that first run — the failure mode is now a hard failure after the bundle is built, rather than a println and a release shipped without mappings.
  3. mobile-secrets may still hold an orphaned android/pocket-casts/sentry.properties — never referenced by .configure, so worth sweeping in the same follow-up that deletes the secret.properties keys.

Review only — no code changes made or pushed.
· Branch: ainfra-2790-remove-sentryproperties-in-favor-of-public-values-and-env

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Area] Gradle [Type] Tooling Related to the Gradle build scripts and the setup or maintenance of the project build process.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants