fix: update tests for ❓ question marker and resolve CodeRabbit findings

- Update test_status_icon_names_are_stable to assert against QUESTION_MARKER
  constant instead of hardcoded "?" so it works in both ASCII and emoji terminals
- Update test_ask_user_renders_question_and_options to expect ❓ prefix on
  per-question lines (matches the standardized question marker from #9d65748)
- Update test_set_todo_list_description snapshot to match the rewritten
  set_todo_list.md description (execution-gated, evidence-driven restructuring)
- Replace hardcoded allowlist in review.py clean --dry-run with _ALLOWED_NAMES
  constant from findings_store (DRY, single source of truth)
- Remove ignore_errors=True from shutil.rmtree in purge_unknown and
  purge_stale_projects; surface OSError via logging.warning so callers can
  observe deletion failures instead of silently ignoring them

Author: elkaix

packages/pythinker-review/src/pythinker_review/cli/review.py

@@ -75,7 +75,7 @@
     status_project,
     triage_project,
 )
-from pythinker_review.store.findings_store import FindingsStore
+from pythinker_review.store.findings_store import _ALLOWED_NAMES, FindingsStore
 from pythinker_review.store.gitignore import ensure_gitignored
 from pythinker_review.store.models import SEVERITY_ORDER, Finding, Pass, RunMeta
 
@@ -379,11 +379,7 @@ def clean(
         if not store.state_dir.exists():
             typer.echo("nothing to clean (.pythinker-review/ does not exist)")
             return
-        unknown = [
-            e.name
-            for e in store.state_dir.iterdir()
-            if e.name not in {"index.json", "runs", "security-scan"}
-        ]
+        unknown = [e.name for e in store.state_dir.iterdir() if e.name not in _ALLOWED_NAMES]
         if unknown:
             typer.echo("would remove: " + ", ".join(sorted(unknown)))
         else:

packages/pythinker-review/src/pythinker_review/security_scan/store.py

@@ -7,6 +7,7 @@
 from __future__ import annotations
 
 import json
+import logging
 import os
 import secrets
 import shutil
@@ -276,7 +277,11 @@ def purge_stale_projects(*, data_root: Path, keep_project_id: str) -> list[str]:
     for entry in data_root.iterdir():
         if not entry.is_dir() or entry.name == keep_project_id:
             continue
-        shutil.rmtree(entry, ignore_errors=True)
+        try:
+            shutil.rmtree(entry)
+        except OSError:
+            logging.getLogger(__name__).warning("Failed to remove stale project %s", entry.name)
+            continue
         removed.append(entry.name)
     return removed
 

packages/pythinker-review/src/pythinker_review/store/findings_store.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 import json
+import logging
 import os
 import shutil
 from pathlib import Path
@@ -104,9 +105,13 @@ def purge_unknown(self) -> list[str]:
         removed: list[str] = []
         for entry in self.state_dir.iterdir():
             if entry.name not in _ALLOWED_NAMES:
-                if entry.is_dir():
-                    shutil.rmtree(entry, ignore_errors=True)
-                else:
-                    entry.unlink(missing_ok=True)
+                try:
+                    if entry.is_dir():
+                        shutil.rmtree(entry)
+                    else:
+                        entry.unlink(missing_ok=True)
+                except OSError:
+                    logging.getLogger(__name__).warning("Failed to remove unknown entry %s", entry)
+                    continue
                 removed.append(entry.name)
         return removed

tests/tools/test_tool_descriptions.py

@@ -140,29 +140,29 @@ def test_set_todo_list_description(set_todo_list_tool: SetTodoList):
     """Test the description of SetTodoList tool."""
     assert set_todo_list_tool.base.description == snapshot(
         """\
-Manage your todo list for tracking task progress.
+Manage your todo list for tracking task progress during execution.
 
-Todo list is a simple yet powerful tool to help you get things done. You typically want to use this tool when the given task involves multiple subtasks/milestones, or, multiple tasks are given in a single request. This tool can help you to break down the task and track the progress.
+**When to set todos (Update mode):**
+Set the todo list **only after the user has explicitly agreed on the plan**. The todo list marks the start of execution — it is not a planning scratch-pad. Do not call this tool while exploring, gathering context, presenting options, or waiting for user feedback. The moment the user says "yes", "do it", "go ahead", or otherwise confirms the approach, set the list and begin.
 
 **Usage modes:**
 
 - **Update mode**: Pass `todos` to set the entire todo list. The previous list is replaced.
 - **Query mode**: Omit `todos` (or pass null) to retrieve the current todo list without changes.
-- **Clear mode**: Pass an empty array `[]` to clear all todos.
+- **Clear mode**: Pass an empty array `[]` to clear all todos when work is fully done.
 
-This is the only todo list tool available to you. That said, each time you want to update the todo list, you need to provide the whole list. Make sure to maintain the todo items and their statuses properly.
+Once the todo list is set, it is the single source of truth for in-progress work. During execution, update item statuses as you complete work (`pending` → `in_progress` → `done`). Only restructure or replace the list when evidence genuinely changes the scope — not for convenience replanning. When in doubt, surface the new evidence to the user before changing the plan.
 
-Once you finished a subtask/milestone, remember to update the todo list to reflect the progress. Also, you can give yourself a self-encouragement to keep you motivated.
+Once you finish a subtask/milestone, update its status before moving to the next item.
 
-Abusing this tool to track too small steps will just waste your time and make your context messy. For example, here are some cases you should not use this tool:
+**Do NOT use this tool:**
 
-- When the user just simply ask you a question. E.g. "What language and framework is used in the project?", "What is the best practice for x?"
-- When it only takes a few steps/tool calls to complete the task. E.g. "Fix the unit test function 'test_xxx'", "Refactor the function 'xxx' to make it more solid."
-- When the user prompt is very specific and the only thing you need to do is brainlessly following the instructions. E.g. "Replace xxx to yyy in the file zzz", "Create a file xxx with content yyy."
+- During the planning or exploration phase, before the user has confirmed the approach.
+- When the user asks a question or requests a review without agreeing to a concrete plan.
+- When the task only takes a few steps/tool calls. E.g. "Fix the unit test function 'test_xxx'".
+- When the user prompt is very specific and fully self-contained. E.g. "Replace xxx to yyy in file zzz".
 
-However, do not get stuck in a rut. Be flexible. Sometimes, you may try to use todo list at first, then realize the task is too simple and you can simply stop using it; or, sometimes, you may realize the task is complex after a few steps and then you can start using todo list to break it down.
-
-IMPORTANT: Do not call this tool repeatedly without making real progress on at least one task between calls. If you are unsure about the current state, use Query mode (omit `todos`) to check before updating. If you find yourself unable to advance any task with your available tools, inform the user about what is blocking you instead of replanning. Repeatedly updating the todo list without doing actual work is counterproductive.
+**IMPORTANT:** Do not call this tool repeatedly without making real progress between calls. Use Query mode to check current state before updating. If you cannot advance any task, surface the blocker to the user instead of replanning. Repeated todo updates without real work are counterproductive.
 """
     )
 

tests/ui_and_conv/test_shell_design_system.py

@@ -14,6 +14,7 @@
     shell_style,
     status_icon,
 )
+from pythinker_code.ui.shell.glyphs import QUESTION_MARKER
 
 
 def _plain(renderable, *, width: int = 80) -> str:
@@ -37,8 +38,8 @@ def test_status_icon_names_are_stable():
     assert status_icon("denied").plain == "×"
     assert status_icon("interrupted").plain == "■"
     assert status_icon("waiting").plain == "○"
-    assert status_icon("question").plain == "?"
-    assert status_icon("approval").plain == "?"
+    assert status_icon("question").plain == QUESTION_MARKER
+    assert status_icon("approval").plain == QUESTION_MARKER
 
 
 def test_running_and_failed_status_icons_use_expected_tones():

tests/ui_and_conv/test_tui_card_tool_renderers.py

@@ -18,6 +18,7 @@
     render_diff,
     render_plain,
 )
+from pythinker_code.ui.shell.glyphs import QUESTION_MARKER
 from pythinker_code.ui.shell.tool_renderers import (
     ToolResultPayload,
     clear_tool_renderers,
@@ -881,7 +882,7 @@ def test_ask_user_renders_question_and_options():
         },
     )
     assert "● Ask 1 question" in rendered
-    assert "● Ask 1 question\n\n● Which auth method?" in rendered
+    assert f"● Ask 1 question\n\n{QUESTION_MARKER} Which auth method?" in rendered
     assert "OAuth" in rendered
     assert "API key" in rendered