Skip to content

Commit 134fd98

Browse files
igerberclaude
andcommitted
Enforce auth on explicit --backend codex; refresh stale doc strings
R5 review on PR #421 returned ✅ Looks good with two non-blocking items: P2: --backend codex (explicit) only checked for the binary, not auth.json. On a machine with codex installed but `codex login` not run, the script would degrade into a confusing late subprocess error inside `codex exec`, and the runtime banner would unconditionally claim "auth.json detected". - _detect_backend("codex") now also requires CODEX_AUTH_PATH to exist and raises a clear "no codex auth found at ~/.codex/auth.json — run `codex login`" if absent. Auto-mode unaffected (already gated on both checks). - Runtime banner only mentions auth.json when it actually exists (defensive — auto-mode gates already require it, but the explicit- mode change above makes the same invariant true everywhere). - Test test_explicit_codex_with_auth: writes auth.json before assert (was implicitly relying on the now-removed "auth not checked" path) - New test test_explicit_codex_errors_when_auth_missing: codex on PATH but no auth.json → RuntimeError "no codex auth found" P3: Four stale doc/CLI strings refreshed for the dual-backend world: - Skill doc bullet describing .claude treatment now reflects the .claude/settings.local.json scanning + .claude/reviews/ exclusion (was: "skipped wholesale") - Skill doc Step 1 default-behavior text mentions auto-detect backend (was: "live API call") - --dry-run argparse + skill doc help: "without invoking the chosen backend (no api call, no codex subprocess)" (was: "without calling the API") - --repo-root argparse help: per-backend behavior (api: required for standard/deep; codex: optional, falls back to cwd) (was: blanket "required unless --context minimal") Tests: 233 pass (1 new). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 81627c5 commit 134fd98

3 files changed

Lines changed: 53 additions & 11 deletions

File tree

.claude/commands/ai-review-local.md

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,9 +58,14 @@ Notes:
5858
definitions, or doc examples. Filename scan still applies to those
5959
dirs (so a real `.env` checked into `tests/` would still be caught).
6060

61-
Heavy dirs (`.venv`, `node_modules`, `__pycache__`, `.claude`, etc.) are
62-
skipped to avoid vendored test fixtures. Both scans include gitignored
63-
files — gitignored `.env` files are exactly what we want to catch.
61+
Heavy dirs (`.venv`, `node_modules`, `__pycache__`, etc.) are skipped from
62+
the walk to avoid vendored test fixtures. The `.claude/` subtree is NOT
63+
skipped wholesale — `.claude/settings.local.json` (gitignored, common
64+
place for OAuth tokens) is content-scanned. Only `.claude/reviews/` and
65+
`.claude/paper-review/` (agent-generated review artifacts that may quote
66+
literal pattern strings) are excluded from the content scan; their
67+
filenames are still scanned. Both scans include gitignored files —
68+
gitignored `.env` files are exactly what we want to catch.
6469

6570
If either scan finds matches, codex is **NOT invoked** and the script
6671
exits 1 unless you pass `--allow-secrets` to acknowledge the surface.
@@ -89,7 +94,8 @@ Notes:
8994
- `--full-registry`: Include the entire REGISTRY.md instead of selective sections
9095
- `--model <name>`: Override the model (default: `gpt-5.4`). Applies to both backends.
9196
- `--timeout <seconds>`: HTTP request timeout. If omitted, defaults to 900 for reasoning models (gpt-5.4, *-pro, o1/o3/o4) and 300 otherwise. *Api backend only.*
92-
- `--dry-run`: Print the compiled prompt without calling the API
97+
- `--dry-run`: Print the compiled prompt without invoking the chosen backend
98+
(no API call, no codex subprocess)
9399

94100
**Reasoning models** (`gpt-5.4-pro`, `o3`, `o4-mini`, etc.) on the api backend:
95101
Reviews may take 10-15 minutes. For deep reviews with reasoning models, combine
@@ -126,7 +132,8 @@ Step 5 invokes the chosen backend:
126132
### Step 1: Parse Arguments
127133

128134
Parse `$ARGUMENTS` for the optional flags listed above. All flags are optional —
129-
the default behavior (standard context, selective registry, gpt-5.4, live API call)
135+
the default behavior (auto-detect backend, standard context for api or
136+
agentic loading for codex, selective registry, gpt-5.4)
130137
requires no arguments.
131138

132139
### Step 2: Validate Prerequisites
@@ -566,7 +573,7 @@ runs `--force-fresh` or when a rebase invalidates the tracked commit.
566573
# Api backend, extra context files
567574
/ai-review-local --backend api --include-files linalg.py,utils.py
568575

569-
# Preview the compiled prompt without calling the API
576+
# Preview the compiled prompt without invoking the chosen backend
570577
/ai-review-local --dry-run
571578

572579
# Force a fresh review (ignore previous review state)

.claude/scripts/openai_review.py

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1454,6 +1454,12 @@ def _detect_backend(requested: str) -> str:
14541454
"Install via `brew install --cask codex` or "
14551455
"`npm install -g @openai/codex`, then run `codex login`."
14561456
)
1457+
if not os.path.exists(CODEX_AUTH_PATH):
1458+
raise RuntimeError(
1459+
f"--backend codex requested but no codex auth found at "
1460+
f"{CODEX_AUTH_PATH}. Run `codex login` first (subscription "
1461+
f"or API key)."
1462+
)
14571463
return "codex"
14581464
if requested == "api":
14591465
return "api"
@@ -1809,7 +1815,10 @@ def main() -> None:
18091815
parser.add_argument(
18101816
"--dry-run",
18111817
action="store_true",
1812-
help="Print compiled prompt to stdout without calling the API",
1818+
help=(
1819+
"Print compiled prompt to stdout without invoking the chosen "
1820+
"backend (no api call, no codex subprocess)"
1821+
),
18131822
)
18141823
# New arguments
18151824
parser.add_argument(
@@ -1822,7 +1831,12 @@ def main() -> None:
18221831
parser.add_argument(
18231832
"--repo-root",
18241833
default=None,
1825-
help="Repository root directory (required unless --context minimal)",
1834+
help=(
1835+
"Repository root directory. Api backend: required when --context "
1836+
"is 'standard' or 'deep' (used for source-file preloading). Codex "
1837+
"backend: optional — falls back to os.getcwd() and is passed to "
1838+
"`codex exec --cd`."
1839+
),
18261840
)
18271841
parser.add_argument(
18281842
"--include-files",
@@ -2183,8 +2197,17 @@ def main() -> None:
21832197

21842198
# Dispatch on backend.
21852199
if backend == "codex":
2200+
# Only mention auth.json if it actually exists. Auto-mode requires it
2201+
# to pick codex; explicit `--backend codex` now also requires it (see
2202+
# _detect_backend) — but defensive check here prevents misleading log
2203+
# if either invariant ever loosens.
2204+
auth_note = (
2205+
f" (auth.json detected at {CODEX_AUTH_PATH})"
2206+
if os.path.exists(CODEX_AUTH_PATH)
2207+
else ""
2208+
)
21862209
print(
2187-
f"Using codex backend (auth.json detected at {CODEX_AUTH_PATH}); "
2210+
f"Using codex backend{auth_note}; "
21882211
f"sending to {args.model} via `codex exec`...",
21892212
file=sys.stderr,
21902213
)

tests/test_openai_review.py

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1870,9 +1870,12 @@ def test_auto_no_auth(self, patched, review_mod):
18701870
# Don't create auth.json
18711871
assert review_mod._detect_backend("auto") == "api"
18721872

1873-
def test_explicit_codex(self, patched, review_mod):
1873+
def test_explicit_codex_with_auth(self, patched, review_mod):
1874+
"""Explicit `--backend codex` requires both the binary AND auth.json
1875+
— without auth, codex would fail late (subprocess) with a confusing
1876+
error; the explicit-request path now fails fast with actionable text."""
18741877
self._set_codex_present(patched, True)
1875-
# auth.json existence is NOT checked when explicitly requested
1878+
patched["auth"].write_text("{}") # auth present
18761879
assert review_mod._detect_backend("codex") == "codex"
18771880

18781881
def test_explicit_api(self, patched, review_mod):
@@ -1886,6 +1889,15 @@ def test_explicit_codex_errors_when_codex_missing(self, patched, review_mod):
18861889
with pytest.raises(RuntimeError, match="codex.*not installed"):
18871890
review_mod._detect_backend("codex")
18881891

1892+
def test_explicit_codex_errors_when_auth_missing(self, patched, review_mod):
1893+
"""Codex installed but `codex login` not done — fast-fail with a
1894+
clear message instead of degrading into a confusing subprocess
1895+
error inside `codex exec`."""
1896+
self._set_codex_present(patched, True)
1897+
# Don't write auth.json
1898+
with pytest.raises(RuntimeError, match="no codex auth found"):
1899+
review_mod._detect_backend("codex")
1900+
18891901

18901902
class TestBuildCodexCmd:
18911903
"""`_build_codex_cmd` constructs the argv for `codex exec`. The literal

0 commit comments

Comments
 (0)