feat(sdk): public client discovery, step-fetch, predicates, and testing module#174
Merged
Conversation
Lets callers pass a fully-custom Predicates instance to setup() instead of the hostname-substring shortcut. Mutually exclusive with llm_hosts — passing both raises ValueError so silent operator misconfiguration surfaces immediately. Re-exports Predicates and DefaultPredicates at the package root so consumers can write `predicates=DefaultPredicates()` without reaching into the private intercept package. Phase 0 commit 1 of the public-helpers SDK PR.
Adds a blessed module-level handle so wrapper libraries (e.g. the planned sf-rewind connector) can bind a recording client at app startup and have a module-level @cached_tool decorator find it at call time, without each consumer reinventing the module-global pattern. API: - rewind_agent.set_default_client(client) - rewind_agent.get_default_client() - rewind_agent.cached_tool(name=...) — module-level decorator that lazy-resolves the active client at call time, runs unrecorded when unbound (safe for import-before-bootstrap order) connector.setup() now binds the default client on entry and restores the previous value on exit (stack semantics, not full ContextVar isolation — accepted trade-off documented in the design plan; revisit if a real workload needs multi-sidecar in one process). set_default_client raises TypeError on non-ExplicitClient inputs to catch the common typo of passing a base-URL string. Phase 0 commit 2 of the public-helpers SDK PR.
Replay handlers today reach into private SDK helpers or hit raw HTTP to
fetch a single step's request/response content. This adds a typed
public helper so any agent (including the planned sf-rewind connector)
can pull step content without parsing untyped JSON blobs.
API:
- ExplicitClient.get_step(session_id, *, step_number, timeline_id=None)
- ExplicitClient.get_step_sync(session_id, *, step_number, timeline_id=None)
- StepResponse (frozen dataclass: step_number, step_type, request_body,
response_body, model, tool_name, raw)
- StepNotFoundError
Implementation hits the existing list endpoint
(/sessions/{id}/steps?timeline=...&include_blobs=1) and filters by
step_number — no new server route. When timeline_id is omitted,
resolves the session's root timeline; explicit timeline (forks)
passes through unchanged.
Phase 0 commit 3 of the public-helpers SDK PR.
Promotes test infrastructure from python/tests/ (private) to a public module so downstream packages — including the planned sf-rewind connector — can write integration-style tests without standing up a real server or vendoring their own stubs. Stable API (covered by SDK semver): - StubRewindServer — in-process stub of /api/sessions/* + record routes - make_dispatch_payload(...) — runner DispatchPayload with sane defaults - wait_for_session(server, name=..., timeout=...) — polling helper Stability boundary: - rewind_agent.testing: __all__ symbols follow normal SDK semver - rewind_agent.testing._unstable: explicitly NOT covered, may break in any 0.x release. Empty in v0.17 — exists as the boundary marker. Bump rewind-agent to 0.17.0 (Phase 0 release) since the public helpers in commits 1-4 of this branch constitute a minor API expansion. Phase 0 commit 4 of the public-helpers SDK PR.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Independent dual-review (Santa Method) on PR #174 surfaced six issues across two reviewers; this commit addresses the convergent + clear-cut findings. Critical (both reviewers agreed): - connector.setup() bound the default client BEFORE entering the try/finally that restored it. An install() or session().__enter__ exception left the module-global polluted across the failure, poisoning all subsequent cached_tool() calls. Wrap the entire body after the bind in an outer try/finally. Regression test added. Tighter API surface (parity with existing convention): - connector.setup(predicates=...) now isinstance-checks against the runtime_checkable Predicates Protocol and raises TypeError on non-conforming inputs — same boundary check as set_default_client(), catching the common typo of passing a callable or string. Documentation: - connector.setup() docstring notes that thread_id / metadata are ignored on the replay-dispatch path. - Module-level cached_tool() docstring notes the default-client binding is process-global (not per-thread / per-task). Test quality: - test_step_response_is_immutable: replace assertRaises(Exception) with FrozenInstanceError so the immutability assertion can't pass for the wrong reason. - test_explicit_timeline_id_passes_through: instrument the mock to record which paths were called and assert (a) /timelines was NOT hit, (b) the caller-supplied timeline_id reached the server URL. Add a counterpart test that locks the auto-resolve path.
…elity Independent dual-review round 2 surfaced six issues across two reviewers, all addressed below. 534 unit tests pass (+3 new since round 1). Error semantics: - get_step_sync used to raise StepNotFoundError on ANY failure because the underlying _get helper swallows exceptions to None. A network blip became indistinguishable from "step doesn't exist." Added a separate RewindServerError (extends RuntimeError) and a strict _get_or_raise() helper that propagates URLError / HTTPError / malformed-JSON / timeout as RewindServerError. StepNotFoundError now means exactly "step is genuinely absent." Replay handlers can now choose to retry transient infra failures. Threading: - set_default_client / get_default_client now serialize reads/writes via an internal threading.Lock — individual ops are atomic. - The stack-restore pattern in connector.setup() is still NOT atomic across concurrent setup() blocks (accepted trade-off), but the caveat is now documented adjacent to set_default_client's definition rather than 50 lines away in cached_tool. cached_tool perf + identity: - Module-level cached_tool used to call client.cached_tool(name)(func) on every invocation, rebuilding the inner wrapper per call. Now cached in a WeakKeyDictionary keyed on the client (so wrappers vanish with the client) with an inner dict keyed on id(func). Per- call cost is one dict lookup. New test locks the wrapper-stability contract so a future refactor can't lose the cache. StubRewindServer fidelity + hygiene: - step_number is now per-session (matches the real Rust server's contract) instead of a global counter. Tests authored against the stub no longer race across sessions. - __exit__ now joins the worker thread (bounded 2s timeout) so a test that asserts "port is free / serve_forever returned" right after exit doesn't race. Test quality: - session().__enter__ failure regression test added (round-1 commit message claimed coverage; only install() was actually mocked). - assertRaises(Exception) on the post-shutdown stub test narrowed to urllib.error.URLError specifically. - Unused asyncio import removed from test_testing_module.py. Santa Method round 2.
…docstring split Independent dual-review round 3: Reviewer B failed, Reviewer C passed. This commit addresses every blocker B flagged + the cleaner suggestions both reviewers agreed on. 537 tests pass (+3 since round 2). URL escaping (round-3 critical): - get_step_sync now URL-quotes BOTH session_id and timeline_id with safe="" so reserved characters don't break the URL or open a path- traversal-shaped surface. Round 2 quoted only timeline_id. - New test verifies the wire-format with a session id containing "/?#" — fails immediately if a future refactor drops the quote. Test integration (round-3 critical): - The "stable wrapper" test in round 2 exercised the resolver in isolation (would pass even if sync_wrapper rebuilt the inner wrapper per call). Replaced with a test that monkey-patches ExplicitClient.cached_tool to count invocations and asserts exactly 1 across 5 calls of the decorated function. Genuinely locks the perf claim from round 2's commit message. Error semantics docstring split: - get_step_sync's docstring lumped "session has no root timeline" under StepNotFoundError, but a 200-but-malformed timelines list (entries exist, none with parent_timeline_id=None) is server data inconsistency, not absence. Now: empty list → StepNotFoundError (true absence); non-empty-no-root → RewindServerError (server inconsistency, retryable). Two new tests lock the split. Cleanup: - Removed dead `if not self._enabled` branch in _get_or_raise (the attribute is set True in __init__ and never mutated). - connector.setup docstring now lists TypeError under "Raises" for the round-1 boundary check. - StubRewindServer's session-start dedup branch now releases the lock before writing the socket — matches the non-dedup branch and prevents slow-write blocking other handlers. - wait_for_session takes server._server.lock around the snapshot read for symmetry with the rest of the stub. Santa Method round 3.
CI's `ruff check .` failed on a leftover import from round-1 hardening: test_step_response_is_immutable imported StepResponse locally but the test only checks the package-level re-export via hasattr() against the already-imported `rewind_agent_module`. The local import was redundant.
risjai
enabled auto-merge (squash)
May 22, 2026 16:02
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.
What
Phase 0 of the public-helpers SDK work — public surfaces wrapper libraries
(e.g. the planned sf-rewind connector) need to integrate with Rewind without
reinventing private SDK patterns.
feat(connector): accept predicates= as alternative to llm_hosts=connector.setup(predicates=…)+Predicates/DefaultPredicatesre-exportfeat(explicit): public set/get_default_client + module-level cached_toolset_default_client,get_default_client, module-levelcached_tooldecorator (lazy resolution at call time, cached inner wrapper per(client, func))feat(explicit): public get_step / get_step_sync helperExplicitClient.get_step{,_sync},StepResponse(frozen dataclass),StepNotFoundError,RewindServerError(transport vs absence distinction)feat(testing): public rewind_agent.testing module + bump 0.17.0rewind_agent.testing.{StubRewindServer, make_dispatch_payload, wait_for_session}+_unstable/boundary; SDK bumped to 0.17.0Why
Replay handlers and connector wrappers reach into private SDK helpers
(
_HostPredicates,_fetch_step_at, contextvar internals) or reimplementthe module-global pattern in every consumer. Phase 0 closes those gaps with
a small, additive public API so:
Predicatesinstance toconnector.setup()without subclassing internal types.@cached_toollives at module level and lazy-resolves the active client.StepResponseand can distinguish "step absent" (
StepNotFoundError) from "transport /server failure" (
RewindServerError) for retry decisions.in-process stub the SDK uses, without vendoring it.
Design notes
fixup commits. Same author + same reviewer makes splitting cost more
than bundling. Each commit individually revertable.
_default_client: module attribute, not aContextVar. Reads/writesserialized by an internal
threading.Lock(atomic per-call). Thesave→bind→restore pattern in
connector.setup()is documented as NOTatomic across concurrent setup() blocks in different threads — accepted
trade-off; revisit if a real workload needs per-task multi-sidecar.
get_stepURL escaping: bothsession_idandtimeline_idareURL-quoted with
safe="". This new helper is built tighter than thesurrounding legacy code which interpolates IDs raw.
rewind_agent.testing: stable surface follows SDK semver in 0.x;rewind_agent.testing._unstable/is the explicit boundary marker forsymbols we don't want to lock in yet.
Santa Method review
Three independent dual-review rounds were run on this branch (two
reviewers per round, fresh agents each round, no shared context):
connector.setup()could leak thedefault-client binding if
install()raised. Fix wraps the entirebody in an outer try/finally. (Commit 5)
_getswallows network errors, so transient infrafailure was indistinguishable from "step doesn't exist." Added
RewindServerError+ strict_get_or_raisehelper. Tightenedthreading docs on
set_default_client. Made stubstep_numberper-session. Added
session().__enter__regression test. (Commit 6)session_idwas not URL-quoted (asymmetric withtimeline_id). Round-2's "stable wrapper" test passed for thewrong reason — replaced with one that counts
client.cached_toolinvocations. Split docstring's mixing of
StepNotFoundErrorandRewindServerError. Removed dead branch in_get_or_raise.(Commit 7)
Testing
537 unit tests pass (was 500 baseline, +37 across the 4 features +
3 review rounds).
Wheel builds clean;
rewind_agent-0.17.0-py3-none-any.whlships thenew
testing/subpackage.No Rust files changed — workspace Cargo version bump not required.
python -m pytest -q --ignore=tests/e2epasses (537 passed)python -m build --wheelsucceedscargo build --releasepasses (no Rust changes — N/A)Tested with
rewind demo && rewind show latest(no behavior change for end users)No new warnings from
cargo clippy(no Rust changes — N/A)