feat(plugin-api): golden .d.ts API snapshot test (epic #470 C1) - #780
Merged
Conversation
@omadia/plugin-api is the type contract the kernel and every plugin compile against, and a breaking change to it is currently invisible at the moment it is made: every consumer lives in this repo, so they are all recompiled in the same commit and tsc stays green while a renamed method or a narrowed type quietly changes what a plugin must build against. Once plugins ship from their own repositories that silence becomes an install-time incident in another repo. Compile the package's declarations into a temp dir, normalize them (comments stripped, blank lines dropped, whitespace collapsed, files in sorted path order) and compare against a committed snapshot. Drift fails with a unified diff naming both the regeneration command and the SemVer bump it implies. - scripts/api-snapshot.mjs --check (default) / --update - api-snapshot/plugin-api.d.ts.snap 34 declaration files, 2331 lines - test/apiSnapshot.test.ts the node:test gate CI runs - README.md what to do when it goes red, with the SemVer table Declarations go to a temp dir rather than dist/, so the check cannot race the test files that import the compiled output and always measures the current src/ instead of a previous build. assertEmitCoverage derives its expectation from disk — every src/**/*.ts must produce a matching .d.ts — so a shifted rootDir cannot leave the check green while it covers less than the package. The middleware root test glob only reaches test/**, never workspace package tests, so the package test dir is added as a second positional glob to that same script. No new required CI job; the existing "middleware (lint + typecheck + test)" job runs the gate. tsconfig.json has rootDir: src, which left the new test tree outside every project. tsconfig.test.json closes that and is folded into the package typecheck. No publishing — the package stays private: true (D1 unchanged). Mutation proof: an added export and a removed re-export each fail the check and the node:test gate with exit 1; reverting restores green. Core-decoupling ratchet unchanged at 3296.
The golden .d.ts snapshot concatenated files in `localeCompare` order, so its bytes depended on ambient ICU collation rather than on the source. Measured on this package: ICU (en-US, de-DE, sv-SE, C and POSIX all agree) orders routinesIntegration.d.ts before routineTarget.d.ts, while code-unit ordering is the reverse -- a Node built without Intl sorts the other way and the check goes permanently red for a reason no source diff explains. Sort by code unit at both call sites and regenerate. The snapshot change is a pure relocation of those two blocks: sorted, old and new are byte-identical. Also pin *.snap to LF. The check compares the committed file byte-for-byte against generated text that always uses \n, and the repo has no `* text=auto`, so a contributor with core.autocrlf=true (the Git-for-Windows installer default) would get a CRLF working copy and a red check with an invisible cause. It is the only .snap in the repo, so the rule renormalizes nothing.
Weegy
added a commit
that referenced
this pull request
Aug 20, 2026
…anded PR #780 (C1) merged to main while this branch was in review, so the snapshot the gate compares against now exists. Merged origin/main and regenerated it with `npm run api:update -w packages/plugin-api`. The diff is exactly this PR's intended surface change and nothing else: ServiceCaller, PerCallerFactory, perCallerService, isPerCallerService, resolvePerCallerService and ServiceNotDeclaredError added, and ServicesAccessor.provide/replace widened to accept a PerCallerFactory. No removals — the DevJob* types were already gone from `src/` before either branch, leaving only a tombstone comment that the snapshot strips. So this PR's own surface change is additive; the 0.1.0 -> 1.0.0 bump is the deliberate departure from 0.x, not a break in this diff. npm run api:check -w packages/plugin-api ✓ up to date npm test -w packages/plugin-api 1/1
Weegy
added a commit
that referenced
this pull request
Aug 20, 2026
#783) * feat(#470): grant-gate ctx.services.get and cut plugin-api 1.0.0 (C2b) Closes the second half of C2 (epic #470 Phase A): the G8 contract break and bug B1. G8 — the contract break, taken once, deliberately. C2a (#555) already deleted `ctx.devJobs` and moved the `DevJob*` view types out of `@omadia/plugin-api` into `middleware/src/devplatform/devJobTypes.ts`, where they travel with the extraction. This records that break and cuts the package at 1.0.0: there is no installed base, nothing is published to npm, and every consumer is a repository we control, so the break is cheap now and expensive later (`implementation.md` §1 row 4). Adds a CHANGELOG naming the removed types, so a consumer grepping its own source for `DevJobDescriptor` lands on the migration note. `harness-channel-api` pinned `^0.1.0` and would have failed to resolve against the bump; repinned to `*` like its siblings. B1 — `ctx.services.get` was a bare pass-through. Any installed plugin could resolve any registered service, `graphPool` included, with no manifest declaration and nothing in the install dialog. `serviceRegistry.ts`'s own header conceded it: "enforcement lives at the consumer seam". The seam now exists. `get` resolves only capability names the plugin declares in `requires:` (or `provides:`, to read back its own registration) and throws the new typed `ServiceNotDeclaredError` otherwise, naming the capability and the manifest field that would grant it. `has` stays ungated — existence is not a capability. Removing `ctx.devJobs` without this would have converted a permission-gated, kernel-attributed accessor into an ungated, self-attributed one (§2.2). So `provide` also accepts `perCallerService(factory)`: the kernel invokes the factory with the id it activated the consumer under, never with an argument the consumer supplies. The factory is a symbol-branded object, so a service that is itself a function cannot be mistaken for one; value providers are untouched. A call-site audit across this repo's built-in plugin packages and all ten standalone plugin repos found 27 (plugin, capability) pairs consumed without being declared — a fail-closed gate in one step would have broken every shipped plugin. Those exact pairs sit behind a dated, frozen, per-plugin allowlist: they warn once and resolve, everything else fails closed. The allowlist is closed in both directions — a different plugin asking for the same name still throws, and an allowlisted plugin asking for a new name still throws. Counter-proof: reverting the gate to `return serviceRegistry.get<T>(name)` fails 9 of the 25 new tests; restoring it passes all 25. Ratchet 3296 → 3300, hand-raised for `middleware/packages` only. All five lines are the new CHANGELOG documenting a removal; three of them are literal strings that cannot be reworded (a spec path, a test filename, the future package name). The first measurement was +31 — the avoidable 26 were reworded away rather than excused, leaving `middleware/test` at its 1,030 baseline. Justification recorded in the baseline JSON, README and acceptance.md. Also corrects plan.md §4.2, which still claimed the `DevJob*` types stay in core — `implementation.md` §2.5 had already flagged it as contradicting §4.1, and shipped code now settles it. * fix(#470 C2b): close three gaps in the ctx.services.get grant gate Cross-family review of PR #783 found the gate correct in shape but incomplete in three ways that a passing test suite did not surface. 1. The dated legacy allowlist was missing 21 (plugin, capability) pairs that are resolved today, 9 of them on @omadia/orchestrator itself. `harness-orchestrator/src/plugin.ts:452` reads 'llmProviderCatalog' unconditionally near the top of activate(), so the gate as merged would have thrown ServiceNotDeclaredError and killed the chat orchestrator at boot. The first audit missed these because the names sit behind exported constants (PROCESS_MEMORY_SERVICE_NAME, PLUGIN_CAPABILITIES_SERVICE, CHANNEL_RESOLVER_SERVICE, ...) rather than string literals, and because some channel plugins resolve capabilities through shared @omadia/channel-sdk helpers instead of a literal call site in their own source. 2. Nothing derived the allowlist from the repository, so the miss above was invisible to CI. test/pluginServiceGrantCoverage.test.ts now walks every middleware/packages/*/manifest.yaml, resolves each `services.get` argument through the TypeScript checker (literals and const identifiers alike, comments excluded), and fails when a name is neither declared nor allowlisted. It found three further gaps on @omadia/ui-orchestrator that hand analysis had also missed. A second case fails on stale built-in rows so the ramp cannot rot. 3. Only ctx.services.get passed the caller. Every other plugin-facing surface — ctx.memory, ctx.entities, ctx.mcp, ctx.subAgents, ctx.llm, ctx.events — still called serviceRegistry.get(name) and therefore silently received KERNEL_SERVICE_CALLER, so a perCallerService provider would have handed the plugin the kernel-scoped instance. That is the exact self-attribution failure the feature exists to prevent. The frozen ServiceCaller built in createPluginContext is now threaded through all six. Also: perCallerService documented "one implementation per consuming plugin" while invoking the factory on every .get(). Resolution is now memoized by factory object then by caller.pluginId, which makes the documented contract true and self-invalidates on provider replacement because a replaced provider is a different object. The gate test fixture stored `manifest: {}`, but memoryDeclared reads permissions off the catalog entry's unparsed manifest — so ctx.memory was undefined regardless of what the fixture declared and the new attribution test asserted on nothing. The fixture now carries the raw manifest document. Ratchet unchanged at 3300; no banned strings added. * chore(#470 C2b): regenerate the plugin-api golden snapshot after C1 landed PR #780 (C1) merged to main while this branch was in review, so the snapshot the gate compares against now exists. Merged origin/main and regenerated it with `npm run api:update -w packages/plugin-api`. The diff is exactly this PR's intended surface change and nothing else: ServiceCaller, PerCallerFactory, perCallerService, isPerCallerService, resolvePerCallerService and ServiceNotDeclaredError added, and ServicesAccessor.provide/replace widened to accept a PerCallerFactory. No removals — the DevJob* types were already gone from `src/` before either branch, leaving only a tombstone comment that the snapshot strips. So this PR's own surface change is additive; the 0.1.0 -> 1.0.0 bump is the deliberate departure from 0.x, not a break in this diff. npm run api:check -w packages/plugin-api ✓ up to date npm test -w packages/plugin-api 1/1
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a golden
.d.tsAPI snapshot gate to@omadia/plugin-api— epic #470, Phase A item C1 (specs/470-dev-platform-plugin/implementation.md§3).packages/plugin-api/scripts/api-snapshot.mjs— compiles the package's declarations and normalizes them into a deterministic snapshot.--check(default) fails with a unified diff on drift;--updateaccepts the current surface.packages/plugin-api/api-snapshot/plugin-api.d.ts.snap— the committed snapshot (2,331 lines, 34 declaration files).packages/plugin-api/test/apiSnapshot.test.ts— thenode:testgate that makes CI run the check.packages/plugin-api/README.md— what to do when the check goes red, with the SemVer table.packages/plugin-api/tsconfig.test.json— so the new test tree is typechecked at all (see design notes).specs/470-dev-platform-plugin/README.md— C1 recorded under Status.No publishing. The package stays
private: true; D1 is untouched.Why
This package is the type contract the kernel and every plugin compile against, and a breaking change to it is currently invisible at the moment it is made. Every consumer lives in this repo, so they are all recompiled in the same commit and
tscstays green while a renamed method, an added parameter, or a narrowed type quietly changes what a plugin must build against. Once plugins ship from their own repositories that silence stops being free — it becomes an install-time incident in someone else's repo, discovered by a customer.The snapshot turns that class of change into a diff in the PR that causes it.
How to update the snapshot
A red check is not a request to run
api:updateand move on — read the diff first. If the change is intended, regenerate and bump the version in the same commit:After the split that version number is the only signal an out-of-repo plugin gets about whether its pinned contract still holds, so a snapshot updated without a bump is the same silent break C1 exists to stop.
Mutation proof
A gate is worth nothing until it has been seen to fail. Both directions were probed against a committed-green snapshot.
Baseline — PASS
Probe 1 — added export (
export type CanaryProbeC1 = { readonly canary: string };appended tosrc/index.ts) — FAIL, exit 1The
node:testgate itself was run under the same mutation, so the proof covers the thing CI executes and not only the script:Probe 2 — removed export (
export * from './pkce.js';deleted fromsrc/index.ts) — FAIL, exit 1Reverted — PASS
Design notes for review
dist/. Under the root suite this check runs alongside test files that import the package's compiled output; rewritingdist/in place would race them. It also keeps the check honest — it always measures the currentsrc/, never what a previous build left behind..d.tsare snapshotted, not justindex.d.ts. The emitted tree is what a consumer'stscactually reads. Probe 2 confirms this catches a re-export removal viaindex.d.tstoo..d.tsstring-literal and template-literal types in this package legitimately contain//and/*(route prefixes, URLs). Six such shapes were verified to survive stripping intact.assertEmitCoveragederives the expectation from disk: everysrc/**/*.tsmust have produced a matching.d.ts, and an empty source list is a hard error. Without it, a shiftedrootDiror a staleincludewould make tsc emit less than the package while the check reported green forever — the permanently-green-no-op failure this repo has been bitten by before (feat(ci): golden-set regression eval for LLM verifier behaviour (#129) #640, CI never runs any.pg.test.ts— the middleware job has no postgres service #565).tsconfig.jsonhasrootDir: src, so the test tree sat outside every project — a type error there would only have surfaced when tsx hit that line at runtime. Addedtsconfig.test.jsonand folded it into the package'stypecheck; verified with--listFilesOnlythat the project actually loads the test file.CI wiring
The
middleware (lint + typecheck + test)job runs the middleware rootnpm run test, whose glob istest/**/*.test.ts— it does not reach workspace package tests. Rather than add a required job, the package test dir was added as a second positional glob to that same script, so the gate runs inside the existing job:scripts/check-test-file-durations.mjsparses--test-timeoutout of that script; the flag is untouched and still parses. The new file measured 766 ms in the full run, against a 60 s fail threshold.Verification
npm install(middleware)package-lock.jsondiffnpm run buildnpm run typechecknpm run typecheck:testnpm run lintnpm run test(full middleware suite)npm test -w packages/plugin-apinpm run test:filetimesnode scripts/check-core-decoupling.mjsCloses part of #470 (item C1).
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Cross-family review (Forge)
Adversarial pass by Forge (GPT-5.4,
reasoning_effort=high) against the code rather than the claims. Five questions, five verdicts. Two real defects found and fixed in08a45d84; the rest verified sound.middleware (lint + typecheck + test)?ci.yml→ rootnpm run test→ glob now carries'packages/plugin-api/test/**/*.test.ts'; ran that glob standalone → 1 pass. Rootnpm run typecheckreaches the package via-w @omadia/plugin-api, which now also runstsconfig.test.json.check-test-file-durations.mjsstill parses--test-timeout=120000out of the edited script.rootDirto the package root: hard error naming all 34 uncovered files, exit non-zero. Emptysrc/is a separate hard error. A narrowedexcludecannot shrink the set either —index.tsre-exports every module, so tsc pulls them all transitively. Thenode:testgate additionally asserts on the success message, so a script that exits 0 without doing work still fails.listFilesordered witha.localeCompare(b)— snapshot bytes depended on ambient ICU collation. Measured on this package's real files: ICU (en-US, de-DE, sv-SE, C, POSIX all agree) putsroutinesIntegration.d.tsbeforeroutineTarget.d.ts; code-unit ordering is the reverse, so a Node built without Intl flips two blocks and reds the check with no source change to explain it. Fixed: code-unit comparator at both call sites, snapshot regenerated. Sorted, old and new snapshots are byte-identical — a pure relocation, zero declaration content changed. CRLF was already neutral insidenormalizeDeclaration; tsc version drift is bounded by the lockfile and both modes resolve the same binary..snapis compared byte-for-byte against text that always uses\n, and the repo has no* text=auto. A contributor withcore.autocrlf=true(Git-for-Windows installer default) gets a CRLF working copy and a permanent red check with an invisible cause. Fixed by pinning*.snap text eol=lfin the root.gitattributes. It is the only.snapin the repo andgit add --renormalize .touches nothing else, so the rule has no blast radius. No Windows runner executes middleware tests, so this was contributor-facing, not CI-facing.--updatesilently accept a failed build?src/index.ts:--updateexited 1 and left the snapshot byte-identical —execFileSyncthrows on tsc's non-zero exit before any write.--checkfails the same way.mkdtempdir, and--composite false --incremental falsemeans no.tsbuildinfo. Verified across three isolated runs plus thenode:testgate: nothing is written inside the package — nodist/touch, no buildinfo,git statusclean.src/is read-only to the check.Re-verified after the fix
node scripts/api-snapshot.mjs --check+export type ForgeProbeC1node:testgate CI runspass 0 / fail 1routineTarget.d.ts(L2027) now precedesroutinesIntegration.d.ts(L2060)npm run typecheck(middleware, all workspaces)npm test -w @omadia/plugin-apinode scripts/check-core-decoupling.mjsNotes, not blockers
npm run lintcoverspackages/plugin-api/src/only, so the newscripts/*.mjsandtest/are unlinted. That is the repo's existing lint scope, not something this PR changed.listFiles(src, '.ts')would map a futuresrc/*.d.tsto*.d.d.tsand hard-fail. No such file exists today, and the failure would be loud rather than silent.--check. The unsafe direction is unreachable: overwriting requires the exact string--update.Verdict: MERGE. The gate does what the PR claims — it runs in the required CI job, it cannot pass while measuring nothing, and it fails on drift in both directions. The one substantive defect was a golden artifact whose byte order came from the environment instead of the language; that is now fixed and the snapshot is provably unchanged in content.