Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions IMPROVEMENT-AUDIT.md
Original file line number Diff line number Diff line change
@@ -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); `<const T>` 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.
23 changes: 23 additions & 0 deletions REPO-QUALITY-BACKLOG.md
Original file line number Diff line number Diff line change
@@ -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.
65 changes: 65 additions & 0 deletions WORKPLAN.md
Original file line number Diff line number Diff line change
@@ -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.
Loading