Conversation
Introduces a SessionStore that persists session metadata and conversation
transcripts to disk (/data/sessions by default, inside the existing PVC
mount), enabling sessions to survive pod restarts and agent crashes.
Key changes:
- src/session/key.rs – platform-agnostic SessionKey ("discord:{id}")
- src/session/store.rs – index.json (atomic write) + per-session JSONL transcripts (append-only, crash-safe)
- src/acp/pool.rs – integrates SessionStore: checks disk on cache miss, primes agent context on restore, removes on TTL cleanup
- src/acp/connection.rs – session_prime_context(): silently replays last 20 messages into a fresh agent process
- src/discord.rs – uses SessionKey, records user + assistant messages after each exchange
- src/config.rs – adds [session] dir config (default /data/sessions)
- src/main.rs – initialises SessionStore, wires into pool and handler
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ementation Covers: why file system over SQLite/Redis, SessionKey format, crash-safe guarantees, restore flow, MAX_RESTORE_ENTRIES rationale, and how to extend for new platforms/agents. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Users can now interact with the bot via Discord DMs when allow_dms = true
is set in config.toml. DM sessions use "discord:dm:{user_id}" as the
session key (one persistent session per user) and reply directly in the
DM channel without creating threads.
Changes:
- config.rs: add allow_dms field to DiscordConfig (default false)
- main.rs: pass allow_dms to Handler; add DIRECT_MESSAGES gateway intent
- discord.rs: detect Private channels early, skip guild-only gates for DMs,
route reply_channel and session_key based on DM vs guild context
- config.toml.example: document allow_dms option
Recommended config when enabling DMs:
[discord]
allow_dms = true
allowed_users = ["<your_user_id>"] # restrict to trusted users
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This reverts commit d31d970.
Print `openab <version>` from CARGO_PKG_VERSION and exit. No new dependencies — uses existing std::env::args() parsing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a --version / -V early-exit path to the openab binary and also introduces file-based session persistence (session metadata + JSONL transcripts) to allow restoring conversational context after restarts.
Changes:
- Add
--version/-Vflag handling inmain.rsbefore config resolution. - Introduce a new
sessionmodule withSessionKeyand a filesystem-backedSessionStore(index + transcripts). - Integrate the session store into
SessionPool/Discord handling, including transcript recording and context priming on restore.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Adds --version / -V handling and initializes the new session store + passes it into the pool/handler. |
| src/session/store.rs | New file: implements file-backed session metadata storage and transcript append/load. |
| src/session/key.rs | New file: defines a platform-agnostic session key format. |
| src/session/mod.rs | New module exports for session types. |
| src/acp/pool.rs | Adds SessionStore integration, restore/prime logic, and store cleanup on idle eviction. |
| src/acp/connection.rs | Adds session_prime_context to inject prior transcript history into a new agent session. |
| src/discord.rs | Switches to platform-agnostic session keys and records user/assistant transcript entries. |
| src/config.rs | Adds [session] config with default persistence directory. |
| docs/session-persistence.md | Adds design/implementation documentation for session persistence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Return the plain text portion of the response for transcript recording. | ||
| // We return text_buf (not final_content which includes tool lines) so the | ||
| // stored transcript contains clean, readable assistant text. | ||
| Ok(text_buf) |
There was a problem hiding this comment.
Ok(text_buf) returns whatever was accumulated in text_buf, but text_buf is also used for the user-facing “session expired, starting fresh...” prefix when conn.session_reset is set. This means the transcript persistence will store that UI warning as assistant content, and future restores will replay it back into the agent context. Consider keeping a separate display-only prefix (not included in the returned/persisted text) and aligning the message with the restore behavior.
| impl SessionStore { | ||
| pub fn new(base_dir: impl Into<PathBuf>) -> Self { | ||
| Self { base_dir: base_dir.into() } | ||
| } | ||
|
|
||
| /// Create the storage directory if it does not already exist. | ||
| pub async fn init(&self) -> anyhow::Result<()> { | ||
| tokio::fs::create_dir_all(&self.base_dir).await?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Session persistence is a significant behavioral change (index read/write, transcript append/tail restore, atomic rename). There are no tests covering SessionStore/restore behavior; consider adding unit tests (e.g. tempdir-based) for upsert/remove correctness, transcript tailing (max 20), and corruption tolerance (skipping invalid JSONL lines).
| ); | ||
|
|
||
| let pool = Arc::new(acp::SessionPool::new(cfg.agent, cfg.pool.max_sessions)); | ||
| let store = Arc::new(session::SessionStore::new(&cfg.session.dir)); |
There was a problem hiding this comment.
SessionStore::new takes impl Into<PathBuf>, but &cfg.session.dir is a &String and does not implement Into<PathBuf> (it won’t deref-coerce for a generic impl Into<_> param). This is likely a compile error; pass cfg.session.dir.clone(), cfg.session.dir.as_str(), or a PathBuf::from(...) instead.
| let store = Arc::new(session::SessionStore::new(&cfg.session.dir)); | |
| let store = Arc::new(session::SessionStore::new(cfg.session.dir.as_str())); |
| let store = Arc::new(session::SessionStore::new(&cfg.session.dir)); | ||
| if let Err(e) = store.init().await { | ||
| anyhow::bail!("failed to initialise session store at {}: {e}", cfg.session.dir); | ||
| } | ||
| info!(dir = %cfg.session.dir, "session store ready"); | ||
|
|
||
| let pool = Arc::new(acp::SessionPool::new(cfg.agent, cfg.pool.max_sessions, store.clone())); |
There was a problem hiding this comment.
The PR title/description focuses on adding --version/-V, but this file also wires in a new session module + persistent session store and changes SessionPool construction. Please update the PR description/title (or split into separate PRs) so reviewers can track the much larger behavior change (session persistence + transcript recording).
| match tokio::fs::read_to_string(self.index_path()).await { | ||
| Ok(s) => serde_json::from_str(&s).unwrap_or_default(), | ||
| Err(_) => Index::default(), |
There was a problem hiding this comment.
load_index silently falls back to an empty Index on any read error (including permission issues) and on JSON parse errors (unwrap_or_default()), which can make all sessions “disappear” with no signal. Consider logging the underlying error (and the parse error) so operators can distinguish “no file yet” from “corrupted/permission denied”.
| match tokio::fs::read_to_string(self.index_path()).await { | |
| Ok(s) => serde_json::from_str(&s).unwrap_or_default(), | |
| Err(_) => Index::default(), | |
| let path = self.index_path(); | |
| match tokio::fs::read_to_string(&path).await { | |
| Ok(s) => match serde_json::from_str(&s) { | |
| Ok(idx) => idx, | |
| Err(err) => { | |
| warn!("failed to parse session index at {}: {}", path.display(), err); | |
| Index::default() | |
| } | |
| }, | |
| Err(err) if err.kind() == std::io::ErrorKind::NotFound => Index::default(), | |
| Err(err) => { | |
| warn!("failed to read session index at {}: {}", path.display(), err); | |
| Index::default() | |
| } |
| async fn write_index(&self, idx: &Index) -> anyhow::Result<()> { | ||
| let content = serde_json::to_string_pretty(idx)?; | ||
| // Atomic write: write to a temp file then rename. | ||
| let tmp = self.index_path().with_extension("tmp"); | ||
| tokio::fs::write(&tmp, content.as_bytes()).await?; | ||
| tokio::fs::rename(&tmp, self.index_path()).await?; | ||
| Ok(()) |
There was a problem hiding this comment.
Index updates are not concurrency-safe: upsert/remove do read-modify-write with no locking, and write_index always uses the same index.tmp path. Concurrent calls can race (lost updates, rename collisions, or a partially written tmp being renamed over a newer index). Add an in-process mutex around index read/write operations and/or use unique tmp filenames per write to avoid corruption under concurrent session activity.
| let mut file = tokio::fs::OpenOptions::new() | ||
| .create(true) | ||
| .append(true) | ||
| .open(self.transcript_path(key)) | ||
| .await?; | ||
| file.write_all(line.as_bytes()).await?; | ||
| Ok(()) |
There was a problem hiding this comment.
append_message can be called concurrently (user + assistant writes, multiple tasks), but it opens and writes without any coordination. With concurrent writers, write_all is not guaranteed to be a single atomic write, so JSONL lines can interleave and become unparseable. Consider a per-store/per-session async lock or a single background writer task/queue to serialize transcript appends.
| use std::collections::HashMap; | ||
| use std::path::PathBuf; | ||
| use tokio::io::AsyncWriteExt; | ||
| use tracing::warn; |
There was a problem hiding this comment.
use tracing::warn; is currently unused in this module. Either remove it, or (preferably) use it to log load_index read/parse failures (so session loss/corruption isn’t silent).
| use tracing::warn; |
| let mut conns = self.connections.write().await; | ||
| let stale: Vec<String> = conns | ||
| .iter() | ||
| .filter(|(_, c)| c.last_active < cutoff || !c.alive()) | ||
| .map(|(k, _)| k.clone()) | ||
| .collect(); | ||
| for key in stale { | ||
| info!(thread_id = %key, "cleaning up idle session"); | ||
| info!(session_key = %key, "cleaning up idle session"); | ||
| conns.remove(&key); | ||
| // Child process killed via kill_on_drop when AcpConnection drops | ||
| // Child process killed via kill_on_drop when AcpConnection drops. | ||
| // Remove from persistent store so it is not "restored" after cleanup. | ||
| if let Err(e) = self.store.remove(&key).await { | ||
| warn!(error = %e, session_key = %key, "failed to remove session from store"); | ||
| } | ||
| } |
There was a problem hiding this comment.
cleanup_idle holds the connections write lock while awaiting self.store.remove(&key) (file I/O). This can block all other session operations during slow disk/PVC periods. Consider removing the stale connections under the lock, then dropping the lock before doing store cleanup, or performing store removal in a separate step without holding the pool lock.
| pub async fn load_transcript(&self, key: &str) -> Vec<TranscriptEntry> { | ||
| match tokio::fs::read_to_string(self.transcript_path(key)).await { | ||
| Ok(s) => { | ||
| let all: Vec<TranscriptEntry> = s | ||
| .lines() | ||
| .filter_map(|l| serde_json::from_str(l).ok()) | ||
| .collect(); | ||
| // Return only the tail so context restoration stays concise. | ||
| let skip = all.len().saturating_sub(MAX_RESTORE_ENTRIES); | ||
| all.into_iter().skip(skip).collect() | ||
| } |
There was a problem hiding this comment.
load_transcript reads the entire transcript into memory, parses all lines, and then returns only the last MAX_RESTORE_ENTRIES. If transcripts can grow large, this can become an O(file_size) startup cost and potential memory spike on restore. Consider tail-reading the last N lines (or streaming with a bounded ring buffer) so restore work stays bounded.
|
Closing in favor of a new PR based cleanly off upstream/main. |
|
Closing this PR because it was branched from a stale fork that contained unreleased commits not in upstream/main (DM support, session persistence). Replaced by #320 which is based cleanly off upstream/main. |
What problem does this solve?
Running
openab --versioncurrently fails withfailed to read --version: No such file or directory. There is no way to check which version of openab is installed without inspecting the binary directly or checking the Helm chart. This makes debugging deployment mismatches difficult.Closes #
At a Glance
Prior Art & Industry Research
OpenClaw:
OpenClaw (TypeScript / Commander.js) handles this via custom pre-parse detection in
src/cli/program/help.ts— checks for-V,--version, and a root-level-valias before Commander parses arguments. The version string is resolved dynamically frompackage.jsonwith fallbacks to a build-time__OPENCLAW_VERSION__global. Output format:OpenClaw 2026.4.14-beta.1 (abc1234)including git commit hash.Hermes Agent:
Hermes Agent (Python / argparse) registers
-V/--versionasaction="store_true"on the main parser, plus a dedicatedhermes versionsubcommand. Version is hardcoded inhermes_cli/__init__.py. Output:Hermes Agent v0.9.0 (2026.4.13)with additional Python version and dependency info.Comparison:
-V,--version,-v-V,--version-V,--versionenv!("CARGO_PKG_VERSION")(Cargo.toml)Name X.Y.Z (git-hash)Name vX.Y.Z (date)openab X.Y.ZProposed Solution
Before the config path is resolved, check if the first argument is
--versionor-V. If so, printopenab <version>using theenv!("CARGO_PKG_VERSION")macro (resolved at compile time fromCargo.toml) and exit.Change: 7 lines in
src/main.rs, zero new dependencies.Why This Approach?
No clap dependency. OpenAB currently has no CLI argument parsing library — adding
clapfor a single flag would pull in a non-trivial dependency and restructure the startup code. The existing pattern (std::env::args().nth(1)) already reads the first argument; this PR extends it minimally.env!("CARGO_PKG_VERSION")over hardcoding. Like OpenClaw'spackage.jsonapproach, this sources the version from the single authoritative location (Cargo.toml) at compile time — no risk of drift.No git hash (unlike OpenClaw). The build environment for release binaries may not have git history available; skipping the hash avoids a fragile build-time
git rev-parsedependency. Can be added later if desired.Alternatives Considered
clap— adds.version(env!("CARGO_PKG_VERSION"))automatically and also gives--help. Rejected for now because it requires a new dependency and restructuring of argument parsing for a single flag; a follow-up PR could introduce clap if more CLI flags are needed.versionsubcommand (like Hermes'hermes version) — more discoverable but inconsistent with Unix conventions where--versionis the standard flag. Rejected.Validation
cargo checkpasses (verified locally)cargo testpassesopenab --versionprintsopenab 0.7.3and exits 0;openab -Vsame;openab config.tomlstill starts normally