Repository navigation
[PRE-368] Add token validation into the Python SDK startup - #17
Conversation
Validate credentials against /api/v1/ping during initialize() so invalid or expired tokens fail fast before workers start. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthrough
ChangesToken validation during initialization
Release metadata updates
Build and publish tasks
Lockfile ignore rule
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/tests/test_failure_handling.py (1)
394-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit cleanup assertion for failed initialize() path (Line 401).
This test currently verifies fail-fast auth behavior but not HTTP-context teardown after validation failure. Please assert
__aexit__/close behavior so this leak path is covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/tests/test_failure_handling.py` around lines 394 - 401, The failed initialize() test only checks the auth error and executor state, but it does not verify that the HTTP context is torn down after validation fails. Update the PrefactorCoreClient initialize() failure test to explicitly assert the cleanup path by checking the stub HTTP context’s __aexit__ / close behavior after await client.initialize() raises, using the existing PrefactorCoreClient, initialize(), and close() flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/prefactor_core/client.py`:
- Around line 145-146: The `initialize()` flow in `PrefactorCoreClient` leaks
the entered HTTP context when `validate_token()` fails because `_initialized` is
never set and `close()` is skipped. Wrap the `self._http.validate_token()` call
in `try/except`, and on failure invoke `self._http.__aexit__()` to clean up the
context before re-raising. Also clear the `_http` reference so the client does
not retain a half-initialized context after the exception.
---
Nitpick comments:
In `@packages/core/tests/test_failure_handling.py`:
- Around line 394-401: The failed initialize() test only checks the auth error
and executor state, but it does not verify that the HTTP context is torn down
after validation fails. Update the PrefactorCoreClient initialize() failure test
to explicitly assert the cleanup path by checking the stub HTTP context’s
__aexit__ / close behavior after await client.initialize() raises, using the
existing PrefactorCoreClient, initialize(), and close() flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5b07b1d-d41a-4b9d-b006-8c608cfcc8e9
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.gitignorepackages/core/src/prefactor_core/client.pypackages/core/tests/test_agent_instance_finish_status.pypackages/core/tests/test_agent_instance_register.pypackages/core/tests/test_failure_handling.pypackages/core/tests/test_sdk_header.pypackages/http/src/prefactor_http/client.pypackages/http/tests/test_client.pypackages/langchain/tests/test_middleware.pypackages/livekit/tests/test_session.py
💤 Files with no reviewable changes (1)
- .gitignore
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mise.toml`:
- Around line 65-68: In the publish flow, the package-index check is too
permissive because the current `curl -sf` path in the PyPI/TestPyPI existence
check treats missing packages and transport/server failures the same, which can
incorrectly fall through to `uv publish`. Update the logic around that check to
inspect the HTTP status explicitly, skip publishing only when the response is
200, and abort the run for any other status or request failure in both the PyPI
and TestPyPI branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if curl -sf "https://pypi.org/pypi/${pypi_name}/${version}/json" >/dev/null; then | ||
| echo "${pypi_name} ${version} already on PyPI, skipping" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,180p' mise.tomlRepository: prefactordev/python-sdk
Length of output: 4271
🌐 Web query:
uv publish --check-url duplicate version behavior PyPI JSON endpoint curl -sf 404 transient failure
💡 Result:
When using uv publish, the --check-url option is designed to verify an index for existing files to avoid redundant uploads or to recover from failed/interrupted publishing attempts [1][2][3]. Behavior with Duplicate Versions: When --check-url is provided, uv queries the index before uploading [4]. If a file with the same name and identical content (hash match) already exists on the index, uv skips the upload [3][4]. If a file with the same name exists but the content (hash) differs, uv errors out to prevent inconsistent package releases [4]. If the upload process itself subsequently errors (e.g., due to a network issue), uv will re-check the index URL to determine if the failure was a race condition caused by a parallel upload of the same file, handling these scenarios gracefully [3][4]. Transient Failures and 404s: While uv includes retry mechanisms for transient upload failures [5][6], issues related to 404 responses during the check-url phase often stem from index-specific behaviors rather than transient network errors. For instance, some indexes (like certain GitLab configurations) may return 404s due to token permission issues or specific path handling when querying for packages [7]. Additionally, there have been reported cases where --check-url fails if the index returns unexpected content types (e.g., text/plain) for a 404 response when a package does not exist, rather than the expected JSON or HTML format [8]. Note on Authentication: A known limitation is that credentials (username/password) provided for the main --publish-url are not automatically used for the --check-url request [9]. If the index requires authentication to check for existing packages, you must explicitly include those credentials in the --check-url itself (e.g., --check-url=https://user:pass@registry.example.com/simple) [9].
Citations:
- 1: https://docs.astral.sh/uv/reference/settings/
- 2: https://docs.astral.sh/uv/guides/package/
- 3: https://docs.rs/uv-cli/latest/uv_cli/struct.PublishArgs.html
- 4: Skip existing, second iteration: Check the index before uploading astral-sh/uv#8531
- 5: https://github.com/astral-sh/uv/blob/main/docs/guides/package.md
- 6: Re-add 3 retries in
uv publishastral-sh/uv#12041 - 7:
uv publishfails with a 404 when uploading to a GitLab.com package registry in CI using CI_JOB_TOKEN astral-sh/uv#13815 - 8:
uv publish --check-urlfails for missing package 404 astral-sh/uv#19459 - 9:
uv publish: Credentials not used in checking if package exists astral-sh/uv#11836
Fail closed when the package-index check is inconclusive. curl -sf treats 404, 5xx, and network/TLS failures the same, so a failed check falls through to uv publish. Check the status code explicitly, skip only on 200, and abort on anything else. Same for the TestPyPI branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mise.toml` around lines 65 - 68, In the publish flow, the package-index check
is too permissive because the current `curl -sf` path in the PyPI/TestPyPI
existence check treats missing packages and transport/server failures the same,
which can incorrectly fall through to `uv publish`. Update the logic around that
check to inspect the HTTP status explicitly, skip publishing only when the
response is 200, and abort the run for any other status or request failure in
both the PyPI and TestPyPI branches.
This PR adds API token validation during SDK startup by calling the
/api/v1/pingendpoint before workers are initialized. Invalid or expired tokens now fail fast with aPrefactorAuthErrorduringinitialize()rather than surfacing later during runtime requests.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
dist/and only publish package versions if missing.uv.lockignore entry.