feat: Agent phase-0 enhancements + review/login hardening - #89
Merged
Conversation
Implements the Phase 0 (high-value, low-risk) batch from the agent gap analysis in tasks/pythinker-agent-enhancement-plan.md, derived from a comparison against the opencode/Kilo Code reference and current agent best practices. Phases 1-5 (~25 items) remain. - subagent plan-mode inheritance (security): a coder/implementer subagent spawned under a plan-mode root now inherits the read-only plan profile, closing a bypass where it could run mutating shell or side-effecting external/MCP tools. The downgrade only affects mutating profiles, so already-read-only roles are not loosened (soul/permission.py). Verified at the resolver layer and via Shell regression + positive-control tests. - prompt-injection defense: declare <untrusted_data> semantics in the system prompt so the existing structural wrapper (utils/trust.py) is honored - treat wrapped tool output as data, never instructions. - tool descriptions: bring 7 stubs (think, web search/fetch, write, replace, grep, skill) up to the read.md/glob.md bar with when-to / when-not-to-use and escalation/scoping hints. - delegation effort scaling: add an explicit agent-count rubric and anti-sprawl rationale to the Agent tool description and orchestration prompt. - plan mode: require a Verification section in the written plan (plan-mode reminders + EnterPlanMode workflow text). - todo: add a cancelled status across all layers + renderer so obsolete tasks stay visible instead of being silently dropped. - telemetry: emit prompt-cache read/creation tokens and a finish-reason proxy on the LLM span, plus new cache-token metric counters. Verification: ruff check + format clean, pyright 0 errors, full unit suite 4532 passed / 0 failed (+1 resolver test = 4533).
Phase 1 / injdef-2 from the agent enhancement plan. The system prompt now declares <untrusted_data> semantics (Phase 0 / injdef-1); this routes the two highest-volume external-content channels through that wrapper so injected directives in tool output are treated as data, not instructions. - ToolResultBuilder.mark_untrusted(): wraps the already-truncated joined buffer once at ok()/error() time, so the closing tag can never be cut by truncation and harness-authored result messages stay outside the wrapper. Empty output is left unwrapped. - Shell: stdout/stderr (the largest untrusted vector) is wrapped on the model-facing result; the live UI stream via emit_output_part stays untagged. - WebSearch: result block wrapped, mirroring FetchURL (which already wraps). - Grep was evaluated and deliberately NOT wrapped: its structured path:line:content output is parsed positionally and only shows fragments of files whose full read (ReadFile) is already wrapped, so the marginal value does not justify changing that output contract. Tests: new positive wrap assertions for Shell and WebSearch; shell snapshot tests unwrap the random nonce to stay deterministic.
…stop
Phase 1 / permgate-1. 'Approve for session' was keyed on the coarse action
string ('run command' for every shell command), so approving one benign command
granted standing approval to arbitrary later commands including rm -rf and
git push --force, and the destructive deliberation backstop never ran on the
interactive auto-approve path.
- permgate-1a: session approval is now keyed by a normalized shell command
signature (base command + git/package subcommand, across all chained segments)
via permission.shell_command_signature(). Approving 'git status' no longer
whitelists 'git push' or 'rm'. The approve-for-session sibling drain matches the
same per-command key (reconstructed from the pending request's display), so it
cannot clear a queued unrelated/destructive command.
- permgate-1b: a destructive/irreversible call is never honored as session-approved
and never recorded as one ('approve for session' on it degrades to a one-time
approve), so a coarse approval can never silently carry an rm -rf / git push
--force. Uses the existing tool_destructive_reason classifier.
Tests: signature distinctness, and an integration test driving Approval.request()
that proves per-command keying and the destructive backstop end-to-end.
Phase 1 / injdef-3. The memory channel already BLOCKS on zero-width / bidi-override characters (the highest-confidence injection-smuggling signal), but the much higher-volume tool-output channel did not neutralize them. - Lift the invisible-char set into utils.trust as the shared INVISIBLE_CHARS, and strip those characters inside UntrustedData.render_for_prompt(). Because every wrapped channel (ReadFile, FetchURL, Shell, WebSearch) flows through that one choke point, all of them are now neutralized. - Strip, do not block: legitimate external content (security advisories, this repo's own fixtures) may contain visible injection-like prose, which the wrapper already marks as data — only the invisible vector is removed outright. The memory scanner keeps its block-on-persist behavior over the same shared set. Tests: invisible chars stripped while visible text and the wrapper are preserved; the memory blocker and the stripper share one source of truth.
Phase 1 / permgate-3 (gated on permgate-1). When several concurrent subagents issue a byte-identical action, the one-time approve path resolved only its own request, so the user was prompted once per sibling — pressure toward blanket approval. On a one-time 'approve', drain pending sibling requests with the SAME fine-grained identity (per-command approval key AND description), reusing the permgate-1 key machinery. Never drains a destructive call (each irreversible action is approved individually) and never writes to auto_approve_actions (one-time coverage of concurrent duplicates, not a standing rule), so it cannot over-approve a different or destructive command that merely shares the coarse action string. Test drives three concurrent requests and asserts approving one git status clears its identical sibling but leaves a different command (git diff) pending.
Phase 1 / permgate-2 + injdef-4. The agent could edit (and auto-approve edits to) its own behavioral config — AGENTS.md (re-injected into every future system prompt), agent-spec YAMLs, .pythinker config — so a one-time injection rewriting one becomes a persistent cross-session backdoor surviving the per-session untrusted-data defense. Edit side (permgate-2): is_config_surface_path() classifies these files; writes to them request approval under the new FileActions.EDIT_CONFIG action, which is non-session-approvable and not covered by the yolo/auto auto-approve bypass, so each config edit re-confirms every time (force-ask, not deny; in unattended auto the existing no-user denial applies). Plan/scratch/report artifacts are excluded. Ingestion side (injdef-4): strip invisible/bidi unicode from each merged AGENTS.md before it lands in the system prompt verbatim, reusing the shared strip_invisible_chars. Visible prose is kept — AGENTS.md is user-authored config, not blocked. Deferred (entangled with concurrent config.py theme work): adding the agent-controllable security keys to SCOPE_LOCKED_PATHS. Tests: config-surface classification, and a config edit re-prompting under yolo and never being recorded as session-approved.
The prompt-injection defense wraps shell/web/search output in <untrusted_data id="..."> tags for the model. The TUI render boundary (_ToolCallBlock._card_result_*) copied that model-facing output verbatim, so the wrapper tags leaked into rendered tool output (and ACP/IDE clients). Strip the envelope once at that single boundary via a new strip_untrusted_envelope() in trust.py (the inverse of render_for_prompt). The model still receives the wrapped form; only display surfaces get clean output.
These errors predate this change but were masked because `make check` short-circuits at the format step before pyright runs; fixing formatting unmasked 21 pyright errors in the Phase-1 security tests. ToolReturnValue.output is `str | list[ContentPart]`; the shell/web untrusted-wrapping tests treated it as `str`. Narrow with `isinstance` asserts and let the local `_unwrap` helpers accept `object`. Type the approval-driver `response` params as `ApprovalResponseKind`. Also formats one pre-existing unformatted line in test_untrusted_display.py.
Completes injdef-2: Shell stdout and WebSearch content were already wrapped, but Grep matched-content lines — external file bytes, a high-volume prompt-injection vector — reached the model unwrapped. Wrap content-mode output from both the ripgrep and Python-fallback paths via ToolResultBuilder.mark_untrusted(), mirroring Shell/ReadFile. Scope to content mode only; files_with_matches/count_matches surface relative, sensitive-filtered path/count metadata, not file bytes. SmartSearch aggregates nested Grep results and re-wraps once, so its internal Grep call uses a new `_wrap=False` flag to take raw output — avoiding nested/escaped <untrusted_data> tags. Display is unaffected: strip_untrusted_envelope already runs at the single render boundary. Tests: content output wrapped (ripgrep + fallback), no-match and files_with_matches not wrapped, SmartSearch wrapped without inner double-wrap; existing content-mode line-format tests unwrap first.
Adds the collision-aware execution plan for the remaining 22 agent-enhancement items, building on pythinker-agent-enhancement-plan.md: a diff-verified done-state ledger, a file->item collision matrix, workstream sequencing (so concurrent PRs never share a hot file), the cross-plan dependency with the God-Object decomposition (A3/A4/A7), branch strategy, and recorded decisions. Includes the per-item gap analysis it references (_gap_actionable.md, _gap_extract.md).
When a model gets stuck repeating failing tool calls, the agent loop kept stepping until the blunt max_steps_per_turn cap (default 1000) — wasting tokens and leaving the human to reconstruct state from an abrupt stop. Add a consecutive-failure backstop: count steps where every tool call errored (reset on any productive step) and, past LoopControl.max_consecutive_failures (default 8, 0 disables), end the turn with a new `stuck` stop reason and a handoff summary of what was tried — a deterministic safeguard independent of model self-correction. Emits an `agent_stuck` telemetry event. Reuses the existing StepOutcome plumbing; `stuck` is treated like `no_tool_calls` for the final message. Tests cover escalation past the threshold, counter reset on a productive step, and 0 disabling it.
When a tool call names a tool that doesn't exist, return a "Did you mean `X`?" hint using a difflib closest-match (cutoff 0.6) over the registered tool names, so the model can self-correct instead of failing blindly. ToolNotFoundError gains an optional `suggestion` arg (backward compatible).
Hitting the step ceiling printed only a static "max steps reached" line, leaving the human to reconstruct what was done and what's left. On MaxStepsReached, the shell and print surfaces now generate a brief, tools-disabled handoff summary (accomplished / remaining / next step) and print it after the static line. It reuses the side-question (btw) mechanism — bounded and tools-denied — so the summary turn cannot itself re-hit the ceiling or mutate the workspace, and it is not written to the main context. Best-effort: any failure falls back to the static line. Generalizes execute_side_question/_build_btw_context with a `system_reminder_text` param (default unchanged) and adds generate_max_steps_handoff. MaxStepsReached is still raised, so the wire/server and acp machine protocols keep their structured status codes.
…(ctxmgmt-2) Adds a cheap tier between "do nothing" and full LLM summarization. When usage crosses a lower threshold (prune_trigger_ratio, default 0.70), large completed tool-result bodies in deep history are replaced with a short placeholder, preserving message order, roles, and tool_call_id pairing. Full SimpleCompaction runs only if still over the higher compaction_trigger_ratio (0.85) afterward, so the lossy summary is deferred or avoided. - compaction.py: pure prune_stale_tool_outputs() + should_prune() (testable in isolation), placed alongside SimpleCompaction — its natural home. - pythinkersoul.py: prune_context() reuses compact_context's rewrite primitive (clear -> write_system_prompt -> checkpoint -> append) with the stubbed history, no LLM call, runs silently; trigger wired prune-before-compact in the loop. - config: prune_trigger_ratio / prune_protect_last / prune_min_chars (set prune_trigger_ratio >= compaction_trigger_ratio to disable the tier). Did not require the standalone A7 (ContextCompactor) extraction: the prune algorithm lives in compaction.py, satisfying extract-first's intent without an out-of-order god-object refactor (decomposition orders A7 last). Tests: prune function (prune/protect/skip/structure), should_prune threshold, and prune_context integration (history rewritten, structure preserved, no-op when nothing stale).
Apply reviewed findings on the agent approval/permission surface: - approval: match the safe-mode unattended-denial check against the compound per-command approval key (the same key approve-for-session stores) so a session-approved Shell command is no longer wrongly denied under auto+safe_mode. - approval: log when 'approve for session' is downgraded to one-time for destructive/config-surface calls, which can never be session-approved, instead of silently degrading. - permission: classify 'git push --delete'/'-d'/':refspec' as destructive so remote-ref deletion routes through deliberation alongside --force. - path: scope AGENTS.md config-surface detection to work_dir's ancestor chain (the set load_agents_md re-injects into the prompt) plus nested files, instead of matching any file named AGENTS.md; thread work_dir through the file-tool callers. Drop the dead '.pythinker/' relative-prefix branch. - background: wrap TaskOutput stdout/stderr in the untrusted-data envelope, closing the prompt-injection vector foreground Shell output already guards. - tools/file: extract a shared classify_edit_action so WriteFile and StrReplaceFile keep an identical outside/config/edit classification. Add regression tests for AGENTS.md injection-set scoping (work_dir in a subdir) and git push remote-ref deletion.
Add a Progress tool that posts a scannable progress note (title + optional one-line body) over the wire via ProgressNote, so the agent can surface checkpoints during long tasks. Register it in the default agent spec and teach extract_key_argument to use the note title as the tool's key argument. Update the agent-spec snapshot and the PyInstaller datas/hiddenimports manifest to include the new module and its bundled description.md.
Add ModelDefenseInjectionProvider, which emits a short, model-family-keyed reminder once per session through the existing dynamic-injection channel instead of bloating the cache-stable static system prompt for every model. Fragments match (and veto) on case-insensitive substrings of the model name; the initial registry reminds Qwen-family models not to drift into Chinese. The provider self-filters (no fragment unless the active model matches), so it is registered unconditionally, and re-arms after context compaction.
Two DONE-but-source-only features from the enhancement plan's §0 ledger now carry regression tests so they cannot silently drift out: - planning-1: assert every plan-mode reminder variant (full/sparse/reentry) mandates a Verification section, the review-first promise the human reviews. - obs-eval-2: drive record_llm_call through an InMemoryMetricReader and assert the cache_read / cache_creation counters receive prompt-cache token usage (and that the >0 guard keeps empty usage out of the series), so a cache-keying regression is detectable from telemetry rather than only a cost spike.
The harvest -> scratch -> journal -> recall pipeline shipped inert: all three durable flags default off, so the recall provider has nothing to rank. Rather than flip the privacy-affecting defaults, add a single opt-in profile. - MemoryConfig.durable_memory: when true, the harvest_enabled / journal_enabled effective-value properties report on (OR semantics) without rewriting the stored individual flags. Consolidation (durable MEMORY.md, approval-gated) stays separately opt-in. - Route the two gates (compaction harvest, session-exit recap) through the effective properties so the profile is honored. - Refresh the stale project_memory JOURNAL docstring (a writer now exists). Deferred (tracked): the dead lexical_recall flag (zero consumers) needs its own drop-vs-wire decision.
…d (skills-1) Loading a skill that references scripts/rotate_pdf.py or references/aws.md gave the model no runtime signal those files exist, forcing an improvised directory listing or a silently-skipped resource. Now the skill body is followed by a base-directory anchor + a sampled, sorted file manifest of the skill's bundled resources. - render_skill_resource_manifest(): subdirectory-form skills only (flat .md skills share the skills root, so enumerating it would leak siblings), gated to hosts that enumerate cheaply (local/ACP), bounded scan with honest truncation reporting, best-effort (never raises). - Centralized in read_skill_text_with_local_specialization so every injection path is consistent: the ReadSkill tool, the slash-command skill runner, and post-compaction skill restoration all surface the manifest identically, after any local specialization. - Fix skill-creator SKILL.md step list referencing non-existent init_skill.py / package_skill.py scripts (the step bodies already describe the manual flow).
…subagent-2) An N-child fan-out (or explore->plan->implement->review chain) could spend 10-15x a single turn with no in-context signal — the orchestrator only learned the cost from the provider bill. Now each subagent reports its cumulative LLM spend. - PythinkerSoul tracks cumulative token usage across its run: every step's LLM call plus compaction's own call (which runs outside the step loop) are folded into soul.cumulative_usage. - ForegroundSubagentRunner emits child_tokens (+ child_cost_usd when priced) in the result envelope and structured extras — on success AND on failure, so a partial-failure fan-out does not under-count spend. - RunAgents sums children's extras into a total_child_tokens batch line. - Cost reuses the existing pricing table; degrades to omitted when unpriced. Scoped to the foreground/in-context path (the orchestrator signal). Background TaskRuntime token plumbing and a StatusSnapshot/footer number are deferred.
…s (mode-1, skills-2) Two documentation-only authoring skills, schema-accurate so they work offline and do not make the model guess Pythinker's own config surface. - agent-creator (mode-1): guided authoring of a project subagent — markdown agent-file form (auto-discovered) vs YAML form (extend inheritance, subagents), discovery dirs + precedence, friendly tool names, persona + structured output contract modeled on the builtin plan/explore agents, round-trip validation. - customize-pythinker (skills-2): the config surfaces no other skill owns — agent YAML schema, the six permission profiles and their flags, plugin.json, and the 13 hook lifecycle events. Defers agent/skill authoring to the dedicated skills. Every schema claim verified against source (a fact-check pass corrected the project-agent precedence: a name matching a builtin is skipped, not overriding). PyInstaller datas snapshot updated for the two new bundled skill files.
…tooldesc-2/ctxmgmt-1) Foreground command and fetched-page output is non-idempotent and unrecoverable once truncated — re-running a build/test is expensive or non-deterministic, and the model was told nothing about how to recover the lost tail. Now, on overflow, the full output is spilled to a session-scoped file and the inline truncation marker is replaced with an actionable hint (ReadFile/Grep the file, or delegate to a read-only explore subagent). - ToolResultBuilder.enable_spill(): opt-in full-output capture; on truncation, writes <session.dir>/tool-output/<tool>-<uuid>.txt and emits the hint. Memory-bounded (SPILL_MAX_CHARS), fail-soft (never breaks the result), idempotent (one file even if ok()+error()), and the tool stem is sanitized so it cannot escape the spill dir. - Wired into foreground Shell and web fetch/search (the unrecoverable channels). Background tasks already spill; Grep/ReadFile already re-read their on-disk source, so they are intentionally untouched. Deferred: a per-session tool-output retention sweep (dirs are reclaimed wholesale on session archival today).
…esses subagent-2 folds compaction-call usage into soul.cumulative_usage in compact_context(). Two test harnesses build a PythinkerSoul via object.__new__ (bypassing __init__) and mock the compaction result; give that mock usage=None so the accumulation is correctly skipped instead of tripping on the missing field.
…-eval-1) Tool/LLM/turn spans appeared as flat sibling roots because start_span created spans without attaching them to the OTel context. Now start_span installs the span as current so children nest into a connected turn -> llm -> tool tree, and each level carries gen_ai.operation.name (invoke_agent / chat / execute_tool) plus gen_ai.tool.name so GenAI-aware backends recognize the hierarchy. Context handling is the careful part (reviewed): - Attach only when telemetry is initialized; the no-op/disabled case skips attach/detach so a Ctrl-C that finalizes the CM from another asyncio context cannot emit OTel's 'Failed to detach context' log. For the enabled case that log (detach swallows the mismatch internally rather than raising) is demoted to CRITICAL alongside the other OTel loggers. - The tool span's manual __enter__/__exit__ now closes on BaseException (CancelledError/KeyboardInterrupt), so its context token detaches in-task rather than leaking until GC. Tests: span nesting, sibling isolation, attribute plumbing, and exception / cancellation both still detach. Removed the now-unreachable detach-mismatch guard and its tests.
…-4, offline core) Behavioral evals were pass/fail-only: a prompt/tool-description change could double the tool calls, blow up tokens, or pick the wrong subagent while still passing the smoke reward, and nothing would flag it. This adds the offline-testable core: - EvalCase: a versioned scenario (query + expected tool trajectory + reference outcome + per-scenario efficiency budgets). - score_eval_case: passes only when within every set budget AND every expected tool was used; reports per-metric breaches + missing tools. - observed_from_metric_reader: reads the efficiency triple (tool calls, tokens, tool errors, step count) back out of an in-process OTel InMemoryMetricReader — the zero-extra-plumbing tap, since the agent loop already emits these. Deferred (live-run slice, documented in the module): wiring this per-scenario into the scripted-echo e2e suite and extending the accuracy_smoke/Harbor result parser — both need a real run + curated corpus; this schema+scorer is their foundation.
… offline core) Pythinker could only test against hand-scripted model behavior — it could not capture a real run and replay it deterministically, nor commit such fixtures safely. This adds the offline-testable core: - a versioned JSON cassette format of request/response interactions; - a redaction pipeline that strips auth headers and secret-like values (sk-/Bearer /AKIA/gh*_/Slack tokens) BEFORE anything is written, so a captured cassette is safe to commit; - a deterministic CassettePlayer that replays responses in order and fails loudly (CassetteMismatch) on exhaustion or a request method mismatch — so drift in what Pythinker SENDS surfaces as a failure, not silent reuse. Deferred (live-run slice, documented in the module): the recorder that captures a live provider behind PYTHINKER_RECORD, and binding the player into the chat_provider boundary (provider classes live in pythinker_core). This format+redaction+replay is their foundation.
The wire e2e snapshots were red: external tool output (shell/web) is wrapped in <untrusted_data id=NONCE> with a per-call random nonce (utils/trust.py, from the untrusted-data defense), but the e2e normalization framework never stabilized it — so any snapshot of wrapped output changed every run. Add nonce normalization to wire_helpers.normalize_value (alongside the existing version/path/uuid handling), making those snapshots deterministic, and refresh the handshake/session snapshots for the additive tools (Recall, Suggest, ListMcpResources, ReadMcpResource) and skills (agent-creator, customize-pythinker) added this branch. Verified stable across three clean runs (65 passed). No behavioral snapshot changes — only nonce→<NONCE> normalization and additive tool/skill-list growth.
Code-review fixes across the agent core (research-backed, TDD): - approval: bind session-approvability on the request record at create time so an approve/approve-for-session drain can never resolve a destructive or config-surface sibling that merely shares the coarse command signature (permgate-1b/3). - approval: fail closed in unattended runs for anything that won't be auto-resolved downstream, fixing two indefinite-wait hangs (destructive call sharing a session-approved key; config-surface edit). - path: treat `.md` agent specs under the agent-spec dirs as a config surface, like the YAML specs (closes a prompt-injection backdoor). - acp: strip the model-facing <untrusted_data> envelope from tool output before it reaches an ACP/IDE client (ACP has no untrusted marking). - subagents: surface a background child's LLM spend in its transcript. - toolset: separate a genuinely-absent MCP capability (METHOD_NOT_FOUND) from a transient failure (visible warning), and dedupe discovery. - tools/utils: offload the on-truncation spill off the event loop (asyncio.to_thread) with an atomic temp+rename write. - recall: arm only after a successful snapshot; defer the working-set scan behind the cheap turn-throttle gate. - pythinkersoul: anchor the post-prune token count to the authoritative count minus the freed delta; extract `_opt_int` for usage parsing.
- chat_provider: carry the parsed response body on APIStatusError so the UI can surface structured 429 detail instead of a stringified exception. - ui/shell: _extract_429_detail returns summary + reset window + server detail (recovered from the stringified body when needed); render them as plain English with the reset window and a dim Server: trail. Escape all provider-supplied fields so bracketed text is not dropped by Rich markup. - auth/openai: force a fresh ChatGPT login screen (prompt=login) so /login can switch accounts, and report whether the account actually changed. - tests: harden the localhost-callback tests against CPU-load timing flakes (generous bounded deadlines + poll-until-done instead of fixed sleeps).
- AGENTS.md: expand contributor/agent guidance. - __main__: opt-in PYTHINKER_TRACE_ASYNCIO diagnostic that mirrors "coroutine was never awaited" warnings (with allocation tracebacks) to a log file, started before any event loop runs. Off by default. - docs: vitepress config + customization/architecture updates.
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:
📝 WalkthroughWalkthroughAdds repository maps/docs, trust-envelope sanitization, new tools (Progress/Suggest/Recall/MCP), approval/permission hardening, soul pruning and stuck backstop, spill-to-disk for large tool outputs, telemetry counters, CLI/MCP helpers, and broad test coverage. ChangesRepository guidance and maps
Trust envelope and display stripping
Approval, permission, and config-surface rules
Tool modules, output contracts, and spill-to-disk
Soul runtime, pruning, recall, telemetry, and CLI
Tests and test utilities
Estimated code review effort: Possibly related PRs:
Suggested labels: ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The promoted AI-risk tripwires (C01-C15) and failure-truthfulness contract were appended at the bottom of AGENTS.md, but the 32 KiB leaf-first merge in load_agents_md truncates the tail first. In web/ and vis/ sessions a nested AGENTS.md also loads, so the always-on safety baseline was the first casualty. Relocate the section to directly after "Non-negotiable rules" so it survives, leaving "Release pipeline gotchas" as the truncation buffer.
Extend rate-limit/usage-limit messaging: _render_429_message now leads with live reset windows fetched from the provider usage endpoint (the streaming 429 carries none) via _format_usage_window_row, and _capture_unparsed_429 logs unrecognised 429 payload shapes to rate-limit-debug.log for precise diagnosis. Add covering tests, refresh task notes, and update the synced changelog.
Document the Unreleased 429/usage-limit messaging + ChatGPT account switch and the agent phase-0 enhancements (Recall tool, read-only MCP resources/prompts, project-scoped .pythinker/mcp.json, subagent token/cost roll-up, truncated-output spill). docs/en/release-notes/changelog.md regenerated via npm run sync.
![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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/tools/test_shell_bash.py (1)
340-346:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an explicit success assertion for the spill-path test.
Line [344] and Line [345] only assert message substrings. If the command path regresses to an error with
similar text, this test can still pass. Addassert not result.is_errorto lock this to the intended
successful spill behavior.🤖 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_shell_bash.py` around lines 340 - 346, The test test_large_output_spills_with_recovery_hint currently only checks substrings in result.message; add an explicit success assertion to ensure the tool returned a successful result by asserting not result.is_error on the result produced by calling shell_tool(Params(...)). Insert assert not result.is_error right after the result is awaited (before the existing message substring asserts) to lock the test to the intended non-error spill behavior.tests/tools/test_skill_tool.py (1)
158-181:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused
runtimeparameter.The
test_skill_body_manifest_follows_local_specializationtest creates its ownskillsdict and doesn't use theruntimefixture. Ruff correctly flagged this.🧹 Proposed fix
async def test_skill_body_manifest_follows_local_specialization(runtime, tmp_path: Path) -> None: +async def test_skill_body_manifest_follows_local_specialization(tmp_path: Path) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tools/test_skill_tool.py` around lines 158 - 181, The test function test_skill_body_manifest_follows_local_specialization has an unused runtime parameter; remove the runtime argument from its async def signature so it becomes async def test_skill_body_manifest_follows_local_specialization(tmp_path: Path) -> None, ensuring no other references to runtime exist in the test body (the test already constructs its own skills dict and calls read_skill_text_with_local_specialization). Leave the rest of the assertions and helper calls (e.g., read_skill_text_with_local_specialization, _skill, skills) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pythinker-core/tests/test_openai_common.py`:
- Around line 225-253: The file fails ruff formatting; run the formatter and
commit the changes: run `ruff format` (or your editor integration) on
packages/pythinker-core/tests/test_openai_common.py to reformat the test class
TestConvertErrorStatusErrorBody and its test method
test_status_error_preserves_body_and_request_id so the file passes CI style
checks, then stage and push the updated file.
In `@src/pythinker_code/__main__.py`:
- Around line 102-109: The except block around writing asyncio warnings to the
diagnostic log is too broad; replace the bare "except Exception" with "except
OSError" so only file I/O errors are caught while still allowing other
exceptions to propagate, and keep the surrounding logic that creates the
directory (log_path.parent.mkdir...), opens the file (log_path.open("a", ...)),
writes warnings.formatwarning(...), and then returns via
_orig_showwarning(message, category, filename, lineno, file, line).
In `@src/pythinker_code/tools/display.py`:
- Line 21: The pinned-todo rendering logic in
src/pythinker_code/ui/shell/visualize/_live_view.py currently suppresses
successful todo tool cards and wasn’t updated to treat the new "cancelled"
status as a visible state; update the filtering in the pinned-todo rendering
path (the function that filters/suppresses todo tool cards) to include
"cancelled" among the statuses considered for display and ensure any suppression
rule that hides "done" does not also hide "cancelled" so cancelled todos remain
represented in the UI (adjust the status checks/sets used when building pinned
todo cards accordingly).
In `@src/pythinker_code/tools/mcp_resource/__init__.py`:
- Around line 31-33: The constructors in this module are missing explicit return
annotations; update both __init__ methods (the one taking toolset:
PythinkerToolset and the other constructor around lines 79-81) to include a
return type of -> None (e.g., change def __init__(...) to def __init__(...) ->
None:) so they comply with the repo's public-method annotation rule.
In `@src/pythinker_code/wire/server.py`:
- Around line 783-786: The loop currently calls request.resolve({}) for
superseded QuestionRequest objects but leaves them in self._pending_requests,
allowing late client responses to hit _handle_response and double-resolve;
update the logic to remove (pop) each dismissed QuestionRequest from
self._pending_requests when you call resolve so they are fully retired. Locate
the loop over self._pending_requests, check isinstance(request, QuestionRequest)
and not request.resolved, call request.resolve({}) and then remove that entry
from self._pending_requests (use the dict key or iterate over a list of keys to
pop safely) so _handle_response will no longer see dismissed requests.
In `@tasks/agent-enhancement-remaining-plan.md`:
- Line 339: Replace the incorrect word "invokable" with "invocable" in the
markdown heading/text that reads "memory-1 (recall TOOL) — new `tools/recall/`:
model-invokable...", i.e., update the phrase to "model-invocable" so the
spell-check CI accepts it; locate the string in the file
tasks/agent-enhancement-remaining-plan.md (the section containing "memory-1
(recall TOOL)" / "tools/recall/") and make that single-word change.
In `@tasks/pythinker-agent-enhancement-plan.md`:
- Line 49: Fix the spelling flagged by CI by replacing every occurrence of the
word "invokable" with "invocable" in the document; search for the token
"invokable" (appears in the Memory & recall agency line and the other two
instances) and update them to "invocable" so the spell-check passes and meaning
is preserved.
In `@tests/core/test_context_pruning.py`:
- Around line 198-200: Remove the duplicated pytest marker above the async test
function: delete one of the two `@pytest.mark.asyncio` decorators that appear
immediately above the async def
test_prune_context_never_increases_token_count(runtime, tmp_path) to leave a
single `@pytest.mark.asyncio` decorator; this eliminates the redundant decorator
and prevents potential pytest errors while keeping the test marked as async.
- Around line 128-135: _remove the unused parameter from the function signature
of _seed_prunable by changing def _seed_prunable(context) -> list[Message]: to
def _seed_prunable() -> list[Message]: because the parameter is never used; then
update all call sites that currently pass a context argument (the two
invocations in this test file that call _seed_prunable(...)) to call
_seed_prunable() with no arguments so the signatures match and tests compile.
In `@tests/core/test_mcp_docker_rm.py`:
- Around line 21-23: The test_keeps_existing_rm function defines an unused
parameter cmd; remove that unused parameter from the test signature so the test
becomes def test_keeps_existing_rm() -> None: and keep the call
ensure_docker_rm("docker", original) as-is (or, alternatively, if you prefer a
variable, use cmd in the call and keep the parameter). Update the function
definition only (remove the defaulted cmd parameter) to eliminate the unused
parameter warning and keep behavior unchanged; references:
test_keeps_existing_rm and ensure_docker_rm.
In `@tests/core/test_project_mcp_config.py`:
- Line 37: The chained assertion "assert found is not None and
found.samefile(cfg)" should be split into two separate checks to avoid
short-circuit masking: first assert that found is not None, then assert that
found.samefile(cfg) — update the test in tests/core/test_project_mcp_config.py
to replace the single chained assertion with two assertions referencing the
existing variables found and cfg (e.g., assert found is not None; assert
found.samefile(cfg)).
- Line 22: The chained assertion "assert found is not None and
found.samefile(cfg)" should be split into two explicit checks to satisfy Ruff
PT018: first assert that 'found' is not None, then assert that
'found.samefile(cfg)' is True. Locate the assertion that uses the variables
'found' and 'cfg' in the test and replace it with two separate assert
statements—e.g., "assert found is not None" followed by "assert
found.samefile(cfg)"—so failures show which condition failed.
---
Outside diff comments:
In `@tests/tools/test_shell_bash.py`:
- Around line 340-346: The test test_large_output_spills_with_recovery_hint
currently only checks substrings in result.message; add an explicit success
assertion to ensure the tool returned a successful result by asserting not
result.is_error on the result produced by calling shell_tool(Params(...)).
Insert assert not result.is_error right after the result is awaited (before the
existing message substring asserts) to lock the test to the intended non-error
spill behavior.
In `@tests/tools/test_skill_tool.py`:
- Around line 158-181: The test function
test_skill_body_manifest_follows_local_specialization has an unused runtime
parameter; remove the runtime argument from its async def signature so it
becomes async def
test_skill_body_manifest_follows_local_specialization(tmp_path: Path) -> None,
ensuring no other references to runtime exist in the test body (the test already
constructs its own skills dict and calls
read_skill_text_with_local_specialization). Leave the rest of the assertions and
helper calls (e.g., read_skill_text_with_local_specialization, _skill, skills)
unchanged.
🪄 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: b465bdde-e8eb-4e93-82cf-cc67dd100028
📒 Files selected for processing (145)
AGENTS.mddocs/.vitepress/config.tsdocs/en/customization/architecture.mdpackages/pythinker-core/src/pythinker_core/chat_provider/__init__.pypackages/pythinker-core/src/pythinker_core/chat_provider/openai_common.pypackages/pythinker-core/src/pythinker_core/tooling/error.pypackages/pythinker-core/tests/test_openai_common.pysrc/pythinker_code/__main__.pysrc/pythinker_code/acp/convert.pysrc/pythinker_code/acp/session.pysrc/pythinker_code/agents/default/agent.yamlsrc/pythinker_code/agents/default/system.mdsrc/pythinker_code/approval_runtime/models.pysrc/pythinker_code/approval_runtime/runtime.pysrc/pythinker_code/auth/openai.pysrc/pythinker_code/background/agent_runner.pysrc/pythinker_code/cli/__init__.pysrc/pythinker_code/cli/mcp.pysrc/pythinker_code/config.pysrc/pythinker_code/memory/recall.pysrc/pythinker_code/project_memory.pysrc/pythinker_code/session_state.pysrc/pythinker_code/skill/__init__.pysrc/pythinker_code/skills/agent-creator/SKILL.mdsrc/pythinker_code/skills/customize-pythinker/SKILL.mdsrc/pythinker_code/skills/skill-creator/SKILL.mdsrc/pythinker_code/soul/agent.pysrc/pythinker_code/soul/approval.pysrc/pythinker_code/soul/btw.pysrc/pythinker_code/soul/compaction.pysrc/pythinker_code/soul/dynamic_injections/model_defense.pysrc/pythinker_code/soul/dynamic_injections/plan_mode.pysrc/pythinker_code/soul/permission.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/soul/toolset.pysrc/pythinker_code/subagents/output.pysrc/pythinker_code/subagents/runner.pysrc/pythinker_code/subagents/usage.pysrc/pythinker_code/telemetry/metrics.pysrc/pythinker_code/telemetry/otel.pysrc/pythinker_code/tools/__init__.pysrc/pythinker_code/tools/agent/__init__.pysrc/pythinker_code/tools/agent/description.mdsrc/pythinker_code/tools/background/__init__.pysrc/pythinker_code/tools/display.pysrc/pythinker_code/tools/file/__init__.pysrc/pythinker_code/tools/file/grep.mdsrc/pythinker_code/tools/file/grep_local.pysrc/pythinker_code/tools/file/replace.mdsrc/pythinker_code/tools/file/replace.pysrc/pythinker_code/tools/file/write.mdsrc/pythinker_code/tools/file/write.pysrc/pythinker_code/tools/mcp_resource/__init__.pysrc/pythinker_code/tools/mcp_resource/list_description.mdsrc/pythinker_code/tools/mcp_resource/read_description.mdsrc/pythinker_code/tools/plan/enter.pysrc/pythinker_code/tools/progress/__init__.pysrc/pythinker_code/tools/progress/description.mdsrc/pythinker_code/tools/recall/__init__.pysrc/pythinker_code/tools/recall/description.mdsrc/pythinker_code/tools/shell/__init__.pysrc/pythinker_code/tools/skill/__init__.pysrc/pythinker_code/tools/skill/description.mdsrc/pythinker_code/tools/suggest/__init__.pysrc/pythinker_code/tools/suggest/description.mdsrc/pythinker_code/tools/think/think.mdsrc/pythinker_code/tools/todo/__init__.pysrc/pythinker_code/tools/todo/set_todo_list.mdsrc/pythinker_code/tools/utils.pysrc/pythinker_code/tools/web/fetch.mdsrc/pythinker_code/tools/web/fetch.pysrc/pythinker_code/tools/web/search.mdsrc/pythinker_code/tools/web/search.pysrc/pythinker_code/ui/print/__init__.pysrc/pythinker_code/ui/print/visualize.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/tool_renderers/todo.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/utils/path.pysrc/pythinker_code/utils/trust.pysrc/pythinker_code/wire/server.pysrc/pythinker_code/wire/types.pytasks/_gap_actionable.mdtasks/_gap_extract.mdtasks/agent-enhancement-remaining-plan.mdtasks/pythinker-agent-enhancement-plan.mdtasks/todo.mdtests/acp/test_session_question.pytests/auth/test_openai_auth.pytests/core/test_agent_spec.pytests/core/test_approval_auto.pytests/core/test_builtin_authoring_skills.pytests/core/test_config.pytests/core/test_context_pruning.pytests/core/test_cumulative_usage.pytests/core/test_default_agent.pytests/core/test_dynamic_injection_hooks.pytests/core/test_llm_cache_metrics.pytests/core/test_max_steps_handoff.pytests/core/test_mcp_cleanup.pytests/core/test_mcp_docker_rm.pytests/core/test_memory_durable_profile.pytests/core/test_model_defense.pytests/core/test_otel_span_tree.pytests/core/test_permission_profiles.pytests/core/test_plan_mode_injection_provider.pytests/core/test_project_mcp_config.pytests/core/test_pythinkersoul_stuck_loop.pytests/core/test_recall_rearm.pytests/core/test_wire_server_steer.pytests/subagents/test_usage_rollup.pytests/telemetry/test_instrumentation.pytests/telemetry/test_otel_resource.pytests/tools/test_agent_tool.pytests/tools/test_grep.pytests/tools/test_mcp_resource.pytests/tools/test_progress.pytests/tools/test_recall.pytests/tools/test_shell_bash.pytests/tools/test_shell_powershell.pytests/tools/test_skill_tool.pytests/tools/test_smart_search.pytests/tools/test_suggest.pytests/tools/test_todo.pytests/tools/test_tool_descriptions.pytests/tools/test_tool_schemas.pytests/tools/test_untrusted_wrapping.pytests/tools/test_web_allowlist_tools.pytests/ui_and_conv/test_rate_limit_message.pytests/ui_and_conv/test_untrusted_display.pytests/utils/test_pyinstaller_utils.pytests/utils/test_result_builder.pytests_e2e/cassette.pytests_e2e/eval_schema.pytests_e2e/test_cassette.pytests_e2e/test_eval_schema.pytests_e2e/test_wire_approvals_tools.pytests_e2e/test_wire_prompt.pytests_e2e/test_wire_protocol.pytests_e2e/test_wire_sessions.pytests_e2e/wire_helpers.pyvis/AGENTS.mdweb/AGENTS.md
- 'unparseable' -> 'unparsable' in the unparsed-429 diagnostic comment so the Typo checker (crate-ci/typos) passes - ruff-format the new JSON-recovery rate-limit test
elkaix
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pythinker_code/soul/pythinkersoul.py (1)
1742-1788:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset the stuck-loop counter on tool-free continuation paths.
_consecutive_failuresonly resets inside theresult.tool_callsbranch. If a no-tool-call step hits the unfinished-intent nudge at Lines 1770-1788, the next all-error tool batch is still counted as "consecutive", so the"stuck"handoff can fire even though the previous step did not have every tool call fail.Suggested fix
if result.tool_calls: # Degenerate-loop backstop: count consecutive steps where every tool # call failed; past the configured threshold, hand control back to the # user with a summary instead of burning steps until max_steps_per_turn. threshold = self._loop_control.max_consecutive_failures if _is_all_error_batch(results): self._consecutive_failures += 1 if threshold and self._consecutive_failures >= threshold: from pythinker_code.telemetry import track summary = _stuck_summary_message( self._consecutive_failures, result.tool_calls, results ) await self._context.append_message(summary) track( "agent_stuck", consecutive_failures=self._consecutive_failures, model=self._runtime.llm.model_name, ) return StepOutcome(stop_reason="stuck", assistant_message=summary) else: self._consecutive_failures = 0 return None + self._consecutive_failures = 0 + # A tool-call-free message normally ends the turn. If it is only a # restatement of intent ("Let me synthesize the findings…") with no🤖 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/pythinkersoul.py` around lines 1742 - 1788, The code only resets self._consecutive_failures inside the result.tool_calls branch, so when a step has no tool calls (the branch after "A tool-call-free message...") we must also clear the counter; update the no-tool-call path in the method containing result.tool_calls, setting self._consecutive_failures = 0 before the unfinished-intent check (i.e. before consulting _intent_nudge_used and calling _looks_like_unfinished_intent on result.message.extract_text) and ensure any early return from that path leaves the counter reset so subsequent _is_all_error_batch checks don't mistakenly treat it as consecutive failures.
🤖 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/__init__.py`:
- Around line 443-459: The helper _capture_unparsed_429 currently swallows all
errors in its broad except block so failures to write the fallback
"rate-limit-debug.log" (e.g., missing/unwritable share dir from get_share_dir())
are hidden; modify the except to surface the failure instead of silently passing
— catch Exception as e and log the failure with context (include path and
exception) via the module logger or a fallback stderr write, or re-raise a
wrapped/logged exception as appropriate; ensure the change references
_capture_unparsed_429, get_share_dir, and the "rate-limit-debug.log" path so the
diagnostic write failures are visible.
- Around line 505-507: The helper _capture_unparsed_429 currently swallows all
exceptions with a bare except/pass which hides failures when writing the
fallback diagnostic; update _capture_unparsed_429 to catch Exception but log the
failure (e.g., call logger.debug or similar with exc_info=True and a clear
message) before returning so the function retains its "never raises" contract;
locate the callsite and guarding logic in _codex_usage_windows (the
provider.type == "openai_codex" branch) to ensure the logging path is reachable
and keep existing control flow otherwise unchanged.
In `@tasks/_gap_actionable.md`:
- Around line 90-91: Add a single blank line immediately before the heading "##
[ctxmgmt-3] Recall is one-shot injection only; no model-invocable cross-session
recall tool" so the heading is separated from the preceding paragraph,
satisfying markdown lint rules; locate the heading text in
tasks/_gap_actionable.md and insert one empty line above it.
In `@tasks/_gap_extract.md`:
- Line 198: Tighten the emphasis markers around the "verify-evidence" token so
the markdown emphasis rule MD037 is satisfied: replace the current spaced
emphasis syntax (e.g. "- **verify-evidence:** ...") with correctly-formed
markers with no extra inner spacing (e.g. "- **verify-evidence:** ..."),
ensuring the asterisks directly surround the text; update the emphasis instance
shown in the diff (the "**verify-evidence:**" occurrence) so the bold/italic
markers are contiguous with the token and re-run markdown linting.
- Around line 188-189: Add a single blank line immediately after the markdown
heading "### [ctxmgmt-3] Recall is one-shot injection only; no model-invocable
cross-session recall tool" so the heading is separated from the following bullet
list; this satisfies markdownlint rule MD022 by ensuring there is an empty line
between the heading and the list.
In `@tasks/pythinker-agent-enhancement-plan.md`:
- Line 273: The markdown heading "#### 3.1 — Model-invocable cross-session
`Recall` tool (`memory-1` / `ctxmgmt-3`) · M · med" is missing the required
blank line per MD022; insert a single blank line immediately after that heading
so there is an empty line between the heading and the following content to
satisfy markdownlint.
---
Outside diff comments:
In `@src/pythinker_code/soul/pythinkersoul.py`:
- Around line 1742-1788: The code only resets self._consecutive_failures inside
the result.tool_calls branch, so when a step has no tool calls (the branch after
"A tool-call-free message...") we must also clear the counter; update the
no-tool-call path in the method containing result.tool_calls, setting
self._consecutive_failures = 0 before the unfinished-intent check (i.e. before
consulting _intent_nudge_used and calling _looks_like_unfinished_intent on
result.message.extract_text) and ensure any early return from that path leaves
the counter reset so subsequent _is_all_error_batch checks don't mistakenly
treat it as consecutive 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: eaefb75c-338f-4510-9a53-f0e73f4c378c
📒 Files selected for processing (17)
AGENTS.mdCHANGELOG.mddocs/en/release-notes/changelog.mdpackages/pythinker-core/tests/test_openai_common.pypyproject.tomlsrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/usage_adapters/openai_chatgpt.pytasks/_gap_actionable.mdtasks/_gap_extract.mdtasks/agent-enhancement-remaining-plan.mdtasks/pythinker-agent-enhancement-plan.mdtests/core/test_approval_auto.pytests/core/test_pythinkersoul_think_only.pytests/core/test_recall_rearm.pytests/ui/usage_adapters/test_openai_chatgpt.pytests/ui_and_conv/test_rate_limit_message.py
- live_view: render cancelled todos (✕, muted+struck) instead of dropping them from the pinned list, now that "cancelled" is a valid todo status - wire/server: pop dismissed QuestionRequests from the pending map on steer so a late client response can't double-resolve a superseded question - ui/shell: log (don't silently swallow) failures to write the fallback 429 diagnostic, so that diagnostic path stays debuggable - mcp_resource: add -> None to the public constructors per the annotation guideline - tests: drop unused params, remove a duplicate @pytest.mark.asyncio, split chained assertions; add coverage for the three behavioral fixes above - tasks/*.md: fix markdownlint blank-line nits Skipped as stale or out of policy: ruff-format/typos findings already fixed in earlier commits; an MD037 false positive on snake_case prose; narrowing the best-effort asyncio-warning catch (would reintroduce a crash path).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/ui_and_conv/test_rate_limit_message.py`:
- Around line 155-174: The test
test_capture_unparsed_429_logs_write_failure_instead_of_swallowing should stop
monkeypatching pythinker_code.ui.shell.logger.debug and instead assert
observable logging via a capture sink (e.g., pytest's caplog fixture): call
_capture_unparsed_429(RuntimeError(...)) ensuring it does not raise, then use
caplog to check a debug/error record was emitted referencing the diagnostic
write failure and that the record contains exc_info=True; specifically remove
the monkeypatch on logger.debug and replace with caplog.set_level(...) and
assertions against caplog.records/messages while still simulating get_share_dir
failure via monkeypatch of pythinker_code.share.get_share_dir returning an
OSError.
🪄 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: 470d8464-3efd-421a-8dc9-3d189a11cb49
📒 Files selected for processing (13)
src/pythinker_code/tools/mcp_resource/__init__.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/visualize/_live_view.pysrc/pythinker_code/wire/server.pytasks/_gap_actionable.mdtasks/_gap_extract.mdtasks/pythinker-agent-enhancement-plan.mdtests/core/test_context_pruning.pytests/core/test_mcp_docker_rm.pytests/core/test_project_mcp_config.pytests/core/test_wire_server_steer.pytests/ui_and_conv/test_live_view_todos.pytests/ui_and_conv/test_rate_limit_message.py
Two fixes surfaced while tracing asyncio coroutine leaks: - Drop Sentry's AsyncioIntegration. Its create_task monkeypatch wrapped every coroutine in `_task_with_sentry_span_creation` (`result = await coro`); when a task was cancelled before its first step during turn/prompt teardown — e.g. a re-armed `WireUISide.receive()` or a prompt_toolkit background task — the wrapper raised before reaching `await coro`, orphaning the inner coroutine and printing "coroutine ... was never awaited" RuntimeWarnings to the console. With traces/profiles off it added no spans, and async exception capture is preserved by the existing handler. Removes the three prompt_toolkit warning-filter band-aids that masked the same noise, and adds a regression test asserting the integration stays unregistered. `__main__` now sets sys.set_coroutine_origin_tracking_depth under PYTHINKER_TRACE_ASYNCIO so future leak origins are traceable. - Canonicalize version-pinned interpreter names in the shell permission guard: `python3.14 -c` / `/usr/bin/python3.12 -c` / `node20 -e` now hit the same mutating/destructive classification as the bare python/node forms, closing a read-only/plan subagent bypass (sys.executable is commonly python3.14).
The default agent's MCP guidance only covered *using* already-connected servers and told it to never touch MCP config. Asked to add or set up a new server, the model fell back on the MCP hosts in its training data (Claude Code / Claude Desktop), cited ~/.claude.json, and refused — claiming it had "no tool to edit" the config, despite having file I/O. Extend the system prompt's MCP section so the agent knows it runs in Pythinker: config lives at ~/.pythinker/mcp.json (global) and ./.pythinker/mcp.json (project), and it can add a server via `pythinker mcp add` or by writing that JSON. Keep the honest caveat that a newly added server only connects on the next Pythinker start, and forbid citing non-Pythinker (Claude) config paths. Add a regression test asserting the prompt names the real config files and CLI, keeps the restart caveat, and steers off the Claude-host paths.
The default agent's system prompt now documents the full MCP lifecycle — add (stdio/http), remove, list, and test — and hard-steers off writing mcpServers into config.yaml/YAML, which Pythinker never parses for MCP (the entry is silently dropped and the server never appears in /mcp). As a backstop, MCP config loading now logs a warning when it finds an mcpServers block in a global or project config.yaml, so a misplaced entry is diagnosable in pythinker.log instead of failing silently. Adds regression tests for the prompt guidance and the loader detection.
…ancements # Conflicts: # CHANGELOG.md
5 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
Completes the agent phase-0 enhancement plan (22 items) and folds in three follow-up commits: a code-review hardening pass, a ChatGPT 429/login UX feature, and docs/diagnostic updates. 56 commits over
main.Major areas (phase-0 plan)
.pythinker/mcp.jsonlayering, docker--rmteardown hygiene.agent-creator+customize-pythinkerbuiltin skills, bundled-resource manifest on load.Follow-up commits in this push
fix:review hardening — closes an approve-for-session drain that could auto-approve a queued destructive sibling; classifies.mdagent specs as a config surface; fixes two unattended fail-closed hangs; strips the<untrusted_data>envelope on the ACP path; surfaces background child spend; separates absent-vs-transient MCP discovery errors; offloads the on-truncation spill off the event loop (atomic write); arms recall only on success; anchors the post-prune token count.feat:429 / login UX — human-friendly usage-limit messaging (plan + reset window + dimServer:trail, all markup-escaped) andprompt=loginChatGPT account switching.chore:docs — AGENTS guidance, opt-inPYTHINKER_TRACE_ASYNCIOdiagnostic, vitepress/architecture docs.Testing
Changed areas green locally (1914 passed across the touched suites); ruff + pyright clean. CI runs the full unit + e2e matrix.
Summary by CodeRabbit
New Features
Improvements
Documentation