fix(copilot): address review comments — slot counting, heartbeat, error detection

- Move run_in_background check before task_spawn_count increment so
  denied background Tasks don't consume a subtask slot
- Replace asyncio.wait_for() with asyncio.timeout() for heartbeat loop
  to avoid leaving the async generator in a broken state
- Tighten _is_tool_error_or_denial: remove overly broad markers
  ("error", "failed", "not found") that cause false positives; add
  markers for actual denial messages ("not supported", "maximum")
This commit is contained in:
Zamil Majdy
2026-02-20 01:19:33 +07:00
parent fed645cb79
commit 23225aa323
2 changed files with 27 additions and 26 deletions

View File

@@ -188,6 +188,18 @@ def create_security_hooks(
# Rate-limit Task (sub-agent) spawns per session
if tool_name == "Task":
# Block background task execution first — denied calls
# should not consume a subtask slot.
if tool_input.get("run_in_background"):
logger.info(f"[SDK] Blocked background Task, user={user_id}")
return cast(
SyncHookJSONOutput,
_deny(
"Background task execution is not supported. "
"Run tasks in the foreground instead "
"(remove the run_in_background parameter)."
),
)
task_spawn_count += 1
if task_spawn_count > max_subtasks:
logger.warning(
@@ -200,19 +212,6 @@ def create_security_hooks(
"Please continue in the main conversation."
),
)
# Block background task execution — background agents stall
# the SSE stream (no messages flow while they run) and get
# killed when the main agent's turn ends.
if tool_input.get("run_in_background"):
logger.info(f"[SDK] Blocked background Task, user={user_id}")
return cast(
SyncHookJSONOutput,
_deny(
"Background task execution is not supported. "
"Run tasks in the foreground instead "
"(remove the run_in_background parameter)."
),
)
# Strip MCP prefix for consistent validation
is_copilot_tool = tool_name.startswith(MCP_TOOL_PREFIX)

View File

@@ -77,6 +77,9 @@ class CapturedTranscript:
_SDK_CWD_PREFIX = WORKSPACE_PREFIX
# Heartbeat interval — keep SSE alive through proxies/LBs during tool execution.
_HEARTBEAT_INTERVAL = 15.0 # seconds
# Appended to the system prompt to inform the agent about available tools.
# The SDK built-in Bash is NOT available — use mcp__copilot__bash_exec instead,
# which has kernel-level network isolation (unshare --net).
@@ -407,7 +410,7 @@ def _format_conversation_context(messages: list[ChatMessage]) -> str | None:
return "<conversation_history>\n" + "\n".join(lines) + "\n</conversation_history>"
def _is_tool_error_or_denial(content: str) -> bool:
def _is_tool_error_or_denial(content: str | None) -> bool:
"""Check if a tool message content indicates an error or denial.
We include these in conversation context so the agent doesn't
@@ -420,14 +423,13 @@ def _is_tool_error_or_denial(content: str) -> bool:
marker in lower
for marker in (
"[security]",
"error",
"failed",
"cannot be bypassed",
"not allowed",
"not supported", # background-task denial
"maximum", # subtask-limit denial
"denied",
"blocked",
"not found",
"not allowed",
"permission",
"cannot be bypassed",
'"iserror": true', # MCP protocol error flag
)
)
@@ -656,15 +658,15 @@ async def stream_chat_completion_sdk(
# Use an explicit async iterator with timeout to send
# heartbeats when the CLI is idle (e.g. executing tools).
# This prevents proxies/LBs from closing the SSE connection.
_HEARTBEAT_INTERVAL = 15.0 # seconds
# asyncio.timeout() is preferred over asyncio.wait_for()
# because wait_for wraps in a separate Task whose cancellation
# can leave the async generator in a broken state.
msg_iter = client.receive_messages().__aiter__()
while not stream_completed:
try:
sdk_msg = await asyncio.wait_for(
msg_iter.__anext__(),
timeout=_HEARTBEAT_INTERVAL,
)
except asyncio.TimeoutError:
async with asyncio.timeout(_HEARTBEAT_INTERVAL):
sdk_msg = await msg_iter.__anext__()
except TimeoutError:
yield StreamHeartbeat()
continue
except StopAsyncIteration: