Skip to content

selfhost(private-config): parseConfigMapping never got #9065's JSON→YAML retry, so a valid layer is silently dropped from the merge #10057

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

src/selfhost/private-config.ts layers a container-private .loopover.yml from three sources (shared base →
global default → per-repo) and deep-merges them. Its parser documents itself as a copy of the runtime
manifest parser:

// src/selfhost/private-config.ts:147
/** Tolerantly parse raw config text into a plain mapping for MERGE PURPOSES ONLY — same 2-line 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. ... */
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;
  }
  ...
}

That is no longer what parseFocusManifestContent does. #9065 added a JSON→YAML retry to the real parser:

// packages/loopover-engine/src/focus-manifest.ts:4556
if (looksLikeJson) {
  try {
    parsed = JSON.parse(trimmed);
  } catch {
    // #9065: a YAML flow mapping (e.g. unquoted keys) can start with "{"/"[" while being invalid strict
    // JSON -- retry as YAML before giving up, matching config-lint.ts's own (offline-only, until now)
    // parseManifestTopLevelObject fallback, so the runtime path recognizes the same manifests the offline
    // validator already accepted instead of rejecting them outright as "not valid JSON".
    try {
      parsed = parseYaml(trimmed);
    } catch { ... }
  }
}

The local copy in src/selfhost/private-config.ts never got it, so the two disagree on exactly the document
class #9065 was about: a YAML flow mapping whose text begins with { — e.g.
{gate: {linkedIssue: advisory}}, which parseYaml accepts and JSON.parse rejects (unquoted keys). The
offline linter (packages/loopover-engine/src/config-lint.ts, re-exported at src/selfhost/config-lint.ts:7)
accepts it too, so an operator who validated their file sees no complaint anywhere.

The consequences differ by how many layers are mounted, and neither is benign:

  • Two or more layers present. combineConfigLayersWithMeta (src/selfhost/private-config.ts:186) drops
    the unparseable layer from parsedLayers and merges the rest. A per-repo layer written in flow-mapping
    form is silently discarded and the repo falls back to the global default / shared base — every per-repo
    setting (gate, autonomy, labels, model) reverts, with only a generic
    "Container-private per-repo manifest (…) is malformed or oversized; ignoring it and continuing."
    warning to explain it. This is the same silent-config-reversion class the boot advisory
    emptyConfigDirAdvisory (src/selfhost/health.ts) and the inert-config report
    (src/selfhost/inert-config.ts) exist to make visible.
  • Exactly one layer present. combineConfigLayersWithMeta falls through to
    return { content: present[present.length - 1]!.text, ... }, so the raw text is handed to
    parseFocusManifestContent, which — thanks to orb(config): no unknown-key validation at runtime — a typo silently disables a safety control with zero warning #9065 — parses it fine. The config is applied correctly, but
    the loader has already emitted the "is malformed or oversized; ignoring it and continuing" warning and
    incremented loopover_private_manifest_warnings_total (src/selfhost/private-config.ts:281). The metric's
    own help text says "a sustained run means a mount is repeatedly serving truncated or invalid config", so
    every load of a perfectly valid manifest produces a false alarm on the dashboard the metric was added for.

The write-validation twin has the same gap and asserts it does not:

// src/selfhost/private-config.ts:401
 *  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. */
export function validateConfigWriteContent(text: string): ConfigValidationResult {

So the MCP config-admin write path (src/mcp/private-config-admin-registry.ts) rejects, with
Failed to parse as JSON: …, a document the runtime loader would happily apply.

Requirements

  • parseConfigMapping (src/selfhost/private-config.ts:152) must retry parseYaml(trimmed) when
    JSON.parse(trimmed) throws on a {/[-leading document, returning the YAML result when it parses and
    null only when both fail — byte-for-byte the same fallback shape as
    packages/loopover-engine/src/focus-manifest.ts:4556.
  • validateConfigWriteContent (src/selfhost/private-config.ts:404) must apply the identical retry, so its
    stated "merges cleanly on read ⇒ validates cleanly on write, and vice versa" invariant holds. Its error
    message on a genuine double failure must still name a parse failure with the underlying error text; it must
    not start returning ok: true for a document neither parser accepts.
  • Both JSDoc blocks must record that the retry mirrors orb(config): no unknown-key validation at runtime — a typo silently disables a safety control with zero warning #9065's fallback in the engine parser, so a future
    reader can see the two are meant to stay in lockstep.
  • Must NOT change: the MAX_FOCUS_MANIFEST_BYTES ceiling check, the "must be a plain mapping (not null,
    array, or scalar)" rejection, combineConfigLayersWithMeta's layer precedence
    (shared → global → per-repo), the single-layer raw-text passthrough at
    src/selfhost/private-config.ts:186's parsedLayers.length <= 1 branches, or the warning text emitted for
    a genuinely malformed layer.
  • No migrations/*.sql file may be added or edited by this issue; it needs no schema change.

⚠️ Required pattern: mirror packages/loopover-engine/src/focus-manifest.ts:4556's nested
try { JSON.parse } catch { try { parseYaml } catch { … } }. What does NOT satisfy this issue:
(a) exporting the engine's parser and rewriting parseConfigMapping to call it — the local copy exists on
purpose (see its own JSDoc) and the two functions return different things (a plain mapping vs a
FocusManifest); (b) always calling parseYaml and dropping the JSON branch — YAML 1.2 is a JSON
superset in most cases but the looksLikeJson split is load-bearing for error messages in
validateConfigWriteContent and must stay; (c) fixing only parseConfigMapping and leaving the write
validator rejecting what the read path now merges, which recreates the same asymmetry in the other
direction; (d) suppressing the warning/metric instead of parsing the layer.

Deliverables

  • parseConfigMapping (src/selfhost/private-config.ts:152) treats "{gate: {linkedIssue: advisory}}"
    (valid YAML flow mapping, invalid strict JSON) as mergeable and still returns null for
    "{not: valid: yaml: [". It is currently module-private, so this must be proven through the exported
    makeLocalManifestReader reader (see the two reader tests below) rather than by calling it directly —
    do not add a new export solely for the test.
  • validateConfigWriteContent returns { ok: true } for "{gate: {linkedIssue: advisory}}" and an
    { ok: false, error } naming a parse failure for "{not: valid: yaml: [".
  • A test at test/unit/private-config.test.ts covering the multi-layer case: mount a global-default
    layer and a per-repo layer written as a YAML flow mapping, call the reader returned by
    makeLocalManifestReader (src/selfhost/private-config.ts:253), and assert the merged content
    contains the per-repo layer's value (not the global default's) and that warnings is empty.
  • A test at test/unit/private-config.test.ts covering the single-layer case: mount only a per-repo
    flow-mapping layer and assert the returned warnings array is empty (no false
    "malformed or oversized" warning), while content still round-trips through
    parseFocusManifestContent to the same manifest.
  • A regression test named for this bug at test/unit/private-config.test.ts asserting that for the
    flow-mapping document, validateConfigWriteContent returns ok: true and
    parseFocusManifestContent (imported from src/signals/focus-manifest) parses it to a non-empty
    manifest — pinning that the private-config write validator and the runtime manifest parser agree on the
    same document.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that adds the retry to parseConfigMapping but leaves validateConfigWriteContent rejecting the same
document — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include
covers src/**/*.ts and packages/loopover-engine/src/**/*.ts; src/selfhost/private-config.ts is measured.
The change is in src/, not packages/loopover-engine/src/, so the engine's dual-upload rule does not apply
here. Each new nested catch adds two branches per function and all four arms need a test: JSON parses
(no retry), JSON fails and YAML parses (retry succeeds), JSON fails and YAML fails (both reject), and the
non-looksLikeJson path (plain YAML document, unchanged). The multi-layer and single-layer reader tests are
what exercise the downstream warning/metric branch at src/selfhost/private-config.ts:280.

Expected Outcome

A container-private config layer written as a YAML flow mapping is parsed by the layer merger exactly as the
runtime manifest parser and the offline linter already parse it: it participates in the deep merge instead of
being silently dropped in favour of the global default, and it stops producing a false
loopover_private_manifest_warnings_total increment on every load. The MCP config-admin write path accepts
the same documents the read path merges, restoring the invariant its own JSDoc claims.

Links & Resources

  • src/selfhost/private-config.ts:147parseConfigMapping and its "same detection as
    parseFocusManifestContent" claim
  • src/selfhost/private-config.ts:186combineConfigLayersWithMeta, where a dropped layer disappears
  • src/selfhost/private-config.ts:280 — the warning + loopover_private_manifest_warnings_total increment
  • src/selfhost/private-config.ts:401validateConfigWriteContent's read/write symmetry claim
  • packages/loopover-engine/src/focus-manifest.ts:4556orb(config): no unknown-key validation at runtime — a typo silently disables a safety control with zero warning #9065's JSON→YAML retry, the pattern to mirror
  • src/selfhost/config-lint.ts:7 — the offline linter that already accepts these documents

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions