convoy: GitHub kb sources

[albertoperdomo2]

Summary

Add a GitHub connector that wires GitHub's hosted remote MCP server (api.githubcopilot.com/mcp/) into a workspace as a knowledge base source. No repositories are cloned — agents read source over GitHub's hosted MCP using an encrypted personal access token.

What it does

• New per-connector module (lib/connectors/github.ts) with config parsing/validation, sanitizer, connection test, and spawner MCP config.
• Dedicated "Add connector" UI section for the PAT and pinned repositories.
• PAT is encrypted at rest and stripped from all API responses.
• "Pinned repositories" are injected into the workspace prompt (AGENTS.md) to guide agents toward the most relevant source. They are advisory, not an enforcement boundary — the real boundary is the token's scope (see ARCHITECTURE.md).

Review follow-ups addressed

• GitHub connectors can now be edited after creation — the PATCH merge preserves the stored PAT when the client omits it.
• Create/PATCH persist the normalized parsed config (trimmed PAT, deduped repos, no stray keys); pinned-repo dedup is case-insensitive.
• Removed dead return null in mcp/server-url.ts to keep the connector-type switch exhaustive.
• Spawn-time repository lookup now logs before falling back to an empty list.
• Pending repo input is committed on save instead of being silently dropped.
• Documented pinned-repos-as-advisory as an accepted risk in ARCHITECTURE.md.

[albertoperdomo2]

/build

[github-actions]

📦 PR Workspace Image Built Successfully

Default (amd64):

ghcr.io/peaberry-studio/arche/workspace:pr-384

Optional arm64:

ghcr.io/peaberry-studio/arche/workspace:pr-384-arm64

[github-actions]

📦 PR Image Built Successfully

Default (amd64):

ghcr.io/peaberry-studio/arche/web:pr-384

Optional arm64:

ghcr.io/peaberry-studio/arche/web:pr-384-arm64

[ixjosemi]

Deep review — patterns, security, bugs, performance

Full pass over the branch: read every changed file plus the surrounding connector/spawner architecture, ran the whole web test suite, tsc, eslint, and built + smoke-tested the workspace image with podman. Verification results at the bottom.

Overall: this follows the per-connector architecture with real discipline (dedicated lib/connectors/github.ts module, own UI section, sanitizer, validator, test handler, spawner MCP config), encryption-at-rest and response sanitization are done right, and test coverage is good at every layer. There is one functional blocker and a handful of small, well-scoped fixes below.

---

1. 🔴 Blocker — GitHub connectors cannot be edited after creation

sanitizeGithubConfigForResponse strips pat from every API response (correct), but the generic PATCH /connectors/[id] merge — mergeConnectorConfigWithPreservedOAuth in apps/web/src/lib/connectors/oauth-config.ts:95 — only preserves OAuth secrets. So any config the client can send back lacks pat, the merged config fails parseGithubConnectorConfig, and the PATCH is rejected with 400 Invalid GitHub connector configuration (validators.ts:99). There is also no edit UI or settings route for GitHub in this PR.

pinnedRepos is exactly the field users will want to change (add another repo), and today the only path is delete + recreate, re-pasting the PAT.

Suggested fix (either):

• Cheapest: in mergeConnectorConfigWithPreservedOAuth, preserve currentConfig.pat when connectorType === 'github' and nextConfig.pat is undefined — same pattern already used for oauthClientSecret at oauth-config.ts:106. ~5 lines + a test.
• Fuller: replicate the meta-ads approach (meta-ads-settings/ route + settings dialog with hasAppSecret-style handling) as a github-settings route.

2. 🟠 Security — document that pinnedRepos is not an enforcement boundary

• Pinned repos are only injected into the workspace prompt (AGENTS.md); they are not sent to the MCP server. The agent can read any repo the PAT can reach. The real boundary is the token's scope, and read-only depends on GitHub honoring X-MCP-Readonly.
• The raw PAT is embedded in the workspace MCP config, readable by any code the agent runs there. This follows the existing precedent for API-key connectors (Linear/Notion inject keys the same way, mcp-connector-config.ts:118), so it's not a regression — but a PAT is typically more powerful than a Notion key.

Suggested fix: no code change required. The UI helper text ("fine-grained token with read access limited to the repositories below") is the right mitigation — please also state this explicitly in ARCHITECTURE.md (or the connector docs) as an accepted risk: pinned repos are advisory; scope your PAT.

What's already done well here: PAT encrypted at rest, sanitized out of GET/PATCH responses, fixed MCP/test URLs (no SSRF surface — the explicit config.host rejection closes endpoint redirection), 8s timeout on the connection test, no token leakage in error messages.

3. 🟡 Dead code weakens exhaustiveness — lib/connectors/mcp/server-url.ts:46

The trailing return null after the switch is unreachable: Google types are narrowed out earlier by the isGoogleWorkspaceConnectorType type guard, and the switch covers the remaining union. Keeping it turns a future "forgot to handle new connector type" from a compile error into a silent null at runtime.

Suggested fix: delete the trailing return null. If tsc then complains, that's the exhaustiveness signal you want.

4. 🟡 Parse inconsistency + raw config persistence — lib/connectors/github.ts

parseGithubConnectorConfig rejects host specifically but ignores every other stray key, and the create/PATCH routes persist the raw config rather than the normalized parsed.config (trimmed pat, deduped repos). Via direct API calls, junk keys (e.g. a round-tripped hasPat) can end up stored. Not exploitable — every consumer re-parses before use — but it's an inconsistent contract.

Suggested fix: validate and persist parsed.config for GitHub. That removes the host special-case, the stored junk keys, and the trim/no-trim asymmetry in one move.

5. 🟡 Silent failure at spawn time — lib/spawner/runtime-artifacts.ts:117

getLinkedRepositoriesForOwner swallows everything, including a DB failure, and returns [] — the workspace starts without the "Linked Repositories" section and leaves no trace. The per-connector catch (corrupt config ⇒ skip, covered by tests) is fine; the outer catch should at least log before returning [].

6. ⚪ Minor

• Pending repo input dropped on save (add-connector/github/section.tsx): text typed into the repo input but not committed with Enter/Add is silently ignored when clicking "Save connector". Consider auto-committing pending input in getSubmission, or at least surfacing an error.
• Case-sensitive dedup: Acme/API and acme/api coexist in pinnedRepos although GitHub repo names are case-insensitive. Normalize with a case-insensitive comparison when deduping.
• PR description is outdated: it says "Enable shallow clones of GitHub repositories as knowledge base sources", but the implementation doesn't clone anything — it wires GitHub's hosted remote MCP. Please update the body so future readers don't hunt for clone logic that doesn't exist.

---

Verification performed

Check	Result
vitest (full web suite)	1609 tests, all green — 6 initial failures were environmental (prisma-desktop client not generated; pass after prisma:generate:all)
tsc --noEmit	0 errors in PR code (repo-wide pre-existing errors in test files only)
eslint (all 39 changed files)	Clean
GitHub CI	6/6 checks passing
Workspace image (podman)	Builds OK with OpenCode 1.17.9; container smoke test OK (git 2.54, node 24, opencode, workspace-agent, init-workspace.sh present). No image changes needed — the GitHub MCP is type: remote, consumed over HTTP by OpenCode

Bottom line: item 1 is the only blocker; items 3–5 are <20 lines combined. With those fixed and the description corrected, this is a clean example of how to extend the connector architecture.

[albertoperdomo2]

@ixjosemi do another pass buddy!

[ixjosemi]

Follow-up review — post-review commits

Reviewed the 4 commits pushed after the previous review (everything since 5094122e). Checked out the branch and re-verified.

TL;DR: All 6 items from the earlier review are addressed — including the blocker — with correct, well-tested implementations. 105/105 tests green across the affected files and tsc clean on all PR code. Good to approve, with two minor non-blocking notes.

Extra (not from the review): def6b6d4 removes compare_commits from the AGENTS.md prompt — it was a non-existent tool name in the GitHub Copilot MCP inventory. Correct, and no references remain.

Verification performed

• vitest over the 7 affected test files → 105 tests, all green. Solid new coverage: pat preservation (keeps the stored one + doesn't overwrite a provided one), case-insensitive dedup keeping the first spelling, and stray-key dropping (host, hasPat).
• tsc --noEmit → 0 errors in PR code.

Two minor notes (non-blocking)

1. DRY — includesRepo is duplicated verbatim in add-connector/github/section.tsx and github-connector-settings-dialog.tsx. Could be lifted into lib/connectors/github.ts next to isGithubPinnedRepo.

2. Latent toolsets fragility — the dialog PATCHes only { pinnedRepos }, and the merge preserves pat but not toolsets. Harmless today because toolsets isn't user-configurable and parseToolsets(undefined) re-applies the ['repos','git'] default. But if toolsets ever becomes editable, editing repos would silently reset it. Worth either preserving it in the merge alongside pat, or leaving an explicit comment.

Nice work turning the review around cleanly.

[ixjosemi]

lgtm!