feat(tui+lsp): TUI theme system, streaming phase 0, LSP subsystem, and report panel polish - #157
Merged
Conversation
Diff add/remove rows are distinguished by background tint only; line numbers and +/- glyphs no longer use green/red accent foreground. Co-authored-by: Cursor <cursoragent@cursor.com>
Port the reference LSP subsystem to a first-class Python feature: a new `LSP` agent tool (definition, references, hover, document/workspace symbols, implementation, full call hierarchy), session-scoped server lifecycle over the Host stdio abstraction, passive diagnostics injected after file edits, and plugin-only server discovery/recommendation (no bundled binaries). Hardening folded in from review: - framing raises LspServerDown on EOF so the read loop fails pending requests instead of busy-spinning (C01/C10); wired LspClient.on_crash -> instance mark_crashed so the max_restarts cap is enforced against real crashes. - LSP tool distinguishes "no server for file type" from an empty server response (definition-not-found now renders guidance, not a false "no server"). - request failures return an error result instead of a false-success ok(). - file-tool edit hook (write/replace) clears delivered diagnostics for the file and wraps change/save notifications in try/except so an LSP hiccup never fails an already-successful write (C03). - no-server file types surface a one-line plugin install hint (per ext/session), gated by recommendation_disabled/never. Persisted >=5 ignore auto-disable is deferred: save_config() rewrites the whole file with no lock/atomic rename, so incremental concurrent writes are unsafe (multi-instance clobber risk). Registered on default + coder agents only; excluded from offline/fail-closed code_reviewer. Servers are plugin-only with PluginPolicy.external_exec gating.
Replace monolithic theme.py with spec/palette/resolver adapters, trim periwinkle inline highlights, add slash prefix and subcommand suggest highlighting, grey out low context-usage bar, and coalesce live streaming repaints. Refresh native install script help and progress UX.
…all UX Switch inline code and ANSI syntax highlights from cyan to accent/periwinkle and blue, add trailing scrollback gaps between tool output and agent prose, style the welcome banner branch with muted warning yellow, and ship the token-activity heatmap for /usage. Refresh install banner animation and tests.
…f/spacing polish Port pythinker-x theme: 32 bundled syntax theme names, Catppuccin Frappe/Macchiato styles, `/theme code` syntax picker, and aligned diff palette. Tool-call subject highlights switch from cyan to brand periwinkle accent; welcome banner uses neutral grey for branch and yellow for model name. Fix diff marker spacing for @-prefixed lines and add composing-block blank-row before the activity line.
Report rendering: - Add `pythinker_report_markdown()` + `report_markdown_style_overrides()` so only H1 headings render bold inside report panels; all other roles drop bold weight - Replace `### section` headings with `# title` routed through report markdown so section headers are bold while body prose stays regular - Add `REPORT_FILE_MARKER` glyph (⌁ / +) for finding location rows; use a grid table to keep long paths indented consistently - Add `secondary` CoreToken and `TuiTokens.secondary` field (dark #AAB0B6, light #8A93A0) for scope/note text that needs less emphasis than `muted` - Replace `Rule(severity …)` section separators with `_render_section_header()` (title line + plain rule below) for cleaner visual hierarchy - Pill-background summary line: severity dots and counts share `tool_pending_bg` background for compact badge appearance - Correct `PROMPT_GLYPH` dark hex to #F1F3F5 (was left behind when activity_label was updated in b9662b7) Worklog / blocks: - `_tool_token_style` now raises `ValueError` on unknown token names instead of silently falling back to `info`; all reachable style values are valid TUI tokens - Insert `BLANK_ROW` between the collapsed card and its sub-tool activity group Selector: - Extract `_default_match` typed nested function in `code_theme.py` to fix pyright `reportUnknownLambdaType` on the prior lambda LSP gaps (from lsp-code-scan-analysis report): - Add `test_lsp_provider_registered_in_subagent_soul` guard test: asserts `LspDiagnosticsInjectionProvider` is in `_injection_providers` and `rearm_injection` is wired for subagent-role `PythinkerSoul` instances - Update blackbox port-status ledger: LSP row promoted from future-approved-only to done (d936b32, 63 tests, PLIP-10)
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR implements a complete Language Server Protocol subsystem with session-scoped lifecycle management and passive diagnostics injection, refactors the theme system into a token-resolution package, introduces comprehensive Markdown normalization with audit-report handling, adds token-activity charts to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Pythinker as Pythinker Runtime
participant Mgr as LspServerManager
participant Inst as LspServerInstance
participant Diag as DiagnosticRegistry
participant Inj as LspDiagnosticsInjectionProvider
User->>Pythinker: File edit via WriteFile
Pythinker->>Mgr: Ensure server started for extension
Mgr->>Inst: Start/initialize LSP server
Inst->>Inst: JSON-RPC initialize handshake
Mgr->>Inst: Send didOpen notification
User->>Pythinker: Subsequent save/change
Mgr->>Inst: Send didChange + didSave
Inst-->>Diag: Publish diagnostics notification
Diag->>Diag: Deduplicate & volume-cap
Inj->>Diag: Check pending diagnostics
Inj->>Pythinker: Inject rendered block
Pythinker->>User: Display diagnostics in context
sequenceDiagram
participant CLI as CLI User
participant Shell as CustomPromptSession
participant Slash as SlashCommandCompleter
participant Theme as ThemeRegistry
CLI->>Shell: Type /theme code<space>
Shell->>Slash: Check first-arg context
Slash-->>Shell: Suggest code-theme options
Shell->>CLI: Ghost-complete theme name
CLI->>Shell: Press Enter
Shell->>Theme: Apply new syntax theme
Theme->>Shell: Redraw with updated highlighting
sequenceDiagram
participant Agent as Agent Step
participant Mgr as MarkdownRenderer
participant Audit as AuditNormalizer
participant Norm as NormalizeMarkdown
participant UI as Shell Display
Agent->>Mgr: Dense audit report text
Mgr->>Audit: detect_audit_report(text)
Audit-->>Mgr: true if parity/matrix pattern
Mgr->>Norm: normalize_model_markdown(..., audit=true)
Norm->>Norm: Collapse matrices, normalize fields, verify prose
Norm-->>Mgr: Condensed structured markdown
Mgr->>UI: Render via pythinker_report_markdown
UI-->>Agent: Compact report panel
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/ui_and_conv/test_pythinker_themes_port.py (1)
64-68: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a regression case for distinct Catppuccin variants.
This test currently misses the non-string resolver path where two different Catppuccin themes can be misclassified as equal.
Proposed test addition
def test_code_themes_match_for_picker_resolves_aliases(): assert code_themes_match_for_picker("github", "github") assert code_themes_match_for_picker("github-dark", "github") + assert not code_themes_match_for_picker("catppuccin-frappe", "catppuccin-macchiato") assert not code_themes_match_for_picker("dracula", "github")🤖 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_pythinker_themes_port.py` around lines 64 - 68, The test_code_themes_match_for_picker_resolves_aliases function is missing regression test cases for distinct Catppuccin variants that can be misclassified as equal through the non-string resolver path. Add assertion statements to the test function that verify different Catppuccin theme variants are correctly identified as distinct (not matching when they should not). These additional assertions should follow the same pattern as the existing assertions in the function, using code_themes_match_for_picker to test pairs of distinct Catppuccin variants and ensuring they return False to prevent regression.src/pythinker_code/ui/shell/visualize/_worklog.py (1)
133-164:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
icon_styledefault change is currently a no-op because the parameter is unused.
Line 140changed the default, butrender_worklog_entry()never readsicon_style(oricon) inLine 145-Line 164. Either wire these params into rendering or remove them from the signature to avoid a misleading API contract.🤖 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/visualize/_worklog.py` around lines 133 - 164, The `render_worklog_entry` function has unused parameters `icon` and `icon_style` in its signature that are never referenced in the function body. To fix this misleading API contract, either remove both parameters from the function signature if they are not needed, or integrate them into the rendering logic by using them to customize the icon when `icon_renderable` is None (likely by modifying how `_state_icon(state)` is called or creating the icon_renderable based on these parameters).Source: Linters/SAST tools
🤖 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 `@scripts/install-native.sh`:
- Around line 644-650: The printf statements on lines 644 and 650 emit ANSI
cursor escape sequences unconditionally, causing control codes to leak into logs
when animation mode is disabled (non-TTY/CI environments). Wrap both printf
calls with a conditional check on the _anim variable: when _anim is enabled,
keep the current ANSI escape sequences for cursor positioning; when _anim is
disabled, output a plain text message without escape codes instead. This ensures
cursor control only affects animated terminal output and does not pollute plain
logs with control characters.
In `@src/pythinker_code/config.py`:
- Around line 1039-1040: The fields startup_timeout, max_restarts, and
recommendation_ignored_count (also at line 1047) currently lack validation
constraints and accept invalid negative or zero values, which causes failures at
runtime instead of config-parse time. Add validation constraints to each of
these Field definitions to ensure they only accept positive values (greater than
0). Use Pydantic's Field constraints (such as gt=0 for greater than zero) to
enforce this validation at config initialization, providing users with
immediate, explicit error messages for invalid configuration inputs.
In `@src/pythinker_code/lsp/client.py`:
- Around line 139-143: The stop() method's graceful shutdown attempt using
send_request("shutdown", None) lacks a timeout, which can cause the method to
hang indefinitely if the server becomes unresponsive, preventing the kill
fallback from ever executing. Add a timeout wrapper (using asyncio.wait_for or
similar timeout mechanism) around the await self.send_request("shutdown", None)
call to ensure it completes or raises an exception within a reasonable
timeframe, allowing the code to proceed to the kill fallback if the graceful
shutdown takes too long.
In `@src/pythinker_code/lsp/diagnostics.py`:
- Around line 124-133: The register_pending method currently skips empty
diagnostic batches with the `if not file.diagnostics: continue` statement, which
allows stale pending diagnostics to persist for that URI. Instead of skipping
empty diagnostics, treat them as a clear signal for that file URI. Remove or
modify the early continue condition so that files with empty diagnostics still
update the server_paths mapping and either clear any existing pending entries or
set empty entries for that URI, ensuring that empty diagnostic payloads properly
remove stale diagnostics rather than leaving them to be injected later. This
same pattern appears to need fixing at another location in the codebase as well
(around line 278-287).
- Around line 88-90: The expression `entry.code or None` on line 89 incorrectly
treats the valid diagnostic code value of `0` as falsy and collapses it to
`None`, which can cause distinct diagnostics to be incorrectly merged in dedup
keys. Replace the falsy check with an explicit None check: instead of using
`entry.code or None`, use `entry.code if entry.code is not None else None` (or
simply `entry.code`) to preserve the `0` value while still handling the None
case correctly.
In `@src/pythinker_code/lsp/framing.py`:
- Around line 39-53: The read_message function in framing.py lacks validation
for excessively large Content-Length values, allowing a malicious server to
force oversized reads and crash the client. After the existing checks for None
and negative content_length values (after line 50), add a guard that enforces a
reasonable maximum limit on the content_length before the stdout.readexactly
call. If the content_length exceeds this maximum, raise an LspProtocolError with
a descriptive message indicating the limit was exceeded. Define an appropriate
constant for the maximum allowed Content-Length (for example, a reasonable upper
bound like 10MB or 100MB depending on protocol requirements).
In `@src/pythinker_code/lsp/manager.py`:
- Around line 103-109: The ensure_started() method restarts a server from ERROR
or STOPPED state but does not clear the _opened_files dictionary, which causes
subsequent open_file() calls to skip sending didOpen notifications to the fresh
server process, leaving document state out of sync. After calling await
server.start() in the ensure_started() method, clear the _opened_files entries
for all documents associated with that server to reset the open-document
tracking state. This same fix should also be applied at the other location
mentioned (lines 128-130) where servers are restarted, to ensure consistent
behavior across all server restart paths.
- Around line 140-141: The `didChange` notification handler is currently sending
a hardcoded version value of 1 for every content change, violating the LSP
protocol requirement that versions must be strictly increasing integers
reflecting the document state after changes. Implement document version tracking
by maintaining a version counter for each open document (likely in the document
manager state or a dictionary keyed by document URI), increment this version
counter each time a didChange event is processed, and update the "version" field
in the notification payload to use the current incremented version instead of
the hardcoded 1 value. Ensure the version is incremented before being included
in the notification to properly reflect the document state after the changes are
applied.
In `@src/pythinker_code/lsp/recommend.py`:
- Around line 151-157: The is_plugin_installed() function is being called
repeatedly inside the loop that iterates over all_lsp_plugins.items(), causing
redundant I/O operations since it reloads installed-plugin state on each
iteration. Move the retrieval of installed plugin IDs outside the loop before
iterating over all_lsp_plugins, cache the result in a collection (such as a
set), and then replace the is_plugin_installed(plugin_id) check inside the loop
with a simple membership test against the cached installed plugin IDs. This
eliminates the repeated I/O calls while maintaining the same filtering behavior.
In `@src/pythinker_code/lsp/service.py`:
- Around line 99-108: In the reinitialize method, you need to cancel any
in-flight init task before replacing _init_event to prevent existing callers
waiting on the old event from hanging indefinitely. Store a reference to the
init task (created when _kickoff_init runs the _run_init coroutine) as a class
variable, and in reinitialize, check if this task exists and is still running,
then call cancel() on it before proceeding with the rest of the reinitialize
logic. This ensures that stale _run_init executions that might be checking
generations (as referenced in the _run_init method around line 156) will be
properly cancelled and won't cause wait_for_init to block indefinitely when
waiting on the old _init_event.
In `@src/pythinker_code/tools/lsp/formatters.py`:
- Line 3: Remove the file-wide Pyright suppression comment that blankets the
entire formatters.py module with reportUnknownArgumentType,
reportUnknownMemberType, and reportUnknownVariableType=false directives.
Instead, add proper type annotations to handle dynamic payload data using
TypedDicts or typed normalization helper functions for the specific functions
and code sections that deal with untyped inputs, while keeping strict Pyright
checks enabled for the rest of the module. This aligns with the coding guideline
that all code in src/pythinker_code/**/*.py must have strict type coverage.
- Around line 409-413: The file_count is hardcoded to 1 whenever symbols exist,
regardless of whether the result contains SymbolInformation from multiple files.
When is_document_symbol is False (indicating SymbolInformation with location
field rather than DocumentSymbol with range), calculate the actual number of
unique files from the symbols instead of hardcoding file_count to 1. Keep the
current hardcoded behavior only when is_document_symbol is True, since
DocumentSymbol results represent a single file.
In `@src/pythinker_code/tools/lsp/symbol_context.py`:
- Around line 55-60: The boundary checks in this code are rejecting valid cursor
positions. On lines 55-56, the condition incorrectly rejects the final line
whenever the read chunk equals MAX_READ_BYTES and we are on the last line, even
when the file was fully read; rethink this logic to only reject when there is
confirmed truncation or incomplete data. On lines 59-60, the check `zero_char >=
len(line_content)` incorrectly rejects the cursor-at-EOL position (where
zero_char equals the line length), which is a valid symbol lookup location;
change the condition to `zero_char > len(line_content)` to allow the EOL cursor
position.
In `@src/pythinker_code/tools/lsp/tool.md`:
- Line 1: The markdown file tool.md is missing a top-level heading at the
beginning, which violates the MD041 markdownlint rule. Add a top-level heading
(H1) using a single hash symbol at the start of the file before the existing
text that describes interacting with Language Server Protocol servers. This
heading should briefly describe the purpose of the file and serve as the main
title.
In `@src/pythinker_code/tools/lsp/tool.py`:
- Around line 50-53: The code checks if self._lsp.manager is not None at the
initial status check, but then awaits _validate_file which can cause the service
state to change asynchronously. Between the initial check and when manager is
actually used at line 69, the manager could become None, causing a crash before
reaching the guarded try block. After awaiting _validate_file, re-validate that
self._lsp.manager is still not None and the status is still SUCCESS before using
the manager to avoid the TOCTOU (Time-of-Check-Time-of-Use) crash scenario.
- Around line 355-365: The code currently swallows exceptions from the
`_run_git_check_ignore` function call and treats failures as "no ignored paths",
allowing ignored files to pass through unfiltered when git checks fail or
timeout. Instead of silently ignoring errors, you must handle exceptions from
`_run_git_check_ignore` explicitly. Either propagate the exception to fail
loudly when git check-ignore fails, or implement a fail-safe approach that
excludes locations when we cannot verify whether they are ignored. Review the
exception handling around the batch processing loop and the
`_run_git_check_ignore` calls to ensure that git failures do not result in
returning unfiltered valid locations.
In `@src/pythinker_code/ui/shell/slash.py`:
- Around line 1055-1063: The input argument to the `/theme code <name>` command
is being lowercased at line 1125, but the membership check at line 1056 in the
arg validation block performs a case-sensitive comparison against the available
themes. This mismatch causes mixed-case custom theme names to be rejected. Fix
this by performing the membership check against `available` using a
case-insensitive comparison (compare the lowercase version of arg against
lowercase versions of available items), and ensure this same case-insensitive
approach is applied at the other affected sites around lines 1125-1127 and 1158
where similar membership or assignment logic occurs with the theme argument.
In `@src/pythinker_code/ui/shell/usage_activity.py`:
- Around line 89-96: The render_activity function computes the current day using
datetime.now() (around line 411) instead of using the TokenActivity.today_index
field that is explicitly provided in the dataclass. This causes the chart
timeline to be anchored to wall-clock time rather than the activity payload's
time window, misaligning month labels and future-cell masking for non-current
snapshots including test loads. Refactor render_activity to use the today_index
from the TokenActivity object for determining which cells should be masked as
future, ensuring the rendering respects the activity payload's window rather
than the current wall-clock time.
- Around line 175-191: The _current_streak function currently breaks immediately
when today's bucket is empty, contradicting the documented behavior that partial
days should not end the streak. Fix this by modifying the loop to not treat
today's empty bucket as a streak-ending condition. Either skip today in the
initial loop check and handle it separately, or modify the loop to iterate from
yesterday backwards (starting from end - 1) while unconditionally counting today
as part of the streak. Ensure the function still returns 0 only when there is
genuinely no activity, not when today simply has a partial/empty bucket.
In `@src/pythinker_code/ui/shell/usage.py`:
- Around line 295-300: The activity path validation in the code that checks if
positional[0] matches an activity view (via _parse_activity_view) does not
validate that there are no additional positional arguments beyond the first one.
When users provide extra arguments like `/usage daily extra`, they are silently
ignored and _print_activity_card is called with only the activity mode, causing
silent misrouting instead of reporting invalid arguments. Add validation to
ensure that when positional[0] is a valid activity view, the positional list
contains exactly one element; if additional arguments are present, report an
error to the user instead of proceeding. This validation needs to be applied at
the activity view check location (around lines 295-300) and also at the sibling
location mentioned at lines 393-394.
In `@src/pythinker_code/ui/shell/visualize/_live_view.py`:
- Around line 282-297: The `_frame_refresh_loop` method lacks proper error
supervision during the main event loop execution—if it raises an exception, the
refresh loop silently fails while the event loop continues, breaking the UI. Add
`frame_task` to the `asyncio.wait(...)` set in your main supervision loop and
call `frame_task.result()` to detect and propagate errors immediately rather
than only supervising it at shutdown. This same supervision issue applies at
multiple sites in the file (around lines 388-391 and 418-425) where background
tasks are created but not properly monitored during execution, so apply the same
pattern of including each background task in the wait set and checking results
to ensure all critical background tasks follow the C08 guideline for proper
lifecycle and error handling.
In `@src/pythinker_code/ui/theme/__init__.py`:
- Around line 62-96: The __all__ list in src/pythinker_code/ui/theme/__init__.py
is not sorted lexicographically, which violates Ruff's RUF022 rule. Sort all the
entries in the __all__ list (lines 62-96) in alphabetical order. Ensure the list
is sorted case-insensitively so that all names appear in lexicographic order as
expected by isort and Ruff linting rules.
In `@src/pythinker_code/utils/rich/markdown.py`:
- Around line 644-647: The issue is that `Style(bgcolor=None)` does not clear
background color when merging styles in Rich, because None values don't override
existing attributes. Fix this by reconstructing the style object when bgcolor is
not None. Instead of merging with Style(bgcolor=None), create a new Style that
explicitly includes only the desired attributes (color, bold, italic, underline,
strike) from the original style while omitting bgcolor entirely. Then merge this
reconstructed style with Style(bold=False). This ensures bgcolor is excluded
from the merged result rather than attempting an ineffective override.
In `@src/pythinker_code/utils/rich/syntax.py`:
- Around line 304-314: The `code_themes_match_for_picker` function at lines
312-313 only compares the types of non-string resolved themes using
`type(resolved_theme) is type(resolved_configured)`, which incorrectly matches
different instances of the same type (e.g., different Catppuccin themes that
both resolve to PygmentsSyntaxTheme). Instead of comparing only the types,
compare the actual resolved theme objects themselves for equality (e.g.,
`resolved_theme == resolved_configured`) to ensure distinct non-string themes
are properly differentiated while still matching identical themes.
In `@tests/tools/test_lsp_client.py`:
- Around line 222-226: Replace the polling loop in the notification test that
uses asyncio.sleep to wait for "params" to appear in the seen dictionary.
Instead, create an asyncio.Event that gets set when "params" is added to seen,
and use asyncio.wait_for with that event's wait() method to reliably wait for
the condition rather than spinning with sleep calls. This will eliminate
flakiness and provide better test diagnostics if the event is not set within the
timeout.
In `@tests/ui_and_conv/test_tui_streaming_phase0.py`:
- Around line 37-53: The test_frame_scheduler_coalesces_multiple_deltas function
uses a fixed 60ms sleep that assumes a frame tick will occur within that time,
but scheduler jitter can cause this to fail on slower CI systems. Replace the
fixed await asyncio.sleep(0.06) with a polling loop that waits for the
live.update method to be called at least once, using a bounded timeout (such as
1-2 seconds) to ensure the frame refresh loop executes reliably before canceling
the task. This approach will handle both fast and slow systems without flaking.
In `@tests/ui/test_usage_activity.py`:
- Around line 106-116: The test_summary_streak_uses_best_format function
validates streak formatting but does not cover the critical edge case where
today has zero tokens while the streak remains ongoing. Add a new test case that
creates an ActivitySummary with zero tokens for both lifetime_tokens and
peak_daily_tokens but maintains a non-zero current_streak_days value to validate
the partial-day behavior is correctly displayed. Additionally, check the code at
lines 228-247 to identify if similar edge case coverage is missing there and
apply the same regression test approach if needed.
---
Outside diff comments:
In `@src/pythinker_code/ui/shell/visualize/_worklog.py`:
- Around line 133-164: The `render_worklog_entry` function has unused parameters
`icon` and `icon_style` in its signature that are never referenced in the
function body. To fix this misleading API contract, either remove both
parameters from the function signature if they are not needed, or integrate them
into the rendering logic by using them to customize the icon when
`icon_renderable` is None (likely by modifying how `_state_icon(state)` is
called or creating the icon_renderable based on these parameters).
In `@tests/ui_and_conv/test_pythinker_themes_port.py`:
- Around line 64-68: The test_code_themes_match_for_picker_resolves_aliases
function is missing regression test cases for distinct Catppuccin variants that
can be misclassified as equal through the non-string resolver path. Add
assertion statements to the test function that verify different Catppuccin theme
variants are correctly identified as distinct (not matching when they should
not). These additional assertions should follow the same pattern as the existing
assertions in the function, using code_themes_match_for_picker to test pairs of
distinct Catppuccin variants and ensuring they return False to prevent
regression.
🪄 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: 859691c1-c7c5-4bc4-903b-879da2f15ddf
⛔ Files ignored due to path filters (5)
docs/.vitepress/config.tsis excluded by!docs/**docs/en/customization/architecture.mdis excluded by!docs/**docs/en/customization/lsp.mdis excluded by!docs/**docs/public/install.shis excluded by!docs/**tasks/blackbox-port-status.mdis excluded by!tasks/**
📒 Files selected for processing (107)
CHANGELOG.mdplips/plip-10-lsp-system.mdscripts/install-native.shsrc/pythinker_code/agents/default/agent.yamlsrc/pythinker_code/agents/default/coder.yamlsrc/pythinker_code/app.pysrc/pythinker_code/config.pysrc/pythinker_code/lsp/__init__.pysrc/pythinker_code/lsp/client.pysrc/pythinker_code/lsp/diagnostics.pysrc/pythinker_code/lsp/framing.pysrc/pythinker_code/lsp/instance.pysrc/pythinker_code/lsp/manager.pysrc/pythinker_code/lsp/plugin_servers.pysrc/pythinker_code/lsp/protocol.pysrc/pythinker_code/lsp/recommend.pysrc/pythinker_code/lsp/service.pysrc/pythinker_code/plugin/manifest.pysrc/pythinker_code/soul/agent.pysrc/pythinker_code/soul/dynamic_injections/lsp_diagnostics.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/tools/file/replace.pysrc/pythinker_code/tools/file/write.pysrc/pythinker_code/tools/lsp/__init__.pysrc/pythinker_code/tools/lsp/formatters.pysrc/pythinker_code/tools/lsp/schemas.pysrc/pythinker_code/tools/lsp/symbol_context.pysrc/pythinker_code/tools/lsp/tool.mdsrc/pythinker_code/tools/lsp/tool.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/components/diff.pysrc/pythinker_code/ui/shell/components/markdown.pysrc/pythinker_code/ui/shell/components/report.pysrc/pythinker_code/ui/shell/design_system.pysrc/pythinker_code/ui/shell/echo.pysrc/pythinker_code/ui/shell/glyphs.pysrc/pythinker_code/ui/shell/mcp_status.pysrc/pythinker_code/ui/shell/motion.pysrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/selectors/code_theme.pysrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/spacing.pysrc/pythinker_code/ui/shell/tool_renderers/_render_utils.pysrc/pythinker_code/ui/shell/tool_renderers/agent.pysrc/pythinker_code/ui/shell/tool_renderers/ask_user.pysrc/pythinker_code/ui/shell/tool_renderers/background.pysrc/pythinker_code/ui/shell/tool_renderers/edit.pysrc/pythinker_code/ui/shell/tool_renderers/find.pysrc/pythinker_code/ui/shell/tool_renderers/grep.pysrc/pythinker_code/ui/shell/tool_renderers/plan.pysrc/pythinker_code/ui/shell/tool_renderers/read.pysrc/pythinker_code/ui/shell/tool_renderers/skill.pysrc/pythinker_code/ui/shell/tool_renderers/web.pysrc/pythinker_code/ui/shell/tool_renderers/write.pysrc/pythinker_code/ui/shell/update.pysrc/pythinker_code/ui/shell/usage.pysrc/pythinker_code/ui/shell/usage_activity.pysrc/pythinker_code/ui/shell/visualize/_blocks.pysrc/pythinker_code/ui/shell/visualize/_dialog_shell.pysrc/pythinker_code/ui/shell/visualize/_interactive.pysrc/pythinker_code/ui/shell/visualize/_live_view.pysrc/pythinker_code/ui/shell/visualize/_worklog.pysrc/pythinker_code/ui/theme.pysrc/pythinker_code/ui/theme/__init__.pysrc/pythinker_code/ui/theme/adapters/__init__.pysrc/pythinker_code/ui/theme/adapters/markdown.pysrc/pythinker_code/ui/theme/adapters/task_browser.pysrc/pythinker_code/ui/theme/capabilities.pysrc/pythinker_code/ui/theme/palettes.pysrc/pythinker_code/ui/theme/pythinker_themes.pysrc/pythinker_code/ui/theme/registry.pysrc/pythinker_code/ui/theme/resolver.pysrc/pythinker_code/ui/theme/spec.pysrc/pythinker_code/utils/rich/markdown.pysrc/pythinker_code/utils/rich/syntax.pytests/test_installation_docs.pytests/tools/test_lsp_client.pytests/tools/test_lsp_diagnostics.pytests/tools/test_lsp_manager.pytests/tools/test_lsp_plugins.pytests/tools/test_lsp_tool.pytests/ui/test_usage_activity.pytests/ui_and_conv/test_live_view_todos.pytests/ui_and_conv/test_plan_display_panel.pytests/ui_and_conv/test_pythinker_themes_port.pytests/ui_and_conv/test_report.pytests/ui_and_conv/test_shell_panel.pytests/ui_and_conv/test_shell_prompt_echo.pytests/ui_and_conv/test_shell_welcome_info.pytests/ui_and_conv/test_slash_completer.pytests/ui_and_conv/test_slash_highlight.pytests/ui_and_conv/test_spacing_primitives.pytests/ui_and_conv/test_statusline_render.pytests/ui_and_conv/test_streaming_content_block.pytests/ui_and_conv/test_theme.pytests/ui_and_conv/test_theme_contract.pytests/ui_and_conv/test_tui_blocks_integration.pytests/ui_and_conv/test_tui_card_tool_renderers.pytests/ui_and_conv/test_tui_streaming_phase0.pytests/ui_and_conv/test_tui_theme_tokens.pytests_e2e/test_wire_approvals_tools.pytests_e2e/test_wire_config.pytests_e2e/test_wire_prompt.pytests_e2e/test_wire_protocol.pytests_e2e/test_wire_sessions.pytests_e2e/test_wire_skills_mcp.pyweb/public/install.sh
💤 Files with no reviewable changes (1)
- src/pythinker_code/ui/theme.py
Refactor shell markdown into focused modules with shared fence scanning, size guards, and capped streaming parse cache. Add audit-profile normalization for dense parity reports (collapsed matrices, field tables, path compaction, and quote gutters) and fix report-prose misparsing of aligned field lines. Align dark prompt border tokens with core border palette.
Show awaiting approval while ExitPlanMode waits for user choice, pause the Considering spinner during question/approval panels, reorder choices, and improve dialog spacing and copy. Also harden LSP frame reads and sync tests.
…Agents Make the PR branch green and address the review. CI failures (15 tests + ruff): - Update agent-spec / config-dump / pyinstaller snapshots for the new LSP tool - Add explicit encoding to LSP framing/symbol_context/tool sources (static check) - Sync dark prompt frame/separator/dialog borders to their core theme tokens - Flush the live view immediately for external (approval/steer) messages and supervise the frame/status refresh loops so a refresh failure surfaces instead of silently freezing the view (fixes 3 external-approval tests) - Wire `_mode` + completer onto the bottom-toolbar unit test's bare session - Fix import sorting / line length CodeRabbit findings: - Critical: inline code spans now actually clear an inherited background (Rich `Style(bgcolor=None)` is a no-op; mutate the copied style instead) - LSP: bounded JSON-RPC frame size + graceful-shutdown timeout, didChange document version tracking, open-doc state cleared on restart, empty diagnostics payloads clear stale entries, code-0 dedup key, TOCTOU re-check, documentSymbol file_count - Theme picker: compare resolved themes by identity so distinct Catppuccin variants no longer match - Usage activity: anchor the chart to the payload window (not wall-clock) and fix the current-streak partial-today contract - Config: validate LSP numeric settings; `/usage` rejects extra activity args; `/theme code <Name>` preserves case; guard install-script cursor escapes to animated mode; drop unused worklog icon params RunAgents: strip stray whitespace-only string entries the model emits between agent objects so a multi-agent launch no longer fails validation. Also: AGENTS.md gains a "Pre-PR gate" checklist (run the full gate, snapshot fix-direction, encoding/bundling/changelog checks) to stop these slipping to CI.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
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/visualize/_interactive.py (1)
735-742:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
_force_refreshis being consumed without guaranteeing a repaint.When
_force_refreshis set while_dirtyand_need_recomposeare both false, this branch clears all flags and returns withoutinvalidate(). That drops forced redraws from TurnEnd/QueueShutDown/external-message paths and can leave stale prompt UI until another event happens.Suggested fix
def _flush_prompt_refresh(self) -> None: if self._force_refresh: - if self._dirty or self._need_recompose: - self._prompt_session.invalidate() + # Forced refresh must always repaint, even when no recomposition flag is set. + self._prompt_session.invalidate() self._dirty = False self._force_refresh = False self._need_recompose = False return if self._need_recompose: self._dirty = True🤖 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/visualize/_interactive.py` around lines 735 - 742, The _flush_prompt_refresh method has a logic error where _force_refresh is being cleared without guaranteeing invalidate() is called. The condition that triggers invalidate() only checks if _dirty or _need_recompose are true, but when _force_refresh is set while both of those flags are false, the method returns without calling invalidate(), dropping the forced redraw. Fix this by modifying the condition to also include _force_refresh in the check, so that invalidate() is called whenever _force_refresh is true, regardless of the state of _dirty and _need_recompose.
🤖 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/ui/shell/markdown/audit.py`:
- Around line 403-406: The normalize_divergence_cards function unconditionally
advances cursor by 2 when label_match is found (line 405), but it does not
verify that the next line is actually an underline rule before doing so. This
causes valid content lines to be skipped. Before advancing cursor by 2, add a
validation check to confirm that lines[cursor + 1] contains an underline rule
(matching the expected underline pattern). Only advance cursor by 2 if this
validation passes; otherwise, handle the case where the next line is not an
underline appropriately to avoid dropping content.
In `@src/pythinker_code/ui/shell/markdown/fences.py`:
- Around line 57-61: The state capture in the loop is happening before the line
is processed by state.feed(), causing opening fence lines to be reported with
the wrong inside_fence value. Move the line `inside = state.active` to after the
`state.feed(body, strict_close=strict_close)` call so that the state value
reflects whether the current line is inside a fence after it has been processed,
aligning with the function contract described in lines 53-54.
In `@src/pythinker_code/ui/shell/tool_renderers/agent.py`:
- Around line 413-434: The `_run_agent_is_resolved()` function does not treat
`awaiting_approval` as a resolved state for async entries. Add
`awaiting_approval` to the set of raw statuses checked in the return statement
of `_run_agent_is_resolved()` (currently checking for "starting", "running",
"created", "launched") so that async entries in the awaiting_approval state are
properly marked as resolved and do not incorrectly display "Initializing…" in
`_run_agent_status_subline()`.
In `@src/pythinker_code/ui/shell/tool_renderers/background.py`:
- Around line 126-137: The metadata parsing logic in the block starting with the
loop checking if ":" is not in raw_line is too broad, treating any line with a
colon as metadata without verifying structured format is being used. This causes
valid output content containing colons to be incorrectly classified as metadata
and dropped. Add a guard condition before the metadata parsing loop to check for
an actual structured format signal (such as the presence of an [output] marker
or similar indicator) before attempting to extract metadata from lines
containing colons. This gate should only allow metadata parsing when the content
is confirmed to be in a structured format. Apply the same fix at the other
affected location mentioned in the comment.
In `@src/pythinker_code/ui/shell/tool_renderers/tool_search.py`:
- Line 86: The startswith method is being called twice on the text variable to
check for two different prefixes that share a common beginning. Instead of using
two separate startswith calls joined with or, consolidate them into a single
startswith call by passing a tuple of both strings as the argument. This
eliminates the redundant prefix scan and resolves the Ruff PIE810 warning.
Locate the conditional statement in the tool_search.py file where
text.startswith is used, and replace the two separate startswith calls with a
single call that accepts a tuple containing both "No visible tools" and "No
visible tools matched".
In `@src/pythinker_code/ui/shell/visualize/_blocks.py`:
- Around line 485-491: The `_render_report_update_body` method only creates the
`ReportUpdateComponent` instance once and reuses it, but `parse_report_update`
is called repeatedly on streaming updates, causing the rendered component to
become stale with old data. Refactor the method to update the
`self._report_update` component with the newly parsed update on each successful
parse, rather than only creating it once. This ensures the component state is
refreshed with each new update, not just initialized on first parse. Also apply
the same fix to the similar method at lines 493-500 that has the same stale
component issue.
In `@tests/ui_and_conv/test_md_normalization_matrix.py`:
- Around line 110-115: The assertion in the
test_streaming_does_not_commit_incomplete_fenced_code function is too lenient
due to the or clause that allows it to pass even when the boundary incorrectly
lands inside an unclosed fence. Remove the or "def foo" in partial[:boundary]
condition from the assert statement, keeping only the check that "```python" is
not present in the partial string up to the boundary, to properly validate that
incomplete fenced code blocks are not committed.
In `@tests/ui_and_conv/test_tui_streaming_phase0.py`:
- Around line 148-166: The test_live_paint_rate_matches_reveal_scheduler test is
brittle because it relies on inspecting source code with regex and hardcoding
the STREAM_FPS constant to 25, both of which can break on harmless refactors.
Instead of using inspect.getsource() and regex parsing to extract the
refresh_per_second argument from the visualize_loop method, refactor the test to
verify observable runtime behavior. Create a _LiveView instance and verify the
actual refresh rate that gets configured when the Live object is constructed, or
observe the rendered timing behavior during execution. This ensures the test
validates real behavior rather than implementation details, making it resilient
to code refactoring.
---
Outside diff comments:
In `@src/pythinker_code/ui/shell/visualize/_interactive.py`:
- Around line 735-742: The _flush_prompt_refresh method has a logic error where
_force_refresh is being cleared without guaranteeing invalidate() is called. The
condition that triggers invalidate() only checks if _dirty or _need_recompose
are true, but when _force_refresh is set while both of those flags are false,
the method returns without calling invalidate(), dropping the forced redraw. Fix
this by modifying the condition to also include _force_refresh in the check, so
that invalidate() is called whenever _force_refresh is true, regardless of the
state of _dirty and _need_recompose.
🪄 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: 9748a9c9-00a6-497a-bdff-c871e96cfc7c
⛔ Files ignored due to path filters (1)
docs/public/install.shis excluded by!docs/**
📒 Files selected for processing (73)
AGENTS.mdCHANGELOG.mdscripts/install-native.shsrc/pythinker_code/config.pysrc/pythinker_code/lsp/client.pysrc/pythinker_code/lsp/diagnostics.pysrc/pythinker_code/lsp/framing.pysrc/pythinker_code/lsp/manager.pysrc/pythinker_code/lsp/service.pysrc/pythinker_code/tools/agent/__init__.pysrc/pythinker_code/tools/lsp/formatters.pysrc/pythinker_code/tools/lsp/symbol_context.pysrc/pythinker_code/tools/lsp/tool.pysrc/pythinker_code/tools/plan/__init__.pysrc/pythinker_code/ui/shell/components/markdown.pysrc/pythinker_code/ui/shell/components/report.pysrc/pythinker_code/ui/shell/components/report_update.pysrc/pythinker_code/ui/shell/markdown/__init__.pysrc/pythinker_code/ui/shell/markdown/audit.pysrc/pythinker_code/ui/shell/markdown/elements.pysrc/pythinker_code/ui/shell/markdown/fences.pysrc/pythinker_code/ui/shell/markdown/normalizers.pysrc/pythinker_code/ui/shell/markdown/renderer.pysrc/pythinker_code/ui/shell/markdown/streaming.pysrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/spacing.pysrc/pythinker_code/ui/shell/statusline.pysrc/pythinker_code/ui/shell/tool_renderers/__init__.pysrc/pythinker_code/ui/shell/tool_renderers/_render_utils.pysrc/pythinker_code/ui/shell/tool_renderers/agent.pysrc/pythinker_code/ui/shell/tool_renderers/background.pysrc/pythinker_code/ui/shell/tool_renderers/plan.pysrc/pythinker_code/ui/shell/tool_renderers/tool_search.pysrc/pythinker_code/ui/shell/usage.pysrc/pythinker_code/ui/shell/usage_activity.pysrc/pythinker_code/ui/shell/visualize/_blocks.pysrc/pythinker_code/ui/shell/visualize/_dialog_shell.pysrc/pythinker_code/ui/shell/visualize/_interactive.pysrc/pythinker_code/ui/shell/visualize/_live_view.pysrc/pythinker_code/ui/shell/visualize/_question_panel.pysrc/pythinker_code/ui/shell/visualize/_worklog.pysrc/pythinker_code/ui/theme/palettes.pysrc/pythinker_code/utils/rich/markdown.pysrc/pythinker_code/utils/rich/syntax.pysrc/pythinker_code/wire/types.pytests/core/test_agent_spec.pytests/core/test_config.pytests/core/test_default_agent.pytests/core/test_wire_message.pytests/tools/test_agent_tool.pytests/tools/test_lsp_diagnostics.pytests/tools/test_lsp_manager.pytests/tools/test_lsp_tool.pytests/ui/test_usage_activity.pytests/ui_and_conv/test_audit_report_rendering.pytests/ui_and_conv/test_empty_think_part_indicator.pytests/ui_and_conv/test_md_normalization_matrix.pytests/ui_and_conv/test_prompt_tips.pytests/ui_and_conv/test_pythinker_themes_port.pytests/ui_and_conv/test_question_panel.pytests/ui_and_conv/test_report.pytests/ui_and_conv/test_report_update.pytests/ui_and_conv/test_shell_design_system.pytests/ui_and_conv/test_spacing_primitives.pytests/ui_and_conv/test_statusline_render.pytests/ui_and_conv/test_streaming_content_block.pytests/ui_and_conv/test_tool_call_block.pytests/ui_and_conv/test_tui_blocks_integration.pytests/ui_and_conv/test_tui_card_tool_renderers.pytests/ui_and_conv/test_tui_streaming_phase0.pytests/ui_and_conv/test_visualize_running_prompt.pytests/utils/test_pyinstaller_utils.pyweb/public/install.sh
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…back Multiple ToolSearch calls during deferred tool discovery now collapse to a single scrollback entry (the last probe), mirroring the blackbox reference's `isAbsorbedSilently` contract for ToolSearch. Intermediate discovery calls no longer produce repeated "Tools(…)" noise lines in the transcript. Also ships two other tested fixes from the collaborative branch: - fix(todo): accept Cursor/Claude TodoWrite shape using `content` instead of `title` (model_validator normalises before Pydantic validation) - feat(ui/slash): bare-segment matching for namespaced slash commands so `/designer` surfaces `/skill:designer-skill` without adding duplicate paths
Skills share a common prefix (pythinker-), so segment-prefix matching cannot disambiguate them. Add a lowest-priority fuzzy subsequence tier on the bare segment (gated at 2+ chars) so a misspelled distinctive word like /gurd surfaces /skill:pythinker-guard. Stronger prefix/alias/segment tiers still rank first; the inserted text stays the canonical command.
Reword the output-token-limit nudge to resume mid-thought without recap, and update the matching test assertion.
…actly The system-reminder injected after a truncated response now matches the blackbox reference verbatim per the AGENTS.md byte-exact text contract: 'Output token limit hit. Resume directly — no apology, no recap of what you were doing. Pick up mid-thought if that is where the cut happened. Break remaining work into smaller pieces.' Update the pinning test assertion to match the new text.
ToolSearch only makes sense with Anthropic's tool_reference/defer_loading beta. The type="anthropic" compat proxies (z.ai/GLM, Kimi, MiniMax, opencode) point at their own endpoints that don't forward that beta, and non-Anthropic providers don't have it at all. Offering ToolSearch there is noise: it re-lists already-visible tools, and weaker tool-callers (observed: GLM-5.2) loop on it forever instead of calling tools directly. Add supports_deferred_tool_search() (genuine api.anthropic.com host + non- haiku, with ENABLE_TOOL_SEARCH override) mirroring the reference's gate, and hide ToolSearch in PythinkerToolset._is_tool_visible when unsupported. The tool stays registered so a /model switch to Claude re-enables it instantly. Rendering is intentionally unchanged (compact absorber kept, not reverted to the reference's verbose generic card).
Root-cause follow-up to the ToolSearch gate: pythinker emits no defer_loading and ToolSearch only searches already-visible, already-callable tools (it unlocks nothing). The old description told the model that "deferred/hidden tool loading" might leave capabilities off the prompt — a false claim that primed weaker models to loop searching for tools they already had. Rewrite it to state plainly that it reveals nothing new and to call tools directly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/soul/toolset.py`:
- Around line 727-738: The ToolSearch visibility gate is only enforced in the
_is_tool_visible() method which controls tool advertisement, but the handle()
method still executes any tool from _tool_dict without checking this gate. To
prevent ToolSearch from executing on unsupported providers, add the same
supports_deferred_tool_search check that exists in _is_tool_visible() to the
handle() method before executing ToolSearch, ensuring the gate is enforced at
execution time not just in tool advertisement.
In `@tests/ui_and_conv/test_tool_search_suppression.py`:
- Around line 108-129: The test test_tool_search_does_not_cross_text_boundary
only sends one ToolSearch sequence before the assistant text, but the test name
and docstring indicate it should verify that ToolSearch groups are NOT collapsed
across a text boundary, which requires at least two ToolSearch sequences. Add a
second ToolSearch sequence after the TextPart message (using the same _ts_call
and _ts_result pattern with a different ID like "ts-2") but before the cleanup
call to properly test that these two separate ToolSearch groups remain
uncollapsed across the text boundary.
🪄 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: a0644178-44ec-4ba3-bd7b-243673461a72
📒 Files selected for processing (18)
CHANGELOG.mdsrc/pythinker_code/agents/default/agent.yamlsrc/pythinker_code/llm.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/soul/toolset.pysrc/pythinker_code/tools/todo/__init__.pysrc/pythinker_code/tools/tool_search/tool_search.mdsrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/tool_renderers/todo.pysrc/pythinker_code/ui/shell/visualize/_blocks.pysrc/pythinker_code/ui/shell/visualize/_live_view.pytests/core/test_default_agent.pytests/core/test_pythinkersoul_stuck_loop.pytests/core/test_tool_search_gating.pytests/tools/test_todo.pytests/tools/test_tool_search.pytests/ui_and_conv/test_slash_completer.pytests/ui_and_conv/test_tool_search_suppression.py
💤 Files with no reviewable changes (1)
- tests/core/test_default_agent.py
Run the lightweight space-column normalizer in the composing preview path and wrap long Severity/Location/What rows with continuation indent so wrap fragments no longer orphan at column 0. Document ToolSearch absence in the default-agent tool snapshot test.
elkaix
force-pushed
the
feat/tui-streaming-pr
branch
2 times, most recently
from
June 16, 2026 23:27
fc67296 to
2035baa
Compare
…cards Boundary-normalize content→title before SetTodoList validation, persist canonical title-only session state, and render validation failures as compact actionable errors instead of a fake persisted todo tree.
elkaix
force-pushed
the
feat/tui-streaming-pr
branch
from
June 16, 2026 23:28
2035baa to
e840d84
Compare
Treat blank titles as missing for Cursor-shape detection, compute indent from ANSI-stripped text, and show invalid args for malformed complete lists.
Resolve review findings across LSP, TUI streaming, toolset, and tests: fail-closed git check-ignore filtering, symbol boundary fixes, report-update refresh, markdown fence/audit hardening, ToolSearch execution gate, and lint/ pyright/test stability improvements including import sort in visualize.
…h noise Allow git check-ignore exit 128 (not a repository) so LSP location filtering works in tmp_path tests and non-git workspaces while keeping fail-closed on real check-ignore errors. Hide fuzzy slash completions when a stronger prefix/alias match exists so /mo resolves to /model only.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pythinker_code/ui/shell/markdown/audit.py (2)
90-94: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse generic-alias factory for Pyright strict mode.
Per codebase conventions for frozen/slotted dataclasses, prefer
default_factory=dict[str, str]overlambda: {}to avoid PyrightreportUnknownVariableTypeerrors in strict mode.Proposed fix
`@dataclass`(slots=True) class _ParityItem: title: str - fields: dict[str, str] = field(default_factory=lambda: {}) + fields: dict[str, str] = field(default_factory=dict[str, str])Based on learnings: "For Python 3.9+ in this repo (when using Pyright strict mode), prefer using generic-alias constructors as dataclass default factories."
🤖 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/markdown/audit.py` around lines 90 - 94, In the _ParityItem dataclass, replace the `default_factory=lambda: {}` with `default_factory=dict[str, str]` for the fields attribute to comply with Pyright strict mode and avoid reportUnknownVariableType errors. Use the generic-alias constructor directly instead of a lambda function.Source: Learnings
16-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the developer-local absolute path from shipped code.
Line 18-19 contains
/Users/panda/Projects/active/Projects/pythinker-code-main/which is a local machine path that won't exist on user systems and leaks internal development structure. Only the relativesrc/pythinker_code/prefix is universally applicable.Proposed fix
PROJECT_PATH_PREFIXES: tuple[str, ...] = ( "src/pythinker_code/", - "/Users/panda/Projects/active/Projects/pythinker-code-main/src/pythinker_code/", - "/Users/panda/Projects/active/Projects/pythinker-code-main/", )🤖 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/markdown/audit.py` around lines 16 - 20, The PROJECT_PATH_PREFIXES tuple contains developer-local absolute paths that will not exist on user systems and expose internal development structure. Remove the two hard-coded paths `/Users/panda/Projects/active/Projects/pythinker-code-main/src/pythinker_code/` and `/Users/panda/Projects/active/Projects/pythinker-code-main/` from the tuple, keeping only the relative path `src/pythinker_code/` which is universally applicable across different development and production environments.
🤖 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/tools/lsp/tool.py`:
- Around line 402-404: The code at line 403 treats all `git check-ignore` exit
code 128 as "outside a git repo" and returns safely, but git uses 128 for
various fatal errors beyond just missing repositories. This silently bypasses
ignored-path filtering for real errors. You must disambiguate by inspecting the
stderr output to confirm it's specifically the "not a git repository" error
message (typically containing "not a git repository" or similar) before
returning True. For other fatal errors producing exit code 128, raise an
exception or handle them as actual dependency failures rather than treating them
as safe defaults, ensuring degraded behavior is explicit rather than hidden.
In `@src/pythinker_code/tools/todo/__init__.py`:
- Around line 84-90: The _normalize_todo_write_args model validator currently
skips normalization when the input is a JSON-encoded string, returning it as-is.
This causes the normalization logic (which handles converting the content field
to title) to be bypassed for JSON string inputs. Fix this by detecting when data
is a JSON string, parsing it to a dict first, normalizing the parsed dict using
the normalize_set_todo_list_args function, and then returning the normalized
result. This ensures normalization is applied consistently whether todos are
provided as a dict or JSON string.
In `@tests/ui_and_conv/test_tui_streaming_phase0.py`:
- Around line 45-47: Replace the polling loop that checks the deadline and
live.update.call_count with asyncio.Event for more idiomatic signaling. Create
an asyncio.Event instance before the loop, use it to wait for the update call
instead of polling with the while loop and asyncio.sleep, and ensure the event
is set when live.update is called (this may require setting it in a callback or
mock side_effect). This follows Ruff's ASYNC110 rule and provides cleaner
wait-loop semantics than the current polling approach with the deadline timeout.
---
Outside diff comments:
In `@src/pythinker_code/ui/shell/markdown/audit.py`:
- Around line 90-94: In the _ParityItem dataclass, replace the
`default_factory=lambda: {}` with `default_factory=dict[str, str]` for the
fields attribute to comply with Pyright strict mode and avoid
reportUnknownVariableType errors. Use the generic-alias constructor directly
instead of a lambda function.
- Around line 16-20: The PROJECT_PATH_PREFIXES tuple contains developer-local
absolute paths that will not exist on user systems and expose internal
development structure. Remove the two hard-coded paths
`/Users/panda/Projects/active/Projects/pythinker-code-main/src/pythinker_code/`
and `/Users/panda/Projects/active/Projects/pythinker-code-main/` from the tuple,
keeping only the relative path `src/pythinker_code/` which is universally
applicable across different development and production environments.
🪄 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: 0502b47c-0257-48ce-b980-7ae79cc6d416
📒 Files selected for processing (29)
CHANGELOG.mdsrc/pythinker_code/lsp/client.pysrc/pythinker_code/lsp/recommend.pysrc/pythinker_code/soul/toolset.pysrc/pythinker_code/tools/lsp/formatters.pysrc/pythinker_code/tools/lsp/symbol_context.pysrc/pythinker_code/tools/lsp/tool.mdsrc/pythinker_code/tools/lsp/tool.pysrc/pythinker_code/tools/todo/__init__.pysrc/pythinker_code/ui/shell/markdown/audit.pysrc/pythinker_code/ui/shell/markdown/fences.pysrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/tool_renderers/agent.pysrc/pythinker_code/ui/shell/tool_renderers/background.pysrc/pythinker_code/ui/shell/tool_renderers/todo.pysrc/pythinker_code/ui/shell/tool_renderers/tool_search.pysrc/pythinker_code/ui/shell/visualize/__init__.pysrc/pythinker_code/ui/shell/visualize/_blocks.pysrc/pythinker_code/ui/shell/visualize/_live_view.pysrc/pythinker_code/ui/theme/__init__.pysrc/pythinker_code/utils/rich/markdown.pytests/core/test_default_agent.pytests/tools/test_lsp_client.pytests/tools/test_todo.pytests/ui_and_conv/test_md_normalization_matrix.pytests/ui_and_conv/test_streaming_content_block.pytests/ui_and_conv/test_tool_search_suppression.pytests/ui_and_conv/test_tui_card_tool_renderers.pytests/ui_and_conv/test_tui_streaming_phase0.py
…x empty-diagnostic clear
- Add table of all 9 LSP operations with their wire method names to lsp.md
- Document plugin server config schema (.lsp.json / lspServers) with all fields,
defaults, constraints, and ${VAR} / ${VAR:-default} env expansion including
PYTHINKER_PLUGIN_ROOT / PYTHINKER_PLUGIN_DATA built-ins
- Note .gitignore filtering on reference/definition/implementation/workspaceSymbol results
- Fix DiagnosticRegistry to clear stale pending state on empty publishDiagnostics
payloads (LSP "clear all diagnostics for this URI" signal)
3 tasks
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
LspService,LspClient,LspServerManager,DiagnosticRegistry, passiveLspDiagnosticsInjectionProviderthat injects pending diagnostics after file edits; 63 tests across client, manager, plugin discovery, tool, and diagnostics hooksui/theme.pyinto a package (spec,palettes,registry,resolver,adapters/) with typedTuiTokensdataclass,TUI_TOKEN_NAMESfrozenset,pythinker-xdark/light palette port, andsecondaryCoreToken/code-themepicker,/best-practices), blink motion, spacing primitives, and streaming content block renderingUsageActivityPanelwith per-provider token burn bars and rate-limit overlaypythinker_report_markdown()withreport_markdown_style_overrides()so only H1 headings render bold;REPORT_FILE_MARKERglyph for finding locations;secondarytoken for scope/note text; pill-background summary line; section header replacesRule(severity)test_lsp_provider_registered_in_subagent_soulassertsLspDiagnosticsInjectionProvideris wired for subagent-role souls; port-status ledger updatedTest plan
make check-pythinker-code— ruff + pyright greenmake test-pythinker-code— all unit tests pass (63 LSP tests, TUI theme contract, streaming, report, usage activity)make test— full suite greenSummary by CodeRabbit
Release Notes
New Features
/usage daily|weekly|cumulativetoken-activity cards./theme codewith syntax-theme selection and preview.UI/UX Improvements
Bug Fixes