First-class call management, VoIP and VPN - #5604
Open
shai-almog wants to merge 184 commits into
Open
Conversation
Two families the framework has never had. `com.codename1.call` makes a call this app carries look, to the OS and to the user, like a call the phone placed: the lock-screen UI, the ringtone while the app is not running, the system call log, the audio handoff. `com.codename1.vpn` installs and controls a VPN configuration the platform runs. Neither carries payload. There is no codec here and no packet forwarding -- those are the app's, and what was actually missing was everything around them. A Codename One app could not ring at all while backgrounded, which is why it could not be a calling app however good its audio was. Both follow the com.codename1.nearby template: a public package plus an spi bridge of primitives only, asynchrony by caller-allocated request id, and a null bridge from CodenameOneImplementation as the capability query. The sub-packages are the opt-in, because the build scans bytecode for a class-name prefix and cannot express an exclusion. `.voip` costs the iOS voip background mode, which Apple rejects an app for carrying without a working call implementation; `.directory` is deliberately NOT a superset of `.session`, because an app that only labels somebody else's caller must not carry MANAGE_OWN_CALLS. Two shapes here exist because leaving them out is expensive later: CallAction, because both platforms kill a call whose action goes unanswered within a few seconds -- so an ignored action is fulfilled rather than dropped; and CallActionListener.providerReset, because an app that does not implement it leaks its media engine and shows calls that are not there. CallId is uppercase-canonical rather than case-insensitive: the identifier is compared as a string on every hop including by servers that never saw this API, and "compare case-insensitively everywhere" only has to be forgotten once. Character.forDigit is not on the device, so the hex table is spelled out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LocalCallBridge and LocalVpnBridge are what the simulator, the desktop builds, the browser port and the unit tests run against. They are simulations, not stubs, and the distinction is the whole point. A stub that answered everything inline and instantly would hide every ordering an app actually has to survive, until the first device build. So: nothing completes in the caller's stack frame; the audio session arrives well after the answer rather than with it, because that gap is where media bugs live; a call reported before anyone was listening is queued and drained later, which is the iOS cold-start path made testable without iOS; and connecting a VPN passes through CONNECTING before CONNECTED. Two refusals are modelled deliberately because they are the ones that are silent on a real device and therefore untestable there. Reporting a call before Calls.configure() fails here, where on Android the platform ignores the call and says nothing. And a loaded VPN profile comes back with no password, because both platforms keep the secret in their own keychain -- a simulation that handed it back would let an app depend on something no device does. setDeferred lets a test hold every scheduled answer and release it by hand, so the delayed ordering these exist to reproduce can be reproduced in a unit test rather than only observed. Every delivery is a named static class. An anonymous one would hold a synthetic reference to the bridge, which SpotBugs reports and which keeps a finished call alive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
72 tests across the house quartet -- Local*Test against the simulation, *DegradationTest for a port that implements nothing, *WireTest for the codec several ports implement by hand, and a *Await helper because AsyncResource has no getError(). The ones worth reading are about orderings rather than return values: answering arrives strictly before the audio session, a provider reset clears every session BEFORE telling anyone (a listener that iterates getSessions() there must not see calls the system already destroyed), an action nobody deferred is fulfilled rather than dropped, and the pending-push queue drains exactly once so a second listener cannot replay a call the user already saw. Four of these were verified by breaking the code and watching them fail -- the auto-fulfill, the reset ordering, the password stripping, and the drain. A test that has never failed is not yet evidence of anything. The analysers found nine real defects in the previous two commits, all fixed rather than suppressed: - CallAction.isAnswered read `answered` unsynchronized while the writer held the lock. The safety net and the application answer from different threads, so a stale false would have sent a second answer for the same token. - Calls.ActionEvent carried a `spare` field nothing read -- dead weight on every inbound event. - LocalCallBridge stored six fields of call state it never read. The fix was to expose them, not delete them: getSimulatedState and friends let a test assert that the facade and the simulated platform AGREE, which is exactly what stops being true when a report is refused and the app keeps its session anyway. Three new tests do that. - VpnWire decoded base64 through String.getBytes(), whose answer depends on the platform default encoding. The alphabet is ASCII; it is spelled out. The remaining 87 were MissingOverride and ForLoopCanBeForeach. Note a clean -source 1.6 build is what proves each @OverRide is on a real override: 26 of them were on this simulation's own test-control methods and the compiler rejected every one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JavaSEPort returns the simulated bridges, and the Simulate menu can script them, via the declarative simulator-hooks manifest rather than hand-written Swing. The hooks are chosen to reproduce traps, because a menu item for "everything works" is worth nothing -- running the app already does that. What is worth a click is a second call arriving while the first is up, the system refusing to ring at all, a VPN prompt the user declines, and credentials refused only after the profile installed cleanly. The most useful of them is "Never Activate The Audio Session". An app that starts media when the user answers, rather than when the audio session activates, behaves correctly on every simulator and is silent on a device, because CallKit owns the session. Withholding it here is the only cheap way to find that mistake, so LocalCallBridge grew a switch for it and both audio activation paths now route through one guarded helper. LocalVpnBridge.setStatus becomes public for the same reason: a tunnel dropping underneath a running app is a state the app must handle and cannot otherwise be produced on a desktop. Verified: ant core clean, both hook classes and JavaSEPort compile against the port's real classpath, and the full core-unittests suite is 5566 tests green. (The javase module's own maven build fails offline while downloading skins, which predates this change.) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five entries, one per leaf package, so an app pays for what it referenced and nothing else. The split is the point and six new tests pin it: owning a call buys CallKit but not PushKit, because the voip background mode rides along with PushKit and Apple rejects an app that carries it without a working call implementation. The directory entry is deliberately NOT a superset -- a caller-ID app never owns a call, so it must not acquire AVFoundation, a microphone string, or MANAGE_OWN_CALLS. Since the table matches on startsWith and cannot express an exclusion, separate packages are the only way to say that, and the test fails if they ever collapse. com.codename1.call itself stays unregistered, so touching a CallHandle costs nothing. Two floors are deliberate and two absences are: session and voip take minSdk 26 because a self-managed ConnectionService arrives exactly there and has nothing to degrade to below it, while managed VPN takes no floor at all -- VpnManager is API 30 but is reached reflectively, so the port reports the capability absent rather than the whole app refusing to install. No entry injects an entitlement, and a test enforces that. Both VPN entitlements are single-element arrays that the ios.entitlements.<key> namespace cannot encode, and the packet-tunnel one must never be injected automatically: Apple grants it case by case, and an App ID that lacks it fails codesigning with an error naming the entitlement and not the reason it appeared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A self-managed ConnectionService, a Connection, a CallScreeningService and a reflective VpnManager bridge. CN1ConnectionService ships in the port rather than being generated at build time. CN1FirebaseMessagingService is a template because it must extend a class from a Gradle dependency the port cannot compile against; android .telecom is part of the platform and compiles here, so that reason does not apply -- and several hundred lines written as a string template would get no compiler, no unit test and no SpotBugs, so every mistake in it would surface only on the build server. It costs nothing to ship: Telecom instantiates a ConnectionService only after a PhoneAccount is registered, which happens inside Calls.configure, so an app that never calls that never loads the class and R8 strips it. The refusal paths are the ones that matter. Telecom refuses a self-managed call during an emergency call, when another app holds one, or when the user has switched this app's calling off, and it does it by calling onCreate*ConnectionFailed. Leaving those unwired produces a request that never answers, which looks exactly like a call that is still ringing. Worse is the unregistered account: add新建IncomingCall then does nothing at all -- no exception, no log, no call -- so the bridge checks first and answers CALL_REFUSED, which is considerably more use than the platform's silence. Android has no audio-session handoff, so CN1Connection synthesizes one when the connection goes active. Otherwise "start media when the audio session arrives" would be iOS-only advice and the Android path would be the untested one. VpnManager and Ikev2Vpn个人资料 (API 30) and RoleManager (API 29) are all absent from the android.jar this port compiles against, so all three are reflective. Naming them directly would mean raising the SDK the whole port builds against, and raising an app's minSdkVersion to 30 for a feature it can report absent would cost far more than the feature is worth. Android's managed profile offers IKEv2 only, so a profile asking for IPSEC is refused rather than quietly installed as something else. Every reflective result is narrowed in its own method outside the try. ParparVM does not check CHECKCAST, so a cast that fails there does not throw and cannot be caught, and check-cast-semantics.sh reports exactly that shape. Three such casts were introduced and all three are gone; the one finding that remains, Display.invokeAndBlock, reports identically on clean master. REC_CATCH_EXCEPTION on the reflective catches is excluded with its reasoning: reflection throws six checked exceptions that all mean "this device does not have the API", and at source level 6 there is no multi-catch, so the alternative is six identical blocks at each of three sites. Verified: android port compiles, SpotBugs 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Scanner flags, manifest fragments, native defines, frameworks, entitlements and the voip background mode, all keyed on which package the bytecode actually referenced. CallManifestFragments is a pure helper for the reason the nearby and bluetooth ones are: the catalog can name a permission but cannot qualify it and cannot emit a <service> at all, and a self-managed ConnectionService needs one carrying its own permission attribute and intent filter. Eleven tests pin it, including the one that matters -- that the directory package alone earns neither MANAGE_OWN_CALLS nor RECORD_AUDIO, because an app that labels somebody else's caller never owns a call and Play Console flags gratuitous telephony permissions. enableNearbyDefine is generalised to enableFeatureDefine rather than copied. Its guarantee is the whole point: replaceInFile is a String.replace, so a marker that is absent is a silent no-op and the build would finish with the native compiled out -- the app ships, the API reports itself unsupported, and nothing says why. The packet-tunnel entitlement is deliberately NOT injected. Apple grants com.apple.developer.networking.networkextension case by case, and an App ID without it fails codesigning with an error naming the entitlement and not the reason it appeared, so the build fails early with a message that says what to enable instead of producing that. Two things found while wiring this rather than by intent: A helper I wrote first published the call provider's identity as ios.plistInject.<key>, a namespace nothing anywhere reads -- the same "writes an arg no consumer reads" shape that previously shipped android.health.privacyPolicyUrl and the ios.entitlements namespace as dead code. It now appends to the Info.plist injection fragment the assembly actually consumes, next to CN1ShareAppGroup. WatchNativeBuilderTest failed on the commit that added the frameworks, which is exactly what that guard exists for. CallKit and PushKit are both present on the watchOS SDK (checked, not guessed), but the watch slice never references them, so they join the weak-link list. 网络Extension was already classified. escapeNearbyPlistText is renamed escapePlistText: the call block needs the same escaping, and a second copy of three replaces is a second place to forget one. Verified: 1355 plugin tests green, SpotBugs unchanged at its 2 pre-existing findings in AbstractCN1Mojo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two chapters, seven compiled snippets, and the index entries, following the Nearby-Devices skeleton: a capability matrix, "branch on the capability queries rather than on platform detection", the honest limitations up front, a developing-without-hardware section and a build-hints table. The limitations are the part worth writing. Call-Management leads with four of them, because each is a mistake that is invisible until a device: start media when the audio session activates rather than when the user answers, or the call is silent with no error anywhere; a pushed call is ALREADY ringing, so the job is to attach media to a call that exists rather than to decide whether to ring; implement providerReset or leak the media engine; and Codename One carries no voice at all, which is a scope statement rather than an apology. The VPN chapter opens by separating the two things that share the name, because conflating them is what wastes the time: asking the platform to run a standard IKEv2 tunnel is portable and self-serve, while shipping a tunnel of your own means a separate iOS process with no JVM in it and an entitlement Apple grants case by case. It also points readers who only want to DETECT a VPN at 网络Manager.isVPNActive(), which has answered that for years and needs nothing from this chapter. Both note what cannot be worked around: CallKit is barred from mainland China storefronts, and the voip background mode gets an app rejected when it carries it without a working call implementation. Verified: validate-guide-snippets passes (712 include-backed blocks, 0 inline), the demos module compiles all seven snippets, vale reports 0 errors 0 warnings 0 suggestions, and asciidoctor renders the whole guide at --failure-level=WARN. The push payload is a literal block rather than [source,json]: the validator requires every source block to be include-backed, and a wire format is not something the demos module can compile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CN1Call.{h,m} and CN1Vpn.m, the Java bridges, and the header defines the
builder flips.
The whole design of CN1Call.m is the VoIP push deadline. iOS terminates the
app if didReceiveIncomingPushWithPayload returns without reporting the call to
CallKit, and repeated offences revoke VoIP push for the installed app -- a
one-strike API. So nothing in that path touches Java: PushKit hands the
payload already parsed, ObjC reads the cn1call key, reports the call, queues
the record, arms a TTL watchdog, and only later -- when application code asks
-- hands any of it up. There is exactly one code path whether the app was
running or not, because a fast path that only ran sometimes would be the half
nobody tested.
The registry is installed from willFinishLaunchingWithOptions rather than
didFinishLaunching: a push can arrive during launch and a registry that does
not exist yet loses it, which is the case that kills the process.
Four decisions worth recording:
cn1clReportIncoming is the single funnel both the push and the Java path go
through, so a socket and a push racing for the same call cannot report the
uuid twice. CallKit answers a duplicate with CallUUIDAlreadyExists and THROWS;
this downgrades it to an update.
A payload with no cn1call key reports a placeholder and ends it immediately.
That looks odd and is strictly better than returning without reporting: the
process survives and the user sees nothing.
A malformed uuid is replaced with a fresh one rather than refused, and the
record is flagged synthesized, so a server bug is findable instead of
presenting as calls that never connect.
The provider's configuration is read from Info.plist alone, never from
anything Java set, because on a cold start the name the user sees has to exist
before app code runs.
CN1Vpn.m stores credentials as keychain references because NEVPNProtocol takes
references rather than strings -- which is precisely why a loaded profile
cannot hand the secret back, and why the Java API says so.
Both files define every symbol in both halves. A build that never referenced
the packages still links them, answering unsupported, because a native method
is kept alive BY its symbol appearing in a native source: absent it, the
dead-code pass drops the Java method and the feature ships inert with a green
build and nothing in the log.
Verified: the iOS port compiles, and check-native-signatures reports 0 fatal
across 1031 native methods -- which is the gate that catches a mangled name,
the failure mode that otherwise compiles, links, and ships silently dead. The
37 iOS problems are pre-existing ORPHAN warnings; the windows and linux fatals
are pre-existing in ports this branch does not touch.
NOT verified: any of this on a device. A real incoming CallKit call from a
cold start and an IKEv2 profile install both need hardware.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A scripted edit two commits ago wrote the file back in Python text mode, which rewrote all 16k lines to LF and turned a nine-line addition into a whole-file diff. That is not cosmetic: it defeats diff-informed review and CodeQL's changed-lines analysis, which is why this repo has been bitten by it before. The file is CRLF on master and stays CRLF. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Blog prose gate✅ No net-new prose findings introduced by this PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e215430f92
ℹ️ 关于 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
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
CN1Call.m is the one file on this branch whose correctness nothing could check. It is ~1500 lines of Objective-C, no Java test can reach it, and the only gate that touched it -- check-native-signatures.sh -- reads names, not bodies. The existing clang job covers three identity files and stops there, so a wrong selector or a bad pointer conversion in the new natives would have been found by a cloud build, on somebody else's time. Adds a step that runs -fsyntax-only over CN1Call.m and CN1Vpn.m in both configurations: the halves an app that never referenced the packages links, and the halves one that did. A separate step rather than more files on the existing one, because these need two more things in the shim that the identity files do not: getThreadLocalData and the generated IOSCallCallbacks upcall header. Worth recording what the shim taught me, since it is the kind of thing that looks like a bug in the code: CODENAME_ONE_THREAD_STATE is not a type. It expands to `struct ThreadLocalData* threadStateData` -- a full parameter declaration INCLUDING the name, which is why every native body can refer to threadStateData without declaring it. The first shim defined it as a typedef and produced five confident "use of undeclared identifier" errors against code that was correct. The upcall prototypes are spelled out rather than derived from the mangled names. A generator would be one more thing to be wrong about, and a mismatch between these and the real ones is caught by check-native-signatures.sh anyway. Verified by running the step's shell verbatim: exit 0 on both arms, and a deliberately mistyped selector fails it with "no visible @interface for 'CXProvider' declares the selector". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
比较d 12 screenshots: 12 matched. |
LanguageTool flagged two British forms: "afterwards" and "recognises". The guide is en-US, and both fixes match what it already does elsewhere -- "afterward" is the majority form and "recognizes"/"recognized" is used throughout. Note "behaviour" and the bare "recognise" are already in languagetool-accept.txt and stay as they are; it was the inflected "recognises" that had no entry. This was the one gate I had no way to run locally, so I pushed blind on it. Now fixed: language-tool-python is installable, the guide renders to HTML with asciidoctor, and scripts/developer-guide/run_languagetool.py runs against it offline. It reports 0 matches with these two corrections, and the paragraph capitalization checker (ruby, also runnable locally) reports 0 issues. 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: 9fa3bc5dbe
ℹ️ 关于 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
|
Collaborator
Author
|
比较d 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
比较d 181 screenshots: 181 matched. |
Codex found fifteen issues on the CodenameOne PR. This is the core and Android half; the builder half follows. Real defects, fixed as reported: - Telecom reports were parked in a single static slot, so two calls reported before the first callback made one callback acknowledge the wrong request and the other acknowledge nothing -- an AsyncResource that never settles. Keyed by call id now, which the request extras already carry. - Android kept the provisioned VPN profile across a process restart but the bridge gated start/stop on an in-memory field, so it answered NOT_CONFIGURED for a profile the platform still held. The platform's own refusal is the authority; a 安全Exception from it now maps to NOT_CONFIGURED and anything else to CONNECTION_FAILED. The profile description is persisted so load() survives a restart too. - Nothing observed the real tunnel: startVpn set CONNECTING and no code path ever moved it. A VPN-transport 网络Callback now drives the status, which is also the only way an app hears about a disconnect from 设置. - The screening service caches its number list in another process and neither setEntries nor reload invalidated it, so updates were ignored until the process died. - Screening status trusted a static flag that is false in any process which did not itself request the role; it asks RoleManager.isRoleHeld now. - An ended session was never removed from Calls.getSessions(), which promises current calls. - CallAction.defer() documented a safety timer that did not exist. Two the review was right about but where I disagreed with the remedy, and a code comment at each site says why: - It asked to drop CAPABILITY_ON_DEMAND on Android. Dropping ALWAYS_ON as well, on BOTH platforms and in the simulation: no ordinary app can ask either OS for always-on, and Vpn个人资料.alwaysOn() was mapping to setBypassable, which is a different guarantee. The method is gone rather than left lying. - It asked to install certificate credentials from the wire record. That cannot be done correctly: SecPKCS12Import and KeyStore both need the passphrase, and Vpn个人资料.certificate(byte[]) carries none -- so the method could never have worked. Removed, with a comment saying what adding it back would take. Wire fields 7 and 8 stay as reserved empty slots so the native parsers' indices do not shift. One I extended: the route-picker capability was dropped from iOS as asked, and from Android and the simulation too. No platform has a system call route picker, so removing it from one port would have left the simulation the only place an app's picker code appeared to work. The safety timer is a java.util.Timer rather than Display.setTimeout, for the reason the Bluetooth operation queue gives: it must work before Display.init, and the device Timer has no daemon constructor -- so it is cancelled alongside its task or its thread keeps a desktop JVM alive. Six new tests, all verified by reverting their fix: two of them failed exactly as intended. A seventh check turned up a wrong premise in my own test rather than a bug, and LocalCallBridge grew primeEndFailure so "the platform refused to end the call" is reachable at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two P1s on the BuildDaemon PR asked me to generate the iOS packet-tunnel extension and declare the Android VpnService. Chasing them turned up something neither comment could see: **com.codename1.vpn.tunnel contains no classes at all.** I created the directory and never put anything in it. So the package had a PlatformFeatureCatalog entry keyed on a prefix nothing can match, two native defines, a BuildException demanding an entitlement Apple grants case by case, capability bits, and a documented chapter section -- about thirty references to a package no application could ever import. Generating an extension for it would have been building scaffolding around an absence. The tunnel half is removed rather than completed. Completing it means an Objective-C or Swift NEPacketTunnelProvider that cannot be written in this framework at all -- the extension process has no ParparVM in it -- plus an Android VpnService and a packet API this branch does not have. That is a separate piece of work, and the guide now says so and points at the two real options instead: express the requirement as IKEv2, or ship a native extension through the existing ios/app_extensions channel. VpnProtocol.CUSTOM goes with it (last ordinal, so nothing shifts). CAPABILITY_CUSTOM_TUNNEL and isCustomTunnelSupported stay in the SPI as a documented seam, answering false on every port including the simulation -- which must not be the one place an app's tunnel code appears to work. Also in this round, from the CodenameOne PR: - Submitted libraries were never scanned. The builders read the app's compiled classes and never open a jar, so a feature used only by a cn1lib got no permissions, no services and no native defines. Nearby solved this with a private walker; that walker is now LibraryClassPrefixScan with the feature-specific parts removed, wired into BOTH builders, so the next family needs no third copy. Five tests, including the nested classes.jar an AAR keeps its bytecode in. - Manifest service suppression was all-or-nothing: an app that hand-declared either service suppressed both, so the other went missing. Per service now, with three tests. Verified: ant core, iOS port, android compile, 1355 plugin tests, 43 catalog tests, 78 call/vpn unit tests, every source gate, vale 0/0/0, and check-native-signatures still 0 fatal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last substantive P1. Referencing com.codename1.call.directory enabled a native define and nothing else: no extension target, and neither of the two Info.plist keys CN1Call.m reads. So the API compiled, linked, and then failed every setEntries with "No App Group is configured" and had no extension to reload. The Android half worked; iOS was inert. Unlike the VPN tunnel dropped in the previous commit, this package is real -- CallDirectory, DirectoryEntry and DirectoryStatus all exist and work on Android -- so the fix is to finish it rather than remove it. IOSCallDirectoryExtensionBuilder is modelled on MatterExtensionBuilder, with one deliberate difference: it emits Objective-C, not Swift. Matter has no choice -- MatterSupport has no Objective-C interface -- while CallKit does, and an extension is memory-capped, so not embedding the Swift runtime is worth having. Three things in the generated handler are requirements rather than choices, and each fails silently when missed. Entries must be added in ascending numerical order or iOS rejects the whole list naming no row, so a row that would break the ordering is skipped rather than allowed to poison the load. An incremental reload must not re-add everything, and no changelog is kept, so it is turned back into a full reload. And the request must COMPLETE rather than fail when no data has been installed yet: failing makes iOS disable the extension. IPhoneBuilder now derives the App Group, adds it to ios.app_groups, writes CN1CallAppGroup and CN1CallDirectoryExtensionIdentifier into Info.plist, and appends the target after the global deployment-target pass -- guarded against the duplicate-target problem the Wallet work hit when fix_xcode_schemes.rb re-runs. Nine tests, plus the three per-service manifest tests and five library-scan tests from the previous round: 1372 plugin tests green. 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: 2800dee3d6
ℹ️ 关于 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".
BD review asked for a <service> guarded by BIND_VPN_SERVICE for com.codename1.vpn.tunnel. There is no such package -- the previous commit removed its configuration -- and the scanner is where a reader would look for the omission, so the reason lives there rather than only in a PR thread nobody reads. 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: 9371cb1b96
ℹ️ 关于 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".
Codex re-reviewed and found eight more, four of them defects in the previous round's fixes rather than in the original code. That is the shape to expect: each fix was right about the line it changed and wrong one level up. Defects in my own fixes: - The deferred-end cleanup checked isAnswered() immediately after dispatch. For a deferred action that is false by construction, and a later fulfil had nothing watching -- so a call the app really did end stayed in getSessions() forever. CallAction grew whenFulfilled, registered BEFORE settle(), so only a fulfilled end forgets and it forgets whenever that happens. - The native end transaction removed the UUID before checking whether CallKit had accepted it. Java correctly retained the session on failure, so the two sides disagreed: a retry answered INVALID_ID for a call the system was still showing. Removed only on success now. - The Android VPN wire was persisted in the consent callback only, so replacing a profile the user had already approved -- which returns no consent intent -- left load() describing the previous one after a restart. - The generated Call Directory handler read the whole TSV into an NSString while its own class documentation promised streaming. A six-figure blocklist is exactly what gets a memory-capped extension killed. It maps the file now and parses numbers straight out of the bytes, so the common row allocates nothing. Defects in the original code: - A fulfilled reject or hang-up never actually ended the Telecom call. The action was delivered to Java and auto-fulfilled, and nothing called setDisconnected/destroy, so the call stayed alive in Telecom and in CONNECTIONS after the user had hung up. - The library scanner set the feature booleans but never fed aiAcc.consume, which is what selects the PlatformFeatureCatalog entries. A library-only app got the defines enabled and CallKit, PushKit, AVFoundation and 网络Extension unlinked -- the exact failure the nearby comment beside it describes. - iOS configureProvider ignored the record entirely, so everything CallConfiguration documents as refining the build-time defaults had no effect. It applies them now, taking care that localizedName is read-only from iOS 14. - Android requestPermissions reported the current mask and never asked, so an app calling the method whose contract says it REQUESTS always saw a denial. Verified: ant core, iOS port, android compile, clang -fsyntax-only on both native arms, 80 call/vpn tests, 1374 plugin+catalog tests, SpotBugs 0 on core and android, PMD/quality-report exit 0, and every source gate. The new deferred-fulfil test was verified by restoring the previous check and watching it fail. 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: 427a34bd7b
ℹ️ 关于 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".
…arator Ten more on CodenameOne, eight on BuildDaemon. The pattern held: several were defects in round 4's fixes. Action settlement and state, all one family: - A listener that threw skipped settle(), so the action went unanswered until the platform timed it out. Every dispatch arm settles in a finally now, and END registers its cleanup BEFORE dispatch so a throwing listener still gets it. - ANSWER set the session ACTIVE before dispatch, so a listener that failed the action -- directly or through the safety timer -- held an active session over a call iOS had put back to ringing and Android had destroyed. HOLD had the same shape. Both move on fulfilment now, as END already did. - setHeld and setMuted mutated the session before the platform had agreed, with no rollback when it refused. - performEndCallAction forgot the native call on DELIVERY rather than on fulfilment, so a failed end left Java and the native bridge disagreeing -- the mirror image of the transaction bug fixed last round. - A stale pushed call was registered as a current session, so every missed cold-start push stayed in getSessions() for the life of the process. It gets a detached session now: the app can still read the handle and the id, and getSessions() keeps its promise. Permissions, which were half-wired end to end: - Android requested CAMERA that the manifest never declared, so the bit could never be granted. It is declared now, behind the project's own call.video statement rather than for every calling app -- a gratuitous camera permission is a Play Console conversation and a prompt the user cannot explain. - iOS never reported or requested camera authorization at all, and had no NSCameraUsageDescription, so an app branching on CAPABILITY_VIDEO was stuck. Both prompts are chained now behind the same hint. Platform truthfulness: - The Android VPN status callback matched ANY VPN transport, so another app's tunnel reported ours as CONNECTED. Android gives an app no way to identify which VPN a transport belongs to; gating on our own start request removes the case that matters and the comment says what it cannot fix. - IPsec profiles with user credentials never enabled extended authentication, so they saved cleanly and could not connect. - A duplicate report was acknowledged as accepted while the original's CallKit completion was still pending; if that was then refused, Java held a session with no system call. Duplicates share the original's outcome now. And the one that would have been hardest to find from a log: ios.app_groups was appended with a COMMA, while generateEntitlements splits it on a space alone. The pair came out as one <string> "group.a,group.b" matching neither configured group, so the host could not reach the container it shares with the extension. declaresAppGroup tolerates either separator when reading, which is exactly what hid it. Verified: ant core, iOS port, android compile, clang on both native arms, 1374 plugin+catalog tests, 83 call/vpn tests, SpotBugs 0, PMD/quality-report exit 0. Three new tests, each verified by reverting its fix; a fourth attempt found a wrong premise in my own test instead, and LocalCallBridge grew primeOperationFailure so "the platform refused" is reachable for hold and mute as well as end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
build-ios failed on 35bdb4b with one test down, VideoIODecodedFramesScreenshotTest: "timeout waiting for DONE stage=created". Not a screenshot mismatch -- all 143 delivered screenshots matched, including that test's own, which arrived 1.4s after the runner had already given up on it. The suite was not slow that run: the median per-test duration was 0.95x a healthy run's. What it had was scattered stalls. FillRoundRect, which draws one rounded rectangle and normally finishes in under 0.1s, sat 26.3 seconds between "awaiting" and its PNG with not one log line in between; GaussianBlur sat 24.3s. Both passed, because a 25s stall still fits inside a 30s budget that nothing else was using. That stall is the condition shouldRetryAfterSilentTimeout() was written for, and its comment already names it. The two VideoIO tests have neither cushion. They drive a real native encode and decode, so they enter the budget already 6-8s spent, and isRetrySafe() is false for both -- deliberately, because their work outlives runTest() and resetForRetry() would let a late done() complete the retry. So the one mechanism that recovers every other stalled test is unavailable to them by construction, and a stall that costs FillRoundRect nothing fails them outright. Both have failed exactly this way, and not only here: on master at 73aa266 both timed out at 30s with the same stage=created message, which is what rules out any one branch. On 35bdb4b the decoded-frames test spent 22.3s in its EDT frame-render phase and a further 49.7s in its worker encode/decode and capture -- two serial phases, so it pays the stall window twice. Three times the default covers the work plus a stall window per phase. The suite's own caps are 2100s absolute and 720s idle, so neither is threatened by a 90s test. This widens a deadline; it does not weaken an assertion -- a test that is genuinely broken still fails, 60 seconds later. 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: 1669702ce6
ℹ️ 关于 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
|
比较d 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
比较d 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
比较d 163 screenshots: 163 matched. |
Collaborator
Author
|
比较d 163 screenshots: 163 matched. |
Collaborator
Author
|
比较d 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
…rsed
Three defects, two of them from review and one found while checking the
first two.
**A failed load answered the stop as if the tunnel were down.**
cn1vpLoadTunnelManager yields a nil manager on ONE path only: the
preferences load errored. With no error it hands back a manager either
way -- a saved one, or a fresh one whose connection is already
disconnected. So the branch carrying the "asking a tunnel that is not
running to stop has got the caller what they asked for" comment was never
the idle case at all; it was "iOS would not tell us what is running", and
it answered YES. Tunnels.stop() reported a stopped tunnel while traffic
could still be routing through it. The start path already did the right
thing with the same nil.
**A malformed CIDR was refused only in the simulator.**
TunnelWire.validate lived in LocalVpnBridge, so route("0/o") was refused
in the simulation and nowhere else: Android threw out of
VpnService.Builder inside the service, and iOS crossed into a separate
extension process where [prefix intValue] reads "foo" as 0 -- and 0 is
the one value that must not be guessed, because /0 is the default route.
An app asking for one subnet installed a route over all traffic and came
up reporting success.
Fixed a layer up rather than where it was reported: validateSetup now
runs in Tunnels.start(), above every bridge, before a request id exists,
so no port has to write the check again and none can disagree about it.
LocalVpnBridge's copy is deleted. The generated extension is still
hardened, because it is a separate process the system relaunches from a
saved configuration an older app version may have written: a strict
parser, routes with an unreadable prefix dropped exactly as wrong-family
routes already are, and an unreadable interface address failing the start
rather than establishing a link nobody asked for. The MTU keeps its
lenient read on purpose -- zero is not a meaningful MTU, so falling
through to the system default is the recoverable answer, and it is what
TunnelWire.mtu picks on the Java side.
**A keychain leak the gate was already reporting as a warning.**
The persistent ref came back through (__bridge_transfer NSData *). This
port compiles with CLANG_ENABLE_OBJC_ARC = NO, where the whole __bridge
family is a no-op, so SecItemAdd's +1 was never consumed and every
profile install leaked. Clang says so -- and said so on every green run,
as a warning that scrolled past. Now written out as an autorelease, and
arc-bridge-casts-disallowed-in-nonarc is promoted to an error, which is
what that gate exists for. Probed both ways: reinstating the cast fails
the gate, removing it passes.
Every test here was checked against the defect it describes. Reverting
the generator fix fails anUnreadablePrefixIsRefusedRatherThanReadAsZero;
removing the Tunnels.start() validation fails
aMalformedSetupNeverReachesAnyBridge, which asserts against a bridge that
is NOT the simulation and counts that it was never asked -- refusing
after asking would satisfy an error assertion and none of the point. One
existing assertion is rewritten: it checked for the local named "v6bits"
rather than for the parsed value reaching NEIPv6设置, so sharing one
parse between the families broke it while the property it named held.
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: 32cff1155c
ℹ️ 关于 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 provider reset can land in the middle of a report, and report() is not EDT-confined -- its own critical section is there because two threads report the same id, socket signalling racing a pushed call being the ordinary way -- while RESET is dispatched on the EDT. So an application thread can be inside report() while the reset runs. Two orderings, one leak: - Reset before openAck. failAll does not touch the request because it does not exist yet. The report goes out, the platform accepts, and SessionHandover sees a stale generation. - Reset between openAck and the bridge call. failAll kills the request, and the report still goes out on the next line. The platform accepts, and the acknowledgement saying so is discarded because its request is gone. In both, the platform holds a ringing or dialing call and this facade has just decided it owns nothing. The whole of what happened was failing the app's AsyncResource. That is correct and insufficient: telling the app is not telling the platform, and nothing else was ever going to. The call outlives the reset that was supposed to have swept everything, addressable by nothing in Java, until the user dismisses it. Review offered two remedies -- recheck the generation atomically with the native handoff, or retire a call accepted after a mismatch. Only the second is enough. The handoff is asynchronous, so a reset can arrive after submission and before acceptance whatever the submit path checks, and holding the SESSIONS monitor across a bridge call invites the deadlock the ports' callbacks make possible. So both abandoning branches now end the call on the platform. The guard took two attempts, and the test is what caught the first one. Retiring only when the identity-checked forget() said this session still owned the id is the idiom reportEndedRemotely uses, and it is wrong here: RESET clears the map before it fails what is in flight, so a handover reached through a reset can never claim the id -- the reset already took it. The fix was inert, and read as correct. The question worth asking is not who used to hold the id but whether ending it would hang up somebody else's call, so releaseUnclaimed answers both under one lock. The sliver where a report registers between the check and the end is left, on the reasoning already recorded at reportEndedRemotely: a tombstone adds a way for an id to become permanently unusable, for a window narrower than the one being fixed. An ordinary refusal still ends nothing, which is the other half and has its own test: DUPLICATE_CALL means the platform holds the ORIGINAL call under that id, and an unconditional retire would hang it up. Both tests were checked against the defect they describe -- breaking the guard fails the first, making the retire unconditional fails the second. The generation branch is not reachable from a single-threaded test, because deliverProviderReset clears the sessions and fails the acknowledgements in one step; the test drives the acknowledgement branch, and both go through the same helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
build-ios-metal failed on GoogleWebMapScreenshotTest: "timeout waiting for DONE stage=retry-created". The test is not broken and neither is the renderer -- what failed is the test's ability to say so. waitForMapReady polls for 24s and only then reports "SKIPPED reason=map-tiles-never-loaded". That skip is the whole point of the code around it, and its comment says why: this test needs the Google Maps JavaScript API to load over the network inside a web view, so a map that never becomes ready establishes that the map could not be fetched here and says nothing about the port. Reporting that as a failure makes an unreachable network indistinguishable from a map that rendered wrongly. Against the plain 30s budget that skip had five seconds to happen in, once form setup, the web view, the 1s post-ready settle and the capture were paid for. So the runner declared the timeout first and the graceful answer was unreachable -- which is word for word the defect the VectorMapScreenshotBaseTest branch immediately above already fixes: "That escape hatch only works if the poll cap is strictly INSIDE this budget, and it was not." Nobody applied it here. The retry makes it worse rather than better. On the first exhaustion the test deliberately goes silent to hand off to the runner's one-shot silent-timeout retry, and on the retry it calls done() itself at +24s -- so the retry needs the runner not to guillotine it, and gets guillotined. In run 33231956992 the retry ran 34.6s without ever reaching its own 24s cap and failed having reported nothing at all. Sized from the cap rather than restating it, exactly as the VectorMap branch is, so the two cannot drift apart when either side is tuned; MAX_WAIT_MS is derived from WAIT_ATTEMPTS for the same reason. The cost falls only on a run where the map never loads: the first attempt's hand-off now waits out this budget rather than the default, so such a run takes about a minute longer and ends in a skip naming the reason instead of a red the suite cannot act on. A map that does load is unaffected and still compared on its pixels. Not this branch's doing -- it touches no map, web view or network code, and the VideoIO widening it does make is unreachable for any test but those two, which run at the end of the suite. What it is, is the second test found in two days whose failure mode is its own budget leaving no room inside the runner's. 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: 2e1e9a0953
ℹ️ 关于 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".
reportStartedConnecting() went through moveUnlessOver(DIALING), which guards ENDED and nothing else. Signalling is asynchronous and an app forwards what it receives, so a "ringing" event that overtakes an "answered" one arrives as reportConnected() followed by reportStartedConnecting() -- and that moved an ACTIVE or HELD session back to DIALING. The platform half is the one a user sees. AndroidCallBridge turns this report into Connection.setDialing(), so the SYSTEM call UI showed a connected call ringing out again. The transition could never have been anything else. Core moves a session to DIALING in exactly two places: Calls' report constructs an outgoing one that way, and this line. So moveUnlessOver(DIALING) was either a no-op on a call that was already dialing or that regression -- there was no case it existed to serve, which is why the transition is gone rather than reordered. Guarded on the state instead, read under the monitor the state is written under, for the reason isMuted() is: signalling reads this off the EDT while the ports deliver a connect on it. The window between that answer and the bridge call is deliberately NOT closed -- it is the same window every report in this class lives with, and a connect landing inside it is a report ordering the platform resolves, not a state this object can contradict. LocalCallBridge's identical unconditional assignment is left alone. A simulation that mirrors what the platform does when called wrongly is faithful, and the guard belongs above every bridge rather than inside one -- the same call made when the tunnel setup validation moved out of LocalVpnBridge and into Tunnels.start(). The test asserts the two halves separately, because they are separate: the session stays ACTIVE, and the platform is not told. Probed against the old code, which fails it with "expected: <ACTIVE> but was: <DIALING>". 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: e692a1eab7
ℹ️ 关于 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".
validateSetup read an empty address as "none given" and skipped it, so
new TunnelSetup().route("0.0.0.0/0") started in the simulation and
acknowledged. Android skips Builder.addAddress for that setup and hands
establish() an interface with no addresses, which addAddress's own
documentation rules out: at least one address must be set before
establish(). A setup the simulator approves and a device cannot bring up
is the single divergence this validation exists to remove, so the empty
address is now refused rather than tolerated.
Refused in validateSetup rather than in a port, for the reason the rest
of the file gives: it is the one place above every bridge, so the
simulation and both ports cannot disagree about it. The two existing
tests that pass a bare TunnelSetup are unaffected -- they install no
bridge, and NOT_SUPPORTED is answered before any setup is read.
address() now says REQUIRED, and the guide says a setup with no address
fails the same way an unreadable one does. It was documented only as
"the prefix may be left off", which described the half that was checked
and said nothing about the half that was not.
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: cb257d8769
ℹ️ 关于 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".
| sb.append(" com_codename1_impl_vpn_ExtensionTunnelHost_received___int(\n"); | ||
| sb.append(" threadStateData, (JAVA_INT)[p length]);\n"); | ||
| sb.append(" }\n"); | ||
| sb.append(" [self cn1ReadPackets];\n"); |
There was a problem hiding this comment.
When an iOS packet tunnel is stopped and restarted without terminating the extension process, the completion handler from the previous readPacketsWithCompletionHandler remains live and unconditionally re-arms itself here. Because its packet delivery calls the static ExtensionTunnelHost.buffer() and received() entry points, once the new start installs a host this stale handler can feed packets from the old read into the new tunnel and continue competing with the new reader; associate each read with a start generation and skip both delivery and re-arming after stopTunnelWithReason: invalidates it.
Useful? React with 👍 / 👎.
Comment on lines
+1301
to
+1302
| [(NETunnelProviderSession *)cn1vpTunnelManager.connection stopVPNTunnel]; | ||
| cn1vpTunnelAck(rid, YES, 0, nil); |
There was a problem hiding this comment.
For a running iOS packet tunnel, stopVPNTunnel only initiates the asynchronous disconnect, but the next line immediately resolves Tunnels.stop() successfully; the loaded-manager branch below does the same. Callers can therefore tear down resources or start a replacement while the old tunnel is still routing and its extension has not received stopTunnelWithReason: yet, despite the nearby failure path explicitly treating success while traffic continues as incorrect; retain the request until the connection reaches Disconnected/Invalid and report failures encountered during that transition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 773c120b36
ℹ️ 关于 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".
| synchronized (this) { | ||
| sent = generation == tunnelGeneration; | ||
| if (sent) { | ||
| context.startService(i); |
There was a problem hiding this comment.
When VPN consent is already granted and Tunnels.start() is invoked while the app is backgrounded, this startService() call is subject to Android 8.0+ background-service launch restrictions and can throw before CN1VpnService gets a chance to promote itself. The catch then reports UNKNOWN, so background reconnects fail even though the app still has VPN authorization; launch with the foreground-service API on supported versions and promote the service immediately while DNS/interface setup continues.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 773c120b36
ℹ️ 关于 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".
Comment on lines
+229
to
+230
| com.codename1.call.session.Calls.deliverAck(requestId, false, | ||
| com.codename1.call.CallError.NOT_SUPPORTED.ordinal(), |
There was a problem hiding this comment.
When CallDirectory.requestScreeningRole() is called on iOS, where the screening role does not exist, this routes false through deliverAck, which converts it into a CallException. The public API explicitly promises that a missing or refused role resolves with false, and both the simulator and Android use deliverAckValue for that outcome, so portable code handling denial in its success callback instead receives an unexpected failure only on iOS.
Useful? React with 👍 / 👎.
build-test (8) went red on 773c120 with a 504 from PATCH /repos/.../issues/comments -- while posting a report whose own first line was "Continuous Quality Report" over 7416 tests, 0 failed. The gate had passed. The step that failed is the one that tells a human so. Publishing is not the gate and was never meant to be one: generate-quality-report.py enforces the analysis earlier in the same job, and this step is declared `if: always()` precisely because it reports rather than decides. It could still take the build down, so a transient GitHub API error was indistinguishable from a real finding. That is worse than a missing comment: a red that says nothing is wrong is what teaches people to re-run reds without reading them, and this repository's rule is the opposite. So the publisher warns instead of throwing, and the warning is a run annotation rather than silence, so a comment that stopped being published is visible. Both call sites also ask actions/github-script to retry, which is the cheaper half -- the comment usually still lands -- but retries alone would have left the failure mode intact, just rarer. Guarded by a test, following the node test-*.mjs convention the website scripts already use, and wired into the Java 8 leg beside the step it is about. It covers all three API calls failing, a missing report, and both healthy paths, because a swallow that also swallowed the happy path would satisfy "does not throw" and publish nothing ever again. Probed against the pre-fix publisher, which it fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
publish-quality-comment.js predates the copyright gate and carried no header. Editing it made it a MODIFIED file, which is what the gate keys on, so it started demanding one. Missed locally for a reason worth writing down: I ran check-copyright-headers.sh before committing, and it reads committed state through git diff -- so the edit was invisible to it and it reported 142 files passed. Run the gates after the commit, not against the working tree, or they answer a question about a tree nobody is going to build. 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: 6bd67fc322
ℹ️ 关于 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".
cn1tnRoutes and cn1tnRoutes6 turned an empty list into the default route. So a setup that supplied an interface address and never called route() captured every packet on the device -- while the same setup on Android carried nothing, because CN1VpnService adds exactly the routes it was given and has no fallback anywhere. The reasoning behind it was written down and is wrong: "a tunnel with no included route carries nothing and an app that set none plainly meant all of it." That guesses at intent, and guesses in the one direction where being wrong cannot be undone by the app -- a tunnel that carries nothing is inert and obvious, a tunnel that quietly carries everything is neither. The API had already said so. TunnelSetup.route documents the full tunnel as the explicit 0.0.0.0/0 and ::/0, never as an absence, so the full tunnel has a spelling and it is not silence. Three implementations, and this was the only one disagreeing: Android adds what it was given, the simulation hands the setup's routes through untouched, iOS invented one. It is also the mistake the filtered case in the v6 helper already names -- a setup listing only v4 routes on a v6 interface captured ALL v6 traffic when the filtered list fell back to the default. That was fixed and the empty-input case deliberately kept the fallback. The only difference between the two is which way the list came to be empty, and neither is a reason to invent a route. Nothing shipped changes: no caller reaches this generator, IPhoneBuilder still refuses the tunnel hint, and the class javadoc says so. The test that asserted the old behaviour is replaced rather than deleted: it now holds that neither helper invents a route and neither special-cases an empty list, which is one property covering both ways of getting there. Probed by reinstating the v4 fallback, which fails it. 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: 5858ce23c1
ℹ️ 关于 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".
Comment on lines
+213
to
+214
| Intent i = ctx.getPackageManager() | ||
| .getLaunchIntentForPackage(ctx.getPackageName()); |
There was a problem hiding this comment.
When an incoming call arrives while the device is locked, this full-screen PendingIntent launches the app's ordinary launcher activity; Android does not transform that activity into a ringing UI, and the Answer/Decline actions remain only on the notification. Unless each application independently detects the extra and builds its own call screen, the user is taken to an arbitrary app form with no way to answer, contrary to the developer guide's promise that the port supplies the full-screen ringing UI. Use a dedicated incoming-call activity (or otherwise render the actions in the launched activity) instead of the generic launcher intent.
Useful? React with 👍 / 👎.
Comment on lines
+658
to
+661
| // Carried to the connection, which is the only place it can be | ||
| // applied: dropping it here left Telecom treating every call as | ||
| // audio-only while the bridge advertised CAPABILITY_VIDEO. | ||
| b.putBoolean(CN1ConnectionService.EXTRA_VIDEO, hasVideo); |
There was a problem hiding this comment.
Fresh evidence after clamping the PhoneAccount configuration is that an audio-only build can still pass video=true to either report method, and this value is copied unchanged into the request. CN1ConnectionService.adopt() then calls CN1Connection.setVideo(true), which advertises bidirectional video and sets the call's video state even though the account lacks video capability and the manifest lacks CAMERA; Telecom can consequently present a video call the app cannot capture. Clamp this value using the same declared-permission/configuration predicate before storing it in the extras.
Useful? React with 👍 / 👎.
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
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
注册 for free
to join this conversation on GitHub.
Already have an account?
登录 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.
Closes two gaps a prior review flagged: call management / VoIP system
integration, and VPN / network extensions. Both were entirely open — a
repo-wide search finds no CallKit, PushKit, ConnectionService, TelecomManager
or NEVPNManager anywhere, and
git log --all --grepfinds no prior attempt.The only telephony surface today is
Display.dial(), which hands a number tothe system dialer and forgets about it, and
Display.isInCall(), which on iOSis not telephony state at all but an app-lifecycle proxy.
网络Extensionis already linked, but only for Wi-Fi hotspot config.
The practical consequence is on record: the WhatsApp-clone post punted VoIP
because it "would require native integration". A Codename One app could not
ring while backgrounded, so it could not be a calling app however good its
audio was.
What this adds
com.codename1.call(.session,.voip,.directory) andcom.codename1.vpn(.profile,.tunnel), following thecom.codename1.nearbytemplate: a public package plus an SPI bridge ofprimitives only, asynchrony by caller-allocated request id, and a null bridge
from
CodenameOneImplementationas the capability query.Neither carries payload. No codec, no signalling, no packet forwarding —
those are the app's. What was missing was everything around the media.
Ports: iOS (CallKit, PushKit, NEVPNManager), Android (self-managed
ConnectionService,VpnManager), JavaSE/desktop (a real simulation, not astub), plus builders, catalog entries and two developer-guide chapters.
The package split is the permission boundary
PlatformFeatureCatalogmatches a class-name prefix withstartsWithandcannot express an exclusion, so the only way to say "this costs less" is to be
a different package.
.sessionbuys CallKit andMANAGE_OWN_CALLS..voipadds PushKit and thevoipbackground mode — which Apple rejectsan app for carrying without a working call implementation.
.directoryis deliberately not a superset: an app that labels somebodyelse's caller never owns a call, and Play Console flags gratuitous telephony
permissions. A test fails if these ever collapse into one entry.
Three things I checked rather than assumed
initVM()isUIApplicationMain, andDisplay.initstarts the EDT beforepostInit().So the VoIP-push deadline problem is not "the VM isn't up" — it is that app
Java isn't listening yet, EDT latency at launch is unbounded, and missing
the deadline is one-strike (the process is killed and the push entitlement
is eventually revoked). ObjC therefore owns the deadline entirely.
android.jar. Telecom isthere;
VpnManager(API 30) andRoleManager(API 29) are not, so thoseare reflective — no
minSdkVersionbump for a feature that can reportitself absent.
WatchOS26.2). They are weak-linked because the watch slice never references
them, not because they are missing.
Bugs this surfaced
ios.plistInject.<key>— a namespace nothing reads. Same shape as thepreviously-dead
android.health.privacyPolicyUrl.WatchNativeBuilderTestfailed on the commit that added the frameworks,which is exactly what that guard exists for.
suppressed — including
CallAction.isAnsweredreading unsynchronized whilethe writer held the lock, which would have sent a second answer for the same
token.
AndroidImplementation.javafrom CRLF to LF,turning nine lines into a 32k-line diff. Caught and reverted.
There is a fourth, in
BuildDaemon, which the companion PR fixes:buildNamespacedEntitlementspicks<string>vs<array>by whether thevalue contains a newline, but trims first — so a single-element array is
inexpressible. Both VPN entitlements are single-element arrays. NFC works
today only by the accident of having two entries.
Verified
ant core; iOS port; Android port; 5566 core-unittests; 1355 plugintests; 43 catalog tests. SpotBugs 0 on core-unittests and android; PMD and the
quality report exit 0;
check-package-info,check-since-tags,check-copyright-headers, the Java-25 markdown gate andcheck-cast-semanticsall clean. Vale 0 errors / 0 warnings / 0 suggestions; asciidoctor renders the
whole guide at
--failure-level=WARN; the snippet validator passes with 712include-backed blocks.
Critically,
check-native-signatures.shreports 0 fatal across 1031 iOSnatives — the gate for the failure where a mangled name compiles, links, and
ships the feature silently dead.
Four tests were verified by reverting their fix and watching them fail: the
action auto-fulfill, the provider-reset ordering, the VPN password stripping,
and the pushed-call drain.
NOT verified
Anything on a device. A real incoming CallKit call from a cold start and an
IKEv2 profile install both need hardware, and I would treat those as the
acceptance criteria before merging.
Pre-existing and untouched: the
Display.invokeAndBlockcast finding (reportsidentically on clean master), the windows/linux native-signature fatals, and 2
SpotBugs findings in
AbstractCN1Mojo.Scope note
The packet-tunnel half is scaffolding by necessity. On iOS the tunnel runs in a
process with no JVM, so its body is generated Swift the developer edits, and
its entitlement needs a case-by-case Apple grant — so the build refuses to
inject it and fails early naming what to enable, rather than producing a
codesigning error that names an entitlement and not the reason it appeared.
Requires codenameone/BuildDaemon#TBD — without it a cloud build gets no
entitlements at all, since this repo's
IPhoneBuilderonly writes theios.entitlements.*args and the consumer lives only in the daemon.🤖 Generated with Claude Code