feat(runtime): add plugin composition foundation - #3250
Conversation
|
Nice direction overall. Splitting the lifecycle kernel from Tool/UI/Event makes the extension work much easier to review and evolve. Before treating K0–K4 as stable, I think two invariants need tightening:
One boundary is also worth making explicit: I do not consider the absence of Tool/UI/protocol wiring a defect in this PR—the narrow slice is a strength. But I would not call the foundation safely unloadable until the cleanup-failure case is handled, or deterministically recoverable until immutable package identity is documented and enforced. 中文摘要整体方向赞成：
本 PR 没有接入 Tool/UI/protocol 是合理的窄切片，不是缺陷；但 cleanup failure 与不可变 package identity 应成为底座稳定前的门槛。 |
xxhZs
changed the title
|
One additional concern after looking specifically at the CAS and recovery semantics: The Store-level CAS is a good no-lost-update mechanism: it locks, re-reads the current revision, validates
I would add Kernel-level tests—not only Store tests—for two complete Kernels competing, recovery under the Host owner lease, CAS-loser fencing/reconciliation, and crash injection immediately before and after the durable commit. In short: the current CAS proves no lost Entry update, but not yet single activation, deterministic conflict recovery, or uncertain-outcome reconciliation. 中文摘要当前 Store CAS 能避免 Entry snapshot 丢更新，但还没有覆盖完整的 Extension authority 恢复语义：
因此现有 CAS 证明的是“Entry 不丢更新”，还没有证明“Fiber 单实例、冲突可恢复、未知结果可对账”。建议增加完整 Kernel 竞争、owner lease、CAS loser 和 commit 前后 crash-injection 测试。 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for putting the Entry/Fiber kernel behind a clear authority boundary. The Store CAS, atomic persistence, stage-before-commit shape, and focused tests are a solid foundation, and this PR has no UI/UX screenshot requirement.
I cannot approve this exact head. I found four additional current-head issues and added them inline below. The existing public findings about exhaustive cleanup, immutable package identity, owner/lease fencing, CAS-loser cleanup, and activation before durable CAS also remain actionable; I have not duplicated those threads.
The PR body and all three commit bodies currently contain no AI-use statement or Generated-by: trailer. Please explicitly state whether generative tooling made a material contribution. If it did, disclose the tool and scope in the PR body and add truthful trailers to each materially assisted commit; if it did not, say that clearly in the body.
Several required workflows are still queued/running, so CI is not yet a merge-ready signal.
AI-assisted review disclosure: OpenAI Codex performed the exact-head lifecycle, authority, module-loading, reconciliation, provenance, and CI analysis; I verified each reproduction, severity, deduplication, and smallest-fix direction before posting.
中文说明
当前新增四个问题：dispose 与 mutation 的生命周期竞态；reload 只 cache-bust 入口而无法刷新静态依赖；Entry 可变引用绕过 authority；structural parent 改挂后不重建后代。已有公开评论中的 cleanup、package identity、lease/CAS fencing 等问题也仍有效，不重复开线程。
另外 PR body 和 3 个 commit 都没有 AI 使用说明。请明确是否有实质性 AI 辅助；若有，body 说明工具与范围，并给相应提交补 Generated-by:；若没有，也请在 body 明确说明。
| } | ||
|
|
||
| async dispose(): Promise<void> { | ||
| await this.#mutationTail; |
There was a problem hiding this comment.
[P1] Serialize disposal with mutations for the full lifecycle transition
dispose() only awaits the mutation tail as it exists at entry. After fibers are cleared, #started remains true while disposers run, so a concurrent install() can successfully commit and activate a new fiber; dispose() then marks the kernel stopped. I reproduced durable and active entries containing the new fiber while every later mutation fails with “Extension kernel has not started,” leaving an unmanaged live effect. Please put recover/restart/dispose and mutations behind one lifecycle queue or explicit state machine, and add this interleaving regression.
中文说明
dispose 只等调用瞬间的 mutation tail；清空 Fiber 后到 disposer 结束前仍显示 started。此窗口 install 会成功激活新 Fiber，随后 dispose 又把 kernel 标成 stopped，留下无法再管理的活跃 effect。应让生命周期操作与 mutation 共用同一队列或状态机。
There was a problem hiding this comment.
Fixed in . /// and every mutation now share one lifecycle queue. The new interleaving regression holds a disposer open, queues an install, and verifies the install is rejected after disposal without a durable or active Fiber leak.
| const modulePath = await this.#resolvePath(entry.packageId); | ||
| const generation = (this.#generation += 1); | ||
| const moduleUrl = pathToFileURL(modulePath); | ||
| moduleUrl.searchParams.set('makaExtensionGeneration', String(generation)); |
There was a problem hiding this comment.
[P2] Reload the extension's whole module graph, not only its entry URL
The generation query cache-busts only the entry module. Relative static imports such as ./dep.mjs keep the same URL and remain in Node's ESM cache. I reproduced changing dep.mjs, calling reload('one'), and receiving ["old", "old"]. Please load each immutable package revision from a new content-addressed/versioned directory so every module URL changes, or narrow/remove the same-path reload contract. Add a multi-file package regression.
中文说明
generation query 只改变入口 URL，静态相对依赖仍命中 Node ESM cache；修改 dep.mjs 后 reload 仍得到旧值。应让整个模块图位于新的不可变版本目录，或收窄当前 reload 契约，并补多文件扩展测试。
There was a problem hiding this comment.
Fixed in . Package Entries now pin an immutable SHA-256 digest, and the trusted registry resolves each digest to a distinct immutable package directory. The entry URL no longer query-busts a mutable path. A two-version, multi-file static-import regression verifies then module graphs.
| if (entries.some((candidate) => candidate.entryId === entry.entryId)) { | ||
| throw new Error(`Extension entry ${entry.entryId} already exists`); | ||
| } | ||
| entries.push(entry); |
There was a problem hiding this comment.
[P2] Keep mutable caller references outside the Entry authority
Install/reconfigure retain caller-owned objects, while the entries getter also returns the internal objects directly. Mutating the original entry after install can make the kernel report enabled: false and changed configuration while the fiber remains active and the durable snapshot still contains the old values. Canonically validate/clone JSON-compatible entry data at mutation boundaries and do not expose mutable internal references; add input- and getter-mutation regressions.
中文说明
当前既保留调用方传入对象，也直接暴露内部 Entry 对象。安装后修改原对象即可让内存视图、活跃 Fiber 与 durable snapshot 三者分裂。请在 authority 边界做规范化 clone/validation，并避免泄露内部可变引用。
There was a problem hiding this comment.
Fixed in . Entry values are canonically validated/cloned after mutation callbacks and at recovery/getter/notification boundaries. The regression mutates both the original nested configuration and a getter projection and verifies kernel/store authority remains unchanged.
| const forceReload = forceReloadIds.has(entry.entryId); | ||
| const parentChanged = entry.parentId ? changed.has(entry.parentId) : false; | ||
| const active = entry.enabled && !hasDisabledAncestor(next, entry); | ||
| if (!active || !entry.packageId) { |
There was a problem hiding this comment.
[P2] Rebuild descendants when a structural ancestor is reparented
For an enabled Entry without packageId, this branch only records changed when it currently has a fiber or is force-reloaded. If structural group G moves from provider A to provider B, G's fingerprint change is not propagated, so child C is reused with A's injected context while the durable graph says its ancestor is B. I reproduced only one activation with provider A after the reparent. Mark changed structural entries from their fingerprint/parent change and rebuild affected descendants; add the A→B reparent regression.
中文说明
无 package 的结构节点改挂父节点时不会进入 changed，后代因此不重建：durable graph 已指向 B，但活跃 child 仍持有 A 的上下文。请传播结构节点的 fingerprint/parent 变化并补改挂回归。
There was a problem hiding this comment.
Fixed in . Structural Entries now propagate their own fingerprint/parent changes through , even without a Fiber. The A→B structural reparent regression verifies the descendant is rebuilt and injects B after previously injecting A.
6b0d4cf to
5ef2682
Compare
|
Addressed the review findings in
Added regressions for each of those paths; targeted Kernel tests are 23/23. Typecheck, build, Biome, and diff check pass. The full local Runtime Host run passed 1027/1028; the unrelated The PR body now states the trusted in-process/untrusted worker boundary, reserves uncertain-outcome reconciliation for future Host protocol wiring ( |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for splitting this out as its own kernel — the Entry (durable) / Fiber (process-local projection) separation is the right shape, and I want to say up front that the queue serialization, the CAS revision discipline, the cycle rejection, and the depth-ordered publish/retire ordering all hold up under attack. The four inline findings from my earlier pass and me2seeks' two are genuinely fixed at this head, and I did not re-raise them.
Reviewed exact head 5ef2682201141d8282e792d47be30be21eaabf8c.
Two problems keep me from calling this a foundation to build on. First, both P1s below turn extension-controlled input into a kernel that can never start again: a durable diagnostic is written unbounded into a 1 MB-capped snapshot, and one throwing disposer aborts the whole recovery. Both were reproduced, both leave #state at 'stopped' so setEnabled/remove reject with "Extension kernel has not started", and both are one-line fixes. A kernel whose recovery is not fail-safe cannot be the thing that owns durable extension state.
Second, and this is the one I would settle before the follow-on Tool/UI/Hook PRs: the only public design record for this work is #2973, which the PR does not link, and whose central invariant is that each Agent Run uses an immutable composition snapshot that later changes must never mutate. This kernel has no Run dimension — one #fibers map mutated in place — so a reconfigure retires and rebuilds a composition under an in-flight Run. The branch briefly carried a plan doc that explicitly chose to drop the Revision/Binding/candidate model and then deleted it, so the deviation exists only in commit history. Either the invariant comes back or the deviation gets published on #2973; what should not happen is that it lands silently inside a PR titled "composition kernel".
Third, structurally: this is +1834/-0 with zero callers anywhere in the repo, and it adds twenty exports to @maka/runtime-host/server — a barrel whose six existing exports each have a real consumer — while the tests import by relative path. Unreachable public surface is un-versioned and un-reviewed against any consumer. I would land the entry store, the kernel, and the package loader as three PRs and add barrel exports in the PR that wires something to them.
Smaller items I am not filing inline: ExtensionFiber.generation is assigned by the loader and every test fake and read by nobody (it was the import cache-buster before the digest fix); lastDiagnostic.phase is copied unchecked by cloneEntry while message is validated, so a forged phase outside the 'recovery' | 'activation' union reaches the durable file; ExtensionKernelDiagnostic.entryId is required but change-notification passes ''; and inject/isolate/intercept are a four-knob injection algebra with no consumer and two different isolate semantics between inject() and resolveForChild(). Also, no CI checks have run on this head, so the body's "previous PR-head CI passed" is not a signal for it, and the branch is still named after the plan doc that was removed.
Review disclosure: this review was prepared with Claude Code running two independent reviewer passes — one on security and correctness/resource bounds, one on integration and simplification — whose findings I merged and deduplicated against the existing threads. The reproductions noted inline were executed by transpiling the kernel standalone; the file has no runtime imports, so that is faithful to this head. The human contributor reviewed the findings before posting.
| } catch (error) { | ||
| await context.dispose(); | ||
| if (!tolerateActivationFailure) throw new ExtensionActivationError(entry.entryId, error); | ||
| diagnostics.set(entry.entryId, error); |
There was a problem hiding this comment.
[P1] Bound the diagnostic before it becomes durable state. withDiagnostics copies error.message verbatim into the Entry and nothing truncates it, despite the body's "persists bounded diagnostics". Reproduced: a snapshot with one enabled entry whose prepare() throws new Error('x'.repeat(2 << 20)) makes recover() write a snapshot past the 1 MB cap in extension-entry-file-store.ts, which throws Extension Entry Tree snapshot exceeds the size limit, aborts the stage, and leaves #state at 'stopped' — so every later recover() fails identically and mutate/setEnabled/remove all reject with "Extension kernel has not started". The offending entry can then only be disabled by hand-editing the snapshot file. The mutation path escapes this by accident, because ExtensionActivationError re-wraps and the text lands in cause; recovery has no such wrapper. Truncate in withDiagnostics, and drop diagnostics rather than fail the durable write when the snapshot would exceed the cap; add a recovery test with an oversized activation message.
| const fiber = await this.#loader.prepare(entry, context); | ||
| staged.set(entry.entryId, { fingerprint, fiber, context }); | ||
| } catch (error) { | ||
| await context.dispose(); |
There was a problem hiding this comment.
[P1] Route this cleanup through #disposeWithDiagnostic like every other disposal site. KernelExtensionContext.dispose() aggregates disposer errors and rethrows, so this bare await escapes before line 394 ever consults tolerateActivationFailure — the one branch whose entire purpose is to tolerate a failure. Reproduced: an entry whose prepare() registers a throwing effect and then throws makes recover() reject with AggregateError: Extension context ... cleanup failed instead of recording a recovery diagnostic; the kernel never reaches 'running' and setEnabled(id, false) then rejects with "Extension kernel has not started" — the same permanently-unstartable state as the finding above, from a different trigger. The same line also degrades the mutation path: the AggregateError replaces the ExtensionActivationError, so #mutate sees failedEntryId === undefined, persists no diagnostic, and still burns a revision writing an unchanged snapshot.
| readonly #loader: ExtensionFiberLoader; | ||
| readonly #options: ExtensionKernelOptions; | ||
| #entries = new Map<string, ExtensionEntry>(); | ||
| #fibers = new Map<string, ActiveFiber>(); |
There was a problem hiding this comment.
[P1] Decide in public whether a composition can be mutated under an in-flight Run. #2973 — the only public design record for this work, and not linked from this PR — states that each Agent Run uses an immutable Extension Composition snapshot and that changes produce a candidate for future work and must never mutate the composition of an active Run. This map is the whole live composition and #commitStage mutates it in place, so a reconfigure retires and rebuilds fibers underneath a Run that is still using them; grep for Run/Turn/snapshot in this file returns nothing. The branch briefly contained a plan doc that explicitly dropped the Revision/Binding/candidate model and was then deleted from the branch, so today the deviation is recorded only in commit history. Either pin compositions per Run and gate #commitStage on drain, or publish the deviation on #2973 and say how the invariant returns — because the follow-on Tool/UI/Hook PRs are being told to build on this shape.
| for (const active of fibers) await this.#disposeWithDiagnostic(active, 'retirement'); | ||
| } | ||
|
|
||
| #enqueue<T>(operation: () => Promise<T>): Promise<T> { |
There was a problem hiding this comment.
[P2] Give the lifecycle queue a deadline and a cancellation path. Searching these three files for Abort, signal, timeout or setTimeout returns nothing: #stage awaits loader.prepare() unbounded, #commitStage awaits fiber.publish() unbounded, disposers are awaited unbounded, and waitForLock blocks with no deadline — while the repo's other lock helper, storage/src/file-update-lock.ts, does bound its wait. Because this single tail is the kernel's whole lifecycle authority, one badly-behaved package wedges everything behind it. Reproduced: a package whose prepare() returns a never-settling promise makes install() hang and then dispose(), enqueued behind it, never resolve — so any Host shutdown awaiting kernel.dispose() hangs with no way to abandon the in-flight stage. The cross-process variant is worse: another process holding the lock file blocks every mutation and dispose() with no deadline and no diagnostic. Pass an AbortSignal into ExtensionFiberLoader.prepare so cancellation reaches the awaited work, and bound the lock wait.
| ); | ||
| } | ||
|
|
||
| function cloneJsonRecord( |
There was a problem hiding this comment.
[P2] Strip the prototype keys at the boundary that advertises canonical validation. cloneJsonRecord is JSON.parse(JSON.stringify(value)) with no reviver, and assertJsonValue only inspects Object.getPrototypeOf(value), which is Object.prototype for a JSON-parsed object carrying an own __proto__ key. Reproduced: a configuration parsed from {"__proto__":{"polluted":"yes"},"constructor":{"prototype":{...}}} reaches the extension with those own property names intact and round-trips through the durable snapshot; a consumer doing Object.assign(target, configuration) then has target's prototype replaced (plain spread is safe, so the blast radius depends on the consumer). It is P2 only because no consumer exists yet — but this function is precisely the boundary the PR advertises as canonical, and the protocol that eventually feeds it client-supplied configuration is the reason it exists. Reject or drop __proto__ / constructor / prototype keys here.
| this.#path = path; | ||
| } | ||
|
|
||
| async read(): Promise<ExtensionEntrySnapshot> { |
There was a problem hiding this comment.
[P2] Check the size before the bytes are resident, and do not follow a symlink here. read() does await readFile(this.#path) and only then compares against MAX_SNAPSHOT_BYTES, so a 1 GB file is fully buffered before the cap rejects it — and write() calls read() while holding the lock, so the spike blocks every other writer too. The lock path a few lines below is hardened with O_NOFOLLOW and a dev/ino stability check; the snapshot path, which is the one carrying the durable state, gets neither. The repo already ships the correct primitive: readBoundedMarkerFile in packages/storage/src/marker-file.ts opens with O_NOFOLLOW|O_NONBLOCK, fstats for size and identity, and only then reads. Reuse it rather than replicating a weaker version.
| `Extension Entry Tree revision must advance from ${expectedRevision} to ${expectedRevision + 1}`, | ||
| ); | ||
| } | ||
| const temporary = `${this.#path}.${randomUUID()}.tmp`; |
There was a problem hiding this comment.
[P2] Remove the temporary file on every failure path, not just a failed rename. rm(temporary, { force: true }) appears only in the rename catch, so a throw from handle.writeFile or handle.sync — ENOSPC, EIO, EDQUOT — closes the handle in the finally and leaves an orphaned <path>.<uuid>.tmp behind forever, with nothing to sweep it. The realistic case is the worst one: a caller retrying a mutation under a full disk creates one new orphan per attempt, consuming the space it is already short of. publishMarkerFile in the same repo gets this right with a tempCreated flag and an unlink in finally.
| } | ||
| } | ||
|
|
||
| async function withEntryStoreLock<T>(lockPath: string, operation: () => Promise<T>): Promise<T> { |
There was a problem hiding this comment.
[P2] Call the repository's advisory-lock helper instead of forking it. withEntryStoreLock and assertStableLockFile reproduce packages/storage/src/artifact-writer-lock.ts step for step — the same module-level gate map, the same gate-promise pattern, the same open, assert-stable, waitForLock, assert-stable, operate, unlock, close sequence, and the same dev/ino identity check — and the PR also forks storage's fs-native-extensions.d.ts with a narrower waitForLock signature and adds a second direct dependency on the native module. Two copies of the repo's locking protocol will drift, and a fix to one will not reach the other. The branch's own removed plan doc said this layer should reuse the existing storage/transaction boundary. Add the missing exports entry to @maka/storage and call the existing helper; the forked .d.ts and the duplicate dependency then delete themselves.
| export { installRuntimeHostLogCapture } from '../process-diagnostics.js'; | ||
| export { | ||
| ExtensionKernel, | ||
| type ExtensionContext, |
There was a problem hiding this comment.
[P2] Add the barrel exports in the PR that wires a consumer. The twenty names added here have no importer anywhere in apps/, packages/cli, packages/eval, or runtime-host's own composition, while the six exports that were already in this barrel each have a real one — and this PR's tests import by relative path, so removing the whole hunk breaks nothing. Public surface that nothing reaches is un-versioned and cannot be reviewed against a caller's needs, which is precisely when API shape mistakes get locked in.
| const PACKAGE_DIGEST = `sha256:${'a'.repeat(64)}` as const; | ||
| const TEST_AUTHORITY = { assertCurrent() {} }; | ||
|
|
||
| type LegacyFiber = Omit<ExtensionFiber, 'publish'>; |
There was a problem hiding this comment.
[P2] Test the contract this PR ships, not a superseded shape. The LegacyFiber/LegacyLoader shim and the ExtensionKernel subclass convert activate into prepare, inject a no-op publish(), supply the authority, and back-fill packageDigest in both mutate() and MemoryStore.read() — so 20 of the 23 tests never exercise the prepare/publish activation boundary, the mandatory authority, or the packageId/packageDigest durable invariant; the scaffolding supplies all three. Since this PR introduces ExtensionFiberLoader, there is no legacy to stay compatible with: the shim is keeping a pre-hardening API alive so the existing tests did not have to be rewritten, which is the parallel path AGENTS.md asks us not to create. Relatedly, ExtensionKernelAuthority has no production implementer and no test in which assertCurrent() throws, so the single-activation guarantee is currently unverified — deleting the interface and its five call sites leaves all 23 tests green. The repo already has assertInteractiveRootOwner and ArtifactWriterLockAuthority.assertCurrentRoot for this; take one of them, and add a test where the authority rejects.
5ef2682 to
f36e9e6
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at f36e9e62. I isolated your own work from the merged main by comparing every file in the PR blob-by-blob against 5ef26822, then transpiled the kernel, loader, entry store and lock helper at this head and ran them: your 31 tests pass, and I added nine adversarial probes on top.
Disposition of my sixteen standing findings: eleven fixed correctly, four partially fixed, one fixed in a way that moved the problem. The eleven are genuinely closed at the mechanism, not just at my repro:
- The dispose/install race is serialized through one lifecycle queue, and the test holds a disposer open while racing an
install()— that is the right shape. - Diagnostics are bounded at 4 KiB with a non-fatal UTF-8 truncation, and
recover()degrades to dropping diagnostics rather than aborting when the store hits capacity. I ran 400 entries × 8 KiB of error text through it and recovery came back clean. - The bare
awaitnow goes through#disposeContextWithDiagnostic, so the activation error is no longer displaced by a cleanup error. __proto__/constructor/prototypeare rejected by an explicit key check reached from bothassertJsonValueand the inject/intercept clone. Rejecting rather than dropping is the right call.published = truemoved afterpublish()resolves, withabortcalled exactly once and the generation directory cleaned.- The entry store now uses
readBoundedMarkerFile/publishMarkerFile: size checked fromfstatbefore any bytes are read,O_NOFOLLOW, dev/ino identity, directory fsync, and temp cleanup on every failure path. The file went from 152 lines to 93. contentDigestand the unused barrel exports were removed rather than justified — both were the option I would have picked.- The
LegacyFiber/LegacyLoadershim is gone; every fiber now implements the realprepare/publish/dispose, the authority is injected explicitly, and there is a test proving a stale owner is rejected before any fiber is prepared. That last one turns the authority from a parameter into a contract.
The extracted advisory-file-lock.ts is exactly what I asked for — one helper, artifact-writer-lock.ts down from 132 lines to 53, no third variant. Note that artifact-writer-bootstrap-lock.ts is still a near-copy of the same protocol; it predates this PR, so it is follow-up material rather than something to fix here.
Where things stand on the four partial fixes, in order of how much is left:
The immutable-package guarantee did not survive the change of mechanism. Replacing the generation query with a mkdtemp + cp of the whole package tree does fix the multi-file case, and your own test proves it. But Node's cp defaults to dereference: false, so symlinks are copied as symlinks — and a relative intra-package symlink gets rewritten to an absolute path into the original tree. I reproduced both: an absolute link and a purely internal relative link each yield ['first', 'first'] after reload, and printing the link target inside the generation directory shows it pointing back at the mutable source. node_modules workspace links and pnpm store links both have that shape. Inline.
prepare still receives the authoritative Entry object. cloneEntry now guards the getter and the mutation entry points, and your test covers both — but the object handed to #loader.prepare is the live one from nextMap, and #stage runs before the durable write. A package that mutates entry.configuration inside prepare changes both the kernel's projection and the persisted snapshot. Inline.
Deadline and cancellation. The operationTimeoutMs plus AbortSignal threading is real work and I accept the PR body's explicit carve-out for packages that ignore the signal. Two things are outside that carve-out: dispose() can now succeed after its own deadline fires while leaving every effect alive, and effect disposers are never handed a signal at all, so a cooperative package has nothing to cooperate with. Both inline; the first is the one I would fix before merging.
Run-snapshot immutability (#2973). The PR body now states plainly that there is no per-Run immutable Composition Snapshot, which is better than leaving it in commit history. But #2973 is not linked from the PR, its last comment is still from 2026-08-13, and there is nothing on dev@. #commitStage still rewrites the whole live composition in place. I am not filing this inline again — it is a contract decision, and the ask is unchanged: either pin the composition per Run and gate on drain, or record the deviation where #2973 can see it.
The new findings below were not raised by anyone; I checked all twenty inline comments and three issue comments on this PR first. Three are P1, and two of them are new consequences of the lifecycle work rather than pre-existing.
Reviewed with Claude Opus as an analysis assistant. Findings marked "reproduced by execution" were run against transpiled sources at this head in a throwaway worktree that has been removed; the root checkout was not modified and no repository-wide tests were run. Evidence grade is stated per finding.
| ); | ||
| this.#fibers.clear(); | ||
| this.#state = 'stopped'; | ||
| for (const active of fibers) await this.#disposeWithDiagnostic(active, 'retirement', signal); |
There was a problem hiding this comment.
[P1] Do not let dispose() report success after its own deadline has fired. Once #runWithDeadline's timeout trips during this disposer loop, every remaining cooperative fiber.dispose(signal) rejects immediately on the abort, #disposeWithDiagnostic swallows each one indistinguishably from an ordinary disposer failure, and dispose() resolves normally with #state at 'stopped' and no lifecycle-timeout emitted — because #runWithDeadline only fences when the operation throws, and here it does not. Reproduced by execution: three fibers where the first burns a 60 ms budget in a 200 ms disposer and the other two abort cooperatively — dispose() resolved, all three effects were still live, diagnostics showed retirement:a/b/c, and no lifecycle-timeout was emitted. The Host's shutdown path awaits this and takes a clean result as permission to exit while sockets, child processes and global registrations survive; and because the state is 'stopped', a subsequent recover() activates them a second time. Regression test: with operationTimeoutMs below the total cleanup cost, assert dispose() rejects — or at minimum emits lifecycle-timeout and leaves the kernel unrecoverable — and that the un-disposed entry ids are enumerable by the caller.
There was a problem hiding this comment.
Fixed in 1c6fdc3. Disposal now exhausts cleanup, rejects after its deadline, retains unresolved live Entry ids, emits the lifecycle-timeout fence, and refuses a second in-process cleanup attempt when the outcome is uncertain. Added a deadline regression.
| } catch (error) { | ||
| await this.#disposeContextWithDiagnostic(context, entry.entryId); | ||
| if (signal.aborted) throw signal.reason; | ||
| if (!tolerateActivationFailure) throw new ExtensionActivationError(entry.entryId, error); |
There was a problem hiding this comment.
[P1] Let a user disable or remove an Entry that cannot activate. Recovery quarantines a failing Entry and records a diagnostic, but every later mutation re-prepares every enabled Entry that has no fiber — the reuse test at :429 keys on current?.fingerprint, and a quarantined Entry has no current — so the first failure throws here and rolls the whole mutation back. With two or more broken Entries, nothing can be committed at all, including the mutation that would disable one of them. Reproduced by execution: with bad1, bad2 and good, recovery leaves active = ['good'] with both diagnostics recorded, and then setEnabled('bad1', false) is rejected by bad2, remove('bad1') is rejected by bad2, and reconfigure('good', …) is rejected by bad1; all three entries stay enabled: true while the revision climbs from 1 to 5, since each failed attempt still burns a durable revision writing diagnostics. The user's only escape is hand-editing the snapshot file — the exact terminal state the bounded-diagnostics fix was meant to remove, reached through a different door. Either carry tolerateActivationFailure into mutations for Entries that are already quarantined, or scope re-preparation to the Entries the mutation actually touches. Regression test: two independently failing Entries plus a healthy one; assert setEnabled(bad1,false) and remove(bad1) succeed, good's fiber identity is unchanged, and a failed stage does not advance the revision.
There was a problem hiding this comment.
Fixed in 1c6fdc3. Unchanged Entries with an existing activation diagnostic remain quarantined during ordinary mutations and are retried only when changed or explicitly reloaded/recovered. Added bad1/bad2/good coverage for disable, remove, and healthy reconfigure.
| operation: () => Promise<T>, | ||
| ): Promise<T> { | ||
| return withArtifactWriterLockPath( | ||
| return withAdvisoryFileLock( |
There was a problem hiding this comment.
[P1] Do not give existing artifact-writer callers a 30-second budget they never asked for. This call passes no options, so withAdvisoryFileLock applies its timeoutMs ?? 30_000 default to both the queue wait and the OS lock wait — where withArtifactWriterLockPath previously awaited previous and waitForLock(fd) unbounded. The extraction is otherwise faithful: the gate map and the dev/ino identity check are preserved verbatim, and the only other difference is error wording that nothing asserts on. But createOperationalStateBackup holds this lock across a SQLite backup plus copyRegularTree over the whole artifact tree, and git-workspace-service.ts holds it across eight clone/worktree operations — all of which can exceed 30 seconds on a large workspace, and all of which would now hard-fail with Timed out queuing … where they used to queue. Mutual exclusion itself is intact: I confirmed by execution that a timed-out waiter deletes its gate entry and later callers are still excluded by the OFD/flock lock, with no overlap. What changed is waiting versus failing, and FIFO fairness. The helper-level behaviour is reproduced by execution; that these callers take the default is confirmed by reading this line. Give AdvisoryFileLockOptions an explicit unbounded mode and use it here — note that passing Infinity will not work, since Node degrades setTimeout(fn, Infinity) to 1 ms. Regression test: in artifact-writer-lock.test.ts, hold the lock past the default budget and assert a queued withArtifactWriterLock still succeeds.
There was a problem hiding this comment.
Fixed in 1c6fdc3. The shared lock now supports an explicit null unbounded mode, and Artifact writer calls it explicitly; the Entry Store retains its bounded timeout.
| let generationOwned = true; | ||
| try { | ||
| signal.throwIfAborted(); | ||
| await cp(packageRoot, generationPackageRoot, { recursive: true, force: false }); |
There was a problem hiding this comment.
[P2] Dereference symlinks when materializing the generation copy, or reject links that escape it. Copying the whole package tree fixes the multi-file reload case your test covers, but cp defaults to dereference: false and verbatimSymlinks: false, so a symlink is copied as a symlink — and a relative one is rewritten to an absolute path pointing back into the original tree. The generation directory is then not a snapshot, and import reads live bytes from a mutable source, contradicting this file's own note that Node cannot mix old and new ESM modules. Reproduced by execution twice: with pkg/dep.mjs as an absolute symlink, and with a purely intra-package relative link pkg/src/dep.mjs -> ../impl/dep.mjs, editing the target and calling reload both returned ['first','first'] — and printing the copied link's target showed it resolving outside the generation root. node_modules workspace links and pnpm store links both have exactly this shape. Pass { dereference: true }, or after copying reject any entry whose realpath leaves generationRoot. Separately worth a note: this now copies the entire tree on every prepare, so a package carrying node_modules pays a full tree copy per reconfigure. Regression test: a package whose dependency is reached through a symlink; assert reload observes the new bytes and that no path inside the generation root resolves outside it.
There was a problem hiding this comment.
Fixed in 1c6fdc3. Generation copies now dereference links so imported module bytes are materialized inside the ephemeral generation. Added an intra-package relative-symlink reload regression.
| throw new Error(`Missing Extension dependencies: ${missing.join(', ')}`); | ||
| } | ||
| signal.throwIfAborted(); | ||
| const fiber = await this.#loader.prepare(entry, context, signal); |
There was a problem hiding this comment.
[P2] Hand prepare a clone, not the authoritative Entry. cloneEntry now protects the entries getter and the mutation entry points — your test covers mutating both the caller's input and the projection — but the object passed here is the live one from nextMap, and #stage runs before the durable write. A package that assigns to entry.configuration inside prepare therefore rewrites the kernel's own state and the persisted snapshot. Reproduced by execution: a package setting entry.configuration.mode = 'package-mutated' during prepare left both kernel.entries[0].configuration and the durable snapshot carrying that value. This is the last remaining outlet for the finding I raised earlier, and it is the one that reaches persistence. Clone here as well. Regression test: a loader that mutates its entry argument; assert neither the projection nor the durable snapshot changes.
There was a problem hiding this comment.
Fixed in 1c6fdc3. The loader receives cloneEntry(entry), and a regression verifies package mutation cannot change either the Kernel projection or durable snapshot.
| ); | ||
| } catch (error) { | ||
| await this.#abortStage(staged, signal); | ||
| this.#state = 'fenced'; |
There was a problem hiding this comment.
[P2] Do not fence the kernel on a deterministic capacity failure, and do not describe it as an authority conflict. FileExtensionEntryStore.write throws ExtensionEntryStoreCapacityError from a Buffer.byteLength check before it takes the lock, so neither durable nor in-memory state has moved — but this catch does not distinguish error kinds (unlike the diagnostic path a few lines above, which already special-cases capacity), so the kernel goes to 'fenced' and every later setEnabled/remove returns Extension kernel is fenced after a durable authority conflict; restart required. There is no authority conflict, and that message sends the next reader after the wrong thing. Configuration comes from the caller with no size limit at any entry point, so a single reconfigure(id, { blob: <2 MB> }) reaches it. Reproduced by execution: after that call the kernel refuses all subsequent mutations, while activeEntryIds correctly still shows the old fiber and the candidate was aborted. Treat capacity like the diagnostic path does — reject the mutation and stay running. Regression test: an oversized reconfigure rejects, the kernel remains running, the fiber identity is unchanged, and a following setEnabled(id, false) succeeds.
There was a problem hiding this comment.
Fixed in 1c6fdc3. ExtensionEntryStoreCapacityError now aborts the candidate and rejects without fencing; the old Fiber and running Kernel remain usable. Added an oversized reconfigure regression followed by a successful disable.
| readonly parent?: ExtensionContext; | ||
| provide<T>(key: string, value: T): void; | ||
| inject<T>(key: string): T | undefined; | ||
| registerEffect(dispose: () => void | Promise<void>): void; |
There was a problem hiding this comment.
[P2] Give effect disposers the AbortSignal that fiber.dispose already receives. disposeActiveFiber calls active.context.dispose() with no argument, and that awaits each registered effect disposer unbounded, so a single effect whose cleanup never settles makes dispose() hang forever and operationTimeoutMs has no effect. This is distinct from a package ignoring its signal, which the PR body explicitly declines to solve: here there is no signal for a well-behaved package to honour, so the carve-out does not cover it. Reproduced by execution: registerEffect(() => new Promise(() => {})) with operationTimeoutMs: 50 left dispose() still pending after 400 ms. Extend the signature to accept an AbortSignal, or bound the context-cleanup loop in the kernel. Regression test: a non-settling effect disposer; assert dispose() rejects within the deadline and the offending entry id is enumerable.
There was a problem hiding this comment.
Fixed in 1c6fdc3. Context effect disposers now receive the lifecycle AbortSignal through registerEffect and context disposal. Added a non-settling cooperative effect regression.
| @@ -0,0 +1,115 @@ | |||
| /// <reference path="./fs-native-extensions.d.ts" /> | |||
There was a problem hiding this comment.
[P2] Test the shared primitive directly. This file is now the single lock authority for two unrelated subsystems — the artifact writer and the Extension Entry Tree — and there is no advisory-file-lock test anywhere under packages/storage/src/__tests__/; grepping the repository for Timed out locking and Timed out queuing returns nothing. Coverage is entirely indirect through artifact-writer-lock.test.ts, whose hold times are far below the 30-second default and therefore never reach the timeout branch that the previous finding is about. Three behaviours introduced here are unverified: the timeoutMs semantics, deleting the gate entry on timeout, and closing without unlocking when acquired === false. Confirmed by reading the file list and by grep at this head. The whole point of consolidating the two forks was to stop them drifting; an untested single authority reintroduces that risk in a different form. Add packages/storage/src/__tests__/advisory-file-lock.test.ts covering mutual exclusion, both timeout messages, gate state after a timeout, and assertStableRegularFile rejecting a non-regular file and a symlink.
There was a problem hiding this comment.
Fixed in 1c6fdc3. Added direct advisory-file-lock tests for gate/OS timeout, post-timeout reuse, explicit unbounded waiting, mutual exclusion, symlink rejection, and non-regular paths. Bounded OS waits now use cancellable tryLock polling rather than leaving a native waiter behind after Promise.race.
| await context.dispose(); | ||
| } catch (error) { | ||
| try { | ||
| this.#options.onDiagnostic?.({ phase: 'recovery-cleanup', entryId, error }); |
There was a problem hiding this comment.
[P3] Report the phase this cleanup actually ran in. phase: 'recovery-cleanup' is hardcoded, but the sole call site at :450 serves both recovery (tolerateActivationFailure = true) and mutation (false), so a consumer cannot tell "a broken entry was quarantined during recovery" from "a user's mutation failed and its candidate could not be cleaned up" — operationally very different events. Confirmed by reading code at this head. Pass the phase in from the call site. Regression test: a mutation whose prepare fails and whose effect disposer also throws; assert the diagnostic phase is candidate-cleanup rather than recovery-cleanup.
There was a problem hiding this comment.
Fixed in 1c6fdc3. Context cleanup receives the phase from its caller, so mutation failures report candidate-cleanup and recovery isolation reports recovery-cleanup. Added the mutation regression.
| : { configuration: cloneJsonRecord(entry.configuration) }), | ||
| ...(entry.dependencies === undefined | ||
| ? {} | ||
| : { dependencies: entry.dependencies.map((value) => requireString(value, 'dependency')) }), |
There was a problem hiding this comment.
[P3] Validate dependencies and isolate like the neighbouring fields. cloneEntry gives entryId, scope, enabled, configuration and lastDiagnostic.phase explicit checks with readable errors, but these two are only tested for undefined before .map — and isSnapshot in the entry store checks three fields, so a malformed or cross-version snapshot reaches here. Reproduced by execution: persisting dependencies: { x: 1 } makes recover() fail with TypeError: entry.dependencies.map is not a function, which is both less diagnosable and stylistically inconsistent with every other field. Assert they are arrays. Regression test: extend recovery validates persisted diagnostic phases with a non-array dependencies and isolate, asserting an error matching /must be an array/.
There was a problem hiding this comment.
Fixed in 1c6fdc3. dependencies/isolate now require arrays; the same boundary audit also requires inject/intercept and lastDiagnostic to be plain records. Added malformed persisted-shape regressions.
87176a8 to
1c6fdc3
Compare
|
I think K0 is exactly the right layer to pin down two architectural contracts that the implementation currently answers mostly by mechanism rather than by an explicit statement. 1. What does a committed Entry mean?The current ordering is Is the intended contract therefore:
If yes, I think this should be stated as a K0 invariant. It explains the ordering and the absence of Last-good cleanly, but it also means future callers need an explicit way to observe desired-vs-actual divergence. If that is not the contract, where does the authoritative last successfully activated state live after a post-CAS publish failure? 2. What semantic namespace does the Context algebra expose?
Concretely:
The Occam question is not whether K0 should avoid defining a framework—it should define one. It is whether these four string operations are the minimal stable composition algebra, or whether they prematurely encode a general service locator before the capability identity contract is known. Would K0 be safer with opaque, namespaced capability identities first, adding alias/intercept semantics only when a concrete composition case requires them? AI-assisted review disclosure: this architectural question was drafted with OpenAI Codex at the reviewer's request. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The engineering here is careful and the tests are real — 44 cases driving actual module bytes and actual files, not fakes. The Fiber/Context projection, the child-first exhaustive cleanup, and the generation directory in the loader have no precedent in this tree and I found nothing to attack in their shape. What follows is not a quality objection.
The architectural question has to be settled first, and it is not a code-review question. At this head none of the 1238 production lines is reachable from a running Host: packages/runtime-host/src/server/index.ts is unmodified and contains no reference to the kernel, and the only importers of extension-kernel.js are the two other new files and the test. ExtensionKernel also carries its own lifecycle (recover/restart/dispose) and its own 'stopped' | 'running' | 'fenced' state machine next to RuntimeHostKernel, without implementing RuntimeHostDomainModule — so Host shutdown will not dispose Extension fibers and Host recovery will not recover the Entry Tree. AGENTS.md puts lifecycle under Runtime Host's sole authority and asks contributors to extend the closest existing seam; the closest seam here is createRuntimeHostDomainModule in host-composition.ts, registered in execution-composition.ts. Either register it in this PR, or land the kernel with its first consumer. A kernel nothing calls cannot be regression-tested for the property that matters most.
Related and worth clearing up rather than escalating: you also have #3003 open as a draft (last updated 17 Aug) adding extension-controller.ts / extension-loader.ts / extension-state-store.ts to the same directory with a Revision/Binding/Candidate model that this PR's description explicitly repudiates, and #2979 was closed "in favor of the newer extension composition/kernel PRs" without naming a successor. Since all three are yours this is bookkeeping, not a governance dispute — but a reviewer landing on this directory cannot currently tell which model is live. Closing or rescoping #3003, and recording the object-model decision on #2973, would fix that.
One intent, or two? The only reachable behaviour change in this PR is the advisory-lock extraction: artifact-writer-lock.ts now delegates to the new advisory-file-lock.ts. That is independently buildable, testable and valuable, and I found no regression in it — timeoutMs: null preserves the unbounded waitForLock contract, the in-process gate and the double stable-regular-file check carry over intact, and the O_NOFOLLOW plus explicit chmod(0o600) hardening is a genuine improvement. It is also a different revert from the kernel. Splitting it out would land ~290 reviewable lines today and leave what remains honestly described as one dead-code intent.
The findings below are the ones I verified myself against the code at this head. Four are blocking as filed; the deadline and onChanged defects are the ones I would fix regardless of how the wiring question is answered, because they are in the kernel's core control flow rather than in the parts K5+ will rewrite.
AI disclosure: this review was produced with Claude Code (Opus 5), with four subagents covering security, correctness/resource bounds, integration and simplification in parallel. Every finding published here I re-derived myself by reading extension-kernel.ts, extension-entry-file-store.ts, server/index.ts, the storage export map and the PR's file list at e581f0b; I dropped several subagent findings that I could not confirm independently, and corrected their framing of the #3003 relationship. The subagents additionally report reproducing several of these by compiling and driving the kernel standalone; I did not re-run those, so treat the specific transcripts as their evidence rather than mine. Per AGENTS.md this is not independent human review.
| } | ||
| } catch (error) { | ||
| const currentEntries = new Set(this.#liveEntries.values()); | ||
| const candidates = [...staged.values()].filter((candidate) => !currentEntries.has(candidate)); |
There was a problem hiding this comment.
[P1] Give cleanup its own deadline instead of the signal that just expired. #runWithDeadline creates one AbortController and threads its signal everywhere, so when the deadline fires, every cleanup path receives an already-aborted signal — this line, #disposeContextWithDiagnostic at 513, the retirement loop at 575, and #dispose at 390. Any disposer that honours cancellation therefore rejects by construction, and honouring it is exactly what this kernel's own API teaches: registerEffect(dispose: (signal: AbortSignal) => …) hands the disposer a signal, and the test at extension-kernel.test.ts:297 asserts disposers receive it. #disposeWithDiagnostic maps that rejection to #cleanupUncertain = true, and grepping every reference to that flag shows nothing ever clears it: recover throws at 163, mutate at 279, dispose and restart at 379. So one slow extension plus one well-behaved disposer escalates to "the host process must be restarted", with the live fibers left non-terminal and no handle to reach them. The perverse corollary is that a disposer which ignores the signal recovers cleanly — the kernel punishes the cooperative implementation. Confirmed by reading the control flow at this head; a subagent reports reproducing the three-way install/dispose/restart rejection with operationTimeoutMs: 20. Fix: a fresh controller (or AbortSignal.timeout) for the teardown phase. Regression test: a fiber whose effect disposer calls signal.throwIfAborted(), force a mutation timeout, then assert kernel.dispose() resolves and activeEntryIds is empty.
| controller.abort(new Error(`Extension ${label} exceeded ${timeoutMs} ms`)); | ||
| }, timeoutMs); | ||
| try { | ||
| return await operation(controller.signal); |
There was a problem hiding this comment.
[P1] Race the operation against the deadline — as written the timeout cannot fire on the case it exists for. setTimeout calls controller.abort(...), and then this line plainly awaits the operation. The lifecycle-timeout diagnostic and the #state = 'fenced' transition live only in the catch below, so both require the operation to actually reject. An operation that never settles — a forgotten await, a fetch with no abort wiring, a disposer returning a promise nobody resolves — produces no rejection, so the deadline is a no-op: no diagnostic, no fence, #state stays 'running', and #enqueue above chains #operationTail onto that same never-settling promise, so every later mutate, remove, restart and dispose queues behind it forever. Host shutdown hangs too. The kernel's most carefully built safety property — bounded lifecycle operations with a visible fence — is unreachable precisely when an extension misbehaves in the most ordinary way. Confirmed by reading this function at this head; a subagent reports reproducing the permanent wedge with a fiber whose dispose() returns new Promise(() => {}). Regression test: that fiber with operationTimeoutMs: 10, assert remove() rejects with the timeout, assert a lifecycle-timeout diagnostic was emitted, and assert a subsequently queued setEnabled also rejects rather than hanging.
| this.#entries = nextMap; | ||
| this.#revision = nextRevision; | ||
| try { | ||
| await this.#options.onChanged?.({ |
There was a problem hiding this comment.
[P1] Dispatch onChanged after the operation leaves the queue, and give it the lifecycle signal. This await runs inside #mutate, which runs inside the single #operationTail slot, and onChanged's signature is (snapshot) => void | Promise<void> — no signal. Two consequences follow directly. First, reentrancy deadlocks: a handler that calls any public kernel method enqueues behind the operation currently awaiting it, and hangs permanently with no diagnostic and no timeout rescue. That is the natural thing for a host to write — a UI mirror reacting to a tree change — and nothing in the type or the doc comment warns against it. Second, because the handler is never given the signal, it cannot cooperate with the deadline even in principle; a handler that never settles wedges the tail forever, which is the same failure as the finding at line 427 but reachable from embedder code rather than extension code. Confirmed by reading #mutate and #enqueue at this head; a subagent reports reproducing both the reentrancy hang and the past-deadline wedge. Regression test: an onChanged that calls kernel.setEnabled — assert the outer install() settles; and one returning a never-settling promise — assert a subsequent dispose() rejects within the deadline.
| }); | ||
|
|
||
| test('advisory lock rejects a symlink lock path', { | ||
| skip: process.platform === 'win32', |
There was a problem hiding this comment.
[P1] Regenerate the Windows skip inventory — CI fails on this as filed. .github/workflows/ci.yml runs npm run windows:inventory, whose --check half re-derives docs/windows-test-inventory.md from every skip: expression that excludes Windows and compares it to the committed file. This PR adds two such declarations — this line and packages/runtime-host/src/__tests__/extension-kernel.test.ts:820 — and does not touch the doc, which is not in the PR's file list at all. npm run windows:inventory:write and commit the result. Confirmed from the PR's file list and the two added skip: lines at this head; a subagent reports running the checker against a throwaway worktree and getting Windows test skip inventory is stale; run npm run windows:inventory:write with exit 1, and a rendered delta of 62 to 64 total rows. Purely mechanical, but it is a required check, so it blocks. The regression test is the check itself.
| export interface ExtensionContext { | ||
| readonly entryId: string; | ||
| readonly scope: ExtensionRootScope; | ||
| readonly parent?: ExtensionContext; |
There was a problem hiding this comment.
[P2] Do not expose parent on the public ExtensionContext. Isolation is implemented entirely in inject and resolveForChild — isolate cuts the upward chain and intercept remaps keys as a lookup descends — but all of it is enforced only along the path that starts at this. Because parent is public and provide/inject/registerEffect are public on it, a fiber reaches its ancestors directly and every declared boundary is skipped: context.parent.parent.inject(k) returns what context.inject(k) was declared to hide. resolveForChild being private does not help; that is a TypeScript annotation, and parent.inject() is the public door. Two further consequences follow from the same field: #validate permits one profile parent to own children in different sessions, so parent.provide(k, v) from one session is resolvable by another through the sanctioned path; and parent.registerEffect(fn) survives remove() of the registering entry, because removal disposes only that entry's own context — so a removed extension still runs teardown at its ancestor's, and since KernelExtensionContext.dispose aggregates effect errors, it can fence the kernel after it is durably gone. Confirmed structurally by reading the interface and the resolution methods at this head; a subagent reports reproducing all three against the PR's own isolate test fixture. The kernel already passes parent contexts by constructor and does not need the public field; alternatively hand fibers a per-entry facade exposing only provide/inject/registerEffect. Regression test: extend the existing isolation test to assert context.parent?.parent?.inject('visible') is also undefined.
| } | ||
| } | ||
|
|
||
| function isSnapshot(value: unknown): value is ExtensionEntrySnapshot { |
There was a problem hiding this comment.
[P2] Version this format and decide its unknown-key policy before anything writes it. isSnapshot checks only that revision is a safe non-negative integer and that each entry has a string entryId, a string scope and a boolean enabled; revision is a CAS counter, not a format discriminator, and there is no schemaVersion. Two gaps follow. Unknown keys are dropped rather than rejected, because cloneEntry builds a whitelist object literal — so an older build round-tripping a snapshot written by a newer one silently discards fields, which is what two Host builds sharing a data root, or a downgrade after an upgrade, will do to each other. And an entry this build cannot interpret fails the whole recovery rather than being quarantined: #recover validates the entire map before any per-entry staging, so one unrecognized scope leaves every healthy sibling inactive and #state at 'stopped', which makes mutate reject — the user cannot even remove the offending entry, and every restart repeats it. That directly contradicts the quarantine property the PR claims, which today covers activation failures only. Worth contrasting with the protocol layer, which gets this right: requireExactRecord rejects unknown keys loudly and RUNTIME_HOST_COMPATIBILITY_EPOCH fences mixed-version peers. This is cheap now, while nothing in production writes the format, and expensive later. Confirmed by reading isSnapshot, cloneEntry and #recover at this head. Regression tests: recover from a snapshot holding one valid entry plus one with an unrecognized scope, and assert the valid one activates and the invalid one can then be removed; and round-trip a snapshot carrying an unknown key, asserting it is preserved or loudly rejected, never silently dropped.
| ".": "./dist/index.js", | ||
| "./credential-store": "./dist/credential-store.js", | ||
| "./file-update-lock": "./dist/file-update-lock.js", | ||
| "./advisory-file-lock": "./dist/advisory-file-lock.js", |
There was a problem hiding this comment.
[P2] Coordinate these two export keys with #3301, which rewrites this whole block and contains neither. extension-entry-file-store.ts imports @maka/storage/advisory-file-lock and @maka/storage/marker-file, so both keys are load-bearing for this PR. Open PR #3301 ("refactor(storage): drop the barrel and publish narrow entrypoints") replaces the entire exports map with a regenerated list; grepping its added lines for advisory-file-lock or marker-file returns nothing, which is expected since it was cut before this branch existed. Whichever lands second must re-add both by hand, and a rebase that takes #3301's block wholesale breaks the runtime-host build with ERR_PACKAGE_PATH_NOT_EXPORTED. Worth a note on both PRs so it is caught at rebase time rather than by a red CI run. Confirmed by reading both PRs' diffs at their current heads. The export map is otherwise correct here: marker-file.ts already exists on main with the signatures the call sites use, and fs-native-extensions.d.ts already declares tryLock, so no typings change is owed.
9e98d92 to
1300bbe
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
COMMENT. All four findings from my previous review are still present, unchanged, and the new commit adds roughly 2050 lines that bring three more P1s with them.
Head note. gh pr view reports 1300bbe36, not the 9e98d92f8 I was pointed at. Both have tree 49efaf444 and parent e581f0bc3, so they are the same content and everything below is against the current head.
The four prior findings — all STILL PRESENT. I am not re-posting them; the existing threads at :442, :578 and :377 are still the right place, and none has a "fixed in" reply.
#runWithDeadline:442is stillreturn await operation(controller.signal);with noPromise.race—grep -c "Promise.race"over the file returns 0. The PR body's defence ("hard termination requires a worker or process boundary and is intentionally not claimed here") does not reach this: the problem is not that the operation keeps running, it is that the returned promise never settles, so the mutation never rejects and#operationTailnever drains. That needs no termination primitive to fix.await this.#options.onChanged?.(...)is still awaited inside the queued mutation at:376-386. Thetry/catchthere isolates a rejecting observer, which was already true at the previous head; a hanging one still stalls every later mutation.- Cleanup still receives the already-aborted signal —
:368,:405,:582,:602,:619,:631all forward the samesignal, anddisposeLiveEntry:811-820puts it onpackageState.signal, which is whatregisterEffect's disposer gets. A disposer that honours the signal as the contract asks it to can do no async cleanup at all after a timeout. #cleanupUncertainis still never cleared. The body now states the fence as intent and I accept that as a contract decision — but the consequence forrestart()anddispose()is new and concrete, so I have filed that part inline as a P2.
On the new commit. refactor(runtime): restore full plugin kernel foundation adds a 948-line DI/Fiber framework, a 761-line composition loader, and a 345-line runtime surface, inside a PR titled feat(runtime-host): add Entry/Fiber composition kernel (K0). Two things about that, before the individual findings:
plugin-composition-loader.ts and plugin-runtime.ts are a second composition authority with no production consumer. MakaCompositionLoader re-implements what ExtensionKernel already owns, against the same concepts: the same three root scopes (plugin-runtime.ts:5 vs extension-kernel.ts:8), an entry tree with inject/isolate/intercept, stage-commit-dispose, and an interned isolation-label map (plugin-composition-loader.ts:663-670 vs extension-kernel.ts:667-674, the same routine). AGENTS.md asks for the closest existing seam rather than a parallel path, and Runtime Host is named as the sole authority for lifecycle. It also cuts against the PR's own architecture statement, which says the design has "no per-Run immutable Composition Snapshot" — plugin-runtime.ts:30-38 and :116-121 define exactly that, with a schemaVersion and a sha256: digest.
And on revert granularity: plugin-kernel.ts has a consumer and is arguably in scope; the loader has none. One revert should not have to undo both. I would drop the loader from this PR until the change that wires it, or split it out.
Eight findings inline — three P1 and five P2, all in the new code except the restart/dispose one. I have deliberately not filed several smaller things: void setup; is dead at plugin-kernel.ts:708; FIBER_PENDING/FIBER_FAILED at plugin-composition-loader.ts:30-31 are hand-copied numeric duplicates of FiberState that a reordering would silently break, as is fiberStateName's positional array at plugin-runtime.ts:317-321; isConstructor is /^class\s/ on Function.prototype.toString in both files and is wrong for anything transpiled; awaitSettled():257-265 is an unbounded while (true) with no deadline; and the loader has no AbortSignal anywhere at all, so a hanging apply blocks #mutation permanently with none of the (broken, but present) deadline machinery extension-kernel has.
Security: clean. No import(, eval(, require(, new Function, createRequire, pathToFileURL, readFile or spawn in any of the three new modules. MakaCompositionLoader.install() takes an already-constructed package object — no path, URL, or manifest resolution, and no filesystem access. The only dynamic import() is still extension-package-loader.ts:93, unchanged here and already covered by the open threads at :54 and :84. Nothing puts secrets in logs.
Test coverage. 13 tests and roughly 350 lines against roughly 2050 lines of new code, and the gap is systematic rather than incidental: not one test in either file asserts what happens when a disposer throws, when cleanup fails, or when re-activation fails during move or replaceSubtree — which is where three of the findings below live. MakaCompositionLoader.move, uninstall, replaceSubtree, awaitSettled, enable/disable, Context.mixin/accessor/once/set, Fiber.restart, the Service base class and MakaPluginTransactionBuffer.commit's failure path are all untested. Two assertions are shape-level rather than behavioural: plugin-composition-loader.test.ts:167 asserts generation >= 41, which holds for anything the code could produce, and :186-189 asserts the returned inspection array rather than the resulting composition state.
biome format and biome lint are clean on all five new and changed TS files.
AI disclosure: this review was assisted by Claude (Opus), which performed the initial code search and cross-checking and executed parts of the new kernel in isolation. Every finding published here I re-derived myself by reading the source at 1300bbe36; where I could only confirm the structure rather than witness the failure, the finding says "confirmed by reading" rather than claiming a reproduction.
| get<T = unknown>(name: string, strict = true): T | undefined { | ||
| const implementation = this.#implementation(name); | ||
| if (!implementation) return undefined; | ||
| if (strict && implementation.fiber.state !== FiberState.ACTIVE) return undefined; |
There was a problem hiding this comment.
[P1] A service provided by one plugin never reaches a plugin that injects it — the kernel's central capability does not work. Context#provide runs its registration effect while the providing Fiber is still LOADING. #notifyService (:488-496) calls each dependent's refreshDependencies(), which resolves through Context#get, and this line rejects any implementation whose owner Fiber is not ACTIVE, so the dependent stays PENDING. When the provider later reaches ACTIVE at :782, #setState only emits internal/status — nothing re-runs #notifyService and nothing listens for that transition, so the dependent is never revisited. Provision from the root Context works, because the root Fiber is ACTIVE from construction — and that is the only shape either new test exercises (plugin-kernel.test.ts:26 and plugin-composition-loader.test.ts:53 both provide from the root), which is why the gap is invisible in CI. Confirmed by reading the source at this head. Regression test needed: plugin A provides a service, plugin B injects it, assert B activates and its body runs.
| }; | ||
| fiber = context.plugin(plugin, entry.configuration ?? {}); | ||
| await fiber.await(); | ||
| staged.set(entry.entryId, { |
There was a problem hiding this comment.
[P1] An Entry whose Fiber never activated is durably committed, with no error and no diagnostic. #stage stores the staged entry immediately after await fiber.await() without checking fiber.state. A Fiber left PENDING — for instance because it injects a service no one provided, which today includes every plugin-provided service (see my note on plugin-kernel.ts:300) — never ran apply, so packageState.prepared stays undefined, and #commitStage:613 gates the publish on active.packageState.prepared && !active.packageState.published and therefore skips it silently. The mutation resolves, the Entry appears in durable state, the loader's prepare was never called, activeEntryIds stays empty, and no diagnostic is emitted. From the user's side the extension is installed, does nothing, and nothing anywhere says why. This is separately fixable from the service-resolution bug — the kernel already knows the Fiber was not published and simply does not report it. Confirmed by reading the source at this head. Regression test needed: install an Entry whose inject names an unprovided service; assert the mutation rejects or records a diagnostic rather than committing silently.
| #enqueue(operation: () => Promise<void>): Promise<void> { | ||
| const task = this.#transition.then(operation, operation); | ||
| this.#transition = task.catch(() => undefined); | ||
| const settled = task.finally(() => { |
There was a problem hiding this comment.
[P1] #enqueue returns a promise nobody has attached a handler to, so any fire-and-forget Fiber load failure becomes an unhandled rejection. this.#transition = task.catch(() => undefined) attaches a handler to task, but settled = task.finally(...) is a new promise that rejects independently, and storing it in this.inertia does not mark it handled. refreshDependencies discards the return value at :634, :650 and :652. Under Node's default --unhandled-rejections=throw that terminates the process. Two reachable shapes: the ordinary ctx.plugin(x) idiom, where a sub-plugin's apply throws; and the deferred reload path, where a later provide triggers #notifyService -> #enqueue(#load) on a plugin that throws. Note the asymmetry that makes this look like an oversight rather than a design: the unprovide path at :290 wraps fiber.await() in Promise.allSettled, while the provide path at :286 discards the same value. extension-kernel always awaits fiber.await() so it does not reach this today, which is exactly why it will not be caught until the first consumer registers a sub-plugin. Confirmed by reading the source at this head. Regression test needed: a fire-and-forget ctx.plugin whose activation throws, asserting no unhandledRejection fires.
| if (this.#disposed) return this.inertia; | ||
| this.#disposed = true; | ||
| await this.#enqueue(async () => { | ||
| await this.#unload(FiberState.DISPOSED); |
There was a problem hiding this comment.
[P2] A failed Fiber.dispose() leaks the Fiber permanently, and every retry then reports success. #unload throws its FiberCleanupError at :812, before #setState(nextState) at :813 — so this await rejects and the three lines of bookkeeping below it never run: the Fiber is never removed from kernel.fibers, runtime.fibers, or its parent's #children. It stays registered with state = UNLOADING and #disposed = true. The retry is worse than the leak: dispose():744 short-circuits on #disposed and returns this.inertia, which #enqueue's finally has already reset to undefined, so the second call resolves immediately with no error while the effect is still live. And because the Fiber was never removed from #children, any ancestor's later #unload maps over it, gets that immediately-resolved promise, and concludes the whole subtree disposed cleanly. ExtensionKernel happens not to mis-report because #disposeWithDiagnostic:644-655 catches the first failure — but the leak is unbounded and the false success is a trap for any other consumer of this kernel. Confirmed by reading the source at this head. Regression test needed: a failing disposer; assert dispose() still rejects on retry and that the Fiber has left kernelFibers().
|
|
||
| async #dispose(signal: AbortSignal): Promise<void> { | ||
| if (this.#cleanupUncertain) { | ||
| throw new Error('Extension cleanup outcome is uncertain; process restart required'); |
There was a problem hiding this comment.
[P2] Once cleanup is uncertain the kernel tells the caller to restart, and then refuses to let them. #cleanupUncertain is initialised false at :137, set at :410, :554, :636, :651 and :665, and never cleared anywhere. The PR body now presents the fence as intent, and I accept that as a contract decision — but the public surface contradicts it. restart() (:211-217) routes through #dispose, which throws right here, so restart() can never run; dispose() (:390) throws here too, so the host cannot even release the instance in order to construct a replacement. A single transient disposer failure — one that would succeed on the next attempt — therefore permanently bricks the kernel object, and the error message names a recovery the class cannot perform. Either make restart() bypass the fence (it is the one operation whose whole purpose is to leave the uncertain state), or remove restart() and dispose() from the surface and document that the host must drop the reference. Confirmed by reading the source at this head. Regression test needed: one failing disposal, then assert restart() recovers, or that neither restart() nor dispose() is exposed.
| } | ||
|
|
||
| async #dispose(entry: LiveEntry): Promise<void> { | ||
| await Promise.allSettled([...entry.children].reverse().map((child) => this.#dispose(child))); |
There was a problem hiding this comment.
[P2] Every cleanup failure in this loader is discarded, so remove() and close() report success while effects stay live. This is one of six Promise.allSettled calls in the file — :263, :324, :339, :349, :399, :409 and here — none of which inspects its results, aggregates the rejections, or has any diagnostic channel to report them on. A child disposer that throws is swallowed and the enclosing remove() resolves. That is the exact opposite of the invariant the PR body claims for this design and that extension-kernel actually upholds: "timed-out or otherwise failed cleanup cannot report success" and "child-first exhaustive cleanup and reports aggregated disposer failures". Whatever the two kernels' relationship ends up being, they should not disagree about whether a failed cleanup is allowed to look like a successful one. Confirmed by reading the source at this head. Regression test needed: a failing child disposer; assert remove() and close() reject or surface an aggregated diagnostic.
| entry.parent = parent; | ||
| const target = parent?.children ?? this.#root(entry.rootId).entries; | ||
| target.splice(Math.min(position, target.length), 0, entry); | ||
| await this.#rebind(entry); |
There was a problem hiding this comment.
[P2] A failed move leaves the tree mutated with no rollback, and reports the entry as active in its new position. The four lines above this one splice the entry out of source, reassign entry.parent, and splice it into target — all before this await. If #rebind fails, nothing undoes any of it, and apply's rollback at :188 cannot help: it is gated on appliedOperations > 0, which is incremented only after the awaited call returns, so for a single-op move the counter is still 0 and #replaceSnapshot(before) never runs. The result is that the tree and the snapshot place the entry under the new parent while the live Fiber's Context is still parented under the old one — so its isolate and intercept resolution is the old parent's — and inspect() reports status: 'active'. The caller sees a thrown error and a tree that claims the operation succeeded. Do the splices after #rebind resolves, or capture and restore on failure. Confirmed by reading the source at this head. Regression test needed: a failing re-activation on move; assert the tree, the snapshot and the Context parentage are all unchanged.
| let label = this.#kernel.serviceLabels.get(name); | ||
| if (!label) { | ||
| label = Symbol(name); | ||
| this.#kernel.serviceLabels.set(name, label); |
There was a problem hiding this comment.
[P2] Any string property read on a Context permanently mints a Symbol here, so serviceLabels grows without bound. The Context proxy's get trap routes any unknown string property to target.get(property) (:531-533), which reaches #implementation -> #label, and #label caches a Symbol(name) for every name it is ever asked about. validateServiceName guards provide, isolate and intercept, but not this path, so the key space is whatever any plugin happens to read — including typos and ordinary property probes. Nothing prunes the map. Have #label take a create-or-not flag and let the read path use a non-minting lookup. Confirmed by reading the source at this head. Regression test needed: assert serviceLabels does not grow for property reads that resolve to no service.
| "./path-containment": "./dist/path-containment.js", | ||
| "./plan-mode": "./dist/plan-mode.js", | ||
| "./plan-tools": "./dist/plan-tools.js", | ||
| "./plugin-composition-loader": "./dist/plugin-composition-loader.js", |
There was a problem hiding this comment.
Scope objection rather than a defect, so no P level. These three subpath exports publish MakaCompositionLoader and the whole plugin-runtime surface as public contracts of @maka/runtime, and at this head nothing in the repository imports either module except the loader's own test file — only plugin-kernel.ts has a real consumer (extension-kernel.ts:1058-1064). This is the same objection as the open thread on packages/runtime-host/src/server/index.ts:12 ("Add the barrel exports in the PR that wires a consumer"), reappearing in a new package. Publishing an export is the hardest thing here to walk back later. Confirmed by reading the diff and the source at this head.
2f49db1 to
c3d06d9
Compare
xxhZs
changed the title
a640896 to
9050cb3
Compare
Migrate the feat-defined Context/Fiber kernel, plugin runtime contracts, composition loader, and public Runtime entrypoints. Keep Host control, persistence, package loading, and product wiring out of scope. Retain regression-tested foundation fixes for Service activation, Fiber transition and config semantics, Effect ownership and disposal, weak Plugin runtime caching, composition Tree failure invariants and inspection, safe Service record names, and contribution registration. Defer atomic Service-provider replacement and rollback, candidate Context Effect publication, and full asynchronous Effect failure policy to follow-up work that can define the required lifecycle contracts. Generated-by: Codex
9050cb3 to
25492b3
Compare
4 participants
Positioning
This PR lands the Runtime foundation extracted from
feat/extension-hook-contributions:Context/Fiber/Servicekernel and Effect ownership;MakaCompositionLoaderfor activation, tree mutation, transaction staging, package-wide reload, switching, and retirement;@maka/runtimesubpath exports.This is intentionally a foundation PR, not an independently usable product or Runtime Host integration. It does not add a Host Controller, durable Store, persistence, trusted package loader, product installation surface, Tool/UI wiring, or default consumer.
A later Host/product slice should migrate the feat-defined
HostExtensionController,HostExtensionRuntime, andHostPluginCompositionStoreinstead of introducing a parallel adapter or persistence model.Scope boundary
The diff remains limited to six Runtime files: three foundation modules, two focused test files, and Runtime package exports. No Host, Storage, or product package is changed. No public product model, persistence contract, or Provider transaction model is introduced. Entry config/inject/intercept values retain the feat shallow-copy contract.
Regression-tested foundation fixes
The implementation repairs foundation behavior without adding a new capability:
constructorandtoString;Explicitly deferred limitations
These require lifecycle or transaction contracts beyond this migration PR and are not claimed as fixed:
apply(),replaceSnapshot(), structural update, package reload, and move/rebind;Validation
git diff --check: passing.model-factory-tool-call-indexandnode-pty-write-lifecycle, outside this PR six-file diff.Generative tooling disclosure
OpenAI Codex made a substantive contribution to implementation, tests, review analysis, and PR wording. The commit includes a truthful
Generated-by: Codextrailer.