fix(cleanup): fix session-task-cursor-crashes dirty follow-up - #310
Conversation
|
Warning Review limit reached
Next review available in: 10 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 agent centralizes cancelled subagent output matching. Cursor frame handling now propagates malformed payload errors while continuing to ignore malformed end-stream payloads. Tests cover unknown protobuf wire types, truncated data, and corrupt gzip payloads. ChangesAgent and Cursor validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 4c4dca3 | Previous: f7be58e | Ratio |
|---|---|---|---|
fib/jit_mlua_hook |
6644745 ns/iter (± 178416) |
6454899 ns/iter (± 38351) |
1.03 |
fib/jit_watchdog |
2239405 ns/iter (± 5866) |
2439913 ns/iter (± 25178) |
0.92 |
fib/jit_none |
2233195 ns/iter (± 20139) |
2445792 ns/iter (± 71418) |
0.91 |
fib/interp_mlua_hook |
8402093 ns/iter (± 34586) |
7618751 ns/iter (± 68223) |
1.10 |
fib/interp_watchdog |
4327850 ns/iter (± 16778) |
3870347 ns/iter (± 29008) |
1.12 |
fib/interp_none |
4351340 ns/iter (± 47791) |
3750416 ns/iter (± 10555) |
1.16 |
buffer_rw/jit_mlua_hook |
582954 ns/iter (± 8359) |
552544 ns/iter (± 1578) |
1.06 |
buffer_rw/jit_watchdog |
191883 ns/iter (± 2428) |
168156 ns/iter (± 720) |
1.14 |
buffer_rw/jit_none |
191666 ns/iter (± 2167) |
168084 ns/iter (± 467) |
1.14 |
buffer_rw/interp_mlua_hook |
1049514 ns/iter (± 10778) |
1055228 ns/iter (± 7659) |
0.99 |
buffer_rw/interp_watchdog |
582338 ns/iter (± 1299) |
642817 ns/iter (± 5040) |
0.91 |
buffer_rw/interp_none |
581171 ns/iter (± 17443) |
638481 ns/iter (± 4989) |
0.91 |
splash_render_120x40 |
73448 ns/iter (± 6417) |
64783 ns/iter (± 4568) |
1.13 |
splash_render_200x60 |
176481 ns/iter (± 14635) |
143828 ns/iter (± 23524) |
1.23 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…rs and centralize cancelled subagent output constants
![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
🤖 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-providers/src/providers/cursor/run.rs`:
- Around line 865-883: Remove the duplicate test function
handle_data_frame_rejects_corrupt_compressed_payload and keep the existing
handle_data_frame_rejects_corrupt_compression coverage. Ensure the retained test
preserves both the gzip error-message assertion and the 502 status assertion if
those checks are present or required.
- Around line 845-863: Update
handle_data_frame_rejects_unknown_wire_type_three_payload to assert the returned
error matches AgentError::Api with status 502, while retaining the wire-type
message check. Add focused malformed-payload tests covering
has_exec_server_message, extract_text_deltas, and extract_thinking_deltas,
verifying each rejects invalid protobuf data through the expected error path.
🪄 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: 1bd0cdff-ad1a-4ab8-a158-41c7e2d1381d
📒 Files selected for processing (2)
n00n-agent/src/agent/tool_dispatch.rsn00n-providers/src/providers/cursor/run.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: MSRV (1.97)
- GitHub Check: Coverage
- GitHub Check: Rustdoc
- GitHub Check: Docs
- GitHub Check: Test
- GitHub Check: Build (Windows)
- GitHub Check: Lint (Windows)
- GitHub Check: Lint
- GitHub Check: Unused deps
- GitHub Check: Test (Windows)
- GitHub Check: Build
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (rust)
- GitHub Check: Criterion
🧰 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-agent/src/agent/tool_dispatch.rsn00n-providers/src/providers/cursor/run.rs
🔇 Additional comments (3)
n00n-agent/src/agent/tool_dispatch.rs (1)
21-26: LGTM!Also applies to: 295-295
n00n-providers/src/providers/cursor/run.rs (2)
726-739: LGTM!
749-754: LGTM!
|
@coderabbitai review |
✅ Action performedReview finished.
|
Resolve conflict in tool_dispatch.rs by keeping both CANCELLED_SUBAGENT_OUTPUTS (from PR) and TOOL_ERROR_LOG_MAX_CHARS (from main).
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-agent/src/agent/tool_dispatch.rs (1)
553-553: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHandle the elapsed-time conversion explicitly.
Line 553 silently maps a failed
Resultconversion tou64::MAX. Use explicitmatchhandling or a named saturating helper.As per coding guidelines, do not silently discard failures with equivalent default fallbacks.
Proposed fix
- elapsed_ms = u64::try_from(elapsed.as_millis()).unwrap_or_else(|_| u64::MAX), + elapsed_ms = { + match u64::try_from(elapsed.as_millis()) { + Ok(millis) => millis, + Err(_) => u64::MAX, + } + },🤖 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-agent/src/agent/tool_dispatch.rs` at line 553, Update the elapsed_ms assignment in the tool dispatch timing logic to handle the u64 conversion explicitly rather than silently using unwrap_or_else with u64::MAX. Use an explicit match or a named saturating helper, preserving the intended maximum-value behavior while making the conversion failure handling visible.Source: Coding guidelines
🤖 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-providers/src/providers/cursor/run.rs`:
- Around line 731-784: Add handler-level tests around handle_data_frame covering
malformed exec, text, and thinking payloads with frame.end_stream set to both
true and false. Verify end-stream parser errors return the normal non-error
outcome, while non-end-stream errors propagate as bad_frame errors; do not rely
on direct parser tests alone.
- Around line 731-784: Update the delta handling in the closure within the
frame-processing function so extract_text_deltas and extract_thinking_deltas
both complete successfully before mutating the caller-owned text or thinking
buffers. Store both parsed collections first, then append their contents and
compute text_deltas, preserving the existing error propagation and outcome
behavior.
---
Outside diff comments:
In `@n00n-agent/src/agent/tool_dispatch.rs`:
- Line 553: Update the elapsed_ms assignment in the tool dispatch timing logic
to handle the u64 conversion explicitly rather than silently using
unwrap_or_else with u64::MAX. Use an explicit match or a named saturating
helper, preserving the intended maximum-value behavior while making the
conversion failure handling visible.
🪄 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: 4295cf39-7210-4dcb-9267-b78a17509e17
📒 Files selected for processing (3)
changelog.d/310.fixed.mdn00n-agent/src/agent/tool_dispatch.rsn00n-providers/src/providers/cursor/run.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: Rustdoc
- GitHub Check: Test (Windows)
- GitHub Check: Lint (Windows)
- GitHub Check: Coverage
- GitHub Check: Build (Windows)
- GitHub Check: MSRV (1.97)
- GitHub Check: Docs
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Analyze (javascript-typescript)
- 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-agent/src/agent/tool_dispatch.rsn00n-providers/src/providers/cursor/run.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/310.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/310.fixed.md
🪛 markdownlint-cli2 (0.23.2)
changelog.d/310.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 (5)
n00n-agent/src/agent/tool_dispatch.rs (4)
21-26: LGTM!Also applies to: 305-305
16-16: LGTM!Also applies to: 27-62, 538-538, 649-664, 687-694, 697-704, 858-875, 2407-2422
398-407: 🩺 Stability & AvailabilityNo change needed.
registryandctx.registryare the sameArc<ToolRegistry>at the existing call points, so lookup and admission share the same registry.> Likely an incorrect or invalid review comment.
549-557: 🔒 Security & PrivacySensitive Data Exposure (CWE-532): Insertion of Sensitive Information into Log File
Verify that tool error logs cannot contain sensitive payloads.
The registered-tool path logs a 1,024-character preview. The local-tool path logs the full error string. Length truncation is not redaction. If a tool returns provider output, credentials, prompts, or session data in its error, those values reach the tracing sink. Confirm that tool implementations sanitize errors before these lines, or log only a fixed error class and byte count.
#!/usr/bin/env bash set -euo pipefail rg -n -C 6 'LocalToolFn|ToolExecResult|error = %|warn!\(' n00n-agent/srcAs per coding guidelines, do not log raw provider payloads, prompts, credentials, or user session data.
Also applies to: 695-696
n00n-providers/src/providers/cursor/run.rs (1)
869-889: LGTM!Also applies to: 935-953
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Follow-up dirty changes: