fix: report learning citations precisely and clearly#141
Conversation
Stop pre-building multi-item "copy this marker" instructions so models are not nudged to over-cite every injected memory. Resolve unique rank fingerprint drift so true markers still count, persist unresolved marker ids locally for diagnostics (not on the publish wire), and show injected vs applied counts in the dashboard.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR changes citation terminology and metrics across the citation pipeline and dashboard, preserves unresolved citation IDs, adds injected-learning counts, and updates session, dashboard, skills, and preferences views to display cited-learning statistics. ChangesCitation pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Assistant
participant StopHook
participant SessionReader
participant Dashboard
Assistant->>StopHook: emit citation marker
StopHook->>SessionReader: store cited_items and unresolved_citations
SessionReader->>Dashboard: provide citation and injection metrics
Dashboard-->>Dashboard: render cited and unresolved-citation status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/src/claude_smart/events/stop.py (1)
299-321: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDeduplicate the resolved registry entry, not only the raw citation token.
Different aliases such as an exact ID and a stale fingerprint can resolve to the same entry, producing duplicate
cited_itemsand inflated applied counts. Normalize raw tokens and track the resolved entry’s canonical ID.Proposed fix
- seen: set[str] = set() + seen_citations: set[str] = set() + seen_entries: set[str] = set() resolved: list[dict[str, Any]] = [] unresolved: list[str] = [] for cid in cited_ids: - if cid in seen: + normalized_cid = cid.casefold() + if normalized_cid in seen_citations: continue - seen.add(cid) + seen_citations.add(normalized_cid) entry = _registry_entry_for_citation(registry, cid) if not entry: unresolved.append(cid) continue + canonical_id = str(entry.get("id") or cid).casefold() + if canonical_id in seen_entries: + continue + seen_entries.add(canonical_id) item: dict[str, Any] = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/src/claude_smart/events/stop.py` around lines 299 - 321, Update the citation loop in the stop-event resolution logic to normalize each raw token before lookup and deduplicate using the resolved registry entry’s canonical ID, rather than only the raw citation token. Ensure aliases such as an exact ID and stale fingerprint produce one resolved item and do not inflate applied counts, while preserving unresolved-token handling.
🧹 Nitpick comments (1)
plugin/dashboard/lib/session-reader.ts (1)
438-449: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParallelize I/O for reading session and injected data.
The addition of
await countUniqueInjected(sessionId)inside thisforloop introduces a second sequential file read per session. To improve dashboard load times without risking file-descriptor limits (which could happen if we parallelized all sessions at once), you can read the two files for each session concurrently.⚡ Proposed refactor
- const records = await readJsonl(fullPath).catch(() => []); + const [records, injectedLearningCount] = await Promise.all([ + readJsonl(fullPath).catch(() => []), + countUniqueInjected(sessionId), + ]); const { turns, publishedUpTo, learningInteractionCount, lastTs, firstTs, preview, host, requestIds, } = foldTurns(records); - const injectedLearningCount = await countUniqueInjected(sessionId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/dashboard/lib/session-reader.ts` around lines 438 - 449, Within the session-processing loop, start the session JSONL read and countUniqueInjected(sessionId) concurrently, then await both results together before calling foldTurns. Preserve the existing fallback of [] for readJsonl failures and keep concurrency scoped to each session rather than parallelizing across sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/src/claude_smart/context_format.py`:
- Around line 353-357: Update the available-memory label in the context
formatting flow around marker_parts so it does not call every linked entry
“Qualifying.” Use neutral wording that reflects these are linked titles pending
the model’s relevance decision, while preserving the existing joined list and
punctuation.
---
Outside diff comments:
In `@plugin/src/claude_smart/events/stop.py`:
- Around line 299-321: Update the citation loop in the stop-event resolution
logic to normalize each raw token before lookup and deduplicate using the
resolved registry entry’s canonical ID, rather than only the raw citation token.
Ensure aliases such as an exact ID and stale fingerprint produce one resolved
item and do not inflate applied counts, while preserving unresolved-token
handling.
---
Nitpick comments:
In `@plugin/dashboard/lib/session-reader.ts`:
- Around line 438-449: Within the session-processing loop, start the session
JSONL read and countUniqueInjected(sessionId) concurrently, then await both
results together before calling foldTurns. Preserve the existing fallback of []
for readJsonl failures and keep concurrency scoped to each session rather than
parallelizing across sessions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 96617689-7578-47ae-9a25-835615163504
📒 Files selected for processing (11)
plugin/dashboard/app/dashboard/page.tsxplugin/dashboard/app/sessions/[sessionId]/page.tsxplugin/dashboard/app/sessions/page.tsxplugin/dashboard/components/common/injected-badge.tsxplugin/dashboard/lib/session-reader.tsplugin/dashboard/lib/types.tsplugin/src/claude_smart/context_format.pyplugin/src/claude_smart/events/stop.pyplugin/src/claude_smart/state.pytests/test_context_format.pytests/test_events.py
Code Review — PR #141
StandardsLayers — clean.
Truthfulness logic — sound. The SpecAll four spec requirements are implemented. One scope addition, two correctness/performance issues:
|
| Axis | Findings | Worst issue |
|---|---|---|
| Standards | 1 perf smell + 1 minor inefficiency | Serial countUniqueInjected in list loop |
| Spec | 1 bug + 1 scope note | Bare rank id bypasses FP guard |
The bare-rank-id bug is the one to fix before merge — it directly contradicts the PR's core correctness invariant. The serial read regression is worth addressing now since the sessions list is already the slowest page.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/dashboard/lib/session-reader.ts (1)
450-485: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winParallelize session file reads.
Currently,
listSessions()iterates over the directory entries sequentially, waiting for each session's file reads to complete before starting the next. As noted in the PR objectives, mapping the entries to promises and awaiting them concurrently viaPromise.all()will significantly improve the dashboard's load performance.⚡ Proposed fix
- const summaries: SessionSummary[] = []; - for (const entry of entries) { - if (!entry.endsWith(".jsonl")) continue; - if (entry.endsWith(".injected.jsonl")) continue; - const fullPath = path.join(dir, entry); - const sessionId = entry.replace(/\.jsonl$/, ""); - const [records, injectedLearningCount] = await Promise.all([ - readJsonl(fullPath).catch(() => []), - countUniqueInjected(sessionId), - ]); - const { - turns, - publishedUpTo, - learningInteractionCount, - appliedLearningCount, - lastTs, - firstTs, - preview, - host, - requestIds, - } = foldTurns(records); - summaries.push({ - session_id: sessionId, - turn_count: turns.length, - learning_interaction_count: learningInteractionCount, - applied_learning_count: appliedLearningCount, - injected_learning_count: injectedLearningCount, - last_activity: lastTs, - first_activity: firstTs, - published_up_to: publishedUpTo, - preview, - source: "local", - host, - request_ids: requestIds, - }); - } + const sessionPromises = entries + .filter((entry) => entry.endsWith(".jsonl") && !entry.endsWith(".injected.jsonl")) + .map(async (entry): Promise<SessionSummary> => { + const fullPath = path.join(dir, entry); + const sessionId = entry.replace(/\.jsonl$/, ""); + const [records, injectedLearningCount] = await Promise.all([ + readJsonl(fullPath).catch(() => []), + countUniqueInjected(sessionId), + ]); + const { + turns, + publishedUpTo, + learningInteractionCount, + appliedLearningCount, + lastTs, + firstTs, + preview, + host, + requestIds, + } = foldTurns(records); + return { + session_id: sessionId, + turn_count: turns.length, + learning_interaction_count: learningInteractionCount, + applied_learning_count: appliedLearningCount, + injected_learning_count: injectedLearningCount, + last_activity: lastTs, + first_activity: firstTs, + published_up_to: publishedUpTo, + preview, + source: "local", + host, + request_ids: requestIds, + }; + }); + + const summaries = await Promise.all(sessionPromises);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/dashboard/lib/session-reader.ts` around lines 450 - 485, Update listSessions() to map eligible session entries to promises and await them with Promise.all(), so readJsonl() and countUniqueInjected() for all sessions run concurrently. Preserve the existing filtering, foldTurns() processing, and SessionSummary fields, excluding invalid or injected entries as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@plugin/dashboard/lib/session-reader.ts`:
- Around line 450-485: Update listSessions() to map eligible session entries to
promises and await them with Promise.all(), so readJsonl() and
countUniqueInjected() for all sessions run concurrently. Preserve the existing
filtering, foldTurns() processing, and SessionSummary fields, excluding invalid
or injected entries as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7cc57564-3bb0-4b10-9e71-a9c75635a292
📒 Files selected for processing (10)
plugin/dashboard/app/dashboard/page.tsxplugin/dashboard/app/sessions/[sessionId]/page.tsxplugin/dashboard/app/sessions/page.tsxplugin/dashboard/components/common/learnings-badge.tsxplugin/dashboard/lib/session-reader.tsplugin/dashboard/lib/types.tsplugin/src/claude_smart/context_format.pyplugin/src/claude_smart/events/stop.pytests/test_context_format.pytests/test_events.py
🚧 Files skipped from review as they are similar to previous changes (6)
- plugin/dashboard/app/sessions/page.tsx
- plugin/dashboard/app/sessions/[sessionId]/page.tsx
- plugin/dashboard/app/dashboard/page.tsx
- plugin/dashboard/lib/types.ts
- plugin/src/claude_smart/context_format.py
- tests/test_context_format.py
Follow-up fixes are ready in b28d9e1.
Validation: These two screenshots use synthetic local data only. They show a re-ranked learning collapsing to 2 injected / 2 applied, and an unresolved marker displayed separately. |
Updated synthetic dashboard evidence
Both screenshots use synthetic local data only. 
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugin/dashboard/lib/session-reader.ts (1)
450-485: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRead sessions concurrently to avoid N+1 file I/O latency.
Both
listSessionsandlistCitedRulesiterate over the session entries sequentially, awaiting each file read before moving to the next. For larger numbers of sessions, this sequential processing significantly increases dashboard load times. You can usePromise.allto read all valid entries concurrently before aggregating them.
plugin/dashboard/lib/session-reader.ts#L450-L485: Replace theforloop inlistSessionswithawait Promise.all(entries.filter(...).map(async (entry) => ...)).plugin/dashboard/lib/session-reader.ts#L242-L293: Apply the same parallelization pattern inlistCitedRulesto concurrently fetch therecordsarrays, then flat-map or reduce them into thestatsmap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/dashboard/lib/session-reader.ts` around lines 450 - 485, Parallelize session processing in listSessions by replacing its sequential loop with Promise.all over valid entries, preserving each entry’s filtering, record reading, counting, folding, and summary construction. Apply the same concurrent record-fetch pattern in listCitedRules, then aggregate the fetched results into the existing stats map. Affected sites: plugin/dashboard/lib/session-reader.ts lines 450-485 (update listSessions); plugin/dashboard/lib/session-reader.ts lines 242-293 (update listCitedRules).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugin/dashboard/lib/session-reader.ts`:
- Around line 450-485: Parallelize session processing in listSessions by
replacing its sequential loop with Promise.all over valid entries, preserving
each entry’s filtering, record reading, counting, folding, and summary
construction. Apply the same concurrent record-fetch pattern in listCitedRules,
then aggregate the fetched results into the existing stats map. Affected sites:
plugin/dashboard/lib/session-reader.ts lines 450-485 (update listSessions);
plugin/dashboard/lib/session-reader.ts lines 242-293 (update listCitedRules).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c110e82-b6c8-4fb1-84a1-3c3f96111112
📒 Files selected for processing (18)
README.mdplugin/dashboard/app/api/rules/cited/route.tsplugin/dashboard/app/dashboard/page.tsxplugin/dashboard/app/preferences/page.tsxplugin/dashboard/app/sessions/[sessionId]/page.tsxplugin/dashboard/app/sessions/page.tsxplugin/dashboard/app/skills/page.tsxplugin/dashboard/components/common/cited-learnings-badge.tsxplugin/dashboard/components/common/injected-badge.tsxplugin/dashboard/components/common/learning-citation-badge.tsxplugin/dashboard/lib/session-reader.tsplugin/dashboard/lib/types.tsplugin/src/claude_smart/cs_cite.pytests/host_learning_harness.pytests/test_codex_support.pytests/test_context_format.pytests/test_cs_cite.pytests/test_events.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_context_format.py
wenchanghan
changed the title
|
Synthetic UI proof for this PR — no customer or real-session data:
|
1 participant
Before
Changed
After
Normal coding sessions
Dashboard
Trust
Validation
Supersedes #140 (refiled from personal fork / wenchanghan).