Add the device debug/MCP loops to the generated agent skill, and give the archetype the Initializr's layout - #5864
Merged
Merged
Conversation
Collaborator
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
![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: d291e9c32d
ℹ️ 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".
| @@ -0,0 +1,29 @@ | |||
| # AGENTS.md | |||
There was a problem hiding this comment.
Because this template is stored under the reserved name AGENTS.md, agents working on scripts/initializr/common/src/main/resources/** will treat it as repository instructions and be redirected to generated-app paths and commands that do not exist in this source tree. This affects maintenance of the bundled skill itself; store the template under a non-reserved filename and rename it only when staging/generated output, as is already done for the Claude stub.
AGENTS.md reference: scripts/initializr/common/src/main/resources/AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
| codename1.arg.ios.onDeviceDebug.proxyHost=127.0.0.1 | ||
| codename1.arg.ios.onDeviceDebug.proxyPort=55333 | ||
| # Optional: hold the app at startup until the debugger attaches. | ||
| codename1.arg.ios.onDeviceDebug.waitForAttach=true |
There was a problem hiding this comment.
When an agent follows this configuration for an MCP-only iOS session, waitForAttach=true defers the application's lifecycle callback until a JDWP debugger attaches, while the MCP server is started later from MyAppName.start(). The app therefore never reaches the MCP starter unless the user also launches the proxy and attaches a debugger; document that ios.onDeviceDebug.waitForAttach must be false when MCP, rather than JDWP, is the purpose of the session.
AGENTS.md reference: scripts/initializr/common/src/main/resources/AGENTS.md:L23-L25
Useful? React with 👍 / 👎.
|
|
||
| The port is on the **device's** loopback, not yours, so it needs a forward: | ||
|
|
||
| - **Android device or emulator** — `adb forward tcp:8765 tcp:8765`, then connect to `127.0.0.1:8765` on your machine. Tear it down with `adb forward --remove tcp:8765` when you are done. |
There was a problem hiding this comment.
When multiple Android devices or emulators are online—the scenario for which the guide already exposes cn1.android.onDeviceDebug.deviceSerial—these bare adb forward and removal commands are ambiguous and fail instead of forwarding to the device selected by the debug goal. Include adb -s <serial> forward ... and the corresponding targeted removal in this flow so MCP reaches the same device as JDWP.
AGENTS.md reference: scripts/initializr/common/src/main/resources/AGENTS.md:L23-L25
Useful? React with 👍 / 👎.
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
Cloudflare Preview
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91ca3e87df
ℹ️ 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".
Comment on lines
+68
to
+73
| ```properties | ||
| codename1.arg.ios.onDeviceDebug=true | ||
| codename1.arg.ios.onDeviceDebug.proxyHost=127.0.0.1 | ||
| codename1.arg.ios.onDeviceDebug.proxyPort=55333 | ||
| # Optional: hold the app at startup until the debugger attaches. | ||
| codename1.arg.ios.onDeviceDebug.waitForAttach=true |
There was a problem hiding this comment.
When this flow is used on a physical iPhone running iOS 14 or later, setting proxyHost to the developer machine's LAN address makes the debugger perform local-network access, but the documented settings never add codename1.arg.ios.NSLocalNetworkUsageDescription. The on-device-debug injection in IPhoneBuilder adds the proxy keys and an ATS exception but does not synthesize this privacy string, so a newly generated app can be denied or terminated before connecting to the proxy unless it already happens to declare the key. Include the purpose-string hint in the physical-device instructions (or have the builder inject one for on-device-debug builds).
Useful? React with 👍 / 👎.
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
…ype the Initializr's layout Issue #5699 reports that a fresh Java 17 project tells an agent nothing about the on-device Maven goals or about driving a build on a phone, and that the Maven archetype and the web Initializr do not generate the same thing. Both halves are real; the issue's first request (an MCP reference) is not - it was filed against ac826b1, and references/mcp-agent-control.md has since landed, so that part is a pointer rather than a new file. references/on-device-debugging.md is the new reference. It covers the Android flow (android.onDeviceDebug, buildAndroidOnDeviceDebug or a local Gradle assembleDebug, android-on-device-debugging, JDWP on 5005, and the flags that matter), the iOS flow (the four ios.onDeviceDebug hints, proxyHost being 127.0.0.1 only for the native simulator, the app dialling the proxy on 55333 while the debugger attaches on 8000, and the invocation limits that make an expression refuse to evaluate), and how to reach the device's MCP port: adb forward on Android, nothing at all on the iOS simulator, a usbmux relay on a physical iPhone. The last one is stated as the route rather than as something we ship - there is no Codename One goal that tunnels it. Discovery is the other half. The Initializr writes AGENTS.md, the skill under .agent-skills/codename-one/ and a thin stub at .claude/skills/codename-one/, while the archetype wrote only the full skill under .claude/ - so an archetype-generated project had no root pointer and no vendor-neutral copy, and an agent that does not know Claude Code's directory layout never found the skill at all. The archetype now stages all three, from the same files, and strips all three for Java 8 the way it already stripped .claude/. AGENTS.md and the Claude stub move out of GeneratorModel's string constants and into files precisely because the archetype now needs them too: a second copy is how the two generators drifted apart in the first place. They sit flat at the root of src/main/resources for the same reason skill/ is repackaged into skill.zip - Codename One's classloader rejects nested resource directories. Verified end to end: the real Initializr ZIPs (scripts/tests/generate-initializr-fixtures.py) carry the new reference and the extended AGENTS.md, a project generated with -DjavaVersion=17 from the rebuilt archetype has all three, and one generated with -DjavaVersion=8 has none of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The MCP chapter already says the socket transport works on a phone, then stops: it never says the port is the phone's loopback rather than yours, so the one thing a reader has to do -- forward it -- is left out. Issue #5699 asks for the attachment route per platform to be explicit. Android is adb forward, and worth pairing with android-on-device-debugging's waitForAttach=false, since an agent session wants the app booted rather than blocked on a debugger. The native iOS simulator needs nothing, because it shares the host's network stack. A physical iPhone needs a usbmux relay we do not ship, which is said plainly rather than left for a reader to discover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Microsoft.Contractions is enabled for the developer guide, and "does not" in the new device-attachment section was the one error in the Vale report that failed "Build Developer Guide Docs". Vale is clean across all 123 guide files with this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…duced
The developer-guide gate fails on ANY LanguageTool match, not just on a
non-zero exit, so the three matches in the new paragraph were build-breaking
even with Vale clean.
Two of them are one sentence: "a usbmux TCP relay" trips EN_A_VS_AN (the rule
guesses a vowel sound for a word it has never seen) and MORFOLOGIK on "usbmux"
itself. Neither is a real defect, but the accept list matches the whole flagged
span, and the span for the article rule is the bare "a" -- which cannot be
accepted without accepting every "a" in the guide. So the sentence names the
mechanism in words the dictionary has ("the USB multiplexing channel") and
keeps iproxy, which is already masked as inline code.
libimobiledevice is a genuine proper noun the dictionary lacks, which is
exactly what the accept list is for.
Verified by rendering the chapter with asciidoctor and running
scripts/developer-guide/run_languagetool.py over it: 0 matches with this, and
the pre-commit text still reports the article and spelling matches, so the
check is answering about this prose rather than passing vacuously. Vale is 0
across all 123 guide files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ap, and multi-device adb Three findings, all real, plus the Vale error the last push left behind. The AGENTS.md body was stored under that literal name inside this repository, so an agent working on scripts/initializr/common/src/main/resources/** would read it as instructions for the Codename One tree -- and it says things like "app source lives in common/src/main/java" and "run the simulator with mvn -pl common cn1:run", which describe a GENERATED application and are wrong here. It moves to agent-skill-agents-md.md and is renamed on staging, the way the Claude stub already was. The two renames have opposite causes and the comments now say which is which. The archetype parity test asserts both halves: the source name exists, and the reserved one does not. waitForAttach on iOS is not the overlay it looks like. CodenameOne_GLAppDelegate wraps the VM callback in cn1_debugger_run_when_ready, which stashes the block until the proxy reports an IDE attached, so start() never runs and a MCP.startSocketServer call inside it never fires. The reference printed waitForAttach=true in the hints block and mentioned the false setting only for Android, which would have left an agent waiting on a port nothing was listening to. Said in the iOS section, in the MCP section, and in the developer guide. adb refuses forward and forward --remove outright when several devices are online rather than picking one, which is the case the reference already hands deviceSerial for. Both commands now show the -s form. Vale: "the usual relay for that is" tripped Microsoft.Contractions, which the previous push introduced while fixing the LanguageTool findings and did not recheck -- Vale had been re-run before that reword, not after. Both gates are now confirmed on the final text: Vale 0 across 123 files, LanguageTool 0 on the rendered chapter. Re-verified end to end after the rename: the real Initializr ZIPs still carry AGENTS.md, and a project generated from a clean-built archetype has AGENTS.md, 37 files under .agent-skills/codename-one, the .claude stub, and no file left under the source name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot loopback
An on-device-debug app dials OUT to the proxy on the developer's machine. For a
physical iPhone that is an address on the Wi-Fi the two share, and since iOS 14
reaching it is consent-gated local-network access that needs a purpose string
declared up front -- which iOS terminates an app for lacking, as the Matter
block in this same builder already says. The build injected CN1ProxyHost,
CN1ProxyPort, CN1ProxyWaitForAttach and an ATS exemption and stopped there, so
an app built exactly as the guide describes could be killed the moment
cn1_debugger dialled out, leaving the proxy waiting and nothing on the device to
say why. NSLocalNetworkUsageDescription was auto-injected only for Bonjour.
Injected only when the proxy host is not loopback. The native simulator shares
the host's loopback, which is not the local network, and an unnecessary purpose
string puts a prompt in front of a developer who never asked for one -- the
reason the nearby flags are kept apart from one another. Ambiguity resolves
towards declaring it: a host that is not recognisably loopback may still be a
LAN name, and the costs are not symmetric -- a spare string costs one prompt in
a build that is debug-only by construction, a missing one costs a session that
cannot start.
Through applyCatalogPlistEntry rather than putArgument, for the reason the
Matter and CallKit blocks both record: the sweep that copies
ios.NS*UsageDescription hints into privacyUsageDescriptions runs long before
this point and the plist is rendered from that map, so a bare argument set here
would never have been read. It fills only a missing value, so a project with
its own wording keeps it.
Verified against a real generated project rather than by reading. Built the
sample through cn1:buildIosXcodeProject's path (-Dcodename1.buildTarget=ios-source)
and read HelloCodenameOne-Info.plist three times:
proxyHost=192.168.1.42 -> the key present exactly once,
plutil -lint OK
proxyHost=127.0.0.1 -> the key absent, no prompt added
proxyHost=192.168.1.42 + own description -> "My own wording." survives
The predicate has its own test. SpotBugs is at zero findings for the module and
the build-hint catalog gate passes; the hint itself is already declared in
IosPrivacy, so nothing new is introduced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…device The build now declares NSLocalNetworkUsageDescription for a non-loopback proxy host, so the app is no longer terminated for reaching the LAN with no purpose string. What that buys is a PROMPT, and a prompt still has to be answered: until somebody taps it on the phone, the app cannot reach the proxy and the session looks exactly like a build that never dialled out. Said where the LAN address is set, because that is the line that causes it, and said with the symptom attached -- a proxy reporting nothing connected is the thing an agent will see, and the phone is the last place it would think to look. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#5868 fixed one way the arm flag could go stale and the regression test it added still failed on Linux CI -- six empty replies in one run. The message that change added is what made the next step possible: "could not re-arm fd=20", and with an errno now attached, 17. EEXIST. The descriptor was already in the host's epoll set. So the flag was never the real problem. ADD and MOD both mean "watch this descriptor for these events", and which one is correct depends on whether the kernel already holds it -- a fact the caller tracks per host, in plain arrays, written from the accepting thread and read by the host thread. Every way of getting that wrong ends the same way: epoll answers EEXIST or ENOENT, the caller sees an IOException, and it drops a connection it has already read a request from without writing a response. Chasing the individual desync paths one at a time would have been chasing symptoms; #5868 closed the biggest and six remained. The tell was that it is Linux-only. The kqueue branch has always been idempotent -- EV_ADD on a knote that exists updates it rather than refusing -- so the same mistaken flag costs nothing on macOS, which is why none of this ever reproduced in a development loop. The asymmetry WAS the bug: the flag is an optimisation that saves a syscall on the common path, and only epoll was treating it as a precondition. Each call now falls back to the other, so both platforms mean the same thing, and the flag keeps the fast path fast without deciding correctness. registerImpl also returns -errno instead of -1, and Reactor puts it in the message. What is left after the fallback is a genuine failure, and EBADF (closed under us) and EPERM (not pollable) ask for different answers from whoever reads the log -- a line saying only "could not watch fd 20" cost a whole round to classify. Measured on Linux, in a container held to 2 CPUs so the window stays open, with the same churn-around-a-parked-request workload the regression test uses: before 3 failures over 4 rounds, each with the re-arm error logged after 0 failures over 4 rounds, then 0 across 6767 requests in 8 more BackendHttpIntegrationTest is green at 72 tests locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
force-pushed
the
issue-5699-skill-mcp-ondevice
branch
from
September 19, 2026 09:26
53892f9 to
a38bb3c
Compare
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.
Closes #5699 — partly. Two of the three requests are real and are implemented here; the first one has already landed.
What the issue asked for, and what it got
1. An MCP reference in the generated skill — already there. The issue was filed against
ac826b1;references/mcp-agent-control.mdhas landed since, andAGENTS.mdandSKILL.mdalready route to it. No newreferences/mcp.md. What was genuinely missing from it is the device half, so it gains a short section pointing at the new reference.2. Real-device Android and iOS references — added. New
references/on-device-debugging.md:android.onDeviceDebug,cn1:buildAndroidOnDeviceDebugor a localbuildAndroidGradleProject+assembleDebug,cn1:android-on-device-debugging, JDWP on 5005, and the flags that matter (deviceSerial,wireless,apk,skipInstall,waitForAttach).ios.onDeviceDebughints,proxyHoststaying127.0.0.1only for the native simulator, the app dialling the proxy on 55333 while the debugger attaches on 8000, and the invocation limits that make an expression silently refuse to evaluate.adb forwardon Android, nothing at all on the iOS simulator, a usbmux relay on a physical iPhone. That last one is stated as the route rather than as something Codename One ships, because no goal tunnels it.references/debugging.mdand theSKILL.mdlookup table now route to it.3. Generated-project parity — fixed. The Initializr writes
AGENTS.md, the skill under.agent-skills/codename-one/, and a thin stub at.claude/skills/codename-one/SKILL.md. The archetype wrote only the full skill under.claude/, so an archetype-generated project had no root pointer and no vendor-neutral copy — an agent that doesn't happen to know Claude Code's directory layout never found the skill. The archetype now stages all three from the same files, and strips all three for Java 8 the way it already stripped.claude/.AGENTS.mdand the Claude stub move out ofGeneratorModel's string constants into files, precisely because the archetype now needs them too: a second copy is how the two generators drifted apart in the first place.The developer guide's MCP chapter gets the same device-attachment paragraph, since it said the transport works on a phone and then never said how to reach the port.
Verification
scripts/tests/generate-initializr-fixtures.py— real Initializr ZIPs across every IDE and both Java versions; the Java 17 ones carry the new reference and the extendedAGENTS.md, the Java 8 ones carry none of it.-DjavaVersion=17producesAGENTS.md+.agent-skills/codename-one/(37 files) + the.claudestub;-DjavaVersion=8produces none of them.Cn1AppArchetypeCertificateWizardTestgains a test pinning the three-part layout; probed by reverting the pom's target path, which fails it.GeneratorModelMatrixTestextended for the new reference and the newAGENTS.mdpointer.check-control-characters.pyclean;check-copyright-headers.sh --base <merge-base>clean.🤖 Generated with Claude Code