Add runtime trajectory artifacts for session lineage export#1096
Closed
chumyin wants to merge 4 commits into
Closed
Add runtime trajectory artifacts for session lineage export#1096chumyin wants to merge 4 commits into
chumyin wants to merge 4 commits into
Conversation
LoongClaw already persisted turns, session events, terminal outcomes, approval requests, and delegate lineage, but operators still lacked one stable artifact that assembled those records into a trajectory export. This change adds an app-layer runtime trajectory contract, exposes the missing persisted turn and terminal-outcome seams needed to build it, and wires a daemon-side `runtime-trajectory` CLI for export/show. Integration coverage now exercises session-only export, lineage export, artifact round-trip, and text rendering. Constraint: Reuse the existing sqlite/session persistence model instead of introducing a new storage backend or daemon-local schema knowledge Rejected: Implement raw sqlite queries only in daemon | would bypass reusable app-layer seams and harden storage coupling in the wrong layer Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep `runtime-trajectory` read-only until replay or learning workflows have their own explicit governance and policy surfaces Tested: cargo fmt --all -- --check; cargo clippy --workspace --all-targets --all-features --locked -- -D warnings; cargo test -p loongclaw-daemon runtime_trajectory --all-features --locked -- --test-threads=1; ./scripts/check_architecture_boundaries.sh; ./scripts/check_dep_graph.sh Tested: cargo test --workspace --locked (reproduces pre-existing failure in `channel::registry::tests::discord_status_splits_config_backed_send_and_stub_serve`) Tested: cargo test --workspace --all-features --locked (reproduces the same pre-existing `discord_status_splits_config_backed_send_and_stub_serve` failure) Not-tested: A completely green full-workspace test run after the unrelated pre-existing Discord channel registry failure is fixed
📝 WalkthroughWalkthroughAdds workspace tracing deps and observability helpers; instruments core flows with structured tracing; implements a runtime trajectory export feature (session-lineage export, artifact schema, canonicalization, stats) and a daemon CLI ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Runtime Trajectory CLI
participant Export as export_runtime_trajectory()
participant Repo as SessionRepository
participant SQLite as Memory/SQLite
participant Canon as Canonicalizer
participant Output as Artifact Writer
CLI->>Export: request export(session_id, mode, config)
Export->>Repo: load_session_summary(requested_session_id)
Repo-->>Export: SessionSummary
Export->>Repo: list_all_events(session_id)
Repo-->>Export: Vec<SessionEventRecord>
Export->>SQLite: session_turn_records_direct(session_id)
SQLite-->>Export: Vec<PersistedConversationTurnRecord}
Export->>Canon: canonicalize_persisted_turns(...)
Canon-->>Export: Vec<RuntimeTrajectoryCanonicalRecord>
Export->>Export: compute_statistics()
Export->>Output: produce RuntimeTrajectoryArtifactDocument
Output-->>CLI: JSON or rendered text
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
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
crates/app/src/session/repository.rs (1)
1508-1544: Two similar upsert methods with subtle differences.This new
upsert_session_terminal_outcomeusesload_session_summary_with_legacy_fallback(supporting legacy sessions inferred from turn history), while the existingupsert_terminal_outcomeat line 1249 usesload_session(requiring explicit session rows).The naming distinction (
session_prefix) is helpful, but consider adding a brief doc comment clarifying when to use each method to prevent confusion.🤖 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 1508 - 1544, Add a short doc comment above upsert_session_terminal_outcome that explains it validates existence via load_session_summary_with_legacy_fallback (which allows legacy sessions inferred from turn history), and contrast it with upsert_terminal_outcome which uses load_session (requiring an explicit session row); mention intended usage scenarios and when to prefer each function so callers won't be confused by the similar names.crates/app/src/provider/runtime_binding.rs (1)
38-46: Test covers only theDirectvariant.The stability test validates
Directbut not theKernel(_)variant. If label stability matters for serialization, consider adding coverage for theKernelcase in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/src/provider/runtime_binding.rs` around lines 38 - 46, The test only asserts the label for ProviderRuntimeBinding::direct(); add a corresponding assertion for the Kernel variant to ensure label stability by constructing a Kernel variant (e.g., ProviderRuntimeBinding::kernel("...") or ProviderRuntimeBinding::Kernel(some_name) depending on the enum constructor) and calling its as_str() to assert the expected stable label; update or add a test in the tests module (near provider_runtime_binding_labels_are_stable) to include ProviderRuntimeBinding::Kernel(...) and assert_eq!(... , "kernel") or the exact expected string returned by as_str().
🤖 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/memory/sqlite.rs`:
- Around line 665-677: The validation error message used when normalizing
session_id is misleading for the direct API; update the call to
normalize_required_str in session_turn_records_direct to use a message referring
to the direct parameter (e.g., "memory.session_turn_records_direct requires
session_id" or similar) instead of "payload.session_id" so callers see correct
wording; locate the normalize_required_str invocation in
session_turn_records_direct and change only the error string to direct-API
phrasing.
In `@crates/app/src/session/trajectory.rs`:
- Around line 161-171: The export assembly opens multiple independent DB
connections (calls via SessionRepository::new then
load_session_summary_with_legacy_fallback, resolve_root_session_id,
collect_export_session_summaries, collect_export_sessions,
build_runtime_trajectory_statistics) which can produce torn/ inconsistent
artifacts; wrap the entire sequence that builds requested_summary,
root_session_id, session_summaries, sessions and statistics in a single
read-only transaction (or obtain a single connection/transaction from
SessionRepository and pass it into those collection functions) so all reads
occur under one consistent snapshot (similar to replace_turns_internal with
TransactionBehavior::Immediate).
In `@crates/daemon/src/main.rs`:
- Line 42: The debug log currently emits the full parsed CLI payload via
tracing::debug!(..., command = ?cli.command, ...), which may leak user-provided
content; change the log to avoid serializing the entire cli.command payload and
instead emit only non-sensitive metadata — e.g., whether a subcommand exists or
the command variant name. Locate the tracing::debug! call that references
cli.command and replace the argument with a safe value such as a boolean like
cli.command.is_some() or the command variant identifier (derive it by
pattern-matching cli.command to extract only the variant name or use
std::mem::discriminant) so no user data is logged.
In `@crates/daemon/src/runtime_trajectory_cli.rs`:
- Around line 161-176: The deserialized artifact in
load_runtime_trajectory_artifact should be validated for schema.version,
surface, and purpose before returning to enforce the versioned contract; after
calling serde_json::from_str in load_runtime_trajectory_artifact, check that
artifact.schema.version == "<expected_version>" (or matches allowed versions),
artifact.schema.surface == "<expected_surface>" and artifact.schema.purpose ==
"<expected_purpose>" and return a CliResult::Err (using the same error format
used elsewhere) if any check fails, including the offending values and the
path.display() in the error message so incompatible or foreign artifacts are
rejected instead of silently accepted.
---
Nitpick comments:
In `@crates/app/src/provider/runtime_binding.rs`:
- Around line 38-46: The test only asserts the label for
ProviderRuntimeBinding::direct(); add a corresponding assertion for the Kernel
variant to ensure label stability by constructing a Kernel variant (e.g.,
ProviderRuntimeBinding::kernel("...") or
ProviderRuntimeBinding::Kernel(some_name) depending on the enum constructor) and
calling its as_str() to assert the expected stable label; update or add a test
in the tests module (near provider_runtime_binding_labels_are_stable) to include
ProviderRuntimeBinding::Kernel(...) and assert_eq!(... , "kernel") or the exact
expected string returned by as_str().
In `@crates/app/src/session/repository.rs`:
- Around line 1508-1544: Add a short doc comment above
upsert_session_terminal_outcome that explains it validates existence via
load_session_summary_with_legacy_fallback (which allows legacy sessions inferred
from turn history), and contrast it with upsert_terminal_outcome which uses
load_session (requiring an explicit session row); mention intended usage
scenarios and when to prefer each function so callers won't be confused by the
similar names.
🪄 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: 5e1c0db7-5ccb-4f22-bdb1-a4afbf682fa7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
Cargo.tomlcrates/app/Cargo.tomlcrates/app/src/acp/manager.rscrates/app/src/channel/mod.rscrates/app/src/lib.rscrates/app/src/memory/mod.rscrates/app/src/memory/sqlite.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/mod.rscrates/app/src/session/repository.rscrates/app/src/session/trajectory.rscrates/app/src/tools/mod.rscrates/daemon/Cargo.tomlcrates/daemon/src/lib.rscrates/daemon/src/main.rscrates/daemon/src/observability.rscrates/daemon/src/runtime_trajectory_cli.rscrates/daemon/tests/integration/cli_tests.rscrates/daemon/tests/integration/mod.rscrates/daemon/tests/integration/runtime_trajectory_cli.rsdocs/product-specs/index.mddocs/product-specs/runtime-trajectory.md
Comment on lines
+665
to
+677
| pub(super) fn session_turn_records_direct( | ||
| session_id: &str, | ||
| config: &MemoryRuntimeConfig, | ||
| ) -> Result<Vec<PersistedConversationTurnRecord>, String> { | ||
| let session_id = normalize_required_str( | ||
| session_id, | ||
| "memory.session_turn_records requires payload.session_id", | ||
| )?; | ||
| let runtime = acquire_memory_runtime(config)?; | ||
| runtime.with_connection("memory.session_turn_records", |conn| { | ||
| query_all_turn_records(conn, session_id) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Use direct-API wording in the validation error message.
At Line 671, the error says payload.session_id, but this function accepts a direct session_id: &str. That can surface misleading errors to callers.
✏️ Proposed fix
pub(super) fn session_turn_records_direct(
session_id: &str,
config: &MemoryRuntimeConfig,
) -> Result<Vec<PersistedConversationTurnRecord>, String> {
let session_id = normalize_required_str(
session_id,
- "memory.session_turn_records requires payload.session_id",
+ "memory.session_turn_records requires session_id",
)?;
let runtime = acquire_memory_runtime(config)?;
runtime.with_connection("memory.session_turn_records", |conn| {
query_all_turn_records(conn, session_id)
})
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/app/src/memory/sqlite.rs` around lines 665 - 677, The validation error
message used when normalizing session_id is misleading for the direct API;
update the call to normalize_required_str in session_turn_records_direct to use
a message referring to the direct parameter (e.g.,
"memory.session_turn_records_direct requires session_id" or similar) instead of
"payload.session_id" so callers see correct wording; locate the
normalize_required_str invocation in session_turn_records_direct and change only
the error string to direct-API phrasing.
Comment on lines
+161
to
+171
| let repo = SessionRepository::new(memory_config)?; | ||
| let requested_summary = repo | ||
| .load_session_summary_with_legacy_fallback(requested_session_id.as_str())? | ||
| .ok_or_else(|| { | ||
| format!("runtime trajectory export session `{requested_session_id}` was not found") | ||
| })?; | ||
| let root_session_id = resolve_root_session_id(&repo, requested_session_id.as_str())?; | ||
| let session_summaries = | ||
| collect_export_session_summaries(&repo, &requested_summary, &root_session_id, export_mode)?; | ||
| let sessions = collect_export_sessions(&repo, memory_config, session_summaries.as_slice())?; | ||
| let statistics = build_runtime_trajectory_statistics(sessions.as_slice()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 '\bfn (session_turn_records_direct|list_visible_sessions|session_lineage_depth|list_all_events|load_terminal_outcome|list_approval_requests_for_session)\b' \
crates/app/src/memory/sqlite.rs crates/app/src/session/repository.rs
rg -n -C4 'TransactionBehavior|transaction\(|unchecked_transaction\(' \
crates/app/src/memory/sqlite.rs crates/app/src/session/repository.rsRepository: loongclaw-ai/loongclaw
Length of output: 11884
🏁 Script executed:
# First, let's look at the export_runtime_trajectory function and surrounding code
sed -n '140,200p' crates/app/src/session/trajectory.rsRepository: loongclaw-ai/loongclaw
Length of output: 2395
🏁 Script executed:
# Check the collect_export_sessions implementation
rg -n -A20 'fn collect_export_sessions' crates/app/src/session/trajectory.rsRepository: loongclaw-ai/loongclaw
Length of output: 1012
🏁 Script executed:
# Check SessionRepository.open_connection to see if each call opens a fresh connection
rg -n -A10 'fn open_connection' crates/app/src/session/repository.rsRepository: loongclaw-ai/loongclaw
Length of output: 670
🏁 Script executed:
# Look for any transaction wrapping in the export path
rg -n -B5 -A15 'fn export_runtime_trajectory' crates/app/src/session/trajectory.rsRepository: loongclaw-ai/loongclaw
Length of output: 906
🏁 Script executed:
# Check if collect_export_sessions opens its own connections or reuses a transaction
rg -n -B3 -A20 'fn collect_export_sessions' crates/app/src/session/Repository: loongclaw-ai/loongclaw
Length of output: 1926
🏁 Script executed:
sed -n '262,295p' crates/app/src/session/trajectory.rsRepository: loongclaw-ai/loongclaw
Length of output: 1306
🏁 Script executed:
# Also check if there are any existing transaction patterns in the export module
rg -n 'transaction|Transaction' crates/app/src/session/trajectory.rsRepository: loongclaw-ai/loongclaw
Length of output: 48
🏁 Script executed:
# Verify session_turn_records_direct opens its own connection
sed -n '665,700p' crates/app/src/memory/sqlite.rsRepository: loongclaw-ai/loongclaw
Length of output: 1610
🏁 Script executed:
# Check if there's any transaction-based access pattern in repository methods
rg -n 'with_connection|open_connection' crates/app/src/session/repository.rs | head -20Repository: loongclaw-ai/loongclaw
Length of output: 1041
🏁 Script executed:
# Verify the exact lines 161-171 match the review snippet
sed -n '161,171p' crates/app/src/session/trajectory.rsRepository: loongclaw-ai/loongclaw
Length of output: 765
🏁 Script executed:
# Check if there's a transaction wrapper around export or any snapshot mechanism
rg -n -B10 'export_runtime_trajectory' crates/app/src/session/trajectory.rs | head -30Repository: loongclaw-ai/loongclaw
Length of output: 490
🏁 Script executed:
# Verify what happens between reading summary and reading related data
rg -n 'lineage_root_session_id|session_lineage_depth|list_visible_sessions' crates/app/src/session/trajectory.rsRepository: loongclaw-ai/loongclaw
Length of output: 353
🏁 Script executed:
# Check what SessionRepository.new does - does it create a shared connection or factory?
rg -n -A15 'impl SessionRepository' crates/app/src/session/repository.rs | head -40Repository: loongclaw-ai/loongclaw
Length of output: 853
🏁 Script executed:
# Verify SessionRepository::new implementation
rg -n -A10 'fn new(' crates/app/src/session/repository.rs | grep -A10 'SessionRepository'Repository: loongclaw-ai/loongclaw
Length of output: 130
🏁 Script executed:
# Check if there's any concurrent write activity that could interleave with export
rg -n 'append_turn|replace_turns|create_session' crates/app/src/session/trajectory.rsRepository: loongclaw-ai/loongclaw
Length of output: 48
🏁 Script executed:
# Let's check if there's any snapshot or transaction wrapping at a higher level
rg -n -B5 -A5 'export_runtime_trajectory' crates/app/src/ --type rust | head -50Repository: loongclaw-ai/loongclaw
Length of output: 900
🏁 Script executed:
# Check if there's any mechanism to coordinate multi-statement reads
rg -n 'with_connection_mut|transaction_with_behavior' crates/app/src/session/repository.rsRepository: loongclaw-ai/loongclaw
Length of output: 140
🏁 Script executed:
# Verify the actual isolation behavior - check if there's any READ UNCOMMITTED or snapshot isolation
rg -n 'PRAGMA|isolation' crates/app/src/memory/sqlite.rsRepository: loongclaw-ai/loongclaw
Length of output: 384
🏁 Script executed:
# Check if SessionRepository methods use any shared connection or transaction context
rg -n 'fn list_visible_sessions' -A30 crates/app/src/session/repository.rs | head -40Repository: loongclaw-ai/loongclaw
Length of output: 1890
🏁 Script executed:
# Verify if load_session_summary_with_legacy_fallback uses a transaction
rg -n 'fn load_session_summary_with_legacy_fallback' -A20 crates/app/src/session/repository.rsRepository: loongclaw-ai/loongclaw
Length of output: 1927
🏁 Script executed:
# Check what isolation level SQLite is using
rg -n 'journal_mode|IMMEDIATE|DEFERRED|EXCLUSIVE' crates/app/src/Repository: loongclaw-ai/loongclaw
Length of output: 336
🏁 Script executed:
# Check if any recent changes to export_runtime_trajectory have addressed the transaction issue
git log --oneline -20 -- crates/app/src/session/trajectory.rs 2>/dev/null || echo "git log not available"Repository: loongclaw-ai/loongclaw
Length of output: 139
🏁 Script executed:
# Verify if there's a pattern elsewhere in the codebase for multi-step operations that needs atomicity
rg -n 'fn.*\(' crates/app/src/session/trajectory.rs | grep -E 'export|collect'Repository: loongclaw-ai/loongclaw
Length of output: 176
🏁 Script executed:
# Check if SQLite WAL mode provides sufficient isolation for the concurrent scenario
rg -n 'synchronous|wal_autocheckpoint' crates/app/src/memory/sqlite.rsRepository: loongclaw-ai/loongclaw
Length of output: 214
🏁 Script executed:
# Check the recent commit that mentions "export"
git show 0535391 --stat 2>/dev/null | head -20 || echo "Cannot show commit details"Repository: loongclaw-ai/loongclaw
Length of output: 1180
🏁 Script executed:
# Look for any subsequent fixes after the export was added
git log --oneline -30 -- crates/app/src/session/trajectory.rs 2>/dev/null | head -20Repository: loongclaw-ai/loongclaw
Length of output: 139
🏁 Script executed:
# Verify if WAL mode with NORMAL synchronous provides sufficient isolation
rg -n 'PRAGMA|isolation_level|wal' crates/app/src/memory/sqlite.rs | head -15Repository: loongclaw-ai/loongclaw
Length of output: 384
🏁 Script executed:
# Check if the export functions accept or pass a transaction context
rg -n 'Transaction|&tx' crates/app/src/session/trajectory.rsRepository: loongclaw-ai/loongclaw
Length of output: 48
Wrap the export assembly in a single transaction to ensure a consistent artifact snapshot.
The export performs multiple independent persistence reads—metadata, turns, events, outcomes, and approval requests—each opening a fresh connection. Without a transaction boundary, a session mutating between reads can cause the artifact to become internally inconsistent (e.g., summary.turn_count mismatch with actual turns, or lineage changes mid-export). WAL mode does not prevent torn reads across separate statements without an explicit transaction. Wrap the entire sequence in a read-only transaction or pass a single connection through all collection steps to match the pattern already used in replace_turns_internal with TransactionBehavior::Immediate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/app/src/session/trajectory.rs` around lines 161 - 171, The export
assembly opens multiple independent DB connections (calls via
SessionRepository::new then load_session_summary_with_legacy_fallback,
resolve_root_session_id, collect_export_session_summaries,
collect_export_sessions, build_runtime_trajectory_statistics) which can produce
torn/ inconsistent artifacts; wrap the entire sequence that builds
requested_summary, root_session_id, session_summaries, sessions and statistics
in a single read-only transaction (or obtain a single connection/transaction
from SessionRepository and pass it into those collection functions) so all reads
occur under one consistent snapshot (similar to replace_turns_internal with
TransactionBehavior::Immediate).
| let _stdin_guard = StdinGuard; | ||
| init_tracing(); | ||
| let cli = Cli::parse(); | ||
| tracing::debug!(target: "loongclaw.daemon", command = ?cli.command, "parsed CLI command"); |
There was a problem hiding this comment.
Avoid logging full parsed CLI payloads.
Logging ?cli.command can capture user-provided content and identifiers in debug logs. Log only non-sensitive metadata (e.g., whether a subcommand was provided).
🔧 Proposed change
- tracing::debug!(target: "loongclaw.daemon", command = ?cli.command, "parsed CLI command");
+ tracing::debug!(
+ target: "loongclaw.daemon",
+ has_explicit_command = cli.command.is_some(),
+ "parsed CLI command"
+ );📝 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
| tracing::debug!(target: "loongclaw.daemon", command = ?cli.command, "parsed CLI command"); | |
| tracing::debug!( | |
| target: "loongclaw.daemon", | |
| has_explicit_command = cli.command.is_some(), | |
| "parsed CLI command" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/daemon/src/main.rs` at line 42, The debug log currently emits the full
parsed CLI payload via tracing::debug!(..., command = ?cli.command, ...), which
may leak user-provided content; change the log to avoid serializing the entire
cli.command payload and instead emit only non-sensitive metadata — e.g., whether
a subcommand exists or the command variant name. Locate the tracing::debug! call
that references cli.command and replace the argument with a safe value such as a
boolean like cli.command.is_some() or the command variant identifier (derive it
by pattern-matching cli.command to extract only the variant name or use
std::mem::discriminant) so no user data is logged.
Comment on lines
+161
to
+176
| fn load_runtime_trajectory_artifact(path: &Path) -> CliResult<RuntimeTrajectoryArtifactDocument> { | ||
| let raw = fs::read_to_string(path).map_err(|error| { | ||
| format!( | ||
| "read runtime trajectory artifact {} failed: {error}", | ||
| path.display() | ||
| ) | ||
| })?; | ||
| let artifact = | ||
| serde_json::from_str::<RuntimeTrajectoryArtifactDocument>(&raw).map_err(|error| { | ||
| format!( | ||
| "decode runtime trajectory artifact {} failed: {error}", | ||
| path.display() | ||
| ) | ||
| })?; | ||
| Ok(artifact) | ||
| } |
There was a problem hiding this comment.
Validate schema.version/surface/purpose before accepting an artifact.
show currently renders any JSON that deserializes into RuntimeTrajectoryArtifactDocument. That makes the versioned artifact contract unenforced and lets future or foreign artifacts be treated as compatible. ``
Proposed fix
fn load_runtime_trajectory_artifact(path: &Path) -> CliResult<RuntimeTrajectoryArtifactDocument> {
let raw = fs::read_to_string(path).map_err(|error| {
format!(
"read runtime trajectory artifact {} failed: {error}",
path.display()
@@
let artifact =
serde_json::from_str::<RuntimeTrajectoryArtifactDocument>(&raw).map_err(|error| {
format!(
"decode runtime trajectory artifact {} failed: {error}",
path.display()
)
})?;
+ if artifact.schema.version
+ != loongclaw_app::session::trajectory::RUNTIME_TRAJECTORY_ARTIFACT_JSON_SCHEMA_VERSION
+ || artifact.schema.surface.as_str()
+ != loongclaw_app::session::trajectory::RUNTIME_TRAJECTORY_ARTIFACT_SURFACE
+ || artifact.schema.purpose.as_str()
+ != loongclaw_app::session::trajectory::RUNTIME_TRAJECTORY_ARTIFACT_PURPOSE
+ {
+ return Err(format!(
+ "runtime trajectory artifact {} has unsupported schema version={} surface={} purpose={}",
+ path.display(),
+ artifact.schema.version,
+ artifact.schema.surface,
+ artifact.schema.purpose
+ ));
+ }
Ok(artifact)
}📝 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
| fn load_runtime_trajectory_artifact(path: &Path) -> CliResult<RuntimeTrajectoryArtifactDocument> { | |
| let raw = fs::read_to_string(path).map_err(|error| { | |
| format!( | |
| "read runtime trajectory artifact {} failed: {error}", | |
| path.display() | |
| ) | |
| })?; | |
| let artifact = | |
| serde_json::from_str::<RuntimeTrajectoryArtifactDocument>(&raw).map_err(|error| { | |
| format!( | |
| "decode runtime trajectory artifact {} failed: {error}", | |
| path.display() | |
| ) | |
| })?; | |
| Ok(artifact) | |
| } | |
| fn load_runtime_trajectory_artifact(path: &Path) -> CliResult<RuntimeTrajectoryArtifactDocument> { | |
| let raw = fs::read_to_string(path).map_err(|error| { | |
| format!( | |
| "read runtime trajectory artifact {} failed: {error}", | |
| path.display() | |
| ) | |
| })?; | |
| let artifact = | |
| serde_json::from_str::<RuntimeTrajectoryArtifactDocument>(&raw).map_err(|error| { | |
| format!( | |
| "decode runtime trajectory artifact {} failed: {error}", | |
| path.display() | |
| ) | |
| })?; | |
| if artifact.schema.version | |
| != loongclaw_app::session::trajectory::RUNTIME_TRAJECTORY_ARTIFACT_JSON_SCHEMA_VERSION | |
| || artifact.schema.surface.as_str() | |
| != loongclaw_app::session::trajectory::RUNTIME_TRAJECTORY_ARTIFACT_SURFACE | |
| || artifact.schema.purpose.as_str() | |
| != loongclaw_app::session::trajectory::RUNTIME_TRAJECTORY_ARTIFACT_PURPOSE | |
| { | |
| return Err(format!( | |
| "runtime trajectory artifact {} has unsupported schema version={} surface={} purpose={}", | |
| path.display(), | |
| artifact.schema.version, | |
| artifact.schema.surface, | |
| artifact.schema.purpose | |
| )); | |
| } | |
| Ok(artifact) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/daemon/src/runtime_trajectory_cli.rs` around lines 161 - 176, The
deserialized artifact in load_runtime_trajectory_artifact should be validated
for schema.version, surface, and purpose before returning to enforce the
versioned contract; after calling serde_json::from_str in
load_runtime_trajectory_artifact, check that artifact.schema.version ==
"<expected_version>" (or matches allowed versions), artifact.schema.surface ==
"<expected_surface>" and artifact.schema.purpose == "<expected_purpose>" and
return a CliResult::Err (using the same error format used elsewhere) if any
check fails, including the offending values and the path.display() in the error
message so incompatible or foreign artifacts are rejected instead of silently
accepted.
…ate flows The runtime trajectory slice was ready, but full-workspace verification still wasn't trustworthy because two tests were sensitive to machine-local state: Discord channel readiness could silently flip when DISCORD_BOT_TOKEN was set, and migrate CLI UX assertions could be pre-empted by parsing an unrelated existing config before required flag validation ran. This follow-up makes the Discord registry assertion hermetic with the existing ScopedEnv guard and moves migrate required-flag validation ahead of config loading. It also adds regression coverage for missing input/output behavior across apply, discover, and rollback modes so future UX regressions fail before runtime/tool execution begins. Constraint: Preserve existing runtime behavior and fix only the test/CLI ordering seams that made verification nondeterministic Rejected: Change Discord channel readiness semantics to ignore configured env defaults | would break legitimate config-backed readiness and hide real operator state Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep CLI-required-flag validation ahead of config loading for UX-critical migrate modes so local config drift cannot mask argument errors Tested: cargo fmt --all -- --check; cargo clippy --workspace --all-targets --all-features --locked -- -D warnings; cargo test -p loongclaw-app channel::registry::tests::discord_status_splits_config_backed_send_and_stub_serve --locked -- --test-threads=1; cargo test -p loongclaw-daemon integration::migrate_cli::migrate_cli_ux_apply_mode_reports_flag_level_output_requirement --locked -- --exact --test-threads=1; cargo test --workspace --locked; cargo test --workspace --all-features --locked Not-tested: none
The runtime trajectory work uncovered one more verification-only problem: ACPX MCP-proxy tests depended on ambient process environment stability while other tests were free to mutate environment variables under the shared test process. That made the all-features test pass non-deterministic even though the runtime code path itself was correct. This change serializes the affected ACPX tests behind the existing ScopedEnv lock so PATH- and runtime-probe-sensitive assertions do not race unrelated environment-mutating tests. Constraint: Preserve the production ACPX runtime behavior and only harden the test harness Rejected: Add more ad-hoc sleeps/retries inside the test body | would hide the shared-environment race instead of removing it Confidence: high Scope-risk: narrow Reversibility: clean Directive: Any ACPX/browser/runtime test that depends on process-global env or PATH should acquire the shared ScopedEnv lock before probing external commands Tested: cargo fmt --all -- --check; cargo clippy --workspace --all-targets --all-features --locked -- -D warnings; cargo test -p loongclaw-app acp::acpx::tests::runtime_backend_uses_agent_proxy_when_mcp_servers_requested --all-features --locked -- --exact --test-threads=1; cargo test --workspace --locked; cargo test --workspace --all-features --locked; ./scripts/check_architecture_boundaries.sh; ./scripts/check_dep_graph.sh Not-tested: No additional non-Unix ACPX coverage because the guarded tests are already `#[cfg(unix)]`
Collaborator
|
Dispatcher review status: Status: Reconciled findings on the current head:
Executable proof run locally on this head:
External review intake:
Blocking conditions before closure:
|
Collaborator
|
Dispatcher review for current head
Verification run on the reviewed head:
All four current CodeRabbit inline comments were also verified against the live head and classified as valid. This PR is blocked pending fixes for the issues above. |
Collaborator
Author
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
session::trajectoryexport contract that assembles full persisted turn history, canonical turn classification, session events, terminal outcomes, and approval requests.runtime-trajectorycommand family withexportandshowsubcommands plus integration coverage and a public product spec.migrate_cli, Discord channel-status test coverage, ACPX MCP-proxy tests) so repo-wide default and all-features verification is deterministic on this branch.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:
Process-global env / PATH stability:
DISCORD_BOT_TOKENunderScopedEnvso it no longer depends on ambient local credentials.ScopedEnvlock before probing command/runtime behavior so they do not race other tests that temporarily mutate PATH or related process-global environment.migrate_clinow validates required flags before loading config, so CLI-level UX errors are deterministic even when a local default config is stale or malformed.User-visible / Operator-visible Changes
loongclaw runtime-trajectory exportloongclaw runtime-trajectory showFailure Recovery
runtime-trajectorycommand family and the new app-layer export module; existing persisted session data remains untouched.Reviewer Focus
crates/app/src/session/trajectory.rscrates/app/src/memory/sqlite.rscrates/app/src/session/repository.rscrates/daemon/src/runtime_trajectory_cli.rscrates/daemon/tests/integration/runtime_trajectory_cli.rscrates/daemon/src/migrate_cli.rs,crates/daemon/tests/integration/migrate_cli.rs,crates/app/src/channel/registry.rs,crates/app/src/acp/acpx.rs