ADFA-4357 Add agent & contributor documentation set#1422
ADFA-4357 Add agent & contributor documentation set#1422hal-eisen-adfa wants to merge 14 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 Walkthrough
WalkthroughAdds 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.
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.
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).
|
|
||
| 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.
| @@ -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).
|
|
||
| ## 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.
| 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.
|
|
||
| ## 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.
| ## 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.
| - [ ] **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.
| @@ -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.
| - **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
|
|
||
| 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.
| - **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
| @@ -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!
|
|
||
| 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.
| @@ -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.
|
|
||
| ## 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?
|
|
||
| ## 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.
| - **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)
|
|
||
| ## 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?
|
|
||
| ## 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?
|
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.
|
|
||
| ## 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.
5 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/.