Skip to content

fix(copilot): add permission handler, event logging, and idle detection race fix #15

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jason Robert (jrob5756) merged 2 commits into from
Mar 3, 2026
Merged

fix(copilot): add permission handler, event logging, and idle detection race fix #15

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2426156
fix(copilot): add permission handler, event logging, and idle detecti…
lucioti Mar 3, 2026
a5071ff
fix(copilot): address PR review — extract permission handler, fix res…
lucioti Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions src/conductor/providers/copilot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,9 @@ async def _execute_sdk_call(
# Build session config with MCP servers from workflow configuration
session_config: dict[str, Any] = {
"model": model,
# Auto-approve all permission requests (shell, write, mcp, read, url)

@jrob5756 Jason Robert (jrob5756) Mar 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Missing resume_session handler: SDK v0.1.28+ also requires on_permission_request when calling resume_session(). Without it, resuming workflows will still raise the permission denied error.

Around line 429 where resume_session is called, pass a config dict:

session = await self._client.resume_session(
    resume_sid,
    {"on_permission_request": self._default_permission_handler},
)

# since Conductor workflows run non-interactively.

@jrob5756 Jason Robert (jrob5756) Mar 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 No audit logging on permission approvals. Since this handler silently auto-approves dangerous categories (shell, write, mcp), consider adding a logger.debug so approvals are visible in --log-file output:

logger.debug("auto-approved permission request: %s", request)
return {"kind": "approved"}

"on_permission_request": lambda _req, _ctx: {"kind": "approved"},

@jrob5756 Jason Robert (jrob5756) Mar 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Lint failure (CI blocker): This inline lambda is causing the ruff format check to fail in CI. Extract it to a named static method with proper type hints and a docstring.

Suggestion:

@staticmethod
def _default_permission_handler(
    request: dict[str, Any],
    invocation: dict[str, str],
) -> dict[str, Any]:
    """Default permission handler that approves all requests.

    SDK v0.1.28+ requires a permission handler on session creation.
    In orchestration mode, we approve all tool permissions since the
    workflow author controls which tools are available to each agent.
    """
    return {"kind": "approved"}

Then reference it here as self._default_permission_handler.

}

# Add temperature if configured
Expand Down Expand Up @@ -621,6 +624,16 @@ def on_event(event: Any) -> None:
nonlocal response_content, error_message
event_type = event.type.value if hasattr(event.type, "value") else str(event.type)

# Log every SDK event for debugging stalls (visible via --log-file)
if logger.isEnabledFor(logging.DEBUG):
tool_info = ""
if event_type == "tool.execution_start":
tn = getattr(event.data, "tool_name", None) or getattr(
event.data, "name", "?"

@jrob5756 Jason Robert (jrob5756) Mar 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Minor: The getattr(event.data, ...) chain will raise AttributeError if the event object itself has no data attribute (e.g., a malformed SDK event). Consider adding a defensive guard:

if event_type == "tool.execution_start" and hasattr(event, "data") and event.data is not None:

Not a regression (existing code has the same pattern), but since this is new code it's worth hardening.

)
tool_info = f" tool={tn}"
logger.debug("sdk_event: %s%s", event_type, tool_info)

# Update last activity on EVERY event (this is key for idle detection!)
last_activity_ref[0] = event_type
last_activity_ref[2] = time.monotonic()
Expand Down Expand Up @@ -1267,6 +1280,11 @@ async def _wait_with_idle_detection(
idle_timeout = self._idle_recovery_config.idle_timeout_seconds

while True:

@jrob5756 Jason Robert (jrob5756) Mar 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Good fix. This early-return guard closes a real race window where session.idle could arrive between a previous done.clear() and the await wait_for(), causing the completion signal to be lost and an unnecessary full idle-timeout wait.

# Check if done was already set (avoids race where session.idle
# arrived between a previous done.clear() and the next wait).
if done.is_set():
return

try:
# Wait for done with idle timeout
await asyncio.wait_for(
Expand All @@ -1288,7 +1306,11 @@ async def _wait_with_idle_detection(
# just hasn't finished yet. Reset recovery counter (new task)

@jrob5756 Jason Robert (jrob5756) Mar 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Good fix. Without this guard, if wait_for raises TimeoutError but done was set concurrently (e.g., session.idle arrived during the timeout window), the unconditional clear() would erase the legitimate completion signal. The check-then-act is safe here since there's no await between is_set() and clear().

# and keep waiting.
recovery_attempts = 0
done.clear()
# Only clear if done hasn't been set in the meantime
# (prevents race where session.idle arrives right as we
# check time_since_last_event).
if not done.is_set():
done.clear()
continue

# Genuinely idle — no events for the full timeout period
Expand Down Expand Up @@ -1321,9 +1343,10 @@ async def _wait_with_idle_detection(
recovery_prompt = self._build_recovery_prompt(last_event_type, last_tool_call)
await session.send({"prompt": recovery_prompt})

# Reset the done event to wait again
# (it may have been set by a previous partial response)
done.clear()
# Reset the done event to wait again — but only if it hasn't
# been set since the recovery prompt was sent.
if not done.is_set():
done.clear()

async def _ensure_client_started(self) -> None:
"""Ensure the Copilot client is started."""
Expand Down
Loading