Repository navigation
fix: harden reviewed trust boundaries - #116
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR hardens plugin permission defaults, OAuth endpoint validation, compaction overflow handling, and session snapshot persistence retries, with expanded tests and changelog documentation. ChangesReview hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant StorageWriter
participant flush
participant retry_states
participant pending_snapshots
StorageWriter->>flush: flush pending snapshots
flush-->>StorageWriter: failed session revisions
StorageWriter->>retry_states: update retry attempts
retry_states->>pending_snapshots: retain or remove revision
StorageWriter->>flush: retry retained snapshot
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
@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: c37ac0ae1a
ℹ️ 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: 2
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/storage_writer.rs (1)
231-276: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
replacecheck inflush()'s failure paths.In both the
ensure_subdirerror branch (lines 246-257) and the main write-failure branch (lines 266-272),pending_guard.get(&id)is checked before reinserting a failed snapshot. Sincemem::takeemptiespending_guardat the top of the function while the same lock guard is held for the entire call, andbatch's keys are unique,pending_guard.get(&id)can never beSomefor an id being reinserted here — the check is always vacuously true. It's harmless today but reads as if concurrent mutation ofpending_guardis possible mid-loop, which it isn't under the current locking.♻️ Suggested simplification
- for (id, snapshot) in batch { - let replace = pending_guard - .get(&id) - .is_none_or(|current| current.revision < snapshot.revision); - if replace { - pending_guard.insert(id, snapshot); - } - } + for (id, snapshot) in batch { + pending_guard.insert(id, snapshot); + }Same simplification applies to the main loop's reinsert at lines 266-272.
🤖 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/storage_writer.rs` around lines 231 - 276, Remove the redundant replace checks in both failure paths of flush. After mem::take empties pending_guard and while the lock is held, directly reinsert each failed snapshot into pending_guard in the ensure_subdir error branch and the write_session failure branch, preserving the existing failed revision reporting.
🤖 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-lua/src/plugin_permissions.rs`:
- Around line 71-74: Import toml::Value at file scope in
n00n-lua/src/plugin_permissions.rs (lines 71-74) and use Value::as_bool; import
isahc::http::Uri at file scope in n00n-agent/src/mcp/oauth/discovery.rs (lines
100-102) and use Uri for parsing. Replace the inline fully qualified type paths
at both sites without changing behavior.
In `@n00n-ui/src/storage_writer.rs`:
- Around line 27-31: Update the RetryState declaration to remove the unnecessary
Copy and Clone derives, leaving it as a non-derived struct while preserving its
existing revision and attempts fields and &mut RetryState usage.
---
Outside diff comments:
In `@n00n-ui/src/storage_writer.rs`:
- Around line 231-276: Remove the redundant replace checks in both failure paths
of flush. After mem::take empties pending_guard and while the lock is held,
directly reinsert each failed snapshot into pending_guard in the ensure_subdir
error branch and the write_session failure branch, preserving the existing
failed revision reporting.
🪄 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: 8ebde30c-9c7a-41aa-adcf-8545589b41ca
📒 Files selected for processing (5)
changelog.d/review-hardening.fixed.mdn00n-agent/src/agent/compaction.rsn00n-agent/src/mcp/oauth/discovery.rsn00n-lua/src/plugin_permissions.rsn00n-ui/src/storage_writer.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Make non-trivial or multi-file changes in a dedicated git worktree on a new branch, preserve unrelated user changes, and do not force-push or push to main.
Ship finished work by committing with a clear Conventional Commit message, pushing the branch, and opening a draft PR; never add AI-agent attribution to authored content.
For repository exploration, prefer structural tools (codegraph,arbor,index), then targeted reads; use grep only as fallback, parallelize independent calls, and filter large outputs.
Research documented behavior before fixing unfamiliar failures or third-party, CLI, API, infrastructure, or deployment issues, and distinguish baseline failures from regressions.
Files:
changelog.d/review-hardening.fixed.mdn00n-agent/src/mcp/oauth/discovery.rsn00n-lua/src/plugin_permissions.rsn00n-agent/src/agent/compaction.rsn00n-ui/src/storage_writer.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Follow the workspace Rust lint configuration in the rootCargo.tomlandclippy.toml; every crate must opt in with[lints] workspace = true.
Do not add unsafe code, FFI, global mutable state,static mut, or unchecked transmute-like behavior without written review and an explicit crate-level lint exception. Existing unsafe calls require#[allow(unsafe_code)]and a SAFETY comment.
Do not useunwrap,expect,panic!,todo!,unimplemented!, ordbg!in production code; use explicit error handling instead. Tests are exempt from unwrap, expect, and panic restrictions.
Do not silently discard failures withunwrap_or,unwrap_or_default,.ok()onResult, or similar defaults. Convert missing data, parse failures, dependency errors, closed channels, and invalid state into typed errors, explicit rejection, or a named fallback with sanitized structured logging.
Use typed error propagation with?,ok_or_else, andmap_err; usethiserrorfor library domain errors andcolor-eyreat binary boundaries.
Do not use wildcard imports. Import types at the top of the file and use short names rather than fully qualified inline paths.
Keep constants at the top of the file immediately after imports, avoid inline magic numbers and strings, and use descriptive variable and function names.
Use#[derive(Copy)]only for structs with one primitive field.
Prefer structured logging with broad useful fields and provide helpful, sanitized error messages.
Keep Rust code minimal and idiomatic: avoid trivial comments, unnecessary state, and bloat; follow KISS, DRY, and SRP.
Do not commit or log credentials, API keys, tokens, cookies, authorization headers, raw provider payloads, prompts, or user session data.
Validate and authorize HTTP, file, queue, config/environment, LLM output, and provider callbacks before mutation or persistence; apply prompt-injection defenses when untrusted content enters prompts or tool calls.
Tool execution must use allowlisted to...
Files:
n00n-agent/src/mcp/oauth/discovery.rsn00n-lua/src/plugin_permissions.rsn00n-agent/src/agent/compaction.rsn00n-ui/src/storage_writer.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,toml}: Run and keep passingcargo fmt --all,cargo check --all,cargo clippy --all --tests -- -D warnings, and workspace tests; usecargo test --workspacewhencargo-nextestis unavailable.
Add new dependencies to the workspaceCargo.tomlfirst, then reference them withworkspace = truein the consuming crate.
Files:
n00n-agent/src/mcp/oauth/discovery.rsn00n-lua/src/plugin_permissions.rsn00n-agent/src/agent/compaction.rsn00n-ui/src/storage_writer.rs
**/*.{rs,rs.in}
📄 CodeRabbit inference engine (AGENTS.md)
Treat LLM and provider output as untrusted input; validate it against schemas, domain constraints, and source evidence before persistence or action.
Files:
n00n-agent/src/mcp/oauth/discovery.rsn00n-lua/src/plugin_permissions.rsn00n-agent/src/agent/compaction.rsn00n-ui/src/storage_writer.rs
🪛 markdownlint-cli2 (0.23.0)
changelog.d/review-hardening.fixed.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (11)
n00n-agent/src/agent/compaction.rs (4)
125-130: LGTM!
331-339: LGTM!
360-360: LGTM!
465-494: LGTM!n00n-lua/src/plugin_permissions.rs (1)
184-202: LGTM!n00n-agent/src/mcp/oauth/discovery.rs (2)
2-2: LGTM!
292-300: LGTM!changelog.d/review-hardening.fixed.md (1)
1-1: LGTM!n00n-ui/src/storage_writer.rs (3)
184-229: LGTM!
72-113: LGTM!Also applies to: 278-303
477-609: LGTM!
|
Addressed all final review feedback in e9078a6, including the outside-diff simplification: failed snapshots are reinserted directly, retry attempts live on each snapshot, and equal-revision replacements reset their retry budget. Full workspace formatting, check, clippy, and 3,853-test nextest suite pass. |
1 participant
Why
A whole-repository correctness and security review found several contained defects: failed session snapshots could be dropped, project plugin manifests defaulted omitted capabilities to allowed, compaction could retry an unchanged overflow request forever, and OAuth endpoint validation accepted attacker-controlled hostname prefixes.
What changed
Verification
cargo fmt --all -- --checkcargo check --allcargo clippy --all --tests -- -D warningscargo nextest run --workspace(3849 passed, 1 skipped)git diff --checkRisk and rollback
The main compatibility change is default-deny behavior for omitted project plugin capabilities. Plugins must explicitly request required capabilities in
plugin.toml. Roll back commite0012ebif this causes unexpected compatibility problems.Review follow-ups
The broader review also identified workspace trust, MCP environment isolation, authenticated updates, MCP cancellation/body limits, persistence health reporting, and provider conformance testing as follow-up work. Those require separate product or architecture decisions and are intentionally excluded from this contained patch.
Changelog
changelog.d/review-hardening.fixed.md