[napetrov]

Part of #687 (novelty retention cache, High).

Summary

 retention oracle, no adjudicator
   ambiguous band -> placeholder absent
-  memoize and store the verdict
+  return it without memoizing or storing
 _CACHE_VERSION retention-cache-v4 -> v5

Evidence

• Before: test_a_lexical_only_run_does_not_store_a_placeholder_for_a_later_llm_run fails on intel/main: a later adjudicated run replays the placeholder as a final cache verdict.
• After: the test passes. Local make ci-local: every step passes except test / Run tests on tests/test_process.py::test_sighup_kills_the_live_groups_too, a timeout under box load that passes when run alone (1 passed).

Merge Danger

Door: two-way

Cache version bumped to v5: entries already on disk under v4 are not read; they can be deleted.

Blast Radius: low

[Copilot]

🟢 Approval recommended

No active review issues require changes before approval.

No open issues 🎉

What changed in this PR

0 new findings; nothing to fix.

This PR addresses the retention-cache issue in #687 by preventing lexical-only placeholder answers from being reused as final verdicts in later adjudicated runs.

Changes:

• Return ambiguous-band placeholders without memoizing or storing them.
• Bump the retention cache to v5 so older entries are not reused.
• Add regression coverage and document the cache behavior and upgrade impact.

File	Description
dokimi/​tests/​test_novelty_silent_defaults.py	Adds a regression test for lexical-only followed by adjudicated runs.
dokimi/​dokimi/​novelty/​retention.py	Skips caching placeholders and advances the cache version to v5.
dokimi/​docs/​skill-novelty.md	Explains v5 invalidation and legacy placeholder entries.
dokimi/​docs/​skill-novelty-walkthrough.md	Clarifies that ambiguous lexical-only answers are not cached.
dokimi/​changelog.d/​20261113_120000_napetrov_fsm687-retention.md	Records the fix and cache upgrade impact.

_{Review effort: Balanced}

[exai-bot]

✅ OpenCodeReview: Review complete: 0 finding(s) across 5 selected item(s).

CI at 67770ea: 26 passing, 3 skipped.

Review details

• Commit reviewed: 67770ea against merge base f12057b
• Coverage: 5 files in 1 groups
• Engine: OpenCodeReview v1.13.5-exai.4.8f73eada, us.openai.gpt-6.1-sol
• Usage: 74k tokens (34k from cache), 12s
• Tool calls: 6 (5 file_read, 1 local_check_results)
• Novelty retention behavior, tests, and documentation: 5 files

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.72%. Comparing base (f12057b) to head (67770ea).
⚠️ Report is 1 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #692   +/-   ##
=======================================
  Coverage   95.71%   95.72%           
=======================================
  Files         415      415           
  Lines       55346    55349    +3     
  Branches     9514     9515    +1     
=======================================
+ Hits        52977    52982    +5     
+ Misses       1466     1465    -1     
+ Partials      903      902    -1     

Files with missing lines	Coverage Δ
dokimi/dokimi/novelty/retention.py	95.04% <100.00%> (+0.12%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vlad-perevezentsev]

Review at 061436a. The fix is right for the direction it targets: a --lexical-only band answer is no longer memoized or stored, and the v5 bump drops the old placeholders. The reverse direction (a lexical-only run reading stored LLM verdicts) and a test for the v4 entries are in the line comments.

What I ran, at 061436a:

• make ci-local --offline: 18,932 passed, 0 failed, 72 skipped; all checks pass.
• A repro: an adjudicated run stores contradicted for a band pair; a --lexical-only oracle on the same --cache-dir then returns contradicted (method cache), and absent with an empty cache. With the one-line change in the line comment it returns absent, and the novelty tests (1,632) pass.

Checked, no issues

• Stats: the new branch still counts the decision and no escalation, so escalation_rate stays 0 for a lexical-only run.
• The full cache bump: a stored entry has no method, so a placeholder absent cannot be told apart from an adjudicated one; dropping all v4 entries is the safe choice, and the docs say the first run pays again.
• Docs and changelog: both pages and the Fixed fragment match the code; the new test fails on the base (the lexical run writes a file there).

[vchamarthi]

Reviewed head 061436a. Nothing new to fix; 2 threads from vlad-perevezentsev are still open. Stops a --lexical-only run from storing its ambiguous-band placeholder and moves the cache to v5; on the head the new test and the v5 bump hold for the input they cover. Two open threads still stand: a lexical-only run still reads adjudicated cache entries, and no test pins the v5 bump.

Verified

• Placeholder not stored: test_a_lexical_only_run_does_not_store_a_placeholder_for_a_later_llm_run passes on the head and pins that a lexical-only ambiguous answer writes no cache file and a later adjudicated run asks the model.
• Placeholder test on main: The same test fails on main, where the lexical-only run writes a cache file.
• Placeholder test strength: Storing the lexical-only placeholder in the memo and on disk makes the test fail on the assertion that the cache directory is empty.
• V4 entries on disk: A v4 entry holding absent for alpha beta gamma delta / alpha beta epsilon zeta is not read on the head: the adjudicator answers retained, method llm.
• Low band boundary: A lexical score of exactly 0.25 on the head gives absent with method lexical and writes one cache file, as on main.
• Lexical-only stats: A lexical-only ambiguous answer on the head counts one decision and no escalation, so escalation_rate stays 0.
• Merge: The PR merges onto current main with no conflict.
• Full test run: The full test run with main merged in shows no new failure; its three failing tests also fail on main.

Open threads from other reviewers:

• still holds on this head: #692 (comment): --lexical-only with a --cache-dir holding an adjudicated contradicted for alpha beta epsilon zeta against alpha beta gamma delta still returns contradicted (method cache) on the head, where absent is expected.
• still holds on this head: #692 (comment): Changing _CACHE_VERSION from retention-cache-v5 to retention-cache-v4 still passes all 96 cases in tests/test_novelty_silent_defaults.py.

Follow-up, not blocking:

• dokimi/dokimi/novelty/retention.py:106: With contradicted stored under alpha beta gamma delta / alpha beta epsilon zeta by an adjudicated run, a client-less oracle on the same --cache-dir returns contradicted (method cache); expected absent (method lexical) (main has it too).

[xaleryb]

• Ran: head + two commits merged with current main, Python 3.13: full suite, ruff, mypy, bandit -ll, the strict docs benchmark gate, check_growth and check_comment_blocks. All clean.
• r4562863278 (lexical-only run reads adjudicated verdicts from a shared --cache-dir). Input: an adjudicated run stores contradicted for a band pair, then a --lexical-only run uses the same cache dir. Observed: contradicted/cache. Expected: absent/lexical, which is what an empty cache gives.
• r4562863290 (nothing pins the v5 bump). Input: _CACHE_VERSION set back to v4. Observed: the suite passes. Expected: a red test.
• Pushed (061436a..1179367):
  • bbef8ed: a lexical oracle skips the disk-cache load. The test runs the reverse order and also checks that an adjudicated oracle still replays the cache with no model call. It extends the changelog fragment.
  • 1179367: a stored v4 band placeholder is re-adjudicated (method == "llm").
• Left for the author: nothing.

[xaleryb]

Re-checked on 1179367 merged with main: a lexical-only run no longer replays an adjudicated verdict from a shared cache dir, an adjudicated oracle still replays the cache with no model call, and a stored v4 placeholder is re-adjudicated; the full suite, ruff, mypy, bandit, docs check --strict and the growth and comment-block checks pass, and CI is green.

[antonwolfy]

LGTM. The fix is right in both directions: a --lexical-only ambiguous-band answer is neither memoized nor stored, and a lexical oracle no longer reads the disk cache, so its output does not depend on what an adjudicated run left in --cache-dir. One nit inline.

What I ran, on a Linux review host at d19d9df:

• make ci-local: 30 passed, 14 skipped, 19,954 tests, changed-line coverage passes. The two gitleaks steps did not run because the scanner download failed; GitHub's own check scans the PR
• Mutation run on retention.py: 11 mutants against the new tests and tests/test_novelty*.py. Storing the placeholder, removing or inverting the self.client is not None guard on the load, reverting _CACHE_VERSION to v4, and changing the placeholder's verdict, method or escalation count each fail the suite. Three survive: the decisions increment (nit inline), the placeholder's score, and memoizing the placeholder, which changes nothing observable because the memo is per oracle and client is fixed at construction.

Checked, no issues

• Pins: the three new tests cover lexical-then-adjudicated, adjudicated-then-lexical (with a check that an adjudicated oracle still replays with no model call), and a stored v4 placeholder being re-adjudicated.
• Docs: skill-novelty.md, skill-novelty-walkthrough.md and the changelog fragment match the code; no other text names v4 or says a lexical-only run reads the cache.
• Callers: analyze.py and shorten.py only use .verdict(...).retained, so nothing depends on lexical results being memoized.
• Commits: every non-merge commit has a Signed-off-by.

[antonwolfy]

LGTM. I pushed a commit per nit, up to beb9eda, which fix the nit from my review:

• beb9eda: dokimi/tests/test_novelty_silent_defaults.py:369: after the lexical verdict call, assert lexical.stats()["decisions"] == 1; deleting self.decisions += 1 now fails the test.

make ci-local at beb9eda: 29 passed, 3 failed (host), 14 skipped; 19952 tests passed, 2 setup timeouts that pass on rerun.

[antonwolfy]

I approved this at beb9eda, and CI passes. The merge waits for you:

• @vlad-perevezentsev: the open thread on dokimi/dokimi/novelty/retention.py:121 (fix(novelty): a lexical-only run no longer stores a placeholder retention verdict #692 (comment))
• @vlad-perevezentsev: the open thread on dokimi/dokimi/novelty/retention.py:34 (fix(novelty): a lexical-only run no longer stores a placeholder retention verdict #692 (comment))

[napetrov]

Re: comment by xaleryb

Two repros: a lexical run reading an adjudicated verdict, and nothing pinning the v5 bump.
Both fixed: bbef8ed and 1179367, with tests.

[napetrov]

Re: comment by antonwolfy

The merge waits for you: the two open threads on retention.py.
Both are answered in their threads: bbef8ed and 1179367.

[napetrov]

Re: review by vlad-perevezentsev

The open points are in the line comments.
Both are fixed: bbef8ed and 1179367, see the threads.

[napetrov]

Re: review by vchamarthi

2 threads from vlad-perevezentsev are still open.
Both fixed in bbef8ed and 1179367.

[napetrov]

Re: review by antonwolfy

The fix is right in both directions.
Thanks. The two threads are fixed in bbef8ed and 1179367.