fix(session): stop phantom repeated keystroke replay on reconnect/flap - #288
Draft
tstapler wants to merge 1441 commits into
Draft
fix(session): stop phantom repeated keystroke replay on reconnect/flap#288tstapler wants to merge 1441 commits into
tstapler wants to merge 1441 commits into
Conversation
…ponent Backend (Epic 1 completion): - server/services/ai_interfaces.go: RulePromptBuilder, AIClient, RulePromptContext - server/services/rule_prompt_builder.go: DefaultRulePromptBuilder with secret redaction - server/services/anthropic_client.go: AnthropicAIClient (HTTP fallback) - server/services/rules_service.go: GenerateSuggestedRule handler, parseSuggestions, attachConflictInfo, buildPromptContext - server/services/approval_handler.go: redact secrets before analytics recording - config/config.go: AnthropicAPIKey field + ANTHROPIC_API_KEY env override Frontend (Epic 2): - web-app/src/lib/hooks/useGenerateRule.ts: wraps GenerateSuggestedRule RPC, manages suggestions[], loading, error, cancel (AbortController), 60s timeout - web-app/src/components/sessions/SuggestedRuleCard.tsx: shared card with confidence badge, conflict/shadow warnings, editable fields, Accept & Discard - web-app/src/components/sessions/SuggestedRuleCard.css.ts: vanilla-extract styles Tests: 13 Go unit + 21 frontend (9 hook + 12 card), all passing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* feat(analytics): program detail panel with subcommand drill-down Add DB-backed time-windowed analytics queries and an inline program detail panel so operators can see exactly which sub-operations are causing escalations before writing a rule. Backend (Go): - Add compound index on (command_program, created_at) to ent schema - Replace full-table-scan ListAnalytics with time-windowed ListAnalyticsSince (WHERE created_at >= ?) — AC-1, AC-2 - Add GetSubcommandBreakdown aggregation query using ent GroupBy — AC-4 - Add ListRecentCommandsByProgram returning last N command previews — AC-5 - Add GetSubcommandTrend returning per-day counts — AC-6 - Add GetProgramAnalytics ConnectRPC method returning SubcommandBreakdown, ExampleCommands, RuleCoverage, DailyTrend — AC-7 Frontend (React/TypeScript): - New ProgramDetailPanel component with subcommand frequency table (count, %, decision breakdown), example commands, rule coverage summary, trend sparklines, and "Add rule →" links — AC-8 through AC-13 - New useProgramAnalytics hook with AbortController cleanup - ApprovalAnalyticsPanel: clicking program row opens inline detail panel - ApprovalRulesPanel: fix panel crush in flex container (flexShrink: 0), use window.location.search in useEffect for URL param pre-fill (avoids useSearchParams/Suspense issues in Next.js static export) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: address copilot review comments on analytics drill-down - Fix 1: exclude NULL command_subcategory rows in GetSubcommandBreakdown to avoid sql.ScanSlice scan errors on nullable GROUP BY columns - Fix 2: replace strings.Fields tokenizer in coveredSubcommands() with regexp.Compile + synthetic "<program> <subcommand>" matching so regex-style patterns (e.g. \bgit\b.*\bpush\b) work correctly - Fix 3: add TestGetProgramAnalytics_ReturnsExpectedFields unit test covering window_days=7 and non-nil response fields - Fix 4: add escapeRegex() helper in ApprovalRulesPanel and use it when prefilling commandPattern to avoid metacharacter injection; switch word boundaries from \b to (?:^|\s)/(?:\s|$) for hyphenated program names - Fix 5: add e.stopPropagation() on Suggest Rule button and "add manually" link so clicking them does not toggle the parent <tr> drill-down row - Fix 6: add tabIndex, role=button, aria-expanded, aria-label, and onKeyDown (Enter/Space) to the clickable <tr> for keyboard accessibility - Fix 7: call setData(null) before setIsLoading(false) in error path of useProgramAnalytics to clear stale data on refresh failure - Fix 8: render per-program daily trend sparkline in ProgramDetailPanel; note that trend data is per-program not per-subcommand (backend limit) - Fix 9: thread caller context through LoadProgramWindow, GetSubcommandBreakdown, and ListRecentCommands instead of context.Background() * feat(rules): add criteria_programs/subcommands to custom rules, deprecate regex prefill Extend custom/runtime approval rules with structured CommandCriteria matching (Programs + Subcommands) as an alternative to regex command_pattern. - ent schema: add optional JSON fields criteria_programs and criteria_subcommands - proto ApprovalRuleProto: fields 15/16 for criteria_programs/subcommands - session/repository.go + ent_repository.go: thread new fields through domain model and DB upsert - rules_store.go: RuleSpec gains CriteriaPrograms/CriteriaSubcommands; specsToRules() populates rule.Criteria when programs are set; reload/upsert pass fields through - rules_service.go: specToProto/ruleToSpec/UpsertApprovalRule thread the new fields; coveredSubcommands() checks criteria-based rules directly without synthetic regex matching - ApprovalRulesPanel.tsx: RuleFormState gains criteriaPrograms/criteriaSubcommands; form inputs added with hint text; URL param pre-fill now populates criteria fields instead of commandPattern regex; match info column shows criteria chips - useApprovalRules.ts: passes criteriaPrograms/criteriaSubcommands to proto request Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fuse.js was running synchronously on every keystroke in the omnibar, causing a 931ms long task during typing. Three changes fix this: - Add 150ms debouncedInput in Omnibar — sessionSearchQuery, displayedSessionResults, and displayedRepoEntries all read from it instead of the raw input value, so Fuse only fires after typing pauses - Add selectActiveSessionsSortedByUpdatedAt (createSelector) to sessionsSlice, removing the inline .filter().sort() that ran on every omnibar render - Add browser profiling phase (Phase 5) to perf:make-it-faster command Update Omnibar.discovery test to assert debounced behaviour.
- Auto-trigger triage on item creation (R1); skip via skipTriage flag - Vagueness detection: prompt users to refine items with <80 char description and no AC before triage (R2) - TriageReviewPanel: diff view of current vs suggested AC, task checklist with estimates/categories, apply/skip/undo actions (R4, R5) - Apply triage suggestions: updates AC list and transitions item to "ready" in one action (R5) - INPUT_REQUIRED notification with deep-link when triage completes (R6) - Trigger Triage button for manual re-triage and error recovery - Fix backlog page infinite reload loop caused by unstable useBacklogService return object; add regression tests (T-STAB-001, T-STAB-002) - Double-trigger guard prevents duplicate triage sessions - Triage prompt instructs AI to return structured tasks[] array for implementation plan
Sessions spawned via the MCP create_session tool now always carry the source:mcp tag so they can be filtered distinctly in the session list.
… sessions Adds a complete session supervision system so automated sessions (backlog triage/work/review and MCP-created) run fully unattended: - Universal driver wiring: SessionDriver now runs for all automated session creation paths. MCP create_session gains the driver call; backlog sessions already flowed through CreateDirectorySession which was wired earlier. - Startup dialog handling: driver answers the Claude Code trust-folder safety check and directory-access dialogs on every poll tick before the initial prompt is sent. - Inactivity detection: after the initial prompt is sent, if the session stays in Ready state with no output change for 10 minutes it is considered stuck and restarted (driverTotalTimeout raised to 25 min as a safety net). - Exit detection: unexpected Stopped status after the initial prompt triggers a retry. One-shot sessions (backlog:triage, backlog:review) are skipped. Sessions that ran for > 5 minutes are treated as normal completions. - Auto-retry with JSONL continuation: on first failure the session is restarted with a prompt built from its JSONL conversation history (last 10 messages, last assistant turn truncated to 500 chars). On second failure the session is added to the ReviewQueue (ReasonStale) for human attention. - Idempotency guard: StartSessionDriver uses atomic.Bool.CompareAndSwap so a second call on the same instance is a no-op. - Panic recovery: defer/recover inside runSessionDriverWithPrompt protects both the initial and retry driver goroutines from crashing the server. Also fixes the omnibar path display: long paths now wrap instead of truncating.
…gger git-status bursts PerfFix-1: WorkspaceService.findInstance called LoadInstances() on every GetVCSStatus, GetWorkspaceInfo, GetWorkspace, and ListWorkspaceTargets RPC. LoadInstances() re-hydrates all sessions from disk via FromInstanceData→Instance.Start, spawning PTY processes and running `tmux ls` for each session — holding the global fork lock and causing ~41.7B cycles / 808 mutex events under profiling. Fix: add LiveInstanceFinder interface satisfied by SessionService, wire it in NewSessionService, and replace the single findInstance method with findInstanceFast (tries O(1) live-poller lookup first, falls back to LoadInstances only for new sessions not yet in the poller). SwitchWorkspace also migrated: SaveInstances accepts a single-element slice, so the full LoadInstances call for the list was unnecessary. PerfFix-2: ReviewQueuePoller.AddInstance now calls PrimeDirtyCacheJitter() on each new session. Sessions added in a burst (e.g. startup) previously had synchronized 15s dirty-cache TTLs, causing all git-status subprocesses to expire and launch simultaneously. Jitter spreads them uniformly across the window. PerfFix-3: forbidigo lint rule guards against future per-request LoadInstances calls with path exclusions for the known-good sites.
HistoryLinker.correlateSession had an early-return guard:
if inst.HasClaudeSession() { return }
This prevented UUID updates when a user ran /clear in Claude Code,
which closes the old JSONL and opens a new one with a different UUID.
On cold restore the instance would resume the pre-/clear conversation
(or fail with "No conversation found" → silent restart via
recoverFromStaleResume) instead of starting fresh as the user intended.
Fix: add force bool parameter to correlateSession.
- ScanAll() (triggered by fsnotify on JSONL creation and on startup)
calls correlateSession(inst, true) for ALL instances, including
already-linked ones, so UUID changes are detected and stored promptly.
- scanAllSessions() (5s polling loop) calls correlateSession(inst, false)
to preserve the existing performance optimization of skipping
already-linked sessions on every tick.
- Start() now calls ScanAll() instead of scanAllSessions() for the
initial startup scan, catching UUID changes that occurred while the
service was down.
Backoff is bypassed when force=true; it still applies to unlinked
sessions in polling mode.
…ervice - session/instance_claude.go: add stateMutex.RLock to HasClaudeSession and GetClaudeSession; add stateMutex.Lock to SetClaudeSession (data race fix) - session/history_linker.go: eliminate TOCTOU in ScanAll by holding a single write lock for both backoff clear and snapshot copy - session/history_linker_test.go: rewrite AlreadyLinked test to exercise correlateSession(force=false) skip path; add regression test for force=true UUID update after /clear (was BLOCKER gap) - session/git/worktree.go: export isDirtyCacheTTL → IsDirtyCacheTTL - session/git/worktree_git.go: update references to IsDirtyCacheTTL - session/git_worktree_manager.go: reference git.IsDirtyCacheTTL constant; add PrimeDirtyCacheJitter() to GitManager interface - server/services/workspace_service.go: correct O(1) → O(N) in LiveInstanceFinder godoc; remove blank line breaking GetWorkspace godoc - server/services/workspace_service_test.go: add stubLiveFinder and tests for fast-path hit and live-finder-nil fallback to storage - server/server.go: document known startup race in HistoryLinker.Start comment
Renames the PTY multiplexer binary and all references from claude-mux to ssq-mux for naming consistency. Includes code, comments, docs, config, installer scripts, and .goreleaser.yaml (also adds ssq-hooks binary entry).
Introduces WorkflowEngine interface to replace the inline validTransitions map in backlog.go, making transition policy explicit and testable. Adds BacklogStatusEvent as an append-only ent entity that records every status transition (from, to, triggered_by, created_at) with an index on (item_id, created_at) for efficient history queries. Also hardens Storage.AddInstance to distinguish constraint errors from other persistence failures on upsert.
Adds WriteToSession(session_id, input, press_enter) to SessionService. Intended for programmatic approval of hook prompts and ad-hoc terminal input from the backlog SessionMonitor panel. Returns immediately after queueing; does not wait for PTY output.
Adds SessionMonitor component to the backlog detail view — shows live terminal output, conversation messages, and quick-action buttons (y/n/1-3) that call WriteToSession. Gated behind backlog:conversation-view feature flag. Adds RepoPathInput — a text input with autocomplete that queries available repo paths via a new useSessionRepoPaths hook. Extends BacklogItemDetail with a status transition timeline and session assignment, and updates BacklogItemForm and BacklogItemBadge accordingly.
… ≤768px) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ream-20260521 # Conflicts: # benchmarks/e2e/latency-baseline.json # benchmarks/frontend/throughput-baseline.json # benchmarks/go/tier1-baseline.txt # gen/proto/go/session/v1/session.pb.go # server/dependencies.go # server/services/session_service.go # web-app/src/gen/session/v1/session_pb.ts
…perf (#98) * fix(verify): resolve sdd:6-verify findings Go: - pruneRepoCache: implement LRU trim — after TTL eviction, sort remaining entries by accessedAtNs and delete coldest until count ≤ repoCacheMaxEntries. Previously the cap was advertised but not enforced (keys slice was dead code). - openRepoEntry: wrap PlainOpenWithOptions error with path context; re-check cache after pruning to eliminate redundant PlainOpen under contention. - findMergeBase: propagate non-ErrObjectNotFound CommitObject errors instead of silently continuing; skip only missing objects from shallow clones. TypeScript: - useSessionNotifications: extract approvalId before object literal, removing two non-null assertions on optional-chain result. - useSessionNotifications: move dedup map prune before lastShown read so the captured value reflects post-prune state. - useSessionNotifications.test: type makeEvent with TestNotificationEvent interface instead of any. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(vcs): move repo/diff caches onto GoGitVCSReader; release mutex before OS calls Move repoCache, repoCacheSize, and diffStatCache from package-level globals to GoGitVCSReader struct fields. GoGitVCSReader{} remains valid — sync.Map and int64 are zero-value safe. This isolates cache state per reader instance, preventing cross-test contamination when multiple GoGitVCSReader values exist in the same process (e.g. concurrent test runs or benchmarks). pruneRepoCache, openRepoEntry, and openWorktree are now methods on *GoGitVCSReader; all callers within the file use g. prefix. HasUncommitted: release the per-repo mutex before the working-tree stat loop and hasUntrackedFiles directory walk. These phases call only os.Lstat and os.ReadDir — no go-git access — so holding the packfile-reader mutex was blocking concurrent VCS operations on the same repo unnecessarily. Index entry fields are captured as a plain []trackedFile slice before the unlock. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(insights): time range filter, session detail drawer, cost projections, perf R1 — Time range filter: preset buttons (Today/7d/30d/90d/All/Custom) + custom date picker persisted via URL search params; from/to wired through to GetInsightsSummary and WatchInsights RPCs. R2 — Per-session detail drawer: slide-over panel (createPortal) showing session metadata, tools breakdown, and skill activations; click any session row to open; Escape/backdrop closes. R3 — Cost projections: useProjectedCost computes projected monthly spend from daily buckets (requires ≥7 days of data); useBudgetThreshold persists a localStorage budget threshold with SSR hydration guard; warning banner + card styling when projected spend exceeds threshold. R4 — Skeleton loaders: InsightsDashboardSkeleton shown immediately on mount before first RPC response; chart dynamic() imports use Skeleton as loading placeholder; Suspense boundary added for useSearchParams. R5 — Virtual scrolling: TableVirtuoso (react-virtuoso) for >50 session rows; scroll position preserved across live updates. R6 — Smooth live updates: surgical per-session state patch on WatchInsights events; exponential backoff reconnect (1s→30s cap) replaces silent drop; chart data transforms wrapped in useMemo to prevent recharts re-renders. R7 — Session table search/filter: Fuse.js path search + model family dropdown + clear button; filter state preserved across live update cycles. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(insights): address review comments — date tz, UTC month bucketing, TableVirtuoso cells, test hydration assertion - InsightsDashboard: serialize date to URL using local timezone (not UTC toISOString) - TimeRangeFilter: parse URL date params as local T00:00:00/T23:59:59 (not UTC midnight) - useProjectedCost: compare bucket dates using UTC methods to match server-side UTC midnight keys - SessionsTable: return cells (not <tr>) from TableVirtuoso itemContent; use components.TableRow for row-level props - useBudgetThreshold test: assert initial isHydrated is false (not just typeof boolean) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: add planning artifacts for insights enhancement * fix(insights): code quality, test coverage, and UX improvements Code quality: - Extract fmtCost/fmtTokens/fmtPct/fmtDate/shortId to insightsFormatters.ts; standardize fmtPct to toFixed(1) across all consumers - Name retry delay magic numbers: INITIAL_RETRY_DELAY_MS / MAX_RETRY_DELAY_MS - Log WatchInsights stream errors to console instead of silently swallowing Correctness: - Filter surgical stream update events against active from/to time range; sessions outside the current filter window no longer appear in live patches Tests (+4): - useProjectedCost: February non-leap (28 days), leap year (29 days), UTC month boundary exclusion - useBudgetThreshold: invalid localStorage value (non-numeric) returns null UX: - ProjectedCostCard: show inline error when budget input is ≤ 0 or NaN - TimeRangeFilter: show range-error message when from > to (custom range) - InsightsDashboard: map raw gRPC error codes to user-friendly messages - SessionDetailDrawer: add aria-describedby + srOnly description for a11y Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(insights): remove last duplicate fmtCost in ProjectedCostCard, import from insightsFormatters Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
The release-please failure is caused by missing PR creation permission (GitHub Actions not permitted to create PRs), not commit body parsing. The parse errors in the logs are debug messages, not failures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…isplayName React hooks must be called unconditionally. DailySpendChart, ModelBreakdownChart, and ModelOverTimeChart all called useMemo after an early return on empty data. Move the memo calls above the early return — they are cheap no-ops on empty arrays and the behaviour is identical. SessionsTable: add displayName to the React.forwardRef TableBody component passed to Virtuoso so the react/display-name lint rule is satisfied. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The react/display-name rule cannot infer a display name for React.forwardRef calls used as values in an object literal inside useMemo. Setting .displayName after the fact is not visible to the lint rule at that location. Using eslint-disable-next-line is the correct suppression for this inert pattern — Virtuoso uses this TableBody ref purely for scroll container sizing, not for React DevTools identification. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e evidence
Research phase (6 parallel agents) plus direct code reading found the
originally-suspected client MessageQueue/reconnect duplication does not
explain repeated single-keystroke delivery (queue items are consumed
exactly once; server input relay sends each message at most once).
session/session_driver.go's startup-dialog auto-answer loop is the
confirmed primary cause: it polls Preview() every 2s and resends
SendKeys("1\n") with no de-duplication/backoff whenever isStartupDialog()
matches, independent of the auto_yes flag. A live-executing test
(session/phase0_repro_test.go) reproduces 3 repeated SendKeys("1\n")
calls when Preview() returns stalled dialog content, matching the
ticket's "over and over" symptom.
The client MessageQueue/epoch-guard gap is real but secondary (per AC3's
explicit text) and will be fixed as additive hardening.
…ial review Plan covers two additive fixes: a content-hash latch on session_driver.go's startup-dialog auto-answer loop (the confirmed root cause), and a connection- generation guard + drop-on-close fix for MessageQueue/useTerminalStream (AC3's explicit secondary hardening requirement), plus an InputDropBadge UX surface and the Go/Jest regression tests required by AC4. Adversarial review caught two real blockers before any code was written: the fix's control-flow could silently starve the driver's inactivity- timeout/ReviewQueue escalation once the dialog latch gave up, and the content-hash approach was vulnerable to incidental formatting jitter (same failure class as the adjacent #164 resize-loop bug). Both resolved and re-verified; architecture and UX reviews landed at CONCERNS only, folded into the plan directly.
Validation phase (3 parallel subagents: test-suite design, pre-mortem, cross-artifact consistency) surfaced one new BLOCKER and one new P1 the Phase 3 review rounds missed: - BLOCKER: no test actually exercised AC4's literal "queued-message-drop- on-close interleaving" scenario (Task 2.1.2 was an isolated close() unit test, not a live-reconnect interleaving test). Added Task 2.2.8. - P1: Preview() returns the entire accumulated PTY buffer, not a tailed "current screen" snapshot, so the content-hash latch's whitespace-only normalization would still misfire in ordinary, non-flapping sessions once any new output changed the whole-buffer hash - a materially larger regression surface than the formatting-jitter case already closed. Task 1.1.2 revised to tail-slice via the existing tailContent/ statusDetectionTailBytes precedent before matching and hashing. Also closed two coverage gaps the validation subagent flagged (a test proving the control-flow fall-through reaches escalation, and a missing Playwright e2e spec required by this repo's feature-registry convention). Readiness gate: PASS (6/6 ACs covered, 0 open blockers/P1s, ADR-001 on disk, no schema changes).
… reconnect input path
Fixes the confirmed root cause of the phantom repeated "1" keystroke bug
(backlog 04089969-0f19-499c-be34-2e8bcfc4f13e): session_driver.go's
startup-dialog auto-answer loop resent SendKeys("1\n") every poll tick
with no de-duplication whenever isStartupDialog() matched, and Preview()
returns the entire accumulated PTY buffer rather than a tailed snapshot,
so the resend could recur in ordinary non-flapping sessions too, not
just during a connection flap.
- session/session_driver.go: content-hash latch (dialogUnanswered ->
dialogAwaitingDismissal -> dialogGaveUp), tail-sliced + whitespace-
normalized before hashing/matching (via the existing tailContent/
statusDetectionTailBytes precedent), bounded retry-on-failure, and a
control-flow fix so a GaveUp/AwaitingDismissal latch falls through to
the existing inactivity-timeout/ReviewQueue escalation instead of
silently wedging the driver loop. Applied to both SendKeys("1\n")
call sites (startup dialog + approval prompt).
- server/services/connectrpc_websocket.go: extracted the WebSocket
input-read loop into runInputReadLoop so it's independently testable
(bounded-exit test proves it stops forwarding input promptly once the
connection closes).
- web-app MessageQueue/useTerminalStream: close() now drops (does not
drain) buffered-but-unsent input; useTerminalStream gained a
connection-generation guard (mirroring the existing usePathCompletions
generation-counter idiom) so an overlapping/rapid reconnect can't leave
two live message loops fighting over the same session. Dropped input
now surfaces via a new InputDropBadge (assertive live-region
announcement + visible badge, coalesced per drop episode).
Confirmed via a live-executing runtime test (session/phase0_repro_test.go,
now superseded by permanent regression coverage in session_driver_test.go)
that the unfixed driver resent the keystroke repeatedly against a stalled
preview buffer. Full adversarial/architecture review + pre-mortem process
in project_plans/phantom-keystroke-replay/ caught and fixed two design
blockers and one P1 gap before this code was written.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q2aGuq28JKW6fT6w2MiKbV
…titute evidence Task 4.1.1's live-browser repro procedure targets the omnibar session- creation path, which never attaches a SessionDriver at all (only CreateDirectorySession/MCP create_session do) - the exact code this fix touches can never run through that path. Also found /tmp is globally pre-trusted on this machine (defeats the trust-dialog trigger) and a log-routing gap in isolated STAPLER_SQUAD_INSTANCE deployments unrelated to this fix. Documented all three for a follow-up live verification pass; recorded the automated re-run of Epic 1's regression suite (same Phase 0 methodology, now bounded instead of unbounded) as substitute evidence.
MUST FIX: InputDropBadge's manual dedup ref broke under React StrictMode
(setup->cleanup->setup replay could clear the dismiss timer without
rearming it, leaving the badge visible forever on first mount in dev).
Collapsed into a single effect with its own cleanup, no ref-based dedup.
CONCERN: window.__e2eTriggerInputDropped now gates on NODE_ENV so it's
tree-shaken out of production builds instead of shipping as an
always-present, unauthenticated script-callable surface.
Also: exhaustive switch (with a panic default) on dialogLatchStatus so a
future 4th status can't silently fall through the unbounded-resend path;
hoisted the duplicated SendKeys("1\n") closure shared by both latch call
sites; removed a redundant useCallback wrapper in TerminalOutput.tsx;
documented the onScrollbackRequest plan deviation.
Reverted one over-eager cleanup: moving all tail-slicing to the call
site (instead of inside answerDialogOnce) broke TestAnswerDialogOnce's
growing-buffer test cases (f)/(g), which call answerDialogOnce directly
with untailed input and rely on its own internal tailContent call.
Restored the internal tail-slice - the "duplication" with the call
site's own tailed value (needed separately for isStartupDialog/
shouldApprovePrompt) is cheap and was never actually redundant once a
unit test exercises the function in isolation.
All Go and Jest suites verified green after this pass (full session +
server/services test trees, plus TerminalOutput's existing suites).
…ager stubs session_driver_test.go's stuckDialogProcessManager fake intentionally returns (nil, nil) for GetPTY/Attach since neither is exercised by the dialog-latch tests. Matches the existing nolint:nilnil convention used in history_detector.go for the same real-fake pattern.
…n sdd:6-verify review Two independent parallel reviews (Go idiom/concurrency, React/TS idiom) each surfaced one real MUST FIX: - GetEffectiveStatus() claimed (via its own comment and callers' comments) to acquire stateMutex.RLock but never did, and the new session_driver_test.go tests wrote inst.Status directly from a second goroutine while the driver loop read it via GetEffectiveStatus — a genuine data race caught by `go test -race`. Fixed by actually taking the RLock in GetEffectiveStatus (matching its documented contract) and routing the tests' cleanup writes through stateMutex.Lock(). - useTerminalStream's disconnect() had no connection-generation guard on its delayed (1000ms) graceful-close timeout or its trailing setIsConnected(false), unlike connect()'s read loop. A disconnect() that armed its timer while still connected, raced by a newer connect() that took over in the meantime, would abort/null the *newer* generation's AbortController and clobber its isConnected state once the stale timer fired — the same class of bug this whole fix exists to close. Fixed by capturing the generation at disconnect() entry and gating both effects on it; new regression test confirmed to fail against the pre-fix code and pass against the fix. Also addressed two Go nitpicks from the same review: replaced the bespoke errSentinel type with errors.New, and removed a local min() helper that redundantly shadowed the Go 1.21+ builtin. go test -race ./session ./server/services and the full web-app Jest suite are green.
…and fixes Documents the two real MUST FIX findings (Go data race, disconnect() reconnect-race gap) surfaced by a fresh two-agent idiom review pass and fixed in f3a876f, per plan.md's existing Post-Implementation Finding convention.
… MCP create_session Completes the literal live-browser-equivalent repro the earlier Post-Implementation Finding could only substitute with re-run unit tests. Created a real session via mcp__stapler-squad__create_session (the path that actually exercises StartSessionDriver), induced the ticket's exact "session not started or paused" flap via pause/resume immediately after creation, and confirmed via server logs the startup dialog was answered exactly once inside the flap window with no resend and no repeated "1" in the final recovered terminal state.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a bug where a single keystroke (observed as
1) got repeatedlyre-delivered to the agent/tmux session while a session's connection was
flapping (connect → "session not started or paused" → reconnect), making
the session unusable.
Two independent, previously-untested code paths contributed:
SessionDriver's startup-dialog auto-answer(
isStartupDialog/shouldApprovePrompt→SendKeys("1\n")) had nobound on how many times it could resend the same answer while the PTY
buffer looked "stuck" (a real flap) or kept growing with unrelated new
output (an ordinary, non-flapping session).
useTerminalStream's input path had no guard againstreplaying/leaking buffered input across a superseded connection
generation during a reconnect.
What Changed
session/session_driver.go: added a hash-basedDialogAnswerLatch(
answerDialogOnce,maxDialogAnswerAttempts=3) that tail-slices the PTYbuffer before matching/hashing so resends are bounded both during a real
stuck-buffer flap and during ordinary active-session output growth.
session/instance_state.go:GetEffectiveStatus()now actuallyacquires
stateMutex.RLock()as its own comment always claimed — fixes areal
-racedata race surfaced by new concurrent tests.server/services/connectrpc_websocket.go: extracted the inputread-goroutine (
runInputReadLoop) with a bounded, prompt exit onconnection close.
web-app/src/lib/hooks/useTerminalStream.ts: added aconnection-generation guard so an overlapping/stale
connect()ordisconnect()can't mutate a newer generation's live connection state;disconnect()'s delayed graceful-close timeout is now generation-gatedtoo (a gap the initial fix missed — see below).
web-app/src/lib/terminal/MessageQueue.ts+ newInputDropBadge.tsx/useDropEpisodeCoalescer.ts: input queuedduring a disconnect is dropped (not replayed) when superseded, and the
user is visibly (badge) and audibly (assertive
aria-liveannouncement)signaled when this happens.
Review History
This branch went through two backlog review cycles:
(
TestAnswerDialogOnce/f_growing_buffer_within_tail_window) — fixed.(Go concurrency, React/TS) caught two further real bugs neither prior
review round found: the
GetEffectiveStatus()data race above, and adisconnect()reconnect-race gap inuseTerminalStreamwhere a staledisconnect's delayed abort could clobber a newer connection generation
— both fixed with dedicated regression tests confirmed to fail pre-fix
and pass post-fix.
mcp__stapler-squad__create_session(the path that actually exercisesStartSessionDriver, unlike the web UI's omnibar): induced the ticket'sexact "session not started or paused" flap via pause/resume immediately
after session creation, and confirmed via server logs the startup dialog
was answered exactly once inside the flap window, with a clean recovery
and zero repeated
1characters. Full details inproject_plans/phantom-keystroke-replay/implementation/plan.md.The backlog review tooling got stuck in a "review" status after the second
submission (a "concurrent modification: expected in_progress, got review"
precondition error persisted across multiple retries and ~35 minutes of
waiting, despite a completed FAIL verdict already being on record) — this
appears to be a backlog-automation state issue rather than anything code-
related, so this PR is being opened for human review per the pipeline's
exhausted-retry fallback rather than waiting indefinitely.
Test plan
go test -race ./session ./server/services— clean, no racesgo test ./session/... ./server/...— all greengo test ./session -run TestAnswerDialogOnce -v— all 7 subtests passcd web-app && npx jest --no-coverage --testPathPatterns="TerminalOutput|MessageQueue|InputDropBadge|useTerminalStream|useDropEpisodeCoalescer"— 8 suites / 94 tests pass, including the new disconnect()-reconnect-race regression test (confirmed to fail against the pre-fix code, pass against the fix)make lint— 0 issuesmake build— web UI + Go binary build successfullymake test— full suite greenmcp__stapler-squad__create_session+ induced pause/resume flap — no repeated phantom keystrokes, clean recovery (see plan.md for full log evidence)main/HEAD viagit stashbefore/after comparison — not caused by this branch, left unaddressed as out of scopeorigin/main— see blocker note at topCloses backlog item
04089969-0f19-499c-be34-2e8bcfc4f13e.🤖 Generated with Claude Code