feat(core): swap shared GitHub PAT for GitHub App with multi-installation routing#1788
Merged
Conversation
…tion routing Adds a new @archon/core/github-auth/ module wrapping @octokit/auth-app with a two-level cache (owner/repo → installationId, installationId → token) and refresh-on-access semantics — no background timers, no leaked handles. GitHubAdapter now takes a `GitHubAuth` discriminated union at construction time. All 4 Octokit callsites (postComment, listComments, repos.get, pulls.get) plus the clone-path token embed route through resolveOctokit + withTokenRefresh, which evicts the cached token and retries once on 401. The secondary self-filter compares against a new botLogin getter that returns `<slug>[bot]` in App mode (still botMention in PAT mode) so PR-C's per-user tokens won't trip it. Webhook `event.installation.id` primes the lookup cache to skip one HTTP round trip per event. Server bootstrap detects App vs PAT mode via env and fails fast if both are configured. In App mode it registers the provider on a module-level singleton that createWorkflowDeps() reads, so the workflow executor's bash/script subprocesses inherit a fresh GH_TOKEN/GITHUB_TOKEN. The new POST /internal/git-credential endpoint (App mode only) backs a POSIX git credential helper installed at clone time, covering workflows that outlive the 1h installation-token expiry. A startup WARN fires if the server binds to a non-loopback interface with App mode active — operators must drop /internal/* at the reverse proxy or bind 127.0.0.1. Backwards compat: solo installs running GITHUB_TOKEN only see zero functional change. Existing 54 PAT-mode adapter tests pass unchanged. Tests: 15 new strictly-mocked auth tests (PRD Q7 — no live api.github.com in CI); 8 new App-mode adapter tests covering multi-install routing, payload short-circuit, 401 retry, AppNotInstalledError surfacing, clone-token resolution, and post-clone credential helper install. Refs PRD: Phase 2 of .claude/PRPs/prds/github-app-and-user-identity.prd.md
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds GitHub App mode alongside PAT: public auth types and factory, private-key loader, token/installation caching, credential helper + installer, adapter refactor for mode-aware calls and 401 retry, server internal credential endpoint with startup guard, workflow token injection, tests, docs, and .env updates. ChangesGitHub App Authentication & Integration
Sequence Diagram: App-mode credential helper flow sequenceDiagram
participant Git as Git
participant Helper as git-credential-archon
participant Archon as Archon:/internal/git-credential
participant Provider as GitHubAppAuthProvider
participant GitHub as GitHub
Git->>Helper: credential request (action=get, host=github.com, path=owner/repo.git)
Helper->>Archon: POST /internal/git-credential { host, path }
Archon->>Provider: getInstallationToken(owner, repo)
Provider->>GitHub: POST /app/installations/:id/access_tokens
GitHub-->>Provider: { token, expires_at }
Provider-->>Archon: token
Archon-->>Helper: { token }
Helper-->>Git: username/password output (x-access-token/<token>)
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/core/src/github-auth/auth.test.ts (1)
219-255: ⚡ Quick winAdd one regression test for invalidation affecting
getOctokitForInstallation.Current tests validate token eviction and Octokit memoization independently, but not the combined 401-recovery behavior. A small test asserting a new instance after
invalidateToken(installationId)would protect this critical path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/github-auth/auth.test.ts` around lines 219 - 255, Add a regression test that ensures invalidateToken(...) triggers creation of a new Octokit instance via getOctokitForInstallation(...): create a provider with makeProvider (or default), obtain an Octokit instance with getOctokitForInstallation('o','r'), call invalidateToken(installationId) for that installation, then call getOctokitForInstallation('o','r') again and assert the second instance is not the same as the first; use/mock mockRequest token responses as needed to simulate token rotation and reference invalidateToken, getOctokitForInstallation, makeProvider and mockRequest to locate the relevant code under test.packages/server/src/index.ts (1)
629-663: 💤 Low valueStatic analysis false positive — this is
RegExp.exec(), notchild_process.exec().The OpenGrep hint flagging "dynamic command passed to child_process.exec" at line 649 is incorrect. The code uses
RegExp.prototype.exec()to parse the owner/repo path, notchild_process.exec().Optional: Consider Zod for request body validation.
Lines 639-642 use a manual type cast for the request body. Per coding guidelines, Zod schemas are preferred for request validation to get runtime type safety.
♻️ Optional: Zod schema for body validation
import { z } from 'zod'; const gitCredentialRequestSchema = z.object({ host: z.string().optional(), path: z.string().optional(), }); // Inside the handler: const parseResult = gitCredentialRequestSchema.safeParse(await c.req.json().catch(() => null)); if (!parseResult.success || parseResult.data.host !== 'github.com') { return c.json({ error: 'unsupported host' }, 400); } const pathStr = parseResult.data.path ?? '';As per coding guidelines: "Use Zod schemas with camelCase naming and descriptive suffixes; always derive types using
z.infer<typeof schema>instead of hand-crafted parallel interfaces."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/index.ts` around lines 629 - 663, The scan flagged a false positive: the code in the handler registered by app.post('/internal/git-credential') uses RegExp.prototype.exec() to parse the owner/repo path (see the /^([^/]+)\/([^/]+?)(?:\.git)?$/ .exec(pathStr) usage) — not child_process.exec(); leave that as-is. Separately, replace the manual cast of the request body ((await c.req.json().catch(() => null)) as { host?: string; path?: string } | null) with a Zod-based runtime validation: define a gitCredentialRequestSchema (z.object({ host: z.string().optional(), path: z.string().optional() })), use safeParse on the parsed JSON, and then read parseResult.data.host/path and return the same error responses if validation fails or host !== 'github.com'; keep the rest of the handler (owner/repo extraction, github.getInstallationToken(owner, repo), error handling) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Line 94: The .env.example has an inline trailing comment on the empty
GITHUB_TOKEN assignment which can confuse dotenv parsers; update the file so
GITHUB_TOKEN= is on its own line with no inline comment and move the explanatory
comment into a separate line above (or below) the GITHUB_TOKEN entry so the
value is unambiguously empty (referencing the GITHUB_TOKEN variable in the
file).
In `@packages/core/src/github-auth/auth.ts`:
- Around line 107-137: The invalidateToken function only deletes tokenCache but
must also evict the per-installation Octokit so retries create a fresh auth
context; update invalidateToken to remove the Octokit instance from octokitCache
(e.g., octokitCache.delete(installationId)) and add a small log entry (using
getLog()) indicating the octokit cache was evicted for the installationId,
ensuring getOctokitForInstallation will construct a new Octokit on subsequent
calls.
In `@packages/docs-web/src/content/docs/adapters/github-app-setup.md`:
- Around line 170-173: Update the "### 401 loop in logs" section to replace the
webhook-secret explanation with authentication/installation causes: explain that
repeated GitHub API 401s are typically due to installation-token or auth issues
(app not installed on the target repo/org, missing or insufficient app
permissions, revoked/expired credentials, or incorrect token handling) and
provide steps to verify the app installation and token validity; move the
existing note about the webhook secret/WEBHOOK_SECRET mismatch into the
signature verification/delivery failures section (or a new subsection) and
clearly label it as relevant to webhook signature verification rather than API
401 errors.
In `@scripts/git-credential-archon.sh`:
- Around line 39-40: The curl call that assigns resp can hang indefinitely and
should include timeouts; update the curl invocation (the line that sets resp) to
add sensible timeout flags such as --connect-timeout 2 and --max-time 5 (or
other project-appropriate values) so the request fails fast if the Archon
endpoint is unreachable, keeping the existing || exit 0 behavior intact; ensure
the modified command still uses -fsS and the same headers/data and captures the
response into resp.
---
Nitpick comments:
In `@packages/core/src/github-auth/auth.test.ts`:
- Around line 219-255: Add a regression test that ensures invalidateToken(...)
triggers creation of a new Octokit instance via getOctokitForInstallation(...):
create a provider with makeProvider (or default), obtain an Octokit instance
with getOctokitForInstallation('o','r'), call invalidateToken(installationId)
for that installation, then call getOctokitForInstallation('o','r') again and
assert the second instance is not the same as the first; use/mock mockRequest
token responses as needed to simulate token rotation and reference
invalidateToken, getOctokitForInstallation, makeProvider and mockRequest to
locate the relevant code under test.
In `@packages/server/src/index.ts`:
- Around line 629-663: The scan flagged a false positive: the code in the
handler registered by app.post('/internal/git-credential') uses
RegExp.prototype.exec() to parse the owner/repo path (see the
/^([^/]+)\/([^/]+?)(?:\.git)?$/ .exec(pathStr) usage) — not
child_process.exec(); leave that as-is. Separately, replace the manual cast of
the request body ((await c.req.json().catch(() => null)) as { host?: string;
path?: string } | null) with a Zod-based runtime validation: define a
gitCredentialRequestSchema (z.object({ host: z.string().optional(), path:
z.string().optional() })), use safeParse on the parsed JSON, and then read
parseResult.data.host/path and return the same error responses if validation
fails or host !== 'github.com'; keep the rest of the handler (owner/repo
extraction, github.getInstallationToken(owner, repo), error handling) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 225c5038-2897-4cf2-b499-72d12f4eadc3
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.env.exampleCLAUDE.mdpackages/adapters/src/forge/github/adapter.test.tspackages/adapters/src/forge/github/adapter.tspackages/adapters/src/forge/github/context.test.tspackages/adapters/src/forge/github/types.tspackages/core/package.jsonpackages/core/src/github-auth/auth.test.tspackages/core/src/github-auth/auth.tspackages/core/src/github-auth/credential-helper-install.tspackages/core/src/github-auth/errors.tspackages/core/src/github-auth/index.tspackages/core/src/github-auth/private-key.tspackages/core/src/github-auth/types.tspackages/core/src/index.tspackages/core/src/workflows/store-adapter.tspackages/docs-web/src/content/docs/adapters/github-app-setup.mdpackages/docs-web/src/content/docs/adapters/github.mdpackages/server/src/index.tspackages/workflows/src/deps.tspackages/workflows/src/executor.tsscripts/git-credential-archon.sh
Collaborator
Author
PR Review Summary — 7 specialized agentsTargeted review focused on the 5 concerns raised on PR-B plus general quality. Findings cross-validated across agents; consensus items flagged. Direct answers to the 5 questions you flaggedQ1. /internal/git-credential loopback guard — defensive enough? if (githubAppAuthProvider && hostname !== '127.0.0.1' && hostname !== 'localhost') {
if (!process.env.ARCHON_ALLOW_INTERNAL_ON_PUBLIC_BIND) {
getLog().fatal({ hostname }, 'github_app.internal_endpoint_public_bind_rejected');
process.exit(1);
}
getLog().warn({ hostname }, 'github_app.internal_endpoint_exposed_check_reverse_proxy');
}Defense-in-depth alternative: bind Q2. withTokenRefresh 401 retry — infinite-loop risk?
Q3. PEM
Q4. Cache TTLs — 55-minute mark behavior, correctness? Lookup cache 1h TTL is the right call — but only if Q2's Q5. Credential helper shell script — injection risk?
Critical issues (must fix before merge)
Important issues
Test coverage gaps
Suggestions
Strengths acknowledged
Verdict — NEEDS FIXESC1 and C2 are the load-bearing concerns:
C3 and C4 are observability defects that will burn debugging time on real production incidents. The test gaps T1 + T2 leave the riskiest new surface (the token-vending endpoint, the dual-mode guard) unverified — both are short tests to add and would catch regressions in the security-critical paths. The structural design is sound: the discriminated union, the fail-fast on dual config, the two-level cache architecture, and the 401-evict-retry shape are all the right calls. Fixes are localized and don't require rework. 🤖 Generated by |
…, lookupCache eviction, retry observability PR-B follow-up addressing the multi-agent review on #1788. Fixes 4 critical defects + 7 important defects + 5 test gaps. Non-blocking suggestions (drop octokitCache, split /internal/* to separate listener) skipped as YAGNI. Critical: • C1 — /internal/git-credential bind safety: replace startup WARN with fail-fast exit when App mode is active and hostname != loopback. Opt-out via ARCHON_ALLOW_INTERNAL_ON_PUBLIC_BIND=1 for deployments where the reverse proxy already drops /internal/*. • C2 — lookupCache eviction on 401: add invalidateRepo(owner, repo) that drops BOTH caches in one call. Adapter's withTokenRefresh uses it instead of the by-id invalidator. An App reinstall (which assigns a NEW installation id) used to be served from the stale lookupCache for up to 1h; now the next call re-resolves the install. • C3 — withTokenRefresh retry observability: wrap fn(fresh) in try/catch with an explicit ERROR log carrying owner/repo + firstStatus + retryStatus. Bounded retry (no third attempt) — second consecutive failure propagates. • C4 — /internal/git-credential failure log upgraded WARN→ERROR. Live credential vending failure should not be filtered as noise. Important: • I1 — installCredentialHelper returns discriminated result {installed|skipped|failed}. Adapter logs the actual outcome instead of false-success in compiled binary builds that ship without scripts/. • I2 — getInstallationTokenById wraps POST in try/catch with ERROR log carrying installationId + HTTP status so the dead installation is greppable. • I3 — git-credential-archon.sh adds explicit case "$path" in */*) check so malformed paths fall through cleanly. • I4 — script echoes one-line stderr diagnostics before each exit 0 fall-through. Unattended workflow now sees the root cause (non-github host, malformed path, curl failure, empty response) instead of "auth failed" with no context. • I5 — PEM CRLF normalization extended to both the inline branch (\\r\\n escape) and the file-load branch. Windows-edited .pem files work. • I6 — auth.ts header no longer lies about lookupCache being refreshed on 401 (paired with C2). • I7 — adapter.ts secondary self-filter comment drops "PR-C" reference; states the actual invariant. • I8 — credential-helper-install.ts header drops "this PR" reference; states the actual condition (source script must exist on disk). Refactor + suggestions taken: • CachedInstallationToken.expiresAt renamed → expiresAtMs. • createGitHubAppAuthProvider validates appId + slug non-empty at the factory boundary, matching loadAppPrivateKey's eager validation. • Dual-mode env detection + /internal path parsing extracted into pure helpers in server/src/github-auth-bootstrap.ts (so the security-critical decisions are testable in isolation). Tests: • T1 — parseGitCredentialPath unit tests (12 cases): owner/repo, .git suffix, single-segment rejection, deep route rejection, .. traversal, null bytes, leading slash, case preservation, GitHub-valid identifiers. • T2 — selectGitHubAuthMode unit tests (8 cases): app, pat, none, incomplete-config rejection, conflict-with-actionable-message. • T3 — withTokenRefresh second-401 propagation: single retry, no infinite loop, single invalidateRepo call. • T5 — PEM CRLF normalization (2 cases: inline \\r\\n, file CRLF). • T6 — lookupCache 1h TTL boundary with fake Date.now. • Existing 401-retry test updated to assert against invalidateRepo (C2 regression guard). Suggestions not adopted: • Drop octokitCache — currently working + tested; reviewer cites no measured cost. KISS keep. • Split /internal/* to a separate loopback-only listener — overengineering when fail-fast bind check + reverse-proxy doc cover the threat model. • shellcheck on the credential helper — would require CI infra for a 30-line POSIX script with one branch. Defer. • Remove !appId/!webhookSecret guards via ! non-null assertions — lint forbids ! assertions; my defensive guards are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/index.ts (1)
747-777:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the public-bind guard before
Bun.serveto make this truly fail-fast.Right now the process binds and logs
server_listeningbefore the guard can throw, so/internal/git-credentialmay be briefly reachable on startup in rejected configs. Validatehostnameand App mode before creating the server.Based on learnings: "Prefer throwing early with a clear error for unsupported/unsafe states — never silently swallow errors" and "Never silently broaden permissions or capabilities. ... otherwise throw (Fail Fast + Explicit Errors)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/index.ts` around lines 747 - 777, The public-bind safety check is executed after creating the server (Bun.serve) and logging server_listening, which can briefly expose /internal/git-credential; move the guard that checks githubAppAuthProvider and hostname (and the ARCHON_ALLOW_INTERNAL_ON_PUBLIC_BIND logic that calls getLog().warn/getLog().fatal and throws) to run before calling Bun.serve and before getLog().info so the process fails fast and never binds to a non-loopback interface in rejected configurations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/github-auth/auth.test.ts`:
- Around line 324-329: The test currently overrides Date.now directly and uses
an inline eslint-disable comment; instead remove the disable and replace the
direct assignment with a lint-safe preserve/restore pattern (e.g., use
jest.spyOn(Date, 'now').mockImplementation(() => mockedNow) or a small typed
helper like preserveAndMockDateNow that captures realNow, sets the mock, and
restores it in afterEach). Update the code referencing realNow, baseTime, and
mockedNow to use that spy/helper and ensure the spy is restored (mockRestore or
helper cleanup) after the test so no inline eslint-disable is needed.
---
Outside diff comments:
In `@packages/server/src/index.ts`:
- Around line 747-777: The public-bind safety check is executed after creating
the server (Bun.serve) and logging server_listening, which can briefly expose
/internal/git-credential; move the guard that checks githubAppAuthProvider and
hostname (and the ARCHON_ALLOW_INTERNAL_ON_PUBLIC_BIND logic that calls
getLog().warn/getLog().fatal and throws) to run before calling Bun.serve and
before getLog().info so the process fails fast and never binds to a non-loopback
interface in rejected configurations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75ce516e-a8a8-49b8-8bd8-4c09ffe38803
📒 Files selected for processing (15)
.env.exampleCLAUDE.mdpackages/adapters/src/forge/github/adapter.test.tspackages/adapters/src/forge/github/adapter.tspackages/core/src/github-auth/auth.test.tspackages/core/src/github-auth/auth.tspackages/core/src/github-auth/credential-helper-install.tspackages/core/src/github-auth/private-key.tspackages/core/src/github-auth/types.tspackages/docs-web/src/content/docs/adapters/github-app-setup.mdpackages/server/package.jsonpackages/server/src/github-auth-bootstrap.test.tspackages/server/src/github-auth-bootstrap.tspackages/server/src/index.tsscripts/git-credential-archon.sh
✅ Files skipped from review due to trivial changes (2)
- packages/server/package.json
- .env.example
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/core/src/github-auth/types.ts
- CLAUDE.md
- scripts/git-credential-archon.sh
- packages/core/src/github-auth/auth.ts
- packages/adapters/src/forge/github/adapter.test.ts
- packages/core/src/github-auth/private-key.ts
- packages/adapters/src/forge/github/adapter.ts
CR1 — .env.example: move inline comment off GITHUB_TOKEN= line so dotenv parsers don't misread the value as `# Same as GH_TOKEN...`. CR2 — auth.ts: invalidateToken AND invalidateRepo now also evict octokitCache. Per-installation Octokit holds its own internal createAppAuth token state; without this eviction the "fresh" Octokit handed to the retry path could keep serving the dead token from the SDK's private cache. Paired with cascading lookupCache eviction already added in b35d9bd. CR3 — github-app-setup.md: "401 loop" section reworked. API 401s are caused by installation / token / permission issues, not webhook secret. Webhook-secret mismatch causes signature-verification failures at the inbound webhook endpoint — split into its own section under "Webhook deliveries fail signature verification". CR4 — git-credential-archon.sh: add `curl --connect-timeout 2 --max-time 5` so git doesn't block indefinitely when Archon isn't listening. Existing fail-soft (`exit 0` on any error) preserved. CR5 — auth.test.ts T6: replace direct Date.now reassignment + eslint-disable with spyOn(Date, 'now').mockImplementation pattern; restore via mockRestore in finally. Lint-clean. CR6 — server/index.ts: move the public-bind fail-fast guard ABOVE Bun.serve so a rejected config never opens the listening socket. Was previously after Bun.serve + server_listening log, briefly exposing /internal/git-credential during the throw window. CR7 — server/index.ts: replace manual type cast on /internal/git- credential request body with a Zod schema (`gitCredentialRequestSchema = z.object({ host: z.string().optional(), path: z.string().optional() })`) and safeParse. Per CLAUDE.md's "always Zod for request validation" convention. Inline declaration because the endpoint is a one-off internal surface (not OpenAPI-published). CR8 — auth.test.ts: regression tests covering CR2. invalidateToken and invalidateRepo both verified to evict the Octokit instance — the next getOctokitForInstallation() returns a brand-new Octokit. Without CR2 these tests fail. All findings addressed; no items skipped.
|
Actionable comments posted: 0 |
Collaborator
Author
Re-review after b35d9bd + 429997fReviewed against the 4 critical + 8 important + 6 test-gap findings from the previous comment. Verified each fix against the actual code on disk, not just commit messages. Critical findings — all resolved
Important findings — all resolved
Test gaps — all closed
CodeRabbit follow-ups — verified
Suggestions not adopted — agree these are YAGNI
One minor observation (not blocking)The Verdict — READY TO MERGEAll 4 critical findings resolved with stronger fixes than I suggested (octokitCache cascade, pre-listen guard placement, defense-in-depth at three layers — adapter / endpoint / shell). All 8 important findings resolved. 5 of 6 test gaps closed (T4 reasonably deferred). CodeRabbit's 8 follow-ups addressed. CI green (ubuntu, windows, docker-build all pass). The structural design is unchanged from PR-B's original shape; the fixes are localized to the auth module, adapter, server bootstrap, and shell helper. Discriminated union remains exhaustively narrowed. Fail-fast principle now correctly applied at every load-bearing decision point. 🤖 Generated by |
…Octokit) Updates the auth.ts header to acknowledge that the cache is effectively three-level, not two. The third layer — `octokitCache: installationId → Octokit` — was already there, but its load-bearing role for 401 retry correctness wasn't documented. Each Octokit instance owns its own private createAppAuth token state, opaque to us; without an octokitCache.delete(id) the "fresh" Octokit handed to the retry path keeps serving the dead token from the SDK's hidden cache, and our visible cache evictions do nothing. References the CodeRabbit comment on PR #1788 that surfaced the issue, so the next reader can trace why invalidateToken / invalidateRepo delete from three caches rather than two. Comment-only change — type-check + tests green.
|
Actionable comments posted: 0 |
This was referenced May 28, 2026
Merged
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
GITHUB_TOKENPAT. One revocation kills everyone, comments all post under one operator's avatar, no auto-rotation, no multi-org support, and webhooks need per-repo setup.user_idplumbing — PR-B replaces the bot identity.@archon/core/github-auth/module wraps@octokit/auth-appwith a two-level cache (owner/repo → installationId,installationId → token, refresh-on-access with a 5-min buffer).GitHubAdaptertakes aGitHubAuthdiscriminated union ({kind:'pat'}|{kind:'app'}); the 4 Octokit callsites + the clone token embed route through per-(owner, repo)resolveOctokit+ awithTokenRefreshwrapper that evicts on 401 and retries once. Server bootstrap fails fast if both modes are configured. NewPOST /internal/git-credentialendpoint (App mode only) backs a POSIX git credential helper installed at clone time so workflows that outlive the 1h token can refresh in place.requires:[github]YAML gate, interim web UI auth, CLIarchon auth github— all PR-C. GitHub Enterprise Server, installation lifecycle webhook events, live integration tests against api.github.com, token-encryption-key rotation tooling — out of scope per PRD.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
loadAppPrivateKey()createGitHubAppAuthProvider()registerGitHubAppAuthProvider()GitHubAdapterconstructorGitHubAuthdiscriminated union (wastoken: string)GitHubAdapterIGitHubAppAuthProviderGitHubAdapter(4 Octokit sites)withTokenRefresh→resolveOctokitthis.octokit.rest.XGitHubAdapter.handleWebhookprimeInstallationLookupGitHubAdapter.ensureRepoReadygetInstallationTokenGitHubAdapter.ensureRepoReadyinstallCredentialHelperstore-adapter.createWorkflowDeps()IGitHubAppAuthProvider.getInstallationTokenWorkflowDeps.resolveBotGitHubTokenworkflows/executor.executeWorkflowdeps.resolveBotGitHubTokenGH_TOKEN/GITHUB_TOKENat workflow startPOST /internal/git-credentialGitHubAdapter.getInstallationTokengit-credential-archon.shPOST /internal/git-credentialLabel Snapshot
risk: mediumsize: Lcore(github-auth module is the heart); also touchesadapters,server,workflows,docscore:github-auth,adapters:github,server:bootstrap,workflows:executorChange Metadata
feature(also touchesrefactorfor the adapter callsites)multi—core(new module + workflow injection wire-up),adapters(GitHubAdapter dual-mode),server(bootstrap + internal endpoint),workflows(subprocess env injection)Linked Issue
user_idfoundation; merged on dev as50beeeca)Validation Evidence (required)
bun run validatestages exit 0 locally; 15 new strictly-mocked auth-module tests (packages/core/src/github-auth/auth.test.ts); 8 new App-mode adapter tests + all 54 existing PAT-mode tests passing (packages/adapters/src/forge/github/adapter.test.ts). No live api.github.com calls in CI per PRD Q7.Security Impact (required)
packages/docs-web/.../github-app-setup.md.@octokit/auth-appmakes a JWT-signed call toGET /repos/{owner}/{repo}/installation(cached 1h) andPOST /app/installations/{id}/access_tokens(cached, refreshed 5min before 1h expiry). Both are standard App-auth flows.installCredentialHelper()copiesscripts/git-credential-archon.shto~/.archon/bin/and writes a per-worktreegit config credential.https://github.com.helperentry. Idempotent; only on App-mode clone./internal/git-credentialendpoint hands out live installation tokens. MUST be exposed on 127.0.0.1 only OR the reverse proxy MUST drop/internal/*. Server emits a startup WARN (github_app.internal_endpoint_exposed_check_reverse_proxy) if bound to a non-loopback interface with App mode active. Documented inCLAUDE.mdAPI endpoints section and the operator setup guide.Compatibility / Migration
GITHUB_APP_ID,GITHUB_APP_PRIVATE_KEY(or_PATH),GITHUB_APP_SLUG,GITHUB_APP_INSTALLATION_ID. Documented in.env.example+github-app-setup.md.GITHUB_TOKENcontinues to work.GITHUB_APP_IDandGITHUB_TOKENare set. Operators must pick one mode explicitly.packages/docs-web/src/content/docs/adapters/github-app-setup.mdStep 6.Human Verification (required)
What was personally validated beyond CI:
bun run validategreen end-to-end (all 6 stages).\nnormalisation, missing-env, bad-PEM); token cache (fresh hit, refresh-near-expiry, default-installation-id skip); install lookup (cache, case-insensitive, 404→AppNotInstalledError); priming; invalidation; per-install Octokit memoisation; multi-install Octokit distinctness.event.installation.idpriming, 401→invalidate+retry,<slug>[bot]self-filter, clone-token resolution (vs env),AppNotInstalledErrorsurfacing, credential-helper install attempted post-clone.AppPrivateKeyErrorat bootstrap, not at first webhook.\n(the.env-quoted convention) → normalised on load.(owner, repo)from two different installations → routed to distinct tokens.event.installationabsent (older / replayed webhook) → falls back to lookup path.api.github.comintegration (deferred per PRD Q7). TheinstallCredentialHelperflow with an actual >1h workflow wasn't end-to-end exercised — covered by unit-level test for the post-clonegit configinvocation, but the real-world refresh path needs a long-running workflow on a deployed instance.Side Effects / Blast Radius (required)
/internal/git-credentialexposed beyond loopback would hand out live tokens — guarded by startup WARN and operator documentation.installCredentialHelperwrites per-worktree git config; idempotent and scoped tocredential.https://github.com.helperso it doesn't collide with operator-set helpers.git push;ghcommands hitting the API after expiry would 401 (operators handle via the helper or by keeping workflows short).github.adapter_mode_appvsgithub.adapter_mode_patmake the active mode obvious.github_auth.install_lookup_completed,github_auth.token_resolve_completed,github_auth.token_cache_evicted_on_401give visibility into token lifecycle.github_app.internal_endpoint_exposed_check_reverse_proxystartup WARN flags misconfigured binds.Rollback Plan (required)
GITHUB_APP_*env vars, setGITHUB_TOKENback, restart Archon. Adapter snaps back to PAT mode. No data migration to undo (no schema changes).github_auth.token_resolve_failedorgithub_auth.install_lookup_failedrepeatedly.<slug>[bot].AppNotInstalledError.Risks and Mitigations
/internal/git-credentialexposed beyond loopback hands out live installation tokens.github-app-setup.md; CLAUDE.md API table flags it as 127.0.0.1-only..envfiles.loadAppPrivateKeysupports both literal newlines and the\nescape; two unit tests cover both paths.invalidateToken+ single retry surfacesAppNotInstalledErrorcleanly if the install really is gone.git pushafter the 1h URL-embedded token expires.installCredentialHelperpost-clone in App mode;POST /internal/git-credentialreturns a fresh token on demand.@octokit/auth-appv8 ESM-only — Bun import resolution.@octokit/restv22 (the matching peer, already used in@archon/adapters) is also ESM-only; the same path works. Verified locally via the new auth tests.Summary by CodeRabbit
New Features
Configuration
Documentation
Tests