Repository navigation
Fail explicitly on unsupported Copilot BYOK tool images #75379
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.
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
Changes from all commits
ae26184
7f9a1e4
e5e7326
1fb796f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| // @ts-check | ||
| "use strict"; | ||
|
|
||
| const { createCopilotInvocationObserver } = require("./copilot_invocation_observer.cjs"); | ||
| const { copilotSessionStateDirectories } = require("./copilot_model_selection.cjs"); | ||
|
|
||
| const COPILOT_TOOL_IMAGE_EXIT_CODE = 80; | ||
| const COPILOT_TOOL_IMAGE_DIAGNOSTIC = | ||
| "Copilot tool image input is unsupported on this BYOK inference path: an image result has no inline image data and may be silently dropped. Refusing to report success without visual content. See https://github.com/github/copilot-cli/issues/4600."; | ||
|
|
||
| /** @param {NodeJS.ProcessEnv} [env] */ | ||
| function isCopilotBYOK(env = process.env) { | ||
| return Boolean(env.COPILOT_PROVIDER_BASE_URL || env.COPILOT_API_KEY); | ||
| } | ||
|
|
||
| /** @param {any} event */ | ||
| function hasUnsupportedCopilotToolImage(event) { | ||
| const binaries = event?.data?.result?.binaryResultsForLlm; | ||
| return event?.type === "tool.execution_complete" && event.data.success === true && Array.isArray(binaries) && binaries.some(binary => binary?.type === "image" && (typeof binary.data !== "string" || !binary.data)); | ||
| } | ||
|
|
||
| /** | ||
| * Inspect only newly appended events for this CLI invocation, including subagents. | ||
| * @param {{sessionId: string, sessionDirs?: string[]}} options | ||
| */ | ||
| function createCopilotImageObserver({ sessionId, sessionDirs = copilotSessionStateDirectories() }) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should fix — What this is: This observer reimplements the incremental session-file tailing already in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| if (!/^[a-z0-9_-]+$/i.test(sessionId)) throw new Error("Copilot image guard requires an exact invocation session ID"); | ||
| return createCopilotInvocationObserver({ | ||
| sessionId, | ||
| sessionDirs, | ||
| onEvent: event => (hasUnsupportedCopilotToolImage(event) ? COPILOT_TOOL_IMAGE_DIAGNOSTIC : ""), | ||
| onError: () => "Copilot tool image capability verification failed; refusing to report an unverified run as successful.", | ||
| }); | ||
| } | ||
|
|
||
| module.exports = { COPILOT_TOOL_IMAGE_EXIT_CODE, COPILOT_TOOL_IMAGE_DIAGNOSTIC, isCopilotBYOK, hasUnsupportedCopilotToolImage, createCopilotImageObserver }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design] This runs
prepareCopilotModelSessionfor every BYOK CLI run, not only runs that need the image guard. Three effects follow: user--continueor--connectand non-UUID--resumenow hard-fail, retries switch from--continueto--session-idon 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
verifyImageson whether the run actually needs the observer, or keep the BYOK retry strategy unchanged and only inject--session-idwhen the image observer is active. Document the--continuerejection in the copilot engine reference.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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