fix: deep-scan correctness fixes (Gemini truncation, wire handoff, theme parity, agent preview, MCP type filter) - #143
Merged
Merged
Conversation
25-agent gap-analysis scout of the blackbox agent-harness reference vs current pythinker. Of ~75 candidate patterns only 13 are actionable (1 adopt-now + 12 adapt); ~44 are already-have (pythinker is frequently more robust), the rest stub-only/rewrite-defer/anti-pattern. Items are framed generically (technique, not the reference's proprietary text) and fit the existing soul/ loop (no swap). Planned in 4 waves: low-risk hardening, permission safety, loop/hook quality, heavier/cross-package.
…bled prompt Renders an agent's fully-assembled system prompt (work dir, OS/shell, merged AGENTS.md, discovered skills) without creating a session, authenticating, or loading MCP. Adds `render_agent_system_prompt` + `build_builtin_system_prompt_args` in soul/agent.py (filesystem/environment only; mirrors the arg assembly in Runtime.create for the read-only inspection path) and a lazy `system-prompt` subcommand. Wave 1 of the reference-adoption arc (adopt-now: dump-system-prompt-entrypoint).
…he enforced constants
bash.md/powershell.md hardcoded the foreground (300s) and background (86400s)
timeout literals, which could silently drift from the MAX_FOREGROUND_TIMEOUT /
MAX_BACKGROUND_TIMEOUT constants that Params actually enforces. Replace the
literals with ${MAX_FOREGROUND_TIMEOUT}/${MAX_BACKGROUND_TIMEOUT} placeholders
rendered from the same constants, and add drift-guard tests (sentinel-render the
markdown; assert the live description states the enforced caps, never a raw
placeholder). Rendered output is identical for the current values.
Wave 1 of the reference-adoption arc (adapt: tool limits interpolated from enforced constants).
The recall block already framed memory as past context that is not an instruction, but did not warn that a recalled fact may be stale — a file, flag, path, or decision it names may have changed or been removed. Add a freshness caveat instructing the model to verify against current code before relying on any recalled fact (failure-truthfulness C13: stale data must not read as authoritative). Single injection site, so the caveat is inlined (no helper). Wave 1 of the reference-adoption arc (adapt: per-memory freshness disclaimer).
Parallel-safe tools previously overlapped without bound: a turn emitting many parallel-safe calls (e.g. dozens of FetchURL) opened that many sockets/file handles at once. Add an asyncio.Semaphore(10) to _ReadWriteGate acquired BEFORE the writer-lock/counter bump so the cap throttles readers without affecting writer draining (a reader queued on the cap has not incremented _active_readers, so it never holds _readers_drained open, and writers never touch the semaphore). Tests cover the cap and the no-deadlock writer-draining invariant. Wave 1 of the reference-adoption arc (adapt: bounded parallel fan-out cap).
Cost was accumulated and displayed but never enforced. Add loop_control.max_session_cost_usd (off by default): when the session's accrued estimated cost reaches the ceiling, the turn stops with a new budget_exhausted outcome and a handoff message before starting another paid step, instead of running to max_steps_per_turn. Goal auto-continuations already halt on any non-no_tool_calls outcome; agent flows now halt on budget_exhausted too. Cost is 0 for unpriced models, so the ceiling fails open (never blocks un-estimable spend). Decision/message logic are pure, boundary-tested helpers. Wave 1 of the reference-adoption arc (adapt: per-session USD spend ceiling).
Add an always-re-confirm deny-set for dangerous host paths so an auto-approve tier can never silently approve them. New is_dangerous_host_path() flags shell startup files, .git internals/hooks, .ssh, .vscode, and git credentials; a new FileActions.EDIT_DANGEROUS is classified BEFORE the workspace branch (so a .git/hooks write inside the workspace is no longer a plain EDIT). The approval gate treats EDIT_DANGEROUS exactly like EDIT_CONFIG via a shared _is_always_confirm_edit predicate: re-confirms even under yolo/auto, never session-approved, denied with feedback in unattended runs. Security-reviewed (SAFE TO MERGE): full parity with the config-edit gate, no fail-open bypass, strict superset of prior protection. Known parity limitation (pre-existing, shared with EDIT_CONFIG, deferred): a .git/.ssh/.vscode directory that is itself a symlink evades name-matching after realpath; Shell-tool writes bypass classify_edit_action entirely. Wave 2 of the reference-adoption arc (adapt: dangerous-dotfile deny-set).
…workspace edits New /accept-edits toggle auto-approves only plain FileActions.EDIT (reversible, in-workspace, non-destructive ordinary file edits) while shell, destructive, outside-workspace, config-surface, and dangerous host edits still prompt. Adds a session-local accept_edits flag (not persisted), _accept_edits_covers() gated by safe_mode (like auto, unlike yolo), a request() auto-approve branch placed after deliberation/unattended-deny, and the /accept-edits slash command. The enable message is safe_mode-truthful (reports edits will still prompt under safe mode). Pairs with the Wave 2 deny-set: dangerous paths classify as EDIT_DANGEROUS (checked first), so they are never covered by EDIT auto-approval. Security-reviewed (SAFE TO MERGE): over-approval bounded to EDIT only; EDIT is produced by exactly one path (classify_edit_action); no persistence leak. Wave 2 of the reference-adoption arc (adapt: accept-edits mode tier).
…p signal) Print mode exits 0 on any non-exception completion, including a stuck/empty terminal that delivered no usable answer. Add a TurnOutcome.produced_answer derived property: True only when the turn ended with a substantive assistant answer (no_tool_calls stop with non-empty text); a forced handoff (stuck / budget_exhausted), a rejected tool call, or an empty final message is False. Surfaced on the existing turn span (turn.produced_answer) and turn.end event so the degenerate-completion rate is measurable. Observational-first: exit-code mapping is deliberately deferred until the rate is measured, to avoid false-positives on tool-only-then-stop turns. Wave 3 of the reference-adoption arc (adapt: terminal-quality success predicate).
A markdown agent's frontmatter can declare required_mcp_servers; spawning that type via the Agent or RunAgents tools is rejected with a clear message when the servers are configured-and-absent (after MCP loading settles), instead of wasting a turn on an agent that cannot reach its tools. While MCP is still loading the spawn is allowed (servers may yet connect). Adds required_mcp_servers to AgentTypeDefinition + markdown frontmatter; a root-only Runtime.mcp_status seam wired from the toolset snapshot; a pure _missing_required_mcp_servers decision function; and AgentTool.check_required_mcp_servers reused by both spawn tools. Also fixes a TurnStopReason annotation in the item-8 test that make check surfaced. Wave 3 of the reference-adoption arc (adapt: required-MCP spawn gate).
…turn A UserPromptSubmit hook could block a prompt but its additionalContext was ignored. Now a non-blocking hook's additional_context is appended to the user turn as a system reminder so the model sees it as context for this prompt, via a pure _user_message_with_hook_context builder. Slash-command parsing keeps reading only the user's original text, never the appended hook context. Ships the clean UserPromptSubmit half per the catalog's staging; the PostToolUse half (await the currently fire-and-forget trigger, gated on has_hooks_for, to inject its additionalContext) is a documented follow-up. Wave 3 of the reference-adoption arc (adapt: hook additionalContext feedback).
Waves 1-3 done (10 items); capture the read-before-write technique, the project-context refactor's test-pin risk, and the max-output-token cross-package blocker for Wave 4 so the remaining work is executable cleanly.
If the agent read a file and it then changed on disk (user edit or another tool), a WriteFile overwrite is now rejected so the external change is not clobbered. ReadFile records the mtime at read; WriteFile rejects an overwrite whose on-disk mtime is newer than the recorded read, and refreshes the read-state after its own write (so consecutive edits are never falsely flagged); StrReplaceFile likewise refreshes after editing. First-contact writes (a file never read) are unaffected. Adapts the reference's read-before-write/file-state-cache to the stale-detection half only — a full read-before-write requirement is incompatible with pythinker's 'write without read' contract (would break ordinary regenerate-file flows). The cache (utils/file_read_cache.py, placed there to avoid a tools<->soul import cycle) keys on the canonical real path, fails open on stat errors, and is per-agent. Unit + e2e tests (stale-blocked, allowed-after-read, consecutive). Wave 4 of the reference-adoption arc (adapt: stale-read detection).
…2, 13) Item 13: reverse-engineered the precise finish_reason seam in PythinkerStreamedMessage -> GenerateResult.truncated -> soul escalation ladder. Item 12: documented the marginal-value/high-brittleness assessment.
pythinker-core captured no finish_reason, so a response cut off by the output-token limit (visible text then cap) was returned as a clean completion — the agent loop could not tell truncation from a normal stop. Capture the provider's finish_reason in PythinkerStreamedMessage (both stream and non-stream paths) and set GenerateResult.truncated when it is 'length'. _generate reads it optionally (getattr) so other stream implementations are unaffected. MockChatProvider gains a finish_reason arg for testing. Stage 1 of the max-output-token recovery ladder (Wave 4, item 13); the soul-side escalate-then-nudge recovery follows in stage 2.
…ation nudges Stage 2 of item 13. StepResult now carries truncated (propagated from GenerateResult). When a step is cut off by the output-token limit and makes no tool call, the soul nudges the model to continue from where it stopped instead of ending the turn with a half-finished answer reported as complete (C06/C13 truthfulness gap). Bounded by loop_control.max_truncation_recoveries (default 3; 0 disables) per turn so a persistently-truncating model cannot loop forever. Decision is a pure, parametrized-tested function; integration tests cover the nudge-and-continue path and the disabled-at-zero path. Wave 4 of the reference-adoption arc (adapt: max-output-token recovery).
Warn (and ignore) a non-list required_mcp_servers frontmatter value, and point a gated spawn at `pythinker mcp auth <server>` for a configured-but-unauthorized server in addition to `pythinker mcp add`. Reformat path.py. Follow-up polish to Wave 3 item 9.
Wave 4 follow-up hardening for the reference-adoption arc: - StrReplaceFile now applies the same stale-read guard as WriteFile via a shared overwrite_is_stale() helper. Exact old-string matching alone cannot catch an external edit that leaves the matched string intact while changing the rest of the file, so gate on the recorded read mtime as WriteFile does. First-contact edits and a tool's own consecutive edits stay unblocked. - Pin the custom .githooks (core.hooksPath) case in the dangerous-host-path deny-set test and call it out in the changelog. - Pin the system-prompt CLI in the PyInstaller hiddenimports expectation so the one-file build keeps shipping it. - Assert the UserPromptSubmit hook-context path wraps stdout in the untrusted-data envelope and strips invisible smuggling characters.
…no-go) - #11 and #13 are committed; mark them DONE in the execution status with their commit refs and the scope adaptations made. - #12 (move AGENTS.md to a separate session-start <system-reminder>) is recorded as an architectural no-go: AGENTS.md must survive compaction and not truncate, and only the system prompt (stored separately, never summarized, 32 KiB budget) satisfies both. A seed message is lossily summarized at the first compaction (prepare() preserves only the last 2 messages verbatim); a dynamic injection is hard-capped at 2048 tokens. Both alternatives would need new compaction-pinning or unbudgeted-injection machinery the project's MVC rules forbid, for marginal non-reference cache value. Keep AGENTS.md in the system prompt.
…nder preamble Move the merged AGENTS.md out of the immutable system prompt (system.md §11) into a session-start, user-role <system-reminder> message that is assembled fresh from runtime state on every model request and is never written to context.history. Robustness — this placement is the whole point. AGENTS.md carries the project's non-negotiable rules, so it must (a) survive compaction and (b) never be truncated. Two tempting homes fail: - a persisted seed message is lossily summarized at the first compaction (the compactor preserves only the last few messages verbatim); - a dynamic injection is hard-capped by the 2048-token injection budget. Assembling it at the step boundary, outside history, sidesteps both: compaction cannot summarize what it never sees, and the injection budget cannot truncate a non-injection. Net per-request tokens are unchanged (it simply moves from the system role to a leading user-role reminder), and the system prompt is now free of per-project content. - render_agents_md_reminder(builtin_args): authoritative <system-reminder> body, or None. - _with_agents_md_preamble(history, builtin_args): prepend it, unmutated, ahead of history. - _step assembles normalize_history(_with_agents_md_preamble(...)). - system.md §11 explains the separate delivery instead of interpolating the block. - pythinker system-prompt appends the reminder below a labeled divider so the dump stays faithful. Resolves the agent.py:66 TODO. Closes Wave 4 #12 of the reference-adoption arc.
…gnal
GenerateResult.truncated drives the agent loop's truncation recovery, but
finish_reason was only an optional getattr property implemented by the Pythinker
and Mock providers. Every other provider returned None, so a reply cut off by the
output-token limit was silently treated as a clean completion — truncation
recovery never fired for Anthropic, OpenAI-legacy, OpenAI-responses, or Google
(4 of 5 real providers). Silent degradation in an execution-critical path.
Make finish_reason a required member of the StreamedMessage Protocol so no
provider can omit it, and map each provider's native truncation signal onto the
loop's 'length' value:
- Anthropic: stop_reason 'max_tokens' -> 'length' (non-stream + message_delta).
- OpenAI-legacy: native choices[0].finish_reason ('length' already).
- OpenAI-responses: incomplete_details.reason 'max_output_tokens' -> 'length'
(handles the response.incomplete terminal event).
- Google GenAI: candidate.finish_reason MAX_TOKENS -> 'length'.
- Chaos: delegates to the wrapped stream; Echo/ScriptedEcho never truncate.
_generate now reads stream.finish_reason directly (contract-guaranteed). Adds a
two-sided test pinning finish_reason='stop' -> not truncated so the negative side
can't pass only because the default is falsy.
…startup gate Two review-driven robustness fixes: - Stale-overwrite guard now tracks (mtime, size), not mtime alone. An external edit within the same filesystem-mtime tick, or one that preserves/restores the mtime (touch -r, archive extraction), left mtime unchanged and slipped past the guard; comparing size as well catches those size-changing edits. ReadFile records both from one stat; Write/StrReplace refresh both after their own write. - mcp_status_snapshot() is now loading-honest: it returns a loading=True snapshot (instead of None) while a deferred MCP startup is queued but has not populated the server map yet. None now means only "no MCP configured", so the required-MCP spawn gate can no longer mistake the startup window for a genuinely-absent server and reject a first-turn subagent. Closes the gap between _missing_required_mcp_servers' documented "[] while still loading" contract and its None-snapshot handling.
…iMax usage - Add Kimi K2.7 Code model and new Kimi Coding Plan provider with Moonshot Anthropic-compatible endpoint - Set GLM-5.2 as default Z.AI model with pinned catalog entry for 1M context - Fix MiniMax token-plan adapter to read percentage-metered usage and convert reset times from milliseconds to seconds - Enhance UI shell modules to surface new providers and integrate new platform catalog entries - Add comprehensive tests for Moonshot and Z.AI auth, and MiniMax usage adapter Reference adoption Wave 2 delivery.
…uppress post-write stat Adversarial-review follow-ups. - Making finish_reason a required StreamedMessage member meant the six soul/auth test doubles that drive a real generate() (StaticStreamedMessage, SequenceStreamedMessage, PartialThenErrorStreamedMessage, and the notifications / cumulative-usage doubles) raised AttributeError at _generate's direct stream.finish_reason read. Give each the no-op property so the now-required contract is satisfied and the suites stay green. - WriteFile's post-write stat() was the only one of the three file tools not wrapped in contextlib.suppress(OSError): a stat hiccup after a successful write fell through to the except handler and reported a false write failure (and skipped the cache refresh, which could then trip a spurious stale flag on retry). Suppress it, keep the success result, and omit the size note when the stat is unavailable — matching read.py and replace.py.
The stale-overwrite guard caught external changes but not the read-completeness
case the deep-scan flagged: the agent reads only part of a large file (a capped or
offset ReadFile), then WriteFile-overwrites it — silently discarding the lines it
never saw, with no external change for the mtime/size guard to catch.
Track read completeness alongside (mtime, size): a ReadFile that started at line 1
and returned every line is complete; a capped/offset read is not. A WriteFile
overwrite of a file whose recorded read was incomplete is now rejected with an
actionable message ("read it in full first"). Scope is deliberately narrow so the
write-without-read contract is preserved: first-contact writes, full-read-then-
overwrite, append, and all StrReplaceFile edits (which preserve unseen bytes) are
unaffected. Completeness is sticky — a full read is not undone by a later partial
re-read of the unchanged bytes — and an overwrite/StrReplace refreshes it to
complete (the agent then knows the whole content), while an append leaves it
incomplete (the prior content is still unseen).
This reverts commit af531ea.
Three visual improvements to the TUI tool-card renderer:
1. **Border reharmonisation** — `border` token updated from the old slate
blue (#3A506D) to a neutral light grey (#e8ebed); `border_muted` follows
suit (#2B3A52 → #b8bcc0). Panels and markdown table borders now recede
instead of competing with accent colours.
2. **RunAgents review findings table** — after a completed RunAgents run
that includes `code-reviewer`, `security-reviewer`, or `review` subagents,
a compact findings panel is appended below the agent tree. It parses
structured severity markers from each reviewer's prose (five forms:
`[SEVERITY]` bullets, colon-bullets, bold-start, named-severity subsection
headers, and markdown table rows) and aggregates Critical / High / Medium /
Low counts with a "Reported by" column listing the contributing agents.
Unparsed reports are tracked separately so the footer ("Parsed N/M · K
kept as unparsed prose") communicates parsing confidence explicitly.
3. **RunAgents name-extra label** — agent entries whose `name_extra` field is
set now render the extra label in bold tool-title style instead of plain
text.
Parser guards:
- `_RE_SEVERITY_IN_HEADER` uses a `(?!-)` lookahead so "## High-level
overview" is not classified as a high-severity section.
- Mid-sentence severity words ("not a high-risk change") are never counted.
17 direct unit tests verify exact severity counts, reporters mapping, and
aggregate behaviour. 7 renderer-level integration tests cover end-to-end
rendering. Related border-colour assertions updated in two existing test files.
- Directory label: periwinkle accent (#B3B9F4) to match skill/highlight color
- Branch label: teal from statusline palette (#64d2c8) to match bottom bar branch
- Diff signs: remove trailing space after +/- so format is {ln} -{content} not {ln} - {content}
- Update tests: accent token assertion for Directory, spacing assertions for diff output
A second candidate with a non-MAX_TOKENS finish_reason (e.g. STOP) would overwrite a previously captured 'length', silently dropping truncation signal. Guard the assignment so 'length' is never overwritten.
Both handoff messages were appended to context but never sent to the wire, so the interactive shell showed no feedback on budget ceiling or stuck-loop exits. Add wire_send(TextPart) after each append_message.
_PROMPT_STYLE_DARK carried six stale slate hexes that diverged from the current _TUI_TOKENS_DARK border/border_muted values. Update them and add a parity test that binds the ptk layer to the token constants.
The agent tree renderer only showed preview text on error/failed agents. Non-review successful agents with a brief or summary_preview now also show a preview line so completed work is visible in the tree.
YAML non-string values (integers, booleans, null) were coerced to strings via str(s), creating permanently unsatisfiable MCP server names. Filter to isinstance(s, str) instead so only real string entries are retained.
Five correctness and UX fixes: Gemini sticky truncation, shell wire handoff for budget/stuck messages, dark-theme ptk hex parity, non-review agent summary preview, and required_mcp_servers type filter.
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThis PR adds truncation reporting and recovery, AGENTS.md reminder delivery, a system-prompt CLI, Kimi auth and model updates, session cost and approval controls, MCP spawn gating, stale file overwrite protection, shell/UI rendering changes, and matching tests and release notes. ChangesCore agent and platform update
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…cements # Conflicts: # CHANGELOG.md
![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: 8
🤖 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/auth/platforms.py`:
- Around line 446-447: The condition at line 446 in the apply_kimi_models block
and at line 461 in the corresponding block should check not only that
kimi_models is not None but also that it is non-empty. Replace the condition `if
kimi_models is not None` with `if kimi_models is not None and kimi_models` at
both locations (lines 446 and 461) to ensure that empty discovery results are
not treated as authoritative, thereby preventing the pruning of all Kimi models
when an empty tuple is returned from discovery.
In `@src/pythinker_code/cli/system_prompt.py`:
- Around line 65-66: The config loading logic at line 65 uses `load_config()`
only when the user config file exists, falling back to empty `Config()`
otherwise. This skips project/local scoped configs that should be loaded via the
normal scoped pipeline. Replace the conditional logic that checks for user
config existence with a call to `load_config()` directly (or an alternative
scoped loader that doesn't require user config to exist) so that project and
local scoped configs are always resolved and included in the prompt dump,
matching runtime behavior. Additionally, add a regression test covering the
scenario where no user config file exists but a project config file is present.
In `@src/pythinker_code/soul/pythinkersoul.py`:
- Around line 1507-1520: The spend ceiling check using _is_over_cost_ceiling()
happens before the step loop, but compact_context() can make billable LLM calls
that increase self._session_cost_usd after the check executes but before the
next step runs. Fix this by re-checking the ceiling immediately after
compact_context() is called (near line 1549) to ensure the session hasn't
exceeded max_session_cost_usd due to compaction costs. If the ceiling is
exceeded after compaction, return a TurnOutcome with stop_reason set to
budget_exhausted, similar to the existing guard at lines 1507-1520, so that the
configured ceiling actually bounds total session spending.
In `@src/pythinker_code/soul/slash.py`:
- Around line 209-210: The accept_edits function is missing a return type
annotation required by the repository's public-function typing rule. Add the
return type annotation `-> None` to the function signature of accept_edits to
declare that this async command handler does not return a value.
In `@src/pythinker_code/tools/file/replace.py`:
- Around line 397-409: There is a TOCTOU (time-of-check-time-of-use) gap in the
file replacement logic. The staleness check using overwrite_is_stale at the
current location (lines 397-409) happens too early, and the file can be modified
externally between this check and the actual write_text call at line 504. Add a
second overwrite_is_stale check immediately before the write_text operation and
return the same ToolError with the stale-read message if the file has been
modified since the initial check.
In `@tests/core/test_pythinkersoul_steer.py`:
- Around line 454-456: Add an explicit return type annotation to the
finish_reason property method to comply with the ANN202 linting rule. The method
currently has no return type hint; add the appropriate return type annotation
(-> None) after the method signature to indicate that the property returns None.
In `@tests/tools/test_agent_tool.py`:
- Line 79: Replace the hardcoded `/tmp/needs_db.yaml` path in the agent_file
parameter with a path constructed using runtime.subagent_store.root to ensure
cross-platform compatibility. Use the same pattern as the other locations in the
file (such as the instances around lines 110, 155, and 199) by constructing the
path as runtime.subagent_store.root / "needs_db.yaml" or similar approach that
leverages the runtime store's root directory instead of a hardcoded temporary
directory.
- Line 88: Split the combined assertion at the test location into two separate
assertions for clearer failure diagnostics. Replace the single assert statement
that checks both `err is not None` and `"db" in err.message` with two distinct
assertions: one to verify that err is not None, and another to verify that "db"
appears in err.message. This way, if the test fails, the error message will
clearly indicate which specific condition failed.
🪄 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: e39961c8-475a-4392-8ebe-b21554b2291d
⛔ Files ignored due to path filters (1)
tasks/reference-adoption-catalog.mdis excluded by!tasks/**
📒 Files selected for processing (89)
AGENTS.mdCHANGELOG.mdpackages/pythinker-core/src/pythinker_core/__init__.pypackages/pythinker-core/src/pythinker_core/_generate.pypackages/pythinker-core/src/pythinker_core/chat_provider/__init__.pypackages/pythinker-core/src/pythinker_core/chat_provider/chaos.pypackages/pythinker-core/src/pythinker_core/chat_provider/echo/echo.pypackages/pythinker-core/src/pythinker_core/chat_provider/echo/scripted_echo.pypackages/pythinker-core/src/pythinker_core/chat_provider/mock.pypackages/pythinker-core/src/pythinker_core/chat_provider/pythinker.pypackages/pythinker-core/src/pythinker_core/contrib/chat_provider/anthropic.pypackages/pythinker-core/src/pythinker_core/contrib/chat_provider/google_genai.pypackages/pythinker-core/src/pythinker_core/contrib/chat_provider/openai_legacy.pypackages/pythinker-core/src/pythinker_core/contrib/chat_provider/openai_responses.pypackages/pythinker-core/tests/api_snapshot_tests/test_google_genai.pypackages/pythinker-core/tests/test_anthropic_finish_reason.pypackages/pythinker-core/tests/test_generate.pypackages/pythinker-core/tests/test_pythinker_stream_usage.pysrc/pythinker_code/agents/default/system.mdsrc/pythinker_code/auth/__init__.pysrc/pythinker_code/auth/kimi.pysrc/pythinker_code/auth/moonshot.pysrc/pythinker_code/auth/platforms.pysrc/pythinker_code/auth/z_ai.pysrc/pythinker_code/cli/_lazy_group.pysrc/pythinker_code/cli/system_prompt.pysrc/pythinker_code/config.pysrc/pythinker_code/memory/recall.pysrc/pythinker_code/soul/agent.pysrc/pythinker_code/soul/approval.pysrc/pythinker_code/soul/flow_runner.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/soul/slash.pysrc/pythinker_code/soul/toolset.pysrc/pythinker_code/subagents/discovery.pysrc/pythinker_code/subagents/models.pysrc/pythinker_code/tools/agent/__init__.pysrc/pythinker_code/tools/file/__init__.pysrc/pythinker_code/tools/file/read.pysrc/pythinker_code/tools/file/replace.pysrc/pythinker_code/tools/file/write.pysrc/pythinker_code/tools/shell/__init__.pysrc/pythinker_code/tools/shell/bash.mdsrc/pythinker_code/tools/shell/powershell.mdsrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/components/diff.pysrc/pythinker_code/ui/shell/oauth.pysrc/pythinker_code/ui/shell/stats_pricing.pysrc/pythinker_code/ui/shell/tool_renderers/agent.pysrc/pythinker_code/ui/shell/usage_adapters/minimax.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/ui/theme.pysrc/pythinker_code/utils/file_read_cache.pysrc/pythinker_code/utils/path.pytests/auth/test_kimi_auth.pytests/auth/test_moonshot_auth.pytests/auth/test_z_ai_auth.pytests/cli/test_system_prompt_cli.pytests/core/test_approval_auto.pytests/core/test_auth_error_handling.pytests/core/test_config.pytests/core/test_cumulative_usage.pytests/core/test_load_agent.pytests/core/test_notifications.pytests/core/test_pythinkersoul_ralph_loop.pytests/core/test_pythinkersoul_retry_recovery.pytests/core/test_pythinkersoul_steer.pytests/core/test_pythinkersoul_stuck_loop.pytests/core/test_recall_provider.pytests/core/test_subagent_discovery.pytests/core/test_toolset_concurrency.pytests/tools/test_agent_tool.pytests/tools/test_file_read_cache.pytests/tools/test_mcp_startup_timeout.pytests/tools/test_shell_timeout_drift.pytests/tools/test_str_replace_file.pytests/tools/test_write_file.pytests/ui/test_console_theme.pytests/ui/usage_adapters/test_minimax.pytests/ui_and_conv/test_review_findings_parser.pytests/ui_and_conv/test_shell_panel.pytests/ui_and_conv/test_shell_welcome_info.pytests/ui_and_conv/test_tui_card_tool_renderers.pytests/ui_and_conv/test_tui_theme_tokens.pytests/ui_and_conv/test_visualize_running_prompt.pytests/utils/test_pyinstaller_utils.pytests_e2e/test_wire_protocol.py
💤 Files with no reviewable changes (1)
- src/pythinker_code/ui/shell/visualize/_blocks.py
…nits - recall.py: make the two wrapped recall-block list entries explicit `+` concatenations instead of relying on implicit adjacent-literal concat (CodeQL py/implicit-string-concatenation-in-list). Output is byte-identical. - test_shell_timeout_drift.py: drop the redundant `from pythinker_code.tools.shell` import and qualify Shell/MAX_*_TIMEOUT through the already-imported `shell_mod` so the module is imported in a single style (CodeQL mixed import).
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
pythinker system-prompt fell back to an empty Config() whenever ~/.pythinker/config.toml was absent, silently dropping project/local scoped settings (e.g. extra_skill_dirs) and printing an inaccurate prompt. Add a persist=False mode to load_config/_load_scoped that runs the full user→project→ local merge with no disk side effects — no share-dir/lock creation, default seeding, JSON→TOML migration, or auto-gitignore, and skipping the project-trust read (trust only gates hooks, which no read-only consumer renders). The default persist=True path is byte-for-byte unchanged. Regression test covers no-user-config + project-config-present and asserts no config is seeded.
The per-session spend ceiling was checked only at the top of the step loop, but proactive compact_context() makes a billable LLM call that raises _session_cost_usd. A session just under max_session_cost_usd could therefore pay for compaction and still run a full model step before the guard re-fired. Add a second ceiling check immediately after the proactive compaction (before _step()), reusing the existing budget_exhausted handoff. Regression test asserts no model step runs once compaction pushes the session over the ceiling.
StrReplaceFile and WriteFile validated staleness before the (unbounded) approval prompt, then wrote the in-memory content wholesale — so an external edit landing during approval was silently clobbered (exact-string matching ran pre-approval and gives no write-time protection). Add a second overwrite_is_stale / _reject_if_stale check immediately after approval, returning the same stale-read error. Regression tests mutate the file during the approval call and assert the external change survives.
- slash.py: annotate the new accept_edits handler with -> None (public src rule). - test_pythinkersoul_steer.py: annotate the finish_reason test-double property. - test_agent_tool.py: replace the hardcoded /tmp agent_file with the in-file runtime.subagent_store.root pattern, and split the combined assertion for clearer failure messages.
Fix the two repo-wide spellcheck failures flagged by the typos CI gate: 'togglable' -> 'toggleable' in a test comment, and 'ede_diagnostic' -> 'edge_diagnostic' in a planning note.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pythinker_code/tools/file/write.py (1)
208-217:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe new approval-window stale checks still miss first-contact edits. Both paths only re-check against
file_read_cache, which means they protect priorReadFilestate but not the tool's own pre-approval read. On the first overwrite/replace in a session, an external edit during approval can still slip through and be silently clobbered.
src/pythinker_code/tools/file/write.py#L208-L217: after readingold_text, snapshot the file's(mtime, size)and compare against that snapshot before Line 227, instead of relying solely on the cache-backed stale check.src/pythinker_code/tools/file/replace.py#L498-L510: after readingoriginal_content, do the same snapshot/re-check before Line 518 so first-contact replace operations cannot overwrite approval-window edits.🤖 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/file/write.py` around lines 208 - 217, The stale-check mechanism currently only validates against file_read_cache, which means first-contact overwrites/replaces during the approval window can miss external edits that occur between the tool's initial read and the write. In src/pythinker_code/tools/file/write.py, after the old_text read (before line 227 where the actual write happens), capture a snapshot of the file's mtime and size, then before writing validate against this snapshot in addition to the existing _reject_if_stale cache check. Apply the same fix in src/pythinker_code/tools/file/replace.py by snapshotting original_content's mtime and size (before line 518 where the replacement write occurs), then validating the file has not changed on disk since that snapshot was taken. This ensures that approval-window edits are detected even on the first overwrite/replace operation in a session.
🤖 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/_lazy_group.py`:
- Around line 53-57: Add a backward-compatible alias for the vis command that
maps to the same module and function as the dashboard command. In the commands
dictionary (where the dashboard entry exists), add a vis entry pointing to
pythinker_code.cli.dashboard and the cli function with an appropriate
description. Ensure the vis entry is added to the commands dictionary but
excluded from the lazy_command_order list to maintain hidden backward
compatibility while keeping it out of the help text.
---
Duplicate comments:
In `@src/pythinker_code/tools/file/write.py`:
- Around line 208-217: The stale-check mechanism currently only validates
against file_read_cache, which means first-contact overwrites/replaces during
the approval window can miss external edits that occur between the tool's
initial read and the write. In src/pythinker_code/tools/file/write.py, after the
old_text read (before line 227 where the actual write happens), capture a
snapshot of the file's mtime and size, then before writing validate against this
snapshot in addition to the existing _reject_if_stale cache check. Apply the
same fix in src/pythinker_code/tools/file/replace.py by snapshotting
original_content's mtime and size (before line 518 where the replacement write
occurs), then validating the file has not changed on disk since that snapshot
was taken. This ensures that approval-window edits are detected even on the
first overwrite/replace operation in a session.
🪄 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: 3866570e-d3e0-4266-870a-122d85c55e1d
⛔ Files ignored due to path filters (2)
docs/en/release-notes/changelog.mdis excluded by!docs/**tasks/reference-adoption-catalog.mdis excluded by!tasks/**
📒 Files selected for processing (20)
AGENTS.mdCHANGELOG.mdsrc/pythinker_code/cli/_lazy_group.pysrc/pythinker_code/cli/system_prompt.pysrc/pythinker_code/config.pysrc/pythinker_code/memory/recall.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/soul/slash.pysrc/pythinker_code/tools/file/replace.pysrc/pythinker_code/tools/file/write.pysrc/pythinker_code/ui/shell/__init__.pytests/cli/test_system_prompt_cli.pytests/core/test_pythinkersoul_steer.pytests/core/test_pythinkersoul_stuck_loop.pytests/tools/test_agent_tool.pytests/tools/test_shell_timeout_drift.pytests/tools/test_str_replace_file.pytests/tools/test_write_file.pytests/ui_and_conv/test_settings_selector.pytests/utils/test_pyinstaller_utils.py
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pythinker_code/tools/file/write.py (1)
208-217:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe new approval-window stale checks still miss first-contact edits. Both paths only re-check against
file_read_cache, which means they protect priorReadFilestate but not the tool's own pre-approval read. On the first overwrite/replace in a session, an external edit during approval can still slip through and be silently clobbered.
src/pythinker_code/tools/file/write.py#L208-L217: after readingold_text, snapshot the file's(mtime, size)and compare against that snapshot before Line 227, instead of relying solely on the cache-backed stale check.src/pythinker_code/tools/file/replace.py#L498-L510: after readingoriginal_content, do the same snapshot/re-check before Line 518 so first-contact replace operations cannot overwrite approval-window edits.🤖 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/file/write.py` around lines 208 - 217, The stale-check mechanism currently only validates against file_read_cache, which means first-contact overwrites/replaces during the approval window can miss external edits that occur between the tool's initial read and the write. In src/pythinker_code/tools/file/write.py, after the old_text read (before line 227 where the actual write happens), capture a snapshot of the file's mtime and size, then before writing validate against this snapshot in addition to the existing _reject_if_stale cache check. Apply the same fix in src/pythinker_code/tools/file/replace.py by snapshotting original_content's mtime and size (before line 518 where the replacement write occurs), then validating the file has not changed on disk since that snapshot was taken. This ensures that approval-window edits are detected even on the first overwrite/replace operation in a session.
🤖 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/_lazy_group.py`:
- Around line 53-57: Add a backward-compatible alias for the vis command that
maps to the same module and function as the dashboard command. In the commands
dictionary (where the dashboard entry exists), add a vis entry pointing to
pythinker_code.cli.dashboard and the cli function with an appropriate
description. Ensure the vis entry is added to the commands dictionary but
excluded from the lazy_command_order list to maintain hidden backward
compatibility while keeping it out of the help text.
---
Duplicate comments:
In `@src/pythinker_code/tools/file/write.py`:
- Around line 208-217: The stale-check mechanism currently only validates
against file_read_cache, which means first-contact overwrites/replaces during
the approval window can miss external edits that occur between the tool's
initial read and the write. In src/pythinker_code/tools/file/write.py, after the
old_text read (before line 227 where the actual write happens), capture a
snapshot of the file's mtime and size, then before writing validate against this
snapshot in addition to the existing _reject_if_stale cache check. Apply the
same fix in src/pythinker_code/tools/file/replace.py by snapshotting
original_content's mtime and size (before line 518 where the replacement write
occurs), then validating the file has not changed on disk since that snapshot
was taken. This ensures that approval-window edits are detected even on the
first overwrite/replace operation in a session.
🪄 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: 3866570e-d3e0-4266-870a-122d85c55e1d
⛔ Files ignored due to path filters (2)
docs/en/release-notes/changelog.mdis excluded by!docs/**tasks/reference-adoption-catalog.mdis excluded by!tasks/**
📒 Files selected for processing (20)
AGENTS.mdCHANGELOG.mdsrc/pythinker_code/cli/_lazy_group.pysrc/pythinker_code/cli/system_prompt.pysrc/pythinker_code/config.pysrc/pythinker_code/memory/recall.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/soul/slash.pysrc/pythinker_code/tools/file/replace.pysrc/pythinker_code/tools/file/write.pysrc/pythinker_code/ui/shell/__init__.pytests/cli/test_system_prompt_cli.pytests/core/test_pythinkersoul_steer.pytests/core/test_pythinkersoul_stuck_loop.pytests/tools/test_agent_tool.pytests/tools/test_shell_timeout_drift.pytests/tools/test_str_replace_file.pytests/tools/test_write_file.pytests/ui_and_conv/test_settings_selector.pytests/utils/test_pyinstaller_utils.py
🛑 Comments failed to post (1)
src/pythinker_code/cli/_lazy_group.py (1)
53-57:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
visas a backward-compatible alias fordashboard.Dropping the previous command key breaks existing CLI invocations/scripts immediately. Keep an alias mapped to the same module (you can keep it out of
lazy_command_orderto hide it from help while preserving compatibility).Suggested minimal fix
lazy_subcommands: dict[str, tuple[str, str, str]] = { @@ "dashboard": ( "pythinker_code.cli.dashboard", "cli", "Run Pythinker Agent Tracing Visualizer.", ), + # Backward-compatible alias; intentionally omitted from lazy_command_order/help. + "vis": ( + "pythinker_code.cli.dashboard", + "cli", + "Deprecated alias for dashboard.", + ), "web": ("pythinker_code.cli.web", "cli", "Run Pythinker CLI web interface."), }As per coding guidelines, “Preserve public compatibility. CLI flags ... semantics need tests/docs when changed.”
Also applies to: 72-72
🤖 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/_lazy_group.py` around lines 53 - 57, Add a backward-compatible alias for the vis command that maps to the same module and function as the dashboard command. In the commands dictionary (where the dashboard entry exists), add a vis entry pointing to pythinker_code.cli.dashboard and the cli function with an appropriate description. Ensure the vis entry is added to the commands dictionary but excluded from the lazy_command_order list to maintain hidden backward compatibility while keeping it out of the help text.Source: Coding guidelines
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
Five correctness and UX fixes from the deep-scan review of this branch, implemented with TDD (failing test first, gate confirmed green after fix).
MAX_TOKENSfinish reason no longer overwrites a previously captured"length"signal. Fixed at both the streaming and non-streaming candidate-iteration paths in the Google GenAI provider. Three regression tests added (non-stream, streaming, and reverse-order scenarios).wire_send(TextPart(...))added at both paths; two tests added asserting the text appears in wire events._PROMPT_STYLE_DARKhad diverged from the current_TUI_TOKENS_DARK.border/.border_mutedtoken constants. Updated to match; parity test added that binds the ptk layer to the token constants so future drift is caught automatically.summary_previewnow show a dim preview line in the RunAgents tree renderer. Review runs continue to use the findings table (unaffected).required_mcp_serversvalues dropped — YAML integers, booleans, andnullentries were coerced to strings ("1","False","None") viastr(s), creating permanently unsatisfiable MCP server names that caused first-turn subagent spawns to be wrongly rejected. Replaced withisinstance(s, str)filter; regression test added.Test plan
make check-pythinker-core— 0 errorsmake check-pythinker-code— 0 errorsmake test-pythinker-core— 307 passed (3 new sticky-truncation tests)make test-pythinker-code— 5697 passed, 7 skipped, 1 xfailedSummary by CodeRabbit
Release Notes
/accept-editsto auto-approve reversible in-workspace edits, plus new spend ceiling (max_session_cost_usd) behavior when budget is exhaustedpythinker system-promptto print the fully assembled system prompt without starting a session