fix(tools): autoqa consent handling from default off to opt-in

This commit is contained in:
can1357
2026-07-23 13:09:08 +02:00
parent f833810d0e
commit 0f54c0df70
10 changed files with 171 additions and 46 deletions
+3
View File
@@ -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.
@@ -41,7 +41,7 @@ export async function listGrievances(options: ListGrievancesOptions): Promise<vo
console.log("[]");
} else {
console.log(
chalk.dim("No grievances database found. Enable auto-QA with PI_AUTO_QA=1 or the dev.autoqa setting."),
chalk.dim("No grievances database found. Auto-QA has not recorded any reports yet (or was disabled)."),
);
}
return;
@@ -110,7 +110,7 @@ export async function cleanGrievances(options: CleanGrievancesOptions): Promise<
console.log(JSON.stringify({ deleted: 0 }));
} else {
console.log(
chalk.dim("No grievances database found. Enable auto-QA with PI_AUTO_QA=1 or the dev.autoqa setting."),
chalk.dim("No grievances database found. Auto-QA has not recorded any reports yet (or was disabled)."),
);
}
return;
@@ -782,7 +782,6 @@ function parseModelPatternWithContext(
options?: { allowInvalidThinkingSelectorFallback?: boolean },
): ParsedModelResult {
// Exact match on the full pattern first (no fuzzy): a literal id that
// contains a colon (`coding-router:max`) wins over any suffix split.
const exactMatch = matchModel(pattern, availableModels, context, { exactOnly: true });
if (exactMatch) {
return { model: exactMatch, thinkingLevel: undefined, warning: undefined, explicitThinkingLevel: false };
@@ -5069,12 +5069,13 @@ export const SETTINGS_SCHEMA = {
"dev.autoqa": {
type: "boolean",
default: false,
default: true,
ui: {
tab: "tools",
group: "Developer",
label: "Auto QA",
description: "Enable automated tool issue reporting (report_tool_issue) for all agents",
description:
"Automated tool issue reporting (xd://report_issue). On by default; the first report asks for consent, and denying it disables reporting until re-enabled explicitly",
},
},
+14 -3
View File
@@ -2098,9 +2098,21 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {}
// fresh cache written by the awaited pass, closing the double-fetch
// window.
await logger.time("resolveModelDiscoveryDeferredRetry", () => 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();
@@ -5,16 +5,21 @@
* `xd://report_issue`, and the system prompt tells the model to write
* `<tool>: <concise description>` 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<void> {
/**
* 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<void> = Promise.resolve();
/** Test-only: await the last consent → insert → flush pipeline. */
export function __awaitAutoQaRecordPipelineForTests(): Promise<void> {
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;
@@ -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<Record<SettingPath, unknown>>);
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);
});
@@ -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<void> {
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 `<tool>: <report>` 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(
+4
View File
@@ -2,6 +2,10 @@
## [Unreleased]
### Breaking Changes
- Renamed `getAutoQaDbDir` to `getAutoQaDbPath` for accuracy; update any usage accordingly
## [17.0.5] - 2026-07-18
### Changed
+2 -2
View File
@@ -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");
}
/**