Connector seam: ABC + ScryfallConnector + handler rewiring + ContextVar resolution#176
Open
NandaScott wants to merge 6 commits into
Open
Connector seam: ABC + ScryfallConnector + handler rewiring + ContextVar resolution#176NandaScott wants to merge 6 commits into
NandaScott wants to merge 6 commits into
Conversation
…#170) - Add scrython/connector.py: Connector ABC with fetch() signature, plus set_default_connector(), use_connector() context manager, and get_connector() with resolution order: per-request kwarg > use_connector scope > set_default_connector > built-in ScryfallConnector default - Add scrython/connectors/scryfall_api.py: ScryfallConnector holds all urllib, User-Agent/Accept/Content-Type headers, and rate-limiter logic; exposes set_user_agent() classmethod - Rewire ScrythonRequestHandler: no direct urllib calls; _fetch_raw() parses the URL and delegates to the resolved connector; caching stays in the handler - Remove _user_agent, _accept, _content_type, _rate_limiter_class, and _override_limiter from ScrythonRequestHandler; remove set_user_agent() classmethod and rate_limit_per_second= kwarg - Remove _rate_limiter_class overrides from Search, Named, Random, Collection - Re-export ScryfallConnector, set_default_connector, use_connector, get_connector at scrython package level - Update tests and fixtures to patch scrython.connectors.scryfall_api.urlopen and scrython.connectors.scryfall_api.RateLimiter; remove tests for removed features (_rate_limiter_class, rate_limit_per_second= kwarg) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 tasks
NandaScott
added a commit
that referenced
this pull request
May 30, 2026
The prompt gates omitted black, but CI runs black --check first and fails the whole run before pytest. #176 opened red for exactly this. Gates now mirror tests.yml: black, ruff check scrython tests, mypy, pytest.
NandaScott
added a commit
that referenced
this pull request
May 30, 2026
* Make sandcastle a blocker-aware issue orchestrator I replaced the simple-loop runner with a custom orchestrator modeled on the tubarr setup, adapted to Scrython. A run now lists open `ready-for-agent` issues, fans out one sandboxed agent per issue, and on the host pushes each branch, opens a PR, and relabels the issue to `needs-review`. The agent commits only; it no longer closes issues or pushes, which was closing them before the work was reviewed. The orchestrator reads the `## Blocked by` section of each issue and only dispatches an issue once every blocker it names is cleared. A blocker counts as cleared when its issue is closed or when the target branch already carries a commit referencing it (e.g. `(#170)`). That commit check is what lets PRD branches work, since merging a slice into a `prd/<slug>` branch lands the commit but does not auto-close the issue. Issues with a milestone target their `prd/<slug>` branch (created from develop on first use); everything else targets develop. DRY_RUN=1 prints the dispatch plan and the blocked list without spawning anything, and ONLY_ISSUE=N restricts a run to a single issue for smoke tests. The prompt is now a per-issue template with ruff, mypy, and pytest gates. * Add black to agent gates to match CI The prompt gates omitted black, but CI runs black --check first and fails the whole run before pytest. #176 opened red for exactly this. Gates now mirror tests.yml: black, ruff check scrython tests, mypy, pytest. * Gate PR creation on agent completion signal run() returns RunResult.completionSignal: the matched signal, or undefined if the agent never signaled before the iteration limit. Open a PR only when the agent both committed and signaled completion; otherwise leave the branch for review instead of opening a red PR on partial work. * Document Sandcastle in the contributing guide Adds a section covering the ready-for-agent to needs-review label flow, how a run works (blocker-aware dispatch, milestone PRD branches, CI-matching gates), one-time setup, and the run commands, so agent-opened PRs and the label flow are not a mystery to contributors.
NandaScott
commented
Jun 1, 2026
The connector seam refactor flattened all endpoints to a single 10/s limiter, dropping the 2/s tier Scryfall enforces on search, named, random, and collection. Move the tiering into ScryfallConnector, which is the only component that should know Scryfall's transport rules: a custom connector (cache, alternate backend) has nothing to throttle, so the Connector ABC stays minimal. - ScryfallConnector._SLOW_ENDPOINTS classifies the resolved endpoint; slow endpoints draw from the shared SlowRateLimiter bucket, the rest from the default RateLimiter bucket. - An injected limiter still flat-overrides tiering for every endpoint. - Add RateLimitWarning, emitted per-request when the active limiter exceeds the endpoint's tier limit (risk of throttle/ban). The default tiered path never trips it; NullRateLimiter is a sanctioned opt-out and is exempt. - Add NullRateLimiter for callers who explicitly want no throttling. - Export RateLimitWarning and NullRateLimiter; patch both tiers in the disable_rate_limiting fixture; add a drift guard asserting every _SLOW_ENDPOINTS string maps to a real cards.py endpoint.
After the connector refactor, rate_limit= was stripped from query params but never forwarded, so rate_limit=False silently kept throttling on. Skipping throttle is a transport concern, so it does not belong on the request handler or the Connector ABC. Drop the toggle and warn (DeprecationWarning) when it is passed, so the removal is never silent. Callers disable throttling explicitly by injecting ScryfallConnector(rate_limiter=NullRateLimiter()). Drop the dead rate_limit=True kwarg from the caching test.
Per review: connector resolution read as loose exported functions. Make Connector the source of truth — set_default(), use(), and current() are classmethods over the type's own ContextVar and default state. Module-level set_default_connector/use_connector/get_connector become thin aliases so the documented scrython.set_default_connector(...) call site is unchanged. Resolve through Connector.current() in the handler. Add resolution tests (kwarg > scope > default > builtin) the original slice never had.
CI ruff caught issues the local run missed: unused method args in the test DummyConnector.fetch (noqa ARG002, the names mirror the ABC), an unsorted inline import block, and a Yoda subset comparison.
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.
Closes #170
Opened by Sandcastle. 1 commit(s) on
sandcastle/issue-170.