ADFA-4357 Add agent & contributor documentation set#1422
ADFA-4357 Add agent & contributor documentation set#1422hal-eisen-adfa wants to merge 26 commits into
Conversation
Add a coordinated set of Markdown docs to onboard both human and AI contributors and to capture the project's architectural decisions. - CLAUDE.md: operational guide for Claude Code (build/test commands, ABI flavors, project constraints); points to ARCHITECTURE.md for architecture rather than duplicating it. - AGENTS.md: operational rules for agents (CI-vs-local, Jira CLI, SonarQube MCP, git message handling); persistence rule now points to ARCHITECTURE.md. - ARCHITECTURE.md: single source of truth for module layout, layering & data flow (UDF), dependency rules, tech stack, state management, and the testing strategy. - REVIEW.md: code-review coaching (exception handling vs the Sentry crash wrapper, LeakCanary leaks, StrictMode, OWASP, tests/coverage, analytics, duplication, docstrings, strings.xml). - SECURITY.md: how to avoid introducing new SonarQube/Snyk/Semgrep blocker findings; vulnerability classes for an Android/Kotlin IDE. - docs/adr/: 8 Architecture Decision Records (MADR/Nygard) plus an index covering persistence-without-Room, on-device builds via the Gradle Tooling API, the vendored toolchain, embedded Termux, per-ABI flavors, Koin DI, the StrictMode whitelist engine, and retaining the com.itsaky.androidide namespace.
|
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:
📝 WalkthroughWalkthroughAdds repository-wide documentation for operating rules, architecture conventions, review standards, security guidance, and ADRs covering persistence, build, runtime, DI, flavors, StrictMode, namespace retention, and Compose. ChangesProject Documentation Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/adr/0005-per-abi-product-flavors.md (1)
39-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrailing artifact at end of file.
Line 39 contains a stray
39character that appears to be a formatting artifact or incomplete truncation.Verify the file ends cleanly. If this is the intended end, remove the stray character.
🤖 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/adr/0005-per-abi-product-flavors.md` at line 39, The file docs/adr/0005-per-abi-product-flavors.md has a stray character "39" at the end that appears to be a formatting artifact. Locate the end of the file and remove this trailing character to ensure the markdown file ends cleanly without any extraneous content.
🧹 Nitpick comments (2)
REVIEW.md (1)
80-80: 💤 Low valueMinor: replace "exactly" with more specific verb.
LanguageTool flags "exactly" as an over-used intensifier. Consider "are" or "represent" depending on intended emphasis, or rephrase to avoid the intensifier.
Example:
-- **No duplication.** If you copy-pasted a block, extract a function/extension into the right `common`/`utils` module. Before adding a helper, grep — we likely already have it. Repeated literals/magic numbers → named constants. +- **No duplication.** If you copy-pasted a block, extract a function/extension into the right `common`/`utils` module. Before adding a helper, grep — we likely already have it. Repeated literals/magic numbers become named constants.Alternatively, keep the intensity but rephrase: "those are the error paths the crash wrapper would otherwise catch" → "those represent the error paths the crash wrapper would otherwise catch".
🤖 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 `@REVIEW.md` at line 80, In the REVIEW.md file, locate the sentence containing "those are exactly what the crash wrapper would otherwise catch in production" and remove the over-used intensifier "exactly" by replacing it with a more specific verb such as "represent" or rephrase the sentence to eliminate the intensifier entirely (for example, change "those are exactly what" to "those represent what" or similar phrasing that conveys the same meaning without the weak intensifier).docs/adr/0005-per-abi-product-flavors.md (1)
9-9: 💤 Low valueMinor: replace "very large" with a stronger adjective for clarity.
LanguageTool flags "very large" as an over-used intensifier. Consider "substantial", "sizable", or "prohibitive" depending on emphasis.
Example:
-Code On The Go is distributed primarily as a **direct APK download** from the App Dev for All website, not exclusively through Google Play, so we cannot rely on Play's automatic per-ABI splitting to slim downloads. +Code On The Go is distributed primarily as a **direct APK download** from the App Dev for All website, not exclusively through Google Play, so we cannot rely on Play's automatic per-ABI splitting to slim substantial downloads.🤖 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/adr/0005-per-abi-product-flavors.md` at line 9, In the file docs/adr/0005-per-abi-product-flavors.md, replace the phrase "very large" with a stronger, more specific adjective in the sentence describing universal APK size. Consider using alternatives such as "substantial", "sizable", or "prohibitive" to provide clearer emphasis on why per-ABI splitting is necessary, as "very large" is flagged as an over-used intensifier. Choose the adjective that best conveys the intended severity of the size concern in the context of direct APK distribution.
🤖 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.
Outside diff comments:
In `@docs/adr/0005-per-abi-product-flavors.md`:
- Line 39: The file docs/adr/0005-per-abi-product-flavors.md has a stray
character "39" at the end that appears to be a formatting artifact. Locate the
end of the file and remove this trailing character to ensure the markdown file
ends cleanly without any extraneous content.
---
Nitpick comments:
In `@docs/adr/0005-per-abi-product-flavors.md`:
- Line 9: In the file docs/adr/0005-per-abi-product-flavors.md, replace the
phrase "very large" with a stronger, more specific adjective in the sentence
describing universal APK size. Consider using alternatives such as
"substantial", "sizable", or "prohibitive" to provide clearer emphasis on why
per-ABI splitting is necessary, as "very large" is flagged as an over-used
intensifier. Choose the adjective that best conveys the intended severity of the
size concern in the context of direct APK distribution.
In `@REVIEW.md`:
- Line 80: In the REVIEW.md file, locate the sentence containing "those are
exactly what the crash wrapper would otherwise catch in production" and remove
the over-used intensifier "exactly" by replacing it with a more specific verb
such as "represent" or rephrase the sentence to eliminate the intensifier
entirely (for example, change "those are exactly what" to "those represent what"
or similar phrasing that conveys the same meaning without the weak intensifier).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 636711f5-1206-408a-9727-feafce7276a9
📒 Files selected for processing (14)
AGENTS.mdARCHITECTURE.mdCLAUDE.mdREVIEW.mdSECURITY.mddocs/adr/0001-persistence-without-room.mddocs/adr/0002-on-device-builds-via-gradle-tooling-api.mddocs/adr/0003-vendored-forked-desktop-toolchain.mddocs/adr/0004-embedded-termux-runtime.mddocs/adr/0005-per-abi-product-flavors.mddocs/adr/0006-koin-dependency-injection.mddocs/adr/0007-strictmode-whitelist-engine.mddocs/adr/0008-retain-androidide-namespace.mddocs/adr/README.md
Promote accessibility from a proposed item to an enforced review section and add a parallel contextual-help (long-press 3-tier) rule, both keyed to existing patterns (ADFA-2667 screen-reader work, the idetooltips module). - REVIEW.md: new sections for content-description coverage and long-press help; matching 60-second-checklist entries; renumber trailing sections. - idetooltips/README.md: state the long-press-for-help-everywhere principle and the three-tier (tooltip / tooltip / web page) help model.
- ADR 0009: new IDE UI is Jetpack Compose, no new XML View screens; the UDF/Koin/StateFlow stack is unchanged. Indexed in docs/adr/README.md. - ARCHITECTURE.md: tech-stack UI row + overview now point to ADR 0009 instead of claiming the IDE is 'Not Compose'. - REVIEW.md: new Compose-only rule in Architecture alignment; accessibility (§8) now gives View + Compose forms for each rule (semantics, clearAndSetSemantics, the HardcodedText lint gap); contextual help (§9) notes idetooltips has no Compose entry point yet (displayTooltipOnLongPress is View-based); promote Offline-first from proposed to an accepted section.
…idge The Compose-only mandate (ADR 0009) and the long-press-everywhere rule (REVIEW.md section 9) need a Compose entry point into the View-based idetooltips system, which does not exist yet. Reference the follow-up ticket from both docs so the gap is tracked, not forgotten. Docs only.
…4382 The README's usage examples document a showIDETooltip() API that no longer exists (real API: TooltipManager.showTooltip / displayTooltipOnLongPress) and claim a Room store the module doesn't use (it's raw SQLite). Add a banner so contributors trust the code until the refresh lands. Docs only.
Leave idetooltips/README.md untouched on this PR. Removes both the design-principle section and the staleness banner added earlier; the README refresh is handled wholesale in ADFA-4382 instead.
- REVIEW.md: new Code-quality rule + 60-second-checklist entry requiring a change to update any module README/ARCHITECTURE.md/ADR it affects, or leave a tracked note. - AGENTS.md: one-line operational pointer to the REVIEW.md rule, so agents that read AGENTS.md (but not REVIEW.md) still apply it.
Tighten prose across CLAUDE.md, AGENTS.md, ARCHITECTURE.md, REVIEW.md, and the ADRs — cut hedging, doubled phrasings, and restated context; no facts, paths, commands, or decisions changed. Also: - REVIEW.md §9: drop the stale showIDETooltip reference in the intro. - ARCHITECTURE.md: reconcile the data-flow UI note with ADR 0009 (existing UI is Views; new UI is Compose) instead of a flat 'not Compose'.
- Experimental feature flag: clarify it's a user-facing early-access opt-in (singular flag), not a kill switch for us to disable features in the field. - Remove the performance-budget proposal; captured as ADFA-4383 instead.
Move it out of 'Open for discussion' into a numbered review section; gate not-yet-stable features behind the user-facing early-access flag. Renumber PR hygiene to §13.
…scussion section The MIN_SDK guard concern doesn't arise in practice; remove the item. It was the last proposal, so remove the empty section scaffolding too. REVIEW.md now ends at §13 PR hygiene.
| New persistence uses **raw SQLite** (`SQLiteOpenHelper` / `SQLiteDatabase`) or the **filesystem / preferences**. Room is **not** used for new code. | ||
|
|
||
| The one exception is the **Recent Projects** feature (`app/src/main/java/com/itsaky/androidide/roomData/recentproject/`, `@Database version = 4`), which predates this decision and is grandfathered in. Do not extend it with new entities or tables. (`idetooltips` still declares unused Room Gradle deps; its tooltip store is raw SQLite — remove those deps.) |
There was a problem hiding this comment.
Not true. AFAIK, raw SQLite is only used for indexing symbols from libraries (and the web server) because we want granular control over the index schema and queries, while also reducing the number of object allocations and overall memory use. Room should be preferred in most cases, but raw SQLite can be used for similar use cases.
There was a problem hiding this comment.
You're right, thanks for the precise correction. Reframed entirely in af92f6c5: Room is now the default, and raw SQLite is documented as the justified exception — symbol indexing (granular schema, fewer allocations), the local web server, and the prebuilt read-only tooltip/help DBs. Renamed the file to 0001-prefer-room-for-persistence.md; Recent Projects is now shown as the example of the Room default, not an exception.
There was a problem hiding this comment.
This could probably be removed if we add a statement somewhere that Room should be preferred, while raw SQLite should be used when the use case is justified (control over the actual schema, performance-critical code, etc.).
There was a problem hiding this comment.
Done in af92f6c5 — the ADR now states Room is preferred and lists the specific conditions under which raw SQLite is justified, rather than framing it as the default.
There was a problem hiding this comment.
This is almost entirely incorrect, but I do believe that we should have a document that distinguishes between the vendored toolchains, their use cases and how they differ from the tooling api toolchains.
- Modules in
composite-build/build-deps*are included in the final APK. They're part of the IDE's runtime and are used to provide certain features to the IDE. - They're kept in composite-builds to reduce build times - composite builds are a separate Gradle build from the main build, they're not built unless their sources are changed, even when we do a clean-build in the main build. Read more here: https://docs.gradle.org/current/userguide/composite_builds.html
- The toolchains in composite builds (
javac,jdk-compiler,jdk-jdeps,jdt, etc.) are used in the IDE's runtime. They're NOT used for providing ANY tooling API features, nor they're used to run the tooling API itself. For example, thejavac,jdk-compilercomposite builds and used to provide Java LSP features, like parsing and analyzing Java source files within the IDE's runtime - without having to invoke the JDK's javac viaProcessBuilder, or building our own custom parser/analyzer. - The tooling API is invoked using a full-blown JDK/JVM using
ProcessBuilderand runs as a daemon. The JDK used to run the tooling API and invoke Gradle builds is built from our appdevforall/terminal-packages repository and packaged within our terminal bootstrap packages. The IDE communicates with the tooling API (running as a daemon) using the JSONRpc protocol (the models/interfaces are defined insubprojects/tooling-api,subprojects/tooling-api-modelandsubprojects/tooling-api-eventsmodules).
There was a problem hiding this comment.
This was the big one — thank you for the detailed writeup. I rewrote the ADR to match it and verified each point against the code (settings.gradle.kts substitution, ToolingServerRunner's ProcessBuilder daemon, ToolingApiLauncher's lsp4j JSON-RPC). It now states that composite-build/build-deps* ship in the APK as IDE runtime (e.g. Java LSP via javac/jdk-compiler/jdt), are kept in composite builds for build-time caching, and are not used by the Tooling API — which is a separate out-of-process JDK from appdevforall/terminal-packages over JSON-RPC. Added a prominent callout drawing that distinction, and fixed two cross-reference lines in ADR 0002 that conflated the two. c9ebce4d
|
|
||
| Code On The Go is the rebranded successor to **AndroidIDE**. The product name, branding, and assets changed, but the inherited codebase carries the original identity deeply: the application id and Gradle namespace are `com.itsaky.androidide` (`BuildConfig.PACKAGE_NAME`), `rootProject.name` is `AndroidIDE`, plus many thousands of references, the generated `R` class, the manifest, package-qualified vendored substitutions, signing identity, and existing installs in the field. | ||
|
|
||
| Changing an Android **application id** breaks the update path for installed users (a different app id is a different app) and disrupts signing/identity continuity. A rename of this size also ripples through the vendored `com.itsaky.androidide.build:*` substitutions ([ADR 0003](0003-vendored-forked-desktop-toolchain.md)). |
There was a problem hiding this comment.
Clarification worth adding: the major reason we have the package name set to com.itsaky.androidide is that it requires us to re-build terminal packages with the updated package name. Also, it is not possible to change package name incrementally (i.e. change in terminal packages first, then the app - or vice-versa). The change needs to be a big-bang change such that we have the package name changed in the terminal packages as well as this codebase (the application) as an atomic change.
There was a problem hiding this comment.
Added your point as the decisive reason in ac5bba54: the package name is baked into the terminal bootstrap packages, so a rename must be an atomic big-bang change across terminal-packages + app, not incremental.
| @@ -0,0 +1,37 @@ | |||
| Code On The Go is an Android IDE — it lets users edit, build, and deploy their own Android apps on-device, like Eclipse or VSCode. | |||
|
|
|||
| There is at least one Android emulator available. Find it with `adb devices -l | grep -v offline`, then use the `ANDROID_SERIAL` env var. | |||
There was a problem hiding this comment.
Not necessarily. x86_64 machines can't run the arm64/arm apps, sometimes not even with a translation layer. In that case, we might end up using physical devices instead (like I have to do right now, although I plan to work on getting the arm64 variant to work on devices with arm64 translation layer - like Waydroid).
There was a problem hiding this comment.
Corrected in ac5bba54 — the app is arm-only (v7/v8, no x86), so an x86_64 emulator can't run it and a physical arm device / arm-translation emulator is often needed. (This now lives in CLAUDE.md after the AGENTS.md merge.)
|
|
||
| ## State Management | ||
|
|
||
| - **UI state is a single immutable `data class`** exposed as a `StateFlow<…UiState>`; the ViewModel mutates a private `MutableStateFlow` via `update { it.copy(...) }`. Derived booleans live as computed properties on the state class (so the UI stays dumb). |
There was a problem hiding this comment.
State should not necessarily be a data class, especially for UI that has multiple, mutually-exclusive states. It can lead to problems like "boolean hell" and state explosion. If a UI has multiple states (loading, installing, processing, completed, failed, cancelled, etc.) - they must almost always be represented with sealed classes. The goal of using sealed classes is to make the state machine explicit and ensure that incompatible states cannot co-exist at the same time.
There was a problem hiding this comment.
Agreed — rewrote the state guidance in ac5bba54 to require sealed types for mutually-exclusive states (loading/content/error/…) and to keep a data class only for genuinely independent fields, calling out the boolean-hell/state-explosion risk explicitly.
| data class PluginManagerUiState( | ||
| val isLoading: Boolean = false, | ||
| val plugins: List<PluginInfo> = emptyList(), | ||
| val isPluginManagerAvailable: Boolean = false, | ||
| val isInstalling: Boolean = false, | ||
| ) { | ||
| val isEmpty: Boolean get() = plugins.isEmpty() && !isLoading // derived in state | ||
| val showEmptyState: Boolean get() = isEmpty && isPluginManagerAvailable | ||
| } |
There was a problem hiding this comment.
This is actually a good example of why it should use sealed classes: isLoading, isInstalling and isEmpty can be true at the same time.
The example should be changed to demonstrate mutually-exclusive state definition using sealed classes.
There was a problem hiding this comment.
Good point — replaced the lead example with a real sealed one (CloneRepoUiState, Idle/Cloning/Success/Error) in ac5bba54. The PluginManagerUiState boolean example is kept but demoted and captioned as the independent-fields-only case, with a warning that coexisting booleans are a smell when the states are actually exclusive.
|
|
||
| ## Code style | ||
|
|
||
| 2-space indents everywhere. Java: Google style (`google-java-format`); Kotlin: `ktfmt` Google-internal style; XML: Android Studio formatter. Branch names must match `.../ADFA-#####` (3–5 digits) — see CONTRIBUTING.md; a pre-commit hook enforces it (`sh ./scripts/install-git-hooks.sh`). |
There was a problem hiding this comment.
Incorrect. Codebase is configured to use Spotless for formatting files. Spotless is configured to use tabs instead of spaces.
There was a problem hiding this comment.
Fixed in ac5bba54 — corrected to tabs via Spotless, and also fixed the formatter names (Java = Eclipse config spotless.eclipse-java.xml, Kotlin/Gradle = ktlint, XML = Eclipse WTP; not ktfmt/google-java-format/Android Studio).
| ## How to use this | ||
|
|
||
| - **Author:** self-review against this list *before* requesting review. Most of it you can check in five minutes. | ||
| - **Reviewer:** you own correctness, leaks, security, and tests. Don't rubber-stamp; don't bikeshed style the formatter already enforces (`ktfmt` / `google-java-format`, 2-space indents). |
There was a problem hiding this comment.
Tabs as indents, not spaces.
There was a problem hiding this comment.
Fixed in ac5bba54 — tabs, and corrected the formatter references (Spotless: ktlint / Eclipse).
| - [ ] **Tests:** non-UI logic has unit coverage. Where coverage is thin, there's logging to diagnose it in the field. | ||
| - [ ] **No duplication:** the change reuses existing helpers instead of copy-pasting. | ||
| - [ ] **Docs:** public classes/functions have KDoc/Javadoc explaining *why*, not *what*; any module `README`/`ARCHITECTURE.md`/ADR the change affects is updated in the same PR. | ||
| - [ ] **Strings** are in `strings.xml`, not inline literals. |
There was a problem hiding this comment.
Worth adding: strings should be added in the :resources module's strings.xml, not per-module. This is to ensure ease of translation.
There was a problem hiding this comment.
Updated in ac5bba54 — the rule now specifies the :resources module's strings.xml (not per-module) to keep localization centralized.
| @@ -0,0 +1,37 @@ | |||
| Code On The Go is an Android IDE — it lets users edit, build, and deploy their own Android apps on-device, like Eclipse or VSCode. | |||
There was a problem hiding this comment.
Let's merge the contents of AGENTS.md directly into CLAUDE.md. Claude Code automatically reads CLAUDE.md on initialization. If we force it to perform a secondary tool call to read AGENTS.md, we waste context window tokens, slow down startup, and risk the agent ignoring crucial operational rules if the file read fails.
There was a problem hiding this comment.
Done in 0d052265 — folded all unique operational content into CLAUDE.md so it's self-contained (no secondary read needed), and left AGENTS.md as a thin pointer so the cross-tool AGENTS.md convention still resolves without duplicating drift-prone content.
| - **Status:** Accepted | ||
| - **Date:** 2026-06-19 | ||
| - **Deciders:** Code On The Go team | ||
|
|
||
| ## Context | ||
|
|
||
| Historically the IDE's own UI is **View-based** — XML layouts, `Fragment`s, and `RecyclerView` with Material Components (see [ADR 0006](0006-koin-dependency-injection.md) context and ARCHITECTURE.md). Newer surfaces already moved to a Unidirectional Data Flow (UDF) architecture — `ViewModel` + `StateFlow`, sealed UI-state/effect types, repositories, Koin DI — but still render through XML and `findViewById`/binding. | ||
|
|
||
| That split has a cost: two ways to build a screen, manual view-state wiring, boilerplate binding code, and UI logic that's awkward to unit-test. Jetpack Compose collapses the view layer into Kotlin, binds naturally to `StateFlow` via `collectAsState()`, and fits the UDF pattern the team already follows. Unlike most ADRs here, this one is **forward-looking** — it sets direction for new work rather than documenting an existing decision. |
There was a problem hiding this comment.
We should have a plugin-api.md file as well, stating best practices like ensuring any changes to the API does not break binary compatibility
There was a problem hiding this comment.
Added docs/plugin-api.md in b737bcec. It's maintainer-facing (how to evolve the API) and reconciles with where we are today — the API isn't frozen and we don't yet guarantee binary compat — but documents the Kotlin binary-compat traps so breaks are deliberate, not accidental: data-class constructor params, enum constants, methods added to plugin-implemented interfaces, and manifest/permission-string renames. Also added a checklist to impact-check changes against the in-tree example plugins and the plugin-examples repo.
|
|
||
| Avoid adding dependencies — we almost certainly already have what you need. Check `build.gradle.kts`. | ||
|
|
||
| Plan before building, and size the change (files + LOC). If it will exceed 500 LOC or 10 files, split it into 2+ change sets so the user can land them as separate PRs. |
There was a problem hiding this comment.
This feels prescriptive and has some negative consequences.
There's overhead from splitting code up. Especially if a future change requires making edits that go across the PRs.
Where we landed at Deepcell was to usually go with one PR per ticket/use case. But try to break the PR up into reviewable commits. Plus separate mechanical changes in different commits from more complex changes that needed more human review. Then ask teammates to review by commit.
Over time, with LLM assistance, people can handle bigger PRs.
There was a problem hiding this comment.
Agreed, softened in a00557db. It now reads: prefer one PR per ticket/use case, and when a change is large, break it into reviewable commits (mechanical/refactor separate from behavioral) with review-by-commit — rather than force-splitting into multiple PRs. ~500 LOC / 10 files is a soft signal to add that commit structure, not a hard cap, and the ceiling rises as LLM-assisted review matures. Thanks — this matches your Deepcell experience.
| - **Reviewer:** you own correctness, leaks, security, and tests. Don't rubber-stamp; don't bikeshed style the formatter already enforces (`ktfmt` / `google-java-format`, 2-space indents). | ||
| - Tie every blocking comment to a concrete risk (a crash, a leak, a CVE class, an untested branch). Tag non-blocking polish as **nit:** so the author can triage. | ||
|
|
||
| ## The 60-second checklist |
There was a problem hiding this comment.
Curious how well this works to catch most of the most important areas a review should catch.
In the REVIEW.md docs there's an explicit note saying that Claude does not look up file references or imports in the REVIEW.md, which might be a bit limiting.
e.g. the ADRs and ARCHITECTURE.md docs are mostly not represented directly in this file and might be missed
As an alternative, having a skill that activates on each code review would allow claude (or subagents) to read other files as guidance.
Could be helpful to ask Claude to run a code review pass on several PRs using what's here. See if it does properly review for architecture or not.
And compare that with a skill that explicitly does an architecture review by reviewing against guidance in ARCHITECTURE.md
There was a problem hiding this comment.
Fair concern. Two changes in 920275da: (1) I inlined the key architecture checks directly into REVIEW.md §10 (UDF, sealed state, Koin, Room, module dependency direction, Compose, system bars) so a review doesn't depend on following links to the ADRs/ARCHITECTURE.md; (2) noted a dedicated architecture-review skill (subagents actually read those docs) as a follow-up. And you're right that the real proof is running it — I've left "validate REVIEW.md against several real PRs and audit misses" as an explicit next step rather than claiming it works.
| @@ -0,0 +1,174 @@ | |||
| # Code Review Guide | |||
There was a problem hiding this comment.
One question I have while reading this -- as a first step, hopefully this helps Claude review the right things.
But then, how does Claude prove it reviewed correctly?
So I think it's worth trying this REVIEW.md as is -- see if it 1) does review all the things you hope it'll review and 2) convinces you it did review thoroughly.
A suggestion for how to address this is here
You can bake it into the REVIEW.md or an associated skill to tell Claude to make sure it did review each item. And then show you proof of what it did.
But that may be overkill if this REVIEW.md already works!
There was a problem hiding this comment.
Added a per-item evidence ledger to "How to use this" in 920275da: a review must show what it checked and the result per area (coverage numbers, LeakCanary/StrictMode output, which requirements were covered) — not a bare LGTM — kept proportional to change size. Modeled on the common-agent-setup pattern you linked.
|
|
||
| The app runs a real StrictMode policy via `StrictModeManager` with a **whitelist engine**. Project rule (see learnings/memory): **the whitelist is only for vendor/framework code we can't change — never for app-owned violations.** If your code trips StrictMode, fix the code. | ||
|
|
||
| - Move disk and network I/O off the main thread (`Dispatchers.IO`, `withContext`). No DB reads, file reads, or `SharedPreferences` first-access on the UI thread. |
There was a problem hiding this comment.
Also maybe any long-running compute (like decoding a large file, etc.) should be off main thread. There was a recent Sentry bug where JSON decoding on main thread caused crashes.
There was a problem hiding this comment.
Added to §3 in 920275da: long-running CPU work (decoding/parsing a large file, JSON (de)serialization, crypto, image/APK processing) goes off the main thread too, not just I/O — Dispatchers.Default — and I cited the JSON-decode-on-main-thread crash as the motivating example.
| @@ -0,0 +1,174 @@ | |||
| # Code Review Guide | |||
|
|
|||
| How to give a good review on Code On The Go. This is a coaching doc, not a gate — use judgment, explain the *why*, and prefer a concrete suggestion (or a diff) over "this is wrong." It complements, and does not replace, [ARCHITECTURE.md](ARCHITECTURE.md) (the source of truth for patterns) and the operational rules in `AGENTS.md` / `CLAUDE.md`. | |||
There was a problem hiding this comment.
Judgement and context are important; however, if there are verifiable things the team almost always cares about, then the earlier Claude catches these "gates" the better.
And if you give Claude a clear way to test the gate, it'll do so more often.
Will add a few comments below to suggest where more verifiable gates might be worth considering.
There was a problem hiding this comment.
Agreed on verifiable gates. Where the team clearly always cares, I made the target checkable rather than aspirational — coverage numbers via JaCoCo, a clean LeakCanary/StrictMode run, offline exercised with the network off — and the evidence ledger asks the reviewer to show those results. Judgment still applies for the rest.
|
|
||
| ## 5. Tests & coverage | ||
|
|
||
| **If the code is not purely UI, expect unit tests in the same PR.** ViewModels, repositories, parsers, builder/tooling logic, and security-sensitive helpers are all testable off-device. |
There was a problem hiding this comment.
This section may benefit from a more verifiable target -- but still leave room for judgement.
e.g. On recent ADFA work, I've asked Claude to aim for 90% line & branch coverage on all new, non-UI code. And asked it to prove it using code coverage results.
Not sure if this matches what the team cares about though -- is there a more specific target that describes what the team cares about?
There was a problem hiding this comment.
Made it verifiable in 920275da: ≥ 50% line & branch on new/changed non-UI code as a deliberate starting bar (rising over time as the suite matures), proven via jacocoAggregateReport — not asserted. UI code is exempt, but logic extracted out of it is not. We picked 50% to start per Hal; happy to raise it once we have a baseline.
|
|
||
| ## 7. Code quality | ||
|
|
||
| - **No duplication.** If you copy-pasted a block, extract a function/extension into the right `common`/`utils` module. Before adding a helper, grep — we likely already have it. Repeated literals/magic numbers → named constants. |
There was a problem hiding this comment.
This is one form of duplication issue. But there are others. I've had some success asking Claude to do a pass to check for loose coupling and good cohesion.
Look for where there might be duplicated logic or behaviour across files. This kind of duplicated logic might come from copy and paste. But it might also come from Claude reimplementing something that's already implemented. Or reimplementing the same thing more than once in different places while building out a feature. Especially when work starts getting split up into chunks by subagent.
There was a problem hiding this comment.
Broadened §7 in 920275da beyond copy-paste: also watch for the same logic reimplemented somewhere we already own it, and behaviour duplicated across a feature built in chunks — especially when work is split across agents/subtasks, where each chunk can reinvent what another already did. Framed it as loose coupling / high cohesion, one owner per concern.
| - **Persistence:** raw SQLite or filesystem — **not Room** (Recent Projects is the lone legacy exception). | ||
| - **UI safety:** never place our UI over the two Android system bars — the top status bar and the bottom navigation bar (`AGENTS.md`). | ||
|
|
||
| ## 11. Offline-first |
There was a problem hiding this comment.
How do you want Claude to verify this and prove it works?
(On tickets I was working on I planned out tests with Claude on which parts of the workflow needed to work offline and then had test cases for it -- had Claude use adb over USB and explicitly turn off the Wifi network for tests)
There was a problem hiding this comment.
Added a concrete method to §11 in 920275da: exercise the flow with the network off — adb shell svc wifi disable && adb shell svc data disable (re-enable after), or airplane mode — confirm the core edit/build/run flow still works, and add an explicit offline test case for the path rather than trusting it by inspection. This mirrors what you did on your tickets.
|
|
||
| ## 10. Architecture alignment | ||
|
|
||
| Hold the change to the patterns in [ARCHITECTURE.md](ARCHITECTURE.md): |
There was a problem hiding this comment.
Mentioned above -- docs suggest Claude might not follow file links in the REVIEW.md?
If this is the case, then could be helpful to bring over a short checklist of the most critical architecture items into this doc.
Does the list below cover the most important things?
There was a problem hiding this comment.
Yes — addressed by inlining the architecture checklist into §10 (920275da) so the critical items are present in REVIEW.md itself and don't rely on Claude following links to the ADRs/ARCHITECTURE.md. A deeper architecture-review skill is noted as a follow-up.
|
|
||
| ## 2. Memory & resource leaks — fix them before LeakCanary does | ||
|
|
||
| LeakCanary runs in debug builds (`debugImplementation`), so leaks *will* surface — catch them in review first. Common offenders here: |
There was a problem hiding this comment.
Below look like good guidelines. Is there a way to check this more explicitly during testing?
e.g. if E2E + unit tests triggered any leaks during automated testing, is there a way to see if LeakCanary flagged anything?
There was a problem hiding this comment.
Added a verification line to §2 in 920275da: after exercising the touched screens in a debug/instrumented run, confirm LeakCanary surfaced no new leak (logcat tag LeakCanary, or the heap-dump notification), and record that clean run as the evidence. LeakCanary runs in debug builds so it's checkable per-run.
|
Feature completeness So could be useful to have Claude look up the Jira ticket to see if the plan covers requirements, whether testing covers the intended flow, and also review to see if all requirements are implemented. One other random thought -- it came up in one of the meetings I was in that the team wished for more updates while work is in progress in the ticket. That's another brief rule you could add to this PR. |
|
Had an hour on the train up from LA to take a look. Looks like a great foundation! Added a few comments. |
|
Follow up question is how do we tell if this PR is good? What testing have we done? Or do we plan to do? e.g. Has this harness been used to implement a ticket? e.g. Should the issues in the code review conventions mostly get caught by Claude before opening a PR going forward? Might be useful to audit PR feedback after this harness is in place to see what it regularly misses. |
|
|
||
| --- | ||
|
|
||
| ## What the three tools catch (and where they differ) |
There was a problem hiding this comment.
Does this PR also depend on Claude's built in /security-review skill?
https://support.claude.com/en/articles/11932705-automated-security-reviews-in-claude-code
Over time, I suspect that may prove to be more powerful (and likely already is) than other tools since it's such a generally useful engineering need.
There was a problem hiding this comment.
Added a SECURITY.md subsection in 920275da on the relationship: /security-review complements the three CI scanners — the scanners own the enforced baseline (Sonar/Semgrep blockers) and dependency CVEs (Snyk), while /security-review is a reasoning-based pass that catches design/logic issues the pattern scanners miss. It's advisory (no baseline/gate), and we recommend running it on security-sensitive changes. Agree it may carry more of the qualitative load over time.
|
|
||
| ## 13. Consider impact on plugins | ||
|
|
||
| Plugins might conflict with each other. New core features might cause plugins to break even if they don't change the plugin API. The review process must consider the impact of the current change on plugins in the wild. |
There was a problem hiding this comment.
This feels a little vague -- when I dropped by the office one Tuesday, I think Hal mentioned that we don't want to freeze the plugin API yet, or even guarantee backward compatibility. Since it's still early and we care more about evolve as we discover what plugins need.
But maybe we may still want to check for breaking API changes and make sure they happen deliberately (documented and justified)?
Also, how does one "consider the impact of a change on plugins in the wild"? Perhaps people familiar with the plugins and CodeOnTheGo can do this. I'm not sure I can. And I'm not sure this is specific enough for Claude to do -- and maybe that's okay.
One mechanical thing I think Claude could help with though is if there's a significant or breaking change, ask it to review at least the plugin-examples repo to see what the impact is on the growing family of example plugins.
There was a problem hiding this comment.
Reworked §13 in b737bcec to match Hal's framing: the plugin API isn't frozen and we don't guarantee compat yet, so the bar is "don't break plugins by accident" — does the change touch the API surface → is any break deliberate and documented → mechanically impact-check against the in-tree example plugins (apk-viewer-plugin, markdown-preview-plugin, keystore-generator-plugin) and, for significant changes, the plugin-examples repo. The "break API compat" specifics live in the new docs/plugin-api.md.
|
|
||
| ## Decision | ||
|
|
||
| New persistence uses **raw SQLite** (`SQLiteOpenHelper` / `SQLiteDatabase`) or the **filesystem / preferences**. Room is **not** used for new code. |
There was a problem hiding this comment.
This should be re-evaluated.
The use of Room should be encouraged/recommended.
There was a problem hiding this comment.
Agreed — the ADR now recommends Room as the default; raw SQLite only for justified exceptions (af92f6c5).
|
|
||
| | # | Decision | Status | | ||
| |---|---|---| | ||
| | [0001](0001-persistence-without-room.md) | Persistence uses SQLite/filesystem, not Room | Accepted | |
There was a problem hiding this comment.
The status of this is not Accepted
There was a problem hiding this comment.
Good catch — set all nine ADRs + the index to Proposed in a00557db. They'll move to Accepted when this PR merges / the team ratifies.
… exceptions Reframes ADR 0001 and cascades to ARCHITECTURE.md, AGENTS.md, REVIEW.md per review feedback from itsaky-adfa and dara-abijo-adfa. Room is the default; raw SQLite is reserved for prebuilt read-only DBs, performance/allocation- critical indexing, and cross-boundary schemas. Recent Projects is the reference example of the default, not an exception. Renames 0001-persistence-without-room.md -> 0001-prefer-room-for-persistence.md.
…ing API Rewrites ADR 0003 per itsaky-adfa's correction (confirmed against the code): composite-build/build-deps* modules ship in the APK and run at IDE runtime (e.g. Java LSP via javac/jdk-compiler/jdt), and live in composite builds for build-time caching. Adds an explicit callout that the Gradle Tooling API is a separate out-of-process JDK from terminal bootstrap packages, driven over JSON-RPC. Fixes two cross-reference lines in ADR 0002 that conflated the two.
Per jatezzz's review: Claude Code auto-reads CLAUDE.md, so a separate AGENTS.md forces a secondary read and risks the operational rules being skipped. Folds all unique AGENTS.md content into CLAUDE.md (emulator/device, Jira CLI, SonarQube MCP, CI-job resolution, official-actions-in-CI, git/gh messaging, keep-docs-current, brevity) and replaces AGENTS.md with a thin pointer so the cross-tool AGENTS.md convention still resolves without duplicated, drift-prone content. Repoints the AGENTS.md citations in REVIEW.md and SECURITY.md to CLAUDE.md.
…arcelize, namespace) Verified each against the code before editing: - Code style: tabs + LF via Spotless (leadingSpacesToTabs), not 2-space; and the right formatters (Java=Eclipse config, Kotlin/Gradle=ktlint, XML=Eclipse WTP), not ktfmt/google-java-format/Android Studio. Fixed CLAUDE.md and REVIEW.md. - State management: require sealed types for mutually-exclusive UI states (no boolean hell); reframed the example to lead with real sealed CloneRepoUiState and caption the PluginManagerUiState boolean example as independent-fields-only. - Added a Parceling row: use @parcelize, never hand-roll Parcelable. - REVIEW.md: strings live in the :resources module's strings.xml. - Emulator: app is arm-only (v7/v8, no x86), so a physical arm device is often needed; an x86_64 emulator can't run it. - ADR 0008: the decisive reason to keep the namespace is the terminal bootstrap packages coupling — a rename must be an atomic big-bang change across both.
- PR sizing (fryanpan): prefer one PR per ticket/use case, break large work into reviewable commits (mechanical vs. behavioral) with review-by-commit; ~500 LOC/10 files is a soft signal, not a hard cap. (CLAUDE.md, REVIEW.md) - ADR status (dara-abijo-adfa): all 9 ADRs + README index Accepted -> Proposed; they ratify to Accepted when this PR merges. - Nits (CodeRabbit): ADR 0005 'very large' -> 'prohibitively large'; REVIEW.md drop the 'exactly' intensifier. (The stray '39' char was already absent.)
- New docs/plugin-api.md (Daniel-ADFA): maintainer-facing plugin API stability & compatibility guide. Defines the contract surface (:plugin-api interfaces/data classes/enums + manifest keys, permission strings, formats), the current policy (API not frozen, backward/binary compat not yet guaranteed but changes must be deliberate/documented/justified), the Kotlin binary-compat traps, a pre-change checklist, and a follow-up to add binary-compat tooling. Grounded in the plugin dev guide and the real :plugin-api module. - REVIEW.md 13 (fryanpan): replaced the vague 'consider impact on plugins' with a concrete check — does it touch the API surface, is any break deliberate and documented, and a mechanical impact check against the in-tree example plugins (apk-viewer / markdown-preview / keystore-generator) and the plugin-examples repo. - Fixed the plugin.json manifest claim -> AndroidManifest.xml <meta-data> in ARCHITECTURE.md and REVIEW.md (meta-data is the primary loader path).
Commits the in-repo author-facing plugin guide (project layout, AndroidManifest meta-data contract, theme-aware icons, building/installing, troubleshooting) and wires reciprocal links between it (how to author) and plugin-api.md (how to evolve the API).
- Per-item evidence ledger: a review must show what it checked and the result, proportional to change size (not bare LGTM). - Feature completeness: start from the Jira ticket; confirm requirements are implemented and the intended flow is tested. Added as lead rule + checklist item. - Coverage target: >=50% line & branch on new non-UI code (rising over time), proven via jacocoAggregateReport; UI exempt. - Architecture (10): inlined the key rules as a checklist (UDF, sealed state, Koin, Room, module dependency direction, Compose, system bars) so reviewers don't have to follow links; noted an architecture-review skill as follow-up. - Threading (3): long-running CPU work off the main thread (JSON decode crash). - Duplication (7): broadened to reimplemented logic / cross-subagent duplication. - Offline (11) and leaks (2): concrete verification steps (adb network off; a clean LeakCanary run) recorded as evidence. - CLAUDE.md: post in-progress ticket updates via the jira CLI. - SECURITY.md: relationship to Claude's /security-review (complements the three CI scanners, doesn't replace the enforced baseline).
…NTRIBUTING.md - CLAUDE.md: new Branch model section — main is release-only (merges from stage), stage is the protected default/integration branch and the base for feature branches, feature branches PR back into stage. Never target main directly. - CONTRIBUTING.md: replaced the stale 'dev branch is protected' line (there is no dev branch; stage is the protected default, main is not) with the correct branch model, and corrected the Source code format section (ktfmt/google-java-format/ 2-space -> Spotless: tabs, ktlint for Kotlin, Eclipse for Java/XML). Edits deliberately avoid the CONTRIBUTING.md regions changed by PR 1478 (community-contribution branch naming) to prevent merge conflicts.
|
Thanks everyone — great round of feedback. Pushed 9 commits addressing all of it, and replied inline on each thread. Summary:
Two things worth a look from the team:
Re-requesting review from @itsaky-adfa, @jatezzz, @dara-abijo-adfa. |
A project skill that forces a read of ARCHITECTURE.md + the ADRs, then checks a diff against the documented patterns (UDF/state, Koin, Room-vs-SQLite, Compose, module boundaries, ABI flavors, dependency substitution, @parcelize, strings), tracing each finding to its ADR/section. Addresses the 'rules in on-demand docs get missed' problem: the skill guarantees the authoritative docs are read at review time rather than relying on prose links. REVIEW.md §10 now points to it. Commits only the skill file under .claude/ (not local settings or hooks).
A non-blocking pre-push hook (.githooks/pre-push/0002-architecture-review-nudge) that reminds the author to run an architecture pass when a push touches first-party Kotlin/Java. It always exits 0 (never gates), and stays silent unless production app source changed — docs/test/vendored-only pushes produce no output. Points at the architecture-review skill and REVIEW.md section 10. Runs via the existing .githooks dispatcher after 0001-run-spotless.
6 participants
ADFA-4357 — Agent & contributor documentation set
Adds a coordinated set of Markdown docs to onboard both human and AI contributors and to record the project's architectural decisions. Docs only — no code or build changes.
What's included
CLAUDE.mdARCHITECTURE.mdfor architecture.AGENTS.mdARCHITECTURE.mdREVIEW.mdstrings.xml.SECURITY.mddocs/adr/ADRs
com.itsaky.androididenamespace after rebrandNotes for reviewers
Content was written against the actual codebase (verified patterns: Koin DI, Firebase
IAnalyticsManager, the StrictMode whitelist engine, Sentry global handler,tooling-apiout-of-process, vendored toolchain). Two claims are author inferences worth a sanity check:ARCHITECTURE.md/SECURITY.md: "Retrofit is in the catalog but effectively unused in app code."Follow-ups (intentionally out of scope)
idetooltips(surfaced by ADR 0001/0003).ARCHITECTURE.mdtodocs/adr/.