Repository navigation
fix(recall): use upvote time for confidence recency (#1278) - #1285
Conversation
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
No other confident findings in the current diff. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@dbdefault Thanks, the fix works. We built 4ad6eb5d and origin/main (8a944380) and rebuilt the search index with the CLI in an isolated HOME. There were two learnings that match the same query, and one of them has a single upvote and no recall:
| Build | Upvote date | Voted doc in the index | Ranking |
|---|---|---|---|
| main | now | confidence 0.42, hotness 0.3 | below the unvoted doc |
| this PR | now | confidence 0.72, hotness 1 | first |
| this PR | 200 days ago | confidence 0.42, hotness 0.3 | below the unvoted doc |
tsc, lint and the related unit tests pass, and so does index-local-votes-1233.
Blocking
[P1 blocking] The description has no real-CLI record, and this change affects recall ranking at runtime (Code Review Rules, first item). One run is enough. For example, upvote a never-recalled learning, rebuild the index, and paste the doc's confidence and hotness from ~/.teamai/search-index.json along with the commands you ran.
Non-blocking
[P2 non-blocking] computeConfidence also drives prune (src/maintenance/prune.ts:38), promote (src/maintenance/promote.ts:26, MIN_CONFIDENCE = 0.90) and --confidence-writeback. Taking the later of the two timestamps can only raise confidence. As a result, a stale learning that already meets the vote thresholds can become promotable after one fresh upvote: 1.0 instead of 0.7. We worked this out from the formula and did not run it. Please mention it in the description. A promote or prune case for a fresh upvote would pin it down.
[P2 non-blocking] Two docs now contradict the change. docs/guide/knowledge.md:102 (and the same line in docs/guide/zh-CN/knowledge.md) and skill-data/core/references/troubleshooting.md:374-375 say upvotes cannot replace the recency factor. After this PR, an upvote does reset recency. These docs need a one-line update in both languages.
[P3 nit] The timestamp comparison is a string compare. That is safe today because every writer uses toISOString().
Note: #1282 (approved, not merged) adds a test at the end of the same describe block in src/__tests__/search-index-local-votes.test.ts. Whichever of the two PRs lands second has to keep both tests. src/utils/search-index.ts merges cleanly.
|
Thanks @SaulMoro for the careful triage and for reproducing it on both builds.
|
|
No confident findings in the specified diff. Resolved: The earlier [P1 blocking] missing real-CLI verification finding is fixed. The PR description now records a build and a representative real-CLI comparison confirming that a recently upvoted, never-recalled document avoids the cold penalty. The verification names an older commit ( |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@dbdefault Thanks. This re-reviews 7dc5701b against our comment at 4ad6eb5d.
- [P1 blocking] Real-CLI record: resolved. The description now has the
recall feedback --positiverun on both builds, and its numbers match ours (0.42/0.3 on main, 0.72/1 on this PR). The record names4ad6eb5, but7dc5701only changes docs, so it still applies. - [P2 non-blocking] promote / prune /
--confidence-writeback: resolved. The description now explains the effect. A dedicated test is optional. - [P2 non-blocking] Docs: resolved.
docs/guide/knowledge.md,docs/guide/zh-CN/knowledge.mdandskill-data/core/references/troubleshooting.mdnow say that recency uses the later of the last recall and the last upvote, and the English and Chinese versions match.
Approving. The order with #1282 is unchanged: whichever PR lands second keeps both tests in search-index-local-votes.test.ts.
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@dbdefault CI is red on 7dc5701b. Every Lint & Test job and the Windows job fail on windows-test-list.test.ts and codex-team-hooks.test.ts. Those failures came from main, not from this PR, and #1287 fixed them in c15fc0db.
Please rebase or merge current main so CI runs again. The code review from the earlier approval still holds, and we'll approve again once CI is green. If #1282 lands first, keep both tests in search-index-local-votes.test.ts, as you planned.
|
No confident findings in the specified diff. Resolved: The earlier [P1 blocking] missing real-CLI verification finding is fixed. The PR description records a build and a representative real-CLI comparison demonstrating that a recently upvoted, never-recalled document avoids the cold penalty. The verification names an older commit ( |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@dbdefault #1282 has landed (c4a61fc3), so this branch now conflicts with main in src/__tests__/search-index-local-votes.test.ts. Both PRs add a test at the end of the same describe block. Please rebase onto current main and keep both tests: #1282's keeps unrelated same-member v1 scores while applying local feedback and your keeps an upvote-only document hot when last_upvoted_at is recent (#1278). src/utils/search-index.ts merges cleanly.
We tried that resolution locally on top of #1282: tsc and lint pass, and so do the vote, confidence and search-index tests. Once the rebased head has green CI, we'll approve and merge.
|
@dbdefault #1282 landed first, so we resolved the conflict on your branch ourselves ( |
|
No confident findings in the specified diff. Resolved: The earlier [P1 blocking] missing real-CLI verification finding is fixed. The PR description documents a build and a representative real-CLI comparison showing that a recently upvoted, never-recalled document avoids the cold penalty. Note: Verification names Review was read-only; no PR code was run, built, or installed. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Approving 2be9947a. The only change since our earlier approval is the merge of current main. It keeps both tests in search-index-local-votes.test.ts, and tsc, lint and the related tests pass locally. We'll merge once CI is green.
Summary
Fixes #1278. A positive vote on a document that was never recalled left
last_recalled_atempty.computeConfidencetreated that as 180 days old, so recency was 0 and confidence fell belowHOT_THRESHOLD, applyingCOLD_PENALTYand demoting the doc in recall search.Changes
lastRecalledAtandlastUpvotedAt.lastUpvotedAtintocomputeConfidence.docs/guide/knowledge.md,skill-data/core/references/troubleshooting.md) now say a fresh upvote also refreshes recency.Real CLI run
Built
dist/withnpm run buildand ran the CLI in a throwawayHOME(temp dir, local bare origin,usageReport: false). The team repo has two learnings that match the same query:retry-backoff.mdandretry-jitter.md. Neither has ever been recalled. Onlyretry-backoffgets one upvote:retry-backoff.mdinsearch-index.jsonrecall --checktop hit4ad6eb5)"votes":1,"confidence":0.72,"hotness":1score=6.3 title="Retry failed requests"(the voted doc)main(8a94438)"votes":1,"confidence":0.42,"hotness":0.3score=5.9 title="Retry failed requests with jitter"(the unvoted doc)In both runs the vote file showed
recalled_count: 0,upvoted_count: 1,last_recalled_at: ""and a freshlast_upvoted_at.retry-jitter.mdhadconfidence: nullandhotness: 1in both.Impact on promote / prune /
--confidence-writebackcomputeConfidencealso drivesprune(src/maintenance/prune.ts),promote(src/maintenance/promote.ts,MIN_CONFIDENCE = 0.90) and--confidence-writeback. Taking the later of the two timestamps can only raise confidence, never lower it. That has two effects:prune/archive: a learning that was upvoted recently but never recalled no longer looks stale, so it is less likely to be archived.promote: a learning that already meets the vote, contributor and age thresholds, but whose last recall is old, can become promotable after one fresh upvote, because recency goes from 0 back to 1. Upvotes can still add at most the capped 30% vote factor.I worked this out from the formula and did not add a promote/prune case in this PR. I'm happy to add one if you'd like it pinned down.
Test plan
npx vitest run src/__tests__/confidence.test.ts src/__tests__/search-index-local-votes.test.ts(24 passed)npx tsc --noEmit,npm run lintnpx vitest run: 11961 passed, 7 failed. The same 7 also fail onmain8a94438on this machine (cursor-bubble-range, cursor-user-images, git-discovery, submodule-checkout, windows-test-list), so this PR doesn't cause them.Note: #1282 adds a test at the end of the same
describeblock insearch-index-local-votes.test.ts. If it lands first, I'll rebase and keep both tests.AI disclosure
This change was prepared with AI assistance (Claude / Grok coding agents). The values above come from real runs on my machine.