diff --git a/docs/audits/2026-08-13-PR-566-rollback-noop-audit.md b/docs/audits/2026-08-13-PR-566-rollback-noop-audit.md new file mode 100644 index 00000000..5ec52ebd --- /dev/null +++ b/docs/audits/2026-08-13-PR-566-rollback-noop-audit.md @@ -0,0 +1,32 @@ +# PR #566 rollback no-op independent audit + +- Date: 2026-08-13 +- Audited branch: `codex/issue-140-rollback-semantics` after merging current `origin/main` +- Scope: `engine/src/chat_store.rs`, `engine/README.md` +- Verdict: **PASS** + +## Findings + +No blocking or non-blocking findings. + +## Independent assessment + +- Range validation happens before the no-op path, so invalid indices remain rejected without mutation. +- An explicit no-op is accepted only when the requested durable ID matches the persisted `active_leaf` with the repository's case-insensitive ULID matcher. +- Legacy no-op detection is deliberately narrower: `active_leaf` must be absent, all parent links must be absent, vector lengths must match, and the target must be the physical tail. +- A dangling persisted `active_leaf` cannot enter the no-op path. It reaches the normal save path, repairs the leaf to the requested message, increments revision, and updates persistence. +- Returning before `save()` preserves `updated_at`, revision, JSONL bytes, and metadata bytes for true no-op retries. +- Sibling branches are unaffected because rollback still removes only active-path entries after the selected target. +- The change is Engine-only; issue #319 visual review is not applicable. + +## Verification + +- `cargo test -p airp-core --lib chat_store::tests::rollback_to --locked -- --nocapture`: 5 passed. +- `cargo test -p airp-core --lib domain::tests::rollback --locked -- --nocapture`: 4 passed. +- `cargo test -p airp-core --lib chat_store::tests::rollback_repairs_dangling_persisted_active_leaf --locked -- --nocapture`: 1 passed. +- `cargo fmt --all -- --check`: passed. +- `git diff --check`: passed. + +## Non-blocking workflow note + +The Agent Browser Exploration failure is in generated test-script syntax before execution. Its workflow and report explicitly mark it non-blocking; it produced no crash, data-loss, or security evidence about this rollback change. The recurrence belongs to the already tracked agent-exploration process family and is not a product finding for #566. diff --git a/engine/README.md b/engine/README.md index e952294c..d36ff5ad 100644 --- a/engine/README.md +++ b/engine/README.md @@ -11,7 +11,7 @@ AIRP Engine 是 AIRP 产品内的无头 RP 引擎。它负责角色卡/世界书 - `/v1/agent/run` 有 step/token/wall-clock/cancel 闸和 typed SSE 事件; - Tavern Card JSON/PNG 导入、canonical/sidecar 落盘和角色 CRUD; - 会话创建/列表、history、append、rollback、regen; -- rollback 在 service/API 与 `ChatLog` 持久化边界都拒绝非法 index;空日志 `index=0` 保留兼容; +- rollback 在 service/API 与 `ChatLog` 持久化边界都拒绝非法 index;空日志 `index=0` 保留兼容;目标明确匹配持久化 `active_leaf`(或 legacy 线性日志的末条)时为纯 no-op,不刷新 `updated_at` / revision,也不写盘;dangling `active_leaf` 仍会修复并持久化; - chat pipeline 先持久化 user message 再推进时间线;assistant 的 live state、`ChatLog` 与 `current.md` 任一关键写入失败都会终止回合,SSE `done` 只在 finalization 全部成功后发送; - 多 Persona 存储、revision、HTTP CRUD/绑定,以及 chat pipeline 的显式/绑定/default 激活; - Persona 删除先验证 ID,revision 清理仅忽略 `NotFound`;路径穿越与其他清理错误 fail-closed 并保留工作副本; diff --git a/engine/src/chat_store.rs b/engine/src/chat_store.rs index 4a66dc4d..47a3ca81 100644 --- a/engine/src/chat_store.rs +++ b/engine/src/chat_store.rs @@ -914,6 +914,9 @@ impl ChatLog { /// #73 方案 B / #37:同步截断 `message_timestamps` / `message_ids` 保持等长。 /// #249:同步截断 `message_candidates` / `message_swipe_index`。 /// 分支对话树:同步截断 `message_parents`,更新 `active_leaf`。 + /// 若目标明确匹配持久化的 `active_leaf`,或日志是 `active_leaf=None` 的 legacy + /// 线性格式且目标为末条,则成功返回但不刷新 `updated_at`、不递增 revision、 + /// 不重写持久化文件。损坏的 dangling `active_leaf` 仍通过正常保存路径修复。 pub fn rollback_to(&mut self, data_root: &Path, index: usize) -> Result<(), AirpError> { let len = self.messages.len(); if (len == 0 && index != 0) || (len > 0 && index >= len) { @@ -935,6 +938,23 @@ impl ChatLog { )) })?; + let target_id = self.message_ids.get(index); + let targets_persisted_leaf = self + .active_leaf + .as_deref() + .is_some_and(|leaf| target_id.is_some_and(|target| crate::ulid::matches(target, leaf))); + let targets_legacy_linear_tail = self.active_leaf.is_none() + && self.message_parents.len() == len + && self.message_parents.iter().all(Option::is_none) + && index + 1 == len; + + // Only explicit persisted state (or the documented legacy linear shape) + // proves this is a no-op. resolve_active_leaf() also falls back for a + // dangling persisted leaf; that corrupt state must reach save() for repair. + if targets_persisted_leaf || targets_legacy_linear_tail { + return Ok(()); + } + // New active_leaf = message at `index` (BEFORE removal). let new_leaf_id = self.message_ids.get(index).cloned(); @@ -1963,6 +1983,115 @@ mod tests { assert!(matches!(err, AirpError::BadRequest(_))); } + #[test] + fn rollback_to_legacy_linear_tail_is_a_pure_noop() { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + make_char_dir(root, "rb_noop_char"); + + let mut seed = ChatLog::new("rb_noop_char"); + seed.save(root).unwrap(); + let jsonl_path = ChatLog::jsonl_path(root, "rb_noop_char"); + let meta_path = ChatLog::meta_path(root, "rb_noop_char"); + // Legacy lines intentionally omit id/ts/parent. Loading derives compatible + // in-memory values, but a no-op rollback must not persist that normalization. + fs::write( + &jsonl_path, + concat!( + "{\"role\":\"user\",\"content\":\"msg0\"}\n", + "{\"role\":\"user\",\"content\":\"msg1\"}\n" + ), + ) + .unwrap(); + let mut log = ChatLog::load_or_create(root, "rb_noop_char").unwrap(); + let jsonl_before = fs::read(&jsonl_path).unwrap(); + let meta_before = fs::read(&meta_path).unwrap(); + let updated_at_before = log.updated_at.clone(); + let revision_before = log.revision; + + log.rollback_to(root, 1).unwrap(); + + assert_eq!(log.updated_at, updated_at_before); + assert_eq!(log.revision, revision_before); + assert_eq!(fs::read(jsonl_path).unwrap(), jsonl_before); + assert_eq!(fs::read(meta_path).unwrap(), meta_before); + let reloaded = ChatLog::load_or_create(root, "rb_noop_char").unwrap(); + assert_eq!(reloaded.updated_at, updated_at_before); + assert_eq!(reloaded.revision, revision_before); + assert_eq!(reloaded.messages.len(), 2); + } + + #[test] + fn rollback_to_explicit_active_leaf_is_a_pure_noop() { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + make_char_dir(root, "rb_explicit_noop_char"); + + let mut log = ChatLog::new("rb_explicit_noop_char"); + for content in ["msg0", "msg1"] { + log.append( + root, + ChatMessage { + role: crate::adapter::MessageRole::User, + content: content.into(), + }, + ) + .unwrap(); + } + let target_id = log.message_ids[1].clone(); + log.active_leaf = Some(format!("m{}", target_id[1..].to_uppercase())); + log.save(root).unwrap(); + let meta_path = ChatLog::meta_path(root, "rb_explicit_noop_char"); + let meta_before = fs::read(&meta_path).unwrap(); + let updated_at_before = log.updated_at.clone(); + let revision_before = log.revision; + + log.rollback_to(root, 1).unwrap(); + + assert_eq!(log.updated_at, updated_at_before); + assert_eq!(log.revision, revision_before); + assert_eq!(fs::read(meta_path).unwrap(), meta_before); + } + + #[test] + fn rollback_repairs_dangling_persisted_active_leaf() { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + make_char_dir(root, "rb_dangling_leaf_char"); + + let mut log = ChatLog::new("rb_dangling_leaf_char"); + for content in ["msg0", "msg1"] { + log.append( + root, + ChatMessage { + role: crate::adapter::MessageRole::User, + content: content.into(), + }, + ) + .unwrap(); + } + let target_id = log.message_ids[1].clone(); + let dangling_id = crate::ulid::derive_legacy_id("missing-message", 99); + assert!(!crate::ulid::matches(&target_id, &dangling_id)); + log.active_leaf = Some(dangling_id); + log.updated_at = "2000-01-01T00:00:00+00:00".into(); + log.save(root).unwrap(); + let revision_before = log.revision; + + let mut reloaded = ChatLog::load_or_create(root, "rb_dangling_leaf_char").unwrap(); + // resolve_active_leaf falls back to the physical tail, but rollback must + // still persist the repair of the dangling metadata value. + assert_eq!(reloaded.resolve_active_leaf(), Some(target_id.as_str())); + reloaded.rollback_to(root, 1).unwrap(); + + assert_eq!(reloaded.active_leaf.as_deref(), Some(target_id.as_str())); + assert_eq!(reloaded.revision, revision_before + 1); + assert_ne!(reloaded.updated_at, "2000-01-01T00:00:00+00:00"); + let persisted = ChatLog::load_or_create(root, "rb_dangling_leaf_char").unwrap(); + assert_eq!(persisted.active_leaf.as_deref(), Some(target_id.as_str())); + assert_eq!(persisted.revision, revision_before + 1); + } + // ── #37 durable message-id contract 不变式 ────────────────────────────── fn make_char_dir(root: &Path, cid: &str) {