Merge origin/main: resolve session_store conflict, fix task.py self-import, remove unused sys import

Co-authored-by: badMade <106821302+badMade@users.noreply.github.com>

Author: Copilot

.jules/sentinel.md

@@ -0,0 +1,4 @@
+## 2024-04-10 - Path Traversal in Session Storage
+**Vulnerability:** Path traversal existed in both the Python (`src/session_store.py`) and Rust (`rust/crates/runtime/src/session_control.rs`) implementations because unsanitized `session_id` strings were used directly in file paths.
+**Learning:** Both reference implementations lacked central validation logic for system identifiers derived from external/user input.
+**Prevention:** Always validate and restrict identifier parameters (like session IDs) by checking for explicit disallow-lists (like path separators `/`, `\`, and directory traversal markers `.`, `..`) before using them in file operations.

docs/issue_task_proposals.md

@@ -0,0 +1,25 @@
+# Codebase issue scan: proposed fix tasks
+
+## 1) Naming consistency task
+- **Issue:** The HTTP user agent string uses `clawd-rust-tools/0.1`, while the codebase appears to use a mix of `claw*` and `clawd*` identifiers across repository/product naming, telemetry, env vars, and file prefixes.
+- **Impact:** Inconsistent telemetry/log tagging and harder grep/observability across services when searching for either `claw` or `clawd` identifiers.
+- **Task proposal:** Decide on the intended prefix convention for these identifiers (`claw*` vs `clawd*`), update the user-agent token to match that convention (ideally via a centrally-defined crate/version-derived user-agent constant), and update any tests that assert the old literal.
+- **Acceptance criteria:** The chosen naming convention is applied consistently to the user-agent token and any related references/tests updated by this task; no behavior changes beyond identifier consistency.
+
+## 2) Bug fix task
+- **Issue:** `src.main` ignores filtering flags (e.g., `--no-plugin-commands`, `--no-skill-commands` for `commands`; `--simple-mode`, `--no-mcp`, `--deny-tool` for `tools`) when `--query ...` is used. This happens because the query paths call `render_command_index(...)` or `render_tool_index(...)` directly without applying the include/exclude filters.
+- **Impact:** CLI behavior is inconsistent and user-provided filtering flags are silently ignored for query usage.
+- **Task proposal:** Refactor command and tool index rendering so query and non-query paths share the same filtered source (e.g., call `get_commands(...)`/`get_tools(...)` first, then apply query search over that filtered set).
+- **Acceptance criteria:** `commands --query X --no-plugin-commands` and `tools --query Y --no-mcp` produce outputs that respect each exclusion flag.
+
+## 3) Code comment/documentation discrepancy task
+- **Issue:** The lane completion module-level docs say completion is detected when "Code pushed (has output file)", but `detect_lane_completion(...)` does not inspect `output_file`; it only trusts the external boolean `has_pushed`.
+- **Impact:** The inline documentation overstates what this function validates itself, which can mislead future maintainers and reviewers.
+- **Task proposal:** Align docs/comments with implementation **or** update implementation to validate `output.output_file` semantics directly (and document exact source-of-truth for "pushed").
+- **Acceptance criteria:** Comments/docs and runtime behavior agree on how push status is determined.
+
+## 4) Test improvement task
+- **Issue:** Snapshot-size tests in `tests/test_porting_workspace.py` use hard minimums (`>=150` commands, `>=100` tools, coverage thresholds), which are brittle for legitimate snapshot churn and can produce noisy failures.
+- **Impact:** Reduced test signal quality; harmless snapshot updates can fail CI even when behavior remains correct.
+- **Task proposal:** Replace absolute magic-number thresholds with invariant-based assertions (e.g., non-empty snapshots, monotonic schema checks, command/tool lookup sanity) plus a single snapshot-contract check tied to explicit version metadata.
+- **Acceptance criteria:** Tests fail only on semantic regressions, not ordinary curated snapshot updates.

patch_pr.py

@@ -1,5 +1,3 @@
-import sys
-
 
 def replace_in_file(filepath, search_str, replace_str):
     with open(filepath, "r") as f:

rust/crates/runtime/src/session_control.rs

@@ -74,13 +74,17 @@ impl SessionStore {
         &self.workspace_root
     }
 
-    #[must_use]
-    pub fn create_handle(&self, session_id: &str) -> SessionHandle {
+    pub fn create_handle(&self, session_id: &str) -> Result<SessionHandle, SessionControlError> {
+        if !is_valid_session_id(session_id) {
+            return Err(SessionControlError::Format(format!(
+                "Invalid session ID: {session_id}"
+            )));
+        }
         let id = session_id.to_string();
         let path = self
             .sessions_root
             .join(format!("{id}.{PRIMARY_SESSION_EXTENSION}"));
-        SessionHandle { id, path }
+        Ok(SessionHandle { id, path })
     }
 
     pub fn resolve_reference(&self, reference: &str) -> Result<SessionHandle, SessionControlError> {
@@ -116,6 +120,11 @@ impl SessionStore {
     }
 
     pub fn resolve_managed_path(&self, session_id: &str) -> Result<PathBuf, SessionControlError> {
+        if !is_valid_session_id(session_id) {
+            return Err(SessionControlError::Format(format!(
+                "Invalid session ID: {session_id}"
+            )));
+        }
         for extension in [PRIMARY_SESSION_EXTENSION, LEGACY_SESSION_EXTENSION] {
             let path = self.sessions_root.join(format!("{session_id}.{extension}"));
             if path.exists() {
@@ -223,7 +232,7 @@ impl SessionStore {
     ) -> Result<ForkedManagedSession, SessionControlError> {
         let parent_session_id = session.session_id.clone();
         let forked = session.fork(branch_name);
-        let handle = self.create_handle(&forked.session_id);
+        let handle = self.create_handle(&forked.session_id)?;
         let branch_name = forked
             .fork
             .as_ref()
@@ -343,12 +352,25 @@ pub fn create_managed_session_handle_for(
     base_dir: impl AsRef<Path>,
     session_id: &str,
 ) -> Result<SessionHandle, SessionControlError> {
+    if !is_valid_session_id(session_id) {
+        return Err(SessionControlError::Format(format!(
+            "Invalid session ID: {session_id}"
+        )));
+    }
     let id = session_id.to_string();
     let path =
         managed_sessions_dir_for(base_dir)?.join(format!("{id}.{PRIMARY_SESSION_EXTENSION}"));
     Ok(SessionHandle { id, path })
 }
 
+#[must_use]
+pub fn is_valid_session_id(session_id: &str) -> bool {
+    if session_id.is_empty() || session_id == "." || session_id.contains("..") {
+        return false;
+    }
+    !session_id.contains(['/', '\\'])
+}
+
 pub fn resolve_session_reference(reference: &str) -> Result<SessionHandle, SessionControlError> {
     resolve_session_reference_for(env::current_dir()?, reference)
 }
@@ -397,6 +419,11 @@ pub fn resolve_managed_session_path_for(
     base_dir: impl AsRef<Path>,
     session_id: &str,
 ) -> Result<PathBuf, SessionControlError> {
+    if !is_valid_session_id(session_id) {
+        return Err(SessionControlError::Format(format!(
+            "Invalid session ID: {session_id}"
+        )));
+    }
     let directory = managed_sessions_dir_for(base_dir)?;
     for extension in [PRIMARY_SESSION_EXTENSION, LEGACY_SESSION_EXTENSION] {
         let path = directory.join(format!("{session_id}.{extension}"));
@@ -713,7 +740,9 @@ mod tests {
         session
             .push_user_text(text)
             .expect("session message should save");
-        let handle = store.create_handle(&session.session_id);
+        let handle = store
+            .create_handle(&session.session_id)
+            .expect("handle should create");
         let session = session.with_persistence_path(handle.path.clone());
         session
             .save_to_path(&handle.path)

src/session_store.py

@@ -16,7 +16,19 @@ class StoredSession:
 DEFAULT_SESSION_DIR = Path(".port_sessions")
 
 
+def validate_session_id(session_id: str) -> None:
+    if "/" in session_id or "\\" in session_id:
+        raise ValueError(f"Invalid session ID: contains path separators ({session_id})")
+    if session_id in (".", ".."):
+        raise ValueError(f"Invalid session ID: cannot be '.' or '..' ({session_id})")
+    if ".." in session_id:
+        raise ValueError(
+            f"Invalid session ID: contains directory traversal ('..') ({session_id})"
+        )
+
+
 def save_session(session: StoredSession, directory: Path | None = None) -> Path:
+    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"
@@ -25,6 +37,7 @@ def save_session(session: StoredSession, directory: Path | None = None) -> Path:
 
 
 def load_session(session_id: str, directory: Path | None = None) -> StoredSession:
+    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(

src/task.py

@@ -1,5 +1,12 @@
 from __future__ import annotations
 
-from .task import PortingTask
+from dataclasses import dataclass
+
+
+@dataclass(frozen=True)
+class PortingTask:
+    name: str
+    description: str
+
 
 __all__ = ["PortingTask"]