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.
This commit is contained in:
can1357
@@ -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 |
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -136,11 +136,17 @@ function stringArrayFromUnknown(value: unknown): string[] {
|
||||
return [];
|
||||
}
|
||||
|
||||
function isRecord(value: unknown): value is Record<string, unknown> {
|
||||
return !!value && typeof value === "object" && !Array.isArray(value);
|
||||
}
|
||||
|
||||
function shallowStringRecord(value: unknown): Record<string, string> {
|
||||
if (!value || typeof value !== "object" || Array.isArray(value)) return {};
|
||||
if (!isRecord(value)) return {};
|
||||
|
||||
const result: Record<string, string> = {};
|
||||
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<string, string> })?.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;
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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",
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -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({
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user