mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
b77fe8fefe
## Why Once the repo-local lint exists, `codex-rs` needs to follow the checked-in convention and CI needs to keep it from drifting. This commit applies the fallback `/*param*/` style consistently across existing positional literal call sites without changing those APIs. The longer-term preference is still to avoid APIs that require comments by choosing clearer parameter types and call shapes. This PR is intentionally the mechanical follow-through for the places where the existing signatures stay in place. After rebasing onto newer `main`, the rollout also had to cover newly introduced `tui_app_server` call sites. That made it clear the first cut of the CI job was too expensive for the common path: it was spending almost as much time installing `cargo-dylint` and re-testing the lint crate as a representative test job spends running product tests. The CI update keeps the full workspace enforcement but trims that extra overhead from ordinary `codex-rs` PRs. ## What changed - keep a dedicated `argument_comment_lint` job in `rust-ci` - mechanically annotate remaining opaque positional literals across `codex-rs` with exact `/*param*/` comments, including the rebased `tui_app_server` call sites that now fall under the lint - keep the checked-in style aligned with the lint policy by using `/*param*/` and leaving string and char literals uncommented - cache `cargo-dylint`, `dylint-link`, and the relevant Cargo registry/git metadata in the lint job - split changed-path detection so the lint crate's own `cargo test` step runs only when `tools/argument-comment-lint/*` or `rust-ci.yml` changes - continue to run the repo wrapper over the `codex-rs` workspace, so product-code enforcement is unchanged Most of the code changes in this commit are intentionally mechanical comment rewrites or insertions driven by the lint itself. ## Verification - `./tools/argument-comment-lint/run.sh --workspace` - `cargo test -p codex-tui-app-server -p codex-tui` - parsed `.github/workflows/rust-ci.yml` locally with PyYAML --- * -> #14652 * #14651
332 lines
13 KiB
Rust
332 lines
13 KiB
Rust
//! Multi-agent picker navigation and labeling state for the TUI app.
|
|
//!
|
|
//! This module exists to keep the pure parts of multi-agent navigation out of [`crate::app::App`].
|
|
//! It owns the stable spawn-order cache used by the `/agent` picker, keyboard next/previous
|
|
//! navigation, and the contextual footer label for the thread currently being watched.
|
|
//!
|
|
//! Responsibilities here are intentionally narrow:
|
|
//! - remember picker entries and their first-seen order
|
|
//! - answer traversal questions like "what is the next thread?"
|
|
//! - derive user-facing picker/footer text from cached thread metadata
|
|
//!
|
|
//! Responsibilities that stay in `App`:
|
|
//! - discovering threads from the backend
|
|
//! - deciding which thread is currently displayed
|
|
//! - mutating UI state such as switching threads or updating the footer widget
|
|
//!
|
|
//! The key invariant is that traversal follows first-seen spawn order rather than thread-id sort
|
|
//! order. Once a thread id is observed it keeps its place in the cycle even if the entry is later
|
|
//! updated or marked closed.
|
|
|
|
use crate::multi_agents::AgentPickerThreadEntry;
|
|
use crate::multi_agents::format_agent_picker_item_name;
|
|
use crate::multi_agents::next_agent_shortcut;
|
|
use crate::multi_agents::previous_agent_shortcut;
|
|
use codex_protocol::ThreadId;
|
|
use ratatui::text::Span;
|
|
use std::collections::HashMap;
|
|
|
|
/// Small state container for multi-agent picker ordering and labeling.
|
|
///
|
|
/// `App` owns thread lifecycle and UI side effects. This type keeps the pure rules for stable
|
|
/// spawn-order traversal, picker copy, and active-agent labels together and separately testable.
|
|
///
|
|
/// The core invariant is that `order` records first-seen thread ids exactly once, while `threads`
|
|
/// stores the latest metadata for those ids. Mutation is intentionally funneled through `upsert`,
|
|
/// `mark_closed`, and `clear` so those two collections do not drift semantically even if they are
|
|
/// temporarily out of sync during teardown races.
|
|
#[derive(Debug, Default)]
|
|
pub(crate) struct AgentNavigationState {
|
|
/// Latest picker metadata for each tracked thread id.
|
|
threads: HashMap<ThreadId, AgentPickerThreadEntry>,
|
|
/// Stable first-seen traversal order for picker rows and keyboard cycling.
|
|
order: Vec<ThreadId>,
|
|
}
|
|
|
|
/// Direction of keyboard traversal through the stable picker order.
|
|
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
|
|
pub(crate) enum AgentNavigationDirection {
|
|
/// Move toward the entry that was seen earlier in spawn order, wrapping at the front.
|
|
Previous,
|
|
/// Move toward the entry that was seen later in spawn order, wrapping at the end.
|
|
Next,
|
|
}
|
|
|
|
impl AgentNavigationState {
|
|
/// Returns the cached picker entry for a specific thread id.
|
|
///
|
|
/// Callers use this when they already know which thread they care about and need the last
|
|
/// metadata captured for picker or footer rendering. If a caller assumes every tracked thread
|
|
/// must be present here, shutdown races can turn that assumption into a panic elsewhere, so
|
|
/// this stays optional.
|
|
pub(crate) fn get(&self, thread_id: &ThreadId) -> Option<&AgentPickerThreadEntry> {
|
|
self.threads.get(thread_id)
|
|
}
|
|
|
|
/// Returns whether the picker cache currently knows about any threads.
|
|
///
|
|
/// This is the cheapest way for `App` to decide whether opening the picker should show "No
|
|
/// agents available yet." rather than constructing picker rows from an empty state.
|
|
pub(crate) fn is_empty(&self) -> bool {
|
|
self.threads.is_empty()
|
|
}
|
|
|
|
/// Inserts or updates a picker entry while preserving first-seen traversal order.
|
|
///
|
|
/// The key invariant of this module is enforced here: a thread id is appended to `order` only
|
|
/// the first time it is seen. Later updates may change nickname, role, or closed state, but
|
|
/// they must not move the thread in the cycle or keyboard navigation would feel unstable.
|
|
pub(crate) fn upsert(
|
|
&mut self,
|
|
thread_id: ThreadId,
|
|
agent_nickname: Option<String>,
|
|
agent_role: Option<String>,
|
|
is_closed: bool,
|
|
) {
|
|
if !self.threads.contains_key(&thread_id) {
|
|
self.order.push(thread_id);
|
|
}
|
|
self.threads.insert(
|
|
thread_id,
|
|
AgentPickerThreadEntry {
|
|
agent_nickname,
|
|
agent_role,
|
|
is_closed,
|
|
},
|
|
);
|
|
}
|
|
|
|
/// Marks a thread as closed without removing it from the traversal cache.
|
|
///
|
|
/// Closed threads stay in the picker and in spawn order so users can still review them and so
|
|
/// next/previous navigation does not reshuffle around disappearing entries. If a caller "cleans
|
|
/// this up" by deleting the entry instead, wraparound navigation will silently change shape
|
|
/// mid-session.
|
|
pub(crate) fn mark_closed(&mut self, thread_id: ThreadId) {
|
|
if let Some(entry) = self.threads.get_mut(&thread_id) {
|
|
entry.is_closed = true;
|
|
} else {
|
|
self.upsert(
|
|
thread_id, /*agent_nickname*/ None, /*agent_role*/ None,
|
|
/*is_closed*/ true,
|
|
);
|
|
}
|
|
}
|
|
|
|
/// Drops all cached picker state.
|
|
///
|
|
/// This is used when `App` tears down thread event state and needs the picker cache to return
|
|
/// to a pristine single-session state.
|
|
pub(crate) fn clear(&mut self) {
|
|
self.threads.clear();
|
|
self.order.clear();
|
|
}
|
|
|
|
/// Returns whether there is at least one tracked thread other than the primary one.
|
|
///
|
|
/// `App` uses this to decide whether the picker should be available even when the collaboration
|
|
/// feature flag is currently disabled, because already-existing sub-agent threads should remain
|
|
/// inspectable.
|
|
pub(crate) fn has_non_primary_thread(&self, primary_thread_id: Option<ThreadId>) -> bool {
|
|
self.threads
|
|
.keys()
|
|
.any(|thread_id| Some(*thread_id) != primary_thread_id)
|
|
}
|
|
|
|
/// Returns live picker rows in the same order users cycle through them.
|
|
///
|
|
/// The `order` vector is intentionally historical and may briefly contain thread ids that no
|
|
/// longer have cached metadata, so this filters through the map instead of assuming both
|
|
/// collections are perfectly synchronized.
|
|
pub(crate) fn ordered_threads(&self) -> Vec<(ThreadId, &AgentPickerThreadEntry)> {
|
|
self.order
|
|
.iter()
|
|
.filter_map(|thread_id| self.threads.get(thread_id).map(|entry| (*thread_id, entry)))
|
|
.collect()
|
|
}
|
|
|
|
/// Returns the adjacent thread id for keyboard navigation in stable spawn order.
|
|
///
|
|
/// The caller must pass the thread whose transcript is actually being shown to the user, not
|
|
/// just whichever thread bookkeeping most recently marked active. If the wrong current thread
|
|
/// is supplied, next/previous navigation will jump in a way that feels nondeterministic even
|
|
/// though the cache itself is correct.
|
|
pub(crate) fn adjacent_thread_id(
|
|
&self,
|
|
current_displayed_thread_id: Option<ThreadId>,
|
|
direction: AgentNavigationDirection,
|
|
) -> Option<ThreadId> {
|
|
let ordered_threads = self.ordered_threads();
|
|
if ordered_threads.len() < 2 {
|
|
return None;
|
|
}
|
|
|
|
let current_thread_id = current_displayed_thread_id?;
|
|
let current_idx = ordered_threads
|
|
.iter()
|
|
.position(|(thread_id, _)| *thread_id == current_thread_id)?;
|
|
let next_idx = match direction {
|
|
AgentNavigationDirection::Next => (current_idx + 1) % ordered_threads.len(),
|
|
AgentNavigationDirection::Previous => {
|
|
if current_idx == 0 {
|
|
ordered_threads.len() - 1
|
|
} else {
|
|
current_idx - 1
|
|
}
|
|
}
|
|
};
|
|
Some(ordered_threads[next_idx].0)
|
|
}
|
|
|
|
/// Derives the contextual footer label for the currently displayed thread.
|
|
///
|
|
/// This intentionally returns `None` until there is more than one tracked thread so
|
|
/// single-thread sessions do not waste footer space restating the obvious. When metadata for
|
|
/// the displayed thread is missing, the label falls back to the same generic naming rules used
|
|
/// by the picker.
|
|
pub(crate) fn active_agent_label(
|
|
&self,
|
|
current_displayed_thread_id: Option<ThreadId>,
|
|
primary_thread_id: Option<ThreadId>,
|
|
) -> Option<String> {
|
|
if self.threads.len() <= 1 {
|
|
return None;
|
|
}
|
|
|
|
let thread_id = current_displayed_thread_id?;
|
|
let is_primary = primary_thread_id == Some(thread_id);
|
|
Some(
|
|
self.threads
|
|
.get(&thread_id)
|
|
.map(|entry| {
|
|
format_agent_picker_item_name(
|
|
entry.agent_nickname.as_deref(),
|
|
entry.agent_role.as_deref(),
|
|
is_primary,
|
|
)
|
|
})
|
|
.unwrap_or_else(|| {
|
|
format_agent_picker_item_name(
|
|
/*agent_nickname*/ None, /*agent_role*/ None, is_primary,
|
|
)
|
|
}),
|
|
)
|
|
}
|
|
|
|
/// Builds the `/agent` picker subtitle from the same canonical bindings used by key handling.
|
|
///
|
|
/// Keeping this text derived from the actual shortcut helpers prevents the picker copy from
|
|
/// drifting if the bindings ever change on one platform.
|
|
pub(crate) fn picker_subtitle() -> String {
|
|
let previous: Span<'static> = previous_agent_shortcut().into();
|
|
let next: Span<'static> = next_agent_shortcut().into();
|
|
format!(
|
|
"Select an agent to watch. {} previous, {} next.",
|
|
previous.content, next.content
|
|
)
|
|
}
|
|
|
|
#[cfg(test)]
|
|
/// Returns only the ordered thread ids for focused tests of traversal invariants.
|
|
///
|
|
/// This helper exists so tests can assert on ordering without embedding the full picker entry
|
|
/// payload in every expectation.
|
|
pub(crate) fn ordered_thread_ids(&self) -> Vec<ThreadId> {
|
|
self.ordered_threads()
|
|
.into_iter()
|
|
.map(|(thread_id, _)| thread_id)
|
|
.collect()
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use super::*;
|
|
use pretty_assertions::assert_eq;
|
|
|
|
fn populated_state() -> (AgentNavigationState, ThreadId, ThreadId, ThreadId) {
|
|
let mut state = AgentNavigationState::default();
|
|
let main_thread_id =
|
|
ThreadId::from_string("00000000-0000-0000-0000-000000000101").expect("valid thread");
|
|
let first_agent_id =
|
|
ThreadId::from_string("00000000-0000-0000-0000-000000000102").expect("valid thread");
|
|
let second_agent_id =
|
|
ThreadId::from_string("00000000-0000-0000-0000-000000000103").expect("valid thread");
|
|
|
|
state.upsert(main_thread_id, None, None, false);
|
|
state.upsert(
|
|
first_agent_id,
|
|
Some("Robie".to_string()),
|
|
Some("explorer".to_string()),
|
|
false,
|
|
);
|
|
state.upsert(
|
|
second_agent_id,
|
|
Some("Bob".to_string()),
|
|
Some("worker".to_string()),
|
|
false,
|
|
);
|
|
|
|
(state, main_thread_id, first_agent_id, second_agent_id)
|
|
}
|
|
|
|
#[test]
|
|
fn upsert_preserves_first_seen_order() {
|
|
let (mut state, main_thread_id, first_agent_id, second_agent_id) = populated_state();
|
|
|
|
state.upsert(
|
|
first_agent_id,
|
|
Some("Robie".to_string()),
|
|
Some("worker".to_string()),
|
|
true,
|
|
);
|
|
|
|
assert_eq!(
|
|
state.ordered_thread_ids(),
|
|
vec![main_thread_id, first_agent_id, second_agent_id]
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn adjacent_thread_id_wraps_in_spawn_order() {
|
|
let (state, main_thread_id, first_agent_id, second_agent_id) = populated_state();
|
|
|
|
assert_eq!(
|
|
state.adjacent_thread_id(Some(second_agent_id), AgentNavigationDirection::Next),
|
|
Some(main_thread_id)
|
|
);
|
|
assert_eq!(
|
|
state.adjacent_thread_id(Some(second_agent_id), AgentNavigationDirection::Previous),
|
|
Some(first_agent_id)
|
|
);
|
|
assert_eq!(
|
|
state.adjacent_thread_id(Some(main_thread_id), AgentNavigationDirection::Previous),
|
|
Some(second_agent_id)
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn picker_subtitle_mentions_shortcuts() {
|
|
let previous: Span<'static> = previous_agent_shortcut().into();
|
|
let next: Span<'static> = next_agent_shortcut().into();
|
|
let subtitle = AgentNavigationState::picker_subtitle();
|
|
|
|
assert!(subtitle.contains(previous.content.as_ref()));
|
|
assert!(subtitle.contains(next.content.as_ref()));
|
|
}
|
|
|
|
#[test]
|
|
fn active_agent_label_tracks_current_thread() {
|
|
let (state, main_thread_id, first_agent_id, _) = populated_state();
|
|
|
|
assert_eq!(
|
|
state.active_agent_label(Some(first_agent_id), Some(main_thread_id)),
|
|
Some("Robie [explorer]".to_string())
|
|
);
|
|
assert_eq!(
|
|
state.active_agent_label(Some(main_thread_id), Some(main_thread_id)),
|
|
Some("Main [default]".to_string())
|
|
);
|
|
}
|
|
}
|