Repository navigation
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Specification Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
No actionable correctness, performance, or security issues were identified in the changed lines. Static review only; the delegated specialist could not access the pre-fetched diff and its output was discarded.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Reviewed head 7f9a1e4 against docs/src/content/docs/specs/copilot-sdk-driver-specification.md v1.0.2. No existing MUST violation was demonstrated; the PR is an intentional fail-closed behavior change.
Proposed specification synchronization (not applied): bump the document to v1.1.0 (Draft, 2026-11-06) and update §3.2, §6.2, and §7.2 to require invocation-scoped validation of successful tool.execution_complete results: every result.binaryResultsForLlm image entry must have non-empty inline data; otherwise fail closed, abort the SDK session with its dedicated image-validation exit code, stop CLI retries, emit a diagnostic, and never classify the run as successful. Add a v1.1.0 change-log entry recording these additions. Preserve all existing token isolation, permission, cleanup, and content-filter obligations.
Evidence: actions/setup/js/copilot_image_guard.cjs; actions/setup/js/copilot_harness.cjs; actions/setup/js/copilot_sdk_session.cjs; copilot_image_guard.test.cjs; and the Copilot engine documentation. Static review only; tests were not run per review contract.
Generated by PR Specification Reviewer for #75379 · copilot · gpt50 · ~4 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review-specs to run again
There was a problem hiding this comment.
What this change does: This adds a BYOK image-capability guard to both Copilot CLI and SDK paths. It tails invocation event logs for CLI and aborts SDK sessions when a successful tool result has an image without inline data.
Should fix:
-
Share the session-event tailer (
actions/setup/js/copilot_image_guard.cjs:L29-L79)- What this is: The new observer duplicates the incremental session-file tailing already in
createCopilotModelObserverinactions/setup/js/copilot_model_selection.cjs. - Problem: The two copies independently handle offsets, truncation, partial JSONL records, and read failures.
- Fix: Extract one shared invocation-event observer and pass the image predicate to it.
- If we skip it: A future change to session evidence rules must be made in both observers, while most of this new function provides no image-specific behavior.
- What this is: The new observer duplicates the incremental session-file tailing already in
Verdict: fix 1 first.
Lean: -40 lines possible.
Generated by ✂️ Ponytail Reviewer for #75379 · codex · routed: gpt56 max · ~5 AIC · ⌖ ~1 AIC · ⊞ 118
Comment /ponytail to run again
| * Inspect only newly appended events for this CLI invocation, including subagents. | ||
| * @param {{sessionId: string, sessionDirs?: string[]}} options | ||
| */ | ||
| function createCopilotImageObserver({ sessionId, sessionDirs = copilotSessionStateDirectories() }) { |
There was a problem hiding this comment.
Should fix — What this is: This observer reimplements the incremental session-file tailing already in createCopilotModelObserver in actions/setup/js/copilot_model_selection.cjs. Problem: The two copies independently handle offsets, truncation, partial JSONL records, and read failures, so a change to session evidence rules must be kept in sync in both paths. Fix: Extract one shared invocation-event observer and pass the image predicate to it. If we skip it: The guard grows another bespoke copy whenever session logging changes, with most of this 51-line loop providing no image-specific behavior.
There was a problem hiding this comment.
Addressed in 1fb796f. Both model verification and image detection now use one invocation-event observer for offsets, truncation, partial records, session ownership, and read failures. Image-specific detection is supplied as a callback. Regression coverage includes truncation and split records.
There was a problem hiding this comment.
🟡 Changes recommended
An unsupported image completion during disconnect or shutdown can arrive after the success result is constructed and bypass post-cleanup error handling, incorrectly allowing exit code 0.
1 open issue
What changed in this PR
This PR adds explicit failure handling for unsupported tool-returned images in gh-aw’s Copilot BYOK runtime. It addresses misleading success reports without attempting to repair upstream image forwarding.
Changes:
- Detect successful image results that lack inline data and monitor invocation-scoped CLI events.
- Abort SDK sessions on detection and prevent retries or success fallbacks.
- Add regression coverage and document the limitation and guard boundaries.
| File | Description |
|---|---|
docs/src/content/docs/reference/engines/copilot.md |
Documents image limitations, guard scope, and upstream ownership. |
actions/setup/js/copilot_sdk_session.cjs |
Adds SDK image detection, abort handling, and failure propagation. |
actions/setup/js/copilot_image_guard.test.cjs |
Tests detection, event monitoring, and CLI/SDK failure behavior. |
actions/setup/js/copilot_image_guard.cjs |
Implements shared image detection and invocation-scoped event monitoring. |
actions/setup/js/copilot_harness.cjs |
Integrates CLI monitoring and stops retries after image failures. |
Review effort: Balanced (auto)
| // timeout intervenes. denialGuardPromise never settles unless the guard fires. | ||
| const result = await Promise.race([session.sendAndWait({ prompt }, sendTimeoutMs), denialGuardPromise]); | ||
|
|
||
| if (unsupportedImageError) throw unsupportedImageError; |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /codebase-design, and /tdd. Requesting changes: the SDK path can still exit 0 after an unsupported image, and the BYOK-wide change to CLI argument handling is untested.
📋 Key Themes & Highlights
Key Themes
- Fail-open SDK path: an image event delivered during disconnect, after
sendAndWaitresolves, setsunsupportedImageErrorbut is never checked. I reproducedexitCode: 0with a stub SDK. - BYOK-wide side effect:
verifyImagesmakesprepareCopilotModelSessionrun for every BYOK CLI run. That rejects user--continue,--connect, and non-UUID--resume, and switches retries to exact-UUID--session-id. None of this is covered by a test. - Unverified test status: I could not run the test suite.
npm cifailed on a TLS certificate error from the registry, so vitest is not installed. Findings are from static reading and one targeted repro. Treat the test status as unverified.
Positive Highlights
- ✅ The fail-closed observer design is sound: read errors produce a diagnostic rather than a pass.
- ✅ Partial JSONL records are handled, and the
denialGuardPromiserejection path has a safe.catch. - ✅ Session-ID validation blocks path traversal, and the docs section clearly states the upstream ownership.
- ✅ Exit code 80 is wired through
handleFailureas non-retryable.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~14 AIC · ⌖ ~1 AIC · ⊞ 5.4K
Comment /matt to run again
| // Verified native retries resume the exact UUID; other CLI paths retain --continue. | ||
| let currentArgs = !copilotSDKMode && attempt > 0 && useContinueOnRetry ? [...resolvedArgs, "--continue"] : resolvedArgs; | ||
| if (!copilotSDKMode && durableSession?.id && attempt > 0 && !useContinueOnRetry) throw new Error("Durable Copilot continuation cannot restart a recorded session as a fresh conversation"); | ||
| const verifyImages = !copilotSDKMode && isCopilotBYOK(); |
There was a problem hiding this comment.
[/codebase-design] This runs prepareCopilotModelSession for every BYOK CLI run, not only runs that need the image guard. Three effects follow: user --continue or --connect and non-UUID --resume now hard-fail, retries switch from --continue to --session-id on the same UUID, and no test covers that retry change. Confirm that exact-UUID resume matches Copilot CLI behavior, or scope this to the image-guard case.
💡 Suggested scope
Gate verifyImages on whether the run actually needs the observer, or keep the BYOK retry strategy unchanged and only inject --session-id when the image observer is active. Document the --continue rejection in the copilot engine reference.
@copilot please address this.
There was a problem hiding this comment.
Verified against the Copilot CLI reference: --session-id <UUID> resumes an existing exact ID, creating a new session only when no match exists. Retained that retry behavior. All BYOK runs need invocation-scoped image observation; 1fb796f documents the resulting selector restrictions and adds exact-argument retry, explicit-resume, rejection, and native pass-through tests. Reference: https://docs.github.com/en/copilot/reference/copilot-cli-reference/cli-command-reference#command-line-options
| // Native shutdown records arrive while disconnecting, after the session result is known. | ||
| const verificationFailure = verification.modelObserver && verification.sessionId ? verification.modelObserver.finalize(verification.sessionId) : ""; | ||
| if (!verificationFailure) return result; | ||
| if (!verificationFailure || result.exitCode === COPILOT_TOOL_IMAGE_EXIT_CODE) return result; |
There was a problem hiding this comment.
[/diagnosing-bugs] This is a fail-open path. An unsupported tool image that arrives during disconnect, after sendAndWait resolves, sets unsupportedImageError and emits session.error. Nothing rechecks that flag before this return, so the run still exits 0. I reproduced this with a stub SDK whose disconnect emits the image event: the result was exitCode: 0.
💡 Suggested fix
Check unsupportedImageError after the session finishes and before returning, and map it to COPILOT_TOOL_IMAGE_EXIT_CODE. Add a regression test where the image event is delivered during disconnect.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 1fb796f. Image diagnostics survive cleanup and are rechecked before final return, forcing exit code 80 even when delivered during disconnect or client.stop. Cleanup diagnostics remain writable while ordinary late callbacks stay ignored. Both cleanup paths have passing regressions.
| data: { toolCallId: "view-call", success: true, result: { content: "Viewed image file successfully.", binaryResultsForLlm: [{ type: "image", mimeType: "image/png", byteLength: 21942, assetId: "sha256:test" }] } }, | ||
| }; | ||
|
|
||
| describe("Copilot BYOK tool image guard", () => { |
There was a problem hiding this comment.
[/tdd] No test covers the BYOK-wide behavior changes: --continue retries now become exact-UUID --session-id retries, and user --continue or non-UUID --resume now throw for every BYOK run. There is also no test for an image event delivered during SDK disconnect.
💡 Suggested tests
Write these red first. (1) A BYOK CLI retry asserts the exact args sequence. (2) A user --continue in BYOK mode asserts the intended rejection. (3) An image event during disconnect asserts exit code 80. Then make them green.
@copilot please address this.
There was a problem hiding this comment.
Added regressions in 1fb796f for exact BYOK retry arguments, full-UUID resume, --continue/--connect/name/prefix rejection, and native pass-through. Disconnect and client.stop image tests reproduced exit code 0 before the fix and now require 80. All 26 Copilot suites pass: 1,077 tests, one skipped.
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 7f9a1e4
|
Assignment running: merge latest main with Sous-chef requested-head: 7f9a1e4 |
…nt-inference-request Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Copilot’s BYOK request assembly can omit tool-returned images while
viewreports success, leaving the model without pixels. This change surfaces that limitation rather than allowing a misleading successful run.copilot_tool_image_unsupported; prevent retries and success fallbacks.view, MCP tools, and subagents while preserving text-only results, inline images, and native routing.