From 995ee476ff69826efd24a60745b585e1c3d59b6b Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 24 Aug 2026 17:18:04 -0700 Subject: [PATCH 1/4] docs: retro for the documentation transports and Brotli dictionary work Four independent reviews of PRs already reported as verified each found a real defect in them. The retrospective records what the two recurring shapes were -- fixing the instance in front of me rather than the class, and claims outrunning their checks -- and turns them into rules rather than resolutions. CLAUDE.md gains a "Verify before you claim" section (sweep the sibling sites, prove the regression test fails without the fix, match the handler to the failure, check every claim), a rule against `git add -A` in a repo whose test runs rewrite tracked fixtures, a rule that long commands are backgrounded and narrated rather than run silently, and a rule that a recommendation carries its own why. The last two come from the user's feedback: a silent multi-minute Spotless run was indistinguishable from a hang, and recommendations given as bare conclusions cost a round-trip to unpack. ADFA-5265 covers the Spotless cost itself. learnings.md gains four entries: the pre-push hook's double Spotless cost, the APPROVED badge not meaning the current code was approved, connectedAndroidTest's bouncycastle failure with the adb/am instrument workaround, and the tracked gradle-sync fixtures that a test run rewrites. Not done, deliberately: the reviewer-side revert check in REVIEW.md and dismissing stale approvals on stage. Both change artifacts other people rely on. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 16 ++++++++++++ docs/process/learnings.md | 10 ++++++++ docs/process/retrospective.md | 48 +++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 34a25504f4..36ee92c52d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,6 +18,7 @@ flox activate -d flox/local -- ./gradlew - **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: background them and narrate.** Anything that can exceed ~60s — `spotlessApply`/`spotlessCheck` (~4.5 min here), `assembleV8Debug`, a full test sweep, and `git push` (the pre-push hook runs Spotless, so a push is itself a multi-minute silent command) — runs in the background, not the foreground. While it runs, say what you see every couple of minutes: 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. 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. @@ -61,12 +62,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. @@ -96,6 +108,10 @@ Read tickets with the local authenticated `jira` CLI (e.g. `jira issue view ADFA 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` / `git add -u` sweeps in whatever else the working tree happens to hold — regenerated test fixtures, multi-MB binaries, files carrying machine-local absolute paths — and buries them in an unrelated commit. This repo has tracked fixtures that a test run rewrites, so the risk is live rather than theoretical. 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. + ### 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 < --json latestReviews,commits`) before treating anything as merge-ready. - Before pushing a follow-up commit to a community PR, check `gh pr view --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 @@ -24,6 +27,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 /build/outputs/apk/androidTest/.../*-androidTest.apk`, then `adb shell am instrument -w -e class /` — find the runner with `aapt2 dump xmltree --file AndroidManifest.xml `. +- 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` are **tracked** and a test run rewrites them with the local machine's absolute paths (ADFA-5264). Reverting them before running the tests does not stick — the next run re-dirties them — so revert *after* your last test run and check `git status` before committing. + ## 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(...)`. diff --git a/docs/process/retrospective.md b/docs/process/retrospective.md index fb4eeadd34..c8e64d7280 100644 --- a/docs/process/retrospective.md +++ b/docs/process/retrospective.md @@ -1,5 +1,53 @@ # Retrospective Log +## 2026-08-24 - Documentation transports, the Brotli dictionary migration, and 24 review threads + +### Time Breakdown +| Started | Phase | 👤 Hands-On Time | 🤖 Agent Time | Problems | +|---------|-------|-----------------|---------------|----------| +| Aug 18 04:42 | Migration script, dictionary re-mint, ODT work | ██████████████████████ 221m | ███████████████████████████████████████████ 422m | ⚠ ProcessPool/forkserver, adb-push mtime trap | +| Aug 21 00:09 | Version gate, device verification, charset + tickets | ██████████ 96m | █████████████████ 170m | ⚠ install signature/downgrade failures | +| Aug 22 00:05 | Reviews on #1725/#1726, ADFA-5220 in both repos, triage | ██████████████████████ 223m | ████████████████████████████████████████ 394m | ⚠ 3 regressions found by reviewers | +| Aug 24 18:21 | Path traversal, containment consolidation, lift to #1736 | ██ 17m | ██ 16m | | +| Aug 24 20:49 | Device: asset-extraction benchmark, ADFA-5258 | ██ 15m | ███ 31m | ⚠ connectedAndroidTest broken | +| Aug 24 21:24 | "do them all" — 21 threads across 7 PRs | ██ 15m | | | +| Aug 24 22:36 | Four code reviews of my own PRs, and their fixes | ███████████████ 154m | █████████ 89m | ⚠ every review found real defects | + +### Metrics +| Metric | Duration | +|--------|----------| +| Total wall-clock | Aug 18 -> Aug 25 (~163h calendar span) | +| Hands-on | 11.4h (742m raw, 684m after merging overlapping turns) | +| Automated agent time | ~18.7h active | +| Idle/testing/away | ~83h | +| Retro analysis time | 4 min | + +Caveat on the last phase: 154m of "hands-on" counts ~6,900 words of machine-generated review findings as reading at 150 wpm. The human did not read those end to end. + +### Key Observations +- Four independent reviews of PRs already reported as verified each found a real defect: a security fix that sanitised the media type but not its parameters; a test suite every expectation of which sat on one boundary, so a `MIN()` stub would have passed it; a shutdown guard on two of three entry points; a `catch (Exception)` that misses the `Error` the PR existed to handle; and 12 MB of machine-local fixtures committed by `git add -A`. +- Two shapes recur. **Partial application**: fixing the instance in front of me and missing its siblings (INSERT but not UPDATE, two entry points of three, the type but not its parameters). **Claims outrunning verification**: a PR body still saying "no behaviour change" two behavioural commits later, a comment asserting a MIME type had rows it does not have, a doc asserting a sibling repo logs a warning it never had. +- What worked, and is worth keeping: settling arguments by measurement rather than debate (the ancestor cache died on 48.0s vs 51.4s; the two documentation transports were settled by 0 differing pixels), and the revert-check habit — reverting a fix to confirm the new test fails. Where the revert-check was skipped, a test silently stopped pinning the behaviour it was named for. +- Friction outside the work itself: Spotless at ~4m33s on every push (double when the hook trips), Gradle daemons dying when builds ran concurrently, and `connectedAndroidTest` broken outright. + +### Feedback +**What worked:** "I asked questions about recommendations I didn't understand." Those questions repeatedly caught things — one surfaced that a statistic was being quoted from a different database than the reviewer had measured; another turned a vague ticket into the sequencing hazard that got fixed in both repos. The flip side is that they had to be asked at all: recommendations were given as conclusions with the reasoning left to be requested. + +**What didn't:** "The Spotless problem made it look like nothing was happening. That was frustrating." Long Gradle invocations ran in the foreground with no output, so working was indistinguishable from hung. + +**Do differently:** "Give me more feedback on long-running tasks so I know if the task is stuck." + +### Actions Taken +| Issue | Action Type | Change | +|-------|-------------|--------| +| Long commands run silently | CLAUDE.md | "Build & test": background anything over ~60s (including `git push`, which runs Spotless via the hook) and report elapsed time, last output line, and whether it is still progressing | +| Work declared verified was not | CLAUDE.md | New section "Verify before you claim": sweep sibling sites, prove the regression test fails without the fix, match the handler to the failure, check every claim | +| `git add -A` swept unrelated files | CLAUDE.md | "Operational rules" -> "Staging commits — no `git add -A`": stage by path, read `git status --short` first | +| Recommendations lacked the why | CLAUDE.md | "Code style": a recommendation carries its one-line why and the rejected alternative | +| Spotless costs 4.5 min per push | Ticket | ADFA-5265 — `:spotlessShell` walks `scripts/**`; prune rather than exclude, same shape as ADFA-4816 | +| Instrumented tests, fixtures, stale approvals, Spotless double cost | Doc | Four new entries in `docs/process/learnings.md` | +| Reviewer-side revert check; dismiss stale approvals on `stage` | Deferred | Both change artifacts other people rely on (REVIEW.md, branch protection) — raised, not applied | + ## 2026-08-13 - ADFA-5088: individual Preferences/Plugin Manager tooltips + docdb SQL scripts ### Time Breakdown From e88af8f50ddbea07a42dc2a4afd2ce2d36a1e35b Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 24 Aug 2026 17:37:05 -0700 Subject: [PATCH 2/4] docs: fix the rule the review found half-right The backgrounding rule I wrote from the retro collides with the thing it was about. spotlessApply writes to the worktree, and git push runs it through the hook, so backgrounding either races any edit or staging that happens while it is in flight. The rule now separates the two: narrate every long command, but background only the ones that do not write to the worktree -- builds, test runs, spotlessCheck. spotlessApply and git push stay in the foreground with the wait narrated. That is the same partial-application shape the retro is about: a real problem, a fix that covered most of it. Two corrections of fact. `git add -u` does not stage untracked files -- it is tracked paths and deletions -- so grouping it with `git add -A` as sweeping in "whatever else" was wrong; the section now states each one's actual scope and keeps the warning that -u still picks up a tracked file something rewrote behind your back, which is exactly what happened here. And the stale-approval check should compare the approving review's commit.oid with headRefOid rather than timestamps, since latestReviews can return an empty commit.oid. Blank lines around the retro tables for MD058. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 6 ++++-- docs/process/learnings.md | 2 +- docs/process/retrospective.md | 6 ++++++ 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 36ee92c52d..9839d710f1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,7 +18,9 @@ flox activate -d flox/local -- ./gradlew - **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: background them and narrate.** Anything that can exceed ~60s — `spotlessApply`/`spotlessCheck` (~4.5 min here), `assembleV8Debug`, a full test sweep, and `git push` (the pre-push hook runs Spotless, so a push is itself a multi-minute silent command) — runs in the background, not the foreground. While it runs, say what you see every couple of minutes: 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. +- **Long-running commands: narrate them, and background only the read-only ones.** Anything that can exceed ~60s — `spotlessApply`/`spotlessCheck` (~4.5 min here), `assembleV8Debug`, a full test sweep, and `git push` (the pre-push hook runs Spotless, so a push is itself a multi-minute silent command) — 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. @@ -110,7 +112,7 @@ The sonarqube MCP server runs in Docker, so Docker must be up before launching C ### Staging commits — no `git add -A` -Stage by explicit path (`git add path/one path/two`). `git add -A` / `git add -u` sweeps in whatever else the working tree happens to hold — regenerated test fixtures, multi-MB binaries, files carrying machine-local absolute paths — and buries them in an unrelated commit. This repo has tracked fixtures that a test run rewrites, so the risk is live rather than theoretical. 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. +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. This repo has tracked fixtures that a test run rewrites, so the risk is live rather than theoretical. 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. ### Multi-line git/gh messages diff --git a/docs/process/learnings.md b/docs/process/learnings.md index 066c182e27..ff1322904b 100644 --- a/docs/process/learnings.md +++ b/docs/process/learnings.md @@ -7,7 +7,7 @@ - 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 approval's `submittedAt` with the last commit's date (`gh pr view --json latestReviews,commits`) before treating anything as merge-ready. +- **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 --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 --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 diff --git a/docs/process/retrospective.md b/docs/process/retrospective.md index c8e64d7280..d44a44f283 100644 --- a/docs/process/retrospective.md +++ b/docs/process/retrospective.md @@ -3,6 +3,7 @@ ## 2026-08-24 - Documentation transports, the Brotli dictionary migration, and 24 review threads ### Time Breakdown + | Started | Phase | 👤 Hands-On Time | 🤖 Agent Time | Problems | |---------|-------|-----------------|---------------|----------| | Aug 18 04:42 | Migration script, dictionary re-mint, ODT work | ██████████████████████ 221m | ███████████████████████████████████████████ 422m | ⚠ ProcessPool/forkserver, adb-push mtime trap | @@ -14,6 +15,7 @@ | Aug 24 22:36 | Four code reviews of my own PRs, and their fixes | ███████████████ 154m | █████████ 89m | ⚠ every review found real defects | ### Metrics + | Metric | Duration | |--------|----------| | Total wall-clock | Aug 18 -> Aug 25 (~163h calendar span) | @@ -38,6 +40,7 @@ Caveat on the last phase: 154m of "hands-on" counts ~6,900 words of machine-gene **Do differently:** "Give me more feedback on long-running tasks so I know if the task is stuck." ### Actions Taken + | Issue | Action Type | Change | |-------|-------------|--------| | Long commands run silently | CLAUDE.md | "Build & test": background anything over ~60s (including `git push`, which runs Spotless via the hook) and report elapsed time, last output line, and whether it is still progressing | @@ -96,6 +99,7 @@ Caveat on the last phase: 154m of "hands-on" counts ~6,900 words of machine-gene ## 2026-07-24 - LeakCanary icon shrink (ADFA-4843), JAXP/PDF.js investigations (ADFA-1491/ADFA-3304), and full blankj:utilcodex removal (ADFA-4649) ### Time Breakdown + | Started | Phase | 👤 Hands-On Time | 🤖 Agent Time | Problems | |---------|-------|-----------------|---------------|----------| | Jul 24 7:52pm | LeakCanary (ADFA-4843): investigate → decide → build → PR | ██ 4m | ██ 18m | | @@ -106,6 +110,7 @@ Caveat on the last phase: 154m of "hands-on" counts ~6,900 words of machine-gene | Jul 24 11:44pm | Jira progress, retro resume | █ 1m | | | ### Metrics + | Metric | Duration | |--------|----------| | Total wall-clock | ~3h 52m | @@ -126,6 +131,7 @@ Caveat on the last phase: 154m of "hands-on" counts ~6,900 words of machine-gene **What didn't:** Waiting for the build system to create an APK — inherent friction in this multi-module Android project, not a request to change approach. ### Actions Taken + | Issue | Action Type | Change | |-------|-------------|--------| | No standing guidance to prefer targeted compiles over full assembles during iteration | CLAUDE.md | Added a "Fast iteration" bullet to Build & test: batch targeted `:module:compileV8DebugKotlin` calls during iteration, reserve `:app:assembleV8Debug` for final verification | From fc7ce5f42988202f44344aa8db07b6f8ef1a29d6 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 24 Aug 2026 18:26:20 -0700 Subject: [PATCH 3/4] docs: correct the Spotless timing claim by measuring it I wrote that Spotless costs ~4.5 minutes and that :spotlessShell accounts for nearly all of it by walking scripts/**. Both are wrong, and the rule I had just written -- every claim needs its check -- is what says to measure before repeating them. Measured on this repo, warm daemon: spotlessCheck 6.5s total, of which :spotlessShell is 750ms, :spotlessKotlin 4.4s, :spotlessXml 2.7s. Cold: spotlessCheck 22s, against 20s for `gradlew help` -- so essentially all of a cold run is daemon start plus configuring this many modules, and Spotless adds ~2s. scripts/ and .githooks together are 38 files; there is no walk to prune. The 4.5 minutes I measured earlier was real but misattributed: at that point the machine was carrying two Gradle daemons, a Kotlin daemon and an orphaned test JVM holding 830 MB, with a daemon having already been OOM-killed that session. What survives is the pre-push double cost -- the hook runs spotlessApply, and if it changes anything the push fails and you pay the invocation twice -- and the narration rule, which is about any long command rather than about Spotless. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 2 +- docs/process/learnings.md | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9839d710f1..f513693a20 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,7 +18,7 @@ flox activate -d flox/local -- ./gradlew - **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 — `spotlessApply`/`spotlessCheck` (~4.5 min here), `assembleV8Debug`, a full test sweep, and `git push` (the pre-push hook runs Spotless, so a push is itself a multi-minute silent command) — 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. +- **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. diff --git a/docs/process/learnings.md b/docs/process/learnings.md index ff1322904b..aa867b5d98 100644 --- a/docs/process/learnings.md +++ b/docs/process/learnings.md @@ -1,7 +1,8 @@ # Learnings ## Spotless / formatting -- **A tripped pre-push hook costs two full Spotless runs, 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 the ~4.5 min again. Run `spotlessApply` yourself before committing. `:spotlessShell` alone accounts for nearly all of that time (it walks `scripts/**`); the rest of the Spotless tasks are seconds. +- **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). From 8cfc586957b66ad3514035440fed47ca2f08c3fe Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 26 Aug 2026 17:58:17 -0700 Subject: [PATCH 4/4] docs: correct two retro claims that went stale before merge Both said the tracked-fixture hazard is live. ADFA-5264 (#1740) merged to stage in the meantime and ignores testing/resources/test-project/.cg/ entirely, so nothing under it is tracked any more. The learnings entry now records what happened and the shape worth remembering -- a tracked file a test run rewrites cannot be kept clean by discipline, only by untracking it -- in past tense. CLAUDE.md's git-add-A rule stands on its own; only its justification needed replacing. The untracked half of the risk is still live, and tests/ is the current example: :gradle-plugin:test leaves files there and tests/test-home is not ignored on stage today. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU --- CLAUDE.md | 2 +- docs/process/learnings.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f513693a20..edb0a1a9fb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -112,7 +112,7 @@ The sonarqube MCP server runs in Docker, so Docker must be up before launching C ### 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. This repo has tracked fixtures that a test run rewrites, so the risk is live rather than theoretical. 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. +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. ### Multi-line git/gh messages diff --git a/docs/process/learnings.md b/docs/process/learnings.md index aa867b5d98..abeca20c8a 100644 --- a/docs/process/learnings.md +++ b/docs/process/learnings.md @@ -33,7 +33,7 @@ - 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` are **tracked** and a test run rewrites them with the local machine's absolute paths (ADFA-5264). Reverting them before running the tests does not stick — the next run re-dirties them — so revert *after* your last test run and check `git status` before committing. +- `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() }`.