fix: standardize ❓ question marker and isolate scratchpad to current session - #72
Conversation
…session **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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPR standardizes the question-marker glyph, makes scratchpads session-scoped with automatic cleanup on exit, requires explicit user approval before SetTodoList, adds review-store cleanup/purge helpers and a ChangesQuestion marker glyph
Session scratchpad and todo gating
Review store housekeeping
Sequence Diagram(s)sequenceDiagram
participant SessionLifecycle
participant CLI as _post_run()
participant Cleanup as cleanup_session_scratch()
participant FS as Filesystem
SessionLifecycle->>CLI: session ends (any exit_code)
CLI->>Cleanup: call cleanup_session_scratch(work_dir, session_id, session_title)
Cleanup->>FS: unlink session scratch file (if regular file)
Cleanup-->>CLI: return (suppress errors)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pythinker_code/agents/default/system.md`:
- Line 116: The sentence in system.md about SetTodoList is too strict and
conflicts with guidance in set_todo_list.md about updating plans as evidence
changes; revise the paragraph referencing SetTodoList so it keeps the
“status-only” rule for normal execution but explicitly allows restructuring the
todo list when new evidence requires changing the plan (or when the user
requests replanning), and add a cross-reference to set_todo_list.md; update the
wording around "update item statuses only (`pending → in_progress → done`), do
not restructure the list mid-execution unless the user asks to replan" to permit
evidence-driven restructuring and point readers to SetTodoList and
set_todo_list.md for the full decision rules.
In `@src/pythinker_code/cli/__init__.py`:
- Around line 984-995: The function currently accepts an unused parameter
cleanup_scratchpad (assigned to `_ = cleanup_scratchpad`) but always runs
cleanup_session_scratch unconditionally; remove the cleanup_scratchpad parameter
from the function signature, delete the dead assignment `_ =
cleanup_scratchpad`, keep the call to
pythinker_code.scratchpad.cleanup_session_scratch as-is, and update any callers
that currently pass cleanup_scratchpad to stop supplying that argument so the
call sites match the new signature.
In `@src/pythinker_code/scratchpad.py`:
- Around line 1123-1139: Update the tests to match the revised
DEFAULT_SCRATCHPAD_SECTION text in scratchpad.py: replace the old "Retain
session scratchpads..." expectation in tests/core/test_scratchpad.py and the
snapshot in tests/core/test_default_agent.py with the new wording that
emphasizes session-only scratch files, automatic cleanup when the session ends,
and using SetTodoList on resume; locate the canonical string in the
DEFAULT_SCRATCHPAD_SECTION constant in src/pythinker_code/scratchpad.py and copy
that exact text into the assertion and snapshot so they match exactly.
In `@src/pythinker_code/tools/todo/set_todo_list.md`:
- Line 1: The top-level markdown heading is missing; update the first line
"Manage your todo list for tracking task progress during execution." in
set_todo_list.md to be an H1 by prefixing it with "# " so the file has a proper
top-level heading for markdownlint and standalone rendering.
- Line 12: The sentence "only update the **status** of existing items (`pending`
→ `in_progress` → `done`)" is ambiguous and conflicts with the learning that
plans should be updated as evidence changes; either (A) relax it by replacing
that sentence with guidance that agents may also propose or apply structural
plan changes when new evidence warrants (e.g., "Agents should update statuses
during normal execution but may add, remove, or reorder todo items and surface
those changes to the user when evidence alters the plan; use 'replan' when user
confirmation is required"), or (B) keep the current strict behavior but
explicitly state the constraint and when to call out for replanning (e.g., "No
structural changes may be made unless the user explicitly requests a replan;
agents may only change status otherwise"), and ensure the document references
the "replan" flow and how agents must surface proposed changes to the user.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 799464b3-261d-435b-b3b0-774973786867
📒 Files selected for processing (9)
CHANGELOG.mdsrc/pythinker_code/agents/default/system.mdsrc/pythinker_code/cli/__init__.pysrc/pythinker_code/scratchpad.pysrc/pythinker_code/tools/todo/set_todo_list.mdsrc/pythinker_code/ui/shell/design_system.pysrc/pythinker_code/ui/shell/glyphs.pysrc/pythinker_code/ui/shell/tool_renderers/ask_user.pysrc/pythinker_code/ui/shell/visualize/_question_panel.py
- 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.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pythinker-review/src/pythinker_review/cli/review.py`:
- Around line 368-397: The dry-run branch hardcodes the allowlist {"index.json",
"runs", "security-scan"}; import and use the single source of truth
_ALLOWED_NAMES from the findings_store module instead: update imports to bring
in _ALLOWED_NAMES (alongside FindingsStore), and replace the inline set used in
the list comprehension (the check in clean() that builds unknown by iterating
store.state_dir.iterdir()) with a membership test against _ALLOWED_NAMES so the
dry-run behavior matches purge_unknown()'s allowlist.
In `@packages/pythinker-review/src/pythinker_review/security_scan/store.py`:
- Around line 266-281: The current purge_stale_projects function uses
shutil.rmtree(..., ignore_errors=True) which silences deletion failures; change
purge_stale_projects to remove ignore_errors and capture failures instead:
update the function signature (purge_stale_projects) to return both removed and
failed project-id lists (e.g., removed: list[str], failed: list[str]), iterate
data_root.iterdir(), for each candidate call shutil.rmtree inside a try/except
catching OSError/Exception, on success append entry.name to removed, on failure
append entry.name to failed (do not swallow the exception), and let the
caller/CLI report or log the failed list so init can warn the user.
In `@packages/pythinker-review/src/pythinker_review/store/findings_store.py`:
- Around line 96-112: The purge_unknown method currently calls
shutil.rmtree(..., ignore_errors=True) which hides deletion failures; modify
purge_unknown to remove ignore_errors=True, catch exceptions from rmtree and
Path.unlink, collect failed entry names (e.g., a new failed: list[str])
alongside removed, and return or expose both removed and failed so callers (CLI)
can report errors; update the method signature/return (purge_unknown) and any
callers to handle the new failure list and include context in logged/returned
messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d686a4ac-0bc6-4810-b359-007ce57f9c60
📒 Files selected for processing (13)
.claude/settings.json.pythinker/reports/arch-review-2026-05-30.md.pythinker/reports/deep-code-scan-findings.md.pythinker/reports/validation-tui-renderer-contract-hardening.mdpackages/pythinker-review/src/pythinker_review/cli/review.pypackages/pythinker-review/src/pythinker_review/cli/security_scan.pypackages/pythinker-review/src/pythinker_review/security_scan/store.pypackages/pythinker-review/src/pythinker_review/store/findings_store.pysrc/pythinker_code/agents/default/system.mdsrc/pythinker_code/cli/__init__.pysrc/pythinker_code/tools/todo/set_todo_list.mdtests/core/test_default_agent.pytests/core/test_scratchpad.py
💤 Files with no reviewable changes (3)
- .pythinker/reports/arch-review-2026-05-30.md
- .pythinker/reports/deep-code-scan-findings.md
- .pythinker/reports/validation-tui-renderer-contract-hardening.md
- 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
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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pythinker-review/src/pythinker_review/cli/review.py (1)
368-392:⚠️ Potential issue | 🟠 MajorWire up the new user-facing
cleanCLI command (wrapper/tests/docs/config)Companion updates for
@app.command(name="clean")inpackages/pythinker-review/src/pythinker_review/cli/review.pyare missing:
src/pythinker_code/cli/review.pywrapper has nocleancommand reference.- No CLI tests under
tests/cli/coverclean.packages/pythinker-review/README.mdmentionsclean-locks, notclean.src/pythinker_code/agents/default/code_reviewer.yamlhas nocleancommand/config entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pythinker-review/src/pythinker_review/cli/review.py` around lines 368 - 392, The new CLI command implemented as `@app.command`(name="clean") (which uses FindingsStore(repo_root=...).purge_unknown()) needs to be wired into the wrapper, tests, docs, and agent config: add a corresponding clean entry in the wrapper CLI module that exposes the same command name, add unit/CLI tests under tests/cli/ to cover both dry-run (when state_dir missing and when unknown items exist) and actual removal behavior asserting purge_unknown() is called and output matches, update the packages/pythinker-review README to document the new clean command (replace or complement the existing clean-locks mention), and add a "clean" command/config entry to the code_reviewer.yaml so the agent can invoke the new CLI command. Ensure tests exercise FindingsStore and purge_unknown paths and the wrapper forwards arguments to the underlying command.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/pythinker-review/src/pythinker_review/cli/review.py`:
- Around line 368-392: The new CLI command implemented as
`@app.command`(name="clean") (which uses
FindingsStore(repo_root=...).purge_unknown()) needs to be wired into the
wrapper, tests, docs, and agent config: add a corresponding clean entry in the
wrapper CLI module that exposes the same command name, add unit/CLI tests under
tests/cli/ to cover both dry-run (when state_dir missing and when unknown items
exist) and actual removal behavior asserting purge_unknown() is called and
output matches, update the packages/pythinker-review README to document the new
clean command (replace or complement the existing clean-locks mention), and add
a "clean" command/config entry to the code_reviewer.yaml so the agent can invoke
the new CLI command. Ensure tests exercise FindingsStore and purge_unknown paths
and the wrapper forwards arguments to the underlying command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dfb920bc-7e2c-4adb-9c53-77a320f1a500
📒 Files selected for processing (6)
packages/pythinker-review/src/pythinker_review/cli/review.pypackages/pythinker-review/src/pythinker_review/security_scan/store.pypackages/pythinker-review/src/pythinker_review/store/findings_store.pytests/tools/test_tool_descriptions.pytests/ui_and_conv/test_shell_design_system.pytests/ui_and_conv/test_tui_card_tool_renderers.py
…onfig - 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
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-06-03-scoped-config.md`:
- Around line 478-482: The error message raised in the validation block that
uses field_path, scope_name and scope_label is hardcoded to provider env var
guidance and should be made dynamic; update the raise ConfigError call so it
detects which locked path is being violated (e.g., "providers", "services", or
"feedback.api_key") and produces an appropriate message—either a generic
instruction pointing to ~/.pythinker/config.toml and environment variables, or
construct the specific env var pattern from the field_path tuple (e.g.,
PYTHINKER_PROVIDER_<NAME>_API_KEY for providers, a different pattern for
services, etc.) so the message matches the actual locked path instead of always
referencing PYTHINKER_PROVIDER_<NAME>_API_KEY.
- Around line 1024-1030: The checklist text is confusingly conflating two
unrelated points; update the note in
docs/superpowers/plans/2026-06-03-scoped-config.md to separately state that
_type_based_merge mutates in-place so the copy module is not required, and that
_apply_env_vars in config.py requires importing os; then ensure the checklist
step only asks to verify that config.py imports os and remove or separate the
unnecessary mention of import copy, referencing the functions _type_based_merge
and _apply_env_vars so reviewers can find the relevant code.
In `@docs/superpowers/specs/2026-06-03-pythinker-scope-config-design.md`:
- Line 17: Several fenced code blocks use plain ``` without language specifiers
and multiple headings lack surrounding blank lines; locate the offending fenced
blocks (search for bare triple-backticks ``` ) and add appropriate language tags
(e.g., ```text, ```python, ```shell) for each block, and ensure every fenced
code block has a blank line before and after it; also locate headings (lines
starting with #) and add a blank line after each heading so following paragraphs
or code blocks are separated.
In `@packages/pythinker-review/README.md`:
- Around line 107-109: The README groups the `clean` command with other
"Reviewflow pure-Python stateful" commands but `clean` operates on a different
state dir; update the text so it's not misleading by either moving `clean` out
of that bullet or adding a clarifying note — specifically mention that `init`,
`map`, `status`, `review`, `ci`, `report`, `show --finding`, `next`, `triage`,
`revalidate`, `fix`, `open-pr`, `doctor`, `clean-locks` operate on
`.pythinker-review-flow/` while the `clean` command targets the diff-save state
in `.pythinker-review/`; adjust the sentence around the `clean` mention
accordingly.
In `@src/pythinker_code/agents/default/code_reviewer.yaml`:
- Line 13: Update the agent spec wording to explicitly mark "pythinker review
clean" as a destructive, runtime-state purge of the .pythinker-review/ directory
and note that the command is reachable from both the standalone Typer app
(registered via `@app.command`(name="clean")) and through the wrapper (delegated
via cli = upstream_review.app), and reaffirm the requirement to run with
--dry-run first; ensure the description parallels existing "mutating" callouts
like fix/open-pr so users understand this is a destructive maintenance
operation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ebd7f550-2ed1-433c-98b0-c3c7d71f1de5
📒 Files selected for processing (5)
docs/superpowers/plans/2026-06-03-scoped-config.mddocs/superpowers/specs/2026-06-03-pythinker-scope-config-design.mdpackages/pythinker-review/README.mdpackages/pythinker-review/tests/unit/test_findings_store.pysrc/pythinker_code/agents/default/code_reviewer.yaml
- set_todo_list.md: add H1 heading (markdownlint MD041) - scoped-config plan: generalize locked-path error message from provider-specific PYTHINKER_PROVIDER_<NAME>_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
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/superpowers/plans/2026-06-03-scoped-config.md (1)
932-933:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove incorrect instruction to import
copy.Line 932 instructs adding
import copy, but the implementation doesn't use it and line 1026 explicitly notes it's not needed. This is contradictory.✂️ Proposed fix
-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: +Add after `_apply_env_vars` in `src/pythinker_code/config.py`:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-06-03-scoped-config.md` around lines 932 - 933, The review points out that adding "import copy" is incorrect and unused; remove the instruction that adds "import copy" at the top of src/pythinker_code/config.py and do not add the import, leaving the implementation of _apply_env_vars unchanged (the note in the file indicates copy is not needed). Ensure no other parts of the diff introduce an unused import for copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/superpowers/plans/2026-06-03-scoped-config.md`:
- Around line 932-933: The review points out that adding "import copy" is
incorrect and unused; remove the instruction that adds "import copy" at the top
of src/pythinker_code/config.py and do not add the import, leaving the
implementation of _apply_env_vars unchanged (the note in the file indicates copy
is not needed). Ensure no other parts of the diff introduce an unused import for
copy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4c19dd95-db59-41da-8d57-fc9f34ac2f56
📒 Files selected for processing (5)
docs/superpowers/plans/2026-06-03-scoped-config.mddocs/superpowers/specs/2026-06-03-pythinker-scope-config-design.mdpackages/pythinker-review/README.mdsrc/pythinker_code/agents/default/code_reviewer.yamlsrc/pythinker_code/tools/todo/set_todo_list.md
Summary
QUESTION_MARKERconstant added toglyphs.pyas the single source of truth. Fixes inconsistency where the inline transcript renderer used●while the interactive question panel used?. All question indicators now render❓(ASCII fallback:?) consistently across every surface: transcript, dialog body, pager, andprompt_other_input._SCRATCHPAD_RECOVERY_NOTEinstructed agents to fast-skim all prior sessions' scratch files on startup, injecting stale planning context from previous sessions; (2) cleanup only ran on successful exit, so files accumulated indefinitely. Now: agents read only their own session's file, andcleanup_session_scratch()fires on all exits (success + interruption).SetTodoListplan-approval gate — Tool description and system prompt updated: set todos only after the user explicitly agrees on the plan. During planning/exploration the tool must not be called. Once set, the list is the single source of truth; only status fields are updated during execution.Root cause
_SCRATCHPAD_RECOVERY_NOTEtold the agent tofast-skim the relevant .pythinker/scratch/*.md labelson every fresh start. With cleanup disabled on interruption, files accumulated — all injected on every new session, causing the agent to see stale planning from previous interrupted sessions and become confused.Test plan
make check-pythinker-codepasses (0 errors, 0 warnings)AskUserQuestionwith multiple questions — verify❓appears consistentlySetTodoListuntil user explicitly approves a planSummary by CodeRabbit
New Features
Updates
Documentation