docs(cloud-agent-next): plan to commit as user via GitHub App user-to-server tokens#3147
docs(cloud-agent-next): plan to commit as user via GitHub App user-to-server tokens#3147kilo-code-bot[bot] wants to merge 2 commits into
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ```ts | ||
| export async function GET(request: Request) { | ||
| const { code, state, error, error_description } = parseQuery(request); | ||
| if (error) return errorRedirect(error_description ?? error); |
There was a problem hiding this comment.
WARNING: error_description is passed directly through to errorRedirect here, but §6.2 Notes say "don't leak GitHub's raw error string into the URL — map known errors to friendly codes." These two instructions are contradictory within the same pseudocode block.
Line 241 uses error_description ?? error as the redirect value, but the note below says to map to friendly codes. The implementation should only use error_description for internal logging and map to a controlled enum (exchange_failed, access_denied, etc.) before putting anything in the redirect URL.
| `${env.PUBLIC_BASE_URL}/api/integrations/github/user-connect/callback` | ||
| ); | ||
| // Optional: prompt=select_account so users with multiple GH accounts pick explicitly. | ||
| authorizeUrl.searchParams.set('prompt', 'select_account'); |
There was a problem hiding this comment.
WARNING: prompt=select_account is not a supported parameter for GitHub's OAuth authorize endpoint. GitHub's OAuth documentation does not recognise prompt; this parameter will be silently ignored. If multi-account selection is desired, this is not achievable via a query parameter on GitHub's standard OAuth flow — it would require the user to be logged out of GitHub first or to use a separate account-switcher flow. Remove this line or note that it has no effect.
| New route in `apps/web` (or a Worker, depending on where existing GitHub webhooks land — check `apps/web/src/app/api/integrations/github/webhook/route.ts` if it exists): | ||
|
|
||
| - Subscribe to `github_app_authorization` (action: `revoked`). | ||
| - Payload includes `sender.id` (GitHub user id). On `revoked`: `UPDATE user_github_app_tokens SET revoked_at = now(), revocation_reason = 'user_revoked' WHERE github_user_id = $sender_id`. |
There was a problem hiding this comment.
WARNING: The webhook revocation update uses WHERE github_user_id = $sender_id, but a single GitHub user ID can map to multiple rows if we ever support both standard and lite app types per user. This is correct in the current schema (the unique index is on (kilo_user_id, github_app_type)), but the WHERE clause is incomplete — it should also filter on the relevant github_app_type values, or explicitly accept that it updates all rows for that GitHub user (which is likely the correct intent). Also, note that the payload field to use is sender.node_id or sender.id (numeric); make sure the stored github_user_id format (numeric string from String(ghUser.id) in §6.2) matches the sender.id payload field format to avoid silent mismatches.
| refresh_token_expires_at: new Date(auth.refreshTokenExpiresAt), | ||
| }).onConflictDoUpdate({ | ||
| target: [userGitHubAppTokens.kilo_user_id, userGitHubAppTokens.github_app_type], | ||
| set: { /* all token + identity columns; reset revoked_at */ }, |
There was a problem hiding this comment.
WARNING: The set: { /* all token + identity columns; reset revoked_at */ } comment leaves a critical implementation detail unspecified. The onConflictDoUpdate set clause must explicitly include updated_at: new Date() — updated_at has .defaultNow() which only applies on INSERT, not on UPDATE. Drizzle does not auto-update it on conflict-update. If this is not spelled out, implementers may miss it, leaving updated_at stale after every reconnect.
|
|
||
| Behaviour: | ||
|
|
||
| 1. Look up `user_github_app_tokens` for `(kiloUserId, appType)`. If missing or `revoked_at IS NOT NULL` → `{ok: false, reason: 'no_user_token'}`. |
There was a problem hiding this comment.
SUGGESTION: The plan conflates two distinct {ok: false} states under a single reason: 'no_user_token': "row does not exist" and "row exists but is revoked". The UI copy in §8.1 maps these identically, but they have different UX implications — a user who was never connected should see "Connect", while a revoked user should see "Reconnect". Consider splitting into reason: 'not_connected' vs reason: 'revoked', especially since the reason: 'revoked' variant is already used separately at line 339/407.
|
|
||
| Two parallel `getUserTokenForRepo` calls for the same user can race on refresh and both spend the refresh token. **Handle this:** | ||
|
|
||
| - Use a Durable Object lock per `kiloUserId` (overkill) **or** a KV-based mutex with `cas` + a 5-minute backoff. Simplest: when refreshing, do a conditional update on the DB row using `WHERE access_token_expires_at = $oldExpiry`; if 0 rows updated, re-read the row (someone else refreshed) and use that token. |
There was a problem hiding this comment.
SUGGESTION: The optimistic-concurrency approach using WHERE access_token_expires_at = $oldExpiry is correct in principle, but relies on the DB driver returning affected-row counts accurately through Drizzle + Hyperdrive. Confirm that Drizzle's update().returning() or affected-rows result works as expected through the Hyperdrive connection pool, and note this in the implementation ticket. The plan says "if 0 rows updated, re-read the row" — make sure the re-read path also checks the new token is not itself expired (another race could have produced a token that is already near-expiry).
|
|
||
| This table holds PII (`github_login`, `github_email`, plus tokens that resolve to a person). **Required updates per `.kilo/rules/gdpr-pii.md`:** | ||
|
|
||
| - `softDeleteUser` (`apps/web/src/lib/user.ts`): hard-delete all `user_github_app_tokens` rows for the user, **and** call `DELETE /applications/{client_id}/grant` (Octokit `apps.deleteAuthorization`) to revoke the user's authorization on the GitHub side. If the GitHub call fails, log + continue — the local rows are gone, which is the GDPR-mandatory part. |
There was a problem hiding this comment.
WARNING: apps.deleteAuthorization requires the access token (not just the client_id) to identify which authorization to delete — specifically it uses Basic Auth with client_id:client_secret and the token in the request body. If the token has already been revoked (e.g., GitHub webhook arrived first and set revoked_at), the access token may be invalid. The plan should specify: (a) decrypt and pass the stored access token to deleteAuthorization; (b) handle 404/422 gracefully since the grant may already be gone; (c) ensure the decryption key is available in the apps/web context where softDeleteUser runs.
| In each existing App (`kiloconnect`, `kiloconnect-lite`, `kiloconnect-development`): | ||
|
|
||
| 1. **Add a new callback URL**: `https://<env>/api/integrations/github/user-connect/callback`. Keep the existing install callback URL. | ||
| 2. **Enable "Expire user authorization tokens"** (already the default for new apps; needs verification on the existing apps). Without this we lose refresh tokens, lose 8h rotation, and must store a non-expiring user token — not acceptable. |
There was a problem hiding this comment.
SUGGESTION: The plan says to enable "Expire user authorization tokens" but notes it "needs verification on the existing apps". If this setting is currently disabled on the live Apps, enabling it will immediately invalidate all existing user tokens that were issued without expiry (though since the feature is not yet built, there are no such tokens yet). However, this is worth calling out explicitly as a verification step before Phase 0, so it does not become a surprise during the App config change.
Code Review SummaryStatus: 11 Issues Found | Recommendation: Address before merge Executive SummaryThe new commit adds container-bound capability enforcement (v2 prefix, Overview
Issue Details (click to expand)WARNING — Unchanged from previous revision (still open)
WARNING — Carried forward from plan review (unchanged)
SUGGESTION — Carried forward from plan review (unchanged)
Incremental Review (new commit 410282b)The new commit introduces container-bound capabilities (
Other Observations (not in diff)Design-level observations from the plan review that remain relevant to the implementation:
Files Reviewed (34 files)
Fix these issues in Kilo Cloud Reviewed by claude-4.6-sonnet-20260217 · 1,717,683 tokens Review guidance: REVIEW.md from base branch |
eshurakov
force-pushed
the
plan/cloud-agent-commit-as-user
branch
from
ab1d756 to
8bcff5c
Compare
| const repositoryPrefix = `/${session.projectPath}`; | ||
| return { | ||
| failure: | ||
| url.pathname.startsWith(repositoryPrefix) || !url.pathname.includes('.git/') |
There was a problem hiding this comment.
WARNING: Fragile repository_mismatch vs invalid_upstream_request classification
The condition !url.pathname.includes('.git/') returns repository_mismatch for any URL whose path contains .git/ but doesn't start with the repo's project path — e.g. https://gitlab.com/attacker/evil.git/files/something. The intent is to distinguish "right origin, wrong repo" from "right origin, not a repo URL at all", but a URL targeting a completely different .git-bearing path on the same host will be labelled repository_mismatch rather than invalid_upstream_request. Both outcomes fail closed, but the mismatch label is semantically wrong and can confuse incident investigations. Consider checking more strictly: url.pathname === repositoryPath || url.pathname.startsWith(repositoryPath + '/') for the mismatch case.
| ); | ||
| if (!runtimeToken.success) return runtimeToken; | ||
|
|
||
| const integration = await this.gitlabLookupService.findGitLabIntegration( |
There was a problem hiding this comment.
WARNING: Redundant findGitLabIntegration DB lookup on non-code-review paths
resolveGitLabRuntimeToken already calls findGitLabIntegration internally for non-code-review sessions (see gitlab-runtime-token-resolver.ts:135). Immediately after, issueGitLabSessionCapability issues another findGitLabIntegration call for the same integration ID. The second call is needed to get integrationType/accountId/accountLogin fields that are not part of GetGitLabTokenSuccess, but on the common (non-code-review) path this doubles the DB queries per issuance. Consider enriching GetGitLabTokenSuccess with the identity fields so the second lookup is only needed for the code-review path where findAuthorizedGitLabIntegrations was used instead.
| const repository = parseGitLabCloneUrl(params.gitUrl, instanceOrigin); | ||
| if (!repository.success) return repository; | ||
| const identity = this.getGitLabSessionIdentity(integration); | ||
| if (!identity) return { success: false, reason: 'no_token' }; |
There was a problem hiding this comment.
WARNING: Incorrect error reason when integration identity is missing
When getGitLabSessionIdentity returns null (both accountId and accountLogin are null on the integration row), this returns { success: false, reason: 'no_token' }. But no_token semantically means "no credential/access-token found", not "integration row is missing required identity fields". This maps to the same client-facing error message as a legitimately absent token, making the two failure modes indistinguishable in logs and user-facing flows. A dedicated reason like 'no_integration_identity' (or reusing 'no_integration_found') would be more accurate.
|
Superseded by #3665 after the parent attribution work merged and this implementation branch was rebased into a clean child-only diff. |
1 participant
Summary
Adds
.plans/cloud-agent-commit-as-user.md— a detailed implementation plan for making cloud-agent-next attribute commits and pushes to the Kilo user who triggered the session, instead of to thekiloconnect[bot]GitHub App identity.The plan picks "Option C" from the prior research: GitHub App user access tokens (user-to-server,
ghu_…). Same App we already have, an additional per-user OAuth step, no PATs, no new App registration. With aghu_token in the git remote URL plus the user's GitHub no-reply email as committer, GitHub attributes the commit and the push to the user (audit log:programmatic_access_type = "GitHub App user-to-server token"). Permissions are intersected with the user's repo access, so the user can never push to repos they don't already have write access to.The plan covers:
github_app_authorizationwebhook).user_github_app_tokenstable (encrypted access + refresh tokens, identity, revocation state) generated viapnpm drizzle generate.apps/webconnect flow:github.connectUserIdentitytRPC mutation, signedstate,/api/integrations/github/user-connect/callback, settings UI states (Connect / Connected / Reconnect).services/git-token-servicenewgetUserTokenForRepoRPC: lookup, refresh near expiry, repo-access check, race-safe DB update, fallback semantics.services/cloud-agent-nexttoken-selection branch insession-prepare, identity-awarecloneGitHubRepoauthor config, mid-session refresh inCloudAgentSessionthat can degrade gracefully to the App token if the user revokes mid-stream.softDeleteUserextension + test, signed-state CSRF defense, webhook signature verification.feature_flag_github_user_token_connect, dogfood criteria, instant flag-off rollback.No code is changed. This PR is plans-only —
.plans/is normally gitignored; the file was force-added for review.Verification
.plans/cloud-agent-commit-as-user.mdend-to-end.Visual Changes
N/A
Reviewer Notes
git pushand clone use the user token). Push back if you'd rather the user token cover more API surface in v1..plans/is gitignored; if we'd rather this plan live indocs/or another tracked path long-term, happy to move it before merge.