feat: publish retrieved learning links#128
Conversation
Attach each injected profile or playbook to one successfully published Assistant interaction with retry-safe persisted state. Add a read-only live/archive health report for evidence windows, link resolution, and session joins.
Coordinate registry readers with writers and retain the last incomplete JSONL line for retry, preventing a publish watermark from permanently skipping an injected learning.
📝 WalkthroughWalkthroughChangesRetrieved Learning Publishing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Session
participant PublishFlow
participant ReflexioAdapter
participant ReflexioAPI
Session->>PublishFlow: provide frozen unpublished interactions
PublishFlow->>ReflexioAdapter: publish with request_id
ReflexioAdapter->>ReflexioAPI: POST retrieved learning payload
ReflexioAPI-->>ReflexioAdapter: publish result
ReflexioAdapter-->>PublishFlow: success or fallback result
PublishFlow->>Session: advance published watermark
Possibly related PRs
🚥 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
🧹 Nitpick comments (4)
scripts/eval_link_report.py (1)
69-84: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrefer line-by-line iteration over
read_text().splitlines()for large archive and injected files.Both
archive_rowsand the registry-reading loop inbuild_reportload entire files into memory viaread_text().splitlines(). Archive files for high-volume tables (e.g.,interactions,requests) can grow significantly, risking OOM in constrained operator environments. Iterating line-by-line withopen()is equally readable and avoids holding the full file in memory.♻️ Proposed refactor for `archive_rows`
def archive_rows(archive_dir: Path, table: str) -> list[dict[str, Any]]: path = archive_dir / f"{table}.jsonl" if not path.is_file(): return [] rows: list[dict[str, Any]] = [] - for line in path.read_text().splitlines(): - if not line.strip(): - continue - try: - record = json.loads(line) - except json.JSONDecodeError: - continue - row = record.get("row") if isinstance(record, dict) else None - if isinstance(row, dict): - rows.append(row) + with path.open("r", encoding="utf-8") as fh: + for line in fh: + if not line.strip(): + continue + try: + record = json.loads(line) + except json.JSONDecodeError: + continue + row = record.get("row") if isinstance(record, dict) else None + if isinstance(row, dict): + rows.append(row) return rows♻️ Proposed refactor for registry reading in `build_report`
registry_entries: list[dict[str, Any]] = [] if sessions_dir.is_dir(): for path in sessions_dir.glob("*.injected.jsonl"): - for line in path.read_text().splitlines(): - try: - entry = json.loads(line) - except json.JSONDecodeError: - continue - if isinstance(entry, dict): - registry_entries.append(entry) + with path.open("r", encoding="utf-8") as fh: + for line in fh: + try: + entry = json.loads(line) + except json.JSONDecodeError: + continue + if isinstance(entry, dict): + registry_entries.append(entry)Also applies to: 197-206
🤖 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 `@scripts/eval_link_report.py` around lines 69 - 84, Update archive_rows and the registry-reading loop in build_report to open their JSONL files and iterate over them line by line instead of using read_text().splitlines(). Preserve the existing blank-line skipping, JSON decoding, filtering, and row-processing behavior while avoiding loading entire files into memory.plugin/src/claude_smart/state.py (2)
254-256: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDedup check is O(n) per entry against a growing list.
candidate in retrievedlinearly scans the per-interaction list (up to_RETRIEVED_LEARNINGS_WIRE_CAPitems), making the loop quadratic in the worst (capped) case. Bounded by the 1000-item cap so real-world impact is small, but asetof(wire_kind, real_id)tuples would make this O(1) with minimal code churn.♻️ Optional dedup optimization
- remaining: list[dict[str, Any]] = [] + remaining: list[dict[str, Any]] = [] + seen: set[tuple[str, str]] = set() for entry in [*pending, *injected_entries]: ... interaction = interactions[assistant_interaction_indexes[target]] retrieved = interaction.setdefault("retrieved_learnings", []) candidate = {"kind": wire_kind, "learning_id": real_id} - if candidate in retrieved: + key = (wire_kind, real_id) + if key in seen or candidate in retrieved: continue if attached_total >= _RETRIEVED_LEARNINGS_WIRE_CAP: truncated += 1 continue retrieved.append(candidate) + seen.add(key) attached_total += 1Note:
seenmust be scoped per interaction (or keyed by interaction index) if dedup should remain per-interaction rather than global.🤖 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/state.py` around lines 254 - 256, Replace the linear candidate membership check in the interaction retrieval loop with a per-interaction set of `(wire_kind, real_id)` keys, and check/add each key as candidates are processed. Keep the `seen` set scoped to each interaction so deduplication remains local, while preserving the existing `retrieved` list output and cap behavior.
187-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo direct unit tests for
attach_retrieved_learnings/retrieved_learning_watermarkin this file.Coverage currently comes only indirectly through
tests/test_publish.py. Given the complexity here (bisect-based eligibility, cap enforcement, kind resolution, pending carryover) and the malformed-retrieved_pending/mismatched-length edge cases discussed above, direct unit tests intests/test_state.pywould make regressions easier to pinpoint.🤖 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/state.py` around lines 187 - 272, Add direct unit tests in tests/test_state.py for attach_retrieved_learnings and the retrieved_learning_watermark flow, covering bisect-based eligibility, wire-cap enforcement, valid and invalid kind resolution, pending carryover, malformed retrieved_pending values, and mismatched record/interaction lengths. Keep the existing indirect publish tests unchanged and assert both mutated interactions and returned watermark/pending state.plugin/src/claude_smart/publish.py (1)
65-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBlind
except Exception(Ruff BLE001) — likely intentional but worth acknowledging.The broad catch matches the stated "best-effort metadata must never block a publish" intent, so it's a defensible use of a wide catch. As noted in the state.py review, this also means a partial mutation of
interactionsinsideattach_retrieved_learningscan slip through silently before the offset bookkeeping is discarded — the try/except boundary alone doesn't guarantee atomicity of the mutation it wraps. If keeping the broad catch, consider a short inline comment/# noqa: BLE001to signal the deviation is deliberate rather than an oversight.🤖 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/publish.py` around lines 65 - 78, Keep the broad exception handling around the best-effort retrieved-learning attachment, but explicitly suppress Ruff BLE001 at the catch site or add a concise inline comment documenting that the broad catch is deliberate because metadata failures must not block publishing. Preserve the existing warning and publish flow.Source: Linters/SAST tools
🤖 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/state.py`:
- Around line 199-261: The attach_retrieved_learnings flow currently mutates
interactions before all pending and injected entries are validated, allowing
partial updates when malformed data raises. Stage validated attachments and
duplicate/capacity decisions without mutating interactions, then commit the
staged retrieved_learnings changes only after processing the entire loop
succeeds; preserve the existing counters, skipping, truncation, and
target-assignment behavior.
---
Nitpick comments:
In `@plugin/src/claude_smart/publish.py`:
- Around line 65-78: Keep the broad exception handling around the best-effort
retrieved-learning attachment, but explicitly suppress Ruff BLE001 at the catch
site or add a concise inline comment documenting that the broad catch is
deliberate because metadata failures must not block publishing. Preserve the
existing warning and publish flow.
In `@plugin/src/claude_smart/state.py`:
- Around line 254-256: Replace the linear candidate membership check in the
interaction retrieval loop with a per-interaction set of `(wire_kind, real_id)`
keys, and check/add each key as candidates are processed. Keep the `seen` set
scoped to each interaction so deduplication remains local, while preserving the
existing `retrieved` list output and cap behavior.
- Around line 187-272: Add direct unit tests in tests/test_state.py for
attach_retrieved_learnings and the retrieved_learning_watermark flow, covering
bisect-based eligibility, wire-cap enforcement, valid and invalid kind
resolution, pending carryover, malformed retrieved_pending values, and
mismatched record/interaction lengths. Keep the existing indirect publish tests
unchanged and assert both mutated interactions and returned watermark/pending
state.
In `@scripts/eval_link_report.py`:
- Around line 69-84: Update archive_rows and the registry-reading loop in
build_report to open their JSONL files and iterate over them line by line
instead of using read_text().splitlines(). Preserve the existing blank-line
skipping, JSON decoding, filtering, and row-processing behavior while avoiding
loading entire files into memory.
🪄 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: 3a6cc658-9165-4b5e-be0c-e89506fcdf7a
📒 Files selected for processing (6)
plugin/src/claude_smart/publish.pyplugin/src/claude_smart/state.pyscripts/eval_link_report.pytests/test_eval_link_report.pytests/test_publish.pytests/test_state.py
Preserve retrieved-learning fields across the 0.2.28 client compatibility window, stage attachment changes atomically, and stream health-report inputs. Persist publish and linkage progress together to close the local crash window.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/eval_link_report.py (1)
68-70: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueStatic-analysis SQL-injection hint here is a false positive.
columns()(and the other f-string queries at Lines 64, 121, 149, 153) interpolate only table/column names sourced from the module constantsRETENTION_TARGETS/LEARNING_IDSor hardcoded literals — never external input. SQLite also can't bind identifiers/PRAGMAtargets as parameters, so an f-string is unavoidable here. Optionally, assertingtableagainst a known allowlist before interpolation would silence the linter and add defense-in-depth.🤖 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 `@scripts/eval_link_report.py` around lines 68 - 70, The SQL-injection warning in columns and the related f-string queries is a false positive because identifiers come only from trusted module constants or literals. Preserve the current identifier interpolation, and optionally validate table/column values against the known allowlist before executing the queries to provide defense in depth and satisfy static analysis.Source: Linters/SAST tools
tests/test_adapter.py (1)
286-311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecouple this test from Reflexio’s runtime schema.
tests/test_adapter.py:286-311depends onreflexio.models.api_schema.service_schemas.InteractionData.model_fieldsat runtime, so the branch under test changes with thereflexio-aiversion present. Stub the import/helper here so the raw-request path stays deterministic.🤖 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 `@tests/test_adapter.py` around lines 286 - 311, Decouple test_pinned_client_sends_retrieved_learnings_without_model_stripping from the installed Reflexio runtime schema by stubbing the InteractionData model-field import or helper used by the adapter during this test. Configure the stub so the raw-request path is selected deterministically, while preserving the existing assertions for retrieved_learnings and the published request.plugin/src/claude_smart/reflexio_adapter.py (1)
140-157: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMove the
kwargsbuild into the non-raw branch. The raw publish path bypassesclient.publish_interaction, so this block is unused there and just adds a bit of noise.🤖 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/reflexio_adapter.py` around lines 140 - 157, Move the construction of kwargs into the non-raw branch that calls client.publish_interaction. Keep the raw _make_request path unchanged and ensure kwargs is only built immediately before its client.publish_interaction(**kwargs) use.
🤖 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/src/claude_smart/reflexio_adapter.py`:
- Around line 140-157: Move the construction of kwargs into the non-raw branch
that calls client.publish_interaction. Keep the raw _make_request path unchanged
and ensure kwargs is only built immediately before its
client.publish_interaction(**kwargs) use.
In `@scripts/eval_link_report.py`:
- Around line 68-70: The SQL-injection warning in columns and the related
f-string queries is a false positive because identifiers come only from trusted
module constants or literals. Preserve the current identifier interpolation, and
optionally validate table/column values against the known allowlist before
executing the queries to provide defense in depth and satisfy static analysis.
In `@tests/test_adapter.py`:
- Around line 286-311: Decouple
test_pinned_client_sends_retrieved_learnings_without_model_stripping from the
installed Reflexio runtime schema by stubbing the InteractionData model-field
import or helper used by the adapter during this test. Configure the stub so the
raw-request path is selected deterministically, while preserving the existing
assertions for retrieved_learnings and the published request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 56d22196-566f-4ad9-b7af-e04bd6127f05
📒 Files selected for processing (6)
plugin/src/claude_smart/publish.pyplugin/src/claude_smart/reflexio_adapter.pyplugin/src/claude_smart/state.pyscripts/eval_link_report.pytests/test_adapter.pytests/test_state.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_state.py
- plugin/src/claude_smart/publish.py
- plugin/src/claude_smart/state.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugin/dashboard/app/dashboard/page.tsx (1)
41-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
ArchiveStatusfrom@/lib/archive-statusinstead of redefining it.The interface is already exported from
archive-status.ts(line 14). Redefining it here risks drift if fields change. Useimport type— it's erased at compile time, so nonode:fs/node:osmodules will leak into the client bundle.♻️ Proposed refactor
import type { AgentPlaybook, PlaybookApplicationStat, SessionSummary, UserPlaybook, UserProfile, } from "`@/lib/types`"; +import type { ArchiveStatus } from "`@/lib/archive-status`";Then remove the local interface definition:
-interface ArchiveStatus { - enabled: boolean; - entryCount: number; - sizeBytes: number; - warningBytes: number; - exceeded: boolean; -} - export default function DashboardPage() {🤖 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/app/dashboard/page.tsx` around lines 41 - 47, Replace the local ArchiveStatus interface in the dashboard page with a type-only import from "`@/lib/archive-status`", and remove the duplicate definition. Keep all existing ArchiveStatus usages unchanged.
🤖 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/app/dashboard/page.tsx`:
- Around line 41-47: Replace the local ArchiveStatus interface in the dashboard
page with a type-only import from "`@/lib/archive-status`", and remove the
duplicate definition. Keep all existing ArchiveStatus usages unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e216a2b-e3d7-4f7d-adca-7e20dd5b22ef
📒 Files selected for processing (5)
plugin/dashboard/app/api/archive-status/route.tsplugin/dashboard/app/dashboard/page.tsxplugin/dashboard/components/common/stat-card.tsxplugin/dashboard/lib/archive-status.tsplugin/dashboard/lib/format.ts
|
Review findings (critical pass against the workstream design; the touched suites are green at head — 102 passed locally). 1. Every publish takes the raw 2. Snapshot fields are not clamped to server caps → poison-pill publish after the vendor bump. 3. Minor: dashboard threshold parsing diverges from the backend. Decision note: removing Also verified good in this pass: byte-offset watermark + pending list persisted atomically with 🤖 Generated with Claude Code |
|
Incorporated the latest review in
The one-off report remains removed as agreed. Full suite: 659 passed; dashboard lint/build passed. |
|
Re-review of the follow-up commits ( All three prior findings are resolved, each with a regression test:
Also improved beyond the findings: One non-blocking observation: once FIFO eviction starts on the reflexio side, archive size sits at ≈100 % of the ceiling permanently, so the tile will be red ("Storage ceiling reached") in steady state. If red is meant to signal "oldest evidence is rotating out," fine; if red should mean "action needed," consider amber for the at-ceiling steady state and reserving red for archive write failures — which, after the fail-open change upstream, are now the silent evidence-loss mode worth surfacing. Verified locally at Ready for lead review from my side. 🤖 Generated with Claude Code |
wenchanghan
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
2 participants
Why
Reflexio already knows how to store and evaluate
retrieved_learnings, but claude-smart does not currently tell it which preferences or skills were shown before an Assistant response. The interaction arrives without that link, so later evaluation cannot attribute the response to the retrieved learning.Before / changed / after
{kind, learning_id}pair to the first eligible Assistant response.flowchart LR A["Learning is shown"] --> B["Local registry keeps its ID"] B --> C["Next Assistant response"] C --> D["Publish kind + learning ID"] D --> E["Reflexio stores the link"] E --> F["On-demand evaluation"]Published data
The only new interaction field is an identity list:
{ "role": "Assistant", "content": "Use pathlib here.", "retrieved_learnings": [ {"kind": "user_playbook", "learning_id": "11"}, {"kind": "profile", "learning_id": "profile-1"} ] }No learning text or custom snapshot is copied into this field.
Reliability
Scope and what follows
This PR records identity pairs for new traffic. It does not add snapshots, an archive, a dashboard surface, or another linkage store.
Reflexio's merged upstream evaluator support stores and judges the field on demand. The companion Reflexio PR makes those evaluations follow an ID through later merges or supersession. After both changes are released, opted-in machines begin retaining the complete response-to-learning identity data needed for future analysis. Separate read-only tools reconstruct the limited pre-writer window and report evidence quality; no historical identity is guessed when lineage is absent.
Verification
git diff --check: passed.Summary by CodeRabbit
New Features
Bug Fixes