feat: add session search and trajectory artifact CLIs#905
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a session transcript search tool and trajectory export/inspect CLIs: repo-level visible-session turn search, an app tool Changes
Sequence Diagram(s)sequenceDiagram
participant Client as CLI Client
participant Daemon as Daemon CLI
participant ToolRuntime as App Tool Runtime
participant Repo as SessionRepository
participant DB as SQLite DB
participant Artifact as Artifact Writer
Client->>Daemon: run session-search (query, limit, session)
Daemon->>ToolRuntime: execute session_search tool (ToolCoreRequest)
ToolRuntime->>Repo: search_visible_session_turns(session_id, query, limit, include_archived, include_descendants)
Repo->>DB: SQL recursive CTE + turn content filter
DB-->>Repo: rows (turn hits)
Repo-->>ToolRuntime: Vec<SessionSearchHit> (with session summaries)
ToolRuntime->>ToolRuntime: render snippet & add metadata per hit
ToolRuntime-->>Daemon: JSON artifact payload (returned_count, hits)
Daemon->>Artifact: persist JSON artifact (tmp write + fsync + rename)
Artifact-->>Daemon: artifact path
Daemon-->>Client: formatted output / JSON
sequenceDiagram
participant Client as CLI Client
participant Daemon as Daemon CLI
participant Config as Config Loader
participant Repo as SessionRepository
participant DB as SQLite DB
participant Artifact as Artifact Writer
Client->>Daemon: run trajectory-export (session, output)
Daemon->>Config: load config & resolve session id
Daemon->>Repo: load session summary
Repo->>DB: query session summary
DB-->>Repo: SessionSummaryRecord
Daemon->>Repo: paginate turns (windowed loads)
loop paginate
Repo->>DB: SELECT turns LIMIT 200 AFTER cursor
DB-->>Repo: turn page
end
Daemon->>Repo: paginate events (windowed loads)
loop paginate events
Repo->>DB: SELECT events LIMIT 200 AFTER cursor
DB-->>Repo: event page
end
Daemon->>Artifact: persist trajectory JSON artifact
Artifact-->>Daemon: artifact path
Daemon-->>Client: formatted output / JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
dependencies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
crates/daemon/tests/integration/cli_tests.rs (1)
283-470: Extract a shared trajectory artifact fixture to reduce duplication.The same
TrajectoryExportArtifactDocumentpayload is repeated across multiple tests, which will make schema updates noisy.♻️ Suggested refactor
+fn sample_trajectory_artifact() -> TrajectoryExportArtifactDocument { + TrajectoryExportArtifactDocument { + schema: TrajectoryExportArtifactSchema { + version: TRAJECTORY_EXPORT_ARTIFACT_JSON_SCHEMA_VERSION, + surface: "trajectory_export".to_owned(), + purpose: "session_replay_evidence".to_owned(), + }, + exported_at: "2026-04-04T00:00:00Z".to_owned(), + config: "/tmp/loongclaw.toml".to_owned(), + session: TrajectoryExportSessionSummary { + session_id: "root-session".to_owned(), + kind: "root".to_owned(), + parent_session_id: None, + label: Some("Root".to_owned()), + state: "completed".to_owned(), + created_at: 1, + updated_at: 2, + archived_at: None, + turn_count: 2, + last_turn_at: Some(2), + last_error: None, + }, + turns: vec![ + TrajectoryExportTurn { role: "user".to_owned(), content: "hello".to_owned(), ts: 1 }, + TrajectoryExportTurn { role: "assistant".to_owned(), content: "world".to_owned(), ts: 2 }, + ], + events: vec![TrajectoryExportEvent { + id: 7, + session_id: "root-session".to_owned(), + event_kind: "delegate_started".to_owned(), + actor_session_id: Some("root-session".to_owned()), + payload_json: json!({"mode": "async"}), + ts: 2, + }], + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon/tests/integration/cli_tests.rs` around lines 283 - 470, Multiple tests repeat the same TrajectoryExportArtifactDocument payload; extract a shared fixture function (e.g., fn sample_trajectory_artifact() -> TrajectoryExportArtifactDocument) that returns the common document used by format_trajectory_export_text and format_trajectory_inspect_text tests, then replace the inline constructions in tests like format_trajectory_export_text, format_trajectory_inspect_text, format_trajectory_inspect_text_summarizes_roles_and_events with calls to that fixture and adjust only the per-test fields (artifact/output path) locally; locate the construction of TrajectoryExportArtifactDocument, TrajectoryExportArtifactSchema, TrajectoryExportSessionSummary, TrajectoryExportTurn, and TrajectoryExportEvent in the tests to move into the fixture and ensure TRAJECTORY_EXPORT_ARTIFACT_JSON_SCHEMA_VERSION and exported_at/config values are set in the fixture (override in tests if needed).crates/app/src/tools/mod.rs (1)
651-703: Log the pre-dispatch validation failures as well.Line 654 and Line 667 can still return before the new
warn!/debug!block is reached, so reserved_loongclawpayloads and malformed trusted runtime narrowing remain invisible in this execution log path. Wrapping the full function body in the timed/logged result would make this observability surface complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/src/tools/mod.rs` around lines 651 - 703, The pre-dispatch checks can return early without logging; before returning in the reserved-internal-context check (where payload_uses_reserved_internal_tool_context is tested) and before propagating Err from trusted_runtime_narrowing_from_payload, emit the same tracing::warn (target "loongclaw.tools") used for post-dispatch failures including requested_tool_name, canonical_tool_name (compute via canonical_tool_name(request.tool_name.as_str() or use requested_tool_name if canonical not yet computed), payload_kind, payload_keys, error = %crate::observability::summarize_error(...), and a short message like "tool pre-dispatch validation failed", then return the Err as before; this ensures failures from payload_uses_reserved_internal_tool_context and trusted_runtime_narrowing_from_payload are visible in the logs while preserving existing control flow involving trusted_internal_tool_payload_enabled, trusted_runtime_narrowing_from_payload, ToolCoreRequest and result handling.crates/app/src/acp/manager.rs (2)
194-195: Use resolved session key for turn logs to prevent correlation drift.
bootstrap.session_keycan differ from the actual reused session key (binding/conversation reuse paths). Prefer resolved keys (metadata.session_keyat start,handle.session_keyon completion/failure) so one runtime session always logs under one key.Suggested patch
- session_key = %bootstrap.session_key, + session_key = %metadata.session_key, @@ - session_key = %bootstrap.session_key, + session_key = %handle.session_key, @@ - session_key = %bootstrap.session_key, + session_key = %handle.session_key,Also applies to: 259-260, 280-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/src/acp/manager.rs` around lines 194 - 195, The turn logs currently use bootstrap.session_key which may differ from the resolved runtime key; update logging to use the resolved session key fields instead: replace uses of bootstrap.session_key with metadata.session_key at the start of a turn and with handle.session_key when logging completion/failure so the same runtime session consistently logs under one session key (adjust the log fields where session_key = %bootstrap.session_key to session_key = %metadata.session_key for start and to session_key = %handle.session_key for completion/failure).
182-183: Clarify latency scope in logs vs aggregated stats.
started_atincludes queueing/setup, whilerecord_turn_completionusesturn_started_ms(execution-focused). Consider renaming log field toend_to_end_duration_ms(or aligning start points) to avoid metric interpretation drift.Also applies to: 249-250, 283-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/src/acp/manager.rs` around lines 182 - 183, The log currently uses started_at (Instant::now() which includes queueing/setup) while record_turn_completion uses turn_started_ms (execution-only), causing metric drift; update the logging to either (1) rename the duration field emitted from Instant::now() to end_to_end_duration_ms everywhere you set started_at (e.g., in the code paths that set started_at at the top of the request/turn) or (2) align the log to use the execution-only start by computing duration against turn_started_ms for the same log entries; modify occurrences that set started_at (the variable named started_at) and the corresponding log field(s) (seen near actor_key_for_bootstrap and the other similar locations) and ensure record_turn_completion and the emitted JSON field names match the chosen scope (use end_to_end_duration_ms if keeping started_at or reuse turn_started_ms-derived duration if aligning).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/app/src/session/repository.rs`:
- Around line 765-845: The visible CTE seeds reachable session IDs only from the
sessions table, so legacy-only sessions that exist only in turns are excluded;
update the visible CTE seed to include session IDs coming from turns for the
root parameter (?1) as well (e.g., add "UNION SELECT session_id FROM turns WHERE
session_id = ?1" or include a combined SELECT that pulls session_id from
sessions and from turns) so the recursive CTE (visible) will find legacy-only
sessions and allow visible_sessions and hits to include them.
In `@crates/app/src/tools/session.rs`:
- Around line 2658-2684: session_search_snippet currently allocates fixed
prefix/suffix budgets before accounting for the matched query, allowing long
queries to exceed max_chars; change the logic to budget the match span first by
computing the match length (use the matched slice content[start..end] or its
char count), subtract that from max_chars to get remaining budget, split that
remaining budget between prefix and suffix (e.g., third/two-thirds or other
policy), then call trim_chars_from_end(&content[..start], prefix_budget) and
trim_chars_from_start(&content[end..], suffix_budget); finally, assemble prefix
+ match + suffix and if necessary clamp the assembled snippet to max_chars and
add "..." where you trimmed using the existing trim helpers (keep function name
session_search_snippet and reuse trim_chars_from_end/trim_chars_from_start).
In `@crates/daemon/src/main.rs`:
- Line 42: The current debug line logs the entire cli.command struct (including
user-provided content) and runs before default resolution; change it to log only
the resolved command variant name after calling
unwrap_or_else(resolve_default_entry_command) so sensitive fields (Ask::message,
SessionSearch::query, RunTask::objective, Migrate::input/output, etc.) are not
emitted. Concretely, move the tracing::debug call to after you call
unwrap_or_else(resolve_default_entry_command) (or wherever you finalize the
command) and replace the `command = ?cli.command` field with a sanitized string
containing only the Commands variant name (e.g., derived from the enum variant)
or remove the field entirely if you prefer no command logging.
In `@crates/daemon/src/trajectory_cli.rs`:
- Around line 51-58: The TrajectoryExportArtifactDocument currently embeds a
machine-specific path via its pub config: String field; remove that field from
the struct definition (TrajectoryExportArtifactDocument) and any code that
serializes/deserializes it so the persisted JSON no longer contains host-local
paths, and instead pass the resolved CLI-local path only to the text-only
routine (format_trajectory_export_text) that needs it; update callers that
constructed TrajectoryExportArtifactDocument to stop supplying config and adjust
format_trajectory_export_text calls to accept the resolved path separately (and
remove any references to config when writing the artifact JSON), and mirror the
same removal at the other occurrence noted (around the second instance of the
struct usage).
In `@crates/daemon/tests/integration/trajectory_export_cli.rs`:
- Around line 16-22: The unique_temp_dir function currently builds a path using
only SystemTime::now().as_nanos(), which can collide under coarse clocks and
parallel tests; update unique_temp_dir to include additional entropy such as the
current process id and a random or UUID component (e.g., pid() and a
rand::random::<u64>() or uuid::Uuid::new_v4()) or, alternatively, use
tempfile::Builder::new().prefix(prefix).tempdir_in(std::env::temp_dir()) to
create a guaranteed-unique directory; modify the function signature/return to
still yield a PathBuf, add the necessary imports (std::process::id, rand or uuid
or tempfile), and ensure the directory name joins prefix, nanos, pid, and
random/uuid to harden uniqueness for parallel test runs.
---
Nitpick comments:
In `@crates/app/src/acp/manager.rs`:
- Around line 194-195: The turn logs currently use bootstrap.session_key which
may differ from the resolved runtime key; update logging to use the resolved
session key fields instead: replace uses of bootstrap.session_key with
metadata.session_key at the start of a turn and with handle.session_key when
logging completion/failure so the same runtime session consistently logs under
one session key (adjust the log fields where session_key =
%bootstrap.session_key to session_key = %metadata.session_key for start and to
session_key = %handle.session_key for completion/failure).
- Around line 182-183: The log currently uses started_at (Instant::now() which
includes queueing/setup) while record_turn_completion uses turn_started_ms
(execution-only), causing metric drift; update the logging to either (1) rename
the duration field emitted from Instant::now() to end_to_end_duration_ms
everywhere you set started_at (e.g., in the code paths that set started_at at
the top of the request/turn) or (2) align the log to use the execution-only
start by computing duration against turn_started_ms for the same log entries;
modify occurrences that set started_at (the variable named started_at) and the
corresponding log field(s) (seen near actor_key_for_bootstrap and the other
similar locations) and ensure record_turn_completion and the emitted JSON field
names match the chosen scope (use end_to_end_duration_ms if keeping started_at
or reuse turn_started_ms-derived duration if aligning).
In `@crates/app/src/tools/mod.rs`:
- Around line 651-703: The pre-dispatch checks can return early without logging;
before returning in the reserved-internal-context check (where
payload_uses_reserved_internal_tool_context is tested) and before propagating
Err from trusted_runtime_narrowing_from_payload, emit the same tracing::warn
(target "loongclaw.tools") used for post-dispatch failures including
requested_tool_name, canonical_tool_name (compute via
canonical_tool_name(request.tool_name.as_str() or use requested_tool_name if
canonical not yet computed), payload_kind, payload_keys, error =
%crate::observability::summarize_error(...), and a short message like "tool
pre-dispatch validation failed", then return the Err as before; this ensures
failures from payload_uses_reserved_internal_tool_context and
trusted_runtime_narrowing_from_payload are visible in the logs while preserving
existing control flow involving trusted_internal_tool_payload_enabled,
trusted_runtime_narrowing_from_payload, ToolCoreRequest and result handling.
In `@crates/daemon/tests/integration/cli_tests.rs`:
- Around line 283-470: Multiple tests repeat the same
TrajectoryExportArtifactDocument payload; extract a shared fixture function
(e.g., fn sample_trajectory_artifact() -> TrajectoryExportArtifactDocument) that
returns the common document used by format_trajectory_export_text and
format_trajectory_inspect_text tests, then replace the inline constructions in
tests like format_trajectory_export_text, format_trajectory_inspect_text,
format_trajectory_inspect_text_summarizes_roles_and_events with calls to that
fixture and adjust only the per-test fields (artifact/output path) locally;
locate the construction of TrajectoryExportArtifactDocument,
TrajectoryExportArtifactSchema, TrajectoryExportSessionSummary,
TrajectoryExportTurn, and TrajectoryExportEvent in the tests to move into the
fixture and ensure TRAJECTORY_EXPORT_ARTIFACT_JSON_SCHEMA_VERSION and
exported_at/config values are set in the fixture (override in tests if needed).
🪄 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: CHILL
Plan: Pro
Run ID: 1327d8f3-6d60-4578-95ff-7c979d2c36fc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlcrates/app/Cargo.tomlcrates/app/src/acp/manager.rscrates/app/src/channel/mod.rscrates/app/src/lib.rscrates/app/src/observability.rscrates/app/src/provider/request_failover_runtime.rscrates/app/src/provider/request_session_runtime.rscrates/app/src/provider/runtime_binding.rscrates/app/src/session/repository.rscrates/app/src/tools/catalog.rscrates/app/src/tools/mod.rscrates/app/src/tools/session.rscrates/daemon/Cargo.tomlcrates/daemon/src/lib.rscrates/daemon/src/main.rscrates/daemon/src/observability.rscrates/daemon/src/trajectory_cli.rscrates/daemon/tests/integration/cli_tests.rscrates/daemon/tests/integration/mod.rscrates/daemon/tests/integration/trajectory_export_cli.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/daemon/src/lib.rs (1)
2861-2885:⚠️ Potential issue | 🟠 MajorUse atomic replace semantics for shared artifact writes.
This helper now sits on the write path for
session-searchandtrajectory-export, but it still writes in place withfs::write. A partial write or interrupted process can leave a truncated JSON artifact on disk, which is exactly the kind of malformed evidence file this PR is trying to avoid. Please switch the shared writer to a same-directory temp-file + replace flow before expanding its reuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon/src/lib.rs` around lines 2861 - 2885, persist_runtime_snapshot_artifact currently writes in-place and can leave truncated JSON on failure; change it to write to a same-directory temporary file and then atomically replace the target. Specifically, in persist_runtime_snapshot_artifact create the parent dir as you do now, then open a temp file in the same parent directory (e.g., parent.join(format!(".{}.tmp", output_path.file_name().unwrap().to_string_lossy())) or use a NamedTempFile in the same dir), write the serde_json::to_string_pretty(payload) bytes to that temp file using write_all, flush and call sync_all on the File to ensure data hits disk, close it, and then call std::fs::rename(temp_path, &output_path) to atomically replace the artifact; convert any IO/serialization errors to the same CliResult error messages as before.
🧹 Nitpick comments (1)
crates/daemon/tests/integration/session_search_cli.rs (1)
37-105: Makeinclude_archived = falseobservable in this fixture.Right now that flag is effectively inert because no visible session is archived. Adding an archived child session here, and asserting it is excluded with
falseand included withtrue, would cover the regression this integration test is meant to guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon/tests/integration/session_search_cli.rs` around lines 37 - 105, The fixture currently never archives any session so include_archived=false is not exercised; update the test collect_session_search_artifact_includes_visible_hits to create an additional session (e.g., "archived-child") as a child of "root-session" using repo.create_session with state set to mvp::session::repository::SessionState::Archived, append a turn containing the same search text (use mvp::memory::append_turn_direct for "archived-child"), then call collect_session_search_artifact twice (once with include_archived = false and once with include_archived = true) and add assertions that the artifact for the first call excludes hits from "archived-child" while the second includes them so the flag behavior is tested; reference the test function name and the collect_session_search_artifact invocation to locate where to add the archived session and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/daemon/src/session_cli.rs`:
- Around line 137-160: The current code silently defaults missing/malformed
top-level fields (visibility, returned_count, hits) which masks contract drift;
change the parsing to validate presence and types and return an error instead of
defaulting: require
payload.get("filters").and_then(Value::as_object).and_then(|f|
f.get("visibility")).and_then(Value::as_str) to exist (no unwrap_or), require
payload.get("returned_count").and_then(Value::as_u64) to exist, and require
payload.get("hits").and_then(Value::as_array) to exist; if any of these are
absent or wrong-typed return an Err with a clear message, and only then iterate
over hits_value and call parse_session_search_hit(&hit_value)? to build hits
(keeping parse_session_search_hit for per-item validation).
---
Outside diff comments:
In `@crates/daemon/src/lib.rs`:
- Around line 2861-2885: persist_runtime_snapshot_artifact currently writes
in-place and can leave truncated JSON on failure; change it to write to a
same-directory temporary file and then atomically replace the target.
Specifically, in persist_runtime_snapshot_artifact create the parent dir as you
do now, then open a temp file in the same parent directory (e.g.,
parent.join(format!(".{}.tmp",
output_path.file_name().unwrap().to_string_lossy())) or use a NamedTempFile in
the same dir), write the serde_json::to_string_pretty(payload) bytes to that
temp file using write_all, flush and call sync_all on the File to ensure data
hits disk, close it, and then call std::fs::rename(temp_path, &output_path) to
atomically replace the artifact; convert any IO/serialization errors to the same
CliResult error messages as before.
---
Nitpick comments:
In `@crates/daemon/tests/integration/session_search_cli.rs`:
- Around line 37-105: The fixture currently never archives any session so
include_archived=false is not exercised; update the test
collect_session_search_artifact_includes_visible_hits to create an additional
session (e.g., "archived-child") as a child of "root-session" using
repo.create_session with state set to
mvp::session::repository::SessionState::Archived, append a turn containing the
same search text (use mvp::memory::append_turn_direct for "archived-child"),
then call collect_session_search_artifact twice (once with include_archived =
false and once with include_archived = true) and add assertions that the
artifact for the first call excludes hits from "archived-child" while the second
includes them so the flag behavior is tested; reference the test function name
and the collect_session_search_artifact invocation to locate where to add the
archived session and assertions.
🪄 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: CHILL
Plan: Pro
Run ID: 1b7222b2-91db-4bc6-93b3-e4e68cf54595
📒 Files selected for processing (6)
crates/daemon/src/lib.rscrates/daemon/src/main.rscrates/daemon/src/session_cli.rscrates/daemon/tests/integration/cli_tests.rscrates/daemon/tests/integration/mod.rscrates/daemon/tests/integration/session_search_cli.rs
✅ Files skipped from review due to trivial changes (1)
- crates/daemon/tests/integration/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/daemon/src/main.rs
- crates/daemon/tests/integration/cli_tests.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/daemon/src/lib.rs (1)
2870-2893:⚠️ Potential issue | 🟠 MajorMake shared artifact writes atomic.
This helper now backs multiple evidence artifacts, but
fs::writetruncates the destination before the new JSON is fully persisted. A crash or short write can leave a malformed or empty artifact where a valid one used to be. Please write to a temp file in the same directory and rename it into place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon/src/lib.rs` around lines 2870 - 2893, persist_runtime_snapshot_artifact currently uses fs::write which can truncate the target and leave a corrupt artifact; change it to write atomically by creating a temp file in the same directory (use output_path.parent() to locate dir), write the serialized `encoded` bytes to that temp file with write_all, call file.sync_all() (and optionally sync_all on the parent directory) to flush to disk, then atomically rename the temp file into place with fs::rename; keep the existing parent directory creation logic and map all IO/serialization errors to CliResult as the current function does.
🧹 Nitpick comments (3)
crates/daemon/tests/integration/session_search_cli.rs (1)
162-168: Exercise the actual session-search write path here.This round-trip test serializes the struct directly, so it never covers
run_session_search_cli(..., output_path)or the shared artifact writer. A persistence regression can still leave this test green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon/tests/integration/session_search_cli.rs` around lines 162 - 168, The test currently writes the artifact by serializing and fs::write directly, so it doesn't exercise the CLI persistence path; update the test to call the actual session-search CLI persistence path instead (e.g., invoke run_session_search_cli or the shared artifact writer used by production) with the same artifact_path so the code under test performs the round-trip; locate the test's direct write block around artifact_path / encoded and replace the fs::write/serde_json::to_string_pretty usage with a call to run_session_search_cli(..., &artifact_path) or the shared writer function (the function name run_session_search_cli and the shared artifact writer should be used to find the correct helper) ensuring the test still asserts the same resulting file contents.crates/daemon/tests/integration/cli_tests.rs (2)
510-562: Potentially redundant test — consider consolidating or differentiating.This test (
format_trajectory_inspect_text_summarizes_roles_and_events) has identical test data and largely overlapping assertions with the previous test (format_trajectory_inspect_text_summarizes_counts). Both verifyfirst_turn_role,last_turn_role, andlatest_event_kind.If the intent was to test edge cases (e.g., empty turns, empty events, single turn), consider modifying this test to use different input data. Otherwise, consider removing the duplicate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon/tests/integration/cli_tests.rs` around lines 510 - 562, The test format_trajectory_inspect_text_summarizes_roles_and_events duplicates assertions/data from format_trajectory_inspect_text_summarizes_counts; either delete this redundant test or change its input to exercise a distinct edge case (e.g., no turns, single turn, or no events) and update assertions accordingly. Locate the test function name format_trajectory_inspect_text_summarizes_roles_and_events and modify the TrajectoryExportArtifactDocument supplied to format_trajectory_inspect_text so the turns/events differ (for example remove the assistant turn to assert last_turn_role is missing or set events to empty to assert latest_event_kind is absent), then adjust the assert! checks to match the new expected output.
297-345: Consider extracting shared test fixture construction.The
SessionSearchArtifactDocumentconstruction (lines 301-337) is nearly identical to the previous test (lines 250-286). A helper function could reduce duplication and make test maintenance easier.♻️ Example helper function
fn sample_session_search_artifact() -> SessionSearchArtifactDocument { SessionSearchArtifactDocument { schema: SessionSearchArtifactSchema { version: SESSION_SEARCH_ARTIFACT_JSON_SCHEMA_VERSION, surface: "session_search".to_owned(), purpose: "session_recall_evidence".to_owned(), }, exported_at: "2026-04-05T00:00:00Z".to_owned(), config: "/tmp/loongclaw.toml".to_owned(), scope_session_id: "root-session".to_owned(), query: "deploy freeze".to_owned(), limit: 5, include_archived: false, visibility: "children".to_owned(), returned_count: 1, hits: vec![SessionSearchArtifactHit { // ... hit fields }], } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon/tests/integration/cli_tests.rs` around lines 297 - 345, The two tests duplicate construction of a SessionSearchArtifactDocument (and its SessionSearchArtifactHit); extract that shared fixture into a helper function like sample_session_search_artifact() returning SessionSearchArtifactDocument and use it from both tests to reduce duplication; update the tests to call sample_session_search_artifact() and modify only the fields that differ (if any) before passing into format_session_search_inspect_text, keeping the unique symbols SessionSearchArtifactDocument and SessionSearchArtifactHit to locate the structs to build the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/daemon/src/session_cli.rs`:
- Around line 371-407: load_session_search_artifact currently only validates
schema fields; after decoding the SessionSearchArtifactDocument (in
load_session_search_artifact, immediately after successfully creating artifact)
add consistency checks to reject tampered artifacts: verify
artifact.result.returned_count equals artifact.result.hits.len(), ensure all
entries in artifact.result.hits have allowed visibility values (e.g., check
artifact.result.hits[*].visibility against the expected enum/strings), and
validate any other invariants (non-negative counts, unique IDs, etc.); on
failure return Err with a clear message referencing the artifact path and the
violated invariant so callers can reject internally inconsistent search
artifacts.
---
Outside diff comments:
In `@crates/daemon/src/lib.rs`:
- Around line 2870-2893: persist_runtime_snapshot_artifact currently uses
fs::write which can truncate the target and leave a corrupt artifact; change it
to write atomically by creating a temp file in the same directory (use
output_path.parent() to locate dir), write the serialized `encoded` bytes to
that temp file with write_all, call file.sync_all() (and optionally sync_all on
the parent directory) to flush to disk, then atomically rename the temp file
into place with fs::rename; keep the existing parent directory creation logic
and map all IO/serialization errors to CliResult as the current function does.
---
Nitpick comments:
In `@crates/daemon/tests/integration/cli_tests.rs`:
- Around line 510-562: The test
format_trajectory_inspect_text_summarizes_roles_and_events duplicates
assertions/data from format_trajectory_inspect_text_summarizes_counts; either
delete this redundant test or change its input to exercise a distinct edge case
(e.g., no turns, single turn, or no events) and update assertions accordingly.
Locate the test function name
format_trajectory_inspect_text_summarizes_roles_and_events and modify the
TrajectoryExportArtifactDocument supplied to format_trajectory_inspect_text so
the turns/events differ (for example remove the assistant turn to assert
last_turn_role is missing or set events to empty to assert latest_event_kind is
absent), then adjust the assert! checks to match the new expected output.
- Around line 297-345: The two tests duplicate construction of a
SessionSearchArtifactDocument (and its SessionSearchArtifactHit); extract that
shared fixture into a helper function like sample_session_search_artifact()
returning SessionSearchArtifactDocument and use it from both tests to reduce
duplication; update the tests to call sample_session_search_artifact() and
modify only the fields that differ (if any) before passing into
format_session_search_inspect_text, keeping the unique symbols
SessionSearchArtifactDocument and SessionSearchArtifactHit to locate the structs
to build the helper.
In `@crates/daemon/tests/integration/session_search_cli.rs`:
- Around line 162-168: The test currently writes the artifact by serializing and
fs::write directly, so it doesn't exercise the CLI persistence path; update the
test to call the actual session-search CLI persistence path instead (e.g.,
invoke run_session_search_cli or the shared artifact writer used by production)
with the same artifact_path so the code under test performs the round-trip;
locate the test's direct write block around artifact_path / encoded and replace
the fs::write/serde_json::to_string_pretty usage with a call to
run_session_search_cli(..., &artifact_path) or the shared writer function (the
function name run_session_search_cli and the shared artifact writer should be
used to find the correct helper) ensuring the test still asserts the same
resulting file contents.
🪄 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: CHILL
Plan: Pro
Run ID: 3ab540d8-1747-4dbf-953e-872b7dc2a75e
📒 Files selected for processing (5)
crates/daemon/src/lib.rscrates/daemon/src/main.rscrates/daemon/src/session_cli.rscrates/daemon/tests/integration/cli_tests.rscrates/daemon/tests/integration/session_search_cli.rs
chumyin
force-pushed
the
feat/session-search-p0
branch
from
4b46b54 to
8a14580
Compare
github-actions
Bot
removed
dependencies
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/daemon/src/lib.rs (1)
3383-3406:⚠️ Potential issue | 🟠 MajorMake the shared artifact writer atomic and parameterize the artifact label.
Making this helper
pub(crate)so the new artifact surfaces can reuse it widens the blast radius of two issues: the non-atomicfs::write()path and the hard-coded "runtime snapshot artifact" error text. An interrupted write can leave a malformed JSON artifact behind, and bothsession_cliandtrajectory_clicallers will report the wrong artifact type on failure. Please convert this to an atomic temp-file + replace pattern and accept an artifact label parameter so each caller can surface the correct error context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon/src/lib.rs` around lines 3383 - 3406, The persist_runtime_snapshot_artifact function must be changed to perform atomic writes and accept an artifact label: add an artifact_label: &str parameter (and rename the function to something generic if you want, e.g., persist_artifact) so callers (session_cli, trajectory_cli) can pass their label; create the parent dir as you already do, then write the pretty-serialized payload to a temporary file in the same directory (unique tmp name), fsync the file, and atomically replace the target via fs::rename (or platform-appropriate replace), and update all error messages in this function (serialize, temp write, rename, create_dir_all) to include the provided artifact_label so the caller-visible error context is correct.
♻️ Duplicate comments (4)
crates/app/src/session/repository.rs (2)
805-814:⚠️ Potential issue | 🟠 MajorLegacy-only current sessions are still excluded from search.
visibleandvisible_sessionsare still built fromsessions, so a current session that only exists via legacyturnsnever enters the search scope. That makesSessionRepository::search_visible_session_turnsreturn no hits for transcripts thatSessionRepository::load_session_summary_with_legacy_fallbackcan surface, and it also misses descendants thatSessionRepository::is_session_visiblewould consider visible when they hang off that legacy root. Mirror the legacy fallback in this query instead of deriving the scope fromsessionsalone.Also applies to: 821-859
799-801:⚠️ Potential issue | 🟡 MinorKeep explicit zero limits as zero.
Line 801 still turns
limit = 0intoLIMIT 1, so callers asking for no hits will get a row back. PreserveLIMIT 0semantics or reject zero up front instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/src/session/repository.rs` around lines 799 - 801, The current clamp forces a minimum of 1 so callers passing limit = 0 become LIMIT 1; instead preserve zero semantics by removing the .max(1) clamp. Update the limit calculation (near normalize_required_text calls for current_session_id and query) to: compute limit = limit.min(i64::MAX as usize) as i64 (or alternatively return an error if you prefer rejecting zero), so LIMIT 0 is allowed rather than being coerced to 1.crates/app/src/tools/session.rs (1)
3318-3345:⚠️ Potential issue | 🟡 Minor
session_search_snippetcan exceed the configuredmax_chars.Line 3325 and Line 3326 reserve prefix/suffix budgets before accounting for matched query length, so longer queries can push snippets over the cap.
💡 Proposed fix
fn session_search_snippet(content: &str, query: &str, max_chars: usize) -> String { if content.chars().count() <= max_chars { return content.to_owned(); } if let Some(start) = content.find(query) { let end = start.saturating_add(query.len()).min(content.len()); - let prefix = trim_chars_from_end(&content[..start], max_chars / 3); - let suffix = trim_chars_from_start(&content[end..], max_chars / 2); + let matched = &content[start..end]; + let match_chars = matched.chars().count(); + let remaining = max_chars.saturating_sub(match_chars); + let prefix_budget = remaining / 3; + let suffix_budget = remaining.saturating_sub(prefix_budget); + let prefix = trim_chars_from_end(&content[..start], prefix_budget); + let suffix = trim_chars_from_start(&content[end..], suffix_budget); let mut snippet = String::new(); if prefix.len() < start { snippet.push_str("..."); } snippet.push_str(&prefix); - snippet.push_str(&content[start..end]); + snippet.push_str(matched); snippet.push_str(&suffix); if end + suffix.len() < content.len() { snippet.push_str("..."); } - return snippet; + if snippet.chars().count() > max_chars { + return trim_chars_from_start(&snippet, max_chars); + } + return snippet; } let mut trimmed = trim_chars_from_start(content, max_chars);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/src/tools/session.rs` around lines 3318 - 3345, session_search_snippet can produce a snippet longer than max_chars because prefix/suffix budgets are computed without subtracting the matched query length; fix by computing the remaining_budget = max_chars.saturating_sub(query.chars().count()) (or query.len() in bytes carefully converted to chars), then allocate prefix_budget and suffix_budget from that remaining_budget (e.g., prefix_budget = remaining_budget / 3, suffix_budget = remaining_budget - prefix_budget), use those budgets when calling trim_chars_from_end and trim_chars_from_start for the prefix and suffix, and ensure all lengths use char counts and saturating math (reference function session_search_snippet, variables start/end, prefix/suffix, and calls to trim_chars_from_end/trim_chars_from_start).crates/daemon/src/trajectory_cli.rs (1)
51-55:⚠️ Potential issue | 🟠 MajorRemove host-local
configfrom the persisted trajectory artifact schema.
configpersists a machine-specific filesystem path inside the JSON artifact, which makes exports non-portable and leaks local path metadata. Keep the resolved path only in CLI-rendered text output.Suggested fix
pub struct TrajectoryExportArtifactDocument { pub schema: TrajectoryExportArtifactSchema, pub exported_at: String, - pub config: String, pub session: TrajectoryExportSessionSummary, pub turns: Vec<TrajectoryExportTurn>, pub events: Vec<TrajectoryExportEvent>, } @@ let artifact = TrajectoryExportArtifactDocument { schema: TrajectoryExportArtifactSchema { version: TRAJECTORY_EXPORT_ARTIFACT_JSON_SCHEMA_VERSION, surface: "trajectory_export".to_owned(), purpose: "session_replay_evidence".to_owned(), }, exported_at, - config: resolved_path.display().to_string(), session: TrajectoryExportSessionSummary { session_id: session_summary.session_id,Also applies to: 118-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon/src/trajectory_cli.rs` around lines 51 - 55, The persisted artifact struct TrajectoryExportArtifactDocument (and the other persisted schema occurrence) currently includes a host-local `config: String` field that embeds machine-specific paths; remove the `config` field from the persisted schema/struct definitions and adjust any serialization/deserialization, builders, and save/load logic that reference TrajectoryExportArtifactDocument::config so exports no longer contain the local path; preserve the resolved config path only in CLI-rendered output code (e.g., where you print or render export results) rather than writing it into the persisted TrajectoryExportArtifactSchema or document.
🧹 Nitpick comments (1)
crates/daemon/tests/integration/cli_tests.rs (1)
565-750: Consider extracting a shared trajectory artifact fixture builder for test clarity.The same
TrajectoryExportArtifactDocumentsetup is repeated in multiple tests; a small helper would reduce duplication and lower schema-update friction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon/tests/integration/cli_tests.rs` around lines 565 - 750, Extract a reusable test fixture builder that returns a TrajectoryExportArtifactDocument to replace the repeated inline structs used in format_trajectory_export_text and format_trajectory_inspect_text tests: create a helper like build_test_trajectory_artifact(...) that constructs the TrajectoryExportArtifactDocument (including TrajectoryExportArtifactSchema, TrajectoryExportSessionSummary, turns Vec<TrajectoryExportTurn>, and events Vec<TrajectoryExportEvent>) with sensible defaults and optional parameters for exported_at, config, session_id, turns, and events; then update the tests (format_trajectory_export_text_summarizes_counts, format_trajectory_inspect_text_summarizes_counts, format_trajectory_inspect_text_summarizes_roles_and_events) to call the helper and pass the artifact path into format_trajectory_export_text / format_trajectory_inspect_text so the tests are shorter and centralized for future schema changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/daemon/src/lib.rs`:
- Around line 3383-3406: The persist_runtime_snapshot_artifact function must be
changed to perform atomic writes and accept an artifact label: add an
artifact_label: &str parameter (and rename the function to something generic if
you want, e.g., persist_artifact) so callers (session_cli, trajectory_cli) can
pass their label; create the parent dir as you already do, then write the
pretty-serialized payload to a temporary file in the same directory (unique tmp
name), fsync the file, and atomically replace the target via fs::rename (or
platform-appropriate replace), and update all error messages in this function
(serialize, temp write, rename, create_dir_all) to include the provided
artifact_label so the caller-visible error context is correct.
---
Duplicate comments:
In `@crates/app/src/session/repository.rs`:
- Around line 799-801: The current clamp forces a minimum of 1 so callers
passing limit = 0 become LIMIT 1; instead preserve zero semantics by removing
the .max(1) clamp. Update the limit calculation (near normalize_required_text
calls for current_session_id and query) to: compute limit = limit.min(i64::MAX
as usize) as i64 (or alternatively return an error if you prefer rejecting
zero), so LIMIT 0 is allowed rather than being coerced to 1.
In `@crates/app/src/tools/session.rs`:
- Around line 3318-3345: session_search_snippet can produce a snippet longer
than max_chars because prefix/suffix budgets are computed without subtracting
the matched query length; fix by computing the remaining_budget =
max_chars.saturating_sub(query.chars().count()) (or query.len() in bytes
carefully converted to chars), then allocate prefix_budget and suffix_budget
from that remaining_budget (e.g., prefix_budget = remaining_budget / 3,
suffix_budget = remaining_budget - prefix_budget), use those budgets when
calling trim_chars_from_end and trim_chars_from_start for the prefix and suffix,
and ensure all lengths use char counts and saturating math (reference function
session_search_snippet, variables start/end, prefix/suffix, and calls to
trim_chars_from_end/trim_chars_from_start).
In `@crates/daemon/src/trajectory_cli.rs`:
- Around line 51-55: The persisted artifact struct
TrajectoryExportArtifactDocument (and the other persisted schema occurrence)
currently includes a host-local `config: String` field that embeds
machine-specific paths; remove the `config` field from the persisted
schema/struct definitions and adjust any serialization/deserialization,
builders, and save/load logic that reference
TrajectoryExportArtifactDocument::config so exports no longer contain the local
path; preserve the resolved config path only in CLI-rendered output code (e.g.,
where you print or render export results) rather than writing it into the
persisted TrajectoryExportArtifactSchema or document.
---
Nitpick comments:
In `@crates/daemon/tests/integration/cli_tests.rs`:
- Around line 565-750: Extract a reusable test fixture builder that returns a
TrajectoryExportArtifactDocument to replace the repeated inline structs used in
format_trajectory_export_text and format_trajectory_inspect_text tests: create a
helper like build_test_trajectory_artifact(...) that constructs the
TrajectoryExportArtifactDocument (including TrajectoryExportArtifactSchema,
TrajectoryExportSessionSummary, turns Vec<TrajectoryExportTurn>, and events
Vec<TrajectoryExportEvent>) with sensible defaults and optional parameters for
exported_at, config, session_id, turns, and events; then update the tests
(format_trajectory_export_text_summarizes_counts,
format_trajectory_inspect_text_summarizes_counts,
format_trajectory_inspect_text_summarizes_roles_and_events) to call the helper
and pass the artifact path into format_trajectory_export_text /
format_trajectory_inspect_text so the tests are shorter and centralized for
future schema changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c9311df-949b-472d-818a-57fed48b4f77
📒 Files selected for processing (13)
crates/app/src/session/repository.rscrates/app/src/tools/catalog.rscrates/app/src/tools/mod.rscrates/app/src/tools/session.rscrates/daemon/src/command_kind.rscrates/daemon/src/lib.rscrates/daemon/src/main.rscrates/daemon/src/session_cli.rscrates/daemon/src/trajectory_cli.rscrates/daemon/tests/integration/cli_tests.rscrates/daemon/tests/integration/mod.rscrates/daemon/tests/integration/session_search_cli.rscrates/daemon/tests/integration/trajectory_export_cli.rs
✅ Files skipped from review due to trivial changes (2)
- crates/daemon/tests/integration/mod.rs
- crates/daemon/tests/integration/trajectory_export_cli.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/app/src/tools/catalog.rs
- crates/daemon/src/session_cli.rs
28 tasks
github-actions
Bot
added
the
documentation
chumyin
force-pushed
the
feat/session-search-p0
branch
from
5ad362c to
0d0d66c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/app/src/tools/session.rs (1)
3342-3389:⚠️ Potential issue | 🟡 Minor
session_search_snippet()can still return more thanmax_chars.Two edge paths still overflow the cap here: a near-cap match can become
...<match>...(for example, a 79-char match under an 80-char cap), and the no-match fallback appends...after already takingmax_charschars. Please clamp both branches before returning and add regressions for them.✂️ Suggested fix
loop { let prefix_trimmed = prefix_budget < prefix_source_chars; let suffix_trimmed = suffix_budget < suffix_source_chars; let prefix_marker_chars = usize::from(prefix_trimmed) * 3; let suffix_marker_chars = usize::from(suffix_trimmed) * 3; @@ } break; } + let prefix_trimmed = prefix_budget < prefix_source_chars; + let suffix_trimmed = suffix_budget < suffix_source_chars; + let marker_chars = usize::from(prefix_trimmed) * 3 + usize::from(suffix_trimmed) * 3; + if matched_chars + marker_chars > max_chars { + return trim_chars_from_start(matched, max_chars); + } + let prefix = trim_chars_from_end(prefix_source, prefix_budget); let suffix = trim_chars_from_start(suffix_source, suffix_budget); let mut snippet = String::new(); - if prefix.chars().count() < prefix_source_chars { + if prefix_trimmed { snippet.push_str("..."); } snippet.push_str(&prefix); snippet.push_str(matched); snippet.push_str(&suffix); - if suffix.chars().count() < suffix_source_chars { + if suffix_trimmed { snippet.push_str("..."); } return snippet; } - let mut trimmed = trim_chars_from_start(content, max_chars); - if trimmed.chars().count() < content.chars().count() { - trimmed.push_str("..."); - } - trimmed + if max_chars <= 3 { + return trim_chars_from_start(content, max_chars); + } + + let mut trimmed = trim_chars_from_start(content, max_chars - 3); + trimmed.push_str("..."); + trimmed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/src/tools/session.rs` around lines 3342 - 3389, session_search_snippet can still exceed max_chars in two paths: the matched branch when adding "..." around a near-cap match and the no-match fallback where "..." is appended after taking max_chars; after building the snippet (and after building trimmed in the no-match fallback) clamp/truncate the final String to at most max_chars characters (ensuring any added "..." fits by removing chars from the appropriate side before appending), using the existing helpers trim_chars_from_start/trim_chars_from_end as needed so character boundaries are preserved, and add regressions that construct these edge cases to assert snippet.chars().count() <= max_chars for session_search_snippet.
🧹 Nitpick comments (2)
crates/app/src/session/repository.rs (1)
837-840: Redundant OR condition in content search.If
instr(t.content, ?4) > 0succeeds (exact case match), the second conditioninstr(lower(t.content), lower(?4)) > 0is guaranteed to succeed as well. Consider simplifying to just the case-insensitive check for consistency:WHERE (?3 OR archived.archived_at IS NULL) AND instr(lower(t.content), lower(?4)) > 0Alternatively, if exact-match prioritization is intended for performance, document this decision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/src/session/repository.rs` around lines 837 - 840, The SQL WHERE clause currently uses a redundant OR: instr(t.content, ?4) > 0 OR instr(lower(t.content), lower(?4)) > 0; simplify to a single case-insensitive check by replacing both with instr(lower(t.content), lower(?4)) > 0 (or explicitly document why you kept the exact-match instr(t.content, ?4) > 0 for performance). Update the condition near the archived filter (archived.archived_at IS NULL) in the query building code in repository.rs where t.content and parameter ?4 are used so only the case-insensitive instr(lower(t.content), lower(?4)) > 0 remains (or add a comment explaining intentional prioritization of exact-match).crates/app/src/tools/session.rs (1)
513-516: Cache workflow metadata per session while rendering hits.A search can return several turns from the same session, and
load_session_workflow_record()repeats the same lineage/delegate-event lookups for each one. Caching bysession_idkeeps this path from turning into an avoidable N+1 query pattern.♻️ Suggested refactor
let returned_count = hits.len(); let mut rendered_hits = Vec::with_capacity(returned_count); + let mut workflow_cache = BTreeMap::new(); for hit in hits { - let workflow = load_session_workflow_record(&repo, &hit.session, None)?; + let session_id = hit.session.session_id.clone(); + let workflow = match workflow_cache.get(&session_id) { + Some(workflow) => workflow.clone(), + None => { + let workflow = load_session_workflow_record(&repo, &hit.session, None)?; + workflow_cache.insert(session_id, workflow.clone()); + workflow + } + }; let rendered_hit = session_search_hit_json(hit, workflow, &request.query); rendered_hits.push(rendered_hit); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/src/tools/session.rs` around lines 513 - 516, The loop over hits calls load_session_workflow_record(&repo, &hit.session, None) for every hit causing N+1 work; cache the workflow per session_id in a local HashMap (e.g., HashMap<SessionId, WorkflowRecord>) before rendering: for each hit, lookup hit.session.id (or session identifier) in the map, if present use the cached workflow, otherwise call load_session_workflow_record, insert the result into the map, then call session_search_hit_json(workflow, &request.query) and push to rendered_hits; keep the existing error propagation (the ? on load_session_workflow_record) so failures still bubble up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/daemon/src/lib.rs`:
- Around line 3386-3449: persist_json_artifact currently writes and fsyncs the
temp file but does not fsync the parent directory after fs::rename, risking loss
of the directory entry on Unix; after successfully renaming
(fs::rename(&temp_path, &output_path)), open the parent directory (parent_path)
as a File (e.g., fs::OpenOptions::new().read(true).open(&parent_path)) and call
sync_all on it on Unix-only targets (cfg(unix)), mapping any IO error into the
same CliResult error path (or at least logging/returning it) and ensure the
remove_file cleanup remains if rename failed; reference persist_json_artifact,
temp_path, parent_path, fs::rename and add the platform-gated parent directory
sync step immediately after the rename and before returning Ok(()).
---
Duplicate comments:
In `@crates/app/src/tools/session.rs`:
- Around line 3342-3389: session_search_snippet can still exceed max_chars in
two paths: the matched branch when adding "..." around a near-cap match and the
no-match fallback where "..." is appended after taking max_chars; after building
the snippet (and after building trimmed in the no-match fallback) clamp/truncate
the final String to at most max_chars characters (ensuring any added "..." fits
by removing chars from the appropriate side before appending), using the
existing helpers trim_chars_from_start/trim_chars_from_end as needed so
character boundaries are preserved, and add regressions that construct these
edge cases to assert snippet.chars().count() <= max_chars for
session_search_snippet.
---
Nitpick comments:
In `@crates/app/src/session/repository.rs`:
- Around line 837-840: The SQL WHERE clause currently uses a redundant OR:
instr(t.content, ?4) > 0 OR instr(lower(t.content), lower(?4)) > 0; simplify to
a single case-insensitive check by replacing both with instr(lower(t.content),
lower(?4)) > 0 (or explicitly document why you kept the exact-match
instr(t.content, ?4) > 0 for performance). Update the condition near the
archived filter (archived.archived_at IS NULL) in the query building code in
repository.rs where t.content and parameter ?4 are used so only the
case-insensitive instr(lower(t.content), lower(?4)) > 0 remains (or add a
comment explaining intentional prioritization of exact-match).
In `@crates/app/src/tools/session.rs`:
- Around line 513-516: The loop over hits calls
load_session_workflow_record(&repo, &hit.session, None) for every hit causing
N+1 work; cache the workflow per session_id in a local HashMap (e.g.,
HashMap<SessionId, WorkflowRecord>) before rendering: for each hit, lookup
hit.session.id (or session identifier) in the map, if present use the cached
workflow, otherwise call load_session_workflow_record, insert the result into
the map, then call session_search_hit_json(workflow, &request.query) and push to
rendered_hits; keep the existing error propagation (the ? on
load_session_workflow_record) so failures still bubble up.
🪄 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: CHILL
Plan: Pro
Run ID: 63bae6b2-21a3-4b67-9ead-ac17ae6fc6b6
📒 Files selected for processing (10)
crates/app/src/session/repository.rscrates/app/src/tools/session.rscrates/daemon/src/command_kind.rscrates/daemon/src/lib.rscrates/daemon/src/session_cli.rscrates/daemon/src/trajectory_cli.rscrates/daemon/tests/integration/cli_tests.rscrates/daemon/tests/integration/session_search_cli.rscrates/daemon/tests/integration/trajectory_export_cli.rsdocs/releases/architecture-drift-2026-04.md
✅ Files skipped from review due to trivial changes (2)
- docs/releases/architecture-drift-2026-04.md
- crates/daemon/tests/integration/trajectory_export_cli.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/daemon/src/command_kind.rs
- crates/daemon/tests/integration/cli_tests.rs
- crates/daemon/src/trajectory_cli.rs
- crates/daemon/src/session_cli.rs
Comment on lines
+3386
to
3449
| pub(crate) fn persist_json_artifact( | ||
| output_path: &str, | ||
| payload: &Value, | ||
| artifact_label: &str, | ||
| ) -> CliResult<()> { | ||
| let output_path = PathBuf::from(output_path); | ||
| if let Some(parent) = output_path.parent() | ||
| && !parent.as_os_str().is_empty() | ||
| { | ||
| fs::create_dir_all(parent).map_err(|error| { | ||
| format!( | ||
| "create runtime snapshot artifact directory {} failed: {error}", | ||
| parent.display() | ||
| ) | ||
| })?; | ||
| } | ||
| let parent_path = output_path | ||
| .parent() | ||
| .filter(|path| !path.as_os_str().is_empty()) | ||
| .map(Path::to_path_buf) | ||
| .unwrap_or_else(|| PathBuf::from(".")); | ||
| fs::create_dir_all(&parent_path).map_err(|error| { | ||
| format!( | ||
| "create {artifact_label} directory {} failed: {error}", | ||
| parent_path.display() | ||
| ) | ||
| })?; | ||
| let encoded = serde_json::to_string_pretty(payload) | ||
| .map_err(|error| format!("serialize runtime snapshot artifact failed: {error}"))?; | ||
| fs::write(&output_path, encoded).map_err(|error| { | ||
| .map_err(|error| format!("serialize {artifact_label} failed: {error}"))?; | ||
| let file_name = output_path | ||
| .file_name() | ||
| .and_then(|name| name.to_str()) | ||
| .unwrap_or("artifact"); | ||
| let process_id = process::id(); | ||
| let timestamp = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .map_err(|error| format!("build {artifact_label} temp path failed: {error}"))? | ||
| .as_nanos(); | ||
| let temp_file_name = format!(".{file_name}.{process_id}.{timestamp}.tmp"); | ||
| let temp_path = parent_path.join(temp_file_name); | ||
|
|
||
| let open_result = fs::OpenOptions::new() | ||
| .write(true) | ||
| .create_new(true) | ||
| .open(&temp_path); | ||
| let mut temp_file = open_result.map_err(|error| { | ||
| format!( | ||
| "write runtime snapshot artifact {} failed: {error}", | ||
| output_path.display() | ||
| "create {artifact_label} temp file {} failed: {error}", | ||
| temp_path.display() | ||
| ) | ||
| })?; | ||
| temp_file.write_all(encoded.as_bytes()).map_err(|error| { | ||
| format!( | ||
| "write {artifact_label} temp file {} failed: {error}", | ||
| temp_path.display() | ||
| ) | ||
| })?; | ||
| temp_file.sync_all().map_err(|error| { | ||
| format!( | ||
| "sync {artifact_label} temp file {} failed: {error}", | ||
| temp_path.display() | ||
| ) | ||
| })?; | ||
| drop(temp_file); | ||
|
|
||
| let rename_result = fs::rename(&temp_path, &output_path); | ||
| if let Err(error) = rename_result { | ||
| let _ = fs::remove_file(&temp_path); | ||
| return Err(format!( | ||
| "replace {artifact_label} {} failed: {error}", | ||
| output_path.display() | ||
| )); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Rust std::fs::rename overwrite existing file on Windows, and is fsync on the parent directory required after rename for crash-safe atomic file replacement?
💡 Result:
Yes, std::fs::rename overwrites existing files on Windows. No, fsync on the parent directory is not required after rename for crash-safe atomic file replacement on NTFS, as rename is a metadata operation protected by NTFS journaling. Rust's std::fs::rename uses MoveFileExW with MOVEFILE_REPLACE_EXISTING on Windows, which replaces the target if it exists. For modern Windows (10 1607+), it prefers atomic FileRenameInformationEx when possible, falling back to non-atomic otherwise, but aims for POSIX-like semantics where feasible. The operation is atomic with respect to other filesystem operations due to NTFS metadata journaling: directory entry changes (unlink old name, link new name, replace target) are logged atomically. On crash, the filesystem recovers to a consistent state—either fully before or after the rename. No partial rename is visible. Unlike Unix (where fsync on parent dir ensures directory metadata durability post-rename), Windows NTFS journaling handles metadata durability without user fsync. std::fs::rename does not fsync, but NTFS ensures crash-safety for metadata ops. For data durability (new file contents), fsync the file before rename.
Citations:
- 1: https://doc.rust-lang.org/stable/std/fs/fn.rename.html
- 2: https://doc.rust-lang.org/std/fs/fn.rename.html
- 3: Win: Fix std::fs::rename failing on Windows Server by attempting the non-atomic rename first rust-lang/rust#138133
- 4: https://stackoverflow.com/questions/167414/is-an-atomic-file-rename-with-overwrite-possible-on-windows
- 5: https://learn.microsoft.com/en-us/archive/msdn-technet-forums/449bb49d-8acc-48dc-a46f-0760ceddbfc3
- 6: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-movefileexa
🏁 Script executed:
# Search for usages of persist_json_artifact
rg "persist_json_artifact" --type rs -B 2 -A 2Repository: loongclaw-ai/loongclaw
Length of output: 92
🏁 Script executed:
# Check if there are similar atomic write patterns elsewhere in the codebase
rg "sync_all\(\)" --type rs -B 3 -A 1 | head -50Repository: loongclaw-ai/loongclaw
Length of output: 92
🏁 Script executed:
# Check for any related filesystem safety patterns
rg "fs::rename|rename_result" --type rs -B 2 -A 2Repository: loongclaw-ai/loongclaw
Length of output: 92
🏁 Script executed:
# Search for usages of persist_json_artifact
rg "persist_json_artifact" -g '*.rs' -B 2 -A 2Repository: loongclaw-ai/loongclaw
Length of output: 1900
🏁 Script executed:
# Check if there are similar atomic write patterns elsewhere
rg "sync_all\(\)" -g '*.rs' -B 3 -A 1 | head -50Repository: loongclaw-ai/loongclaw
Length of output: 3379
🏁 Script executed:
# Check for any related filesystem safety patterns
rg "fs::rename|\.sync_all" -g '*.rs' -B 2 -A 2 | head -100Repository: loongclaw-ai/loongclaw
Length of output: 6515
Add parent directory fsync after rename for crash-safe atomic file replacement on Unix.
This function handles multiple critical artifact outputs (trajectory exports, session searches, runtime snapshots) but only fsyncs the temp file content. On Unix platforms, a crash after the rename but before parent directory metadata is persisted can still lose the final directory entry. Windows is unaffected—NTFS journaling ensures metadata durability without user-level fsync.
The suggested fix below handles this correctly with platform-specific logic:
Proposed hardening
let rename_result = fs::rename(&temp_path, &output_path);
if let Err(error) = rename_result {
let _ = fs::remove_file(&temp_path);
return Err(format!(
"replace {artifact_label} {} failed: {error}",
output_path.display()
));
}
+ #[cfg(unix)]
+ {
+ let parent_dir = fs::File::open(&parent_path).map_err(|error| {
+ format!(
+ "open {artifact_label} directory {} for sync failed: {error}",
+ parent_path.display()
+ )
+ })?;
+ parent_dir.sync_all().map_err(|error| {
+ format!(
+ "sync {artifact_label} directory {} failed: {error}",
+ parent_path.display()
+ )
+ })?;
+ }
Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub(crate) fn persist_json_artifact( | |
| output_path: &str, | |
| payload: &Value, | |
| artifact_label: &str, | |
| ) -> CliResult<()> { | |
| let output_path = PathBuf::from(output_path); | |
| if let Some(parent) = output_path.parent() | |
| && !parent.as_os_str().is_empty() | |
| { | |
| fs::create_dir_all(parent).map_err(|error| { | |
| format!( | |
| "create runtime snapshot artifact directory {} failed: {error}", | |
| parent.display() | |
| ) | |
| })?; | |
| } | |
| let parent_path = output_path | |
| .parent() | |
| .filter(|path| !path.as_os_str().is_empty()) | |
| .map(Path::to_path_buf) | |
| .unwrap_or_else(|| PathBuf::from(".")); | |
| fs::create_dir_all(&parent_path).map_err(|error| { | |
| format!( | |
| "create {artifact_label} directory {} failed: {error}", | |
| parent_path.display() | |
| ) | |
| })?; | |
| let encoded = serde_json::to_string_pretty(payload) | |
| .map_err(|error| format!("serialize runtime snapshot artifact failed: {error}"))?; | |
| fs::write(&output_path, encoded).map_err(|error| { | |
| .map_err(|error| format!("serialize {artifact_label} failed: {error}"))?; | |
| let file_name = output_path | |
| .file_name() | |
| .and_then(|name| name.to_str()) | |
| .unwrap_or("artifact"); | |
| let process_id = process::id(); | |
| let timestamp = SystemTime::now() | |
| .duration_since(UNIX_EPOCH) | |
| .map_err(|error| format!("build {artifact_label} temp path failed: {error}"))? | |
| .as_nanos(); | |
| let temp_file_name = format!(".{file_name}.{process_id}.{timestamp}.tmp"); | |
| let temp_path = parent_path.join(temp_file_name); | |
| let open_result = fs::OpenOptions::new() | |
| .write(true) | |
| .create_new(true) | |
| .open(&temp_path); | |
| let mut temp_file = open_result.map_err(|error| { | |
| format!( | |
| "write runtime snapshot artifact {} failed: {error}", | |
| output_path.display() | |
| "create {artifact_label} temp file {} failed: {error}", | |
| temp_path.display() | |
| ) | |
| })?; | |
| temp_file.write_all(encoded.as_bytes()).map_err(|error| { | |
| format!( | |
| "write {artifact_label} temp file {} failed: {error}", | |
| temp_path.display() | |
| ) | |
| })?; | |
| temp_file.sync_all().map_err(|error| { | |
| format!( | |
| "sync {artifact_label} temp file {} failed: {error}", | |
| temp_path.display() | |
| ) | |
| })?; | |
| drop(temp_file); | |
| let rename_result = fs::rename(&temp_path, &output_path); | |
| if let Err(error) = rename_result { | |
| let _ = fs::remove_file(&temp_path); | |
| return Err(format!( | |
| "replace {artifact_label} {} failed: {error}", | |
| output_path.display() | |
| )); | |
| } | |
| Ok(()) | |
| pub(crate) fn persist_json_artifact( | |
| output_path: &str, | |
| payload: &Value, | |
| artifact_label: &str, | |
| ) -> CliResult<()> { | |
| let output_path = PathBuf::from(output_path); | |
| let parent_path = output_path | |
| .parent() | |
| .filter(|path| !path.as_os_str().is_empty()) | |
| .map(Path::to_path_buf) | |
| .unwrap_or_else(|| PathBuf::from(".")); | |
| fs::create_dir_all(&parent_path).map_err(|error| { | |
| format!( | |
| "create {artifact_label} directory {} failed: {error}", | |
| parent_path.display() | |
| ) | |
| })?; | |
| let encoded = serde_json::to_string_pretty(payload) | |
| .map_err(|error| format!("serialize {artifact_label} failed: {error}"))?; | |
| let file_name = output_path | |
| .file_name() | |
| .and_then(|name| name.to_str()) | |
| .unwrap_or("artifact"); | |
| let process_id = process::id(); | |
| let timestamp = SystemTime::now() | |
| .duration_since(UNIX_EPOCH) | |
| .map_err(|error| format!("build {artifact_label} temp path failed: {error}"))? | |
| .as_nanos(); | |
| let temp_file_name = format!(".{file_name}.{process_id}.{timestamp}.tmp"); | |
| let temp_path = parent_path.join(temp_file_name); | |
| let open_result = fs::OpenOptions::new() | |
| .write(true) | |
| .create_new(true) | |
| .open(&temp_path); | |
| let mut temp_file = open_result.map_err(|error| { | |
| format!( | |
| "create {artifact_label} temp file {} failed: {error}", | |
| temp_path.display() | |
| ) | |
| })?; | |
| temp_file.write_all(encoded.as_bytes()).map_err(|error| { | |
| format!( | |
| "write {artifact_label} temp file {} failed: {error}", | |
| temp_path.display() | |
| ) | |
| })?; | |
| temp_file.sync_all().map_err(|error| { | |
| format!( | |
| "sync {artifact_label} temp file {} failed: {error}", | |
| temp_path.display() | |
| ) | |
| })?; | |
| drop(temp_file); | |
| let rename_result = fs::rename(&temp_path, &output_path); | |
| if let Err(error) = rename_result { | |
| let _ = fs::remove_file(&temp_path); | |
| return Err(format!( | |
| "replace {artifact_label} {} failed: {error}", | |
| output_path.display() | |
| )); | |
| } | |
| #[cfg(unix)] | |
| { | |
| let parent_dir = fs::File::open(&parent_path).map_err(|error| { | |
| format!( | |
| "open {artifact_label} directory {} for sync failed: {error}", | |
| parent_path.display() | |
| ) | |
| })?; | |
| parent_dir.sync_all().map_err(|error| { | |
| format!( | |
| "sync {artifact_label} directory {} failed: {error}", | |
| parent_path.display() | |
| ) | |
| })?; | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/daemon/src/lib.rs` around lines 3386 - 3449, persist_json_artifact
currently writes and fsyncs the temp file but does not fsync the parent
directory after fs::rename, risking loss of the directory entry on Unix; after
successfully renaming (fs::rename(&temp_path, &output_path)), open the parent
directory (parent_path) as a File (e.g.,
fs::OpenOptions::new().read(true).open(&parent_path)) and call sync_all on it on
Unix-only targets (cfg(unix)), mapping any IO error into the same CliResult
error path (or at least logging/returning it) and ensure the remove_file cleanup
remains if rename failed; reference persist_json_artifact, temp_path,
parent_path, fs::rename and add the platform-gated parent directory sync step
immediately after the rename and before returning Ok(()).
8b8f309 to
c1bd67c
Compare
github-actions
Bot
removed
the
conversation
chumyin
force-pushed
the
feat/session-search-p0
branch
from
c1bd67c to
af98804
Compare
github-actions
Bot
added
memory
chumyin
force-pushed
the
feat/session-search-p0
branch
from
77f9706 to
6ea1c13
Compare
github-actions
Bot
added
the
acp
Collaborator
LoongClaw QA Review — PR #905Reviewed commit: Findings
CodeRabbit Major Findings Classification
Coverage
Blockers
VerdictReview complete with high-risk 3-worker pipeline. Cannot mark resolved: PR needs rebase, budget_items conflict needs intentional resolution, 3 net-new CodeRabbit findings need addressing, and loongclaw-dev QA must run post-rebase. |
chumyin
force-pushed
the
feat/session-search-p0
branch
from
2f7bb66 to
fb0995f
Compare
github-actions
Bot
removed
documentation
github-actions
Bot
added
the
documentation
This was referenced Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LoongClaw did not have operator-facing continuity and evidence surfaces for replay-oriented session work on the current
devline. There was no built-in transcript search CLI, no stable search artifact that could be inspected later, and no stable session trajectory artifact export/inspect path.This keeps LoongClaw behind on the continuity/evidence plane even though the runtime already has strong session structure, visibility rules, and governance seams.
Added
session_searchat the session repository + app tool layer, addedloong session-searchwith stable artifact output, addedloong session-search-inspect, addedloong trajectory-export, addedloong trajectory-inspect, extracted the daemon-side session-search artifact logic into a dedicated module, and extracted the command-kind logging map into its own daemon module so the restacked branch stays within architecture budgets on the latestorigin/dev.No semantic/vector retrieval, no background indexing, no scheduler/cron work, no RL/training integration, no capability-promotion automation, and no dependency / lockfile / deny policy changes.
Linked Issues
Change Type
Touched Areas
Risk Track
Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace --lockedcargo test --workspace --all-features --lockedCommands and evidence:
Results:
session_searchandtrajectory_*tests passed on the clean restacked branch.clippypassed on the clean restacked branch.cargo test --workspace --lockedis currently blocked onconversation::tests::handle_turn_with_runtime_safe_lane_plan_skips_runtime_events_when_disabled, tracked in [Bug]: safe-lane runtime event disable test miscounts trust projection events #915 and patched separately in test(conversation): narrow safe-lane runtime event assertion #916.crates/app/src/conversation/*.cargo test --workspace --all-features --lockedwas not rerun on the restacked branch.User-visible / Operator-visible Changes
loong session-searchCLI for transcript search across visible sessions.loong session-searchcan emit a stable search artifact to an output path.loong session-search-inspectCLI for inspecting exported search artifacts.loong trajectory-exportCLI for exporting stable session trajectory artifacts.loong trajectory-inspectCLI for inspecting exported trajectory artifacts.Failure Recovery
Revert the five PR commits or remove the new app-tool / daemon CLI entry points on
dev.Session visibility leaks, archived-session filter regressions, malformed session-search artifacts, malformed trajectory artifacts, or CLI/logging surface drift after the current
loongbaseline restack.Reviewer Focus
session_search.command_kind_for_loggingcoverage keeps the new session/trajectory commands aligned with the currentloongcommand surface.Summary by CodeRabbit
Release Notes
New Features
session-search,session-search-inspect,trajectory-export, andtrajectory-inspect.Tests