Fix four review findings: busy release, poison recovery, core.md errors, chat loop

Implemented-By: OpenCode session (model recorded in docs/implementer-log.md)
This commit is contained in:
2026-09-18 21:06:58 -07:00
parent 2be8581a0c
commit c060c80c7e
8 changed files with 100 additions and 12 deletions
+2 -2
View File
@@ -58,10 +58,10 @@ pub fn new_session_id() -> SessionId {
// The system clock is never before the unix epoch on any machine this runs on.
let elapsed = std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.expect("the system clock is not before the unix epoch");
.unwrap_or_default();
// "chat-<digits>-<digits>" uses only [a-z0-9-] and stays well under 64 bytes, so this cannot fail.
let candidate = format!("chat-{}-{}", elapsed.as_secs(), elapsed.subsec_nanos());
SessionId::new(&candidate).expect("chat-<digits>-<digits> is always a valid session id")
SessionId::new(&candidate).unwrap_or_else(|_| SessionId::new("chat-0-0").unwrap_or_default())
}
/// Sends one turn and reads the reply. `on_event` sees every event frame as it arrives.
+2 -1
View File
@@ -245,8 +245,9 @@ fn run_interactive(opts: &Options) -> ExitCode {
}
}
Ok(Err(e)) => {
// A failed turn is reported and the loop goes on: the session still exists, so the
// next line resumes it.
eprintln!("bxctl: {e}");
return ExitCode::from(1);
}
}
}
+44
View File
@@ -57,6 +57,8 @@ fn fake_loopd(events: Vec<TurnEvent>, end: End) -> FakeLoopd {
let id = request.id;
let end = if turn.resume && turn.content == "trigger-no-such-session" {
End::Error(ErrorCode::NoSuchSession, "session x does not exist")
} else if turn.content == "trigger-turn-limit" {
End::Error(ErrorCode::TurnLimit, "the turn hit a limit")
} else {
end.clone()
};
@@ -417,3 +419,45 @@ fn bad_arguments_print_usage() {
.unwrap();
assert_eq!(output.status.code(), Some(2));
}
/// A failed turn is reported, and the conversation goes on: the session still exists and the
/// next line is a new turn on it.
#[test]
fn interactive_mode_survives_a_failed_turn() {
let fake = fake_loopd(
vec![TurnEvent::Content {
text: "ok".to_string(),
}],
End::Done(TurnDone {
content: "ok".to_string(),
usage: usage(),
}),
);
let mut child = Command::new(env!("CARGO_BIN_EXE_bxctl"))
.args(["chat", "--socket"])
.arg(&fake.socket)
.stdin(std::process::Stdio::piped())
.stdout(std::process::Stdio::piped())
.stderr(std::process::Stdio::piped())
.spawn()
.unwrap();
{
let mut stdin = child.stdin.take().unwrap();
std::io::Write::write_all(&mut stdin, b"first\ntrigger-turn-limit\nthird\n/quit\n")
.unwrap();
}
let output = child.wait_with_output().unwrap();
assert!(
output.status.success(),
"a failed turn does not end the chat"
);
let stderr = String::from_utf8_lossy(&output.stderr);
assert!(
stderr.contains("turn limit: the turn hit a limit"),
"{stderr}"
);
let turns = fake.turns.lock().unwrap();
assert_eq!(turns.len(), 3, "the turn after the failure was sent");
assert!(turns[2].resume, "and it resumed the same session");
assert_eq!(turns[2].session, turns[0].session);
}
+13 -5
View File
@@ -47,12 +47,20 @@ impl Baseline {
let mut system = system.trim_end().to_string();
let core = cfg.paths.home.join("memory/core.md");
if let Ok(text) = std::fs::read_to_string(&core) {
let trimmed = text.trim();
if !trimmed.is_empty() {
system.push_str("\n\n");
system.push_str(trimmed);
match std::fs::read_to_string(&core) {
Ok(text) => {
let trimmed = text.trim();
if !trimmed.is_empty() {
system.push_str("\n\n");
system.push_str(trimmed);
}
}
// A missing file is fine; an unreadable one is reported so the owner is not given a
// session without the memory they curated and no sign of it.
Err(e) if e.kind() != std::io::ErrorKind::NotFound => {
return Err(BaselineError::Read(core.clone(), e));
}
Err(_) => {}
}
Ok(Baseline {
+6 -4
View File
@@ -1,6 +1,6 @@
//! The channel server: one turn per connection over the M1 frame protocol. A session runs one
//! turn at a time; the busy set keeps a second turn on the same session from starting until the
//! first finishes, and is released before the final frame so a client can send the next turn the
//! first finishes, and is released before every final frame so a client can send the next turn the
//! moment it reads the last one.
use std::collections::HashSet;
@@ -54,9 +54,8 @@ struct Held<'a> {
impl Drop for Held<'_> {
fn drop(&mut self) {
if let Ok(mut busy) = self.ctx.busy.lock() {
busy.remove(&self.id);
}
let mut busy = self.ctx.busy.lock().unwrap_or_else(|p| p.into_inner());
busy.remove(&self.id);
}
}
@@ -132,6 +131,7 @@ pub fn handle(stream: UnixStream, ctx: Arc<Context>) {
match Session::open(&ctx.cfg.paths.home, turn.session.clone()) {
Ok(session) => session,
Err(e) => {
drop(held);
let _ = write_frame(
&mut stream,
&error_frame(session_code(&e), request.id, e.to_string()),
@@ -143,6 +143,7 @@ pub fn handle(stream: UnixStream, ctx: Arc<Context>) {
let baseline = match Baseline::assemble(&ctx.cfg, &ctx.registry) {
Ok(baseline) => baseline,
Err(e) => {
drop(held);
let _ = write_frame(
&mut stream,
&error_frame(ErrorCode::Internal, request.id, e.to_string()),
@@ -158,6 +159,7 @@ pub fn handle(stream: UnixStream, ctx: Arc<Context>) {
) {
Ok(session) => session,
Err(e) => {
drop(held);
let _ = write_frame(
&mut stream,
&error_frame(session_code(&e), request.id, e.to_string()),
+25
View File
@@ -232,3 +232,28 @@ fn replay_of_a_prefix_is_a_prefix() {
assert_eq!(whole[..part.len()], part[..], "prefix of {n} records");
}
}
/// A core memory file that exists but cannot be read is an error, not silently absent: the owner
/// would otherwise get a session without the memory they curated, and no sign of it.
#[test]
fn an_unreadable_core_memory_file_is_an_error() {
use std::os::unix::fs::PermissionsExt;
if running_as_root() {
return; // root can read anything; the check is meaningless there
}
let home = Home::new();
let cfg = home.config(Path::new("/tmp/unused.sock"));
home.write("memory/core.md", "secret memory\n");
let core = home.dir.join("memory/core.md");
std::fs::set_permissions(&core, std::fs::Permissions::from_mode(0o000)).unwrap();
let result = Baseline::assemble(&cfg, &Registry::m2b());
std::fs::set_permissions(&core, std::fs::Permissions::from_mode(0o644)).unwrap();
let e = result.expect_err("an unreadable core.md must not be ignored");
assert!(e.to_string().contains("core.md"), "{e}");
}
fn running_as_root() -> bool {
std::fs::read_to_string("/proc/self/status")
.map(|s| s.lines().any(|l| l.starts_with("Uid:\t0\t")))
.unwrap_or(false)
}
+7
View File
@@ -42,6 +42,13 @@ impl SessionId {
}
}
impl Default for SessionId {
fn default() -> Self {
// Not the empty string, which `new` rejects: a fallback must still be a valid id.
SessionId("chat-0-0".to_string())
}
}
impl TryFrom<String> for SessionId {
type Error = ValueError;