feat(agent): MCP prompts, recall windows, git injection, and compaction hardening - #155
Merged
Conversation
Phase 0 inventory for docs/superpowers/plans/agent_enhancment.md: whole-source area map, 61-task status table, de-scope proofs, and phase gates. Phase 1 safety tasks marked done after verify-existing audit.
Add GitStatusInjectionProvider reusing collect_git_context, gated by the new git_status_injection config key (default on). Includes stale-snapshot wording and focused provider tests.
Stop retrying failed proactive compaction blindly by surfacing an explicit handoff, and publish MCP tools after concurrent connection in configured server order so duplicate names resolve deterministically.
Preserve original tool inputs for hook events, pin compaction tool-call pairing, and record the exclusive-shell cascade decision in the port ledger.
Let Recall read sanitized transcript slices by rendered message offset and count, so agents can retrieve precise prior-session context without loading an entire transcript.
Record the verified Phase 3 memory, recall, skill resource, frontmatter, and plugin-scope decisions against the focused tests that cover them.
Expose MCP prompt templates as a read-only tool that renders server prompt messages as untrusted content, and record the verified MCP resource and lifecycle rows.
Keep ExitPlanMode aligned with plan-mode reminders so written plans include concrete verification before user approval.
Update the port ledger with existing trace, stuck-loop, and max-step handoff coverage while leaving cassette and eval work explicit.
Mark plan-mode restrictions, usage roll-up, and suggestion accept flow as verify-existing with explicit remaining gaps from the phase audits.
When CompactionBegin fires, snapshot the pinned todo list (all statuses, elapsed_s=0) and pass it as a renderable into _CompactionBlock. The block now renders the todo rows (⎿ header + indented items) in place of the static tip; the tip remains as a fallback when no todos exist. No change to the spinner animation, title prefix, or progress bar.
Normalize MCP server keys with collision errors, inject docker --rm on mcp.json load, sanitize telemetry tool labels, and add suggestion prefill, media caps, plan verification warnings, markdown agent fields, and eval budget gating.
Normalize MCP CLI lookups and add per-server disconnect/reconnect/refresh. Fix compaction failures below threshold, wire/ACP max-steps handoff, and isolated tests_ai eval schema; extend recall search and shell test stability.
…t_status_injection Co-Authored-By: Mohamed Elkholy <mohamedmoradelkholy1@gmail.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 25 minutes and 4 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThis PR adds plugin marketplace and discovery support, MCP normalization and lifecycle commands, runtime handoff and injection controls, tool/UI behavior updates, and a self-contained eval schema and runner. ChangesPlugin platform and marketplace
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}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/tools/test_suggest.py (1)
47-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen the assertion to verify label and prefill appear in output.
The test only checks
rendered is not None, which is trivially true. It doesn't verify that the label "Review my changes" or prefill "/review" actually appear in the rendered output. As per coding guidelines, tests with trivially-true assertions should be flagged.🧪 Strengthen the test assertion
def test_suggestion_block_renders_label_and_prefill() -> None: from pythinker_code.ui.shell.visualize._blocks import _SuggestionBlock + from pythinker_code.ui.shell.components import render_plain block = _SuggestionBlock(Suggestion(label="Review my changes", prefill="/review")) rendered = block.compose() - assert rendered is not None + rendered_text = render_plain(rendered, width=100) + assert "Review my changes" in rendered_text + assert "/review" in rendered_text + assert "(Alt+S to accept)" in rendered_text🤖 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/tools/test_suggest.py` around lines 47 - 52, The test_suggestion_block_renders_label_and_prefill function has a weak assertion that only checks if rendered is not None, which does not verify that the actual label "Review my changes" or prefill "/review" values appear in the rendered output. Replace the trivial assertion with actual assertions that check the rendered string contains the label and prefill values passed to the Suggestion object, ensuring the test actually validates that _SuggestionBlock.compose() correctly includes these values in its output.Source: Coding guidelines
src/pythinker_code/tools/recall/__init__.py (1)
93-148:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the docstring to document the new windowing parameters.
The function signature added
message_offsetandmax_messages, but the docstring (lines 100–104) still only mentions budget truncation and doesn't explain the offset/limit behavior. Document the new parameters and their interaction with internal-message skipping.📝 Suggested docstring addition
def _render_transcript( context_file: Path, budget: int, *, message_offset: int = 0, max_messages: int | None = None, ) -> str: """Render a session's message log into a budgeted, sanitized transcript. Internal (``_``-prefixed) roles are skipped. Each message's text is sanitized; a block that trips the secret/injection scanner becomes ``[redacted]`` rather - than leaking or silently vanishing. Stops once the char budget is reached. + than leaking or silently vanishing. + + Args: + context_file: Path to the session's context.jsonl. + budget: Maximum character count for the rendered transcript. + message_offset: Skip this many rendered non-internal messages before returning content. + max_messages: Return at most this many rendered messages (None = unlimited). + + Stops once the char budget or max_messages is reached. """🤖 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/recall/__init__.py` around lines 93 - 148, The _render_transcript function signature now includes message_offset and max_messages parameters for windowing message selection, but the docstring does not document these parameters or explain how they interact with the logic that skips internal roles. Update the docstring to add descriptions of both parameters, explaining that message_offset skips the first N messages from the transcript, max_messages limits how many messages are included in the output, and that both parameters operate after internal-role filtering is applied.src/pythinker_code/cli/__init__.py (1)
273-294:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConvert MCP prepare/normalization failures to
typer.BadParameterin CLI parsing.Invalid
--mcp-config-file/--mcp-configpayloads that fail inprepare_mcp_config_dict(...)currently escape as internal exceptions instead of parameter-scoped CLI errors.Suggested fix
def _load_mcp_configs_from_cli_inputs( mcp_config_file: list[Path] | None, mcp_config: list[str] | None, ) -> list[Any]: @@ - from .mcp import prepare_mcp_config_dict + from pythinker_code.exception import MCPConfigError + from .mcp import prepare_mcp_config_dict @@ for conf in file_configs: try: configs.append(prepare_mcp_config_dict(json.loads(conf.read_text(encoding="utf-8")))) except json.JSONDecodeError as e: raise typer.BadParameter( f"Invalid JSON in MCP config file {conf}: {e}", param_hint="--mcp-config-file", ) from e except OSError as e: raise typer.BadParameter( f"Cannot read MCP config file {conf}: {e}", param_hint="--mcp-config-file", ) from e + except MCPConfigError as e: + raise typer.BadParameter( + f"Invalid MCP config in file {conf}: {e}", + param_hint="--mcp-config-file", + ) from e for conf in raw_mcp_config: try: configs.append(prepare_mcp_config_dict(json.loads(conf))) except json.JSONDecodeError as e: raise typer.BadParameter(f"Invalid JSON: {e}", param_hint="--mcp-config") from e + except MCPConfigError as e: + raise typer.BadParameter(f"Invalid MCP config: {e}", param_hint="--mcp-config") from eAs per coding guidelines, use explicit error contracts and avoid blurred failure states between invalid input and internal errors.
🤖 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 273 - 294, The code currently only catches json.JSONDecodeError and OSError exceptions, but prepare_mcp_config_dict() may raise additional exceptions when the JSON is valid but the MCP configuration itself is invalid. Wrap the prepare_mcp_config_dict() calls in both the file_configs loop and the raw_mcp_config loop with try-except blocks to catch any exceptions that prepare_mcp_config_dict() might raise (beyond JSONDecodeError), and convert those to typer.BadParameter exceptions with appropriate error messages indicating which parameter failed. This ensures all validation failures in prepare_mcp_config_dict() are properly reported as parameter errors rather than escaping as internal exceptions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pythinker_code/soul/toolset.py`:
- Around line 1284-1296: The tool unregistration in the
`_unregister_mcp_server_tools` method happens eagerly and immediately deletes
tools from the registry before refresh I/O completes, causing previously working
tools to be permanently lost if refresh fails. Additionally, when a tool is
unregistered, same-named fallback tools from other connected servers are not
republished, causing tool names to disappear. Refactor the MCP tool registration
logic to use a two-phase approach: first fetch and inventory the complete set of
tool changes needed across all operations, then atomically apply all changes and
rebuild the published MCP tools registry in configured order. Ensure that on any
failure, prior registrations are preserved intact and raise MCPRuntimeError
consistently from the operation boundary. This applies to both the unregister
operation shown and any related refresh or reconnect operations that currently
unregister tools eagerly.
- Around line 1231-1234: The call to _publish_connected_mcp_tools(runtime) on
line 1234 happens after the MCPRuntimeError is raised on line 1232, so it never
executes when any server fails to connect. This prevents already-connected
servers from being published and available in the session. Move the
_publish_connected_mcp_tools(runtime) call to execute before the raise statement
that throws the MCPRuntimeError, ensuring all successfully-connected servers are
published even when some servers fail to connect.
In `@src/pythinker_code/telemetry/names.py`:
- Around line 33-41: Add a comment above the head variable assignment in the
_bounded_mcp_label function to explain the slice length constraints. The comment
should clarify that the hardcoded slice lengths (16 for safe_server and 24 for
safe_tool) are chosen to ensure the resulting label fits within the
_TELEMETRY_TOOL_NAME_MAX limit (64 characters) when combined with the fixed
prefixes "mcp__", "__", and the 8-character digest suffix with underscore
separator.
In `@src/pythinker_code/ui/shell/slash.py`:
- Around line 2253-2299: The code currently silently ignores extra tokens
provided after the valid arguments to the /mcp command. Add validation after
splitting args to check for unexpected operand counts for each verb. For the
disconnect and reconnect verbs, verify that exactly two parts exist (verb and
server_name); if more are provided, print a usage error message and return. For
the refresh verb, verify that at most two parts exist (verb and optional
server_name); if more are provided, print a usage error message and return.
Place these validations early within the respective if blocks for disconnect,
reconnect, and refresh to catch and reject commands with extraneous arguments.
In `@src/pythinker_code/ui/shell/visualize/_interactive.py`:
- Around line 518-522: In the display_suggestion method, the staged prefill from
a previous suggestion can persist when a new suggestion has an empty prefill.
Modify the logic to always reset or clear the staged suggestion prefill
unconditionally (regardless of whether event.prefill is empty), and then
conditionally stage the new prefill only when event.prefill.strip() is
non-empty. This ensures stale prefill text from previous suggestions does not
remain active.
In `@src/pythinker_code/utils/mcp_names.py`:
- Around line 38-56: The docstring for the normalize_mcp_servers_in_config
function does not document that the input config dictionary is mutated in-place
when assigning the normalized servers back to config["mcpServers"]. Update the
docstring to explicitly state that the function modifies the input config dict
in-place, not just that it re-keys mcpServers entries. This clarifies the side
effect and the reason for returning the modified config object.
In `@tests/core/test_config.py`:
- Around line 54-55: The test_config.py file currently only covers the happy
path for the new config contracts around loop_control.max_compaction_failures
and git_status_injection. Add edge-case tests that assert rejection when
max_compaction_failures is set to an invalid value like 0 to ensure the
validation logic properly rejects out-of-bounds values, and add explicit test
cases that parse and validate git_status_injection set to false to ensure this
behavior is properly tested beyond the default snapshot. These additional test
cases will lock in the new contract validations and prevent regression on edge
cases and failure paths.
In `@tests/core/test_exit_plan_mode_verification.py`:
- Around line 10-12: The function
test_exit_plan_mode_description_requires_verification_section is defined in two
locations with overlapping but non-identical test coverage:
tests/core/test_exit_plan_mode_verification.py lines 10-12 checks only for
"Verification section" in the description, while
tests/tools/test_tool_descriptions.py lines 26-30 checks for both "Verification
section" and "smallest command, test, or check". To resolve this duplication,
either consolidate both tests into a single file with comprehensive assertions
covering all required description elements, or rename them to reflect their
distinct purposes (for example, rename one to test_verification_section_helper
and the other to test_exit_plan_mode_description_content) to make their
different scopes clear.
In `@tests/tools/test_suggest.py`:
- Around line 55-68: The test_accept_staged_suggestion_prefill_inserts_text
function is tightly coupled to internal implementation details by mocking the
private _session attribute and internal buffer structure. Refactor this test to
use the public API of CustomPromptSession instead: remove the direct
instantiation with __new__, avoid mocking private attributes like _session, and
create the session through its proper constructor. If necessary, verify behavior
through public methods or integration-style testing that exercises the actual
prompt session initialization and methods without bypassing internal
architecture.
In `@tests/ui_and_conv/test_shell_slash_commands.py`:
- Around line 252-256: The assertion in the test only checks for "MCP Tools" but
the actual rendered output from the source code at mcp_status.py:85 includes the
emoji character as "🔌 MCP Tools". The sanitize_ansi() function preserves
Unicode characters like emojis, so to properly validate the rendering and avoid
masking regressions, update the assertion from assert "MCP Tools" in output to
assert "🔌 MCP Tools" in output to match the full expected output and align
with the parallel test in test_shell_mcp_status.py.
---
Outside diff comments:
In `@src/pythinker_code/cli/__init__.py`:
- Around line 273-294: The code currently only catches json.JSONDecodeError and
OSError exceptions, but prepare_mcp_config_dict() may raise additional
exceptions when the JSON is valid but the MCP configuration itself is invalid.
Wrap the prepare_mcp_config_dict() calls in both the file_configs loop and the
raw_mcp_config loop with try-except blocks to catch any exceptions that
prepare_mcp_config_dict() might raise (beyond JSONDecodeError), and convert
those to typer.BadParameter exceptions with appropriate error messages
indicating which parameter failed. This ensures all validation failures in
prepare_mcp_config_dict() are properly reported as parameter errors rather than
escaping as internal exceptions.
In `@src/pythinker_code/tools/recall/__init__.py`:
- Around line 93-148: The _render_transcript function signature now includes
message_offset and max_messages parameters for windowing message selection, but
the docstring does not document these parameters or explain how they interact
with the logic that skips internal roles. Update the docstring to add
descriptions of both parameters, explaining that message_offset skips the first
N messages from the transcript, max_messages limits how many messages are
included in the output, and that both parameters operate after internal-role
filtering is applied.
In `@tests/tools/test_suggest.py`:
- Around line 47-52: The test_suggestion_block_renders_label_and_prefill
function has a weak assertion that only checks if rendered is not None, which
does not verify that the actual label "Review my changes" or prefill "/review"
values appear in the rendered output. Replace the trivial assertion with actual
assertions that check the rendered string contains the label and prefill values
passed to the Suggestion object, ensuring the test actually validates that
_SuggestionBlock.compose() correctly includes these values in its output.
🪄 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: c3a7b5da-e438-4b6d-af28-4b8e4067d480
⛔ Files ignored due to path filters (1)
tasks/blackbox-port-status.mdis excluded by!tasks/**
📒 Files selected for processing (60)
CHANGELOG.mdsrc/pythinker_code/acp/session.pysrc/pythinker_code/agents/default/agent.yamlsrc/pythinker_code/cli/__init__.pysrc/pythinker_code/cli/mcp.pysrc/pythinker_code/config.pysrc/pythinker_code/soul/agent.pysrc/pythinker_code/soul/dynamic_injections/git_status.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/soul/toolset.pysrc/pythinker_code/subagents/discovery.pysrc/pythinker_code/telemetry/names.pysrc/pythinker_code/tools/file/read_media.pysrc/pythinker_code/tools/mcp_resource/__init__.pysrc/pythinker_code/tools/mcp_resource/prompt_description.mdsrc/pythinker_code/tools/plan/__init__.pysrc/pythinker_code/tools/plan/description.mdsrc/pythinker_code/tools/recall/__init__.pysrc/pythinker_code/tools/recall/description.mdsrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/visualize/_blocks.pysrc/pythinker_code/ui/shell/visualize/_interactive.pysrc/pythinker_code/ui/shell/visualize/_live_view.pysrc/pythinker_code/utils/mcp_names.pysrc/pythinker_code/utils/media_limits.pysrc/pythinker_code/wire/server.pytests/core/test_agent_spec.pytests/core/test_config.pytests/core/test_default_agent.pytests/core/test_exit_plan_mode_verification.pytests/core/test_git_status_injection_provider.pytests/core/test_max_steps_handoff.pytests/core/test_mcp_cli_names.pytests/core/test_mcp_docker_rm.pytests/core/test_mcp_lifecycle.pytests/core/test_mcp_name_normalization.pytests/core/test_pythinkersoul_retry_recovery.pytests/core/test_simple_compaction.pytests/core/test_subagent_discovery.pytests/core/test_toolset.pytests/subagents/test_usage_rollup.pytests/telemetry/test_tool_name_sanitize.pytests/test_eval_harness_wiring.pytests/tools/test_mcp_resource.pytests/tools/test_recall.pytests/tools/test_suggest.pytests/tools/test_tool_descriptions.pytests/ui_and_conv/_md_contract_helpers.pytests/ui_and_conv/test_compaction_block.pytests/ui_and_conv/test_report.pytests/ui_and_conv/test_shell_slash_commands.pytests/utils/test_media_limits.pytests/utils/test_pyinstaller_utils.pytests_ai/eval_cases.example.jsontests_ai/eval_gate.pytests_ai/eval_schema.pytests_ai/scripts/run.pytests_e2e/eval_schema.py
- Fix was_parsed contract regression: empty {"findings": []} JSON blocks
now correctly return was_parsed=True; presence of any ```report block
short-circuits the markdown fallback entirely
- Narrow except clause to json.JSONDecodeError only; add isinstance guard
to replace the AttributeError/TypeError catch
- Add ponytail comment to _RE_REPORT_BLOCK explaining tolerant LLM fencing
- Wire _OL_ITEM_RE module constant into _loosen_tight_ordered_lists
- Add behavior tests for _loosen_tight_ordered_lists (fence passthrough,
already-spaced, mixed prose, unordered list unchanged)
- Add tests for JSON report block parsing (happy path, empty findings,
malformed JSON, multiple blocks, fallback suppression)
Update the port ledger for Phase 7 verify-existing and done items, surface Recall plan_slug matches in search output, rank source paths above test paths in @-completion, tighten reviewer ```report``` JSON parsing, and wire a session-wide live output-token accumulator into the shell spinner readout.
Introduce marketplace registry/CLI, install flow, session-scoped plugin enable policy (with external Claude/Codex opt-in), and plugin-contributed lifecycle hooks wired into app startup.
Split the activation policy into discover_external (safe model-invoked artifacts:
skills, commands, agents) and external_exec (executable artifacts: hooks, MCP).
Claude/Codex plugins auto-detect by default for safe artifacts while their
auto-running hooks/MCP stay opt-in, so users with those tools installed need no
symlink and get no redundancy (discovery reads in place and de-dups by name).
Harden from a deep-scan review:
- policy_from_config drops blank 'enabled' entries so a stray [""] can no longer
silently disable every plugin (all-blank -> enable all).
- narrow the hook-translation except to pydantic.ValidationError.
- collapse the three identical safe-artifact collectors into one helper.
- document the ${CLAUDE_PLUGIN_ROOT} expansion trust boundary (reference-exact).
Add an autouse conftest fixture neutralizing the external plugin roots so the
suite never reads the developer's real ~/.claude/~/.codex during discovery.
…-var expansion
Add the plugin enable/disable CLI surface, marketplace install path, and
${CLAUDE_PLUGIN_ROOT}-style plugin-var expansion, with the two-flag activation
policy (discover_external / external_exec) and a disabled list in config.
…ew findings - Publish connected MCP tools before raising on partial connect failure, so servers that connected stay callable when a sibling fails. - Rebuild the published MCP registry atomically on disconnect/refresh/reconnect: a failed refresh no longer drops live tools, and disconnecting a server falls its shadowed tool names back to other still-connected servers instead of orphaning them. - Convert refresh inventory timeouts/errors to MCPRuntimeError at the boundary. - Validate /mcp verb arity instead of silently using only the first operand. - Convert MCP config errors to typer.BadParameter in CLI parsing. - Stage suggestion prefill unconditionally so an empty prefill clears stale text. - Document telemetry slice budget, in-place mcpServers normalization, and recall windowing params; strengthen suggest/config/emoji assertions; drop a duplicate ExitPlanMode description test; add MCP lifecycle failure-path tests.
elkaix
force-pushed
the
feat/agent-behaviour-tweaks
branch
from
June 16, 2026 02:07
c69e9e2 to
49b08de
Compare
Member
Author
CodeRabbit / review findings addressed (commit
|
…osure) Plugins may declare 'dependencies' (name / name@marketplace, with forward-compat @^version suffixes stripped and object-form accepted). Two faithful adaptations of the reference, keyed by bare name since discovery de-dups by name: - Load time: verify_and_demote disables any plugin whose declared dependencies are not present+enabled (fixed-point; cascades; records per-plugin load errors distinguishing 'installed-but-disabled' from 'not-found'). - Install time: 'marketplace install' pulls transitive dependencies from the same marketplace, skipping already-installed ones, detecting cycles, and blocking cross-marketplace dependencies (install those from their own marketplace first).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_plugin_policy.py (1)
149-153:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInitialize policy state before each test, not only after.
Line 149-153 resets to default only at teardown. The first test can start with leaked global policy from earlier modules, creating order-dependent flakes. Set default in setup and restore with the returned token in teardown.
Suggested fix
`@pytest.fixture`(autouse=True) -def _reset_policy(): - yield - set_plugin_policy(PluginPolicy()) +def _reset_policy() -> None: + token = set_plugin_policy(PluginPolicy()) + try: + yield + finally: + reset_plugin_policy(token)🤖 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/test_plugin_policy.py` around lines 149 - 153, The `_reset_policy()` fixture only resets the policy state in teardown (after yield), leaving the global policy potentially polluted from previous tests during setup. Modify the fixture to initialize a default PluginPolicy before the test runs (before yield) and restore the original state after the test completes (after yield). Capture the token returned by the first set_plugin_policy call in setup and use it to restore the previous policy state in teardown, ensuring each test starts with a clean state regardless of test execution order.src/pythinker_code/cli/__init__.py (1)
277-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-object MCP JSON before normalization.
At Line 279 and Line 298,
json.loads(...)can return a non-dict value.prepare_mcp_config_dict(...)assumes a mapping and can throwAttributeError, which escapes yourtyper.BadParametercontract.Proposed fix
@@ for conf in file_configs: try: - configs.append(prepare_mcp_config_dict(json.loads(conf.read_text(encoding="utf-8")))) + parsed = json.loads(conf.read_text(encoding="utf-8")) + if not isinstance(parsed, dict): + raise typer.BadParameter( + f"Invalid MCP config in file {conf}: top-level JSON must be an object", + param_hint="--mcp-config-file", + ) + configs.append(prepare_mcp_config_dict(parsed)) @@ for conf in raw_mcp_config: try: - configs.append(prepare_mcp_config_dict(json.loads(conf))) + parsed = json.loads(conf) + if not isinstance(parsed, dict): + raise typer.BadParameter( + "Invalid MCP config: top-level JSON must be an object", + param_hint="--mcp-config", + ) + configs.append(prepare_mcp_config_dict(parsed))🤖 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 277 - 303, The json.loads() calls at both locations can return non-dict values, and prepare_mcp_config_dict() expects a mapping, so passing a non-dict will cause an unhandled AttributeError. Add validation immediately after json.loads() to ensure the parsed result is a dict (using isinstance check), and raise typer.BadParameter with an appropriate error message if it is not a dict. Also add AttributeError to the exception handlers as a catch-all to ensure any unexpected attribute access errors from prepare_mcp_config_dict() are properly converted to typer.BadParameter instead of escaping.
♻️ Duplicate comments (1)
src/pythinker_code/soul/toolset.py (1)
1446-1453:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound session-holder shutdown before closing MCP clients.
_stop_mcp_session_holder()awaits the holder task without a timeout. If the holder hangs while exitingasync with info.client,disconnect_mcp_server(),reconnect_mcp_server(), andcleanup()can block forever before_MCP_CLOSE_TIMEOUT_Sis ever applied.🛡️ Proposed fix
async def _stop_mcp_session_holder(self, info: MCPServerInfo) -> None: if info.session_stop is not None: info.session_stop.set() task = info.session_holder_task if task is None: return - with contextlib.suppress(asyncio.CancelledError): - await task + try: + await asyncio.wait_for(asyncio.shield(task), timeout=_MCP_CLOSE_TIMEOUT_S) + except TimeoutError: + logger.warning("MCP session holder did not stop within timeout; cancelling") + task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await task🤖 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/soul/toolset.py` around lines 1446 - 1453, The `_stop_mcp_session_holder()` method awaits the holder task indefinitely without a timeout, which can cause the entire shutdown sequence (affecting `disconnect_mcp_server()`, `reconnect_mcp_server()`, and `cleanup()`) to hang forever. Wrap the await of `info.session_holder_task` with a timeout using `_MCP_CLOSE_TIMEOUT_S` to ensure the session-holder shutdown is bounded before the MCP clients are closed.
🤖 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/plugin.py`:
- Around line 664-675: The code deletes files and directories at paths from
record.install_path without verifying they are within the plugin cache
directory, creating a security risk where corrupted metadata could delete
arbitrary user files. Before deleting each path in the loop where path.unlink()
and shutil.rmtree() are called, validate that the resolved path is actually
under the plugin cache root directory returned by plugin_cache_dir(). Only
proceed with the deletion if this boundary check passes, otherwise skip that
record to ensure deletions are constrained to the plugin cache.
In `@src/pythinker_code/plugin/artifacts.py`:
- Around line 85-88: The exception handler that catches malformed or unreadable
`.mcp.json` files (catching OSError and json.JSONDecodeError when
mcp_path.read_text is called) silently sets raw to None without logging the
error, making MCP capability loss invisible during plugin discovery. Add logging
to the except block to record the error details including the file path and the
reason for the failure, ensuring that degraded/fallback behavior is visible and
maintains failure-truthfulness as per coding guidelines.
In `@src/pythinker_code/plugin/install.py`:
- Line 53: The _SAFE_GIT_URL regex pattern at
src/pythinker_code/plugin/install.py line 53 currently allows insecure git
transport protocols (http:// and git://) which enable unauthenticated fetches of
executable plugin content. Remove http:// and git:// from the regex pattern in
_SAFE_GIT_URL, keeping only the secure protocols: https://, ssh://, git@, and
file://. This change at line 53 will propagate to the usage sites at lines 70-71
which reference this pattern, ensuring untrusted external content is fetched
only through secure channels.
- Around line 152-154: The use of shutil.rmtree(..., ignore_errors=True) on
destination paths that may be symlinks suppresses errors and can leave stale
installation or cache paths behind if removal fails. Remove the
ignore_errors=True parameter from all shutil.rmtree calls in the install.py file
that clean up destination paths before cloning or copying (specifically in the
blocks handling symlink cleanup before _git_clone and related operations).
Instead, implement proper error handling that logs or raises an exception when
removal fails, ensuring that unexpected cleanup failures are not silently
ignored per coding guidelines.
In `@src/pythinker_code/plugin/installed.py`:
- Around line 90-96: The record_install and remove_install functions perform
unsynchronized read-modify-write operations on the installed plugins registry,
creating a race condition where concurrent calls can lose updates. Add proper
locking synchronization around both the read (load_installed_plugins) and write
(save_installed_plugins) operations in both record_install and remove_install to
ensure these sequences are atomic and prevent concurrent requests from
overwriting each other's changes.
In `@src/pythinker_code/plugin/loader.py`:
- Around line 88-92: The filesystem iteration on line 88 using
`current.iterdir()` returns items in non-deterministic order, causing the list
comprehension that filters directories to produce variable ordering across
different hosts. This leads to the first-wins deduplication logic at line 134
potentially selecting different plugin versions depending on filesystem order.
Sort the children list after filtering to ensure deterministic ordering before
appending items to the stack, guaranteeing consistent plugin selection across
all environments.
In `@src/pythinker_code/plugin/marketplace.py`:
- Around line 139-143: The exception handler catching broad Exception in the
KnownMarketplaceEntry.model_validate call can mask real regressions and silently
drop entries. Replace the generic Exception catch with specific exception types
that are expected from validation failures such as ValidationError or other
Pydantic parse/validation exceptions. This will allow unexpected errors to
propagate while still gracefully handling expected validation failures, making
the code more resilient and easier to debug.
- Around line 155-174: The add_marketplace and remove_marketplace functions
perform read-modify-write operations on the marketplace registry without
synchronization, creating a race condition where concurrent calls can overwrite
each other's changes. Add file-level locking around the read-modify-write
sections in both functions to ensure atomic updates to the marketplace registry.
Acquire the lock before calling load_known_marketplaces in each function and
release it after save_known_marketplaces completes to prevent concurrent
processes from interfering with each other's changes.
- Around line 82-92: The marketplace source detection logic currently accepts
both HTTP and HTTPS URLs by checking trimmed.startswith(("http://",
"https://")). To prevent network attackers from tampering with marketplace
metadata in transit, modify this condition to only accept HTTPS URLs. If HTTP
support is needed for localhost-only development cases, add an explicit check
for that scenario separately before rejecting non-HTTPS sources.
In `@src/pythinker_code/plugin/policy.py`:
- Line 40: The docstring for the plugin policy (around line 40) states
"pythinker-only" but this contradicts the actual default behavior where
PluginPolicy.discover_external defaults to True (as shown on line 26), meaning
external plugins are enabled by default. Update the docstring to accurately
reflect that the default policy enables external plugin discovery, not just
pythinker-only plugins, so it matches the actual implementation defaults.
In `@src/pythinker_code/soul/agent.py`:
- Around line 646-651: The exception handler in the plugin MCP validation block
only catches pydantic.ValidationError, but prepare_mcp_config_dict(plugin_raw)
can raise other configuration errors that will cause agent loading to fail
instead of gracefully skipping malformed entries. Identify what exceptions
prepare_mcp_config_dict() can raise and update the except clause to catch those
exceptions in addition to pydantic.ValidationError, ensuring all errors from
both the prepare_mcp_config_dict() call and the MCPConfig.model_validate() call
are caught and logged as warnings to maintain fail-soft behavior.
In `@src/pythinker_code/soul/toolset.py`:
- Around line 1375-1408: The assignments to server_info.tools,
server_info.resources, and server_info.prompts at the end of the
_inventory_mcp_server method occur inside the async context manager before
__aexit__ completes. To make the inventory refresh side-effect-free until fully
successful, move these three assignment statements outside the async with block
(after it exits successfully) while keeping the local variable collections
(local_tools, resources, prompts) inside the block. This ensures that if any
error occurs during context exit or the outer wait_for timeout, the server_info
remains unmodified and the published registry stays consistent with the exposed
callable tools.
In `@src/pythinker_code/telemetry/names.py`:
- Around line 33-43: The _bounded_mcp_label function exports safe_server
directly in telemetry labels, which could leak PII or sensitive data if the
server parameter contains paths, hostnames, tokens, or credentials. Hash the
safe_server segment by computing its sha256 digest (similar to how the full
label is hashed) and use the first 8 hex characters as a safe replacement.
Update both the initial label construction and the head variable to use this
hashed server value instead of safe_server and safe_server[:16] respectively,
ensuring no unencrypted sensitive server data is exposed in telemetry output.
In `@src/pythinker_code/tools/mcp_resource/__init__.py`:
- Around line 133-142: The _render_prompt_messages function currently falls back
to stringifying the entire content object when the .text attribute is absent,
which can serialize unbounded nested structures into the prompt. Instead, apply
the same safe content-handling pattern already implemented in the
ReadMcpResource tool: after checking for content.text attribute, emit a bounded
placeholder like [binary content omitted: mime, size] when .text is not
available, rather than defaulting to the raw content object. This prevents
unexpectedly large or opaque content from being serialized into model context.
In `@src/pythinker_code/tools/mcp_resource/prompt_description.md`:
- Line 1: The markdown file prompt_description.md is missing a top-level H1
heading at the start, which violates the MD041 markdownlint rule. Add an H1
heading (using # syntax) on the first line before the current plain text
content. The heading should be descriptive and introduce the document topic
appropriately.
In `@src/pythinker_code/ui/shell/tool_renderers/agent.py`:
- Around line 121-135: The code at line 130+ assumes every findings item is a
dictionary with a `.get()` method, but a malformed payload like
`{"findings":"high"}` will crash at runtime. Additionally, the function returns
`was_parsed=True` even when the JSON structure is invalid, falsely indicating
successful parsing. Add validation after parsing the JSON to ensure findings is
actually a list/tuple and each finding is a dictionary before attempting to call
`.get("severity", "")` on it. Only return `was_parsed=True` when the payload has
been fully validated as a proper _ReportBlock structure with findings as a
sequence of dictionaries; if validation fails at any point, continue to the next
block without marking it as parsed.
In `@src/pythinker_code/ui/shell/visualize/_live_view.py`:
- Around line 911-913: The _pinned_todo_block method call in the compaction
rendering is using a hardcoded width value of 80 pixels, which does not adapt to
the actual terminal width and causes layout issues in both narrow and wide
terminals. Replace the hardcoded width=80 parameter with a call to
current_console_width() so that the compaction todo block dynamically matches
the same console width used in the live layout behavior.
- Line 870: The token-rate sample window `_turn_token_samples` is not being
cleared when a new turn snapshot is taken, causing it to retain samples from
prior turns which leads to skewed or negative token deltas. At both line 870 and
line 882 where the per-turn token baseline is reset via
`snapshot_output_tokens_for_turn()` and similar operations, also clear the
`_turn_token_samples` collection to ensure the sample window starts fresh for
each turn, preventing stale samples from affecting token rate calculations.
In `@src/pythinker_code/utils/media_limits.py`:
- Around line 11-15: The format_byte_limit function currently does not handle
non-positive (zero or negative) byte limit values, allowing invalid inputs to
produce misleading output like "0 KB" or negative values. Add an explicit guard
clause at the beginning of the format_byte_limit function to check if
limit_bytes is less than or equal to 0, and either raise a ValueError with a
clear error message or return an appropriate error representation (such as
"Invalid byte limit" or similar) before proceeding to the existing size
formatting logic. This ensures invalid inputs are rejected explicitly rather
than producing normal-looking but incorrect output.
In `@tests_ai/eval_gate.py`:
- Around line 47-50: The code at the eval_case lookup check is silently skipping
unknown case names with a continue statement, which masks contract mismatches
between report and eval cases. Replace the silent continue when eval_case is
None with explicit error handling that raises an exception or returns an error
status, making the mismatch visible and preventing the budget gate from
incorrectly passing when report/eval case contracts drift.
In `@tests_ai/eval_schema.py`:
- Around line 17-20: The integer fields max_tool_calls, max_total_tokens,
max_tool_errors, and max_steps in the eval_schema.py file currently allow
negative values which can distort gating outcomes. Apply Pydantic Field
constraints with ge=0 to each of these fields to reject negative values at the
schema level. This same constraint pattern should also be applied to similar
budget and metric fields in the other location mentioned (lines 37-41) to ensure
consistent validation across all such fields.
- Line 26: The schema_version field is currently typed as int, which allows any
integer value and silently accepts mismatched schema versions. Replace the int
type annotation with Literal using the expected schema version value (e.g.,
Literal[1]) to enforce version validation at the type level. This approach is
more idiomatic than using a custom model_validator and ensures incompatible eval
case payloads are rejected during deserialization of the schema_version field.
In `@tests_ai/scripts/run.py`:
- Around line 147-148: The load_eval_cases function call on line 147 does not
handle parse or validation exceptions, allowing tracebacks to bubble up to the
user instead of providing a clear CLI error message. Wrap the load_eval_cases
call in a try-except block that catches exceptions and provides an explicit,
actionable error message to the user, similar to how the load_report function
handles failures. The error should inform the user what went wrong with loading
the eval cases file and suggest corrective actions before exiting.
In `@tests/core/test_mcp_name_normalization.py`:
- Around line 23-28: The assertion at line 27 in the
test_normalize_bounds_overlong_names function is tautological because
normalized.endswith(normalized.split("_")[-1]) will always be true by
definition, so it cannot catch regressions in truncation or suffix behavior.
Replace this assertion with a meaningful check that validates the actual
expected behavior of the normalize_mcp_server_name function when handling
overlong names, such as verifying that a specific suffix or hash is preserved
and properly positioned in the truncated result.
In `@tests/core/test_pythinkersoul_retry_recovery.py`:
- Around line 627-628: Remove the monkeypatch.setattr calls patching the private
_step method at two locations: tests/core/test_pythinkersoul_retry_recovery.py
lines 627-628 and lines 671-673. Instead of mocking this internal implementation
detail, refactor these tests to assert observable outcomes such as provider call
counts, final history messages, stop reasons, or wire output to validate the
expected behavior without relying on internal method patching.
In `@tests/test_plugin_marketplace.py`:
- Line 76: The regex pattern in the pytest.raises match parameter on line 76
contains an unescaped dot character that matches any character instead of a
literal dot. Update the match string from "must be .json" to "must be \\.json"
to properly escape the dot in the regex pattern, ensuring the test only passes
when the error message contains the literal text "must be .json" and not
unintended variations.
---
Outside diff comments:
In `@src/pythinker_code/cli/__init__.py`:
- Around line 277-303: The json.loads() calls at both locations can return
non-dict values, and prepare_mcp_config_dict() expects a mapping, so passing a
non-dict will cause an unhandled AttributeError. Add validation immediately
after json.loads() to ensure the parsed result is a dict (using isinstance
check), and raise typer.BadParameter with an appropriate error message if it is
not a dict. Also add AttributeError to the exception handlers as a catch-all to
ensure any unexpected attribute access errors from prepare_mcp_config_dict() are
properly converted to typer.BadParameter instead of escaping.
In `@tests/test_plugin_policy.py`:
- Around line 149-153: The `_reset_policy()` fixture only resets the policy
state in teardown (after yield), leaving the global policy potentially polluted
from previous tests during setup. Modify the fixture to initialize a default
PluginPolicy before the test runs (before yield) and restore the original state
after the test completes (after yield). Capture the token returned by the first
set_plugin_policy call in setup and use it to restore the previous policy state
in teardown, ensuring each test starts with a clean state regardless of test
execution order.
---
Duplicate comments:
In `@src/pythinker_code/soul/toolset.py`:
- Around line 1446-1453: The `_stop_mcp_session_holder()` method awaits the
holder task indefinitely without a timeout, which can cause the entire shutdown
sequence (affecting `disconnect_mcp_server()`, `reconnect_mcp_server()`, and
`cleanup()`) to hang forever. Wrap the await of `info.session_holder_task` with
a timeout using `_MCP_CLOSE_TIMEOUT_S` to ensure the session-holder shutdown is
bounded before the MCP clients are closed.
🪄 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: 2af8c307-f7cd-49a4-a505-ab28eeba0a07
⛔ Files ignored due to path filters (3)
docs/en/customization/plugins.mdis excluded by!docs/**tasks/blackbox-port-status.mdis excluded by!tasks/**tasks/clean-code-guard-followups.mdis excluded by!tasks/**
📒 Files selected for processing (95)
AGENTS.mdCHANGELOG.mdsrc/pythinker_code/acp/session.pysrc/pythinker_code/agents/default/agent.yamlsrc/pythinker_code/app.pysrc/pythinker_code/cli/__init__.pysrc/pythinker_code/cli/mcp.pysrc/pythinker_code/cli/plugin.pysrc/pythinker_code/config.pysrc/pythinker_code/plugin/artifacts.pysrc/pythinker_code/plugin/directories.pysrc/pythinker_code/plugin/install.pysrc/pythinker_code/plugin/installed.pysrc/pythinker_code/plugin/integration.pysrc/pythinker_code/plugin/loader.pysrc/pythinker_code/plugin/manifest.pysrc/pythinker_code/plugin/marketplace.pysrc/pythinker_code/plugin/policy.pysrc/pythinker_code/prompt_templates.pysrc/pythinker_code/skill/__init__.pysrc/pythinker_code/soul/agent.pysrc/pythinker_code/soul/dynamic_injections/git_status.pysrc/pythinker_code/soul/live_tokens.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/soul/toolset.pysrc/pythinker_code/subagents/discovery.pysrc/pythinker_code/telemetry/names.pysrc/pythinker_code/tools/file/read_media.pysrc/pythinker_code/tools/mcp_resource/__init__.pysrc/pythinker_code/tools/mcp_resource/prompt_description.mdsrc/pythinker_code/tools/plan/__init__.pysrc/pythinker_code/tools/plan/description.mdsrc/pythinker_code/tools/recall/__init__.pysrc/pythinker_code/tools/recall/description.mdsrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/components/markdown.pysrc/pythinker_code/ui/shell/components/report.pysrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/tool_renderers/agent.pysrc/pythinker_code/ui/shell/visualize/_blocks.pysrc/pythinker_code/ui/shell/visualize/_interactive.pysrc/pythinker_code/ui/shell/visualize/_live_view.pysrc/pythinker_code/utils/mcp_names.pysrc/pythinker_code/utils/media_limits.pysrc/pythinker_code/wire/server.pytests/cli/test_plugin_marketplace_cli.pytests/conftest.pytests/core/test_agent_spec.pytests/core/test_config.pytests/core/test_default_agent.pytests/core/test_exit_plan_mode_verification.pytests/core/test_git_status_injection_provider.pytests/core/test_live_tokens.pytests/core/test_max_steps_handoff.pytests/core/test_mcp_cli_names.pytests/core/test_mcp_docker_rm.pytests/core/test_mcp_lifecycle.pytests/core/test_mcp_name_normalization.pytests/core/test_plugin_manager.pytests/core/test_pythinkersoul_retry_recovery.pytests/core/test_simple_compaction.pytests/core/test_subagent_discovery.pytests/core/test_toolset.pytests/subagents/test_usage_rollup.pytests/telemetry/test_tool_name_sanitize.pytests/test_eval_harness_wiring.pytests/test_plugin_install.pytests/test_plugin_integration.pytests/test_plugin_loader.pytests/test_plugin_manifest.pytests/test_plugin_marketplace.pytests/test_plugin_policy.pytests/tools/test_mcp_resource.pytests/tools/test_recall.pytests/tools/test_suggest.pytests/tools/test_tool_descriptions.pytests/ui_and_conv/_md_contract_helpers.pytests/ui_and_conv/test_compaction_block.pytests/ui_and_conv/test_file_completer.pytests/ui_and_conv/test_live_view_todos.pytests/ui_and_conv/test_markdown_guards.pytests/ui_and_conv/test_md_repair_characterization.pytests/ui_and_conv/test_report.pytests/ui_and_conv/test_review_findings_parser.pytests/ui_and_conv/test_shell_slash_commands.pytests/ui_and_conv/test_tui_transcript_enhancements.pytests/ui_and_conv/test_visualize_running_prompt.pytests/utils/test_media_limits.pytests/utils/test_pyinstaller_utils.pytests_ai/eval_cases.example.jsontests_ai/eval_gate.pytests_ai/eval_schema.pytests_ai/scripts/run.pytests_e2e/eval_schema.py
Plugins may declare userConfig options and reference them as ${user_config.KEY}
in MCP server configs and hook commands. Values come from [plugins.options.<plugin>]
config (carried on PluginPolicy). An artifact that references an unconfigured
option is skipped fail-soft rather than run with a blank.
Scope note: content substitution (skill/agent/command markdown), the
PYTHINKER_PLUGIN_OPTION_* hook env vars, keychain-backed sensitive storage, and
the interactive enable-time prompt are not ported — each needs changes outside the
plugin package (artifacts carry no plugin id; HookDef has no env field). Documented
under 'Not yet ported'.
- install: reject plaintext http://, git:// git transports (supply-chain); safe path removal that handles leftover symlinks instead of ignore_errors. - marketplace: reject plaintext http:// sources; narrow per-entry parse catch to ValidationError. - artifacts: log unreadable .mcp.json instead of silently dropping MCP servers. - loader: sort discovery iteration so first-wins dedupe is deterministic (C11). - cli uninstall: constrain deletions to the plugin cache root (corrupted metadata can't delete arbitrary files; symlinks unlinked, never followed). - soul/agent: plugin MCP fail-soft branch also catches normalization shape errors. - policy: correct the default-policy docstring (external safe artifacts auto-detect). Deferred with documented ceiling: cross-process locking for installed/marketplace registry writes (saves are atomic; only a rare concurrent-CLI race remains, and there is no portable lock primitive to reuse).
- telemetry/names: hash the user-named MCP server segment so server names can't leak paths/tokens/PII into telemetry labels. - mcp_resource: render non-text prompt message content as a bounded '[binary content omitted]' placeholder (same safe handling as ReadMcpResource); add the H1 the tool description was missing. - tool_renderers/agent: validate report JSON shape before counting (no crash on non-list findings / non-object items) and only mark was_parsed on a valid JSON object — a malformed block is reported as unparsed, not false success. - live_view: reset the per-turn token-rate sample window on each turn snapshot; use the real console width for the compaction todo block instead of width=80. - utils/media_limits: explicit guard for non-positive byte limits. - soul/toolset: assign MCP inventory atomically after discovery succeeds. - eval harness: ge=0 on budget/metric fields, Literal[1] schema_version (fail closed on mismatch), guard non-list report 'cases', actionable --eval-cases load error. - tests: real hash-suffix assertion for overlong MCP names; corrected reviewer parser tests to the fixed malformed/wrong-shape contract.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
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/cli/plugin.py (1)
665-683:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake uninstall fail truthfully when filesystem cleanup fails.
At Line [665], install records are removed before on-disk cleanup. At Line [682],
ignore_errors=Truesuppresses deletion failures, but Line [686] still reports success. This can leave orphaned plugin files while the registry says the plugin is uninstalled.Suggested fix
- records = load_installed_plugins().get(f"{plugin}@{marketplace}", []) - if not remove_install(plugin, marketplace): + records = load_installed_plugins().get(f"{plugin}@{marketplace}", []) + if not records: typer.echo(f"'{plugin}@{marketplace}' is not installed", err=True) raise typer.Exit(1) @@ + cleanup_errors: list[str] = [] for record in records: path = Path(record.install_path) @@ if path.is_symlink(): path.unlink(missing_ok=True) elif path.is_dir(): - shutil.rmtree(path, ignore_errors=True) + try: + shutil.rmtree(path) + except OSError as exc: + cleanup_errors.append(f"{path}: {exc}") + + if cleanup_errors: + for err in cleanup_errors: + typer.echo(f"Error: failed to remove plugin path: {err}", err=True) + raise typer.Exit(1) + + remove_install(plugin, marketplace)As per coding guidelines, never return success after a required internal step failed (C01).
🤖 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/plugin.py` around lines 665 - 683, The uninstall operation removes install records from the registry before cleaning up files on disk, but the shutil.rmtree call at line 682 uses ignore_errors=True which silently suppresses deletion failures. This allows the function to report success even when orphaned plugin files remain on disk. Remove the ignore_errors=True parameter from shutil.rmtree and instead track whether any deletion operation fails during the loop (for both path.unlink and shutil.rmtree calls). If any file deletion fails, log an error message and raise a typer.Exit with a non-zero exit code to ensure the command reports failure rather than success, preventing false claims that the uninstall completed successfully when files are left behind.Source: Coding guidelines
♻️ Duplicate comments (1)
tests_ai/eval_gate.py (1)
44-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed on malformed/unknown report cases instead of silently skipping.
At Line [46] and Line [53], silent
continuecan hide report/case contract drift and undercount failures.Proposed surgical fix
def gate_report(report: list[dict[str, object]], cases: list[EvalCase]) -> list[EvalVerdict]: @@ - for entry in report: + unknown_case_names: set[str] = set() + for idx, entry in enumerate(report): report_cases = entry.get("cases", []) if not isinstance(report_cases, list): - continue # malformed entry: "cases" must be a list — skip it + raise ValueError(f"Malformed report entry at index {idx}: 'cases' must be a list") for case in report_cases: @@ eval_case = cases_by_name.get(name) if eval_case is None: - continue + if name: + unknown_case_names.add(name) + continue verdicts.append(score_eval_case(eval_case, observed_from_report_case(case))) + if unknown_case_names: + unknown = ", ".join(sorted(unknown_case_names)) + raise ValueError(f"Unknown eval case name(s) in report: {unknown}") return verdictsAs per coding guidelines, unexpected states and contract mismatches must be explicit rather than silently ignored.
🤖 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_ai/eval_gate.py` around lines 44 - 53, Replace the silent continue statements with explicit failure handling to detect contract mismatches and report/case drift. At the point where you check if case is not a dict (the isinstance check), instead of silently continuing, raise an exception with a descriptive error message about the malformed case entry. Similarly, at the point where you check if eval_case is None after the cases_by_name.get lookup, instead of silently continuing, raise an exception with a message indicating the case name was not found in the expected cases mapping. This ensures unexpected or missing cases fail explicitly rather than being silently skipped and potentially undercounting failures.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pythinker_code/config.py`:
- Around line 1063-1070: The plugins.options field is not scope-locked and can
be set in project or local configuration, which allows repo-controlled config to
influence executable plugin parameters through substitution into MCP server
configs and hook commands. This is a security concern as external/untrusted
config should not control sensitive runtime values. Add scope-locking to the
options field definition to restrict it to user scope only, preventing
project-level and local-level configuration from setting these values.
In `@src/pythinker_code/plugin/install.py`:
- Around line 351-371: The `_install_with_deps()` function calls
`_materialize_and_record()` before validating all dependencies, which can leave
the plugin partially installed if dependency validation fails. To fix this,
either validate all dependencies from the manifest and recursively check the
entire dependency closure before calling `_materialize_and_record()` at line
352, or after calling `_materialize_and_record()`, wrap the dependency loop
starting at line 354 in a try-except block that rolls back the recorded plugin
(by removing the install path and deleting the record) before re-raising any
MarketplaceError or exception from the recursive `_install_with_deps()` calls to
ensure atomicity and prevent partial installs.
In `@src/pythinker_code/plugin/integration.py`:
- Around line 137-145: The validation of `${user_config.*}` placeholders is
being skipped entirely when `manifest.user_config` is falsy because the
substitution logic is guarded by `if options is not None`. This allows
unresolved placeholders to silently pass through into executable artifacts. You
must ensure that placeholder validation always occurs regardless of whether
user_config is configured. Modify the logic at line 137-145 (anchor site) to
validate for unresolved `${user_config.*}` placeholders even when options is
None, and apply the same validation pattern at the sibling sites at lines
209-219 and 248-249. The validation should detect and raise an error when these
placeholders remain unresolved, rather than allowing them to pass through
silently.
In `@src/pythinker_code/plugin/manifest.py`:
- Around line 84-93: The `_normalize_dependencies()` function preserves
surrounding whitespace in dependency names (e.g., " lib " or {"name": "lib "}),
but downstream matching is exact-name based. Trim whitespace from all dependency
identifiers: apply `.strip()` to the string entry after the regex substitution
in the first branch, apply `.strip()` to the name variable extracted from the
dict entry, and apply `.strip()` to the marketplace value before using it in the
formatted string on the last line of the function.
In `@tests_ai/scripts/run.py`:
- Around line 149-156: The gate_report() call is currently outside the
try-except error boundary that wraps load_eval_cases(). Move the gate_report()
call inside a try-except block that catches appropriate exceptions (likely
OSError, ValidationError, or similar) and converts them to a user-facing
SystemExit error with a clear message that includes the actual error details,
similar to the pattern used for the load_eval_cases() error handling above it.
In `@tests/test_plugin_dependency.py`:
- Around line 99-105: The test function
test_install_blocks_cross_marketplace_dependency currently only verifies that a
MarketplaceError is raised during the cross-marketplace dependency installation,
but it does not assert that the system properly rolled back after the failure.
Add an assertion after the pytest.raises context manager completes to verify
that "app@mk" was not left installed or recorded in the plugin state, ensuring
that partial-install scenarios cannot regress undetected.
In `@tests/test_plugin_integration.py`:
- Around line 252-278: The test function
`test_plugin_hook_defs_substitutes_user_config` currently only verifies the
success case where the user config option is provided. Add a companion test case
that verifies the fail-soft behavior when required user_config values are
missing. Create a test that calls `integration.plugin_hook_defs` with a
`PluginPolicy` that omits the required "flag" option from the options
dictionary, then assert that the hook is properly skipped (either empty or not
present in the returned definitions), ensuring both the happy-path and
failure-path scenarios are covered as required by coding guidelines.
---
Outside diff comments:
In `@src/pythinker_code/cli/plugin.py`:
- Around line 665-683: The uninstall operation removes install records from the
registry before cleaning up files on disk, but the shutil.rmtree call at line
682 uses ignore_errors=True which silently suppresses deletion failures. This
allows the function to report success even when orphaned plugin files remain on
disk. Remove the ignore_errors=True parameter from shutil.rmtree and instead
track whether any deletion operation fails during the loop (for both path.unlink
and shutil.rmtree calls). If any file deletion fails, log an error message and
raise a typer.Exit with a non-zero exit code to ensure the command reports
failure rather than success, preventing false claims that the uninstall
completed successfully when files are left behind.
---
Duplicate comments:
In `@tests_ai/eval_gate.py`:
- Around line 44-53: Replace the silent continue statements with explicit
failure handling to detect contract mismatches and report/case drift. At the
point where you check if case is not a dict (the isinstance check), instead of
silently continuing, raise an exception with a descriptive error message about
the malformed case entry. Similarly, at the point where you check if eval_case
is None after the cases_by_name.get lookup, instead of silently continuing,
raise an exception with a message indicating the case name was not found in the
expected cases mapping. This ensures unexpected or missing cases fail explicitly
rather than being silently skipped and potentially undercounting failures.
🪄 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: 0c0e7dce-60bf-41d0-adb3-a66710a2d7c7
⛔ Files ignored due to path filters (1)
docs/en/customization/plugins.mdis excluded by!docs/**
📒 Files selected for processing (31)
CHANGELOG.mdsrc/pythinker_code/app.pysrc/pythinker_code/cli/plugin.pysrc/pythinker_code/config.pysrc/pythinker_code/plugin/artifacts.pysrc/pythinker_code/plugin/dependency.pysrc/pythinker_code/plugin/install.pysrc/pythinker_code/plugin/installed.pysrc/pythinker_code/plugin/integration.pysrc/pythinker_code/plugin/loader.pysrc/pythinker_code/plugin/manifest.pysrc/pythinker_code/plugin/marketplace.pysrc/pythinker_code/plugin/options.pysrc/pythinker_code/plugin/policy.pysrc/pythinker_code/soul/agent.pysrc/pythinker_code/soul/toolset.pysrc/pythinker_code/telemetry/names.pysrc/pythinker_code/tools/mcp_resource/__init__.pysrc/pythinker_code/tools/mcp_resource/prompt_description.mdsrc/pythinker_code/ui/shell/tool_renderers/agent.pysrc/pythinker_code/ui/shell/visualize/_live_view.pysrc/pythinker_code/utils/media_limits.pytests/core/test_config.pytests/core/test_mcp_name_normalization.pytests/test_plugin_dependency.pytests/test_plugin_integration.pytests/test_plugin_marketplace.pytests/ui_and_conv/test_review_findings_parser.pytests_ai/eval_gate.pytests_ai/eval_schema.pytests_ai/scripts/run.py
Docs build failed because an inline-code span `pythinker plugin
enable/disable <name>` wrapped across a line break, so VitePress's Vue
compiler read `<name>` as an unclosed HTML tag. Keep the span on one line
and regenerate the synced docs changelog.
Address unresolved review findings:
- Lock the plugin install / marketplace registries' read-modify-write
cycles with a shared cross-process file_lock helper (no lost updates on
concurrent CLI writers).
- Make MCP inventory refresh side-effect-free: _inventory_mcp_server now
returns the discovered inventory and the caller assigns it only after
the awaited call succeeds, so a timeout/__aexit__ failure can't leave
the published registry inconsistent with the callable tools.
- Roll back partially materialized plugins when dependency resolution
fails, so a failed install never leaves partial on-disk/registry state.
- Always validate ${user_config.*} placeholders (even when a manifest
declares no userConfig) so an unresolved placeholder fails soft instead
of reaching an executable artifact literally.
- Scope-lock plugins.options to user config; it feeds executable plugin
MCP/hook artifacts and must not be repo-controlled.
- Only mark a reviewer ```report block parsed when findings is a list;
raise on unknown eval-case names and wrap gate_report in the CLI error
boundary; trim plugin dependency identifiers before exact-name matching.
Add/adjust tests for rollback, fail-soft hook substitution, the unknown
eval-case error, and the new report-parse and MCP-refresh contracts.
This was referenced Jul 6, 2026
This was referenced Jul 20, 2026
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
InvokeMcpPrompttool — agents can now invoke prompts exposed by connected MCP servers; bundled with list/read in the tool manifest and PyInstaller datagit_status_injectionflag injects a bounded git snapshot into the root agent's startup promptmax_compaction_failuresconfig cap + fail-open handoff on proactive compaction failure; in-progress todos replace random tips in the compaction blockTest plan
make check-pythinker-codepasses (ruff + pyright)make test-pythinker-codepasses (includes updated snapshots forInvokeMcpPrompt,max_compaction_failures,git_status_injection, andprompt_description.md)pythinker mcpprompt invocation against a locally connected servergit_status_injection = trueSummary by CodeRabbit
New Features
/mcp disconnect|reconnect|refresh) with live status/tool updates, plus connected-server MCP prompt invocation.${user_config...}option substitution.Bug Fixes
Improvements