Fix Python permission handler in custom agents guide#672
Fix Python permission handler in custom agents guide#672patniko wants to merge 8 commits intocustom-agents-guidefrom
Conversation
* Add cross-repo issue analysis agentic workflow Adds an agentic workflow that analyzes issues filed in copilot-sdk to determine if the root cause is in copilot-agent-runtime. When a runtime fix is needed, it automatically creates a linked issue and draft PR in the runtime repo. Triggers on new issues and manual workflow_dispatch. Requires a CROSS_REPO_PAT secret with access to both repos. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Rename secret to RUNTIME_TRIAGE_TOKEN Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The agent's GitHub MCP tools need the PAT to read files from copilot-agent-runtime. The github-token under safe-outputs only applies to safe output jobs, not the agent itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add steps: to clone copilot-agent-runtime onto the runner - Add bash tools (grep, find, cat) so agent searches locally - Update prompt to reference local filesystem paths - Rename workflow to SDK Runtime Triage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e0850b0 to
3095ba0
Compare
The agent runs in a chroot that only sees GITHUB_WORKSPACE. Clone copilot-agent-runtime into the workspace directory so the agent can search it with local bash tools. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Each updates entry needs patterns: ['*'] when using multi-ecosystem-groups so Dependabot batches all dependencies into a single weekly PR instead of creating individual PRs per dependency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3095ba0 to
8058f4f
Compare
There was a problem hiding this comment.
Pull request overview
Updates documentation and E2E coverage around permissions/session-resume/streaming behavior across SDKs, plus adds a new agentic workflow for cross-repo issue analysis.
Changes:
- Fix Python
on_permission_requestexample signature in the custom agents guide. - Add/adjust E2E tests + snapshots for streaming delta behavior (including after session resume) and for continued stateful conversation after resume.
- Standardize CI detection in harnesses to
GITHUB_ACTIONS=="true"and add a cross-repo issue analysis workflow (+ compiled lock file).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/snapshots/streaming_fidelity/should_produce_deltas_after_session_resume.yaml | Adds snapshot for streaming deltas after resuming a session |
| test/snapshots/session/should_resume_a_session_using_the_same_client.yaml | Extends resume snapshot with a follow-up message to validate statefulness |
| test/snapshots/session/should_resume_a_session_using_a_new_client.yaml | Extends resume snapshot (new client) with a follow-up message to validate statefulness |
| test/snapshots/session/should_receive_streaming_delta_events_when_streaming_is_enabled.yaml | Removes snapshot for deleted streaming-delta test in session suite |
| test/snapshots/session/should_pass_streaming_option_to_session_creation.yaml | Removes snapshot for deleted “streaming option accepted” test |
| test/harness/replayingCapiProxy.ts | Switches CI strictness check to GITHUB_ACTIONS |
| python/e2e/testharness/context.py | Switches fake-token CI detection to GITHUB_ACTIONS |
| python/e2e/test_streaming_fidelity.py | Adds Python E2E streaming fidelity test module |
| python/e2e/test_session.py | Adds stateful follow-up assertions after resume; removes older streaming tests |
| nodejs/test/e2e/streaming_fidelity.test.ts | Adds “deltas after session resume” coverage |
| nodejs/test/e2e/session.test.ts | Adds stateful follow-up assertions after resume; removes older streaming tests |
| nodejs/test/e2e/harness/sdkTestContext.ts | Introduces isCI based on GITHUB_ACTIONS and reuses it |
| go/internal/e2e/testharness/context.go | Switches fake-token CI detection to GITHUB_ACTIONS |
| go/internal/e2e/streaming_fidelity_test.go | Adds Go E2E streaming fidelity tests |
| go/internal/e2e/session_test.go | Adds stateful follow-up assertions after resume; removes older streaming tests |
| dotnet/test/StreamingFidelityTests.cs | Adds .NET E2E streaming fidelity tests |
| dotnet/test/SessionTests.cs | Adds stateful follow-up assertions after resume; removes older streaming tests |
| dotnet/test/Harness/E2ETestContext.cs | Switches CI detection to GITHUB_ACTIONS for token + snapshot writing |
| docs/guides/custom-agents.md | Adds/updates custom agents guide (including Python handler signature fix) |
| .github/workflows/cross-repo-issue-analysis.md | Adds agentic workflow definition (source markdown) |
| .github/workflows/cross-repo-issue-analysis.lock.yml | Adds compiled workflow lock file for CI execution |
| .github/aw/actions-lock.json | Adds gh-aw setup action pin for the new workflow |
Comments suppressed due to low confidence (1)
go/internal/e2e/streaming_fidelity_test.go:151
- In the resume subtest, the original session is never destroyed before calling ResumeSession on a new client. Session.Destroy() is what releases resources and is the documented way to continue later via ResumeSession; not destroying here can leak resources and may prevent the session state from being properly finalized/persisted before resume. Please destroy
session(and handle the returned error) before creating/resumingsession2, and also ensuresession2is destroyed at the end of the subtest.
session, err := client.CreateSession(t.Context(), &copilot.SessionConfig{
OnPermissionRequest: copilot.PermissionHandler.ApproveAll,
Streaming: false,
})
if err != nil {
t.Fatalf("Failed to create session: %v", err)
}
_, err = session.SendAndWait(t.Context(), copilot.MessageOptions{Prompt: "What is 3 + 6?"})
if err != nil {
t.Fatalf("Failed to send message: %v", err)
}
// Resume using a new client
newClient := ctx.NewClient()
defer newClient.ForceStop()
session2, err := newClient.ResumeSession(t.Context(), session.SessionID, &copilot.ResumeSessionConfig{
OnPermissionRequest: copilot.PermissionHandler.ApproveAll,
Streaming: true,
})
if err != nil {
| session, err := client.CreateSession(t.Context(), &copilot.SessionConfig{ | ||
| OnPermissionRequest: copilot.PermissionHandler.ApproveAll, | ||
| Streaming: true, | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create session with streaming: %v", err) | ||
| } | ||
|
|
||
| var events []copilot.SessionEvent | ||
| session.On(func(event copilot.SessionEvent) { | ||
| events = append(events, event) | ||
| }) | ||
|
|
||
| _, err = session.SendAndWait(t.Context(), copilot.MessageOptions{Prompt: "Count from 1 to 5, separated by commas."}) | ||
| if err != nil { | ||
| t.Fatalf("Failed to send message: %v", err) | ||
| } | ||
|
|
||
| // Should have streaming deltas before the final message | ||
| var deltaEvents []copilot.SessionEvent | ||
| for _, e := range events { | ||
| if e.Type == "assistant.message_delta" { | ||
| deltaEvents = append(deltaEvents, e) | ||
| } | ||
| } | ||
| if len(deltaEvents) < 1 { | ||
| t.Error("Expected at least 1 delta event") | ||
| } | ||
|
|
||
| // Deltas should have content | ||
| for _, delta := range deltaEvents { | ||
| if delta.Data.DeltaContent == nil { | ||
| t.Error("Expected delta to have content") | ||
| } | ||
| } | ||
|
|
||
| // Should still have a final assistant.message | ||
| hasAssistantMessage := false | ||
| for _, e := range events { | ||
| if e.Type == "assistant.message" { | ||
| hasAssistantMessage = true | ||
| break | ||
| } | ||
| } | ||
| if !hasAssistantMessage { | ||
| t.Error("Expected a final assistant.message event") | ||
| } | ||
|
|
||
| // Deltas should come before the final message | ||
| firstDeltaIdx := -1 | ||
| lastAssistantIdx := -1 | ||
| for i, e := range events { | ||
| if e.Type == "assistant.message_delta" && firstDeltaIdx == -1 { | ||
| firstDeltaIdx = i | ||
| } | ||
| if e.Type == "assistant.message" { | ||
| lastAssistantIdx = i |
There was a problem hiding this comment.
The sessions created in this test file are not destroyed at the end of the subtests. Other Go E2E tests call Session.Destroy() to release resources and avoid cross-test interference; leaving sessions alive until client.ForceStop() can cause resource leaks and make later subtests observe unexpected session/event state. Consider adding a defer/t.Cleanup that calls Destroy() (and checks/logs errors) for each created session.
This issue also appears on line 130 of the same file.
| --- | ||
| description: Analyzes copilot-sdk issues to determine if a fix is needed in copilot-agent-runtime, then opens a linked issue and suggested-fix PR there | ||
| on: | ||
| issues: | ||
| types: [opened] | ||
| workflow_dispatch: | ||
| inputs: |
There was a problem hiding this comment.
The PR title/description describe a documentation-only fix in the custom agents guide, but this PR also adds/changes multiple E2E tests and snapshots across Node/Python/Go/.NET and introduces a new GitHub Actions workflow. Please update the PR title/description to reflect the broader scope, or split unrelated changes into separate PRs to keep review and rollback risk manageable.
Fix
The Python
on_permission_requestlambda in the custom agents guide only accepted one argument (lambda req:), but the handler signature requires two positional arguments(request, invocation). This would cause aTypeErrorat runtime.Before:
"on_permission_request": lambda req: {"kind": "approved"}After:
"on_permission_request": lambda req, inv: {"kind": "approved"}Review performed
Verified all code samples in the guide against actual SDK source across all 4 languages: