feat(channel-api): public API channel with server-to-server API keys + scopes (#438, #439) - #549
Merged
Conversation
New built-in channel package @omadia/channel-api exposes POST /api/public/v1/chat — a documented, self-authenticating HTTP entry point external systems can drive without a channel adapter or the operator UI. Streams the same NDJSON event framing as /chat/stream and dispatches through CoreApi.handleTurnStream, so PII masking (privacy-guard), memory, and the knowledge graph apply exactly as they do for every other channel. Credential model (locked design decision on the issue): each API key is its own identity (ChannelUserRef kind:'custom', id:'key:<id>'), not a delegate for a human end-user. Full v1 security posture: - API keys are vault-backed (this plugin's own ctx.secrets namespace, no DB migration) and verified with crypto.timingSafeEqual against a sha256 hash; the plaintext is returned exactly once, at creation. - Per-key configurable rate limits (fixed-window token bucket, 429 on overage). - Explicit revoke endpoint (POST /api/public/v1/admin/keys/:id/revoke) that fails the next request immediately. - Usage audit log (who/what/when) recorded on every authenticated call. Key lifecycle (GET/POST /api/public/v1/admin/keys, revoke) is mounted under the same /api/public/v1 prefix but deliberately NOT added to publicPaths.ts's exemption list — only .../chat is public. Key management stays behind the normal operator session cookie. Tests: unit coverage for the token/store/rate-limiter/audit-log modules, router-level wiring (auth/rate-limit/revoke/audit/NDJSON framing), a real-orchestrator + real-privacy-guard integration test that asserts masking actually ran (not just that the plugin was called), and a publicPaths exemption test. Closes #438
Addresses two blocking findings from adversarial review of the
@omadia/channel-api public-API-plugin branch:
1. Cross-key session leak: CoreApi derives its orchestrator scope as
`${channelId}::${conversationId}`, and channelId is fixed per plugin
instance (shared by every API key). The router forwarded the
caller-supplied conversationId unchanged, so two different API keys
posting with the same conversationId collided on the exact same
core-side scope and could recall each other's transcript/context.
Fixed by namespacing the internal conversationId with the verified
key's id (`${key.id}:${callerConversationId}`) before it reaches
CoreApi — a per-key UUID that is never caller-controlled, so the
collision is now structurally impossible.
2. Audit log guarantee was false: only the success path recorded an
entry, with `status: 'ok'` written optimistically before dispatch —
a missing/invalid key, an over-quota call, or an orchestrator throw
was either never audited or misrecorded as 'ok'. Every call that
gets past key verification (i.e. every authenticated call) is now
audited exactly once, with a status reflecting the real outcome:
'ok' | 'rate_limited' | 'invalid_request' | 'error'.
Also updates docs/security-architecture.md with a new section covering
this ingress's credential model, vault-backed hash-only-at-rest
storage, constant-time verification, fixed-window per-key rate
limiting, and the revocation flow (AGENTS.md requires this doc for any
credential-handling change).
New tests cover both fixes: two keys sharing an identical
caller-supplied conversationId now produce distinct internal scopes,
and every authenticated outcome (ok / rate_limited / invalid_request /
error) produces exactly one accurately-statused audit entry while
unauthenticated calls produce none.
A third finding from the same review round claimed the issue requires
salted key hashes and that no key-management UI exists. Verified
against the actual issue #438 body and all comments via `gh api`
(REST): salting is never mentioned anywhere in the issue, and is not a
meaningful mitigation for a 256-bit random high-entropy key (no
dictionary/rainbow-table surface for a salt to defend against — same
reasoning as GitHub PATs and Stripe API keys). adminKeysRouter.ts
already provides key create/list/revoke, contradicting the "no
key-management UI" claim outright. Neither is implemented here.
Documents the public HTTP API surface added by #438 for external integrators: the single public route (POST /api/public/v1/chat), the bearer API-key auth model (with a pointer to the operator-only, session-gated admin key-management endpoints), the NDJSON-only streaming response shape (explicitly no non-streaming variant in v1), rate limiting (429) and invalid/revoked-key behavior (401), and a minimal curl example. Matches this repo's existing harness-* package README conventions. No code changes.
…xes (#438) Addresses adversarial-review findings on the public API plugin branch: - chatRouter.ts recorded audit status 'ok' whenever the forward-events loop over deps.core.handleTurnStream completed without throwing — but the orchestrator (and the verifier wrapper, in verifier mode) can yield an in-band {type:'error', message} event on the already-open stream WITHOUT throwing, same bug class as issue #403. Now tracks whether an error-type event was forwarded during iteration and audits accordingly. Added a regression test with a scripted async iterable that yields text_delta then error (no throw), asserting the audit status is 'error'. - README's event table claimed agent_bound is emitted on this route; it isn't — that event is synthesized by the kernel's own /api/chat/stream route handler, not by CoreApi.handleTurnStream (what this plugin calls directly). Removed the row and added one for the verifier-mode {type:'verifier'} event that can follow done. - docs/security-architecture.md § 8 and the README's rate-limiting section now state explicitly that the rate limiter is in-memory and per-process — resets on restart, not shared across replicas/instances — as an accepted v1 trade-off (same bar as httpAccessor.ts's TokenBucket). No code change; a distributed limiter was explicitly declined for v1. - harness-channel-api's peerDependencies on @omadia/channel-sdk and @omadia/plugin-api were open '*' ranges, violating CONTRIBUTING.md's dependency-hardening policy. Pinned both to ^0.1.0, matching their current in-workspace versions. Note: omadia-ui-channel and harness-channel-sdk (and several other packages) already violate the same open-peer-range policy with their own '*' deps. Pre-existing, out of scope for this PR — worth a maintainer follow-up issue.
…session check (#438) The admin key-lifecycle router (/api/public/v1/admin/keys) was completely unauthenticated: mounting via core.registerRouter only gates on active/inactive, never auth, and the doc comment claiming protection from merely not being in publicPaths.ts was false. Any anonymous caller could mint, list, or revoke API keys. Fixed at the kernel level per repo-owner direction, since no plugin previously had a way to check the operator session cookie: - PluginContext gains an optional ctx.operatorAuth (OperatorAuthAccessor), matching the existing oauthTokens/flows optionality convention. - The kernel implementation (operatorAuthAccessor.ts) wraps evaluateSessionToken, extracted out of requireAuth.ts so there is exactly one code path deciding session validity, used by both the Express middleware and the plugin accessor. - Threaded through createPluginContext and into every plugin runtime (ToolPluginRuntime, DynamicAgentRuntime, DefaultChannelRegistry) so any future plugin needing an operator-only admin surface can reuse it. - adminKeysRouter.ts now applies it as router-level middleware ahead of every route: missing/invalid session -> 401 (requireAuth's {code, message} shape); ctx.operatorAuth unavailable -> 503, fail closed rather than silently unauthenticated. - New end-to-end coverage in adminKeysRouter.test.ts against the REAL accessor (no stubbing of the auth decision): no-cookie, invalid-cookie, valid-cookie, and the fail-closed path. - Corrected the false publicPaths-only claim in docs/security-architecture.md, the package README, CHANGELOG.md, and middleware-agent-handoff.md; publicPathsExemption.test.ts's framing is updated to describe publicPaths as necessary but not sufficient.
…enation (#438) The key-id namespacing (`${key.id}:${callerConversationId}`) fixed cross-key collisions but was itself still lossy: SessionLogger's sanitizeScope collapses any punctuation run to a single '-', lowercases, and truncates to 80 chars before persisting the scope. Two DIFFERENT caller-supplied conversationIds under the SAME key could still land on the identical sanitized scope — e.g. "case/a" vs "case?a", or two long ids differing only past the truncation cutoff — letting one conversation thread recall another thread's memory/graph content under the same key. chatRouter.ts now derives the internal conversationId as sha256(key.id:callerConversationId) (hex digest) instead of plain string concatenation. The fixed-width, already-lowercase-alphanumeric output can't be mangled or truncated by sanitizeScope into colliding with a different digest, regardless of its exact transform rules. Adds regression coverage sending both collision shapes (punctuation, long-id truncation) through the real createApiChatRouter and asserting the resulting scopes differ via the real graphScopeFor/sanitizeScope. Updates existing conversationId assertions to the new hashed format and adds a docs/CHANGELOG.md entry per AGENTS.md's bugfix documentation rule.
Post-review: confirmed the raw `key:<uuid>` userRef.id is NOT a plugin-specific gap. Cite the exact code that disproves it: - ChannelUserRef.id is documented as opaque channel-native id (harness-channel-sdk/src/incoming.ts) - orchestratorDispatcher.ts passes userRef.id through unresolved for every channel, no exceptions - resolveOrCreateChannelIdentity is only invoked from the browser-login flow (src/index.ts), never per-turn; even omadia-ui-channel's own canvas turns use the raw session.subject, not session.omadiaUserId - src/routes/chat.ts's resolveUserId() already documents the identical behaviour for Teams/generic HTTP callers: ingestRun rejects unresolved ids by design (no auto-create), so the run-trace is dropped while the Session/Turn transcript still persists via ingestTurn No behavioural change — this plugin already matches the established, repo-wide pattern. Inventing per-key identity resolution here would add a new pattern no other channel implements. Rebuilt on top of bf64f70 (the sha256 conversationId fix) after discovering the prior 'baf22d0' commit had been built on a stale branch ref (95e2c31, pre-dating bf64f70) due to a worktree/ref race during the automated review-fixup loop — that version silently dropped the hash fix. This commit restores it while keeping the same investigation notes.
…438) An earlier round of docs/changelog notes for this branch claimed /api/public/v1/admin/keys was completely unauthenticated and reachable by any anonymous caller. A runtime reproduction mirroring the real mount order (src/index.ts's broad app.use('/api', requireAuth, ...) mount, which runs before pluginRouteRegistry.mountAll(app) later in boot) disproves that: the admin-keys path was already covered by that pre-existing session gate, the same mechanism protecting every other non-exempted channel route via publicPaths.ts. Corrects docs/security-architecture.md §8, docs/CHANGELOG.md's fixup entries, and docs/middleware-agent-handoff.md to describe the accurate history: the endpoint was already session-gated implicitly (mount order + publicPaths.ts omission); ctx.operatorAuth adds an explicit, non-implicit check so the guarantee doesn't depend on that coincidence, and publishes a reusable accessor for future admin surfaces. No source or test changes — the kernel-level ctx.operatorAuth work, adminKeysRouter.ts, and operatorAuthAccessor.ts are kept exactly as they were; only the narrative describing why they exist is corrected.
…public-v1 Branch was 7 commits behind origin/main. Resolved conflicts: - docs/CHANGELOG.md, docs/security-architecture.md: both sides added new ## sections under the same anchor; kept both, renumbered security-architecture.md's duplicate '## 8.' (this branch's Public API ingress section collided with main's new '## 8. What lives in the vault' from #437) to '## 9.', shifted the old '## 9. Reviewer checklist' to '## 10.', and updated the few '§ 8' cross-references (CHANGELOG.md, middleware-agent-handoff.md, this package's README.md) to '§ 9'. - middleware/package.json: both sides edited the same npm script strings (main reordered @omadia/plugin-privacy-guard earlier in the build/dev/typecheck chains; this branch appended @omadia/channel-api). Took main's ordering and inserted this branch's @omadia/channel-api / harness-channel-api/src/ entries at the same relative position this branch originally used. Also had to manually restore two dependency entries (cookie ^0.7.2, @types/cookie ^0.6.0) that a first pass at this merge accidentally dropped by reconstructing the whole scripts block from main's file instead of only patching the conflicting keys -- caught by a build failure (TS7016, missing cookie types) before this commit, not shipped. Verified after merge: npm install && npm run build && npm run typecheck (0 errors) && npm run lint (0 errors, 1 pre-existing unrelated warning) && npm run test (5088 tests, 5082 pass, 2 fail, 4 skipped -- both failures are different, unrelated files each full-suite run (uiPrefsRoute.test.ts HTTP-parser flake, devPlatformRoutes.test.ts 501-vs-200 flake), both pass cleanly in isolation, matching this project's documented pre-existing full-suite test-pollution pattern.
…#439) Issue #438 shipped a working bearer-token credential, but it lived inside the @omadia/channel-api plugin and gated exactly one route. A server-to-server caller (the driving case is a Laravel/PHP integration with no human session) needs API keys as an authentication method any route can adopt, and once more than the chat route is reachable, authentication alone stops being enough. The primitives move to a new workspace package @omadia/api-key-auth. That is the only home both sides can reach: the kernel must never import a channel plugin, and a plugin cannot import kernel source (its tsconfig has rootDir: src and it resolves deps only as @omadia/*). Same role plugin-api and channel-sdk already play. The package stays dependency-free apart from an express peer -- its storage dependency is a structural subset that SecretsAccessor already satisfies -- so no new npm dependency, matching #438. requireApiKey deliberately does not populate req.session: SessionClaims.role is hard-typed 'admin', so synthesizing a session for a machine would make every session-reading route downstream treat a key as an operator. The principal lands on req.apiKey instead, so a route has to opt in. Scopes default to ['chat:write'] for a key with no persisted scopes field -- exactly the one capability a pre-#439 key had. Defaulting them to '*' would also keep them working and would silently widen every existing key to whatever scoped surface lands next. publicPaths.ts is not broadened; /api/public/v1/chat remains the only exempted API-key route.
…alformed (#439) `normalizeScopes` collapsed two different situations into a capability GRANT: a `scopes` field that is ABSENT (a genuine pre-#439 record, where the legacy `['chat:write']` default is exactly right) and one that is PRESENT but unreadable (corruption, or a future writer bug). A vault record holding `"scopes": "memory:read"` — a string instead of an array — or `"scopes": ["Chat:Write"]` — rejected by SCOPE_PATTERN — hydrated to `['chat:write']` and authenticated against POST /api/public/v1/chat. Either is plausibly a key an operator deliberately restricted AWAY from chat, so the fallback handed back precisely the access that was removed. The two cases are now decided separately. Absent -> the legacy default. Present-but-malformed (not an array, empty array, or an array with any invalid entry) -> the empty scope set: the key still authenticates, and every `hasScope` check on it fails closed with 403. Partially-valid arrays deny too rather than silently narrowing to the valid subset — a record we cannot read faithfully is one we must not guess at, and half a scope set is harder to debug than none. Each case logs a warning so an operator can tell a corrupt record from a revoked key. `create()` is unchanged and always persists the resolved scope set explicitly, which is what keeps "no scopes field at all" meaning "pre-#439" on read rather than "written by us and lost". test/auth/apiKeyScopes.test.ts asserted the old fail-open (`normalizeScopes(['nonsense'])` -> legacy default); that assertion is inverted, not left standing, and joined by explicit cases for string-instead-of-array, uppercase, empty-array, absent-field and partially-valid input, plus end-to-end 403 coverage through `requireApiKey` and through the store. Rebased onto 95e2c31, which added the kernel-level `ctx.operatorAuth` gate to the same admin router this change touches. Both survive: the gate runs ahead of every route, the scopes additions sit on top of it, and a new test proves an anonymous POST carrying `scopes: ['*']` is rejected 401 and mints nothing.
…faulting it (#439) normalizeScopes treats a persisted empty scopes array as corruption and denies every capability, but create() resolved the same value to the legacy ['chat:write'] default. One field therefore meant 'deny everything' on read and 'grant chat' on write, and an operator posting {"scopes": []} to /api/public/v1/admin/keys was handed a chat-capable key. Only an omitted field now resolves to the default. An explicit [] is rejected at both layers: 400 from the admin route via zod .min(1), and a throw from assertValidScopes for callers using the package directly.
# Conflicts: # docs/CHANGELOG.md # middleware/package-lock.json # middleware/package.json
…Router.ts comment The route-level doc comment still described the admin/keys routes as having been 'completely unauthenticated' before ctx.operatorAuth was added. docs/security-architecture.md was corrected on this point during issue #438 (b0cbb75) but the source comment in this file was never updated to match, so it still carried the disproven claim. Corrects it to match the accurate account: the broad app.use('/api', requireAuth, ...) mount in middleware/src/index.ts already covered this router before ctx.operatorAuth existed; ctx.operatorAuth replaces that implicit, mount-order-dependent coverage with an explicit one that travels with the router.
security-architecture.md's Public API ingress section is §9 (renumbered during the earlier #438-main merge to resolve a duplicate '## 8.' collision with #437's vault section); this one reference in the package README still pointed at §8. Caught by codex's final review of the consolidated #438+#439 branch.
This was referenced Jul 30, 2026
Weegy
enabled auto-merge (squash)
July 30, 2026 10:27
Weegy
added a commit
that referenced
this pull request
Jul 30, 2026
The channel-api work added 3 dev-platform references (test/packages). Fourth legitimate raise: main ADDED dev-platform code; core did not re-acquire a dependency.
Weegy
added a commit
that referenced
this pull request
Jul 30, 2026
The channel-api work added 3 dev-platform references (test/packages). Same legitimate raise as on the C5 branch.
Weegy
added a commit
that referenced
this pull request
Jul 30, 2026
…ncy-safe (#552) * feat(plugins): allow .sql in distributed plugin packages A distributed plugin ships its own schema as `migrations/*.sql`, but the zip extractor rejected the extension, so the package failed ingest with `zip.forbidden_extension`. Adding `.sql` was previously refused because it would have escalated the manifest-identity path traversal (an id of `..` with version `migrations` resolved onto the real migrations directory) from a directory delete into arbitrary SQL executed at boot. That traversal was closed in 09ff9cd, and the remaining routes from uploaded content to a migrator-scanned path were re-verified before this change: - all eight SQL migrators resolve their directory from their own `import.meta.url`, the fixed `middleware/migrations`, or an operator env override — never from package content; - the three extraction call sites land only under state dirs (uploaded-packages staging/final, builder previews), never in the shipped code tree; - the `node_modules` symlink at the packages root is charset-valid as an id and is blocked by the reserved-root check, not by the charset gate. Tests pin those conditions rather than just the happy path: `.sql` accepted by the default allowlist, still rejected under an explicit override that omits it, and no `.sql` surviving outside the packages root for a traversing identity, the reserved `node_modules` id, or a Zip-Slip entry name. Shared package-zip fixtures move to `test/_helpers/pluginPackageZip.ts` so both install-pipeline suites use one builder. * fix(migrations): make all 8 SQL migrators safe against concurrent multi-replica boot Every migrator was read-ledger -> filter -> apply with no mutual exclusion, so two replicas booting together both executed the same pending list. `CREATE ... IF NOT EXISTS` masks that; `ALTER TABLE ... ADD CONSTRAINT` does not — the loser gets 42710 and its boot fails. `conductorWebhookEndpointStore.pg.test.ts` already carried a test-level `migrateWithRetry` workaround for the same race. Design (identical in all eight, no shared helper — that is separate work): - Read the ledger BEFORE locking and return early when nothing is pending, so the steady-state boot takes no lock and can never queue behind a migrating replica. - Acquire with `pg_try_advisory_lock(4410, hashtext(<ledger table>))` in a bounded 2s poll, never the blocking `pg_advisory_lock`: three of the eight run inside a plugin `activate()` that ToolPluginRuntime hard-caps at 10s, and the knowledge-graph plugin can already have spent 6s in `waitForPostgres` first. The try-variant also returns a boolean the code actually reads, so the session always knows whether it holds the lock. - Re-read the ledger UNDER the lock, so a replica that queued behind a winner never re-applies what the winner just applied. - A loser that never gets the lock re-reads the ledger: if the winner finished, it continues; otherwise it throws a retryable error naming the pending files. The message says "timed out" so `bootstrap.retryErroredPlugins` classifies it as transient instead of latching the plugin `errored`. - `pg_advisory_unlock` runs on the success path only, inside `try`, and its boolean is read. It is never awaited in `finally`, where it could hang on a half-open connection (these pools set no statement_timeout) or replace the original migration error. Any path that cannot prove the lock was released ends at `client.release(true)`, which destroys the connection and releases the session lock with it. `client.release()` always runs. - `ensureLedger` retries once on 42P07/23505: `CREATE TABLE IF NOT EXISTS` is not atomic against a concurrent `CREATE TABLE` of the same name. Tests cover, for all eight: no lock when there is no work, lock taken before the first migration, the under-lock re-read, the bounded loser error, the loser whose winner finished, an original error preserved through a failing migration, a throwing unlock, an unlock that reports not-held, a driver that models no advisory locks, the ledger DDL race, and that 6s + 2s fits the 10s activate cap. * feat(plugins): allow .sql in packages + make the 8 migrators concurrency-safe Two of three wave-2 packets shipped after review; the third is held back again. .SQL IN THE ZIP ALLOWLIST — unblocked by #548. It was rejected in the first batch because it would have escalated the then-live path traversal from directory delete/replace into arbitrary SQL execution. #548 closed that, and the reviewer verified independently that no route remains from uploaded content to any migrator-scanned path: all 8 migrators derive their directory from import.meta.url or an operator env var, never from package content, and the 3 extraction destinations are staging/preview paths with generated names. The stronger argument, which the analysis had buried: .js/.mjs/.cjs are already allowlisted and dynamic-import()ed by the runtime, so anyone who can get a zip to ingest() already holds in-process code execution. .sql is strictly weaker than what the trust boundary already grants — which makes it security-inert independently of the migrator analysis. Also adds `migrations` to both boilerplate build-zip.mjs INCLUDE lists. Without it the directory is silently dropped at packaging time: the copy loop skips anything not on the list, so an author following the boilerplate gets a green install and no schema. The allowlist entry alone was necessary but not sufficient. MIGRATOR CONCURRENCY — shipped on the third attempt. Every migrator was read-ledger -> filter -> apply with no mutual exclusion, so two replicas booting together both execute; IF NOT EXISTS masks it, ADD CONSTRAINT does not (42710). Two earlier designs were rejected, and both rejections shaped this one: an unbounded pg_advisory_lock is unusable because three of the eight migrators run inside a plugin activate() that ToolPluginRuntime caps at 10s — turning a rare race into a deterministic boot failure; and a retry that called pg_advisory_unlock without reading its boolean return could not distinguish "released" from "never held", making the mechanism silently inert. STILL HELD BACK: the DynamicAgentRuntime rollback, now on its second rejection. It does not cover the timeout path — withTimeout is a bare Promise.race and does not cancel, so after the rollback runs and clears the in-flight marker, the orphaned activate() continues and re-registers. That is the same defect class already documented for ToolPluginRuntime, and it wants a cancellation token rather than another rollback tweak. 5,224 pass, typecheck clean, ratchet held at 3,303. * chore(470): resync wave2 with main (#549) — baseline 3,303 → 3,306 The channel-api work added 3 dev-platform references (test/packages). Same legitimate raise as on the C5 branch.
Weegy
added a commit
that referenced
this pull request
Aug 12, 2026
…C5) (#554) * refactor(conductor): delete the dead dev-job step coupling (epic #470 C5) The Conductor's `dev.job` step was built in W3 and never wired: `conductor/index.ts` constructs `ConductorRunExecutor` without a `devJob` dep, so the dispatch branch was permanently false; the launch half had no implementation at all (`createConductorJob`, `setAwaitId`, `getAwaitId` existed only as interface members); `DevJobOutcomeEmitter.emit()` had no caller in `src/`; nothing scheduled the reconciliation sweep; and no bundled template referenced `dev.job` (which `listActions` rejects anyway). Per `specs/470-dev-platform-plugin/dormant-capabilities.md` §1 the verdict is DELETE, not genericise — nothing needs the generic version. Deleted whole: `conductor/devJobStepEffect.ts`, `devplatform/devJobConductorBridge.ts`, `test/conductorDevJobStep.test.ts`. Stripped from `runExecutor.ts`: both imports, the `devJob` field + ctor dep, `DevJobPortUnavailableError`, the dispatch branch, `resolveDevJobAwait`, `reconcileTerminalDevJobAwaits`, `openDevJobAwait`. `awaitStore.listWaiting` drops its `AND channel_type <> 'dev_job'` predicate outright rather than keeping a generic filter. `openHumanAwait` is now the single caller of `create`, so every await has a human holder by construction and the excluded set is provably empty; a filter whose complement no member can enter asserts an invariant in the wrong place, and a future non-human await kind would have to remember to add itself to survive it. New `test/conductorAwaitStore.test.ts` guards the channel-agnostic contract (mutation-checked: re-adding a channel predicate fails it). `dev_jobs.conductor_await_id` (migration 0024) is KEPT — migrations are forward-only here and a DROP is the one irreversible act. It is marked as an orphaned column so the schema is not misread as evidence of a live feature. Core-decoupling ratchet: 3303 -> 3164 (-139), no zone rose. * refactor(conductor): delete the dead dev-job step coupling (C5) First change in this epic that moves the decoupling count DOWN. 3,303 → 3,167 (−136): middleware/src −96, middleware/test −43. WHAT THIS IS NOT: the Conductor is a live feature — 31 files, ~6,200 LOC backend, 23 UI files, 7 migrations, its own spec. Runs, steps, human approval gates, templates, the Designer canvas are all untouched. All 204 conductor tests pass. WHAT WAS DEAD: one step type inside it. `dev.job` was meant to let a workflow launch a Dev Platform job, park the run, and resume on its terminal outcome. It was built and never wired: - conductor/index.ts constructs the executor with NO devJob dep, so the dispatch branch was permanently false — and `git log -S` shows it was never wired in ANY commit - the launch half had no implementation at all: createConductorJob, setAwaitId and getAwaitId existed only as interface members, and dev_jobs.conductor_await_id was never selected or written - DevJobOutcomeEmitter.emit() had no caller in src/ - nothing scheduled the reconciliation sweep - no bundled template referenced dev.job, and listActions never included it, so the step could not pass validation Removed: conductor/devJobStepEffect.ts (122 LOC), devplatform/ devJobConductorBridge.ts (113), test/conductorDevJobStep.test.ts (358), and every dev-job reference in runExecutor.ts (31 → 0), awaitStore.ts (6 → 0) and routes.ts (1 → 0). KEPT: migration 0024's conductor_await_id column. Migrations are forward-only here and dropping a column is the one irreversible act in this change; it is marked orphaned instead. THE SUBTLE PART: awaitStore's human-inbox query excluded `channel_type = 'dev_job'`. That predicate is now gone rather than genericised, because after the delete `openHumanAwait` is the sole writer of conductor_awaits — a filter whose complement no code path can populate asserts an invariant in the wrong place, and a channel allow/denylist would fail open for a future await kind that forgot to register itself. The compensating test needed a fix the review caught: its fixtures were teams/telegram/web, so restoring `<> 'dev_job'` would have left it green — it could not detect the exact regression it exists for. Added a dev_job fixture row and mutation-checked it: with the predicate restored 1 fail, without it 3 pass. Also propagates the decision into the specs, which still described the step as a capability the extraction must carry and H2 as a registry to build. H2 needed no mechanism after all — the coupling was dead, so deleting it was the whole fix. * chore(470): resync C5 with main (#549) — baseline 3,167 → 3,170 The channel-api work added 3 dev-platform references (test/packages). Fourth legitimate raise: main ADDED dev-platform code; core did not re-acquire a dependency. * chore(470): resync C5 with main (#552, #553) — baseline 3,167 * chore(470): resync C5 decoupling baseline with main — 3,448 → 3,312
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
1 participant
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
Consolidates two related, independently-developed branches into one feature: a documented public HTTP API for driving omadia's chat flow, authenticated by server-to-server API keys with per-key scopes.
@omadia/channel-api, a channel plugin exposingPOST /api/public/v1/chat(bearer-key auth, NDJSON streaming through the sameCoreApi.handleTurnStreamdispatch every other channel uses, rate limiting, audit log) and/api/public/v1/admin/keysfor key lifecycle.@omadia/api-key-auth— kernel-importable, unlike a channel plugin — added a generalrequireApiKeyExpress middleware any kernel route can adopt, and added per-key scopes (defaultchat:writefor every pre-Server-to-server API keys (bearer tokens) for plugin integrations #439 key, so nothing existing silently loses access).This branch reconciles both: #439's 3 commits rebased onto #438's fully-
main-merged tip, with #438's later fixes (see below) re-applied on top of #439's restructuring.What's included
Credential model. Each API key is its own identity (
ChannelUserRef{ kind: 'custom', id: 'key:<keyId>' }), not a delegate for a human end-user. Keys are vault-backed, sha256-hashed at rest (no DB migration), shown once at creation, verified withcrypto.timingSafeEqualwithout early return.Scopes (#439). Every key carries an explicit scope set;
chat:writeis the only scope in play today. Two deliberate fail-closed rules, each with its own regression coverage:scopesfield denies every capability rather than defaulting to a grant.scopes: []at key-creation time is rejected (400), not silently widened to the legacy default.Two independent, non-overlapping auth mechanisms for two different route groups:
ctx.operatorAuth(session-based, kernel-published, used by/admin/keys) andrequireApiKey(bearer-key based, used by/chat). Neither duplicates or gaps the other.Collision-proof conversation scoping. The internal
conversationIdhanded toCoreApiissha256(key.keyId:callerConversationId)— a fixed-width hash, not string concatenation — so neither a different key nor a different caller-supplied id under the same key can collide on the same core-side scope, regardless ofSessionLogger's lossy downstream sanitization (punctuation-collapsing, truncation).Accurate security documentation.
docs/security-architecture.md§ 9 describes the real history:/admin/keyswas always covered by the kernel's existing broadapp.use('/api', requireAuth, ...)session gate (verified empirically against the real mount order, not just by reading code);ctx.operatorAuthadds an explicit check so that protection doesn't depend on mount-order/publicPaths.tscoincidence, and publishes a reusable pattern for future admin-surface plugins.Review history
Both branches went through multiple rounds of adversarial review (Claude + codex) independently before consolidation, including catching and fixing: an audit-log accuracy gap for in-band stream errors (same bug class as #403), a cross-key and same-key
conversationIdcollision, and a false "critical unauthenticated admin endpoint" claim that was investigated, empirically disproven, and corrected rather than left standing. The consolidated branch itself was then independently re-reviewed end-to-end by codex againstorigin/main, verdictprReady: true, zero blocking findings.Verification
cd middleware && npm run typecheck && npm run lint && npm run test— clean (0 type errors, 0 lint errors). Full suite passes; the handful of failures seen across repeated runs are pre-existing full-suite-only test pollution (a different unrelated file each run, always green in isolation), not caused by this branch. Scoped suite (test/channelApi/**,test/auth/**, 147 tests) passes 100% on every run.Closes #438
Closes #439
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.