feat(memory): dosage-aware guideline retrieval (core + top-k) - #288
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesDosage-aware guideline selection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Requester
participant CLI_or_MCP
participant EvolveClient
participant Retrieval
participant Formatter
Requester->>CLI_or_MCP: request guidelines for task
CLI_or_MCP->>EvolveClient: select_guidelines(namespace, task, options)
EvolveClient->>Retrieval: filtered entities and configured parameters
Retrieval-->>EvolveClient: core and retrieved guidelines
EvolveClient-->>CLI_or_MCP: GuidelineSelection
CLI_or_MCP->>Formatter: format_selection(selection)
Formatter-->>Requester: formatted guideline block
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: 3
🧹 Nitpick comments (7)
altk_evolve/config/evolve.py (1)
12-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd bounds validation for the new retrieval/consolidation numeric settings.
retrieval_top_k,core_support,min_support,retrieval_near_core_thresh, andretrieval_dedup_threshare all plainint/floatwith no range constraints, even though they're loadable from env vars. An invalid override (e.g., a negativetop_k, or a threshold outside[0, 1]) would pass validation silently and corrupt retrieval ranking/filtering downstream instead of failing fast at config load.Guideline.supportinschema/guidelines.pyalready establishes theField(ge=...)pattern for exactly this kind of dosage-signal field.♻️ Proposed bounds validation
- retrieval_top_k: int = 10 - core_support: int = 3 - min_support: int = 1 # non-destructive sup2/sup3 filter on the candidate pool + retrieval_top_k: int = Field(default=10, ge=0) + core_support: int = Field(default=3, ge=0) + min_support: int = Field(default=1, ge=0) # non-destructive sup2/sup3 filter on the candidate pool retrieval_similarity_key: Literal["source_task", "guideline_text"] = "source_task" - retrieval_near_core_thresh: float = 0.75 - retrieval_dedup_thresh: float = 0.90 + retrieval_near_core_thresh: float = Field(default=0.75, ge=0.0, le=1.0) + retrieval_dedup_thresh: float = Field(default=0.90, ge=0.0, le=1.0)🤖 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 `@altk_evolve/config/evolve.py` around lines 12 - 30, Add explicit range validation to the numeric config fields in the evolve settings so bad env overrides fail at load time instead of propagating downstream. Update the evolve configuration class to constrain retrieval_top_k, core_support, and min_support to nonnegative/positive integer bounds as appropriate, and clamp retrieval_near_core_thresh and retrieval_dedup_thresh to valid [0, 1] float ranges using the same Field-style validation pattern already used for dosage fields like Guideline.support. Keep the existing names and defaults, but ensure the config model itself rejects invalid values.altk_evolve/schema/guidelines.py (1)
32-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove
support/evidencefromConsolidatedGuideline.altk_evolve/llm/guidelines/clustering.py::_attribute_supportrecomputes both fromsource_indices, so the model’s values are ignored. A leaner response schema would avoid asking for fields that are immediately discarded.🤖 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 `@altk_evolve/schema/guidelines.py` around lines 32 - 49, Remove the unused support/evidence fields from ConsolidatedGuideline in the schema and keep only the fields needed for consolidation, including source_indices. Update ConsolidatedGuidelineResponse and any related schema definitions so the model is no longer asked to populate values that _attribute_support in clustering.py recomputes from source_indices anyway. Verify any consumers of ConsolidatedGuideline still work with the leaner response shape.tests/unit/test_combine_guidelines.py (1)
91-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of support/evidence semantics, but the cross-guideline overlap edge case (see
clustering.py_attribute_support) isn't tested.
test_combine_cluster_dedupes_repeated_source_indicescovers duplicate indices within onesource_indiceslist, but there's no test for two separate consolidated guidelines both claiming the same index — the scenario where content can be silently dropped without triggering the support-conservation warning.Adding a test would help lock in the fix for that issue.
🤖 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/unit/test_combine_guidelines.py` around lines 91 - 169, Add a regression test for the cross-guideline overlap case in the clustering flow: two separate consolidated guidelines should not both consume the same source index without preserving support. Extend the tests around combine_cluster and _attribute_support so a repeated index across different merged outputs is either deduplicated safely or surfaced via the existing support-conservation warning, and assert that total support is still conserved and no content is silently dropped.tests/unit/test_retrieval.py (1)
92-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for in-pool
dedup_thresh(non-core duplicate pair).Near-core dedup is tested, but the separate in-pool dedup path (two non-core candidates duplicating each other) in
select_guidelinesisn't exercised by any test here.As per path instructions,
tests/**/*.py: "All new features need tests (unit + e2e where applicable)".🤖 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/unit/test_retrieval.py` around lines 92 - 104, Add a unit test in test_retrieval.py to cover the in-pool dedup path in select_guidelines: create two non-core candidates that are near-duplicates of each other, set dedup_thresh so they should collapse, and verify only one survives in sel.retrieved while unrelated candidates remain. Reuse the existing helpers (_make_entity, self._model, and select_guidelines) so the new test clearly exercises the separate non-core duplicate-pair behavior, not the near-core drop case already covered by test_near_core_candidate_dropped.Source: Path instructions
altk_evolve/frontend/client/evolve_client.py (3)
187-193: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnrecognized
modesilently falls back to "lossless".If
modeis anything other than"none"/"lossy"(e.g. a typo inconsolidation_modeconfig), it's silently treated as"lossless"with no warning, hiding misconfiguration.🛡️ Proposed fix to warn on unexpected values
- combine_mode = "lossy" if mode == "lossy" else "lossless" + if mode not in ("lossy", "lossless"): + logger.warning("Unrecognized consolidation mode %r; defaulting to 'lossless'.", mode) + combine_mode = "lossy" if mode == "lossy" else "lossless"🤖 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 `@altk_evolve/frontend/client/evolve_client.py` around lines 187 - 193, The consolidate-mode handling in evolve_client.py silently maps any unexpected value to "lossless", which can hide bad configuration. Update the mode resolution logic around evolve_client.py’s consolidation flow so that the existing `mode`/`combine_mode` branch explicitly recognizes only "none", "lossy", and "lossless", and logs a warning when `self.config.consolidation_mode` or the provided `mode` is unrecognized before falling back. Keep the current behavior for valid values, but make the fallback visible via the same logger used in this method.
242-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated support-parsing logic diverges from
retrieval._support().This inlines the same "extract support from metadata" logic as
retrieval.py's_support(), but without itsmax(1, ...)clamp, so a corrupted/negative metadata value would be counted differently here than during selection.🤖 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 `@altk_evolve/frontend/client/evolve_client.py` around lines 242 - 243, The support aggregation in `EvolveClient` is duplicating the metadata parsing logic from `retrieval._support()` and currently omits the same lower-bound clamp. Update the support calculation in this consolidation path to reuse the shared support extraction behavior (or mirror `_support()` exactly) so metadata values are normalized consistently, including clamping any missing, zero, or negative support to at least 1. Keep the change localized around the support summation in `evolve_client.py` and refer to `retrieval._support()` as the source of truth.
264-317: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing truncation warning parity with
cluster_guidelines.
cluster_guidelineswarns when the fetched entity count hitslimit(Lines 160-166, unchanged), butselect_guidelinesfetches with the samelimit=10000pattern (Line 303) without any equivalent warning. If a namespace has more guidelines thanlimit, core/retrieved selection could silently omit high-support guidelines with no signal to the caller.♻️ Proposed fix to mirror `cluster_guidelines`'s warning
entities = self.get_all_entities(namespace_id, filters={"type": "guideline"}, limit=limit) + if len(entities) >= limit: + logger.warning( + "Fetched %d entities (hit limit=%d); guideline selection may be incomplete.", + len(entities), + limit, + ) entities = _filter_by_evidence(entities, str(evidence_filter))🤖 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 `@altk_evolve/frontend/client/evolve_client.py` around lines 264 - 317, Add truncation-warning parity to select_guidelines in evolve_client.py: after get_all_entities(...) with limit, detect when the fetched guideline count reaches the limit and emit the same kind of warning that cluster_guidelines uses. Keep the selection flow unchanged, but mirror the existing cluster_guidelines warning logic so callers are told when core/retrieved guidelines may be incomplete due to truncation.
🤖 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 `@altk_evolve/frontend/mcp/mcp_server.py`:
- Around line 344-353: The get_relevant_guidelines logging in mcp_server.py is
leaking the full user-supplied task text at INFO, unlike the save_trajectory
pattern that only records presence for sensitive inputs. Update the logger call
in get_relevant_guidelines to log only non-sensitive metadata at INFO, such as
namespace, top_k, core_support, and boolean presence flags, and move the raw
task value to DEBUG if it must be retained for troubleshooting.
- Around line 354-361: The MCP selection flow in mcp_server.py only handles
NamespaceNotFoundException around client.select_guidelines, but it should also
catch ValueError the same way the CLI does. Update the select_guidelines call
path in this tool to intercept ValueError, return a clear retrieval-unavailable
message consistent with select_entities in cli.py, and keep the existing
namespace retry logic unchanged for NamespaceNotFoundException.
In `@altk_evolve/llm/guidelines/clustering.py`:
- Around line 156-218: In _attribute_support, a ConsolidatedGuideline can be
skipped when all its source_indices were already assigned, which silently drops
its content without being caught by the support conservation warning. Update the
loop over consolidated so that empty idxs is treated as a detected overlap case:
emit a warning/debug log with the cg.source_indices and cg.content instead of
just continuing. If only some indices are unassigned, also consider logging the
partial overlap so the first-come attribution behavior is visible. Keep the
existing total_in/total_out check, but add logging around the drop path to
preserve the “no advice is ever dropped” guarantee.
---
Nitpick comments:
In `@altk_evolve/config/evolve.py`:
- Around line 12-30: Add explicit range validation to the numeric config fields
in the evolve settings so bad env overrides fail at load time instead of
propagating downstream. Update the evolve configuration class to constrain
retrieval_top_k, core_support, and min_support to nonnegative/positive integer
bounds as appropriate, and clamp retrieval_near_core_thresh and
retrieval_dedup_thresh to valid [0, 1] float ranges using the same Field-style
validation pattern already used for dosage fields like Guideline.support. Keep
the existing names and defaults, but ensure the config model itself rejects
invalid values.
In `@altk_evolve/frontend/client/evolve_client.py`:
- Around line 187-193: The consolidate-mode handling in evolve_client.py
silently maps any unexpected value to "lossless", which can hide bad
configuration. Update the mode resolution logic around evolve_client.py’s
consolidation flow so that the existing `mode`/`combine_mode` branch explicitly
recognizes only "none", "lossy", and "lossless", and logs a warning when
`self.config.consolidation_mode` or the provided `mode` is unrecognized before
falling back. Keep the current behavior for valid values, but make the fallback
visible via the same logger used in this method.
- Around line 242-243: The support aggregation in `EvolveClient` is duplicating
the metadata parsing logic from `retrieval._support()` and currently omits the
same lower-bound clamp. Update the support calculation in this consolidation
path to reuse the shared support extraction behavior (or mirror `_support()`
exactly) so metadata values are normalized consistently, including clamping any
missing, zero, or negative support to at least 1. Keep the change localized
around the support summation in `evolve_client.py` and refer to
`retrieval._support()` as the source of truth.
- Around line 264-317: Add truncation-warning parity to select_guidelines in
evolve_client.py: after get_all_entities(...) with limit, detect when the
fetched guideline count reaches the limit and emit the same kind of warning that
cluster_guidelines uses. Keep the selection flow unchanged, but mirror the
existing cluster_guidelines warning logic so callers are told when
core/retrieved guidelines may be incomplete due to truncation.
In `@altk_evolve/schema/guidelines.py`:
- Around line 32-49: Remove the unused support/evidence fields from
ConsolidatedGuideline in the schema and keep only the fields needed for
consolidation, including source_indices. Update ConsolidatedGuidelineResponse
and any related schema definitions so the model is no longer asked to populate
values that _attribute_support in clustering.py recomputes from source_indices
anyway. Verify any consumers of ConsolidatedGuideline still work with the leaner
response shape.
In `@tests/unit/test_combine_guidelines.py`:
- Around line 91-169: Add a regression test for the cross-guideline overlap case
in the clustering flow: two separate consolidated guidelines should not both
consume the same source index without preserving support. Extend the tests
around combine_cluster and _attribute_support so a repeated index across
different merged outputs is either deduplicated safely or surfaced via the
existing support-conservation warning, and assert that total support is still
conserved and no content is silently dropped.
In `@tests/unit/test_retrieval.py`:
- Around line 92-104: Add a unit test in test_retrieval.py to cover the in-pool
dedup path in select_guidelines: create two non-core candidates that are
near-duplicates of each other, set dedup_thresh so they should collapse, and
verify only one survives in sel.retrieved while unrelated candidates remain.
Reuse the existing helpers (_make_entity, self._model, and select_guidelines) so
the new test clearly exercises the separate non-core duplicate-pair behavior,
not the near-core drop case already covered by test_near_core_candidate_dropped.
🪄 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
Run ID: e5742dec-fb94-41a3-a340-72ee55350c18
📒 Files selected for processing (11)
altk_evolve/cli/cli.pyaltk_evolve/config/evolve.pyaltk_evolve/frontend/client/evolve_client.pyaltk_evolve/frontend/mcp/mcp_server.pyaltk_evolve/llm/guidelines/clustering.pyaltk_evolve/llm/guidelines/prompts/combine_guidelines.jinja2altk_evolve/llm/guidelines/retrieval.pyaltk_evolve/schema/guidelines.pyaltk_evolve/sync/phoenix_sync.pytests/unit/test_combine_guidelines.pytests/unit/test_retrieval.py
| def _attribute_support( | ||
| entities: list[RecordedEntity], | ||
| consolidated: list[ConsolidatedGuideline], | ||
| member_support: list[int], | ||
| member_evidence: list[Evidence | None], | ||
| ) -> list[Guideline]: | ||
| """Map consolidated guidelines back to their source members, conserving total support. | ||
|
|
||
| Each input index is attributed to exactly one output guideline (the first that claims | ||
| it via ``source_indices``). Any index the model failed to cover is carried through | ||
| unchanged as its own guideline, so no advice is ever dropped and ``sum(support)`` is | ||
| preserved. | ||
| """ | ||
| n = len(entities) | ||
| assigned = [False] * n | ||
| out: list[Guideline] = [] | ||
|
|
||
| for cg in consolidated: | ||
| # Dedupe within a single guideline's source_indices so a repeated index (e.g. | ||
| # [0, 0, 1]) can't double-count its member's support. | ||
| seen: set[int] = set() | ||
| idxs: list[int] = [] | ||
| for i in cg.source_indices: | ||
| if isinstance(i, int) and 0 <= i < n and not assigned[i] and i not in seen: | ||
| seen.add(i) | ||
| idxs.append(i) | ||
| if not idxs: | ||
| continue | ||
| for i in idxs: | ||
| assigned[i] = True | ||
| out.append( | ||
| Guideline( | ||
| content=cg.content, | ||
| rationale=cg.rationale, | ||
| category=cg.category, | ||
| trigger=cg.trigger, | ||
| implementation_steps=cg.implementation_steps, | ||
| support=sum(member_support[i] for i in idxs), | ||
| evidence=_merge_evidence([member_evidence[i] for i in idxs]), | ||
| ) | ||
| ) | ||
|
|
||
| # Fail-safe: any uncovered member survives unchanged as its own guideline (lossless). | ||
| for i in range(n): | ||
| if assigned[i]: | ||
| continue | ||
| md = entities[i].metadata or {} | ||
| out.append( | ||
| Guideline( | ||
| content=str(entities[i].content), | ||
| rationale=str(md.get("rationale", "")), | ||
| category=_coerce_category(md.get("category")), # type: ignore[arg-type] | ||
| trigger=str(md.get("trigger", "")), | ||
| implementation_steps=_normalize_steps(md.get("implementation_steps")), | ||
| support=member_support[i], | ||
| evidence=member_evidence[i], | ||
| ) | ||
| ) | ||
|
|
||
| total_in, total_out = sum(member_support), sum(g.support for g in out) | ||
| if total_out != total_in: | ||
| logger.warning("Support not conserved during consolidation (in=%d, out=%d).", total_in, total_out) | ||
| return out |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Overlapping source_indices across consolidated guidelines silently drop content, undetected by the conservation check.
_attribute_support claims indices on a first-come basis per cg in consolidated. If a later ConsolidatedGuideline lists indices that are all already claimed by an earlier one (e.g., the model mistakenly emits two guidelines both covering [0, 1]), idxs becomes empty and the whole guideline (its content, rationale, etc.) is dropped via continue — with no logging.
Crucially, the total_in != total_out warning at Line 215-217 will not catch this: the support for the already-claimed indices was already counted by the first claimant, so totals still match even though a fully-formed piece of guidance was discarded. This silently violates the function's own documented guarantee that "no advice is ever dropped."
This is a real risk since the prompt only instructs (Line 34 of the jinja2 template) that indices appear exactly once — it doesn't guarantee it, and LLMs frequently violate such instructions.
🐛 Proposed fix: log dropped/partially-claimed guidelines instead of silently discarding
for cg in consolidated:
# Dedupe within a single guideline's source_indices so a repeated index (e.g.
# [0, 0, 1]) can't double-count its member's support.
seen: set[int] = set()
idxs: list[int] = []
+ already_claimed: list[int] = []
for i in cg.source_indices:
- if isinstance(i, int) and 0 <= i < n and not assigned[i] and i not in seen:
+ if not (isinstance(i, int) and 0 <= i < n) or i in seen:
+ continue
+ seen.add(i)
+ if assigned[i]:
+ already_claimed.append(i)
+ else:
idxs.append(i)
+ if already_claimed:
+ logger.warning(
+ "Consolidated guideline %r references already-claimed indices %s; content may be lost.",
+ cg.content,
+ already_claimed,
+ )
if not idxs:
continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _attribute_support( | |
| entities: list[RecordedEntity], | |
| consolidated: list[ConsolidatedGuideline], | |
| member_support: list[int], | |
| member_evidence: list[Evidence | None], | |
| ) -> list[Guideline]: | |
| """Map consolidated guidelines back to their source members, conserving total support. | |
| Each input index is attributed to exactly one output guideline (the first that claims | |
| it via ``source_indices``). Any index the model failed to cover is carried through | |
| unchanged as its own guideline, so no advice is ever dropped and ``sum(support)`` is | |
| preserved. | |
| """ | |
| n = len(entities) | |
| assigned = [False] * n | |
| out: list[Guideline] = [] | |
| for cg in consolidated: | |
| # Dedupe within a single guideline's source_indices so a repeated index (e.g. | |
| # [0, 0, 1]) can't double-count its member's support. | |
| seen: set[int] = set() | |
| idxs: list[int] = [] | |
| for i in cg.source_indices: | |
| if isinstance(i, int) and 0 <= i < n and not assigned[i] and i not in seen: | |
| seen.add(i) | |
| idxs.append(i) | |
| if not idxs: | |
| continue | |
| for i in idxs: | |
| assigned[i] = True | |
| out.append( | |
| Guideline( | |
| content=cg.content, | |
| rationale=cg.rationale, | |
| category=cg.category, | |
| trigger=cg.trigger, | |
| implementation_steps=cg.implementation_steps, | |
| support=sum(member_support[i] for i in idxs), | |
| evidence=_merge_evidence([member_evidence[i] for i in idxs]), | |
| ) | |
| ) | |
| # Fail-safe: any uncovered member survives unchanged as its own guideline (lossless). | |
| for i in range(n): | |
| if assigned[i]: | |
| continue | |
| md = entities[i].metadata or {} | |
| out.append( | |
| Guideline( | |
| content=str(entities[i].content), | |
| rationale=str(md.get("rationale", "")), | |
| category=_coerce_category(md.get("category")), # type: ignore[arg-type] | |
| trigger=str(md.get("trigger", "")), | |
| implementation_steps=_normalize_steps(md.get("implementation_steps")), | |
| support=member_support[i], | |
| evidence=member_evidence[i], | |
| ) | |
| ) | |
| total_in, total_out = sum(member_support), sum(g.support for g in out) | |
| if total_out != total_in: | |
| logger.warning("Support not conserved during consolidation (in=%d, out=%d).", total_in, total_out) | |
| return out | |
| def _attribute_support( | |
| entities: list[RecordedEntity], | |
| consolidated: list[ConsolidatedGuideline], | |
| member_support: list[int], | |
| member_evidence: list[Evidence | None], | |
| ) -> list[Guideline]: | |
| """Map consolidated guidelines back to their source members, conserving total support. | |
| Each input index is attributed to exactly one output guideline (the first that claims | |
| it via ``source_indices``). Any index the model failed to cover is carried through | |
| unchanged as its own guideline, so no advice is ever dropped and ``sum(support)`` is | |
| preserved. | |
| """ | |
| n = len(entities) | |
| assigned = [False] * n | |
| out: list[Guideline] = [] | |
| for cg in consolidated: | |
| # Dedupe within a single guideline's source_indices so a repeated index (e.g. | |
| # [0, 0, 1]) can't double-count its member's support. | |
| seen: set[int] = set() | |
| idxs: list[int] = [] | |
| already_claimed: list[int] = [] | |
| for i in cg.source_indices: | |
| if not (isinstance(i, int) and 0 <= i < n) or i in seen: | |
| continue | |
| seen.add(i) | |
| if assigned[i]: | |
| already_claimed.append(i) | |
| else: | |
| idxs.append(i) | |
| if already_claimed: | |
| logger.warning( | |
| "Consolidated guideline %r references already-claimed indices %s; content may be lost.", | |
| cg.content, | |
| already_claimed, | |
| ) | |
| if not idxs: | |
| continue | |
| for i in idxs: | |
| assigned[i] = True | |
| out.append( | |
| Guideline( | |
| content=cg.content, | |
| rationale=cg.rationale, | |
| category=cg.category, | |
| trigger=cg.trigger, | |
| implementation_steps=cg.implementation_steps, | |
| support=sum(member_support[i] for i in idxs), | |
| evidence=_merge_evidence([member_evidence[i] for i in idxs]), | |
| ) | |
| ) | |
| # Fail-safe: any uncovered member survives unchanged as its own guideline (lossless). | |
| for i in range(n): | |
| if assigned[i]: | |
| continue | |
| md = entities[i].metadata or {} | |
| out.append( | |
| Guideline( | |
| content=str(entities[i].content), | |
| rationale=str(md.get("rationale", "")), | |
| category=_coerce_category(md.get("category")), # type: ignore[arg-type] | |
| trigger=str(md.get("trigger", "")), | |
| implementation_steps=_normalize_steps(md.get("implementation_steps")), | |
| support=member_support[i], | |
| evidence=member_evidence[i], | |
| ) | |
| ) | |
| total_in, total_out = sum(member_support), sum(g.support for g in out) | |
| if total_out != total_in: | |
| logger.warning("Support not conserved during consolidation (in=%d, out=%d).", total_in, total_out) | |
| return out |
🤖 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 `@altk_evolve/llm/guidelines/clustering.py` around lines 156 - 218, In
_attribute_support, a ConsolidatedGuideline can be skipped when all its
source_indices were already assigned, which silently drops its content without
being caught by the support conservation warning. Update the loop over
consolidated so that empty idxs is treated as a detected overlap case: emit a
warning/debug log with the cg.source_indices and cg.content instead of just
continuing. If only some indices are unassigned, also consider logging the
partial overlap so the first-come attribution behavior is visible. Keep the
existing total_in/total_out check, but add logging around the drop path to
preserve the “no advice is ever dropped” guarantee.
There was a problem hiding this comment.
Good catch, but this is #283's consolidation code, which has already merged to main; after rebasing this PR onto main the diff no longer touches clustering.py. Tracking it as a separate fix against main rather than bundling unrelated merged code into this retrieval PR.
There was a problem hiding this comment.
@jayaramkr, that makes sense — since #283 is already merged and this PR will no longer touch clustering.py after rebasing, bundling this fix here would mix unrelated code into the retrieval change. Tracking it separately against main is the right call.
Let me know if you'd like me to open a follow-up issue for the _attribute_support overlap-handling gap so it doesn't get lost — happy to draft it with the details from this thread.
jayaramkr
changed the title
Add a selective alternative to injecting the whole playbook every task. Weaker models do worse with the full playbook and better with a small, targeted dose (the capability-dependent dosage finding); this makes that dose a first-class operation over the support signal from the consolidation PR. - llm/guidelines/retrieval.py: select_guidelines(entities, task_query, ...) -> GuidelineSelection. Always-on core = guidelines with support >= core_support; plus top-k candidates ranked by cosine(task_query, source task_description), after a non-destructive min_support floor (sup2/sup3), near-core drop and in-pool dedup. Reuses the clustering embedder; format_selection renders an injectable block. - config: injection_mode (static|retrieval), retrieval_top_k, core_support, min_support, retrieval_similarity_key, near-core/dedup thresholds, evidence_filter. - EvolveClient.select_guidelines: fetch guidelines, apply evidence_filter, delegate to select_guidelines with config defaults. - MCP get_relevant_guidelines tool and CLI `evolve entities select` expose it. Tests: core always included regardless of similarity, top-k ranked by source-task similarity, min_support filtering, near-core drop, evidence filtering. Verified end-to-end on the filesystem backend with the real MiniLM embedder. Signed-off-by: JAYARAM RADHAKRISHNAN <jayaramkr@us.ibm.com>
22d9eb2 to
328105a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
altk_evolve/frontend/mcp/mcp_server.py (2)
344-353: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRaw
tasktext logged at INFO, unlike the presence-only pattern used for user/session IDs.
save_trajectoryin this same file deliberately logs only boolean presence foruser_id/session_idat INFO (actual values only at DEBUG), but this tool logs the fulltaskstring directly at INFO.taskis arbitrary user-supplied text and may carry sensitive details (names, account info, etc.) that the codebase's own convention elsewhere tries to avoid persisting in logs.🔒 Proposed fix to align with the existing convention
logger.info( - "get_relevant_guidelines for task=%s (namespace=%s, top_k=%s, core_support=%s, user_present=%s, session_present=%s)", - task, + "get_relevant_guidelines (namespace=%s, top_k=%s, core_support=%s, task_len=%s, user_present=%s, session_present=%s)", resolved_ns, top_k, core_support, + len(task), user_id is not None, session_id is not None, ) + logger.debug("get_relevant_guidelines task=%s", task)🤖 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 `@altk_evolve/frontend/mcp/mcp_server.py` around lines 344 - 353, Update the INFO log in get_relevant_guidelines to avoid logging the raw user-supplied task text; log only a boolean presence indicator, matching save_trajectory’s treatment of user_id and session_id, while preserving the other diagnostic fields.
354-361: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNo handling for
ValueErrorfromselect_guidelines, unlike the CLI surface.
cli.py'sselect_entitiesexplicitly catchesValueErrorfromclient.select_guidelines("Retrieval unavailable... Configure the embedding model/backend"), implying this call can legitimately raiseValueErroron misconfiguration. This MCP tool has no equivalent handling, so the same failure would propagate as an unhandled exception through the tool call instead of a clear error message.🛡️ Proposed fix to mirror the CLI's handling
client = get_client() try: selection = client.select_guidelines(resolved_ns, task, top_k=top_k, core_support=core_support) except NamespaceNotFoundException: _evict_namespace(resolved_ns) resolved_ns = _resolve_namespace(namespace_id) selection = client.select_guidelines(resolved_ns, task, top_k=top_k, core_support=core_support) + except ValueError as e: + logger.error("Retrieval unavailable for get_relevant_guidelines: %s", e) + return f"Guideline retrieval unavailable: {e}" return format_selection(selection)🤖 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 `@altk_evolve/frontend/mcp/mcp_server.py` around lines 354 - 361, Update the MCP tool’s select_guidelines flow around client.select_guidelines to catch ValueError and return the same clear retrieval-unavailable message used by the CLI’s select_entities handling, while preserving the existing NamespaceNotFoundException retry behavior.
🤖 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.
Duplicate comments:
In `@altk_evolve/frontend/mcp/mcp_server.py`:
- Around line 344-353: Update the INFO log in get_relevant_guidelines to avoid
logging the raw user-supplied task text; log only a boolean presence indicator,
matching save_trajectory’s treatment of user_id and session_id, while preserving
the other diagnostic fields.
- Around line 354-361: Update the MCP tool’s select_guidelines flow around
client.select_guidelines to catch ValueError and return the same clear
retrieval-unavailable message used by the CLI’s select_entities handling, while
preserving the existing NamespaceNotFoundException retry behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5883f4b6-218e-488c-8b74-f1f25a7f2b9e
📒 Files selected for processing (6)
altk_evolve/cli/cli.pyaltk_evolve/config/evolve.pyaltk_evolve/frontend/client/evolve_client.pyaltk_evolve/frontend/mcp/mcp_server.pyaltk_evolve/llm/guidelines/retrieval.pytests/unit/test_retrieval.py
🚧 Files skipped from review as they are similar to previous changes (3)
- altk_evolve/cli/cli.py
- altk_evolve/llm/guidelines/retrieval.py
- tests/unit/test_retrieval.py
jayaramkr
changed the title
…ress review
Addresses adversarial-review and CodeRabbit findings on the dosage-aware
retrieval feature:
- config: constrain retrieval knobs (retrieval_top_k >= 0, core_support >= 1,
min_support >= 1, thresholds in [0,1]) and add a validator enforcing
min_support <= core_support so a floor can't drop core-qualifying guidelines.
Default injection_mode to "static" so existing get_guidelines callers are
unaffected.
- mcp: honor injection_mode in get_guidelines ("retrieval" routes through the
dosage-aware core + top-k path); previously the field was read nowhere.
- mcp: log get_relevant_guidelines task_len at INFO and the raw task only at
DEBUG (CodeRabbit: task is arbitrary user text); catch ValueError from
select_guidelines and return a clear message, mirroring the CLI.
- retrieval: fix min_support docstring (applies to the whole pool before the
core/candidate split) and document the source_task content fallback.
- tests: cover the dedup-drop branch, assert the client fetches the full pool
(limit=10000, not the get_all_entities default of 100), and verify
injection_mode routing in get_guidelines.
SummaryThis PR adds dosage-aware guideline retrieval with an always-on support-based core plus top-k task-relevant guidelines. The latest update keeps existing No issues met the confidence threshold for review comments. Testing
|
…o rewritten consolidation Main rewrote clustering.py / consolidate_guidelines (#283, support-conserving consolidation) and added the dosage-aware retrieval path (#288). Main's new behavior is taken wholesale; the memory-hook seam is re-applied on top: - clustering.py: kept main's ConsolidatedGuidelineResponse + mode-aware prompt rendering; both completion() call sites now use the dispatch_llm_pre_call result (purpose="guideline_combination"), hoisted outside the retry loop. - evolve_client.py: kept main's consolidation_mode / lossy handling; restored the INTERNAL read seam (backend._search_entities_impl) so a redacting memory_post_read plugin cannot make consolidation permanently persist the redacted view. initialize_hooks wiring preserved. - config/evolve.py: both sides — main's dosage knobs and our hooks field. - test_combine_guidelines.py: adopted main's _make_client helper and its support-conservation tests; re-applied the internal-seam assertions. - uv.lock: regenerated (uv lock --check clean). Seam re-audit post-merge: 8 completion() sites under altk_evolve/, all covered by a hoisted dispatch_llm_pre_call. New retrieval.py performs no backend I/O; its caller EvolveClient.select_guidelines reads via the public, hooked get_all_entities -> search_entities path. No new backend template-method bypasses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
What & why
Injecting the whole playbook every task is only best for strong models; weaker models do better with a small, targeted dose (the capability-dependent dosage finding — e.g. gpt-oss-120b regresses on the full playbook but gains from selective retrieval). This adds that selective dose as a first-class operation over the
supportsignal from the consolidation PR.This branch is stacked on
feat/memory-consolidation-modes(#283). Since a cross-fork PR can't target a fork branch, this PR is opened againstmain, so its diff currently includes #283's two commits:6690c82feat(memory): support-conserving guideline consolidation ← feat(memory): support-conserving guideline consolidation #283ca1d183fix(memory): dedupe source_indices … ← feat(memory): support-conserving guideline consolidation #28322d9eb2feat(memory): dosage-aware guideline retrieval (core + top-k) ← the net-new change hereOnce #283 merges, I'll rebase this onto
mainand the diff will collapse to just the retrieval commit. Review the last commit for what's new in this PR.Changes (this PR's commit)
llm/guidelines/retrieval.py(new):select_guidelines(entities, task_query, …) -> GuidelineSelection.support >= core_support(recurred across many tasks → generalize) — always included.top_kcandidates ranked bycosine(task_query, source task_description), after a non-destructivemin_supportfloor (the sup2/sup3 filter), a near-core drop (already covered), and in-pool dedup._get_sentence_transformer, numpy cosine — no torch).format_selectionrenders an injectable block.config/evolve.py):injection_mode(static|retrieval),retrieval_top_k,core_support,min_support,retrieval_similarity_key, near-core/dedup thresholds,evidence_filter.EvolveClient.select_guidelines): fetch guidelines, applyevidence_filter, delegate with config defaults.get_relevant_guidelines(task, top_k, core_support, …)and CLIevolve entities select --task … [--top-k --core-support].Tests
tests/unit/test_retrieval.py: core always included regardless of similarity, top-k ranked by source-task similarity,min_supportfiltering, near-core drop,evidence_filter, and the client wiring. Full unit suite green;ruff/ruff format/mypyclean. Verified end-to-end on the filesystem backend with the real MiniLM embedder (a Venmo task correctly retrieves the Venmo guideline).Compatibility
Additive: new module, new config fields with defaults, new client method / MCP tool / CLI command. Existing retrieval (
get_entities/get_guidelines,search_entities) is untouched. Guidelines withoutsupportmetadata are treated assupport=1.Summary by CodeRabbit
entities selectCLI command for viewing selected guidelines.