From 11fa6ec3b5fedfafb3a1c06bf6caa28fb73b6597 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Sun, 19 Jul 2026 12:37:44 +0000 Subject: [PATCH] =?UTF-8?q?fix(engine):=20apply=20the=20JSON=E2=86=92YAML?= =?UTF-8?q?=20fallback=20in=20config-lint's=20canonical=20parser=20too?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit config-lint had two independent top-level-object parsers for the same manifest text that disagreed on JSON/YAML fallback. `parseTopLevelObject` (used by `unknownTopLevelWarnings`) retried with `parseYaml` when `JSON.parse` threw on a `{`/`[`-prefixed text — YAML flow mappings can start that way while still being valid manifest syntax. `parseCanonicalTopLevelObject` (used by `recognizedFieldsFor`) did not: it caught the `JSON.parse` failure and returned null → [], so `buildConfigLintReport` silently reported zero recognized fields for a valid YAML-flow-mapping manifest that `unknownTopLevelWarnings` happily parsed and warned about — the two functions produced inconsistent results for the same input in the same report call. De-duplicate the two into one shared `parseManifestTopLevelObject` used by both callers, so recognizedFieldsFor and unknownTopLevelWarnings can never drift on whether a given manifest text parses. Add a regression test asserting a valid YAML flow mapping starting with `{` now yields non-empty recognized fields consistent with the unknown-field warning for the same text. Closes #7244 --- packages/loopover-engine/src/config-lint.ts | 29 +++++++-------------- test/unit/selfhost-config-lint.test.ts | 21 +++++++++++++-- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/packages/loopover-engine/src/config-lint.ts b/packages/loopover-engine/src/config-lint.ts index 3242a9c80c..2fbb9fe625 100644 --- a/packages/loopover-engine/src/config-lint.ts +++ b/packages/loopover-engine/src/config-lint.ts @@ -63,7 +63,7 @@ export function lintManifestText(text: string | null | undefined): SelfHostConfi } function recognizedFieldsFor(text: string | null | undefined): string[] { - const parsed = parseCanonicalTopLevelObject(text); + const parsed = parseManifestTopLevelObject(text); if (parsed === null) return []; return TOP_LEVEL_FIELDS.filter( (field) => field !== "source" && Object.prototype.hasOwnProperty.call(parsed, field), @@ -77,10 +77,7 @@ const RETIRED_FIELD_MIGRATION_WARNINGS: Record = { }; export function unknownTopLevelWarnings(text: string | null | undefined): string[] { - const raw = text ?? ""; - const trimmed = raw.trim(); - if (!trimmed || isOversize(raw)) return []; - const parsed = parseTopLevelObject(trimmed); + const parsed = parseManifestTopLevelObject(text); if (parsed === null) return []; const keys = Object.keys(parsed).filter((key) => !TOP_LEVEL_FIELD_SET.has(key)); // `hasOwnProperty.call`, NOT `key in`: a manifest field named like an Object.prototype member @@ -96,30 +93,24 @@ export function unknownTopLevelWarnings(text: string | null | undefined): string ]; } -function parseCanonicalTopLevelObject(text: string | null | undefined): Record | null { +// Single top-level-object parser shared by both `recognizedFieldsFor` and `unknownTopLevelWarnings` so the two +// can never disagree on whether a given manifest text parses. When the text looks like JSON (`{`/`[`) but +// `JSON.parse` throws, it retries with `parseYaml`: YAML flow mappings can start with "{" or "[" (e.g. unquoted +// keys) while still being valid manifest syntax, so a strict-JSON failure alone must not be treated as unparseable. +function parseManifestTopLevelObject(text: string | null | undefined): Record | null { const raw = text ?? ""; const trimmed = raw.trim(); if (!trimmed || isOversize(raw)) return null; const looksLikeJson = trimmed.startsWith("{") || trimmed.startsWith("["); - try { - return topLevelObjectOrNull(looksLikeJson ? JSON.parse(trimmed) : parseYaml(trimmed)); - } catch { - return null; - } -} - -function parseTopLevelObject(text: string): Record | null { - const looksLikeJson = text.startsWith("{") || text.startsWith("["); if (looksLikeJson) { try { - const parsed = JSON.parse(text); - return topLevelObjectOrNull(parsed); + return topLevelObjectOrNull(JSON.parse(trimmed)); } catch { - // YAML flow mappings can start with "{" or "[" while still being valid manifest syntax. + // Fall through to YAML: a `{`/`[` prefix can be a valid YAML flow mapping that is invalid strict JSON. } } try { - return topLevelObjectOrNull(parseYaml(text)); + return topLevelObjectOrNull(parseYaml(trimmed)); } catch { return null; } diff --git a/test/unit/selfhost-config-lint.test.ts b/test/unit/selfhost-config-lint.test.ts index 00748428aa..da2cbb5fd7 100644 --- a/test/unit/selfhost-config-lint.test.ts +++ b/test/unit/selfhost-config-lint.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { lintManifestText } from "../../src/selfhost/config-lint"; +import { lintManifestText, unknownTopLevelWarnings } from "../../src/selfhost/config-lint"; import { MAX_FOCUS_MANIFEST_BYTES } from "../../src/signals/focus-manifest"; describe("lintManifestText (#2079)", () => { @@ -271,7 +271,9 @@ unknownSecretKey: super-secret-value const result = lintManifestText("{wantedPaths: [src/], unknownSecretKey: secret}"); expect(result.ok).toBe(false); - expect(result.recognizedFields).toEqual([]); + // recognizedFieldsFor now applies the same JSON->YAML fallback as unknownTopLevelWarnings, so a flow-mapping + // manifest that is valid YAML but invalid strict JSON reports its real recognized fields instead of []. + expect(result.recognizedFields).toEqual(["wantedPaths"]); expect(result.warnings).toEqual([ "Manifest content was not valid JSON; ignoring it and falling back to deterministic signals.", "Manifest contains unknown top-level field: unknownSecretKey.", @@ -279,6 +281,21 @@ unknownSecretKey: super-secret-value expect(JSON.stringify(result)).not.toContain("secret"); }); + it("REGRESSION (#7244): recognizes fields in a YAML flow-mapping manifest starting with '{' consistently with unknown-field detection", () => { + // `{settings: ..., wantedPaths: ..., mysteryKey: ...}` is a valid YAML flow mapping but invalid strict JSON + // (unquoted keys). unknownTopLevelWarnings already fell back to YAML and saw the real fields; recognizedFieldsFor + // did NOT (it caught the JSON.parse failure and returned null -> []), so buildConfigLintReport silently reported + // zero recognized fields for a fully-parseable, valid manifest. Both must now agree the manifest parses. + const text = "{settings: {commentMode: all_prs}, wantedPaths: [src/], mysteryKey: hidden}"; + const result = lintManifestText(text); + + // recognizedFieldsFor (surfaced via result.recognizedFields) sees the real fields instead of silently empty. + expect(result.recognizedFields).toEqual(["wantedPaths", "settings"]); + // ...consistent with unknownTopLevelWarnings, which flags the unknown key on the SAME parsed object. + expect(unknownTopLevelWarnings(text)).toEqual(["Manifest contains unknown top-level field: mysteryKey."]); + expect(JSON.stringify(result)).not.toContain("hidden"); + }); + it("keeps known JSON manifests quiet and non-object JSON invalid", () => { expect(lintManifestText(JSON.stringify({ gate: { enabled: true, checkMode: "required" } }))).toMatchObject({ ok: true,