fix: align coding agent trace scopes#130
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:XL
WalkthroughAdds an alignment subsystem that normalizes gateway requests, extracts and aliases session IDs, promotes child sessions into parent-owned subagents, and propagates correlation metadata through LLM and tool lifecycles. ChangesCross-Session Alignment and Subagent Promotion
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant Alignment
participant SessionManager
participant Session
Client->>Gateway: HTTP gateway request (headers, body)
Gateway->>Alignment: gateway_session_id(headers, body, route)
Alignment->>Gateway: resolved session_id
Gateway->>SessionManager: build_llm_gateway_start(resolved session_id, metadata)
SessionManager->>Alignment: route_event_through_alias(NormalizedEvent)
Alignment->>SessionManager: rewritten event (parent session id, alias_metadata)
SessionManager->>Session: apply_event_to_session(rewritten event)
Session->>Session: start_subagent / end_subagent (async) / close_subagent_scope
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/cli/src/alignment/codex.rs`:
- Around line 134-145: subagent_context_from_transcript performs blocking file
I/O (std::fs::File::open and BufReader::read_line) which can block an async
executor; change this to an async-safe approach by either (A) making
subagent_context_from_transcript async and using tokio::fs::File::open +
tokio::io::BufReader::read_line to read the single line asynchronously, then
parse and call subagent_context_from_value, or (B) keep it sync but wrap the
blocking work in tokio::task::spawn_blocking (move the File open + read_line +
serde_json::from_str into the spawned closure) and await the result, ensuring
callers in async contexts don't block the runtime.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b939f4b0-94da-45ec-a9d8-19444e256210
📒 Files selected for processing (11)
crates/cli/src/alignment/claude_code.rscrates/cli/src/alignment/codex.rscrates/cli/src/alignment/mod.rscrates/cli/src/gateway.rscrates/cli/src/main.rscrates/cli/src/session.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/alignment_codex_tests.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/tests/coverage/session_tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)
**/*.rs: Runcargo fmt --allfor FFI work as it is Rust work
Runjust test-rustfor FFI validation
Runcargo clippy --workspace --all-targets -- -D warningsto enforce warnings-as-errors linting
**/*.rs: Runcargo fmt --allfor Rust code formatting
Runcargo clippy --workspace --all-targets -- -D warningsto enforce Rust linting with no warnings
Runjust test-rustas the shared-runtime build/test wrapper for Rust changesUse Rust snake_case naming convention for Rust code
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor Rust code formatting when Node changes touch Rust files
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting when Rust files changed as part of Node work
**/*.rs: Always runjust test-rustwhen any Rust code changes
Always runcargo fmt --allwhen any Rust code changes
Always runcargo clippy --workspace --all-targets -- -D warningswhen any Rust code changesIf any Rust files changed as part of the Python work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
Files:
crates/cli/src/main.rscrates/cli/src/alignment/claude_code.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/alignment_codex_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/src/gateway.rscrates/cli/src/alignment/mod.rscrates/cli/src/alignment/codex.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/src/session.rs
**/*.{rs,go,js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Include SPDX license headers in all Rust, Go, JavaScript, and TypeScript source files using C-style comment syntax
Files:
crates/cli/src/main.rscrates/cli/src/alignment/claude_code.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/alignment_codex_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/src/gateway.rscrates/cli/src/alignment/mod.rscrates/cli/src/alignment/codex.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/src/session.rs
**/*.{rs,py,go,js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use
SONAR_IGNORE_START/SONAR_IGNORE_ENDmarkers only for documented false positives that cannot be resolved in code; keep ignored blocks small, add explanatory comments, and require reviewer sign-off
Files:
crates/cli/src/main.rscrates/cli/src/alignment/claude_code.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/alignment_codex_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/src/gateway.rscrates/cli/src/alignment/mod.rscrates/cli/src/alignment/codex.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/src/session.rs
**/*.{js,ts,tsx,jsx,py,rs,go,java,c,cpp,h,cc,cxx,cs,rb,php,swift,kt}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changed files must be formatted with the language-native formatter
Files:
crates/cli/src/main.rscrates/cli/src/alignment/claude_code.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/alignment_codex_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/src/gateway.rscrates/cli/src/alignment/mod.rscrates/cli/src/alignment/codex.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/src/session.rs
**/*.{py,js,ts,tsx,go,rs,md}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Format changed files with the language-native formatter before the final lint/test pass
Files:
crates/cli/src/main.rscrates/cli/src/alignment/claude_code.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/alignment_codex_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/src/gateway.rscrates/cli/src/alignment/mod.rscrates/cli/src/alignment/codex.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/src/session.rs
**/*.{rs,py,js,ts,tsx,go}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
During iteration, prefer
uv run pre-commit run --files <changed files...>for targeted validation
Files:
crates/cli/src/main.rscrates/cli/src/alignment/claude_code.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/alignment_codex_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/src/gateway.rscrates/cli/src/alignment/mod.rscrates/cli/src/alignment/codex.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/src/session.rs
{crates/**/tests/**,python/tests/**,go/nemo_flow/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_flow/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/alignment_codex_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/tests/coverage/gateway_tests.rs
🔇 Additional comments (47)
crates/cli/src/alignment/mod.rs (8)
1-20: LGTM!
21-96: LGTM!
98-192: LGTM!
194-266: LGTM!
268-349: LGTM!
351-479: LGTM!
481-536: LGTM!
538-541: LGTM!crates/cli/src/alignment/claude_code.rs (1)
1-50: LGTM!crates/cli/tests/coverage/alignment_claude_code_tests.rs (1)
1-91: LGTM!crates/cli/src/alignment/codex.rs (2)
147-245: LGTM!
247-339: LGTM!crates/cli/tests/coverage/alignment_codex_tests.rs (1)
1-269: LGTM!crates/cli/tests/coverage/alignment_tests.rs (1)
1-317: LGTM!crates/cli/src/main.rs (1)
7-7: LGTM!crates/cli/src/gateway.rs (6)
26-26: LGTM!Also applies to: 90-91
108-165: LGTM!
549-569: LGTM!
694-709: LGTM!
776-789: LGTM!
802-877: LGTM!crates/cli/tests/coverage/gateway_tests.rs (1)
5-5: LGTM!Also applies to: 82-101, 167-170, 175-178, 185-185, 194-194, 198-222, 275-319, 347-351, 364-368, 385-389, 403-407, 496-506, 516-535, 545-553, 558-562, 565-572, 575-583, 586-594
crates/cli/src/session.rs (16)
4-4: LGTM!Also applies to: 12-14, 25-28, 40-46, 100-106
136-168: LGTM!
170-177: LGTM!
187-265: LGTM!
280-339: LGTM!
391-412: LGTM!
414-464: LGTM!
467-508: LGTM!
510-597: LGTM!
614-651: LGTM!
653-677: LGTM!
679-765: LGTM!
806-881: LGTM!
905-1016: LGTM!
1032-1108: LGTM!Also applies to: 1149-1206
1239-1456: LGTM!crates/cli/tests/coverage/session_tests.rs (9)
69-83: LGTM!
159-199: LGTM!
201-286: LGTM!
288-351: LGTM!
353-525: LGTM!
527-656: LGTM!
658-964: LGTM!
2087-2367: LGTM!
3015-3023: LGTM!
| fn subagent_context_from_transcript(event: &SessionEvent) -> Option<SubagentContext> { | ||
| let transcript_path = json_string_at(&event.metadata, &[&["transcript_path"][..]]) | ||
| .or_else(|| json_string_at(&event.payload, &[&["transcript_path"][..]]))?; | ||
| let file = std::fs::File::open(transcript_path).ok()?; | ||
| let mut reader = BufReader::new(file); | ||
| let mut line = String::new(); | ||
| if reader.read_line(&mut line).ok()? == 0 { | ||
| return None; | ||
| } | ||
| let value = serde_json::from_str::<Value>(&line).ok()?; | ||
| subagent_context_from_value(&value) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Blocking file I/O on async path.
subagent_context_from_transcript performs synchronous std::fs::File::open and BufReader::read_line. If called from an async context (e.g., within a Tokio runtime), this blocks the executor thread.
Given the comment that this reads only one line and is a fallback path, the impact is likely low in practice. Consider using tokio::fs or spawn_blocking if profiling shows contention.
🤖 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 `@crates/cli/src/alignment/codex.rs` around lines 134 - 145,
subagent_context_from_transcript performs blocking file I/O (std::fs::File::open
and BufReader::read_line) which can block an async executor; change this to an
async-safe approach by either (A) making subagent_context_from_transcript async
and using tokio::fs::File::open + tokio::io::BufReader::read_line to read the
single line asynchronously, then parse and call subagent_context_from_value, or
(B) keep it sync but wrap the blocking work in tokio::task::spawn_blocking (move
the File open + read_line + serde_json::from_str into the spawned closure) and
await the result, ensuring callers in async contexts don't block the runtime.
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:XXL
Signed-off-by: Will Killian <wkillian@nvidia.com>
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 `@crates/cli/src/session.rs`:
- Around line 1829-1838: The helper llm_status_teaches_request_affinity
currently treats the "subagent_start" status as teaching request affinity;
remove "subagent_start" from the match arm so only high-confidence statuses
(explicit, single_hint, matched_hint, active_subagent, request_affinity) return
true. Update llm_status_teaches_request_affinity to exclude the "subagent_start"
variant and leave record_llm_request_affinity / llm_request_affinity behavior
unchanged so the weak heuristic does not promote a low-confidence guess into
stable request affinity.
- Around line 474-479: The apply_start_alias function currently sets
start.session_id from alias but only fills start.subagent_id when None; change
it so the alias's subagent wins: in apply_start_alias (operating on
LlmGatewayStart and SessionAlias) always set start.subagent_id =
Some(alias.subagent_id.clone()) (or alternatively return an error on mismatch if
you prefer rejection semantics) so the alias-owned subagent becomes
authoritative and cannot be accidentally left as a conflicting existing subagent
id.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4eae72c1-5d72-4bc6-8d22-42ad69481ffd
📒 Files selected for processing (4)
crates/cli/src/alignment/mod.rscrates/cli/src/session.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/session_tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)
**/*.rs: Runcargo fmt --allfor FFI work as it is Rust work
Runjust test-rustfor FFI validation
Runcargo clippy --workspace --all-targets -- -D warningsto enforce warnings-as-errors linting
**/*.rs: Runcargo fmt --allfor Rust code formatting
Runcargo clippy --workspace --all-targets -- -D warningsto enforce Rust linting with no warnings
Runjust test-rustas the shared-runtime build/test wrapper for Rust changesUse Rust snake_case naming convention for Rust code
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor Rust code formatting when Node changes touch Rust files
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting when Rust files changed as part of Node work
**/*.rs: Always runjust test-rustwhen any Rust code changes
Always runcargo fmt --allwhen any Rust code changes
Always runcargo clippy --workspace --all-targets -- -D warningswhen any Rust code changesIf any Rust files changed as part of the Python work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
Files:
crates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{rs,go,js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Include SPDX license headers in all Rust, Go, JavaScript, and TypeScript source files using C-style comment syntax
Files:
crates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{rs,py,go,js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use
SONAR_IGNORE_START/SONAR_IGNORE_ENDmarkers only for documented false positives that cannot be resolved in code; keep ignored blocks small, add explanatory comments, and require reviewer sign-off
Files:
crates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{js,ts,tsx,jsx,py,rs,go,java,c,cpp,h,cc,cxx,cs,rb,php,swift,kt}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changed files must be formatted with the language-native formatter
Files:
crates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{py,js,ts,tsx,go,rs,md}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Format changed files with the language-native formatter before the final lint/test pass
Files:
crates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{rs,py,js,ts,tsx,go}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
During iteration, prefer
uv run pre-commit run --files <changed files...>for targeted validation
Files:
crates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/session_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
{crates/**/tests/**,python/tests/**,go/nemo_flow/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_flow/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/session_tests.rs
🔇 Additional comments (3)
crates/cli/tests/coverage/alignment_tests.rs (1)
1-352: LGTM!crates/cli/tests/coverage/session_tests.rs (1)
1-3209: LGTM!crates/cli/src/alignment/mod.rs (1)
1-601: LGTM!
| fn apply_start_alias(start: &mut LlmGatewayStart, alias: &SessionAlias) { | ||
| start.session_id = Some(alias.parent_session_id.clone()); | ||
| if start.subagent_id.is_none() { | ||
| start.subagent_id = Some(alias.subagent_id.clone()); | ||
| } | ||
| start.metadata = merge_metadata(start.metadata.clone(), alias.metadata()); |
There was a problem hiding this comment.
Make the alias-owned subagent authoritative.
When start.session_id is rewritten through a child-session alias, preserving a conflicting start.subagent_id can attach that child thread's LLM to the wrong sibling scope in the parent session. The alias is the stronger ownership signal here, so this should overwrite or reject conflicting subagent ids instead of only filling None.
Suggested fix
fn apply_start_alias(start: &mut LlmGatewayStart, alias: &SessionAlias) {
start.session_id = Some(alias.parent_session_id.clone());
- if start.subagent_id.is_none() {
- start.subagent_id = Some(alias.subagent_id.clone());
- }
+ start.subagent_id = Some(alias.subagent_id.clone());
start.metadata = merge_metadata(start.metadata.clone(), alias.metadata());
}🤖 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 `@crates/cli/src/session.rs` around lines 474 - 479, The apply_start_alias
function currently sets start.session_id from alias but only fills
start.subagent_id when None; change it so the alias's subagent wins: in
apply_start_alias (operating on LlmGatewayStart and SessionAlias) always set
start.subagent_id = Some(alias.subagent_id.clone()) (or alternatively return an
error on mismatch if you prefer rejection semantics) so the alias-owned subagent
becomes authoritative and cannot be accidentally left as a conflicting existing
subagent id.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/cli/src/session.rs (2)
1829-1838:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't persist request affinity from
subagent_start.
set_last_subagent_start_owneris documented as a weak heuristic, but this helper turns"subagent_start"into durablellm_request_affinity. That can lock later requests onto the wrong worker from a guess instead of a high-confidence signal.Suggested fix
fn owner_status_teaches_request_affinity(status: &str) -> bool { matches!( status, "explicit" | "single_hint" | "matched_hint" | "active_subagent" - | "subagent_start" | "request_affinity" ) }🤖 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 `@crates/cli/src/session.rs` around lines 1829 - 1838, The helper owner_status_teaches_request_affinity currently treats "subagent_start" as a status that should persist request affinity; remove "subagent_start" from the matches so it no longer yields true. Update the match arms in owner_status_teaches_request_affinity to exclude "subagent_start" (keep the other statuses: "explicit", "single_hint", "matched_hint", "active_subagent", "request_affinity") and add/adjust any unit tests or comments referring to persisting affinity from subagent_start accordingly.
474-479:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the alias-owned subagent authoritative.
Once Line 475 rewrites the LLM start onto the parent session, preserving an existing
start.subagent_idcan still attach the call and its later response hints to the wrong sibling scope. The alias is the stronger ownership signal here, so it should overwrite any conflicting subagent id.Suggested fix
fn apply_start_alias(start: &mut LlmGatewayStart, alias: &SessionAlias) { start.session_id = Some(alias.parent_session_id.clone()); - if start.subagent_id.is_none() { - start.subagent_id = Some(alias.subagent_id.clone()); - } + start.subagent_id = Some(alias.subagent_id.clone()); start.metadata = merge_metadata(start.metadata.clone(), alias.metadata()); }🤖 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 `@crates/cli/src/session.rs` around lines 474 - 479, In apply_start_alias replace the conditional preservation of start.subagent_id so the alias-owned subagent becomes authoritative: always assign start.subagent_id = Some(alias.subagent_id.clone()) (instead of only when None) after copying the parent session id; keep the existing session id assignment and metadata merge (LlmGatewayStart, SessionAlias, start.subagent_id, alias.subagent_id, merge_metadata).
🤖 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.
Duplicate comments:
In `@crates/cli/src/session.rs`:
- Around line 1829-1838: The helper owner_status_teaches_request_affinity
currently treats "subagent_start" as a status that should persist request
affinity; remove "subagent_start" from the matches so it no longer yields true.
Update the match arms in owner_status_teaches_request_affinity to exclude
"subagent_start" (keep the other statuses: "explicit", "single_hint",
"matched_hint", "active_subagent", "request_affinity") and add/adjust any unit
tests or comments referring to persisting affinity from subagent_start
accordingly.
- Around line 474-479: In apply_start_alias replace the conditional preservation
of start.subagent_id so the alias-owned subagent becomes authoritative: always
assign start.subagent_id = Some(alias.subagent_id.clone()) (instead of only when
None) after copying the parent session id; keep the existing session id
assignment and metadata merge (LlmGatewayStart, SessionAlias, start.subagent_id,
alias.subagent_id, merge_metadata).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1632ae85-1d84-492f-98ff-cf3ef67fb7ce
📒 Files selected for processing (4)
crates/cli/src/alignment/mod.rscrates/cli/src/session.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/session_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Rust / Test (linux-amd64)
- GitHub Check: Rust / Test (linux-arm64)
- GitHub Check: Rust / Test (windows-arm64)
- GitHub Check: Rust / Test (macos-arm64)
- GitHub Check: Rust / Test (windows-amd64)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)
**/*.rs: Runcargo fmt --allfor FFI work as it is Rust work
Runjust test-rustfor FFI validation
Runcargo clippy --workspace --all-targets -- -D warningsto enforce warnings-as-errors linting
**/*.rs: Runcargo fmt --allfor Rust code formatting
Runcargo clippy --workspace --all-targets -- -D warningsto enforce Rust linting with no warnings
Runjust test-rustas the shared-runtime build/test wrapper for Rust changesUse Rust snake_case naming convention for Rust code
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor Rust code formatting when Node changes touch Rust files
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting when Rust files changed as part of Node work
**/*.rs: Always runjust test-rustwhen any Rust code changes
Always runcargo fmt --allwhen any Rust code changes
Always runcargo clippy --workspace --all-targets -- -D warningswhen any Rust code changesIf any Rust files changed as part of the Python work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
Files:
crates/cli/tests/coverage/alignment_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{rs,go,js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Include SPDX license headers in all Rust, Go, JavaScript, and TypeScript source files using C-style comment syntax
Files:
crates/cli/tests/coverage/alignment_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{rs,py,go,js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use
SONAR_IGNORE_START/SONAR_IGNORE_ENDmarkers only for documented false positives that cannot be resolved in code; keep ignored blocks small, add explanatory comments, and require reviewer sign-off
Files:
crates/cli/tests/coverage/alignment_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{js,ts,tsx,jsx,py,rs,go,java,c,cpp,h,cc,cxx,cs,rb,php,swift,kt}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changed files must be formatted with the language-native formatter
Files:
crates/cli/tests/coverage/alignment_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{py,js,ts,tsx,go,rs,md}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Format changed files with the language-native formatter before the final lint/test pass
Files:
crates/cli/tests/coverage/alignment_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{rs,py,js,ts,tsx,go}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
During iteration, prefer
uv run pre-commit run --files <changed files...>for targeted validation
Files:
crates/cli/tests/coverage/alignment_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
{crates/**/tests/**,python/tests/**,go/nemo_flow/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_flow/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/cli/tests/coverage/alignment_tests.rs
Signed-off-by: Will Killian <wkillian@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/cli/src/session.rs (2)
2159-2169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't teach request affinity from
subagent_start.
subagent_startis documented as a weak heuristic (lines 1789-1800), butowner_status_teaches_request_affinitypromotes it into stablellm_request_affinity. This can lock later matching requests onto the wrong worker from a guess, defeating the "high-confidence only" intent inrecord_llm_request_affinity.Suggested fix
fn owner_status_teaches_request_affinity(status: &str) -> bool { matches!( status, "explicit" | "single_hint" | "matched_hint" | "active_subagent" - | "subagent_start" | "request_affinity" ) }🤖 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 `@crates/cli/src/session.rs` around lines 2159 - 2169, The function owner_status_teaches_request_affinity currently treats "subagent_start" as a status that promotes stable llm_request_affinity; remove "subagent_start" from owner_status_teaches_request_affinity so that only high-confidence statuses (explicit, single_hint, matched_hint, active_subagent, request_affinity) return true, and update any related tests or callers (e.g., record_llm_request_affinity) to rely on the revised predicate; ensure no other code still assumes subagent_start implies stable affinity.
538-543:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the alias-owned subagent authoritative.
When
start.session_idis rewritten through a child-session alias, preserving a conflictingstart.subagent_idcan attach that child thread's LLM to the wrong sibling scope in the parent session. The alias is the stronger ownership signal here, so this should overwrite or reject conflicting subagent ids instead of only fillingNone.Suggested fix
fn apply_start_alias(start: &mut LlmGatewayStart, alias: &SessionAlias) { start.session_id = Some(alias.parent_session_id.clone()); - if start.subagent_id.is_none() { - start.subagent_id = Some(alias.subagent_id.clone()); - } + start.subagent_id = Some(alias.subagent_id.clone()); start.metadata = merge_metadata(start.metadata.clone(), alias.metadata()); }🤖 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 `@crates/cli/src/session.rs` around lines 538 - 543, In apply_start_alias, when applying a child-session alias to an LlmGatewayStart, the alias's subagent must be authoritative: always set start.subagent_id to Some(alias.subagent_id.clone()) instead of only assigning when start.subagent_id.is_none(); keep the existing start.session_id assignment and metadata merge (merge_metadata(start.metadata.clone(), alias.metadata())), but replace the conditional subagent fill with an unconditional overwrite so the alias's subagent cannot be left conflicting with the rewritten session_id.
🤖 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 `@crates/cli/src/alignment/mod.rs`:
- Line 22: REQUEST_AFFINITY_KEY_MAX_CHARS is defined but the 24-character
minimum inside request_affinity_key is a magic number; add a named constant
REQUEST_AFFINITY_KEY_MIN_CHARS = 24 and replace the hardcoded 24 in the
request_affinity_key function (and any other occurrences around the
request-affinity logic) with this constant to make the policy explicit and
consistent with REQUEST_AFFINITY_KEY_MAX_CHARS.
- Around line 584-589: The function looks_like_json_payload currently attempts
to parse the entire input with serde_json on every call; change it to first trim
leading whitespace and short-circuit non-JSON by returning false unless the
first non-whitespace char is '{' or '[' (indicating object/array), and only then
call serde_json::from_str::<Value> to confirm Ok(Value::Object(_) |
Value::Array(_)); update the function name and logic where needed to reference
looks_like_json_payload so callers get the optimized behavior.
---
Duplicate comments:
In `@crates/cli/src/session.rs`:
- Around line 2159-2169: The function owner_status_teaches_request_affinity
currently treats "subagent_start" as a status that promotes stable
llm_request_affinity; remove "subagent_start" from
owner_status_teaches_request_affinity so that only high-confidence statuses
(explicit, single_hint, matched_hint, active_subagent, request_affinity) return
true, and update any related tests or callers (e.g.,
record_llm_request_affinity) to rely on the revised predicate; ensure no other
code still assumes subagent_start implies stable affinity.
- Around line 538-543: In apply_start_alias, when applying a child-session alias
to an LlmGatewayStart, the alias's subagent must be authoritative: always set
start.subagent_id to Some(alias.subagent_id.clone()) instead of only assigning
when start.subagent_id.is_none(); keep the existing start.session_id assignment
and metadata merge (merge_metadata(start.metadata.clone(), alias.metadata())),
but replace the conditional subagent fill with an unconditional overwrite so the
alias's subagent cannot be left conflicting with the rewritten session_id.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1c07dd5d-3a4a-4b05-86f7-e1741d04a55c
📒 Files selected for processing (13)
crates/cli/src/adapters/mod.rscrates/cli/src/alignment/claude_code.rscrates/cli/src/alignment/mod.rscrates/cli/src/gateway.rscrates/cli/src/server.rscrates/cli/src/session.rscrates/cli/tests/coverage/adapters_tests.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/tests/coverage/session_tests.rscrates/core/src/observability/plugin_component.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check / Run
🧰 Additional context used
📓 Path-based instructions (11)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)
**/*.rs: Runcargo fmt --allfor FFI work as it is Rust work
Runjust test-rustfor FFI validation
Runcargo clippy --workspace --all-targets -- -D warningsto enforce warnings-as-errors linting
**/*.rs: Runcargo fmt --allfor Rust code formatting
Runcargo clippy --workspace --all-targets -- -D warningsto enforce Rust linting with no warnings
Runjust test-rustas the shared-runtime build/test wrapper for Rust changesUse Rust snake_case naming convention for Rust code
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor Rust code formatting when Node changes touch Rust files
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting when Rust files changed as part of Node work
**/*.rs: Always runjust test-rustwhen any Rust code changes
Always runcargo fmt --allwhen any Rust code changes
Always runcargo clippy --workspace --all-targets -- -D warningswhen any Rust code changesIf any Rust files changed as part of the Python work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
Files:
crates/cli/src/server.rscrates/cli/tests/coverage/adapters_tests.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/src/alignment/claude_code.rscrates/core/src/observability/plugin_component.rscrates/cli/src/adapters/mod.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/src/gateway.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{rs,go,js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Include SPDX license headers in all Rust, Go, JavaScript, and TypeScript source files using C-style comment syntax
Files:
crates/cli/src/server.rscrates/cli/tests/coverage/adapters_tests.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/src/alignment/claude_code.rscrates/core/src/observability/plugin_component.rscrates/cli/src/adapters/mod.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/src/gateway.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{rs,py,go,js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use
SONAR_IGNORE_START/SONAR_IGNORE_ENDmarkers only for documented false positives that cannot be resolved in code; keep ignored blocks small, add explanatory comments, and require reviewer sign-off
Files:
crates/cli/src/server.rscrates/cli/tests/coverage/adapters_tests.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/src/alignment/claude_code.rscrates/core/src/observability/plugin_component.rscrates/cli/src/adapters/mod.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/src/gateway.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{js,ts,tsx,jsx,py,rs,go,java,c,cpp,h,cc,cxx,cs,rb,php,swift,kt}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changed files must be formatted with the language-native formatter
Files:
crates/cli/src/server.rscrates/cli/tests/coverage/adapters_tests.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/src/alignment/claude_code.rscrates/core/src/observability/plugin_component.rscrates/cli/src/adapters/mod.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/src/gateway.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{py,js,ts,tsx,go,rs,md}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Format changed files with the language-native formatter before the final lint/test pass
Files:
crates/cli/src/server.rscrates/cli/tests/coverage/adapters_tests.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/src/alignment/claude_code.rscrates/core/src/observability/plugin_component.rscrates/cli/src/adapters/mod.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/src/gateway.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
**/*.{rs,py,js,ts,tsx,go}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
During iteration, prefer
uv run pre-commit run --files <changed files...>for targeted validation
Files:
crates/cli/src/server.rscrates/cli/tests/coverage/adapters_tests.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/src/alignment/claude_code.rscrates/core/src/observability/plugin_component.rscrates/cli/src/adapters/mod.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/src/gateway.rscrates/cli/tests/coverage/alignment_tests.rscrates/cli/src/alignment/mod.rscrates/cli/src/session.rs
{crates/**/tests/**,python/tests/**,go/nemo_flow/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_flow/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/cli/tests/coverage/adapters_tests.rscrates/cli/tests/coverage/alignment_claude_code_tests.rscrates/cli/tests/coverage/server_tests.rscrates/cli/tests/coverage/gateway_tests.rscrates/cli/tests/coverage/alignment_tests.rs
crates/{core,adaptive}/**
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes to
crates/coreorcrates/adaptivemust run the full language matrix
Files:
crates/core/src/observability/plugin_component.rs
{crates/core,crates/adaptive}/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-wasm-binding/SKILL.md)
If the change touched shared runtime semantics in
crates/coreorcrates/adaptive, also usevalidate-change
Files:
crates/core/src/observability/plugin_component.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
When
crates/corechanges, run the full validation matrix across Rust, Python, Go, Node.js, and WebAssembly
crates/core/**/*.rs: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths. Keep errors explicit and binding-appropriate at the wrapper layer.
Files:
crates/core/src/observability/plugin_component.rs
crates/{core,adaptive}/**/*.rs
⚙️ CodeRabbit configuration file
crates/{core,adaptive}/**/*.rs: Review the Rust runtime for async correctness, scope isolation, middleware ordering, and event lifecycle regressions.
Pay close attention to task-local/thread-local scope propagation, callback lifetimes, stream finalization, and root_uuid isolation.
Public API changes should preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/core/src/observability/plugin_component.rs
🔇 Additional comments (20)
crates/cli/src/server.rs (1)
110-110: LGTM!crates/cli/tests/coverage/adapters_tests.rs (1)
622-623: LGTM!Also applies to: 639-644
crates/cli/tests/coverage/alignment_claude_code_tests.rs (1)
1-138: LGTM!crates/cli/tests/coverage/server_tests.rs (1)
276-277: LGTM!Also applies to: 306-309
crates/cli/src/alignment/claude_code.rs (1)
19-77: LGTM!crates/core/src/observability/plugin_component.rs (1)
15-17: LGTM!Also applies to: 174-179, 204-204, 646-647, 650-673, 862-875
crates/cli/src/adapters/mod.rs (1)
384-393: LGTM!crates/cli/tests/coverage/gateway_tests.rs (1)
5-5: LGTM!Also applies to: 8-8, 82-101, 167-170, 176-178, 185-185, 194-223, 275-319, 347-351, 364-368, 385-389, 403-407, 496-506, 516-535, 545-553, 558-562, 565-593, 669-720, 722-796, 798-801
crates/cli/src/gateway.rs (4)
562-631: LGTM!The
GatewayCallGuardcorrectly implements idempotent finalization:sessions.take()ensures either the asyncfinish()path or theDroppath executes cleanup exactly once. Thetry_current().spawn()pattern in Drop is appropriate for fire-and-forget cleanup when the stream is dropped mid-flight.
878-891: LGTM!
904-979: LGTM!
26-31: LGTM!Also applies to: 90-91, 117-117, 270-270, 283-286, 371-371, 375-384, 415-421, 427-434, 436-439, 530-560, 652-653, 662-662, 674-676, 796-798
crates/cli/tests/coverage/alignment_tests.rs (1)
1-538: LGTM!Comprehensive test coverage for alignment behavior: gateway session/subagent/identifier precedence, agent kind inference, session scope policy, request affinity key extraction across provider formats, event routing through aliases, and JSON/metadata utilities.
crates/cli/src/alignment/mod.rs (1)
1-658: LGTM!crates/cli/src/session.rs (6)
220-244: LGTM!Idle sweeper correctly uses weak references to avoid extending manager lifetime, and skips sessions with active gateway calls, LLMs, or tools.
1204-1254: LGTM!Per-subagent scope stacks correctly enable out-of-order parallel subagent completion without corrupting the task-local stack. The stack is seeded with parent scopes and stored separately.
1286-1326: LGTM!
close_subagent_scopecorrectly cleans up: removes from subagents/stacks, pops scope using the subagent's own stack, marks completed, and prunes tool hints and request affinity tied to the closed subagent.
1407-1424: LGTM!
hook_llm_ownercorrectly recovers subagent ownership fromllm_correlation_subagent_idmetadata stamped by alignment routing, keeping aliased child LLMs under the correct subagent scope.
1570-1646: LGTM!LLM owner resolution correctly adds
request_affinity_ownerin the precedence chain between matched hints and sticky owner, with proper cleanup when the affinity-mapped subagent no longer exists.
4-4: LGTM!Also applies to: 12-14, 25-28, 38-48, 96-126, 150-191, 749-814, 983-1059, 1149-1162, 1164-1170, 1776-1800, 1802-1830, 1848-1868
| pub(crate) mod claude_code; | ||
| pub(crate) mod codex; | ||
|
|
||
| const REQUEST_AFFINITY_KEY_MAX_CHARS: usize = 4096; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Document the affinity key minimum length constant.
REQUEST_AFFINITY_KEY_MAX_CHARS is defined but the 24-character minimum in request_affinity_key is a magic number. Consider extracting it to a named constant (e.g., REQUEST_AFFINITY_KEY_MIN_CHARS) for consistency and to clarify the policy.
Also applies to: 373-378
🤖 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 `@crates/cli/src/alignment/mod.rs` at line 22, REQUEST_AFFINITY_KEY_MAX_CHARS
is defined but the 24-character minimum inside request_affinity_key is a magic
number; add a named constant REQUEST_AFFINITY_KEY_MIN_CHARS = 24 and replace the
hardcoded 24 in the request_affinity_key function (and any other occurrences
around the request-affinity logic) with this constant to make the policy
explicit and consistent with REQUEST_AFFINITY_KEY_MAX_CHARS.
| fn looks_like_json_payload(text: &str) -> bool { | ||
| matches!( | ||
| serde_json::from_str::<Value>(text), | ||
| Ok(Value::Object(_) | Value::Array(_)) | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
JSON parse on every candidate could be costly for large tool-result payloads.
looks_like_json_payload parses the entire string to detect object/array structure. For multi-KB tool results this adds overhead on every affinity check. A prefix heuristic (check if trimmed text starts with { or [ before parsing) would short-circuit obvious non-JSON cases.
♻️ Suggested optimization
fn looks_like_json_payload(text: &str) -> bool {
+ let first = text.trim_start().chars().next();
+ if !matches!(first, Some('{') | Some('[')) {
+ return false;
+ }
matches!(
serde_json::from_str::<Value>(text),
Ok(Value::Object(_) | Value::Array(_))
)
}🤖 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 `@crates/cli/src/alignment/mod.rs` around lines 584 - 589, The function
looks_like_json_payload currently attempts to parse the entire input with
serde_json on every call; change it to first trim leading whitespace and
short-circuit non-JSON by returning false unless the first non-whitespace char
is '{' or '[' (indicating object/array), and only then call
serde_json::from_str::<Value> to confirm Ok(Value::Object(_) | Value::Array(_));
update the function name and logic where needed to reference
looks_like_json_payload so callers get the optimized behavior.
1 participant
Overview
This PR improves CLI trace alignment for coding-agent runs so LLM and tool spans correlate to the correct turn, agent, or subagent scope across Codex and Claude Code flows. It keeps long-lived harness sessions as metadata, represents user-visible work as bounded turn agent scopes, and moves provider-specific correlation into alignment modules so session and gateway code can stay focused on lifecycle orchestration.
Details
crates/cli/src/alignment/as the owner for Codex- and Claude Code-specific trace alignment, session id recovery, subagent aliasing, Codex ChatGPT OAuth gateway normalization, and reusable metadata helpers.SessionStartevents record metadata but do not emit long-lived top-level session spans.codex-turn,claude-code-turn, andhermes-turn, withnemo_flow_scope_role,turn_index, and harness metadata for filtering.agentscopes under the active turn, including Codex child-thread promotion and Claude CodeAgenttool completion cleanup.crates/cli/tests/coveragefor alignment adapters, gateway identifier/auth behavior, session aliasing, subagent cleanup, turn naming/output, idle cleanup, streaming final responses, and LLM/tool correlation.cargo fmt --all -- --checkgit diff --checkcargo test -p nemo-flow-clicargo test -p nemo-flow observabilitycargo clippy -p nemo-flow-cli --all-targets -- -D warningscargo fmt,cargo clippy, andcargo checkWhere should the reviewer start?
crates/cli/src/alignment/mod.rsto understand the boundary: alignment owns provider-specific interpretation and cross-session alias state, while session/gateway code stays generic.crates/cli/src/session.rsby lifecycle path rather than linearly:start_agent,open_turn,start_subagent,close_turn,resolve_llm_owner, andresolve_tool_ownerare the main decisions.agentscopes; subagent scopes still emit under the active turn.crates/cli/src/alignment/codex.rsandcrates/cli/src/alignment/claude_code.rsas adapters. Codex covers child-thread parentage and ChatGPT OAuth gateway normalization; Claude Code covers native session headers andAgenttool completion.crates/cli/src/gateway.rsfor the managed execution handoff, especially streaming final-response capture and active-call accounting for idle cleanup.alignment_tests.rsfor generic alias routing,alignment_codex_tests.rs/alignment_claude_code_tests.rsfor adapter-specific behavior,session_tests.rsfor turn/subagent ownership, andgateway_tests.rsfor gateway handoff cases.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Refactor
Tests
Other