Implement provider resilience primitive#187
Merged
Merged
Conversation
Batch A of the resilience review fixes (providers crate + builder wiring): - H5/M2: all three remote providers (Anthropic, OpenAI, OpenRouter) now wrap their transport core in ResilientProvider internally; the builder only supplies the policy/classifier. AnthropicProvider gains new_with_headers_and_resilience (replacing new_with_headers_and_timeouts) and retry parity from every constructor. - H4: deterministic setup failures (wrong api kind, unencodable request) surface as in-stream Fatal errors and short-circuit the retry budget; the resilience layer recovers typed ProviderErrors from setup Errs by downcast instead of hardcoding Transient. - H6: Anthropic transport failures, HTTP error bodies, and in-band SSE error events route through the shared classifier in openai_error.rs (new classify_error_payload entry point); 429/529/overloaded are retryable with Retry-After hints honored, 4xx stays fatal. - M11: ResilientProvider::compact gets the same bounded retry/timeout treatment as stream (typed retryable errors retried, untyped treated as fatal so capability errors don't loop). - M12: backoff jitter is applied inside the max_backoff cap. - M13: worker-to-consumer channel is bounded (64) and sends race the cancellation token so a stalled consumer can't wedge the worker. - M10: x-api-key/Authorization headers marked sensitive in JsonHttpClient. - M7: full sad/happy-path test suite for http_client (timeouts, cancel, status classification, malformed bodies, header failures). - M8: five copy-pasted HTTP test helpers collapsed into test_http module. - M9: one shared reqwest client factory for both transports. - L4: halter prelude re-exports provider types from the providers module. - L6: ProviderWarning no longer commits an attempt. - L7: CommitDedup doc corrected (in-stream dedup only). - L8: provider_url/public_provider_url deduped into http_client::join_url. - L9: retry-after parsing understands minute units (1m, 2min, 6m59.6s). - L10: unreachable-looking wildcard arms replaced with kind.retryable(). - L11: jitter entropy simplified (RandomState per-instance keys only). - L12: ResponsesProvider inherent/trait duplication collapsed; capabilities precomputed at construction. 35 new tests in halter-providers (152 -> 187). Workspace: 810 passed, 0 failed; clippy --all-features --all-targets clean apart from the pre-existing derivable-Default lint in halter-config (finding L13, owned by another batch); default-feature build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mit and turn-loop hot paths - H1/M6: make hook-store eviction session-scoped. Every live handle for a session shares one EvictionGuard (found via a weak reference on the store entry), so dropping a separately-constructed handle — e.g. the temporary parent handles built during subagent hook dispatch — no longer evicts the parent's accumulated hook state; only the last drop does. Eviction also verifies guard liveness so a dying guard cannot evict an entry owned by a newer one. The trace recorder stays decoupled from handle lifetime (handle gaps are legitimate for sessions that keep tracing); the guard doc now states that invariant instead of a temporary-handle carve-out. Regression tests verified to fail against the pre-fix behavior. - H7: commit_and_publish now takes the committing session's blueprint and short-circuits ancestor lookup before any I/O when forwarding is off; the commit hot path no longer reloads (and deserializes) the session per commit. Removed the now-unused forwarding_ancestors_for_session. - H8/M5: probe_git runs on spawn_blocking, is probed once per turn (reused across provider iterations), and is hardened against hostile repo config (-c core.fsmonitor= / core.hooksPath=, scrubbed env, GIT_CONFIG_* pinned to the null device) since the harness operates in untrusted checkouts. - M3: close racing start_turn no longer detaches an uncancelled turn task: registration re-checks generation/state under the lock and cancels+aborts the orphaned task otherwise (register_running_turn). - M4: SubagentStop-hook-driven turn resubmission is bounded by a cap (8); tripping it fails the subagent with a descriptive error. - M1/L2: prefix_cache_key now fingerprints exactly the rendered prefix bytes (plus the cache boundary hash), covering dynamic Append segments, and no longer double-hashes content_hash+text per segment. - L1: collapse the dead if/else around tool-call dedupe (warn-only branch). - L3: delete the tautological assertion in the ToolConcurrency conformance test (the exhaustive match remains the enforcement). - L5: documented why AssembledPrompt carries both segments and ordered_segments at the population site (removing the field requires halter-protocol/providers changes owned by another batch). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ndings
Batch C of the codebase review remediation:
- H2: converge SessionStore optimistic-concurrency semantics on structural
equality in both backends (sqlite now deserializes and compares with
SessionState::PartialEq instead of comparing JSON strings, which
disagreed with the memory store on IndexMap key order); documented the
contract on the trait and added a shared conformance suite
(crates/halter-session/tests/store_conformance.rs) run against both
backends, including the previously divergent map-ordering case.
- H3 + M18: thread the turn/session CancellationToken into hook dispatch
via HookInvocationContext.cancel; turn interruption now aborts the
in-flight hook and skips remaining ones, making the Cancelled arm
reachable. Session-level entry points without a turn scope pass an
explicit never-firing token.
- H9 + M19 + L19: CI gains a default-features (no-sqlite) clippy+test job,
an MSRV job, a native Windows check job, a cargo-audit job, and a
concurrency group cancelling superseded runs; the primary clippy gate
now uses --all-targets. Cargo.lock is intentionally gitignored, so
--locked is deliberately not used. Bumped workspace rust-version from
1.86 to 1.88: the declared MSRV was unbuildable (time, zip,
playwright-rs, and vendored brush crates require 1.87/1.88).
- M14 + L15: ProviderError's JsonSchema is now hand-written to match the
hand-written serde wire format exactly (retryable always emitted;
backoff_hint optional); removed the serde attrs the manual impls
ignored; added a schema-matches-wire test.
- M15: SqliteSessionStore serves load/replay/list from a small pool of
read-only WAL connections (round-robin) so reads no longer serialize
behind the single writer; :memory: databases fall back to the writer.
- M16: the bounded-body test now exercises the real
accumulate_response_body_bounded against a genuinely streamed local
HTTP response; deleted the hand-copied accumulator twin.
- M17: tests for the dispatch core using real subprocesses: run_command's
exit-code contract (0 parse / 0-malformed error / 2 block / other
error / signal error / mid-run cancellation) and the extracted pure
handler_run_result status mapping (Stopped/Blocked/Completed/
Cancelled/Failed).
- M20: hook registries store Arc<ConfiguredHandler>, so per-event
dispatch clones pointers instead of compiled regexes and config values.
- M21: ModelSlot has a hand-written Deserialize so a typo'd key in an
inline model table surfaces ModelConfig's field-level error (naming
the key) instead of serde's opaque untagged-enum message.
- L13: derive Default for ResilienceConfig (was the derivable_impls
clippy failure); also fixed three collapsible_if lints newly flagged
by current clippy so the -D warnings gate is fully green.
- L14: delete the vestigial DEFAULT_TEMPERATURE constant.
- L16: cross-field retry-config validation (base backoff vs max backoff,
deadline must admit one backoff) on both the global block and the
effective per-provider merge, with table-driven tests.
- L17: unclosed ${ in hook env expansion now passes through literally
instead of dropping the dollar sign.
- L18: the CLI logs only the parsed command's variant name, not the full
command (which carries the run task text).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mirror upstream brush-core, which removed the nightly-only feature gate and stubbed get_device_and_inode on Windows (NotSupportedOnThisPlatform, with upstream's TODO to implement via file index / volume serial number). Unblocks stable-toolchain compilation on windows-latest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ation Replace the vendored brush fork wholesale with pristine crates.io releases: brush-core 0.5.0 (as halter-brush-core) and brush-builtins 0.2.0 (as halter-brush-builtins), bumping brush-parser to 0.4.0. The old fork's Windows layer, formatting drift, and one-off fixes are all dropped -- upstream 0.5.0 builds for x86_64-pc-windows-msvc on stable natively and ships its own async command-substitution reader. The only functional divergence carried forward is the cancellation plumbing, reimplemented idiomatically on the new base: - ExecutionParameters carries an optional CancellationToken (set_cancel_token is the halter-facing entry point); the interpreter checks it at every Execute impl entry and loop iteration - ChildProcess::wait / ExecutionSpawnResult::wait accept the token and surface ProcessWaitResult::Cancelled (mapped to exit code 130) - tokio_process spawns with kill_on_drop so abandoned children die - builtins: `command` passes the token through; `read` polls input in bounded slices on Unix so blocked reads cancel promptly (with tests) halter-tools adapts to the 0.4->0.5 API changes (Shell::builder, env_mut()/jobs() accessors, run_string SourceInfo argument, ExecutionExitCode::BrokenPipe, brush-parser 0.4 Parser::new and Coprocess AST variant in the shell policy visitors). Divergence inventory and re-vendor procedure documented in vendor/VENDORING.md. Verified: workspace clippy -D warnings clean, 869 tests green, windows-msvc cross-checks of both vendored crates pass, and an end-to-end harness confirms external processes, pure-builtin loops, and piped reads all cancel within ~300ms without leaking children. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Closes #183
Tests