refactor(cli): split managed Oxigraph supervisor ownership - #2201
Conversation
|
@otReviewAgent Please run a merge-readiness review of the current head. This is the remaining behavior-neutral #2163 supervisor extraction; CI and the live managed-ownership gate are green. |
|
@otReviewAgent Please run a convergence review against current head 95932b1. The latest push replaced free-form lifecycle mutation with legal state intents, removed the half-initialized recovery cycle through one-time exit-handler registration, and passes 66/66 focused tests. |
|
Review Agent: please run a fresh merge-readiness review against current head |
|
@otReviewAgent Please review current head |
|
Heads-up for whoever rebases this stack onto the current integration head — a clean rebase here will produce a broken build, deterministically: #2195 (now merged into integration) added The fix is one line at Please build the storage package (and run |
…ip authority (#2189) * chore(storage): add an exports map and stop publishing daemon ownership authority The package published dist wholesale: every internal module was reachable by deep import, so removing the ownership mint from the barrel alone would have been a narrowing that cannot fail -- the reachable surface would not change. The exports map is what makes the narrowing real. "." is the public API; one documented internal entry point, ./internal/managed-oxigraph-ownership-v1, carries the daemon plumbing (the supervisor is the mint's only production consumer; the tests and the live gate exercise the authority deliberately). All 425 in-repo imports of the package were bare specifiers -- measured, not assumed -- so no consumer needed anything but the six ownership call sites moved to the internal entry point. moduleResolution is NodeNext, so tsc enforces the same map the runtime does. Resolution is a property of the packed artifact, not the source tree; this repo has shipped a release where packaging silently dropped tarball files. The managed-ownership lane therefore packs, extracts into a scratch node_modules, and probes with import.meta.resolve, which exercises the map without executing the module graph (registry-absent workspace deps cannot masquerade as resolution verdicts). The step then deletes the map from the extracted copy and requires the probe to FAIL: the tripwire is mutation- checked on every CI run, because a probe that cannot fail proves nothing. Fixes #2165. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(storage): make the internal entry a physical module and the pack gate package-owned Review round 1 of #2189, all three findings taken. The internal entry point is now a real module, src/internal/ managed-oxigraph-ownership-v1.ts, not a package.json alias onto a root *-internal file: the exported internal/ namespace matches a physical source namespace, and the next internal-only export has an obvious home instead of inventing another alias. Consumer specifiers are unchanged. The packed-artifact gate moves into the package that owns the invariant: scripts/verify-pack-exports.mjs, run as `pnpm run test:package-exports`. CI now orchestrates one command instead of embedding shell in a devnet-scenario workflow, and a storage maintainer can run the gate locally by name. The gate also gains the check whose absence the review correctly called out: resolution alone cannot catch the PRIMARY regression, because re-adding the mint to the barrel keeps every resolution probe green. The gate now proves the surface itself both ways -- the packed barrel (js and d.ts, comments stripped) names no ownership symbol, and the internal entry actually serves them, verified by IMPORTING the packed entry and enumerating its runtime exports (possible precisely because the authority module is dependency-free). Types have no runtime, so they are asserted statically: the shim links to the authority module and the authority d.ts declares every type. Both halves are mutation-checked on every run: deleting the exports map from the extracted copy must fail the resolution gate, and re-exporting the mint from the extracted barrel must fail the surface gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(storage): harden packed ownership boundary * fix(storage): preserve public ownership refusal error * fix(storage): keep the package manifest resolvable through the exports map The remaining piece of #2189 round 2. An exports map flips subpath access from open to allowlist, and the allowlist omitted ./package.json -- so tooling that reads package metadata through Node resolution broke as a side effect unrelated to hiding the ownership authority. The manifest subpath is the ecosystem-standard exception; the probe expectation flips to prove it RESOLVES while every dist/* deep import stays refused. The error-contract and gate-structure findings were fixed concurrently in 542e97d (dedicated error module on the barrel, phase-structured gate with a positive publishes-the-error check); this commit adds only what that one did not cover. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(storage): one module identity; the pack gate imports like a consumer Review round 3 of #2189. The CI break first: the #2163 supervisor split rebased in with a barrel import of ManagedOxigraphSupervisorHandoffV1, which this branch had removed from the barrel. oxigraph-server-supervisor.ts now uses the internal entry. The compatibility shim is deleted. The authority has ONE source identity -- src/internal/managed-oxigraph-ownership-v1.ts -- and all twenty in-package importers point at it directly. This resolves the tension between two review rounds that asked for opposite things (make src/internal the physical home; do not keep two names for one singleton-sensitive module): the clean rename satisfies both, and the WeakMap authority table has exactly one module path. The gate stops being a bespoke parser. Workspace deps are linked into the scratch install so the packed barrel and internal entry are IMPORTED as a consumer imports them, with namespace membership asserted -- an aliased re-export that merely mentions a name in text can no longer produce a false verdict in either direction. The type surface is a compiled consumer fixture under NodeNext: allowed imports plain, the forbidden mint import under a suppression that turns into an unused directive if the mint returns. The regex d.ts parser, emitted-JS scanner, and comment-stripper are deleted. Three mutants, each through the real path it guards: exports map deleted (resolution fails), mint re-exported in dist/index.js (runtime namespace assertion fails), mint re-exported in dist/index.d.ts (type fixture fails). Each with a restore check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(storage/cli): narrow the authority entry; derive the gate end to end Review round 4 of #2189, all four findings taken. The internal entry is narrowly the ownership authority again: the public error is no longer doubled through it, and every file that only needs to recognize the error imports the error module (or the barrel), not the authority. The gate PINS this -- the probe asserts the error is absent from the authority entry's namespace. Nothing in the gate is hand-maintained anymore. The scratch consumer's dependency graph is read from the packed package.json rather than a hard-coded list, and the forbidden-surface set is DERIVED: every runtime export of the authority entry is internal-only, so the probe fails if ANY of them appears on the barrel -- adding an authority export automatically extends the gate. A representative non-mint mutant (attach helper re-exported on the barrel) proves the derived set is what fails the gate, not a special case on the mint's name; the type fixture gains a representative forbidden type-only import. The CLI daemon's exceptional dependency on the storage-internal subpath is one auditable doorway: managed-oxigraph-ownership-bridge.ts re-exports exactly what the daemon uses, and the four daemon modules import the bridge. When the storage-side authority moves, one file knows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(storage): isolate ownership authority imports * fix(storage/agent): identity-based leak gate as a thin runner; fix a live agent-test break Review round 5 of #2189, all four findings taken. The aliased-leak bug first, because it was real: leak detection compared KEYS, so `export { mint as createStorageOwner }` published the authority under a new name and passed. Detection is now by VALUE IDENTITY -- every authority export is checked against every barrel export value, so an alias leaks under any name (primitives skipped, stated in the fixture, since identity cannot distinguish equal primitives). A dedicated aliased-mint mutant proves it. The exports map is now asserted EXACT: the packed manifest's keys must equal the three-entry allowlist, so an undocumented subpath cannot appear without failing the gate. The gate is a thin runner over data and static fixtures: the contract is one frozen GATE block; the runtime probe and the consumer type fixture are real files under scripts/pack-gate/ (parameterized by env, parseable by tooling, no generated source strings); mutants share one helper with restore checks. The agent test regression the review caught was LIVE: the test imported the public error from the authority entry, which stopped exporting it -- a runtime break invisible to tsc because the agent tsconfig includes only src/, and none of my round-4 runs executed the agent tests. Imports split correctly now (error from the barrel, authority from the internal entry); agent tests re-run green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(storage): pin every removed ownership type in the consumer fixture Runtime namespace checks cannot see type-only exports, so each of the five removed ownership types is now individually pinned in the static fixture -- any one returning to the barrel flips its suppression to an unused directive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(storage): match the repo's canonical exports condition shape types/import/default on both JS-bearing entries, consistent with sibling packages -- storage is no longer the one package resolving ESM through the default catch-all alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(storage): pin the barrel's value-export name set against a snapshot Review round 7 of #2189. Identity detection catches re-exports of authority values; it cannot see a WRAPPER function around the mint, which is not identity-equal, and no mechanical check can decide what a fresh function invokes. The honest mechanical answer is the reviewer's own suggestion: pin the barrel's export-name set. The packed barrel's 83 value exports live in pack-gate/barrel-value-exports.json; the probe asserts exact set equality with added/removed names in the failure message, so ANY new public name -- wrapper or not -- is a visible reviewed diff to the snapshot in the same PR, never a silent addition. A wrapper-mint mutant proves it: exporting createStorageOwner delegating to the mint fails the snapshot assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(cli): route split supervisor through ownership bridge --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Jurij Skornik <jurij.skornik@gmail.com>
bdb7838 to
246e0e6
Compare
|
@otReviewAgent Please run a fresh merge-readiness review against current head 246e0e6. The branch is rebased onto exact integration head b30bfc1; the prior operation-mesh and brittle architecture-test findings are addressed with narrow capability wiring and parsed import-graph rules. Local validation passes: full dependency closure and CLI/Storage builds, 66/66 focused tests, packed exports gate with all mutants, harness typecheck, and live managed-ownership gate 31/31. |
|
@otReviewAgent Please review current head e77eb1a for merge readiness. The initial current-head CI exposed one integration-added Storage test still importing the pre-#2189 ownership path; whole-repo scanning found no other stale source/test specifiers, and the exact 35-file Storage CI lane now passes locally (623/623). Prior convergence fixes and all previously reported local validation remain intact. |
|
@otReviewAgent Please run a fresh convergence review of current head f01b16e. The remaining valid test-topology concern is addressed: the boundary test now protects only the stable public facade and single Storage-authority bridge, with import and re-export parsing. The broad-context and package-semver comments have evidence-based replies in their existing threads. Focused boundary/state validation passes 3/3; all prior behavior, Storage, package, typecheck, and live evidence remains unchanged. |
2 participants
Summary
OxigraphSupervisorGenerationV1; legal lifecycle and clean-handoff decisions stay behind named state-owner intents.Pick-limited capabilities instead of whole sibling operation objects; the architecture gate protects the stable public facade and cross-package authority bridge without freezing operation topology or call names.b30bfc19f: repoint integration-added lane-controller and guard-test imports to the physical internal ownership module and extend the packed public-surface snapshot with the 17 legitimate Storage values added by integration.Related
Diagrams
Managed supervisor lifecycle
Supervisor operation dependencies
Before:
sequenceDiagram participant Composer participant Startup participant Handoff participant Recovery participant Shutdown Composer->>Startup: construct with whole Shutdown owner Composer->>Handoff: construct with whole Recovery and Shutdown owners Startup->>Shutdown: beginTermination() and stopLocked() Handoff->>Shutdown: prove release and begin termination Handoff->>Recovery: reviveLocked() and scheduleRevive()After:
sequenceDiagram participant Composer participant Startup participant Handoff participant Recovery participant Shutdown Composer->>Startup: inject callbacks bound to Shutdown Composer->>Handoff: inject callbacks bound to Recovery and Shutdown Startup->>Shutdown: terminate and stop via injected callback Handoff->>Recovery: revive or schedule via injected callback Handoff->>Shutdown: prove release or terminate via injected callbackStorage ownership-authority resolution
Before:
sequenceDiagram participant Ext as External consumer participant Daemon as CLI daemon participant Resolver as Node resolver participant Storage as dkg-storage Daemon->>Resolver: import package root Resolver->>Storage: resolve dist/index.js Storage-->>Daemon: public barrel including ownership authority Ext->>Resolver: import package/dist/internal-file.js Resolver->>Storage: unrestricted deep resolution Storage-->>Ext: internal module resolvesAfter:
sequenceDiagram participant Ext as External consumer participant Daemon as CLI daemon participant Resolver as Node resolver participant Storage as dkg-storage Daemon->>Resolver: import package/internal/managed-oxigraph-ownership-v1 Resolver->>Storage: resolve documented internal entry Storage-->>Daemon: narrow ownership authority Ext->>Resolver: import package/dist/internal-file.js Resolver-->>Ext: ERR_PACKAGE_PATH_NOT_EXPORTED Ext->>Resolver: import package root Resolver->>Storage: resolve dist/index.js Storage-->>Ext: public barrel without ownership authorityFiles changed
packages/cli/src/daemon/oxigraph-server.tspackages/cli/src/daemon/oxigraph-server-contract.tspackages/cli/src/daemon/oxigraph-server-supervisor.tspackages/cli/src/daemon/oxigraph-supervisor-state.tspackages/cli/src/daemon/oxigraph-supervisor-generation.tspackages/cli/src/daemon/oxigraph-supervisor-{startup,recovery,shutdown,handoff}-operations.tspackages/cli/src/daemon/oxigraph-supervisor-{child,lifecycle,probes,revive,ownership}.tspackages/cli/src/daemon/managed-oxigraph-ownership-bridge.tspackages/cli/test/oxigraph-server-module-boundary.test.tspackages/cli/test/{oxigraph-server,oxigraph-managed}.test.tspackages/storage/package.jsonpackages/storage/src/internal/managed-oxigraph-ownership-v1.tspackages/storage/src/index.tspackages/storage/src/managed-oxigraph-backend-unowned-error.tspackages/storage/src/system-record-lane-controller-contract-v1.tspackages/storage/scripts/verify-pack-exports.mjspackages/storage/scripts/pack-gate/*packages/storage/src/**,packages/storage/test/**packages/agent/test/**,packages/cli/test/support/**.github/workflows/system-record-managed-ownership.yml,devnet/issue-2052-managed-ownership/run.tsTest plan
b30bfc19fc72bb0728dd076c1485602afb0212fa.v22.22.0.pnpm --filter @origintrail-official/dkg build.pnpm --filter @origintrail-official/dkg-storage build.pnpm --filter @origintrail-official/dkg-storage run test:package-exports(all properties held and all mutants were killed/restored).pnpm typecheck:live:system-record-managed-ownership.pnpm test:live:system-record-managed-ownership(31/31 checks across three pinned predecessor commits).git diff --checkand confirm generatedlocalhost_contracts.json, ignored evidence, secrets, and unrelated local files are excluded.