diff --git a/IMPROVEMENT-AUDIT.md b/IMPROVEMENT-AUDIT.md new file mode 100644 index 0000000000..37a3b363bd --- /dev/null +++ b/IMPROVEMENT-AUDIT.md @@ -0,0 +1,94 @@ +# Codebase Audit — VERIFIED edition (v2) + +Worktree: `../agent-device-improve` (branch `improve/codebase-audit`, HEAD `07023eb20`). +v1 findings from 5 parallel explorations; **every claim then adversarially verified by 8 independent agents against code at the same HEAD plus live GitHub state**. This version incorporates all verification verdicts: confirmed items keep their evidence, refuted/dangerous proposals are marked as such and not repeated as recommendations. Do not implement anything marked ✗ or ⚠ without re-reading the cited constraint. + +--- + +## Verified, act on these + +### V1. Android fill/scroll pay cold instrumentation lifecycles — wire the scope that already exists +Fill runs 4 full instrumentation lifecycles (7 on retry) because `fill-verification.ts` never passes `helperSessionScope`, while daemon-session helper scoping **already exists** (`src/core/interactors/android.ts:89-91`, PR #862). Scroll similarly falls back to one-shot instrumentation (`touch-helper.ts:108-136, 55-106`). Warm captures are ~100–400ms vs 10–25s cold. +- **Fix:** pass the existing daemon-session scope through fill/scroll's capture path. Release on session close via the existing retirement/quarantine machinery. +- ⚠ Handle abnormal daemon termination (crash/SIGKILL) explicitly: a warm session that never receives a close event is exactly the UiAutomation-squatting exposure the quarantine machinery exists for — happy-path release-on-close alone widens it. +- ⚠ **Do NOT share the captured hierarchy across the 0/150/350ms verification samples** — the samples exist to observe settling; share the warm session, not the captured data. +- ⚠ Skip `force-stop` only when `graceful.acknowledged && graceful.exited` (confirmed exit, not mere acknowledgement) or you reintroduce UiAutomation squatting. + +### V2. Android tap-readiness dumpsys storm — confirmed, understated +No-dialog common case runs both dumpsys variants per phase = 4 spawns/tap (`interaction-touch-android-readiness.ts:37-63`, `app-lifecycle.ts:126-133`); the `requireNoBlockingDialog` wait-loop variant reaches ~78 adb spawns (`android-system-dialog.ts:368-387`). +- **Fix:** parse one dump for focus + dialog together; cache the post-check ~1s; alternate probes in wait loops. Preserve incident-derived parse order (#1832). + +### V3. O(n²·d) descendant collection on every iOS snapshot — cleanest quick win +`noise.ts:305-321` rebuilds a full index Map per candidate via `collectDescendantsByParentIndex` (`tree.ts:38-53`); the WeakMap-cached `getDescendantEndPositions` primitive (`tree.ts:84-105`) is a semantically safe drop-in. Safe to land standalone — `noise.ts` is TS post-processing downstream of the Swift presentation arc (#1929 merged at this HEAD). + +### V4. Find ranking = unimplemented #1690 — strongest confirmed structural item +Per-candidate `Map`/viewport rebuilds fire on any ≥2 raw matches (`find-match-resolution.ts:113-137` → `interaction-targeting.ts:128-193`). Issue #1690 describes the exact fix; its executor-plan excerpts have drifted past #1876's split — re-verify before executing. +- Citation corrections: `snapshot-visibility.ts` is not on find's path (second rebuild is local `resolveRootViewportRect`); **do not delete `listSelectorChainMatches`** (legitimate callers exist) — only the redundant call in replay's `resolveRecordedTarget` (`packages/selectors/src/internal/replay.ts:100-139`) goes. + +### V5. Daemon code-signature walk — confirmed, mis-scoped fix +Measured: 801 files, ~40–50ms, every invocation even on healthy reuse, double on takeover (`daemon-client-lifecycle.ts:215,643`; `code-signature.ts:26-63`) — but **only on the dev-from-source path**; dist is 1–5ms. +- ⚠ v1's three fixes all miss the cost center: health fields don't change on source edits; build-time emission has no build to hook. **Right fix:** keep dist check as-is; give source mode a cheaper fingerprint (path:size list, no contents) or accept it. +- PR #1771 already fixed #1545's signature-regex root cause; #1545 remains open only for the orphan-daemon defect. + +### V6. Web backend process-per-command — confirmed empirically, fix needs upstream +~205ms measured per CLI call; click = 3, fill = 5, paced scroll up to 20; N+1 `get box @ref` (`agent-browser-provider.ts:168-195`, `agent-browser-snapshot.ts:21,39-50`). The pinned agent-browser CLI has **no serve/batch mode**, so a long-lived child isn't buildable from agent-device alone. +- **Action:** upstream feature request (batch composite commands / serve mode), or version-pinned socket-protocol work. + +### V7. Confirmed small wins +- **A4 deep-link memo:** plutil spawn per installed app, serial, uncached (`app-resolution.ts:181-196,315-340`); `createTtlMemo` pattern ready to reuse. +- **A11:** diagnostics double-redaction/per-event sort/sync append (`diagnostics.ts:140,214,241`); stale-bundle uninstalls → `Promise.allSettled` (sibling precedent) (`runner-session.ts:362-374`); `openIosDevice` booted-memo bypass is a one-line simplification corroborated by the code's own "~0.7s per spawn" comment (`app-launch.ts:157-166`); HarmonyOS per-scroll viewport dump cache (`harmonyos/snapshot.ts:46-56`) needs invalidation for split-screen/multi-window changes and for device rotation not routed through the `setOrientation` interactor — neither hits the proposed orientation/open hooks. +- **B4.4 ownerFiles standalone table:** sibling precedent `src/daemon/route-owner-files.ts` proves totality survives outside the row spreads. +- **B4.5 row factories:** 14 local-cli rows (not 12); `` generics preserve literal-name inference (proven by `buildOwnerFiles`). +- **B4.1 capability bolt-on:** genuine late addition from #1683; replace throw guard with completeness test. +- **B6 adb transport injection:** feasible; only ~4 direct `runCmd('adb')` sites remain. +- **B9 http-server split:** confirmed; if anything v1 undercounted the jobs. +- **B12 partial:** `containsPoint` ×2 and the two byte-identical `rectArea` copies (`screenshot-diff-overlay-matches.ts`, `screenshot-overlay-rects.ts`) are safe to merge into kernel. Leave the two defensive-null variants (`rect?.width ?? 0`) and `find-match-resolution.ts`'s `POSITIVE_INFINITY` missing-rect sentinel alone — different semantics, not dupes. +- **AGENTS.md stale `apps.ts` claim:** apps.ts is a 15-line barrel; update the doc. + +--- + +## Dropped or heavily corrected — do not implement as written + +| Item | Verdict | +| --- | --- | +| **A8 chunk enlargement** | ✗ The 8-char chunking IS the fix for #531's on-device truncation. Right lever: promote the existing IME-helper batch broadcast (whole string in one `am broadcast`), currently gated behind `--test-ime`. | +| **B10 flatten-to-score** | ✗ Stages 2/4 are emptiness-guarded fallbacks that ARE the fix for the #1318 empty-refs bug. Keep only: #1255 red-first test + `truncated` emission + single `isForeignOverlayDismissTarget` evaluation. | +| **B7 replay return-values refactor** | ✗ The three-method shape is the published `AdReplayStepRuntime` contract in `packages/ad-replay`, deliberately settled by the #1555 review which removed response-bag returns. Cross-package API renegotiation, not cleanup. | +| **B3 deferred-outcome rewrite** | ✗ The "unused generics" are a documented dependency-inversion device avoiding the R9 type cycle; 67 incident-derived tests (#1542/#1569/#1600/#1601) guard the cluster. High risk, low value. | +| **B2 teardown directive** | Corrected to near-nothing: one 9-line sniffing site; `repairSessionHeld`/`sessionActive` are typed, ADR-0016-documented fields **already released in v0.20.10** (v1's "unreleased API" caveat was backwards). Fix = import existing `ReplayDivergenceResume` type client-side. | +| **A7 status-bar normalization** | Downgraded: opt-in for plain `screenshot` (default-on only for `diff screenshot`); memo already drops steady-state to 2 spawns; session-scoped overrides = frozen fake status bar between commands (product call). | +| **A6 settle baseline-as-sample** | Mechanically coherent but iOS/XCTest-only and repurposes a protective baseline; bench-gated or drop. | +| **B12 paced-scroll merge** | The two steppers encode different pacing philosophies (pixel-derived vs duration-derived) — deliberate, not copy-paste. | +| **B1 RequestContext collapse** | Reframed: overlaps ADR-0019's actively staged request-bound-runtime migration (same seam, commits landing this month). Contribute to that governed arc; don't rival it. Note: wrapper claim was 3-of-4 — the replay wrapper contains real fallback logic. | +| **B4.2 settle four-declaration map** | Stale — grammar/MCP settle surfaces already derive from the descriptor (test-pinned). Accurate map lives in #1652 (6 of 10 surfaces don't derive + 2 live bugs). Redirect work there. | +| **B4.3 config-flag tri-state** | Fine ONLY as required no-default field — the `cli-config-trust` suite guards reporter-RCE/token-smuggling paths. Fail closed. | +| **B5 CLI lazy startup** | `--version` framing refuted (`runVersionFastPath` imports nothing heavy) but the problem is 3x worse than v1 claimed: eager closure ≈ 383 modules / ~62k lines (not 121/19.5k). Sound refactor, touches ~120 facet files; must keep eager grammar aggregation for `--help` + config validation. Staged only. | +| **B8 Chrome fleet** | "Kills developer Chrome" refuted — matcher only reaches isolated Chrome-for-Testing installs. Real missed hazard: **two concurrent daemons share the global state dir and can reap each other's >5-min-idle browsers.** PID-file + doctor move still right; scope the fix to cross-daemon ownership. | + +### Bonus live bug found during verification +**Every web snapshot node has `enabled`/`focused` silently undefined**: JSON refs carry only role+name (verified against pinned binary) and nothing extracts the `[disabled]` bracket annotation from the text tree. File an issue; small fix in `agent-browser-snapshot.ts`. + +--- + +## C. Issue/ADR cross-check (corrected) + +| Item | Verified status | +| --- | --- | +| #1690 find index-once | Not implemented; plan drifted past #1876's split — re-verify excerpts first. Execute (V4). | +| #1652 settle surfaces | The accurate settle map (6 of 10 underived + 2 bugs). Absorbs v1's B4.2. | +| #1832 Android conformance | Sequence V1/V2 with it. | +| #1545 | Root cause fixed by PR #1771; open remainder is orphan-daemon defect only. | +| #1370 breaking cleanups | Still pending; fold into next-major. Confirmed. | +| ADR dangling item | It is **ADR-0016 consequences item 6** (not 0015); blocks #1336 whose `ready-for-agent` label covers only the spike. Track or descope. | +| #1626 / #1635 / #1797 | Already resolved in-session: #1626 kept open w/ CaptureHint-seam comment; #1635 closed as superseded (draft PR #1930 executes sequencing); #1797 migration underway (#1845→#1931 landed). | + +--- + +## D. Corrected action order + +1. **DONE (tier 1):** V3 → #1938 · A4 memo → #1939 (merged) · web enabled/focused → #1940 (merged) + scope follow-up #1951 · AGENTS.md apps.ts fix → #1954 · B12 safe dedups → #1953. Remaining A11 items (diagnostics, Promise.allSettled uninstalls, booted-memo guard, HarmonyOS viewport cache) not yet filed. +2. **Android campaign:** V1 (wire existing `helperSessionScope`) + V2 (dumpsys dedup) + A8-as-IME-batch-promotion. +3. **V4 = execute #1690** with citation fixes. +4. **V5** source-mode-scoped fingerprint; **B2** shrinks to a type import. +5. **Registry:** B4.1/B4.4/B4.5 as written; settle → #1652; B4.3 fail-closed only; **B5** staged. +6. **Deliberate tier:** B6, B9 stand; B1 folds into ADR-0019; B3/B7/B10-rewrite dropped absent explicit appetite; B8 scoped to cross-daemon browser ownership. diff --git a/REPO-QUALITY-BACKLOG.md b/REPO-QUALITY-BACKLOG.md new file mode 100644 index 0000000000..bb739ae37d --- /dev/null +++ b/REPO-QUALITY-BACKLOG.md @@ -0,0 +1,23 @@ +# Repo-quality backlog (flagged during audit/execution, not scheduled) + +1. **Reviewer bot deletes PR head branches mid-review** — #1940's remote branch was deleted while + review was in flight, auto-closing the PR; the follow-up fix had to land as a new PR (#1951). + Consider a branch-protection rule preventing head-branch deletion on open PRs. +2. **Merged-with-known-P1 window** — #1940 merged 5 minutes after review, before the scoped fix + landed; the trailing-value bug lived in main until #1951. If review comments are blocking, + consider a "changes requested" gate or a fast-follow label discipline. +3. **AGENTS.md prose owned by gates** — the over-budget file list rots (apps.ts went stale). A + structural lint/gate should own the 300/500/1000 thresholds and emit the list; AGENTS.md links + to it instead of copying it (docs-ownership rule). +4. **Stale provenance comments in `src/core/command-descriptor/registry.ts`** (~130-133, 214-220) + narrate deleted tables ("copied VERBATIM from daemon-command-registry") and migration phases; + replace with one-line ADR pointers. +5. **`test:unit` wall time ~6 min** — fine for now; if tier-5 B5 lands, re-measure startup closure + and consider sharding the unit bundle. +6. **Agent-tooling hazard (external)**: file-write tools can silently overwrite existing files + without read-first enforcement — caused the #1939 P0 (deleted test suite caught only by review). + Not repo-fixable, but worth a repo-level guard: a CI check that test-file count per source module + never decreases (the ratchet pins sizes but not existence). +7. **`createTtlMemo` lazy-expiry footgun** — default accumulates stale keys in long-lived daemons; + every new memo must remember `scheduleExpiry: true`. Consider making scheduled expiry the default + or linting call sites. diff --git a/WORKPLAN.md b/WORKPLAN.md new file mode 100644 index 0000000000..dbccfbb2b9 --- /dev/null +++ b/WORKPLAN.md @@ -0,0 +1,65 @@ +# Tier Work Plan — chief-of-staff tracker + +Owner: improve-worktree session. Source of truth for scope: `IMPROVEMENT-AUDIT.md` (verified v2). +Rule: max 2 workers in parallel; each worker gets its own git worktree + brief; workers commit but +never push — chief reviews, runs `claude -p` adversarial review, then pushes + opens PR. + +## Worker waves + +| Wave | Worker | Scope (audit ref) | Worktree | Status | +| --- | --- | --- | --- | --- | +| A | A1 | V1 Android helper-session wiring (+ force-stop exit gate) | /Users/thymikee/Developer/agent-device/.worktrees/opencode/worker-android | BLOCKED: claude OAuth expired — needs interactive re-login; worktrees ready at .worktrees/opencode/ | +| A | A2 | V4 find index-once (#1690) | /Users/thymikee/Developer/agent-device/.worktrees/opencode/worker-find | BLOCKED: claude OAuth expired — needs interactive re-login; worktrees ready at .worktrees/opencode/ | +| B | B1 | V2 dumpsys dedup + A8 IME batch promotion | /Users/thymikee/Developer/agent-device/.worktrees/opencode/worker-android2 | pending | +| B | B2 | V5 source-mode fingerprint + B2 ReplayDivergenceResume type import | .worktrees/opencode/worker-daemon-client | **PR #2004 MERGED** (absent-paths cache soundness, lazy closure, pin 365) | +| C | C1 | A11 leftovers batch (diagnostics dedup, Promise.allSettled uninstalls, openIosDevice booted-memo guard, HarmonyOS viewport cache) | ../agent-device-worker-a11 | pending | +| C | C2 | B6 adb transport injection (~4 call sites) | ../agent-device-worker-adb | pending | +| D | D1 | B9 http-server split (route table + install-source schema) | ../agent-device-worker-http | pending | +| D | D2 | B4 registry cells (capability bolt-on, ownerFiles table, row factories) | ../agent-device-worker-registry | pending | +| E | E1 | B8 scoped cross-daemon browser ownership (PID file + doctor) | ../agent-device-worker-web-lifecycle | pending | +| E | E2 | #1652 settle surfaces (6 underived + 2 bugs named in issue) | ../agent-device-worker-settle | pending | +| — | — | selector/replay split (#1978) → **PR #1988 MERGED**; tier-1 sweep: #1938/#1939/#1940/#1951/#1953/#1954/#1971/#1974 all MERGED; wave B: #2003/#2004 both MERGED. 12/12 PRs merged, 0 open. | — | — | +| F | F1 | B5 staged CLI lazy startup (grammar data vs executables) | ../agent-device-worker-cli-lazy | pending — largest; needs its own wave | + +Not scheduled (dropped by verification): B3, B7, B10-rewrite. B1 RequestContext = contribute to +ADR-0019 arc, not a standalone worker (needs maintainer coordination). V6 web upstream ask = file +issue only, chief does it directly. + +## Gate for every worker + +- Red-first for behavior changes (observed red vs pre-fix code). +- Targeted vitest + `pnpm check:affected --run` green before commit. +- `pnpm format`; conventional commit; tests mirror source topology; no compat shapes. +- Device-facing changes: live evidence per `docs/agents/device-verification.md` or stated residual risk. + +## Repo-quality backlog (flagged, save for later) + +See `REPO-QUALITY-BACKLOG.md`. + + +## Babysitting rota (standing) + +- Chief polls all open PRs each cycle; CI fixers dispatched from `BABYSIT-BRIEF.md`. +- #1953, #1954, #1938: MERGED. +- #1971: smoke failure was a flake (green on rerun); 0 failing checks; awaiting review. +- #1974: ratchet banked via extraction shrink (pin 1658→1495); pushed `9e047ca08`; 0 failing checks. +- Rule: babysitters take freed worker slots; implementation waves keep priority when both compete. + + +## Merge ledger (all chief-dispatched PRs) + +| # | Title | Tier | +| --- | --- | --- | +| #1938 | perf(ios): suppression child index once per pass | 1 | +| #1939 | perf(ios): memoize url-scheme probes per bundle mtime | 1 | +| #1940 + #1951 | fix(web): enabled=false from [disabled] annotation (+ scope follow-up) | 1 | +| #1953 | refactor: kernel rect helper dedups | 1 | +| #1954 | docs: stale apps.ts warning removed | 1 | +| #1971 | perf(find): index topology once per ranking pass (#1690) | 3 | +| #1988 | perf(selectors): replay target in one matching pass (#1978) | 3 | +| #1974 | perf(android): warm helper session across fill/scroll | 2 | +| #2003 | perf(android): one dumpsys read; IME batch text entry | 2 | +| #2004 | perf(daemon): stat-revalidated source code-signature cache | 4 | +| #2012* | web daemon-shutdown session close (maintainer-authored) | — | + +Next: wave C — A11 leftovers batch + B6 adb transport injection.