From f83b57d350395f59c354ae97d7e844faeb80944e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 12:11:55 +0000 Subject: [PATCH 1/4] Implement the when-clause evaluator and context service MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit compileWhen tokenizes and parses the design.md §6.4 grammar once into an AST (WhenParseError on malformed clauses) and evaluates against a plain context getter: bare-key truthiness, string equality, !/&&/|| with correct precedence, unknown keys falsy. createContextService provides the flat Map with set/get per ContextNamespace plus an internal onDidChange (Object.is change detection, Disposable listeners). Also clones HostError on HostLog.append (follow-up to the post-merge CodeRabbit comment on PR #42). Fixes #5 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- packages/core/src/commands/registry.test.ts | 10 + packages/core/src/host/errors.ts | 4 +- packages/core/src/index.ts | 15 +- packages/core/src/keymap/context.test.ts | 104 ++++++ packages/core/src/keymap/context.ts | 66 ++++ packages/core/src/keymap/index.ts | 19 +- packages/core/src/keymap/when.test.ts | 205 ++++++++++++ packages/core/src/keymap/when.ts | 334 ++++++++++++++++++++ 8 files changed, 752 insertions(+), 5 deletions(-) create mode 100644 packages/core/src/keymap/context.test.ts create mode 100644 packages/core/src/keymap/context.ts create mode 100644 packages/core/src/keymap/when.test.ts create mode 100644 packages/core/src/keymap/when.ts diff --git a/packages/core/src/commands/registry.test.ts b/packages/core/src/commands/registry.test.ts index e99ab22..3360c43 100644 --- a/packages/core/src/commands/registry.test.ts +++ b/packages/core/src/commands/registry.test.ts @@ -287,3 +287,13 @@ test("HostLog.entries returns a snapshot, not the internal records", () => { expect(log.entries()[0]?.error.message).toBe("original"); }); + +test("HostLog.append clones the incoming error, isolating later caller mutations", () => { + const log = createHostLog(); + const err: HostError = { message: "original" }; + log.append("error", err); + + err.message = "mutated by caller"; + + expect(log.entries()[0]?.error.message).toBe("original"); +}); diff --git a/packages/core/src/host/errors.ts b/packages/core/src/host/errors.ts index 53b78a9..95b387d 100644 --- a/packages/core/src/host/errors.ts +++ b/packages/core/src/host/errors.ts @@ -45,7 +45,9 @@ export function createHostLog(): HostLog { const records: HostLogEntry[] = []; return { append(level, error) { - records.push({ level, error }); + // Clone on the way in as well: a caller mutating the error object it + // passed must not rewrite the stored record (append-only contract). + records.push({ level, error: { ...error } }); }, entries() { // Snapshot: cloning each entry (and its error) keeps the log diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 601b7bf..ae2dc7c 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -6,7 +6,20 @@ export { type CommandRegistry, type CommandRegistryDeps, } from "./commands/index"; -export { KEYMAP_PLACEHOLDER } from "./keymap/index"; +export { + compileWhen, + createContextService, + WhenParseError, + type CompiledWhen, + type ContextService, + type WhenAndNode, + type WhenContextGetter, + type WhenEqNode, + type WhenKeyNode, + type WhenNode, + type WhenNotNode, + type WhenOrNode, +} from "./keymap/index"; export { BUFFER_PLACEHOLDER } from "./buffer/index"; export { UI_PLACEHOLDER } from "./ui/index"; export { CONFIG_PLACEHOLDER } from "./config/index"; diff --git a/packages/core/src/keymap/context.test.ts b/packages/core/src/keymap/context.test.ts new file mode 100644 index 0000000..1250380 --- /dev/null +++ b/packages/core/src/keymap/context.test.ts @@ -0,0 +1,104 @@ +import { expect, test } from "bun:test"; +import { createContextService } from "./context"; + +test("set then get round-trips the value", () => { + const context = createContextService(); + + context.set("editorLangId", "ts"); + + expect(context.get("editorLangId")).toBe("ts"); +}); + +test("get on an unset key returns undefined", () => { + const context = createContextService(); + + expect(context.get("neverSet")).toBeUndefined(); +}); + +test("set fires onDidChange with the changed key", () => { + const context = createContextService(); + const seen: string[] = []; + context.onDidChange((key) => seen.push(key)); + + context.set("editorFocus", true); + + expect(seen).toEqual(["editorFocus"]); +}); + +test("setting a genuinely different value fires again with the same key", () => { + const context = createContextService(); + const seen: string[] = []; + context.onDidChange((key) => seen.push(key)); + + context.set("editorLangId", "ts"); + context.set("editorLangId", "js"); + + expect(seen).toEqual(["editorLangId", "editorLangId"]); + expect(context.get("editorLangId")).toBe("js"); +}); + +test("setting an identical value does not fire onDidChange", () => { + const context = createContextService(); + context.set("editorFocus", true); + + const seen: string[] = []; + context.onDidChange((key) => seen.push(key)); + context.set("editorFocus", true); + + expect(seen).toEqual([]); +}); + +test("setting the same NaN value twice does not fire (Object.is semantics)", () => { + const context = createContextService(); + context.set("metric", NaN); + + const seen: string[] = []; + context.onDidChange((key) => seen.push(key)); + context.set("metric", NaN); + + expect(seen).toEqual([]); +}); + +test("dispose stops the listener from receiving further changes", () => { + const context = createContextService(); + const seen: string[] = []; + const subscription = context.onDidChange((key) => seen.push(key)); + + context.set("a", 1); + subscription.dispose(); + context.set("a", 2); + + expect(seen).toEqual(["a"]); + expect(context.get("a")).toBe(2); +}); + +test("double-dispose is a no-op", () => { + const context = createContextService(); + const subscription = context.onDidChange(() => undefined); + + subscription.dispose(); + expect(() => subscription.dispose()).not.toThrow(); +}); + +test("multiple listeners each receive the change", () => { + const context = createContextService(); + const seenA: string[] = []; + const seenB: string[] = []; + context.onDidChange((key) => seenA.push(key)); + context.onDidChange((key) => seenB.push(key)); + + context.set("explorerFocus", true); + + expect(seenA).toEqual(["explorerFocus"]); + expect(seenB).toEqual(["explorerFocus"]); +}); + +test("distinct keys are stored independently", () => { + const context = createContextService(); + + context.set("editorFocus", true); + context.set("explorerFocus", false); + + expect(context.get("editorFocus")).toBe(true); + expect(context.get("explorerFocus")).toBe(false); +}); diff --git a/packages/core/src/keymap/context.ts b/packages/core/src/keymap/context.ts new file mode 100644 index 0000000..58ff0ca --- /dev/null +++ b/packages/core/src/keymap/context.ts @@ -0,0 +1,66 @@ +/** + * The context service (Req 4.6, design.md §6.4): a flat key/value store + * that `when` clauses ({@link WhenContextGetter}) read from. Built as a + * factory function — `createContextService()` — rather than a class, to + * match the rest of core (`createCommandRegistry`, `createHostLog`). + * + * Core sets keys like `editorFocus`, `editorTextFocus`, `editorLangId`, + * and focus-tracking keys as focus moves; extensions set their own (e.g. + * `explorerFocus`) through `tecode.context.set`. + */ + +import type { ContextNamespace, Disposable, Event, Listener } from "@tecode/api"; + +/** + * The context service's internal shape: `set`/`get` are exactly + * `tecode.context` ({@link ContextNamespace}); `onDidChange` is exposed + * only to internal consumers (focus tracking, the palette, the keymap + * service's binding re-evaluation) — it is not part of the public + * `ContextNamespace` surface extensions see. Because `ContextService` + * extends `ContextNamespace`, the public projection is just picking + * `{ set, get }` off of it; no separate wrapper object is needed. + */ +export interface ContextService extends ContextNamespace { + /** Fires with the key that changed whenever `set` actually changes its + * value (design.md §6.4). Setting a key to a value it already holds + * does not fire. */ + onDidChange: Event; +} + +/** Build a context service (Req 4.6). Backed by a single + * `Map` — no per-namespace nesting, no schema. */ +export function createContextService(): ContextService { + const store = new Map(); + const listeners = new Set>(); + + function get(key: string): T | undefined { + return store.get(key) as T | undefined; + } + + function set(key: string, value: unknown): void { + const previous = store.get(key); + // Object.is (not ===) so re-setting NaN to NaN is correctly treated + // as "unchanged" rather than spuriously firing a change event. + if (Object.is(previous, value)) return; + store.set(key, value); + // Snapshot before iterating: a listener that disposes itself (or + // another listener) during the loop must not perturb this dispatch. + for (const listener of Array.from(listeners)) { + listener(key); + } + } + + function onDidChange(listener: Listener): Disposable { + listeners.add(listener); + let disposed = false; + return { + dispose() { + if (disposed) return; + disposed = true; + listeners.delete(listener); + }, + }; + } + + return { get, set, onDidChange }; +} diff --git a/packages/core/src/keymap/index.ts b/packages/core/src/keymap/index.ts index db19238..e2035d7 100644 --- a/packages/core/src/keymap/index.ts +++ b/packages/core/src/keymap/index.ts @@ -1,3 +1,16 @@ -// Placeholder for the keymap service (key event pipeline, chord state -// machine, when-clause evaluator). -export const KEYMAP_PLACEHOLDER = true; +// The keymap service (design.md §6): the when-clause evaluator (§6.4) and +// context service land here first; the input pipeline, resolution model +// (§6.2), and chord state machine (§6.3) are later tasks. +export { + compileWhen, + WhenParseError, + type CompiledWhen, + type WhenAndNode, + type WhenContextGetter, + type WhenEqNode, + type WhenKeyNode, + type WhenNode, + type WhenNotNode, + type WhenOrNode, +} from "./when"; +export { createContextService, type ContextService } from "./context"; diff --git a/packages/core/src/keymap/when.test.ts b/packages/core/src/keymap/when.test.ts new file mode 100644 index 0000000..fcbf786 --- /dev/null +++ b/packages/core/src/keymap/when.test.ts @@ -0,0 +1,205 @@ +import { describe, expect, spyOn, test } from "bun:test"; +import { compileWhen, WhenParseError, __whenTestHooks } from "./when"; + +/** Build a context getter from a plain object, for table-driven tests. */ +function contextOf(values: Record): (key: string) => unknown { + return (key) => values[key]; +} + +describe("compileWhen — valid clauses", () => { + const cases: { + name: string; + clause: string; + context: Record; + expected: boolean; + }[] = [ + { name: "bare truthy key", clause: "editorFocus", context: { editorFocus: true }, expected: true }, + { name: "bare falsy key", clause: "editorFocus", context: { editorFocus: false }, expected: false }, + { + name: "bare key, unknown → falsy", + clause: "editorFocus", + context: {}, + expected: false, + }, + { + name: "string equality, match", + clause: "editorLangId == 'ts'", + context: { editorLangId: "ts" }, + expected: true, + }, + { + name: "string equality, mismatch", + clause: "editorLangId == 'ts'", + context: { editorLangId: "js" }, + expected: false, + }, + { + name: "string equality, double-quoted literal", + clause: 'editorLangId == "ts"', + context: { editorLangId: "ts" }, + expected: true, + }, + { + name: "string equality against unknown key", + clause: "editorLangId == 'ts'", + context: {}, + expected: false, + }, + { + name: "negation", + clause: "!editorFocus", + context: { editorFocus: false }, + expected: true, + }, + { + name: "double negation", + clause: "!!editorFocus", + context: { editorFocus: true }, + expected: true, + }, + { + name: "&& both true", + clause: "a && b", + context: { a: true, b: true }, + expected: true, + }, + { + name: "&& one false", + clause: "a && b", + context: { a: true, b: false }, + expected: false, + }, + { + name: "|| one true", + clause: "a || b", + context: { a: false, b: true }, + expected: true, + }, + { + name: "|| both false", + clause: "a || b", + context: { a: false, b: false }, + expected: false, + }, + { + name: "&& binds tighter than || (a || b && c), a true short-circuits", + clause: "a || b && c", + context: { a: true, b: false, c: false }, + expected: true, + }, + { + name: "&& binds tighter than || (a || b && c), a false needs b&&c", + clause: "a || b && c", + context: { a: false, b: true, c: false }, + expected: false, + }, + { + name: "&& binds tighter than || (a || b && c), a false, b&&c true", + clause: "a || b && c", + context: { a: false, b: true, c: true }, + expected: true, + }, + { + name: "parentheses override precedence: (a || b) && c, false", + clause: "(a || b) && c", + context: { a: true, b: false, c: false }, + expected: false, + }, + { + name: "parentheses override precedence: (a || b) && c, true", + clause: "(a || b) && c", + context: { a: true, b: false, c: true }, + expected: true, + }, + { + name: "negated group", + clause: "!(a && b)", + context: { a: true, b: true }, + expected: false, + }, + { + name: "whitespace tolerance: tabs/newlines/extra spaces", + clause: " a\t&&\n( b ||c ) ", + context: { a: true, b: false, c: true }, + expected: true, + }, + { + name: "combined equality and negation", + clause: "editorTextFocus && !explorerFocus && editorLangId == 'ts'", + context: { editorTextFocus: true, explorerFocus: false, editorLangId: "ts" }, + expected: true, + }, + ]; + + for (const { name, clause, context, expected } of cases) { + test(`${name}: ${JSON.stringify(clause)}`, () => { + const compiled = compileWhen(clause); + expect(compiled.evaluate(contextOf(context))).toBe(expected); + expect(compiled.source).toBe(clause); + }); + } +}); + +describe("compileWhen — malformed clauses throw WhenParseError", () => { + const badClauses = [ + "(a && b", + "a && b)", + "a &&", + "&& a", + "a ||", + "a == 'ts' extra", + "a #b", + "== 'ts'", + "", + " ", + "a =='unterminated", + "a == ts", + "!", + "a && !", + ]; + + for (const clause of badClauses) { + test(`throws on: ${JSON.stringify(clause)}`, () => { + expect(() => compileWhen(clause)).toThrow(WhenParseError); + }); + } + + test("WhenParseError message includes the offending clause", () => { + try { + compileWhen("a &&"); + throw new Error("expected compileWhen to throw"); + } catch (err) { + expect(err).toBeInstanceOf(WhenParseError); + expect((err as WhenParseError).clause).toBe("a &&"); + expect((err as WhenParseError).message).toContain("a &&"); + } + }); +}); + +describe("compileWhen — AST-cache guarantee", () => { + test("parses the clause exactly once, regardless of how many times evaluate is called", () => { + const parseSpy = spyOn(__whenTestHooks, "parse"); + const before = parseSpy.mock.calls.length; + + const compiled = compileWhen("editorTextFocus && editorLangId == 'ts'"); + expect(parseSpy.mock.calls.length).toBe(before + 1); + + for (let i = 0; i < 5; i++) { + compiled.evaluate(contextOf({ editorTextFocus: true, editorLangId: "ts" })); + } + + expect(parseSpy.mock.calls.length).toBe(before + 1); + parseSpy.mockRestore(); + }); + + test("two separate compileWhen calls each parse once, independently", () => { + const parseSpy = spyOn(__whenTestHooks, "parse"); + const before = parseSpy.mock.calls.length; + + compileWhen("a"); + compileWhen("b"); + + expect(parseSpy.mock.calls.length).toBe(before + 2); + parseSpy.mockRestore(); + }); +}); diff --git a/packages/core/src/keymap/when.ts b/packages/core/src/keymap/when.ts new file mode 100644 index 0000000..2bbab67 --- /dev/null +++ b/packages/core/src/keymap/when.ts @@ -0,0 +1,334 @@ +/** + * The when-clause parser and evaluator (Req 4.5, design.md §6.4). A tiny + * recursive-descent grammar over context-key equality and boolean + * combinators: + * + * ``` + * expr := or + * or := and ("||" and)* + * and := unary ("&&" unary)* + * unary := "!" unary | primary + * primary:= key | key "==" value | "(" expr ")" + * ``` + * + * {@link compileWhen} parses a clause exactly once into a {@link WhenNode} + * AST and returns a {@link CompiledWhen} whose `evaluate` re-walks that + * cached AST — the binding table (Task 1.5) compiles each `when` once at + * registration and evaluates it on every keystroke without re-parsing. + */ + +/* ------------------------------------------------------------------ */ +/* AST */ +/* ------------------------------------------------------------------ */ + +/** A bare context-key reference (`editorTextFocus`), true when the + * context value is truthy. */ +export interface WhenKeyNode { + readonly kind: "key"; + readonly key: string; +} + +/** A string-equality test (`editorLangId == 'ts'`). The grammar only + * admits a quoted string literal on the right of `==`, so `value` is a + * plain `string`, not `unknown` — the *comparison* is against an + * `unknown` context value at evaluation time. */ +export interface WhenEqNode { + readonly kind: "eq"; + readonly key: string; + readonly value: string; +} + +/** Logical negation (`!editorFocus`). */ +export interface WhenNotNode { + readonly kind: "not"; + readonly operand: WhenNode; +} + +/** Logical AND (`a && b`), higher precedence than `||`. */ +export interface WhenAndNode { + readonly kind: "and"; + readonly left: WhenNode; + readonly right: WhenNode; +} + +/** Logical OR (`a || b`), lowest precedence. */ +export interface WhenOrNode { + readonly kind: "or"; + readonly left: WhenNode; + readonly right: WhenNode; +} + +/** The when-clause AST — a discriminated union on `kind` (design.md §6.4). */ +export type WhenNode = WhenKeyNode | WhenEqNode | WhenNotNode | WhenAndNode | WhenOrNode; + +/** + * Thrown when a `when` clause fails to tokenize or parse. The message + * names both what went wrong and the offending clause; callers (the + * keybinding table, Task 1.5) are expected to catch this, log it, and + * skip the binding rather than let it crash the process. + */ +export class WhenParseError extends Error { + /** The full clause text that failed to parse. */ + readonly clause: string; + + constructor(reason: string, clause: string) { + super(`Invalid when clause "${clause}": ${reason}`); + this.name = "WhenParseError"; + this.clause = clause; + } +} + +/* ------------------------------------------------------------------ */ +/* Tokenizer */ +/* ------------------------------------------------------------------ */ + +type TokenKind = "id" | "string" | "==" | "&&" | "||" | "!" | "(" | ")" | "eof"; + +interface Token { + readonly kind: TokenKind; + readonly value: string; + /** Character offset in the source clause, for error messages. */ + readonly pos: number; +} + +const ID_START = /[A-Za-z_]/; +const ID_CONT = /[A-Za-z0-9_.]/; + +/** Split `clause` into a token stream, always terminated by a single + * `"eof"` sentinel token — this lets the parser index the token array + * without manual bounds checks (`noUncheckedIndexedAccess` is off + * project-wide, so the sentinel is what actually keeps lookahead safe). */ +function tokenize(clause: string): Token[] { + const tokens: Token[] = []; + const n = clause.length; + let i = 0; + + while (i < n) { + const ch = clause[i] as string; + + if (ch === " " || ch === "\t" || ch === "\n" || ch === "\r") { + i++; + continue; + } + if (ch === "(" || ch === ")") { + tokens.push({ kind: ch, value: ch, pos: i }); + i++; + continue; + } + if (ch === "!") { + tokens.push({ kind: "!", value: "!", pos: i }); + i++; + continue; + } + if (ch === "&" && clause[i + 1] === "&") { + tokens.push({ kind: "&&", value: "&&", pos: i }); + i += 2; + continue; + } + if (ch === "|" && clause[i + 1] === "|") { + tokens.push({ kind: "||", value: "||", pos: i }); + i += 2; + continue; + } + if (ch === "=" && clause[i + 1] === "=") { + tokens.push({ kind: "==", value: "==", pos: i }); + i += 2; + continue; + } + if (ch === "'" || ch === '"') { + const quote = ch; + let j = i + 1; + let value = ""; + while (j < n && clause[j] !== quote) { + value += clause[j]; + j++; + } + if (j >= n) { + throw new WhenParseError( + `unterminated string literal starting at position ${i}`, + clause, + ); + } + tokens.push({ kind: "string", value, pos: i }); + i = j + 1; + continue; + } + if (ID_START.test(ch)) { + let j = i + 1; + while (j < n && ID_CONT.test(clause[j] as string)) j++; + tokens.push({ kind: "id", value: clause.slice(i, j), pos: i }); + i = j; + continue; + } + + throw new WhenParseError(`unrecognized character "${ch}" at position ${i}`, clause); + } + + tokens.push({ kind: "eof", value: "", pos: n }); + return tokens; +} + +/** Render a token for an error message. */ +function describeToken(token: Token): string { + if (token.kind === "eof") return "end of input"; + if (token.kind === "string") return `string literal "${token.value}"`; + return `"${token.value}"`; +} + +/* ------------------------------------------------------------------ */ +/* Recursive-descent parser */ +/* ------------------------------------------------------------------ */ + +/** Parse a full when clause into a {@link WhenNode}, per the §6.4 grammar. + * Throws {@link WhenParseError} on any lexical or syntactic problem, + * including trailing input after a complete expression. */ +function parseWhen(clause: string): WhenNode { + const tokens = tokenize(clause); + let pos = 0; + + const peek = (): Token => tokens[pos] as Token; // safe: eof sentinel + const advance = (): Token => { + const token = peek(); + pos++; + return token; + }; + + function parseOr(): WhenNode { + let left = parseAnd(); + while (peek().kind === "||") { + advance(); + const right = parseAnd(); + left = { kind: "or", left, right }; + } + return left; + } + + function parseAnd(): WhenNode { + let left = parseUnary(); + while (peek().kind === "&&") { + advance(); + const right = parseUnary(); + left = { kind: "and", left, right }; + } + return left; + } + + function parseUnary(): WhenNode { + if (peek().kind === "!") { + advance(); + return { kind: "not", operand: parseUnary() }; + } + return parsePrimary(); + } + + function parsePrimary(): WhenNode { + const token = peek(); + + if (token.kind === "(") { + advance(); + const inner = parseOr(); + const close = peek(); + if (close.kind !== ")") { + throw new WhenParseError(`expected ")" but found ${describeToken(close)}`, clause); + } + advance(); + return inner; + } + + if (token.kind === "id") { + advance(); + if (peek().kind === "==") { + advance(); + const value = peek(); + if (value.kind !== "string") { + throw new WhenParseError( + `expected a string literal after "==" but found ${describeToken(value)}`, + clause, + ); + } + advance(); + return { kind: "eq", key: token.value, value: value.value }; + } + return { kind: "key", key: token.value }; + } + + throw new WhenParseError( + `expected a context key, "!", or "(" but found ${describeToken(token)}`, + clause, + ); + } + + const ast = parseOr(); + const trailing = peek(); + if (trailing.kind !== "eof") { + throw new WhenParseError(`unexpected trailing input at ${describeToken(trailing)}`, clause); + } + return ast; +} + +/** + * Indirection around {@link parseWhen} used only so `when.test.ts` can + * spy on it to prove {@link compileWhen}'s AST-cache guarantee (parse + * once, evaluate many times) without exporting the parser as public API. + * Not part of the module's public surface — `packages/core/src/keymap/index.ts` + * does not re-export it. + */ +export const __whenTestHooks = { parse: parseWhen }; + +/* ------------------------------------------------------------------ */ +/* Evaluation */ +/* ------------------------------------------------------------------ */ + +/** Reads a single context value by key, as supplied by the context + * service (`tecode.context.get`, Req 4.6). Unknown keys return + * `undefined`. */ +export type WhenContextGetter = (key: string) => unknown; + +function evaluateNode(node: WhenNode, get: WhenContextGetter): boolean { + switch (node.kind) { + case "key": + return Boolean(get(node.key)); + case "eq": { + const actual = get(node.key); + // Unknown keys resolve `undefined` and never satisfy `==` (design.md + // §6.4) — checked explicitly so a clause like `x == 'undefined'` + // can't accidentally match an unset key. + if (actual === undefined) return false; + return String(actual) === node.value; + } + case "not": + return !evaluateNode(node.operand, get); + case "and": + return evaluateNode(node.left, get) && evaluateNode(node.right, get); + case "or": + return evaluateNode(node.left, get) || evaluateNode(node.right, get); + } +} + +/** A `when` clause parsed once into a cached {@link WhenNode}, ready to be + * evaluated repeatedly against a context getter. */ +export interface CompiledWhen { + /** The original clause text, e.g. `"editorTextFocus && !explorerFocus"`. */ + readonly source: string; + /** Evaluate the compiled AST against `get`. Never throws — the AST is + * already known-valid; unknown keys are simply falsy. */ + evaluate(get: WhenContextGetter): boolean; +} + +/** + * Compile a `when` clause (Req 4.5, design.md §6.4). Parses `clause` + * exactly once and returns a {@link CompiledWhen} whose `evaluate` walks + * the cached AST — safe to call on every keystroke. Throws + * {@link WhenParseError} if `clause` is not valid; callers that register + * many bindings (Task 1.5) should catch that per-binding and skip it + * rather than let one bad clause abort startup. + */ +export function compileWhen(clause: string): CompiledWhen { + const ast = __whenTestHooks.parse(clause); + return { + source: clause, + evaluate(get: WhenContextGetter): boolean { + return evaluateNode(ast, get); + }, + }; +} From 9fb2ae95b8da7a3194c2e8114422ed863eadabb4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 12:16:17 +0000 Subject: [PATCH 2/4] Treat Symbol context values as not-equal in when eq evaluation String(Symbol) throws TypeError, which would break evaluate's never-throwing contract on every keystroke; a Symbol can never equal a string literal, so return false. Adds a regression test, per CodeRabbit review. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- packages/core/src/keymap/when.test.ts | 8 ++++++++ packages/core/src/keymap/when.ts | 4 ++++ 2 files changed, 12 insertions(+) diff --git a/packages/core/src/keymap/when.test.ts b/packages/core/src/keymap/when.test.ts index fcbf786..b7dab2e 100644 --- a/packages/core/src/keymap/when.test.ts +++ b/packages/core/src/keymap/when.test.ts @@ -203,3 +203,11 @@ describe("compileWhen — AST-cache guarantee", () => { parseSpy.mockRestore(); }); }); + +test("eq against a Symbol context value is false rather than throwing", () => { + const compiled = compileWhen("editorLangId == 'ts'"); + const get = (key: string) => + key === "editorLangId" ? Symbol("ts") : undefined; + + expect(compiled.evaluate(get)).toBe(false); +}); diff --git a/packages/core/src/keymap/when.ts b/packages/core/src/keymap/when.ts index 2bbab67..844ccc2 100644 --- a/packages/core/src/keymap/when.ts +++ b/packages/core/src/keymap/when.ts @@ -294,6 +294,10 @@ function evaluateNode(node: WhenNode, get: WhenContextGetter): boolean { // §6.4) — checked explicitly so a clause like `x == 'undefined'` // can't accidentally match an unset key. if (actual === undefined) return false; + // Symbols can't be stringified (String() throws TypeError) and can + // never equal a string literal — treat as not-equal to keep + // evaluate's never-throwing contract. + if (typeof actual === "symbol") return false; return String(actual) === node.value; } case "not": From 0109e11837ddaa442d388e8d8eee83ca142d8426 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 12:18:43 +0000 Subject: [PATCH 3/4] Apply CodeRabbit nitpicks on when/context PR - compileWhen calls parseWhen directly; the mutable __whenTestHooks object is replaced by a read-only @internal parse counter, so nothing on the production parse path can be altered by consumers - context change dispatch isolates listener exceptions so one throwing listener cannot stop the others or propagate out of set(); regression test added Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- packages/core/src/keymap/context.test.ts | 15 +++++++++++++++ packages/core/src/keymap/context.ts | 7 ++++++- packages/core/src/keymap/when.test.ts | 18 +++++++----------- packages/core/src/keymap/when.ts | 22 +++++++++++++++------- 4 files changed, 43 insertions(+), 19 deletions(-) diff --git a/packages/core/src/keymap/context.test.ts b/packages/core/src/keymap/context.test.ts index 1250380..e341100 100644 --- a/packages/core/src/keymap/context.test.ts +++ b/packages/core/src/keymap/context.test.ts @@ -102,3 +102,18 @@ test("distinct keys are stored independently", () => { expect(context.get("editorFocus")).toBe(true); expect(context.get("explorerFocus")).toBe(false); }); + +test("a throwing listener does not stop other listeners or break set", () => { + const context = createContextService(); + const seen: string[] = []; + context.onDidChange(() => { + throw new Error("bad listener"); + }); + context.onDidChange((key) => { + seen.push(key); + }); + + expect(() => context.set("editorFocus", true)).not.toThrow(); + expect(seen).toEqual(["editorFocus"]); + expect(context.get("editorFocus")).toBe(true); +}); diff --git a/packages/core/src/keymap/context.ts b/packages/core/src/keymap/context.ts index 58ff0ca..a830f93 100644 --- a/packages/core/src/keymap/context.ts +++ b/packages/core/src/keymap/context.ts @@ -46,7 +46,12 @@ export function createContextService(): ContextService { // Snapshot before iterating: a listener that disposes itself (or // another listener) during the loop must not perturb this dispatch. for (const listener of Array.from(listeners)) { - listener(key); + try { + listener(key); + } catch { + // Isolate listener failures: one throwing listener must not stop + // the remaining listeners or propagate out of set(). + } } } diff --git a/packages/core/src/keymap/when.test.ts b/packages/core/src/keymap/when.test.ts index b7dab2e..59369e5 100644 --- a/packages/core/src/keymap/when.test.ts +++ b/packages/core/src/keymap/when.test.ts @@ -1,5 +1,5 @@ -import { describe, expect, spyOn, test } from "bun:test"; -import { compileWhen, WhenParseError, __whenTestHooks } from "./when"; +import { describe, expect, test } from "bun:test"; +import { compileWhen, WhenParseError, __getParseCountForTests } from "./when"; /** Build a context getter from a plain object, for table-driven tests. */ function contextOf(values: Record): (key: string) => unknown { @@ -178,29 +178,25 @@ describe("compileWhen — malformed clauses throw WhenParseError", () => { describe("compileWhen — AST-cache guarantee", () => { test("parses the clause exactly once, regardless of how many times evaluate is called", () => { - const parseSpy = spyOn(__whenTestHooks, "parse"); - const before = parseSpy.mock.calls.length; + const before = __getParseCountForTests(); const compiled = compileWhen("editorTextFocus && editorLangId == 'ts'"); - expect(parseSpy.mock.calls.length).toBe(before + 1); + expect(__getParseCountForTests()).toBe(before + 1); for (let i = 0; i < 5; i++) { compiled.evaluate(contextOf({ editorTextFocus: true, editorLangId: "ts" })); } - expect(parseSpy.mock.calls.length).toBe(before + 1); - parseSpy.mockRestore(); + expect(__getParseCountForTests()).toBe(before + 1); }); test("two separate compileWhen calls each parse once, independently", () => { - const parseSpy = spyOn(__whenTestHooks, "parse"); - const before = parseSpy.mock.calls.length; + const before = __getParseCountForTests(); compileWhen("a"); compileWhen("b"); - expect(parseSpy.mock.calls.length).toBe(before + 2); - parseSpy.mockRestore(); + expect(__getParseCountForTests()).toBe(before + 2); }); }); diff --git a/packages/core/src/keymap/when.ts b/packages/core/src/keymap/when.ts index 844ccc2..c640ba7 100644 --- a/packages/core/src/keymap/when.ts +++ b/packages/core/src/keymap/when.ts @@ -183,6 +183,7 @@ function describeToken(token: Token): string { * Throws {@link WhenParseError} on any lexical or syntactic problem, * including trailing input after a complete expression. */ function parseWhen(clause: string): WhenNode { + parseCount += 1; const tokens = tokenize(clause); let pos = 0; @@ -266,14 +267,21 @@ function parseWhen(clause: string): WhenNode { return ast; } +/** Total {@link parseWhen} invocations since module load — read-only + * observability for the AST-cache tests; nothing in the production path + * can be altered through it. */ +let parseCount = 0; + /** - * Indirection around {@link parseWhen} used only so `when.test.ts` can - * spy on it to prove {@link compileWhen}'s AST-cache guarantee (parse - * once, evaluate many times) without exporting the parser as public API. - * Not part of the module's public surface — `packages/core/src/keymap/index.ts` - * does not re-export it. + * @internal Test-only, read-only: how many times the parser has run since + * module load. Lets `when.test.ts` prove {@link compileWhen}'s AST-cache + * guarantee (parse once, evaluate many times) without exposing a mutable + * hook on the production parse path. Not re-exported from + * `packages/core/src/keymap/index.ts`. */ -export const __whenTestHooks = { parse: parseWhen }; +export function __getParseCountForTests(): number { + return parseCount; +} /* ------------------------------------------------------------------ */ /* Evaluation */ @@ -328,7 +336,7 @@ export interface CompiledWhen { * rather than let one bad clause abort startup. */ export function compileWhen(clause: string): CompiledWhen { - const ast = __whenTestHooks.parse(clause); + const ast = parseWhen(clause); return { source: clause, evaluate(get: WhenContextGetter): boolean { From c319d2297e85d3b9668f8b9e4c0c4e4d72d9d8f3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 12:43:57 +0000 Subject: [PATCH 4/4] Guard eq stringification against non-convertible values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit String() also throws for values with no usable primitive conversion (e.g. null-prototype objects), not just Symbols — wrap the comparison in try/catch returning false so evaluate keeps its never-throwing contract. Adds an Object.create(null) regression test, per CodeRabbit review. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- packages/core/src/keymap/when.test.ts | 8 ++++++++ packages/core/src/keymap/when.ts | 9 ++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/packages/core/src/keymap/when.test.ts b/packages/core/src/keymap/when.test.ts index 59369e5..ac136a3 100644 --- a/packages/core/src/keymap/when.test.ts +++ b/packages/core/src/keymap/when.test.ts @@ -207,3 +207,11 @@ test("eq against a Symbol context value is false rather than throwing", () => { expect(compiled.evaluate(get)).toBe(false); }); + +test("eq against a value with no primitive conversion is false rather than throwing", () => { + const compiled = compileWhen("editorLangId == 'ts'"); + const get = (key: string) => + key === "editorLangId" ? Object.create(null) : undefined; + + expect(compiled.evaluate(get)).toBe(false); +}); diff --git a/packages/core/src/keymap/when.ts b/packages/core/src/keymap/when.ts index c640ba7..e8dcab8 100644 --- a/packages/core/src/keymap/when.ts +++ b/packages/core/src/keymap/when.ts @@ -306,7 +306,14 @@ function evaluateNode(node: WhenNode, get: WhenContextGetter): boolean { // never equal a string literal — treat as not-equal to keep // evaluate's never-throwing contract. if (typeof actual === "symbol") return false; - return String(actual) === node.value; + try { + return String(actual) === node.value; + } catch { + // Values with no usable primitive conversion (e.g. a + // null-prototype object) also make String() throw — same + // treatment: not equal, never throw. + return false; + } } case "not": return !evaluateNode(node.operand, get);