From 9ad0415d2c58aa83e32cb43d8a0282e62f5e9b67 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 16:48:34 +0000 Subject: [PATCH 1/3] Add configuration service: JSONC parsing, OS paths, layered settings, file watching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements Task 1.10 (Issue #11, Req 9, design.md §11): the config service backing tecode.config. - host/paths.ts: OS-dependent config-directory resolution (POSIX ~/.config/tecode/, Windows %APPDATA%\tecode\), confined to this module so no other code branches on process.platform. Derives the user settings.json / keybindings.json paths and a workspace .tecode/settings.json path. - config/jsonc.ts: a small hand-written, never-throwing JSONC parser (no dependency, per design.md §11/§15) that strips // and block comments plus trailing commas while tracking string-literal state (escaped quotes included), then delegates to JSON.parse. Failures report a message with a best-effort 1-based line/column; Bun's JSON.parse carries no position info, so that path documents and falls back to line 1, column 1 while still attempting to recover a V8-style "position N" when present. - config/service.ts: createConfigService(deps) layers defaults (from registerConfiguration's ConfigurationContribution schemas) under user settings.json under workspace .tecode/settings.json (later wins), exposes get()/onDidChange (ConfigChangeEvent with a dot-boundary-aware affectsConfiguration), watches all three files plus keybindings.json via an injectable fs seam (defaulting to node:fs/promises + node:fs.watch, guarded against watching a missing file), diffs merged keys structurally so a reload that reproduces identical values fires no event, keeps the last-good layer on a parse error while reporting through the log/sink, and best-effort validates value types against registered schemas (mismatch => warning, still served). Initialization returns synchronously and exposes a `ready` promise rather than making the factory async, matching every other createX() in core. - Wires both modules into their barrels, replacing the CONFIG_PLACEHOLDER export. Tests: 50 new (21 jsonc, 7 paths, 22 service — including a fake-fs suite covering layering, defaults, live reload, affectsConfiguration boundary cases, parse-error resilience, and type-mismatch warnings, plus one real-filesystem watch integration test). Full suite: 300 pass, 0 fail. `bun run lint` and `tsc --noEmit` clean across every package. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- packages/core/src/config/index.ts | 18 +- packages/core/src/config/jsonc.test.ts | 165 +++++++ packages/core/src/config/jsonc.ts | 220 +++++++++ packages/core/src/config/service.test.ts | 567 ++++++++++++++++++++++ packages/core/src/config/service.ts | 568 +++++++++++++++++++++++ packages/core/src/host/index.ts | 7 + packages/core/src/host/paths.test.ts | 73 +++ packages/core/src/host/paths.ts | 43 ++ packages/core/src/index.ts | 11 +- 9 files changed, 1669 insertions(+), 3 deletions(-) create mode 100644 packages/core/src/config/jsonc.test.ts create mode 100644 packages/core/src/config/jsonc.ts create mode 100644 packages/core/src/config/service.test.ts create mode 100644 packages/core/src/config/service.ts create mode 100644 packages/core/src/host/paths.test.ts create mode 100644 packages/core/src/host/paths.ts diff --git a/packages/core/src/config/index.ts b/packages/core/src/config/index.ts index b2589b5..1ea30c6 100644 --- a/packages/core/src/config/index.ts +++ b/packages/core/src/config/index.ts @@ -1,2 +1,16 @@ -// Placeholder for settings/keybindings loading, JSONC parser, file watcher. -export const CONFIG_PLACEHOLDER = true; +// The configuration service (design.md §11, Req 9): a hand-written JSONC +// parser, the layered (defaults ← user ← workspace) settings service, and +// its file watching, live reload, and raw keybindings access. + +export { + parseJsonc, + type JsoncFailure, + type JsoncParseResult, + type JsoncSuccess, +} from "./jsonc"; +export { + createConfigService, + type ConfigService, + type ConfigServiceDeps, + type ConfigServiceFs, +} from "./service"; diff --git a/packages/core/src/config/jsonc.test.ts b/packages/core/src/config/jsonc.test.ts new file mode 100644 index 0000000..ff1e2b0 --- /dev/null +++ b/packages/core/src/config/jsonc.test.ts @@ -0,0 +1,165 @@ +import { describe, expect, test } from "bun:test"; +import { parseJsonc } from "./jsonc"; + +describe("parseJsonc — happy paths", () => { + test("parses plain JSON with no comments or trailing commas", () => { + const result = parseJsonc('{"a": 1, "b": [1, 2, 3]}'); + expect(result.ok).toBe(true); + if (result.ok) expect(result.value).toEqual({ a: 1, b: [1, 2, 3] }); + }); + + test("empty input is a parse failure (not valid JSON), never throws", () => { + const result = parseJsonc(""); + expect(result.ok).toBe(false); + }); + + test("whitespace-only input is a parse failure, never throws", () => { + const result = parseJsonc(" \n\t "); + expect(result.ok).toBe(false); + }); + + test("strips a line comment", () => { + const result = parseJsonc(`{ + // this is a comment + "a": 1 + }`); + expect(result.ok).toBe(true); + if (result.ok) expect(result.value).toEqual({ a: 1 }); + }); + + test("strips a trailing line comment after a value", () => { + const result = parseJsonc('{"a": 1 // trailing\n}'); + expect(result.ok).toBe(true); + if (result.ok) expect(result.value).toEqual({ a: 1 }); + }); + + test("strips a block comment", () => { + const result = parseJsonc('{ /* comment */ "a": 1 }'); + expect(result.ok).toBe(true); + if (result.ok) expect(result.value).toEqual({ a: 1 }); + }); + + test("strips a multi-line block comment", () => { + const result = parseJsonc(`{ + /* + * multi + * line + */ + "a": 1 + }`); + expect(result.ok).toBe(true); + if (result.ok) expect(result.value).toEqual({ a: 1 }); + }); + + test("strips a trailing comma in an object", () => { + const result = parseJsonc('{"a": 1, "b": 2,}'); + expect(result.ok).toBe(true); + if (result.ok) expect(result.value).toEqual({ a: 1, b: 2 }); + }); + + test("strips a trailing comma in an array", () => { + const result = parseJsonc("[1, 2, 3,]"); + expect(result.ok).toBe(true); + if (result.ok) expect(result.value).toEqual([1, 2, 3]); + }); + + test("strips a trailing comma across whitespace and a newline", () => { + const result = parseJsonc('{\n "a": 1,\n}'); + expect(result.ok).toBe(true); + if (result.ok) expect(result.value).toEqual({ a: 1 }); + }); + + test("strips nested trailing commas in objects and arrays together", () => { + const result = parseJsonc('{"a": [1, 2,], "b": {"c": 3,},}'); + expect(result.ok).toBe(true); + if (result.ok) expect(result.value).toEqual({ a: [1, 2], b: { c: 3 } }); + }); + + test("combines comments and trailing commas", () => { + const result = parseJsonc(`{ + // editor settings + "editor.tabSize": 2, // spaces + /* block */ "editor.insertSpaces": true, + }`); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.value).toEqual({ + "editor.tabSize": 2, + "editor.insertSpaces": true, + }); + } + }); +}); + +describe("parseJsonc — strings survive comment/comma-like content", () => { + test("a string containing // is not treated as a comment", () => { + const result = parseJsonc('{"url": "https://example.com"}'); + expect(result.ok).toBe(true); + if (result.ok) expect(result.value).toEqual({ url: "https://example.com" }); + }); + + test("a string containing /* is not treated as a comment start", () => { + const result = parseJsonc('{"note": "look: /* not a comment */ ok"}'); + expect(result.ok).toBe(true); + if (result.ok) expect(result.value).toEqual({ note: "look: /* not a comment */ ok" }); + }); + + test("a string containing a comma before a closing brace is preserved", () => { + const result = parseJsonc('{"note": "a, b,"}'); + expect(result.ok).toBe(true); + if (result.ok) expect(result.value).toEqual({ note: "a, b," }); + }); + + test("an escaped quote inside a string does not end the string early", () => { + const result = parseJsonc('{"note": "she said \\"hi\\", // not a comment"}'); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.value).toEqual({ note: 'she said "hi", // not a comment' }); + } + }); + + test("a backslash-escaped backslash before a quote does not confuse string end", () => { + const result = parseJsonc('{"path": "C:\\\\", "ok": 1}'); + expect(result.ok).toBe(true); + if (result.ok) expect(result.value).toEqual({ path: "C:\\", ok: 1 }); + }); +}); + +describe("parseJsonc — failure reporting", () => { + test("broken input reports ok:false with a message and a 1-based line/column", () => { + const result = parseJsonc("{ this is not json }"); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(typeof result.message).toBe("string"); + expect(result.message.length).toBeGreaterThan(0); + expect(result.line).toBeGreaterThanOrEqual(1); + expect(result.column).toBeGreaterThanOrEqual(1); + } + }); + + test("an unterminated object reports a failure, not a throw", () => { + expect(() => parseJsonc('{"a": 1')).not.toThrow(); + const result = parseJsonc('{"a": 1'); + expect(result.ok).toBe(false); + }); + + test("a stray trailing comma-comment combo that leaves invalid JSON still fails cleanly", () => { + const result = parseJsonc('{"a": 1,, "b": 2}'); + expect(result.ok).toBe(false); + }); + + test("never throws across a wide variety of garbage input", () => { + const garbageInputs = [ + "{{{{{", + "]]]]", + '"unterminated string', + "/* unterminated block comment", + "// just a comment", + "null null", + "\u0000\u0001", + ]; + for (const input of garbageInputs) { + expect(() => parseJsonc(input)).not.toThrow(); + } + }); +}); diff --git a/packages/core/src/config/jsonc.ts b/packages/core/src/config/jsonc.ts new file mode 100644 index 0000000..767f7f5 --- /dev/null +++ b/packages/core/src/config/jsonc.ts @@ -0,0 +1,220 @@ +/** + * A small, hand-written, tolerant JSONC parser (Req 9.1, design.md §11, + * §15): strips `//` line comments, block comments, and trailing + * commas — all while tracking string-literal state so a comment marker or + * comma that merely *appears inside a string* survives untouched — then + * delegates to `JSON.parse`. No JSON/JSONC library dependency (house + * convention, design.md §11/§15): this is the whole implementation. + * + * Never throws: {@link parseJsonc} always returns a + * {@link JsoncParseResult}, so callers (the config service) can report a + * failure through a `StatusSink` instead of crashing. + */ + +/** A successful parse: the decoded value. */ +export interface JsoncSuccess { + ok: true; + value: T; +} + +/** A failed parse: a message plus the best-effort 1-based line/column of + * the problem, for forwarding to a `StatusSink`/status bar (Req 9.1, + * design.md §14). */ +export interface JsoncFailure { + ok: false; + message: string; + /** 1-based line number. */ + line: number; + /** 1-based column number. */ + column: number; +} + +export type JsoncParseResult = JsoncSuccess | JsoncFailure; + +/** + * Blank out `//` line comments and block comments in `text`, replacing every commented + * character with a space (newlines inside block comments are preserved as + * newlines). Tracks JSON string-literal state (with `\"`-escape awareness) + * so `//`/`/*` sequences *inside* a string are left untouched. + * + * Replacing rather than deleting keeps every remaining character at its + * original offset, so line/column arithmetic on the sanitized text matches + * the original file exactly — this is what lets {@link lineColAt} below + * work directly against the post-strip text. + */ +function stripComments(text: string): string { + const n = text.length; + const out: string[] = new Array(n); + let i = 0; + let inString = false; + + while (i < n) { + const ch = text[i]!; + + if (inString) { + out[i] = ch; + if (ch === "\\" && i + 1 < n) { + // Copy the escaped character verbatim too (notably `\"`, which + // must not be mistaken for the string's closing quote). + out[i + 1] = text[i + 1]!; + i += 2; + continue; + } + if (ch === '"') inString = false; + i++; + continue; + } + + if (ch === '"') { + inString = true; + out[i] = ch; + i++; + continue; + } + + if (ch === "/" && text[i + 1] === "/") { + while (i < n && text[i] !== "\n") { + out[i] = " "; + i++; + } + continue; + } + + if (ch === "/" && text[i + 1] === "*") { + out[i] = " "; + out[i + 1] = " "; + i += 2; + while (i < n && !(text[i] === "*" && text[i + 1] === "/")) { + out[i] = text[i] === "\n" ? "\n" : " "; + i++; + } + if (i < n) { + // Blank the closing `*/` itself. + out[i] = " "; + out[i + 1] = " "; + i += 2; + } + // An unterminated block comment (no closing `*/`) blanks to EOF — + // JSON.parse reports whatever remains (typically nothing useful), + // which is an acceptable MVP outcome for a malformed file. + continue; + } + + out[i] = ch; + i++; + } + + return out.join(""); +} + +/** + * Blank a trailing comma — one whose next non-whitespace character (outside + * a string) is `}` or `]` — to a space, again preserving every other + * character's offset. Must run *after* {@link stripComments} so a comma + * inside a now-blanked comment can never be mistaken for a real one. + */ +function stripTrailingCommas(text: string): string { + const n = text.length; + const out = text.split(""); + let inString = false; + + for (let i = 0; i < n; i++) { + const ch = out[i]!; + + if (inString) { + if (ch === "\\") { + i++; + continue; + } + if (ch === '"') inString = false; + continue; + } + + if (ch === '"') { + inString = true; + continue; + } + + if (ch === ",") { + let j = i + 1; + while (j < n && /\s/.test(out[j]!)) j++; + if (j < n && (out[j] === "}" || out[j] === "]")) { + out[i] = " "; + } + } + } + + return out.join(""); +} + +/** 1-based line/column of `offset` within `text`. */ +function lineColAt(text: string, offset: number): { line: number; column: number } { + let line = 1; + let column = 1; + const end = Math.min(offset, text.length); + for (let i = 0; i < end; i++) { + if (text[i] === "\n") { + line++; + column = 1; + } else { + column++; + } + } + return { line, column }; +} + +/** Render a caught `unknown` as a message string without risking a second + * throw (matches registry.ts's/documentManager.ts's `describeError`). */ +function describeError(err: unknown): string { + try { + if (err instanceof Error) return err.message; + return String(err); + } catch { + return "Unknown error"; + } +} + +/** + * Best-effort extraction of a character offset from a `JSON.parse` error + * message. Engines built on V8 (Node, and Bun's `Response`/`fetch` JSON + * paths) sometimes format this as `"... at position N"`; Bun's own + * `JSON.parse` (JavaScriptCore) does not include a position at all as of + * this writing. When no offset can be recovered, {@link parseJsonc} falls + * back to reporting line 1, column 1 alongside the raw message — still + * useful for the status bar, just not pinpoint-accurate (documented + * trade-off, design.md §11). + */ +function extractOffset(message: string): number | undefined { + const match = /position\s+(\d+)/i.exec(message); + if (!match) return undefined; + const offset = Number(match[1]); + return Number.isFinite(offset) ? offset : undefined; +} + +/** + * Parse JSONC text: comments and trailing commas are stripped (Req 9.1), + * then the result is handed to `JSON.parse`. Never throws — parse failures + * come back as a {@link JsoncFailure} with a 1-based line/column the caller + * can forward to a `StatusSink` (design.md §11, §14). + */ +export function parseJsonc(text: string): JsoncParseResult { + let sanitized: string; + try { + sanitized = stripTrailingCommas(stripComments(text)); + } catch (cause) { + // Stripping is pure string scanning and should never throw, but a + // parser is a public boundary (house convention) — guard it anyway. + return { ok: false, message: describeError(cause), line: 1, column: 1 }; + } + + try { + const value = JSON.parse(sanitized) as T; + return { ok: true, value }; + } catch (cause) { + const message = describeError(cause); + const offset = extractOffset(message); + const { line, column } = + offset === undefined ? { line: 1, column: 1 } : lineColAt(sanitized, offset); + return { ok: false, message, line, column }; + } +} diff --git a/packages/core/src/config/service.test.ts b/packages/core/src/config/service.test.ts new file mode 100644 index 0000000..acf4032 --- /dev/null +++ b/packages/core/src/config/service.test.ts @@ -0,0 +1,567 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { ConfigChangeEvent } from "@tecode/api"; +import type { HostError } from "../host/errors"; +import { createHostLog } from "../host/errors"; +import { + getUserKeybindingsPath, + getUserSettingsPath, + getWorkspaceSettingsPath, +} from "../host/paths"; +import { createConfigService, type ConfigServiceFs } from "./service"; + +/** A `StatusSink` stub that records every error it receives (matches + * documentManager.test.ts's `createRecordingSink`). */ +function createRecordingSink() { + const errors: HostError[] = []; + return { + errors, + sink: { + error(err: HostError) { + errors.push(err); + }, + }, + }; +} + +/** An in-memory {@link ConfigServiceFs}: `readFile` serves whatever + * `setFile` last stored (or ENOENT), and `triggerChange` synchronously + * invokes every listener registered via `watch` for that path — the + * "injected fake fs/watcher where the test fires the change callback + * synchronously" seam the task plan calls for. */ +function createFakeFs(initial: Record = {}): { + fs: ConfigServiceFs; + setFile(path: string, content: string): void; + deleteFile(path: string): void; + triggerChange(path: string): void; + watchedPaths(): string[]; +} { + const files = new Map(Object.entries(initial)); + const watchers = new Map void>>(); + return { + fs: { + async readFile(path) { + const content = files.get(path); + if (content === undefined) { + throw Object.assign(new Error(`ENOENT: ${path}`), { code: "ENOENT" }); + } + return content; + }, + watch(path, onChange) { + let set = watchers.get(path); + if (!set) { + set = new Set(); + watchers.set(path, set); + } + set.add(onChange); + return { + close() { + set?.delete(onChange); + }, + }; + }, + }, + setFile(path, content) { + files.set(path, content); + }, + deleteFile(path) { + files.delete(path); + }, + triggerChange(path) { + const set = watchers.get(path); + if (set) for (const cb of Array.from(set)) cb(); + }, + watchedPaths() { + return Array.from(watchers.keys()); + }, + }; +} + +/** Poll `predicate` until it is true or `timeoutMs` elapses, yielding + * between checks — used instead of a single fixed sleep so reload chains + * (fake-fs: microtask-only; real-fs: genuine watch latency) settle + * reliably without over- or under-waiting. */ +async function waitFor(predicate: () => boolean, timeoutMs = 2000): Promise { + const start = Date.now(); + while (!predicate()) { + if (Date.now() - start > timeoutMs) { + throw new Error("waitFor: timed out waiting for predicate"); + } + await new Promise((resolve) => setTimeout(resolve, 5)); + } +} + +describe("ConfigService.get — layering (Req 9.2, 9.3)", () => { + test("workspace overrides user overrides defaults, per key", async () => { + const userPath = getUserSettingsPath(); + const workspaceRoot = "/fake-workspace"; + const workspacePath = getWorkspaceSettingsPath(workspaceRoot); + const fake = createFakeFs({ + [userPath]: JSON.stringify({ "editor.tabSize": 2, "editor.wordWrap": "off" }), + [workspacePath]: JSON.stringify({ "editor.tabSize": 4 }), + }); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, workspaceRoot, fs: fake.fs }); + service.registerConfiguration({ + properties: { + "editor.tabSize": { type: "number", default: 8 }, + "editor.insertSpaces": { type: "boolean", default: true }, + }, + }); + await service.ready; + + expect(service.get("editor.tabSize")).toBe(4); // workspace wins over user and defaults + expect(service.get("editor.wordWrap")).toBe("off"); // user only, no workspace override + expect(service.get("editor.insertSpaces")).toBe(true); // defaults only + service.dispose(); + }); + + test("with no workspaceRoot, only defaults and user settings apply", async () => { + const userPath = getUserSettingsPath(); + const fake = createFakeFs({ [userPath]: JSON.stringify({ "editor.tabSize": 2 }) }); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + expect(service.get("editor.tabSize")).toBe(2); + service.dispose(); + }); + + test("get on a missing key returns undefined", async () => { + const fake = createFakeFs(); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + expect(service.get("nothing.here")).toBeUndefined(); + service.dispose(); + }); + + test("a missing settings file is treated as an empty layer, not an error", async () => { + const fake = createFakeFs(); // no files at all + const log = createHostLog(); + const { sink, errors } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + expect(errors).toHaveLength(0); + expect(log.entries()).toHaveLength(0); + service.dispose(); + }); +}); + +describe("ConfigService.registerConfiguration (Req 9.3)", () => { + test("populates defaults for properties that declare one", async () => { + const fake = createFakeFs(); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + service.registerConfiguration({ + properties: { + "widget.enabled": { type: "boolean", default: true }, + "widget.count": { type: "number", default: 3 }, + "widget.noDefault": { type: "string" }, + }, + }); + + expect(service.get("widget.enabled")).toBe(true); + expect(service.get("widget.count")).toBe(3); + expect(service.get("widget.noDefault")).toBeUndefined(); + service.dispose(); + }); + + test("disposing the registration removes its defaults again", async () => { + const fake = createFakeFs(); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + const reg = service.registerConfiguration({ + properties: { "widget.enabled": { type: "boolean", default: true } }, + }); + expect(service.get("widget.enabled")).toBe(true); + + reg.dispose(); + expect(service.get("widget.enabled")).toBeUndefined(); + + // Idempotent. + expect(() => reg.dispose()).not.toThrow(); + service.dispose(); + }); +}); + +describe("ConfigService — live reload via watch (Req 9.4)", () => { + test("a changed user setting fires onDidChange with correct affectsConfiguration", async () => { + const userPath = getUserSettingsPath(); + const fake = createFakeFs({ [userPath]: JSON.stringify({ "editor.tabSize": 2 }) }); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + const events: ConfigChangeEvent[] = []; + service.onDidChange((e) => events.push(e)); + + fake.setFile(userPath, JSON.stringify({ "editor.tabSize": 4 })); + fake.triggerChange(userPath); + await waitFor(() => events.length > 0); + + expect(service.get("editor.tabSize")).toBe(4); + expect(events).toHaveLength(1); + const event = events[0]!; + + // equal key + expect(event.affectsConfiguration("editor.tabSize")).toBe(true); + // section is an ancestor of the changed key (a coarser query) + expect(event.affectsConfiguration("editor")).toBe(true); + // the changed key is an ancestor of a finer, hypothetical descendant query + expect(event.affectsConfiguration("editor.tabSize.nested")).toBe(true); + // bare-prefix collision without a dot boundary must not match + expect(event.affectsConfiguration("editorX")).toBe(false); + + service.dispose(); + }); + + test("a reload that reproduces identical values fires no event", async () => { + const userPath = getUserSettingsPath(); + const fake = createFakeFs({ [userPath]: JSON.stringify({ "editor.tabSize": 2 }) }); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + const events: unknown[] = []; + service.onDidChange(() => events.push("fired")); + + // Rewritten with byte-identical JSON content: JSON.parse still yields + // a fresh object reference, but every value is structurally equal — + // no onDidChange should fire. + fake.setFile(userPath, JSON.stringify({ "editor.tabSize": 2 })); + fake.triggerChange(userPath); + + // Give the reload chain a chance to run before asserting silence — + // there is no "fired" event to wait for here, so wait for the reload + // to actually complete via a side channel: re-triggering with a real + // change and waiting for *that* event proves the first reload had + // already finished (reload chains are per-file and FIFO). + fake.setFile(userPath, JSON.stringify({ "editor.tabSize": 5 })); + fake.triggerChange(userPath); + await waitFor(() => events.length > 0); + + expect(events).toHaveLength(1); // only the second, real change fired + expect(service.get("editor.tabSize")).toBe(5); + service.dispose(); + }); + + test("a deep-equal array/object value reloading does not fire a spurious event", async () => { + const userPath = getUserSettingsPath(); + const fake = createFakeFs({ + [userPath]: JSON.stringify({ "files.exclude": ["a", "b"], "editor.opts": { x: 1 } }), + }); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + const events: unknown[] = []; + service.onDidChange(() => events.push("fired")); + + fake.setFile( + userPath, + JSON.stringify({ "files.exclude": ["a", "b"], "editor.opts": { x: 1 }, extra: "z" }), + ); + fake.triggerChange(userPath); + await waitFor(() => service.get("extra") === "z"); + + // Only "extra" changed — the deep-equal array/object values must not + // have produced a change too (implicitly verified: no throw/hang), and + // exactly one event fired. + expect(events).toHaveLength(1); + service.dispose(); + }); + + test("workspace file changes fire events scoped only to the workspace layer", async () => { + const userPath = getUserSettingsPath(); + const workspaceRoot = "/fake-workspace-2"; + const workspacePath = getWorkspaceSettingsPath(workspaceRoot); + const fake = createFakeFs({ + [userPath]: JSON.stringify({ "editor.tabSize": 2 }), + [workspacePath]: JSON.stringify({}), + }); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, workspaceRoot, fs: fake.fs }); + await service.ready; + + const events: ConfigChangeEvent[] = []; + service.onDidChange((e) => events.push(e)); + + fake.setFile(workspacePath, JSON.stringify({ "explorer.showHidden": true })); + fake.triggerChange(workspacePath); + await waitFor(() => events.length > 0); + + expect(service.get("explorer.showHidden")).toBe(true); + expect(events[0]!.affectsConfiguration("explorer.showHidden")).toBe(true); + expect(events[0]!.affectsConfiguration("editor.tabSize")).toBe(false); + service.dispose(); + }); +}); + +describe("ConfigService — parse-error resilience (Req 9, design.md §14)", () => { + test("a parse error on reload keeps the last-good layer and reports line/column via the sink", async () => { + const userPath = getUserSettingsPath(); + const fake = createFakeFs({ [userPath]: JSON.stringify({ "editor.tabSize": 2 }) }); + const log = createHostLog(); + const { sink, errors } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + expect(service.get("editor.tabSize")).toBe(2); + + const events: unknown[] = []; + service.onDidChange(() => events.push("fired")); + + fake.setFile(userPath, "{ this is not valid json"); + fake.triggerChange(userPath); + await waitFor(() => errors.length > 0); + + expect(service.get("editor.tabSize")).toBe(2); // unchanged — last-good kept + expect(events).toHaveLength(0); // merged view never touched, no event + const reported = errors.at(-1)!; + expect(reported.message).toMatch(/line \d+, column \d+/); + const errorEntries = log.entries().filter((e) => e.level === "error"); + expect(errorEntries.length).toBeGreaterThan(0); + + service.dispose(); + }); + + test("a top-level non-object settings file is rejected and the last-good layer is kept", async () => { + const userPath = getUserSettingsPath(); + const fake = createFakeFs({ [userPath]: JSON.stringify({ "editor.tabSize": 2 }) }); + const log = createHostLog(); + const { sink, errors } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + fake.setFile(userPath, JSON.stringify([1, 2, 3])); + fake.triggerChange(userPath); + await waitFor(() => errors.length > 0); + + expect(service.get("editor.tabSize")).toBe(2); + service.dispose(); + }); + + test("keybindings.json is required to be a top-level array", async () => { + const keybindingsPath = getUserKeybindingsPath(); + const fake = createFakeFs({ [keybindingsPath]: JSON.stringify({ not: "an array" }) }); + const log = createHostLog(); + const { sink, errors } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + expect(service.getKeybindingEntries()).toEqual([]); + expect(errors.length).toBeGreaterThan(0); + service.dispose(); + }); +}); + +describe("ConfigService — best-effort type validation (Req 9.3 MVP policy)", () => { + test("a value whose type mismatches its schema is served, and logged as a warning", async () => { + const userPath = getUserSettingsPath(); + const fake = createFakeFs({ + [userPath]: JSON.stringify({ "editor.tabSize": "not-a-number" }), + }); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + service.registerConfiguration({ + properties: { "editor.tabSize": { type: "number", default: 4 } }, + }); + await service.ready; + + // Served anyway — the MVP never rejects a value, only warns. + expect(service.get("editor.tabSize")).toBe("not-a-number"); + const warnings = log.entries().filter((e) => e.level === "warning"); + expect(warnings.some((w) => w.error.message.includes("editor.tabSize"))).toBe(true); + service.dispose(); + }); + + test("a value matching its schema type produces no warning", async () => { + const userPath = getUserSettingsPath(); + const fake = createFakeFs({ [userPath]: JSON.stringify({ "editor.tabSize": 4 }) }); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + service.registerConfiguration({ + properties: { "editor.tabSize": { type: "number", default: 4 } }, + }); + await service.ready; + + const warnings = log.entries().filter((e) => e.level === "warning"); + expect(warnings).toHaveLength(0); + service.dispose(); + }); +}); + +describe("ConfigService — keybindings (Req 9.1)", () => { + test("getKeybindingEntries reflects the parsed keybindings.json array", async () => { + const keybindingsPath = getUserKeybindingsPath(); + const entries = [{ key: "ctrl+s", command: "workspace.save" }]; + const fake = createFakeFs({ [keybindingsPath]: JSON.stringify(entries) }); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + expect(service.getKeybindingEntries()).toEqual(entries); + service.dispose(); + }); + + test("onKeybindingsChange fires on initial load and again on reload", async () => { + const keybindingsPath = getUserKeybindingsPath(); + const fake = createFakeFs({ + [keybindingsPath]: JSON.stringify([{ key: "a", command: "x" }]), + }); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const calls: (readonly unknown[])[] = []; + const service = createConfigService({ + log, + sink, + fs: fake.fs, + onKeybindingsChange: (entries) => calls.push(entries), + }); + await service.ready; + expect(calls).toHaveLength(1); + expect(calls[0]).toEqual([{ key: "a", command: "x" }]); + + fake.setFile(keybindingsPath, JSON.stringify([{ key: "b", command: "y" }])); + fake.triggerChange(keybindingsPath); + await waitFor(() => calls.length > 1); + + expect(calls[1]).toEqual([{ key: "b", command: "y" }]); + service.dispose(); + }); + + test("a throwing onKeybindingsChange callback does not break loading", async () => { + const keybindingsPath = getUserKeybindingsPath(); + const fake = createFakeFs({ [keybindingsPath]: JSON.stringify([]) }); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ + log, + sink, + fs: fake.fs, + onKeybindingsChange: () => { + throw new Error("boom"); + }, + }); + await expect(service.ready).resolves.toBeUndefined(); + const errorEntries = log.entries().filter((e) => e.level === "error"); + expect(errorEntries.some((e) => e.error.message.includes("boom"))).toBe(true); + service.dispose(); + }); +}); + +describe("ConfigService.dispose", () => { + test("closes watchers; a change after dispose is not observed", async () => { + const userPath = getUserSettingsPath(); + const fake = createFakeFs({ [userPath]: JSON.stringify({ "editor.tabSize": 2 }) }); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + const events: unknown[] = []; + service.onDidChange(() => events.push("fired")); + + service.dispose(); + fake.setFile(userPath, JSON.stringify({ "editor.tabSize": 99 })); + fake.triggerChange(userPath); + + // No watcher left to observe the change; give any stray async work a + // moment to (not) run, then assert nothing happened. + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(events).toHaveLength(0); + expect(service.get("editor.tabSize")).toBe(2); + }); + + test("dispose is idempotent", async () => { + const fake = createFakeFs(); + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + service.dispose(); + expect(() => service.dispose()).not.toThrow(); + }); +}); + +describe("ConfigService — a watch on a nonexistent file does not throw (Req 9)", () => { + test("a watch() that throws synchronously is caught, logged, and does not break load", async () => { + const userPath = getUserSettingsPath(); + const fake = createFakeFs({ [userPath]: JSON.stringify({ "editor.tabSize": 2 }) }); + const throwingFs: ConfigServiceFs = { + readFile: fake.fs.readFile, + watch(path) { + throw Object.assign(new Error(`ENOENT: ${path}`), { code: "ENOENT" }); + }, + }; + const log = createHostLog(); + const { sink } = createRecordingSink(); + const service = createConfigService({ log, sink, fs: throwingFs }); + + await expect(service.ready).resolves.toBeUndefined(); + expect(service.get("editor.tabSize")).toBe(2); + const warnings = log.entries().filter((e) => e.level === "warning"); + expect(warnings.length).toBeGreaterThan(0); + service.dispose(); + }); +}); + +describe("ConfigService — real filesystem integration (design.md §16)", () => { + let dir: string; + + afterEach(async () => { + if (dir) await rm(dir, { recursive: true, force: true }); + }); + + test("reads a real workspace settings.json and reloads on a real fs.watch change", async () => { + dir = await mkdtemp(join(tmpdir(), "tecode-config-svc-")); + const tecodeDir = join(dir, ".tecode"); + await mkdir(tecodeDir, { recursive: true }); + const settingsPath = join(tecodeDir, "settings.json"); + await writeFile(settingsPath, JSON.stringify({ "editor.tabSize": 2 }), "utf8"); + + const log = createHostLog(); + const { sink } = createRecordingSink(); + // No fs override: exercises the real node:fs/promises + node:fs.watch + // default. The user-level paths (~/.config/tecode/...) point at real + // OS paths too; a missing user config is the expected, harmless case + // (ENOENT -> empty layer). + const service = createConfigService({ log, sink, workspaceRoot: dir }); + await service.ready; + + expect(service.get("editor.tabSize")).toBe(2); + + const events: ConfigChangeEvent[] = []; + service.onDidChange((e) => events.push(e)); + + await writeFile(settingsPath, JSON.stringify({ "editor.tabSize": 4 }), "utf8"); + + // fs.watch delivery can be slow/coalesced — poll with a deadline + // rather than a single fixed sleep. + await waitFor(() => service.get("editor.tabSize") === 4, 10_000); + expect(events.some((e) => e.affectsConfiguration("editor.tabSize"))).toBe(true); + + service.dispose(); + }, 15_000); +}); diff --git a/packages/core/src/config/service.ts b/packages/core/src/config/service.ts new file mode 100644 index 0000000..5596bdf --- /dev/null +++ b/packages/core/src/config/service.ts @@ -0,0 +1,568 @@ +/** + * The configuration service (Req 9, design.md §11): layers defaults (from + * `contributes.configuration` schemas) under the user's `settings.json` + * under a workspace's `.tecode/settings.json` (later wins), backs + * `tecode.config.get`/`onDidChange`, and watches all three files (plus the + * user's `keybindings.json`) for live reload. + * + * Built with {@link createConfigService} rather than a class (house + * convention — matches `createCommandRegistry`, `createDocumentManager`, + * `createContextService`). + * + * **Initialization design choice**: `createConfigService` returns + * synchronously (it does no I/O before returning), then kicks off the + * initial file reads and watcher setup in the background. Callers that need + * to know the initial load has settled — e.g. the host, before it opens the + * initial file/directory (design.md §17's startup order: "load + * configuration" first) — `await` the returned `ready` promise; `get()` + * itself never blocks and is always safe to call, returning whatever the + * merged view currently holds (just schema defaults, before `ready` + * settles). This mirrors the rest of core: no `createX` factory is `async`, + * so a caller composing several services together never needs to sequence + * `await`s just to wire them up. + */ + +import { readFile as nodeReadFile } from "node:fs/promises"; +import { watch as nodeWatch } from "node:fs"; +import type { + ConfigChangeEvent, + ConfigurationContribution, + ConfigurationPropertySchema, + Disposable, + Event, + Listener, +} from "@tecode/api"; +import type { HostError, HostLog, StatusSink } from "../host/errors"; +import { + getUserKeybindingsPath, + getUserSettingsPath, + getWorkspaceSettingsPath, +} from "../host/paths"; +import { parseJsonc } from "./jsonc"; + +/** + * The narrow filesystem seam {@link createConfigService} needs: reading a + * file's text and watching it for changes. Exists as an injectable seam + * (defaulting to `node:fs/promises` + `node:fs`'s `watch`) so tests can + * simulate reads/changes deterministically, without touching the real + * filesystem or real watch latency (matches `documentManager.ts`'s + * `DocumentManagerFs` seam). Not part of the public API surface. + */ +export interface ConfigServiceFs { + readFile(path: string): Promise; + /** Start watching `path`; `onChange` is called (with no arguments) on + * every change the underlying watcher reports. Returns a handle whose + * `close()` stops watching. */ + watch(path: string, onChange: () => void): { close(): void }; +} + +function createNodeConfigFs(): ConfigServiceFs { + return { + readFile: (path) => nodeReadFile(path, "utf8"), + watch: (path, onChange) => { + const watcher = nodeWatch(path, () => onChange()); + return { + close() { + watcher.close(); + }, + }; + }, + }; +} + +/** Dependencies for {@link createConfigService}. */ +export interface ConfigServiceDeps { + /** Structured log for parse errors and type-mismatch warnings (design.md + * §14). */ + log: HostLog; + /** Where user-facing config errors are surfaced (Req 9, design.md §14). */ + sink: StatusSink; + /** The open workspace's root directory. The workspace settings layer + * (`/.tecode/settings.json`, Req 9.2) is only active when + * this is provided — a single-file session with no workspace has no + * third layer. */ + workspaceRoot?: string; + /** Filesystem seam — see {@link ConfigServiceFs}. Defaults to + * `node:fs/promises` + `node:fs.watch`. */ + fs?: ConfigServiceFs; + /** Called (guarded) after the user keybindings file is first loaded and + * again after every successful reload, with the raw parsed entries. The + * real keymap-layer wiring lands in a later task (design.md §11); this is + * just the hook it will attach to. */ + onKeybindingsChange?: (entries: readonly unknown[]) => void; +} + +/** The config service — the implementation behind `tecode.config`, plus the + * schema registry and raw keybindings access that only core-internal + * callers (the extension host, the keymap service) need. */ +export interface ConfigService { + /** Read a key from the merged (defaults ← user ← workspace) view (Req + * 9.3). Keys are flat, dot-separated strings (e.g. `"editor.tabSize"`) — + * settings files are flat objects keyed this way; a JSON object *value* + * (e.g. for a `"type": "object"` schema) is stored as-is under its one + * key, not split into further path segments (design.md §11). */ + get(key: string): T | undefined; + /** Fires whenever a live reload changes the merged view (Req 9.4). Never + * fires for a reload that reproduces identical values. */ + onDidChange: Event; + /** + * Register a schema (Req 9.3): each property's `default` (when present) + * populates the defaults layer, and its `type` is remembered for + * best-effort validation (MVP policy — see {@link ConfigService.get}'s + * TSDoc: a user/workspace value whose `typeof` mismatches the declared + * type is logged as a warning but still served, never rejected). + * Returns a {@link Disposable} that removes this contribution's defaults + * and schemas again. + */ + registerConfiguration(contribution: ConfigurationContribution): Disposable; + /** The raw entries currently parsed from the user's `keybindings.json` + * (an array of whatever shape the file holds — this service does not + * interpret keybinding entries, only loads/watches the file). Empty + * when the file is absent or fails to parse. */ + getKeybindingEntries(): readonly unknown[]; + /** Resolves once the initial read of all three settings files (and + * `keybindings.json`) has completed and watchers are armed — see this + * module's top-of-file TSDoc for why this is a promise rather than an + * `async` factory. */ + ready: Promise; + /** Close every file watcher. Idempotent. */ + dispose(): void; +} + +/** Extract an errno-style `code` (e.g. `"ENOENT"`) from a caught unknown + * (matches `documentManager.ts`'s `errorCode`). */ +function errorCode(err: unknown): string | undefined { + if (typeof err === "object" && err !== null && "code" in err) { + const code = (err as { code?: unknown }).code; + if (typeof code === "string") return code; + } + return undefined; +} + +/** Render a caught `unknown` as a message string without risking a second + * throw (matches `registry.ts`'s/`documentManager.ts`'s `describeError`). */ +function describeError(err: unknown): string { + try { + if (err instanceof Error) return err.message; + return String(err); + } catch { + return "Unknown error"; + } +} + +/** Structural equality over JSON-shaped values (objects/arrays/primitives) + * — needed because re-parsing an unchanged file produces new object/array + * references even when nothing actually changed, and a reload must only + * fire `onDidChange` for keys whose *value* changed (Req 9.4). */ +function deepEqual(a: unknown, b: unknown): boolean { + if (Object.is(a, b)) return true; + if (typeof a !== typeof b) return false; + if (a === null || b === null || a === undefined || b === undefined) return false; + if (Array.isArray(a) || Array.isArray(b)) { + if (!Array.isArray(a) || !Array.isArray(b) || a.length !== b.length) return false; + return a.every((v, i) => deepEqual(v, b[i])); + } + if (typeof a === "object" && typeof b === "object") { + const ak = Object.keys(a as Record); + const bk = Object.keys(b as Record); + if (ak.length !== bk.length) return false; + return ak.every((k) => + deepEqual((a as Record)[k], (b as Record)[k]), + ); + } + return false; +} + +/** Whether `value`'s runtime shape matches a schema's declared `type`. */ +function matchesSchemaType( + value: unknown, + type: ConfigurationPropertySchema["type"], +): boolean { + switch (type) { + case "string": + return typeof value === "string"; + case "number": + return typeof value === "number"; + case "boolean": + return typeof value === "boolean"; + case "array": + return Array.isArray(value); + case "object": + return typeof value === "object" && value !== null && !Array.isArray(value); + } +} + +/** Whether `key` (a changed config key) affects `section` (a queried key): + * equal, `key` is a dot-descendant of `section`, or `section` is a + * dot-descendant of `key` — but never a bare-prefix match with no dot + * boundary (design.md §11: `"editor.tabSize"` affects `"editor"` and + * `"editor.tabSize"` but not `"editorX"`). */ +function keyAffectsSection(key: string, section: string): boolean { + return ( + key === section || key.startsWith(`${section}.`) || section.startsWith(`${key}.`) + ); +} + +/** + * Build a config service (Req 9). `deps.log`/`deps.sink` are required; + * everything else is optional (see {@link ConfigServiceDeps}). + */ +export function createConfigService(deps: ConfigServiceDeps): ConfigService { + const { log, sink, workspaceRoot } = deps; + const fs = deps.fs ?? createNodeConfigFs(); + + const userSettingsPath = getUserSettingsPath(); + const workspaceSettingsPath = workspaceRoot + ? getWorkspaceSettingsPath(workspaceRoot) + : undefined; + const keybindingsPath = getUserKeybindingsPath(); + + const schemas = new Map(); + const defaultsLayer: Record = {}; + let userLayer: Record = {}; + let workspaceLayer: Record = {}; + let merged: Record = {}; + let keybindingEntries: unknown[] = []; + + const changeListeners = new Set>(); + const watcherHandles: { close(): void }[] = []; + let disposed = false; + + /** Guarded `log.append` (matches `registry.ts`'s `logSafely`). */ + function logSafely(level: "error" | "warning", err: HostError): void { + try { + log.append(level, err); + } catch { + // Swallowed: reporting a reporting failure has nowhere left to go. + } + } + + /** Guarded `sink.error` (matches `registry.ts`'s `notifySafely`). */ + function notifySafely(err: HostError): void { + try { + sink.error(err); + } catch { + // Swallowed — see logSafely. + } + } + + function computeMerged(): Record { + return { ...defaultsLayer, ...userLayer, ...workspaceLayer }; + } + + function diffChangedKeys( + before: Record, + after: Record, + ): string[] { + const keys = new Set([...Object.keys(before), ...Object.keys(after)]); + const changed: string[] = []; + for (const key of keys) { + if (!deepEqual(before[key], after[key])) changed.push(key); + } + return changed; + } + + function fireChange(changedKeys: string[]): void { + const event: ConfigChangeEvent = { + affectsConfiguration(section: string) { + return changedKeys.some((key) => keyAffectsSection(key, section)); + }, + }; + // Snapshot before iterating: a listener that disposes itself (or + // another listener) mid-dispatch must not perturb this loop (matches + // keymap/context.ts's onDidChange pattern). + for (const listener of Array.from(changeListeners)) { + try { + listener(event); + } catch (cause) { + logSafely("error", { + message: `ConfigService onDidChange listener threw: ${describeError(cause)}`, + }); + } + } + } + + /** Recompute the merged view and, when any key's value actually changed, + * fire `onDidChange` (Req 9.4). Called after every layer update + * (defaults registration, or a successful reload). */ + function rebuildMerged(): void { + const next = computeMerged(); + const changed = diffChangedKeys(merged, next); + merged = next; + if (changed.length > 0) fireChange(changed); + } + + /** Best-effort schema-type validation for one freshly loaded layer (Req + * 9.3 MVP policy): a mismatch is logged as a warning and the value is + * still served — never rejected, never blocks the layer from loading. */ + function validateLayerTypes(layer: Record, layerLabel: string): void { + for (const [key, value] of Object.entries(layer)) { + const schema = schemas.get(key); + if (!schema) continue; + if (!matchesSchemaType(value, schema.type)) { + logSafely("warning", { + message: + `Config value for "${key}" in ${layerLabel} does not match its declared ` + + `type "${schema.type}"; serving it anyway (MVP policy, design.md §11).`, + }); + } + } + } + + /** Read + parse one settings file into a flat layer object. Returns the + * new layer (possibly `{}` for a missing file) on success, or `undefined` + * on any failure — the caller keeps whatever layer it already had (Req + * 9's "keep last-good configuration" policy). Every failure path reports + * through `log`/`sink`. */ + async function loadSettingsLayer( + path: string, + label: string, + ): Promise | undefined> { + let text: string; + try { + text = await fs.readFile(path); + } catch (cause) { + if (errorCode(cause) === "ENOENT") return {}; + const message = `Failed to read ${label} (${path}): ${describeError(cause)}`; + logSafely("error", { message, path }); + notifySafely({ message, path }); + return undefined; + } + + const parsed = parseJsonc(text); + if (!parsed.ok) { + const message = `${label} (${path}) line ${parsed.line}, column ${parsed.column}: ${parsed.message}`; + logSafely("error", { message, path }); + notifySafely({ message, path }); + return undefined; + } + if (typeof parsed.value !== "object" || parsed.value === null || Array.isArray(parsed.value)) { + const message = `${label} (${path}) must be a JSON object at the top level`; + logSafely("error", { message, path }); + notifySafely({ message, path }); + return undefined; + } + + const layer = parsed.value as Record; + validateLayerTypes(layer, label); + return layer; + } + + /** Read + parse `keybindings.json` into a raw entry array. Same + * keep-last-good-on-failure contract as {@link loadSettingsLayer}. */ + async function loadKeybindingsLayer(path: string): Promise { + let text: string; + try { + text = await fs.readFile(path); + } catch (cause) { + if (errorCode(cause) === "ENOENT") return []; + const message = `Failed to read user keybindings (${path}): ${describeError(cause)}`; + logSafely("error", { message, path }); + notifySafely({ message, path }); + return undefined; + } + + const parsed = parseJsonc(text); + if (!parsed.ok) { + const message = `user keybindings (${path}) line ${parsed.line}, column ${parsed.column}: ${parsed.message}`; + logSafely("error", { message, path }); + notifySafely({ message, path }); + return undefined; + } + if (!Array.isArray(parsed.value)) { + const message = `user keybindings (${path}) must be a JSON array at the top level`; + logSafely("error", { message, path }); + notifySafely({ message, path }); + return undefined; + } + return parsed.value; + } + + function invokeKeybindingsHook(): void { + if (!deps.onKeybindingsChange) return; + try { + deps.onKeybindingsChange(keybindingEntries.slice()); + } catch (cause) { + logSafely("error", { + message: `onKeybindingsChange callback threw: ${describeError(cause)}`, + }); + } + } + + async function reloadUserSettings(): Promise { + if (disposed) return; + const next = await loadSettingsLayer(userSettingsPath, "user settings"); + if (next !== undefined) { + userLayer = next; + rebuildMerged(); + } + } + + async function reloadWorkspaceSettings(): Promise { + if (disposed || !workspaceSettingsPath) return; + const next = await loadSettingsLayer(workspaceSettingsPath, "workspace settings"); + if (next !== undefined) { + workspaceLayer = next; + rebuildMerged(); + } + } + + async function reloadKeybindings(): Promise { + if (disposed) return; + const next = await loadKeybindingsLayer(keybindingsPath); + if (next !== undefined) { + keybindingEntries = next; + invokeKeybindingsHook(); + } + } + + // Per-file reload chains: two watch events for the same file firing in + // quick succession must not run overlapping reads, or a slower-to-finish + // older read could land after — and clobber — a newer one's result. + let userReloadChain: Promise = Promise.resolve(); + let workspaceReloadChain: Promise = Promise.resolve(); + let keybindingsReloadChain: Promise = Promise.resolve(); + + function scheduleUserReload(): void { + userReloadChain = userReloadChain.then(reloadUserSettings, reloadUserSettings); + } + function scheduleWorkspaceReload(): void { + workspaceReloadChain = workspaceReloadChain.then( + reloadWorkspaceSettings, + reloadWorkspaceSettings, + ); + } + function scheduleKeybindingsReload(): void { + keybindingsReloadChain = keybindingsReloadChain.then( + reloadKeybindings, + reloadKeybindings, + ); + } + + /** Start watching one file, guarding against `fs.watch` throwing when the + * path does not (yet) exist. Documented MVP limitation: a file that + * appears later is not picked up automatically — the watch attempt is + * only made once, here, at startup — needing a reload/restart instead + * (design.md §11 does not require inotify-on-parent-directory tracking + * for the MVP). */ + function watchFile(path: string, onChange: () => void, label: string): void { + try { + const handle = fs.watch(path, onChange); + if (disposed) { + // A dispose() landed while this synchronous call was in flight + // (impossible in practice given JS's single-threaded execution, + // but cheap to guard) — do not leak the handle. + try { + handle.close(); + } catch { + // Best-effort. + } + return; + } + watcherHandles.push(handle); + } catch (cause) { + logSafely("warning", { + message: + `Could not watch ${label} (${path}) for changes: ${describeError(cause)}. ` + + `If this file is created later, reload/restart tecode to pick it up (MVP limitation).`, + path, + }); + } + } + + function startWatchers(): void { + if (disposed) return; + watchFile(userSettingsPath, scheduleUserReload, "user settings"); + if (workspaceSettingsPath) { + watchFile(workspaceSettingsPath, scheduleWorkspaceReload, "workspace settings"); + } + watchFile(keybindingsPath, scheduleKeybindingsReload, "user keybindings"); + } + + async function initialLoad(): Promise { + const [userResult, workspaceResult, keybindingsResult] = await Promise.all([ + loadSettingsLayer(userSettingsPath, "user settings"), + workspaceSettingsPath + ? loadSettingsLayer(workspaceSettingsPath, "workspace settings") + : Promise.resolve>({}), + loadKeybindingsLayer(keybindingsPath), + ]); + if (disposed) return; + userLayer = userResult ?? {}; + workspaceLayer = workspaceResult ?? {}; + keybindingEntries = keybindingsResult ?? []; + // Initial build: set directly rather than going through rebuildMerged + // — there is no meaningful "previous" state to diff against yet, and + // no listener could have subscribed before this promise was even + // returned to the caller, so no onDidChange fires for startup. + merged = computeMerged(); + invokeKeybindingsHook(); + startWatchers(); + } + + function get(key: string): T | undefined { + return merged[key] as T | undefined; + } + + function onDidChange(listener: Listener): Disposable { + changeListeners.add(listener); + let listenerDisposed = false; + return { + dispose() { + if (listenerDisposed) return; + listenerDisposed = true; + changeListeners.delete(listener); + }, + }; + } + + function registerConfiguration(contribution: ConfigurationContribution): Disposable { + const keys: string[] = []; + for (const [key, schema] of Object.entries(contribution.properties)) { + schemas.set(key, schema); + keys.push(key); + if ("default" in schema) { + defaultsLayer[key] = schema.default; + } + } + rebuildMerged(); + + let regDisposed = false; + return { + dispose() { + if (regDisposed) return; + regDisposed = true; + for (const key of keys) { + schemas.delete(key); + delete defaultsLayer[key]; + } + rebuildMerged(); + }, + }; + } + + function getKeybindingEntries(): readonly unknown[] { + return keybindingEntries.slice(); + } + + function dispose(): void { + disposed = true; + for (const handle of watcherHandles.splice(0)) { + try { + handle.close(); + } catch { + // Best-effort: a watcher that fails to close cleanly is not worth + // surfacing — dispose() has nowhere to report it either. + } + } + } + + return { + get, + onDidChange, + registerConfiguration, + getKeybindingEntries, + ready: initialLoad(), + dispose, + }; +} diff --git a/packages/core/src/host/index.ts b/packages/core/src/host/index.ts index 11f7e32..b04c20e 100644 --- a/packages/core/src/host/index.ts +++ b/packages/core/src/host/index.ts @@ -12,6 +12,13 @@ export { type StatusSink, } from "./errors"; +export { + getUserConfigDir, + getUserKeybindingsPath, + getUserSettingsPath, + getWorkspaceSettingsPath, +} from "./paths"; + /** Placeholder for the remaining extension-host behavior (discovery, * manifest validation, activation) — see design.md §4. */ export const HOST_PLACEHOLDER = true; diff --git a/packages/core/src/host/paths.test.ts b/packages/core/src/host/paths.test.ts new file mode 100644 index 0000000..4530a64 --- /dev/null +++ b/packages/core/src/host/paths.test.ts @@ -0,0 +1,73 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { homedir } from "node:os"; +import { join } from "node:path"; +import { + getUserConfigDir, + getUserKeybindingsPath, + getUserSettingsPath, + getWorkspaceSettingsPath, +} from "./paths"; + +const originalPlatform = process.platform; +const originalAppData = process.env["APPDATA"]; + +function setPlatform(platform: NodeJS.Platform): void { + Object.defineProperty(process, "platform", { value: platform }); +} + +afterEach(() => { + setPlatform(originalPlatform); + if (originalAppData === undefined) delete process.env["APPDATA"]; + else process.env["APPDATA"] = originalAppData; +}); + +describe("getUserConfigDir — POSIX", () => { + beforeEach(() => setPlatform("linux")); + + test("resolves to ~/.config/tecode", () => { + expect(getUserConfigDir()).toBe(join(homedir(), ".config", "tecode")); + }); + + test("darwin also uses ~/.config/tecode (no macOS-specific branch)", () => { + setPlatform("darwin"); + expect(getUserConfigDir()).toBe(join(homedir(), ".config", "tecode")); + }); +}); + +describe("getUserConfigDir — Windows", () => { + beforeEach(() => setPlatform("win32")); + + test("resolves to %APPDATA%\\tecode when APPDATA is set", () => { + process.env["APPDATA"] = "C:\\Users\\test\\AppData\\Roaming"; + expect(getUserConfigDir()).toBe( + join("C:\\Users\\test\\AppData\\Roaming", "tecode"), + ); + }); + + test("falls back to ~/AppData/Roaming/tecode when APPDATA is unset", () => { + delete process.env["APPDATA"]; + expect(getUserConfigDir()).toBe( + join(homedir(), "AppData", "Roaming", "tecode"), + ); + }); +}); + +describe("derived file paths", () => { + beforeEach(() => setPlatform("linux")); + + test("getUserSettingsPath appends settings.json to the config dir", () => { + expect(getUserSettingsPath()).toBe(join(getUserConfigDir(), "settings.json")); + }); + + test("getUserKeybindingsPath appends keybindings.json to the config dir", () => { + expect(getUserKeybindingsPath()).toBe( + join(getUserConfigDir(), "keybindings.json"), + ); + }); + + test("getWorkspaceSettingsPath appends .tecode/settings.json to the workspace root", () => { + expect(getWorkspaceSettingsPath("/home/user/project")).toBe( + join("/home/user/project", ".tecode", "settings.json"), + ); + }); +}); diff --git a/packages/core/src/host/paths.ts b/packages/core/src/host/paths.ts new file mode 100644 index 0000000..0d9136d --- /dev/null +++ b/packages/core/src/host/paths.ts @@ -0,0 +1,43 @@ +/** + * OS-dependent configuration-directory resolution (Req 9.1, design.md §11), + * confined entirely to this module: every other module that needs a config + * path calls one of these helpers rather than branching on + * `process.platform` itself. + * + * - POSIX (macOS, Linux, ...): `~/.config/tecode/`. + * - Windows: `%APPDATA%\tecode\`, falling back to `~/AppData/Roaming/tecode` + * when `APPDATA` is unset (rare, but not impossible in a stripped-down + * shell). + */ + +import { homedir } from "node:os"; +import { join } from "node:path"; + +/** The user-level tecode configuration directory for the current OS (Req + * 9.1). Does not create the directory or check that it exists — callers + * that need it to exist do that themselves. */ +export function getUserConfigDir(): string { + if (process.platform === "win32") { + const appData = process.env["APPDATA"]; + if (appData) return join(appData, "tecode"); + return join(homedir(), "AppData", "Roaming", "tecode"); + } + return join(homedir(), ".config", "tecode"); +} + +/** Path to the user-level `settings.json` (Req 9.1). */ +export function getUserSettingsPath(): string { + return join(getUserConfigDir(), "settings.json"); +} + +/** Path to the user-level `keybindings.json` (Req 9.1). */ +export function getUserKeybindingsPath(): string { + return join(getUserConfigDir(), "keybindings.json"); +} + +/** Path to a workspace's `.tecode/settings.json`, overlaid on top of user + * settings when the workspace declares one (Req 9.2). `workspaceRoot` is + * the workspace's root directory (an absolute path). */ +export function getWorkspaceSettingsPath(workspaceRoot: string): string { + return join(workspaceRoot, ".tecode", "settings.json"); +} diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 7d2c107..60cfbe3 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -59,5 +59,14 @@ export { type UndoStackDeps, } from "./buffer/index"; export { UI_PLACEHOLDER } from "./ui/index"; -export { CONFIG_PLACEHOLDER } from "./config/index"; +export { + createConfigService, + parseJsonc, + type ConfigService, + type ConfigServiceDeps, + type ConfigServiceFs, + type JsoncFailure, + type JsoncParseResult, + type JsoncSuccess, +} from "./config/index"; export { API_BUILDER_PLACEHOLDER } from "./api/index"; From 2cb387bad3917d448c71d30d71cba840322eb9a9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 17:11:22 +0000 Subject: [PATCH 2/3] Handle async watcher errors and drop Object.prototype from the merged view Two review findings: - An FSWatcher is an EventEmitter: an "error" event with no listener is rethrown as an uncaught exception and would kill the whole process on an asynchronous watcher failure (file deleted, OS watch limit, stale handle). The fs seam's watch() now takes an onError callback; the node implementation absorbs the "error" event, closes the dead watcher, and hands the failure to the service, which logs a warning noting that live reload for that file stops until restart. - The merged view was a plain object literal, so get("toString") and friends returned inherited Object.prototype members for unconfigured keys. The merged view is now built on a null prototype (initial value included). Regression tests cover both: prototype-member lookups return undefined, and firing a watcher's error callback records a warning without throwing. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- packages/core/src/config/service.test.ts | 49 +++++++++++++++++++++++ packages/core/src/config/service.ts | 50 ++++++++++++++++++++---- 2 files changed, 92 insertions(+), 7 deletions(-) diff --git a/packages/core/src/config/service.test.ts b/packages/core/src/config/service.test.ts index acf4032..3f21acd 100644 --- a/packages/core/src/config/service.test.ts +++ b/packages/core/src/config/service.test.ts @@ -565,3 +565,52 @@ describe("ConfigService — real filesystem integration (design.md §16)", () => service.dispose(); }, 15_000); }); + +describe("createConfigService — review regressions", () => { + test("get() does not leak Object.prototype members for unconfigured keys", async () => { + const log = createHostLog(); + const { sink } = createRecordingSink(); + const fake = createFakeFs(); + const service = createConfigService({ log, sink, fs: fake.fs }); + await service.ready; + + expect(service.get("toString")).toBeUndefined(); + expect(service.get("constructor")).toBeUndefined(); + expect(service.get("hasOwnProperty")).toBeUndefined(); + service.dispose(); + }); + + test("an asynchronous watcher failure is reported as a warning, not thrown", async () => { + const log = createHostLog(); + const { sink } = createRecordingSink(); + const errorCallbacks: ((cause: unknown) => void)[] = []; + const failingFs: ConfigServiceFs = { + async readFile(path) { + throw Object.assign(new Error(`ENOENT: ${path}`), { code: "ENOENT" }); + }, + watch(path, onChange, onError) { + void path; + void onChange; + if (onError) errorCallbacks.push(onError); + return { + close() { + // Nothing to release in this stub. + }, + }; + }, + }; + const service = createConfigService({ log, sink, fs: failingFs }); + await service.ready; + + // Every watcher's async failure path funnels through the onError + // callback — firing it must only record a warning. + expect(errorCallbacks.length).toBeGreaterThan(0); + for (const cb of errorCallbacks) cb(new Error("watch limit reached")); + + const warnings = log.entries().filter((e) => e.level === "warning"); + expect( + warnings.some((e) => e.error.message.includes("watch limit reached")), + ).toBe(true); + service.dispose(); + }); +}); diff --git a/packages/core/src/config/service.ts b/packages/core/src/config/service.ts index 5596bdf..e222489 100644 --- a/packages/core/src/config/service.ts +++ b/packages/core/src/config/service.ts @@ -51,16 +51,34 @@ import { parseJsonc } from "./jsonc"; export interface ConfigServiceFs { readFile(path: string): Promise; /** Start watching `path`; `onChange` is called (with no arguments) on - * every change the underlying watcher reports. Returns a handle whose - * `close()` stops watching. */ - watch(path: string, onChange: () => void): { close(): void }; + * every change the underlying watcher reports, and `onError` (when + * given) receives asynchronous watcher failures — an implementation + * must never let one escape as an unhandled `"error"` event. Returns a + * handle whose `close()` stops watching. */ + watch( + path: string, + onChange: () => void, + onError?: (cause: unknown) => void, + ): { close(): void }; } function createNodeConfigFs(): ConfigServiceFs { return { readFile: (path) => nodeReadFile(path, "utf8"), - watch: (path, onChange) => { + watch: (path, onChange, onError) => { const watcher = nodeWatch(path, () => onChange()); + // An FSWatcher is an EventEmitter: an "error" event with no + // listener is rethrown as an uncaught exception and kills the whole + // process. Absorb it, close the now-dead watcher, and hand the + // failure to the caller to report instead. + watcher.on("error", (cause) => { + try { + watcher.close(); + } catch { + // Already closed/broken — nothing more to release. + } + onError?.(cause); + }); return { close() { watcher.close(); @@ -221,7 +239,7 @@ export function createConfigService(deps: ConfigServiceDeps): ConfigService { const defaultsLayer: Record = {}; let userLayer: Record = {}; let workspaceLayer: Record = {}; - let merged: Record = {}; + let merged: Record = Object.create(null) as Record; let keybindingEntries: unknown[] = []; const changeListeners = new Set>(); @@ -247,7 +265,15 @@ export function createConfigService(deps: ConfigServiceDeps): ConfigService { } function computeMerged(): Record { - return { ...defaultsLayer, ...userLayer, ...workspaceLayer }; + // Null prototype: config keys are arbitrary strings, so a plain + // literal would leak Object.prototype members — get("toString") must + // be undefined unless actually configured. + return Object.assign( + Object.create(null) as Record, + defaultsLayer, + userLayer, + workspaceLayer, + ); } function diffChangedKeys( @@ -447,7 +473,17 @@ export function createConfigService(deps: ConfigServiceDeps): ConfigService { * for the MVP). */ function watchFile(path: string, onChange: () => void, label: string): void { try { - const handle = fs.watch(path, onChange); + const handle = fs.watch(path, onChange, (cause) => { + // Asynchronous watcher failure (file deleted, OS watch limit, + // stale handle, ...): the watcher is closed by the fs seam; live + // reload for this file stops until restart. Report, don't crash. + logSafely("warning", { + message: + `Watcher for ${label} (${path}) failed: ${describeError(cause)}. ` + + `Live reload for this file is disabled until restart (MVP limitation).`, + path, + }); + }); if (disposed) { // A dispose() landed while this synchronous call was in flight // (impossible in practice given JS's single-threaded execution, From 1e9d243004c6a847e24ae7d90d3178653c1237b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 17:13:22 +0000 Subject: [PATCH 3/3] Apply config-service review nitpicks - registerConfiguration revalidates the already-loaded user/workspace layers, so type-mismatch warnings no longer depend on whether a schema registration landed before or after the initial load. - The duplicate-key registration policy (last-write-wins; dispose removes the key outright without restoring an earlier contribution) is recorded as a deliberate MVP trade-off in TSDoc, as is the decision not to debounce watch events (serialized reloads self-heal a mid-write parse error on the burst's final event). - The JSONC error-position regex is anchored to "at position N" so a config file merely containing the words "position 123" cannot skew the reported line/column. - The real-filesystem integration test redirects HOME/APPDATA into its temp dir for the factory call, so it never reads or watches the real user's ~/.config/tecode files. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- packages/core/src/config/jsonc.ts | 2 +- packages/core/src/config/service.test.ts | 20 ++++++++++++++++---- packages/core/src/config/service.ts | 15 +++++++++++++++ 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/packages/core/src/config/jsonc.ts b/packages/core/src/config/jsonc.ts index 767f7f5..a051d3c 100644 --- a/packages/core/src/config/jsonc.ts +++ b/packages/core/src/config/jsonc.ts @@ -185,7 +185,7 @@ function describeError(err: unknown): string { * trade-off, design.md §11). */ function extractOffset(message: string): number | undefined { - const match = /position\s+(\d+)/i.exec(message); + const match = /\bat position\s+(\d+)/i.exec(message); if (!match) return undefined; const offset = Number(match[1]); return Number.isFinite(offset) ? offset : undefined; diff --git a/packages/core/src/config/service.test.ts b/packages/core/src/config/service.test.ts index 3f21acd..be1b144 100644 --- a/packages/core/src/config/service.test.ts +++ b/packages/core/src/config/service.test.ts @@ -544,10 +544,22 @@ describe("ConfigService — real filesystem integration (design.md §16)", () => const log = createHostLog(); const { sink } = createRecordingSink(); // No fs override: exercises the real node:fs/promises + node:fs.watch - // default. The user-level paths (~/.config/tecode/...) point at real - // OS paths too; a missing user config is the expected, harmless case - // (ENOENT -> empty layer). - const service = createConfigService({ log, sink, workspaceRoot: dir }); + // default. Redirect the user-level config directory into this test's + // temp dir (os.homedir()/%APPDATA% both follow these env vars) so the + // test never reads or watches the real user's ~/.config/tecode files. + const savedHome = process.env["HOME"]; + const savedAppData = process.env["APPDATA"]; + process.env["HOME"] = dir; + process.env["APPDATA"] = dir; + let service: ReturnType; + try { + service = createConfigService({ log, sink, workspaceRoot: dir }); + } finally { + if (savedHome === undefined) delete process.env["HOME"]; + else process.env["HOME"] = savedHome; + if (savedAppData === undefined) delete process.env["APPDATA"]; + else process.env["APPDATA"] = savedAppData; + } await service.ready; expect(service.get("editor.tabSize")).toBe(2); diff --git a/packages/core/src/config/service.ts b/packages/core/src/config/service.ts index e222489..4f775fe 100644 --- a/packages/core/src/config/service.ts +++ b/packages/core/src/config/service.ts @@ -445,6 +445,10 @@ export function createConfigService(deps: ConfigServiceDeps): ConfigService { // Per-file reload chains: two watch events for the same file firing in // quick succession must not run overlapping reads, or a slower-to-finish // older read could land after — and clobber — a newer one's result. + // No debounce is applied (deliberate MVP trade-off): a burst of + // coalesced fs.watch events just runs a few serialized reloads, and a + // mid-write parse error is reported once and self-heals on the burst's + // final event since the last-good layer is kept meanwhile. let userReloadChain: Promise = Promise.resolve(); let workspaceReloadChain: Promise = Promise.resolve(); let keybindingsReloadChain: Promise = Promise.resolve(); @@ -552,6 +556,12 @@ export function createConfigService(deps: ConfigServiceDeps): ConfigService { }; } + /** MVP policy for duplicate keys (deliberate, documented trade-off): key + * registration is last-write-wins, and disposing ANY registration that + * touched a key removes that key's schema and default outright — an + * earlier contribution's schema is not restored. Extensions do not share + * configuration keys in practice; revisit with a per-key registration + * stack if that assumption ever breaks. */ function registerConfiguration(contribution: ConfigurationContribution): Disposable { const keys: string[] = []; for (const [key, schema] of Object.entries(contribution.properties)) { @@ -561,6 +571,11 @@ export function createConfigService(deps: ConfigServiceDeps): ConfigService { defaultsLayer[key] = schema.default; } } + // A registration can land after the initial load finished: revalidate + // the already-loaded layers so type-mismatch warnings do not depend on + // registration/ready ordering. + validateLayerTypes(userLayer, "user settings"); + validateLayerTypes(workspaceLayer, "workspace settings"); rebuildMerged(); let regDisposed = false;