Skip to content

feat(review): report rejected logins as logged out, and review with environment credentials - #339

Merged
moq-bot[bot] merged 9 commits into
mainfrom
quest/m0/review-login
Nov 2, 2026
Merged

moq-bot[bot] merged 9 commits into
mainfrom
quest/m0/review-login

Conversation

@kixelated

@kixelated kixelated commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator

Implements and deletes quest/m0/review-login.md.

Summary

  • Rejected logins fail as logged out. When a reviewer turn fails on authentication, quest review now raises the same usage error (exit 2) as a signed-out CLI, <harness> is not logged in; run \`, in place of a raw 401. It doesn't send an extra probe request. This covers the case where codex login status` exits 0 on a revoked token.

    • Claude Code: claude -p reports api_error_status: 401 on its result, and the check reads that field. As a fallback it also matches the result text: Failed to authenticate, API Error: 401, Invalid API key, Please run /login.
    • Codex: matches 401 Unauthorized or sign in again in the turn.failed message. When no turn ran, it checks stderr instead, which is where a failed token refresh is reported (Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again.).
  • Environment credentials. The status check still runs with strippedEnvironment. If it fails, the reviewer keeps the credential its CLI reads from the environment and the review goes ahead:

    • Claude Code: ANTHROPIC_API_KEY, ANTHROPIC_AUTH_TOKEN, CLAUDE_CODE_OAUTH_TOKEN
    • Codex: CODEX_API_KEY

    If that credential is also rejected, the error names it: codex rejected CODEX_API_KEY; replace it, or run \codex login``.

  • Rerouting variables stay stripped. ANTHROPIC_BASE_URL, CLAUDE_CODE_USE_BEDROCK, CLAUDE_CODE_USE_VERTEX and CLAUDE_CODE_USE_FOUNDRY are still never passed. While one of them is set, no Claude credential passes either, since it belongs to that other endpoint. The error names the variable instead of just saying "not logged in": claude is not logged in, and quest review strips ANTHROPIC_BASE_URL, which would reroute it; run \claude auth login``.

  • Logged-out errors name the profile checked. Each one names the stored login the status check read, such as claude is not logged in to the profile CLAUDE_CONFIG_DIR=/home/me/.claude-personal; run \claude auth login`, or ... to its default profile (CLAUDE_CONFIG_DIR is unset). That covers the failed status check, a rejection after a passing check, and a stripped reroute. Codex names CODEX_HOME` the same way.

  • README and quest review --help describe the behavior.

Evidence from the real CLIs

I probed both CLIs with bogus credentials in throwaway config dirs (Claude Code 2.1.316, codex-cli 0.165.0):

  • claude -p --output-format json with a bad ANTHROPIC_API_KEY returns {"is_error":true,"api_error_status":401,"result":"Invalid API key · Fix external API key"}. With a bad CLAUDE_CODE_OAUTH_TOKEN the result text is Failed to authenticate. API Error: 401 Invalid bearer token.
  • claude auth status exits 0 when only ANTHROPIC_API_KEY, ANTHROPIC_AUTH_TOKEN, CLAUDE_CODE_OAUTH_TOKEN, or CLAUDE_CODE_USE_BEDROCK/_VERTEX is set. So for Claude, stripping was the only cause of feat(gst): moqsrc takes a max-delay moq#7084 and fix(watch): accept structural read-side origins moq#7251.
  • codex exec --json reads CODEX_API_KEY and ignores OPENAI_API_KEY: with only OPENAI_API_KEY set, the request goes out with no auth header. codex login status prints "Not logged in" even when CODEX_API_KEY is set. A failed request ends in {"type":"turn.failed","error":{"message":"unexpected status 401 Unauthorized: ..."}}.

Choices made without asking

The quest's Plan left these to the implementer: "weigh that against why STRIPPED exists before choosing which names pass". Please check them when reviewing:

  1. A stored login wins over an environment credential. A credential is passed only when the status check fails without it, so the stored login comes first. That is the reverse of each CLI's own order. The reason: a shell that exports ANTHROPIC_API_KEY for other tools would otherwise move every review from a subscription login to API billing, and today's behavior for logged-in users stays exactly as it is. The other option is to always pass the credential and let the CLI's own order decide, which is closer to the Plan's wording but would change billing for users who have both.
  2. Which variables pass. Only the credentials each CLI actually reads (listed above). OPENAI_API_KEY stays stripped because codex exec ignores it. Harness markers such as CLAUDECODE, CLAUDE_CODE_ENTRYPOINT and CODEX_THREAD_ID stay stripped too, so the reviewer never acts as a sub-session.
  3. A rerouting variable also withholds the credentials. ANTHROPIC_BASE_URL and the provider switches stay stripped, and while one is set no credential passes, so a gateway token is never sent to api.anthropic.com. CLAUDE_CODE_USE_GATEWAY/_MANTLE/_ANTHROPIC_* also exist in the binary but aren't documented, so they are not named. The only effect of leaving them out is a less specific error message.
  4. The error for a rejected environment credential is different. It names the variable instead of repeating "not logged in", because <login> alone would not fix a bad variable. It doesn't name a profile, since no stored login was used.
  5. Codex names its profile too. The quest asks only for the Claude profile (CLAUDE_CONFIG_DIR). Codex's CODEX_HOME selects its stored login in the same way and is kept by STRIPPED in the same way, so one profile field per reviewer covers both and avoids a Claude-only branch.

Test plan

  • test/review.test.ts: the fake CLIs gained FAKE_LOGGED_OUT, where only a credential variable authenticates (the fake claude auth status counts it and codex login status doesn't, like the real ones), and FAKE_REJECTED, which fails each turn with each CLI's real auth error in each style it uses. New tests:
    • a rejected login fails as logged out after a passing status check: Claude with and without a status field, Codex via turn.failed and via stderr
    • a reviewer with no stored login reviews with its environment credential, and the other harness's credentials and markers still don't reach it
    • a stored login wins over ANTHROPIC_API_KEY
    • a rejected environment credential is named in the error
    • ANTHROPIC_BASE_URL withholds ANTHROPIC_AUTH_TOKEN and is named in the error
    • a logged-out error names CLAUDE_CONFIG_DIR or CODEX_HOME, both when signed out and when rejected after a passing check (setup() unsets both, so the session's own profile can't change the expected messages)
  • just check and just test pass at f9166c2 (878 vitest tests plus the 432-test shell and lifecycle suites), run from a session with CLAUDE_CONFIG_DIR set.

Merge from main

  • Merged origin/main at 3c8cdce (never rebased). The one conflict was in test/review.test.ts, and both sides were kept. Each fake CLI's env filter now captures this PR's ANTHROPIC* and main's PATH (moq-dev/quest#341). Main's new "reviewer CLI is found on PATH" test sits beside this PR's credential tests. Main sanitizes PATH inside run(), so the status check and turns this PR adds get it too.
  • The previous head 4b89fb3 was a quest land merge of main into 3f09da1, and its PR diff matches 3f09da1's (only hunk offsets and surrounding context differ). The review below covers the whole branch at the new head.
  • The CLAUDE_CONFIG_DIR test leak previously listed as a follow-up is fixed on main by moq-dev/quest#340, so it's dropped.

Review

  • Reviewer: Codex (quest review), gpt-6.1-sol at medium effort, session 01a19a18-98e5-77f1-a942-8b1c0eef506a, started fresh (--new) after merging origin/main at 3c8cdce with a hand-resolved conflict
  • Round 2 reviewed head f9166c2dfc642450a067d841f9cd5526e2057eaa against origin/main at 3c8cdce: 0 findings
  • Round 1 (head e689a3d, the merge) reported 1 finding:
    • medium src/cli/review.ts:463: the quest's Goal requires the "not logged in" error to name the CLAUDE_CONFIG_DIR profile it checked (decided 2026-10-31), and the error didn't. Fixed in f9166c2: every logged-out error names the profile (CLAUDE_CONFIG_DIR, or CODEX_HOME for Codex) or the default when unset, with tests for both harnesses on both paths.
  • Earlier sessions: 01a198ee-48fe-7351-984a-6b82a7e92763 (gpt-6.1-sol, medium) was clean on 3f09da1 after one fix (disabled provider switches counted as reroutes). 01a195bd-4275-7441-98bb-779dc7963eda (gpt-6-astra, --deep) was clean on 26a53dd after one fix (stderr-only auth errors).

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

kixelated and others added 2 commits October 31, 2026 22:49
Quest-Claim: 819bc587da592102
…nvironment credentials

A reviewer turn that fails on authentication (a 401, or an expired or
revoked token the status check missed) now raises the same "not logged
in" usage error as a signed-out CLI. A reviewer CLI with no stored login
keeps the credential it reads from the environment, unless a stripped
variable would send it to another endpoint, which the error then names.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title Review handles expired and environment logins feat(review): report rejected logins as logged out, and review with environment credentials Nov 1, 2026
…derr

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review November 1, 2026 06:19
@coderabbitai

coderabbitai Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Important

  • Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d9523033-c191-4c52-9cb5-bf02a61efe83
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

kixelated and others added 2 commits November 1, 2026 13:02
Claude Code turns CLAUDE_CODE_USE_BEDROCK, _VERTEX, and _FOUNDRY on only for
1, true, yes, or on, so a disabled switch no longer withholds an environment
credential.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Landing 3f09da1d1e91dcb55f5d6b9470bf1c572877b133 via /quest-merge.

  • Merged origin/main without rebasing. The conflicts in the README and quest review --help came from the removal of --deep (moq-dev/quest#361). The resolution keeps this PR's credential text and main's single pinned model and effort. The 4 failing checks had been cancelled because of the conflict, and all 13 pass now.
  • A local Codex review of the merge found one problem: CLAUDE_CODE_USE_BEDROCK=0 or false counted as a reroute. Fixed in 3f09da1 with regression tests. Round 2 found nothing; see ## Review.
  • moq-dev/quest#316 overlaps this PR. Its Codex fix (no stored-login requirement when CODEX_API_KEY is set) is already covered here, because a failed status check falls back to CODEX_API_KEY. What fix(review): preserve reviewer connection credentials #316 does that this PR deliberately does not is pass gateway and provider routing (ANTHROPIC_BASE_URL, ANTHROPIC_CUSTOM_HEADERS, the Bedrock/Vertex/Foundry endpoints). That routing is left for the maintainer to decide.

(Written by Claude Opus 5.5)

@kixelated
kixelated enabled auto-merge (squash) November 1, 2026 21:15
kixelated added a commit that referenced this pull request Nov 1, 2026
#339 finishes and deletes review-login, including reviewing
with credentials from the environment. Narrowing it here would conflict
with that deletion, and the gateway quest would plan that work twice.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
moq-bot Bot pushed a commit that referenced this pull request Nov 1, 2026
* quest: plan the 2026-11-01 follow-ups (wip)

* quest: plan the 2026-11-01 follow-ups

Fifteen follow-ups the maintainer settled in a /quest-plan interview:
land work (fixer TLS, squash-mismatch comment, landing status), CLI and
agent fixes, runner review checks, and two m1 quests. Also brings
existing quests in line with #349, #276's close, validateFix, #246, and
#341.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* quest: leave review-login to #339 and build the gateway quest on it

#339 finishes and deletes review-login, including reviewing
with credentials from the environment. Narrowing it here would conflict
with that deletion, and the gateway quest would plan that work twice.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: moq-bot <moq-bot@users.noreply.github.com>
@moq-bot
moq-bot Bot disabled auto-merge November 1, 2026 23:25
@moq-bot moq-bot Bot removed the quest:merge label Nov 1, 2026
@moq-bot

moq-bot Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

quest land removed this pull request from the queue: GitHub refused /repos/moq-dev/quest/pulls/339: Pull Request has merge conflicts; the rest of the batch was not merged.

kixelated and others added 2 commits November 1, 2026 18:28
The quest's Goal asks the "not logged in" error to name the Claude profile
its status check read. Every logged-out error, at the status check, after an
API rejection, and with a stripped reroute, now names CLAUDE_CONFIG_DIR, or
CODEX_HOME for Codex, or says the default profile when unset.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated enabled auto-merge (squash) November 2, 2026 02:41
@moq-bot
moq-bot Bot merged commit 3b71497 into main Nov 2, 2026
1 check passed
@moq-bot
moq-bot Bot deleted the quest/m0/review-login branch November 2, 2026 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant