From 5760b58fee643762b7f4ff6a167a41a05bd900bd Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Thu, 23 Jul 2026 01:07:11 -0700 Subject: [PATCH] fix: enable interactive TaskV2 and harden Bash execution --- src/entrypoints/agent_server_cli.py | 8 ++ src/server/agent_server.py | 5 +- src/tool_system/tools/bash/bash_tool.py | 12 +- src/tool_system/tools/bash/sleep_detection.py | 9 +- tests/test_bash_sleep_detection.py | 22 ++++ tests/test_task_v2_session_gating.py | 36 ++++++ tests/test_tool_system_tools.py | 23 ++++ ui-tui/src/__tests__/todoSurface.test.ts | 54 ++++++++ ui-tui/src/gatewayClient.ts | 121 +++++++++++++++++- 9 files changed, 281 insertions(+), 9 deletions(-) create mode 100644 tests/test_bash_sleep_detection.py create mode 100644 tests/test_task_v2_session_gating.py diff --git a/src/entrypoints/agent_server_cli.py b/src/entrypoints/agent_server_cli.py index babbd423e..d4307c035 100644 --- a/src/entrypoints/agent_server_cli.py +++ b/src/entrypoints/agent_server_cli.py @@ -167,6 +167,14 @@ def run_agent_server_subcommand(argv: list[str]) -> int: ) args = parser.parse_args(argv) + # The agent server is the backend for an interactive TUI/direct-connect + # client even though its own stdin/stdout are pipes. Do not let the + # process-level TTY check classify it like ``--print``: TaskV2 is the + # interactive task surface, while TodoWrite remains the headless surface. + from src.bootstrap.state import set_is_interactive + + set_is_interactive(True) + # In --stdio mode the inbound reader owns stdin, so the EOF watcher (which # also reads stdin) must NOT run — EOF is detected by the reader instead. if args.exit_on_parent and not args.stdio: diff --git a/src/server/agent_server.py b/src/server/agent_server.py index b0683599e..4b5ccb7b8 100644 --- a/src/server/agent_server.py +++ b/src/server/agent_server.py @@ -4080,7 +4080,10 @@ def _build_runtime(sess: _AgentSession, perm_mode: str | None) -> None: permission_context=perm_setup.context, abort_controller=AbortController(), ) - tool_context.options.is_non_interactive_session = True + # Agent-server sessions are driven by an interactive TUI/direct-connect + # client. The transport is non-TTY, but the session is not headless + # (notably, it must expose TaskV2 rather than TodoWrite). + tool_context.options.is_non_interactive_session = False # Scheduled tasks (/loop, Cron*, ScheduleWakeup): the tools write to # the session's scheduler; the worker's idle branch fires due prompts. tool_context.cron_scheduler = sess.cron_scheduler diff --git a/src/tool_system/tools/bash/bash_tool.py b/src/tool_system/tools/bash/bash_tool.py index ab6a23062..172acabd1 100644 --- a/src/tool_system/tools/bash/bash_tool.py +++ b/src/tool_system/tools/bash/bash_tool.py @@ -192,6 +192,12 @@ def _run_bash_with_abort( def _try_extract_cd(command: str) -> Path | None: + """Return the target only for a standalone ``cd `` command. + + Compound commands must run in the shell. Treating + ``cd /work && make`` as a pure directory change silently discards + everything after the path. + """ stripped = command.strip() if not stripped.startswith("cd "): return None @@ -199,7 +205,7 @@ def _try_extract_cd(command: str) -> Path | None: parts = shlex.split(stripped, posix=True) except ValueError: return None - if len(parts) >= 2 and parts[0] == "cd": + if len(parts) == 2 and parts[0] == "cd": return Path(parts[1]) return None @@ -449,8 +455,8 @@ def _bash_validate_input( return ValidationResult.fail( f"Blocked: {sleep_pattern}. Run blocking commands in the background " "with run_in_background: true -- you'll get a completion notification " - "when done. If you genuinely need a delay (rate limiting, deliberate " - "pacing), keep it under 2 seconds.", + "when done. If you genuinely need a short delay (rate limiting, " + "deliberate pacing), keep it at 5 seconds or less.", error_code=10, ) return ValidationResult.ok() diff --git a/src/tool_system/tools/bash/sleep_detection.py b/src/tool_system/tools/bash/sleep_detection.py index d5ab98429..00682673e 100644 --- a/src/tool_system/tools/bash/sleep_detection.py +++ b/src/tool_system/tools/bash/sleep_detection.py @@ -1,7 +1,8 @@ -"""Detect standalone or leading ``sleep N`` patterns that should be avoided. +"""Detect long standalone or leading ``sleep N`` patterns to avoid. -Catches ``sleep 5``, ``sleep 5 && check``, ``sleep 5; check`` -- but not -sleep inside pipelines, subshells, or scripts (those are fine). +Catches ``sleep 6``, ``sleep 6 && check``, ``sleep 6; check`` -- but permits +the 1-5 second waits recommended by the Bash tool prompt. Sleep inside +pipelines, subshells, or scripts is left alone. """ from __future__ import annotations @@ -22,7 +23,7 @@ def detect_blocked_sleep_pattern(command: str) -> str | None: if not m: return None secs = int(m.group(1)) - if secs < 2: + if secs <= 5: return None rest = " ".join(p.strip() for p in parts[1:] if p.strip()) diff --git a/tests/test_bash_sleep_detection.py b/tests/test_bash_sleep_detection.py new file mode 100644 index 000000000..30f180483 --- /dev/null +++ b/tests/test_bash_sleep_detection.py @@ -0,0 +1,22 @@ +from src.tool_system.tools.bash.sleep_detection import detect_blocked_sleep_pattern + + +def test_short_sleeps_match_prompt_and_are_allowed() -> None: + for seconds in range(1, 6): + assert detect_blocked_sleep_pattern( + f"sleep {seconds}; curl -s http://localhost:8080" + ) is None + + +def test_long_leading_sleep_is_blocked() -> None: + assert detect_blocked_sleep_pattern("sleep 6 && echo ready") == ( + "sleep 6 followed by: echo ready" + ) + + +def test_long_standalone_sleep_is_blocked() -> None: + assert detect_blocked_sleep_pattern("sleep 30") == "standalone sleep 30" + + +def test_sleep_inside_subshell_is_not_treated_as_leading_sleep() -> None: + assert detect_blocked_sleep_pattern("(sleep 30); echo ready") is None diff --git a/tests/test_task_v2_session_gating.py b/tests/test_task_v2_session_gating.py new file mode 100644 index 000000000..968286eef --- /dev/null +++ b/tests/test_task_v2_session_gating.py @@ -0,0 +1,36 @@ +from __future__ import annotations + +from src.bootstrap.state import get_is_interactive, set_is_interactive +from src.permissions.types import ToolPermissionContext +from src.tool_system.defaults import build_default_registry +from src.tool_system.registry import get_tools + + +def _enabled_tool_names() -> set[str]: + registry = build_default_registry() + permission_context = ToolPermissionContext(mode="bypassPermissions") + return {tool.name for tool in get_tools(registry, permission_context)} + + +def test_interactive_session_exposes_task_v2_and_hides_todo_write(monkeypatch) -> None: + previous = get_is_interactive() + monkeypatch.delenv("CLAUDE_CODE_ENABLE_TASKS", raising=False) + try: + set_is_interactive(True) + names = _enabled_tool_names() + assert {"TaskCreate", "TaskGet", "TaskList", "TaskUpdate"} <= names + assert "TodoWrite" not in names + finally: + set_is_interactive(previous) + + +def test_print_session_keeps_todo_write_and_hides_task_v2(monkeypatch) -> None: + previous = get_is_interactive() + monkeypatch.delenv("CLAUDE_CODE_ENABLE_TASKS", raising=False) + try: + set_is_interactive(False) + names = _enabled_tool_names() + assert "TodoWrite" in names + assert not {"TaskCreate", "TaskGet", "TaskList", "TaskUpdate"} & names + finally: + set_is_interactive(previous) diff --git a/tests/test_tool_system_tools.py b/tests/test_tool_system_tools.py index 4fbfc3c24..836b50aaa 100644 --- a/tests/test_tool_system_tools.py +++ b/tests/test_tool_system_tools.py @@ -197,6 +197,29 @@ def test_bash_blocks_sudo(self) -> None: with self.assertRaises(Exception): BashTool.call({"command": "sudo echo nope"}, self.ctx) + def test_bash_standalone_cd_updates_persistent_cwd(self) -> None: + child = self.root / "child" + child.mkdir() + + out = BashTool.call({"command": f"cd {child}"}, self.ctx).output + + self.assertEqual(out["cwd"], str(child)) + self.assertEqual(self.ctx.cwd, child) + + def test_bash_compound_cd_executes_remainder_and_updates_cwd(self) -> None: + child = self.root / "child" + child.mkdir() + + out = BashTool.call( + {"command": f"cd {child} && printf executed > marker && pwd"}, + self.ctx, + ).output + + self.assertEqual(out["exit_code"], 0) + self.assertEqual(out["stdout"].strip(), str(child)) + self.assertEqual((child / "marker").read_text(encoding="utf-8"), "executed") + self.assertEqual(self.ctx.cwd, child) + def _run_bash_with_pty_parent_stdin(self, tool_input: dict) -> dict: # Dup a real pty over fd 0 for the duration of one BashTool.call to # simulate clawcodex's REPL inheriting a terminal -- the failure mode diff --git a/ui-tui/src/__tests__/todoSurface.test.ts b/ui-tui/src/__tests__/todoSurface.test.ts index 8bf07582a..7792b74bf 100644 --- a/ui-tui/src/__tests__/todoSurface.test.ts +++ b/ui-tui/src/__tests__/todoSurface.test.ts @@ -79,6 +79,60 @@ describe('GatewayClient todo mapping', () => { gw.kill() }) + + it('projects TaskV2 lifecycle calls into the checklist', async () => { + const proc = new FakeProc() + harness.proc = proc + const events: any[] = [] + const gw = new GatewayClient() + + gw.on('event', (e: any) => events.push(e)) + gw.start() + gw.drain() + + const complete = async (id: string, name: string, input: unknown, result: string) => { + const expectedCount = events.filter(e => e.type === 'tool.complete').length + 1 + proc.line({ message: { content: [{ id, input, name, type: 'tool_use' }] }, type: 'assistant' }) + proc.line({ + message: { content: [{ content: result, is_error: false, tool_use_id: id, type: 'tool_result' }] }, + type: 'user' + }) + await vi.waitFor(() => expect(events.filter(e => e.type === 'tool.complete')).toHaveLength(expectedCount)) + + return events.filter(e => e.type === 'tool.complete').at(-1).payload.todos + } + + expect( + await complete( + 't1', + 'TaskCreate', + { activeForm: 'Fixing auth', description: 'Details', subject: 'Fix auth' }, + '{"task":{"id":"abc123","subject":"Fix auth"}}' + ) + ).toEqual([{ activeForm: 'Fixing auth', content: 'Fix auth', id: 'abc123', status: 'pending' }]) + + expect( + await complete('t2', 'TaskUpdate', { status: 'in_progress', taskId: 'abc123' }, 'Updated task #abc123 status') + ).toEqual([{ activeForm: 'Fixing auth', content: 'Fix auth', id: 'abc123', status: 'in_progress' }]) + + expect( + await complete( + 't3', + 'TaskList', + {}, + '{"tasks":[{"id":"abc123","subject":"Fix auth","status":"completed"},{"id":"def456","subject":"Add tests","status":"pending"}]}' + ) + ).toEqual([ + { activeForm: 'Fixing auth', content: 'Fix auth', id: 'abc123', status: 'completed' }, + { content: 'Add tests', id: 'def456', status: 'pending' } + ]) + + expect( + await complete('t4', 'TaskUpdate', { status: 'deleted', taskId: 'abc123' }, 'Updated task #abc123 deleted') + ).toEqual([{ content: 'Add tests', id: 'def456', status: 'pending' }]) + + gw.kill() + }) }) // ── turnController: silent completion, no stranded spinner ────────────────── diff --git a/ui-tui/src/gatewayClient.ts b/ui-tui/src/gatewayClient.ts index 59676dbef..c1af997b1 100644 --- a/ui-tui/src/gatewayClient.ts +++ b/ui-tui/src/gatewayClient.ts @@ -157,6 +157,16 @@ function todosFromInput(name: string | undefined, input: unknown): undefined | u return Array.isArray(todos) ? todos : undefined } +type TaskTodo = { + activeForm?: string + content: string + id: string + status: 'completed' | 'in_progress' | 'pending' +} + +const taskListLine = /^#(\S+)\s+\[(pending|in_progress|completed)\]\s+(.+)$/ +const createdTaskId = /^Task #(\S+) created successfully:/m + // Per-tool result summaries, matching the original Claude Code transcript // (tools/*/UI.tsx): Read → "Read N lines", Grep/Glob → "Found N …", Bash → // first 3 stdout lines + overflow hint, errors → red "Error: …" capped at 10 @@ -414,6 +424,10 @@ export class GatewayClient extends EventEmitter { private seenSubagents = new Set() // Tool inputs by tool_use id, so tool_result can render an Edit/Write diff. private toolInputs = new Map() + // TaskV2 mutates one task at a time, unlike TodoWrite which carries the + // complete checklist in every call. Keep a session-local projection so the + // existing checklist HUD can render both protocols. + private taskTodos = new Map() private msgStarted = false private pending = new Map() private pendingExit: null | number | undefined @@ -1620,6 +1634,7 @@ export class GatewayClient extends EventEmitter { const isError = Boolean(b.is_error) const fullText = typeof b.content === 'string' ? b.content : safeJson(b.content) const resultText = formatToolResult(stored?.name, fullText, isError, webSearch) + const taskTodos = isError ? undefined : this.taskTodosFromResult(stored?.name, stored?.input, fullText) // Read shows no expand hint (the summary loses nothing the user // needs — the file is in context), so retain nothing for it. const expandable = stored?.name !== 'Read' @@ -1634,7 +1649,7 @@ export class GatewayClient extends EventEmitter { result_raw: expandable ? rawToolResult(resultText, fullText) : undefined, result_text: resultText, structured_diff: isError ? undefined : structured, - todos: isError ? undefined : todosFromInput(stored?.name, stored?.input), + todos: taskTodos ?? (isError ? undefined : todosFromInput(stored?.name, stored?.input)), tool_id: b.tool_use_id }, type: 'tool.complete' @@ -1694,6 +1709,110 @@ export class GatewayClient extends EventEmitter { } } + private taskTodosFromResult(name: string | undefined, input: unknown, result: string): undefined | TaskTodo[] { + if (!name?.startsWith('Task') || !input || typeof input !== 'object') { + return undefined + } + + const args = input as Record + let parsed: Record | undefined + + try { + const value = JSON.parse(result) + + if (value && typeof value === 'object' && !Array.isArray(value)) { + parsed = value + } + } catch { + // Older backends emitted the human-readable TaskV2 strings parsed below. + } + + if (name === 'TaskCreate') { + const id = String(parsed?.task?.id ?? result.match(createdTaskId)?.[1] ?? '').trim() + const content = String(args.subject ?? '').trim() + + if (id && content) { + const activeForm = String(args.activeForm ?? '').trim() + this.taskTodos.set(id, { + ...(activeForm && { activeForm }), + content, + id, + status: 'pending' + }) + } + } else if (name === 'TaskUpdate') { + const id = String(args.taskId ?? '').trim() + + if (parsed?.success === false) { + return [...this.taskTodos.values()] + } else if (id && args.status === 'deleted') { + this.taskTodos.delete(id) + } else if (id) { + const current = this.taskTodos.get(id) + + if (current) { + const content = String(args.subject ?? current.content).trim() + const activeForm = String(args.activeForm ?? current.activeForm ?? '').trim() + const status = + args.status === 'pending' || args.status === 'in_progress' || args.status === 'completed' + ? args.status + : current.status + this.taskTodos.set(id, { + ...(activeForm && { activeForm }), + content, + id, + status + }) + } + } + } else if (name === 'TaskList') { + const listed = new Map() + + if (Array.isArray(parsed?.tasks)) { + for (const task of parsed.tasks) { + const id = String(task?.id ?? '').trim() + const content = String(task?.subject ?? '').trim() + const status = task?.status + + if (!id || !content || (status !== 'pending' && status !== 'in_progress' && status !== 'completed')) { + continue + } + const previous = this.taskTodos.get(id) + listed.set(id, { + ...(previous?.activeForm && { activeForm: previous.activeForm }), + content, + id, + status + }) + } + } else { + for (const line of result.split('\n')) { + const match = line.match(taskListLine) + + if (!match) {continue} + const [, id, status, content] = match + const previous = this.taskTodos.get(id!) + listed.set(id!, { + ...(previous?.activeForm && { activeForm: previous.activeForm }), + content: content!, + id: id!, + status: status as TaskTodo['status'] + }) + } + } + + // "No tasks found" is an authoritative empty list. For other malformed + // output, preserve the projection rather than blanking the HUD. + if (listed.size || Array.isArray(parsed?.tasks) || result.trim() === 'No tasks found') { + this.taskTodos = listed + } + } else { + return undefined + } + + return [...this.taskTodos.values()] + } + private ensureMsgStart(): void { if (!this.msgStarted) { this.msgStarted = true