Repository navigation
[3387 by Claude Opus 5.5] Renew Connect instance sign-in credentials through the relay (fixes #3382) - #3388
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…hat the relay can renew are now renewed automatically instead of ending in `missing_refresh_token`. The new and existing grant/auth API suites pass (96 tests), as do the related UI tests, typecheck and lint. I haven't run any of it against a real relay, since the relay change ships separately. What it does - Redemption: `connectAuth.ts` now reads `access_token_expires_at` and `renewal: "relay"` and stores them on the grant. A new migration adds `renewal` plus lease and backoff columns to `github_user_grants`. If a relay sends neither field, nothing changes. If the migration hasn't run yet, sign-in still works. - Proactive renewal: about 10 minutes before expiry, with up to 2 minutes of jitter, the grant is renewed through `POST /v1/instance-auth/renew`. The new token replaces the old one only if the stored credential hasn't changed, so a fresh browser sign-in always wins. - One renewal at a time: callers in the same process share one renewal. API, daemon and workers share a database lease on the grant row. I checked that the concurrency test fails when the lease is broken. - 401 handling: the existing forced-refresh paths (metadata routes, browser session sync, MCP) now renew once for these grants, and the caller retries once. Grants without `renewal` keep today's missing-refresh-token check. - Failures: - `renewal_revoked`, `renewal_expired` and `reauth_required` set the grant to `reauth_required` with that code as the reason. - `renewal_unavailable` keeps a still-valid token and drops back to today's behaviour. If the token was rejected or has expired, it requires sign-in with reason `missing_refresh_token`. - Network errors, 5xx and 429 back off (honouring `Retry-After`) and never mark the grant `reauth_required`. - Logging: each outcome is logged under `[connect-grant-renewal]` with only a hash of the grant id, the outcome, expiry, error code and next retry time. The tests check that no tokens or user ids appear. - UI: `GITHUB_REAUTH_REQUIRED` responses now include a `reason` field. The Desktop sign-in notice shows it as a bold first line, for example "Your GitHub authorization for Connect was revoked." I captured two preview screenshots of that notice. - Docs: there's a new "Connect browser sign-in renewal" section in `docs/docs/operations/github-auth.md` and a troubleshooting note pointing to it. Things to know - Renewal needs traffic: it happens on the next use of the grant inside the renewal window; there is no background timer. If nothing uses the grant for hours, the token expires and the next use renews it first. - Expired token, relay down: I chose not to mark the grant `reauth_required` here. Callers get a retryable "temporarily unavailable" until the relay recovers, because forcing a sign-in would throw away a grant the relay can still renew. The issue would also allow marking it once the token has expired; this is easy to change if you prefer that. - Not covered: the MCP credential path in `mcp/connect.ts` is a separate flow and is unchanged. - Session-cookie errors: `reason` is only attached to the metadata-route errors that Desktop uses. Browser-session 401s still return the generic "Please log in again." - File splits: `githubUserGrantService.ts` was already at 399 of the 400-line lint limit. To stay under it I moved the token encryption helpers into `githubUserGrantCrypto.ts`. The renewal logic is in `connectGrantRenewal.ts` (relay call and error mapping) and `connectGrantRenewer.ts` (lease and storage). - Updated test: one existing assertion in `githubMetadataAuth.test.ts` now expects the added `reason: 'missing_refresh_token'`. The new tests are in `packages/api/test/connectGrantRenewal.test.ts`.
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR adds durable relay renewal, coordinated requests, retry handling, and clearer sign-in guidance. It needs minor corrections before merge. ✅ Conditional credential updates — Renewal writes require the original encrypted credential and lease owner, protecting a newer browser sign-in. ✅ Recovery guidance — Recognized renewal failures propagate through the API to Desktop, while unknown reasons receive generic guidance. The supplied current-head checks show 21 passed and no failures. This review uses static analysis only; no commands were run. The issue description is explicitly truncated, so unseen requirements were not assessed. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F1: 🔴 Preserve explicit non-expiring redemption
F2: 🔴 Honor the full Retry-After interval
SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Exercise shared preview integrationThe production service injects ScoreThe implementation has useful concurrency protections and broad focused coverage, with passing current checks. Two localized contract errors remain before merge. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix |
|
✅ Applied the requested follow-up changes in commit 16ca62e
Summary of ChangesI fixed F1 and F2, and each now has a regression test. The renewal suite passes (20 tests), as do the connect-auth, metadata-auth and grant-rotation suites (28 tests); lint and typecheck are clean. To check the new tests, I ran them against the old source files and both failed, so they do catch the original bugs. F1: explicit non-expiring redemption (
F2: full
Other places with the same mistake: none found.
Nothing here changes what users see, so I made no preview. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…renewal suite passes (20 tests), as do the connect-auth, metadata-auth and grant-rotation suites (28 tests); lint and typecheck are clean. To check the new tests, I ran them against the old source files and both failed, so they do catch the original bugs. I fixed F1 and F2, and each now has a regression test. The renewal suite passes (20 tests), as do the connect-auth, metadata-auth and grant-rotation suites (28 tests); lint and typecheck are clean. To check the new tests, I ran them against the old source files and both failed, so they do catch the original bugs. **F1: explicit non-expiring redemption** (`packages/api/connectAuth.ts`) - `tokenExpiresAt` now returns no expiry as soon as the relay sends `access_token_expires_at: null`. The legacy `expires_in` is only used when the explicit field is missing or malformed. - New test: a redemption carrying both `expires_in: 300` and `access_token_expires_at: null` stores a null expiry. Resolving the grant then returns the token without calling the relay. **F2: full `Retry-After` interval** (`packages/api/connectGrantRenewal.ts`) - `renewalBackoffMs` now caps only its own exponential backoff at 10 minutes. It then takes whichever is longer: that value or the relay's full `Retry-After`. - New test: a 429 with `Retry-After: 1800` stores a retry time at least 30 minutes away. The test also checks the function directly: a 30-minute server delay is kept, and a high failure count alone still stays under the 10-minute cap. **Other places with the same mistake:** none found. - The renew response parser already treats `null` as non-expiring, and treats a missing `access_token_expires_at` the same way. - `ConnectGrantRenewer.apply` is the only caller of `renewalBackoffMs`. - The docs say renewal "honours `Retry-After`", which is now true, so I didn't change them. Nothing here changes what users see, so I made no preview. PR: #3388 Comment by: @integry (ID: 6510825330) Model: Claude Opus 5.5
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR adds durable relay renewal, cross-process coordination, retry backoff, and clearer reauthorization messages. It needs a localized concurrency correction before merge. ✅ Conditional credential replacement — Renewal writes check the original encrypted credential and lease owner, protecting a newer browser sign-in from an older renewal response. ✅ Useful recovery distinctions — Transient failures preserve active grants, while terminal relay codes become specific desktop recovery messages. ✅ Focused coverage — Added tests cover renewal timing, concurrent callers, newer sign-ins, legacy grants, and Retry-After handling. This is a static review; no commands or tests were run. The supplied issue description is truncated, so requirements beyond its visible portion could not be assessed. Current checks show no failures and four pending checks. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F3: 🔴 Enforce backoff when acquiring the lease
SuggestionsThese are optional follow-ups and are not sent to S2: 🟢 Reject missing renewal expiry
S3: 🟢 Exercise the production shared-store pathThe new tests construct ScoreThe implementation is well scoped and has substantial focused coverage, but cross-process callers can bypass persisted relay backoff. Correct that race before merging; the remaining checks are still pending. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix |
|
✅ Applied the requested follow-up changes in commit 0886420
Summary of ChangesI fixed F3: a backoff that one process saves now stops other processes from renewing the same grant, even if they read the grant before the backoff was saved. Fix (
Tests (
Both new tests fail when run against the previous I didn't find anything else outside F3 that needs reporting. Nothing in this change is visible in the UI, so there are no previews. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…r processes from renewing the same grant, even if they read the grant before the backoff was saved. I fixed F3: a backoff that one process saves now stops other processes from renewing the same grant, even if they read the grant before the backoff was saved. **Fix** (`packages/api/connectGrantRenewer.ts`, `renewWithLease`) - **The lease check now includes the retry deadline.** The database update that takes the lease only succeeds if `renewal_retry_after_ms` is empty or has passed. This happens in the same atomic update as the credential, status and lease checks. In the reported sequence, B is refused, waits out any lease, and `renewIfDue` rereads the grant. B then keeps using the still-valid token, or gets `temporarily_unavailable` if the token has expired. It sends no second relay request, and the 30-minute deadline stays in place. - **The failure count comes from the leased row.** Without this, B could still shorten the backoff once a deadline had passed. Its stale row would show fewer failures than the database holds, so it would save a lower count and a shorter wait. The failure count is now read from the database after the lease is taken, so the backoff keeps growing. **Tests** (`packages/api/test/connectGrantRenewal.test.ts`) - **Stale read during a 429:** a second process reads the due grant, then the first gets a 429 with `Retry-After: 1800`, saves the backoff and releases the lease. When the second process continues with its old row, the test checks that the relay was called once, the deadline is unchanged, the failure count is still 1, and the current token comes back. - **Stale failure count:** the old row shows 0 failures while the database holds 3 and the deadline has passed. After a 503, the test checks that 4 failures are saved with a future deadline. Both new tests fail when run against the previous `connectGrantRenewer.ts` and pass with the fix. The renewal suite passes (22 tests), as do the connect-auth, metadata-auth and grant suites (23 tests). Lint with zero warnings and typecheck are clean for `packages/api`. These suites only run with `NODE_ENV=test`; without it they exit before any test runs. I didn't find anything else outside F3 that needs reporting. Nothing in this change is visible in the UI, so there are no previews. PR: #3388 Comment by: @integry (ID: 6511180274) Model: Claude Opus 5.5
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR adds durable relay renewal, coordinated renewal requests, retry backoff, and clearer Desktop recovery messages. It needs a localized correctness fix before merge. ✅ Conditional credential replacement — Renewal writes require both the original encrypted credential and lease owner, preventing an in-flight renewal from overwriting a newer browser sign-in. ✅ Persistent retry backoff — Lease acquisition checks the stored retry deadline, including when another process holds a stale grant snapshot. ✅ Scoped recovery messages — Desktop displays allowlisted reasons and falls back to generic guidance for unknown values. The supplied checks report no failures and five pending checks. This review is static; no commands were run. The issue specification is explicitly truncated, so assessment is limited to its visible requirements and the supplied implementation. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F4: 🔴 Reject renewal responses missing expiry
SuggestionsThese are optional follow-ups and are not sent to S4: 🟢 Exercise the production shared-store integrationThe new renewal tests construct ScoreThe implementation has useful concurrency safeguards and focused coverage, but malformed successful responses can disable expiry-driven renewal. The remaining checks are pending, with no reported failures. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix |
|
✅ Applied the requested follow-up changes in commit 4d0d390
Summary of ChangesI fixed F4: a renewal response that has no The fix ( Regression test ( Verification:
Similar code checked: the only other caller of 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…pires_at` is now rejected instead of being saved as a token that never expires. I fixed F4: a renewal response that has no `access_token_expires_at` is now rejected instead of being saved as a token that never expires. **The fix** (`packages/api/connectGrantRenewal.ts`, in `requestRelayRenewal`): the field is now passed straight to `parseConnectExpiry`. That function returns `undefined` when the field is absent, so the existing `invalid_response` check now catches it. An explicit `null` still means "non-expiring", as before. When the field is missing, the grant keeps its stored expiry, a transient backoff is recorded, and the grant stays active while the current token is still valid. **Regression test** (`packages/api/test/connectGrantRenewal.test.ts`): I added "a success response missing `access_token_expires_at`" to the existing table of transient failures. That test already checks that the failure count goes up, a retry time is set, no second relay call happens during backoff, and a forced resolve returns `temporarily_unavailable`. I also added a check, run for every case in the table, that the original expiry is still stored. **Verification:** - The renewal suite passes: 23 tests, 0 failures. I ran it with `PROPR_DEMO_MODE=true` because the file fails to load without GitHub auth configured, and that happened before my change too. - I ran the suite with the old version of `connectGrantRenewal.ts` and the new test failed, so it does catch the bug. - ESLint (`--max-warnings 0`) and `tsc --noEmit` for `packages/api` are clean. **Similar code checked:** the only other caller of `parseConnectExpiry` is the redemption path in `connectAuth.ts` (`tokenExpiresAt`). There, a missing field is expected, because the issue says these fields are additive and older relays don't send them. In that case it falls back to `expires_in` rather than storing "never expires", so it doesn't have this bug and I didn't change it. The renewal endpoint is new and its response always includes `access_token_expires_at`, so a missing field there can only mean a malformed response. PR: #3388 Comment by: @integry (ID: 6511502981) Model: Claude Opus 5.5
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR adds durable relay renewal, database-backed coordination, transient-failure backoff, and specific reauthorization guidance. It appears merge-ready within the visible scope. ✅ Conditional credential updates — Renewal writes require the original encrypted credential and lease owner, protecting a newer browser sign-in from stale results. ✅ Retryable outage handling — Transient failures preserve active grants, persist backoff, and return temporary unavailability when the credential has expired. ✅ Focused coverage — Added tests cover concurrent renewal, stale backoff reads, terminal errors, and sign-in during renewal. Current-head checks report 21 passed and no failures or pending checks. This was a static review; no commands or tests were run. The supplied issue description is truncated, so omitted acceptance criteria could not be assessed. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S5: 🟢 Cover shared-preview integrationThe production S6: 🟢 Prefer explicit error codesIn ScoreThe implementation has substantial focused coverage and passing current-head checks, with no verified merge blocker in the supplied code. Production shared-preview integration would benefit from additional coverage. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
AI Implementation Summary
Closes #3387
Branch:
3387/claude-opus-5-5-renew-connect-instance-si-20261029-1810-fbsCommits: ✅ Changes committed (d8ee3fa)
AI Processing Completed
Execution Details:
Summary:
I've implemented the instance side of #3387. Connect sign-in grants that the relay can renew are now renewed automatically instead of ending in
missing_refresh_token. The new and existing grant/auth API suites pass (96 tests), as do the related UI tests, typecheck and lint. I haven't run any of it against a real relay, since the relay change ships separately.What it does
connectAuth.tsnow readsaccess_token_expires_atandrenewal: "relay"and stores them on the grant. A new migration addsrenewalplus lease and backoff columns togithub_user_grants. If a relay sends neither field, nothing changes. If the migration hasn't run yet, sign-in still works.POST /v1/instance-auth/renew. The new token replaces the old one only if the stored credential hasn't changed, so a fresh browser sign-in always wins.renewalkeep today's missing-refresh-token check.renewal_revoked,renewal_expiredandreauth_requiredset the grant toreauth_requiredwith that code as the reason.renewal_unavailablekeeps a still-valid token and drops back to today's behaviour. If the token was rejected or has expired, it requires sign-in with reasonmissing_refresh_token.Retry-After) and never mark the grantreauth_required.[connect-grant-renewal]with only a hash of the grant id, the outcome, expiry, error code and next retry time. The tests check that no tokens or user ids appear.GITHUB_REAUTH_REQUIREDresponses now include areasonfield. The Desktop sign-in notice shows it as a bold first line, for example "Your GitHub authorization for Connect was revoked." I captured two preview screenshots of that notice.docs/docs/operations/github-auth.mdand a troubleshooting note pointing to it.Things to know
reauth_requiredhere. Callers get a retryable "temporarily unavailable" until the relay recovers, because forcing a sign-in would throw away a grant the relay can still renew. The issue would also allow marking it once the token has expired; this is easy to change if you prefer that.mcp/connect.tsis a separate flow and is unchanged.reasonis only attached to the metadata-route errors that Desktop uses. Browser-session 401s still return the generic "Please log in again."githubUserGrantService.tswas already at 399 of the 400-line lint limit. To stay under it I moved the token encryption helpers intogithubUserGrantCrypto.ts. The renewal logic is inconnectGrantRenewal.ts(relay call and error mapping) andconnectGrantRenewer.ts(lease and storage).githubMetadataAuth.test.tsnow expects the addedreason: 'missing_refresh_token'. The new tests are inpackages/api/test/connectGrantRenewal.test.ts.Detailed Logs:
ece3529b-0011-4a0f-b779-e2680e64caf0Log files stored at:
/usr/src/app/logs/executions/issue-3387-2026-10-29T18-28-29-320Z-conversation.json/usr/src/app/logs/executions/issue-3387-2026-10-29T18-28-29-320Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3387.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!
Visual preview
Desktop GitHub sign-in notice: grant not renewable
For a grant issued before Connect renewal (renewal_unavailable → missing_refresh_token), the notice suggests signing in once to get a renewable authorization.
Original unavailable: managed storage is unavailable or the upload has expired.
Desktop GitHub sign-in notice: Connect authorization revoked
The Desktop recovery notice now leads with the precise reason when the relay reports renewal_revoked. Rendered from the real component and desktop.css in headless Chromium.
Original unavailable: managed storage is unavailable or the upload has expired.