diff --git a/.claude/skills/retro/scripts/analyze_transcript.py b/.claude/skills/retro/scripts/analyze_transcript.py index bca5fc269e..8f367e20c0 100644 --- a/.claude/skills/retro/scripts/analyze_transcript.py +++ b/.claude/skills/retro/scripts/analyze_transcript.py @@ -25,6 +25,9 @@ - Skill injections ("Base directory for this skill:") - Local command outputs (, ) + - Image tool results ("[Image: original ...]"), which are the agent's own Read of a + screenshot arriving in the human role -- counting those as human turns adds reading + and buffer time nobody spent (~11 of 51 minutes in one session that drove a device). """ from __future__ import annotations @@ -55,6 +58,7 @@ SYSTEM_MESSAGE_PATTERNS = [ re.compile(r"^Base directory for this skill:"), + re.compile(r"^\[Image: original \d+x\d+"), re.compile(r"^<(command-name|local-command|system-reminder)"), re.compile(r"^"), re.compile(r"^This session is being continued from a previous conversation"), diff --git a/CLAUDE.md b/CLAUDE.md index 34a25504f4..6b5d2775ea 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,6 +57,7 @@ See **[ARCHITECTURE.md](ARCHITECTURE.md)** — the single source of truth for th - **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. - **Every screen must survive 2x font scale.** Users with low vision run large system fonts, and a screen that clips or hides content at 2.0 is broken for them. Verify any new or changed screen at font scale **1.0 and 2.0** (see Build & test, Emulator / device) and say in the PR that you did. Text grows, so: use `sp` for text and `dp` for spacing — never an `sp` dimen as a margin or padding; don't box text in a fixed `dp` height or width; give content that can grow somewhere to scroll; and reserve `maxLines`/`singleLine`/`ellipsize` for text that is genuinely disposable. - **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. +- **Before designing a way to tune a mechanism, ask whether the mechanism can go.** When the evidence for a problem scales with a *rate* or a *volume* — connections per second, requests per page, bytes per call — the cheapest fix is usually to stop generating them, not to make each one better. Check what the platform already offers to remove the mechanism entirely (ADFA-5172/5176: a per-request TCP connection to our own process, where `WebViewClient.shouldInterceptRequest` removed the socket instead of HTTP keep-alive making it cheaper) before writing the plan for the tuned version. - **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. @@ -65,6 +66,8 @@ See **[ARCHITECTURE.md](ARCHITECTURE.md)** — the single source of truth for th **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`). +When the ratchet pulls a whole file in, **land that reformat as its own commit, before the behavioral one**. A 160-line whitespace diff sitting on top of a 10-line change hides the change; split, and the reviewer reads what matters. Say so in the reformat commit's message so nobody hunts for behavior in it. + 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. **Code comments** follow the same discipline: diff --git a/docs/process/learnings.md b/docs/process/learnings.md index 7c4224a00d..654a72f167 100644 --- a/docs/process/learnings.md +++ b/docs/process/learnings.md @@ -8,12 +8,23 @@ - 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 +- A config data class whose **default** values call framework APIs (e.g. `ServerConfig`'s paths default to `Environment.getExternalStorageDirectory()`) makes itself unconstructable in a JVM unit test — `RuntimeException: Method ... not mocked`, thrown from the constructor before your test body runs. Any new test has to pass *every* such parameter explicitly, which is easy to miss when copying a config from a test that already does. Prefer lazily-resolved paths in new config types. - `Handler.removeCallbacks(Runnable)` only removes callbacks posted by that *exact* `Handler` instance, not just the same `Looper` — `Handler(Looper.getMainLooper()).removeCallbacks(x)` won't cancel something posted via a *different* `Handler` bound to the same looper. Any post/cancel pair needs to share one `Handler` instance (see `TaskExecutor.mainThreadHandler`, added when replacing blankj's `ThreadUtils.getMainHandler()`). +## Serving content to a WebView +- A WebView can be handed content **in-process** through `WebViewClient.shouldInterceptRequest`, returning a `WebResourceResponse` built from a stream — no socket, no port, no handshake. It intercepts *whatever URL the WebView loads*, so an existing `http://localhost:PORT/...` URL space needs **no rewriting**: strings.xml entries, link builders and even a published plugin-API contract keep working while the transport underneath changes (ADFA-5176 turned 31 TCP connections per documentation page into 0 this way). +- A WebView does **not** decode an intercepted response, so hand back decompressed bytes and don't bother with `Content-Encoding`. Give `WebResourceResponse` the bare MIME type with the charset as its own argument, and pass `null` for binary types — claiming a charset on an image makes the WebView try to decode it as text. +- `shouldInterceptRequest` never sees a POST body, and `WebResourceResponse` can't answer a range request with 206. Neither mattered for documentation (the WebView asked for a whole 407 KB PDF), but a range-dependent viewer would need the socket path. +- Android's WebView cannot render a PDF at all: pointing one at a `application/pdf` URL shows a blank page, identically over HTTP or in-process. + +## Android system SQLite +- Don't assume the JSON1 extension. On a Samsung Android 13 device, `JSON_OBJECT`/`JSON_GROUP_ARRAY` fail at runtime with `no such function: JSON_OBJECT` even though the same query runs fine against the same database file under a desktop sqlite3. Any query using JSON functions needs either a fallback or a documented minimum, and a JSON-based endpoint can be dead on real hardware while passing every desktop test. + ## Reverse-engineering a library before porting it - When writing a same-name drop-in for a third-party utility (to remove the dependency without changing call-site behavior), don't guess its semantics from memory/docs — extract the AAR's `classes.jar` and run `javap -c` against the actual bytecode to confirm exact chaining/wrapping behavior, especially for fluent/reflection-style APIs where a subtle mismatch (e.g., wrapping a field's *declared* type vs. its *runtime* class) changes behavior at existing call sites. ## MockK +- To unit-test code that touches WebView plumbing without Robolectric: `mockkStatic(android.os.Environment::class)` for `getExternalStorageDirectory()`, and a plain `mockk` stubbing only `host`/`port`/`path`. Keep the framework *construction* out of the unit under test — a `WebResourceResponse` constructor throws `Stub!` in a JVM test, so split the decision (which content answers this request) from the wrapping, and test the decision. - Migrating a mocked call from a Java static method (`mockkStatic(SomeClass::class)`) to a Kotlin top-level extension function requires `mockkStatic("com.package.FileNameKt")` (the compiled JVM facade class name) instead — `mockkStatic(ExtensionReceiver::class)` doesn't work for extension functions. ## Measuring a real before/after delta diff --git a/docs/process/retrospective.md b/docs/process/retrospective.md index fb4eeadd34..521c464307 100644 --- a/docs/process/retrospective.md +++ b/docs/process/retrospective.md @@ -1,5 +1,57 @@ # Retrospective Log +## 2026-08-18 - ADFA-5172/5175/5176: the local WebServer's 1 s stall, and removing the socket instead + +### Time Breakdown +| Started | Phase | 👤 Hands-On Time | 🤖 Agent Time | Problems | +|---------|-------|-----------------|---------------|----------| +| Aug 17 9:42pm | Ticket read + accept-loop instrumentation | ██ 7m | █ 12m | | +| Aug 17 9:54pm | Build, drive, root-cause the stall | ▌5m | ███ 30m | ⚠ HelpActivity not exported, so the measurement needed a throwaway manifest tweak; one flaky arm | +| Aug 17 10:28pm | Keep-alive design + ADFA-5175 filed | █ 10m | █ 9m | | +| Aug 17 10:37pm | ADFA-5175 stage 1, transport pivot, ADFA-5176 spike | █ 8m | ██████████ 100m | ⚠ 3 Spotless whole-file reformats; direction changed mid-implementation | +| Aug 18 12:26am | Extraction onto the ADFA-5153 base | ▌5m | █████████████ 130m | ⚠ merge conflicts, plus a stale KDoc and dangling brace from moving code by script | +| Aug 18 2:39am | Tests, Pebble move, cleanup, two PRs | █ 8m | ██████████████ 140m | ⚠ tests written just before the API they cover moved | +| Aug 18 5:01am | Review fixes + CodeRabbit replies | █ 11m | ████ 40m | | +| Aug 18 8:03am | Retro | ▌1m | ██ 20m | | + +### Metrics +| Metric | Duration | +|--------|----------| +| Total wall-clock | 10h 21m | +| Hands-on | 53 min (9%) | +| Automated agent time | ~6h 20m (61%) | +| Idle/testing/away | ~3h 10m (30%) | +| Retro analysis time | 6 min | +| Cost | $344 (481+ calls, 594K output tokens) | + +13 user messages, most of them one to three words. Only user-message timestamps are exact, so the agent/idle split is estimated from the work performed. + +### Key Observations +- The two longest unattended stretches were the most productive: "build and drive" (30m, root cause established with kernel counters and a control-listener comparison) and "proceed" (130m, a cross-module extraction, built and device-verified). Three-word prompts, high leverage. +- **The most valuable question came from the user, and should have come from the agent.** "Could we use a different transport?" arrived *after* ADFA-5175 was filed and keep-alive was already being built. The agent's own evidence -- drop rate scaling with connection *rate* -- pointed at "open fewer connections", and `shouldInterceptRequest` was the obvious mechanism. It designed a way to tune the mechanism instead of asking whether the mechanism was needed. Result: a filed ticket whose plan was invalidated a day later, and the keep-alive work stopped after stage 1. +- Rework was formatting tax and transplant fixups, not logic: three whole-file Spotless reformats (~500 whitespace lines, kept out of behavioral diffs by hand), and 4-5 failed python patch asserts from over-long match anchors. +- Zero substantive corrections from the user across 13 messages. Steering, not fixing. +- The device work needed a temporary `android:exported="true"` on HelpActivity to be scriptable at all; it was kept on a throwaway branch and reverted, but it is a recurring cost of driving activities that are (correctly) not exported. +- The retro script counted the agent's own screenshot reads as user turns. Fixing it moved hands-on from 51 to 53 minutes rather than down as predicted: the phantom turns' buffers disappear, but their assistant output is re-attributed to the real prompts. + +### Feedback +**What worked:** Autonomy. The long unattended stretches were where the value was. +**What didn't:** The transport question should have come from the agent, not the user. + +### Actions Taken +| Issue | Action Type | Change | +|-------|-------------|--------| +| Designed keep-alive to tune a mechanism before asking whether the mechanism could go | CLAUDE.md | "Plan and size before building": new bullet -- when the evidence scales with a rate or volume, check whether the platform can remove the mechanism before planning the tuned version, citing ADFA-5172/5176 | +| Ratchet reformats risk burying behavioral diffs | CLAUDE.md | Code style: state the convention -- land a whole-file reformat as its own commit, before the behavioral one, and say so in its message | +| `ServerConfig`-style defaults that call framework APIs break any new JVM test | learnings.md | Added under Android / Kotlin, with the failure mode (constructor throws before the test body runs) | +| Testing WebView interception without Robolectric | learnings.md | Added under MockK: `mockkStatic(android.os.Environment::class)` plus a mocked `Uri`, and split the decision from the framework construction | +| How in-process WebView serving actually behaves | learnings.md | New "Serving content to a WebView" section: interception matches any URL so existing URL spaces need no rewriting; no response decoding; no POST body; no 206; WebView cannot render a PDF | +| Android system SQLite may lack JSON1 | learnings.md + ticket | New "Android system SQLite" section, plus ADFA-5179 | +| Retro script counted screenshot reads as user turns | Skill | `analyze_transcript.py`: filter `[Image: original NxN...]` tool results out of the human role | +| Bookshelf 500s where SQLite lacks JSON1 | Ticket | ADFA-5179 (Bug), linked to ADFA-5176 | +| Documentation PDFs render blank in HelpActivity | Ticket | ADFA-5180 (Bug), linked to ADFA-5176 | +| Tests written just before the API they cover moved | No action | One-off: the risk was flagged and the order was chosen deliberately; cost was ~10 lines of test edits | + ## 2026-08-13 - ADFA-5088: individual Preferences/Plugin Manager tooltips + docdb SQL scripts ### Time Breakdown