Repository navigation
fix: explain dynamic app contract fallbacks without exception text - #497
veryCrunchy wants to merge 78 commits into
Conversation
Classify App Store contract acquisition failures in the contract module and translate them once in shared JVM code for Android and desktop. A runtime linkage failure, such as a static initializer the Android regex engine rejects, now becomes a client-fault fallback instead of escaping with a class name as its message. Record a typed fallback reason on metadata-only discoveries and derive the fallback card copy from it, so missing contracts, unconfirmed versions, unreachable or unavailable App Store releases, rejected packages, and unsupported contracts each explain what happened. Extend the brace regex architecture check to Android-only source sets and initialize every contract module facade in the Android instrumented test.
Obiente previewNC Native · Obiente updates this comment as the preview changes. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1e0f3d646
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Return cancellation unchanged from the package verification wrapper so the acquisition boundary still recognizes it as control flow. Publish a downloaded App Store catalog to the local cache on a best effort basis, matching verified-contract caching. A read-only cache directory or a full disk no longer fails an acquisition that already has the catalog, and is never reported as an unreachable App Store.
Discover androidApp source sets instead of listing only main, so debug, release, directApk, and instrumented test sources are checked too. JVM unit test sources stay excluded because they never run on ICU.
…ontract-acquisition # Conflicts: # tools/kotlin-file-size-baseline.txt
Signed-off-by: veryCrunchy <me@verycrunchy.dev>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9eb8dcf9d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ontract-acquisition # Conflicts: # tools/kotlin-file-size-baseline.txt # website/public/screenshots/capture-manifest.json
A completed acquisition without a contract no longer collapses to null. The contract module reports whether no release matches the installed version, the linked source refused or rate limited the last exact-tag fallback, or the inspected releases provide no usable contract. The shared JVM boundary carries that reason to discovery, and the fallback card derives its copy and action from it.
List the new catalog cache, trust verifier, and YAML parser facades in the Android initializer test, and exercise the real catalog path with a failing cache and YAML parsing on the Android runtime. A JVM unit test loads every packaged facade and fails when one is missing from that list.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 830f783b4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Catalog parsing now records which required value made each entry for the requested app unusable. When no usable release matches and a dropped entry would have been selected, acquisition reports a malformed source instead of claiming the App Store lists no matching release. The reason crosses the platform boundary and has its own fallback copy.
…sons The same-origin OCS catalog, spec, and static contract probes now record the first inconclusive result as a typed failure: no response, a server error, or rate limiting. A definitive absence records nothing. When the App Store then completes without a contract, an inconclusive server probe takes precedence and the card offers a retry instead of claiming that neither source provides an API. Diagnostics keep both sources.
… accepted result Every cached-contract return (an automatic cache hit and a limited refresh serving the cache) now goes through the publication fence. A read superseded since it started reports the newest accepted result, and while a definitive no-contract result or an invalidation is the latest accepted result, a cache read reports it instead of the stale contract.
…talog evidence When a catalog has more than one record with the requested app id and they differ, no record is selected; the catalog reports a DuplicateApp dropped entry, so the result is MalformedSource instead of whichever record came first. Identical copies are read once, and duplicates of other apps do not matter.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cf4906c82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
When the OCS API Viewer catalog has more than one record for the app and they differ, none is read: the probe records UnreadableResponse with a catalog-stage duplicate-records diagnostic instead of importing whichever record came first. Identical copies, compared as JSON values, are read once, and duplicates of other apps are ignored, matching the App Store catalog rule.
Same-origin probes now treat only 404 and 410 as definitive absence. 401, 403, 429, and 5xx keep their kinds, and every other unsuccessful status, such as 400, 408, an unfollowed 3xx, or a 1xx, is a new inconclusive UnexpectedStatus result ranked with the other inconclusive kinds. The fallback mapping and the inconclusive check are exhaustive over the kinds, and diagnostics still carry only the status class.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb8c22a731
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…nly APIs Android loads its framework org.json ahead of the library the contract module compiles against, and the framework has no JSONObject.similar, so the duplicate-record check would fail there with NoSuchMethodError. A small structural equality over keys, length, has, and opt replaces it: key order does not matter, numbers compare by value, and an explicit null differs from a missing key. The Android facade list includes the new file.
…t android.jar A JVM unit test reads every org.json member and class the packaged contract module references and requires each to exist in the org.json classes of the compileSdk android.jar. The Android plugin's boot classpath is passed to unit tests as a system property, so the check needs no SDK path guessing, and a missing member fails with the using class, member, and descriptor.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf88bb545f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… replacement A store now removes any invalidation marker for the entry before it publishes the new contract, so a marker always predates the entry it invalidates and a later load can no longer delete a fresh contract. When the marker cannot be removed, the contract is not written where the next load would discard it: the acquisition serves it from memory, reports cacheStore InvalidationNotCleared, and the shared JVM boundary records a bounded VERIFIED_CONTRACT_NOT_PERSISTED diagnostic.
…ages A request to the app's linked source repository that produces no response now throws ContractLinkedSourceNetworkException where the source is known, classified as LinkedSourceUnreachable instead of a generic network failure. It maps to a new fallback reason with its own copy, so an outage of the linked repository is no longer reported as the App Store being unreachable, and a failed linked source never produces no usable contract.
…ontract-acquisition # Conflicts: # website/public/screenshots/capture-manifest.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 708d3b3e60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…idation A store now deletes an invalidated cache entry first, clears its marker and process record only once the entry is gone, and writes the replacement last. A failed delete keeps the invalidation and writes nothing; a failed write or move after the delete leaves no invalidated entry, and a valid entry that cannot be replaced is deleted or invalidated. Process death at any step leaves either the invalidated entry with its marker or no invalidated entry. A store that throws is reported as Failed so the not-persisted diagnostic covers it.
…n fence A contract read from the persistent cache entered the in-memory cache unconditionally, so a cache-only read that a refresh superseded could replace the refreshed contract in memory. Promotion now runs under the fence with the reading acquisition's generation, is skipped once a newer result was accepted or the latest result says there is no usable contract, and never replaces a contract already in memory.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0c7d1473a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Catalog download, caching, and the refresh window move unchanged from the acquirer into AppStoreCatalogReader, so catalog publication can be ordered in one place.
A caller that read the clock before another refresh of the same catalog updated the window could finish later, move the window back, and start a second download in the same window.
An older catalog download could overwrite a newer catalog in the cache after a refresh, so the next automatic acquisition read the removed release again and published the obsolete contract. Catalog writes are now ordered per catalog URL by download generation; transient failures fence nothing. Contract results record the catalogs they were read from and are accepted only while those catalogs are current, so an acquisition that read a replaced catalog runs again, and a result read from the newer catalog outranks one read from the replaced catalog.
Outcome
Advances #493 and #473.
On Android, every dynamic app (Forms, Collectives, News, Tables, and others) showed "No verified native API" with a raw class name such as
AppOwnedOpenApiContractKt. The reported nightlies contained a top-level regex with an unescaped}inAppOwnedOpenApiContract.kt. Desktop Java accepts it, but Android's ICU engine rejects it, so the class initializer failed. The first attempt threwExceptionInInitializerErrorand every retry threwNoClassDefFoundError, which is why "Retry discovery" changed the message in #473. Commit e4fac17, already onmain, replaced that regex. This PR removes the remaining ways exception text reaches the user and gives each fallback a clear explanation.JvmAppStoreContractAcquisition.kt) maps acquired contracts once for Android and desktop, replacing duplicated mapping in both services. It catchesLinkageError, so class-load failures no longer escape to the app info screen. Cancellation and other errors still propagate.ui/src/androidMainand everyandroidApp/src/*source set except JVM unit tests (main,debug,release,directApk,androidTest), with self-test fixtures for each.uiAndroid target now depends on:contractAcquisition.DYNAMIC_APP_DESCRIPTOR.mdandPLATFORMS.mdare updated.Audit: all regex literals in Android-bound contract code were checked for ICU-incompatible syntax. The org.json, YAML, XML and
java.nioAPIs the module uses are available at minSdk 26. Nothing else was found.On a fixed build, Tables 1.0.10 gets its contract from its app-owned
openapi.json. News 28.7.0 has no OpenAPI file and gets verified read routes from its upstream routes and controllers. No endpoints were invented.Verification
bash tools/check-repository.shpasseschanges/unreleased/fragments record the changeRun on Windows with JDK 21:
:contractAcquisition:test: 91 tests pass, 5 skipped.:ui:desktopTest(DynamicContractFallbackDiscovery,JvmAppStoreContractAcquisition,DynamicNativeRuntime,DynamicDiscoveryDiagnostics,DynamicDiscoveryPersistence): 106 tests pass.:androidApp:testDebugUnitTest :androidApp:compileDebugAndroidTestKotlin: pass.:ui:desktopTest: 3780 tests, 1 failure inNoteEditorContentTest.shortEditorKeepsSaveVisibleAndDetailsPreserveDraftAndPreview. It does not touch changed code, passed in a later run on this branch, and passes onmain; treated as a flake but not proven.bash tools/check-kotlin-architecture.sh,bash tools/test-kotlin-architecture.sh,node tools/changelog-fragments.mjs validate: pass.New tests:
ContractAcquisitionFailureTest(12, including cancellation during verification and catalog cache write failures),JvmAppStoreContractAcquisitionTest(4),DynamicContractFallbackDiscoveryTest(6), and a new method inAppOwnedOpenApiAndroidInstrumentedTestthat initializes every top-level class in the contract module on Android.Not run: instrumented tests on an emulator or device (
AppOwnedOpenApiAndroidInstrumentedTest, andPublicBudgetContractInstrumentedTestwith-e publicBudgetProbe true),:androidApp:verifyReleaseLintGate(worth running because of the new dependency),:androidApp:assembleDebug,:ui:createDistributable.Compatibility and risk
PLATFORMS.mdkeeps its previous review date because the full inventory was not re-reviewed.Visual changes
The fallback card copy changes per reason. Screenshots for phone, tablet and desktop are not yet captured.