Skip to content

Commit 76b90e8

Browse files
authored
fix: background reliability, TUI/agent loop, and CodeRabbit hardening (#13)
* fix(todo): parse JSON-encoded string passed as todos list 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. * fix: harden background task sync and reliability across subsystems 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. * fix(background): crash-consistent agent-task status (H2) + prune aged 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. * fix(background): don't reconcile a live agent's record off a stale terminal 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(). * fix(tui): stop rendering the todo list twice during an in-flight turn 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. * fix(background): steer away from blocking on one task when siblings run 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. * fix(soul): nudge once when a turn ends on a bare statement of intent 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. * fix: address CodeRabbit review findings across subsystems - 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. * ci: satisfy spell-check and host formatting gates - 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. * ci: fix Windows host pyright and main-package formatting gates - 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. * test: refresh default-config snapshot for new background fields 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. * fix: log idle-reminder task-count failures; test grep fallback via public 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.
1 parent 4253c53 commit 76b90e8

38 files changed

Lines changed: 1545 additions & 206 deletions

packages/pythinker-host/src/pythinker_host/local.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,11 @@ async def wait(self) -> int:
6868
return await self._process.wait()
6969

7070
async def kill(self) -> None:
71+
# If the process has already exited (and been reaped), its pid/pgid
72+
# may have been recycled by the OS; signaling it could hit an
73+
# unrelated process group. Skip the group-kill in that case.
74+
if self._process.returncode is not None:
75+
return
7176
if os.name != "nt":
7277
try:
7378
os.killpg(os.getpgid(self._process.pid), signal.SIGKILL)

packages/pythinker-host/tests/test_local_host.py

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@
22

33
import asyncio
44
import os
5+
import signal
56
import sys
67
from collections.abc import Generator
78
from pathlib import Path, PurePosixPath, PureWindowsPath
9+
from typing import Any
810

911
import pytest
1012

@@ -196,3 +198,62 @@ async def test_exec_wait_timeout(local_host: LocalHost):
196198
if process.returncode is None:
197199
await process.kill()
198200
await process.wait()
201+
202+
203+
@pytest.mark.skipif(os.name == "nt", reason="POSIX process-group signal path")
204+
async def test_kill_skips_signal_after_process_exit(
205+
local_host: LocalHost, monkeypatch: pytest.MonkeyPatch
206+
):
207+
"""Once a process has exited (and been reaped), its pid/pgid may be recycled
208+
by the OS, so kill() must not send a process-group signal."""
209+
import pythinker_host.local as local_module
210+
211+
process = await local_host.exec(*_python_code_args("import sys; sys.exit(0)"))
212+
await process.wait()
213+
assert process.returncode is not None
214+
215+
# Fail loudly if kill() reaches the signal path at all: with the returncode
216+
# guard it must short-circuit before even resolving the process group.
217+
calls: list[str] = []
218+
219+
def _record_getpgid(pid: int) -> int:
220+
calls.append("getpgid")
221+
return 0
222+
223+
def _record_killpg(pgid: int, sig: int) -> None:
224+
calls.append("killpg")
225+
226+
monkeypatch.setattr(local_module.os, "getpgid", _record_getpgid)
227+
monkeypatch.setattr(local_module.os, "killpg", _record_killpg)
228+
229+
await process.kill()
230+
231+
assert calls == []
232+
233+
234+
@pytest.mark.skipif(os.name == "nt", reason="POSIX process-group signal path")
235+
async def test_kill_signals_running_process(local_host: LocalHost, monkeypatch: pytest.MonkeyPatch):
236+
"""A still-running process is killed via its process group."""
237+
import pythinker_host.local as local_module
238+
239+
process = await local_host.exec(*_python_code_args("import time; time.sleep(30)"))
240+
assert process.returncode is None
241+
242+
# os.killpg / signal.SIGKILL are POSIX-only; this test is skipped on Windows
243+
# but pyright still type-checks the body, so route them through Any holders
244+
# to keep the strict host gate green on the win32 platform stubs.
245+
os_any: Any = local_module.os
246+
signal_any: Any = signal
247+
real_killpg = os_any.killpg
248+
sent: list[int] = []
249+
250+
def _record_killpg(pgid: int, sig: int) -> None:
251+
sent.append(sig)
252+
real_killpg(pgid, sig) # actually terminate so the process is not leaked
253+
254+
monkeypatch.setattr(local_module.os, "killpg", _record_killpg)
255+
256+
await process.kill()
257+
await process.wait()
258+
259+
assert sent and sent[0] == signal_any.SIGKILL

pyproject.toml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,4 +178,13 @@ Encrypter = "Encrypter"
178178
# Hex session IDs (e.g. 06ba6c38) contain "ba".
179179
ba = "ba"
180180
uest = "uest"
181+
# Verb stems in the unfinished-intent detection regex (soul/pythinkersoul.py)
182+
# match word prefixes ("prepar" → prepare/preparing); kept as bare stems on
183+
# purpose so they cover every inflection.
184+
prepar = "prepar"
185+
generat = "generat"
186+
provid = "provid"
187+
updat = "updat"
188+
examin = "examin"
189+
continu = "continu"
181190

src/pythinker_code/auth/github_feedback.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import asyncio
44
import time
5-
import webbrowser
65
from dataclasses import dataclass
76
from typing import Any, cast
87

@@ -150,7 +149,9 @@ async def login_github_feedback(
150149
)
151150
if open_browser:
152151
try:
153-
webbrowser.open(auth.verification_uri)
152+
from pythinker_code.utils.term import open_url_in_browser
153+
154+
open_url_in_browser(auth.verification_uri)
154155
except Exception as exc:
155156
logger.warning("Failed to open browser: {error}", error=exc)
156157
yield OAuthEvent("waiting", "Waiting for GitHub authorization...")

src/pythinker_code/auth/oauth.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
import tempfile
1111
import time
1212
import uuid
13-
import webbrowser
1413
from collections.abc import AsyncGenerator, AsyncIterator
1514
from contextlib import asynccontextmanager, suppress
1615
from dataclasses import dataclass
@@ -660,7 +659,9 @@ async def login_pythinker_code(
660659
)
661660
if open_browser:
662661
try:
663-
webbrowser.open(auth.verification_uri_complete)
662+
from pythinker_code.utils.term import open_url_in_browser
663+
664+
open_url_in_browser(auth.verification_uri_complete)
664665
except Exception as exc:
665666
logger.warning("Failed to open browser: {error}", error=exc)
666667

src/pythinker_code/auth/openai.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
import json
88
import secrets
99
import time
10-
import webbrowser
1110
from collections.abc import AsyncIterator
1211
from dataclasses import dataclass
1312
from typing import Any, cast
@@ -329,7 +328,9 @@ async def _wait_for_browser_code(open_browser: bool = True) -> tuple[str, str, s
329328

330329
auth_url = _build_authorize_url(redirect_uri=redirect_uri, pkce=pkce, state=state)
331330
if open_browser:
332-
webbrowser.open(auth_url)
331+
from pythinker_code.utils.term import open_url_in_browser
332+
333+
open_url_in_browser(auth_url)
333334

334335
try:
335336
code, error = await asyncio.wait_for(result, timeout=15 * 60)

src/pythinker_code/background/agent_runner.py

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -99,35 +99,41 @@ async def run(self) -> None:
9999
id=self._task_id,
100100
t=self._timeout_s,
101101
)
102-
self._runtime.subagent_store.update_instance(self._agent_id, status="failed")
103-
self._manager._mark_task_timed_out(
104-
self._task_id, f"Agent task timed out after {self._timeout_s}s"
102+
self._manager.finalize_agent_task(
103+
self._task_id,
104+
self._agent_id,
105+
outcome="timed_out",
106+
reason=f"Agent task timed out after {self._timeout_s}s",
105107
)
106108
output.error(
107109
_timeout_recovery_message(timeout_s=self._timeout_s, agent_id=self._agent_id)
108110
)
109111
else:
110112
# Internal timeout (e.g. aiohttp request) — treat as generic failure
111113
logger.exception("Background agent runner failed")
112-
self._runtime.subagent_store.update_instance(self._agent_id, status="failed")
113-
self._manager._mark_task_failed(self._task_id, str(exc))
114+
self._manager.finalize_agent_task(
115+
self._task_id, self._agent_id, outcome="failed", reason=str(exc)
116+
)
114117
output.error(str(exc))
115118
except asyncio.CancelledError:
116-
self._runtime.subagent_store.update_instance(self._agent_id, status="killed")
117-
self._manager._mark_task_killed(self._task_id, "Stopped by TaskStop")
119+
self._manager.finalize_agent_task(
120+
self._task_id, self._agent_id, outcome="killed", reason="Stopped by TaskStop"
121+
)
118122
output.stage("cancelled")
119123
raise
120124
except RunCancelled:
121125
# RunCancelled is Exception (not BaseException), so re-raising it from
122126
# an asyncio.create_task would trigger "Task exception was never retrieved".
123127
# Just mark killed and return — cleanup is already done.
124-
self._runtime.subagent_store.update_instance(self._agent_id, status="killed")
125-
self._manager._mark_task_killed(self._task_id, "Run was cancelled")
128+
self._manager.finalize_agent_task(
129+
self._task_id, self._agent_id, outcome="killed", reason="Run was cancelled"
130+
)
126131
output.stage("cancelled")
127132
except Exception as exc:
128133
logger.exception("Background agent runner failed")
129-
self._runtime.subagent_store.update_instance(self._agent_id, status="failed")
130-
self._manager._mark_task_failed(self._task_id, str(exc))
134+
self._manager.finalize_agent_task(
135+
self._task_id, self._agent_id, outcome="failed", reason=str(exc)
136+
)
131137
output.error(str(exc))
132138
finally:
133139
# Whatever happens in approval cleanup below, the dict pop must
@@ -197,22 +203,24 @@ async def _ui_loop_fn(wire: Wire) -> None:
197203
),
198204
)
199205
if failure is not None:
200-
self._manager._mark_task_failed(self._task_id, failure.message)
201-
self._runtime.subagent_store.update_instance(self._agent_id, status="failed")
206+
self._manager.finalize_agent_task(
207+
self._task_id, self._agent_id, outcome="failed", reason=failure.message
208+
)
202209
output.stage(f"failed: {failure.brief}")
203210
return
204211
output.stage("run_soul_finished")
205212

206213
if final_response is None:
207-
self._manager._mark_task_failed(
208-
self._task_id, "Agent completed but produced no output."
214+
self._manager.finalize_agent_task(
215+
self._task_id,
216+
self._agent_id,
217+
outcome="failed",
218+
reason="Agent completed but produced no output.",
209219
)
210-
self._runtime.subagent_store.update_instance(self._agent_id, status="failed")
211220
output.stage("failed: empty output")
212221
return
213222
output.summary(final_response)
214-
self._runtime.subagent_store.update_instance(self._agent_id, status="idle")
215-
self._manager._mark_task_completed(self._task_id)
223+
self._manager.finalize_agent_task(self._task_id, self._agent_id, outcome="completed")
216224

217225
def _on_approval_runtime_event(self, event: ApprovalRuntimeEvent) -> None:
218226
request = event.request

0 commit comments

Comments
 (0)