feat(#578): keychain-asks — request, owner-approval, grant (phase 3/4) by Weegy · Pull Request #774 · byte5ai/omadia

Weegy · 2026-08-20T12:50:16Z

Summary

Phase 3/4 of #578 (credential keychain with grants and a broker). The
keychain-asks lifecycle: an agent requests a personal credential from
its owner; approval atomically creates the grant. Denial or expiry
resolves the ask without ever touching credential_grants.

Base: feat/578-keychain-p2-broker (phase 2/4, PR #772), not main.
Stacked per the phase cut in
docs/plans/phase4a-578-keychain-prompt-2026-08-20.md — merge order is
#769 → #772 → this PR.

The Conductor-await investigation the scoping prompt asked for

Before building anything new, I checked whether Conductor's existing
await mechanism (middleware/src/conductor/awaitStore.ts — TTL'd
pending human actions, principal_kind/principal_ref, atomic
claim-then-act) "carries" this.

It carries the pattern, not the table. conductor_awaits is FK'd
NOT NULL to run_id/step_id — every row belongs to a workflow run
and step. A keychain ask has neither; it's a standalone request an
agent makes mid-conversation, not a step inside a Conductor workflow.
Reusing the table would mean inventing a fake run and step for every
ask — not reuse, a disguise. What DOES carry over, deliberately
mirrored in the new table/store:

a principal as (kind, ref), never a formatted string;
TTL evaluated against a caller-supplied now, never the row's own
now() — same Flaky: the resume/reaper conformance test races the wall clock (60ms budget) #709/fix(#709): anchor the resume/reaper test on the store's own clock #710 anchor discipline isGrantActive (phase 1)
already follows;
"claim, then act": resolving an ask is one atomic UPDATE ... WHERE status = 'pending', exactly like ConductorAwaitStore.close and
phase 1's markGrantConsumed — same defence against two concurrent
approvals producing two grants.

What this delivers

migrations/0041_credential_asks.sql: the credential_asks
table. 0038 reserved (Epic: Satellites — outbound-only edge nodes (Raspberry Pi class) that pair with a device code, run selected agents, and bridge internal-only systems securely into the main omadia #746), 0039 turn_receipts (Persist per-turn audit receipts (receipt store) #757), 0040
credentials (Credential keychain with grants and a broker that keeps secrets away from the agent #578 phase 1, PR feat(#578): credential keychain data model + store (phase 1/4) #769); this series continues at
0041.
src/credentials/asks.ts: CredentialAsk/CredentialAskStore
types, InMemoryCredentialAskStore, validateNewAskInput,
isAskActionable, assertAskableCredential. Asks only apply to
personal credentials — a service credential has no single owner
to ask; it's reached through the broker (phase 2) under an
administratively-issued grant.
src/credentials/postgresCredentialAskStore.ts: the durable
store. approve() is the one method in the whole feature that opens
its own transaction — claiming the ask and creating its grant must
not be observable half-done, because an ask marked approved with
no grant behind it is a promise the broker cannot keep.
src/routes/credentialAsks.ts: create / list-pending-for-owner /
list-mine / approve / deny / cancel. Not mounted into
middleware/src/index.ts — same deliberate choice phases 1 and 2
made (see their PR descriptions): keeps this phase's blast radius to
new files only, avoids a merge collision with the parallel Skills as scope-owned, shareable artifacts (grants, org promotion, git skill packs) #577
session's own index.ts changes. A future integration step mounts
it behind requireAuth, like every other /api/v1/admin/* router
(audience/routes.ts is the precedent).

What's deliberately NOT in this phase

Routing an ask into the requester's or owner's live chat thread.
The issue's concept describes an ask as "TTL'd, routed back to the
requesting thread" — that live-notification half requires touching the
orchestrator/channel messaging surface, which the binding surface
separation for this issue keeps out of reach (agentBuilder.ts,
existing skill routes). This PR delivers the complete request → approve
/ deny → grant lifecycle as a tested store + HTTP route; today, an
owner discovers a pending ask by listing it (GET /pending?owner=...),
not by being pinged in their chat. Wiring that notification is a
follow-up — flagged explicitly in asks.ts's module header and again
here so it isn't mistaken for an oversight.

Blast radius

New files only (8 files). Nothing existing was touched.
No changes to src/services/skill*, agentBuilder.ts,
src/routes/admin.ts, middleware/src/index.ts, or any existing
route.
Migration is additive; credential_asks.grant_id FKs to
credential_grants(id) (phase 1's table) with the default RESTRICT
— a grant a resolved ask points to cannot be deleted out from under
the audit trail.

Tests

37 tests: 31 unit/route (no external dependency) + 6 against a real
Postgres.

test/credentialAsks.test.ts — 19 tests: validateNewAskInput,
isAskActionable, assertAskableCredential,
InMemoryCredentialAskStore's full lifecycle including double-resolve
and expired-ask handling.
test/postgresCredentialAskStore.pg.test.ts — 6 tests against a real
Postgres (pgvector/pgvector:pg17), including the race: two
genuinely concurrent connections calling approve() on the same ask
— exactly one wins, exactly one credential_grants row exists.
Skips cleanly (issue Two scratch-container port traps: 55438 and 55439 are both claimed by hardcoded test defaults #572) with no test DB configured.
test/postgresCredentialAskStoreFailure.test.ts — 5 tests, including
one with a hand-built fake PoolClient that lets the atomic claim
succeed but makes the grant INSERT fail, asserting ROLLBACK was
actually issued (not just that the promise rejected).
test/credentialAskRoutes.test.ts — 7 tests against a real Express
app on an ephemeral port (adminProvidersRoute.test.ts's pattern),
covering the full HTTP lifecycle including the 409-on-already-resolved
and TTL-clamp cases.

Also verified the actual 0041_credential_asks.sql file applies
cleanly (BEGIN; ...; ROLLBACK;) against a real Postgres.

Full local verification on top of #772's latest commit:

npm run build, npm run lint, npm run typecheck: clean.
npm run typecheck:test (issue npm run typecheck never typechecks middleware/test/ #573 ratchet): "406 known error(s),
no regressions" — same baseline as feat(#578): credential keychain data model + store (phase 1/4) #769/feat(#578): credential broker — the egress-stamping layer (phase 2/4) #772.
npm run test (full suite, ~7300 tests with a Postgres URL set):
7298 passed. 18 failures, all in ConductorWebhookSubscriptionStore (pg), none related to this PR — confirmed to be schema drift on my
long-lived shared local dev Postgres container (that table is
missing a run_id column a later, unrelated migration added; my
only DROP TABLE commands during this work targeted
credentials/credential_grants with no CASCADE, which Postgres
would have refused had anything else depended on them). Not expected
to reproduce against CI's fresh Postgres container; flagging in case
it's a real, separately-tracked issue rather than purely local state.

Mutation-check evidence

#	Mutation	Result
1	`approve()`'s atomic claim: dropped `AND ask_expires_at > $now`	Caught by the TTL test — an ask past its deadline would otherwise still approve.
2	`approve()`'s atomic claim: dropped `AND status = 'pending'`	Caught DIRECTLY by the concurrent-approval race test against real Postgres — both connections won, two grant rows existed.
3	Removed the `ROLLBACK` call on the second-write (grant insert) failure path	Caught by the fake-`PoolClient` test asserting `ROLLBACK` was actually issued.
4	Route: removed the "`once` mode requires `requestedGrantExpiresAt`" 400 guard	Caught — the store's own `validateNewAskInput` still refused it (defense in depth), but with a different error code than the route-level guard promises, which the test asserts precisely.

All four mutations reverted; final state (this PR) is the code shown
in the diff, full suite re-verified green afterward.

Open questions for Marcel

Live notification is out of scope here (see above) — confirming
that's expected for this phase, not a gap to fill before merge.
The route is unmounted, same as phases 1/2's stores/broker — should
a follow-up PR wire all three phases' surfaces into
middleware/src/index.ts + an agent-callable tool together, once
Skills as scope-owned, shareable artifacts (grants, org promotion, git skill packs) #577's parallel work has landed and the merge-conflict risk in
index.ts/agentBuilder.ts is lower?
ConductorWebhookSubscriptionStore (pg) local-only failure above —
worth a quick check on a clean CI run to confirm it really is just
container staleness on my machine and not a real, separately-tracked
regression.

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

Data model and durable store for the credential keychain (#578), the credential-side counterpart to Privacy Shield: encrypted, fingerprinted credentials owned by a principal, and grants (audience scope, once vs standing, purpose, expiry, revocation) that let a principal use one. No route, no tool yet — phase 1 is data model + storage only, per the phase cut in docs/plans/phase4a-578-keychain-prompt-2026-08-20.md. - packages/harness-channel-sdk/src/credentials.ts (NEW, additive): Credential / CredentialGrant / CredentialStore types, InMemoryCredentialStore, isGrantActive, validateNewGrantInput, fingerprintSecret. Barrel export appended to the END of index.ts to avoid merge conflicts with the parallel #577 session. - src/credentials/crypto.ts: AES-256-GCM seal/unseal, reusing fileVault's resolveMasterKey under a DIFFERENT env var (CREDENTIAL_KEYCHAIN_KEY) and dev-key file — separate trust domain from the provider-secret vault, sharing only the key-resolution code. - src/credentials/postgresCredentialStore.ts: durable CredentialStore, built the same way as PostgresGrantStore / PostgresAttachmentBindingStore (does not own the pool, throws rather than swallows a failure). - src/credentials/credentialStoreFactory.ts: explicit Postgres-vs-in-memory choice, so the vault no-pool case is a stated decision, not an implicit fallback. - migrations/0040_credentials.sql: credentials + credential_grants tables. 0038 is reserved (#746); 0039 is turn_receipts (#757). Why a dedicated store instead of a second GrantStore: a credential grant needs expiry/purpose/once-vs-standing metadata GrantStore's capability-string model cannot express. The coarse layer (does this principal have any right to reach the broker at all) still reuses the existing GrantStore/resolveCapabilities mechanism in phase 2; this table is the fine layer underneath it. Rationale is written out in credentials.ts's module header and the migration's own comments. Tests: 59 (49 unit + 10 against a real Postgres, skips cleanly with no test DB configured). Mutation-tested: isGrantActive's expiry boundary, principal-canonicalisation in activeGrant (an earlier version of this test passed even with canonicalisation removed, because both principals were built via makePrincipal which already canonicalises — fixed to use a raw, non-canonical Principal literal so the store's own canonicalisation is what's under test), the activeGrant active-filter, and the revokeGrant/markGrantConsumed idempotency guards. Every mutant was caught after the fix; dist was rebuilt between channel-sdk mutation runs.

headerName only made sense for the "header" injection scheme. Phase 2 (the broker) also needs a query-parameter NAME for the "query-param" scheme, which had nowhere to go under the old field. Renamed before phase 2 lands on top of this, rather than working around the gap there: "header" uses it as the header name, "query-param" as the parameter name, "bearer"/"basic-password" ignore it (the whole secret IS the value). No behavioural change for "bearer" (the only scheme phase 1's own tests exercise) — this is a rename, not new logic.

`fsp.readdir(dir).catch(() => [])` inferred the catch handler's return as `never[]`, which the test/tsconfig.json project (checked separately from src/ per #573) flagged as a new, previously-unbaselined error — `npm run typecheck` (src-only) never saw it, only `npm run typecheck:test` does, and that is what CI's "Typecheck (test + scripts trees, ratchet)" step runs. Annotated the handler's return type explicitly. CI failed on this before the Test steps even ran (they were skipped, not green) — verified locally with `npm run typecheck:test`, now reporting "406 known error(s), no regressions".

The broker: an agent names a `service` credential and describes a request (host, method, path). The broker decides whether the calling principal, right now, may use that credential for exactly that request, and if so decrypts the secret and stamps it onto the outbound call itself. The caller receives only the response, never the secret, on either the success or the failure path. - src/credentials/requestMatching.ts: normalizeHost/Method, normalizePathForMatch (traversal-safe via path.posix.normalize, which clamps `..` at the root), matchPath (boundary-safe prefix matching, not a bare startsWith). - src/credentials/brokerMetrics.ts: counters built the same shape as securityScreenMetrics.ts (#749) per the scoping prompt's instruction to reuse that pattern — count every outcome, consecutive-denial streak alert. - src/credentials/broker.ts: CredentialBroker. Fail-closed on every check (unknown/revoked credential, wrong kind, no active grant, host/method/path mismatch, malformed declaration, store outage). Every denial is counted and (when onAudit is wired) audited via BrokerAuditEvent — fingerprint only, never the secret. Security-relevant design points, each with its own test: - BrokerRequestDescriptor.host is compared against the credential's OWN declared host before dispatch — the SSRF-prevention check: an agent naming the right credential but the wrong destination denies with host-not-allowed rather than exfiltrating to an attacker host. - Path prefix matching normalises both sides and requires a segment boundary, closing both the traversal hole (/api/../admin -> /admin) and the naive-startsWith hole (/v1 matching /v1extra). - All non-mutating checks run BEFORE a `once` grant is consumed, so a request that was always going to be refused never burns the caller's single-use permission. The atomic markGrantConsumed call is the last gate before dispatch; its own false-return (lost a race to a concurrent use of the same grant) is a fail-closed denial (grant-consumed-concurrently), covered by a test that simulates the race via a store wrapper. - A caller-supplied header cannot override or discover the injected Authorization/header/query-param value. - query-param injection URL-encodes both the key and the secret. Tests: 87 (59 unit covering requestMatching/brokerMetrics/broker in isolation and via InMemoryCredentialStore, no external dependency). Mutation-tested: matchPath's boundary check, normalizePathForMatch's traversal clamp, the host-not-allowed check (SSRF guard), the check ordering that protects a once grant from being burned by a host/path-rejected request, query-param URL-encoding, and the deny-path metrics counter. Every mutant was caught; all reverted.

The full ask lifecycle: an agent requests a `personal` credential from its owner; approval atomically creates the grant. Denial or expiry resolves the ask without ever touching credential_grants. Investigated whether Conductor's await mechanism (middleware/src/conductor/awaitStore.ts) carries this before building anything new, per the scoping prompt. It carries the PATTERN (kind/ref principal, TTL evaluated against a caller-supplied `now`, atomic claim-then-act) but not the table: conductor_awaits is FK'd NOT NULL to a workflow run and step, and a keychain ask is neither — it is a standalone request, not a step inside a Conductor workflow. Mirrored the pattern in a dedicated table instead of forcing a fake run/step onto every ask. - migrations/0041_credential_asks.sql: credential_asks table. 0038 reserved (#746), 0039 turn_receipts (#757), 0040 credentials (#578 phase 1); this series continues at 0041. - src/credentials/asks.ts: CredentialAsk / CredentialAskStore types, InMemoryCredentialAskStore. Asks only apply to `personal` credentials (assertAskableCredential) — a `service` credential has no single owner to ask; it is reached through the broker (phase 2) under an administratively-issued grant. - src/credentials/postgresCredentialAskStore.ts: the durable store. approve() is the one method in this whole feature that opens its own transaction: claiming the ask and creating its grant must not be observable half-done, because an ask marked "approved" with no grant behind it is a promise the broker cannot keep. The claim itself is a single atomic UPDATE ... WHERE status = 'pending' AND ask_expires_at > $now, closing the #709/#710 race the same way markGrantConsumed/ConductorAwaitStore.close already do — verified against a REAL Postgres with two genuinely concurrent connections racing approve() on the same ask (exactly one wins, exactly one grant row exists). - src/routes/credentialAsks.ts: create / list-pending-for-owner / list-mine / approve / deny / cancel. Not mounted into middleware/src/index.ts yet — same deliberate choice phases 1 and 2 made, to keep this phase's blast radius to new files only and avoid a merge collision with the parallel #577 session's own index.ts changes. A future integration step mounts it behind requireAuth like every other /api/v1/admin/* router. Deliberately NOT in this phase: routing an ask into the requester's or owner's live chat thread. That requires touching the orchestrator/channel messaging surface, which the binding surface separation for this issue keeps out of reach (agentBuilder.ts, existing skill routes). An owner currently discovers a pending ask by listing it, not by being pinged — wiring that notification is a follow-up, stated explicitly in asks.ts's module header and the PR body. Tests: 43 (37 unit/route + 6 against a real Postgres including the concurrency race). Mutation-tested: the TTL check in the atomic claim, the pending-status guard that makes the claim exclusive (caught directly by the concurrent-approval race test against real Postgres), the rollback-on-second-write-failure path (a fake PoolClient asserts ROLLBACK actually ran), and the route's once-mode validation guard. Every mutant was caught; all reverted. Full local verification on top of phase 1 + phase 2: npm run build, npm run lint, npm run typecheck, npm run typecheck:test (406 known errors, no regressions — same baseline), and the full npm run test suite. One unrelated pre-existing failure observed locally (ConductorWebhookSubscriptionStore (pg), 18 sub-tests) — confirmed to be schema drift on the long-lived shared local dev Postgres container (that table is missing a run_id column a later migration added; my only DROP TABLE commands during this work targeted credentials/ credential_grants with no CASCADE, which would have failed had anything else depended on them) — not caused by this branch and not expected to reproduce against CI's fresh Postgres container.

0041 was claimed on main by 0041_receipt_hash_chain while this stack was in flight; the sibling credentials migration moves 0040 -> 0042 on the P1 branch for the same reason. The migrator tracks by full filename and sorts lexicographically, so this is hygiene, not correctness — but numbers only help if they stay unique. 0038 remains reserved for #746. No code references the filename (verified by grep).

…asks

This branch was stacked on P1, which carried 0040_credentials.sql; on main the same migration landed renamed as 0042_credentials.sql (0040 had been claimed twice while the stack was in flight). After merging main in, both filenames existed with byte-identical content — on a fresh install the migrator (tracked by filename) would have applied the same DDL twice, and the second CREATE TABLE would abort the run. Keep the canonical 0042.

…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.

Weegy added 6 commits August 20, 2026 14:13

Weegy changed the base branch from feat/578-keychain-p2-broker to main August 20, 2026 13:40

Weegy added 2 commits August 20, 2026 15:40

Merge remote-tracking branch 'origin/main' into feat/578-keychain-p3-…

eb1088d

…asks

Weegy merged commit 6c9081f into main Aug 20, 2026
14 of 15 checks passed

Weegy deleted the feat/578-keychain-p3-asks branch August 20, 2026 13:55

Navigation Menu

Uh oh!

feat(#578): keychain-asks — request, owner-approval, grant (phase 3/4) - #774

feat(#578): keychain-asks — request, owner-approval, grant (phase 3/4)#774
Weegy merged 8 commits into
mainfrom
feat/578-keychain-p3-asks

Weegy commented Aug 20, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Weegy commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

The Conductor-await investigation the scoping prompt asked for

What this delivers

What's deliberately NOT in this phase

Blast radius

Tests

Mutation-check evidence

Open questions for Marcel

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Weegy commented Aug 20, 2026 •

edited

Loading