feat(plugin): demand-start shared mc-host lazily - #54
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ) { | ||
| return cached.handle; | ||
| } | ||
| this.liveRoutes.delete(cached.handle.channel); |
There was a problem hiding this comment.
Bug: this.liveRoutes does not exist on McHostClient.
liveRoutes is a field of ActiveConnection (declared at line 182 and accessed everywhere else as active.liveRoutes / conn.liveRoutes, e.g. line 1803), not of McHostClient. This line calls this.liveRoutes.delete(...) from inside managedRouteHandle, where this is the client.
This branch runs whenever a cached managed route's credential fingerprints go stale (e.g. a rotated ANTHROPIC_API_KEY for an opencode/pi session) — exactly the eviction path this PR adds. It will throw TypeError: Cannot read properties of undefined (reading 'delete') instead of transparently reopening the route.
Should this be active?.liveRoutes.delete(cached.handle.channel) (guarding for active === null, mirroring the null-check just above for currentIdentity)?
| if (Array.isArray(value)) return `[${value.map(canonicalJson).join(",")}]`; | ||
| if (value !== null && typeof value === "object") { | ||
| return `{${Object.entries(value as Record<string, unknown>) | ||
| .sort(([left], [right]) => left.localeCompare(right)) |
There was a problem hiding this comment.
Locale-sensitive sort in a digest computation used for trust verification.
canonicalJson sorts object keys with left.localeCompare(right). This directly contradicts the codebase's existing canonical-JSON convention in packages/plugin/src/shared/stable-json.ts, which explicitly sorts by code-point order and documents why:
Code-point sort (NOT localeCompare). Stable across runtimes/locales.
verifyPackage (this file, ~line 232) computes sha256(canonicalJson(manifest)) and compares it against entry.payload_manifest_digest from the trust index for native payload verification. If the digest was produced with code-point ordering (or on a runtime/locale where ICU orders some key pair differently than this locale-aware sort), a legitimate untampered manifest can fail verification and verifyPackage throws native_payload_invalid, breaking managed native-host launch in a way that's very hard to reproduce/debug (it depends on the runtime's default locale).
Suggest reusing stableStringify from shared/stable-json.ts or switching this sort to plain </> comparison to match it.
| const daemonVer = active.generation.daemonVer; | ||
| if (daemonVer === null) return null; | ||
| const daemonId = active.generation.authenticatedDaemonId; | ||
| if (daemonVer === null || daemonId === null) return null; |
There was a problem hiding this comment.
Behavior change: authenticated now returns null for a connected peer with no daemon id, instead of a partial authenticated record.
Previously (per the diff) this getter returned { daemonVer, daemonId: active.generation.authenticatedDaemonId, proof: "current" } whenever daemonVer !== null, even if daemonId was null. Now it returns null outright when daemonId === null.
connection.ts:752 sets authenticatedDaemonId = null whenever the handshake result's daemonId isn't a Uint8Array, while still setting daemonVer and advancing phase to "frames" — i.e. a live, usable connection can have a null daemon id.
probeManagedReadiness in managed-policy.ts (~line 79) treats authenticated === null as fatal: throw new Error("authenticated peer disappeared"). With this change, a channel/provider whose handshake legitimately omits a daemon id gets misclassified as "peer disappeared" rather than a degraded-but-authenticated/live state.
Today this only seems to be exercised by ShmFrameChannel's test-only path, but flagging since it's a real contract change on a shared getter — worth confirming no production channel can produce a null daemon id, or restoring the partial-record behavior if one can.
| "missing_identity", | ||
| ); | ||
| } | ||
| const identity = baseIdentity; |
There was a problem hiding this comment.
routeCacheKey (used a few lines below via identity) never actually varies by credential fingerprint on first use.
routeCacheKey (line ~2210) includes identity.credential_fingerprints in the key it builds, but identity here is just baseIdentity — the caller-supplied identity — before it's ever passed through identityForConnection. identityForConnection only runs later: in the new staleness-recheck branch below (this.identityForConnection(active, baseIdentity)) or inside openCachedRoute.
So on first use, two calls with the same project_root/harness/session but different credential fingerprints hash to the same this.routes key — the credential-scoping this PR adds only takes effect via the post-hoc recheck-and-evict branch a few lines down (which itself has a bug, see the this.liveRoutes.delete comment on that branch), not at the cache-key level where routeCacheKey's new logic implies it should apply.
Is relying entirely on the recheck-and-evict path intentional, or should identityForConnection be applied before computing key?
Review summaryReviewed the demand-start lifecycle changes for the shared
Also noted but not inline (lower severity / cleanup):
Tests, docs, and the overall lifecycle-owner design (bounded deadlines, KTD invariants, passive probing) look solid — the concerns above are all in the credential-fingerprint routing and digest-verification code added in this PR. |
1 participant
Summary
Stack
PR 7 of 10. Base:
stack/mc-host-06-harness-runtime.Validation
Post-Deploy Monitoring & Validation
Watch managed start outcomes, storage-starting waits, Synapse readiness, and fallback rates for one release cycle. Roll back if passive probes start a daemon or cancelled callers terminate shared startup. Owner: plugin maintainers.