From eeec0c61d731785873f2b5c434c2377bcca203ab Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 30 Oct 2026 17:25:43 +0000 Subject: [PATCH] quest: review keeps a reviewer's environment credentials (#295) * quest: review keeps a reviewer's environment credentials Co-authored-by: Claude Opus 5.5 --- quest/m0/README.md | 2 +- quest/m0/review-login.md | 22 ++++++++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/quest/m0/README.md b/quest/m0/README.md index bf94f38b..8c146ba2 100644 --- a/quest/m0/README.md +++ b/quest/m0/README.md @@ -41,7 +41,7 @@ ownership-manifest design: - [Merge through a host gate](/quest/m0/host-land/README.md) - inline merge picks, small batches landed from the maintainer's host, and a Codex fixer instead of bisection - [Codex cloud off](/quest/m0/codex-cloud-off.md) - automatic Codex cloud reviews stop once no open PR relies on them -- [Review reports an expired login](/quest/m0/review-login.md) - `quest review` says to log in again instead of failing with a 401 +- [Review handles expired and environment logins](/quest/m0/review-login.md) - `quest review` says to log in again instead of failing with a 401, and reviews with credentials from the environment - [Live sandbox test](/quest/m0/review-sandbox.md) - the real reviewer CLIs run nothing a reviewed head plants - [Kix decision UI](/quest/m0/kix/README.md) - planning and implementation through prompts and summaries, run on the maintainer's own runners - [Agent test environment](/quest/m0/agent-test-env.md) - `just test` passes inside agent sessions diff --git a/quest/m0/review-login.md b/quest/m0/review-login.md index a9389511..9dc82e7e 100644 --- a/quest/m0/review-login.md +++ b/quest/m0/review-login.md @@ -1,10 +1,11 @@ -# [XS] Review reports an expired login +# [S] Review handles expired and environment logins ## Goal When a reviewer CLI's login has expired or been revoked, `quest review` fails with the same "`` is not logged in; run ``" error it gives a -signed-out CLI, instead of a raw 401 from the reviewer's turn. +signed-out CLI, instead of a raw 401 from the reviewer's turn. A reviewer +whose only credentials come from the environment still reviews. ## Plan @@ -20,3 +21,20 @@ Decided 2026-10-29 (user, /quest-plan of the follow-ups from - Keep the `status` check for the signed-out case. - Cover it with the fake-CLI tests: one reviewer that exits with each CLI's auth error text. + +Decided 2026-10-30 (user, /quest-plan of the follow-ups from a moq-dev/moq +`/quest-complete` session): ✅ extend this quest / drop. + +- `quest review --harness claude` reported "claude is not logged in" in + sessions where Claude Code's credentials came only from `ANTHROPIC_*` or + `CLAUDE*` variables. `strippedEnvironment` (`src/cli/agent.ts`, used at + `src/cli/review.ts`'s status check) removes every one except + `CLAUDE_CONFIG_DIR`, so the status check fails. Codex-written PRs then got + no Claude review (moq-dev/moq#7084, moq-dev/moq#7251). +- Let a reviewer keep the credential its own CLI would read from the + environment, while still stripping variables that would redirect billing or + configuration. Weigh that against why `STRIPPED` exists before choosing + which names pass. If a credential is stripped on purpose, the error names the + variable instead of saying "not logged in". +- Test with a fake reviewer that authenticates only through an environment + variable.