fix(agent,lua,storage,ui): harden agent spawn and session recursion - #327
Conversation
…sion # Conflicts: # n00n-lua/src/api/agent.rs
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds bounded session lineage, caller identity propagation, direct tool bootstrapping, nested dispatch tracking, lifecycle persistence, and orchestration-tool filtering across Lua, agent, storage, UI, plugins, and configuration layers. ChangesSession lineage and orchestration
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant LuaPlugin
participant EventLoop
participant SessionLineageGuard
participant AgentLoop
participant Agent
LuaPlugin->>EventLoop: create session with caller and bootstrap
EventLoop->>SessionLineageGuard: reserve and validate child
EventLoop->>AgentLoop: spawn identity-aware runtime
AgentLoop->>Agent: run direct tool
Agent-->>EventLoop: emit lifecycle and completion event
EventLoop->>SessionLineageGuard: release or update lineage activity
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: 0ee6fb8 | Previous: 4416c7b | Ratio |
|---|---|---|---|
fib/jit_mlua_hook |
7220471 ns/iter (± 362606) |
6606586 ns/iter (± 346043) |
1.09 |
fib/jit_watchdog |
1481293 ns/iter (± 10587) |
2475729 ns/iter (± 42461) |
0.60 |
fib/jit_none |
1478712 ns/iter (± 13881) |
2469806 ns/iter (± 61687) |
0.60 |
fib/interp_mlua_hook |
8106078 ns/iter (± 332068) |
7998577 ns/iter (± 165861) |
1.01 |
fib/interp_watchdog |
2877602 ns/iter (± 23052) |
3865826 ns/iter (± 32246) |
0.74 |
fib/interp_none |
2870443 ns/iter (± 16188) |
3751872 ns/iter (± 68104) |
0.77 |
buffer_rw/jit_mlua_hook |
731701 ns/iter (± 10785) |
564783 ns/iter (± 9734) |
1.30 |
buffer_rw/jit_watchdog |
80326 ns/iter (± 517) |
168065 ns/iter (± 4049) |
0.48 |
buffer_rw/jit_none |
80545 ns/iter (± 517) |
167992 ns/iter (± 1332) |
0.48 |
buffer_rw/interp_mlua_hook |
1083199 ns/iter (± 68863) |
1110018 ns/iter (± 11704) |
0.98 |
buffer_rw/interp_watchdog |
456674 ns/iter (± 1002) |
655420 ns/iter (± 6550) |
0.70 |
buffer_rw/interp_none |
457592 ns/iter (± 715) |
655785 ns/iter (± 4913) |
0.70 |
splash_render_120x40 |
52674 ns/iter (± 3243) |
61601 ns/iter (± 7686) |
0.86 |
splash_render_200x60 |
135135 ns/iter (± 5831) |
131089 ns/iter (± 25095) |
1.03 |
This comment was automatically generated by workflow using github-action-benchmark.
|
@codex review |
![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: e244668b69
ℹ️ 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".
- Accept origin/main for SessionIdentity/identity types - Accept origin/main for plugin_state_store/context_liveness - Accept origin/main for set_deadline Option - Accept origin/main for active_tools - Accept origin/main for apply_body_overrides - Accept origin/main for reasoning_effort_fields - Keep PR's agent spawn and session recursion hardening - Keep PR's nested_only parameter in spawn_runtime_request - Keep PR's deferred plugin state store read - Accept origin/main for generated baselines/docs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: caf8dfcb07
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a9e74680d
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
n00n-ui/src/event_loop.rs (3)
718-722: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSkip stored sessions with invalid lineage instead of aborting startup.
The loop already tolerates two failure modes.
AppSession::loadfailures push a startup warning and continue at Line 665.resolved_rootfailures push a startup warning and continue at Line 703.live_sessionfailures at Line 718 use?and abortEventLoop::new, so the TUI does not start.One historical session with a malformed lineage record therefore blocks the whole application. Apply the same warn-and-skip handling.
🐛 Proposed fix
- let mut node = live_session(&session) - .map_err(|error| eyre!("invalid stored session lineage: {error}"))?; + let mut node = match live_session(&session) { + Ok(node) => node, + Err(error) => { + startup_warnings.push(format!( + "Skipped stored session {} with invalid lineage: {error}", + session.id + )); + continue; + } + }; node.runtime_present = false; node.execution_active = false; live_sessions.push(node);🤖 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 `@n00n-ui/src/event_loop.rs` around lines 718 - 722, Update the stored-session loop around live_session so invalid lineage errors are recorded as startup warnings and the affected session is skipped, matching the existing AppSession::load and resolved_root failure handling. Remove the error propagation that aborts EventLoop::new while preserving normal processing for valid sessions.
1543-1563: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winA single load failure aborts the remaining cancel targets.
The loop iterates over
targetand all its descendants. For a target with no live runtime,latest_snapshotandAppSession::loadboth use?. If one descendant has no persisted session file, the closure returns immediately.Earlier targets in the loop are already cancelled and saved. Later targets keep their active lifecycle and queued work, and the reply reports a failure. The caller cannot tell which sessions were cancelled.
Record the failure per target and continue, then report the aggregate result after the loop.
🤖 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 `@n00n-ui/src/event_loop.rs` around lines 1543 - 1563, Update the cancellation loop around `latest_snapshot`, `AppSession::load`, and `cancel_stored_session` so a persistence failure is recorded for the current `session_id` instead of propagated with `?`. Continue processing all remaining targets, then return the aggregate failure after the loop while preserving successful cancellations and saves.
1414-1417: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrop plugin state when a new session is aborted.
spawn_runtimehydrates plugin state for the new session at Lines 367-379. Both abort paths tear the runtime down without releasing that state. Line 1415 calls onlyruntime.handles.cancel(). Line 1459-1460 callsremove_runtimeandcancel().
SessionRequest::Deleteat Line 1283 callsrt.app.drop_plugin_state(runtime_id)for this reason, andshutdowncallshandle.drop_state_ownerat Line 2205. Without the same call here, the Lua host keeps state for a session that no longer exists.🐛 Proposed fix
let id = runtime.id(); if let Err(error) = self.lineage.commit_new(reservation, id) { + runtime.app.drop_plugin_state(id); runtime.handles.cancel(); return Err(error.to_string()); }if let Err(error) = start_result { - let runtime = self.remove_runtime(idx); + let mut runtime = self.remove_runtime(idx); + runtime.app.drop_plugin_state(id); runtime.handles.cancel();Also applies to: 1458-1467
🤖 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 `@n00n-ui/src/event_loop.rs` around lines 1414 - 1417, Release the hydrated plugin state on both new-session abort paths in spawn_runtime: after the lineage commit failure before returning the error, and alongside remove_runtime/cancel() in the later abort path. Reuse the existing runtime identifier with app.drop_plugin_state, matching SessionRequest::Delete and shutdown’s state-owner cleanup.site/docs/content/lua-api/_index.md (1)
2809-2811: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClarify the direct-bootstrap option contract.
n00n.session.new()is ambiguous at this spot: the documentation needs the exact accepted keys and whethertoolandinputmust both be supplied for host-executed bootstrap. Keep each option on its own line so field optionality and conditions are easy for readers to scan.🤖 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 `@site/docs/content/lua-api/_index.md` around lines 2809 - 2811, Update the n00n.session.new() option documentation to list parent_id, tool, input, and title as distinct entries, explicitly defining each type and optionality; state that direct host-executed bootstrap requires both tool and input, while title remains optional.Source: Coding guidelines
n00n-ui/src/agent/agent_loop.rs (1)
238-296: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRestore
clear_cancel_triggeron therun_toolerror path.
agent.run_tool(...).await?returns early whenrun_toolfails. This skipsself.clear_cancel_trigger(run_id)on Line 282, so the entry set byself.set_cancel_trigger(run_id, trigger)on Line 248 stays incancel_mapforever for thatrun_id.do_agent_runavoids this exact pitfall: it stores the result, unconditionally clears the cancel trigger, and only then propagates the error orCancelledstate.Restructure
do_direct_tool_runto clear the cancel trigger before returning on error.🐛 Proposed fix
- let result = agent - .run_tool(format!("bootstrap-{run_id}"), tool, input) - .await?; - drop(agent); - self.clear_cancel_trigger(run_id); + let result = agent + .run_tool(format!("bootstrap-{run_id}"), tool, input) + .await; + drop(agent); + self.clear_cancel_trigger(run_id); + let result = result?; if result.is_error {🤖 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 `@n00n-ui/src/agent/agent_loop.rs` around lines 238 - 296, Restructure do_direct_tool_run to store the result of agent.run_tool(...).await instead of propagating immediately, then drop the agent and call clear_cancel_trigger(run_id) unconditionally before handling the result. After cleanup, propagate the run error and preserve the existing result.is_error handling and Done event behavior.n00n-lua/src/runtime.rs (1)
3065-3092: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDrop the unused
nestedfield fromComputeHeaderandComputePermissionScopes.These request producers pass
nestedfromLuaToolInvocation, but current runtimes handleComputeHeader/ComputePermissionScopesinline and discardnested.spawn_runtime_requestalso ignores it after thenested_onlyfilter. Remove the field from these variants and senders unless they need lifecycle/deferred behavior likeCallToolandStartTool.🤖 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 `@n00n-lua/src/runtime.rs` around lines 3065 - 3092, Remove the unused nested field from the Request::ComputeHeader and Request::ComputePermissionScopes variants and update all corresponding LuaToolInvocation producers and pattern matches. Preserve their current inline handling and reply behavior, without adding lifecycle or deferred processing; leave nested support intact for requests such as CallTool and StartTool.src/cmd/tui_bridge.rs (2)
191-201: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a valid cancel forwarding test.
The malformed-ID test returns before
stop_onebuildsSessionRequest::Cancel. It does not verifycaller_id: Noneorhost_control: truefor a valid ID. Add a receiver-based test that consumes a valid cancel request and sends a reply. This catches a regression that would make host cancellation fail in the event loop. (raw.githubusercontent.com)Also applies to: 354-383
🤖 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 `@src/cmd/tui_bridge.rs` around lines 191 - 201, Add a receiver-based test for the valid-ID path in stop_one, using a valid SessionRef, consuming the emitted SessionRequest::Cancel, and replying through the request channel. Assert the request preserves caller_id: None and host_control: true, while retaining the existing malformed-ID coverage.
127-146: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve worker fallback for unhinted requests.
When
backendis omitted,ControlPlane::resolvecalls the TUI backend first and falls through toWorkerBackendonly forControlError::NotFound. These checks returnControlError::InvalidIdfor valid worker IDs such asworker-1, so defaultmessage,resume, andstoprequests never reachWorkerBackend. Keep strict validation for an explicitly selected TUI backend, but defer this format error until the other backend has been tried, or make the resolver distinguish an unhinted TUI miss. Add a mixed-backend regression withbackend: None. (raw.githubusercontent.com)Also applies to: 160-161, 191-201
🤖 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 `@src/cmd/tui_bridge.rs` around lines 127 - 146, Defer SessionRef format validation in the unhinted request paths used by message, resume_one, and stop so ControlPlane::resolve can fall through from the TUI backend to WorkerBackend for valid worker IDs such as “worker-1”. Preserve strict InvalidId behavior when backend explicitly selects TUI, and add a mixed-backend regression covering backend: None.
🤖 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-ui/src/event_loop.rs`:
- Around line 993-1024: Bound direct tool output before assigning it to session
metadata in the direct_bootstrap_active handling, including both ToolDone and
Error branches. Reuse the existing UI truncation behavior via
ui_config.tool_output_lines, or apply an explicit byte limit, so direct_output
remains within the session record size limit while preserving the error flag and
existing metadata flow.
- Around line 1443-1447: Route the set_execution_active failure in the prompt
branch through the existing start_result cleanup path instead of propagating it
with ?. Update the flow around commit_new, push_runtime, and start_result so
errors from set_execution_active trigger runtime removal, handle cancellation,
and rollback_new before returning.
In `@n00n-ui/src/session_lineage.rs`:
- Around line 198-202: Update both rollback branches around
self.rebuild_topology() so rollback failures are handled without propagating
them: log each rollback error, then return the original operation error captured
in error. Preserve the existing session removal and successful rollback
behavior.
In `@n00n-ui/src/storage_writer.rs`:
- Around line 257-263: Update latest_snapshot to receive the writer response
with recv_timeout using a LATEST_SNAPSHOT_TIMEOUT constant defined beside
RETRY_DELAY, and return a typed SessionError when the timeout expires so
event-loop callers can fall back to AppSession::load. Change the result and
Op::Latest response handling to use Arc<AppSession> and avoid cloning the full
session on the writer thread, while preserving existing writer-gone error
handling.
In `@site/docs/content/lua-api/_index.md`:
- Line 6008: Update the documentation for M.orchestration_tools() to state that
it returns a table of orchestration tool names, and add a concise usage example
showing callers using that table for tool filtering.
---
Outside diff comments:
In `@n00n-lua/src/runtime.rs`:
- Around line 3065-3092: Remove the unused nested field from the
Request::ComputeHeader and Request::ComputePermissionScopes variants and update
all corresponding LuaToolInvocation producers and pattern matches. Preserve
their current inline handling and reply behavior, without adding lifecycle or
deferred processing; leave nested support intact for requests such as CallTool
and StartTool.
In `@n00n-ui/src/agent/agent_loop.rs`:
- Around line 238-296: Restructure do_direct_tool_run to store the result of
agent.run_tool(...).await instead of propagating immediately, then drop the
agent and call clear_cancel_trigger(run_id) unconditionally before handling the
result. After cleanup, propagate the run error and preserve the existing
result.is_error handling and Done event behavior.
In `@n00n-ui/src/event_loop.rs`:
- Around line 718-722: Update the stored-session loop around live_session so
invalid lineage errors are recorded as startup warnings and the affected session
is skipped, matching the existing AppSession::load and resolved_root failure
handling. Remove the error propagation that aborts EventLoop::new while
preserving normal processing for valid sessions.
- Around line 1543-1563: Update the cancellation loop around `latest_snapshot`,
`AppSession::load`, and `cancel_stored_session` so a persistence failure is
recorded for the current `session_id` instead of propagated with `?`. Continue
processing all remaining targets, then return the aggregate failure after the
loop while preserving successful cancellations and saves.
- Around line 1414-1417: Release the hydrated plugin state on both new-session
abort paths in spawn_runtime: after the lineage commit failure before returning
the error, and alongside remove_runtime/cancel() in the later abort path. Reuse
the existing runtime identifier with app.drop_plugin_state, matching
SessionRequest::Delete and shutdown’s state-owner cleanup.
In `@site/docs/content/lua-api/_index.md`:
- Around line 2809-2811: Update the n00n.session.new() option documentation to
list parent_id, tool, input, and title as distinct entries, explicitly defining
each type and optionality; state that direct host-executed bootstrap requires
both tool and input, while title remains optional.
In `@src/cmd/tui_bridge.rs`:
- Around line 191-201: Add a receiver-based test for the valid-ID path in
stop_one, using a valid SessionRef, consuming the emitted
SessionRequest::Cancel, and replying through the request channel. Assert the
request preserves caller_id: None and host_control: true, while retaining the
existing malformed-ID coverage.
- Around line 127-146: Defer SessionRef format validation in the unhinted
request paths used by message, resume_one, and stop so ControlPlane::resolve can
fall through from the TUI backend to WorkerBackend for valid worker IDs such as
“worker-1”. Preserve strict InvalidId behavior when backend explicitly selects
TUI, and add a mixed-backend regression covering backend: None.
🪄 Autofix
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: aa583860-69e8-49dc-92df-8df89dfa1b21
📒 Files selected for processing (19)
n00n-agent/src/types.rsn00n-lua/src/loader.rsn00n-lua/src/runtime.rsn00n-lua/tests/plugin_host.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/chat.rsn00n-ui/src/event_loop.rsn00n-ui/src/session_lineage.rsn00n-ui/src/storage_writer.rsplugins/workflow/init.luasite/docs/content/lua-api/_index.mdsrc/cmd/tui_bridge.rssrc/print.rssrc/sdk_mode.rs
💤 Files with no reviewable changes (1)
- n00n-ui/src/app/queue.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: Test (Windows)
- GitHub Check: Rustdoc
- GitHub Check: Build (Windows)
- GitHub Check: MSRV (1.97)
- GitHub Check: Coverage
- GitHub Check: Lint (Windows)
- GitHub Check: Test (macOS)
- GitHub Check: Test
- 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-agent/src/types.rsn00n-ui/src/chat.rssrc/sdk_mode.rssrc/print.rsn00n-ui/src/storage_writer.rsn00n-lua/src/loader.rsn00n-ui/src/app/mod.rsn00n-storage/src/sessions.rsn00n-ui/src/app/session.rsn00n-ui/src/agent/agent_loop.rsn00n-lua/tests/plugin_host.rsn00n-ui/src/agent/shared_queue.rssrc/cmd/tui_bridge.rsn00n-ui/src/session_lineage.rsn00n-lua/src/runtime.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
🔇 Additional comments (20)
n00n-storage/src/sessions.rs (1)
937-940: LGTM!Also applies to: 3331-3343, 4188-4201, 4798-4820
n00n-ui/src/session_lineage.rs (2)
93-162: LGTM!Also applies to: 216-345, 439-464, 494-561, 564-630, 646-922
403-423: 🎯 Functional CorrectnessNo change needed for tombstone traversal.
descendants_ofreturns tombstone children forSessionRequest::Delete, whileDeletealready adds the requested session itself before applying tombstone filtering downstream. The existing tests cover tombstone lineage for counts and deleted descendants.n00n-ui/src/event_loop.rs (1)
160-177: LGTM!Also applies to: 218-250, 310-317, 347-431, 457-462, 657-717, 753-753, 802-864, 962-975, 978-992, 1025-1070, 1072-1120, 1253-1307, 1327-1359, 1363-1442, 1448-1457, 1468-1542, 1564-1594, 1657-1667, 1686-1721, 1861-1909, 1911-1974, 2180-2227, 2279-2381
n00n-ui/src/app/session.rs (1)
27-35: LGTM!Also applies to: 51-72, 227-260, 271-306, 343-401
n00n-ui/src/storage_writer.rs (1)
117-120: LGTM!Also applies to: 204-211, 902-925
site/docs/content/lua-api/_index.md (1)
6027-6027: LGTM!plugins/workflow/init.lua (1)
405-424: LGTM!n00n-lua/src/runtime.rs (1)
19-20: LGTM!Also applies to: 154-259, 350-391, 663-720, 784-831, 1036-1204, 3450-4076
n00n-lua/tests/plugin_host.rs (1)
6098-6357: LGTM!n00n-lua/src/loader.rs (1)
663-676: LGTM!Also applies to: 977-1008
n00n-ui/src/agent/agent_loop.rs (2)
58-155: LGTM!Also applies to: 205-208
162-178: 🩺 Stability & AvailabilityNo change needed for the
QueueDrainedpath.Cancellation clears the shared queue before emitting
CancelAgent, so skipped stale entries do not reachAgentLoop::run()to suppressdrain_generation(). The agent queue respawn path preserves pending items unchanged rather than dropping post-cancellation runs.> Likely an incorrect or invalid review comment.n00n-ui/src/agent/shared_queue.rs (1)
13-13: LGTM!Also applies to: 76-354, 356-389, 520-550
n00n-ui/src/app/mod.rs (1)
215-237: LGTM!Also applies to: 1321-1355, 1960-1973
n00n-ui/src/chat.rs (1)
250-250: LGTM!n00n-agent/src/types.rs (1)
1042-1044: LGTM!src/print.rs (1)
370-370: LGTM!src/sdk_mode.rs (1)
1058-1058: LGTM!src/cmd/tui_bridge.rs (1)
15-15: LGTM!Also applies to: 396-407, 422-422, 442-447, 457-467, 489-491, 508-508, 548-552
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc88cf564e
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
n00n-ui/src/event_loop.rs (1)
384-449: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the duplicate plugin-state hydration.
spawn_runtimehydrates the root scope and, for child sessions, the child scope before Line 449.app.hydrate_plugin_state()then hydrates the current session scope again.Each runtime creation can run plugin hydration twice for the same identity and snapshot. This can duplicate plugin initialization or state side effects. Keep one hydration path and add root and child regression coverage.
🤖 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 `@n00n-ui/src/event_loop.rs` around lines 384 - 449, Remove the redundant app.hydrate_plugin_state() call at the end of spawn_runtime, preserving the existing root and child hydration performed through lua_event_handle. Add regression coverage for both root and child runtime creation to verify each plugin scope is hydrated exactly once.site/docs/content/lua-api/_index.md (1)
2809-2811: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the direct-bootstrap constraints.
State that
toolcannot be combined withprompt. State thatinputandtitlerequiretool. This prevents callers from sending an invalid request that the API rejects.As per coding guidelines: "User documentation should be warm, simple, concise, easy for non-native English speakers, story-oriented, without em-dashes, emojis, or an AI tone."
🤖 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 `@site/docs/content/lua-api/_index.md` around lines 2809 - 2811, Update the session parameter documentation near parent_id to state that tool cannot be used with prompt, and that input and title require tool. Keep the wording concise, warm, and clear for non-native English speakers, without changing the API behavior.Source: Coding guidelines
plugins/lib/n00n/subagent.lua (1)
25-33: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire a boolean to enable orchestration tools.
A non-boolean truthy value, such as
"false", bypasses the default denylist. Reject non-booleanopts.allow_orchestrationvalues. Enable recursive orchestration only when the value is exactlytrue.Proposed fix
function M.launch(ctx, opts) if not opts then return nil, "opts is required", nil, nil, nil end + if opts.allow_orchestration ~= nil and type(opts.allow_orchestration) ~= "boolean" then + return nil, "opts.allow_orchestration must be a boolean", nil, nil, nil + end🤖 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/lib/n00n/subagent.lua` around lines 25 - 33, Update excluded_tools so orchestration is enabled only when opts.allow_orchestration is exactly the boolean true; treat nil, false, and all non-boolean values—including truthy strings—as denied and populate the existing ORCHESTRATION_TOOLS denylist for them.
🤖 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.
Outside diff comments:
In `@n00n-ui/src/event_loop.rs`:
- Around line 384-449: Remove the redundant app.hydrate_plugin_state() call at
the end of spawn_runtime, preserving the existing root and child hydration
performed through lua_event_handle. Add regression coverage for both root and
child runtime creation to verify each plugin scope is hydrated exactly once.
In `@plugins/lib/n00n/subagent.lua`:
- Around line 25-33: Update excluded_tools so orchestration is enabled only when
opts.allow_orchestration is exactly the boolean true; treat nil, false, and all
non-boolean values—including truthy strings—as denied and populate the existing
ORCHESTRATION_TOOLS denylist for them.
In `@site/docs/content/lua-api/_index.md`:
- Around line 2809-2811: Update the session parameter documentation near
parent_id to state that tool cannot be used with prompt, and that input and
title require tool. Keep the wording concise, warm, and clear for non-native
English speakers, without changing the API behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d3e18a9c-93e2-4a42-99f8-af1bd189d6f3
📒 Files selected for processing (8)
n00n-lua/tests/plugin_host.rsn00n-ui/src/app/session.rsn00n-ui/src/app/tests.rsn00n-ui/src/event_loop.rsn00n-ui/src/session_lineage.rsn00n-ui/src/storage_writer.rsplugins/lib/n00n/subagent.luasite/docs/content/lua-api/_index.md
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Build (Windows)
- GitHub Check: Rustdoc
- GitHub Check: Test (macOS)
- GitHub Check: Coverage
- GitHub Check: Lint (macOS)
- GitHub Check: MSRV (1.97)
- GitHub Check: Lint (Windows)
- GitHub Check: Test (Windows)
- GitHub Check: Lint
- GitHub Check: Test
- 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-lua/tests/plugin_host.rsn00n-ui/src/app/tests.rsn00n-ui/src/app/session.rsn00n-ui/src/storage_writer.rsn00n-ui/src/event_loop.rsn00n-ui/src/session_lineage.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
🔇 Additional comments (8)
n00n-ui/src/session_lineage.rs (1)
5-5: LGTM!Also applies to: 117-162, 165-264, 281-389, 416-567, 609-679, 938-983
n00n-ui/src/event_loop.rs (1)
26-27: LGTM!Also applies to: 75-75, 168-210, 631-829, 999-1155, 1266-1738, 1879-1991, 2204-2227, 2366-2427
n00n-ui/src/app/session.rs (1)
15-15: LGTM!Also applies to: 52-74, 229-293, 345-403
n00n-ui/src/storage_writer.rs (1)
25-25: LGTM!Also applies to: 118-120, 205-212, 258-279, 689-695, 926-969
n00n-ui/src/app/tests.rs (1)
22-23: LGTM!Also applies to: 2469-2471
plugins/lib/n00n/subagent.lua (1)
13-22: LGTM!Also applies to: 161-215
site/docs/content/lua-api/_index.md (1)
6009-6014: LGTM!Also applies to: 6032-6032
n00n-lua/tests/plugin_host.rs (1)
3060-3060: LGTM!Also applies to: 6106-6106, 6191-6191, 6320-6320, 6351-6351
|
@codex review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
n00n-ui/src/event_loop.rs (1)
169-181: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCancel paused-team state when it is the only remaining work.
If
direct_paused_teamis set after the direct team tool completes,had_workis false when the lifecycle is terminal and all queues are empty.cancel_stored_sessionthen returns without clearing the resumable team state. The live cancellation predicate has the same gap near Line 1623.Include
direct_paused_team.is_some()in both cancellation predicates. Add a test for a terminal session that contains only paused-team state.Proposed fix
let had_work = session.meta.lifecycle.is_active() || !session.meta.queued_messages.is_empty() || !session.meta.queued_submissions.is_empty() - || !session.meta.queued_direct_tools.is_empty(); + || !session.meta.queued_direct_tools.is_empty() + || session.meta.direct_paused_team.is_some();|| !self.sessions[idx].app.queue.is_empty() || has_restorable_work(&self.sessions[idx].app.state.session) + || self.sessions[idx] + .app + .state + .session + .meta + .direct_paused_team + .is_some()🤖 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 `@n00n-ui/src/event_loop.rs` around lines 169 - 181, Update the had_work predicate in cancel_stored_session and the corresponding live cancellation predicate near the event-loop cancellation logic to include direct_paused_team.is_some(). Add a test covering a terminal session whose only remaining state is paused-team data, verifying cancellation clears that state.
🤖 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.
Outside diff comments:
In `@n00n-ui/src/event_loop.rs`:
- Around line 169-181: Update the had_work predicate in cancel_stored_session
and the corresponding live cancellation predicate near the event-loop
cancellation logic to include direct_paused_team.is_some(). Add a test covering
a terminal session whose only remaining state is paused-team data, verifying
cancellation clears that state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5e310cf5-7234-471b-95cb-9392234f683b
📒 Files selected for processing (2)
n00n-storage/src/sessions.rsn00n-ui/src/event_loop.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: Test
- GitHub Check: Docs
- GitHub Check: Test (macOS)
- GitHub Check: Build (Windows)
- GitHub Check: Lint (Windows)
- GitHub Check: Lint (macOS)
- GitHub Check: Build
- GitHub Check: Coverage
- GitHub Check: MSRV (1.97)
- GitHub Check: Test (Windows)
- GitHub Check: Lint
- GitHub Check: Criterion
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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-storage/src/sessions.rsn00n-ui/src/event_loop.rs
🔇 Additional comments (2)
n00n-storage/src/sessions.rs (1)
941-942: LGTM!Also applies to: 4192-4216
n00n-ui/src/event_loop.rs (1)
79-79: LGTM!Also applies to: 297-339, 366-366, 415-420, 1065-1069, 1393-1401, 1560-1562, 1751-1760, 1951-1951, 1963-1971, 2009-2010, 2041-2041, 2353-2356, 2423-2458, 2461-2483
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b13fd78bf8
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
n00n/n00n-lua/src/api/session.rs
Line 114 in 81d3af7
Authorize callers before deleting sessions
When a Lua tool or command running in one session calls n00n.session.delete, this request carries only the supplied session ID, unlike prompt and cancel requests that attach the authoritative caller identity. Because IDs are discoverable through session.list() and session.live(), the event loop will cascade-delete any nonfocused session, including an unrelated root and its stored history, without a lineage or trusted-host authorization check.
AGENTS.md reference: AGENTS.md:L78-L78
n00n/n00n-ui/src/event_loop.rs
Lines 1669 to 1670 in 81d3af7
Read pending snapshots before renaming stored sessions
After cancelling a stored descendant, the cancel path queues its sanitized snapshot and replies before the writer necessarily flushes it. If that session is renamed immediately, this branch loads the older disk snapshot instead of storage_writer.latest_snapshot; its later-generation save can therefore restore the cancelled prompt or direct bootstrap, which executes when the session is focused again. Use the pending snapshot as the authoritative base before changing the title.
AGENTS.md reference: AGENTS.md:L78-L78
ℹ️ 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".
|
@coderabbitai review |
|
1 participant
Dirty changes: