chore(tui): address Phase 2 bot review on narrow tab-core harvest

Six fixes for the bot review comments landed on PR #2864 head
649d3990. See phase2-playbook.md §7 for the triage rationale.

* persistence.rs: oversized state file now surfaces an InvalidData
  error instead of silently returning a default. The old behaviour
  would let the next save overwrite the oversized file and destroy
  the user's data. Test updated to expect the error.

* persistence.rs: PersistedDelegation gains a `status` field so
  in-flight `InProgress` delegations aren't silently demoted to
  `Pending` on restart. The snapshot now writes the live status
  and restore_from_snapshot honours it. Adds a regression test.

* mention.rs: resolve_tab_mention no longer sorts its input — tab
  mentions (@Tab2) must map to the visual order in the tab bar,
  not to an arbitrary ID sort. Test updated.

* manager.rs: `pending_tasks` renamed to `completed_delegations`
  because the getter returns completed DelegationResults, not
  in-flight tasks. Docstring points to the in-flight getter
  `pending_delegations` to avoid the same confusion recurring.

* manager.rs: delegate_task and start_meeting now validate that
  the from/to tab IDs (or all participant IDs) currently exist
  in the manager. Returns `None` on any unknown ID, preventing
  orphaned tasks / meetings with stale tab references. Two new
  regression tests cover both methods.

Local CI matrix (Windows runner, flags matching ci.yml):
- cargo fmt --all -- --check: exit 0
- cargo clippy --workspace --all-features --locked -- -D warnings: exit 0
- cargo test --workspace --all-features --locked: 4282 pass, 6 fail
  (the 6 failures are pre-existing on the baseline; not caused
  by this PR)
- git diff --exit-code -- Cargo.lock: exit 0

The three deferred threads (#1 close_tab cleanup, #5 cross_tab_links
snapshot, #8 TabGroup::new collision risk) are explicitly out of
scope here; they belong to the follow-up collab/UI PR per the
narrow-harvest promise to Hmbown.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

crates/tui/src/tui/tab/manager.rs

@@ -192,6 +192,7 @@ impl TabManager {
                 to_tab: t.to_tab.0,
                 description: t.description.clone(),
                 priority: t.priority,
+                status: t.status,
                 created_at: t.created_at,
                 completed_at: t.completed_at,
                 result: t.result.clone(),
@@ -254,6 +255,8 @@ impl TabManager {
         self.next_tab_id = max_seen_id + 1;
 
         // Restore active delegations so cross-tab work survives restarts.
+        // We honour the persisted status (`InProgress` is preserved) so
+        // work-in-progress isn't silently demoted to `Pending` on restart.
         for d in &state.delegations {
             let task = super::delegator::DelegationTask {
                 task_id: d.task_id.clone(),
@@ -261,7 +264,7 @@ impl TabManager {
                 to_tab: TabId(d.to_tab),
                 description: d.description.clone(),
                 priority: d.priority,
-                status: super::delegator::DelegationStatus::Pending,
+                status: d.status,
                 created_at: d.created_at,
                 deadline: None,
                 completed_at: d.completed_at,
@@ -429,8 +432,13 @@ impl TabManager {
         }
     }
 
-    /// Get pending tasks for a tab
-    pub fn pending_tasks(&self, id: TabId) -> Vec<&DelegationResult> {
+    /// Get completed delegation results for a tab.
+    ///
+    /// Despite the historical name, this returns **completed** results
+    /// (via `delegator.results_for_tab`), not in-flight tasks. Use
+    /// [`Self::pending_delegations`] for tasks that are still pending or
+    /// in progress.
+    pub fn completed_delegations(&self, id: TabId) -> Vec<&DelegationResult> {
         self.delegator.results_for_tab(id)
     }
 
@@ -463,7 +471,12 @@ impl TabManager {
             .unwrap_or_default()
     }
 
-    /// Delegate a task from one tab to another
+    /// Delegate a task from one tab to another.
+    ///
+    /// Returns `None` if either the `from` or `to` tab does not currently
+    /// exist in the manager. This defensive check prevents orphaned
+    /// delegations from being created with stale tab IDs after a tab
+    /// has been closed.
     pub fn delegate_task(
         &mut self,
         from: TabId,
@@ -471,6 +484,11 @@ impl TabManager {
         description: String,
         priority: Priority,
     ) -> Option<String> {
+        let has_from = self.tabs.iter().any(|t| t.metadata.id == from);
+        let has_to = self.tabs.iter().any(|t| t.metadata.id == to);
+        if !has_from || !has_to {
+            return None;
+        }
         self.delegator
             .create_delegation(from, to, description, priority)
     }
@@ -506,8 +524,17 @@ impl TabManager {
         self.delegator.peek_pending_for_tab(tab_id).is_some()
     }
 
-    /// Start a meeting
+    /// Start a meeting.
+    ///
+    /// Returns `None` if any participant tab does not currently exist in
+    /// the manager. This defensive check prevents meetings from being
+    /// created with stale tab IDs after a tab has been closed.
     pub fn start_meeting(&mut self, topic: String, participants: Vec<TabId>) -> Option<String> {
+        for p in &participants {
+            if !self.tabs.iter().any(|t| t.metadata.id == *p) {
+                return None;
+            }
+        }
         self.meeting_manager.start_meeting(topic, participants)
     }
 
@@ -873,4 +900,101 @@ mod tests {
         assert_eq!(group.len(), 1);
         assert_eq!(group[0].len(), 0);
     }
+
+    #[test]
+    fn test_delegate_task_rejects_unknown_tabs() {
+        use super::super::Priority;
+
+        let mut manager = TabManager::new();
+        let from = manager
+            .create_tab("Source".to_string(), TabType::Chat)
+            .unwrap();
+
+        // Unknown `to` tab — should be rejected.
+        let bogus_to = TabId(9999);
+        assert!(
+            manager
+                .delegate_task(from, bogus_to, "x".to_string(), Priority::Normal)
+                .is_none()
+        );
+
+        // Unknown `from` tab — should be rejected.
+        let bogus_from = TabId(9998);
+        let to = manager
+            .create_tab("Target".to_string(), TabType::Chat)
+            .unwrap();
+        assert!(
+            manager
+                .delegate_task(bogus_from, to, "x".to_string(), Priority::Normal)
+                .is_none()
+        );
+
+        // Known tabs — should succeed.
+        let id = manager.delegate_task(from, to, "real".to_string(), Priority::Normal);
+        assert!(id.is_some());
+    }
+
+    #[test]
+    fn test_start_meeting_rejects_unknown_participants() {
+        let mut manager = TabManager::new();
+        let a = manager.create_tab("A".to_string(), TabType::Chat).unwrap();
+        let _b = manager.create_tab("B".to_string(), TabType::Chat).unwrap();
+        let bogus = TabId(4242);
+
+        // Any unknown participant — should be rejected.
+        assert!(
+            manager
+                .start_meeting("topic".to_string(), vec![a, bogus])
+                .is_none()
+        );
+        assert!(
+            manager
+                .start_meeting("topic".to_string(), vec![bogus])
+                .is_none()
+        );
+
+        // All-known participants — should succeed.
+        let meeting_id = manager.start_meeting("topic".to_string(), vec![a, _b]);
+        assert!(meeting_id.is_some());
+    }
+
+    #[test]
+    fn test_snapshot_restore_preserves_in_progress_status() {
+        use super::super::Priority;
+        use super::super::delegator::DelegationStatus;
+
+        let mut manager = TabManager::new();
+        let from = manager
+            .create_tab("Source".to_string(), TabType::Chat)
+            .unwrap();
+        let to = manager
+            .create_tab("Target".to_string(), TabType::Chat)
+            .unwrap();
+
+        // Create a delegation and mark it InProgress (simulating a worker
+        // that has taken the task but not yet completed it).
+        let task_id = manager
+            .delegate_task(from, to, "work".to_string(), Priority::High)
+            .unwrap();
+        manager.delegator.start_task(&task_id);
+        assert_eq!(
+            manager.delegator.all_pending()[0].status,
+            DelegationStatus::InProgress
+        );
+
+        // Snapshot and restore on a fresh manager.
+        let snapshot = manager.snapshot();
+        let mut restored = TabManager::new();
+        restored.restore_from_snapshot(&snapshot);
+
+        // The in-flight `InProgress` status must survive the restart —
+        // demoting it to `Pending` would lose work-in-progress. Query
+        // through `all_pending` (not `pending_delegations`) so the test
+        // is not coupled to the public read-filter on `pending_for_tab`,
+        // which intentionally only surfaces not-yet-started tasks.
+        let restored_tasks = restored.delegator.all_pending();
+        assert_eq!(restored_tasks.len(), 1);
+        assert_eq!(restored_tasks[0].status, DelegationStatus::InProgress);
+        assert_eq!(restored_tasks[0].task_id, task_id);
+    }
 }

crates/tui/src/tui/tab/mention.rs

@@ -151,16 +151,20 @@ fn parse_usize(s: &[u8]) -> Option<usize> {
 
 /// Given a tab number (1-indexed) and the list of tab IDs, return the
 /// matching TabId. Returns `None` if the index is out of range.
+///
+/// The caller is expected to pass the IDs in **visual order** (i.e. the
+/// order they appear in the tab bar). We deliberately do not sort the
+/// list here — tab mentions like `@Tab2` should map to the second tab the
+/// user sees, not the second-smallest ID.
 pub fn resolve_tab_mention<'a, I>(tab_number: usize, tab_ids: I) -> Option<u64>
 where
     I: IntoIterator<Item = &'a u64>,
 {
-    let mut sorted: Vec<u64> = tab_ids.into_iter().copied().collect();
-    sorted.sort_unstable();
-    if tab_number == 0 || tab_number > sorted.len() {
+    let ids: Vec<u64> = tab_ids.into_iter().copied().collect();
+    if tab_number == 0 || tab_number > ids.len() {
         return None;
     }
-    Some(sorted[tab_number - 1])
+    Some(ids[tab_number - 1])
 }
 
 #[cfg(test)]
@@ -205,12 +209,13 @@ mod tests {
 
     #[test]
     fn test_resolve_tab_mention() {
+        // Tab IDs in the visual order they appear in the tab bar.
         let tab_ids = vec![100, 50, 200];
-        // Tab 1 = smallest id (50)
-        assert_eq!(resolve_tab_mention(1, tab_ids.iter()), Some(50));
-        // Tab 2 = middle id (100)
-        assert_eq!(resolve_tab_mention(2, tab_ids.iter()), Some(100));
-        // Tab 3 = largest id (200)
+        // Tab 1 = first in visual order (100)
+        assert_eq!(resolve_tab_mention(1, tab_ids.iter()), Some(100));
+        // Tab 2 = second in visual order (50)
+        assert_eq!(resolve_tab_mention(2, tab_ids.iter()), Some(50));
+        // Tab 3 = third in visual order (200)
         assert_eq!(resolve_tab_mention(3, tab_ids.iter()), Some(200));
         // Out of range
         assert_eq!(resolve_tab_mention(4, tab_ids.iter()), None);

crates/tui/src/tui/tab/persistence.rs

@@ -16,6 +16,7 @@ use std::path::{Path, PathBuf};
 use chrono::{DateTime, Utc};
 use serde::{Deserialize, Serialize};
 
+use super::delegator::DelegationStatus;
 use super::{Priority, TabId, TabMetadata, TabType};
 
 /// Current schema version. Bump when making breaking changes to the
@@ -57,6 +58,10 @@ pub struct PersistedDelegation {
     pub to_tab: u64,
     pub description: String,
     pub priority: Priority,
+    /// Status of the delegation when it was snapshotted. Without this field,
+    /// an in-flight `InProgress` task is silently demoted to `Pending` on
+    /// restart, losing work-in-progress state.
+    pub status: DelegationStatus,
     pub created_at: DateTime<Utc>,
     pub completed_at: Option<DateTime<Utc>>,
     pub result: Option<String>,
@@ -122,13 +127,23 @@ pub fn load_from_file(path: &Path) -> std::io::Result<PersistedTabState> {
     // Size check
     let metadata = std::fs::metadata(path)?;
     if metadata.len() > MAX_FILE_SIZE {
-        tracing::warn!(
+        // Silently returning `default()` would let the next save overwrite
+        // the oversized file and destroy the user's data. Surface the error
+        // so the application can refuse to save and preserve the file.
+        tracing::error!(
             size = metadata.len(),
             max = MAX_FILE_SIZE,
             path = %path.display(),
-            "Tab state file too large, ignoring"
+            "Tab state file too large, refusing to load"
         );
-        return Ok(PersistedTabState::default());
+        return Err(std::io::Error::new(
+            std::io::ErrorKind::InvalidData,
+            format!(
+                "Tab state file size {} exceeds maximum allowed size {}",
+                metadata.len(),
+                MAX_FILE_SIZE
+            ),
+        ));
     }
 
     let content = std::fs::read_to_string(path)?;
@@ -215,6 +230,7 @@ mod tests {
                 to_tab: 2,
                 description: "Review code".to_string(),
                 priority: Priority::High,
+                status: DelegationStatus::Pending,
                 created_at: Utc::now(),
                 completed_at: None,
                 result: None,
@@ -307,9 +323,12 @@ mod tests {
         }
         std::fs::write(&path, content).unwrap();
 
-        // Should return default state, not panic
-        let loaded = load_from_file(&path).unwrap();
-        assert!(loaded.tabs.is_empty());
+        // Should return an error rather than silently overwriting the file
+        // on next save. Silently returning a default would destroy the
+        // user's data.
+        let result = load_from_file(&path);
+        assert!(result.is_err());
+        assert_eq!(result.unwrap_err().kind(), std::io::ErrorKind::InvalidData);
 
         let _ = std::fs::remove_file(&path);
         let _ = std::fs::remove_dir(&dir);