Merge pull request #3273 from nankingjing/fix-duplicate-assistant-text-3258

fix(cli): stop reprinting streamed assistant text after 'Done' in non-compact text mode

rust/crates/runtime/src/file_ops.rs

@@ -40,17 +40,32 @@ fn is_binary_file(path: &Path) -> io::Result<bool> {
 /// the workspace boundary (e.g. via `../` traversal or symlink).
 #[allow(dead_code)]
 fn validate_workspace_boundary(resolved: &Path, workspace_root: &Path) -> io::Result<()> {
-    if !resolved.starts_with(workspace_root) {
-        return Err(io::Error::new(
-            io::ErrorKind::PermissionDenied,
-            format!(
-                "path {} escapes workspace boundary {}",
-                resolved.display(),
-                workspace_root.display()
-            ),
-        ));
+    let workspace_root = normalize_for_comparison(workspace_root);
+    let comparable = normalize_for_comparison(resolved);
+    if comparable.starts_with(&workspace_root) || resolves_within(resolved, &workspace_root) {
+        return Ok(());
     }
-    Ok(())
+    Err(io::Error::new(
+        io::ErrorKind::PermissionDenied,
+        format!(
+            "path {} escapes workspace boundary {}",
+            comparable.display(),
+            workspace_root.display()
+        ),
+    ))
+}
+
+/// Whether `resolved` names a location inside `workspace_root` once the
+/// filesystem resolves it. Windows accepts several spellings of one location
+/// that [`Path::starts_with`] treats as different: letter case (a working
+/// directory typed as `c:\users\...` for `C:\Users\...`), the `\\.\` device
+/// namespace, and 8.3 short names. Only consulted after the lexical comparison
+/// fails, so it can accept such a spelling but never widens what already
+/// compared inside, and a path whose canonical form is outside stays rejected.
+fn resolves_within(resolved: &Path, workspace_root: &Path) -> bool {
+    resolved
+        .canonicalize()
+        .is_ok_and(|canonical| normalize_for_comparison(&canonical).starts_with(workspace_root))
 }
 
 /// Text payload returned by file-reading operations.
@@ -527,9 +542,29 @@ fn build_grep_content_output(
 }
 
 fn canonicalize_workspace_root(workspace_root: &Path) -> PathBuf {
-    workspace_root
+    let canonical = workspace_root
         .canonicalize()
-        .unwrap_or_else(|_| workspace_root.to_path_buf())
+        .unwrap_or_else(|_| workspace_root.to_path_buf());
+    normalize_for_comparison(&canonical)
+}
+
+/// Normalize Windows' extended-length path prefix before comparing paths.
+///
+/// `std::fs::canonicalize` returns paths with a `\\?\` prefix on Windows,
+/// while a workspace root may still be represented as a regular path when it
+/// cannot be canonicalized (for example, while a workspace is bootstrapping).
+/// These paths refer to the same location but do not compare equal with
+/// [`Path::starts_with`]. Keep the comparison representation stable without
+/// changing the paths returned to callers.
+fn normalize_for_comparison(path: &Path) -> PathBuf {
+    let raw = path.to_string_lossy();
+    if let Some(unc_path) = raw.strip_prefix(r"\\?\UNC\") {
+        return PathBuf::from(format!(r"\\{unc_path}"));
+    }
+    if let Some(dos_path) = raw.strip_prefix(r"\\?\") {
+        return PathBuf::from(dos_path);
+    }
+    path.to_path_buf()
 }
 
 fn should_skip_glob_dir(entry: &DirEntry) -> bool {
@@ -777,8 +812,9 @@ mod tests {
 
     use super::{
         component_contains_glob, derive_glob_walk_root, edit_file, expand_braces, glob_search,
-        grep_search, is_symlink_escape, read_file, read_file_in_workspace, write_file,
-        write_file_in_workspace, GrepSearchInput, MAX_WRITE_SIZE,
+        grep_search, is_symlink_escape, normalize_for_comparison, read_file,
+        read_file_in_workspace, write_file, write_file_in_workspace, GrepSearchInput,
+        MAX_WRITE_SIZE,
     };
 
     fn temp_path(name: &str) -> std::path::PathBuf {
@@ -906,6 +942,86 @@ mod tests {
         let _ = std::fs::remove_dir_all(&outside);
     }
 
+    #[test]
+    fn normalizes_windows_extended_length_prefixes_for_boundary_checks() {
+        let dos_path = PathBuf::from(r"C:\workspace\file.txt");
+        let extended_dos_path = PathBuf::from(r"\\?\C:\workspace\file.txt");
+        assert_eq!(
+            normalize_for_comparison(&extended_dos_path),
+            dos_path,
+            "extended DOS paths should compare like their regular form"
+        );
+
+        let unc_path = PathBuf::from(r"\\server\share\workspace\file.txt");
+        let extended_unc_path = PathBuf::from(r"\\?\UNC\server\share\workspace\file.txt");
+        assert_eq!(
+            normalize_for_comparison(&extended_unc_path),
+            unc_path,
+            "extended UNC paths should compare like their regular form"
+        );
+    }
+
+    // Backslash-separated paths only parse into path components on Windows.
+    #[test]
+    #[cfg(windows)]
+    fn accepts_equivalent_extended_path_at_workspace_boundary() {
+        let root = PathBuf::from(r"C:\workspace");
+        let resolved = PathBuf::from(r"\\?\C:\workspace\src\main.rs");
+        super::validate_workspace_boundary(&resolved, &root)
+            .expect("equivalent Windows path representations should be accepted");
+    }
+
+    #[test]
+    #[cfg(unix)]
+    fn accepts_a_path_that_resolves_inside_the_workspace() {
+        let workspace = temp_path("boundary-alias-workspace");
+        let aliases = temp_path("boundary-alias-links");
+        std::fs::create_dir_all(workspace.join("src")).expect("workspace dir should be created");
+        std::fs::create_dir_all(&aliases).expect("alias dir should be created");
+        let alias = aliases.join("src");
+        std::os::unix::fs::symlink(workspace.join("src"), &alias).expect("symlink should create");
+        let root = workspace
+            .canonicalize()
+            .expect("workspace should canonicalize");
+
+        super::validate_workspace_boundary(&alias, &root)
+            .expect("a spelling that resolves inside the workspace should be accepted");
+        super::validate_workspace_boundary(&aliases, &root)
+            .expect_err("a path that resolves outside the workspace must stay rejected");
+
+        let _ = std::fs::remove_dir_all(&workspace);
+        let _ = std::fs::remove_dir_all(&aliases);
+    }
+
+    // Windows resolves one location from spellings that compare unequal as
+    // paths, letter case and the `\\.\` device namespace among them.
+    #[test]
+    #[cfg(windows)]
+    fn accepts_other_windows_spellings_of_a_workspace_path() {
+        let workspace = temp_path("Boundary-Case-Workspace");
+        std::fs::create_dir_all(workspace.join("src")).expect("workspace dir should be created");
+        let root = workspace
+            .canonicalize()
+            .expect("workspace should canonicalize");
+
+        let lower = PathBuf::from(workspace.join("src").to_string_lossy().to_lowercase());
+        super::validate_workspace_boundary(&lower, &root)
+            .expect("a lower-cased spelling of a workspace path should be accepted");
+
+        let device = PathBuf::from(format!(r"\\.\{}", workspace.join("src").display()));
+        super::validate_workspace_boundary(&device, &root)
+            .expect("a device-namespace spelling of a workspace path should be accepted");
+
+        let parent = workspace
+            .parent()
+            .expect("workspace has a parent")
+            .to_path_buf();
+        super::validate_workspace_boundary(&parent, &root)
+            .expect_err("the workspace's parent must stay rejected");
+
+        let _ = std::fs::remove_dir_all(&workspace);
+    }
+
     #[test]
     #[cfg(unix)]
     fn workspace_write_rejects_parent_symlink_escape_regression_3007_class() {

rust/crates/rusty-claude-cli/src/main.rs

@@ -7763,15 +7763,20 @@ impl LiveCli {
         match result {
             Ok(summary) => {
                 self.replace_runtime(runtime)?;
+                // The assistant response was already streamed to the terminal live
+                // during the turn (emit_output = true), so reprinting the full text
+                // here would duplicate the entire message. When text was streamed,
+                // emit a newline first so `spinner.finish`'s line-clear lands on a
+                // fresh line instead of erasing the last streamed line.
+                let final_text = final_assistant_text(&summary);
+                if !final_text.is_empty() {
+                    println!();
+                }
                 spinner.finish(
                     "✨ Done",
                     TerminalRenderer::new().color_theme(),
                     &mut stdout,
                 )?;
-                let final_text = final_assistant_text(&summary);
-                if !final_text.is_empty() {
-                    println!("{final_text}");
-                }
                 println!();
                 if let Some(event) = summary.auto_compaction {
                     println!(
@@ -7903,6 +7908,9 @@ impl LiveCli {
                         match new_runtime.run_turn(input, Some(&mut rp)) {
                             Ok(summary) => {
                                 self.replace_runtime(new_runtime)?;
+                                if !final_assistant_text(&summary).is_empty() {
+                                    println!();
+                                }
                                 spinner.finish(
                                     if round == 0 {
                                         "✨ Done (after auto-compact)"

rust/crates/rusty-claude-cli/tests/compact_output.rs

@@ -130,7 +130,7 @@ fn compact_flag_streaming_text_only_emits_final_message_text() {
 }
 
 #[test]
-fn text_prompt_mode_prints_final_assistant_text_after_spinner() {
+fn text_prompt_mode_streams_assistant_text_once_before_spinner_completion() {
     // given a workspace pointed at the mock Anthropic service running the
     // streaming_text scenario which only emits a single assistant text block
     let runtime = tokio::runtime::Runtime::new().expect("tokio runtime should build");
@@ -162,7 +162,7 @@ fn text_prompt_mode_prints_final_assistant_text_after_spinner() {
         ],
     );
 
-    // then stdout should contain the final assistant text, not just spinner output
+    // then the streamed answer should appear once before the spinner completion
     assert!(
         output.status.success(),
         "text prompt run should succeed\nstdout:\n{}\n\nstderr:\n{}",
@@ -171,19 +171,19 @@ fn text_prompt_mode_prints_final_assistant_text_after_spinner() {
     );
     let stdout = String::from_utf8(output.stdout).expect("stdout should be utf8");
     let plain_stdout = strip_ansi_codes(&stdout);
-    assert!(
-        plain_stdout.contains("Mock streaming says hello from the parity harness."),
-        "text prompt stdout should include the assistant text ({stdout:?})"
+    let assistant_text = "Mock streaming says hello from the parity harness.";
+    assert_eq!(
+        plain_stdout.matches(assistant_text).count(),
+        1,
+        "text prompt stdout should contain the streamed assistant text exactly once ({stdout:?})"
     );
     assert!(
         plain_stdout.contains("✔ ✨ Done"),
         "text prompt stdout should still include spinner completion ({stdout:?})"
     );
     assert!(
-        plain_stdout
-            .lines()
-            .any(|line| line == "Mock streaming says hello from the parity harness."),
-        "text prompt stdout should print the assistant text as its own line ({stdout:?})"
+        plain_stdout.find(assistant_text) < plain_stdout.find("✔ ✨ Done"),
+        "text prompt stdout should stream the assistant text before spinner completion ({stdout:?})"
     );
 
     fs::remove_dir_all(&workspace).expect("workspace cleanup should succeed");

src/session_store.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import json
+import re
 from dataclasses import asdict, dataclass
 from pathlib import Path
 
@@ -15,16 +16,31 @@ class StoredSession:
 
 DEFAULT_SESSION_DIR = Path('.port_sessions')
 
+# Session ids become filenames inside the session directory, so they must not be
+# able to escape it via path separators or traversal. Restrict to the charset
+# produced by legitimate ids (uuid4 hex, `session-<digits>-<n>`).
+_SESSION_ID_PATTERN = re.compile(r'\A[A-Za-z0-9_-]+\Z')
+
+
+def _validate_session_id(session_id: str) -> str:
+    if not _SESSION_ID_PATTERN.match(session_id):
+        raise ValueError(
+            f'invalid session_id {session_id!r}: only [A-Za-z0-9_-] characters are allowed'
+        )
+    return session_id
+
 
 def save_session(session: StoredSession, directory: Path | None = None) -> Path:
+    session_id = _validate_session_id(session.session_id)
     target_dir = directory or DEFAULT_SESSION_DIR
     target_dir.mkdir(parents=True, exist_ok=True)
-    path = target_dir / f'{session.session_id}.json'
+    path = target_dir / f'{session_id}.json'
     path.write_text(json.dumps(asdict(session), indent=2))
     return path
 
 
 def load_session(session_id: str, directory: Path | None = None) -> StoredSession:
+    session_id = _validate_session_id(session_id)
     target_dir = directory or DEFAULT_SESSION_DIR
     data = json.loads((target_dir / f'{session_id}.json').read_text())
     return StoredSession(

tests/test_security_scope.py

@@ -9,9 +9,66 @@ from src.models import PermissionDenial
 from src.path_scope import WorkspacePathScope, extract_path_candidates
 from src.permissions import ToolPermissionContext
 from src.query_engine import QueryEnginePort
+from src.session_store import StoredSession, load_session, save_session
 from src.tools import execute_tool
 
 
+class SessionStorePathTraversalTests(unittest.TestCase):
+    def test_traversal_session_id_is_rejected_on_load(self) -> None:
+        with tempfile.TemporaryDirectory() as tmp:
+            store = Path(tmp) / 'store'
+            store.mkdir()
+            # A secret living outside the session store that traversal would reach.
+            (Path(tmp) / 'secret.json').write_text('{}')
+
+            with self.assertRaises(ValueError):
+                load_session('../secret', directory=store)
+
+    def test_absolute_and_separator_session_ids_are_rejected(self) -> None:
+        with tempfile.TemporaryDirectory() as tmp:
+            store = Path(tmp) / 'store'
+            store.mkdir()
+            for bad_id in (
+                '/etc/passwd',
+                'nested/child',
+                r'nested\child',
+                r'C:\Windows\win.ini',
+                '..',
+                'a/../../b',
+            ):
+                with self.assertRaises(ValueError):
+                    load_session(bad_id, directory=store)
+
+    def test_traversal_session_id_is_rejected_on_save(self) -> None:
+        with tempfile.TemporaryDirectory() as tmp:
+            store = Path(tmp) / 'store'
+            store.mkdir()
+            session = StoredSession(
+                session_id='../escape',
+                messages=('hello',),
+                input_tokens=1,
+                output_tokens=2,
+            )
+            with self.assertRaises(ValueError):
+                save_session(session, directory=store)
+            self.assertFalse((Path(tmp) / 'escape.json').exists())
+
+    def test_legitimate_session_id_round_trips(self) -> None:
+        with tempfile.TemporaryDirectory() as tmp:
+            store = Path(tmp) / 'store'
+            session = StoredSession(
+                session_id='session-1775386832313-0',
+                messages=('hi', 'there'),
+                input_tokens=3,
+                output_tokens=4,
+            )
+            path = save_session(session, directory=store)
+
+            self.assertEqual(store, path.parent)
+            loaded = load_session('session-1775386832313-0', directory=store)
+            self.assertEqual(session, loaded)
+
+
 class WorkspacePathScopeTests(unittest.TestCase):
     def test_direct_parent_escape_is_denied(self) -> None:
         with tempfile.TemporaryDirectory() as tmp: