Skip to content
18 changes: 18 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ flox activate -d flox/local -- ./gradlew <task>
- **Single unit test:** `flox activate -d flox/local -- ./gradlew :module:test --tests "com.itsaky.androidide.SomeTest"`
- **Module unit tests:** `flox activate -d flox/local -- ./gradlew :testing:unit:test`
- **Fast iteration:** during multi-file/multi-module changes, verify with targeted `:module:compileV8DebugKotlin`/`compileV8DebugJavaWithJavac` invocations (batch several modules into one Gradle call) rather than a full assemble. Reserve `:app:assembleV8Debug` for final end-to-end verification — it's slow (multi-minute) in this multi-module project, and running it after every small change adds up.
- **Long-running commands: narrate them, and background only the read-only ones.** Anything that can exceed ~60s — `assembleV8Debug`, a full test sweep, a cold Gradle invocation of any kind (daemon start plus configuring this many modules is ~20s before any task runs, and far worse under memory pressure), and `git push`, which runs Spotless through the hook — gets a line before it starts saying what is running and roughly how long it takes, and a status line every couple of minutes while it runs: elapsed time, the last output line, whether it is still progressing. A silent terminal is indistinguishable from a hang, and saying which it is is your job, not the user's to ask.

Background a command only if it does **not** write to the worktree. A build, a test run, or `spotlessCheck` is safe to background. `spotlessApply` and `git push` (which runs it through the hook) rewrite tracked files, so editing or staging while one is in flight races it — keep those in the foreground and narrate the wait instead.

When the user names a CI/CD job ("the sonar job", "the analyze workflow"), read `.github/workflows/*.yml` — the YAML is the authoritative gradle/shell invocation. Don't reverse-engineer it from gradle tasks or build files.

Expand Down Expand Up @@ -61,12 +64,23 @@ See **[ARCHITECTURE.md](ARCHITECTURE.md)** — the single source of truth for th
- `.androidide_root` is a sentinel file tests use to locate the project root — don't delete it.
- Avoid http or https links which go off-device. When such links are unavoidable, warn the user beforehand and offer to cancel the action.

## Verify before you claim

Four independent reviews of work already reported as verified each found a real defect in it. Before calling a change done, verified, or behaviour-neutral:

- **Sweep the siblings, not just the site in front of you.** After a fix, grep for the other places the same pattern lives — the `UPDATE` beside the `INSERT` you fixed, the third entry point beside the two you guarded, the `Content-Type` *parameters* beside the media type you sanitised. Say in the PR which sites you checked, and which you deliberately left alone.
- **Prove the regression test fails without the fix.** Revert the fix, run the new test, confirm it fails *for the reason it is named for*, then restore. A test that passes against the unfixed code pins nothing. Watch for expectations that all coincide with one boundary value: if every case equals the minimum, a `MIN()` stub passes the whole suite.
- **Match the handler to the failure the change exists to fix.** `catch (Exception)` does not catch an `Error`. A guard on two of three call sites is not a guard.
- **Every claim needs its check.** "No behaviour change", "this MIME type has rows in the shipped database", "that tool logs a warning" are all testable — run the query, read the sibling repo's source, diff the commits — or don't write them. Re-read the PR body before pushing: a description that was true at commit 1 is often false by commit 3.

## Code style

**Tabs** for indentation, **LF** line endings — enforced by **Spotless**. The `ratchetFrom = origin/stage` ratchet is **file-level, not line-level**: it checks every file that differs from `origin/stage` and reformats each such file *in full*, so editing even one line of a file whose existing indentation doesn't conform (e.g. a layout XML using 4 spaces) pulls the **whole file** under the ratchet and requires reindenting it to tabs — a one-line edit can become a whole-file reformat. Java uses the **Eclipse** formatter (`spotless.eclipse-java.xml`, with member sorting + import ordering); Kotlin and `*.gradle.kts` use **ktlint**; XML uses the **Eclipse WTP** formatter. Run `./gradlew spotlessApply` to fix formatting before pushing — the `.githooks` pre-push hook does this automatically once hooks are installed and enabled (`sh ./scripts/install-git-hooks.sh`, no conflicting `core.hooksPath`). Branch names must match `.../ADFA-#####` (3–5 digits) — see CONTRIBUTING.md; a pre-commit hook enforces it (`sh ./scripts/install-git-hooks.sh`).

Keep docs, tickets, commit messages, and PR descriptions crisp — say it once, lead with the point, cut hedging and restated context. Brevity is the soul of wit; a reader's attention is the scarce resource.

Brevity governs the artifact, not the reasoning. When you recommend something to the user — a design choice, a library, a way to split a PR — give the one-line *why* and the alternative you rejected, not the conclusion alone. A bare recommendation costs a round-trip of "tell me more", and the user should never have to ask twice to see the trade-off.

**Code comments** follow the same discipline:
- Short and to-the-point: comment the non-obvious *why* (a workaround, a constraint, a subtle invariant), not what the code already states. Cut restated context.
- **No separator or decorative comments.** No banner bars, `// ====` rules, or ASCII-art dividers; let structure, naming, and small functions carry organization.
Expand Down Expand Up @@ -104,6 +118,10 @@ The names are case-sensitive as written — note the lowercase `review` and `mer

The sonarqube MCP server runs in Docker, so Docker must be up before launching Claude Code. Its first launch pulls a ~225MB image (`mcp/sonarqube:latest`) that exceeds Claude Code's 30s MCP handshake timeout — so the first connect reports a timeout though nothing is broken. Pre-pull the image (or let one launch finish) so later `/mcp` reconnects succeed. `docker system prune` removes it and brings back the slow first launch.

### Staging commits — no `git add -A`

Stage by explicit path (`git add path/one path/two`). `git add -A` sweeps in whatever else the working tree happens to hold, untracked files included — regenerated test fixtures, multi-MB binaries, files carrying machine-local absolute paths — and buries them in an unrelated commit. `git add -u` is narrower (tracked paths only, and it stages deletions), but it still picks up any tracked file something rewrote behind your back, which is how a 12 MB regenerated fixture reached a commit here. The untracked half of that risk is live here: a `:gradle-plugin:test` run leaves files under `tests/`, and `tests/test-home` is not currently ignored. Run `git status --short` first and account for every line; if a regenerated file genuinely belongs in the change, say so in the commit message.

Comment thread
coderabbitai[bot] marked this conversation as resolved.
### Multi-line git/gh messages

Default to writing the body to a tempfile via the Write tool, then `git commit -F /tmp/msg.txt` or `gh pr create --body-file /tmp/body.md`. Use heredoc/`--body "$(cat <<EOF ...)"` only for short messages with no shell-special characters.
Expand Down
11 changes: 11 additions & 0 deletions docs/process/learnings.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
# Learnings

## Spotless / formatting
- **A tripped pre-push hook costs two Gradle invocations, not one.** The hook runs `spotlessApply`, and if that *changes* anything it fails the push and leaves the tree dirty — so you commit the formatting and pay it again. Run `spotlessApply` yourself before committing.
- **Spotless is not the slow part; a cold Gradle invocation is.** Measured on this repo: `spotlessCheck` is ~6.5s warm and ~22s cold, of which ~20s is daemon start plus configuration (`gradlew help` alone costs the same). `:spotlessShell` is 750ms — it targets `.githooks/**` and `scripts/**`, which together are 38 files. A multi-minute Spotless run means something else is wrong: a cold daemon under memory pressure, or a machine with orphaned JVMs holding gigabytes (see ADFA-5265, filed on the wrong premise and corrected by measurement).
- Root `spotlessCheck` passing alone proves little: it only fails on the implicit-dependency check when a task that *writes into the source tree* is in the same invocation (e.g. `:common:compileV8DebugKotlin spotlessCheck`). CI ran it standalone, so that class of failure reproduced only on developer machines.
- The Spotless ratchet (`ratchetFrom = origin/stage`) is **file-level, not line-level**: a one-line edit to a file whose indentation doesn't already conform reformats the *whole* file (e.g. a 4-space layout XML gets fully reindented to tabs). Run `spotlessApply` after editing so the whole-file reformat lands in your commit, not in CI.
- The `.githooks/pre-push/0001-run-spotless` hook runs `spotlessCheck`/`spotlessApply` locally and blocks a push on violations — but only if git hooks are actually enabled: no conflicting global `core.hooksPath` (it silently overrides `.git/hooks`), and a portable dispatcher (`find -executable` is GNU-only and no-ops on macOS BSD find — see ADFA-4833).

## Git / GitHub
- **An `APPROVED` badge does not mean the current code was approved.** This repo does not dismiss stale reviews on push, so an approval can predate the commits it appears to cover — five PRs at once, in one case, one of them approved two weeks before the code it labelled. Compare the commit each approval points at with the PR head: `gh pr view <n> --json reviews,headRefOid` and check the approving review's `commit.oid` against `headRefOid`. Timestamps are a weaker proxy, and `latestReviews` can come back with an empty `commit.oid`, so use `reviews`.
- Before pushing a follow-up commit to a community PR, check `gh pr view <n> --json headRepositoryOwner` — the PR head is usually on the contributor's **fork**, so a same-named push to `origin` doesn't touch the PR and just creates a confusing dead branch that has to be deleted.

## Android / Kotlin
Expand All @@ -24,6 +28,13 @@
- `.bail on` is required for a `BEGIN;...COMMIT;`-wrapped script to actually be atomic: without it, a mid-script SQL error prints to stderr but the script keeps going, including reaching the final `COMMIT`, which persists whatever succeeded before the error. `.bail` also can't see `.system` shell failures directly — if a step's success depends on a shell command's exit status, assert it in SQL (e.g. a temp table with a `CHECK` constraint) rather than relying on `.bail` to catch it.
- Don't write a `.system` command's output to a fixed, guessable filename directly under `/tmp` (CWE-377) — another local user could pre-plant a symlink there or race the write against your later read. Create an owner-only working directory instead (`rm -rf` it, then `mkdir -m 700` it — the mode is set atomically at creation, so there's no window where it's briefly wider), write everything under that, and remove it when done. `mkdir` itself can fail (e.g. another user recreates the path between the `rm -rf` and the `mkdir`) — that's a `.system` failure `.bail` won't catch either, so assert the directory's mode in SQL before trusting it, the same way you'd guard the Brotli step above. A fresh `mktemp -d` per run would be even better, but it doesn't fit this script shape: each `.system` line is its own subshell, so a path it generates can't be carried into later `.system`/`READFILE()` calls without writing it to another fixed, guessable file first.

## Instrumented (androidTest) runs
- `./gradlew :module:connectedV8DebugAndroidTest` fails here before any test runs: `NoClassDefFoundError: org/bouncycastle/asn1/edec/EdECObjectIdentifiers`, thrown while AGP's Unified Test Platform mints a TLS cert for its result-listener server. Not the device, the APK, or the tests (ADFA-5258). Workaround that does work: `adb install -r <module>/build/outputs/apk/androidTest/.../*-androidTest.apk`, then `adb shell am instrument -w -e class <TestClass> <testPackage>/<runner>` — find the runner with `aapt2 dump xmltree --file AndroidManifest.xml <apk>`.
- Assertions in an instrumented test tell you the return value, not the log. To check a diagnostic actually fires (and stays quiet when it should), clear logcat before the run and grep it after — a warning that cries wolf is worse than none.

## Test fixtures that write themselves
- `testing/resources/test-project/.cg/gradle-sync/{project,sync}.pb` used to be **tracked**, and a test run rewrote them with the local machine's absolute paths — so they arrived in unrelated commits, a 12 MB binary among them. Reverting before a run didn't stick, because the next run re-dirtied them. ADFA-5264 (#1740) ignored the whole `.cg/` directory, so this is fixed; it is recorded because the shape recurs. A tracked file that a test run rewrites cannot be kept clean by discipline, only by untracking it.

## Kotlin LSP test harness
- Disposing the `KtLspTestEnvironment` in a unit test (`env.close()`, or `Disposer.dispose(env.project)`) throws `AssertionError: Write access is allowed inside write-action only`. IntelliJ requires model teardown to run inside a write action. This is why `KtLspTestRule`'s teardown has `env.close()` commented out as "fails in test cases". To dispose deterministically in a test, wrap it: `ApplicationManager.getApplication().runWriteAction { env.close() }`.
- The index/compilation environment lifecycle is racy: background `IndexWorker` coroutines call `PsiManager.findFile(project)` and will crash with `Project is already disposed` if the project is disposed before the workers are stopped. Always stop & join `KtSymbolIndex.close()` (and cancel related scopes) before `Disposer.dispose(...)`.
Loading
Loading