fix: remediate Orca runtime and architecture audit - #17
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis release updates runtime ownership, typed surface APIs, task-tree cancellation, provider-neutral tool schemas, TUI session handling, transcript performance, contract validation, CI workflows, documentation, and v0.3.3 release metadata. Changesv0.3.3 audit remediation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
crates/orca-provider/examples/update_plan_strict_realapi.rs (1)
260-264: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the verdict label to match the new check.
provider_path_checkno longer validates the schema or executes the tool. It parses the returned arguments and confirms thatplanis an array. The printed verdict still claims "validates + executes", so the output misreports what the example checked.📝 Proposed wording fix
verdict( - "default endpoint: update_plan call validates + executes", + "default endpoint: update_plan call returns JSON arguments with a plan array", default_ok, &default_detail, );🤖 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/orca-provider/examples/update_plan_strict_realapi.rs` around lines 260 - 264, Update the verdict label in the provider_path_check result to describe parsing the returned arguments and confirming that plan is an array, replacing the outdated “validates + executes” wording while leaving the check and verdict behavior unchanged.crates/orca-tui/src/app.rs (2)
9649-9651: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReap the previous thread through
reap_hosted_threadhere too.
install_hosted_sessiondisposes of a replaced thread withreap_hosted_thread, which retries off the controller thread. This goal-resume path still callsprevious.shutdown()inline. A slow shutdown therefore blocks the controller loop and delays every queued user action, which is the conditionreap_hosted_threadwas added to avoid. Use the same helper.♻️ Proposed change
if let Some(previous) = thread.take() { - let _ = previous.shutdown(); + reap_hosted_thread(previous); }🤖 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/orca-tui/src/app.rs` around lines 9649 - 9651, Update the goal-resume replacement path around the thread handoff to use reap_hosted_thread for the previous thread instead of calling previous.shutdown() inline. Preserve the existing conditional ownership transfer and ensure shutdown is retried off the controller thread, matching install_hosted_session behavior.
781-792: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCI blocker: the runtime surface contract validator reports drift for
resume_terminal_render.Both Windows jobs fail with "Runtime surface contract validation failed: harmless associated TUI function drifted for resume_terminal_render", from
node --test scripts/test-validate-runtime-surface-contract.mjs. The validator tracks this symbol as a "harmless associated TUI function", butresume_terminal_renderis declared here as a free function. Either update the expected entry in the validator to match the current declaration, or restore the shape the contract expects. The release cannot ship while this check fails.#!/bin/bash # Locate the contract entry for this symbol and the classifier that emits the message. rg -nP -C6 'resume_terminal_render' scripts/ rg -nP -C6 'harmless associated TUI function' scripts/🤖 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/orca-tui/src/app.rs` around lines 781 - 792, Resolve the runtime surface contract drift for resume_terminal_render by locating its validator entry and classifier in the runtime-surface validation script, then either update the expected declaration shape to match this free function or restore the function’s expected associated-TUI shape. Ensure the validator passes without changing unrelated symbols.Source: Pipeline failures
crates/orca-tui/src/types.rs (2)
1890-1900: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
replace_messagerebuilds the whole tool-call index on the streaming path.
rebuild_tool_call_indiceswalks every message.replace_messagenow calls it on every replacement.StreamingMarkdownAction::FreezeTailcallsreplace_messagefor each frozen assistant chunk, so this is an O(n) scan per streamed block. The test at line 2588 incrates/orca-tui/src/transcript_view.rsbuilds 1000 such blocks.Apply the same narrowing that
mutate_messageuses at lines 1909 to 1920: rebuild only when the tool-call identity at that index changes.⚡ Proposed fix
pub(crate) fn replace_message(&mut self, index: usize, message: ChatMessage) -> bool { self.reconcile_message_tracking(); if index >= self.messages.len() { return false; } self.remove_applied_highlight_for_message(index); + let previous_tool_id = match &self.messages[index] { + ChatMessage::ToolCall { id, .. } => Some(id.clone()), + _ => None, + }; + let next_tool_id = match &message { + ChatMessage::ToolCall { id, .. } => Some(id.clone()), + _ => None, + }; self.messages[index] = message; - self.rebuild_tool_call_indices(); + if previous_tool_id != next_tool_id { + self.rebuild_tool_call_indices(); + } self.touch_message(index); true }🤖 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/orca-tui/src/types.rs` around lines 1890 - 1900, Update replace_message to avoid unconditionally calling rebuild_tool_call_indices after replacing a message. Before assignment, capture the existing tool-call identity at index, then compare it with the replacement’s identity and rebuild only when that identity changes, matching the narrowing logic used by mutate_message; preserve the existing bounds check, highlight removal, touch_message call, and return behavior.
1644-1661: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLength-based change detection misses in-place tool-id edits.
structure_changedonly comparesmessage_revisions.len()withmessages.len().messagesispub(crate), so any code in the crate can replace aChatMessage::ToolCallid in place without changing the length.reconcile_message_trackingthen skipsrebuild_tool_call_indices, andassert_tool_call_index_consistenton line 1660 fails thedebug_assert_eq!in debug builds. In release builds the lookups return the wrong message index.No current caller reaches that state. The test at line 8551 edits a tool id in place, but
apply_edit_highlight_resultreturns before it callstouch_message, so the assertion is not evaluated.Make the invariant enforceable rather than incidental. Route every in-place edit through
mutate_message, which already detects tool-id changes, and narrowmessagesfrompub(crate)to private with accessors.🤖 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/orca-tui/src/types.rs` around lines 1644 - 1661, Make the message-tracking invariant enforceable by changing the messages field from pub(crate) to private and exposing only appropriate accessors. Update all in-place message mutations to go through mutate_message, which detects tool-id changes and triggers index rebuilding; adjust callers, including the edit-highlight path and tests, to use that API. Keep reconcile_message_tracking responsible for length-based reconciliation while ensuring tool-call index consistency after every mutation.crates/orca-tui/src/surface_client.rs (1)
2329-2362: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThe background monitor emits full-thread projection snapshots concurrently with the foreground drain loop.
monitor_background_presentationruns on its own thread and forwardsSurfaceProjectionSyncedfor the whole thread state.drain_operation_with_boundaryforwards the same event kind from the foreground path. The two producers have no shared ordering.
AppState::apply_surface_projection_stateincrates/orca-tui/src/types.rsapplies each snapshot unconditionally. A late snapshot from this monitor can therefore revertusage,usage_revision,title, andactive_surface_operation_idto older values.Fence this event by revision, or restrict the background monitor to the fields it owns. See the consolidated comment for the shared fix.
🤖 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/orca-tui/src/surface_client.rs` around lines 2329 - 2362, Prevent monitor-generated SurfaceProjectionSynced snapshots from overwriting newer foreground state: update the shared projection application path around AppState::apply_surface_projection_state to accept a snapshot only when its usage_revision is at least as new as the currently applied revision, while preserving newer values for usage, title, and active_surface_operation_id. Ensure both monitor_background_presentation and drain_operation_with_boundary use this fenced behavior.
🧹 Nitpick comments (29)
.github/workflows/release.yml (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the
actions/setup-nodemajor version across workflows.This workflow and
.github/workflows/runtime-contract.ymlpinactions/setup-node@v5, while.github/workflows/windows-ci.ymlpinsactions/setup-node@v6. All three run the same Node validation scripts. Use one major version so the Node setup behavior stays identical across CI surfaces.🤖 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 @.github/workflows/release.yml around lines 22 - 24, Update the actions/setup-node reference in the release workflow and runtime-contract workflow to use the same major version as windows-ci.yml, ensuring all Node validation workflows share identical setup behavior..github/workflows/runtime-contract.yml (1)
33-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a concurrency group to cancel superseded runs.
The job runs on every push to a pull request branch and on every push to
main. Without a concurrency group, older runs keep consuming runners while a newer commit is validated.♻️ Proposed concurrency group
permissions: contents: read +concurrency: + group: runtime-contract-${{ github.ref }} + cancel-in-progress: true + jobs: validate: runs-on: ubuntu-latest🤖 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 @.github/workflows/runtime-contract.yml around lines 33 - 42, Add a workflow-level concurrency group to runtime-contract.yml that identifies runs by workflow and ref, and enable cancellation of in-progress runs so superseded pull-request or main-branch validations are stopped.crates/orca-runtime/src/tool_turn.rs (1)
107-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving
root_task_idnext totask_registryfor consistency.The subagent batch path carries
root_task_idinRuntimeSubagentBatchToolTurnRuntime, next totask_registry(Lines 327-328). The normal path carries the same value inRuntimeNormalToolTurnRequestinstead ofRuntimeNormalToolTurnRuntime, which already holdstask_registryandcancel.root_task_ididentifies a task-registry entry, so the runtime struct is the more cohesive home. Aligning both paths makes the ownership boundary easier to follow.🤖 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/orca-runtime/src/tool_turn.rs` at line 107, Move root_task_id from RuntimeNormalToolTurnRequest into RuntimeNormalToolTurnRuntime, placing it alongside task_registry and cancel; update all constructors, call sites, and field accesses accordingly while preserving the existing value flow and keeping the subagent batch representation consistent.README.md (1)
112-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider translation parity for the new Reliability section.
This PR adds the Reliability section to
README.mdandREADME.zh-CN.md. The repository also shipsREADME.ja-JP.md,README.vi.md,README.ko-KR.md,README.es-419.md, andREADME.pt-BR.md. Those files will not describe the new reliability guarantees. Add the section to the remaining translations, or track the gap.🤖 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 `@README.md` around lines 112 - 126, The new Reliability section is missing from the remaining localized README files. Add equivalent translated Reliability sections to README.ja-JP.md, README.vi.md, README.ko-KR.md, README.es-419.md, and README.pt-BR.md, preserving the same five guarantees and placement used in the existing README translations.crates/orca-runtime/tests/runtime_host.rs (1)
2865-2974: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting this test into a completion test and a cancellation test.
The function builds two independent hosts and threads. Lines 2867-2931 cover background completion and event ordering. Lines 2933-2973 cover shutdown cancellation. A failure in either half reports the same test name, which makes attribution harder. Two tests also let the cancellation half run without the completion setup cost.
Also note Line 2901: the assertion that
release_markerdoes not exist can only observe a file that the test itself writes at Line 2912, so it does not observe provider state. Assert on the task record or an observed event instead.🤖 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/orca-runtime/tests/runtime_host.rs` around lines 2865 - 2974, Split background_controller_trace_equivalence into separate completion/event-ordering and shutdown-cancellation tests, preserving each half’s setup and assertions so failures identify the affected behavior. In the completion test, replace the release_marker.exists assertion with an assertion on the admitted task record or an observed event that verifies the provider remains in-flight; keep the test-controlled marker write only for releasing completion.crates/orca-runtime/src/acp/supervisor.rs (1)
6360-6390: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit the two expectations in
assert_persisted_terminal_wait_limit_state.The predicate compares a per-event
matches!result againstexpect_completedand then ANDs a secondmatches!on the same event. The two branches happen to be correct: thetruebranch requires aCompletedwait-for-exit event, and thefalsebranch is satisfiable only by anObservationUnavailablewait-for-exit event. The reason is not visible from the code, because the first operand is also true for every unrelated event.State the expected state directly instead. The assertion then reads as one claim per branch.
♻️ Proposed clarity refactor
fn assert_persisted_terminal_wait_limit_state(path: &std::path::Path, expect_completed: bool) { let events = persisted_surface_events(path); - assert!(events.iter().any(|event| { - matches!( - event, - crate::surface::SurfaceEvent::Tool( - crate::surface::ToolPatch::CapabilityCallChanged { - call: crate::surface::SurfaceCapabilityCall { - kind: crate::surface::SurfaceCapabilityCallKind::TerminalWaitForExit, - state: crate::surface::SurfaceCapabilityCallState::Completed { .. }, - .. - }, - }, - ) - ) == expect_completed - && matches!( - event, - crate::surface::SurfaceEvent::Tool( - crate::surface::ToolPatch::CapabilityCallChanged { - call: crate::surface::SurfaceCapabilityCall { - kind: crate::surface::SurfaceCapabilityCallKind::TerminalWaitForExit, - state: - crate::surface::SurfaceCapabilityCallState::Completed { .. } - | crate::surface::SurfaceCapabilityCallState::ObservationUnavailable { .. }, - .. - }, - }, - ) - ) - })); + let observed = events.iter().any(|event| { + matches!( + event, + crate::surface::SurfaceEvent::Tool( + crate::surface::ToolPatch::CapabilityCallChanged { + call: crate::surface::SurfaceCapabilityCall { + kind: crate::surface::SurfaceCapabilityCallKind::TerminalWaitForExit, + state, + .. + }, + }, + ) if if expect_completed { + matches!(state, crate::surface::SurfaceCapabilityCallState::Completed { .. }) + } else { + matches!( + state, + crate::surface::SurfaceCapabilityCallState::ObservationUnavailable { .. } + ) + } + ) + }); + assert!( + observed, + "expected terminal wait-for-exit state for expect_completed={expect_completed}" + ); }🤖 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/orca-runtime/src/acp/supervisor.rs` around lines 6360 - 6390, Refactor assert_persisted_terminal_wait_limit_state so the assertion explicitly branches on expect_completed: require a TerminalWaitForExit event with Completed state when true, and an event with ObservationUnavailable state when false. Remove the current per-event boolean comparison and separate matches! conjunction, while preserving the persisted event search.crates/orca-provider/src/tool_schema.rs (1)
80-82: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueHandle array-form
itemsandprefixItemsfor future definitions.
require_all_propertiesrecurses intoitemsonly when the value is an object. JSON Schema also allows tuple form, whereitems(orprefixItemsin 2020-12) is an array of subschemas. In that case the early return in the recursion leaves the branch schemas untouched, and theirrequiredlists stay narrower than strict mode expects.No current definition uses tuple form, so this is defensive only.
♻️ Proposed defensive handling
- if let Some(items) = object.get_mut("items") { - require_all_properties(items); - } + for keyword in ["items", "prefixItems"] { + match object.get_mut(keyword) { + Some(Value::Array(branches)) => { + for branch in branches { + require_all_properties(branch); + } + } + Some(items) => require_all_properties(items), + None => {} + } + }🤖 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/orca-provider/src/tool_schema.rs` around lines 80 - 82, Update require_all_properties to handle array-form items by recursively applying the same schema transformation to every subschema in the array, while preserving the existing object-form handling. Add equivalent traversal for prefixItems arrays so tuple branch schemas receive complete required lists as well.crates/orca-provider/src/deepseek_http.rs (1)
972-978: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the two tests that still claim path defaulting.
parse_list_files_without_path_defaults_to_dotandparse_glob_with_pattern_only_defaults_path_to_dotnow assert thattargetisNone. The parser no longer derives a default path. The names describe removed behavior and will mislead future readers.♻️ Proposed renames
- fn parse_list_files_without_path_defaults_to_dot() { + fn parse_list_files_without_path_derives_no_target() {- fn parse_glob_with_pattern_only_defaults_path_to_dot() { + fn parse_glob_with_pattern_only_derives_no_target() {Also applies to: 1129-1137
🤖 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/orca-provider/src/deepseek_http.rs` around lines 972 - 978, Rename the tests parse_list_files_without_path_defaults_to_dot and parse_glob_with_pattern_only_defaults_path_to_dot to reflect that missing paths leave target as None, removing the outdated “defaults to dot” wording while preserving their assertions and behavior.crates/orca-runtime/src/lib.rs (2)
59-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the blanket
allowonruntime_surface.
#[allow(dead_code, unused_imports)]applies to the whole module tree. It suppresses dead-code detection for every item insideruntime_surface, not only the items that external fixtures exercise. Genuinely unused internal code will then stay invisible.Prefer placing the
allowon the specific items that the compiler reports as unused, or gate the fixture-only surface behind a feature or#[cfg(test)]re-export.🤖 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/orca-runtime/src/lib.rs` around lines 59 - 61, Remove the module-level #[allow(dead_code, unused_imports)] from runtime_surface in lib.rs, then apply narrower allowances only to the specific fixture-facing items that produce compiler warnings, or gate those exports with the existing fixture/test configuration. Preserve warnings for genuinely unused code within runtime_surface.
74-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the privacy doctests fail only on the intended access.
Each
compile_failblock also passes if the referenced path or item disappears. Split the existing blocks into two statements: one that compiles only if the type/method/field exists, and one that exercises the private access. ForAuthorityFingerprint, the runtime exposes.operation_id()but the struct field remains private; bind the type with a public method first, then assert the field access cannot compile.🤖 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/orca-runtime/src/lib.rs` around lines 74 - 89, Update the privacy doctests in lib.rs so each compile_fail block first references the target type, method, or field in a compiling statement, then separately tests only the intended private access. For AuthorityFingerprint, use the public operation_id() method to establish the type exists before asserting that direct struct-field access fails, and apply the same existence-versus-privacy separation to the other doctests.crates/orca-runtime/src/runtime_actor/capability.rs (1)
402-429: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd backoff and a bound to the commit retry.
On a failed commit,
resolve_commit_effectalways schedules the next attempt 100 ms later and re-inserts the transition. The delay never grows, and no attempt counter exists. If the commit keeps failing, the actor retries the same transition forever every 100 ms, and the waiter never receives a reply. Store an attempt count inPendingSurfaceCapabilityTransition, apply exponential backoff with a ceiling, and fail the waiter after a maximum number of attempts.🤖 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/orca-runtime/src/runtime_actor/capability.rs` around lines 402 - 429, Update resolve_commit_effect and PendingSurfaceCapabilityTransition to track retry attempts, increase the retry delay exponentially up to a defined maximum, and stop retrying after the maximum attempt count. When the limit is reached, complete the transition with an appropriate failure reply instead of reinserting it; preserve the existing successful-commit path.crates/orca-runtime/src/runtime_actor/background.rs (1)
14-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid formatting
usize::MAXin capacity errors.
DuplicateTaskreturnsusize::MAXfromcapacity(), but theEnsureCapacitycaller formats this into an error message asruntime host background task capacity exhausted (18446744073709551615). For this variant, return no capacity data instead, e.g.Option<usize>or expose capacity only onCapacityExceeded.🤖 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/orca-runtime/src/runtime_actor/background.rs` around lines 14 - 21, Update BackgroundAdmissionError::capacity to avoid returning usize::MAX for DuplicateTask; make capacity data optional or expose it only for CapacityExceeded, and adjust the EnsureCapacity caller to omit the numeric capacity when none is available while preserving the existing exhausted-capacity message for CapacityExceeded.crates/orca-tools/src/web_search.rs (2)
256-298: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the result formatting shared with
execute_or_cancel.Lines 277-292 duplicate the numbering and formatting block in
execute_or_cancel(lines 82-96). The test adapter can drift from the production output format without any test failing. Extract one helper and call it from both paths.♻️ Proposed shared helper
fn format_results(results: Vec<SearchResult>) -> String { results .into_iter() .enumerate() .map(|(index, result)| { format!( "{}. {}\n{}\n{}", index + 1, result.title, result.description, result.url ) }) .collect::<Vec<_>>() .join("\n\n") }🤖 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/orca-tools/src/web_search.rs` around lines 256 - 298, Extract the duplicated result-numbering and text assembly from execute_or_cancel and execute_exa_at_or_cancel into a shared format_results helper accepting the search results collection. Replace both inline formatting blocks with calls to this helper, while preserving each path’s existing truncation and ToolResult handling.
233-238: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCache the shared
reqwest::Client.Both Brave and Exa calls build a new client via
search_client()on every search. Reuse one constant client withOnceLockand clone it, sincereqwest::Clientis designed to be created once and keeps connection pooling/TLS configuration across requests.🤖 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/orca-tools/src/web_search.rs` around lines 233 - 238, Update search_client() to initialize a single reqwest::Client through OnceLock, preserving the existing timeout and SearchError mapping during initialization, then return a clone of the cached client for each caller. Ensure both Brave and Exa search paths continue using search_client() while reusing the shared connection pool.crates/orca-tui/src/app.rs (3)
8223-8234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable rename fallback.
Line 8213 already returns early when
threadisNone. Theunwrap_or_elsebranch at lines 8229-8233 that produces "current conversation surface is unavailable" cannot run. Bind the thread once and drop the fallback.♻️ Proposed simplification
- let rename_result = thread - .as_ref() - .map(|runtime_thread| { - TuiSurfaceActions::new(runtime_thread.typed_surface()) - .rename_current_session(&session_id, &title) - }) - .unwrap_or_else(|| { - Err(std::io::Error::other( - "current conversation surface is unavailable", - )) - }); - match rename_result { + let runtime_thread = thread.as_ref().expect("session_id implies a live thread"); + match TuiSurfaceActions::new(runtime_thread.typed_surface()) + .rename_current_session(&session_id, &title) + {🤖 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/orca-tui/src/app.rs` around lines 8223 - 8234, In the rename flow around TuiSurfaceActions::new, rely on the earlier early return that guarantees thread is present: bind or unwrap thread once, invoke typed_surface().rename_current_session directly, and remove the as_ref/map/unwrap_or_else fallback producing “current conversation surface is unavailable.”
111-126: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider not panicking when the relay thread cannot spawn.
spawn_attached_event_sendercalls.expect("spawn TUI attachment relay"). A spawn failure panics the controller thread, and every later session switch depends on this helper.reap_hosted_threadin this same file already handles a spawn failure with an inline fallback, so the file is inconsistent about this failure mode. Returning the error and reporting it throughTuiEvent::OperationRejectedwould keep the session usable.🤖 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/orca-tui/src/app.rs` around lines 111 - 126, Update spawn_attached_event_sender to handle Builder::spawn failure without panicking: return or propagate the error and report it through TuiEvent::OperationRejected, matching the fallback approach used by reap_hosted_thread. Preserve the existing attachment relay behavior when the thread starts successfully.
73-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the three outcomes of
accept_attached_tui_event.The signature returns
Result<Option<TuiEvent>, ()>.Ok(Some(event))means forward,Ok(None)means consumed, andErr(())means rejected as stale. The unit error carries no reason, and the call site at lines 628-633 must collapseOk(None)andErr(())into the same arm. A small enum such asAttachedEventRouting { Forward(TuiEvent), Consumed, Stale }makes the contract explicit.🤖 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/orca-tui/src/app.rs` around lines 73 - 76, Replace the ambiguous Result<Option<TuiEvent>, ()> contract of accept_attached_tui_event with a named routing enum such as AttachedEventRouting, representing Forward(TuiEvent), Consumed, and Stale outcomes. Update the function’s return paths and the call site handling around the attached-event flow so Ok(None) and Err(()) become the single Consumed arm while stale events map to Stale and forwarded events preserve their TuiEvent.crates/orca-tools/src/schema.rs (1)
67-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the policy filter branches.
canonical_tool_definitionsnow decides which tools the model can see. The branches carry distinct rules:Basehides goal tools,Goalshows them,Subagentalways admits MCP and external tools and always hidessubagent, andAllowedadmits only resolved canonical names. The file currently tests only argument normalization. Add cases for each selection branch, including an alias name inToolPolicy::allowedso thecanonical_allowed_namesresolution stays covered.🤖 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/orca-tools/src/schema.rs` around lines 67 - 101, Add focused tests for canonical_tool_definitions covering Base hiding goal tools, Goal including them, Allowed filtering to resolved canonical names, and Subagent admitting MCP/external tools while excluding “subagent”. Include an alias in ToolPolicy::allowed and assert it resolves through canonical_allowed_names; reuse the existing test registry and tool fixtures where possible.crates/orca-tui/src/types.rs (2)
3263-3267: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
reset_message_trackingdiscards the whole transcript render cache on every completed response.
reset_message_trackingclearsmessage_revisionsand callstranscript_render_cache.clear(). Every message then receives a new revision, so the nextpreparerebuilds and re-wraps every message.reconcile_assistant_responseruns on everyAssistantResponseCompletedevent.The reset is the correct fix for the real problem: the preceding filter removes messages from the middle of the list, so a length-based reconcile would misalign revisions with messages. The cost is a full transcript re-render per turn, which grows with transcript length and now also restarts the incremental reflow work added in
crates/orca-tui/src/transcript_view.rs.Consider filtering
messagesandmessage_revisionstogether in the same pass, then callingretainon the render cache with the retention mask. That preserves the surviving revisions and cached lines and keeps the two vectors aligned.🤖 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/orca-tui/src/types.rs` around lines 3263 - 3267, The completed-response reconciliation should preserve render state for retained messages instead of calling reset_message_tracking, which clears message_revisions and transcript_render_cache. Update reconcile_assistant_response to filter messages and message_revisions together in one pass, build the corresponding retention mask, and retain only matching entries in transcript_render_cache while keeping the vectors aligned.
1822-1864: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMake the session reset total instead of a hand-maintained field list.
reset_session_projectionre-lists roughly thirty fields. Nothing ties that list toAppState. A new session-scoped field added toAppStatewill not be reset here, and no test or compiler check reports the omission. That is the same class of stale-state defect this PR removes elsewhere.Group the session-scoped fields into their own struct that derives
Default, then reset with one assignment. The runtime settings that must survive a reset, such asmodel_name,reasoning_effort,approval_mode, and the syntax configuration, stay outside that struct. A destructuringletover the session struct then makes any new field a compile error until it is handled.
active_session_attachmentis also not reset. Confirm that aSessionAttachmentActivatedevent always follows a reset. If it does not, events fenced to the new attachment are rejected byaccept_attached_tui_eventincrates/orca-tui/src/app.rs.🤖 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/orca-tui/src/types.rs` around lines 1822 - 1864, Refactor AppState so all session-scoped fields currently reset by reset_session_projection, including active_session_attachment, are grouped into a Default-derived session projection struct, while persistent runtime settings remain on AppState. Replace the manual reset list with a single session-struct assignment and destructure the old value so adding a new session field requires explicit handling at compile time. Verify SessionAttachmentActivated follows every reset; otherwise ensure the new attachment is initialized or preserved so accept_attached_tui_event does not reject valid new-session events.crates/orca-tui/src/surface_actions.rs (2)
19-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the process-global scope of the rename failure injection.
RENAME_SAVED_SESSION_FAILURESis one static for the whole test binary. Cargo runs tests in parallel threads in that binary. If another test callsrename_saved_sessionwhile an injection is armed, that test consumes the injected failure instead.Add a doc comment that states the caller must hold the shared process lock, and take the lock in every test that arms the injection.
♻️ Proposed documentation
#[cfg(test)] +/// One-shot failure injection for `rename_saved_session`. +/// +/// The counter is process-global. Callers must hold the shared +/// process-environment lock so that a parallel test cannot consume the +/// injected failure. static RENAME_SAVED_SESSION_FAILURES: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0);🤖 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/orca-tui/src/surface_actions.rs` around lines 19 - 27, Document that RENAME_SAVED_SESSION_FAILURES is process-global and callers of inject_rename_saved_session_failure_once must hold the shared process lock. Update every test that arms this injection to acquire that lock before calling the helper and retain it through the affected rename_saved_session operation.
238-262: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: collapse the two goal-resume wrappers.
resume_goal_and_runandresume_goal_and_run_with_starteddiffer only by thestartedcallback. Keep one method and let the other delegate with a no-op closure, or drop the non-started wrapper and let callers pass|| {}.♻️ Proposed refactor
pub(crate) fn resume_goal_and_run( &self, prompt: String, control: &TuiSurfaceTaskControl, event_tx: &mpsc::Sender<TuiEvent>, ) -> io::Result<TuiHostedOperationOutcome> { - crate::surface_client::resume_goal_and_run(&self.thread, prompt, control, event_tx) + self.resume_goal_and_run_with_started(prompt, control, event_tx, || {}) }🤖 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/orca-tui/src/surface_actions.rs` around lines 238 - 262, Collapse the duplicate goal-resume wrappers around `resume_goal_and_run` and `resume_goal_and_run_with_started` into a single API: either have `resume_goal_and_run` delegate with a no-op `started` closure or update callers to use `resume_goal_and_run_with_started` directly. Preserve the existing behavior and callback execution for callers that provide `started`.crates/orca-tui/src/session_picker_actions.rs (1)
393-404: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the staged clipboard payload.
The test proves that no
UserActionis emitted. It does not prove thatCopySessionIdstaged the correct session ID. Add one assertion so a regression that stages the wrong ID fails here.♻️ Proposed test assertion
assert_eq!(state.session_picker_phase, SessionPickerPhase::Browsing); assert!(rx.try_recv().is_err()); + assert_eq!(state.pending_clipboard_copy.as_deref(), Some("two")); }🤖 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/orca-tui/src/session_picker_actions.rs` around lines 393 - 404, Extend the test around the Enter key handling in the session picker Actions phase to assert that the staged clipboard payload contains the expected session ID, “two.” Keep the existing assertion that no UserAction is emitted, and use the existing clipboard state or accessor used by CopySessionId rather than adding new behavior.crates/orca-tui/src/surface_projection.rs (1)
631-640: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: reuse
core_usage_totalshere.The inline
UsageTotalsconstruction repeatscore_usage_totalsat line 1230, including the micros-to-dollars conversion. Reuse the helper so the conversion stays in one place.♻️ Proposed refactor
SurfaceEvent::Usage(usage) => { projected.push(TuiEvent::UsageUpdated { revision: usage.revision.get(), - usage: UsageTotals { - input_tokens: usage.thread_total.input_tokens, - output_tokens: usage.thread_total.output_tokens, - cache_tokens: usage.thread_total.cache_tokens, - estimated_cost_usd: usage.thread_total.estimated_cost_usd_micros as f64 - / 1_000_000.0, - }, + usage: core_usage_totals(&usage.thread_total), }); }🤖 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/orca-tui/src/surface_projection.rs` around lines 631 - 640, In the usage update projection that builds TuiEvent::UsageUpdated, replace the inline UsageTotals construction and micros-to-dollars conversion with the existing core_usage_totals helper used elsewhere in surface_projection.rs. Preserve the same revision and usage values while centralizing the conversion logic.crates/orca-tui/src/transcript_view.rs (5)
292-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the invariant that makes positional reuse safe.
search_index.entriesis indexed by message position, but positions shift whenretainortruncateruns. The cache stays correct only becausesearch_generationcomes from a monotonic counter, so a shifted entry never matches the generation stored at its new position and always triggers a rescan.That invariant is not stated anywhere. A future change that reuses or resets generations would silently return matches from a different message. Add a comment on
CachedSearchEntrythat records the requirement.♻️ Proposed comment
#[derive(Clone, Debug)] +/// Cached per-message matches for the current query. +/// +/// Entries are keyed by message position, which shifts on `retain` and +/// `truncate`. Correctness depends on `search_generation` values being +/// globally unique per rebuild: a shifted entry then never matches the +/// generation at its new position, so it is always rescanned. struct CachedSearchEntry { generation: u64, matches: Vec<TranscriptSearchMatch>, }Also applies to: 537-560
🤖 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/orca-tui/src/transcript_view.rs` around lines 292 - 302, Add a comment on CachedSearchEntry documenting that entries are position-indexed and remain safe after retain or truncate only because search_generation is monotonic; shifted entries must fail the generation check and be rescanned. Explicitly require that generations are never reused or reset.
985-1015: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared visible-range computation.
visible_message_rangerepeats the first half ofviewport(lines 1090 to 1126): the samelive_start,base_height,total_height,max_scroll,absolute_scroll,absolute_end,first_visible, andlast_visiblecomputation.The reflow prioritization is only correct while these two computations agree. If one changes,
prepareprioritizes messages thatviewportdoes not render, and visible messages reflow late. Extract one private helper and call it from both places.🤖 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/orca-tui/src/transcript_view.rs` around lines 985 - 1015, Extract the duplicated visible-range calculation from visible_message_range and viewport into one private helper returning the computed message range. Move the shared live_start, height, scroll, and first/last visible calculations into that helper, then update both callers to use it while preserving their existing rendering and prioritization behavior.
878-891: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOptional: group the
prepare_entryrender parameters.
prepare_entrytakes ten arguments and needs#[allow(clippy::too_many_arguments)].theme,width,syntax_theme_revision,tick,force_expand, andtheme_identityalways travel together and already exist as a group inTranscriptRenderContext. Pass one small struct instead so the allow attribute can be removed.The rest of the function is correct. The completion cleanup reads the anchor before clearing the schedule, so the final frame still reports
adjusted_scroll.Also applies to: 893-983
🤖 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/orca-tui/src/transcript_view.rs` around lines 878 - 891, Group the related render parameters theme, width, syntax_theme_revision, tick, force_expand, and theme_identity into a small struct, reusing TranscriptRenderContext where appropriate, and pass that struct to prepare_entry instead of separate arguments. Update prepare_entry and all call sites, then remove its clippy::too_many_arguments allowance while preserving the existing adjusted_scroll and completion cleanup behavior.
3090-3167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe time-based budget arm has no coverage.
The test sets
with_reflow_entry_budget(32), so it exercises only the entry-count arm. Production calls supply no entry budget and rely on the 5 ms wall-clock arm at line 832. That arm is untested, including thebudgeted_entries > 0progress guarantee.Add a case that omits
with_reflow_entry_budgetand asserts that eachpreparecall rebuilds at least one pending entry and that the reflow still converges. A wall-clock assertion would be flaky, so assert progress and convergence rather than elapsed time.🤖 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/orca-tui/src/transcript_view.rs` around lines 3090 - 3167, Extend transcript_reflow_is_budgeted_and_converges with a separate reflow scenario that omits with_reflow_entry_budget, exercising the default wall-clock budget path in TranscriptRenderCache::prepare. On each prepare iteration, assert last_prepare_visited() is greater than zero while reflow_pending_for_test() remains true, and retain a bounded loop assertion to verify convergence without asserting elapsed time.
755-791: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffA second layout change during a pending reflow recomputes the anchor from a mixed-width layout.
When
layout_changedis true again whilereflow_scheduleis still pending, this code creates a new schedule with every index pending and a new anchor fromself.reflow_anchor(window). At that momentcumulative_heightsholds new-width heights for already-rebuilt entries and old-width heights for the rest. The anchor row derived from that mixture does not describe a single layout, so the preserved scroll position drifts on rapid successive resizes.The view recovers once the reflow finishes, so this is a transient effect. Consider keeping the original anchor when a schedule is already pending, or recording the layout each entry was built with so the anchor is computed from a consistent basis.
Separately,
self.reflow_generationandReflowSchedule::generationalways hold the same value. The only reader is thedebug_assert_eq!at line 783. Removing one of them removes duplicated state.🤖 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/orca-tui/src/transcript_view.rs` around lines 755 - 791, Update the layout-change handling around reflow_schedule so a pending reflow preserves its existing anchor instead of recomputing one from mixed-width cumulative_heights; only compute a new anchor when starting a schedule with no pending reflow. Also remove the redundant reflow generation state and adjust ReflowSchedule and the debug assertion to retain only the generation value that is actually needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a4ebe29-53c4-4c5d-95ab-db68014ab35d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (125)
.github/workflows/release.yml.github/workflows/runtime-contract.yml.github/workflows/windows-ci.ymlCargo.tomlREADME.mdREADME.zh-CN.mdcrates/orca-core/src/mcp_types.rscrates/orca-platform/src/fs/atomic.rscrates/orca-provider/Cargo.tomlcrates/orca-provider/examples/reasoning_replay_realapi.rscrates/orca-provider/examples/update_plan_strict_realapi.rscrates/orca-provider/src/context.rscrates/orca-provider/src/deepseek_http.rscrates/orca-provider/src/lib.rscrates/orca-provider/src/tool_schema.rscrates/orca-runtime/src/acp/agent.rscrates/orca-runtime/src/acp/supervisor.rscrates/orca-runtime/src/agent_common.rscrates/orca-runtime/src/agent_loop.rscrates/orca-runtime/src/child_agent_loop_setup.rscrates/orca-runtime/src/child_agent_tests.rscrates/orca-runtime/src/child_agent_types.rscrates/orca-runtime/src/controller.rscrates/orca-runtime/src/goal_actor.rscrates/orca-runtime/src/goal_store.rscrates/orca-runtime/src/lib.rscrates/orca-runtime/src/lifecycle.rscrates/orca-runtime/src/runtime_actor/background.rscrates/orca-runtime/src/runtime_actor/capability.rscrates/orca-runtime/src/runtime_actor/commit.rscrates/orca-runtime/src/runtime_actor/goal.rscrates/orca-runtime/src/runtime_actor/mod.rscrates/orca-runtime/src/runtime_host.rscrates/orca-runtime/src/runtime_subagent_call.rscrates/orca-runtime/src/runtime_surface/commands.rscrates/orca-runtime/src/runtime_surface/hub.rscrates/orca-runtime/src/runtime_surface/identity.rscrates/orca-runtime/src/runtime_surface/interaction.rscrates/orca-runtime/src/runtime_surface/mod.rscrates/orca-runtime/src/runtime_surface/operation.rscrates/orca-runtime/src/runtime_surface/projection.rscrates/orca-runtime/src/runtime_surface/reducer.rscrates/orca-runtime/src/runtime_surface/store.rscrates/orca-runtime/src/runtime_turn_loop.rscrates/orca-runtime/src/server.rscrates/orca-runtime/src/server/connection_supervisor.rscrates/orca-runtime/src/server/direct_interaction_adapter.rscrates/orca-runtime/src/server/opaque_permission_router.rscrates/orca-runtime/src/server/processors/mcp_elicitation.rscrates/orca-runtime/src/server/processors/permission.rscrates/orca-runtime/src/server/processors/turn.rscrates/orca-runtime/src/server/processors/user_input.rscrates/orca-runtime/src/server/surface_adapter.rscrates/orca-runtime/src/shell_session.rscrates/orca-runtime/src/subagent_async_worker.rscrates/orca-runtime/src/subagent_execution.rscrates/orca-runtime/src/system_prompt.rscrates/orca-runtime/src/tasks.rscrates/orca-runtime/src/thread.rscrates/orca-runtime/src/tool_execution.rscrates/orca-runtime/src/tool_invocation.rscrates/orca-runtime/src/tool_router.rscrates/orca-runtime/src/tool_turn.rscrates/orca-runtime/src/workflow/runner.rscrates/orca-runtime/tests/acp_import_boundary.rscrates/orca-runtime/tests/jsonl_import_boundary.rscrates/orca-runtime/tests/jsonl_surface_routing.rscrates/orca-runtime/tests/runtime_host.rscrates/orca-runtime/tests/runtime_surface_attach.rscrates/orca-runtime/tests/runtime_surface_commit.rscrates/orca-runtime/tests/runtime_surface_domain.rscrates/orca-runtime/tests/runtime_surface_host.rscrates/orca-runtime/tests/runtime_surface_interaction.rscrates/orca-runtime/tests/runtime_surface_manifest.rscrates/orca-runtime/tests/runtime_surface_operation.rscrates/orca-runtime/tests/runtime_surface_reducer.rscrates/orca-runtime/tests/runtime_surface_types.rscrates/orca-tools/Cargo.tomlcrates/orca-tools/src/lib.rscrates/orca-tools/src/registry.rscrates/orca-tools/src/schema.rscrates/orca-tools/src/web_search.rscrates/orca-tui/Cargo.tomlcrates/orca-tui/src/agent_runtime.rscrates/orca-tui/src/app.rscrates/orca-tui/src/lib.rscrates/orca-tui/src/mention_search_manager.rscrates/orca-tui/src/runtime_event_projection.rscrates/orca-tui/src/session_picker_actions.rscrates/orca-tui/src/surface_actions.rscrates/orca-tui/src/surface_boundary_tests.rscrates/orca-tui/src/surface_client.rscrates/orca-tui/src/surface_projection.rscrates/orca-tui/src/transcript_view.rscrates/orca-tui/src/types.rscrates/orca-tui/src/ui.rscrates/orca-tui/src/vim.rscrates/orca-windows-runner/src/main.rscrates/orca-windows-sandbox/src/capabilities.rsdocs/architecture/adr/0005-runtime-host-operation-control-plane.mddocs/release-process.mddocs/releases/v0.3.3.mddocs/reports/2026-08-03-orca-audit-remediation-evidence.mddocs/reports/2026-08-03-rust-source-text-assertion-inventory.mddocs/superpowers/plans/2026-08-03-orca-audit-remediation-v032.mddocs/superpowers/specs/2026-07-21-runtime-owned-typed-surface-private-contract.digest.jsondocs/superpowers/specs/2026-07-21-runtime-owned-typed-surface-private-contract.manifest.jsondocs/superpowers/specs/2026-07-28-native-windows-platform-foundation.manifest.jsondocs/superpowers/specs/2026-08-02-tui-session-lifecycle-commands-design.mddocs/superpowers/specs/2026-08-03-orca-audit-remediation-v032-design.mdnpm/orca/package.jsonscripts/test-validate-runtime-surface-contract.mjsscripts/validate-runtime-surface-contract.mjssite/public/sitemap.xmlsite/src/changelog/Changelog.tsxsite/src/shared.tstests/cli_architecture_contract.rstests/dependency_architecture_contract.rstests/history_contract.rstests/provider_contract.rstests/runtime_lifecycle_contract.rstests/subagent_contract.rstests/thread_store_contract.rstests/workflow_host_contract.rstests/workflow_tool_contract.rs
💤 Files with no reviewable changes (9)
- crates/orca-runtime/tests/jsonl_import_boundary.rs
- crates/orca-tui/Cargo.toml
- crates/orca-runtime/src/server.rs
- crates/orca-tui/src/vim.rs
- crates/orca-runtime/tests/acp_import_boundary.rs
- crates/orca-provider/Cargo.toml
- crates/orca-runtime/tests/jsonl_surface_routing.rs
- crates/orca-runtime/src/shell_session.rs
- crates/orca-core/src/mcp_types.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (1)
crates/orca-runtime/src/runtime_host.rs (1)
4324-4337: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound supervisor worker admission.
The command channel does not bound work after dequeue. Both paths spawn independent blocking work without a permit. The FIFO test shows that a session-store operation can block indefinitely. Repeated requests can accumulate queued Tokio tasks and blocking jobs until the host loses availability.
Use a host-wide bounded work queue or acquire a permit before spawning. Return a busy error when capacity is exhausted.
crates/orca-runtime/src/runtime_host.rs#L4324-L4337: bound session-store workers beforetokio::spawn.crates/orca-runtime/src/runtime_host.rs#L4368-L4388: use the same bound for thread-start preparation.📍 Affects 1 file
crates/orca-runtime/src/runtime_host.rs#L4324-L4337(this comment)crates/orca-runtime/src/runtime_host.rs#L4368-L4388🤖 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/orca-runtime/src/runtime_host.rs` around lines 4324 - 4337, Bound host worker admission in dispatch_host_store at crates/orca-runtime/src/runtime_host.rs:4324-4337 by acquiring the host-wide work permit before spawning Tokio and blocking work, returning a busy io error when capacity is exhausted. Apply the same bound and busy-error behavior to the thread-start preparation path at crates/orca-runtime/src/runtime_host.rs:4368-4388, reusing the shared capacity mechanism rather than creating separate limits.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/reports/2026-08-03-orca-audit-remediation-evidence.md (1)
46-61: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDo not record future verification as completed.
As of August 3, 2026, the document cannot claim that verification completed on August 4, 2026. Mark the pre-version matrix as planned, or replace it with evidence from August 3, 2026. Keep the post-version matrix pending until that complete verification has actually run.
Proposed correction
-Pre-version verification on 2026-08-04 completed every command in Task 23 Step -3. +Pre-version verification is planned for 2026-08-04. Record completion only +after every command in Task 23 Step 3 has run successfully.🤖 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 `@docs/reports/2026-08-03-orca-audit-remediation-evidence.md` around lines 46 - 61, Update the verification status in the report so it does not claim August 4 pre-version or commit 7749b824 post-version checks were completed as of August 3. Mark the pre-version matrix as planned unless supported by August 3 evidence, and keep the post-version matrix pending until the complete verification actually runs.
🧹 Nitpick comments (1)
scripts/test-validate-runtime-surface-contract.mjs (1)
1202-1223: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope workflow checks to each runtime-contract job.
The current checks count checkout/command occurrences in the whole workflow file. In release.yml, other checkout uses with
fetch-depth: 0can satisfyminimumOccurrenceseven if thetestjob is shallow. Split each workflow byjobs:→ job ID →steps:, then require the checkout/command blocks inside the relevant job jobs.🤖 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 `@scripts/test-validate-runtime-surface-contract.mjs` around lines 1202 - 1223, Update the workflow validation loop in scripts/test-validate-runtime-surface-contract.mjs to inspect each runtime-contract job’s steps rather than counting matches across the entire workflow. Parse each workflow from jobs through job IDs and steps, identify the relevant runtime-contract jobs, and require the full-history checkout plus both validation commands within every required job; do not let unrelated jobs satisfy minimumOccurrences.
🤖 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 @.github/workflows/release.yml:
- Around line 22-23: Disable persisted checkout credentials for the checkout
steps at .github/workflows/release.yml lines 22-23,
.github/workflows/runtime-contract.yml lines 38-39, and both
.github/workflows/windows-ci.yml locations at lines 53-54 and 118-119 by setting
persist-credentials to false; retain credentials only for steps that explicitly
require authenticated Git access.
---
Duplicate comments:
In `@docs/reports/2026-08-03-orca-audit-remediation-evidence.md`:
- Around line 46-61: Update the verification status in the report so it does not
claim August 4 pre-version or commit 7749b824 post-version checks were completed
as of August 3. Mark the pre-version matrix as planned unless supported by
August 3 evidence, and keep the post-version matrix pending until the complete
verification actually runs.
---
Nitpick comments:
In `@scripts/test-validate-runtime-surface-contract.mjs`:
- Around line 1202-1223: Update the workflow validation loop in
scripts/test-validate-runtime-surface-contract.mjs to inspect each
runtime-contract job’s steps rather than counting matches across the entire
workflow. Parse each workflow from jobs through job IDs and steps, identify the
relevant runtime-contract jobs, and require the full-history checkout plus both
validation commands within every required job; do not let unrelated jobs satisfy
minimumOccurrences.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a4ebe29-53c4-4c5d-95ab-db68014ab35d
📒 Files selected for processing (7)
.gitattributes.github/workflows/release.yml.github/workflows/runtime-contract.yml.github/workflows/windows-ci.ymldocs/reports/2026-08-03-orca-audit-remediation-evidence.mdscripts/test-validate-runtime-surface-contract.mjsscripts/validate-runtime-surface-contract.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/validate-runtime-surface-contract.mjs
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/orca-runtime/src/goal_actor.rs (1)
1584-1599: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBound
sendand document timed-out mutability.
recv_timeout(...)can returnTimeoutwhile the command has already been sent on a fullACTOR_MAILBOX_CAPACITYchannel or is already running. Surface mutations can replay viastore_commit_id, but legacy commands do not carry that identity, so a retry can apply the same mutation twice. Use a bounded send path here, and add aGoalActorError::Timeoutdoc note that the caller cannot treat the mutation as rolled back.🤖 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/orca-runtime/src/goal_actor.rs` around lines 1584 - 1599, The request method currently performs an unbounded send and does not document that a timeout may occur after mutation starts. Replace the sender call in request with the bounded send mechanism using the request timeout, preserve Closed handling, and update the GoalActorError::Timeout documentation to state that callers must not assume the mutation was rolled back or safely retry it.crates/orca-runtime/src/tool_invocation.rs (1)
329-343: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNo action needed.
normalize_tool_requestrewrites the effective tool name to the registry'srequested_name, so mixinginvocation.requested.namewithinvocation.effective.targetcan display a different tool than the approval target/action describe.🤖 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/orca-runtime/src/tool_invocation.rs` around lines 329 - 343, Update approval_request_for_invocation to use the normalized effective tool name consistently when populating the approval description and tool fields, matching invocation.effective.target and action; preserve the existing approval ID and optional-action behavior.crates/orca-tui/src/app.rs (1)
9649-9660: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftGoal resumption replaces the session without rotating the attachment or clearing notifications.
This path installs a new runtime session by hand instead of calling
install_hosted_session, and it never callsrotate_attached_event_sender. Two consequences follow.First,
session_attachmentandevent_txstay bound to the previous generation. Events still queued from the replaced session passaccept_attached_tui_eventand mutate state for the resumed goal session. That is exactly the staleness that the fencing added in this PR is meant to prevent; every other switch path (NewSession,ForkCurrentSession,ResumeSavedSession,ForkSavedSession) rotates.Second,
_pending_workflow_notificationsis accepted and ignored, so workflow notifications queued by the replaced session survive into the resumed session.install_hosted_sessionclears them.Route this path through
install_hosted_sessionand rotate the relay after it returns, the same wayUserAction::ResumeSavedSessiondoes. The rotation must happen in the controller loop, becausesession_attachmentandevent_txlive there; consider returning a signal from this function or moving the switch into the caller.🤖 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/orca-tui/src/app.rs` around lines 9649 - 9660, The goal-resumption path must use install_hosted_session instead of manually replacing thread, so it clears pending workflow notifications and performs the standard session setup. Update the surrounding goal-resumption function and its caller as needed to return a switch signal, then rotate the relay with rotate_attached_event_sender in the controller loop after installation, matching UserAction::ResumeSavedSession and ensuring session_attachment and event_tx are rebound to the resumed session.
🟡 Minor comments (17)
crates/orca-runtime/tests/runtime_host.rs-3458-3468 (1)
3458-3468: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe fixed sleep and 100 ms budget make this test timing-dependent.
The test sleeps 50 ms to let the goal request reach the blocked SQLite database, then requires
snapshot()to answer within 100 ms. On a loaded CI runner the spawned thread can miss that window even when the actor loop is free. The failure message then blames the actor loop for a scheduling delay.Wait for an observable signal that the goal request is in flight, and give the snapshot a larger budget that is still far below
TEST_TIMEOUT.🤖 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/orca-runtime/tests/runtime_host.rs` around lines 3458 - 3468, Replace the fixed sleep before snapshotting in the runtime test with an observable synchronization signal confirming the goal request is in flight. Increase the snapshot recv timeout to a larger budget that remains well below TEST_TIMEOUT, while preserving the existing assertion and failure behavior in the snapshot flow.crates/orca-runtime/tests/runtime_host.rs-3533-3569 (1)
3533-3569: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe
try_recvdrain can observe an incomplete trace.
try_recvdoes not block.wait_operation_terminalconfirms that the terminal is committed, but subscription delivery is a separate path. If the batch containingcompaction_running,compaction_completed, andterminal_committedhas not arrived yet, the loop exits early and theassert_eq!at line 3562 fails with a short trace.Collect events until the trace reaches the expected length or
TEST_TIMEOUTelapses.💚 Proposed adjustment
let mut trace = Vec::new(); - while let Some(item) = subscription.try_recv() { + let deadline = Instant::now() + TEST_TIMEOUT; + while trace.len() < 3 { + assert!(Instant::now() < deadline, "incomplete surface trace: {trace:?}"); + let Some(item) = subscription.try_recv() else { + std::thread::sleep(Duration::from_millis(5)); + continue; + }; let SurfaceSubscriptionItem::Batch { batch } = item else { continue; };🤖 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/orca-runtime/tests/runtime_host.rs` around lines 3533 - 3569, Replace the one-shot try_recv drain in the subscription trace assertion with a bounded wait that continues collecting matching events until the expected three-entry trace is complete or TEST_TIMEOUT elapses. Preserve the existing event ordering and filtering in the SurfaceSubscriptionItem/SurfaceEvent matching logic, and assert the collected trace after the timeout-aware loop.crates/orca-runtime/tests/runtime_host.rs-2901-2911 (1)
2901-2911: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe in-flight assertion cannot fail.
Only the test creates
release_marker, at line 2912. At line 2901 the file therefore never exists, so!release_marker.exists()holds whatever the background provider does. The assertion message also claims the check happens "while foreground admission is attempted", but the foreground turn starts later at line 2906.Assert the observable background state instead, for example that the task is still
Runningafter the foreground turn is admitted and before the marker is written.💚 Proposed adjustment
let admitted = thread.task_registry().get(&task_id).unwrap(); assert_eq!(admitted.status, TaskStatus::Running); assert!(admitted.is_backgrounded); - assert!( - !release_marker.exists(), - "background provider must remain in-flight while foreground admission is attempted" - ); let foreground = thread .start_turn( HostedTurnRequest::new("concurrent foreground").with_event_observer(observer.clone()), io::sink(), ) .expect("background ownership releases foreground admission"); + assert_eq!( + thread.task_registry().get(&task_id).unwrap().status, + TaskStatus::Running, + "background provider must remain in flight while the foreground turn is admitted" + ); std::fs::write(&release_marker, "release").expect("release background completion");🤖 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/orca-runtime/tests/runtime_host.rs` around lines 2901 - 2911, Replace the vacuous release_marker.exists() assertion in the concurrent admission test with an assertion on the background task’s observable state, verifying it remains Running after the foreground turn is admitted and before the marker is written. Update the assertion message to describe this ordering, while preserving the existing foreground start and marker behavior.crates/orca-runtime/src/runtime_actor/background.rs-172-183 (1)
172-183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck the duplicate before the capacity.
admit_taskcallsensure_capacity(1)first. When the controller is at capacity and the caller re-admits an existingtask_id, the caller receivesCapacityExceededinstead ofDuplicateTask. A retry path that treatsDuplicateTaskas success then sees a spurious capacity failure.The duplicate check consumes no new slot, so evaluate it first.
🐛 Proposed check order
pub(crate) fn admit_task( &mut self, task_id: String, task: Task, ) -> Result<(), BackgroundAdmissionError> { - self.ensure_capacity(1)?; if self.tasks.contains_key(&task_id) { return Err(BackgroundAdmissionError::DuplicateTask { task_id }); } + self.ensure_capacity(1)?; self.tasks.insert(task_id, task); Ok(()) }🤖 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/orca-runtime/src/runtime_actor/background.rs` around lines 172 - 183, Update admit_task to check self.tasks for an existing task_id before calling ensure_capacity(1). Return DuplicateTask immediately for duplicates, while preserving the existing capacity validation and insertion behavior for new tasks.crates/orca-runtime/src/runtime_actor/goal.rs-200-208 (1)
200-208: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
schedule_pause_settlementdrops the settlement without telling the caller.The method ignores the settlement in two cases: a settlement is already pending, or
operation_idis not the active control. In both cases it returns(). The caller cannot detect the loss, so a requested Goal pause can never reach the surface.Return whether the controller accepted the settlement.
🐛 Proposed signal for the caller
pub(crate) fn schedule_pause_settlement( &mut self, operation_id: OperationId, settlement: PauseEvent, - ) { + ) -> bool { if self.pending_pause_settlement.is_none() && self.has_active_control(operation_id) { self.pending_pause_settlement = Some((operation_id, settlement)); + return true; } + false }🤖 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/orca-runtime/src/runtime_actor/goal.rs` around lines 200 - 208, Update `schedule_pause_settlement` to return a boolean indicating whether the settlement was accepted: return true only when no settlement is pending and `has_active_control(operation_id)` succeeds, and false when either condition rejects it. Preserve the existing assignment behavior for accepted settlements and update callers to handle the returned signal.tests/history_contract.rs-9-26 (1)
9-26: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe mutex serializes this helper, but it does not make
set_varsound.
cargo testruns the tests in one binary across several threads.std::env::set_varmutates process-wide state, which is why the call needsunsafe.ORCA_HOME_LOCKonly serializes calls towith_process_orca_home. It does not stop another test thread from reading the environment at the same moment, so the data race remains.The panic handling is correct:
catch_unwindrestores the previous value and keeps the mutex unpoisoned.Make the constraint explicit. Either document that every environment reader in this binary must go through this helper, or move the two process-scoped tests into their own integration test binary so no other thread runs concurrently.
📝 Proposed documentation of the constraint
+/// Serializes process-wide `ORCA_HOME` mutation for tests that call runtime host +/// APIs directly. `cargo test` runs this binary multi-threaded, so every test in +/// this file that depends on `ORCA_HOME` must either pass it per child process +/// through `Command::env` or acquire this helper. fn with_process_orca_home<T>(home: &Path, run: impl FnOnce() -> T) -> T {🤖 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 `@tests/history_contract.rs` around lines 9 - 26, Make the process-wide ORCA_HOME mutation safe by isolating the process-scoped tests in their own integration-test binary, or ensure every environment reader in this test binary uses with_process_orca_home. Preserve the existing catch_unwind restoration and ORCA_HOME_LOCK behavior, and document the chosen constraint explicitly near with_process_orca_home.crates/orca-runtime/src/runtime_actor/capability.rs-610-621 (1)
610-621: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winA waiter/outcome mismatch drops the reply channel silently.
The
_ => Nonearm runs after the code removed the resident call and took the waiter. TheSyncSenderthen drops without a value. The blocking caller observes a channel disconnect instead of a typedio::Error, which hides the real cause.Assert the invariant in debug builds, and return an explicit error in release builds.
🛡️ Proposed explicit failure
- _ => None, + (waiter, _) => { + debug_assert!(false, "capability waiter does not match its settled outcome"); + let error = || { + io::Error::other("capability waiter does not match its settled outcome") + }; + Some(match waiter { + ResidentSurfaceCapabilityWaiter::ReadTextFile(reply) => { + CapabilityReply::ReadTextFile { reply, result: Err(error()) } + } + ResidentSurfaceCapabilityWaiter::WriteTextFile(reply) => { + CapabilityReply::WriteTextFile { reply, result: Err(error()) } + } + ResidentSurfaceCapabilityWaiter::TerminalCreate(reply) => { + CapabilityReply::TerminalCreate { reply, result: Err(error()) } + } + ResidentSurfaceCapabilityWaiter::TerminalObservation(reply) => { + CapabilityReply::TerminalObservation { reply, result: Err(error()) } + } + ResidentSurfaceCapabilityWaiter::TerminalCleanup(reply) => { + CapabilityReply::TerminalCleanup { reply, result: Err(error()) } + } + }) + }🤖 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/orca-runtime/src/runtime_actor/capability.rs` around lines 610 - 621, Update the waiter/outcome match in the capability reply construction so a mismatched pair asserts the invariant in debug builds and returns an explicit typed io::Error reply in release builds instead of None. Preserve the existing successful and Failed TerminalObservation handling, and ensure the taken SyncSender always receives a CapabilityReply.crates/orca-runtime/src/lib.rs-67-89 (1)
67-89: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPin each
compile_faildoctest to an expected Rust error code.
compile_failpasses for any compilation error, so these tests do not prevent accidental changes that break the privacy contract. Add an explicit expected code to each fence.🤖 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/orca-runtime/src/lib.rs` around lines 67 - 89, Update the three compile_fail doctest fences in the crate-level documentation to include the specific expected Rust compiler error code for each privacy violation. Pin the SurfaceCursor import, AuthorityFingerprint Debug requirement, and operation_id field access examples to their intended diagnostics while preserving the existing privacy assertions.crates/orca-runtime/src/controller.rs-616-616 (1)
616-616: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDrop the
Some(String)layer inAgentLoopContext::with_root_task_id.
ThreadTurnContext::root_task_idstoresNoneby default, butAgentLoopContext::with_root_task_idcurrently replacesNonewithSome("".into()).Option<&str>mapsNoneto itself in tool execution;Some("")maps to the string"".🤖 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/orca-runtime/src/controller.rs` at line 616, Update AgentLoopContext::with_root_task_id to preserve an absent root task ID as None instead of converting it to Some("") when called with request.root_task_id(). Ensure ThreadTurnContext::root_task_id remains None by default and only actual IDs become Some values.docs/superpowers/specs/2026-07-21-runtime-owned-typed-surface-private-contract.manifest.json-7085-7085 (1)
7085-7085: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the stale action-count test vector.
The invariant now requires 30 current
UserActionvariants. Thetui.enum_inventoryvector still requires 21 variants at lines 6437-6444. Update that vector to require 30 variants so the manifest has one closed inventory contract.🤖 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 `@docs/superpowers/specs/2026-07-21-runtime-owned-typed-surface-private-contract.manifest.json` at line 7085, Update the tui.enum_inventory test vector to require 30 UserAction variants instead of 21, matching the closed_inventory.current_tui_user_actions invariant and keeping the manifest’s action-count contracts consistent.crates/orca-runtime/src/tasks.rs-586-605 (1)
586-605: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winConsult persisted parent state when creating a child.
create_subagent_with_parentonly checkscancelled_rootsand in-memorytasks.cancelled_rootsis process-local andTaskControl::cancelis not persisted, so a worker process can reattach without the parents’ cancellation tokens. A child created after the root was cancelled can still be inserted asQueuedwith an un-cancelled token; load the persisted parent record and check ancestor statuses recursively before accepting the child.🤖 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/orca-runtime/src/tasks.rs` around lines 586 - 605, Update create_subagent_with_parent to load the persisted parent record and recursively inspect its ancestors before inserting the child, rather than relying only on process-local cancelled_roots and TaskControl::cancel. Treat any ancestor with a terminal or stopping/cancelled status as parent_cancelled, then preserve the existing Stopping status, timestamp, and token cancellation behavior before persisting the child.crates/orca-tui/src/app.rs-1965-1985 (1)
1965-1985: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA missing
/bin/killturns the reaping assertions into no-ops.
mcp_process_is_alivereturnsfalsewhenCommand::status()fails, and a missing or non-executable/bin/killproduces exactly thatErr.wait_for_mcp_process_exitthen exits on its first iteration, so both reaping assertions pass without observing anything. The test would report success even if the runtime leaked every stdio process.Distinguish "the probe failed" from "the process is gone".
🐛 Proposed fix
#[cfg(unix)] fn mcp_process_is_alive(pid: &str) -> bool { - std::process::Command::new("/bin/kill") + let status = std::process::Command::new("kill") .args(["-0", pid]) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) .status() - .is_ok_and(|status| status.success()) + .expect("kill -0 must be available to probe MCP fixture processes"); + status.success() }🤖 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/orca-tui/src/app.rs` around lines 1965 - 1985, Update mcp_process_is_alive to distinguish a failed /bin/kill probe from a successful check showing the process is gone, rather than mapping Command::status errors to false. Make probe failures propagate or otherwise fail the reaping assertion, while preserving false only when the command successfully confirms the process is absent; keep wait_for_mcp_process_exit using that distinction to ensure leaked processes cannot pass.crates/orca-tui/src/app.rs-9128-9140 (1)
9128-9140: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe plan fallback is unreachable when the snapshot has messages.
The whole fallback block is gated on
messages.is_empty(). A resumed session whose typed snapshot already contains messages therefore never consultstranscript.plan, soplanstaysNoneand the restored plan does not render. Theif plan.is_none()check inside the block only helps when the snapshot has no messages at all.Evaluate the two fallbacks independently.
🐛 Proposed fix
- if messages.is_empty() + if (messages.is_empty() || plan.is_none()) && let HistoryMode::Resume(selector) | HistoryMode::Fork(selector) = mode && let Ok(transcript) = RuntimeSurfaceHostHandle::load_saved_session(selector) { - messages = transcript - .messages - .into_iter() - .filter_map(chat_message_from_history) - .collect(); + if messages.is_empty() { + messages = transcript + .messages + .into_iter() + .filter_map(chat_message_from_history) + .collect(); + } if plan.is_none() { plan = transcript.plan; } }🤖 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/orca-tui/src/app.rs` around lines 9128 - 9140, Separate transcript loading from message restoration in the HistoryMode::Resume/Fork path: load the saved session whenever applicable, restore transcript.messages only when the existing messages collection is empty, and apply transcript.plan independently when plan.is_none(). Preserve current behavior for non-empty messages and already-present plans.crates/orca-tools/src/web_search.rs-473-484 (1)
473-484: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid relying on proxy discovery for this test.
reqwest::Client::builder()readsHTTP_PROXY,ALL_PROXY, and similar environment variables by default. If either variable routes the test request through an external proxy, the server bound to127.0.0.1never accepts andlistener.accept()blocks indefinitely. Disable proxy discovery with.no_proxy()on the test client, or add a bounded accept timeout so the test fails fast.🤖 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/orca-tools/src/web_search.rs` around lines 473 - 484, Update the test client setup in web_search_cancellation_preempts_http_timeout to disable reqwest proxy discovery with no_proxy(), ensuring the request always reaches the local TcpListener. Keep the existing cancellation and timeout assertions unchanged.crates/orca-tui/src/types.rs-3107-3114 (1)
3107-3114: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winApply the revision guard consistently for usage snapshots.
TuiEvent::UsageUpdatedaccepts a snapshot only whenrevisionis strictly greater thanusage_revision.apply_surface_projection_stateat line 1728 assignsusageandusage_revisionwith no comparison, so a projection carrying an older revision moves the stored revision backwards. After that, a staleUsageUpdatedthat was previously rejected becomes acceptable.Guard the projection assignment with the same comparison, or add a comment stating that
SurfaceProjectionSyncedis always the newest usage snapshot on this channel.🤖 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/orca-tui/src/types.rs` around lines 3107 - 3114, Update apply_surface_projection_state to apply the usage snapshot only when its revision is strictly greater than the current usage_revision, matching the guard in TuiEvent::UsageUpdated; otherwise preserve both the existing usage and revision so stale projections cannot move the stored revision backward.crates/orca-tui/src/types.rs-1689-1708 (1)
1689-1708: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
get(first)?before reading the tool-call row.
tool_call_message_index(&id)stores the firstToolCall { id, .. }index, but no release-path invariant guarantees that same row remains aToolCallafter later message changes. In a release build,self.messages[first]can panic with a stale cache entry; returnself.messages.get(first)?first.🤖 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/orca-tui/src/types.rs` around lines 1689 - 1708, The receiving_tool_call_message_index method should safely handle a stale index from tool_call_message_index by retrieving the first message through self.messages.get(first)? before checking is_receiving, returning None when the index is out of bounds while preserving the existing search behavior for valid entries.docs/release-process.md-42-62 (1)
42-62: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the pre-release clippy gate and version-sync step between the checklist and the design spec.
The operational checklist and the design spec disagree on the same "minimum pre-release check matrix": the clippy invocation differs (
-j 1versus-- -D warnings), and the version-sync script is present in one but not the other. The PR's own verification summary states that "version synchronization" was checked before this release, so the script exists; the checklist a releaser actually follows should include it.
docs/release-process.md#L42-L62: addnode scripts/release/test-verify-version-sync.mjsto the pre-release command list, and confirm whethercargo clippy --workspace --all-targets --locked -j 1(current text) or-- -D warnings(design doc) is the intended policy; update the surrounding guidance at lines 57-61 to match whichever is chosen.docs/superpowers/specs/2026-08-03-orca-audit-remediation-v032-design.md#L432-L449: ifdocs/release-process.mdis the authoritative operational policy (clippy warnings tracked separately, not gated), update this matrix's clippy line to drop-- -D warningsso the design spec does not contradict the actual release process.🤖 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 `@docs/release-process.md` around lines 42 - 62, Align the pre-release matrices across docs/release-process.md lines 42-62 and docs/superpowers/specs/2026-08-03-orca-audit-remediation-v032-design.md lines 432-449: add node scripts/release/test-verify-version-sync.mjs to the release checklist, retain the non-gating clippy policy by removing -- -D warnings from the design matrix, and ensure the surrounding release guidance consistently states that clippy warnings are tracked separately rather than treated as release failures.
🤖 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/orca-provider/src/deepseek_http.rs`:
- Around line 267-271: DeepSeek requests receive an empty
ProviderConfig.tools_override, so no registry, MCP, or external tools are
advertised. Update the runtime ProviderConfig construction to build the complete
combined tool-definition schema and assign it to tools_override instead of an
empty vector. Preserve the existing deepseek_tools_schema and
cap_tools_for_deepseek processing in the request path.
In `@crates/orca-runtime/src/child_agent_loop_setup.rs`:
- Around line 47-53: Update the tools_override setup in the child-agent loop to
construct AgentToolPolicyContext for request.depth and request.subagent_type
from the parent loop’s tool policy, replacing
AgentToolPolicyContext::unrestricted(). Pass that inherited context to
provider_tool_schema_override while preserving the existing registry and
external-tools arguments.
In `@crates/orca-runtime/src/goal_store.rs`:
- Around line 1179-1181: Make the receipt-digest fence mandatory for
resume/edit-and-run inputs handled by prepare_goal_run_for_surface and
edit_goal_and_prepare_run_for_surface: reject GoalRunInput::Supplied when
expected_receipt_digest is None, and retain the existing digest comparison for
Some values. Keep the optional variant valid only for production supply paths.
In `@crates/orca-runtime/src/runtime_actor/background.rs`:
- Around line 265-292: Update BackgroundOperationController’s
has_pending_completion and pending_completion_operation_ids methods to include
approval_resolutions alongside the existing completion maps. Ensure quiescence
checks and operation ID collection recognize deferred approval resolutions
scheduled through next_retry, while preserving the current sorting and
deduplication behavior.
In `@crates/orca-runtime/src/runtime_actor/capability.rs`:
- Around line 402-429: Update resolve_commit_effect and the pending transition
state to track retry attempts, increase the uncommitted retry delay
exponentially with a defined maximum ceiling, and stop retrying after a bounded
attempt count. When the limit is reached, resolve the associated waiter as
failed and avoid retaining the transition for another retry; preserve the
existing committed-transition path.
In `@crates/orca-runtime/src/runtime_actor/commit.rs`:
- Around line 294-303: The single-occupancy controllers must reject new pending
work instead of overwriting existing values. In
crates/orca-runtime/src/runtime_actor/commit.rs#L294-L303, update
prepare_terminalization to return the rejected Terminalization when the slot is
occupied, and make begin_attempt compare operation_id before taking the stored
value. In crates/orca-runtime/src/runtime_actor/goal.rs#L143-L146, update
set_pending_recovery to return the rejected PendingRecovery when
pending_recovery is already Some, allowing callers to retry or fail the
operation.
In `@crates/orca-runtime/src/runtime_host.rs`:
- Around line 13509-13534: Make Goal run creation atomic with surface admission
in the flow surrounding settle_typed_goal_surface_worker and
prepare_surface_admission: validate and prepare admission before committing and
acknowledging the CreateAndPrepareRun/EditAndPrepareRun mutation, or ensure
every subsequent admission failure writes a compensating recovery mutation. Do
not leave a durable active run when prepare_surface_admission rejects the
operation.
- Around line 4324-4337: The dispatch_host_store path currently launches
unbounded spawn_blocking jobs, bypassing HOST_COMMAND_CAPACITY; add a bounded
store-work controller or semaphore shared by all store operations, acquire
capacity before dispatching work, and reject or defer requests when capacity is
exhausted. Ensure permits remain held through completion and are released on
every success, error, or task-join failure.
- Around line 3247-3263: Bound the retry loop in the shutdown flow around
send_thread_shutdown and ThreadShutdownAck::Retry using a deadline or retry
budget. Continue sleeping and retrying within the bound, then return an
appropriate shutdown-timeout error once it expires while preserving the
runtime’s durable recovery state.
In `@crates/orca-tui/src/app.rs`:
- Around line 8937-8952: Update reap_hosted_thread so failed shutdown retries
use increasing backoff rather than a fixed 10 ms delay, while preserving the
existing retry behavior. When all attempts are exhausted, emit a warning that
includes enough context to diagnose the unreaped session and its failed
shutdown; retain the fallback shutdown handling when spawning the reaper thread
fails.
In `@crates/orca-tui/src/surface_boundary_tests.rs`:
- Around line 98-110: Bind the manifest inventory test to the complete
UserAction variant list instead of validating only the local CURRENT_ACTIONS
constant. Update the assertion around CURRENT_ACTIONS and the manifest’s
closed_inventory.current_tui_user_actions to derive or compare against
UserAction, ensuring ResumeOperation and CancelOperation are included and future
additions or renames cause the test to fail.
In `@crates/orca-tui/src/surface_client.rs`:
- Around line 366-421: Update crates/orca-tui/src/surface_client.rs lines
366-421 in update_session_metadata to return the committed
SessionMetadataRevision and preserve MutationReply::Uncommitted with a distinct
typed stale-precondition outcome rather than formatting it into io::Error.
Update crates/orca-tui/src/surface_actions.rs lines 143-179 in
rename_current_session to use that committed revision for compensation instead
of rereading the snapshot, and report a stale compensation outcome to the user.
In `@crates/orca-tui/src/surface_projection.rs`:
- Around line 167-193: Avoid rebuilding and emitting SurfaceProjectionSynced for
every batch in project_typed_batch. Compare the newly derived
SurfaceProjectionState from_surface_snapshot result with the previously emitted
state and append the sync event only when it differs, or restrict projection to
batches containing terminal, task, workflow, goal, usage, context, or session
changes. Preserve existing WorkflowTasksUpdated handling and use
SurfaceProjectionState’s PartialEq implementation.
In `@crates/orca-tui/src/transcript_view.rs`:
- Around line 825-867: Update the reflow loop’s break condition around
reflow_started, budgeted_entries, and self.reflow_schedule so the 5 ms
elapsed-time budget applies whenever a pending schedule is being drained,
regardless of whether reflow_window is Some. Preserve the explicit
reflow_entry_budget behavior and stop processing once either budget is reached.
In `@crates/orca-tui/src/types.rs`:
- Around line 3263-3267: In reconcile_assistant_response, replace the
reset_message_tracking call after filtering messages with retain_messages so
surviving messages preserve their existing revisions, transcript render cache
entries, and active search identities. Keep the proposed_plan_parser reset and
reasoning-message handling unchanged.
- Around line 2560-2562: Replace the panic in the TuiEvent::Attached branch of
AppState::update with non-fatal handling: retain a debug_assert for development
if useful, but ignore or otherwise safely reduce the event in release builds.
Prefer updating the fencing layer so AppState::update receives an event type
that cannot contain Attached, if that refactor is within scope.
In `@docs/reports/2026-08-03-orca-audit-remediation-evidence.md`:
- Around line 46-61: Remove the future-dated completed verification claims from
docs/reports/2026-08-03-orca-audit-remediation-evidence.md lines 46-61 and leave
the evidence pending until execution. Restore pending pre-version verification
status in docs/superpowers/plans/2026-08-03-orca-audit-remediation-v032.md lines
1119-1140, and restore pending post-version verification status at lines
1158-1163 until the matrix reruns.
---
Outside diff comments:
In `@crates/orca-runtime/src/goal_actor.rs`:
- Around line 1584-1599: The request method currently performs an unbounded send
and does not document that a timeout may occur after mutation starts. Replace
the sender call in request with the bounded send mechanism using the request
timeout, preserve Closed handling, and update the GoalActorError::Timeout
documentation to state that callers must not assume the mutation was rolled back
or safely retry it.
In `@crates/orca-runtime/src/tool_invocation.rs`:
- Around line 329-343: Update approval_request_for_invocation to use the
normalized effective tool name consistently when populating the approval
description and tool fields, matching invocation.effective.target and action;
preserve the existing approval ID and optional-action behavior.
In `@crates/orca-tui/src/app.rs`:
- Around line 9649-9660: The goal-resumption path must use
install_hosted_session instead of manually replacing thread, so it clears
pending workflow notifications and performs the standard session setup. Update
the surrounding goal-resumption function and its caller as needed to return a
switch signal, then rotate the relay with rotate_attached_event_sender in the
controller loop after installation, matching UserAction::ResumeSavedSession and
ensuring session_attachment and event_tx are rebound to the resumed session.
---
Minor comments:
In `@crates/orca-runtime/src/controller.rs`:
- Line 616: Update AgentLoopContext::with_root_task_id to preserve an absent
root task ID as None instead of converting it to Some("") when called with
request.root_task_id(). Ensure ThreadTurnContext::root_task_id remains None by
default and only actual IDs become Some values.
In `@crates/orca-runtime/src/lib.rs`:
- Around line 67-89: Update the three compile_fail doctest fences in the
crate-level documentation to include the specific expected Rust compiler error
code for each privacy violation. Pin the SurfaceCursor import,
AuthorityFingerprint Debug requirement, and operation_id field access examples
to their intended diagnostics while preserving the existing privacy assertions.
In `@crates/orca-runtime/src/runtime_actor/background.rs`:
- Around line 172-183: Update admit_task to check self.tasks for an existing
task_id before calling ensure_capacity(1). Return DuplicateTask immediately for
duplicates, while preserving the existing capacity validation and insertion
behavior for new tasks.
In `@crates/orca-runtime/src/runtime_actor/capability.rs`:
- Around line 610-621: Update the waiter/outcome match in the capability reply
construction so a mismatched pair asserts the invariant in debug builds and
returns an explicit typed io::Error reply in release builds instead of None.
Preserve the existing successful and Failed TerminalObservation handling, and
ensure the taken SyncSender always receives a CapabilityReply.
In `@crates/orca-runtime/src/runtime_actor/goal.rs`:
- Around line 200-208: Update `schedule_pause_settlement` to return a boolean
indicating whether the settlement was accepted: return true only when no
settlement is pending and `has_active_control(operation_id)` succeeds, and false
when either condition rejects it. Preserve the existing assignment behavior for
accepted settlements and update callers to handle the returned signal.
In `@crates/orca-runtime/src/tasks.rs`:
- Around line 586-605: Update create_subagent_with_parent to load the persisted
parent record and recursively inspect its ancestors before inserting the child,
rather than relying only on process-local cancelled_roots and
TaskControl::cancel. Treat any ancestor with a terminal or stopping/cancelled
status as parent_cancelled, then preserve the existing Stopping status,
timestamp, and token cancellation behavior before persisting the child.
In `@crates/orca-runtime/tests/runtime_host.rs`:
- Around line 3458-3468: Replace the fixed sleep before snapshotting in the
runtime test with an observable synchronization signal confirming the goal
request is in flight. Increase the snapshot recv timeout to a larger budget that
remains well below TEST_TIMEOUT, while preserving the existing assertion and
failure behavior in the snapshot flow.
- Around line 3533-3569: Replace the one-shot try_recv drain in the subscription
trace assertion with a bounded wait that continues collecting matching events
until the expected three-entry trace is complete or TEST_TIMEOUT elapses.
Preserve the existing event ordering and filtering in the
SurfaceSubscriptionItem/SurfaceEvent matching logic, and assert the collected
trace after the timeout-aware loop.
- Around line 2901-2911: Replace the vacuous release_marker.exists() assertion
in the concurrent admission test with an assertion on the background task’s
observable state, verifying it remains Running after the foreground turn is
admitted and before the marker is written. Update the assertion message to
describe this ordering, while preserving the existing foreground start and
marker behavior.
In `@crates/orca-tools/src/web_search.rs`:
- Around line 473-484: Update the test client setup in
web_search_cancellation_preempts_http_timeout to disable reqwest proxy discovery
with no_proxy(), ensuring the request always reaches the local TcpListener. Keep
the existing cancellation and timeout assertions unchanged.
In `@crates/orca-tui/src/app.rs`:
- Around line 1965-1985: Update mcp_process_is_alive to distinguish a failed
/bin/kill probe from a successful check showing the process is gone, rather than
mapping Command::status errors to false. Make probe failures propagate or
otherwise fail the reaping assertion, while preserving false only when the
command successfully confirms the process is absent; keep
wait_for_mcp_process_exit using that distinction to ensure leaked processes
cannot pass.
- Around line 9128-9140: Separate transcript loading from message restoration in
the HistoryMode::Resume/Fork path: load the saved session whenever applicable,
restore transcript.messages only when the existing messages collection is empty,
and apply transcript.plan independently when plan.is_none(). Preserve current
behavior for non-empty messages and already-present plans.
In `@crates/orca-tui/src/types.rs`:
- Around line 3107-3114: Update apply_surface_projection_state to apply the
usage snapshot only when its revision is strictly greater than the current
usage_revision, matching the guard in TuiEvent::UsageUpdated; otherwise preserve
both the existing usage and revision so stale projections cannot move the stored
revision backward.
- Around line 1689-1708: The receiving_tool_call_message_index method should
safely handle a stale index from tool_call_message_index by retrieving the first
message through self.messages.get(first)? before checking is_receiving,
returning None when the index is out of bounds while preserving the existing
search behavior for valid entries.
In `@docs/release-process.md`:
- Around line 42-62: Align the pre-release matrices across
docs/release-process.md lines 42-62 and
docs/superpowers/specs/2026-08-03-orca-audit-remediation-v032-design.md lines
432-449: add node scripts/release/test-verify-version-sync.mjs to the release
checklist, retain the non-gating clippy policy by removing -- -D warnings from
the design matrix, and ensure the surrounding release guidance consistently
states that clippy warnings are tracked separately rather than treated as
release failures.
In
`@docs/superpowers/specs/2026-07-21-runtime-owned-typed-surface-private-contract.manifest.json`:
- Line 7085: Update the tui.enum_inventory test vector to require 30 UserAction
variants instead of 21, matching the closed_inventory.current_tui_user_actions
invariant and keeping the manifest’s action-count contracts consistent.
In `@tests/history_contract.rs`:
- Around line 9-26: Make the process-wide ORCA_HOME mutation safe by isolating
the process-scoped tests in their own integration-test binary, or ensure every
environment reader in this test binary uses with_process_orca_home. Preserve the
existing catch_unwind restoration and ORCA_HOME_LOCK behavior, and document the
chosen constraint explicitly near with_process_orca_home.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 24-26: Align the actions/setup-node major version used in the
release workflow with the corresponding Node 22 validation steps in
runtime-contract.yml and windows-ci.yml. Update the setup-node reference
consistently across all three workflows so contract validation uses the same
action version everywhere.
In @.github/workflows/runtime-contract.yml:
- Around line 33-35: Add a workflow-level or validate-job concurrency
configuration in runtime-contract.yml using a pull-request-specific group key
and enable cancel-in-progress, so newer runs cancel superseded validate jobs
while preserving the existing validation gate.
- Around line 37-39: Disable credential persistence on all three checkout steps:
add persist-credentials: false alongside fetch-depth in
.github/workflows/runtime-contract.yml lines 37-39,
.github/workflows/windows-ci.yml lines 52-54 for native-x64, and
.github/workflows/windows-ci.yml lines 117-119 for native-arm64.
In `@crates/orca-provider/src/tool_schema.rs`:
- Around line 64-90: Update require_all_properties to set additionalProperties
to false for every object schema it normalizes, including nested objects, while
preserving recursive traversal. Handle items whether it is a single schema or an
array of schemas, recursively applying require_all_properties to each tuple item
instead of returning without normalization.
- Around line 113-137: Extend the tests around
deepseek_strict_tools_schema_for_endpoint and require_all_properties to cover a
non-beta base_url returning None and definitions with no strict_capable entry
returning None. Add a recursive schema fixture containing nested objects under
properties, items, and anyOf, then assert required includes every property at
each nested level.
In `@crates/orca-runtime/src/goal_actor.rs`:
- Around line 839-857: Remove the unused test-only GoalActor::delay_for_test
helper, since the test sends GoalActorCommand::DelayForTest directly through
handle.sender. Keep the existing direct command path and related behavior
unchanged.
In `@crates/orca-runtime/src/lib.rs`:
- Around line 59-61: Remove the crate-wide #[allow(dead_code, unused_imports)]
from the private runtime_surface module so compiler warnings expose unused
surface types and removed re-exports. Narrow any necessary allowances to the
specific fixture-only items, or apply them only under the test configuration,
while preserving external fixture coverage.
In `@crates/orca-runtime/src/runtime_actor/background.rs`:
- Around line 14-21: Update BackgroundAdmissionError::capacity to return
Option<usize>, yielding Some(capacity) for CapacityExceeded and None for
DuplicateTask. Adjust every caller of capacity to handle the absent value and
choose the appropriate duplicate-task presentation instead of reporting an
invented limit.
In `@crates/orca-runtime/src/runtime_actor/commit.rs`:
- Around line 67-72: The test-only SurfaceCommitResolution::Aborted variant is
indistinguishable from Committed because resolve_attempt returns the effect for
both. Remove Aborted and update the affected test to use Committed, or change
resolve_attempt so Aborted drops the effect and is not reported as committed,
ensuring the test verifies distinct behavior.
In `@crates/orca-runtime/src/subagent_execution.rs`:
- Around line 458-477: Refactor execute_subagent_tool to accept a context struct
grouping its related runtime parameters, following the existing
RuntimeSubagentBatchToolTurnRuntime pattern. Include root_task_id and
workflow_ipc in the grouped context so callers cannot transpose adjacent
optional arguments, then update all production and test call sites to construct
and pass the context while preserving behavior.
In `@crates/orca-runtime/src/tool_invocation.rs`:
- Around line 207-265: Refactor prepare_tool_invocation to delegate its
normalization and action-derivation logic to
prepare_tool_invocation_with_external, passing config.subagents.max_depth and
config.external_tools. Preserve the existing requested/effective/action behavior
while leaving prepare_tool_invocation_with_external as the single
implementation.
In `@crates/orca-runtime/tests/runtime_surface_commit.rs`:
- Around line 602-619: Update FakeLedger::probe_commit and its receipt state to
track receipts by SurfaceCommitId, including batches left prepared when
append_complete_batch encounters Fault::Partial. Return CommitProbe::Prepared
for matching prepared entries and CommitProbe::Present for committed entries,
while preserving Conflict and Absent behavior for mismatched or unknown commits.
In `@crates/orca-runtime/tests/runtime_surface_reducer.rs`:
- Line 1630: Update the snapshot comparison in the test around
replayed.snapshot() and state.snapshot() to use assert_eq! so failures display
both differing snapshots; apply this only if the snapshot type implements Debug,
otherwise retain the existing assert! comparison.
In `@crates/orca-runtime/tests/runtime_surface_types.rs`:
- Around line 2031-2032: Update both occurrences of SurfaceIncarnation in the
affected test to use the unqualified imported name from
orca_runtime::surface::*, while leaving the try_from_bytes calls and surrounding
behavior unchanged.
In `@crates/orca-tools/src/schema.rs`:
- Around line 86-92: Update the ToolSelection::Subagent(_) branch to apply the
allowed filter to ToolName::Mcp(_) and ToolName::External(_) tools instead of
admitting them unconditionally. Preserve the existing subagent-name handling and
ensure the allowed_tools list controls all tool kinds consistently.
In `@crates/orca-tools/src/web_search.rs`:
- Around line 276-292: Extract the shared result-formatting and truncation logic
from execute_or_cancel and execute_exa_at_or_cancel into a single helper. Have
both execution paths call that helper, preserving the existing numbered
title/description/URL output and truncate_output behavior so tests validate
production formatting.
In `@crates/orca-tui/src/app.rs`:
- Around line 1073-1100: Add a harness-level test that bypasses
spawn_unwrapped_tui_test_event_sender, preserves AttachedTuiEvent envelopes,
performs a session switch, and asserts events emitted from the previous session
retain its attachment id. Use the existing harness setup and session-switch
flow, ensuring the test exercises rotate_attached_event_sender and fails if the
prior session’s events carry a new or missing id.
In `@crates/orca-tui/src/mention_search_manager.rs`:
- Around line 129-156: Add replacement tests for consume_catalog_dirty that
inject catalog results through catalog_result_tx without starting a server.
Verify a current-generation result replaces the catalog, advances the active
token/session generation, and sets the search phase appropriately; add a
mismatched-generation case asserting the result is rejected and state remains
unchanged.
In `@crates/orca-tui/src/surface_actions.rs`:
- Around line 19-27: Scope RENAME_SAVED_SESSION_FAILURES to the test that arms
it so concurrent tests cannot consume another test’s injected failure. Prefer
guarding every test using inject_rename_saved_session_failure_once and
rename_saved_session with crate::test_support::lock_process_env, including the
additional usage around the referenced lines; alternatively replace the
process-global counter with thread-local state while preserving one-shot failure
behavior.
In `@crates/orca-tui/src/surface_client.rs`:
- Around line 1114-1136: Refactor resume_goal_and_run to delegate to
resume_goal_and_run_with_started, reusing its existing goal-fence and
supplied_goal_input logic instead of constructing the ResumeAndRun action
locally. Preserve the current controller, event sender, prompt, and outcome
behavior while eliminating the duplicate closure.
- Around line 2633-2636: Update the process-environment guard used by the
affected tests in surface_client.rs so it owns the ORCA_HOME override and
restores the previous value in Drop, even when a test panics; pass the desired
ORCA_HOME value through lock_process_env instead of setting and restoring it in
each test. Also ensure lock_process_env recovers from a poisoned mutex so later
tests can acquire it, and apply this pattern to every test in the file using the
guard.
In `@crates/orca-tui/src/transcript_view.rs`:
- Around line 985-1015: Extract the shared scroll clamping and cumulative-height
partitioning logic from visible_message_range and viewport into one private
helper returning (first_visible, last_visible) for first_retained_message,
requested_scroll, and visible_height. Update both callers to use this helper,
preserving their existing viewport and reflow anchoring behavior while
eliminating duplicated geometry calculations.
- Around line 537-560: Add a concise comment where
CachedMessage::search_generation is defined, documenting that generations are
globally unique and are required to detect position shifts in the position-keyed
search index. In the existing assert-style test coverage, add a debug_assert
that all populated cached entries have distinct search_generation values,
without changing the search behavior.
- Around line 1077-1082: Update retain_messages and its retain flow to remap
reflow_schedule.pending_indices and anchor through the existing retained_mask
instead of calling reset_reflow_after_structural_change. Preserve surviving
pending entries in their new compacted positions and translate the anchor
message index similarly, retaining it when its message survives; only discard
entries or the anchor when their messages are removed.
In `@crates/orca-tui/src/types.rs`:
- Line 1897: Update replace_message around rebuild_tool_call_indices to compare
the message’s previous and current tool-call identity, matching the existing
logic in mutate_message, and rebuild the index only when that identity changes.
Preserve replacement behavior while avoiding the full scan for unchanged tool
calls.
- Line 1660: Restrict assert_tool_call_index_consistent to points where the
tool-call index can change: invoke it after rebuild_tool_call_indices and when
push_message inserts a new indexed entry. Remove it from
reconcile_message_tracking and avoid triggering it through mutate_message,
touch_message, replace_message, truncate_messages, or retain_messages.
- Around line 1724-1735: Update apply_surface_projection_state to move
projection.session_id, title, usage, current_goal, foreground_operation_id, and
workflow_tasks directly into the corresponding assignments, avoiding
unconditional clones. Preserve assert_surface_projection_consistent by creating
only the required debug/test-only clone or snapshot under #[cfg(any(test,
debug_assertions))] before moving those fields, while keeping release builds
free of this cloning work.
In `@tests/dependency_architecture_contract.rs`:
- Around line 59-64: Add an explicit failure message to the exact root
dependency assert_eq! in the normal_dependencies assertion, stating that the
root dependency set must remain intentionally minimal or thin. Keep the expected
dependency set and assertion behavior unchanged.
In `@tests/history_contract.rs`:
- Around line 193-277: Update exec_fork_creates_child_with_parent_metadata to
assert exactly one session document exists before forking, capture the source
document contents, and assert they remain byte-for-byte unchanged afterward.
Remove the overlapping session_fork_copies_history_and_keeps_source_durable test
while preserving its distinct source-durability assertion in the existing test.
In `@tests/provider_contract.rs`:
- Around line 46-53: Update the read_file assertion in the provider contract
test to verify only that read_file.description is non-empty, removing the exact
prose substring check while preserving the existing input_schema and
strict_capable assertions.
In `@tests/subagent_contract.rs`:
- Around line 28-38: Extend the assertions in the request_stop_tree test to
verify that the root task foreground transitions to TaskStatus::Stopping and
that stopped does not contain detached.id. Keep the existing owned-subagent and
detached-task status assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (3)
crates/orca-runtime/src/runtime_host.rs (3)
3247-3263: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound shutdown retries.
At Line 3247,
shutdownretries forever after everyThreadShutdownAck::Retry. A persistent ledger, capability, or Goal Store failure can therefore block the caller permanently.Use a deadline or retry budget. Return a shutdown-timeout error after the budget expires while the runtime retains its durable recovery state.
🤖 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/orca-runtime/src/runtime_host.rs` around lines 3247 - 3263, Bound the retry loop in the shutdown flow around send_thread_shutdown and ThreadShutdownAck::Retry using a deadline or retry budget. Continue sleeping and retrying within the bound, then return an appropriate shutdown-timeout error once it expires while preserving the runtime’s durable recovery state.
4324-4337: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Limit concurrent blocking store operations.
At Line 4331, each JSONL request creates a detached task and a
spawn_blockingjob. The host supervisor immediately accepts more commands, soHOST_COMMAND_CAPACITYdoes not bound already-dispatched store work. A blocked session read, including the FIFO case covered by the new test, can accumulate an unbounded backlog during request bursts.Add a bounded store-work controller or semaphore. Reject or defer requests when the limit is reached.
🤖 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/orca-runtime/src/runtime_host.rs` around lines 4324 - 4337, The dispatch_host_store path currently launches unbounded spawn_blocking jobs, bypassing HOST_COMMAND_CAPACITY; add a bounded store-work controller or semaphore shared by all store operations, acquire capacity before dispatching work, and reject or defer requests when capacity is exhausted. Ensure permits remain held through completion and are released on every success, error, or task-join failure.
13509-13534: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep Goal run creation atomic with surface admission.
At Line 13509,
settle_typed_goal_surface_workercommits and acknowledges the Goal Store mutation beforeprepare_surface_admissionruns.prepare_surface_admissioncan reject the operation when terminal or admission work is pending. The method then returns an error without rolling back or recovering the durableCreateAndPrepareRunorEditAndPrepareRunmutation.This can leave a Goal with an active run that has no admitted surface operation. Prepare and validate admission before committing the Goal run, or write a compensating recovery mutation on every later admission failure.
🤖 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/orca-runtime/src/runtime_host.rs` around lines 13509 - 13534, Make Goal run creation atomic with surface admission in the flow surrounding settle_typed_goal_surface_worker and prepare_surface_admission: validate and prepare admission before committing and acknowledging the CreateAndPrepareRun/EditAndPrepareRun mutation, or ensure every subsequent admission failure writes a compensating recovery mutation. Do not leave a durable active run when prepare_surface_admission rejects the operation.
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/orca-runtime/src/tasks.rs (1)
1744-1786: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHold the session lock across the migration read and write.
write_current_taskcorrectly locks, then reads, merges, and writes.load_session_recordsdoes not: it callsread_session_recordswithout a lock, and then callswrite_session_records, which acquires the lock only for the write. A concurrentwrite_current_taskfrom an attached worker can commit a new or updated record between the read and the write. The whole-session write then discards that record.The same pattern applies to
load_record_by_idat Lines 1767-1776 and toopen_persistentat Lines 295-306, which both rewrite a full session after an unlocked read.🔒️ Proposed fix: lock around read and write
fn load_session_records(&self, session_id: &str) -> io::Result<HashMap<String, TaskRecord>> { - let (records, changed) = self.read_session_records(session_id)?; - if changed { - self.write_session_records(session_id, &records)?; - } - Ok(records) + let _session_lock = ExclusiveFileLock::acquire(&self.session_lock_path(session_id)) + .map_err(io::Error::other)?; + let (records, changed) = self.read_session_records(session_id)?; + if changed { + self.write_session_records_unlocked(session_id, &records)?; + } + Ok(records) }
write_current_taskalready holds the session lock when it callsread_session_records, so it must keep calling the unlocked read and write helpers. Confirm thatExclusiveFileLockis not reentrant before you reuse the locked variant on that path.🤖 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/orca-runtime/src/tasks.rs` around lines 1744 - 1786, Hold the session lock across each full-session read/modify/write migration in load_record_by_id, load_session_records, and open_persistent to prevent concurrent writes from being overwritten. Acquire the session-specific ExclusiveFileLock before read_session_records and use the unlocked write helper while the guard remains held; preserve write_current_task’s existing lock/unlocked-helper pairing because ExclusiveFileLock is not reentrant.crates/orca-runtime/src/tool_invocation.rs (1)
329-341: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse the effective request source for approval record content.
prepare_tool_invocationresolves aliases and extractseffective.target;normalize_tool_requestcan change bothnameandtarget.approval_request_for_invocationcopiesid/name/descriptionfromrequestedbuttargetfromeffective, so aliases show the requested tool name next to the effective target. Useinvocation.effectivefor the approval description,tool, andtarget; keep the approval staging id fromrequested.idonly if preapproval matching depends on it.🤖 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/orca-runtime/src/tool_invocation.rs` around lines 329 - 341, Update approval_request_for_invocation to consistently use invocation.effective for the approval description, tool name, and target, so aliases display the resolved request details. Preserve invocation.requested.id for the staging id only where required for preapproval matching.crates/orca-tui/src/ui.rs (1)
517-543: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEnsure confirmation selection returns to the current action list.
ConfirmArchiverestoresselected: 3andConfirmDeleterestoresselected: 4. Those values map toRenameandCopy session IDfor current sessions, soEscfrom confirmation can resume a session action flow at the wrong index. Re-calculate the return selection fromavailable_session_actions(...)before re-enteringSessionPickerPhase::Actions.🤖 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/orca-tui/src/ui.rs` around lines 517 - 543, Update the ConfirmArchive and ConfirmDelete confirmation-return paths to derive selected from available_session_actions(...) for the current session before restoring SessionPickerPhase::Actions, rather than using hard-coded indices 3 or 4. Preserve the appropriate current action selection so Esc resumes the same session action flow.
🟡 Minor comments (12)
crates/orca-runtime/src/runtime_host.rs-37862-37920 (1)
37862-37920: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore
ORCA_HOMEduring unwinding.An
expector assertion before Lines 37917-37920 skips restoration. The temporary directory then drops whileORCA_HOMEstill points to it. Later tests can use an invalid test home.Use an RAII environment guard with the existing test environment lock.
🤖 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/orca-runtime/src/runtime_host.rs` around lines 37862 - 37920, Add an RAII guard around the ORCA_HOME mutation in this test, using the existing lock_test_env synchronization, so the previous value is restored automatically during both normal completion and unwinding. Remove the manual restoration block after host shutdown and ensure the guard handles both an existing ORCA_HOME value and an unset variable.crates/orca-runtime/src/runtime_actor/goal.rs-143-146 (1)
143-146: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
set_pending_recoveryoverwrites the slot in release builds.
debug_assert!(self.pending_recovery.is_none())is removed in release builds, so a second call replaces the stored recovery and the first one is lost. Reject the second call, or return the displaced value.🤖 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/orca-runtime/src/runtime_actor/goal.rs` around lines 143 - 146, Update set_pending_recovery to prevent silently replacing an existing pending_recovery in release builds: either reject a second assignment or return the displaced PendingRecovery value, while preserving the existing behavior for an empty slot.crates/orca-runtime/src/runtime_actor/goal.rs-156-208 (1)
156-208: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
bind_activeandschedule_pause_settlementcan discard another operation's state without signalling it.Two problems appear in this pair of methods:
bind_activesetsself.active_control = NonewhencontrolisNone, and it always clearspending_pause_settlement. It does not compareoperation_idwith the current binding. A call for operation B therefore drops the active control and the pending pause settlement of operation A.schedule_pause_settlementreturns unit. When a settlement already exists, or when the operation does not hold the active control, the settlement is dropped and the caller cannot detect the drop.Compare the operation id before clearing state in
bind_active. Returnboolfromschedule_pause_settlementso the caller can handle a rejected settlement.🤖 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/orca-runtime/src/runtime_actor/goal.rs` around lines 156 - 208, Update bind_active to compare operation_id with the currently bound operation before replacing active_control or clearing pending_pause_settlement, preserving another operation’s state when the binding does not match. Change schedule_pause_settlement to return bool, returning true only when the settlement is stored and false when one is already pending or the operation lacks active control, then update its callers to handle the rejection.crates/orca-runtime/src/runtime_actor/capability.rs-610-622 (1)
610-622: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winA mismatched waiter and outcome pair loses the reply.
The
_ => Nonearm covers combinations such as aTerminalObservationwaiter with aTerminalCleanupCompletedoutcome. At that pointremovealready took the resident call andtakealready took the waiter, so theSyncSenderis dropped. The blocked caller then observes a channel disconnect instead of a typedio::Error, which hides the cause.Reply with an explicit
io::ErrorKind::InvalidDataerror for the mismatch instead of returningNone.🤖 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/orca-runtime/src/runtime_actor/capability.rs` around lines 610 - 622, The mismatch arm in the capability waiter/outcome matching logic should return a RuntimeActorEffect::ReplyCapability containing the extracted waiter and a typed io::Error with ErrorKind::InvalidData, rather than returning None. Update the match around ResidentSurfaceCapabilityWaiter and PendingSurfaceCapabilityWaiterOutcome while preserving the existing successful and Failed handling.crates/orca-runtime/src/runtime_actor/background.rs-172-183 (1)
172-183: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
admit_taskdrops the rejected task without cancelling it.On
DuplicateTaskthe function takes ownership oftaskand returns an error that carries onlytask_id. The task value is dropped. IfTaskowns a spawned worker, the caller loses the only handle that can callManagedBackgroundTask::cancel, and the worker keeps running untracked. Return the rejected task, or cancel it before returning.🛡️ Proposed fix
pub(crate) fn admit_task( &mut self, task_id: String, task: Task, ) -> Result<(), BackgroundAdmissionError> { self.ensure_capacity(1)?; if self.tasks.contains_key(&task_id) { + task.cancel(); return Err(BackgroundAdmissionError::DuplicateTask { task_id }); } self.tasks.insert(task_id, task); Ok(()) }🤖 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/orca-runtime/src/runtime_actor/background.rs` around lines 172 - 183, Update admit_task to explicitly cancel the incoming task before returning DuplicateTask, using Task’s existing ManagedBackgroundTask cancellation path; preserve the capacity check and successful insertion behavior, and ensure the duplicate error still reports task_id.docs/superpowers/plans/2026-08-03-orca-audit-remediation-v032.md-783-785 (1)
783-785: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the Markdown tokenization.
use super::*is parsed as emphasis markup. Wrap the identifier in inline code to resolve MD037.Proposed fix
-List exports explicitly in mod.rs. Replace production use super::* imports with exact sibling imports. +List exports explicitly in mod.rs. Replace production `use super::*` imports with exact sibling imports.🤖 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 `@docs/superpowers/plans/2026-08-03-orca-audit-remediation-v032.md` around lines 783 - 785, Update the “Step 3: Replace globs” Markdown text so the Rust import identifier use super::* is wrapped in inline-code formatting, preserving the existing wording and checklist state.Source: Linters/SAST tools
docs/superpowers/specs/2026-07-21-runtime-owned-typed-surface-private-contract.manifest.json-7083-7085 (1)
7083-7085: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep the action-count contract consistent.
Line 7085 requires 30 current actions.
tui.enum_inventorystill requires “all 21 current variants exactly once” at Line 6441. Update that test vector to 30, or the manifest has conflicting baseline requirements.Proposed fix
- "all 21 current variants exactly once", + "all 30 current variants exactly once",🤖 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 `@docs/superpowers/specs/2026-07-21-runtime-owned-typed-surface-private-contract.manifest.json` around lines 7083 - 7085, Update the tui.enum_inventory action-count invariant to require 30 current variants, matching the phase_0a_manifest_invariants requirement for closed_inventory.current_tui_user_actions. Preserve the existing uniqueness and exact-variant validation while removing the conflicting count of 21.crates/orca-tui/src/app.rs-9128-9140 (1)
9128-9140: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe persisted-transcript fallback hides load failures.
If the typed snapshot is empty, this block loads the saved session and projects its messages.
if let Ok(transcript) = ...discards the error, so a corrupt or missing transcript produces an empty transcript view with the label "Resumed saved conversation." and no explanation for the user. Report the failure throughTuiEvent::Erroror return it, so the caller can surface it.🤖 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/orca-tui/src/app.rs` around lines 9128 - 9140, Update the persisted-transcript fallback around RuntimeSurfaceHostHandle::load_saved_session so load errors are not discarded by if let Ok. Propagate the failure or report it through TuiEvent::Error, while preserving the existing message and plan projection on successful loads.tests/workflow_host_contract.rs-12-13 (1)
12-13: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPin
host.mjsto LF in.gitattributesfor the embedded fixture.
include_str!embeds the working-copy bytes, and the current.gitattributesonly preserves LF for the listed JSONL/spec paths. Addcrates/orca-runtime/src/workflow/host.mjs text eol=lfso the protocol fixture does not change on CRLF checkouts.🤖 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 `@tests/workflow_host_contract.rs` around lines 12 - 13, Add the exact `crates/orca-runtime/src/workflow/host.mjs text eol=lf` rule to `.gitattributes`, alongside the existing LF-preserved fixture paths, so the `WORKFLOW_HOST_SCRIPT` include_str! bytes remain LF-normalized on all checkouts.crates/orca-tui/src/app.rs-1927-2093 (1)
1927-2093: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winConfirm the MCP fixture test terminates when the shell fixture is unavailable.
stdio_mcp_serverhard-codes/bin/sh, andmcp_process_is_alivehard-codes/bin/kill./bin/killdoes not exist on every Unix target; on many Linux distributions the binary is/usr/bin/killor a shell builtin only. If/bin/killis absent,status()returnsErr,is_ok_andyieldsfalse, andwait_for_mcp_process_exitreturns immediately, so the reaping assertions pass without verifying anything. ResolvekillthroughPATH, or assert that the command itself is available.🐛 Proposed fix for the liveness probe
- std::process::Command::new("/bin/kill") + std::process::Command::new("kill") .args(["-0", pid])🤖 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/orca-tui/src/app.rs` around lines 1927 - 2093, Update mcp_process_is_alive to resolve the kill executable through PATH instead of hard-coding /bin/kill, and make command-unavailable errors fail the fixture rather than returning false. Preserve wait_for_mcp_process_exit’s polling behavior so it genuinely verifies that the MCP process exits.crates/orca-tui/src/types.rs-1689-1708 (1)
1689-1708: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winIndex
messagesthroughgetso a stale index cannot panic.
receiving_tool_call_message_indexindexesself.messages[first]and slicesself.messages[first + 1..]with a value read fromtool_call_indices. The consistency check that protects this invariant compiles to an empty function in release builds. If any future mutation path forgets to rebuild the index, a release build panics with an out-of-bounds access inside the event reducer instead of degrading to a miss.🛡️ Proposed guard
let first = self.tool_call_message_index(id)?; - if is_receiving(&self.messages[first]) { + let Some(first_message) = self.messages.get(first) else { + debug_assert!(false, "tool call index points past the message list"); + return None; + }; + if is_receiving(first_message) { return Some(first); } - self.messages[first + 1..] + self.messages + .get(first + 1..)? .iter() .rposition(is_receiving) .map(|offset| first + 1 + offset)🤖 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/orca-tui/src/types.rs` around lines 1689 - 1708, Update receiving_tool_call_message_index to access messages through get using the index returned by tool_call_message_index, returning None when the index is stale or out of bounds. Avoid direct messages[first] indexing and ensure the subsequent search starts only from a valid message position, preserving the existing receiving-message lookup behavior.tests/cli_architecture_contract.rs-24-26 (1)
24-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the tautological
--modeassertion.
stdout.contains("--mode")also matches inside"--model", since"--model"starts with the six characters"--mode". The preceding iteration already assertsstdout.contains("--model"), so this line always passes once--modelis present, even if the binary never documents a separate--modeflag. The test does not verify--modeindependently.🔧 Proposed fix using exact token matching
+ let tokens: Vec<&str> = stdout.split_whitespace().collect(); for option in ["--resume", "--fork", "--continue", "--model", "--mode"] { - assert!(stdout.contains(option), "root help is missing {option}"); + assert!( + tokens.iter().any(|token| *token == option), + "root help is missing {option}" + ); }🤖 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 `@tests/cli_architecture_contract.rs` around lines 24 - 26, Replace the substring-based option checks in the root-help assertion loop with exact token matching so --mode cannot be satisfied by --model. Preserve independent verification of every option, including --model and --mode, using the help output’s token boundaries.
🧹 Nitpick comments (27)
crates/orca-runtime/tests/runtime_surface_types.rs (1)
2031-2032: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant fully-qualified path.
orca_runtime::surface::*is already imported at Line 1. Use the unqualifiedSurfaceIncarnationat both sites for consistency with the rest of the file.♻️ Proposed simplification
let ephemeral = CommitClass::Ephemeral { - incarnation: orca_runtime::surface::SurfaceIncarnation::try_from_bytes(uuid_v7_bytes(4)) - .unwrap(), + incarnation: SurfaceIncarnation::try_from_bytes(uuid_v7_bytes(4)).unwrap(), live_revision: LiveRevision::try_new(1).unwrap(), commit_id, };let _cursor = SurfaceCursor { thread_id: SurfaceThreadId::try_from_bytes([6; 16]).unwrap(), - incarnation: orca_runtime::surface::SurfaceIncarnation::try_from_bytes(uuid_v7_bytes(6)) - .unwrap(), + incarnation: SurfaceIncarnation::try_from_bytes(uuid_v7_bytes(6)).unwrap(), next_seq: SequenceNumber::new(0),Also applies to: 2130-2131
🤖 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/orca-runtime/tests/runtime_surface_types.rs` around lines 2031 - 2032, Update both `SurfaceIncarnation::try_from_bytes` call sites in `runtime_surface_types.rs` to use the imported unqualified `SurfaceIncarnation` type instead of the redundant `orca_runtime::surface::` path.crates/orca-runtime/tests/runtime_surface_commit.rs (1)
622-661: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd the partial-append recovery path to this trace test.
The test uses
Fault::Failed, which leavesself.receiptunset.probe_committhen reportsAbsent, so re-appending the same batch is the correct recovery. The risky path isFault::Partial: the append may have landed durably,probe_commitreportsPresent, and the coordinator must adopt the existing receipt instead of appending a second time. The newprobe_commitlogic at lines 602-619 exists to serve that path, but no test drives it.Add a second case that injects
Fault::Partialwith a pre-seeded matching receipt, then assert the ledger records no additionalAppend.🤖 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/orca-runtime/tests/runtime_surface_commit.rs` around lines 622 - 661, The commit_controller_trace_equivalence test currently covers only failed-append retry; add a second partial-append recovery case using Fault::Partial and a pre-seeded receipt matching the prepared batch. Run commit_actor_batch, verify the coordinator adopts the existing receipt and preserves the expected state, and assert the ledger records no additional Append while retaining the existing checkpoint behavior..github/workflows/runtime-contract.yml (1)
33-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a concurrency group and credential hardening for this new workflow.
The job runs on every push and pull request that touches the runtime paths. Two optional improvements:
- Add a
concurrencygroup so superseded runs cancel instead of queueing.- Set
persist-credentials: falseon the checkout. The job only reads the repository and runs validators, so theGITHUB_TOKENdoes not need to stay in.git/config. This also resolves the zizmorartipackedfinding.♻️ Proposed workflow hardening
permissions: contents: read +concurrency: + group: runtime-contract-${{ github.ref }} + cancel-in-progress: true + jobs: validate: runs-on: ubuntu-latest steps: - uses: actions/checkout@v5 with: fetch-depth: 0 + persist-credentials: false🤖 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 @.github/workflows/runtime-contract.yml around lines 33 - 44, Add workflow hardening to the runtime-contract workflow: configure a concurrency group that cancels superseded runs, and set checkout’s persist-credentials option to false in the actions/checkout step. Keep the existing validation job and toolchain setup unchanged.Source: Linters/SAST tools
crates/orca-runtime/tests/runtime_host.rs (1)
2865-2974: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting this test and extracting the event-polling helper.
Two points, both optional:
- The function covers two independent behaviours with two hosts: background completion tracing (lines 2867-2931) and background cancellation on shutdown (lines 2933-2973). Two named tests would show which behaviour regressed and would let them run in parallel.
- Lines 2918-2929 re-implement the deadline-and-sleep loop that
wait_until_task_status(line 3389) already provides for the task registry. Await_until_event(&observer, predicate, message)helper would remove the duplicate loop and give the failure a consistent message.Also add a message to the timing assertion at line 2965, so a failure states which shutdown exceeded the timeout.
🤖 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/orca-runtime/tests/runtime_host.rs` around lines 2865 - 2974, Split background_controller_trace_equivalence into separate named tests for background completion tracing and cancellation during shutdown, preserving each scenario’s setup and assertions so failures identify the affected behavior. Extract the repeated observer deadline polling into a wait_until_event helper and use it for the background completion event check. Add a descriptive failure message to the cancellation shutdown timing assertion identifying the shutdown timeout.crates/orca-runtime/src/tool_router.rs (1)
222-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider grouping the
execute_subagent_toolparameters into a struct.The call now passes 15 positional arguments.
root_task_idwas inserted betweentask_registryandworkflow_ipc. The types differ, so the compiler catches a swap today. A parameter struct, like theRuntime*Contextstructs already used in this module, would keep future insertions safe and readable.🤖 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/orca-runtime/src/tool_router.rs` around lines 222 - 241, Refactor the execute_subagent_tool interface and its call sites to accept a dedicated context/parameter struct instead of 15 positional arguments. Group the existing inputs, including task_registry, root_task_id, workflow_ipc, and subagent_child_executor, using the module’s established Runtime*Context pattern, and update the RuntimeSpecialToolDispatch::Subagent path to construct and pass that struct without changing behavior..github/workflows/release.yml (1)
24-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the
actions/setup-nodeversion across workflows.This job pins
actions/setup-node@v5..github/workflows/windows-ci.ymlpinsactions/setup-node@v6in both jobs. Both run the same Node validation commands withnode-version: 22. Use one major version in all workflows to keep the validation environment identical.♻️ Proposed alignment
- - uses: actions/setup-node@v5 + - uses: actions/setup-node@v6 with: node-version: 22🤖 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 @.github/workflows/release.yml around lines 24 - 26, Update the actions/setup-node step in the release workflow to use the same major version as the corresponding setup-node steps in windows-ci.yml, while preserving node-version 22 and the existing validation commands.crates/orca-runtime/src/tool_turn.rs (1)
1476-1476: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a populated
root_task_id.Both new test initializations set
root_task_id: None. Add one positive test coveringSome(root_task_id)so thatToolExecutionContextand the subagent task tree continue to receive the root task contract when task-tree cancellation depends on it.Also applies to: 2820
🤖 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/orca-runtime/src/tool_turn.rs` at line 1476, Add a positive test alongside the existing ToolExecutionContext test initializations, setting root_task_id to Some(...) with a valid task identifier. Verify the populated root task ID is propagated into the subagent task tree and supports task-tree cancellation, while preserving the existing None-case coverage.crates/orca-provider/src/tool_schema.rs (1)
64-90: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnforce
additionalProperties: falsefor all typed objects in strict schema normalization.DeepSeek strict mode requires every JSON Schema object to set
additionalProperties: false, whilerequire_all_propertiesonly addsrequired. Normalize this field alongsiderequiredinrequire_all_propertiesso strict-capable definitions do not fail strict-schema validation and fall back to the non-strict tool list.🤖 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/orca-provider/src/tool_schema.rs` around lines 64 - 90, Update require_all_properties so every schema node with type "object" also sets additionalProperties to false, alongside populating required from its properties. Keep the existing recursive traversal for nested properties, items, and combinator branches unchanged.crates/orca-runtime/src/lib.rs (1)
59-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueScope the
dead_codeallowance instead of applying it to the whole module.
#[allow(dead_code, unused_imports)]covers every item inruntime_surface. The module is large, so this suppresses genuine dead-code and unused-import warnings for all future changes inside it. Move the allowance to the specific items that external fixtures exercise, or gate it behind#[cfg_attr(not(test), allow(dead_code))]on those items.🤖 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/orca-runtime/src/lib.rs` around lines 59 - 61, Restrict the dead_code allowance currently attached to the runtime_surface module to only the specific externally exercised items within runtime_surface. Remove the module-level suppression, retain unused_imports handling only where needed, and use cfg_attr(not(test), allow(dead_code)) on those individual items when appropriate.crates/orca-runtime/src/goal_store.rs (1)
1179-1181: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe receipt-digest fence is opt-in and duplicated across both entry points. Both fences use
expected_receipt_digest.is_some_and(|digest| digest != state.last_receipt_digest), so a caller that passesNoneskips the check with no compile-time signal, and the same predicate is written twice.
crates/orca-runtime/src/goal_store.rs#L1179-L1181: makePrepareGoalRunForSurfaceInput::expected_receipt_digestrequired, or extract one shared fence helper that both call sites use.crates/orca-runtime/src/goal_store.rs#L1335-L1337: apply the same decision toEditGoalAndPrepareRunForSurfaceInput::expected_receipt_digest.🤖 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/orca-runtime/src/goal_store.rs` around lines 1179 - 1181, The receipt-digest fence is optional and duplicated across both entry points. In crates/orca-runtime/src/goal_store.rs lines 1179-1181, update PrepareGoalRunForSurfaceInput::expected_receipt_digest and the shared fence logic; in lines 1335-1337, apply the same decision to EditGoalAndPrepareRunForSurfaceInput::expected_receipt_digest. Prefer making both fields required so callers cannot silently skip validation, or extract one shared helper used by both paths to centralize the predicate.crates/orca-runtime/src/runtime_actor/mod.rs (1)
8-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting reply effects from commit effects.
RuntimeActorEffectmixesCommitCapability, which only the runtime actor can apply, with the two reply variants, which any caller can apply.apply_runtime_actor_reply_effectincrates/orca-runtime/src/runtime_host.rs(lines 1263-1266) handles this withunreachable!("capability commit effects require the runtime actor"). A separateRuntimeActorReplyEffecttype for the two reply variants would remove that panic path by construction.🤖 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/orca-runtime/src/runtime_actor/mod.rs` around lines 8 - 24, Split RuntimeActorEffect into a runtime-only commit effect and a RuntimeActorReplyEffect containing ReplyCapability and ReplyOperation. Update apply_runtime_actor_reply_effect and all reply-effect producers/consumers to use the new reply type, eliminating the unreachable capability-commit branch while preserving commit handling exclusively in the runtime actor.crates/orca-runtime/src/runtime_actor/background.rs (1)
14-21: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReturn a missing capacity instead of a sentinel value.
BackgroundAdmissionError::DuplicateTaskhas no configured capacity, but.capacity()still returnsusize::MAX. Rename or change the method toOption<usize>and call.unwrap_or_default()in the runtime host log so a duplicate task does not render with the configured capacity value.🤖 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/orca-runtime/src/runtime_actor/background.rs` around lines 14 - 21, Change BackgroundAdmissionError::capacity to return Option<usize>, returning Some(capacity) for CapacityExceeded and None for DuplicateTask. Update the runtime host logging call to use unwrap_or_default() so duplicate tasks render a missing capacity rather than usize::MAX.tests/history_contract.rs (1)
9-26: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSerialize tests that mutate
ORCA_HOMEor remove the mutation.
ORCA_HOME_LOCKonly serializeswith_process_orca_home; the other tests in this binary still readORCA_HOMEconcurrently while the parent process environment changes. This can make concurrent subtests race and flake. Prefer passing the home path through an explicit API, or move the env mutation into an isolated process/test binary.🤖 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 `@tests/history_contract.rs` around lines 9 - 26, Update the tests around with_process_orca_home and ORCA_HOME_LOCK so every test that reads or mutates ORCA_HOME is serialized under the same lock, or remove the process-environment dependency by passing the home path through an explicit API. Ensure no test in this binary can concurrently read ORCA_HOME while another test changes it.tests/provider_contract.rs (1)
113-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative case for the non-beta endpoint, and move the helper out of the test list.
Line 113 only covers the
/betaendpoint, wheredeepseek_strict_tools_schema_for_endpointreturnsSome. Add an assertion that a non-beta base URL returnsNone, so the endpoint gate itself is covered. Also movedefinitionbelow the last test, so the test list stays contiguous.🤖 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 `@tests/provider_contract.rs` around lines 113 - 131, Add a negative assertion alongside the existing beta-endpoint test verifying that deepseek_strict_tools_schema_for_endpoint returns None for a non-beta base URL. Move the definition helper below the final test function so all test functions remain contiguous.crates/orca-runtime/src/subagent_execution.rs (1)
458-476: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider grouping the
execute_subagent_toolparameters into a context struct.
execute_subagent_toolnow takes 19 positional parameters, including three adjacent optional values. The batch path already usesRuntimeSubagentBatchToolTurnContextfor the same data. A similar struct for the single-subagent path would reduce call-site errors and keep both paths symmetric. Defer this if the release scope must stay narrow.🤖 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/orca-runtime/src/subagent_execution.rs` around lines 458 - 476, Group the 19 parameters of execute_subagent_tool into a dedicated single-subagent context struct, mirroring RuntimeSubagentBatchToolTurnContext and including the adjacent optional values together. Update the function signature and all call sites to pass and access this context while preserving existing behavior.crates/orca-runtime/src/tool_invocation.rs (1)
88-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
provider_tool_schema_overridenow always returnsSome.Every path builds a policy and returns
Some(...). TheOptionwrapper no longer expresses "no override". ReturningVec<ProviderToolDefinition>would remove theexpectcalls at the call sites and in the tests. Defer this if the release scope must stay narrow.🤖 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/orca-runtime/src/tool_invocation.rs` around lines 88 - 119, The provider_tool_schema_override function always constructs and returns tool definitions, so remove its unnecessary Option wrapper and return Vec<ProviderToolDefinition> directly. Update all call sites and tests that unwrap or expect this result to use the returned vector directly, preserving the existing policy selection and canonical_tool_definitions mapping.tests/dependency_architecture_contract.rs (1)
7-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
cargo metadata --lockedmakes this test environment-sensitive.The test spawns
cargofrom inside a test binary. It fails whencargois absent fromPATH, when the sandbox is offline for a cold registry, or whenCargo.lockneeds an update for an unrelated reason. The failure message then points at the architecture contract instead of the real cause. Consider parsingCargo.lockand the manifests directly, or asserting onoutput.statuswith a message that names the environment requirement.🤖 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 `@tests/dependency_architecture_contract.rs` around lines 7 - 19, Update the cargo_metadata helper to avoid relying on executing cargo metadata with --locked, preferably by parsing Cargo.lock and the manifests directly. If invoking cargo remains necessary, make the environment requirement explicit in the failure message, including that cargo must be available and the locked metadata command must run successfully.tests/runtime_lifecycle_contract.rs (1)
53-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrack intra-crate module boundary coverage separately.
The inventory says source-read dependency/import tests were removed and replaced by cargo metadata, typed facades, and behavioral tests, but
cargo_metadata_enforces_the_thin_root_dependency_graphonly checks crate and target paths. If modules must enforce dependencies such asagent_loopnot depending on the controller, add a small structural import check instead of source text scanning.🤖 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 `@tests/runtime_lifecycle_contract.rs` around lines 53 - 54, Extend cargo_metadata_enforces_the_thin_root_dependency_graph with a focused structural check for intra-crate module imports, including that agent_loop does not depend on the controller. Use parsed module or dependency metadata rather than scanning source text, while preserving the existing crate and target path assertions.crates/orca-tools/src/web_search.rs (2)
233-238: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider reusing one
reqwest::Clientacross searches.
search_clientbuilds a newreqwest::Clientfor every search call. Each client owns its own connection pool and TLS configuration, so repeated searches pay full TLS handshake cost and never reuse connections. Store one client in aOnceLockand clone it, becausereqwest::Clientis cheap to clone and internally shared.♻️ Proposed shared-client change
-fn search_client() -> Result<reqwest::Client, SearchError> { - reqwest::Client::builder() - .timeout(SEARCH_TIMEOUT) - .build() - .map_err(|error| SearchError::Failed(format!("failed to build web search client: {error}"))) -} +fn search_client() -> Result<reqwest::Client, SearchError> { + static CLIENT: std::sync::OnceLock<Result<reqwest::Client, String>> = + std::sync::OnceLock::new(); + CLIENT + .get_or_init(|| { + reqwest::Client::builder() + .timeout(SEARCH_TIMEOUT) + .build() + .map_err(|error| format!("failed to build web search client: {error}")) + }) + .clone() + .map_err(SearchError::Failed) +}🤖 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/orca-tools/src/web_search.rs` around lines 233 - 238, Update search_client to cache the configured reqwest::Client in a OnceLock and return a clone on subsequent calls, preserving the existing SEARCH_TIMEOUT and SearchError mapping during initialization.
473-510: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe cancellation test can hang if the search fails before the server accepts.
accepted_rx.recv_timeoutbounds the accept step, butworker.join()afterwards has no bound. If the request path returns early, for example whenparse_argsor client construction fails, the worker still terminates, so join returns. The remaining risk is the 250 ms wall-clock assertion on a loaded CI runner, because the poll interval is 10 ms and the thread must also unwind the request. Consider raising the bound to compare against the 25 secondSEARCH_TIMEOUTinstead of a tight absolute value.🤖 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/orca-tools/src/web_search.rs` around lines 473 - 510, Relax the timing assertion in web_search_cancellation_preempts_http_timeout so it remains reliable on loaded CI runners, comparing cancellation completion against the existing 25-second SEARCH_TIMEOUT rather than the tight 250 ms limit. Preserve the cancellation status assertion and server synchronization.crates/orca-tui/src/app.rs (1)
8937-8952: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
reap_hosted_threadabandons a runtime thread silently after 320 ms.The loop retries
shutdown32 times with a 10 ms sleep, then returns without any record. If the previous session refuses to stop, the runtime thread and its MCP child processes stay alive for the rest of the process lifetime, and no log or event reports it. Add a trace or warning on the final failure so the leak is observable.♻️ Proposed change to record the abandoned shutdown
.spawn(move || { for _ in 0..32 { if thread.shutdown().is_ok() { return; } std::thread::sleep(Duration::from_millis(10)); } + tracing::warn!("previous TUI session thread did not shut down; abandoning it"); });🤖 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/orca-tui/src/app.rs` around lines 8937 - 8952, Update reap_hosted_thread so that after all 32 shutdown retries fail, it emits a trace or warning recording the abandoned runtime-thread shutdown, including enough context to identify the failure. Preserve the existing retry and fallback behavior for spawn failures.crates/orca-tui/src/mention_search_manager.rs (1)
305-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCatalog discovery now has no test coverage.
The removed registry test was the only case that exercised
refresh_catalog_async,consume_catalog_dirty, and the merge of catalog candidates intostate.mention.candidates. The remaining tests only drive file-search snapshots, so a regression in the catalog path stays undetected. Add a test that installsTuiSurfaceActionsand asserts that a catalog result reachesstate.mention.candidates.🤖 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/orca-tui/src/mention_search_manager.rs` around lines 305 - 330, The catalog discovery flow lacks coverage for asynchronous refresh and candidate merging. Add a focused test around refresh_catalog_async that installs TuiSurfaceActions, waits for the catalog result through consume_catalog_dirty, and asserts the discovered catalog candidate appears in state.mention.candidates; preserve the existing file-search snapshot tests and use the established test fixtures and synchronization helpers.crates/orca-tools/src/schema.rs (1)
103-113: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider avoiding the JSON string round trip in
normalize_tool_arguments.
normalize_tool_argumentsserializes theValueto a string only so the existingnormalize_raw_argumentshelpers can run, then parses the string again.normalize_tool_requestthen serializes the result a third time. For largeupdate_planpayloads this repeats allocation and parsing work on every tool call. If you exposeValue-level normalizers inupdate_planandupdate_goal, both functions can operate on the parsed value directly.Also applies to: 115-156
🤖 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/orca-tools/src/schema.rs` around lines 103 - 113, Refactor normalize_tool_arguments to avoid serializing and reparsing the Value: expose Value-level normalization helpers from update_plan::normalize_raw_arguments and update_goal::normalized_update_raw_arguments (or their underlying logic), then apply them directly to the input value for ToolName::UpdatePlan and ToolName::UpdateGoal. Preserve the existing fallback and unchanged behavior for other tool names, and ensure normalize_tool_request can reuse the resulting Value without an additional normalization round trip.crates/orca-tui/src/surface_client.rs (1)
1114-1155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelegate
resume_goal_and_runtoresume_goal_and_run_with_started.Both functions build the identical
GoalMutationAction::ResumeAndRunclosure. The only difference is thestartedcallback. Keeping two copies means a future change to the resume fence or input handling must land twice.♻️ Proposed deduplication
pub(crate) fn resume_goal_and_run( thread: &RuntimeSurfaceThreadHandle, prompt: String, controller: &TuiSurfaceTaskControl, event_tx: &mpsc::Sender<TuiEvent>, ) -> io::Result<TuiHostedOperationOutcome> { - run_goal_mutation( - thread, - controller, - event_tx, - || {}, - move |snapshot| { - let goal = snapshot - .goal - .as_ref() - .ok_or_else(|| io::Error::other("no goal is currently set"))?; - Ok(GoalMutationAction::ResumeAndRun { - fence: goal_fence(goal), - input: supplied_goal_input(&prompt)?, - }) - }, - ) + resume_goal_and_run_with_started(thread, prompt, controller, event_tx, || {}) }🤖 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/orca-tui/src/surface_client.rs` around lines 1114 - 1155, Update resume_goal_and_run to delegate to resume_goal_and_run_with_started, passing the existing thread, prompt, controller, event_tx, and a no-op started callback. Remove its duplicated run_goal_mutation and ResumeAndRun closure while preserving the current behavior and return type.crates/orca-tui/src/session_picker_actions.rs (1)
9-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deriving the returning selection index from
SessionPickerAction.The action list is now dynamic, but three unchanged branches still return hardcoded selections:
RenamingEsc usesselected: 2,ConfirmArchiveEsc usesselected: 3, andConfirmDeleteEsc usesselected: 4. These values are correct today because Archive and Delete only exist for non-active sessions. A future reorder of the enum or a new action breaks them silently. A small index helper removes the duplicated ordering knowledge.♻️ Proposed helper
pub(crate) fn available_session_actions( active_session_id: Option<&str>, selected_session_id: &str, ) -> Vec<SessionPickerAction> { let mut actions = vec![ SessionPickerAction::Resume, SessionPickerAction::Fork, SessionPickerAction::Rename, ]; if active_session_id != Some(selected_session_id) { actions.extend([SessionPickerAction::Archive, SessionPickerAction::Delete]); } actions.push(SessionPickerAction::CopySessionId); actions } + +pub(crate) fn session_action_index( + active_session_id: Option<&str>, + selected_session_id: &str, + action: SessionPickerAction, +) -> usize { + available_session_actions(active_session_id, selected_session_id) + .iter() + .position(|candidate| *candidate == action) + .unwrap_or_default() +}🤖 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/orca-tui/src/session_picker_actions.rs` around lines 9 - 46, Derive return-selection indices from SessionPickerAction instead of hardcoded values in the Renaming, ConfirmArchive, and ConfirmDelete escape branches. Add a helper on SessionPickerAction that returns each action’s position in available_session_actions, accounting for the conditional Archive/Delete entries, and use it for those selections while preserving current behavior.crates/orca-tui/src/surface_actions.rs (1)
19-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the shared failure-injection counter against cross-test interference.
RENAME_SAVED_SESSION_FAILURESis process-global. Rust runs#[test]functions in parallel threads inside one binary. If another test callsrename_saved_sessionwhile an injection is armed, that unrelated test consumes the injected failure and fails. Serialize the injecting test with the same process lock used elsewhere in this crate, or scope the counter to a thread-local.🛡️ Proposed thread-local scoping
#[cfg(test)] -static RENAME_SAVED_SESSION_FAILURES: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); +thread_local! { + static RENAME_SAVED_SESSION_FAILURES: std::cell::Cell<usize> = const { std::cell::Cell::new(0) }; +} #[cfg(test)] pub(crate) fn inject_rename_saved_session_failure_once() { - RENAME_SAVED_SESSION_FAILURES.store(1, std::sync::atomic::Ordering::SeqCst); + RENAME_SAVED_SESSION_FAILURES.with(|remaining| remaining.set(1)); }Note: a thread-local only works when the rename runs on the test thread. If the rename runs on a worker thread, keep the atomic and serialize the test instead.
Also applies to: 52-62
🤖 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/orca-tui/src/surface_actions.rs` around lines 19 - 27, Guard the test-only failure injection used by inject_rename_saved_session_failure_once and RENAME_SAVED_SESSION_FAILURES against parallel-test interference. Prefer serializing the injecting test with the existing process lock used elsewhere in the crate; otherwise make the counter thread-local only if rename_saved_session executes on the same test thread. Ensure unrelated tests cannot consume the armed failure.crates/orca-tui/src/surface_boundary_tests.rs (1)
85-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRestore a compile-time link between
CURRENT_ACTIONSandUserAction.The test now compares the manifest against the hardcoded
CURRENT_ACTIONStable only. The previous version validated the parsedUserActionvariants. With that check removed, a newUserActionvariant can ship without any manifest row, and the test still passes. An exhaustivematchoverUserActionmakes the compiler fail when a variant is added and the table is not updated.♻️ Proposed compile-time guard
#[test] fn current_actions_cover_every_user_action_variant() { // The compiler fails this match when a `UserAction` variant is added or renamed. fn action_id(action: &crate::types::UserAction) -> &'static str { use crate::types::UserAction::*; match action { NewSession => "NewSession", ForkCurrentSession { .. } => "ForkCurrentSession", // ... one arm per variant ... ResumeOperation { .. } => "ResumeOperation", CancelOperation { .. } => "CancelOperation", } } let _ = action_id; }🤖 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/orca-tui/src/surface_boundary_tests.rs` around lines 85 - 111, Restore an exhaustive compile-time check linking CURRENT_ACTIONS to crate::types::UserAction. In the surface-boundary tests, add a helper such as action_id that matches every UserAction variant and returns its action identifier, including payload-bearing variants; keep the helper referenced so the compiler checks it and requires updates when variants are added or renamed.
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 (2)
crates/orca-tui/src/surface_boundary_tests.rs (1)
204-204: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd replacement coverage for the runtime-surface action path.
The remaining tests cover manifest classifi cation and closed entrypoint routes, not action execution. Add runtime-surface tests that send
TaskControlActionrequests throughRuntimeSurfaceClientHandlefor task control/ownership,BackgroundCurrentTurn, history operations, runner selection, dispatcher wiring, and runtime control.🤖 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/orca-tui/src/surface_boundary_tests.rs` at line 204, Add replacement runtime-surface coverage in the tests around RuntimeSurfaceClientHandle, sending TaskControlAction requests through the client for task control and ownership, BackgroundCurrentTurn, history operations, runner selection, dispatcher wiring, and runtime control. Retain the existing manifest-classification and closed-entrypoint tests, but ensure each requested action path is exercised through the runtime surface rather than only tested indirectly.crates/orca-runtime/tests/runtime_surface_types.rs (1)
193-193: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd production-code guards before relying on the export manifest.
runtime_surface_public_exportsallows only the listed runtime surface items, but it is an artifact-only constraint andphase_0b_gate.production_code_authorizedremainsfalse. The Node tests do not assert that an internal surface item or unauthorized control command inthread_commandsis rejected when emitted by Rust code. Add a failing case such aspub use commands::*, an unlisted command export, orcommands::InternalSurfaceuntil runtime code is authorized against the manifest.🤖 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/orca-runtime/tests/runtime_surface_types.rs` at line 193, Update the production runtime authorization path, rather than only the artifact test around runtime_surface_public_exports, so phase_0b_gate.production_code_authorized becomes true only after Rust-emitted exports and thread_commands are validated against the manifest. Add a failing coverage case for an internal surface item or unauthorized control command (for example via commands::InternalSurface or an unlisted command export), and ensure the production code rejects it before relying on the export manifest.
🧹 Nitpick comments (1)
crates/orca-runtime/src/child_agent_tests.rs (1)
155-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
tool_policy_labelcontract.The test sets
request.tool_policy_labelto"review-only"at Line 165, but Lines 174-179 only verify the allowed tool. The test can pass ifprepare_child_agent_loopdrops the label. Assert the structured setup field that should contain"review-only".🤖 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/orca-runtime/src/child_agent_tests.rs` around lines 155 - 179, The test prepare_child_agent_loop_applies_request_tool_allowlist_to_provider_schema must also verify that request.tool_policy_label is preserved in the structured setup result. Add an assertion against the setup field responsible for the policy label, expecting "review-only", while retaining the existing allowed-tool assertion.
🤖 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/orca-provider/src/tool_schema.rs`:
- Around line 68-74: Update the typed-object detection in the schema
normalization logic to also recognize a "type" array containing "object", while
preserving support for the existing string form. Ensure nullable object schemas
receive both additionalProperties=false and required normalization, and add a
strict-schema test covering a nullable nested object.
In `@crates/orca-runtime/src/runtime_host.rs`:
- Around line 14016-14028: Update the collision branch around
GoalOperationController::set_pending_recovery to preserve the entire rejected
PendingSurfaceGoalCompletionRecovery, including its active and message fields,
in the controller’s pending recovery slot for later dispatch retry. Do not
retain only rejected.active or conditionally restore self.active; ensure the
rejected recovery remains available while preserving the existing
terminal-blocked state.
---
Outside diff comments:
In `@crates/orca-runtime/tests/runtime_surface_types.rs`:
- Line 193: Update the production runtime authorization path, rather than only
the artifact test around runtime_surface_public_exports, so
phase_0b_gate.production_code_authorized becomes true only after Rust-emitted
exports and thread_commands are validated against the manifest. Add a failing
coverage case for an internal surface item or unauthorized control command (for
example via commands::InternalSurface or an unlisted command export), and ensure
the production code rejects it before relying on the export manifest.
In `@crates/orca-tui/src/surface_boundary_tests.rs`:
- Line 204: Add replacement runtime-surface coverage in the tests around
RuntimeSurfaceClientHandle, sending TaskControlAction requests through the
client for task control and ownership, BackgroundCurrentTurn, history
operations, runner selection, dispatcher wiring, and runtime control. Retain the
existing manifest-classification and closed-entrypoint tests, but ensure each
requested action path is exercised through the runtime surface rather than only
tested indirectly.
---
Nitpick comments:
In `@crates/orca-runtime/src/child_agent_tests.rs`:
- Around line 155-179: The test
prepare_child_agent_loop_applies_request_tool_allowlist_to_provider_schema must
also verify that request.tool_policy_label is preserved in the structured setup
result. Add an assertion against the setup field responsible for the policy
label, expecting "review-only", while retaining the existing allowed-tool
assertion.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: faeac46d-d2e0-49e9-800c-8b3bcf55809e
📒 Files selected for processing (51)
.gitattributes.github/workflows/pages.yml.github/workflows/release.yml.github/workflows/runtime-contract.yml.github/workflows/verify-release.yml.github/workflows/windows-ci.ymlcrates/orca-provider/examples/update_plan_strict_realapi.rscrates/orca-provider/src/tool_schema.rscrates/orca-runtime/src/child_agent_loop_setup.rscrates/orca-runtime/src/child_agent_tests.rscrates/orca-runtime/src/goal_actor.rscrates/orca-runtime/src/goal_store.rscrates/orca-runtime/src/lib.rscrates/orca-runtime/src/runtime_actor/background.rscrates/orca-runtime/src/runtime_actor/capability.rscrates/orca-runtime/src/runtime_actor/commit.rscrates/orca-runtime/src/runtime_actor/goal.rscrates/orca-runtime/src/runtime_host.rscrates/orca-runtime/src/runtime_surface/commands.rscrates/orca-runtime/src/runtime_surface/projection.rscrates/orca-runtime/src/runtime_surface/reducer.rscrates/orca-runtime/src/tasks.rscrates/orca-runtime/src/tool_invocation.rscrates/orca-runtime/src/tool_turn.rscrates/orca-runtime/tests/runtime_host.rscrates/orca-runtime/tests/runtime_surface_commit.rscrates/orca-runtime/tests/runtime_surface_domain.rscrates/orca-runtime/tests/runtime_surface_reducer.rscrates/orca-runtime/tests/runtime_surface_types.rscrates/orca-tools/src/schema.rscrates/orca-tools/src/web_search.rscrates/orca-tui/src/app.rscrates/orca-tui/src/runtime_event_projection.rscrates/orca-tui/src/session_picker_actions.rscrates/orca-tui/src/surface_actions.rscrates/orca-tui/src/surface_boundary_tests.rscrates/orca-tui/src/surface_client.rscrates/orca-tui/src/surface_projection.rscrates/orca-tui/src/transcript_view.rscrates/orca-tui/src/types.rsdocs/release-process.mddocs/releases/v0.3.3.mddocs/reports/2026-08-03-orca-audit-remediation-evidence.mddocs/superpowers/plans/2026-08-03-orca-audit-remediation-v032.mddocs/superpowers/specs/2026-07-21-runtime-owned-typed-surface-private-contract.digest.jsondocs/superpowers/specs/2026-07-21-runtime-owned-typed-surface-private-contract.manifest.jsondocs/superpowers/specs/2026-08-03-orca-audit-remediation-v032-design.mdscripts/validate-runtime-surface-contract.mjstests/cli_architecture_contract.rstests/history_contract.rstests/provider_contract.rs
🚧 Files skipped from review as they are similar to previous changes (30)
- crates/orca-runtime/src/child_agent_loop_setup.rs
- docs/superpowers/specs/2026-07-21-runtime-owned-typed-surface-private-contract.digest.json
- docs/releases/v0.3.3.md
- tests/cli_architecture_contract.rs
- crates/orca-provider/examples/update_plan_strict_realapi.rs
- docs/reports/2026-08-03-orca-audit-remediation-evidence.md
- crates/orca-runtime/tests/runtime_surface_domain.rs
- crates/orca-runtime/tests/runtime_surface_commit.rs
- .gitattributes
- .github/workflows/runtime-contract.yml
- crates/orca-runtime/src/goal_store.rs
- docs/superpowers/plans/2026-08-03-orca-audit-remediation-v032.md
- crates/orca-tui/src/surface_projection.rs
- tests/provider_contract.rs
- .github/workflows/windows-ci.yml
- tests/history_contract.rs
- crates/orca-tools/src/web_search.rs
- crates/orca-tui/src/session_picker_actions.rs
- docs/superpowers/specs/2026-08-03-orca-audit-remediation-v032-design.md
- crates/orca-runtime/src/runtime_surface/commands.rs
- crates/orca-runtime/src/goal_actor.rs
- crates/orca-runtime/src/runtime_actor/commit.rs
- crates/orca-tui/src/transcript_view.rs
- crates/orca-runtime/src/runtime_surface/projection.rs
- crates/orca-runtime/src/runtime_actor/goal.rs
- crates/orca-tui/src/app.rs
- crates/orca-runtime/src/lib.rs
- docs/release-process.md
- crates/orca-runtime/src/tasks.rs
- crates/orca-tui/src/types.rs
1 participant
Summary
Verification
cargo nextest run -p orca-tui --test tui --no-fail-fast --test-threads 1: 1023/1023 passedcargo nextest run --workspace --locked --no-fail-fast: 2490/2490 passedFull evidence:
docs/reports/2026-08-03-orca-audit-remediation-evidence.mdSummary by CodeRabbit
New Features
Bug Fixes
Documentation