feat(#577): sharing via GrantStore + admin-gated promotion + cron write-guard (P3) - #771
Merged
Conversation
Adds the model layer for scope-owned, tamper-evident skills: - migrations/0040_skill_ownership_lifecycle.sql: owner_scope, lifecycle_status (draft/reviewed/published/archived), manifest_signature, manifest_signed_at columns on the existing skills table (0003). - src/services/skillLifecycle.ts: pure model — SkillOwnerScope (ScopeId restricted to personal/group/org), the lifecycle transition matrix, requiredCapabilities parsing from frontmatter (throws SkillManifestError with a field-naming message on malformed input, never silently drops — #690 guard), canonical manifest serialization + HMAC-SHA256 sign/verify. - src/services/skillLifecycleStore.ts: thin Postgres store over the new columns (assignPersonalOwner, transition), standalone from AgentGraphStore to keep this phase's surface to src/services/skill*. - test/skillLifecycle.test.ts (30 tests): exhaustive 4x4 transition matrix, byte-exact canonical manifest lock, capability-order independence, capability case-sensitivity (mirrors #724's role-key precedent), tamper detection, malformed-signature handling. - test/skillOwnershipLifecycleStore.pg.test.ts (6 tests, pg-gated): full draft->reviewed->published->archived flow against real Postgres, publish-gate rejection, re-signing on every transition.
Adds resolveSkillByName in src/services/skillResolver.ts: given a skill name and a requester's scope, pick the single winning skill row across personal -> shared -> team -> org, in that strict order (#577 Kernkonzept #4). Pure and synchronous, layered on P1's ScopeId/lifecycle model (#767) without touching GrantStore directly — membership/sharing are passed in pre-resolved (SkillResolutionContext), same seam P1 used for capability grants. Resolves by `name`, not `slug`: skills.slug is globally UNIQUE (0003), so no schema change is needed for two scopes to own same-named skills. Two invariants get explicit, separately-mutation-tested coverage: - precedence order (personal beats org even at equal names -- the 'non-empty result from the wrong level' danger named in the phase spec, parallel to the quorum='all' fail-open lesson from #726) - the lifecycle gate: only 'published' skills are eligible candidates, filtered BEFORE bucketing, so a draft at a higher-precedence level can never outrank a published skill lower down. A third case is guarded structurally: two candidates tying within one bucket return an explicit 'ambiguous' result (level + full candidate list) rather than an arbitrary pick -- same 'absence/uncertainty is a type' posture as resolveCapabilities (#575) and RoleSourceRegistry (#333). 15 tests in test/skillResolver.test.ts, all pure (no pg gate needed). Mutation-tested: reversing bucket precedence order fails 3 tests; dropping the published-status filter fails 3 tests.
…te-guard (P3) Adds the sharing/promotion/write-guard layer on top of P1 (skill ownership/lifecycle) and P2 (scope-ordered resolution): - src/services/skillLifecycle.ts: assertHumanActor / SkillAutomationWriteBlocked -- the enforced cron write-guard (#577 Kernkonzept #6). Checks ScopeId.kind === 'system' (scopeId.ts's own documented boundary: "no human is present in any of them" -- routine/schedule/conductor/ conductor-builder), so it needs no new taxonomy. Threaded as the first check in every mutating store method -- a blocked write never reaches the database (asserted in the pg tests, not just at the pure-function level). - src/services/skillLifecycleStore.ts: - assignPersonalOwner and transition now take an actorScope and call assertHumanActor before any query. - New promoteSkillOwnerScope(skillId, targetScope, opts): the ONLY way a skill reaches team/org ownership (#577 Kernkonzept #5 -- no direct creation there). Requires the skill be already 'published', re-signs the manifest at the NEW ownerScope + SAME status (promotion is a signature-changing event, since ownerScope is a signed field). Admin-gating itself (an authenticated-session check) is left to the route layer -- this method enforces the cron guard and the published-only invariant, nothing about roles. - src/services/skillSharing.ts: sharing = a grant over GrantStore (#575), not a parallel ACL (#577 Kernkonzept #5). Encodes "skill X is shared with principal Y" as a Capability string (skill:read:<id>) and resolveSharedSkillIds(principal, roles, grants) turns a resolved capability set back into the ReadonlySet<string> P2's resolveSkillByName needs for its 'shared' bucket. Denials subtract from grants (same rule as the audience floor). Deliberately does NOT collapse "unresolved" (partial role lookup) to empty at this layer -- SharedSkillIdsResult keeps the fact visible; toSharedSkillIdsSet is the explicit fail-closed adapter for callers who just want the resolver input. Consumes GrantStore/resolveCapabilities only -- no edits to grants.ts. Tests: 15 new pure tests (skillSharing.test.ts) covering direct grants, role-grant union, denial subtraction, unresolved-vs-empty, and the fail-closed adapter; 2 new pure tests for assertHumanActor (all 4 system origins blocked, every other ScopeId kind passes); 6 new pg-gated tests covering the cron guard on all three mutating methods and the full promotion flow (org target, team target, draft-refusal). Mutation-tested: disabling assertHumanActor fails 4 tests across both the pure and pg-gated suites; dropping the published-only gate on promoteSkillOwnerScope fails 1 pg-gated test. Both reverted; working tree clean afterward. Not in this PR: the admin-gated HTTP route itself. Wiring a session-authenticated Express route touches src/index.ts (the shared app-bootstrap file, ~3800 lines, high concurrent-edit traffic across this repo's parallel issue-harness sessions) and needs to correctly replicate the existing session/auth middleware chain -- a promotion endpoint with a subtly wrong auth check is a real security regression, not a place to move fast. The service-layer method (promoteSkillOwnerScope) is complete and fully tested; mounting it behind route + session auth is left as a follow-up (naturally lands with P4's admin UI, which needs a concrete endpoint contract anyway).
Add/add conflicts in three P1-origin files: main carries P1's squash (#767), this branch carries P1 plus P3's deliberate evolution of the same files (transition options gained the actor guard, assignPersonalOwner was reshaped). Verified before resolving: git log on main shows ONLY the #767 squash ever touched these paths, so every main-side-only line is a line P3 changed on purpose — resolved with the branch side.
This was referenced Aug 20, 2026
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.
Summary
Phase 3 of #577 (skills as scope-owned, shareable artifacts). Adds sharing via
GrantStore, admin-gated promotion, and the cron write-guard — service-layer only. The admin-gated HTTP route itself is deliberately deferred (see "Not in this PR").Base:
feat/577-skill-artifacts-p2(P2, #768), NOTmain. Stacked — merge order is #767 (P1) -> #768 (P2) -> this PR.What's in this PR
Cron write-guard (#577 Kernkonzept #6 — "automations may not modify skills, as an enforced guard, not a convention")
src/services/skillLifecycle.ts:assertHumanActor(actorScope)/SkillAutomationWriteBlocked. ChecksScopeId.kind === 'system'—scopeId.ts's own header already documents that kind as "no human is present in any of them" (routine/schedule/conductor/conductor-builder), so this needed no new taxonomy, just reusing Scope model with audience-floor permission intersection for shared rooms #575's existing one.src/services/skillLifecycleStore.ts:assignPersonalOwnerandtransitionnow take anactorScopeand call the guard before any query runs — a blocked write never reaches the database. Verified at both the pure-function level and the pg-gated level (the pg test asserts the row is unchanged after a rejected cron write, not just that the call threw).Admin-gated promotion (#577 Kernkonzept #5 — "no direct creation in org/team scope, only admin-gated promotion")
promoteSkillOwnerScope(skillId, targetScope, opts)onPgSkillOwnershipLifecycleStore— the only way a skill ever reachesgroup/orgownership (mirroringassignPersonalOwner, which only ever assignspersonal). Enforces:published— an unreviewed draft has no business reaching a wider audience;ownerScope, same status — sinceownerScopeis a signed field (Skills as scope-owned, shareable artifacts (grants, org promotion, git skill packs) #577 P1), promotion is a signature-changing event, not just a column update.Sharing = a grant, not a parallel ACL (#577 Kernkonzept #5)
src/services/skillSharing.ts. Encodes "skill X is shared with principal Y" as aCapabilitystring (skill:read:<skillId>) andresolveSharedSkillIds(principal, roles, grants)turns a resolved capability set (via Scope model with audience-floor permission intersection for shared rooms #575'sresolveCapabilities) back into theReadonlySet<string>that P2'sresolveSkillByName(SkillResolutionContext.sharedSkillIds) needs for itssharedbucket. Denials subtract from grants — same ruleaudienceFloor.tsapplies. ConsumesGrantStore/resolveCapabilitiesonly — zero edits togrants.ts.unresolvedrole lookup (a partial/thrown role source) to an empty set at this layer —SharedSkillIdsResultkeeps that fact visible, distinct from "nothing is actually shared".toSharedSkillIdsSetis the explicit fail-closed adapter for callers who just want the resolver's plain-set input. The module header explains why fail-closed is safe HERE specifically (it can only hide a skill from the resolver, never leak one), unlike on the audience-floor path where the same collapse would be dangerous.Test coverage
test/skillSharing.test.ts(15 pure tests): capability round-trip, direct-grant resolution, role-grant union, denial subtraction, unresolved-vs-empty distinction, the fail-closed adapter.test/skillLifecycle.test.ts(+2 tests):assertHumanActorpasses for every non-systemScopeIdkind, throwsSkillAutomationWriteBlocked(with the rightactorScopeattached) for all 4 system origins.test/skillOwnershipLifecycleStore.pg.test.ts(+6 pg-gated tests): cron guard on all three mutating methods (assignPersonalOwner,transition,promoteSkillOwnerScope) verifying the row is untouched after rejection; full promotion flow to an org home and to a team (group) home; promotion refused on a draft skill.Mutation evidence
assertHumanActornever throws (guard disabled)assignPersonalOwner,transition,promoteSkillOwnerScope)lifecycleStatus !== 'published'gate onpromoteSkillOwnerScopepromoteSkillOwnerScope refuses a draft skillBoth mutations reverted after confirming failure;
git diff --staton the mutated files matched only the legitimate P3 diff afterward (no residue).Full-suite regression
6962 tests / 6950 pass / 0 fail / 12 pre-existing skips, both
npm test(non-pg) and the pg-gated suites run against an ephemeral local Postgres.Blast radius
skillLifecycle.ts,skillLifecycleStore.ts, and their tests) — all additive/threading changes (new exports, a new required parameter on two existing methods, a new method). No behavior change to what P1 already shipped for the transitions it already covered.skillSharing.ts+ its test).@omadia/channel-sdkconsumed only (resolveCapabilities,GrantStore,Principal,RoleSourceRegistry) — zero edits togrants.ts, consistent with the binding surface separation.src/index.tschange — see below.Not in this PR (explicitly deferred, with reasoning)
src/index.ts(the shared app-bootstrap file, ~3800 lines, high concurrent-edit traffic across this repo's parallel issue-harness sessions) and correctly replicating the existingreq.session.omadia_user_idauth chain used elsewhere (e.g.routes/bulkPromotion.ts). A promotion endpoint with a subtly wrong auth check is a real security regression, not something to rush.promoteSkillOwnerScopeitself is complete and fully tested; mounting it behind a route + session auth is a natural fit for P4 (admin UI), which needs a concrete endpoint contract anyway.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.