feat(tui): show background tasks in the tasks pane - #512
Conversation
The pane was fed only by EngineEvent::SubagentUpdate, so a `&`-prefixed shell job, a workflow or an MCP monitor never appeared in it — the only way to see one was to type `/tasks list`. The pane claimed to be the place where running work is visible while showing a subset of it. Rows now carry a source and the pane groups them under "agents" and "background" headings. Subagent rows keep arriving as events; background rows are polled from the shared TaskManager, which emits none. Two things that matter more than the feature: The poll is gated on `live || !app.tasks.is_empty()`, matching how the animation tick is gated, so an idle session still never wakes. The sync returns whether anything actually changed and only marks the frame dirty then. Without that, a finished task sitting in the list would repaint the screen every 750ms forever and the zero-frame idle property would be quietly gone. Background rows are rebuilt wholesale rather than upserted, because the manager owns them: a task that disappears there has to disappear here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 086682b218
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .into_iter() | ||
| .map(|t| { |
There was a problem hiding this comment.
When an Agent call uses run_in_background, the query loop already emits a SubagentUpdate row keyed by subagent_id, while spawn_background_agent also registers the same run as a TaskKind::LocalAgent with a separate task ID. Mapping every manager record here therefore adds a second row for the same agent; moreover, the event-driven row remains working after the tool reports that it started in the background, while only the manager row eventually becomes done, leaving the pane indefinitely showing the agent as both working and finished. Filter or reconcile LocalAgent records rather than treating them as independent background work.
Useful? React with 👍 / 👎.
| app.flush_stream(); | ||
| } | ||
| // Background-task rows (`&` shell jobs, workflows, monitors). | ||
| _ = tasks_tick.tick(), if live || !app.tasks.is_empty() => { |
There was a problem hiding this comment.
If the first manager task is created near the end of a turn—or the turn is cancelled before the next 750 ms tick—app.tasks can still be empty when live becomes false. This guard then disables the only TaskManager poll indefinitely, and because the manager emits no event, the task never appears until another turn happens. A final sync on turn completion or a manager-driven wakeup is needed so short or late-starting background jobs cannot be missed.
Useful? React with 👍 / 👎.
| if last_source != Some(t.source) { | ||
| if last_source.is_some() { | ||
| lines.push(Line::from("")); | ||
| } | ||
| lines.push(Line::from(Span::styled( |
There was a problem hiding this comment.
On terminals narrower than 110 columns, render.rs:68-72 allocates only a five-row task strip. With one subagent and one background task, these new per-source headings plus the separator and two lines per task require seven rows; because agents sort first, the background task's status and headline are clipped even though this change is intended to expose it. The strip must grow, scroll, or render a more compact grouped layout.
Useful? React with 👍 / 👎.
The tasks pane listed work but offered no way into it — seeing that a build was running told you nothing about what it printed. Arrow keys move the selection while the pane is open and the composer is empty; Enter opens the selected task's output as a tool card in the transcript. The keys are gated the same way the queue pane gates its own, and yield to it when both are open, so nothing is double-bound. The read happens in the run loop rather than on the key path, because `read_output` is async and hits the filesystem. Only TaskManager rows can be drilled into. A subagent row's id is a subagent *type* rather than a task id, so there is no output to read for one; the pane says that instead of opening an empty card. Closing that gap needs the engine to capture per-subagent output, which is a larger change than a pane can make.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
… the pane - fold TaskManager LocalAgent records into their event-driven subagent row via a new payload subagent_id instead of listing the agent twice; folded rows keep the manager task id so drill-in still opens output - sync the manager once when a turn ends so a task registered moments before completion is not hidden behind the idle poll guard - size the narrow-terminal strip to the grouped list (it clipped the background group) and report hidden rows instead of silent truncation - scrub deceptive characters from pane headlines like other surfaces
|
@codex review Addressed all three findings on c649443:
Also routed pane headlines through the deceptive-character scrub (escape_deceptive), matching the transcript and permission surfaces. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c649443c89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (_, KeyCode::Enter) | ||
| if app.tasks_visible() && !app.show_queue_pane && app.input.is_empty() => | ||
| { | ||
| app.drill_into_selected_task(); |
There was a problem hiding this comment.
When the tasks pane is visible and the composer is empty, this modifier-agnostic arm also captures Ctrl+Enter before the later app.interject() arm. During a live turn with a queued prompt, Ctrl+Enter therefore opens task output instead of cancelling and sending the queued prompt as documented; restrict this arm to unmodified Enter so the global send-now binding remains reachable.
Useful? React with 👍 / 👎.
| task_id: Some(row.id), | ||
| }); | ||
| } | ||
| next.sort_by_key(|t| (t.source.heading(), t.state.order())); |
There was a problem hiding this comment.
When a polled task changes state, this sort can move rows while tasks_selected remains a positional index. For example, selecting the second working task and then having the first task finish can make the marker—and the subsequent Enter drill-in—silently target a different task; preserve the selected task ID across reconciliation and derive its new index afterward.
Useful? React with 👍 / 👎.
| // Drill-in: fetch the selected task's output off the UI path. | ||
| _ = std::future::ready(()), if app.pending_task_output.is_some() => { | ||
| if let Some(id) = app.pending_task_output.take() { | ||
| let out = task_manager.read_output(&id).await; |
There was a problem hiding this comment.
When a background task has a large output file or the cache filesystem is slow, awaiting this call blocks the sole TUI loop because TaskManager::read_output internally uses synchronous std::fs::read_to_string; terminal input, cancellation, and engine events cannot be handled until the entire file is read, after which the unbounded text is also inserted into the transcript. Run the read on a blocking worker or use async I/O, and bound or tail the displayed output.
AGENTS.md reference: AGENTS.md:L120-L125
Useful? React with 👍 / 👎.
| let cut = max_rows - 1; | ||
| let visible = task_ends.iter().filter(|&&e| e < cut).count(); | ||
| let hidden = app.tasks.len().saturating_sub(visible); | ||
| lines.truncate(cut); |
There was a problem hiding this comment.
When the pane cannot fit all tasks, this always truncates to the first rows even though Up/Down continues cycling tasks_selected through every task. Once the selection reaches a hidden task, the marker disappears and Enter opens output for a row the user cannot see; window or scroll the rendered rows around the selection rather than rendering a fixed prefix.
Useful? React with 👍 / 👎.
…ping - read task output via tokio::fs off the event loop (spawned task + channel) instead of a sync read under the manager lock, and tail long output to the last 200 lines in the transcript card - keep the pane selection attached to its task across re-sorts instead of the positional index silently retargeting Enter - claim only unmodified Up/Down/Enter for the pane so Ctrl+Enter interject and Alt+Enter newline stay reachable - window overflowed pane rows around the selection so the marked task is always visible
|
@codex review On 6762edf, addressing the four new findings:
Two findings from this round appear to be re-posts of items already fixed in c649443, unchanged in 6762edf — please re-check against the current code:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6762edf92e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(existing) = next.iter_mut().find(|t| t.agent_id == sid) { | ||
| existing.state = state; | ||
| existing.task_id = Some(row.id); |
There was a problem hiding this comment.
When two background Agent calls have the same description, resolve_subagent_id gives them the same ID because the tool schema does not expose subagent_id and the fallback is the description's first 32 characters. This lookup therefore folds both distinct manager records into one entry, repeatedly overwriting its state and task_id; only one task appears and Enter opens whichever output was processed last, with TaskManager's hash-map iteration making that choice unstable. Reconcile only one matching manager record with an event row and retain additional task IDs as separate rows.
Useful? React with 👍 / 👎.
| if lines.len() > max_rows && max_rows >= 2 { | ||
| let visible_h = max_rows - 1; | ||
| let sel_end = task_ends.get(app.tasks_selected).copied().unwrap_or(0); | ||
| let offset = sel_end | ||
| .saturating_sub(visible_h - 1) | ||
| .min(lines.len() - visible_h); |
There was a problem hiding this comment.
On a short terminal where the task block's inner height is two rows, reserving one row for the overflow message leaves visible_h == 1; anchoring the slice to task_ends then renders only the selected task's headline and omits its status row containing the ❯ marker. With an inner height below two, this branch is skipped and the paragraph clips from the list's beginning, which can hide the selected task entirely. The overflow window needs to prioritize the selected marker row when fewer than two task-content rows fit.
Useful? React with 👍 / 👎.
| // Background-task rows (`&` shell jobs, workflows, monitors). | ||
| _ = tasks_tick.tick(), if live || !app.tasks.is_empty() => { | ||
| let rows = manager_rows(&task_manager).await; | ||
| app.sync_background_tasks(rows); |
There was a problem hiding this comment.
After any event-driven subagent row is added, app.tasks never becomes empty because those rows are intentionally retained by reconciliation, so this guard keeps the 750 ms timer active for the remainder of the session even when the TaskManager has no running work. Every otherwise-idle wake snapshots, allocates, sorts, and compares the manager rows, defeating the event loop's documented zero-wakeup idle behavior; gate polling on manager work that can still change, rather than the combined pane contents.
Useful? React with 👍 / 👎.
… marker - a second LocalAgent record sharing a subagent id keeps its own row (manager rows are id-sorted, so the oldest task claims the event row) - gate the 750 ms poll on manager work that can still change, restoring zero idle wakeups once persistent subagent rows exist; seed the pane once at startup so adopted tasks show before the first turn - prioritize the selection's marker row when the overflow window is one or two rows tall
|
@codex review On fcef7a9, addressing this round's three new findings:
Restating from the previous round — these three re-posted findings were already fixed and remain fixed at this head; please verify against current code rather than the pre-fix context:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcef7a9d44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .filter(|t| t.source == TaskSource::Subagent) | ||
| .cloned() |
There was a problem hiding this comment.
When two live Agent calls share a subagent ID, the first sync creates an extra manager-backed row whose source is also Subagent; on the next 750 ms sync this filter copies that synthesized row, but reconciliation cannot claim it by the shared ID and appends the same task again. The modern TUI therefore grows by one duplicate on every poll while the extra task remains working. Preserve only event-owned rows here, or explicitly reuse existing rows by task_id when rebuilding.
Useful? React with 👍 / 👎.
| // The manager's map iterates in arbitrary order; sort by task id so | ||
| // reconciliation (which record folds into an event row) and row | ||
| // order are stable across polls. | ||
| rows.sort_by(|a, b| a.id.cmp(&b.id)); |
There was a problem hiding this comment.
TaskManager::allocate_id emits unpadded IDs such as a9 and a10, so this lexical comparison places a10 before a9. For same-subagent records crossing a digit boundary, the newer task consequently claims the event row despite the stated oldest-wins behavior, and pane order is not chronological. Sort by the parsed numeric suffix, with a deterministic fallback for legacy IDs.
Useful? React with 👍 / 👎.
| _ = tasks_tick.tick(), if live || app.has_live_manager_tasks() => { | ||
| let rows = manager_rows(&task_manager).await; | ||
| app.sync_background_tasks(rows); |
There was a problem hiding this comment.
/tasks clear
In the modern TUI, after a killed task has been polled into its terminal state, /tasks clear removes it from TaskManager while this guard remains false because no manager-backed row is working. The slash-command bridge does not perform its own pane sync, so the removed task remains visible indefinitely and drilling into it reports “Task not found” until another turn happens to trigger a sync. Manager-mutating slash commands need an explicit refresh without reintroducing an idle timer.
Useful? React with 👍 / 👎.
…d resync - re-claim manager-backed rows by their task id so identical polls converge instead of appending a duplicate per tick; rows whose record vanished (/tasks clear) are dropped, pure event rows stay - sort manager rows by the id's numeric sequence (a9 before a10) so the oldest-wins fold and pane order survive digit boundaries - resync the pane after every slash command so /tasks kill and clear reflect immediately even while the gated poll is parked
|
@codex review On 0e28204, addressing this round's three new findings:
Re-posted findings already fixed at this head (unchanged code, verified): turn-reap final sync (run.rs), pane strip sized from tasks::pane_rows (render.rs), selection re-derived by task identity after every reconcile (app.rs), and the poll gated on has_live_manager_tasks rather than pane emptiness (run.rs). Please evaluate against the current head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e28204e26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (m, KeyCode::Enter) | ||
| if m.is_empty() | ||
| && app.tasks_visible() | ||
| && !app.show_queue_pane | ||
| && app.input.is_empty() => |
There was a problem hiding this comment.
When a turn aborts or errors, the loop retains queued prompts and explicitly tells the user to press Enter to send them (lines 670–673). If any task row is visible—which is the default once tasks exist—and the queue pane is closed, this earlier arm captures that Enter and only opens task output, so the retained prompt is never dispatched. Prioritize a nonempty queue while idle or exclude that state from the drill-in guard.
Useful? React with 👍 / 👎.
|
@codex review On e266dee, addressing the one new finding: the pane's Enter arm now also requires an empty queue, so "queued prompts kept — press Enter to send" dispatches the retained prompt as promised; drill-in claims Enter only when nothing is waiting. Regression test added (queued_prompts_take_enter_before_task_drill_in). The other five findings on 0e28204 were re-posts of items already fixed in the reviewed code — each is covered by an existing regression test at this head: turn-reap final sync; strip sized from tasks::pane_rows (narrow_terminal_strip_shows_the_background_group); selection identity across re-sorts (selection_follows_its_task_across_a_resort); poll gated on has_live_manager_tasks (poll_gate_tracks_live_manager_work_only); shared-id convergence via claim-by-task-id (tasks_sharing_a_subagent_id_stay_distinct asserts row count stays 2 across repeated identical polls); slash-bridge pane resync after every command (run.rs, after commands::execute). Please verify against the current head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e266dee0b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let tm = task_manager.clone(); | ||
| let tx = task_out_tx.clone(); | ||
| tokio::spawn(async move { | ||
| let out = tm.read_output(&id).await; |
There was a problem hiding this comment.
When a selected task has produced a very large output file—or is still rapidly appending to one—this drill-in path calls read_output, which uses read_to_string to allocate the entire file before show_task_output discards all but 200 lines. Moving the read to a spawned task keeps input responsive but does not prevent memory exhaustion and can still terminate the TUI; read a bounded tail from the file instead of materializing the complete output.
Useful? React with 👍 / 👎.
|
@codex review On a3fadab, addressing the one new finding: added TaskManager::read_output_tail (seeks to the last N bytes, notes how many were skipped, never materializes the full file) and the drill-in path now reads a 256 KiB tail before the 200-line display trim. Test: read_output_tail_skips_the_head_of_large_output. The six other findings on e266dee are re-posts of items already fixed in the code under review, each with a regression test at this head (listed in the previous comment; the Enter/queue one is queued_prompts_take_enter_before_task_drill_in). Please evaluate against the current head — a thumbs-up or a finding on genuinely current code both work. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3fadab411
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match &row.task_id { | ||
| Some(id) => { | ||
| self.pending_task_output = Some(id.clone()); | ||
| self.status_message = format!("loading output for {id}…"); |
There was a problem hiding this comment.
When the selected task is still Working, this branch requests its output unconditionally, but run_command_task does not create or overwrite the .out file until finalize_shell_task runs after process exit (background.rs:897,915). Drilling into a newly running task therefore produces a read-error card instead of the intended “no output yet”; worse, because task IDs and the shared cache paths are reused across non-persistent process starts, an existing b1.out can display stale output from a previous run. Initialize/truncate the output file when registering the task, or prevent drill-in until output belongs to the current run.
Useful? React with 👍 / 👎.
Ids and cache paths recur across process starts, so a drill-in on a freshly running task could read ENOENT or a previous run's leftovers. Creating the file empty at registration makes both read as 'no output yet'.
|
@codex review On fa9afc9, addressing the one new finding: both registration paths (spawn_command and register/register_with_color) now truncate-create the task's output file up front, so drilling into a running task reads as empty ("no output yet") instead of ENOENT, and a recycled id can never surface a previous run's output. Test: registering_truncates_any_stale_output_file. The remaining six findings are verbatim re-posts of items already fixed and regression-tested at the reviewed head (detailed in prior comments). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa9afc9303
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Truncate up front: ids and cache paths recur across process | ||
| // starts, so a reader must never see a previous run's output — | ||
| // and reading before this run finishes means "empty", not ENOENT. | ||
| let _ = std::fs::File::create(&output_file); |
There was a problem hiding this comment.
When two Agent-Code processes run the same task kind, each TaskManager starts its counter at 1 and task_output_path places both records in the same global cache path (for example, both use a1.out). Consequently, registering a1 in the second process now truncates the first process's still-visible output immediately; there is no ownership check that distinguishes a stale file from one belonging to a live session. Use session-unique output paths or collision-resistant IDs rather than truncating solely by recycled task id.
Useful? React with 👍 / 👎.
| let unclaimed = next.iter().enumerate().position(|(i, t)| { | ||
| !claimed[i] | ||
| && (t.task_id.as_deref() == Some(row.id.as_str()) | ||
| || (t.task_id.is_none() && t.agent_id == sid)) |
There was a problem hiding this comment.
If a manager poll synthesizes a LocalAgent row before its queued SubagentUpdate is applied, the later event adds a second row. On subsequent polls this predicate chooses the earlier synthesized row through its matching task_id instead of folding into the unclaimed event row with agent_id == sid; when the manager row becomes terminal, polling can stop with both a stale working event row and a terminal synthesized row permanently visible. Prefer an unclaimed matching event row when one exists, and discard the synthesized row for the same task.
Useful? React with 👍 / 👎.
| // Truncate up front: ids and cache paths recur across process | ||
| // starts, so a reader must never see a previous run's output — | ||
| // and reading before this run finishes means "empty", not ENOENT. | ||
| let _ = std::fs::File::create(&output_file); |
There was a problem hiding this comment.
This synchronous filesystem operation runs directly inside async task registration, so a slow or stalled cache filesystem blocks the Tokio worker that is also driving tool execution and the TUI. Use tokio::fs::File::create(...).await (and handle the result) in both registration paths to preserve the repository's async-I/O invariant.
AGENTS.md reference: AGENTS.md:L125-L125
Useful? React with 👍 / 👎.
|
@codex review On 2e7baec, addressing two of the three new findings:
Declared deviation on the third (cross-session truncation): concurrent processes colliding on recycled ids/shared cache paths is a pre-existing property of the task id scheme — child stdout redirection already overwrites the same path regardless of this PR's truncate. Making ids collision-resistant or output paths session-scoped is a task-identity redesign beyond this pane PR; it should be a follow-up issue (happy to file it). The truncate here strictly improves the common single-session case (no ENOENT, no stale reads). The seven other findings are verbatim re-posts of items already fixed and regression-tested at the reviewed head; see prior comments for the mapping. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e7baeca84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| source: TaskSource, | ||
| ) { | ||
| let state = TaskState::parse(state); | ||
| if let Some(existing) = tasks.iter_mut().find(|t| t.agent_id == agent_id) { |
There was a problem hiding this comment.
When a subagent's explicit ID or description-derived ID matches an existing manager task ID (for example, a foreground Agent call described as b1 while background task b1 exists), this lookup updates the background row instead of inserting a subagent row because it ignores source. The next manager sync rebuilds that background row and discards the subagent update entirely, so the active agent disappears from the pane; restrict the match to the requested source (and preserve the separate manager-backed reconciliation logic).
Useful? React with 👍 / 👎.
|
@codex review On 86e18d6: upserts now match rows by id AND source, so an agent id colliding with a background task id creates its own row instead of hijacking (and then losing) the background row. Test: colliding_ids_across_sources_stay_separate_rows. The seven remaining findings are verbatim re-posts of items already fixed and regression-tested at the reviewed head (mapping in prior comments); the cross-session truncation deviation stands as declared. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86e18d6284
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| state, | ||
| headline: row.headline, | ||
| source: TaskSource::Subagent, | ||
| task_id: Some(row.id), |
There was a problem hiding this comment.
When a LocalAgentExecutor run is registered with the shared manager (the embeddable task-executor path), this synthesized row becomes drillable, but LocalAgentExecutor::execute only calls set_status after AgentTool.call and never writes the returned content to the task output file, unlike LocalWorkflowExecutor. Because registration pre-creates an empty .out file, pressing Enter on a completed LocalAgent row always renders (no output yet) instead of the subagent result; persist the result before marking the task terminal.
Useful? React with 👍 / 👎.
|
@codex review On 08172d7: LocalAgentExecutor now persists the subagent's result via TaskManager::write_output before setting terminal status, matching the workflow executor and the write_output contract, so drill-in on a completed executor-path agent shows the result instead of "(no output yet)". The seven remaining findings are verbatim re-posts of items already fixed and regression-tested at the reviewed head; mapping in prior comments. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08172d7fa2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Truncate up front: ids and cache paths recur across process | ||
| // starts, so a reader must never see a previous run's output — | ||
| // and reading before this run finishes means "empty", not ENOENT. | ||
| if let Err(e) = tokio::fs::File::create(&output_file).await { |
There was a problem hiding this comment.
When two CLI or embedded instances run under the same account, each TaskManager::new starts its counter at 1 while task_output_path uses the same global cache directory, so their first same-kind tasks share paths such as b1.out. This new eager File::create lets one instance immediately erase a completed or still-readable result owned by the other (and subsequent reads may expose the other task's output); use process/session-unique paths or coordinate ID allocation before truncating.
Useful? React with 👍 / 👎.
| if let Ok(r) = &outcome { | ||
| let _ = tm.write_output(id, &r.content).await; | ||
| } |
There was a problem hiding this comment.
When AgentTool.call returns a non-cancellation Err such as a timeout or spawn failure, this if let Ok skips write_output and then marks the record failed. Completion injection and task drill-in read the pre-created output file rather than the TaskStatus::Failed message, so the failure reason is lost and drill-in reports (no output yet); persist e.to_string() for error outcomes before setting terminal status, as the workflow executor does.
Useful? React with 👍 / 👎.
|
@codex review On 626b103: executor Err outcomes are now persisted to the task output as well, so a Failed row's drill-in shows the error rather than "(no output yet)". On the cross-manager truncation (re-raised as P1): the deviation declared two rounds ago stands. Concurrent managers sharing recycled ids and one global cache path collide with or without the pre-truncation — the child stdout redirect already overwrites the same file. The durable fix is session-scoped output paths or collision-resistant task ids, which is a task-identity redesign out of scope for this tasks-pane PR; I will file a follow-up issue referencing this thread. The remaining eight findings are verbatim re-posts of items fixed and regression-tested at the reviewed head (mapping in prior comments). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 626b103792
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn selected_task_key(&self) -> Option<(String, super::tasks::TaskSource)> { | ||
| self.tasks | ||
| .get(self.tasks_selected) | ||
| .map(|t| (t.agent_id.clone(), t.source)) |
There was a problem hiding this comment.
When a manager poll synthesizes a completed LocalAgent row before its delayed SubagentUpdate arrives, the selected row is keyed by its temporary agent_id (for example, a3). The next sync folds the manager record into the event row and deletes that synthetic row; because this key cannot recognize the replacement, keep_task_selection only clamps the old positional index. With another same-state agent in the pane, the selection can therefore move to that agent and Enter opens the wrong task's output. Use the manager task_id as the canonical selection identity whenever it exists.
Useful? React with 👍 / 👎.
|
@codex review On 3084ef3: the pane selection key now prefers the backing task id, so a selection survives the synthesized-row-to-event-row absorb where agent_id changes. Test extended: a_late_event_row_absorbs_the_synthesized_row asserts the selection lands on the absorbed row. Nine remaining findings are verbatim re-posts of items fixed and regression-tested at the reviewed head, plus the declared cross-manager truncation deviation (follow-up issue to come). Mapping in prior comments. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3084ef3a77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| name: "Task output".into(), | ||
| detail: id.to_string(), | ||
| result: Some(body), | ||
| is_error: false, |
There was a problem hiding this comment.
When a task is cleared or its output file becomes unreadable after drill-in starts, read_output_tail returns Err and this method formats a failure message, but the unconditional is_error: false renders the card with a green check and success styling. Derive is_error from the Result so users are not shown a successful “Task output” operation when the read actually failed.
Useful? React with 👍 / 👎.
|
@codex review On d23a494: the drill-in card now derives is_error from the read result, so a failed read renders with error styling. Test updated (an_unreadable_task_reports_the_error_instead_of_looking_empty asserts is_error). Note: the Coverage failure on 3084ef3 was services::oauth::tests::browser_launcher_reports_non_zero_exit_status — an OAuth test this PR does not touch, green on every prior head; a tarpaulin flake that this push re-runs. Ten remaining findings are verbatim re-posts of items fixed and regression-tested at the reviewed head, plus the declared cross-manager truncation deviation. Mapping in prior comments. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Update: drill-in added (D4-27, partial)
The pane listed work but offered no way into it — seeing that a build was running told you nothing about what it printed.
↑/↓move the selection while the pane is open and the composer is empty;Enteropens the selected task's output as a tool card in the transcript. The keys are gated exactly like the queue pane's and yield to it when both are open, so nothing is double-bound. The read happens in the run loop, not on the key path, becauseread_outputis async and touches the filesystem.Only
TaskManagerrows can be drilled into, and this is a real limit rather than an oversight: a subagent row's id comes fromresolve_subagent_id(input)— a subagent type, not a task id — so there is no output to read for one. The pane says so instead of opening an empty card (drilling_into_a_subagent_row_explains_there_is_no_output). Closing that properly needs the engine to capture per-subagent output, which is more than a pane change.So D4-27 is partially closed: drill-in exists for every row that has something to drill into.
Added tests: selection wrap/clamp including the empty-pane case, drill-in requesting the right id, the subagent refusal, output landing in the transcript, and an unreadable task reporting the error rather than looking empty.
578 bin tests pass; clippy and fmt clean.