App intents: Siri, Spotlight and shortcuts from one declaration - #5559
Open
shai-almog wants to merge 137 commits into
Open
App intents: Siri, Spotlight and shortcuts from one declaration#5559shai-almog wants to merge 137 commits into
shai-almog wants to merge 137 commits into
Conversation
…codename1.intents)
Siri, Spotlight, the Shortcuts app, an Android launcher shortcut and a widget
button all ask an application the same question: what can you do, and will you
do it now? This models them as one concept. An application declares a capability
once and the framework projects it to each of them.
@AppIntent(value = "log_workout", title = "Log a workout",
phrases = {"Log a workout in ${applicationName}"}, headless = true)
public static IntentResult logWorkout(@IntentParam("minutes") int minutes) {
WorkoutStore.append(minutes);
return IntentResult.spoken("Logged " + minutes + " minutes.");
}
Three constraints shaped the design, and each of them fails silently if ignored:
Reflection does not exist on a translated iOS build. Class.getAnnotation returns
null there and Class.forName is banned in core, and the dead-code eliminator
strips any method with no Java caller. So the handler is a public static method
and the build generates a direct static call to it. Nothing is looked up at
runtime.
CHECKCAST is unchecked in ParparVM, so a bad cast hands the wrong object onward
instead of throwing. The generated parameter coercion therefore never casts a
payload value: every branch is instanceof-guarded, and an entity arrives as its
id and becomes an object only through its own BY_ID query.
Swift cannot call the translated Java. The eliminator scans only .m sources for
mangled symbols, so a method named solely from Swift becomes an empty stub with
no error anywhere. The App Intents declarations are consequently a thin Swift
shell that hands off to CN1IntentHost, an Objective-C shim; the behaviour stays
in Objective-C and Java.
Most of the feature is not Swift at all. Core Spotlight carries indexing and
NSUserActivity carries donation, both Objective-C and both long predating the
port's deployment floor -- so an application that only publishes content to
device search pays nothing, including no change to its deployment target. Only a
declared @AppIntent can contribute the App Intents floor, and two build hints
(ios.intents.minDeploymentTarget, ios.intents.appIntents) keep that decision with
the developer. A deployment target pinned below what a declared intent needs is
reported rather than silently raised or silently dropped.
Android gets launcher shortcuts, donation and genuinely headless execution -- the
last easier there than on iOS, since the port already boots without an Activity.
It does not get Siri: Android has no contract by which an assistant invokes a
capability and receives a typed result, so phrases, system disambiguation and
spoken results are documented as iOS-only and isVoiceInvocationSupported()
answers false rather than pretending.
Intents and @route stay separate because they answer different questions -- a
deep link has no return channel and cannot disambiguate. They meet only where an
intent opens the app, so opensRoute builds the URL from the bound parameters and
navigates through the existing route table. One screen, one route pattern.
Also fixes a live @route bug found on the way in: ProcessAnnotationsMojo gated
dispatch on class-level annotations alone, so the documented static-factory
@route form was silently dropped in real builds. The per-processor tests missed
it by calling processClass directly, bypassing the Mojo; the new test drives the
Mojo itself and fails without the fix.
Verified: 4853 core tests, 642 plugin tests, SpotBugs and PMD at zero over core,
no new cast-semantics baseline entries, and the processor run end to end against
the new sample -- which is how the nested-class emission bug (Outer$Inner is not
legal in generated source) was caught. The Xcode-side questions the plan records
still need a Mac: whether App Intents metadata is emitted below iOS 16, and the
build server's Swift version.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![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: 727474bb0f
ℹ️ 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".
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Four failures, all real and all mine. Android SpotBugs (SIC_INNER_SHOULD_BE_STATIC_ANON, x2). CN1IntentService held two anonymous inner classes. Both capture nothing from the service, so they are now named static nested classes -- which also stops them pinning the enclosing instance for the life of an invocation. I had compiled the Android port locally but not run its SpotBugs; both android and ios modules are clean now. Copyright headers on the two new Swift files. The checker requires the complete block header starting with /*, which Swift accepts; my // banner was judged partial. The existing surfaces Swift files carry no header at all and so are never checked, which is why they were not a useful model. Inline guide snippets. The developer guide forbids [source,java] blocks inline -- snippets have to live under a compiled source root and be included by tag, so they are compiled and cannot rot. The five blocks moved into AppIntentsJava00NSnippet.java and the chapter now includes them. They compile. Renamed com.codename1.intents.Entity to AppEntity, which is what compiling those snippets exposed: it collides with com.codename1.annotations.Entity (the ORM) under the wildcard imports both the guide scaffold and ordinary CN1 apps use. I had already named the annotation @IntentEntity to dodge that clash and missed the runtime type. Better found here than by an app that uses the ORM and intents together. Verified: 4853 core tests, 642 plugin tests, SpotBugs and PMD zero over core, android and ios, cast-semantics clean, copyright clean, all 662 guide snippet includes validated, the guide renders warning-free, and the demo snippets compile on JDK 17. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21f1a3ee92
ℹ️ 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".
Collaborator
Author
|
Compared 12 screenshots: 12 matched. |
All seven were correct. Fixed rather than argued with. P1 Any installed app could invoke any capability. The trampoline has to be exported, because the launcher is what starts a shortcut, and it accepted an arbitrary intent id with attacker-supplied parameters. Keeping the service unexported was no defence, since the activity proxied straight to it. It now refuses a caller that is neither this application nor a resolved home-screen launcher, and refuses silently so probing learns nothing from the difference between a rejected known id and a rejected unknown one. P1 NSUserActivityTypes was emitted inside the surfaces branch, so an app that declared intents without also enabling the widget extension got no plist keys and donation appeared to succeed while nothing was ever suggested. It has nothing to do with widgets; it is now emitted for any app that declares an intent. P1 The generated perform() turned every outcome into a dialog-only success. A failed handler made Shortcuts record success and run the following actions; values, entities and routes were discarded. It now throws CN1IntentFailure on failure and returns the value through ReturnsValue<String>. Navigation for an opens() result moved into the framework rather than the generated Swift, which fixes Android at the same time -- the route table is Java, and the platforms only know how to bring the app forward. P1 discoverable=false was ignored: isDiscoverable was emitted only when the unrelated destructive flag was set, and then always true. A donation-only capability was therefore listed in the Shortcuts catalogue before it had ever been donated. Now emitted from the flag it belongs to, and destructive gets what it actually wanted, a requestConfirmation() before the handler runs. P2 A cold-start headless shortcut visibly opened the app: the trampoline asked the declaration table which does not exist yet when a tap starts a dead process -- the common case. The headless flag now travels in the shortcut URI, so no runtime lookup is needed. P2 Non-required parameters were emitted as non-optional Swift types, so the system prompted for a value the handler was willing to do without and the Java-side default never applied. Optional now, and an absent value is left out of the payload entirely. P2 clearIndex only knew what this process had indexed, while shortcuts outlive the process -- so after a restart it removed nothing and every previously indexed item stayed visible. It now asks the platform what is actually published. The cast-semantics gate then caught two of the new methods holding implicit casts inside catch(Throwable); only the platform call is guarded now. Verified: 4855 core tests, 651 plugin tests, SpotBugs zero over core, android, ios and the plugin, PMD zero, cast-semantics clean, copyright clean, 662 guide snippet includes validated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14e4b1ac1c
ℹ️ 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".
Contributor
Cloudflare Preview
|
Contributor
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
One of the six was already fixed by the previous commit (destructive intents now call requestConfirmation before the handler runs). The rest were real. P1 The generated shortcuts.xml would have failed the Android build outright. android:shortcutShortLabel is defined as a resource reference, so AAPT rejects a literal during resource linking. Labels now go into a generated cn1_shortcuts_strings.xml and the shortcut references them. P1 A static shortcut for a parameterized intent invoked the handler with nulls. A static shortcut carries no parameters and Android has no picker on that path, so the log_workout example in our own sample would have logged a zero-minute workout of no kind. Only intents that can run as declared -- no required parameter, or a default for each -- become launcher shortcuts now, and the build says which were skipped and why. They stay invocable and donatable with values bound. P2 exposure was recorded in the Java declaration but omitted from intents.json, so both builders treated a MODEL-only intent as a normal one and published it to Siri and the launcher. It travels in the manifest now and both builders honour it. An absent list still means the default, so nothing built before the field existed changes behaviour. P2 A closed vocabulary was documented and never enforced: the generated dispatcher coerced whatever arrived and called the handler. It now rejects a value outside the declared options rather than quietly substituting the default, which would run the handler with something nobody asked for. P2 Registering a dynamic intent produced an id nothing could run. The dispatcher only knows build-time ids, so every registered parameterization failed as unknown. registerDynamicIntent now takes a DynamicIntent that names the declared intent it runs and the values it binds; invocation resolves to that base with the bound values underneath anything supplied at call time, so a binding is a default rather than a lock. A parameterization that names an undeclared base, or shadows a declared id, is refused instead of being advertised. The cast-semantics gate then caught the new lookup holding a for-each inside catch(Throwable) -- a CHECKCAST ParparVM expands to nothing, so the guard was protecting against something that cannot fire where it matters. Restructured. Verified: 4861 core tests, 658 plugin tests, SpotBugs zero over core, android, ios and the plugin, PMD zero, cast-semantics clean, copyright clean, 662 guide snippet includes validated, guide renders warning-free. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a529a528f
ℹ️ 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".
Vale reported 17 alerts and LanguageTool 2 matches on the new chapter, and both are zero-tolerance in CI. All in the chapter I added. Vale: contractions the Microsoft style wants (can't, that's, they're, it's, isn't), punctuation moved inside quotes, a capital after a heading colon, rhetorical questions rewritten out of the first person, and the adverbs "silently" and "quietly" removed -- which in two places meant saying the thing plainly instead, that no error is reported anywhere and that the build stops rather than acting behind your back. LanguageTool: "on screen" used as a predicate adjective, and "afterwards" where American English prefers "afterward". Verified locally: vale clean across all 114 guide files, LanguageTool 0 matches against the rendered HTML, asciidoctor renders warning-free, and all 662 snippet includes still validate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b803c071c
ℹ️ 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".
P1 The trampoline's caller check was itself spoofable. getReferrer() returns the
caller-supplied Intent.EXTRA_REFERRER in preference to anything the platform
attributes, so an attacker just claimed to be the launcher. Android offers no
reliable caller identity for startActivity at all -- getCallingPackage() is null
unless the caller used startActivityForResult, which a launcher does not.
So trust now comes from the URI rather than the caller. Shortcuts published at
runtime carry a nonce minted on first use into private preferences, which no
other app can read; those URIs may do anything. A URI without it -- a build-time
static shortcut, or a fabrication -- is held to exactly what the launcher already
offers: discoverable, not destructive, no unsatisfied required parameter. Its
parameters are dropped, so nothing can be injected. An attacker is bounded to
what the user could already do by tapping the app's own shortcut.
P1 Parameterized Siri phrases did not bind. Only ${applicationName} was
translated, so "${minutes}" stayed literal text: the phrase read the placeholder
back to the user and never supplied the argument. Parameters now become
\(.$name); a placeholder naming nothing is left alone rather than emitting an
interpolation that would not compile.
P1 Deleting the last declaration left the previous build's output in
target/classes to be packaged again -- publishing removed shortcuts and calling
handlers that no longer exist. The empty case now deletes what it used to write.
P2 The Android service capped every wait at 25 seconds, so an intent declaring
more had its runtime torn down mid-handler with nothing reported. The backstop
now derives from the declaration, plus a margin, since the framework already
enforces the real deadline.
P2 ios.intents.appIntents=false removed the donation implementation along with
the declarations, so NSClassFromString found nothing and donation silently did
nothing -- the opposite of what the option promises. The bridge and donation shim
are now generated either way.
P2 A required parameter with no default became null, 0 or false and the handler
ran anyway. It now fails before any side effect, including an entity id that
resolves to nothing, which is just as missing as an absent value.
P2 Concurrent Intents.index() calls could consume each other's staged
thumbnails. Stage-and-submit is now one transaction.
P2 A MODEL-only feature raised the iOS deployment floor even though it produces
no App Intent. The floor is decided from the intents that actually reach the
platform.
P2 A donated parameterization stopped working once the process died: the
shortcut outlives the runtime but the binding did not. Donation now resolves to
the base intent with the bound values merged in, so the shortcut carries
something the build-time registry can run.
SpotBugs then caught a SecureRandom constructed for a single use; it is held now.
Verified: 4861 core tests, 660 plugin tests, SpotBugs zero over core, android,
ios and the plugin, PMD zero, cast-semantics clean, copyright clean, vale clean
across 114 guide files, 662 snippet includes validated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dccbd002e4
ℹ️ 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".
Three of these are regressions the previous round's security fix introduced, which is a fair reminder that locking a door can wall in the people who live there. P1 Every build-time static shortcut merely opened the app. A static shortcut has no nonce -- there is no runtime at build time to mint one -- and on a cold start the declaration table does not exist either, so the new unauthenticated-request guard rejected the invocation before anything could queue it. A request that arrives before the declarations do is now parked and judged when they appear, using registerIntents as the hook, since that fires exactly when the generated dispatcher installs itself. P1 The "open" branch ignored the nonce entirely, so any installed app could foreground this one and drive its selection handler with an entity id of its choosing. Indexed shortcuts are always published at runtime and therefore always carry the nonce, so requiring it costs nothing and closes the hole. P1 The headless flag was taken from the URI even on an unauthenticated request, which let another app run a non-headless intent in a process with no window. It is now believed only from a URI this application published, and the declaration wins wherever it is available. P2 Donation was gated on iOS 16 even though NSUserActivity donation predates App Intents by years, so the ios.intents.appIntents=false opt-out kept the bridge and still donated nothing. Donation is now plain Objective-C with no availability gate and no Swift at all -- which removes the dependency rather than working around it, so the Swift donation shim is gone and the opt-out emits no Swift. P2 Images staged for a result were never released, so an app returning fresh imagery on each invocation retained every blob for the life of the process. The completion consumes them now, as indexing already did. Verified: 4861 core tests, 660 plugin tests, SpotBugs zero over core, android, ios and the plugin, PMD zero, cast-semantics clean, copyright clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52eb32ad38
ℹ️ 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".
P1 An iOS cold start dropped the activity. On the legacy lifecycle the platform delivers a donated NSUserActivity before the generated bootstrap runs, so the declaration lookup necessarily failed and the activity was reported unhandled -- tapping a donated shortcut from a dead process launched the app and did nothing. It is queued now and run once setDispatcher installs the table, then dropped if nothing turns out to declare it. Only ids shaped like ours are held: a handoff or third-party type is reverse-DNS and cannot match, so this claims nothing that belongs to somebody else. P2 donate() bypassed exposure. The static builders filter a MODEL-only intent out of the launcher and out of App Intents, but the runtime donation path published it anyway -- exposing exactly what it opted out of. Checked at the call now. P2 A dynamic id could contain a colon. Android identifies indexed entries by that separator, so donating a "call:mum" and then clearing the content index would disable the donated action along with real content. Dynamic ids are held to the same shape the build enforces for declared ones. Not fixed, and documented in the code where it lives rather than argued about: the iOS result value is reduced to text. ReturnsValue<T> fixes T in the struct's signature at build time, while a handler's value is IntentResult.value(Object) -- nothing in the declaration says what type it will be, and it can differ between two invocations of the same intent. ReturnsValue<String> is the only type-stable choice for every handler, and the alternative is returning no value at all. The wire document keeps the JSON type for consumers that care. Typing it properly needs the declaration to state a return type, which is a deliberate future addition rather than something derivable from what exists today. PMD then objected to a helper named like a constant; renamed. Verified: 4866 core tests, SpotBugs zero over core, android, ios and the plugin, PMD zero, cast-semantics clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9d596fb60
ℹ️ 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".
P2 Indexed entity thumbnails never reached Android. The serializer hands the bytes over in the images map -- the JSON carries only the name they were registered under -- and the bridge discarded it, so every indexed entry showed the application icon and a list of content was a column of identical squares. The blobs are matched by name now and become the shortcut's icon, falling back to the app icon only when an entity has none. P2 An optional Date ignored its declared default. Every other type honoured one, so a date silently arrived as null instead of the documented fallback -- most visibly on iOS, where an absent optional parameter is deliberately left out of the payload entirely. Verified: 660 plugin tests, SpotBugs zero over android and the plugin, cast-semantics clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 984e2b9cbd
ℹ️ 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".
…d snippets P2 Snippets were documented and silently thrown away. Java serialized the node tree and its images; the generated Swift returned only the value and the spoken line, and nothing ever rendered one. They render now, and by reusing the surfaces renderer rather than growing a second layout vocabulary -- a snippet is a small layout shown while the app may be off screen, which is exactly what a widget is, so cn1RenderNode already knew how. The renderer is written under the same filenames the surfaces builder uses, so an app with both features gets one copy rather than two conflicting declarations. The images reach it as files in a per-invocation cache directory, which is how the widget extension resolves them too. P2 A declared opensRoute template was never expanded. The framework navigated only when the handler returned an explicit URL, so an intent that returned ok() foregrounded the app onto whatever screen it was already showing -- which is the one thing opensRoute exists to prevent. Expanded from the values the intent ran with; a placeholder with no value navigates nowhere rather than routing somewhere unintended. P2 With ios.intents.appIntents=false the plist keys were omitted, so donation ran and iOS had no declaration for the activity types it produced. Emitted for that configuration too. P2 iOS donation did not resolve a parameterization, so a suggestion outlived the process while its binding did not and failed as unknown after a restart. It now resolves to the base intent with the bindings merged, as Android already did. Verified: 4868 core tests, SpotBugs zero over core, android, ios and the plugin, PMD zero, cast-semantics clean, copyright clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb4107af9a
ℹ️ 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".
…omise P1 CN1IntentBridge.swift would not compile on an App Intents build. It uses CustomLocalizedStringResourceConvertible, which AppIntents declares, but only imported Foundation -- canImport tests availability without importing, and the import in the separately generated CN1AppIntents.swift is file-scoped. P1 asTools() was documented in three places and did not exist, so an application following the Exposure.MODEL documentation would fail to compile. It is implemented now: model-exposed intents project down to com.codename1.ai.Tool with a JSON schema built from their declared parameters, and a handler that runs the intent and returns its serialized result. Entity parameters are described as the id string that actually crosses the boundary. The projection stays one-way and opt-in -- a model calls a capability because it inferred it should, which is not the same trust as a person asking for it by name. P2 Route template values were substituted verbatim, so an id containing "/" added a path segment and stopped matching the route it was built for, and "?" or "#" could invent a query or a fragment. Encoded now: the value is data, the template is structure. P2 The Android thumbnail map was staged on the bridge instance, so two threads indexing at once could have the second overwrite it between the first parsing its JSON and reading its blobs -- publishing the other request's thumbnails or none. Carried through as a parameter, the same fix the iOS staging race got. PMD then wanted @OverRide on the new handler. Verified: 4872 core tests, SpotBugs zero over core, android, ios and the plugin, PMD zero, cast-semantics clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 860ef935d1
ℹ️ 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".
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
…they never owed A real Xcode toolchain turned four assumptions into measurements. The deployment target was the big one. The design carried a third tier -- an app declaring an @AppIntent pays the App Intents floor -- as a fallback in case Xcode refused to extract metadata below it. It does not refuse. A target built at IPHONEOS_DEPLOYMENT_TARGET = 13.0 with availability-fenced declarations emits a complete Metadata.appintents carrying every intent and entity. So the tier is gone: ios.intents.minDeploymentTarget now defaults to contributing nothing, no app's minimum moves, and the intents are simply not offered on devices too old to run them. The pin conflict it used to raise is now only reachable when a project deliberately asks for the floor and deliberately pins below it. Three compile failures in the generated Swift, each of which would have shipped: - A snippet result needs `import SwiftUI` as well as `import AppIntents`. The .result(value:dialog:view:) overload lives in the _AppIntents_SwiftUI cross-import overlay, which only activates when both modules are imported; without it the call is "extra argument 'view'". - A phrase parameter interpolates as \(\.$name), a key path. AppShortcutPhraseToken itself carries only .applicationName; the parameter interpolation is declared over KeyPath<Intent, IntentParameter<Value>>. - CN1SurfaceRenderer.swift is now compiled into the app target too, which can deploy far below the extension's 16.1. Its ProgressView / Link / Text(_:style:) uses are iOS 14, so the functions carry @available(iOS 14.0, *), and an app that declares an intent but publishes no widgets gets a stub CN1SurfaceConfig.swift rather than an unresolved cn1SurfacesAppGroup. And Apple enforces two phrase rules the processor did not: at most one parameter per phrase, and that parameter must be an entity. Both are halting errors in appintentsmetadataprocessor that produce no metadata at all -- the app builds, ships, and has no intents. The processor now reports them against the declaration that caused them, along with phrases on a non-discoverable intent. The sample and the guide carried an invalid two-parameter phrase; they no longer do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd4ca73313
ℹ️ 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".
Two different launcher shortcuts tapped before the declaration table loaded left only the second: parkUntrustedRequest cleared the queue on every call. The first tap's wake started the runtime and ran the surviving request, and its own wake then found the runtime already up and returned with nothing to do, so one user action disappeared with nothing logged. The comment justified the clear as "the user pressing twice", which is true of a repeated id and false of two different ones. Repeats of the same id still collapse to one run. Distinct ids are all kept, bounded at eight because the queue is filled from an exported entry point, and registerIntents now drains all of them. That turns the service handshake into a counting problem: parkedFinished was a single flag, so the first handler to finish told CN1IntentService every handler was done and the runtime could be torn down under the others. It is a count of outstanding invocations now, and the budget the service waits out is the longest of the declarations rather than the last one dispatched. Verified reflectively against the compiled class: two distinct taps plus a repeat leave exactly the two ids, and twenty more leave eight. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
resolveBridge reached through Display for the bridge, and on the earliest cold-start path -- before Display.init, which this feature deliberately supports -- that dereferences an implementation which is still null. The throw was caught, and then Log.e was called from the handler: it routes through the same missing implementation, so it can throw on its own, and that second throw escapes resolveBridge and completeOrQueue with it. The answer is never queued, the Swift continuation is never resumed, and the assistant waits forever on an intent that has already finished. Nothing reports it, because the reporting is what failed. Two changes. The dereference is not attempted before Display is initialized -- that is a supported moment, not an error, and returning null lets the caller queue the result the way it already knows how. And the logging inside the handler is itself guarded, because a lost log line costs incomparably less than a hung assistant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The declared ids are recorded so a cold start can recognise its own activity types before the dispatcher exists, and they are rewritten on every publication. An update that removes the last @AppIntent while still using the package to index content publishes nothing, and the processor has deleted the bootstrap by then -- so nothing rewrites the record and the previous version's ids survive. They were still being claimed. No dispatcher would ever arrive to drain the queue, so a stale suggestion opened the app and did nothing, and an application continuation that reused one of those ids was swallowed permanently. The proof that the record is stale is already written in the method: the bootstrap installs the dispatcher from the stub's main, before Display.init. So a Display that is up with no dispatcher means this build declares nothing at all, and anything the record still names belongs to a version that is gone. The claim is refused and the record dropped, outside the lock because it writes to storage. The pre-Display window is untouched, which is the one case the queue exists for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…e generated half The sample declared no @AppIntent, so CI never built the half of this feature that the build writes. No declaration means IPhoneBuilder emits no Swift at all -- CN1AppIntents, CN1AppEntities, the AppShortcutsProvider and the CN1IntentHost shim were never handed to Xcode -- and AndroidGradleBuilder writes no cn1_shortcuts.xml for AAPT to accept or reject. Every generated-code defect found in review over the last day was the kind a compiler reports in a second, and none of them could have been caught here. The declarations are chosen for the shapes that generate differently rather than for realism: a closed vocabulary (a Swift AppEnum), an optional parameter with a default, an entity with all three queries (an AppEntity plus its EntityQuery, and the key-path phrase interpolation), a destructive intent (the confirmation branch), and a parameterless one -- the only shape Android publishes as a static launcher shortcut, which is what gets the resource path compiled. Its title carries an apostrophe on purpose, so AAPT itself proves the string-resource escaping rather than a unit test's idea of it. IntentsApiTest covers the runtime half on the device VM: the generated registry, the coercion around every parameter, the declared default, a value outside the closed vocabulary, and entity resolution behind an id -- including an id that resolves to nothing. Coercion under a VM whose CHECKCAST is unchecked is not the same code path as coercion in a JUnit suite, which is the reason it belongs here. The test reports whether the generated dispatcher is installed rather than assuming it. It is installed by the bootstrap the device builders splice into the stub, so the table is populated on iOS and Android and empty on ports with no such splice; the dispatch assertions run where there is something to dispatch, and say so in the log where there is not. Verified before pushing rather than by watching CI: the processor accepts the set and generates a 4-intent, 1-entity registry; the Swift it produces type-checks against the iOS 26.2 SDK at -target arm64-apple-ios13.0 with the bridging header and the shared renderer; the Android resources come out with formatted="false", the escaped apostrophe, a resolved package and exactly one shortcut. That type-check also found the last defect fixed here: an intent with no parameters emitted `var params` that is never mutated, which is a Swift warning in a file the developer never wrote and cannot fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Adding a test to the device suite is only half of adding it: the contract requires every registered test to belong to a feature, and three separate jobs enforce that. All three failed on the same sentence -- "Registered tests without a feature: IntentsApiTest" -- from the contract validator, its own unit tests, and the website build, which normalizes every port report through the same code. So the test now has a feature, and each checked-in port report carries it as "not-run", which is what it is: this test has never run on any port, and the first real run of each suite replaces the entry. Their summaries are recomputed from the results rather than edited by hand, because the validator compares the two. The contract's own test asserts the registered-test count, which moves from 171 to 172. Also adds the licence header to UIManagerLargeTextScaleTest, which arrived from master in the merge and has none. It fails the header gate inside this PR's diff range whoever wrote it, and the gate is right: the file is missing a header the repo requires. Verified with the two entry points CI uses: `port_status.py validate` reports a valid contract of 172 tests across 58 features and 11 ports, `accept` takes a checked-in report, and the contract's 36 unit tests pass -- which is one better than the branch was before, since the run had a pre-existing error that this mapping resolves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
The test asserted everything correctly and then hung: an assertion-only test has to call done(), and mine returned true without it. The runner waited out the full budget, retried once, waited again, and reported "timeout waiting for DONE stage=retry-created" -- which is the runner being right about a test that never said it had finished. Sixty-three other tests in this suite call done(); this one did not. The catch now calls fail() as well, for the same reason in the other direction: a test that throws and merely returns false is reported as hung rather than as failed, which hides what actually went wrong behind a timeout. The run that caught this is worth recording, because it is the coverage working. Android built the APK with the generated cn1_shortcuts.xml and cn1_shortcuts_strings.xml, AAPT accepted both, the app installed and started, and the log shows the generated registry present on the device with all four declarations: "supported=true headless=true voice=false indexing=true declarations=4". Every dispatch assertion ran against real generated code on a real emulator -- coercion, the declared default, the closed vocabulary and entity resolution -- and the two deliberate rejections came back as failed results with the framework logging the cause, exactly as intended. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
The iOS device build failed compiling the Swift this feature generates: "CN1IntentBridge.swift:70:9: error: cannot find 'CN1IntentHost' in scope", three times over. CN1IntentHost.h was staged next to the Swift and nothing ever imported it. Objective-C declarations in the same target are not automatically visible to Swift -- they have to come through the bridging header -- and that header was written containing a single comment and never added to. The same sweep that hands the generated Swift to the app target globs only '*.swift', so CN1IntentHost.m was staged, would now be imported, and still never compiled: an undefined symbol at link rather than a missing type at compile. It is named explicitly rather than globbing '*.m', because that directory is full of translated Objective-C the project already lists. This was reported in review and I marked it resolved on the strength of a grep that matched the bridging header's *existence* rather than an import of this file. It took an actual device build to disprove that, which is the coverage the sample's new declarations bought: before them no CI job compiled a line of this Swift. Verified by reproducing the failure and the fix outside Xcode: with the bridging header as the builder used to write it, swiftc reports exactly the CI error at exactly line 70; with the line this now appends, the whole generated set type-checks against the iOS 26.2 SDK at -target arm64-apple-ios13.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The Linux half of the Windows cross-build sat in its first step for a full hour and was cancelled having installed nothing; the screenshot-comment job then failed downstream because the artifact it downloads was never produced. The visible failure was "Artifact not found for name: windows-cross-screenshot-raw", which says nothing about the hour spent inside apt. Every network call in that step is now bounded and retried. scripts/ci/apt-get-install.sh already exists for exactly this -- its header documents the same hang taking out archetype-smoke on 2026-08-18 -- and this workflow had simply never adopted it; it wraps the install in timeout 600 and retries, and skips the round trip when the packages are already present. apt.llvm.org is a single host with no mirror and llvm.sh both adds its repo and installs from it, so that call gets an explicit timeout as well as retries: retry alone does not help against a download that never returns. Both trigger blocks now list the three CI shell helpers this step runs through, so a change to one is exercised here rather than merged untested -- the same reason every other consumer lists them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The Mac Catalyst build failed to compile the delegate:
error: call to undeclared function
'com_codename1_impl_ios_IOSIntentCallbacks_nativeSpotlightItemSelected___java_lang_String'
error: call to undeclared function
'com_codename1_impl_ios_IOSIntentCallbacks_nativeUserActivity___...'
Both branches call translated Java entry points and nothing imported the header that
declares them. The iOS slice accepted that as an implicit declaration and Catalyst does
not, so the same source compiled on one Apple platform and not the other -- and the
platform that rejected it is the one whose job runs the whole suite through xcodebuild.
The class is translated in that build: com_codename1_impl_ios_IOSIntentCallbacks.m is
compiled there, and nothing referenced its header at all, which is the shape of the bug.
The import sits inside the CN1_USE_INTENTS guard because CodenameOne_GLViewController.h
undefines that for watchOS and tvOS, where the class is not translated and the header does
not exist.
Found by the sample declaring intents. Before that, no CI job compiled these branches on
any platform, because CN1_USE_INTENTS is only defined for an app that references the
package and no app in this repository did.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…ging My previous attempt bounded every network call in that step except the first one, and the first one is where it hangs. The job sat for 59 minutes after "Get:5 noble-security InRelease" and was cancelled having installed nothing, for the second time in a day, each time surfacing downstream as "Artifact not found for name: windows-cross-screenshot-raw" -- a message about a job that never ran. Acquire::Retries does not help here: it covers a request that *fails*, and a mirror that accepts the connection and then stops sending never fails, so apt waits on it forever. The per-request Timeout options bound each transfer, the outer `timeout` bounds the whole run in case they do not, and a second attempt costs little because a stalled mirror is usually a different mirror next time while a genuinely broken source fails the same way twice. The step also carries timeout-minutes: 20 now. Adding up the inner bounds gave a worst case of 97 minutes against a 60-minute job, which is not a bound at all -- and retuning the shared apt-get-install.sh to fix that arithmetic would change four other workflows. A step cap is the honest control: it cannot compound, it leaves the build the budget it needs, and it attributes the failure to the step that stalled rather than to a cancelled job. The llvm.sh allowance drops from 3x900s to 2x600s for the same reason. Every consumer of apt-get-update.sh gets the bound, which is the point of it being shared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The bound added earlier worked -- the website build reported "first attempt failed or stalled; retrying once" and gave up after ten minutes instead of hanging for an hour -- and then failed the job anyway with exit 124. That is a better failure, but it is still a red PR that no change in this repository can fix: archive.ubuntu.com stalled mid-fetch on two consecutive attempts, the runner's own azure mirror having been Ign'd, and every job that starts by refreshing apt would have failed for as long as that lasted. Refreshing the index is not what any of these jobs are here to do. The runner images ship with a populated index, so an install of ordinary packages usually succeeds from it, and apt-get-install.sh is bounded and exits non-zero naming the package it could not fetch. The gate is the install, not the refresh -- which is what apt-get-install.sh already assumed, since it has always invoked this script with `|| true`. The standalone callers now agree with it. Verified both halves rather than reasoned about them: with sudo forced to fail, this script exits 0 and prints the warning chain, and apt-get-install.sh still exits 1 with "could not install: <package>". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The cross-build's step cap fired 20 minutes in, and the log shows why: it reached add-apt-repository at 21:42 having started at 21:24. Eighteen minutes went on fetching the package index, and the install never got a fair attempt. That was my own compounding. apt-get-install.sh refreshes inside its three-attempt retry loop, which cost nothing while the refresh was unbounded and instant-failing -- and once I made the refresh bounded and retried, each of those three attempts could spend the whole refresh budget before trying to install. The refresh now happens once, before the loop. A stale index is not what makes an install fail three times in a row; an unreachable archive is, and refetching it three times does not make it reachable. The cross-build step also stops refreshing explicitly, because the helper it calls next already does. Verified: with sudo forced to fail, the refresh now runs once rather than three times, and apt-get-install.sh still exits 1 with "could not install: <package>". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Mac Catalyst rejected the shim with "call to undeclared function 'com_codename1_impl_ios_IOSIntentCallbacks_nativeQueryEntities___java_lang_String_java_lang_String_java_lang_String'". The file already imported the callbacks header. What it lacked was xmlvm.h, which is what defines NEW_CODENAME_ONE_VM -- so the #else branch compiled, naming the old-VM spelling without the _R_<returnType> suffix that the translator emits for a method returning one, and the header declares only the suffixed form. The iOS slice let that through as an implicit declaration and Catalyst does not, so the same file compiled on one Apple platform and not the other. Every other native in this port includes xmlvm.h first for exactly this reason; this one was written without it. This is the third defect of the same shape found by the sample declaring intents -- a native call that no CI job had ever compiled, because CN1_USE_INTENTS is only defined for an app that references the package and none did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Declaring an intent broke the tvOS build with 48 errors: every App Intents type needs tvOS 16 and the slice's floor is 13, so CN1AppIntents.swift and CN1AppEntities.swift could not compile. They reach that target because they are staged into <Main>-src for the iOS app target and this builder copies that directory wholesale. Excluded rather than availability-widened, because the feature is already compiled out here: CodenameOne_GLViewController.h undefines CN1_USE_INTENTS for TARGET_OS_TV, so the natives behind these declarations are unsupported stubs on this slice. Widening the annotations would have produced a tvOS app advertising actions that cannot run, which is worse than one that offers none. The surfaces renderer travels with them because a snippet reuses it -- GraphicsContext needs tvOS 15 -- and the tvOS slice built fine without it until intents started staging it. The watch slice was already fine: it compiles a specific file list rather than the whole directory, so it never picked these up. This is the same review finding I resolved earlier on a grep that matched the words "watch" and "tv" in an unrelated comment; the tvOS half was never actually implemented, and only a build with a declared intent could say so -- build-ios-tv passed on this branch until the sample declared one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
1 participant
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.
Siri, Spotlight, the Shortcuts app, an Android launcher shortcut and a widget button all ask an application the same question: what can you do, and will you do it now? This models them as one concept under
com.codename1.intents. You declare a capability once and the framework projects it to each of them.There was no existing support for any of this —
NSSiriUsageDescriptiononly ever fell out of the genericios.NS*UsageDescriptionmechanism, and Android had none at all.Three constraints shaped the design
Each of them fails silently if ignored, which is why they drove the shape rather than being worked around:
Reflection does not exist on a translated iOS build.
Class.getAnnotationreturns null there,Class.forNameis a compile error in core, and the dead-code eliminator strips any method with no Java caller. So a handler is apublic staticmethod and the build generates a direct static call to it. Nothing is looked up at runtime.CHECKCASTis unchecked in ParparVM, so a bad cast hands the wrong object onward instead of throwing. The generated parameter coercion therefore never casts a payload value — every branch isinstanceof-guarded, and an entity arrives as its id and becomes an object only through its ownBY_IDquery.Swift cannot call the translated Java.
Parser.readNativeFilesscans only*.mfor mangled symbols, so a Java method named solely from Swift becomes an empty stub with no error anywhere. The App Intents declarations are consequently a thin Swift shell that hands off toCN1IntentHost, an Objective-C shim; the behaviour stays in Objective-C and Java.Most of this is not Swift at all
Core Spotlight carries indexing and
NSUserActivitycarries donation — both Objective-C, both long predating the port's deployment floor. So an app that only publishes content to device search pays nothing, including no change to its deployment target.Only a declared
@AppIntentcan contribute the App Intents floor, and even that is a fallback: every generated Swift type is availability-guarded. Two build hints keep the decision with the developer:ios.intents.minDeploymentTargetios.intents.appIntents=falseA deployment target pinned below what a declared intent needs is reported rather than silently raised or silently dropped.
Android is not Siri parity, and doesn't claim to be
Android has no contract by which an assistant invokes a capability and receives a typed result. So phrases, system disambiguation and spoken results are documented as iOS-only and
isVoiceInvocationSupported()answersfalse. What Android does get is real: launcher shortcuts, donation, and genuinely headless execution — easier there than on iOS, since the port already boots without an Activity.Intents and
@Routestay separateThey answer different questions: a deep link has no return channel and cannot disambiguate. They meet only where an intent opens the app, so
opensRoutebuilds the URL from the bound parameters and navigates through the existing route table. One screen, one route pattern, two ways in. Nothing in the routing layer changed.A live
@Routebug fixed on the way inProcessAnnotationsMojogated dispatch on class-level annotations alone, so the documented static-factory@Routeform was silently dropped in real builds. The per-processor tests missed it by callingprocessClassdirectly, bypassing the Mojo. The new test drives the Mojo itself and fails without the fix.Verification
check-cast-semanticsbaseline entriesOuter$Inneris not legal in generated source; a nested entity is the ordinary case)What still needs a Mac
The plan records these as spikes and they are unchanged by this PR:
IPHONEOS_DEPLOYMENT_TARGET = 16? If yes, no app ever changes floor and the third tier above is dropped entirely.xcodeVersion >= 9.The Objective-C/Java halves are verified locally; the Swift and native compilation are not, and cannot be here.
Companion PR
The builder changes are mirrored to BuildDaemon, without which cloud builds would silently ship without intents while the docs promise them.
🤖 Generated with Claude Code