Skip to content

Adc gemini#57

Merged
doramirdor merged 5 commits into
NadirRouter:mainfrom
froody:adc_gemini
May 25, 2026
Merged

Adc gemini#57
doramirdor merged 5 commits into
NadirRouter:mainfrom
froody:adc_gemini

Conversation

@froody
Copy link
Copy Markdown
Contributor

@froody froody commented May 19, 2026

Mainly add ADC auth so that I can use GOOGLE_CLOUD_LOCATION/GOOGLE_CLOUD_PROJECT instead of an api key to access google models. Also made mid tier display, now my status looks like

% nadirclaw status
NadirClaw Status
----------------------------------------
Simple model:  Qwen3-Coder-480B-A35B-Instruct
Mid model:     gemini-3-flash-preview
Complex model: gemini-3.1-pro-preview
Tier config:   explicit (env vars)
...

@doramirdor
Copy link
Copy Markdown
Collaborator

Review notes (automated scheduled check)

Thanks @froody — ADC support is a great addition and you also caught two real bugs along the way. Tests pass locally (663 ✓). A few items worth addressing before merge:

🐛 Real bug fixes (good)

  1. _dispatch_model_stream Gemini path: _stream_gemini is an async def ... yield ... (i.e., async generator), but it was being consumed with a plain for loop. That code path would have raised TypeError: 'async_generator' object is not iterable the moment anyone actually streamed via Gemini. The async for fix is correct, and the misleading "_stream_gemini is a sync generator" comment is rightly gone.
  2. Robust chunk / finish_reason parsing: defending against the google-genai SDK returning enum-like objects (with .value / .name) is reasonable.
  3. savings.py None tolerance: selected_model: None and tier: None show up in real logs for failed/aborted requests — both fixes are valid.

✋ Please address

1. Duplicate property definitions in nadirclaw/settings.py (lines 46-59 of the PR diff) — MID_MODEL, REASONING_MODEL, and FREE_MODEL are already defined further down in the file (lines ~98, 172, 177 on this branch), with proper fallbacks:

# Already present:
@property
def MID_MODEL(self) -> str:
    return os.getenv("NADIRCLAW_MID_MODEL", "") or self.SIMPLE_MODEL  # line 98

# Your additions (lines 46-59) define them again as empty-string-only stubs.

Python silently keeps only the last @property assignment per class, so the existing definitions still win and your three additions are effectively dead code — but they make the file confusing to read. Suggest removing the new stubs at lines 46-59. (I verified at runtime: settings.MID_MODEL still returns the SIMPLE fallback, not "".)

2. Unnecessary test loosening in tests/test_e2e.py:

-            "messages": [{"role": "user", "content": "Solve the halting problem"}],
+            "messages": [{"role": "user", "content": "Solve the halting problem test free profile"}],
...
-        assert routing["strategy"] == "profile:free"
+        assert "profile:free" in routing["strategy"]

I reverted these two lines locally and the test still passes — the strategy is literally "profile:free" (set at server.py:1343). Looks like a leftover from local debugging. Either restore the strict assertion (matches the sibling test_reasoning_profile_routes_to_complex), or tell me what you were actually trying to test — happy to be wrong here.

🟡 Minor

  • _get_gemini_client ADC caching: caching the client under "adc_default" is fine because google.auth.default() returns auto-refreshing Credentials. No action needed; just flagging since the original api_key-keyed cache had per-key isolation that this loses (which is also fine — ADC is a singleton concept).
  • The two _call_gemini / _stream_gemini early-return guards were both deleted; _get_gemini_client now centralizes the "no creds" HTTPException. Worth a one-line comment in those callers pointing at the new behavior, but not blocking.
  • Optional: the JSONResponse headers=_routing_headers(...) change is defensive — FastAPI does merge headers from the implicit response: Response parameter onto the returned response, but being explicit is clearer.

Suggested next step

Squash these fixes:

  • drop the three duplicate properties in settings.py
  • restore the strict assertion + original prompt in test_free_profile_routes_to_simple

…and this is ready to merge. The ADC feature is genuinely useful and the streaming fix is overdue.

— posted by scheduled review bot on main

@doramirdor
Copy link
Copy Markdown
Collaborator

Friendly ping @froody — no rush, but I'd love to land the ADC support and your _stream_gemini async-generator fix (that one's a real bug). The only blockers from my review above are tiny:

  1. Drop the three duplicate @property stubs at nadirclaw/settings.py lines 46–59 (the real definitions further down already cover them with fallbacks).
  2. Restore the strict routing["strategy"] == "profile:free" assertion + original prompt in tests/test_e2e.py.

If you're swamped, happy to push those two changes to your branch myself (if maintainerCanModify is on) or open a stacked PR with just the cleanup so we can merge yours unchanged — whichever you prefer. Let me know.

— scheduled review bot follow-up

@doramirdor
Copy link
Copy Markdown
Collaborator

Quick administrative note: I approved the fork CI workflow run (it had been sitting in action_required since first push — that's on me for not catching it in my earlier reviews). It just went green across Python 3.10 / 3.11 / 3.12 — see run 26099724743. No new asks; the two cleanup items from my 2026-05-20 review are still the only blockers. — scheduled review bot

- Remove three duplicate @Property stubs at settings.py lines 46-59;
  MID_MODEL, REASONING_MODEL, and FREE_MODEL are already defined
  further down with proper SIMPLE_MODEL/COMPLEX_MODEL fallbacks
- Restore strict 'profile:free' assertion and original prompt in
  tests/test_e2e.py::test_free_profile_routes_to_simple

All 663 tests pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@doramirdor
Copy link
Copy Markdown
Collaborator

Pushed cleanup as 01a4704 directly to your branch (since maintainerCanModify is on) — covers the two items from the 2026-05-20 review:

  • Dropped the three duplicate @property stubs in nadirclaw/settings.py.
  • Restored the strict routing["strategy"] == "profile:free" assertion and original prompt in tests/test_e2e.py.

Full suite still green locally (663 ✓). Waiting on CI; will merge once it goes green unless you object in the next few hours. Thanks again for the ADC support and the streaming async-generator fix — both legit wins.

— scheduled review bot

@doramirdor
Copy link
Copy Markdown
Collaborator

CI is green on the cleanup commit — run 26362224241 across Python 3.10 / 3.11 / 3.12.

I'll leave the merge button to the next scheduled check (~24h) so you have a real window to push back if you'd rather take a different cut at the cleanup — otherwise it'll go in then.

— scheduled review bot

@doramirdor doramirdor merged commit d36170e into NadirRouter:main May 25, 2026
3 checks passed
@doramirdor doramirdor mentioned this pull request May 25, 2026
doramirdor added a commit that referenced this pull request May 26, 2026
Bundles the ADC + Gemini streaming fix landed since 0.17.0:

- Application Default Credentials support for Gemini (#57)
- Fix Gemini streaming async-generator consumption (#57)
- Robust google-genai chunk / finish_reason parsing (#57)
- savings.py tolerates None selected_model / tier in logs (#57)
- nadirclaw status shows mid-tier model when configured (#57)

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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.

3 participants