Repository navigation
fix: explain dynamic app contract fallbacks without exception text - #497
veryCrunchy wants to merge 68 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.
…ontract-acquisition
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f60743ce58
ℹ️ 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".
…ses one catalog The refresh window is shared per catalog URL across apps and versions, so a refresh can download one catalog while the other is refused. A refused catalog no longer ends the acquisition early: candidates from the catalog that was read are inspected first, and the refused catalog is uninspected evidence that ranks as RecentlyChecked after a refused linked source and before an unreadable catalog. A cached verified contract is still served when a limited refresh finds nothing newer.
…ontract-acquisition # Conflicts: # tools/kotlin-file-size-baseline.txt # website/public/screenshots/capture-manifest.json
A mechanical move of the metadata fallback notice out of NextcloudNativeApp.kt into DynamicContractFallbackCard.kt, with no behaviour change. The size baseline for NextcloudNativeApp.kt drops from 11006 to 11001, leaving room for the account settings action that follows.
…ck card The 401 fallback card now has an Open account settings action. A typed DynamicContractFallbackActions passes the dynamic app screen only the callbacks it may use; the card shows an action only when its callback exists, and the 401 state never offers the rediscovery retry. The action leaves the app for Settings through the inline-editor navigation guard and signs nothing out, so the account entry's Sign out and revoke access stays the person's choice. Other states keep Check again.
…ontract-acquisition # Conflicts: # tools/kotlin-file-size-baseline.txt
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21a58787f4
ℹ️ 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".
… 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".
| // A newly verified contract supersedes an earlier invalidation of the same entry. | ||
| clearInvalidation(target) |
There was a problem hiding this comment.
If an earlier removal created the .invalid marker and that marker still cannot be deleted, this store publishes the newly verified contract and then silently leaves the marker in place. A later cache load, including after a process restart, sees the marker and discard(file) deletes the fresh contract, so an offline launch loses a valid cached contract. Clear the tombstone before publishing the replacement, or treat marker-removal failure as a failed persistent store.
AGENTS.md reference: AGENTS.md:L442-L443
Useful? React with 👍 / 👎.
| DynamicContractFallbackReason.AppStoreUnreachable -> DynamicContractFallbackNotice( | ||
| "App Store not reachable", | ||
| "The Nextcloud App Store could not be reached to verify this app's API. " + | ||
| "Check the connection and try again.", |
There was a problem hiding this comment.
When the App Store catalog and signed package were reached successfully but fetchOptionalGitHubResource fails with an IOException while probing the linked tag, the shared classifier still maps that failure to AppStoreUnreachable. This notice then incorrectly tells the user that the Nextcloud App Store could not be reached; make the copy include the app's linked source, or preserve the failing source in the typed outcome.
AGENTS.md reference: AGENTS.md:L482-L484
Useful? React with 👍 / 👎.
This branch was successfully deployed
1 participant
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.