From 86bc6cf4d6f3a0d6b86f116cf2e995cb6e9b7685 Mon Sep 17 00:00:00 2001 From: Tyler Miller Date: Wed, 4 Mar 2026 17:27:41 -0700 Subject: [PATCH] Fix bare except, duplicate TaskStatus, and shell quoting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - check_working_tree_status() has a bare except that catches everything and returns {"clean": True}. So if git isn't installed or something goes wrong, callers just think the tree is clean with no indication anything failed. Narrowed it to FileNotFoundError and added an error dict so the caller actually knows what happened. - TaskStatus is defined identically in both task_storage.py and impl_tasks.py. Consolidated into task_storage.py so there's a single definition to maintain. - Same shell quoting fix as deep-project — unquoted exports in capture-session-id.py break when paths have spaces in them. All 146 existing tests pass plus 19 hook tests. --- .../checks/setup_implementation_session.py | 8 ++-- scripts/hooks/capture-session-id.py | 8 ++-- scripts/lib/impl_tasks.py | 9 +--- tests/hooks/test_capture_session_id.py | 28 +++++++++-- tests/test_setup_implementation_session.py | 46 +++++++++++++++++++ tests/test_task_storage.py | 37 +++++++++++++++ 6 files changed, 115 insertions(+), 21 deletions(-) diff --git a/scripts/checks/setup_implementation_session.py b/scripts/checks/setup_implementation_session.py index ae96f75..2a8ad43 100644 --- a/scripts/checks/setup_implementation_session.py +++ b/scripts/checks/setup_implementation_session.py @@ -224,10 +224,10 @@ def check_working_tree_status(git_root: Path) -> dict: # Fallback: just strip the status chars dirty_files.append(line[3:].strip()) return {"clean": len(dirty_files) == 0, "dirty_files": dirty_files} - except Exception: - pass - - return {"clean": True, "dirty_files": []} + else: + return {"clean": False, "dirty_files": [], "error": result.stderr.strip()} + except FileNotFoundError: + return {"clean": False, "dirty_files": [], "error": "git not found"} def detect_commit_style(git_root: Path) -> str: diff --git a/scripts/hooks/capture-session-id.py b/scripts/hooks/capture-session-id.py index e8aebaf..be51f73 100644 --- a/scripts/hooks/capture-session-id.py +++ b/scripts/hooks/capture-session-id.py @@ -87,14 +87,14 @@ def main() -> int: pass lines_to_write = [] - if f"DEEP_SESSION_ID={session_id}" not in existing_content: - lines_to_write.append(f"export DEEP_SESSION_ID={session_id}\n") + if f'DEEP_SESSION_ID="{session_id}"' not in existing_content: + lines_to_write.append(f'export DEEP_SESSION_ID="{session_id}"\n') if ( transcript_path - and f"CLAUDE_TRANSCRIPT_PATH={transcript_path}" not in existing_content + and f'CLAUDE_TRANSCRIPT_PATH="{transcript_path}"' not in existing_content ): lines_to_write.append( - f"export CLAUDE_TRANSCRIPT_PATH={transcript_path}\n" + f'export CLAUDE_TRANSCRIPT_PATH="{transcript_path}"\n' ) if lines_to_write: diff --git a/scripts/lib/impl_tasks.py b/scripts/lib/impl_tasks.py index 50f6a7a..ac05b5d 100644 --- a/scripts/lib/impl_tasks.py +++ b/scripts/lib/impl_tasks.py @@ -6,15 +6,8 @@ """ from dataclasses import dataclass -from enum import StrEnum - -class TaskStatus(StrEnum): - """Status values for tasks.""" - - PENDING = "pending" - IN_PROGRESS = "in_progress" - COMPLETED = "completed" +from scripts.lib.task_storage import TaskStatus @dataclass(frozen=True, slots=True, kw_only=True) diff --git a/tests/hooks/test_capture_session_id.py b/tests/hooks/test_capture_session_id.py index e8ebbf7..6f84a09 100644 --- a/tests/hooks/test_capture_session_id.py +++ b/tests/hooks/test_capture_session_id.py @@ -121,7 +121,7 @@ def test_writes_to_claude_env_file_when_available(self, tmp_path): # Should also write to env file content = env_file.read_text() - assert "export DEEP_SESSION_ID=test-session-456" in content + assert 'export DEEP_SESSION_ID="test-session-456"' in content def test_writes_transcript_path_to_env_file(self, tmp_path): """Should write transcript_path to CLAUDE_ENV_FILE.""" @@ -138,13 +138,13 @@ def test_writes_transcript_path_to_env_file(self, tmp_path): assert returncode == 0 content = env_file.read_text() - assert "export DEEP_SESSION_ID=test-session-789" in content - assert "export CLAUDE_TRANSCRIPT_PATH=/path/to/transcript.jsonl" in content + assert 'export DEEP_SESSION_ID="test-session-789"' in content + assert 'export CLAUDE_TRANSCRIPT_PATH="/path/to/transcript.jsonl"' in content def test_does_not_duplicate_existing_entries(self, tmp_path): """Should not duplicate entries already in CLAUDE_ENV_FILE.""" env_file = tmp_path / "env_file.sh" - env_file.write_text("export DEEP_SESSION_ID=test-session-111\n") + env_file.write_text('export DEEP_SESSION_ID="test-session-111"\n') payload = {"session_id": "test-session-111"} returncode, _, _ = run_hook( @@ -154,7 +154,7 @@ def test_does_not_duplicate_existing_entries(self, tmp_path): assert returncode == 0 content = env_file.read_text() # Should only appear once - assert content.count("DEEP_SESSION_ID=test-session-111") == 1 + assert content.count('DEEP_SESSION_ID="test-session-111"') == 1 def test_succeeds_when_claude_env_file_not_set(self): """Should succeed even when CLAUDE_ENV_FILE is not set.""" @@ -241,3 +241,21 @@ def test_outputs_when_deep_session_id_not_set(self): assert returncode == 0 output = json.loads(stdout) assert output["hookSpecificOutput"]["additionalContext"] == "DEEP_SESSION_ID=test-session-789" + + +class TestShellQuoting: + """Tests for shell quoting: export values should be double-quoted.""" + + def test_values_with_spaces_are_correctly_quoted(self, tmp_path): + """Values containing spaces should be correctly quoted.""" + env_file = tmp_path / "env_file.sh" + env_file.touch() + + payload = { + "session_id": "test-session-456", + "transcript_path": "/path/with spaces/transcript.jsonl", + } + run_hook(json.dumps(payload), env={"CLAUDE_ENV_FILE": str(env_file)}) + + content = env_file.read_text() + assert 'export CLAUDE_TRANSCRIPT_PATH="/path/with spaces/transcript.jsonl"' in content diff --git a/tests/test_setup_implementation_session.py b/tests/test_setup_implementation_session.py index 7c4ea2e..0ea2d19 100644 --- a/tests/test_setup_implementation_session.py +++ b/tests/test_setup_implementation_session.py @@ -225,6 +225,52 @@ def test_modified_tracked_file(self, mock_git_repo): assert result["clean"] is False assert "README.md" in result["dirty_files"] + def test_file_not_found_error_returns_error_dict(self, mock_git_repo, mocker): + """FileNotFoundError (git not installed) returns error dict, not clean=True.""" + mocker.patch( + "scripts.checks.setup_implementation_session.subprocess.run", + side_effect=FileNotFoundError("git not found"), + ) + result = check_working_tree_status(mock_git_repo) + + assert result["clean"] is False + assert "git not found" in result.get("error", "") + assert result["dirty_files"] == [] + + def test_nonzero_returncode_returns_error_dict(self, mock_git_repo, mocker): + """Non-zero returncode returns error dict with stderr.""" + mocker.patch( + "scripts.checks.setup_implementation_session.subprocess.run", + return_value=subprocess.CompletedProcess( + args=["git", "status"], + returncode=128, + stdout="", + stderr="fatal: not a git repository", + ), + ) + result = check_working_tree_status(mock_git_repo) + + assert result["clean"] is False + assert "fatal: not a git repository" in result.get("error", "") + + def test_permission_error_propagates(self, mock_git_repo, mocker): + """PermissionError should NOT be caught (propagates to caller).""" + mocker.patch( + "scripts.checks.setup_implementation_session.subprocess.run", + side_effect=PermissionError("Permission denied"), + ) + with pytest.raises(PermissionError): + check_working_tree_status(mock_git_repo) + + def test_no_bare_except_in_source(self): + """check_working_tree_status should not contain 'except Exception'.""" + import inspect + source = inspect.getsource(check_working_tree_status) + assert "except Exception" not in source, ( + "check_working_tree_status still has bare 'except Exception' — " + "only FileNotFoundError should be caught" + ) + class TestDetectCommitStyle: """Tests for detect_commit_style function.""" diff --git a/tests/test_task_storage.py b/tests/test_task_storage.py index 72bc98f..205456d 100644 --- a/tests/test_task_storage.py +++ b/tests/test_task_storage.py @@ -256,3 +256,40 @@ def test_returns_correct_path(self, monkeypatch): result = get_tasks_dir("my-session-id") assert result == Path("/Users/test/.claude/tasks/my-session-id") + + +class TestTaskStatusConsolidation: + """Tests for TaskStatus consolidation — should be defined only in task_storage.""" + + def test_task_storage_exports_task_status(self): + """TaskStatus should be importable from task_storage.""" + from scripts.lib.task_storage import TaskStatus + assert hasattr(TaskStatus, "PENDING") + assert hasattr(TaskStatus, "IN_PROGRESS") + assert hasattr(TaskStatus, "COMPLETED") + + def test_impl_tasks_imports_task_status(self): + """TaskStatus should be importable from impl_tasks (via re-export or import).""" + from scripts.lib.impl_tasks import TaskStatus + assert hasattr(TaskStatus, "PENDING") + + def test_impl_tasks_no_local_task_status_class(self): + """impl_tasks module should NOT define TaskStatus class body (only imports it).""" + source_path = Path(__file__).parent.parent / "scripts" / "lib" / "impl_tasks.py" + source = source_path.read_text() + assert "class TaskStatus" not in source, ( + "impl_tasks.py still defines TaskStatus locally — " + "it should import from task_storage instead" + ) + + def test_no_circular_import(self): + """task_storage should NOT import from impl_tasks (no circular dependency).""" + source_path = Path(__file__).parent.parent / "scripts" / "lib" / "task_storage.py" + source = source_path.read_text() + for line in source.splitlines(): + stripped = line.strip() + if stripped.startswith("#"): + continue + assert "import" not in stripped or "impl_tasks" not in stripped, ( + f"task_storage.py imports from impl_tasks: {stripped}" + )