refactor(coding-agent): streamlined codebase by deduplicating helper logic and shims

- Consolidated duplicated inline thinking level comparisons into a unified `concreteThinkingLevel` helper.
- Enhanced legacy tool shims to respect isolated session settings and support legacy options.
- Cleaned up redundant UI render requests and extra status-line updates.
- Refactored `grep` tool shim to configure context dynamically via isolated settings.
- Disabled platform-incompatible shell shim tests on Windows environments.
This commit is contained in:
can1357
2026-07-02 02:40:08 +02:00
parent d84a54e579
commit 51684b4b1d
14 changed files with 90 additions and 67 deletions
+7 -5
View File
@@ -30,7 +30,12 @@ import { buildServiceTierByFamily, serviceTierForAllFamilies, serviceTierSetting
import { Settings } from "../config/settings";
import benchPrompt from "../prompts/bench.md" with { type: "text" };
import { discoverAuthStorage, loadCliExtensionProviders } from "../sdk";
import { AUTO_THINKING, resolveThinkingLevelForModel, shouldDisableReasoning, toReasoningEffort } from "../thinking";
import {
concreteThinkingLevel,
resolveThinkingLevelForModel,
shouldDisableReasoning,
toReasoningEffort,
} from "../thinking";
const DEFAULT_RUNS = 10;
const DEFAULT_PAR = 4;
@@ -477,10 +482,7 @@ function resolveBenchModels(
resolved.push({
selector,
model,
thinking: resolveThinkingLevelForModel(
model,
result.thinkingLevel === AUTO_THINKING ? undefined : result.thinkingLevel,
),
thinking: resolveThinkingLevelForModel(model, concreteThinkingLevel(result.thinkingLevel)),
});
}
if (errors.length > 0) {
@@ -11,7 +11,7 @@ import {
import { MODEL_ROLE_IDS } from "../config/model-roles";
import type { Settings } from "../config/settings";
import MODEL_PRIO from "../priority.json" with { type: "json" };
import { AUTO_THINKING, type ConfiguredThinkingLevel } from "../thinking";
import { concreteThinkingLevel } from "../thinking";
export interface ResolvedCommitModel {
model: Model<Api>;
@@ -21,6 +21,11 @@ export interface ResolvedCommitModel {
* central force-refresh + account-rotation policy.
*/
apiKey: ApiKey;
/**
* Commit-time inference is stateless: session-level auto classification
* isn't available, so an explicit `:auto` selector collapses to "no
* override" and the model's own default level fills in.
*/
thinkingLevel?: ThinkingLevel;
}
@@ -29,13 +34,6 @@ type CommitModelRegistry = ModelLookupRegistry &
getApiKey: (model: Model<Api>) => Promise<string | undefined>;
};
// Commit-time inference is stateless: session-level auto classification isn't
// available, so an explicit `:auto` selector collapses to "no override" and
// the model's own default level fills in.
function coerceCommitThinkingLevel(level: ConfiguredThinkingLevel | undefined): ThinkingLevel | undefined {
return level === AUTO_THINKING ? undefined : level;
}
export async function resolvePrimaryModel(
override: string | undefined,
settings: Settings,
@@ -57,7 +55,7 @@ export async function resolvePrimaryModel(
return {
model,
apiKey: modelRegistry.resolver(model),
thinkingLevel: coerceCommitThinkingLevel(resolved?.thinkingLevel),
thinkingLevel: concreteThinkingLevel(resolved?.thinkingLevel),
};
}
@@ -75,7 +73,7 @@ export async function resolveSmolModel(
return {
model: resolvedSmol.model,
apiKey: modelRegistry.resolver(resolvedSmol.model),
thinkingLevel: coerceCommitThinkingLevel(resolvedSmol.thinkingLevel),
thinkingLevel: concreteThinkingLevel(resolvedSmol.thinkingLevel),
};
}
}
@@ -32,6 +32,7 @@ import MODEL_PRIO from "../priority.json" with { type: "json" };
import {
AUTO_THINKING,
type ConfiguredThinkingLevel,
concreteThinkingLevel,
parseThinkingLevel,
resolveThinkingLevelForModel,
} from "../thinking";
@@ -136,12 +137,9 @@ function resolveGlobScopePattern(
// Coerce the `auto` sentinel to a concrete-only view so scope callers stay
// typed on `ThinkingLevel` and `enabledModels: [\"openai/*:auto\"]` doesn't
// pin a stray per-model level.
const concrete = (level: ConfiguredThinkingLevel | undefined): ThinkingLevel | undefined =>
level === AUTO_THINKING ? undefined : level;
const strictSuffix = splitThinkingSuffix(pattern);
if (strictSuffix.level !== undefined) {
const thinkingLevel = concrete(strictSuffix.level);
const thinkingLevel = concreteThinkingLevel(strictSuffix.level);
return {
models: matchingGlobModels(strictSuffix.base, availableModels),
thinkingLevel,
@@ -155,7 +153,7 @@ function resolveGlobScopePattern(
if (literalMatches.length > 0) {
return { models: literalMatches, thinkingLevel: undefined, explicitThinkingLevel: false };
}
const thinkingLevel = concrete(maxSuffix.level);
const thinkingLevel = concreteThinkingLevel(maxSuffix.level);
return {
models: matchingGlobModels(maxSuffix.base, availableModels),
thinkingLevel,
@@ -18,7 +18,7 @@ import type { AgentToolResult, AgentToolUpdateCallback } from "@oh-my-pi/pi-agen
import type { TSchema } from "@oh-my-pi/pi-ai";
import { Text } from "@oh-my-pi/pi-tui";
import { parseFrontmatter as parseOmpFrontmatter } from "@oh-my-pi/pi-utils";
import { Settings } from "../config/settings";
import { type SettingPath, Settings } from "../config/settings";
import { EditTool } from "../edit";
import {
DEFAULT_MAX_BYTES,
@@ -46,6 +46,8 @@ type LegacyCodingToolName = (typeof LEGACY_CODING_TOOL_NAMES)[number];
type LegacyRegistryToolName = LegacyCodingToolName | "grep" | "glob";
type LegacyBuiltinToolDefinition = ToolDefinition & { [LEGACY_BUILTIN_TOOL_MARKER]: true };
type LegacySettingOverrides = Partial<Record<SettingPath, unknown>>;
interface LegacyThemeLike {
fg(color: string, text: string): string;
bold(text: string): string;
@@ -79,10 +81,18 @@ export interface BashToolOptions {
}
export interface ReadToolOptions {
/** Auto-resize large images; maps onto the `images.autoResize` setting. Default: true. */
autoResizeImages?: boolean;
}
export interface GrepToolOptions {
/**
* Unsupported. The historical grep operations seam (isDirectory/readFile for
* context lines) never delegated the search itself — ripgrep always ran
* locally — and the built-in native grep tool exposes no filesystem seam at
* all. Supplying operations throws at tool creation instead of silently
* searching the local filesystem.
*/
operations?: unknown;
}
@@ -123,7 +133,6 @@ const legacyGrepSchema = Type.Object({
ignoreCase: Type.Optional(Type.Boolean({ description: "Case-insensitive search" })),
literal: Type.Optional(Type.Boolean({ description: "Treat pattern as a literal string" })),
context: Type.Optional(Type.Number({ description: "Context lines" })),
limit: Type.Optional(Type.Number({ description: "Maximum matches" })),
});
const legacyFindSchema = Type.Object({
@@ -149,18 +158,22 @@ function markToolDefinition<TParams extends TSchema, TDetails>(
return tool;
}
function legacyToolSession(cwd: string): ToolSession {
function legacyToolSession(cwd: string, settingOverrides?: LegacySettingOverrides): ToolSession {
return {
cwd,
hasUI: false,
getSessionFile: () => null,
getSessionSpawns: () => null,
settings: Settings.isolated(),
settings: Settings.isolated(settingOverrides),
};
}
function createRegistryTool(cwd: string, name: LegacyRegistryToolName): Tool {
const session = legacyToolSession(cwd);
function createRegistryTool(
cwd: string,
name: LegacyRegistryToolName,
settingOverrides?: LegacySettingOverrides,
): Tool {
const session = legacyToolSession(cwd, settingOverrides);
switch (name) {
case "bash":
return new BashTool(session);
@@ -337,10 +350,6 @@ async function executeLegacyBashOperations(
}
}
function createLegacyTool(_cwd: string, definition: ToolDefinition): ToolDefinition {
return definition;
}
/** Parse frontmatter using the historical Pi package-root helper. */
export interface ParsedFrontmatter<T extends Record<string, unknown> = Record<string, unknown>> {
frontmatter: T;
@@ -368,8 +377,12 @@ export function defineTool<TParams extends TSchema = TSchema, TDetails = unknown
}
/** Create the legacy read tool definition. */
export function createReadToolDefinition(cwd: string, _options?: ReadToolOptions): ToolDefinition {
const tool = createRegistryTool(cwd, "read");
export function createReadToolDefinition(cwd: string, options?: ReadToolOptions): ToolDefinition {
const tool = createRegistryTool(
cwd,
"read",
options?.autoResizeImages === undefined ? undefined : { "images.autoResize": options.autoResizeImages },
);
return markToolDefinition({
name: "read",
label: "Read",
@@ -392,7 +405,7 @@ export function createReadToolDefinition(cwd: string, _options?: ReadToolOptions
/** Create the legacy read tool. */
export function createReadTool(cwd: string, options?: ReadToolOptions): ToolDefinition {
return createLegacyTool(cwd, createReadToolDefinition(cwd, options));
return createReadToolDefinition(cwd, options);
}
/** Create the legacy bash tool definition. */
@@ -441,11 +454,19 @@ export function createBashToolDefinition(cwd: string, options?: BashToolOptions)
/** Create the legacy bash tool. */
export function createBashTool(cwd: string, options?: BashToolOptions): ToolDefinition {
return createLegacyTool(cwd, createBashToolDefinition(cwd, options));
return createBashToolDefinition(cwd, options);
}
/** Create the legacy grep tool definition. */
export function createGrepToolDefinition(cwd: string, _options?: GrepToolOptions): ToolDefinition {
export function createGrepToolDefinition(cwd: string, options?: GrepToolOptions): ToolDefinition {
if (options?.operations) {
throw new Error(
"Legacy GrepToolOptions.operations is not supported: the built-in grep tool searches the local " +
"filesystem natively and exposes no pluggable filesystem seam (the historical seam only customized " +
"context-line reads; the search itself always ran locally). Register a custom grep tool via " +
"defineTool() instead of passing operations to createGrepTool()/createGrepToolDefinition().",
);
}
const tool = createRegistryTool(cwd, "grep");
return markToolDefinition({
name: "grep",
@@ -465,7 +486,17 @@ export function createGrepToolDefinition(cwd: string, _options?: GrepToolOptions
const pattern = booleanField(params, "literal") ? escapeRegexLiteral(rawPattern) : rawPattern;
const searchPath = stringField(params, "path") ?? ".";
const glob = stringField(params, "glob");
return tool.execute(
const context = numberField(params, "context");
// The new grep reads context from settings fixed at construction; build a
// per-call tool when the model passes an explicit legacy `context`.
const grepTool =
context === undefined
? tool
: createRegistryTool(cwd, "grep", {
"grep.contextBefore": Math.max(0, Math.floor(context)),
"grep.contextAfter": Math.max(0, Math.floor(context)),
});
return grepTool.execute(
toolCallId,
{
pattern,
@@ -481,7 +512,7 @@ export function createGrepToolDefinition(cwd: string, _options?: GrepToolOptions
/** Create the legacy grep tool. */
export function createGrepTool(cwd: string, options?: GrepToolOptions): ToolDefinition {
return createLegacyTool(cwd, createGrepToolDefinition(cwd, options));
return createGrepToolDefinition(cwd, options);
}
/** Create the legacy find tool definition. */
@@ -537,7 +568,7 @@ export function createFindToolDefinition(cwd: string, options?: FindToolOptions)
/** Create the legacy find tool. */
export function createFindTool(cwd: string, options?: FindToolOptions): ToolDefinition {
return createLegacyTool(cwd, createFindToolDefinition(cwd, options));
return createFindToolDefinition(cwd, options);
}
/** Create the legacy ls tool definition. */
@@ -582,7 +613,7 @@ export function createLsToolDefinition(cwd: string, options?: LsToolOptions): To
/** Create the legacy ls tool. */
export function createLsTool(cwd: string, options?: LsToolOptions): ToolDefinition {
return createLegacyTool(cwd, createLsToolDefinition(cwd, options));
return createLsToolDefinition(cwd, options);
}
/** Create legacy read, bash, edit, and write tools. */
@@ -5,7 +5,7 @@ import { extractTextContent, extractToolCall, parseJsonPayload } from "../commit
import guidedGoalInterviewPrompt from "../prompts/goals/guided-goal-interview.md" with { type: "text" };
import guidedGoalSystemPrompt from "../prompts/goals/guided-goal-system.md" with { type: "text" };
import type { AgentSession } from "../session/agent-session";
import { AUTO_THINKING, shouldDisableReasoning, toReasoningEffort } from "../thinking";
import { concreteThinkingLevel, shouldDisableReasoning, toReasoningEffort } from "../thinking";
const RESPOND_TOOL_NAME = "respond";
@@ -92,6 +92,7 @@ export async function runGuidedGoalTurn(
// never sent verbatim to the plan/slow provider. Deobfuscated again below before display/use.
const obfuscator = session.obfuscator;
const promptText = obfuscator?.hasSecrets() ? obfuscator.obfuscate(userPrompt) : userPrompt;
const thinkingLevel = concreteThinkingLevel(resolved.thinkingLevel);
const response = await instrumentedCompleteSimple(
resolved.model,
{
@@ -102,10 +103,8 @@ export async function runGuidedGoalTurn(
{
apiKey: session.modelRegistry.resolver(resolved.model, session.sessionId),
signal: options.signal,
reasoning: toReasoningEffort(resolved.thinkingLevel === AUTO_THINKING ? undefined : resolved.thinkingLevel),
disableReasoning: shouldDisableReasoning(
resolved.thinkingLevel === AUTO_THINKING ? undefined : resolved.thinkingLevel,
),
reasoning: toReasoningEffort(thinkingLevel),
disableReasoning: shouldDisableReasoning(thinkingLevel),
toolChoice: { type: "tool", name: RESPOND_TOOL_NAME },
},
{ telemetry: resolveTelemetry(session.agent.telemetry, session.sessionId), oneshotKind: "guided_goal_setup" },
+4 -4
View File
@@ -74,7 +74,7 @@ import { shouldShowStartupSplash } from "./startup-splash";
import { discoverTitleSystemPromptFile, resolvePromptInput } from "./system-prompt";
import { createPersistedSubagentReviverFactory } from "./task/persisted-revive";
import { initTelemetryExport, isTelemetryExportEnabled } from "./telemetry-export";
import { AUTO_THINKING, parseConfiguredThinkingLevel } from "./thinking";
import { concreteThinkingLevel, parseConfiguredThinkingLevel } from "./thinking";
import type { LspStartupServerInfo } from "./tools";
import {
getChangelogPath,
@@ -894,9 +894,9 @@ async function buildSessionOptions(
if (scopedModels.length > 0) {
// `auto` is a session-level concept only; per-scoped-model (Ctrl+P) thinking
// overrides stay concrete, so coerce the auto default to "unset" here.
const defaultThinkingLevelSetting = parseConfiguredThinkingLevel(activeSettings.get("defaultThinkingLevel"));
const defaultThinkingLevel =
defaultThinkingLevelSetting === AUTO_THINKING ? undefined : defaultThinkingLevelSetting;
const defaultThinkingLevel = concreteThinkingLevel(
parseConfiguredThinkingLevel(activeSettings.get("defaultThinkingLevel")),
);
options.scopedModels = scopedModels.map(scopedModel => ({
model: scopedModel.model,
thinkingLevel: scopedModel.explicitThinkingLevel
@@ -157,7 +157,6 @@ export class SelectorController {
const result = await previewTheme(themeName);
if (result.success) {
this.ctx.statusLine.invalidate();
this.ctx.ui.requestRender();
this.ctx.ui.invalidate();
this.ctx.ui.requestRender();
}
@@ -2664,7 +2664,6 @@ export class InteractiveMode implements InteractiveModeContext {
// plan-approved prompt is the source of the reference injection.
this.session.markPlanReferenceSent();
const planModePrompt = prompt.render(planModeApprovedPrompt, {
planContent,
planFilePath: options.planFilePath,
contextPreserved: options.preserveContext === true,
});
+4 -3
View File
@@ -135,6 +135,7 @@ import { wrapStreamFnWithProviderConcurrency } from "./task/provider-concurrency
import {
AUTO_THINKING,
type ConfiguredThinkingLevel,
concreteThinkingLevel,
parseConfiguredThinkingLevel,
parseThinkingLevel,
resolveProvisionalAutoLevel,
@@ -1343,7 +1344,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {}
// Concrete level the agent/session start with. With `auto` this is the
// provisional level shown until the first per-turn classification resolves;
// `auto` itself stays a session-only concept handled by AgentSession.
let effectiveThinkingLevel: ThinkingLevel | undefined = thinkingLevel === AUTO_THINKING ? undefined : thinkingLevel;
let effectiveThinkingLevel: ThinkingLevel | undefined = concreteThinkingLevel(thinkingLevel);
if (model) {
const resolvedModel = model;
effectiveThinkingLevel = logger.time("resolveThinkingLevelForModel", () =>
@@ -1941,7 +1942,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {}
// `thinking.defaultLevel` must not become sticky.
thinkingLevel = pickInitialThinkingLevel(restoredModel);
autoThinking = thinkingLevel === AUTO_THINKING;
effectiveThinkingLevel = thinkingLevel === AUTO_THINKING ? undefined : thinkingLevel;
effectiveThinkingLevel = concreteThinkingLevel(thinkingLevel);
effectiveThinkingLevel = logger.time("resolveThinkingLevelForModel", () =>
autoThinking
? resolveProvisionalAutoLevel(restoredModel)
@@ -1997,7 +1998,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {}
// so the role's explicit selector (e.g. `:max`) now applies.
thinkingLevel = pickInitialThinkingLevel(resolvedDefaultModel);
autoThinking = thinkingLevel === AUTO_THINKING;
effectiveThinkingLevel = thinkingLevel === AUTO_THINKING ? undefined : thinkingLevel;
effectiveThinkingLevel = concreteThinkingLevel(thinkingLevel);
effectiveThinkingLevel = logger.time("resolveThinkingLevelForModel", () =>
autoThinking
? resolveProvisionalAutoLevel(resolvedDefaultModel)
@@ -72,7 +72,6 @@ export interface TuiBuiltinSlashCommand extends BuiltinSlashCommand {
function refreshStatusLine(ctx: InteractiveModeContext): void {
ctx.statusLine.invalidate();
ctx.ui.requestRender();
ctx.ui.requestRender();
}
/** `/fast status` label for the active model: "on" when its family is priority, else "off". */
+5
View File
@@ -124,6 +124,11 @@ export const AUTO_THINKING = "auto" as const;
/** A thinking selector as configured by the user — a concrete level or `auto`. */
export type ConfiguredThinkingLevel = ThinkingLevel | typeof AUTO_THINKING;
/** Maps the session-level `auto` sentinel to `undefined`; concrete levels pass through. */
export function concreteThinkingLevel(level: ConfiguredThinkingLevel | undefined): ThinkingLevel | undefined {
return level === AUTO_THINKING ? undefined : level;
}
/** Metadata used to render the `auto` selector value alongside concrete levels. */
export interface ConfiguredThinkingLevelMetadata {
value: ConfiguredThinkingLevel;
@@ -12,7 +12,7 @@ import { getModelMatchPreferences, resolveModelRoleValue } from "../config/model
import type { Settings } from "../config/settings";
import MODEL_PRIO from "../priority.json" with { type: "json" };
import commitSystemPrompt from "../prompts/system/commit-message-system.md" with { type: "text" };
import { AUTO_THINKING, toReasoningEffort } from "../thinking";
import { concreteThinkingLevel, toReasoningEffort } from "../thinking";
const COMMIT_SYSTEM_PROMPT = prompt.render(commitSystemPrompt);
const MAX_DIFF_CHARS = 4000;
@@ -56,10 +56,7 @@ function getSmolModelCandidates(
settings,
matchPreferences,
});
addCandidate(
configuredSmol.model,
configuredSmol.thinkingLevel === AUTO_THINKING ? undefined : configuredSmol.thinkingLevel,
);
addCandidate(configuredSmol.model, concreteThinkingLevel(configuredSmol.thinkingLevel));
for (const pattern of MODEL_PRIO.smol) {
const needle = pattern.toLowerCase();
@@ -855,6 +855,8 @@ describe("github tool", () => {
expect(runGit(remoteFixture.repoRoot, ["remote", "get-url", "forksrc"])).toBe(remoteFixture.forkBare);
});
it("does not depend on localized git remote-add stderr for existing remotes", async () => {
// The shim is a bash script resolved via `which`; neither exists on Windows.
if (process.platform === "win32") return;
const originalPath = process.env.PATH;
const fakeBin = await fs.mkdtemp(path.join(os.tmpdir(), "omp-fake-git-"));
const realGitResult = Bun.spawnSync(["which", "git"], { stdout: "pipe", stderr: "pipe" });
@@ -1,7 +1,7 @@
import { describe, expect, it } from "bun:test";
import type { AuthStorage, FetchImpl } from "@oh-my-pi/pi-ai";
import { searchDuckDuckGo } from "@oh-my-pi/pi-coding-agent/web/search/providers/duckduckgo";
import { SEARCH_PROVIDER_OPTIONS, SearchProviderError } from "@oh-my-pi/pi-coding-agent/web/search/types";
import { SearchProviderError } from "@oh-my-pi/pi-coding-agent/web/search/types";
import { formatSearchProviderFailures } from "../../src/web/search/provider";
const fakeAuthStorage = {
@@ -265,11 +265,4 @@ describe("DuckDuckGo web search provider", () => {
expect(message).toContain("configure a credentialed provider");
expect(message).not.toContain("codex: 401 unauthorized");
});
it("documents DuckDuckGo as a best-effort datacenter-sensitive fallback", () => {
const option = SEARCH_PROVIDER_OPTIONS.find(item => item.value === "duckduckgo");
expect(option?.description).toContain("Credential-free best-effort fallback");
expect(option?.description).toContain("datacenter/shared-egress IPs");
});
});