From 0f54c0df705eea9c334a8828808d45015bc5cdf7 Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 23 Jul 2026 13:09:08 +0200 Subject: [PATCH] fix(tools): autoqa consent handling from default off to opt-in --- packages/coding-agent/CHANGELOG.md | 3 + .../coding-agent/src/cli/grievances-cli.ts | 4 +- .../coding-agent/src/config/model-resolver.ts | 1 - .../src/config/settings-schema.ts | 5 +- packages/coding-agent/src/sdk.ts | 17 ++- .../src/tools/report-tool-issue.ts | 107 +++++++++++++----- .../test/settings-manager.test.ts | 12 +- .../test/tools/report-tool-issue.test.ts | 60 +++++++++- packages/utils/CHANGELOG.md | 4 + packages/utils/src/dirs.ts | 4 +- 10 files changed, 171 insertions(+), 46 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 102529b6a..7dcc2eba8 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -11,9 +11,12 @@ ### Changed - Adjusted retry fallback handling to recognize discovery-only and runtime extension providers, preventing spurious unknown-provider warnings. +- Restored Auto QA's ask-the-user default: `dev.autoqa` defaults to `true` again, so the first `xd://report_issue` write pops the consent dialog instead of the feature being silently off. Denying consent (or `dev.autoqa: false` / `PI_AUTO_QA=0`) fully disables prompt injection; an explicitly configured `dev.autoqa: true` overrides a past denial. Also restored the #1224 guarantee lost in the xd:// device consolidation: the grievance row is inserted only after consent resolves to granted (or `PI_AUTO_QA_PUSH=1`), so nothing touches the local database while consent is unset or denied. ### Fixed +- Fixed Auto QA grievance recording silently dropping every report since the xd:// device consolidation: `openAutoQaDb` treated the database file path (`~/.omp/autoqa.db`) as a directory and tried to open `autoqa.db/autoqa.db` inside it, which fails on legacy installs (the flat file blocks the directory) and fresh ones alike (SQLite does not create parent directories). Also restored the `busy_timeout` pragma dropped in the same refactor (#2421). Renamed `getAutoQaDbDir` to `getAutoQaDbPath` to match what it returns. + - Fixed the setup wizard hiding the selected row on short terminals (e.g. 24x80): the provider sign-in, theme, and web-search lists now fit their windows to the visible height, and decorative chrome (sign-in hint, theme mock preview) yields to the list when space is tight. - Fixed restored sessions replaying terminal aborted or errored assistant turns, which could repeatedly fail continuation from an assistant role; `/retry` now consults the persisted transcript so the failed turn remains retryable without re-entering provider context. - Fixed `get_available_models` and `set_model` RPCs racing background model discovery on cold start by awaiting the in-flight refresh before reading the registry. RPC/ACP clients that query the catalog or select a model immediately after session ready previously saw only statically-bundled models until discovery completed seconds later. diff --git a/packages/coding-agent/src/cli/grievances-cli.ts b/packages/coding-agent/src/cli/grievances-cli.ts index fdf675761..6e9d07a69 100644 --- a/packages/coding-agent/src/cli/grievances-cli.ts +++ b/packages/coding-agent/src/cli/grievances-cli.ts @@ -41,7 +41,7 @@ export async function listGrievances(options: ListGrievancesOptions): Promise runtimeDiscoveryPromise); - const availableModelsAfterRuntime = modelRegistry.getAll(); + const matchPreferences = getModelMatchPreferences(settings); const runtimeResolved = deferredModelPatterns.some(pattern => - availableModelsAfterRuntime.some(m => `${m.provider}/${m.id}` === pattern), + pattern.split(",").some(selector => { + const trimmedSelector = selector.trim(); + if (!trimmedSelector) return false; + const resolved = resolveCliModel({ + cliModel: trimmedSelector, + modelRegistry, + settings, + preferences: matchPreferences, + }); + return Boolean( + resolved.model || (resolved.configuredPatterns && resolved.configuredPatterns.length > 0), + ); + }), ); if (!runtimeResolved && modelRegistry.getDiscoverableProviders().length > 0) { await logger.time("resolveModelDiscoveryFallbackNonRuntime", () => @@ -2108,7 +2120,6 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} ); } const availableModels = modelRegistry.getAll(); - const matchPreferences = getModelMatchPreferences(settings); const expandedModelPatterns = deferredModelPatterns.flatMap(pattern => pattern.split(",").flatMap(selector => { const trimmedSelector = selector.trim(); diff --git a/packages/coding-agent/src/tools/report-tool-issue.ts b/packages/coding-agent/src/tools/report-tool-issue.ts index c9d9bf6d4..47736e08e 100644 --- a/packages/coding-agent/src/tools/report-tool-issue.ts +++ b/packages/coding-agent/src/tools/report-tool-issue.ts @@ -5,16 +5,21 @@ * `xd://report_issue`, and the system prompt tells the model to write * `: ` there when auto-QA is enabled. * - * Enabled by default; gated behind PI_AUTO_QA=1 / `dev.autoqa` so a user who - * flips the setting off short-circuits injection entirely. + * Enabled by default (`dev.autoqa` defaults to true); `PI_AUTO_QA=0` or an + * explicit `dev.autoqa: false` short-circuits injection entirely. When the + * user is only enabled by default (never configured `dev.autoqa` themselves), + * a persisted `dev.autoqaConsent: "denied"` also disables injection so a "No" + * in the consent dialog fully turns the feature off. * Records grievances to a local SQLite database; never throws from the device * dispatch path. * - * Before the first record lands, the user's consent is checked. If they've - * never been asked (`dev.autoqaConsent === "unset"`) the process-global - * consent handler — wired by `InteractiveMode` to a Yes/No popup — is invoked - * exactly once and the decision is persisted. Subsequent calls (including from - * subagents) read the cached decision without prompting. + * Nothing is written until consent resolves. If the user has never been asked + * (`dev.autoqaConsent === "unset"`) the process-global consent handler — + * wired by `InteractiveMode` to a Yes/No popup — is invoked exactly once and + * the decision is persisted; a denial (or dismissal) drops the pending report + * without touching the database. Subsequent calls (including from subagents) + * read the cached decision without prompting. `PI_AUTO_QA_PUSH=1` bypasses + * the dialog for headless environments. * * When the user grants consent, push is automatically active against the * bundled endpoint (`dev.autoqaPush.endpoint`, default `qa.omp.sh`). Each @@ -24,11 +29,13 @@ * the network and never throws. */ import { Database } from "bun:sqlite"; +import * as fs from "node:fs"; +import * as path from "node:path"; import type { AgentToolResult } from "@oh-my-pi/pi-agent-core"; import type { FetchImpl } from "@oh-my-pi/pi-ai"; import type { Component } from "@oh-my-pi/pi-tui"; import { Text } from "@oh-my-pi/pi-tui"; -import { $env, $flag, getAutoQaDbDir, getInstallId, logger, VERSION } from "@oh-my-pi/pi-utils"; +import { $env, $flag, getAutoQaDbPath, getInstallId, logger, VERSION } from "@oh-my-pi/pi-utils"; import type { Settings } from ".."; import type { Theme } from "../modes/theme/theme"; import { renderStatusLine, truncateToWidth } from "../tui"; @@ -88,8 +95,23 @@ function parseReportIssueBody(text: string): { tool: string; report: string } { throw new ToolError(`Invalid report format. ${reportIssueDeviceUsage()}`); } +/** + * Whether Auto-QA is active for this session. + * + * Precedence: `PI_AUTO_QA` env flag > explicit `dev.autoqa` setting > + * default-on unless the user previously denied consent. The denial veto only + * applies to the default: explicitly configuring `dev.autoqa: true` re-enables + * injection (recording still no-ops until consent is granted). + */ export function isAutoQaEnabled(settings?: Settings): boolean { - return $flag("PI_AUTO_QA", !!settings?.get("dev.autoqa")); + let fallback = false; + if (settings) { + const enabled = !!settings.get("dev.autoqa"); + fallback = settings.isConfigured("dev.autoqa") + ? enabled + : enabled && settings.get("dev.autoqaConsent") !== "denied"; + } + return $flag("PI_AUTO_QA", fallback); } // ─────────────────────────────────────────────────────────────────────────── @@ -233,15 +255,18 @@ let cachedDb: Database | null = null; /** * Open (or return the cached handle for) the auto-QA SQLite database at - * `~/.omp/agent/autoqa.db`, creating the schema lazily. Returns `null` when - * the agent data dir cannot be resolved. + * `~/.omp/autoqa.db` (XDG: `$XDG_DATA_HOME/omp/autoqa.db`), creating the + * schema lazily. Returns `null` when the path cannot be resolved or opened. */ export function openAutoQaDb(): Database | null { if (cachedDb) return cachedDb; - const dir = getAutoQaDbDir(); - if (!dir) return null; + const dbPath = getAutoQaDbPath(); + if (!dbPath) return null; try { - const db = new Database(`${dir}/autoqa.db`, { create: true }); + fs.mkdirSync(path.dirname(dbPath), { recursive: true }); + const db = new Database(dbPath, { create: true }); + // Install the busy handler BEFORE any lock-taking statement. See #2421. + db.run("PRAGMA busy_timeout = 5000"); db.exec(` CREATE TABLE IF NOT EXISTS grievances ( id INTEGER PRIMARY KEY AUTOINCREMENT, @@ -252,6 +277,17 @@ export function openAutoQaDb(): Database | null { created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, pushed INTEGER NOT NULL DEFAULT 0 ); + `); + // Legacy DBs (May 2026) predate `created_at`. ALTER TABLE only accepts + // constant defaults, so add it empty and backfill before the index below. + const hasCreatedAt = db.prepare("SELECT 1 FROM pragma_table_info('grievances') WHERE name = 'created_at'").get(); + if (!hasCreatedAt) { + db.exec(` + ALTER TABLE grievances ADD COLUMN created_at TEXT NOT NULL DEFAULT ''; + UPDATE grievances SET created_at = CURRENT_TIMESTAMP WHERE created_at = ''; + `); + } + db.exec(` CREATE INDEX IF NOT EXISTS grievances_pushed_created_at_idx ON grievances (pushed, created_at, id); `); @@ -471,23 +507,38 @@ export async function flushGrievances( } } -/** Record a grievance row and trigger the background consent/flush pipeline. */ -async function recordToolIssue(session: ToolSession, tool: string, report: string): Promise { +/** + * Most recently scheduled record pipeline. Never rejects (the pipeline + * swallows its own errors); retained so tests can await the fire-and-forget + * work deterministically via {@link __awaitAutoQaRecordPipelineForTests}. + */ +let lastRecordPipeline: Promise = Promise.resolve(); + +/** Test-only: await the last consent → insert → flush pipeline. */ +export function __awaitAutoQaRecordPipelineForTests(): Promise { + return lastRecordPipeline; +} + +/** + * Queue a grievance for recording. The consent → insert → flush pipeline is + * fire-and-forget: nothing is written until the user grants consent (or + * `PI_AUTO_QA_PUSH=1` forces headless recording), and the device result + * returns immediately so the model never waits on the dialog or the network. + */ +function recordToolIssue(session: ToolSession, tool: string, report: string): void { const canonicalTool = tool.startsWith("proxy_") ? tool.slice("proxy_".length) : tool; - const db = openAutoQaDb(); - if (!db) return; - db.prepare("INSERT INTO grievances (model, version, tool, report) VALUES (?, ?, ?, ?)").run( - session.getActiveModelString?.() ?? "unknown", - VERSION, - canonicalTool, - report, - ); - void (async () => { + const model = session.getActiveModelString?.() ?? "unknown"; + lastRecordPipeline = (async () => { try { - await resolveAutoQaConsent(session.settings); + if (!$flag("PI_AUTO_QA_PUSH") && !(await resolveAutoQaConsent(session.settings))) return; + const db = openAutoQaDb(); + if (!db) return; + db.prepare( + "INSERT INTO grievances (model, version, tool, report, created_at) VALUES (?, ?, ?, ?, CURRENT_TIMESTAMP)", + ).run(model, VERSION, canonicalTool, report); await flushGrievances(db, session.settings); } catch (error) { - logger.debug("autoqa post-insert pipeline failed", { error: String(error) }); + logger.debug("autoqa consent pipeline failed", { error: String(error) }); } })(); } @@ -504,7 +555,7 @@ export async function dispatchReportIssueDevice( try { if (isAutoQaEnabled(session.settings)) { const { tool, report } = parseReportIssueBody(text); - await recordToolIssue(session, tool, report); + recordToolIssue(session, tool, report); } } catch (error) { if (error instanceof ToolError) throw error; diff --git a/packages/coding-agent/test/settings-manager.test.ts b/packages/coding-agent/test/settings-manager.test.ts index 7dc13c676..a54b40cca 100644 --- a/packages/coding-agent/test/settings-manager.test.ts +++ b/packages/coding-agent/test/settings-manager.test.ts @@ -776,7 +776,7 @@ describe("Settings", () => { expect(settings.get("grep.enabled")).toBe(true); }); - it("migrates nested dev.autoqa.consent and todo.reminders.max without enabling parents", async () => { + it("migrates nested dev.autoqa.consent and todo.reminders.max without configuring parents", async () => { await writeSettings({ dev: { autoqa: { consent: "granted" } }, todo: { reminders: { max: 5 } }, @@ -785,7 +785,7 @@ describe("Settings", () => { const settings = await Settings.init({ cwd: projectDir, agentDir }); expect(settings.get("dev.autoqaConsent")).toBe("granted"); - expect(settings.get("dev.autoqa")).toBe(false); + expect(settings.get("dev.autoqa")).toBe(true); expect(settings.isConfigured("dev.autoqa")).toBe(false); expect(settings.get("todo.remindersMax")).toBe(5); expect(settings.get("todo.reminders")).toBe(true); @@ -798,7 +798,7 @@ describe("Settings", () => { const settings = await Settings.init({ cwd: projectDir, agentDir }); expect(settings.get("dev.autoqaConsent")).toBe("denied"); - expect(settings.get("dev.autoqa")).toBe(false); + expect(settings.isConfigured("dev.autoqa")).toBe(false); expect(settings.get("todo.remindersMax")).toBe(2); expect(settings.get("todo.reminders")).toBe(true); }); @@ -812,7 +812,7 @@ describe("Settings", () => { const settings = await Settings.init({ cwd: projectDir, agentDir }); expect(settings.get("dev.autoqaConsent")).toBe("granted"); - expect(settings.get("dev.autoqa")).toBe(false); + expect(settings.isConfigured("dev.autoqa")).toBe(false); expect(settings.get("todo.remindersMax")).toBe(9); expect(settings.get("todo.reminders")).toBe(true); }); @@ -837,7 +837,7 @@ describe("Settings", () => { "dev.autoqa.consent": consent, } as Partial>); expect(settings.get("dev.autoqaConsent")).toBe(consent); - expect(settings.get("dev.autoqa")).toBe(false); + expect(settings.isConfigured("dev.autoqa")).toBe(false); } }); @@ -867,7 +867,7 @@ describe("Settings", () => { const reloaded = await Settings.loadIsolated({ cwd: projectDir, agentDir }); expect(reloaded.get("dev.autoqaConsent")).toBe("denied"); - expect(reloaded.get("dev.autoqa")).toBe(false); + expect(reloaded.isConfigured("dev.autoqa")).toBe(false); expect(reloaded.get("todo.remindersMax")).toBe(1); expect(reloaded.get("todo.reminders")).toBe(true); }); 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 aa6a59f43..a5062aae2 100644 --- a/packages/coding-agent/test/tools/report-tool-issue.test.ts +++ b/packages/coding-agent/test/tools/report-tool-issue.test.ts @@ -4,6 +4,7 @@ import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; import * as reportIssue from "@oh-my-pi/pi-coding-agent/tools/report-tool-issue"; import { + __awaitAutoQaRecordPipelineForTests, __resetAutoQaConsentForTests, __resetAutoQaFlushStateForTests, dispatchReportIssueDevice, @@ -23,6 +24,7 @@ function openTempDb(): Database { version TEXT NOT NULL, tool TEXT NOT NULL, report TEXT NOT NULL, + created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, pushed INTEGER NOT NULL DEFAULT 0 ); `); @@ -105,6 +107,22 @@ describe("flushGrievances", () => { expect(isAutoQaEnabled(Settings.isolated({ "dev.autoqa": false }))).toBe(true); }); + it("enables auto QA by default with consent still unset", () => { + expect(isAutoQaEnabled(Settings.isolated())).toBe(true); + }); + + it("vetoes default-on auto QA once the user denied consent", () => { + expect(isAutoQaEnabled(Settings.isolated({ "dev.autoqaConsent": "denied" }))).toBe(false); + }); + + it("keeps explicitly enabled auto QA on despite denied consent", () => { + expect(isAutoQaEnabled(Settings.isolated({ "dev.autoqa": true, "dev.autoqaConsent": "denied" }))).toBe(true); + }); + + it("stays off when explicitly disabled", () => { + expect(isAutoQaEnabled(Settings.isolated({ "dev.autoqa": false }))).toBe(false); + }); + it("skips network when consent is missing and leaves rows intact", async () => { insertGrievance(db, "glob", "weird ordering"); const fetchSpy = vi.fn(async () => new Response("unexpected", { status: 200 })); @@ -336,12 +354,26 @@ describe("dispatchReportIssueDevice", () => { __resetAutoQaConsentForTests(); }); + /** Drain the fire-and-forget consent → insert → flush pipeline. */ + async function settlePipeline(): Promise { + await __awaitAutoQaRecordPipelineForTests(); + } + + /** Auto QA on, consent already granted, push disabled (empty endpoint). */ + function consentedSettings(): Settings { + return Settings.isolated({ + "dev.autoqa": true, + "dev.autoqaConsent": "granted", + "dev.autoqaPush.endpoint": "", + }); + } + it("records a grievance from `: ` text", async () => { Bun.env.PI_AUTO_QA = "1"; const db = openTempDb(); const openSpy = vi.spyOn(reportIssue, "openAutoQaDb").mockReturnValue(db); try { - const session = { settings: Settings.isolated({ "dev.autoqa": true }) } as ToolSession; + const session = { settings: consentedSettings() } as ToolSession; const { result, xdev } = await dispatchReportIssueDevice( session, "read: selector parse dropped trailing line", @@ -350,6 +382,7 @@ describe("dispatchReportIssueDevice", () => { expect(first?.type).toBe("text"); if (first?.type === "text") expect(first.text).toBe("Noted, thanks!"); expect(xdev.tool).toBe("report_issue"); + await settlePipeline(); expect(selectIds(db)).toHaveLength(1); const row = db.prepare("SELECT tool, report FROM grievances").get() as { tool: string; report: string }; expect(row).toEqual({ tool: "read", report: "selector parse dropped trailing line" }); @@ -364,8 +397,9 @@ describe("dispatchReportIssueDevice", () => { const db = openTempDb(); const openSpy = vi.spyOn(reportIssue, "openAutoQaDb").mockReturnValue(db); try { - const session = { settings: Settings.isolated({ "dev.autoqa": true }) } as ToolSession; + const session = { settings: consentedSettings() } as ToolSession; await dispatchReportIssueDevice(session, "grep\nreported matches include a deleted file"); + await settlePipeline(); const row = db.prepare("SELECT tool, report FROM grievances").get() as { tool: string; report: string }; expect(row).toEqual({ tool: "grep", report: "reported matches include a deleted file" }); } finally { @@ -374,6 +408,28 @@ describe("dispatchReportIssueDevice", () => { } }); + it("writes nothing while consent is unresolved", async () => { + Bun.env.PI_AUTO_QA = "1"; + const originalPush = Bun.env.PI_AUTO_QA_PUSH; + delete Bun.env.PI_AUTO_QA_PUSH; + const db = openTempDb(); + const openSpy = vi.spyOn(reportIssue, "openAutoQaDb").mockReturnValue(db); + try { + // Consent unset and no UI handler registered → resolves to false. + const session = { settings: Settings.isolated({ "dev.autoqa": true }) } as ToolSession; + const { result } = await dispatchReportIssueDevice(session, "read: selector parse dropped trailing line"); + const first = result.content[0]; + if (first?.type === "text") expect(first.text).toBe("Noted, thanks!"); + await settlePipeline(); + expect(selectIds(db)).toHaveLength(0); + } finally { + if (originalPush === undefined) delete Bun.env.PI_AUTO_QA_PUSH; + else Bun.env.PI_AUTO_QA_PUSH = originalPush; + openSpy.mockRestore(); + db.close(); + } + }); + it("rejects malformed body text with a usage hint", async () => { const session = { settings: Settings.isolated({ "dev.autoqa": true }) } as ToolSession; await expect(dispatchReportIssueDevice(session, "just a vague sentence")).rejects.toThrow( diff --git a/packages/utils/CHANGELOG.md b/packages/utils/CHANGELOG.md index d294fa107..5e43fbffa 100644 --- a/packages/utils/CHANGELOG.md +++ b/packages/utils/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Breaking Changes + +- Renamed `getAutoQaDbDir` to `getAutoQaDbPath` for accuracy; update any usage accordingly + ## [17.0.5] - 2026-07-18 ### Changed diff --git a/packages/utils/src/dirs.ts b/packages/utils/src/dirs.ts index cdddbbfdf..15895b1aa 100644 --- a/packages/utils/src/dirs.ts +++ b/packages/utils/src/dirs.ts @@ -632,8 +632,8 @@ export function getDocsRsCacheDir(): string { return dirs.rootSubdir("webcache", "cache"); } -/**Get AutoQa db directory */ -export function getAutoQaDbDir(): string { +/** Get the auto-QA grievances SQLite database path (~/.omp/autoqa.db; XDG: $XDG_DATA_HOME/omp/autoqa.db). */ +export function getAutoQaDbPath(): string { return dirs.rootSubdir("autoqa.db", "data"); } /**