Skip to content

fix(core): one malformed persona_bots row takes down the whole Host (Invalid PersonaBot Registry record) #3213

Closed

Description

@hkhkhkhh2211

Summary

A single row in persona_bots whose modelPlan does not match isPersonaBotModelPlan makes createPersonaBotRegistry() throw during apply, so the whole @botharness/core plugin goes failed. The registry, memory, inbox and schedules of every PersonaBot go down together until someone repairs the row by hand.

decode() already has a sanitizer for exactly this class of problem — it deletes appearance, banner and primaryColor when they do not validate — but modelPlan / modelPlanRevision were never added to that list, so they reach the throw at the bottom.

Impact

Observed on v1.6.0 (2026-10-30, after a Host restart):

  • @botharness/core fiber failed; botharness-client / botharness-browser / botharness-computer unusable as a consequence.
  • Error surface was a single line: Invalid PersonaBot Registry record.
  • Recovery required stopping the Host, editing botharness.db by hand, and re-running entry.init().

This is not a version regression: packages/core/src/bots/registry.ts:309 (throw new Error('Invalid PersonaBot Registry record')) is byte-identical in 1.5.2 and 1.6.0, so downgrading does not help.

Root cause

packages/core/src/bots/registry.ts — decode() (lines 286–311):

const decode = (text: string, slug: string): PersonaBotRecord => {
  const parsed: unknown = JSON.parse(text);
  if (… 'appearance' in parsed && !isUsableAvatarAppearance(…))
    delete (parsed as { appearance?: unknown }).appearance;
  if (… 'banner' in parsed && !isBotBanner(parsed.banner))
    delete (parsed as { banner?: unknown }).banner;
  if (… 'primaryColor' in parsed && !isBotPrimaryColor(parsed.primaryColor))
    delete (parsed as { primaryColor?: unknown }).primaryColor;
  // ← no sanitizer for modelPlan / modelPlanRevision
  if (!isPersonaBotRecord(parsed, slug)) throw new Error('Invalid PersonaBot Registry record');
  return recordSnapshot(parsed);
};

and list() (lines 338–345) maps decode over every row with no per-row isolation:

const list = (): PersonaBotRecord[] =>
  port.read((database) =>
    database.prepare('SELECT slug, body FROM persona_bots').all()
      .map((row) => decode(String(row['body']), String(row['slug'])))
      .sort((left, right) => left.slug.localeCompare(right.slug)));

createPersonaBotRegistry() calls list() during construction, so the throw propagates out of apply and the fiber fails. One bad row, whole Host down.

Reproduction

Write a row whose modelPlan is a bare route (a shape an agent or a hand-edit can plausibly produce) plus any integer modelPlanRevision:

{
  "slug": "bot-<id>",
  "displayName": "…",
  "workspaces": [],
  "createdAt": "2026-10-29T…Z",
  "modelPlan": { "provider": "some-provider", "model": "some-model", "reasoningEffort": "low" },
  "modelPlanRevision": 139
}

isPersonaBotRecord() (packages/core/src/bots/persona-bot.ts:222) then fails on modelPlan, decode() throws, list() throws, apply throws.

Note the write path never had a chance to reject it either: the only sanctioned writer is the registry's own write() (transactions tagged ['bot-registry']), but any process holding the SQLite file can append a row, and the schema only enforces CHECK (json_valid(body)) — validity of shape is checked on read, never on write.

Proposed patch

Minimal, and it follows the three sanitizers already in decode() rather than inventing a new mechanism. Dropping an unusable modelPlan puts that bot back on host-default routing, which is already the documented meaning of modelPlan === undefined (record.modelPlan === void 0 && record.model === void 0 ? 'host-default' : 'set'), so the blast radius is one bot's routing instead of the whole Host.

Verified to apply on main at 295405d (git apply --check exit 0, result byte-identical to the intended file):

diff --git a/packages/core/src/bots/registry.ts b/packages/core/src/bots/registry.ts
index 295405d..a9129c7 100644
--- a/packages/core/src/bots/registry.ts
+++ b/packages/core/src/bots/registry.ts
@@ -56,11 +56,12 @@ import {
 } from '../memory/soul.js';
 import type { TelemetryEventName } from '../telemetry/events.js';
 import type { TelemetryCapture } from '../telemetry/service.js';
-import type {
-  AssignmentModelOption,
-  ModelPreset,
-  ModelRoute,
-  PersonaBotModelPlan,
+import {
+  isPersonaBotModelPlan,
+  type AssignmentModelOption,
+  type ModelPreset,
+  type ModelRoute,
+  type PersonaBotModelPlan,
 } from '../models/presets.js';
 import { parsePromptLimitValues, type PromptLimitValues } from '../prompts/prompt-limits.js';
 import { parsePromptPartOverrides, type PromptPartOverrides } from '../prompts/prompt-presets.js';
@@ -306,6 +307,21 @@ export function createPersonaBotRegistry(options: PersonaBotRegistryOptions): Pe
       !isBotPrimaryColor(parsed.primaryColor)
     )
       delete (parsed as { primaryColor?: unknown }).primaryColor;
+    if (
+      typeof parsed === 'object' &&
+      parsed !== null &&
+      'modelPlan' in parsed &&
+      !isPersonaBotModelPlan(parsed.modelPlan)
+    )
+      delete (parsed as { modelPlan?: unknown }).modelPlan;
+    if (
+      typeof parsed === 'object' &&
+      parsed !== null &&
+      'modelPlanRevision' in parsed &&
+      (!Number.isSafeInteger(parsed.modelPlanRevision) ||
+        (parsed.modelPlanRevision as number) < 0)
+    )
+      delete (parsed as { modelPlanRevision?: unknown }).modelPlanRevision;
     if (!isPersonaBotRecord(parsed, slug)) throw new Error('Invalid PersonaBot Registry record');
     return recordSnapshot(parsed);
   };

isPersonaBotModelPlan already exists at packages/core/src/models/presets.ts:179 and is what isPersonaBotRecord uses for the same field, so no new predicate is introduced. modelPlanRevision gets the same treatment because persona-bot.ts:216-221 rejects a non-safe-integer / negative revision, which has the identical failure mode.

Deliberately left out of this patch

list() still throws on any row that fails decode after sanitizing (e.g. a missing displayName). I would suggest handling that separately, because skipping invalid rows silently hides data loss — it needs a diagnostics channel (a logged/surfaced list of skipped slugs) rather than a bare try { } catch { continue }. Happy to send that as a second PR if you want the direction.

Environment

  • @botharness/core 1.6.0 (also reproduces on 1.5.2), Windows, Node 24, local SQLite 3.53.1.
  • Data check on the affected Host after hand-repair: 7 rows in persona_bots, all 7 pass isPersonaBotRecord, 1 row in model_presets passes isModelPreset.
  • Search for this error string on GitHub returns 0 results, so this is presumably the first report.

Activity

  1. hkhkhkhh2211 commented on Oct 30, 2026

    @hkhkhkhh2211
    Author

    Follow-up after re-reading the module — I want to correct the emphasis in the original report.

    The decode() sanitizer above fixes the blast radius. The actual root cause is one layer up: write() persists without ever validating the record it is about to store.

    // packages/core/src/bots/registry.ts:319-329
    const write = (record: PersonaBotRecord): void => {
      port.transaction(
        (database) => {
          database
            .prepare(`INSERT INTO persona_bots (slug, body) VALUES (?, ?)
            ON CONFLICT (slug) DO UPDATE SET body = excluded.body`)
            .run(record.slug, JSON.stringify(recordSnapshot(record)));
        },
        ['bot-registry'],
      );
    };

    Validation exists only on the read side (decode → isPersonaBotRecord) and, for some fields, in the domain helpers. The table itself only enforces shape-free validity:

    CREATE TABLE persona_bots (slug TEXT PRIMARY KEY, body TEXT NOT NULL CHECK (json_valid(body)));

    So the invariant "every persisted body satisfies isPersonaBotRecord" is enforced nowhere at the point of persistence.

    What makes this worth fixing at the write site rather than only at the read site:

    • The sanctioned paths are already safe. setModelPlan() builds the object itself (revision + 1, sourcePresetId: "", sourcePresetName: "", orchestrator, assignmentDefault, assignmentModels, appliedAt: now()), and every other mutator goes through recordSnapshot. If those writers are the only writers, the invariant holds by construction.
    • The failure we hit came from an out-of-band writer — a process that opened botharness.db directly and appended a row. The schema cannot express the invariant (json_valid only checks JSON syntax), so the only place left that can referee is write() itself.
    • Failing at the write site turns a Host-wide outage into a rejected call at the moment the bad value is produced, with the offending field in the error. Failing at the read site turns it into an outage that appears on the next restart, far from the cause.

    Suggested addition (same style as the existing guards, no new predicate):

    const write = (record: PersonaBotRecord): void => {
      if (!isPersonaBotRecord(record, record.slug)) {
        throw new Error(`Refusing to persist an invalid PersonaBot Registry record: ${record.slug}`);
      }
      …
    };

    With this in place the decode() sanitizer stops being the primary defence and becomes what it should be: a repair path for databases that were already damaged before the guard existed (and for the import path, which writes rows it decoded from disk rather than from memory).

    If you would rather keep the write path unchanged, the decode() patch alone is still worth taking — it just means the same class of damage has to be discovered by a Host restart instead of being rejected at the source.

  2. DoodleBears commented on Oct 31, 2026

    @DoodleBears
    Contributor

    Agent work claim

    Read the issue and all comments; no existing active claim or open PR for #3213 found. Correction to the follow-up: validation inside registry.write() cannot reject a different process's direct SQLite writes. Historical/direct-SQL recovery must therefore be enforced at the existing registry decode boundary; a sanctioned-write guard can only be defence in depth.

  3. added
    agent:activeOne or more coding-agent tasks currently claim this issue; see claim comments for task IDs
    on Oct 31, 2026
  4. DoodleBears commented on Oct 31, 2026

    @DoodleBears
    Contributor

    Agent work outcome

    Reproduction and corrected causal boundary

    All fixtures used new temporary databases, never an existing user's database. The initial unmodified implementation failed twice at createCore -> createPersonaBotRegistry -> list -> decode with the exact error Invalid PersonaBot Registry record, after closing the owner and changing a row through a separate node:sqlite connection. One case used a bare-route modelPlan plus revision 139; the other retained a valid plan but used revision -1.

    The minimized fixture needs only one SQL-inserted identity and one damaged optional field, with no legacy bot.json. Removing the damaged field makes the control pass. The actual red-capable command was:

    pnpm exec vitest run packages/core/test/registry-database.test.ts -t 'minimal direct-SQL|starts Core after direct-SQL'

    The minimized baseline was 4 failed / 1 passed, all failures carrying the exact registry exception (9.06 seconds). Adding only a write guard left all four failures unchanged (9.05 seconds): direct SQL demonstrably bypasses registry.write(). Recovering only modelPlan left two revision failures; recovering both fields made the original five cases pass. The same command now passes all 16 focused cases after expanding malformed-value coverage.

    Change

    • Use the existing application-defined registry decode boundary to discard only invalid optional modelPlan/modelPlanRevision fields, then validate the remaining complete record. Keep every Bot, valid plans, valid revision watermarks, identity/access/Memory data, and legacy model choices. A missing plan keeps existing routing semantics: a retained legacy model is not replaced; otherwise routing inherits the Host default.
    • Reads do not eagerly rewrite SQL rows. Ordinary subsequent sanctioned writes, including any pre-existing startup normalization, persist the validated snapshot. No alternate store, quarantine, row skipping, schema authority or second source of truth was added.
    • Add validation before sanctioned writes as defence in depth, not a claim to police arbitrary SQLite writers. Add regression coverage and a bilingual change fragment.
    • Existing persistence/module boundaries are unchanged, so no new ADR or DSH API mechanism is introduced.

    Evidence and limits

    • Four focused files passed: registry-database, registry, model-preset-database and operational-database: 111/111. On Windows the Git-import test needed process-local core.autocrlf=false; no repository/global Git configuration was changed.
    • pnpm lint, touched-file formatting check, pnpm changelog:check, git diff --check and pnpm build passed. Global tsc --noEmit is blocked by an untouched error at packages/client/test/mentions.test.ts:92 (TS2353, botSlug).
    • Baseline failure reproduction was at the real registry/Core factory boundary, not a complete unpatched DSH Host launch. Separately, the repaired build booted a fresh isolated DSH 0.2.1-rc.2 Host with two externally SQL-damaged Bots plus a healthy Bot. Launcher health and doctor returned authenticated API success; botharness/list returned all three. A read-only canonical SQL query during that run confirmed the bad plan/revision were still stored, proving successful startup was not a manual database repair. That task-owned Host was stopped and its temporary home deleted.
    • Client observation remained unobserved (observe exit 2); UI rendering, live model calls, Inbox/Schedule execution and full unpatched-Host failure were not verified.
    • Remaining gap: this is bounded model-settings recovery, not general per-record isolation. Other invalid required fields or unrelated malformed settings still fail record validation and may stop Core. They are not silently omitted from the Bot list. Keep that broader scope explicit rather than treating all malformed-row isolation as complete.

    No other active claim was present at handoff. The task's agent:active label can be removed while leaving this issue open for review and the stated gap.

  5. removed
    agent:activeOne or more coding-agent tasks currently claim this issue; see claim comments for task IDs
    on Oct 31, 2026
  6. DoodleBears commented on Oct 31, 2026

    @DoodleBears
    Contributor

    Agent work claim — integration

  7. added
    agent:activeOne or more coding-agent tasks currently claim this issue; see claim comments for task IDs
    on Oct 31, 2026
  8. DoodleBears commented on Oct 31, 2026

    @DoodleBears
    Contributor

    Agent work outcome — integrated locally, not released

    • Task: codex/local/01a191cc-31b6-7271-bf8b-6ed6689e15d0; state: handed-off pending overall delivery.
    • Reviewed and integrated the isolated t3/07f80e4f patch into the integration checkout; no commit, PR, push, merge or release.
    • Independently installed only its regression tests first. On the unmodified integration source, pnpm exec vitest run packages/core/test/registry-database.test.ts -t "minimal direct-SQL|starts Core after direct-SQL" failed 15 cases with the original Invalid PersonaBot Registry record error; the healthy control passed. Applied the production fix, reran the identical command: 16 passed.
    • Broader integration regressions: registry.test.ts, registry-database.test.ts and model-preset-database.test.ts — 3 files, 75 tests passed. Scoped lint, format, changelog and diff checks passed. Original handoff patch passes reverse-apply check after integration.
    • Retains the child task evidence and limits: corrected isolated Host started and authenticated API returned test Bots, but Client/model/Inbox/Schedule execution are not claimed. Arbitrary corruption outside these optional model fields remains a separate gap. The child reported an unrelated global typecheck failure at mentions.test.ts:92; not changed here.
    • Issue stays open for delivery; no changes to [Performance] Switching channels/conversations in Bot mode freezes renderer main thread at ~113% CPU (1.6.0 follow-up to #2413) #3214 or unrelated working-tree edits.
  9. removed
    agent:activeOne or more coding-agent tasks currently claim this issue; see claim comments for task IDs
    on Oct 31, 2026
  10. DoodleBears commented on Oct 31, 2026

    @DoodleBears
    Contributor

    Agent work claim — PR delivery

    The PR will use Closes #3213; no merge or release is authorized by this step. Other corruption outside optional model settings remains explicitly out of scope.

  11. added
    agent:activeOne or more coding-agent tasks currently claim this issue; see claim comments for task IDs
    on Oct 31, 2026
  12. DoodleBears commented on Oct 31, 2026

    @DoodleBears
    Contributor

    Agent work outcome — standalone PR ready for review

    • Task: codex/local/01a191cc-31b6-7271-bf8b-6ed6689e15d0; state: handed-off for review.
    • PR: fix(core): recover malformed optional Bot model settings #3239, head 38ac4b6976ec9982dd9c38f07fd281330c41f271; isolated one-commit, three-file fix rebased onto current main.
    • The PR uses Closes fix(core): one malformed persona_bots row takes down the whole Host (Invalid PersonaBot Registry record) #3213, so this issue remains open until merge. No merge or release performed.
    • Current-base reproduction: 15 failing corruption cases / 1 healthy control; restored fix passes all 16, with 4 files / 115 tests green. Root TypeScript, lint, scoped format, changelog and build pass.
    • Current-head isolated DSH rc.3 Host was started after external SQL corruption. Authenticated API returned all three test Bots; read-only SQL proved malformed fields were still stored. Task-owned Host stopped and temporary Profile removed.
    • Exact-head verification and remaining gaps are in the PR comment. No Client/model/Inbox/Schedule runtime acceptance claimed; generic other-field corruption remains out of scope. CI/review are independent gates.
  13. removed
    agent:activeOne or more coding-agent tasks currently claim this issue; see claim comments for task IDs
    on Oct 31, 2026
  14. DoodleBears commented on Oct 31, 2026

    @DoodleBears
    Contributor

    PR #3239 CI update: affected tests, core/static checks, docs build and isolated Host e2e smoke passed. The aggregate gate is blocked by Expo Doctor requiring newer patch versions in unchanged mobile dependencies; details and exact job link are in the PR. No unrelated dependency upgrade or merge bypass was made. Issue remains open pending merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions