From ac904fc70ccc6c3051209e0dd0a328c950468e1b Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 26 Jun 2026 13:14:44 +0200 Subject: [PATCH] fix: patched Kimi model edit mode fallback - Added a fallback from `hashline` to `replace` mode for Kimi-family models to resolve compatibility issues. - Introduced `PI_STRICT_EDIT_MODE` environment variable to bypass automatic model-specific edit-mode fallbacks. - Updated `getEditVariantForModel` to perform case-insensitive matching for model variant configurations. - Added comprehensive unit tests for edit mode resolution and settings configuration. --- docs/environment-variables.md | 1 + packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/scripts/bench-guard.ts | 2 +- packages/coding-agent/src/config/settings.ts | 16 +++- packages/coding-agent/src/utils/edit-mode.ts | 21 ++++- packages/coding-agent/test/edit-mode.test.ts | 92 +++++++++++++++++++ .../test/settings-manager.test.ts | 16 ++++ .../typescript-edit-benchmark/src/runner.ts | 22 +++++ scripts/edit-benchmark.py | 2 +- 9 files changed, 164 insertions(+), 9 deletions(-) create mode 100644 packages/coding-agent/test/edit-mode.test.ts diff --git a/docs/environment-variables.md b/docs/environment-variables.md index dd485c7d6..078ea5fe4 100644 --- a/docs/environment-variables.md +++ b/docs/environment-variables.md @@ -332,6 +332,7 @@ Extra conditional behavior: | `OLLAMA_CONTEXT_LENGTH` | Positive integer context-window override for implicit Ollama discovery; affects OMP context budgeting only and does not change Ollama's runtime `num_ctx` | | `LLAMA_CPP_BASE_URL` | Default implicit Llama.cpp discovery base URL override (`http://127.0.0.1:8080` if unset) | | `PI_EDIT_VARIANT` | Forces edit tool variant when valid (`patch`, `replace`, `hashline`, `apply_patch`) | +| `PI_STRICT_EDIT_MODE` | If `1`, disables built-in model-specific edit-mode fallbacks, so the configured/global `edit.mode` is used unless `PI_EDIT_VARIANT` or `edit.modelVariants` overrides it | | `PI_FORCE_IMAGE_PROTOCOL` | Forces supported image protocol (`kitty`, `iterm2`/`iterm`, `sixel`, `none`) where used | | `PI_ALLOW_SIXEL_PASSTHROUGH` | Allows SIXEL passthrough when `PI_FORCE_IMAGE_PROTOCOL=sixel` | | `PI_NO_PTY` | If `1`, disables interactive PTY path for bash tool | diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 947d38e7c..8903a162f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- Fixed Kimi-family models defaulting to hashline edit mode; they now fall back to `replace` unless `edit.modelVariants`, `PI_EDIT_VARIANT`, or `PI_STRICT_EDIT_MODE` explicitly opts into hashline. - Fixed MCP OAuth discovery rejecting Atlassian-style cross-host issuer metadata during the resource-server fallback probe; issuer matching now remains enforced for advertised auth-server candidates but no longer blocks fallback metadata where the resource host and authorization-server issuer differ. ([#3551](https://github.com/can1357/oh-my-pi/issues/3551)) - Fixed plan approval applying the wrong execution model when the model-tier slider sat on the model that exit would restore. The match check now compares the selected role's effective thinking level against the pre-plan thinking level, so picking the active planning tier is retained and picking a same-model tier with an explicit thinking suffix (e.g. `default = sonnet:off` while plan-mode raised thinking to `high`) goes through `applyRoleModel` instead of silently restoring the pre-plan level. ([#3554](https://github.com/can1357/oh-my-pi/issues/3554)) - Fixed plan approval applying the wrong execution model when the model-tier slider sat on the model that exit would restore. The match check now compares the selected role's effective thinking level against the pre-plan thinking level, and a singleton cycle (only `modelRoles.plan` configured, default unset, so `getRoleModelCycle` synthesizes a lone `default` entry from the active plan model and the slider stays hidden) is no longer pinned as the execution tier — approval falls through to the pre-plan restore instead of silently switching back to the plan model. ([#3554](https://github.com/can1357/oh-my-pi/issues/3554)) diff --git a/packages/coding-agent/scripts/bench-guard.ts b/packages/coding-agent/scripts/bench-guard.ts index 2ce365d9f..63f5a10b9 100755 --- a/packages/coding-agent/scripts/bench-guard.ts +++ b/packages/coding-agent/scripts/bench-guard.ts @@ -22,7 +22,7 @@ import * as path from "node:path"; const THRESHOLD = 1.05; // 5% regression budget const BASELINE_PATH = path.join(import.meta.dir, "..", "bench", "boot-baseline.json"); -const BENCH_COMMAND = "PI_TIMING=x bun src/cli.ts"; +const BENCH_COMMAND = "PI_TIMING=x PI_STRICT_EDIT_MODE=1 bun src/cli.ts"; const cwd = path.join(import.meta.dir, ".."); function medianOf(hyperfineJson: string): number { diff --git a/packages/coding-agent/src/config/settings.ts b/packages/coding-agent/src/config/settings.ts index 977a8ae75..4e382877b 100644 --- a/packages/coding-agent/src/config/settings.ts +++ b/packages/coding-agent/src/config/settings.ts @@ -136,11 +136,17 @@ function stringArrayFromUnknown(value: unknown): string[] { return []; } +function isRecord(value: unknown): value is Record { + return !!value && typeof value === "object" && !Array.isArray(value); +} + function shallowStringRecord(value: unknown): Record { - if (!value || typeof value !== "object" || Array.isArray(value)) return {}; + if (!isRecord(value)) return {}; const result: Record = {}; - for (const [key, item] of Object.entries(value)) { + for (const key in value) { + if (!Object.hasOwn(value, key)) continue; + const item = value[key]; if (typeof item === "string") { result[key] = item; } @@ -481,10 +487,10 @@ export class Settings { */ getEditVariantForModel(model: string | undefined): EditMode | null { if (!model) return null; - const variants = (this.#merged.edit as { modelVariants?: Record })?.modelVariants; - if (!variants) return null; + const variants = shallowStringRecord(getByPath(this.#merged, ["edit", "modelVariants"])); + const modelLower = model.toLowerCase(); for (const pattern in variants) { - if (model.includes(pattern)) { + if (modelLower.includes(pattern.toLowerCase())) { const value = normalizeEditMode(variants[pattern]); if (value) { return value; diff --git a/packages/coding-agent/src/utils/edit-mode.ts b/packages/coding-agent/src/utils/edit-mode.ts index 8c2cdfc85..a4212cfa9 100644 --- a/packages/coding-agent/src/utils/edit-mode.ts +++ b/packages/coding-agent/src/utils/edit-mode.ts @@ -1,4 +1,4 @@ -import { $env } from "@oh-my-pi/pi-utils"; +import { $env, $flag } from "@oh-my-pi/pi-utils"; export type EditMode = "replace" | "patch" | "hashline" | "apply_patch"; @@ -13,6 +13,19 @@ const EDIT_MODE_IDS = { export const EDIT_MODES = Object.keys(EDIT_MODE_IDS) as EditMode[]; +const HASHLINE_EXCLUDED_MODEL_MODES: Array<{ pattern: string; mode: EditMode }> = [ + { pattern: "kimi", mode: "replace" }, +]; + +function resolveHashlineExcludedModelMode(model: string | undefined): EditMode | null { + if (!model) return null; + const modelLower = model.toLowerCase(); + for (const entry of HASHLINE_EXCLUDED_MODEL_MODES) { + if (modelLower.includes(entry.pattern)) return entry.mode; + } + return null; +} + export function normalizeEditMode(mode?: string | null): EditMode | undefined { if (!mode) return undefined; return EDIT_MODE_IDS[mode as keyof typeof EDIT_MODE_IDS]; @@ -37,5 +50,9 @@ export function resolveEditMode(session: EditModeSessionLike): EditMode { if (envMode) return envMode; const settingsMode = normalizeEditMode(String(session.settings.get("edit.mode") ?? "")); - return settingsMode ?? DEFAULT_EDIT_MODE; + const mode = settingsMode ?? DEFAULT_EDIT_MODE; + if (mode === "hashline" && !$flag("PI_STRICT_EDIT_MODE")) { + return resolveHashlineExcludedModelMode(activeModel) ?? mode; + } + return mode; } diff --git a/packages/coding-agent/test/edit-mode.test.ts b/packages/coding-agent/test/edit-mode.test.ts new file mode 100644 index 000000000..cc5c36fc0 --- /dev/null +++ b/packages/coding-agent/test/edit-mode.test.ts @@ -0,0 +1,92 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { type EditMode, type EditModeSessionLike, resolveEditMode } from "@oh-my-pi/pi-coding-agent/utils/edit-mode"; + +const originalEditVariant = Bun.env.PI_EDIT_VARIANT; +const originalStrictEditMode = Bun.env.PI_STRICT_EDIT_MODE; + +function restoreEnv(): void { + if (originalEditVariant === undefined) { + delete Bun.env.PI_EDIT_VARIANT; + } else { + Bun.env.PI_EDIT_VARIANT = originalEditVariant; + } + if (originalStrictEditMode === undefined) { + delete Bun.env.PI_STRICT_EDIT_MODE; + } else { + Bun.env.PI_STRICT_EDIT_MODE = originalStrictEditMode; + } +} + +function createSession(args: { + activeModel?: string; + modelVariant?: EditMode | null; + settingsMode?: EditMode; +}): EditModeSessionLike { + return { + getActiveModelString: () => args.activeModel, + settings: { + get: () => args.settingsMode ?? "hashline", + getEditVariantForModel: () => args.modelVariant ?? null, + }, + }; +} + +describe("resolveEditMode", () => { + beforeEach(() => { + delete Bun.env.PI_EDIT_VARIANT; + delete Bun.env.PI_STRICT_EDIT_MODE; + }); + + afterEach(() => { + restoreEnv(); + }); + + test("falls back from hashline to replace for Kimi models", () => { + delete Bun.env.PI_EDIT_VARIANT; + + expect(resolveEditMode(createSession({ activeModel: "openrouter/moonshotai/Kimi-K2-Instruct" }))).toBe("replace"); + }); + + test("does not exclude non-Kimi Moonshot models", () => { + delete Bun.env.PI_EDIT_VARIANT; + + expect(resolveEditMode(createSession({ activeModel: "moonshot/moonshot-v1-128k" }))).toBe("hashline"); + }); + + test("keeps explicit model variants ahead of the Kimi fallback", () => { + delete Bun.env.PI_EDIT_VARIANT; + + expect( + resolveEditMode( + createSession({ activeModel: "openrouter/moonshotai/Kimi-K2-Instruct", modelVariant: "hashline" }), + ), + ).toBe("hashline"); + }); + + test("keeps PI_EDIT_VARIANT ahead of the Kimi fallback", () => { + Bun.env.PI_EDIT_VARIANT = "hashline"; + + expect(resolveEditMode(createSession({ activeModel: "openrouter/moonshotai/Kimi-K2-Instruct" }))).toBe( + "hashline", + ); + }); + + test("only falls back when the resolved mode is hashline", () => { + delete Bun.env.PI_EDIT_VARIANT; + + expect( + resolveEditMode( + createSession({ activeModel: "openrouter/moonshotai/Kimi-K2-Instruct", settingsMode: "apply_patch" }), + ), + ).toBe("apply_patch"); + }); + + test("keeps strict edit mode ahead of the Kimi fallback", () => { + delete Bun.env.PI_EDIT_VARIANT; + Bun.env.PI_STRICT_EDIT_MODE = "1"; + + expect(resolveEditMode(createSession({ activeModel: "openrouter/moonshotai/Kimi-K2-Instruct" }))).toBe( + "hashline", + ); + }); +}); diff --git a/packages/coding-agent/test/settings-manager.test.ts b/packages/coding-agent/test/settings-manager.test.ts index dce19a62f..b0384e475 100644 --- a/packages/coding-agent/test/settings-manager.test.ts +++ b/packages/coding-agent/test/settings-manager.test.ts @@ -364,6 +364,22 @@ describe("Settings", () => { }); }); + describe("getEditVariantForModel", () => { + it("matches configured model variants case-insensitively", async () => { + await writeSettings({ + edit: { + modelVariants: { + kimi: "hashline", + }, + }, + }); + + const settings = await Settings.init({ cwd: projectDir, agentDir }); + + expect(settings.getEditVariantForModel("openrouter/moonshotai/Kimi-K2-Instruct")).toBe("hashline"); + }); + }); + describe("migrations", () => { it("maps removed atom edit mode settings to hashline", async () => { await writeSettings({ diff --git a/packages/typescript-edit-benchmark/src/runner.ts b/packages/typescript-edit-benchmark/src/runner.ts index 2e37e090a..902d453cd 100644 --- a/packages/typescript-edit-benchmark/src/runner.ts +++ b/packages/typescript-edit-benchmark/src/runner.ts @@ -1033,6 +1033,13 @@ async function runSingleTask( let zeroToolRetries = 0; let providerFailureRetries = 0; + const previousEnv = { + PI_EDIT_VARIANT: process.env.PI_EDIT_VARIANT, + PI_EDIT_FUZZY: process.env.PI_EDIT_FUZZY, + PI_EDIT_FUZZY_THRESHOLD: process.env.PI_EDIT_FUZZY_THRESHOLD, + PI_STRICT_EDIT_MODE: process.env.PI_STRICT_EDIT_MODE, + PI_NO_TITLE: process.env.PI_NO_TITLE, + }; try { const sessionSetup = await prepareBenchmarkSessionSetup({ config, @@ -1052,6 +1059,7 @@ async function runSingleTask( if (config.editFuzzyThreshold !== undefined) process.env.PI_EDIT_FUZZY_THRESHOLD = config.editFuzzyThreshold === "auto" ? "auto" : String(config.editFuzzyThreshold); + process.env.PI_STRICT_EDIT_MODE = "1"; process.env.PI_NO_TITLE = "1"; const useInProcess = config.inProcess !== false; @@ -1332,6 +1340,20 @@ async function runSingleTask( } catch (err) { error = err instanceof Error ? err.message : String(err); await logEvent({ type: "error", error }); + } finally { + const restoreEnvKey = (key: keyof typeof previousEnv) => { + const value = previousEnv[key]; + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + }; + restoreEnvKey("PI_EDIT_VARIANT"); + restoreEnvKey("PI_EDIT_FUZZY"); + restoreEnvKey("PI_EDIT_FUZZY_THRESHOLD"); + restoreEnvKey("PI_STRICT_EDIT_MODE"); + restoreEnvKey("PI_NO_TITLE"); } const duration = Date.now() - startTime; diff --git a/scripts/edit-benchmark.py b/scripts/edit-benchmark.py index 6ed967d8c..86d1926dc 100755 --- a/scripts/edit-benchmark.py +++ b/scripts/edit-benchmark.py @@ -58,7 +58,7 @@ def build_spec(variant: str) -> BenchmarkSpec: description=f"Benchmark edit tool in {variant} mode across models with simple edit tasks.", workspace_prefix=f"{variant}-benchmark", tools=("edit", "read"), - env={"PI_EDIT_VARIANT": variant}, + env={"PI_EDIT_VARIANT": variant, "PI_STRICT_EDIT_MODE": "1"}, initial_prompt=prompt, retry_instruction=retry, )