feat(guidelines): consistency-based guideline generation#289
Open
evduester wants to merge 64 commits into
Open
feat(guidelines): consistency-based guideline generation#289evduester wants to merge 64 commits into
evduester wants to merge 64 commits into
Conversation
Adds `generate_consistency_guidelines()` ported from the pre-sync `kaizen/llm/tips/consistency_tips.py` onto the new `altk_evolve` layout. Uses `agent-consistency` (optional extra) to resample the trajectory, score per-step uncertainty, and focus the LLM prompt on the highest- uncertainty steps. Returns `list[GuidelineGenerationResult]` to match the shape of `generate_guidelines()`. The `agent-consistency` package is wired as an editable source at `../agent-consistency`; install with `uv sync --extra consistency`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`PhoenixSync(use_consistency_guidelines=True)` and `evolve sync phoenix --consistency` route trajectory processing through the consistency guideline generator instead of the default `generate_guidelines()`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Updates the demo shell script and README to reflect the upstream rename of `kaizen` -> `altk_evolve`: - `KAIZEN_*` env vars -> `EVOLVE_*` - `kaizen` CLI -> `evolve` - `python -m kaizen.cli.cli` -> `evolve` - `KaizenClient` -> `EvolveClient` - `extract_trajectories.py` -> `scripts/extract_trajectories.py` - Replaces hard-coded `/Users/duester/Work/kaizen` with a placeholder - Matches new `generated_guidelines_*` output filename from the script Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…r, guidelines_mode, auto-mcp support
- Vendor consistency_analyzer package (9 files) replacing IBM-internal agent-consistency path dep
- Replace inference_utils.py with LiteLLM adapter; strip dead code (predictive entropy, kernels, CUGA paths)
- Add generate_consistency_guidelines() pipeline: transform IR → resample → score → generate
- Add guidelines_mode param ("regular"|"consistency"|"both") to PhoenixSync and save_trajectory MCP tool
- Replace --consistency CLI flag with --guidelines-mode [regular|consistency|both]
- Add generation_method metadata field to all auto-generated guidelines
- Add model fallback (llm_settings.guidelines_model) when trajectory carries no model info
- Unit tests: _process_trajectory dispatch, CLI flag, MCP params, IR transformation, pure functions
- E2E tests: consistency pipeline (openai_agents + smolagents), auto-mcp consistency mode
- Integration design doc: docs/consistency_guidelines_integration.md
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolves 3 conflicts between the guidelines_mode refactoring and the phoenix span-extraction fix (PR #273) that was merged into main: - phoenix_sync.py: keep origin/main's span_kind variable form in _is_llm_span - mcp_server.py: switch trajectory persistence to _persist_entities() helper - test_phoenix_sync.py: retain new TestProcessTrajectoryGuidelinesMode class Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pipeline
- Move agent_config.yaml into consistency_analyzer/ (correct home; code already referenced it there)
- Fix NumericFractionConsistencyMetric._get_distance: samples[j] → samples[i+1+j] (pairwise loop bug)
- Fix resampling model fallback: trajectory.get("model") or None, avoiding "unknown" blocking fallback
- Guard segmentation behind n_steps >= 2 to prevent LLM hallucinating subtasks on single-step trajectories
- Remove model param from save_trajectory MCP tool (always use llm_settings.guidelines_model fallback)
- Remove success_probability dead code from consistency guideline generation
- Reduce debug artifacts: keep trajectory_*.json, trajectory_ir_*_cns.json, consistency_score_card_*.json, guidelines_*_consistency.json
- Add guidelines_*_regular.json debug artifact for regular pipeline (phoenix_sync.py)
- Add EVOLVE_GUIDELINES_MODE and EVOLVE_DEBUG_DIR to .env.example
- Update docs for EVOLVE_GUIDELINES_MODE env var and removed MCP model param
- Unit tests: 114 tests for vendored consistency_analyzer modules; TestSegmentationGuard; updated for renames
- E2E tests: smolagents MCP test; both-mode smolagents+Phoenix test; always write to consistency_debug/
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a consistency analyzer and consistency-based guideline generation pipeline selectable through regular, consistency, or both modes. Integrates it with MCP and Phoenix sync, adds configuration and packaging support, and introduces unit and E2E coverage. ChangesConsistency guidelines feature
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant MCPOrPhoenixSync
participant ConsistencyGuidelines
participant ConsistencyAnalyzer
participant EvolveBackend
Agent->>MCPOrPhoenixSync: submit trajectory
MCPOrPhoenixSync->>ConsistencyGuidelines: generate consistency guidelines
ConsistencyGuidelines->>ConsistencyAnalyzer: resample and analyze trajectory
ConsistencyAnalyzer-->>ConsistencyGuidelines: score card and uncertainty data
ConsistencyGuidelines-->>MCPOrPhoenixSync: guideline results
MCPOrPhoenixSync->>EvolveBackend: persist tagged guideline entities
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…d consistency_analyzer - Auto-format 20 files (consistency_analyzer modules, consistency_guidelines, phoenix_sync, tests) - Add per-file ruff ignores for consistency_analyzer/: E402/E722/F841 are upstream issues in vendored code Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add types-PyYAML dev dependency (yaml import was untyped) - Add mypy ignore_errors override for vendored consistency_analyzer (upstream type issues) - Remove class-scoped import in test_consistency_analyzer.py (mypy misc error) - Add yaml to ignore_missing_imports override (belt-and-suspenders) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
tests/e2e/test_e2e_consistency_pipeline.py-172-204 (1)
172-204: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBlocking
readline()can hang the test indefinitely.The timeout check runs before
readline(), butreadline()itself blocks until a line is available or the pipe closes. If the sync process hangs without producing output,readline()blocks forever — thefinallyblock withprocess.terminate()is never reached, leaking the subprocess and hanging the test.🔧 Proposed fix using `select` to avoid blocking
+import select + try: while True: if time.time() - sync_start > timeout: print(f"Timeout waiting for consistency sync ({timeout}s)") break + # Check if data is available before blocking on readline() + ready, _, _ = select.select([process.stdout], [], [], 0.5) + if not ready: + if process.poll() is not None: + break + continue + line = process.stdout.readline() if not line: if process.poll() is not None: break time.sleep(0.1) continueAlso applies to: 348-377
🤖 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/e2e/test_e2e_consistency_pipeline.py` around lines 172 - 204, The consistency-sync polling loop uses process.stdout.readline(), which can block forever before the timeout is checked again. Update the loop in the consistency sync helper/test to use a non-blocking readiness check (for example via select or equivalent) before reading from stdout, so the timeout can still be enforced even when the subprocess produces no output. Apply the same fix to the duplicated sync-waiting block referenced by the other occurrence in this test file, and keep the existing guidance around verbose_sync, resampling_ran, and the generated guidelines match handling intact.altk_evolve/llm/guidelines/consistency_analyzer/sample_preprocessing.py-48-51 (1)
48-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
parse_code_responseproduces garbage when no```fence is present.When
responsecontains no triple-backtick,find("```")returns-1. Line 49 then evaluatesresponse[-1:](last character), and line 51 evaluatesresponse[:2](first two chars of that). The function silently returns a 1–2 character string instead of an empty string or the original input. Add an early guard.🛡️ Proposed fix
def parse_code_response(response: str) -> str: + if "```" not in response: + return response.strip() # remove everything preceding the python code response = response[response.find("```"):]🤖 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/consistency_analyzer/sample_preprocessing.py` around lines 48 - 51, `parse_code_response` in `sample_preprocessing.py` mishandles responses without a triple-backtick fence because it slices using the result of `find("```")` and `rfind("```")` even when no fence exists. Add an early guard at the start of `parse_code_response` to detect the no-fence case and return a safe result (such as the stripped original response) before the existing fence-trimming logic runs. Keep the fix localized around the response preprocessing block that currently removes text before and after the code fence.docs/consistency_guidelines_integration.md-58-60 (1)
58-60: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
max_samplesvalue doesn't match actual config.The YAML example here shows
max_samples: 10, but the actualagent_config.yamlhasmax_samples: 5. Update the docs to match the shipped default.🤖 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 `@docs/consistency_guidelines_integration.md` around lines 58 - 60, The YAML example for the sampling settings is out of sync with the shipped default, since it shows a different max_samples value than the actual agent_config.yaml. Update the example in the consistency guidelines so the max_samples entry matches the real default used by the configuration, and keep the surrounding aggregation/max_steps context unchanged.altk_evolve/llm/guidelines/consistency_analyzer/agent_config.yaml-7-8 (1)
7-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
max_samplesvalue doesn't match documentation.The actual config sets
max_samples: 5, butdocs/consistency_guidelines_integration.md(lines 58-60) showsmax_samples: 10. Align one to the other to avoid confusion when users override viaconfig_path=.🤖 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/consistency_analyzer/agent_config.yaml` around lines 7 - 8, The `agent_config.yaml` values are out of sync with the documented consistency analyzer settings, specifically `max_samples` in the `consistency_analyzer` config. Update either the YAML entry or the corresponding documentation so the `max_samples` value matches across the `agent_config` and the integration guide, and keep `max_steps` unchanged unless it is also intended to align.run_openai_agents_demo_with_tips.sh-71-76 (1)
71-76: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComment mentions
--consistencyflag that doesn't exist in the command.The comment says "Pass --consistency to use the resampling-based consistency guideline generator" but the
evolve sync phoenixcommand below doesn't pass any such flag. Consistency mode is controlled byEVOLVE_GUIDELINES_MODEenv var, not a CLI flag. Either addexport EVOLVE_GUIDELINES_MODE=consistencybefore the command or remove the misleading comment.🔧 Proposed fix
-# Pass --consistency to use the resampling-based consistency guideline generator -# instead of the default generate_guidelines flow (requires the `consistency` extra). +export EVOLVE_GUIDELINES_MODE=consistency uv run evolve sync phoenix \🤖 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 `@run_openai_agents_demo_with_tips.sh` around lines 71 - 76, The comment above the evolve sync command is misleading because it mentions a --consistency flag that is not used by the `uv run evolve sync phoenix` invocation. Update the script to either set `EVOLVE_GUIDELINES_MODE=consistency` before calling `evolve sync phoenix` or remove the outdated comment so it matches the actual `generate_guidelines`/consistency behavior controlled by the environment variable.altk_evolve/llm/guidelines/consistency_analyzer/single_step_consistency.py-174-182 (1)
174-182: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMetric label uses
metric_configinstead ofthis_configwhen alternates are present.When
alternatesexist,this_configis set fromfind_matching_alternateand used for the actual consistency computation (line 178), but line 182 reads the metric label from the originalmetric_config. If the matched alternate has different fields or metric, the label will be wrong.🔧 Proposed fix
- metric = "mixed" if "fields" in metric_config else metric_config.get("metric", "mixed") + metric = "mixed" if "fields" in this_config else this_config.get("metric", "mixed")🤖 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/consistency_analyzer/single_step_consistency.py` around lines 174 - 182, The metric label in single_step_consistency.py is being derived from metric_config even when alternates are resolved into this_config, so the label can mismatch the actual consistency config. Update the metric assignment in the consistency calculation block to read from the resolved this_config (the value returned by find_matching_alternate) rather than the original metric_config, and make sure the fallback logic still works when alternates are absent.altk_evolve/llm/guidelines/consistency_analyzer/utils.py-55-55 (1)
55-55: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix
find_matching_alternatetype annotation.
alternatesis annotated asdictbut is iterated as a list (for alternate in alternates) and called withmetric_config["alternates"], which is a list from the YAML config. The annotation should belist.🔧 Proposed fix
-def find_matching_alternate(alternates: dict, parsed_actual: dict) -> dict: +def find_matching_alternate(alternates: list, parsed_actual: dict) -> dict:🤖 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/consistency_analyzer/utils.py` at line 55, The `find_matching_alternate` signature uses the wrong type for `alternates`; it is iterated like a list and passed `metric_config["alternates"]`, so update the annotation in `find_matching_alternate` to reflect a list of alternates rather than a dict. Make sure the parameter type matches the YAML-driven structure and keep the return type unchanged if it still returns a single matching alternate.README_DEMO_SCRIPTS.md-79-82 (1)
79-82: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFenced code blocks missing language identifiers.
Static analysis (markdownlint MD040) flags these fenced blocks as missing a language tag.
📝 Proposed fix
-``` +```text trajectory_openai_agents_105121.json generated_guidelines_openai_agents_105121.json```diff -``` +```text AuthenticationError: team not allowed to access model. This team can only access models=['Azure/gpt-4o', ...]</details> Also applies to: 198-200 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@README_DEMO_SCRIPTS.mdaround lines 79 - 82, Add a language identifier to
each fenced code block flagged by markdownlint MD040 in README_DEMO_SCRIPTS.md,
including the examples near trajectory_openai_agents_105121.json and the
AuthenticationError snippet. Update the affected markdown fences to use an
appropriate tag such as text so the blocks remain rendered correctly while
satisfying the lint rule.</details> <!-- cr-comment:v1:ce71200dd30fa0b6f8ec42d4 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>altk_evolve/llm/guidelines/consistency_guidelines.py-219-245 (1)</summary><blockquote> `219-245`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **"Agent reasoning" branch assumes string content; list-type content (Agents SDK) isn't handled.** This function's docstring context and `_can_segment_trajectory` both explicitly anticipate assistant `content` being a list of blocks (e.g., `function_call` items) for Agents SDK trajectories. Here, a non-`tool_calls` step with list content falls into the `else` branch and is treated as if `content` were a string (`len(content)`, slicing, `+ "..."`), producing the Python list's repr in the generated prompt instead of meaningful text. <details> <summary>🐛 Proposed fix</summary> ```diff else: step_type = "Agent reasoning" - content = step.get("content", "") or "" + content = step.get("content", "") or "" + if not isinstance(content, str): + content = str(content) if len(content) > 500: content = content[:500] + "..." this_step_text = content🤖 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/consistency_guidelines.py` around lines 219 - 245, The “Agent reasoning” path in the message formatting logic assumes `step["content"]` is always a string, but Agents SDK assistant messages can provide content as a list of blocks. Update the formatting in the assistant-step loop to detect list content before the `len(content)`/slice handling, and convert those blocks into readable text instead of using the list repr. Keep the existing `tool_calls` handling in place, and make the change in the same message-processing block that sets `step_type` and `this_step_text`.altk_evolve/llm/guidelines/consistency_guidelines.py-398-407 (1)
398-407: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude Groq from constrained decoding here.
altk_evolve/llm/guidelines/consistency_guidelines.py:389-398should match the other guideline paths and skip the JSON-schema branch for Groq-backed models; otherwise this flow can take the constrained-decoding path that the rest of the guideline pipeline avoids.🤖 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/consistency_guidelines.py` around lines 398 - 407, The constrained decoding check in consistency_guidelines should explicitly skip Groq-backed models so this path matches the other guideline flows. Update the logic around get_supported_openai_params, supports_response_schema, and constrained_decoding_supported to gate out llm_settings.custom_llm_provider == "groq" before enabling the JSON-schema branch, keeping the behavior aligned with the rest of the guideline pipeline.altk_evolve/llm/guidelines/consistency_guidelines.py-302-325 (1)
302-325: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid the global
litellm.enable_json_schema_validationtoggle here. Passenable_json_schema_validation=oncompletion()instead; mutating the module-level flag can leak across overlapping guideline calls in the same process.🤖 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/consistency_guidelines.py` around lines 302 - 325, The guideline generation flow in the completion call path is mutating the module-level litellm.enable_json_schema_validation flag, which can leak across concurrent calls. Update the logic around the completion() invocations in the constrained_decoding_supported branch to pass enable_json_schema_validation directly on completion() instead of setting litellm.enable_json_schema_validation globally, and remove the global toggle from both branches while preserving the existing clean_llm_response handling in consistency_guidelines.py.
🧹 Nitpick comments (9)
tests/e2e/test_e2e_consistency_pipeline.py (1)
57-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared helpers to reduce duplication.
Both test functions share ~100 lines of near-identical code for agent execution (Step 1), trace verification (Step 2), and sync output monitoring (Step 3). Extracting these into helpers would prevent divergence and simplify future updates.
Suggested helpers:
_run_agent(script_path, project_name, timeout=90)→subprocess.CompletedProcess_verify_traces(phoenix_server, project_name)→str(trace count)_run_sync_monitor(process, timeout, verbose)→tuple[bool, int, str](resampling_ran, guideline_count, full_output)The
_consistency_analyzer_available()function (line 45) also duplicates_consistency_available()intest_e2e_mcp_consistency.pyandtest_e2e_smolagent_mcp.py— consider consolidating into a shared fixture inconftest.py.🤖 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/e2e/test_e2e_consistency_pipeline.py` around lines 57 - 399, Both E2E tests duplicate the same agent-run, Phoenix trace-check, and sync-log polling logic, so extract that repeated flow into shared helpers to keep the tests aligned. Move the Step 1/2/3 behavior in `test_e2e_consistency_pipeline` and `test_e2e_both_mode_smolagents` into helpers such as `_run_agent`, `_verify_traces`, and `_run_sync_monitor`, then have both tests call those helpers instead of inlining the subprocess and parsing code. Also consider centralizing `_consistency_analyzer_available()` with the other availability checks in a shared `conftest.py` fixture/helper so the consistency gating logic is defined once.altk_evolve/llm/guidelines/consistency_analyzer/sample_preprocessing.py (1)
647-693: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared parse-dispatch helper to eliminate duplicated if/elif chains.
The same six-branch
if/elifblock mappingresponse_typeto a parser function appears twice — once for sampled responses (lines 647–658) and once for the actual response (lines 681–692). Any new response type must be added in both places, risking divergence.♻️ Proposed refactor
+def _parse_single_response(response, response_type: str): + """Dispatch to the appropriate parser based on response_type.""" + if response_type == "code": + return parse_code_response(response) + elif response_type == "json": + return parse_json_response(response) + elif response_type == "react": + return parse_react_response(response) + elif response_type == "react_aw": + return parse_react_aw_response(response) + elif response_type == "thought_code": + return parse_thought_code_response(response) + elif response_type == "tool_calls": + return parse_tool_calls_response(response) + return response + def extract_parsed_responses_from_trajectory(trajectory:dict, config: dict)-> dict: ... for response in response_samples: - if agent_config["response_type"] == "code": - parsed_response = parse_code_response(response) - elif agent_config["response_type"] == "json": - parsed_response = parse_json_response(response) - ... + parsed_response = _parse_single_response(response, agent_config["response_type"]) ... actual_response = step.get("raw_response", "") - if agent_config["response_type"] == "code": - parsed_response = parse_code_response(actual_response) - elif agent_config["response_type"] == "json": - parsed_response = parse_json_response(actual_response) - ... + parsed_response = _parse_single_response(actual_response, agent_config["response_type"])🤖 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/consistency_analyzer/sample_preprocessing.py` around lines 647 - 693, The response-type parsing logic is duplicated in the sampled-response loop and the actual-response path, making future updates easy to miss. Extract the `response_type` to parser mapping into a shared helper in `sample_preprocessing.py` and have both the sampling block and the `step["raw_response"]` handling call it, so `parse_code_response`, `parse_json_response`, `parse_react_response`, `parse_react_aw_response`, `parse_tool_calls_response`, and `parse_thought_code_response` are all dispatched from one place.altk_evolve/llm/guidelines/consistency_analyzer/consistency_aggregator.py (1)
85-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring references
pmiaggregation mode but no implementation exists.
get_agg_fcnlists'pmi'in its docstring (line 90) and theConsistencyAggregatorclass docstring (line 114) mentions PMI support, but there is nopmibranch in the function. Either remove the reference or add a stub that raises a clear "not implemented" error.🤖 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/consistency_analyzer/consistency_aggregator.py` around lines 85 - 107, The aggregation selector in get_agg_fcn is inconsistent with its own documentation because it advertises pmi support but has no matching branch. Update get_agg_fcn and the ConsistencyAggregator docs so they agree: either remove pmi from the described modes or add an explicit pmi branch that raises a clear not-implemented error, and keep the existing mean, rms, geo_mean, and product handling unchanged.docs/consistency_guidelines_integration.md (1)
121-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifiers to fenced code blocks.
Three fenced code blocks (lines 121, 129, 162) lack language tags, triggering markdownlint MD040 warnings. Use
bashfor the CLI/ENV examples.Also applies to: 129-129, 162-162
🤖 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 `@docs/consistency_guidelines_integration.md` at line 121, The fenced code blocks in the consistency guidelines markdown are missing language specifiers, which triggers markdownlint MD040 warnings. Update each affected fenced block in this section to include the appropriate language tag, using bash for the CLI and environment example blocks, and ensure the same fix is applied consistently to the other affected fences in the document.Source: Linters/SAST tools
altk_evolve/llm/guidelines/consistency_analyzer/agent_config.yaml (1)
23-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out debug lines.
These two lines (
# response_type: tool_calls/# metric: jaccard) underAnyAgent_contentare leftover scratch with inconsistent indentation. Either delete them or convert to a proper YAML comment explaining the alternative config.🤖 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/consistency_analyzer/agent_config.yaml` around lines 23 - 24, Remove the leftover commented-out debug lines under AnyAgent_content in agent_config.yaml: the commented response_type and metric entries are scratch artifacts with inconsistent indentation. Either delete those comments entirely or rewrite them as a single properly indented YAML comment that clearly explains the alternative configuration, keeping the surrounding AnyAgent_content block tidy.altk_evolve/llm/guidelines/consistency_analyzer/single_step_consistency.py (1)
53-53: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider
>=instead of>for minimum sample threshold.
len(field_samples) > min_samplesrequires strictly more thanMIN_FRACTION * max_samplessamples. Withmax_samples=5,min_samples=2, so you need 3+ samples (60%) rather than 2+ (40%). If the intent is "at least half," use>=.🤖 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/consistency_analyzer/single_step_consistency.py` at line 53, The sample-threshold check in single_step_consistency.py is too strict because it uses `len(field_samples) > min_samples`, which excludes cases where the sample count exactly meets the minimum. Update the condition in the consistency analysis logic to use `>=` instead of `>` so the `field_samples` threshold in the relevant branch is inclusive and matches the intended minimum-sample behavior.altk_evolve/llm/guidelines/consistency_analyzer/consistency_metric.py (1)
456-462: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
np.triu_indicesinstead of filtering by!= 0.Filtering
flattened_array[flattened_array != 0]to extract the upper triangle is fragile — a legitimate cosine similarity of exactly0.0(orthogonal embeddings) would be silently excluded from the mean. Usenp.triu_indicesfor correct index-based extraction.♻️ Proposed refactor
# extract the upper triangle and zero-out the lower triangle - upper_half = np.triu(np.asarray(similarities), k=1) - flattened_array = upper_half.flatten() - # remove the zeroes from the lower triangle - embedding_pairwise_similarities = flattened_array[flattened_array != 0] + sim_array = np.asarray(similarities) + n = sim_array.shape[0] + embedding_pairwise_similarities = sim_array[np.triu_indices(n, k=1)] # aggregate over all pairwise similarities mean_embedding_similarity = np.mean(embedding_pairwise_similarities)🤖 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/consistency_analyzer/consistency_metric.py` around lines 456 - 462, The pairwise similarity aggregation in consistency_metric.py is incorrectly removing legitimate 0.0 cosine similarities by filtering `flattened_array != 0`, which can skew `mean_embedding_similarity`. Update the logic that builds `embedding_pairwise_similarities` to use `np.triu_indices` on the similarities array instead of flatten-and-filter, so the upper-triangle values are selected by index rather than by value. Keep the rest of the mean calculation the same, but make sure the extraction happens from the `similarities` matrix directly.altk_evolve/llm/guidelines/consistency_analyzer/inference_utils.py (1)
21-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMutable default argument
tools: list = [].Not currently mutated, but a common footgun if a future edit appends to
toolsin place — the empty list would be shared across all calls.🧹 Proposed fix
- tools: list = [], + tools: list | None = None,🤖 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/consistency_analyzer/inference_utils.py` around lines 21 - 30, The get_response_sampling function currently uses a mutable default for tools, which can be shared across calls if it is ever modified in place. Update the function signature to use a non-mutable default and initialize a fresh list inside get_response_sampling when tools is not provided, keeping the existing behavior for callers while preventing shared state.altk_evolve/frontend/mcp/mcp_server.py (1)
534-577: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated entity-construction logic between "regular" and "consistency" branches (and phoenix_sync.py).
Both branches build near-identical
Entity(...)list comprehensions differing only ingeneration_methodand the results source; the same pattern is repeated inphoenix_sync.py. Consider extracting a shared helper (e.g.,_build_guideline_entities(results, metadata_base, generation_method)).🤖 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 534 - 577, The guideline entity construction is duplicated between the regular and consistency branches, making the MCP server harder to maintain and also mirrored in phoenix_sync.py. Extract the shared `Entity(...)` list-building logic from the `guideline_entities` assembly into a helper such as `_build_guideline_entities(results, metadata_base, generation_method)` and use it in both `generate_guidelines` and `generate_consistency_guidelines` paths, keeping only the result source and generation method different.
🤖 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 556-560: The MCP trajectory payload is incomplete because it only
forwards messages and trace_id, so tool-calling steps lose model and tools
context compared with the Phoenix-sync path. Update the trajectory built in the
MCP save path around generate_consistency_guidelines/transform_trajectory_to_IR
to include the same richer fields (especially model and tools, plus any other
trajectory metadata already available) so tool_calls can be named and resampled
with the real tool schemas.
- Around line 536-577: Wrap the guideline generation in mcp_server.py with error
handling so failures in generate_guidelines or generate_consistency_guidelines
do not abort the whole request after the trajectory has already been persisted.
Add try/except around the regular and consistency branches in the guideline
entity assembly path, log or record the exception with task context (for example
task_id and guidelines_mode), and fall back to returning the saved
trajectory/read-back response with an empty or partial guideline_entities list.
Reference the generate_guidelines, generate_consistency_guidelines, and
guideline_entities assembly blocks when making the fix.
In `@altk_evolve/llm/guidelines/consistency_analyzer/consistency_analysis.py`:
- Around line 18-30: The aggregate trajectory uncertainty calculation in
consistency_analysis.py should handle the same -1 sentinel used for step-level
uncertainty instead of always subtracting from 1.0. Update the logic in the
return block that builds the trajectory summary so aggregate_step_consistency
values of -1 (and any missing/default sentinel) map to
aggregate_trajectory_uncertainty = -1, while valid consistency scores still use
1.0 - value. Keep the behavior consistent with the step processing in the same
function and avoid producing out-of-range values.
In `@altk_evolve/llm/guidelines/consistency_analyzer/consistency_metric.py`:
- Around line 26-38: The caching helpers `get_sentence_transformer_small` and
`get_sentence_transformer_large` are only updating a local parameter, so the
module-level model variables never retain the loaded `SentenceTransformer`
instance. Update these functions to assign to the actual shared state in
`consistency_metric.py` (for example by declaring the corresponding global model
variables or otherwise removing the shadowing), so
`get_metric_instance("sbert_small")` and the large-model path reuse the same
cached object instead of reloading on every call.
- Around line 506-522: The NumericFractionConsistencyMetric._get_distance
implementation currently returns a raw mean absolute difference, which can
exceed 1 and break the 1.0 - distance consistency calculation. Update
_get_distance in consistency_metric.py to normalize the computed distance into
the [0, 1] range before returning it, while preserving the no-samples fallback
of -1.0. Ensure get_consistency_and_distance continues to rely on this
normalized value so consistency scores stay within the module’s expected bounds.
In `@altk_evolve/llm/guidelines/consistency_analyzer/resampling.py`:
- Around line 66-102: The old-format CUGA resampling path in resampling.py is
unreachable because the early skip for missing llm_params returns before that
fallback can run. Update the step-processing logic in the resampling loop so the
legacy prompts-based handling is checked before any continue that skips steps
without llm_params. Keep the existing llm_params path for newer trajectories,
but make the old CUGA branch (the prompt_item/human_prompt_item handling)
reachable when step["prompts"] exists and llm_params is absent.
- Around line 61-64: The resampling loop in `resampling.py` is exiting too early
and the old-format CUGA fallback is unreachable. In the logic around the step
checks (including the `if "sampling" in step` guard and the `if "llm_params" not
in step` branch), replace the early exit with a per-step skip so later steps
still get processed, and restructure the CUGA handling so the legacy `else` path
can actually run when `llm_params` is absent. Keep the behavior localized inside
the resampling pass rather than terminating the whole function from the first
matching step.
In `@altk_evolve/llm/guidelines/consistency_analyzer/sample_preprocessing.py`:
- Around line 224-228: Replace the bare exception handlers in
sample_preprocessing.py with narrower catches. In the JSON parsing block around
the response cleanup and json.loads call, change the generic except clauses to
except Exception, or even better except json.JSONDecodeError for the parse step.
Also update the BaseException handler later in the same module to Exception so
SystemExit and KeyboardInterrupt are not swallowed.
In `@altk_evolve/llm/guidelines/consistency_analyzer/single_step_consistency.py`:
- Around line 126-129: The `tool_calls` path in `check_sample_validity` is
validating `raw_samples`, but `compute_step_consistency` and
`compute_json_step_consistency` actually use `parsed_samples`, so the checks are
out of sync. Update `check_sample_validity` to treat `tool_calls` like the
structured/JSON cases used by `compute_step_consistency`, and validate
`parsed_samples` instead of `raw_samples` so empty parsed tool-call samples are
rejected before consistency is computed.
In `@altk_evolve/llm/guidelines/consistency_analyzer/utils.py`:
- Around line 187-190: In compute_weighted_sum_consistency, avoid mutating
field["name"] when handling the list case; instead, compute a local normalized
name variable from field["name"] and use that for the field_consistencies lookup
and weight update. Keep the original field dict unchanged so callers reusing
step_cns_list do not see side effects.
In `@altk_evolve/sync/phoenix_sync.py`:
- Around line 812-885: The `_process_trajectory()` flow marks a trace as
processed before the guideline pipelines complete, so failures in
`generate_guidelines()` or `generate_consistency_guidelines()` can prevent
retries. Update the logic around the trajectory write and
`self.client.update_entities()` so the trace is only considered processed after
guideline generation succeeds, or record a retryable failure state instead; use
the existing `_process_trajectory`, `guideline_entities`, and
`_get_processed_trace_ids` flow to keep failed traces eligible for a later
retry.
In `@scripts/extract_trajectories.py`:
- Around line 150-154: The output handling in extract_trajectories.py is
incorrectly filtering assistant messages on content, which drops completion
spans that only have tool_calls. Update the output message loop in the extractor
to accept assistant output entries when tool call data is present under
llm.output_messages.{i}.message.tool_calls, and build the completion record from
the role plus tool_calls even when content is absent. Use the existing output
message parsing around the loop over output_indices and the messages.append path
to make the change without relying on content as the only condition.
In `@tests/e2e/test_e2e_consistency_pipeline.py`:
- Around line 45-52: The availability guard in _consistency_analyzer_available()
is importing the wrong module path, so it always fails and skips the E2E flow.
Update the importlib.import_module check to use the vendored
consistency_analyzer path under altk_evolve.llm.guidelines.consistency_analyzer,
matching the module path used by the other consistency tests.
In `@tests/e2e/test_e2e_smolagent_mcp.py`:
- Around line 136-180: The e2e smolagent MCP test can pass on stale
`guidelines_*.json` files left in `consistency_debug`, so clean that directory
(or at least remove existing guidelines artifacts) before running
`_run_smolagent_and_extract_messages()` and `save_trajectory`. Use the existing
`debug_dir` setup in `test_e2e_smolagent_mcp.py` and ensure the verification
after `Client.call_tool_mcp("save_trajectory", ...)` only sees files produced by
the current run.
---
Minor comments:
In `@altk_evolve/llm/guidelines/consistency_analyzer/agent_config.yaml`:
- Around line 7-8: The `agent_config.yaml` values are out of sync with the
documented consistency analyzer settings, specifically `max_samples` in the
`consistency_analyzer` config. Update either the YAML entry or the corresponding
documentation so the `max_samples` value matches across the `agent_config` and
the integration guide, and keep `max_steps` unchanged unless it is also intended
to align.
In `@altk_evolve/llm/guidelines/consistency_analyzer/sample_preprocessing.py`:
- Around line 48-51: `parse_code_response` in `sample_preprocessing.py`
mishandles responses without a triple-backtick fence because it slices using the
result of `find("```")` and `rfind("```")` even when no fence exists. Add an
early guard at the start of `parse_code_response` to detect the no-fence case
and return a safe result (such as the stripped original response) before the
existing fence-trimming logic runs. Keep the fix localized around the response
preprocessing block that currently removes text before and after the code fence.
In `@altk_evolve/llm/guidelines/consistency_analyzer/single_step_consistency.py`:
- Around line 174-182: The metric label in single_step_consistency.py is being
derived from metric_config even when alternates are resolved into this_config,
so the label can mismatch the actual consistency config. Update the metric
assignment in the consistency calculation block to read from the resolved
this_config (the value returned by find_matching_alternate) rather than the
original metric_config, and make sure the fallback logic still works when
alternates are absent.
In `@altk_evolve/llm/guidelines/consistency_analyzer/utils.py`:
- Line 55: The `find_matching_alternate` signature uses the wrong type for
`alternates`; it is iterated like a list and passed
`metric_config["alternates"]`, so update the annotation in
`find_matching_alternate` to reflect a list of alternates rather than a dict.
Make sure the parameter type matches the YAML-driven structure and keep the
return type unchanged if it still returns a single matching alternate.
In `@altk_evolve/llm/guidelines/consistency_guidelines.py`:
- Around line 219-245: The “Agent reasoning” path in the message formatting
logic assumes `step["content"]` is always a string, but Agents SDK assistant
messages can provide content as a list of blocks. Update the formatting in the
assistant-step loop to detect list content before the `len(content)`/slice
handling, and convert those blocks into readable text instead of using the list
repr. Keep the existing `tool_calls` handling in place, and make the change in
the same message-processing block that sets `step_type` and `this_step_text`.
- Around line 398-407: The constrained decoding check in consistency_guidelines
should explicitly skip Groq-backed models so this path matches the other
guideline flows. Update the logic around get_supported_openai_params,
supports_response_schema, and constrained_decoding_supported to gate out
llm_settings.custom_llm_provider == "groq" before enabling the JSON-schema
branch, keeping the behavior aligned with the rest of the guideline pipeline.
- Around line 302-325: The guideline generation flow in the completion call path
is mutating the module-level litellm.enable_json_schema_validation flag, which
can leak across concurrent calls. Update the logic around the completion()
invocations in the constrained_decoding_supported branch to pass
enable_json_schema_validation directly on completion() instead of setting
litellm.enable_json_schema_validation globally, and remove the global toggle
from both branches while preserving the existing clean_llm_response handling in
consistency_guidelines.py.
In `@docs/consistency_guidelines_integration.md`:
- Around line 58-60: The YAML example for the sampling settings is out of sync
with the shipped default, since it shows a different max_samples value than the
actual agent_config.yaml. Update the example in the consistency guidelines so
the max_samples entry matches the real default used by the configuration, and
keep the surrounding aggregation/max_steps context unchanged.
In `@README_DEMO_SCRIPTS.md`:
- Around line 79-82: Add a language identifier to each fenced code block flagged
by markdownlint MD040 in README_DEMO_SCRIPTS.md, including the examples near
trajectory_openai_agents_105121.json and the AuthenticationError snippet. Update
the affected markdown fences to use an appropriate tag such as text so the
blocks remain rendered correctly while satisfying the lint rule.
In `@run_openai_agents_demo_with_tips.sh`:
- Around line 71-76: The comment above the evolve sync command is misleading
because it mentions a --consistency flag that is not used by the `uv run evolve
sync phoenix` invocation. Update the script to either set
`EVOLVE_GUIDELINES_MODE=consistency` before calling `evolve sync phoenix` or
remove the outdated comment so it matches the actual
`generate_guidelines`/consistency behavior controlled by the environment
variable.
In `@tests/e2e/test_e2e_consistency_pipeline.py`:
- Around line 172-204: The consistency-sync polling loop uses
process.stdout.readline(), which can block forever before the timeout is checked
again. Update the loop in the consistency sync helper/test to use a non-blocking
readiness check (for example via select or equivalent) before reading from
stdout, so the timeout can still be enforced even when the subprocess produces
no output. Apply the same fix to the duplicated sync-waiting block referenced by
the other occurrence in this test file, and keep the existing guidance around
verbose_sync, resampling_ran, and the generated guidelines match handling
intact.
---
Nitpick comments:
In `@altk_evolve/frontend/mcp/mcp_server.py`:
- Around line 534-577: The guideline entity construction is duplicated between
the regular and consistency branches, making the MCP server harder to maintain
and also mirrored in phoenix_sync.py. Extract the shared `Entity(...)`
list-building logic from the `guideline_entities` assembly into a helper such as
`_build_guideline_entities(results, metadata_base, generation_method)` and use
it in both `generate_guidelines` and `generate_consistency_guidelines` paths,
keeping only the result source and generation method different.
In `@altk_evolve/llm/guidelines/consistency_analyzer/agent_config.yaml`:
- Around line 23-24: Remove the leftover commented-out debug lines under
AnyAgent_content in agent_config.yaml: the commented response_type and metric
entries are scratch artifacts with inconsistent indentation. Either delete those
comments entirely or rewrite them as a single properly indented YAML comment
that clearly explains the alternative configuration, keeping the surrounding
AnyAgent_content block tidy.
In `@altk_evolve/llm/guidelines/consistency_analyzer/consistency_aggregator.py`:
- Around line 85-107: The aggregation selector in get_agg_fcn is inconsistent
with its own documentation because it advertises pmi support but has no matching
branch. Update get_agg_fcn and the ConsistencyAggregator docs so they agree:
either remove pmi from the described modes or add an explicit pmi branch that
raises a clear not-implemented error, and keep the existing mean, rms, geo_mean,
and product handling unchanged.
In `@altk_evolve/llm/guidelines/consistency_analyzer/consistency_metric.py`:
- Around line 456-462: The pairwise similarity aggregation in
consistency_metric.py is incorrectly removing legitimate 0.0 cosine similarities
by filtering `flattened_array != 0`, which can skew `mean_embedding_similarity`.
Update the logic that builds `embedding_pairwise_similarities` to use
`np.triu_indices` on the similarities array instead of flatten-and-filter, so
the upper-triangle values are selected by index rather than by value. Keep the
rest of the mean calculation the same, but make sure the extraction happens from
the `similarities` matrix directly.
In `@altk_evolve/llm/guidelines/consistency_analyzer/inference_utils.py`:
- Around line 21-30: The get_response_sampling function currently uses a mutable
default for tools, which can be shared across calls if it is ever modified in
place. Update the function signature to use a non-mutable default and initialize
a fresh list inside get_response_sampling when tools is not provided, keeping
the existing behavior for callers while preventing shared state.
In `@altk_evolve/llm/guidelines/consistency_analyzer/sample_preprocessing.py`:
- Around line 647-693: The response-type parsing logic is duplicated in the
sampled-response loop and the actual-response path, making future updates easy
to miss. Extract the `response_type` to parser mapping into a shared helper in
`sample_preprocessing.py` and have both the sampling block and the
`step["raw_response"]` handling call it, so `parse_code_response`,
`parse_json_response`, `parse_react_response`, `parse_react_aw_response`,
`parse_tool_calls_response`, and `parse_thought_code_response` are all
dispatched from one place.
In `@altk_evolve/llm/guidelines/consistency_analyzer/single_step_consistency.py`:
- Line 53: The sample-threshold check in single_step_consistency.py is too
strict because it uses `len(field_samples) > min_samples`, which excludes cases
where the sample count exactly meets the minimum. Update the condition in the
consistency analysis logic to use `>=` instead of `>` so the `field_samples`
threshold in the relevant branch is inclusive and matches the intended
minimum-sample behavior.
In `@docs/consistency_guidelines_integration.md`:
- Line 121: The fenced code blocks in the consistency guidelines markdown are
missing language specifiers, which triggers markdownlint MD040 warnings. Update
each affected fenced block in this section to include the appropriate language
tag, using bash for the CLI and environment example blocks, and ensure the same
fix is applied consistently to the other affected fences in the document.
In `@tests/e2e/test_e2e_consistency_pipeline.py`:
- Around line 57-399: Both E2E tests duplicate the same agent-run, Phoenix
trace-check, and sync-log polling logic, so extract that repeated flow into
shared helpers to keep the tests aligned. Move the Step 1/2/3 behavior in
`test_e2e_consistency_pipeline` and `test_e2e_both_mode_smolagents` into helpers
such as `_run_agent`, `_verify_traces`, and `_run_sync_monitor`, then have both
tests call those helpers instead of inlining the subprocess and parsing code.
Also consider centralizing `_consistency_analyzer_available()` with the other
availability checks in a shared `conftest.py` fixture/helper so the consistency
gating logic is defined once.
🪄 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: 1c9cfa75-dcfd-4cdc-a90d-65dc16965298
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.env.example.gitignoreREADME_DEMO_SCRIPTS.mdaltk_evolve/frontend/mcp/mcp_server.pyaltk_evolve/llm/guidelines/consistency_analyzer/__init__.pyaltk_evolve/llm/guidelines/consistency_analyzer/agent_config.yamlaltk_evolve/llm/guidelines/consistency_analyzer/consistency_aggregator.pyaltk_evolve/llm/guidelines/consistency_analyzer/consistency_analysis.pyaltk_evolve/llm/guidelines/consistency_analyzer/consistency_metric.pyaltk_evolve/llm/guidelines/consistency_analyzer/inference_utils.pyaltk_evolve/llm/guidelines/consistency_analyzer/resampling.pyaltk_evolve/llm/guidelines/consistency_analyzer/sample_preprocessing.pyaltk_evolve/llm/guidelines/consistency_analyzer/single_step_consistency.pyaltk_evolve/llm/guidelines/consistency_analyzer/utils.pyaltk_evolve/llm/guidelines/consistency_guidelines.pyaltk_evolve/llm/guidelines/prompts/generate_consistency_guidelines.jinja2altk_evolve/sync/phoenix_sync.pydocs/consistency_guidelines_integration.mdpyproject.tomlrun_openai_agents_demo_with_tips.shscripts/extract_trajectories.pytests/e2e/conftest.pytests/e2e/test_e2e_consistency_pipeline.pytests/e2e/test_e2e_mcp_consistency.pytests/e2e/test_e2e_pipeline.pytests/e2e/test_e2e_smolagent_mcp.pytests/unit/test_cli.pytests/unit/test_consistency_analyzer.pytests/unit/test_consistency_guidelines.pytests/unit/test_mcp_server.pytests/unit/test_phoenix_sync.py
💤 Files with no reviewable changes (1)
- tests/e2e/test_e2e_pipeline.py
Trajectory is already durably persisted before guidelines run; a transient LLM/network error during generation should log a warning and return the trajectory readback rather than aborting the whole call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…step naming Without the OpenAI tools schema, transform_trajectory_to_IR always uses the AnyAgent prefix and skips attaching tool definitions to tool_calls steps, degrading resampling quality. tools is optional and JSON-encoded to stay MCP-protocol compatible. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…entinel When the aggregator returns -1 (no scoreable steps), 1.0 - (-1) produced 2.0 which is outside [0,1] and inconsistent with step-level handling. Mirror the same sentinel check used for step_uncertainty on line 18. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Parameter names shadowed the module-level globals, so assignments inside the functions only updated local variables and the global stayed None forever — causing a full model reload on every call. Drop the parameters and use global declarations instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… to [0,1) Raw mean absolute difference is unbounded, so 1.0 - distance could go negative for any difference > 1. Apply d / (1 + d) normalization which maps [0, ∞) → [0, 1) monotonically, keeping consistency scores in [0, 1]. Applied consistently in both _get_distance and get_distance_from_chosen_trajectory. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… branch Two bugs in resample_trajectory: - 'return trajectory' on an already-sampled step aborted the whole loop, leaving later steps unsampled; changed to 'continue' - The old-format CUGA else branch was unreachable dead code; removed and simplified the step dispatch to a flat guard + direct llm_params path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Bare except: and except BaseException: catch SystemExit/KeyboardInterrupt, masking critical signals. All three sites are JSON parsing contexts; replaced with except (json.JSONDecodeError, ValueError). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
check_sample_validity validated raw_samples for tool_calls, but compute_step_consistency reads parsed_samples for the same response type. Move tool_calls into the parsed_samples branch of check_sample_validity so validation catches missing parsed data before computation runs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…m_consistency List-to-string join was done in-place on the caller's dict, causing a surprising side effect if step_cns_list is reused. Use a local variable instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cceeds Previously the trajectory entity was written before guideline generation ran. If generation failed, the trace was already marked processed and would never be retried. Now the trajectory write is buffered and only committed after generation completes successfully, keeping failed traces eligible for retry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tories The output message loop only gated on content is not None, silently dropping assistant completion spans that carry tool_calls but no content. Mirror the same pattern used by the input loop: accept the message when tool_call keys are present and extract them into the OpenAI tool_calls format. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
consistency_analyzer is vendored under altk_evolve.llm.guidelines.consistency_analyzer; the top-level path never resolves so the guard always returned False and all consistency e2e tests were unconditionally skipped. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… test The debug directory is shared across e2e tests. Without clearing guidelines_*.json before the run, the completion assertion at line 177 could pass on artifacts from a previous test, giving a false positive. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
msg: dict on line 150 shadowed the msg variable from line 105 in the same function scope, triggering mypy's no-redef check. Drop the annotation since mypy can infer the type from the dict literal. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
altk_evolve/llm/guidelines/consistency_analyzer/single_step_consistency.py (1)
54-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPotential off-by-one:
>should likely be>=formin_samplesthreshold.
min_samplesis computed asint(MIN_FRACTION * max_samples)(line 184), whereMIN_FRACTION = 0.5. The strict>means you need more than 50% of samples, not at least 50%. For example, withmax_samples=2,min_samples=1, butlen(field_samples) > 1requires 2+ samples (100%). Fields with exactlymin_samplesparsed samples are silently skipped, potentially yielding undefined consistency (-1) when computation is feasible.If the intent is "at least MIN_FRACTION of samples must be parsed," use
>=:- if field_samples and len(field_samples) > min_samples and metric != "None": + if field_samples and len(field_samples) >= min_samples and metric != "None":🤖 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/consistency_analyzer/single_step_consistency.py` at line 54, The sample-threshold check in `single_step_consistency` is too strict because it uses `len(field_samples) > min_samples`, which skips fields that meet the intended minimum exactly. Update the condition in `single_step_consistency` to allow fields with at least the computed threshold by changing the comparison to include equality, and keep the existing `field_samples` and `metric != "None"` guards intact.
🧹 Nitpick comments (2)
tests/unit/test_consistency_analyzer.py (1)
15-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
@pytest.mark.unitmarkers to test classes.As per coding guidelines, unit tests should use the
pytest.mark.unitmarker. None of the test classes in this file carry the marker. Add it at the class level or configure it viapyproject.toml/conftest.pyif already handled globally.♻️ Example: adding the marker to a test class
+import pytest + + +@pytest.mark.unit class TestInvertListOfDictionaries: def test_basic_inversion(self):🤖 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_consistency_analyzer.py` around lines 15 - 180, The unit test classes in TestInvertListOfDictionaries, TestFlattenResponse, TestExtractFieldValuesFromResponses, TestFindMatchingAlternate, TestRescaleWeights, and TestComputeWeightedSumConsistency are missing the required pytest.mark.unit marker. Add the marker at the class level for each test class in test_consistency_analyzer.py, or confirm the marker is applied globally via existing pytest configuration if that is already the project’s approach.Source: Coding guidelines
altk_evolve/llm/guidelines/consistency_analyzer/resampling.py (1)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog denominator uses full step count instead of potentially truncated
steps.When
max_steps != -1,stepsis truncated but the progress log showslen(trajectory['steps'])(full count) as the denominator.🔧 Suggested fix
- logger.info(f"+++ Resampling step: {step['name']} ({j + 1}/{len(trajectory['steps'])})") + logger.info(f"+++ Resampling step: {step['name']} ({j + 1}/{len(steps)})")🤖 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/consistency_analyzer/resampling.py` at line 65, The resampling progress log in the loop over steps uses the full trajectory length as the denominator even when `steps` has been truncated by `max_steps`. Update the `logger.info` call in `resample` to use the length of the actual `steps` sequence being iterated so the progress count matches the processed subset. Keep the message format the same, but reference the truncated collection instead of `trajectory['steps']`.
🤖 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/llm/guidelines/consistency_analyzer/single_step_consistency.py`:
- Around line 120-126: The validation path is directly indexing
step["parsed_response"] when alternates are enabled, which can raise KeyError
instead of returning a false validity result. Update check_sample_validity to
verify the field exists before calling find_matching_alternate/flatten_response,
and return the existing invalid tuple with a clear message when it is missing;
apply the same guard in compute_step_consistency where the parsed response is
used downstream. Use the check_sample_validity and compute_step_consistency
logic to locate and wrap all parsed_response access with an existence check.
---
Outside diff comments:
In `@altk_evolve/llm/guidelines/consistency_analyzer/single_step_consistency.py`:
- Line 54: The sample-threshold check in `single_step_consistency` is too strict
because it uses `len(field_samples) > min_samples`, which skips fields that meet
the intended minimum exactly. Update the condition in `single_step_consistency`
to allow fields with at least the computed threshold by changing the comparison
to include equality, and keep the existing `field_samples` and `metric !=
"None"` guards intact.
---
Nitpick comments:
In `@altk_evolve/llm/guidelines/consistency_analyzer/resampling.py`:
- Line 65: The resampling progress log in the loop over steps uses the full
trajectory length as the denominator even when `steps` has been truncated by
`max_steps`. Update the `logger.info` call in `resample` to use the length of
the actual `steps` sequence being iterated so the progress count matches the
processed subset. Keep the message format the same, but reference the truncated
collection instead of `trajectory['steps']`.
In `@tests/unit/test_consistency_analyzer.py`:
- Around line 15-180: The unit test classes in TestInvertListOfDictionaries,
TestFlattenResponse, TestExtractFieldValuesFromResponses,
TestFindMatchingAlternate, TestRescaleWeights, and
TestComputeWeightedSumConsistency are missing the required pytest.mark.unit
marker. Add the marker at the class level for each test class in
test_consistency_analyzer.py, or confirm the marker is applied globally via
existing pytest configuration if that is already the project’s approach.
🪄 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: 75356c54-c06a-44df-a5f3-7c817742f145
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
altk_evolve/frontend/mcp/mcp_server.pyaltk_evolve/llm/guidelines/consistency_analyzer/consistency_aggregator.pyaltk_evolve/llm/guidelines/consistency_analyzer/consistency_analysis.pyaltk_evolve/llm/guidelines/consistency_analyzer/consistency_metric.pyaltk_evolve/llm/guidelines/consistency_analyzer/inference_utils.pyaltk_evolve/llm/guidelines/consistency_analyzer/resampling.pyaltk_evolve/llm/guidelines/consistency_analyzer/sample_preprocessing.pyaltk_evolve/llm/guidelines/consistency_analyzer/single_step_consistency.pyaltk_evolve/llm/guidelines/consistency_analyzer/utils.pyaltk_evolve/llm/guidelines/consistency_guidelines.pyaltk_evolve/sync/phoenix_sync.pypyproject.tomlscripts/extract_trajectories.pytests/e2e/test_e2e_consistency_pipeline.pytests/e2e/test_e2e_mcp_consistency.pytests/e2e/test_e2e_smolagent_mcp.pytests/unit/test_consistency_analyzer.pytests/unit/test_consistency_guidelines.pytests/unit/test_mcp_server.pytests/unit/test_phoenix_sync.py
💤 Files with no reviewable changes (1)
- altk_evolve/llm/guidelines/consistency_analyzer/inference_utils.py
🚧 Files skipped from review as they are similar to previous changes (14)
- altk_evolve/llm/guidelines/consistency_analyzer/consistency_analysis.py
- altk_evolve/frontend/mcp/mcp_server.py
- tests/unit/test_phoenix_sync.py
- tests/unit/test_consistency_guidelines.py
- tests/e2e/test_e2e_consistency_pipeline.py
- tests/e2e/test_e2e_mcp_consistency.py
- altk_evolve/sync/phoenix_sync.py
- tests/unit/test_mcp_server.py
- altk_evolve/llm/guidelines/consistency_analyzer/consistency_metric.py
- tests/e2e/test_e2e_smolagent_mcp.py
- altk_evolve/llm/guidelines/consistency_analyzer/utils.py
- altk_evolve/llm/guidelines/consistency_analyzer/consistency_aggregator.py
- scripts/extract_trajectories.py
- altk_evolve/llm/guidelines/consistency_analyzer/sample_preprocessing.py
'pmi' was listed in the get_agg_fcn docstring and ConsistencyAggregator class description but has no implementation; passing it falls through to the else-raise branch. Remove it from both docstrings so callers aren't misled into using it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…shape The documented score_card keys (trajectory_name, task_score, aggregate_consistency, step_consistencies) shared zero overlap with the actual keys returned by create_consistency_score_card (task, total_steps, aggregation, aggregate_trajectory_uncertainty, steps). Updated to match the real return value. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
With MAX_SAMPLES=2 and MIN_FRACTION=0.5, min_samples=1. The strict > required len(field_samples) > 1 (100% of samples), not the intended >=50%. Fields with exactly min_samples parsed samples were silently skipped, returning -1 consistency when computation was feasible. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… loops process.stdout.readline() blocks until a line is available, so the timeout check at the top of the loop was never reached if the subprocess stalled without producing output. Adding a select() poll (0.5s) makes the loop re-evaluate the timeout on every iteration and avoids hanging the test indefinitely if the sync process hangs silently. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolve conflicts in mcp_server.py and phoenix_sync.py: - Retain consistency-pipeline split (regular/consistency/both) with per-pipeline try/except guards from this branch - Add support=1 field introduced by the consolidation PR (#283) to guideline entity metadata in both files Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jayaramkr
requested changes
Jul 16, 2026
Collaborator
There was a problem hiding this comment.
Re-reviewed against the updated branch (now merged with today's main). This is a big step up — every blocker from the last round is genuinely fixed, and I re-ran each of my original repros to confirm, not just read the diff. Requesting changes only for one remaining correctness gap plus a few medium items; the details are in the inline comments and the bulk of the work here is solid.
My original findings — status (all verified by re-running my repros)
| Prior finding | Status |
|---|---|
B1 agent_config.yaml not packaged |
✅ Fixed — now ships in the wheel (package-data + MANIFEST.in) |
B2 .env ignored (raw os.environ) |
✅ Fixed — new GuidelinesSettings(BaseSettings); verified .env is honored |
B3 negative cosine → aggregator ValueError |
✅ Fixed — max(0.0, mean); confirmed NaN can't reach the >= 0 guard |
B4 jaccard("","") == 1.0 signal inversion |
✅ Fixed — returns 0.0; no collateral (empties are filtered upstream) |
B5 format_trajectory_data crashes (4 shapes) |
|
| H both-mode discards regular guidelines on consistency failure | ✅ Fixed — try/except; reproduced that regular guidelines + trajectory survive |
H MCP save_trajectory bare except swallowing errors |
✅ Fixed — per-pipeline try/except + logger.error(exc_info=True) |
H --guidelines-mode CLI flag missing |
✅ Fixed — restored, works end-to-end |
hardcoded thresholds / is_groq guard / clean_llm_response on both branches / no step cap / unguarded debug writes |
✅ All fixed |
The cosine clamp, the jaccard fix, and the NumericFraction pairwise-loop fix are all correct and introduce no new bug (verified by execution).
Still open / new (ranked)
- H1 (correctness, still open) —
agent_config.yamlstill defines only 3 of the 6 step names the IR produces.AnyAgent_tool_calls/*_othersteps get resampled (LLM cost paid) then silently dropped from the score card. Reproduced end-to-end on a smolagents-style trajectory. Details onagent_config.yaml. - H2 (structural) — CI still runs no pytest on
pull_request, and this PR adds a mypyignore_errors=true+ ruffE722/F841/E402carve-out forconsistency_analyzer/*. Net: the vendored package now has neither static nor dynamic checks on a PR. Details onpyproject.toml. (Repo-wide issue, not solely this PR's to fix — but worth a CI job that runs the 164 new tests.) - M1 —
format_trajectory_datadiscards the function name on non-JSON tool args (renders- call_7instead ofexecute_sql(...)). Inline onconsistency_guidelines.py. - M2 —
pyyaml/numpy/pandas/scipyare imported by the pipeline but declared in none of[project.dependencies](transitive-only). Inline onpyproject.toml. - M3 —
n=samplesprovider assumption: a provider that returns 1 choice → empty score card → signal-less guidelines. Now warns (good) but still proceeds. Inline oninference_utils.py. - M4 (test gap) — the both-mode failure-isolation fix (the H above) has no test using
side_effect=raisefor consistency inbothmode. The 11 new mode tests cover only happy-path dispatch andgeneration_methodtagging. Worth one regression test asserting regular guidelines survive a consistency exception. - Low — thresholds in
config/guidelines.pyaren't range-validated (inline); single-sampledistance = 2.0; dead always-Noneconsistency_stepsfield; unguardedsamples[0]in the numeric dispatch;resampling.pydict-path uses key-presence vs truthiness fortool_calls.
Claude's Verdict
Night-and-day from the first pass — the blockers are all resolved and verified. I'm marking request-changes for H1 (a real, reproducible correctness drop for non-OpenAI-schema tool-calling agents — exactly the smolagents case the e2e tests target) plus the medium items; none are large. Once agent_config.yaml covers the missing step names (or unmapped names log instead of dropping), and M1/M2 land, this is in good shape to merge.
gaodan-fang
requested changes
Jul 16, 2026
gaodan-fang
left a comment
gaodan-fang
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed the current head against main. Requesting changes for four newly verified integration issues; each inline comment includes a minimal fix and regression-test shape. These are in addition to the still-open step-configuration gap, and I avoided duplicating the existing M1/M3 threads.
Two step types were being added to the IR, resampled at LLM cost, and then silently dropped because agent_config.yaml has no entry for them: 1. Steps where _classify_step_response returns "other" (malformed or degenerate turns) — no meaningful resampling target exists. 2. AnyAgent tool_calls steps (tool_calls present but no OpenAI tools schema) — without the schema we cannot instruct the model to call tools, so any resample produces bogus results. Skipping both at IR construction time ensures the IR only ever contains OpenAIAgent_content, OpenAIAgent_tool_calls, and AnyAgent_content — exactly the three entries in agent_config.yaml. No YAML changes needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ailure in format_trajectory_data Previously a JSONDecodeError on tool call arguments fell back to just the call id, dropping both the function name and the raw arg string — the highest-signal tokens for a high-uncertainty step. Now the inner try/except only wraps the JSON parsing; on failure it falls back to the raw args string while still emitting the function name. The outer KeyError handler (missing 'function' key) still falls back to the call id as a last resort. Strengthened the corresponding test to assert both name and raw args appear in the output rather than just checking "Agent tool calls". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The consistency pipeline imports yaml (consistency_guidelines.py), numpy (consistency_aggregator.py, consistency_metric.py), pandas and scipy.stats (consistency_metric.py). These were only available transitively via arize-phoenix and sentence-transformers — a transitive dep drop would break consistency/both mode with an ImportError, caught and logged silently in MCP, leaving the operator with no signal. Declaring them explicitly makes the dependency durable. No install overhead in practice since all four are already in the resolved set. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…an 2 choices Previously a provider that ignores n>1 triggered only a warning, then returned 1 choice. Every downstream metric would hit its len(samples)<=1 guard, the score card would be empty, skip_on_no_uncertainty would short-circuit, and zero guidelines would be produced after paying for the full resampling budget — with only a single log line as evidence. Now: if len(choices) < 2, raise EvolveException immediately (the EvolveException re-raise bypasses the retry loop since retrying the same provider won't produce more choices). In both mode this lets the regular pipeline succeed and surfaces a real error to the operator. The count-mismatch warning is kept for the case where we got at least 2 choices but fewer than requested. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ions - Remove E722 and F841 from consistency_analyzer per-file-ignores in pyproject.toml; all violations were genuine and are now fixed in sample_preprocessing.py - Remove dead position-tracking variables (original_response, thought_pos, action_pos, action_input_pos, final_ans_pos) from sample_preprocessing.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sholds Add Field(ge=0.0, le=1.0) to both threshold fields and a model_validator that raises if low_uncertainty_threshold > high_uncertainty_threshold. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In "both" mode swallowing the exception is correct — regular guidelines still get written and the trace is processed. In "consistency" mode there are no regular guidelines, so swallowing causes the trajectory entity to be written (marking the trace processed) with zero guidelines generated. Re-raising lets the outer per-trace handler leave the trace unprocessed and eligible for retry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…llback phoenix_sync stores "unknown" when no model can be read from a span's attributes. Since "unknown" is a non-empty string, `model or fallback` would silently use it as a real model ID instead of falling back to llm_settings.guidelines_model. Normalize it to None at the point of ingestion so the fallback always applies. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ck model When the trajectory carries no model (or "unknown"), resampling falls back to llm_settings.guidelines_model. Without the provider, litellm has to guess the routing from the model name alone, which fails for providers that require an explicit custom_llm_provider (e.g. watsonx). Thread custom_llm_provider through resample_trajectory → get_response_sampling, but only inject it for steps that use the configured fallback — for steps with a per-trajectory model, let litellm infer the provider to avoid misrouting a model from a different provider. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…od on UPDATE
resolve_conflicts() was restoring metadata only for ADD events, leaving UPDATE
events with {} metadata. This caused generation_method (and all other provenance)
to be silently wiped whenever a guideline entity was updated via conflict resolution.
Fix: for UPDATE, start from the old entity's metadata. Additionally, union any
generation_method values from incoming new entities that were not explicitly ADD'd
(i.e. those merged into the update) into a generation_methods list so both regular
and consistency provenance survive a merge.
Update the existing test that asserted UPDATE metadata == {} (documenting the bug),
and add two new tests: one verifying metadata preservation and one verifying the
generation_methods union.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jayaramkr
requested changes
Jul 17, 2026
jayaramkr
left a comment
jayaramkr
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed against 05b70c4. This round resolved essentially everything from the last review, and I verified each fix by execution (not just reading the diff). Strong response — credit below. The remaining items are all new regressions introduced by the fixes themselves, not the original findings; they share a theme (the skip / normalize changes weren't propagated to every reader of the same data).
Last round's findings — all fixed ✅ (verified by re-running the repros)
| Prior finding | Status |
|---|---|
B1 agent_config.yaml not packaged |
✅ ships in the wheel |
B2 .env ignored |
✅ GuidelinesSettings; verified .env honored |
| B3 negative-cosine crash / B4 jaccard inversion | ✅ both correct, no collateral |
| M1 function name dropped on non-JSON tool args | ✅ name + raw args preserved (verified all 5 shapes) |
| M2 undeclared runtime deps | ✅ numpy/pandas/pyyaml/scipy now in [project.dependencies] |
| M3 silent degradation on short choice count | ✅ now raises EvolveException |
| H2 no pytest on PR + mypy/ruff carve-out | ✅ check-code.yaml now runs pytest on pull_request; ruff ignore narrowed to just E402 (bare-except/unused-var re-enabled) |
| threshold validation | ✅ out-of-range and high < low now rejected |
| both-mode failure isolation | ✅ verified: consistency failure preserves regular guidelines; consistency-only re-raises |
| conflict-resolution UPDATE wiped metadata | ✅ the metadata-preservation half is correct (and support-conservation on main is confirmed unaffected — consolidation bypasses resolve_conflicts) |
Problems introduced by the new fixes (ranked)
Details inline. Summary:
- CRITICAL — the skip-unscorable-steps fix makes IR
step_numberdense over scorable steps, butformat_trajectory_datastill numbers every assistant message positionally, so uncertainty markers land on the wrong step (even on skipped, never-scored steps). Silent misattribution — worse than the silent drop it replaced. - HIGH —
_can_segment_trajectorynow over-approves (single-function_calllist content), sosegment_trajectoryindices no longer map 1:1 to IR steps; valid subtasks get dropped andstep_rangeis misapplied. - HIGH — the "normalize unknown model" fix is incomplete:
transform_trajectory_to_IRre-reads the raw model and"unknown"survives, so resampling callslitellm.completion(model="unknown")→ fails → no consistency guidelines for any phoenix trace lacking model attributes (the exact case the fix targeted). - MEDIUM-HIGH — the
generation_methodunion pulls methods from unrelated/dropped new entities into every UPDATE (fabricated provenance). - MEDIUM — a mixed merge
pop()sgeneration_method(the key every consumer + the both-mode e2e test read) and writesgeneration_methods(read by nothing but its own new test), sometadata.get("generation_method")→None, violatingtest_e2e_mcp_consistency.py's contract.
The CRITICAL and the two HIGHs share one root cause — a change to how one producer numbers/normalizes steps wasn't mirrored in the other readers of that data — so they're a focused fix.
…conflict metadata
CRITICAL: transform_trajectory_to_IR now advances step_number for every
assistant message (including skipped ones) so IR step numbers are positional
and stay in sync with format_trajectory_data's positional counting and
segment_trajectory's indices. Previously, dense (scorable-only) numbering
caused uncertainty markers to land on the wrong step in the summary sent
to the guideline LLM.
HIGH (same root): n_steps split into n_scorable_steps (guards/segmentation
minimum) and n_positional_steps (subtask range validation), so valid
subtasks whose end step falls on a skipped message are no longer dropped.
HIGH: strip the "unknown" model sentinel in transform_trajectory_to_IR
(trajectory.get("model") or None doesn't catch it since "unknown" is
truthy). The fix in generate_consistency_guidelines was bypassed because
transform_trajectory_to_IR re-read the raw model into each IR step's
llm_params, causing resampling to call litellm with model="unknown".
MEDIUM-HIGH + MEDIUM (conflict resolution): drop the generation_method
union that over-attributed provenance from unrelated/dropped entities to
every UPDATE in a batch. Keep only the metadata-preservation fix (seed
UPDATE metadata from the old entity's stored metadata). This restores the
scalar generation_method key that every consumer and e2e test reads.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jayaramkr
approved these changes
Jul 17, 2026
jayaramkr
left a comment
jayaramkr
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed against ef80c5b. This round is a focused 54-line diff that resolves all five findings from the last review, and I verified each fix by execution in an isolated worktree (not just by reading the diff). Approving. Two non-blocking notes below.
Last round's findings — all fixed ✅ (re-ran each repro)
| Prior finding | Fix | Verified |
|---|---|---|
| CRITICAL — uncertainty markers attach to the wrong step | step_number += 1 now fires for every assistant message (including skipped ones), so IR numbering is positional and matches format_trajectory_data |
My original repro now flags Step 2 (the reasoning step), not Step 1 (the skipped tool call). New regression test test_skipped_step_advances_step_number covers both orderings |
HIGH — _can_segment_trajectory / segment↔IR mapping broken |
subtask ranges validated against n_positional_steps (count of all assistant turns) |
Under the guard, parse_openai_agents_trajectory's step count == n_positional_steps (3==3); indices line up |
HIGH — "unknown" model leaked past the fix into resampling |
sentinel stripped in transform_trajectory_to_IR (raw_model != "unknown") |
model="unknown" → IR llm_params.model=None → falls back correctly, so resampling no longer calls completion(model="unknown") |
MEDIUM-HIGH — generation_method union fabricates provenance |
union removed; UPDATE preserves the old entity's metadata (no cross-entity attribution) | Confirmed — the metadata-preservation half is kept; no fabricated provenance |
MEDIUM — writes unread generation_methods plural, breaks the both-mode e2e contract |
plural key gone; singular generation_method preserved |
test_e2e_mcp_consistency.py's generation_method in ("regular","consistency") assertion is satisfied again |
Baselines in the worktree: 56 unit tests pass (consistency + conflict-resolution), ruff clean, mypy clean.
I also checked the changes don't introduce new problems: IR step_number is now non-contiguous (skips consume a number), but no consumer indexes by it — the aggregator iterates steps by list position and consistency_analysis.py:23 carries step.get("step_number", i) straight through to the score card, so the gaps are harmless. And the provenance under-attribution on UPDATE (a consistency entity merged into an existing regular one stays tagged regular) is the deliberate, documented simplification suggested last round — acceptable.
Two non-blocking notes
- Stale docstring —
_can_segment_trajectorystill says segment indices "map 1:1 to IR step numbers." The real invariant is now positional-count alignment (the IR skips steps but still numbers them positionally). The code is correct; only the comment is imprecise. - Parallel gap in the regular pipeline — this PR guards the consistency path (
n_scorable_steps >= 2), butsegment_trajectoryin the regular pipeline still has no minimal-length guard, so a single-step trajectory can fan out into multiple near-identical segments there. Out of scope for this PR — filed separately as #293.
Nice work — the response to the last round was thorough and the fixes are clean. 🚀
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
5 participants
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.
Summary
Adds a new consistency-based guideline generation pipeline alongside the existing regular pipeline, controlled by a single
EVOLVE_GUIDELINES_MODEenv var.agent-consistencypackage intoaltk_evolve/llm/guidelines/consistency_analyzer/, replacing the pip path dependency. Includes a custominference_utils.pyLiteLLM adapter and strips dead code (predictive entropy, CUGA paths, embedding kernels).generate_consistency_guidelines()— new pipeline: transforms a raw trajectory to IR, resamples each step N times, scores consistency per step, then generates guidelines focused on the highest-uncertainty steps.EVOLVE_GUIDELINES_MODEenv var (regular|consistency|both) — replaces the--consistencyCLI flag and themodelparameter on thesave_trajectoryMCP tool. Both the Phoenix sync and MCP paths respect it; model selection always falls back tollm_settings.guidelines_model.EVOLVE_DEBUG_DIRenv var — when set, writes 5 debug artifacts per trajectory: raw trajectory, IR with consistency scores, consistency score card, consistency guidelines, regular guidelines.NumericFractionConsistencyMetric._get_distancepairwise loop (samples[j]→samples[i+1+j]); resampling model fallback blocked by literal"unknown"string; segmentation guard (n_steps >= 2) preventing LLM hallucinating subtasks on single-step trajectories.generation_methodmetadata — all auto-generated guidelines now carry"regular"or"consistency"in their metadata.Test plan
uv run pytest tests/unit/ --ignore=tests/unit/test_milvus_backend.py -q— 487 unit tests pass (includes 114 new tests for vendored consistency_analyzer modules, segmentation guard tests, updated CLI/MCP/phoenix_sync tests)uv run --env-file .env pytest tests/e2e/test_e2e_consistency_pipeline.py --run-e2e -v -s— consistency pipeline e2e (openai_agents + smolagents via Phoenix)uv run --env-file .env pytest tests/e2e/test_e2e_consistency_pipeline.py::test_e2e_both_mode_smolagents --run-e2e -v -s— both-mode e2e (regular + consistency in one sync pass)uv run --env-file .env pytest tests/e2e/test_e2e_mcp_consistency.py --run-e2e -v -s— consistency via MCPsave_trajectoryuv run --env-file .env pytest tests/e2e/test_e2e_smolagent_mcp.py --run-e2e -v -s— smolagents MCP pathSummary by CodeRabbit
regular,consistency, orbothmodes (including MCP save_trajectory and Phoenix sync tagging withgeneration_method).--guidelines-modeoption to the Phoenix sync CLI.