fix(plugin): validate installed.json records instead of casting them

[LHMQ878]

Problem

readInstalled validated the file envelope but not its records:

const parsed = JSON.parse(text) as InstalledFile;
if (typeof parsed !== 'object' || parsed === null || !Array.isArray(parsed.plugins)) {
  throw new Error('installed.json is not a valid InstalledFile object');
}
return parsed;

The as InstalledFile cast makes every non-optional field in InstalledRecord an unchecked assumption about on-disk data — and installed.json is hand-editable, survives downgrades, and is written by older versions.

Consumers dereference those fields unconditionally, so a malformed record surfaces as a crash far from the file that caused it:

• PluginManager.load does next.set(entry.id, await this.materialize(entry)) for every entry (packages/agent-core/src/plugin/manager.ts:44-51, and the v2 counterpart at packages/agent-core-v2/src/app/plugin/manager.ts:62-69). A null entry throws Cannot read properties of null (reading 'id').
• materialize then passes an unvalidated entry.root straight into parseManifest.
• The plugins status panel renders `${info.github.ref.kind}:${info.github.ref.value}` behind only an info.github !== undefined guard, so a github record persisted without a ref throws Cannot read properties of undefined (reading 'value').

persist() also writes such records back out unchanged, so a bad edit is durable.

Fix

Validate each record with zod (already a dependency of both packages) and fail the read naming the offending index:

Failed to parse <home>/plugins/installed.json: plugins[1] is not a valid installed record: …

Rejecting rather than dropping is deliberate, and matches the contract already in the code:

• The existing test is named throws on a corrupt installed.json instead of silently dropping it.
• Both managers already treat a load failure as a first-class state: core-impl.ts:277-282 captures it, and assertPluginsLoaded (core-impl.ts:1162) rethrows from /plugins and every mutator with "Fix the file at …/plugins/installed.json and run /plugins reload", while createSession degrades silently so the harness still starts. PluginServiceImpl.loadOnce does the same in v2.
• So the failure path is already safe: mutators rethrow before reaching persist(), meaning a rejected load cannot overwrite the file with a half-loaded map.

Silently dropping records would instead make a user's installed plugins vanish with no explanation.

Unknown keys are preserved (z.looseObject) so a record written by a newer version round-trips through an older one instead of losing fields on the next write — covered by a test.

Applied to both copies of the store (packages/agent-core and packages/agent-core-v2), which are otherwise identical apart from line width.

Tests

packages/agent-core/test/plugin/store.test.ts (added to the existing file, no new test file): a table of 9 rejected shapes — null, a bare string, missing id, non-string root, unknown source, enabled: 'yes', installedAt: 5, a github record with no ref, a github record with ref.kind: 'commit' — plus one asserting the index is reported for the second record, plus the unknown-key round-trip.

packages/agent-core-v2/test/app/plugin/manager.test.ts: one consumer-side test that load() rejects with the index instead of crashing on entry.id.

Verification

Control experiment, reverting only the implementation and re-running:

• packages/agent-core/src/plugin/store.ts reverted → 10 of the new tests fail, each as AssertionError: promise resolved "{ version: 1, plugins: [ null ] }" instead of rejecting — i.e. the bad record really was handed back typed as an InstalledRecord.
• packages/agent-core-v2/src/app/plugin/store.ts reverted → the manager test fails with expected [Function] to throw error matching /plugins\[0\]/ but got 'Cannot read properties of null (reading 'id')', reproducing the crash-far-from-cause this change removes.

Suites, against a clean-main baseline (I'm on Windows, so some pre-existing failures are environmental):

	baseline on main	with this change
agent-core test/plugin	138 passed, 3 failed	149 passed, 3 failed
agent-core-v2 test/app/plugin + test/agent/plugin	80 passed, 1 file failed to load	81 passed, 1 file failed to load

The 3 agent-core failures are identical on main (symlink handling and frontmatter parsing); the v2 file that fails to load is bashParserService needing a built @moonshot-ai/tree-sitter-bash. No new failures.

• agent-core tsc --noEmit: clean.
• agent-core-v2 tsc --noEmit: only the same two pre-existing Cannot find module '@moonshot-ai/tree-sitter-bash' errors (unbuilt workspace package).
• oxlint on the four changed files: 0 errors; 1 warning on a pre-existing line (manager.test.ts:135, untouched).

Note

I found this while trying to reproduce #2553 (kimi-cli). I could not confirm it is that crash — the github.ref path is only reached from the plugin details view, not when /plugins opens — so this is filed on its own merits, not as a fix for that issue.

[changeset-bot]

🦋 Changeset detected

Latest commit: f6127ae

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@moonshot-ai/agent-core	Patch
@moonshot-ai/agent-core-v2	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 79f9b1b4f6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Preserve future GitHub metadata during validation

When a newer install record has extra fields inside github (for example metadata added by a later client), this z.object parse strips those keys before readInstalled returns, so a caller that reads and writes the file back no longer round-trips the record even though the top-level schema is loose. Please make the nested metadata/ref schemas loose or passthrough as well; the v2 copy has the same issue.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Remove the v2 inline rationale comment

In the v2 package, comments are constrained to the top-of-file /** */ module header, but this new block comment sits in the body next to schema declarations and explains implementation rationale. Please remove it or fold only the module-level responsibility into the header so the file stays within the v2 comment convention.

AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L13-L13

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Keep the v2 test rationale out of the body

This new inline test comment is inside a v2 test body, but the scoped guide allows comments only in the top-of-file /** */ header rather than beside statements. Please encode this rationale in the test name/assertion or the file header instead of leaving an inline body comment.

AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L13-L13

Useful? React with 👍 / 👎.

[LHMQ878]

Thanks — all three addressed in f6127ae.

P2, preserve future GitHub metadata — this was a real bug, and worse than described

You're right, and the reach is wider than github: the top-level schema was looseObject, but every nested schema was z.object, which strips. So github, github.ref and each capabilities.mcpServers entry all dropped unknown keys.

The reason this matters more than a lossy read is that the store is read-modify-write — PluginManager calls readInstalled, mutates, then writeInstalled — so a stripped read doesn't just hide a field from one caller, it persists the loss on the next write.

Reproduced against the schemas as they stood, with a record as a newer client would have written it:

input  : {"id":"p",…,"futureTopLevel":"kept-by-loose",
          "github":{"owner":"o","repo":"r","ref":{"kind":"tag","value":"v1","resolvedAt":"T"},
                    "installedSha":"abc","signature":"sig-from-newer-client"},
          "capabilities":{"mcpServers":{"s":{"enabled":true,"transport":"stdio"}}}}
current: {"id":"p",…,"github":{"owner":"o","repo":"r","ref":{"kind":"tag","value":"v1"},
                               "installedSha":"abc"},
          "capabilities":{"mcpServers":{"s":{"enabled":true}}},"futureTopLevel":"kept-by-loose"}

current loses github.signature       : true
current loses github.ref.resolvedAt  : true
current loses mcpServers.s.transport : true

Note futureTopLevel survives — which is exactly why the existing round-trip test I'd added passed. It only asserted on a top-level unknown key, so it certified a guarantee the nested levels were quietly breaking.

Fixed in both copies by making every level loose, and the test now covers the nesting and the rewrite:

it('keeps unknown keys nested inside github and capabilities, and survives a rewrite', async () => {
  await writeRaw(home, { version: 1, plugins: [fromNewerVersion] });
  const read = await readInstalled(home);
  expect(read.plugins[0]).toEqual(fromNewerVersion);

  // The load-edit-save path an older client actually takes.
  await writeInstalled(home, read);
  const reread = await readInstalled(home);
  expect(reread.plugins[0]).toEqual(fromNewerVersion);
});

Control experiment — I reverted only the nested schemas back to z.object and re-ran, to confirm the test actually catches it rather than passing for its own reasons:

- with nested z.object   : 1 failed | 17 passed   <- diff names exactly signature,
                                                     ref.resolvedAt, mcpServers.s.transport
- with nested looseObject : 18 passed

The 17 pre-existing tests pass either way, which confirms nothing else in the suite covered this.

P1, v2 inline rationale comment

Moved into the top-of-file /** */ header, in the identity-line form the sibling manager.ts uses. packages/agent-core-v2/src/app/plugin/store.ts had no module header at all before, so this adds the missing one rather than just relocating text:

/**
 * `plugin` domain (L3) — owns the on-disk `installed.json` record store.
 *
 * Reads, validates, and atomically rewrites the installed-plugin file for
 * `PluginManager`. Records are validated rather than cast because the file is
 * hand-editable and survives downgrades; every schema is loose at every level
 * so a record written by a newer client round-trips through an older one.
 */

The v1 copy keeps its inline block comment — packages/agent-core/ has no scoped AGENTS.md, and the surrounding code there comments inline — with the nesting rationale added.

P1, v2 inline test comment

Folded into the test name:

-  it('fails the load with the offending index when installed.json holds a bad record', async () => {
-    // `load()` dereferences `entry.id` and `entry.root` for every record, so a
-    // malformed one has to be rejected at the store instead of crashing here.
+  it('rejects a bad installed.json record at the store, naming its index, rather than dereferencing it in load()', async () => {

Verification

agent-core     test/plugin/store.test.ts   18 passed
agent-core     test/plugin/                3 failed | 150 passed
agent-core-v2  test/app/plugin/            81 passed  (9 files)
oxlint (4 changed files)                   0 errors
tsc -p packages/agent-core                 clean
check-domain-layers                        OK (985 files)

The 3 agent-core failures are identical on unmodified main with my changes stashed (3 failed | 149 passed — same three, one fewer total since this PR adds a test). Two are EPERM: operation not permitted, symlink from running as a non-admin Windows user (manager.test.ts:139, manifest.test.ts:171); the third is a CRLF-sensitive frontmatter assertion in commands.test.ts:12. None touch the plugin store.

tsc -p packages/agent-core-v2 reports two errors, both TS2307: Cannot find module '@moonshot-ai/tree-sitter-bash' in src/app/bashParser/bashParserService.ts — an unbuilt workspace dependency in my checkout, unrelated to these files.

---

AI disclosure: I used an AI coding assistant while investigating and preparing this change. I reviewed the diff line by line, ran every command above myself, and can explain why the nested levels needed the same treatment as the top level.