Skip to content

Python: scope under-specified approve-for-session permission decisions - #7607

Open
Giles Odigwe (giles17) wants to merge 3 commits into
microsoft:mainfrom
giles17:fix-copilot-session-approval
Open

Python: scope under-specified approve-for-session permission decisions#7607
Giles Odigwe (giles17) wants to merge 3 commits into
microsoft:mainfrom
giles17:fix-copilot-session-approval

Conversation

@giles17

Copy link
Copy Markdown
Contributor

Motivation & Context

A permission handler that returns a bare PermissionDecisionApproveForSession() — a
natural way to express "approve this for the rest of the session" — crashes the whole
run. PermissionDecisionApproveForSession carries an optional approval (tool prompts)
and an optional domain (URL prompts), so it can be constructed with neither. That
serializes to {"kind": "approve-for-session"}, which the Copilot CLI cannot interpret:
it dereferences the absent approval and throws Cannot read properties of undefined (reading 'commandIdentifiers'). Because the crash is inside the CLI process rather than
in Python, it takes down the entire run instead of failing a single tool call.

The scope is never actually ambiguous — the permission request that triggered the prompt
already describes what is being approved — so the framework can reconstruct it.

Description & Review Guide

  • What are the major changes?

    • _agent.py: the resolved on_permission_request handler is now wrapped in
      _build_session_kwargs so its decisions are normalized before reaching the SDK. An
      under-specified approve-for-session decision (both approval and domain unset) is
      scoped from the request that triggered it: shell → an approval for that prompt's
      command identifiers; read/write/memory → the matching approval variant; mcp → server +
      tool; custom-tool → tool name; extension prompts → operation / extension name; url →
      domain from the URL's hostname.
    • Narrow, never widen: when the prompt reports can_offer_session_approval=False, or the
      request kind has no session-scoped approval (such as a hook prompt), the decision is
      downgraded to a single-use PermissionDecisionApproveOnce() and a warning is logged.
    • Decisions that already specify a scope are forwarded unchanged, and handler exceptions
      still propagate so the SDK's deny-on-error behavior is preserved.
    • Tests and a README section documenting the behavior.
  • What is the impact of these changes?

    • The reported scenario now works instead of crashing. The only input whose behavior
      changes is the one that currently kills the CLI process, so nothing that works today
      regresses. The public API, the handler type, and _permission_handler (which still
      stores the raw handler) are unchanged; wrapping happens only when building session
      kwargs.
  • What do you want reviewers to focus on?

    • Whether inferring a session scope from the request is the right call versus always
      downgrading to a single-use approval. The inferred scope matches what the CLI's own
      interactive "approve for session" would grant, and the narrow-never-widen rule ensures
      we never grant more than was requested.

Related Issue

Fixes #7553

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

PermissionDecisionApproveForSession carries an optional `approval` (tool
prompts) and an optional `domain` (URL prompts), so it can be constructed
with neither. A bare PermissionDecisionApproveForSession() serializes to
{"kind": "approve-for-session"}, which the Copilot CLI cannot interpret: it
dereferences the absent approval and crashes the CLI process with "Cannot
read properties of undefined (reading 'commandIdentifiers')", taking the
whole run down rather than failing a single tool call.

Wrap the resolved permission handler so such decisions are scoped using the
request that triggered them: shell prompts become an approval for that
prompt's command identifiers, MCP prompts an approval for that server and
tool, URL prompts an approval for that URL's domain, and so on.

The decision is only ever narrowed, never widened. When the prompt reports
can_offer_session_approval=False, or the request kind has no session-scoped
approval (such as a hook prompt), the decision is downgraded to a single-use
approval and a warning is logged. Decisions that already specify a scope are
forwarded unchanged, and handler exceptions still propagate so the SDK's
deny-on-error behavior is preserved.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1b45752e-b602-4117-8304-3c8a8b877e3e
Copilot AI balanced review requested due to automatic review settings August 10, 2026 20:01
@agent-framework-automation agent-framework-automation Bot added documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python labels Aug 10, 2026
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Python Test Coverage

Python Test Coverage Report •
FileStmtsMissCoverMissing
packages/github_copilot/agent_framework_github_copilot
   _agent.py4822594%98–99, 144, 153–155, 159, 267–268, 272, 712, 727–728, 807, 820, 957, 961, 1130, 1164–1165, 1204, 1207, 1344, 1419, 1437
TOTAL45679425790% 

Python Unit Test Overview

Tests Skipped Failures Errors Time
9264 36 💤 0 ❌ 0 🔥 2m 27s ⏱️

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Scopes bare session-approval decisions from their triggering permission request to prevent Copilot CLI crashes.

Changes:

  • Normalizes under-specified session approvals.
  • Downgrades unsupported scopes to one-time approval.
  • Adds regression tests and usage documentation.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
_agent.py Implements permission-scope normalization.
test_github_copilot_agent.py Adds regression coverage.
README.md Documents automatic scoping behavior.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agent Framework Review — Iteration 1

Completed passes: 5 | Result: No high-severity findings

Scope: full PR (1 commit(s)): 1182e35102cb

Review passes

  • Correctness (gpt-5.6-sol) — No issues found in this pass.
  • Security Reliability (claude-opus-4.8) — No issues found in this pass.
  • Test Coverage (gpt-5.6-sol) — No issues found in this pass.
  • Failure Modes (claude-opus-4.8) — No issues found in this pass.
  • Design Approach (claude-opus-4.8) — No issues found in this pass.

The permission-handler wrapper returned PermissionHandlerType (the sync-or-async
union), so awaiting its result in tests was rejected by the stricter CI type
checkers (pyrefly, ty, zuban). Give the wrapper a dedicated
AsyncPermissionHandlerType return type, and narrow the awaited result with an
isinstance assert before accessing its scope in the async-handler test.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1b45752e-b602-4117-8304-3c8a8b877e3e
Cover the two previously-untested branches of _derive_session_approval:
extension-management preserves the request operation, and
extension-permission-access preserves the extension name. Both assert the
serialized approval payload as well.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1b45752e-b602-4117-8304-3c8a8b877e3e
@giles17
Giles Odigwe (giles17) marked this pull request as ready for review August 10, 2026 20:59

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agent Framework Review — Iteration 2

Completed passes: 5 | Result: No high-severity findings

Scope: 2 net-new commit(s): c0dfe303854e, c67cc9fae52e

Review passes

  • Correctness (gpt-5.6-sol) — No issues found in this pass.
  • Security Reliability (claude-opus-4.8) — No issues found in this pass.
  • Test Coverage (gpt-5.6-sol) — No issues found in this pass.
  • Failure Modes (claude-opus-4.8) — No issues found in this pass.
  • Design Approach (claude-opus-4.8) — No issues found in this pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: TypeError: Cannot read properties of undefined (reading 'commandIdentifiers')

2 participants