Repository navigation
Conversation
- Invalid UTF-8 on stdin no longer kills the ACP server; it returns a JSON-RPC parse error and keeps running. - Malformed/overflowing request ids no longer coerce to RequestId::Null; they are rejected with an invalid-request error (-32600). - Extracted read_request() to make line/id validation testable. Fixes: n00n-acp/src/server.rs
Running `n00n` with no prompt in a pipe/non-TTY previously panicked with a ratatui init error. Now it exits with code 1 and a clear message pointing users at --print. Fixes: src/cmd/tui.rs
|
Warning Review limit reached
Next review available in: 45 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 (17)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds secret/PII validation to editing tools, exfiltration checks to Bash, and Python sandbox tests. It also improves ACP parsing, MCP shutdown task ownership, and non-terminal TUI startup handling. ChangesContent and execution safety controls
Runtime reliability and lifecycle handling
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 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: 8cc1eea | Previous: 8d5809e | Ratio |
|---|---|---|---|
fib/jit_mlua_hook |
6626107 ns/iter (± 61148) |
6678342 ns/iter (± 253554) |
0.99 |
fib/jit_watchdog |
2221705 ns/iter (± 7096) |
2220199 ns/iter (± 12990) |
1.00 |
fib/jit_none |
2225958 ns/iter (± 43499) |
2218881 ns/iter (± 33232) |
1.00 |
fib/interp_mlua_hook |
8187601 ns/iter (± 36001) |
8091425 ns/iter (± 114645) |
1.01 |
fib/interp_watchdog |
4367937 ns/iter (± 21549) |
4326050 ns/iter (± 14651) |
1.01 |
fib/interp_none |
4330719 ns/iter (± 25587) |
4302519 ns/iter (± 21751) |
1.01 |
buffer_rw/jit_mlua_hook |
585172 ns/iter (± 12121) |
585064 ns/iter (± 2008) |
1.00 |
buffer_rw/jit_watchdog |
192189 ns/iter (± 371) |
191572 ns/iter (± 450) |
1.00 |
buffer_rw/jit_none |
192050 ns/iter (± 317) |
191410 ns/iter (± 393) |
1.00 |
buffer_rw/interp_mlua_hook |
1047156 ns/iter (± 11097) |
1047263 ns/iter (± 5989) |
1.00 |
buffer_rw/interp_watchdog |
583686 ns/iter (± 15516) |
586663 ns/iter (± 8150) |
0.99 |
buffer_rw/interp_none |
583318 ns/iter (± 3085) |
582762 ns/iter (± 1557) |
1.00 |
splash_render_120x40 |
50331 ns/iter (± 8524) |
48447 ns/iter (± 2593) |
1.04 |
splash_render_200x60 |
196697 ns/iter (± 1769) |
159346 ns/iter (± 17058) |
1.23 |
This comment was automatically generated by workflow using github-action-benchmark.
…O errors - `read_request` now only emits `parse_error` for `InvalidData` (invalid UTF-8). Other `read_line` I/O errors are propagated as `color-eyre` reports, causing `serve` to exit instead of mis-reporting a parse error and looping. - Unit tests downcast the returned `Report` to `AcpError` before checking the error code. - `tests/non_tty.rs` now uses a per-process temp directory to avoid races.
|
@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: e229b71e2a
ℹ️ 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".
…e/edit secret validation - Cancel the MCP manager command-loop task if shutdown times out. - Require `justification` for bash commands that may exfiltrate data (curl, wget, nc, ncat, dig, nslookup, encoded data piped to network tools). - Add `plugins/lib/n00n/secret_check.lua` with heuristic secret/PII detection. - Require `justification` for write/edit/multiedit/edit_lines/insert_lines when new content may contain secrets/PII. - Regenerate token-profile baseline for the expanded tool schemas.
w0wl0lxd
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3b0d16efe
ℹ️ 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: 11
🤖 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-acp/src/server.rs`:
- Around line 67-69: Update the writer task and serve flow so serialization,
write, and flush failures are returned as a typed error instead of discarded.
Have serve observe the writer task result, stop processing requests when the
writer reports failure, and propagate that error rather than returning success
on EOF; preserve the existing JSON-RPC responses for individual line, JSON, and
request-id errors.
- Around line 610-677: Import the repository’s test-case attribute in server.rs
and replace #[test] with #[test_case] on the six unit tests shown:
request_id_accepts_valid_ids, request_id_rejects_invalid_types_and_overflow,
read_request_returns_none_on_eof,
read_request_returns_parse_error_on_invalid_utf8,
read_request_returns_invalid_request_on_overflow_id, and
read_request_parses_null_id. Keep their existing snake_case names and test
bodies unchanged.
In `@n00n-agent/src/mcp/mod.rs`:
- Around line 626-633: Update the task mutex access in the shutdown flow around
task.cancel to explicitly match the lock result instead of silently calling
PoisonError::into_inner. On poisoned recovery, use an explicitly named fallback
guard and emit a sanitized structured warn! event; otherwise preserve the
existing task extraction and cancellation behavior.
- Around line 626-633: Update the shutdown path around McpHandle’s stored task
and shutdown_all so transport teardown does not depend on the cancelled command
loop completing. Ensure the cancellation owner retains access to the inner entry
and ToolIndex, clears or publishes an empty index before terminating child
process groups, and then kills/reaps the associated processes even when
task.cancel().await is used.
- Around line 301-303: Import smol::Task at module scope in the module
containing the task field, then update the task field’s type from smol::Task<()>
to Task<()> while preserving its existing Arc, Mutex, and Option structure.
In `@plugins/bash/init.lua`:
- Around line 245-273: Replace the raw "nc" and "od " substring checks in the
encoded-data and command-substitution conditions with command-boundary-aware
patterns, reusing the anchoring approach already used by the earlier netcat
detection in this function. Preserve detection for actual nc/ncat and od
commands while preventing matches inside ordinary words such as “sync” or
“good”.
- Around line 211-277: The command-boundary detection in
exfiltration_command_reason only recognizes network tools at script start, after
pipes, and after &&. Update every network-command prefix check in
exfiltration_command_reason, including encoded-data and command-substitution
paths, to recognize commands after ;, ||, and newlines as well, or reuse the
existing collect_guard_commands segmentation logic used by broad_command_reason
while preserving current reasons.
In `@plugins/edit/init.lua`:
- Around line 273-277: Extract the duplicated secret/justification validation
into a shared require_justification helper in secret_check.lua, accepting text,
justification, and tool name and returning the existing error table or nil.
Replace the inline checks at plugins/edit/init.lua lines 273-277, 348-357,
425-429, and 480-484, plus plugins/write/init.lua lines 75-79, passing each
site’s tool name and preserving the existing per-edit loop behavior.
In `@plugins/lib/n00n/secret_check.lua`:
- Around line 79-136: Update SECRET_ASSIGNMENT_PATTERN and the M.check detection
flow so credential-style assignments are actually evaluated. Remove the
unsupported "|" alternation and check each secret-key suffix with valid Lua
pattern matching, including assignments such as userCredential=..., while
preserving the existing token and authorization-header checks.
In `@tests/non_tty.rs`:
- Around line 6-7: Update the state-directory cleanup before create_dir_all to
handle remove_dir_all’s Result explicitly: treat only the NotFound error as
expected, and fail the test for every other cleanup error before recreating the
directory. Preserve the existing create_dir_all behavior.
- Around line 30-33: Update the non-TTY assertion in the test to require a
non-success exit code and the exact expected terminal error text in stderr,
removing the permissive `code == 1` alternative. Preserve the existing
diagnostic output while ensuring the test verifies the terminal guard was
reached.
🪄 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: 499bfece-ec19-43fb-908c-7558f7b9aadb
📒 Files selected for processing (13)
changelog.d/230.fixed.mdn00n-acp/src/server.rsn00n-agent/src/mcp/mod.rsn00n-interpreter/src/runner.rsn00n-lua/tests/plugin_host.rsn00n-token-profile/baselines/cold_start.jsonplugins/bash/init.luaplugins/code_execution/init.luaplugins/edit/init.luaplugins/lib/n00n/secret_check.luaplugins/write/init.luasrc/cmd/tui.rstests/non_tty.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-interpreter/src/runner.rssrc/cmd/tui.rstests/non_tty.rsn00n-agent/src/mcp/mod.rsn00n-lua/tests/plugin_host.rsn00n-acp/src/server.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/230.fixed.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/230.fixed.md
🪛 Luacheck (1.2.0)
plugins/lib/n00n/secret_check.lua
[warning] 79-79: unused variable 'SECRET_ASSIGNMENT_PATTERN'
(W211)
🪛 markdownlint-cli2 (0.23.1)
changelog.d/230.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 (10)
plugins/code_execution/init.lua (1)
15-15: LGTM!n00n-interpreter/src/runner.rs (1)
508-531: LGTM!n00n-acp/src/server.rs (2)
94-105: LGTM!
124-164: LGTM!n00n-agent/src/mcp/mod.rs (1)
1164-1164: LGTM!Also applies to: 1351-1351, 1521-1521
changelog.d/230.fixed.md (1)
1-1: MD041 heading warning does not apply to this fragment.Based on learnings from this repository, files in
changelog.d/whose names begin with a numeric fragment identifier are headingless by design, since generated release sections provide the heading. This applies here, so the markdownlint MD041 hint on Line 1 should not be actioned.Source: Learnings
plugins/bash/init.lua (1)
22-22: LGTM!Also applies to: 688-688, 716-716, 740-740, 792-796
n00n-lua/tests/plugin_host.rs (1)
3172-3210: LGTM!Also applies to: 5938-5987
n00n-token-profile/baselines/cold_start.json (1)
7-20: LGTM!src/cmd/tui.rs (1)
263-268: LGTM!
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
n00n-acp/src/server.rs (1)
133-142: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn
Invalid Requestfor non-request payloads.A missing
iddoes not by itself make a payload a notification.{},null, and{"method": 1}reach the end of this branch without a response. ReturnAcpError::invalid_request()withRequestId::Nullwhen the value is neither a response nor a request with a stringmethod. JSON-RPC requires an invalid-request response for JSON that is not a valid Request object. (jsonrpc.org)Proposed fix
} else if let Some(id) = id { server.respond(id, Err(AcpError::invalid_request())); + } else { + server.respond(RequestId::Null, Err(AcpError::invalid_request())); }🤖 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-acp/src/server.rs` around lines 133 - 142, Update the incoming payload dispatch around handle_incoming_response, handle_request, and handle_notification so any value that is neither a response nor an object with a string method produces an invalid-request response using RequestId::Null. Preserve notification handling only for valid method payloads without an id, and retain id-specific responses for valid request-shaped payloads.
🤖 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-acp/src/server.rs`:
- Around line 124-130: Update the invalid request-ID branch in the surrounding
JSON stream processing loop to reject the current value and continue processing
subsequent values instead of terminating the loop with break. Add a test
covering an invalid-ID request followed by a valid request on the same input
line, verifying both the error response and later valid request are handled.
In `@site/docs/content/lua-api/_index.md`:
- Around line 5924-5928: Align the published documentation with the actual
coverage of the secret_check.lua detector by removing or qualifying general
PII-detection claims and describing only secret-keyword/token patterns and
Basic/Bearer authorization headers. Update site/docs/content/lua-api/_index.md
lines 5924-5928 and the write, edit, multiedit, edit_lines, and insert_lines
justification descriptions in site/docs/content/tools/_index.md lines 45, 56,
66, 78, and 87 respectively; do not add detector rules unless explicitly
implementing and testing them.
- Around line 5930-5935: Update the n00n.secret_check example so
require("n00n.secret_check") is assigned to the local module variable used by
M.check and M.reason, making the block executable; alternatively label the block
explicitly as a pseudocode/signature excerpt if it is not intended to run.
In `@typos.toml`:
- Line 26: Update the secret ignore pattern in typos.toml to use the
case-insensitive pattern `(?i)secret`, replacing the current `[Ss]ecret` entry
so all capitalization variants are ignored.
---
Outside diff comments:
In `@n00n-acp/src/server.rs`:
- Around line 133-142: Update the incoming payload dispatch around
handle_incoming_response, handle_request, and handle_notification so any value
that is neither a response nor an object with a string method produces an
invalid-request response using RequestId::Null. Preserve notification handling
only for valid method payloads without an id, and retain id-specific responses
for valid request-shaped payloads.
🪄 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: 851319eb-41d2-4a5d-a3d4-eda075c156e0
📒 Files selected for processing (5)
n00n-acp/src/server.rsplugins/lib/n00n/secret_check.luasite/docs/content/lua-api/_index.mdsite/docs/content/tools/_index.mdtypos.toml
💤 Files with no reviewable changes (1)
- plugins/lib/n00n/secret_check.lua
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: Coverage
- GitHub Check: MSRV (1.97)
- GitHub Check: Test (Windows)
- GitHub Check: Build (Windows)
- GitHub Check: Build
- GitHub Check: Rustdoc
- GitHub Check: Lint (Windows)
- GitHub Check: Format (Rust)
- GitHub Check: Docs
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (rust)
- GitHub Check: Criterion
🧰 Additional context used
📓 Path-based instructions (2)
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/tools/_index.mdsite/docs/content/lua-api/_index.md
**/*.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-acp/src/server.rs
🔇 Additional comments (2)
n00n-acp/src/server.rs (1)
94-105: LGTM!site/docs/content/tools/_index.md (1)
24-24: LGTM!Also applies to: 44-44, 47-47, 59-59, 69-69, 81-81
|
Addressed the remaining review findings in commit
Validation: |
|
| Filename | Overview |
|---|---|
| n00n-acp/src/server.rs | Fixes invalid UTF-8 panic by catching InvalidData and responding with parse_error; changes request_id to return Result so malformed/overflowing IDs are rejected with invalid_request instead of silently coercing to Null; adds writer failure detection to surface stdout errors. |
| n00n-agent/src/mcp/mod.rs | Stores the command-loop task in McpHandle so it can be explicitly cancelled when graceful shutdown times out, preventing the previously leaked background task. |
| n00n-agent/src/mcp/stdio.rs | Separates pid capture from ChildGuard (stored separately so child_pids() is lock-free), wraps ChildGuard in async Mutex, and adds force_shutdown that calls kill_and_reap after setting the alive flag. |
| n00n-agent/src/mcp/transport.rs | Adds force_shutdown as a default no-op to McpTransport trait, so non-stdio transports don't need to implement it. |
| plugins/lib/n00n/secret_check.lua | New heuristic secret/PII detection module; the 8 keywords ending with = or : in SECRET_KEYWORDS are unreachable via contains_keyword(key) since the gmatch key-capture pattern cannot produce strings containing = or :. |
| plugins/bash/init.lua | Adds exfiltration_command_reason to gate curl, wget, nc, ncat, dig, nslookup, and encoded-data patterns behind a justification; the network variable and its two dependent branches are dead code (previously flagged). |
| plugins/write/init.lua | Integrates secret_check.require_justification before the actual file write; correctly placed before path resolution so the error is returned without side effects. |
| plugins/edit/init.lua | Adds secret_check gates to edit, multiedit, edit_lines, and insert_lines; multiedit uses the shared top-level justification field and prefixes error messages with 0-based edit index. |
| src/cmd/tui.rs | Adds pre-flight TTY check (stdin and stdout) that returns a clear error instead of panicking inside ratatui when running in a pipe/non-TTY environment. |
| plugins/code_execution/init.lua | Removes sys and os from the Python preamble; removing sys may silently break generated scripts that use sys.exit() or sys.stderr (previously flagged). |
Sequence Diagram
sequenceDiagram
participant LLM as LLM / Tool caller
participant Tool as write / edit / multiedit
participant SC as secret_check.lua
participant FS as Filesystem
LLM->>Tool: call(content, justification?)
Tool->>SC: require_justification(content, justification, tool_name)
SC->>SC: secret_assignment_keyword(content)
alt "keyword match AND value >= 16 chars"
SC-->>Tool: "{ llm_output: error, is_error: true }"
Tool-->>LLM: error — provide justification
else authorization header literal
SC-->>Tool: "{ llm_output: error, is_error: true }"
Tool-->>LLM: error — provide justification
else no match OR justification provided
SC-->>Tool: nil (ok)
Tool->>FS: write / apply edit
Tool-->>LLM: success
end
Reviews (2): Last reviewed commit: "docs: regenerate lua API reference" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@cursor review verbose=true |
|
Bugbot request id: serverGenReqId_f4d8795e-1d90-46b3-aa6b-1bdddb5af855 |
Bugbot rules debugNo rules were used for this review. https://cursor.com/docs/bugbot#team-rules Bugbot request id: serverGenReqId_f4d8795e-1d90-46b3-aa6b-1bdddb5af855 |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f4d8795e-1d90-46b3-aa6b-1bdddb5af855) |
There was a problem hiding this comment.
w0wl0lxd has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
- Fix writer task error propagation in serve flow - Add test_case attribute to unit tests in n00n-acp/src/server.rs - Add invalid_id_continues_to_next_value test - Fix poisoned mutex handling in MCP shutdown with structured warn - Update secret_check.lua docs with actual coverage - Add child_pids method to McpTransport trait
…te cold-start baseline
Resolved conflict in n00n-acp/src/server.rs: - Added .await to handle_request call (from main) - Preserved error handling logic for invalid requests (from PR branch) - Fixed missing closing brace
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cc1eea67b
ℹ️ 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".
|
|
||
| local function secret_assignment_keyword(s) | ||
| local l = lower(s) | ||
| for key, value in l:gmatch("([%w_%-%.]+)%s*[:=]%s*[\"']?([A-Za-z0-9+/_%-]+)") do |
There was a problem hiding this comment.
For JSON-style credentials such as {"api_key": "abcdefghijklmnop"}, this pattern cannot match because it requires : or = immediately after the captured key and does not allow the closing quote. The write handler therefore persists a common credential-file representation without requiring justification; support quoted keys or parse the relevant serialization formats before mutation.
AGENTS.md reference: AGENTS.md:L78-L78
Useful? React with 👍 / 👎.
| end), | ||
|
|
||
| handler = function(input, ctx) | ||
| local secret_error = secret_check.require_justification(input.new_string, input.justification, "edit") |
There was a problem hiding this comment.
When an existing file contains API_KEY=PLACEHOLDER and an edit replaces only PLACEHOLDER with a long credential, input.new_string has no secret-bearing key, so this check passes and the resulting assignment is persisted without justification. The same fragment-only check is used by the other edit variants; validate the reconstructed file content inside the edit callback before writing it.
AGENTS.md reference: AGENTS.md:L78-L78
Useful? React with 👍 / 👎.
| #[test] | ||
| fn read_request_returns_parse_error_on_invalid_utf8() { | ||
| // Invalid UTF-8 is handled in the serve loop with InvalidData error kind | ||
| // It responds with parse_error and continues | ||
| } |
There was a problem hiding this comment.
This test contains no invocation or assertion, so it passes even if the serve loop again terminates on invalid UTF-8 instead of returning a parse error and continuing; the adjacent EOF test has the same problem. Drive the server with invalid bytes and assert the emitted JSON-RPC response and continued request processing.
AGENTS.md reference: AGENTS.md:L54-L56
Useful? React with 👍 / 👎.
|
Closing this conflicting multi-area bug-hunt bundle. Several ideas remain useful, but unresolved ACP and secret-validation defects require separate focused PRs from current main. |
Pull request was closed
1 participant
Summary
This PR collects the fixes from bug-hunt Phase 2.
Phase 1 confirmed fixes
ACP server died on invalid UTF-8 (
n00n-acp/src/server.rs)n00n acpno longer exits with a panic when binary/invalid UTF-8 is written to stdin.-32700) and keeps running.ACP request id coercion to
RequestId::Null(n00n-acp/src/server.rs)Null.invalid_requesterror (-32600) andid: null, per JSON-RPC.TUI panic in non-TTY environments (
src/cmd/tui.rs)n00nwithout a prompt in a pipe/non-TTY now exits with code 1 and a clear message instead of a ratatui panic.tests/non_tty.rsto guard this path.Phase 2 confirmed fixes
MCP manager shutdown task leak (
n00n-agent/src/mcp/mod.rs)McpHandlenow stores the command-loop task and cancels it if graceful shutdown times out, preventing a leaked background task.bashtool exfiltration checks (plugins/bash/init.lua)curl,wget,nc,ncat,dig,nslookup, and encoded data piped to network tools) now require ajustification.Write/edit secret and PII validation (
plugins/write/init.lua,plugins/edit/init.lua,plugins/lib/n00n/secret_check.lua)plugins/lib/n00n/secret_check.luaheuristically detects secret/PII patterns in tool content.write,edit,multiedit,edit_lines, andinsert_linesnow return an error when the new content may contain a secret/PII pattern and nojustificationis provided.Test plan
cargo fmt --all✅cargo check --all✅cargo clippy --all --tests -- -D warnings✅cargo nextest run --workspace✅ (4585 passed, 1 flake incmd::tui_bridge::tests::spawn_serves_tui_list_over_uds; passes individually)cargo nextest run -p n00n-lua✅ (978 passed)n00n-token-profile/baselines/cold_start.jsonto account for expanded tool schemas.Note
This work was done in an isolated worktree:
fix/bughunt-phase2-20260802offorigin/main.Note
Medium Risk
Changes touch agent I/O, process lifecycle, and permission guardrails; false positives on bash or secret heuristics could block legitimate workflows until justified.
Overview
Hardens ACP and TUI I/O, fixes MCP shutdown leaks, and adds guardrails on
bashand file-editing tools.ACP races stdin reads against stdout write failures, surfaces write errors instead of ignoring them, treats invalid UTF-8 on stdin as JSON-RPC parse errors (keeps serving), and rejects malformed
idvalues withinvalid_requestinstead of coercing to null. TUI refuses to start when stdin/stdout are not a TTY, pointing users to--print.MCP keeps the command-loop task on
McpHandle, runsforce_shutdown(clear index/snapshot, kill process groups, reap stdio children) when graceful shutdown times out, and tightens stdio teardown via explicitforce_shutdownon transports.bashadds exfiltration heuristics (network/DNS tools, encoded data piped to network commands, etc.) that requirejustification, with tests so benign commands likesyncare not flagged by substring false positives.write,edit,multiedit,edit_lines, andinsert_linesuse newn00n.secret_checkto block likely secrets/credentials withoutjustification. Docs, token-profile baselines, andcode_executionpreamble trim (os/sysremoved from injected imports) reflect the expanded schemas and sandbox posture; interpreter tests assertopen()is sandbox-blocked.Reviewed by Cursor Bugbot for commit c5ebcc6. Bugbot is set up for automated code reviews on this repo. Configure here.