feat(lua): add scoped plugin session state runtime - #320
Conversation
|
Warning Review limit reached
Next review available in: 23 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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds scoped Lua plugin state with bounded JSON snapshots, session identity propagation, lifecycle-aware runtime operations, storage preservation for malformed data, and cleanup on session or plugin removal. ChangesLua plugin state lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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: df82868 | Previous: 85788fe | Ratio |
|---|---|---|---|
fib/jit_mlua_hook |
6488886 ns/iter (± 261339) |
6449569 ns/iter (± 150630) |
1.01 |
fib/jit_watchdog |
2579156 ns/iter (± 44218) |
2445576 ns/iter (± 7006) |
1.05 |
fib/jit_none |
2577857 ns/iter (± 5961) |
2456614 ns/iter (± 7807) |
1.05 |
fib/interp_mlua_hook |
7901588 ns/iter (± 22739) |
7597814 ns/iter (± 33414) |
1.04 |
fib/interp_watchdog |
3779472 ns/iter (± 12675) |
3866238 ns/iter (± 80063) |
0.98 |
fib/interp_none |
3755839 ns/iter (± 26936) |
3766700 ns/iter (± 82952) |
1.00 |
buffer_rw/jit_mlua_hook |
555964 ns/iter (± 1215) |
554478 ns/iter (± 1724) |
1.00 |
buffer_rw/jit_watchdog |
168015 ns/iter (± 677) |
167914 ns/iter (± 253) |
1.00 |
buffer_rw/jit_none |
168075 ns/iter (± 246) |
167950 ns/iter (± 635) |
1.00 |
buffer_rw/interp_mlua_hook |
1075832 ns/iter (± 12367) |
1050245 ns/iter (± 9415) |
1.02 |
buffer_rw/interp_watchdog |
640828 ns/iter (± 4556) |
615666 ns/iter (± 9517) |
1.04 |
buffer_rw/interp_none |
638931 ns/iter (± 4338) |
615345 ns/iter (± 32005) |
1.04 |
splash_render_120x40 |
75931 ns/iter (± 3154) |
69990 ns/iter (± 6126) |
1.08 |
splash_render_200x60 |
133804 ns/iter (± 14411) |
140362 ns/iter (± 18432) |
0.95 |
This comment was automatically generated by workflow using github-action-benchmark.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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-338: Update the imports in n00n-agent/src/tools/mod.rs to add
SessionRef only if it is not already imported, then replace all six
n00n_storage::id::SessionRef references in SessionIdentity with the imported
SessionRef name, keeping constants immediately after the import section.
In `@n00n-lua/src/api/util/ctx.rs`:
- Around line 375-381: Extract the duplicated PluginStateScope parsing and
invalid-scope error construction from state_get, state_replace, and state_remove
into a shared parse_state_scope helper. Update all three methods to use the
helper while preserving their existing LuaValue::Nil and error-message behavior.
- Around line 412-427: Update state_replace and state_remove in
n00n-lua/src/api/util/ctx.rs at lines 412-427 and 442-453 to stop calling
store.get, destructure the prior-value payload returned by store.replace and
store.remove, and convert that returned value to Lua before returning it.
Preserve existing error handling and nil behavior when no prior value exists.
In `@n00n-lua/src/api/util/state_convert.rs`:
- Around line 71-88: Enable serde_json’s workspace/global dependency with the
arbitrary_precision feature so JsonValue::Number retains original numeric
literals. Keep the existing json_to_lua number validation in the
JsonValue::Number branch, ensuring out-of-range and high-precision values are
rejected as StateConvertError::UnrepresentableNumber.
In `@n00n-lua/src/runtime.rs`:
- Around line 1144-1174: Update the RuntimeWake::Closed branch in the runtime
drain loop to stop polling closed channels and wait only for lifecycle changes
once either spawn_rx or request_rx has closed. Preserve handling for
RuntimeWake::Spawn, RuntimeWake::Request, and RuntimeWake::Lifecycle, while
ensuring the loop yields instead of immediately repeating when a receiver is
closed.
- Around line 2576-2589: Update the Request::ClearPlugin handling so
host.unload() does not call rt.state.drop_plugin(&plugin) or otherwise mark the
plugin’s keys as managed for deletion. Preserve managed-key removal for the
explicit owner reset/clear path, while retaining captured plugin state across
unload and subsequent capture_state()/hydrate_state() operations.
In `@n00n-lua/src/state.rs`:
- Around line 280-367: Extract the shared base-snapshot reconstruction from
validate_replacement, validate_removal, and capture into a candidate_for helper
accepting inner, identity, and an optional skipped StateKey. Preserve the
existing identity filtering and value-versus-removal replay semantics, then have
each caller apply its replacement or removal delta after obtaining the candidate
so all three paths remain identical.
In `@n00n-lua/tests/plugin_host.rs`:
- Around line 4540-4551: Extract the repeated "state context is no longer
active" text into a named constant, such as STALE_CTX_ERR, placed immediately
after the imports. Replace all four occurrences in the affected assertions with
that constant, following the existing SESSION_CLOSED_ERR and
VALIDATION_PROMPT_NO_PROVIDER_ERR pattern.
- Around line 4464-4475: Extend the unload verification around
handle.capture_state to assert that plugin_a has no payload in
StoredStateScope::Session as well as Root, covering the session-a state
established earlier. Capture the post-unload state with a fresh revision greater
than the prior revision 8, and use that same revision consistently for both
scope assertions.
- Around line 4108-4153: Increase the delayed async job duration in the test
source used by the delayed_state invocation from 0.05 seconds to 0.5 seconds.
Keep the existing event wait and capture_state flow unchanged so the test
reliably exercises draining while the async callback remains in flight.
In `@n00n-storage/src/sessions.rs`:
- Around line 552-564: Remove the redundant public insert_plugin_state API, or
mark it #[deprecated] and direct callers to set_plugin_state while migration
occurs. Keep set_plugin_state as the single canonical implementation and avoid
duplicating its documentation or behavior.
- Around line 509-524: Update set_state_revision to avoid cloning and fully
validating the snapshot when the new revision has the same encoded size as the
current revision; only perform the existing size validation when the revision’s
serialized representation grows, while preserving regression checks and state
updates.
🪄 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: e9fc8033-07fa-4d53-9661-d70385d4ed90
📒 Files selected for processing (16)
changelog.d/320.added.mdn00n-agent/src/agent/run.rsn00n-agent/src/headless.rsn00n-agent/src/tools/mod.rsn00n-lua/src/api/agent.rsn00n-lua/src/api/util/ctx.rsn00n-lua/src/api/util/mod.rsn00n-lua/src/api/util/state_convert.rsn00n-lua/src/error.rsn00n-lua/src/lib.rsn00n-lua/src/loader.rsn00n-lua/src/runtime.rsn00n-lua/src/state.rsn00n-lua/tests/plugin_host.rsn00n-storage/src/sessions.rsn00n-ui/src/agent/agent_loop.rs
📜 Review details
🧰 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-lua/src/lib.rsn00n-lua/src/api/util/mod.rsn00n-lua/src/error.rsn00n-agent/src/headless.rsn00n-ui/src/agent/agent_loop.rsn00n-lua/src/loader.rsn00n-lua/src/api/util/state_convert.rsn00n-lua/tests/plugin_host.rsn00n-agent/src/agent/run.rsn00n-agent/src/tools/mod.rsn00n-lua/src/state.rsn00n-lua/src/api/agent.rsn00n-storage/src/sessions.rsn00n-lua/src/runtime.rsn00n-lua/src/api/util/ctx.rs
🧠 Learnings (2)
📚 Learning: 2026-07-31T05:40:20.137Z
Learnt from: w0wl0lxd
Repo: w0wl0lxd/n00n PR: 203
File: changelog.d/203.fixed.md:1-2
Timestamp: 2026-07-31T05:40:20.137Z
Learning: Files in changelog.d/ whose names begin with a numeric fragment identifier are headingless changelog fragments. Treat their contents as entry bodies because generated release sections provide the headings; do not report Markdown MD041 or add an H1 heading to these fragments. This does not apply to changelog.d/README.md.
Applied to files:
changelog.d/320.added.md
📚 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/320.added.md
🪛 markdownlint-cli2 (0.23.1)
changelog.d/320.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 (40)
n00n-lua/tests/plugin_host.rs (4)
15-16: LGTM!
4064-4094: LGTM!
4167-4191: LGTM!
4193-4262: LGTM!Also applies to: 4264-4363
changelog.d/320.added.md (1)
1-1: LGTM!n00n-agent/src/tools/mod.rs (1)
361-362: LGTM!Also applies to: 630-630, 672-673, 749-751, 792-818
n00n-agent/src/agent/run.rs (1)
22-24: LGTM!Also applies to: 171-171, 218-218, 277-277, 506-506, 890-890, 1149-1149, 2146-2146, 3359-3359
n00n-agent/src/headless.rs (1)
24-26: LGTM!Also applies to: 268-268, 523-523
n00n-ui/src/agent/agent_loop.rs (2)
12-13: LGTM!Also applies to: 59-59, 346-346
148-148: 🩺 Stability & AvailabilityNo change needed.
The UI session launch path passes
Some(SessionRef::from(session.id)), and respawn restores the session reference before creating the nextAgentLoop.n00n-lua/src/api/agent.rs (3)
20-21: LGTM!Also applies to: 39-39, 97-97, 723-723, 855-864, 890-891
1411-1418: 🩺 Stability & AvailabilityNo change needed.
LuaSession::drop()callsSessionState::close()for userdata GC paths, and the explicit:close()path also runsclose()before dropping the userdata.
645-648: 🩺 Stability & AvailabilityNo change needed.
sessionis only exposed from the Lua toolhandlerpath, whereRequest::CallToolhas already attached plugin state using the same agent identity.n00n-storage/src/sessions.rs (3)
857-878: 📐 Maintainability & Code QualityConfirm the quarantine log never records snapshot content.
The fallback logs only
error.classify(), so no payload data reaches the log. That matches the guideline that forbids logging raw payloads. The explicit named fallback also satisfies the rule against silently discarding failures. One note:raw.clone()duplicates the whole JSON value on every malformed load; the clone is only needed on the error path, soserde_json::from_valueon a borrowed copy is unavoidable here withoutfrom_value_ref. The current form is acceptable.As per coding guidelines: "Do not log raw provider payloads, prompts, credentials, or user session data."
Source: Coding guidelines
54-54: LGTM!Also applies to: 280-312, 368-384, 398-399, 446-447, 477-478, 530-550, 570-603, 609-627, 636-665, 679-683, 708-711, 735-737, 775-775, 806-836
4918-4934: LGTM!Also applies to: 4936-5182, 5202-5211
n00n-lua/src/state.rs (4)
14-105: LGTM!
129-279: LGTM!
386-407: LGTM!
409-888: LGTM!n00n-lua/src/api/util/ctx.rs (3)
93-101: LGTM!Also applies to: 129-130, 164-165
230-284: LGTM!
298-339: LGTM!Also applies to: 364-374, 456-459, 479-492, 505-507, 537-539
n00n-lua/src/api/util/mod.rs (1)
6-6: LGTM!n00n-lua/src/api/util/state_convert.rs (4)
9-56: LGTM!
89-106: LGTM!Also applies to: 118-155
157-267: LGTM!
269-495: LGTM!n00n-lua/src/error.rs (1)
28-29: LGTM!n00n-lua/src/lib.rs (1)
13-13: LGTM!n00n-lua/src/loader.rs (2)
9-20: LGTM!
611-680: 🩺 Stability & AvailabilityNo caller blocks on these state operations.
Only the plugin host tests call
hydrate_state,capture_state,reset_state, ordrop_state_owner, so there is no in-tree path where these blocking calls block a UI or async runtime thread.n00n-lua/src/runtime.rs (8)
41-49: LGTM!Also applies to: 183-200, 307-313, 1220-1220, 1313-1313
690-723: LGTM!
1008-1112: LGTM!
1178-1196: LGTM!
1816-1832: LGTM!Also applies to: 1993-1996, 2277-2277, 2307-2309
2505-2575: LGTM!
2631-2656: LGTM!
2667-2742: LGTM!
|
@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: 0ab3569cd4
ℹ️ 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".
- Update drain_runtime to stop polling closed channels and wait only for lifecycle changes once either spawn_rx or request_rx has closed - Remove rt.state.drop_plugin() call from Request::ClearPlugin handling to preserve plugin state across unload - Extract shared base-snapshot reconstruction into candidate_for helper to eliminate duplication in validate_replacement, validate_removal, and capture - Extract repeated 'state context is no longer active' text into STALE_CTX_ERR constant - Extend unload verification to assert plugin_a has no payload in both StoredStateScope::Session and Root - Increase delayed async job duration from 0.05s to 0.5s for reliable drain testing - Remove redundant insert_plugin_state API, keeping set_plugin_state as canonical implementation - Update set_state_revision to skip full validation when encoded size does not grow
Plugin state should persist across unload since we removed drop_plugin call. Update assertion to verify state is retained rather than removed.
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7314f44bdb
ℹ️ 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".
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
n00n-storage/src/sessions.rs (1)
536-554: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPropagate snapshot mutation failures during capture.
set_plugin_statereturns an error for malformed plugin containers and exceeded bounds.n00n-lua/src/state.rs:243-273discards that error withlet _ = candidate.set_plugin_state(...).capturecan then persist and cache a candidate that silently omits the current scoped state.Make
candidate_forreturnResultand propagate the mutation error throughcapture. Do not updateinner.baseswhen candidate construction fails. Add a regression test for capture with a malformed preserved container or an aggregate-size failure.As per coding guidelines: do not silently discard failures; return typed errors.
🤖 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-storage/src/sessions.rs` around lines 536 - 554, The candidate construction path in candidate_for currently discards set_plugin_state failures, allowing incomplete state to be persisted. Change candidate_for to return a typed Result, propagate set_plugin_state errors through capture, and ensure inner.bases is updated only after successful candidate construction; add a regression test covering a malformed preserved container or aggregate-size failure.Source: Coding guidelines
n00n-lua/src/api/util/ctx.rs (1)
401-406: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
parse_state_scopeinstate_replace.
state_get(Line 381) andstate_remove(Line 435) call the new helper.state_replacestill callsPluginStateScope::parseand repeats the error text as a literal. The two messages can drift apart. Route this path through the helper as well.♻️ Proposed fix
- let Some(scope) = PluginStateScope::parse(&scope) else { - return Ok(( - LuaValue::Nil, - Some("state scope must be 'session' or 'root'".to_owned()), - )); + let scope = match parse_state_scope(&scope) { + Ok(scope) => scope, + Err(error) => return Ok((LuaValue::Nil, Some(error.to_owned()))), };🤖 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/api/util/ctx.rs` around lines 401 - 406, Update state_replace to use the existing parse_state_scope helper instead of calling PluginStateScope::parse directly and duplicating the error message. Preserve the helper’s returned success and error behavior, matching the state_get and state_remove paths.
🤖 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 `@Cargo.toml`:
- Line 112: Remove arbitrary_precision from the workspace-level serde_json
dependency so it remains unconfigured for member crates and preserves normal Lua
conversion behavior. If large-number preservation is required, implement it only
in the persisted state payload path using a focused copy, string representation,
or serde_json::value::RawValue fallback rather than enabling the feature
globally.
In `@n00n-lua/src/runtime.rs`:
- Around line 2628-2641: Refactor the repeated drain_runtime invocation in the
surrounding loop by introducing a context struct containing the six
loop-invariant runtime, executor, gate, lifecycle, and receiver references.
Construct this context once before the loop, update drain_runtime to accept the
context plus deferred, and replace all seven eight-argument calls and their
existing break guards with the new two-argument form while preserving behavior.
In `@n00n-lua/src/state.rs`:
- Around line 243-273: Update candidate_for to return
Result<StoredSessionStateSnapshot, PluginStateError> and propagate failures from
set_plugin_state and remove_plugin_state with ?. Adjust capture,
validate_replacement, and validate_removal to call candidate_for with ? so
snapshot errors reject the operation instead of being discarded.
---
Outside diff comments:
In `@n00n-lua/src/api/util/ctx.rs`:
- Around line 401-406: Update state_replace to use the existing
parse_state_scope helper instead of calling PluginStateScope::parse directly and
duplicating the error message. Preserve the helper’s returned success and error
behavior, matching the state_get and state_remove paths.
In `@n00n-storage/src/sessions.rs`:
- Around line 536-554: The candidate construction path in candidate_for
currently discards set_plugin_state failures, allowing incomplete state to be
persisted. Change candidate_for to return a typed Result, propagate
set_plugin_state errors through capture, and ensure inner.bases is updated only
after successful candidate construction; add a regression test covering a
malformed preserved container or aggregate-size failure.
🪄 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: 61d377a4-5db8-47aa-9cfa-14d5b158641c
📒 Files selected for processing (7)
Cargo.tomln00n-agent/src/tools/mod.rsn00n-lua/src/api/util/ctx.rsn00n-lua/src/runtime.rsn00n-lua/src/state.rsn00n-lua/tests/plugin_host.rsn00n-storage/src/sessions.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
**/Cargo.toml: Use the workspace lint configuration from the rootCargo.tomlandclippy.toml; every crate must opt in with[lints] workspace = true.
Add new dependencies to the workspace root first, then reference them withworkspace = truein member crates; avoid floating versions and disable unused default features.
Files:
Cargo.toml
**/*.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/tools/mod.rsn00n-lua/src/state.rsn00n-lua/tests/plugin_host.rsn00n-lua/src/api/util/ctx.rsn00n-lua/src/runtime.rsn00n-storage/src/sessions.rs
🔇 Additional comments (13)
n00n-agent/src/tools/mod.rs (2)
746-748: Use the importedSessionReftype.Line 747 bypasses the existing import with
n00n_storage::id::SessionRef::generate(). UseSessionRef::generate().As per coding guidelines: “Import types at the top of the file and use short imported names.”
Source: Coding guidelines
305-332: LGTM!n00n-storage/src/sessions.rs (3)
519-529: LGTM!
562-718: LGTM!
4758-4764: LGTM!Also applies to: 4861-4867, 4879-4893, 4913-4919, 5210-5245, 5257-5262, 5281-5287, 5300-5335
n00n-lua/tests/plugin_host.rs (4)
238-238: LGTM!
4110-4110: LGTM!
4466-4485: LGTM!
4551-4553: LGTM!Also applies to: 4570-4570
n00n-lua/src/state.rs (1)
275-289: LGTM!Also applies to: 303-311
n00n-lua/src/api/util/ctx.rs (1)
28-32: LGTM!Also applies to: 415-430, 434-455
n00n-lua/src/runtime.rs (2)
2748-2839: LGTM!
1164-1177: 🩺 Stability & AvailabilityNo shutdown delay for
Request::Shutdownon the ordinary channel.
Dropsetsinner.shutdown.store(true, Ordering::Release)beforeinner.tx.send(Request::Shutdown), so any ordinary-channelShutdownis covered by the drain’s atomic flag on each loop iteration.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9cd9bd448
ℹ️ 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".
Previously, candidate_for silently discarded errors from set_plugin_state and remove_plugin_state, which could cause capture to return a snapshot with managed values silently omitted. Now these errors are propagated through the Result chain, ensuring data integrity issues are surfaced to callers.
|
@coderabbitai review @codex review The production persistence finding is implemented in stacked draft #332. |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba67440ad8
ℹ️ 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".
Primary agents with ToolFilter::Only can now discover and run MCP tools loaded via search, even when allow_dynamic_mcp_tools is false. The effective_tool_filter function now unconditionally adds tool_search to the filter when MCP is present, ensuring agents can discover MCP tools through the tool_search stub while still respecting the original filter constraints. This fixes the issue where filter_provider_tools dropped the MCP tool_search stub immediately after mcp.extend_tools, preventing discovery of deferred MCP tools for agents with non-empty allowed_tools configurations.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/agent/run.rs`:
- Around line 856-870: Update effective_tool_filter so the forced tool_search
inclusion is skipped when self.config.disabled_tools disables
crate::mcp::TOOL_SEARCH_TOOL_NAME, while preserving forced discovery for
ToolFilter::Only otherwise. Add coverage for an MCP-enabled agent with
disabled_tools containing "tool_search", verifying the effective filter excludes
it.
🪄 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: e4b58285-09f7-4b1a-b5e8-3eec9ff8c0af
📒 Files selected for processing (3)
n00n-agent/src/agent/run.rsn00n-lua/src/api/util/ctx.rsn00n-lua/src/state.rs
📜 Review details
🧰 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-lua/src/state.rsn00n-agent/src/agent/run.rsn00n-lua/src/api/util/ctx.rs
🔇 Additional comments (5)
n00n-agent/src/agent/run.rs (2)
171-171: LGTM!Also applies to: 218-218, 277-277, 506-506, 894-894, 1153-1153
1449-1460: LGTM!Also applies to: 1483-1500, 2153-2153, 3366-3366
n00n-lua/src/state.rs (2)
223-225: LGTM!Also applies to: 247-272, 281-281, 308-309
355-357: LGTM!Also applies to: 711-738
n00n-lua/src/api/util/ctx.rs (1)
235-244: LGTM!Also applies to: 303-379, 401-428, 432-452, 478-551
The disabled tool_search policy finding is fixed and tested in 6afdebd; the thread is resolved.
…tate-runtime # Conflicts: # n00n-agent/src/agent/run.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
Summary
Validation
cargo fmt --all -- --checkcargo check --allcargo clippy --all --tests -- -D warningscargo nextest run -p n00n-lua -p n00n-storage(1200 passed)cargo nextest run --workspace(4660 passed, 1 skipped)Deferred
Production root-session hydrate/capture/drop persistence and
todo_writemigration remain a separate follow-up.