From 9d6574866fe6243fc643b3c192b5c6af19913fee Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Wed, 3 Jun 2026 12:28:22 -0400 Subject: [PATCH 1/9] =?UTF-8?q?fix:=20standardize=20=E2=9D=93=20question?= =?UTF-8?q?=20marker=20and=20isolate=20scratchpad=20to=20current=20session?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Question marker (❓):** - Add QUESTION_MARKER constant to glyphs.py as the single source of truth (❓ in unicode, ? in ASCII mode) - Replace hardcoded "●" in ask_user.py inline renderer and "?" in _question_panel.py (3 occurrences) and design_system.py with QUESTION_MARKER - All question indicators now use the same glyph across transcript, dialog body, pager, and prompt_other_input **Scratchpad session isolation (Option B):** - DEFAULT_SCRATCHPAD_SECTION: remove cross-session skim instruction; agents now read only their own session's file, never other sessions' files - _SCRATCHPAD_RECOVERY_NOTE: same — read own session only, ignore others - Add cleanup_session_scratch() that deletes only the current session's scratch file (not all files like the full cleanup_scratch does) - _post_run in CLI now calls cleanup_session_scratch on ALL exits (success and interruption), not just success — prevents 87-file accumulation **SetTodoList plan-approval gate:** - set_todo_list.md: new rule — set todos only after user explicitly agrees on the plan, not during planning/exploration; list is SSOT during execution - system.md: SetTodoList guidance updated to match — "marks start of execution, not planning; status-updates only during execution" Root cause of post-interrupt confusion: _SCRATCHPAD_RECOVERY_NOTE instructed agents to fast-skim ALL prior sessions' files on startup, and cleanup only ran on successful exit, so interrupted sessions accumulated indefinitely. --- src/pythinker_code/agents/default/system.md | 2 +- src/pythinker_code/cli/__init__.py | 12 +++- src/pythinker_code/scratchpad.py | 62 ++++++++++++------- .../tools/todo/set_todo_list.md | 24 +++---- src/pythinker_code/ui/shell/design_system.py | 6 +- src/pythinker_code/ui/shell/glyphs.py | 6 ++ .../ui/shell/tool_renderers/ask_user.py | 3 +- .../ui/shell/visualize/_question_panel.py | 12 ++-- 8 files changed, 81 insertions(+), 46 deletions(-) diff --git a/src/pythinker_code/agents/default/system.md b/src/pythinker_code/agents/default/system.md index 186a75fd..ec2792e0 100644 --- a/src/pythinker_code/agents/default/system.md +++ b/src/pythinker_code/agents/default/system.md @@ -113,7 +113,7 @@ You have the capability to output any number of tool calls in a single response. For any non-trivial request, decompose before acting: - Preview the terrain first: scan the directory structure, file headers, and relevant module boundaries before choosing an implementation path. -- Use `SetTodoList` for multi-step work so the user can see the active plan and progress. +- **`SetTodoList` marks the start of execution, not planning.** Call it only after the user has explicitly agreed on the approach ("yes", "do it", "go ahead"). Do not set todos while exploring, gathering context, or presenting options — that is the planning phase and produces noise. Once set, the todo list is the single source of truth: update item statuses only (`pending → in_progress → done`), do not restructure the list mid-execution unless the user asks to replan. - **Granular todos, not umbrella todos.** Each todo must name a single concrete deliverable a human can recognize as "this part is done." Avoid umbrella titles like "Determine X" or "Investigate Y" that cover hours of parallel work — they freeze the progress UI while real work happens underneath. If a single todo would stay `in_progress` for more than ~3 minutes, it is too coarse: split it before launching work. - **One todo per dispatched child.** When you launch `RunAgents` with N children, the visible todo list MUST contain one in_progress sub-todo per child (or per independent objective the batch covers) **before** the batch starts. Update each sub-todo to `done` as that child returns — do not wait for the whole batch to finish to flip a single umbrella todo. Same rule applies to multiple parallel `Agent` calls in the same turn. - Split broad work into independent chunks; use parallel tool calls or focused subagents for chunks that do not depend on each other. diff --git a/src/pythinker_code/cli/__init__.py b/src/pythinker_code/cli/__init__.py index 6d28d348..ad99efa1 100644 --- a/src/pythinker_code/cli/__init__.py +++ b/src/pythinker_code/cli/__init__.py @@ -983,10 +983,16 @@ def _print_resume_hint(session: Session) -> None: async def _post_run( last_session: Session, exit_code: int, *, cleanup_scratchpad: bool = False ) -> None: - # Session scratchpads are retained as compact history for future recall. - # ``cleanup_scratchpad`` is kept for call-site compatibility but no longer - # triggers automatic deletion after a successful run. + # Always clean up this session's scratch file on exit (success or interruption) + # so files never accumulate. Todo list and context persist separately. _ = cleanup_scratchpad + from pythinker_code.scratchpad import cleanup_session_scratch + + await cleanup_session_scratch( + last_session.work_dir, + session_id=last_session.id, + session_title=last_session.title, + ) if exit_code == ExitCode.SUCCESS and getattr( getattr(config, "memory", None), "journal_recaps", False ): diff --git a/src/pythinker_code/scratchpad.py b/src/pythinker_code/scratchpad.py index 3227f147..f81f7ac6 100644 --- a/src/pythinker_code/scratchpad.py +++ b/src/pythinker_code/scratchpad.py @@ -1018,6 +1018,28 @@ async def append_scratch_event( ) +async def cleanup_session_scratch( + work_dir: HostPath, + *, + session_id: str, + session_title: str | None = None, +) -> None: + """Delete only the current session's scratch file on exit. Never raises. + + Called on every session exit (success and interruption) so files never + accumulate across sessions. The todo list and conversation context persist + separately in session state and are unaffected. + """ + if not _is_local_host(): + return + try: + path = session_scratch_path(work_dir, session_id=session_id, session_title=session_title) + if path.is_file() and not path.is_symlink(): + path.unlink(missing_ok=True) + except Exception: + logger.debug("scratchpad session cleanup failed") + + async def cleanup_scratch( work_dir: HostPath, *, git_runner: GitRunner | None = None ) -> ScratchpadCleanupResult: @@ -1098,23 +1120,22 @@ async def _unlink(path: Path) -> None: SCRATCHPAD_SECTION_END = "" DEFAULT_SCRATCHPAD_SECTION = ( - "As the root agent, treat named `.pythinker/scratch/*.md` files as the " - "minimal session memory for context-aware work. The runtime auto-creates a per-session block " - "with stable recall labels (for example `session:`, `workspace:`, " - "`ui:`, `source:`) and compact milestones such as " - "session start, todo summaries, agent/task starts, and task terminal status. " + "As the root agent, use your session's `.pythinker/scratch/-*.md` " + "file as private working notes for the **current session only**. " + "The runtime auto-creates it with stable recall labels (for example `session:`, " + "`workspace:`, `ui:`, `source:`). " "Record durable working notes with the `Scratchpad` tool — classify each with " - "`kind` (decision / evidence / blocker / next / note) — instead of editing these " - "files by hand. Keep each note short and organized: current objective, searchable " - "labels, load-bearing evidence, decisions, blockers, and next verification " - "checkpoint. On a fresh run, or whenever the user asks about prior session " - "work/history/context, fast-skim the relevant `.pythinker/scratch/*.md` " - "labels and current session block before answering. Do not paste full logs, " - "raw prompts, command output, secrets, or duplicate the whole `SetTodoList` " - "checklist into the file. Retain session scratchpads after successful " - "completion as compact history for future recall; remove them only when the " - "user explicitly asks for cleanup. Subagents do not create their own scratch " - "files." + "`kind` (decision / evidence / blocker / next / note) — instead of editing files by hand. " + "Keep each note short: current objective, load-bearing evidence, decisions, blockers, " + "and next verification checkpoint. " + "Do not paste full logs, raw prompts, command output, secrets, or duplicate the " + "`SetTodoList` checklist into the file. " + "Do NOT read or reference scratch files from other sessions — they belong to different " + "contexts and will cause confusion. " + "Session files are automatically cleaned up when the session ends. " + "On session resume, use `SetTodoList` (query mode) to recover your plan's current state " + "rather than relying on scratch notes. " + "Subagents do not create their own scratch files." ) _SECTION_UNAVAILABLE = ( @@ -1123,11 +1144,10 @@ async def _unlink(path: Path) -> None: ) _SCRATCHPAD_RECOVERY_NOTE = ( - "Startup recovery: prior scratchpad history exists under `.pythinker/scratch/` " - "or legacy `.pythinker/scratch.md`. If you are the root agent, fast-skim labels " - "and the current/relevant session block before planning or answering so you " - "can recover context; keep the history unless the user explicitly asks for " - "cleanup." + "Recovery: a scratch file exists for your session. Read only your own session's " + "file (the one whose name starts with your session short ID) to recover working " + "notes. Ignore any files belonging to other sessions — they are stale and will " + "be auto-cleaned." ) diff --git a/src/pythinker_code/tools/todo/set_todo_list.md b/src/pythinker_code/tools/todo/set_todo_list.md index 441dd8d3..8e7dbf48 100644 --- a/src/pythinker_code/tools/todo/set_todo_list.md +++ b/src/pythinker_code/tools/todo/set_todo_list.md @@ -1,23 +1,23 @@ -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, only update the **status** of existing items (`pending` → `in_progress` → `done`). Do not restructure or replace the list mid-execution unless the user explicitly asks to replan. -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. diff --git a/src/pythinker_code/ui/shell/design_system.py b/src/pythinker_code/ui/shell/design_system.py index 9e3f187b..f54c51cc 100644 --- a/src/pythinker_code/ui/shell/design_system.py +++ b/src/pythinker_code/ui/shell/design_system.py @@ -10,7 +10,7 @@ from rich.text import Text from pythinker_code.ui.shell.components.render_utils import cell_width, truncate_to_width -from pythinker_code.ui.shell.glyphs import TRANSCRIPT_ACTIVE_MARKER +from pythinker_code.ui.shell.glyphs import QUESTION_MARKER, TRANSCRIPT_ACTIVE_MARKER from pythinker_code.ui.theme import tui_rich_style @@ -53,8 +53,8 @@ class ShellTone(StrEnum): "denied": ("×", ShellTone.WARNING), "interrupted": ("■", ShellTone.MUTED), "waiting": ("○", ShellTone.MUTED), - "question": ("?", ShellTone.WARNING), - "approval": ("?", ShellTone.ACCENT), + "question": (QUESTION_MARKER, ShellTone.WARNING), + "approval": (QUESTION_MARKER, ShellTone.ACCENT), } diff --git a/src/pythinker_code/ui/shell/glyphs.py b/src/pythinker_code/ui/shell/glyphs.py index 781e1697..1ba89db8 100644 --- a/src/pythinker_code/ui/shell/glyphs.py +++ b/src/pythinker_code/ui/shell/glyphs.py @@ -55,6 +55,11 @@ #: List/detail bullet (U+2022) used in status panels; falls back to an asterisk #: under ASCII mode so legacy code pages and ``TERM=dumb`` stay clean. LIST_BULLET: Final = "*" if _ASCII_GLYPHS else "•" +#: Canonical glyph shown before a question prompt (AskUserQuestion tool). +#: Unambiguously signals "this is a question" in every context: transcript +#: inline view, interactive dialog body, pager, and ``prompt_other_input``. +#: ASCII mode falls back to plain ``?`` for legacy terminals. +QUESTION_MARKER: Final = "?" if _ASCII_GLYPHS else "❓" __all__ = [ "SPINNER_FRAMES", @@ -72,4 +77,5 @@ "TRANSCRIPT_ACTIVE_MARKER", "TRANSCRIPT_TOOL_GUTTER", "LIST_BULLET", + "QUESTION_MARKER", ] diff --git a/src/pythinker_code/ui/shell/tool_renderers/ask_user.py b/src/pythinker_code/ui/shell/tool_renderers/ask_user.py index 54ce11c0..05a25214 100644 --- a/src/pythinker_code/ui/shell/tool_renderers/ask_user.py +++ b/src/pythinker_code/ui/shell/tool_renderers/ask_user.py @@ -7,6 +7,7 @@ from rich.console import Group, RenderableType from rich.text import Text +from pythinker_code.ui.shell.glyphs import QUESTION_MARKER from pythinker_code.ui.shell.spacing import blank_row from pythinker_code.ui.shell.tool_renderers import ( ToolRenderContext, @@ -82,7 +83,7 @@ def _render_call(ctx: ToolRenderContext) -> RenderableType: children.append(blank_row()) question_text = as_str(q.get("question")) or "" if question_text: - children.append(fg("accent", f"● {question_text}")) + children.append(fg("accent", f"{QUESTION_MARKER} {question_text}")) opts = q.get("options") if isinstance(opts, list): opts_list = cast("list[Any]", opts) diff --git a/src/pythinker_code/ui/shell/visualize/_question_panel.py b/src/pythinker_code/ui/shell/visualize/_question_panel.py index 3621c3a8..a3e7f4d2 100644 --- a/src/pythinker_code/ui/shell/visualize/_question_panel.py +++ b/src/pythinker_code/ui/shell/visualize/_question_panel.py @@ -15,6 +15,7 @@ from pythinker_code.ui.shell.components.markdown import PythinkerMarkdown as Markdown from pythinker_code.ui.shell.components.render_utils import sanitize_ansi from pythinker_code.ui.shell.console import console, render_to_ansi +from pythinker_code.ui.shell.glyphs import QUESTION_MARKER from pythinker_code.ui.shell.keyboard import KeyEvent from pythinker_code.ui.shell.keymap import key_text from pythinker_code.ui.shell.spacing import blank_row @@ -139,7 +140,8 @@ def render(self, *, other_input_text: str | None = None) -> RenderableType: lines.append(Text.from_markup(" ".join(tab_parts))) lines.append(blank_row()) - lines.append(Text.from_markup(f"[{_tok.warning}]? {_safe_markup_text(q.question)}[/]")) + q_markup = f"[{_tok.warning}]{QUESTION_MARKER} {_safe_markup_text(q.question)}[/]" + lines.append(Text.from_markup(q_markup)) if q.multi_select: lines.append(Text(" (SPACE to toggle, ENTER to submit)", style="dim italic")) lines.append(blank_row()) @@ -304,9 +306,8 @@ def render_full_body(self) -> list[RenderableType]: def show_question_body_in_pager(panel: QuestionRequestPanel) -> None: _warn = get_tui_tokens().warning with console.screen(), console.pager(styles=True): - console.print( - Text.from_markup(f"[{_warn}]? {_safe_markup_text(panel.current_question_text)}[/]") - ) + q_markup = f"[{_warn}]{QUESTION_MARKER} {_safe_markup_text(panel.current_question_text)}[/]" + console.print(Text.from_markup(q_markup)) console.print() for renderable in panel.render_full_body(): console.print(renderable) @@ -314,7 +315,8 @@ def show_question_body_in_pager(panel: QuestionRequestPanel) -> None: async def prompt_other_input(question_text: str) -> str: _warn = get_tui_tokens().warning - console.print(Text.from_markup(f"\n[{_warn}]? {_safe_markup_text(question_text)}[/]")) + q_markup = f"\n[{_warn}]{QUESTION_MARKER} {_safe_markup_text(question_text)}[/]" + console.print(Text.from_markup(q_markup)) console.print(Text(" Enter your answer:", style="dim")) try: session: PromptSession[str] = PromptSession() From 1c47cfd4274966cbfd425b82ebc1434ea6e9ff05 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Wed, 3 Jun 2026 12:28:56 -0400 Subject: [PATCH 2/9] chore: add CHANGELOG entries for question marker and scratchpad fixes --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 973a0a80..b4f03a9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,10 @@ GitHub Releases page; `0.8.0` is the new starting line. ## Unreleased +- **❓ question marker standardized across all question surfaces.** A new `QUESTION_MARKER` constant in `glyphs.py` replaces the inconsistent mix of `●` (inline transcript) and `?` (interactive panel, pager, prompt) with a single `❓` glyph (ASCII fallback: `?`) used everywhere. +- **Scratchpad isolated to current session; cleans up on interruption.** Agents no longer fast-skim all prior sessions' scratch files on startup, eliminating the post-interrupt confusion where stale planning from previous sessions was injected into a new session's context. Scratch files are now deleted on every session exit — success or interruption — so files no longer accumulate. +- **`SetTodoList` gated behind explicit plan approval.** The tool description and agent system prompt now require that todos are set only after the user agrees on the plan. During planning and exploration the tool must not be called; once set, the list is the single source of truth for execution with status-only updates. + ## 0.32.0 (2026-06-03) ### What changed in this release From c7de3213981d97f58596bee26b186e017b0a08a4 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Wed, 3 Jun 2026 12:46:32 -0400 Subject: [PATCH 3/9] fix: hook path resolution and .pythinker-review folder cleanup guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix .claude/hooks/check-changelog.sh failing with "No such file or directory" when the Bash tool runs from a subdirectory; resolve the git repo root via `git rev-parse --show-toplevel` before invoking the script so the path is always absolute. - Add `FindingsStore.purge_unknown()` and `_ALLOWED_NAMES` constant to findings_store.py; only index.json, runs/, and security-scan/ are expected inside .pythinker-review/ — anything else (e.g. a stray report file) is removed by the new `pythinker-review clean` command. - Add `purge_stale_projects()` to security_scan/store.py; old project IDs from one-off or renamed audit runs (like `deep-sec-audit`) now accumulate 400+ files with no cleanup. `pythinker-security-scan init` calls this automatically and prints what was pruned. - Delete three stale .pythinker/reports/ files left over from prior investigation sessions. --- .claude/settings.json | 2 +- .pythinker/reports/arch-review-2026-05-30.md | 319 ------------- .pythinker/reports/deep-code-scan-findings.md | 426 ------------------ ...idation-tui-renderer-contract-hardening.md | 136 ------ .../src/pythinker_review/cli/review.py | 31 ++ .../src/pythinker_review/cli/security_scan.py | 8 + .../pythinker_review/security_scan/store.py | 19 + .../pythinker_review/store/findings_store.py | 19 + src/pythinker_code/agents/default/system.md | 2 +- src/pythinker_code/cli/__init__.py | 11 +- .../tools/todo/set_todo_list.md | 2 +- tests/core/test_default_agent.py | 9 +- tests/core/test_scratchpad.py | 10 +- 13 files changed, 92 insertions(+), 902 deletions(-) delete mode 100644 .pythinker/reports/arch-review-2026-05-30.md delete mode 100644 .pythinker/reports/deep-code-scan-findings.md delete mode 100644 .pythinker/reports/validation-tui-renderer-contract-hardening.md diff --git a/.claude/settings.json b/.claude/settings.json index fc5b99df..6c5452d3 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -6,7 +6,7 @@ "hooks": [ { "type": "command", - "command": "bash .claude/hooks/check-changelog.sh", + "command": "bash -c 'root=$(git rev-parse --show-toplevel 2>/dev/null || echo .); bash \"$root/.claude/hooks/check-changelog.sh\"'", "timeout": 15, "statusMessage": "Checking CHANGELOG gate..." } diff --git a/.pythinker/reports/arch-review-2026-05-30.md b/.pythinker/reports/arch-review-2026-05-30.md deleted file mode 100644 index e9e4cb07..00000000 --- a/.pythinker/reports/arch-review-2026-05-30.md +++ /dev/null @@ -1,319 +0,0 @@ -# Validated Architectural Review Report - -**Review scope:** `src/pythinker_code/background/`, `src/pythinker_code/memory/`, and `src/pythinker_code/auth/opencode_go.py` -**Validation date:** 2026-05-30 -**Validated against:** current working tree at `d3bf815627fdcfcf87f1f8abbabbb2d6749419a0` -**Status:** findings re-validated against the live tree (tests re-run, usages re-grepped). Each finding now carries a concrete, behavior-preserving fix. - ---- - -## Summary - -The original draft mixed valid cleanup opportunities with stale or incorrect findings. This version keeps only findings that still match the current codebase, and pairs each with the most robust fix that preserves existing behavior. - -**Current actionable findings:** - -1. `memory/retriever.py` still has an unused abstraction seam around `Retriever` / `LexicalRetriever` / `SqliteFts5Retriever`. -2. `background/manager.py` still duplicates locked task-runtime status mutation logic across six `_mark_task_*` methods. -3. `background/manager.py` still reaches into private store locking/write helpers where the public `update_runtime()` API can cover most cases. -4. `memory/recall.py` still passes a `store_path` argument that is immediately discarded. -5. `memory/consolidation.py` still calls the private `ProjectMemoryStore._ensure_dir()` method. - -**Optional cleanups:** - -- `Counter(doc)` can replace the manual term-frequency loop in `memory/retriever.py`. -- OpenCode Go response parsing can be clarified, but any Pydantic refactor must preserve current tolerant parsing behavior. - -**Corroborating evidence gathered during re-validation:** - -- The retriever seam (Finding 1) has **zero production references** — `SqliteFts5Retriever` and `sqlite_fts5_available` appear only in `retriever_sqlite.py` itself and one test; the only retriever used in the app path is `LexicalRetriever`. -- The migration target in Finding 3, `BackgroundTaskStore.update_runtime()`, is **already the established pattern** in this module (`manager.py:277`, `manager.py:659`, `worker.py:276`). The six `_mark_task_*` methods and the stale-recovery block are the remaining holdouts. -- `update_runtime_under_lock()` does **not** exist anywhere in the tree — no new lock-exposing API is needed. - -The stale and incorrect draft entries have been removed; the sections below list only current findings, robust fixes, and optional cleanups that still match the codebase. - ---- - -## Recommended fix order - -The findings interact. Applying them in this order avoids rework and keeps each diff behavior-preserving: - -1. **Finding 4** — drop the dead `store_path` parameter. This removes the `_ensure_dir()` call at `recall.py:252`. -2. **Finding 5** — add the public `ensure_root()` alias. After step 1, `consolidation.py:36` is the only external `_ensure_dir()` caller, so this isolates the change. -3. **Finding 2** — extract the status-transition helper. Route it through `update_runtime()`. -4. **Finding 3** — step 3 already eliminates the private `_runtime_lock` / `_write_runtime_unlocked` usage in the six mark-methods (`manager.py:821-912`). Only the stale-recovery block (`manager.py:598-626`) remains; decide its treatment per Finding 3 below. -5. **Finding 1** — delete the unused retriever seam. -6. **Optional A / B** — apply if desired; B is "leave as-is" by default. - ---- - -## Validated Findings - -### 1. Unused retriever abstraction and SQLite seam - -**Files:** - -- `src/pythinker_code/memory/retriever.py:7,49-54` -- `src/pythinker_code/memory/retriever_sqlite.py` (entire file) - -**Severity:** Low -**Category:** Overengineering / YAGNI - -`Retriever` is an abstract base class with no production polymorphic dispatch. `LexicalRetriever` is the real implementation used by recall (`recall.py:13,94`), while `SqliteFts5Retriever` is a capability seam that delegates directly to `LexicalRetriever`. - -Re-validation tightened this: `SqliteFts5Retriever` and `sqlite_fts5_available` have **no production callers at all** — they are referenced only inside `retriever_sqlite.py` and a single test (`tests/core/test_memory_phase_bcd.py:18,158`, `test_sqlite_retriever_falls_back_to_lexical`). There is no `memory/__init__.py` re-exporting these symbols, so nothing outside the package depends on them. - -**Robust fix:** - -Delete the dead seam outright — it is safe because there is no public export and no production dispatch: - -- Delete `src/pythinker_code/memory/retriever_sqlite.py`. -- In `retriever.py`, remove the `Retriever` ABC (lines 49-51) and make `LexicalRetriever` a plain class (`class LexicalRetriever:`). Remove the now-orphaned `from abc import ABC, abstractmethod` import (line 7). -- Keep the name `LexicalRetriever` — it is imported at `recall.py:13` and used at `recall.py:94`; renaming is pure churn for no behavioral gain. -- Delete the test `test_sqlite_retriever_falls_back_to_lexical` and its import at `tests/core/test_memory_phase_bcd.py:18`. (The fallback it asserts no longer exists once the wrapper is gone.) - -Do **not** reintroduce an FTS5 abstraction until there is a measured indexed implementation and a real dispatch path. If a future plugin contract genuinely needs polymorphism, add the protocol back at that point with a concrete second implementation — not before. - ---- - -### 2. Duplicated background task status mutation methods - -**File:** `src/pythinker_code/background/manager.py:821-922` -**Severity:** Medium -**Category:** Duplication / maintainability - -There are six near-identical status mutation methods, each performing the same locked read-check-mutate-write sequence: - -- `_mark_task_running` -- `_mark_task_awaiting_approval` -- `_mark_task_completed` -- `_mark_task_failed` -- `_mark_task_timed_out` -- `_mark_task_killed` - -**Semantics that must be preserved exactly:** - -- terminal states must not be overwritten (early no-op); -- `updated_at` is set to now on every applied transition; -- `_mark_task_running()` sets `heartbeat_at = updated_at` and clears `failure_reason`; -- `completed` clears `failure_reason`; `completed`/`failed`/`timed_out`/`killed` set `finished_at = updated_at`; `running`/`awaiting_approval` do **not** set `finished_at`; -- `timed_out` sets both `interrupted` and `timed_out`; `killed` sets `interrupted`; -- telemetry fires **only** when the transition was actually applied (not on the terminal no-op), guarded by `started_at and finished_at`, with reason labels unchanged: `completed` → `success=True` (no reason); `failed` → `reason="error"`; `timed_out` → `reason="timeout"`; `killed` → `reason="killed"`; `running`/`awaiting_approval` → no telemetry. - -**Robust fix:** - -Extract one private helper that owns the locked, terminal-guarded write via the **public** `update_runtime()` API, and signals whether the transition was applied so telemetry stays in the callers: - -```python -def _transition_status( - self, - task_id: str, - *, - mutate: Callable[[TaskRuntime], None], -) -> TaskRuntime | None: - """Locked, terminal-guarded status write via the public store API. - - Stamps ``updated_at`` then applies ``mutate``. Returns the resulting - runtime when the transition was applied, or ``None`` when the task was - already terminal (no write performed, so callers skip telemetry). - """ - applied = False - - def _apply(runtime: TaskRuntime) -> bool: - nonlocal applied - if is_terminal_status(runtime.status): - return False - runtime.updated_at = time.time() - mutate(runtime) - applied = True - return True - - runtime = self._store.update_runtime(task_id, _apply) - return runtime if applied else None -``` - -Each mark-method becomes a thin mutator plus its own telemetry. Two representative cases: - -```python -def _mark_task_running(self, task_id: str) -> None: - def mutate(r: TaskRuntime) -> None: - r.status = "running" - r.heartbeat_at = r.updated_at - r.failure_reason = None - - self._transition_status(task_id, mutate=mutate) - -def _mark_task_completed(self, task_id: str) -> None: - def mutate(r: TaskRuntime) -> None: - r.status = "completed" - r.finished_at = r.updated_at - r.failure_reason = None - - runtime = self._transition_status(task_id, mutate=mutate) - if runtime and runtime.started_at and runtime.finished_at: - from pythinker_code.telemetry import track - - track( - "background_task_completed", - success=True, - duration_s=runtime.finished_at - runtime.started_at, - ) -``` - -`_mark_task_failed` / `_mark_task_timed_out` / `_mark_task_killed` follow the same shape, each setting its status, `finished_at`, flags, and `failure_reason` in `mutate`, then emitting telemetry with its own `reason` label guarded by `if runtime and runtime.started_at and runtime.finished_at`. `_mark_task_awaiting_approval` uses only `mutate` with no telemetry. - -This collapses six bodies to one shared critical section while keeping each method's distinct mutation and telemetry explicit. Ensure `Callable` is imported (`from collections.abc import Callable`) if it is not already. - ---- - -### 3. Manager still uses private store lock/write internals - -**Files:** - -- `src/pythinker_code/background/manager.py:598-626` (stale-task recovery) -- `src/pythinker_code/background/manager.py:821-912` (the six mark-methods) -- `src/pythinker_code/background/store.py:143-153` (`update_runtime`) - -**Severity:** Medium -**Category:** Architectural coupling - -`BackgroundTaskManager` calls private store internals directly — `self._store._runtime_lock(...)` and `self._store._write_runtime_unlocked(...)` — in both the mark-methods and the stale-recovery block. The store already exposes `update_runtime(task_id, update_fn)`, which performs the locked read-modify-write and returns the resulting runtime, and which the manager **already uses** at `manager.py:277` and `manager.py:659` (and the worker at `worker.py:276`). No `update_runtime_under_lock()` API is needed and none exists. - -**Robust fix:** - -- **Six mark-methods (`821-912`):** resolved for free by Finding 2 — routing `_transition_status` through `update_runtime()` removes every private `_runtime_lock` / `_write_runtime_unlocked` call in these methods. -- **Stale-recovery block (`598-626`):** this block reads *both* `read_runtime` and `read_control` under the same lock and branches on `fresh_control.kill_requested_at` (`manager.py:600,615`). `update_runtime`'s callback is only *passed* the runtime, but it is a closure over `self._store`, so it can call `self._store.read_control(view.spec.id)` itself — that read executes inside the same `_runtime_lock` and does not deadlock, because `read_runtime`/`read_control` are lock-free (`store.py` takes `_runtime_lock` only in `write_runtime`/`update_runtime`; the current block already calls `read_control` while holding the lock). So this path *can* migrate with **no new store API**. Two reasonable options: - 1. **Leave it as-is.** The block is a single, well-commented critical section (`manager.py:595-597` explains *why* the lock spans the read and the write). Holding `_runtime_lock` here is correct and the private access is localized. Most surgical, lowest-risk — the default recommendation. - 2. **Migrate via a closure that reads control in-callback.** Move the body into a `recover_stale(runtime) -> bool` closure passed to `update_runtime()`; the early-outs (terminal / not-yet-stale) become `return False`, replacing today's `continue`. The surrounding per-view loop is unchanged — each iteration calls `update_runtime(view.spec.id, recover_stale)`: - - ```python - def recover_stale(runtime: TaskRuntime) -> bool: - if is_terminal_status(runtime.status): - return False - progress = ( - runtime.heartbeat_at or runtime.started_at - or runtime.updated_at or view.spec.created_at - ) - if now - progress <= stale_after: - return False - control = self._store.read_control(view.spec.id) # inside the lock via the closure - heartbeat_missing = runtime.heartbeat_at is None - runtime.finished_at = now - runtime.updated_at = now - if control.kill_requested_at is not None: - runtime.status = "killed" - runtime.interrupted = True - runtime.failure_reason = control.kill_reason or "Killed during recovery" - else: - runtime.status = "lost" - runtime.failure_reason = ( - "Background worker never heartbeat after startup" - if heartbeat_missing - else "Background worker heartbeat expired" - ) - return True - - self._store.update_runtime(view.spec.id, recover_stale) - ``` - -Neither option needs a new lock-exposing or recovery-specific store method. Prefer (1) for minimal churn, or (2) if you want zero private-internal access from the manager. - ---- - -### 4. `build_recall_block()` accepts an unused `store_path` - -**File:** `src/pythinker_code/memory/recall.py:86-97,252-258` -**Severity:** Low -**Category:** Dead parameter / unnecessary private access - -`build_recall_block()` accepts `store_path`, then immediately discards it with `_ = store_path` (`recall.py:97`). The call site computes that value through `self._store._ensure_dir()` (`recall.py:252`) purely to feed the dead parameter. `build_recall_block` is called only at `recall.py:253` and three tests, so the signature is safe to change. - -**Robust fix:** - -- Remove the `store_path` parameter from `build_recall_block()` (`recall.py:92`) and delete the `_ = store_path` discard line (`recall.py:97`). -- At the call site, delete `store_root = await self._store._ensure_dir()` (`recall.py:252`) and the `store_path=str(store_root / "memory")` argument (`recall.py:258`). This also removes one private `_ensure_dir()` usage. -- Update the three call sites that pass the kwarg, dropping `store_path=...`: - - `tests/core/test_recall_provider.py:39` (`test_build_recall_block_includes_open_todos_and_facts`) - - `tests/core/test_recall_provider.py:52` (`test_build_recall_block_empty_when_nothing`) - - `tests/core/test_recall_provider.py:63` (`test_build_recall_block_open_todos_only_does_not_suggest_missing_files`) - -The recall output is unchanged — the parameter and its value were never used in the block. The only incidental difference is that recall no longer eagerly creates the store directory via `_ensure_dir()`; the recall read path (candidates are passed in already loaded) does not depend on that side effect. - ---- - -### 5. Memory consolidation calls a private store method - -**File:** `src/pythinker_code/memory/consolidation.py:35-37` -**Severity:** Low -**Category:** Architectural coupling - -`inbox_dir()` calls `ProjectMemoryStore._ensure_dir()` from outside `project_memory.py` and suppresses the private-usage warning (`consolidation.py:36`). After Finding 4 removes the `recall.py:252` usage, `consolidation.py:36` is the **only** external `_ensure_dir()` caller. - -**Robust fix:** - -- Add a public async alias on `ProjectMemoryStore` that delegates to the existing implementation: - - ```python - async def ensure_root(self) -> Path: - """Public entry point for ``_ensure_dir`` used by collaborators.""" - return await self._ensure_dir() - ``` - -- Update `consolidation.py:36` to `root = await store.ensure_root()` and drop the `# pyright: ignore[reportPrivateUsage]` suppression. - -Keep `_ensure_dir()` as the single source of truth; `ensure_root()` is only a public surface so collaborators don't reach into a private method. (Naming note: the background and notifications stores use a private `_ensure_root`; the public `ensure_root` here is intentional and reads cleanly as the published method.) - ---- - -## Optional Cleanups - -### A. Use `collections.Counter` for term frequency - -**File:** `src/pythinker_code/memory/retriever.py:76-78` -**Severity:** Low - -Replace the manual term-frequency loop: - -```python -tf: dict[str, int] = {} -for term in doc: - tf[term] = tf.get(term, 0) + 1 -``` - -with `tf = Counter(doc)` (`from collections import Counter`). This is a drop-in: the scoring loop guards every access with `if term not in tf: continue` before reading `tf[term]`, so `Counter`'s default-zero behavior changes nothing. Readability cleanup only — not a correctness issue. The adjacent document-frequency loop (`retriever.py:67-70`) could likewise use `df.update(set(doc))`, but leave it unless you are already touching that block. - ---- - -### B. OpenCode Go response parsing can be made clearer, but not stricter by accident - -**File:** `src/pythinker_code/auth/opencode_go.py:179-228` -**Severity:** Low/Medium - -The current implementation manually validates `/models` and `models.dev` payloads with `isinstance()` and casts. The behavior is intentionally tolerant: - -- malformed top-level payloads return an empty result (`_extract_model_ids` → `[]`, `_parse_models_dev_metadata` → `{}`); -- malformed list/dict entries are skipped (`continue`) while valid entries are kept; -- models.dev enrichment is best-effort and must not break login. - -**Robust fix: prefer leaving this as-is.** The current `isinstance`/`cast` code already encodes exactly the tolerance required, and a Pydantic rewrite risks silently regressing it for no functional gain. If clarity is the goal, add a short comment documenting the tolerance contract rather than rewriting the parser. - -If a Pydantic refactor is nonetheless mandated, it must preserve those semantics: validate entries **item-by-item** with `model_config = ConfigDict(extra="ignore")`, skipping individual `ValidationError`s, and never reject the whole response because one entry is malformed. Top-level shape mismatch must still yield empty results, and enrichment failure must never propagate into the login path. - ---- - -## Verification Performed - -Targeted validation command (re-run during this pass): - -```bash -uv run pytest \ - tests/background/test_manager.py::test_recover_agent_view_does_not_clobber_terminal_runtime_from_stale_view \ - tests/background/test_manager.py::test_mark_task_completed_is_lock_protected \ - tests/background/test_worker.py::test_worker_completes_successfully \ - tests/core/test_memory_phase_bcd.py::test_sqlite_retriever_falls_back_to_lexical \ - tests/core/test_recall_provider.py::test_build_recall_block_includes_open_todos_and_facts -``` - -Result: **5 passed** (re-confirmed 2026-05-30, `0.08s`). - -Note: after applying Finding 1 (delete the retriever seam) and Finding 4 (drop `store_path`), two of these tests change by design — `test_sqlite_retriever_falls_back_to_lexical` is deleted with the seam, and `test_build_recall_block_includes_open_todos_and_facts` drops its `store_path` kwarg. Re-run the full `tests/background` and `tests/core` suites after each fix to confirm no behavioral regressions. diff --git a/.pythinker/reports/deep-code-scan-findings.md b/.pythinker/reports/deep-code-scan-findings.md deleted file mode 100644 index 3308f05e..00000000 --- a/.pythinker/reports/deep-code-scan-findings.md +++ /dev/null @@ -1,426 +0,0 @@ -# Deep Code Scan: Architectural Criticism & Bug Analysis - -**Date:** 2026-05-30 -**Scope:** Recent changes across auth, background tasks, memory, soul loop, file tools, and UI components -**Analyzed by:** 4 parallel agents (architecture scout, bug hunter, simplicity reviewer, overengineering scanner) - ---- - -## Executive Summary - -Found **1 critical bug** (race condition in background task recovery), **3 high-severity architectural issues** (over-engineering, duplication, fragility), and **5 medium-severity elegance violations**. The codebase shows signs of defensive over-engineering and speculative abstractions that could be simplified. - ---- - -## CRITICAL: The Known Bug - -### Race Condition in Background Task Recovery - -**Location:** `src/pythinker_code/background/manager.py:628-656` in `_recover_agent_view()` - -**The Problem:** The method reads the task runtime **without holding the lock**, then later writes a "recoverable" status **with the lock**. Between the read and write, the task could complete normally, causing the authoritative "completed" status to be overwritten with "recoverable". - -**Root Cause:** -```python -def _recover_agent_view(self, view: TaskView, *, now: float, live_agent_ids: set[str]) -> None: - runtime_status: TaskStatus = view.runtime.status # ← READ WITHOUT LOCK - if not is_terminal_status(runtime_status): - if view.spec.id in self._live_agent_tasks: - return - runtime = view.runtime.model_copy() - runtime.status = "recoverable" if agent_id is not None else "lost" - self._store.write_runtime(view.spec.id, runtime) # ← WRITE WITH LOCK -``` - -The safety check in `_write_runtime_unlocked` only prevents overwriting a terminal status with a non-terminal one: -```python -if is_terminal_status(current.status) and not is_terminal_status(runtime.status): - return -``` - -But both "completed" and "recoverable" are terminal, so the check passes and the overwrite happens. - -**Impact:** -- Task appears "recoverable" when it actually completed successfully -- User may unnecessarily resume a task that already finished -- Subagent status reconciliation may incorrectly mark the instance as "idle" instead of respecting the completed state - -**Reproduction:** -1. Start a background agent task -2. Task completes and writes `status = "completed"` to disk -3. Before `reconcile()` runs, the manager's `_live_agent_tasks` dict is cleared (e.g., process restart) -4. `recover()` is called, sees the task as "running" (stale view), marks it "recoverable" -5. The authoritative "completed" status is lost - -**The Fix:** -```python -def _recover_agent_view(self, view: TaskView, *, now: float, live_agent_ids: set[str]) -> None: - agent_id_raw = (view.spec.kind_payload or {}).get("agent_id") - agent_id = agent_id_raw if isinstance(agent_id_raw, str) else None - - # Re-read under lock to get authoritative current state - with self._store._runtime_lock(view.spec.id): - runtime = self._store.read_runtime(view.spec.id) - if is_terminal_status(runtime.status): - # Already terminal, just reconcile subagent status - self._reconcile_subagent_status(agent_id, runtime.status, live_agent_ids) - return - - if view.spec.id in self._live_agent_tasks: - return - - # Mark as recoverable/lost - runtime.finished_at = now - runtime.updated_at = now - runtime.status = "recoverable" if agent_id is not None else "lost" - runtime.failure_reason = ( - "In-process background agent is no longer running; resume the stored agent " - f"instance {agent_id} to continue." - if agent_id is not None - else "In-process background agent is no longer running" - ) - self._store._write_runtime_unlocked(view.spec.id, runtime) - - self._reconcile_subagent_status(agent_id, runtime.status, live_agent_ids) -``` - ---- - -## HIGH: Overengineering & YAGNI Violations - -### 1. Speculative Abstract Retriever Class - -**Location:** `src/pythinker_code/memory/retriever.py:49-52` - -**The Problem:** The `Retriever` abstract base class defines a single `retrieve` method, but only one implementation (`LexicalRetriever`) exists. This is speculative infrastructure - an abstraction with no second implementation to justify it. - -**The Code:** -```python -class Retriever(ABC): - @abstractmethod - async def retrieve(self, query: RecallQuery, budget_tokens: int) -> list[RankedBlock]: ... - -class LexicalRetriever(Retriever): - """Hand-rolled BM25 + recency decay + label/path boost. Stdlib only.""" - # ... only implementation -``` - -**The Fix:** Remove the abstract class and make `LexicalRetriever` a concrete class. If a second retriever is needed later, introduce the abstraction then. - -```python -class LexicalRetriever: - """Hand-rolled BM25 + recency decay + label/path boost. Stdlib only.""" - - async def retrieve(self, query: RecallQuery, budget_tokens: int) -> list[RankedBlock]: - # ... implementation -``` - ---- - -### 2. Duplicated Background Task State Management - -**Location:** `src/pythinker_code/background/manager.py:817-918` - -**The Problem:** Four nearly identical methods (`_mark_task_running`, `_mark_task_completed`, `_mark_task_failed`, `_mark_task_timed_out`, `_mark_task_killed`) share the same lock→read→check→write→telemetry skeleton. This violates DRY and creates maintenance burden. - -**The Code:** -```python -def _mark_task_completed(self, task_id: str) -> None: - with self._store._runtime_lock(task_id): - runtime = self._store.read_runtime(task_id) - if is_terminal_status(runtime.status): - return - runtime.status = "completed" - runtime.updated_at = time.time() - runtime.finished_at = runtime.updated_at - runtime.failure_reason = None - self._store._write_runtime_unlocked(task_id, runtime) - # telemetry... - -def _mark_task_failed(self, task_id: str, reason: str) -> None: - with self._store._runtime_lock(task_id): - runtime = self._store.read_runtime(task_id) - if is_terminal_status(runtime.status): - return - runtime.status = "failed" - runtime.updated_at = time.time() - runtime.finished_at = runtime.updated_at - runtime.failure_reason = reason - self._store._write_runtime_unlocked(task_id, runtime) - # telemetry... - -# ... 3 more similar methods -``` - -**The Fix:** Extract a single generic method: - -```python -def _mark_task_terminal( - self, - task_id: str, - status: TaskStatus, - *, - reason: str | None = None, - interrupted: bool = False, - timed_out: bool = False, -) -> None: - with self._store._runtime_lock(task_id): - runtime = self._store.read_runtime(task_id) - if is_terminal_status(runtime.status): - return - runtime.status = status - runtime.updated_at = time.time() - runtime.finished_at = runtime.updated_at - runtime.failure_reason = reason - runtime.interrupted = interrupted - runtime.timed_out = timed_out - self._store._write_runtime_unlocked(task_id, runtime) - - # Telemetry - if runtime.started_at and runtime.finished_at: - from pythinker_code.telemetry import track - duration = runtime.finished_at - runtime.started_at - track( - "background_task_completed", - success=(status == "completed"), - duration_s=duration, - reason="timeout" if timed_out else ("killed" if interrupted else ("error" if status == "failed" else None)), - ) - -# Then the specific methods become one-liners: -def _mark_task_completed(self, task_id: str) -> None: - self._mark_task_terminal(task_id, "completed") - -def _mark_task_failed(self, task_id: str, reason: str) -> None: - self._mark_task_terminal(task_id, "failed", reason=reason) - -def _mark_task_timed_out(self, task_id: str, reason: str) -> None: - self._mark_task_terminal(task_id, "failed", reason=reason, interrupted=True, timed_out=True) - -def _mark_task_killed(self, task_id: str, reason: str) -> None: - self._mark_task_terminal(task_id, "killed", reason=reason, interrupted=True) -``` - ---- - -### 3. Fragile Markdown Table Repair Heuristics - -**Location:** `src/pythinker_code/ui/shell/components/markdown.py` (400+ lines across multiple functions) - -**The Problem:** Complex heuristic logic for repairing malformed markdown tables is brittle and requires ongoing maintenance as LLM output patterns change. The code attempts to fix common table formatting errors but lacks clear contracts for what constitutes "valid" vs "repairable" input. - -**Evidence:** Architecture scout identified this as a "fragility risk" - changes to LLM output patterns could require ongoing maintenance. - -**The Fix:** Two options: - -**Option A (Simpler):** Accept that LLM-generated tables may be malformed and render them as-is, letting the user see the raw output. Document the limitation. - -**Option B (More robust):** Define a strict contract for table validation and repair, with clear test cases for each heuristic. Move the repair logic to a separate, well-tested module with explicit input/output examples. - -Recommend **Option A** for simplicity - the repair heuristics are solving a problem that may not be worth the complexity. - ---- - -## MEDIUM: Elegance & Simplicity Violations - -### 1. Unreachable Null Guard in Retriever - -**Location:** `src/pythinker_code/memory/retriever.py:66` - -**The Problem:** The `if n` guard is unreachable because the function already returns early on empty candidates at line 62. - -**The Code:** -```python -async def retrieve(self, query: RecallQuery, budget_tokens: int) -> list[RankedBlock]: - if not self._candidates or budget_tokens <= 0: - return [] - docs = [_tokenize(c.content + " " + c.title) for c in self._candidates] - n = len(docs) - avgdl = sum(len(d) for d in docs) / n if n else 0.0 # ← `if n` is unreachable -``` - -**The Fix:** -```python -avgdl = sum(len(d) for d in docs) / n -``` - ---- - -### 2. Manual Term-Frequency Dictionary Building - -**Location:** `src/pythinker_code/memory/retriever.py:76-78` - -**The Problem:** Manual loop to build a term-frequency dict when `collections.Counter` does this in one line. - -**The Code:** -```python -tf: dict[str, int] = {} -for term in doc: - tf[term] = tf.get(term, 0) + 1 -``` - -**The Fix:** -```python -from collections import Counter -tf = Counter(doc) -``` - ---- - -### 3. Dead Bool Return in Worker Finish Callback - -**Location:** `src/pythinker_code/background/worker.py:250-274` - -**The Problem:** The `finish_runtime` callback always returns `True`, but the return value is used to decide whether to write the runtime. Since it always returns `True`, the return is dead code. - -**The Code:** -```python -def finish_runtime(runtime: TaskRuntime) -> bool: - runtime.finished_at = time.time() - # ... mutations ... - return True # ← always True, never False - -store.update_runtime(task_id, finish_runtime) -``` - -**The Fix:** Either: -- Remove the return and change `update_runtime` to always write -- Or make the return meaningful (e.g., return False if the runtime is already terminal) - ---- - -### 4. Trivial Wrapper Method - -**Location:** `src/pythinker_code/background/manager.py:148-155` - -**The Problem:** `active_task_count()` is a trivial one-line wrapper around `_active_task_count()`. One of them is unnecessary. - -**The Code:** -```python -def _active_task_count(self) -> int: - return sum( - 1 for view in self._store.list_views() if not is_terminal_status(view.runtime.status) - ) - -def active_task_count(self) -> int: - """Return the number of non-terminal background tasks.""" - return self._active_task_count() -``` - -**The Fix:** Merge into a single public method: -```python -def active_task_count(self) -> int: - """Return the number of non-terminal background tasks.""" - return sum( - 1 for view in self._store.list_views() if not is_terminal_status(view.runtime.status) - ) -``` - ---- - -### 5. Heavy isinstance Guard Chains on Controlled JSON - -**Location:** `src/pythinker_code/auth/opencode_go.py:179-193, 196-228` - -**The Problem:** Extensive `isinstance` checks on JSON API responses that are controlled by the API contract. The code is defensively checking every level of the JSON structure when it could use try/except or Pydantic validation. - -**The Code:** -```python -def _extract_model_ids(data: object) -> list[str]: - if not isinstance(data, dict): - return [] - raw_items = cast(dict[str, Any], data).get("data") - if not isinstance(raw_items, list): - return [] - ids: list[str] = [] - for item in cast(list[Any], raw_items): - if not isinstance(item, dict): - continue - model_id = cast(dict[str, Any], item).get("id") - if isinstance(model_id, str) and model_id: - ids.append(model_id) - return ids -``` - -**The Fix:** Use Pydantic models to validate the API response structure: -```python -from pydantic import BaseModel - -class ModelsResponse(BaseModel): - data: list[ModelItem] - -class ModelItem(BaseModel): - id: str - -def _extract_model_ids(data: object) -> list[str]: - try: - response = ModelsResponse.model_validate(data) - return [item.id for item in response.data] - except (ValidationError, TypeError): - return [] -``` - ---- - -## Architectural Coherence Issues - -### Cross-Process Locking Complexity - -**Location:** `src/pythinker_code/background/store.py:116-137` - -**The Problem:** The background task system uses cross-process file locking (`fcntl.flock`) to coordinate between the manager and worker processes. This is necessary but adds significant complexity, and race conditions in this area could be difficult to debug. - -**Assessment:** This is **acceptable complexity** given the requirement for cross-process coordination, but the locking logic should be better documented and tested. - -**Recommendation:** Add integration tests that specifically exercise the locking behavior under concurrent access. - ---- - -## Positive Findings - -The following areas were flagged as **well-designed** despite initial suspicion: - -1. **API error classification** (`opencode_go.py:137-174`) - Necessary complexity for telemetry, properly exposed for testing -2. **StrReplaceFile tool** (`tools/file/replace.py`) - Clean validation logic, correct batch edit handling -3. **Background task store** (`background/store.py`) - Proper use of atomic writes, good fallback handling - ---- - -## Recommendations Summary - -### Immediate (Critical) -1. **Fix the race condition** in `_recover_agent_view` by holding the lock during the entire read-check-write sequence - -### Short-term (High Value) -2. **Remove the abstract Retriever class** - YAGNI violation with no second implementation -3. **Consolidate the `_mark_task_*` methods** - Reduce duplication and maintenance burden -4. **Simplify markdown table handling** - Either document limitations or add proper contracts/tests - -### Medium-term (Elegance) -5. **Remove unreachable code** (null guard in retriever) -6. **Use `collections.Counter`** for term-frequency counting -7. **Remove dead return values** (finish_runtime callback) -8. **Merge trivial wrapper methods** (active_task_count) -9. **Use Pydantic for API validation** instead of manual isinstance chains - ---- - -## Files Analyzed - -- `src/pythinker_code/auth/opencode_go.py` -- `src/pythinker_code/background/manager.py` -- `src/pythinker_code/background/store.py` -- `src/pythinker_code/background/worker.py` -- `src/pythinker_code/memory/consolidation.py` -- `src/pythinker_code/memory/retriever.py` -- `src/pythinker_code/soul/pythinkersoul.py` -- `src/pythinker_code/soul/toolset.py` -- `src/pythinker_code/tools/file/replace.py` -- `src/pythinker_code/ui/shell/components/markdown.py` -- `src/pythinker_code/ui/shell/components/report.py` - ---- - -**Report generated by:** Pythinker Deep Code Scan -**Agents:** architecture-scout, bug-hunter, simplicity-reviewer, overengineering-scanner -**Cross-validated against:** Live code reads and manual inspection diff --git a/.pythinker/reports/validation-tui-renderer-contract-hardening.md b/.pythinker/reports/validation-tui-renderer-contract-hardening.md deleted file mode 100644 index d8851885..00000000 --- a/.pythinker/reports/validation-tui-renderer-contract-hardening.md +++ /dev/null @@ -1,136 +0,0 @@ -# Validation Report: TUI Renderer Contract Hardening Review - -## Verdict - -The pasted review is **partially validated**. It correctly identifies a real edge-case mismatch in `_CODE_SPAN_RE` and a weak test assertion, but it overstates severity and contains at least one unvalidated/likely incorrect GFM/table-escape claim. I would not treat the pasted report's "2 high-priority issues" as release blockers. - -## Scope and evidence - -Validated against branch `feat/tui-renderer-contract-hardening` using targeted reads and commands. - -Commands run: - -```bash -git rev-list --count main..HEAD -git diff --name-only main...HEAD | wc -l -git diff --shortstat main...HEAD -uv run pytest tests/ui_and_conv/test_md_table_contract.py tests/ui_and_conv/test_md_stream_idempotency.py -q -uv run pytest tests/ui_and_conv -q -uv run python - <<'PY' -from markdown_it import MarkdownIt -from pythinker_code.ui.shell.components.markdown import _escape_code_span_pipes - -md = MarkdownIt('commonmark').enable('table') -for label, row in [ - ('mismatched closing run', '| `a | b`` | rest |'), - ('double slash before pipe', '| `a \\\\| b` | rest |'), -]: - src = '| Expr | Meaning |\n| --- | --- |\n' + row + '\n' - cells = [] - for token in md.parse(src): - if token.type == 'inline': - cells.append((token.content, [(c.type, c.content) for c in (token.children or [])])) - print(label) - print('escaped-row:', repr(_escape_code_span_pipes(row))) - print('markdown-it inline cells:', cells) -PY -``` - -Results: - -- Branch metadata from `main...HEAD`: **16 commits**, **24 files changed**, `1966 insertions(+), 51 deletions(-)`. This does **not** match the pasted report's "15 commits / 16 files" claim. -- Targeted tests: `15 passed`. -- Full `tests/ui_and_conv`: `1305 passed`. - -## Finding validation - -### 1. `_CODE_SPAN_RE` mismatched backtick behavior - -Status: **Validated as an edge case, severity overstated.** - -Evidence: - -- `src/pythinker_code/ui/shell/components/markdown.py` defines `_CODE_SPAN_RE = re.compile(r"(?P`+)(?P.*?)(?P=ticks)")`. -- For `| `a | b`` | rest |`, `_escape_code_span_pipes` returns `| `a \| b`` | rest |`. -- `markdown-it-py` without the pre-escape parses the row cells as text fragments `('`a')` and `('b``')`, not a balanced code span. - -Interpretation: - -The observation is technically real: the regex can treat the first backtick of a longer closing run as the equal-length closer. However, this input is already malformed, and the normalizer is explicitly a repair/tolerance path for LLM-produced table rows. The product decision is whether to enforce strict GFM code-span boundaries or keep forgiving repair behavior. - -Recommended action: - -- Add a characterization test for mismatched backtick runs. -- Decide and document the intended policy: - - strict GFM: do not escape pipes in mismatched runs; or - - tolerant LLM repair: keep current behavior and test it as intentional. - -Suggested severity: **Low/Medium**, not High. - -### 2. Double-backslash before pipe claim - -Status: **Not validated; likely incorrect for the current parser contract.** - -Evidence: - -For row `| `a \\| b` | rest |`: - -- `_escape_code_span_pipes` returns it unchanged. -- `markdown-it-py` parses a valid table row with cells: - - `code_inline` content: `a \| b` - - second cell: `rest` -- The table structure remains intact. - -Interpretation: - -The pasted report assumes even/odd backslash semantics that do not match the observed `markdown-it-py` table behavior used by this code path. The current implementation's simple negative lookbehind preserves table structure for this case. The proposed regex change could alter literal backslash rendering and should not be applied without a concrete failing renderer test. - -Recommended action: - -- Do **not** treat this as a confirmed bug. -- If this edge matters, first add a renderer-level characterization test for the desired source-to-rendered output. - -Suggested severity: **None / advisory only**. - -### 3. `test_table_with_piped_inline_code_keeps_columns` assertion precision - -Status: **Validated, but low severity.** - -Evidence: - -The test in `tests/ui_and_conv/test_md_table_contract.py` checks only that `bitwise or`, `plain`, and `text` appear in rendered output. Those checks are useful but do not strongly prove table structure survived. - -Recommended action: - -Strengthen with a structural assertion that the header/data relationship survives, or with a lower-level normalized-markdown/token assertion. Keep it simple; avoid brittle visual-layout assertions. - -Suggested severity: **Low**. - -### 4. Parameterizing test strings - -Status: **Valid nit, not a defect.** - -This is maintainability advice only. Current explicit tests are readable and acceptable. - -Suggested severity: **Nit / optional**. - -### 5. Stream intermediate-state assertions - -Status: **Not validated as useful.** - -The existing stream test already validates exact reassembly and no duplicated rendered rows. The suggested `1 <= len(committed) <= 10` check is arbitrary and may become brittle if commit-boundary heuristics change without user-visible regression. - -Recommended action: - -Do not add the suggested slice-count assertion. If stronger coverage is needed, assert a named invariant tied to user-visible behavior, not an arbitrary count. - -## Recommended next actions - -1. Correct the review metadata: current branch evidence is 16 commits / 24 files from `main...HEAD`. -2. Add one characterization test for mismatched backtick runs in `_escape_code_span_pipes`. -3. Optionally strengthen `test_table_with_piped_inline_code_keeps_columns` with a non-brittle structural assertion. -4. Do not implement the pasted report's double-backslash regex recommendation unless a failing renderer-level test proves the desired behavior. - -## Notes - -The graphify knowledge graph may be stale because files changed in this session; validation above used targeted raw-file reads and deterministic commands rather than relying on the graph for modified areas. diff --git a/packages/pythinker-review/src/pythinker_review/cli/review.py b/packages/pythinker-review/src/pythinker_review/cli/review.py index ce6e905f..6f6d23e4 100644 --- a/packages/pythinker-review/src/pythinker_review/cli/review.py +++ b/packages/pythinker-review/src/pythinker_review/cli/review.py @@ -365,6 +365,37 @@ def diff( ) +@app.command(name="clean") +def clean( + repo: Path = typer.Option(Path.cwd(), "--repo", "--root"), + dry_run: bool = typer.Option(False, "--dry-run"), +) -> None: + """Remove unexpected files from .pythinker-review/ that are not part of its schema. + + Safe to run at any time. Use --dry-run to preview without deleting. + """ + store = FindingsStore(repo_root=repo.resolve()) + if dry_run: + 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"} + ] + if unknown: + typer.echo("would remove: " + ", ".join(sorted(unknown))) + else: + typer.echo("nothing to clean") + return + removed = store.purge_unknown() + if removed: + typer.secho(f"removed: {', '.join(sorted(removed))}", fg=typer.colors.YELLOW) + else: + typer.echo("nothing to clean") + + @app.command(name="init") def init_stateful( force: bool = typer.Option(False, "--force"), diff --git a/packages/pythinker-review/src/pythinker_review/cli/security_scan.py b/packages/pythinker-review/src/pythinker_review/cli/security_scan.py index 315fb66d..099aa5f3 100644 --- a/packages/pythinker-review/src/pythinker_review/cli/security_scan.py +++ b/packages/pythinker-review/src/pythinker_review/cli/security_scan.py @@ -34,6 +34,7 @@ from pythinker_review.security_scan.store import ( ensure_project, load_all_file_records, + purge_stale_projects, read_info, read_project_settings, write_info, @@ -90,6 +91,13 @@ def init( info_path = data_root / pid / "INFO.md" if force_info or not info_path.exists(): write_info(pid, _default_info(pid, detected.tags), data_root=data_root) + removed = purge_stale_projects(data_root=data_root, keep_project_id=pid) + if removed: + typer.secho( + f"purged stale project data: {', '.join(removed)}", + fg=typer.colors.YELLOW, + err=True, + ) typer.echo( json.dumps( {"projectId": pid, "dataRoot": str(data_root), "techTags": detected.tags}, diff --git a/packages/pythinker-review/src/pythinker_review/security_scan/store.py b/packages/pythinker-review/src/pythinker_review/security_scan/store.py index f35399e1..f4be080a 100644 --- a/packages/pythinker-review/src/pythinker_review/security_scan/store.py +++ b/packages/pythinker-review/src/pythinker_review/security_scan/store.py @@ -9,6 +9,7 @@ import json import os import secrets +import shutil import socket import subprocess from collections.abc import Iterable @@ -262,6 +263,24 @@ def iter_report_paths(project_id: str, *, data_root: Path) -> Iterable[Path]: return root.iterdir() +def purge_stale_projects(*, data_root: Path, keep_project_id: str) -> list[str]: + """Remove every project directory in data_root whose name != keep_project_id. + + Called automatically by the CLI init command so stale project IDs from renamed + or one-off audit runs do not accumulate in .pythinker-review/security-scan/data/. + Returns the list of project IDs that were deleted. + """ + if not data_root.exists(): + return [] + removed: 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) + removed.append(entry.name) + return removed + + def _dump(model: Any) -> dict[str, Any]: if isinstance(model, dict): return model diff --git a/packages/pythinker-review/src/pythinker_review/store/findings_store.py b/packages/pythinker-review/src/pythinker_review/store/findings_store.py index 9d95e8de..f2f9cf39 100644 --- a/packages/pythinker-review/src/pythinker_review/store/findings_store.py +++ b/packages/pythinker-review/src/pythinker_review/store/findings_store.py @@ -12,6 +12,7 @@ _STATE_DIR = ".pythinker-review" _INDEX_LIMIT = 200 +_ALLOWED_NAMES = frozenset({"index.json", "runs", "security-scan"}) class FindingsStore: @@ -91,3 +92,21 @@ def _update_index(self, meta: RunMeta) -> None: run_dir = self._run_dir(run_id) if run_dir.is_dir(): shutil.rmtree(run_dir, ignore_errors=True) + + def purge_unknown(self) -> list[str]: + """Remove entries in state_dir that are not part of the expected structure. + + Returns the names of whatever was deleted. Safe to call at any time; + does nothing if state_dir does not exist yet. + """ + if not self.state_dir.exists(): + return [] + 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) + removed.append(entry.name) + return removed diff --git a/src/pythinker_code/agents/default/system.md b/src/pythinker_code/agents/default/system.md index ec2792e0..08cc9637 100644 --- a/src/pythinker_code/agents/default/system.md +++ b/src/pythinker_code/agents/default/system.md @@ -113,7 +113,7 @@ You have the capability to output any number of tool calls in a single response. For any non-trivial request, decompose before acting: - Preview the terrain first: scan the directory structure, file headers, and relevant module boundaries before choosing an implementation path. -- **`SetTodoList` marks the start of execution, not planning.** Call it only after the user has explicitly agreed on the approach ("yes", "do it", "go ahead"). Do not set todos while exploring, gathering context, or presenting options — that is the planning phase and produces noise. Once set, the todo list is the single source of truth: update item statuses only (`pending → in_progress → done`), do not restructure the list mid-execution unless the user asks to replan. +- **`SetTodoList` marks the start of execution, not planning.** Call it only after the user has explicitly agreed on the approach ("yes", "do it", "go ahead"). Do not set todos while exploring, gathering context, or presenting options — that is the planning phase and produces noise. Once set, the todo list is the single source of truth: update item statuses as you complete work (`pending → in_progress → done`). Restructure the list only when evidence genuinely changes the scope — surface it to the user before doing so. - **Granular todos, not umbrella todos.** Each todo must name a single concrete deliverable a human can recognize as "this part is done." Avoid umbrella titles like "Determine X" or "Investigate Y" that cover hours of parallel work — they freeze the progress UI while real work happens underneath. If a single todo would stay `in_progress` for more than ~3 minutes, it is too coarse: split it before launching work. - **One todo per dispatched child.** When you launch `RunAgents` with N children, the visible todo list MUST contain one in_progress sub-todo per child (or per independent objective the batch covers) **before** the batch starts. Update each sub-todo to `done` as that child returns — do not wait for the whole batch to finish to flip a single umbrella todo. Same rule applies to multiple parallel `Agent` calls in the same turn. - Split broad work into independent chunks; use parallel tool calls or focused subagents for chunks that do not depend on each other. diff --git a/src/pythinker_code/cli/__init__.py b/src/pythinker_code/cli/__init__.py index ad99efa1..d70fbede 100644 --- a/src/pythinker_code/cli/__init__.py +++ b/src/pythinker_code/cli/__init__.py @@ -980,12 +980,9 @@ def _print_resume_hint(session: Session) -> None: if not session.is_empty(): _emit_fatal_error(f"\nTo resume this session: pythinker -r {session.id}") - async def _post_run( - last_session: Session, exit_code: int, *, cleanup_scratchpad: bool = False - ) -> None: + async def _post_run(last_session: Session, exit_code: int) -> None: # Always clean up this session's scratch file on exit (success or interruption) # so files never accumulate. Todo list and context persist separately. - _ = cleanup_scratchpad from pythinker_code.scratchpad import cleanup_session_scratch await cleanup_session_scratch( @@ -1064,11 +1061,7 @@ async def _reload_loop(session_id: str | None) -> tuple[str | None, int]: await _post_run(session, ExitCode.SUCCESS) return "vis", ExitCode.SUCCESS assert last_session is not None - await _post_run( - last_session, - exit_code, - cleanup_scratchpad=(ui == "print" and prompt is not None), - ) + await _post_run(last_session, exit_code) return None, exit_code except (SwitchToWeb, SwitchToVis): # Currently handled inside the loop (return), but re-raise explicitly diff --git a/src/pythinker_code/tools/todo/set_todo_list.md b/src/pythinker_code/tools/todo/set_todo_list.md index 8e7dbf48..e9327bf7 100644 --- a/src/pythinker_code/tools/todo/set_todo_list.md +++ b/src/pythinker_code/tools/todo/set_todo_list.md @@ -9,7 +9,7 @@ Set the todo list **only after the user has explicitly agreed on the plan**. The - **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 when work is fully done. -Once the todo list is set, it is the single source of truth for in-progress work. During execution, only update the **status** of existing items (`pending` → `in_progress` → `done`). Do not restructure or replace the list mid-execution unless the user explicitly asks to replan. +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 finish a subtask/milestone, update its status before moving to the next item. diff --git a/tests/core/test_default_agent.py b/tests/core/test_default_agent.py index f19a0fc0..7b9a7a1f 100644 --- a/tests/core/test_default_agent.py +++ b/tests/core/test_default_agent.py @@ -133,7 +133,7 @@ async def test_default_agent(runtime: Runtime): For any non-trivial request, decompose before acting: - Preview the terrain first: scan the directory structure, file headers, and relevant module boundaries before choosing an implementation path. -- Use `SetTodoList` for multi-step work so the user can see the active plan and progress. +- **`SetTodoList` marks the start of execution, not planning.** Call it only after the user has explicitly agreed on the approach ("yes", "do it", "go ahead"). Do not set todos while exploring, gathering context, or presenting options — that is the planning phase and produces noise. Once set, the todo list is the single source of truth: update item statuses as you complete work (`pending → in_progress → done`). Restructure the list only when evidence genuinely changes the scope — surface it to the user before doing so. - **Granular todos, not umbrella todos.** Each todo must name a single concrete deliverable a human can recognize as "this part is done." Avoid umbrella titles like "Determine X" or "Investigate Y" that cover hours of parallel work — they freeze the progress UI while real work happens underneath. If a single todo would stay `in_progress` for more than ~3 minutes, it is too coarse: split it before launching work. - **One todo per dispatched child.** When you launch `RunAgents` with N children, the visible todo list MUST contain one in_progress sub-todo per child (or per independent objective the batch covers) **before** the batch starts. Update each sub-todo to `done` as that child returns — do not wait for the whole batch to finish to flip a single umbrella todo. Same rule applies to multiple parallel `Agent` calls in the same turn. - Split broad work into independent chunks; use parallel tool calls or focused subagents for chunks that do not depend on each other. @@ -141,7 +141,7 @@ async def test_default_agent(runtime: Runtime): - Re-read the plan after each phase and adjust it when new evidence changes the approach. -As the root agent, treat named `.pythinker/scratch/*.md` files as the minimal session memory for context-aware work. The runtime auto-creates a per-session block with stable recall labels (for example `session:`, `workspace:`, `ui:`, `source:`) and compact milestones such as session start, todo summaries, agent/task starts, and task terminal status. Record durable working notes with the `Scratchpad` tool — classify each with `kind` (decision / evidence / blocker / next / note) — instead of editing these files by hand. Keep each note short and organized: current objective, searchable labels, load-bearing evidence, decisions, blockers, and next verification checkpoint. On a fresh run, or whenever the user asks about prior session work/history/context, fast-skim the relevant `.pythinker/scratch/*.md` labels and current session block before answering. Do not paste full logs, raw prompts, command output, secrets, or duplicate the whole `SetTodoList` checklist into the file. Retain session scratchpads after successful completion as compact history for future recall; remove them only when the user explicitly asks for cleanup. Subagents do not create their own scratch files. +As the root agent, use your session's `.pythinker/scratch/-*.md` file as private working notes for the **current session only**. The runtime auto-creates it with stable recall labels (for example `session:`, `workspace:`, `ui:`, `source:`). Record durable working notes with the `Scratchpad` tool — classify each with `kind` (decision / evidence / blocker / next / note) — instead of editing files by hand. Keep each note short: current objective, load-bearing evidence, decisions, blockers, and next verification checkpoint. Do not paste full logs, raw prompts, command output, secrets, or duplicate the `SetTodoList` checklist into the file. Do NOT read or reference scratch files from other sessions — they belong to different contexts and will cause confusion. Session files are automatically cleaned up when the session ends. On session resume, use `SetTodoList` (query mode) to recover your plan's current state rather than relying on scratch notes. Subagents do not create their own scratch files. Before every tool response, ask whether another independent read/search/check can run in the same turn. Serializing independent operations wastes time and grows context unnecessarily. @@ -703,10 +703,11 @@ async def test_default_agent_background_bash_guardrails(runtime: Runtime): @pytest.mark.skipif(platform.system() == "Windows", reason="Skipping test on Windows") async def test_default_agent_scratchpad_guardrails(runtime: Runtime): agent = await load_agent(DEFAULT_AGENT_FILE, runtime, mcp_configs=[]) - assert ".pythinker/scratch/*.md" in agent.system_prompt - assert "minimal session memory" in agent.system_prompt + assert ".pythinker/scratch/-*.md" in agent.system_prompt + assert "current session only" in agent.system_prompt assert "Do not paste full logs" in agent.system_prompt assert "Subagents do not create their own scratch files" in agent.system_prompt + assert "Do NOT read or reference scratch files from other sessions" in agent.system_prompt import dataclasses diff --git a/tests/core/test_scratchpad.py b/tests/core/test_scratchpad.py index df646eab..2663ce79 100644 --- a/tests/core/test_scratchpad.py +++ b/tests/core/test_scratchpad.py @@ -595,11 +595,11 @@ async def test_cleanup_symlink_unlinks_link_not_target(tmp_path): def test_render_available_matches_default_constant(): text = render_scratchpad_section(_AVAILABLE) assert text == DEFAULT_SCRATCHPAD_SECTION - assert "minimal session memory" in text - assert ".pythinker/scratch/*.md" in text + assert "current session only" in text + assert ".pythinker/scratch/-*.md" in text assert "SetTodoList" in text assert "full logs" in text - assert "Retain session scratchpads" in text + assert "automatically cleaned up" in text def test_render_unavailable_is_the_guard_line(): @@ -612,8 +612,8 @@ def test_render_unavailable_is_the_guard_line(): def test_render_available_with_existing_scratchpad_adds_recovery_instruction(): text = render_scratchpad_section(_AVAILABLE, scratch_exists=True) assert DEFAULT_SCRATCHPAD_SECTION in text - assert "prior scratchpad history exists" in text - assert "fast-skim labels" in text + assert "Recovery: a scratch file exists" in text + assert "Read only your own session's" in text def test_refresh_replaces_existing_marked_scratchpad_block(): From 092c7728e7b3e0488330d8e76bd084d767ca52af Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Wed, 3 Jun 2026 13:04:22 -0400 Subject: [PATCH 4/9] =?UTF-8?q?fix:=20update=20tests=20for=20=E2=9D=93=20q?= =?UTF-8?q?uestion=20marker=20and=20resolve=20CodeRabbit=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- .../src/pythinker_review/cli/review.py | 8 ++----- .../pythinker_review/security_scan/store.py | 7 +++++- .../pythinker_review/store/findings_store.py | 13 ++++++---- tests/tools/test_tool_descriptions.py | 24 +++++++++---------- tests/ui_and_conv/test_shell_design_system.py | 5 ++-- .../test_tui_card_tool_renderers.py | 3 ++- 6 files changed, 34 insertions(+), 26 deletions(-) diff --git a/packages/pythinker-review/src/pythinker_review/cli/review.py b/packages/pythinker-review/src/pythinker_review/cli/review.py index 6f6d23e4..d9939d13 100644 --- a/packages/pythinker-review/src/pythinker_review/cli/review.py +++ b/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: diff --git a/packages/pythinker-review/src/pythinker_review/security_scan/store.py b/packages/pythinker-review/src/pythinker_review/security_scan/store.py index f4be080a..ac4eca06 100644 --- a/packages/pythinker-review/src/pythinker_review/security_scan/store.py +++ b/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 diff --git a/packages/pythinker-review/src/pythinker_review/store/findings_store.py b/packages/pythinker-review/src/pythinker_review/store/findings_store.py index f2f9cf39..274727eb 100644 --- a/packages/pythinker-review/src/pythinker_review/store/findings_store.py +++ b/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 diff --git a/tests/tools/test_tool_descriptions.py b/tests/tools/test_tool_descriptions.py index 0923046c..426522ba 100644 --- a/tests/tools/test_tool_descriptions.py +++ b/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. """ ) diff --git a/tests/ui_and_conv/test_shell_design_system.py b/tests/ui_and_conv/test_shell_design_system.py index 9c69b817..f099793a 100644 --- a/tests/ui_and_conv/test_shell_design_system.py +++ b/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(): diff --git a/tests/ui_and_conv/test_tui_card_tool_renderers.py b/tests/ui_and_conv/test_tui_card_tool_renderers.py index f5296df6..e0490dcb 100644 --- a/tests/ui_and_conv/test_tui_card_tool_renderers.py +++ b/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 From f517487b562f948ad3b7a8f65a5a72a935d593f8 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Wed, 3 Jun 2026 13:07:54 -0400 Subject: [PATCH 5/9] docs: add multi-scope config design spec Covers User/Project/Local scope hierarchy, type-based merge rules (scalars override, lists concatenate, dicts deep-merge), hard scope locks for sensitive fields, env var overlay, provenance-enriched error messages, and full backward compatibility guarantees. --- ...026-06-03-pythinker-scope-config-design.md | 318 ++++++++++++++++++ 1 file changed, 318 insertions(+) create mode 100644 docs/superpowers/specs/2026-06-03-pythinker-scope-config-design.md diff --git a/docs/superpowers/specs/2026-06-03-pythinker-scope-config-design.md b/docs/superpowers/specs/2026-06-03-pythinker-scope-config-design.md new file mode 100644 index 00000000..fc33a7fb --- /dev/null +++ b/docs/superpowers/specs/2026-06-03-pythinker-scope-config-design.md @@ -0,0 +1,318 @@ +# Pythinker Multi-Scope Configuration Design + +**Date:** 2026-06-03 +**Branch:** `feat/scoped-config` +**Status:** Approved — ready for implementation planning + +--- + +## Overview + +Pythinker currently loads a single flat config from `~/.pythinker/config.toml` (user-global only). This design adds a three-scope layered system — **User**, **Project**, and **Local** — with type-based merging and hard security locks, inspired by Claude Code's settings architecture. + +--- + +## Scope Hierarchy + +``` +Priority Scope File Location Secrets Allowed? +────────────────────────────────────────────────────────────────────────────────── + 1 (high) Env Vars PYTHINKER_* environment variables ✅ yes + 2 Local .pythinker/config.local.toml ❌ locked + 3 Project .pythinker/config.toml ❌ locked + 4 (low) User ~/.pythinker/config.toml ✅ yes +``` + +**Project root resolution:** Walk up from `cwd` to the nearest `.git/`. If no git root is found, project and local scopes are silently skipped — pythinker stays fully usable outside git repositories. + +**Mental model for users:** +> *Project configs dictate behavior and structure. User configs dictate identity and access.* + +--- + +## File Locations + +``` +~/.pythinker/ +└── config.toml ← User scope (existing file, unchanged format) + +/ +└── .pythinker/ + ├── config.toml ← Project scope (committed to git) + └── config.local.toml ← Local scope (gitignored, personal overrides) +``` + +`.pythinker/config.local.toml` is automatically added to the project's `.gitignore` when pythinker first detects or creates it. + +--- + +## Merge Rules + +Applied in order: User → Project → Local (Local wins for scalars). + +| Field type | Behavior | Example | +|---|---|---| +| **Scalar** (str, bool, int, float) | Deepest scope wins | `theme = "light"` in local overrides `"dark"` in user | +| **List** | Concatenate widest→deepest | All `hooks` from user + project + local run | +| **List (dedup fields)** | Concatenate then deduplicate, preserving first-seen order | `extra_skill_dirs`, `allowed_domains` | +| **Dict** (nested object) | Deep-merge recursively | `tui.style` overrides without wiping `tui.smooth_streaming` | + +**Dedup fields:** `extra_skill_dirs`, `allowed_domains`. Order-preserving deduplication via `dict.fromkeys()`. + +**Env vars** always win over all file scopes for scalars. They are not subject to scope-lock checks. + +--- + +## Security: Scope-Locked Paths + +The following paths are **invalid in project and local scope** because they contain secrets that must not be committed to git: + +```python +SCOPE_LOCKED_PATHS: frozenset[tuple[str, ...]] = frozenset({ + ("providers",), # entire providers block (contains api_key per provider) + ("services",), # entire services block (contains api_key fields) + ("feedback", "api_key"), # only the api_key leaf; feedback.endpoint_url is allowed +}) +``` + +Non-secret feedback fields (`endpoint_url`, `github_client_id`, `github_repo`) are **allowed** in project scope. + +**Violation error message:** +``` +ConfigError: 'providers' cannot be set in .pythinker/config.toml (project scope). + Move it to ~/.pythinker/config.toml or set PYTHINKER_PROVIDER__API_KEY + in the environment. +``` + +--- + +## Environment Variable Map + +Flat top-level scalar fields map to `PYTHINKER_` env vars. Pydantic coerces string values to the correct type during `model_validate()`. + +```python +ENV_FIELD_MAP: dict[str, tuple[str, ...]] = { + "PYTHINKER_DEFAULT_MODEL": ("default_model",), + "PYTHINKER_THEME": ("theme",), + "PYTHINKER_DEFAULT_YOLO": ("default_yolo",), + "PYTHINKER_DEFAULT_PLAN_MODE": ("default_plan_mode",), + "PYTHINKER_TELEMETRY": ("telemetry",), + # … one entry per top-level scalar field in Config +} +``` + +Env var provenance is recorded as `f"env {env_key}"` (e.g. `"env PYTHINKER_THEME"`) so validation errors attribute the exact variable. + +--- + +## Resolution Pipeline + +``` +cwd → _find_project_root() + found: /my-project/.git → project_root = /my-project + not found → project_root = None (skip project + local) + +─── INGEST ────────────────────────────────────────────────────────────────── +user_dict = tomlkit.loads(~/.pythinker/config.toml) or {} +project_dict = tomlkit.loads(.pythinker/config.toml) or {} (skipped if no root) +local_dict = tomlkit.loads(.pythinker/config.local.toml) or {} (skipped if no root) + + TOMLKitError on any file → ConfigError("Invalid TOML in : …") + +─── GUARD ─────────────────────────────────────────────────────────────────── +_check_scope_locks(project_dict, ".pythinker/config.toml") +_check_scope_locks(local_dict, ".pythinker/config.local.toml") + + Walks SCOPE_LOCKED_PATHS in the raw dict; raises ConfigError on first violation. + +─── MERGE ─────────────────────────────────────────────────────────────────── +provenance: dict = {} +merged = _type_based_merge({}, user_dict, provenance, "~/.pythinker/config.toml") +merged = _type_based_merge(merged, project_dict, provenance, ".pythinker/config.toml") +merged = _type_based_merge(merged, local_dict, provenance, ".pythinker/config.local.toml") + + Scalar: provenance["theme"] = ".pythinker/config.local.toml" + List: provenance["hooks"] = "~/.pythinker/config.toml+.pythinker/config.toml" + (base-case: if no existing entry, provenance[key] = scope) + Dict: provenance["tui"]["style"] = ".pythinker/config.toml" + +─── ENV OVERLAY ───────────────────────────────────────────────────────────── +For each (env_key, path) in ENV_FIELD_MAP: + if os.environ.get(env_key) is not None: + _set_nested(merged, path, value) + _set_nested(provenance, path, f"env {env_key}") + +─── VALIDATE ──────────────────────────────────────────────────────────────── +try: + config = Config.model_validate(merged) +except ValidationError as exc: + enriched = [] + for err in exc.errors(): + scope = _lookup_provenance(provenance, tuple(err["loc"])) + field = ".".join(str(p) for p in err["loc"]) + enriched.append(f" {field}: {err['msg']} [from {scope}]") + raise ConfigError("Invalid configuration:\n" + "\n".join(enriched)) from exc +``` + +--- + +## Key Internal Functions + +### `_find_project_root(cwd: Path) -> Path | None` +Walks parent directories from `cwd` looking for `.git/`. Returns the directory containing `.git/`, or `None`. + +### `_check_scope_locks(scope_dict: dict, scope_name: str) -> None` +For each path in `SCOPE_LOCKED_PATHS`, walks `scope_dict` following the path tuple. Raises `ConfigError` on the first found violation. Check is on raw dict keys before Pydantic validation. + +### `_type_based_merge(base: dict, overlay: dict, provenance: dict, scope: str) -> dict` +Recursive merge with type dispatch: +- **Scalar:** `base[key] = overlay[key]`; `provenance[key] = scope` +- **List:** `base[key] = base[key] + overlay[key]`; apply `dict.fromkeys()` if key in `DEDUP_LIST_FIELDS`; `provenance[key] = f"{existing}+{scope}" if existing else scope` +- **Dict:** recurse into `_type_based_merge(base[key], overlay[key], provenance.setdefault(key, {}), scope)` + +### `_apply_env_vars(merged: dict, provenance: dict) -> None` +Iterates `ENV_FIELD_MAP`. For each key present in `os.environ`, calls `_set_nested` on both `merged` and `provenance`. Stores the raw string; Pydantic coerces the type during `model_validate()`. + +### `_set_nested(d: dict, path: tuple[str, ...], value: object) -> None` +Walks `path` into `d`, creating intermediate dicts as needed, sets the leaf. + +### `_lookup_provenance(prov: dict | str, loc: tuple) -> str` +```python +def _lookup_provenance(prov: dict | str, loc: tuple) -> str: + if not loc or isinstance(prov, str): + return prov if isinstance(prov, str) else "unknown scope" + head, *tail = loc + # Pydantic uses integer indices for list elements — map back to collection scope + if isinstance(head, int): + return prov if isinstance(prov, str) else _lookup_provenance(prov, tuple(tail)) + if isinstance(prov, dict) and head in prov: + return _lookup_provenance(prov[head], tuple(tail)) + return "unknown scope" +``` + +--- + +## `utils/gitignore.py` — `ensure_gitignored` + +``` +ensure_gitignored(git_root: Path, pattern: str, comment: str = "") -> None +``` + +1. Locate `git_root / ".gitignore"`. +2. If absent: create file from scratch (handles `FileNotFoundError`). +3. Read current content; if `pattern` already appears as a line, return (no-op). +4. Check for trailing newline; append one if missing. +5. Append `# {comment}\n{pattern}\n` (comment line omitted if `comment` is empty). + +Called during `_load_scoped` when local config is first written or detected, with: +```python +ensure_gitignored(project_root, ".pythinker/config.local.toml", comment="Added by pythinker") +``` + +--- + +## `Config` Model Changes + +Two new metadata fields (both `exclude=True` — never serialised): + +```python +source_scopes: dict[str, Path] = Field(default_factory=dict, exclude=True) +# e.g. {"user": Path("~/.pythinker/config.toml"), +# "project": Path(".pythinker/config.toml"), +# "local": Path(".pythinker/config.local.toml")} +``` + +`source_file` and `is_from_default_location` retain their existing semantics: +- `source_file` → user config path when resolved via scope pipeline; explicit path when `--config` is used +- `is_from_default_location` → `True` when user scope came from the default `~/.pythinker/config.toml` + +--- + +## Backward Compatibility + +| Scenario | Behavior | +|---|---| +| `load_config(explicit_path)` | Unchanged — single file, no scope resolution | +| `load_config()` outside git repo | User config only — identical to today | +| `load_config()` in git repo, no `.pythinker/` | User config only — silent fallback | +| Existing `~/.pythinker/config.toml` format | No change — still the user scope file | +| JSON→TOML migration logic | Unaffected — applies only to user scope file | +| `--config` CLI flag | Bypasses scope resolution entirely | + +--- + +## Public API Changes + +```python +# Unchanged signature — now routes through scope resolution when config_file is None +def load_config(config_file: Path | None = None) -> Config: ... + +# New internal entry point (not exported) +def _load_scoped(project_root: Path | None) -> Config: ... +``` + +No call sites in `cli/__init__.py`, `app.py`, or elsewhere need changes — existing `load_config()` calls automatically get scope resolution. + +--- + +## Error Message Examples + +``` +# Scope-lock violation +ConfigError: 'providers' cannot be set in .pythinker/config.toml (project scope). + Move it to ~/.pythinker/config.toml or set PYTHINKER_PROVIDER__API_KEY + in the environment. + +# Validation error with scope attribution +ConfigError: Invalid configuration: + default_model: Value 'gpt-9-turbo' not found in models [from .pythinker/config.local.toml] + tui.style: Invalid value 'rainbow' [from .pythinker/config.toml] + +# Env var validation error +ConfigError: Invalid configuration: + theme: Invalid value 'neon' [from env PYTHINKER_THEME] +``` + +--- + +## Test Plan + +### Unit tests (`tests/core/test_config.py`) + +**Merge algorithm (`_type_based_merge`)** +- `scalar_override` — local beats project beats user +- `list_concat` — user + project + local order preserved +- `list_dedup` — duplicate path in `extra_skill_dirs` appears once +- `dict_deep_merge` — nested key override without wiping siblings +- `provenance_scalar` — `provenance["theme"] == ".pythinker/config.local.toml"` +- `provenance_list` — composite string for multi-scope list +- `provenance_nested` — `provenance["tui"]["style"] == ".pythinker/config.toml"` +- `list_base_case` — single scope list → scope name only (no `+`) + +**Scope locks (`_check_scope_locks`)** +- `providers_in_project` — ConfigError, message cites file +- `services_in_local` — ConfigError +- `feedback_api_key_locked` — ConfigError on `{"feedback": {"api_key": "x"}}` +- `feedback_url_allowed` — no error on `{"feedback": {"endpoint_url": "https://…"}}` +- `clean_dict` — no error when locked paths absent + +**Provenance lookup (`_lookup_provenance`)** +- `scalar_path` — `loc=("theme",)` → correct scope string +- `nested_path` — `loc=("tui","style")` → correct scope string +- `list_index` — `loc=("hooks", 0, "command")` → parent collection scope (no crash) +- `partial_path` — `loc=("tui","nonexistent")` → `"unknown scope"` +- `empty_loc` — `loc=()` → returns provenance root value + +**Env var overlay (`_apply_env_vars`)** +- `known_key` — `PYTHINKER_THEME=light` → `merged["theme"]=="light"`, provenance set +- `unknown_key` — `PYTHINKER_XYZZY` ignored (not in `ENV_FIELD_MAP`) +- `bool_coercion` — `PYTHINKER_DEFAULT_YOLO=true` stored as string; Pydantic coerces on validate + +### Integration tests (full pipeline) +- `all_three_scopes` — three TOML files on disk → correct merged Config +- `user_only` — no git root → user config only +- `project_absent` — git root found, no `.pythinker/` → user config only +- `local_absent` — project present, local absent → user + project merged +- `scope_lock_violation` — providers in project → ConfigError before validation +- `validation_error_attribution` — bad `default_model` in local → error names local file +- `env_override` — `PYTHINKER_THEME` beats local file value From 7b5eacaecd30543bf54b4cd4234a65ba326afbd3 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Wed, 3 Jun 2026 13:18:08 -0400 Subject: [PATCH 6/9] docs: add scoped config implementation plan --- .../plans/2026-06-03-scoped-config.md | 1214 +++++++++++++++++ 1 file changed, 1214 insertions(+) create mode 100644 docs/superpowers/plans/2026-06-03-scoped-config.md diff --git a/docs/superpowers/plans/2026-06-03-scoped-config.md b/docs/superpowers/plans/2026-06-03-scoped-config.md new file mode 100644 index 00000000..c1b0e2ef --- /dev/null +++ b/docs/superpowers/plans/2026-06-03-scoped-config.md @@ -0,0 +1,1214 @@ +# Scoped Configuration Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Replace pythinker's single-file config with a three-scope system (User → Project → Local) using type-based merging, hard security locks on sensitive fields, and env-var overrides. + +**Architecture:** Load raw TOML dicts from up to three files, check scope-locked fields before merging, then type-merge (scalars override deepest-wins, lists concatenate, dicts deep-merge) into a single dict, overlay `PYTHINKER_*` env vars, and validate once through Pydantic. A parallel provenance map tracks which scope each value came from so validation errors name the source file. + +**Tech Stack:** Python 3.12+, `tomlkit` (already in deps), `pydantic` v2 (already in deps), `pytest` + `monkeypatch` for tests. + +**Spec:** `docs/superpowers/specs/2026-06-03-pythinker-scope-config-design.md` + +--- + +## File Map + +| Action | Path | Responsibility | +|--------|------|----------------| +| Modify | `src/pythinker_code/config.py` | All new constants, helpers, pipeline functions, `Config` field, `load_config` wiring | +| Create | `src/pythinker_code/utils/gitignore.py` | `ensure_gitignored` utility | +| Modify | `tests/core/test_config.py` | Unit + integration tests for pipeline functions | +| Create | `tests/utils/test_gitignore.py` | Tests for `ensure_gitignored` | + +No other files need changes — all existing `load_config()` call sites automatically gain scope resolution. + +--- + +## Task 1: Sync `_find_project_root` in `config.py` + +**Files:** +- Modify: `src/pythinker_code/config.py` +- Test: `tests/core/test_config.py` + +> **Context:** `utils/path.py` already has an async `find_project_root` that returns `work_dir` when no `.git` is found. We need a sync version that returns `None` — different enough to warrant a new private function in `config.py` rather than changing the shared one. + +- [ ] **Step 1: Write the failing test** + +Add to `tests/core/test_config.py`: + +```python +from pythinker_code.config import _find_project_root + + +def test_find_project_root_finds_git_root(tmp_path): + git_dir = tmp_path / ".git" + git_dir.mkdir() + subdir = tmp_path / "src" / "pkg" + subdir.mkdir(parents=True) + assert _find_project_root(subdir) == tmp_path + + +def test_find_project_root_returns_none_outside_git(tmp_path): + # tmp_path itself has no .git ancestor in practice + assert _find_project_root(tmp_path) is None + + +def test_find_project_root_finds_root_in_cwd(tmp_path): + (tmp_path / ".git").mkdir() + assert _find_project_root(tmp_path) == tmp_path +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +cd /home/ai/Projects/pythinker-code-main +.venv/bin/pytest tests/core/test_config.py::test_find_project_root_finds_git_root tests/core/test_config.py::test_find_project_root_returns_none_outside_git tests/core/test_config.py::test_find_project_root_finds_root_in_cwd -v +``` + +Expected: `ImportError` or `AttributeError` — `_find_project_root` does not exist yet. + +- [ ] **Step 3: Implement `_find_project_root`** + +Add after the `get_share_dir` import block in `src/pythinker_code/config.py`, before the `AgentExecutionProfile` definition: + +```python +def _find_project_root(cwd: Path) -> Path | None: + """Walk up from cwd to find the nearest directory containing .git/. + + Returns None when no .git marker is found before reaching the filesystem + root, so callers can skip project/local scopes without a fallback. + """ + current = cwd.resolve() + while True: + if (current / ".git").exists(): + return current + parent = current.parent + if parent == current: + return None + current = parent +``` + +- [ ] **Step 4: Run tests to verify they pass** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_find_project_root_finds_git_root tests/core/test_config.py::test_find_project_root_returns_none_outside_git tests/core/test_config.py::test_find_project_root_finds_root_in_cwd -v +``` + +Expected: all 3 PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/pythinker_code/config.py tests/core/test_config.py +git commit -m "feat(config): add sync _find_project_root helper" +``` + +--- + +## Task 2: `utils/gitignore.py` — `ensure_gitignored` + +**Files:** +- Create: `src/pythinker_code/utils/gitignore.py` +- Create: `tests/utils/test_gitignore.py` + +- [ ] **Step 1: Write failing tests** + +Create `tests/utils/test_gitignore.py`: + +```python +from __future__ import annotations + +from pathlib import Path + +import pytest + +from pythinker_code.utils.gitignore import ensure_gitignored + + +def test_creates_gitignore_when_absent(tmp_path): + ensure_gitignored(tmp_path, ".pythinker/config.local.toml", comment="Added by pythinker") + gi = tmp_path / ".gitignore" + assert gi.exists() + content = gi.read_text() + assert ".pythinker/config.local.toml" in content + assert "Added by pythinker" in content + + +def test_appends_to_existing_gitignore(tmp_path): + gi = tmp_path / ".gitignore" + gi.write_text("*.pyc\n", encoding="utf-8") + ensure_gitignored(tmp_path, ".pythinker/config.local.toml") + content = gi.read_text() + assert "*.pyc" in content + assert ".pythinker/config.local.toml" in content + + +def test_no_op_when_pattern_already_present(tmp_path): + gi = tmp_path / ".gitignore" + gi.write_text(".pythinker/config.local.toml\n", encoding="utf-8") + ensure_gitignored(tmp_path, ".pythinker/config.local.toml") + # No duplicate + lines = [l for l in gi.read_text().splitlines() if l == ".pythinker/config.local.toml"] + assert len(lines) == 1 + + +def test_fixes_missing_trailing_newline(tmp_path): + gi = tmp_path / ".gitignore" + gi.write_text("*.pyc", encoding="utf-8") # no trailing newline + ensure_gitignored(tmp_path, ".pythinker/config.local.toml") + content = gi.read_text() + # Pattern must start on its own line, not appended to "*.pyc" + assert "\n.pythinker/config.local.toml" in content + + +def test_omits_comment_when_empty(tmp_path): + ensure_gitignored(tmp_path, ".pythinker/config.local.toml", comment="") + content = (tmp_path / ".gitignore").read_text() + assert ".pythinker/config.local.toml" in content + assert "#" not in content +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +.venv/bin/pytest tests/utils/test_gitignore.py -v +``` + +Expected: `ModuleNotFoundError` — `utils/gitignore.py` does not exist yet. + +- [ ] **Step 3: Implement `ensure_gitignored`** + +Create `src/pythinker_code/utils/gitignore.py`: + +```python +from __future__ import annotations + +from pathlib import Path + + +def ensure_gitignored(git_root: Path, pattern: str, comment: str = "") -> None: + """Append *pattern* to /.gitignore if not already present. + + Creates .gitignore if the file does not exist. Handles missing trailing + newline before appending. Prepends a comment line when *comment* is given. + """ + gi_path = git_root / ".gitignore" + + if gi_path.exists(): + content = gi_path.read_text(encoding="utf-8") + # Check if pattern is already present as a standalone line + if any(line.strip() == pattern for line in content.splitlines()): + return + else: + content = "" + + lines_to_append: list[str] = [] + if content and not content.endswith("\n"): + lines_to_append.append("\n") + if comment: + lines_to_append.append(f"# {comment}\n") + lines_to_append.append(f"{pattern}\n") + + with gi_path.open("a", encoding="utf-8") as f: + f.write("".join(lines_to_append)) +``` + +- [ ] **Step 4: Run tests to verify they pass** + +```bash +.venv/bin/pytest tests/utils/test_gitignore.py -v +``` + +Expected: all 5 PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/pythinker_code/utils/gitignore.py tests/utils/test_gitignore.py +git commit -m "feat(utils): add ensure_gitignored utility" +``` + +--- + +## Task 3: Constants and helper functions in `config.py` + +**Files:** +- Modify: `src/pythinker_code/config.py` +- Test: `tests/core/test_config.py` + +> **Context:** Add the constants (`SCOPE_LOCKED_PATHS`, `DEDUP_LIST_FIELDS`, `ENV_FIELD_MAP`) and the two small helper functions (`_set_nested`, `_lookup_provenance`). These are pure functions with no side effects and can be tested in isolation. + +- [ ] **Step 1: Write failing tests** + +Add to `tests/core/test_config.py`: + +```python +from pythinker_code.config import _lookup_provenance, _set_nested + + +def test_set_nested_flat(): + d: dict = {} + _set_nested(d, ("theme",), "light") + assert d == {"theme": "light"} + + +def test_set_nested_deep(): + d: dict = {} + _set_nested(d, ("tui", "style"), "card") + assert d == {"tui": {"style": "card"}} + + +def test_set_nested_overwrites_existing(): + d = {"tui": {"style": "pythinker", "smooth_streaming": True}} + _set_nested(d, ("tui", "style"), "card") + assert d["tui"]["style"] == "card" + assert d["tui"]["smooth_streaming"] is True # sibling preserved + + +def test_lookup_provenance_scalar(): + prov = {"theme": ".pythinker/config.local.toml"} + assert _lookup_provenance(prov, ("theme",)) == ".pythinker/config.local.toml" + + +def test_lookup_provenance_nested(): + prov = {"tui": {"style": ".pythinker/config.toml"}} + assert _lookup_provenance(prov, ("tui", "style")) == ".pythinker/config.toml" + + +def test_lookup_provenance_list_index(): + # Pydantic gives loc=("hooks", 0, "command") for a bad list element. + # Should return the collection scope, not crash. + prov = {"hooks": "~/.pythinker/config.toml+.pythinker/config.toml"} + assert _lookup_provenance(prov, ("hooks", 0, "command")) == "~/.pythinker/config.toml+.pythinker/config.toml" + + +def test_lookup_provenance_partial_path(): + prov = {"tui": {"style": ".pythinker/config.toml"}} + assert _lookup_provenance(prov, ("tui", "nonexistent")) == "unknown scope" + + +def test_lookup_provenance_empty_loc(): + prov = "~/.pythinker/config.toml" + assert _lookup_provenance(prov, ()) == "~/.pythinker/config.toml" + + +def test_lookup_provenance_unknown(): + prov: dict = {} + assert _lookup_provenance(prov, ("missing_key",)) == "unknown scope" +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_set_nested_flat tests/core/test_config.py::test_lookup_provenance_scalar -v +``` + +Expected: `ImportError` — `_set_nested`, `_lookup_provenance` not defined yet. + +- [ ] **Step 3: Add constants and helpers to `config.py`** + +Add after the `_find_project_root` function: + +```python +# --------------------------------------------------------------------------- +# Scope system constants +# --------------------------------------------------------------------------- + +SCOPE_LOCKED_PATHS: frozenset[tuple[str, ...]] = frozenset( + { + ("providers",), # contains api_key per provider — must stay in user scope + ("services",), # contains api_key fields — must stay in user scope + ("feedback", "api_key"), # only the key, not the whole feedback section + } +) + +DEDUP_LIST_FIELDS: frozenset[str] = frozenset({"allowed_domains", "extra_skill_dirs"}) + +ENV_FIELD_MAP: dict[str, tuple[str, ...]] = { + "PYTHINKER_DEFAULT_MODEL": ("default_model",), + "PYTHINKER_DEFAULT_THINKING": ("default_thinking",), + "PYTHINKER_DEFAULT_THINKING_EFFORT": ("default_thinking_effort",), + "PYTHINKER_AGENT_EXECUTION_PROFILE": ("agent_execution_profile",), + "PYTHINKER_DEFAULT_YOLO": ("default_yolo",), + "PYTHINKER_ASK_USER_QUESTION_POLICY": ("ask_user_question_policy",), + "PYTHINKER_AUTO_DELIBERATE_DESTRUCTIVE_ACTIONS": ("auto_deliberate_destructive_actions",), + "PYTHINKER_SKIP_AUTO_PROMPT_INJECTION": ("skip_auto_prompt_injection",), + "PYTHINKER_DEFAULT_PLAN_MODE": ("default_plan_mode",), + "PYTHINKER_DEFAULT_EDITOR": ("default_editor",), + "PYTHINKER_THEME": ("theme",), + "PYTHINKER_SHOW_THINKING_STREAM": ("show_thinking_stream",), + "PYTHINKER_PREVENT_IDLE_SLEEP": ("prevent_idle_sleep",), + "PYTHINKER_TELEMETRY": ("telemetry",), + "PYTHINKER_SESSION_RETENTION_DAYS": ("session_retention_days",), + "PYTHINKER_MERGE_ALL_AVAILABLE_SKILLS": ("merge_all_available_skills",), +} + + +# --------------------------------------------------------------------------- +# Pipeline helpers +# --------------------------------------------------------------------------- + + +def _set_nested(d: dict, path: tuple[str, ...], value: object) -> None: + """Walk *path* into *d*, creating intermediate dicts, then set the leaf.""" + node = d + for part in path[:-1]: + if part not in node or not isinstance(node[part], dict): + node[part] = {} + node = node[part] + node[path[-1]] = value + + +def _lookup_provenance(prov: "dict | str", loc: tuple) -> str: + """Recursively follow *loc* through the provenance map. + + Integer elements (Pydantic list indices) are skipped — we map them back + to the parent collection's scope string so error messages stay useful. + Returns "unknown scope" when the path cannot be fully resolved. + """ + if not loc or isinstance(prov, str): + return prov if isinstance(prov, str) else "unknown scope" + head, *tail = loc + if isinstance(head, int): + return prov if isinstance(prov, str) else _lookup_provenance(prov, tuple(tail)) + if isinstance(prov, dict) and head in prov: + return _lookup_provenance(prov[head], tuple(tail)) + return "unknown scope" +``` + +- [ ] **Step 4: Run tests to verify they pass** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_set_nested_flat tests/core/test_config.py::test_set_nested_deep tests/core/test_config.py::test_set_nested_overwrites_existing tests/core/test_config.py::test_lookup_provenance_scalar tests/core/test_config.py::test_lookup_provenance_nested tests/core/test_config.py::test_lookup_provenance_list_index tests/core/test_config.py::test_lookup_provenance_partial_path tests/core/test_config.py::test_lookup_provenance_empty_loc tests/core/test_config.py::test_lookup_provenance_unknown -v +``` + +Expected: all 9 PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/pythinker_code/config.py tests/core/test_config.py +git commit -m "feat(config): add scope constants and provenance helpers" +``` + +--- + +## Task 4: `_check_scope_locks` + +**Files:** +- Modify: `src/pythinker_code/config.py` +- Test: `tests/core/test_config.py` + +- [ ] **Step 1: Write failing tests** + +Add to `tests/core/test_config.py`: + +```python +from pythinker_code.config import _check_scope_locks + + +def test_scope_lock_providers_in_project(): + with pytest.raises(ConfigError, match="'providers'.*project scope"): + _check_scope_locks({"providers": {"openai": {}}}, ".pythinker/config.toml") + + +def test_scope_lock_services_in_local(): + with pytest.raises(ConfigError, match="'services'.*local scope"): + _check_scope_locks({"services": {"pythinker_ai_search": {}}}, ".pythinker/config.local.toml") + + +def test_scope_lock_feedback_api_key(): + with pytest.raises(ConfigError, match="'feedback.api_key'"): + _check_scope_locks( + {"feedback": {"api_key": "secret"}}, ".pythinker/config.toml" + ) + + +def test_scope_lock_feedback_url_allowed(): + # feedback.endpoint_url is NOT locked — should not raise + _check_scope_locks( + {"feedback": {"endpoint_url": "https://internal.example.com"}}, + ".pythinker/config.toml", + ) + + +def test_scope_lock_clean_dict(): + _check_scope_locks({"theme": "light", "default_model": "gpt-4"}, ".pythinker/config.toml") + + +def test_scope_lock_error_mentions_env_var(): + with pytest.raises(ConfigError, match="PYTHINKER_PROVIDER"): + _check_scope_locks({"providers": {}}, ".pythinker/config.toml") +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_scope_lock_providers_in_project tests/core/test_config.py::test_scope_lock_clean_dict -v +``` + +Expected: `ImportError` — `_check_scope_locks` not defined yet. + +- [ ] **Step 3: Implement `_check_scope_locks`** + +Add after `_lookup_provenance` in `src/pythinker_code/config.py`: + +```python +def _check_scope_locks(scope_dict: dict, scope_name: str) -> None: + """Raise ConfigError if *scope_dict* contains any scope-locked field paths. + + Checks every path in SCOPE_LOCKED_PATHS by walking the raw dict before + Pydantic validation, so secrets are blocked before they can be merged. + """ + for path in SCOPE_LOCKED_PATHS: + node: object = scope_dict + for part in path: + if not isinstance(node, dict) or part not in node: + break + else: + field_path = ".".join(path) + # Derive a short scope label for the error message + if "local" in scope_name: + scope_label = "local scope" + elif "project" in scope_name or scope_name.startswith(".pythinker"): + scope_label = "project scope" + else: + scope_label = scope_name + raise ConfigError( + f"'{field_path}' cannot be set in {scope_name} ({scope_label}).\n" + f" Move it to ~/.pythinker/config.toml or set " + f"PYTHINKER_PROVIDER__API_KEY in the environment." + ) +``` + +- [ ] **Step 4: Run tests to verify they pass** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_scope_lock_providers_in_project tests/core/test_config.py::test_scope_lock_services_in_local tests/core/test_config.py::test_scope_lock_feedback_api_key tests/core/test_config.py::test_scope_lock_feedback_url_allowed tests/core/test_config.py::test_scope_lock_clean_dict tests/core/test_config.py::test_scope_lock_error_mentions_env_var -v +``` + +Expected: all 6 PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/pythinker_code/config.py tests/core/test_config.py +git commit -m "feat(config): add _check_scope_locks with path-level secret detection" +``` + +--- + +## Task 5: `_type_based_merge` + +**Files:** +- Modify: `src/pythinker_code/config.py` +- Test: `tests/core/test_config.py` + +- [ ] **Step 1: Write failing tests** + +Add to `tests/core/test_config.py`: + +```python +from pythinker_code.config import _type_based_merge + + +def test_merge_scalar_override(): + prov: dict = {} + result = _type_based_merge({"theme": "dark"}, {"theme": "light"}, prov, ".pythinker/config.local.toml") + assert result["theme"] == "light" + assert prov["theme"] == ".pythinker/config.local.toml" + + +def test_merge_scalar_three_scopes(): + prov: dict = {} + base = _type_based_merge({}, {"theme": "dark"}, prov, "~/.pythinker/config.toml") + base = _type_based_merge(base, {"theme": "solarized"}, prov, ".pythinker/config.toml") + base = _type_based_merge(base, {"theme": "light"}, prov, ".pythinker/config.local.toml") + assert base["theme"] == "light" + assert prov["theme"] == ".pythinker/config.local.toml" + + +def test_merge_list_concat(): + prov: dict = {} + base = _type_based_merge({}, {"hooks": [{"event": "Stop", "command": "a"}]}, prov, "~/.pythinker/config.toml") + base = _type_based_merge(base, {"hooks": [{"event": "Stop", "command": "b"}]}, prov, ".pythinker/config.toml") + assert len(base["hooks"]) == 2 + assert base["hooks"][0]["command"] == "a" + assert base["hooks"][1]["command"] == "b" + + +def test_merge_list_concat_provenance(): + prov: dict = {} + base = _type_based_merge({}, {"hooks": []}, prov, "~/.pythinker/config.toml") + base = _type_based_merge(base, {"hooks": []}, prov, ".pythinker/config.toml") + assert prov["hooks"] == "~/.pythinker/config.toml+.pythinker/config.toml" + + +def test_merge_list_base_case_provenance(): + prov: dict = {} + _type_based_merge({}, {"hooks": []}, prov, "~/.pythinker/config.toml") + assert prov["hooks"] == "~/.pythinker/config.toml" + + +def test_merge_list_dedup_extra_skill_dirs(): + prov: dict = {} + base = _type_based_merge({}, {"extra_skill_dirs": ["/a", "/b"]}, prov, "~/.pythinker/config.toml") + base = _type_based_merge(base, {"extra_skill_dirs": ["/b", "/c"]}, prov, ".pythinker/config.toml") + # /b appears in both — should appear only once (first occurrence kept) + assert base["extra_skill_dirs"] == ["/a", "/b", "/c"] + + +def test_merge_dict_deep(): + prov: dict = {} + base = _type_based_merge( + {}, {"tui": {"style": "pythinker", "smooth_streaming": True}}, prov, "~/.pythinker/config.toml" + ) + base = _type_based_merge( + base, {"tui": {"style": "card"}}, prov, ".pythinker/config.toml" + ) + assert base["tui"]["style"] == "card" + assert base["tui"]["smooth_streaming"] is True # sibling preserved + assert prov["tui"]["style"] == ".pythinker/config.toml" + assert prov["tui"]["smooth_streaming"] == "~/.pythinker/config.toml" + + +def test_merge_key_only_in_overlay(): + prov: dict = {} + result = _type_based_merge({}, {"theme": "dark"}, prov, "~/.pythinker/config.toml") + assert result["theme"] == "dark" + assert prov["theme"] == "~/.pythinker/config.toml" +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_merge_scalar_override tests/core/test_config.py::test_merge_list_concat -v +``` + +Expected: `ImportError` — `_type_based_merge` not defined yet. + +- [ ] **Step 3: Implement `_type_based_merge`** + +Add after `_check_scope_locks` in `src/pythinker_code/config.py`: + +```python +def _type_based_merge(base: dict, overlay: dict, provenance: dict, scope: str) -> dict: + """Merge *overlay* into *base* using type-based rules, tracking provenance. + + Rules: + - Scalar (str/bool/int/float/None): overlay wins, provenance records scope. + - List: base + overlay concatenated; DEDUP_LIST_FIELDS deduplicated + (order-preserving, first occurrence wins). + - Dict: recurse so nested keys can be independently overridden. + + Mutates *base* and *provenance* in place; also returns *base* for chaining. + """ + for key, value in overlay.items(): + if key not in base: + base[key] = value + provenance[key] = scope + elif isinstance(value, list) and isinstance(base[key], list): + combined = base[key] + value + if key in DEDUP_LIST_FIELDS: + combined = list(dict.fromkeys(combined)) + base[key] = combined + existing = provenance.get(key) + provenance[key] = f"{existing}+{scope}" if existing else scope + elif isinstance(value, dict) and isinstance(base[key], dict): + _type_based_merge( + base[key], + value, + provenance.setdefault(key, {}), + scope, + ) + else: + base[key] = value + provenance[key] = scope + return base +``` + +- [ ] **Step 4: Run tests to verify they pass** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_merge_scalar_override tests/core/test_config.py::test_merge_scalar_three_scopes tests/core/test_config.py::test_merge_list_concat tests/core/test_config.py::test_merge_list_concat_provenance tests/core/test_config.py::test_merge_list_base_case_provenance tests/core/test_config.py::test_merge_list_dedup_extra_skill_dirs tests/core/test_config.py::test_merge_dict_deep tests/core/test_config.py::test_merge_key_only_in_overlay -v +``` + +Expected: all 8 PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/pythinker_code/config.py tests/core/test_config.py +git commit -m "feat(config): add _type_based_merge with dedup and provenance tracking" +``` + +--- + +## Task 6: `_apply_env_vars` + +**Files:** +- Modify: `src/pythinker_code/config.py` +- Test: `tests/core/test_config.py` + +- [ ] **Step 1: Write failing tests** + +Add to `tests/core/test_config.py`: + +```python +from pythinker_code.config import _apply_env_vars + + +def test_apply_env_vars_known_key(monkeypatch): + monkeypatch.setenv("PYTHINKER_THEME", "light") + merged: dict = {} + prov: dict = {} + _apply_env_vars(merged, prov) + assert merged["theme"] == "light" + assert prov["theme"] == "env PYTHINKER_THEME" + + +def test_apply_env_vars_unknown_key_ignored(monkeypatch): + monkeypatch.setenv("PYTHINKER_XYZZY_UNKNOWN", "whatever") + merged: dict = {} + prov: dict = {} + _apply_env_vars(merged, prov) + assert "xyzzy_unknown" not in merged + + +def test_apply_env_vars_bool_coercion(monkeypatch): + monkeypatch.setenv("PYTHINKER_DEFAULT_YOLO", "true") + merged: dict = {} + prov: dict = {} + _apply_env_vars(merged, prov) + # Stored as string; Pydantic coerces during model_validate + assert merged["default_yolo"] == "true" + assert prov["default_yolo"] == "env PYTHINKER_DEFAULT_YOLO" + + +def test_apply_env_vars_overrides_existing(monkeypatch): + monkeypatch.setenv("PYTHINKER_THEME", "light") + merged = {"theme": "dark"} + prov = {"theme": "~/.pythinker/config.toml"} + _apply_env_vars(merged, prov) + assert merged["theme"] == "light" + assert prov["theme"] == "env PYTHINKER_THEME" +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_apply_env_vars_known_key tests/core/test_config.py::test_apply_env_vars_unknown_key_ignored -v +``` + +Expected: `ImportError` — `_apply_env_vars` not defined yet. + +- [ ] **Step 3: Implement `_apply_env_vars`** + +Add after `_type_based_merge` in `src/pythinker_code/config.py`: + +```python +def _apply_env_vars(merged: dict, provenance: dict) -> None: + """Overlay PYTHINKER_* env vars onto *merged*, updating *provenance*. + + Values are stored as raw strings; Pydantic coerces them during + model_validate(). Only keys in ENV_FIELD_MAP are recognised; all others + are silently ignored. + """ + for env_key, path in ENV_FIELD_MAP.items(): + value = os.environ.get(env_key) + if value is not None: + _set_nested(merged, path, value) + _set_nested(provenance, path, f"env {env_key}") +``` + +- [ ] **Step 4: Run tests to verify they pass** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_apply_env_vars_known_key tests/core/test_config.py::test_apply_env_vars_unknown_key_ignored tests/core/test_config.py::test_apply_env_vars_bool_coercion tests/core/test_config.py::test_apply_env_vars_overrides_existing -v +``` + +Expected: all 4 PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/pythinker_code/config.py tests/core/test_config.py +git commit -m "feat(config): add _apply_env_vars with ENV_FIELD_MAP" +``` + +--- + +## Task 7: `source_scopes` field on `Config` + +**Files:** +- Modify: `src/pythinker_code/config.py` +- Test: `tests/core/test_config.py` + +> **Context:** Add `source_scopes: dict[str, Path]` as an `exclude=True` metadata field alongside the existing `source_file` and `is_from_default_location` fields. It is never serialised. + +- [ ] **Step 1: Write a failing test** + +Add to `tests/core/test_config.py`: + +```python +def test_config_source_scopes_default_empty(): + config = get_default_config() + assert config.source_scopes == {} + + +def test_config_source_scopes_not_in_dump(): + config = get_default_config() + dumped = config.model_dump() + assert "source_scopes" not in dumped +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_config_source_scopes_default_empty tests/core/test_config.py::test_config_source_scopes_not_in_dump -v +``` + +Expected: `AttributeError` — `source_scopes` does not exist yet. + +- [ ] **Step 3: Add `source_scopes` to `Config`** + +In `src/pythinker_code/config.py`, inside the `Config` class, add alongside the existing `source_file` field: + +```python +source_scopes: dict[str, Path] = Field( + default_factory=dict, + description=( + "Paths of config files that contributed to this resolved config, keyed by scope name. " + "e.g. {'user': Path('~/.pythinker/config.toml'), 'project': Path('.pythinker/config.toml')}." + ), + exclude=True, +) +``` + +- [ ] **Step 4: Run tests to verify they pass** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_config_source_scopes_default_empty tests/core/test_config.py::test_config_source_scopes_not_in_dump -v +``` + +Expected: both PASS. + +- [ ] **Step 5: Run existing config tests to confirm no regression** + +```bash +.venv/bin/pytest tests/core/test_config.py -v +``` + +Expected: all existing tests PASS. + +- [ ] **Step 6: Commit** + +```bash +git add src/pythinker_code/config.py tests/core/test_config.py +git commit -m "feat(config): add source_scopes metadata field to Config" +``` + +--- + +## Task 8: `_load_scoped` pipeline function + +**Files:** +- Modify: `src/pythinker_code/config.py` +- Test: `tests/core/test_config.py` + +> **Context:** This is the heart of the feature. It wires all previous functions into the five-step pipeline: Ingest → Guard → Merge → Env → Validate. + +- [ ] **Step 1: Write integration tests** + +Add to `tests/core/test_config.py`: + +```python +import tomlkit +from pythinker_code.config import _load_scoped + + +def _write_toml(path: Path, data: dict) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(tomlkit.dumps(data), encoding="utf-8") # type: ignore[arg-type] + + +def test_load_scoped_user_only(tmp_path, monkeypatch): + monkeypatch.setenv("PYTHINKER_SHARE_DIR", str(tmp_path)) + _write_toml(tmp_path / "config.toml", {"theme": "light"}) + config = _load_scoped(project_root=None) + assert config.theme == "light" + assert config.source_scopes["user"] == (tmp_path / "config.toml").resolve() + + +def test_load_scoped_project_overrides_user(tmp_path, monkeypatch): + monkeypatch.setenv("PYTHINKER_SHARE_DIR", str(tmp_path)) + _write_toml(tmp_path / "config.toml", {"theme": "dark"}) + project_root = tmp_path / "myproject" + _write_toml(project_root / ".pythinker" / "config.toml", {"theme": "solarized"}) + config = _load_scoped(project_root=project_root) + assert config.theme == "solarized" + + +def test_load_scoped_local_overrides_project(tmp_path, monkeypatch): + monkeypatch.setenv("PYTHINKER_SHARE_DIR", str(tmp_path)) + _write_toml(tmp_path / "config.toml", {"theme": "dark"}) + project_root = tmp_path / "myproject" + _write_toml(project_root / ".pythinker" / "config.toml", {"theme": "solarized"}) + _write_toml(project_root / ".pythinker" / "config.local.toml", {"theme": "light"}) + config = _load_scoped(project_root=project_root) + assert config.theme == "light" + + +def test_load_scoped_hooks_concatenate(tmp_path, monkeypatch): + monkeypatch.setenv("PYTHINKER_SHARE_DIR", str(tmp_path)) + _write_toml(tmp_path / "config.toml", {"hooks": [{"event": "Stop", "command": "user-hook"}]}) + project_root = tmp_path / "myproject" + _write_toml( + project_root / ".pythinker" / "config.toml", + {"hooks": [{"event": "Stop", "command": "project-hook"}]}, + ) + config = _load_scoped(project_root=project_root) + commands = [h.command for h in config.hooks] + assert "user-hook" in commands + assert "project-hook" in commands + + +def test_load_scoped_scope_lock_violation(tmp_path, monkeypatch): + monkeypatch.setenv("PYTHINKER_SHARE_DIR", str(tmp_path)) + _write_toml(tmp_path / "config.toml", {}) + project_root = tmp_path / "myproject" + _write_toml( + project_root / ".pythinker" / "config.toml", + {"providers": {"bad": {"type": "openai", "base_url": "x", "api_key": "sk-x"}}}, + ) + with pytest.raises(ConfigError, match="'providers'"): + _load_scoped(project_root=project_root) + + +def test_load_scoped_validation_error_attributes_scope(tmp_path, monkeypatch): + monkeypatch.setenv("PYTHINKER_SHARE_DIR", str(tmp_path)) + _write_toml(tmp_path / "config.toml", {}) + project_root = tmp_path / "myproject" + _write_toml( + project_root / ".pythinker" / "config.local.toml", + {"theme": "neon"}, # invalid value + ) + with pytest.raises(ConfigError, match="config.local.toml"): + _load_scoped(project_root=project_root) + + +def test_load_scoped_env_override(tmp_path, monkeypatch): + monkeypatch.setenv("PYTHINKER_SHARE_DIR", str(tmp_path)) + monkeypatch.setenv("PYTHINKER_THEME", "light") + _write_toml(tmp_path / "config.toml", {"theme": "dark"}) + project_root = tmp_path / "myproject" + _write_toml(project_root / ".pythinker" / "config.toml", {"theme": "solarized"}) + config = _load_scoped(project_root=project_root) + assert config.theme == "light" # env beats all file scopes + + +def test_load_scoped_source_scopes_populated(tmp_path, monkeypatch): + monkeypatch.setenv("PYTHINKER_SHARE_DIR", str(tmp_path)) + _write_toml(tmp_path / "config.toml", {}) + project_root = tmp_path / "myproject" + _write_toml(project_root / ".pythinker" / "config.toml", {}) + config = _load_scoped(project_root=project_root) + assert "user" in config.source_scopes + assert "project" in config.source_scopes + assert "local" not in config.source_scopes # local file absent +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_load_scoped_user_only tests/core/test_config.py::test_load_scoped_scope_lock_violation -v +``` + +Expected: `ImportError` — `_load_scoped` not defined yet. + +- [ ] **Step 3: Implement `_load_scoped`** + +Add after `_apply_env_vars` in `src/pythinker_code/config.py`. Also add `import copy` at the top of the file if not already present: + +```python +def _load_scoped(project_root: Path | None) -> Config: + """Run the five-step scoped config resolution pipeline. + + Steps: Ingest → Guard → Merge → Env → Validate. + Returns a fully-validated Config with source_scopes populated. + """ + from pythinker_code.utils.gitignore import ensure_gitignored + + # ── INGEST ──────────────────────────────────────────────────────────── + default_user_file = get_config_file().expanduser().resolve(strict=False) + # Trigger JSON→TOML migration if needed (existing logic) + if not default_user_file.exists(): + migration_error = _migrate_json_config_to_toml() + if migration_error is not None: + raise ConfigError( + f"Legacy config file has incompatible settings; please fix or " + f"rename/delete {migration_error.config_file} to continue. " + f"Errors: {migration_error.errors}" + ) from None + + def _read_toml(path: Path) -> dict: + if not path.exists(): + return {} + try: + return dict(tomlkit.loads(path.read_text(encoding="utf-8"))) + except TOMLKitError as exc: + raise ConfigError(f"Invalid TOML in {path}: {exc}") from exc + + user_file = default_user_file + user_dict = _read_toml(user_file) + + project_file: Path | None = None + local_file: Path | None = None + project_dict: dict = {} + local_dict: dict = {} + + if project_root is not None: + project_file = project_root / ".pythinker" / "config.toml" + local_file = project_root / ".pythinker" / "config.local.toml" + project_dict = _read_toml(project_file) + local_dict = _read_toml(local_file) + + # ── GUARD ───────────────────────────────────────────────────────────── + if project_file is not None: + _check_scope_locks(project_dict, str(project_file)) + if local_file is not None: + _check_scope_locks(local_dict, str(local_file)) + + # ── MERGE ───────────────────────────────────────────────────────────── + provenance: dict = {} + merged = _type_based_merge({}, user_dict, provenance, str(user_file)) + if project_dict: + merged = _type_based_merge(merged, project_dict, provenance, str(project_file)) + if local_dict: + merged = _type_based_merge(merged, local_dict, provenance, str(local_file)) + + # ── ENV OVERLAY ─────────────────────────────────────────────────────── + _apply_env_vars(merged, provenance) + + # ── VALIDATE ────────────────────────────────────────────────────────── + try: + config = Config.model_validate(merged) + except ValidationError as exc: + enriched: list[str] = [] + for err in exc.errors(): + scope = _lookup_provenance(provenance, tuple(err["loc"])) + field = ".".join(str(p) for p in err["loc"]) + enriched.append(f" {field}: {err['msg']} [from {scope}]") + raise ConfigError("Invalid configuration:\n" + "\n".join(enriched)) from exc + + # ── METADATA ────────────────────────────────────────────────────────── + config.is_from_default_location = True + config.source_file = user_file + if user_file.exists(): + config.source_scopes["user"] = user_file + if project_file is not None and project_file.exists(): + config.source_scopes["project"] = project_file + if local_file is not None and local_file.exists(): + config.source_scopes["local"] = local_file + # Auto-gitignore local config so it is never accidentally committed + ensure_gitignored( + project_root, # type: ignore[arg-type] + ".pythinker/config.local.toml", + comment="Added by pythinker", + ) + + return config +``` + +- [ ] **Step 4: Verify `import copy` is present** (we use `_type_based_merge` which mutates in-place, so no `copy` needed — but double-check the imports at the top of `config.py` include `os` which `_apply_env_vars` uses) + +```bash +grep "^import os" /home/ai/Projects/pythinker-code-main/src/pythinker_code/config.py +``` + +Expected: `import os` found. If not, add `import os` to the imports. + +- [ ] **Step 5: Run integration tests** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_load_scoped_user_only tests/core/test_config.py::test_load_scoped_project_overrides_user tests/core/test_config.py::test_load_scoped_local_overrides_project tests/core/test_config.py::test_load_scoped_hooks_concatenate tests/core/test_config.py::test_load_scoped_scope_lock_violation tests/core/test_config.py::test_load_scoped_validation_error_attributes_scope tests/core/test_config.py::test_load_scoped_env_override tests/core/test_config.py::test_load_scoped_source_scopes_populated -v +``` + +Expected: all 8 PASS. + +- [ ] **Step 6: Commit** + +```bash +git add src/pythinker_code/config.py tests/core/test_config.py +git commit -m "feat(config): add _load_scoped five-step pipeline" +``` + +--- + +## Task 9: Wire `load_config` + full regression sweep + +**Files:** +- Modify: `src/pythinker_code/config.py` +- Test: `tests/core/test_config.py` + +> **Context:** Update `load_config` to route through `_load_scoped` when called with no explicit file path. When an explicit path is given, use the original code path unchanged. Run the full test suite to verify no regression. + +- [ ] **Step 1: Write a backward-compatibility test** + +Add to `tests/core/test_config.py`: + +```python +def test_load_config_explicit_path_bypasses_scoping(tmp_path): + """--config flag must bypass scope resolution entirely.""" + config_file = tmp_path / "explicit.toml" + config_file.write_text('theme = "light"\n', encoding="utf-8") + config = load_config(config_file) + assert config.theme == "light" + assert config.source_file == config_file.resolve() + # source_scopes is empty because no scope pipeline was run + assert config.source_scopes == {} + + +def test_load_config_no_args_uses_scope_resolution(tmp_path, monkeypatch): + """load_config() with no args routes through scoped pipeline.""" + monkeypatch.setenv("PYTHINKER_SHARE_DIR", str(tmp_path)) + (tmp_path / "config.toml").write_text('theme = "light"\n', encoding="utf-8") + # No git root in tmp_path — falls back to user-only + config = load_config() + assert config.theme == "light" + assert "user" in config.source_scopes +``` + +- [ ] **Step 2: Run these tests to verify they fail** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_load_config_explicit_path_bypasses_scoping tests/core/test_config.py::test_load_config_no_args_uses_scope_resolution -v +``` + +Expected: `test_load_config_no_args_uses_scope_resolution` FAIL (source_scopes empty because `load_config` hasn't been updated yet). + +- [ ] **Step 3: Update `load_config` in `config.py`** + +Replace the start of `load_config` so it routes through `_load_scoped` when no explicit file is given: + +```python +def load_config(config_file: Path | None = None) -> Config: + """Load configuration, resolving up to three scopes when no explicit file is given. + + When *config_file* is None (the default), the scoped pipeline runs: + User (~/.pythinker/config.toml) → Project (.pythinker/config.toml) → + Local (.pythinker/config.local.toml), merged with type-based rules. + + When *config_file* is given explicitly (e.g. via --config), that single + file is loaded directly with no scope resolution — preserving the legacy + behaviour used by tests and the CLI --config flag. + """ + if config_file is None: + project_root = _find_project_root(Path.cwd()) + return _load_scoped(project_root) + + # ── Explicit path: legacy single-file load (unchanged) ──────────────── + default_config_file = get_config_file().expanduser().resolve(strict=False) + config_file = config_file.expanduser().resolve(strict=False) + is_default_config_file = config_file == default_config_file + logger.debug("Loading config from file: {file}", file=config_file) + + if is_default_config_file and not config_file.exists(): + migration_error = _migrate_json_config_to_toml() + if migration_error is not None: + raise ConfigError( + f"Legacy config file has incompatible settings; please fix or " + f"rename/delete {migration_error.config_file} to continue. " + f"Errors: {migration_error.errors}" + ) from None + + if not config_file.exists(): + config = get_default_config() + logger.debug("No config file found, creating default config: {config}", config=config) + save_config(config, config_file) + config.is_from_default_location = is_default_config_file + config.source_file = config_file + return config + + try: + config_text = config_file.read_text(encoding="utf-8") + if config_file.suffix.lower() == ".json": + data = json.loads(config_text) + else: + data = tomlkit.loads(config_text) + config = Config.model_validate(data) + except json.JSONDecodeError as e: + raise ConfigError(f"Invalid JSON in configuration file {config_file}: {e}") from e + except TOMLKitError as e: + raise ConfigError(f"Invalid TOML in configuration file {config_file}: {e}") from e + except ValidationError as e: + raise ConfigError(f"Invalid configuration file {config_file}: {e}") from e + config.is_from_default_location = is_default_config_file + config.source_file = config_file + return config +``` + +- [ ] **Step 4: Run the two new tests** + +```bash +.venv/bin/pytest tests/core/test_config.py::test_load_config_explicit_path_bypasses_scoping tests/core/test_config.py::test_load_config_no_args_uses_scope_resolution -v +``` + +Expected: both PASS. + +- [ ] **Step 5: Run the full config test suite** + +```bash +.venv/bin/pytest tests/core/test_config.py -v +``` + +Expected: all tests PASS. If `test_load_config_sets_source_file` fails because `source_scopes` is now non-empty, update its assertion to only check `source_file` and `is_from_default_location`. + +- [ ] **Step 6: Run the broader test suite** + +```bash +.venv/bin/pytest tests/ -x -q --ignore=tests/e2e 2>&1 | tail -30 +``` + +Expected: no new failures. Fix any failures before committing. + +- [ ] **Step 7: Run the linter/formatter** + +```bash +cd /home/ai/Projects/pythinker-code-main && make check-pythinker-code +``` + +Expected: all checks pass. Fix any ruff errors before committing. + +- [ ] **Step 8: Commit** + +```bash +git add src/pythinker_code/config.py tests/core/test_config.py +git commit -m "feat(config): wire load_config to scope resolution pipeline + +When called with no explicit file path, load_config now discovers +User → Project → Local scopes relative to the nearest .git root, +merges them with type-based rules, overlays PYTHINKER_* env vars, +and validates once through Pydantic with provenance-enriched errors. +Explicit --config path continues to bypass scope resolution." +``` + +--- + +## Self-Review Checklist + +- [x] **`_find_project_root`** — Task 1 ✓ +- [x] **`ensure_gitignored`** — Task 2 ✓ (including FileNotFoundError / create-if-absent, trailing newline, comment hygiene) +- [x] **`SCOPE_LOCKED_PATHS`, `DEDUP_LIST_FIELDS`, `ENV_FIELD_MAP`** — Task 3 ✓ +- [x] **`_set_nested`, `_lookup_provenance`** — Task 3 ✓ (integer index bypass in lookup) +- [x] **`_check_scope_locks`** with path-level check — Task 4 ✓ (`feedback.api_key` locked, `feedback.endpoint_url` allowed) +- [x] **`_type_based_merge`** with all three dispatch branches + dedup — Task 5 ✓ +- [x] **`_apply_env_vars`** with full `ENV_FIELD_MAP` — Task 6 ✓ +- [x] **`source_scopes` field** on `Config` — Task 7 ✓ (exclude=True, not serialised) +- [x] **`_load_scoped`** five-step pipeline — Task 8 ✓ (auto-gitignore in metadata step) +- [x] **`load_config` wiring + regression sweep** — Task 9 ✓ +- [x] **Backward compatibility** — explicit `--config` path still bypasses scoping (Task 9 Step 3) +- [x] **All test names** reference functions defined in earlier tasks — no forward references +- [x] **Human-readable scope strings** passed as `scope` param (file paths, not tags like `"local"`) +- [x] **Provenance base-case** for list: `scope` alone when no prior entry (Task 5 impl) From f2a64ee9abbaf5f1cd2c79d90ea4ac2680d377d2 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Wed, 3 Jun 2026 13:26:45 -0400 Subject: [PATCH 7/9] fix: add purge_unknown tests and wire clean command into docs/agent config - Add three unit tests for FindingsStore.purge_unknown: noop when state_dir missing, keeps allowed names, removes unknown files and dirs - Document pythinker-review clean command in README alongside clean-locks - Add pythinker review clean guidance to code_reviewer.yaml so the agent knows to use it for stale review state maintenance --- packages/pythinker-review/README.md | 4 +-- .../tests/unit/test_findings_store.py | 33 +++++++++++++++++++ .../agents/default/code_reviewer.yaml | 1 + 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/packages/pythinker-review/README.md b/packages/pythinker-review/README.md index dac48d90..f404e746 100644 --- a/packages/pythinker-review/README.md +++ b/packages/pythinker-review/README.md @@ -105,8 +105,8 @@ Phase 1 now ports the highest-value behavior from the mounted blackbox repos: outside the reviewed chunk/feature, unsafe paths, omitted/truncated line ranges, or non-matching evidence snippets; invalid sibling findings are recorded as drops without failing the whole run. - Reviewflow pure-Python stateful commands cover `init`, `map`, `status`, `review`, `ci`, - `report`, `show --finding`, `next`, `triage`, `revalidate`, `fix`, `open-pr`, `doctor`, and - `clean-locks`. + `report`, `show --finding`, `next`, `triage`, `revalidate`, `fix`, `open-pr`, `doctor`, + `clean-locks`, and `clean` (removes unexpected entries from `.pythinker-review/`). - Code-review prompt parity covers partial-diff caveats, concrete trigger scenarios, test analysis, suggested regression tests, and minimum fix scope. - Code-reviewr PR assistant parity adds read-only `describe`, `improve`/`suggest`, `ask`, diff --git a/packages/pythinker-review/tests/unit/test_findings_store.py b/packages/pythinker-review/tests/unit/test_findings_store.py index f30a8921..d43cfc21 100644 --- a/packages/pythinker-review/tests/unit/test_findings_store.py +++ b/packages/pythinker-review/tests/unit/test_findings_store.py @@ -78,3 +78,36 @@ def test_begin_twice_without_finalize_raises(tmp_path: Path) -> None: with pytest.raises(RuntimeError, match="begin\\(\\) called twice"): store.begin(meta) store.finalize(meta) + + +def test_purge_unknown_noop_when_state_dir_missing(tmp_path: Path) -> None: + store = FindingsStore(repo_root=tmp_path) + assert store.purge_unknown() == [] + + +def test_purge_unknown_keeps_allowed_names(tmp_path: Path) -> None: + state_dir = tmp_path / ".pythinker-review" + state_dir.mkdir() + (state_dir / "index.json").write_text("{}", encoding="utf-8") + (state_dir / "runs").mkdir() + (state_dir / "security-scan").mkdir() + store = FindingsStore(repo_root=tmp_path) + assert store.purge_unknown() == [] + assert (state_dir / "index.json").exists() + assert (state_dir / "runs").exists() + assert (state_dir / "security-scan").exists() + + +def test_purge_unknown_removes_unknown_file_and_dir(tmp_path: Path) -> None: + state_dir = tmp_path / ".pythinker-review" + state_dir.mkdir() + stray_file = state_dir / "stray.json" + stray_file.write_text("{}", encoding="utf-8") + stray_dir = state_dir / "old-report" + stray_dir.mkdir() + (stray_dir / "data.txt").write_text("x", encoding="utf-8") + store = FindingsStore(repo_root=tmp_path) + removed = store.purge_unknown() + assert set(removed) == {"stray.json", "old-report"} + assert not stray_file.exists() + assert not stray_dir.exists() diff --git a/src/pythinker_code/agents/default/code_reviewer.yaml b/src/pythinker_code/agents/default/code_reviewer.yaml index bcdb8693..7327d88a 100644 --- a/src/pythinker_code/agents/default/code_reviewer.yaml +++ b/src/pythinker_code/agents/default/code_reviewer.yaml @@ -10,6 +10,7 @@ agent: - Use `pythinker review diff` by default for bounded branch/diff review; add `--with-security` when the parent requests security coverage. - For repo-wide, long-running, resumable, or feature-slice review requests, prefer the stateful flow: `pythinker review init`, `pythinker review map`, then `pythinker review review --limit --jobs ` followed by `report`/`next`/`show`/`triage` as needed. - Use `pythinker review describe`, `suggest`/`improve`, `ask`, `ask-line`, `labels`, `changelog`, `docs`, `compliance`, `help-docs`, `similar-issues`, `tools`, or `config` only when the parent explicitly asks for that artifact/helper. + - Use `pythinker review clean` to remove unexpected entries from `.pythinker-review/` when the parent requests maintenance or cleanup of stale review state; pass `--dry-run` first to preview what will be removed. - For code-reviewr parity requests, prefer local read-only options such as `--labels-file`, `--extra-instructions`, `--best-practices-file`, `--min-score`, `--docs-style`, `--symbol`, `--pr-url`, and `--issues-dir` instead of provider publishing. - Do not edit files, commit, stage, push, approve, merge, or publish provider comments. From 199f3170f372505b0675b6d291e61595c9249c61 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Wed, 3 Jun 2026 13:48:12 -0400 Subject: [PATCH 8/9] fix: address all CodeRabbit review findings on PR #72 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - set_todo_list.md: add H1 heading (markdownlint MD041) - scoped-config plan: generalize locked-path error message from provider-specific PYTHINKER_PROVIDER__API_KEY to the generic PYTHINKER_* form so it's correct for services and feedback.api_key violations too - scoped-config plan: rename step 4 label from "Verify import copy" to "Verify import os" — copy is not needed, os is required by _apply_env_vars - spec doc: add language specifiers to 5 bare code fences (text/python), add blank lines before 3 code blocks, add blank lines after 6 headings to pass markdownlint MD040/MD022 - pythinker-review README: separate `clean` from Reviewflow stateful commands and note it targets .pythinker-review/ not .pythinker-review-flow/ - code_reviewer.yaml: mark `pythinker review clean` as destructive purge and distinguish the two state dirs --- .../plans/2026-06-03-scoped-config.md | 8 +++++--- ...026-06-03-pythinker-scope-config-design.md | 20 +++++++++++++------ packages/pythinker-review/README.md | 4 +++- .../agents/default/code_reviewer.yaml | 2 +- .../tools/todo/set_todo_list.md | 2 +- 5 files changed, 24 insertions(+), 12 deletions(-) diff --git a/docs/superpowers/plans/2026-06-03-scoped-config.md b/docs/superpowers/plans/2026-06-03-scoped-config.md index c1b0e2ef..a67a62ce 100644 --- a/docs/superpowers/plans/2026-06-03-scoped-config.md +++ b/docs/superpowers/plans/2026-06-03-scoped-config.md @@ -477,8 +477,8 @@ def _check_scope_locks(scope_dict: dict, scope_name: str) -> None: scope_label = scope_name raise ConfigError( f"'{field_path}' cannot be set in {scope_name} ({scope_label}).\n" - f" Move it to ~/.pythinker/config.toml or set " - f"PYTHINKER_PROVIDER__API_KEY in the environment." + f" Move it to ~/.pythinker/config.toml or set the corresponding " + f"PYTHINKER_* environment variable." ) ``` @@ -1021,7 +1021,9 @@ def _load_scoped(project_root: Path | None) -> Config: return config ``` -- [ ] **Step 4: Verify `import copy` is present** (we use `_type_based_merge` which mutates in-place, so no `copy` needed — but double-check the imports at the top of `config.py` include `os` which `_apply_env_vars` uses) +- [ ] **Step 4: Verify `import os` is present** + +Check that `config.py` imports `os` (required by `_apply_env_vars`). Note: `_type_based_merge` mutates in-place, so `import copy` is not needed. ```bash grep "^import os" /home/ai/Projects/pythinker-code-main/src/pythinker_code/config.py diff --git a/docs/superpowers/specs/2026-06-03-pythinker-scope-config-design.md b/docs/superpowers/specs/2026-06-03-pythinker-scope-config-design.md index fc33a7fb..ce4a35b8 100644 --- a/docs/superpowers/specs/2026-06-03-pythinker-scope-config-design.md +++ b/docs/superpowers/specs/2026-06-03-pythinker-scope-config-design.md @@ -14,7 +14,7 @@ Pythinker currently loads a single flat config from `~/.pythinker/config.toml` ( ## Scope Hierarchy -``` +```text Priority Scope File Location Secrets Allowed? ────────────────────────────────────────────────────────────────────────────────── 1 (high) Env Vars PYTHINKER_* environment variables ✅ yes @@ -32,7 +32,7 @@ Priority Scope File Location Secrets Allowe ## File Locations -``` +```text ~/.pythinker/ └── config.toml ← User scope (existing file, unchanged format) @@ -78,7 +78,8 @@ SCOPE_LOCKED_PATHS: frozenset[tuple[str, ...]] = frozenset({ Non-secret feedback fields (`endpoint_url`, `github_client_id`, `github_repo`) are **allowed** in project scope. **Violation error message:** -``` + +```text ConfigError: 'providers' cannot be set in .pythinker/config.toml (project scope). Move it to ~/.pythinker/config.toml or set PYTHINKER_PROVIDER__API_KEY in the environment. @@ -107,7 +108,7 @@ Env var provenance is recorded as `f"env {env_key}"` (e.g. `"env PYTHINKER_THEME ## Resolution Pipeline -``` +```text cwd → _find_project_root() found: /my-project/.git → project_root = /my-project not found → project_root = None (skip project + local) @@ -159,24 +160,30 @@ except ValidationError as exc: ## Key Internal Functions ### `_find_project_root(cwd: Path) -> Path | None` + Walks parent directories from `cwd` looking for `.git/`. Returns the directory containing `.git/`, or `None`. ### `_check_scope_locks(scope_dict: dict, scope_name: str) -> None` + For each path in `SCOPE_LOCKED_PATHS`, walks `scope_dict` following the path tuple. Raises `ConfigError` on the first found violation. Check is on raw dict keys before Pydantic validation. ### `_type_based_merge(base: dict, overlay: dict, provenance: dict, scope: str) -> dict` + Recursive merge with type dispatch: - **Scalar:** `base[key] = overlay[key]`; `provenance[key] = scope` - **List:** `base[key] = base[key] + overlay[key]`; apply `dict.fromkeys()` if key in `DEDUP_LIST_FIELDS`; `provenance[key] = f"{existing}+{scope}" if existing else scope` - **Dict:** recurse into `_type_based_merge(base[key], overlay[key], provenance.setdefault(key, {}), scope)` ### `_apply_env_vars(merged: dict, provenance: dict) -> None` + Iterates `ENV_FIELD_MAP`. For each key present in `os.environ`, calls `_set_nested` on both `merged` and `provenance`. Stores the raw string; Pydantic coerces the type during `model_validate()`. ### `_set_nested(d: dict, path: tuple[str, ...], value: object) -> None` + Walks `path` into `d`, creating intermediate dicts as needed, sets the leaf. ### `_lookup_provenance(prov: dict | str, loc: tuple) -> str` + ```python def _lookup_provenance(prov: dict | str, loc: tuple) -> str: if not loc or isinstance(prov, str): @@ -194,7 +201,7 @@ def _lookup_provenance(prov: dict | str, loc: tuple) -> str: ## `utils/gitignore.py` — `ensure_gitignored` -``` +```text ensure_gitignored(git_root: Path, pattern: str, comment: str = "") -> None ``` @@ -205,6 +212,7 @@ ensure_gitignored(git_root: Path, pattern: str, comment: str = "") -> None 5. Append `# {comment}\n{pattern}\n` (comment line omitted if `comment` is empty). Called during `_load_scoped` when local config is first written or detected, with: + ```python ensure_gitignored(project_root, ".pythinker/config.local.toml", comment="Added by pythinker") ``` @@ -257,7 +265,7 @@ No call sites in `cli/__init__.py`, `app.py`, or elsewhere need changes — exis ## Error Message Examples -``` +```text # Scope-lock violation ConfigError: 'providers' cannot be set in .pythinker/config.toml (project scope). Move it to ~/.pythinker/config.toml or set PYTHINKER_PROVIDER__API_KEY diff --git a/packages/pythinker-review/README.md b/packages/pythinker-review/README.md index f404e746..4c16b91a 100644 --- a/packages/pythinker-review/README.md +++ b/packages/pythinker-review/README.md @@ -106,7 +106,9 @@ Phase 1 now ports the highest-value behavior from the mounted blackbox repos: evidence snippets; invalid sibling findings are recorded as drops without failing the whole run. - Reviewflow pure-Python stateful commands cover `init`, `map`, `status`, `review`, `ci`, `report`, `show --finding`, `next`, `triage`, `revalidate`, `fix`, `open-pr`, `doctor`, - `clean-locks`, and `clean` (removes unexpected entries from `.pythinker-review/`). + and `clean-locks` (all operate on `.pythinker-review-flow/`). +- `clean` command removes unexpected entries from the diff-save state dir (`.pythinker-review/`), + which is a separate directory from the Reviewflow state. - Code-review prompt parity covers partial-diff caveats, concrete trigger scenarios, test analysis, suggested regression tests, and minimum fix scope. - Code-reviewr PR assistant parity adds read-only `describe`, `improve`/`suggest`, `ask`, diff --git a/src/pythinker_code/agents/default/code_reviewer.yaml b/src/pythinker_code/agents/default/code_reviewer.yaml index 7327d88a..d1ad020e 100644 --- a/src/pythinker_code/agents/default/code_reviewer.yaml +++ b/src/pythinker_code/agents/default/code_reviewer.yaml @@ -10,7 +10,7 @@ agent: - Use `pythinker review diff` by default for bounded branch/diff review; add `--with-security` when the parent requests security coverage. - For repo-wide, long-running, resumable, or feature-slice review requests, prefer the stateful flow: `pythinker review init`, `pythinker review map`, then `pythinker review review --limit --jobs ` followed by `report`/`next`/`show`/`triage` as needed. - Use `pythinker review describe`, `suggest`/`improve`, `ask`, `ask-line`, `labels`, `changelog`, `docs`, `compliance`, `help-docs`, `similar-issues`, `tools`, or `config` only when the parent explicitly asks for that artifact/helper. - - Use `pythinker review clean` to remove unexpected entries from `.pythinker-review/` when the parent requests maintenance or cleanup of stale review state; pass `--dry-run` first to preview what will be removed. + - Use `pythinker review clean` to **destructively purge** unexpected files from `.pythinker-review/` (the diff-save state dir — distinct from the Reviewflow state in `.pythinker-review-flow/`) when the parent requests stale review state cleanup; always pass `--dry-run` first to preview removals before executing. - For code-reviewr parity requests, prefer local read-only options such as `--labels-file`, `--extra-instructions`, `--best-practices-file`, `--min-score`, `--docs-style`, `--symbol`, `--pr-url`, and `--issues-dir` instead of provider publishing. - Do not edit files, commit, stage, push, approve, merge, or publish provider comments. diff --git a/src/pythinker_code/tools/todo/set_todo_list.md b/src/pythinker_code/tools/todo/set_todo_list.md index e9327bf7..09cf56ea 100644 --- a/src/pythinker_code/tools/todo/set_todo_list.md +++ b/src/pythinker_code/tools/todo/set_todo_list.md @@ -1,4 +1,4 @@ -Manage your todo list for tracking task progress during execution. +# Manage your todo list for tracking task progress during execution. **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. From ac745c0a732056c7502ede3d8b298fc03cc8fde8 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Wed, 3 Jun 2026 14:17:21 -0400 Subject: [PATCH 9/9] fix: update snapshot for set_todo_list H1 heading change --- tests/tools/test_tool_descriptions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tools/test_tool_descriptions.py b/tests/tools/test_tool_descriptions.py index 426522ba..b985a7e1 100644 --- a/tests/tools/test_tool_descriptions.py +++ b/tests/tools/test_tool_descriptions.py @@ -140,7 +140,7 @@ 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 during execution. +# Manage your todo list for tracking task progress during execution. **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.