← Commit history

Dev skill: capture the teardown/sweep doctrine (the freeze bug), resolver purity (the session-map purge), multi-maintainer repo safety (last-push-wins + the three gates), and the release-vs-pkg trap. All four are generalizable engineering lessons, not changelog.

John Lauer ·3d92eb5928 ·3mo ago ·parent 7a20206
1 file changed +136
dev-skills/pup-bridge-dev/SKILL.md+136
@@ -907,3 +907,139 @@ user base (e.g. a wave of policy-bound-Chrome breakage) — the identity-dialog it on any binary) and the freeze was our own reap bug. If a specific box has a policy-bound or broken Chrome, that is a per-machine `browser_use {browser:"cft"}` pin, not a fleet reversal. Keep the bundled puppeteer's CfT build current regardless, so the last-resort binary isn't ancient.++## ⛔ TEARDOWN + SWEEP DOCTRINE (2026-07-24 — this froze a user's laptop; do NOT re-derive)++A `browser_close` left orphaned Chromium renderers spinning at ~a full core each. On John's 16-core+laptop overall CPU hit 88-98%, `dwm` (the cursor compositor) was starved to 38-63%, **the mouse froze+and he had to reboot.** A second user independently reported pup at 90% CPU. James's issue #14 (three+sessions running two days, battery drained) was the same root cause. Four rules came out of it, and+they generalize to ANY cleanup code in this bridge.++**1. NEVER fire-and-forget a cleanup whose success you report.** The old reap was+`execFile(...)` with no `await`, so `browser_close` returned `ok` *before* the sweep finished or+failed. A close that left processes behind still said `ok`. If a caller can't distinguish "cleaned up"+from "gave up", the verb is lying. **Await the cleanup, then report a COUNT** (`chromeProcsRemaining`,+which must be 0) so the caller can verify rather than trust.++**2. A cleanup that runs under load must not enumerate the whole world.** The old sweep pulled the+FULL `Win32_Process` table and filtered in PowerShell afterward, with an 8s timeout. At 88-98% CPU —+*the exact condition the runaway renderers create* — that enumeration doesn't finish in 8s. It timed+out and silently gave up, **precisely when it was needed most.** Push the filter server-side+(`Get-CimInstance -Filter "Name='chrome.exe' OR Name='msedge.exe'"`) so only candidate rows are+materialized. A diagnostic that fails under the conditions it diagnoses is worse than none.++**3. NEVER sweep from an EMPTY inventory.** `reapOrphanPupChromes` decides "orphan" as *"no live+session owns this profile"*. An empty session map therefore makes EVERY window an orphan — the sweep+would kill the user's live windows. The map is legitimately empty-but-pending at boot before+`recoverSessions()` finishes (and, pre-#16-fix, after a resolver purged it). It now REFUSES to run when+the session map is empty while session files still exist on disk. The AUMID prune already encodes the+same doctrine ("with nothing to distinguish stale from pending, doing nothing is the correct answer").+**Generalize: a destructive sweep must treat an empty inventory as "I don't know", never as "kill all".**++**4. Scope every destructive sweep by OWNERSHIP, and prove the scoping.** The reap only matches+processes whose command line contains pup's own profile dir, so it can never touch the user's real+browser. Verified live: closing two demo windows reaped 17 processes to 0 **while another AI thread's+window (7 procs) was untouched**. When you ship a sweep, test the sparing, not just the killing.++**Verify with GROUND TRUTH, not your own reporting.** Use AD's `process_list` (`{nameFilter:"chrome"}`,+keys: `pid`/`commandLine`/`parentPid`) to count real OS processes before and after. pup asserting+`chromeProcsRemaining: 0` and AD independently showing 0 are two different claims; the second is proof.++## Resolvers must be PURE — the session-map purge (issue #16, fixed v1.9.92)++Symptom that made no sense: `browser_list_windows` reported **0 windows** while those windows were open+on screen and fully drivable by `sessionId`. It survived `bridge_kill` + respawn, so it looked like+persistent poison. It was neither a crash nor a wedge — it was a **read accessor deleting its own data.**++```js+// getActiveSession() — a RESOLVER that silently purged the map+for (const [id, s] of sessions) {+  if (isSessionAlive(s)) { activeSessionId = id; return s; }+  sessions.delete(id);        // ← no _lostBrowser guard, unlike resolveSession()+}+```++Compounded by a liveness check that consulted only our bookkeeping:++```js+function isSessionAlive(s) {            // OLD+  const be = browsers.get(s.profileName);+  return be && be.browser.isConnected();+}+```++`handleBrowserDisconnect()` does `browsers.delete(profileName)` on ANY CDP blip (sleep/wake, renderer+crash, puppeteer hiccup) while deliberately KEEPING the sessions (`_lostBrowser = true`) and logging+"run browser_rescan to recover". So one blip made every session on that profile read as dead, and the+next verb that omitted a `sessionId` walked the map and **deleted the very entries the disconnect+handler had just preserved.** `bridge_kill` didn't help because the respawn recovered from disk and+then the first resolver call wiped it again.++**Three durable rules:**++- **A resolver must not mutate the collection it reads.** Removal belongs to the close and rescan+  paths. Mark state (`_lostBrowser = true`) instead of deleting; let an explicit reaper own deletion.+- **Two functions doing the same job must agree.** `resolveSession()` had the `_lostBrowser` guard and+  a comment explaining it; `getActiveSession()` didn't. When you find near-duplicate logic, diff the+  guards — a divergence between siblings is where this class of bug lives.+- **Ask the OBJECT, not your bookkeeping.** `isSessionAlive` now falls back to+  `page.browser().isConnected()`. A session that can still be driven must never report as not-alive;+  a false "dead" is what fed the purge.++**Reproduce this class cheaply before shipping a fix:** replicate the data shapes in a standalone node+snippet and run old-vs-new side by side. Mine printed `OLD -> sessions remaining: 0` / `NEW -> 2`,+which proved the mechanism in seconds without touching the live box. Then confirm on the real machine:+kill a window's actual Chrome via `process_kill`, call a NO-sessionId verb (the trigger), and assert+`browser_list_windows` still lists everything.++## Multi-maintainer repo safety — last-push-wins WILL destroy work (issue #19)++Two maintainers each pushing a full `src/` from their own clone, with no freshness check, means+**whoever pushes last silently wins.** Kyle pushed a stale clone over my newer source; I compounded it+by pushing 6-file SUBSETS and by cutting a release from a divergent line. Net state: the repo carried+`BRIDGE_VERSION 1.9.90` sitting on a **1.9.85 tree** — worse than either mistake alone, because it+*looks* correct. 364-line diff, 129 lines of his work missing from mine and all of 1.9.86-1.9.90+missing from his.++**Nothing user-facing broke** — installs follow the RELEASE (manifest + sha256), not the repo. But the+repo stopped being a safe base to cut from, which is a silent trap for the next release.++Three gates now in `/tmp/ship.sh`, and every one of them exists because something got through:++- **GATE 0 — repo freshness, FAIL CLOSED.** Before pushing, fetch the repo's current `src/server.js`+  and count substantive lines present there but missing locally. Over threshold → **abort** with merge+  instructions. Override only via explicit `FORCE_PUSH=1` after a deliberate merge.+- **Full-tree push.** Push every file under `src/`, never a subset. A subset push leaves the repo a mix+  of fresh and stale files while `BRIDGE_VERSION` reads current — that is how `recorder-icon.ico` and+  the mac icons vanished in the 1.9.86 line.+- **GATE 0b — stale metadata.** Versioned files OUTSIDE `src/` (e.g. `page.json`) can also roll+  backwards: my local read **1.8.38** against a published **1.9.74**. Compare and refuse.++**TEST YOUR GATE BY BREAKING SOMETHING.** I stripped 59 lines from my local tree and attempted a real+release: the gate caught 52 missing lines, refused, and nothing shipped. An untested safety gate is a+belief, not a control.++**Merge doctrine when trees diverge:** take the RELEASED tree as the base (it is what users run), then+re-apply the other party's additive hunks onto it. Classify every hunk first —+*their-addition* (take) vs *revert-of-my-work* (skip) vs *mixed* (hand-merge). Say out loud in the+issue which of their changes you did NOT take and why; silently overriding a colleague is how the next+divergence starts.++## The RELEASE and the PKG are different artifacts — shipping one does NOT ship the other++Caught by Kyle 2026-07-24: pup's published **skills package was 17 releases behind** the bridge. Every+version after 1.9.74 returns **404** at `/api/v1/packages/adom/<slug>/<ver>/tarball` — those are+release tags, not packages. So `pkg install` silently resolved to the **1.9.74** tarball: stale docs,+and a published `install.sh` that wrote the start-here skill to+`~/.claude/skills/adom-desktop-puppeteer-bridge/` while its own `SKILL.md` declares `name: pup` — a+slug that contradicts its frontmatter, its trigger words, and every cross-reference in the sub-skills.++**The trap:** `release create` + `release upload` ships the RUNTIME. It does nothing for the SKILLS+tarball. A bridge can be 17 releases current on the runtime and 17 releases stale on the docs its+consumers actually read, with no error anywhere.++**Rules:** when SKILL.md / install.sh / uninstall.sh / package.json change, `pkg publish` in the SAME+session as the release. Verify by fetching the tarball at the version you just shipped (a 404 means you+published a release, not a package) and by diffing the packaged files against the repo copies — Kyle+found the slug bug only because he compared all nine files, not just the docs he came for.