fix: background reliability, TUI/agent loop, and CodeRabbit hardening - #13
Merged
Conversation
LLMs occasionally serialize the todos array as a JSON string instead of a proper JSON array, causing Pydantic validation to fail with "Input should be a valid list". Add a before-validator that transparently parses the string via json.loads when detected.
Validated subset of the TUI/background-agent reliability scan. Changes: - background: serialise the runtime read-modify-write in every _mark_task_* and in recover() under a cross-process per-task lock (store._runtime_lock + _write_runtime_unlocked) so a worker heartbeat landing mid-sequence is no longer lost; add a SIGTERM->SIGKILL escalation fallback in _best_effort_kill that only fires if the task is still running; cap a bash task's output.log at config.background.max_output_bytes (default 50 MiB) so a chatty task cannot exhaust disk. - pythinker-host: skip the process-group kill once the child has exited and been reaped, since the OS may have recycled its pid/pgid. - web fetch: follow redirects manually and re-validate every hop against the SSRF guard, closing a public->link-local (metadata endpoint) redirect bypass. - grep fallback: bound the pure-Python search with a wall-clock deadline mirroring the ripgrep timeout and report partial results. - browser launch: route OAuth/feedback URL opens through a detached open_url_in_browser() so browser chatter cannot corrupt the TUI or consume key presses meant for it. - cli: restore the terminal to a sane state on SIGTERM/SIGQUIT and via atexit. - live view: use the theme "warning" token instead of a hardcoded accent. - mcp/toolset: annotate the fastmcp OAuth provider as Any for pyright. Tests added/updated across the background, tools, ui, and host suites.
… terminal tasks (M9) H2: route every terminal agent-task update through one BackgroundTaskManager.finalize_agent_task() that writes the authoritative TaskRuntime first and the derived subagent record last, replacing the eight ad-hoc (update_instance, _mark_task_*) pairs in BackgroundAgentRunner whose ordering was inconsistent (run() wrote the record first, _run_core the task). recover() now reconciles a subagent record still stuck at running_background to the status implied by the authoritative TaskRuntime — for terminal tasks too — closing the crash/kill window that left TaskRuntime and AgentInstanceRecord divergent. A resumed running_foreground or already-terminal record is never clobbered. M9: prune terminal background-task directories older than config.background.task_retention_days (default 7) opportunistically during reconcile(); never removes non-terminal tasks or tasks whose worker is alive. Tests: crash/kill reconciliation, foreground no-clobber, finalize end-state, reconcile pruning, and cleanup_old_tasks unit coverage.
…rminal task The H2 recover() reconciliation could corrupt a currently-running agent. When an agent_id is reused — a background resume mints a new task_id while the prior run's task stays terminal in the store — recover() saw the old terminal task alongside a running_background record and reset the live agent's record to the old task's terminal status. AgentInstanceRecord.last_task_id is not maintained, so gate the reconcile on the set of agent_ids owned by live in-process tasks and skip those. Regression test: old terminal task + live resumed task sharing one agent_id; the live agent's running_background record must survive recover().
When a turn is in flight the pinned status tail already renders the todo list under its verb spinner. The background-task status line was reading the same `_latest_todos` and appending the rows again, so the list showed twice while the agent worked. Restrict the duplicated rows to the between-turns case (show_verb=True) where the background line is the only surface; suppress them when the pinned tail is active.
Blocking on a single task with TaskOutput(block=true) waits only for that task and freezes the turn until the slowest sibling finishes, so completion notifications for the others land with no listener. Add that guidance to the TaskOutput tool description, the Agent tool's next_step hints, and the idle-completion system-reminder (which now reports how many background tasks are still running). Steers the model to return control and rely on automatic re-wake instead.
Models sometimes end a message with a transitional preamble ("Let me
synthesize the findings into a unified report.") but attach no tool call
and produce no result. The loop treats any tool-call-free message as the
final answer, so the turn ends before the promised work is done. Detect
that shape conservatively and inject a one-shot system-reminder asking the
model to deliver the result or make the tool call. Capped at once per turn
so a stubborn model can still finish.
- background/manager: re-read TaskControl inside the recovery lock so a kill landing between list_views() and lock acquisition is honored as killed, not mislabeled lost; derive the subagent record from the runtime that actually won the terminal race instead of the requested outcome. - cli: guard SIGQUIT registration behind hasattr — it is POSIX-only and was crashing CLI startup on Windows. - grep_local: make _iter_python_search_files lazy so the per-file timeout check fires during discovery instead of after the whole tree is walked. - web/fetch: drop the misleading await on the synchronous response.release(). - ui/shell/slash: add missing return type annotation. - tests: add a fail-closed validation test for SetTodoList params.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds background-task runtime locks and cleanup, recovery and finalize flows with delayed SIGKILL escalation, worker output-size limiting, SSRF-safe redirect handling, a platform-aware browser opener, unfinished-intent nudging, UI/prompt tweaks, CLI/LocalHost safety fixes, and many tests. ChangesBackground Task Lifecycle and Tooling Enhancements
Sequence Diagram(s)(Skipped — main flows are internal lifecycle reconciliations and file-level helpers; no cross-system sequence requiring extra diagram.) Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested Labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- typos: accept the verb stems (prepar/generat/provid/updat/examin/continu) used as word-prefix alternatives in the unfinished-intent detection regex. - ruff format packages/pythinker-host/tests/test_local_host.py, which was left unformatted and failed `make check-pythinker-host` across the matrix.
elkaix
changed the title
![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: 4
🤖 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-host/tests/test_local_host.py`:
- Around line 202-255: The file contains formatting changes flagged by Ruff; run
the code formatter and commit the result so CI passes: format the tests
containing the functions test_kill_skips_signal_after_process_exit and
test_kill_signals_running_process (the test_local_host module) using your
project's Ruff formatter (e.g., the ruff format command or your pre-configured
formatter), then stage and commit the updated file before pushing.
In `@src/pythinker_code/soul/pythinkersoul.py`:
- Around line 234-236: The unfinished-intent regex contains misspelled token
stems that break the typo-check pipeline; update the alternatives inside the
regex (the sequences like "synthesi", "summari", and "produc") to correctly
spelled stems while preserving the trailing \w* matching behavior—for example
replace "synthesi" with "synthes", "summari" with "summar", and "produc" with
"produce" (and similarly correct any other mistyped stems) in the regex string
used in pythinker_code/soul/pythinkersoul.py so the pattern continues to match
inflected forms but no longer contains typos.
In `@src/pythinker_code/ui/shell/__init__.py`:
- Around line 796-804: The code currently swallows all exceptions when computing
active_running using list_task_views inside the PythinkerSoul branch; replace
the contextlib.suppress block with a try/except that catches Exception as e,
logs the exception with diagnostic context (e.g., using the module/logger in
this file) and then falls back to active_running = 0 so the code can continue;
keep the surrounding check for isinstance(self.soul, PythinkerSoul) and ensure
the logged message references list_task_views,
self.soul.runtime.background_tasks, and the fact this affects
_background_idle_reminder/run_soul_command.
In `@tests/tools/test_grep.py`:
- Around line 12-19: Replace assertions that target the private helper
_python_grep and module-level time monkeypatch with a test that exercises the
public Grep API: call await Grep()(Params(...)) and assert the timeout behavior
from that call. Force the implementation to use the Python fallback by making
the module choose no ripgrep binary (e.g., monkeypatch _find_existing_rg to
return None or set _rg_binary_name to None) so Grep falls back to _python_grep
internally, then assert the observable timeout/exception from the awaited Grep()
invocation rather than inspecting _python_grep directly.
🪄 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: 3a29d4d6-c697-4686-a846-9c7276d8e6dc
📒 Files selected for processing (36)
packages/pythinker-host/src/pythinker_host/local.pypackages/pythinker-host/tests/test_local_host.pysrc/pythinker_code/auth/github_feedback.pysrc/pythinker_code/auth/oauth.pysrc/pythinker_code/auth/openai.pysrc/pythinker_code/background/agent_runner.pysrc/pythinker_code/background/manager.pysrc/pythinker_code/background/store.pysrc/pythinker_code/background/worker.pysrc/pythinker_code/cli/__init__.pysrc/pythinker_code/cli/mcp.pysrc/pythinker_code/config.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/soul/toolset.pysrc/pythinker_code/tools/agent/__init__.pysrc/pythinker_code/tools/background/output.mdsrc/pythinker_code/tools/file/grep_local.pysrc/pythinker_code/tools/todo/__init__.pysrc/pythinker_code/tools/web/fetch.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/visualize/_live_view.pysrc/pythinker_code/utils/term.pytests/background/test_manager.pytests/background/test_store.pytests/background/test_worker.pytests/core/test_unfinished_intent_nudge.pytests/tools/test_fetch_url.pytests/tools/test_grep.pytests/tools/test_todo.pytests/tools/test_tool_descriptions.pytests/ui_and_conv/test_background_idle_reminder.pytests/ui_and_conv/test_live_view_todos.pytests/ui_and_conv/test_shell_feedback_slash.pytests/ui_and_conv/test_visualize_running_prompt.py
- test_local_host: route POSIX-only os.killpg / signal.SIGKILL through Any holders so the strict host pyright gate passes on the win32 platform stubs (the test is already skipped at runtime on Windows). Verified with `pyright --pythonplatform Windows`. - ruff format tests/ui_and_conv/test_shell_feedback_slash.py, which failed the main-package `ruff format --check` gate.
The default-config dump snapshot was stale: earlier commits added `task_retention_days` and `max_output_bytes` to BackgroundConfig without updating tests/core/test_config.py, failing test-pythinker-code across the matrix. Add both fields in dump order.
…blic API Address two CodeRabbit review findings on PR #13: - ui/shell: replace contextlib.suppress(Exception) around the idle-reminder active-task count with a try/except that logs at debug (active_running still falls back to 0), so background-task introspection failures are no longer invisible. - tests/tools/test_grep: exercise the Python fallback's wall-clock bound through the public Grep() API with a forced ripgrep-unavailable path instead of calling _python_grep directly, and drop the now-unused import.
This was referenced May 29, 2026
elkaix
added a commit
that referenced
this pull request
Jun 14, 2026
…eme parity, agent preview, MCP type filter) (#143) * docs(tasks): add verified reference-adoption catalog 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. * feat(cli): add read-only `system-prompt` command to inspect the assembled 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). * fix(shell): interpolate timeout caps into the tool description from the 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). * feat(memory): warn that recalled notes are a point-in-time snapshot 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). * feat(toolset): bound parallel-safe tool fan-out with a concurrency cap 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). * feat(loop): enforce an optional per-session USD spend ceiling 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). * feat(approval): always re-confirm edits to sensitive host files 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). * feat(approval): add accept-edits auto-approve tier for reversible in-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). * feat(loop): add terminal-quality predicate (observable degenerate-stop 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). * feat(subagents): gate spawn on declarative required MCP servers 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). * feat(hooks): inject UserPromptSubmit additionalContext into the user 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). * docs(tasks): record adoption-arc status + Wave 4 execution notes 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. * feat(file): stale-overwrite guard via a per-agent file read cache 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). * docs(tasks): record executable plan for the remaining Wave 4 items (12, 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. * feat(core): surface a truncation signal on GenerateResult 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. * feat(loop): recover from output-token truncation with bounded continuation 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). * polish(subagents): harden required-MCP gate diagnostics 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. * feat(file): extend the stale-overwrite guard to StrReplaceFile 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. * docs(tasks): record Wave 4 outcomes (#11/#13 done, #12 architectural 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. * feat(prompt): deliver merged AGENTS.md as a session-start system-reminder 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. * fix(core): make output-token truncation a required StreamedMessage signal 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. * fix(file,mcp): harden stale-overwrite detection and the required-MCP 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. * feat(auth,providers): add Kimi provider and GLM-5.2 defaults, fix MiniMax 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. * fix(core,file): satisfy the finish_reason contract in test doubles; suppress 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. * feat(file): reject a full overwrite of a partially-read file 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). * Revert "feat(file): reject a full overwrite of a partially-read file" This reverts commit af531ea. * feat(tui): review findings table, lighter borders, and RunAgents polish 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. * feat(tui): standardize welcome banner colors and diff sign spacing - 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 * fix(core): make Gemini finish_reason sticky once 'length' is captured 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. * test(core): add streaming + reverse-order sticky-truncation test coverage * fix(soul): wire budget-exhausted and stuck-loop messages to shell 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. * fix(ui): sync dark-theme ptk hex values to TUI token constants _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. * fix(ui): restore preview line for non-review successful agents 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. * fix(subagents): drop non-string values from required_mcp_servers 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. * docs(changelog): add Unreleased entries for deep-scan fixes 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. * style(memory,tests): satisfy CodeQL implicit-concat and mixed-import 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). * fix(config): resolve scoped config for read-only system-prompt dump 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. * fix(soul): re-check spend ceiling after billable compaction 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. * fix(file): re-check staleness after approval to close TOCTOU window 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. * style(soul,tests): address CodeRabbit type/hygiene nits - 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(typo): toggleable, edge_diagnostic 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.
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
Hardens background-task reliability and fixes several agent-loop / TUI papercuts surfaced while running multi-agent scans, plus a batch of CodeRabbit review findings.
Background task reliability
TaskRuntimewritten before the derived subagent record, withrecover()reconciling a divergent pair after a mid-write crash.agent_id).TaskControlinside the runtime lock, so a kill landing betweenlist_views()and lock acquisition is honored askilledrather than mislabeledlost.finalize_agent_taskderives the subagent status from the runtime that actually won the terminal race, not the requested outcome.Agent loop & TUI
TaskOutput(block=true)on a single task when siblings are still running — blocking freezes the turn until the slowest finishes and strands the others' completion notifications. Guidance added to the tool description, Agentnext_stephints, and the idle-completion reminder (which now reports how many tasks are still running).CodeRabbit findings
SIGQUITregistration behindhasattr— POSIX-only; was crashing CLI startup on Windows._iter_python_search_fileslazy so the grep fallback's per-file timeout check fires during discovery instead of after the whole tree is walked.awaiton the synchronousresponse.release()in the redirect-revalidation path.parse JSON-encoded string passed as todos list(fail-closed on invalid JSON) + added regression test.Testing
pytestacross affected suites (background manager/store/worker, todo, grep, fetch, tool descriptions, soul intent-nudge, TUI render) — all green.ruffandpyrightclean on all changed source files.Summary by CodeRabbit
New Features
Improvements
Bug Fixes