feat(github): MVP commit-as-user via GitHub App user-to-server tokens#3209
feat(github): MVP commit-as-user via GitHub App user-to-server tokens#3209kilo-code-bot[bot] wants to merge 14 commits into
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental Update (commits since 222b013)New commits since last review:
Files changed since
All Previous Issues — Status
Other Observations (not in diff)
Files Reviewed (51 files)
Reviewed by claude-sonnet-4.6 · 871,626 tokens Review guidance: REVIEW.md from base branch |
Code ReviewBug (must fix):
|
There was a problem hiding this comment.
One correctness bug must be fixed before merge (see inline comment above).
Bug: startPreparationAsync in session-prepare.ts does not pass identityKiloUserId to the Durable Object's PreparationInput. The user-token path in executePreparationSteps (async-preparation.ts) is gated on input.identityKiloUserId being set, so user-token resolution is silently skipped on every autoInitiate=true session (the normal production fast path). All sessions on the async path fall back to the app bot token even when ENABLE_GITHUB_USER_TOKENS=true.
Fix: Add identityKiloUserId: ctx.userId to the startPreparationAsync call block in session-prepare.ts (mirroring what the sync prepare() call already does at line ~691).
A secondary non-blocking style issue: the double as-cast in getOctokitStatus (user-github-token-service.ts) should use flow-sensitive typing with a single narrowing cast instead.
|
Fixed both issues:
|
7966ae6 to
1cc4af8
Compare
|
Refinery code review passed. All quality gates pass. Summary: The MVP commit-as-user implementation is well-structured and secure. The OAuth state is properly HMAC-signed with TTL, tokens are encrypted at rest with a dedicated key, the GDPR soft-delete is covered, and the feature is safely gated behind Minor observations (no changes required):
Both are minor and do not affect correctness or security. Safe to merge. |
c8103bf to
3bc6d77
Compare
|
Refinery code review passed. All quality gates pass. |
d8c7da9 to
e393c62
Compare
|
Refinery code review passed. All quality gates pass. Reviewed the full diff across all 40 changed files. The implementation is correct and well-structured: Schema & Migration: Security: HMAC-SHA256 signed OAuth state with 10-minute TTL and timing-safe comparison; AES-256-GCM encrypted token storage; GDPR: Feature gate: Fallback logic: Token selection in both Worker module-scope safety: Test coverage: Route callback, oauth-state, github-apps-router, session-prepare (gate on/off, fallback), git-identity builder, and async-preparation all have meaningful tests. Mocks are used only where truly necessary (external OAuth and Octokit APIs). WIP commit ( One minor style note (non-blocking): the blank line between the import block and type declaration was removed in |
Refinery Code ReviewOverall the implementation is solid — schema, GDPR deletion, HMAC-signed state, AES-256-GCM encryption, feature gate, and tests are all well done. Three issues need addressing before merge. 🔴 Issue 1:
|
|
Refinery code review passed. All quality gates pass. Reviewed the full diff across all 39 changed files. The implementation is well-structured:
No issues found. |
|
Refinery code review passed. All quality gates pass. All previously identified issues from prior review rounds have been resolved:
Implementation is correct and complete across all three MVPs. |
|
Refinery code review passed. All quality gates pass. |
|
Refinery code review passed. All quality gates pass. All previously raised issues have been addressed:
Code is correct, secure (no token logging, encryption keys isolated, GDPR soft-delete updated), and gated off by default ( |
Refinery Code ReviewThe PR is well-structured with good test coverage, correct HMAC state signing/verification, proper GDPR soft-delete coverage, clean feature-gate approach, and correct fallback behavior. Two issues should be fixed before merging. 🔴 Issue 1 — Dynamic import in hot pathFile:
import { decryptWithSymmetricKey } from '@kilocode/encryption';Remove the 🔴 Issue 2 — Token redaction missing in logged GitHub error responseFile: The error log at import { redactGitHubTokens } from '@kilocode/worker-utils/redact-headers';
// ...in the error handler:
console.error('Failed to revoke GitHub authorization:', {
status: response.status,
body: redactGitHubTokens(await response.text()),
});🟡 Minor note — Feature-gate not applied to query/mutation endpoints
|
Refinery Code ReviewThree issues found. One is a clear code quality problem (must fix); two are minor MVP tradeoffs documented here for follow-up. 🔴 Must Fix — Dynamic
|
There was a problem hiding this comment.
Two env-var documentation issues that will cause silent failures or confuse local dev setup. No logic or security bugs found in the core implementation — the OAuth flow, encryption, GDPR handling, token refresh fallback, and feature gating all look correct.
eaf3aea to
a88ba34
Compare
* feat(db): add user_github_app_tokens table with enums and migration * chore(env): plumb USER_GH_APP_TOKEN_ENCRYPTION_KEY across web and git-token-service * feat(worker-utils): add redactGitHubTokens for ghu_/ghr_ patterns * fix: address PR review - GDPR soft-delete gap and encryption key in vars * fix: use correct column name github_app_type in test --------- Co-authored-by: Toast (gastown) <Toast@gastown.local>
…serTokenForRepo RPC (#3203) * feat(github): MVP-2 connect mutation, callback, settings UI, and getUserTokenForRepo RPC * fix(git-token-service): add missing deps, fix timestamp type, typecheck passes * fix(web): fix typecheck errors in MVP-2 code - Use 'expiresAt' in auth narrowing for GitHubAppAuthentication union type - Fix updated_at to use toISOString() (timestamp mode: 'string') - Fix access_token_expires_at in user.test.ts to use toISOString() - Add missing eq import in github-apps-router.test.ts - Narrow userConnectionStatus.login access with 'login' in guard - Apply oxfmt formatting * fix: remove unused imports flagged by code review --------- Co-authored-by: Toast (gastown) <Toast@gastown.local> Co-authored-by: Maple (gastown) <Maple@gastown.local>
…ity-aware author config + env-var gate (#3201) * feat(cloud-agent-next): MVP-3 user-token selection, identity-aware git author, env-var gate - Add GitIdentity discriminated union (app | user) in types/git-identity.ts - Add buildGitAuthor() to workspace.ts for identity-aware git user.name/email - Update cloneGitHubRepo() to accept GitIdentity or legacy env-obj - Gate user-token preference behind ENABLE_GITHUB_USER_TOKENS env var - session-prepare.ts: try getUserTokenForRepo first when gate=true, fall back to app token - async-preparation.ts: same token-selection logic for autoInitiate (async) path - Persist identityKind + identityKiloUserId in DO session state (schemas, types, prepare()) - CloudAgentSession.refreshGitHubToken: mid-session user-token renewal; on failure falls back to app installation token, emits one-time warning, downgrades persisted identityKind - Both startExecutionV2 token paths (initiatePrepared + followup) use refreshGitHubToken - Add ENABLE_GITHUB_USER_TOKENS to wrangler.jsonc (default false) and .dev.vars.example - Add getUserTokenForRepo to GitTokenService type in types.ts - Tests: buildGitAuthor snapshots, token-selection with gate on/off, fallback cases * fix(cloud-agent-next): address PR review feedback - Replace as-cast on caught Octokit error with type guard in user-github-token-service - Fix mid-session app-token fallback when githubInstallationId is absent (sessions prepared via user-token path don't store installationId; after identity downgrade to 'app', resolve a fresh installation token via resolveGitHubTokenForRepo) --------- Co-authored-by: Toast (gastown) <Toast@gastown.local> Co-authored-by: Maple (gastown) <Maple@gastown.local>
- Replace `as any` in github-apps-router.test.ts with properly typed user via defineTestUser helper; insert user to DB in beforeEach to satisfy FK constraint on user_github_app_tokens - Replace inline import() type annotations in git-token-service/index.ts with explicit named type imports from user-github-token-service.ts - Remove unused GITHUB_OAUTH_STATE_SECRET import from oauth-state.test.ts - Reduce double `as` cast in user-github-token-service.ts to a single targeted cast after narrowing with 'status' in e check - Sanitize user-controlled error_description before logging to prevent log injection in github/user-connect/callback/route.ts - Run oxfmt on all changed files
…ipping in sanitizeForLog
fix(session-prepare): forward identityKind and identityKiloUserId to startPreparationAsync The test fixtures in github-apps-router.test.ts and route.test.ts used a base64 key that decoded to 35 bytes, causing EncryptionConfigurationError. Replace with a valid 32-byte key. The async fast-path (autoInitiate=true) in session-prepare.ts was not forwarding identityKind/identityKiloUserId to startPreparationAsync. Since resolvedGitIdentity is resolved before the autoInitiate branch, we can forward it directly, mirroring what the sync prepare() call does.
….test.ts The user_github_app_tokens table has a FK constraint on kilo_user_id referencing kilocode_users. Tests were failing with a FK violation because no user was inserted before the token rows. Add insertTestUser call in beforeEach to satisfy the constraint.
…st.ts
NextResponse.redirect with a full URL (constructed via new URL(path, base))
returns the absolute URL in the Location header. The test was asserting
.toBe('/account/integrations') which never matches the full URL. Use
.toContain('/account/integrations') instead.
…base conflict with 0130
…drop body from revoke error log
222b013 to
c1bf1f0
Compare
Refinery Code ReviewThe implementation is solid overall — the OAuth HMAC-signed state, token encryption, GDPR coverage, feature gate, and fallback logic are all well done. Three issues to address before merging: 1. BLOCKING: Merge conflicts with
|
|
Refinery code review passed. All quality gates pass. Review summary:
|
|
Refinery code review passed. All quality gates pass. Code review summary:
Blocker: merge conflicts. The branch is currently with main (). The branch needs to be rebased onto main before it can merge. The conflicts appear to be in the pnpm lockfile and migration journal based on git history divergence. No code issues require a rework — the rebase is the only remaining action. |
|
Refinery code review passed. All quality gates pass. Code review summary:
Blocker: merge conflicts. The branch is currently CONFLICTING with main (mergeStateStatus: DIRTY). The branch needs to be rebased onto main before it can merge. No code issues require a rework — the rebase is the only remaining action. |
|
Refinery code review passed. All quality gates pass. All previously raised issues have been addressed:
Core implementation is sound: OAuth state is HMAC-signed with TTL, tokens are AES-256-GCM encrypted at rest, repo access is KV-cached, the feature is off by default, and the mid-session fallback to app token is graceful. |
Summary
user_github_app_tokenstable to store encrypted GitHub user-to-server (OAuth) tokens, with GDPR soft-delete coverage.ENABLE_GITHUB_USER_TOKENS.getUserTokenForRepoRPC togit-token-servicethat decrypts the stored token, verifies repo access (with KV cache), and returns the token + user identity.session-prepare(both in the router handler andasync-preparation) to prefer the user token when the feature gate is enabled, falling back to the app installation token transparently.GitIdentitytype andbuildGitAuthorhelper for identity-aware git commit attribution (user vs. app-bot).refreshGitHubTokenmethod onCloudAgentSessionthat handles mid-session user-token expiry with a graceful downgrade to the app token and a one-time user-visible warning.redactGitHubTokensutility toworker-utilsto preventghu_/ghr_tokens from appearing in logs.Verification
user_github_app_tokensrows are deleted.ENABLE_GITHUB_USER_TOKENS=false(default) → verify no change in existing app-token behaviour.Visual Changes
N/A (new card added to GitHub integrations settings page, gated behind feature flag).
Reviewer Notes
ENABLE_GITHUB_USER_TOKENS=falsein wrangler.jsonc and.dev.vars.example). Safe to merge without enabling.disconnectUserIdentitycalls GitHub's DELETE/applications/{client_id}/grantto revoke the authorization server-side; failures are logged but do not block the local row deletion.