From 6efe86c07b0ad7f242d8ca81f6d5c2c637d82bb4 Mon Sep 17 00:00:00 2001 From: can1357 Date: Sat, 6 Jun 2026 16:30:09 +0200 Subject: [PATCH] fix(coding-agent): honored boolean env flag overrides over settings - Made PI_INTENT_TRACING, PI_AUTO_QA, PI_PY, and PI_JS take precedence when set. - Fell back to config when the env flag is unset instead of ORing. - Surfaced PI_PY=0/PI_JS=0 in the disabled-backend error messages. --- packages/coding-agent/CHANGELOG.md | 4 ++ packages/coding-agent/src/sdk.ts | 2 +- .../coding-agent/src/tools/eval-backends.ts | 23 +++------- packages/coding-agent/src/tools/eval.ts | 9 ++-- .../src/tools/report-tool-issue.ts | 2 +- .../test/tools/eval-fallback.test.ts | 44 ++++++++++++++++++- .../test/tools/report-tool-issue.test.ts | 31 ++++++++++++- 7 files changed, 90 insertions(+), 25 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 0960b0150..21058af16 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed boolean environment flag overrides that were ORed with settings, so `PI_INTENT_TRACING=0`, `PI_AUTO_QA=0`, and per-backend eval flags now take precedence when present while falling back to config when unset. + ## [15.9.67] - 2026-06-06 ### Added diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index eb129dcd1..70bbcb58c 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -1723,7 +1723,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} const repeatToolDescriptions = settings.get("repeatToolDescriptions"); const eagerTasks = settings.get("task.eager"); - const intentField = settings.get("tools.intentTracing") || $flag("PI_INTENT_TRACING") ? INTENT_FIELD : undefined; + const intentField = $flag("PI_INTENT_TRACING", settings.get("tools.intentTracing")) ? INTENT_FIELD : undefined; const rebuildSystemPrompt = async ( toolNames: string[], tools: Map, diff --git a/packages/coding-agent/src/tools/eval-backends.ts b/packages/coding-agent/src/tools/eval-backends.ts index 7f9cb6e7e..7839387d9 100644 --- a/packages/coding-agent/src/tools/eval-backends.ts +++ b/packages/coding-agent/src/tools/eval-backends.ts @@ -1,4 +1,4 @@ -import { $env, $flag } from "@oh-my-pi/pi-utils"; +import { $flag } from "@oh-my-pi/pi-utils"; import type { ToolSession } from "."; export interface EvalBackendsAllowance { @@ -6,21 +6,6 @@ export interface EvalBackendsAllowance { js: boolean; } -/** - * Parse PI_PY / PI_JS environment variables. Each is a boolean flag; unset - * means "not specified, defer to settings". Returns null when neither is set - * so the caller can fall through to `readEvalBackendsAllowance` per key. - */ -function getEvalBackendsFromEnv(): EvalBackendsAllowance | null { - const pyEnv = $env.PI_PY; - const jsEnv = $env.PI_JS; - if (pyEnv === undefined && jsEnv === undefined) return null; - return { - python: pyEnv === undefined ? true : $flag("PI_PY"), - js: jsEnv === undefined ? true : $flag("PI_JS"), - }; -} - /** Read per-backend allowance from settings (defaults true). */ export function readEvalBackendsAllowance(session: ToolSession): EvalBackendsAllowance { return { @@ -34,5 +19,9 @@ export function readEvalBackendsAllowance(session: ToolSession): EvalBackendsAll * override the per-key settings; otherwise settings (defaults true) win. */ export function resolveEvalBackends(session: ToolSession): EvalBackendsAllowance { - return getEvalBackendsFromEnv() ?? readEvalBackendsAllowance(session); + const settings = readEvalBackendsAllowance(session); + return { + python: $flag("PI_PY", settings.python), + js: $flag("PI_JS", settings.js), + }; } diff --git a/packages/coding-agent/src/tools/eval.ts b/packages/coding-agent/src/tools/eval.ts index 31336d9e2..cb8a6b80d 100644 --- a/packages/coding-agent/src/tools/eval.ts +++ b/packages/coding-agent/src/tools/eval.ts @@ -130,11 +130,12 @@ function timeoutSecondsFromMs(timeoutMs: number): number { } async function resolveBackend(session: ToolSession, language: EvalLanguage): Promise { - const allowPy = (session.settings.get("eval.py") as boolean | undefined) ?? true; - const allowJs = (session.settings.get("eval.js") as boolean | undefined) ?? true; + const backends = resolveEvalBackends(session); + const allowPy = backends.python; + const allowJs = backends.js; if (language === "python") { - if (!allowPy) throw new ToolError("Python backend is disabled (eval.py = false)."); + if (!allowPy) throw new ToolError("Python backend is disabled (PI_PY=0 or eval.py = false)."); if (!(await pythonBackend.isAvailable(session))) { throw new ToolError( 'Python backend is unavailable in this session. Pass language: "js" or install the python kernel.', @@ -142,7 +143,7 @@ async function resolveBackend(session: ToolSession, language: EvalLanguage): Pro } return { backend: pythonBackend }; } - if (!allowJs) throw new ToolError("JavaScript backend is disabled (eval.js = false)."); + if (!allowJs) throw new ToolError("JavaScript backend is disabled (PI_JS=0 or eval.js = false)."); return { backend: jsBackend }; } diff --git a/packages/coding-agent/src/tools/report-tool-issue.ts b/packages/coding-agent/src/tools/report-tool-issue.ts index 0526612ed..e723ff491 100644 --- a/packages/coding-agent/src/tools/report-tool-issue.ts +++ b/packages/coding-agent/src/tools/report-tool-issue.ts @@ -41,7 +41,7 @@ function buildReportToolIssueParams(activeBuiltinNames: readonly string[]) { } export function isAutoQaEnabled(settings?: Settings): boolean { - return $flag("PI_AUTO_QA") || !!settings?.get("dev.autoqa"); + return $flag("PI_AUTO_QA", !!settings?.get("dev.autoqa")); } // ─────────────────────────────────────────────────────────────────────────── diff --git a/packages/coding-agent/test/tools/eval-fallback.test.ts b/packages/coding-agent/test/tools/eval-fallback.test.ts index 112fd43f8..083b5faa6 100644 --- a/packages/coding-agent/test/tools/eval-fallback.test.ts +++ b/packages/coding-agent/test/tools/eval-fallback.test.ts @@ -1,10 +1,21 @@ -import { afterEach, describe, expect, it, vi } from "bun:test"; +import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import * as evalIndex from "@oh-my-pi/pi-coding-agent/eval"; import * as pyKernel from "@oh-my-pi/pi-coding-agent/eval/py/kernel"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; import { EvalTool } from "@oh-my-pi/pi-coding-agent/tools/eval"; +import { resolveEvalBackends } from "@oh-my-pi/pi-coding-agent/tools/eval-backends"; +let originalPiPy: string | undefined; +let originalPiJs: string | undefined; + +function restoreEnv(name: "PI_PY" | "PI_JS", value: string | undefined): void { + if (value === undefined) { + delete Bun.env[name]; + return; + } + Bun.env[name] = value; +} function makeSession(settings = Settings.isolated()): ToolSession { return { cwd: "/tmp/eval-test", @@ -29,8 +40,17 @@ const mockResult = { }; describe("EvalTool language dispatch", () => { + beforeEach(() => { + originalPiPy = Bun.env.PI_PY; + originalPiJs = Bun.env.PI_JS; + delete Bun.env.PI_PY; + delete Bun.env.PI_JS; + }); + afterEach(() => { vi.restoreAllMocks(); + restoreEnv("PI_PY", originalPiPy); + restoreEnv("PI_JS", originalPiJs); }); it('dispatches to the JS backend when cell.language === "js"', async () => { @@ -100,4 +120,26 @@ describe("EvalTool language dispatch", () => { }), ).rejects.toThrow(/eval\.js = false/); }); + + it("uses settings for eval backends whose env flag is unset", () => { + Bun.env.PI_PY = "1"; + const settings = Settings.isolated(); + settings.set("eval.py", false); + settings.set("eval.js", false); + + expect(resolveEvalBackends(makeSession(settings))).toEqual({ python: true, js: false }); + }); + + it("lets PI_JS disable js execution even when eval.js is enabled", async () => { + Bun.env.PI_JS = "0"; + const settings = Settings.isolated(); + settings.set("eval.js", true); + const tool = new EvalTool(makeSession(settings)); + + await expect( + tool.execute("call-js-env-disabled", { + cells: [{ language: "js", code: "const x = 1;" }], + }), + ).rejects.toThrow(/PI_JS=0/); + }); }); diff --git a/packages/coding-agent/test/tools/report-tool-issue.test.ts b/packages/coding-agent/test/tools/report-tool-issue.test.ts index 60d41632c..dcbee11be 100644 --- a/packages/coding-agent/test/tools/report-tool-issue.test.ts +++ b/packages/coding-agent/test/tools/report-tool-issue.test.ts @@ -1,7 +1,11 @@ import { Database } from "bun:sqlite"; import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; -import { __resetAutoQaFlushStateForTests, flushGrievances } from "@oh-my-pi/pi-coding-agent/tools/report-tool-issue"; +import { + __resetAutoQaFlushStateForTests, + flushGrievances, + isAutoQaEnabled, +} from "@oh-my-pi/pi-coding-agent/tools/report-tool-issue"; import * as piUtils from "@oh-my-pi/pi-utils"; import { hookFetch } from "@oh-my-pi/pi-utils"; @@ -57,11 +61,23 @@ function pushSettings(overrides: Record = {}): Settings { }); } +let originalPiAutoQa: string | undefined; + +function restoreAutoQaEnv(): void { + if (originalPiAutoQa === undefined) { + delete Bun.env.PI_AUTO_QA; + return; + } + Bun.env.PI_AUTO_QA = originalPiAutoQa; +} + describe("flushGrievances", () => { let db: Database; beforeEach(() => { __resetAutoQaFlushStateForTests(); + originalPiAutoQa = Bun.env.PI_AUTO_QA; + delete Bun.env.PI_AUTO_QA; vi.spyOn(piUtils, "getInstallId").mockReturnValue("11111111-2222-3333-4444-555555555555"); db = openTempDb(); }); @@ -69,9 +85,22 @@ describe("flushGrievances", () => { afterEach(() => { vi.restoreAllMocks(); __resetAutoQaFlushStateForTests(); + restoreAutoQaEnv(); db.close(); }); + it("lets PI_AUTO_QA=false disable auto QA when the setting is enabled", () => { + Bun.env.PI_AUTO_QA = "0"; + + expect(isAutoQaEnabled(Settings.isolated({ "dev.autoqa": true }))).toBe(false); + }); + + it("lets PI_AUTO_QA=true enable auto QA when the setting is disabled", () => { + Bun.env.PI_AUTO_QA = "1"; + + expect(isAutoQaEnabled(Settings.isolated({ "dev.autoqa": false }))).toBe(true); + }); + it("skips network when consent is missing and leaves rows intact", async () => { insertGrievance(db, "find", "weird ordering"); const fetchSpy = vi.fn(() => new Response("unexpected", { status: 200 }));