fix(coding-agent): renamed settings keys to avoid nested-value lookup collisions

- Updated schema and runtime paths to use `dev.autoqaConsent` and `todo.remindersMax`, including auto-QA consent reads/persistence and todo reminder limit checks.
- Adjusted settings expectations so obsolete BM25-discovery keys were dropped on load and `tools.xdev` now kept its default unless explicitly set.
- Added/updated tests for the setting key migration and refreshed issue-consent flows, plus a new `refreshMCPTools` test for steered `xdev-mount-notice` updates without prompt rebuilds.
This commit is contained in:
can1357
2026-07-15 19:08:37 +02:00
parent bf3764fa4d
commit 46ad908245
13 changed files with 93 additions and 44 deletions
+5 -2
View File
@@ -19,6 +19,7 @@
- Added per-agent prewalk for subagents, featuring a `prewalk` frontmatter field, a `task.agentPrewalk` settings override toggled from the `/agents` dashboard, and a `task.prewalk` boolean (default off) to arm the bundled generic `task` agent.
### Changed
- Renamed `"dev.autoqa.consent"` to `"dev.autoqaConsent"` and `"todo.reminders.max"` to `"todo.remindersMax"` to eliminate nested configuration prefix collisions in standard JSON/YAML.
- Made the hashline seen-line guard opt-in and off by default, and stopped excluding column-clipped (>512-char) lines from a snapshot's seen set, allowing single-line edits on long lines to apply without a full-width re-read.
- Changed the default `astGrep.enabled` setting to `false`.
@@ -27,10 +28,12 @@
- Renamed the system prompt's project-context section wrapper from `<context>` to `<repo-rules>` to prevent collisions with the `task` tool's `context` parameter under in-band XML tool dialects.
- Rendered `read xd://` calls in a compact grouped read view instead of a full tool-execution card.
- Updated `--tools` to reject unknown tool names with a usage error instead of silently narrowing the toolset.
- Capped the `xd://` device docs inlined into the system prompt to a 48k-char budget (10k per device) to prevent large MCP catalogs from bloating requests; devices past the cap are listed by name and summary and fetched on demand.
- Migrated legacy BM25-discovery settings keys: `tools.discoveryMode: "off"` now maps to `tools.xdev: false`, and obsolete keys have been removed from the configuration.
- Capped the `xd://` device docs inlined into the system prompt to a 48k-char budget (10k per device) to prevent large MCP catalogs from bloating requests; devices past the cap are listed by name and summary and fetched on demand. External (dynamic-mount) tool descriptions embed at most 200 chars — schemas stay intact, full text via `read xd://<tool>`.
- Dead BM25-discovery settings keys (`tools.discoveryMode`, `tools.essentialOverride`, `mcp.discoveryMode`, `mcp.discoveryDefaultServers`) are cleaned from configs on load; `tools.xdev` keeps its default (`true`).
- Mid-session `xd://` mount changes (e.g. MCP connect/disconnect) no longer rewrite the system prompt: the delta is announced to the model as a steered system notice ("these tools became available" / "no longer mounted"), so the provider prompt cache stays intact; device docs join the prompt on the next unrelated rebuild.
### Fixed
- Fixed a bug where a nested configuration value (like `dev.autoqa.consent` / `dev.autoqaConsent`) would incorrectly satisfy a parent key lookup (like `dev.autoqa`), causing Auto QA to be enabled and prompt for consent by default when it should have been disabled.
- Fixed compiled appserver startup deadlocking before socket creation when user extensions were present.
- Fixed Bash internal URLs remaining unresolved when used as unquoted arguments inside command substitutions.
@@ -3460,7 +3460,7 @@ export const SETTINGS_SCHEMA = {
},
},
"todo.reminders.max": {
"todo.remindersMax": {
type: "number",
default: 3,
ui: {
@@ -5024,8 +5024,10 @@ export const SETTINGS_SCHEMA = {
*
* Owned by `packages/coding-agent/src/tools/report-tool-issue.ts` via the
* process-global consent handler registered by `InteractiveMode`.
*
* @default "unset"
*/
"dev.autoqa.consent": {
"dev.autoqaConsent": {
type: "enum",
values: ["unset", "granted", "denied"] as const,
default: "unset" as const,
@@ -820,10 +820,9 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac
/**
* True while the last painted pending-call shape opted into a full viewport
* repaint at the first result (`forceFirstResultViewportRepaint`) — e.g. the
* streamed SSH placeholder (`⏳ SSH: […]` / `$ …`) or a collapsed write tail
* window, both of which the first result render re-anchors instead of
* preserving. Kept as a per-paint fact so a topology-changing update that
* repaint at the first result (`forceFirstResultViewportRepaint`) — e.g. a
* collapsed write tail window, which the first result render re-anchors
* instead of preserving. Kept as a per-paint fact so a topology-changing update that
* lands before the pending rows reach the terminal skips the reset.
*/
#needsFirstResultViewportRepaintAtRender(): boolean {
@@ -11829,7 +11829,7 @@ export class AgentSession {
return false;
}
const remindersMax = this.settings.get("todo.reminders.max");
const remindersMax = this.settings.get("todo.remindersMax");
if (this.#todoReminderCount >= remindersMax) {
logger.debug("Todo completion: max reminders reached", { count: this.#todoReminderCount });
return false;
@@ -11,7 +11,7 @@
* dispatch path.
*
* Before the first record lands, the user's consent is checked. If they've
* never been asked (`dev.autoqa.consent === "unset"`) the process-global
* 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.
@@ -128,7 +128,7 @@ let persistentConsentSettings: Settings | null = null;
* subagent boundaries (subagents share this module instance), so a grant in
* the parent applies immediately to children — including children that spawned
* BEFORE the grant and would otherwise see a stale snapshot of
* `dev.autoqa.consent` in their isolated `Settings`.
* `dev.autoqaConsent` in their isolated `Settings`.
*
* `null` = never asked, never cached.
*/
@@ -163,7 +163,7 @@ export function __resetAutoQaConsentForTests(): void {
function readPersistedConsent(settings: Settings | undefined): boolean | null {
if (!settings) return null;
const stored = settings.get("dev.autoqa.consent");
const stored = settings.get("dev.autoqaConsent");
if (stored === "granted") return true;
if (stored === "denied") return false;
return null;
@@ -172,13 +172,13 @@ function readPersistedConsent(settings: Settings | undefined): boolean | null {
function persistConsent(localSettings: Settings | undefined, granted: boolean): void {
const value = granted ? "granted" : "denied";
try {
localSettings?.set("dev.autoqa.consent", value);
localSettings?.set("dev.autoqaConsent", value);
} catch (error) {
logger.warn("Failed to persist auto-QA consent to local settings snapshot", { error: String(error) });
}
if (persistentConsentSettings && persistentConsentSettings !== localSettings) {
try {
persistentConsentSettings.set("dev.autoqa.consent", value);
persistentConsentSettings.set("dev.autoqaConsent", value);
} catch (error) {
logger.warn("Failed to persist auto-QA consent to persistent settings", { error: String(error) });
}
@@ -280,7 +280,7 @@ export interface FlushResult {
*/
export interface FlushOptions {
/**
* Skip the `dev.autoqa.consent === "granted"` gate in
* Skip the `dev.autoqaConsent === "granted"` gate in
* {@link resolvePushConfig}. Endpoint configuration is still required.
* Reserved for explicit user-driven pushes (CLI `grievances push`,
* future debug recipes); never set from the device's auto-flush path.
@@ -345,7 +345,7 @@ function resolvePushConfig(settings: Settings | undefined, bypassConsent: boolea
// user clearly intends to ship regardless of dialog state. The
// `PI_AUTO_QA_PUSH` env flag stays as a CI/headless override too.
if (!bypassConsent) {
const consented = settings?.get("dev.autoqa.consent") === "granted";
const consented = settings?.get("dev.autoqaConsent") === "granted";
if (!consented && !$flag("PI_AUTO_QA_PUSH")) return null;
}
@@ -128,7 +128,7 @@ describe("AgentSession auto-compaction queue resume", () => {
settings: Settings.isolated({
"compaction.autoContinue": false,
"todo.reminders": true,
"todo.reminders.max": 3,
"todo.remindersMax": 3,
}),
modelRegistry,
extensionRunner,
@@ -156,7 +156,7 @@ describe("AgentSession mid-run todo reconciliation nudge", () => {
"compaction.enabled": false,
"todo.enabled": true,
"todo.reminders": true,
"todo.reminders.max": 3,
"todo.remindersMax": 3,
});
const toolSession: ToolSession = {
cwd: tempDir.path(),
@@ -141,7 +141,7 @@ describe("AgentSession todo reminder async-job deferral", () => {
"compaction.enabled": false,
"todo.enabled": true,
"todo.reminders": true,
"todo.reminders.max": 3,
"todo.remindersMax": 3,
}),
modelRegistry,
agentId: "Main",
@@ -131,7 +131,7 @@ describe("AgentSession todo reminder self-continuation suppression", () => {
"compaction.enabled": false,
"todo.enabled": true,
"todo.reminders": true,
"todo.reminders.max": 3,
"todo.remindersMax": 3,
}),
modelRegistry,
});
@@ -6,6 +6,7 @@ import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
import type { CustomTool } from "@oh-my-pi/pi-coding-agent/extensibility/custom-tools/types";
import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session";
import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager";
import { XdevRegistry } from "@oh-my-pi/pi-coding-agent/tools/xdev";
import { type } from "arktype";
// Cache-stability invariant: when MCP servers reconnect with byte-identical tool
@@ -68,6 +69,7 @@ describe("AgentSession refreshMCPTools rebuild skipping", () => {
interface NewSessionOptions {
getMcpServerInstructions?: () => Map<string, string> | undefined;
getLocalCalendarDate?: () => string;
xdevRegistry?: XdevRegistry;
}
function newSession(
@@ -101,6 +103,7 @@ describe("AgentSession refreshMCPTools rebuild skipping", () => {
}),
getMcpServerInstructions: options.getMcpServerInstructions,
getLocalCalendarDate: options.getLocalCalendarDate,
xdevRegistry: options.xdevRegistry,
});
sessions.push(session);
return { session };
@@ -497,4 +500,53 @@ describe("AgentSession refreshMCPTools rebuild skipping", () => {
await session.refreshMCPTools([tool]);
expect(rebuildCount).toBe(2);
});
it("announces xd:// mount deltas as steered notices instead of rebuilding the prompt", async () => {
let rebuildCount = 0;
const { session } = newSession(
async toolNames => {
rebuildCount++;
return `tools:${toolNames.join(",")}`;
},
{ xdevRegistry: new XdevRegistry([]) },
);
const search = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "Search nucleus");
const fetch = createMcpCustomTool("mcp__nucleus_fetch", "nucleus", "fetch", "Fetch nucleus");
const noticeTexts = () =>
session.agent
.peekSteeringQueue()
.flatMap(msg =>
msg.role === "custom" && msg.customType === "xdev-mount-notice" && typeof msg.content === "string"
? [msg.content]
: [],
);
// First refresh: initial signature record → one rebuild; the MCP tool is
// discoverable, so it mounts as a device and is announced.
await session.refreshMCPTools([search]);
expect(rebuildCount).toBe(1);
expect(noticeTexts().at(-1)).toContain("xd://mcp__nucleus_search");
// Mount-only change: NO rebuild (prompt stays byte-stable), a notice
// announces the new device.
await session.refreshMCPTools([search, fetch]);
expect(rebuildCount).toBe(1);
const mountNotice = noticeTexts().at(-1) ?? "";
expect(mountNotice).toContain("became available");
expect(mountNotice).toContain("xd://mcp__nucleus_fetch");
expect(mountNotice).not.toContain("No longer mounted");
// Unmount: still no rebuild, the removal is announced.
await session.refreshMCPTools([search]);
expect(rebuildCount).toBe(1);
const unmountNotice = noticeTexts().at(-1) ?? "";
expect(unmountNotice).toContain("No longer mounted");
expect(unmountNotice).toContain("xd://mcp__nucleus_fetch");
// Identical refresh: no new notice, no rebuild.
const noticeCount = noticeTexts().length;
await session.refreshMCPTools([search]);
expect(rebuildCount).toBe(1);
expect(noticeTexts().length).toBe(noticeCount);
});
});
@@ -694,7 +694,7 @@ describe("Settings", () => {
expect(settings.get("grep.enabled")).toBe(true);
});
it("maps legacy tools.discoveryMode 'off' to tools.xdev false and drops dead discovery keys", async () => {
it("drops dead BM25-discovery keys and leaves tools.xdev at its default", async () => {
await writeSettings({
tools: { discoveryMode: "off", essentialOverride: ["read"] },
mcp: { discoveryMode: "auto", discoveryDefaultServers: ["gh"] },
@@ -702,17 +702,10 @@ describe("Settings", () => {
const settings = await Settings.init({ cwd: projectDir, agentDir });
expect(settings.get("tools.xdev")).toBe(false);
});
it("keeps tools.xdev default for non-'off' legacy discovery modes and honors an explicit xdev", async () => {
await writeSettings({ tools: { discoveryMode: "auto" } });
const settings = await Settings.init({ cwd: projectDir, agentDir });
// No migration mapping: legacy discovery intent is discarded, xdev
// keeps its own default. An explicit xdev value is untouched.
expect(settings.get("tools.xdev")).toBe(true);
await writeSettings({ tools: { discoveryMode: "off", xdev: true } });
const explicit = await Settings.init({ cwd: projectDir, agentDir });
expect(explicit.get("tools.xdev")).toBe(true);
expect(settings.isConfigured("tools.xdev")).toBe(false);
});
it("migrates from settings.json containing comments", async () => {
@@ -27,11 +27,11 @@ describe("resolveAutoQaConsent", () => {
expect(await resolveAutoQaConsent(settings)).toBe(false);
// Default-deny must NOT persist anything — the next process invocation
// gets to re-prompt instead of being silently stuck on "no".
expect(settings.get("dev.autoqa.consent")).toBe("unset");
expect(settings.get("dev.autoqaConsent")).toBe("unset");
});
it("returns persisted `granted` without invoking the handler", async () => {
const settings = Settings.isolated({ "dev.autoqa.consent": "granted" });
const settings = Settings.isolated({ "dev.autoqaConsent": "granted" });
let calls = 0;
setAutoQaConsentHandler(async () => {
calls += 1;
@@ -42,7 +42,7 @@ describe("resolveAutoQaConsent", () => {
});
it("returns persisted `denied` without invoking the handler", async () => {
const settings = Settings.isolated({ "dev.autoqa.consent": "denied" });
const settings = Settings.isolated({ "dev.autoqaConsent": "denied" });
let calls = 0;
setAutoQaConsentHandler(async () => {
calls += 1;
@@ -75,8 +75,8 @@ describe("resolveAutoQaConsent", () => {
expect(await b).toBe(true);
expect(await c).toBe(true);
expect(calls).toBe(1);
expect(local.get("dev.autoqa.consent")).toBe("granted");
expect(persistent.get("dev.autoqa.consent")).toBe("granted");
expect(local.get("dev.autoqaConsent")).toBe("granted");
expect(persistent.get("dev.autoqaConsent")).toBe("granted");
});
it("persists a `denied` decision so the next call short-circuits", async () => {
@@ -91,8 +91,8 @@ describe("resolveAutoQaConsent", () => {
expect(await resolveAutoQaConsent(local)).toBe(false);
expect(await resolveAutoQaConsent(local)).toBe(false);
expect(calls).toBe(1);
expect(local.get("dev.autoqa.consent")).toBe("denied");
expect(persistent.get("dev.autoqa.consent")).toBe("denied");
expect(local.get("dev.autoqaConsent")).toBe("denied");
expect(persistent.get("dev.autoqaConsent")).toBe("denied");
});
it("does not cache or persist when the handler throws (allows re-prompt)", async () => {
@@ -108,7 +108,7 @@ describe("resolveAutoQaConsent", () => {
// transient, not a stuck "no".
expect(await resolveAutoQaConsent(settings)).toBe(false);
expect(calls).toBe(2);
expect(settings.get("dev.autoqa.consent")).toBe("unset");
expect(settings.get("dev.autoqaConsent")).toBe("unset");
});
it("does not cache or persist when the handler returns null (dismiss/ESC)", async () => {
@@ -126,8 +126,8 @@ describe("resolveAutoQaConsent", () => {
// Second call must re-prompt — a stray ESC isn't a permanent opt-out.
expect(await resolveAutoQaConsent(local)).toBe(false);
expect(calls).toBe(2);
expect(local.get("dev.autoqa.consent")).toBe("unset");
expect(persistent.get("dev.autoqa.consent")).toBe("unset");
expect(local.get("dev.autoqaConsent")).toBe("unset");
expect(persistent.get("dev.autoqaConsent")).toBe("unset");
});
it("falls back to the registered persistent settings when the local snapshot is unset", async () => {
@@ -135,7 +135,7 @@ describe("resolveAutoQaConsent", () => {
// (which lost the consent edit made on the parent), but the host's
// persistent Settings carries the real decision.
const subagentLocal = Settings.isolated();
const hostPersistent = Settings.isolated({ "dev.autoqa.consent": "granted" });
const hostPersistent = Settings.isolated({ "dev.autoqaConsent": "granted" });
let calls = 0;
setAutoQaConsentHandler(async () => {
calls += 1;
@@ -60,7 +60,7 @@ function pushSettings(overrides: Record<string, unknown> = {}): Settings {
"dev.autoqa": true,
// Consent is the push opt-in; `granted` is what `resolvePushConfig`
// gates on (or `PI_AUTO_QA_PUSH=1` for headless overrides).
"dev.autoqa.consent": "granted",
"dev.autoqaConsent": "granted",
"dev.autoqaPush.endpoint": "https://qa.example.com/grievances",
...overrides,
});
@@ -110,7 +110,7 @@ describe("flushGrievances", () => {
const fetchSpy = vi.fn(async () => new Response("unexpected", { status: 200 }));
// `denied` is the user-facing kill switch for push.
const result = await flushGrievances(db, pushSettings({ "dev.autoqa.consent": "denied" }), {
const result = await flushGrievances(db, pushSettings({ "dev.autoqaConsent": "denied" }), {
fetch: mockFetch(fetchSpy),
});