diff --git a/CLAUDE.md b/CLAUDE.md index 34a25504f4..edb0a1a9fb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,6 +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: 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. @@ -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. @@ -96,6 +110,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` 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 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 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 @@ -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 /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` 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(...)`. diff --git a/docs/process/retrospective.md b/docs/process/retrospective.md index fb4eeadd34..d44a44f283 100644 --- a/docs/process/retrospective.md +++ b/docs/process/retrospective.md @@ -1,5 +1,56 @@ # 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 @@ -48,6 +99,7 @@ ## 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 | | @@ -58,6 +110,7 @@ | Jul 24 11:44pm | Jira progress, retro resume | █ 1m | | | ### Metrics + | Metric | Duration | |--------|----------| | Total wall-clock | ~3h 52m | @@ -78,6 +131,7 @@ **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 |