Skip to content

fix(chatgpt-review): unify timeout budget, native insertion, clipboard retry, heartbeat, stall recovery - #656

Merged
BorisTyshkevich merged 1 commit into
mainfrom
fix/chatgpt-review-robustness-and-performance
Aug 9, 2026
Merged

fix(chatgpt-review): unify timeout budget, native insertion, clipboard retry, heartbeat, stall recovery#656
BorisTyshkevich merged 1 commit into
mainfrom
fix/chatgpt-review-robustness-and-performance

Conversation

@BorisTyshkevich

Copy link
Copy Markdown
Collaborator

Summary

Investigated why chatgpt-review's Playwright interactions sometimes failed/stalled during issue #630's long /ship run, by reading the actual code, mining 55 real SessionStore records and dozens of real raw CLI output files from $TMPDIR across the whole run, and running live experiments against real ChatGPT tabs. Found and fixed 5 distinct, evidence-backed root causes:

  1. Unified timeout budgettimeoutMs previously bounded only waitForCompletion; setup phases (assertReady/upload/fillAndSend/waitForPermanentConversationUrl) were each independently uncounted against it, so real worst-case wall time was timeoutMs + ~300s. Confirmed live: 6 real invocations across Epic: extract the Fetch-native ClickHouse client into a reusable package #630 were killed with zero output ever flushed. Now one deadline covers the whole call.
  2. Fast native prompt insertionfillAndSend now inserts via execCommand('insertText') in-page first (measured live: 80KB in ~50ms regardless of size), falling back to Playwright's own .fill() only if that fails. Confirmed live that .fill() can still genuinely time out at 120s×2 under real load even after the existing headroom fix (Epic: extract the Fetch-native ClickHouse client into a reusable package #630: 246.9s elapsed, both attempts exhausted).
  3. Clipboard-copy retry for plan-author mode — a single failed clipboard read used to fall back to innerText, which can never contain the literal # parsePlanAuthorResponse's heading check requires. Confirmed live: this exact invalid_response recurred 6 times across real Epic: extract the Fetch-native ClickHouse client into a reusable package #630/ADR-0005: adopt @clickhouse/client-web behind the SQL Browser transport adapter #585 invocations. Now retries up to 2 more times before falling back, for that mode specifically.
  4. Heartbeat persistence — a periodic progress snapshot is now written alongside the session record, so a process killed externally with zero stdout still leaves something recoverable.
  5. Automatic stall recovery — one automatic stop+nudge attempt when generation shows zero text growth for an extended period while still "generating" (a live tool call stuck mid-turn) — previously only recoverable by a human manually intervening, now documented as the last-resort manual fallback in skills/ship/references/review-loops.md.

Test plan

  • node --test tests/*.test.mjs — 57/57 pass (11 new tests, one per fix plus the unified-deadline composition specifically)
  • node scripts/chatgpt-review.mjs doctor against the real connected Chrome — passes
  • One full real end-to-end review() call against a fresh conversation: native insertion used (no .fill() fallback needed), completed in 14.7s, heartbeat fired with the expected shape, response captured correctly (disposable test conversation closed afterward, no real conversations touched)
  • Investigated and ruled out a scare during live testing: repeated failures against pre-existing tabs turned out to be caused by my own test scripts leaving stale/dangling CDP connections behind across many rapid successive experiments — not a defect in the fix. Confirmed by cleanly connecting/disconnecting (proper try/finally) against a fresh page, which worked immediately.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz

…ry clipboard, add heartbeat and stall recovery

Diagnosed from real session/state logs across issue #630's whole 8-phase run (55
real session records, dozens of real raw CLI output files) plus live experiments
against production ChatGPT tabs:

1. review()'s timeoutMs previously bounded only waitForCompletion -- assertReady
   (~15s) + upload (~30s) + fillAndSend (up to 240s via its own retry) +
   waitForPermanentConversationUrl (~15s) were ALL uncounted against it, so real
   worst-case wall time was timeoutMs + ~300s, not timeoutMs. Confirmed live: 6
   real invocations across issue #630 were killed with zero output ever flushed
   ("Waiting for ChatGPT response..." and nothing else). Now one deadline covers
   the whole review() call; each setup phase still gets its own short ceiling,
   capped by Math.min against the overall deadline.
2. fillAndSend now inserts via document.execCommand('insertText') in-page first
   (measured live: 80KB lands in ~50ms regardless of size), falling back to
   Playwright's own actionability-checked .fill() only if that fails --
   confirmed live that .fill() can still genuinely time out at 120s x 2 under
   real load even after the existing headroom fix (issue #630, 246.9s elapsed,
   both attempts exhausted).
3. copyPreferredResponseText retries a failed clipboard copy up to 2 more times
   for plan-author mode specifically, before falling back to innerText (which
   can never contain the literal '#' parsePlanAuthorResponse's heading check
   requires) -- confirmed live: "not complete Markdown with a heading" recurred
   6 times across real issue #630/#585 invocations, all traceable to this
   single-attempt-no-retry gap.
4. SessionStore.writeHeartbeat + review()'s onHeartbeat callback persist a
   periodic progress snapshot (throttled to heartbeatIntervalMs) alongside the
   session record, so a process killed externally with zero stdout still
   leaves something recoverable.
5. waitForCompletion attempts one automatic stop+nudge recovery when generation
   shows zero text growth for noProgressStallMs while still "generating" -- a
   live tool call stuck mid-turn, previously only recoverable by a human
   manually clicking Stop and nudging the same conversation (now documented as
   a purely manual procedure in skills/ship/references/review-loops.md).

Verified: 57/57 unit tests (11 new, covering each fix + the unified-deadline
composition specifically), doctor mode against the real connected Chrome, and
one full real end-to-end review() call against a fresh conversation (native
insertion used, no Playwright .fill() fallback needed, completed in 14.7s,
heartbeat fired with the expected shape, response captured correctly).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz
@BorisTyshkevich
BorisTyshkevich merged commit 9bf222f into main Aug 9, 2026
8 checks passed
@BorisTyshkevich
BorisTyshkevich deleted the fix/chatgpt-review-robustness-and-performance branch August 9, 2026 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant