Conversation
Unify CLI authentication by making --user/--password explicit Typer flags and extracting resolve_token() into a shared utility. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix signature (base_url instead of httpx.Client), document resolution chain precedence rationale, add auth status no-credentials behavior, fix Docker paths, add SecretStr support, include list_rooms/login class methods, add ZNDRAW_EMAIL migration warning, and clarify test rewrite scope. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 tasks: shared auth_utils, connection.py refactor, auth status update, all subcommand signatures, ZnDraw client integration, Docker/docs env var rename, and final verification. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TDD: 12 tests covering validation (mutual exclusion, partial combos, SecretStr), resolution chain (explicit, stored, guest), login failure, and ZNDRAW_EMAIL migration warning. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…wrapper Add UserOpt/PasswordOpt option types, update get_connection/get_zndraw signatures, remove os import (env var handling moved to auth_utils). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Supports login validation with mutual exclusion. No guest fallback. Reports token_source as "login" for user/password auth. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mechanical update of ~60 command functions across 18 files. Every command now shows --user and --password in --help output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ed_token Replace bespoke auth chain in __post_init__ and ad-hoc auth in list_rooms()/login() with shared auth_utils.resolve_token(). Breaking: ZnDraw(user="x") without password now raises ValueError instead of falling back to Settings().guest_password. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update docker-compose files and SKILL.md to reflect new env var name. auth_utils.py emits a DeprecationWarning for old ZNDRAW_EMAIL usage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…_task - Update test_client_integration: pass password explicitly (no more implicit Settings().guest_password fallback) - Remove test_client_token_discovery: tested _try_stored_token which no longer exists (covered by test_resolve_token.py) - Fix _execute_task: pass token to child ZnDraw, skip /users/me lookup when token is explicit to avoid extra HTTP round-trip Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
for more information, see https://pre-commit.ci
|
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 (1)
📝 WalkthroughWalkthroughCentralizes authentication via a new Changes
Sequence Diagram(s)sequenceDiagram
actor CLI as CLI / Client
participant Resolve as resolve_token()
participant Store as TokenStore
participant Server as Server API
CLI->>Resolve: resolve_token(base_url, token?, user?, password?)
alt explicit token provided
Resolve-->>CLI: return token
else user & password provided
Resolve->>Server: POST /v1/auth/jwt/login {user,password}
Server-->>Resolve: {access_token}
Resolve-->>CLI: return access_token
else try stored token
Resolve->>Store: get(base_url)
Store-->>Resolve: stored_token?
alt stored token found
Resolve->>Server: GET /v1/auth/users/me (stored_token)
alt 200 OK
Server-->>Resolve: user_data
Resolve-->>CLI: return stored_token
else 401/403
Resolve->>Store: delete(base_url)
Resolve->>Server: POST /v1/auth/guest
Server-->>Resolve: {access_token}
Resolve-->>CLI: return access_token
end
else no stored token
Resolve->>Server: POST /v1/auth/guest
Server-->>Resolve: {access_token}
Resolve-->>CLI: return access_token
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #878 +/- ##
==========================================
- Coverage 91.37% 91.18% -0.19%
==========================================
Files 179 179
Lines 17292 17270 -22
==========================================
- Hits 15800 15748 -52
- Misses 1492 1522 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_client_integration.py (1)
105-156:⚠️ Potential issue | 🟠 MajorConvert negative-case tests to module-level parametrized functions; inconsistent exception translation during auth.
The test structure violates the coding guideline that tests must be module-level functions with
pytest.mark.parametrize, not class methods. The three negative auth cases (test_login_nonexistent_user_raises,test_login_wrong_password_raises,test_user_without_password_raises) should be combined into a single parametrized test.Additionally,
resolve_token()duringZnDraw.__post_init__()raiseshttpx.HTTPStatusErrordirectly, whereas other API calls go throughAPIManager.raise_for_status()which translates toZnDrawError. Consider whether auth failures should be wrapped consistently before escaping the publicZnDraw()API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_client_integration.py` around lines 105 - 156, Combine the three negative auth tests into one module-level pytest.mark.parametrize function (replace test_login_nonexistent_user_raises, test_login_wrong_password_raises, test_user_without_password_raises with a single parametrized test that supplies (user,password,expected_exception) and asserts the expected exception when calling ZnDraw(...)). Also make auth error translation consistent by changing resolve_token() (called from ZnDraw.__post_init__) to not raise raw httpx.HTTPStatusError: either call APIManager.raise_for_status() or catch httpx.HTTPStatusError and re-raise ZnDrawError so auth failures are wrapped the same way as other API calls (adjust the parametrized test to expect ZnDrawError after this change).src/zndraw/cli_agent/rooms.py (1)
61-80:⚠️ Potential issue | 🟡 MinorAdd
conn.close()to clean up the HTTP client.The
get_connection()call returns aConnectionobject wrapping anhttpx.Client, which holds resources (connection pool, sockets). Bothcreate_room(line 74) andset_default_room(line 161) create a connection but never callconn.close(). This causes resource leaks. Add cleanup:with cli_error_handler(): conn = get_connection(url, token, user, password) try: # ... make requests ... finally: conn.close()Or refactor
Connectionto support the context manager protocol (__enter__/__exit__) for automatic cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zndraw/cli_agent/rooms.py` around lines 61 - 80, The create_room (and similarly set_default_room) function opens a Connection via get_connection but never closes it, leaking the underlying httpx.Client; update create_room to ensure the Connection is cleaned up by wrapping the request logic so conn.close() is always called (e.g., create a try/finally around the code that uses conn and call conn.close() in finally), or make the Connection type support context management and use it in a with statement; reference get_connection, Connection, conn.close(), create_room and set_default_room when applying the fix.
🧹 Nitpick comments (2)
src/zndraw/cli_agent/auth.py (1)
117-156: Keepauth statuson the shared auth path.
status()now re-implements the validation and JWT-login branch instead of delegating to the shared resolver/helper, so auth-rule changes can drift here again. Consider factoring out a no-guest variant rather than carrying a second credential-resolution implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zndraw/cli_agent/auth.py` around lines 117 - 156, The status() implementation duplicates credential validation and the JWT-login branch (the token/user/password mutual-exclusion checks, the httpx POST to "/v1/auth/jwt/login", and the store.get resolved-token fallback), which risks drift from the shared resolver; refactor status() to call the existing shared credential-resolution helper used elsewhere (or extract a small no-guest variant in that shared module) instead of reimplementing logic, ensuring the helper returns active_token and token_source (or equivalent) and that status() only handles the json_print output path; retain the same semantics for token/user/password checks, the login POST, and stored-token lookup when delegating.docs/superpowers/specs/2026-03-19-cli-auth-consistency-design.md (1)
112-118: Minor documentation inconsistency regarding backwards compatibility.Lines 112-114 state "Hard rename, no backwards compatibility" for
ZNDRAW_EMAIL→ZNDRAW_USER, but Line 117 describes a migration warning whenZNDRAW_EMAILis set. Consider clarifying: the rename is breaking (the old variable won't work), but there's a helpful warning to guide users during migration.📝 Suggested clarification
## Environment Variables -Hard rename, no backwards compatibility: +Breaking rename with migration warning: - `ZNDRAW_EMAIL` -> `ZNDRAW_USER` - `ZNDRAW_PASSWORD` unchanged -As a migration aid, `resolve_token()` emits a warning to stderr if `ZNDRAW_EMAIL` is set but `ZNDRAW_USER` is not, telling users to rename the variable. +As a migration aid, `resolve_token()` emits a warning to stderr if `ZNDRAW_EMAIL` is set but `ZNDRAW_USER` is not. The old variable is not used; the warning tells users to rename it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-03-19-cli-auth-consistency-design.md` around lines 112 - 118, Update the wording around the env var rename to clarify it's a breaking change but that a migration aid exists: change "Hard rename, no backwards compatibility" to explicitly state that ZNDRAW_EMAIL will no longer be used (breaking) and that resolve_token() will still emit a stderr warning when ZNDRAW_EMAIL is present but ZNDRAW_USER is not, guiding users to rename ZNDRAW_EMAIL → ZNDRAW_USER; mention ZNDRAW_PASSWORD remains unchanged and that the warning is only a temporary migration aid, not full backwards compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/zndraw/auth_utils.py`:
- Around line 60-76: The deprecation warning for ZNDRAW_EMAIL never populates
the legacy value into the user variable, causing a breaking change; before the
validation block that checks token/user/password, alias the old env var into the
expected name by setting user = user or os.environ.get("ZNDRAW_USER") or
os.environ.get("ZNDRAW_EMAIL") (preserving the existing warnings.warn call), so
that existing ZNDRAW_EMAIL+ZNDRAW_PASSWORD setups continue to work while still
emitting the DeprecationWarning; update the logic around the variables token,
user, and password in auth_utils.py accordingly.
- Around line 99-110: The code currently deletes the cached token whenever the
/v1/auth/users/me request returns non-200; change this to only evict on explicit
auth failures by checking resp.status_code and calling store.delete(base_url)
only when resp.status_code is 401 or 403. Locate the logic around
get_token_store(), the entry variable and the client call to "/v1/auth/users/me"
and modify the condition so that successful (200) returns the token, transient
errors (5xx/429/other) do not delete the entry, and only 401/403 trigger
store.delete(base_url).
In `@src/zndraw/cli_agent/connection.py`:
- Around line 229-241: The current except block for httpx.HTTPStatusError in the
_resolve_token() caller always treats errors as 401; update it to preserve the
upstream HTTP status by reading exc.response.status_code and pass that into die,
and set the exit code to EXIT_SERVER_ERROR for 5xx responses (use same logic as
cli_error_handler()) otherwise use EXIT_CLIENT_ERROR; keep the die message
"Authentication Failed" and include str(exc) for the detail. Reference:
_resolve_token, httpx.HTTPStatusError, die, EXIT_SERVER_ERROR,
EXIT_CLIENT_ERROR.
In `@tests/test_resolve_token.py`:
- Around line 42-71: Collapse the four similar tests that call resolve_token
into one pytest.mark.parametrize test (e.g., rename to
test_resolve_token_invalid_args) that iterates over tuples of (kwargs,
expected_message); for each case import resolve_token and call it inside with
pytest.raises(ValueError, match=expected_message). Parametrize the cases for:
({'token':'t','user':'u','password':'p'}, "Cannot combine"),
({'token':'t','password':'p'}, "Cannot combine"), ({'user':'u'}, "Missing
--password"), and ({'password':'p'}, "Missing --user") so the validation matrix
is concise and easy to extend.
---
Outside diff comments:
In `@src/zndraw/cli_agent/rooms.py`:
- Around line 61-80: The create_room (and similarly set_default_room) function
opens a Connection via get_connection but never closes it, leaking the
underlying httpx.Client; update create_room to ensure the Connection is cleaned
up by wrapping the request logic so conn.close() is always called (e.g., create
a try/finally around the code that uses conn and call conn.close() in finally),
or make the Connection type support context management and use it in a with
statement; reference get_connection, Connection, conn.close(), create_room and
set_default_room when applying the fix.
In `@tests/test_client_integration.py`:
- Around line 105-156: Combine the three negative auth tests into one
module-level pytest.mark.parametrize function (replace
test_login_nonexistent_user_raises, test_login_wrong_password_raises,
test_user_without_password_raises with a single parametrized test that supplies
(user,password,expected_exception) and asserts the expected exception when
calling ZnDraw(...)). Also make auth error translation consistent by changing
resolve_token() (called from ZnDraw.__post_init__) to not raise raw
httpx.HTTPStatusError: either call APIManager.raise_for_status() or catch
httpx.HTTPStatusError and re-raise ZnDrawError so auth failures are wrapped the
same way as other API calls (adjust the parametrized test to expect ZnDrawError
after this change).
---
Nitpick comments:
In `@docs/superpowers/specs/2026-03-19-cli-auth-consistency-design.md`:
- Around line 112-118: Update the wording around the env var rename to clarify
it's a breaking change but that a migration aid exists: change "Hard rename, no
backwards compatibility" to explicitly state that ZNDRAW_EMAIL will no longer be
used (breaking) and that resolve_token() will still emit a stderr warning when
ZNDRAW_EMAIL is present but ZNDRAW_USER is not, guiding users to rename
ZNDRAW_EMAIL → ZNDRAW_USER; mention ZNDRAW_PASSWORD remains unchanged and that
the warning is only a temporary migration aid, not full backwards compatibility.
In `@src/zndraw/cli_agent/auth.py`:
- Around line 117-156: The status() implementation duplicates credential
validation and the JWT-login branch (the token/user/password mutual-exclusion
checks, the httpx POST to "/v1/auth/jwt/login", and the store.get resolved-token
fallback), which risks drift from the shared resolver; refactor status() to call
the existing shared credential-resolution helper used elsewhere (or extract a
small no-guest variant in that shared module) instead of reimplementing logic,
ensuring the helper returns active_token and token_source (or equivalent) and
that status() only handles the json_print output path; retain the same semantics
for token/user/password checks, the login POST, and stored-token lookup when
delegating.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10f0acdf-fec0-4b90-a6fd-35a9617166fb
📒 Files selected for processing (29)
docker/production/docker-compose.yamldocker/standalone/docker-compose.yamldocs/superpowers/plans/2026-03-19-cli-auth-consistency.mddocs/superpowers/specs/2026-03-19-cli-auth-consistency-design.mdskills/zndraw/SKILL.mdsrc/zndraw/auth_utils.pysrc/zndraw/cli_agent/admin.pysrc/zndraw/cli_agent/auth.pysrc/zndraw/cli_agent/bookmarks.pysrc/zndraw/cli_agent/chat.pysrc/zndraw/cli_agent/connection.pysrc/zndraw/cli_agent/extensions.pysrc/zndraw/cli_agent/figures.pysrc/zndraw/cli_agent/frames.pysrc/zndraw/cli_agent/geometries.pysrc/zndraw/cli_agent/gif.pysrc/zndraw/cli_agent/jobs.pysrc/zndraw/cli_agent/mount.pysrc/zndraw/cli_agent/presets.pysrc/zndraw/cli_agent/rooms.pysrc/zndraw/cli_agent/screenshots.pysrc/zndraw/cli_agent/selection.pysrc/zndraw/cli_agent/selection_groups.pysrc/zndraw/cli_agent/sessions.pysrc/zndraw/cli_agent/step.pysrc/zndraw/client/core.pytests/test_client_integration.pytests/test_client_token_discovery.pytests/test_resolve_token.py
💤 Files with no reviewable changes (1)
- tests/test_client_token_discovery.py
…ate_credentials - Remove duplicate get_token_store() from connection.py, re-export from auth_utils - Extract validate_credentials() helper in auth_utils, reuse in auth status - Fix stale docstring in ZnDraw.password (no longer inferred from Settings) - Remove unused _user/_password params from open_room command Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
for more information, see https://pre-commit.ci
…ion on 5xx - Bridge ZNDRAW_EMAIL → user with DeprecationWarning (proper deprecation window instead of silent breakage for existing setups) - Only evict stored token on 401/403, raise on transient 5xx/429 - Preserve real HTTP status code in CLI resolve_token error handler - Parametrize validation tests into single test function Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/zndraw/cli_agent/connection.py (1)
224-236:⚠️ Potential issue | 🟠 MajorPreserve the real failure class in this wrapper.
This still collapses upstream 5xx auth failures, transport errors, and malformed auth responses into synthetic 401s. That makes outages look like bad credentials, and the
RequestErrorbranch even prints a 401 while exiting withEXIT_CONNECTION_ERROR.💡 Suggested mapping
try: return _resolve_token(base_url, token=token, user=user, password=password) except ValueError as exc: die(str(exc), str(exc), 400, EXIT_CLIENT_ERROR) except httpx.HTTPStatusError as exc: - die("Authentication Failed", str(exc), 401, EXIT_CLIENT_ERROR) - except (httpx.RequestError, KeyError) as exc: + status = exc.response.status_code + title = "Authentication Failed" if status < 500 else "Server Error" + exit_code = EXIT_CLIENT_ERROR if status < 500 else EXIT_SERVER_ERROR + die(title, str(exc), status, exit_code) + except httpx.RequestError as exc: die( - "Authentication Failed", - f"Failed to authenticate: {exc}", - 401, + "Connection Error", + str(exc), + 503, EXIT_CONNECTION_ERROR, ) + except KeyError as exc: + die("Server Error", f"Malformed auth response: {exc}", 502, EXIT_SERVER_ERROR)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zndraw/cli_agent/connection.py` around lines 224 - 236, The current exception wrapper around _resolve_token collapses different failure classes into synthetic 401s; update the except handlers in the function that calls _resolve_token so they preserve the real failure class and status: keep the ValueError -> die(..., 400, EXIT_CLIENT_ERROR) for malformed input, change the httpx.HTTPStatusError handler to extract exc.response.status_code (or use 500 if missing) and pass that status and EXIT_CLIENT_ERROR/EXIT_SERVER_ERROR appropriately, change the httpx.RequestError handler to use a connection-related status (e.g. 503) and EXIT_CONNECTION_ERROR and include the original exception type/message, and handle KeyError separately (e.g. 502 or 500 with EXIT_CONNECTION_ERROR) so logs and exit codes reflect actual upstream transport vs auth vs response-parsing failures; reference _resolve_token, httpx.HTTPStatusError, httpx.RequestError, KeyError, ValueError and die when making changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/zndraw/cli_agent/auth.py`:
- Around line 113-119: The validate_credentials(token, user, password) call
inside the cli_error_handler block can raise ValueError which bubbles up as a
422; to keep this command consistent with other auth flows, catch ValueError
locally around validate_credentials in src/zndraw/cli_agent/auth.py and re-raise
or return an error that maps to the same 400 contract used by
connection.resolve_token(); i.e., wrap validate_credentials in a try/except
ValueError and convert that exception into the same CLI error/response used by
the resolve_token path (preserving the original message) so auth status behaves
consistently with the other auth-aware entrypoints.
In `@src/zndraw/cli_agent/connection.py`:
- Around line 24-25: The module removed the get_token_store symbol causing
ImportError in cli_agent/auth.py and cli_agent/admin.py; restore a re-export in
src/zndraw/cli_agent/connection.py by importing get_token_store from its
original module (where it was defined) and exposing it at module level (and add
it to __all__ if present) so existing imports like "from .connection import
get_token_store" continue to work; locate the symbol get_token_store and
re-export it from connection.py alongside the existing find_running_server
import.
---
Duplicate comments:
In `@src/zndraw/cli_agent/connection.py`:
- Around line 224-236: The current exception wrapper around _resolve_token
collapses different failure classes into synthetic 401s; update the except
handlers in the function that calls _resolve_token so they preserve the real
failure class and status: keep the ValueError -> die(..., 400,
EXIT_CLIENT_ERROR) for malformed input, change the httpx.HTTPStatusError handler
to extract exc.response.status_code (or use 500 if missing) and pass that status
and EXIT_CLIENT_ERROR/EXIT_SERVER_ERROR appropriately, change the
httpx.RequestError handler to use a connection-related status (e.g. 503) and
EXIT_CONNECTION_ERROR and include the original exception type/message, and
handle KeyError separately (e.g. 502 or 500 with EXIT_CONNECTION_ERROR) so logs
and exit codes reflect actual upstream transport vs auth vs response-parsing
failures; reference _resolve_token, httpx.HTTPStatusError, httpx.RequestError,
KeyError, ValueError and die when making changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e54fa32-a036-4c96-8125-752104563fef
📒 Files selected for processing (5)
src/zndraw/auth_utils.pysrc/zndraw/cli_agent/auth.pysrc/zndraw/cli_agent/connection.pysrc/zndraw/cli_agent/rooms.pysrc/zndraw/client/core.py
✅ Files skipped from review due to trivial changes (1)
- src/zndraw/client/core.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/zndraw/auth_utils.py
Comment on lines
113
to
+119
| with cli_error_handler(): | ||
| from zndraw.auth_utils import validate_credentials | ||
|
|
||
| resolved_url = resolve_url(url) | ||
| store = get_token_store() | ||
|
|
||
| validate_credentials(token, user, password) |
There was a problem hiding this comment.
Keep invalid credential combinations on the same 400 contract as the rest of the CLI.
validate_credentials() raises ValueError, and under cli_error_handler() that comes back as a 422. The shared connection.resolve_token() path returns 400 for the same bad --token / --user / --password combinations, so auth status is now the odd one out. Catch that ValueError locally if you want this command to stay aligned with the other auth-aware entrypoints.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/zndraw/cli_agent/auth.py` around lines 113 - 119, The
validate_credentials(token, user, password) call inside the cli_error_handler
block can raise ValueError which bubbles up as a 422; to keep this command
consistent with other auth flows, catch ValueError locally around
validate_credentials in src/zndraw/cli_agent/auth.py and re-raise or return an
error that maps to the same 400 contract used by connection.resolve_token();
i.e., wrap validate_credentials in a try/except ValueError and convert that
exception into the same CLI error/response used by the resolve_token path
(preserving the original message) so auth status behaves consistently with the
other auth-aware entrypoints.
Comment on lines
24
to
25
| from zndraw.server_manager import find_running_server | ||
|
|
There was a problem hiding this comment.
Re-export get_token_store here or the CLI will not import.
src/zndraw/cli_agent/auth.py and src/zndraw/cli_agent/admin.py still import get_token_store from .connection, and CI is already failing test collection with ImportError. Add the re-export back in this module, or update every remaining importer in the same PR.
🐛 Minimal fix
+from zndraw.auth_utils import get_token_store # noqa: F401
from zndraw.server_manager import find_running_server📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| from zndraw.server_manager import find_running_server | |
| from zndraw.auth_utils import get_token_store # noqa: F401 | |
| from zndraw.server_manager import find_running_server | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/zndraw/cli_agent/connection.py` around lines 24 - 25, The module removed
the get_token_store symbol causing ImportError in cli_agent/auth.py and
cli_agent/admin.py; restore a re-export in src/zndraw/cli_agent/connection.py by
importing get_token_store from its original module (where it was defined) and
exposing it at module level (and add it to __all__ if present) so existing
imports like "from .connection import get_token_store" continue to work; locate
the symbol get_token_store and re-export it from connection.py alongside the
existing find_running_server import.
The re-export from auth_utils was lost during rebase, causing ImportError on CI for admin.py, auth.py, and any module importing get_token_store from connection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/zndraw/cli_agent/connection.py (1)
24-24:⚠️ Potential issue | 🔴 CriticalCritical: Missing
get_token_storere-export causes CI import failures.Both
admin.pyandauth.pyimportget_token_storefrom.connection, butconnection.pydoes not define or re-export it (it is defined inauth_utils.py). This causes anImportErrorat module load time. Add the re-export to fix.🐛 Proposed fix
+from zndraw.auth_utils import get_token_store # noqa: F401 # re-export for CLI modules from zndraw.server_manager import find_running_server🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zndraw/cli_agent/connection.py` at line 24, Add a re-export for get_token_store in connection.py so modules importing it from .connection (e.g., admin.py and auth.py) succeed: import get_token_store from auth_utils (where it is defined) and export it from connection.py (alongside the existing find_running_server import) by adding a single import and ensuring get_token_store is exposed from the module namespace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/zndraw/cli_agent/connection.py`:
- Line 24: Add a re-export for get_token_store in connection.py so modules
importing it from .connection (e.g., admin.py and auth.py) succeed: import
get_token_store from auth_utils (where it is defined) and export it from
connection.py (alongside the existing find_running_server import) by adding a
single import and ensuring get_token_store is exposed from the module namespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c476c0e-8c71-4bc4-b7b1-34b5bd01415d
📒 Files selected for processing (3)
src/zndraw/auth_utils.pysrc/zndraw/cli_agent/connection.pytests/test_resolve_token.py
…n.py Re-export via bare import was stripped by pre-commit as "unused". Restore as a proper function definition that pre-commit won't touch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores