feat(#778): wire #577 skill-promotion route + #578 credential-asks mount (W1) - #792
Merged
Conversation
…unt (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.
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 W1 (route mounts) of #778 — the wiring wave that connects the fully-built, fully-tested #577 (skills ownership/lifecycle) and #578 (credential keychain) service layers to their HTTP surfaces. Both #771 (P3) and #774 (Phase 3) deliberately stopped short of touching
middleware/src/index.tsto keep their own blast radius to new files only; this PR is that debt.Base:
origin/main. No stacking — this is the first of the four #778 phases (W1 routes → W2 agent tools → W3 admin UIs → W4 ask-notification/gateway-caller).What's wired
Skill promotion (#577 P3 —
promoteSkillOwnerScope)POST /api/v1/admin/skills/:skillId/promote(src/routes/skillPromotion.ts) — the only path a skill ever reachesgroup/orgownership.routes/bulkPromotion.ts'sreq.session.omadia_user_idchain exactly — same 401 shape (auth.required), same single-tenant "every authenticated session is an operator" posture. This is the auth check feat(#577): sharing via GrantStore + admin-gated promotion + cron write-guard (P3) #771's own PR description explicitly deferred rather than rush.src/services/skillManifestSigningKey.tsresolves (generate-once, vault-persisted) the HMAC key the store re-signs a promoted skill's manifest with — mirrorsauth/sessionSigningKey.ts's pattern exactly, but under its own vault scope (core:skills) since a data-integrity key is a different trust domain than an auth-token key (same reasoningcredentials/crypto.tsgives for keeping the credential-keychain master key separate from the provider-secret vault's).graphPoolis available (same gatebulkPromotionServicealready uses — the store needs a real Postgres pool).Credential asks (#578 Phase 3)
routes/credentialAsks.ts(feat(#578): keychain-asks — request, owner-approval, grant (phase 3/4) #774) at/api/v1/admin/credential-asks, behindrequireAuth.CredentialAskStorenor theCredentialStorebehind it was constructed anywhere inindex.tsbefore this PR (verified: zero references). This PR also resolves the credential-keychain's master key (CREDENTIAL_KEYCHAIN_KEYenv / dev-file fallback, viaresolveCredentialMasterKey— built by Credential keychain with grants and a broker that keeps secrets away from the agent #578 P1, never called until now) and builds the store via the existingcredentialStoreFactory.ts(Postgres whengraphPoolis configured, in-memory otherwise).Wiring tests (the actual point of #778)
A router that is fully unit/route-tested standalone and never mounted passes every one of those tests — that is exactly how both surfaces sat unreachable for a full phase.
index.tsrunsmain()unconditionally at import time (DB pools, mDNS,app.listen) and is not designed to be booted from a test — confirmed no test in this repo does that.test/778RouteMounts.wiring.test.ts— asserts the live (comment-stripped) source ofindex.tscontains bothapp.use(...)mount lines with the correct path +requireAuth+ router-factory call. A regression-guard sub-test proves the comment-stripping step itself works (so a commented-out mount can't false-positive).test/skillPromotionRoute.test.ts— realapp.listen(0)+fetchbehavioral coverage: 401 with no session, 400 on a malformed body, 200 promoting toorg/groupwith theactorScopebuilt from the session, 404/409/403 error-code mapping.SkillPromotionRouteDeps.storeis narrowed toPick<PgSkillOwnershipLifecycleStore, 'promoteSkillOwnerScope'>so the test uses a fake store instead of a realPool.Mutation evidence
Commented out the live skill-promotion
app.use(...)line inindex.ts(regex against the real file, not a copy) and reran778RouteMounts.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 diffafter revert showed zero residue.dist/rebuilt vianpm run buildbefore and after the mutation.Full-suite regression
7417 tests / 7405 pass / 0 fail / 12 pre-existing skips (
npm test, non-pg), run after mergingorigin/main(#782,42e91c8d) into this branch.Migration
None — reuses existing tables (
skillsfrom0040,credentials/credential_asksfrom0042/0043).0045(publish_versions) and0046(plugin_public_path_grants, landed via #782 while this PR was in flight) are both taken — next free number is0047for any following #778 phase that needs one.#783 compatibility (
ctx.services.getgrant gate)Checked explicitly per the task brief:
index.tsnever goes throughctx.services.getfor anything this PR touches — it constructsPgSkillOwnershipLifecycleStoreand the credential stores directly, the same wayaudienceGrantStore/bulkPromotionServicealready do at the composition-root level. No plugin manifest changes needed for W1. (W2 — agent tools registered throughharness-orchestrator/src/plugin.ts— will need newrequires:entries in that plugin's manifest, since new service names it doesn't already declare are NOT on #783's legacy allowlist; see the manifest.)Blast radius
middleware/src/index.ts: additive only — 2 new import blocks, 2 newconstkey resolutions placed next to the existingsessionSigningKeyresolution, 2 newapp.use(...)mounts inserted directly after the existing bulk-promotion mount block. No reordering of existing code.skillLifecycle.ts,skillLifecycleStore.ts,credentialAsks.ts,asks.ts,postgresCredentialAskStore.ts,credentialStoreFactory.ts) — consumed only, per the task brief's "consume, don't edit" constraint.SkillLifecycleTransitionRejected/SkillAutomationWriteBlockedare re-exported nowhere new; the route imports them directly from the existing service modules.Not in this PR (tracked as later #778 phases)
credential_ask/ broker agent tools behind operator flags (patternsandbox_execute_enabled), registered throughharness-orchestrator/src/plugin.ts's existing native-tool-registry seam. Requires new manifestrequires:entries for whatever service names the plugin resolves (credentialAskStore, broker access) — these are NOT on feat(#470): grant-gate ctx.services.get and cut plugin-api 1.0.0 (C2b) #783'sLEGACY_UNDECLARED_SERVICE_GRANTS_2026_08_20allowlist for@omadia/orchestrator, so an undeclaredctx.services.get(...)call would throwServiceNotDeclaredErrorimmediately. Also requiresindex.tstoctx.services.provide(...)the broker/ask store as services in the first place (today nothing publishes them) — that provide-side wiring is itself new scope, not something this PR's direct-construction approach already covers.i18n:check+ parity suite +i18n:literalsratchet, Lume tokens). Not attempted this session — sizeable, independent scope from the route-mount work here.createGrantCheckedResolveTarget(the anonymous v1 gateway strips identity by design — not to be softened). Explicitly "only if confident" per the task brief; deferred to keep this PR's blast radius to what's been fully proven.Open questions for Marcel
skill:read:<skillId>naming,basic-password= fulluser:pass) don't affect this phase's surface.provide()d fromindex.tsunconditionally, or only when the credential keychain has a real Postgres pool (mirroring howbulkPromotionServiceis conditional ongraphPool)? The in-memory fallback works within one process, so I lean toward "always provide, let the operator flag gate whether a tool uses it" — flagging for confirmation before W2 starts.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.