Conversation
The experimental editor fails with `rest_no_route` on some Atomic sites because `siteApiRoot` is a WP.com proxy root that already embeds `wp/v2/sites/<id>` while `siteApiNamespace` is empty, so GutenbergKit appends each already-namespaced path unchanged and every request 404s. Add the instrumentation that identifies which of the two inputs is wrong and where the bad value entered the system: - `GutenbergKitSettingsBuilder` logs the resolved root alongside the site classification it was derived from, and raises an error-level canary when the direct-host branch is handed a proxy root. A `ConfigSource` parameter distinguishes the preloader (which re-reads the site from the store) from the editor (which uses the copy serialized into its intent), so lines that disagree point at a stale in-memory model rather than a stale row. - `SiteSqlUtils.insertOrUpdateSite` logs the `wpApiRestUrl` the WellSql mapper is about to persist on insert. The mapper calls the getter, which synthesizes a proxy root while the site reads as WP.com Simple, and the update path excludes the column — so the insert decides the value for good. - `SiteStore.persistAppPasswordColumns` and the auto-auth mint log the root they persist, covering the paths that rewrite the column later. Diagnostics only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aCE7dERu43yYPzoxrqeq4
`getWpApiRestUrl()` returned "https://public-api.wordpress.com/wp/v2/sites/<id>" whenever the site read as WP.com Simple, so callers would "get the correct endpoint without needing site-type checks". That value is not the same kind of URL as a stored one: it already embeds the `wp/v2/sites/<id>` namespace, while every reader treats the property as a bare direct-host root and appends an already-namespaced path to it. Worse, the value did not stay derived. The generated WellSql mapper reads this getter (`SiteModelMapper.put("WP_API_REST_URL", item.getWpApiRestUrl())`) and `insertOrUpdateSite` writes the full row on insert, while its update path excludes the column. So a site added to the app while still Simple had the proxy root written into WP_API_REST_URL at insert and kept it for good — no later sync could correct it. Once such a site transferred to Atomic and acquired an application password, `GutenbergKitSettingsBuilder` used the stored proxy root as a direct-host root with an empty namespace, and every editor request carried `wp/v2/sites/<id>` twice and 404'd with `rest_no_route`. That also explains why the same site worked on a second device: a device that first saw the site after the transfer inserted the row with a null column. Return only the stored value. No caller needs the synthesis — WP.com-routed requests go through `WpComUrlResolver` (`WpApiClientProvider.getWpApiClient` / `getApiUrlResolver`) or `wpComRestClient` (`ReactNativeStore`), and every reader of this property pairs it with a `${url}/wp-json` fallback because it wants a direct host. Existing rows are already poisoned; the migration follows separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aCE7dERu43yYPzoxrqeq4
Sites added to the app while they were WP.com Simple had a synthesized proxy root written into WP_API_REST_URL at insert. The column is excluded from the generic full-row update, so nothing rewrites it — existing installs stay broken after the getter change alone. Null the derived values so REST discovery can repopulate a real root. The prefix match is safe: only the removed synthesizer produced URLs of the form https://public-api.wordpress.com/wp/v2/sites/<id>, and a genuine direct-host root never has that shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aCE7dERu43yYPzoxrqeq4
The direct-host branch pairs `siteApiRoot` with an empty `siteApiNamespace`, so it needs a bare root. A root under the WP.com proxy already embeds `wp/v2/sites/<id>`; using one there makes GutenbergKit append an already-namespaced path to an already-namespaced root, and every editor request 404s with `rest_no_route`. The getter no longer produces such a value and the migration clears the stored ones, so this is a backstop — but it's the layer that knows the two inputs have to agree, and it turns a silent storm of 404s into a logged fallback to the site's own host. Adds the matrix cell the suite was missing: a WPCom-flagged site holding an application password. The existing coverage only exercised a Jetpack site there, which is why the combination that breaks went unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aCE7dERu43yYPzoxrqeq4
`persistAppPasswordColumns` runs in tests that don't stub `android.util.Log`, so the diagnostic added while tracing CMM-2383 failed three SiteStoreTest cases with "Method e in android.util.Log not mocked". It has served its purpose: it established that the proxy root arrived here already set rather than being synthesized at this point, which is what moved the search to the insert. The insert-time log in `SiteSqlUtils` covers the origin and the editor routing log covers the symptom, so drop this one rather than loosen the module's unit-test config for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aCE7dERu43yYPzoxrqeq4
Detekt flagged two thresholds the previous commit crossed: LongMethod on `buildPostConfiguration` (61 of a maximum 60) and LargeClass on `GutenbergKitSettingsBuilderTest`. Extract the root, namespace and auth header into `resolveApiTransport`. They are derived from one another and only valid together — a WP.com root needs the namespace to carry `sites/<id>/`, a direct-host root needs it empty, and the auth header has to match the host being addressed. Holding them in one `ApiTransport` names that invariant, which is precisely what went wrong in CMM-2383, and leaves `buildPostConfiguration` assembling the configuration rather than deriving it. Move the API-routing tests into `GutenbergKitSettingsBuilderApiRoutingTest` for the same reason: the routing matrix is a coherent unit, and separating it returns the original class to its previous size. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aCE7dERu43yYPzoxrqeq4
Contributor
|
|
Contributor
|
|
Contributor
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
`GutenbergEditorPreloaderTest` stubs `buildPostConfiguration` with argument matchers, so adding the `source` parameter left it one matcher short and all 15 cases failed with "6 matchers expected, 5 recorded". A Kotlin default doesn't help here — the mock still records the call with every argument. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aCE7dERu43yYPzoxrqeq4
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #23301 +/- ##
==========================================
+ Coverage 38.04% 38.06% +0.01%
==========================================
Files 2353 2354 +1
Lines 128959 129005 +46
Branches 17976 17981 +5
==========================================
+ Hits 49068 49103 +35
- Misses 75871 75878 +7
- Partials 4020 4024 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Findings from review of #23301. The proxy check lived in one consumer while the PR description named six, and its prefix disagreed with the migration's. WP.com Simple sites advertise `https://public-api.wordpress.com/wp-json/?rest_route=/sites/<domain>` during discovery, and `ApplicationPasswordLoginHelper` persists whatever discovery returns — so a second proxy shape reaches the column, survives a migration matching only `/wp/v2/sites/%`, and leaves the editor logging a rejection every launch while `WpServiceProvider` and `getApplicationPasswordClient` keep using the bad root with no error of their own. Move the predicate to `WPComApiProxy` and apply it where the value is written: `SiteSqlUtils.updateWpApiRestUrl` is the sole writer of the column, so refusing there covers every reader at once. The migration now shares the same prefix, and the editor keeps a backstop for rows predating both. Also normalize the trailing slash. `api-fetch`'s `createRootURLMiddleware` strips a path's leading slash before concatenating onto the root, and `buildEditorAssetsEndpoint` concatenates with no separator, so a slash-less root yields `…/wp-jsonwp/v2/types` — the same failure this PR fixes. `WpApiClientProvider` builds exactly such roots and they reach the column through app-password login. Roots carrying a query string are the plain permalink `?rest_route=` form and are left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aCE7dERu43yYPzoxrqeq4
Findings from review of #23301. - `persistAppPasswordColumns` claimed the credential gate is what stops a credential-less sync from blanking a discovered root. It isn't: the isNotEmpty() check below does that, and the credential check gates the whole write. Someone trusting the old wording could drop the takeIf and reintroduce the clobber. - `logResolvedApiRoot` described `getWpApiRestUrl()` as synthesizing a WP.com proxy root and read `isSimple=true` as meaning the persisted root is about to be wrong. Both were true when the log was added to trace this bug and are false as merged — the synthesis is gone. - `SiteModelTest` still assigned a siteId that no assertion reads now that the proxy-URL expectation became assertNull. - `ConfigSource` was a generic name at package level for a type only this builder uses; nest it so ownership is obvious. `internal` would have been the narrower fix but cascades onto the public function that takes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aCE7dERu43yYPzoxrqeq4
Findings from review of #23301. The migration is the whole fix for existing installs and was verified only by hand on one device. A wrong LIKE pattern ships as a silent no-op — the upgrade succeeds and the rows stay broken — so assert the statement directly: both proxy shapes cleared, a direct-host root untouched, a lookalike domain (public-api.wordpress.com.example.net) untouched, and null rows left alone. Confirmed the test has teeth by narrowing the pattern back to the wp/v2 form: exactly the rest_route case fails, which is the shape the first revision missed. Rows are seeded with raw SQL because updateWpApiRestUrl now refuses the values the migration exists to clean up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aCE7dERu43yYPzoxrqeq4
updateWpApiRestUrl now also returns 0 when it refuses a WP.com proxy URL, so "no site with localId=..." would name the wrong cause — the kind of confidently wrong log that sent this investigation down a blind alley in the first place. Report both possibilities; the writer logs which one it was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aCE7dERu43yYPzoxrqeq4
dcalhoun
marked this pull request as ready for review
September 6, 2026 02:06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


TL;DR
Fix CMM-2383. The editor failed to load with
rest_no_routeon some Atomic sites because their stored REST API root was a WP.com proxy URL that already contained thewp/v2/sites/<id>namespace, so every request carried the namespace twice. While the erroneous URL surfaced as an editor failure, the impact is far wider, albeit latent.Description
Symptom
Every editor dependency 404s with
rest_no_route, and the editor never loads:Those decompose exactly as
siteApiRoot + path, withsiteApiNamespaceempty. The root already embedswp/v2/sites/<id>, so GutenbergKit appends an already-namespaced path to an already-namespaced root.Root cause
SiteModel.getWpApiRestUrl()returned a synthesized WP.com proxy root whenever the site read as WP.com Simple, so callers would "get the correct endpoint without needing site-type checks". But that value isn't the same kind of URL as a stored one, and every reader treats the property as a bare direct-host root. Introduced in #22765.The value also didn't stay derived. The generated WellSql mapper reads the getter —
cv.put("WP_API_REST_URL", item.getWpApiRestUrl())— andinsertOrUpdateSitewrites the full row on insert, while its update path excludes the column. So a site added to the app while it was still Simple had the proxy root written intoWP_API_REST_URLat insert and kept it permanently; no later sync could correct it.Once such a site transferred to Atomic and acquired an application password,
GutenbergKitSettingsBuildertook the direct-host branch, used the stored proxy root, and set an empty namespace.Why it looked user-specific
The row is poisoned at insert, so only on the device that added the site while it was Simple — in practice the device that created it. A device that first sees the site after the Atomic transfer inserts it with a null column and works fine. That's why the reported site failed for its creator and worked for an administrator added later, and why pull-to-refresh never helped.
The application password isn't a cause; it's the switch that exposes the already-poisoned column by flipping
shouldUseWPComRestApitofalse.Scope beyond the editor
Everything that reads
wpApiRestUrlas a direct-host root was affected:WpApiClientProvider.getApplicationPasswordClient/getDirectHostApiUrlResolver,WpServiceProvider(the wordpress-rs Posts list),ApplicationPasswordsNetwork, andApplicationPasswordValidator. That accounts for the Posts-list failures and the "Unable to connect to your site" banner seen alongside the editor error.The change
SiteModel.getWpApiRestUrl()returns the stored value only. No caller needed the synthesis: WP.com-routed traffic goes throughWpComUrlResolver(WpApiClientProvider.getWpApiClient/getApiUrlResolver) orwpComRestClient(ReactNativeStoredispatches onisUsingWpComRestApi). Every reader of the property wants a direct-host root, and each pairs it with a${url}/wp-jsonfallback — though note that five fluxc readers use a bare?:, so the fallback doesn't fire for an empty string (see follow-ups).SiteSqlUtils.updateWpApiRestUrlrefuses WP.com proxy URLs. It is the sole writer of the column, so rejecting there covers every reader at once rather than each having to recognise the shape. The predicate lives in a newWPComApiProxyand is shared with the migration, so a value the writer rejects can't survive in an older row.api-fetch'screateRootURLMiddlewarestrips a path's leading slash before concatenating onto the root, andbuildEditorAssetsEndpointconcatenates with no separator, so a slash-less root yields…/wp-jsonwp/v2/types— the same failure class.WpApiClientProviderbuilds exactly such roots and they reach the column via app-password login. Roots carrying a query string are the plain-permalink?rest_route=form and are left alone.WP_API_REST_URLvalues matchinghttps://public-api.wordpress.com/wp/v2/sites/%. Required: existing installs are already poisoned and nothing else rewrites that column. The prefix match is safe — only the removed synthesizer produced that shape.GutenbergKitSettingsBuilderrejects a proxy root in the direct-host branch and falls back to the site's own host, logging when it does. A backstop, but this is the layer that knows the root and namespace have to agree.resolveApiTransportnow derives root, namespace and auth header together, since they're only valid as a set.ConfigSource(preloader vs editor), andSiteSqlUtilslogs the value the mapper is about to persist on insert. These were how the origin was found and are worth keeping as a canary.Testing instructions
The reproduction needs the site to be created in the app — that's the step that inserts the poisoned row.
Reproduce on
trunkfirst:https://public-api.wordpress.com/wp/v2/sites/<id>/wp/v2/...Then install this branch over it (do not clear app data — the migration is the point):
Upgrading database from version 212 to 213WP_API_REST_URLis now null, and that legitimate…/wp-json/roots on other sites are untouched:root=https://<site>/wp-json/, and norest_no_routeRegression checks:
wpcomRest=true root=https://public-api.wordpress.com/and asites/<id>/namespace — unchanged behaviourVerified on device
Both previously-broken Atomic sites now load. The migration cleared 8 poisoned rows and left 3 legitimate roots untouched. A Simple site and a private Atomic site that already worked were unchanged.
Known gaps, deliberately left out
Both are pre-existing, neither is made worse here, and each wants its own tests and blast-radius review.
clearWpApiRestUrlwrites"", notNULL. Five fluxc readers use a bare?:, so an empty string reads as "present" and resolves to a host-less base URL. Its one caller isremoveApplicationPassword, so the trigger is narrow — but it is sticky, becauseReactNativeStoretreats non-null as "already discovered" and therefore stops re-running discovery that would otherwise heal the row.SiteModelserialized into its intent, while the preloader deliberately re-reads from the store (GutenbergEditorPreloaderdoesgetSiteByLocalId(...) ?: siteprecisely because provisioning mints credentials mid-flight). A snapshot taken before provisioning yields the WP.com bearer path where the preloader used the direct host. Both transports work, so the realistic cost is a preload cache miss rather than a failure — theConfigSourcetagging on the routing log exists to measure whether it happens in the field before changing behaviour.Related
rest_no_routestring but are a different bug — a well-formed request to a route that genuinely doesn't exist. Its fix degrades optional dependencies, so it would not have helped here:/wp/v2/typesand/wp/v2/themesare required.${site.url}/wp-json/, which works — but it's what will let discovery actually populate the column for those sites.🤖 Generated with Claude Code
https://claude.ai/code/session_017aCE7dERu43yYPzoxrqeq4