Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions actions/setup/js/copilot_harness.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@

"use strict";

const { COPILOT_TOOL_IMAGE_EXIT_CODE, isCopilotBYOK, createCopilotImageObserver } = require("./copilot_image_guard.cjs");

const { iterateJsonl } = require("./jsonl_helpers.cjs");
require("./shim.cjs");

Expand Down Expand Up @@ -2085,9 +2087,10 @@ async function main() {
// 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();

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.

[/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.

Copy link
Copy Markdown
Contributor Author

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

const verifyModel = !copilotSDKMode && (!process.env.GH_AW_PHASE || process.env.GH_AW_PHASE === "agent") && Boolean(process.env.COPILOT_MODEL);
let sessionId;
if (!copilotSDKMode && (verifyModel || durableSession)) {
if (!copilotSDKMode && (verifyModel || verifyImages || durableSession)) {
const continuedId = attempt === 0 ? durableSession?.id || undefined : useContinueOnRetry ? nativeModelSessionId : undefined;
const session = prepareCopilotModelSession(resolvedArgs, continuedId);
currentArgs = session.args;
Expand All @@ -2104,6 +2107,7 @@ async function main() {
observeStrategy(strategy);
const safeArgs = currentArgs.map((arg, i) => (currentArgs[i - 1] === "--prompt" || currentArgs[i - 1] === "-p" ? "<redacted>" : arg));
const modelObserver = !verifyModel ? { check: () => "" } : createCopilotModelObserver({ configuredModel: selectionConfiguredModel, resolvedModel: process.env.COPILOT_MODEL || "", sessionId, logger: log });
const imageObserver = verifyImages ? createCopilotImageObserver({ sessionId }) : { check: () => "" };
// Driver mode: run copilot_sdk_driver.cjs as a normal subprocess. The harness has
// already started the sidecar; the driver only opens an SDK client connection.
const result = await runProcess({
Expand All @@ -2114,7 +2118,12 @@ async function main() {
logArgs: safeArgs,
env: childEnv,
stdin: promptStdin,
runtimeGuard: { shouldTerminate: () => ({ terminate: Boolean(modelObserver.check()), reason: "copilot_model_verification" }) },
runtimeGuard: {
shouldTerminate: () => {
if (imageObserver.check()) return { terminate: true, reason: "copilot_tool_image_unsupported" };
return { terminate: Boolean(modelObserver.check()), reason: "copilot_model_verification" };
},
},
postResultWatchdog: safeOutputsPath
? {
shouldArm: () => hasTerminalSafeOutput(safeOutputsPath),
Expand All @@ -2124,6 +2133,15 @@ async function main() {
});
// The SDK driver observes its main session, records any substitution it refused and
// exits with COPILOT_MODEL_VERIFICATION_EXIT_CODE on any verification failure.
const imageDiagnostic = imageObserver.check(true);
if (imageDiagnostic || (copilotSDKMode && result.exitCode === COPILOT_TOOL_IMAGE_EXIT_CODE)) {
result.exitCode = 1;
result.runtimeGuardReason = "copilot_tool_image_unsupported";
if (imageDiagnostic) {
log(imageDiagnostic);
result.output += `\n${imageDiagnostic}`;
}
}
const modelSubstitution = copilotSDKMode ? readCopilotModelSubstitution() : modelObserver.check(true);
if (modelSubstitution || (copilotSDKMode && result.exitCode === COPILOT_MODEL_VERIFICATION_EXIT_CODE)) {
result.exitCode = 1;
Expand All @@ -2140,6 +2158,10 @@ async function main() {
return { ...result, strategy };
},
handleFailure: async ({ attempt, result }) => {
if (result.runtimeGuardReason === "copilot_tool_image_unsupported") {
result.failureClasses = ["copilot_tool_image_unsupported"];
return { action: "stop" };
}
if (result.runtimeGuardReason === "copilot_model_verification") {
result.failureClasses = ["copilot_model_verification"];
return { action: "stop" };
Expand Down
36 changes: 36 additions & 0 deletions actions/setup/js/copilot_image_guard.cjs
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() }) {

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 — 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 };
Loading
Loading