Skip to content

Commit 5f14ef7

Browse files
ADFA-4357 Add agent & contributor documentation set (#1422)
* ADFA-4357 Add agent & contributor documentation set 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. * ADFA-4357 Document accessibility & contextual-help review rules 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. * ADFA-4357 Record Compose-for-new-UI decision; offline-first rule - 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. * ADFA-4357 Note ADFA-4381 follow-up for the Compose long-press help bridge 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. * ADFA-4357 Flag idetooltips/README.md as stale; track refresh in ADFA-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. * ADFA-4357 Revert all idetooltips/README.md changes on this branch 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. * ADFA-4357 Add doc-sync rule: update module docs in the same change - 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. * ADFA-4357 Add brevity directive for docs/tickets/messages to AGENTS.md * ADFA-4357 Concision pass on the doc set; fix stale tooltip API ref 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'. * ADFA-4357 Reframe experimental-flag item; move perf budget to ADFA-4383 - 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. * ADFA-4357 Promote experimental-flag rule to accepted §12 in REVIEW.md 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. * ADFA-4357 Drop backward-compat proposal and the now-empty Open-for-discussion 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. * Update REVIEW.md to cover the impact of changes upon plugins * Typo - Update REVIEW.md * Update CLAUDE.md with guidance regarding off-device links * ADFA-4357: Flip persistence default to Room; raw SQLite for justified 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. * ADFA-4357: Correct ADR 0003 — separate in-IDE toolchain from the Tooling 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. * ADFA-4357: Merge AGENTS.md into CLAUDE.md (self-contained) 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. * ADFA-4357: Fix factual errors flagged by itsaky (formatting, state, Parcelize, 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. * ADFA-4357: Soften PR-splitting rule, set ADRs to Proposed, fix nits - 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.) * ADFA-4357: Add plugin-api.md and rework the plugin-impact review rule - 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). * ADFA-4357: Add PLUGIN_AUTHORING.md and cross-link with plugin-api.md 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). * ADFA-4357: Make REVIEW.md verifiable and self-contained (fryanpan) - 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). * ADFA-4357: Document the main/stage/feature branch model; fix stale CONTRIBUTING.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. * ADFA-4357: Add architecture-review skill; wire it into REVIEW.md §10 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). * ADFA-4357: Add pre-push architecture-review nudge to .githooks 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. * docs: add rule for code comments Signed-off-by: Akash Yadav <akashyadav@appdevforall.org> * docs: prefer collectAsStateWithLifecycle() over collectAsState() Signed-off-by: Akash Yadav <akashyadav@appdevforall.org> * docs: explicitly state why targetSdk is pinned at API 28 Signed-off-by: Akash Yadav <akashyadav@appdevforall.org> * docs: add clarification on per-abi splits Signed-off-by: Akash Yadav <akashyadav@appdevforall.org> * docs: add clarification on why Hilt was rejected Signed-off-by: Akash Yadav <akashyadav@appdevforall.org> --------- Signed-off-by: Akash Yadav <akashyadav@appdevforall.org> Co-authored-by: Akash Yadav <akashyadav@appdevforall.org>
1 parent 9b1bf97 commit 5f14ef7

20 files changed

Lines changed: 1496 additions & 11 deletions
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
---
2+
name: architecture-review
3+
description: Review a code change against Code On The Go's architecture — ARCHITECTURE.md and the ADRs in docs/adr/. Forces a read of those authoritative docs (they are NOT reliably in context otherwise), then checks the diff against UDF/state, Koin DI, Room-vs-SQLite, Compose, module boundaries, ABI flavors, dependency substitution, @Parcelize, and strings placement, reporting violations with the ADR/section each comes from. Use when asked to review architecture alignment, check a change against ARCHITECTURE.md/the ADRs, or as the §10 step of a code review.
4+
metadata:
5+
author: Hal Eisen
6+
keywords:
7+
- architecture
8+
- review
9+
- adr
10+
- udf
11+
- code-review
12+
- codeonthego
13+
---
14+
15+
## Why this is a skill (not just REVIEW.md §10)
16+
17+
ARCHITECTURE.md (~4.9k tokens) and the ADRs (~7.3k) are **not** in context during normal work, and prose links in REVIEW.md are not reliably followed. This skill exists to **guarantee the read**: it opens the authoritative docs, then checks the diff against them. Do not review architecture from memory or from the summary in this file alone.
18+
19+
## When to invoke
20+
21+
- "Review this against the architecture / ARCHITECTURE.md / the ADRs."
22+
- "Does this change follow our patterns?" / architecture-alignment review.
23+
- As the §10 step of a full code review — its output feeds REVIEW.md's evidence ledger.
24+
25+
## Step 1 — Scope the diff
26+
27+
Pick the target and get the changed files + hunks:
28+
- Working tree: `git diff --stat` and `git diff`.
29+
- This branch vs the integration branch: `git diff origin/stage...HEAD` (feature branches are based on `stage`).
30+
- A GitHub PR: `gh pr diff <N>`.
31+
32+
**Exclude vendored/generated code** — it's not held to our patterns: `composite-builds/build-deps*`, `subprojects/{aaptcompiler,builder-model-impl,flashbar,xml-dom}`, `termux/`, `eventbus/`, `LayoutEditor/`, `**/build/`, generated `R`/`BuildConfig`. Review only first-party Kotlin/Java/XML/Gradle changes.
33+
34+
## Step 2 — READ the authoritative docs (mandatory)
35+
36+
Before judging anything, read:
37+
- `ARCHITECTURE.md` (whole file — module map, dependency rules, tech stack, **State Management**, testing).
38+
- **Every** file in `docs/adr/*.md`. At minimum open the ones a diff can plausibly violate: 0001 (Room), 0003 (substitution), 0005 (flavors), 0006 (Koin), 0009 (Compose). Read the rest if the change is broad.
39+
40+
Do not skip this because the rules "look familiar" — they are the source of truth and they change.
41+
42+
## Step 3 — Check the diff against the rules
43+
44+
For each changed first-party file, check the applicable rules. Each rule cites its source so findings are traceable.
45+
46+
| # | Rule | Source |
47+
|---|---|---|
48+
| 1 | **UDF:** new screens use `ViewModel` + `StateFlow<UiState>`, sealed `UiEvent`/`UiEffect`, a repository for data; composables collect via `collectAsStateWithLifecycle()` (lifecycle-aware, Android's strongly-recommended default; `collectAsState()` is only for platform-agnostic/KMP code, which we don't have); no I/O or business logic in composables/Activities/Fragments. | ARCHITECTURE.md → State Management |
49+
| 2 | **Sealed state for mutually-exclusive states** (loading/content/error/…): not a `data class` of independent `Boolean`s that can contradict each other ("boolean hell"). | ARCHITECTURE.md → State Management |
50+
| 3 | **Koin DI**, constructor injection; register new singletons/ViewModels in the module. No hand-rolled singletons/service locators (the documented `ServiceLocator` aside). | ADR 0006 |
51+
| 4 | **Persistence:** Room is the default for relational data; raw SQLite only for a justified exception (prebuilt read-only DB, perf/allocation-critical indexing, cross-boundary schema) — and the PR must say which. Non-relational → filesystem/preferences (DataStore). | ADR 0001 |
52+
| 5 | **`@Parcelize`** (`kotlin-parcelize`) for `Parcelable`; never hand-implement it unless Parcelize genuinely can't. | ARCHITECTURE.md → Parceling |
53+
| 6 | **New UI is Jetpack Compose** — a new XML-layout / `Fragment`-rendered screen for the IDE's own UI is a violation (existing XML screens are fine until reworked). | ADR 0009 |
54+
| 7 | **Module boundaries / dependency direction:** UI → ViewModel → Repository → data source; features depend on `common`/`utils`, not the reverse; no new cross-feature or upward dependency. | ARCHITECTURE.md → module map |
55+
| 8 | **ABI flavors:** new Android modules get `v7`/`v8` via `composite-builds/build-logic` centrally — no per-module flavor blocks, no flavorless `assembleDebug`. (`:plugin-api` is intentionally flavorless.) | ADR 0005 |
56+
| 9 | **Dependency substitution:** don't add a Maven coordinate for something already vendored/substituted (`build-deps*`); don't add a new dependency without checking `gradle/libs.versions.toml` first. | ADR 0003 |
57+
| 10 | **Strings** live in the `:resources` module's `strings.xml` (not per-module, not inline literals). | REVIEW.md §7 |
58+
| 11 | **UI never drawn over the two system bars** (top status bar, bottom navigation bar). | CLAUDE.md |
59+
60+
Rules 1, 2, 6 apply to UI changes; 4, 5 to data/model changes; 8, 9 to Gradle changes. Judge by what the diff touches — don't flag rules a file doesn't engage.
61+
62+
For a **large diff (~15+ first-party files)**, fan out: spawn a subagent per dimension (UI/state, DI, persistence, Gradle/modules), each instructed to read the relevant ADR and report only its dimension's findings; then merge. For a small diff, do it inline.
63+
64+
## Step 4 — Report
65+
66+
Output a findings table, most-severe first. Every finding must cite the rule source and give a concrete fix. If a rule was checked and passes, say so (the evidence ledger wants pass/fail, not silence).
67+
68+
```
69+
### Architecture review — <scope>
70+
Docs read: ARCHITECTURE.md, docs/adr/0001,0003,0005,0006,0009 (+others as needed)
71+
72+
| Verdict | File:line | Rule | Source | Fix |
73+
|---|---|---|---|---|
74+
| ❌ | ui/FooScreen.kt:42 | Mutually-exclusive state as booleans | ADR 0001 sibling / State Mgmt | Model as a sealed FooUiState (Loading/Content/Error) |
75+
| ⚠️ | data/BarStore.kt:10 | Raw SQLite without stated justification | ADR 0001 | Use Room, or state which exception applies in the PR |
76+
| ✅ | — | Koin DI | ADR 0006 | new VM registered in module, constructor-injected |
77+
78+
Summary: <n> violations, <n> warnings, <n> checks passed.
79+
```
80+
81+
- **❌ violation** = contradicts a rule; blocking. **⚠️ warning** = likely issue / missing justification. **✅** = checked and clean.
82+
- If nothing architectural changed (e.g. a docs- or test-only diff), say so explicitly rather than inventing findings.
83+
84+
## Notes
85+
86+
- This is a *conformance* check against our documented patterns — not a general bug hunt (use `/code-review` for correctness). Keep findings tied to a specific ADR/section.
87+
- If a rule seems wrong or outdated for the change at hand, flag it as a possible ADR update rather than forcing the code to fit — the ADRs are `Proposed`, not immutable.
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
#!/bin/bash
2+
3+
# Non-blocking nudge: when a push includes changes to first-party app source,
4+
# remind the author to run an architecture pass (the architecture-review skill /
5+
# REVIEW.md §10) before opening the PR.
6+
#
7+
# This is a REMINDER, not a gate — it always exits 0 and never blocks a push.
8+
# It stays silent unless the push actually touches first-party Kotlin/Java, so
9+
# docs-, test-, and vendored-only pushes produce no output.
10+
11+
set -u
12+
13+
cyan=$(tput setaf 6 2>/dev/null || true)
14+
yellow=$(tput setaf 3 2>/dev/null || true)
15+
reset=$(tput sgr0 2>/dev/null || true)
16+
17+
# Determine the commits being pushed. Prefer the tracked upstream; fall back to
18+
# the integration branch (feature branches are based on stage). If neither is
19+
# resolvable, stay quiet rather than nag.
20+
if git rev-parse --abbrev-ref --symbolic-full-name '@{upstream}' >/dev/null 2>&1; then
21+
range="@{upstream}..HEAD"
22+
elif git rev-parse --verify -q origin/stage >/dev/null 2>&1; then
23+
range="origin/stage..HEAD"
24+
else
25+
exit 0
26+
fi
27+
28+
changed=$(git diff --name-only "$range" 2>/dev/null) || exit 0
29+
[ -n "$changed" ] || exit 0
30+
31+
# Keep only first-party production Kotlin/Java: drop tests, generated build
32+
# output, and vendored subtrees (mirrors Spotless's commonTargetExcludes).
33+
firstparty=$(printf '%s\n' "$changed" \
34+
| grep -E '\.(kt|java)$' \
35+
| grep -vE '(^|/)(build|src/test|src/androidTest)/' \
36+
| grep -vE '^(composite-builds/build-deps|termux/|eventbus/|LayoutEditor/|subprojects/(aaptcompiler|builder-model-impl|flashbar|xml-dom|llama\.cpp)/)' \
37+
|| true)
38+
39+
[ -n "$firstparty" ] || exit 0
40+
41+
count=$(printf '%s\n' "$firstparty" | grep -c .)
42+
43+
echo ""
44+
echo "${cyan}[architecture nudge]${reset} this push changes ${count} first-party source file(s)."
45+
echo "${yellow} Consider an architecture pass before opening the PR:${reset}"
46+
echo " - run the ${cyan}architecture-review${reset} skill (reads ARCHITECTURE.md + the ADRs, checks the diff), or"
47+
echo " - self-check against ${cyan}REVIEW.md section 10${reset} (UDF, sealed state, Koin, Room, module boundaries, Compose)."
48+
echo "${yellow} (Reminder only — your push continues.)${reset}"
49+
echo ""
50+
51+
exit 0

‎AGENTS.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
# AGENTS.md
2+
3+
The operational rules for AI agents and contributors live in **[CLAUDE.md](CLAUDE.md)** — build/test commands, ABI flavors, emulator/device selection, project constraints, and the CI/Jira/SonarQube/git-messaging conventions.
4+
5+
This file is a pointer so agents that follow the `AGENTS.md` convention find the guidance; the content is maintained in one place (CLAUDE.md) to avoid drift.

0 commit comments

Comments
 (0)