From 0440a9e59ebbb88ee6779109e72269638de0326c Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 13 Aug 2026 00:36:43 -0700 Subject: [PATCH 1/3] docs: retro for ADFA-5088 (Preferences/Plugin Manager tooltips + docdb SQL) Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 1 + docs/process/learnings.md | 4 ++++ docs/process/retrospective.md | 42 +++++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 22b9a794b6..dc3b64ee94 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -37,6 +37,7 @@ 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. - **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. - **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. diff --git a/docs/process/learnings.md b/docs/process/learnings.md index 852a02c8b3..b4238ff61f 100644 --- a/docs/process/learnings.md +++ b/docs/process/learnings.md @@ -19,6 +19,10 @@ ## Measuring a real before/after delta - To measure an actual size/perf delta for a change (not just estimate it), use `git worktree add `, build there, and diff the artifacts — avoids disturbing the current working tree or stashing. +## SQLite CLI scripting +- The sqlite3 CLI's `.system` dot-command can hit a content-dependent shell-parsing failure when a line chains multiple operators (`;`, `&&`, `||`, parentheses) — reproduces for some strings and not others, so it won't show up in a quick smoke test. Keep each `.system` line to one plain `command | pipe > file`. +- `.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. + ## 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 4285d3c04a..3d3d7a5fc5 100644 --- a/docs/process/retrospective.md +++ b/docs/process/retrospective.md @@ -1,5 +1,47 @@ # Retrospective Log +## 2026-08-13 - ADFA-5088: individual Preferences/Plugin Manager tooltips + docdb SQL scripts + +### Time Breakdown +| Started | Phase | 👤 Hands-On Time | 🤖 Agent Time | Problems | +|---------|-------|-----------------|---------------|----------| +| Aug 12, 7:38am | Setup & research (branch, ticket, docdb schema + Preferences tag investigation via 2 background agents) | ▏ ~3m | ████ 42m | | +| Aug 13, 5:12am | Implement fixup commits + fold in Plugin Manager screen (investigated via background agent, then implemented) | ▌ ~5m | █████ 45m | ⚠ mid-session scope addition | +| Aug 13, 6:01am | Architecture review + open PR + Jira update | ▏ ~1m | ███ 28m | | +| Aug 13, 6:30am | Code review response — verified findings, discovered stale local DB, rewrote SQL scripts | ▋ ~6m | █ 13m | ⚠ near-miss: caught mid-review only because the user pushed back | +| Aug 13, 6:49am | Fixups, ADFA-5121 follow-up ticket, wrap-up | ▎ ~2m | ▋ 7m | | +| Aug 13, ~7:00am | Second review round: fail-fast SQL fix (`.bail on` + guard table), validated against user-supplied real DB copies (est.) | ▌ ~8m | ████ 35m | ⚠ `.system` shell-parsing rabbit hole before finding the right fix | +| Aug 13, ~8:00am | Retro + 2 follow-up PRs (docdb doc gotchas, CLAUDE.md provenance rule) (est.) | ▊ ~10m | ███ 25m | | + +*(A ~20.9h overnight gap between the first two phases is excluded from the bars/percentages below as idle time, not work. The last two rows are estimated from context, not re-run through the transcript-analysis script.)* + +### Metrics +| Metric | Duration | +|--------|----------| +| Total active wall-clock | ~4h | +| Hands-on | ~35 min (15%) | +| Automated agent time | ~195 min (85%) | +| Idle (overnight, between sessions) | ~20.9h (excluded above) | +| Retro analysis time | ~3 min (script run) + manual extension for later phases | + +### Key Observations +- **The one real near-miss**: a SQL script was built and validated against `assets/documentation.db` — a 213MB file that's `.gitignore`d and downloaded by a Gradle task, not a committed repo asset. Its schema and content were treated as ground truth (including writing "confirmed via sqlite3" claims into the script's own header) without ever running `git ls-files`/`git check-ignore` on it. The local copy was stale; the real database already had curated production content for 5 of the tags the script was about to write to, which would have been silently overwritten. Caught only because the user independently checked the schema and pushed back. +- **Second review round found a related, second-order bug**: a `BEGIN;...COMMIT;` wrapper without `.bail on` doesn't actually give atomicity — verified empirically that a mid-script SQL error still lets `COMMIT` through with whatever succeeded before it. Fixed with `.bail on` plus a temp-table `CHECK` constraint that turns a silently-empty Brotli payload into a catchable SQL error. +- **A costly (but ultimately abandoned) detour**: significant time went into reverse-engineering a content-dependent shell-parsing failure in the sqlite3 CLI's `.system` dot-command (some strings triggered a dash syntax error, most didn't, with no clean single hypothesis found). The eventual fix sidestepped the problem entirely — kept `.system` lines simple and did the fail-fast check in SQL instead of shell chaining — rather than continuing to chase the CLI quirk's root cause. +- **Good pattern reinforced twice**: both times a bulk SQL rewrite was needed under time pressure, it was done via a small Python script parsing and regenerating the statements programmatically, rather than hand-editing 60+ lines — this avoided introducing new content errors while doing a structural change. +- **Real-world validation loop with the user**: the user independently ran the scripts against copies of the real database (`.save`, current, `.new`) and handed back concrete artifacts (file paths, MD5 comparison) rather than descriptions — this was more useful than any amount of scratch-DB testing alone, and surfaced that an earlier script version had already partially, successfully applied to the "before" copy. + +### Feedback +**What worked:** Not directly stated this session — inferred from the user's engagement pattern (quick short replies, handing over real artifacts to check rather than describing them). +**What didn't:** "Don't make assumptions about large binary files. They may be maintained and updated outside the repository." (direct user feedback, in response to the stale-DB near-miss) + +### Actions Taken +| Issue | Action Type | Change | +|-------|-------------|--------| +| No standing guidance against treating a large binary asset's on-disk content as ground truth without checking provenance | CLAUDE.md | Added a bullet to "Project-specific constraints": check `git ls-files`/`git check-ignore` and how an asset is provisioned before trusting its schema/content — generalizes beyond docdb to ~6 other gitignored, externally-fetched assets in `app/build.gradle.kts` | +| `documentation.db`-specific provenance and SQL-authoring gotchas (`.system` chaining, `.bail on` + guard-table pattern) not documented anywhere a future SQL-script author would find them | Doc | `docs/documentation-database.md` updated via PR #1666 (ADFA-5123): provenance warning in "Where it lives", new "Writing one-off SQL scripts against this database" subsection | +| Dead `UseSytemShell` preference (class never instantiated, underlying setting never read elsewhere) found while auditing for tooltip coverage | Ticket | Filed ADFA-5121 | + ## 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 From 28b4963ca08d769b3a030f9d54861d78ea5a2ccb Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 13 Aug 2026 00:56:44 -0700 Subject: [PATCH 2/3] docs: add insecure-temp-file learning from ADFA-5088 follow-up Co-Authored-By: Claude Sonnet 5 --- docs/process/learnings.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/process/learnings.md b/docs/process/learnings.md index b4238ff61f..8c93043619 100644 --- a/docs/process/learnings.md +++ b/docs/process/learnings.md @@ -22,6 +22,7 @@ ## SQLite CLI scripting - The sqlite3 CLI's `.system` dot-command can hit a content-dependent shell-parsing failure when a line chains multiple operators (`;`, `&&`, `||`, parentheses) — reproduces for some strings and not others, so it won't show up in a quick smoke test. Keep each `.system` line to one plain `command | pipe > file`. - `.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. ## 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() }`. From 26a2025feffe27b96721e9b8cee1ab8c6097752e Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 13 Aug 2026 12:34:57 -0700 Subject: [PATCH 3/3] docs: address review feedback on retro docs - learnings.md: note that mkdir can itself fail the same way .bail can't see other .system failures, and that mktemp -d isn't a drop-in fix here since each .system line is its own subshell with no state carried to the next one. - retrospective.md: add the blank lines markdownlint (MD058) wants around the three new tables. Co-Authored-By: Claude Sonnet 5 --- docs/process/learnings.md | 2 +- docs/process/retrospective.md | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/process/learnings.md b/docs/process/learnings.md index 8c93043619..7c4224a00d 100644 --- a/docs/process/learnings.md +++ b/docs/process/learnings.md @@ -22,7 +22,7 @@ ## SQLite CLI scripting - The sqlite3 CLI's `.system` dot-command can hit a content-dependent shell-parsing failure when a line chains multiple operators (`;`, `&&`, `||`, parentheses) — reproduces for some strings and not others, so it won't show up in a quick smoke test. Keep each `.system` line to one plain `command | pipe > file`. - `.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. +- 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. ## 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() }`. diff --git a/docs/process/retrospective.md b/docs/process/retrospective.md index 3d3d7a5fc5..fb4eeadd34 100644 --- a/docs/process/retrospective.md +++ b/docs/process/retrospective.md @@ -3,6 +3,7 @@ ## 2026-08-13 - ADFA-5088: individual Preferences/Plugin Manager tooltips + docdb SQL scripts ### Time Breakdown + | Started | Phase | 👤 Hands-On Time | 🤖 Agent Time | Problems | |---------|-------|-----------------|---------------|----------| | Aug 12, 7:38am | Setup & research (branch, ticket, docdb schema + Preferences tag investigation via 2 background agents) | ▏ ~3m | ████ 42m | | @@ -16,6 +17,7 @@ *(A ~20.9h overnight gap between the first two phases is excluded from the bars/percentages below as idle time, not work. The last two rows are estimated from context, not re-run through the transcript-analysis script.)* ### Metrics + | Metric | Duration | |--------|----------| | Total active wall-clock | ~4h | @@ -36,6 +38,7 @@ **What didn't:** "Don't make assumptions about large binary files. They may be maintained and updated outside the repository." (direct user feedback, in response to the stale-DB near-miss) ### Actions Taken + | Issue | Action Type | Change | |-------|-------------|--------| | No standing guidance against treating a large binary asset's on-disk content as ground truth without checking provenance | CLAUDE.md | Added a bullet to "Project-specific constraints": check `git ls-files`/`git check-ignore` and how an asset is provisioned before trusting its schema/content — generalizes beyond docdb to ~6 other gitignored, externally-fetched assets in `app/build.gradle.kts` |