Skip to content

fix(cli): emit login start URL before polling under non-TTY #1034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ZachDreamZ wants to merge 1 commit into from
+92 −10
Closed

fix(cli): emit login start URL before polling under non-TTY #1034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/cli/account-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ export interface AccountDeps {
loadConfigImpl?: () => OcxConfig;
stdinImpl?: AccountStdin;
stdinTimeoutMs?: number;
/** Test injection for synchronous stdout writes (login start URL). */
stdoutImpl?: (chunk: string) => void;
/** Test/platform injection for the official Codex login in a restricted staging home. */
spawnCodexLoginImpl?: (codexHome: string) => NativeMainLoginChild;
/** Legacy test seam. Production always uses the spawned child handle above. */
Expand Down
37 changes: 27 additions & 10 deletions src/cli/account-auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { writeSync } from "node:fs";
import {
CliUsageError,
printData,
Expand Down Expand Up @@ -33,6 +34,30 @@ interface LoginStart {
deviceCode?: string;
}

/** Synchronous fallback for `stdoutImpl`; reaches a pipe immediately. */
function writeSyncStdout(chunk: string): void {
writeSync(1, chunk);
}
Comment on lines +37 to +40

@coderabbitai coderabbitai Bot Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.ts

Repository: 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:

🏁 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.ts

Repository: 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 -240

Repository: 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:

🌐 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:

🌐 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:

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.


/**
* Announce the login start before the polling loop holds the process open.
*
* `console.log` to a non-TTY stdout can stay buffered for minutes on some
* platforms, so `ocx account login` piped or redirected to a file appeared to
* hang instead of showing the authorization URL (issue #1007). Writing fd 1
* synchronously delivers the URL to the user before the first poll.
*/
function printLoginStart(start: LoginStart, deps: RuntimeApiDeps): void {
const lines: string[] = [];
if (start.url) lines.push(`Open this URL to sign in:\n${start.url}`);
if (start.instructions) lines.push(start.instructions);
if (start.flowId) lines.push(`Flow: ${start.flowId}`);
if (start.deviceCode) lines.push(`Device code: ${start.deviceCode}`);
if (lines.length === 0) return;
const write = deps.stdoutImpl ?? writeSyncStdout;
write(`${lines.join("\n")}\n`);
}

/** `-` means "read it from stdin", the documented way to pass a code silently. */
const STDIN_SENTINEL = "-";

Expand Down Expand Up @@ -81,11 +106,7 @@ async function login(argv: string[], deps: RuntimeApiDeps): Promise<void> {
method: "POST",
body: JSON.stringify({ ...(id ? { id } : {}), ...(reauth ? { reauth: true } : {}) }),
}, deps);
if (!wantsJson) {
if (start.url) console.log(`Open this URL to sign in:\n${start.url}`);
if (start.instructions) console.log(start.instructions);
if (start.flowId) console.log(`Flow: ${start.flowId}`);
}
if (!wantsJson) printLoginStart(start, deps);
if (code && start.flowId) {
await runtimeRequest("/api/codex-auth/login/code", {
method: "POST",
Expand Down Expand Up @@ -119,11 +140,7 @@ async function login(argv: string[], deps: RuntimeApiDeps): Promise<void> {
method: "POST",
body: JSON.stringify({ provider, addAccount: !reauth, ...(reauth && id ? { accountId: id, reauth: true } : {}) }),
}, deps);
if (!wantsJson) {
if (start.url) console.log(`Open this URL to sign in:\n${start.url}`);
if (start.instructions) console.log(start.instructions);
if (start.deviceCode) console.log(`Device code: ${start.deviceCode}`);
}
if (!wantsJson) printLoginStart(start, deps);
if (code) {
await runtimeRequest("/api/oauth/login/code", {
method: "POST",
Expand Down
5 changes: 5 additions & 0 deletions src/cli/runtime-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ export interface RuntimeApiDeps {
/** Test injection for commands that read a secret from stdin instead of argv. */
stdinImpl?: CliStdin;
stdinTimeoutMs?: number;
/**
* Test injection for output that must reach the user before a long-running
* command holds the process open (for example, the login start URL).
*/
stdoutImpl?: (chunk: string) => void;
}

export class CliUsageError extends Error {
Expand Down
58 changes: 58 additions & 0 deletions tests/cli-account.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ let codexAccounts: Array<Record<string, unknown>> = [];
let oauthAccounts: Array<Record<string, unknown>> = [];
let oauthActiveId: string | null = "acct_1";
let oauthLoginStatus: Record<string, unknown> = { loggedIn: false };
let codexLoginStatus: Record<string, unknown> = { status: "pending" };
let keyEntries: Array<Record<string, unknown>> = [];
let keyActiveId: string | null = "key_1";
let logs: string[] = [];
Expand Down Expand Up @@ -290,6 +291,10 @@ async function mockManagementApi(req: Request): Promise<Response> {
return json(oauthLoginStatus);
}

if (req.method === "GET" && url.pathname === "/api/codex-auth/login-status") {
return json(codexLoginStatus);
}

return json({ error: `unhandled mock endpoint: ${req.method} ${url.pathname}` }, 404);
}

Expand Down Expand Up @@ -354,6 +359,7 @@ beforeEach(() => {
];
oauthActiveId = "acct_1";
oauthLoginStatus = { loggedIn: false };
codexLoginStatus = { status: "pending" };
keyEntries = [{
id: "key_1",
label: "personal",
Expand Down Expand Up @@ -1281,6 +1287,58 @@ describe("ocx account CLI (issue #180 matrix)", () => {

});

describe("login announces the start URL before polling (issue #1007)", () => {
test("OAuth login writes the URL synchronously even when stdout is not a TTY", async () => {
oauthLoginStatus = { loggedIn: false };
const chunks: string[] = [];
let seenBeforeFirstPoll = false;
let polls = 0;
const sleepSpy = spyOn(Bun, "sleep").mockImplementation(async () => {
polls += 1;
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.");
Comment on lines +1298 to +1310

@coderabbitai coderabbitai Bot Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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: Set seenBeforeFirstPoll only when the captured output contains both the authorization URL and "Sign in, then paste the redirect URL.".
  • tests/cli-account.test.ts#L1323-L1335: Set seenBeforeFirstPoll only 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

} finally {
sleepSpy.mockRestore();
}
});

test("Codex login writes the URL and flow id before the first poll", async () => {
codexLoginStatus = { status: "done", email: "j***@example.com" };
const chunks: string[] = [];
let seenBeforeFirstPoll = false;
let polls = 0;
const sleepSpy = spyOn(Bun, "sleep").mockImplementation(async () => {
polls += 1;
if (polls === 1) seenBeforeFirstPoll = chunks.join("").includes("https://auth.example/authorize");
});
try {
const result = await run(
["login", "openai"],
{ ...defaultDeps(), stdoutImpl: (chunk: string) => chunks.push(chunk) },
);

expect(result.code).toBe(0);
expect(result.stdout).toContain("Logged in as j***@example.com.");
expect(seenBeforeFirstPoll).toBe(true);
expect(chunks.join("")).toContain("Open this URL to sign in:\nhttps://auth.example/authorize");
expect(chunks.join("")).toContain("Flow: flow-mock");
} finally {
sleepSpy.mockRestore();
}
});
});

test("39: a login error wins over a retained OAuth credential", async () => {
oauthLoginStatus = {
loggedIn: true,
Expand Down
Loading