fix(selfhost): mirror the engine's JSON->YAML retry in private-config parsing

src/selfhost/private-config.ts

@@ -144,20 +144,37 @@ function extractReviewMapping(mapping: Record<string, unknown>): Record<string,
   return null;
 }
 
-/** Tolerantly parse raw config text into a plain mapping for MERGE PURPOSES ONLY — same 2-line YAML/JSON detection
+/** Tolerantly parse raw config text into a plain mapping for MERGE PURPOSES ONLY — same YAML/JSON detection
  *  `parseFocusManifestContent` (focus-manifest.ts) uses, duplicated locally rather than exported from there so that
- *  file's public surface stays unchanged for what is otherwise two lines of logic. Returns null — "not mergeable" —
- *  for empty/oversized text, a parse error, or a parsed value that isn't a plain mapping (null/array/scalar); every
- *  one of those cases makes the caller fall back to legacy single-candidate behavior instead of attempting a merge. */
+ *  file's public surface stays unchanged for what is otherwise a handful of lines of logic, INCLUDING #9065's
+ *  JSON→YAML retry (focus-manifest.ts:4556): a YAML flow mapping (e.g. unquoted keys) can start with "{"/"[" while
+ *  being invalid strict JSON, so a `JSON.parse` failure on a `{`/`[`-leading document retries as YAML before giving
+ *  up. The two parsers are meant to stay in lockstep. Returns null — "not mergeable" — for empty/oversized text, a
+ *  parse error (on both attempts, when retried), or a parsed value that isn't a plain mapping (null/array/scalar);
+ *  every one of those cases makes the caller fall back to legacy single-candidate behavior instead of attempting a
+ *  merge. */
 function parseConfigMapping(text: string): Record<string, unknown> | 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<string, unknown>;
@@ -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." };

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<typeof import("yaml")>();
+  return { ...actual, parse: vi.fn(actual.parse) };
+});
+
 async function readLocalManifestContent(reader: RepoFocusManifestFetcher, repo: string): Promise<string | null> {
   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)", () => {