feat: write hand-written command messages to stderr by khoinguyenpham04 · Pull Request #355 · cloudflare/cf

khoinguyenpham04 · 2026-10-19T13:49:38Z

Hand-written commands printed notices and results on stdout, so cf auth logout > out.txt wrote "You are not currently logged in" into the file. They now use stderr, like generated commands.

Covers cf auth login / logout, the auth profile commands, cf cli telemetry enable / disable, cf migrate and the cf login notice.
Messages use clack log lines and the existing ✓ success line. A next step goes on its own line, so it copies cleanly.
cf auth login with CLOUDFLARE_API_TOKEN set prints one warning instead of two.
New mockOutputStreams test helper. Tests check that stdout stays empty.

Not in this PR: prompts and spinners, and the init/build/deploy flows.

Add lib/ui/log.ts with clack log lines on stderr, the ✓ success line and copyable command lines. formatOutput now prints its success line through the shared helper, and the account hint reuses the guide width. Add mockOutputStreams, which captures console calls and direct stream writes per stream so tests can assert that stdout holds only output.

cf auth login/logout, the auth profile commands, cf cli telemetry enable/disable and cf migrate printed notices, warnings and results on stdout. They now go to stderr as clack log lines and ✓ success lines, so redirected stdout holds only command output, as with generated commands. A command to run next is printed on its own line, and cf auth login with CLOUDFLARE_API_TOKEN set prints a single warning. Refs cloudflare#53

With CLOUDFLARE_API_TOKEN set, login printed messages about stored or expired OAuth tokens before refusing. Warn about the token first.

printSuccess moves to lib/output.ts so commands that only print a success line do not load @clack/prompts. mockOutputStreams treats stderr as a non-terminal unless a test opts in, and cf auth create gains a test.

github-actions · 2026-10-19T13:49:56Z

Hi @khoinguyenpham04. Thanks for contributing to cf!

During the open beta, we’re being very intentional about the changes we accept so that we can prioritise stability for GA. We’re limiting pull requests to contributions coordinated with the team in advance, so this PR has been automatically closed.

We’d still love to hear about your use case, so please make sure it’s captured in a cf issue. If the proposed change fits our priorities, we’d be happy to take another look at this PR.

clanky-the-clanker · 2026-10-19T14:03:18Z

💬 Approved with comments — The move from stdout to stderr is consistent across the commands this pull request covers, and the tests now assert that stdout stays empty. There are two non-blocking INFO notes: a silent !ok branch in cf auth login, and wrangler-tests assertions that no longer check the message text.

Findings: 2 info

_{The decision is advisory: this comment never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #355.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## INFO (2)

### [Code Quality] `cf auth login` now returns silently with exit 0 when `login()` returns false. The branch should be unreachable after the env pre-check, but if the two env checks ever differ it prints nothing. Add a warning or remove the branch.
- **File:** packages/cli/src/commands/auth/login.ts:131

### [Code Quality] The wrangler-tests logout and profiles assertions now only check that stdout is empty, so they would also pass if the command printed nothing. The message text is covered only by the tests in packages/cli.
- **File:** packages/wrangler-tests/src/__tests__/logout.test.ts

📋 Code Quality: 💬 2 info

The stdout-to-stderr move is applied consistently. Two minor notes: a silent branch in cf auth login and weaker assertions in the wrangler-tests suites.

Recommendation: 💬 Optional Nits

[INFO] cf auth login now returns silently with exit 0 when login() returns false. The branch should be unreachable after the env pre-check, but if the two env checks ever differ it prints nothing. Add a warning or remove the branch. (packages/cli/src/commands/auth/login.ts:131)
[INFO] The wrangler-tests logout and profiles assertions now only check that stdout is empty, so they would also pass if the command printed nothing. The message text is covered only by the tests in packages/cli. (packages/wrangler-tests/src/__tests__/logout.test.ts)

View full review

Silent !ok branch in cf auth login (packages/cli/src/commands/auth/login.ts:131-133). The env-credential warning is now printed before the login starts, by a check that calls getAuthFromEnv({ allowGlobalAuthKey: false }). The warning that used to follow a failed login() is gone, so if (!ok) return; now exits 0 and prints nothing. login() is documented to return false only when env credentials are present, so this branch should be unreachable. It would still be reached if the env check inside @cloudflare/workers-auth differs from this one, for example by accepting a global API key. Consider adding a log.warn in that branch, or removing the branch if it really is dead.
Weaker wrangler-tests assertions (packages/wrangler-tests/src/__tests__/logout.test.ts, packages/wrangler-tests/src/__tests__/profiles.test.ts). These tests used to check the message text. They now only check that std.out is "", which also passes if the command prints nothing at all. The new tests in packages/cli cover the message text, so this is a gap in the compatibility suite, not in the feature itself. If clack's mock in that suite makes stderr content impossible to check, the current assertion is acceptable.

Other points checked:

runWranglerMigration already supports output: "stderr".
telemetryStatusLine is still used by cf cli telemetry status.
Removing info() from ui/format.ts matches the updated Library Map row in AGENTS.md.

✅ All other checks passed

🚀 Release: ✅ LGTM

The changeset is present for this user-facing output change. Risk: low.

🔒 Security: ✅ LGTM

The change moves output from stdout to stderr. The new getAuthFromEnv check in login only detects whether env credentials are present and never prints the token. Authentication and authorization behaviour is unchanged.

📖 AGENTS.md: ✅ LGTM

The AGENTS.md edits document the new stderr output convention and update the ui/format.ts Library Map entry to match the removed info helper.

⚡ Performance: ✅ LGTM

The new ui/log.ts imports clack. account-hint.ts already imported clack, so moving GUIDE_WIDTH adds no startup cost.

📝 Documentation: ✅ LGTM

The changeset and the AGENTS.md updates match the new behaviour.

_{Reviewed commit: 9a82bbce}

petebacondarwin · 2026-10-19T14:04:21Z

clanky review

pkg-pr-new · 2026-10-19T14:05:31Z

npm i https://pkg.pr.new/cf@355

commit: 971c883

…ule as guidance

…ility tests

petebacondarwin · 2026-10-19T14:28:27Z

clanky review

khoinguyenpham04 · 2026-10-19T14:45:31Z

clanky review

dario-piotrowicz · 2026-10-19T15:40:04Z

clanky review

petebacondarwin · 2026-10-19T15:40:09Z

clanker review

dario-piotrowicz

In all the tests that are removing some assertion for stdout shouldn't corresponding assertions for stderr be added? (sorry I did comment this in a couple of places before thinking of commenting it here 🙈)

# Conflicts: # packages/cli/src/commands/auth/login.ts

dario-piotrowicz

LGTM 😄

khoinguyenpham04 added 4 commits October 19, 2026 14:44

fix: check for an env token before stored OAuth state in cf auth login

d448ce4

With CLOUDFLARE_API_TOKEN set, login printed messages about stored or expired OAuth tokens before refusing. Warn about the token first.

refactor: keep clack off the success line and pin stderr in tests

dcf08d0

printSuccess moves to lib/output.ts so commands that only print a success line do not load @clack/prompts. mockOutputStreams treats stderr as a non-terminal unless a test opts in, and cf auth create gains a test.

khoinguyenpham04 requested review from a team, dimitropoulos and srikrsna as code owners October 19, 2026 13:49

khoinguyenpham04 requested a review from dario-piotrowicz October 19, 2026 13:49

github-actions Bot closed this Oct 19, 2026

petebacondarwin reopened this Oct 19, 2026

khoinguyenpham04 added 5 commits October 19, 2026 15:14

test: cover the remaining auth message branches and check migrate stdout

8135082

docs: note the success line is terminal-only and phrase the message r…

0a1ab35

…ule as guidance

fix: don't report removed OAuth tokens when cf auth logout keeps them

30276b5

test: expect empty stdout from auth commands in the Wrangler compatib…

8f1c8db

…ility tests

docs: say which warning cf auth login now prints once

8720d9d

docs: note which output has not moved to stderr yet

9a82bbc

dario-piotrowicz reviewed Oct 19, 2026

View reviewed changes

Comment thread packages/cli/src/commands/auth/logout.ts

Comment thread packages/wrangler-tests/src/__tests__/logout.test.ts

Comment thread packages/wrangler-tests/src/__tests__/logout.test.ts Outdated

khoinguyenpham04 added 3 commits October 19, 2026 17:45

test: assert auth messages on stderr in the Wrangler compatibility tests

9a58fb0

docs: link the workers-auth logout behaviour cf relies on

87f1d41

docs: drop the version-pinned workers-auth link from the logout comment

22e164f

petebacondarwin mentioned this pull request Oct 20, 2026

refactor: use cf and deploy previews cloudflare/forge#55

Merged

Merge remote-tracking branch 'upstream/main' into noah/stdout-output

075faa9

# Conflicts: # packages/cli/src/commands/auth/login.ts

khoinguyenpham04 added 2 commits October 20, 2026 12:13

test: expect the keyring login notice on stderr

05eb492

test: drop the clack log helper from the Wrangler compatibility tests

971c883

dario-piotrowicz reviewed Oct 20, 2026

View reviewed changes

Comment thread packages/cli/src/commands/auth/logout.ts Outdated

Comment thread packages/cli/src/commands/auth/logout.ts

Comment thread packages/wrangler-tests/src/__tests__/helpers/mock-clack-log.ts Outdated

dario-piotrowicz approved these changes Oct 20, 2026

View reviewed changes

dario-piotrowicz added this pull request to the merge queue Oct 20, 2026

Merged via the queue into cloudflare:main with commit 0050a39 Oct 20, 2026
9 checks passed

Conversation

khoinguyenpham04 commented Oct 19, 2026

Uh oh!

github-actions Bot commented Oct 19, 2026

Uh oh!

clanky-the-clanker Bot commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📋 Code Quality: 💬 2 info

🚀 Release: ✅ LGTM

🔒 Security: ✅ LGTM

📖 AGENTS.md: ✅ LGTM

⚡ Performance: ✅ LGTM

📝 Documentation: ✅ LGTM

Uh oh!

petebacondarwin commented Oct 19, 2026

Uh oh!

pkg-pr-new Bot commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petebacondarwin commented Oct 19, 2026

Uh oh!

khoinguyenpham04 commented Oct 19, 2026

Uh oh!

dario-piotrowicz commented Oct 19, 2026

Uh oh!

petebacondarwin commented Oct 19, 2026

Uh oh!

dario-piotrowicz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dario-piotrowicz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

clanky-the-clanker Bot commented Oct 19, 2026 •

edited

Loading

pkg-pr-new Bot commented Oct 19, 2026 •

edited

Loading