fix(ui): isolate and restore agent sessions - #254
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (29)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds durable session ownership and lifecycle tracking, session-tree controls, interrupted-session recovery, paused team-run resumption, background-session visibility, and explore-only delegation enforcement. ChangesAgent delegation and session context
Session runtime and UI
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TaskPicker
participant EventLoop
participant SessionStorage
participant AgentRuntime
User->>TaskPicker: select session action
TaskPicker->>EventLoop: focus, cancel, resume, or continue session
EventLoop->>SessionStorage: load or update lifecycle metadata
EventLoop->>AgentRuntime: restore, cancel, or resume session tree
AgentRuntime-->>EventLoop: return runtime events and tool outputs
EventLoop->>SessionStorage: persist updated session state
EventLoop-->>TaskPicker: refresh session descriptors
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Criterion
Details
| Benchmark suite | Current: 736ed63 | Previous: b36e929 | Ratio |
|---|---|---|---|
fib/jit_mlua_hook |
5066531 ns/iter (± 80371) |
6455055 ns/iter (± 172769) |
0.78 |
fib/jit_watchdog |
1927566 ns/iter (± 8909) |
2494875 ns/iter (± 4312) |
0.77 |
fib/jit_none |
1893455 ns/iter (± 61940) |
2493442 ns/iter (± 15255) |
0.76 |
fib/interp_mlua_hook |
5841565 ns/iter (± 15779) |
7718671 ns/iter (± 72364) |
0.76 |
fib/interp_watchdog |
3005037 ns/iter (± 35159) |
4073914 ns/iter (± 16857) |
0.74 |
fib/interp_none |
3013945 ns/iter (± 6627) |
3956463 ns/iter (± 10333) |
0.76 |
buffer_rw/jit_mlua_hook |
430496 ns/iter (± 1423) |
554260 ns/iter (± 1328) |
0.78 |
buffer_rw/jit_watchdog |
130124 ns/iter (± 169) |
167782 ns/iter (± 628) |
0.78 |
buffer_rw/jit_none |
130173 ns/iter (± 471) |
167759 ns/iter (± 338) |
0.78 |
buffer_rw/interp_mlua_hook |
809197 ns/iter (± 18509) |
1044457 ns/iter (± 12359) |
0.77 |
buffer_rw/interp_watchdog |
495780 ns/iter (± 20330) |
629318 ns/iter (± 4016) |
0.79 |
buffer_rw/interp_none |
496679 ns/iter (± 11339) |
639390 ns/iter (± 4827) |
0.78 |
splash_render_120x40 |
56033 ns/iter (± 3749) |
66065 ns/iter (± 4263) |
0.85 |
splash_render_200x60 |
134447 ns/iter (± 11701) |
149400 ns/iter (± 22243) |
0.90 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@codex review @coderabbitai review |
|
✅ Action performedReview finished.
|
![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: 7bb5afbdb1
ℹ️ 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 result { | ||
| ChatEventResult::Done => { | ||
| self.status_bar.clear_flash(); | ||
| self.state.session.meta.lifecycle = SessionLifecycle::Succeeded; |
There was a problem hiding this comment.
When a TurnEnd message is queued while the current turn is streaming, AgentLoop immediately starts that message after emitting Done for the first turn. This assignment persists Succeeded, while the subsequent QueueItemConsumed event only draws the message and never restores Running. The session therefore appears completed, disables cancellation in the task picker, and can be restored as successfully finished even while the queued provider call is active. Set the lifecycle back to Running when a deferred item is consumed, or only mark success after the queue is empty.
Useful? React with 👍 / 👎.
| runtime.app.set_agent_sessions(project_agent_sessions( | ||
| runtime.id(), | ||
| &self.stored_agent_sessions, | ||
| &descriptors, |
There was a problem hiding this comment.
The projection combines current runtime descriptors with a stored-session snapshot populated only at startup. If a background child is created during this process and the user later runs /new in that child, the runtime switches to a new session ID after saving the old one, but the old child is in neither descriptors nor stored_agent_sessions. It consequently disappears from its parent's Ctrl+X panel until restart despite remaining on disk. Archive the outgoing runtime descriptor into the stored set, or refresh the stored snapshot when runtimes change sessions.
Useful? React with 👍 / 👎.
|
| Filename | Overview |
|---|---|
| n00n-ui/src/event_loop.rs | Largest change: adds stored-session reconciliation at startup, sync_agent_sessions (called every tick), cancel_session_tree, resume_session, ensure_live_session, shutdown_lifecycle, and handlers for four new Action variants. One inconsistency: ContinueSubagent spawns a live session without checking MAX_LIVE_SESSIONS. |
| n00n-ui/src/agent/agent_loop.rs | Adds do_direct_tool for injecting a tool call directly into agent history (used by ResumeTeam) and is_paused_team_output. do_direct_tool hardcodes AgentMode::Build regardless of session mode; for team-resume that is likely harmless but undocumented. |
| n00n-ui/src/app/mod.rs | Adds AgentSessionEntry, TaskTarget, new TaskStatus variants, section grouping, and lifecycle writes across all terminal-event paths. Subagent prompt routing is now scoped to the active chat, fixing cross-agent input capture. |
| n00n-storage/src/sessions.rs | Adds SessionLifecycle and root_id to SessionMeta/SessionSummary; bumps scan cache to v4; cleans up stale v2/v3 caches; removes legacy parent-inference heuristic. Backward compatibility is tested. |
| n00n-agent/src/agent/tool_dispatch.rs | Adds delegation_policy_denied guard (checked before any tool runs) and unit tests covering all variants. Clean, well-tested addition. |
| n00n-lua/src/api/agent.rs | Propagates delegation_policy into SessionState; moves parent_cancels/child_id onto LuaSession so cancel can be sync; AgentEvent::Error is no longer silently swallowed and is now forwarded to the parent context. |
| plugins/sessions/init.lua | Rewrites build_tree to group descendants into synthetic category nodes (Research, Teams, Workflows, etc.) instead of direct parent-child nesting; adds a Recovered sessions group for malformed entries. |
| plugins/task/init.lua | Background task sessions now pass parent_id from ctx:session_id(), inherit delegation_policy = explore_only, and return a title field. Foreground task sessions also exclude delegation tools. |
| n00n-ui/src/app/session.rs | Relocates paused_team_run here (was in event_loop.rs); adds continued_subagent_session helper that copies parent/root ownership. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Agent session starts] -->|session_id propagated to ToolContext| B{Tool dispatched}
B -->|DelegationPolicy::ExploreOnly| C{Is delegation tool?\ntask / team / workflow / fusion_delegate}
C -->|Yes| D[Return DELEGATION_POLICY_DENIED error]
C -->|No| E[Run tool normally]
B -->|DelegationPolicy::Configured| E
E -->|team tool paused| F[SessionLifecycle::Paused written to disk]
F -->|ResumeTeam queued| G[QueueItem::ResumeTeam popped by agent_loop]
G --> H[do_direct_tool injects\nassistant+user messages into history]
H --> I[team plugin resumes run_id]
J[Startup: stored sessions scanned] -->|Running/WaitingInput found| K[reconcile_restored_lifecycle\nInterrupted, saved to disk]
K --> L[stored_agent_sessions populated]
L -->|sync_agent_sessions every tick| M[project_agent_sessions\nscopes to active root tree]
M --> N[Task picker shows background + live sessions]
N -->|Delete key| O[cancel_session_tree\ncancels id + descendants]
N -->|r key| P[resume_session → ensure_live_session\n→ push ResumeTeam]
N -->|Enter| Q[FocusSession → focus_session\n→ ensure_live_session]
Comments Outside Diff (1)
-
n00n-ui/src/agent/agent_loop.rs, line 793 (link)do_direct_toolhardcodesAgentMode::Buildfor everyResumeTeamdispatch, even if the session was started in a different mode. Becauseteamis only ever invoked in build mode this is harmless today, but if the mode is ever read inside theteamplugin (or a future caller reusesdo_direct_toolfor a different tool) the context would be silently wrong. Consider deriving the mode from the session's ownAgentModeinstead.Prompt To Fix With AI
This is a comment left during a code review. Path: n00n-ui/src/agent/agent_loop.rs Line: 793 Comment: `do_direct_tool` hardcodes `AgentMode::Build` for every `ResumeTeam` dispatch, even if the session was started in a different mode. Because `team` is only ever invoked in build mode this is harmless today, but if the mode is ever read inside the `team` plugin (or a future caller reuses `do_direct_tool` for a different tool) the context would be silently wrong. Consider deriving the mode from the session's own `AgentMode` instead. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
### Issue 1
n00n-ui/src/event_loop.rs:1726-1737
`ContinueSubagent` bypasses the `MAX_LIVE_SESSIONS` cap. Every other session-creation path (`SessionRequest::New`, `ensure_live_session`) checks `self.sessions.len() >= MAX_LIVE_SESSIONS` before calling `push_runtime`. Omitting that guard here means a user can exceed the 64-session ceiling by pressing 'r' on finished subagent chats.
```suggestion
Action::ContinueSubagent { name, messages } => {
if self.sessions.len() >= MAX_LIVE_SESSIONS {
self.focused_app().flash(format!(
"live session limit reached ({MAX_LIVE_SESSIONS})"
));
return;
}
let model = self.ctx.model_slot.load().model.spec();
let session = continued_subagent_session(
&self.sessions[idx].app.state.session,
&model,
&name,
messages,
);
let position = self.push_runtime(self.ctx.spawn_runtime(session));
self.sessions[position].app.save_session();
self.set_focus(position);
}
```
### Issue 2
n00n-ui/src/agent/agent_loop.rs:793
`do_direct_tool` hardcodes `AgentMode::Build` for every `ResumeTeam` dispatch, even if the session was started in a different mode. Because `team` is only ever invoked in build mode this is harmless today, but if the mode is ever read inside the `team` plugin (or a future caller reuses `do_direct_tool` for a different tool) the context would be silently wrong. Consider deriving the mode from the session's own `AgentMode` instead.
### Issue 3
n00n-ui/src/agent/agent_loop.rs:22
`TEAM_TOOL_NAME` is defined here, also defined as `pub(crate) const TEAM_TOOL_NAME` in `n00n-ui/src/app/session.rs`, and re-declared inside the `event_loop::tests` module. The three definitions are not shared so a future rename could drift. The `pub(crate)` copy in `session.rs` could be re-exported and reused here instead.
### Issue 4
n00n-ui/src/event_loop.rs:1003-1015
`sync_agent_sessions` is called on every tick and rebuilds the full descriptor list and runs `project_agent_sessions` (with ancestor traversal) for every live session. With `MAX_LIVE_SESSIONS = 64` and a large number of stored sessions that's O(n² + n·s) per tick. The `set_agent_sessions` equality guard prevents redundant UI redraws, but the computation itself runs unconditionally. Consider scheduling the sync only when a session's lifecycle or parent metadata actually changes (dirty flag / generation counter) rather than every tick.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "docs(lua): regenerate session API refere..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Action::ContinueSubagent { name, messages } => { | ||
| let model = self.ctx.model_slot.load().model.spec(); | ||
| let session = continued_subagent_session( | ||
| &self.sessions[idx].app.state.session, | ||
| &model, | ||
| &name, | ||
| messages, | ||
| ); | ||
| let position = self.push_runtime(self.ctx.spawn_runtime(session)); | ||
| self.sessions[position].app.save_session(); | ||
| self.set_focus(position); | ||
| } |
There was a problem hiding this comment.
ContinueSubagent bypasses the MAX_LIVE_SESSIONS cap. Every other session-creation path (SessionRequest::New, ensure_live_session) checks self.sessions.len() >= MAX_LIVE_SESSIONS before calling push_runtime. Omitting that guard here means a user can exceed the 64-session ceiling by pressing 'r' on finished subagent chats.
| Action::ContinueSubagent { name, messages } => { | |
| let model = self.ctx.model_slot.load().model.spec(); | |
| let session = continued_subagent_session( | |
| &self.sessions[idx].app.state.session, | |
| &model, | |
| &name, | |
| messages, | |
| ); | |
| let position = self.push_runtime(self.ctx.spawn_runtime(session)); | |
| self.sessions[position].app.save_session(); | |
| self.set_focus(position); | |
| } | |
| Action::ContinueSubagent { name, messages } => { | |
| if self.sessions.len() >= MAX_LIVE_SESSIONS { | |
| self.focused_app().flash(format!( | |
| "live session limit reached ({MAX_LIVE_SESSIONS})" | |
| )); | |
| return; | |
| } | |
| let model = self.ctx.model_slot.load().model.spec(); | |
| let session = continued_subagent_session( | |
| &self.sessions[idx].app.state.session, | |
| &model, | |
| &name, | |
| messages, | |
| ); | |
| let position = self.push_runtime(self.ctx.spawn_runtime(session)); | |
| self.sessions[position].app.save_session(); | |
| self.set_focus(position); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: n00n-ui/src/event_loop.rs
Line: 1726-1737
Comment:
`ContinueSubagent` bypasses the `MAX_LIVE_SESSIONS` cap. Every other session-creation path (`SessionRequest::New`, `ensure_live_session`) checks `self.sessions.len() >= MAX_LIVE_SESSIONS` before calling `push_runtime`. Omitting that guard here means a user can exceed the 64-session ceiling by pressing 'r' on finished subagent chats.
```suggestion
Action::ContinueSubagent { name, messages } => {
if self.sessions.len() >= MAX_LIVE_SESSIONS {
self.focused_app().flash(format!(
"live session limit reached ({MAX_LIVE_SESSIONS})"
));
return;
}
let model = self.ctx.model_slot.load().model.spec();
let session = continued_subagent_session(
&self.sessions[idx].app.state.session,
&model,
&name,
messages,
);
let position = self.push_runtime(self.ctx.spawn_runtime(session));
self.sessions[position].app.save_session();
self.set_focus(position);
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| use n00n_lua::EventHandle; | ||
| use n00n_providers::{AgentError, Message, Model, OpenAiOptions, System, TokenUsage}; | ||
| use n00n_providers::{ | ||
| AgentError, ContentBlock, Message, Model, OpenAiOptions, RequestOptions, Role, System, |
There was a problem hiding this comment.
TEAM_TOOL_NAME is defined here, also defined as pub(crate) const TEAM_TOOL_NAME in n00n-ui/src/app/session.rs, and re-declared inside the event_loop::tests module. The three definitions are not shared so a future rename could drift. The pub(crate) copy in session.rs could be re-exported and reused here instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: n00n-ui/src/agent/agent_loop.rs
Line: 22
Comment:
`TEAM_TOOL_NAME` is defined here, also defined as `pub(crate) const TEAM_TOOL_NAME` in `n00n-ui/src/app/session.rs`, and re-declared inside the `event_loop::tests` module. The three definitions are not shared so a future rename could drift. The `pub(crate)` copy in `session.rs` could be re-exported and reused here instead.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| self.tick_periodic_save(); | ||
| } | ||
|
|
||
| fn sync_agent_sessions(&mut self) { | ||
| let descriptors = self.runtime_descriptors(); | ||
| for runtime in &mut self.sessions { | ||
| runtime.app.set_agent_sessions(project_agent_sessions( | ||
| runtime.id(), | ||
| &self.stored_agent_sessions, | ||
| &descriptors, | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
sync_agent_sessions is called on every tick and rebuilds the full descriptor list and runs project_agent_sessions (with ancestor traversal) for every live session. With MAX_LIVE_SESSIONS = 64 and a large number of stored sessions that's O(n² + n·s) per tick. The set_agent_sessions equality guard prevents redundant UI redraws, but the computation itself runs unconditionally. Consider scheduling the sync only when a session's lifecycle or parent metadata actually changes (dirty flag / generation counter) rather than every tick.
Prompt To Fix With AI
This is a comment left during a code review.
Path: n00n-ui/src/event_loop.rs
Line: 1003-1015
Comment:
`sync_agent_sessions` is called on every tick and rebuilds the full descriptor list and runs `project_agent_sessions` (with ancestor traversal) for every live session. With `MAX_LIVE_SESSIONS = 64` and a large number of stored sessions that's O(n² + n·s) per tick. The `set_agent_sessions` equality guard prevents redundant UI redraws, but the computation itself runs unconditionally. Consider scheduling the sync only when a session's lifecycle or parent metadata actually changes (dirty flag / generation counter) rather than every tick.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@n00n-agent/src/tools/mod.rs`:
- Around line 303-308: Remove Copy from the derives on DelegationPolicy while
retaining Clone and the other traits. Update the ToolContext construction to
explicitly clone self.delegation_policy wherever the policy is passed by value.
In `@n00n-storage/src/sessions.rs`:
- Around line 1909-1918: Introduce a named ScannedMeta struct with title,
updated_at, first_message, root_id, and lifecycle fields, and change both
try_decode_last_meta_at and find_last_frame_meta to return Option<ScannedMeta>.
Update their construction, field access, and the call sites in
find_last_frame_meta and scan_zst_header to use named fields instead of
positional tuple destructuring.
- Around line 4050-4081: Add assertions to
session_lifecycle_is_backward_compatible_and_persisted covering
SessionLifecycle::Idle serialization: verify the idle lifecycle field is omitted
and deserializes back to Idle. Extend session_list_includes_parent_and_lifecycle
with a second list_in call and assert its parent_id, root_id, and lifecycle
match the first result, exercising the cached ScanCacheEntry path.
- Around line 1753-1754: Update both MetaScan decode sites that currently
construct MetaScan with title, updated_at, and meta so they deserialize only the
needed root_id and lifecycle fields directly into the scan structure. Remove the
flattened SessionMeta usage from MetaScan while preserving the existing title
and updated_at extraction and scan behavior.
In `@n00n-ui/src/agent/agent_loop.rs`:
- Around line 45-53: Centralize the paused team-result contract: in
n00n-ui/src/agent/agent_loop.rs lines 45-53, remove the private TEAM_TOOL_NAME
and local is_paused_team_output predicate, then import both from
crate::app::session; in n00n-ui/src/app/session.rs line 21, retain
TEAM_TOOL_NAME as the single pub(crate) definition and extract the
paused-payload validation from paused_team_run into a pub(crate) helper used by
both call sites.
In `@n00n-ui/src/app/mod.rs`:
- Around line 985-1031: Update the task-picker key handling around the
Delete/Backspace and Char('r') branches so unmatched picker input is delegated
to self.task_picker.handle_key(key). Preserve the existing cancel, resume, and
ContinueSubagent actions when their conditions match, but only return
immediately for those handled actions; otherwise fall through to the picker
handler.
- Around line 1943-1952: In the ChatEventResult::Done branch for chat_idx == 0,
synchronize the session state before checking paused_team_run, preserving the
refresh currently performed by save_session/session_snapshot. Determine whether
the run is paused, set lifecycle to Paused or Succeeded accordingly, then call
save_session only once after the final lifecycle is selected.
In `@n00n-ui/src/app/view.rs`:
- Line 328: Update the overlay-zone gate in view to require is_main_chat()
alongside the existing plan-form and layout conditions, matching the gates in
view and render_bottom_panel so subagent chat input areas remain selectable.
In `@n00n-ui/src/event_loop.rs`:
- Around line 1250-1294: Update the session-tree parent-map construction in
n00n-ui/src/event_loop.rs lines 1250-1294 to merge self.stored_agent_sessions
with live self.sessions before calling session_depth, preserving valid chains
through non-live ancestors. Apply the same merge in lines 1424-1449 before
calling is_descendant so live descendants are cancelled even when an
intermediate ancestor is stored rather than live.
- Line 986: Optimize tick’s sync_agent_sessions path by caching a fingerprint of
runtime descriptors and the stored session list, including titles, and returning
early when unchanged. Within each actual sync, build parents and explicit_roots
once from the full stored/live data and pass them into project_agent_sessions
for every runtime. Update project_agent_sessions and owned_session_root usage to
reuse these maps without allocating per-agent sets.
- Around line 2027-2048: Import SharedTranscript alongside the existing
n00n_agent type imports at the top of the file, then update sync_agent_mirrors
to use SharedTranscript directly in its transcript parameter type instead of
n00n_agent::SharedTranscript. Leave the rest of the function unchanged.
- Around line 226-249: Add a concise explanatory comment in owned_session_root
immediately before the `(!parents.contains_key(¤t)).then_some(root)`
return, documenting that an absent parent entry means the parent was not
restored and allows trusting the persisted root, while a present `None`
indicates a different root and must reject it.
- Line 2089: Update the event_loop.rs tests to import and use the shared
app-session TEAM_TOOL_NAME constant defined in session.rs, removing the local
duplicate declaration and replacing test references to the hardcoded team tool
name.
- Around line 1424-1449: Update cancel_session_tree to collect matching session
IDs rather than indices, then re-resolve each ID to its current position
immediately before calling cancel_current_run and dispatch. Preserve the
existing lifecycle and descendant filtering, and ensure the loop skips IDs no
longer present after earlier dispatch mutations.
- Around line 328-341: In control_status_json, replace the paused_team map_or
call using std::convert::identity with unwrap_or(Value::Null), and apply the
same simplification at the other paused_team occurrences around the referenced
status-building code. Leave the output handling unchanged.
- Around line 1451-1477: Update resume_session after ensure_live_session so any
failure to find a resumable team run rolls back the newly loaded runtime,
matching the cleanup performed by SessionRequest::New. Remove the runtime and
cancel its handles before returning the “This session has no resumable team run”
error, while preserving the existing resume flow when a valid run_id is found.
- Around line 1726-1737: Add MAX_LIVE_SESSIONS and MAX_SESSION_DEPTH checks to
the Action::ContinueSubagent branch before creating the
continued_subagent_session. Reuse the existing guard logic and flash-message
behavior from SessionRequest::New or ensure_live_session, returning without
spawning or focusing a runtime when either limit is reached.
In `@plugins/sessions/init.lua`:
- Around line 255-260: Ensure nil session IDs are validated before tree
construction begins, rather than relying on the by_id assignment in build_tree.
Add the validation in the shared normalize_session path or perform a complete
upfront check in refresh(), covering every session before any s.id/n.id table
indexing; preserve build_tree’s existing behavior for valid IDs.
In `@site/docs/content/lua-api/_index.md`:
- Line 819: Update the delegation_policy entry in the n00n.agent.session() API
documentation to list the supported values, “configured” and “explore_only,” and
state that “explore_only” blocks fusion_delegate, task, team, and workflow while
preserving the default of “configured.”
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 899fa0f9-4780-497d-86ff-c7b0d17bbd9d
📒 Files selected for processing (21)
changelog.d/agent-session-runtime.added.mdn00n-agent/src/agent/run.rsn00n-agent/src/agent/tool_dispatch.rsn00n-agent/src/tools/mod.rsn00n-lua/src/api/agent.rsn00n-lua/src/api/util/ctx.rsn00n-storage/src/sessions.rsn00n-ui/src/agent/agent_loop.rsn00n-ui/src/agent/shared_queue.rsn00n-ui/src/app/mod.rsn00n-ui/src/app/queue.rsn00n-ui/src/app/session.rsn00n-ui/src/app/tests.rsn00n-ui/src/app/view.rsn00n-ui/src/components/mod.rsn00n-ui/src/event_loop.rsplugins/lib/n00n/subagent.luaplugins/sessions/init.luaplugins/task/init.luaplugins/team/init.luasite/docs/content/lua-api/_index.md
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: Greptile Review
- GitHub Check: Coverage
- GitHub Check: Test
- GitHub Check: MSRV (1.97)
- GitHub Check: Lint
- GitHub Check: Docs
- GitHub Check: Build (Windows)
- GitHub Check: Build
- GitHub Check: Rustdoc
- GitHub Check: Test (macOS)
- GitHub Check: Lint (macOS)
- GitHub Check: Test (Windows)
- GitHub Check: Lint (Windows)
- GitHub Check: Criterion
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Do not add unsafe code, FFI, global mutable state,static mut, or unchecked transmute-like behavior without written review, an explicit lint exception, and a SAFETY comment where applicable.
Do not useunwrap,expect,panic!,todo!,unimplemented!, ordbg!in production Rust code; tests are exempt from the unwrap/expect/panic restriction.
Do not silently discard failures withunwrap_or,unwrap_or_default,.ok()onResult, or equivalent defaults; return typed errors, reject the operation, or use an explicitly named fallback with sanitized structured logging.
Use idiomatic Rust, descriptive names, minimal state, and avoid unnecessary comments, bloat, and magic numbers or strings.
Import types at the top of the file and use short imported names; keep constants immediately after imports.
UseResult<T, E>and explicit error handling instead of panics; usethiserrorfor library/domain errors andcolor-eyreat binary edges.
Use#[derive(Copy)]only for structs containing one primitive field.
Prefer structured logging with useful fields and provide helpful, sanitized error messages.
Place unit tests in the same file inside#[cfg(test)]modules; use#[test_case]and snake_case test names.
Propagate typed errors with?,ok_or_else, andmap_err; library crates usethiserrorand binaries usecolor-eyre.
Treat LLM and provider output as untrusted input; validate schemas, domain constraints, and source evidence before persistence or action.
Do not log raw provider payloads, prompts, credentials, or user session data, and never commit credentials, API keys, tokens, cookies, or auth headers.
Validate and authorize HTTP, file, queue, configuration/environment, LLM, and provider-callback inputs before mutation or persistence.
Tool execution requires allowlisted tools, scoped credentials, explicit user context, audit events, and refusal or denial tests.
Files:
n00n-ui/src/app/queue.rsn00n-lua/src/api/util/ctx.rsn00n-ui/src/app/session.rsn00n-ui/src/components/mod.rsn00n-agent/src/tools/mod.rsn00n-agent/src/agent/tool_dispatch.rsn00n-agent/src/agent/run.rsn00n-ui/src/app/view.rsn00n-ui/src/app/tests.rsn00n-lua/src/api/agent.rsn00n-ui/src/agent/agent_loop.rsn00n-ui/src/agent/shared_queue.rsn00n-storage/src/sessions.rsn00n-ui/src/app/mod.rsn00n-ui/src/event_loop.rs
site/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
User documentation should be warm, simple, concise, easy for non-native English speakers, story-oriented, without em-dashes, emojis, or an AI tone.
Files:
site/docs/content/lua-api/_index.md
🧠 Learnings (1)
📚 Learning: 2026-07-31T19:15:04.814Z
Learnt from: w0wl0lxd
Repo: w0wl0lxd/n00n PR: 206
File: changelog.d/orchestration-hardening.fixed.md:1-1
Timestamp: 2026-07-31T19:15:04.814Z
Learning: Files in changelog.d are changelog fragments intended for user-facing release notes and may begin directly with summary prose. Do not flag a missing Markdown H1 or require an H1 solely because Markdownlint MD041 reports it in these fragment files.
Applied to files:
changelog.d/agent-session-runtime.added.md
🪛 markdownlint-cli2 (0.23.1)
changelog.d/agent-session-runtime.added.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (58)
n00n-agent/src/agent/run.rs (1)
22-23: LGTM!Also applies to: 225-225, 283-283, 309-314, 887-887, 903-903
n00n-agent/src/tools/mod.rs (1)
310-334: LGTM!Also applies to: 586-586, 603-603
n00n-agent/src/agent/tool_dispatch.rs (1)
36-36: LGTM!Also applies to: 266-273, 381-383, 1046-1065
n00n-lua/src/api/agent.rs (1)
20-21: LGTM!Also applies to: 119-126, 632-632, 677-681, 827-827, 847-848, 869-869, 888-888, 906-907, 1380-1380, 1469-1470, 1560-1560, 1689-1691, 2224-2231
n00n-lua/src/api/util/ctx.rs (1)
240-250: LGTM!Also applies to: 403-403, 426-426, 451-461
plugins/lib/n00n/subagent.lua (1)
182-182: LGTM!plugins/task/init.lua (1)
22-22: LGTM!Also applies to: 91-103, 186-187, 226-226, 259-260
plugins/team/init.lua (1)
806-810: LGTM!Also applies to: 829-833
changelog.d/agent-session-runtime.added.md (1)
1-1: LGTM!n00n-storage/src/sessions.rs (8)
37-38: LGTM!
166-194: LGTM!
375-378: LGTM!
1771-1774: LGTM!Also applies to: 1851-1852
1868-1874: LGTM!
1931-1932: LGTM!Also applies to: 1948-1954, 1970-1970
2071-2090: LGTM!Also applies to: 2105-2105, 2126-2127
2379-2380: LGTM!Also applies to: 3690-3690, 3716-3717
n00n-ui/src/event_loop.rs (17)
10-12: LGTM!Also applies to: 24-26, 36-49, 72-73
136-165: LGTM!
180-225: LGTM!
251-326: LGTM!
344-388: Accept the projection logic; the per-tick cost is flagged at the caller.The ownership filtering is correct: stored and live parents merge, live descriptors override stored ones by id, and only descendants sharing
active_rootsurvive. The repeated map construction is a caller-side cost; see the comment onsync_agent_sessionsat lines 1006-1016.
390-412: LGTM!
494-494: LGTM!Also applies to: 713-757, 781-786, 809-809
828-834: LGTM!
1049-1050: LGTM!
1182-1184: LGTM!Also applies to: 1197-1199, 1216-1217
1315-1320: LGTM!
1409-1422: LGTM!
1490-1496: LGTM!
1713-1725: LGTM!
1946-1977: LGTM!
2066-2088: LGTM!Also applies to: 2091-2282, 2310-2345
166-178: 🩺 Stability & AvailabilityNo change needed. A paused team checkpoint is preserved as
Paused, soresume_sessioncan still find it.n00n-ui/src/app/mod.rs (12)
113-165: LGTM!
174-192: LGTM!
612-619: LGTM!Also applies to: 633-633, 645-647
688-697: LGTM!
699-756: LGTM!
758-791: LGTM!
860-861: LGTM!
974-984: LGTM!Also applies to: 1032-1047
1356-1387: LGTM!
1486-1486: LGTM!Also applies to: 1515-1515, 1609-1610
1898-1899: LGTM!Also applies to: 1915-1931
1933-1941: LGTM!Also applies to: 1965-1965
n00n-ui/src/app/queue.rs (1)
8-8: LGTM!Also applies to: 414-414
n00n-ui/src/app/view.rs (1)
33-34: LGTM!Also applies to: 80-85, 99-99, 134-142, 170-170, 397-398
n00n-ui/src/components/mod.rs (1)
39-39: LGTM!Also applies to: 209-221
n00n-ui/src/agent/agent_loop.rs (4)
12-24: LGTM!
251-343: LGTM!
655-662: LGTM!
213-221: 🗄️ Data Integrity & IntegrationNo change needed for the team resume schema.
goalis required andresumeis declared in theteamtool schema, so{"goal": "", "resume": resume_id}matches the accepted input shape.n00n-ui/src/agent/shared_queue.rs (1)
27-27: LGTM!Also applies to: 77-88, 109-120, 131-138, 184-184, 239-243, 301-301, 313-313, 346-346, 379-408
plugins/sessions/init.lua (3)
36-47: LGTM!
177-254: LGTM!Also applies to: 261-309
623-627: LGTM!n00n-ui/src/app/tests.rs (1)
19-19: LGTM!Also applies to: 559-641, 1386-1407, 1549-1677, 1700-1728, 3438-3455, 3464-3498, 3675-3675, 3919-3936, 3953-3957
| local function build_tree(sessions, expanded_state) | ||
| local by_id = {} | ||
| for _, s in ipairs(sessions) do | ||
| by_id[s.id] = s | ||
| for _, session in ipairs(sessions) do | ||
| by_id[session.id] = session | ||
| session.children = {} | ||
| end |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether normalize_session guarantees an id, and where ids originate.
rg -n -C6 'normalize_session' plugins/sessions/init.lua
rg -n -C3 '\.id\b' plugins/sessions/init.lua | head -60Repository: w0wl0lxd/n00n
Length of output: 2501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file size =="
wc -l plugins/sessions/init.lua
echo "== normalize_session body =="
sed -n '155,175p' plugins/sessions/init.lua
echo "== refresh/build_tree section =="
sed -n '175,315p' plugins/sessions/init.lua
echo "== all .id key usages =="
python3 - <<'PY'
from pathlib import Path
p=Path('plugins/sessions/init.lua')
for i,line in enumerate(p.read_text().splitlines(),1):
if '.id' in line:
print(f"{i}: {line}")
PYRepository: w0wl0lxd/n00n
Length of output: 6483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository references to sessions id fields =="
rg -n -C3 'session\.[^=\s]+|n00n\.session\.[^(]+' -g '*.lua' . | head -200
echo "== lua syntax nil-table-index probe =="
if command -v lua >/dev/null 2>&1; then
lua - <<'LUA'
pcall(function() local t = {}; t[nil] = 1 end)
print(`#pcall`(function() local t = {}; t[nil] = 1 end))
LUA
elif command -v luajit >/dev/null 2>&1; then
luajit - <<'LUA'
pcall(function() local t = {}; t[nil] = 1 end)
print(`#pcall`(function() local t = {}; t[nil] = 1 end))
LUA
else
echo "lua/luajit not available"
fiRepository: w0wl0lxd/n00n
Length of output: 12952
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib
p = pathlib.Path('plugins/sessions/init.lua')
needles = [
'n00n.session.live()',
'stored_map[st.id]',
"seen[s.id]",
'stored_map[s.id]',
'seen[st.id]',
'board.rank[s.id]',
'board.items and board.items[idx] and board.items[idx].id or nil',
'board.confirm ~= s.id',
'board.confirm = s.id',
'n00n.session.focus(s.id)',
'n00n.session.delete(s.id)',
'board.deleted[s.id]',
'find_stored(s.id)',
'board.rename = { id = s.id',
'local id = board.rename.id',
'n.id == id',
'board.deleted[st.id]',
]
for i, line in enumerate(p.read_text().splitlines(), 1):
for n in needles:
if n in line:
print(f"{i}: {line.strip()}")
break
PY
printf '\n== lines 560-605 and 105-110 =='
sed -n '560,605p' plugins/sessions/init.lua
sed -n '100,112p' plugins/sessions/init.luaRepository: w0wl0lxd/n00n
Length of output: 2292
Make nil session ids fail before building the tree.
refresh() indexes several tables by s.id / n.id before build_tree(), so a nil id is already rejected before line 258. build_tree() can fail separately, so either handle all nil ids up front or move this indexing check into normalize_session().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/sessions/init.lua` around lines 255 - 260, Ensure nil session IDs are
validated before tree construction begins, rather than relying on the by_id
assignment in build_tree. Add the validation in the shared normalize_session
path or perform a complete upfront check in refresh(), covering every session
before any s.id/n.id table indexing; preserve build_tree’s existing behavior for
valid IDs.
7bb5afb to
f1e45b0
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 736ed639fb
ℹ️ 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 decoder = Decoder::new(file).map_err(StorageError::from)?; | ||
| let mut reader = BufReader::new(decoder); |
There was a problem hiding this comment.
When a session .jsonl is corrupt or attacker-controlled, this unbounded decoder lets read_line grow the String to the full decompressed record and accepts arbitrary zstd window sizes; a small compressed file can therefore exhaust memory while sessions are listed or restored. This commit removes the prior decoded-byte, record-size, and window-log limits, so reinstate those limits before deserializing persisted file data.
AGENTS.md reference: AGENTS.md:L73-L78
Useful? React with 👍 / 👎.
|
@coderabbitai review |
✅ Action performedReview finished.
|
- Add MAX_LIVE_SESSIONS and MAX_SESSION_DEPTH checks to ContinueSubagent - Import shared TEAM_TOOL_NAME in tests instead of duplicating - Replace map_or(..., std::convert::identity) with unwrap_or(...) - Fix runtime_root and cancel_session_tree to include stored sessions in parent map - Fix cancel_session_tree to collect IDs instead of indices to avoid invalidation - Remove runtime when resume fails to prevent slot leak - Import SharedTranscript instead of qualifying inline - Document delegation_policy values in Lua API docs
- Do not truncate compaction history on transient server_overloaded errors. - Check MCP tool exclusion before permission enforcement in tool dispatch. - Replace disallowed unwrap_or calls with match expressions and a redundant closure with a method reference.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a02553fab5
ℹ️ 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 parents: HashMap<_, _> = self | ||
| .sessions | ||
| .iter() | ||
| .map(|runtime| (runtime.id(), runtime.app.state.session.meta.parent_id)) | ||
| .collect(); |
There was a problem hiding this comment.
When a user focuses a stored descendant while its root or parent remains unloaded, this map contains only live runtimes. session_depth follows the descendant's persisted parent_id, cannot find that stored ancestor, and returns None, so any background task or team call using n00n.session.new is rejected as an "invalid session parent chain". Extend this map with stored_agent_sessions, as the ownership and cancellation paths already do.
Useful? React with 👍 / 👎.
| QueueItem::ResumeTeam { resume_id, .. } => { | ||
| self.do_direct_tool( | ||
| &event_tx, | ||
| run_id, | ||
| TEAM_TOOL_NAME, | ||
| &serde_json::json!({ "goal": "", "resume": resume_id }), | ||
| ) |
There was a problem hiding this comment.
When the paused run originally used waves=true, pressing r submits only an empty goal and the resume ID. plugins/team/init.lua later sees input.waves == nil and selects run_autonomous instead of run_waves, so the saved wave_index and step_index are ignored and work can restart through the wrong executor. Preserve the original team options in the checkpoint or paused payload and restore them in this direct invocation.
Useful? React with 👍 / 👎.
|
This branch depends on the legacy session identity APIs (ToolContext.session_id, SessionMeta lifecycle/root_id, DelegationPolicy, TaskCell.session_id) that have been removed from main. Rebasing/merging would require a full rewrite against the new SessionIdentity model. Closing as stale/obsolete; a new PR should be opened if the feature is still needed. |
Pull request was closed
1 participant
Summary
Tests
cargo test -p n00n-ui --libcargo test -p n00n-storage --libcargo test -p n00n-agent --libcargo test -p n00n-lua --libRUST_TEST_THREADS=1 cargo test -p n00n-lua --test plugin_hostcargo test -p n00n-lua --test real_plugins_restorecargo check --allcargo clippy --all --tests -- -D warningscargo fmt --all -- --checkcargo nextest run --workspace(4628 passed, 1 skipped; one flaky UDS startup test failed, then passed alone)