From 26a256267d5272eba8efd54419ed822a2543821a Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 18 Aug 2026 08:13:35 -0700 Subject: [PATCH] docs: retro for the WebServer stall work (ADFA-5172/5175/5176) Nine actions from the session, all executed. CLAUDE.md gains the lesson that cost the most: before designing a way to tune a mechanism, ask whether the mechanism can go. Keep-alive was filed and started before anyone asked whether documentation needed a socket at all, and the evidence -- drop rate scaling with connection rate -- had been pointing at "open fewer connections" the whole time. It also states the convention that kept three whole-file Spotless reformats out of the diffs that mattered. learnings.md gains what the session found the hard way: config defaults that call framework APIs make themselves unconstructable in a JVM test; how to test WebView interception without Robolectric; how in-process serving behaves (matches any URL, no response decoding, no POST body, no 206, and a WebView cannot render a PDF at all); and that Android's system SQLite may have no JSON1, which is what breaks the bookshelf on real hardware while passing every desktop test. The retro script counted the agent's own screenshot reads as user turns. Filtering them moved hands-on from 51 to 53 minutes rather than down: the phantom buffers go away, but their assistant output is re-attributed to the real prompts. Two pre-existing bugs found while verifying on device are now filed as ADFA-5179 (bookshelf 500 without JSON1) and ADFA-5180 (PDFs blank in HelpActivity). --- .../retro/scripts/analyze_transcript.py | 4 ++ CLAUDE.md | 3 ++ docs/process/learnings.md | 11 ++++ docs/process/retrospective.md | 52 +++++++++++++++++++ 4 files changed, 70 insertions(+) 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 dc3b64ee94..298e54295d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,6 +40,7 @@ See **[ARCHITECTURE.md](ARCHITECTURE.md)** — the single source of truth for th - **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. - **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. +- **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. @@ -48,6 +49,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