feat: add github communicator (Issues + PR comments)#234
Open
LissaGreense wants to merge 37 commits into
Open
Conversation
Declare GitHubProviderConfigSchema and extend the communicator discriminated union with a "github" kind, matching the existing slack/telegram/discord/teams/whatsapp shape. All fields are optional with env-var fallbacks; the schema deliberately omits credential_id and webhook_secret because credentials resolve at runtime via Link wiring, not the workspace.yml block. This is the config-layer slice from the v2 plan (docs/plans/2026-05-08-github-communicator-design.v2.md §"Schema additions"). Atlas adapter-factory, Link githubAppProvider, and the /signals/github route land in follow-ups. ## Progress - Task: github-communicator — config schema slice - Decisions: Mirror DiscordProviderConfigSchema exactly (all-optional, env fallbacks). Keep default_destination as a v2 placeholder so the shape stays parallel without committing to proactive outbound now. - Key Learnings: CommunicatorKindSchema's z.enum reformats from a single-line array to a multi-line one once it crosses biome's print width — picked up incidentally when adding "github". The per-platform `unexpected_field` rejection is table-driven via it.each, so adding a new platform is a one-line table append. - Files: packages/config/src/signals.ts, packages/config/src/communicators.ts, packages/config/src/communicators.test.ts
The `KIND_DISPLAY_NAMES` Record<CommunicatorKind, string> in the connect_communicator tool became non-exhaustive when "github" was added to the canonical CommunicatorKindSchema enum. Add the github display name and update the tool description so the LLM knows GitHub's surface is Issue/PR comments (not "type to Friday from a chat client" — that framing is wrong for github). ## Progress - Task: github-communicator — fix exhaustive map cascade from #1 - Decisions: Phrase the new platform as "GitHub (Issue/PR comments)" in the description so the agent doesn't pitch it as a chat client; bump "five platforms" to "six platforms" so the count stays honest. - Key Learnings: `Record<UnionType, V>` is the load-bearing exhaustive consumer to watch when adding a kind to a canonical enum — switch statements with default branches will silently swallow new kinds, Record literals will not. `deno check` cache is non-deterministic on cold runs (same files report different errors run-to-run); `--reload` is the source of truth. - Files: packages/system/agents/workspace-chat/tools/connect-communicator.ts
Adds the GitHub chat adapter to atlasd's per-workspace adapter map. Pins @chat-adapter/github@4.27.0 (matches the other five chat-adapter pins), extends `PlatformCredentials` with a github variant, adds "github" to `CHAT_PROVIDERS`, and branches `buildAdapter` to call `createGitHubAdapter` in multi-tenant mode (omits `installationId` so the adapter auto-extracts it from inbound webhook payloads). Daemon-side credentials transform: adds `GithubLinkSecretSchema` + `resolveGithubFromLink` to `chat-sdk-instance.ts`, mirroring the Slack / Discord / Teams / WhatsApp Link-resolution pattern. v1 is Link-only — no yml-inline or env-var fallback because the secret depends on auto-populated `bot_user_slug` / `bot_user_id` fields captured by the github-app provider's `health()` at credential-save time. The stored `installation_id` is parsed for schema completeness but not forwarded to the adapter. ## Progress - Task: #3 Wire @chat-adapter/github into atlasd adapter factory - Decisions: - Multi-tenant mode (omit installationId) per design v2 §"Routing — body-based, multi-tenant"; one App, many installations, many workspaces, one shared webhook_secret. - app_id stored in Link as number, forwarded to adapter as string — chat-sdk's GitHubAdapterMultiTenantAppConfig requires string. - botUserSlug → adapter's `userName` field (with [bot] suffix); botUserId → adapter's `botUserId` (rename-immune fallback). - No env/yml fallback for github (unlike slack/discord) — the auto-populated bot fields can't be hand-rolled in workspace.yml. - Key Learnings: - chat-sdk's `GitHubAdapterConfig` is a discriminated union over four auth modes (PAT, single-tenant App, multi-tenant App, auto-detect); the multi-tenant variant requires `appId` + `privateKey` and uses `installationId?: never` to enforce omission at the type level. - The adapter's `userName` and `botUserId` are independent self-message-detection paths — passing both means a github App rename only breaks slug-based @mention detection, not bot-self filtering. - `apps/link/src/providers/github-app.ts` is pre-existing untracked WIP for slice 3 (Link provider) and currently has a typecheck error; isolated `deno check apps/atlasd/src/chat-sdk/adapter-factory.ts` passes, but `chat-sdk-instance.ts` transitively imports the broken Link file via `@atlas/core/mcp-registry/credential-resolver`. - Files: - apps/atlasd/package.json - apps/atlasd/src/chat-sdk/adapter-factory.ts - apps/atlasd/src/chat-sdk/chat-sdk-instance.ts - deno.lock
New apikey provider at `apps/link/src/providers/github-app.ts` (id:
`github-app`, distinct from the existing `github` PAT provider). User
pastes `app_id`, `private_key` (PEM), `webhook_secret`, and
`installation_id`; `health()` mints an App-JWT (RS256, iat=now-60s,
exp=now+9m) and walks `GET /app` -> `GET /app/installations/{id}` ->
`GET /users/<slug>%5Bbot%5D` to validate the keypair and capture the
bot user's identity. Captured `bot_user_slug` (with literal `[bot]`
suffix to match webhook payloads byte-for-byte) and rename-immune
`bot_user_id` are surfaced via `autoFields()` and merged into the
stored secret.
Also reorders `routes/credentials.ts` so `health()` runs before
`autoFields()` — required so providers can capture server-derived
identity into the stored secret. No behavior change for existing
providers (none combine `health` with `autoFields`).
PKCS1 PEMs (the format GitHub distributes) are round-tripped through
`node:crypto.createPrivateKey` to PKCS8 before handing to jose, since
jose only imports PKCS8.
## Progress
- Task: #2 — Add githubAppProvider in Link with health-check + bot-user capture
- Decisions:
- Closure pattern for autoFields/health share state (single-tenant per
save flow; concurrent saves of the same provider don't realistically
race in this app).
- Reordered credentials.ts to call health() before autoFields() so the
closure pattern actually populates stored secrets.
- JWT lib: jose (already an apps/link dep, used by tests).
- PKCS1->PKCS8 conversion via node:crypto round-trip rather than a new
dep or hand-rolled ASN.1 parsing.
- Key Learnings:
- jose's importPKCS8 rejects PKCS1 PEMs (BEGIN RSA PRIVATE KEY); GitHub
distributes keys in PKCS1, so a node:crypto round-trip via
createPrivateKey + export({type:"pkcs8"}) is the path of least
resistance.
- GitHub `/users/{login}` accepts `[bot]` literally only when URL-
encoded (`%5Bbot%5D`); raw brackets get rejected.
- Existing credentials.ts route ran autoFields BEFORE health, which
blocks any provider whose autoFields needs a server identity probe.
Reordering is safe because no current provider has both.
- Files:
- apps/link/src/providers/github-app.ts (new)
- apps/link/src/providers/github-app.test.ts (new)
- apps/link/src/providers/constants.ts (add GITHUB_APP_PROVIDER)
- apps/link/src/index.ts (register githubAppProvider)
- apps/link/src/routes/credentials.ts (run health before autoFields)
Add the github branch to deriveConnectionId so the daemon can wire GitHub-App credentials into the communicator routing table. Mirrors the discord/teams/whatsapp pattern: per-kind credential schema asserts only the field used as the routing key, then `String(...)` coerces to the table's string-typed connection_id column. GitHub webhooks carry `installation.id` on every App-scoped event, and the App is multi-tenant by design — so the (App, installation) pair is the only routing key the daemon needs to find the right workspace. Link's source-of-truth schema stores installation_id as a positive integer, but storage round-trips can stringify numerics; accept both shapes via z.union and coerce on the way out. Adds three test cases mirroring the discord pattern: numeric input, pre-stringified input, and missing-field rejection. ## Progress - Task: #4 — Add github branch to deriveConnectionId with test - Decisions: - z.union([number, string]) over z.coerce.string(): the union keeps the input shape visible in error paths and matches Link's int-typed field on the happy path. coerce would silently swallow garbage like `installation_id: {}`. - Test the string-stored case explicitly — storage layers (Postgres JSONB, env-var injection) routinely stringify numerics, and a schema that accepts both shapes is worthless without a test that proves it. - Top-of-file doc comment updated to include GitHub in the platform list; deriveConnectionId doc comment gets a github bullet matching the discord/teams style. - Key Learnings: the per-kind CredentialSecretSchemas in this file intentionally assert ONLY the field used as the routing key — the full provider-side schema (with private_key validation, PEM checks, etc.) lives in apps/link/src/providers/. Don't duplicate validation across the boundary; the daemon trusts Link's stored secret and only re-parses what it routes on. - Files: apps/atlasd/src/services/communicator-wiring.ts, apps/atlasd/src/services/communicator-wiring.test.ts
Body-routed inbound webhook handler for GitHub App events. Mirrors the
slack handler's clone-before-consume + parse + 400/404 tail, but uses
the wiring-table path (`resolveCommunicatorByConnection`) directly
instead of `findWorkspaceByProvider` — wiring is now the source of
truth for github (workspace.yml carries `{ kind: github }` only, with
no inline secrets, so there's no legacy yml-fallback to consult).
Adds `"github"` to the `PlatformProvider` union and `PROVIDER_LABELS`
record so `delegateToWebhook` accepts it. Defines
`GitHubInstallationPayloadSchema` for the routing-key extraction.
Error semantics: 400 on bad JSON / missing `installation.id`, 404 on
unwired installation (GitHub retries with backoff), 500 on adapter
init failure. No GET handshake — GitHub doesn't do one.
## Progress
- Task: #5 Add /signals/github route handler
- Decisions:
- Use `resolveCommunicatorByConnection` directly rather than
`findWorkspaceByProvider` — github has no legacy yml/env fallback
to fall through to, and the wiring path is one fewer indirection.
- Stringify `installation.id` at the boundary: GitHub sends it as a
number, but `connection_id` columns are strings across the wiring
table for shape consistency with other providers (`app_id`,
`application_id`, etc.).
- 404 on unwired installation rather than 200-no-op: GitHub's retry
semantics mean a transient daemon outage between webhook delivery
and Link being reachable resolves itself; a silent 200 would mask
the unwired-but-installed configuration drift.
- Key Learnings:
- The slack/teams/whatsapp routes use `findWorkspaceByProvider`
which itself tries the wiring table first when `configValue` is
set, then falls back to yml-config iteration. For github there's
no yml-config to iterate, so the direct call is more honest.
- `c.req.raw.clone()` MUST happen before `await c.req.text()` —
Hono's body-text helper consumes the underlying stream, and the
chat-sdk adapter needs the raw body intact for HMAC-SHA256
verification against the stored `webhook_secret`.
- Files:
- apps/atlasd/routes/signals/platform.ts
Adds a `POST /github` describe block to platform.test.ts mirroring the shape of the slack/teams suites. The github route handler shipped in 48abf37 with no tests because it depended on `resolveCommunicatorByConnection` (already mocked at the file level for the slack suite) and the `@chat-adapter/github` package; both were already wired before this commit. Ten new cases pin every branch the route owns: - happy path: wiring hit + cloned-body + GitHub headers preserved - numeric installation.id stringified at the boundary (matches the wiring table's string-typed connection_id column) - 400 on invalid JSON / missing installation.id (no wiring call) - 404 on unwired installation (GitHub retries with backoff) - 404 on Link unreachable (no yml-fallback to consult) - 404 when the workspace has no github webhook on Chat SDK - 500 when Chat SDK creation fails / webhook handler throws - 401 from adapter propagates verbatim — signature verification is the adapter's job, not the route's `makeChatSdkInstance`'s `webhookKey` union extended to include "github" so the helper accepts the new platform without a cast. ## Progress - Task: github-communicator — add route-level tests for /signals/github - Decisions: - Test the route's contract (parse, lookup, delegate, forward), not the chat-sdk adapter's signature verification — that lives in the adapter and has its own test surface in the chat-sdk repo. The "401 from adapter propagates" test pins the boundary explicitly. - Drive the wiring mock directly per-case rather than introducing a helper — each case wires one specific resolver shape and the boilerplate is small enough that DRY-ing it would obscure intent. - Stringify-at-the-boundary test uses a 10-digit number so the "trust the int" regression isn't masked by JS auto-coercion that happens with smaller ints. - 401-propagation test was prompted by design v2 §"Testing Decisions" user story #10 (bad HMAC). Real bad-HMAC verification belongs to the chat-sdk adapter; pinning the route's pass-through behavior is the route-level contract that satisfies the story. - Key Learnings: - The github route does NOT iterate `daemon.getWorkspaceManager().list()` — it goes purely through wiring resolution. So the happy path needs no workspace config in the daemon mock, only a Chat SDK resolver. This diverges from slack/teams routes which DO iterate workspaces (yml-fallback path). - I deliberately broke `String(routing.data.installation.id)` → raw cast and confirmed both the happy-path test (404 from wiring miss on numeric key) and the stringify-boundary test failed — standard red-then-green check that the assertions actually exercise the path. - Files: - apps/atlasd/routes/signals/platform.test.ts
Covers the three gaps left by Patina's route-level tests, which mocked the
chat-sdk webhook stub:
1. `buildChatSdkAdapters` constructs a real `@chat-adapter/github` adapter
when given github credentials.
2. The adapter performs real HMAC-SHA256 verification — valid signatures
pass (200), tampered ones return 401.
3. `resolveGithubFromLink` (via `resolvePlatformCredentials`) maps Link's
snake_case secret to the camelCase `PlatformCredentials` shape, with
`app_id: number → appId: string` coercion and `installation_id` dropped
so the adapter stays in multi-tenant mode.
Mocks only at the network boundary via `vi.stubGlobal('fetch')`. Adapter,
factory, and resolver are exercised real.
## Progress
- Task: #7 Add chat-sdk github adapter integration test (real HMAC + Link mapping)
- Decisions:
- Used GitHub `ping` event for the HMAC tests — adapter returns 200 after
signature check without touching `installation` / chat state, keeping the
test free of Chat instance / state-adapter setup.
- Skipped the `chat.webhooks.github` wrapper and called `adapter.handleWebhook`
directly. The wrapper only does `chat.handleWebhook(name, ...)` →
`adapter.handleWebhook(...)`; testing the wrapper would force `Chat` init
without adding behavioral coverage.
- Dropped a fake PEM into `privateKey` — the field is only parsed during
outbound JWT minting (post / edit / react), not during inbound webhook
verification. HMAC uses `webhookSecret` only.
- Key Learnings:
- GitHubAdapter constructor accepts a private_key string verbatim and only
parses it lazily inside `getOctokit`. Tests of inbound paths can pass any
PEM-shaped string without touching jose.
- `chat.webhooks[name]` is a thin lambda over `adapter.handleWebhook` — for
pure-adapter coverage, going through `buildChatSdkAdapters` and grabbing
`adapters.github` is enough; `new Chat({...})` adds startup cost
(state-adapter connect, per-adapter initialize) without exercising new
code.
- `resolvePlatformCredentials` reads `LINK_SERVICE_URL` lazily per call
(via `getLinkServiceUrl()`), so flipping it in `beforeEach` and stubbing
`fetch` is sufficient — no module reloads needed.
- Files: apps/atlasd/src/chat-sdk/github-adapter-integration.test.ts
Module-scoped `captured` shared between `health()` and `autoFields()` was
racy under concurrent credential saves: B's `captured = Bdata` could fire
between A's `await health()` resolving and A's sync `autoFields()` call,
causing A to store B's bot identity. Triggerable deterministically in
tests; rare but possible in production when responses land in the same
event-loop tick.
`health()` already returned `metadata: { bot_user_slug, bot_user_id }`.
The credentials route now consumes that metadata directly, deleting the
module global and the github-app `autoFields()` hook. `autoFields` stays
on the provider interface for stateless generators (telegram, whatsapp).
…cator `api_url` was accepted by the workspace.yml schema and the daemon-side `GithubLinkSecretSchema`, but `GithubAppSecretSchema` (Link provider input) used non-strict `z.object` and silently stripped it before storage. Setup instructions never mentioned it. Result: `api_url` could not reach the adapter through any supported path — pure dead code across three layers. Drop the field. GitHub Enterprise support remains deferred to v2; when revisited, plumb `api_url` through `GithubAppSecretSchema` end-to-end.
…icator GitHubProviderConfigSchema's app_id and installation_id were inert: their describe text claimed env-var fallback, but no code path read GITHUB_APP_ID or GITHUB_INSTALLATION_ID. Drop the fields and the matching test rows. GitHubCredentialSecretSchema's installation_id accepted z.union([number, string]), but the JSON round-trip through Cypher storage preserves number types and the Link provider only emits numbers. Tighten to z.number().int().positive() and drop the string-shape test row.
- platform.ts: emit `github_signal_routed { installationId, workspaceId }`
on the success path so "agent not responding to mentions" debugging
can correlate webhook to workspace at info level (parity with the
teams/whatsapp routes, supports user story #12).
- github-app.ts: setup step 9 claimed GitHub has no API to set an App's
webhook URL post-creation; `PATCH /app/hook/config` does exist. Reword
to describe the chosen design (manual paste = simplest contract)
rather than a non-existent constraint.
Collaborator
|
😮 GH as a communicator is sick |
Wire `github` into the playground's hardcoded `CommunicatorKind` unions so the workspace info card renders a Connect button for it. Backend already accepts the kind via `CommunicatorKindSchema`; without these UI strings the row was silently dropped by `asKind()`. Touched: communicators-card, communicator-apikey-form, connect-communicator (+ `KIND_DISPLAY_NAMES.github = "GitHub"`), tool-call-card, and the useConnect/useDisconnectCommunicator mutation input types.
Communicator kind → Link provider id is identity for the other 5 kinds, but github diverges: Link's PAT-based `githubProvider` already owns id `"github"` (see apps/link/src/providers/constants.ts), so the App provider is registered as `"github-app"`. The form was passing kind directly, which would have fetched the PAT secretSchema (single `token` field), saved a PAT-shaped credential, then failed `GithubLinkSecretSchema.safeParse` in atlasd at webhook resolution time.
…check A fresh workspace whose only config is `communicators.github` was falling into the empty-hero branch on `/platform/[workspaceId]`, hiding the Communicators card that the Info tab is supposed to surface. Treat declared communicators as a non-empty signal so the tabs render and the card becomes reachable. ## Progress - Task: B-1 — unblock QA case C-70 (Communicators card never renders for github-only workspace) - Decisions: chose option #1 (extend isEmpty) over hoisting the card out of the !isEmpty branch — preserves the empty-hero UX for truly empty workspaces and keeps the existing tab structure intact for non-empty ones. Card is reachable via Info tab, matching how jobs/signals/agents surface. - Key Learnings: `isEmpty` on the workspace overview is the sole gate for the entire tab block (Activity + Info), so anything that should appear on the Info tab in an otherwise-empty workspace must be reflected in the derive — not just in the section it lives under. - Files: tools/agent-playground/src/routes/platform/[workspaceId]/+page.svelte
CredentialSecretForm previously typed every field as string, so
github-app's `app_id` / `installation_id` (declared `type: integer` in
Link's secretSchema) were sent as strings and rejected by Link's Zod
validator with `expected number, received string`.
Read each property's `type` from the JSON Schema, render integer/number
fields as `<input type="number">`, and coerce values to numbers at submit
time. Propagate `Record<string, string | number>` through the submit
callback chain (CredentialSecretForm → communicator-apikey-form,
connect-service, mcp-credentials-panel → useCredentialConnect.submitApiKey).
Drop the now-unnecessary `as Record<string, unknown>` cast at the replace
call site.
## Progress
- Task: B-2 — github communicator Connect form returns "expected number, received string"
- Decisions:
- Parse the JSON Schema `type` via a dedicated Zod enum
(`string|integer|number`) instead of widening to all JSON Schema
types — github-app is the only provider using non-string fields and
YAGNI applies to boolean/object/array shapes.
- Coerce at the form boundary (Number + Number.isFinite/isInteger checks)
rather than relying solely on `<input type="number">`, since the
browser still hands back a string via `bind:value` and we want a
consistent submit signature regardless of HTML input quirks.
- Widen `submitApiKey`/`onSubmit` to `Record<string, string | number>`
instead of `Record<string, unknown>` — keeps callers honest about the
JSON-serialisable shape we actually support.
- Key Learnings:
- Link's `/v1/providers/<id>` returns a JSON Schema with per-property
`type` annotations; the playground had been throwing this away by
parsing properties as `z.object({}).passthrough()`. Anything new
that adds non-string secret fields needs the form to read `type`.
- Svelte's `bind:value` on `<input type="number">` keeps the bound
variable as a string until the browser parses it — coerce at submit,
don't trust the input type alone.
- github-app is registered under provider id `"github-app"` because
Link's PAT-based github provider already owns `"github"`; the
communicator wrapper maps `kind="github"` → `providerId="github-app"`
(see apps/link/src/providers/constants.ts).
- Files:
- tools/agent-playground/src/lib/components/credential-secret-form.svelte
- tools/agent-playground/src/lib/components/shared/communicator-apikey-form.svelte
- tools/agent-playground/src/lib/components/chat/connect-service.svelte
- tools/agent-playground/src/lib/components/mcp/mcp-credentials-panel.svelte
- tools/agent-playground/src/lib/use-credential-connect.svelte.ts
Two coupled fixes for the github-app Connect form retry:
1. Regression from the previous B-2 commit: `<input type="number">` +
Svelte 5 `bind:value` auto-coerces the bound variable to a number, so
`fieldValues["app_id"]` became a number and the `.trim()` calls in
`handleSubmit` blew up with `TypeError: ...?.trim is not a function`.
Render integer/number fields as `<input type="text" inputmode="numeric"
pattern="[0-9]*">` instead — keeps `fieldValues` as `Record<string,
string>`, the mobile keyboard hint is preserved, and submit-time
`Number()` coercion (which we already do) still produces the right JSON
payload for Link's Zod schema.
2. B-3: PEM `private_key` was rendered as `<input type="password">` and
the browser stripped pasted newlines, so the PKCS1/PKCS8 BEGIN-marker
regex in `GithubAppSecretSchema` saw a single concatenated line and
rejected it. Tag `private_key` with `.meta({ format: "multiline" })` —
`z.toJSONSchema` passes it through to the playground, which reads
`format` alongside `type` and renders a `<textarea>` for that case.
The convention is opt-in: any future provider that needs multi-line
secret input just adds the same meta tag.
## Progress
- Task: B-2 regression + B-3 (PEM textarea) — surfaced when retrying QA C-71
- Decisions:
- Picked option #1 from the team lead's two-way fix (drop
`<input type="number">` rather than handle non-string values in
`handleSubmit`). Svelte 5's auto-coerce on number inputs couples the
binding type to the input element type, which is exactly the kind of
brittleness that caused the original bug. Text-input + submit-time
`Number()` keeps the binding type predictable.
- Used JSON Schema `format: "multiline"` (via Zod `.meta()`) instead of
a name heuristic like `/key|pem|cert/i`. Explicit, scales to future
providers, and `z.toJSONSchema` already preserves `.meta()` keys
verbatim — verified with a one-off `deno eval`.
- Sensitive textarea isn't masked. Dev-only tool, masking a textarea
breaks PEM legibility for the operator confirming what they pasted,
and the field still funnels through the same secure storage path.
- Key Learnings:
- **Svelte 5 `bind:value` auto-coerces based on `<input type>`.** A
`type="number"` input writes a number into the bound variable, not a
string — even when the bound `$state` is typed `Record<string,
string>`. If you want to preserve string semantics (e.g. for shared
`.trim()` paths), use `type="text"` + `inputmode`/`pattern` for the
keyboard hint and coerce at submit.
- **Single-line `<input>` strips pasted newlines** regardless of `type`
(`text`, `password`, etc.). Anything multi-line — PEM, JSON blobs,
SSH keys — must be a `<textarea>` from the start; the browser
silently collapses on paste with no DOM event to react to.
- **Zod 4's `.meta()` flows through `z.toJSONSchema`.** Arbitrary keys
pass through verbatim, so the Friday convention of declaring
`format: "multiline"` (or any future hint) on the Link side is a
one-liner per provider with no extra plumbing.
- Files:
- tools/agent-playground/src/lib/components/credential-secret-form.svelte
- apps/link/src/providers/github-app.ts
The third leg of the github-app health check (`GET /users/<slug>[bot]`) was
sending the same App-JWT-bearing headers as `/app` and `/app/installations/{id}`
and getting 401 from live GitHub, breaking C-71/C-10 at credential-save time.
App JWTs are only scoped to App-level endpoints; `/users/{login}` is public and
rejects them. Switch that one fetch to a fresh anonymous header set (Accept,
X-GitHub-Api-Version, User-Agent) and add a regression guard asserting the
bot-user call carries no Authorization header.
## Progress
- Task: B-4 — /users/<bot> with App JWT 401s against live GitHub
- Decisions: chose option #1 (drop Authorization header) over installation
tokens — `/users/{login}` data is public, no need to mint an extra token for
a read-only health probe. Lowest blast radius.
- Key Learnings: GitHub App JWTs are NOT a general-purpose Bearer credential —
they only authenticate `/app` and `/app/installations/...`. Sending an App
JWT to an otherwise-public endpoint is treated as malformed creds and 401s,
which is misleading because anonymous access to the same URL succeeds.
Mocked fetch tests can hide this class of bug entirely (any header passes
the mock); when the production call hits a real provider with strict scoping
rules, add an assertion on the auth header itself, not just the URL.
- Files: apps/link/src/providers/github-app.ts,
apps/link/src/providers/github-app.test.ts
GitHub stores bot accounts with a literal `[bot]` suffix on the slug
(e.g. `friday-bot[bot]`), and Link captures it that way in
`apps/link/src/providers/github-app.ts`. The chat-sdk's `detectMention`
compiles a literal regex `/@${userName}\b/i` against the raw comment
body — but humans typing on github.com only ever write `@friday-bot`,
so the suffix-bearing slug never matched and the agent never ran.
Strip the `[bot]` suffix in `adapter-factory.ts` before passing
`userName` to `createGitHubAdapter`. Self-filtering still works: the
adapter compares `_botUserId` (numeric) against `comment.user.id` to
ignore its own posts, so the cleaned-up `userName` only affects mention
detection.
## Progress
- Task: Fix B-5 — strip [bot] from userName in github adapter wiring
- Decisions: Surgical inline `.replace(/\[bot\]$/, "")` at the call
site, not a helper — this is the only place in the codebase that
hands `botUserSlug` to a chat-sdk adapter, and the dual-use of the
field (`userName` for mentions, `botUserId` for self-filter) is the
thing that makes the fix safe. Inlining keeps that reasoning visible.
Updated the platform.test.ts fixture body from `@friday-bot[bot]
hello` to `@friday-bot hello` to match what real GitHub UI emits
post-fix; the route doesn't inspect the body, so the assertion is
cosmetic, but the fixture should mirror reality.
- Key Learnings: chat-sdk's GitHub adapter uses `userName` and
`botUserId` for two completely different purposes — `userName` is
compiled into a literal `@${name}\b` regex for mention detection,
while `botUserId` (numeric) is what the adapter compares against
`comment.user.id` to skip self-replies. They are NOT
interchangeable, and a slug that round-trips correctly through
`botUserId`-based filtering can still silently break mentions. Any
future code that hands a stored bot identifier to a chat adapter
needs to think about which of the two roles it's filling.
- Files: apps/atlasd/src/chat-sdk/adapter-factory.ts,
apps/atlasd/src/chat-sdk/adapter-factory.test.ts,
apps/atlasd/routes/signals/platform.test.ts
Github thread ids contain `/` (e.g. `github:org/repo:issue:1`). Hono's
single-segment `:chatId` matcher in the workspace chat router can't
capture them, so the daemon returned 404 when the workspace-chat agent
fetched chat history. The agent silently fell back to `messages = []`,
`streamText` then threw `AI_InvalidPromptError`, and the bot posted an
empty body that GitHub rejected with 422 "Body cannot be blank".
Wrap `session.streamId` with `encodeURIComponent` at both call sites in
workspace-chat.agent.ts (the GET history fetch and the latent title
PATCH). Hono's `c.req.param()` decodes the value back before lookup, so
the storage layer sees the original id.
Add a regression `test.each` in chat.test.ts covering chatIds with
slash+colon, multiple slashes, and other URL-fishy characters — asserts
the encoded URL routes correctly and ChatStorage.getChat is called with
the decoded original.
## Progress
- Task: B-6 — chatId path param breaks on slash for github communicator
- Decisions: Path A (encode at client edge) over Path B (regex route
matcher). The `:chatId` parent has 4 sibling sub-routes (`/stream`,
`/message`, `/title`, `/_debug`); Path B would need careful
`:chatId{[^/]+}` constraints on the parent plus separate handling
for the bare endpoint, with non-trivial conflict risk. Path A is
surgical: only the workspace-chat agent has slash-bearing ids today,
and `encodeURIComponent` round-trips losslessly through Hono.
- Key Learnings: Hono's `:param` matcher only spans a single path
segment by default — any route param that can carry `/` must be
encoded by callers. Hono `c.req.param()` does decode via
`decodeURIComponent_`, so encoded and unencoded URLs both resolve to
the same handler input. `client.workspaceChat(...)[":chatId"].$get`
uses Hono's `replaceUrlParam` which substitutes raw — no auto-encode
on the client side. Worth checking other adapter threadIds for `/`
before they ship.
- Files: apps/atlasd/routes/workspaces/chat.test.ts,
packages/system/agents/workspace-chat/workspace-chat.agent.ts
Follow-up (B-7, separate commit): chat-sdk-instance.ts:894-898
`sourceWasSet` allowlist omits "github" — cosmetic/analytics only,
doesn't gate retrieval.
The setSource allowlist in createMessageHandler omitted the github adapter,
so github-originated threads fell through to the "atlas" default in
ChatSdkStateAdapter.subscribe. Retrieval still worked (chat lookups don't
key on source), but analytics and any source-based filtering saw the wrong
value. Add "github" to the allowlist and extend the source union (state
adapter, storage input, jetstream Zod schema, chat-migration Zod schema)
so the value type-checks end-to-end. Add a regression test asserting
setSource is called with ("github-thread-id", "github").
Lands as a follow-up to B-6 (e4a991e) — the live e2e flow worked without
this fix because it's cosmetic, but setSource should reflect the actual
adapter for analytics correctness.
## Progress
- Task: B-7 — add github to setSource allowlist
- Decisions: Original brief said "one-line fix", but the source union was
shared across 4 files (state adapter type, storage input type, two Zod
schemas). Type-checking the one-liner required extending all of them.
Kept the changes minimal — just appending "github" to each enumeration.
- Key Learnings: ChatSdkStateAdapter.subscribe defaults to source="atlas"
when threadSources lacks an entry — so an omitted allowlist entry
silently mistags rather than erroring. The chat source is enforced via
a Zod enum (jetstream-backend.ts) and a parallel KV migration schema
(chat-migration.ts); both must stay in sync with the TS unions in
chat-sdk-state-adapter.ts and storage.ts. There's no DB CHECK constraint
— chats live in NATS KV — so the only gate is the Zod enum.
- Files: apps/atlasd/src/chat-sdk/chat-sdk-instance.ts,
apps/atlasd/src/chat-sdk/chat-sdk-instance.test.ts,
apps/atlasd/src/chat-migration.ts,
packages/core/src/chat/chat-sdk-state-adapter.ts,
packages/core/src/chat/jetstream-backend.ts,
packages/core/src/chat/storage.ts
# Conflicts: # tools/agent-playground/src/lib/components/shared/communicator-apikey-form.svelte
… data Dependabot #244 bumped hono to 4.12.18 in package.json across 5 workspaces but left the `hono/client` import map entries at 4.12.16 in deno.json and tools/agent-playground/deno.json. Server route types resolved to 4.12.18 while RPC client types resolved to 4.12.16; 4.12.18 added a new [GET_MATCH_RESULT] symbol on HonoRequest, so TS rejected the constraint and collapsed the v2 client type to `unknown` (175 errors). Also inline the chat.test.ts mock data object so biome's `expand: never` formatter is satisfied.
- atlas-cli: encodeURIComponent(argv.id) in `chat <id>` URL builder so github thread ids (containing `/`) round-trip through Hono's single-segment `:chatId` matcher (sibling miss to e4a991e). - atlasd,link: stop storing `bot_user_slug` with the `[bot]` suffix. Reverses 6a5f45c — the strip in adapter-factory was load-bearing for chat-sdk's `@${userName}\b` mention regex, but the simpler fix is to not add the suffix at health() in the first place. `bot_user_id` remains the rename-immune identity used for bot-self detection. - atlasd: add issue_comment.created integration test that closes Plan §4's promise. Previously only `ping` was tested, which short-circuits in chat-adapter/github before handleIssueComment is reached. New test attaches a minimal chat stub and asserts threadId == "github:acme/widgets:issue:42". - link: document healthMetadata-wins-on-key-collision precedence in credentials.ts and add a Recent Deliveries troubleshooting hint to github-app setupInstructions for the silent webhook_secret failure mode.
…en adapter test coverage - Hoist COMMUNICATOR_KIND_TO_PROVIDER_ID to @atlas/config; svelte form consumes it. - Add pull_request_review_comment case to webhook integration test via it.each. - Align test fixtures with bare bot_user_slug (no [bot] suffix) as stored by github-app provider. - Enrich github_link_credential_invalid_secret log with credentialProvider/expectedProvider/hint to diagnose PAT-vs-App misconfiguration.
# Conflicts: # apps/atlasd/package.json # apps/atlasd/routes/workspaces/chat.test.ts # deno.lock # packages/system/agents/workspace-chat/workspace-chat.agent.ts
Post-merge semantic conflict: main's #265 changed StreamRegistry.createStream to require (workspaceId, chatId), and our "pre-sets the source for github-originated threads" test (added in 972aa6c) still used the old single-arg form. Vitest passed because JS is forgiving with arg counts, but `deno check` rightly flagged it — and the stream was being created under the wrong key, so the test was passing for the wrong reason.
Contributor
Author
|
Did a full manual QA pass on the latest commit. Adding the communicator via the playground UI:
End-to-end on github.com (real webhooks via cloudflare tunnel):
Lower layers exercised too: synthetic HMAC happy + bad-sig/invalid-JSON/missing-installation/unwired-installation negative paths all return the right codes. |
SvelteKit decodes the `[...path]` rest param, so `%2F` arrives as a literal `/` and the rebuilt URL splits GitHub chat IDs like `github:owner/repo:issue:N` across path segments — the daemon's `:chatId` route only matched `github:owner` and 404'd, leaving github chats in the sidebar but with empty message bodies. Forward `request.url`'s raw pathname instead of `params.path` so encoded `/` and `:` survive verbatim. The daemon's Hono routes already decode percent-encoded path params correctly, so no daemon-side change needed. Slack/Discord/Telegram thread IDs don't contain `/`, which is why this only surfaced now. Adds a regression test pinning that `%2F`/`%3A` round-trip through the proxy and land on the upstream URL exactly as sent.
Vpr99
reviewed
May 12, 2026
Contributor
Vpr99
left a comment
Vpr99
left a comment
There was a problem hiding this comment.
Five inline notes. Items here track to what's worth doing after a Karpathy-lens pass over my full review at docs/reviews/2026-05-12-github-communicator.md — the other findings in that doc are nitpicks I'd skip.
| .optional() | ||
| .describe("Placeholder for v2 proactive outbound (unused in v1)."), | ||
| }); | ||
| export type GitHubProviderConfig = z.infer<typeof GitHubProviderConfigSchema>; |
Contributor
There was a problem hiding this comment.
PR description defers proactive outbound to v2, but this schema ships default_destination anyway. Shipping it now locks the shape to string — v2 will likely want {repo, number} or similar. Suggest deleting GitHubProviderConfigSchema and letting pickConfigForKind("github", ...) return {}. Re-add with the right shape when v2 lands.
Contributor
Author
There was a problem hiding this comment.
Done in 7753d27 — GitHubProviderConfigSchema deleted~
| comment: { | ||
| id: 555, | ||
| body: "hey @friday-bot can you take a look?", | ||
| user: { id: 200, login: "alice", type: "User" }, |
Contributor
There was a problem hiding this comment.
bot_user_id is plumbed end-to-end to prevent the bot replying to itself, but no test asserts the guard fires. Add a case where comment.user.id === botUserId and assert processMessage is not called. The prod failure mode — infinite reply loop — is loud enough to earn a regression test.
| installation_id: 1, | ||
| }); | ||
| expect(result.success).toBe(true); | ||
| }); |
Contributor
There was a problem hiding this comment.
This test verifies the Zod regex accepts the PKCS1 header on a fake string, but no health() call ever runs against a real PKCS1 key. GitHub's .pem downloads default to PKCS1 — this is the operator-default install path. In beforeAll, also export the keypair as PKCS1 (keyObject.export({ format: 'pem', type: 'pkcs1' })) and add one health() test that uses it. ~5 lines.
| default_destination: "octo/repo", | ||
| }); | ||
| expect(parsed).toEqual({ kind: "github", default_destination: "octo/repo" }); | ||
| }); |
Contributor
There was a problem hiding this comment.
These two acceptance tests call schema.parse() and assert the input equals the output — they test Zod, not app behavior. Suggest deleting both. The rejection tests below stay (strictness is app behavior, since Link owns webhook_secret). Doubly true if you drop default_destination per the other comment.
| * returned `metadata.bot_user_slug` / `metadata.bot_user_id` are valid | ||
| * identifiers for the App's bot user. `webhook_secret` is **not** validated — | ||
| * there is no GitHub API surface to probe it; the first real webhook is the | ||
| * de-facto check. |
Contributor
There was a problem hiding this comment.
The first real webhook is the de-facto check framing isn't quite right — the provider never sees that webhook. The chat-sdk adapter validates the HMAC at signal time, after wiring is saved. A typo here silently 401s every inbound webhook with no setup-time signal.
Suggest: webhook_secret is not validated and a typo will silently 401 every inbound webhook — see Troubleshooting step 10.
`DefaultChatTransport` builds resume/stop URLs as `${api}/${id}/stream`
without URL-encoding `id`. GitHub chat IDs contain literal `/`
(e.g. `github:owner/repo:issue:N`), so the path splits and the daemon's
`:chatId` route 404s — surfaced as a "404 Not Found" banner at the
bottom of the chat page after the proxy decoding fix landed.
The AI SDK has no hook for URL path construction (`prepare*` methods
only customize body / api-base), and encoding upstream cascades through
the daemon's POST body lookups and the lossy `sanitizeKeyComponent` KV
keys. Wrap the transport's fetch instead and re-encode just the
`/chat/<rawId>` segment when the id isn't already URL-safe — a no-op
for atlas/slack/discord/telegram/whatsapp chat IDs.
Adds 5 unit tests covering string / URL / Request input forms, the
URL-safe no-op path, and the body-only POST endpoint.
…hema, add self-reply + PKCS1 tests) - Drop GitHubProviderConfigSchema and `default_destination` placeholder; re-add with the correct shape when v2 proactive outbound ships - Delete two Zod-only acceptance tests (strictness still covered by rejection tests) - Add regression test for the bot self-reply filter (comment.user.id === botUserId → processMessage not invoked) - Cover signAppJwt's PKCS1 fallback with a real PKCS1-formatted key in health() — operator-default install path - Correct the webhook_secret trust-contract comment to point operators at setupInstructions step 10
# Conflicts: # deno.lock
# Conflicts: # apps/atlasd/src/services/communicator-wiring.ts # tools/agent-playground/src/lib/components/chat/user-chat.svelte # tools/agent-playground/src/routes/api/daemon/[...path]/+server.ts
…ca-pool When the local origin is HTTPS (FRIDAY_TLS_CERT/KEY set), cloudflared has no entry for the private s2s CA in its system trust store, so its default cert verify against the webhook-tunnel listener fails and the public edge returns 502 to every inbound webhook. Pass the s2s CA bundle (FRIDAY_TLS_CA) as --origin-ca-pool so the loopback hop stays authenticated against the same CA the rest of the mesh trusts — preserves mutual-auth posture instead of disabling verify on that hop.
…-origin-ca-pool" This reverts commit 8978a38.
# Conflicts: # tools/agent-playground/src/lib/components/chat/connect-service.svelte # tools/agent-playground/src/lib/components/credential-secret-form.svelte # tools/agent-playground/src/lib/components/shared/communicator-apikey-form.svelte # tools/agent-playground/src/lib/use-credential-connect.svelte.ts
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
3 participants
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
githubcommunicator kind alongside slack/telegram/discord/teams/whatsapp. Workspaces declarecommunicators: { github: { kind: "github" } }; agents respond to@mentionsin Issues and PR comments via@chat-adapter/githubin multi-tenant mode — one GitHub App can serve many installations across many workspaces, body-routed byinstallation.idexactly like slack/teams/whatsapp route by their app/recipient/phone IDs.installation.idis the natural per-tenant routing key (mirrorsapp_idfor slack); Apps support per-repo scoping at install time so workspace owners don't need to manage allowlists in Friday. The existing PATgithubProviderin Link is untouched and continues to serve skills/agents calling the GitHub REST API directly.githubAppProvidervalidates credentials via a 3-call sequence (/app→/app/installations/{id}→/users/<slug>[bot]) and auto-capturesbot_user_slug(with literal[bot]suffix matching webhookcomment.user.login) + rename-immunebot_user_id; config schema gains thegithubliteral; atlasd factory branches build the adapter and/signals/githubroute resolves byinstallation.idvia thecommunicator_wiringtable.Test Plan
deno checkclean across all 16 touched filesdeno task lintclean on touched filesdeno task fmt— no fixes neededpackages/config— 5842 passed (schema parse + reject + workspace.yml round-trip)apps/link— 361 passed (12 new github-app provider tests: real RSA keypair signing, 3-call health, autoFields capture, all 4 negative paths)apps/atlasd— 1024 passed:installation.idcoercion, 400/404/500 paths, 401 propagation from adapterderiveConnectionIdgithub branch: 3 tests (numeric input, pre-stringified, missing field)resolveGithubFromLinkmapping: snake_case → camelCase,app_id: 12345 → appId: "12345",installation_iddropped (pins multi-tenant mode at the type level)apps/link/src/providers/github.tsbyte-identical tomain— zero migration risk for existing skills using GitHub PATsDesign
Full design:
docs/plans/2026-05-08-github-communicator-design.v2.mdOut of scope (deferred to v2)
default_destinationplaceholder shipped, unused)packages/bundled-agents/src/github/)webhook_secretactive validation (no GitHub API to probe; first real webhook is the smoke test)Known follow-ups
defineApiKeyProviderAPI has a soft architectural smell: providers needing server-derived data captured duringhealth()and surfaced viaautoFields()use module-scoped closure state. RefactoringautoFields(metadata)to receiveHealthResult.metadata(already in the type) would eliminate the closure and the concurrent-save race. Out of scope for this PR; documented indocs/learnings/.