@sigitcodereview / sigit / commits / 22a4122

Address review feedback on session/list

Four things the review flagged: - `updatedAt` carried only whole seconds, so two threads saved in the same second were indistinguishable to a client sorting its import list by it. The formatter now emits milliseconds. - `persist_session` fell back to the process cwd when the session had no recorded one. That cwd may have been moved by a later session in the same process, which would file the thread under the wrong project; it now writes no sidecar and logs instead, leaving the thread unlisted rather than mis-listed. - Directory filtering canonicalized both sides and compared the raw paths when that failed, so a project directory that had been deleted could stop matching itself. `same_dir` now takes exact equality first and only canonicalizes to resolve symlinks and `..` for directories that exist. - Documented why a user message with no text hands the title to the next one, and covered it — plus a one-character prompt — with tests. Co-Authored-By: siGit Code <noreply@sigit.si> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Seto Elkahfi committed Sep 16, 2026 at 12:33 UTC 22a4122bce1ec743f118580ff161381e011b8e99

3 files changed +93 -29

src/main.rs

+53 -15

@@ -445,10 +445,21 @@ fn history_message_text(message: &serde_json::Value) -> String {

445 }

446 }

447

448 -/// Compare directories the way `session/list` filtering needs: resolve symlinks

449 -/// and `..` when the path exists, otherwise fall back to the path as given.

450 -fn normalize_dir(path: &std::path::Path) -> PathBuf {

451 - std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf())

448 +/// Do two paths name the same directory, for `session/list` filtering?

449 +///

450 +/// Canonicalizing resolves the symlinks and `..` segments that make a client's

451 +/// `cwd` and a stored one differ while naming the same place (`/tmp` vs

452 +/// `/private/tmp` on macOS). It only works on a directory that still exists,

453 +/// though, so a plain equality check runs first and keeps a session listed

454 +/// after its project directory has been moved or removed.

455 +fn same_dir(a: &std::path::Path, b: &std::path::Path) -> bool {

456 + if a == b {

457 + return true;

458 + }

459 + match (std::fs::canonicalize(a), std::fs::canonicalize(b)) {

460 + (Ok(a), Ok(b)) => a == b,

461 + _ => false,

462 + }

463 }

464

465 /// Rebuild the `session/update` notifications that re-draw a saved conversation

@@ -1433,21 +1444,21 @@ impl SiGitAgent {

1444 ///

1445 /// The sidecar is written here rather than at session start so a thread the

1446 /// user never spoke in doesn't leave one behind, and so a listed session

1436 - /// always has history to import.

1447 + /// always has history to import. Only the directory the client named on the

1448 + /// session request is recorded — never the process cwd, which a later

1449 + /// session in the same process may have moved: a thread listed under the

1450 + /// wrong project is worse than one that isn't listed at all.

1451 async fn persist_session(&self, session_id: &SessionId, snapshot: &[serde_json::Value]) {

1452 let key = session_id.to_string();

1453 if let Err(error) = session_store::save(&key, snapshot) {

1454 log::warn!("session({session_id}) save failed: {error}");

1455 return;

1456 }

1443 - let cwd = self

1444 - .session_cwd

1445 - .lock()

1446 - .ok()

1447 - .and_then(|guard| guard.clone())

1448 - .or_else(|| std::env::current_dir().ok());

1449 - if let Some(cwd) = cwd {

1450 - session_store::save_meta(&key, &cwd, &workspace::additional_roots());

1457 + match self.session_cwd.lock().ok().and_then(|guard| guard.clone()) {

1458 + Some(cwd) => session_store::save_meta(&key, &cwd, &workspace::additional_roots()),

1459 + None => log::warn!(

1460 + "session({session_id}) has no recorded cwd — saved, but it won't be listed"

1461 + ),

1462 }

1463 }

1464

@@ -1461,14 +1472,14 @@ impl SiGitAgent {

1472 &self,

1473 args: ListSessionsRequest,

1474 ) -> agent_client_protocol::Result<ListSessionsResponse> {

1464 - let filter = args.cwd.as_ref().map(|cwd| normalize_dir(cwd));

1475 + let filter = args.cwd.clone();

1476 let sessions: Vec<SessionInfo> = session_store::list()

1477 .into_iter()

1478 .filter(|entry| entry.message_count > 0)

1479 .filter_map(|entry| {

1480 let cwd = entry.cwd?;

1481 if let Some(filter) = &filter

1471 - && normalize_dir(&cwd) != *filter

1482 + && !same_dir(&cwd, filter)

1483 {

1484 return None;

1485 }

@@ -4253,6 +4264,33 @@ async fn main() -> anyhow::Result<()> {

4264 mod tests {

4265 use super::*;

4266

4267 + #[test]

4268 + fn same_dir_matches_through_symlinks_and_survives_a_missing_directory() {

4269 + let root = std::env::temp_dir().join(format!("sigit_same_dir_{}", std::process::id()));

4270 + let real = root.join("real");

4271 + let link = root.join("link");

4272 + let _ = std::fs::remove_dir_all(&root);

4273 + std::fs::create_dir_all(&real).unwrap();

4274 +

4275 + assert!(same_dir(&real, &real));

4276 + assert!(!same_dir(&real, &root.join("elsewhere")));

4277 +

4278 + // A stored path that reaches the same directory by another route still

4279 + // matches the client's.

4280 + assert!(same_dir(&root.join("real/../real"), &real));

4281 + #[cfg(unix)]

4282 + {

4283 + std::os::unix::fs::symlink(&real, &link).unwrap();

4284 + assert!(same_dir(&link, &real));

4285 + }

4286 +

4287 + // A project that has since been deleted can't be canonicalized; an

4288 + // exact path still matches, so its threads stay listable.

4289 + std::fs::remove_dir_all(&root).unwrap();

4290 + assert!(same_dir(&real, &real));

4291 + assert!(!same_dir(&link, &real));

4292 + }

4293 +

4294 #[test]

4295 fn session_context_message_lists_every_root_of_a_multi_root_project() {

4296 let primary = PathBuf::from("/tmp/primary");

src/session_store.rs

+38 -12

@@ -200,6 +200,12 @@ fn message_text(message: &Value) -> String {

200

201 /// A human-readable title for a session: the first line of its first user

202 /// message, trimmed and cut to [`TITLE_MAX_CHARS`].

203 +///

204 +/// A user message with no text at all — one that carried only an attachment,

205 +/// say — is passed over for the next one rather than producing a blank title;

206 +/// a message with any text, however short, wins. When no user message has text

207 +/// there is no title, and the client falls back to whatever it shows for an

208 +/// untitled thread.

209 fn session_title(contents: &str) -> Option<String> {

210 for line in contents.lines() {

211 if line.trim().is_empty() {

@@ -269,14 +275,19 @@ pub fn list() -> Vec<SessionEntry> {

275 sessions

276 }

277

272 -/// Format a timestamp as an ISO 8601 UTC instant (`2026-09-16T10:08:15Z`),

273 -/// which is what ACP's `SessionInfo.updatedAt` expects. Hand-rolled rather than

274 -/// pulling in a date crate for one field; pre-epoch times clamp to the epoch.

278 +/// Format a timestamp as an ISO 8601 UTC instant with milliseconds

279 +/// (`2026-09-16T10:08:15.123Z`), which is what ACP's `SessionInfo.updatedAt`

280 +/// expects. The sub-second part is not decoration: two sessions saved in the

281 +/// same second would otherwise carry identical timestamps and a client sorting

282 +/// its import list by `updatedAt` could not tell which thread is the recent

283 +/// one. Hand-rolled rather than pulling in a date crate for one field;

284 +/// pre-epoch times clamp to the epoch.

285 pub fn iso8601(time: std::time::SystemTime) -> String {

276 - let secs = time

286 + let since_epoch = time

287 .duration_since(std::time::UNIX_EPOCH)

278 - .map(|d| d.as_secs())

279 - .unwrap_or(0);

288 + .unwrap_or_default();

289 + let secs = since_epoch.as_secs();

290 + let millis = since_epoch.subsec_millis();

291 let days = (secs / 86_400) as i64;

292 let seconds_of_day = secs % 86_400;

293 let (hour, minute, second) = (

@@ -298,7 +309,7 @@ pub fn iso8601(time: std::time::SystemTime) -> String {

309 let month = if mp < 10 { mp + 3 } else { mp - 9 };

310 let year = year_of_era + era * 400 + i64::from(month <= 2);

311

301 - format!("{year:04}-{month:02}-{day:02}T{hour:02}:{minute:02}:{second:02}Z")

312 + format!("{year:04}-{month:02}-{day:02}T{hour:02}:{minute:02}:{second:02}.{millis:03}Z")

313 }

314

315 #[cfg(test)]

@@ -456,22 +467,37 @@ mod tests {

467 None

468 );

469 assert_eq!(session_title(""), None);

470 +

471 + // A user message with no text of its own — an attachment-only turn —

472 + // hands the title to the next one rather than coming back blank.

473 + let attachment_only = "{\"role\":\"user\",\"content\":\" \"}\n\

474 + {\"role\":\"user\",\"content\":\"the real question\"}\n";

475 + assert_eq!(

476 + session_title(attachment_only).as_deref(),

477 + Some("the real question")

478 + );

479 + // However short, any text wins.

480 + assert_eq!(

481 + session_title("{\"role\":\"user\",\"content\":\"?\"}\n").as_deref(),

482 + Some("?")

483 + );

484 }

485

486 #[test]

487 fn iso8601_formats_utc_instants() {

488 use std::time::{Duration, UNIX_EPOCH};

489

465 - assert_eq!(iso8601(UNIX_EPOCH), "1970-01-01T00:00:00Z");

466 - // 2026-09-16T10:08:15Z

490 + assert_eq!(iso8601(UNIX_EPOCH), "1970-01-01T00:00:00.000Z");

491 + // 2026-09-16T10:08:15.123Z — milliseconds keep two saves in the same

492 + // second apart when a client sorts its import list.

493 assert_eq!(

468 - iso8601(UNIX_EPOCH + Duration::from_secs(1_789_553_295)),

469 - "2026-09-16T10:08:15Z"

494 + iso8601(UNIX_EPOCH + Duration::from_millis(1_789_553_295_123)),

495 + "2026-09-16T10:08:15.123Z"

496 );

497 // A leap day, to exercise the civil-date arithmetic.

498 assert_eq!(

499 iso8601(UNIX_EPOCH + Duration::from_secs(1_709_164_800)),

474 - "2024-02-29T00:00:00Z"

500 + "2024-02-29T00:00:00.000Z"

501 );

502 }

503 }

tests/acp_session_list.rs

+2 -2

@@ -228,8 +228,8 @@ fn saved_sessions_are_listed_for_their_project() {

228 );

229 let updated_at = info["updatedAt"].as_str().unwrap_or_default();

230 assert!(

231 - updated_at.len() == 20 && updated_at.ends_with('Z'),

232 - "updatedAt should be an ISO 8601 UTC instant, got {updated_at:?}"

231 + updated_at.len() == 24 && updated_at.ends_with('Z') && updated_at.contains('.'),

232 + "updatedAt should be an ISO 8601 UTC instant with milliseconds, got {updated_at:?}"

233 );

234

235 // … and not for a different one.