fix(coding-agent): used session settings in file guards
Passed session-scoped settings through Edit and Write generated-file checks and fell back to schema defaults when no global singleton exists. Guarded inline image sizing against an uninitialized global settings proxy and added isolated-session regression coverage. Fixes #6549
This commit is contained in:
@@ -22,6 +22,8 @@
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed Edit and Write tools failing with `Settings not initialized` in isolated sessions by using each tool session's settings for generated-file guards, with safe schema defaults for standalone guards and inline-image rendering ([#6549](https://github.com/can1357/oh-my-pi/issues/6549)).
|
||||
|
||||
- Fixed `todo` calls that omit `op` hard-failing validation ("op must be operation to apply (was missing)"): the tool now validates leniently and infers the op for unambiguous payloads (`list` → `init`, `phase`+`items` → `append`, bare `items` on an empty list → `init`); `op` stays required in the schema, and ambiguous op-less calls surface the schema error as a retryable tool error.
|
||||
- Fixed credential-free web search engines (SearXNG, DuckDuckGo, Google, Startpage, Ecosia, Mojeek, and the Public Web fan-out) returning zero results for queries with `site:` paths (e.g. `site:github.com/owner/repo`) or `inurl:` operators: scraper engines only match `site:` against a bare domain and DuckDuckGo ignores `inurl:` entirely, so such queries silently emptied the result set and fell through to the next provider in the chain. A shared `formatScraperQuery` formatter now structurally demotes path-carrying `site:` and all `inurl:` values to plain search terms (covering OR-grouped and quoted directives) while preserving bare-domain `site:` filters, negated operators, and each engine's supported syntax; the pipeline post-filter still enforces the demoted constraints on returned sources.
|
||||
- Fixed `ast_edit` previews reading like applied edits to the model: the `⟨proposed⟩` badge was TUI-only, so the model-visible result (hashline header + `-`/`+` rows, identical to applied edit output) carried no staged-proposal signal. The preview result now leads with a "Staged as a proposal — files NOT modified yet" notice naming `xd://resolve`/`xd://reject`, the injected resolve reminder names the source tool, and the `ast_edit` tool prompt documents the two-phase flow.
|
||||
|
||||
@@ -121,7 +121,7 @@ export class HashlineFilesystem extends Filesystem {
|
||||
throw error;
|
||||
}
|
||||
// Refuse edits against generated files (lockfiles, models.json, …).
|
||||
assertEditableFileContent(content, relativePath);
|
||||
assertEditableFileContent(content, relativePath, this.session.settings);
|
||||
return content;
|
||||
}
|
||||
|
||||
|
||||
@@ -1816,7 +1816,7 @@ export async function executePatchSingle(
|
||||
const resolvedPath = resolvePlanPath(session, path);
|
||||
const resolvedRename = rename ? resolvePlanPath(session, rename) : undefined;
|
||||
|
||||
await assertEditableFile(resolvedPath, path);
|
||||
await assertEditableFile(resolvedPath, path, session.settings);
|
||||
|
||||
// Capture pre-edit content so we can verify the write actually hit disk.
|
||||
// `LspFileSystem.writeFile` delegates to a writethrough callback that, in
|
||||
|
||||
@@ -185,7 +185,7 @@ export class StreamingEditGuard {
|
||||
#abortForAutoGeneratedPath(toolCall: ToolCall, filePath: string, resolvedPath: string): void {
|
||||
if (this.#lastToolCallId === toolCall.id) return;
|
||||
this.#lastToolCallId = toolCall.id;
|
||||
void assertEditableFile(resolvedPath, filePath).catch(error => {
|
||||
void assertEditableFile(resolvedPath, filePath, this.#host.settings).catch(error => {
|
||||
if (!(error instanceof ToolError) || this.#lastToolCallId !== toolCall.id) return;
|
||||
if (!this.#abortTriggered) {
|
||||
this.#abortTriggered = true;
|
||||
|
||||
@@ -7,7 +7,8 @@
|
||||
import * as path from "node:path";
|
||||
import { isEnoent, peekFile } from "@oh-my-pi/pi-utils";
|
||||
import { LRUCache } from "lru-cache/raw";
|
||||
import { settings } from "../config/settings";
|
||||
import { settings as globalSettings, isSettingsInitialized, type Settings } from "../config/settings";
|
||||
import { getDefault } from "../config/settings-schema";
|
||||
import { ToolError } from "./tool-errors";
|
||||
|
||||
/**
|
||||
@@ -283,15 +284,26 @@ async function getAutoGeneratedMarker(filePath: string): Promise<string | undefi
|
||||
return marker;
|
||||
}
|
||||
|
||||
function shouldBlockAutoGeneratedFiles(activeSettings?: Settings): boolean {
|
||||
if (activeSettings) return activeSettings.get("edit.blockAutoGenerated");
|
||||
if (isSettingsInitialized()) return globalSettings.get("edit.blockAutoGenerated");
|
||||
return getDefault("edit.blockAutoGenerated");
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a file is auto-generated by examining its content.
|
||||
* Throws ToolError if the file appears to be auto-generated.
|
||||
*
|
||||
* @param absolutePath - Absolute path to the file
|
||||
* @param displayPath - Path to show in error messages (relative or as provided)
|
||||
* @param activeSettings - Session settings; falls back to global settings or the schema default
|
||||
*/
|
||||
export async function assertEditableFile(absolutePath: string, displayPath?: string) {
|
||||
if (!settings.get("edit.blockAutoGenerated")) {
|
||||
export async function assertEditableFile(
|
||||
absolutePath: string,
|
||||
displayPath?: string,
|
||||
activeSettings?: Settings,
|
||||
): Promise<void> {
|
||||
if (!shouldBlockAutoGeneratedFiles(activeSettings)) {
|
||||
return;
|
||||
}
|
||||
const pathForDisplay = displayPath ?? absolutePath;
|
||||
@@ -308,9 +320,10 @@ export async function assertEditableFile(absolutePath: string, displayPath?: str
|
||||
*
|
||||
* @param content - File content to check (can be full content or prefix)
|
||||
* @param displayPath - Path to show in error messages
|
||||
* @param activeSettings - Session settings; falls back to global settings or the schema default
|
||||
*/
|
||||
export function assertEditableFileContent(content: string, displayPath: string): void {
|
||||
if (!settings.get("edit.blockAutoGenerated")) {
|
||||
export function assertEditableFileContent(content: string, displayPath: string, activeSettings?: Settings): void {
|
||||
if (!shouldBlockAutoGeneratedFiles(activeSettings)) {
|
||||
return;
|
||||
}
|
||||
|
||||
|
||||
@@ -13,7 +13,8 @@ import type { Component } from "@oh-my-pi/pi-tui";
|
||||
import { getKeybindings, replaceTabs, truncateToWidth } from "@oh-my-pi/pi-tui";
|
||||
import { pluralize } from "@oh-my-pi/pi-utils";
|
||||
import { formatKeyHints, type KeyId } from "../config/keybindings";
|
||||
import { settings } from "../config/settings";
|
||||
import { isSettingsInitialized, settings } from "../config/settings";
|
||||
import { getDefault } from "../config/settings-schema";
|
||||
import type { Theme } from "../modes/theme/theme";
|
||||
import { Hasher } from "../tui/utils";
|
||||
import { formatDimensionNote, type ResizedImage } from "../utils/image-resize";
|
||||
@@ -27,8 +28,12 @@ export { replaceTabs, truncateToWidth, wrapTextWithAnsi } from "@oh-my-pi/pi-tui
|
||||
|
||||
/** Resolve inline image dimension caps from settings and viewport. */
|
||||
export function resolveImageOptions(): { maxWidthCells: number; maxHeightCells?: number } {
|
||||
const maxWidthCells = settings.get("tui.maxInlineImageColumns");
|
||||
const rowSetting = Math.max(0, settings.get("tui.maxInlineImageRows"));
|
||||
const activeSettings = isSettingsInitialized() ? settings : undefined;
|
||||
const maxWidthCells = activeSettings?.get("tui.maxInlineImageColumns") ?? getDefault("tui.maxInlineImageColumns");
|
||||
const rowSetting = Math.max(
|
||||
0,
|
||||
activeSettings?.get("tui.maxInlineImageRows") ?? getDefault("tui.maxInlineImageRows"),
|
||||
);
|
||||
const viewportRows = process.stdout.rows;
|
||||
const viewportFraction = viewportRows ? Math.floor(viewportRows * 0.6) : 0;
|
||||
let maxHeightCells: number | undefined;
|
||||
|
||||
@@ -1215,7 +1215,7 @@ export class WriteTool implements AgentTool<typeof writeSchema, WriteToolDetails
|
||||
|
||||
// Check if file exists and is auto-generated before overwriting
|
||||
if (await fs.exists(absolutePath)) {
|
||||
await assertEditableFile(absolutePath, path);
|
||||
await assertEditableFile(absolutePath, path, this.session.settings);
|
||||
}
|
||||
|
||||
const displayPath = formatPathRelativeToCwd(absolutePath, this.session.cwd);
|
||||
|
||||
@@ -21,7 +21,7 @@ import { createMockModel } from "@oh-my-pi/pi-ai/providers/mock";
|
||||
import { AssistantMessageEventStream } from "@oh-my-pi/pi-ai/utils/event-stream";
|
||||
import { getBundledModel } from "@oh-my-pi/pi-catalog/models";
|
||||
import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry";
|
||||
import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
|
||||
import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
|
||||
import { EditTool } from "@oh-my-pi/pi-coding-agent/edit";
|
||||
import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session";
|
||||
import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage";
|
||||
@@ -248,12 +248,6 @@ it("multi-entry edit on an auto-generated file surfaces isError + error text ins
|
||||
const originalVariant = Bun.env.PI_EDIT_VARIANT;
|
||||
Bun.env.PI_EDIT_VARIANT = "patch";
|
||||
|
||||
// The auto-generated guard reads from the *global* settings singleton, so we
|
||||
// must initialize it (the per-tool `Settings.isolated(...)` we pass into the
|
||||
// EditTool isn't what the guard sees).
|
||||
resetSettingsForTest();
|
||||
await Settings.init({ inMemory: true, cwd: tempDir, overrides: { "edit.blockAutoGenerated": true } });
|
||||
|
||||
try {
|
||||
const sessionFile = path.join(tempDir, "session.jsonl");
|
||||
const sessionDir = path.join(tempDir, "session");
|
||||
|
||||
@@ -1,9 +1,9 @@
|
||||
import { afterEach, beforeAll, beforeEach, describe, expect, it } from "bun:test";
|
||||
import { afterEach, beforeEach, describe, expect, it } from "bun:test";
|
||||
import * as fs from "node:fs/promises";
|
||||
import * as os from "node:os";
|
||||
import * as path from "node:path";
|
||||
import type { AgentToolResult } from "@oh-my-pi/pi-agent-core";
|
||||
import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
|
||||
import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
|
||||
import { EditTool, type ExecuteHashlineSingleOptions, executeHashlineSingle } from "@oh-my-pi/pi-coding-agent/edit";
|
||||
import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools";
|
||||
import type { ReadToolDetails } from "@oh-my-pi/pi-coding-agent/tools/read";
|
||||
@@ -17,12 +17,6 @@ function textOutput(result: AgentToolResult<ReadToolDetails>): string {
|
||||
.join("\n");
|
||||
}
|
||||
|
||||
beforeAll(async () => {
|
||||
// The edit path's auto-generated-file guard reads the global Settings proxy.
|
||||
resetSettingsForTest();
|
||||
await Settings.init({ inMemory: true, cwd: process.cwd() });
|
||||
});
|
||||
|
||||
function createSession(cwd: string, approvedPlan?: { artifactsDir: string; planFilePath: string }): ToolSession {
|
||||
const settings = Settings.isolated();
|
||||
settings.set("read.summarize.enabled", false);
|
||||
|
||||
@@ -500,7 +500,7 @@ it("aborts auto-generated file edits as soon as the path is available", async ()
|
||||
waitBeforeFirstDelta.resolve();
|
||||
await promptPromise;
|
||||
|
||||
expect(checkSpy).toHaveBeenCalledWith(generatedPath, "generated.ts");
|
||||
expect(checkSpy).toHaveBeenCalledWith(generatedPath, "generated.ts", session.settings);
|
||||
expect(abortSpy).toHaveBeenCalled();
|
||||
expect(abortSignalRef.current?.aborted ?? false).toBe(true);
|
||||
const lastAssistant = lastAssistantMessage(session.state.messages);
|
||||
|
||||
@@ -1,49 +1,75 @@
|
||||
import { beforeAll, describe, expect, it } from "bun:test";
|
||||
import { afterAll, beforeAll, describe, expect, it } from "bun:test";
|
||||
import * as fs from "node:fs/promises";
|
||||
import * as os from "node:os";
|
||||
import * as path from "node:path";
|
||||
import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
|
||||
import { assertEditableFile, assertEditableFileContent } from "@oh-my-pi/pi-coding-agent/tools/auto-generated-guard";
|
||||
import { resolveImageOptions } from "@oh-my-pi/pi-coding-agent/tools/render-utils";
|
||||
import { ToolError } from "@oh-my-pi/pi-coding-agent/tools/tool-errors";
|
||||
|
||||
let tempDir: string;
|
||||
let testSettings: Settings;
|
||||
const GENERATED_TYPESCRIPT = "// Code generated by sqlc. DO NOT EDIT.\n\nexport const foo = 1;";
|
||||
|
||||
beforeAll(async () => {
|
||||
resetSettingsForTest();
|
||||
tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "auto-gen-guard-"));
|
||||
await Settings.init({ inMemory: true, cwd: tempDir });
|
||||
testSettings = Settings.isolated();
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
resetSettingsForTest();
|
||||
await fs.rm(tempDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
describe("assertEditableFileContent", () => {
|
||||
it("detects canonical TypeScript generated header", () => {
|
||||
const content = "// Code generated by sqlc. DO NOT EDIT.\n\nexport const foo = 1;";
|
||||
expect(() => assertEditableFileContent(content, "test.ts")).toThrow(ToolError);
|
||||
expect(() => assertEditableFileContent(GENERATED_TYPESCRIPT, "test.ts", testSettings)).toThrow(ToolError);
|
||||
});
|
||||
|
||||
it("detects @generated marker", () => {
|
||||
const content = "// @generated\n\nexport const foo = 1;";
|
||||
expect(() => assertEditableFileContent(content, "test.ts")).toThrow(ToolError);
|
||||
expect(() => assertEditableFileContent(content, "test.ts", testSettings)).toThrow(ToolError);
|
||||
});
|
||||
|
||||
it("detects generated-by marker for Python files", () => {
|
||||
const content = "# Generated by buf\n\nvalue = 1";
|
||||
expect(() => assertEditableFileContent(content, "test.py")).toThrow(ToolError);
|
||||
expect(() => assertEditableFileContent(content, "test.py", testSettings)).toThrow(ToolError);
|
||||
});
|
||||
|
||||
it("detects generated-by marker for SQL files", () => {
|
||||
const content = "-- generated by sqlc\n\nselect 1;";
|
||||
expect(() => assertEditableFileContent(content, "query.sql")).toThrow(ToolError);
|
||||
expect(() => assertEditableFileContent(content, "query.sql", testSettings)).toThrow(ToolError);
|
||||
});
|
||||
|
||||
it("detects generated markers in leading block comments", () => {
|
||||
const content = "/*\n * Code generated by mockery. DO NOT EDIT.\n */\nexport const foo = 1;";
|
||||
expect(() => assertEditableFileContent(content, "test.ts")).toThrow(ToolError);
|
||||
expect(() => assertEditableFileContent(content, "test.ts", testSettings)).toThrow(ToolError);
|
||||
});
|
||||
|
||||
it("detects kysely-codegen generated header", () => {
|
||||
const content =
|
||||
"/**\n * This file was generated by kysely-codegen.\n * Please do not edit it manually.\n */\n\nexport interface Database {}";
|
||||
expect(() => assertEditableFileContent(content, "db.ts")).toThrow(ToolError);
|
||||
expect(() => assertEditableFileContent(content, "db.ts", testSettings)).toThrow(ToolError);
|
||||
});
|
||||
|
||||
it("defaults to blocking when global settings are uninitialized", () => {
|
||||
resetSettingsForTest();
|
||||
expect(() => assertEditableFileContent(GENERATED_TYPESCRIPT, "test.ts")).toThrow(ToolError);
|
||||
});
|
||||
|
||||
it("honors isolated session settings without global initialization", () => {
|
||||
const permissiveSettings = Settings.isolated({ "edit.blockAutoGenerated": false });
|
||||
expect(() => assertEditableFileContent(GENERATED_TYPESCRIPT, "test.ts", permissiveSettings)).not.toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
describe("resolveImageOptions", () => {
|
||||
it("uses schema defaults when global settings are uninitialized", () => {
|
||||
resetSettingsForTest();
|
||||
const options = resolveImageOptions();
|
||||
expect(options.maxWidthCells).toBe(100);
|
||||
expect(options.maxHeightCells).toBeGreaterThan(0);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -51,17 +77,17 @@ describe("assertEditableFile", () => {
|
||||
it("detects content marker from file prefix", async () => {
|
||||
const filePath = path.join(tempDir, "service.ts");
|
||||
await Bun.write(filePath, "// Code generated by sqlc. DO NOT EDIT.\nexport const foo = 1;");
|
||||
await expect(assertEditableFile(filePath)).rejects.toBeInstanceOf(ToolError);
|
||||
await expect(assertEditableFile(filePath, undefined, testSettings)).rejects.toBeInstanceOf(ToolError);
|
||||
});
|
||||
|
||||
it("allows normal files", async () => {
|
||||
const filePath = path.join(tempDir, "normal.ts");
|
||||
await Bun.write(filePath, "// Regular source file\nexport const foo = 1;");
|
||||
await expect(assertEditableFile(filePath)).resolves.toBeUndefined();
|
||||
await expect(assertEditableFile(filePath, undefined, testSettings)).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
it("handles missing files gracefully", async () => {
|
||||
const filePath = path.join(tempDir, "does-not-exist.ts");
|
||||
await expect(assertEditableFile(filePath)).resolves.toBeUndefined();
|
||||
await expect(assertEditableFile(filePath, undefined, testSettings)).resolves.toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { beforeAll, describe, expect, it } from "bun:test";
|
||||
import { describe, expect, it } from "bun:test";
|
||||
import * as fs from "node:fs/promises";
|
||||
import * as os from "node:os";
|
||||
import * as path from "node:path";
|
||||
@@ -32,12 +32,6 @@ async function makeWorkspace(): Promise<string> {
|
||||
}
|
||||
|
||||
describe("write refuses read-selector misfires", () => {
|
||||
beforeAll(async () => {
|
||||
// assertEditableFile (auto-generated guard) reads the global settings proxy
|
||||
// when overwriting an existing file.
|
||||
await Settings.init({ inMemory: true });
|
||||
});
|
||||
|
||||
it("fails closed on a missing selector-suffixed target with empty content and points at read()", async () => {
|
||||
const dir = await makeWorkspace();
|
||||
const write = new WriteTool(session(dir));
|
||||
|
||||
Reference in New Issue
Block a user