Fix CI badge to reference master branch#1
Merged
Merged
Conversation
The badge URL defaulted to the repo's default branch (main) but our actual branch is master, causing a stale failing status. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
risjai
added a commit
that referenced
this pull request
Apr 10, 2026
…arison Enterprise-grade evaluation system for measuring agent output quality, not just structural regressions. Closes the #1 feature gap vs LangSmith. New crate: rewind-eval - DatasetManager: versioned test-case collections with JSONL import/export - 5 built-in deterministic scorers: exact_match, contains, regex, json_schema, tool_use_match - ExperimentRunner: subprocess protocol (stdin JSON → stdout JSON), timeout, error handling - ExperimentComparison: side-by-side delta computation with regression/improvement detection CLI (rewind eval): - dataset create/import/export/show/list/delete/add-from-session - evaluator create/list/delete - run: execute target command against dataset, score, aggregate - compare: diff two experiments with color-coded output - --json output for CI pipelines, --fail-below threshold with exit code 1 Storage: 6 new SQLite tables (datasets, dataset_examples, evaluators, experiments, experiment_results, experiment_scores) with versioning and content-addressed blob storage. Web API: 7 read-only GET routes under /api/eval/* MCP: 5 read-only tools (list/show datasets, list/show/compare experiments) Python SDK: Dataset, evaluate(), @evaluator decorator, compare(), 4 built-in scorers Web UI: EvalDashboard with tabs (Datasets/Experiments/Compare), DatasetBrowser, ExperimentList, ExperimentDetail with expandable scores, ExperimentComparison with color-coded deltas, ScoreBadge component Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 10, 2026
risjai
added a commit
that referenced
this pull request
Apr 12, 2026
- Fix #1 (BUG): Thread session_name through _init_proxy so fallthrough creates the session with the caller's name, not hardcoded "default" - Fix #2 (DESIGN): _init_proxy returns bool instead of mutating _mode via hidden side effect; init() handles the mode switch - Fix #4: Remove dead urllib.error import - Fix #5: Gate proxy health endpoint on GET method only - Fix #6: Add version field to proxy health response for consistency with rewind-web - Fix #7: Add 5 new tests — slow proxy timeout, non-Rewind server rejection, session name preservation, uninit after fallthrough, init() mode assertion - Fix #8: Validate health response body (check "status": "ok") to prevent false positives from non-Rewind services on the same port Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
risjai
added a commit
that referenced
this pull request
Apr 12, 2026
Add a ProxyCircuitBreaker that detects proxy failure mid-session and transparently falls through to direct recording. The circuit breaker: - Detects failures from LLM SDK connection errors (APIConnectionError, APITimeoutError) — no per-request health checks, zero latency on the happy path - Trips to OPEN after 2 consecutive connection failures - In OPEN state, preemptively skips the proxy and retries via a throwaway client pointing at the original upstream URL (thread-safe, no shared mutable state) - Creates a local Store + Recorder for direct-mode recording while proxy is down (session named "<original> (proxy-fallback)") - After 30s recovery timeout, probes the proxy in HALF_OPEN state - On successful probe, tears down direct resources and resumes proxy mode Design decisions (from review): - Throwaway client for retries (resolves thread-safety issue with base_url swap — review finding #1) - Preemptive skip in OPEN state (zero latency penalty — finding #2) - New session in local store on trip (simple, no cross-store merge — finding #3) - APITimeoutError included in detection (finding #5) Known limitations: - Mid-stream proxy failure cannot be retried (LLM responses are non-deterministic) - Timeout detection takes SDK timeout period (60-120s) × threshold 23 new tests: error detection (7), state machine (13), integration (3). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6 tasks
risjai
added a commit
that referenced
this pull request
Apr 12, 2026
- Fix #1 (HIGH): Add `from __future__ import annotations` for Python 3.9 compat — `str | None` syntax is 3.10+ - Fix #2 (MEDIUM): Remove APITimeoutError from detection — timeouts may originate from slow upstream, not dead proxy. False trips when proxy is healthy but LLM is slow. - Fix #3 (MEDIUM): Skip record_success() for streaming calls — stream object returned doesn't mean data has flowed. Proxy could die mid-stream after failure count was reset. - Fix #4 (MEDIUM): Add retry-path test — verifies direct recorder stores steps correctly after circuit trips to OPEN - Fix #5 (LOW): Copy `organization` in throwaway OpenAI client for org-scoped API key compatibility Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
risjai
added a commit
that referenced
this pull request
Apr 12, 2026
* feat: mid-session circuit breaker for proxy mode (Phase 3) Add a ProxyCircuitBreaker that detects proxy failure mid-session and transparently falls through to direct recording. The circuit breaker: - Detects failures from LLM SDK connection errors (APIConnectionError, APITimeoutError) — no per-request health checks, zero latency on the happy path - Trips to OPEN after 2 consecutive connection failures - In OPEN state, preemptively skips the proxy and retries via a throwaway client pointing at the original upstream URL (thread-safe, no shared mutable state) - Creates a local Store + Recorder for direct-mode recording while proxy is down (session named "<original> (proxy-fallback)") - After 30s recovery timeout, probes the proxy in HALF_OPEN state - On successful probe, tears down direct resources and resumes proxy mode Design decisions (from review): - Throwaway client for retries (resolves thread-safety issue with base_url swap — review finding #1) - Preemptive skip in OPEN state (zero latency penalty — finding #2) - New session in local store on trip (simple, no cross-store merge — finding #3) - APITimeoutError included in detection (finding #5) Known limitations: - Mid-stream proxy failure cannot be retried (LLM responses are non-deterministic) - Timeout detection takes SDK timeout period (60-120s) × threshold 23 new tests: error detection (7), state machine (13), integration (3). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #51 review findings - Fix #1 (HIGH): Add `from __future__ import annotations` for Python 3.9 compat — `str | None` syntax is 3.10+ - Fix #2 (MEDIUM): Remove APITimeoutError from detection — timeouts may originate from slow upstream, not dead proxy. False trips when proxy is healthy but LLM is slow. - Fix #3 (MEDIUM): Skip record_success() for streaming calls — stream object returned doesn't mean data has flowed. Proxy could die mid-stream after failure count was reset. - Fix #4 (MEDIUM): Add retry-path test — verifies direct recorder stores steps correctly after circuit trips to OPEN - Fix #5 (LOW): Copy `organization` in throwaway OpenAI client for org-scoped API key compatibility Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: move imports to top of file to satisfy ruff E402 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
14 tasks
risjai
added a commit
that referenced
this pull request
Apr 13, 2026
Fixes all items from PR #56 review: #1 (P0 bug) Retry by OpenAI exception type, not string matching: - New `_is_retryable()` checks `openai.RateLimitError`, `openai.APIStatusError(500/502/503)`, `ConnectionError`, `TimeoutError` - "Token limit 500 exceeded" no longer triggers false retries - Added 7 tests including retry-then-succeed integration test #2 (P0 bug) Add --force flag to bypass stale score cache: - `rewind eval score --force` skips the `get_timeline_score` cache check - Upsert in `create_timeline_score` now reachable after code changes #3 (P1 security) Sanitize API keys from stderr before storing: - `sanitize_secrets()` redacts `sk-[a-zA-Z0-9_-]{10,}` patterns - Applied to LLM judge subprocess stderr before writing to reasoning - 3 tests for single key, multiple keys, and clean text #4 (P2) Remove unused `_session_id` from `extract_timeline_output()`: - Public API now takes `(store, timeline_id)` — cleaner signature - All callers (CLI + tests) updated #5 (P2) Compare each fork against main, not just first vs last: - Delta section now shows one line per fork, each compared to main - Works correctly for 3+ timelines #6 (P3) ValueError exits with code 1 in subprocess: - `ValueError` and `RuntimeError` now exit non-zero so Rust surfaces them as errors, not misleading zero scores - Test added for correctness-without-expected path Tests: 26 Rust + 41 Python passing, clippy clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 13, 2026
risjai
added a commit
that referenced
this pull request
Apr 14, 2026
Four issues caught by independent reviewers (both flagged #1 and #2): 1. Command parsing: split_whitespace() → sh -c delegation. Handles quoted args, paths with spaces, pipes, and shell variables correctly. 2. Proxy readiness: replaced 500ms sleep with health endpoint polling (20 attempts × 100ms). Surfaces bind errors instead of swallowing them with `let _ =`. Bails with clear message if proxy doesn't start. 3. Session source guard: changed from blocking Direct only to allowlisting Proxy only. Hooks and OtelImport sessions also lack proxy-compatible HTTP blobs and would fail confusingly during replay. 4. Async process: std::process::Command::status() (blocking) → tokio::process::Command::status().await. Prevents blocking the tokio runtime during long-running agent commands. Also: added clap `requires = "apply"` on --command flag so it can't be used without --apply (previously silently ignored). Made-with: Cursor
risjai
added a commit
that referenced
this pull request
Apr 14, 2026
…ewriting (#90) * fix: sync Python store schema with Rust — add tool_name and source columns The Python store was missing two v0.6 migrations that the Rust schema already had: `steps.tool_name` and `sessions.source`. This caused `get_steps()` to omit tool_name, which the upcoming `rewind fix` diagnosis engine needs to identify which tool calls failed. Changes: - Add v0.6 ALTER TABLE migrations matching Rust db.rs lines 301-304 - Add `tool_name` param to `create_step()` and column to `get_steps()` - Add tests for tool_name presence in step dicts Made-with: Cursor * feat: add rewind fix diagnosis engine (python/rewind_agent/fix.py) LLM-powered diagnosis subprocess that analyzes failed agent sessions and returns structured output: root cause, failed step, fix type, fix params, and confidence level. Uses function calling to force structured output. Supports 5 fix types: swap_model, inject_system, adjust_temperature, retry_step (exploit non-determinism), and no_fix (agent code issue). Follows the exact subprocess protocol from llm_judge.py: stdin JSON, stdout JSON, 120s timeout, non-zero exit on config errors. Made-with: Cursor * feat: add `rewind fix` CLI command — AI-powered session diagnosis Adds the `rewind fix <session>` command that analyzes a failed agent session and prints a structured diagnosis with root cause, suggested fix type, and confidence level. The command loads session data from the local store, assembles a diagnostic payload, and shells out to `python3 -m rewind_agent.fix` (same subprocess protocol as llm_judge). Supports: --step N (target specific step), --expected (soft failures), --json (machine output), --diagnosis-model. The --apply, --command, and --hypothesis flags are defined but stubbed for Phase 2-3. Made-with: Cursor * feat: add proxy request rewriting for `rewind fix --apply` Adds RewriteConfig struct and apply_rewrites() to the proxy, enabling LLM request modification after the fork point during replay. Supports three rewrite types: - model swap (same-provider only; cross-provider swaps blocked) - system message injection (OpenAI role:system + Anthropic string/array) - temperature override Key design decisions from Santa Method review: - Provider detection uses model name, not body structure - Anthropic array-format system (for prompt caching) handled correctly - Content-Length header filtered from copy loop (reqwest recalculates) - Cross-provider model swaps log warning and skip (not silently fail) Includes 14 unit tests covering all rewrite paths and edge cases. Made-with: Cursor * fix: address Santa Method review findings - Add sys.exit(1) to Python catch-all exception handler so Rust CLI correctly surfaces unexpected diagnosis failures as errors (both reviewers flagged this independently) - Handle stdin write failure explicitly instead of silently swallowing with `let _` — kills child and reports payload size on pipe failure - Add try/except for json.loads of LLM function call arguments to produce a clear error when diagnosis model returns malformed output Made-with: Cursor * feat: implement `rewind fix --apply` and `--command` orchestration Completes Phase 3: the full diagnose → fork → replay-with-patch → score loop. Three modes now work: - `rewind fix latest` — diagnosis only (Phase 1, unchanged) - `rewind fix latest --apply` — diagnose, dry-run preview, fork, start proxy with rewrites, wait for Ctrl+C, print savings - `rewind fix latest --apply --command "python agent.py"` — fully automated: spawns agent with OPENAI_BASE_URL and ANTHROPIC_BASE_URL pointing at the patched proxy, waits for completion, prints savings Also implements: - `--hypothesis "swap_model:gpt-4o"` to skip diagnosis and directly test a fix theory (power user escape hatch) - Dry-run preview with y/N confirmation (skip with --yes) - Direct-mode session guard (--apply requires proxy-recorded sessions) - retry_step fix type starts proxy with no rewrite config Made-with: Cursor * fix: address Santa review of Phase 3 orchestration Four issues caught by independent reviewers (both flagged #1 and #2): 1. Command parsing: split_whitespace() → sh -c delegation. Handles quoted args, paths with spaces, pipes, and shell variables correctly. 2. Proxy readiness: replaced 500ms sleep with health endpoint polling (20 attempts × 100ms). Surfaces bind errors instead of swallowing them with `let _ =`. Bails with clear message if proxy doesn't start. 3. Session source guard: changed from blocking Direct only to allowlisting Proxy only. Hooks and OtelImport sessions also lack proxy-compatible HTTP blobs and would fail confusingly during replay. 4. Async process: std::process::Command::status() (blocking) → tokio::process::Command::status().await. Prevents blocking the tokio runtime during long-running agent commands. Also: added clap `requires = "apply"` on --command flag so it can't be used without --apply (previously silently ignored). Made-with: Cursor * fix: address 6 PR review items from risjai 1. Bug: run_fix_subprocess now uses spawn_blocking to avoid blocking the tokio runtime (same issue Santa caught for run_agent_command) 2. Bug: --step N errors when step not found instead of silently falling through to the auto-detection cascade 3. Design: added TODO comment for abort() → graceful shutdown 4. Design: renamed "Scoring both timelines..." to "Replay savings" since we show token/cost savings, not LLM eval scores 5. Nit: added ANTHROPIC_BASE_URL to manual --apply instructions 6. Nit: added RewriteConfig::is_empty() guard so rewrite log only fires when at least one field is set Made-with: Cursor * fix: resolve CI lint failures (ruff F841 + clippy collapsible-if) - Remove unused step_id assignment in test_get_steps_includes_tool_name - Refactor nested if-let to map_err pattern in run_fix_subprocess Made-with: Cursor
10 tasks
risjai
added a commit
that referenced
this pull request
Apr 21, 2026
…I wiring Addresses the review on #133. Must-fix (reviewer #1, #2): - Bump rand from 0.8 → 0.9 so the workspace ships a single rand major version (previously duplicated alongside quinn/proptest/tungstenite/otel) - Drop the generic `B` and `transmute_body` helper from auth_middleware. axum 0.8's from_fn_with_state passes Request<axum::body::Body> directly. Should-fix (reviewer #3, #4, #6): - Scope `?token=<token>` query-param fallback to /api/ws ONLY. Browsers can't set Authorization on WebSocket upgrades; REST must not accept this form because it leaks via Referer, server logs, and browser history. - Fail-closed now also catches IPv6 :: via is_loopback() already returning false for unspecified addrs (added explicit test). - Token file creation is now atomic: OpenOptions::create_new + mode(0o600). Closes the umask window where fs::write created at 0644 before the subsequent chmod tightened it. Also handles AlreadyExists (concurrent writer won the race → re-read their token). UI wiring for token-enabled deployments: - New web/src/lib/auth.ts — minimal localStorage-backed token holder - api.ts injects Authorization: Bearer on every request; prompts the user and retries once on 401, clears on second 401 - use-websocket.ts appends ?token= to the WS URL when a token is stored - SPA static assets still load without a token (only /api/* and /api/ws are gated), so the UI loads the login prompt fine Nits: - Add TODO for `rewind auth rotate` command (follow-up) - Tighten resolve_with_env to pub(crate) — it's test-only, not API surface - Replace tmp.keep() with a returned TempDir guard so test data dirs are cleaned up on drop - Replace flaky 100ms timeout sentinels with health-probe loops that wait for /_rewind/health to return 200 Tests: - 17/17 auth_tests pass (was 12) — new: IPv6 ::, REST rejects ?token=, WS accepts/rejects with ?token=, concurrent token-file race - Full workspace: 360+ tests, 0 failures - Clippy clean on all modified files - Manual smoke: dashboard loads, Bearer-auth REST works, WS upgrade with ?token= returns 101, without returns 401 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7 tasks
risjai
added a commit
that referenced
this pull request
Apr 27, 2026
Santa returned NAUGHTY with 7 blocking findings. This commit addresses all of them and adds 7 directed regression tests. Pre-push verification suite (ruff + pytest local + pytest bare-env + clippy + cargo test) all green — including a CI-equivalent simulated-bare-env pytest run that hides httpx/requests/aiohttp via meta_path blocker (367 passed, 12 skipped, 0 failed in that env). ## Fix → Finding mapping ### #6 — Version bump skipped unpublished 0.15.0 Reverted python/pyproject.toml + python/rewind_agent/__init__.py from 0.16.0 back to 0.15.0. Per CLAUDE.md track 2: master is at 0.15.0, PyPI is at 0.14.8 — 0.15.0 is unpublished, so Phase 1 changes ride with it. Bumping to 0.16.0 would skip the unpublished version. ### #1 follow-up — install tests must work in CI's bare env test_intercept_install.py asserted all three adapters patched. CI doesn't install httpx/requests/aiohttp, so HTTPX_AVAILABLE etc. all flip to False, install() no-ops the missing adapters, and the unconditional asserts fail. Made the assertions conditional on *_AVAILABLE flags. Each adapter is asserted patched if-and-only-if its library is importable. The test_install_with_custom_predicates_applies_to_httpx test gains a self.skipTest() guard since it actually exercises the transport patch with a MockTransport (httpx-required). ### #4 — Strict-match 409 was silently swallowed ExplicitClient._post wraps urllib in `try: ... except Exception: return None`. When the server returns HTTP 409 strict-match divergence, urllib raises HTTPError(409) → swallowed → caller sees None → adapters interpret as cache miss → fall through to live. Strict mode is broken. Fix: new ExplicitClient._post_replay_lookup with explicit 409 handling. HTTP 409 raises a new typed exception RewindReplayDivergenceError (exported from rewind_agent). Other 4xx/5xx errors still degrade to None (cache miss) so transient Rewind outages don't break the agent's normal flow. get_replayed_response / _async use the new method; their docstrings note that strict-mode divergence propagates as an exception. RewindReplayDivergenceError carries the server's diagnostic message plus optional target_step / stored_hash / incoming_hash for caller diagnostics. ### #5 — httpx configured default transport options were dropped When user did `httpx.Client(verify=False, http2=True, ...)` without transport=, our patched __init__ replaced transport=None with a fresh RewindHTTPTransport() BEFORE httpx's __init__ ran. That short- circuited httpx's own configured-default construction, dropping verify / cert / trust_env / http2 / proxies / limits / local_address / retries / socket_options / default_encoding / etc. Fix: two-mode patched_init. Mode (a) user passed transport=X — wrap X (existing behavior). Mode (b) user passed only top-level config — call original_init first (httpx builds its configured default at self._transport), then post-init wrap self._transport with RewindHTTPTransport(_inner=…). Same fix applied to AsyncClient.__init__. The only fragility is that we read self._transport which is httpx's documented internal attribute (stable across 0.x). ### #2 — Live streaming responses pre-read body before passthrough _flow.{handle_intercepted_async, _serve_cache_miss_sync} called `await resp.json()` / resp.json() on the live response BEFORE returning. For streaming responses this: - httpx: raises ResponseNotRead (body not yet streamed) - requests: marks _content_consumed=True, blocking iter_content - aiohttp: closes the connection after read User code's streaming iteration breaks every time. Fix: detect streaming via OR-combined transport hints + body-aware detect_streaming(req) — see #3. For STREAMING misses: pass through the live response immediately, record with placeholder response_value=None and zero tokens. Tee-based stream-recording (matching the Rust proxy's handle_streaming_response) is documented as a follow-up (v1.1) — proper capture without consuming requires a wrapping ByteStream that yields chunks AND captures, fired on stream completion. For NON-streaming misses (the common case for buffered chat completions): pre-read remains correct — httpx/requests/aiohttp buffer the body eagerly anyway. ### #3 — Body-only "stream": true wasn't routed to streaming Adapters set req.stream from transport-level signals (Accept: text/event-stream, library-specific stream=True hints). A request with `{"stream": true}` in the JSON body but no Accept header was treated as non-streaming — buffered cache hit instead of synthetic SSE. Fix: handle_intercepted_{sync,async} now OR-combines is_streaming (adapter signal) with detect_streaming(req) (Phase 0 body-aware helper that recognizes the JSON `"stream": true` field via regex, plus Accept header, plus explicit RewindRequest.stream). ### #7 — docs and ray-agent migration deferred (not in this PR) The plan listed docs/intercept-quickstart.md and the ray-agent port as Phase 1 deliverables. They're absent from this PR and the PR description should say so explicitly. The ray-agent migration files (use-cases/ray-agent/code/rewind_setup.py, use-cases/ray-agent/INTEGRATION-DIFF-V2.md) live in the local working tree but use-cases/ is .gitignore'd in this repo — they're meant to be copied into the ray-agent repo (separate repository). This was already noted in the version-bump commit message but the PR top-level description didn't make it visible. Will update the PR description with a clearer "deferred" section in the next reply to Santa. docs/intercept-quickstart.md is a follow-up; the in-tree rustdoc on intercept.install() and the operator-facing README in the use-cases/ray-agent/ directory cover the immediate need. ## New regression tests (7 in tests/test_intercept_santa_fixes.py) - test_strict_match_409_raises_typed_error — Santa #4 - test_non_409_http_error_is_swallowed_to_cache_miss — Santa #4 boundary - test_verify_false_setting_survives_intercept_install — Santa #5 - test_user_supplied_transport_is_wrapped_not_replaced — Santa #5 mode (a) - test_streaming_miss_passes_through_without_consuming_body — Santa #2 - test_cache_hit_with_body_stream_true_emits_synthetic_sse — Santa #3 - test_install_only_patches_available_adapters — Santa #1 follow-up ## Pre-push verification routine (followed BEFORE this push) User correctly called out that I'd been pushing without running the full lint/test stack locally. Established a 5-stage pre-push routine that mirrors CI exactly: 1. ruff check . — Python lint (CI's exact invocation) 2. pytest tests/ in local env (with all libs installed) 3. pytest tests/ in simulated bare env (httpx/requests/aiohttp blocked via sys.meta_path) — catches CI-only failures locally 4. cargo clippy -- -D warnings — Rust CI's exact invocation 5. cargo test --workspace — Rust CI's exact invocation All 5 stages green for this commit: - ruff: All checks passed! - pytest local: 420 passed, 1 skipped (was 413; +7 Santa regression tests) - pytest bare env: 367 passed, 12 skipped, 0 failed (CI mirror) - cargo clippy: clean - cargo test: all green, 0 FAILED Adding scripts/pre-push-check.sh as a reusable harness in a follow-up commit so future sessions don't have to re-derive this routine. Made-with: Cursor
risjai
added a commit
that referenced
this pull request
Apr 27, 2026
…ness bugs Re-review at commit 1f9a2dd identified four real bugs surviving the first round of Santa fixes. CI was green and prior items were resolved, but mock-transport tests masked the real httpx behavior. This commit fixes all four with directed regression tests using shapes that match real (not mock) library behavior. ## Fix → Re-review finding ### Re-review #1 — httpx ResponseNotRead crashes on real transport ``_flow._read_response_body_{sync,async}`` called ``resp.json()`` directly. Real httpx response bodies are NOT auto-read at the transport layer; they're streamed lazily and ``.json()`` raises ``httpx.ResponseNotRead`` until ``.read()`` / ``.aread()`` runs. The prior tests used ``httpx.MockTransport`` + ``httpx.Response(json=…)`` which auto-reads the body via the constructor — masking the bug. Fix: explicit body materialization before parsing. Sync path: try ``resp.read()`` (httpx) before ``resp.json()``. ``read()`` is idempotent on already-read responses (requests has ``_content`` populated; aiohttp doesn't reach the sync path). Async path: try ``await resp.aread()`` (httpx) first, falling back to ``await resp.read()`` (aiohttp) if aread isn't present. Both libraries diverge on whether the read API is async; we handle both. Broadened the exception-catch in both paths from ``(json.JSONDecodeError, ValueError)`` to bare ``Exception`` so any transport-specific exception (httpx.ResponseNotRead, aiohttp's ContentTypeError, etc.) degrades to None recording rather than propagating up through the user's call site. ### Re-review #2 — RewindHTTPTransport leaks _inner ``RewindHTTPTransport`` and ``RewindAsyncHTTPTransport`` delegated ``handle_request`` / ``handle_async_request`` to ``self._inner`` but didn't override ``close()`` / ``aclose()``. When the user's Client closes (explicitly or via ``__exit__``), only our own resources got released — the wrapped configured/user transport leaked its connection pool, SSL context, and cert verifier for the process lifetime. Fix: explicit close / aclose forwarding. def close(self): if self._inner is not None: try: self._inner.close() except Exception: pass super().close() Async variant uses ``await self._inner.aclose()``. Both swallow exceptions on the inner close to preserve the super().close() invariant — even a buggy inner transport shouldn't leak our own cleanup. ### Re-review #3 — aiohttp base_url + relative paths bypass intercept ``aiohttp_middleware._build_rewind_request`` did ``url = str(str_or_url)`` before aiohttp resolves relative URLs against ``ClientSession._base_url``. So ``session.post("/v1/chat/completions")`` (typical OpenAI SDK pattern when configured with a custom gateway base URL) produced a RewindRequest whose URL was just the path. Host-based predicates (the default — matches ``api.openai.com`` etc.) silently failed, the request bypassed interception, no recording. Fix: new ``_resolve_url(session, str_or_url)`` helper that replicates aiohttp's own resolution logic. If ``session._base_url`` is set and the supplied URL doesn't start with a scheme (``http:``/``https:``/``ws:``/``wss:``), yarl-join the two. yarl is a hard aiohttp dep so importing it inside the helper is safe — we wouldn't be in this code path otherwise. Defensive: if yarl join blows up on a malformed URL, return the bare string. Predicate match will fail (silent bypass) but at least we don't crash the user's request. ### Re-review #4 — PR description metadata stale Code reverted to SDK 0.15.0 in the previous fix commit, but the PR description still said "0.15.0 → 0.16.0" and listed ``./scripts/publish-pypi.sh`` for SDK 0.16.0 in post-merge actions. Will update the PR body via ``gh pr edit`` in the same push. ## Regression tests added (9 new in test_intercept_santa_fixes.py) Total Santa fix tests: 7 (initial) + 9 (re-review) = **16 tests, all green**. Re-review #1 coverage: - test_sync_read_called_before_json_on_unread_response — constructs a real ``httpx.Response`` with ByteStream (not pre-read) and verifies ``_read_response_body_sync`` reads + parses correctly - test_async_aread_called_before_json_on_unread_response — same for async path - test_buffered_cache_miss_through_real_transport_records_tokens — end-to-end through AsyncClient. The MockTransport handler returns ``httpx.Response(stream=ByteStream(body))`` (not the prior ``json=body`` shape that auto-reads) so this test actually exercises the fix. Re-review #2 coverage: - test_sync_close_propagates_to_inner_transport — spy MockTransport counts close() calls; assert >= 1 after Client.close() - test_async_aclose_propagates_to_inner_transport — async equivalent Re-review #3 coverage: - test_relative_path_resolves_against_base_url — direct unit test on _resolve_url with a yarl URL base - test_absolute_url_passes_through_unchanged — absolute URLs aren't mangled even when base_url is set - test_no_base_url_returns_input_unchanged — sessions without base_url stringify the path as-is - test_aiohttp_session_base_url_match_records_via_predicate — end-to-end through aiohttp.ClientSession(base_url=...) + relative POST + default host predicate. Asserts recording fires (= predicate matched the resolved URL). ## Pre-push verification (codified in scripts/pre-push-check.sh) All 5 stages green BEFORE this push: - ruff check . — clean - pytest tests/ (local env, all libs) — 429 passed, 1 skipped (was 420; +9 re-review regression tests) - pytest tests/ (simulated bare env, libs hidden via meta_path) — 367 passed, 12 skipped, 0 failed (CI mirror) - cargo clippy -- -D warnings — clean - cargo test --workspace — all green The local-pre-push routine exists specifically because PR #149's prior CI iterations missed bugs that depended on real-vs-mock behavior; this re-review's #1 finding (ResponseNotRead) was visible ONLY when constructing test responses with the ``stream=ByteStream(...)`` shape that matches real transport output, not the ``Response(json=...)`` shape that auto-buffers. Future regression tests should default to the stream= shape for httpx wherever realism matters. Made-with: Cursor
8 tasks
risjai
added a commit
that referenced
this pull request
Apr 27, 2026
Reviewer caught two real correctness bugs in the Phase 2 decorator. Both fixed with directed regression tests using the REAL ExplicitClient guard (not patched away). ## Review #2 — custom cache_key didn't actually control cache identity `_build_request_payload()` included `args_repr` and `kwargs_repr` in the request body. Phase 0's content validation hashes the WHOLE body to derive the request_hash, so two calls with the same custom ``cache_key=lambda client, q: q`` but different client objects (different memory addresses → different reprs) produced different request_hashes. Cache miss every time. The custom cache_key was effectively cosmetic. Fix: identity-only payload. Request body now contains only ``_rewind_decorator``, ``fn_name``, ``cache_key``. Args / kwargs are NOT in the payload — when user passes ``cache_key=lambda client, q: q``, the unstable client repr is correctly invisible to the server hash. For dashboard display, fn_name + cache_key suffices to identify the call. Power users wanting human-readable display in the dashboard can use plain-string cache_keys (e.g. ``f"chat:{model}:{question[:50]}"``) instead of opaque SHA-256 hashes. Regression tests: - ``test_custom_cache_key_payload_is_independent_of_arg_reprs`` — two object() instances with different reprs but same custom key produce IDENTICAL request payloads. - ``test_decorator_with_custom_cache_key_records_stable_request_across_clients`` — end-to-end through the decorator: two recordings with different clients + same key produce equal request bodies. - ``test_default_cache_key_payload_independent_of_unstable_object_args`` — pins the EXPECTED non-stability of default keys (the whole reason custom cache_key exists), so a future "smart" default doesn't silently drift. ## Review #1 — no-session quickstart records nothing `ExplicitClient.record_llm_call()` returns None silently when ``_session_id`` is unset. The quickstart docs claimed "First call hits OpenAI, records the return value" but unless the user separately wrapped execution in ``client.session(...)``, ``ensure_session(...)``, or ``init()``, recording was a silent no-op. Tests bypassed this with ``patch.object(ExplicitClient, "record_llm_call")``, masking the bug. This is consistent with the rest of the SDK (init() / intercept have the same precondition), but the docs need to say so. Auto- session creation in the decorator was considered and rejected: naming, lifecycle, and thread-safety concerns; the explicit-session pattern matches what users already do for other parts of the SDK. Fix: - New "Session requirement" section at the top of docs/cached-llm-call.md with three patterns (scoped session, long-lived ensure_session, init() auto-session). - Quickstart updated to wrap the example in ``with client.session("my-quickstart"):``. - Replay path documented inline. Regression tests: - ``test_no_session_function_runs_and_returns_live_result`` — verify decorator is a silent no-op for recording when no session active; function still runs, returns live result correctly. - ``test_no_session_record_is_silent_no_op`` — calls the REAL ``ExplicitClient.record_llm_call`` (no patch) with no session and asserts it returns None without raising. The test the reviewer asked for: shows the decorator's behavior under the actual production guard. - ``test_active_session_records_via_real_client_path`` — inverse: session-active path reaches _post (verified via patch on _post, not on record_llm_call). Confirms the guard semantics in both directions. ## Test count - cached_call tests: 26 → **32** (+6 review regressions) - Total Python tests: pre-push routine reports green across all 5 stages ## Pre-push verification All 5 stages green BEFORE push (scripts/pre-push-check.sh): - ruff: clean - pytest local: passes - pytest bare-env (CI mirror): passes - cargo clippy: clean - cargo test --workspace: clean Made-with: Cursor
8 tasks
3 tasks
risjai
added a commit
that referenced
this pull request
Apr 28, 2026
…tep+1 (#156) The dispatch shape A path was passing a.at_step as the create_replay_context from_step, conflating two distinct concepts: - at_step is the FORK point (controls which inherited prefix the fork timeline shares with main, and where new recordings get numbered) - from_step is the cache CURSOR base (controls which recorded step the next lookup targets) Combined with peek_next_replay_step returning current_step + 1, the agent's first cache lookup targeted recorded step at_step + 1. Concrete repro on dev1 with at_step=1, session "What clusters are running?" (3-step recording): agent first LLM call -> lookup target = 1 + 1 = 2 step 2 is tool_call, expected LLM -> MISS agent first tool call -> lookup target = 1 + 2 = 3 step 3 is LLM, expected tool -> MISS Both lookups missed, the agent re-ran every step live, and the dashboard diff rendered the entire recording as "main only" past step at_step. Looked like the cache was broken; was actually ordinal misalignment. Fix: always pass 0 as from_step. Ray Serve runners (and any other client that spawns the agent fresh per dispatch) re-execute from scratch, so the cache must align with recorded step #1 on the first call. The fork still inherits the at_step prefix so operators get the "fork from step N" UX they expect; only the cursor is decoupled from the fork point. 20 replay_jobs_tests pass including new regression shape_a_replay_context_starts_cursor_at_recording_step_one. Made-with: Cursor
This was referenced Apr 29, 2026
shivam2199
pushed a commit
to shivam2199/rewind
that referenced
this pull request
Apr 29, 2026
…CAL-02) (agentoptics#133) * feat(auth): fail-closed auth for non-loopback web server binds Addresses CRITICAL-02 from the security audit: when Rewind bound to a non-loopback address (documented K8s pattern with REWIND_BIND_HOST=0.0.0.0), all API/WebSocket/OTLP routes were exposed without authentication. Any reachable host could read full LLM prompts/responses, pollute sessions, or chain into the SSRF via the OTel export endpoint. Changes: - New `rewind-web` auth middleware with constant-time token comparison (subtle::ConstantTimeEq) - `WebServer::run()` refuses to start on non-loopback bind without a configured token; `--no-auth` is an explicit escape hatch - New `--auth-token` / `REWIND_AUTH_TOKEN` CLI flags; auto-generates a 64-char hex token at ~/.rewind/auth_token (chmod 0600) on first non-loopback start - Loopback behavior unchanged (backward compat for MCP/SDK/CLI flows) - /_rewind/health bypasses auth (liveness probe) - 6 unit tests + 12 integration tests covering fail-closed, escape hatch, token precedence, file permissions, and per-route enforcement Scope: loopback deployments remain open by default. Loopback WebSocket-CSRF (MEDIUM-09) is partially addressed — fully closed on non-loopback, still open on loopback (follow-up PR). Plan: /Users/jain.r/.claude/plans/squishy-strolling-bachman.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(auth): address PR agentoptics#133 review — rand dedup, WS ?token=, TOCTOU, UI wiring Addresses the review on agentoptics#133. Must-fix (reviewer agentoptics#1, agentoptics#2): - Bump rand from 0.8 → 0.9 so the workspace ships a single rand major version (previously duplicated alongside quinn/proptest/tungstenite/otel) - Drop the generic `B` and `transmute_body` helper from auth_middleware. axum 0.8's from_fn_with_state passes Request<axum::body::Body> directly. Should-fix (reviewer agentoptics#3, agentoptics#4, agentoptics#6): - Scope `?token=<token>` query-param fallback to /api/ws ONLY. Browsers can't set Authorization on WebSocket upgrades; REST must not accept this form because it leaks via Referer, server logs, and browser history. - Fail-closed now also catches IPv6 :: via is_loopback() already returning false for unspecified addrs (added explicit test). - Token file creation is now atomic: OpenOptions::create_new + mode(0o600). Closes the umask window where fs::write created at 0644 before the subsequent chmod tightened it. Also handles AlreadyExists (concurrent writer won the race → re-read their token). UI wiring for token-enabled deployments: - New web/src/lib/auth.ts — minimal localStorage-backed token holder - api.ts injects Authorization: Bearer on every request; prompts the user and retries once on 401, clears on second 401 - use-websocket.ts appends ?token= to the WS URL when a token is stored - SPA static assets still load without a token (only /api/* and /api/ws are gated), so the UI loads the login prompt fine Nits: - Add TODO for `rewind auth rotate` command (follow-up) - Tighten resolve_with_env to pub(crate) — it's test-only, not API surface - Replace tmp.keep() with a returned TempDir guard so test data dirs are cleaned up on drop - Replace flaky 100ms timeout sentinels with health-probe loops that wait for /_rewind/health to return 200 Tests: - 17/17 auth_tests pass (was 12) — new: IPv6 ::, REST rejects ?token=, WS accepts/rejects with ?token=, concurrent token-file race - Full workspace: 360+ tests, 0 failures - Clippy clean on all modified files - Manual smoke: dashboard loads, Bearer-auth REST works, WS upgrade with ?token= returns 101, without returns 401 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(auth): use percent-encoding crate; strengthen loopback test Follow-up to the PR agentoptics#133 second-pass review. Blocker resolved: - Replace the hand-rolled percent-decoder in `extract_token` with `percent_encoding::percent_decode_str` (already transitive via reqwest/ url/axum; now an explicit dep). Deletes ~30 lines and fixes two bugs in the hand-rolled version: 1. `+` was translated to space. That's `application/x-www-form-urlencoded` semantics; RFC 3986 query strings treat `+` as a literal. A token with a `+` would fail comparison. 2. `i + 2 < bytes.len()` was off-by-one, so a valid trailing `%AB` escape was treated as literal chars at the end of the query. Regression tests for both bugs: - `ws_upgrade_accepts_token_with_literal_plus` — token `abc+def+ghi` - `ws_upgrade_accepts_token_with_trailing_percent_escape` — token `abc/` encoded as trailing `%2F` Nit addressed: - `run_allows_loopback_bind_without_token` now also hits `/api/sessions` (not just `/_rewind/health`, which bypasses auth regardless). This actually exercises the backward-compat claim that loopback + no-token leaves normally-protected routes open. Not addressed (flagged as follow-up, non-blocking): - Access-log redaction of `?token=` values — no access-log layer exists yet; address when one is introduced - Replace `window.prompt` with a proper modal UI Tests: 19/19 auth (was 17), full workspace 360+/0, clippy clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): update api.test.ts for auth wiring + handle missing localStorage CI build was failing because the existing `api.test.ts` expected bare `fetch(path)` calls, but `api.ts` now always routes through `request()` which adds a `headers` argument (empty when no token). Two fixes: 1. Update the test assertions to match the new call shape. Added coverage for the auth injection path: - Authorization: Bearer is sent when a token is stored - POST requests merge auth with Content-Type correctly - Unauthed (loopback default) still passes through 2. `auth.ts` now has an in-memory fallback when `window.localStorage` is unavailable. jsdom in this vitest setup doesn't expose localStorage, so `setToken`/`getToken` silently no-oped. Real browsers still use localStorage; Safari private mode and jsdom fall through to memory. Tests: - 113/113 UI tests pass (was 111) - 19/19 auth_tests still green - UI build succeeds 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2 tasks
6 tasks
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.
Summary
mainbranch, but our actual branch ismaster?branch=masterparameter to the badge URLTest plan
🤖 Generated with Claude Code