Skip to content

fix(mcp): preserve replay and reject provider-unsafe dialect schemas (VC-621) - #747

Open
hussainph wants to merge 9 commits into
mainfrom
volli/VC-621-provider-safe-dialects-wip
Open

hussainph wants to merge 9 commits into
mainfrom
volli/VC-621-provider-safe-dialects-wip

Conversation

@hussainph

@hussainph hussainph commented Oct 4, 2026 •

Copy link
Copy Markdown
Owner

Summary

Follow-up to merged #733 (VC-621), fixing its new schema admission and baseline replay regressions without changing provider adapters or advertised/frozen schemas.

  • Keep the original bounded Ajv2020.validateSchema acceptance path first, including empty $schema and bundled 2020-12 /meta/* URIs. Pin the superset-of-pre-fix(mcp): accept recognized JSON Schema dialects (VC-621) #733 acceptance property over a corpus and exercise durable and renderer event decoding.
  • Newly admitted draft-07/2019-09 input schemas must pass their own bundled meta-schema AND a validation-only 2020-12 body check. Refuse old tuple items arrays with a safe, actionable prefixItems reason, never translate them.
  • Dual meta-validation is not full provider compatibility. Pi's actual non-strict Anthropic adapter retains only root type/properties/required. Conservatively refuse new old-draft references, legacy dependency/recursive semantics, and root assertions this projection would silently drop. Also refuse old-draft prefixItems: those drafts ignore it, and a 2020-12 interpretation can weaken the original items constraint. No speculative provider rewrite.
  • Preserve unchanged input/output objects and $schema; output schemas are never model-declared and retain their own-dialect validation. Keep all bounds, no-fetch/no-register guarantees and lazy/CSP-safe module import. Renderer replay under CSP (VC-636) remains separate; correct the inaccurate test comment.
  • Exercise the real Anthropic request converter with mocked fetch and native compaction/count-token emitted payloads with synthetic auth/fetch; no provider network or real credentials.

Verification

All passed:

  • pnpm install --frozen-lockfile --offline --ignore-scripts
  • cd packages/shared && vp test run src/mcp.test.ts src/session-event-codec.test.ts --coverage --coverage.include=src/mcp.ts --maxWorkers="$VOLLI_CONCURRENCY_HINT" — 302 tests, 100% statements/branches/functions/lines for mcp.ts.
  • cd packages/agent-runtime && vp test run src/pi/mcp-tools.test.ts src/pi/provider-compaction.test.ts --maxWorkers="$VOLLI_CONCURRENCY_HINT" — 73 tests.
  • cd packages/agent-runtime && vp test run src/pi/mcp-tools.test.ts -t 'imports the browser-bundled' --maxWorkers="$VOLLI_CONCURRENCY_HINT" — CSP import check passed.
  • cd packages/host-core && vp test run src/mcp --maxWorkers="$VOLLI_CONCURRENCY_HINT" — 254 tests, including discovery regression (also repeated discovery's 8 tests after the review fix).
  • vp run --filter @volli/shared typecheck, vp run --filter @volli/agent-runtime typecheck, vp run --filter @volli/host-core typecheck — passed.
  • vp check, git diff --check — passed, repeated after git fetch origin && volli worktree sync.
  • Re-ran the verifier's probe.mjs against an esbuild bundle of current mcp.ts, plus its raw stdio fixture through host-core's real opener/discovery path. Local artifacts in this worktree: .tmp/vc621/probe.out, discovery.out, discovery-runner.ts, extended-dialect-server.mjs.

Probe results and limits

Schema Discovery
draft-07 plain compatible subset Usable
draft-07 tuple items Refused; tells server to use 2020-12 prefixItems
2019-09 plain compatible subset Usable
2019-09 tuple items Same specific refusal
Old-draft prefixItems/items interaction Refused; tells server to declare 2020-12
2020-12 Usable, unchanged baseline behavior
Unknown dialect Refused with the original generic reason
Empty $schema, 2020-12 /meta/core Usable, baseline compatibility restored

The original fixture's d07_plain also has root additionalProperties: false. It is intentionally refused because Anthropic's projection drops that assertion. The extended local fixture proves plain draft-07 and 2019-09 without discarded root constraints remain usable. This follows the owner’s later no-silent-constraint-loss requirement; it is not a promise that every old-draft schema without tuples is provider-compatible.

Pre-existing limitations for schemas accepted before #733 remain unchanged, including dropped root definitions/constraints in provider projections. The owner separately reported Mobbin sign-in and tool use working on dev main b460a63 containing #732/#733; this Session did not perform that retest. Exact raw Mobbin dialect/schemas/cause remain unconfirmed, and this corrective follow-up makes no claim that Mobbin uses tuples. No live Mobbin/provider calls, credential reads, or user tool/config selection changes by this Session. No merge; owner/orchestrator retains merge authority after verification and a green CI gate.

Independent review

  • Standards review 8ee98afd: zero hard violations or actionable smells.
  • Spec review 07b4ec3f: found one concrete prefixItems/legacy items semantic-weakening blocker; fixed in f71dd0f3e with a red-before/green-after regression for both old dialects.
  • Independent closure check 2508a6ef: blocker resolved, baseline/output behavior unchanged, no new concrete issues.

Done: fetched/synced the ticket worktree, installed frozen offline dependencies with scripts disabled, and inspected validation, docs, runtime tests, and verifier artifacts. No source changes made.

Not done: 2020-12 body check, actionable provider rejection reason, superset-of-main compatibility rule, docs/tests, focused verification, probes, or CI. Ownership transferred to Board Session 144f856f before implementation.
@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 5c32d492-31ea-49e1-ab73-fb659261fd15
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@hussainph

Copy link
Copy Markdown
Owner Author

Live Mobbin caveat and handoff

Owner-reported Mobbin success was on dev main b460a63d0 containing #732/#733, before #747. It does not verify this corrective follow-up.

#747 deliberately refuses newly admitted old-draft root assertions such as additionalProperties: false, which Pi's non-strict Anthropic projection would discard. If Mobbin actually publishes such a schema, #747 could make its tools unusable again. Its exact raw $schema, root assertions, and tuple presence have not been established; do not infer them from the owner's successful Session.

Remaining live verification belongs to the Board/owner: retest Mobbin on a build containing #747 head f71dd0f3e258abeedc9829ad0fc2dce78da2ae11, confirm all three tools survive discovery, and verify owner-driven tool use in a fresh Session. Existing frozen tool lists cannot establish new discovery acceptance. Safely inspect raw schema fields through a permitted read-only catalog path if available; never read credentials or change user selections to do it. This Session performed no live Mobbin/provider calls.

Public evidence read in this Session via web_fetch:

All three pages expose parameter tables and schema-change excerpts (for example string/enum properties output_tool and output_destination), not a complete raw inputSchema. The returned text establishes neither root dialect nor additionalProperties, so it does not resolve this caveat. No authenticated catalog or search calls were made.

Board Session 144f856f owns CI follow-up. This Session stopped its duplicate CI watcher. PR #747 is to remain unmerged pending Board verification/authorization; no source edits were made for this caveat.

@hussainph

Copy link
Copy Markdown
Owner Author

Merge-readiness review: blocked on frozen-session compatibility

Updated this branch with origin/main (5f36dc3ff) and added CSP replay coverage for the restored empty and bundled /meta/* declarations (9d4a88105). Updated stale comments now that renderer CSP replay is implemented on main. No provider-schema rewriting or selection changes.

Blocking finding: packages/shared/src/mcp.ts:682-696 applies the new discovery restrictions to durable replay too. packages/shared/src/session-event-codec.ts:1386-1396 calls this sanitizer and throws when reading existing frozen tool definitions. Main (and tags including v0.2.2 through v0.2.3-canary.3) already contains #733, which admits draft-07 root additionalProperties: false and tuple schemas. Preserving only the pre-#733 acceptance set does not preserve Sessions created on those shipped builds.

Verified against esbuild bundles of the real codec, overriding only mcp.ts with origin/main for the baseline: both a draft-07 root additionalProperties: false schema and a draft-07 tuple schema pass main discovery and main durable decoding, but the identical recorded session.input.recorded / tool-surface payload throws in this head. This is independent of whether Mobbin uses either schema.

An owner decision is needed on how existing frozen definitions remain replayable/attachable while new discovery refuses provider-unsafe schemas. Silently dropping or rewriting the frozen tools would not satisfy the compatibility contract. I have not guessed a recovery policy or bypassed the new guards. The existing owner-requested fresh-discovery/live Mobbin verification also remains outstanding.

Targeted verification passed:

  • pnpm install --frozen-lockfile
  • vp check; shared, agent-runtime, and host-core package typechecks; git diff --check
  • Shared MCP + codec + browser-CSP codec tests: 344 passed, mcp.ts 100% statements/branches/functions/lines
  • Agent-runtime MCP + provider-compaction tests: 73 passed
  • Host-core MCP discovery tests: 8 passed

CI is running on 9d4a8810556e54e284460fb6357fb1573a648112; no merge or auto-merge was requested or performed.

@hussainph

Copy link
Copy Markdown
Owner Author

Implemented the maintainer decision in 37b9321 (after merging origin/main a662494 via 92b4e7a): previously accepted frozen schemas remain accepted as-is on durable replay. Fresh discovery still applies the provider-unsafe legacy-dialect restrictions.

The codec and whole-frozen-set validator now explicitly use replay acceptance: the unchanged JSON/bounds checks and own-dialect meta-validation shipped in #733, plus the pre-#733 Ajv2020 compatibility path. Replay skips only the new admission restrictions; it does not drop tools, rewrite schemas, or relax corruption checks. This also covers attach/surface validation callers of the frozen-set validator.

Compared main’s acceptance logic systematically against each new restriction. The real-codec corpus covers all 17 projected root assertions, tuple items, older-draft unknown keywords that fail the new 2020-12 body check, all six legacy-provider keyword exclusions, and nested schema positions, across HTTP/HTTPS and fragment aliases of draft-07, 2019-09 and 2020-12. Accepted input/output schemas round-trip byte-identically in both birth tool-surface records and refresh deltas. Renderer CSP regressions additionally pin previously frozen tuples, root additionalProperties:false, and dependencies while asserting that identical fresh discovery remains rejected.

Verification passed:

  • pnpm install --frozen-lockfile; vp check; @volli/shared typecheck; git diff --check.
  • Eight focused shared test files: 485 tests; mcp.ts and session-event-codec.ts both 100% statements/branches/functions/lines.
  • Agent-runtime MCP/provider-compaction: 73 tests; host-core MCP discovery: 8 tests.
  • CI is running on 37b9321; no merge or auto-merge performed.

Still outstanding: owner/Board live Mobbin fresh-discovery verification on a build containing the current head, confirming all three tools survive discovery and owner-driven tool use in a fresh Session. No live Mobbin/provider calls were made; frozen-session replay does not establish fresh-discovery compatibility.

@hussainph

Copy link
Copy Markdown
Owner Author

CI follow-up on current head 37b9321: the single bounded 20-minute gh pr checks 747 --watch --fail-fast timed out without any red check. Check + Build, Test (packages), all desktop tests, desktop coverage, all five N−1 lanes, host container, core/cloud smokes and smoke shards 2/3 and 3/3 are green. Smoke shard 1/3 is still in progress, so CI gate is not yet green. Run: https://github.com/hussainph/volli-code/actions/runs/39430918017 . No rerun, merge or auto-merge performed. Live Mobbin fresh-discovery verification remains outstanding.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant