Skip to content

FEAT: Add GitHub Copilot SDK target and native judge retries - #2828

Open
Adrian Gavrila (adrian-gavrila) wants to merge 8 commits into
microsoft:mainfrom
adrian-gavrila:adrian-gavrila-copilot-target-design
Open

Adrian Gavrila (adrian-gavrila) wants to merge 8 commits into
microsoft:mainfrom
adrian-gavrila:adrian-gavrila-copilot-target-design

Conversation

@adrian-gavrila

Copy link
Copy Markdown
Contributor

Description

Add GitHubCopilotTarget for native text conversations through the optional
github-copilot SDK extra.

  • Support explicit model selection, initial system instructions, existing-login/token
    authentication, caller-selected working directories, request deadlines, and local retention.
  • Own native sessions per PyRIT conversation, with serialized turns, scoped reset, and
    caller-owned terminal cleanup. Keep history noneditable and remote session export disabled.
  • Keep a selected conversation's reset outcome independent of unrelated cleanup failures,
    while preserving its own release errors and caller cancellation.
  • Enable self-contained text judgments in SelfAskRefusalScorer and
    SelfAskTrueFalseScorer. Malformed judge JSON retries use fresh conversations while
    preserving the original evidence and shared retry limits.
  • Derive judge compatibility from the shared target requirements; preserve existing
    editable-history behavior and SelfAskQuestionAnswerScorer's current restrictions.
  • Keep generic scorer tests independent of the optional SDK, with a separate test of
    native session creation and cleanup through the Copilot adapter.

Known upstream limitation

uv.lock selects SDK 1.0.14, whose runtime 1.0.85 produced an unhandled
shutdownType="destroy" notification callback exception after successful scoring.
An upstream report has been filed. The exception is not suppressed.
Cleanup and native-data deletion remain unverified for that pair.
Authentication checks with SDK 1.0.11/runtime 1.0.79 completed without cleanup errors.

Additional scorer integrations, adversarial workflow integration, editable-history/rewind
support, and custom model endpoints are not included.

Tests and Documentation

  • 246 related unit tests passed with SDK 1.0.14, including overlapping reset/cleanup
    success and failure cases.
  • 437 tests covering target orchestration, the Copilot adapter, affected scorers, JSON
    retries, and lazy imports passed with SDK 1.0.11 on the final integrated revision.
  • Generic scorer suites passed 187 tests with Copilot imports blocked and zero skips.
  • Lint, formatting, type, and async checks passed. Tests were also checked against
    deliberately injected faults: incorrect payloads, blocked event-loop execution,
    and missing session release.
  • Isolated live requests validated stored Copilot login, explicit PAT, all three token
    environment variables, and gh fallback on SDK 1.0.11/runtime 1.0.79.
  • Native continuity and refusal scoring were exercised live. TrueFalse scoring and
    malformed-JSON recovery were validated offline.
  • Independent offline checks verified that reset preserves selected-session failures and
    caller cancellation without inheriting an unrelated session's cleanup error.

This PR contains no documentation changes. The full repository test suite and hosted CI
have not been run.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
self,
*,
model_name: str,
github_token: str | None = None,

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.

wondering if we should add something like GITHUB_TOKEN_ENVIRONMENT_VARIABLE as a class constant and resolve via default_values.get_non_required_value(env_var_name=..., passed_value=github_token)

@hannahwestra25

Copy link
Copy Markdown
Contributor

can we add docs for this

_capture_response(response)
try:
return _parse(response)
except InvalidJsonException:

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.

reset_conversation_async runs only in the InvalidJsonException branch (308); the success return at 306 never releases. Nothing else does — _ObjectiveTargetConversationLifecycle resets only the objective target, and cleanup_target_async has no caller in pyrit/. Line 163 mints a new id per call, so every score leaks a session. Use try/finally

category: Sequence[str] | str | None = None,
objective: str | None = None,
normalizer: PromptNormalizer | None = None,
fresh_conversation_per_attempt: bool = False,

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.

can't we derive this from chat_target ?

is_non_text = message_piece.converted_value_data_type != "text"
if is_non_text and not self._prompt_target.capabilities.supports_editable_history:
raise ValueError(
"non-text scoring requires editable history; fresh-conversation retries support text only."

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.

should we be checking input modalities on the target here ?

@romanlutz Roman Lutz (romanlutz) 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.

GHCP comments, will manually review as well

Comment on lines +144 to +146
request = normalized_conversation[-1].get_piece()
conversation_id = request.conversation_id or ""
conversation = self._conversations.setdefault(conversation_id, _ConversationState())

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.

🔴 Must Fix: reject missing conversation IDs instead of sharing one session

MessagePiece.conversation_id defaults to None, and the public send_prompt_async() accepts that value. Mapping it to "" makes separate requests with no ID reuse the same native Copilot session. In an offline mock reproduction, two independent messages with no ID caused one create_session() call and two sends on that session, so the second request would inherit the first request's native history.

Please reject a missing or empty conversation ID before starting the SDK, as WebsocketTarget does, or assign and return a distinct ID for each independent request. Add coverage for direct calls without IDs, not just calls through PromptNormalizer.

Comment on lines +165 to +171
except BaseException:
retirement_task = asyncio.create_task(self._retire_conversation_async(conversation=conversation))
try:
await asyncio.shield(retirement_task)
except asyncio.CancelledError:
await retirement_task
raise

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.

🔴 Must Fix: preserve caller cancellation when retirement fails

If the caller cancels a send and delete_session() fails while retiring that session, the cleanup error escapes before the original CancelledError is re-raised. I reproduced this offline by cancelling a blocked send and making deletion raise RuntimeError: awaiting the send raised that cleanup error, and task.cancelled() was False despite a cancellation request.

Please retain the original exception and preserve cancellation even when retirement fails, while keeping the cleanup error available as its cause. The client-startup cleanup below already handles this distinction. The cancellation test should include a failing retirement, not only a successful one.

Comment on lines +340 to +345
async with self._lifecycle_condition:
conversation.session = session
return session
except BaseException:
await self._cleanup_allocated_session_async(client=client, session_id=session_id)
raise

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.

🔴 Must Fix: keep ownership of a partially created session when cleanup fails

conversation.session is only assigned after create_session() returns. If the SDK creates the native session but its post-create setup fails, and the deletion here also fails, the target loses the allocated session ID. Later cleanup_target_async() skips that conversation because session is None and can return successfully without retrying deletion.

I reproduced this with a synthetic allocation and a deletion that fails once: two whole-target cleanups plus a reset still made only one deletion attempt. The SDK's stop() explicitly preserves session data on disk, so stopping the client does not satisfy retain_session=False in this case.

Please track the allocated ID before creation and retain it until deletion succeeds, so terminal cleanup can retry it or report that the owned session remains unreleased. Add the partial-create plus failed-delete case to the existing cleanup tests.

Comment on lines +257 to +263
if cleanup_task is not None:
try:
await asyncio.shield(cleanup_task)
except asyncio.CancelledError:
raise
except Exception:
async with self._lifecycle_condition:

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.

🟡 Should Fix: distinguish caller cancellation from cancellation of shared cleanup

asyncio.shield() also raises CancelledError when the task being awaited is cancelled. That does not necessarily mean this reset's caller was cancelled.

For example, start reset_conversation_async(A) while whole-target cleanup is releasing A. If A is released successfully but releasing unrelated B raises CancelledError, the cleanup task becomes cancelled and this branch cancels A's reset too. An offline mock reproduction left A fully released while its reset task was cancelled with task.cancelling() == 0.

Please preserve genuine cancellation of the reset caller, but determine the selected conversation's outcome separately when shared cleanup is cancelled. Add this case alongside the existing unrelated-RuntimeError test.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants