Tracing dashboard redesign + robust project-memory capacity handling - #126
Closed
elkaix wants to merge 8 commits into
Closed
Tracing dashboard redesign + robust project-memory capacity handling#126elkaix wants to merge 8 commits into
elkaix wants to merge 8 commits into
Conversation
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.
Contributor
📝 WalkthroughWalkthroughThis PR introduces best-practices auto-injection via config flag, expands memory capacity reporting with status/inventory, adds MCP-skill fallback resolution, improves batch validation errors, implements ChangesConfiguration, Memory Capacity & Recap Toggle
MCP Skill Bridge & ReadSkill Fallback
StrReplaceFile Batch Validation & UI Enhancements
Session Scratch Cleanup & Recap Sanitization
System Prompt Guardrails & Changelog
Frontend: Usage Dashboard & Statistics Refactor
Dependencies & Build Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
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/app.py (1)
355-377:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBest-practices modification is lost on resumed sessions.
When a resumed session has a saved
context.system_prompt(line 368), the agent replacement at line 375 usesrefresh_system_prompt_scratchpad_section(context.system_prompt, ...), which operates on the old context prompt—discarding the best-practices enhancement applied at line 360.Result:
best_practices_alwaysonly works for new sessions, not resumed ones.🔧 Proposed fix: apply best practices after context restoration
Move the best-practices application after the context restoration block so it applies to both new and resumed sessions:
agent = await load_agent( agent_file, runtime, mcp_configs=mcp_configs or [], start_mcp_loading=not defer_mcp_loading, ) - - if runtime.config.best_practices_always: - from pythinker_code.prompts import apply_always_on_best_practices - - agent = dataclasses.replace( - agent, - system_prompt=apply_always_on_best_practices(agent.system_prompt, enabled=True), - ) if startup_progress is not None: startup_progress("Restoring conversation...") context = Context(session.context_file) await context.restore() if context.system_prompt is not None: from pythinker_code.scratchpad import refresh_system_prompt_scratchpad_section refreshed_system_prompt = refresh_system_prompt_scratchpad_section( context.system_prompt, runtime.builtin_args.PYTHINKER_SCRATCHPAD_SECTION, ) agent = dataclasses.replace(agent, system_prompt=refreshed_system_prompt) else: await context.write_system_prompt(agent.system_prompt) + + if runtime.config.best_practices_always: + from pythinker_code.prompts import apply_always_on_best_practices + + agent = dataclasses.replace( + agent, + system_prompt=apply_always_on_best_practices(agent.system_prompt, enabled=True), + ) + # Update context so the enhanced prompt is persisted + await context.write_system_prompt(agent.system_prompt) soul = PythinkerSoul(agent, context=context)This ensures the best-practices text is applied to the final system prompt (whether from agent spec or restored context) and persisted for future resumes.
🤖 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/app.py` around lines 355 - 377, The best-practices augmentation (runtime.config.best_practices_always + apply_always_on_best_practices and dataclasses.replace of agent.system_prompt) must be applied after restoring/refreshing the Context so resumed sessions keep the enhancement; move the runtime.config.best_practices_always block to follow the Context(session.context_file); await context.restore(); logic and after refresh_system_prompt_scratchpad_section/context.write_system_prompt so you call apply_always_on_best_practices on the final system prompt (whether refreshed from context or the agent spec) and then dataclasses.replace the agent with that resulting system_prompt (and persist it via context.write_system_prompt if needed).
🤖 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: Replace the silent contextlib.suppress around the
cleanup call so failures are still best-effort but logged: instead of "with
contextlib.suppress(Exception): await cleanup_session_scratch(...)" wrap the
await in a try/except Exception as e and log the exception (e.g., via
logging.getLogger(__name__).exception(...) or the module's existing logger)
including context (_latest_created_session.id, .work_dir, .title) so
cleanup_session_scratch failures are recorded for diagnosis.
In `@src/pythinker_code/tools/skill/_mcp_bridge.py`:
- Around line 37-67: The candidate-extraction logic in
find_mcp_server_for_skill_name duplicates skill_lookup_keys; replace lines that
build candidates (the raw/suffix/prefix block) with a call to
skill_lookup_keys(skill_name) to obtain the candidate list, then iterate that
list (normalizing with normalize_skill_name as before) while using
_index_mcp_servers(mcp_tools) to find a matching server; keep the early return
of server, tools and the seen dedup set logic if needed, and leave
_index_mcp_servers and normalize_skill_name calls intact.
In `@tests/core/test_session.py`:
- Around line 686-689: The ARG001 lint warning is triggered because the test
fixture parameter isolated_share_dir is unused; rename the parameter to
_isolated_share_dir in the test_exception_cleanup_removes_session_scratch_file
test to acknowledge the fixture side effects without using it, and apply the
same rename to the other new test that also declares isolated_share_dir so both
tests compile lint-clean (update the function signatures only, e.g., def
test_exception_cleanup_removes_session_scratch_file(... isolated_share_dir:
Path, ...) -> ... to ... _isolated_share_dir: Path ...).
- Around line 653-669: The helper _simulate_exception_cleanup currently calls
await session.delete() unprotected, diverging from the CLI which suppresses
deletion failures; modify _simulate_exception_cleanup so that the await
session.delete() call is wrapped in contextlib.suppress(Exception) (reuse the
existing contextlib import) so both cleanup_session_scratch and the
session.delete() step suppress exceptions and mirror the CLI behavior; keep
checks using session.is_empty() and ensure no other behavior changes.
In `@tests/ui_and_conv/test_settings_recaps_slash.py`:
- Around line 62-80: The test mutates config_for_save but doesn't assert that
the persistence path was invoked; update
test_recap_singular_on_persists_and_reloads to assert the save call on the
mocked save_config the same way the plural-form test does: verify
shell_slash.save_config was called (e.g., assert called_once) and that it was
invoked with the expected arguments (the config file
path/runtime.config.source_file and config_for_save) so the test ensures
persistence, not just in-memory mutation.
In `@tests/ui_and_conv/test_visualize_running_prompt.py`:
- Around line 396-399: Add explicit return type annotations (-> str) to the test
stub methods in class _BlockingTaskOutputDelegate: change the signatures of
render_agent_status(self, columns: int) and render_pinned_status_tail(self,
columns: int) to include -> str so they read render_agent_status(self, columns:
int) -> str and render_pinned_status_tail(self, columns: int) -> str.
---
Outside diff comments:
In `@src/pythinker_code/app.py`:
- Around line 355-377: The best-practices augmentation
(runtime.config.best_practices_always + apply_always_on_best_practices and
dataclasses.replace of agent.system_prompt) must be applied after
restoring/refreshing the Context so resumed sessions keep the enhancement; move
the runtime.config.best_practices_always block to follow the
Context(session.context_file); await context.restore(); logic and after
refresh_system_prompt_scratchpad_section/context.write_system_prompt so you call
apply_always_on_best_practices on the final system prompt (whether refreshed
from context or the agent spec) and then dataclasses.replace the agent with that
resulting system_prompt (and persist it via context.write_system_prompt if
needed).
🪄 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: 72b2bf36-a29e-4e52-823c-e2f3b813f276
⛔ Files ignored due to path filters (2)
vis/package-lock.jsonis excluded by!**/package-lock.jsonweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (56)
CHANGELOG.mdsrc/pythinker_code/agents/default/system.mdsrc/pythinker_code/app.pysrc/pythinker_code/cli/__init__.pysrc/pythinker_code/config.pysrc/pythinker_code/project_memory.pysrc/pythinker_code/prompts/__init__.pysrc/pythinker_code/session_recap.pysrc/pythinker_code/skills/designer-skill/SKILL.mdsrc/pythinker_code/soul/slash.pysrc/pythinker_code/tools/file/replace.pysrc/pythinker_code/tools/memory/__init__.pysrc/pythinker_code/tools/memory/memory.mdsrc/pythinker_code/tools/skill/__init__.pysrc/pythinker_code/tools/skill/_mcp_bridge.pysrc/pythinker_code/tools/skill/description.mdsrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/selector.pysrc/pythinker_code/ui/shell/selectors/oauth.pysrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/visualize/_live_view.pysrc/pythinker_code/ui/theme.pytests/core/test_best_practices_slash.pytests/core/test_builtin_authoring_skills.pytests/core/test_config.pytests/core/test_project_memory.pytests/core/test_session.pytests/core/test_slash_recap.pytests/test_session_recap.pytests/tools/test_mcp_skill_bridge.pytests/tools/test_memory_tool.pytests/tools/test_skill_tool.pytests/tools/test_str_replace_file.pytests/ui_and_conv/test_live_view_notifications.pytests/ui_and_conv/test_memory_slash.pytests/ui_and_conv/test_prompt_tips.pytests/ui_and_conv/test_selectors_simple.pytests/ui_and_conv/test_settings_recaps_slash.pytests/ui_and_conv/test_slash_completer.pytests/ui_and_conv/test_visualize_running_prompt.pytests/utils/test_pyinstaller_utils.pyvis/package.jsonvis/src/App.tsxvis/src/components/metric-card.tsxvis/src/components/ui/card.tsxvis/src/features/sessions-explorer/explorer-toolbar.tsxvis/src/features/sessions-explorer/project-group.tsxvis/src/features/sessions-explorer/session-card.tsxvis/src/features/sessions-explorer/sessions-explorer.tsxvis/src/features/statistics/statistics-view.tsxvis/src/features/usage/usage-heatmap.tsxvis/src/features/usage/usage-trend-chart.tsxvis/src/features/usage/usage-view.tsxvis/src/index.cssweb/package.json
Comment on lines
+1206
to
+1213
| with contextlib.suppress(Exception): | ||
| from pythinker_code.scratchpad import cleanup_session_scratch | ||
|
|
||
| await cleanup_session_scratch( | ||
| _latest_created_session.work_dir, | ||
| session_id=_latest_created_session.id, | ||
| session_title=_latest_created_session.title, | ||
| ) |
Contributor
There was a problem hiding this comment.
Log best-effort cleanup failures instead of fully suppressing them.
At Line 1206, exceptions from cleanup_session_scratch(...) are swallowed without any trace. Keep it best-effort, but log failures so repeated scratch-cleanup regressions are diagnosable.
Proposed fix
- with contextlib.suppress(Exception):
- from pythinker_code.scratchpad import cleanup_session_scratch
-
- await cleanup_session_scratch(
- _latest_created_session.work_dir,
- session_id=_latest_created_session.id,
- session_title=_latest_created_session.title,
- )
+ try:
+ from pythinker_code.scratchpad import cleanup_session_scratch
+
+ await cleanup_session_scratch(
+ _latest_created_session.work_dir,
+ session_id=_latest_created_session.id,
+ session_title=_latest_created_session.title,
+ )
+ except Exception:
+ logger.debug(
+ "Best-effort scratch cleanup failed for session {session_id}",
+ session_id=_latest_created_session.id,
+ exc_info=True,
+ )As per coding guidelines, exception handlers that silently swallow errors should be logged or re-raised.
🤖 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/cli/__init__.py` around lines 1206 - 1213, Replace the
silent contextlib.suppress around the cleanup call so failures are still
best-effort but logged: instead of "with contextlib.suppress(Exception): await
cleanup_session_scratch(...)" wrap the await in a try/except Exception as e and
log the exception (e.g., via logging.getLogger(__name__).exception(...) or the
module's existing logger) including context (_latest_created_session.id,
.work_dir, .title) so cleanup_session_scratch failures are recorded for
diagnosis.
Source: Coding guidelines
Comment on lines
+37
to
+67
| def find_mcp_server_for_skill_name( | ||
| skill_name: str, | ||
| mcp_tools: Mapping[str, object], | ||
| ) -> tuple[str, list[str]] | None: | ||
| """Match a skill name (or plugin alias) to a connected MCP server.""" | ||
| servers = _index_mcp_servers(mcp_tools) | ||
| if not servers: | ||
| return None | ||
|
|
||
| candidates: list[str] = [] | ||
| raw = skill_name.strip() | ||
| if raw: | ||
| candidates.append(raw) | ||
| if ":" in raw: | ||
| suffix = raw.rsplit(":", 1)[-1].strip() | ||
| if suffix: | ||
| candidates.append(suffix) | ||
| prefix = raw.split(":", 1)[0].strip() | ||
| if prefix: | ||
| candidates.append(prefix) | ||
|
|
||
| seen: set[str] = set() | ||
| for candidate in candidates: | ||
| norm = normalize_skill_name(candidate) | ||
| if norm in seen: | ||
| continue | ||
| seen.add(norm) | ||
| for server, tools in servers.items(): | ||
| if normalize_skill_name(server) == norm: | ||
| return server, tools | ||
| return None |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Eliminate duplicated candidate-building logic.
Lines 46-56 duplicate the candidate extraction logic from skill_lookup_keys (lines 10-20). Call skill_lookup_keys(skill_name) to get normalized candidates, then iterate through them directly.
♻️ Proposed refactor
def find_mcp_server_for_skill_name(
skill_name: str,
mcp_tools: Mapping[str, object],
) -> tuple[str, list[str]] | None:
"""Match a skill name (or plugin alias) to a connected MCP server."""
servers = _index_mcp_servers(mcp_tools)
if not servers:
return None
- candidates: list[str] = []
- raw = skill_name.strip()
- if raw:
- candidates.append(raw)
- if ":" in raw:
- suffix = raw.rsplit(":", 1)[-1].strip()
- if suffix:
- candidates.append(suffix)
- prefix = raw.split(":", 1)[0].strip()
- if prefix:
- candidates.append(prefix)
-
- seen: set[str] = set()
- for candidate in candidates:
- norm = normalize_skill_name(candidate)
- if norm in seen:
- continue
- seen.add(norm)
+ lookup_keys = skill_lookup_keys(skill_name)
+ seen: set[str] = set()
+ for norm in lookup_keys:
+ if norm in seen:
+ continue
+ seen.add(norm)
for server, tools in servers.items():
if normalize_skill_name(server) == norm:
return server, tools
return None🤖 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/tools/skill/_mcp_bridge.py` around lines 37 - 67, The
candidate-extraction logic in find_mcp_server_for_skill_name duplicates
skill_lookup_keys; replace lines that build candidates (the raw/suffix/prefix
block) with a call to skill_lookup_keys(skill_name) to obtain the candidate
list, then iterate that list (normalizing with normalize_skill_name as before)
while using _index_mcp_servers(mcp_tools) to find a matching server; keep the
early return of server, tools and the seen dedup set logic if needed, and leave
_index_mcp_servers and normalize_skill_name calls intact.
Comment on lines
+653
to
+669
| async def _simulate_exception_cleanup(session: Session | None) -> None: | ||
| """Replicate exception-path cleanup from cli/__init__.py _reload_loop.""" | ||
| import contextlib | ||
|
|
||
| if session is None: | ||
| return | ||
| with contextlib.suppress(Exception): | ||
| from pythinker_code.scratchpad import cleanup_session_scratch | ||
|
|
||
| await cleanup_session_scratch( | ||
| session.work_dir, | ||
| session_id=session.id, | ||
| session_title=session.title, | ||
| ) | ||
| if session.is_empty(): | ||
| await session.delete() | ||
|
|
Contributor
There was a problem hiding this comment.
Keep the test helper’s exception semantics aligned with CLI behavior.
At Line 667, _simulate_exception_cleanup deletes empty sessions without suppression, but the real CLI path suppresses deletion failures. This can produce false negatives in tests that are meant to mirror runtime behavior.
Proposed fix
async def _simulate_exception_cleanup(session: Session | None) -> None:
@@
if session.is_empty():
- await session.delete()
+ with contextlib.suppress(Exception):
+ await session.delete()🤖 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_session.py` around lines 653 - 669, The helper
_simulate_exception_cleanup currently calls await session.delete() unprotected,
diverging from the CLI which suppresses deletion failures; modify
_simulate_exception_cleanup so that the await session.delete() call is wrapped
in contextlib.suppress(Exception) (reuse the existing contextlib import) so both
cleanup_session_scratch and the session.delete() step suppress exceptions and
mirror the CLI behavior; keep checks using session.is_empty() and ensure no
other behavior changes.
Comment on lines
+686
to
+689
| async def test_exception_cleanup_removes_session_scratch_file( | ||
| isolated_share_dir: Path, | ||
| work_dir: HostPath, | ||
| ): |
Contributor
There was a problem hiding this comment.
Resolve Ruff ARG001 on newly added test fixture parameters.
isolated_share_dir is unused in both new tests (Line 687 and Line 722), which triggers ARG001. Rename to _isolated_share_dir to keep fixture side effects while keeping lint clean.
Proposed fix
async def test_exception_cleanup_removes_session_scratch_file(
- isolated_share_dir: Path,
+ _isolated_share_dir: Path,
work_dir: HostPath,
):
@@
async def test_exception_cleanup_removes_scratch_even_for_nonempty_session(
- isolated_share_dir: Path,
+ _isolated_share_dir: Path,
work_dir: HostPath,
):Also applies to: 721-724
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 687-687: Unused function argument: isolated_share_dir
(ARG001)
🤖 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_session.py` around lines 686 - 689, The ARG001 lint warning
is triggered because the test fixture parameter isolated_share_dir is unused;
rename the parameter to _isolated_share_dir in the
test_exception_cleanup_removes_session_scratch_file test to acknowledge the
fixture side effects without using it, and apply the same rename to the other
new test that also declares isolated_share_dir so both tests compile lint-clean
(update the function signatures only, e.g., def
test_exception_cleanup_removes_session_scratch_file(... isolated_share_dir:
Path, ...) -> ... to ... _isolated_share_dir: Path ...).
Source: Linters/SAST tools
Comment on lines
+62
to
+80
| @pytest.mark.asyncio | ||
| async def test_recap_singular_on_persists_and_reloads( | ||
| runtime: Runtime, tmp_path: Path, monkeypatch | ||
| ) -> None: | ||
| config_path = (tmp_path / "config.toml").resolve() | ||
| runtime.config.source_file = config_path | ||
| runtime.config.tui.turn_recaps = False | ||
| app = _make_shell_app(runtime, tmp_path) | ||
|
|
||
| config_for_save = get_default_config() | ||
| monkeypatch.setattr(shell_slash, "load_config", Mock(return_value=config_for_save)) | ||
| monkeypatch.setattr(shell_slash, "save_config", Mock()) | ||
| monkeypatch.setattr(shell_slash.console, "print", Mock()) | ||
|
|
||
| with pytest.raises(Reload): | ||
| await _run_settings(app, "recap on") | ||
|
|
||
| assert config_for_save.tui.turn_recaps is True | ||
|
|
Contributor
There was a problem hiding this comment.
Assert the persistence call path, not only the mutated object.
This test currently passes if save_config(...) is skipped but config_for_save is mutated. Add explicit call assertions (matching the plural-form test) so alias behavior verifies persistence too.
Suggested test hardening
`@pytest.mark.asyncio`
async def test_recap_singular_on_persists_and_reloads(
runtime: Runtime, tmp_path: Path, monkeypatch
) -> None:
@@
- monkeypatch.setattr(shell_slash, "load_config", Mock(return_value=config_for_save))
- monkeypatch.setattr(shell_slash, "save_config", Mock())
+ load_mock = Mock(return_value=config_for_save)
+ save_mock = Mock()
+ monkeypatch.setattr(shell_slash, "load_config", load_mock)
+ monkeypatch.setattr(shell_slash, "save_config", save_mock)
@@
with pytest.raises(Reload):
await _run_settings(app, "recap on")
+ load_mock.assert_called_once_with(config_path)
+ save_mock.assert_called_once_with(config_for_save, config_path)
assert config_for_save.tui.turn_recaps is TrueAs per coding guidelines: “Flag tests missing assertions, or with trivially-true assertions.”
🤖 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/ui_and_conv/test_settings_recaps_slash.py` around lines 62 - 80, The
test mutates config_for_save but doesn't assert that the persistence path was
invoked; update test_recap_singular_on_persists_and_reloads to assert the save
call on the mocked save_config the same way the plural-form test does: verify
shell_slash.save_config was called (e.g., assert called_once) and that it was
invoked with the expected arguments (the config file
path/runtime.config.source_file and config_for_save) so the test ensures
persistence, not just in-memory mutation.
Source: Coding guidelines
Comment on lines
+396
to
+399
| def render_agent_status(self, columns: int): # noqa: ARG002 | ||
| return "TaskOutput(agent-reviewer · block, timeout 600s)" | ||
|
|
||
| def render_pinned_status_tail(self, columns: int): # noqa: ARG002 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Optional: Add return type annotations to test stub methods.
Ruff flags missing return type annotations for the private methods in _BlockingTaskOutputDelegate. While this is a test stub, adding -> str would improve consistency:
- def render_agent_status(self, columns: int): # noqa: ARG002
+ def render_agent_status(self, columns: int) -> str: # noqa: ARG002
return "TaskOutput(agent-reviewer · block, timeout 600s)"
- def render_pinned_status_tail(self, columns: int): # noqa: ARG002
+ def render_pinned_status_tail(self, columns: int) -> str: # noqa: ARG002
return ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def render_agent_status(self, columns: int): # noqa: ARG002 | |
| return "TaskOutput(agent-reviewer · block, timeout 600s)" | |
| def render_pinned_status_tail(self, columns: int): # noqa: ARG002 | |
| def render_agent_status(self, columns: int) -> str: # noqa: ARG002 | |
| return "TaskOutput(agent-reviewer · block, timeout 600s)" | |
| def render_pinned_status_tail(self, columns: int) -> str: # noqa: ARG002 | |
| return "" |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 396-396: Missing return type annotation for private function render_agent_status
Add return type annotation: str
(ANN202)
[warning] 399-399: Missing return type annotation for private function render_pinned_status_tail
Add return type annotation: str
(ANN202)
🤖 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/ui_and_conv/test_visualize_running_prompt.py` around lines 396 - 399,
Add explicit return type annotations (-> str) to the test stub methods in class
_BlockingTaskOutputDelegate: change the signatures of render_agent_status(self,
columns: int) and render_pinned_status_tail(self, columns: int) to include ->
str so they read render_agent_status(self, columns: int) -> str and
render_pinned_status_tail(self, columns: int) -> str.
Source: Linters/SAST tools
Member
Author
|
Superseded by #127 — this PR auto-closed when the branch was renamed from |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch bundles the tracing-dashboard redesign with a robustness fix for the project-memory tool.
Project-memory capacity handling (this session)
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) exceedsread as satisfiable (2085+113 < 2200) but wasn't — and the agent had no way to see the true ceiling or what was stored.used/limit, and a compact inventory (index, size, preview) so the nextremove/replaceis guided, not guessed.listaction for mid-session introspection (status()/capacity()on the store)./memorynow shows per-store capacity and a "nearly full" panel at ≥85%.recall.py+consolidation.py+JOURNAL.md+ atomic writes already cover retrieval/compaction/durability.Tracing dashboard redesign
vis/web.Verification
status/list, replace-overage, and the/memorycapacity line.Notes / tradeoffs
snapshot()already truncates gracefully and the header points the agent to the files. LeftINJECTION_BUDGET_BYTESat 8 KB to avoid inflating every prompt.Summary by CodeRabbit
Release Notes
New Features
best_practices_alwaysconfig option to auto-inject best-practices guidance/recapcommand withon/offtoggle plus inline autosuggest/memory listaction with capacity and inventory reportingImprovements
Reliability