Repository navigation
Rubrics on every path, a receipt, and a retry policy that matches the docs - #8
Merged
Merged
Conversation
The runtime constructors took a plain string, so a choice or a score built from runtime options could not carry examples and the cross-option rules the derives enforce had nowhere to run. Both now take `IntoRubric`, `Choose` and `Score` hold `Rubric` values, and the checks that need more than one rubric in view run when the question is asked: an example shared by two options or two levels, and a counterexample on a level. A declaration is legal under the derive and at runtime, or under neither. The derive is untouched and `choose` wraps its already-rendered strings in a `Rubric` that carries no parts, so every existing declaration puts the same bytes on the wire.
Three gaps in the retry policy, all measured against the live docs. A `429` on a startup `models()` call failed app boot, because that endpoint was not retried at all. A refused connection, a reset or a TLS handshake failure on the first attempt failed the whole batch, although the request reached nobody and is safe to send again. And a `529` dropped the `retry-after` the client had already parsed, while a `429` kept it. The two endpoints now drive one retry loop, so the budget, the backoff and the span shape cannot drift apart. A read timeout and a body failure stay non-retryable: the request reached a server, and resending would double the wall time the timeout promises. `Error::Overloaded` gains `retry_after`, which is breaking.
Three things a caller could not reach. The response's `model` and `usage` were parsed, written to the span and dropped, so cost attribution or pinning a threshold to the version that answered meant running an OpenTelemetry pipeline or reimplementing the policy layer; `ask_with_receipt` returns them beside the answer and `ask` is that with the receipt dropped. The reqwest client was built inside `ClientBuilder` and nothing could replace it, so a proxy, a client certificate or a network-free test was impossible; `http(..)` takes one and `GuideBuilder::client(..)` takes the whole client. And `from_env` was only the one-liner, so reading the environment and then setting a house policy meant repeating the three variable names. Every setting that lives on an injected client is refused by name when it is also set on the guide builder, and a timeout beside `http(..)` likewise: a setting that silently does nothing is the failure mode these guard against. `reqwest` joins dev-dependencies so the tests can build the client that `http(..)` takes. It is already a dependency of the library, so the tree is unchanged.
The contract's §3 said Rust's runtime constructors take a plain string and that widening them was deferred; both are now false, and §4 gains the receipt and the retry policy the two SDKs must agree on. design.md records the sync-Rust rejection and notes that one of the three reasons for flattening rubric examples lapsed while the two measured ones stand. The test ceiling goes from 30 to 36. This release added six behaviours nothing else could pin, and the reason is written where the ceiling is. `spec/` regenerates byte-identically: neither the wire nor the rendering moved. `examples/otlp/Cargo.lock` is refreshed because it pins the library by path and CI builds it with --locked.
Six places claimed a connect timeout is retried. None is. The client sets one overall deadline per attempt and never a connect_timeout, and under that reqwest classifies a connect-phase timeout as is_timeout(), not is_connect() — which is the predicate the retry loop uses. So the claim was false everywhere it appeared. Setting connect_timeout to make it true would buy a retry nobody asked for and multiply the wall time the timeout setting promises. The claim is corrected instead: a connection failure is refused, reset or a TLS handshake; a timeout of any phase is not retried. A client handed in through http(..) brings its own classification, which is now said out loud because that is the one way a connect timeout becomes retryable here. Python is changing to match; the shared wording is the revised retry-policy bullet in the 0.2.0 deltas.
api::ClientBuilder::http takes a reqwest::Client and nothing exported one. A caller had to add reqwest themselves and keep it on the same major, and getting that wrong produces a type error naming neither the cause nor the fix, because two majors in a tree are two unrelated Client types. api::reqwest is the one guideme links, so the injection path is guideme::api::reqwest::Client with no dependency and no version to track. The cost is real and is now written down in both places that govern it: a reqwest major bump is a breaking change for guideme, under Releasing in AGENTS.md, and the trade against a transport trait of our own is under Decisions in docs/design.md. The layout invariant says client.rs is the only file that may use reqwest's API and api/mod.rs only re-exports the crate.
The layout row said every rule the derives enforce is enforced again at runtime. Three are not and cannot be: at least two variants, unit variants only, one fallback. Those are about the declaration, and a runtime constructor has no declaration to apply them to. Scoped to the rubric rules, which is the set docs/contract.md and rubric.rs already describe, with the derive-only rules named so the gap reads as deliberate rather than missed.
The five settings an injected client already carries were hand-listed twice in build(): once to refuse them, once to apply them. A sixth setter would have been added to one list and forgotten in the other, and the failure mode is the exact one the refusal exists to prevent — a setting that silently does nothing. They move into a private Transport that both halves destructure exhaustively, so a sixth field is a compile error in both places until someone decides what it means beside an injected client. The refusal now also names from_env(), which sets api_key and sometimes base_url without the caller writing either, so the message pointed at settings they never typed. client()'s docs say the same and say where the key goes instead.
The response is the API's to grow, and what it grows that a caller wants is what Receipt is for. Without the attribute, adding a field later is a breaking change for anyone who wrote a struct literal or an exhaustive pattern; with it, reading fields keeps working and construction stays ours. Cheaper now than at the first field we want to add.
"When a response arrived" is not the condition. A 200 whose body fails to read or decode is a response that arrived and is marked `transport` or `protocol`, never `200`. The condition is a non-200 status, which is what the code does and what a dashboard filtering on it needs to know. Both statements of the rule, the field table and the error list, now say the same thing.
Nothing built the front page, so every block was prose that happened to look like Rust. include_str! into a cfg(doctest) item puts all thirteen under cargo test --doc, which the gate already runs, and costs no nextest entry. Making them compile changed them. The fragments that referenced an undefined guide, ticket or helper are now the functions a reader would actually write, which says where the async boundary is instead of hiding it; invented callables became comments. Blocks that would open a connection are no_run: they compile, they do not dial. The two shell blocks were untagged, which rustdoc reads as Rust, and are now sh. tracing-subscriber gains env-filter and fmt so the one-line subscriber the README recommends is a line that is known to work. It is a dev-dependency feature; the shipped crate is unaffected.
Both came out of review and both are public surface, so the release notes have to carry them: api::reqwest and the breaking-change consequence it creates for a reqwest major bump, and Receipt being non_exhaustive.
Every other statement of the retry rule says the classification belongs to whatever client is handed in; this one said flatly that a timeout never produces a retry event. A client built with connect_timeout makes a connect timeout is_connect(), so it is retried and does emit one.
The wiremock example was a complete test that silently assumed a dependency the reader does not have. Names the version guideme's own dev-deps pin. The batching example's hidden setup imported Levels and never used it.
The dev-dependency on reqwest contradicted the re-export shipped in the same release: the README tells callers to reach the client through guideme::api::reqwest, and the one test exercising that path reached it another way, so the advertised path was never compiled. It now uses the re-export and the dev-dependency is gone. The tree is unchanged either way; what changes is that the documented path is the tested one.
cargo package materialises the README at the package root, because the
manifest pointed one directory up. The packaged src/lib.rs therefore sat
below it and include_str!("../../README.md") resolved outside the crate, so
cargo test --doc on published sources failed with "couldn't read
src/../../README.md". Build and docs.rs were unaffected, which is why
nothing caught it here.
guideme/README.md is now a symlink to the repository README and the
manifest points at it, so the worktree has the same shape the package
does and one path works in both. cargo package dereferences the symlink
and still ships README.md at the root.
Verified against a copy of the packaged tree: the old path fails there and
the new one passes all fourteen doctests. AGENTS.md says not to replace the
symlink with a copy, and names the check.
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.
0.2.0. Two breaking changes, both in the validated proposal, and the additions that go with
them.
spec/regenerates byte-identically: neither the wire nor the rubric rendering moved.Breaking
choose_amongtakes(K: Into<String>, Option<R: IntoRubric>);score_levelstakesR: IntoRubric. The handoff's sealedIntoOptionalRubricdesign does not compile —coherence cannot prove
Option<T>is neverInto<String>— so the item type carries theOptioninstead. A call passingSome("text"), ownedStrings, aCow, or pairs from ahelper generic over
Into<String>still compiles. The one shape that changes is a listwhere every option is bare:
[("a", None::<&str>), ("b", None)].Error::Overloaded { retry_after: Option<Duration> }, where it was a unit variant. A529parsed the header and dropped it while a
429kept it.What a caller could not do before
so the cross-option rules the derives enforce had nowhere to run. Both runtime constructors
now take a
Rubric, and the rules that need more than one rubric in view — an example sharedby two options or two levels, a counterexample on a level — are
Error::Configwhen thequestion is asked. A declaration is legal under the derive and at runtime, or under neither.
docs/contract.md§3 loses the paragraph that said Rust could not do this.ask_with_receiptreturnsReceipt<T> { answer, model, usage }.Both fields were parsed, written to the span and dropped, so cost attribution meant running
an OpenTelemetry pipeline.
askis the same call with the receipt dropped.api::ClientBuilder::http(reqwest::Client)andGuideBuilder::client(api::Client). Every setting an injected client already carries isrefused by name when it is also set on the guide builder, and a
timeoutbesidehttp(..)likewise.GuideBuilder::from_env(), soGuide::builder().from_env()?.policy(HOUSE).build()?works.Guide::from_env()stays anddelegates.
GuideBuilder::backoff(Duration), which the client had and the guide did not.Receipt,ModelInfoandUsageat the crate root.Retry policy
GET /v1/modelsis retried on429/529. It was not retried at all, so a throttle on astartup
models()call failed the boot — which the API docs say the SDKs handle. Bothendpoints now drive one loop, so the budget, the backoff and the span shape cannot drift.
timeout. The request reached nobody, so sending it again is safe. A read timeout and a body
failure are still not retried — those reached a server.
guideme.retrycarrieserror.type = "transport"and nohttp.response.status_codefor those. Exactly one of the two is on every retry event. Telemetry contract change;
docs/observability.mdhas the table.Tests
35 entries, against a ceiling
AGENTS.mdraises from 30 to 36 in this PR with the reasonwritten beside it. Six new runtime entries, each in an existing category: the receipt and the
injected client through
wiremock, the models retry and the529retry-afterthroughwiremock, the two cross-option rules through the rendered request body, and the connectionretry through a capturing
Layer— which pins the attempt count exactly rather than byelapsed time, and pins the new event field set at the same time.
Two compile-time blocks, no runtime cost: nine caller shapes for the new signatures, and a
Sendpin onGuide::askfutures so anRcreaching one cannot break everytokio::spawncaller on a patch upgrade.
Follow-up
spec/SOURCEinguideme-pythonneeds the merge commit of this PR; the drift job there iswhat will say so.