chore: ship the licence text and require a CLA - #91
Merged
Conversation
The server's advisor groups what it measures by the shape of the query, so its report can name a statement but not the place that wrote it: one line of Cypher usually has a dozen call sites. Built with debug_source_tracking, the client now sends the file, line and function each query was written at, and the report names the line instead. app_name and app_version ride along, naming the service when several share a database. The keys are the contract the Rust driver already speaks, and this fills the one it has to leave empty: it reads the caller through #[track_caller], and a Location carries no function name, while a Python frame does. Off by default and free while off: the flag gates the frame read itself, and the metadata argument is not passed at all, so a query makes the call it made before this existed. The frame is read directly rather than through inspect.stack, which walks the whole stack and opens each frame's source file to quote lines around it: file I/O per frame to answer a question about one of them. The synchronous client reads the location on the way in rather than inside the coroutine, because it hands that coroutine to an event loop and the frame that called it has returned by the time it runs. A query handed to create_task is past saving that way, so it reports nothing: what is left on the stack is an event-loop frame, and the advisor would collect the queries of every unrelated task under that one line, which is worse than silence.
Reading the location inside the coroutine only worked for a query that was awaited where it was written. Everything that runs a coroutine as a task starts the body long after the calling frame has returned, so create_task, gather, wait_for, shield and TaskGroup all reported an event-loop frame, which the call-site filter then discarded: concurrent queries, the ones most worth attributing, silently sent nothing. The query methods now read the location when they are CALLED, which is the one moment all of those share, and are plain methods returning a coroutine so there is such a moment at all. Two ways the aid could take a query down with it, both now closed. A path or function name outside ASCII went into a metadata key with no -bin suffix, which carries an HTTP/2 header value and must be ASCII; gRPC enforces that on the client, so the call failed before it was sent and the query never reached the server. A checkout under a non-ASCII path is enough to trigger it, and Python allows such a function name too. The values are escaped now, not dropped: an escaped path still names the file and still groups with itself, and dropping the file key alone would make the server discard the whole context anyway. The frame read caught only ValueError, the failure a stack shorter than the walk gives. An application whose audit hook refuses the sys._getframe event raises whatever it likes instead, and that escaped before the request was built. To a caller all of these are one thing — no location — and none is worth failing a query over. Carries a regression test per fix: the five scheduling wrappers, a location whose file, function and application name are all non-ASCII, and a frame read that raises. All were seen failing first.
The previous escape covered the wrong range. gRPC refuses a control character in a metadata value exactly as it refuses a non-ASCII one, and 0x7f with them, but backslashreplace leaves all of those untouched: a newline, a tab or a NUL went out raw and failed the call before it was sent. So enabling source tracking could still be the reason every query fails, which is what the escape existed to prevent. The likeliest way in is mundane rather than exotic. An application name read from a file arrives with the newline that ended it, and a POSIX path may legally contain one. The bar is now printable ASCII, stated as a predicate rather than delegated to a codec whose range would have to be verified. Values that need nothing — every ordinary path and name — are returned unchanged after two scans, so the common path allocates nothing where it previously built a new string every time. Carries a regression test with a newline, a tab and a NUL across the file, function and application values, seen failing first. The escaped output was also put through grpc to confirm it now reaches the transport, where the raw one was rejected.
…g call sites Two ways the previous round went wrong. Making the query methods plain methods returning a coroutine took the call site back from every scheduling wrapper, but it also changed the answer to a question callers ask: iscoroutinefunction said False. Code that dispatches on it broke, most consequentially create_autospec, which then built a synchronous double whose result a test cannot await. That happened with tracking off, so it fell on people not using the feature at all. The methods are now marked as standing in for the coroutine functions they replace, both ways, since the lever differs by version. The escaping was not injective, and for this feature that is not a lost detail but a wrong answer: the advisor groups by what it receives, so two call sites arriving as one string are reported as one place in the code, with the queries of one attributed to the other. A name holding a real newline came out as a name holding the four characters that spell its escape, and a character outside the basic plane came out as a character inside it followed by a digit. The escape character now escapes itself and each form is padded to a fixed width. Carries a regression test per defect, each seen failing first: both introspection predicates and an autospecced double that gets awaited, a newline against its literal spelling, and an astral character against a BMP one followed by a digit. The encoding was also brute-forced over an alphabet of the characters that trip it, three deep: no collisions, nothing outside the permitted range.
…ines Marking a plain method as standing in for a coroutine function only worked from 3.12, where the marker is honoured. On 3.11 it reaches asyncio.iscoroutinefunction and therefore mock, but inspect.iscoroutinefunction has no such lever and answered False, which the tests caught there. The methods are coroutine functions again, and the location is read at the moment that serves both halves: attribute lookup. It happens in the caller's own frame, and it happens for every way of running the query, since create_task, gather, wait_for, shield and a TaskGroup all begin with `client.cypher(...)`. What the caller gets is a partial over the coroutine function, which inspect looks through, so the predicates are right on every supported version rather than on the newest ones. The signature is declared without self, because a descriptor that is not a plain function is not recognised as a method by create_autospec, which then leaves self in and binds the first real argument to it. With tracking off the lookup returns the ordinary bound method: no partial, no frame read, nothing. Verified on 3.11 as well as 3.12, since the version was the whole point: both predicates on the class, on a transaction and on a tracking instance, and an autospecced double that gets awaited.
Rewriting the underlying function's signature to drop `self` was read again every time Python bound the method, taking the first remaining parameter off with it: `inspect.signature(client.cypher)` reported `params` first and no `query` at all, with tracking off as well as on. Anything inspecting a bound callable to validate arguments, inject dependencies or generate a wrapper would build that interface. The self-less signature exists for `create_autospec`, which reaches the method through the class, so it now lives on a separate object returned by class-level access only. The function every binding is derived from keeps its true signature. Carries a regression test on both bindings and on a transaction statement. Part of #78
Reading the call site when the method was looked up filed every query of a bound method kept for later under the one line that stored it: `run_query = client.cypher` followed by calls from a dozen places reported the assignment. Dependency injection and callback-style code hold on to a bound method routinely, so the advisor would merge exactly the call sites the feature exists to tell apart. The read moves into the call, which is the moment every path shares: `client.cypher(...)` evaluates in the caller's own frame whatever is then done with the coroutine, so scheduling it as a task still reports the caller and not the event loop. The coroutine contract holds because the wrapper derives from functools.partial, which the predicates unwrap. Carries a regression test for a saved method, and for two of its calls staying two distinct locations. Part of #78
create_text_index and drop_text_index reach the server through an internal cypher call, so the location read there was a frame inside this package: not a call site, discarded, and two public query paths left silently outside the per-query attribution the client advertises. Both helpers now read their own caller and pass it down to the statement, on the asynchronous and the synchronous client alike. Carries a regression test per helper on each client, and one holding the default path quiet. Part of #78
Reading `f_code` raises its own `object.__getattr__` audit event, apart from the `sys._getframe` one, so a hardened application permitting the first and rejecting the second had the exception come out of the read — past the guard, and into a query that then failed rather than going out unattributed. A debugging aid must never be why a query fails, whichever of the two events the hook objects to. The extraction moves inside the guard that already covers getting the frame, which is the same nothing to a caller either way. Carries a regression test per refused attribute, and one for the query that still goes out. Part of #78
The unbound form — passing the instance in, as AsyncCoordinodeClient.cypher(client, "...") — reaches the method through the class and so bypasses the binding entirely. Nothing on the way in read the caller, and a client with tracking on sent that query with no location at all: quiet, since an unattributed query is also what every unreadable frame produces. It now reads its caller as well. From inside the coroutine, which is as early as this form allows and right for the direct await it is written as; scheduling this particular form as a task lands on an event-loop frame and is left unattributed rather than misattributed, as every unreadable location is here. Carries a regression test for the attributed call and for the default path staying silent. Part of #78
Every package directory now carries the Apache-2.0 text and a NOTICE naming the copyright holder, so the built wheels include both; the README linked to a LICENSE file that was not there, and the published wheels contained no licence text at all. Contributions are accepted under a Contributor License Agreement that keeps the contributor's copyright and grants the holder the right to distribute the work under any terms. A CLA Assistant workflow collects signatures in the pull request and stores them on a dedicated branch. Package metadata names the maintainer; the README's support section keeps only the channel that exists.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (20)
✨ 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 |
💡 Codex Review Keep autospecced class patches callable
When tests use Treat call-site normalization failures as missing locations
When a query originates from dynamically compiled code with a relative Restrict the CLA allowlist to known accounts
The Pin the write-privileged CLA action to a commit
This ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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
dist-info/licenses/LICENSE,dist-info/licenses/NOTICE).CLA.md); a CLA Assistant workflow collects signatures in the pull request and stores them on thecla-signaturesbranch.Testing
uv build --wheelincoordinode/produces a wheel whosedist-info/licenses/contains LICENSE (11,358 bytes, canonical text) and NOTICE. The workflow file parses as YAML; its first real run happens on the next external pull request.Closes #90