Add check() and the dashboard API client, and re-measure the gateway - #2
Merged
Conversation
This is one commit because it does not split into ones that build: the README is tested by tests/test_readme.py, and the same probe runs that produced api.py also moved values in the provider definition that proxy.py and errors.py read. The library stopped being a pure string builder. Two network-touching halves arrive: - check() opens one CONNECT, by name and never on construction, and reports what the gateway said about it. - Client wraps the dashboard account API against the vendor's OpenAPI 3.0.3 document, fetched 2026-09-09, 24 paths. The specification is documentation and gets no more authority than a vendor's client did. Every path here was called from a real account on 2026-09-08 and 2026-09-09, and the document is wrong in four measured places, all recorded in the docstring of the method affected: - statistics dates are dd-mm-yyyy, while `format: date` in the type is a string the server cannot parse - three paging conventions, not one, and both page-numbered endpoints are 1-based - sub-users/ ends a walk with an empty payload, whitelist/ips with 404 - whitelist/ip/upsert refuses the two fields its own schema marks required; protocol is what it lacks, and the server does not apply the default: HTTP it declares, so this client sends it The gateway's status codes were re-measured by raw CONNECT and most of the table moved. A bad country is 407 and not the 406 four artifacts had carried for four weeks, which is the case for client-side validation rather than against it. Nothing is released. __version__ stays 0.1.2 and the CHANGELOG entry is under Unreleased, so the PyPI page is unchanged until a version bump.
Step 1 of the release procedure and nothing more: no tag, no GitHub Release, so `publish.yml` has not fired and PyPI still serves 0.1.2. The changelog heading carries no time for that reason - every other heading in the file is a PyPI upload record and there is nothing to record yet. The bump is worth making before the release rather than with it. Committed `main` and published 0.1.2 have been the same library since 0.1.2 shipped; this branch is not, it adds `check()` and the dashboard API client, and while both trees answered `0.1.2` there was no way to tell from the outside which one a bug report was against. That is the exact failure this repository hit with 0.1.1, logged in CHANGELOG.md.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_api.py">
<violation number="1" location="tests/test_api.py:1161">
P3: This source-text assertion does not establish that the package avoids third-party HTTP dependencies. Replace the substring scan with a runtime/import-metadata check that fails for alternate import forms as well.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| source = module.__file__ | ||
| with open(source, encoding="utf-8") as handle: | ||
| text = handle.read() | ||
| assert "import requests" not in text |
Contributor
There was a problem hiding this comment.
P3: This source-text assertion does not establish that the package avoids third-party HTTP dependencies. Replace the substring scan with a runtime/import-metadata check that fails for alternate import forms as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_api.py, line 1161:
<comment>This source-text assertion does not establish that the package avoids third-party HTTP dependencies. Replace the substring scan with a runtime/import-metadata check that fails for alternate import forms as well.</comment>
<file context>
@@ -0,0 +1,1180 @@
+ source = module.__file__
+ with open(source, encoding="utf-8") as handle:
+ text = handle.read()
+ assert "import requests" not in text
+ assert "import httpx" not in text
+
</file context>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/nodemaven/api.py">
<violation number="1" location="src/nodemaven/api.py:1606">
P2: This change makes the existing pagination regression test fail: a short non-empty page advances the offset by its returned length instead of the requested limit. Update the test and documented contract to expect the new cap-aware offset, or implement a separate rule that preserves fixed-size offsets without skipping capped rows.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| return None | ||
| if size <= 0: | ||
| return None | ||
| following[paging.cursor_key] = cursor + len(page.results) |
Contributor
There was a problem hiding this comment.
P2: This change makes the existing pagination regression test fail: a short non-empty page advances the offset by its returned length instead of the requested limit. Update the test and documented contract to expect the new cap-aware offset, or implement a separate rule that preserves fixed-size offsets without skipping capped rows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/nodemaven/api.py, line 1606:
<comment>This change makes the existing pagination regression test fail: a short non-empty page advances the offset by its returned length instead of the requested limit. Update the test and documented contract to expect the new cap-aware offset, or implement a separate rule that preserves fixed-size offsets without skipping capped rows.</comment>
<file context>
@@ -1603,7 +1603,7 @@ def _next_step(page: Page) -> Optional[Tuple[str, Dict[str, Any]]]:
if size <= 0:
return None
- following[paging.cursor_key] = cursor + size
+ following[paging.cursor_key] = cursor + len(page.results)
else:
following[paging.cursor_key] = cursor + 1
</file context>
The five suggestions applied through the web interface were each half of a fix. This is the other half, plus what the suite should have caught. iterate() advanced the offset by the limit asked for instead of by the rows returned, so cities(limit=10000) walked past the end of a collection this server caps at 1000 rows of 1965. Three tests covered that case and none could fail: two asserted the buggy offsets, one of them saying so in its own name, and the third counted rows only, which the fake transport cannot distinguish because it replays a queued list whatever query string it gets. All three now assert the offset sequence. The 429 message told callers to read retry_after, which this client can never fill - Transport returns (status, bytes) and the headers are gone before anything sees Retry-After. The message and the class docstring now say so; the attribute stays, because it belongs to the class rather than to this transport. The README's statistics examples still sent ISO dates while the prose two paragraphs below them had already been corrected against --phase 10. A test reads the dates out of the fenced blocks now. A provider port is validated at load rather than coerced with int(), which accepted 0. A bad port from the definition no longer raises a message blaming a port= the caller did not pass. normalize and connect_reactions are type-checked, so normalize = "city" stops being iterated character by character. A values list is folded when its parameter is normalized, through one _fold() called from both sites instead of two copies of three steps. test_check.py caught Exception in four places where it meant ParamError, and _headings() in test_readme.py read every code comment as a heading, so the two anchor tests were checking a superset of their subject. 341 tests green. Two of the review's twelve findings are declined and the CHANGELOG says why: carrying repeated headers breaks Check.headers as a dict for no measured reason, and the proposed import-scan test was too vague to pin anything.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The review's one remaining finding, and it is a defect the previous commit
introduced. Correcting the ISO dates in those examples left the keyword
names alone: this server's filters are start and end, confirmed against the
vendor specification's parameter list and against --phase 10, which got 200
from start=20-08-2026.
Client forwards **filters as query parameters unaltered and this server
ignores a query parameter it does not know, so start_date= was accepted by
Python, sent, and dropped - and with the range then absent the call answers
500 about the missing range rather than about the name. That is the norotate
failure mode one layer up: Proxy refuses an unknown gateway parameter
because the gateway answers 200 and drops it, and Client forwards an unknown
API parameter in silence for the same server behaviour. Validating **filters
needs a per-endpoint list of legal names and is not done here.
Three tests pin the examples instead: the filter names, the date format, and
that no example omits start, end and period together - domain_statistics
("acct-1") did, and all three statistics endpoints answer 500 to that. The
docstrings on statistics_requests and domain_statistics now repeat the rule
rather than pointing at statistics_data, because the one carrying it was not
the one anybody read.
The date test from the previous commit would not have caught this and would
have gone quiet on the fix: it scanned for start_date=, written from the
examples rather than from the API, so correcting the names would have left
it matching nothing and passing on an empty list. It survives on its assert
dated guard.
343 tests green.
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
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.
Two calls reach the network now, and neither runs on import.
Proxystillbuilds a string and opens nothing; that half of the package stays testable with
no socket and no account, which is why the new code sits on separate objects.
proxy.check(), andCheck,CheckError. One CONNECT to the gateway,the reply handed back whole: status, the reason phrase verbatim, every header,
the elapsed seconds, the exit address when the gateway sends one. A refusal is
a return value - a 407 is the gateway answering the question - and
CheckErroris raised only when nothing came back at all.
Client,Page, and the dashboard account API. 19 of the vendor's 24paths. The five that are not wrapped are listed in the changelog rather than
left silent.
Where this differs from the vendor specification, and why
The client was written against the vendor's OpenAPI 3.0.3 document and then the
document was checked against the server. It is wrong in four places that a
generated client cannot survive, all measured 2026-09-09 from the account
owner's own connection:
dd-mm-yyyyand the type saysformat: date.20-08-2026answers 200 with 21 data points;2026-08-20answers 400 with a body byte-identical to
not-a-date. So the machine-readablehalf is not a rival spelling the server declines, it is a string the server
cannot parse, and every generated client sends the one form that fails.
limit/offset,page/per_page,page/page_size. An unknown query parameter is ignored, so a hard-codedpair silently does nothing on two thirds of the surface. Both page-numbered
endpoints are 1-based, measured, and they mark the end of a collection
differently -
sub-users/answers an empty payload,whitelist/ipsanswers404.
whitelist/ip/upsertneedsprotocol, which its own schema marksoptional with
default: HTTP. The server does not apply that default: thebody without it is refused 400
Please enter a valid protocol(HTTP or SOCKS5)., the same body with it is accepted 201. Soupsert_whitelist_ip()sends
protocolon every call - a client-side compensation for a server-sidedefect, not a convenience.
zip_codes,statisticsandwhitelist_ipsanswer 200 with the front end's HTML, byte-identical to adeliberately nonexistent path;
update_sub_useris a PUT to the collectionand not a PATCH.
That last point is why every probe behind this branch compares a response digest
against a negative control: this host answers an unrouted path with 200 and
6415 bytes of SPA HTML, so calling a path cannot tell you whether the path
exists, and a status code is not evidence.
Each of these is written into the docstring of the method it constrains, with
its date, so the next reader gets the measurement and not the conclusion.
Two things a reviewer should push on
iterate()stops on an empty page, not a short one. The server capscitiesat 1000 rows of 1965, so short read as the end.proof the object is gone on this API. That pair was the strongest check
available and it returned a wrong answer once: an address deleted at 15:13 and
absent from the listing a second later was refused at 15:18 as already
whitelisted. It was settled at 16:18 - the same address was accepted 201, so
the delete is real and the uniqueness check lags it by somewhere between 5 and
62 minutes. The bound is loose and nothing here narrows it.
Version
__version__is 0.1.3. There is no tag and no Release, so nothing is uploadedand PyPI still serves 0.1.2. The bump is in this branch rather than in the
release commit because both trees answering
0.1.2is exactly how 0.1.1 turnedinto two different libraries with one version number.
Verification
330 tests, all green, no network in any of them. The live measurements above
were made by a probe outside this repository and are quoted with their dates.
Summary by cubic
Adds two explicit network operations:
Proxy.check()performs one CONNECT and returns the gateway's response, whileClientwraps 19 of the vendor's 24 dashboard API paths. These are the package's only socket-touching calls, and neither runs on import; existingProxyconstruction stays network-free. Callers need separate gateway credentials and API credentials for these operations.API and gateway changes
startandend; the server silently drops unknown query parameters, so the previousstart_date/end_datespelling was accepted, dropped, and then answered 500 for the missing range.iterate()to advance by rows returned rather than the limit requested; provider ports are validated at load, andnormalize/connect_reactionsare type-checked.__version__to 0.1.3; no release or migration is required yet, and PyPI still serves 0.1.2.Verification
Written for commit 1ce201b. Summary will update on new commits.