Skip to content

feat: tracing dashboard redesign and robust project-memory tooling - #127

Merged
elkaix merged 15 commits into
mainfrom
feat/enhancements-and-bug-fixes
Jun 13, 2026
Merged

feat: tracing dashboard redesign and robust project-memory tooling#127
elkaix merged 15 commits into
mainfrom
feat/enhancements-and-bug-fixes

Conversation

@elkaix

@elkaix elkaix commented Jun 13, 2026

Copy link
Copy Markdown
Member

Supersedes #126 (auto-closed when its head branch was renamed to feat/enhancements-and-bug-fixes).

Summary

This branch bundles the tracing-dashboard redesign with a robustness fix for the project-memory tool, plus supporting agent-UX and dependency work.

Project-memory capacity handling

A user hit a loop where the agent thrashed through 5 failed memory writes against a near-full store and had to be interrupted. Root cause: the budget check silently added a 3-char entry delimiter the error never disclosed, so 2085/2200, entry (113) exceeds read as satisfiable (2085+113 < 2200) but wasn't, and the agent had no way to see the true ceiling or what was stored.

  • Delimiter-aware, legible errors: rejections now report exact free chars, the entry's real cost (content + separator), used/limit, and a compact inventory (index, size, preview) so the next remove/replace is guided, not guessed.
  • New read-only list action for mid-session introspection (status()/capacity() on the store).
  • Full-store education: on a capacity rejection the user-facing tool card explains what happened (nothing lost, task continues) and how to free space.
  • Raised limits: MEMORY 2200 to 5000, USER 1375 to 2500 (within the 8 KB injection budget).
  • /memory now shows per-store capacity and a "nearly full" panel at >=85%.

Tracing dashboard redesign

  • New Usage page (GitHub-style activity heatmap + insights panel + area trend chart), richer statistics view, single blue accent, shared metric/card components, soft page background.
  • Daily Usage and trend charts now measure their container and render full width (no letterboxed SVG).
  • Slash completer ranks command-name matches above alias matches (so /report lists /reports first).
  • TUI: diff context lines render in normal text color.
  • MCP skill bridge, agent guardrails, session UX fixes.
  • Frontend deps: react bumped to 19.2.7 in vis/web on vite 8.

Verification

  • New tests pin delimiter-overhead rejection, exact free-space reporting, status/list, replace-overage, the /memory capacity line, and the slash-completer ordering.
  • ruff + format + pyright clean (0 errors); overlap test suites pass; vis typecheck + build green.

Notes / tradeoffs

  • With raised limits, heavily multibyte memory (e.g. Arabic) can exceed the 8 KB injection budget sooner; snapshot() already truncates gracefully and the header points the agent to the files. Left INJECTION_BUDGET_BYTES at 8 KB to avoid inflating every prompt.
  • Charts remain dependency-free SVG (no recharts).

Summary by CodeRabbit

  • New Features

    • Designer-skill MCP bridge + documentation; MCP bridge resolution for missing local skills.
    • /recap on|off toggle (persisted when possible).
    • Always-on best-practices mode at startup.
    • Memory tool: list action and index-based replace/remove.
    • Usage dashboard with heatmap and trend charts.
  • Bug Fixes

    • Improved multi-edit error reporting (no partial application).
    • Stronger evidence/claim guardrails and recap sanitization.
    • Scratch-file cleanup on exceptional session exits.
    • Clearer edited-file diff context coloring.
  • UI/Style Improvements

    • Login/OAuth selector styling and status token refinements.
    • Memory capacity indicators in /memory and improved spinner/status visuals.

elkaix added 8 commits June 13, 2026 08:44
ReadSkill now resolves a connected MCP server name (including plugin-style
aliases like designer-skill:designer-skill) to a bridge listing that server's
tools instead of "Skill not found", and ships a designer-skill stub that routes
frontend work to the MCP tools.

best_practices_always config option folds the full /best-practices engineering
guidance into the root session's system prompt at startup (default off), so the
guardrails apply to every new session without running the command.

StrReplaceFile returns a precise, actionable error when a multi-edit batch fails
schema validation (e.g. entries collapsed by a streaming glitch), naming the bad
entries and steering toward single-edit calls; valid edits are never partially
applied.

The default system prompt now requires absence claims ("no em-dashes", "no
leftover debug", "matches the source") to be backed by an actual zero-hit scan,
and to re-ask rather than act on a self-authored reading of a non-responsive
clarifying answer.

Also: /recap on|off toggle with grey autosuggest; recaps strip <system-reminder>
blocks; configured /login providers get distinct state styling; braille
background spinner and hanging-indent working tips; scratch files are cleaned up
on exception-path session exit. Removes leftover debug instrumentation from the
skill tool.
Unchanged context lines in file-edit diff snippets (Write, StrReplaceFile, and
all diff cards) used a muted grey (tool_diff_context), which read as dimmed
against the normal body text. Match the normal `text` token instead — terminal
default foreground in dark, #213853 in light — so edited-file previews are easy
to read. Added/removed lines keep their green/red styling, so changes still
stand out.
Refresh the Statistics and Sessions surfaces of the agent tracing
visualizer and add a new Usage page, keeping the existing neutral/zinc
identity (no new brand accent) and the dependency-free SVG charts.

- Add shared shadcn-style Card primitives (12px radius, subtle shadow)
- Statistics: icon-tile metric cards with helper lines, titled chart/
  tool/table cards, rounded bars, bordered hover-row project table,
  width-contained layout, and an empty state
- Sessions: focus rings and a search hint in the toolbar, softer card
  radius with hover lift, folder-tile project group headers
- New Usage page: summary cards, a GitHub-style activity heatmap keyed
  on daily turns (monochrome intensity ramp), and a turn-trend chart
- Header: app icon tile, subtitle, accessible theme toggle; add the
  Usage tab as polished pills
- Add a prefers-reduced-motion safety block to global CSS

Behavior, data flow, and DOM/event contracts are unchanged.
Typing /report surfaced /report_error first because its "report" alias
was an exact match, ranked above /reports (a command-name prefix match).
Rank by match tier (name exact, name prefix, alias exact, alias prefix)
then by command-name length, so the closest command name wins. /report
now lists /reports first.

Drop the now-unused _command_lookup and add regression tests.
Apply a soft-enterprise analytics treatment across the tracing
visualizer and make the Usage visualizations feel intentional.

- Introduce a single restrained blue accent (primary/ring tokens) for
  light and dark; charts and the heatmap now carry visual hierarchy
- Shared premium MetricCard (rounded-2xl, icon tile, hover lift); used
  by Statistics and Usage
- Usage: heatmap and a new Usage Insights panel sit side by side to use
  the available width; larger blue GitHub-style heatmap with a
  Turns/Sessions toggle and Less/More legend
- New area trend chart with gradient fill, gridlines, axis labels, and a
  hover tooltip (dependency-free SVG)
- Statistics: single accent tool bars with a neutral error badge instead
  of red segments; blue daily-usage series
- Soft muted page background and consistent rounded-2xl cards

Charts remain hand-rolled SVG (no recharts). Behavior and data flow are
unchanged.
Align the web and vis frontends on the latest stable React (19.2.7,
@types/react 19.2.17) on top of vite 8, and refresh both lockfiles.
Both frontends type-check and build cleanly under vite 8 + react 19.2.7.
The chart used a fixed 600x140 viewBox with maxHeight, so
preserveAspectRatio letterboxed it: the plot rendered ~600px wide and
floated centered in a much wider card. Measure the container width with
a ResizeObserver and render the SVG full width (taller, with gridlines
and a sessions Y axis), matching the Usage trend chart.
…cation

The project-memory budget check silently added a 3-char entry delimiter that
the rejection message never disclosed, so a near-full store reported e.g.
"2085/2200, entry (113) exceeds" — math that reads as satisfiable (2085+113<2200)
but isn't. With no visibility into the true ceiling or what was stored, the agent
could only blind-shrink the entry and loop until interrupted.

- project_memory: delimiter-aware accounting; rejections now report exact free
  chars, the entry's real cost (content + separator), used/limit, and a compact
  inventory (index, size, preview) so the next remove/replace is guided. Add
  status() and capacity(); flag capacity failures via MemoryOpResult.full.
- Memory tool: new read-only `list` action for mid-session introspection; on a
  full-store rejection, append a plain-language explanation (nothing lost, task
  continues, how to free space) to the user-facing tool card.
- Raise limits MEMORY 2200->5000, USER 1375->2500 (within the 8 KB injection budget).
- /memory: show per-store capacity and a "nearly full" guidance panel at >=85%.
- memory.md: best-effort housekeeping guidance — don't loop on rejection.
@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@elkaix, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 44 minutes and 33 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a39d55c8-1a0e-4321-91d2-77e5afd591c5

📥 Commits

Reviewing files that changed from the base of the PR and between dcb1a19 and 317d858.

⛔ Files ignored due to path filters (1)
  • docs/en/customization/architecture.md is excluded by !docs/**
📒 Files selected for processing (4)
  • scripts/build_vis.py
  • src/pythinker_code/cli/__init__.py
  • src/pythinker_code/tools/memory/__init__.py
  • web/AGENTS.md
📝 Walkthrough

Walkthrough

This PR adds an always-on best-practices option, index-aware project memory accounting and gated memory writes, MCP skill-bridge lookup fallback, /recap on|off persistence and sanitization, stricter StrReplace multi-edit validation, shell/autocomplete and selector styling tweaks, and a new usage analytics frontend with charts and tests.

Changes

Main Feature Integration

Layer / File(s) Summary
Best-practices always-on configuration
src/pythinker_code/config.py, src/pythinker_code/prompts/__init__.py, src/pythinker_code/app.py, src/pythinker_code/agents/default/system.md, tests/core/test_best_practices_slash.py, CHANGELOG.md
Adds Config.best_practices_always, apply_always_on_best_practices(...), startup wiring to inject guidance into session system prompt when enabled, and guardrail wording refinements.
Recap toggle persistence & sanitization
src/pythinker_code/soul/slash.py, src/pythinker_code/session_recap.py, src/pythinker_code/ui/shell/__init__.py, src/pythinker_code/ui/shell/prompt.py, tests/core/test_slash_recap.py, tests/ui_and_conv/test_settings_recaps_slash.py
/recap accepts on/off, updates runtime.config.tui.turn_recaps, persists to config file when present (reverts on failure), filters <system-reminder> blocks from recap input, and wires recap state into prompt exact suggestions.
Project memory accounting & index-based ops
src/pythinker_code/project_memory.py, tests/core/test_project_memory.py
Adds used/overhead accounting helpers, inventory snapshots, raises char limits, adds _locate() and index-based replace/remove, and enriches capacity/status responses.
Memory tool list/index and gated writes
src/pythinker_code/tools/memory/__init__.py, src/pythinker_code/tools/memory/routing_guard.py, src/pythinker_code/tools/memory/memory.md, tests/tools/test_memory_tool.py, tests/tools/test_memory_routing_guard.py
Memory Params adds list and index; Memory supports list, threads index to store methods, appends actionable free-space instructions on full, and gates flagged writes via routing_signals + runtime approval. Memory policy doc updated.
Skill MCP bridge and ReadSkill fallback
src/pythinker_code/tools/skill/_mcp_bridge.py, src/pythinker_code/tools/skill/__init__.py, src/pythinker_code/skills/designer-skill/SKILL.md, tests/tools/test_mcp_skill_bridge.py, tests/tools/test_skill_tool.py, tests/utils/test_pyinstaller_utils.py, tests_e2e/test_wire_protocol.py
Adds skill_lookup_keys, MCP-server indexer, find_mcp_server_for_skill_name, and mcp_skill_bridge_content; ReadSkill now searches multiple keys and falls back to MCP bridge content when no local skill exists. Designer-skill doc and tests added; packaging/wire snapshots updated.
StrReplaceFile malformed batch handling
src/pythinker_code/tools/file/replace.py, tests/tools/test_str_replace_file.py
Adds _malformed_edit_batch_error(...) and an overridden call() that prefers the targeted malformed-batch ToolError to avoid partial application on multi-edit failures.
CLI exception-path scratch cleanup
src/pythinker_code/cli/__init__.py, tests/core/test_session.py
On unexpected exceptions, attempts best-effort scratchpad cleanup for the latest session and centralizes test helper _simulate_exception_cleanup(...) to assert scratch removal behavior.
Shell autocomplete, selector styling, spinner & live-view tip
src/pythinker_code/ui/shell/prompt.py, src/pythinker_code/ui/shell/selector.py, src/pythinker_code/ui/shell/selectors/oauth.py, src/pythinker_code/ui/shell/slash.py, src/pythinker_code/ui/shell/visualize/_live_view.py, src/pythinker_code/ui/theme.py, tests under tests/ui_and_conv/
Adds exact inline suggestions callback, tiered completer matching (including alias-label display), turn_recaps_provider wiring, selector split-description styling, OAuth provider status styling, spinner rendering adjustments, working-tip rendering via render_message_response(...), theme token tweaks, and matching tests.
Frontend usage analytics and statistics redesign
vis/src/features/usage/*, vis/src/components/*, vis/src/features/statistics/statistics-view.tsx, vis/src/App.tsx, vis/src/index.css, vis/package.json, web/package.json, tests under tests/ui_and_conv/
Adds UsageView with heatmap and trend chart components, MetricCard and Card UI primitives, integrates a Usage explorer tab, redesigns Statistics view to responsive SVG charts and card-based layout, updates theme tokens and reduced-motion rule, bumps React/TypeScript deps, and adds UI tests.
Test & snapshot updates
tests/core/, tests/tools/, tests/ui_and_conv/, tests/utils/, tests_e2e/
Extensive tests added/updated for best-practices application, memory accounting and gating, MCP bridge behavior, recap toggles, StrReplace validation, shell autocomplete/selector behavior, live-view tip wrapping, and packaging/wire snapshots.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

enhancement, dependencies

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/enhancements-and-bug-fixes

@elkaix elkaix changed the title Tracing dashboard redesign + robust project-memory capacity handling feat: tracing dashboard redesign and robust project-memory tooling Jun 13, 2026
elkaix added 2 commits June 13, 2026 09:55
Add a structural detector that nudges the agent toward editing the
authoritative project file when a memory write looks like a rule or
value-assignment that belongs in a file (a recurring failure mode: a
correction rephrased as a "preference" and stored in memory while the
governing file stays stale). The guard never blocks — it only appends a
one-line advisory on add/replace — so false positives cost a sentence.

Also let replace/remove identify an entry by 0-based `index` (from
`list`) as a deterministic alternative to `old_text` substring matching;
out-of-range indices report the inventory so the retry is guided.

Slash completer now surfaces the matched alias as the menu label
(`/res` -> `/resume`) while keeping name matches ranked above alias-only
matches.

memory.md guidance updated for authoritative-files-first and the new
index-based locator.
Regenerate the inline snapshots that pin auto-discovered state so they
match the new code:
- pyinstaller `hiddenimports` now includes the new
  `pythinker_code.tools.memory.routing_guard` module.
- wire-handshake slash-command list: `/recap` usage gained `on|off` and
  the `skill:designer-skill` command was added.

Make the recap slash-command test await type-safe: the registry types
commands as `None | Awaitable[None]`, so guard with `isinstance(ret,
Awaitable)` before awaiting (matching the existing shell-slash test
helper) instead of awaiting the union directly, which pyright rejects.
Comment thread tests/core/test_slash_recap.py Fixed
@codecov

codecov Bot commented Jun 13, 2026

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/pythinker_code/ui/shell/prompt.py (1)

2198-2205: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Exact /recap ghost text is shadowed by the slash menu.

When the buffer contains /recap at line start, SlashCommandCompleter still yields a /recap completion, Lines 2594-2606 auto-select it, and the Enter binding on Lines 2238-2242 accepts that menu item before submit. So the new exact_suggestions hook never drives the common /recap → /recap on|off flow. Either suppress slash completions once an exact suggestion exists, or have the Enter path prefer buffer.suggestion first.

🤖 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 `@src/pythinker_code/ui/shell/prompt.py` around lines 2198 - 2205, The ghost
`/recap` suggestion is being overridden by the slash menu; fix by preferring the
exact auto-suggestion or suppressing slash completions: update the Enter/submit
handler to check buffer.suggestion first and accept it if present (before
calling the code that accepts a SlashCommandCompleter menu entry), and/or modify
SlashCommandCompleter (or SlashCommandAutoSuggest usage) to skip emitting slash
completions when exact_suggestions (the callable passed to
SlashCommandAutoSuggest) returns a non-empty exact match. Target the Enter
binding/submit routine and the SlashCommandAutoSuggest/SlashCommandCompleter
logic so that buffer.suggestion is accepted first or slash completions are
suppressed when an exact suggestion exists.
🤖 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/cli/__init__.py`:
- Around line 1206-1213: The cleanup block currently uses
contextlib.suppress(Exception) which silently hides import or runtime errors;
change it to an explicit try/except around the import and await
cleanup_session_scratch call, catch Exception as e, and log the failure at debug
level (include exc_info/stacktrace) while preserving best-effort semantics (do
not re-raise). Target the block that imports pythinker_code.scratchpad and calls
cleanup_session_scratch with _latest_created_session.work_dir,
session_id=_latest_created_session.id,
session_title=_latest_created_session.title, and ensure both import failures and
runtime failures are logged via the module logger (use existing logger variable)
so failures are visible without altering behavior.

In `@src/pythinker_code/soul/slash.py`:
- Around line 77-95: The code sets soul.runtime.config.tui.turn_recaps before
attempting persistence, so if load_config/save_config (functions load_config,
save_config) raise ConfigError/OSError the in-memory state remains changed;
modify the handler to capture the previous value (prev =
soul.runtime.config.tui.turn_recaps) before changing, then set
soul.runtime.config.tui.turn_recaps = enabled, attempt load_config/save_config,
and in the except block revert soul.runtime.config.tui.turn_recaps = prev and
still call wire_send(TextPart(...)) to report the failure; ensure the revert
happens for all persistence failures so runtime state matches the reported
outcome.

In `@src/pythinker_code/tools/memory/__init__.py`:
- Around line 90-93: The hardcoded full-store message always says “Project
memory … full” even when the store target is "user"; update the code that builds
the full-store guidance (the branch that references target and returns the
multi-line string) to choose target-specific wording: when target == "project"
keep the existing "Project memory…" text, when target == "user" change to "User
memory for this repo is full…" (or similar user-target phrasing), or build the
message from a small template that inserts the capitalized target name into the
sentence; ensure the conditional/template is used where the current multi-line
string is returned so the message accurately reflects the target variable.

In `@tests/core/test_session.py`:
- Around line 687-689: Rename the unused test parameter isolated_share_dir to
_isolated_share_dir in the two test function signatures that were modified (the
occurrences at the other nearby test as well) so Ruff ARG001 no longer flags it;
alternatively, add an explicit noqa comment to the parameter if you prefer to
keep the name. Ensure you update the parameter in both places where
isolated_share_dir was added (the two test definitions) and do not change any
other logic or uses (this parameter is only for fixture side-effects).

In `@tests/ui_and_conv/test_settings_recaps_slash.py`:
- Around line 62-80: The test test_recap_singular_on_persists_and_reloads
currently mocks shell_slash.save_config but doesn't assert it was invoked;
update the test to verify persistence by asserting the mocked save function was
called (e.g., assert shell_slash.save_config.called or assert
shell_slash.save_config.call_count == 1) after the Reload is raised and after
asserting config_for_save.tui.turn_recaps is True; reference the mocked symbol
shell_slash.save_config and the test function name when making the change.

---

Outside diff comments:
In `@src/pythinker_code/ui/shell/prompt.py`:
- Around line 2198-2205: The ghost `/recap` suggestion is being overridden by
the slash menu; fix by preferring the exact auto-suggestion or suppressing slash
completions: update the Enter/submit handler to check buffer.suggestion first
and accept it if present (before calling the code that accepts a
SlashCommandCompleter menu entry), and/or modify SlashCommandCompleter (or
SlashCommandAutoSuggest usage) to skip emitting slash completions when
exact_suggestions (the callable passed to SlashCommandAutoSuggest) returns a
non-empty exact match. Target the Enter binding/submit routine and the
SlashCommandAutoSuggest/SlashCommandCompleter logic so that buffer.suggestion is
accepted first or slash completions are suppressed when an exact suggestion
exists.
🪄 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: 06fe983c-efb4-4e7b-99cd-457f58fb0d11

📥 Commits

Reviewing files that changed from the base of the PR and between d020e4e and b1f32ca.

⛔ Files ignored due to path filters (2)
  • vis/package-lock.json is excluded by !**/package-lock.json
  • web/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (59)
  • CHANGELOG.md
  • src/pythinker_code/agents/default/system.md
  • src/pythinker_code/app.py
  • src/pythinker_code/cli/__init__.py
  • src/pythinker_code/config.py
  • src/pythinker_code/project_memory.py
  • src/pythinker_code/prompts/__init__.py
  • src/pythinker_code/session_recap.py
  • src/pythinker_code/skills/designer-skill/SKILL.md
  • src/pythinker_code/soul/slash.py
  • src/pythinker_code/tools/file/replace.py
  • src/pythinker_code/tools/memory/__init__.py
  • src/pythinker_code/tools/memory/memory.md
  • src/pythinker_code/tools/memory/routing_guard.py
  • src/pythinker_code/tools/skill/__init__.py
  • src/pythinker_code/tools/skill/_mcp_bridge.py
  • src/pythinker_code/tools/skill/description.md
  • src/pythinker_code/ui/shell/__init__.py
  • src/pythinker_code/ui/shell/prompt.py
  • src/pythinker_code/ui/shell/selector.py
  • src/pythinker_code/ui/shell/selectors/oauth.py
  • src/pythinker_code/ui/shell/slash.py
  • src/pythinker_code/ui/shell/visualize/_live_view.py
  • src/pythinker_code/ui/theme.py
  • tests/core/test_best_practices_slash.py
  • tests/core/test_builtin_authoring_skills.py
  • tests/core/test_config.py
  • tests/core/test_project_memory.py
  • tests/core/test_session.py
  • tests/core/test_slash_recap.py
  • tests/test_session_recap.py
  • tests/tools/test_mcp_skill_bridge.py
  • tests/tools/test_memory_routing_guard.py
  • tests/tools/test_memory_tool.py
  • tests/tools/test_skill_tool.py
  • tests/tools/test_str_replace_file.py
  • tests/ui_and_conv/test_live_view_notifications.py
  • tests/ui_and_conv/test_memory_slash.py
  • tests/ui_and_conv/test_prompt_tips.py
  • tests/ui_and_conv/test_selectors_simple.py
  • tests/ui_and_conv/test_settings_recaps_slash.py
  • tests/ui_and_conv/test_slash_completer.py
  • tests/ui_and_conv/test_visualize_running_prompt.py
  • tests/utils/test_pyinstaller_utils.py
  • tests_e2e/test_wire_protocol.py
  • vis/package.json
  • vis/src/App.tsx
  • vis/src/components/metric-card.tsx
  • vis/src/components/ui/card.tsx
  • vis/src/features/sessions-explorer/explorer-toolbar.tsx
  • vis/src/features/sessions-explorer/project-group.tsx
  • vis/src/features/sessions-explorer/session-card.tsx
  • vis/src/features/sessions-explorer/sessions-explorer.tsx
  • vis/src/features/statistics/statistics-view.tsx
  • vis/src/features/usage/usage-heatmap.tsx
  • vis/src/features/usage/usage-trend-chart.tsx
  • vis/src/features/usage/usage-view.tsx
  • vis/src/index.css
  • web/package.json

Comment thread src/pythinker_code/cli/__init__.py Outdated
Comment thread src/pythinker_code/soul/slash.py
Comment thread src/pythinker_code/tools/memory/__init__.py Outdated
Comment thread tests/core/test_session.py Outdated
Comment thread tests/ui_and_conv/test_settings_recaps_slash.py
The react 19.2.7 / vite 8.0.16 bump left both lockfiles inconsistent:
they were missing the platform-specific optional @emnapi/* wasm-binding
deps (oxc-parser, oxc-resolver, rolldown). Because those are optional
per-OS deps, a partial `--package-lock-only` regen on one platform does
not capture the set CI's Linux runner needs, so `npm ci` kept failing
with EUSAGE ("package.json and package-lock.json are not in sync") and
broke `make build-web` / build-vis in the PyInstaller onefile jobs.

Regenerate both lockfiles from a clean install so they carry the full
cross-platform optional-dep tree. Direct deps are unchanged (react
19.2.7, vite 8.0.16, typescript 5.9.3); the churn is transitive/optional
ordering. Verified `npm ci` succeeds and `build-web`/`build-vis` produce
assets in both web/ and vis/.
@elkaix
elkaix force-pushed the feat/enhancements-and-bug-fixes branch from b1f32ca to eaee384 Compare June 13, 2026 14:19
elkaix added 3 commits June 13, 2026 10:43
- cli: replace silent contextlib.suppress(Exception) in the exception-path
  session cleanup with explicit try/except that logs at debug, so import or
  runtime failures during best-effort cleanup are traceable (CodeRabbit).
- slash (/recap): revert the in-memory turn_recaps toggle when persistence
  fails, so runtime state matches the reported failure instead of silently
  diverging (CodeRabbit, major).
- test_slash_recap: await the recap command via a type-honest cast instead of
  a bare `await ret` (clears the "statement has no effect" code-scanning flag).
- test_session: silence ARG001 on the three exception-cleanup tests with
  @pytest.mark.usefixtures (renaming the param to _isolated_share_dir would
  break pytest fixture injection).
- test_settings_recaps_slash: assert save_config is actually called in the
  singular `/recap on` alias test (CodeRabbit).
When an add/replace looks like a rule, a value/limit, or names a project file
(the existing routing_guard signals), gate it behind user confirmation before
writing — so the agent can't silently fill memory with corrections that belong
in an authoritative file. Plain durable facts (no signals) still write directly.

Uses the shared approval flow (Runtime.approval), so yolo/auto-approve and
"allow for session" are honored automatically:
- approved -> the entry is written;
- declined by the user -> not written; the agent is told to edit the governing
  file instead, with the user's feedback;
- no user available (headless) -> skipped cleanly, reported as skipped (not an
  error), so unattended runs never accumulate flagged memory.

This replaces the weaker post-write advisory nudge with a real gate. Also makes
the full-store guidance target-specific ("Project memory" vs "User memory")
instead of always saying "Project memory" (CodeRabbit).
Regenerating web/package-lock.json bumped ultracite within `^7.1.1` to 7.8.3,
which restructured its package exports and dropped `ultracite/core` —
web/biome.json extends `ultracite/core`, so `make check-web` (biome) failed
with "Could not resolve ultracite/core". Pin ultracite to exactly 7.1.1 (the
version main uses) until biome.json is migrated to newer ultracite. The
cross-platform @emnapi lockfile fix is preserved; `npm ci`, biome, and the web
build all pass.
@elkaix

elkaix commented Jun 13, 2026

Copy link
Copy Markdown
Member Author

Addressed the review feedback in 75f1927, f5eb04e, dcb1a19:

  • cli silent suppression → explicit try/except logging at debug.
  • /recap toggle → reverts in-memory state when persistence fails.
  • memory full-store text → target-specific ("Project memory" / "User memory").
  • test_session ARG001 → fixed via @pytest.mark.usefixtures("isolated_share_dir"). (The suggested rename to _isolated_share_dir would break pytest fixture injection, since pytest matches fixtures by exact parameter name.)
  • test_settings_recaps_slash → now asserts save_config was called.
  • test_slash_recap "statement has no effect" → reworked to await cast(Awaitable[None], recap_slash(...)).

Not changed (with reason): the /recap ghost-text "shadowed by slash menu" note. This is working as designed: the exact suggestion is prompt_toolkit auto-suggest (ghost text), accepted via Tab/→/Ctrl-E, while Enter accepting the slash-completion menu is the universal behavior for every command. Making Enter prefer the suggestion over the menu would diverge from all other commands and risk the test-pinned submit path, for a minor cosmetic gain (/recap already submits a valid recap). Happy to revisit if you want the on/off toggle to be the default Enter action.

@elkaix

elkaix commented Jun 13, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/core/test_slash_recap.py (1)

35-75: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Add a regression test for persistence failure rollback in /recap on.

Current coverage validates success and session-only behavior, but it doesn’t assert the rollback path when load_config/save_config fails. Please add one negative-path test to lock in the Line 95-97 revert behavior.

Proposed test shape
+from pythinker_code.config import ConfigError, get_default_config
@@
+async def test_recap_on_reverts_runtime_when_save_fails(
+    runtime: Runtime, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    config_path = (tmp_path / "config.toml").resolve()
+    runtime.config.source_file = config_path
+    runtime.config.tui.turn_recaps = False
+    soul = _make_soul(runtime, tmp_path)
+    sent: list[TextPart] = []
+
+    monkeypatch.setattr(
+        "pythinker_code.soul.slash.load_config",
+        Mock(side_effect=ConfigError("bad config")),
+    )
+    monkeypatch.setattr("pythinker_code.soul.slash.wire_send", lambda msg: sent.append(msg))
+
+    await _run_recap(soul, "on")
+
+    assert runtime.config.tui.turn_recaps is False
+    assert any("Failed to save recap setting" in msg.text for msg in sent)

As per coding guidelines, “Reject or flag … tests covering only happy paths (C14).”

🤖 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 `@tests/core/test_slash_recap.py` around lines 35 - 75, Add a negative-path
test to tests/core/test_slash_recap.py that exercises rollback when persistence
fails: create a test similar to test_recap_on_persists_and_updates_runtime but
set runtime.config.source_file to a path and initial
runtime.config.tui.turn_recaps = False, monkeypatch
pythinker_code.soul.slash.load_config to return config_for_save and monkeypatch
pythinker_code.soul.slash.save_config to raise an Exception (or mock that
raises), monkeypatch wire_send to capture sent messages, call await
_run_recap(soul, "on"), then assert that runtime.config.tui.turn_recaps was
reverted back to False, that save_config was invoked (assert called_once), that
config_for_save.tui.turn_recaps was not left True (or reverted), and that
wire_send produced an error message (assert any("failed" in msg.text.lower() or
"could not" in msg.text.lower() for msg in sent)).

Source: Coding guidelines

🤖 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/cli/__init__.py`:
- Around line 1206-1221: Remove the duplicate local import of logger inside the
try block: delete the line "from pythinker_code.utils.logging import logger" and
let the existing logger from the outer scope be used; keep the local import of
cleanup_session_scratch and the try/except unchanged so the await
cleanup_session_scratch(...) call and the logger.opt(exception=True).debug(...)
line reference the already-in-scope logger.

In `@src/pythinker_code/tools/memory/__init__.py`:
- Around line 116-118: The debug log in the memory approval gate currently logs
content preview (content[:60]) which may leak secrets/PII; update the logger
usage in the memory approval flow (the logger.bind(...).debug call) to remove
the raw content preview and log only structural metadata (e.g., target and
signals) or a constant redaction marker (e.g., "<REDACTED_PREVIEW>") instead;
ensure no other code in the same approval gate emits content before store-level
screening.

In `@web/package.json`:
- Line 84: The devDependency "ultracite" was pinned to an exact version
("7.1.1") which blocks automatic patch updates; revert it to a semver range by
changing the version string for "ultracite" in package.json back to "^7.1.1" (or
another appropriate caret/range) so dev tooling can receive patch/security fixes
automatically.

---

Outside diff comments:
In `@tests/core/test_slash_recap.py`:
- Around line 35-75: Add a negative-path test to tests/core/test_slash_recap.py
that exercises rollback when persistence fails: create a test similar to
test_recap_on_persists_and_updates_runtime but set runtime.config.source_file to
a path and initial runtime.config.tui.turn_recaps = False, monkeypatch
pythinker_code.soul.slash.load_config to return config_for_save and monkeypatch
pythinker_code.soul.slash.save_config to raise an Exception (or mock that
raises), monkeypatch wire_send to capture sent messages, call await
_run_recap(soul, "on"), then assert that runtime.config.tui.turn_recaps was
reverted back to False, that save_config was invoked (assert called_once), that
config_for_save.tui.turn_recaps was not left True (or reverted), and that
wire_send produced an error message (assert any("failed" in msg.text.lower() or
"could not" in msg.text.lower() for msg in sent)).
🪄 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: c703c9e0-8857-4114-bec3-9d9a2be85e86

📥 Commits

Reviewing files that changed from the base of the PR and between b1f32ca and dcb1a19.

⛔ Files ignored due to path filters (2)
  • vis/package-lock.json is excluded by !**/package-lock.json
  • web/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (9)
  • src/pythinker_code/cli/__init__.py
  • src/pythinker_code/soul/slash.py
  • src/pythinker_code/tools/memory/__init__.py
  • src/pythinker_code/tools/memory/memory.md
  • tests/core/test_session.py
  • tests/core/test_slash_recap.py
  • tests/tools/test_memory_tool.py
  • tests/ui_and_conv/test_settings_recaps_slash.py
  • web/package.json

Comment thread src/pythinker_code/cli/__init__.py Outdated
Comment thread src/pythinker_code/tools/memory/__init__.py
Comment thread web/package.json
Comment thread web/package.json
- memory gate: stop logging the raw content preview (content[:60]) in the
  routing-flagged debug log — a declined/blocked write could otherwise leak
  secrets/PII; log only target + signals (CodeRabbit, major).
- cli: drop the redundant local `logger` import in the exception-path cleanup;
  the enclosing function already binds `logger` in scope (CodeRabbit).
- docs/build: the react bump moved web/vis to Vite 8, but build_vis.py, web/
  AGENTS.md, and architecture.md still said "Vite 7". Update the labels (the
  Node engine constraint ^20.19.0 || >=22.12.0 is unchanged in Vite 8).
@elkaix
elkaix merged commit 58c710c into main Jun 13, 2026
36 checks passed
@elkaix
elkaix deleted the feat/enhancements-and-bug-fixes branch June 13, 2026 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant