fix(cli): emit login start URL before polling under non-TTY - #1034
fix(cli): emit login start URL before polling under non-TTY#1034ZachDreamZ wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe login flows now synchronously emit authorization details before polling. Account and runtime dependencies support injected stdout writers. Tests cover OAuth timeout behavior and Codex login completion. ChangesLogin output flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/cli-account.test.ts`:
- Around line 1298-1310: Strengthen the pre-poll assertions in
tests/cli-account.test.ts at lines 1298-1310 and 1323-1335: update each
seenBeforeFirstPoll assignment in the login tests so it becomes true only when
the captured output contains both the authorization URL and that test’s required
login-start text—“Sign in, then paste the redirect URL.” at lines 1298-1310, and
“Flow: flow-mock” at lines 1323-1335.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: be491caf-284a-4ed6-a8ad-fd0fae5f3457
📒 Files selected for processing (4)
src/cli/account-api.tssrc/cli/account-auth.tssrc/cli/runtime-api.tstests/cli-account.test.ts
| if (polls === 1) seenBeforeFirstPoll = chunks.join("").includes("https://auth.example/authorize"); | ||
| }); | ||
| try { | ||
| const result = await run( | ||
| ["login", "anthropic"], | ||
| { ...defaultDeps(), stdoutImpl: (chunk: string) => chunks.push(chunk) }, | ||
| ); | ||
|
|
||
| expect(result.code).toBe(2); | ||
| expect(result.stderr).toContain("login timed out"); | ||
| expect(seenBeforeFirstPoll).toBe(true); | ||
| expect(chunks.join("")).toContain("Open this URL to sign in:\nhttps://auth.example/authorize"); | ||
| expect(chunks.join("")).toContain("Sign in, then paste the redirect URL."); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert every login-start field before polling.
These tests only require the authorization URL before the first Bun.sleep. A regression can defer OAuth instructions or the Codex flow ID until after polling and still pass.
tests/cli-account.test.ts#L1298-L1310: SetseenBeforeFirstPollonly when the captured output contains both the authorization URL and"Sign in, then paste the redirect URL.".tests/cli-account.test.ts#L1323-L1335: SetseenBeforeFirstPollonly when the captured output contains both the authorization URL and"Flow: flow-mock".
As per path instructions, shared CLI behavior changes require focused regression coverage.
📍 Affects 1 file
tests/cli-account.test.ts#L1298-L1310(this comment)tests/cli-account.test.ts#L1323-L1335
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cli-account.test.ts` around lines 1298 - 1310, Strengthen the pre-poll
assertions in tests/cli-account.test.ts at lines 1298-1310 and 1323-1335: update
each seenBeforeFirstPoll assignment in the login tests so it becomes true only
when the captured output contains both the authorization URL and that test’s
required login-start text—“Sign in, then paste the redirect URL.” at lines
1298-1310, and “Flow: flow-mock” at lines 1323-1335.
Source: Path instructions
fa72a9a to
0b33eeb
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/account-auth.ts`:
- Around line 37-40: Update writeSyncStdout to encode chunk once and loop until
every byte is written, advancing by the returned byte count. Handle EAGAIN and
EINTR by retrying, and handle zero-byte writes without silently truncating the
payload or spinning indefinitely.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 51350931-a6ff-40db-a3c6-820d01c227af
📒 Files selected for processing (4)
src/cli/account-api.tssrc/cli/account-auth.tssrc/cli/runtime-api.tstests/cli-account.test.ts
| /** Synchronous fallback for `stdoutImpl`; reaches a pipe immediately. */ | ||
| function writeSyncStdout(chunk: string): void { | ||
| writeSync(1, chunk); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate file ---'
ast-grep outline src/cli/account-auth.ts
printf '%s\n' '--- relevant source ---'
sed -n '1,180p' src/cli/account-auth.ts
printf '%s\n' '--- writeSync usages and declarations ---'
rg -n --glob '*.ts' --glob '*.tsx' 'writeSync|writeSyncStdout|printLoginStart' .Repository: lidge-jun/opencodex
Length of output: 10501
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- local runtime versions ---'
node --version 2>/dev/null || true
bun --version 2>/dev/null || true
printf '%s\n' '--- standalone Node pipe probe ---'
node - <<'JS'
'use strict';
const fs = require('node:fs');
const { spawnSync } = require('node:child_process');
if (process.platform !== 'linux') {
console.log(`SKIP: pipe probe requires Linux; platform=${process.platform}`);
process.exit(0);
}
// The child writes a payload larger than a typical pipe capacity to a pipe
// whose read end is kept open but is not drained while the write runs.
// This distinguishes a single write result from a write-all loop without
// importing or executing repository code.
const script = `
const fs = require("node:fs");
const fd = Number(process.argv[1]);
const payload = Buffer.alloc(1024 * 1024, 0x61);
try {
const n = fs.writeSync(fd, payload, 0, payload.length);
process.stdout.write(JSON.stringify({ ok: true, written: n, requested: payload.length }));
} catch (error) {
process.stdout.write(JSON.stringify({ ok: false, name: error.name, code: error.code, message: error.message }));
process.exitCode = 1;
}
`;
const result = spawnSync(process.execPath, ['-e', script, '3'], {
stdio: ['ignore', 'pipe', 'ignore', 'pipe'],
timeout: 500,
});
console.log(JSON.stringify({
status: result.status,
signal: result.signal,
timedOut: result.error?.code === 'ETIMEDOUT',
output: result.stdout.toString(),
}));
JS
printf '%s\n' '--- repository write-all patterns ---'
sed -n '55,85p' src/codex/history-provider.ts
sed -n '170,190p' src/responses/spill-store.tsRepository: lidge-jun/opencodex
Length of output: 2687
🌐 Web query:
Node.js fs.writeSync partial writes to pipes and Bun node:fs writeSync semantics
💡 Result:
In both Node.js and Bun, the fs.writeSync method does not guarantee that the entire input buffer or string will be written to a pipe (or TTY) in a single call [1][2]. Key Semantics and Behaviors: 1. Partial Writes: When writing to non-blocking file descriptors—such as pipes or TTYs, which Node.js and Bun often configure as non-blocking—fs.writeSync may perform a "short write" (a partial write) [1][3]. It returns the number of bytes actually written, rather than throwing an error or blocking until the full amount is sent [1][4][2]. The application is responsible for checking this return value and, if necessary, re-attempting the write with the remaining data [1]. 2. Blocking vs. Non-blocking: While fs.writeSync is technically a "synchronous" operation, it interfaces with underlying system calls (like write(2)) [4][5]. If the underlying file descriptor is in non-blocking mode (common for stdout/stderr pipes), the system call may return immediately if the pipe buffer is full, resulting in a partial write or an EAGAIN error [3]. Node.js historically does not retry these writes automatically within fs.writeSync [1]. 3. Bun Compatibility: Bun implements its own version of the Node.js fs API to maintain parity [2][6]. Its fs.writeSync behavior generally aligns with Node.js, returning the count of bytes written and adhering to standard syscall patterns [2]. Bun has also addressed specific issues regarding write offsets and buffer handling to ensure its implementation remains consistent with Node.js expectations [6][7]. 4. Practical Implications: Using fs.writeSync on pipes can lead to data truncation if the return value is ignored [1]. To avoid this, developers should wrap fs.writeSync in a loop that continues writing until the entire buffer is consumed or use asynchronous stream-based APIs (e.g., stream.write or pipe), which manage buffering and flow control automatically [1][8]. Note: Some newer experimental stream APIs in Node.js provide higher-level synchronous path abstractions (such as with FileHandle.writer) that may handle these complexities differently, but the standard fs.writeSync remains a low-level primitive with the semantics described above [9][10][11].
Citations:
- 1:
fs.writeSynctruncates long lines nodejs/node#1541 - 2: https://bun.com/reference/node/fs/writeSync
- 3: https://stackoverflow.com/questions/76465639/node-cli-program-writefilesync-gets-eagain-error-when-output-redirected-to-a-p
- 4: fs.writeFileSync won't write to pipes nodejs/node-v0.x-archive#6877
- 5: openSync/writeSync/closeSync produces empty file nodejs/node#7879
- 6: fs: short write in createWriteStream overwrites head of file (NaN position coerced to 0) oven-sh/bun#36135
- 7: node:fs: stop writeFile from truncating files opened with a non-truncating flag oven-sh/bun#33355
- 8: https://stackoverflow.com/questions/6471004/how-can-i-write-blocking-in-stdout-with-node-js
- 9: https://nodejs.org/api/fs.html
- 10: https://bun.com/reference/node/fs/promises/FileHandle/writer
- 11: https://github.com/nodejs/node/blob/main/doc/api/fs.md
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- local runtime versions ---'
node --version 2>/dev/null || true
bun --version 2>/dev/null || true
printf '%s\n' '--- standalone Node pipe probe ---'
node - <<'JS'
'use strict';
const fs = require('node:fs');
const { spawnSync } = require('node:child_process');
if (process.platform !== 'linux') {
console.log(`SKIP: pipe probe requires Linux; platform=${process.platform}`);
process.exit(0);
}
const script = `
const fs = require("node:fs");
const fd = Number(process.argv[1]);
const payload = Buffer.alloc(1024 * 1024, 0x61);
try {
const n = fs.writeSync(fd, payload, 0, payload.length);
process.stdout.write(JSON.stringify({ ok: true, written: n, requested: payload.length }));
} catch (error) {
process.stdout.write(JSON.stringify({ ok: false, name: error.name, code: error.code, message: error.message }));
process.exitCode = 1;
}
`;
const result = spawnSync(process.execPath, ['-e', script, '3'], {
stdio: ['ignore', 'pipe', 'ignore', 'pipe'],
timeout: 500,
});
console.log(JSON.stringify({
status: result.status,
signal: result.signal,
timedOut: result.error?.code === 'ETIMEDOUT',
output: result.stdout.toString(),
}));
JS
printf '%s\n' '--- repository write-all patterns ---'
sed -n '55,85p' src/codex/history-provider.ts
sed -n '170,190p' src/responses/spill-store.tsRepository: lidge-jun/opencodex
Length of output: 2687
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- exact Node writeSync documentation ---'
node -p 'require("node:fs").writeSync.toString()' 2>/dev/null || true
curl -L --fail --silent --show-error \
https://raw.githubusercontent.com/nodejs/node/v24.15.0/doc/api/fs.md |
rg -n -A18 -B4 'fs\.writeSync|writeSync\(fd' | head -120
printf '%s\n' '--- explicit non-blocking pipe probe ---'
python3 - <<'PY'
import os
import subprocess
import sys
import fcntl
rfd, wfd = os.pipe()
flags = fcntl.fcntl(wfd, fcntl.F_GETFL)
fcntl.fcntl(wfd, fcntl.F_SETFL, flags | os.O_NONBLOCK)
script = r'''
const fs = require("node:fs");
const fd = Number(process.argv[1]);
const size = Number(process.argv[2]);
const mode = process.argv[3];
const value = "x".repeat(size);
try {
const written = mode === "string"
? fs.writeSync(fd, value)
: fs.writeSync(fd, Buffer.from(value), 0, size);
process.stdout.write(JSON.stringify({ok: true, written, requested: size}));
} catch (error) {
process.stdout.write(JSON.stringify({
ok: false, name: error.name, code: error.code, written: error.bytesWritten,
message: error.message
}));
}
'''
for mode in ("string", "buffer"):
for size in (1, 4096, 65536, 1048576):
p = subprocess.run(
[sys.executable, "-c", "pass"], # replaced below
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)
p = subprocess.run(
["node", "-e", script, str(wfd), str(size), mode],
pass_fds=(wfd,),
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
timeout=2,
text=True,
)
print(mode, size, p.stdout.strip())
os.close(wfd)
os.close(rfd)
PY
printf '%s\n' '--- CLI tests and stdout dependency contract ---'
rg -n -A8 -B8 'account login|stdoutImpl|printLoginStart|writeSyncStdout' tests src/cli/runtime-api.ts src/cli 2>/dev/null | head -240Repository: lidge-jun/opencodex
Length of output: 20932
🌐 Web query:
site:nodejs.org/api/fs.html fs.writeSync return value partial write non-blocking pipe
💡 Result:
In Node.js, the fs.writeSync method behaves differently depending on the context in which it is used. For standard file system operations, fs.writeSync(fd, buffer, offset, length, position) returns the number of bytes written [1][2][3]. Because it is a synchronous operation, it typically blocks until the write completes or an error occurs. However, in the context of Node.js streams and the pipeTo pipeline, fs.writeSync has a distinct implementation designed for a try-sync fast path [4][5]. In this specific context, fs.writeSync returns a boolean: true if the write succeeded, and false if the caller should fall back to the asynchronous write method [4]. This return value is used to signal flow control, such as when the writer is closed, an async operation is already in flight, or the write would exceed specific limits [4]. Regarding non-blocking pipes, synchronous methods like fs.writeSync generally block the event loop until the operation completes. If you are working with pipes that may be non-blocking or have backpressure constraints, relying on the return value for flow control (as specified in the stream implementation) or using the asynchronous variants (write/writev) is the standard practice to avoid blocking the event loop [4][5].
Citations:
- 1: https://nodejs.org/docs/latest-v5.x/api/fs.html
- 2: https://nodejs.org/docs/v0.3.1/api/fs.html
- 3: http://nodejs.org/docs/v0.5.4/api/fs.html
- 4: https://nodejs.org/api/fs.html
- 5: https://beta.docs.nodejs.org/fs.html
🌐 Web query:
Bun node:fs writeSync documentation nonblocking pipe partial write EAGAIN
💡 Result:
In Bun, the node:fs writeSync function is a direct wrapper that maps to the underlying operating system's write operation for the provided file descriptor [1][2]. If the file descriptor is a non-blocking pipe (or socket), writeSync does not automatically retry if the write operation cannot complete immediately [3]. Instead, it will return the number of bytes successfully written (which may be less than the length of the input buffer) or throw an error with code: 'EAGAIN' if no data could be written [3]. This behavior differs from the intuitive expectation that a "synchronous" function should block until the entire payload is delivered [3]. Because Bun's standard streams (like process.stdout) can sometimes be configured as non-blocking pipes, applications writing large amounts of data to these streams may encounter partial writes or EAGAIN errors when the OS pipe buffer fills up [4][3]. To safely write an entire buffer to a non-blocking file descriptor using writeSync, you must manually implement a retry loop [5][3]. A robust implementation should: 1. Track the current offset within the buffer [5]. 2. Call writeSync in a loop until the entire buffer is consumed [5]. 3. Handle partial writes by updating the offset based on the returned byte count [5]. 4. Catch EAGAIN (and potentially EINTR) errors, and implement a brief wait (e.g., using a short sleep or Atomics.wait) before retrying the operation with the remaining data [5][3]. Bun also provides a retryEAGAIN callback option within fs.Utf8StreamOptions for asynchronous stream operations, which allows you to define custom logic for handling EAGAIN or EBUSY when using higher-level stream APIs, but this does not apply to the low-level fs.writeSync call [6][7].
Citations:
- 1: https://bun.sh/reference/node/fs/writeSync
- 2: https://github.com/oven-sh/bun/blob/main/src/js/node/fs.ts
- 3: bug: io.cts output()/error() throws EAGAIN on a full non-blocking stdout pipe under parallel tests (ubuntu CI flake) open-gsd/gsd-core#1008
- 4: FileSink: record the opened fd in setup() so stdio force-sync clears O_NONBLOCK oven-sh/bun#35956
- 5: https://github.com/mauricekleine/fluncle/blob/main/apps/cli/src/output.ts
- 6: https://bun.com/reference/node/fs/Utf8StreamOptions/retryEAGAIN
- 7: https://bun.com/reference/node/fs/Utf8StreamOptions
🌐 Web query:
Bun stdout file descriptor blocking non-blocking pipe writeSync
💡 Result:
In Bun, the behavior of fs.writeSync when writing to a stdout file descriptor that is a pipe depends on whether the underlying pipe is configured in non-blocking mode [1][2]. 1. Blocking vs. Non-blocking: By default, stdout in many environments may be treated as a non-blocking pipe [1][2]. If you use fs.writeSync on a non-blocking pipe and the pipe buffer is full, the write operation will return a result indicating an error—typically EAGAIN or EWOULDBLOCK—rather than blocking the thread to wait for space [1][2]. 2. Handling EAGAIN: Because writeSync does not automatically block to retry when a pipe is full, your application must handle these partial writes or retry errors manually [2]. Developers often implement a loop that detects EAGAIN, waits for the pipe to become writable (e.g., using Atomics.wait or similar mechanisms if appropriate), and then attempts to write the remaining bytes [2]. 3. Internal Implementation: Bun's internal C/Zig code distinguishes between different file types [1]. For pipes, it may use specific non-blocking write routines [1]. When the runtime itself manages writes (e.g., via Bun.write or process.stdout.write), it handles backpressure and buffering automatically to ensure data is not silently truncated [1][3][4]. However, when you bypass these abstractions using fs.writeSync, you assume responsibility for handling the state of the file descriptor [2]. In summary, writeSync on a non-blocking stdout pipe in Bun is not implicitly blocking; if the pipe is full, it will return an EAGAIN error, requiring your code to perform its own retry logic to ensure the full payload is written [1][2].
Citations:
- 1: https://github.com/oven-sh/bun/blob/7e57e529/src/io/PipeWriter.zig
- 2: https://github.com/mauricekleine/fluncle/blob/main/apps/cli/src/output.ts
- 3: https://bun.sh/reference/bun/BunFile/writer
- 4: stdio: run write completion callbacks in write order oven-sh/bun#33500
Write the complete login-start payload at src/cli/account-auth.ts:38-39.
When fd 1 is a non-blocking pipe, writeSync(1, chunk) can short-write or throw EAGAIN. The ignored byte count can truncate the authorization URL or instructions. Encode the string once, loop over the remaining bytes, and handle EAGAIN/EINTR plus zero-byte progress.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/account-auth.ts` around lines 37 - 40, Update writeSyncStdout to
encode chunk once and loop until every byte is written, advancing by the
returned byte count. Handle EAGAIN and EINTR by retrying, and handle zero-byte
writes without silently truncating the payload or spinning indefinitely.
1 participant
Summary
Fixes #1007.
ocx account loginprinted the authorization URL withconsole.log, which stays buffered when stdout is not a TTY (piped or redirected). Because the command then enters its polling loop, the URL did not reach the user until the process flushed, which made the manual redirect-URL flow look hung under non-TTY stdout.The login-start announcement (URL, instructions, flow id / device code) is now written synchronously to stdout fd 1 before the first poll. Output text and
--jsonbehavior are unchanged. The writer is injectable (stdoutImpl), so the ordering is covered by an in-process regression test.Verification
bun test tests/cli-account.test.ts— 65 pass, 0 fail; includes two new regression tests asserting the URL is emitted before the first poll for both OAuth and Codex logins.bun run typecheck— passbun run lint:gui— passbun run privacy:scan— passbun run doctor:gui:if-changed— skip (nogui/changes)bun run test— 8202 pass / 6 skip / 16 fail on this machine. All 16 failures reproduce identically on unmodifieddevHEAD (WindowsEBUSYfile-lock in the e2e isolated-home helper plus 5s timeouts while the machine was heavily loaded; the suite itself reported taking 2347s vs its normal ~210s), i.e. pre-existing environmental failures unrelated to this change.Checklist
--jsonpath is unchanged.)Summary by CodeRabbit