Skip to content

feat(#578): credential keychain data model + store (phase 1/4) - #769

Merged
Weegy merged 6 commits into
mainfrom
feat/578-keychain
Aug 20, 2026
Merged

feat(#578): credential keychain data model + store (phase 1/4)#769
Weegy merged 6 commits into
mainfrom
feat/578-keychain

Conversation

@Weegy

@Weegy Weegy commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Phase 1/4 of #578 (credential keychain with grants and a broker). Data model
and durable store only — no route, no tool yet, per the phase cut in
docs/plans/phase4a-578-keychain-prompt-2026-08-20.md. Built inside its own
worktree/branch (feat/578-keychain) since a parallel session is working
#577 concurrently; surface separation from that prompt was followed (channel
SDK export appended at the end of index.ts, new files only under a
credentials/ namespace, a fresh migration number, nothing touched under
src/services/skill*, agentBuilder.ts, or src/routes/admin.ts).

What this delivers

  • packages/harness-channel-sdk/src/credentials.ts (new, additive):
    Credential, CredentialGrant, CredentialStore types,
    InMemoryCredentialStore, isGrantActive, validateNewGrantInput,
    fingerprintSecret. Barrel-exported at the end of
    packages/harness-channel-sdk/src/index.ts to avoid merge conflicts with
    the parallel Skills as scope-owned, shareable artifacts (grants, org promotion, git skill packs) #577 session's own additions there.
  • middleware/src/credentials/crypto.ts: AES-256-GCM seal/unseal, same
    envelope as the existing secret vault (fileVault.ts), reusing its
    resolveMasterKey — but under a different env var
    (CREDENTIAL_KEYCHAIN_KEY) and dev-key filename, so the keychain and the
    provider-secret vault are different trust domains that happen to share
    code, not a shared key.
  • middleware/src/credentials/postgresCredentialStore.ts: the durable
    CredentialStore, built the same way as PostgresGrantStore /
    PostgresAttachmentBindingStore — does not own the pool, and every method
    is allowed to throw rather than swallow a failure into an empty/absent
    result (a caller that turned "Postgres is unreachable" into "no active
    grant" would make an outage indistinguishable from an honest revocation).
  • middleware/src/credentials/credentialStoreFactory.ts: explicit
    Postgres-vs-in-memory choice (createCredentialStore(pool, key)), so the
    "vault no-pool case" the scoping prompt calls out by name is a stated
    decision with its own tests, not an implicit fallback.
  • middleware/migrations/0040_credentials.sql: credentials +
    credential_grants tables. 0038 is reserved for the Satellites epic
    (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 landed as turn_receipts (Persist per-turn audit receipts (receipt store) #757) while this branch was in
    flight; this PR uses 0040 (verified free against current origin/main
    after merging it in).

Design decision: a dedicated store, not a second GrantStore

The issue frames credential grants as flowing "through the existing
GrantStore mechanism, not alongside it." That reuse is real but happens at
the coarse layer: phase 2's broker will gate broker access itself behind
an ordinary capability (credential:broker:use) resolved the normal #575
way, through the same GrantStore every other capability goes through.

What GrantStore cannot express is the fine layer this PR owns: a
capability grant is a bare boolean with no expiry, no purpose, no
once-vs-standing distinction, and no per-grant revocation trail. The issue
asks for all four explicitly. Encoding that into a synthetic capability
string would either lose the metadata or smuggle it into the string itself
(which resolveCapabilities treats as opaque). A dedicated table is not a
parallel grant system — it's the metadata GrantStore's shape has no room
for. Full reasoning is in credentials.ts's module header and the
migration's comments.

Blast radius

  • New files only except one additive append to
    packages/harness-channel-sdk/src/index.ts (barrel export block appended
    at the end — pure addition, no existing export touched).
  • Nothing wired into middleware/src/index.ts (the composition root) yet —
    that's phase 2's job, when the broker actually consumes the store. Phase 1
    is inert in production: no route, no tool, no call site.
  • No changes to src/services/skill*, agentBuilder.ts, src/routes/admin.ts,
    or any existing route.
  • Migration is additive (CREATE TABLE IF NOT EXISTS, idempotent indexes,
    rollback comment) and does not touch any existing table.

Tests

59 tests, all green:

  • 49 unit tests (credentialKeychainDataModel.test.ts,
    credentialCrypto.test.ts, credentialStoreFactory.test.ts,
    postgresCredentialStoreFailure.test.ts) — no external dependency.
  • 10 tests against a real Postgres (postgresCredentialStore.pg.test.ts),
    run locally against pgvector/pgvector:pg17 (omadia-dev-postgres-1);
    skips cleanly with a logged reason when no test DB is configured
    (GRAPH_PG_TEST_URL / MEMORY_PG_TEST_URL / DATABASE_URL, issue Two scratch-container port traps: 55438 and 55439 are both claimed by hardcoded test defaults #572).
  • Verified the actual 0040_credentials.sql file applies cleanly
    (idempotent, BEGIN; ...; ROLLBACK;) against the real Postgres, not just
    the test file's inline schema copy.
  • npm run build (full ordered workspace build) and npx tsc --noEmit
    clean; eslint clean on all new/changed files; ran the existing
    test/audience*.test.ts suite (118 tests) unmodified to confirm the
    additive channel-sdk export causes no regression.

Mutation-check evidence

Rebuilt packages/harness-channel-sdk/dist between every channel-sdk
mutation (per the scoping prompt's warning — tests import the package via
its built dist, not source, so an unrebuilt mutant is invisible).

# Mutation Result
1 isGrantActive: expiry boundary <=< Caught immediately by the boundary test.
2 principalsMatch: dropped canonicalizePrincipalRef Survived on the first pass — both test principals were built via makePrincipal, which already canonicalises at construction, so the test never exercised the store's own canonicalisation. Fixed the test to use a raw, non-canonical Principal object literal (the shape a row reconstructed from a differently-spelled external source would take); re-ran — now caught. Reverted the mutation, kept the strengthened test.
3 PostgresCredentialStore.activeGrant: dropped the isGrantActive filter over query results Not caught by existing tests (no pg-level expiry/consumed-grant coverage existed) — added two new pg tests (expired-but-not-revoked grant, consumed-but-not-expired once grant) first, confirmed they pass on unmutated code, then re-applied the mutation: both new tests failed as expected.
4 PostgresCredentialStore.revokeGrant: dropped AND revoked_at IS NULL Caught by the existing idempotent-revoke pg test (true then expected false, got true).
5 PostgresCredentialStore.markGrantConsumed: dropped AND consumed_at IS NULL No pg-level idempotency coverage existed for this method (only the in-memory store had it) — added a pg test, confirmed green on unmutated code, then re-applied the mutation: caught.

All five mutations reverted; final state (this PR) is the code shown in the
diff, tests re-verified green.

Migration number

0040. origin/main at merge time has 0039 (turn_receipts, #757) as the
highest; 0038 is reserved for #746. If the parallel #577 session or another
PR lands 0040 first, I'll renumber before merge — flagging here per the
binding surface-separation rule.

Open questions for Marcel

  • Broker declaration fields (host/injectionScheme/allowedMethods/pathPrefixes)
    are stored now but unenforced until phase 2 — confirms the phase cut, not
    raising a concern, just flagging that a service credential created today
    carries no actual egress restriction until then.
  • CredentialInjectionScheme is 'bearer' | 'header' | 'basic-password' | 'query-param'
    — a guess at what phase 2 will need based on common API auth schemes. Happy
    to narrow/widen once the broker's actual requirements are clearer.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Weegy added 3 commits August 20, 2026 14:13
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".
Weegy added 3 commits August 20, 2026 14:59
0040 and 0041 were both claimed on main while this branch was in flight
(0040_privacy_miss_reports + 0040_skill_ownership_lifecycle, and
0041_receipt_hash_chain). The migrator tracks by full filename and applies
in lexicographic order, so a duplicate prefix would run fine — but the
series stays legible only if numbers keep meaning something. 0038 remains
reserved for #746. No code references the filename (verified by grep).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant