feat(memory): dosage-aware guideline retrieval (core + top-k) [WIP, dependent on 283)#288
Open
jayaramkr wants to merge 3 commits into
Open
feat(memory): dosage-aware guideline retrieval (core + top-k) [WIP, dependent on 283)#288jayaramkr wants to merge 3 commits into
jayaramkr wants to merge 3 commits into
Conversation
added 3 commits
July 6, 2026 19:53
Make guideline consolidation carry a support count and evidence polarity so downstream selection can dose memory by how well-attested each guideline is (the capability-dependent dosage finding from the AppWorld experiments). - schema: add `support` (>=1) and `evidence` (success|failure|both|None) to Guideline; add ConsolidatedGuideline/ConsolidatedGuidelineResponse that carry `source_indices` so consolidation attributes support exactly. - clustering.combine_cluster: sum member support and merge evidence per output guideline via source_indices; carry any uncovered member through unchanged (lossless fail-safe) so total support is conserved and no advice is dropped. Add a `mode` arg (lossless|lossy) driving conservative vs aggressive merging. - combine_guidelines prompt: request `source_indices` (0-based) and lossless framing; add an aggressive variant for lossy mode. - config: add consolidation_mode (none|lossless|lossy) and lossy_target_num_guidelines. Support-threshold *filtering* (sup2/sup3) is left to non-destructive selection in a follow-up, not deletion here. - evolve_client.consolidate_guidelines: pass mode through, short-circuit on none, persist support/evidence, and report support_before/after (ConsolidationResult) so conservation is observable. - phoenix_sync / mcp_server: stamp support=1 on freshly mined guidelines. Tests: support summed, evidence merged (success+failure -> both), uncovered members carried through, none-mode no-op, and support conserved end-to-end. Signed-off-by: JAYARAM RADHAKRISHNAN <jayaramkr@us.ibm.com>
Address CodeRabbit review on the consolidation PR: - _attribute_support: dedupe indices within a single consolidated guideline's source_indices before summing, so a repeated index (e.g. [0, 0, 1]) can't count a member's support twice and inflate the conserved total. Adds a regression test. - ConsolidationResult docstring: drop the reference to a non-existent "support" consolidation mode; describe support-threshold pruning as a future step. Signed-off-by: JAYARAM RADHAKRISHNAN <jayaramkr@us.ibm.com>
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>
📝 WalkthroughWalkthroughThis PR introduces dosage-aware guideline retrieval and mode-aware consolidation. Guideline schema gains ChangesDosage-aware guideline consolidation and retrieval
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI_or_MCP
participant EvolveClient
participant Retrieval as retrieval.select_guidelines
User->>CLI_or_MCP: request guidelines(task, top_k, core_support)
CLI_or_MCP->>EvolveClient: select_guidelines(namespace, task_query, ...)
EvolveClient->>EvolveClient: filter entities by evidence
EvolveClient->>Retrieval: candidate entities, thresholds
Retrieval-->>EvolveClient: GuidelineSelection(core, retrieved)
EvolveClient-->>CLI_or_MCP: GuidelineSelection
CLI_or_MCP-->>User: formatted core + retrieved guidelines
sequenceDiagram
participant EvolveClient
participant combine_cluster
participant LLM
participant AttributeSupport as _attribute_support
EvolveClient->>combine_cluster: entities, mode
combine_cluster->>LLM: prompt (merge_style, target count)
LLM-->>combine_cluster: ConsolidatedGuidelineResponse
combine_cluster->>AttributeSupport: consolidated guidelines, entities
AttributeSupport-->>combine_cluster: Guideline list (support, evidence merged)
combine_cluster-->>EvolveClient: list[Guideline]
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
Comment on lines
+344
to
+353
| resolved_ns = _resolve_namespace(namespace_id) | ||
| logger.info( | ||
| "get_relevant_guidelines for task=%s (namespace=%s, top_k=%s, core_support=%s, user_present=%s, session_present=%s)", | ||
| task, | ||
| resolved_ns, | ||
| top_k, | ||
| core_support, | ||
| user_id is not None, | ||
| session_id is not None, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Raw task text logged at INFO, unlike the presence-only pattern used for user/session IDs.
save_trajectory in this same file deliberately logs only boolean presence for user_id/session_id at INFO (actual values only at DEBUG), but this tool logs the full task string directly at INFO. task is 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)📝 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.
Suggested change
| resolved_ns = _resolve_namespace(namespace_id) | |
| logger.info( | |
| "get_relevant_guidelines for task=%s (namespace=%s, top_k=%s, core_support=%s, user_present=%s, session_present=%s)", | |
| task, | |
| resolved_ns, | |
| top_k, | |
| core_support, | |
| user_id is not None, | |
| session_id is not None, | |
| ) | |
| resolved_ns = _resolve_namespace(namespace_id) | |
| logger.info( | |
| "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, 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.
Comment on lines
+354
to
+361
| 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) | ||
| return format_selection(selection) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No handling for ValueError from select_guidelines, unlike the CLI surface.
cli.py's select_entities explicitly catches ValueError from client.select_guidelines ("Retrieval unavailable... Configure the embedding model/backend"), implying this call can legitimately raise ValueError on 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)
+ 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)📝 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.
Suggested change
| 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) | |
| return format_selection(selection) | |
| 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, 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.
Comment on lines
+156
to
+218
| 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.
Suggested change
| 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.
jayaramkr
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
New Features
Bug Fixes
Tests