fix(stella-model): land the area:model audit batch (#566)

[macanderson]

Closes #566

Batched cleanup for area:model — ten of the issue's eighteen items, each landed with a witness test verified to fail on the old code (git stash-style: production line reverted in place, test run, restored).

What changed

Behavior changes (each with a witness)

anthropic.rs — every system turn survives the hoist. to_anthropic_messages did system = Some(message.content.clone()), so a conversation carrying more than one system turn (a skill preamble plus an injected policy block) silently lost every earlier one — on Anthropic alone, with no error and no log line. Now accumulates into a Vec<String> and joins with a blank line, exactly as to_openai_input and to_gemini_request_parts already do.
Prompt-cache note: for any caller that sends more than one system turn, the cached system prefix bytes change once, on the first turn after this lands. That is the price of not dropping instructions.
Witness: every_system_turn_survives_the_hoist_joined_by_a_blank_line (plus no_system_turn_yields_no_system_prompt, pinning that "no system turn" still means no field on the wire).

anthropic.rs / openai.rs / zai.rs — pricing is resolved scoped to the provider. All three constructors used the unscoped Catalog::resolve, whose documented semantics are "when the same slug exists on several providers, the first row wins". Harmless against the seed, but after stella models refresh merges the models.dev master list the same slug legitimately appears under several providers, and an Anthropic turn could be costed at OpenRouter's list price. Now resolve_for("anthropic"|"openai"|"zai", &model), matching gemini/vertex/bedrock and zai's own with_identity re-resolve.
Witnesses: pricing_is_scoped_to_anthropic_and_never_adopts_another_providers_row, pricing_is_scoped_to_openai_and_never_adopts_another_providers_row, new_resolves_pricing_scoped_to_the_zai_identity.

openai.rs — finish_reason is mapped, not hard-coded None. Some(FinishReason::ToolCalls) when the turn produced tool calls, Some(FinishReason::Stop) otherwise, mirroring zai.rs. Length stays unreachable: response.incomplete already aborts the turn with a typed error rather than returning a truncated Ok (pinned by the existing complete_returns_err_on_response_incomplete_not_truncated_ok). Nothing previously asserted None, so no existing test needed editing.
Witness: complete_maps_finish_reason_to_stop_and_tool_calls.

bedrock.rs — the SigV4 signer no longer slices a caller-supplied string. let date = &input.amz_date[..8] panics on a value shorter than 8 bytes or whose byte 8 is not a char boundary; SigningInput is pub(crate) with amz_date documented as "fixed in tests", so the invariant lived only in prose. Now get(..8).unwrap_or(input.amz_date). Both botocore golden vectors still pass byte-for-byte — well-formed dates are unaffected.
Witness: sign_does_not_panic_on_a_malformed_amz_date.

credential.rs — CredentialsFile gets a redacting Debug. It derived Debug over a BTreeMap<String, String> of plaintext provider keys, so any {:?} dumped every secret in the file. Hand-written impl printing path, the provider count, and "<redacted>"; Debug dropped from CredentialsFileData (it keeps Serialize/Deserialize for the TOML round-trip). Mirrors stella-media/src/credential.rs. Nothing in the workspace formats either type with {:?}.
Witness: debug_never_prints_a_stored_key.

openai.rs — prompt_cache_key gets the process-wide sequence suffix. It was stella-<pid>-<nanos>, the exact construction zai.rs documents as insufficient and fixes with a SESSION_SEQ: AtomicU64. Siblings built inside one nanosecond shared a cache-routing key, undoing the shard separation the field exists to provide. Same four-line atomic, now stella-<pid>-<nanos>-<seq>.
Witness: siblings_minted_in_the_same_nanosecond_get_distinct_prompt_cache_keys — the formatting half is split from the clock read (prompt_cache_key_at) precisely so the test can hand it the same instant twice. A loop over real constructions is not a witness here: the clock is fine-grained enough that 64 back-to-back builds never collided on the old code, which is why the deterministic form exists. distinct_provider_constructions_get_distinct_prompt_cache_keys remains as the end-to-end pin.

provider_listing.rs — pagination cursors are percent-encoded. format!("{base}/v1/models?limit=1000&after_id={id}") and the Gemini pageToken twin interpolated provider-controlled opaque tokens raw; a value containing &, #, or + truncates the query, injects a parameter, or decodes wrong — and the failure mode is a wrong or empty page, not an error. Both now build through reqwest::Url::parse + query_pairs_mut().append_pair(...). get_json's &str signature is unchanged.
Witness: fetch_anthropic_percent_encodes_a_cursor_carrying_query_syntax (cursor claude-a&limit=1#x). The existing fetch_anthropic_paginates_with_after_id_and_sends_auth_headers still passes unchanged.

provider_listing.rs — exhausting MAX_PAGES is an error, not a quiet stop. fetch_anthropic and fetch_gemini broke out of the for and returned the partial list as success, so a provider legitimately serving more than 10 pages produced a silently truncated catalog and a model picker missing rows. Both now track whether the loop ended on next == None and return Err(String) naming the cap otherwise. This is a live-path behavior change the issue's Fix text asks for explicitly.
Witness: pagination_that_never_terminates_errors_instead_of_truncating.

Non-behavioral

attachment.rs — no more decode-then-re-encode. resolve_one decoded an inline AttachmentSource::Data payload and then immediately BASE64.encoded the same bytes back for every binary kind. The decode stays as validation; the caller's original string is now threaded through to the Image/Pdf/Audio/Video arms, with BASE64.encode(&bytes) kept as the fallback for the Path source. bytes is still needed for the Text arm's inline_text.
No witness: the STANDARD engine requires canonical padding and rejects both whitespace and trailing bits, so anything that decodes re-encodes to the identical string — this is a pure allocation/CPU saving with no observable output change. inline_data_source_passes_through_decoded already pins the value.

zai.rs / zai/tests.rs — s/behaviour/behavior/ at the two sites the issue names (both inside the same grok-4 reasoning-passthrough doc comment). The behaviour occurrences in docs/design/ are outside this issue's scope and were left alone.

Deferred

Eight items are untouched. Each is deferred for a reason the issue itself states, not for convenience.

• attachment.rs:78 — blocking std::fs::read in resolve_one. The issue files it under its own no-async-change rule. Making wire_parts async is cross-cutting across five adapter call sites and their request-build paths, and the ingest-side Path→Data hydration lives outside stella-model entirely. Not verifiable by any local test.
• credential.rs:236 — warn or refuse on a group/world-readable credentials.toml. Needs a new advisory channel on a public API (a new CredentialError variant or a returned advisory) plus a refuse-vs-warn-vs-chmod decision the issue explicitly assigns to the owner.
• credential.rs:63 — zeroize ApiKey on drop. Requires a new workspace dependency (zeroize/secrecy), which the issue puts out of scope and AGENTS.md gates behind a licensing / cargo deny decision. Doing only the provider_listing.rs:370 bearer_auth half would leave the stated threat model unchanged.
• openai.rs:485 / gemini.rs:572 / vertex.rs:95 / bedrock.rs:548 — implement Provider::complete_observed. Confirmed (only anthropic.rs and zai.rs implement it), but the fix rewrites aggregate_openai_stream and aggregate_gemini_stream signatures and changes concurrency-visible behavior on every turn for four adapters; Bedrock cannot participate until it streams. Too large and too behavior-central for a cleanup batch.
• cache_economics.rs:186 — the CacheWarmth / is_cache_expired_rewrite TTL-boundary disagreement. Both behaviors are explicitly test-pinned (warmth_counts_down_and_saturates_to_expired, expired_rewrite_needs_both_a_cold_gap_and_an_actual_write); the issue's own Fix says picking one contract is a product decision.
• sse.rs:91 — MAX_BUFFERED_EVENT_BYTES + a consumed cursor. Deferred by the Fix text as a change to the decoder's error surface, which every adapter maps onto its own ProviderError classification. Choosing the cap value and the new terminal error is a design call with workspace-wide reach.
• zai.rs:492 / anthropic.rs:345 / openai.rs:236 / bedrock.rs:301 — the unreachable!() WirePart arms. Each arm sits behind a *_CAPS const fixed in the same file, so no test can reach them without editing the caps — there is no witness that fails on the old code. It also requires inventing four adapter-specific degrade wordings.
• zai.rs:34 — rename to chat_completions/ChatCompletionsProvider and replace the self.id string gates with a DialectQuirks struct. The Fix text calls it "a public-API rename touching stella-cli — squarely out of an audit's remit". It would also collide with this PR's zai.rs edits.
• An explicit pricing fallback for a slug that exists in the catalog only under a different provider id (the follow-on to the resolve_for change). The issue marks this sub-item deferred to the owner: choosing between None/0.0, a cross-provider fallback, or a hard error is a metering-policy decision. Today that case resolves to None → cost_usd 0.0, which the three new scoping witnesses pin.

Issue checklist

• anthropic.rs:507 — only the last system message reaches the wire
• anthropic.rs:36 — unscoped Catalog::resolve for pricing
• attachment.rs:78 — blocking std::fs::read on the async request-build path
• credential.rs:236 — no permission check on credentials.toml
• credential.rs:63 — ApiKey not zeroized on drop
• openai.rs:485 — four adapters inherit the silent complete_observed default
• openai.rs:568 — finish_reason hard-coded None
• attachment.rs:90 — decode-then-re-encode of inline base64
• bedrock.rs:901 — panicking amz_date[..8] slice
• cache_economics.rs:186 — TTL-boundary disagreement
• credential.rs:215 — derived Debug over plaintext keys
• openai.rs:63 — colliding prompt_cache_key
• provider_listing.rs:226 — raw cursor interpolation into query strings
• provider_listing.rs:87 — MAX_PAGES exhaustion indistinguishable from a clean finish
• sse.rs:91 — unbounded SseDecoder buffer + quadratic drain
• zai.rs:492 — unreachable!() WirePart arms
• zai.rs:34 — vendor-named shared adapter + string-equality identity gates
• zai.rs:351 — behaviour → behavior

Verification

From the worktree:

cargo fmt --check                                        # clean
cargo clippy --workspace --all-targets -- -D warnings    # clean
cargo test -p stella-model                               # 250 + 1 + 13 passed, 0 failed
cargo test --workspace                                   # 58 suites, 0 failed
make no-scratch                                          # OK

Workspace-wide clippy and tests were run (not just stella-model) because dropping Debug from CredentialsFile is visible to stella-cli. No pre-existing failures were encountered.

Files also being touched by #557

stella-model/src/credential.rs and stella-model/src/bedrock.rs are shared with #557. The edits here are deliberately narrow: one struct's Debug (plus one test) in credential.rs, and one line in sigv4::sign (plus one test) in bedrock.rs.

[sourcery-ai]

Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
stella-cli-docs	Ready	Preview, Comment	Jul 25, 2026 7:51pm

[sourcery-ai]

Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery