ADFA-5083: Host-side MCP server with the first CoGo-aware tool - #1659
Open
hal-eisen-adfa wants to merge 17 commits into
Open
ADFA-5083: Host-side MCP server with the first CoGo-aware tool#1659hal-eisen-adfa wants to merge 17 commits into
hal-eisen-adfa wants to merge 17 commits into
Conversation
Host-side Kotlin/JVM server in a standalone Gradle build at mcp/, using the official MCP Kotlin SDK over Streamable HTTP on loopback. One tool: ping. Versions verified against Maven Central rather than assumed: kotlin-sdk-server 0.15.0 pulls Ktor 3.5.1 and kotlin-stdlib 2.4.0, so the build needs Kotlin >= 2.4.0 - the repo catalog's 2.3.0 would reject the SDK's metadata. Records why .mcp.json stays untouched (a committed http entry pointing at an unstarted process breaks startup for every dev), why loopback gets no TLS, and that root Spotless does reach into top-level standalone dirs, so mcp/ uses tabs.
Kotlin 2.4.10 is a floor, not a preference: kotlin-sdk-server 0.15.0 ships stdlib 2.4.0 metadata that a 2.3.x compiler rejects. The standalone build keeps that off the root catalog entirely - mcp/ is absent from settings.gradle.kts, the same way apk-viewer-plugin is. SdkResolutionTest exists to make that floor fail loudly and on its own, rather than surfacing later tangled up in a protocol error.
Both were wrong in a way that would have cost the next reader time: - flox activate -d ../flox/local from inside mcp/ fails outright. The env's on-activate hook aborts unless activated from the repo root, so every Gradle command becomes: flox activate -d flox/local -- bash -c 'cd mcp && ./gradlew X' - io.ktor:ktor-client-sse does not exist at any version. The client SSE plugin ships inside ktor-client-core, which arrives transitively via kotlin-sdk-client.
PingTest drives the real MCP client over real Streamable HTTP - initialize, tools/list, tools/call - rather than calling the handler directly. The transport is the only thing this change actually adds, so testing anything less would prove nothing. Verified independently with raw curl against a running server: the handshake returns serverInfo cogo-mcp, tools/list returns ping, and tools/call returns pong. cogoMcpServer() is split out from main() so the test mounts the identical server the entrypoint does, without main()'s wait = true blocking the suite. Binds 127.0.0.1 only - the server is unauthenticated. Tests use port 0 so the suite never collides with a server running on 3000.
javap reports the handler as Function3, but ClientConnection is a Kotlin receiver compiled to a leading JVM argument - the lambda takes one parameter, not two. Caught by the compiler while implementing.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough
WalkthroughThe pull request adds an isolated Kotlin/JVM MCP server under ChangesStandalone MCP server
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt (1)
15-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd KDoc for the public declarations.
These public declarations have no KDoc.
mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt#L15-L36: Document the server metadata, registered tool contract, and returnedServerlifecycle.mcp/src/main/kotlin/com/itsaky/androidide/mcp/Main.kt#L9-L16: Document port argument behavior, the default port, and the loopback-only security constraint.mcp/src/test/kotlin/com/itsaky/androidide/mcp/PingTest.kt#L17-L68: Document the Streamable HTTP contract covered by the public test class and test functions.As per coding guidelines, “Public classes, functions, and non-obvious logic must have KDoc or Javadoc documenting contracts, rationale, threading, nullability, side effects, or units.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt` around lines 15 - 36, Add KDoc to the public declarations in CogoMcpServer.kt (lines 15-36), documenting server metadata, the ping tool contract, and the returned Server lifecycle; add KDoc to the declarations in Main.kt (lines 9-16), documenting port argument handling, the default port, and loopback-only binding; and add KDoc to the public test class and test functions in PingTest.kt (lines 17-68), documenting the Streamable HTTP contract each covers.Source: Coding guidelines
mcp/src/test/kotlin/com/itsaky/androidide/mcp/SdkResolutionTest.kt (2)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public test contract.
Add KDoc that states this is a Kotlin metadata and SDK compatibility smoke test. The assertions alone look like a model-property test.
As per coding guidelines: “Public classes, functions, and non-obvious logic must have KDoc or Javadoc documenting contracts, rationale, threading, nullability, side effects, or units.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/src/test/kotlin/com/itsaky/androidide/mcp/SdkResolutionTest.kt` around lines 7 - 9, Add KDoc to the public test class or its `sdk types load under this kotlin version` test function, explicitly documenting that it is a Kotlin metadata and SDK compatibility smoke test. Keep the existing assertions and test behavior unchanged.Source: Coding guidelines
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository test stack for the MCP tests.
Replace
kotlin("test")andkotlin.testimports withlibs.tests.junit.jupiter,libs.tests.google.truth, and their JUnit Jupiter and Truth APIs in the source and plan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/src/test/kotlin/com/itsaky/androidide/mcp/SdkResolutionTest.kt` around lines 4 - 5, The MCP tests currently use Kotlin’s test stack instead of the repository-standard JUnit Jupiter and Google Truth stack. Update SdkResolutionTest.kt to use the JUnit Jupiter and Truth APIs, update mcp/build.gradle.kts to use libs.tests.junit.jupiter and libs.tests.google.truth rather than kotlin("test"), and revise both referenced sections of docs/superpowers/plans/2026-08-11-mcp-server-hello-world.md to describe the same dependencies and imports.Source: Coding guidelines
docs/superpowers/specs/2026-08-10-mcp-server-design.md (1)
81-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the documented server factory separate from
main().This sample constructs and registers
Serverinsidemain(). The planned contract requirescogoMcpServer(): Serverto own tool registration, whilemain()only starts Ktor and mounts that factory. Align this sample with that split so an implementation copied from the design remains testable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-08-10-mcp-server-design.md` around lines 81 - 100, Split the sample into a `cogoMcpServer(): Server` factory that constructs the `Server` and registers the `ping` tool, then update `main()` to only parse the port, start Ktor, and mount `cogoMcpServer()` through `mcpStreamableHttp`. Preserve the existing server configuration and tool behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-08-10-mcp-server-design.md`:
- Around line 118-121: Update the documented flox invocation in the MCP run
instructions to activate from the repository root, then change into mcp/ within
the activated shell before running ./gradlew run. Apply the same correction to
the repeated invocation referenced by the comment.
In `@mcp/README.md`:
- Around line 22-23: Update the README section describing loopback binding to
explicitly state that 127.0.0.1 prevents remote clients but does not
authenticate local processes, so any local process can call the unauthenticated
/mcp endpoint. Replace the “no network hop to intercept” claim with this
local-access warning, and retain the requirement for TLS and authentication
before binding to non-loopback interfaces.
- Around line 19-20: Update the alternate-port command in the mcp README to
invoke the mcp build explicitly from the documented repository root, matching
the complete Flox command used by the default startup example while preserving
the 8080 port argument.
In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/Main.kt`:
- Line 10: Update the argument parsing in the main startup flow around
DEFAULT_PORT so zero or one argument is accepted, while invalid non-numeric or
out-of-range ports are rejected. Require the parsed port to be within 1..65535,
print an explicit error, and return without starting the server; only use
DEFAULT_PORT when no argument is provided.
In `@mcp/src/test/kotlin/com/itsaky/androidide/mcp/PingTest.kt`:
- Around line 12-15: Update the mcp test configuration in build.gradle.kts to
add JUnit Jupiter and Truth dependencies, and configure the test task for the
JUnit Platform if needed. In PingTest, replace the kotlin.test.Test and
assertEquals imports with the corresponding JUnit Jupiter and Truth APIs while
preserving the existing test behavior.
- Around line 45-48: Update the `handshake reports the server identity` test to
assert both `client.serverVersion?.name` against `SERVER_NAME` and
`client.serverVersion?.version` against `SERVER_VERSION`, preserving the
existing connected-client test flow.
---
Nitpick comments:
In `@docs/superpowers/specs/2026-08-10-mcp-server-design.md`:
- Around line 81-100: Split the sample into a `cogoMcpServer(): Server` factory
that constructs the `Server` and registers the `ping` tool, then update `main()`
to only parse the port, start Ktor, and mount `cogoMcpServer()` through
`mcpStreamableHttp`. Preserve the existing server configuration and tool
behavior.
In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt`:
- Around line 15-36: Add KDoc to the public declarations in CogoMcpServer.kt
(lines 15-36), documenting server metadata, the ping tool contract, and the
returned Server lifecycle; add KDoc to the declarations in Main.kt (lines 9-16),
documenting port argument handling, the default port, and loopback-only binding;
and add KDoc to the public test class and test functions in PingTest.kt (lines
17-68), documenting the Streamable HTTP contract each covers.
In `@mcp/src/test/kotlin/com/itsaky/androidide/mcp/SdkResolutionTest.kt`:
- Around line 7-9: Add KDoc to the public test class or its `sdk types load
under this kotlin version` test function, explicitly documenting that it is a
Kotlin metadata and SDK compatibility smoke test. Keep the existing assertions
and test behavior unchanged.
- Around line 4-5: The MCP tests currently use Kotlin’s test stack instead of
the repository-standard JUnit Jupiter and Google Truth stack. Update
SdkResolutionTest.kt to use the JUnit Jupiter and Truth APIs, update
mcp/build.gradle.kts to use libs.tests.junit.jupiter and libs.tests.google.truth
rather than kotlin("test"), and revise both referenced sections of
docs/superpowers/plans/2026-08-11-mcp-server-hello-world.md to describe the same
dependencies and imports.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf00a71c-af40-41e8-ae3a-e1be5b7958a2
⛔ Files ignored due to path filters (1)
mcp/gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (13)
docs/superpowers/plans/2026-08-11-mcp-server-hello-world.mddocs/superpowers/specs/2026-08-10-mcp-server-design.mdmcp/.gitignoremcp/README.mdmcp/build.gradle.ktsmcp/gradle/wrapper/gradle-wrapper.propertiesmcp/gradlewmcp/gradlew.batmcp/settings.gradle.ktsmcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.ktmcp/src/main/kotlin/com/itsaky/androidide/mcp/Main.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/PingTest.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/SdkResolutionTest.kt
Comment on lines
+118
to
+121
| ```bash | ||
| # from mcp/ | ||
| flox activate -d ../flox/local -- ./gradlew run | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the documented flox invocation.
These commands activate flox from mcp/. The plan states that the flox activation hook rejects activation outside the repository root. Start from the repository root, then change into mcp/ inside the activated shell.
Proposed correction
-# from mcp/
-flox activate -d ../flox/local -- ./gradlew run
+# from the repository root
+flox activate -d flox/local -- bash -c 'cd mcp && ./gradlew run'Also applies to: 164-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-08-10-mcp-server-design.md` around lines 118 -
121, Update the documented flox invocation in the MCP run instructions to
activate from the repository root, then change into mcp/ within the activated
shell before running ./gradlew run. Apply the same correction to the repeated
invocation referenced by the comment.
Comment on lines
+19
to
+20
| Listens on `http://127.0.0.1:3000/mcp`. Pass a different port as the first | ||
| argument: `./gradlew run --args 8080`. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the alternate-port command in the mcp build.
From the documented repository root, ./gradlew run --args 8080 uses the root wrapper and omits cd mcp. Use the same complete Flox command as the default startup example.
Proposed fix
-argument: `./gradlew run --args 8080`.
+argument: `flox activate -d flox/local -- bash -c 'cd mcp && ./gradlew run --args 8080'`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Listens on `http://127.0.0.1:3000/mcp`. Pass a different port as the first | |
| argument: `./gradlew run --args 8080`. | |
| Listens on `http://127.0.0.1:3000/mcp`. Pass a different port as the first | |
| argument: `flox activate -d flox/local -- bash -c 'cd mcp && ./gradlew run --args 8080'`. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/README.md` around lines 19 - 20, Update the alternate-port command in the
mcp README to invoke the mcp build explicitly from the documented repository
root, matching the complete Flox command used by the default startup example
while preserving the 8080 port argument.
Comment on lines
+22
to
+23
| Loopback only, and no TLS - there is no network hop to intercept. Binding a | ||
| non-loopback interface would require both TLS and authentication first. |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Document the loopback trust boundary.
127.0.0.1 blocks remote network clients, but it does not authenticate local processes. The server is unauthenticated, so any local process can call /mcp. Replace the “no network hop to intercept” claim with an explicit local-access warning before adding adb-backed or project-mutating tools.
Proposed wording
-Loopback only, and no TLS - there is no network hop to intercept. Binding a
-non-loopback interface would require both TLS and authentication first.
+Loopback only. This endpoint has no TLS or authentication, so local processes
+can connect to it. Binding a non-loopback interface requires TLS and
+authentication first.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Loopback only, and no TLS - there is no network hop to intercept. Binding a | |
| non-loopback interface would require both TLS and authentication first. | |
| Loopback only. This endpoint has no TLS or authentication, so local processes | |
| can connect to it. Binding a non-loopback interface requires TLS and | |
| authentication first. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/README.md` around lines 22 - 23, Update the README section describing
loopback binding to explicitly state that 127.0.0.1 prevents remote clients but
does not authenticate local processes, so any local process can call the
unauthenticated /mcp endpoint. Replace the “no network hop to intercept” claim
with this local-access warning, and retain the requirement for TLS and
authentication before binding to non-loopback interfaces.
| const val DEFAULT_PORT = 3000 | ||
|
|
||
| fun main(args: Array<String>) { | ||
| val port = args.firstOrNull()?.toIntOrNull() ?: DEFAULT_PORT |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reject invalid port arguments.
The port parser accepts negative and out-of-range integers. A non-numeric argument also silently starts the server on port 3000. Validate zero or one argument and require a port in 1..65535. Print an explicit error and return for invalid input.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/Main.kt` at line 10, Update the
argument parsing in the main startup flow around DEFAULT_PORT so zero or one
argument is accepted, while invalid non-numeric or out-of-range ports are
rejected. Require the parsed port to be within 1..65535, print an explicit
error, and return without starting the server; only use DEFAULT_PORT when no
argument is provided.
Comment on lines
+12
to
+15
| import kotlin.test.Test | ||
| import kotlin.test.assertEquals | ||
| import io.ktor.client.engine.cio.CIO as ClientCIO | ||
| import io.ktor.server.cio.CIO as ServerCIO |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -HI '^(libs\.versions\.toml|build\.gradle\.kts)$' . | sort
rg -n -i 'junit|jupiter|truth|kotlin-test' mcp/build.gradle.kts gradle/libs.versions.toml 2>/dev/null || trueRepository: appdevforall/CodeOnTheGo
Length of output: 5611
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- mcp/build.gradle.kts ---'
cat -n mcp/build.gradle.kts
printf '%s\n' '--- MCP test files and test imports ---'
git ls-files 'mcp/src/test/**/*' | sort
rg -n '^(import )?(org\.junit|com\.google\.common\.truth|kotlin\.test)|assertThat|assertEquals|`@Test`' mcp/src/test 2>/dev/null || true
printf '%s\n' '--- Version-catalog aliases and usage ---'
sed -n '255,285p' gradle/libs.versions.toml
rg -n 'libs\.tests\.(junitJupiter|googleTruth)|tests-junit-jupiter|tests-google-truth' --glob '*.gradle.kts' .Repository: appdevforall/CodeOnTheGo
Length of output: 4840
Use JUnit Jupiter and Truth for this test.
mcp/build.gradle.kts declares only kotlin("test"); useJUnitPlatform() does not add these APIs. Add the JUnit Jupiter and Truth test dependencies, then replace the kotlin.test imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/src/test/kotlin/com/itsaky/androidide/mcp/PingTest.kt` around lines 12 -
15, Update the mcp test configuration in build.gradle.kts to add JUnit Jupiter
and Truth dependencies, and configure the test task for the JUnit Platform if
needed. In PingTest, replace the kotlin.test.Test and assertEquals imports with
the corresponding JUnit Jupiter and Truth APIs while preserving the existing
test behavior.
Source: Coding guidelines
Comment on lines
+45
to
+48
| fun `handshake reports the server identity`() = | ||
| withConnectedClient { client -> | ||
| assertEquals("cogo-mcp", client.serverVersion?.name) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
find "$HOME/.gradle/caches/modules-2/files-2.1/io.modelcontextprotocol" \
-type f -name '*.jar' -print0 2>/dev/null |
while IFS= read -r -d '' jar; do
if jar tf "$jar" | grep -qE 'Client.class|Implementation.class'; then
echo "== $jar =="
javap -classpath "$jar" \
io.modelcontextprotocol.kotlin.sdk.client.Client \
io.modelcontextprotocol.kotlin.sdk.types.Implementation \
2>/dev/null || true
fi
doneRepository: appdevforall/CodeOnTheGo
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== candidate files =='
fd -i 'PingTest.kt|libs.versions.toml|build.gradle.kts|settings.gradle.kts' . | sort
echo '== PingTest.kt =='
ping_test="$(fd -i 'PingTest.kt' . | head -n 1)"
if [ -n "${ping_test}" ]; then
cat -n "${ping_test}"
fi
echo '== MCP SDK declarations and identity constants =='
rg -n -C 3 \
'modelcontextprotocol|SERVER_NAME|SERVER_VERSION|serverVersion|Implementation\(' \
--glob '*.kt' --glob '*.kts' --glob '*.toml' --glob '*.gradle' . \
| head -n 300Repository: appdevforall/CodeOnTheGo
Length of output: 28347
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== server identity definition =='
server_file="$(fd -i 'CogoMcpServer.kt' . | head -n 1)"
cat -n "${server_file}" | sed -n '1,28p'
echo '== resolved MCP SDK artifacts =='
find "$HOME/.gradle/caches/modules-2/files-2.1" \
-type f \( -iname '*kotlin-sdk*0.15.0*.jar' -o -iname '*kotlin-sdk*0.15.0*.module' -o -iname '*kotlin-sdk*0.15.0*.pom' \) \
-print 2>/dev/null | sort || true
echo '== static identity consistency check =='
python3 - "${server_file}" <<'PY'
import re
import sys
text = open(sys.argv[1], encoding="utf-8").read()
name = re.search(r'const val SERVER_NAME\s*=\s*"([^"]+)"', text)
version = re.search(r'const val SERVER_VERSION\s*=\s*"([^"]+)"', text)
server_info = re.search(
r'Implementation\(name\s*=\s*SERVER_NAME,\s*version\s*=\s*SERVER_VERSION\)',
text,
)
print(f"SERVER_NAME={name.group(1) if name else '<missing>'}")
print(f"SERVER_VERSION={version.group(1) if version else '<missing>'}")
print(f"serverInfoUsesBothConstants={bool(server_info)}")
PYRepository: appdevforall/CodeOnTheGo
Length of output: 1581
Assert the full server identity.
Compare client.serverVersion?.name and client.serverVersion?.version with SERVER_NAME and SERVER_VERSION. The current assertion allows a server-version regression to pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/src/test/kotlin/com/itsaky/androidide/mcp/PingTest.kt` around lines 45 -
48, Update the `handshake reports the server identity` test to assert both
`client.serverVersion?.name` against `SERVER_NAME` and
`client.serverVersion?.version` against `SERVER_VERSION`, preserving the
existing connected-client test flow.
Source: Learnings
Probes whether Code On The Go is installed on the attached device. First step toward CoGo-awareness; the tool surface stays deliberately small. Adb is a fun interface over the process boundary so the tool's logic is tested without a device, and SystemAdb is the one real implementation. SystemAdb is covered by running /bin/echo and /bin/sh rather than adb itself, so the adapter is genuinely exercised in CI without an emulator attached. A failed adb call reports isError rather than "not installed" - not knowing is not the same as knowing it is absent, and collapsing the two would make a missing device look like a missing app. Two parsing hazards are pinned by tests: pm list packages matches substrings, so com.itsaky.androidide.debug must not satisfy a query for com.itsaky.androidide; and adb shell emits CRLF, so an untrimmed compare would silently never match. Verified against emulator-5554 through the real MCP transport, not just fakes: tools/call is_cogo_installed returns "is installed", matching adb shell pm list packages. Test fixture extracted from PingTest so both suites share one server-and-client harness.
hal-eisen-adfa
changed the title
tools/list already listed every tool, but initialize said almost nothing about what the server was for. An agent had to infer the whole purpose from two tool descriptions. - initialize now returns instructions: what the server drives, that tools act on adb's default device, and that an adb failure is not a negative answer. - Every tool carries a title alongside name and description. - listChanged drops from true to false. The tool set is fixed at construction, so the old value promised a notifications/tools/list_changed that would never arrive - advertising a capability we do not implement. Tool descriptions also now say when to reach for the tool, not just what it does; that string is the only such signal an agent gets. ServerDescriptionTest pins all three so they cannot silently rot, and so a new tool added without a title fails the build rather than shipping unlabelled.
Launching the app is not enough to reach home. MainActivity.onCreate calls tryOpenLastProject(), and autoOpenProjects defaults to true, so the real path is Splash -> Onboarding -> MainActivity -> Editor. Clearing ide_last_project does not help either: tryOpenLastProject() falls back to the most recently modified project, so it opens something regardless. Only the boolean prevents it. So the tool force-stops the app (a running app holds its preferences in memory and would write them back over the edit), rewrites that one preference via run-as, relaunches MainActivity by explicit component, and polls dumpsys until the resumed activity really is MainActivity. If it ends up in the editor it says so and returns isError, rather than claiming success it cannot back. Two costs are stated plainly in the tool description: it force-stops the app, and the preference change persists. The XML edit lives in Kotlin (withAutoOpenDisabled), not in on-device sed, because sed could not be tested. The first version used sed '/KEY/d' and corrupted the file on its second run - a previous run had left <map> sharing a line with the boolean, so deleting the line deleted the tag. Fake-based tests were green throughout; only running it twice against a real emulator exposed it. The pure function has an idempotency test that would have caught it. The poll budget also went from 10 to 30 attempts: a cold start after force-stop measured about 6s, and the old 4.5s budget reported a false failure. Launch uses the explicit component because debug builds ship a second LAUNCHER activity (LeakCanary), which makes monkey -c LAUNCHER ambiguous. Verified twice in a row against emulator-5554: both runs reach home, the prefs file stays valid, <map> survives, and the key appears exactly once.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt (1)
79-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd KDoc for public MCP declarations.
Document contracts, side effects, blocking behavior, and cleanup ownership for these public declarations.
mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt#L79-L85: document the server factory, ADB dependency, and polling parameters.mcp/src/main/kotlin/com/itsaky/androidide/mcp/Adb.kt#L3-L16: documentAdbResult,Adb, andSystemAdb, including process execution and output semantics.mcp/src/test/kotlin/com/itsaky/androidide/mcp/McpTestFixture.kt#L16-L19: document embedded-server lifecycle and coroutine behavior.As per coding guidelines, “Public classes, functions, and non-obvious logic must have KDoc or Javadoc documenting contracts, rationale, threading, nullability, side effects, or units.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt` around lines 79 - 85, Add KDoc for the public cogoMcpServer factory in mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt:79-85, describing its ADB dependency, polling parameters and units, server contract, blocking behavior, side effects, and cleanup ownership. Add KDoc for AdbResult, Adb, and SystemAdb in mcp/src/main/kotlin/com/itsaky/androidide/mcp/Adb.kt:3-16, documenting process execution and output semantics. Add KDoc for the embedded-server fixture lifecycle and coroutine behavior in mcp/src/test/kotlin/com/itsaky/androidide/mcp/McpTestFixture.kt:16-19.Source: Coding guidelines
mcp/src/test/kotlin/com/itsaky/androidide/mcp/ServerDescriptionTest.kt (1)
3-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse JUnit Jupiter and Truth for all MCP tests.
Replace
kotlin.testin the seven MCP test files with JUnit Jupiter annotations and Truth assertions. Add the required test dependencies. Use MockK only where mocks are needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/src/test/kotlin/com/itsaky/androidide/mcp/ServerDescriptionTest.kt` around lines 3 - 6, Replace kotlin.test imports and usages with JUnit Jupiter test annotations and Truth assertions across mcp/src/test/kotlin/com/itsaky/androidide/mcp/ServerDescriptionTest.kt:3-6, AutoOpenPreferenceTest.kt:3-6, CogoHomeTest.kt:6-8, CogoInstalledTest.kt:5-7, and SystemAdbTest.kt:3-4; update the remaining two MCP test files similarly. Add the required JUnit Jupiter and Truth test dependencies, and introduce MockK only in tests that require mocking.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp/README.md`:
- Line 18: Change the “Why cogo_home rewrites a preference” heading from level
three to level two so it follows the document’s existing heading hierarchy and
satisfies markdownlint rule MD001.
- Around line 38-41: Update the README statement about fakes to acknowledge that
stateful fakes can round-trip file contents; describe the pure function as
simplifying tests by avoiding filesystem state, not as the only testable design.
Preserve the explanation that the XML edit occurs in Kotlin through
withAutoOpenDisabled.
- Line 12: Update the cogo_home description in the MCP README to replace
“permanently disables” with wording that states auto-open-project remains
disabled until the user enables it again, matching the behavior documented
elsewhere.
In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/Adb.kt`:
- Around line 16-31: Update Adb.run to catch ProcessBuilder startup IOException
and return an explicit AdbResult error instead of propagating it. Add a bounded
timeout for process completion while continuing concurrent stdout/stderr
draining; on timeout, terminate the process and return an error result. Preserve
the interrupt status when joining the stderr drain thread, and add coverage for
a missing executable and a command exceeding the timeout.
---
Nitpick comments:
In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt`:
- Around line 79-85: Add KDoc for the public cogoMcpServer factory in
mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt:79-85, describing
its ADB dependency, polling parameters and units, server contract, blocking
behavior, side effects, and cleanup ownership. Add KDoc for AdbResult, Adb, and
SystemAdb in mcp/src/main/kotlin/com/itsaky/androidide/mcp/Adb.kt:3-16,
documenting process execution and output semantics. Add KDoc for the
embedded-server fixture lifecycle and coroutine behavior in
mcp/src/test/kotlin/com/itsaky/androidide/mcp/McpTestFixture.kt:16-19.
In `@mcp/src/test/kotlin/com/itsaky/androidide/mcp/ServerDescriptionTest.kt`:
- Around line 3-6: Replace kotlin.test imports and usages with JUnit Jupiter
test annotations and Truth assertions across
mcp/src/test/kotlin/com/itsaky/androidide/mcp/ServerDescriptionTest.kt:3-6,
AutoOpenPreferenceTest.kt:3-6, CogoHomeTest.kt:6-8, CogoInstalledTest.kt:5-7,
and SystemAdbTest.kt:3-4; update the remaining two MCP test files similarly. Add
the required JUnit Jupiter and Truth test dependencies, and introduce MockK only
in tests that require mocking.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cf49f25-1ea9-441a-8663-d3ccdb2b2fec
📒 Files selected for processing (11)
mcp/README.mdmcp/TODO.txtmcp/src/main/kotlin/com/itsaky/androidide/mcp/Adb.ktmcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/AutoOpenPreferenceTest.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/CogoHomeTest.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/CogoInstalledTest.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/McpTestFixture.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/PingTest.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/ServerDescriptionTest.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/SystemAdbTest.kt
| |---|---|---| | ||
| | `ping` | none | Returns `pong`. Health check for the transport itself. | | ||
| | `is_cogo_installed` | none | Reports whether `com.itsaky.androidide` is installed on the attached device. | | ||
| | `cogo_home` | none | Brings CoGo to its home screen and confirms it arrived. **Destructive:** force-stops the app and permanently disables auto-open-project. | |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Replace “permanently disables” with persistent wording.
cogo_home disables auto-open-project until the user enables it again. “Permanently” contradicts Lines 35-36.
Proposed fix
-| `cogo_home` | none | Brings CoGo to its home screen and confirms it arrived. **Destructive:** force-stops the app and permanently disables auto-open-project. |
+| `cogo_home` | none | Brings CoGo to its home screen and confirms it arrived. **Destructive:** force-stops the app and disables auto-open-project until the user re-enables it. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `cogo_home` | none | Brings CoGo to its home screen and confirms it arrived. **Destructive:** force-stops the app and permanently disables auto-open-project. | | |
| | `cogo_home` | none | Brings CoGo to its home screen and confirms it arrived. **Destructive:** force-stops the app and disables auto-open-project until the user re-enables it. | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/README.md` at line 12, Update the cogo_home description in the MCP README
to replace “permanently disables” with wording that states auto-open-project
remains disabled until the user enables it again, matching the behavior
documented elsewhere.
| fails. Not knowing is not the same as knowing the app is absent, and collapsing | ||
| the two would make a missing device look like a missing app. | ||
|
|
||
| ### Why `cogo_home` rewrites a preference |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a level-two heading here.
The document starts with a level-one heading. This ### heading skips level two and triggers markdownlint rule MD001.
Proposed fix
-### Why `cogo_home` rewrites a preference
+## Why `cogo_home` rewrites a preference📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Why `cogo_home` rewrites a preference | |
| ## Why `cogo_home` rewrites a preference |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 18-18: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/README.md` at line 18, Change the “Why cogo_home rewrites a preference”
heading from level three to level two so it follows the document’s existing
heading hierarchy and satisfies markdownlint rule MD001.
Source: Linters/SAST tools
Comment on lines
+38
to
+41
| The XML edit happens in Kotlin (`withAutoOpenDisabled`), not in on-device `sed`, | ||
| specifically so it can be tested. The first version used `sed '/KEY/d'` and | ||
| corrupted the file on its second run by deleting the `<map>` tag, which shared a | ||
| line with the boolean. A fake cannot round-trip a file; only a pure function can. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the statement about fakes.
A stateful fake can round-trip file contents. The pure function makes the XML transformation easier to test without filesystem state, but it is not the only testable design.
Proposed wording
-A fake cannot round-trip a file; only a pure function can.
+Using a pure function makes the XML transformation directly testable without filesystem state.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| The XML edit happens in Kotlin (`withAutoOpenDisabled`), not in on-device `sed`, | |
| specifically so it can be tested. The first version used `sed '/KEY/d'` and | |
| corrupted the file on its second run by deleting the `<map>` tag, which shared a | |
| line with the boolean. A fake cannot round-trip a file; only a pure function can. | |
| The XML edit happens in Kotlin (`withAutoOpenDisabled`), not in on-device `sed`, | |
| specifically so it can be tested. The first version used `sed '/KEY/d'` and | |
| corrupted the file on its second run by deleting the `<map>` tag, which shared | |
| a line with the boolean. Using a pure function makes the XML transformation | |
| directly testable without filesystem state. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/README.md` around lines 38 - 41, Update the README statement about fakes
to acknowledge that stateful fakes can round-trip file contents; describe the
pure function as simplifying tests by avoiding filesystem state, not as the only
testable design. Preserve the explanation that the XML edit occurs in Kotlin
through withAutoOpenDisabled.
Comment on lines
+16
to
+31
| override fun run(args: List<String>): AdbResult { | ||
| val process = ProcessBuilder(listOf(executable) + args).start() | ||
|
|
||
| // Drain stderr on its own thread: filling one pipe buffer while the other | ||
| // goes unread deadlocks the child. | ||
| val stderr = StringBuilder() | ||
| val drain = | ||
| Thread { | ||
| process.errorStream.bufferedReader().forEachLine { stderr.appendLine(it) } | ||
| } | ||
| drain.start() | ||
|
|
||
| val stdout = process.inputStream.bufferedReader().readText() | ||
| drain.join() | ||
|
|
||
| return AdbResult(exitCode = process.waitFor(), stdout = stdout, stderr = stderr.toString()) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Handle adb startup failures and command timeouts locally.
ProcessBuilder.start() throws when adb is absent or cannot start. The exception escapes the tool handler instead of producing an MCP error result.
The current reads and waitFor() have no timeout. A stalled adb process can block an MCP request indefinitely. Catch IOException, use a bounded process wait with concurrent stream draining, terminate timed-out processes, and preserve interruption when joining drain threads.
Add tests for a missing executable and a command that exceeds the timeout.
As per coding guidelines, “Catch recoverable I/O, parsing, IPC, git, and plugin failures locally; convert them into explicit error states, never allow unexpected exceptions to reach the global GlitchTip crash handler.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/Adb.kt` around lines 16 - 31,
Update Adb.run to catch ProcessBuilder startup IOException and return an
explicit AdbResult error instead of propagating it. Add a bounded timeout for
process completion while continuing concurrent stdout/stderr draining; on
timeout, terminate the process and return an error result. Preserve the
interrupt status when joining the stderr drain thread, and add coverage for a
missing executable and a command exceeding the timeout.
Source: Coding guidelines
LeakCanary declares leakcanary.internal.activity.LeakLauncherActivity as a
second MAIN/LAUNCHER activity-alias, so every debug build advertised two
launcher entries. Any generic launch resolved to the system ResolverActivity
instead of the IDE:
cmd package resolve-activity -a MAIN -c LAUNCHER com.itsaky.androidide
-> android/com.android.internal.app.ResolverActivity
That breaks anything driving the app generically - monkey -c LAUNCHER, tapping
the icon, UI automation - and debug builds are the norm for on-device tooling.
LeakCanary gates the alias on android:enabled="@bool/leak_canary_add_launcher_icon",
so overriding that boolean in the debug source set is the sanctioned fix; no
manifest-merger override is needed.
Leak reports are unaffected. Only the launcher alias is disabled; LeakActivity
stays registered and is still reachable from LeakCanary's notification.
Verified on emulator-5554 after rebuild and reinstall:
resolve-activity ResolverActivity -> .activities.SplashActivity
enabled LAUNCHER activities 2 -> 1 (SplashActivity only)
monkey -p PKG -c LAUNCHER now launches the IDE
LeakActivity still registered
PRIORITIES.md supersedes TODO.txt: the rubric, all 25 items scored, and the reachability facts measured against a real emulator rather than read off the source. Three list corrections came out of that - items 1-4 are fragments not activities, item 5 must target com.termux.app.TermuxActivity, and item 19 cannot use an intent at all. The headline is that the top three items need no UI whatsoever. adbFailure becomes internal so every adb-backed tool shares one definition of 'a failed call is not a negative answer'.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp/PRIORITIES.md`:
- Around line 1-4: Rebase the ADFA-5083-mcp feature branch onto the latest
origin/stage so PR 1659 is based on the branch it targets, resolving any
conflicts and preserving the intended changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8387707f-c913-4188-b611-e3806221f9a5
📒 Files selected for processing (2)
mcp/PRIORITIES.mdmcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt
Comment on lines
+1
to
+4
| # cogo-mcp tool priorities | ||
|
|
||
| Supersedes `TODO.txt`. Scored 2026-08-11 against a real emulator, not from | ||
| reading the source alone -- several entries changed once measured. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
base_ref="$(gh pr view --json baseRefName --jq '.baseRefName')"
test "$base_ref" = "stage"
test "$base_ref" != "main"
git merge-base --is-ancestor origin/stage HEADRepository: appdevforall/CodeOnTheGo
Length of output: 235
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- repository state ---'
git status --short --branch || true
printf '%s\n' '--- remotes ---'
git remote -v || true
printf '%s\n' '--- current commit ---'
git rev-parse HEAD || true
printf '%s\n' '--- local and remote stage refs ---'
git show-ref | grep -E '(^|/)(stage)$' || true
printf '%s\n' '--- pull request metadata ---'
gh pr view --repo appdevforall/CodeOnTheGo --json number,headRefName,baseRefName,headRepositoryOwner,headRefOid 2>&1 || trueRepository: appdevforall/CodeOnTheGo
Length of output: 1065
🏁 Script executed:
#!/usr/bin/env bash
set -u
head_sha="$(git rev-parse HEAD)"
stage_sha="$(git rev-parse origin/stage)"
printf 'HEAD=%s\norigin/stage=%s\n' "$head_sha" "$stage_sha"
if git merge-base --is-ancestor origin/stage HEAD; then
echo 'stage_is_ancestor=true'
else
echo 'stage_is_ancestor=false'
fi
printf '%s\n' '--- pull requests associated with HEAD ---'
gh api "repos/appdevforall/CodeOnTheGo/commits/${head_sha}/pulls" \
-H 'Accept: application/vnd.github+json' \
--jq '.[] | {number, baseRefName: .base.ref, headRefName: .head.ref, headSha: .head.sha}' 2>&1 || trueRepository: appdevforall/CodeOnTheGo
Length of output: 450
Rebase the feature branch onto stage.
PR 1659 targets stage, but origin/stage is not an ancestor of ADFA-5083-mcp.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/PRIORITIES.md` around lines 1 - 4, Rebase the ADFA-5083-mcp feature
branch onto the latest origin/stage so PR 1659 is based on the branch it
targets, resolving any conflicts and preserving the intended changes.
Source: Coding guidelines
The three highest-scoring items in PRIORITIES.md, built in parallel. None of
them drives the UI - they are adb shell reads - which is exactly why they ranked
top: highest agent value and lowest cost at the same time.
list_projects mirrors ProjectValidations.kt one level deep, so it returns only
what the IDE would actually open. Stray .cgt archives and Flutter directories
are excluded; on the test device that is 2 of 5 entries, and both surviving
project names contain spaces. The whole filter runs in one shell command so no
filename ever crosses the adb boundary and has to be re-quoted to survive.
list_templates reads the .cgt archives in place with unzip -p rather than
pulling 1.6MB. template.json is not strict JSON - it carries unquoted keys like
{identifier: "APP_NAME"} - and the corruption is inconsistent across templates,
so a JSON parser would have worked on some and thrown on others. Regex it is.
list_project_files resolves the open project from the last-opened-project
preference, then lists it. run-as cannot read /storage/emulated/0, so only the
preference read uses run-as and the find runs as the shell user. The sentinel
also comes back XML-escaped, so both spellings are handled.
All three report "no data" distinctly from "adb failed": an empty projects dir,
an un-onboarded device, and no open project are answers, not failures.
69 tests, 0 failures. Verified through the real MCP transport against
emulator-5554: 2 projects, 9 templates, and a correct non-error "no project
open".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
mcp/src/test/kotlin/com/itsaky/androidide/mcp/ProjectsTest.kt (1)
25-30: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the
/bin/shdependency so the test does not fail on hosts without it.
listingOfruns the shipped command through/bin/shon the host. Themcpmodule shipsgradlew.bat, so Windows is a supported developer environment and/bin/shis absent there. The test then fails with an execution error rather than a meaningful assertion.Add a JUnit assumption so these tests are skipped when
/bin/shis not executable, and keep the pure-parsing tests running everywhere.♻️ Proposed change
private fun listingOf(dir: String): ProjectListing { + assumeTrue(File("/bin/sh").canExecute(), "requires a POSIX shell on the host") val result = SystemAdb(executable = "/bin/sh").run(listOf("-c", listProjectsCommand(dir)))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/src/test/kotlin/com/itsaky/androidide/mcp/ProjectsTest.kt` around lines 25 - 30, Add a JUnit assumption at the start of listingOf to require that /bin/sh exists and is executable, causing command-based project-listing tests to skip on unsupported hosts while leaving pure parsing tests unchanged.mcp/src/main/kotlin/com/itsaky/androidide/mcp/Templates.kt (2)
85-98: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider collapsing the per-template reads into fewer adb round trips.
The loop runs one
adb shell unzip -pper archive and then one more per declared template.core.cgtalone declares nine templates, so a listing costs ten process spawns plus ten device shell invocations. Each spawn is measurable on a slow device or over TCP adb.A single
run-as ... sh -cscript that unzips the index and everytemplate/template.jsonin one pass, with a delimiter between members, reduces this to two round trips. Defer this if the current latency is acceptable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/Templates.kt` around lines 85 - 98, In the archive-processing flow around parseTemplateIndex, reduce per-template adb.run invocations by batching index and template.json reads through a single run-as shell script per archive, using unambiguous delimiters to separate members. Preserve adbFailure handling and the existing describe(path, template.stdout) behavior, and retain the current approach if latency is considered acceptable.
68-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the three public tool entry points. Each new MCP tool is backed by a public function that returns either a plain text answer or an error
CallToolResult. None of the three documents that distinction, and each one encodes a non-obvious rule about which device states are answers rather than failures. Add KDoc to each.
mcp/src/main/kotlin/com/itsaky/androidide/mcp/Templates.kt#L68-L72: document that an empty or absent templates directory returns a plain answer, and that any non-zero adb exit returns an error result.mcp/src/main/kotlin/com/itsaky/androidide/mcp/Projects.kt#L79-L86: document that an absent projects directory returns a plain answer, and that a non-zero adb exit returns an error result.mcp/src/main/kotlin/com/itsaky/androidide/mcp/ProjectFiles.kt#L107-L124: document that no open project returns a plain answer, that the listing is truncated atMAX_LISTED_FILES, and that a non-zero adb exit returns an error result.As per coding guidelines: "Public classes, functions, and non-obvious logic must have KDoc or Javadoc documenting contracts, rationale, threading, nullability, side effects, or units."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/Templates.kt` around lines 68 - 72, Add KDoc to the public functions listTemplates in mcp/src/main/kotlin/com/itsaky/androidide/mcp/Templates.kt:68-72, listProjects in mcp/src/main/kotlin/com/itsaky/androidide/mcp/Projects.kt:79-86, and the project-files entry point in mcp/src/main/kotlin/com/itsaky/androidide/mcp/ProjectFiles.kt:107-124. Document that empty or absent templates/projects directories and no open project return plain answers, that ProjectFiles truncates listings at MAX_LISTED_FILES, and that any non-zero adb exit returns an error CallToolResult.Source: Coding guidelines
mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt (1)
141-143: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove all blocking ADB handlers to
Dispatchers.IO.
kotlin-sdk-server:0.15.0runs handlers onDispatchers.Defaultby default.SystemAdb.runwaits for external processes, so wrap all ADB-backed handlers inwithContext(Dispatchers.IO)to avoid occupying Default worker threads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt` around lines 141 - 143, Update the ADB-backed handler registrations, including the handler invoking listProjects(adb), to execute their blocking SystemAdb.run work inside withContext(Dispatchers.IO). Apply this consistently to every handler that accesses ADB while preserving each handler’s existing results and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/Templates.kt`:
- Around line 63-66: Update readMemberCommand to shell-quote both archive and
member using the existing shellQuoted helper from ProjectFiles; promote that
helper to internal visibility so it can be reused. Add coverage for an archive
name containing spaces and a member path containing shell metacharacters,
verifying the generated command remains safely quoted and parses correctly.
---
Nitpick comments:
In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.kt`:
- Around line 141-143: Update the ADB-backed handler registrations, including
the handler invoking listProjects(adb), to execute their blocking SystemAdb.run
work inside withContext(Dispatchers.IO). Apply this consistently to every
handler that accesses ADB while preserving each handler’s existing results and
behavior.
In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/Templates.kt`:
- Around line 85-98: In the archive-processing flow around parseTemplateIndex,
reduce per-template adb.run invocations by batching index and template.json
reads through a single run-as shell script per archive, using unambiguous
delimiters to separate members. Preserve adbFailure handling and the existing
describe(path, template.stdout) behavior, and retain the current approach if
latency is considered acceptable.
- Around line 68-72: Add KDoc to the public functions listTemplates in
mcp/src/main/kotlin/com/itsaky/androidide/mcp/Templates.kt:68-72, listProjects
in mcp/src/main/kotlin/com/itsaky/androidide/mcp/Projects.kt:79-86, and the
project-files entry point in
mcp/src/main/kotlin/com/itsaky/androidide/mcp/ProjectFiles.kt:107-124. Document
that empty or absent templates/projects directories and no open project return
plain answers, that ProjectFiles truncates listings at MAX_LISTED_FILES, and
that any non-zero adb exit returns an error CallToolResult.
In `@mcp/src/test/kotlin/com/itsaky/androidide/mcp/ProjectsTest.kt`:
- Around line 25-30: Add a JUnit assumption at the start of listingOf to require
that /bin/sh exists and is executable, causing command-based project-listing
tests to skip on unsupported hosts while leaving pure parsing tests unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e679ec3c-1a46-44e5-b902-249f44c79957
📒 Files selected for processing (11)
mcp/README.mdmcp/TODO.txtmcp/src/main/kotlin/com/itsaky/androidide/mcp/CogoMcpServer.ktmcp/src/main/kotlin/com/itsaky/androidide/mcp/ProjectFiles.ktmcp/src/main/kotlin/com/itsaky/androidide/mcp/Projects.ktmcp/src/main/kotlin/com/itsaky/androidide/mcp/Templates.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/PingTest.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/ProjectFilesTest.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/ProjectsTest.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/ServerDescriptionTest.ktmcp/src/test/kotlin/com/itsaky/androidide/mcp/TemplatesTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- mcp/src/test/kotlin/com/itsaky/androidide/mcp/ServerDescriptionTest.kt
- mcp/TODO.txt
Comment on lines
+63
to
+66
| private fun readMemberCommand( | ||
| archive: String, | ||
| member: String, | ||
| ) = "run-as $COGO_PACKAGE sh -c \"unzip -p $TEMPLATES_DIR/$archive $member\"" |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Quote the archive name and member path in the device shell command.
readMemberCommand interpolates archive and member into the shell string without quoting. Both values are untrusted input from the device: archive comes from ls output, and member is built from a "path" value read out of an archive's templates.json.
Two consequences:
- An archive file name that contains a space, for example
my templates.cgt, producesunzip -p .../my templates.cgt templates.json. The device shell splits this into the wrong arguments and the read fails. - A name or
pathvalue that contains shell metacharacters, for example`id`or;rm -rf ..., is executed by the device shell insiderun-as $COGO_PACKAGE. A plugin that drops a crafted.cgtfile, or a crafted archive index, gains command execution in the app sandbox.
ProjectFiles.kt already solves this with shellQuoted. Promote that helper to internal and reuse it here.
🔒️ Proposed fix
In mcp/src/main/kotlin/com/itsaky/androidide/mcp/ProjectFiles.kt:
-private fun shellQuoted(path: String) = "'" + path.replace("'", """'\''""") + "'"
+internal fun shellQuoted(path: String) = "'" + path.replace("'", """'\''""") + "'"In mcp/src/main/kotlin/com/itsaky/androidide/mcp/Templates.kt:
private fun readMemberCommand(
archive: String,
member: String,
-) = "run-as $COGO_PACKAGE sh -c \"unzip -p $TEMPLATES_DIR/$archive $member\""
+) = "run-as $COGO_PACKAGE sh -c \"unzip -p ${shellQuoted("$TEMPLATES_DIR/$archive")} ${shellQuoted(member)}\""Add a test with an archive name that contains a space and a path value that contains a metacharacter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/src/main/kotlin/com/itsaky/androidide/mcp/Templates.kt` around lines 63 -
66, Update readMemberCommand to shell-quote both archive and member using the
existing shellQuoted helper from ProjectFiles; promote that helper to internal
visibility so it can be reused. Add coverage for an archive name containing
spaces and a member path containing shell metacharacters, verifying the
generated command remains safely quoted and parses correctly.
Refactor only - no behaviour change, and the test files are untouched, which is
what proves it.
The three list tools arrived one-per-file with pure parsing functions, but
CogoMcpServer.kt had grown to 264 lines holding server construction, tool
registration, cogo_home and is_cogo_installed. Bring the older code in line:
CogoHome.kt and CogoInstalled.kt now sit beside Projects.kt, Templates.kt and
ProjectFiles.kt, and CogoMcpServer.kt is registration only.
The duplication being collapsed is real, not anticipated. PREFS_PATH and the
preferences-read command were byte-identical in two files, the
run-as ... sh -c wrapper was hand-built in five places, and
run(listOf("shell", ...)) appeared at a dozen call sites. Those move to
CogoDevice.kt and Adb.shell().
Deliberately not collapsed: Projects.kt trims line ends while the others trim
both. That reads as duplication but the difference is load-bearing - trimEnd
protects a project name with a leading space.
Verified against emulator-5554 after the refactor, since fakes cannot prove
command construction: all six tools return what they did before, and cogo_home
still preserves an unrelated preference while writing its own exactly once.
Three independent reviews (correctness, silent-failure, test-quality) against a real emulator, with mutation testing. Fourteen defects, three of them critical: a confirmed command injection through plugin-supplied template archives, a path where cogo_home overwrites every user preference and reports success, and list_project_files describing a project that is not open. They outrank every remaining tool on the list. The faults live in the shared command-building idiom, so each new tool would copy them, and nothing runs these tests in CI to catch a regression. Also adds a seventh ask of the CoGo app. Unlike the other six it is not a convenience: ide_last_project is written on open and never cleared, so there is no on-device signal for 'nothing is open' and no way for the tool to answer honestly.
Captures where this stands so it can be picked up cold: six working tools, the fourteen review defects that now outrank the remaining backlog, the three open decisions, and the flox invocation that is easy to get wrong. Also records that emulator-5554 is not pristine - it carries a locally rebuilt debug APK, auto-open-project is permanently disabled by cogo_home, and only two of the five directories under CodeOnTheGoProjects are valid projects. Each of those would otherwise look like a bug to whoever resumes.
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.
A host-side MCP server that lets an AI coding agent probe Code On The Go
without hand-rolling adb commands, plus the first CoGo-aware tool.
Host-side rather than in-APK: nothing ships to the device, so this carries no
app risk and iterates independently of releases.
Tools
pingpong. Health check for the transport itself.is_cogo_installedcom.itsaky.androidideis installed on the attached device.cogo_homelist_projects/storage/emulated/0/CodeOnTheGoProjects.list_templateslist_project_filesmcp/PRIORITIES.mdscores all 25 candidate tools against a rubric and recordsthe reachability facts measured on a real device. The three
list_*tools arethe top three: they drive no UI at all, so they are simultaneously the highest
value and the lowest cost.
Three findings from building them are worth a reviewer's attention:
template.jsonis not strict JSON ({identifier: "APP_NAME"}), and thecorruption is inconsistent across templates - a JSON parser would work on
some and throw on others, which is a nastier failure mode than uniform
breakage. Hence regex extraction.
run-ascannot read/storage/emulated/0. Only the preference read usesrun-as; thefindruns as the shell user. The obvious "run-as everything"shape would have silently returned an empty listing.
do.
list_projectsdoes its whole filter in one shell command so no filenameever crosses the adb boundary needing to be re-quoted.
All three distinguish "no data" from "adb failed": an empty projects directory,
an un-onboarded device, and no open project are answers, not errors.
Why
cogo_homerewrites a preferenceLaunching the app does not reach home.
MainActivity.onCreatecallstryOpenLastProject()andautoOpenProjectsdefaults to true, so the realpath is Splash -> Onboarding -> MainActivity -> Editor. Clearing
ide_last_projectdoes not help either:tryOpenLastProject()falls back tovalidProjects.maxByOrNull { it.lastModified() }, so it opens somethingregardless. Only the boolean prevents it.
So the tool force-stops the app (a running app holds its preferences in memory
and would write them back over the edit), rewrites that one key via
run-as,relaunches
MainActivityby explicit component, and polls until the resumedactivity really is
MainActivity. Landing in the editor returnsisErrorrather than a success it cannot back.
Two costs, both stated in the tool's own description: it force-stops the app,
and the preference change persists. It also needs a debuggable build for
run-as.The XML edit is a pure Kotlin function, not on-device
sed, becausesedcouldnot be tested - see the bug note below.
Self-description
initializereturns instructions describing what the server drives and theadb caveats; every tool carries a
title; andlistChangedisfalsebecause the tool set is fixed at construction - the previous
truepromised anotifications/tools/list_changedthat would never arrive.ServerDescriptionTestpins all three, so a tool added without a title fails the build.
Review notes
settings.gradle.kts,gradle/libs.versions.tomland.mcp.jsonare unchanged; the branch diff isonly
docs/andmcp/.mcp/is a standalone Gradle build, invisible to theAndroid build, the same way
apk-viewer-plugin/is.kotlin-sdk-server:0.15.0ships
kotlin-stdlib 2.4.0metadata that the catalog's 2.3.0 compilerrejects outright.
SdkResolutionTestexists to make that fail loudly and onits own rather than tangled up in a protocol error.
Adbis afun interfaceover the process boundary. Tool logic istested with fakes;
SystemAdbis exercised against/bin/echoand/bin/sh. No emulator or device is needed to run the suite.is_cogo_installedreturns
isErrorwhen adb exits non-zero rather than reporting "notinstalled" - collapsing the two would make a missing device look like a
missing app. There is a test pinning exactly this.
pm list packagesmatchessubstrings, so
com.itsaky.androidide.debugmust not satisfy a query forcom.itsaky.androidide; andadb shellemits CRLF, so an untrimmed comparewould silently never match.
.mcp.jsonis deliberately not modified. It is committed and shared; anhttpentry aimed at a process nobody started makes Claude Code report aconnection failure at startup for every developer.
mcp/README.mddocumentsthe snippet for local use.
non-loopback interface would require TLS and authentication first.
mcp/uses tabs and
spotlessApplyruns from the repo root.spotlessCheckpasses.A bug worth reading about
cogo_home's preference edit was originally on-devicesed. Fake-based testswere green, and it corrupted the prefs file on its second run: a previous
run had left
<map>sharing a line with the boolean, sosed '/KEY/d'deletedthe tag along with it.
A fake cannot round-trip a file, so no fake-based test could have caught it.
Only running it twice against a real emulator did. The fix moves the edit into a
pure Kotlin function (
withAutoOpenDisabled) that has an idempotency testasserting
f(f(x)) == f(x).The same run exposed a second bug: the poll budget was ~4.5s against a ~6s cold
start, so it reported a false failure. Now 30 attempts.
Verification
69 tests, 0 skipped, 0 failures from a clean build. Tests drive the server
through the real MCP client over Streamable HTTP rather than calling handlers
directly.
Also confirmed against a real device (
emulator-5554) through the runningserver, since fakes prove the parsing but only a device proves the adb command
string:
matching
adb shell pm list packages | grep itsaky.Reviewing by commit
The history is ordered for review: scaffolding, then transport, then the first
real tool. Each commit builds and its tests pass.
Next
More CoGo-aware tools - launch state, current activity, project and build
state. Device/serial selection is not yet supported;
SystemAdbshells out towhatever
adbpicks as the default device, and reports adb's own error whenthat is ambiguous.
Design spec:
docs/superpowers/specs/2026-08-10-mcp-server-design.mdPlan:
docs/superpowers/plans/2026-08-11-mcp-server-hello-world.md