feat(#470): grant-gate ctx.services.get and cut plugin-api 1.0.0 (C2b) - #783
Merged
Conversation
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.
…act-services-gate
…act-services-gate
…act-services-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.
…act-services-gate
…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
This was referenced Aug 20, 2026
Weegy
enabled auto-merge (squash)
August 20, 2026 16:02
Weegy
added a commit
that referenced
this pull request
Aug 20, 2026
C2b landed on main as #783 with extra Forge fixes (grant-gate coverage, perCallerService memoization, plugin-facing caller attribution, 63 allowlist pairs). Main's C2b is the base truth here; C7's additions are re-applied on top of it. - pluginServiceGrants.ts: main's 63-pair allowlist taken whole (C7 added nothing here). - pluginServiceGrantGate.test.ts: main's suite taken whole, then C7's borrowed-pool wiring test re-added and the warn-once case switched back to a read-through assertion (borrowPool hands back a Proxy, not the pool). - packages/plugin-api/src/pluginContext.ts: main's perCallerService memoization kept; C7's ctx.sql surface merged cleanly around it. - src/platform/pluginContext.ts: C7's SQL gate, borrowPool wiring and migration imports re-applied over main's caller-attribution changes. - CHANGELOG / spec README: both sides kept, C7 section placed between C1 and C8. plugin-api 1.0.0 -> 1.1.0 and the golden .d.ts snapshot regenerated: C7 adds ctx.sql plus SqlAccessor/SqlPermission/MigrationReport/RunMigrationsOptions and three error classes. All additive, so MINOR per the package README table. Core-decoupling ratchet unchanged at 3300 (identical to main's baseline).
Weegy
added a commit
that referenced
this pull request
Aug 20, 2026
…unt (W1) (#792) * feat(#778): wire #577 skill-promotion route + #578 credential-asks mount (W1) Mounts two fully built, previously-unreachable surfaces into the composition root (`middleware/src/index.ts`) — the wiring debt #778 exists to close. ## Skill promotion (#577 P3) - New `src/routes/skillPromotion.ts`: `POST /api/v1/admin/skills/:skillId/promote`, the HTTP surface for `PgSkillOwnershipLifecycleStore.promoteSkillOwnerScope` (the only path a skill ever reaches `group`/`org` ownership). Session auth replicates `routes/bulkPromotion.ts`'s `req.session.omadia_user_id` chain EXACTLY (same 401 shape, same "every authenticated session is an operator" posture) — the auth-check precedent #771's PR body explicitly deferred this route to get right, not rush. - New `src/services/skillManifestSigningKey.ts`: resolves (generate-once, persist) the HMAC key `promoteSkillOwnerScope` re-signs a skill's tamper- evident manifest with. Mirrors `auth/sessionSigningKey.ts` exactly — same vault, same "generate on first boot, reuse forever" pattern — but under its own vault scope (`core:skills`, not `core:auth`): a data-integrity key is a different trust domain than an auth-token key, the same reasoning `credentials/crypto.ts` already gives for keeping the credential-keychain master key separate from the provider-secret vault's. - `index.ts`: constructs `PgSkillOwnershipLifecycleStore` and mounts the route ONLY when `graphPool` is available (same gate `bulkPromotionService` uses) — the store needs a real Postgres pool. ## Credential asks (#578 Phase 3) - Mounts the already-built, already-route-tested `routes/credentialAsks.ts` (#774) at `/api/v1/admin/credential-asks`, behind `requireAuth`. - The router needs a `CredentialAskStore`, which needed a `CredentialStore` behind it (`InMemoryCredentialAskStore`'s constructor takes one) — NEITHER was constructed anywhere in `index.ts` before this PR, so this also resolves the credential-keychain's own master key (`CREDENTIAL_KEYCHAIN_KEY` env, `resolveCredentialMasterKey` — built by #578 P1, never called until now) and builds the store via the existing `credentialStoreFactory.ts` (Postgres when `graphPool` is configured, in-memory otherwise — same explicit backend choice that factory already documents). ## Wiring tests (the point of this issue) A router that exists, is fully unit/route-tested standalone, and is never mounted passes every one of those tests — that is exactly how both surfaces sat unreachable for a full phase. `index.ts` runs `main()` unconditionally at import time (DB pools, mDNS, `app.listen`) and is not designed to be booted from a test — verified no test in this repo does that. - `test/778RouteMounts.wiring.test.ts`: asserts the LIVE (comment-stripped) source of `index.ts` contains both `app.use(...)` mount lines with the correct path + `requireAuth` + router factory call. Mutation-checked: with the skill-promotion mount line commented out, this test fails (see below). - `test/skillPromotionRoute.test.ts`: real `app.listen(0)` + `fetch` behavioral coverage for the new route — 401 with no session, 400 on a malformed body, 200 promoting to `org`/`group` scope with the actorScope built from the session, 404/409/403 error-code mapping. `SkillPromotionRouteDeps.store` is narrowed to `Pick<PgSkillOwnershipLifecycleStore, 'promoteSkillOwnerScope'>` so the test uses a fake store instead of a real `Pool`. ## Mutation evidence Commented out the skill-promotion `app.use(...)` line in `index.ts` (regex match on the real mount, not a copy) and reran `778RouteMounts.wiring.test.ts`: 1 failure, exactly the mounted-router assertion — the other four assertions (import present, credential-asks mount, etc.) stayed green as expected. Reverted; `git diff` after revert showed zero residue. `dist/` rebuilt via `npm run build` before and after. ## Full-suite regression 7417 tests / 7405 pass / 0 fail / 12 pre-existing skips (`npm test`, non-pg). ## Migration None — reuses existing tables (`skills` from 0040, `credentials`/ `credential_asks` from 0042/0043). Confirmed 0045 (`publish_versions`) is the latest; next free number is 0046 for any following #778 phase that needs one. ## Blast radius - `middleware/src/index.ts`: additive only — 2 new imports blocks, 2 new `const` resolutions near existing key resolution, 2 new `app.use(...)` mounts inserted after the existing bulk-promotion mount block. No reordering of existing code. - 4 new files (2 src, 2 test). Zero edits to the #577/#578 service-layer files themselves (`skillLifecycle.ts`, `skillLifecycleStore.ts`, `credentialAsks.ts`, `asks.ts`, `postgresCredentialAskStore.ts`) — consumed only. - Compatible with #783's `ctx.services.get` grant gate: `index.ts` never goes through `ctx.services.get` for anything this PR touches — it constructs `PgSkillOwnershipLifecycleStore` and the credential stores directly, the same way `audienceGrantStore`/`bulkPromotionService` already do. No plugin manifest changes needed for W1. Base: origin/main. Part of #778 (wiring wave) — W1 of 4 phases (routes / agent tools / admin UIs / notification+gateway-caller). W2-W4 tracked separately; see PR description for scope notes. * fix(#778): satisfy the test-tree typecheck ratchet Two real type errors CI's #573 ratchet caught that plain `tsc` over src/ never sees: the automation-blocked stub used origin 'cron', which is not a member of SystemScopeOrigin ('schedule' is the recognised machine origin — and with the correct origin the `as ScopeId` cast becomes unnecessary), and the session stub satisfied only the omadia_user_id field while the route's type expects full SessionClaims. Fixed rather than baselined.
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
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.
Closes the second half of C2 in epic #470 Phase A: the G8 contract break and bug B1. Moves zero dev-platform code.
What and why
G8 — the contract break, taken once and deliberately
C2a (#555) already deleted
ctx.devJobsand moved theDevJob*view types out of@omadia/plugin-apiintomiddleware/src/devplatform/devJobTypes.ts, where they travel with the extraction and the plugin repo will own them as@omadia/dev-platform-plugin-api. I re-verified that: the package exports zeroDevJob*types today, andadmin-v1carries nodev_jobs/DevJobDTO fields (grepping fordev_job|DevJob|devJobinmiddleware/src/api/admin-v1.tsreturns nothing).What was missing was the record and the version. This PR cuts
@omadia/plugin-api1.0.0 and adds aCHANGELOG.mddocumenting the break in English.implementation.md§1 row 4 is the rationale: there is no installed base, nothing is published to npm, and every consumer is a repository we control — the break is cheap now and expensive later.One landmine found:
harness-channel-api/package.jsonpinned"@omadia/plugin-api": "^0.1.0", the only package that did. It would have failed to resolve against the bump. Repinned to"*"like its 22 siblings.B1 —
ctx.services.getwas a bare pass-throughpluginContext.tshanded the service registry straight through. Any installed plugin could resolve any registered service —graphPool, the same Postgres pool core uses, included — with no manifest declaration, no operator consent, and nothing in the install dialog.serviceRegistry.ts's own header conceded the design: "This registry is a naked service-locator; enforcement lives at the consumer seam."The seam now exists:
middleware/src/platform/pluginServiceGrants.ts.get(name)resolves only capability names the plugin declares inrequires:— orprovides:, so a provider can read back its own registration, which is not an escalation since it holds the implementation anyway. Anything else throws the new typedServiceNotDeclaredError, naming both the capability and the manifest field that would grant it. No new manifest field is invented:pluginContext.ts's own capability docblock already states that capability names are the service-registry keys.hasstays ungated. It answers a yes/no existence question and hands over no capability; gating it would only turn feature-probing into exception-handling.A plugin with no catalog entry is granted nothing, mirroring the existing
scratchEnabledprecedent one function below: an id the kernel cannot find a manifest for is an id whose permissions cannot be checked, and unknown permissions are denied permissions.The trap this closes (§2.2)
Removing
ctx.devJobswithout this would have converted a permission-gated, kernel-attributed accessor into an ungated, self-attributed one — the only remaining path being B1's naked locator, with the plugin passing its ownpluginIdintolistGrantedRepoIds.So
provide()now also acceptsperCallerService(factory). The kernel invokes the factory with aServiceCallerbuilt from the id it activated the consumer under, never from an argument the consumer supplies. The consumer's only input is the service name; there is no second parameter through which it could name itself something else, and there is a test assertingservices.get.length === 1so a future refactor cannot quietly add one.The factory is a symbol-branded object, not a bare function. Detecting it with
typeof impl === 'function'would have broken every service that is a function — there is a test registering exactly that and asserting it comes back untouched. Value providers are otherwise entirely unaffected.ServiceRegistry.get(name, caller?)defaults toKERNEL_SERVICE_CALLER, so core's direct.get()call sites keep working unchanged and are attributed to the kernel rather than to whichever plugin happens to be on the stack.@omadia/plugin-api0.1.0 → 1.0.0DevJobKind,DevJobStatus,DevJobDescriptor,DevJobCreateRequest,DevJobEventRecord,DevJobsAccessor,PluginContext.devJobsno longer exportedpermissions.devJobsstill installs and activates unchanged — regression-pinned inmanifestDevJobsLegacyKey.test.tsctx.services.get(name)throwsServiceNotDeclaredErrorfor undeclared names"<name>@<major>"to the manifest'srequires:. The dated allowlist below grandfathers everything currently shippedServicesAccessor.provide/.replacewidened toT | PerCallerFactory<T>ServicesAccessor(the kernel, and test doubles typed against it) sees itFull detail in
middleware/packages/plugin-api/CHANGELOG.md.Call-site audit — every
ctx.services.getin core and all ten plugin reposRead at
mainacrossmiddleware/packages/*and the standalone repos under~/sources/omadia-*. Service-name constants were resolved to their literal values. A fail-closed gate in one step would have broken 14 of the 20 plugins that callservices.get— hence the ramp.requires:@omadia/orchestratorattachmentBindings,audienceGrants,graphPool,nudgeProviders,privacyRedact,responseGuard,sessionBriefing,tigrisStore,turnHookRegistry,turnReceiptStore@omadia/channel-teams📦anthropicClient,embeddingClient,graphPool,graphTenantId,microsoft365.graph,tigrisStore,topicDetector,turnContext@omadia/plugin-plan-runnerknowledgeGraph,processMemory,turnHookRegistry@omadia/verifiergraphPool,odoo.client@omadia/orchestrator-extrasagentPriorities,graphPool@omadia/channel-telegram📦memoryStore,turnContext@omadia/agent-odoo-hr📦odoo.agentToolkit.hr,odoo.client@omadia/agent-confluence📦confluence.client,confluence.toolkit@omadia/agent-odoo-accounting📦odoo.agentToolkit.accounting@omadia/plugin-officeprivacyRedact@omadia/diagramsmemoryStore@omadia/ui-channelgraphTenantId@omadia/knowledge-graph-inmemoryturnContext@omadia/knowledge-graph-neonturnContext@omadia/memory-postgres@omadia/ui-orchestrator@omadia/integration-odoo📦@omadia/integration-confluence📦@omadia/plugin-web-searchharness-channel-sdkchatAgent@^1📦 = shipped through hub.omadia.ai from a standalone repo, i.e. cannot be fixed in this PR.
27 (plugin, capability) pairs are grandfathered in
LEGACY_UNDECLARED_SERVICE_GRANTS_2026_08_20— frozen, dated, and keyed per plugin id, not per name. It is closed in both directions:graphPoolstill throws;Both are tested. The object and its inner arrays are
Object.freezed, and the test asserts apushonto an inner array throws — freezing only the outer object would have left[...].push('anthropicClient')wide open.Follow-ups this audit surfaced
harness-plugin-privacy-guarddeclares the capabilityprivacy.redact@1but registers the service under the keyprivacyRedact. Capability name and service key disagree, so norequires:entry could grant it — the twoprivacyRedactallowlist rows cannot be retired until one side is renamed. This contradicts the documented invariant that capability names are service keys.requires:doubles as the activation dependency —resolveEligiblePluginsholds back a consumer whose requires are unmet. So a plugin that consumes a service optionally (harness-diagrams→memoryStore,verifier→graphPool) cannot declare it without risking non-activation when no provider is installed. This is the real reason most rows exist, and it wants anoptional_requiresmanifest field. A manifest-schema change is a bigger contract move that belongs in C4/C7, not here.middleware/src/index.tsnotes plugins should get the tenant id viactx.services.get('graphTenantId')— two plugins do, neither declares it.Counter-proof
Reverting the gate in
pluginContext.tsto the original one-liner:→ 9 of the 25 new tests fail, including
throws ServiceNotDeclaredError for a capability the manifest never mentions,invokes the factory with the kernel-known id, and all three allowlist-closure tests. Restoring the gate → 25/25 pass. The tests observe the gate, they do not merely coexist with it.Decoupling ratchet: 3296 → 3300 (hand-raised,
middleware/packages84 → 89)First measurement was +31. The avoidable 26 were reworded away, not excused:
devJobsas their example capability → renamed torepoGrants(−16);perCallerServicedocblock example useddevJobs/makeDevJobsFor→ neutral names (−2);middleware/testis back at its 1,030 baseline, unchanged.The residual +5 is the new
packages/plugin-api/CHANGELOG.md, all of it prose about code that left. A changelog recording thatDevJobDescriptorwas deleted has to name it, or a consumer grepping its own source finds nothing and the record is worthless. Three of the five lines are literal strings that cannot be reworded at all: a spec path (specs/470-dev-platform-plugin/dormant-capabilities.md), a test filename (manifestDevJobsLegacyKey.test.ts), and the future package name (@omadia/dev-platform-plugin-api) — the same class of justification as #557's path string. The other two are the removal heading and the six type names.Nothing in core references the dev platform as a result of this PR. Justification recorded consistently in all three places the README demands:
decoupling-baseline.json,README.md, andacceptance.md.Also
plan.md§4.2 still claimed theDevJob*types stay in@omadia/plugin-apias "a published, versioned contract that third-party plugins consume viactx.devJobs".implementation.md§2.5 had already flagged that this contradicts §4.1 and that two implementers would build incompatible package boundaries. Shipped code now settles it — struck through with a pointer to the resolution rather than silently deleted.Commands run
Node v22.22.3 via nvm, after merging
origin/main(5d2b5608).The 4 failures are environmental, and I proved it rather than asserting it. They are in
palaiaHybridRetrievalNeon.test.tsandneonDatasetFilterEscaping.test.ts, all failing withrelation "graph_nodes" / "datasets" does not exist— I stood up a throwaway Postgres on:55438with no migrated schema. I then stashed this branch's changes entirely and re-ran those two files against the unmodified tree: the same 4 failed. Not a regression from this PR. Zero gate-related failures anywhere in the suite.web-uiuntouched, so its typecheck was not run.package-lock.jsonmoves by exactly 2 lines, both intentional (the version bump and theharness-channel-apirepin) — no noise.Note for the C1 golden snapshot (#470 C1)
C1 has not landed on
mainyet —middleware/packages/plugin-api/still has no.d.tssnapshot, so there was nothing to update here. Its branchfeat/470-c1-plugin-api-golden-snapshotis in flight. This PR adds five exports to the plugin-api surface (perCallerService,isPerCallerService,resolvePerCallerService,ServiceNotDeclaredError, plus theServiceCaller/PerCallerFactorytypes) and widens twoServicesAccessormembers, so whichever of the two merges second must regenerate the snapshot with its--updatemode — deliberately, since the surface change is intended.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Cross-family review (Forge)
Adversarial review against the code by GPT-5.4 (
codex exec,reasoning_effort=high) — a deliberately non-Anthropic lineage, so the blind spots are not the ones the author and the Claude-family reviewers share. Every claim below was checked against source, not against the PR description. Three real defects were found and fixed ina6b259f2; the rest of the audit came back clean.Findings
@omadia/orchestratoritself.harness-orchestrator/src/plugin.ts:452resolvesllmProviderCatalogunconditionally near the top ofactivate(). As merged, the gate would throwServiceNotDeclaredErrorthere and the chat orchestrator would not activate. Also missing:nativeToolRegistry,processMemory,palaiaExcerpt,microsoft365.graph,nudgeStateStore,pluginCapabilities,installedPluginConfigReader,installedPluginToolsReadyReader; plus@omadia/orchestrator-extras→processMemory,@omadia/channel-teams×5,@omadia/channel-telegram→channelResolver, and new rows for@omadia/integration-odoo,@omadia/channel-discord,@omadia/channel-slack,@omadia/channel-whatsapp.test/pluginServiceGrantCoverage.test.ts.ctx.services.getpassed the caller.ctx.memory,ctx.entities,ctx.mcp,ctx.subAgents,ctx.llmandctx.eventsall calledserviceRegistry.get(name)with no caller and therefore silently gotKERNEL_SERVICE_CALLER— aperCallerServiceprovider would have handed the plugin the kernel-scoped implementation. That is precisely the self-attribution failure §2.2 exists to prevent.perCallerServicedocumented "one implementation per consuming plugin" but invoked the factory on every.get().orchestrator.ts:544documents a per-turnservices.get, so a per-caller provider would have allocated per turn.manifest: {}, butmemoryDeclaredreads permissions off the catalog entry's unparsedmanifestfield —ctx.memorywasundefinedno matter what the fixture declared, so the new attribution test asserted on nothing.provides:self-declaration is a grant.classifyServiceGrantreturnsself-providedpurely from the manifest; a plugin declaringprovides: ["graphPool@1"]is grantedget('graphPool')without ever callingprovide(), and unlikerequires:it pays no activation cost (resolveEligiblePluginsnever holds it back). The docblock's justification — "it holds the implementation anyway" — only holds if it actually registered it.PluginCatalog.load()documents "on ID collision with the built-in catalog the uploaded version wins". An uploaded plugin declaringidentity.id: "@omadia/orchestrator"therefore inherits that id's 19-name legacy allowlist (graphPool,tigrisStore,privacyRedact,turnReceiptStore,audienceGrants, …). Pre-existing collision behaviour, but the allowlist is what turns it into a service-grant escalation.parseCapabilityRef(raw).namediscards the range, sorequires: ["graphPool@^99"]grantsgraphPoolwhatever major is registered. Version is enforced at resolve time, not atgettime. Worth one sentence in the docblock.Clean — checked and found correct
requiresversion-range tricks: no bypass. You still have to declare the name, andrequirescarries a real activation cost.agentIdthatcreatePluginContextreceives, never a caller-supplied string. Closed per plugin and per name — verified by the existing tests, and theObject.freezetest pins it.ctxsurfaces: no surface hands a plugin the rawServiceRegistry. The kernel-mediated accessors have their own permission gates (finding chore(deps,ci): Bump docker/login-action from 3 to 4 #3 was about attribution, not about an ungated escalation).has()left ungated: correct — it hands over no capability.serviceRegistry.get(): unaffected, correctly attributed toKERNEL_SERVICE_CALLER. No non-plugin caller can throw, because onlyctx.services.getis gated.CapabilityResolverprovider-before-consumer graph is untouched; the gate is a read-time check inside an already-activated context, not a new resolution edge.plugin-api/CHANGELOG.md, all prose or unrewordable literals (a spec path, a test filename, a future package name, the removal heading, the type list).git diff origin/main...HEAD -- middleware/packagesshows zero new matches under any*/src/. Nothing re-acquires a dep.middleware/packages84 → 89 is honest.plugin-api1.0.0 pins:harness-channel-apiwas the only stale^0.1.0. Swept all 23 in-repopackage.jsonfiles and all sibling repos under~/sources/omadia-*— every other consumer already pins"*". No follow-up needed.The coverage test earned its place immediately
It is not a rubber stamp: run against the allowlist as merged it fails, and on the first green run it surfaced three gaps that neither the original audit nor the reviewer's own manual sweep had found —
@omadia/ui-orchestrator→agentToolInvoker,canvasOutputRegistry,deterministicActionRegistry. It resolves eachservices.getargument through the TypeScript checker rather than by grep, so const identifiers resolve and doc-comment mentions do not produce phantom call sites (that false-positive class is exactly what made the first audit's picture look complete).Mutation-proved: dropping the
llmProviderCatalogrow makes it fail withCounts, since the PR text and the code disagreed: the allowlist as merged held 37 pairs while its docblock said 27. This review added 26 more (9 on
@omadia/orchestrator, 3 on@omadia/ui-orchestrator, 5 on@omadia/channel-teams, 1 each on@omadia/orchestrator-extrasand@omadia/channel-telegram, and new rows for@omadia/integration-odoo,@omadia/channel-discord,@omadia/channel-slack,@omadia/channel-whatsapp), for a verified 63 across 19 plugin ids — which is what the docblock now says. Erratum: commita6b259f2's message says "21 pairs"; the correct figure is 26, as recorded here and in the code.C1 golden snapshot (#780) — landed mid-review, snapshot regenerated
#780 merged to
mainwhile this review was running (6f6956b9).origin/mainwas merged into this branch (clean, no conflicts —plugin-api/package.jsontook C1's new scripts alongside this PR's1.0.0), and the snapshot was regenerated deliberately, perpackages/plugin-api/README.md§ What to do when the check fails: read the diff first, decide what it means, then accept it.npm run api:check -w packages/plugin-apiwas red before the update, and the diff is exactly this PR's intended surface change and nothing else:Worth recording, because it changes how the version bump reads: there are no removals in this diff. The
DevJob*types were already gone fromplugin-api/src/onmainbefore either branch — all that survives is a tombstone comment, which the snapshot strips. By the README's own table this PR's surface change is therefore additive (MINOR-shaped). The0.1.0 → 1.0.0bump is still right, but it is the deliberate departure from0.x— where breaking changes were always permitted — not a break contained in this diff. The PR's "contract break, taken once" framing is about the package leaving0.x, and reading it as "this diff removes symbols" would be wrong.Verification
(All gates re-run after merging
origin/mainat6f6956b9; every number above is post-merge.)Verdict: MERGE once findings #6 and #7 are carried into #470 as follow-up issues. They are pre-existing design questions this PR surfaces rather than regressions it introduces, and neither is a reason to hold the boot-breaking fix.