fix(index): teach the keyword-only call shape when upsert_records gets positional args#694
Open
joerg84 wants to merge 2 commits into
Open
fix(index): teach the keyword-only call shape when upsert_records gets positional args#694joerg84 wants to merge 2 commits into
joerg84 wants to merge 2 commits into
Conversation
…s positional args upsert_records(ns, records) previously failed with the interpreter's bare 'takes 1 positional argument but 3 were given', which names neither the parameters nor the fix. The keyword-only contract is deliberate (parameter order changed across SDK generations, so positionals would silently mis-bind); the failure just needs to say so. Add a reject_positional_args guard that raises PineconeTypeError (a TypeError subclass) carrying the exact call shape, applied to the sync, async, and gRPC variants. inspect.signature still reports the real keyword-only signature via __wrapped__. Fixes #691 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…methods The async branch wrapped with 'async def', deferring the guard to await time - an un-awaited misuse returned a coroutine and a 'never awaited' warning instead of the error, where native keyword-only binding raises at the call. Use one plain wrapper for both cases (returning the coroutine for async methods) so the check runs at call time, and mark the wrapper via inspect.markcoroutinefunction on 3.12+ so iscoroutinefunction still reports correctly. Flagged by Cursor Bugbot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3ac800e. Configure here.
| return fn(*args, **kwargs) | ||
|
|
||
| if sys.version_info >= (3, 12) and inspect.iscoroutinefunction(fn): | ||
| inspect.markcoroutinefunction(wrapper) |
There was a problem hiding this comment.
Coroutine detection broken on older Python
Low Severity
reject_positional_args only applies inspect.markcoroutinefunction on Python 3.12+, so on supported 3.10/3.11 inspect.iscoroutinefunction and asyncio.iscoroutinefunction report False for AsyncIndex.upsert_records. Callers that branch on that check can treat the method as sync and mishandle the returned coroutine.
Reviewed by Cursor Bugbot for commit 3ac800e. Configure here.
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
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.


Problem
index.upsert_records("ns", records)fails with the interpreter's bareTypeError: Index.upsert_records() takes 1 positional argument but 3 were given— naming neither the parameters nor the fix. The keyword-only contract is deliberate and stays (parameter order changed across SDK generations, so accepting positionals would silently mis-bind old-style calls); the failure just needs to say so.Fix
New
reject_positional_args(example)guard in_internal/validation.py, applied to the sync, async, and gRPCupsert_records:PineconeTypeErrorsubclassesTypeError, so existingexcept TypeErrorhandlers (and the existingtest_upsert_records_keyword_onlytest) keep working unchanged.functools.wrapspreserves__wrapped__, soinspect.signature()still reports the real keyword-only signature — verified by a new test.Testing
uv run pytest tests/unit: 4603 passed (socket_options linux/darwin failures are pre-existing on macOS main, untouched).ruff check,ruff format --check,mypy --strictclean on all touched files.Fixes #691
Found by a cold-agent audit: 2/2 SDK-path agents guessed the positional form and burned 3–4 turns on the bare TypeError; one read the SDK source to find the contract.
🤖 Generated with Claude Code
Note
Low Risk
Behavior change is limited to error messaging for invalid call sites; successful keyword-only calls are unchanged and
PineconeTypeErrorremains aTypeErrorsubclass for existing handlers.Overview
Adds a reusable
reject_positional_argsdecorator inpinecone/_internal/validation.pyand applies it toupsert_recordson syncIndex,AsyncIndex, andGrpcIndex.When callers pass extra positional arguments (e.g.
upsert_records("ns", records)), the SDK now raisesPineconeTypeErrorwith the correct keyword-only example and a note that parameter order changed across SDK generations—instead of Python’s generic “takes 1 positional argument but N were given.” The wrapper still allows onlyselfpositionally, preservesinspect.signatureviafunctools.wraps, and for async methods fails at call time (before a coroutine is created); on Python 3.12+ async wrappers are marked forinspect.iscoroutinefunction.Unit tests cover message content for sync/async/gRPC, async call-time behavior, and keyword-only signature introspection on
Index.upsert_records.Reviewed by Cursor Bugbot for commit 3ac800e. Bugbot is set up for automated code reviews on this repo. Configure here.