feat(harness): add session_store — durable session history and run ledger - #90
Conversation
…dger
Adds `harness::session_store`, the SQLite-backed (WAL + FTS5) history layer
for sessions, messages, tool calls, cost metadata, and parent/child lineage,
plus `run_ledger` for background agent/workflow execution state.
Ported from OpenHuman's `agent/session_db`, which was generic runtime
machinery sitting in a host: its only coupling to that host was reading
`workspace_dir` off a `Config`. Entry points now take `&Path` directly, so the
crate derives `{workspace}/session_db/sessions.db` itself and no host type
crosses the boundary.
Port notes:
- `anyhow` is not a dependency here, so error handling funnels through a new
`TinyAgentsError::Storage` variant. `context.rs` provides a `StorageContext`
trait mirroring `anyhow::Context`'s shape (including the `Option` impl for
"row expected but absent"), which kept the conversion mechanical.
- `From<rusqlite::Error>` is added under the `sqlite` feature so driver calls
can use `?` directly; it is the only new blanket conversion.
- `chrono` gains the `serde` feature — ledger records carry `DateTime<Utc>`
across the serde boundary. FTS5 needs no cargo feature; `bundled` already
compiles it in.
- Gated behind `sqlite`, alongside the existing graph checkpointer.
This module is history, not durability: nothing resumes from it. Resume stays
with `graph::checkpoint`, and live runtime data stays with `harness::store`.
34 tests move across intact and pass.
Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded a SQLite-gated session module and durable run ledger. The change includes schemas, typed APIs, recording and search operations, run and team coordination workflows, contextual storage errors, documentation, and extensive tests. ChangesSQLite persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller as Session caller
participant Ops as session::ops
participant Store as session::store
participant SQLite as SQLite and FTS5
Caller->>Ops: record or search session data
Ops->>Store: open connection or transaction
Store->>SQLite: persist records and indexes
SQLite-->>Ops: return rows and search matches
Ops-->>Caller: return typed session results
sequenceDiagram
participant Member as Team member
participant Ledger as run_ledger::ops
participant SQLite as SQLite transaction
participant Task as Agent team task
Member->>Ledger: claim task
Ledger->>SQLite: validate dependencies and guarded claim
SQLite-->>Ledger: return claim outcome
Member->>Ledger: submit completion evidence
Ledger->>SQLite: validate ownership and quality gates
SQLite->>Task: persist completion or failure state
Ledger-->>Member: return completion outcome
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4478dd441
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`harness::session_store` becomes `session`. Session history is a persistence domain in its own right, not part of the agent loop: nothing in `harness` reads from it, and a host can use it without running a harness at all — indexing sessions produced elsewhere, or recovering orchestration state at boot before any agent exists. Filing it under `harness::` implied a dependency that does not exist in either direction. Sits alongside the crate's other top-level domains (`graph`, `registry`, `repl`, `rlm`) and keeps its `sqlite` feature gate, which simply moves from `harness/mod.rs` to `lib.rs`. Pure rename: no behaviour, no schema, no on-disk change. Public path is now `tinyagents::session::*`. 1415 tests pass, clippy clean under -D warnings, fmt clean. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
… module Path-only update across 27 files: tinyagents::harness::session_store::* becomes tinyagents::session::*. No behaviour change; the DB path and the session_db / run_ledger RPC namespaces are untouched. Tracks tinyhumansai/tinyagents#90. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2233c02b9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ports All four code findings are pre-existing defects carried over from OpenHuman, not introduced by the move. Each fix is pinned by a regression test, and the first two were mutation-checked (revert the fix, the test fails; restore it, it passes). P1 — partial telemetry upsert failed outright. The counters are `Option` so a caller can update one field alone, but the columns are `NOT NULL DEFAULT` and SQLite does not apply a column default to an explicitly supplied NULL. So recording only `model` or only `error` hit a NOT NULL constraint on a run's first write. The insert side now coalesces to the column default; the update side re-reads the SAME parameter and coalesces to the stored value, keeping per-field optionality. `excluded.*` cannot serve the update side — it observes the already-coalesced insert row, so a `None` would read as 0 and clobber the stored counter. P1 — run-event sequences raced. `SELECT MAX(sequence) + 1` followed by a separate INSERT is read-modify-write: two connections appending for the same run read the same value and the loser fails the `(run_id, sequence)` primary key, silently dropping a real event. Allocation now happens inside the INSERT via a sub-select with RETURNING, so SQLite's write lock serializes it. P2 (severity understated — it is a panic on ordinary user data) — the FTS snippet sliced at a raw byte offset. `&content[..2000]` panics whenever byte 2000 lands inside a multi-byte character, which any long non-ASCII message can do. Worse than a crash: the message INSERT has already autocommitted, so the row survives with no FTS entry and is permanently unsearchable. Now walks back to a character boundary, as the tool-output path already did. P2 — team task claim/completion had no isolation. `with_connection` hands out an autocommit connection, so the documented compare-and-swap contract did not hold across statements: a dependency could flip out of `done` between the check and the claim. Added `session::store::with_transaction` (BEGIN IMMEDIATE, commit on Ok, best-effort rollback on Err) and routed both coordination operations through it. Immediate rather than deferred so racing claims serialize at BEGIN instead of failing at COMMIT after one has already decided it won. P1 — public surface was reachable only through the module path. AGENTS.md requires exports centralized in `lib.rs`; added feature-gated root re-exports for the record/query entry points and the ledger's coordination types. One test caught itself: the UTF-8 regression first used a 2-byte character, where byte 2000 is a boundary, so it passed vacuously. Switched to a 3-byte character so the offset genuinely lands mid-character. 1420 tests pass, clippy clean under -D warnings, fmt clean. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 081e0e18f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
P2 — plain-text search hit the FTS5 parser. `SessionSearchParams::query` is documented as plain text, but was bound straight to `MATCH`, so ordinary input (`C++`, `foo-bar`, `file.rs`, a stray quote) returned a syntax or `no such column` error instead of results. Each whitespace-separated term is now emitted as a quoted FTS5 string literal (quotes escaped by doubling) joined with AND, so punctuation is data and multi-term queries stay conjunctive. P2 — an upsert could strand a claimed task. A claim only means anything while the task is `in_progress`, but editing a claimed task through the upsert left `claimed_by_member_id`/`claim_token` set on a `todo` row: a new claim saw AlreadyClaimed, completion saw NotClaimed, and release/shutdown skipped it because they only match `in_progress`. The claim is now cleared whenever the new status is not `in_progress`, and preserved when it is, so an unrelated edit cannot steal a live claim. P2 — a failed completion gate discarded submitted evidence. Evidence accumulates across attempts, so dropping it punished the caller for an unrelated gate failure: after fixing a dependency, a retry that did not resend the same links failed `require_evidence` on evidence already submitted. The failed-gate update now persists the merged evidence alongside the verdict. P1 — test layout. AGENTS.md requires module-local tests in a dedicated `test.rs` per module directory; this module had `ops_tests.rs` plus inline `mod tests` blocks in four files, which would have established a conflicting convention in a large new feature. Consolidated into `session/test.rs` and `session/run_ledger/test.rs`, with the helpers they reach widened to `pub(super)`. All 62 session tests survive the move. P1 — missing module README. AGENTS.md requires one for complex modules. Added `src/session/README.md` covering the schema, the FTS behaviour, the coordination guarantees, and the operational constraints — each of the non-obvious rules there is pinned by a named test. 1424 tests pass, clippy clean under -D warnings, fmt clean. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
…tdown P1 — `record_tool_call` returned the wrong id. `index_fts_tool` inserts into the `sessions_fts` virtual table, which moves `last_insert_rowid()`, and the id was read after that call — so callers got an FTS rowid addressing no tool call at all. Any real session already has its session row in `sessions_fts`, so the two counters diverge immediately. `record_message` already ordered these correctly; this path did not. The id is now captured directly after the insert. P2 — `shutdown_agent_team_member` documented one transaction but ran on the autocommit helper, so its read and two updates had no isolation: a task completed between the released-ID query and the release update was still reported as released, and a failure on the member update could leave tasks released while the member stayed active, even as the call returned an error. Routed through `with_transaction`, which the claim and completion paths already use. The regression test asserts its own premise first — that the FTS insert really does move `last_insert_rowid()` — so it cannot pass vacuously if the ordering is ever reintroduced. 1425 tests pass, clippy clean under -D warnings, fmt clean. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54b188b8e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 2. Only the current claimant may complete, and only while in progress. | ||
| let is_claimant = task.claimed_by_member_id.as_deref() == Some(member_id); | ||
| let in_progress = task.status == AgentTeamTaskStatus::InProgress; |
There was a problem hiding this comment.
When the same member releases and reclaims a task with a new token, a late completion from the previous worker still passes this check because completion validates only member_id. It can therefore mark the new attempt done and persist stale evidence. Accept and compare the stored claim_token so only the worker holding the current claim can complete the task.
Useful? React with 👍 / 👎.
| "UPDATE agent_team_tasks | ||
| SET claimed_by_member_id = ?1, claim_token = ?2, status = 'in_progress', updated_at = ?3 | ||
| WHERE id = ?4 AND team_id = ?5 AND claimed_by_member_id IS NULL", |
There was a problem hiding this comment.
When an unclaimed task is already done—for example after inserting or upserting it with that status—this update still matches and changes it back to in_progress, so an ordinary claim call can undo durable completion. Add a status predicate that permits only claimable states, at minimum excluding terminal done tasks.
Useful? React with 👍 / 👎.
| raw.split_whitespace() | ||
| .map(|term| format!("\"{}\"", term.replace('"', "\"\""))) | ||
| .collect::<Vec<_>>() | ||
| .join(" AND ") |
There was a problem hiding this comment.
When search terms occur in different messages of the same session, joining them with FTS AND returns no result because each message is indexed as a separate sessions_fts row and MATCH requires all terms in one row. For example, a session with alpha in one message and beta in another is omitted from an alpha beta search; intersect matching session_id sets per term or index one aggregate FTS document per session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (15)
src/session/run_ledger/types.rs (3)
464-464: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift
gate_statusis stringly typed while every sibling status is an enum.
AgentTeamTaskusesAgentTeamTaskStatusforstatusbut a bareStringforgate_status.src/session/README.mdline 101 documentsgate_status = "failed"as a defined value, so the set is closed. An untyped field lets a typo persist to storage and forces every consumer to compare raw strings.Introduce an
AgentTeamGateStatusenum with the sameas_str/parsepair the other status enums use.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/run_ledger/types.rs` at line 464, Replace AgentTeamTask.gate_status’s String type with a new AgentTeamGateStatus enum, following the existing status-enum patterns and implementing the same as_str and parse methods. Preserve the documented closed set of gate-status values, including "failed", and update any serialization or parsing usage in the surrounding types to use the enum.
514-515: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
ClaimOutcomeandCompletionOutcomederiveSerializebut notDeserialize.Both enums are re-exported at the crate root (
src/lib.rslines 93-98). A host can send an outcome over a wire protocol but cannot read one back. Every other public record type in this file derives both traits. AddDeserializeunless the one-way shape is deliberate.♻️ Proposed fix
-#[derive(Debug, Clone, PartialEq, Serialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase", tag = "kind")] pub enum ClaimOutcome {-#[derive(Debug, Clone, PartialEq, Serialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase", tag = "kind")] pub enum CompletionOutcome {Also applies to: 534-535
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/run_ledger/types.rs` around lines 514 - 515, Update the public ClaimOutcome and CompletionOutcome enum definitions to derive Deserialize alongside Serialize, preserving their existing serde rename and tag configuration and matching the other public record types in the module.
253-256: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse one integer width for shared pagination fields.
AgentRunListRequestandRunEventListRequestuseOption<u32>, whileWorkflowRunListRequestandAgentTeamListRequestuseOption<u64>. This requires casts when callers share pagination values. Remove the unsupportedTypeSchema::U64rationale and document the crate-local pagination limits and conversions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/run_ledger/types.rs` around lines 253 - 256, Standardize the pagination fields in AgentRunListRequest, RunEventListRequest, WorkflowRunListRequest, and AgentTeamListRequest on Option<u32> so callers can share values without casts. Remove the TypeSchema::U64 rationale and document the crate-local pagination limits and any required conversions at the shared pagination definitions.src/session/ops.rs (3)
183-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the char-boundary truncation into one helper.
record_tool_callandindex_fts_contentimplement the same backward scan to a UTF-8 boundary with different byte caps.src/session/test.rslines 234-244 copies the logic a third time to predict the expected value, so the test cannot catch a divergence in the production code. One helper removes all three copies.♻️ Proposed helper
+/// Truncates `s` to at most `max_bytes`, cutting on a UTF-8 character +/// boundary. Slicing at a raw byte offset panics on multi-byte input. +pub(super) fn truncate_on_char_boundary(s: &str, max_bytes: usize) -> &str { + if s.len() <= max_bytes { + return s; + } + let mut cutoff = max_bytes; + while cutoff > 0 && !s.is_char_boundary(cutoff) { + cutoff -= 1; + } + &s[..cutoff] +}let bounded_output = tool_output.map(|o| { - if o.len() <= MAX_TOOL_OUTPUT_BYTES { - o.to_string() - } else { - let mut cutoff = MAX_TOOL_OUTPUT_BYTES; - while cutoff > 0 && !o.is_char_boundary(cutoff) { - cutoff -= 1; - } - let mut truncated = o[..cutoff].to_string(); - truncated.push_str("\n...[truncated]"); - truncated - } + let bounded = truncate_on_char_boundary(o, MAX_TOOL_OUTPUT_BYTES); + if bounded.len() == o.len() { + o.to_string() + } else { + format!("{bounded}\n...[truncated]") + } });- let snippet = if content.len() > MAX_FTS_SNIPPET_BYTES { - let mut cutoff = MAX_FTS_SNIPPET_BYTES; - while cutoff > 0 && !content.is_char_boundary(cutoff) { - cutoff -= 1; - } - &content[..cutoff] - } else { - content - }; + let snippet = truncate_on_char_boundary(content, MAX_FTS_SNIPPET_BYTES);Also applies to: 584-592
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/ops.rs` around lines 183 - 195, Extract the shared UTF-8-safe truncation logic into a helper, such as near the existing session utilities, and have record_tool_call, index_fts_content, and the corresponding src/session/test.rs expectation use it with their respective byte caps. Preserve the current behavior for untruncated values, boundary-safe slicing, and the "\n...[truncated]" suffix.
390-396: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueThe COUNT and the page SELECT run outside one transaction.
search_sessions_innerandlist_sessions(lines 280-286) issue aCOUNT(*)and then a separateSELECT.with_connectionis autocommit, so a concurrent writer can commit between the two statements. The returnedtotalthen disagrees with the returned page.If the callers accept an approximate total, state that in the doc comment. If they do not, route both statements through
with_transaction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/ops.rs` around lines 390 - 396, The COUNT and page SELECT in search_sessions_inner and list_sessions must share one transaction to keep total and returned rows consistent under concurrent writes. Route both statements through with_transaction using the existing connection and preserve the current query results and parameter handling; otherwise explicitly document that total is approximate if transactional consistency is not intended.
222-241: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy lift
get_sessionreports a missing session as a storage error.A session that does not exist is an ordinary query result, not a database failure.
TinyAgentsError::Storageis documented insrc/error.rslines 225-233 as an operation failure of the backing database. A caller must now match on the message text to tell "no such session" from "database is locked".
get_workflow_runinsrc/session/run_ledger/ops.rsreturnsResult<Option<_>>and lets the caller decide (line 149). Alignget_sessionwith that shape, or add a distinct not-found signal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/ops.rs` around lines 222 - 241, Update get_session to distinguish an absent session from database failures by returning an optional session result, matching get_workflow_run’s Result<Option<_>> contract, and return None when rows.next() yields no record. Preserve query and row-mapping errors as errors, and update affected callers to handle the optional result without matching Storage message text.src/session/context.rs (1)
20-29: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a lazy context argument.
storage_contexttakes&str. Call sites that need interpolation must build the string before the call, soformat!runs on the success path too. Example:src/session/store.rslines 33-36 and line 40 allocate on everywith_connectioncall. A second method that takes a closure keeps the common&strform and removes the allocation from the hot path.♻️ Proposed addition
pub(crate) trait StorageContext<T> { /// Wraps the failure as a storage error prefixed with `context`. fn storage_context(self, context: &str) -> Result<T>; + + /// Wraps the failure as a storage error prefixed with the lazily built + /// `context`, so the message is only formatted on the error path. + fn storage_context_with<C: Display>(self, context: impl FnOnce() -> C) -> Result<T>; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/context.rs` around lines 20 - 29, Update the StorageContext trait and its Result implementation to add a lazy context variant accepting a closure that produces the context string, while retaining storage_context(&str) for static messages. Use the lazy method at interpolated with_connection call sites in session store code so formatting occurs only when the Result is an error.src/session/run_ledger/mod.rs (1)
12-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the run-ledger submodules private, as
src/session/mod.rsdoes.
ops,store, andtypesare declaredpuband their items are also re-exported below. Downstream users then have two paths to every item, for examplerun_ledger::upsert_agent_runandrun_ledger::ops::upsert_agent_run. The sibling module keepsopsandstoreprivate (src/session/mod.rslines 67-71) and exposes one path.♻️ Proposed fix
-pub mod ops; -pub mod store; -pub mod types; +mod ops; +mod store; +pub mod types;Note:
src/session/run_ledger/ops.rsline 113 callscrate::session::store::with_connection, andsrc/session/mod.rsre-exportswith_connection, so verify the visibility change compiles for internal callers ofrun_ledger::store.As per coding guidelines: "Keep public API exports centralized in
src/lib.rsso downstream users have a predictable surface."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/run_ledger/mod.rs` around lines 12 - 14, Make the ops, store, and types module declarations in run_ledger private, matching the visibility pattern in session/mod.rs, while preserving the existing item re-exports as the sole public access path. Update internal references such as ops’s with_connection call to use the appropriate crate-visible or session-level re-export so compilation remains intact, and keep public API exposure centralized through the existing exports.Source: Coding guidelines
src/session/types.rs (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTake
selfby value inas_str.
SessionStatusisCopy. Every status enum insrc/session/run_ledger/types.rsdeclarespub fn as_str(self). Match that signature here for consistency.♻️ Proposed fix
- pub fn as_str(&self) -> &'static str { + pub fn as_str(self) -> &'static str {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/types.rs` at line 14, Update SessionStatus::as_str to take self by value instead of borrowing &self, matching the existing as_str signatures for Copy status enums in src/session/run_ledger/types.rs while preserving its current return behavior.src/session/mod.rs (1)
77-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-export
rusqliteor restrict the helpers to crate visibility.The
sqlitefeature gates these public helpers, but their callback types still exposerusqlite::Connectionwithout a crate re-export. Callers must add a compatible directrusqlitedependency to name the type, which couples this API torusqliteversion changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/mod.rs` at line 77, Update the public helper exports around db_path, with_connection, and with_transaction so their rusqlite::Connection callback types are usable without requiring callers to depend directly on rusqlite: either publicly re-export the compatible rusqlite API from the session module or reduce these helpers and their signatures to crate visibility. Keep the sqlite feature gating intact and apply the chosen visibility/API change consistently.src/session/run_ledger/ops.rs (3)
1463-1488: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
map_agent_run_rowruns one extra telemetry query per row.Line 1483 calls
get_optional_run_telemetry, which prepares and executes a statement for every mapped row.list_agent_runs(Line 410) maps up to 500 rows per page, so one list call can issue up to 501 queries. Fetch the telemetry columns with aLEFT JOIN run_telemetryin the two agent-run queries and map them from the same row. Keep the per-row helper only for the single-rowget_agent_run_innerpath if the join complicates it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/run_ledger/ops.rs` around lines 1463 - 1488, Eliminate the per-row telemetry query from list operations by adding a LEFT JOIN to run_telemetry in both agent-run list queries and selecting the telemetry columns in their result sets. Update map_agent_run_row to read telemetry directly from the joined row, while retaining get_optional_run_telemetry only for the single-row get_agent_run_inner path if needed. Ensure the list mapping still handles missing telemetry as optional.
99-101: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRead the run back inside the same connection.
Line 101 opens a second connection through
get_agent_run. The write closure already has a connection and the schema is initialized. Reading inside the closure removes one file open per upsert and closes the window in which another writer changes the row between the write and the read. The same pattern applies toupsert_workflow_run(Line 149),upsert_agent_team(Line 594),upsert_agent_team_member(Line 734), andupsert_agent_team_task(Line 859).♻️ Proposed change for `upsert_agent_run`
- .storage_context("upsert agent run")?; - Ok(()) - })?; - - get_agent_run(workspace_dir, &upsert.id)?.storage_context("agent run missing after upsert") + .storage_context("upsert agent run")?; + get_agent_run_inner(conn, &upsert.id)?.storage_context("agent run missing after upsert") + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/run_ledger/ops.rs` around lines 99 - 101, Update upsert_agent_run and the corresponding upsert_workflow_run, upsert_agent_team, upsert_agent_team_member, and upsert_agent_team_task functions to read the persisted row within their existing write-connection closures. Replace the post-closure get_agent_run-style lookup with the closure’s connection query, preserving the existing returned model and missing-row error behavior.
184-190: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReturn the original payload instead of re-parsing the serialized string.
Line 188 parses
payload_jsonback into aValueand falls back to{}on failure.event.payloadis still owned here. Move it into the result. This removes one parse per append and removes a silent fallback that can return a payload that differs from the stored row.♻️ Proposed change
Ok(RunEvent { run_id: event.run_id, sequence: next_sequence as u64, event_type: event.event_type, - payload: serde_json::from_str(&payload_json).unwrap_or_else(|_| json!({})), + payload: event.payload, timestamp: now, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/run_ledger/ops.rs` around lines 184 - 190, Update the RunEvent construction in the append operation to assign the original owned event.payload directly to payload instead of parsing payload_json with serde_json::from_str and falling back to an empty object. Preserve the remaining fields unchanged.src/session/run_ledger/test.rs (2)
174-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a concurrent append case for the sequence allocator.
This test appends events from one thread, so it proves density and per-run independence. It does not exercise the race that the implementation comment in
src/session/run_ledger/ops.rs(Lines 159-165) describes. Spawn several threads that append to the samerun_idin the same workspace, then assert that the returned sequences are unique and form a contiguous range. That case fails on a read-then-write allocator and passes on the current single-statement allocator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/run_ledger/test.rs` around lines 174 - 207, Add a concurrent append scenario to run_event_sequences_are_allocated_by_the_insert: clone or otherwise share the workspace and spawn several threads appending events with the same run_id, collect their returned sequence values, then assert the values are unique and equal to the contiguous range from 1 through the number of appends. Retain the existing sequential density and per-run independence assertions.
507-547: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for completion, shutdown, and claiming a
donetask.This file exercises claim, release, and member transitions, but three operations in the layer have no test.
complete_agent_team_task: no case coversCompleted,GateFailed(unmet dependency, owner mismatch, missing evidence underrequire_evidence), orNotClaimed. This function holds the gate logic and two compare-and-swap guards.shutdown_agent_team_member: no case covers the bulk release ofin_progresstasks or the returnedreleasedids.- Claiming a
donetask: this test markstask-adone at Lines 525-543, which clears the claim on that row. A follow-up claim ontask-ademonstrates the missing status guard raised onsrc/session/run_ledger/ops.rsLines 964-979. Add that case together with the guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/run_ledger/test.rs` around lines 507 - 547, Expand run-ledger tests to cover complete_agent_team_task outcomes: Completed, GateFailed for unmet dependencies, owner mismatch, and missing evidence when require_evidence is enabled, plus NotClaimed and both compare-and-swap guards. Add shutdown_agent_team_member coverage asserting in-progress tasks are released and returned released IDs are correct. Extend claim_blocked_until_dependency_done to attempt claiming task-a after it is marked done, and add the status guard in claim_agent_team_task rejecting done tasks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 48-51: Update the three comments in Cargo.toml that reference
harness::session_store to use the promoted top-level session module path,
session::session_store, while preserving the existing bundled/FTS5 rationale and
wording otherwise.
In `@src/session/mod.rs`:
- Around line 64-65: Update the module-level documentation link near the session
schema and FTS coordination text so it resolves in rendered rustdoc: either
point it to the repository-hosted README file or inline the README via the
module’s documentation attribute using include_str!("README.md").
In `@src/session/ops.rs`:
- Around line 42-66: Use with_transaction instead of with_connection in
record_session_start, record_message, and record_tool_call so each base-row
insert and its corresponding index_fts_session, index_fts_content, or
index_fts_tool call commit atomically; preserve the existing SQL, parameters,
error context, and callback logic.
In `@src/session/README.md`:
- Around line 93-97: Update the README paragraph describing coordination
operations and BEGIN IMMEDIATE to reflect the current SQLITE_BUSY behavior when
no busy timeout is configured, or revise it after the store’s busy-timeout
handling is added; do not claim racing claims wait and serialize unless the
implementation guarantees that behavior.
- Around line 36-47: Update the session README statements to match the actual
API and schema: clarify that only the functions re-exported by src/lib.rs are
available at the crate root, while list_messages, list_tool_calls,
list_children, mark_interrupted, with_connection, and with_transaction remain
under session::. Correct the table-count description to state that there are ten
non-FTS tables plus one FTS5 virtual table, matching the schema table.
In `@src/session/run_ledger/ops.rs`:
- Around line 391-392: Update the offset conversion in the surrounding
request-handling function to use i64::try_from(request.offset.unwrap_or(0))
instead of an as i64 cast, and propagate the conversion failure with the same
storage context and error-handling pattern used by list_workflow_runs and
list_agent_teams. Keep the existing default offset behavior unchanged.
- Around line 793-795: Update the upsert handling around gate_status and the
conflict clause to avoid pairing a reset status with a stale gate_reason. When
upsert.gate_status is None, preserve the existing stored gate_status (or
alternatively clear gate_reason whenever the status becomes pending), while
retaining the current behavior for explicitly supplied statuses.
- Around line 1183-1235: Update shutdown_agent_team_member to use
with_transaction instead of with_connection, keeping the existence check,
released-ID query, task release, member update, and final member read within one
transaction. Preserve the existing transaction error propagation and returned
(member, released) result.
- Around line 964-979: Update the compare-and-swap in claim_agent_team_task to
require the task status is eligible for claiming, preventing done tasks from
being changed back to in_progress. Before executing the update, distinguish a
done task and return the appropriate non-claim outcome instead of reporting
AlreadyClaimed; preserve the existing AlreadyClaimed behavior for tasks blocked
by an active claimant.
In `@src/session/store.rs`:
- Around line 27-44: Update with_connection so schema initialization is
performed once per database path instead of on every operation. Add a
synchronized per-path initialization guard, such as
OnceLock<Mutex<HashSet<PathBuf>>> or an equivalent user_version check, and call
init_schema only when the current database has not been initialized; preserve
connection creation and callback behavior.
- Around line 55-65: Configure a nonzero busy timeout immediately after opening
each SQLite connection in with_connection, using the established timeout value
so concurrent BEGIN IMMEDIATE calls wait for the write lock. Update
src/session/README.md lines 93-97 to retain the serialization claim only with
this configured timeout and explicitly name its value; the store.rs
documentation should likewise reflect the timeout-backed behavior.
In `@src/session/test.rs`:
- Around line 228-263: Update the tests tool_output_truncation,
mark_interrupted_updates_running, and session_end_updates_cost_fields to
exercise the public session APIs instead of duplicating their SQL or
transformation logic. Use a TempDir workspace root and invoke record_tool_call,
mark_interrupted, and record_session_end respectively, following the setup
pattern in run_ledger tests; retain assertions on the resulting persisted values
and statuses.
- Around line 508-517: Update wal_mode_is_set to use a temporary file-backed
database instead of with_memory_connection, then initialize it through
init_schema before querying PRAGMA journal_mode. Assert that the reported mode
is "wal" and retain the test’s existing error propagation and cleanup behavior.
In `@src/session/types.rs`:
- Around line 33-34: Apply #[serde(rename_all = "camelCase")] consistently to
the public session types SessionRecord, SessionMessage, SessionToolCall, and
SessionSearchResult, matching SessionSearchParams and the run-ledger types.
Ensure their serialized field names use camelCase without changing the Rust
field names or other behavior.
---
Nitpick comments:
In `@src/session/context.rs`:
- Around line 20-29: Update the StorageContext trait and its Result
implementation to add a lazy context variant accepting a closure that produces
the context string, while retaining storage_context(&str) for static messages.
Use the lazy method at interpolated with_connection call sites in session store
code so formatting occurs only when the Result is an error.
In `@src/session/mod.rs`:
- Line 77: Update the public helper exports around db_path, with_connection, and
with_transaction so their rusqlite::Connection callback types are usable without
requiring callers to depend directly on rusqlite: either publicly re-export the
compatible rusqlite API from the session module or reduce these helpers and
their signatures to crate visibility. Keep the sqlite feature gating intact and
apply the chosen visibility/API change consistently.
In `@src/session/ops.rs`:
- Around line 183-195: Extract the shared UTF-8-safe truncation logic into a
helper, such as near the existing session utilities, and have record_tool_call,
index_fts_content, and the corresponding src/session/test.rs expectation use it
with their respective byte caps. Preserve the current behavior for untruncated
values, boundary-safe slicing, and the "\n...[truncated]" suffix.
- Around line 390-396: The COUNT and page SELECT in search_sessions_inner and
list_sessions must share one transaction to keep total and returned rows
consistent under concurrent writes. Route both statements through
with_transaction using the existing connection and preserve the current query
results and parameter handling; otherwise explicitly document that total is
approximate if transactional consistency is not intended.
- Around line 222-241: Update get_session to distinguish an absent session from
database failures by returning an optional session result, matching
get_workflow_run’s Result<Option<_>> contract, and return None when rows.next()
yields no record. Preserve query and row-mapping errors as errors, and update
affected callers to handle the optional result without matching Storage message
text.
In `@src/session/run_ledger/mod.rs`:
- Around line 12-14: Make the ops, store, and types module declarations in
run_ledger private, matching the visibility pattern in session/mod.rs, while
preserving the existing item re-exports as the sole public access path. Update
internal references such as ops’s with_connection call to use the appropriate
crate-visible or session-level re-export so compilation remains intact, and keep
public API exposure centralized through the existing exports.
In `@src/session/run_ledger/ops.rs`:
- Around line 1463-1488: Eliminate the per-row telemetry query from list
operations by adding a LEFT JOIN to run_telemetry in both agent-run list queries
and selecting the telemetry columns in their result sets. Update
map_agent_run_row to read telemetry directly from the joined row, while
retaining get_optional_run_telemetry only for the single-row get_agent_run_inner
path if needed. Ensure the list mapping still handles missing telemetry as
optional.
- Around line 99-101: Update upsert_agent_run and the corresponding
upsert_workflow_run, upsert_agent_team, upsert_agent_team_member, and
upsert_agent_team_task functions to read the persisted row within their existing
write-connection closures. Replace the post-closure get_agent_run-style lookup
with the closure’s connection query, preserving the existing returned model and
missing-row error behavior.
- Around line 184-190: Update the RunEvent construction in the append operation
to assign the original owned event.payload directly to payload instead of
parsing payload_json with serde_json::from_str and falling back to an empty
object. Preserve the remaining fields unchanged.
In `@src/session/run_ledger/test.rs`:
- Around line 174-207: Add a concurrent append scenario to
run_event_sequences_are_allocated_by_the_insert: clone or otherwise share the
workspace and spawn several threads appending events with the same run_id,
collect their returned sequence values, then assert the values are unique and
equal to the contiguous range from 1 through the number of appends. Retain the
existing sequential density and per-run independence assertions.
- Around line 507-547: Expand run-ledger tests to cover complete_agent_team_task
outcomes: Completed, GateFailed for unmet dependencies, owner mismatch, and
missing evidence when require_evidence is enabled, plus NotClaimed and both
compare-and-swap guards. Add shutdown_agent_team_member coverage asserting
in-progress tasks are released and returned released IDs are correct. Extend
claim_blocked_until_dependency_done to attempt claiming task-a after it is
marked done, and add the status guard in claim_agent_team_task rejecting done
tasks.
In `@src/session/run_ledger/types.rs`:
- Line 464: Replace AgentTeamTask.gate_status’s String type with a new
AgentTeamGateStatus enum, following the existing status-enum patterns and
implementing the same as_str and parse methods. Preserve the documented closed
set of gate-status values, including "failed", and update any serialization or
parsing usage in the surrounding types to use the enum.
- Around line 514-515: Update the public ClaimOutcome and CompletionOutcome enum
definitions to derive Deserialize alongside Serialize, preserving their existing
serde rename and tag configuration and matching the other public record types in
the module.
- Around line 253-256: Standardize the pagination fields in AgentRunListRequest,
RunEventListRequest, WorkflowRunListRequest, and AgentTeamListRequest on
Option<u32> so callers can share values without casts. Remove the
TypeSchema::U64 rationale and document the crate-local pagination limits and any
required conversions at the shared pagination definitions.
In `@src/session/types.rs`:
- Line 14: Update SessionStatus::as_str to take self by value instead of
borrowing &self, matching the existing as_str signatures for Copy status enums
in src/session/run_ledger/types.rs while preserving its current return behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d5fd136-02b5-4124-a5c2-2e23e4d26674
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlsrc/error.rssrc/lib.rssrc/session/README.mdsrc/session/context.rssrc/session/mod.rssrc/session/ops.rssrc/session/run_ledger/mod.rssrc/session/run_ledger/ops.rssrc/session/run_ledger/store.rssrc/session/run_ledger/test.rssrc/session/run_ledger/types.rssrc/session/store.rssrc/session/test.rssrc/session/types.rs
| # `bundled` compiles SQLite with FTS5 already enabled, which | ||
| # `harness::session_store` relies on for its `sessions_fts` cross-session | ||
| # search table. There is no separate `fts5` cargo feature at this version — do | ||
| # not add one, it does not resolve. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale module path in these comments.
The three new comments name harness::session_store. The PR promotes that API to the top-level session module (src/lib.rs line 85, src/session/mod.rs). Rename the references so the manifest rationale matches the shipped module path.
📝 Proposed comment updates
# `bundled` compiles SQLite with FTS5 already enabled, which
-# `harness::session_store` relies on for its `sessions_fts` cross-session
+# `session` relies on for its `sessions_fts` cross-session
# search table. There is no separate `fts5` cargo feature at this version — do
# not add one, it does not resolve.-# `serde` is required by `harness::session_store`, whose records carry
+# `serde` is required by `session`, whose records carry
# `DateTime<Utc>` timestamps across the serde boundary.-# Throwaway workspace roots for `harness::session_store` tests, which exercise
+# Throwaway workspace roots for `session` tests, which exercise
# the real on-disk SQLite path rather than an in-memory database.Also applies to: 61-62, 84-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` around lines 48 - 51, Update the three comments in Cargo.toml
that reference harness::session_store to use the promoted top-level session
module path, session::session_store, while preserving the existing bundled/FTS5
rationale and wording otherwise.
| //! See [`README.md`](./README.md) for the schema, the FTS behaviour, and the | ||
| //! coordination guarantees. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The relative README link does not resolve in rendered docs.
rustdoc does not copy src/session/README.md into the generated output. On docs.rs this link returns 404. Point the link at the repository file, or inline the relevant content with #![doc = include_str!("README.md")].
📝 Proposed fix
-//! See [`README.md`](./README.md) for the schema, the FTS behaviour, and the
-//! coordination guarantees.
+//! See `src/session/README.md` in the repository for the schema, the FTS
+//! behaviour, and the coordination guarantees.📝 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.
| //! See [`README.md`](./README.md) for the schema, the FTS behaviour, and the | |
| //! coordination guarantees. | |
| //! See `src/session/README.md` in the repository for the schema, the FTS | |
| //! behaviour, and the coordination guarantees. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/session/mod.rs` around lines 64 - 65, Update the module-level
documentation link near the session schema and FTS coordination text so it
resolves in rendered rustdoc: either point it to the repository-hosted README
file or inline the README via the module’s documentation attribute using
include_str!("README.md").
| with_connection(workspace_dir, |conn| { | ||
| conn.execute( | ||
| "INSERT INTO sessions ( | ||
| id, agent_definition_id, agent_definition_name, session_key, | ||
| parent_session_id, thread_id, source_channel, status, model, | ||
| transcript_path, started_at | ||
| ) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, 'running', ?8, ?9, ?10)", | ||
| params![ | ||
| id, | ||
| agent_definition_id, | ||
| agent_definition_name, | ||
| session_key, | ||
| parent_session_id, | ||
| thread_id, | ||
| source_channel, | ||
| model, | ||
| transcript_path, | ||
| now.to_rfc3339(), | ||
| ], | ||
| ) | ||
| .storage_context("failed to insert session")?; | ||
|
|
||
| index_fts_session(conn, id, agent_definition_name)?; | ||
| Ok(()) | ||
| })?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The row insert and its FTS index write are not atomic.
All three recording functions use with_connection, which is autocommit. Each conn.execute commits on its own. If index_fts_session, index_fts_content, or index_fts_tool fails, the sessions, session_messages, or session_tool_calls row is already committed and stays permanently absent from sessions_fts. The comment at lines 578-583 states this exact failure mode for the panic case, but the same gap remains for any FTS insert error, for example SQLITE_BUSY or a disk error.
with_transaction exists for this (src/session/store.rs line 59). Use it so the row and its index entry commit together.
🐛 Proposed fix for `record_message`; apply the same change to `record_session_start` and `record_tool_call`
- with_connection(workspace_dir, |conn| {
+ with_transaction(workspace_dir, |conn| {
conn.execute(
"INSERT INTO session_messages (Also applies to: 137-161, 197-219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/session/ops.rs` around lines 42 - 66, Use with_transaction instead of
with_connection in record_session_start, record_message, and record_tool_call so
each base-row insert and its corresponding index_fts_session, index_fts_content,
or index_fts_tool call commit atomically; preserve the existing SQL, parameters,
error context, and callback logic.
| Re-exported from the crate root (see `src/lib.rs`); the full surface stays | ||
| reachable under `session::` and `session::run_ledger::`. | ||
|
|
||
| - **Recording** — `record_session_start`, `record_message`, `record_tool_call`, | ||
| `record_session_end` | ||
| - **Querying** — `get_session`, `list_sessions`, `search_sessions`, | ||
| `list_messages`, `list_tool_calls`, `list_children` | ||
| - **Recovery** — `mark_interrupted` | ||
| - **Run ledger** — agent runs, workflow runs, teams, members, tasks, run events, | ||
| and telemetry, with the claim/completion coordination primitives | ||
| - **Connections** — `with_connection` (autocommit) and `with_transaction` | ||
| (`BEGIN IMMEDIATE`) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Two statements in this section do not match the code.
- Line 36 says the listed surface is "Re-exported from the crate root".
src/lib.rslines 100-104 re-export onlyget_session,list_sessions,record_message,record_session_end,record_session_start,record_tool_call, andsearch_sessions.list_messages,list_tool_calls,list_children,mark_interrupted,with_connection, andwith_transactionare reachable only undersession::. - Line 51 says "Six tables plus one FTS5 virtual table". The table below lists ten non-FTS tables:
sessions,session_messages,session_tool_calls,agent_runs,workflow_runs,run_events,run_telemetry,agent_teams,agent_team_members, andagent_team_tasks.
📝 Proposed fixes
-Re-exported from the crate root (see `src/lib.rs`); the full surface stays
-reachable under `session::` and `session::run_ledger::`.
+The recording and querying entry points and the run-ledger types are re-exported
+from the crate root (see `src/lib.rs`). The rest of the surface, marked below
+with †, is reachable only under `session::` and `session::run_ledger::`.-Six tables plus one FTS5 virtual table, created on demand and idempotently:
+Ten tables plus one FTS5 virtual table, created on demand and idempotently:Also applies to: 51-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/session/README.md` around lines 36 - 47, Update the session README
statements to match the actual API and schema: clarify that only the functions
re-exported by src/lib.rs are available at the crate root, while list_messages,
list_tool_calls, list_children, mark_interrupted, with_connection, and
with_transaction remain under session::. Correct the table-count description to
state that there are ten non-FTS tables plus one FTS5 virtual table, matching
the schema table.
| **Coordination operations need `with_transaction`, not `with_connection`.** | ||
| `with_connection` is autocommit, which gives ordering but no isolation. Claim | ||
| and completion read state and then act on it, so they take the write lock up | ||
| front with `BEGIN IMMEDIATE` — racing claims serialize at `BEGIN` rather than | ||
| failing at `COMMIT` after one has already decided it won. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This guarantee does not hold without a busy timeout.
The text states that racing claims "serialize at BEGIN rather than failing at COMMIT". src/session/store.rs sets no busy_timeout, so the second BEGIN IMMEDIATE returns SQLITE_BUSY immediately instead of waiting. See the comment on src/session/store.rs lines 55-65. Update this paragraph after that fix lands, or state the current behaviour.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/session/README.md` around lines 93 - 97, Update the README paragraph
describing coordination operations and BEGIN IMMEDIATE to reflect the current
SQLITE_BUSY behavior when no busy timeout is configured, or revise it after the
store’s busy-timeout handling is added; do not claim racing claims wait and
serialize unless the implementation guarantees that behavior.
| pub fn with_connection<T>( | ||
| workspace_dir: &Path, | ||
| f: impl FnOnce(&Connection) -> Result<T>, | ||
| ) -> Result<T> { | ||
| let db_path = db_path(workspace_dir); | ||
| if let Some(parent) = db_path.parent() { | ||
| std::fs::create_dir_all(parent).storage_context(&format!( | ||
| "failed to create session_db directory: {}", | ||
| parent.display() | ||
| ))?; | ||
| } | ||
|
|
||
| let conn = Connection::open(&db_path) | ||
| .storage_context(&format!("failed to open session DB: {}", db_path.display()))?; | ||
|
|
||
| init_schema(&conn)?; | ||
| f(&conn) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
init_schema runs on every operation.
with_connection opens a connection, calls create_dir_all, and then executes the full DDL batch for each call. record_message (src/session/ops.rs line 137) uses with_connection, so a run that records one message per turn re-parses and re-executes roughly twenty DDL statements plus two PRAGMA statements each time. src/session/run_ledger/ops.rs line 114 then calls init_run_ledger_schema on top of that.
The DDL is idempotent, so this is a cost concern, not a correctness one. Consider running the schema once per database path, for example with a std::sync::OnceLock<Mutex<HashSet<PathBuf>>> of initialized paths, or by checking PRAGMA user_version and skipping the batch when the version already matches.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/session/store.rs` around lines 27 - 44, Update with_connection so schema
initialization is performed once per database path instead of on every
operation. Add a synchronized per-path initialization guard, such as
OnceLock<Mutex<HashSet<PathBuf>>> or an equivalent user_version check, and call
init_schema only when the current database has not been initialized; preserve
connection creation and callback behavior.
| /// `BEGIN IMMEDIATE` rather than the default deferred begin: it takes the | ||
| /// write lock up front, so two racing claims serialize at `BEGIN` instead of | ||
| /// discovering the conflict at COMMIT time and failing with `SQLITE_BUSY` | ||
| /// after one of them has already decided it won. | ||
| pub fn with_transaction<T>( | ||
| workspace_dir: &Path, | ||
| f: impl FnOnce(&Connection) -> Result<T>, | ||
| ) -> Result<T> { | ||
| with_connection(workspace_dir, |conn| { | ||
| conn.execute_batch("BEGIN IMMEDIATE") | ||
| .storage_context("begin session DB transaction")?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No busy timeout is configured, so BEGIN IMMEDIATE does not serialize concurrent writers. SQLite's default busy timeout is 0 and with_connection opens a new connection per call. A second BEGIN IMMEDIATE therefore returns SQLITE_BUSY immediately instead of waiting for the write lock. Both the code comment and the README state the opposite guarantee.
src/session/store.rs#L55-L65: callconn.busy_timeout(...)inwith_connectionright afterConnection::open, so a racing writer waits for the lock instead of failing.src/session/README.md#L93-L97: keep the serialization claim only after the busy timeout is set, and name the configured timeout value.
📍 Affects 2 files
src/session/store.rs#L55-L65(this comment)src/session/README.md#L93-L97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/session/store.rs` around lines 55 - 65, Configure a nonzero busy timeout
immediately after opening each SQLite connection in with_connection, using the
established timeout value so concurrent BEGIN IMMEDIATE calls wait for the write
lock. Update src/session/README.md lines 93-97 to retain the serialization claim
only with this configured timeout and explicitly name its value; the store.rs
documentation should likewise reflect the timeout-backed behavior.
| fn tool_output_truncation() { | ||
| with_memory_connection(|conn| { | ||
| let session_id = "trunc-sess"; | ||
| insert_test_session(conn, session_id, "agent", "key"); | ||
|
|
||
| let large_output = "x".repeat(MAX_TOOL_OUTPUT_BYTES + 1000); | ||
| let bounded = if large_output.len() <= MAX_TOOL_OUTPUT_BYTES { | ||
| large_output.clone() | ||
| } else { | ||
| let mut cutoff = MAX_TOOL_OUTPUT_BYTES; | ||
| while cutoff > 0 && !large_output.is_char_boundary(cutoff) { | ||
| cutoff -= 1; | ||
| } | ||
| let mut truncated = large_output[..cutoff].to_string(); | ||
| truncated.push_str("\n...[truncated]"); | ||
| truncated | ||
| }; | ||
|
|
||
| conn.execute( | ||
| "INSERT INTO session_tool_calls (session_id, tool_name, tool_output, status, created_at) | ||
| VALUES (?1, 'test', ?2, 'ok', ?3)", | ||
| params![session_id, bounded, Utc::now().to_rfc3339()], | ||
| )?; | ||
|
|
||
| let stored: String = conn.query_row( | ||
| "SELECT tool_output FROM session_tool_calls WHERE session_id = ?1", | ||
| params![session_id], | ||
| |r| r.get(0), | ||
| )?; | ||
| assert!(stored.len() <= MAX_TOOL_OUTPUT_BYTES + 20); | ||
| assert!(stored.ends_with("[truncated]")); | ||
|
|
||
| Ok(()) | ||
| }) | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
These three tests do not call the functions they claim to cover.
tool_output_truncationcopies the truncation logic fromsrc/session/ops.rslines 183-195, inserts the result, and asserts on its own computation. Ifrecord_tool_callchanges its cap or drops the marker, this test still passes.mark_interrupted_updates_runningruns theUPDATEstatement directly instead of callingmark_interrupted.session_end_updates_cost_fieldsruns theUPDATEstatement directly instead of callingrecord_session_end.
tempfile is already a dev-dependency (Cargo.toml line 86). Use a TempDir workspace root and call the public functions, as src/session/run_ledger/test.rs does.
💚 Sketch for `mark_interrupted_updates_running`
#[test]
fn mark_interrupted_updates_running() {
- with_memory_connection(|conn| {
- insert_test_session(conn, "run1", "agent", "key1");
- ...
- let changed = conn.execute(
- "UPDATE sessions SET status = 'interrupted', ended_at = ?1
- WHERE status = 'running'",
- params![now.to_rfc3339()],
- )?;
- assert_eq!(changed, 1);
+ let dir = tempfile::TempDir::new().unwrap();
+ let workspace = dir.path();
+
+ record_session_start(
+ workspace, "run1", "agent", "agent", "key1", None, None, None, None, None,
+ )
+ .unwrap();
+ record_session_start(
+ workspace, "run2", "agent", "agent", "key2", None, None, None, None, None,
+ )
+ .unwrap();
+ record_session_end(
+ workspace, "run2", SessionStatus::Completed, 1, 0, 0, 0, 0.0,
+ )
+ .unwrap();
+
+ assert_eq!(mark_interrupted(workspace).unwrap(), 1);
+ assert_eq!(
+ get_session(workspace, "run1").unwrap().status,
+ SessionStatus::Interrupted
+ );
+ assert_eq!(
+ get_session(workspace, "run2").unwrap().status,
+ SessionStatus::Completed
+ );
}Also applies to: 266-298, 301-335
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/session/test.rs` around lines 228 - 263, Update the tests
tool_output_truncation, mark_interrupted_updates_running, and
session_end_updates_cost_fields to exercise the public session APIs instead of
duplicating their SQL or transformation logic. Use a TempDir workspace root and
invoke record_tool_call, mark_interrupted, and record_session_end respectively,
following the setup pattern in run_ledger tests; retain assertions on the
resulting persisted values and statuses.
| #[test] | ||
| fn wal_mode_is_set() { | ||
| with_memory_connection(|conn| { | ||
| let mode: String = conn.query_row("PRAGMA journal_mode", [], |r| r.get(0))?; | ||
| // In-memory DBs may report "memory" instead of "wal" | ||
| assert!(mode == "wal" || mode == "memory"); | ||
| Ok(()) | ||
| }) | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
wal_mode_is_set never checks WAL.
with_memory_connection opens :memory:. SQLite always reports memory for that database, so the mode == "wal" branch is unreachable and the assertion always passes on the wrong value. Open a TempDir-backed database to test the pragma that init_schema sets.
💚 Proposed fix
#[test]
fn wal_mode_is_set() {
- with_memory_connection(|conn| {
- let mode: String = conn.query_row("PRAGMA journal_mode", [], |r| r.get(0))?;
- // In-memory DBs may report "memory" instead of "wal"
- assert!(mode == "wal" || mode == "memory");
- Ok(())
- })
- .unwrap();
+ let dir = tempfile::TempDir::new().unwrap();
+ super::store::with_connection(dir.path(), |conn| {
+ let mode: String = conn.query_row("PRAGMA journal_mode", [], |r| r.get(0))?;
+ assert_eq!(mode, "wal");
+ Ok(())
+ })
+ .unwrap();
}📝 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.
| #[test] | |
| fn wal_mode_is_set() { | |
| with_memory_connection(|conn| { | |
| let mode: String = conn.query_row("PRAGMA journal_mode", [], |r| r.get(0))?; | |
| // In-memory DBs may report "memory" instead of "wal" | |
| assert!(mode == "wal" || mode == "memory"); | |
| Ok(()) | |
| }) | |
| .unwrap(); | |
| } | |
| #[test] | |
| fn wal_mode_is_set() { | |
| let dir = tempfile::TempDir::new().unwrap(); | |
| super::store::with_connection(dir.path(), |conn| { | |
| let mode: String = conn.query_row("PRAGMA journal_mode", [], |r| r.get(0))?; | |
| assert_eq!(mode, "wal"); | |
| Ok(()) | |
| }) | |
| .unwrap(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/session/test.rs` around lines 508 - 517, Update wal_mode_is_set to use a
temporary file-backed database instead of with_memory_connection, then
initialize it through init_schema before querying PRAGMA journal_mode. Assert
that the reported mode is "wal" and retain the test’s existing error propagation
and cleanup behavior.
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct SessionRecord { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serde casing is inconsistent across the public session types.
SessionSearchParams uses #[serde(rename_all = "camelCase")]. SessionRecord, SessionMessage, SessionToolCall, and SessionSearchResult use the default snake_case. Every run-ledger record type in src/session/run_ledger/types.rs uses camelCase. A JSON client therefore sends camelCase parameters and receives snake_case records.
Field names are a public serialization contract. Fix the convention now; changing it after release breaks consumers.
♻️ Proposed alignment on camelCase
#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
pub struct SessionRecord { #[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
pub struct SessionMessage { #[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
pub struct SessionToolCall {Also applies to: 80-82, 103-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/session/types.rs` around lines 33 - 34, Apply #[serde(rename_all =
"camelCase")] consistently to the public session types SessionRecord,
SessionMessage, SessionToolCall, and SessionSearchResult, matching
SessionSearchParams and the run-ledger types. Ensure their serialized field
names use camelCase without changing the Rust field names or other behavior.
…mmit tinyhumansai/tinyagents#90 merged as 107a515. The gitlink referenced a branch commit (2233c02) that predated the review fixes; it now points at merged main. Co-authored-by: Medulla <medulla@tinyhumans.ai>
1 participant
What
Adds
harness::session_store— the SQLite-backed (WAL + FTS5) history layer for agentsessions: sessions, messages, tool calls, cost metadata, parent/child lineage, plus a
run_ledgerfor background agent/workflow execution state.Ported from OpenHuman's
agent/session_db, which was generic runtime machinery sitting ina host. Its only coupling to that host was reading
workspace_diroff aConfig, so entrypoints now take
&Pathand the crate derives{workspace}/session_db/sessions.dbitself.No host type crosses the boundary.
Why this belongs here, and what deliberately does not
This is history, not durability — nothing resumes from it. Resume stays with
graph::checkpoint; live runtime data stays withharness::store. This answers"what happened", supports cross-session search, and lets a host recover orchestration
state after a restart.
A companion audit found two neighbouring bodies of code that look like they belong here
and do not. Recording them so this is not re-litigated:
session_importreads legacy OpenHuman formats (session_raw/,DDMMYYYYfolders, legacy Markdown) and writes them into this crate's stores. It is ahost-side adapter that already consumes
harness::store; moving it would put a host'slegacy directory layout inside the generic runtime.
transcript.rsis a durable on-disk format with.mdrendering. Upstreamingit would make a live user-data format public API of this crate. That is a crate-roadmap
decision, not a cleanup, and it is not proposed here.
Port notes
anyhowis not a dependency here, so error handling funnels through a newTinyAgentsError::Storagevariant.session_store/context.rsprovides aStorageContexttrait deliberately mirroringanyhow::Context's shape — including theOptionimpl for "row expected but absent" — which kept the conversion mechanical andreviewable.
From<rusqlite::Error>is added under thesqlitefeature so driver calls can use?.chronogainsserde: ledger records carryDateTime<Utc>across the serde boundary.bundledalready compiles it in, and there is nofts5feature at rusqlite 0.40 (adding one does not resolve). Noted inCargo.tomlsoit is not "fixed" later.
sqlitefeature, alongside the graph checkpointer.Semver note for reviewers
TinyAgentsErroris a public non-#[non_exhaustive]enum, so the newStoragevariant isa minor-breaking change for any downstream matching it exhaustively. Flagging it explicitly
rather than letting it ride in silently.
Verification
cargo test --features sqlite --lib— 1415 passed, 0 failed (34 are the moved suite,which came across intact, plus 4 new for the context shim).
cargo clippy --all-targets --all-features -- -D warnings— clean.cargo fmt --check— clean.The four wide
record_*signatures carry a scoped#[allow(clippy::too_many_arguments)]with a reason. Grouping them into a record struct is worth doing and is an API change
rather than part of a move, so it is left as follow-up.
Merge order — this PR goes FIRST
Head of a three-repo chain. Nothing blocks it; it depends on no other PR.
tinyhumansai/openhuman#5447(rewrites 29 call sites onto this crate, then repointsvendor/tinyagentsat this PR's merge SHA) →tinyhumansai/workflow-openhuman#6(advances theopenhuman/gitlink)After this merges, do not delete the
agent-sessions-to-tinyagentsbranch until #5447has repointed its submodule. That gitlink currently references a commit on this branch, and
deleting it first would leave #5447 pointing at an unreachable object.
Summary by CodeRabbit
New Features
Documentation
Tests