From 71e9e74e220ab555d5ec67b6797a968edd18c430 Mon Sep 17 00:00:00 2001 From: Hesham Salman Date: Fri, 20 Mar 2026 22:13:59 -0400 Subject: [PATCH] fix: align ultraplan h/l navigation with visual sidebar order Navigation used getNavigableInstances() which builds its list in plan-execution order, but the sidebar renders via FlattenGroupsForDisplay which uses group-structure order. These orderings diverge because instances are added to groups in creation order, not plan order. Now navigateToNextInstance() uses getInstanceDisplayOrder() (the same ordering the sidebar renders) filtered to the navigable instance set, ensuring h/l move in the direction the user visually expects. Also fixes: - handlePlanFileCheckResult silently swallowing SetPlan errors, leaving users stuck in a dead session - Removes dead SelectedNavIdx field from ultraplan state - Updates getNavigableInstances doc comment to reflect its reduced role as a membership set provider --- AGENTS.md | 1 + CHANGELOG.md | 2 ++ internal/tui/ultraplan.go | 25 ++++++++++++++++++++----- internal/tui/view/ultraplan/context.go | 1 - 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c01b2a5a..867d3410 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -365,6 +365,7 @@ Patterns and conventions observed in this codebase that aren't covered by the ge - **Config field → StartOptions override chain** — `ClaudeBackendConfig` stores persistent defaults; `StartOptions` provides per-invocation overrides. In `BuildStartCommand`, each flag uses a priority chain: `StartOptions` value > `ClaudeBackend` value > no flag. See `firstNonEmpty`/`firstPositive`/`mergeUnique` helpers in `internal/ai/backend.go`. This enables role-specific behavior (e.g., `PermissionMode: "plan"` for reviewers). - **Per-role factory creation in bridgewire** — `PipelineExecutor.attachBridges` creates a *per-team* `instanceFactory` when `RoleOverrides` contains an entry for the team's role. The factory carries `ai.StartOptions` that flow through `Orchestrator.StartInstanceWithOverrides → newInstanceManager → ManagerOptions.StartOverrides → Manager.Start()`. The default shared factory is used for teams without role overrides. - **Capture loop recovery pattern** — `Manager.captureLoop()` detects tmux server death at four distinct points (heartbeat check, session status query, unresponsive threshold, capture failure). All four sites call `attemptSessionRecovery()` before `handleSessionEnded()`. Recovery creates a fresh tmux session and resumes the Claude session via `--resume`. The persistent input handler auto-reconnects to the new session (same socket name) without explicit re-initialization. +- **Navigation must follow visual display order** — The ultraplan sidebar is rendered via `FlattenGroupsForDisplay` (group-structure order), but navigation used to use `getNavigableInstances` (plan-execution order). These orderings diverge because instances are added to groups in creation order, not plan order. Any keyboard navigation that moves between sidebar items must use `getInstanceDisplayOrder()` as its ordering source, filtered to the set of navigable items, to stay consistent with what the user sees. --- diff --git a/CHANGELOG.md b/CHANGELOG.md index 23945df8..aa24600f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Subprocess Mode** - Removed the experimental subprocess execution mode (`experimental.subprocess_mode`), including the `internal/streamjson/` package, `subprocessFactory`, and all related config/TUI/wiring plumbing. Pipeline instances now always use the tmux-based execution backend. ### Fixed +- **Ultraplan h/l Navigation Reversed** - Fixed `h` and `l` keybindings navigating in the opposite visual direction in ultraplan mode with groups. Navigation used plan-execution order (`getNavigableInstances`) while the sidebar rendered in group-structure order (`FlattenGroupsForDisplay`), causing the two orderings to diverge. Navigation now follows the visual display order filtered to navigable instances. +- **Silent Plan Validation Failure** - Fixed `handlePlanFileCheckResult` silently swallowing `SetPlan` errors, leaving users stuck in a session that would never progress. Now sets an error message and transitions to `PhaseFailed`, matching the identical error handling in `handlePlanParsed`. - **Missing Sentinel File in Pipeline Execution** - Fixed task instances not writing `.claudio-task-complete.json` in the Orchestration 2.0 pipeline path. The bridge's `BuildTaskPrompt` relied solely on `--append-system-prompt-file` to inject the completion protocol, which left instances unaware of the sentinel file convention. The completion protocol is now embedded directly in the task prompt as defense-in-depth. - **Pipeline Task Grouping in Sidebar** - Fixed execution task instances appearing as ungrouped in the sidebar when using the pipeline execution path (Orchestration 2.0, the default since #659). The bridge's `SessionRecorder` was created with nil callbacks, so newly created instances were never added to the ultraplan's `InstanceGroup`. Wired the `OnAssign` callback to `Coordinator.AssignTaskInstance`, which populates `TaskToInstance` before routing the instance to the correct "Group N" subgroup. Also fixed a latent ordering bug in the legacy `ExecutionOrchestrator` path where `AddInstanceToGroup` was called before `AssignTaskToInstance`, causing instances to fall through to `SubgroupTypeUnknown`. - **Stale Display Early in Session** - Fixed capture loop never populating the output buffer when there's no scrollback (screen not yet full). A single `lastOutput` tracking variable was shared between visible-only and full captures; when a visible capture detected new content and set `lastOutput`, the subsequent forced full capture (which returns identical bytes when there's no scrollback) found no change and skipped the buffer write. Split into independent `lastVisibleOutput` and `lastFullOutput` so each capture type compares against its own history. diff --git a/internal/tui/ultraplan.go b/internal/tui/ultraplan.go index 05400fa2..54ffcb7f 100644 --- a/internal/tui/ultraplan.go +++ b/internal/tui/ultraplan.go @@ -109,9 +109,10 @@ func (m Model) isInstanceSelected(instanceID string) bool { return false } -// getNavigableInstances returns an ordered list of instance IDs that can be navigated to. +// getNavigableInstances returns the set of instance IDs that can be navigated to. // Only includes instances from phases that have started or completed. -// Order: Planning → Execution tasks (in order) → Synthesis → Consolidation +// Note: The ordering of the returned slice is not used for navigation — navigation +// order comes from getInstanceDisplayOrder(). Only set membership matters here. func (m *Model) getNavigableInstances() []string { if m.ultraPlan == nil || m.ultraPlan.Coordinator == nil { return nil @@ -299,9 +300,22 @@ func (m *Model) navigateToNextInstance(direction int) bool { return false } - // Update the navigable instances list + // Build the navigable set from ultraplan-specific logic m.updateNavigableInstances() - instances := m.ultraPlan.NavigableInstances + navigableSet := make(map[string]bool, len(m.ultraPlan.NavigableInstances)) + for _, id := range m.ultraPlan.NavigableInstances { + navigableSet[id] = true + } + + // Use the display order (matches visual sidebar rendering) filtered to navigable instances. + // This ensures h/l navigation direction matches the visual top-to-bottom order. + displayOrder := m.getInstanceDisplayOrder() + var instances []string + for _, id := range displayOrder { + if navigableSet[id] { + instances = append(instances, id) + } + } if len(instances) == 0 { return false @@ -341,7 +355,6 @@ func (m *Model) navigateToNextInstance(direction int) bool { m.pauseInstance(oldInst.ID) } m.activeTab = i - m.ultraPlan.SelectedNavIdx = nextIdx m.ensureActiveVisible() // Resume the new active instance's capture m.resumeActiveInstance() @@ -1050,6 +1063,8 @@ func (m *Model) handlePlanFileCheckResult(msg tuimsg.PlanFileCheckResultMsg) (te // Set the plan if err := m.ultraPlan.Coordinator.SetPlan(msg.Plan); err != nil { + m.errorMessage = fmt.Sprintf("Plan file check completed but plan is invalid: %v", err) + m.ultraPlan.Coordinator.Manager().SetPhase(orchestrator.PhaseFailed) return m, nil } diff --git a/internal/tui/view/ultraplan/context.go b/internal/tui/view/ultraplan/context.go index 9aee696a..87fa271a 100644 --- a/internal/tui/view/ultraplan/context.go +++ b/internal/tui/view/ultraplan/context.go @@ -41,7 +41,6 @@ type State struct { // Phase-aware navigation state NavigableInstances []string // Ordered list of navigable instance IDs - SelectedNavIdx int // Index into navigableInstances // Group re-trigger mode RetriggerMode bool // When true, next digit key triggers group re-trigger