Skip to content

Cache relevance scores for unchanged tool results - #36

Closed
j-ctang wants to merge 1 commit into
stagingfrom
feat/32-relevance-cache
Closed

j-ctang wants to merge 1 commit into
stagingfrom
feat/32-relevance-cache

Conversation

@j-ctang

@j-ctang j-ctang commented Oct 14, 2026 •

Copy link
Copy Markdown
Owner

Closes #32
Risk: risk:feature

Summary

  • Cache scores in JevRelevanceScorer keyed by goal and tool_use_id.
  • Send only uncached candidates to TypeSafe; cap the cache at 500 entries.
  • Tests for reuse, goal change, and the size bound. npm run check passes.

Commit History

  • Cache relevance scores by goal and tool

Sensitive Changes

None.

Rollback

git revert <merge-sha>

Summary by CodeRabbit

  • Performance
    • Relevance scoring now reuses previous results for the same goal and candidate, reducing repeated scoring work. Results remain distinct across different goals, and newly calculated results are cached up to a limit of 500 entries.

Signed-off-by: j-ctang <272994374+j-ctang@users.noreply.github.com>
@j-ctang j-ctang added the risk:feature Other changes; human review required; no landing authority label Oct 14, 2026
@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

📝 Walkthrough

JevRelevanceScorer caches scores by goal and tool-use ID. It requests scores only for uncached candidates and stores new scores while the cache has fewer than 500 entries. Tests cover repeated candidates, changed goals, mixed cached and uncached candidates, and the cache size bound.

Changes: Score cache

Layer / File(s) Summary
Cache lookup and score validation
src/services/relevanceScorer.ts, test/jevService.test.ts
The scorer reuses scores for matching goals and tool-use IDs, and requests scores only for uncached candidates. Tests cover repeated candidates, changed goals, mixed cached and uncached candidates, and a cache size bound of 500 entries.

Priority: ⬇️ Low

Merge Risk: 🟡 Moderate · up to fb2ba

After the cache fills, newer tool results cannot benefit from score reuse. Add oldest-entry eviction before merging.

Security architecture risk: 🟡 Moderate · up to fb2ba

Scores can be reused for different tool results when their goals and identifiers match. This can let one request influence another request’s pruning decisions. The default listener is local-only, and cached values are only numeric scores, which limits exposure.

Retained concerns

  • Medium · security · inferred: The shared cache identifies scores only by goal and tool-use ID, allowing different candidate content or another conversation to inherit an earlier score. A local caller able to seed a matching key can influence subsequent eligible pruning without evaluation of the current content. Unlike the base scorer, this bypasses fresh scoring after content-aware decision-memory invalidation; an incorrect drop can then be remembered persistently.
Security review details

Security Blast Radius

  • inferred — The affected ownership boundary is between requests and conversations handled by one proxy process. Cross-conversation influence requires matching goals and tool-use IDs and an eligible pruning operation. The inspected path establishes pruning-integrity impact, not remote reachability, credential exposure or privilege escalation.

Security Findings and Attack Paths

  • inferred — A caller with local proxy access and knowledge of a target goal/ID pair can submit different candidate content under that pair to seed an admitted score. A later matching candidate can inherit that score without fresh evaluation, changing its drop/keep outcome if the score crosses the cutoff. The resulting prepared request is forwarded upstream. This is a conditional integrity attack path, not demonstrated data disclosure or tool execution.

Trust Boundaries and Controls

  • observed — The application mounts the proxy without an application authentication step, although the production listener is loopback-only. Candidate extraction rejects duplicate or unmatched IDs within a request, but does not establish cross-request identity. Existing decision memory fingerprints candidate content; the new cache uses a weaker identity. The listener and existing memory behavior predate this PR.

Resilience and Maintainability Implications

  • observed — The production asker rejects failed HTTP responses and invalid or missing answers before returning scores, preventing those failed batches from populating the cache. Pruning failures return the current request through the existing fail-open path. These controls contain evaluation failures but do not detect a valid numeric score reused for the wrong candidate.

Hardening Proposals

  • proposed — Bind cache reuse to the evaluated candidate content and intended conversation ownership, and provide an explicit invalidation or bypass mechanism for full rescoring. Recovery should also account for persisted pruning decisions derived from stale scores.
Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Linked Issues check Warning Issue #32 requires a bounded cache that evicts the oldest entry when it reaches 500 entries and caches the new score. JevRelevanceScorer.score uses if (this.cache.size < 500) before set, so a fu… When the cache has 500 entries, remove its oldest entry before storing each new score. Add or retain a test that fills the cache, verifies the oldest entry is evicted, and verifies the new and recent entries remain cached.
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: caching relevance scores for unchanged tool results.
Description check Passed The description includes the issue reference, risk, summary, commit history, sensitive changes, rollback steps, test status, and key implementation details. It omits the optional Before/After section,…
Out of Scope Changes check Passed The reviewed changes are limited to src/services/relevanceScorer.ts and related tests in test/jevService.test.ts. The scorer and tests implement the caching behavior requested by issue #32. No unr…
Full details: Linked Issues check

Explanation

Issue #32 requires a bounded cache that evicts the oldest entry when it reaches 500 entries and caches the new score. JevRelevanceScorer.score uses if (this.cache.size &lt; 500) before set, so a full cache never evicts and never stores later scores. The implementation does include goal text and toolUseId in the key, reuses cached scores, and excludes cached candidates from TypeSafe requests. The reviewed tests cover cache behavior, but the implementation does not meet the eviction requirement.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed due to a network error.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository: j-ctang/claude-code-jev-prune/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 71944c34-e860-47a3-ade7-e2d1d3be9e8a
📥 Commits

Reviewing files that changed from the base of the PR and between d183c05 and fb2baad.

📒 Files selected for processing: 2
  • src/services/relevanceScorer.ts
  • test/jevService.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

Comment on lines +178 to +180
if (this.cache.size < 500) {
this.cache.set(JSON.stringify([goal, toolUseId]), score);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Evict the oldest score before inserting a new one.

After 500 distinct goal and tool-use ID pairs, this condition rejects every new score. Later calls for those pairs ask TypeSafe again. When the cache is full, delete its oldest entry and insert the new score. Extend the size-bound test to confirm that a recent entry is reused after eviction. citeturn0search0

@j-ctang

j-ctang commented Oct 14, 2026

Copy link
Copy Markdown
Owner Author

Verdict: Changes needed

Review tier: S

  • blocker src/services/relevanceScorer.ts:35: I see no eviction. Once the cache has 500 entries, I skip the insert, so new scores are never cached. The issue says to evict the oldest entry so new scores are still cached. When size is 500 or more, delete the first key from the Map (this.cache.keys().next().value) before I set the new entry. Map keeps insertion order, so this evicts the oldest.
  • blocker test/jevService.test.ts:84: The size test only checks size <= 500. The issue requires a test that recent entries survive eviction, and I see none. I add a test that scores more than 500 candidates, then re-scores the most recent ones and asserts no new requests. I also assert that the oldest entry is asked again.

Follow-ups (outside this PR):

  • I see no test that scoring with an empty uncached list sends no requests and does not call the usage reporter.

@j-ctang j-ctang added the planted-bug Seeded-defect PR for reviewer evaluation; never merge label Oct 15, 2026
@j-ctang

j-ctang commented Oct 15, 2026

Copy link
Copy Markdown
Owner Author

Reviewer evaluation PR. Closing without merge.

@j-ctang j-ctang closed this Oct 15, 2026
@j-ctang
j-ctang deleted the feat/32-relevance-cache branch October 15, 2026 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

planted-bug Seeded-defect PR for reviewer evaluation; never merge risk:feature Other changes; human review required; no landing authority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant