feat(agent-runtime): add dream system consolidation#688
Conversation
Deploying corvus with
|
| Latest commit: |
2dec988
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://55d12d8e.corvus-42x.pages.dev |
| Branch Preview URL: | https://feat-dream-system-consolidat.corvus-42x.pages.dev |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 23 minutes and 49 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughImplements the Dream System for long-term memory consolidation by introducing per-session dream state management with SQLite persistence and markdown sidecar snapshots. Gateway completion now records session_id before dream evaluation. Hygiene auto-closes stale sessions triggering dream eligibility checks. Adds configurable session/time thresholds and comprehensive specification documents. Changes
Sequence DiagramsequenceDiagram
participant Gateway
participant Memory/Dream
participant SQLite
participant Markdown
Gateway->>Memory/Dream: finalize_generated_session_if_needed(session_id)
activate Memory/Dream
Memory/Dream->>Memory/Dream: record_session_completion(session_id)
Memory/Dream->>SQLite: upsert dream_sessions (Pending)
Memory/Dream->>Markdown: append summary line to MEMORY.md
Memory/Dream-->>Gateway: completion recorded
deactivate Memory/Dream
Gateway->>Memory/Dream: run_if_triggered_with_config()
activate Memory/Dream
Memory/Dream->>Memory/Dream: dream_eligibility(session_id)
alt Eligible
Memory/Dream->>SQLite: query pending/failed sessions
Memory/Dream->>Markdown: consolidate, deduplicate, prune
Memory/Dream->>SQLite: mark Completed, emit artifacts
Memory/Dream->>Markdown: export DREAM_STATE_SNAPSHOT.json
else Not Completed or Already Consolidated
Memory/Dream-->>Gateway: None (no consolidation)
end
deactivate Memory/Dream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-04-26 to 2026-04-26 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
clients/agent-runtime/src/memory/dream.rs (4)
573-732: 🧹 Nitpick | 🔵 TrivialTests cover happy path; please add one for the failure modes flagged above.
The new test suite is solid for eligibility, dedup, and lock contention, but doesn't exercise:
- corrupted
dream_state.jsonrecovery,- prefix-colliding session ids (e.g.
sess-1andsess-12written sequentially),prune_to_budgetevicting a just-added session summary,- concurrent
record_session_completioncalls (usestd::thread::scope).Each one maps directly to a real bug surfaced in this review; locking them in as regression tests would be cheap insurance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/dream.rs` around lines 573 - 732, Add new unit tests in the tests module that assert the described failure modes: create a corrupted DREAM_STATE_FILE and verify run_now recovers and marks affected sessions as Failed/Retry (use record_session_completion, DREAM_STATE_FILE, run_now and inspect DreamState); write sequential sessions with prefix collisions like "sess-1" and "sess-12" and assert both are stored and deduplicated correctly via dream_eligibility and run_if_triggered; add a test that triggers prune_to_budget by creating enough completed sessions to exceed budget and verify a just-added session isn't immediately evicted (use record_session_completion, run_now, and inspect resulting state/session_reports); and add a concurrency test using std::thread::scope that calls record_session_completion concurrently and asserts no lost or corrupted entries and deterministic session_ids (use record_session_completion, DreamLockGuard, and dream_eligibility to validate).
196-256:⚠️ Potential issue | 🟠 MajorSessions can be marked
Completedeven when their summary line was pruned fromMEMORY.md.
run_nowpushes per-session summary lines intomerged_lines(L201), thenprune_to_budgetmay drop them at L221 (line/byte budget enforced from the tail), yetmark_sessions_completedat L240 unconditionally flips every session insession_idstoCompletedwith the deterministicartifact_ref. Result:dream_eligibilityreturnsAlreadyConsolidatedwhile no corresponding- Dream summary for completed session …line exists on disk — also breaking the prefix-detection logic inrecord_session_completionfor any subsequent retry.Either (a) verify each session's line survived pruning before marking
Completed(mark the rest asFailed/Pendingfor next run), or (b) reserve budget for session summaries before pruning unrelated history.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/dream.rs` around lines 196 - 256, The code currently marks every session in session_ids as Completed regardless of whether its summary line survived dedupe/prune; change run_now so you determine which sessions actually remain in the final pruned lines and only pass those to mark_sessions_completed (keep artifact_ref_for_session and session_reports in sync): e.g., build a mapping from session_id -> its expected summary string before dedupe/prune, run dedupe_preserving_order/prune_to_budget, then filter session_ids/artifact_refs/session_reports to only the ones whose summary string appears in pruned (and mark the others as Pending/Failed or leave out so they can be retried); alternatively implement pre-reservation of lines for session summaries before pruning, but do not unconditionally call mark_sessions_completed for all original session_ids.
523-530:⚠️ Potential issue | 🟠 MajorSilent state corruption —
unwrap_or_default()discards every recorded session.If
dream_state.jsonbecomes invalid (partial write, schema drift, disk corruption),serde_json::from_str(&raw).unwrap_or_default()returnsDreamState::default()and the nextstore_stateoverwrites the file, permanently erasing allcompleted_sessions. The very idempotency state the spec requires for "no duplicate Dream consolidation" is the thing being silently nuked.Propagate the parse error (or quarantine the file to
dream_state.json.corrupt-<ts>and surface a warning) so corruption doesn't masquerade as a clean slate.🛡️ Propagate parse errors
- let raw = fs::read_to_string(path)?; - Ok(serde_json::from_str(&raw).unwrap_or_default()) + let raw = fs::read_to_string(&path)?; + serde_json::from_str(&raw).map_err(|err| { + anyhow::anyhow!("dream_state.json at {} is corrupt: {err}", path.display()) + })As per coding guidelines: "Flag unnecessary clones, unchecked panics in production paths, and weak error context."
545-571:⚠️ Potential issue | 🟠 Major
dream.lockwedges indefinitely after process crash — no stale-holder recovery.If the process is
SIGKILL'd, panics without cleanup, or the host reboots mid-run, the lockfile persists. Every subsequentrun_nowcall sees the file exists and returnsOk(None)(Skipped/Busy) forever. The currentDreamLockGuardusescreate_new(true)plusDrop-timeremove_file, with no stale detection.
fs2is already a dependency; usefs2::FileExt::try_lock_exclusiveto let the OS release the lock automatically on process death. Alternatively, persist{pid, timestamp}in the lockfile and treat locks older than a configurable threshold or with dead PIDs as stale. Either approach prevents permanent wedges.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/dream.rs` around lines 545 - 571, The current DreamLockGuard (struct DreamLockGuard and its acquire method) creates a lockfile with create_new(true) and relies on Drop removing the file, which wedges if the process dies; change DreamLockGuard to hold the opened File (not just PathBuf) and use fs2::FileExt::try_lock_exclusive on that File when creating/opening dream_lock_path(workspace_dir) so the OS auto-releases locks if the process dies; update acquire to open (create if needed) the file, attempt try_lock_exclusive and return Ok(Some(Self { file })) on success, treat AlreadyLocked as Ok(None), and remove the file-removal logic from Drop (or only unlink if lock held) so stale-file scenarios are avoided — alternatively implement writing {pid,timestamp} into the file and treat older/dead-PID locks as stale in acquire if you prefer that approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/gateway/mod.rs`:
- Line 1704: The log call tracing::debug!("dream session completion recording
failed: {error}"); lacks session context—update the call to include the
session_id (use the local variable named session_id) so failures can be
correlated; prefer structured fields (e.g., include session_id and error as
fields and a short message) when updating the tracing::debug invocation in the
function that records dream session completion.
In `@clients/agent-runtime/src/memory/dream.rs`:
- Around line 12-13: The hardcoded Dream triggers (DREAM_SESSION_TRIGGER_COUNT
and DREAM_TIME_TRIGGER_HOURS) must be made configurable via the existing
configuration layer: add a DreamTriggerConfig struct with session_count and
time_hours (defaulting to 5 and 24), wire it into the RuntimeConfig/workspace
config snapshot used in gateway/mod.rs, replace direct uses of
DREAM_SESSION_TRIGGER_COUNT and DREAM_TIME_TRIGGER_HOURS in
clients/agent-runtime/src/memory/dream.rs with reads from the injected
DreamTriggerConfig (or pass the config into functions where those constants are
referenced), and add a unit test that loads a non-default DreamTriggerConfig to
assert the code honors the overridden thresholds. Ensure the new config has
reasonable defaults and is surfaced to operators via the same config plumbing
used elsewhere (e.g., RuntimeConfig).
- Around line 274-282: artifact_already_written currently checks for a substring
which can collide on prefix IDs; update the check to look for an exact line
match instead of contains: compute the expected line string (the same format
used now) and replace the content.contains(...) logic in the
artifact_already_written computation with something like reading the file and
using lines().any(|l| l.trim() == expected_line) or a regex anchored to ^ and $;
keep using core_path, session_id and the existing
read_to_string().map(...).unwrap_or(false) flow but change the inner predicate
to exact line equality (or anchored regex) so only the exact "- Dream summary
for completed session {session_id}" line marks the artifact as written.
- Around line 391-413: The parameter artifact_refs on mark_sessions_completed is
unused (it gets ignored and replaced by artifact_ref_for_session) — remove
artifact_refs from the function signature and delete the dead let _ =
artifact_refs; line, then update all call sites that currently pass
&artifact_refs to call mark_sessions_completed with the remaining arguments;
alternatively, if you prefer to use the prebuilt artifact_refs, set
record.artifact_refs = artifact_refs.clone() instead of calling
artifact_ref_for_session(session_id) (update whichever approach you choose
consistently across mark_sessions_completed and its callers).
- Around line 266-325: record_session_completion performs an unprotected
read-modify-write using load_state and store_state and must be serialized to
avoid lost concurrent updates; acquire the same DreamLockGuard used by run_now
at the start of record_session_completion (or protect the shared state with a
module-level Mutex<DreamState>) so the function holds the lock for the duration
of loading, mutating, and storing state; ensure the lock is released after
store_state and keep existing behavior for artifact_already_written and existing
session handling unchanged.
In `@clients/agent-runtime/src/memory/hygiene.rs`:
- Around line 386-390: The loop after drop(conn) may propagate a single
record_session_completion(workspace_dir, session_id)? error and abort, leaving
later sessions updated in DB but not recorded; change the loop over
stale_ids.iter().take(affected) to call memory::record_session_completion inside
a try/catch-like handling (i.e., handle Result) that logs the error with
session_id and continues, while optionally collecting failures into a Vec to
return or surface after the loop; reference drop(conn), stale_ids, affected,
record_session_completion, and workspace_dir when implementing the
continue-on-error behavior and final aggregated handling.
- Around line 364-390: The current SELECT/UPDATE/take(affected) sequence in
hygiene.rs can produce TOCTOU bugs because stale_ids (from the SELECT) may
differ from the rows actually updated by conn.execute, and partial failures
calling memory::record_session_completion can leave DB updated but completions
unrecorded; fix by either (A) starting an IMMEDIATE transaction around the
SELECT and UPDATE to freeze the snapshot (begin immediate; perform SELECT into
stale_ids; perform UPDATE; commit) before calling
memory::record_session_completion, or (B) replace the two-step SELECT+UPDATE
with a single UPDATE ... RETURNING id so you obtain exactly the set of updated
session IDs (use the returned IDs instead of stale_ids and remove the separate
take(affected) logic); additionally ensure atomicity for completion recording by
either performing the completions inside the same transaction or by continuing
on errors while collecting/reporting failures so partial progress is handled
consistently (adjust code around conn.prepare, conn.execute, stale_ids,
take(affected), and memory::record_session_completion).
In `@clients/agent-runtime/src/memory/snapshot.rs`:
- Line 175: The call to restore_dream_state_snapshot should be best-effort and
not make snapshot hydration fail; wrap the
restore_dream_state_snapshot(workspace_dir)? invocation so any error is caught
and logged as a warning (e.g., if let Err(e) =
restore_dream_state_snapshot(workspace_dir) { warn!(... , ?e); }) and do not
propagate the error, leaving hydrate_from_snapshot()'s success determination to
the core memory restore flow; update the call site in snapshot.rs to handle
Result from restore_dream_state_snapshot without using the ? operator and use
the project's logging (log::warn or tracing::warn) to record the error.
- Line 85: The call to export_dream_state_snapshot should be moved earlier in
export_snapshot so it runs even when the early-return branch is taken;
specifically, relocate the export_dream_state_snapshot(workspace_dir)?
invocation out of the current late position and place it before the early-return
checks in the export_snapshot function (or call it unconditionally at the top of
export_snapshot after validating workspace_dir) so DREAM_STATE_SNAPSHOT.json is
written even if there are no core memories; ensure you reference the
export_snapshot function and the export_dream_state_snapshot symbol when making
the change.
- Around line 236-245: The current restore_dream_state_snapshot function returns
early when parse_dream_state_snapshot(&raw) yields an empty records vector,
which drops important sidecar metadata (e.g., last_successful_run_at,
last_report). Remove the early return on records.is_empty() and instead proceed
to hydrate the Dream state from the parsed snapshot (even if completed_sessions
is empty): keep reading raw, call parse_dream_state_snapshot, then apply the
metadata fields from the parsed record(s) into the runtime state (using the same
code path you have for non-empty snapshots) so that metadata is preserved;
ensure any logic that expects completed_sessions handles an empty list safely.
In `@openspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.md`:
- Around line 485-495: The spec currently omits a stale-lock recovery strategy
for dream.lock (as referenced in ADR-5 and sections §6.4 and §11); update the
design to either define an automatic stale-lock breaker (e.g., record holder PID
+ heartbeat or use OS-level flock semantics with mtime+PID fallback and a safe
reclaim policy) or explicitly document the operator recovery procedure (delete
dream.lock) and include detection/validation steps, safety checks, and where to
log/alert when reclaiming the lock; mention “dream.lock”, ADR-5, §6.4, and §11
so reviewers can find the change.
In
`@openspec/changes/archive/2025-08-07-2026-04-26-dream-system/verify-report.md`:
- Around line 94-118: Table cells containing raw pipe characters (e.g.,
"pending|running|completed|failed" and "dream/session/{session_id}") are
breaking the Markdown table and triggering MD056; update those cells in the
archive to escape pipes with backslashes (e.g.,
pending\|running\|completed\|failed and dream/session\{session_id\}) or replace
them with a comma-separated or worded form so the table columns remain intact;
ensure references like `dream_eligibility`, `record_session_completion`,
`DreamEligibility::NotCompleted`, `artifact_ref_for_session`, and the `Dream
summary for completed session {session_id}` wording in `memory/dream.rs` and
`memory/markdown.rs` remain accurate after the escape or rephrase.
---
Outside diff comments:
In `@clients/agent-runtime/src/memory/dream.rs`:
- Around line 573-732: Add new unit tests in the tests module that assert the
described failure modes: create a corrupted DREAM_STATE_FILE and verify run_now
recovers and marks affected sessions as Failed/Retry (use
record_session_completion, DREAM_STATE_FILE, run_now and inspect DreamState);
write sequential sessions with prefix collisions like "sess-1" and "sess-12" and
assert both are stored and deduplicated correctly via dream_eligibility and
run_if_triggered; add a test that triggers prune_to_budget by creating enough
completed sessions to exceed budget and verify a just-added session isn't
immediately evicted (use record_session_completion, run_now, and inspect
resulting state/session_reports); and add a concurrency test using
std::thread::scope that calls record_session_completion concurrently and asserts
no lost or corrupted entries and deterministic session_ids (use
record_session_completion, DreamLockGuard, and dream_eligibility to validate).
- Around line 196-256: The code currently marks every session in session_ids as
Completed regardless of whether its summary line survived dedupe/prune; change
run_now so you determine which sessions actually remain in the final pruned
lines and only pass those to mark_sessions_completed (keep
artifact_ref_for_session and session_reports in sync): e.g., build a mapping
from session_id -> its expected summary string before dedupe/prune, run
dedupe_preserving_order/prune_to_budget, then filter
session_ids/artifact_refs/session_reports to only the ones whose summary string
appears in pruned (and mark the others as Pending/Failed or leave out so they
can be retried); alternatively implement pre-reservation of lines for session
summaries before pruning, but do not unconditionally call
mark_sessions_completed for all original session_ids.
- Around line 545-571: The current DreamLockGuard (struct DreamLockGuard and its
acquire method) creates a lockfile with create_new(true) and relies on Drop
removing the file, which wedges if the process dies; change DreamLockGuard to
hold the opened File (not just PathBuf) and use fs2::FileExt::try_lock_exclusive
on that File when creating/opening dream_lock_path(workspace_dir) so the OS
auto-releases locks if the process dies; update acquire to open (create if
needed) the file, attempt try_lock_exclusive and return Ok(Some(Self { file }))
on success, treat AlreadyLocked as Ok(None), and remove the file-removal logic
from Drop (or only unlink if lock held) so stale-file scenarios are avoided —
alternatively implement writing {pid,timestamp} into the file and treat
older/dead-PID locks as stale in acquire if you prefer that approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef8bbb6f-93dc-480a-80d0-6850524239a8
📒 Files selected for processing (17)
clients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/memory/dream.rsclients/agent-runtime/src/memory/hygiene.rsclients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/memory/snapshot.rsclients/agent-runtime/src/memory/sqlite.rsopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/proposal.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/specs/gateway-integration/spec.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/specs/gateway/spec.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/specs/sessions/spec.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/state.yamlopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/tasks.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/verify-report.mdopenspec/config.yamlopenspec/specs/gateway/spec.mdopenspec/specs/sessions/spec.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: sonar
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{md,mdx}
⚙️ CodeRabbit configuration file
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
For user-facing docs, check EN/ES parity or explicitly note pending translation gaps.
Files:
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/specs/gateway-integration/spec.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/specs/gateway/spec.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/tasks.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/verify-report.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/proposal.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/specs/sessions/spec.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.mdopenspec/specs/sessions/spec.mdopenspec/specs/gateway/spec.md
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/specs/gateway-integration/spec.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/state.yamlopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/specs/gateway/spec.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/tasks.mdopenspec/config.yamlopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/verify-report.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/proposal.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/specs/sessions/spec.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.mdclients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/memory/hygiene.rsopenspec/specs/sessions/spec.mdclients/agent-runtime/src/memory/sqlite.rsopenspec/specs/gateway/spec.mdclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/memory/snapshot.rsclients/agent-runtime/src/memory/dream.rs
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/memory/hygiene.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/memory/snapshot.rsclients/agent-runtime/src/memory/dream.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/memory/hygiene.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/memory/snapshot.rsclients/agent-runtime/src/memory/dream.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/memory/hygiene.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/memory/snapshot.rsclients/agent-runtime/src/memory/dream.rs
clients/agent-runtime/src/{security,gateway,tools}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Treat
src/security/,src/gateway/,src/tools/as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Files:
clients/agent-runtime/src/gateway/mod.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/gateway/mod.rs
🧠 Learnings (6)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/tasks.mdopenspec/config.yamlclients/agent-runtime/src/gateway/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/.github/**/*.{yml,yaml} : For workflow/template-only changes, ensure YAML/template syntax validity
Applied to files:
openspec/config.yaml
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Include threat/risk notes and rollback strategy for security, runtime, and gateway changes; add or update tests for boundary checks and failure modes
Applied to files:
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.md
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Applied to files:
clients/agent-runtime/src/memory/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/gateway/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
Applied to files:
clients/agent-runtime/src/gateway/mod.rs
🪛 LanguageTool
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/verify-report.md
[style] ~138-~138: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...me session_id in one runtime test. 3. Add a gateway-facing test or assertion set ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/proposal.md
[style] ~130-~130: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...s/agent-runtime/src/memory/dream.rs`. - Existing memory backend support for SQLite, mark...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.md
[style] ~204-~204: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rtifacts through the active backend. 8. Dream writes replay metadata marking the sess...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~312-~312: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...storing user-facing long-term memory in markdown while keeping replay metadata separate ...
(MARKDOWN_NNP)
[style] ~479-~479: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... as busy depending on lock ownership. - If pending/failed, attempt determinist...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.22.1)
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/verify-report.md
[warning] 99-99: Table column count
Expected: 3; Actual: 6; Too many cells, extra data will be missing
(MD056, table-column-count)
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/proposal.md
[warning] 32-32: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 41-41: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 87-87: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 92-92: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 96-96: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 100-100: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (7)
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/state.yaml (1)
1-6: This state.yaml uses an incomplete and non-standard schema relative to other archived changes.The file lives in
archive/and has averify-report.md, yetphase: proposewith only[explore, propose]completed. Additionally, the schema differs from other archived changes which usecurrent_phase,completed,next, andupdatedfields. Compare to2026-04-02-runtime-sandbox-hardening(also archived and verified):change: runtime-sandbox-hardening current_phase: archive completed: [ explore, propose, spec, design, tasks, apply, verify, archive ] next: none updated: 2026-04-02Update the schema and phase metadata to match the established pattern for archived changes.
clients/agent-runtime/src/gateway/mod.rs (2)
3624-3627: Good test setup extraction.Nice small helper to keep Dream-flow tests focused on behavior instead of repetitive session setup.
3709-3823: Strong regression coverage for Dream ordering/idempotency.These tests exercise the key runtime contract transitions (
NotCompleted→AlreadyConsolidated) and protect repeat-finalization behavior.openspec/changes/archive/2025-08-07-2026-04-26-dream-system/specs/sessions/spec.md (1)
1-76: Sessions delta spec is internally consistent.The completion-first ordering and idempotency requirements line up cleanly with the hygiene impl snippet (
close_stale_sessionsrecords completion per stale id) and with the runtime contract enforced indream.rs. No technical-accuracy gaps spotted.openspec/changes/archive/2025-08-07-2026-04-26-dream-system/specs/gateway/spec.md (1)
1-114: Archived gateway delta is aligned with runtime impl.Trigger-only gateway, completion-before-evaluation, and per-session idempotency all map to the
finalize_generated_session_if_neededsequencing anddream_eligibilitychecks. Nothing to flag.openspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.md (1)
1-689: Design doc otherwise reads cleanly.LanguageTool flags at L204/L312/L479 are stylistic and not worth churn. The design's contract — completion-recorded-first, per-session state machine, sidecar replay metadata — is consistent with the implementation under review.
openspec/specs/gateway/spec.md (1)
2381-2528: Synced Dream requirements match the runtime contract.The eligibility/idempotency/persistence/trigger-only blocks are byte-aligned with the archived delta and with
dream_eligibility+finalize_generated_session_if_neededbehavior. No spec/code drift.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
clients/agent-runtime/src/memory/dream.rs (2)
138-160: 🧹 Nitpick | 🔵 TrivialSkipped/Busy report doesn't reflect pending sessions.
When the lock is busy, the early-return report sets
sessions_considered: 0andsessions_processed: 0. Operators inspecting the skipped report can't tell why the run was skipped (was there nothing to do, or was the lock held with N pending sessions waiting?). Consider populatingsessions_consideredfrompending_session_ids(&load_state(...))even on the busy path so the busy/skipped report carries useful triage info.Trade-off: an extra
load_stateread on the busy path. Cheap, but flags this as optional rather than required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/dream.rs` around lines 138 - 160, When DreamLockGuard::acquire(...) returns None (the busy/early-return path in run_now), populate the MemoryConsolidationReport's sessions_considered (and optionally sessions_processed) by reading the current state and counting pending sessions instead of zeroing them; call load_state(workspace_dir) (or pending_session_ids(&load_state(...))) to compute pending count and set sessions_considered to that value and sessions_processed appropriately so the skipped/busy report reflects how many sessions were waiting.
552-559:⚠️ Potential issue | 🟠 Major🔴 Silent fallback corrupts Dream idempotency on a single bad byte.
serde_json::from_str(&raw).unwrap_or_default()swallows any deserialization error and hands back an emptyDreamState. A single truncated write, schema drift, or partially-flushed file makes the runtime "forget" every recordedcompleted_sessionsentry — every prior session immediately becomesNotCompleted, time/count triggers re-fire, andrecord_session_completionre-creates records that overwrite the (still-on-disk) state on the nextstore_statecall. That permanently destroys the corrupted history rather than letting an operator inspect or restore fromDREAM_STATE_SNAPSHOT.json.🛡️ Fail loud, then let the snapshot/restore path recover
-fn load_state(workspace_dir: &Path) -> Result<DreamState> { - let path = dream_state_path(workspace_dir); - if !path.exists() { - return Ok(DreamState::default()); - } - let raw = fs::read_to_string(path)?; - Ok(serde_json::from_str(&raw).unwrap_or_default()) -} +fn load_state(workspace_dir: &Path) -> Result<DreamState> { + let path = dream_state_path(workspace_dir); + if !path.exists() { + return Ok(DreamState::default()); + } + let raw = fs::read_to_string(&path)?; + serde_json::from_str(&raw).map_err(|e| { + anyhow::anyhow!( + "dream state at {} is corrupted: {e}; restore from {} or delete to reset", + path.display(), + DREAM_SNAPSHOT_STATE_FILENAME + ) + }) +}As per coding guidelines: "prefer explicit errors over silent fallback for unsupported critical paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/dream.rs` around lines 552 - 559, The current load_state function silently swallows JSON deserialization errors via serde_json::from_str(&raw).unwrap_or_default(), causing loss of persisted state; change load_state (and references to dream_state_path) to fail loudly on malformed or truncated JSON by propagating or returning the serde_json::from_str error instead of falling back to DreamState::default(), e.g., replace the unwrap_or_default with proper ?/map_err to return an Err with context so operators can inspect DREAM_STATE_SNAPSHOT.json and use the snapshot/restore path rather than losing history when store_state later overwrites disk.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/config/schema.rs`:
- Around line 1629-1635: The DreamTriggerConfig struct allows non-positive
values that can cause continuous triggering; add runtime validation ensuring
session_count > 0 and time_hours > 0. Implement a validate method on
DreamTriggerConfig (or a From/TryFrom/Validate trait impl used by your config
loader) that checks session_count and time_hours and returns an error if they
are invalid, and ensure the config deserialization/initialization path calls
this validation (or use serde custom deserializer for DreamTriggerConfig to
enforce positive values at parse time). Reference: DreamTriggerConfig,
session_count, time_hours.
In `@clients/agent-runtime/src/memory/dream.rs`:
- Around line 355-370: The current dream_eligibility function returns
DreamEligibility::Eligible for DreamSessionStatus::Failed which is ambiguous;
add a new enum variant (e.g., DreamEligibility::RetryAfterFailure) to the
DreamEligibility enum and update dream_eligibility to return RetryAfterFailure
when the matched record has status DreamSessionStatus::Failed (keep Pending ->
Eligible, Completed -> AlreadyConsolidated); also update any call sites or docs
that consume DreamEligibility to handle the new variant.
- Around line 295-353: The function record_session_completion currently calls
DreamLockGuard::acquire_blocking which blocks the Tokio thread; change
record_session_completion to async and move the blocking work (the lock
acquisition and the file/FS-heavy state load/store and MEMORY.md checks) into a
tokio::task::spawn_blocking closure so the async
gateway::finalize_generated_session_if_needed can await it without stalling the
runtime; update the signature to async fn record_session_completion(...) ->
Result<DreamSessionStateRecord>, perform the existing synchronous logic inside
the spawn_blocking closure (including use of DreamLockGuard::acquire_blocking,
load_state, store_state, and artifact_ref_for_session), and adjust all callers
(notably finalize_generated_session_if_needed) to .await the new async function.
In `@clients/agent-runtime/src/memory/hygiene.rs`:
- Around line 589-614: The test name
close_stale_sessions_records_dream_completion_input is misleading and should be
renamed to reflect the assertion; change the test function name to
close_stale_sessions_marks_session_dream_eligible and keep the #[tokio::test]
attribute because SqliteMemory::upsert_session(...).await is used; ensure
references to close_stale_sessions and crate::memory::dream_eligibility remain
unchanged so the test still calls close_stale_sessions(workspace, 24) and
asserts DreamEligibility::Eligible for the "stale-dream-sess" session.
In `@clients/agent-runtime/src/memory/snapshot.rs`:
- Around line 217-247: In export_dream_state_snapshot, avoid silently swallowing
remove_file errors: replace the throwaway let _ = fs::remove_file(snapshot_path)
with an explicit match on the error (match fs::remove_file(...) { Ok(_) => {},
Err(e) if e.kind() == ErrorKind::NotFound => {}, Err(e) => debug!(... , e) }) so
non-NotFound failures (e.g. EACCES) are logged at debug; and to preserve
byte-stable sidecar snapshots, after validating JSON with
serde_json::from_str(&raw) keep the parse for sanity but write a byte-identical
snapshot by using fs::copy(state_path, snapshot_path) (or write raw bytes
unchanged) instead of re-serializing via serde_json::to_vec_pretty(&json) in
export_dream_state_snapshot; keep restore_dream_state_snapshot behavior
unchanged.
- Around line 271-280: The parse_dream_state_snapshot function is actually only
used to validate JSON input (restore_dream_state_snapshot writes the original
raw string back), so rename parse_dream_state_snapshot to
validate_dream_state_snapshot, change its signature to return Result<()> and
drop the Vec<DreamSessionStateRecord> return (or alternatively keep the Vec but
add a clear doc comment stating it's only for test round-trip assertions);
update the function body to deserialize into DreamStateEnvelope as before but
return Ok(()) on success, update any callers/tests to call the new
validate_dream_state_snapshot (or to accept the documented semantics if you keep
the Vec), and adjust associated doc comments referencing
parse_dream_state_snapshot and restore_dream_state_snapshot to avoid implying
the parsed records are persisted.
In `@openspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.md`:
- Around line 487-492: The spec references a lockfile named "dream.lock" but the
runtime currently uses the constant "dream_state.json" as the lock artifact (the
string constant named "dream_state.json" in the Dream runtime), causing
dangerous mismatch; choose one canonical artifact name and make code and docs
consistent: either rename the runtime constant to "dream.lock" (and update any
code paths that read/write that constant and recovery logic in the Dream
runtime), or update this design doc to reference "dream_state.json" and adjust
the stale-lock recovery wording to match the runtime behavior/log fields; ensure
the chosen name is used everywhere (spec text, any logging messages that mention
the file name, and the recovery validation guidance) so manual reclaim
instructions and logs unambiguously point to the same file.
In
`@openspec/changes/archive/2025-08-07-2026-04-26-dream-system/verify-report.md`:
- Around line 79-89: The document's summary line "Compliance summary: 10/17
scenarios compliant, 7 partial, 0 failing, 0 untested." is inconsistent with the
"Remaining partial scenarios" list that contains only six items; update the file
by either adding the missing partial scenario to the "Remaining partial
scenarios" list (ensuring its title and brief rationale match the others) or by
correcting the count in the "Compliance summary" line to "6 partial" so the
header and the list are internally consistent; edit the lines containing the
"Compliance summary" string and the "Remaining partial scenarios" section to
keep wording and formatting consistent with existing items.
---
Outside diff comments:
In `@clients/agent-runtime/src/memory/dream.rs`:
- Around line 138-160: When DreamLockGuard::acquire(...) returns None (the
busy/early-return path in run_now), populate the MemoryConsolidationReport's
sessions_considered (and optionally sessions_processed) by reading the current
state and counting pending sessions instead of zeroing them; call
load_state(workspace_dir) (or pending_session_ids(&load_state(...))) to compute
pending count and set sessions_considered to that value and sessions_processed
appropriately so the skipped/busy report reflects how many sessions were
waiting.
- Around line 552-559: The current load_state function silently swallows JSON
deserialization errors via serde_json::from_str(&raw).unwrap_or_default(),
causing loss of persisted state; change load_state (and references to
dream_state_path) to fail loudly on malformed or truncated JSON by propagating
or returning the serde_json::from_str error instead of falling back to
DreamState::default(), e.g., replace the unwrap_or_default with proper ?/map_err
to return an Err with context so operators can inspect DREAM_STATE_SNAPSHOT.json
and use the snapshot/restore path rather than losing history when store_state
later overwrites disk.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7506947d-6b1e-4238-9b71-3b821942fcd7
📒 Files selected for processing (9)
clients/agent-runtime/src/config/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/memory/dream.rsclients/agent-runtime/src/memory/hygiene.rsclients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/memory/snapshot.rsopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/verify-report.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (7)
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/config/mod.rsclients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/memory/hygiene.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/memory/snapshot.rsclients/agent-runtime/src/memory/dream.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/config/mod.rsclients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/memory/hygiene.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/memory/snapshot.rsclients/agent-runtime/src/memory/dream.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/config/mod.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/config/schema.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/config/mod.rsclients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/memory/hygiene.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/memory/snapshot.rsclients/agent-runtime/src/memory/dream.rs
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/agent-runtime/src/config/mod.rsopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.mdclients/agent-runtime/src/memory/mod.rsopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/verify-report.mdclients/agent-runtime/src/memory/hygiene.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/memory/snapshot.rsclients/agent-runtime/src/memory/dream.rs
**/*.{md,mdx}
⚙️ CodeRabbit configuration file
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
For user-facing docs, check EN/ES parity or explicitly note pending translation gaps.
Files:
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/verify-report.md
clients/agent-runtime/src/{security,gateway,tools}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Treat
src/security/,src/gateway/,src/tools/as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Files:
clients/agent-runtime/src/gateway/mod.rs
🧠 Learnings (4)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
clients/agent-runtime/src/config/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Include threat/risk notes and rollback strategy for security, runtime, and gateway changes; add or update tests for boundary checks and failure modes
Applied to files:
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.md
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/memory/hygiene.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/memory/snapshot.rsclients/agent-runtime/src/memory/dream.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Applied to files:
clients/agent-runtime/src/memory/mod.rsclients/agent-runtime/src/memory/hygiene.rs
🪛 LanguageTool
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.md
[style] ~204-~204: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rtifacts through the active backend. 8. Dream writes replay metadata marking the sess...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~312-~312: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...storing user-facing long-term memory in markdown while keeping replay metadata separate ...
(MARKDOWN_NNP)
[style] ~479-~479: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... as busy depending on lock ownership. - If pending/failed, attempt determinist...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/verify-report.md
[style] ~138-~138: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...me session_id in one runtime test. 3. Add a gateway-facing test or assertion set ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (12)
openspec/changes/archive/2025-08-07-2026-04-26-dream-system/verify-report.md (1)
99-99: Good fix on Markdown table pipe escaping.Escaping
pending\|running\|completed\|failedkeeps the table structure valid and resolves the prior MD056 rendering risk.As per coding guidelines,
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.openspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.md (1)
569-635: Strong coverage for failure modes, tests, and rollback path.The design now explicitly documents boundary/failure tests plus rollout/rollback behavior in a way that is actionable for runtime and ops.
Based on learnings: Include threat/risk notes and rollback strategy for security, runtime, and gateway changes; add or update tests for boundary checks and failure modes.
Also applies to: 660-692
clients/agent-runtime/src/config/mod.rs (1)
8-16: Re-export and default coverage for Dream config look good.
DreamTriggerConfigis now part of the public config surface, and the added tests pin default values andRuntimeConfigconstruction with the new field.Also applies to: 128-159
clients/agent-runtime/src/gateway/mod.rs (3)
1703-1708: Structured Dream failure logging now includes session context — good fix.This improves correlation under concurrency and keeps the log payload non-sensitive.
3628-3631: Test helper sets up session lifecycle preconditions cleanly.Creating and ending the session in one helper keeps the Dream tests deterministic.
3713-3827: Dream completion tests provide solid regression coverage.The new cases cover completion ordering, “not completed” gating, and repeat-call idempotency for generated sessions.
clients/agent-runtime/src/memory/hygiene.rs (1)
364-408: LGTM — addresses prior TOCTOU and partial-failure concerns.The transaction-wrapped SELECT+UPDATE plus the warn-and-continue loop on
record_session_completionfailures is a solid resolution to the previously raised hazards. WAL snapshot isolation keepsupdated_idsconsistent with the rows the UPDATE actually mutates, so reportingupdated_ids.len()is accurate.One small note: in the failure branch (Line 405) you log the failed session list, but the function still returns
Ok— meaning closures recorded as "successful" can include sessions whose Dream completion was never persisted. That's likely intentional (best-effort hygiene), but worth confirming the snapshot/restore path on a subsequent run can re-converge those sessions toPendingviarecord_session_completionagain, since the DB no longer marks them'active'.clients/agent-runtime/src/memory/mod.rs (1)
21-26: LGTM — re-exports align with new public surface.The new symbols (
dream_eligibility,DreamConfig,DreamEligibility,DreamSessionReport,DreamSessionStateRecord,DreamSessionStatus) are all consumed bygateway/mod.rsandmemory/hygiene.rs. No issues.clients/agent-runtime/src/memory/dream.rs (4)
305-310: LGTM — substring-collision fix landed correctly.
content.lines().any(|line| line.trim() == expected_line)properly anchors the artifact-detection to a full line, eliminating thesess-1vssess-12collision flagged in the prior review. Good follow-up.
422-446: LGTM — deadartifact_refsparameter removed.Signature now matches the body's actual behavior (deterministic
artifact_ref_for_session). Cleaner contract.
217-230: Test coverage looks solid for the four-phase contract.
run_if_triggered_honors_non_default_trigger_config,concurrent_record_session_completion_preserves_all_entries,retries_failed_session_after_manual_recovery_and_keeps_single_logical_result, andbusy_run_does_not_consume_pending_session_and_succeeds_after_lock_releasecollectively pin down the issue#526acceptance criteria (configurable triggers, lock contention, single logical artifact across retries, snapshot replay). Nice work.
485-501: Session IDs are always lowercase UUIDs; the collapse risk does not materialize.All session IDs are generated via
Uuid::new_v4().to_string()at coordinator.rs:625, which always produces lowercase hex (e.g.,xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx). The case-insensitive dedupe indedupe_preserving_ordercannot collapse distinct session IDs that differ only by case because no two UUIDs can do so. The hypothetical scenario (e.g., "Dream summary for completed session SessA" vs. "sessa") cannot occur.No documentation or code changes are needed to address this concern.
> Likely an incorrect or invalid review comment.
| pub fn record_session_completion( | ||
| workspace_dir: &Path, | ||
| session_id: &str, | ||
| ) -> Result<DreamSessionStateRecord> { | ||
| let _lock_guard = DreamLockGuard::acquire_blocking(workspace_dir)?; | ||
|
|
||
| let mut state = load_state(workspace_dir)?; | ||
| state.sessions_since_last_run = state.sessions_since_last_run.saturating_add(1); | ||
| let count = state.sessions_since_last_run; | ||
| let now = Utc::now().to_rfc3339(); | ||
| let artifact_ref = artifact_ref_for_session(session_id); | ||
|
|
||
| let core_path = workspace_dir.join("MEMORY.md"); | ||
| let expected_line = format!("- Dream summary for completed session {session_id}"); | ||
| let artifact_already_written = core_path.exists() | ||
| && fs::read_to_string(&core_path) | ||
| .map(|content| content.lines().any(|line| line.trim() == expected_line)) | ||
| .unwrap_or(false); | ||
|
|
||
| if let Some(existing) = state | ||
| .completed_sessions | ||
| .iter_mut() | ||
| .find(|record| record.session_id == session_id) | ||
| { | ||
| if existing.status != DreamSessionStatus::Completed && artifact_already_written { | ||
| existing.status = DreamSessionStatus::Completed; | ||
| existing.completed_at = Some(now.clone()); | ||
| existing.last_attempt_at = Some(now.clone()); | ||
| existing.artifact_refs = vec![artifact_ref]; | ||
| existing.failure_reason = None; | ||
| } | ||
| let existing = existing.clone(); | ||
| store_state(workspace_dir, &state)?; | ||
| return Ok(existing); | ||
| } | ||
|
|
||
| let record = DreamSessionStateRecord { | ||
| session_id: session_id.to_string(), | ||
| status: if artifact_already_written { | ||
| DreamSessionStatus::Completed | ||
| } else { | ||
| DreamSessionStatus::Pending | ||
| }, | ||
| trigger_reason: DreamTriggerReason::SessionCount, | ||
| completion_recorded_at: now.clone(), | ||
| last_attempt_at: artifact_already_written.then(|| now.clone()), | ||
| completed_at: artifact_already_written.then(|| now.clone()), | ||
| artifact_refs: if artifact_already_written { | ||
| vec![artifact_ref] | ||
| } else { | ||
| vec![] | ||
| }, | ||
| failure_reason: None, | ||
| }; | ||
| state.completed_sessions.push(record.clone()); | ||
| state | ||
| .completed_sessions | ||
| .sort_by(|a, b| a.session_id.cmp(&b.session_id)); | ||
| store_state(workspace_dir, &state)?; | ||
| Ok(count) | ||
| Ok(record) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm gateway calls record_session_completion directly from async context (no spawn_blocking).
rg -nP -B3 -A5 'record_session_completion\s*\(' clients/agent-runtime/src/gateway/
rg -nP 'spawn_blocking' clients/agent-runtime/src/gateway/mod.rsRepository: dallay/corvus
Length of output: 789
🏁 Script executed:
# Get the full context of finalize_generated_session_if_needed to confirm it's async
rg -nP -B10 -A20 'finalize_generated_session_if_needed' clients/agent-runtime/src/gateway/mod.rs | head -60Repository: dallay/corvus
Length of output: 2312
🏁 Script executed:
# Find all callers of record_session_completion across the codebase
rg -nP 'record_session_completion' clients/agent-runtime/src/ --type rsRepository: dallay/corvus
Length of output: 84
🏁 Script executed:
# Check the DreamLockGuard::acquire_blocking implementation
rg -nP -B5 -A15 'fn acquire_blocking|impl.*DreamLockGuard' clients/agent-runtime/src/memory/dream.rsRepository: dallay/corvus
Length of output: 1377
🏁 Script executed:
# Broader search for all record_session_completion callers without file type restriction
rg 'record_session_completion' clients/agent-runtime/src/ -A2 -B2Repository: dallay/corvus
Length of output: 7875
🏁 Script executed:
# Search for hygiene loop and Dream consolidation mentioned in the review
rg -i 'hygiene|consolidat' clients/agent-runtime/src/memory/ -nRepository: dallay/corvus
Length of output: 4629
🏁 Script executed:
# Check if there are any spawn_blocking calls around this code path in gateway
rg 'spawn_blocking|task::block_in_place' clients/agent-runtime/src/gateway/ -nRepository: dallay/corvus
Length of output: 39
🏁 Script executed:
# Check where hygiene::run_if_due is called and in what context
rg -n 'hygiene::run_if_due' clients/agent-runtime/src/ -B5 -A5Repository: dallay/corvus
Length of output: 1052
🏁 Script executed:
# Look at the hygiene.rs file to understand if record_session_completion call is blocking there
sed -n '1,150p' clients/agent-runtime/src/memory/hygiene.rsRepository: dallay/corvus
Length of output: 4749
🏁 Script executed:
# Check the memory/mod.rs context where hygiene is run
sed -n '85,110p' clients/agent-runtime/src/memory/mod.rsRepository: dallay/corvus
Length of output: 1227
🏁 Script executed:
# Find where make_memory is called
rg 'make_memory' clients/agent-runtime/src/ -B3 -A3Repository: dallay/corvus
Length of output: 2095
🏁 Script executed:
# Check the full context of the test that shows the issue (around line 3747)
sed -n '3730,3765p' clients/agent-runtime/src/gateway/mod.rsRepository: dallay/corvus
Length of output: 1444
🟠 Blocking file lock in async request handler stalls Tokio worker.
record_session_completion is called synchronously from gateway/mod.rs::finalize_generated_session_if_needed (an async handler at line 1703). The function calls DreamLockGuard::acquire_blocking which invokes file.lock_exclusive(), a blocking OS file lock that parks the calling thread. During a Dream consolidation run (which can take seconds while writing MEMORY.md), the request handler thread blocks, unfairly starving the Tokio worker pool.
Recommended fixes (pick one):
- Make
record_session_completionasync and run the locked section insidetokio::task::spawn_blocking. - Keep it sync but require async callers to wrap in
spawn_blocking. Document this on the function. - Use
try_lock_exclusivewith bounded retry/backoff instead of unbounded blocking.
Violates coding guideline: "Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/src/memory/dream.rs` around lines 295 - 353, The
function record_session_completion currently calls
DreamLockGuard::acquire_blocking which blocks the Tokio thread; change
record_session_completion to async and move the blocking work (the lock
acquisition and the file/FS-heavy state load/store and MEMORY.md checks) into a
tokio::task::spawn_blocking closure so the async
gateway::finalize_generated_session_if_needed can await it without stalling the
runtime; update the signature to async fn record_session_completion(...) ->
Result<DreamSessionStateRecord>, perform the existing synchronous logic inside
the spawn_blocking closure (including use of DreamLockGuard::acquire_blocking,
load_state, store_state, and artifact_ref_for_session), and adjust all callers
(notably finalize_generated_session_if_needed) to .await the new async function.
| **Global lock:** retain a coarse lock such as `dream.lock` to serialize a Dream run over a workspace. | ||
| This prevents overlapping scanners/pruners and is especially important for markdown/file backends. | ||
|
|
||
| **Stale-lock recovery:** ADR-5, §6.4, and §11 require a documented recovery posture for `dream.lock`. | ||
| Use OS-level advisory file locking where available so process death automatically releases the held lock even if the lock file path remains on disk. When a reclaim attempt detects an existing `dream.lock` file but cannot acquire the OS lock, treat the run as busy and log the refusal. If an operator suspects a stale file-path artifact, validate that no active Corvus process still holds the lock (for example via process inspection or a retry after the suspected owner exits) before deleting `dream.lock`. Any manual reclaim should log the workspace path, detection reason, and validation outcome so postmortems can distinguish a safe cleanup from a live-lock contention event. | ||
|
|
There was a problem hiding this comment.
Lockfile name is inconsistent with current runtime implementation.
This section documents dream.lock, but runtime currently defines the Dream lock constant as dream_state.json in clients/agent-runtime/src/memory/dream.rs (Line 12). The mismatch makes manual recovery steps risky (operators may delete the wrong file and impact replay metadata/state). Please align spec and implementation to one canonical lock artifact name and recovery procedure.
As per coding guidelines **/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/archive/2025-08-07-2026-04-26-dream-system/design.md` around
lines 487 - 492, The spec references a lockfile named "dream.lock" but the
runtime currently uses the constant "dream_state.json" as the lock artifact (the
string constant named "dream_state.json" in the Dream runtime), causing
dangerous mismatch; choose one canonical artifact name and make code and docs
consistent: either rename the runtime constant to "dream.lock" (and update any
code paths that read/write that constant and recovery logic in the Dream
runtime), or update this design doc to reference "dream_state.json" and adjust
the stale-lock recovery wording to match the runtime behavior/log fields; ensure
the chosen name is used everywhere (spec text, any logging messages that mention
the file name, and the recovery validation guidance) so manual reclaim
instructions and logs unambiguously point to the same file.
|
1 participant
Related Issues
Fixes #526
Summary
clients/agent-runtimegatewayandsessionsspecsTested Information
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warningscargo testmake web-test-allpnpm --dir clients/web checkVerify status for the archived change is
PASS_WITH_WARNINGS; remaining warnings are about some spec scenarios relying on partial evidence rather than dedicated end-to-end scenario tests.Documentation Impact
openspec/specs/gateway/spec.mdopenspec/specs/sessions/spec.mdopenspec/changes/archive/2025-08-07-2026-04-26-dream-system/Breaking Changes
Checklist