fix(auth): reliable auth-check exit codes + passive validation contract (#1569)#1570
Conversation
…act (#1569) Implements the full auth-verification contract from #1569 for unattended automation (systemd/cron health checks). 1. `auth check` text mode now exits non-zero on any failed executed check, matching `--json`. The Rich-table renderer previously always exited 0, so a health check using `auth check --test` (without `--json`) silently treated expired auth as healthy. Both modes now share one process contract; skipped (`None`) checks do not count as failures. 2. Passive validation: new public `fetch_tokens_passive` does a strictly read-only token fetch — no `NOTEBOOKLM_REFRESH_CMD`, no keepalive poke, no inline PSIDTS `RotateCookies` recovery (it uses the no-recovery strict loader, so a missing/expired PSIDTS surfaces as a `ValueError` instead of a network POST + disk write), and no cookie persistence. `auth check --test --passive` routes through it; the transport-neutral `run_auth_check(AuthCheckPlan(passive=True))` exposes it to MCP/HTTP. 3. `auth refresh --verify` confirms a token fetch actually works after refresh (passive probe), exiting non-zero if the post-refresh cookies still fail — especially valuable with `--browser-cookies`, which rewrites the jar but does not otherwise verify it. 4. macOS login: the "Login not detected within 5 minutes" timeout now hints at `notebooklm login --browser chrome` to reuse an already signed-in system Chrome session. Additive public API (compat audit clean, no allowlist entries). Docs (cli-reference, troubleshooting, CHANGELOG) and the CLI contract baseline updated. Full suite green (9177 passed); mypy + ruff clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@claude review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a read-only passive token-fetch API and CLI flag for auth readiness probes, fixes text-mode auth check exit codes to match JSON behavior, implements post-refresh verification for auth refresh, and improves macOS browser-login timeout guidance. ChangesPassive authentication and health-check validation
User-facing documentation and error guidance
Test infrastructure and guardrail updates
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
Claude finished @teng-lin's task in 2m 39s —— View job PR Review
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces passive authentication validation for unattended monitors, adding a strictly read-only fetch_tokens_passive path that avoids mutating state, running refresh commands, or writing to disk. It also updates the auth check command to exit non-zero on failures in both text and JSON modes, introduces a --verify flag for auth refresh, and improves macOS login recovery hints. The feedback recommends enhancing diagnostic exception handling to prevent potential sensitive information disclosure and explicitly specifying encoding="utf-8" in Path.write_text() calls across tests to ensure cross-platform compatibility.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| except Exception as exc: # noqa: BLE001 — surface any failure as a clean exit 1 | ||
| click.echo( | ||
| f"Error: refresh completed but the post-refresh token fetch failed: {exc}", | ||
| err=True, | ||
| ) |
There was a problem hiding this comment.
When handling exceptions in read-only diagnostic tools or status checks, surface only the exception type name (e.g., using type(exc).__name__) instead of the raw exception message or object to avoid echoing sensitive information like raw file paths or values in the diagnostic output.
| except Exception as exc: # noqa: BLE001 — surface any failure as a clean exit 1 | |
| click.echo( | |
| f"Error: refresh completed but the post-refresh token fetch failed: {exc}", | |
| err=True, | |
| ) | |
| except Exception as exc: # noqa: BLE001 — surface any failure as a clean exit 1 | |
| click.echo( | |
| f"Error: refresh completed but the post-refresh token fetch failed: {type(exc).__name__}", | |
| err=True, | |
| ) |
References
- When handling exceptions in read-only diagnostic tools or status checks, surface only the exception type name (e.g., using 'type(exc).name') instead of the raw exception message or object to avoid echoing sensitive information like raw file paths or values in the diagnostic output.
There was a problem hiding this comment.
Keeping as-is — this would contradict the established convention in this module. test_auth_refresh_failure_does_not_print_exception_class documents that auth refresh deliberately surfaces the exception message (str(exc)) and drops type(exc).__name__ (the class name was the "implementation-detail leakage" that was removed). This handler already prints str(exc), not the class name, so it's consistent with that policy.
Showing the message is also the whole point of --verify for unattended use: the operator needs the actual failure reason (expired cookies vs. network error vs. missing file) in cron logs — collapsing it to a type name would gut the diagnostic value. The token-fetch redirect message is already URL-redacted via _safe_url, and storage paths aren't secret in this codebase (they're printed by auth check / auth refresh themselves).
… passive coverage - Add explicit encoding="utf-8" to write_text/read_text in the new auth tests (gemini-code-assist: Windows cp1252 defensiveness). - Add test_auth_check_passive_with_env_auth_passes_none_path: covers the passive probe under NOTEBOOKLM_AUTH_JSON (token_path=None), the env-auth gap noted by the claude review bot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough reviews. Addressed in 11e2172:
|
|
Acknowledged. The additional test coverage for the environment-auth path and the consistent encoding usage are noted. |
2 participants
Summary
Implements the full auth-verification contract requested in #1569 for unattended automation (systemd/cron health checks). Four coordinated changes, one coherent theme: make auth verification a reliable, side-effect-aware contract.
1.
auth checktext mode exits non-zero on failure (the bug)The Rich-table renderer (
cli/_session_render.py) printed failed checks but always exited0, while--jsonexited non-zero — so a health check usingauth check --testwithout--jsonsilently treated expired auth as healthy. Both modes now share one process contract: exit0only when every executed check passes; non-zero otherwise. Skipped (None) checks (e.g. the token fetch without--test) do not count as failures.2. Passive validation mode (
--passive+fetch_tokens_passive)auth check --testrunsfetch_tokens_with_domains, which on auth expiry runsNOTEBOOKLM_REFRESH_CMD, fires the keepalive rotation poke, can trigger inline PSIDTSRotateCookiesrecovery, and writesstorage_state.json— none of which a passive readiness probe should do. New publicnotebooklm.auth.fetch_tokens_passiveis strictly read-only:NOTEBOOKLM_REFRESH_CMD;poke=False);ValueErrorinstead of a network POST + disk write;auth check --test --passiveroutes the token probe through it. The transport-neutralrun_auth_check(AuthCheckPlan(..., passive=True))exposes the same probe to the MCP/HTTP adapters (the Python-API ask in the issue).3.
auth refresh --verifyA successful refresh command does not prove the resulting cookies authenticate (they may still redirect to sign-in).
--verifyruns the passive probe after refresh and exits non-zero if it fails — especially valuable with--browser-cookies, which rewrites the jar but does not otherwise verify it.4. macOS login recovery hint
The
Login not detected within 5 minutestimeout now suggestsnotebooklm login --browser chrometo reuse an already signed-in system Chrome session (often detects immediately; sidesteps bundled-Chromium issues on macOS).Contract
auth check(both text and--json) exits0iff all executed checks pass.auth check --test --passiveandauth refresh --verifygive automation a fail-loud, side-effect-free readiness signal.Testing
mypy+ruffclean.NOTEBOOKLM_REFRESH_CMD, and no inline PSIDTS recovery (a regression test proven to fail on the non-strict loader).auth.__all__public-surface guardrail.Compatibility
Additive public API (
fetch_tokens_passive);scripts/audit_public_api_compat.pyexits 0 with no new allowlist entries — not an API-breaking change. Docs updated:cli-reference.md,troubleshooting.md,CHANGELOG.md.Closes #1569.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--passivetoauth checkfor read-only credential validation and--verifytoauth refreshto confirm cookies after refreshBug Fixes
auth checktext mode now exits non‑zero on failures (matches JSON behavior)--browser chromeDocumentation
Tests