feat(login): --device — RFC 8628 device-code login for headless machines - #74
Conversation
insta login --device mints a device code against the platform's Better Auth mount (/api/auth/device*), prints a console link the human opens on ANY device, and polls until approval hands back a normal session token — the login path for VMs/SSH/CI where the loopback --oauth callback can never be reached. Poll loop honors authorization_pending, slow_down (+5s per RFC 8628 §3.5), expiry, and denial; poster+wait are injected for tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K219odKzTozTGj2gbrJZUS
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Review: feat(login): --device (RFC 8628 device-code login)
Summary: A clean, well-tested implementation of the RFC 8628 device grant that mirrors the existing loginOauth session-storage path; the poll-loop contract matches Better Auth's documented device-plugin behavior, and I found no blocking issues.
Requirements context
insta-cli has no /docs/superpowers/ (or docs/specs/) directory — the feature spec lives in the companion PR InsForge/insta-platform#163 (serves /api/auth/device/*), as the PR body states. Assessed against the PR description + RFC 8628 + the platform Better Auth device-authorization contract (verified against Better Auth v1.6.23 docs via context7). No matching in-repo spec/plan to consult.
I verified the crux of the design: the poll loop matches OAuth error codes off ApiError.message (src/commands/auth.ts:92-99), which only works because ApiClient.request maps res.body?.error → the error message and throws on status ≥ 400 (src/api.ts:40). Better Auth's device plugin returns these states (authorization_pending, slow_down, expired_token, access_denied) as RFC 6749 { error, error_description } envelopes with HTTP 400, and success returns { access_token } — so the mapping, the field names, and the grant.access_token read (auth.ts:90-91) all line up with the documented server behavior. The DI test fake (throw new ApiError(400, code)) faithfully models the real client.
Findings
Critical
(none)
Suggestion
- Functionality / robustness — unvalidated numeric response fields can produce
NaN(src/commands/auth.ts:80-84).intervalandexpires_infrom the/device/coderesponse are used arithmetically without a fallback. If the server ever omitsinterval,Math.max(undefined, 1)→NaN, andwait(NaN)resolves immediately → the loop hot-polls (which will then tripslow_down). Ifexpires_inis omitted,deadline = Date.now() + NaN→NaN,Date.now() < NaNisfalse, so the loop never runs and login fails instantly with "expired before it was approved". Blast radius is low because Better Auth always populates both fields (its own client defaultsinterval = 5), so this is a defensive hardening ask, not a live bug — recommend defaultingintervalto5and treating a missing/NaNexpires_inas an explicit error rather than silent instant-expiry. - Software engineering / tests — the client-side deadline path is untested (
src/commands/auth.ts:83,100-101). The 5 new tests cover pending→retry,slow_downbackoff, denial, serverexpired_token, and unexpected-error rethrow — good protocol coverage. The naturalwhile (Date.now() < deadline)client-side expiry (the belt to the server'sexpired_tokensuspenders) isn't exercised; because the injectedwaitdoesn't advanceDate.now(), it's awkward to reach today. Not required, but ifdeadlineis refactored later there's no regression guard. Worth a note or a small injectable-clock seam.
Information
- Missing
scopeon the code request (src/commands/auth.ts:76). The request sends only{ client_id: 'insta-cli' }; Better Auth's device client example passesscope: "openid profile email".scopeis optional and the resulting session's scope is a platform-side (#163) decision, so this is fine as-is — flagging only so scope handling is a conscious choice, consistent with what the console approval page will grant. verification_uriis declared but unused (src/commands/auth.ts:64,78). Onlyverification_uri_complete+user_codeare printed. Showing the complete URI while asking the human to confirm the displayeduser_codeis a good anti-phishing check and is RFC-acceptable; the plainverification_uriis the documented fallback for surfaces that can't use the complete URI. Minor.- Token duplicated as both access and refresh (
src/commands/auth.ts:56,58).setSession({ accessToken: token, refreshToken: token })is identical to the existingloginOauth(auth.ts:40-42), so the device path stays consistent with the browser path by design — noting only that any future correctness question aboutrefresh()using a session token applies equally to both flows, not something this PR introduces. - Cross-repo dependency (correctness gate). Merge safety is coupled to insta-platform#163 delivering the HTTP-400 +
{ error: <code> }envelope and anaccess_token-shaped success body that the platform's/meaccepts as a bearer. The PR already declares this dependency; the CLI side matches the documented Better Auth contract, so no code change is needed here — just sequence the merges so the CLI doesn't ship ahead of the endpoints.
Security
No secrets logged (only the resolved email is printed on success); device_code travels only in the request body; no new dependencies; auth path is additive and does not weaken existing checks.
Performance
Poll cadence honors the server interval and is bounded by expires_in; no busy-wait (subject to the NaN-interval Suggestion above), no blocking I/O on a hot path, no N+1.
Verdict
approved (informational — zero Critical findings; the two Suggestions are non-blocking hardening/coverage items). Actual GitHub approval remains a separate human action.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/index.ts">
<violation number="1" location="src/index.ts:61">
P3: The login help/description now advertise --device, but a headless user running bare `insta login` still gets the error '--email is required (or use --oauth <github|google>)' which omits the new headless path. Since --oauth's loopback callback can never work on a VM/SSH box, this message actively misdirects exactly the audience --device targets; update it to also mention --device.</violation>
</file>
<file name="src/commands/auth.ts">
<violation number="1" location="src/commands/auth.ts:78">
P2: Headless users can receive no usable verification URL when the server omits optional `verification_uri_complete`, even though the required `verification_uri` is available. Falling back to `start.verification_uri` keeps the device login usable.</violation>
<violation number="2" location="src/commands/auth.ts:81">
P1: Device login busy-polls when the authorization response omits optional `interval`, because `Math.max(undefined, 1)` yields `NaN` and the timer fires immediately. Applying RFC 8628's default interval of 5 seconds when the field is absent avoids hammering the token endpoint.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| .option('--email <email>', 'account email') | ||
| .option('--password <password>', 'account password (else $INSTA_PASSWORD or prompt)') | ||
| .option('--oauth <provider>', 'browser OAuth login: github | google') | ||
| .option('--device', 'device-code login: approve from a browser on any other machine (VMs, SSH, CI)') |
There was a problem hiding this comment.
P3: The login help/description now advertise --device, but a headless user running bare insta login still gets the error '--email is required (or use --oauth <github|google>)' which omits the new headless path. Since --oauth's loopback callback can never work on a VM/SSH box, this message actively misdirects exactly the audience --device targets; update it to also mention --device.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/index.ts, line 61:
<comment>The login help/description now advertise --device, but a headless user running bare `insta login` still gets the error '--email is required (or use --oauth <github|google>)' which omits the new headless path. Since --oauth's loopback callback can never work on a VM/SSH box, this message actively misdirects exactly the audience --device targets; update it to also mention --device.</comment>
<file context>
@@ -54,10 +54,11 @@ function resolveVersion(): string {
.option('--email <email>', 'account email')
.option('--password <password>', 'account password (else $INSTA_PASSWORD or prompt)')
.option('--oauth <provider>', 'browser OAuth login: github | google')
+ .option('--device', 'device-code login: approve from a browser on any other machine (VMs, SSH, CI)')
.option('--api-url <url>', 'control-plane API base URL')
.option('--env <name>', `deployment environment: ${ENV_NAMES.join(' | ')}`)
</file context>
…on --device in bare-login error Review round 1 (jwfing + cubic): default interval to 5s when absent (was NaN -> hot-poll), fail loudly on missing expires_in (was NaN deadline -> bogus instant expiry), fall back to verification_uri when verification_uri_complete is absent, and point bare `insta login` errors at --device for headless machines. Adds tests for all three response shapes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K219odKzTozTGj2gbrJZUS
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Review: feat(login): --device (RFC 8628 device-code login)
Summary: A well-scoped, well-tested addition of insta login --device that faithfully implements the RFC 8628 device-authorization poll loop and reuses the existing session-storage path; no blocking issues found.
Requirements context
No matching spec/plan found — this repo has no docs/superpowers/ or docs/specs/ directory (only README.md/CONTRIBUTING.md/CLAUDE.md/skills). Assessed against the PR description, the RFC 8628 spec, and the better-auth device-authorization plugin contract. I cross-checked the platform-side contract the CLI assumes (field names + error codes) against the live better-auth v1.6.x docs via context7 — the CLI matches it exactly (/device/code → device_code, user_code, verification_uri, verification_uri_complete, interval, expires_in; /device/token → {access_token} on success, else {error: <code>} with authorization_pending/slow_down/expired_token/access_denied/invalid_grant). No stale or hallucinated API usage.
Findings
Critical
(none)
Suggestion
- Software engineering / testing —
test/device-login.test.tsexercisesdeviceGrantthoroughly (pending→retry, slow_down backoff, denial, expiry, unexpected-error rethrow, default interval, missingexpires_in,verification_uri_completefallback — genuinely good coverage), but theloginDeviceglue atsrc/commands/auth.ts:51-61is untested: the/api/auth/device/*endpoint paths, the doublesetSession, the follow-upGET /me, andpersist(). A typo in the endpoint prefix or a regression in the session wiring wouldn't be caught. Low blast radius, and the analogousloginOauthis similarly untested, so this is a suggestion rather than a blocker.
Information
- Functionality —
deviceGrantstores the granted token as both access and refresh token (src/commands/auth.ts:56,58), identical to the existingloginOauthpath (auth.ts:40,42). If a better-auth session token is not a valid input toPOST /auth/refresh(src/api.ts:73-84), the one-shot 401 refresh will simply fail and the user re-runs login. This is a pre-existing pattern, not introduced here — noting only so device login inherits the same refresh semantics as browser OAuth. - Robustness — a
200from/device/tokenwith noaccess_tokenfield would makedeviceGrantreturnundefined(auth.ts:98-99), which then gets stored as an empty session. The contract makes this near-impossible; a defensiveif (!grant.access_token) throw …would fail more legibly, mirroring the loudexpires_inguard already added atauth.ts:81-84. - Dependency coupling — correctness depends on InsForge/insta-platform#163 (a) mounting better-auth at
/api/authon the same host as the control-plane routes, and (b) returning RFC 8628 error bodies as{error: <code>}with HTTP ≥400, soApiError.message(set frombody.erroratsrc/api.ts:40) carries the OAuth code the poll loop switches on. The CLI side is correct against the better-auth device-plugin contract; flagging only that this PR cannot be exercised end-to-end until #163 and the console/devicepage land.
Dimension notes
- Security — no security-relevant regressions. No secrets/tokens are logged (only the user-facing
verification_uri/user_codeare printed to stdout); the token is persisted the same way as existing logins; no new user input reaches SQL/shell/HTTP unsafely; no auth checks weakened. The device flow correctly does not need the loopbackstateCSRF token that browser OAuth uses. - Performance — the poll loop is bounded by the server
deadlineand waitsintervalseconds before every poll; theexpires_in/intervalNaN guards specifically prevent a hot-polling loop. No concerns.
Verdict
approved (informational — zero Critical findings; the Suggestion/Information items are non-blocking). Human approval via the separate approve flow. Posted as a COMMENT per bot policy.
There was a problem hiding this comment.
3 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/auth.ts">
<violation number="1" location="src/commands/auth.ts:82">
P2: Malformed but finite `expires_in` values can make headless login poll past its local expiry indefinitely; validate the computed deadline or bound the lifetime before entering the loop. `Number.MAX_VALUE` passes this check, but the millisecond conversion overflows to `Infinity`.</violation>
<violation number="2" location="src/commands/auth.ts:89">
P2: An `interval: Infinity` response makes device login hot-poll the token endpoint; reject non-finite intervals before applying `Math.max`. `Number(start.interval) || 5` does not treat `Infinity` as missing, and Node reduces the resulting timer to 1 ms.</violation>
</file>
<file name="test/device-login.test.ts">
<violation number="1" location="test/device-login.test.ts:92">
P3: This new test patches the process-global `process.stdout.write` to capture `info()` output. It restores the writer in `finally`, so it won't leak, but the approach has two downsides worth noting: it replaces process-wide stdout for the duration of the awaited call (fragile if this file is ever run with parallel tests or if anything else writes to stdout concurrently), and if the assertion inside the test fails, the captured `lines` are never replayed, so a failure loses the output that would help debugging. A more isolated approach would be to have `deviceGrant` return the link it prints (or accept an output sink like it already accepts an injected poster/wait), letting the test assert on the returned value instead of intercepting global stdout.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| it('falls back to verification_uri when the complete variant is absent', async () => { | ||
| const lines: string[] = [] | ||
| const write = process.stdout.write.bind(process.stdout) | ||
| process.stdout.write = ((s: string) => { lines.push(String(s)); return true }) as typeof process.stdout.write |
There was a problem hiding this comment.
P3: This new test patches the process-global process.stdout.write to capture info() output. It restores the writer in finally, so it won't leak, but the approach has two downsides worth noting: it replaces process-wide stdout for the duration of the awaited call (fragile if this file is ever run with parallel tests or if anything else writes to stdout concurrently), and if the assertion inside the test fails, the captured lines are never replayed, so a failure loses the output that would help debugging. A more isolated approach would be to have deviceGrant return the link it prints (or accept an output sink like it already accepts an injected poster/wait), letting the test assert on the returned value instead of intercepting global stdout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/device-login.test.ts, line 92:
<comment>This new test patches the process-global `process.stdout.write` to capture `info()` output. It restores the writer in `finally`, so it won't leak, but the approach has two downsides worth noting: it replaces process-wide stdout for the duration of the awaited call (fragile if this file is ever run with parallel tests or if anything else writes to stdout concurrently), and if the assertion inside the test fails, the captured `lines` are never replayed, so a failure loses the output that would help debugging. A more isolated approach would be to have `deviceGrant` return the link it prints (or accept an output sink like it already accepts an injected poster/wait), letting the test assert on the returned value instead of intercepting global stdout.</comment>
<file context>
@@ -66,4 +66,38 @@ describe('deviceGrant', () => {
+ it('falls back to verification_uri when the complete variant is absent', async () => {
+ const lines: string[] = []
+ const write = process.stdout.write.bind(process.stdout)
+ process.stdout.write = ((s: string) => { lines.push(String(s)); return true }) as typeof process.stdout.write
+ try {
+ const { verification_uri_complete: _omitted, ...startWithoutComplete } = START
</file context>
Review round 2 (cubic + jwfing): interval:Infinity survived the || 5 fallback and Node truncates setTimeout(Infinity) to ~1ms (hot-poll) — treat any non-finite interval as the RFC default 5s. expires_in:Number.MAX_VALUE overflowed the ms conversion to an Infinity deadline (poll forever) — cap lifetime at 1h. A 200 token response without access_token now errors instead of storing an empty session. Tests for all three. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K219odKzTozTGj2gbrJZUS
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Review: feat(login): --device — RFC 8628 device-code login
Summary: A clean, well-tested implementation of RFC 8628 device-code login; the poll loop matches the Better Auth device-authorization contract and I found no blocking issues — the remaining items are platform-dependency confirmations and robustness polish.
Requirements context
No matching spec/plan found — insta-cli has no docs/superpowers/ (nor docs/specs/) directory. Assessed against the PR description, RFC 8628, and the Better Auth device-authorization plugin contract (verified live via context7: POST /device/token returns 200 {access_token, token_type, expires_in, refresh_token?} on success and non-2xx {error, error_description} with the OAuth error code in error on pending/slow_down/denied/expired — which is exactly what ApiClient.request() surfaces as ApiError.message = body.error, api.ts:40).
Findings
Critical
(none)
Suggestion
Functionality — transient network errors abort the whole wait (src/commands/auth.ts:108-114)
Inside the poll loop, only ApiError messages are matched; any other throw (e instanceof ApiError false → code = '') falls straight to throw e. A single transient transport failure — fetch rejecting with a TypeError on a dropped connection, which is exactly what happens on the flaky SSH/CI links this feature targets — therefore kills a 15-minute wait that would otherwise have succeeded on the next poll. RFC 8628 clients normally tolerate transient transport errors and keep polling until the deadline. Consider treating non-ApiError failures as retryable (continue until deadline) rather than fatal.
Functionality — device base-path + token-acceptance depend on insta-platform#163 (src/commands/auth.ts:78,100,57)
Device calls hit /api/auth/device/*, while every other CLI call uses /auth/* and /me (no /api prefix) against the same apiUrl base. This is correct only if the platform mounts Better Auth at its default /api/auth basePath and the access_token the device grant returns is accepted as a Bearer by the control-plane /me route (the PR body asserts "a normal Better Auth session token"). Both are untestable in this repo and the entire flow hinges on them — worth an explicit cross-check against insta-platform#163 before merge, and ideally a manual end-to-end run against staging.
Software engineering — loginDevice wrapper is untested (src/commands/auth.ts:51-61)
The protocol core (deviceGrant) has excellent coverage (11 tests: pending/slow_down/denial/expiry/unexpected-error, interval default + non-finite, malformed/capped expires_in, missing access_token, verification_uri fallback). The wrapper that stores the session, calls /me, and persists is not exercised. This is consistent with loginOauth (also untested), so low priority — but a thin test asserting the token is stored and the /me user is persisted would guard the wiring.
Information
refresh_tokenfrom the grant is discarded (src/commands/auth.ts:56,100-107): the response type is narrowed to{ access_token?: string }and the session is stored as{ accessToken: token, refreshToken: token }. Better Auth optionally returns a realrefresh_token; capturing it when present would let/auth/refreshuse a proper refresh token instead of replaying the access token. Consistent withloginOauthtoday, so purely a note.- "expires in Nm" reflects the capped lifetime (
src/commands/auth.ts:87-91): the message usesmin(expires_in, 3600), so it would understate a server-sent expiry above 1h. Cosmetic; the cap itself (guarding theNumber.MAX_VALUE → Infinitydeadline overflow) is a good defensive touch. - Doc mirror ships separately: the PR notes
skills/insta/cli-reference.mdupdates alongside in the superproject submodule — not in this diff, so nothing to review here; just flagging the sequencing so the docs land with the feature.
Security & Performance
- Security: No new dependencies. No secrets logged — only the
user_codeand verification URL are printed, which are meant to be shown to the human; the session token is never echoed. Auth checks elsewhere are untouched; all request bodies are JSON (no injection surface). No concerns. - Performance: The poll loop is properly bounded by both
interval(with carefulNaN/Infinityguards that prevent hot-polling) and a finitedeadline, with correct RFC 8628 §3.5slow_downback-off. No concerns.
Verdict
approved (informational — human approval via the separate approve flow). Zero Critical findings. The two functionality Suggestions (network-error resilience, and confirming the /api/auth path + token acceptance against insta-platform#163) are worth addressing but are non-blocking.
Review round 3 (jwfing): a dropped connection mid-wait (the flaky SSH/CI links this flow exists for) aborted the whole 15-minute login; RFC 8628 clients tolerate transport failures and keep polling until the deadline. Only ApiError — a real server answer — can end the loop now, and the missing-access_token guard moved outside the retry path so a malformed 200 still fails loudly instead of retrying into an empty session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K219odKzTozTGj2gbrJZUS
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/auth.ts">
<violation number="1" location="src/commands/auth.ts:95">
P2: An oversized finite `interval` still reaches `sleepSeconds` unchanged, where `s * 1000` overflows Node's timer range and is scheduled as ~1 ms, causing hot-polling of `/api/auth/device/token`. Bounding the interval before the timer call—and also bounding the `slow_down` increment—would preserve safe pacing for malformed numeric responses.</violation>
<violation number="2" location="src/commands/auth.ts:106">
P2: A truthy non-string `access_token` (for example `123`) passes this check, so `loginDevice` stores it through `setSession` and sends a malformed bearer value to `/me` instead of rejecting the response. Validating that the field is a non-empty string at this boundary would keep malformed token responses from contaminating the login flow.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| client_id: 'insta-cli', | ||
| })) as { access_token?: string } | ||
| // A 200 without a token must not become an empty stored session. | ||
| if (!grant?.access_token) throw new Error('malformed token response (missing access_token)') |
There was a problem hiding this comment.
P2: A truthy non-string access_token (for example 123) passes this check, so loginDevice stores it through setSession and sends a malformed bearer value to /me instead of rejecting the response. Validating that the field is a non-empty string at this boundary would keep malformed token responses from contaminating the login flow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/auth.ts, line 106:
<comment>A truthy non-string `access_token` (for example `123`) passes this check, so `loginDevice` stores it through `setSession` and sends a malformed bearer value to `/me` instead of rejecting the response. Validating that the field is a non-empty string at this boundary would keep malformed token responses from contaminating the login flow.</comment>
<file context>
@@ -78,24 +78,32 @@ export async function deviceGrant(post: DevicePoster, wait: (s: number) => Promi
- })) as { access_token: string }
+ })) as { access_token?: string }
+ // A 200 without a token must not become an empty stored session.
+ if (!grant?.access_token) throw new Error('malformed token response (missing access_token)')
return grant.access_token
} catch (e) {
</file context>
| if (!grant?.access_token) throw new Error('malformed token response (missing access_token)') | |
| if (typeof grant?.access_token !== 'string' || !grant.access_token) throw new Error('malformed token response (missing access_token)') |
| // Absent OR non-finite interval = the RFC 8628 §3.2 default 5s: NaN would fire the timer | ||
| // instantly and Infinity gets truncated to ~1ms by Node — both hot-poll the token endpoint. | ||
| const rawInterval = Number(start.interval) | ||
| let interval = Number.isFinite(rawInterval) ? Math.max(rawInterval, 1) : 5 |
There was a problem hiding this comment.
P2: An oversized finite interval still reaches sleepSeconds unchanged, where s * 1000 overflows Node's timer range and is scheduled as ~1 ms, causing hot-polling of /api/auth/device/token. Bounding the interval before the timer call—and also bounding the slow_down increment—would preserve safe pacing for malformed numeric responses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/auth.ts, line 95:
<comment>An oversized finite `interval` still reaches `sleepSeconds` unchanged, where `s * 1000` overflows Node's timer range and is scheduled as ~1 ms, causing hot-polling of `/api/auth/device/token`. Bounding the interval before the timer call—and also bounding the `slow_down` increment—would preserve safe pacing for malformed numeric responses.</comment>
<file context>
@@ -78,24 +78,32 @@ export async function deviceGrant(post: DevicePoster, wait: (s: number) => Promi
+ // Absent OR non-finite interval = the RFC 8628 §3.2 default 5s: NaN would fire the timer
+ // instantly and Infinity gets truncated to ~1ms by Node — both hot-poll the token endpoint.
+ const rawInterval = Number(start.interval)
+ let interval = Number.isFinite(rawInterval) ? Math.max(rawInterval, 1) : 5
+ const deadline = Date.now() + lifetime * 1000
while (Date.now() < deadline) {
</file context>
|
Re the review's cross-repo confirmation ask: verified live against the merged insta-platform main (14dfbd1) — booted |
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Review: feat(login): --device — RFC 8628 device-code login
Summary: A clean, well-tested RFC 8628 device-authorization login for headless machines; the protocol handling is careful and the DI-based test suite is thorough — no blocking issues found.
Requirements context
No matching spec/plan found — insta-cli has no docs/superpowers/ or docs/specs/ directory (only CONTRIBUTING.md). Assessed against the PR description, RFC 8628, and the Better Auth device-authorization plugin contract (verified against Better Auth v1.6.23 docs). Cross-repo dependency InsForge/insta-platform#163 serves the /api/auth/device/* endpoints.
Findings
🔴 Critical
(none)
I specifically checked the one place this design could break: the poll loop matches e.message against literal OAuth codes (authorization_pending, slow_down, …). This only works if api.request maps the response's top-level error string into ApiError.message (src/api.ts:40) and the platform returns RFC 8628-compliant HTTP 400 { "error": "<code>" } on non-terminal states. Better Auth's device plugin (v1.6.23) is RFC 8628-compliant — 400 with a top-level error code on pending/slow_down/expired/denied, and { access_token } on success — so the mapping holds. The NaN/Infinity guards on expires_in and interval (auth.ts:83-95) correctly prevent instant-expiry and hot-poll failure modes, and each is covered by a test.
🟡 Suggestion
-
Functionality / cross-repo contract —
src/commands/auth.ts:78,101. The whole feature hinges on insta-platform#163 returning RFC-shaped errors (400 { error: "authorization_pending" }), not200with an error body. If the platform's Better Auth mount ever answers pending withHTTP 200 { error: … },api.requestwon't throw,grant.access_tokenisundefined, and login dies immediately with "malformed token response (missing access_token)" on the first poll. The code is correct for the documented/standard contract — worth a one-line integration check against #163's actual responses (and confirmingclient_id: 'insta-cli'is registered as a trusted device client) before both merge. -
Software engineering / docs mirror — flag surface.
CONTRIBUTING.md:41-44states a flag change "is only half done until it is mirrored ininsta/cli-reference.md" (inInsForge/insta-skills). The PR body notes this ships as a companion PR in the skills submodule; since it's a separate repo it can't appear in this diff. Please land theinsta-skillscli-reference.mdupdate in lockstep — that file is how coding agents discover--device. -
Software engineering / test scope —
src/commands/auth.ts:51-61. Tests exercisedeviceGrant(the pure protocol) exhaustively, butloginDevice's wiring (setSession→/me→persist) is untested. This mirrorsloginOauth, which is also untested here, so it's consistent with the repo — noting only that the token-to-/me-to-persist sequence is the one part with no coverage.
🔵 Information
-
Functionality —
src/commands/auth.ts:56-59. Device sessions store the session token as bothaccessTokenandrefreshToken, identical tologinOauth(auth.ts:40-42). If that token isn't a valid refresh credential,/auth/refreshsilently fails and the user re-logs in. This is pre-existing OAuth-login behavior inherited unchanged, not introduced by this PR. -
Functionality —
src/commands/auth.ts:57. If the final/mecall throws after a successful approval, nothing is persisted and the user must re-approve. Same structure asloginOauth; low blast radius. Precedence order inlogin()(--devicebefore--oauthbefore email,auth.ts:19-20) is sensible and matches the help text. -
Security. No new user input reaches SQL/shell; the client is a pure HTTP caller.
device_code/access_tokenare never logged; onlyuser_codeand the server-providedverification_uriare printed. Server-controlled strings are printed to the terminal, but the server is the auth server being logged into (trusted), so no meaningful injection surface. No secrets in error messages. -
Performance. Poll loop is properly paced: waits
intervalbefore every poll,Math.max(rawInterval, 1)floors it at 1s,slow_downadds 5s, and the deadline is bounded by the cappedlifetime. No hot loops, N+1s, or blocking work.
Verdict
approved (informational — human approval still required via the GitHub approve flow). Zero Critical findings; the Suggestions are cross-repo coordination and a docs mirror, none blocking. Nicely done, especially the malformed-response guards and the injected post/wait test seam.
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Review: feat(login): --device — RFC 8628 device-code login
Summary: A clean, well-tested RFC 8628 device-code login that correctly mirrors the existing OAuth session-storage path; the protocol handling is textbook-correct and the edge-case hardening is genuinely thorough.
Requirements context
No matching spec/plan found — insta-cli has no docs/superpowers/ or docs/specs/ directory (consistent with the repo's docs-light convention). Assessed against the PR description, CONTRIBUTING.md conventions, RFC 8628, and the Better Auth device-authorization plugin contract (verified via context7: /device/token returns flat {error, error_description} codes at HTTP 400, endpoint paths /api/auth/device/{code,token}, grant_type, and default interval 5s — all match the implementation).
Critical
(none)
Suggestion
Functionality — the device-token response's refresh_token is discarded (src/commands/auth.ts:56-58, 77/118)
Better Auth's /device/token success response includes both access_token and an optional refresh_token (confirmed in the plugin docs). deviceGrant returns only the access_token (a string), and loginDevice stores it as both tokens: setSession({ accessToken: token, refreshToken: token }). On a later 401 the client posts that access token to /auth/refresh as a refreshToken, which will almost certainly be rejected — so device sessions likely can't refresh and silently expire, forcing a re-login. This exactly mirrors the existing loginOauth limitation (the loopback bridge only ever hands back one token), so it's not a regression — but unlike OAuth, the device response does carry a usable refresh_token. Consider capturing it (widen deviceGrant's return to {access_token, refresh_token?}) so device logins get genuine refresh behaviour. Non-blocking; the failure mode is a benign re-login, no data loss.
Functionality — only expires_in is validated; verification_uri/user_code aren't (src/commands/auth.ts:88-90)
The malformed-response guarding on expires_in/interval is excellent. But if the authorization response is missing both verification_uri_complete and verification_uri (or user_code), the printed instructions become undefined / check it shows this code: undefined. Low blast radius (the platform controls this response), and the existing test asserts the single-fallback case avoids undefined, but a one-line presence check would fail loudly like the expires_in path does.
Information
- Software engineering — test coverage is a highlight. 12 tests exercise every protocol branch (pending → retry,
slow_down+5s cumulative backoff,access_denied,expired_token,invalid_grantrethrow, transport-blip tolerance, missing/Infinityinterval → 5s default,MAX_VALUElifetime cap, tokenless 200,verification_urifallback). The injectedposter+waitDI matches the repo convention (CONTRIBUTING.md§Architecture,ensure-skills.ts) and lets the whole loop run without network or real timers. The commit history shows genuine TDD hardening.loginDeviceitself (the network wrapper) is untested, but that's consistent withloginOauth/browserOauth. - Security — no concerns. No secrets logged (
user_code/verification_uriare meant to be displayed; the session token is persisted to config, never printed). Device endpoints correctly call with{ auth: false }(pre-auth). All inputs flow as JSON body params — no shell/SQL/URL-interpolation surface. No new dependencies. No auth/authorization checks weakened. - Performance — no concerns. The poll loop is paced by
intervalwithslow_downbackoff and a harddeadline; the NaN/Infinity interval cases that would otherwise hot-poll the token endpoint are explicitly guarded (src/commands/auth.ts:92-95) — a nice catch. - Merge ordering. Depends on InsForge/insta-platform#163 (serves
/api/auth/device/*and registersclient_id: insta-cli) and the console/deviceapproval page — neither is live-verifiable from this repo.--devicewill error until #163 is deployed. - Doc mirror. Per
CONTRIBUTING.md, a flag change is only half done untilinsta/cli-reference.md(InsForge/insta-skills) is updated. The PR body says this ships alongside as a submodule mirror — worth confirming it actually lands.
Verdict
approved (informational — the human still approves via the GitHub approve flow). Zero Critical findings; posting as a comment. The two Suggestions (capture the real refresh_token; guard verification_uri/user_code) are worth a look but non-blocking.
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/device-login.test.ts">
<violation number="1" location="test/device-login.test.ts:98">
P3: The 'caps an absurd expires_in' test grants the token on the first poll, so the loop returns before the deadline check ever runs — it passes whether or not the lifetime cap exists, so it doesn't actually guard the 'instead of polling forever' behavior its name claims. Consider asserting the capped loop terminates: e.g. drive the wait with a fake that rejects the wait once the deadline would be exceeded, or otherwise exercise the pending->expiry path so the cap regression would be caught.</violation>
</file>
<file name="src/commands/auth.ts">
<violation number="1" location="src/commands/auth.ts:107">
P2: The new `continue` for every non-ApiError now retries *any* poster failure — not just true network blips — until the deadline, then reports the misleading 'device login expired' message. If the transport failure is persistent (e.g. the API host is unreachable for the whole login) or the poster throws a genuine bug/parse error that isn't an ApiError, the user is stuck for the full ~15 minutes polling a dead endpoint and the underlying error is silently discarded. Consider logging the swallowed error (info/warn) and, optionally, bounding the consecutive-transport-failure retries so a truly dead connection fails fast instead of consuming the whole window.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| client_id: 'insta-cli', | ||
| })) as { access_token?: string } | ||
| } catch (e) { | ||
| if (!(e instanceof ApiError)) continue // transport blip (dropped SSH/CI link) — keep polling until deadline |
There was a problem hiding this comment.
P2: The new continue for every non-ApiError now retries any poster failure — not just true network blips — until the deadline, then reports the misleading 'device login expired' message. If the transport failure is persistent (e.g. the API host is unreachable for the whole login) or the poster throws a genuine bug/parse error that isn't an ApiError, the user is stuck for the full ~15 minutes polling a dead endpoint and the underlying error is silently discarded. Consider logging the swallowed error (info/warn) and, optionally, bounding the consecutive-transport-failure retries so a truly dead connection fails fast instead of consuming the whole window.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/auth.ts, line 107:
<comment>The new `continue` for every non-ApiError now retries *any* poster failure — not just true network blips — until the deadline, then reports the misleading 'device login expired' message. If the transport failure is persistent (e.g. the API host is unreachable for the whole login) or the poster throws a genuine bug/parse error that isn't an ApiError, the user is stuck for the full ~15 minutes polling a dead endpoint and the underlying error is silently discarded. Consider logging the swallowed error (info/warn) and, optionally, bounding the consecutive-transport-failure retries so a truly dead connection fails fast instead of consuming the whole window.</comment>
<file context>
@@ -78,33 +78,44 @@ export async function deviceGrant(post: DevicePoster, wait: (s: number) => Promi
+ })) as { access_token?: string }
} catch (e) {
- const code = e instanceof ApiError ? e.message : ''
+ if (!(e instanceof ApiError)) continue // transport blip (dropped SSH/CI link) — keep polling until deadline
+ const code = e.message
if (code === 'authorization_pending') continue
</file context>
|
|
||
| // Huge-but-finite expires_in (Number.MAX_VALUE) overflows the ms conversion to Infinity; the | ||
| // lifetime cap keeps the deadline finite. Grant still resolves normally. | ||
| it('caps an absurd expires_in instead of polling forever', async () => { |
There was a problem hiding this comment.
P3: The 'caps an absurd expires_in' test grants the token on the first poll, so the loop returns before the deadline check ever runs — it passes whether or not the lifetime cap exists, so it doesn't actually guard the 'instead of polling forever' behavior its name claims. Consider asserting the capped loop terminates: e.g. drive the wait with a fake that rejects the wait once the deadline would be exceeded, or otherwise exercise the pending->expiry path so the cap regression would be caught.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/device-login.test.ts, line 98:
<comment>The 'caps an absurd expires_in' test grants the token on the first poll, so the loop returns before the deadline check ever runs — it passes whether or not the lifetime cap exists, so it doesn't actually guard the 'instead of polling forever' behavior its name claims. Consider asserting the capped loop terminates: e.g. drive the wait with a fake that rejects the wait once the deadline would be exceeded, or otherwise exercise the pending->expiry path so the cap regression would be caught.</comment>
<file context>
@@ -85,6 +85,44 @@ describe('deviceGrant', () => {
+
+ // Huge-but-finite expires_in (Number.MAX_VALUE) overflows the ms conversion to Infinity; the
+ // lifetime cap keeps the deadline finite. Grant still resolves normally.
+ it('caps an absurd expires_in instead of polling forever', async () => {
+ const { post, wait } = fakeFlow(['token:sess-h'], { ...START, expires_in: Number.MAX_VALUE })
+ await expect(deviceGrant(post, wait)).resolves.toBe('sess-h')
</file context>
2 participants
What
insta login --device— the login path for machines with no usable browser (agents in VMs, SSH boxes, CI). The existing--oauthflow can never work there: its callback targets a loopback listener on the machine running the CLI.The human opens the link on any machine, signs in, approves; the CLI's poll returns a normal Better Auth session token — stored exactly like the browser-OAuth login, so
/me, refresh, and logout behave identically.Mechanics
POST /api/auth/device/code(client_idinsta-cli), pollsPOST /api/auth/device/token.intervalbefore every poll,authorization_pending→ retry,slow_down→ +5s backoff, server-side expiry and console denial → clear errors with a retry hint.deviceGranttakes injected poster + wait (repo DI pattern) — tests exercise the whole protocol without network or timers.Depends on
/deviceapproval page (PR to follow)Testing
npm run typecheckclean; 22 files / 186 tests green (5 new protocol tests).Note
skills/insta/cli-reference.md(superproject skills submodule) mirror update ships alongside this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01K219odKzTozTGj2gbrJZUS
Summary by cubic
Adds
insta login --device, an RFC 8628 device-code login for headless machines where no browser is available. Users approve in any browser; the CLI stores a normal session with clearer, more resilient behavior.New Features
--deviceprints a verification link and user code, then polls until approval per RFC 8628 (authorization_pending,slow_down+5s, expiry, denial).--api-url/--env; requires/api/auth/device/code,/api/auth/device/token, and a console/devicepage.Bug Fixes
expires_inat 1h and fails fast if missing; errors on token responses missingaccess_token; tolerates transient network errors (only server OAuth errors stop the loop); falls back toverification_uriwhenverification_uri_completeis absent.insta loginerror now points headless users to--device.Written for commit 574072f. Summary will update on new commits.