feat: Implement and archive slash session commands with SQLite persistence#534
Conversation
…532) * feat(agent): add slash session commands Implement deterministic /resume, /suspend, /tldr, and /compact handling before normal agent execution. Persist resumable session state in SQLite snapshots and wire hydration through the memory loader with focused ingress regressions and validation coverage. * chore(openspec): archive slash session commands change Sync the approved slash-session command delta into the main specs and archive the completed change artifacts after implementation and verification. * fix(agent): harden slash session command state handling Scope resumable-session listings to the caller, make slash-session state updates patch-based, preserve real storage failures, and fix preview/idempotency ingress handling. Also align channel session keys, tighten pending hydration behavior, and update archived/current session specs to match the enforced access rules. * fix(agent): align slash-session regressions with stable keys Update channel and preview-path regressions after the slash-session hardening changes, and adjust the SQLite retry test to simulate a missing hydration snapshot without violating foreign-key constraints. * fix(gateway): restore metrics test expectations Correct the metrics endpoint regressions introduced while updating preview-path slash-session assertions so the gateway hook can validate the branch cleanly. * fix: address inline and outside diff review comments for slash-session-commands - memory_loader.rs: add append_pending_resume_context call to CerebroMemoryLoader - gateway/mod.rs: add update_session_activity calls before early returns in stream handler - gateway/mod.rs: add deny-by-default guard for execution_mode=plan in legacy path - gateway/webhook_dispatch.rs: add clarifying comment for SessionCommand event_frames - memory/sqlite.rs: fail closed on deserialization errors instead of unwrap_or(Value::Null) - session_commands/service.rs: add caller ownership check for targeted resume path - session_commands/service.rs: use non-panicking state lookup instead of require_state - session_commands/types.rs: sanitize StorageFailure error messages for user-facing output - design.md: fix SessionCommandSpec example to match actual registry.rs - spec.md: add caller-scoped visibility check requirement for targeted resume * fix: address additional inline and outside diff review comments - memory_loader.rs: reorder recall before append_pending_resume_context - channels/mod.rs: include reply_target in channel_session_id for conversation uniqueness - gateway/mod.rs: remove plan-mode rejection from SessionCommand branch (allow when dispatcher enabled) - sqlite.rs: use SQL upsert with COALESCE for apply_session_state_patch (avoid RMW in Rust) - sqlite.rs: use FromSqlConversionFailure for JSON deserialization errors - session_commands/service.rs: log sanitized error instead of raw error - session_commands/service.rs: use targeted get_session for ownership check, limit pagination - design.md: add caller authorization check to /resume flow, reword channel-backed sentence - channels test: update expected session format to include reply_target
yacosta738
changed the title
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds deterministic slash-session commands (/resume, /suspend, /tldr, /compact) intercepted at ingress and handled without model/tool execution. Introduces SQLite-only session snapshot/state persistence, Memory trait extensions for session operations, one-shot pending hydration prepended to context, CLI/gateway/channel wiring, and tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Ingress as Gateway/Channel/CLI
participant Evaluator as evaluate_ingress
participant Service as SessionCommandService
participant SQLite as SqliteMemory
participant Agent as Agent Loop / MemoryLoader
Client->>Ingress: send message (e.g., "/tldr")
Ingress->>Evaluator: evaluate_ingress(prompt, session_id, caller_token_hash)
alt Slash Command Recognized
Evaluator->>Service: dispatch(command context)
Service->>SQLite: load_session_transcript_excerpt(session_id)
SQLite-->>Service: entries
Service->>SQLite: create_session_snapshot(kind, payload, resume_capable)
SQLite-->>Service: SessionSnapshotRecord
Service->>SQLite: apply_session_state_patch(patch)
SQLite-->>Service: SessionStateRecord
Service-->>Evaluator: SessionCommandResult
Evaluator-->>Ingress: IngressDecision::SessionCommand
Ingress-->>Client: deterministic response (no model/tool execution)
else Continue / Blocking
Evaluator-->>Ingress: IngressDecision::Continue/Blocking
opt Continue
Ingress->>Agent: proceed to normal execution
Agent->>SQLite: take_pending_resume_hydration(session_id)
SQLite-->>Agent: optional SessionSnapshotRecord (one-shot)
Agent-->>Ingress: enriched context / normal response
end
opt Blocking
Ingress-->>Client: blocking/approval response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-04-21 to 2026-04-21 |
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-04-15 to 2026-04-15 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clients/agent-runtime/src/gateway/mod.rs (1)
2023-2077:⚠️ Potential issue | 🟠 MajorMove legacy slash-session interception ahead of the plan/cost guards.
Line 2026 and Line 2044 reject the legacy
/webhookrequest before Line 2060 gets a chance to short-circuit deterministic session commands. That means/tldr,/resume, etc. still fail withplan_mode_requires_dispatcherorcost_governance_requires_dispatcheron/webhookwhen the dispatcher is off, even though/web/chat/streamalready handles the same commands without touching the provider. This makes the two gateway entrypoints disagree and breaks session lifecycle commands in cost-enabled legacy deployments.Suggested fix
- // Plan mode enforcement: fail-closed if dispatcher is disabled but Plan mode is requested - let resolved_execution_mode = - resolve_webhook_execution_mode(server_execution_mode, webhook_body.execution_mode); - if resolved_execution_mode == ExecutionMode::Plan { - let response = ( - StatusCode::FORBIDDEN, - Json(serde_json::json!({ - "error": { - "code": "plan_mode_requires_dispatcher", - "message": "Plan mode requires webhook_dispatcher_enabled=true in server config", - } - })), - ); - release_idempotency_key(&state, reserved_idempotency_key, false); - update_session_activity_if_persisted(&state, &session_id, token_hash.as_deref(), false) - .await; - return response; - } - - log_webhook_runtime_path(&session_id, false, "dispatcher_flag_disabled"); - - if config.cost.enabled { - let response = ( - StatusCode::SERVICE_UNAVAILABLE, - Json(serde_json::json!({ - "error": { - "code": "cost_governance_requires_dispatcher", - "message": "Cost governance requires the webhook dispatcher path when cost.enabled=true", - } - })), - ); - release_idempotency_key(&state, reserved_idempotency_key, false); - update_session_activity_if_persisted(&state, &session_id, token_hash.as_deref(), false) - .await; - return response; - } - if let Some((response, persist_idempotency)) = canonical_outcome_early_response( &state, &session_id, &scrubbed_message, token_hash.as_deref(), @@ return response; } + + // Plan mode enforcement: fail-closed if dispatcher is disabled but Plan mode is requested + let resolved_execution_mode = + resolve_webhook_execution_mode(server_execution_mode, webhook_body.execution_mode); + if resolved_execution_mode == ExecutionMode::Plan { + let response = ( + StatusCode::FORBIDDEN, + Json(serde_json::json!({ + "error": { + "code": "plan_mode_requires_dispatcher", + "message": "Plan mode requires webhook_dispatcher_enabled=true in server config", + } + })), + ); + release_idempotency_key(&state, reserved_idempotency_key, false); + update_session_activity_if_persisted(&state, &session_id, token_hash.as_deref(), false) + .await; + return response; + } + + log_webhook_runtime_path(&session_id, false, "dispatcher_flag_disabled"); + + if config.cost.enabled { + let response = ( + StatusCode::SERVICE_UNAVAILABLE, + Json(serde_json::json!({ + "error": { + "code": "cost_governance_requires_dispatcher", + "message": "Cost governance requires the webhook dispatcher path when cost.enabled=true", + } + })), + ); + release_idempotency_key(&state, reserved_idempotency_key, false); + update_session_activity_if_persisted(&state, &session_id, token_hash.as_deref(), false) + .await; + return response; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/gateway/mod.rs` around lines 2023 - 2077, The legacy slash-session interception must run before the plan/cost guards so deterministic session commands (e.g., /tldr, /resume) can short-circuit; move the block that calls canonical_outcome_early_response(&state, &session_id, &scrubbed_message, token_hash.as_deref()).await (and its associated release_idempotency_key/reserved_idempotency_key and update_session_activity_if_persisted calls) to occur before the resolve_webhook_execution_mode check that compares resolved_execution_mode == ExecutionMode::Plan and before the config.cost.enabled check; ensure log_webhook_runtime_path, the forbidden/service_unavailable responses, and the early-return logic still execute in the same way when canonical_outcome_early_response returns None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/agent/memory_loader.rs`:
- Around line 181-185: The match arm that forwards Err(error) from
memory.take_pending_resume_hydration(session_id) should attach session-scoped
context before returning so failures include the session_id for triage; update
the Err branch (the match around take_pending_resume_hydration) to map or wrap
the error with context that includes the session_id (e.g., via anyhow::Context
or map_err to add "pending resume hydration failed for session {session_id}")
instead of returning the raw error; keep the special-case
is_slash_session_unsupported_error branch unchanged.
In `@clients/agent-runtime/src/channels/mod.rs`:
- Around line 1852-1859: The call in handle_ingress_outcome passes None for the
caller scope to evaluate_ingress, which allows caller-agnostic visibility for
commands like /resume; change handle_ingress_outcome to obtain a stable
per-sender scope from the provided Channel (or Memory) and pass that scope
instead of None to evaluate_ingress(session_id, content, <caller_scope>)—if the
Channel cannot provide a per-sender scope, then detect resumable-session
commands (e.g., "/resume" or other ingress outcomes that act on NULL-scoped
sessions) and return an explicit rejection/error before calling evaluate_ingress
to prevent cross-sender session access. Ensure you reference
handle_ingress_outcome, evaluate_ingress, Channel, and session_id while plumbing
or validating the caller scope.
- Around line 158-165: The conversation history map is still keyed by the old
channel+sender format while channel_session_id(msg) now includes reply_target,
causing different threads from the same sender to share histories; update every
place that uses format!("{}_{}", msg.channel, msg.sender) (e.g., where
conversation_histories is accessed/inserted) to use channel_session_id(msg)
instead so keys include reply_target, or compute the same three-part key
(channel, sender, reply_target) before reads/writes to conversation_histories to
ensure thread-level isolation.
In `@clients/agent-runtime/src/gateway/mod.rs`:
- Around line 2301-2340: The SSE branch for BlockingOutcome::ApprovalRequired
currently matches ApprovalRequired { tool } and omits the machine-readable
denial reason; update the match to capture the reason (e.g., ApprovalRequired {
tool, reason }) and include "reason": reason in the serde_json payload emitted
to StreamProcessingOutcome::Error so the SSE approval denial JSON mirrors the
webhook/dispatcher variant; keep the existing scrub_sensitive_boundary_text and
session activity update logic intact (symbols to change: ingress_decision
handling, BlockingOutcome::ApprovalRequired, the payload JSON, and the
StreamProcessingOutcome::Error return).
In `@clients/agent-runtime/src/gateway/webhook_dispatch.rs`:
- Around line 403-413: The SessionCommand arm currently always returns
WebhookTerminalOutcome::Completed and ignores the command success flag; update
the IngressDecision::SessionCommand handling to inspect result.success (or the
equivalent success field on result) and set outcome to
WebhookTerminalOutcome::Completed when true and WebhookTerminalOutcome::Failed
when false, preserving response_text (result.message) and leaving
event_frames/tools_called empty; reference the IngressDecision::SessionCommand
match arm, the result variable, WebhookTurnResult struct, and
WebhookTerminalOutcome enum when making the change.
In `@clients/agent-runtime/src/main.rs`:
- Around line 1524-1541: maybe_handle_cli_session_command currently ignores the
success flag from evaluate_ingress's SessionCommand result and always returns
Ok, causing failing slash commands to exit 0; update the match arm for
crate::pre_execution::IngressDecision::SessionCommand to inspect the returned
value's success indicator (e.g., the SessionCommandResult or its success field
from the SessionCommand { result, success, ... } payload), and if it indicates
failure return an Err (propagating a clear CLI error) instead of
Ok(Some(result)); otherwise return Ok(Some(result)) so successful session
commands continue to behave the same.
- Around line 1400-1405: The CLI slash-command interceptor is only invoked for
the `corvus agent` path; wire the same interception into the `corvus code` path
by invoking maybe_handle_cli_session_command(&config, raw_message).await? (the
same call used at the top-level) before handle_code_command() processes the
message so `/resume`, `/suspend`, `/tldr`, `/compact` get short-circuited;
update the code path that consumes message (the variable named
message/raw_message) to run maybe_handle_cli_session_command and return its
result if Some, otherwise fall through to handle_code_command(), and add a
regression test that runs the `corvus code --message "/tldr"` CLI invocation
asserting the interceptor path is taken. Also validate input boundaries on the
message and preserve existing auth/authz/secret handling when moving this logic.
In `@clients/agent-runtime/src/memory/sqlite.rs`:
- Around line 1997-2006: Test gap: SessionFieldPatch::Clear is only exercised on
never-set fields; add a test that sets a field then clears it to catch the
COALESCE bug. In clients/agent-runtime/src/memory/sqlite.rs add a tokio::test
(e.g. clear_patch_actually_clears_previously_set_field) that uses temp_sqlite(),
calls upsert_session("session-1", None), creates a snapshot with
create_session_snapshot, applies apply_session_state_patch with
latest_tldr_snapshot_id: SessionFieldPatch::Set(snapshot.id.clone()), verifies
via get_session_state_record that latest_tldr_snapshot_id is Some(...), then
applies apply_session_state_patch with latest_tldr_snapshot_id:
SessionFieldPatch::Clear and finally asserts get_session_state_record returns
latest_tldr_snapshot_id.is_none(). Ensure the test mirrors the provided example
to reproduce the COALESCE failure.
- Around line 1377-1412: The insert/update treats SessionFieldPatch::Clear the
same as Keep because Clear is mapped to None and the SQL uses
COALESCE(excluded.X, session_state.X); change the logic in the SessionFieldPatch
handling for conn.execute (the patch variables like latest_tldr, latest_compact,
pending_hydration, suspended_at and lifecycle_value) to produce an explicit
"clear" boolean flag per field (e.g., clear_latest_tldr, clear_latest_compact,
etc.) or a sentinel value, then update the ON CONFLICT DO UPDATE expressions for
session_state (fields latest_tldr_snapshot_id, latest_compact_snapshot_id,
pending_hydration_snapshot_id, suspended_at) to use CASE WHEN ?clear_field THEN
NULL ELSE COALESCE(excluded.field, session_state.field) END so Clear sets NULL,
Set uses the provided value, and Keep preserves the existing value; adjust the
prepared params passed to conn.execute accordingly.
- Around line 350-356: Remove the unused helper function apply_field_patch by
deleting its definition, and update or confirm apply_session_state_patch remains
consistent (it currently handles Keep/Clear inline); specifically remove fn
apply_field_patch<T: Clone>(current: Option<T>, patch: &SessionFieldPatch<T>) ->
Option<T> and any imports or derives only used by it, and ensure
apply_session_state_patch (and any callers) keep the intended semantics (no-op
vs clear) so behavior stays unchanged.
- Around line 262-276: lifecycle_from_row and snapshot_kind_from_row currently
map unknown strings to rusqlite::Error::InvalidQuery which drops the invalid
value; change both functions to return a rusqlite error that includes the
unexpected value (for example using rusqlite::Error::FromSqlConversionFailure
with a boxed error or any other rusqlite Error variant that accepts a
cause/message) so logs and debugging show the exact invalid string; update
lifecycle_from_row and snapshot_kind_from_row to build that richer error
including the original value string and return it on the _ match arm.
In `@clients/agent-runtime/src/session_commands/parser.rs`:
- Around line 8-11: The parser currently accepts "/tldr" and "/suspend" even
when followed by extra text; update the parsing so these two commands are only
recognized when the input equals the command exactly (ignoring surrounding
whitespace). Modify the calls to Self::parse_with_prefix for the "/tldr" and
"/suspend" branches (or add a post-check after parse_with_prefix) to verify that
the substring remaining after the matched prefix contains no non-whitespace
characters before returning Some(SessionSlashCommand::Tldr) or
Some(SessionSlashCommand::Suspend); otherwise return None so trailing arguments
(e.g. "/tldr please") are not treated as the command.
In `@clients/agent-runtime/src/session_commands/service.rs`:
- Around line 178-227: The code in handle_resume treats None caller_token_hash
as unrestricted access by calling list_resumable_sessions(None, ...); change
this to fail closed: if caller_token_hash is None return a
SessionCommandError::Unauthorized (or SessionCommandError::InvalidResumeTarget)
instead of performing the visibility check, or implement a targeted ownership
check in Memory (e.g., add/use a method like
memory.is_session_visible_to(session_id, caller_hash)) and call that using
caller_hash; update the caller-ownership branch to stop calling
list_resumable_sessions(None, ...) and either require caller_token_hash or use
the targeted visibility API (references: handle_resume, memory.get_session,
memory.list_resumable_sessions).
In `@clients/agent-runtime/src/session_commands/types.rs`:
- Around line 25-42: The current fallback returns the raw sanitized message
(sanitized) which can leak backend internals; change the final fallback to
return a fixed public message like "storage unavailable" instead of sanitized,
and ensure the detailed sanitized and error_str are recorded only in internal
logs (e.g., via the project's logger) before returning the generic message;
update the block around the keyword checks in types.rs (referencing sanitized
and error_str and the code path that produces
SessionCommandError::StorageFailure) so no unchecked backend text is returned to
callers.
In `@openspec/changes/archive/2026-04-14-slash-session-commands/design.md`:
- Around line 126-130: The documented hydration sequence is out of sync with
MemoryLoader: the code performs the normal memory recall before prepending the
one-shot resumed context block, so update the text (both the block referencing
the sequence around MemoryLoader and the similar block at the other occurrence)
to reflect that order — i.e., MemoryLoader asks memory for a pending hydration
snapshot, normal memory recall runs, then the snapshot payload is prepended as a
one‑shot resume context and pending_hydration_snapshot_id is cleared atomically;
use the exact symbols MemoryLoader, pending_hydration_snapshot_id, “resume
context”/“one-shot resumed context block”, and “normal memory recall” to locate
and edit the two places.
- Around line 237-289: Update the documented Memory trait to match the current
code: add the missing async methods get_session_snapshot and
apply_session_state_patch and change the signature of list_resumable_sessions to
include the caller_token_hash parameter (i.e., list_resumable_sessions(&self,
caller_token_hash: &str, limit: u32, offset: u32) ->
anyhow::Result<Vec<ResumableSessionEntry>>), and ensure the existing methods
(load_session_transcript_excerpt, create_session_snapshot,
get_session_state_record, update_session_state_record,
take_pending_resume_hydration) remain present with the same signatures and
placeholder bail messages so the docs reflect the exact trait API (refer to
Memory, get_session_snapshot, apply_session_state_patch,
list_resumable_sessions, load_session_transcript_excerpt).
In
`@openspec/changes/archive/2026-04-14-slash-session-commands/specs/sessions/spec.md`:
- Around line 149-150: The spec wording incorrectly states that /resume
immediately loads the authoritative resume snapshot; update the text to describe
/resume as performing deferred hydration: explain that handle_resume() only
validates and records pending_hydration_snapshot_id (it does not load the
compact payload) and that the actual context payload is loaded by the memory
loader on the next normal turn, so the returned user-visible result identifies
the resumed session but does not imply immediate injection of context. Also
apply the same clarification to the related paragraphs around lines 163-165.
In `@openspec/changes/archive/2026-04-14-slash-session-commands/tasks.md`:
- Line 14: The task text is inaccurate about recall ordering; update the
checklist item in tasks.md so it matches the implementation in
clients/agent-runtime/src/agent/memory_loader.rs: state that normal recall is
performed first, then any persisted resume context is prepended into the final
assembled context, and that tests should verify atomic clearing of
pending_hydration_snapshot_id when hydration completes; ensure the wording
references adding RED hydration tests for memory_loader.rs and explicitly checks
recall-before-resume-context ordering and atomic clearing of
pending_hydration_snapshot_id.
In `@openspec/specs/sessions/spec.md`:
- Around line 271-278: The spec currently only defines caller-scoped visibility
for `/resume` when invoked by an authenticated bearer token; add explicit rules
for non-bearer ingress (channel and CLI) by either (a) defining how caller scope
is derived for those entry points (e.g., map channel/CLI requests to the
originating channel identity or CLI user identity and apply the same
visibility/ownership rules used by `/resume` and the sessions listing), or (b)
state that `/resume` and the resumable sessions listing are unsupported for
requests without a verifiable caller identity and must return an
authorization/unsupported response; update the three `/resume` bullets to
reference this rule so implementations (and functions that enforce resume
visibility) such as the resume handler and sessions listing logic know to apply
the same caller-scoped visibility or reject non-identified requests.
---
Outside diff comments:
In `@clients/agent-runtime/src/gateway/mod.rs`:
- Around line 2023-2077: The legacy slash-session interception must run before
the plan/cost guards so deterministic session commands (e.g., /tldr, /resume)
can short-circuit; move the block that calls
canonical_outcome_early_response(&state, &session_id, &scrubbed_message,
token_hash.as_deref()).await (and its associated
release_idempotency_key/reserved_idempotency_key and
update_session_activity_if_persisted calls) to occur before the
resolve_webhook_execution_mode check that compares resolved_execution_mode ==
ExecutionMode::Plan and before the config.cost.enabled check; ensure
log_webhook_runtime_path, the forbidden/service_unavailable responses, and the
early-return logic still execute in the same way when
canonical_outcome_early_response returns None.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b25ae2c-10f8-45af-8868-72fb7fdf9f67
📒 Files selected for processing (28)
clients/agent-runtime/crates/corvus-traits/src/memory.rsclients/agent-runtime/src/agent/memory_loader.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/lib.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/memory/lucid.rsclients/agent-runtime/src/memory/markdown.rsclients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/memory/none.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/src/memory/traits.rsclients/agent-runtime/src/pre_execution/mod.rsclients/agent-runtime/src/session_commands/mod.rsclients/agent-runtime/src/session_commands/parser.rsclients/agent-runtime/src/session_commands/registry.rsclients/agent-runtime/src/session_commands/service.rsclients/agent-runtime/src/session_commands/types.rsopenspec/changes/archive/2026-04-14-slash-session-commands/design.mdopenspec/changes/archive/2026-04-14-slash-session-commands/proposal.mdopenspec/changes/archive/2026-04-14-slash-session-commands/specs/agent-loop/spec.mdopenspec/changes/archive/2026-04-14-slash-session-commands/specs/sessions/spec.mdopenspec/changes/archive/2026-04-14-slash-session-commands/state.yamlopenspec/changes/archive/2026-04-14-slash-session-commands/tasks.mdopenspec/changes/archive/2026-04-14-slash-session-commands/verify-report.mdopenspec/specs/agent-loop/spec.mdopenspec/specs/sessions/spec.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: report / Contributor Quality Report
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (9)
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/lib.rsclients/agent-runtime/src/memory/none.rsclients/agent-runtime/src/memory/markdown.rsclients/agent-runtime/src/session_commands/mod.rsclients/agent-runtime/src/memory/lucid.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/agent/memory_loader.rsclients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/session_commands/registry.rsclients/agent-runtime/src/memory/traits.rsclients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/pre_execution/mod.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/session_commands/types.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/parser.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/src/session_commands/service.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/lib.rsclients/agent-runtime/src/memory/none.rsclients/agent-runtime/src/memory/markdown.rsclients/agent-runtime/src/session_commands/mod.rsclients/agent-runtime/src/memory/lucid.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/agent/memory_loader.rsclients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/session_commands/registry.rsclients/agent-runtime/src/memory/traits.rsclients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/pre_execution/mod.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/session_commands/types.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/parser.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/crates/corvus-traits/src/memory.rsclients/agent-runtime/src/session_commands/service.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/lib.rsclients/agent-runtime/src/memory/none.rsclients/agent-runtime/src/memory/markdown.rsclients/agent-runtime/src/session_commands/mod.rsclients/agent-runtime/src/memory/lucid.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/agent/memory_loader.rsclients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/session_commands/registry.rsclients/agent-runtime/src/memory/traits.rsclients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/pre_execution/mod.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/session_commands/types.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/parser.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/crates/corvus-traits/src/memory.rsclients/agent-runtime/src/session_commands/service.rs
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/agent-runtime/src/lib.rsclients/agent-runtime/src/memory/none.rsopenspec/changes/archive/2026-04-14-slash-session-commands/state.yamlclients/agent-runtime/src/memory/markdown.rsclients/agent-runtime/src/session_commands/mod.rsclients/agent-runtime/src/memory/lucid.rsclients/agent-runtime/src/main.rsopenspec/changes/archive/2026-04-14-slash-session-commands/proposal.mdopenspec/specs/agent-loop/spec.mdopenspec/changes/archive/2026-04-14-slash-session-commands/verify-report.mdclients/agent-runtime/src/agent/memory_loader.rsclients/agent-runtime/src/memory/mod.rsopenspec/changes/archive/2026-04-14-slash-session-commands/tasks.mdclients/agent-runtime/src/session_commands/registry.rsclients/agent-runtime/src/memory/traits.rsclients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/pre_execution/mod.rsopenspec/changes/archive/2026-04-14-slash-session-commands/specs/agent-loop/spec.mdopenspec/changes/archive/2026-04-14-slash-session-commands/specs/sessions/spec.mdopenspec/specs/sessions/spec.mdclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/session_commands/types.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/parser.rsclients/agent-runtime/src/memory/sqlite.rsopenspec/changes/archive/2026-04-14-slash-session-commands/design.mdclients/agent-runtime/crates/corvus-traits/src/memory.rsclients/agent-runtime/src/session_commands/service.rs
clients/agent-runtime/src/main.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/main.rs: Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Keep startup path lean and avoid heavy initialization in command parsing flow
Files:
clients/agent-runtime/src/main.rs
**/*.{md,mdx}
⚙️ CodeRabbit configuration file
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
For user-facing docs, check EN/ES parity or explicitly note pending translation gaps.
Files:
openspec/changes/archive/2026-04-14-slash-session-commands/proposal.mdopenspec/specs/agent-loop/spec.mdopenspec/changes/archive/2026-04-14-slash-session-commands/verify-report.mdopenspec/changes/archive/2026-04-14-slash-session-commands/tasks.mdopenspec/changes/archive/2026-04-14-slash-session-commands/specs/agent-loop/spec.mdopenspec/changes/archive/2026-04-14-slash-session-commands/specs/sessions/spec.mdopenspec/specs/sessions/spec.mdopenspec/changes/archive/2026-04-14-slash-session-commands/design.md
clients/agent-runtime/src/{security,gateway,tools}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Treat
src/security/,src/gateway/,src/tools/as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Files:
clients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/gateway/mod.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/gateway/mod.rs
clients/agent-runtime/src/channels/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Implement
Channeltrait insrc/channels/with consistentsend,listen, andhealth_checksemantics and cover auth/allowlist/health behavior with tests
Files:
clients/agent-runtime/src/channels/mod.rs
🧠 Learnings (10)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/src/lib.rsclients/agent-runtime/src/memory/none.rsclients/agent-runtime/src/memory/markdown.rsclients/agent-runtime/src/session_commands/mod.rsclients/agent-runtime/src/memory/lucid.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/agent/memory_loader.rsclients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/session_commands/registry.rsclients/agent-runtime/src/memory/traits.rsclients/agent-runtime/src/pre_execution/mod.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/session_commands/types.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/parser.rsclients/agent-runtime/src/memory/sqlite.rsopenspec/changes/archive/2026-04-14-slash-session-commands/design.mdclients/agent-runtime/crates/corvus-traits/src/memory.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Keep startup path lean and avoid heavy initialization in command parsing flow
Applied to files:
clients/agent-runtime/src/lib.rsclients/agent-runtime/src/session_commands/mod.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/session_commands/registry.rsclients/agent-runtime/src/pre_execution/mod.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/session_commands/types.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/parser.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
clients/agent-runtime/src/lib.rsclients/agent-runtime/src/memory/markdown.rsclients/agent-runtime/src/main.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/providers/**/*.rs : Implement `Provider` trait in `src/providers/` and register in `src/providers/mod.rs` factory when adding a new provider
Applied to files:
clients/agent-runtime/src/session_commands/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
clients/agent-runtime/src/main.rsclients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/gateway/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
Applied to files:
clients/agent-runtime/src/main.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/gateway/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/Cargo.toml : Do not add heavy dependencies for minor convenience; justify new crate additions
Applied to files:
clients/agent-runtime/src/main.rsclients/agent-runtime/src/memory/traits.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
clients/agent-runtime/src/main.rsclients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/gateway/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Applied to files:
clients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/memory/traits.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/Cargo.toml : Preserve release-size profile assumptions in `Cargo.toml` and avoid adding heavy dependencies unless clearly justified
Applied to files:
clients/agent-runtime/src/memory/traits.rs
🪛 LanguageTool
openspec/changes/archive/2026-04-14-slash-session-commands/proposal.md
[style] ~83-~83: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...in openspec/specs/sessions/spec.md. - Existing shared ingress/pre-execution path used ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
openspec/changes/archive/2026-04-14-slash-session-commands/specs/sessions/spec.md
[style] ~228-~228: The double modal “requires snapshot” is nonstandard (only accepted in certain dialects). Consider “to be snapshot”.
Context: ...- WHEN /compact or /resume requires snapshot/state persistence APIs - THEN the runti...
(NEEDS_FIXED)
openspec/specs/sessions/spec.md
[style] ~568-~568: The double modal “requires snapshot” is nonstandard (only accepted in certain dialects). Consider “to be snapshot”.
Context: ...- WHEN /compact or /resume requires snapshot/state persistence APIs - THEN the runti...
(NEEDS_FIXED)
openspec/changes/archive/2026-04-14-slash-session-commands/design.md
[style] ~390-~390: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..._state.lifecycle_state='suspended'. 6. Validate latest_compact_snapshot_id` points to ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.22.0)
openspec/changes/archive/2026-04-14-slash-session-commands/verify-report.md
[warning] 111-111: Files should end with a single newline character
(MD047, single-trailing-newline)
🔇 Additional comments (10)
clients/agent-runtime/src/memory/mod.rs (1)
27-30: Re-export surface looks correct for the new slash-session memory contract.This expansion is coherent with the updated trait API and keeps the runtime shim ergonomic.
clients/agent-runtime/src/pre_execution/mod.rs (1)
44-84: Ingress interception ordering is implemented cleanly.Recognized session commands short-circuit before canonical evaluation, while non-matching inputs continue through the existing blocking classifier path.
clients/agent-runtime/src/memory/traits.rs (1)
3-8: Compatibility shim update is consistent with the expanded trait contract.No issues in the added re-exports.
clients/agent-runtime/src/memory/sqlite.rs (5)
80-86: LGTM: PRAGMA tuning and foreign key enforcement.Enabling
PRAGMA foreign_keys = ONis essential for the newsession_snapshotsandsession_statetables to enforce referential integrity.
1510-1514: Intentional retry behavior on missing snapshot — verify this is desired.When
pending_hydration_snapshot_idpoints to a deleted snapshot, the code returns an error but leaves the pointer intact for retry. This is tested inpending_hydration_missing_snapshot_keeps_pointer_for_retry.However, this creates a permanent error state requiring manual intervention. Consider whether clearing the dangling pointer (and logging the inconsistency) would be more operationally friendly.
1444-1455: LGTM: Token-scoped resumable session listing.Using
IS ?3correctly handles bothNULL(unscoped sessions) and actual token values for ownership filtering. The join chain ensures only validly suspended sessions with resume-capable snapshots are returned.
1217-1254: LGTM: Session transcript excerpt loading.Properly validates session existence, filters by
session_idandconversationcategory, with consistent ordering for deterministic results.
1256-1295: LGTM: Snapshot creation with proper FK validation.Correctly validates the session exists before inserting the snapshot, preventing orphaned records.
clients/agent-runtime/src/gateway/mod.rs (1)
4482-4533: Good regression coverage for provider bypass on deterministic session commands.These tests lock in the important behavior that legacy webhook and SSE slash-session commands short-circuit without invoking the provider.
Also applies to: 6031-6080
clients/agent-runtime/crates/corvus-traits/src/memory.rs (1)
357-417: Explicit unsupported defaults look good.These trait defaults fail loudly instead of silently no-oping, which keeps non-SQLite backends from pretending slash-session persistence succeeded.
Based on learnings, "Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths".
- memory_loader: add anyhow::Context import; wrap take_pending_resume_hydration error with session_id context - channels/mod.rs: fix BlockingOutcome::ApprovalRequired pattern (add reason field); fix LoopEvent::ApprovalRequired 2-tuple patterns; use channel_session_id(msg) key for conversation_histories - gateway/mod.rs: move canonical_outcome_early_response before plan/cost guards so /tldr, /resume short-circuit deterministically; add WebhookTerminalOutcome ::Failed arm to label, response, and SSE dispatcher; fix LoopEvent:: ApprovalRequired 2-tuple patterns - gateway/webhook_dispatch.rs: add WebhookTerminalOutcome::Failed variant; inspect result.success in SessionCommand arm; fix ApprovalRequired pattern - pre_execution/mod.rs: add approval_reason: None to test struct literals - unified_loop.rs: fix ApprovalRequired(..) match patterns in tests - memory/sqlite.rs: fix SessionFieldPatch::Clear COALESCE bug with explicit clear flags and CASE WHEN; fix clear_lifecycle (Option<> means Keep, not Clear); use COALESCE(?2, 'active') in INSERT to avoid NOT NULL on lifecycle; add clear_patch_actually_clears_previously_set_field regression test - session_commands/service.rs: fail-closed on None caller_token_hash in handle_resume; fix brace/indentation in else branch; update tests to pass caller-hash and add listed entries for visibility - session_commands/types.rs: return fixed 'storage unavailable' fallback - session_commands/parser.rs: exact-match /tldr and /suspend - main.rs: propagate SessionCommand failure as Err; wire slash-command intercept into corvus code path - openspec docs: sync hydration sequence, Memory trait signatures, /resume deferred-hydration wording, tasks recall ordering, sessions spec caller scope Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Deploying corvus with
|
| Latest commit: |
5c8e23b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://da5c3df3.corvus-42x.pages.dev |
| Branch Preview URL: | https://develop.corvus-42x.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
clients/agent-runtime/src/main.rs (2)
781-781: 🧹 Nitpick | 🔵 TrivialSame single-field pattern issue in
loop_event_kind.Line 781 uses
ApprovalRequired(_)but should beApprovalRequired(_, _)for the two-field variant.♻️ Suggested fix
- crate::agent::unified_loop::LoopEvent::ApprovalRequired(_) => "approval_required", + crate::agent::unified_loop::LoopEvent::ApprovalRequired(_, _) => "approval_required",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/main.rs` at line 781, The match arm in function loop_event_kind incorrectly matches the two-field enum variant by using LoopEvent::ApprovalRequired(_)—update it to match both fields as LoopEvent::ApprovalRequired(_, _) so the pattern covers the two-tuple variant correctly; locate the match that returns "approval_required" and change the pattern to ApprovalRequired(_, _) to fix the compile/mismatch issue.
715-719: 🧹 Nitpick | 🔵 TrivialPattern match on
ApprovalRequireduses single-field syntax but variant is now two-field.Line 718 matches
LoopEvent::ApprovalRequired(_)but the variant was changed toApprovalRequired(String, String). This will compile but only captures the first field via the wildcard.This appears intentional (checking for existence, not extracting values), but the pattern should match the actual variant shape for clarity.
♻️ Suggested: Use explicit two-field pattern for clarity
- crate::agent::unified_loop::LoopEvent::ApprovalRequired(_) + crate::agent::unified_loop::LoopEvent::ApprovalRequired(_, _)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/main.rs` around lines 715 - 719, The pattern match inside the closure iterating preview.events is using the single-field wildcard 'ApprovalRequired(_)' but the enum variant is now two-field (ApprovalRequired(String, String)); update the match to the correct shape—use 'LoopEvent::ApprovalRequired(_, _)' in the closure (the iter().any(...) check) so the pattern reflects the actual variant signature and clearly indicates you only care about presence, not values.clients/agent-runtime/src/agent/unified_entrypoint.rs (1)
39-47:⚠️ Potential issue | 🔴 CriticalAdd missing
approval_reasonfield inmain.rsstruct initialization to fix compilation error.The
CanonicalOutcomestruct now requiresapproval_reason: Option<String>(line 44 ofunified_entrypoint.rs), but the construction atmain.rs:734omits this field, preventing compilation.Fix at main.rs:734
return crate::agent::unified_entrypoint::CanonicalOutcome { session_id: preview.session_id, events: preview.events, approval_required: None, + approval_reason: None, timeout_aborted: false, fallback_response: if preview.used_fallback {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/agent/unified_entrypoint.rs` around lines 39 - 47, The CanonicalOutcome struct now includes an approval_reason: Option<String> field; update the code that constructs CanonicalOutcome (the struct literal around where CanonicalOutcome is instantiated in main.rs) to provide that field (e.g., approval_reason: None or approval_reason: Some(existing_reason.clone()) as appropriate), ensuring the field name exactly matches approval_reason: Option<String> to fix the missing-field compilation error.
♻️ Duplicate comments (1)
clients/agent-runtime/src/channels/mod.rs (1)
704-709:⚠️ Potential issue | 🔴 CriticalKeep caller scoping when no concrete
Channelis available.
caller_scopeis only derived fromchannel, so any message injected throughChannelRuntimeHandlefrom a non-registered ingress path still callsevaluate_ingress(..., None). That reopens unscoped/resumelist/load behavior for those senders. Pass the logical channel identity intohandle_ingress_outcomeand derive scope fromchannel_name + sender, or reject resumable session commands when no verifiable scope can be established.Suggested direction
- if handle_ingress_outcome( - target_channel.as_ref(), + if handle_ingress_outcome( + &msg.channel, + target_channel.as_ref(), ctx.memory.as_ref(), &session_id, &msg.sender, &msg.reply_target, &user_text, @@ async fn handle_ingress_outcome( + channel_name: &str, channel: Option<&Arc<dyn Channel>>, memory: &dyn Memory, session_id: &str, sender: &str, reply_target: &str, content: &str, ) -> Option<()> { - let caller_scope = channel.map(|ch| { + let caller_scope = Some({ use sha2::{Digest, Sha256}; let mut hasher = Sha256::new(); - hasher.update(ch.name().as_bytes()); + hasher.update(channel_name.as_bytes()); hasher.update(b":"); hasher.update(sender.as_bytes()); hex::encode(hasher.finalize()) });As per coding guidelines "Security first, performance second. Validate input boundaries, auth/authz implications, and secret management."
Also applies to: 1853-1875
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/mod.rs` around lines 704 - 709, The caller scoping is lost when messages come through ChannelRuntimeHandle without a registered Channel (caller_scope is only derived from channel), so update the ingress flow to propagate a logical channel identity into handle_ingress_outcome (instead of relying solely on channel), have handle_ingress_outcome (and evaluate_ingress) derive caller_scope from channel_name + msg.sender when channel is None, and reject any resumable /resume session commands if a verifiable scope cannot be established; tie this logic to ChannelRuntimeHandle, handle_ingress_outcome, evaluate_ingress, caller_scope, channel_name, sender, and session_id so unregistered ingress paths cannot reuse scoped resume behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/gateway/mod.rs`:
- Around line 1778-1785: The current match arm for
IngressDecision::SessionCommand always returns StatusCode::OK even when success
is false; change it to return a 422 Unprocessable Entity with a structured error
body (include code = "session_command_failed" and a message from result.message)
when success == false, otherwise keep the existing 200 OK response; update the
match arm for IngressDecision::SessionCommand (using variables result, success,
state.model, session_id) to branch on success and return either
((StatusCode::OK, Json(body)), true) or ((StatusCode::UNPROCESSABLE_ENTITY,
Json(error_body)), false) accordingly so webhook error handling matches
dispatcher behavior.
- Around line 2177-2202: The SessionCommand branch currently always emits a
"chunk" + "done" SSE; change it to inspect result.success on the
ingress_decision::SessionCommand payload and emit a success-shaped stream only
when result.success is true: keep the existing chunk + done events (including
message_id and session_id) in that case. If result.success is false, emit an SSE
"error" event (matching the dispatcher path) with code =
"session_command_failed" and any available error details from result (and do not
emit the success "done" payload). Update the logic around
ingress_decision::SessionCommand, result, and the events construction so the
stream reflects the success flag.
In `@clients/agent-runtime/src/session_commands/service.rs`:
- Around line 206-230: The duplicate targeted lookup in service.rs (the second
call to self.memory.get_session producing _session) should be removed because
the session was already fetched earlier; instead reuse the earlier session
result for existence checks or simply rely on the visibility check that follows.
Remove the unnecessary get_session invocation and its error mapping (the block
that creates _session), keep the visibility check using
self.memory.list_resumable_sessions(...).iter().any(...), and return
SessionCommandError::InvalidResumeTarget via the same path if not visible;
optionally, if this is a hot path, add a
Memory::is_session_visible_to(session_id, caller_hash) method and call that from
the resume logic to perform an O(1) ownership/visibility check instead of
scanning list_resumable_sessions. Ensure map_storage_error and
SessionCommandError::InvalidResumeTarget usages remain unchanged.
In `@openspec/specs/sessions/spec.md`:
- Around line 571-575: Update the scenario titled "Scenario: SQLite backend
exposes slash-session persistence operations" to replace the awkward phrase
"requires snapshot/state persistence APIs" with clearer wording — e.g., "needs
snapshot/state persistence APIs" or "requires snapshot/state persistence
operations" — so the step reads: "WHEN `/compact` or `/resume` needs
snapshot/state persistence APIs" (or use "requires ... operations"); ensure the
amended sentence maintains technical accuracy and aligns with the existing spec
tone.
- Around line 243-249: Update the session semantics to specify how the new
suspended state interacts with activity updates and stale auto-close: modify
SESS-3/SESS-5 behavior so that sessions with state "suspended" (the
session-state record set by /suspend) are excluded from normal activity-counting
and the existing ended_at IS NULL stale-auto-close rule, or alternatively define
a distinct expiry rule for suspended sessions (e.g., suspended_expires_at or a
separate TTL) and describe how /resume consults that expiry to remain
deterministic; reference the /suspend and /resume endpoints, the session-state
record and the fields ended_at and any activity timestamp used by auto-close so
readers and implementers know to exclude or treat suspended sessions
differently.
---
Outside diff comments:
In `@clients/agent-runtime/src/agent/unified_entrypoint.rs`:
- Around line 39-47: The CanonicalOutcome struct now includes an
approval_reason: Option<String> field; update the code that constructs
CanonicalOutcome (the struct literal around where CanonicalOutcome is
instantiated in main.rs) to provide that field (e.g., approval_reason: None or
approval_reason: Some(existing_reason.clone()) as appropriate), ensuring the
field name exactly matches approval_reason: Option<String> to fix the
missing-field compilation error.
In `@clients/agent-runtime/src/main.rs`:
- Line 781: The match arm in function loop_event_kind incorrectly matches the
two-field enum variant by using LoopEvent::ApprovalRequired(_)—update it to
match both fields as LoopEvent::ApprovalRequired(_, _) so the pattern covers the
two-tuple variant correctly; locate the match that returns "approval_required"
and change the pattern to ApprovalRequired(_, _) to fix the compile/mismatch
issue.
- Around line 715-719: The pattern match inside the closure iterating
preview.events is using the single-field wildcard 'ApprovalRequired(_)' but the
enum variant is now two-field (ApprovalRequired(String, String)); update the
match to the correct shape—use 'LoopEvent::ApprovalRequired(_, _)' in the
closure (the iter().any(...) check) so the pattern reflects the actual variant
signature and clearly indicates you only care about presence, not values.
---
Duplicate comments:
In `@clients/agent-runtime/src/channels/mod.rs`:
- Around line 704-709: The caller scoping is lost when messages come through
ChannelRuntimeHandle without a registered Channel (caller_scope is only derived
from channel), so update the ingress flow to propagate a logical channel
identity into handle_ingress_outcome (instead of relying solely on channel),
have handle_ingress_outcome (and evaluate_ingress) derive caller_scope from
channel_name + msg.sender when channel is None, and reject any resumable /resume
session commands if a verifiable scope cannot be established; tie this logic to
ChannelRuntimeHandle, handle_ingress_outcome, evaluate_ingress, caller_scope,
channel_name, sender, and session_id so unregistered ingress paths cannot reuse
scoped resume behavior.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46670ea4-fde7-4192-ab55-c46a50da428c
📒 Files selected for processing (16)
clients/agent-runtime/src/agent/memory_loader.rsclients/agent-runtime/src/agent/unified_entrypoint.rsclients/agent-runtime/src/agent/unified_loop.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/src/pre_execution/mod.rsclients/agent-runtime/src/session_commands/parser.rsclients/agent-runtime/src/session_commands/service.rsclients/agent-runtime/src/session_commands/types.rsopenspec/changes/archive/2026-04-14-slash-session-commands/design.mdopenspec/changes/archive/2026-04-14-slash-session-commands/specs/sessions/spec.mdopenspec/changes/archive/2026-04-14-slash-session-commands/tasks.mdopenspec/specs/sessions/spec.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: report / Contributor Quality Report
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (9)
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/agent/unified_entrypoint.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/pre_execution/mod.rsclients/agent-runtime/src/agent/memory_loader.rsclients/agent-runtime/src/agent/unified_loop.rsclients/agent-runtime/src/session_commands/parser.rsclients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/session_commands/types.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/agent/unified_entrypoint.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/pre_execution/mod.rsclients/agent-runtime/src/agent/memory_loader.rsclients/agent-runtime/src/agent/unified_loop.rsclients/agent-runtime/src/session_commands/parser.rsclients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/session_commands/types.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/agent/unified_entrypoint.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/pre_execution/mod.rsclients/agent-runtime/src/agent/memory_loader.rsclients/agent-runtime/src/agent/unified_loop.rsclients/agent-runtime/src/session_commands/parser.rsclients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/session_commands/types.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/agent-runtime/src/agent/unified_entrypoint.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/pre_execution/mod.rsopenspec/changes/archive/2026-04-14-slash-session-commands/tasks.mdclients/agent-runtime/src/agent/memory_loader.rsopenspec/specs/sessions/spec.mdclients/agent-runtime/src/agent/unified_loop.rsopenspec/changes/archive/2026-04-14-slash-session-commands/specs/sessions/spec.mdclients/agent-runtime/src/session_commands/parser.rsclients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/session_commands/types.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rsopenspec/changes/archive/2026-04-14-slash-session-commands/design.md
clients/agent-runtime/src/main.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/main.rs: Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Keep startup path lean and avoid heavy initialization in command parsing flow
Files:
clients/agent-runtime/src/main.rs
**/*.{md,mdx}
⚙️ CodeRabbit configuration file
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
For user-facing docs, check EN/ES parity or explicitly note pending translation gaps.
Files:
openspec/changes/archive/2026-04-14-slash-session-commands/tasks.mdopenspec/specs/sessions/spec.mdopenspec/changes/archive/2026-04-14-slash-session-commands/specs/sessions/spec.mdopenspec/changes/archive/2026-04-14-slash-session-commands/design.md
clients/agent-runtime/src/{security,gateway,tools}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Treat
src/security/,src/gateway/,src/tools/as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Files:
clients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/gateway/mod.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/gateway/mod.rs
clients/agent-runtime/src/channels/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Implement
Channeltrait insrc/channels/with consistentsend,listen, andhealth_checksemantics and cover auth/allowlist/health behavior with tests
Files:
clients/agent-runtime/src/channels/mod.rs
🧠 Learnings (8)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/src/agent/unified_entrypoint.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/pre_execution/mod.rsopenspec/changes/archive/2026-04-14-slash-session-commands/tasks.mdclients/agent-runtime/src/agent/memory_loader.rsclients/agent-runtime/src/agent/unified_loop.rsclients/agent-runtime/src/session_commands/parser.rsclients/agent-runtime/src/gateway/webhook_dispatch.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/session_commands/types.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rsopenspec/changes/archive/2026-04-14-slash-session-commands/design.md
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Applied to files:
clients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/src/main.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Keep startup path lean and avoid heavy initialization in command parsing flow
Applied to files:
clients/agent-runtime/src/main.rsclients/agent-runtime/src/session_commands/parser.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/session_commands/types.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
clients/agent-runtime/src/main.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
clients/agent-runtime/src/main.rsclients/agent-runtime/src/pre_execution/mod.rsopenspec/specs/sessions/spec.mdclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
Applied to files:
clients/agent-runtime/src/main.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/gateway/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
clients/agent-runtime/src/main.rsclients/agent-runtime/src/pre_execution/mod.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/session_commands/types.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/tools/**/*.rs : Implement `Tool` trait in `src/tools/` with strict parameter schema, validate and sanitize all inputs, and return structured `ToolResult` without panics in runtime path
Applied to files:
clients/agent-runtime/src/session_commands/types.rs
🪛 LanguageTool
openspec/specs/sessions/spec.md
[style] ~574-~574: The double modal “requires snapshot” is nonstandard (only accepted in certain dialects). Consider “to be snapshot”.
Context: ...- WHEN /compact or /resume requires snapshot/state persistence APIs - THEN the runti...
(NEEDS_FIXED)
openspec/changes/archive/2026-04-14-slash-session-commands/specs/sessions/spec.md
[style] ~228-~228: The double modal “requires snapshot” is nonstandard (only accepted in certain dialects). Consider “to be snapshot”.
Context: ...- WHEN /compact or /resume requires snapshot/state persistence APIs - THEN the runti...
(NEEDS_FIXED)
openspec/changes/archive/2026-04-14-slash-session-commands/design.md
[style] ~398-~398: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..._state.lifecycle_state='suspended'. 6. Validate latest_compact_snapshot_id` points to ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (43)
openspec/changes/archive/2026-04-14-slash-session-commands/specs/sessions/spec.md (1)
149-165: LGTM — deferred hydration flow is now accurately documented.The spec correctly describes
/resumeas settingpending_hydration_snapshot_idwithout immediate context injection, with actual payload loading delegated to the memory loader on the next normal turn. This addresses the prior review feedback.clients/agent-runtime/src/session_commands/types.rs (2)
7-44: LGTM — storage error sanitization now fails closed.The fallback at line 43 correctly returns a fixed
"storage unavailable"message instead of potentially leaking backend internals. The debug-level internal logging is appropriate for troubleshooting.
46-133: LGTM — well-structured command types and error handling.Clear separation of command variants, context, results, and errors. The
message()method provides deterministic, user-safe error strings without leaking internal details.clients/agent-runtime/src/session_commands/parser.rs (2)
5-56: LGTM — parser correctly enforces exact vs prefix matching.The
parse_exacthelper at lines 34-45 addresses the prior review feedback by rejecting/tldr pleaseand/suspend now. Theparse_with_prefixhelper properly gates prefix commands like/resume-laterfrom being misinterpreted.
59-128: Solid test coverage for parser edge cases.Tests verify exact-match rejection, trailing whitespace acceptance, target/args parsing, and slash-like fallthrough. Good regression coverage.
openspec/changes/archive/2026-04-14-slash-session-commands/design.md (2)
126-131: LGTM — hydration sequence documentation is now accurate.The flow correctly describes: recall runs first → MemoryLoader checks for pending hydration → snapshot payload prepended → pending marker cleared atomically. This matches the implementation.
237-297: LGTM — Memory trait contract documentation is now current.The trait now includes
get_session_snapshot,apply_session_state_patch, and the updatedlist_resumable_sessionssignature withcaller_token_hash: Option<&str>. This addresses the prior review feedback about stale signatures.openspec/changes/archive/2026-04-14-slash-session-commands/tasks.md (1)
14-15: LGTM — task description now reflects correct hydration ordering.Line 14 accurately describes the implementation: normal recall runs first, then resume context is prepended, with atomic clearing of
pending_hydration_snapshot_id.clients/agent-runtime/src/agent/unified_loop.rs (2)
30-30: LGTM —ApprovalRequiredvariant extended to carry reason.The two-field variant
ApprovalRequired(String, String)cleanly separates the tool name from the approval message, and downstream consumers inchannels/mod.rsandgateway/mod.rscorrectly destructure it.
104-107: LGTM — approval event construction is correct.Emits both the tool name and a descriptive message, consistent with the updated variant shape.
clients/agent-runtime/src/agent/unified_entrypoint.rs (1)
177-186: LGTM — approval extraction handles the new two-field variant correctly.The loop finds the first
ApprovalRequired(tool, reason)event and captures both fields, then breaks. Efficient and correct.clients/agent-runtime/src/main.rs (4)
1400-1405: LGTM — slash command interception wired into agent CLI path.Early return for recognized session commands before canonical loop evaluation. Consistent with the code path handling.
1524-1547: LGTM — CLI errors now propagated correctly.The function returns
Err(anyhow!(...))whensuccessis false (lines 1538-1542), fixing the prior issue where failing slash commands would exit0. This preserves the CLI contract.
1559-1569: LGTM — slash command interception now coverscorvus codepath.This addresses the prior review feedback about
/tldr,/suspend, etc. behaving differently incorvus codevscorvus agent. Both paths now share the same interception logic.
3407-3421: Good regression test for CLI session command interception.Verifies that
/tldris handled before agent execution and returns the expected SQLite-required message for unsupported backends.clients/agent-runtime/src/memory/sqlite.rs (7)
262-290: LGTM on error reporting improvements.The
lifecycle_from_rowandsnapshot_kind_from_rowhelpers now include the invalid value in the error message viaFromSqlConversionFailure, which aids debugging data integrity issues.
292-302: LGTM on session existence validation.
ensure_session_existscorrectly checks for the session before proceeding with dependent operations.
1371-1448: Verify Clear semantics work correctly with the new CASE WHEN approach.The implementation now uses explicit clear flags with
CASE WHEN ?N THEN NULL ELSE COALESCE(...)which should properly differentiateClear(force NULL) fromKeep(preserve existing). The testclear_patch_actually_clears_previously_set_fieldat line 3248 validates this.One subtlety: the
lifecycle_statecolumn isNOT NULL DEFAULT 'active', but the INSERT usesCOALESCE(?2, 'active')which correctly handles a None lifecycle on insert. Theclear_lifecycleflag is hardcoded tofalse(line 1407), which is correct since lifecycle cannot be NULL.
1506-1543: Transaction correctly commits only after clearing the pointer.The
take_pending_resume_hydrationproperly:
- Opens a transaction
- Reads the pending snapshot ID
- Loads the snapshot (bailing with an error if missing, preserving the pointer for retry)
- Clears
pending_hydration_snapshot_id- Commits
However, when the snapshot is missing (line 1532), the transaction is committed before bailing. This means the pointer remains intact for retry, which matches the test expectation at line 2108. Good defensive design.
1450-1504: Token scoping usesIS ?3which handles NULL correctly for unscoped queries.The SQL
AND s.token_hash IS ?3at line 1471 correctly handles bothSome("token")(exact match) andNone(matches only rows wheretoken_hash IS NULL). This enforces proper session isolation.Test at line 2166 confirms unscoped queries return empty when sessions have tokens.
2054-2112: Good defensive test for missing snapshot retry semantics.The test
pending_hydration_missing_snapshot_keeps_pointer_for_retryverifies that when the referenced snapshot is deleted (simulating data corruption or race), the operation fails with an error message but the pointer remains for retry. This matches the caller's expectation inmemory_loader.rswhich propagates such errors with context.
3247-3301: Test coverage for Clear semantics addresses prior review feedback.The
clear_patch_actually_clears_previously_set_fieldtest properly:
- Sets a field via
SessionFieldPatch::Set- Verifies it's set
- Clears it via
SessionFieldPatch::Clear- Verifies it's
NoneThis validates the COALESCE bug fix.
clients/agent-runtime/src/agent/memory_loader.rs (3)
69-83: Correct ordering: recall before one-shot consumption.Calling
memory.recall()first ensures that if recall fails, the pending resume context isn't consumed. This prevents losing the one-shot hydration on transient failures.
174-217: Solid error handling and validation for resume hydration.The helper:
- Returns
Ok(false)for unsupported backends (line 184) — graceful degradation- Wraps errors with session context (line 186-188) — addresses prior review feedback
- Validates
SessionSnapshotKind::Compact(line 194-196) — type safety- Requires
resume_contextin payload (line 210) — fails explicitly if malformedThe formatted output is safe and doesn't include sensitive data.
571-599: Good test coverage for one-shot resume context consumption.The test
default_loader_prepends_persisted_resume_context_onceverifies:
- First call includes
[Resumed session context]- Second call does not (one-shot consumed)
This validates the atomic consumption behavior.
clients/agent-runtime/src/gateway/webhook_dispatch.rs (4)
28-32: Newcaller_token_hashfield enables session ownership validation.The addition of
caller_token_hash: Option<String>toWebhookTurnRequestallows the ingress layer to enforce session ownership during slash-command dispatch.
37-37: NewFailedoutcome distinguishes command failures from runtime errors.
WebhookTerminalOutcome::Failedis used specifically for slash-session command failures (e.g., unsupported backend), keeping it distinct fromErrorwhich indicates runtime/internal failures.
396-441: Correct ingress interception with proper outcome mapping.The
executefunction now:
- Calls
evaluate_ingresswithcaller_token_hashfor session-scoped authorization- Maps
SessionCommandsuccess/failure toCompleted/Failed— addresses past review feedback- Short-circuits provider execution for session commands (no agent/tool calls)
- Properly routes
Blockingoutcomes through existing machineryThe pattern match is exhaustive with
IngressDecision::Continue => {}allowing normal flow.
1052-1085: Good integration test for slash-command interception.The test
execute_intercepts_slash_session_commands_before_provider_executionverifies:
- Provider is never called (call count = 0)
- Outcome is
Failed(notCompletedorError)- Response text contains "require sqlite" (clear error messaging)
This validates the short-circuit behavior and proper error propagation.
clients/agent-runtime/src/pre_execution/mod.rs (5)
7-12: BlockingOutcome now carries structured reason.Adding
reason: StringtoApprovalRequiredenables downstream handlers to provide context-rich denial messages instead of just the tool name.
14-22: IngressDecision enum is well-structured for routing.The three variants cover all cases:
Continue: proceed with normal agent executionBlocking: pre-execution determined the request should be blockedSessionCommand: slash command handled without agent involvement
44-84: evaluate_ingress correctly threads caller_token_hash for session ownership.The function:
- Parses the prompt for slash commands
- Passes
caller_token_hashintoCommandContextfor ownership validation- Wraps dispatch errors into a structured
SessionCommandResultwithsuccess: false- Falls back to existing
evaluate + classify_blockingfor non-slash promptsThe error message is sourced from
error.message()which should contain user-safe text.
86-107: classify_blocking uses approval_reason with sensible default.When
approval_reasonisNone, a default message is generated from the tool name. This ensures consistent error messaging.
254-269: Tests verify slash command interception and pass-through behavior.
ingress_classifies_supported_slash_commands_before_pre_execution:/tldris intercepted (returnsSessionCommandwithsuccess: falsedue to non-SQLite backend)ingress_preserves_unknown_slash_like_input:/resume-lateris not a recognized command, so it passes through asContinueThis validates the parser's exact-match behavior and prevents false positives on slash-prefixed user messages.
clients/agent-runtime/src/session_commands/service.rs (9)
1-18: LGTM.Imports, constants, and struct definition are clean. The
'alifetime correctly ties the service to its borrowedMemoryreference.
20-74: LGTM.Solid state machine: validates session is active and not suspended, persists snapshot, and atomically updates state. Error paths are explicit.
76-135: LGTM.Creates a resume-capable snapshot and stores the context needed for later hydration. State patch correctly sets
latest_compact_snapshot_id.
137-176: LGTM.Proper invariant enforcement: suspension requires a resume-capable compact snapshot. The lifecycle transition to
Suspendedand timestamp recording are correct.
272-296: LGTM.Listing branch correctly passes the caller hash for ownership filtering and uses a reasonable limit.
299-409: LGTM.Helper methods are clean. The snapshot ownership check at lines 383-390 (validating
snapshot.session_id != session_id) is a good security guard against cross-session snapshot references. Error sanitization at line 402-403 logs only sanitized details.
412-455: LGTM.
truncate_previewcorrectly useschars()for Unicode-safe truncation. The ellipsis character…is a nice touch.
636-643: Consider:FakeMemory::list_resumable_sessionsignorescaller_token_hash.The fake returns
self.listedregardless of the caller filter. This is fine for unit-testing the service state machine, but integration tests should verify the real SQLite implementation filters by ownership. No change required here; just a note for test coverage strategy.
677-1026: LGTM.Comprehensive test coverage for success and failure paths. Good use of
Some("caller-hash")in resume tests to validate the authorization requirement.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 5 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
clients/agent-runtime/src/session_commands/service.rs (1)
206-214:⚠️ Potential issue | 🟠 MajorStop using page 1 of
list_resumable_sessionsas the visibility check.This still false-negatives once a caller has more than 1000 resumable sessions: a valid target outside that first page is rejected as
InvalidResumeTarget. Use a targetedsession_id + caller_hashvisibility lookup instead of a paginated membership scan.As per coding guidelines,
**/*: Security first, performance second. Validate input boundaries, auth/authz implications, and secret management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/session_commands/service.rs` around lines 206 - 214, The current visibility check uses memory.list_resumable_sessions with page-limited results which can false-negative when a caller has >1000 sessions; instead perform a targeted existence/visibility lookup using the target_session_id and caller_hash (e.g., call a memory method that fetches/queries by session_id with caller context like get_session/get_resumable_session or implement session_exists_for_caller(session_id, caller_hash)), map errors via self.map_storage_error as before, and use that boolean result in place of the paginated any(...) membership test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/gateway/mod.rs`:
- Around line 3919-3952: The tests (e.g.,
canonical_outcome_early_response_intercepts_slash_session_commands) use
MockMemory but the new slash-session behavior surfaces failures as the
sqlite-backed memory error contract; replace MockMemory with the sqlite-backed
memory implementation used elsewhere (the in-memory/sqlite helper used in other
tests) when constructing AppState and update the assertions for
canonical_outcome_early_response and the other listed tests (4524-4575,
6073-6123) to expect the new error contract: for JSON flows assert a 422
response with {"error":{"code":"session_command_failed"}} (and session_id where
applicable) and for streaming flows assert an event "error" plus the error
payload instead of the old 200/chunk+done success shape so the tests match
canonical_outcome_early_response behavior.
In `@clients/agent-runtime/src/session_commands/service.rs`:
- Around line 528-530: The fake backend helpers (e.g., get_session) currently
ignore session_id, caller_token_hash, limit, and offset; update get_session (and
the similar helpers at the other spots) to look up sessions keyed by the
incoming session_id instead of returning self.session unconditionally, enforce
caller filtering by validating caller_token_hash against the stored
SessionEntry/Caller info, and implement paging for list-style helpers by
applying limit and offset to the returned collection; reference the async
function get_session and the other session-listing helpers so you replace the
underscore-prefixed unused params with real lookups and filtering logic that
mirrors the real service contract.
- Around line 414-432: build_resume_context currently concatenates full
transcript entries and operator notes with no size limits, which can bloat
snapshots; modify build_resume_context to truncate each MemoryEntry.content to a
safe per-entry max (e.g., 1-2k chars) and enforce a global max total context
length (e.g., 8-16k chars) by trimming older or lower-priority entries until
under the cap, also truncate args/operator notes similarly, and ensure
truncation is UTF-8-safe; apply this bounded context before calling
create_session_snapshot so snapshots and resume prompts cannot exceed the
enforced limits.
In `@openspec/specs/sessions/spec.md`:
- Line 595: The file ends with an extra or missing trailing newline causing
markdownlint MD047; open the spec.md and ensure the final character after the
last table row containing "1.0.0 | 2026-03-28 | Initial specification from
session-memory-visibility change" is a single newline (i.e., the file terminates
with exactly one LF and no extra blank lines or missing newline).
---
Duplicate comments:
In `@clients/agent-runtime/src/session_commands/service.rs`:
- Around line 206-214: The current visibility check uses
memory.list_resumable_sessions with page-limited results which can
false-negative when a caller has >1000 sessions; instead perform a targeted
existence/visibility lookup using the target_session_id and caller_hash (e.g.,
call a memory method that fetches/queries by session_id with caller context like
get_session/get_resumable_session or implement
session_exists_for_caller(session_id, caller_hash)), map errors via
self.map_storage_error as before, and use that boolean result in place of the
paginated any(...) membership test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3e1df45-a7bd-4db2-97bf-42b283b32ded
📒 Files selected for processing (3)
clients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rsopenspec/specs/sessions/spec.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: report / Contributor Quality Report
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{md,mdx}
⚙️ CodeRabbit configuration file
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
For user-facing docs, check EN/ES parity or explicitly note pending translation gaps.
Files:
openspec/specs/sessions/spec.md
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
openspec/specs/sessions/spec.mdclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
clients/agent-runtime/src/{security,gateway,tools}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Treat
src/security/,src/gateway/,src/tools/as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Files:
clients/agent-runtime/src/gateway/mod.rs
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/gateway/mod.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
🧠 Learnings (5)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
Applied to files:
clients/agent-runtime/src/gateway/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
clients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
clients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Keep startup path lean and avoid heavy initialization in command parsing flow
Applied to files:
clients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/session_commands/service.rs
🪛 LanguageTool
openspec/specs/sessions/spec.md
[style] ~581-~581: The double modal “needs snapshot” is nonstandard (only accepted in certain dialects). Consider “to be snapshot”.
Context: ...te - WHEN /compact or /resume needs snapshot/state persistence operations - THEN the...
(NEEDS_FIXED)
🪛 markdownlint-cli2 (0.22.0)
openspec/specs/sessions/spec.md
[warning] 595-595: Files should end with a single newline character
(MD047, single-trailing-newline)
🔇 Additional comments (3)
openspec/specs/sessions/spec.md (2)
251-256: Good regression guard for suspended sessions.This explicitly prevents suspended sessions from being auto-closed or activity-mutated, which closes the lifecycle ambiguity and protects
/resumedeterminism.
283-288: Auth scope is now explicit for non-bearer ingress.The derived-scope/unsupported behavior here is clear and aligns with least-privilege visibility rules for
/resume.clients/agent-runtime/src/gateway/mod.rs (1)
668-697: No changes needed. TheLoopEventenum variant isToolDispatchStarted(String)and the gateway helpers at lines 672 and 687 correctly match this current variant name. The code is in sync with the enum definition.> Likely an incorrect or invalid review comment.
| | Version | Date | Changes | | ||
| |---------|------------|-------------------------------------------------------------| | ||
| | 1.0.0 | 2026-03-28 | Initial specification from session-memory-visibility change | | ||
| | 1.0.0 | 2026-03-28 | Initial specification from session-memory-visibility change | No newline at end of file |
There was a problem hiding this comment.
Fix trailing newline lint violation.
markdownlint reports MD047 at Line 595. Ensure the file ends with exactly one newline.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 595-595: Files should end with a single newline character
(MD047, single-trailing-newline)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/specs/sessions/spec.md` at line 595, The file ends with an extra or
missing trailing newline causing markdownlint MD047; open the spec.md and ensure
the final character after the last table row containing "1.0.0 | 2026-03-28 |
Initial specification from session-memory-visibility change" is a single newline
(i.e., the file terminates with exactly one LF and no extra blank lines or
missing newline).
- convert deploy-docs.yml to manual-only deployment - remove push/release triggers - add workflow_dispatch input for docs_ref (tag/branch/SHA) - checkout uses docs_ref to publish specific version - enable preview artifacts in docs-quality.yml - upload built dist as artifact on successful build - add step summary with download instructions This fixes the pages environment protection rule blocking tag deploys by separating stable publication (manual dispatch) from CI validation (preview artifacts for PR/main changes).
…vice.rs issues - gateway/mod.rs: replace MockMemory with SqliteMemory in slash-session tests - canonical_outcome_early_response_intercepts_slash_session_commands - legacy_webhook_preview_intercepts_slash_session_commands - web_chat_stream_returns_deterministic_slash_session_sse_without_provider_execution - update assertions to expect 422 + session_command_failed error - session_commands/service.rs: - FakeMemory::get_session now looks up by session_id - build_resume_context adds truncation limits (2k per entry, 16k global) - visibility check uses get_session instead of paginated list - openspec/specs/sessions/spec.md: add trailing newline
* fix(test): update slash-session tests to use SqliteMemory and fix service.rs issues - gateway/mod.rs: replace MockMemory with SqliteMemory in slash-session tests - canonical_outcome_early_response_intercepts_slash_session_commands - legacy_webhook_preview_intercepts_slash_session_commands - web_chat_stream_returns_deterministic_slash_session_sse_without_provider_execution - update assertions to expect 422 + session_command_failed error - session_commands/service.rs: - FakeMemory::get_session now looks up by session_id - build_resume_context adds truncation limits (2k per entry, 16k global) - visibility check uses get_session instead of paginated list - openspec/specs/sessions/spec.md: add trailing newline * feat(agent-runtime): add slash command registry core * test(agent-runtime): fix slash registry alias dispatch coverage * test(registry): add test for alias resolution in slash command registry * fix: address review feedback for slash command registry core - parser: fix split_primary_target to return args after target - registry: strengthen validate_name to require len >= 2 and lowercase second char - registry: detect canonical vs alias conflicts and duplicate aliases in same descriptor - service: fix context truncation to trim from end preserving header - service: remove redundant get_session call in handle_resume - gateway: handle SessionCommand after session upsert for session-dependent commands - docs: fix 'prompt side' -> 'prompt side-effects' in proposal - docs: fix verify-report.md to use H1 instead of H2 * style: rustfmt fixes for registry and gateway
# Conflicts: # clients/agent-runtime/src/gateway/mod.rs # clients/agent-runtime/src/session_commands/service.rs
* fix(test): update slash-session tests to use SqliteMemory and fix service.rs issues - gateway/mod.rs: replace MockMemory with SqliteMemory in slash-session tests - canonical_outcome_early_response_intercepts_slash_session_commands - legacy_webhook_preview_intercepts_slash_session_commands - web_chat_stream_returns_deterministic_slash_session_sse_without_provider_execution - update assertions to expect 422 + session_command_failed error - session_commands/service.rs: - FakeMemory::get_session now looks up by session_id - build_resume_context adds truncation limits (2k per entry, 16k global) - visibility check uses get_session instead of paginated list - openspec/specs/sessions/spec.md: add trailing newline * feat(agent-runtime): add typed slash command contract * test(agent-runtime): fix stream slash command regression expectations * feat(agent-runtime): enhance slash command handling with execution mode support * fix(gateway): format webhook_dispatch.rs for pre-push hook
) * feat(agent-runtime): unify slash command transport handling (#541) * fix: Return 200 for blocking outcomes, add SSE tests Change pre-execution blocking responses to use HTTP 200 for SSE endpoints instead of 403/408 so the server-sent events contract is preserved. Add two tokio tests verifying the /web/chat/stream SSE output for approval_required and timeout blocking outcomes (checks event: error, expected error codes/messages, and that the provider is not executed). These tests enable webhook_dispatcher in the temp config to exercise the webhook dispatcher code path. * fix(agent-runtime): preserve slash failure transport contracts (#541) * test(agent-runtime): align preview slash error code expectations
…on-tests test(agent-runtime): harden slash command regressions (#543)
…ch in minor protection
fix: rook 583 apply
Fixed 11 file(s) based on 16 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
feat(rook): add SQLite persistence for ProviderAccount, ProviderPool, ModelRoute, and RoutingPolicy
…onality and update timeout durations
# Conflicts: # clients/rook/Cargo.lock # clients/rook/Cargo.toml # clients/rook/migrations/0001_initial.sql # clients/rook/src/db/account.rs # clients/rook/src/db/mod.rs # clients/rook/src/db/pool.rs # clients/rook/src/db/route.rs # clients/rook/src/services/account.rs # clients/rook/src/services/health.rs # clients/rook/src/services/pool.rs # clients/rook/src/services/route.rs
…tings - Add SqliteAccountService, SqlitePoolService, SqliteRouteService wired to the correct db layer methods (insert_*, get_*, delete_*); update implemented as delete + insert since the db layer has no update_* ops - Add SettingsService trait with InMemorySettingsService and SqliteSettingsService backed by a single-row upsert pattern - Add db/settings.rs with load_settings / save_settings helpers - Add migration 0002_settings.sql creating the settings table - Add RookRegistry using concrete types (not dyn Trait) to stay compatible with RPITIT async traits; exposes typed accessors for all four services - Expand domain/mod.rs with RookSettings and a sensible Default Close #584
feat(rook): build registry services for account, pool, route, and settings
- coordinator.rs: swap string-based test for exhaustive compile-time match - coordinator.rs: add Some(_) catch-all for future AgentExecutionError variants - log.rs: replace invalid Duration::from_mins(2) with from_secs(120) - delegate.rs: rename test to reflect schema-only validation - chatOnboardingContract.spec.ts: inline roadmap fixture, use regex assertions - vite.config.js: narrow server.fs.allow to specific paths - vite.config.ts: make fs.allow conditional on test mode - verify-report.md: change H2 to H1 heading - spec.md: add non-normative note on implicit kind/sender/recipient
…foundations feat(coordinator): complete track 4 slice 1 foundations
… unused settings module
…ack chains - Add RoutingEngine with resolve(), select_from_pool(), try_fallback() - Support Priority, Failover, RoundRobin, and Weighted (SWRR) strategies - Handle recursive async fallback chains via Box::pin - Track RR cursor and SWRR counters in Arc<Mutex<HashMap<PoolId, ...>>> - Expose SqliteDb from RookRegistry in test cfg for adversarial FK scenarios - Fix select_failover to order by priority instead of assuming insert order - All 14 routing tests pass, 0 warnings Close #586
- weighted_state: change from Vec<i64> (by index) to HashMap<AccountId, i64> so state persists even when candidate order changes - select_* fns: return Result<Account, RookError> and handle PoisonError instead of panicking on lock poisoning - cycle_detection test: require "cycle" explicitly, not accept "exhausted" - weighted_distribution test: tighter bounds (heavy: 24-32, light: 1-16) to catch regressions Close #586
- Restore routing module with AccountId-keyed weighted_state - Restore health service with Clone derive - Remove synchronous .await from health calls - Reset to working state after merge conflict resolution
Block whitespace-only scope/topic updates and add a regression test for mem_update.
fix(cerebro): reject blank memory update fields
- Remove redundant let idx = idx in select_round_robin - Replace expect with ok_or in select_weighted to propagate errors - Use single connection for PRAGMA FK in test (acquire()) - Add visited_pools HashSet to resolve_pool for cycle detection - Add tracing::warn when missing account in pool_members - Return error when pool has members but none found - Pass pre-fetched pool to try_fallback to avoid re-fetch - Import ProviderPool for try_fallback signature
feat(rook): implement routing engine with strategy dispatch and fallback chains
… (Slice 2) Add SupervisedOrchestrationService with full child agent lifecycle management: - DelegateLaunchTool: launch child agents with resource limits and isolation - DelegateInspectTool: query child agent status and OrchestrationHandle state - DelegateCancelTool: gracefully cancel or force-kill child agents - Coordinator integration: route delegate_* tools through supervised service - Bootstrap: classify new delegation tools in agent profile system Covers 38/38 spec scenarios, 3691 tests passing, zero clippy warnings. Closes #525 Slice 2.
|
This pull request introduces significant enhancements and refactoring to the session memory management and message handling in the agent runtime, focusing on supporting resumable session context, improving error handling for unsupported backends, and refining channel message processing. The key changes include new session state and snapshot types, robust error signaling for unsupported memory backends, and improved logic for resuming session context in the memory loader.
Session memory and snapshot support:
corvus-traits/src/memory.rsfor session lifecycle (SlashSessionLifecycle), snapshot kinds (SessionSnapshotKind), session state and snapshot records, and patch/mutation structures to enable fine-grained session management and resuming capabilities.ResumableSessionEntrystruct to represent resumable sessions, supporting richer session state tracking.Error handling for unsupported backends:
SlashSessionUnsupportedBackendError) and helper functions to clearly signal when slash-session commands are used with a non-SQLite backend, and provided default trait method implementations that return this error for unsupported operations. [1] [2]Memory loader improvements:
DefaultMemoryLoaderandCerebroMemoryLoaderto call recall before attempting to append pending resume context, ensuring one-shot resume context is only consumed if recall succeeds. Added theappend_pending_resume_contextasync helper to format and prepend resumed session context from a snapshot, with robust error handling for unsupported backends. [1] [2] [3]Channel message processing and session ID logic:
channels/mod.rsto include the sender and reply target, ensuring unique session IDs for different conversations/threads.Test and dependency updates:
NoneMemoryin channel tests. [1] [2]These changes collectively improve the robustness, clarity, and extensibility of session management and message handling in the agent runtime, especially around session resumption and error signaling for unsupported configurations.