From 1da7df1aee9c3ecad68976e20c2241a17dd7e71b Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Mon, 27 Apr 2026 15:53:46 +0900 Subject: [PATCH] Revert "Merge pull request #3657 from code-yeongyu/refactor/replace-zwsp-with-real-spaces" This reverts commit f1a11f2c92dded0a8c2f505b0fb9a8fdc28f8a69, reversing changes made to 62c19ce0ef6ddd48a7e59660d3ef72348da1e38a. --- .../claude-code-session-state/state.ts | 8 +- src/plugin-handlers/AGENTS.md | 27 +--- src/plugin-handlers/agent-config-handler.ts | 9 +- src/shared/agent-display-names.test.ts | 25 +-- src/shared/agent-display-names.ts | 27 +++- src/shared/agent-runtime-name-sort.test.ts | 152 ------------------ 6 files changed, 35 insertions(+), 213 deletions(-) delete mode 100644 src/shared/agent-runtime-name-sort.test.ts diff --git a/src/features/claude-code-session-state/state.ts b/src/features/claude-code-session-state/state.ts index 0eccbf9b3..049366166 100644 --- a/src/features/claude-code-session-state/state.ts +++ b/src/features/claude-code-session-state/state.ts @@ -18,16 +18,12 @@ const registeredAgentAliases = new Map() const ZERO_WIDTH_CHARACTERS_REGEX = /[\u200B\u200C\u200D\uFEFF]/g -function stripSortPrefix(name: string): string { - return name.replace(ZERO_WIDTH_CHARACTERS_REGEX, "").replace(/^\s+/, "") -} - function normalizeRegisteredAgentName(name: string): string { - return stripSortPrefix(name).toLowerCase() + return name.replace(ZERO_WIDTH_CHARACTERS_REGEX, "").toLowerCase() } function normalizeStoredAgentName(name: string): string { - return stripSortPrefix(name) + return name.replace(ZERO_WIDTH_CHARACTERS_REGEX, "") } export function registerAgentName(name: string): void { diff --git a/src/plugin-handlers/AGENTS.md b/src/plugin-handlers/AGENTS.md index 8d0154485..df6c8bf14 100644 --- a/src/plugin-handlers/AGENTS.md +++ b/src/plugin-handlers/AGENTS.md @@ -8,45 +8,28 @@ The canonical agent order is **sisyphus → hephaestus → prometheus → atlas* This order is enforced via two mechanisms working together: 1. `CANONICAL_CORE_AGENT_ORDER` in `agent-priority-order.ts` controls object key insertion order -2. `agent-key-remapper.ts` injects leading-space-prefixed runtime names into the `name` field for OpenCode's `localeCompare` sort +2. `agent-key-remapper.ts` injects ZWSP-prefixed runtime names into the `name` field for OpenCode's `localeCompare` sort ### Why Two Mechanisms -OpenCode's `Agent.list()` sorts agents by `name` field via `localeCompare`. Object key order alone is not enough. The `name` field carries leading ASCII spaces (4-3-2-1 descending) so core agents sort before alphabetically-named agents. +OpenCode's `Agent.list()` sorts agents by `name` field via `localeCompare`. Object key order alone is not enough. The `name` field carries ZWSP prefixes (1-4 chars) so core agents sort before alphabetically-named agents. -The prefix lengths are intentionally **descending** (sisyphus=4, hephaestus=3, prometheus=2, atlas=1) because `localeCompare` puts strings with more leading whitespace before strings with fewer. Reference: see `agent-runtime-name-sort.test.ts` for empirical verification. - -### Why ASCII Spaces, Not ZWSP - -Earlier versions used ZWSP (`\u200B`) prefixes hoping they would be invisible to users. They silently failed: Unicode collation algorithms treat zero-width characters as ignorable at the primary level, so ZWSP-prefixed names sorted as if the prefix did not exist. The result was alphabetical order interleaving core and non-core agents. - -ASCII space (`\u0020`) is the only character that: -- Sorts before alphabetic characters reliably under all locales -- Renders correctly in every terminal (no glyph substitution) -- Is valid in HTTP header values (RFC 7230) when placed in the `name` field - -The leading-space prefix MUST NOT appear in: +ZWSP is intentionally used in the `name` field only. It MUST NOT appear in: - Object keys (used as HTTP header values, causes RFC 7230 violations) - Display names returned by `getAgentDisplayName()` - Config keys -### Backward Compatibility - -`stripAgentListSortPrefix()` strips both the new leading-space prefix AND legacy ZWSP/zero-width characters. Existing sessions and configs from the ZWSP era continue to resolve correctly. - ### History -Agent ordering caused 15+ commits, 8+ PRs, and multiple reverts due to: +Agent ordering has caused 15+ commits, 8+ PRs, and multiple reverts due to: 1. Early ZWSP attempts that leaked into HTTP headers via object keys 2. Object.entries() iteration order depending on merge sequence 3. Multiple code paths assembling agents differently -4. The ZWSP prefix being silently broken in `localeCompare` sort (resolved in this commit by switching to leading ASCII spaces) ### Forbidden Patterns DO NOT introduce: -- ZWSP in any field (broken in `localeCompare`, replaced by leading ASCII spaces) -- Leading whitespace in object keys or display names (allowed only in `name` field via `getAgentRuntimeName()`) +- ZWSP in object keys or display names (only allowed in `name` field via `getAgentRuntimeName()`) - Runtime sort shims or comparators - Alternative ordering constants - Object.entries() order dependencies diff --git a/src/plugin-handlers/agent-config-handler.ts b/src/plugin-handlers/agent-config-handler.ts index c8f2a7810..384871114 100644 --- a/src/plugin-handlers/agent-config-handler.ts +++ b/src/plugin-handlers/agent-config-handler.ts @@ -2,7 +2,7 @@ import { createBuiltinAgents } from "../agents"; import { createSisyphusJuniorAgentWithOverrides } from "../agents/sisyphus-junior"; import type { OhMyOpenCodeConfig } from "../config"; import { isTaskSystemEnabled, log, migrateAgentConfig } from "../shared"; -import { AGENT_DISPLAY_NAMES, getAgentConfigKey, getAgentRuntimeName } from "../shared/agent-display-names"; +import { getAgentRuntimeName } from "../shared/agent-display-names"; import { AGENT_NAME_MAP } from "../shared/migration"; import { registerAgentName } from "../features/claude-code-session-state"; import { @@ -189,11 +189,8 @@ export async function applyAgentConfig(params: { if (isSisyphusEnabled && builtinAgents.sisyphus) { if (configuredDefaultAgent) { - const configKey = getAgentConfigKey(configuredDefaultAgent); - const isKnownBuiltin = configKey in AGENT_DISPLAY_NAMES; - (params.config as { default_agent?: string }).default_agent = isKnownBuiltin - ? getAgentRuntimeName(configKey) - : configuredDefaultAgent; + (params.config as { default_agent?: string }).default_agent = + getAgentRuntimeName(configuredDefaultAgent); } else { (params.config as { default_agent?: string }).default_agent = getAgentRuntimeName("sisyphus"); diff --git a/src/shared/agent-display-names.test.ts b/src/shared/agent-display-names.test.ts index e4abea669..2c3d732cd 100644 --- a/src/shared/agent-display-names.test.ts +++ b/src/shared/agent-display-names.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "bun:test" -import { AGENT_DISPLAY_NAMES, getAgentConfigKey, getAgentDisplayName, getAgentListDisplayName, getAgentRuntimeName, normalizeAgentForPrompt, normalizeAgentForPromptKey } from "./agent-display-names" +import { AGENT_DISPLAY_NAMES, getAgentConfigKey, getAgentDisplayName, getAgentListDisplayName, normalizeAgentForPrompt, normalizeAgentForPromptKey } from "./agent-display-names" describe("getAgentDisplayName", () => { it("returns display name for lowercase config key (new format)", () => { @@ -194,11 +194,11 @@ describe("getAgentConfigKey", () => { }) describe("getAgentListDisplayName", () => { - it("returns clean display names for object keys (no leading whitespace, RFC 7230 safe)", () => { - expect(getAgentListDisplayName("sisyphus")).toBe("Sisyphus - Ultraworker") - expect(getAgentListDisplayName("hephaestus")).toBe("Hephaestus - Deep Agent") - expect(getAgentListDisplayName("prometheus")).toBe("Prometheus - Plan Builder") - expect(getAgentListDisplayName("atlas")).toBe("Atlas - Plan Executor") + it("applies invisible stable-sort prefixes to the core agent list", () => { + expect(getAgentListDisplayName("sisyphus")).toBe("\u200BSisyphus - Ultraworker") + expect(getAgentListDisplayName("hephaestus")).toBe("\u200B\u200BHephaestus - Deep Agent") + expect(getAgentListDisplayName("prometheus")).toBe("\u200B\u200B\u200BPrometheus - Plan Builder") + expect(getAgentListDisplayName("atlas")).toBe("\u200B\u200B\u200B\u200BAtlas - Plan Executor") }) it("keeps non-core agents unprefixed for list display", () => { @@ -206,19 +206,6 @@ describe("getAgentListDisplayName", () => { }) }) -describe("getAgentRuntimeName", () => { - it("applies leading-space stable-sort prefixes so OpenCode localeCompare yields canonical order", () => { - expect(getAgentRuntimeName("sisyphus")).toBe(" Sisyphus - Ultraworker") - expect(getAgentRuntimeName("hephaestus")).toBe(" Hephaestus - Deep Agent") - expect(getAgentRuntimeName("prometheus")).toBe(" Prometheus - Plan Builder") - expect(getAgentRuntimeName("atlas")).toBe(" Atlas - Plan Executor") - }) - - it("keeps non-core agents unprefixed (no entry in AGENT_LIST_SORT_PREFIXES)", () => { - expect(getAgentRuntimeName("oracle")).toBe("oracle") - }) -}) - describe("normalizeAgentForPrompt", () => { it("strips core UI ordering prefixes back to canonical display names", () => { expect(normalizeAgentForPrompt(getAgentListDisplayName("sisyphus"))).toBe("Sisyphus - Ultraworker") diff --git a/src/shared/agent-display-names.ts b/src/shared/agent-display-names.ts index 10bf91845..324fac785 100644 --- a/src/shared/agent-display-names.ts +++ b/src/shared/agent-display-names.ts @@ -27,10 +27,10 @@ export const AGENT_DISPLAY_NAMES: Record = { } const AGENT_LIST_SORT_PREFIXES: Record = { - sisyphus: " ", - hephaestus: " ", - prometheus: " ", - atlas: " ", + sisyphus: "\u200B", + hephaestus: "\u200B\u200B", + prometheus: "\u200B\u200B\u200B", + atlas: "\u200B\u200B\u200B\u200B", } const INVISIBLE_AGENT_CHARACTERS_REGEX = /[\u200B\u200C\u200D\uFEFF]/g @@ -40,7 +40,7 @@ export function stripInvisibleAgentCharacters(agentName: string): string { } export function stripAgentListSortPrefix(agentName: string): string { - return stripInvisibleAgentCharacters(agentName).replace(/^\s+/, "") + return stripInvisibleAgentCharacters(agentName) } export function getAgentRuntimeName(configKey: string): string { @@ -50,20 +50,31 @@ export function getAgentRuntimeName(configKey: string): string { return prefix ? `${prefix}${displayName}` : displayName } +/** + * Get display name for an agent config key. + * Uses case-insensitive lookup for backward compatibility. + * Returns original key if not found. + */ export function getAgentDisplayName(configKey: string): string { + // Try exact match first const exactMatch = AGENT_DISPLAY_NAMES[configKey] if (exactMatch !== undefined) return exactMatch - + + // Fall back to case-insensitive search const lowerKey = configKey.toLowerCase() for (const [k, v] of Object.entries(AGENT_DISPLAY_NAMES)) { if (k.toLowerCase() === lowerKey) return v } - + + // Unknown agent: return original key return configKey } +/** + * Runtime-facing agent name used for OpenCode list ordering. + */ export function getAgentListDisplayName(configKey: string): string { - return getAgentDisplayName(configKey) + return getAgentRuntimeName(configKey) } const REVERSE_DISPLAY_NAMES: Record = Object.fromEntries( diff --git a/src/shared/agent-runtime-name-sort.test.ts b/src/shared/agent-runtime-name-sort.test.ts deleted file mode 100644 index c39b4a545..000000000 --- a/src/shared/agent-runtime-name-sort.test.ts +++ /dev/null @@ -1,152 +0,0 @@ -/// - -import { describe, expect, it, test } from "bun:test" - -import { - AGENT_DISPLAY_NAMES, - getAgentRuntimeName, - normalizeAgentForPromptKey, -} from "./agent-display-names" - -// OpenCode Agent.list() sorts via remeda sortBy: default_agent desc, then name asc localeCompare. -// Reference: ../opencode/packages/opencode/src/agent/agent.ts:284-293. -// Earlier ZWSP prefixes silently failed: Unicode collation treats zero-width chars as ignorable. -function simulateOpencodeSort(agentNames: string[], defaultName: string): string[] { - return [...agentNames].sort((a, b) => { - const aIsDefault = a === defaultName ? 1 : 0 - const bIsDefault = b === defaultName ? 1 : 0 - if (aIsDefault !== bIsDefault) return bIsDefault - aIsDefault - return a.localeCompare(b) - }) -} - -describe("OpenCode Agent.list() sort with runtime-name prefixes", () => { - describe("#given the four core agents and a mix of non-core agents", () => { - test("#when sorted using opencode-style sortBy #then core agents come first in canonical order", () => { - const sisyphus = getAgentRuntimeName("sisyphus") - const hephaestus = getAgentRuntimeName("hephaestus") - const prometheus = getAgentRuntimeName("prometheus") - const atlas = getAgentRuntimeName("atlas") - - const allAgents = [ - sisyphus, - hephaestus, - prometheus, - atlas, - "athena", - "explore", - "metis", - "oracle", - ] - - const sorted = simulateOpencodeSort(allAgents, sisyphus) - const orderedConfigKeys = sorted.map((name) => normalizeAgentForPromptKey(name)) - - expect(orderedConfigKeys).toEqual([ - "sisyphus", - "hephaestus", - "prometheus", - "atlas", - "athena", - "explore", - "metis", - "oracle", - ]) - }) - - test("#when default_agent is unset #then canonical core order still holds via prefix alone", () => { - const sisyphus = getAgentRuntimeName("sisyphus") - const hephaestus = getAgentRuntimeName("hephaestus") - const prometheus = getAgentRuntimeName("prometheus") - const atlas = getAgentRuntimeName("atlas") - - const allAgents = [hephaestus, prometheus, atlas, sisyphus, "athena", "oracle"] - - const sorted = simulateOpencodeSort(allAgents, "no-such-default-agent") - const orderedConfigKeys = sorted.map((name) => normalizeAgentForPromptKey(name)) - - expect(orderedConfigKeys.slice(0, 4)).toEqual([ - "sisyphus", - "hephaestus", - "prometheus", - "atlas", - ]) - }) - }) - - describe("#given input array in random order", () => { - test("#when sorted with opencode comparator #then result is always canonical", () => { - const sisyphus = getAgentRuntimeName("sisyphus") - const hephaestus = getAgentRuntimeName("hephaestus") - const prometheus = getAgentRuntimeName("prometheus") - const atlas = getAgentRuntimeName("atlas") - const nonCore = ["athena", "explore", "librarian", "metis", "oracle"] - const allAgents = [...nonCore, atlas, prometheus, hephaestus, sisyphus] - - for (let attempt = 0; attempt < 25; attempt += 1) { - const shuffled = [...allAgents] - for (let i = shuffled.length - 1; i > 0; i -= 1) { - const j = Math.floor(Math.random() * (i + 1)) - ;[shuffled[i], shuffled[j]] = [shuffled[j], shuffled[i]] - } - const sorted = simulateOpencodeSort(shuffled, sisyphus) - const orderedConfigKeys = sorted.map((name) => normalizeAgentForPromptKey(name)) - - expect(orderedConfigKeys).toEqual([ - "sisyphus", - "hephaestus", - "prometheus", - "atlas", - "athena", - "explore", - "librarian", - "metis", - "oracle", - ]) - } - }) - }) - - describe("#given runtime names containing only core agents", () => { - test("#when sorted #then sisyphus, hephaestus, prometheus, atlas in that order", () => { - const sisyphus = getAgentRuntimeName("sisyphus") - const hephaestus = getAgentRuntimeName("hephaestus") - const prometheus = getAgentRuntimeName("prometheus") - const atlas = getAgentRuntimeName("atlas") - - const sorted = simulateOpencodeSort([atlas, prometheus, hephaestus, sisyphus], sisyphus) - const orderedConfigKeys = sorted.map((name) => normalizeAgentForPromptKey(name)) - - expect(orderedConfigKeys).toEqual([ - "sisyphus", - "hephaestus", - "prometheus", - "atlas", - ]) - }) - }) - - describe("#given the prefix is meant to render in OpenCode TUI", () => { - it("uses ASCII whitespace so terminals render the prefix without character corruption", () => { - const runtimeNames = Object.keys(AGENT_DISPLAY_NAMES).map(getAgentRuntimeName) - const invisibleCharsRegex = /[\u200B\u200C\u200D\uFEFF]/ - - for (const name of runtimeNames) { - expect(invisibleCharsRegex.test(name)).toBe(false) - } - }) - - it("only adds leading whitespace, never trailing or interior whitespace beyond the display name", () => { - const sisyphus = getAgentRuntimeName("sisyphus") - const hephaestus = getAgentRuntimeName("hephaestus") - const prometheus = getAgentRuntimeName("prometheus") - const atlas = getAgentRuntimeName("atlas") - - for (const name of [sisyphus, hephaestus, prometheus, atlas]) { - const trimmed = name.trimStart() - expect(name.length).toBeGreaterThanOrEqual(trimmed.length) - expect(trimmed.endsWith(" ")).toBe(false) - } - }) - }) -})