Repository navigation
refactor(llm)!: remove stale duplicate of the harness response cache - #65
Conversation
The cache module now only carries the per-request CachePolicy and the canonical_value JSON helper, since the response cache, its keys and the prompt-layout guard live in the host harness. canonical_value is exported so host-derived cache keys canonicalize identically, and the sha2 dependency is dropped along with the removed hashing code. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Document the breaking removal of the duplicated harness response cache from tinyinference_llm::cache, since the maintained versions live in tinyagents-harness and nothing used the stale copies. Also note that canonical_value is now public and the crate no longer depends on sha2. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Remove the sha2 entry from the dependency list in Cargo.lock, reflecting that the crate is no longer a direct dependency. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
💤 Files with no reviewable changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe cache module retains ChangesCache API surface
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~12 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The cache API removal has no established merge-blocking issue; it is ready for normal merge checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The retained request-policy behavior is unchanged, and no new security bypass was established. Residual risk concerns downstream compatibility and cache enforcement, which could not be independently verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
I’m a rabbit with a policy to keep, Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52278b0e37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| //! cache markers; the response cache itself, its keys and the prompt-layout | ||
| //! guard live in the host harness (`tinyagents-harness`'s `cache` module). |
There was a problem hiding this comment.
When a consumer other than TinyAgents needs response caching, moving ResponseCache, cache-key construction, and the default in-memory implementation exclusively into tinyagents-harness forces that consumer either to depend on an agent runtime or to duplicate the contract. The repository boundary explicitly assigns provider-neutral cache APIs to TinyInference while reserving agent/runtime concerns for consumers, so retain the neutral trait and key API here while leaving harness-specific SQLite and singleflight implementations downstream.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 2 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Ready for maintainer review Review snapshot
Completeness: Complete What changedThe review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below. FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. Findings
Before mergeNone. How this fits togetherflowchart LR
n0["with_capacity"]:::impacted
n1["new"]:::impacted
n2["put"]:::impacted
n3["cache_key"]:::impacted
n4["response_cache_capacity_zero_retains_last"]:::impacted
n0 -->|calls| n1
n1 -->|calls| n0
n3 -->|calls| n1
n4 -->|calls| n0
n4 -->|tests| n0
n4 -->|calls| n2
n4 -->|tests| n2
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0028 · 152,670 in / 7,891 out · 13,443 cached (9%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0014 · 68,391 in / 4,683 out · 6,349 cached (9%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0012 · 45,424 in / 1,606 out · 3,574 cached (8%) · gpt-5.6-luna
tests: $0.0001 · 12,959 in / 156 out · 1,856 cached (14%) · glm-5.3-flash
description: $0.0001 · 13,433 in / 196 out · 1,536 cached (11%) · glm-5.3-flash
| } | ||
| /// Both flags default to `false` (no caching / no protection) so a host is | ||
| /// safe-by-default and opts must be explicit. | ||
| #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] |
There was a problem hiding this comment.
Apply defaults when deserializing incomplete policies
Although CachePolicy documents both flags as defaulting to false and derives Default, serde does not use that implementation for missing fields. Deserializing a valid partial policy such as {} or {"ttl_ms": 1000} therefore fails with a missing-field error. Add a serde default for the struct so omitted fields receive the documented defaults.
[RULE] serde-defaults ·
Keep the trimmed cache module: upstream's side of the conflict only touched code this branch removes, and canonical_value is already pub. Co-authored-by: Medulla <medulla@tinyhumans.ai>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0055 · 129,118 in / 9,466 out · 12,158 cached (9%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0026 · 51,248 in / 3,291 out · 6,605 cached (13%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0025 · 37,752 in / 3,957 out · 5,361 cached (14%) · gpt-5.6-luna
tests: $0.0001 · 13,313 in / 795 out · 64 cached (0%) · glm-5.3-flash
description: $0.0001 · 13,911 in / 668 out · 64 cached (0%) · glm-5.3-flash
| pub struct CachePolicy { | ||
| /// When `true`, the host may look up (and write) local response cache | ||
| /// entries before calling the provider. | ||
| pub response_cache_enabled: bool, |
There was a problem hiding this comment.
Apply defaults when deserializing incomplete policies
The policy documents both flags as defaulting to false, but neither field has #[serde(default)]. Deserializing a persisted or externally supplied policy that omits either flag therefore fails instead of applying the documented safe default. Add #[serde(default)] to both boolean fields.
Additional critique observation
Apply defaults when deserializing incomplete policies
[RULE] serde-defaults
The boolean fields do not have #[serde(default)], so deserializing a policy that omits either flag fails instead of using the documented safe defaults. For example, serde_json::from_value::<CachePolicy>(serde_json::json!({"ttl_ms": 1000})) returns a missing-field error for response_cache_enabled. Add serde defaults to both boolean fields so persisted or externally supplied policies containing only optional settings remain compatible.
Suggested change for this observation (reference only)
#[serde(default)]
pub response_cache_enabled: bool,
/// When `true`, middleware must preserve the order and content of cacheable
/// prefix segments, and providers that support it mark the stable prefix
/// for their own prompt cache.
#[serde(default)]
pub protect_prompt_prefix: bool,
[RULE] serde-default ·
| } | ||
| /// Sets the entry TTL. | ||
| pub fn with_ttl(mut self, ttl: std::time::Duration) -> Self { | ||
| self.ttl_ms = Some(ttl.as_millis() as u64); |
There was a problem hiding this comment.
Handle durations larger than the TTL field can represent
A Duration can contain more milliseconds than u64 can represent, while ttl_ms cannot. The cast silently truncates the u128 millisecond count, so a very large requested TTL is stored as a different, much smaller TTL. Reject unrepresentable durations or otherwise define and test an explicit saturation policy instead of silently changing the value.
Additional critique observation
Handle durations larger than the TTL field can represent
[RULE] lossy-integer-conversion
Duration::as_millis() returns u128, but this cast stores only u64. A duration larger than u64::MAX milliseconds is silently saturated/reduced to the maximum representable TTL, so with_ttl no longer preserves the requested expiry and entries can expire far earlier than requested. Either widen ttl_ms to a type that can represent all accepted Duration values or reject/handle unrepresentable durations explicitly.
Additional tests observation
Handle durations larger than the TTL field can represent
[RULE] lossy-duration-cast
with_ttl stores ttl.as_millis() as u64; a Duration whose millisecond count exceeds u64::MAX silently truncates (in practice as_millis itself saturates above ~584 million years, but the as u64 cast is lossy for the full u128 range), producing a wrong TTL rather than an error or saturation. Unchanged since first raised.
Suggested change for this observation (reference only)
self.ttl_ms = Some(u64::try_from(ttl.as_millis()).unwrap_or(u64::MAX));
[RULE] integer-overflow-conversion ·
| /// | ||
| /// Both flags default to `false` (no caching / no protection) so a host is | ||
| /// safe-by-default and opts must be explicit. | ||
| #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] |
There was a problem hiding this comment.
Apply defaults when deserializing incomplete policies
Deserializing a partial CachePolicy — e.g. {"response_cache_enabled": true} from a config file missing the newer keys — fails outright, because the bool fields have no #[serde(default)]. Only ttl_ms and namespace get defaults. Add field-level defaults so an older or partial policy still parses as safe-by-default. Unchanged since first raised; the removal of surrounding types did not touch this.
[RULE] incomplete-deserialize ·
1 participant
Summary
tinyinference_llm::cachecarried an old copy of the tinyagents harness response cache. Its module doc still opened with "Harness cache module". The harness copy (tinyagents-harness/src/cache/) has since moved on to scoped keys, a SQLite store and singleflight, and it is the one hosts actually use. This PR deletes the stale copy and keepsCachePolicy, whichModelRequest::cache_policyand the Anthropic provider depend on.Removed (about 430 lines of source and 249 lines of tests):
cache_key(&ModelRequest) -> StringResponseCachetrait,InMemoryResponseCache(withDEFAULT_CAPACITY,new,with_capacity) and the crate-privateLruResponseMapPromptCacheLayout(from_request,prefix_ids,fingerprint,is_prefix_stable_against) andCacheLayoutEventhex_digest,fold_canonicalandfnv1a_hex, pluscache/types.rssha2dependency oftinyinference-llm, which nothing else in the crate used (tinyinference-providersstill uses it)Kept, at the same paths:
cache::CachePolicy, with its fields, serde shape,enabled,ttl,with_ttlandwith_namespaceall unchanged. Only the doc comments changed, because they linked to the removed types.cache::canonical_value, which is nowpubwith the same body and doc text as Export replace_text_blocks, canonical_value and the JSON schema validator #64. Export replace_text_blocks, canonical_value and the JSON schema validator #64 makes it public so tinyagents can drop its own copy, and the open tinyagentsdedupe-clonesbranch already importstinyinference_llm::cache::canonical_value. Without this, deletingcache_keywould have left the function as dead code. Whichever of the two PRs merges second will have a small conflict in this file, and the resolution is to keep thepub fn.Related issue
None.
API or behavior changes
This is a breaking change: the public items listed above are removed. No runtime behavior changes.
Before deleting anything, I checked usage with grep across this repo,
libraries/tinyagents(main, its worktrees and its vendored copies), the openhuman superproject (crates/and everyvendor/submodule, recursively), and every otherlibraries/*repo:tinyinference_llm::cache::areCachePolicy(plus intra-doc links toCachePolicy::protect_prompt_prefix) andcanonical_value(the pending Export replace_text_blocks, canonical_value and the JSON schema validator #64 / tinyagentsdedupe-clonespair).cache_key,ResponseCache,InMemoryResponseCache,PromptCacheLayoutorCacheLayoutEventfrom tinyinference. Every hit for those names resolves totinyagents_harness::cache. For example, openhuman-core importstinyagents_harness::cache::{InMemoryResponseCache, CacheLayoutEvent}.tinyagents-harnessre-exports the wholetinyinference_llmcrate, but nothing reaches these items through that re-export.Validation
All commands were run in this branch at stable 1.99.0:
cargo fmt --all -- --check: cleancargo +stable clippy --all-targets --all-features -- -D warnings: cleancargo build --all-targets --all-features: covered by the clippy and test buildscargo test --workspace --all-features: 1019 passed, 0 failedRUSTDOCFLAGS="-D warnings" cargo doc -p tinyinference-llm --no-deps: clean, with no broken intra-doc linksAs a downstream check, I made a throwaway checkout of tinyagents
main(2cc39826) withvendor/tinyinferencepointed at this branch, and it was deleted afterwards:cargo check -p tinyagents-harness -p tinyagents-integration-tests --all-targets --all-features: cleancachelib tests passed (57), as didwave2_cache_store,wave2_cache_layoutandwave2_cache_key_scope(7, 11 and 14).wave2_cache_storeexercises theCachePolicybuilders.Tests
The old
cache/mod_tests.rsonly tested the removed items, so I replaced it with four tests:CachePolicydefaultscanonical_valuesorting keys at every depthDocumentation
The module doc now says what the module is for: carrying the per-request policy, while the host harness owns the response cache. I also added a CHANGELOG entry under Unreleased, in a new "Breaking changes" section.
Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the descriptionSummary by CodeRabbit