Skip to content

Commit 9c0317d

Browse files
fix(cli): review round 3 — poll retry, download flush errors
Both findings are flaws in round 1's fixes rather than in the original code. ## A redeemable login was thrown away (Cursor, High) `pollForKey` treated every non-429 status as terminal. But the poll route releases its mint reservation on any mint failure — its own comment says "a later poll can retry" — so a transient 5xx or a same-second name conflict ended the login after the user had already approved in the browser, forcing a full restart for something the server had deliberately left recoverable. Retryable is now 409, 429, and 5xx. Everything else stays terminal: 400 means a malformed request id or verifier and 401/403/404 mean the server is refusing on purpose, so retrying those would just spin to the 15-minute timeout. ## A failed download reported success (Greptile, P1) `file.end(resolve)` passes the flush error to the callback as its argument, so the pump fulfilled *with* the error and the command printed "Saved" for a truncated file. Confirmed against node directly — `end`'s callback receives the errno. It now rejects on that argument, which is the path an ENOSPC actually takes, since the bytes may not reach disk until the final flush. Adds `device-flow.test.ts` (11 tests: the retry matrix, transport failure, terminal refusals, and that the poll secret never enters the browser URL) and `hand-written.test.ts` covering the download's overwrite guard and flush failure. The two retry tests fail against the previous code; the flush test needs `/dev/full` and so runs in CI rather than on macOS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EsThUPqZXjwuuyRjBbmVkj
1 parent 0ca127c commit 9c0317d

4 files changed

Lines changed: 214 additions & 5 deletions

File tree

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
import { afterEach, describe, expect, it, vi } from 'vitest'
2+
import { buildApprovalUrl, createAuthRequest, pollForKey } from './device-flow.js'
3+
4+
const ENDPOINT = 'https://sim.test'
5+
6+
function reply(status: number, body: unknown): Response {
7+
return new Response(JSON.stringify(body), { status }) as Response
8+
}
9+
10+
const COMPLETE = {
11+
status: 'complete',
12+
key: { id: 'k1', apiKey: 'sim_abc' },
13+
scope: 'platform',
14+
workspaceId: 'ws_1',
15+
workspaceBound: true,
16+
}
17+
18+
afterEach(() => {
19+
vi.restoreAllMocks()
20+
vi.useRealTimers()
21+
})
22+
23+
/** Drives the poll loop without waiting out its real 2s interval. */
24+
async function poll(responses: Array<() => Response>) {
25+
let call = 0
26+
vi.spyOn(globalThis, 'fetch').mockImplementation(async () => responses[call++]())
27+
vi.spyOn(globalThis, 'setTimeout').mockImplementation(((fn: () => void) => {
28+
fn()
29+
return 0 as unknown as NodeJS.Timeout
30+
}) as never)
31+
32+
const auth = createAuthRequest()
33+
return { result: await pollForKey(ENDPOINT, auth), calls: () => call }
34+
}
35+
36+
describe('pollForKey', () => {
37+
it('returns the key once the approval completes', async () => {
38+
const { result } = await poll([() => reply(200, COMPLETE)])
39+
expect(result).toMatchObject({ apiKey: 'sim_abc', scope: 'platform', workspaceBound: true })
40+
})
41+
42+
it('keeps polling while the approval is pending', async () => {
43+
const { result, calls } = await poll([
44+
() => reply(200, { status: 'pending' }),
45+
() => reply(200, { status: 'pending' }),
46+
() => reply(200, COMPLETE),
47+
])
48+
expect(calls()).toBe(3)
49+
expect(result.apiKey).toBe('sim_abc')
50+
})
51+
52+
it('retries a 5xx, because the server released the approval for a later poll', async () => {
53+
// The regression: treating every non-429 as terminal threw away an approval
54+
// the user had already granted in the browser.
55+
const { result } = await poll([
56+
() => reply(500, { error: 'Failed to generate API key' }),
57+
() => reply(200, COMPLETE),
58+
])
59+
expect(result.apiKey).toBe('sim_abc')
60+
})
61+
62+
it('retries a same-second name conflict', async () => {
63+
const { result } = await poll([
64+
() => reply(409, { error: 'A personal API key named "CLI (…)" already exists.' }),
65+
() => reply(200, COMPLETE),
66+
])
67+
expect(result.apiKey).toBe('sim_abc')
68+
})
69+
70+
it('retries a rate-limited poll', async () => {
71+
const { result } = await poll([() => reply(429, {}), () => reply(200, COMPLETE)])
72+
expect(result.apiKey).toBe('sim_abc')
73+
})
74+
75+
it('survives a transport failure without ending the login', async () => {
76+
let call = 0
77+
vi.spyOn(globalThis, 'fetch').mockImplementation(async () => {
78+
if (call++ === 0) throw new Error('ECONNRESET')
79+
return reply(200, COMPLETE)
80+
})
81+
vi.spyOn(globalThis, 'setTimeout').mockImplementation(((fn: () => void) => {
82+
fn()
83+
return 0 as unknown as NodeJS.Timeout
84+
}) as never)
85+
86+
const result = await pollForKey(ENDPOINT, createAuthRequest())
87+
expect(result.apiKey).toBe('sim_abc')
88+
})
89+
90+
it('gives up on a deliberate refusal rather than spinning to the timeout', async () => {
91+
await expect(
92+
poll([() => reply(400, { error: 'verifier must be a base64url secret' })])
93+
).rejects.toThrow('verifier must be a base64url secret')
94+
})
95+
96+
it('gives up on a 403', async () => {
97+
await expect(poll([() => reply(403, { error: 'Forbidden' })])).rejects.toThrow('Forbidden')
98+
})
99+
})
100+
101+
describe('createAuthRequest', () => {
102+
it('mints a 43-character base64url request id, challenge, and secret', () => {
103+
const auth = createAuthRequest()
104+
for (const value of [auth.request, auth.challenge, auth.pollSecret]) {
105+
expect(value).toMatch(/^[A-Za-z0-9\-_]{43}$/)
106+
}
107+
})
108+
109+
it('uses a pairing alphabet with no look-alike characters', () => {
110+
// The code is compared across two screens; O/0 and I/1 would defeat that.
111+
for (let i = 0; i < 50; i++) {
112+
expect(createAuthRequest().pairing).toMatch(
113+
/^[ABCDEFGHJKLMNPQRSTUVWXYZ23456789]{4}-[ABCDEFGHJKLMNPQRSTUVWXYZ23456789]{4}$/
114+
)
115+
}
116+
})
117+
118+
it('never puts the poll secret in the browser URL', () => {
119+
const auth = createAuthRequest()
120+
const url = buildApprovalUrl(ENDPOINT, auth, 'platform', 'ws_1')
121+
expect(url).toContain(encodeURIComponent(auth.challenge))
122+
expect(url).not.toContain(auth.pollSecret)
123+
})
124+
})

packages/sim-cli/src/auth/device-flow.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,23 @@ const PAIRING_ALPHABET = 'ABCDEFGHJKLMNPQRSTUVWXYZ23456789'
1818
const POLL_INTERVAL_MS = 2000
1919
const POLL_TIMEOUT_MS = 15 * 60 * 1000
2020

21+
/**
22+
* Poll statuses that leave the approval still redeemable, so the login should
23+
* keep waiting rather than making the user restart the browser handoff.
24+
*
25+
* The poll route releases its mint reservation on any mint failure — its own
26+
* comment says "a later poll can retry" — so giving up on those threw away an
27+
* approval the user had already granted. A transient 5xx or a same-second name
28+
* conflict (409) is exactly that case.
29+
*
30+
* 429 is the poll cadence hitting the per-IP bucket, not a refusal.
31+
*
32+
* Everything else stays terminal: 400 means a malformed request id or verifier,
33+
* and 401/403/404 mean the server is refusing on purpose. Retrying those just
34+
* spins until the 15-minute timeout.
35+
*/
36+
const RETRYABLE_POLL_STATUSES = new Set([409, 429, 500, 502, 503, 504])
37+
2138
export type CliAuthScope = 'copilot' | 'platform'
2239

2340
export interface AuthRequest {
@@ -122,9 +139,7 @@ export async function pollForKey(
122139
const raw = await response.text()
123140

124141
if (!response.ok) {
125-
// 429 is the poll cadence bumping the per-IP bucket, not a refusal —
126-
// back off and keep the login alive instead of making the user restart.
127-
if (response.status !== 429) {
142+
if (!RETRYABLE_POLL_STATUSES.has(response.status)) {
128143
let message = `Login failed with status ${response.status}`
129144
try {
130145
const body = JSON.parse(raw) as { error?: unknown }
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
import { createWriteStream, existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'
2+
import { tmpdir } from 'node:os'
3+
import { join } from 'node:path'
4+
import { afterEach, beforeEach, describe, expect, it } from 'vitest'
5+
import { streamToFile } from './hand-written.js'
6+
7+
let dir: string
8+
9+
beforeEach(() => {
10+
dir = mkdtempSync(join(tmpdir(), 'sim-dl-'))
11+
})
12+
13+
afterEach(() => {
14+
rmSync(dir, { recursive: true, force: true })
15+
})
16+
17+
function bodyOf(chunks: string[]): ReadableStream<Uint8Array> {
18+
return new ReadableStream({
19+
start(controller) {
20+
for (const chunk of chunks) controller.enqueue(new TextEncoder().encode(chunk))
21+
controller.close()
22+
},
23+
})
24+
}
25+
26+
describe('streamToFile', () => {
27+
it('writes the body to disk', async () => {
28+
const target = join(dir, 'out.txt')
29+
await streamToFile(bodyOf(['hello ', 'world']), createWriteStream(target, { flags: 'wx' }))
30+
expect(existsSync(target)).toBe(true)
31+
})
32+
33+
it('refuses to clobber an existing file, naming --force', async () => {
34+
const target = join(dir, 'out.txt')
35+
writeFileSync(target, 'precious')
36+
// The destination usually comes from the server's content-disposition, so a
37+
// silent truncate could destroy a file the caller never named.
38+
await expect(
39+
streamToFile(bodyOf(['new']), createWriteStream(target, { flags: 'wx' }))
40+
).rejects.toThrow(/already exists.*--force/s)
41+
})
42+
43+
it('overwrites when the caller asked for it', async () => {
44+
const target = join(dir, 'out.txt')
45+
writeFileSync(target, 'old')
46+
await streamToFile(bodyOf(['new']), createWriteStream(target, { flags: 'w' }))
47+
expect(existsSync(target)).toBe(true)
48+
})
49+
50+
it.skipIf(!existsSync('/dev/full'))(
51+
'rejects when the final flush fails instead of reporting success',
52+
async () => {
53+
// `end`'s callback receives the flush error; passing `resolve` straight in
54+
// made that error the resolution value, so a truncated download printed
55+
// "Saved". /dev/full only errors at flush time, which is the exact path.
56+
await expect(
57+
streamToFile(bodyOf(['x'.repeat(64 * 1024)]), createWriteStream('/dev/full'))
58+
).rejects.toThrow(/Could not write/)
59+
}
60+
)
61+
})

packages/sim-cli/src/commands/hand-written.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,10 @@ type Row = QueryRowsResponse['data'][number]
2727
* are structurally incompatible under this TS config, and bridging them needs a
2828
* cast that would erase exactly the typing this keeps honest.
2929
*/
30-
async function streamToFile(body: ReadableStream<Uint8Array>, file: WriteStream): Promise<void> {
30+
export async function streamToFile(
31+
body: ReadableStream<Uint8Array>,
32+
file: WriteStream
33+
): Promise<void> {
3134
// Registered before the first write, not after the loop. `createWriteStream`
3235
// opens lazily, so an EEXIST/EACCES/ENOSPC can surface at any point — with no
3336
// listener attached it is an unhandled 'error' event that takes down the
@@ -50,7 +53,13 @@ async function streamToFile(body: ReadableStream<Uint8Array>, file: WriteStream)
5053
reader.releaseLock()
5154
}
5255

53-
await new Promise<void>((resolve) => file.end(resolve))
56+
// `end`'s callback receives the error from a failed final flush (ENOSPC is
57+
// the common one, since the bytes may not hit disk until here). Passing
58+
// `resolve` directly made that error the resolution *value*, so the pump
59+
// fulfilled and the command printed "Saved" for a truncated file.
60+
await new Promise<void>((resolve, reject) => {
61+
file.end((error?: Error | null) => (error ? reject(error) : resolve()))
62+
})
5463
})()
5564

5665
try {

0 commit comments

Comments
 (0)