feat(guidelines): add consistency-fast pipeline, simplify configuration - #300
Open
evduester wants to merge 9 commits into
Open
feat(guidelines): add consistency-fast pipeline, simplify configuration#300evduester wants to merge 9 commits into
evduester wants to merge 9 commits into
Conversation
Resampling-based consistency guidelines (now "accurate") cost N extra LLM calls per trajectory, too expensive to run at volume. Adds "fast": the guideline-generation LLM self-judges each step's confidence in the same call that produces guidelines, at standard-mode cost. EVOLVE_CONSISTENCY_METHOD selects between them and now defaults to fast, since most volume use cases can't afford resampling on every trajectory. Also, while wiring this up: - Renames guidelines_mode regular/both -> standard/all for clearer terminology (breaking, no backward-compat aliases). - Moves the three accurate-method uncertainty tuning knobs (high/low threshold, skip_on_no_uncertainty) from env vars into agent_config.yaml, alongside the pipeline's other advanced settings, since they only apply to the accurate method. - Fixes two bugs surfaced during live testing: resampling silently dropped the configured LLM provider whenever a step recorded its own model, and uncertainty labels claimed "HIGH" for steps that only cleared the low threshold. - Rewrites the accurate-method prompt's analysis instructions, which had drifted into asking the LLM to re-judge step confidence from scratch (redundant with the uncertainty scores it's already given) and referenced an undefined "weak steps" term. - Documents EVOLVE_CONSISTENCY_METHOD, previously shipped without docs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPhoenix sync now uses ChangesGuideline generation modes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant GuidelinesSettings
participant PhoenixSync
participant MCPServer
participant ConsistencyGenerator
CLI->>GuidelinesSettings: set guidelines_mode and consistency_method
PhoenixSync->>ConsistencyGenerator: select fast or accurate method
MCPServer->>ConsistencyGenerator: generate consistency guidelines
ConsistencyGenerator-->>PhoenixSync: return guidelines and generation metadata
PhoenixSync-->>MCPServer: persist standard or consistency results
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 4
🧹 Nitpick comments (4)
tests/unit/test_consistency_guidelines.py (1)
560-566: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a normal
jsonimport in the helper.
__import__("json").dumps(payload)is hard to read. Importjsonat module scope or inside the helper.♻️ Proposed change
def _mock_completion_response(self, payload: dict): + import json from unittest.mock import MagicMock response = MagicMock() response.choices = [MagicMock()] - response.choices[0].message.content = __import__("json").dumps(payload) + response.choices[0].message.content = json.dumps(payload) return response🤖 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_guidelines.py` around lines 560 - 566, Update _mock_completion_response to use a normal json import instead of dynamically calling __import__("json"); add the import at module scope or within the helper, then call json.dumps(payload) when assigning message.content.altk_evolve/llm/guidelines/consistency_guidelines.py (1)
593-664: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared completion and parse block.
_generate_fast_guideline_resultrepeats the constrained/unconstrainedcompletioncalls,clean_llm_responsehandling, and the JSON repair plus validation ladder from_generate_guideline_result(Lines 376-426). Only the prompt, the hookpurpose, and the log text differ. Extract a helper that takes the rendered prompt and the purpose, then returns aGuidelineGenerationResult. This keeps future parsing fixes in one place.🤖 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 593 - 664, Extract the duplicated completion, response cleaning, JSON repair, and validation logic from _generate_fast_guideline_result and _generate_guideline_result into a shared helper accepting the rendered prompt and dispatch purpose, returning GuidelineGenerationResult. Preserve constrained and unconstrained decoding behavior, empty-response handling, and each caller’s existing purpose and log context while routing both functions through the helper.altk_evolve/frontend/mcp/mcp_server.py (1)
623-639: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a shared method-dispatch helper.
altk_evolve/sync/phoenix_sync.pycontains the sameconsistency_method == "fast"branch, the same deferred imports, and the sameconsistency-fast/consistencytag mapping. A single helper that returns the generator callable and the tag would keep the two entry points aligned when a third method is added.🤖 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 623 - 639, The consistency method dispatch in the guidelines flow duplicates the fast/default branch, deferred imports, and tag mapping found in phoenix_sync.py. Extract a shared helper that selects and returns the appropriate consistency generator callable and its "consistency-fast" or "consistency" tag, then update both entry points to use it so future methods remain aligned.tests/e2e/test_e2e_mcp_consistency.py (1)
29-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
_guidelines_envcontext manager in two e2e test files. Both files define the same helper that mutatesos.environand reinitializes theguidelines_settingssingleton. A future fix to the reload behavior must then be applied twice.
tests/e2e/test_e2e_mcp_consistency.py#L29-L51: move_guidelines_envinto a shared module, for exampletests/e2e/conftest.py, and import it here.tests/e2e/test_e2e_smolagent_mcp.py#L32-L55: delete the local copy and import the shared helper.🤖 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_mcp_consistency.py` around lines 29 - 51, Move the duplicated _guidelines_env context manager from tests/e2e/test_e2e_mcp_consistency.py lines 29-51 into a shared helper in tests/e2e/conftest.py, preserving its environment restoration and guidelines_settings reinitialization behavior; delete the local copy from tests/e2e/test_e2e_smolagent_mcp.py lines 32-55 and import the shared _guidelines_env in both test files.
🤖 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/cli/cli.py`:
- Around line 545-548: Apply Ruff formatting to the consistency_method option
declaration in altk_evolve/cli/cli.py (lines 545-548), the validator warning
calls in altk_evolve/config/guidelines.py (lines 18-30), and the new patch(...)
expressions in tests/unit/test_phoenix_sync.py (lines 1287-1317). Run uv run
ruff format . and stage updated tracked files with git add -u.
In `@altk_evolve/llm/guidelines/prompts/generate_consistency_guidelines.jinja2`:
- Line 16: Correct the misspelled word “truely” in the guideline text near
“Generate only what's needed” to “truly,” without changing the surrounding
wording.
In `@docs/guides/guidelines.md`:
- Around line 23-24: Update the fast method description in the guidelines table
to remove references to repeated runs and clarify that it self-assesses within
the generation call, running once per generated subtask when segmentation
succeeds or once for an unsegmented trajectory.
In `@tests/e2e/test_e2e_consistency_pipeline.py`:
- Around line 424-425: Update the error-pattern check in the fast consistency
test to also recognize “called on trajectory with no steps” and
“generate_consistency_guidelines_fast called with empty messages” as LLM-side
failures, alongside the existing RateLimitError and budget-exceeded patterns, so
llm_error_occurred is set before the final assertion.
---
Nitpick comments:
In `@altk_evolve/frontend/mcp/mcp_server.py`:
- Around line 623-639: The consistency method dispatch in the guidelines flow
duplicates the fast/default branch, deferred imports, and tag mapping found in
phoenix_sync.py. Extract a shared helper that selects and returns the
appropriate consistency generator callable and its "consistency-fast" or
"consistency" tag, then update both entry points to use it so future methods
remain aligned.
In `@altk_evolve/llm/guidelines/consistency_guidelines.py`:
- Around line 593-664: Extract the duplicated completion, response cleaning,
JSON repair, and validation logic from _generate_fast_guideline_result and
_generate_guideline_result into a shared helper accepting the rendered prompt
and dispatch purpose, returning GuidelineGenerationResult. Preserve constrained
and unconstrained decoding behavior, empty-response handling, and each caller’s
existing purpose and log context while routing both functions through the
helper.
In `@tests/e2e/test_e2e_mcp_consistency.py`:
- Around line 29-51: Move the duplicated _guidelines_env context manager from
tests/e2e/test_e2e_mcp_consistency.py lines 29-51 into a shared helper in
tests/e2e/conftest.py, preserving its environment restoration and
guidelines_settings reinitialization behavior; delete the local copy from
tests/e2e/test_e2e_smolagent_mcp.py lines 32-55 and import the shared
_guidelines_env in both test files.
In `@tests/unit/test_consistency_guidelines.py`:
- Around line 560-566: Update _mock_completion_response to use a normal json
import instead of dynamically calling __import__("json"); add the import at
module scope or within the helper, then call json.dumps(payload) when assigning
message.content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8d67941-664e-4489-9ffe-e913301735b4
📒 Files selected for processing (21)
altk_evolve/cli/cli.pyaltk_evolve/config/guidelines.pyaltk_evolve/frontend/mcp/mcp_server.pyaltk_evolve/llm/guidelines/consistency_analyzer/agent_config.yamlaltk_evolve/llm/guidelines/consistency_analyzer/resampling.pyaltk_evolve/llm/guidelines/consistency_guidelines.pyaltk_evolve/llm/guidelines/prompts/generate_consistency_guidelines.jinja2altk_evolve/llm/guidelines/prompts/generate_consistency_guidelines_fast.jinja2altk_evolve/sync/phoenix_sync.pydocs/guides/configuration.mddocs/guides/guidelines.mddocs/guides/low-code-tracing.mddocs/guides/phoenix-sync.mddocs/tutorials/guidelines-loop.mdtests/e2e/test_e2e_consistency_pipeline.pytests/e2e/test_e2e_mcp_consistency.pytests/e2e/test_e2e_smolagent_mcp.pytests/unit/test_conflict_resolution.pytests/unit/test_consistency_guidelines.pytests/unit/test_mcp_server.pytests/unit/test_phoenix_sync.py
Addresses CodeRabbit's docstring-coverage gate (was 56.86%, below the 80% threshold) by documenting the functions this PR added or modified that lacked one — validators and format_trajectory_data in production code, plus test methods that previously relied on descriptive names alone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Fix "truely" -> "truly" typo in the accurate-method prompt. - Correct the fast-method cost description in guidelines.md: it makes one LLM call per generated subtask (same cadence as standard mode), not always exactly one call per trajectory. - Fast e2e test's LLM-error detection now also matches "called on trajectory with no steps" and "called with empty messages", matching the accurate tests' patterns — without this, a trajectory with no scorable steps would fail the test's final assertion even though the sync completed cleanly. The formatting nitpick (cli.py/config/guidelines.py/test_phoenix_sync.py) was already resolved by an earlier commit; `ruff format .` confirms no changes needed across the repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
gaodan-fang
reviewed
Aug 4, 2026
gaodan-fang
left a comment
gaodan-fang
left a comment
Collaborator
There was a problem hiding this comment.
Review pass on the branch diff against main. These came out of an automated (Codex) review that I then verified by hand against the code; I've dropped its findings that turned out to be arguing with decisions your summary already documents, and confirmed the ones below.
The tool_calls one is the substantive find — I reproduced it, details inline. The other two inline notes are questions rather than defect claims.
One nit that has no diff line to hang off: .env.example:14 still reads
# EVOLVE_GUIDELINES_MODE=regular # Options: regular, consistency, both
Given the intentional no-aliases rename, both regular and both there now hit coerce_invalid_mode and silently become standard. Anyone uncommenting that line gets a warning-level log and not the mode they asked for. It's the only stale reference to the old values left in tracked source — everything else is consistent.
To be explicit about what I am not flagging: the regular/both → standard/all break and its lack of back-compat aliases both read as deliberate per your summary, so I've left them alone beyond that one stale comment.
.env.example still showed regular/both, which coerce_invalid_mode now silently rewrites to standard since the rename has no back-compat aliases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
EVOLVE_HIGH_UNCERTAINTY_THRESHOLD / EVOLVE_LOW_UNCERTAINTY_THRESHOLD / EVOLVE_SKIP_ON_NO_UNCERTAINTY moved to agent_config.yaml, but GuidelinesSettings' extra="ignore" meant a deployment still setting them lost the values with no signal. Warn at settings-load time instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_trajectory Assistant messages using the Chat Completions/Phoenix shape (content: null, tool_calls: [...]) fell through to the "skip empty assistant messages" branch, silently dropping the tool call. Only the Agents SDK/Responses API shape (content as a list of function_call items) was handled. Since generate_consistency_guidelines_fast (now the default) builds its prompt straight from this parser's steps_list, the self-judging LLM never saw tool use at all for native-protocol traces — the ones with trajectory["tools"] populated, which the accurate path treats as its best-case input. generate_guidelines (standard mode) shared the same gap. segment_trajectory calls the same parser and defines its step indices relative to its steps_list, so fixing it here keeps both callers aligned without touching the accurate path's separate _can_segment_trajectory guard. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… resampling Resampling in accurate mode forwards EVOLVE_CUSTOM_LLM_PROVIDER for every step regardless of the traced model, treating it as a single deployment-wide routing setting. That setting also defaults to "openai" merely from OPENAI_API_KEY/OPENAI_BASE_URL being present, not from an explicit operator choice — so a Phoenix project mixing providers (e.g. claude-* and gpt-* traces) can silently misroute non-OpenAI steps during accurate-mode resampling. Documents the contract: set EVOLVE_CUSTOM_LLM_PROVIDER explicitly per sync for mixed-provider namespaces, or use fast (the default), which never resamples. Also corrects the configuration reference table, which listed EVOLVE_CUSTOM_LLM_PROVIDER's default as None — it's actually openai whenever OPENAI_API_KEY/OPENAI_BASE_URL is set. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/guidelines.py`:
- Around line 86-117: Update the native tool-call argument formatting in the
tool-call extraction block to verify that parsed args are a dictionary before
iterating with .items(). Route JSON arrays, scalars, None, and non-string
non-mapping arguments through the existing raw-argument fallback, while
preserving formatted output for dictionary arguments. Add regression coverage
for a JSON array and a non-string argument.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7244d7a6-dc4d-479a-8683-90103ba4f130
📒 Files selected for processing (8)
.env.examplealtk_evolve/config/guidelines.pyaltk_evolve/llm/guidelines/guidelines.pyaltk_evolve/llm/guidelines/prompts/generate_consistency_guidelines.jinja2docs/guides/configuration.mddocs/guides/guidelines.mdtests/e2e/test_e2e_consistency_pipeline.pytests/unit/test_guidelines.py
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/guides/configuration.md
- docs/guides/guidelines.md
- altk_evolve/llm/guidelines/prompts/generate_consistency_guidelines.jinja2
- altk_evolve/config/guidelines.py
- tests/e2e/test_e2e_consistency_pipeline.py
…on-dict args json.loads(args_str) can validly return a list, scalar, or None (e.g. arguments="[1, 2, 3]"), and args.items() on those raises AttributeError, which the except (JSONDecodeError, TypeError) clause doesn't catch. That aborted parsing instead of falling back to the raw-argument string like every other malformed-input case here does. Reject non-dict args explicitly so they route through the same fallback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
gaodan-fang
reviewed
Aug 5, 2026
gaodan-fang
left a comment
gaodan-fang
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed at 06b32cd. Verified each fix by running it rather than reading it — three of the four are solid, and one is 90% there with a remaining edge I'd want closed before merge.
Verified fixed:
.env.example/ docs (dbabe95) — staleregular/bothgone; theEVOLVE_CUSTOM_LLM_PROVIDERtable entry now documents the implicit-default behavior, which is a better outcome than what I asked for.- Removed uncertainty env vars (ea5d895) — confirmed the warning fires with
EVOLVE_HIGH_UNCERTAINTY_THRESHOLD/EVOLVE_SKIP_ON_NO_UNCERTAINTYset, and stays silent when unset. Names the YAML keys and theconfig_path=route, so an operator hitting it knows where to go. Resolves my note. - Mixed-provider resampling (bc65272) — the docs answer stands on its own: it states the deployment-wide contract explicitly, names the implicit
openaidefault as the trigger, and gives two concrete outs. I checked thefastrecommendation holds —generate_consistency_guidelines_fasthas noresample_trajectorycall, so there is genuinely no routing step to misroute. Treating this as a documented design decision rather than a bug is the right call; resolved. - Malformed tool-call arguments (06b32cd) — probed JSON arrays, bare scalars, invalid JSON, pre-parsed dicts,
null, and atool_callsentry with nofunctionkey at all. None crash; each falls back to raw display orunknown. Good defensive follow-up, and it wasn't something I'd flagged.
Still open: one inline comment on guidelines.py — the native tool-call fix is elif-gated on content, so turns carrying both text and tool_calls still lose the call. Details and a repro there.
Checks on my end: pytest tests/unit → 660 passed, 8 skipped; ruff check and ruff format --check clean. (The two collection errors I hit were from unrelated untracked files in my own working tree, not this branch.)
…o carry text
An Anthropic-shape turn `[{"type":"text",...},{"type":"tool_use",...}]`
collapses into a single Chat Completions message with both a non-empty
`content` string and `tool_calls`. The text-content and tool_calls
branches were chained with elif, so the tool call was silently dropped
whenever text content was also present, yielding zero captured
function_calls despite tool use.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
2 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
consistency-fastguideline generation method: instead ofresampling each trajectory step and scoring uncertainty externally (the
existing
accuratepipeline), the guideline-generation LLM self-judgeseach step's confidence in the same call that produces guidelines — same
cost profile as
standardmode, no extra LLM calls. Controlled byEVOLVE_CONSISTENCY_METHOD(fast/accurate), which now defaults tofastsince most volume use cases can't afford resampling on everytrajectory.
EVOLVE_GUIDELINES_MODEvaluesregular/both→standard/allfor clearer terminology. Breaking change, no backward-compat aliases.
accurate-method uncertainty tuning knobs(
high_uncertainty_threshold,low_uncertainty_threshold,skip_on_no_uncertainty) from env vars intoagent_config.yaml, next tothe pipeline's other advanced settings, since they only apply to that one
method.
recorded its own model, causing misrouted/failed calls.
HIGH UNCERTAINTYfor steps that onlycleared the low threshold, not the high one — now correctly labeled
ELEVATED UNCERTAINTY.accurate-method prompt's analysis instructions, which haddrifted into asking the LLM to re-judge step confidence from scratch
(redundant with the uncertainty scores it's already given) and referenced
an undefined "weak steps" term.
EVOLVE_CONSISTENCY_METHOD, which had shipped without docs.Test plan
pytest tests/unit— 635 passedruff check/ruff format --check— cleanmypyon touched modules — cleanmkdocs build --strict— no new warningstest_e2e_consistency_pipeline.py,test_e2e_mcp_consistency.py,test_e2e_smolagent_mcp.py) against areal LLM backend — 12/12 real test cases pass (
fast,accurate, andallmode, including the new defaultall+fastcombination); the6
[asyncio-milvus]parametrizations error at fixture setup on apre-existing, unrelated missing
pymilvusdependency in thisenvironment
standard/consistency-accurate/consistency-fastguideline output on deliberately-confused agenttrajectories
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Summary by CodeRabbit
New Features
standard,consistency, andallguideline modes.fastandaccurateconsistency-generation methods, withfastas the default.Bug Fixes
Documentation