feat(hdk): one-call connector.setup() + three-tier integration guide#171
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Today every new agent/framework that wants to use Rewind reinvents the HTTP-emitter wheel — see the wider AI infra ecosystem where private LLM gateways and bespoke transports each end up with hundreds of LOC of custom session/event-emitter code. The SDK already ships the parts: intercept.install() for HTTP transports, ExplicitClient for custom transports, and contextvar-aware sessions to tie them together. This adds docs/hdk.md, a three-tier decision guide that routes integrators to the simplest API that works (5 LOC for HTTP, ~30 LOC for custom transport, ~50 LOC for org-wide connectors), with worked examples and the common "session not active so recording silently no-ops" pitfall called out. Cross-links from framework-integrations.md, runners.md, and the README's feature/compatibility tables. No code changes; no version bump required. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
c5b8fdc to
8c9a6d5
Compare
…t in one block Adds rewind_agent.connector.setup(name=...): a context manager that composes ExplicitClient.session() + intercept.install() in the right order so callers don't have to remember the dance. Eliminates the silent-no-op trap where intercept records nothing because no session is active — the most common Tier-1 integration mistake. Behavior - Wraps the with-block in a fresh session, sets _session_id and _timeline_id contextvars, installs HTTP intercept, yields the ExplicitClient for non-HTTP record_* paths inside the block, and cleans up on exit. - Honors REWIND_ENABLED=0 as a true zero-overhead kill switch (yields None, no install, no HTTP). - Honors REWIND_URL (default 127.0.0.1:4800) and REWIND_LLM_HOSTS (comma-separated) for env-based config; kwargs override env. - When REWIND_SESSION_ID and REWIND_REPLAY_CONTEXT_ID are set (runner-subprocess pattern from docs/runners.md), skips creating a fresh session and lets intercept.install() attach to the existing replay context — no phantom sessions during runner-driven replays. - Idempotent intercept install: doesn't uninstall on exit if the caller already had it installed at entry. Implementation - python/rewind_agent/connector.py — ~150 LOC including docstrings. - python/rewind_agent/__init__.py — re-exports the connector module in __all__ and updates the package docstring. - python/tests/test_connector.py — 12 tests covering kill switch (env + kwarg), session lifecycle, replay-dispatch detection, host predicates from env / kwarg / defaults, idempotent install, and public export. Version - python/pyproject.toml + __init__.py bumped 0.16.0 → 0.16.1 per CLAUDE.md Track 2 (python/ files changed; 0.16.0 is on PyPI). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The connector lands in this same PR (commit 21a3f00). Update the integration docs to recommend it as the one-liner for any agent on httpx/requests/aiohttp, and demote the manual ExplicitClient.session + intercept.install dance to a "finer control" subsection for the long-running-process / many-short-sessions case. - docs/hdk.md Tier 1 now leads with `with rewind_agent.connector.setup(name=...)` and documents REWIND_ENABLED / REWIND_URL / REWIND_LLM_HOSTS knobs. The manual two-step pattern still appears (under "When you need finer control") with the silent-no-op pitfall flagged. - docs/framework-integrations.md "Bring your own framework" section shows the one-liner. - README's Feature Guides + Agent Frameworks tables updated to match. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Found while smoke-testing the new connector against a rewind server on a non-default port: ExplicitClient hard-coded http://127.0.0.1:4800 and ignored \$REWIND_URL. The intercept layer's lazy singleton at intercept/_flow.py:130-135 calls ExplicitClient() with no args, so when the server runs on a different port intercept silently records to nowhere — the rewind server returns 404 on the wrong port and _post swallows the exception in DEBUG. Resolution order is now: explicit kwarg > \$REWIND_URL > localhost default. Matches the convention intercept._install._bootstrap_replay_ context_from_env already uses for the runner-subprocess path. The connector module passes base_url=... explicitly via its setup() kwarg + env-var path, so its behavior is unchanged. The fix benefits every direct ExplicitClient user and the intercept singleton. Tests: 3 new cases in test_explicit.py covering default, env-var fallback, and kwarg-wins precedence. Full python suite: 491 passed (488 previously + 3 new), 1 skipped, 0 failed. Smoke: started rewind on port 4802, set REWIND_URL=...:4802, ran the connector + httpx.post(api.openai.com) — session on 4802 shows total_steps=1 with the recorded step. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
risjai
left a comment
There was a problem hiding this comment.
Code Review
Solid PR. The connector closes a real footgun (silent no-op when intercept is installed without an active session), the docs are well-organized, and the tests cover the interesting branches. Twelve new tests is appropriate for the surface area. A few correctness/quality items worth addressing before merge — none are blockers.
Correctness
1. _resolve_hosts() no longer falls back to the strict default list when REWIND_LLM_HOSTS="". (connector.py:78-82)
When llm_hosts kwarg is None and the env var is empty/unset, _resolve_hosts returns (), so predicates is None and intercept.install() falls through to DefaultPredicates. That's the intended behavior, and it works — but the inline comment in test_no_hosts_uses_default_predicates is the only place this is documented. Worth adding a one-line docstring note on _resolve_hosts ("empty tuple => use intercept's DefaultPredicates"); otherwise a future reader might add a "fix" that wraps DefaultPredicates here unnecessarily.
2. _is_replay_dispatch() and the REWIND_REPLAY_CONTEXT_TIMELINE_ID mismatch. (connector.py:96-107)
_install._bootstrap_replay_context_from_env treats REWIND_REPLAY_CONTEXT_TIMELINE_ID as effectively required — without it, live cache misses don't have a defined recording target and the bootstrap function logs a WARN. But connector.setup() only checks REWIND_SESSION_ID + REWIND_REPLAY_CONTEXT_ID to detect replay mode. If a caller sets only those two (no timeline), setup() skips session creation and hands off to install(), which then warns that timelines won't bind correctly — and the caller silently gets a half-broken replay.
Two options:
- Treat all three as required for
_is_replay_dispatch()to returnTrue, mirroring the bootstrap function's contract. - Document explicitly in
setup()'s docstring that all three env vars must be set together for replay mode (and the test at test_connector.py:127-145 sets all three already, which suggests the timeline var was always intended).
3. ExplicitClient env var resolution duplicates intercept._install's logic. (explicit.py:102-108)
The new __init__ reads REWIND_URL itself, and _bootstrap_replay_context_from_env does the same. Now connector.setup also reads it (connector.py:150) before passing the resolved URL down. That's three call sites with identical default strings. If the default port ever changes (or someone adds https:// defaults for SaaS), all three need to be updated in lockstep. Consider either:
- Making
connector.setuppassbase_urlthrough unchanged and letExplicitClient.__init__do the resolution, or - Extracting a small
_resolve_base_url(base_url: str | None) -> strhelper and using it everywhere.
The current code works, but the redundancy is the kind of thing that drifts.
Test quality
4. test_partial_replay_env_does_not_trigger_replay_path is hard to follow. (test_connector.py:147-155)
The two mock.patch.dict contexts plus the os.environ.pop() inside is confusing on first read. A simpler version:
def test_partial_replay_env_does_not_trigger_replay_path(self):
env = {k: v for k, v in os.environ.items() if k not in ("REWIND_REPLAY_CONTEXT_ID",)}
env["REWIND_SESSION_ID"] = "s-only"
with mock.patch.dict(os.environ, env, clear=True):
self.assertFalse(_is_replay_dispatch())Also worth adding the inverse case (only REWIND_REPLAY_CONTEXT_ID set, no REWIND_SESSION_ID) for symmetry.
5. _HostPredicates._hosts is accessed directly in tests. (test_connector.py:171, 192)
Asserting on a private attribute (preds._hosts) couples the test to the implementation. The functional test at line 197 (test_predicate_matches_substring) is the better pattern — call is_llm_call() and verify behavior. Consider replacing the _hosts assertion with a behavioral one ("calling is_llm_call on a request whose netloc contains a.example returns True; same for b.example and c.example; whitespace-only entries do NOT match").
Style / minor
6. Iterable[str] | None then tuple(llm_hosts) consumes the iterable. (connector.py:79-82)
If a caller passes a generator (legal for Iterable), _resolve_hosts consumes it before _HostPredicates.__init__ sees it — fine for the current single-use call site, but Iterable[str] is broader than the function actually needs. Sequence[str] | None more accurately describes the contract, and matches what the docstring example shows (("llm-gateway.corp.example",)).
7. enabled=None triple state in setup(). (connector.py:116)
enabled: bool | None with None meaning "consult env" is fine but worth documenting on the parameter itself ("None (default) reads $REWIND_ENABLED; False forces off; True forces on"). The current docstring just says "Override the $REWIND_ENABLED kill switch" which doesn't make the tri-state behavior obvious.
8. Docstring example for non-HTTP transport in connector.py:35-38.
client.record_llm_call(req, resp.dict(), model="...", duration_ms=...)record_llm_call is a positional/keyword-mixed call here. Looking at explicit.py:279, the signature is keyword-only after request/response. The example will work because request and response are positional in the signature, but it's worth mirroring the keyword-style of the more thorough example in docs/hdk.md:103-108 for consistency.
Docs
- docs/hdk.md — the three-tier framing is great. The "Common pitfalls" section is the right place for the silent-no-op warning. Consider adding one more pitfall: "Calling
setup()withoutawaitin async code" —@contextmanager(sync) is what's exported, not@asynccontextmanager. An async caller usingasync with rewind_agent.connector.setup(...)will get aTypeErrorat runtime. Either document the sync-only nature or ship an async variant. - The hdk.md Tier 3 example uses
http://localhost:4800while connector.py useshttp://127.0.0.1:4800— worth picking one for consistency.
Security
No new secrets / network surfaces beyond the existing ExplicitClient HTTP path. The kill switch (REWIND_ENABLED=0) is appropriately strict (yields None, no HTTP, no install) — that's the right default for prod.
Verdict
Approve once items 2 (replay timeline mismatch) and 5 (private-attribute test coupling) are addressed. Items 1, 3, 4, 6, 7, 8 are nice-to-haves and can land in a follow-up.
…vioral tests Addresses 8 items from PR review on #171. Correctness - _is_replay_dispatch() now requires all three replay env vars (REWIND_SESSION_ID + REWIND_REPLAY_CONTEXT_ID + REWIND_REPLAY_CONTEXT_TIMELINE_ID). Previously a partial setup (no timeline) would skip session creation and hand off to intercept.install(), which warns about an undefined recording target — a half-broken replay. Now the connector falls through to normal session-start in that case. - Drop the now-duplicated REWIND_URL resolution from connector.setup(). After the prior fix made ExplicitClient honor REWIND_URL, the connector forwarding it explicitly was a third drift-prone copy. Single source of truth lives in ExplicitClient.__init__. Test quality - Replace private-attribute assertions on _HostPredicates._hosts with behavioral assertions on is_llm_call(). - Simplify test_partial_replay_env... using a clean env builder helper. Add a parameterized test that covers each replay env var individually missing, plus a sanity test that the connector actually starts a fresh session when replay env is incomplete. - Add coverage that the predicate chain falls through to DefaultPredicates' provider list (api.openai.com etc.) when a custom host doesn't match. Style / docs - Type hints: Iterable[str] → Sequence[str] (matches actual contract). - Docstring tri-state for enabled parameter (None reads env, True/ False explicit override). - Module-docstring record_llm_call example uses keyword args consistently with the bigger example in docs/hdk.md. - _resolve_hosts docstring spells out: empty tuple => caller MUST pass None to install() so DefaultPredicates is used. - docs/hdk.md uses 127.0.0.1 consistently (was localhost in two places). - New "Common pitfall" entry: connector.setup() is sync — async callers use plain `with`, not `async with`, even inside async def. Tests: 492 passed, 1 skipped (was 491 + 1). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Thanks for the careful review @risjai — addressed all 8 items in 382d977. Quick rundown: Approve-conditions
Other items
|
risjai
left a comment
There was a problem hiding this comment.
Re-review of commit 382d977
The fixes are well-structured and the test refactor is a clear improvement (subtests across _ALL_REPLAY_VARS, behavioral assertions on is_llm_call, single source of truth for REWIND_URL). Nice work on items 3, 4, 5, 6, 7, and the new "Common pitfall" sync-vs-async note.
One issue from the original "replay-mode contract" fix needs another look, plus a few small follow-ups.
Correctness — issue introduced by the fix to item 2
The partial-replay fall-through clobbers the freshly-started session via the intercept bootstrap. (connector.py:191-203, intercept/_install.py:131-185)
The new _is_replay_dispatch() requires all three env vars. Good. But when only two are set (e.g. operator forgot REWIND_REPLAY_CONTEXT_TIMELINE_ID), the connector now falls through to:
client.session(name)→ creates a fresh session on the server, sets_session_id/_timeline_idcontextvars.install(predicates=...)→ which still calls_bootstrap_replay_context_from_env()(_install.py:122)._bootstrap_replay_context_from_envonly requiresREWIND_SESSION_ID + REWIND_REPLAY_CONTEXT_ID(_install.py:154) — timeline is treated as optional with a WARN log.- It calls
client.attach_replay_context(...)which_session_id.set(env_session_id)/_replay_context_id.set(env_ctx_id)(explicit.py:626-629) — overwriting the freshly-started session's contextvars.
Net result: a fresh session IS created on the server (orphaned) AND subsequent recorded calls go to the env-provided replay context with no defined timeline. This contradicts the new docstring at connector.py:65-72 ("falls through to the normal session-start path so recording still works correctly") — recording does NOT go to the new session.
The current test test_partial_replay_env_falls_through_to_session_start only asserts /api/sessions/start was hit — it doesn't assert that _session_id.get() matches the new session after install() returns. Add that assertion and this regression should surface.
Two ways to fix:
- Option A (recommended): Tighten the connector's contract — when partial replay env is detected, raise or log + unset the offending vars before calling
install(). This fails fast on misconfiguration (the intent of the original review item). - Option B: Tighten
_install._bootstrap_replay_context_from_envto also requireREWIND_REPLAY_CONTEXT_TIMELINE_IDso its contract matches the connector's. This is the more durable fix because it aligns the two consumers of these env vars on the same contract; the WARN-log path becomes unreachable.
I'd lean toward Option B since it removes the contract divergence at the source.
Test follow-up
Assert contextvar state after install() in the partial-replay fall-through test. (test_connector.py:176-184)
Inside the with setup(...) block, assert _session_id.get() equals the freshly-allocated session id (e.g. "s-1" from the mock handler), not the env-supplied "s-replay". That's the assertion that would have caught the bootstrap-clobber bug above.
def test_partial_replay_env_falls_through_to_session_start(self):
env = self._replay_env(REWIND_REPLAY_CONTEXT_TIMELINE_ID=None)
with mock.patch.dict(os.environ, env, clear=True):
with setup(name="incomplete-replay", base_url=self.base_url):
# The fresh session's contextvars must be live, not the
# env-supplied (incomplete) replay context's.
self.assertEqual(_session_id.get(), "s-1")
self.assertIsNone(_replay_context_id.get())
self.assertEqual(len(_MockHandler.sessions_started), 1)Minor
Doc inconsistency. docs/hdk.md Tier 3 example at line 165 now uses 127.0.0.1, but the Tier 1 callout block earlier on the page still has examples that don't reference any URL — that's fine. Just confirming the only localhost references left are in narrative text, not example code. (Looks correct to me on reread.)
_is_replay_dispatch() truthiness check. (connector.py:128-132)
bool(a and b and c) returns False if any of the three env values is the empty string. That's the intended behavior, but worth a one-line comment so a future reader doesn't "fix" it to is not None.
Verdict
Approve once the partial-replay clobber is addressed (Option B preferred) and the test asserts contextvar state. Test-only changes if Option A is chosen; one-line change in _install.py if Option B. Everything else from round 1 looks resolved.
|
Thanks @risjai — you caught a real clobber bug in my round-1 fix. Addressed in 985a5f0 with Option B (your recommendation): Root cause (per your trace):
Fix: Tests:
Minor: added a comment on the
|
risjai
left a comment
There was a problem hiding this comment.
Round 3 review of 985a5f0 — LGTM
Option B applied as suggested. The contract is now consistent across all consumers.
Verified:
_bootstrap_replay_context_from_envusesset_count = sum(bool(v) for ...)and only attaches onset_count == 3. Partial subsets log one WARN and skip — no half-attach. (_install.py:155-186)- The connector's partial-fall-through test now has the assertions that would have caught my round-2 regression:
_session_id.get() == "s-1",_timeline_id.get() == "tl-1",_replay_context_id.get() is Noneinside thewithblock. (test_connector.py:189-195) TestEnvVarBootstrapContract.test_partial_subsets_skip_attachparametrically covers all six partial subsets (3 single-var + 3 two-var) plus the all-three and none-set cases. Strong coverage at the source.test_runner.pyupdated: the previously namedtest_install_bootstraps_from_env(which asserted the old "two-vars attach" behavior) is nowtest_install_bootstraps_only_when_all_three_env_vars_set, asserting the new contract. The existing all-three-attach path is still covered bytest_install_bootstraps_with_timeline_id_from_env.
One nit, non-blocking:
test_install_bootstraps_with_timeline_id_from_env in test_runner.py and TestEnvVarBootstrapContract.test_all_three_set_attaches in test_intercept_install.py now both cover the same code path (all-three → attach succeeds). Slight redundancy. If you want to consolidate, the contract test in test_intercept_install.py is the better home; the runner test could shrink to a one-liner subprocess sanity check. Not worth blocking on.
Approving. The HDK landing slice — connector + docs + version bump — is good to merge once CI passes. Nice round-trip on the review feedback.
…d-2 review)
Reviewer caught a clobber bug introduced by the round-1 fix: with two
of three replay env vars set, the connector would (correctly) fall
through to start a fresh session, then intercept.install() would run
_bootstrap_replay_context_from_env on the still-incomplete env, attach
to the env-supplied replay context, and OVERWRITE the freshly-started
session's contextvars. Recordings then went to the env-supplied
session (orphaning the new one) on an undefined timeline.
Tightening at the source: _bootstrap_replay_context_from_env now
requires REWIND_SESSION_ID + REWIND_REPLAY_CONTEXT_ID +
REWIND_REPLAY_CONTEXT_TIMELINE_ID together, or it skips the attach
entirely with one WARN. Aligns the contract with connector.setup() —
both consumers now agree "all three or nothing."
Why Option B over Option A (per the reviewer's recommendation):
- Single source of truth removes the contract divergence at the source.
- Fixes the bug for direct intercept.install() callers too, not just
the connector. Anyone using the manual session+install pattern
documented in docs/hdk.md gets the same protection.
- The documented runner-subprocess path (docs/runners.md:139-145)
always sets all three from the dispatch payload — no in-tree
consumer is affected. The previous WARN-and-attach branch was a
half-broken state, not a useful one.
Tests:
- New TestEnvVarBootstrapContract in test_intercept_install.py:
- all-three-set attaches; none-set is silent no-op; every partial
subset (3 single-var + 3 pair = 6 subtests) skips the attach and
leaves contextvars unset.
- test_connector.test_partial_replay_env_falls_through_to_session_start
now asserts the contextvar state inside the with-block: _session_id
matches the freshly-created mock session ("s-1"), _timeline_id is
the new timeline, _replay_context_id stays None. This is the
assertion that would have caught my original regression.
- test_runner.py: two existing tests updated for the new contract
(the previous "two-vars-set attaches" test is replaced with a
"two-vars-set does NOT attach" sanity check).
Full suite: 495 passed, 1 skipped, 9 subtests passed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
985a5f0 to
6fe8579
Compare
Summary
This PR ships the full HDK landing slice — code + docs in one PR. Combines what was previously split across PR 171 (docs) and #172 (code).
rewind_agent.connector.setup(name=...): a context manager that composesExplicitClient.session()+intercept.install()in the right order. Eliminates the silent-no-op trap where intercept records nothing because no session is active.docs/hdk.md: three-tier decision guide routing integrators toconnector.setup()(HTTP, one-liner),ExplicitClient(custom transport, ~30 LOC), or a small connector wrapper (org-wide defaults).docs/framework-integrations.md,docs/runners.md, README Feature Guides + Agent Frameworks tables.rewind-agent0.16.0 → 0.16.1 per CLAUDE.md Track 2.ExplicitClient.__init__()now honors$REWIND_URL(was hardcoded to127.0.0.1:4800). Found via smoke testing on a non-default port; the intercept singleton was silently recording to nowhere whenREWIND_URLwas set.Why
Today every new agent/framework that wants to use Rewind reinvents the HTTP-emitter wheel. The pattern repeats across the AI infra ecosystem: private LLM gateways and bespoke transports each end up with hundreds of LOC of custom session/event-emitter code, and that custom code drifts over time because there is no shared contract.
The SDK already shipped the parts (
ExplicitClient,intercept.install(), contextvar-aware sessions). But the working pattern requires three things in the right order: open a session → callinstall(...)→ tear both down on exit. Get the order wrong and intercept records nothing — silently, becauserecord_llm_callreturnsNonewhen no session is active (explicit.py:276-278).After this PR, every Tier-1 integration becomes:
That replaces several hundred LOC of bespoke session-management code per integrator with one
withblock.What's in
connector.setup()REWIND_ENABLED=0makessetup()a true no-op (yieldsNone, no HTTP, no install).REWIND_LLM_HOSTS=a.example,b.example(comma-separated) extends the strict-by-default LLM provider list. Kwargs override env.REWIND_URLenv var orbase_url=kwarg.REWIND_SESSION_ID+REWIND_REPLAY_CONTEXT_IDare set (runner-subprocess pattern fromdocs/runners.md),setup()skips creating a fresh session and letsintercept.install()attach to the existing replay context. No phantom sessions during runner-driven replays.ExplicitClientfor callers that needrecord_llm_call/record_tool_callon custom transports (gRPC, in-process LLMs).Tests
12 tests in
python/tests/test_connector.pycovering kill switch (env + kwarg), session start/end,thread_id/metadatapropagation, replay-dispatch env detection, host predicates from kwarg/env/defaults, idempotent intercept install, and the public module export.Full python suite locally:
488 passed, 1 skipped, 0 failed(the 1 skip is unrelated env-conditional).Why first-class API in
rewind_agent(not a separate package)The original plan considered shipping the connector as an external package per org. After Santa Method review, the abstraction is fully generic: nothing in it is specific to a particular gateway, app, or framework. Every integrator would write the same shape. Keeping it in the SDK gives every consumer one blessed pattern, eliminates the silent-no-op pitfall, and avoids per-team copy-paste drift.
This is meaningfully smaller than the
FrameworkAdapterProtocol Santa Method review rejected from the original plan: no new type, noStore/Recorderleak, no entrypoint discovery, no Protocol — just one@contextmanagerfunction that calls two existing public APIs in the right order.Smoke test results
End-to-end verified against a live
target/release/rewind 0.15.0server (default + non-default port):import rewind_agentresolvesconnector.setupis_installed()toggles correctly across thewithblock_session_id/_timeline_idcontextvars set inside the blockPOST /api/sessions/start)api.openai.cominterceptedCompletedon exit_is_replay_dispatch()=True, no phantom sessionREWIND_URL=...:4802→ both connector and intercept singleton record to 4802Test plan
cd python && pytest tests/test_connector.py -v— 12/12 green.cd python && pytest --ignore=tests/e2e— full suite green (488 passed locally).docs/hdk.mdon GitHub and confirm all internal links resolve (intercept-quickstart.md,recording-api.md,runners.md,framework-integrations.md).with rewind_agent.connector.setup("smoke"): httpx.post("https://api.openai.com/v1/chat/completions", ...)against a runningrewind webserver; confirm session shows up viarewind show latest.REWIND_SESSION_ID+REWIND_REPLAY_CONTEXT_IDin a subshell; confirmsetup()does NOT create a phantom/api/sessions/startcall.python/scripts/publish-pypi.sh.🤖 Generated with Claude Code