Skip to content

fix: isolate copilot_sdk_driver test session state writes to prevent false-positive tool-denial issues #39940

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.

Sign up for GitHub

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

Merged
pelikhan merged 5 commits into from
Jun 18, 2026
+22 −3
Merged

fix: isolate copilot_sdk_driver test session state writes to prevent false-positive tool-denial issues #39940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c8f4e3d
Initial plan
Copilot Jun 18, 2026
a1bd2e4
Initial progress report (no changes yet)
Copilot Jun 18, 2026
d9ee22e
fix: isolate copilot_sdk_driver test session state writes to prevent …
Copilot Jun 18, 2026
9ae9bfb
fix: restore pre-existing GH_AW_SESSION_STATE_BASE_DIR in test teardown
Copilot Jun 18, 2026
d64b8bf
Merge remote-tracking branch 'origin/main' into copilot/aw-fix-jsweep…
Copilot Jun 18, 2026
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
18 changes: 17 additions & 1 deletion actions/setup/js/copilot_sdk_driver.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,26 @@
import { describe, it, expect, vi } from "vitest";
import { describe, it, expect, vi, beforeAll, afterAll } from "vitest";
import { createRequire } from "module";
import * as fs from "fs";
import * as os from "os";
import * as path from "path";

const require = createRequire(import.meta.url);
const { runWithCopilotSDK, parsePermissionConfigFromServerArgs } = require("./copilot_sdk_driver.cjs");

describe("copilot_sdk_driver.cjs", () => {
let testSessionStateDir;
let prevSessionStateDir;
beforeAll(() => {

@github-actions github-actions Bot Jun 18, 2026

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.

[/diagnose] The beforeAll/afterAll hooks correctly prevent test-generated events from polluting the production session-state path, but there's no assertion confirming the redirect actually works. Without a self-verifying test, a future refactor that breaks the isolation could go undetected until handle_agent_failure.cjs fires again.

💡 Consider a smoke-test assertion

After a runWithCopilotSDK call in an existing test, you could assert:

// At the end of an existing test that exercises a session with a known sessionId:
const entries = fs.readdirSync(testSessionStateDir, { recursive: true });
expect(entries.length).toBeGreaterThan(0); // confirms writes landed in the isolated dir

This turns the fix into a regression guard, not just an isolation helper.

prevSessionStateDir = process.env.GH_AW_SESSION_STATE_BASE_DIR;
testSessionStateDir = fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-test-session-state-"));
process.env.GH_AW_SESSION_STATE_BASE_DIR = testSessionStateDir;
});
afterAll(() => {
if (prevSessionStateDir === undefined) delete process.env.GH_AW_SESSION_STATE_BASE_DIR;
else process.env.GH_AW_SESSION_STATE_BASE_DIR = prevSessionStateDir;
if (testSessionStateDir) fs.rmSync(testSessionStateDir, { recursive: true, force: true });
});

describe("runWithCopilotSDK", () => {
it("disconnects session and stops client on success", async () => {
const disconnect = vi.fn().mockResolvedValue(undefined);
Expand Down
7 changes: 5 additions & 2 deletions actions/setup/js/copilot_sdk_session.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,11 @@ function extractPromptFromArgs(args) {
* RuntimeConnection: typeof import("@github/copilot-sdk").RuntimeConnection,
* approveAll: typeof import("@github/copilot-sdk").approveAll
* },
* sessionStateBaseDir?: string,

@github-actions github-actions Bot Jun 18, 2026

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.

[/tdd] sessionStateBaseDir is added to the JSDoc and function signature but the production call site in copilot_sdk_driver.cjs (line 121) doesn't forward it — making the parameter unreachable via the normal CLI entrypoint. No test exercises it directly either; all tests rely on the env var path instead.

💡 Options to consider
  1. Remove the parameter and rely solely on the env var (simpler API; the env var already satisfies test isolation needs).
  2. Keep the parameter but add a unit test that passes sessionStateBaseDir explicitly and asserts that event files land there — ensuring it doesn't silently regress:
it('respects explicit sessionStateBaseDir over env var', async () => {
  const explicitDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sdk-explicit-'));
  try {
    await runWithCopilotSDK({ ..., sessionStateBaseDir: explicitDir });
    // assert files written to explicitDir, not testSessionStateDir
  } finally {
    fs.rmSync(explicitDir, { recursive: true, force: true });
  }
});

Either way, the current state leaves the parameter untested and unused in production.

* }} options
* @returns {Promise<{exitCode: number, output: string, hasOutput: boolean, durationMs: number}>}
*/
async function runWithCopilotSDK({ sdkUri, prompt, logger, attempt = 0, model, connectionToken, provider, maxToolDenials, permissionConfig, coreLogger, sdkModule }) {
async function runWithCopilotSDK({ sdkUri, prompt, logger, attempt = 0, model, connectionToken, provider, maxToolDenials, permissionConfig, coreLogger, sdkModule, sessionStateBaseDir }) {

@github-actions github-actions Bot Jun 18, 2026

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.

sessionStateBaseDir parameter is added but never exercised in tests: the test suite bypasses it entirely, relying only on process.env.GH_AW_SESSION_STATE_BASE_DIR.

💡 Detail

The parameter was introduced specifically to give callers a clean injection point, but copilot_sdk_driver.test.cjs never passes it — beforeAll sets the env var instead. This means:

  1. The parameter override branch (sessionStateBaseDir ?? ...) is dead code from a test-coverage perspective.
  2. Any future breakage of the parameter path (e.g. a destructuring refactor that drops it) won't be caught.

The test should pass sessionStateBaseDir directly and stop relying on the env var, or the parameter should be removed if the env var is the intended permanent mechanism.

// Lazy-require to avoid loading the SDK when it is not needed.
// The SDK is large and has side-effects on import (worker threads, etc.).
const { CopilotClient, RuntimeConnection, approveAll } = sdkModule ?? require("@github/copilot-sdk");
Expand All @@ -106,7 +107,9 @@ async function runWithCopilotSDK({ sdkUri, prompt, logger, attempt = 0, model, c

// Session state directory — mirrors the target path used by unified_timeline.cjs.
// /tmp/gh-aw/sandbox/agent/logs/copilot-session-state/{sessionId}/events.jsonl
const sessionStateBase = path.join(os.tmpdir(), "gh-aw", "sandbox", "agent", "logs", "copilot-session-state");
// GH_AW_SESSION_STATE_BASE_DIR may be set in tests to redirect writes to an isolated directory.

@github-actions github-actions Bot Jun 18, 2026

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.

[/diagnose] The comment frames GH_AW_SESSION_STATE_BASE_DIR as a test-only mechanism, but the env var is a legitimate runtime override too — useful for CI job isolation, ephemeral sandboxes, or any scenario where the caller can't pass sessionStateBaseDir directly. Consider rewording to make its general-purpose intent clear.

💡 Suggested rewording 
// Override session-state base directory (e.g. for test isolation or CI sandboxing).
const sessionStateBase = sessionStateBaseDir ?? process.env.GH_AW_SESSION_STATE_BASE_DIR ?? defaultSessionStateBase;

Coupling the comment to "tests" may mislead future readers who want to use it in other contexts.

const defaultSessionStateBase = path.join(os.tmpdir(), "gh-aw", "sandbox", "agent", "logs", "copilot-session-state");
const sessionStateBase = sessionStateBaseDir ?? process.env.GH_AW_SESSION_STATE_BASE_DIR ?? defaultSessionStateBase;

@github-actions github-actions Bot Jun 18, 2026

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.

Production code now has an undocumented env-var override that can silently lose all audit events: GH_AW_SESSION_STATE_BASE_DIR is described as a test mechanism but lives in the unconditional production fallback chain.

💡 Detail and suggested fix

If this env var is set in any real agent environment — by accident, CI variable leakage, or a misconfigured workflow — all session-state JSONL writes are silently redirected to an arbitrary path. handle_agent_failure.cjs reads the default path and finds nothing, turning every real tool-denial event into a silent false-negative. No warning is emitted.

The cleaner design is to keep the env-var lookup only in tests, pass the override via the sessionStateBaseDir parameter, and remove process.env.GH_AW_SESSION_STATE_BASE_DIR from this fallback chain:

// Production code — no env var
const sessionStateBase = sessionStateBaseDir ?? defaultSessionStateBase;

In the test harness, pass the dir explicitly:

await runWithCopilotSDK({ ..., sessionStateBaseDir: testSessionStateDir });

If keeping the env var, at minimum emit a visible log line when it overrides the default:

if (process.env.GH_AW_SESSION_STATE_BASE_DIR && !sessionStateBaseDir) {
  logger?.warn?.(`[session] GH_AW_SESSION_STATE_BASE_DIR overrides default session state path`);
}


/** @type {ReadonlyArray<NonNullable<import("@github/copilot-sdk").CopilotClientOptions["logLevel"]>>} */
const VALID_LOG_LEVELS = ["none", "error", "warning", "info", "debug", "all"];
Expand Down
Loading