From 26bed7733b0dde36de7e94057dcd0530b75e2aa2 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Fri, 31 Jul 2026 07:23:45 +0000 Subject: [PATCH] fix(selfhost): mirror the engine's JSON->YAML retry in private-config parsing parseConfigMapping and validateConfigWriteContent detected JSON vs YAML the same way parseFocusManifestContent does but never got its retry: a leading `{`/`[` document that's valid YAML flow-mapping syntax but invalid strict JSON was treated as unparseable, silently dropping a per-repo config layer from the merge (or, with only one layer mounted, applying it correctly while still firing a false "malformed" warning and metric increment). The write validator had the identical gap, rejecting documents the read path already merges. Both now retry as YAML on a JSON parse failure before giving up, matching the engine parser byte-for-byte. --- src/selfhost/private-config.ts | 63 ++++++++++++++++------ test/unit/private-config.test.ts | 91 +++++++++++++++++++++++++++++++- 2 files changed, 138 insertions(+), 16 deletions(-) diff --git a/src/selfhost/private-config.ts b/src/selfhost/private-config.ts index 3de007f7d3..6935244600 100644 --- a/src/selfhost/private-config.ts +++ b/src/selfhost/private-config.ts @@ -144,20 +144,37 @@ function extractReviewMapping(mapping: Record): Record | null { const trimmed = text.trim(); if (!trimmed || trimmed.length > MAX_FOCUS_MANIFEST_BYTES) return null; const looksLikeJson = trimmed.startsWith("{") || trimmed.startsWith("["); let parsed: unknown; - try { - parsed = looksLikeJson ? JSON.parse(trimmed) : parseYaml(trimmed); - } catch { - return null; + if (looksLikeJson) { + try { + parsed = JSON.parse(trimmed); + } catch { + // #9065: retry as YAML before giving up — see this function's JSDoc. + try { + parsed = parseYaml(trimmed); + } catch { + return null; + } + } + } else { + try { + parsed = parseYaml(trimmed); + } catch { + return null; + } } if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) return null; return parsed as Record; @@ -398,9 +415,11 @@ export type ConfigBackupEntry = { name: string; path: string; mtimeMs: number }; /** Validate write content against the same YAML/JSON-mapping shape the read path's own * {@link parseConfigMapping} enforces for merging, but with a specific, actionable error message instead of a * bare null — a write rejection needs to tell the caller WHY, unlike a read fallback which just moves on to the - * next layer. Deliberately reuses the same MAX_FOCUS_MANIFEST_BYTES ceiling and JSON/YAML detection heuristic - * (leading `{`/`[`) as parseConfigMapping so a document that would merge cleanly on read also validates cleanly - * on write, and vice versa. */ + * next layer. Deliberately reuses the same MAX_FOCUS_MANIFEST_BYTES ceiling, JSON/YAML detection heuristic + * (leading `{`/`[`), and #9065 JSON→YAML retry (focus-manifest.ts:4556) as parseConfigMapping so a document that + * would merge cleanly on read also validates cleanly on write, and vice versa; the two are meant to stay in + * lockstep. On a genuine double failure (neither JSON nor the YAML retry parses) the error still names the + * original JSON parse failure and its message, since that's the branch a `{`/`[`-leading document took first. */ export function validateConfigWriteContent(text: string): ConfigValidationResult { const trimmed = text.trim(); if (!trimmed) return { ok: false, error: "Content is empty." }; @@ -409,10 +428,24 @@ export function validateConfigWriteContent(text: string): ConfigValidationResult } const looksLikeJson = trimmed.startsWith("{") || trimmed.startsWith("["); let parsed: unknown; - try { - parsed = looksLikeJson ? JSON.parse(trimmed) : parseYaml(trimmed); - } catch (error) { - return { ok: false, error: `Failed to parse as ${looksLikeJson ? "JSON" : "YAML"}: ${error instanceof Error ? error.message : String(error)}` }; + if (looksLikeJson) { + try { + parsed = JSON.parse(trimmed); + } catch (jsonError) { + // #9065: retry as YAML before rejecting — mirrors parseConfigMapping's retry, which mirrors + // focus-manifest.ts:4556's. + try { + parsed = parseYaml(trimmed); + } catch { + return { ok: false, error: `Failed to parse as JSON: ${jsonError instanceof Error ? jsonError.message : String(jsonError)}` }; + } + } + } else { + try { + parsed = parseYaml(trimmed); + } catch (error) { + return { ok: false, error: `Failed to parse as YAML: ${error instanceof Error ? error.message : String(error)}` }; + } } if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { return { ok: false, error: "Content must parse to a YAML/JSON mapping (object) at the top level, not a scalar, array, or null." }; diff --git a/test/unit/private-config.test.ts b/test/unit/private-config.test.ts index d2333c3ed0..5651440ded 100644 --- a/test/unit/private-config.test.ts +++ b/test/unit/private-config.test.ts @@ -1,7 +1,8 @@ import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; +import { parse as yamlParse } from "yaml"; import { GLOBAL_CONFIG_CANDIDATES, isReviewSkillEnabled, @@ -22,6 +23,14 @@ import { import { loadRepoReviewContext, setLocalReviewContextReader, type RepoFocusManifestFetcher } from "../../src/signals/focus-manifest-loader"; import { MAX_FOCUS_MANIFEST_BYTES, parseFocusManifestContent } from "../../src/signals/focus-manifest"; +// Wraps the real `yaml` parser in a spyable fn (default behavior unchanged) so a couple of #9065 +// retry tests below can force a single non-Error throw without touching every other test in this +// file, which all rely on real YAML parsing. +vi.mock("yaml", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, parse: vi.fn(actual.parse) }; +}); + async function readLocalManifestContent(reader: RepoFocusManifestFetcher, repo: string): Promise { const result = await reader(repo); if (result === null) return null; @@ -398,6 +407,57 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { }); }); +describe("makeLocalManifestReader — #9065 JSON→YAML retry for a leading `{`/`[` layer", () => { + it("merges a per-repo YAML flow-mapping layer with the global default instead of silently dropping it", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); + writeFileSync(join(dir, ".loopover.yml"), "gate:\n duplicates: block\n"); // global default + mkdirSync(join(dir, "repo")); + writeFileSync(join(dir, "repo", ".loopover.yml"), "{gate: {linkedIssue: advisory}}"); // valid YAML flow mapping, invalid strict JSON + const reader = makeLocalManifestReader(dir); + const loaded = await readLocalManifestLoad(reader!, "owner/repo"); + expect(loaded!.warnings).toEqual([]); // no false "malformed or oversized" warning + const manifest = parseFocusManifestContent(loaded!.content); + expect(manifest.gate.linkedIssue).toBe("advisory"); // per-repo layer's value, not dropped in favor of the global default + expect(manifest.gate.duplicates).toBe("block"); // still inherited from global — both layers participated in the merge + }); + + it("serves a lone per-repo YAML flow-mapping layer with no false malformed warning", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); + mkdirSync(join(dir, "repo")); + writeFileSync(join(dir, "repo", ".loopover.yml"), "{gate: {linkedIssue: advisory}}"); + const reader = makeLocalManifestReader(dir); + const loaded = await readLocalManifestLoad(reader!, "owner/repo"); + expect(loaded!.warnings).toEqual([]); // single-layer passthrough must not increment the false-alarm metric either + expect(loaded!.content).toBe("{gate: {linkedIssue: advisory}}"); // raw text, unchanged + const manifest = parseFocusManifestContent(loaded!.content); + expect(manifest.gate.linkedIssue).toBe("advisory"); // round-trips through the runtime manifest parser to the same value + }); + + it("still drops a per-repo layer that isn't JSON-shaped and fails plain YAML parsing, and still warns", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); + writeFileSync(join(dir, ".loopover.yml"), "gate:\n duplicates: block\n"); // global default + mkdirSync(join(dir, "repo")); + writeFileSync(join(dir, "repo", ".loopover.yml"), "gate: [unterminated"); // doesn't start with `{`/`[` — plain-YAML branch, still fails to parse + const reader = makeLocalManifestReader(dir); + const loaded = await readLocalManifestLoad(reader!, "owner/repo"); + expect(loaded!.content).toBe("gate:\n duplicates: block\n"); // per-repo layer dropped, falls back to global alone + expect(loaded!.warnings).toHaveLength(1); + expect(loaded!.warnings[0]).toContain("per-repo manifest"); + }); + + it("still drops a per-repo layer that fails both the JSON and the YAML retry, and still warns", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); + writeFileSync(join(dir, ".loopover.yml"), "gate:\n duplicates: block\n"); // global default + mkdirSync(join(dir, "repo")); + writeFileSync(join(dir, "repo", ".loopover.yml"), "{not: valid: yaml: ["); // starts with `{`, invalid JSON AND invalid YAML + const reader = makeLocalManifestReader(dir); + const loaded = await readLocalManifestLoad(reader!, "owner/repo"); + expect(loaded!.content).toBe("gate:\n duplicates: block\n"); // per-repo layer dropped, falls back to global alone + expect(loaded!.warnings).toHaveLength(1); + expect(loaded!.warnings[0]).toContain("per-repo manifest"); // dropped-layer warning still fires, same as before the retry + }); +}); + describe("makeLocalManifestReader — review.shared_config overlay (#2046)", () => { it("records sharedConfigSource when the shared base contributes a review block", async () => { const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); @@ -654,6 +714,35 @@ describe("validateConfigWriteContent (#7721)", () => { it("accepts a valid JSON mapping", () => { expect(validateConfigWriteContent('{"gate": {"mode": "advisory"}}')).toEqual({ ok: true }); }); + it("accepts a YAML flow mapping that isn't valid strict JSON (#9065 retry)", () => { + expect(validateConfigWriteContent("{gate: {linkedIssue: advisory}}")).toEqual({ ok: true }); + }); + it("still rejects a document that fails both the JSON attempt and the YAML retry", () => { + const result = validateConfigWriteContent("{not: valid: yaml: ["); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toContain("Failed to parse as JSON"); + }); + it("agrees with parseFocusManifestContent on a flow-mapping document (#9065 read/write symmetry)", () => { + const flowMapping = "{gate: {linkedIssue: advisory}}"; + expect(validateConfigWriteContent(flowMapping)).toEqual({ ok: true }); + const manifest = parseFocusManifestContent(flowMapping); + expect(manifest.gate.linkedIssue).toBe("advisory"); // the runtime parser accepts the same document, to a non-empty manifest + }); + it("stringifies a non-Error thrown by the initial JSON attempt when the YAML retry also fails", () => { + const jsonSpy = vi.spyOn(JSON, "parse").mockImplementationOnce(() => { + throw "boom"; // eslint-disable-line no-throw-literal + }); + const result = validateConfigWriteContent("{not: valid: yaml: ["); + jsonSpy.mockRestore(); + expect(result).toEqual({ ok: false, error: "Failed to parse as JSON: boom" }); + }); + it("stringifies a non-Error thrown by the plain-YAML parse attempt", () => { + vi.mocked(yamlParse).mockImplementationOnce(() => { + throw "boom"; // eslint-disable-line no-throw-literal + }); + const result = validateConfigWriteContent("gate:\n mode: advisory\n"); + expect(result).toEqual({ ok: false, error: "Failed to parse as YAML: boom" }); + }); }); describe("writeGlobalConfig / readGlobalConfigRaw (#7721)", () => {