diff --git a/CLAUDE.md b/CLAUDE.md index dc3b64ee94..0240747a43 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -37,9 +37,11 @@ See **[ARCHITECTURE.md](ARCHITECTURE.md)** — the single source of truth for th - **Avoid new dependencies** — the build almost certainly already has what's needed. Check `gradle/libs.versions.toml` and `build.gradle.kts` first. - **Persistence:** prefer **Room** for relational data and the filesystem/preferences for settings; raw SQLite only for justified exceptions — see [ADR 0001](docs/adr/0001-prefer-room-for-persistence.md). -- **Don't treat a large binary asset's on-disk content as ground truth without checking its provenance first.** Run `git ls-files ` / `git check-ignore -v `, and grep the build files for how it's provisioned, before relying on its current schema or row content. Several assets here (e.g. `assets/documentation.db`, and the SDK/bootstrap/Gradle zips alongside it) are `.gitignore`d and fetched by a Gradle task from an external URL (see the `Asset(...)` list in `app/build.gradle.kts`) — a locally-cached copy can be stale independent of git commit history and silently diverge from the maintained original. +- **Don't treat a large binary asset's on-disk content as ground truth without checking its provenance first.** Run `git ls-files ` / `git check-ignore -v `, and grep the build files for how it's provisioned, before relying on its current schema or row content. Several assets here (e.g. `assets/documentation.db`, and the SDK/bootstrap/Gradle zips alongside it) are `.gitignore`d and fetched by a Gradle task from an external URL (see the `Asset(...)` list in `app/build.gradle.kts`) — a locally-cached copy can be stale independent of git commit history and silently diverge from the maintained original. This includes at least one *tracked* asset too (`assets/core.cgt`): running Gradle for any reason — including the pre-push hook's `spotlessCheck` — can silently re-fetch/regenerate it even when your change is unrelated, dirtying the working tree. Check `git status`/`git diff --stat` before any broad staging (`git add -A`) so a regenerated asset copy doesn't ride along into an unrelated commit. - **Protect the two Android system bars** in any UI work: the top status bar (clock, notifications, status icons) and the bottom navigation bar (home, back, recents). Don't draw over or intercept them. -- **Plan and size before building.** Prefer **one PR per ticket/use case** — don't force-split a coherent change (splitting has its own overhead when later edits span the pieces). When a change is large, break it into **reviewable commits** — mechanical/refactor commits separate from behavioral ones — and offer review-by-commit. Treat ~500 LOC / ~10 files as a signal to reach for that commit structure, not a hard cap; the ceiling rises as LLM-assisted review matures. For staged multi-commit refactors (e.g. removing a dependency across many files/modules), order stages easiest-to-hardest and independently compile/test each stage (see Build & test's fast-iteration guidance) before moving to the next, so a failure is isolated to the stage that caused it. +- **Plan and size before building.** Prefer **one PR per ticket/use case** — don't force-split a coherent change (splitting has its own overhead when later edits span the pieces). When a change is large, break it into **reviewable commits** — mechanical/refactor commits separate from behavioral ones — and offer review-by-commit. Treat ~500 LOC / ~10 files as a signal to reach for that commit structure, not a hard cap; the ceiling rises as LLM-assisted review matures. For staged multi-commit refactors (e.g. removing a dependency across many files/modules), order stages easiest-to-hardest and independently compile/test each stage (see Build & test's fast-iteration guidance) before moving to the next, so a failure is isolated to the stage that caused it. For any change touching a cross-repo or otherwise hard-to-migrate schema (e.g. `documentation.db`'s tables, owned by the separate `OfflineDocumentationTools` project — see `docs/documentation-database.md`), state the proposed schema/design explicitly and pause for confirmation before writing code; walking back a converged-on-after-the-fact schema costs more than one extra round of discussion up front. +- **Grep for existing call sites before using a native/loader API.** Before writing code that touches a native library's real decoder/encoder (e.g. brotli4j), grep for existing call sites of its init/loader method (`Brotli4jLoader.ensureAvailability()` — see `ToolsManager.java`, `AssetsInstallationHelper.kt`, `BrotliCompressor.kt`, `AddBrotliFileToAssetsTask.kt`) rather than rediscovering the requirement via a native crash. Also verify a version-catalog artifact declared for a native/desktop platform (e.g. `libs.brotli4j.linux.x64`) is actually wired as a `testImplementation`/`implementation` somewhere — a catalog entry with zero consumers means no prior test ever exercised that code path, so it can be silently broken. +- **Default batch/migration scripts over many independent rows to parallel execution**, not sequential, when each row's work is dominated by process-spawn or I/O overhead (e.g. shelling out to a CLI tool per row) rather than shared state — a thread pool is usually a 3–10x win for negligible extra complexity, and accepting a slow serial run is a missed offer, not a safe default. - **Keep docs in step with code.** When you change code, update the docs that describe it in the same change — a module's `README.md`, `ARCHITECTURE.md`, or an ADR — so a doc never outlives the API it documents (see REVIEW.md, Code quality). If the doc fix is out of scope, file a ticket rather than let it drift. - `.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. diff --git a/docs/process/learnings.md b/docs/process/learnings.md index 7c4224a00d..db2a8000e4 100644 --- a/docs/process/learnings.md +++ b/docs/process/learnings.md @@ -27,3 +27,21 @@ ## 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(...)`. + +## Brotli with a custom/raw dictionary +- A dictionary-compressed Brotli stream and a plain one are **not reliably distinguishable at decode time**. Verified empirically: decoding with the wrong dictionary (a different one, "none" when one was used, or vice versa) does not consistently throw — it can "succeed" while silently returning different bytes than were originally compressed, depending on how the corrupted back-references happen to land (200-trial stress test: plain decode of dictionary-compressed data failed 100% of the time; decode with a *different* dictionary sometimes decoded to wrong bytes without erroring at all). The only safe design is a single dictionary that's never retrained/replaced once anything has been compressed against it — a per-row "which dictionary/none" flag looks appealing but can't be verified after the fact if it's ever wrong. +- brotli4j's raw-dictionary API (`BrotliInputStream.attachDictionary(ByteBuffer)`, `Encoder`/`PreparedDictionary` on the encode side) requires a **direct** `ByteBuffer` (`ByteBuffer.allocateDirect(...)`) — a heap-backed `ByteBuffer.wrap(byteArray)` throws `IllegalArgumentException: only direct buffers allowed`. A single direct buffer instance is safe to reuse across many decode calls (position/limit reset to the full buffer after each use in testing) — no need to `duplicate()` per call. +- Cross-tool compatibility isn't a given: verify empirically (e.g. a small standalone JVM program using the project's actual native jars, fed a fixture produced by the *other* tool's real code path) that a dictionary trained/applied by one tool's CLI (zstd `--train-fastcover` + `brotli -D`) decodes correctly via a different tool's library API (brotli4j) — don't assume compatibility from both being "the same" underlying format. +- The Python `brotli` package (installed via pip) has **no dictionary parameter at all** on `compress()`/`decompress()` — dictionary-aware compression from Python means shelling out to the `brotli` CLI's `-D` flag (or switching to `brotlicffi`, not evaluated here). + +## Android on-device testing (adb) +- A flaky adb connection that enumerates then drops on the very next command is worth diagnosing, not just retrying blind: `lsusb -t` / `/sys/bus/usb/devices/*/speed` shows negotiated link speed, and `lsusb -v` shows the device's USB mode (e.g. `idProduct ... "Galaxy series, misc. (MTP mode)"`). MTP mode (file-transfer interfaces alongside ADB) and hub-routed connections (multiple devices sharing one upstream bus) are both more prone to this kind of transient drop than a direct, ADB-only connection. +- `adb shell monkey -p -c android.intent.category.LAUNCHER 1` can launch the **wrong** activity in a debug build that bundles LeakCanary — LeakCanary registers its own `LAUNCHER`-category activity (`leakcanary.internal.activity.LeakLauncherActivity`), and monkey doesn't guarantee which matching activity it picks. Launch the real entry point explicitly instead: `adb shell am start -n /.activities.` (find it via the `` with the `MAIN`/`LAUNCHER` intent-filter in `AndroidManifest.xml`). + +## Batch/migration script performance +- A per-row CLI-subprocess-bound batch script (e.g. shelling out to `brotli` once per database row) should default to parallel execution, not sequential — process-spawn overhead, not CPU work, dominates at scale, and a thread pool is a 3-6x wall-clock win for negligible added complexity (measured on a 30,000-row real migration). `ThreadPoolExecutor(max_workers=None)` already defaults to `min(32, cpu_count+4)`, tuned for exactly this I/O/subprocess-bound shape — no need to hand-pick a worker count. +- When parallelizing row-by-row work against SQLite: a single `sqlite3.Connection` isn't safe to share across threads. Give each worker its own **read-only** connection (`sqlite3.connect(f"file:{path}?mode=ro", uri=True)`) for the read-heavy part, and keep the actual writes (delete/insert) serialized on the original caller's connection — SQLite requires serialized writes anyway, and writes are typically fast relative to the parallelizable read+compress work. +- An in-memory SQLite database (`:memory:`) has no file path and can't be shared across connections at all — a test fixture that needs to exercise multi-connection code (e.g. a parallelized script opening its own worker connections) must use a real temp file, not `:memory:`. + +## Claude Code tooling +- `ScheduleWakeup` is scoped to `/loop` dynamic-mode sessions specifically — it is not a general-purpose "check back on this background task later" mechanism. A `Bash` command started with `run_in_background: true` already delivers a completion notification on its own; there's no need to schedule a separate wakeup timer to poll it, and doing so outside a `/loop` session fires a wakeup with a sentinel prompt that doesn't apply to the conversation. diff --git a/docs/process/retrospective.md b/docs/process/retrospective.md index fb4eeadd34..da515b660c 100644 --- a/docs/process/retrospective.md +++ b/docs/process/retrospective.md @@ -1,5 +1,54 @@ # Retrospective Log +## 2026-08-14 - ADFA-5153: shared-dictionary Brotli compression for documentation.db (cross-repo, + docdb-studio fix) + +### Time Breakdown + +| Started | Phase | 👤 Hands-On Time | 🤖 Agent Time | Problems | +|---------|-------|-----------------|---------------|----------| +| Aug 14 7:48pm | Investigate WebView/dictionary support, discover 3-pipeline fragmentation, iterate schema design | ██▌ 25m | ████████▌ 85m | ⚠ schema redesigned 3x; one ~67min research+background-agent stretch | +| Aug 14 9:40pm | Implement dictionary pipeline (`populate_db.py`, migration script), kick off real-DB migration | █▌ 15m | █ 10m | | +| Aug 14 10:06pm | `WebServer.kt` read-side, parallel testing | █▌ 14m | █▌ 15m | ⚠ 3 fix-rerun cycles (2 real gaps, 1 self-inflicted) | +| Aug 14 10:28pm | Docs, Jira, commits (both repos) | ▌ 7m | ▌ 2m | | +| Aug 14 10:37pm | On-device deploy & verify | █▌ 14m | █ 10m | ⚠ adb/USB dropped 3x; wrong launcher activity first try | +| Aug 14 11:10pm | `docdb-studio` fix, push + PRs, architecture review, retro, parallelize migration script | ▌ 5m | ████████▌ 85m | ⚠ pre-push hook silently dirtied an unrelated binary asset (caught, not committed) | + +### Metrics + +| Metric | Duration | +|--------|----------| +| Total wall-clock | ~3h 44m | +| Hands-on | ~80 min (36%) | +| Automated agent time | ~144 min (64%) | +| Idle/testing/away | included in agent time above (background builds/tests ran concurrently with conversation) | +| Retro analysis time | ~5 min | + +### Key Observations +- The agent worked most independently during the ~67-minute early research stretch (background Explore subagent + WebSearch/WebFetch + `javap` on the real brotli4j jar) and during the final docdb-studio fix — both were genuinely open technical questions resolvable by investigation rather than needing user input, and both surfaced real, non-obvious findings (Compression Dictionary Transport's actual mechanics; a mismatched Brotli dictionary decodes silently wrong rather than failing loudly). +- Most user interaction was in the schema-design phase: the user corrected the design three times (per-row dictionary/no-dictionary flag → single dictionary embedded in the database → "convert everything, never retrain"). Each correction was a real simplification, but it took ~5 rounds to converge — see the new CLAUDE.md guidance below. +- Two real, avoidable gaps got caught and fixed during WebServer.kt testing: a missing `Brotli4jLoader.ensureAvailability()` call in a new JVM test (a quick grep of 4 existing call sites would have caught it before the first failed run), and the app module's test dependencies never having a desktop-native brotli4j artifact wired in at all (pre-existing, unrelated to this session, but only surfaced now that a test actually exercised brotli4j's real decoder on JVM). +- One near-miss handled well, not turned into a mistake: the pre-push hook's Gradle invocation (`spotlessCheck`) silently regenerated an unrelated, already-committed binary asset (`assets/core.cgt`, externally-fetched) — caught via `git status`/`git diff --stat` before staging, restored, and excluded from the commit. +- **User feedback (direct):** the whole-database migration script (`migrate_content_to_dictionary_brotli.py`) processed ~30,000 Content rows strictly sequentially, each spawning its own `brotli` subprocess — this should have been parallelized proactively rather than accepted as a slow serial run. Fixed post-hoc: a `ThreadPoolExecutor`-based rewrite measured 3-6x faster on synthetic benchmarks, pushed as an update to the still-open PR. + +### Feedback +**What worked:** Not directly asked before the session's main work concluded; the user's engagement pattern (terse directives, decisive corrections, real-device verification) mirrors prior sessions' noted preference for low-friction, high-trust collaboration. +**What didn't:** "Claude could have offered to parallelize the conversion of the old database format to the new database format. Doing thousands of rows in the content table took a long time and could have been ten times faster with parallelization." (direct user feedback, acted on immediately) + +### Actions Taken + +| Issue | Action Type | Change | +|-------|-------------|--------| +| No guidance to grep for existing native/loader-API call sites before writing new code against one (missed `Brotli4jLoader.ensureAvailability()`) | CLAUDE.md | Added bullet to Project-specific constraints: grep for existing init/loader call sites; verify a version-catalog native/desktop artifact is actually wired as a dependency, not just declared | +| Pre-push hook's Gradle run silently regenerated an unrelated tracked asset (`assets/core.cgt`) | CLAUDE.md | Extended the existing binary-asset-provenance bullet: any Gradle invocation (including hooks) can silently re-fetch these assets; check `git status`/`git diff --stat` before broad staging | +| Schema design redirected 3x before converging | CLAUDE.md | Extended "Plan and size before building": for cross-repo/hard-to-migrate schema changes, state the proposed design explicitly and pause for confirmation before writing code | +| Migration script ran serially over ~30,000 rows instead of being offered/built parallel from the start | CLAUDE.md + code fix | Added standing rule (default per-row batch/migration scripts to parallel execution when dominated by process-spawn/I/O overhead); also parallelized `migrate_content_to_dictionary_brotli.py` itself via `ThreadPoolExecutor`, pushed to the open PR | +| Brotli dictionary-mismatch decode behavior (silently wrong, not a reliable failure) | learnings.md | New "Brotli with a custom/raw dictionary" section (4 entries: mismatch behavior, direct-`ByteBuffer` requirement, cross-tool verification approach, Python `brotli` package's lack of dictionary support) | +| ADB/USB flakiness (MTP mode + hub routing dropped the connection repeatedly) | learnings.md | New "Android on-device testing (adb)" section: `lsusb -t`/`/sys/bus/usb/devices/*/speed` diagnosis technique | +| `monkey -c LAUNCHER` opened LeakCanary's debug-build launcher instead of the app | learnings.md | Same section: use `am start -n /.activities.` explicitly in debug builds bundling LeakCanary | +| Batch-script parallelization pattern (SQLite thread-safety, `:memory:` vs real file for multi-connection tests) | learnings.md | New "Batch/migration script performance" section (3 entries) | +| Called `ScheduleWakeup` outside a `/loop` context (self-caught, reverted) | learnings.md | New "Claude Code tooling" section: that tool is `/loop`-mode only; background `Bash` tasks already self-notify | +| ~67 minutes of empirical pre-verification (WebView/brotli4j internals) before writing code | No action | One-off judgment call on a genuinely non-obvious correctness risk, not a repeatable repo-specific policy | + ## 2026-08-13 - ADFA-5088: individual Preferences/Plugin Manager tooltips + docdb SQL scripts ### Time Breakdown