Skip to content

[REVIEW STAGING] Support CLAUDE_CODE_EFFORT and CLAUDE_CODE_OAUTH_TOKEN for the Claude Code agent - #1

Closed
rrailton wants to merge 2 commits into
mainfrom
claude-effort-env-var
Closed

[REVIEW STAGING] Support CLAUDE_CODE_EFFORT and CLAUDE_CODE_OAUTH_TOKEN for the Claude Code agent#1
rrailton wants to merge 2 commits into
mainfrom
claude-effort-env-var

Conversation

@rrailton

@rrailton rrailton commented Jul 20, 2026

Copy link
Copy Markdown
Owner

Note

Fork-internal staging PR for review only — do not merge. The real PR will target dbt-labs/ade-bench:main with this same diff after review.

What

Two opt-in env vars for the Claude Code agent, in two separable commits:

  1. CLAUDE_CODE_EFFORT — appends --effort <value> (low|medium|high|xhigh|max) to the claude invocation. This is the Claude Code counterpart of OPENAI_CODEX_REASONING_EFFORT (DI-4253: support OPENAI_CODEX_REASONING_EFFORT env var dbt-labs/ade-bench#149), with the same design: an env var rather than a constructor kwarg, so no harness/factory plumbing changes. Value is shlex.quoted. Note --effort requires a recent Claude Code CLI; the setup script installs latest, so this can't regress existing runs.
  2. CLAUDE_CODE_OAUTH_TOKEN — when set, the sandboxed CLI authenticates with a long-lived subscription OAuth token (from claude setup-token) instead of ANTHROPIC_API_KEY, letting subscription holders run benchmarks against their plan quota rather than metered API billing. The API key remains the default; this commit is separable if a single auth path is preferred.

Both are covered by unit tests mirroring test_openai_codex_agent.py (no-env-var, value emitted, shell-quoting; plus auth-precedence cases). Neither changes behavior when the env vars are unset.

Why

Same motivation as dbt-labs#149: comparing agents/models at matched effort levels. As a working example, we used this to run all 60 ready duckdb+dbt tasks — 75 trials per arm once per-task prompt variants are included — at 1 attempt per trial (Claude Code pinned to 2.1.207) comparing claude-fable-5 --effort low vs claude-opus-4-8 --effort xhigh:

fable-5 / low opus-4-8 / xhigh
pass@1 72.0% (54/75) 74.7% (56/75)
discordant tasks 2 4
sign test p = 0.69 (parity)
avg wall-clock/trial 85s 181s
output tokens (arm) 237k 913k

i.e. statistical parity on outcomes with a ~2x latency and ~4x token gap — the kind of readout this flag makes easy to produce.

Testing

  • pytest tests/agents/installed_agents/ — 17 passed (11 existing + 6 new)
  • Both env vars verified end-to-end in the benchmark runs above: the --effort flag is present in the recorded container commands (commands.txt artifacts), and OAuth auth confirmed via subscription usage accounting
  • Per-trial modelUsage audit across all 75 fable-5 trials confirmed no cross-model fallback occurred (fable-5 + the CLI's internal Haiku utility model only)

🤖 Left by Claude Code

@rrailton rrailton self-assigned this Jul 20, 2026
@rrailton

Copy link
Copy Markdown
Owner Author

High

Benchmark log artifacts committed at repo root (full-run.out, full-run2.out) — Goal-fit: Goal-critical · Effort: trivial
Verified: PR file list shows both files ADDED (296 + 290 lines); read their heads — they're raw matrix-runner console logs (harness tables, run IDs, timestamps). Checked .gitignore at PR head: outputs/, logs/, *.log are ignored but *.out is not.

These are run artifacts from the experiment described in the PR body, not part of either commit's stated scope (neither commit message mentions them). The PR body says the same diff will target dbt-labs/ade-bench:main — upstream maintainers will (rightly) ask for their removal, and they'd sit unignored at the repo root inviting more of the same. Suggested fix: drop both files from the branch (they're fine as evidence in the PR description table, a gist, or a PR comment); optionally add *.out to .gitignore if the matrix runner keeps writing them.

Low

No fail-fast validation of CLAUDE_CODE_EFFORT values (claude_code_agent.py:55-56) — Goal-fit: Polish · Effort: one-edit
Verified: read full claude_code_agent.py and openai_codex_agent.py; the Codex counterpart (OPENAI_CODEX_REASONING_EFFORT) also passes the value through unvalidated, so this matches the precedent the PR deliberately mirrors.

A typo (CLAUDE_CODE_EFFORT=hgih) isn't caught until the CLI errors inside every trial's container, potentially burning a batch of trials before anyone notices. Validating against {low, medium, high, xhigh, max} at command-construction time would fail the run immediately with a clear message. Counterargument: the valid set is CLI-version-dependent, and hardcoding it means the harness lags the CLI — which is presumably why Codex didn't validate either. Reasonable to leave as-is for upstream-parity; flagging so it's a decision, not an oversight.

ab check still gates on ANTHROPIC_API_KEY in OAuth-only environments (ade_bench/cli/ab/check.py:143) — Goal-fit: Goal-adjacent · Effort: one-edit
Verified: grepped all non-agent consumers of ANTHROPIC_API_KEY; the only other one is the ab check environment anthropic preflight, which knows nothing about the OAuth token.

A subscription-only user (the exact audience of the second commit) who runs the preflight gets told their Anthropic credentials are missing even though benchmarks will work. Worst case is confusion, not breakage. Suggested fix: teach check_anthropic_key to accept CLAUDE_CODE_OAUTH_TOKEN as satisfying the check — or note the limitation in the upstream PR body so maintainers can decide.

Unescaped single-quote wrapping in env setup applies to the new token (abstract_installed_agent.py:71) — Goal-fit: Hypothetical · Effort: one-edit
Verified: read _create_env_setup_file — it emits export KEY='value' with no escaping; the OAuth token is a fixed-alphabet sk-ant-oat01-… string that cannot contain '.
Inferred: token alphabet from claude setup-token output format, not from a spec.

Pre-existing pattern (API keys go through the same path), so not introduced by this PR — noting only because this PR adds a new secret flowing through it. No action needed unless touching that function anyway.

Handoff readiness

  • PR description is strong: motivation, precedent link (dbt-labs #149 — verify that renders as intended in the upstream repo, where it will auto-link correctly; in this fork it links to nothing), test evidence, and end-to-end verification including a modelUsage fallback audit. Nothing missing for an upstream reviewer except the artifact cleanup above.
  • Tests mirror test_openai_codex_agent.py in structure and location — an upstream reviewer will recognize the pattern immediately.
  • The --effort-requires-recent-CLI caveat is already noted in the PR body. ✔
Scoping verdict (for author's merge decision)
  • Goal-fit: the two commits precisely serve the stated goal and are cleanly separable as described; the only scope drift is the two accidental .out artifacts.
  • Polish ratio (this round): 1 goal-critical / 1 goal-adjacent / 1 polish / 1 hypothetical out of 4 findings.
  • Recommendation: one more round after the artifact removal, then ship to upstream.

🤖 Left by Claude Code

Mirrors OPENAI_CODEX_REASONING_EFFORT (dbt-labs#149): an optional env-var override
for Claude Code's --effort level (low|medium|high|xhigh|max), useful when
comparing agents or model configurations at matched effort levels. Set via
env var rather than a constructor kwarg so the existing harness/factory
plumbing doesn't need to change.
@rrailton
rrailton force-pushed the claude-effort-env-var branch from 7c8e047 to aa8abf4 Compare July 20, 2026 14:58
rrailton added a commit that referenced this pull request Jul 20, 2026
…nner

Review findings from #1: pilot logs were swept into
history by git add -A; effort typos previously burned a container batch
before the CLI rejected them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Eet1UBLbqnwM4fHQKdNxLM
@rrailton

Copy link
Copy Markdown
Owner Author

All four findings resolved or dispositioned; branch force-pushed (old tip preserved at backup/claude-effort-env-var-v1).

  • [High] .out artifacts committed — Fixed by rewriting the branch: full-run.out/full-run2.out are gone from history (they'd been swept in by a git add -A). The rewrite also fixed a related defect the review didn't flag: old commit 1 contained the OAuth _env change, so the commits weren't actually separable as the PR body claimed. Now: 0fdc736 = effort only, aa8abf4 = OAuth only. The fork working branch had the same problem (pilot*.out tracked) — untracked there and *.out/matrix-runs.log added to the fork's .gitignore (1bc2f9e, fork-only).
  • [Low] No effort validation — Left as-is upstream, per the review's own counterargument: the valid set is CLI-version-dependent and the unvalidated pass-through matches the Codex precedent this PR mirrors. The burn-a-batch risk is real for our own runs, so validation added to the fork-only run-matrix.sh wrapper instead (1bc2f9e).
  • [Low] ab check OAuth-blind — Fixed in aa8abf4: check_anthropic_key now accepts CLAUDE_CODE_OAUTH_TOKEN (checked before the API key so OAuth-only environments emit no spurious failure). Folded into commit 2 since it completes that commit's subscription-auth story.
  • [Low] Env-file quoting — No action, per the review's own conclusion: pre-existing path, and the sk-ant-oat01-… token alphabet cannot contain a single quote.
  • Handoff note on #149 rendering — Confirmed; the upstream PR body will use the bare #149 form.

Tests: 17/17 passing on the rewritten branch.


🤖 Left by Claude Code

@rrailton

Copy link
Copy Markdown
Owner Author

Medium

Trial arithmetic in the PR body doesn't reconcile (PR description, "Why" section) — Goal-fit: Goal-adjacent · Effort: trivial
Verified: pass@1 fractions are internally consistent (54/75 = 72.0%, 56/75 = 74.7%) and the sign test checks out (2 vs 4 discordant of 6, two-sided p ≈ 0.69).
Inferred: the task-to-trial mapping — I can't see the run manifest.

"all 60 ready duckdb+dbt tasks (75 trials/arm, 1 attempt)" — 60 tasks × 1 attempt is 60 trials, not 75. Presumably some tasks run multiple variants/trials, but an upstream reviewer doing the same arithmetic will trip on it, and this body ships verbatim to dbt-labs. Suggested fix: one clause explaining where the extra 15 trials come from (e.g. "60 tasks, 75 trials/arm after task variants"), or correct whichever number is wrong.

Low

Whitespace-handling skew between the preflight and the agent (claude_code_agent.py:33 vs the check.py OAuth branch) — Goal-fit: Hypothetical · Effort: trivial
Verified: read full claude_code_agent.py and the check.py diff; the preflight does (os.getenv(...) or "").strip() while _env truthy-checks the raw value.

A whitespace-only CLAUDE_CODE_OAUTH_TOKEN (a stray export CLAUDE_CODE_OAUTH_TOKEN=' ' in a .env) is treated as unset by ab check — which then passes via the API key — but as set by _env, which silently prefers the garbage token over the valid key, and every trial fails auth at runtime after a green preflight. Unlikely input, but the fix is one .strip() in _env to match the preflight's semantics.

No unit test for the check.py OAuth branch — Goal-fit: Polish · Effort: one-edit
Verified: searched the whole tests/ tree — no CLI/check.py tests exist anywhere, so untested matches repo convention; the agent-side precedence cases are covered in TestAuthEnv.

The precedence logic now lives in two places (agent _env and preflight) but only one is tested. Matching-convention argument is legitimate; flagging so it's a decision, not an oversight.

Handoff readiness

  • Verified the PR body's "the setup script installs latest" claim against the actual upstream dbt-labs/ade-bench setup script — it is unpinned there, so the claim is accurate for the target repo (the fork's 2.1.207 pin is fork-local and outside this diff). ✔
  • The round-1 resolution comment gives an upstream reviewer a clean audit trail of what changed between branch versions, including the backup ref. ✔
  • Only the trial-count reconciliation above stands between this and upstream submission.
Scoping verdict (for author's merge decision)
  • Goal-fit: the rewritten branch matches the stated goal exactly; the two commits are now genuinely separable and no scope drift remains.
  • Polish ratio (this round): 0 goal-critical / 1 goal-adjacent / 1 polish / 1 hypothetical out of 3 findings.
  • Recommendation: ship now (after the one-line PR-body number reconciliation).
  • Round note: no load-bearing gaps remain and the polish ratio is rising — a third round would be past the point of diminishing returns for a diff this size.

🤖 Left by Claude Code

@rrailton
rrailton marked this pull request as draft July 20, 2026 15:08
When set, the Claude Code agent authenticates the sandboxed CLI with a
long-lived subscription OAuth token (from `claude setup-token`) instead of
an API key, letting subscription holders run benchmarks against their
plan's usage quota rather than metered API billing. ANTHROPIC_API_KEY
remains the default when the token is not set.

Also teaches the `ab check environment anthropic` preflight to accept the
token, so OAuth-only environments don't report missing credentials.
@rrailton
rrailton force-pushed the claude-effort-env-var branch from aa8abf4 to 4d93e4e Compare July 20, 2026 15:09
@rrailton

Copy link
Copy Markdown
Owner Author

Round-2 findings resolved; commit 2 amended and force-pushed (aa8abf44d93e4e).

  • [Medium] Trial arithmetic — Fixed in the PR body: now reads "all 60 ready duckdb+dbt tasks — 75 trials per arm once per-task prompt variants are included — at 1 attempt per trial". The extra 15 trials are the harness expanding tasks that define multiple prompt variants (e.g. f1003.hard, analytics_engineering002.medium).
  • [Low] .strip() skew — Fixed in 4d93e4e: _env now strips the token before the truthy check, matching the preflight's semantics, with a new test locking it (test_whitespace_oauth_token_treated_as_unset — whitespace-only token must not shadow a valid API key). 18/18 tests pass.
  • [Low] No check.py test — Deliberately left untested: no CLI/check tests exist anywhere in the repo, and introducing the first test scaffolding for cli/ab/ inside this PR would be scope creep for upstream. The agent-side precedence semantics are what matter for trial correctness, and those are covered by TestAuthEnv.

Taking the round note's recommendation: no round 3; proceeding to the upstream submission after author sign-off.


🤖 Left by Claude Code

@rrailton

Copy link
Copy Markdown
Owner Author

Review staging complete (2 rounds, all findings resolved). Shipped upstream as dbt-labs#153 — closing without merge as intended; branch retained.


🤖 Left by Claude Code

@rrailton rrailton closed this Jul 20, 2026
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