feat(runtime): add Plugin Platform foundation - #3729
Conversation
d399570 to
71803a2
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found blocking issues.
[P2] Install rollback can silently fail while the new package appears committed
plugin-package-store.ts:193 reports persistence_failed when both new target and .previous-* exist, but recovery later deletes the old package and confirms the new executable. Must report commit_outcome_unknown and fence until the directory is durable.
[P2] Fence check is outside the serialized queue
Two same-tick mutations both pass #assertMutable() before the fence is set; the second queued mutation still executes and overwrites the ambiguous state. Must re-check inside the serialized callback.
[P2] Output size checked after commit
Desired tree size (512 KiB) is only checked on return. A store within the 2 MiB input limit can accumulate entries that make apply/query responses exceed the limit after commit, leaving the commit durable but the caller with internal_failure.
[P3] Foundation without concrete consumer
~4.7k lines introduce client/UI/config APIs with no consumer yet; many helpers are unused. Consider delivering as minimal vertical slices rather than a large foundation.
Checks on 71803a267d are not green due to local-only surface enumeration mismatch — not green.
简体中文
存在三项持久化/并发/输出阻断与一项熵增观察。
e535c71 to
580a099
Compare
xxhZs
changed the title
|
Addressed all four findings in
Verification: full workspace build, Biome, ASF headers, and diff check passed; Runtime 3037/3037 active tests and CLI 455/455 passed; Plugin Platform + CLI focused coverage is 25/25. Runtime Host passed 1195/1196 in the parallel run; the sole unrelated shared registration-directory race passed 3/3 isolated reruns. |
580a099 to
e535c71
Compare
xxhZs
changed the title
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Update on e535c712e3:
[P1] Package replacement publishes before authority commit
plugin-package-store.ts:146-202 renames new bytes to canonical before commit(); plugin-platform.ts:154-189 reloads Entries before Composition authority commits. On crash between publish/authority commit, restart replays old packageLayers with new patch — mixed durable authority. Also 193-199 silently demotes restore/sync failures to persistence_failed and no longer fences unknown canonical.
Fix: atomize publish with authority commit before converging runtime; restore fencing for unknown persistence.
[P1] Queued mutation bypasses unknown-outcome drain
Removal of #assertMutable() inside queue allows a queued operation to execute after a prior commit_outcome_unknown drain/poison, overwriting the prior authority fact with stale in-memory state.
Fix: re-check #assertMutable() at dequeue time and keep commit_outcome_unknown fencing; restore regression test.
[P2] Uninstall leaves authority mutated on package_in_use failure
plugin-platform.ts:247-260 deletes layer and converges before checking surviving overlay still uses package — default layer already removed when error returned.
[P2] Response codec limit below durable limit
Composition allows 2 MiB but query/apply response capped 512 KiB — accumulated authority can outgrow response and become un-decodable after commit.
Also hard Standards breaches on phase ordering and close aggregation require fix.
Checks on e535c712e3 are test: IN_PROGRESS — not green.
简体中文
存在包替换与排队栅栏等阻塞。
e535c71 to
85b5aac
Compare
|
Reworked the current head against the latest review in
Added crash-boundary, queued-fence, uninstall, response-size, close-aggregation, and CLI consumer regressions. Full Runtime (3037), Runtime Host (1198), and CLI (455) suites pass, along with full build, Biome, ASF, Windows inventory, and diff checks. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Third review pass on 85b5aac09. All four findings from the previous round are genuinely fixed — I verified each: the fence now re-checks at the serialized dequeue point, replacement journals base→next, both package_in_use checks moved ahead of the authority commit, and paging bounds the response. Bundle import defenses (safePath, sha256, symlink rejection, open(...,'wx')) are sound; I could not construct an escape.
This round found four P1s. Details are inline; two mechanisms span files the diff does not touch, so they are here.
The remote-owner grant is the one I would look at first. operations.ts adds all six operations to REMOTE_OWNER_OPERATION_GRANTS. Four of them (plugin.composition.apply, plugin.package.uninstall, plugin.package.reload, plugin.platform.query) are plain defineOperation, so canUseHostPaths: false does not gate them. runtime-host-access-command.ts:251 — unchanged by this PR — builds every --preset credential from that whole list. A preset-issued remote credential can therefore activate a package into any root, remove or disable any Entry, and uninstall packages outright, all durably. "Packages are trusted code" is the accepted premise of this PR, but it governs what the code may do, not which principal decides when and where it runs.
The provide collision needs plugin-kernel.ts to see: #label() returns a kernel-global symbol when no isolate mapping exists, and provide() throws if that label is already in #kernel.services. Every make-before-break path stages the new Fiber while the old one still holds the label, so move and reload fail for any Entry calling ctx.provide. isolate does not help — it makes the service private to the subtree. Graded P2 only because this PR ships no contribution consumers yet, so no service-providing plugin exists in tree; it must be fixed before one does.
Three P3s that have no line in the diff to attach to: validatePluginRootId accepts any non-empty session: scopeId, so __proto__ throws a TypeError from siblings.splice instead of being cleanly rejected; decodeScalarRecord/decodeIsolate silently drop prototype-named keys rather than rejecting them; and validateExtensionConfiguration reads input[key] through the prototype chain, so a property named toString/constructor/valueOf makes the package permanently unconfigurable with an error that blames the operator.
Simplification — not blocking, but this is a new 5.6k-line subsystem and worth doing before it grows consumers
Dead on arrival: MakaCompositionLoader.restoreComposition (no consumer, not even a test), walkLive, PluginPackageStore.list(), PluginPackageStore.install() (only caller is a test, and it hardcodes publish(0, 1)), and the .previous-/.staging-/.rejected- branches in recover() — no production path writes a top-level directory with those prefixes.
Duplicate authorities: extension-bundle.ts and plugin-package-store.ts carry near-identical MAX_FILES/MAX_FILE_BYTES/16 MiB limits plus line-for-line copies of safePath and collect; extension-package-manifest.ts:138 holds a third copy of the path predicate that omits the posix.normalize check, so the manifest accepts paths the store rejects. PluginPackageLoaderError + translate() is an identity relabeling — the coordinator's two mapping blocks are character-for-character the same policy. #draining is set alongside #poisoned at all five fence sites, and #poisoned alone already fences; only beginDrain() needs it.
简体中文
第三轮，针对 85b5aac09。上一轮四条都真修好了，逐条验过。本轮四条 P1，详情在行内。
最该先看的是远程授权：六个操作全进 REMOTE_OWNER_OPERATION_GRANTS，其中四个不受 canUseHostPaths 约束，而 preset 从整张表签发凭据。“包是可信代码”管的是代码能做什么，不是哪个主体决定它何时何地运行。
另外两条 P1 在包存储的崩溃恢复上：rollback 中途被杀会导致重启时删掉已恢复的旧包；journal 缺失被当成损坏，导致整个插件子系统跨重启永久围栏。这两条所在的 #recoverInstall 目前测试覆盖为零。
Reviewed with help from Claude.
| 'plan.query', | ||
| 'plan.turn.start', | ||
| 'plugin.composition.apply', | ||
| 'plugin.package.export', |
There was a problem hiding this comment.
[P1] All six operations land in REMOTE_OWNER_OPERATION_GRANTS, but only install and export are defineHostPathOperation. apply, uninstall, reload and query are plain defineOperation, so a remote credential with canUseHostPaths: false still gets them — and presets are built from this entire list. That lets a remote principal durably rewrite Host plugin authority. Was this deliberate? Nothing in the diff says so. Suggest local-owner-only for the foundation, or at minimum dropping the mutating four from the preset.
| const candidateExists = await exists(candidate); | ||
| const targetExists = await exists(target); | ||
| const previousExists = await exists(previous); | ||
| if (authorityGeneration === transaction.baseGeneration) { |
There was a problem hiding this comment.
[P1] This branch infers "the directory at target is the candidate's bytes" from !candidateExists && targetExists, but that is also true after a partial rollback. rollbackPublishedInstall renames target→rejected, then previous→target, then rejected→candidate, with no sync between. Kill the process after step 2: target holds the restored good package, the transaction dir holds transaction.json + rejected, authority is still at baseGeneration. Recovery then rms the restored package and has no previous to put back. Rollback is the ordinary failed-upgrade path, so this is one crash away.
| } | ||
|
|
||
| async function readTransaction(root: string): Promise<PackageInstallTransaction> { | ||
| let value: unknown; |
There was a problem hiding this comment.
[P1] readTransaction catches everything from readFile, so a missing transaction.json is treated exactly like a corrupt one. #recoverInstall throws before its rm(transactionRoot), recover() catches into #poisoned, and #poisoned is never cleared anywhere — so every subsequent start re-fences and only manual filesystem surgery recovers.
The window is large: prepareInstall creates the transaction directory but writeTransaction only runs inside publish(), so it spans writing up to 256 files, the cp, and await import() of plugin code. commit()'s rm -rf unlinks children first, so a successful install can produce this state too.
ENOENT is provably safe to discard — the journal is written and fsynced before the first rename, so no journal means publish() never ran.
| await platform.installPackage(await writeFixturePackage(root, 'recover-package', 'recover')); | ||
| await platform.close(); | ||
| const packages = join(control, 'plugin-packages-v2'); | ||
| await rename(join(packages, 'recover-package'), join(packages, '.previous-owner-death')); |
There was a problem hiding this comment.
[P1] This renames a package to a top-level .previous-owner-death, but prepareInstall creates previous at join(transaction, 'previous') — inside the .install-<uuid> directory. No production path writes a top-level .previous-*, so this tests a state owner death cannot leave behind, and its only real effect is keeping the unreachable .previous- branch alive.
The larger point: nothing in the suite ever creates an .install-<uuid>/ with a transaction.json, so #recoverInstall's three-way generation arbitration — the crash-consistency authority of this subsystem — has zero coverage. Both P1s I filed on plugin-package-store.ts live there.
Relatedly, all four fault-injection stores override replace(), so no test makes a syscall fail. Moving published = true one line earlier in HostPluginCompositionStore.replace keeps the suite green while inverting the commit_outcome_unknown classification the whole fence rests on.
| const next = compositionAuthority( | ||
| planned.generation, | ||
| this.#authority.packageLayers, | ||
| Object.freeze([...this.#authority.overlays, ...normalizedInput.operations]), |
There was a problem hiding this comment.
[P2] Overlays are append-only and nothing ever shortens them — an insert and its later removal both live here forever. Once the log crosses 4096 operations or the 2 MiB file cap, store.replace throws and apply reports persistence_failed with "Runtime state was not changed": accurate, permanently true, and pointing at the wrong cause. Every later apply fails identically and recovery means hand-editing plugin-composition-v2.json. Startup also replays the whole log every time.
| authority: PersistedPluginComposition, | ||
| ): Promise<MakaCompositionState> { | ||
| let working = emptyCompositionState(); | ||
| for (const extensionId of authority.packageLayers) { |
There was a problem hiding this comment.
[P2] This folds packageLayers + overlays the same way #composeLayers does, but with different rules: #composeLayers normalizes configuration per layer inside the fold and then validates, while this applies every operation raw and normalizes the whole tree afterwards, with no validation. The desired tree after a restart is therefore derived by a different code path than the one that accepted it.
Recovery does need to be fail-open and to adopt the durable generation, but those are parameters, not a second algorithm — one fold taking {validate, generation} covers both and removes the ordering difference.
| ); | ||
| } | ||
| if (planned) { | ||
| await this.#replaceDesiredComposition(planned, packageLayers, this.#authority.overlays); |
There was a problem hiding this comment.
[P2] The package_in_use checks correctly moved ahead of this line, but the failure path after it did not. #replaceDesiredComposition durably commits the authority without the package layer; if packages.uninstall then fails with a non-ENOENT rename error it throws persistence_failed, and the catch restores the runtime without ever restoring the authority.
The result is durable authority saying "uninstalled", bytes still on disk, runtime still holding the package, and the caller told it failed — and after restart the patch is never replayed, so the package silently disappears.
| const replacement = await this.#stage( | ||
| serialize(current), | ||
| current.rootId, | ||
| current.parent, |
There was a problem hiding this comment.
[P2] #reloadPackage stages the new mount before disposing the previous one, and the same make-before-break shape is in #rebind, #replace, and #replaceComposition. Because PluginContext#label() falls back to a kernel-global symbol and provide() rejects a label already in #kernel.services, the staged Fiber's ctx.provide throws while the old Fiber still holds it — so move and reload fail for any Entry that provides a service.
isolate is not a workaround: it gives the provider a fresh private symbol each stage, so consumers looking up the global one no longer find it. No test covers this because every provide in the suite happens on loader.root or an external Context, never on a plugin entry.
| // that case would unnecessarily dispose the current Fiber and lose | ||
| // its registered contributions. | ||
| if (appliedOperations > 0) await this.#replaceSnapshot(before, 'rollback'); | ||
| if (appliedOperations > 0) await this.#replaceComposition(before, 'rollback'); |
There was a problem hiding this comment.
[P2] Two things here.
before is the full compositionState() — profile, desktop-ui, and every session:* — so a batch that fails after one applied operation stops and restarts every plugin Fiber in every unrelated session, losing in-memory state and re-registering contributions. That contradicts the comment just above, and the suite treats "does not restart unrelated Entries" as a correctness requirement for reload at :714.
Second: if #replaceComposition itself throws, #roots/#entries keep the half-applied state, the generation counter is not advanced, and throw error on the next line is never reached — so the original failure reason is replaced by the rollback error. An AggregateError([error, rollbackError]) would at least preserve it.
| for (let index = cursor; index < values.length && items.length < limit; index += 1) { | ||
| const candidate = [...items, values[index] as T]; | ||
| if ( | ||
| Buffer.byteLength(JSON.stringify({ view, items: candidate, nextCursor: index + 1 }), 'utf8') > |
There was a problem hiding this comment.
[P3] This 480 KiB budget and MAX_FRAME_BYTES = 512 * 1024 in protocol/plugin-platform.ts:55 are one budget written twice, with a 32 KiB implicit headroom, and the two measure different objects (pre-encode structure here, decoded result there). Today the headroom covers it; changing either alone will not be caught, since no query test ever produces more than one item.
2 participants
Summary
This PR adds an end-to-end Runtime Host Plugin Platform: trusted Package ingestion, Package-owned Composition patches, durable Package layers plus User overlays, derived Desired Entry state, Runtime convergence, crash recovery, bounded inspection, and CLI management.
The resolved Entry Tree is not persisted. The durable composition authority contains only
generation, orderedpackageLayers, and finaloverlays. Startup replays installed Package patches and User overlays, derives the Desired Entry Tree, and restores healthy Runtime Entries.Runtime composition
MakaCompositionStateinsert,update,move, andremoveoperationsprofile,desktop-ui, and per-Session rootsMakaCompositionLoaderwith Package-wide reload, full replacement, fail-open recovery, and state exportPackage contract and storage
Packages use
maka.extension.jsonand may declare a bounded YAML Composition patch. The manifest supports display metadata, Package dependencies, typed configuration properties, defaults, enums, required keys, and a trusted Runtime entry..maka-extensionBundle imports/exportsCrash-safe replacement transaction
Package replacement uses a recoverable transaction:
The transaction retains the previous Package until the Composition authority outcome is known. Startup compares the journal with the durable authority generation:
baseGeneration: restore the previous PackagenextGeneration: retain the new PackageUnknown Package or Composition commit outcomes return
commit_outcome_unknown; a newly established fence is checked again when every queued mutation reaches the serialized dequeue point.DSH-style Package composition layers
Package patches are applied in installation order and User overlays are applied last:
Bounded protocol and concrete CLI consumer
The Runtime Host exposes:
plugin.platform.queryplugin.package.installplugin.package.uninstallplugin.package.reloadplugin.package.exportplugin.composition.applyplugin.platform.queryuses boundedstatus,packages,entries, andfailuresviews. Collection views are cursor-paged and capped by item count and encoded byte size. Entry inspection is flattened per page while retaining parent identities.Composition apply accepts a bounded command and returns only the committed generation. The durable Overlay store may accumulate up to its 2 MiB authority limit without making a successful commit impossible to acknowledge.
Every public operation has a CLI consumer:
Runtime Host recovery, drain, close, authorization, strict codecs, and compatibility epoch 50 are wired for this lifecycle. Close attempts every owned resource and aggregates failures.
Startup recovery
Scope
This PR does not add Desktop management UI, Agent-facing management tools, Marketplace integration, concrete Tool/UI/Hook/Event contribution consumers, or a per-Run generation lease.
Verification
git diff --checkpassed