feat: agent harness enhancements - #122
Merged
Merged
Conversation
FetchURL wrote pre-rendered <untrusted_data> envelopes into the ToolResultBuilder, so when a page exceeded the builder's character limit, truncation cut off the closing </untrusted_data> tag. The model then received an unterminated untrusted-data envelope (the exact failure mode the wrapper exists to prevent), and strip_untrusted_envelope could not strip the torn envelope, so the raw envelope leaked into display/UI paths. Write the raw text and call builder.mark_untrusted() instead (the same idiom SearchWeb uses), so wrapping happens after truncation in ok() and the closing tag can never be cut. Applied to all three write sites: the verbatim text/plain+markdown path, the trafilatura extraction path, and the fetch-service path. The spill file now holds raw unwrapped output, matching the documented spill contract. Regression tests cover all three paths with >50k-char pages that trigger builder truncation, asserting the envelope stays well-formed and strip_untrusted_envelope round-trips.
External drivers (FlowRunner._flow_turn, the /goal and /learn slash handlers) were calling the private PythinkerSoul._turn directly, each carrying a pyright reportPrivateUsage suppression. Add a public turn() method that documents the single-turn contract: one user message in, one full agent turn out (model steps plus tool calls until the model stops), with TurnOutcome conveying stop_reason (no_tool_calls / tool_rejected / stuck), the final assistant message, and step count. turn() does not emit TurnBegin/TurnEnd wire framing; callers frame the turn themselves, as run() does. turn() is a thin delegate to _turn on purpose: many tests monkeypatch soul._turn to stub turn execution, so _turn stays the single implementation/patch point and those patches keep intercepting turns started through turn(). The three external call sites now use turn() with the suppressions removed; internal self._turn calls are unchanged, and the runtime_checkable Soul protocol is deliberately untouched.
Two gates identified special tool classes by mechanisms that break invisibly on rename/move: - check_tool_call_allowed (permission.py) and _is_external_side_effect_tool (toolset.py) matched external adapters (MCPTool, WireExternalTool, PluginTool) by module/qualname strings. This is a permission gate that FAILS OPEN: moving or renaming any of these classes silently drops them out of external-tool permission gating with no test failure. A planned refactor moves MCPTool out of toolset.py, so the trap is defused first. - _tool_defers_execution_started duck-typed on the private _approval attribute to decide whether ToolExecutionStarted is deferred until after approval. Both gates now read explicit class-level flags instead: - external_side_effect_tool (ClassVar) on MCPTool, WireExternalTool, and PluginTool, documented as a security contract and pinned by tests so a future move/rename that loses the flag fails CI instead of failing open. - emits_tool_execution_started_after_approval on every approval-gated tool class (Shell, WriteFile, StrReplaceFile, TaskInput, TaskStop, Terminal, PluginTool), matching the existing RunAgentsTool precedent; the hasattr(_approval) fallback is removed. Routing is behavior-preserving: the same tool classes pass through the same gates before and after, and the Shell/network branches keep priority in check_tool_call_allowed.
Add the verified design-adoption blueprint (ranked, review-caveated refactor plan for cleaner agent/runtime layering) and update the task log with this branch's three landed tasks, their review outcomes, and the deferred follow-ups, including the known machine-local PTY shell-cancel test failure verified pre-existing on main.
Add a root-only OrchestrationInjectionProvider that nudges substantial normal-mode tasks toward the lightest effective work shape (direct tools, SetTodoList, foreground RunAgents, verification), throttled via a history-scanned reminder marker and suppressed under plan/auto/goal/subagent modes. Sharpen the matching system-prompt guidance, refresh a feature tip to promote /goal, and rephrase design-source comment attributions as generic agent-enhancement notes.
…ent-harness-enhancements
ACP sessions cannot present interactive questions (the session loop signals QuestionNotSupported), so advertising AskUserQuestion invites a wasted model step per question. replace_tools now hides the tool from the model-facing list while keeping it registered, so a stray call still resolves through the graceful textual fallback. Harmonize the task log after merging refactor/agent-contract-and-tool-metadata.
- FetchURL: await spill_to_disk() at the trafilatura and fetch-service sites so large pages spill off the event loop instead of falling back to the synchronous spill inside ok(). - MCPTool: declare emits_tool_execution_started_after_approval so the ToolExecutionStarted event defers until approval resolves, matching every other approval-gated tool; the old _approval duck-typing missed this class because it requests via runtime.approval. Pinned in test_toolset.py. - spinner_words: genericize remaining external credit wording. - tasks/todo.md: record the fixes; document why structural flag enforcement for future adapters is deferred (no shared adapter base until the toolset split lands).
Synthesized from a 14-cluster map+adversarial-verify workflow comparing the local reference agent harness against src/pythinker_code. Each item records current state, verifier evidence, an adoption sketch fitted to pythinker's design, effort/value, and target files; three refuted claims are pinned so they are not re-implemented. Includes execution discipline for the multi-writer branch (checkpoint = TDD + clean-code-guard + green gates, hot-file serialization).
A crash between persisting an assistant tool-call message and its tool results leaves context.jsonl with a dangling call or an orphaned result; after resume every provider request then fails with a pairing error. restore() and revert_to() now run a pure repair pass that synthesizes an explicit lost-result message for unpaired calls and drops orphaned or duplicate tool results, logging each repair. The file is never rewritten, so re-repair on each restore stays idempotent. Plan item: context-mgmt/history-invariant-repair (Tier 1).
Plan mode now teaches a two-kinds-of-unknowns rule (explore repo-discoverable facts yourself; surface preference/scope decisions early via AskUserQuestion with a recommended default), records unanswered defaults under an Assumptions section, gates ExitPlanMode on a decision-complete plan (no decisions left to the implementer), and adds a plan-file brevity rubric (3-5 short sections, subsystem-grouped bullets). Phrase pins lock the new clauses into all three reminder variants. Plan item: prompts-instructions/decision-complete-plan-mode (Tier 1).
Headless failure diagnostics (provider errors, max-steps + handoff, interrupt, unknown errors) printed plain text to stdout, corrupting the stream-json channel for machine parsers. All diagnostics now route to the pre-redirect stderr fd (falling back to sys.stderr), and stream-json mode additionally emits one structured error record — a Notification with category=run, type=error, the failure class, and the exit code — written via raw stdout so rich cannot soft-wrap the JSON line. Plan item: protocol-headless/channel-discipline (Tier 1).
Review-class subagents (review, code_reviewer, security_reviewer) received scope purely via the parent's prompt text and burned their first turns rediscovering branch, dirty files, and the merge base. The git-context prefix now also resolves the merge base against the first existing base ref (origin/main, main, master), names the exact review scope (git diff <sha>...HEAD), omits it when HEAD is the base, and is injected for reviewer-class agents alongside explore. Plan item: review-mode/deterministic-review-target-resolution (Tier 1, agent-dispatch slice).
Provider-emitted parallel tool calls all executed concurrently — two mutating tools (WriteFile + Shell from one assistant message) could race with no ordering guarantee. Tool dispatch now runs through a reader-writer gate: tools declaring supports_parallel (read-only builtins: ReadFile, ReadMediaFile, Glob, Grep, SmartSearch, Think, Recall, ListMcpResources, ReadMcpResource, SearchWeb, FetchURL) overlap freely, while everything else — including unflagged plugin/MCP tools, the safe default — executes exclusively in dispatch order. Writers drain in-flight readers and cannot be starved. Plan item: tools-registry-codemode/concurrency-policy (Tier 1).
A provider context-length 400 was telemetry-classified but treated as fatal: the step raised and the turn died, even though the proactive prune/compact thresholds run on heuristic counts that can undercount. Two recovery layers, both bounded: - Agent loop: on a context_overflow-classified step error, prune (best-effort), force a full compaction, and retry the step — once per turn; telemetry records recovered vs failed. - SimpleCompaction: the compaction request itself carries the whole to-compact slice and can overflow too; on a context-length rejection it drops the oldest half and retries, terminally falling back to the preserved tail plus an explicit dropped-context note. classify_api_error moves to soul/api_errors.py (re-exported from pythinkersoul) so compaction can classify without a circular import. Plan items: core-loop/reactive-overflow-recovery + context-mgmt/context-overflow-recovery (Tier 1).
A cloned repository's .pythinker/config.toml merged unconditionally, so its [[hooks]] shell commands auto-executed at session start — arbitrary code execution from cloning a repo. Project/local-scope hooks now load only after the user records trust: - New project_trust store (user-scope trusted_projects.json, atomic writes, fail-closed on corruption) keyed by the resolved repo root, so the repo itself can never grant its own trust. - _load_scoped strips hooks from untrusted project/local scopes with a warning naming /trust as the fix; broken TOML in an untrusted project degrades to an empty scope instead of blocking startup (trusted projects keep the loud error). - /trust on|off persists the per-project decision alongside the session flags and points at /reload for hook activation. - find_project_root promoted to public API (the /trust path needs it). Plan item: config-features/per-project-trust-gating (Tier 1). Out of scope (own plan item): sanitize-and-warn for scope-locked keys in untrusted scopes — they keep the existing loud ConfigError.
…tics Config models ignore extra keys, so a typo'd key silently vanished and changed behavior with no signal. After merge, the raw dict is now diffed against the model field tree (aliases and AliasChoices honored; recursion follows provable shapes only — nested models, dict-of-model maps, lists of models — so unmodellable values can never false- positive). Each finding warns with the dotted path and the scope file it came from via the existing provenance map; PYTHINKER_STRICT_CONFIG=1 escalates to ConfigError for CI use. Plan item: config-features/unknown-config-key-detection (Tier 1).
/model discarded the entire conversation by starting a fresh session. The switch now summarizes the outgoing session with the OUTGOING model — only plain text crosses the model boundary, so the incoming provider never sees foreign thinking blocks or tool-call schemas — and seeds the new session's context with it before Reload. Best-effort with a start-fresh fallback on empty history, summarization failure, or model_switch_carryover=false. SimpleCompaction gains summarize_all() (no preserved tail) atop the extracted overflow-halving summarizer. Plan item: core-loop/model-switch-context-continuity (Tier 1).
The first ls or git status of a session always interrupted the user with an approval dialog. soul/permission.py gains is_known_safe_command(): a positive allowlist, fail closed — the mutation guard's hidden-command/substitution/newline, write-redirection, and network/mutation rejections run first, then every ;/&&/||/| segment must start with an allowlisted read-only binary or read-only git subcommand (--output rejected). Wrappers (sudo/env/time) are never unwrapped, and absolute command paths must live in a system bin dir so a workspace-local fake git cannot ride its basename onto the allowlist. Shell consults it only in the root agent (subagent approval requests stay — they are part of the unattended-denial defense surface) and only after the deny gate, so elision can never override a deny. Elisions are tracked in telemetry; the started event fires at the elision point. Plan item: exec-safety/known-safe-command-auto-approval (Tier 1).
The safe-command elision accepted any KEY=VALUE prefix, so PATH=/tmp/evil ls would resolve ls from the attacker directory — defeating the system-bin pinning — and LD_PRELOAD/DYLD_*/GIT_PAGER prefixes could inject code into otherwise read-only commands. Only harmless locale/timezone assignments (LANG/LC_*/TZ) may now prefix an elidable command; every other assignment fails closed to the normal approval prompt. Flagged by automated security review.
The approval-protocol e2e tests drove Shell with 'echo ok', which the new known-safe elision now runs without a prompt — the round-trip these tests exist to pin never started. 'env echo ok' keeps stdout identical while the wrapper prefix disqualifies elision, so the approval exchange still exercises request/approve/reject. Fallout from 5dc87aa (caught by the full tests_e2e scope).
…tics A hung MCP connect left the server in 'connecting' forever and blocked every agent turn (the loop awaits MCP loading with no bound). Connect + inventory is now wrapped in asyncio.wait_for governed by a new mcp.client.startup_timeout_ms (default 30s), and every connect failure is classified into one short actionable line — timeout names the config knob, 401/unauthorized names the exact 'pythinker mcp auth' command, ENOENT names the missing binary — carried on MCPServerInfo and MCPServerSnapshot and rendered by /mcp instead of a bare 'failed'. Plan item: mcp/per-server-startup-timeout-diagnostics (Tier 1).
The serde and e2e snapshots pin wire-model dumps; the new optional MCPServerSnapshot.error field appears as null in them. Applied via --inline-snapshot=fix (deliberate, follows 05f8642).
…dTools) A server listing 30 tools floods the model tool list with all of them. mcp.json server entries now accept optional enabledTools (exclusive allowlist) and disabledTools (denylist, wins on conflict): filtered tools are skipped at connect time — never registered in the toolset or runtime.mcp_tools — and MCPTool re-checks membership at call time as defense in depth for tool maps shared with subagents and future live tool-list updates. No filter fields keeps today's permissive behavior. Plan item: mcp/per-server-tool-allow-deny-filtering (Tier 1).
Operational cwd/path-resolution sites (26 across tools, soul, permission, app, UI) now read runtime.work_dir — work_dir_override or the session's — instead of reaching through runtime.session.work_dir. copy_for_subagent accepts work_dir_override (re-rendering the child's PYTHINKER_WORK_DIR/_LS prompt args) and propagates it to grandchildren; the shared session keeps owning persistence paths. Behavior-preserving with no override set; full suite green (5381 local + e2e, the two TimeoutError wire tests verified pre-existing/machine-local on the stashed tree). Phase P1 of tasks/worktree-isolation-design.md; P2 wires the worktree lifecycle into the background runner.
…ts (P2) isolation='worktree' only recorded intent; parallel coder/implementer children shared one working tree and could clobber each other. The background runner now creates a detached git worktree of HEAD per write-profile child under <session>/worktrees/<agent_id>, points the child runtime at it through the P1 work_dir seam (prompt work-dir args re-rendered), and on completion appends the worktree path plus a diff summary to the final report so the orchestrator merges deliberately. Clean worktrees are removed; changed or failed ones are retained. Non-git roots fail before any model spend with an actionable error; read-profile children log and ignore the request; resume reuses the existing worktree. Local subprocesses are safe here — the manager enforces a local backend for agent tasks. Phase P2 of tasks/worktree-isolation-design.md; P3 (RunAgents batch) remains.
The RunAgents → Agent → create_agent_task → BackgroundAgentRunner chain already threads isolation per child, so P2 enforcement covers batch fan-outs; the parameter description now states the enforced semantics (per-child worktrees, diff-summary reports, deliberate merging) instead of 'records an intent'. Closes tasks/worktree-isolation-design.md.
Whitespace drift or smart-punctuation mismatch in StrReplaceFile's old string hard-failed with 'not found', burning a re-read + retry turn — the drift is invisible in numbered ReadFile output. After the exact match and CRLF fallback miss, a line-window seek now retries with graduated relaxations (trailing-whitespace -> indentation -> unicode-punctuation); the first firing tier replaces the ACTUAL matched file slice — never the needle text — adopting the slice's CRLF style and trailing newline, and the tool message names the relaxation. Ambiguity contract preserved: multiple fuzzy hits without replace_all error with the tier named. Deferred (low value): opt-in final-newline normalization for whole-file writes. Plan item: patch-file-tools/graduated-fuzzy-matching-ladder (Tier 1).
Enforcement was rich (profiles, safe mode, yolo/auto, session approvals, shlex-based command classification) but invisible prompt-side — the model discovered policy through denied tool calls. A new PermissionsInjectionProvider renders the enforced profile, posture flags, mutation/network allowances, session-approved actions, and the command-shaping rules the gate can actually classify. Fingerprinted on (profile, yolo, auto, safe_mode, approvals): re-emits exactly on posture changes, after compaction, and on auto toggles; root-only (subagent overlays already document their constraints). Approval gains read accessors is_safe_mode/session_approved_actions. History-shape test pins scoped to their subject; wire-session e2e snapshots refreshed. Plan item: prompts-instructions/dynamic-permissions-state (Tier 1).
- CRITICAL: GIT_CONTEXT_AGENT_TYPES used underscored reviewer names while registered type names are dashed (code-reviewer/security-reviewer), so reviewer agents silently missed the git-context injection; names fixed and a pin added asserting every gate name is a real profile key. - Foreground isolation requests now fail fast on Agent AND RunAgents instead of warning-and-proceeding unisolated (degraded behavior was presented as authoritative); warning pin updated to the new contract. - Unknown-config-key diagnostics now also run for explicit loads (--config-file / --config text) via single-source provenance. - Failure/timeout/cancel paths name the retained isolation worktree in the task output (retention is deliberate for resume, never silent). - Best-effort prune in overflow recovery logs its failure instead of contextlib.suppress. - supports_parallel flags annotated (: bool); test helpers cleaned (fail-fast _git asserts, unused _ListingClient params dropped).
…eview A 16-agent adversarial review (4 dimensions, every finding refuted-or- confirmed against live code) confirmed 11 findings; all fixed except one deliberate deferral (exclusive gate held across approval waits — needs the approval-split refactor; recorded in tasks/todo.md). - DATA LOSS (high): a child that committed its work left a clean worktree, so cleanup removed it and orphaned the commits. Creation now records a base-SHA sidecar (next to the worktree, never inside it); commits ahead of base count as changes and force retention, with unknown provenance failing closed to retention. - FALSE ISOLATION (high): foreground shell inherited the process cwd and relative file-tool paths resolved against it, so isolated children mutated the original repo. Host exec (protocol, local, ssh, ACP fallback) gained a cwd argument; foreground shell passes the runtime work dir, and write/replace/read resolve relative paths against it while preserving the relative-escape error contract. - REGRESSION (high): safe mode now disables the read-only-command prompt elision — users who disabled auto-approval keep every checkpoint. - REGRESSION (high): untrusted-project hook stripping now publishes a session notification (web/ACP visible), not just a shell log line. - MCP readOnlyHint annotations enable supports_parallel via property; worktree add/remove serializes per repo; CHANGELOG documents the same-step serialization and stderr-diagnostics behavior changes.
Contributor
|
Warning Review limit reached
More reviews will be available in 8 minutes and 55 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughFactual summary: broad runtime/tooling updates: exec cwd propagation; runtime work-dir override and subagent fork/isolation; fuzzy file-replace; MCP tool scoping/timeouts/concurrency; strict config diagnostics and per-project trust; compaction/overflow recovery; context repair; many related tests and UI tweaks. ChangesExecution / Host
Runtime / Subagents / Worktrees
Tools / Toolset / Concurrency / MCP
Permissions, Approval, and Safe-command elision
Compaction, Context, and Error Classification
Config and Project Trust
Soul public API and providers
UI, Print runner, and many tests/snapshots
Estimated code review effort Possibly related PRs
Suggested labels ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/pythinker_code/subagents/core.py (1)
134-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCollect git context from the effective child work dir, not the parent runtime.
work_dir_overrideis forwarded intobuild_builtin_instance(), but the prompt prefix still callscollect_git_context(runtime.builtin_args.PYTHINKER_WORK_DIR). In isolated-worktree runs that gives review agents branch/dirty/merge-base data for the parent checkout instead of the child checkout they will actually inspect.A local fix is to derive the git-context path from
spec.work_dir_overridefirst, then fall back toruntime.builtin_args.PYTHINKER_WORK_DIR.Also applies to: 160-165
🤖 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/subagents/core.py` around lines 134 - 139, The code forwards spec.work_dir_override into builder.build_builtin_instance but still calls collect_git_context(runtime.builtin_args.PYTHINKER_WORK_DIR), which causes git metadata to be gathered from the parent runtime instead of the actual child workdir; update the calls that compute git context (the collect_git_context invocations around the build_builtin_instance usage and the similar block at lines 160-165) to prefer spec.work_dir_override if present and only fall back to runtime.builtin_args.PYTHINKER_WORK_DIR when spec.work_dir_override is falsy, ensuring collect_git_context is invoked on the effective child work directory used by build_builtin_instance.tests/core/test_wire_message.py (1)
157-191: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a legacy-deserialization pin for
MCPServerSnapshot.error.This updates the serialized wire shape, but the test only blesses the new payload. Please add one deserialize assertion for an older
StatusUpdatepayload where each server omitserror, so session replay/backward-compatibility stays covered instead of being implicit.As per coding guidelines, wire/UI changes should maintain backward compatibility or add migration handling for persisted session data.
🤖 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/core/test_wire_message.py` around lines 157 - 191, Add a backward-compatibility test that deserializes an older StatusUpdate payload where MCPServerSnapshot objects omit the "error" field: construct a legacy payload dict matching the snapshot structure used in the test but with servers as [{"name":"context7","status":"connecting","tools":[]}] (no "error"), call the deserialization routine (use deserialize_wire_message to mirror serialize_wire_message) and assert the resulting StatusUpdate has an MCPServerSnapshot with servers[0].error is None (or equals the original msg), ensuring MCPServerSnapshot.error is handled when missing.Source: Coding guidelines
src/pythinker_code/background/agent_runner.py (1)
231-243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReport retained isolation worktrees on these early failure exits too.
If either branch here returns after isolation was set up,
_append_worktree_report()never runs and_note_retained_worktree(output)is skipped. A child can therefore leave retained edits behind, but the parent gets no path/disposition for them on summary failure or empty-output failure.Suggested fix
if failure is not None: self._finalize_safely(outcome="failed", reason=failure.message) output.error(_failure_recovery_message(reason=failure.message, agent_id=self._agent_id)) + self._note_retained_worktree(output) output.stage(f"failed: {failure.brief}") return @@ if final_response is None: self._finalize_safely( outcome="failed", reason="Agent completed but produced no output." ) + self._note_retained_worktree(output) output.stage("failed: empty output") returnAs per coding guidelines, observable output must reflect partial failure accurately and preserve source-to-output lineage for retained artifacts.
🤖 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/background/agent_runner.py` around lines 231 - 243, The early-return branches after checking `failure` and `final_response` must report retained isolation worktrees before exiting: call `_append_worktree_report()` and `_note_retained_worktree(output)` (or at least `_note_retained_worktree(output)` if `_append_worktree_report()` is conditional elsewhere) immediately prior to each `return` in the `if failure is not None:` and `if final_response is None:` blocks so retained edits are surfaced; preserve existing calls to `_finalize_safely(outcome=..., reason=...)` and `output.stage(...)` and ensure you pass the same `output` object and use `self._agent_id` context when invoking the reporting helpers.Source: Coding guidelines
src/pythinker_code/soul/toolset.py (1)
1068-1115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStage MCP inventory locally until connect succeeds.
_open_and_inventory()appends toserver_info.tools/resources/promptsbefore thewait_for(...)completes. If the timeout/error happens afterlist_tools()but before the later capability calls, the server is markedfailedwhilemcp_status_snapshot()still exposes stale tools for that failed server. Build those collections in locals and assign them only on success, or clear them in the failure path.Also applies to: 1130-1132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pythinker_code/soul/toolset.py` around lines 1068 - 1115, The inventory routine _open_and_inventory mutates server_info.tools/resources/prompts in-place before the wait_for completes, causing stale visible tools if the connect times out or errors; modify _open_and_inventory to build local lists (e.g., local_tools, local_resources, local_prompts) and only assign them to server_info.tools, server_info.resources, and server_info.prompts after all discovery calls (including _discover_optional_capability) succeed, and ensure the failure/except path does not leave partial state by either clearing server_info.* or never mutating it on error; update the call site that calls self._register_mcp_tools(server_name, server_info.tools) to occur after the successful assignment so registration only happens for fully-initialized inventories.
🤖 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 `@CHANGELOG.md`:
- Line 26: Update the typo in the CHANGELOG example: replace the incorrect
config key literal "defaut_yolo" with the correct "default_yolo" so the example
matches the schema and no longer fails the typo-check job; keep the surrounding
sentence and the mention of PYTHINKER_STRICT_CONFIG=1 intact.
In `@packages/pythinker-host/src/pythinker_host/ssh.py`:
- Around line 318-320: The current logic in create_process() builds the command
using effective_cwd = cwd or self._cwd and then cd'ing to it directly, which
treats relative cwd values as relative to the SSH login dir; change this so that
if cwd is provided and it is a relative path (does not start with "/"), first
resolve it against self._cwd (e.g., compute resolved_cwd =
os.path.normpath(os.path.join(self._cwd or "/", cwd))) and then use that
resolved_cwd as the directory you shlex.quote and prepend to the command; ensure
you still fall back to self._cwd when cwd is None and handle cases where
self._cwd may be None or empty.
In `@src/pythinker_code/config.py`:
- Around line 399-408: The current try/except around _read_toml(project_file)
and _read_toml(local_file) clears both project_dict and local_dict if either
parse fails; change this to parse each file independently: call
_read_toml(project_file) inside its own try/except that on ConfigError logs the
warning (including exc) and sets only project_dict = {}, and separately call
_read_toml(local_file) in its own try/except that sets only local_dict = {} on
failure; keep using the same logger message and ConfigError handling but ensure
each failure only empties the corresponding variable (project_dict or
local_dict) and not both.
In `@src/pythinker_code/project_trust.py`:
- Around line 73-81: set_project_trusted currently does an unlocked
read-modify-write using _read_trusted_roots and _write_trusted_roots, which can
lose concurrent updates; wrap the whole read-modify-write sequence in a
cross-process single-writer guard (e.g. acquire a file lock on the same backing
file like trusted_projects.json or use a portable lock library) before calling
_read_trusted_roots, perform the add/discard on normalized, call
_write_trusted_roots, then release the lock so no two processes can interleave;
ensure the lock is held across the entire sequence and that errors always
release the lock.
In `@src/pythinker_code/soul/agent.py`:
- Around line 406-412: The current work_dir_override branch only replaces
PYTHINKER_WORK_DIR and PYTHINKER_WORK_DIR_LS but leaves PYTHINKER_AGENTS_MD and
PYTHINKER_AGENTS_MD_FENCE pointing at the parent; update this by recomputing the
AGENTS payload for the override and including PYTHINKER_AGENTS_MD and
PYTHINKER_AGENTS_MD_FENCE in the replace call (or create and pass an
override-specific builtin_args instance) so that when load_agent() resolves
external markdown using runtime.work_dir it uses the overridden AGENTS.md
values; modify the code around builtin_args/replace (the work_dir_override
handling) to derive and inject the correct AGENTS values from work_dir_override
(and work_dir_ls) before creating the child runtime.
In `@src/pythinker_code/soul/context.py`:
- Around line 132-133: After calling repair_history_invariants(self._history)
ensure token accounting is recomputed from the repaired history: recalculate
self._token_count and self._pending_token_estimate using the repaired
self._history (e.g., call estimate_text_tokens on the appropriate slice of the
repaired history and recompute any token totals) so counts reflect post-repair
state; alternatively move the repair_history_invariants call to before any usage
replay/accounting logic so that functions like estimate_text_tokens and any
updates to self._token_count operate on the repaired _history (apply same change
for the other occurrence around lines 284-285).
In `@src/pythinker_code/soul/dynamic_injections/permissions_state.py`:
- Around line 41-47: The injected posture fingerprint currently only includes
the resolved PermissionProfile (variables profile, approval, approved) so it
misses changes to runtime.config.agent_execution_profile; update the fingerprint
construction used in build/inject_posture (where fingerprint is created) to also
include the current agent_execution_profile (or an explicit execution_profile
gate flag) so changes to runtime.config.agent_execution_profile cause a new
fingerprint and reinjection; ensure any equality checks or cache keys that use
fingerprint (the same symbol) are updated so reinjection occurs when execution
profile changes and stale/incorrect policy guidance cannot be presented.
In `@src/pythinker_code/soul/dynamic_injections/plan_mode.py`:
- Around line 157-160: The reminder string in plan_mode.py is truncated at the
end of the "Exit" rule; update the literal that contains the paragraph
mentioning "5. Exit — call ExitPlanMode..." so the final clause is explicit and
unambiguous (for example append "or ask via AskUserQuestion"). Locate the string
in the module where ExitPlanMode is referenced and extend the trailing "or ask"
fragment to a complete instruction (e.g., "or ask via AskUserQuestion") so
callers of ExitPlanMode have a clear required action.
In `@src/pythinker_code/soul/permission.py`:
- Around line 572-634: The current allowlist in
is_known_safe_command/_is_safe_readonly_segment only ensures no mutation/network
but misses path-bearing operands (e.g., cat /etc/shadow, ~/, git -C /other), so
update _is_safe_readonly_segment to reject or validate any argument that is
path-like: treat tokens that are absolute (start with "/"), home-expanded ("~"),
contain "/" or "../" or look like a Windows drive ("C:\") as path-bearing and
either (a) validate those paths are inside the allowed workspace root via a new
helper (e.g., validate_paths_within_workspace(paths, workspace_root)) called
from _is_safe_readonly_segment, or (b) if runtime validation is not available,
conservatively reject segments that contain any path-like args; also
special-case git handling (in _git_subcommand branch) to disallow -C/--git-dir
or path operands outside workspace. Modify _is_safe_readonly_segment to return
False when any unvalidated path-like token is present and add/reuse symbols
_SAFE_READONLY_COMMANDS, _SAFE_GIT_SUBCOMMANDS, and the new
validate_paths_within_workspace helper for clarity.
In `@src/pythinker_code/soul/toolset.py`:
- Around line 1299-1307: The supports_parallel property currently trusts remote
MCP metadata (_mcp_tool.annotations.readOnlyHint) to decide parallel safety;
change it to treat remote input as untrusted and only enable shared/parallel
execution when the tool is validated locally (e.g., via a trusted allowlist or
explicit local config). Replace reliance on getattr(annotations, "readOnlyHint")
with a local check (call or lookup such as a new or existing
trusted_tools/allowlist check) and keep the default behavior exclusive;
reference the supports_parallel property and _mcp_tool/annotations/readOnlyHint
symbols when updating the logic so remote annotations are ignored unless the
tool is explicitly trusted locally.
In `@src/pythinker_code/subagents/core.py`:
- Around line 16-27: The imports use TextPart/ThinkPart from
pythinker_code.wire.types which causes isinstance checks and new Message content
parts in filter_history_for_fork (and the related code around the 88-97 area) to
mismatch pythinker_core.message.Message parts; replace those imports so you
import Message, TextPart, and ThinkPart from pythinker_core.message (i.e.,
ensure the module uses the core message part types wherever Message is used,
including in filter_history_for_fork) so isinstance checks and new Message
construction use the correct classes.
In `@src/pythinker_code/subagents/runner.py`:
- Around line 291-298: The current except block around
Context(file_backend=...).restore() silently returns None, which converts an
explicit fork (req.fork_context) into a blank run; instead, when
parent_context.restore() fails you must not drop the fork silently: check the
incoming fork flag (e.g. req.fork_context or self._runtime.request.fork_context)
and on failure either (A) raise or propagate an error to abort the spawn so the
caller sees a hard failure, or (B) return an explicit degraded fork result
object (a clear marker in the subagent result/output path) that includes a
source/status like "context-read-failure" and the original exception; keep use
of Context, parent_context.restore(), and filter_history_for_fork but ensure the
error path surfaces the failure rather than returning None.
In `@src/pythinker_code/subagents/worktree.py`:
- Around line 67-69: When dest.exists() is true do not assume it is a valid
worktree: call a helper (e.g., is_valid_worktree(dest, repo_dir)) that verifies
dest belongs to repo_dir (for example by running git -C <repo_dir> worktree list
--porcelain and ensuring dest is listed, or by checking dest/.git points to the
repo worktrees dir), and if the check fails either raise WorktreeError or
remove/recreate the worktree before returning; update the early-return in the
function that currently checks dest.exists() and ensure callers such as
worktree_change_summary() will only run against verified worktrees.
In `@src/pythinker_code/tools/file/read_media.py`:
- Around line 50-52: The ReadMediaFile tool is marked supports_parallel=True
which can cause high memory use; change ReadMediaFile.supports_parallel to False
(or remove that attribute) so calls remain serialized, and if you need
concurrency later add a controlled concurrency cap or implement a
streaming/read-chunk path in the ReadMediaFile implementation to avoid loading
the full payload (refer to the ReadMediaFile class and its supports_parallel
attribute).
In `@src/pythinker_code/ui/shell/slash.py`:
- Around line 405-415: The seed message created in slash.py is incorrectly
persisted with role="user"; change it to a synthetic system (or assistant) turn
so the carry-over summary isn't treated as user input: update the Message
instantiation assigned to seed to use role="system" (or "assistant") and keep
the same content, then append it via
Context(file_backend=new_session.context_file).append_message(seed) as before so
downstream logic and UI don't misinterpret the summary as a user turn.
In `@tests/core/test_config_unknown_keys.py`:
- Line 3: Tests use misspelled keys like "defaut_yolo" and "tpyo" which trip the
typo-check pipeline; replace those typo'd keys with intentionally-unknown but
correctly spelled tokens (e.g., "default_yolo_unknown" and "typo_unknown")
wherever they appear (including the other occurrences noted), and update the
assertions to expect these new unknown-key names instead of the misspellings so
the tests keep the unknown-key semantics without failing the CI typo checker.
In `@tests/core/test_pythinkersoul_ralph_loop.py`:
- Around line 219-232: Replace the hard-coded full permissions reminder TextPart
used in the Message objects (the long "<system-reminder>...Permissions state:
profile 'implement'..." block) with a lightweight assertion or shared helper
that only verifies a reminder was injected in the right position for the
Ralph-loop tests (functions/classes exercising loop replay/stop behavior),
rather than pinning exact wording; update the tests around the Message/TextPart
construction (where Message(...) and TextPart(...) are created) to either call
the existing reminder-builder helper or assert that a TextPart containing a
reminder marker (e.g., startswith "<system-reminder>" or contains "Permissions
state") is present, and leave exact-text snapshot checks to the dedicated
injection tests referenced in the comment.
In `@tests/core/test_pythinkersoul_steer.py`:
- Around line 139-143: The current filter that builds persisted from
soul.context.history is too broad because it drops any user message containing
"Permissions state:"; change the predicate to only ignore messages that are
system-injected permission reminders by replacing the substring check with a
call to is_system_reminder_message(m) combined with the permissions prefix check
(e.g., if is_system_reminder_message(m) and "Permissions state:" in
m.extract_text(" ")). Update the list comprehension that produces persisted
(iterating soul.context.history and using m.role/m.extract_text) so it only
filters out messages where is_system_reminder_message(m) is true and the text
contains the permissions prefix.
In `@tests/core/test_safe_command_elision.py`:
- Around line 110-129: The inner "import pytest" inside
TestSafeModeKeepsPrompts.test_safe_mode_blocks_elision is redundant because
pytest is already imported at module scope; remove that inner import statement
so the test uses the module-level pytest import (locate the line with the nested
import in the test_safe_mode_blocks_elision method and delete it).
In `@tests/core/test_toolset_concurrency.py`:
- Around line 40-45: The helper function _toolset currently sets
HookEngine(cwd="/tmp") which hardcodes a POSIX-only path; update _toolset to
accept or derive a portable temporary directory (e.g., accept a cwd parameter,
use tempfile.gettempdir(), or use a test-provided tmp_path) and pass that into
PythinkerToolset._hook_engine = HookEngine([], cwd=portable_dir) so tests run on
Windows and CI; modify the _toolset signature or call sites accordingly and
ensure references to PythinkerToolset and HookEngine._hook_engine use the
portable_dir value.
In `@tests/core/test_work_dir_seam.py`:
- Around line 17-21: The test test_subagent_clone_inherits_by_default should not
assert object identity for builtin_args; update the assertion after calling
runtime.copy_for_subagent(agent_id="a1", subagent_type="coder") to verify
observable behavior instead: keep the work_dir equality check (child.work_dir ==
runtime.session.work_dir) but replace assert child.builtin_args is
runtime.builtin_args with assertions that the PYTHINKER_* values in
child.builtin_args match those in runtime.builtin_args (e.g., check specific
keys or that all keys starting with "PYTHINKER_" have equal values between
child.builtin_args and runtime.builtin_args) so the test accepts both shared and
copy-on-write implementations of builtin_args.
In `@tests/tools/test_memory_tool.py`:
- Around line 27-35: The test helper _runtime currently sets
work_dir=session.work_dir so tests won't catch a regression where Memory uses
runtime.session.work_dir; update tests/tools/test_memory_tool.py to add a case
where runtime.work_dir and runtime.session.work_dir differ (e.g., tmp_path /
"repo" vs tmp_path / "different_repo") by calling _runtime with a modified
session or constructing a runtime where session.work_dir != work_dir, then
instantiate Memory (or the store creation path used in tests) with that runtime
and assert the store's underlying path equals runtime.work_dir (not
runtime.session.work_dir) to ensure the store uses the explicit runtime.work_dir
seam.
---
Outside diff comments:
In `@src/pythinker_code/background/agent_runner.py`:
- Around line 231-243: The early-return branches after checking `failure` and
`final_response` must report retained isolation worktrees before exiting: call
`_append_worktree_report()` and `_note_retained_worktree(output)` (or at least
`_note_retained_worktree(output)` if `_append_worktree_report()` is conditional
elsewhere) immediately prior to each `return` in the `if failure is not None:`
and `if final_response is None:` blocks so retained edits are surfaced; preserve
existing calls to `_finalize_safely(outcome=..., reason=...)` and
`output.stage(...)` and ensure you pass the same `output` object and use
`self._agent_id` context when invoking the reporting helpers.
In `@src/pythinker_code/soul/toolset.py`:
- Around line 1068-1115: The inventory routine _open_and_inventory mutates
server_info.tools/resources/prompts in-place before the wait_for completes,
causing stale visible tools if the connect times out or errors; modify
_open_and_inventory to build local lists (e.g., local_tools, local_resources,
local_prompts) and only assign them to server_info.tools, server_info.resources,
and server_info.prompts after all discovery calls (including
_discover_optional_capability) succeed, and ensure the failure/except path does
not leave partial state by either clearing server_info.* or never mutating it on
error; update the call site that calls self._register_mcp_tools(server_name,
server_info.tools) to occur after the successful assignment so registration only
happens for fully-initialized inventories.
In `@src/pythinker_code/subagents/core.py`:
- Around line 134-139: The code forwards spec.work_dir_override into
builder.build_builtin_instance but still calls
collect_git_context(runtime.builtin_args.PYTHINKER_WORK_DIR), which causes git
metadata to be gathered from the parent runtime instead of the actual child
workdir; update the calls that compute git context (the collect_git_context
invocations around the build_builtin_instance usage and the similar block at
lines 160-165) to prefer spec.work_dir_override if present and only fall back to
runtime.builtin_args.PYTHINKER_WORK_DIR when spec.work_dir_override is falsy,
ensuring collect_git_context is invoked on the effective child work directory
used by build_builtin_instance.
In `@tests/core/test_wire_message.py`:
- Around line 157-191: Add a backward-compatibility test that deserializes an
older StatusUpdate payload where MCPServerSnapshot objects omit the "error"
field: construct a legacy payload dict matching the snapshot structure used in
the test but with servers as
[{"name":"context7","status":"connecting","tools":[]}] (no "error"), call the
deserialization routine (use deserialize_wire_message to mirror
serialize_wire_message) and assert the resulting StatusUpdate has an
MCPServerSnapshot with servers[0].error is None (or equals the original msg),
ensuring MCPServerSnapshot.error is handled when missing.
🪄 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: 14206c22-8dad-4a70-8b47-f49431e8ad6c
⛔ Files ignored due to path filters (5)
tasks/agent-enhancement-remaining-plan.mdis excluded by!tasks/**tasks/agent-harness-adoption-plan.mdis excluded by!tasks/**tasks/design-adoption-blueprint.mdis excluded by!tasks/**tasks/todo.mdis excluded by!tasks/**tasks/worktree-isolation-design.mdis excluded by!tasks/**
📒 Files selected for processing (110)
CHANGELOG.mdREADME.mdpackages/pythinker-host/src/pythinker_host/__init__.pypackages/pythinker-host/src/pythinker_host/local.pypackages/pythinker-host/src/pythinker_host/ssh.pysrc/pythinker_code/acp/host.pysrc/pythinker_code/acp/tools.pysrc/pythinker_code/agents/default/system.mdsrc/pythinker_code/app.pysrc/pythinker_code/background/agent_runner.pysrc/pythinker_code/background/manager.pysrc/pythinker_code/config.pysrc/pythinker_code/llm.pysrc/pythinker_code/plugin/tool.pysrc/pythinker_code/project_trust.pysrc/pythinker_code/soul/agent.pysrc/pythinker_code/soul/api_errors.pysrc/pythinker_code/soul/approval.pysrc/pythinker_code/soul/compaction.pysrc/pythinker_code/soul/context.pysrc/pythinker_code/soul/dynamic_injections/model_defense.pysrc/pythinker_code/soul/dynamic_injections/orchestration.pysrc/pythinker_code/soul/dynamic_injections/permissions_state.pysrc/pythinker_code/soul/dynamic_injections/plan_mode.pysrc/pythinker_code/soul/flow_runner.pysrc/pythinker_code/soul/permission.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/soul/slash.pysrc/pythinker_code/soul/toolset.pysrc/pythinker_code/subagents/builder.pysrc/pythinker_code/subagents/core.pysrc/pythinker_code/subagents/git_context.pysrc/pythinker_code/subagents/runner.pysrc/pythinker_code/subagents/worktree.pysrc/pythinker_code/thinking.pysrc/pythinker_code/tools/agent/__init__.pysrc/pythinker_code/tools/background/__init__.pysrc/pythinker_code/tools/file/glob.pysrc/pythinker_code/tools/file/grep_local.pysrc/pythinker_code/tools/file/read.pysrc/pythinker_code/tools/file/read_media.pysrc/pythinker_code/tools/file/replace.pysrc/pythinker_code/tools/file/write.pysrc/pythinker_code/tools/mcp_resource/__init__.pysrc/pythinker_code/tools/memory/__init__.pysrc/pythinker_code/tools/recall/__init__.pysrc/pythinker_code/tools/shell/__init__.pysrc/pythinker_code/tools/think/__init__.pysrc/pythinker_code/tools/todo/__init__.pysrc/pythinker_code/tools/web/fetch.pysrc/pythinker_code/tools/web/search.pysrc/pythinker_code/ui/print/__init__.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/components/bash_execution.pysrc/pythinker_code/ui/shell/components/markdown.pysrc/pythinker_code/ui/shell/components/render_utils.pysrc/pythinker_code/ui/shell/mcp_status.pysrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/spinner_words.pysrc/pythinker_code/ui/shell/stats_pricing.pysrc/pythinker_code/ui/shell/tips.pysrc/pythinker_code/wire/types.pytests/acp/test_acp_tool_visibility.pytests/core/test_auto_injection.pytests/core/test_compaction_overflow.pytests/core/test_config.pytests/core/test_config_unknown_keys.pytests/core/test_context_history_repair.pytests/core/test_default_agent.pytests/core/test_goal_auto_continuation.pytests/core/test_model_switch_carryover.pytests/core/test_orchestration_injection_provider.pytests/core/test_overflow_recovery.pytests/core/test_permission_profiles.pytests/core/test_permissions_injection_provider.pytests/core/test_plan_mode.pytests/core/test_plan_mode_injection_provider.pytests/core/test_project_trust.pytests/core/test_public_turn.pytests/core/test_pythinkersoul_ralph_loop.pytests/core/test_pythinkersoul_steer.pytests/core/test_safe_command_elision.pytests/core/test_skip_auto_prompt_injection.pytests/core/test_toolset.pytests/core/test_toolset_concurrency.pytests/core/test_wire_message.pytests/core/test_work_dir_seam.pytests/subagents/test_context_fork.pytests/subagents/test_git_context_gate.pytests/subagents/test_worktree_isolation.pytests/test_git_context.pytests/tools/test_agent_tool.pytests/tools/test_fetch_url.pytests/tools/test_mcp_startup_timeout.pytests/tools/test_mcp_tool_filter.pytests/tools/test_memory_tool.pytests/tools/test_replace_fuzzy.pytests/tools/test_todo.pytests/tools/test_tool_schemas.pytests/tools/test_untrusted_wrapping.pytests/tools/test_write_file.pytests/ui_and_conv/test_memory_slash.pytests/ui_and_conv/test_print_channel_discipline.pytests/ui_and_conv/test_prompt_tips.pytests/ui_and_conv/test_shell_slash_commands.pytests/ui_and_conv/test_tui_components.pytests_e2e/test_wire_approvals_tools.pytests_e2e/test_wire_sessions.pytests_e2e/test_wire_skills_mcp.py
Address all bot review feedback on the agent-harness branch: Security / CI gates: - project_trust: store SHA-256 digests instead of clear-text paths (CodeQL clear-text-storage) and serialize read-modify-write behind a cross-process file lock; read legacy clear-text stores for compat. - typos: rename intentional config-key fixtures to validly-spelled unknown keys; fix `unparsable`/`default_yolo_typo` prose in planning doc. - api_errors: drop redundant `400 <= status < 500` guard (always true after the >=500 early return). Correctness: - ssh: resolve relative cwd against the host's tracked cwd, not the SSH login dir. - config: read untrusted project/local scopes independently so one bad file no longer discards the other. - context: re-repair the post-usage slice so token accounting reflects the repaired history; keep tool messages with no call id. - soul/agent + subagents/builder: recompute AGENTS.md payload for a child worktree override instead of inheriting the parent's. - subagents/core: import TextPart/ThinkPart from pythinker_core.message; collect git context from the effective child work dir. - subagents/runner: fail an explicit context fork loudly instead of silently degrading to a blank child. - subagents/worktree: validate a pre-existing dest is a registered worktree before reusing it. - background/agent_runner: report retained isolation worktrees on the early failure/empty-output exits too. - slash: persist the carry-over summary as a system turn, not a user turn. Safety hardening: - permission: refuse prompt elision for read-only commands with path-bearing operands (cat /etc/shadow, git -C /other, ../secret). - toolset: keep MCP tools exclusive in the same-step gate (ignore untrusted remote readOnlyHint); stage MCP inventory locally until connect succeeds. - read_media: keep ReadMediaFile serialized (large in-memory payloads). - permissions_state: include agent_execution_profile in the injection fingerprint so profile switches reinject. plan_mode: complete the truncated exit-rule sentence and wrap multi-line reminder literals in parentheses (fixes the implicit-concat warning without splitting sentences across rendered lines). Tests: cover the trust-store hashing/legacy path, the work_dir and runtime.work_dir seams by behavior not identity, the MCP exclusive default, legacy StatusUpdate deserialization, and the new unsafe path-operand commands; stop pinning full reminder text in loop tests.
The verb spinner (shimmering "Working…/Thinking…") was gated only on `_active_turn_depth > 0`, i.e. the whole turn. When the agent started a long-running foreground command — a dev server via npm/docker, a watch task — the agent coroutine just awaits the subprocess, but the shimmer kept animating for the full turn, falsely signalling active agent cognition. The tool card already shows an animated running marker plus streaming output, so the shimmer was redundant and misleading. Suppress the working indicator while any foreground tool is mid-execution (execution started, no result yet, not a detached background agent) on both render surfaces — the non-interactive Rich Live path and the interactive pinned status tail. The shimmer now means "the agent is thinking" and reappears the moment the command returns. Platform-agnostic: the root cause was turn-level gating, not Windows-specific. Adds `_ToolCallBlock.is_executing` and `_LiveView._foreground_tool_executing()`, and a test pinning that the pinned tail is empty mid-execution and returns once the tool finishes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/pythinker_code/ui/shell/slash.py (1)
1791-1803:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply trust revocation to the current hook runtime too.
/trust offpersists the new trust state, but it never unloads project hooks already present insoul.hook_engine. A repo hook loaded at startup can still fire later in this session, so the revocation only takes effect after a reload. Force a reload here or rebuild the hook engine before returning. As per coding guidelines, project-scope auto-executed config must load only after the user trusts the project root.Minimal safe direction
if mode in {"off", "no", "untrust", "safe"}: state.trusted = False state.safe_mode = True soul.runtime.approval.set_safe_mode(True) soul.runtime.approval.set_yolo(False) soul.runtime.approval.set_auto(False) soul.runtime.session.state.approval.auto_approve_actions.clear() soul.runtime.session.save_state() _persist_project_trust(soul, trusted=False) console.print( f"[{_t_trust.warning}]Workspace untrusted. Safe mode enabled; " "auto-approval is disabled.[/]" ) - return + raise Reload(session_id=soul.runtime.session.id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pythinker_code/ui/shell/slash.py` around lines 1791 - 1803, Persisting trust state with _persist_project_trust(...) is fine but you must also remove/unload project hooks from the live runtime so they cannot fire after `/trust off`; after calling _persist_project_trust(soul, trusted=False) and before the console.print return, rebuild or reload the hook engine on the current runtime (e.g., call a method on soul.hook_engine to clear project hooks and rebuild the engine or explicit unload like soul.hook_engine.reload() / soul.hook_engine.rebuild() / soul.hook_engine.clear_project_hooks()) so project-scoped hooks are removed immediately; ensure the call happens in the same branch where state.trusted is set False so hooks are not left active for the session.Source: Coding guidelines
src/pythinker_code/config.py (1)
359-365:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTranslate read failures into
ConfigErrorin_read_toml().Lines 399-414 only recover from
ConfigError. A permission error or otherOSErrorfrompath.read_text()still escapes here and bricks startup in an untrusted repo, even though that branch is explicitly trying to ignore unreadable project/local scopes. WrapOSErrorhere too so trusted loads still fail with an actionable config error and untrusted loads can degrade safely.Suggested fix
def _read_toml(path: Path) -> dict[str, Any]: if not path.exists(): return {} try: return dict(tomlkit.loads(path.read_text(encoding="utf-8"))) + except OSError as exc: + raise ConfigError(f"Failed to read {path}: {exc}") from exc except TOMLKitError as exc: raise ConfigError(f"Invalid TOML in {path}: {exc}") from exc🤖 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/config.py` around lines 359 - 365, The _read_toml function currently only converts TOMLKitError to ConfigError but lets OSError (e.g., permission errors from path.read_text()) propagate; update _read_toml to also catch OSError and re-raise it as a ConfigError (raising ConfigError(f"Unable to read {path}: {exc}") from exc) so that both TOML parsing failures and filesystem read failures are translated into ConfigError for callers that handle trusted vs untrusted loads; locate and modify the _read_toml function and ensure the except block handles (TOMLKitError, OSError) and raises ConfigError from the original exception.src/pythinker_code/soul/api_errors.py (1)
44-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict this branch back to 4xx responses.
status < 500now classifies non-4xxAPIStatusErrors too. With the current fallbackstatus=0, a statusless error is reported as4xx_client/context_overflowinstead of genericapi, which can drive the wrong overflow-recovery path in the shared soul/compaction classifier.Suggested fix
- if status < 500: + if 400 <= status < 500: msg_lower = str(e).lower() if any(marker in msg_lower for marker in _CONTEXT_OVERFLOW_MARKERS): return "context_overflow", status_code return "4xx_client", status_code return "api", status_code🤖 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/api_errors.py` around lines 44 - 49, The current branch treats any status < 500 as a 4xx client error which misclassifies statusless or non-4xx APIStatusError; change the conditional so it only treats explicit 4xx codes (400 <= status < 500) as the client/context_overflow branch, leaving other values (including 0 or None) to fall through to the "api" category; update the logic around the `status` check in the function containing the shown snippet (the block referencing `status`, `_CONTEXT_OVERFLOW_MARKERS`, and returning `"context_overflow"` / `"4xx_client"`) to use a 400-499 range check.src/pythinker_code/soul/dynamic_injections/plan_mode.py (1)
157-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't hard-require
AskUserQuestionwhen ACP hides it.
replace_tools()removesAskUserQuestionfrom the model-visible ACP tool list, but these reminders still tell the model to use it and even require turns to end with it orExitPlanMode. On ACP that creates an impossible workflow and pushes the model toward a guaranteed wasted step. Make the reminder conditional on tool availability/host, or point ACP sessions at the supported text fallback instead.Also applies to: 228-235, 267-273
🤖 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/dynamic_injections/plan_mode.py` around lines 157 - 205, The guidance text in plan_mode.py unconditionally requires AskUserQuestion (and forces turns to end with it or ExitPlanMode) even when replace_tools() removes AskUserQuestion from the ACP tool list; update the code that builds the guidance (the lines.extend block in the Plan/plan-mode generator) to check runtime tool availability (e.g., detect presence of AskUserQuestion in the active tools array or a host/ACP flag exposed by replace_tools()) and: if AskUserQuestion is unavailable, remove or replace the sentences that mandate AskUserQuestion with a fallback message instructing to use the supported text-based fallback or only ExitPlanMode; ensure references to AskUserQuestion, ExitPlanMode, and replace_tools() are adjusted so the reminder is conditional rather than hard-required.tests/core/test_safe_command_elision.py (1)
35-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPin the new Windows and git-dir deny branches.
The production change added explicit deny logic for Windows drive paths and
git --git-dir/--work-tree, but this corpus only pins-Cand POSIX-style paths. Add a fewUNSAFEcases for those branches so a regression cannot silently reopen approval elision there.💡 Suggested test additions
UNSAFE = [ "rm -rf /tmp/x", "git push", "git commit -m x", "git branch new-branch", "git status --output=/tmp/f", "git -C /other/repo status", + "git --git-dir=/other/repo/.git status", + "git --work-tree=/other/repo status", "cat /etc/shadow", "cat ~/.ssh/id_rsa", "cat ../secret.txt", + r"cat C:\Windows\System32\config\SAM", "git log --output=/tmp/f --oneline",As per coding guidelines, flag security-sensitive changes whose tests do not cover edge cases and failure paths (C14).
Source: Coding guidelines
src/pythinker_code/background/agent_runner.py (1)
288-291:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuote the recovery command's worktree path.
The retained-worktree hint embeds the raw path into a shell command. If the session or repo path contains spaces, the cleanup command breaks on the exact failure path where the user needs it. Wrap the operand with
shlex.quote(...)before writing it out.Suggested fix
+import shlex + output.stage( f"worktree_retained: {self._worktree_path} (resume reuses it; remove with " - f"`git worktree remove {self._worktree_path}`)" + f"`git worktree remove {shlex.quote(str(self._worktree_path))}`)" )🤖 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/background/agent_runner.py` around lines 288 - 291, The hint message in output.stage currently embeds self._worktree_path raw into a shell command; wrap the path with shlex.quote(...) before formatting so paths with spaces or special chars are safely quoted (update the f-string passed to output.stage to use shlex.quote(self._worktree_path) and add an import for shlex at the top of the module); ensure the quoted value is used in both the message and the inline `git worktree remove` example.src/pythinker_code/subagents/runner.py (1)
302-323:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't persist a fresh instance before fork-history loading can fail.
For non-resume runs,
_prepare_instance()creates the subagent record before_load_fork_history()runs. If parent-context restore fails, the exception escapes with a brand-new instance still stored asidle. That instance is then resumable even though the requested spawn never launched, and a later resume skips fork seeding entirely.Load the fork history before creating a new instance, or mark the created record failed before re-raising. As per coding guidelines, required-step failures must not leave behind success-looking state.
Suggested fix
- prepared = await self._prepare_instance(req) + fork_history = None + if req.fork_context and not req.resume: + fork_history = await self._load_fork_history() + + prepared = await self._prepare_instance(req) agent_id = prepared.record.agent_id actual_type = prepared.actual_type resumed = prepared.resumed @@ - fork_history = None - if req.fork_context and not resumed: - fork_history = await self._load_fork_history() spec = SubagentRunSpec( agent_id=agent_id, type_def=type_def, launch_spec=launch_spec, prompt=req.prompt,🤖 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/subagents/runner.py` around lines 302 - 323, The run() flow persists a fresh subagent record in _prepare_instance() before _load_fork_history() can fail, leaving a resumable idle instance on error; modify run() so that when req.fork_context is provided and resumed is False you call _load_fork_history() before creating/persisting the new instance (i.e., before calling _prepare_instance()) OR, if you prefer minimal change, wrap the fork-history load in a try/except immediately after _prepare_instance() and on exception mark the created record as failed (use the same record/state update API used elsewhere — reference prepared.record and whatever store methods update state) then re-raise; ensure fork history is available before marking the instance as idle so failed restores do not leave behind resumable instances.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pythinker_code/soul/agent.py`:
- Around line 409-415: The code currently sets agents_md = work_dir_agents_md or
"" which treats None as an explicit empty override and clears
PYTHINKER_AGENTS_MD for children; change this to preserve the parent payload
when work_dir_agents_md is None by only replacing PYTHINKER_AGENTS_MD when an
explicit value (including empty string) is provided. Locate the agents_md
assignment and the replace(...) call that sets PYTHINKER_AGENTS_MD and
PYTHINKER_AGENTS_MD_FENCE (and the helper _agents_md_fence) and update the logic
so that if work_dir_agents_md is None you leave the existing parent value in
builtin_args unchanged, otherwise use the provided value and compute the fence
via _agents_md_fence.
In `@src/pythinker_code/soul/context.py`:
- Around line 58-61: In the repair routine in context.py that iterates messages
(the branch checking if message.role == "tool"), stop preserving tool messages
where message.tool_call_id is None; instead only keep tool messages whose
message.tool_call_id is in open_call_ids (i.e., remove the clause that appends
messages with tool_call_id is None) so malformed/unpaired tool messages are
dropped and the pairing invariant is enforced when repairing history.
In `@src/pythinker_code/subagents/worktree.py`:
- Around line 55-58: When a git call fails in _is_registered_worktree you must
not collapse failures into False; instead, if _git(...) returns a non-zero exit,
raise WorktreeError carrying the original git stderr (and useful context like
the subcommand/args or stdout) so the real repo/git failure is propagated to the
caller. Replace the current "return False" on non-zero exit with a WorktreeError
construction that includes stderr and the invoked args from the _git call, and
apply the same pattern to the other places in this module that call _git for
worktree operations (use the same WorktreeError and include stderr/context there
too).
In `@tests/tools/test_memory_tool.py`:
- Line 27: Annotate the helper function _runtime with an explicit return type to
satisfy Ruff ANN202: add a return type annotation on def _runtime(tmp_path,
role="root", work_dir=None) (e.g., -> Any or the specific Runtime/Fixture type
used in tests) and import typing.Any if using Any; prefer the concrete type if
there is an existing Runtime/TestFixture type in the codebase and update the
signature accordingly.
---
Outside diff comments:
In `@src/pythinker_code/background/agent_runner.py`:
- Around line 288-291: The hint message in output.stage currently embeds
self._worktree_path raw into a shell command; wrap the path with
shlex.quote(...) before formatting so paths with spaces or special chars are
safely quoted (update the f-string passed to output.stage to use
shlex.quote(self._worktree_path) and add an import for shlex at the top of the
module); ensure the quoted value is used in both the message and the inline `git
worktree remove` example.
In `@src/pythinker_code/config.py`:
- Around line 359-365: The _read_toml function currently only converts
TOMLKitError to ConfigError but lets OSError (e.g., permission errors from
path.read_text()) propagate; update _read_toml to also catch OSError and
re-raise it as a ConfigError (raising ConfigError(f"Unable to read {path}:
{exc}") from exc) so that both TOML parsing failures and filesystem read
failures are translated into ConfigError for callers that handle trusted vs
untrusted loads; locate and modify the _read_toml function and ensure the except
block handles (TOMLKitError, OSError) and raises ConfigError from the original
exception.
In `@src/pythinker_code/soul/api_errors.py`:
- Around line 44-49: The current branch treats any status < 500 as a 4xx client
error which misclassifies statusless or non-4xx APIStatusError; change the
conditional so it only treats explicit 4xx codes (400 <= status < 500) as the
client/context_overflow branch, leaving other values (including 0 or None) to
fall through to the "api" category; update the logic around the `status` check
in the function containing the shown snippet (the block referencing `status`,
`_CONTEXT_OVERFLOW_MARKERS`, and returning `"context_overflow"` /
`"4xx_client"`) to use a 400-499 range check.
In `@src/pythinker_code/soul/dynamic_injections/plan_mode.py`:
- Around line 157-205: The guidance text in plan_mode.py unconditionally
requires AskUserQuestion (and forces turns to end with it or ExitPlanMode) even
when replace_tools() removes AskUserQuestion from the ACP tool list; update the
code that builds the guidance (the lines.extend block in the Plan/plan-mode
generator) to check runtime tool availability (e.g., detect presence of
AskUserQuestion in the active tools array or a host/ACP flag exposed by
replace_tools()) and: if AskUserQuestion is unavailable, remove or replace the
sentences that mandate AskUserQuestion with a fallback message instructing to
use the supported text-based fallback or only ExitPlanMode; ensure references to
AskUserQuestion, ExitPlanMode, and replace_tools() are adjusted so the reminder
is conditional rather than hard-required.
In `@src/pythinker_code/subagents/runner.py`:
- Around line 302-323: The run() flow persists a fresh subagent record in
_prepare_instance() before _load_fork_history() can fail, leaving a resumable
idle instance on error; modify run() so that when req.fork_context is provided
and resumed is False you call _load_fork_history() before creating/persisting
the new instance (i.e., before calling _prepare_instance()) OR, if you prefer
minimal change, wrap the fork-history load in a try/except immediately after
_prepare_instance() and on exception mark the created record as failed (use the
same record/state update API used elsewhere — reference prepared.record and
whatever store methods update state) then re-raise; ensure fork history is
available before marking the instance as idle so failed restores do not leave
behind resumable instances.
In `@src/pythinker_code/ui/shell/slash.py`:
- Around line 1791-1803: Persisting trust state with _persist_project_trust(...)
is fine but you must also remove/unload project hooks from the live runtime so
they cannot fire after `/trust off`; after calling _persist_project_trust(soul,
trusted=False) and before the console.print return, rebuild or reload the hook
engine on the current runtime (e.g., call a method on soul.hook_engine to clear
project hooks and rebuild the engine or explicit unload like
soul.hook_engine.reload() / soul.hook_engine.rebuild() /
soul.hook_engine.clear_project_hooks()) so project-scoped hooks are removed
immediately; ensure the call happens in the same branch where state.trusted is
set False so hooks are not left active for the 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: 7c759446-d917-46c7-b5ef-bdd746e4801d
⛔ Files ignored due to path filters (1)
tasks/agent-harness-adoption-plan.mdis excluded by!tasks/**
📒 Files selected for processing (33)
CHANGELOG.mdpackages/pythinker-host/src/pythinker_host/ssh.pysrc/pythinker_code/background/agent_runner.pysrc/pythinker_code/config.pysrc/pythinker_code/project_trust.pysrc/pythinker_code/soul/agent.pysrc/pythinker_code/soul/api_errors.pysrc/pythinker_code/soul/context.pysrc/pythinker_code/soul/dynamic_injections/permissions_state.pysrc/pythinker_code/soul/dynamic_injections/plan_mode.pysrc/pythinker_code/soul/permission.pysrc/pythinker_code/soul/toolset.pysrc/pythinker_code/subagents/builder.pysrc/pythinker_code/subagents/core.pysrc/pythinker_code/subagents/runner.pysrc/pythinker_code/subagents/worktree.pysrc/pythinker_code/tools/file/read_media.pysrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/visualize/_blocks.pysrc/pythinker_code/ui/shell/visualize/_interactive.pysrc/pythinker_code/ui/shell/visualize/_live_view.pytests/acp/test_acp_tool_visibility.pytests/core/test_config_unknown_keys.pytests/core/test_project_trust.pytests/core/test_pythinkersoul_ralph_loop.pytests/core/test_pythinkersoul_steer.pytests/core/test_safe_command_elision.pytests/core/test_toolset_concurrency.pytests/core/test_wire_message.pytests/core/test_work_dir_seam.pytests/tools/test_mcp_tool_filter.pytests/tools/test_memory_tool.pytests/ui_and_conv/test_visualize_running_prompt.py
The earlier `if status < 500` simplification was behaviour-changing: after the `status >= 500` early return the upper bound is always true (CodeQL "redundant comparison" + "unreachable code" on the `return "api"` fallback), but dropping the `>= 400` lower bound also routed sub-400 statuses (e.g. the `status=0` default for non-HTTP-ish errors) into `4xx_client` instead of the generic `api` bucket. Use `if status >= 400` — equivalent to the original `400 <= status < 500` given the preceding return, with the `api` fallback reachable again for status < 400. Also parenthesize the remaining sparse plan-mode reminder concatenation so CodeQL's implicit-string-concatenation check stays quiet without splitting the line across the rendered output.
- soul/agent: when overriding a child's work dir, only replace PYTHINKER_AGENTS_MD when an explicit value is provided; None now keeps the parent payload instead of silently clearing inherited context. - soul/context: drop tool results with no tool_call_id during pairing repair. Keeping them left malformed history that re-broke the next provider request — the exact failure the repair exists to prevent. - subagents/worktree: a failed `git worktree list` no longer collapses to "not a registered worktree" (which could send the operator to delete a path holding the child's only work); raise WorktreeError with the git stderr instead. - tests/memory: annotate the `_runtime` helper return type (ANN202).
The drop of id-less tool results during pairing repair (previous commit) correctly removes malformed history, but three pending-token tests fed bare `tool` messages (no tool_call_id, no opening assistant tool call) as token ballast through the restore/repair path, so they now under-counted. Production tool results always carry the originating tool_call_id, so model the fixtures realistically: an assistant message that opens a tool call plus a paired tool result, both after the last `_usage`. They survive pairing repair and keep the pending estimate intact — exercising the post-`_usage` slice accounting without depending on malformed history.
Merged
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
2 participants
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
enabledTools/disabledTools), per-server startup timeout with actionable diagnostics, FetchURL untrusted-envelope fix after truncationturn()contract, strict stdout/stderr channel disciplineTest plan
make checkpasses (ruff + pyright + pytest + tests_e2e)enabledTools/disabledToolsgate correctly inmcp.jsonconfig.yaml, confirm warning with line/source infoSummary by CodeRabbit
New Features
Improvements
Bug Fixes