diff --git a/context/knowledge/gotchas/hera-view.md b/context/knowledge/gotchas/hera-view.md index e8f9ac2e..0b75ac43 100644 --- a/context/knowledge/gotchas/hera-view.md +++ b/context/knowledge/gotchas/hera-view.md @@ -15,6 +15,7 @@ M6a scaffolds the native Hera view: a `HeraPage` (rail | coordinator pane | agen - **PTY size alignment (CLAUDE.md rule 5): a hera pane and the main agent pane compute the SAME size for the same rect — they are the same `TerminalPane` widget.** When binding a session, `bindPane` calls `ForceResyncPTY()` so a session previously sized for the full-width main agent view gets resized DOWN to the narrower hera pane on the next Draw (without it, the seeded `ptyCols` matches `wantCols` and no resize fires, leaving the agent painting at the stale width). `SyncPanes()` (App tick GOROUTINE + spinner loop, never a QueueUpdateDraw callback) issues the Resize RPC off the main thread. It is safe to call off-tab: a pane that wasn't drawn this frame has zero `pendingResize`, so an off-tab `SyncPanes` can't fight the main agent view's resize of the same task. Never reach for `Sync()` to fix a size mismatch — fix the alignment. - **A plain `ForceResyncPTY()` resize can't repair scrollback already committed at a different width — Hera panes never had the main agent view's kill+resume safety net for that (BUG-074), and are MORE exposed to it than the main view.** SIGWINCH re-flows only live UI; cursor-positioning codes baked into earlier PTY output stay wrong once re-emulated at a new size (the `agent.ShouldKickRerender`/`RerenderMargin` mechanism exists precisely for this — see `App.maybeKickRerender`/gotchas/pty-terminal.md — but it was wired ONLY into the main agent view's entry path, never into `bindPane`). Reproduced offline against a real dogfood session log (small, well under any 8MB-window concern — this is independent of BUG-068/BUG-073's lossy-tail-rebuild mechanism): a session resized once during its real life shows a duplicated footer/blank-frame pattern when replayed at a fixed size, exactly matching a live screenshot report. Hera panes hit this MORE than the main view because `bindPane` calls `ForceResyncPTY()` unconditionally on every single bind — any task viewed repeatedly in Hera accumulates real size transitions in its history over time. Fix: `App.maybeKickRerenderAtWidth(task, sess, panelCols)` is the width-parameterized core extracted from `maybeKickRerender` (which now just supplies `computePTYSize()`'s result); `App.heraKickRerender(taskID, panelCols)` is the Hera-facing entry point, wired via `HeraPage.SetRerenderKicker` (mirrors `SetSessionResolver`'s pattern — resolves `task`/`sess` by ID itself so `internal/tui/hera` never needs direct `db`/`runner` access). Shares `isRedundantAttach`'s cache (keyed by task ID only, not by which surface asked — a task alternately viewed from the main view and a Hera pane can kick once per surface-switch; accepted, see design.md for `fix-hera-pane-rerender-kick`). - **The kick check is evaluated from `Draw()` (`HeraPage.maybeKickPaneRerender`), never from `bindPane` itself, despite `bindPane` being where the session is actually resolved.** `bindPane` runs synchronously in the input handler, BEFORE Draw() has had a chance to give a newly-shown pane its real rect — the clearest case is a coordinator selected first (details mode: the agent pane is never shown, never `SetRect`, tracked width stays 0) then a worker selected for the first time this session: at THAT bind moment the agent pane's width is still 0, so a synchronous read there would silently skip the check for the very first coordinator→worker transition every session. Fix: `Draw()` calls `maybeKickPaneRerender(bound, kickedFor, cols)` right after each of its four `SetRect` call sites (fullscreen coord/agent, split coord/agent) — `cols` there is always the JUST-computed, correct value. `coordKickedFor`/`agentKickedFor` (paired with `coordBound`/`agentBound`) suppress a redundant call every frame while the same task stays bound and visible — NOT a correctness gate (that's still `isRedundantAttach`, downstream), purely an optimization to skip the DB+runner lookup; `bindPane` resets the marker to `""` on unbind so a later rebind to the SAME task still gets evaluated. +- **The kick itself is debounced by a 300ms wall-clock dwell (`hera.KickDebounce`, fix-hera-tick-and-kick-perf) — ordinary rail nav ALONE (no resize, no fullscreen toggle) swings a bound pane between full-width and split-width, crossing `RerenderMargin` on every hop of a fast Cmd+Arrow traversal and kicking every transiently-bound task ("kick storm").** `maybeKickPaneRerender` now arms a per-pane `kickPending{taskID, cols, deadline}` on first seeing a newly-bound task past the margin instead of firing immediately; only a LATER call, once the dwell elapses AND the same task is STILL bound, actually invokes `kickRerender`. A rebind to a DIFFERENT task before the dwell re-arms against the new task, discarding the old one un-fired. **`bindPane` also zeroes `pending` on unbind** — omitting this (the first implementation attempt) left a stale, already-past deadline surviving an unbind+rebind-to-the-SAME-task, firing the kick immediately instead of dwelling afresh (caught by `TestPanes_KickDebounce_UnbindMidDwellThenRebindSameTask`). No new goroutine/timer — checked on the same Draw cadence, mirroring `Refresher`'s goroutine-free style, via an injectable `kickNow`/`SetKickClock` seam for tests. - **Main-thread-safe pane reads only.** Pane operations on the tview thread use the lock-free/local SessionHandle methods (`Alive`, `RecentOutput*`, `WriteInput`, `Session()`) — never the RPC-blocking `PID/IsIdle/PTYSize/InitialPTYSize`. The blocking `SyncPTYSize`→`Resize` RPC runs only from the tick goroutine via `SyncPanes`. `SetSession`/`SetTaskID`/`applySelection`/`reconcileSessions` are main-goroutine-only (SetSession resets emulator state). - **`reconcileSessions` (App tick, main thread) does the nil→live late bind AND the dead→live re-resolve (BUG-013).** A pane bound to a task with no live session yet (coordinator/worker still starting) gets its session attached on a later tick. `doRefresh` re-runs `applySelection` after `SetModel` because the rebuild replaces the model's backing arrays, invalidating the prior `Selection` pointers (task IDs usually match, so `bindPane` no-ops and emulators survive). - **A present-but-DEAD pane session MUST be re-resolved, not just a changed taskID (BUG-013).** When the daemon tears a pane's stream down (StreamLost relay / daemon bounce) while the agent PTY is still alive, `RemoteSession.Alive()` flips false but the agent lives. The pane held the dead handle forever: `reconcileOne` used to bail on ANY present session and `bindPane` no-ops on the unchanged taskID, so `forwardKey` dropped every keystroke (silently) until a full TUI restart re-dialed the stream. Fix: `reconcileOne` treats `!Alive()` like nil and re-resolves — the daemon client re-dials a fresh stream on a cache-miss `Get` when the daemon reports the process alive. Replace ONLY with a live, DISTINCT handle; leave the pane alone when the resolver yields nil (process gone → replay) or the same not-yet-evicted handle (retry next tick), or you reset the emulator every tick. `forwardKey` now logs the drop (`[hera-view]`) and re-resolves before dropping. Orthogonal to the `Enter`-reattach path, which leaves a LIVE coordinator navigate-only and restarts a truly dead PROCESS — BUG-013 is a live process with a stale handle. @@ -26,6 +27,9 @@ M6a scaffolds the native Hera view: a `HeraPage` (rail | coordinator pane | agen - **Freelance-kind roles are HOISTED into a top-level Freelance section, not nested under their orchestrator.** `BuildModel` skips active freelance-kind roles when filling an orchestrator's `Roles` and appends them to `Model.Freelance` instead. This is a 6a read-only interpretation of the "freelance" section (Hera derived it from unmanaged live argus tasks; that data source is out of scope for the read-only hera store). 6b can revisit if real unmanaged-task freelance is wanted. - **`ready_to_close` (M4) is read from the task-addressed `task_meta` "hera" namespace, NOT the hera tables.** `BuildModel` does one `ListMetaByNamespace("hera")` batch read and flags `RoleView.ReadyToClose` when the role's bound task has `ready_to_close=true`. The rail renders it with the distinct `theme.IconReview` mark, which WINS over the role's idle/working/blocked/done status icon but LOSES to needs-input `(?)` (BUG-A: an actively-blocked worker is not ready to close). A meta read error is non-fatal (the flag just doesn't render). - **The rail rebuild runs on the tview thread; that's fine because hera-store reads are mutex-guarded and fast — the "never on the UI thread" rule is about GIT, not DB reads.** `Refresher` (the Argus-native analog of Hera's `RailRefresher`) is goroutine-/timer-free: `Schedule()` is driven by the app tick + tab entry and coalesces bursts into one rebuild per debounce window via an injected clock (`SetNow`), so it's deterministic to test. `refreshHera` is `heraPage.Refresh()` on tab entry (forces a flush) and `ScheduleRefresh()` on the tick while `ActiveTab()==TabHera`. +- **`doRefresh`'s `BuildModel`+`SetModel` pass is gated by cheap change-detection (`HeraPage.shouldRebuild`/`markRebuilt`, fix-hera-tick-and-kick-perf) — the debounce above only bounds how OFTEN a rebuild opportunity arrives, not whether one that arrives is actually worth paying for; every rebuild used to run `ListHeraOrchestrators(true)` (archived included) + all bindings + `Rail.buildRows()`'s full `canonicalParents`/`structuralReach` graph walk UNCONDITIONALLY, once a second, scaling with TOTAL historical role/orchestrator/binding count (Aaron's ~900+) regardless of active-agent count — measured at 34ms/30MB/133k allocs per rebuild at that scale (`internal/tui/hera/doRefresh_bench_test.go`).** The gate combines a cheap SQLite `PRAGMA data_version` fingerprint (`db.DB.DataVersion`, near-O(1), catches any OTHER connection's write — the dominant real source, since daemon/MCP-driven hera mutations use a separate connection from the TUI's own) with a `maps.Equal` comparison of the four per-tick RUNTIME maps also fed into the model (`needsInput`/`sessionIdle`/`sessionRunning`/`sustainedActive` — bounded by LIVE session count, cheap even at 900+ total roles); either changing triggers a rebuild, so a quiet DB with active agents still animates the spinner/`(?)` glyphs correctly. Post-gate steady-state cost measured at ~850ns/400B/12 allocs per tick (idle) — a ~35,000x reduction. +- **`PRAGMA data_version` does NOT change what a connection reads back after ITS OWN write (SQLite's documented same-connection blind spot, verified against `modernc.org/sqlite` under this repo's WAL DSN) — the TUI's `a.db` both reads (the tick) and writes (hera mutations, reconciliation) through ONE connection.** Closed not by ignoring the blind spot but by `HeraPage.Refresh()` (the general "force it now" primitive — tab entry, every `App.heraRefresh()`-driven mutation, and tests) calling `InvalidateChangeGate()` before flushing, so ITS OWN "forces an immediate rebuild" doc contract holds regardless of the gate. **Scoping the invalidation to `heraRefresh()` alone (the first implementation attempt) broke `TestRefresh_StatusStepReprojectsPlanNode`**, which writes through the same `*db.DB` the page reads from and calls `Refresh()` directly, expecting an unconditional rebuild — `Refresh()` itself is the right chokepoint, not each individual caller. `ScheduleRefresh` (the tick's own periodic, non-forced call) is deliberately left subject to the gate. +- **Explicit non-goal, not silently dropped: the gate does NOT cover `refreshTasksWithIDs`'s own base reads** (`db.Tasks()`, `ListMetaByNamespace("pr")`/`("hera")`, `ManagedTaskIDs()`) or lazy-load archived rows out of `Rail.buildRows()`'s graph algorithms — see `openspec/changes/archive/.../fix-hera-tick-and-kick-perf/proposal.md`'s Impact section for why both were scoped out (more un-audited local-write call sites; risk to `rail.go`'s heavily invariant-laden fold logic, respectively). The double `Tasks()` fetch per tick (once in `refreshTasksWithIDs`, once inside `BuildModel`) IS de-duplicated — `HeraPage.SetTasks` + an internal `tasksReader` wrapper serve `BuildModel`'s `Tasks()` call from the App's already-fetched snapshot instead of a second query, constructed transiently at the `BuildModel` call site (never stored back into `p.reader`) so it can't interfere with the fingerprint's own type-assertion against the unwrapped reader. - **`HeraPage.Machine()`, not `Focus()`.** A method named `Focus()` collides with `tview.Primitive.Focus(func(tview.Primitive))` and silently breaks the Primitive interface (the page stops being a valid tview widget). The focus-machine accessor is `Machine()`. The `FocusMachine` is wired for 6b (Advance/Retreat/present-pane rebalance); 6a keeps focus on the rail and only reads `State()` to pick the border-highlight palette. - **The rail navigates with `j`/`k`/Up/Down and folds with Space — never arrows-for-parent-nav like Hera's upstream rail did.** Historically the rail was BARRED from binding Left/Right because the global handler ate them for tab switching; that arrow-key tab-cycling has since been removed (tab nav is `1`/`2`/`3` only), so Left/Right now fall through to the page: a terminal-focused pane (coord/worker-agent) forwards them to the PTY, a coordinator's details region routes them to the embedded plan widget's cursor (slot/member nav, M7), while the rail leaves them unused (free for future horizontal navigation). - **`Ctrl+Alt+Left/Right` walk the focus ladder (Retreat/Advance), mirroring `Tab`/`Backtab`.** Handled in `HeraPage.InputHandler`'s top-level key switch BEFORE the per-region forward, so they reach `FocusMachine.Retreat`/`Advance` rather than being forwarded to a focused terminal pane's PTY (a plain Left/Right still forwards). The modifier check is `Modifiers()&(ModCtrl|ModAlt) != 0` — **either** modifier, not strictly both — matching the main agent view's Ctrl+Alt+arrow pane switch (`app.go`); terminals are inconsistent about which of Ctrl/Alt they report for this chord, so the loose check is the proven pattern. This binding lives in the page handler (not `handleGlobalKey`) precisely because plain Left/Right intentionally fall through the global handler to the focused view. diff --git a/context/knowledge/index.md b/context/knowledge/index.md index bffecac3..b468e0f7 100644 --- a/context/knowledge/index.md +++ b/context/knowledge/index.md @@ -16,7 +16,7 @@ Non-obvious invariants and gotchas, split by topic. Read the relevant file when | [gotchas/misc.md](gotchas/misc.md) | DB patterns, Go idioms, Codex, Pi/ollama prelaunch, MCP, PRs, file explorer, quick-add, scheduled tasks, links, task auto-naming, settings two-pane UI, plugin-scoped token tagging, task_meta sidecar, plugin settings registry, Claude session discovery (folder encoding, JSONL parse), config.toml override layer (precedence, partial-map zeroing, mtime live-reload), hera plugin contract (POST/DELETE /notify), model selection (--model injection, codex-resume re-append), per-schedule model override (TaskCreator taskModel → HeadlessInput.Model), wall-clock duration clamp, link PR indicator (IsPR single-source + prLinkGutter), openspec archive workflow (rc0-on-abort, authoring-order fold, orphaned-superseded-requirement audit, authoring-drift fixes, doubled-date-prefix on pre-dated change folders), diligence profiles (resolution fail-open, in-repo precedence, daemon-side resolution vs sandbox EPERM, [panel] deferred seam, 3 schema columns, embedded seed install via InstallDefaults, archetype/rigor JSON casing fix + native-dispatch convention pointer), opencode backend (capture-style/no --session-id, --session resume, root-commit-keyed capture filtered by directory, SQLite-first+JSON-fallback fail-open, MCP type:"remote", ctrl+r switcher excludes it), builtin skills/routing spawn-time injection (unconditional --add-dir + --append-system-prompt-file, self-gating content, isTestBinary() short-circuit, ensureBuiltinRoutingFn test seam, manual install-claude-skills.sh mechanism retired, skills embed-drift vs .claude/skills unfixed, generic directory-tree iteration with no per-name whitelist, hera-review/hera-review-test-adversary parity shipped + hera-spawn-review/resolve-archetype-model parity shipped once profile_resolve/internal-review infra landed), landing risk of stacking work on a not-yet-merged integration branch (hera-nav-palette took 3 attempts across #865/#882/#886 — worktree/branch cleanup before merge stranded stacked branches twice despite the code itself being correct), `argus doctor` diligence-profile-library check cross-ref (add-doctor-profile-check: see gotchas/daemon-rpc.md) | 194 | | [gotchas/orchestration.md](gotchas/orchestration.md) | depends_on DAG / depswatcher / link-unlink-halt / plan_slug all RETIRED (Hera is the single model; base_branch kept; tasks start immediately), task_set_result opaque-to-daemon, ARGUS_TASK_ID env export, (name,project) idempotency, schema column ordering, hera schema/store M1 (FK-cascade, NULL partial-index, per-(task,orchestrator) multi-binding + ErrHeraAmbiguous, transactional role+binding), hera M4 (born-bound spawn + AfterPersist hook; auto-adopt removed; ReconcileBindings keyed on task-row; HeraCoordinatorOrientation headline = coord DISPATCHES not implements (actual work → hera_spawn_worker regardless of repo; self-invoking hera_new_orchestrator = relabel-and-implement-solo antipattern, not a sub-team); new orch only for multi-project/phase sub-team whose WORKERS do the work; hera_new_orchestrator CODE guard rejects a caller already coordinating a DIFFERENT orch, before orch-create, fail-open, MCP-only), hera M5 (subtree TLDR roll-up: SubtreeOrchIDs BFS + cycle guard, tree_read_cursors cascade, advisory per-role cursor), hera plan-DAG substrate (planned node = role w/ no binding, gate on role-status done, failed-blocker → HOLD + ping coordinator, in-tx DFS cycle check, check-in pulled via inbox), hera_join reject+redirect to new hera_move tool (fix-hera-join-move-binding; self-promotion remains the only 2+-binding path), coordinator context management (coord-hook global-settings requirement + ARGUS_TASK_ID-first self-gate, unconditional context_size stamp vs budget-gated block, presence-=-block no-cooldown nudge, hera_status handoff_note/request_recycle widened to ANY hera-bound role kind not just coordinator (add-worker-bounce), request_recycle as flag-only vs watcher-driven action, recycle_coord self-service idle-wait vs human-forced immediate (human-forced stays coordinator-only), RecycleWatcher.tickTask role-kind filter widened to worker/freelance (coordinator-kind still preferred among 2+ live bindings), SessionID-clear-before-Recycle ordering, best-effort stray-job cleanup, zero-follow-up-call seed prompt, B key bounces worker/freelance via self-service instruct-and-wait instead of no-op (add-worker-bounce), no-op only on an empty selection), coord-hook stamp gate ALSO widened coordinator-only → any hera-bound role for context_size specifically (add-worker-context-indicator, budget/nudge/recycle stays coordinator-only — a separate widening from add-worker-bounce's hera_status/RecycleWatcher/B-key changes above), RecycleRunner.Restart must resolve via the caller-supplied roleID not a re-derived taskID-keyed lookup (fix-recycle-restart-ambiguous-binding, dual-bound self-service recycle previously wedged forever on ErrHeraAmbiguous), hera plan-DAG hygiene (add-hera-plan-hygiene: planned-node parent-orchestrator liveness filter + cascade-cancel on archive/nuke is belt-and-braces not either-alone, materializeNode consecutive-failure escalation mirrors holdAndPing's ping-once dedup not EscalateParkedSelection's torn-read threshold, freelance-role flat-section staleness is data-hygiene not a display bug) | 78 | | [gotchas/dag-rendering.md](gotchas/dag-rendering.md) | dagview is now a LAYOUT LIBRARY (dagview.Compute longest-path stage placement) consumed by internal/tui/planview — standalone widget no longer mounted; PLAN-DAG widget (planview) replaced the retired orchestration-tree graph, heraTreeNodes/tree.go DELETED. Sugiyama-lite layer math, rune-vs-byte truncation, single-line edges, branch-change log-only. planview node State/colour from RoleView.TaskStatus/TaskResult ({"failed":true}→red ✕); planview surfaces OnEnter/OnDrillIn/OnDrillOut/OnBranchChange (read-only nav, no edit callbacks); branchShape folds cursor+fanned-group+orch-title | 18 | -| [gotchas/hera-view.md](gotchas/hera-view.md) | Native Hera view (`internal/tui/hera`): 2nd tab display label "Projects" (internal names stay Hera); 2nd tab always native (cfg.Hera.Enabled gates only daemon MCP tools, not tick refresh); structural multi-binding fan-out; freelance section; ready_to_close from task_meta; goroutine-free debounced Refresher on UI thread; rail nav j/k/Up/Down + Space (tab nav 1/2/3 only); no Sync + full-rect coverage. Panes fed from in-process runner ring (poll, not SSE); coord-vs-agent session rule; multi-binding disambiguated by role.OrchID; PTY align via ForceResyncPTY + off-thread SyncPanes. Thin mutation layer (ops.go over M1; shared agent.SpawnHeraWorker); rail keyset via OnXxx callbacks; multi-binding isolation; s/S step hera ROLE status; modal.ConfirmModal + NewInputForm; remote=nil ⇒ inert. Details 2 modes (worker→terminal, coordinator→stacked roster-over-PLAN — same geometry as roster-over-tree); embedded PLAN-DAG graph via heraPlanNodesWithBridge(orch, bridgeIndex) — coordinators not plan nodes, Drillable needs WithBridge form; planview↔hera import one-way (hera imports planview); OnDrillIn page-owned (drillIntoChild→bridgeIndex→PushOrch), OnEnter App-owned; handleDetailsKey Esc-at-root escapes pane; node colour from TaskStatus/TaskResult. Ctrl+Z→fullscreen (closes the Claude-Code-own-supervisor detach footgun, worse than a mere SIGTSTP); Enter revives dead (startSession) or suspended worker (reviveHeraWorker→KickRerender, idle+not-blocked gated; live coordinator navigate-only); jumpToLeaf expands ancestor coordinators (EnsureAncestorsExpanded via canonicalParents + OrchIDsForTask) before SelectByTaskID so a folded coord doesn't swallow the join (BUG-007); J-detach (DetachCoordinator) = re-parent teardown without recreate, shared teardownParentLinks, idempotent, detach sentinel by pointer identity; nested sub-coord = headerless worker-bridge row → heraCoordReparentTarget qualifies worker w/ BridgeChildOrchID!=0 so J detach/re-parent reaches the child orch (plain worker never misclassified); needs-input rail gate is LIVENESS-based for ALL kinds (buildRoleView `taskInProgress || rv.Live`; live worker/coordinator/freelance surfaces (?) regardless of task status, incl. a worker in in_review per #707 — needsInputForHeraRail admits the heraManaged union (workers+coordinators), BUG-A supersedes the BUG-028 worker-only carve-out; BUG-023 now guarded by binding-liveness not task-status; flat task-list stays in_progress-gated); needs-input OUTRANKS ready_to_close in RoleStatusIcon (an actively-blocked worker is not ready to close, BUG-A); coordinator-less orch header surfaces rollup via OrchView.SubtreeNeedsInput in drawOrchRow else-if (BUG-028); content-aware spinner (RoleView.IsActive gated on !SessionIdle, fed from App content-idle set so a parked fullscreen agent stops animating, BUG-036); IsActive spinner OUTRANKS ready_to_close/failed/done in RoleStatusIcon (BUG-F, icon-precedence completion of BUG-C; resting case kept via IsActive's running/!idle gate); bulk cascade-nuke silent multi-second freeze (BUG-062, no data race — synchronous per-task session-stop RPC on the tview goroutine) fixed via backgrounded stop (heraGoSafe) + SyncPTYSize panic recovery mirroring Draw(); kanban_status (add-hera-kanban-status) independent 4th axis on top-level coordinators (active/backlog/blocked/done, default active, hera_orchestrators column no CHECK) grouping the rail's Active bucket with dividers, stepped by m/M (wraps, distinct from s/S role-status clamp); needs-input rollup EXCLUDES archived nodes (exclude-archived-from-needs-input-rollup) via a DEDICATED archive-aware orchSubtreeNeedsInput walk (not BridgeSubtree reuse — BridgeSubtree keeps archived rows for dimmed-in-place rendering), gating descent on the bridging role's Archived AND the worker-bridge target orch's own !c.Archived; archived role's own row still shows (?); rail Enter-reattach `live` check now tests Alive() not mere non-nil (BUG-064, shared HeraPage.sessionLive) — a cached-but-disconnected coordinator handle (BUG-013) used to make the first Enter skip reattach and only focus, requiring a second Enter routed through the pane's own InputHandler to actually restart; kanban groups auto-fold to the focused group (add-kanban-focus-fold): Active gains a uniform header/divider (headerless special-case retired), Rail.focusedKanban resolved BEFORE buildRows via focusGroupOf (chicken-and-egg), step() boundary-crossing expand/collapse via landOnGroupMember, SetModel/SelectByTaskID/EnsureAncestorsExpanded each independently re-focus, not persisted; ctrl+j switcher literal case (mirrors Ctrl+Z) + exported JumpToTask (jumpToLeaf now a thin wrapper); ctrl+k global palette Hera reach (two enumerated non-keymap literal rows — fullscreen/copy — + heraRailActionRegistry over existing OnXxx callbacks); rail partial-fold reveal (appendOrchWorkers/appendWorkerRow revealOnly mode + appendOrchRevealPath, extends the one true traversal rather than forking a parallel one, fold state never mutated); BUG-064 (nested-reveal-lost-on-reexpand variant): appendWorkerRow's bridged-child branch needed the same else-if child.SubtreeNeedsInput fallback appendPinnedRole already had, else re-expanding an outer coordinator while a nested sub-coordinator stayed collapsed silently dropped the nested needs-input leaf; ctrl+g jump-to-next-needs-input (Rail.NextNeedsInputTaskID scan-and-cycle over built row order + HeraPage.JumpToNextNeedsInput reusing JumpToTask verbatim; candidates require row.role!=nil so a top-level coordinator's own need — folded into the rrOrch header, unreachable via SelectByTaskID — is deliberately excluded, unlike a nested sub-coordinator's bridging row), worker/freelance context-pressure indicator (add-worker-context-indicator: always-reserved trailing 2-col slot, coordinator-excluded, local-mode-only ContextPercent, bare coordinator count), context-size undercounting root cause is transcript_path's documented async-write lag not sidechains (fix-context-stop-lag: no Stop-hook field carries usage data, last_assistant_message is plain text; bounded retry-and-take-max in readContextSizeReal is the fix, gated by an early exit — contextSizeReadPrevious seam compares one scan against the task's prior stamp, skipping the retry unless the scan is below-prior or there's no prior stamp yet, so the ~200ms budget isn't an unconditional per-turn tax across the whole coordinator/worker/freelance fleet; isSidechain skip kept as cheap defensive hardening, empirically a no-op under the current CLI); ctrl+g/ctrl+b excursion re-arm was count-based not identity-based (BUG-069, live dogfood repro) — a stale never-resolved needs-input role kept the count >=1 forever so a restore re-armed and froze on the very next rebuild regardless of novelty, silently discarding the operator's post-restore navigation; fixed via role-ID set-membership tracking (`Rail.armedNeedsInputIDs`/`Model.needsInputRoleIDs`/`hasNewNeedsInputID`) that continuously refreshes until a genuinely new distinct role id appears; separately confirmed (not fixed) a narrow pre-existing `currentRef()`/`restoreCursor` gap shared with BUG-002 for cursor-on-fold-row capture; the identity-tracking fix still froze a bogus snapshot on a Rail's first-ever `SetModel` call when stale needs-input predated a TUI launch/relaunch (BUG-070, discovered dogfooding BUG-069 — no rows yet for `currentRef()`, fold state still the previous session's persisted layout) — fixed via a `r.rows == nil` early-return guard (seed-only, no capture) on the literal first call; the partial-fold reveal was stateless across rebuilds, so a selected role revealed only via needs-input vanished (yanking cursor + panes) the instant its own flag cleared (BUG-071) — fixed via `Rail.applyStickyReveal` forcing `SubtreeNeedsInput` along the selected row's ancestor chain in the fresh model before `buildRows`, re-derived from the current cursor identity every rebuild so it releases the moment selection moves elsewhere; size-drift kill+resume kick extended to Hera panes (BUG-074, `heraKickRerender`/`maybeKickPaneRerender`) — a plain `ForceResyncPTY()` can't repair scrollback already committed at a different width, and Hera panes are MORE exposed than the main agent view since `bindPane` resizes on every single bind; evaluated from `Draw()` (fresh per-pane width via `coordKickedFor`/`agentKickedFor`), never from `bindPane` itself (whose tracked width can still be 0 for a pane not yet shown, e.g. the agent pane during details mode); BUG-076 (false "Session not running" after ordinary rail nav) — root cause was `handleSessionExitUI`'s post-kick auto-restart gate checking ONLY `a.mode==modeAgent`, never true on the Hera tab, so every BUG-074 size-drift kick fired from a Hera pane genuinely stopped the session and then always skipped the restart as "user navigated away"; fixed via `App.isViewingTaskSession`/`HeraPage.IsBoundToTask` recognizing the Hera-tab-with-bound-pane case too; needs-input sharpened to suppress on sustained activity (narrow-needs-input-sustained-active: `RoleView.SustainedActive`, per-TASK not per-role so a dual-bound sub-coordinator's stale-blocked worker hat is suppressed by the coordinator hat's own sustained activity with no per-hat lookup; `needsInputOwn()` checks it first; fed via a NEW `agent.SustainedActivityTick` grace-tolerant sibling of `ResumeActivityTick`, not a change to `ResumeActivityTick` itself) | 171 | +| [gotchas/hera-view.md](gotchas/hera-view.md) | Native Hera view (`internal/tui/hera`): 2nd tab display label "Projects" (internal names stay Hera); 2nd tab always native (cfg.Hera.Enabled gates only daemon MCP tools, not tick refresh); structural multi-binding fan-out; freelance section; ready_to_close from task_meta; goroutine-free debounced Refresher on UI thread; rail nav j/k/Up/Down + Space (tab nav 1/2/3 only); no Sync + full-rect coverage. Panes fed from in-process runner ring (poll, not SSE); coord-vs-agent session rule; multi-binding disambiguated by role.OrchID; PTY align via ForceResyncPTY + off-thread SyncPanes. Thin mutation layer (ops.go over M1; shared agent.SpawnHeraWorker); rail keyset via OnXxx callbacks; multi-binding isolation; s/S step hera ROLE status; modal.ConfirmModal + NewInputForm; remote=nil ⇒ inert. Details 2 modes (worker→terminal, coordinator→stacked roster-over-PLAN — same geometry as roster-over-tree); embedded PLAN-DAG graph via heraPlanNodesWithBridge(orch, bridgeIndex) — coordinators not plan nodes, Drillable needs WithBridge form; planview↔hera import one-way (hera imports planview); OnDrillIn page-owned (drillIntoChild→bridgeIndex→PushOrch), OnEnter App-owned; handleDetailsKey Esc-at-root escapes pane; node colour from TaskStatus/TaskResult. Ctrl+Z→fullscreen (closes the Claude-Code-own-supervisor detach footgun, worse than a mere SIGTSTP); Enter revives dead (startSession) or suspended worker (reviveHeraWorker→KickRerender, idle+not-blocked gated; live coordinator navigate-only); jumpToLeaf expands ancestor coordinators (EnsureAncestorsExpanded via canonicalParents + OrchIDsForTask) before SelectByTaskID so a folded coord doesn't swallow the join (BUG-007); J-detach (DetachCoordinator) = re-parent teardown without recreate, shared teardownParentLinks, idempotent, detach sentinel by pointer identity; nested sub-coord = headerless worker-bridge row → heraCoordReparentTarget qualifies worker w/ BridgeChildOrchID!=0 so J detach/re-parent reaches the child orch (plain worker never misclassified); needs-input rail gate is LIVENESS-based for ALL kinds (buildRoleView `taskInProgress || rv.Live`; live worker/coordinator/freelance surfaces (?) regardless of task status, incl. a worker in in_review per #707 — needsInputForHeraRail admits the heraManaged union (workers+coordinators), BUG-A supersedes the BUG-028 worker-only carve-out; BUG-023 now guarded by binding-liveness not task-status; flat task-list stays in_progress-gated); needs-input OUTRANKS ready_to_close in RoleStatusIcon (an actively-blocked worker is not ready to close, BUG-A); coordinator-less orch header surfaces rollup via OrchView.SubtreeNeedsInput in drawOrchRow else-if (BUG-028); content-aware spinner (RoleView.IsActive gated on !SessionIdle, fed from App content-idle set so a parked fullscreen agent stops animating, BUG-036); IsActive spinner OUTRANKS ready_to_close/failed/done in RoleStatusIcon (BUG-F, icon-precedence completion of BUG-C; resting case kept via IsActive's running/!idle gate); bulk cascade-nuke silent multi-second freeze (BUG-062, no data race — synchronous per-task session-stop RPC on the tview goroutine) fixed via backgrounded stop (heraGoSafe) + SyncPTYSize panic recovery mirroring Draw(); kanban_status (add-hera-kanban-status) independent 4th axis on top-level coordinators (active/backlog/blocked/done, default active, hera_orchestrators column no CHECK) grouping the rail's Active bucket with dividers, stepped by m/M (wraps, distinct from s/S role-status clamp); needs-input rollup EXCLUDES archived nodes (exclude-archived-from-needs-input-rollup) via a DEDICATED archive-aware orchSubtreeNeedsInput walk (not BridgeSubtree reuse — BridgeSubtree keeps archived rows for dimmed-in-place rendering), gating descent on the bridging role's Archived AND the worker-bridge target orch's own !c.Archived; archived role's own row still shows (?); rail Enter-reattach `live` check now tests Alive() not mere non-nil (BUG-064, shared HeraPage.sessionLive) — a cached-but-disconnected coordinator handle (BUG-013) used to make the first Enter skip reattach and only focus, requiring a second Enter routed through the pane's own InputHandler to actually restart; kanban groups auto-fold to the focused group (add-kanban-focus-fold): Active gains a uniform header/divider (headerless special-case retired), Rail.focusedKanban resolved BEFORE buildRows via focusGroupOf (chicken-and-egg), step() boundary-crossing expand/collapse via landOnGroupMember, SetModel/SelectByTaskID/EnsureAncestorsExpanded each independently re-focus, not persisted; ctrl+j switcher literal case (mirrors Ctrl+Z) + exported JumpToTask (jumpToLeaf now a thin wrapper); ctrl+k global palette Hera reach (two enumerated non-keymap literal rows — fullscreen/copy — + heraRailActionRegistry over existing OnXxx callbacks); rail partial-fold reveal (appendOrchWorkers/appendWorkerRow revealOnly mode + appendOrchRevealPath, extends the one true traversal rather than forking a parallel one, fold state never mutated); BUG-064 (nested-reveal-lost-on-reexpand variant): appendWorkerRow's bridged-child branch needed the same else-if child.SubtreeNeedsInput fallback appendPinnedRole already had, else re-expanding an outer coordinator while a nested sub-coordinator stayed collapsed silently dropped the nested needs-input leaf; ctrl+g jump-to-next-needs-input (Rail.NextNeedsInputTaskID scan-and-cycle over built row order + HeraPage.JumpToNextNeedsInput reusing JumpToTask verbatim; candidates require row.role!=nil so a top-level coordinator's own need — folded into the rrOrch header, unreachable via SelectByTaskID — is deliberately excluded, unlike a nested sub-coordinator's bridging row), worker/freelance context-pressure indicator (add-worker-context-indicator: always-reserved trailing 2-col slot, coordinator-excluded, local-mode-only ContextPercent, bare coordinator count), context-size undercounting root cause is transcript_path's documented async-write lag not sidechains (fix-context-stop-lag: no Stop-hook field carries usage data, last_assistant_message is plain text; bounded retry-and-take-max in readContextSizeReal is the fix, gated by an early exit — contextSizeReadPrevious seam compares one scan against the task's prior stamp, skipping the retry unless the scan is below-prior or there's no prior stamp yet, so the ~200ms budget isn't an unconditional per-turn tax across the whole coordinator/worker/freelance fleet; isSidechain skip kept as cheap defensive hardening, empirically a no-op under the current CLI); ctrl+g/ctrl+b excursion re-arm was count-based not identity-based (BUG-069, live dogfood repro) — a stale never-resolved needs-input role kept the count >=1 forever so a restore re-armed and froze on the very next rebuild regardless of novelty, silently discarding the operator's post-restore navigation; fixed via role-ID set-membership tracking (`Rail.armedNeedsInputIDs`/`Model.needsInputRoleIDs`/`hasNewNeedsInputID`) that continuously refreshes until a genuinely new distinct role id appears; separately confirmed (not fixed) a narrow pre-existing `currentRef()`/`restoreCursor` gap shared with BUG-002 for cursor-on-fold-row capture; the identity-tracking fix still froze a bogus snapshot on a Rail's first-ever `SetModel` call when stale needs-input predated a TUI launch/relaunch (BUG-070, discovered dogfooding BUG-069 — no rows yet for `currentRef()`, fold state still the previous session's persisted layout) — fixed via a `r.rows == nil` early-return guard (seed-only, no capture) on the literal first call; the partial-fold reveal was stateless across rebuilds, so a selected role revealed only via needs-input vanished (yanking cursor + panes) the instant its own flag cleared (BUG-071) — fixed via `Rail.applyStickyReveal` forcing `SubtreeNeedsInput` along the selected row's ancestor chain in the fresh model before `buildRows`, re-derived from the current cursor identity every rebuild so it releases the moment selection moves elsewhere; size-drift kill+resume kick extended to Hera panes (BUG-074, `heraKickRerender`/`maybeKickPaneRerender`) — a plain `ForceResyncPTY()` can't repair scrollback already committed at a different width, and Hera panes are MORE exposed than the main agent view since `bindPane` resizes on every single bind; evaluated from `Draw()` (fresh per-pane width via `coordKickedFor`/`agentKickedFor`), never from `bindPane` itself (whose tracked width can still be 0 for a pane not yet shown, e.g. the agent pane during details mode); BUG-076 (false "Session not running" after ordinary rail nav) — root cause was `handleSessionExitUI`'s post-kick auto-restart gate checking ONLY `a.mode==modeAgent`, never true on the Hera tab, so every BUG-074 size-drift kick fired from a Hera pane genuinely stopped the session and then always skipped the restart as "user navigated away"; fixed via `App.isViewingTaskSession`/`HeraPage.IsBoundToTask` recognizing the Hera-tab-with-bound-pane case too; needs-input sharpened to suppress on sustained activity (narrow-needs-input-sustained-active: `RoleView.SustainedActive`, per-TASK not per-role so a dual-bound sub-coordinator's stale-blocked worker hat is suppressed by the coordinator hat's own sustained activity with no per-hat lookup; `needsInputOwn()` checks it first; fed via a NEW `agent.SustainedActivityTick` grace-tolerant sibling of `ResumeActivityTick`, not a change to `ResumeActivityTick` itself); kick-storm debounce (fix-hera-tick-and-kick-perf: `hera.KickDebounce` 300ms dwell before `maybeKickPaneRerender` actually fires, arm-then-fire `kickPending` per pane, re-arms on rebind-to-different-task, `bindPane` zeroes it on unbind) + tick change-detection gate (`HeraPage.shouldRebuild`/`markRebuilt` skip `BuildModel`+`buildRows` entirely when a cheap `PRAGMA data_version` fingerprint AND the 4 runtime maps are unchanged since the last rebuild — ~35,000x steady-state cost reduction measured via `doRefresh_bench_test.go`; `data_version`'s same-connection blind spot closed by `HeraPage.Refresh()` itself invalidating the gate, not just `heraRefresh`; base task-list reads and archived-row lazy-loading explicitly NOT covered, named follow-ups) | 175 | | [gotchas/messaging.md](gotchas/messaging.md) | task_messages caps (64 KiB / 500 unread / 50/min), self-send rejection, recipient existence check, reliable-notify delivery (single-writer, Ctrl+U pre-clear, CR not LF, 5-min deadline, ack-cancels), archive cleanup, task_ask polling, REST send; hera M2 (read_at NULL invariant, enqueue-time delivery stamp, hera: delivery-ID prefix, eager inbox cancel, doorbell trust boundary, worker-done-must-not-archive-role, hera_send auto-revive-on-send reuses ReviveRole verbatim + soft-fail never blocks delivery) | 34 | | [gotchas/remote-tui.md](gotchas/remote-tui.md) | `--remote URL --token` mode: apiclient + apistore architecture, two compile-time assertions, four TUI sites that type-assert to *db.DB for local-only ops, raw endpoints for full model.Task round-trip, 30s config refresher, daemon-admin actions that don't apply remotely | 13 | | [gotchas/events.md](gotchas/events.md) | Events ring + SSE stream: emission-outside-mu invariant, subscribe-before-snapshot fencing, sink save/restore in tests, ring eviction shape, idle watcher unconditional-run, task.completed double-emit, session.needs_input daemon-authoritative idle-gated sticky watcher, never-idle-parked-prompt flagged via content-stability fingerprint (BUG-032, streaming false-positive guard), emulated-screen detection for cursor-addressed alt-screen prompts (BUG-033, ScreenRenderer reuse-via-RIS, raw fast-path + emulate-on-miss), needs-input sticky flag clears on input-delivered-or-archive not signal-decay (BUG-034, shared agent.NeedsInputClear + needsInputSince baseline; clear filter reads LastUserInput NOT LastInput — system reliable-notify delivery uses WriteInputSystem so it never clears a parked worker's autonomous (?), the BUG-034 regression fix), never-idle pass flags free-text endsInQuestion gated on the working-affordance ("esc to interrupt") being ABSENT — content-stability ALONE re-breaks BUG-032 (BUG-035 GAP A, agent.AwaitingInputFingerprint replaces SelectionPromptFingerprint); selection matches any numbered option + wording-tolerant chooser footer (BUG-035 GAP B), content-aware idle for fullscreen agents (agent.ContentIdle emulated-screen stability + working-affordance gate, parallel to Session.IsIdle NOT folded into it; idle-push once-on-transition via shouldFireIdlePush cycle gate; RoleView.SessionIdle suppresses the rail spinner, BUG-036), never-converging content fingerprint escalates via a bounded consecutive-tick counter rather than loosening the chrome allowlist (BUG-029, agent.ParkedSelectionSignal + agent.EscalateParkedSelection, NeedsInputEscalationTicks=8, separate path from ContentFingerprint itself), escalation counter's original all-or-nothing reset was fragile against an isolated single-tick detection miss (blinking cursor glyph, or a torn read racing the daemon's concurrent log-file writer) — a genuinely, continuously-parked hera worker could never reach the threshold, explaining a live "first sibling flags reliably, later siblings under the same coordinator never do" repro; fixed via a one-tick grace period (negative-sentinel encoding, `escalated` stays true through an already-past-threshold grace tick to avoid flicker) rather than loosening detection itself (BUG-060), a fixed-size tail window can be PERMANENTLY (not just occasionally) flooded by Claude's blinking-cursor redraw until real content falls out of reach — deterministic 100%-miss confirmed via live repro, not a torn read (BUG-061, agent.SubstantiveTail expand-on-degenerate-tail read + degenerateSuffixStart raw-byte periodicity trim, wired into both the TUI disk-log read and the push watcher's ring-buffer read; sticky carry-forward in both detectNeedsInputSticky and computeNeedsInput no longer re-requires a fresh tail match, agent.NeedsInputClear is the only clear path), no hera adopt/reconcile loop on the ring, a cleared flag can be PERMANENTLY re-stuck by a stale re-candidacy after a candidacy gap (BUG-063, NeedsInputClear baseline forgotten the instant a task drops out of `candidates` even for one tick; fixed via a `running`-scoped cleared-marker (`prevCleared`/`newCleared`) that survives the gap and suppresses a same-timestamp re-candidacy; accepted scope limit: can't distinguish stale content from a genuinely distinct second prompt at the same timestamp), a hera coordinator's relayed answer (WriteInputSystem) could never clear the flag through BUG-034's own user-input path even after the worker demonstrably resumed real work (BUG-065, NeedsInputClear gained a third `resumedOf` clear condition fed by agent.ResumeActivityTick — mirrors EscalateParkedSelection but tracks sustained "working"-affordance ticks with no grace period on a miss, since under-clearing is safe but a false clear is not), a role's SELF-REPORTED hera_status="blocked" is a wholly separate signal ORed into the same rail (?) glyph (RoleView.needsInputOwn) with no auto-clear of its own — set only by an explicit hera_status tool call or manual s/S, so a direct pane reply never cleared it (BUG-066, agent.ClearBlockedRoleStatus: direct-reply-after-blockedAt clears immediately with no threshold, OR the same BUG-065 resumed-activity signal for a coordinator-relayed answer; db.ListBlockedHeraRoleBindings/ClearBlockedRoleStatus read/write split; App.autoClearBlockedHeraRoles + Server.autoClearBlockedHeraRoles run as a separate small pass scoped to the usually-empty blocked set, not folded into computeNeedsInput/detectNeedsInputSticky), BUG-063's own accepted scope limit resurfaces (and is fixed) in a multi-question AskUserQuestion/brainstorm flow — a SEPARATE, pre-existing bug from BUG-066, not a #904 regression, confirmed via a cross-task shared-ScreenRenderer test that disproves contamination (BUG-067, NeedsInputClear gained a fingerprintOf param + ClearedMarker{At,FP,HasFP} replacing the plain timestamp marker so a stale-recandidacy suppression additionally requires matching CONTENT, not just timestamp — a distinct later prompt at the identical lastInputOf timestamp now re-arms instead of being silently swallowed), a worker that resolves its own block and settles into idle FASTER than the resumed-activity threshold had NO clear path at all — stuck until an incidental keystroke (BUG-072, NeedsInputClear gained a fourth `settledOf` clear condition fed by agent.SettleTick — re-runs the SAME idle-gated signal check that raises the flag as a negative/clearing signal, gated on genuine Session.IsIdle() so it can never conflate with BUG-061's flooding hazard, small NeedsInputSettleTicks=2 threshold since idle rules out flooding by construction), ResumeActivityTick's zero-grace design proven (by its own new test) to never let a genuinely-but-burstily-active session cross its threshold at all — a NEW grace-tolerant sibling `agent.SustainedActivityTick` (one-tick grace mirroring BUG-060, own separate tick-counter, reuses NeedsInputResumeTicks) backs the Hera rail's `RoleView.SustainedActive` display gate ONLY, leaving ResumeActivityTick itself and its BUG-065/BUG-066 callers unchanged (narrow-needs-input-sustained-active) | 21 | diff --git a/internal/db/db.go b/internal/db/db.go index f7d6245d..f874486b 100644 --- a/internal/db/db.go +++ b/internal/db/db.go @@ -131,6 +131,27 @@ func (d *DB) WithTx(fn func(tx *sql.Tx) error) error { return tx.Commit() } +// DataVersion returns SQLite's PRAGMA data_version counter for this +// connection: a cheap (near-O(1)) fingerprint that changes whenever ANY +// OTHER connection commits a write to this database file (confirmed to work +// identically in WAL mode, which this DB always opens with). It is a +// cache-invalidation probe, not a general change feed: SQLite does NOT bump +// the value THIS connection reads back for a write made through itself (the +// documented same-connection blind spot — verified against this driver). +// Callers that also write through d must pair this with their own explicit +// invalidation for writes made via this connection rather than relying on +// the returned value alone. See gotchas/hera-view.md. +func (d *DB) DataVersion() (int64, error) { + d.mu.Lock() + defer d.mu.Unlock() + + var v int64 + if err := d.conn.QueryRow(`PRAGMA data_version`).Scan(&v); err != nil { + return 0, fmt.Errorf("data_version: %w", err) + } + return v, nil +} + // --- Helpers --- func formatTime(t time.Time) string { diff --git a/internal/db/db_test.go b/internal/db/db_test.go index 953050a5..8105d016 100644 --- a/internal/db/db_test.go +++ b/internal/db/db_test.go @@ -403,6 +403,56 @@ func TestDB_PruneCompleted_NoneToRemove(t *testing.T) { } } +// TestDB_DataVersion pins the cross-connection cache-invalidation contract +// DataVersion exists for (see gotchas/hera-view.md and +// openspec/changes/fix-hera-tick-and-kick-perf/design.md Decision 4): a write +// through a DIFFERENT connection to the same file bumps the value THIS +// connection reads, a write through THIS SAME connection does NOT (the +// documented same-connection blind spot — verified against modernc.org/sqlite +// under the exact WAL DSN Open uses), and the value is stable across repeated +// reads when nothing wrote in between. Uses two real file-backed *DB +// instances (t.TempDir()-backed, never touching real ~/.argus/) since +// OpenInMemory's `:memory:` DSN gives each connection its own private +// database — cross-connection visibility can't be exercised there. +func TestDB_DataVersion(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "data.sql") + + d1, err := Open(path) + testutil.NoError(t, err) + defer func() { _ = d1.Close() }() + d2, err := Open(path) + testutil.NoError(t, err) + defer func() { _ = d2.Close() }() + + before, err := d2.DataVersion() + testutil.NoError(t, err) + + testutil.NoError(t, d1.Add(&model.Task{Name: "t1"})) + + after, err := d2.DataVersion() + testutil.NoError(t, err) + if after == before { + t.Fatalf("expected data_version to change after a cross-connection write, both = %d", before) + } + + // Stable across repeated reads with no intervening write. + stable1, err := d2.DataVersion() + testutil.NoError(t, err) + stable2, err := d2.DataVersion() + testutil.NoError(t, err) + testutil.Equal(t, stable1, stable2) + + // Same-connection blind spot: d1's own next write does not change what + // d1 itself reads back. + sameBefore, err := d1.DataVersion() + testutil.NoError(t, err) + testutil.NoError(t, d1.Add(&model.Task{Name: "t2"})) + sameAfter, err := d1.DataVersion() + testutil.NoError(t, err) + testutil.Equal(t, sameAfter, sameBefore) +} + func TestDB_TaskPersistence(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "data.sql") diff --git a/internal/tui/app.go b/internal/tui/app.go index 5ecee91a..0760547d 100644 --- a/internal/tui/app.go +++ b/internal/tui/app.go @@ -2559,6 +2559,10 @@ func (a *App) refreshTasksWithIDs(runningIDs, idleIDs []string) { return } a.tasks = tasks + // Feed the Hera rail's own model rebuild from the snapshot just fetched + // above, so it doesn't pay for a second, redundant full-table fetch of + // the same data (see gotchas/hera-view.md). + a.heraPage.SetTasks(a.tasks) // Snapshot the previous needs-input set before we overwrite it. The // sticky pass below uses this to carry forward detections that have // fallen out of idleIDs — Claude's prompt UI emits periodic animation diff --git a/internal/tui/hera/changegate_test.go b/internal/tui/hera/changegate_test.go new file mode 100644 index 00000000..e943a00a --- /dev/null +++ b/internal/tui/hera/changegate_test.go @@ -0,0 +1,238 @@ +package hera + +import ( + "path/filepath" + "testing" + + "github.com/drn/argus/internal/db" + "github.com/drn/argus/internal/model" + "github.com/drn/argus/internal/testutil" +) + +// openFileDB opens a real, file-backed *db.DB in a fresh t.TempDir() — never +// touching real ~/.argus/. Unlike memDB's in-memory connection, a file-backed +// DB can be opened MULTIPLE times against the same path, which is required to +// exercise a genuine cross-connection PRAGMA data_version change (an +// in-memory `:memory:` DSN gives each connection its own private database). +func openFileDB(t *testing.T) *db.DB { + t.Helper() + path := filepath.Join(t.TempDir(), "data.sql") + d, err := db.Open(path) + testutil.NoError(t, err) + t.Cleanup(func() { _ = d.Close() }) + return d +} + +// noFingerprintReader wraps a HeraReader so its OWN method set is exactly +// HeraReader's declared methods — DataVersion is NOT promoted even though the +// wrapped value implements it (Go only promotes an embedded INTERFACE +// field's declared methods, never the dynamic value's full method set beyond +// that interface). Mirrors the remote-mode nil reader and any HeraReader test +// double that never grows a DataVersion method. +type noFingerprintReader struct{ HeraReader } + +// TestHeraPage_ShouldRebuild_FirstCallAlwaysTrue: no prior snapshot exists yet. +func TestHeraPage_ShouldRebuild_FirstCallAlwaysTrue(t *testing.T) { + p := NewHeraPage(openFileDB(t)) + testutil.Equal(t, p.shouldRebuild(), true) +} + +// TestHeraPage_ShouldRebuild_QuiescentSkipsAfterMarkRebuilt proves the core +// gate: once markRebuilt has snapshotted a rebuild, a second call with +// nothing changed reports false. +func TestHeraPage_ShouldRebuild_QuiescentSkipsAfterMarkRebuilt(t *testing.T) { + p := NewHeraPage(openFileDB(t)) + testutil.Equal(t, p.shouldRebuild(), true) + p.markRebuilt() + testutil.Equal(t, p.shouldRebuild(), false) + // Repeated calls with nothing changed stay false. + testutil.Equal(t, p.shouldRebuild(), false) +} + +// TestHeraPage_ShouldRebuild_DBFingerprintChangeTriggersRebuild proves a +// cross-connection DB write (a genuinely different data_version, the kind a +// daemon-driven hera mutation would produce) flips the gate back to true. +func TestHeraPage_ShouldRebuild_DBFingerprintChangeTriggersRebuild(t *testing.T) { + path := filepath.Join(t.TempDir(), "data.sql") + reader, err := db.Open(path) + testutil.NoError(t, err) + defer func() { _ = reader.Close() }() + writer, err := db.Open(path) + testutil.NoError(t, err) + defer func() { _ = writer.Close() }() + + p := NewHeraPage(reader) + testutil.Equal(t, p.shouldRebuild(), true) + p.markRebuilt() + testutil.Equal(t, p.shouldRebuild(), false) + + testutil.NoError(t, writer.Add(&model.Task{Name: "cross-conn"})) + + testutil.Equal(t, p.shouldRebuild(), true) +} + +// TestHeraPage_ShouldRebuild_RuntimeMapChangesTriggerRebuild proves each of +// the four per-tick runtime maps, changed INDIVIDUALLY with the DB +// fingerprint held stable, flips the gate — DB-only gating would freeze the +// rail's spinner/needs-input glyphs whenever the DB itself is quiet. +func TestHeraPage_ShouldRebuild_RuntimeMapChangesTriggerRebuild(t *testing.T) { + cases := []struct { + name string + apply func(p *HeraPage) + }{ + {"needsInput", func(p *HeraPage) { p.SetNeedsInput([]string{"t1"}) }}, + {"sessionIdle", func(p *HeraPage) { p.SetSessionIdle([]string{"t1"}) }}, + {"sessionRunning", func(p *HeraPage) { p.SetSessionRunning([]string{"t1"}) }}, + {"sustainedActive", func(p *HeraPage) { p.SetSustainedActive([]string{"t1"}) }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p := NewHeraPage(openFileDB(t)) + testutil.Equal(t, p.shouldRebuild(), true) + p.markRebuilt() + testutil.Equal(t, p.shouldRebuild(), false) + + tc.apply(p) + + testutil.Equal(t, p.shouldRebuild(), true) + p.markRebuilt() + testutil.Equal(t, p.shouldRebuild(), false) + }) + } +} + +// TestHeraPage_ShouldRebuild_UnsupportedFingerprintAlwaysRebuilds proves a +// reader without a DataVersion method (any HeraReader test double, or the +// remote-mode nil reader) is always treated as "changed" — the gate must +// never suppress a rebuild it cannot prove is safe to skip. +func TestHeraPage_ShouldRebuild_UnsupportedFingerprintAlwaysRebuilds(t *testing.T) { + p := NewHeraPage(noFingerprintReader{HeraReader: memDB(t)}) + testutil.Equal(t, p.shouldRebuild(), true) + p.markRebuilt() + testutil.Equal(t, p.shouldRebuild(), true) // still true — nothing "proved" safe +} + +// TestHeraPage_ShouldRebuild_RemoteModeAlwaysRebuilds: a nil reader (remote +// mode) has no fingerprint either, and must degrade identically. +func TestHeraPage_ShouldRebuild_RemoteModeAlwaysRebuilds(t *testing.T) { + p := NewHeraPage(nil) + testutil.Equal(t, p.shouldRebuild(), true) + p.markRebuilt() + testutil.Equal(t, p.shouldRebuild(), true) +} + +// TestHeraPage_InvalidateChangeGate proves InvalidateChangeGate forces the +// NEXT shouldRebuild call to report true even with a stable fingerprint and +// unchanged runtime maps — the mechanism Refresh() uses to guarantee its own +// "forces an immediate rebuild" contract regardless of the gate. +func TestHeraPage_InvalidateChangeGate(t *testing.T) { + p := NewHeraPage(openFileDB(t)) + testutil.Equal(t, p.shouldRebuild(), true) + p.markRebuilt() + testutil.Equal(t, p.shouldRebuild(), false) + + p.InvalidateChangeGate() + + testutil.Equal(t, p.shouldRebuild(), true) +} + +// TestHeraPage_Refresh_ForcesRebuildDespiteSameConnectionBlindSpot is the +// end-to-end regression for design.md Decision 5: a hera mutation written +// through the SAME connection this page reads from does NOT bump the +// fingerprint as this page's own next read reports it (the documented +// same-connection blind spot, confirmed directly below) — yet Refresh() +// still performs a full rebuild that picks it up, because Refresh() itself +// invalidates the gate before flushing. +func TestHeraPage_Refresh_ForcesRebuildDespiteSameConnectionBlindSpot(t *testing.T) { + d := openFileDB(t) + p := NewHeraPage(d) + p.Refresh() + before := len(p.Rail().Model().Active) + len(p.Rail().Model().Pinned) + testutil.Equal(t, before, 0) + + fpBefore := p.lastFingerprint + seedBoundRole(t, d, seedOrch(t, d, "orch"), "coord", db.HeraKindCoordinator, "t-coord") + + // Confirm the blind spot is real for this exact scenario: reading the + // fingerprint back through the SAME connection (d, == p.reader) does NOT + // show the write that connection itself just made. + fpAfter, ok := p.dbFingerprint() + testutil.Equal(t, ok, true) + testutil.Equal(t, fpAfter, fpBefore) + + // Refresh() must still pick up the change. + p.Refresh() + after := len(p.Rail().Model().Active) + len(p.Rail().Model().Pinned) + testutil.Equal(t, after >= 1, true) +} + +// coordRoleView finds the "coord"-named role in m's first Active orchestrator +// (test helper for asserting on rebuilt-model content). +func coordRoleView(t *testing.T, m Model) RoleView { + t.Helper() + for _, rv := range m.Active[0].Roles { + if rv.Name == "coord" { + return rv + } + } + t.Fatal("coord role not found in rebuilt model") + return RoleView{} +} + +// TestHeraPage_DoRefresh_SkipsRebuildWhenQuiescent proves doRefresh itself +// (not just the shouldRebuild unit) honors the gate: calling it again with +// nothing changed does not re-run BuildModel (the rail's model keeps the +// role's NeedsInput=false it was built with), while a genuine runtime-map +// change (needsInput) is picked up on the very next call. +func TestHeraPage_DoRefresh_SkipsRebuildWhenQuiescent(t *testing.T) { + d := openFileDB(t) + orch := seedOrch(t, d, "orch") + seedBoundRole(t, d, orch, "coord", db.HeraKindCoordinator, "t-coord") + + p := NewHeraPage(d) + p.Refresh() + testutil.Equal(t, coordRoleView(t, p.Rail().Model()).NeedsInput, false) + + // Flag the coordinator's task as needing input, but do NOT tell the page + // (SetNeedsInput not called) — doRefresh should skip the rebuild entirely + // since nothing IT knows about changed, so the rail's model still shows + // the stale (but at-last-rebuild-accurate) NeedsInput=false. + p.doRefresh() + testutil.Equal(t, coordRoleView(t, p.Rail().Model()).NeedsInput, false) + + // Now tell the page — the very next doRefresh picks it up. + p.SetNeedsInput([]string{"t-coord"}) + p.doRefresh() + testutil.Equal(t, coordRoleView(t, p.Rail().Model()).NeedsInput, true) +} + +// countingTasksReader wraps a HeraReader and counts calls to Tasks(), so +// tests can assert whether BuildModel's Tasks() call actually reached the +// underlying store or was served from a supplied snapshot instead. +type countingTasksReader struct { + HeraReader + calls int +} + +func (r *countingTasksReader) Tasks() ([]*model.Task, error) { + r.calls++ + return r.HeraReader.Tasks() +} + +// TestHeraPage_SetTasks_AvoidsRedundantFetch proves doRefresh serves +// BuildModel's Tasks() call from a snapshot supplied via SetTasks instead of +// hitting the underlying reader a second time, when one has been supplied — +// and falls back to the reader's own fetch, unchanged, when it hasn't. +func TestHeraPage_SetTasks_AvoidsRedundantFetch(t *testing.T) { + d := openFileDB(t) + testutil.NoError(t, d.Add(&model.Task{Name: "t1"})) + counting := &countingTasksReader{HeraReader: d} + + p := NewHeraPage(counting) + p.Refresh() // no SetTasks call yet — falls back to the reader's own Tasks() + testutil.Equal(t, counting.calls, 1) + + p.SetTasks([]*model.Task{{ID: "supplied", Name: "supplied"}}) + p.Refresh() + testutil.Equal(t, counting.calls, 1) // unchanged — served from the snapshot instead +} diff --git a/internal/tui/hera/doRefresh_bench_test.go b/internal/tui/hera/doRefresh_bench_test.go new file mode 100644 index 00000000..fd6c7810 --- /dev/null +++ b/internal/tui/hera/doRefresh_bench_test.go @@ -0,0 +1,98 @@ +package hera + +import ( + "fmt" + "testing" + "time" + + "github.com/drn/argus/internal/db" + "github.com/drn/argus/internal/model" +) + +// seedBoundRoleForBench mirrors seedBoundRole (model_test.go) but takes +// testing.TB so it's usable from a *testing.B — kept separate rather than +// widening seedBoundRole's signature, which many existing *testing.T tests +// already call. +func seedBoundRoleForBench(tb testing.TB, d *db.DB, orchID int64, name string, kind db.HeraRoleKind, taskID string) { + tb.Helper() + role, err := d.CreateHeraRole(db.CreateHeraRoleInput{OrchestratorID: orchID, Name: name, Kind: kind, ArgusProject: "p"}) + if err != nil { + tb.Fatal(err) + } + if err := d.Add(&model.Task{ID: taskID, Name: taskID, Status: model.StatusInProgress, Project: "p", CreatedAt: time.Now()}); err != nil { + tb.Fatal(err) + } + if _, err := d.CreateHeraBinding(db.CreateHeraBindingInput{RoleID: role.ID, ArgusTaskID: taskID, WorktreePath: "/wt/" + taskID}); err != nil { + tb.Fatal(err) + } +} + +// seedLargeHistory populates an in-memory DB with numOrchs archived +// orchestrators (each with a coordinator + worker role bound to distinct +// tasks) — approximating Aaron's real dogfood scale (~900 historical +// orchestrators/roles/bindings, per the fix-hera-tick-and-kick-perf mission) +// — plus one small ACTIVE orchestrator so the model isn't degenerate. Kept in +// this package (not a throwaway script) so the measurement in +// openspec/changes/fix-hera-tick-and-kick-perf/tasks.md §5 is reproducible +// via `go test -bench` rather than a one-off transcript. +func seedLargeHistory(b *testing.B, numOrchs int) *db.DB { + b.Helper() + d, err := db.OpenInMemory() + if err != nil { + b.Fatal(err) + } + b.Cleanup(func() { _ = d.Close() }) + for i := 0; i < numOrchs; i++ { + name := fmt.Sprintf("archived-orch-%d", i) + o, err := d.CreateHeraOrchestrator(name, "") + if err != nil { + b.Fatal(err) + } + coordTask := fmt.Sprintf("t-coord-%d", i) + wkrTask := fmt.Sprintf("t-wkr-%d", i) + seedBoundRoleForBench(b, d, o.ID, "coord", db.HeraKindCoordinator, coordTask) + seedBoundRoleForBench(b, d, o.ID, "wkr", db.HeraKindWorker, wkrTask) + if err := d.ArchiveHeraOrchestrator(o.ID); err != nil { + b.Fatal(err) + } + } + // One small active orchestrator so the model isn't entirely archived — + // mirrors Aaron's "5-6 active agents" alongside the large history. + active, err := d.CreateHeraOrchestrator("active-orch", "") + if err != nil { + b.Fatal(err) + } + seedBoundRoleForBench(b, d, active.ID, "coord", db.HeraKindCoordinator, "t-active-coord") + seedBoundRoleForBench(b, d, active.ID, "wkr", db.HeraKindWorker, "t-active-wkr") + return d +} + +// BenchmarkDoRefresh_AlwaysRebuild reproduces the cost of the PRE-FIX +// behavior: BuildModel+SetModel ran unconditionally on every tick opportunity +// regardless of whether anything changed. InvalidateChangeGate before each +// iteration forces exactly that. +func BenchmarkDoRefresh_AlwaysRebuild(b *testing.B) { + d := seedLargeHistory(b, 450) // ~900 roles + ~900 bindings, matching Aaron's reported ~900-row scale + p := NewHeraPage(d) + p.Refresh() + b.ResetTimer() + for i := 0; i < b.N; i++ { + p.InvalidateChangeGate() + p.doRefresh() + } +} + +// BenchmarkDoRefresh_SteadyStateGated reproduces the cost of the POST-FIX +// behavior with nothing changing between rebuild opportunities: only the +// FIRST doRefresh call (folded into setup, excluded via ResetTimer after one +// warm call) pays the full cost; every subsequent call should be the +// near-zero shouldRebuild() check alone. +func BenchmarkDoRefresh_SteadyStateGated(b *testing.B) { + d := seedLargeHistory(b, 450) + p := NewHeraPage(d) + p.Refresh() // pay the one real rebuild before timing starts + b.ResetTimer() + for i := 0; i < b.N; i++ { + p.doRefresh() // nothing changed — should be gated to a near-instant skip + } +} diff --git a/internal/tui/hera/page.go b/internal/tui/hera/page.go index 465596e7..8208b8f4 100644 --- a/internal/tui/hera/page.go +++ b/internal/tui/hera/page.go @@ -1,6 +1,10 @@ package hera import ( + "maps" + "time" + + "github.com/drn/argus/internal/model" "github.com/drn/argus/internal/tui/keymap" "github.com/drn/argus/internal/tui/planview" "github.com/drn/argus/internal/tui/terminal" @@ -99,6 +103,31 @@ type HeraPage struct { // for a role whose bound task has demonstrated several consecutive ticks of // genuine activity (narrow-needs-input-sustained-active). sustainedActive map[string]bool + // tasks is the App's already-fetched full task snapshot this tick + // (App.tasks), set via SetTasks. doRefresh wraps the reader with it so + // BuildModel's Tasks() call is served from here instead of a second, + // redundant fetch of the same data refreshTasksWithIDs already performed + // this tick — see gotchas/hera-view.md. tasksKnown distinguishes "never + // supplied" (nil tasks is ambiguous with a genuinely empty task list) so + // a caller/test that never calls SetTasks still gets BuildModel's + // original self-fetch behavior, unchanged. + tasks []*model.Task + tasksKnown bool + + // --- tick change-detection gate (fix-hera-tick-and-kick-perf) --- + // + // fpKnown/lastFingerprint/lastNeedsInput/lastSessionIdle/ + // lastSessionRunning/lastSustainedActive cache the state observed at the + // LAST full rebuild, so shouldRebuild can cheaply prove "nothing that + // could affect the rendered rail has changed" and doRefresh can skip + // BuildModel+SetModel entirely. See shouldRebuild/markRebuilt and + // design.md Decision 4. + fpKnown bool + lastFingerprint int64 + lastNeedsInput map[string]bool + lastSessionIdle map[string]bool + lastSessionRunning map[string]bool + lastSustainedActive map[string]bool // tierResolver stamps the diligence-tiering readout (AppliedModel/Effort + // ProfileWarning) onto each RoleView during doRefresh. The App wires it (local @@ -133,6 +162,13 @@ type HeraPage struct { // its size-drift kick evaluated this binding — see maybeKickPaneRerender. coordKickedFor string agentKickedFor string + // coordKickPending / agentKickPending are the armed-but-unfired kick- + // debounce state per pane (see kickPending, maybeKickPaneRerender). + coordKickPending kickPending + agentKickPending kickPending + // kickNow overrides the kick-debounce clock in tests (SetKickClock); nil + // in production, where kickClockNow falls back to time.Now. + kickNow func() time.Time // 6c mutation callbacks. The rail-focus key handler maps keys to these, // passing the current Selection (the multi-binding-disambiguated (role,orch) @@ -504,6 +540,31 @@ func (p *HeraPage) SetSustainedActive(ids []string) { p.sustainedActive = m } +// SetTasks records the App's already-fetched full task snapshot for this +// tick (App.tasks — the same list refreshTasksWithIDs fetched moments ago for +// the plain task list). doRefresh reuses it instead of paying for a second, +// redundant full-table fetch inside BuildModel. Pure setter; MUST run on the +// tview thread. Never calling this (remote mode, or a test that constructs a +// HeraPage without wiring it) leaves BuildModel's own self-fetch behavior +// completely unchanged. +func (p *HeraPage) SetTasks(tasks []*model.Task) { + p.tasks = tasks + p.tasksKnown = true +} + +// tasksReader wraps a HeraReader and serves a pre-fetched task snapshot from +// Tasks() instead of the wrapped reader's own fetch — see SetTasks. Every +// other HeraReader method passes through unchanged via interface embedding. +// Constructed transiently at the BuildModel call site (never stored back +// into HeraPage.reader itself), so it never interferes with dbFingerprint's +// type-assertion against the original, unwrapped reader. +type tasksReader struct { + HeraReader + tasks []*model.Task +} + +func (r *tasksReader) Tasks() ([]*model.Task, error) { return r.tasks, nil } + // SetClipboardHint toggles whether the focused terminal pane advertises a // staged agent clipboard payload via a `(ctrl+y copy)` border-title affordance. // The App refreshes it each tick from the daemon for the focused pane's task @@ -524,8 +585,16 @@ func (p *HeraPage) ClipboardHint() bool { return p.clipReady } func (p *HeraPage) ScheduleRefresh() { p.refresher.Schedule() } // Refresh forces an immediate rail rebuild (used on tab entry so the rail is -// fresh the instant the tab opens). MUST run on the tview thread. +// fresh the instant the tab opens, and by every hera-mutation handler via +// App.heraRefresh). "Forces" means it — InvalidateChangeGate runs first, so +// doRefresh's change-detection gate (see shouldRebuild) never silently turns +// this into a no-op, even for a caller whose preceding write went through the +// SAME store connection this page reads from (PRAGMA data_version's +// documented same-connection blind spot — see design.md Decision 5). Callers +// that only want a rebuild WHEN something changed should call ScheduleRefresh +// instead, which is subject to the gate. MUST run on the tview thread. func (p *HeraPage) Refresh() { + p.InvalidateChangeGate() p.refresher.Schedule() p.refresher.Flush() } @@ -534,7 +603,17 @@ func (p *HeraPage) Refresh() { // remote mode the reader is nil → BuildModel returns an empty model and Draw // renders the unavailable banner, so this stays a cheap no-op. func (p *HeraPage) doRefresh() { - m, err := BuildModel(p.reader, p.needsInput, p.sessionIdle, p.sessionRunning, p.sustainedActive) + if !p.shouldRebuild() { + uxlog.Log("[hera-view] rail refresh skipped: no change since last rebuild (fingerprint=%d)", p.lastFingerprint) + return + } + start := time.Now() + + reader := p.reader + if reader != nil && p.tasksKnown { + reader = &tasksReader{HeraReader: reader, tasks: p.tasks} + } + m, err := BuildModel(reader, p.needsInput, p.sessionIdle, p.sessionRunning, p.sustainedActive) if err != nil { uxlog.Log("[hera-view] rail refresh failed: %v", err) return @@ -561,10 +640,89 @@ func (p *HeraPage) doRefresh() { // pointers are stale — re-derive and rebind (task IDs usually unchanged, so // bindPane is a no-op and the emulators are preserved). p.applySelection() - uxlog.Log("[hera-view] rail refreshed: pinned=%d active=%d archived=%d freelance=%d (remote=%v)", - len(m.Pinned), len(m.Active), len(m.Archived), len(m.Freelance), p.remote) + p.markRebuilt() + uxlog.Log("[hera-view] rail refreshed: pinned=%d active=%d archived=%d freelance=%d (remote=%v) took=%s", + len(m.Pinned), len(m.Active), len(m.Archived), len(m.Freelance), p.remote, time.Since(start).Round(time.Microsecond)) } +// dataVersioner is implemented by *db.DB (checked via a type assertion, not +// added to the HeraReader interface itself, so test fakes and the remote nil +// reader are unaffected). See db.DB.DataVersion. +type dataVersioner interface { + DataVersion() (int64, error) +} + +// dbFingerprint returns the underlying store's cheap change-fingerprint, or +// (0, false) when the reader doesn't support one (remote mode's nil reader, +// or a test HeraReader fake) — shouldRebuild always treats that as "changed" +// so the gate never suppresses a rebuild it cannot prove is safe to skip. +func (p *HeraPage) dbFingerprint() (int64, bool) { + dv, ok := p.reader.(dataVersioner) + if !ok { + return 0, false + } + v, err := dv.DataVersion() + if err != nil { + return 0, false + } + return v, true +} + +// shouldRebuild reports whether doRefresh's full BuildModel+SetModel pass is +// worth paying for: true on the very first call (no prior snapshot to compare +// against), whenever the reader's cheap DB fingerprint is unsupported or has +// moved since the last rebuild, or whenever any of the four per-tick runtime +// maps that also feed the model (needsInput/sessionIdle/sessionRunning/ +// sustainedActive) differs from the snapshot taken at the last rebuild. Those +// four are bounded by LIVE/ACTIVE session count, not total history, so +// comparing them is cheap even when the DB fingerprint is stable — gating on +// DB state alone would freeze the rail's spinner/needs-input glyphs whenever +// the DB is quiet but agents are still actively producing output (see +// gotchas/hera-view.md and design.md Decision 4). +func (p *HeraPage) shouldRebuild() bool { + if !p.fpKnown { + return true + } + fp, hasFP := p.dbFingerprint() + if !hasFP || fp != p.lastFingerprint { + return true + } + return !maps.Equal(p.needsInput, p.lastNeedsInput) || + !maps.Equal(p.sessionIdle, p.lastSessionIdle) || + !maps.Equal(p.sessionRunning, p.lastSessionRunning) || + !maps.Equal(p.sustainedActive, p.lastSustainedActive) +} + +// markRebuilt snapshots the state a just-completed rebuild was based on, for +// the NEXT shouldRebuild call to compare against. The four runtime maps are +// stored by reference, not deep-cloned: every SetXxx setter always assigns a +// freshly-allocated map (or nil) rather than mutating one in place (confirmed +// by reading all four), so the map this rebuild read is never subsequently +// mutated out from under the stored snapshot — the NEXT tick's setter call +// replaces p.needsInput etc. with an entirely new map object instead. +func (p *HeraPage) markRebuilt() { + p.fpKnown = true + if fp, ok := p.dbFingerprint(); ok { + p.lastFingerprint = fp + } + p.lastNeedsInput = p.needsInput + p.lastSessionIdle = p.sessionIdle + p.lastSessionRunning = p.sessionRunning + p.lastSustainedActive = p.sustainedActive +} + +// InvalidateChangeGate forces the NEXT shouldRebuild call to return true +// regardless of what the DB fingerprint reports. PRAGMA data_version (the +// fingerprint source) does not change what THIS connection reads back after +// a write made through itself (SQLite's documented same-connection blind +// spot — see db.DB.DataVersion) — App.heraRefresh calls this right alongside +// its existing forced Refresh() after every interactive hera mutation, so a +// local write is never silently missed by the gate on the next tick, even +// though the mutating user already sees their own change instantly via that +// same forced Refresh() (which bypasses this gate entirely — see design.md +// Decision 5). +func (p *HeraPage) InvalidateChangeGate() { p.fpKnown = false } + // Draw computes the three-region layout and paints each region, covering the // full bounding rect (DrawBorderedPanel / FillArea) so no stale cells survive — // per the CLAUDE.md UX-rendering rules (no Sync; full-rect coverage instead). @@ -641,7 +799,7 @@ func (p *HeraPage) Draw(screen tcell.Screen) { p.agentX, p.agentW = rx+rw, 0 p.coordPane.SetFocused(true) p.coordPane.SetRect(rx, y, rw, h) - p.maybeKickPaneRerender(p.coordBound, &p.coordKickedFor, rw) + p.maybeKickPaneRerender(p.coordBound, &p.coordKickedFor, rw, &p.coordKickPending) p.coordPane.Draw(screen) case FocusAgent: p.agentX, p.agentW = rx, rw @@ -651,7 +809,7 @@ func (p *HeraPage) Draw(screen tcell.Screen) { } else { p.agentPane.SetFocused(true) p.agentPane.SetRect(p.agentX, y, rw, h) - p.maybeKickPaneRerender(p.agentBound, &p.agentKickedFor, rw) + p.maybeKickPaneRerender(p.agentBound, &p.agentKickedFor, rw, &p.agentKickPending) p.agentPane.Draw(screen) } } @@ -666,7 +824,7 @@ func (p *HeraPage) Draw(screen tcell.Screen) { if coordW >= 2 { p.coordPane.SetFocused(p.focus.State() == FocusCoord) p.coordPane.SetRect(rx, y, coordW, h) - p.maybeKickPaneRerender(p.coordBound, &p.coordKickedFor, coordW) + p.maybeKickPaneRerender(p.coordBound, &p.coordKickedFor, coordW, &p.coordKickPending) p.coordPane.Draw(screen) } if agentW >= 2 { @@ -675,7 +833,7 @@ func (p *HeraPage) Draw(screen tcell.Screen) { } else { p.agentPane.SetFocused(p.focus.State() == FocusAgent) p.agentPane.SetRect(p.agentX, y, agentW, h) - p.maybeKickPaneRerender(p.agentBound, &p.agentKickedFor, agentW) + p.maybeKickPaneRerender(p.agentBound, &p.agentKickedFor, agentW, &p.agentKickPending) p.agentPane.Draw(screen) } } diff --git a/internal/tui/hera/panes.go b/internal/tui/hera/panes.go index 45e94391..87cc856a 100644 --- a/internal/tui/hera/panes.go +++ b/internal/tui/hera/panes.go @@ -1,6 +1,8 @@ package hera import ( + "time" + "github.com/drn/argus/internal/app/agentview" "github.com/drn/argus/internal/tui/keyenc" "github.com/drn/argus/internal/tui/terminal" @@ -96,16 +98,16 @@ func (p *HeraPage) applySelection() { // orchestrator's coordinator (== the sub-coord's own session for a bridge row), // for a worker it is the selected orchestrator's coordinator. if p.detailsMode { - p.bindPane(p.coordPane, &p.coordBound, &p.coordKickedFor, detailsOrch.CoordTaskID(), "coord") - p.bindPane(p.agentPane, &p.agentBound, &p.agentKickedFor, "", "agent") + p.bindPane(p.coordPane, &p.coordBound, &p.coordKickedFor, &p.coordKickPending, detailsOrch.CoordTaskID(), "coord") + p.bindPane(p.agentPane, &p.agentBound, &p.agentKickedFor, &p.agentKickPending, "", "agent") p.details.SetOrch(detailsOrch, p.prMeta) // The Details region stacks the roster over the plan graph, so reproject // this coordinator's plan DAG on every selection (the roster reads straight // from the model; the plan widget needs the scoped node/edge set rebuilt). p.rebuildPlan(detailsOrch) } else { - p.bindPane(p.coordPane, &p.coordBound, &p.coordKickedFor, p.sel.CoordTaskID(), "coord") - p.bindPane(p.agentPane, &p.agentBound, &p.agentKickedFor, p.sel.TaskID(), "agent") + p.bindPane(p.coordPane, &p.coordBound, &p.coordKickedFor, &p.coordKickPending, p.sel.CoordTaskID(), "coord") + p.bindPane(p.agentPane, &p.agentBound, &p.agentKickedFor, &p.agentKickPending, p.sel.TaskID(), "agent") } } @@ -147,8 +149,13 @@ func (p *HeraPage) detailsOrch() *OrchView { // task the pane currently shows. kickedFor tracks which bound taskID has // already had its size-drift kick evaluated (see maybeKickPaneRerender) — // reset here on unbind so a later rebind to the SAME task gets a fresh -// evaluation rather than being silently skipped by a stale marker. -func (p *HeraPage) bindPane(tp *terminal.TerminalPane, bound, kickedFor *string, taskID, label string) { +// evaluation rather than being silently skipped by a stale marker. pending is +// the paired kick-debounce state (see kickPending); it is ALSO reset to its +// zero value on unbind — without this, a rebind to the SAME task after an +// unbind would compare equal to the stale pending.taskID and keep the OLD +// (possibly long-past) deadline, firing the kick immediately instead of +// dwelling afresh. +func (p *HeraPage) bindPane(tp *terminal.TerminalPane, bound, kickedFor *string, pending *kickPending, taskID, label string) { if *bound == taskID { return } @@ -159,6 +166,7 @@ func (p *HeraPage) bindPane(tp *terminal.TerminalPane, bound, kickedFor *string, tp.SetSession(nil) *bound = "" *kickedFor = "" + *pending = kickPending{} return } var sess agentview.TerminalAdapter @@ -203,6 +211,31 @@ func (p *HeraPage) bindPane(tp *terminal.TerminalPane, bound, kickedFor *string, *bound = taskID } +// KickDebounce is the wall-clock dwell maybeKickPaneRerender waits, once a +// bound pane's width first crosses agent.RerenderMargin, before it actually +// invokes the wired RerenderKicker. Hera's own layout swings a coordinator/ +// agent pane between full-width (fullscreen/Details mode) and roughly +// half-width (split mode) on ORDINARY RAIL NAV ALONE — no fullscreen toggle +// or terminal resize needed — so crossing the margin on every hop of a rapid +// Cmd+Arrow traversal binds several distinct tasks in quick succession. Each +// kick is a real Session.Stop()+restart+full-conversation replay, so without +// a dwell a fast multi-row traversal bursts many of these back to back (the +// "kick storm" — see gotchas/hera-view.md). 300ms is comfortably longer than +// a single keystroke-to-Draw round trip yet short enough that a genuine +// dwell-and-stay still kicks promptly. +const KickDebounce = 300 * time.Millisecond + +// kickPending tracks an armed-but-unfired size-drift kick candidate for one +// pane (HeraPage.coordKickPending / agentKickPending). The zero value means +// nothing is pending. cols is updated on every Draw so the eventual kick (if +// it fires) uses the LATEST observed width, not the width from the moment +// the dwell first armed. +type kickPending struct { + taskID string + cols int + deadline time.Time +} + // maybeKickPaneRerender evaluates the shared size-drift kill+resume decision // (see RerenderKicker) for a pane's currently bound task, using cols from the // rect Draw() JUST set on it. Called from each of Draw's four SetRect+Draw @@ -210,25 +243,60 @@ func (p *HeraPage) bindPane(tp *terminal.TerminalPane, bound, kickedFor *string, // SetRect, so cols is always fresh — never bindPane's own possibly-stale // tracked width (see bindPane's doc comment for why). // +// The actual kickRerender call is debounced by KickDebounce (see its doc +// comment): the first call that sees a newly-bound task (pending.taskID != +// bound) arms pending with a fresh deadline instead of firing immediately. +// Only a LATER call, once the dwell has elapsed AND the SAME task is still +// bound, actually invokes kickRerender. A rebind to a DIFFERENT task before +// the dwell elapses re-arms pending against the new task, discarding the old +// one un-fired — this is what suppresses the kick storm from a fast rail +// traversal. bindPane ALSO resets pending to its zero value on unbind: without +// that, a rebind to the SAME task after an intervening unbind would compare +// equal to the stale pending.taskID and keep the OLD deadline, firing +// immediately instead of dwelling afresh — caught by +// TestPanes_KickDebounce_UnbindMidDwellThenRebindSameTask (see design.md +// Decision 2). +// // kickedFor is a per-pane marker (HeraPage.coordKickedFor / agentKickedFor) -// preventing a redundant call every frame while the SAME task stays bound and -// visible; it is NOT the correctness gate against re-kicking (that is -// App.isRedundantAttach's job, keyed by task ID and width) — it only avoids -// paying for a DB lookup + runner.Get on every Draw. bindPane resets it to "" -// on unbind, so a later rebind to the same task still gets a fresh -// evaluation. -func (p *HeraPage) maybeKickPaneRerender(bound string, kickedFor *string, cols int) { +// preventing a redundant evaluation every frame once the kick has actually +// fired for the current bind; it is NOT the correctness gate against +// re-kicking (that is App.isRedundantAttach's job, keyed by task ID and +// width) — it only avoids paying for a DB lookup + runner.Get on every Draw +// after the kick already fired. bindPane resets it to "" on unbind, so a +// later rebind to the same task still gets a fresh evaluation. +func (p *HeraPage) maybeKickPaneRerender(bound string, kickedFor *string, cols int, pending *kickPending) { if p.kickRerender == nil || bound == "" || cols <= 0 || *kickedFor == bound { return } + now := p.kickClockNow() + if pending.taskID != bound { + *pending = kickPending{taskID: bound, deadline: now.Add(KickDebounce)} + } + pending.cols = cols + if now.Before(pending.deadline) { + return // still dwelling + } *kickedFor = bound // cols is a terminal column count — bounded by realistic screen widths, // nowhere near uint16's range; gosec G115 flags the conversion but it's // safe (matches the pattern already used for the analogous conversion in // terminalpane.go's ring-wrap catch-up). - p.kickRerender(bound, uint16(cols)) //nolint:gosec // see comment + p.kickRerender(bound, uint16(pending.cols)) //nolint:gosec // see comment } +// kickClockNow returns the current time for the kick debounce, defaulting to +// time.Now and overridable via SetKickClock (test seam). +func (p *HeraPage) kickClockNow() time.Time { + if p.kickNow != nil { + return p.kickNow() + } + return time.Now() +} + +// SetKickClock overrides the kick-debounce clock (test seam) — mirrors +// Refresher.SetNow. Production code never calls this. +func (p *HeraPage) SetKickClock(fn func() time.Time) { p.kickNow = fn } + // reconcileSessions (re)resolves the live session for each fed pane on the tick, // mirror of the main agent view's tick re-resolution. MUST run on the tview main // thread. It covers both late-bind (a session that came up after selection) and diff --git a/internal/tui/hera/panes_test.go b/internal/tui/hera/panes_test.go index 96e69c91..19c7e396 100644 --- a/internal/tui/hera/panes_test.go +++ b/internal/tui/hera/panes_test.go @@ -176,14 +176,45 @@ func TestPanes_IsBoundToTask(t *testing.T) { testutil.Equal(t, p.IsBoundToTask(""), false) } -// TestPanes_DrawInvokesRerenderKicker proves Draw calls the wired +// kickRecorder wires a HeraPage's RerenderKicker to a capturing slice plus a +// fake, test-controlled clock (see HeraPage.SetKickClock) so debounce tests +// can advance time deterministically without a real 300ms sleep. +type kickRecord struct { + taskID string + cols uint16 +} + +func newKickRecorder(p *HeraPage) (kicks *[]kickRecord, advance func(time.Duration)) { + var recorded []kickRecord + p.SetRerenderKicker(func(taskID string, cols uint16) { + recorded = append(recorded, kickRecord{taskID, cols}) + }) + now := time.Now() + p.SetKickClock(func() time.Time { return now }) + return &recorded, func(d time.Duration) { now = now.Add(d) } +} + +func kicksFor(kicks []kickRecord, taskID string) []uint16 { + var cols []uint16 + for _, k := range kicks { + if k.taskID == taskID { + cols = append(cols, k.cols) + } + } + return cols +} + +// TestPanes_DrawInvokesRerenderKicker proves Draw evaluates the wired // RerenderKicker with each pane's OWN fresh width — for BOTH the coordinator -// pane and the worker/agent pane — exactly once per genuine bind, and that a -// repeated Draw at the SAME bound task does not re-invoke it. The check is -// evaluated from Draw (not bindPane) specifically because bindPane runs in -// the input handler, before Draw has had a chance to give a newly-shown pane -// (e.g. the agent pane, hidden while a coordinator was selected in details -// mode) a real rect — see maybeKickPaneRerender's doc comment. +// pane and the worker/agent pane — but debounces the actual invocation: the +// FIRST Draw after a genuine bind only arms the pending kick (KickDebounce +// design), it does not fire immediately. Only a LATER Draw, once the dwell +// has elapsed for the SAME bound task, actually invokes the kicker, exactly +// once. The check is evaluated from Draw (not bindPane) specifically because +// bindPane runs in the input handler, before Draw has had a chance to give a +// newly-shown pane (e.g. the agent pane, hidden while a coordinator was +// selected in details mode) a real rect — see maybeKickPaneRerender's doc +// comment. func TestPanes_DrawInvokesRerenderKicker(t *testing.T) { d := memDB(t) orch := seedOrch(t, d, "orch") @@ -194,15 +225,7 @@ func TestPanes_DrawInvokesRerenderKicker(t *testing.T) { wkrSess := &fakeSession{id: "t-wkr", alive: true} p := NewHeraPage(d) p.SetSessionResolver(resolverFor(map[string]*fakeSession{"t-coord": coordSess, "t-wkr": wkrSess})) - - type kick struct { - taskID string - cols uint16 - } - var kicks []kick - p.SetRerenderKicker(func(taskID string, cols uint16) { - kicks = append(kicks, kick{taskID, cols}) - }) + kicks, advance := newKickRecorder(p) p.Refresh() // Select the worker BEFORE any Draw — this is exactly the sequence that @@ -221,41 +244,155 @@ func TestPanes_DrawInvokesRerenderKicker(t *testing.T) { p.SetRect(0, 0, 120, 30) p.Draw(sim) - kicksFor := func(taskID string) []uint16 { - var cols []uint16 - for _, k := range kicks { - if k.taskID == taskID { - cols = append(cols, k.cols) - } - } - return cols - } - wkrKicks := kicksFor("t-wkr") - coordKicks := kicksFor("t-coord") + // First Draw after a genuine bind only ARMS the dwell — no kick yet. + testutil.Equal(t, len(*kicks), 0) + + // Advance the clock past the dwell and draw again: the SAME tasks are + // still bound, so the kick fires now, exactly once per pane. + advance(KickDebounce) + p.Draw(sim) + + wkrKicks := kicksFor(*kicks, "t-wkr") + coordKicks := kicksFor(*kicks, "t-coord") if len(wkrKicks) != 1 { - t.Fatalf("expected exactly one kick check for the worker/agent pane bind, got %d", len(wkrKicks)) + t.Fatalf("expected exactly one kick for the worker/agent pane bind, got %d", len(wkrKicks)) } if len(coordKicks) != 1 { - t.Fatalf("expected exactly one kick check for the coordinator pane bind, got %d", len(coordKicks)) + t.Fatalf("expected exactly one kick for the coordinator pane bind, got %d", len(coordKicks)) } for _, c := range append(wkrKicks, coordKicks...) { if c == 0 { - t.Errorf("kick check used cols=0 — pane width wasn't resolved from a real rect") + t.Errorf("kick used cols=0 — pane width wasn't resolved from a real rect") } } - // A second Draw at the same bound tasks must not re-invoke the kicker. - kicks = nil + // A further Draw at the same bound tasks must not re-invoke the kicker. + *kicks = nil p.Draw(sim) - testutil.Equal(t, len(kicks), 0) + testutil.Equal(t, len(*kicks), 0) // Unbinding and rebinding to the SAME task must re-evaluate (kickedFor is - // reset on unbind) rather than being silently suppressed forever. - p.bindPane(p.AgentPane(), &p.agentBound, &p.agentKickedFor, "", "agent") - p.bindPane(p.AgentPane(), &p.agentBound, &p.agentKickedFor, "t-wkr", "agent") - kicks = nil + // reset on unbind) rather than being silently suppressed forever — but it + // still goes through the dwell again. + p.bindPane(p.AgentPane(), &p.agentBound, &p.agentKickedFor, &p.agentKickPending, "", "agent") + p.bindPane(p.AgentPane(), &p.agentBound, &p.agentKickedFor, &p.agentKickPending, "t-wkr", "agent") + *kicks = nil + p.Draw(sim) + testutil.Equal(t, len(kicksFor(*kicks, "t-wkr")), 0) // armed, not yet fired + advance(KickDebounce) + p.Draw(sim) + testutil.Equal(t, len(kicksFor(*kicks, "t-wkr")), 1) +} + +// TestPanes_KickDebounce_FastTraversalNeverKicks is the kick-storm regression: +// a fast multi-row rail traversal (Cmd+Arrow across several rows) rebinds the +// agent pane to a DIFFERENT task on every hop, none of them staying bound for +// the full debounce dwell. None of the transiently-bound tasks should ever be +// kicked — each hop's pending kick must be discarded, un-fired, by the next +// hop's rebind. +func TestPanes_KickDebounce_FastTraversalNeverKicks(t *testing.T) { + d := memDB(t) + orch := seedOrch(t, d, "orch") + for _, name := range []string{"a", "b", "c"} { + seedBoundRole(t, d, orch, name, db.HeraKindWorker, "t-"+name) + } + p := NewHeraPage(d) + p.SetSessionResolver(resolverFor(map[string]*fakeSession{ + "t-a": {id: "t-a", alive: true}, + "t-b": {id: "t-b", alive: true}, + "t-c": {id: "t-c", alive: true}, + })) + kicks, advance := newKickRecorder(p) + p.Refresh() + + sim := tcell.NewSimulationScreen("UTF-8") + testutil.NoError(t, sim.Init()) + defer sim.Fini() + sim.SetSize(120, 30) + p.SetRect(0, 0, 120, 30) + + // Hop across all three rows in quick succession, well under the dwell + // between each hop — exactly a fast Cmd+Arrow traversal. + for _, name := range []string{"a", "b", "c"} { + testutil.Equal(t, selectRoleByName(p, name), true) + p.Draw(sim) + advance(KickDebounce / 10) + } + + testutil.Equal(t, len(*kicks), 0) + + // Confirm the mechanism isn't just permanently wedged: staying on the + // LAST row past the dwell still kicks it. + advance(KickDebounce) + p.Draw(sim) + testutil.Equal(t, len(kicksFor(*kicks, "t-c")) >= 1, true) + testutil.Equal(t, len(kicksFor(*kicks, "t-a")), 0) + testutil.Equal(t, len(kicksFor(*kicks, "t-b")), 0) +} + +// TestPanes_KickDebounce_DwellAndStayStillKicks proves the anti-corruption +// kick is not silently lost — only delayed — for the legitimate case: a rail +// selection that stays put past the dwell still fires exactly once. +func TestPanes_KickDebounce_DwellAndStayStillKicks(t *testing.T) { + d := memDB(t) + orch := seedOrch(t, d, "orch") + seedBoundRole(t, d, orch, "wkr", db.HeraKindWorker, "t-wkr") + p := NewHeraPage(d) + p.SetSessionResolver(resolverFor(map[string]*fakeSession{"t-wkr": {id: "t-wkr", alive: true}})) + kicks, advance := newKickRecorder(p) + p.Refresh() + testutil.Equal(t, selectRoleByName(p, "wkr"), true) + + sim := tcell.NewSimulationScreen("UTF-8") + testutil.NoError(t, sim.Init()) + defer sim.Fini() + sim.SetSize(120, 30) + p.SetRect(0, 0, 120, 30) + + p.Draw(sim) // arms + testutil.Equal(t, len(*kicks), 0) + advance(KickDebounce) + p.Draw(sim) // fires + testutil.Equal(t, len(kicksFor(*kicks, "t-wkr")), 1) +} + +// TestPanes_KickDebounce_UnbindMidDwellThenRebindSameTask proves an unbind +// that happens mid-dwell doesn't let a rebind to the SAME task fire the kick +// any earlier than a fresh dwell would — bindPane's unbind path resets +// kickedFor, and the rebind's taskID comparison in maybeKickPaneRerender is +// what re-arms pending, so the dwell always restarts cleanly rather than +// firing on stale pending state (design.md Decision 2). +func TestPanes_KickDebounce_UnbindMidDwellThenRebindSameTask(t *testing.T) { + d := memDB(t) + orch := seedOrch(t, d, "orch") + seedBoundRole(t, d, orch, "wkr", db.HeraKindWorker, "t-wkr") + p := NewHeraPage(d) + p.SetSessionResolver(resolverFor(map[string]*fakeSession{"t-wkr": {id: "t-wkr", alive: true}})) + kicks, advance := newKickRecorder(p) + p.Refresh() + testutil.Equal(t, selectRoleByName(p, "wkr"), true) + + sim := tcell.NewSimulationScreen("UTF-8") + testutil.NoError(t, sim.Init()) + defer sim.Fini() + sim.SetSize(120, 30) + p.SetRect(0, 0, 120, 30) + + p.Draw(sim) // arms against t-wkr + advance(KickDebounce / 2) + + // Unbind, then immediately rebind to the SAME task mid-dwell. + p.bindPane(p.AgentPane(), &p.agentBound, &p.agentKickedFor, &p.agentKickPending, "", "agent") + p.bindPane(p.AgentPane(), &p.agentBound, &p.agentKickedFor, &p.agentKickPending, "t-wkr", "agent") + + // Only half the ORIGINAL dwell has elapsed — not yet due. + p.Draw(sim) + testutil.Equal(t, len(*kicks), 0) + + // A full dwell past the rebind fires exactly once. + advance(KickDebounce) p.Draw(sim) - testutil.Equal(t, len(kicksFor("t-wkr")), 1) + testutil.Equal(t, len(kicksFor(*kicks, "t-wkr")), 1) } // TestPanes_CoordinatorSelectionShowsDetails is a locked must-have: selecting a @@ -443,7 +580,7 @@ func TestPanes_BindLifecycle(t *testing.T) { testutil.Equal(t, p.AgentPane().Session(), prev) // Unbind: bindPane with "" clears the pane. - p.bindPane(p.AgentPane(), &p.agentBound, &p.agentKickedFor, "", "agent") + p.bindPane(p.AgentPane(), &p.agentBound, &p.agentKickedFor, &p.agentKickPending, "", "agent") testutil.Equal(t, p.agentBound, "") testutil.Nil(t, p.AgentPane().Session()) } diff --git a/internal/tui/heraactions.go b/internal/tui/heraactions.go index ddf3ccd5..25a12c31 100644 --- a/internal/tui/heraactions.go +++ b/internal/tui/heraactions.go @@ -45,6 +45,12 @@ func heraGoSafe(label string, fn func()) { // main thread; the only slow op (spawn) is dispatched to a goroutine. // heraRefresh rebuilds the rail immediately after a mutation and repaints. +// HeraPage.Refresh() (called below) always forces the rebuild — it +// invalidates doRefresh's change-detection gate itself before flushing, so a +// mutation just written through a.db (invisible to the tick's OWN next +// PRAGMA data_version read — SQLite's documented same-connection blind spot, +// see design.md Decision 5) is never silently missed here or on the tick +// immediately following. func (a *App) heraRefresh() { a.heraPage.Refresh() a.forceRedraw("hera mutation") diff --git a/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/.openspec.yaml b/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/.openspec.yaml new file mode 100644 index 00000000..1b062d3a --- /dev/null +++ b/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-04 diff --git a/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/README.md b/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/README.md new file mode 100644 index 00000000..954cd03b --- /dev/null +++ b/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/README.md @@ -0,0 +1,3 @@ +# fix-hera-tick-and-kick-perf + +Debounce the Hera pane size-drift kick and gate the 1s tick's Hera model rebuild on cheap change-detection, to fix TUI lag and elevated RSS from rail navigation and unconditional per-tick history-sized work diff --git a/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/design.md b/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/design.md new file mode 100644 index 00000000..a440abdb --- /dev/null +++ b/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/design.md @@ -0,0 +1,79 @@ +## Context + +Two distinct, previously-undiagnosed performance defects were found by reading the code against a live report (Aaron's daily-dogfooded TUI process at ~59GB RSS after ~14-15h uptime, daemon/supervisor both small and normal — client-side, not daemon/supervisor). Both live in the native Hera view (`internal/tui/hera`) and its App-level tick wiring (`internal/tui/app.go`). + +**Defect 1 — kick storm.** `agent.ShouldKickRerender` (`internal/agent/rerender.go`) is an intentional, already-shipped fix (BUG-074/BUG-076) for a real corruption bug: a session's committed scrollback baked at one PTY width renders wrong when re-emulated at a meaningfully different width (≥`RerenderMargin`=15 cols), and only a kill+`--session-id`-resume repairs it (a plain SIGWINCH cannot). `HeraPage.bindPane` + `Draw`'s `maybeKickPaneRerender` call sites evaluate this on every pane (re)bind. The defect: Hera's own layout swings a bound pane between full-terminal-width (fullscreen/Details mode) and roughly half-width (split mode) purely from *rail cursor movement between a coordinator row and a worker row* — no resize or fullscreen toggle needed. `kickedFor`'s dedup is keyed per bound task, so it does nothing to stop a fast multi-row traversal (Cmd+Arrow across several rows) from binding N distinct tasks in quick succession and kicking every one of them — each a real `Session.Stop()` + restart + full conversation replay. + +**Defect 2 — unconditional O(history) tick work.** `App.onTick` (1s ticker) unconditionally calls `refreshTasksWithIDs`, which fetches all tasks (~910 rows for Aaron) plus a reconciliation pass, two `ListMetaByNamespace` scans (~959 rows each), and `ManagedTaskIDs()`. While the Hera tab is active, it also calls `HeraPage.ScheduleRefresh()`, which — debounced to at most once per 100ms, i.e. effectively once per 1s tick in practice — runs `BuildModel` (fetches ALL orchestrators *including archived*, all live + latest bindings, hera-namespaced meta, and re-fetches the FULL task list a second time) followed by `Rail.buildRows()`, whose `canonicalParents()`/`structuralReach()` walk the WHOLE model — archived orchestrators included — every single time. None of this is gated on whether anything actually changed. Aaron reported stalling while sitting still with only 5-6 active agents, which is the signature of cost proportional to *total historical* role/orchestrator/binding count (~900+ for Aaron), not active-agent count. + +## Goals / Non-Goals + +**Goals:** +- Eliminate the kick storm from ordinary rail navigation without weakening the anti-corruption kick itself in any way a genuine dwell-and-stay would notice (still fires, ~300ms later). +- Cap the steady-state (nothing-changed) cost of the Hera tab's periodic tick refresh close to zero, regardless of total historical row count, without introducing a silent staleness bug for the many ways the rail's rendered state can legitimately change (DB writes from the daemon or another connection, and the four runtime maps that drive the model but aren't stored in the DB at all). +- De-duplicate the one clearly-wasteful redundant full task fetch per tick. +- Measure, not assume: confirm the gate actually reduces per-tick cost, and be honest about whether it explains the full 59GB or leaves a real, separate retention path unaccounted for. + +**Non-Goals (this change):** +- Gating `refreshTasksWithIDs`'s own base reads (`db.Tasks()`, `ListMetaByNamespace("pr")`, `ListMetaByNamespace("hera")` via `readHeraRoles`/`readPRStates`, `ManagedTaskIDs()` via `readManagedTasks`) behind the same change-detection signal. These feed the plain Tasks-tab list, which has far more mutation call sites (pin, archive, rename, status change, new task, delete, ...) than the Hera-specific chokepoint (`App.heraRefresh`) this change's invalidation relies on; auditing every one of them to prove no silent staleness was judged too large a correctness surface for this PR. Named follow-up. +- Lazy-loading archived orchestrators/roles inside `Rail.buildRows()`'s graph algorithms (`canonicalParents`/`structuralReach`) so a rebuild that DOES have to happen (something genuinely changed) also gets structurally cheaper. `context/knowledge/gotchas/hera-view.md` documents a long history of subtle, hard-to-get-right bugs in exactly this fold/nesting/reveal logic (BUG-002/004/007/064/069/070/071 and others); restructuring it to defer archived-section processing is a real, separate, higher-risk change. This PR's change-detection gate already caps the *steady-state* cost of that same code (skips it entirely when idle), which is the dominant complaint — it just doesn't make an actually-needed rebuild cheaper. Named follow-up. +- Reducing Aaron's actual historical task/role row count (archiving/deleting ~900 tasks) — his call, not this fix's job; the fix must perform reasonably regardless of how much history exists. +- Any change to `agent.ShouldKickRerender`, `RerenderMargin`, or the kill+resume mechanism itself. + +## Decisions + +### 1. Kick debounce lives in `HeraPage.maybeKickPaneRerender`, as a pending-kick dwell — not a new goroutine, not a change to the shared predicate. + +`maybeKickPaneRerender` is already the single, Draw-driven, per-pane choke point evaluated after each `SetRect` (see the prior `fix-hera-pane-rerender-kick` change's Decision 3 for why it's evaluated there and not in `bindPane`). It already tracks per-pane state (`coordKickedFor`/`agentKickedFor`). Adding a second piece of per-pane state — an armed-but-unfired `kickPending{taskID, cols, deadline}` — keeps the debounce colocated with the existing dedup, evaluated on the same Draw cadence, with no new goroutine or timer: exactly the "goroutine-free debounced... driven by the existing app tick" pattern `hera.Refresher` already established for the rail rebuild debounce (mirrored here, not reused directly, since `Refresher` debounces a single global rebuild callback and this needs two independent per-pane deadlines with data — task ID and cols — carried alongside). + +Alternative considered: extend `Refresher` itself to also gate the kick. Rejected — `Refresher` is a generic "coalesce repeated Schedule() calls into one rebuild" primitive with no concept of "which task", and forcing it to also carry per-pane kick state would conflate two independently-evolving debounces (rail-rebuild-debounce vs. kick-debounce) behind one abstraction for no code-sharing benefit (the actual due-time comparison is three lines). + +### 2. A rebind to a *different* task before the dwell elapses re-arms the timer against the new task; `bindPane` additionally clears the pending state to its zero value on unbind. + +The mission's framing ("cancel/clear the pending state immediately on unbind or rebind to a different task") is satisfied for the "different task" half by a single check: `maybeKickPaneRerender` compares the incoming `bound` task ID against `pending.taskID`; any mismatch re-arms fresh against the new task, discarding the old deadline un-fired. + +The "unbind" half turned out NOT to be free, contrary to the first draft of this design: an initial version left `bindPane` unaware of `kickPending` entirely, reasoning that an unbind (`bound == ""`) is already an early-return in `maybeKickPaneRerender`, so a stale `pending` would simply go unused until the next genuine bind re-armed it via the taskID comparison. `TestPanes_KickDebounce_UnbindMidDwellThenRebindSameTask` (written before this fix, per TDD) disproved that: if a kick already fired once for a task (setting a deadline in the past), and the pane is later unbound and rebound to that SAME task, `pending.taskID != bound` is FALSE (unchanged), so the stale, long-past deadline survives the rebind and the very next `Draw` fires the kick immediately — no fresh dwell at all. `bindPane` now takes the pending pointer and resets it to its zero value in the unbind branch (alongside its existing `kickedFor` reset), so any later rebind — same task or different — always arms a fresh deadline. This touched `bindPane`'s signature and all nine call sites (four in `panes.go`'s `applySelection`, five direct calls across `panes_test.go`), a larger diff than the original plan but the one actually required for correctness. + +### 3. Dwell = 300ms. + +Comfortably longer than a single keystroke-to-Draw round trip (so a deliberate pause on one row reliably survives it) and well under a second (so a genuine dwell-and-stay still feels responsive — the prior behavior was instant, so 300ms is the entire added latency budget for the legitimate case). Chosen from the mission's suggested 250-400ms range; no measurement suggested a more precise number is warranted, and it's a single named constant (`hera.KickDebounce`) if retuning is ever needed. + +### 4. The Hera model rebuild's change-detection combines a cross-connection DB fingerprint with a same-process runtime-map equality check — neither alone is sufficient. + +- **DB fingerprint alone is insufficient**: the model's `RoleView`s also carry `NeedsInput`/`SessionIdle`/`SessionRunning`/`SustainedActive`, which are runtime-derived (agent output/idle detection), not stored in any DB row. Gating purely on "did the DB change" would freeze the rail's spinner and needs-input glyphs whenever the DB is stable but agents are still actively producing output or going idle — exactly the common case Aaron reported ("5-6 active agents... sitting still"). The gate therefore also compares these four maps (via `maps.Equal`) against a snapshot taken at the last rebuild; they're bounded by *live/active* session count (small, e.g. 5-6 entries), not total history, so the comparison is cheap even when the DB fingerprint is stable. +- **Runtime-map equality alone is insufficient**: a DB-only change (e.g. a hera worker's binding created/ended via a daemon-driven MCP tool call, from a process this TUI never observes through the four runtime maps) needs to be detected too. Hence the DB fingerprint. +- **The DB fingerprint is `PRAGMA data_version`** (`DB.DataVersion`), not a hand-rolled row-count/hash scheme: it's SQLite's own built-in, purpose-built "did any OTHER connection commit a write to this file" counter, near-O(1) to read (confirmed via a throwaway cross-connection experiment: a write via connection A is visible as a bumped value on connection B's very next read; connection A's OWN subsequent read of its OWN write does NOT bump — see next decision for why that blind spot doesn't cause staleness here). No schema change, no new column, no migration. +- **Why not add an `updated_at` column + `MAX(updated_at)` instead?** Would require a migration touching every write path across `tasks`/`hera_orchestrators`/`hera_roles`/`hera_bindings`/`task_meta` to keep it correctly bumped, a much larger and riskier surface than a read-only pragma already provided by SQLite for exactly this purpose. + +### 5. `PRAGMA data_version`'s same-connection blind spot is closed by explicit invalidation at `App.heraRefresh`, not by ignoring it. + +Empirically confirmed (scratch two-connection test against `modernc.org/sqlite`): a write committed through connection A does NOT change the `data_version` value connection A itself reads back afterward — only a DIFFERENT connection observes the bump. The TUI's own `a.db` is a single connection that both reads (the tick) and writes (hera mutations via `hera.Ops`, reconciliation's `SetStatus`) through the SAME connection. Relying on `data_version` alone would mean a TUI-key-triggered hera mutation (pin, status step, kanban step, hide, spawn, nuke) — indistinguishable from "nothing changed" to the tick's own next `data_version` read — could leave the gate skipping a rebuild that should happen. + +This is a non-issue in practice for the interactive path itself: every existing hera mutation handler already calls `App.heraRefresh()` immediately after its write, which calls `HeraPage.Refresh()` (`Schedule()`+`Flush()`, bypassing the rail-rebuild debounce) — so the mutating user already sees their own change instantly. The natural place to close the same-connection gap is `Refresh()` itself, not each individual caller: `Refresh()`'s own doc contract already promises "forces an immediate rail rebuild" (it's used for tab-entry too, and by test code that expects a call to it to be unconditional), so `Refresh()` now calls `HeraPage.InvalidateChangeGate()` before flushing, guaranteeing the gate never silently turns a "forced" refresh into a no-op — regardless of whether the caller is `heraRefresh`, tab entry, or a test. `ScheduleRefresh` (the tick's own periodic, NOT-forced call) is deliberately left subject to the gate; only the "force it now" entry point bypasses it. This was caught by an existing regression test (`TestRefresh_StatusStepReprojectsPlanNode`) that writes directly through the same `*db.DB` the page reads from and then calls `Refresh()` expecting an unconditional rebuild — the first implementation attempt (invalidating only inside `heraRefresh`) broke it, which is exactly the class of caller `Refresh()`'s own contract needs to cover regardless of which App-level wrapper reaches it. + +### 6. The gate lives in `internal/tui/hera` (co-located with the state it protects), using an unexported type-assertion for the fingerprint — not a new required method on `HeraReader`. + +`HeraReader` is satisfied by `*db.DB` in production and by lighter fakes in tests; adding a new required method would force every test fake to grow a no-op implementation for no benefit. Instead, an unexported `dataVersioner` interface (`DataVersion() (int64, error)`) is type-asserted against `p.reader` the same way `App` already type-asserts `a.db.(*db.DB)` elsewhere for other local-only optimizations (e.g. `readManagedTasks`'s remote fallback). A reader that doesn't implement it (remote mode's nil reader, or a test fake) always takes the full rebuild path — identical to today's behavior, never a regression for those callers. + +### 7. The double `Tasks()` fetch is de-duplicated by wrapping the reader passed to `BuildModel`, not by changing `BuildModel`'s signature. + +`refreshTasksWithIDs` already fetches the full task list once per tick; `BuildModel` (called moments later, same tick, via `doRefresh`) was calling `r.Tasks()` again for the identical data. `HeraPage` gains a `SetTasks([]*model.Task)` setter (mirroring the existing `SetNeedsInput`/`SetSessionIdle`/etc. pattern) that `App` feeds from `a.tasks` right where it feeds the other four maps. `doRefresh` wraps `p.reader` in a small unexported `tasksReader` (embeds `HeraReader`, overrides only `Tasks()` to return the cached slice) before calling `BuildModel`, so `BuildModel` itself is completely unchanged — it still calls `r.Tasks()` exactly as before, on whatever reader it's handed. The wrap is skipped (passing `p.reader` straight through) when `p.reader` is nil (remote mode) or `SetTasks` was never called, so `BuildModel` falls back to fetching itself, byte-identical to today. + +Alternative considered: add a `tasks []*model.Task` parameter directly to `BuildModel`. Rejected — `BuildModel` has ~42 direct call sites across the existing test suite (`model_test.go`, `bug028_repro_test.go`, `bug024_test.go`, `plan_test.go`, `pin_nonroot_test.go`, `model_sustainedactive_test.go`), and a signature change would force a purely-mechanical `nil`-argument edit across all of them for a change whose only production caller is `doRefresh`. The wrapper keeps the diff confined to `page.go`/`model.go`'s package-private surface, with zero edits to unrelated existing tests. (This also keeps `HeraPage.dbFingerprint`'s `p.reader.(dataVersioner)` type-assertion working correctly against the UNWRAPPED reader — the wrapper is constructed transiently at the `BuildModel` call site, never stored back into `p.reader` itself, so the two features don't interact.) + +## Risks / Trade-offs + +- **[Risk] The dwell adds ~300ms of latency to a genuine dwell-and-stay's anti-corruption kick.** → Accepted; explicitly signed off in the mission brief as a reasonable trade against a kick storm. The corruption the kick prevents is a rendering artifact repaired the moment the kick actually fires — a 300ms-later fire still repairs it before the user would plausibly notice the pre-kick corrupted frame at a glance. +- **[Risk] The change-detection gate's invalidation relies on an enumerated, not exhaustive, list of local-write call sites (`App.heraRefresh` + the one reconciliation `SetStatus` call).** → Scoped deliberately: every OTHER hera-mutation call path in this codebase already funnels through `heraRefresh` (verified by reading `heraactions.go`), and it is the natural single chokepoint precisely because it already exists as "the one place every hera mutation forces a refresh." A new hera mutation added later that does NOT call `heraRefresh` would already have a worse bug (its own change wouldn't render immediately) independent of this gate — so this dependency doesn't introduce a new failure mode, it inherits an existing one. +- **[Risk] `PRAGMA data_version` behavior is a SQLite implementation detail relied upon rather than a documented Go API contract.** → It's a long-standing, stable, documented core SQLite pragma (not an extension), and behavior was empirically verified against this repo's actual driver (`modernc.org/sqlite`) rather than assumed. A reader that doesn't support it (remote mode, test fakes) degrades to "always rebuild" — never silently wrong, only silently non-optimized. +- **[Risk] Skipping the DB re-fetch could theoretically miss a change if `data_version` wrapped or reset.** → SQLite's counter is a monotonically-changing value for the life of the file; a reset only happens on schema changes/vacuum-equivalent operations, none of which argus performs during normal operation. Even in that unlikely case, the failure mode is "one extra unneeded rebuild is skipped," recoverable on the very next tick that DOES change something — not a permanently stuck stale state. + +## Migration Plan + +No data migration; no schema change. `DB.DataVersion()` is a pure read-only pragma passthrough. Purely additive TUI-internal wiring (new fields, new setter, new gate function) — rollback is a plain revert. + +## Open Questions + +- Should the change-detection gate's scope be widened to `refreshTasksWithIDs`'s own base reads in a follow-up, once this narrower gate is dogfooded and its invalidation-chokepoint approach is proven safe in practice? Left as a named follow-up (see proposal.md Impact) rather than attempted here, given the larger number of un-audited local-write call sites feeding the plain task list. +- Is 300ms the right dwell for Fix 1, or should it be tuned after dogfooding shows how fast Aaron's actual Cmd+Arrow traversal cadence is? Chosen from the mission's suggested range with no dogfood measurement backing the specific number; flagged as easy to retune (one named constant) if 300ms proves too short/long in practice. +- Does the measurement in this PR (see tasks.md §5) show the fix caps sustained RSS growth, or does a separate, still-unaccounted-for retention path remain? **Answered, partially:** `BenchmarkDoRefresh_AlwaysRebuild`/`BenchmarkDoRefresh_SteadyStateGated` (`internal/tui/hera/doRefresh_bench_test.go`, ~900 roles/bindings seeded — Aaron's reported scale) measured **34.3ms and ~29.7MB/132,903 allocs per rebuild** pre-fix (unconditional every tick) vs **~844ns and 400B/12 allocs** per tick post-fix when idle — roughly a **35,000x** reduction in per-tick allocation volume and CPU cost while quiescent. At 1 tick/second this is ~30MB/s of allocation churn eliminated, i.e. on the order of **hundreds of GB of cumulative garbage-collector churn per hour** that no longer happens while idle — strong evidence this is a major contributor to sustained GC/heap pressure. **What this does NOT prove:** allocation volume is not the same as RETAINED memory; Go's GC reclaims transient garbage, so this benchmark alone cannot confirm what fraction (if any) of the reported 59GB RSS was retained-forever vs. GC-pressure-inflated heap targets vs. a separate, still-unidentified leak. Confirming that requires an actual longitudinal RSS comparison of the real TUI dogfooded for a comparable multi-hour period — not performed in this session (infeasible within a single PR's time budget) — flagged as a follow-up: dogfood this fix and compare RSS growth over a similar-length session against the 59GB baseline. diff --git a/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/proposal.md b/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/proposal.md new file mode 100644 index 00000000..fd7178dc --- /dev/null +++ b/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/proposal.md @@ -0,0 +1,34 @@ +## Why + +Aaron's daily-dogfooded `argus` TUI process was observed at ~59GB RSS / 46% of system RAM / 36% CPU after ~14-15h uptime, while the daemon (280MB) and session-supervisor (24MB) stayed small — a client-side (TUI process) problem, not daemon/supervisor. Restarting the TUI temporarily fixes it. Code investigation found two distinct, real, unconditional-per-tick costs in the native Hera view: + +1. **Kick storm on ordinary rail navigation.** Hera's own layout swings a bound pane's width between full-terminal-width (fullscreen/Details mode) and roughly half-width (split mode) as the rail cursor moves between a coordinator row and a worker row — no fullscreen toggle or terminal resize involved. `agent.ShouldKickRerender`'s `RerenderMargin` (15 cols) is crossed by this alone, so a fast multi-row Cmd+Arrow traversal binds several distinct tasks in quick succession and `HeraPage.maybeKickPaneRerender` invokes the wired `RerenderKicker` (`App.heraKickRerender`) once per newly-bound task — each a real `Session.Stop()` + `--session-id` restart + full conversation replay. `kickedFor`'s dedup only prevents repeats on the *same* bound task; it does nothing for a rapid traversal across *different* tasks. + +2. **Unconditional O(total-DB-rows) work on the 1s UI tick.** `App.onTick` → `refreshTasksWithIDs` fetches every task row every second regardless of activity, and — while the Hera tab is active — also drives `HeraPage.doRefresh` → `BuildModel` + `Rail.buildRows()` once per tick (debounced to at most 100ms, which in practice means once per 1s tick). `BuildModel` calls `ListHeraOrchestrators(true)` (including fully-archived history), `ListHeraLiveBindings`/`ListHeraLatestBindings`, and re-fetches the full task list a SECOND time (duplicating `refreshTasksWithIDs`'s own fetch in the same tick); `Rail.buildRows()` then runs `canonicalParents()`/`structuralReach()` — graph algorithms over the WHOLE model, archived orchestrators included — every single time. Aaron reported stalling/fluctuating responsiveness while sitting still with only 5-6 active agents, which points squarely at cost that scales with *total historical* role/orchestrator/binding count (Aaron has ~900+), not active-agent count. Sustained large-map/slice allocation on a 1Hz cadence for hours is a plausible major contributor to sustained elevated RSS/GC pressure. + +## What Changes + +- **Debounce the Hera pane size-drift kick.** `HeraPage.maybeKickPaneRerender` gains a ~300ms wall-clock dwell: the first `Draw` that finds a newly-bound task armed past the rerender margin does NOT fire the kick immediately — it arms a per-pane pending-kick record. Only a *later* `Draw`, once the dwell has elapsed AND the same task is still bound, actually invokes the wired `RerenderKicker`. A rebind to a *different* task before the dwell elapses re-arms against the new task and discards the old one un-fired. A genuine dwell-and-stay still kicks, just ~300ms later than today. The kick mechanism itself (`agent.ShouldKickRerender`, `RerenderMargin`, the idle/prompt/pending gates, the exit-handler resume path) is completely unchanged — this only gates *when* `maybeKickPaneRerender` calls the kicker, matching the existing goroutine-free, UI-thread-driven debounce style already used by `hera.Refresher`. +- **Gate the Hera model rebuild on cheap change-detection.** `HeraPage.doRefresh` skips the full `BuildModel` + `Rail.SetModel` pass whenever nothing that could affect the rendered rail has changed since the last rebuild: a cheap SQLite `PRAGMA data_version` fingerprint (near-O(1), catches any daemon/other-process-driven DB write) combined with a bounded-size equality check over the four per-tick runtime maps (`needsInput`/`sessionIdle`/`sessionRunning`/`sustainedActive` — sized by *active* session count, not total history) that also feed the model. `data_version` has a documented same-connection blind spot (a write made through the SAME connection doesn't bump the value that connection reads back), so every TUI-side hera mutation's existing immediate-refresh chokepoint (`App.heraRefresh`) also invalidates the cached fingerprint, guaranteeing the very next opportunity does a full rebuild rather than relying on the blind spot staying harmless. +- **De-duplicate the double full task-list fetch.** `refreshTasksWithIDs` and `BuildModel` each independently call `Tasks()` on the same tick; `BuildModel` now accepts the already-fetched task snapshot instead of re-querying it. +- Add temporary, explicitly-scoped measurement (tick timing + a documented manual RSS soak procedure) to confirm the fix actually reduces per-tick cost and to honestly report whether it accounts for the full 59GB or leaves a separate, unexplained retention path as a named follow-up. + +## Capabilities + +### New Capabilities + +(none) + +### Modified Capabilities + +- `hera-view`: the "PTY size alignment on bind" requirement (area 6, kick-on-drift) gains a debounce dwell before the kick fires. A new requirement describes the Hera model rebuild's change-detection gate (skip conditions, the runtime-map equality check, and the `heraRefresh` invalidation chokepoint). + +## Impact + +- `internal/tui/hera/panes.go` (`maybeKickPaneRerender`, `bindPane` doc): new per-pane pending-kick dwell state. +- `internal/tui/hera/page.go` (`doRefresh`): change-detection gate wrapping the existing `BuildModel`/`SetModel` call; new cached-fingerprint/runtime-map-snapshot fields. +- `internal/tui/hera/page.go` (`doRefresh`): wraps the reader passed to `BuildModel` so its existing `Tasks()` call is served from the App's already-fetched snapshot instead of a second underlying fetch, when one has been supplied via the new `HeraPage.SetTasks`. +- `internal/db/*.go`: new `DB.DataVersion()` (`PRAGMA data_version` passthrough). +- `internal/tui/heraactions.go` (`heraRefresh`): invalidates the cached fingerprint so the same-connection blind spot never causes a missed rebuild. +- `internal/tui/app.go`: threads `a.tasks` into the Hera rebuild path instead of a second fetch. +- **Explicitly out of scope, named follow-ups (not silently dropped):** gating `refreshTasksWithIDs`'s own `db.Tasks()`/`ListMetaByNamespace`("pr")/`ListMetaByNamespace`("hera")/`ManagedTaskIDs()` reads on the same change-detection signal (left unconditional, as today — these feed the plain Tasks-tab list, which has many more mutation call sites than the Hera-mutation chokepoint this change relies on, and auditing all of them was judged too large a correctness surface for this PR); lazy-loading archived orchestrators/roles in `Rail.buildRows()`'s graph algorithms so a purely-idle steady state also stops paying for them structurally (deferred given the risk to `rail.go`'s heavily invariant-laden fold/nesting logic — see `context/knowledge/gotchas/hera-view.md`'s long history of subtle bugs in that exact code path). The change-detection gate in this PR still caps the *steady-state* cost of both by skipping the rebuild entirely when idle, which is the dominant complaint (stalling while sitting still); it does not reduce the cost of a rebuild that DOES need to happen while historical data remains large. diff --git a/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/specs/hera-view/spec.md b/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/specs/hera-view/spec.md new file mode 100644 index 00000000..70e64439 --- /dev/null +++ b/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/specs/hera-view/spec.md @@ -0,0 +1,101 @@ +## MODIFIED Requirements + +### Requirement: PTY size alignment on bind (area 6) + +The system SHALL resize a bound session to the (narrower) hera pane when binding it, by calling `ForceResyncPTY()` so a session previously sized for the full-width main agent view is resized down on the next Draw, with `SyncPanes()` issuing the Resize RPC off the main thread. Pane operations on the tview thread SHALL use only lock-free/local session methods; the blocking resize RPC runs only from the tick goroutine. The view SHALL NOT use `screen.Sync()` to paper over a size mismatch. + +A plain PTY resize (SIGWINCH) only re-flows a session's LIVE UI — it cannot repair scrollback already committed at a different width, because cursor-positioning codes baked into earlier PTY output remain wrong once re-emulated at a new size. When binding a session (coordinator pane or worker/agent pane) whose recorded initial PTY width differs from the current hera pane's width by at least the shared rerender margin, the system SHALL evaluate the SAME kill+resume decision the main agent view applies on entry (`agent.ShouldKickRerender`), using the hera pane's own current width — not the main agent view's. The decision SHALL be skipped when a kick is already pending for the task, when the session lacks a resumable session ID, when the agent is not idle (deferred, not lost), or when the agent is blocked on a user prompt (deferred, never dismissed). The redundant-attach cache SHALL be shared with the main agent view's (keyed by task ID, not by which surface is asking), so a task already evaluated at its current attach width is not re-evaluated on every pane rebind. + +The kick decision, once its gates pass, SHALL NOT fire immediately: it SHALL be debounced by a short wall-clock dwell (300ms) so that ordinary rail navigation — which alone swings a bound pane between full-width (fullscreen/Details) and roughly half-width (split), crossing the rerender margin with no resize or fullscreen toggle involved — does not kill+restart a session for every row a fast multi-row traversal passes through. The first evaluation of a newly-bound task past the margin SHALL arm a pending kick (recording the task and its current width) rather than firing; only a LATER evaluation, once the dwell has elapsed AND the same task is still the bound target, SHALL actually invoke the kick. A rebind to a DIFFERENT task before the dwell elapses SHALL discard the prior pending kick un-fired and arm fresh against the new task. The dwell SHALL be evaluated without a new goroutine or timer, on the same Draw-driven cadence `maybeKickPaneRerender` already runs on. + +Derived from: `internal/tui/hera/panes.go:86` (`bindPane` ForceResyncPTY), `internal/tui/hera/panes.go:153` (`SyncPanes`), `internal/tui/hera/panes.go:172` (`forwardKey` main-thread-safe reads), `internal/tui/hera/panes.go` (`maybeKickPaneRerender`, called from `page.go`'s `Draw` right after each pane's `SetRect` — not from `bindPane`, since a pane hidden by details mode has no real width yet at bind time; now also gated by a `kickPending` dwell), `internal/tui/app.go` (`maybeKickRerenderAtWidth`, `heraKickRerender`, `HeraPage.SetRerenderKicker`), `internal/agent/rerender.go` (`ShouldKickRerender`, `RerenderMargin`), `context/knowledge/gotchas/hera-view.md` (BUG-074, and the kick-debounce bullet added by this change), `context/knowledge/gotchas/pty-terminal.md`. + +#### Scenario: Bind resizes a full-width session down + +- **WHEN** a session sized for the full-width agent view is bound into a hera pane +- **THEN** `ForceResyncPTY` arms an unconditional resize and `SyncPanes` applies it off the main thread + +#### Scenario: Off-tab SyncPanes is a no-op + +- **WHEN** `SyncPanes` is called while the Hera tab is not active +- **THEN** no resize fires (panes not drawn this frame have zero pending resize), so it cannot fight the main agent view's resize of the same task + +#### Scenario: Binding a session with drifted committed width kills and resumes it, after a dwell + +- **WHEN** the coordinator pane or the worker/agent pane binds a live, idle, resumable session whose `InitialPTYSize` cols differ from the pane's current cols by at least the rerender margin, no kick is already pending for that task, and the SAME task remains bound for at least the debounce dwell +- **THEN** the session is stopped and the existing exit-handler resumes it via `--session-id` at the pane's current dimensions, so its scrollback re-renders at the current width instead of staying corrupted at the old one + +#### Scenario: A busy or prompt-blocked session is not killed on bind + +- **WHEN** a bind's width drift meets the rerender margin but the session is not idle, or is idle only because it is blocked on a user prompt +- **THEN** the session is left running (no kick), so an in-flight tool call or an `AskUserQuestion` overlay is never interrupted + +#### Scenario: A redundant rebind at the same width does not re-evaluate the kick + +- **WHEN** a task is rebound into a hera pane at a width already evaluated for that task (whether the prior evaluation was from the same pane, the other hera pane, or the main agent view) +- **THEN** the kick predicate is skipped for that rebind + +#### Scenario: A backend without a resumable session ID is never kicked + +- **WHEN** a bound task's session has no resumable session ID (e.g. a Codex-backed task) +- **THEN** no kick is attempted regardless of width drift + +#### Scenario: A fast multi-row rail traversal never kicks any of the transiently-bound tasks + +- **WHEN** the rail cursor moves across several rows in quick succession (each hop rebinding a different task past the rerender margin), and no single task stays bound for the full debounce dwell +- **THEN** none of the transiently-bound tasks are kicked — each hop's pending kick is discarded, un-fired, by the next hop's rebind + +#### Scenario: A genuine dwell-and-stay still kicks, just later + +- **WHEN** the rail cursor lands on a row and stays there past the debounce dwell, and the bound task's width drift still meets the rerender margin at that point +- **THEN** the kick fires exactly once, ~300ms after the bind rather than immediately + +### Requirement: Debounced rail refresh on the UI thread (area 6) + +The system SHALL rebuild the rail model via a goroutine-free, timer-free debounced `Refresher` driven by the app tick and tab entry. `Schedule()` coalesces bursts into one rebuild per debounce window; tab entry forces an immediate flush. Rebuilds run on the tview thread because hera-store reads are mutex-guarded and fast (the "never on the UI thread" rule is about git, not DB reads). After `SetModel` the selection is re-derived and the panes rebound, so stale model pointers are refreshed. + +Within the debounce window's rebuild opportunity, the system SHALL additionally skip the actual `BuildModel`+`SetModel` rebuild work when a cheap change-detection check proves nothing that could affect the rendered rail has changed since the last rebuild: a SQLite `PRAGMA data_version` fingerprint of the underlying store (unchanged since the last rebuild) AND all four per-tick runtime maps fed into the model (`needsInput`, `sessionIdle`, `sessionRunning`, `sustainedActive`) equal to their values at the last rebuild. The very first rebuild opportunity SHALL always run (no prior snapshot to compare against). A store that does not expose a data-version fingerprint (remote mode's nil reader; a test double) SHALL always be treated as changed, so the gate never suppresses a rebuild it cannot prove is safe to skip. Because a write made through the SAME store connection that performs the tick's own reads does not change what that connection's own `PRAGMA data_version` read reports (a documented SQLite same-connection blind spot), every interactive hera mutation's existing immediate-refresh path SHALL also invalidate the cached fingerprint, so the tick immediately following any such mutation always takes the full rebuild path regardless of what the fingerprint reports. + +When the App has already fetched the full task list this tick (it always has, for the plain task list), the Hera model rebuild SHALL reuse that snapshot rather than performing a second, redundant full task-list fetch of its own; a rebuild opportunity for which no such snapshot has been supplied SHALL fetch it itself, unchanged from today. + +Derived from: `internal/tui/hera/refresher.go` (`Refresher`), `internal/tui/hera/page.go:138` (`ScheduleRefresh`), `internal/tui/hera/page.go:150` (`doRefresh`, now gated by `shouldRebuild`/`markRebuilt`), `internal/tui/hera/panes.go:59` (`applySelection` re-run), `internal/db` (`DB.DataVersion`), `internal/tui/heraactions.go` (`heraRefresh` fingerprint invalidation), `context/knowledge/gotchas/hera-view.md`. + +#### Scenario: Burst of writes coalesces to one rebuild + +- **WHEN** several store writes schedule refreshes within one debounce window +- **THEN** the rail rebuilds once + +#### Scenario: Tab entry forces a fresh rail + +- **WHEN** the Hera tab is opened +- **THEN** the refresher flushes immediately so the rail is current the instant the tab appears + +#### Scenario: A quiescent tick skips the rebuild entirely + +- **WHEN** a rebuild opportunity arrives (debounce window elapsed) and neither the store's data-version fingerprint nor any of the four runtime maps has changed since the last rebuild +- **THEN** `BuildModel`/`SetModel` are not called; the rail keeps its last-built model unchanged + +#### Scenario: A DB-only change (no runtime-map change) still triggers a rebuild + +- **WHEN** the store's data-version fingerprint has changed since the last rebuild (e.g. a daemon-driven hera binding write) but none of the four runtime maps differ +- **THEN** the rail rebuilds + +#### Scenario: A runtime-only change (no DB write) still triggers a rebuild + +- **WHEN** the data-version fingerprint is unchanged but at least one of `needsInput`/`sessionIdle`/`sessionRunning`/`sustainedActive` differs from the last-rebuild snapshot (e.g. an active agent produced output or went idle) +- **THEN** the rail rebuilds, so the spinner/needs-input glyphs never freeze while the underlying DB rows are stable + +#### Scenario: A local hera mutation is never missed by the gate + +- **WHEN** a TUI-side hera mutation (pin, status step, kanban step, hide, spawn, nuke) writes through the same store connection the tick reads from, and its handler calls the existing immediate-refresh path +- **THEN** the cached fingerprint is invalidated as part of that path, so the next rebuild opportunity takes the full rebuild regardless of whether the fingerprint alone would have reported a change + +#### Scenario: A store without a data-version fingerprint always rebuilds + +- **WHEN** the reader does not implement the fingerprint (remote mode, or a test double) +- **THEN** every rebuild opportunity runs the full `BuildModel`/`SetModel` pass, identical to behavior before this change + +#### Scenario: The Hera model rebuild reuses a supplied task snapshot instead of re-fetching + +- **WHEN** the App has already fetched the task list this tick and supplies it to the Hera rebuild +- **THEN** the rebuild uses that snapshot and performs no second underlying task-list fetch of its own diff --git a/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/tasks.md b/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/tasks.md new file mode 100644 index 00000000..b3c8ef80 --- /dev/null +++ b/openspec/changes/archive/2026-08-04-fix-hera-tick-and-kick-perf/tasks.md @@ -0,0 +1,51 @@ +## 1. Kick debounce (Fix 1) + +- [x] 1.1 Add `KickDebounce` constant (300ms) and a `kickPending{taskID, cols, deadline}` type in `internal/tui/hera/panes.go`. +- [x] 1.2 Add `HeraPage.coordKickPending`/`agentKickPending` fields and a `kickNow func() time.Time` clock seam (defaults to `time.Now` in `NewHeraPage`), plus a `SetKickClock` test setter mirroring `hera.Refresher.SetNow`. +- [x] 1.3 Rewrite `maybeKickPaneRerender` to arm-then-fire: a newly-bound task (vs. `pending.taskID`) arms a fresh deadline instead of kicking immediately; a later call past the deadline for the SAME task fires the kick via the existing `kickRerender` callback, unchanged otherwise. +- [x] 1.4 Update the four `Draw` call sites to pass the corresponding pending struct. + +## 2. Tests for the kick debounce + +- [x] 2.1 Update `TestPanes_DrawInvokesRerenderKicker` (semantics changed: the FIRST Draw after a bind no longer fires immediately) to use the clock seam: first Draw arms but does not kick; advancing the clock past the dwell and drawing again fires exactly once; a further Draw does not re-fire. +- [x] 2.2 New test: a rebind to a DIFFERENT task before the dwell elapses never kicks the first task (the kick-storm regression case — simulates a fast rail traversal across 3+ rows, none within the dwell). +- [x] 2.3 New test: a genuine dwell-and-stay (same task bound across multiple Draws, clock advanced past the dwell) fires exactly once. +- [x] 2.4 New test: unbind clears the effective pending state (rebinding to the SAME task after an unbind mid-dwell re-arms rather than firing early from stale state) — covers Decision 2's reasoning directly rather than just asserting it in prose. + +## 3. Tick change-detection gate (Fix 2) + +- [x] 3.1 `internal/db`: add `DB.DataVersion() (int64, error)` (`PRAGMA data_version` passthrough). +- [x] 3.2 `internal/tui/hera`: add unexported `dataVersioner` interface + `HeraPage.dbFingerprint() (int64, bool)` type-asserting `p.reader`. +- [x] 3.3 `internal/tui/hera`: add `HeraPage.SetTasks([]*model.Task)`; `BuildModel` accepts a tasks parameter, falling back to `r.Tasks()` when not supplied (nil). +- [x] 3.4 `internal/tui/hera`: add `HeraPage.shouldRebuild() bool` (first-ever call, OR fingerprint changed/unsupported, OR any of the 4 runtime maps changed via `maps.Equal` against the last-rebuild snapshot) and `markRebuilt()` (snapshot fingerprint + the 4 maps by reference — safe because every producer always reassigns a fresh map rather than mutating in place, confirmed by reading all 4 setters). +- [x] 3.5 `doRefresh` calls `shouldRebuild()` first; skips (with a `uxlog.Log("[hera] ...")` noting the skip, per the CLAUDE.md silently-skipped-work logging rule) when false, else proceeds as today and calls `markRebuilt()` at the end. +- [x] 3.6 `internal/tui/hera/page.go`: `HeraPage.Refresh()` (the general "force it now" primitive already used by tab-entry, `heraRefresh`, and tests) calls `InvalidateChangeGate()` before flushing, so its own doc contract ("forces an immediate rebuild") holds regardless of caller — NOT scoped to `heraRefresh` alone (see design.md Decision 5 for why the first, narrower attempt broke an existing test). +- [x] 3.7 `internal/tui/app.go`: feed `HeraPage.SetTasks(a.tasks)` alongside the existing `SetNeedsInput`/etc. calls in `refreshTasksWithIDs`. + +## 4. Tests for the change-detection gate + +- [x] 4.1 `shouldRebuild`/`markRebuilt` table tests: first call always true; unchanged fingerprint + unchanged maps → false; changed fingerprint → true; each of the 4 maps individually changed → true; unsupported fingerprint (fake reader without `DataVersion`) → always true. +- [x] 4.2 `doRefresh` integration test (`TestHeraPage_DoRefresh_SkipsRebuildWhenQuiescent`): two calls with nothing changed → the rail's rebuilt model keeps the coordinator role's stale `NeedsInput=false` (no rebuild happened); a call after `SetNeedsInput` with a genuinely different set → the model reflects `NeedsInput=true` on the very next call. +- [x] 4.3 `DB.DataVersion` test (`TestDB_DataVersion`, `internal/db`): value changes after a write from a SECOND connection to the same file; stays stable across repeated reads with no writes; a write through the SAME connection does NOT change what that connection reads back (mirrors the cross-connection experiment from design.md, made a permanent regression test — `t.TempDir()`-backed, no real `~/.argus/`). +- [x] 4.4 Gate-invalidation test: rather than testing `App.heraRefresh` directly (it lives in package `tui`, not `hera`, and would need a full App fixture), tested at the layer `heraRefresh` calls into — `TestHeraPage_InvalidateChangeGate` (unit) and `TestHeraPage_Refresh_ForcesRebuildDespiteSameConnectionBlindSpot` (end-to-end: a same-connection write is confirmed invisible to `dbFingerprint()`, yet `Refresh()` still rebuilds and the rail model reflects the change). +- [x] 4.5 Reader-wrapping tasks-dedup test (`TestHeraPage_SetTasks_AvoidsRedundantFetch`, using a call-counting `HeraReader` wrapper): supplying a snapshot via `SetTasks` keeps the underlying reader's `Tasks()` call count at 1 (paid once, before `SetTasks`); never calling `SetTasks` falls back to the reader's own fetch on every rebuild, unchanged from today. + +## 5. Measurement (required by the mission, not optional) + +- [x] 5.1 Added a permanent `time.Since` timing around `doRefresh`'s full-rebuild path (`internal/tui/hera/page.go`, logged in the existing `[hera-view] rail refreshed: ...` line only when a rebuild actually happens, plus a `[hera-view] rail refresh skipped: ...` line on the gated no-op path) — kept as a shipped log line, not removed: it's cheap (one `time.Since` per genuine rebuild, never per-tick) and gives an ongoing signal if a future change reintroduces expensive per-rebuild work. +- [x] 5.2 Added `internal/tui/hera/doRefresh_bench_test.go` (`BenchmarkDoRefresh_AlwaysRebuild` / `BenchmarkDoRefresh_SteadyStateGated`), seeding ~900 roles/~900 bindings across 450 archived orchestrators + 1 active one (matching Aaron's reported scale) — a reproducible `go test -bench` measurement rather than a one-off transcript. Results (Apple M5 Max, `-benchtime 30x -benchmem`): **pre-fix** (unconditional rebuild every tick) = 34.3ms, 29.7MB, 132,903 allocs per tick; **post-fix steady-state** (idle, nothing changed) = 844ns, 400B, 12 allocs per tick — ~35,000x reduction in per-tick cost while idle. +- [x] 5.3 Documented in `design.md`'s Open Questions: the fix meaningfully reduces steady-state per-tick cost (measured above) and this is strong evidence of a major GC-pressure contributor (~30MB/s of eliminated allocation churn while idle), but allocation-rate reduction is NOT the same as proof of retained-memory reduction — whether it explains the full reported 59GB RSS is explicitly flagged as unconfirmed, with a named follow-up (dogfood + compare RSS over a comparable multi-hour session) rather than assumed. +- [x] 5.4 Kept: the `doRefresh` timing log (ongoing regression signal, near-zero cost) and the two benchmarks (regression guard, `go test -bench`-only — never runs during plain `go test ./...`, zero CI cost). Nothing purely-diagnostic was added that needed removal before merge. + +## 6. Docs and gates + +- [x] 6.1 `context/knowledge/gotchas/hera-view.md`: added a bullet for the kick debounce (why it exists, the 300ms constant, the re-arm-on-rebind behavior, and the unbind-clear fix caught by TDD). +- [x] 6.2 `context/knowledge/gotchas/hera-view.md`: added bullets for the tick change-detection gate — the `data_version` same-connection blind spot and why `Refresh()`'s own invalidation (not just `heraRefresh`) closes it, the measured cost reduction, and the explicit non-goals (base task-list reads + archived-row lazy-loading still unconditional/full-cost, named follow-ups). +- [x] 6.3 Updated `context/knowledge/index.md`'s `hera-view.md` row: bullet count 171 → 175, with a summary of the new coverage. +- [x] 6.4 `make pre-pr` confirmed clean: build/vet/fmt-check/lint-pr all green; `vuln` fails only on 3 pre-existing stdlib CVEs (CI `continue-on-error`, unrelated to this diff — confirmed via `.github/workflows/ci.yml`); `test-cover-gate` green at 88.8% (floor 88%) once two pre-existing, documented environmental issues are accounted for: (a) this hera-worker sandbox's own `ARGUS_MODEL`/`ARGUS_TASK_ID`/`ARGUS_ARCHETYPE`/`ARGUS_PROFILE` env vars leak into 2 `internal/agent` profile-env tests (confirmed passing with those unset; CI has no such vars); (b) `TestSmoke_NewTaskFormPaste` is a documented pre-existing `-race` flake (confirmed flaky even in isolation, 4/5 runs passing, unrelated to any file this PR touches). A clean run with the sandbox env excluded passed fully green (exit 0). +- [x] 6.5 `openspec archive fix-hera-tick-and-kick-perf` run in this PR, before merge — archived as `2026-08-04-fix-hera-tick-and-kick-perf`. + +## 7. Follow-ups (not in this change — flagged, not silently dropped) + +- [x] 7.1 Flagged (proposal.md Impact, design.md Non-Goals): widening the change-detection gate to `refreshTasksWithIDs`'s own base reads (`db.Tasks()`, both `ListMetaByNamespace` calls, `ManagedTaskIDs()`) once this narrower gate is dogfooded. Not implemented in this change. +- [x] 7.2 Flagged (proposal.md Impact, design.md Non-Goals): lazy-loading archived orchestrators/roles in `Rail.buildRows()`'s graph algorithms so an ACTUALLY-needed rebuild is also cheaper, not just less frequent. Not implemented in this change (risk to `rail.go`'s invariant-laden fold logic). diff --git a/openspec/specs/hera-view/spec.md b/openspec/specs/hera-view/spec.md index 3e1bfa65..161caf2e 100644 --- a/openspec/specs/hera-view/spec.md +++ b/openspec/specs/hera-view/spec.md @@ -382,7 +382,9 @@ The system SHALL resize a bound session to the (narrower) hera pane when binding A plain PTY resize (SIGWINCH) only re-flows a session's LIVE UI — it cannot repair scrollback already committed at a different width, because cursor-positioning codes baked into earlier PTY output remain wrong once re-emulated at a new size. When binding a session (coordinator pane or worker/agent pane) whose recorded initial PTY width differs from the current hera pane's width by at least the shared rerender margin, the system SHALL evaluate the SAME kill+resume decision the main agent view applies on entry (`agent.ShouldKickRerender`), using the hera pane's own current width — not the main agent view's. The decision SHALL be skipped when a kick is already pending for the task, when the session lacks a resumable session ID, when the agent is not idle (deferred, not lost), or when the agent is blocked on a user prompt (deferred, never dismissed). The redundant-attach cache SHALL be shared with the main agent view's (keyed by task ID, not by which surface is asking), so a task already evaluated at its current attach width is not re-evaluated on every pane rebind. -Derived from: `internal/tui/hera/panes.go:86` (`bindPane` ForceResyncPTY), `internal/tui/hera/panes.go:153` (`SyncPanes`), `internal/tui/hera/panes.go:172` (`forwardKey` main-thread-safe reads), `internal/tui/hera/panes.go` (`maybeKickPaneRerender`, called from `page.go`'s `Draw` right after each pane's `SetRect` — not from `bindPane`, since a pane hidden by details mode has no real width yet at bind time), `internal/tui/app.go` (`maybeKickRerenderAtWidth`, `heraKickRerender`, `HeraPage.SetRerenderKicker`), `internal/agent/rerender.go` (`ShouldKickRerender`, `RerenderMargin`), `context/knowledge/gotchas/hera-view.md` (BUG-074), `context/knowledge/gotchas/pty-terminal.md`. +The kick decision, once its gates pass, SHALL NOT fire immediately: it SHALL be debounced by a short wall-clock dwell (300ms) so that ordinary rail navigation — which alone swings a bound pane between full-width (fullscreen/Details) and roughly half-width (split), crossing the rerender margin with no resize or fullscreen toggle involved — does not kill+restart a session for every row a fast multi-row traversal passes through. The first evaluation of a newly-bound task past the margin SHALL arm a pending kick (recording the task and its current width) rather than firing; only a LATER evaluation, once the dwell has elapsed AND the same task is still the bound target, SHALL actually invoke the kick. A rebind to a DIFFERENT task before the dwell elapses SHALL discard the prior pending kick un-fired and arm fresh against the new task. The dwell SHALL be evaluated without a new goroutine or timer, on the same Draw-driven cadence `maybeKickPaneRerender` already runs on. + +Derived from: `internal/tui/hera/panes.go:86` (`bindPane` ForceResyncPTY), `internal/tui/hera/panes.go:153` (`SyncPanes`), `internal/tui/hera/panes.go:172` (`forwardKey` main-thread-safe reads), `internal/tui/hera/panes.go` (`maybeKickPaneRerender`, called from `page.go`'s `Draw` right after each pane's `SetRect` — not from `bindPane`, since a pane hidden by details mode has no real width yet at bind time; now also gated by a `kickPending` dwell), `internal/tui/app.go` (`maybeKickRerenderAtWidth`, `heraKickRerender`, `HeraPage.SetRerenderKicker`), `internal/agent/rerender.go` (`ShouldKickRerender`, `RerenderMargin`), `context/knowledge/gotchas/hera-view.md` (BUG-074, and the kick-debounce bullet added by this change), `context/knowledge/gotchas/pty-terminal.md`. #### Scenario: Bind resizes a full-width session down @@ -394,9 +396,9 @@ Derived from: `internal/tui/hera/panes.go:86` (`bindPane` ForceResyncPTY), `inte - **WHEN** `SyncPanes` is called while the Hera tab is not active - **THEN** no resize fires (panes not drawn this frame have zero pending resize), so it cannot fight the main agent view's resize of the same task -#### Scenario: Binding a session with drifted committed width kills and resumes it +#### Scenario: Binding a session with drifted committed width kills and resumes it, after a dwell -- **WHEN** the coordinator pane or the worker/agent pane binds a live, idle, resumable session whose `InitialPTYSize` cols differ from the pane's current cols by at least the rerender margin, and no kick is already pending for that task +- **WHEN** the coordinator pane or the worker/agent pane binds a live, idle, resumable session whose `InitialPTYSize` cols differ from the pane's current cols by at least the rerender margin, no kick is already pending for that task, and the SAME task remains bound for at least the debounce dwell - **THEN** the session is stopped and the existing exit-handler resumes it via `--session-id` at the pane's current dimensions, so its scrollback re-renders at the current width instead of staying corrupted at the old one #### Scenario: A busy or prompt-blocked session is not killed on bind @@ -414,11 +416,25 @@ Derived from: `internal/tui/hera/panes.go:86` (`bindPane` ForceResyncPTY), `inte - **WHEN** a bound task's session has no resumable session ID (e.g. a Codex-backed task) - **THEN** no kick is attempted regardless of width drift +#### Scenario: A fast multi-row rail traversal never kicks any of the transiently-bound tasks + +- **WHEN** the rail cursor moves across several rows in quick succession (each hop rebinding a different task past the rerender margin), and no single task stays bound for the full debounce dwell +- **THEN** none of the transiently-bound tasks are kicked — each hop's pending kick is discarded, un-fired, by the next hop's rebind + +#### Scenario: A genuine dwell-and-stay still kicks, just later + +- **WHEN** the rail cursor lands on a row and stays there past the debounce dwell, and the bound task's width drift still meets the rerender margin at that point +- **THEN** the kick fires exactly once, ~300ms after the bind rather than immediately + ### Requirement: Debounced rail refresh on the UI thread (area 6) The system SHALL rebuild the rail model via a goroutine-free, timer-free debounced `Refresher` driven by the app tick and tab entry. `Schedule()` coalesces bursts into one rebuild per debounce window; tab entry forces an immediate flush. Rebuilds run on the tview thread because hera-store reads are mutex-guarded and fast (the "never on the UI thread" rule is about git, not DB reads). After `SetModel` the selection is re-derived and the panes rebound, so stale model pointers are refreshed. -Derived from: `internal/tui/hera/refresher.go` (`Refresher`), `internal/tui/hera/page.go:138` (`ScheduleRefresh`), `internal/tui/hera/page.go:150` (`doRefresh`), `internal/tui/hera/panes.go:59` (`applySelection` re-run). +Within the debounce window's rebuild opportunity, the system SHALL additionally skip the actual `BuildModel`+`SetModel` rebuild work when a cheap change-detection check proves nothing that could affect the rendered rail has changed since the last rebuild: a SQLite `PRAGMA data_version` fingerprint of the underlying store (unchanged since the last rebuild) AND all four per-tick runtime maps fed into the model (`needsInput`, `sessionIdle`, `sessionRunning`, `sustainedActive`) equal to their values at the last rebuild. The very first rebuild opportunity SHALL always run (no prior snapshot to compare against). A store that does not expose a data-version fingerprint (remote mode's nil reader; a test double) SHALL always be treated as changed, so the gate never suppresses a rebuild it cannot prove is safe to skip. Because a write made through the SAME store connection that performs the tick's own reads does not change what that connection's own `PRAGMA data_version` read reports (a documented SQLite same-connection blind spot), every interactive hera mutation's existing immediate-refresh path SHALL also invalidate the cached fingerprint, so the tick immediately following any such mutation always takes the full rebuild path regardless of what the fingerprint reports. + +When the App has already fetched the full task list this tick (it always has, for the plain task list), the Hera model rebuild SHALL reuse that snapshot rather than performing a second, redundant full task-list fetch of its own; a rebuild opportunity for which no such snapshot has been supplied SHALL fetch it itself, unchanged from today. + +Derived from: `internal/tui/hera/refresher.go` (`Refresher`), `internal/tui/hera/page.go:138` (`ScheduleRefresh`), `internal/tui/hera/page.go:150` (`doRefresh`, now gated by `shouldRebuild`/`markRebuilt`), `internal/tui/hera/panes.go:59` (`applySelection` re-run), `internal/db` (`DB.DataVersion`), `internal/tui/heraactions.go` (`heraRefresh` fingerprint invalidation), `context/knowledge/gotchas/hera-view.md`. #### Scenario: Burst of writes coalesces to one rebuild @@ -430,6 +446,36 @@ Derived from: `internal/tui/hera/refresher.go` (`Refresher`), `internal/tui/hera - **WHEN** the Hera tab is opened - **THEN** the refresher flushes immediately so the rail is current the instant the tab appears +#### Scenario: A quiescent tick skips the rebuild entirely + +- **WHEN** a rebuild opportunity arrives (debounce window elapsed) and neither the store's data-version fingerprint nor any of the four runtime maps has changed since the last rebuild +- **THEN** `BuildModel`/`SetModel` are not called; the rail keeps its last-built model unchanged + +#### Scenario: A DB-only change (no runtime-map change) still triggers a rebuild + +- **WHEN** the store's data-version fingerprint has changed since the last rebuild (e.g. a daemon-driven hera binding write) but none of the four runtime maps differ +- **THEN** the rail rebuilds + +#### Scenario: A runtime-only change (no DB write) still triggers a rebuild + +- **WHEN** the data-version fingerprint is unchanged but at least one of `needsInput`/`sessionIdle`/`sessionRunning`/`sustainedActive` differs from the last-rebuild snapshot (e.g. an active agent produced output or went idle) +- **THEN** the rail rebuilds, so the spinner/needs-input glyphs never freeze while the underlying DB rows are stable + +#### Scenario: A local hera mutation is never missed by the gate + +- **WHEN** a TUI-side hera mutation (pin, status step, kanban step, hide, spawn, nuke) writes through the same store connection the tick reads from, and its handler calls the existing immediate-refresh path +- **THEN** the cached fingerprint is invalidated as part of that path, so the next rebuild opportunity takes the full rebuild regardless of whether the fingerprint alone would have reported a change + +#### Scenario: A store without a data-version fingerprint always rebuilds + +- **WHEN** the reader does not implement the fingerprint (remote mode, or a test double) +- **THEN** every rebuild opportunity runs the full `BuildModel`/`SetModel` pass, identical to behavior before this change + +#### Scenario: The Hera model rebuild reuses a supplied task snapshot instead of re-fetching + +- **WHEN** the App has already fetched the task list this tick and supplies it to the Hera rebuild +- **THEN** the rebuild uses that snapshot and performs no second underlying task-list fetch of its own + ### Requirement: Archive, pin, rename, delete, and status ops act on the selection (area 7) The system SHALL apply each mutation to the SELECTED `(role, orchestrator)` from the rail cursor, never a bare task ID. Archive and pin toggles read the current row state from the store to choose direction. Pinning clears archived state (pin and archive are mutually exclusive). Rename surfaces a name-conflict error for the caller to display. Status advance/revert step the hera role status ladder (idle → working → blocked → done), clamped at the ends, and reaching `done` on a WORKER role also rolls the bound task to in_review (soft-fail). Stepping a WORKER role to any NON-`done` status (revert off `done`, or any other step) clears the bound task's `meta:hera.ready_to_close` mark via `ClearHeraReadyToClose` (the inverse of the done-roll's stamp), so the rail glyph — which checks `ready_to_close` FIRST in its precedence — reflects the new hera status instead of staying pinned to the review `✓`. The clear is soft-fail (the status update always lands) and touches meta only; the task's argus WORKFLOW status is owned by the session lifecycle and is never changed by a status step. Mutations are thin adapters over existing store methods; the spawn path is the shared `agent.SpawnHeraWorker` primitive, run off the main thread.