Rename tips references to guidelines#215
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 42 minutes and 23 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (48)
📝 WalkthroughWalkthroughRenames and migrates "tips" to "guidelines" across code, tests, prompts, docs, examples, and configs: adds a new guidelines schema, removes the tips schema, updates LLM prompts/handlers, config keys, CLI/client/sync/MCP flows, UI placeholders, and extensive test updates to use guideline terminology and fields. Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/guides/configuration.md (1)
22-50:⚠️ Potential issue | 🟠 MajorEnvironment variable for guidelines generation has been silently renamed without backward compatibility.
The field
guidelines_modelinaltk_evolve/config/llm.pybinds toEVOLVE_GUIDELINES_MODEL(via pydantic-settings withenv_prefix="EVOLVE_"), notEVOLVE_TIPS_MODELas documented. Without an alias, settingEVOLVE_TIPS_MODELwill be silently ignored and the default model used instead.The documentation at lines 22, 30, and 46 of
docs/guides/configuration.mdreferences the oldEVOLVE_TIPS_MODEL. Additionally,examples/low_code/manual_phoenix_demo.pystill readsEVOLVE_TIPS_MODELdirectly from the environment, which only works because it bypasses pydantic-settings.Either:
- Update the docs and examples to use
EVOLVE_GUIDELINES_MODEL, or- Add a
validation_aliasto the field to preserve backward compatibility forEVOLVE_TIPS_MODELand document both names with the old one marked as deprecated.(Note:
EVOLVE_CONFLICT_RESOLUTION_MODELandEVOLVE_FACT_EXTRACTION_MODELare correctly named in both code and docs.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/configuration.md` around lines 22 - 50, The docs/examples reference EVOLVE_TIPS_MODEL but the code field guidelines_model in altk_evolve/config/llm.py binds to EVOLVE_GUIDELINES_MODEL, so EVOLVE_TIPS_MODEL is ignored; either restore backward compatibility by adding a validation_alias to the guidelines_model field to accept the legacy name (so pydantic-settings will read EVOLVE_TIPS_MODEL as well), or update docs and examples to use EVOLVE_GUIDELINES_MODEL (and mark EVOLVE_TIPS_MODEL deprecated) — if you choose compatibility, modify guidelines_model in altk_evolve/config/llm.py to include a validation_alias for the legacy env name, and if you choose documentation, update docs/guides/configuration.md (lines referencing EVOLVE_TIPS_MODEL) and examples/low_code/manual_phoenix_demo.py to read EVOLVE_GUIDELINES_MODEL instead and add a deprecation note for EVOLVE_TIPS_MODEL.tests/e2e/test_e2e_pipeline.py (1)
210-248:⚠️ Potential issue | 🟡 MinorRequire a positive guideline count before passing.
generated 0 guidelinescurrently sets the success flag, so the E2E test can pass without any guideline being generated. This is also a good spot to finish the staletips_foundrename.Proposed fix
- tips_found = False + guidelines_found = False @@ match = re.search(r"generated (\d+) guidelines", line_stripped) if match: - count = match.group(1) - print(f"\n✅ SUCCESS: Generated {count} guidelines!") - tips_found = True + count = int(match.group(1)) + if count > 0: + print(f"\n✅ SUCCESS: Generated {count} guidelines!") + guidelines_found = True + break - break @@ - if not tips_found: + if not guidelines_found:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/test_e2e_pipeline.py` around lines 210 - 248, The test currently treats any "generated X guidelines" as success even when X is "0" and still uses the stale variable name tips_found; update the logic so you parse count = int(match.group(1)) and only set success when count > 0, rename tips_found to guidelines_found (and update all references including the final if not guidelines_found: and the success print) to make intent clear and ensure the test fails when zero guidelines are generated.altk_evolve/cli/cli.py (1)
353-397:⚠️ Potential issue | 🟡 MinorHandle unconfigured guideline clustering without a traceback.
client.cluster_guidelinesandclient.consolidate_guidelinescan raiseValueErrorwhen the cluster model is not configured (altk_evolve/frontend/client/evolve_client.py:89-126). The CLI currently misses that path.🐛 Proposed fix
try: clusters = client.cluster_guidelines(namespace, threshold=effective_threshold) except NamespaceNotFoundException: console.print(f"[red]Namespace '{namespace}' not found.[/red]") raise typer.Exit(1) + except ValueError as e: + console.print(f"[red]Clustering failed: {e}[/red]") + raise typer.Exit(1) @@ - except EvolveException as e: + except (EvolveException, ValueError) as e: console.print(f"[red]Consolidation failed: {e}[/red]") raise typer.Exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@altk_evolve/cli/cli.py` around lines 353 - 397, The CLI currently only catches NamespaceNotFoundException and EvolveException but misses ValueError thrown by client.cluster_guidelines and client.consolidate_guidelines when the clustering model is unconfigured; update the try/except blocks around client.cluster_guidelines(...) and client.consolidate_guidelines(...) to also catch ValueError and handle it cleanly (no traceback) by printing a user-facing message via console.print (e.g., explain that the clustering model is not configured and how to configure it) and exiting or returning appropriately; reference the functions cluster_guidelines and consolidate_guidelines and preserve existing handling for NamespaceNotFoundException and EvolveException, adding ValueError handling near those existing except clauses.altk_evolve/sync/phoenix_sync.py (1)
28-35:⚠️ Potential issue | 🟡 MinorBreaking API change:
SyncResult.tips_generated→guidelines_generated.
SyncResultis part of the public Python API exercised indocs/guides/phoenix-sync.md(the example code readsresult.guidelines_generated). Any external user or script that previously accessedresult.tips_generatedwill fail withAttributeErrorafter upgrading, with no deprecation path.Consider one of:
- Add a migration note to the top-level changelog entry for this PR (the current CHANGELOG changes only retroactively rewrite prior release notes — they don't document this rename as a breaking change for the upcoming release).
- Keep a transitional
tips_generatedalias (e.g., via a@property) that emits aDeprecationWarningfor one release cycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@altk_evolve/sync/phoenix_sync.py` around lines 28 - 35, SyncResult introduced a breaking rename from tips_generated to guidelines_generated; restore a transitional alias by adding a tips_generated property on the SyncResult dataclass that returns self.guidelines_generated and emits a DeprecationWarning (use the warnings module) so existing callers don’t break immediately; implement the property method on the SyncResult class (keep the dataclass fields unchanged) and include a short comment noting it’s temporary for one release cycle.CHANGELOG.md (1)
38-435:⚠️ Potential issue | 🟡 MinorConsider preserving historical changelog entries verbatim.
The changes retroactively rewrite past release notes (v1.0.0 through v1.0.6) to say "guideline" instead of "tip". However, the referenced commits (e.g.,
d373e7e,d2a4d0a,8ea2051,1154b4a,ce1ead3) and PRs (#56,#58,#60,#73,#124) were authored with "tip"/"tips" terminology in their actual commit messages and bodies. Rewriting past changelog entries detaches the file from the git history it summarizes and can mislead users who cross-reference a release note with the underlying commit.Consider either leaving historical entries untouched and only applying the rename going forward, or adding a one-line note at the top of the changelog explaining the terminology migration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 38 - 435, The changelog retroactively renames "tip"/"tips" to "guideline" across historical entries (seen in sections "v1.0.0" through "v1.0.6" and references like commits `d373e7e`, `d2a4d0a`, `8ea2051`, `1154b4a`, `ce1ead3`), which breaks fidelity to git history; revert those past release lines back to the original "tip"/"tips" wording (or preserve the original phrasing verbatim) and instead add a single-line migration note at the top of CHANGELOG.md explaining that terminology has been changed moving forward and mapping "tip" => "guideline" for clarity; update the entries under headings like "### Bug Fixes" and "### Features" in the historic release blocks (v1.0.0–v1.0.6) to match the original commit language while leaving new releases using "guideline".
🧹 Nitpick comments (8)
tests/unit/test_mcp_server.py (1)
26-31: Finish renaming stale test locals.
mock_tipandtip_entityare still tip terminology in a guideline-specific test. Renaming them keeps this test aligned with the PR-wide terminology change.Proposed cleanup
- mock_tip = MagicMock() - mock_tip.content = "Always write unit tests" - mock_tip.category = "testing" - mock_tip.rationale = "Helps catch bugs early" - mock_tip.trigger = "writing code" - mock_result.guidelines = [mock_tip] + mock_guideline = MagicMock() + mock_guideline.content = "Always write unit tests" + mock_guideline.category = "testing" + mock_guideline.rationale = "Helps catch bugs early" + mock_guideline.trigger = "writing code" + mock_result.guidelines = [mock_guideline] @@ - tip_entity = entities[0] - assert tip_entity.type == "guideline" + guideline_entity = entities[0] + assert guideline_entity.type == "guideline"Also applies to: 48-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_mcp_server.py` around lines 26 - 31, Rename outdated "tip" locals to guideline terminology: replace mock_tip with mock_guideline (and tip_entity with guideline_entity) where used to build mock_result.guidelines and in assertions; update the MagicMock assignments (mock_tip.content, .category, .rationale, .trigger) to use the new variable name and update any references at the other occurrence (around the second instance noted) so tests consistently use mock_guideline/mock_guideline_entity and mock_result.guidelines.tests/platform_integrations/test_entity_io_core.py (1)
49-58: Prefer the shared platform fixture/helper for filesystem setup.These changed collision and load fixtures still create files inline. Please use or extend the
conftest.pyhelper fixtures for this test data instead of manualtouch()/write_text()setup.As per coding guidelines, “Use fixtures from
conftest.pyto create test data (bob_fixtures, file_assertions, etc.) - do not manually create files”.Also applies to: 144-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/platform_integrations/test_entity_io_core.py` around lines 49 - 58, Replace the inline filesystem setup in tests test_increments_suffix_on_collision and test_keeps_incrementing (which currently use temp_project_dir / "my-guideline".touch() and similar) with the shared fixtures/helpers from conftest.py (e.g., use the project/file creation fixture such as bob_fixtures or the file-creation helper used elsewhere) so the test data is created via the reusable fixture instead of manual touch()/write_text(); keep the assertions that call entity_io.unique_filename unchanged and ensure the fixture produces the same filenames ("my-guideline.md" and "my-guideline-2.md") so the expected paths (temp_project_dir / "my-guideline-2.md" and / "my-guideline-3.md") still match.tests/platform_integrations/test_retrieve.py (1)
39-47: Use the platform test-data fixture/helper for entity files.These changed setups still create markdown fixtures inline. Please route guideline fixture creation through the repository’s
conftest.pyhelpers so platform integration test data stays centralized.As per coding guidelines, “Use fixtures from
conftest.pyto create test data (bob_fixtures, file_assertions, etc.) - do not manually create files”.Also applies to: 108-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/platform_integrations/test_retrieve.py` around lines 39 - 47, The test is creating entity markdown files inline using variables own_dir and sub_dir instead of the central test-data helpers; replace the manual mkdir/write_text blocks in tests/platform_integrations/test_retrieve.py with the conftest fixtures (e.g. use the provided bob_fixtures or other test-data helper functions) to create the owned and subscribed guideline entities and their metadata, and use file_assertions if assertions about file presence/content are needed; update both occurrences (the owned entity block and the subscribed entity block) to call the existing fixture helper APIs rather than manually writing files.tests/platform_integrations/test_sync.py (1)
152-174: Finish the local rename from tip to guideline.
tip_onenow points atguideline-one.md; renaming it keeps this test aligned with the rest of the PR.♻️ Proposed cleanup
- tip_one = p["evolve_dir"] / "entities" / "subscribed" / "alice" / "guideline" / "guideline-one.md" - assert tip_one.exists() + guideline_one = p["evolve_dir"] / "entities" / "subscribed" / "alice" / "guideline" / "guideline-one.md" + assert guideline_one.exists() @@ - assert not tip_one.exists() + assert not guideline_one.exists()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/platform_integrations/test_sync.py` around lines 152 - 174, Rename the local test variable `tip_one` to `guideline_one` in tests/platform_integrations/test_sync.py and update every usage (the Path construction, the initial exists() assert and the final not exists() assert) so the name matches the rest of the PR; ensure references to `tip_one` (in the Path assignment and both asserts) are replaced with `guideline_one` to complete the local rename from "tip" to "guideline".altk_evolve/frontend/api/routes.py (1)
216-218: Rename the staletip_metalocal.This branch now validates
Guideline; keeping the oldtipname makes the rename incomplete and can confuse future schema work.♻️ Proposed cleanup
- tip_meta = {k: v for k, v in req.metadata.items() if k != "content"} - Guideline(content=req.content, **tip_meta) + guideline_meta = {k: v for k, v in req.metadata.items() if k != "content"} + Guideline(content=req.content, **guideline_meta)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@altk_evolve/frontend/api/routes.py` around lines 216 - 218, The local variable tip_meta is a stale name now that Guideline is being validated; rename tip_meta to a clearer name (e.g., guideline_meta or guideline_kwargs) in the block where you build metadata from req.metadata and where you pass it into Guideline(content=req.content, **...), update any other references in the same scope to use the new name, and run tests to ensure no remaining references to tip_meta remain (look for usages around the Guideline call and any subsequent code in routes.py).tests/unit/test_cli.py (1)
655-671: Assert the renamed result label, not only the count.The
20assertion would still pass if the CLI accidentally printed a stale “Tips generated” row. Add an explicit label check for this rename.🧪 Proposed test hardening
assert result.exit_code == 0 assert "Sync Results" in result.stdout + assert "Guidelines generated" in result.stdout assert "10" in result.stdout # processed assert "5" in result.stdout # skipped assert "20" in result.stdout # guidelines_generated🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_cli.py` around lines 655 - 671, In test_sync_phoenix_displays_results, the test currently only checks numeric counts; update it to assert the explicit renamed label appears (e.g., check that "Guidelines generated" is present in result.stdout after runner.invoke(app, ["sync", "phoenix"]) and that the line showing 20 is associated with that label), and optionally assert the old label (e.g., "Tips generated") is not present to prevent false positives; reference the test function test_sync_phoenix_displays_results and the PhoenixSync mock used by runner.invoke/app to locate where to add these assertions.tests/unit/test_phoenix_sync.py (1)
562-587: Rename staletip-prefixed locals for consistency.The test logic and assertions were updated to "guidelines", but several local variables still use the old
tipnaming:mock_tip1,mock_tip2,tip_update_call,tip_entities(here and the similarmock_tipat lines 630-634). This creates a confusing mix of terminology in the same test. Since this is an all-in rename PR, finishing the rename in these locals would keep the file consistent.♻️ Proposed rename
- # Create mock Guideline objects with required attributes - mock_tip1 = MagicMock() - mock_tip1.content = "Guideline 1 content" - mock_tip1.category = "strategy" - mock_tip1.rationale = "Guideline 1 rationale" - mock_tip1.trigger = "Guideline 1 trigger" - mock_tip2 = MagicMock() - mock_tip2.content = "Guideline 2 content" - mock_tip2.category = "optimization" - mock_tip2.rationale = "Guideline 2 rationale" - mock_tip2.trigger = "Guideline 2 trigger" - mock_generate_guidelines.return_value = GuidelineGenerationResult(guidelines=[mock_tip1, mock_tip2], task_description="Hello") + # Create mock Guideline objects with required attributes + mock_guideline1 = MagicMock() + mock_guideline1.content = "Guideline 1 content" + mock_guideline1.category = "strategy" + mock_guideline1.rationale = "Guideline 1 rationale" + mock_guideline1.trigger = "Guideline 1 trigger" + mock_guideline2 = MagicMock() + mock_guideline2.content = "Guideline 2 content" + mock_guideline2.category = "optimization" + mock_guideline2.rationale = "Guideline 2 rationale" + mock_guideline2.trigger = "Guideline 2 trigger" + mock_generate_guidelines.return_value = GuidelineGenerationResult( + guidelines=[mock_guideline1, mock_guideline2], task_description="Hello" + ) result = phoenix_sync.sync(limit=10) assert result.processed == 1 assert result.guidelines_generated == 2 phoenix_sync.client.update_entities.assert_called() # Verify provenance metadata is persisted in guideline entities - tip_update_call = phoenix_sync.client.update_entities.call_args_list[-1] - tip_entities = tip_update_call.kwargs["entities"] - assert all(e.metadata.get("task_description") == "Hello" for e in tip_entities) - assert all(e.metadata.get("source_task_id") == "t1" for e in tip_entities) - assert all(e.metadata.get("source_span_id") == "s1" for e in tip_entities) - assert all(e.metadata.get("creation_mode") == "auto-phoenix" for e in tip_entities) + guideline_update_call = phoenix_sync.client.update_entities.call_args_list[-1] + guideline_entities = guideline_update_call.kwargs["entities"] + assert all(e.metadata.get("task_description") == "Hello" for e in guideline_entities) + assert all(e.metadata.get("source_task_id") == "t1" for e in guideline_entities) + assert all(e.metadata.get("source_span_id") == "s1" for e in guideline_entities) + assert all(e.metadata.get("creation_mode") == "auto-phoenix" for e in guideline_entities)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_phoenix_sync.py` around lines 562 - 587, Rename the stale "tip"-prefixed local variables in this test to "guideline" for consistency: change mock_tip1/mock_tip2 to mock_guideline1/mock_guideline2 (and any other mock_tip at lines ~630-634), and rename tip_update_call and tip_entities to guideline_update_call and guideline_entities; update their usages in the assertions that check phoenix_sync.client.update_entities call args and the metadata checks (references: mock_generate_guidelines return GuidelineGenerationResult, phoenix_sync.sync, phoenix_sync.client.update_entities) so the variable names match the updated "guidelines" terminology throughout the test.tests/unit/test_combine_guidelines.py (1)
38-64: StaleTIPSidentifiers remaining after rename.The constant
SAMPLE_TIPS(line 38) and the test methodtest_combine_cluster_returns_tips(line 64) still carry the old terminology even though the file, classes, and assertions were renamed toguidelines. Consider finishing the rename for consistency.♻️ Proposed rename
-SAMPLE_TIPS = [ +SAMPLE_GUIDELINES = [ { "content": "Use retry logic for flaky APIs", @@ ] @@ - def test_combine_cluster_returns_tips(self, _mock_params, _mock_schema, mock_completion): - mock_completion.return_value = _mock_completion_response(SAMPLE_TIPS) + def test_combine_cluster_returns_guidelines(self, _mock_params, _mock_schema, mock_completion): + mock_completion.return_value = _mock_completion_response(SAMPLE_GUIDELINES)(Also update the remaining
SAMPLE_TIPSreferences at lines 87 and 114.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_combine_guidelines.py` around lines 38 - 64, Rename the leftover TIPS identifiers to guidelines: change the constant SAMPLE_TIPS to SAMPLE_GUIDELINES and update all usages (including assertions and variables inside TestCombineCluster and helper references) and rename the test method test_combine_cluster_returns_tips to test_combine_cluster_returns_guidelines (and update any other occurrences of SAMPLE_TIPS at the remaining places referenced in the file). Ensure imports/patches still reference altk_evolve.llm.guidelines.clustering and that any variable names inside the test (e.g., mocks or expected values) reflect SAMPLE_GUIDELINES so names are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/guides/phoenix-sync.md`:
- Around line 129-133: The ASCII diagram's third box ("Generate Guidelines") is
wider than the others causing misalignment; update the diagram so all three
boxes share the same interior width by either expanding the top/bottom borders
(e.g., change the sibling box borders like "┌───────────────┐" /
"└───────┬───────┘" to match the wider width) or shorten the label to fit the
existing 15-character interior (e.g., "Gen Guidelines" or "Guidelines" trimmed)
so the lines containing "Deduplicate", "Convert to OpenAI format", and "Generate
Guidelines" render with consistent box widths.
In `@examples/low_code/litellm_demo.py`:
- Around line 13-16: The print statement currently always shows
llm_settings.guidelines_model while the completion call may use the
EVOLVE_EXAMPLE_AGENT_MODEL override; fix this by computing the actual model
string once (e.g., model = os.environ.get("EVOLVE_EXAMPLE_AGENT_MODEL") or
llm_settings.guidelines_model) and then use that variable in both the print and
the completion(...) call so both log and request show the same model; update
references to llm_settings.guidelines_model and the completion(...) invocation
accordingly.
In `@README.md`:
- Around line 136-141: The phrase "full audibility" in the "Guideline
Provenance" section is a typo; change it to "auditability" so the sentence reads
that source_task_id provides full auditability; update the README.md text under
the "Guideline Provenance" heading and verify surrounding mentions of
`metadata`, `creation_mode`, and `source_task_id` remain consistent.
---
Outside diff comments:
In `@altk_evolve/cli/cli.py`:
- Around line 353-397: The CLI currently only catches NamespaceNotFoundException
and EvolveException but misses ValueError thrown by client.cluster_guidelines
and client.consolidate_guidelines when the clustering model is unconfigured;
update the try/except blocks around client.cluster_guidelines(...) and
client.consolidate_guidelines(...) to also catch ValueError and handle it
cleanly (no traceback) by printing a user-facing message via console.print
(e.g., explain that the clustering model is not configured and how to configure
it) and exiting or returning appropriately; reference the functions
cluster_guidelines and consolidate_guidelines and preserve existing handling for
NamespaceNotFoundException and EvolveException, adding ValueError handling near
those existing except clauses.
In `@altk_evolve/sync/phoenix_sync.py`:
- Around line 28-35: SyncResult introduced a breaking rename from tips_generated
to guidelines_generated; restore a transitional alias by adding a tips_generated
property on the SyncResult dataclass that returns self.guidelines_generated and
emits a DeprecationWarning (use the warnings module) so existing callers don’t
break immediately; implement the property method on the SyncResult class (keep
the dataclass fields unchanged) and include a short comment noting it’s
temporary for one release cycle.
In `@CHANGELOG.md`:
- Around line 38-435: The changelog retroactively renames "tip"/"tips" to
"guideline" across historical entries (seen in sections "v1.0.0" through
"v1.0.6" and references like commits `d373e7e`, `d2a4d0a`, `8ea2051`, `1154b4a`,
`ce1ead3`), which breaks fidelity to git history; revert those past release
lines back to the original "tip"/"tips" wording (or preserve the original
phrasing verbatim) and instead add a single-line migration note at the top of
CHANGELOG.md explaining that terminology has been changed moving forward and
mapping "tip" => "guideline" for clarity; update the entries under headings like
"### Bug Fixes" and "### Features" in the historic release blocks
(v1.0.0–v1.0.6) to match the original commit language while leaving new releases
using "guideline".
In `@docs/guides/configuration.md`:
- Around line 22-50: The docs/examples reference EVOLVE_TIPS_MODEL but the code
field guidelines_model in altk_evolve/config/llm.py binds to
EVOLVE_GUIDELINES_MODEL, so EVOLVE_TIPS_MODEL is ignored; either restore
backward compatibility by adding a validation_alias to the guidelines_model
field to accept the legacy name (so pydantic-settings will read
EVOLVE_TIPS_MODEL as well), or update docs and examples to use
EVOLVE_GUIDELINES_MODEL (and mark EVOLVE_TIPS_MODEL deprecated) — if you choose
compatibility, modify guidelines_model in altk_evolve/config/llm.py to include a
validation_alias for the legacy env name, and if you choose documentation,
update docs/guides/configuration.md (lines referencing EVOLVE_TIPS_MODEL) and
examples/low_code/manual_phoenix_demo.py to read EVOLVE_GUIDELINES_MODEL instead
and add a deprecation note for EVOLVE_TIPS_MODEL.
In `@tests/e2e/test_e2e_pipeline.py`:
- Around line 210-248: The test currently treats any "generated X guidelines" as
success even when X is "0" and still uses the stale variable name tips_found;
update the logic so you parse count = int(match.group(1)) and only set success
when count > 0, rename tips_found to guidelines_found (and update all references
including the final if not guidelines_found: and the success print) to make
intent clear and ensure the test fails when zero guidelines are generated.
---
Nitpick comments:
In `@altk_evolve/frontend/api/routes.py`:
- Around line 216-218: The local variable tip_meta is a stale name now that
Guideline is being validated; rename tip_meta to a clearer name (e.g.,
guideline_meta or guideline_kwargs) in the block where you build metadata from
req.metadata and where you pass it into Guideline(content=req.content, **...),
update any other references in the same scope to use the new name, and run tests
to ensure no remaining references to tip_meta remain (look for usages around the
Guideline call and any subsequent code in routes.py).
In `@tests/platform_integrations/test_entity_io_core.py`:
- Around line 49-58: Replace the inline filesystem setup in tests
test_increments_suffix_on_collision and test_keeps_incrementing (which currently
use temp_project_dir / "my-guideline".touch() and similar) with the shared
fixtures/helpers from conftest.py (e.g., use the project/file creation fixture
such as bob_fixtures or the file-creation helper used elsewhere) so the test
data is created via the reusable fixture instead of manual touch()/write_text();
keep the assertions that call entity_io.unique_filename unchanged and ensure the
fixture produces the same filenames ("my-guideline.md" and "my-guideline-2.md")
so the expected paths (temp_project_dir / "my-guideline-2.md" and /
"my-guideline-3.md") still match.
In `@tests/platform_integrations/test_retrieve.py`:
- Around line 39-47: The test is creating entity markdown files inline using
variables own_dir and sub_dir instead of the central test-data helpers; replace
the manual mkdir/write_text blocks in
tests/platform_integrations/test_retrieve.py with the conftest fixtures (e.g.
use the provided bob_fixtures or other test-data helper functions) to create the
owned and subscribed guideline entities and their metadata, and use
file_assertions if assertions about file presence/content are needed; update
both occurrences (the owned entity block and the subscribed entity block) to
call the existing fixture helper APIs rather than manually writing files.
In `@tests/platform_integrations/test_sync.py`:
- Around line 152-174: Rename the local test variable `tip_one` to
`guideline_one` in tests/platform_integrations/test_sync.py and update every
usage (the Path construction, the initial exists() assert and the final not
exists() assert) so the name matches the rest of the PR; ensure references to
`tip_one` (in the Path assignment and both asserts) are replaced with
`guideline_one` to complete the local rename from "tip" to "guideline".
In `@tests/unit/test_cli.py`:
- Around line 655-671: In test_sync_phoenix_displays_results, the test currently
only checks numeric counts; update it to assert the explicit renamed label
appears (e.g., check that "Guidelines generated" is present in result.stdout
after runner.invoke(app, ["sync", "phoenix"]) and that the line showing 20 is
associated with that label), and optionally assert the old label (e.g., "Tips
generated") is not present to prevent false positives; reference the test
function test_sync_phoenix_displays_results and the PhoenixSync mock used by
runner.invoke/app to locate where to add these assertions.
In `@tests/unit/test_combine_guidelines.py`:
- Around line 38-64: Rename the leftover TIPS identifiers to guidelines: change
the constant SAMPLE_TIPS to SAMPLE_GUIDELINES and update all usages (including
assertions and variables inside TestCombineCluster and helper references) and
rename the test method test_combine_cluster_returns_tips to
test_combine_cluster_returns_guidelines (and update any other occurrences of
SAMPLE_TIPS at the remaining places referenced in the file). Ensure
imports/patches still reference altk_evolve.llm.guidelines.clustering and that
any variable names inside the test (e.g., mocks or expected values) reflect
SAMPLE_GUIDELINES so names are consistent.
In `@tests/unit/test_mcp_server.py`:
- Around line 26-31: Rename outdated "tip" locals to guideline terminology:
replace mock_tip with mock_guideline (and tip_entity with guideline_entity)
where used to build mock_result.guidelines and in assertions; update the
MagicMock assignments (mock_tip.content, .category, .rationale, .trigger) to use
the new variable name and update any references at the other occurrence (around
the second instance noted) so tests consistently use
mock_guideline/mock_guideline_entity and mock_result.guidelines.
In `@tests/unit/test_phoenix_sync.py`:
- Around line 562-587: Rename the stale "tip"-prefixed local variables in this
test to "guideline" for consistency: change mock_tip1/mock_tip2 to
mock_guideline1/mock_guideline2 (and any other mock_tip at lines ~630-634), and
rename tip_update_call and tip_entities to guideline_update_call and
guideline_entities; update their usages in the assertions that check
phoenix_sync.client.update_entities call args and the metadata checks
(references: mock_generate_guidelines return GuidelineGenerationResult,
phoenix_sync.sync, phoenix_sync.client.update_entities) so the variable names
match the updated "guidelines" terminology throughout the test.
🪄 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: c0e1dd8f-30c1-46e6-8526-4b5df2d845b6
⛔ Files ignored due to path filters (4)
docs/assets/architecture-dark.svgis excluded by!**/*.svgdocs/assets/architecture-light.svgis excluded by!**/*.svgdocs/assets/architecture-wide-dark.svgis excluded by!**/*.svgdocs/assets/architecture-wide-light.svgis excluded by!**/*.svg
📒 Files selected for processing (43)
CHANGELOG.mdREADME.mdaltk_evolve/cli/cli.pyaltk_evolve/config/llm.pyaltk_evolve/frontend/api/routes.pyaltk_evolve/frontend/client/evolve_client.pyaltk_evolve/frontend/mcp/mcp_server.pyaltk_evolve/frontend/ui/src/components/CreateEntityModal.tsxaltk_evolve/llm/guidelines/__init__.pyaltk_evolve/llm/guidelines/clustering.pyaltk_evolve/llm/guidelines/guidelines.pyaltk_evolve/llm/guidelines/prompts/combine_guidelines.jinja2altk_evolve/llm/guidelines/prompts/generate_guidelines.jinja2altk_evolve/schema/guidelines.pyaltk_evolve/schema/tips.pyaltk_evolve/sync/phoenix_sync.pydocs/archive/save-session-as-skill-design.mddocs/guides/configuration.mddocs/guides/low-code-tracing.mddocs/guides/phoenix-sync.mdexamples/low_code/litellm_demo.pyexamples/low_code/openai_agents_demo.pyexamples/low_code/simple_openai.pyexamples/low_code/smolagents_demo.pyplatform-integrations/bob/evolve-full/custom_modes.yamlplatform-integrations/claude/plugins/evolve-lite/skills/recall/SKILL.mdplatform-integrations/claude/plugins/evolve-lite/skills/save/SKILL.mdtests/e2e/test_e2e_pipeline.pytests/e2e/test_mcp.pytests/platform_integrations/conftest.pytests/platform_integrations/test_audit.pytests/platform_integrations/test_entity_io_core.pytests/platform_integrations/test_publish.pytests/platform_integrations/test_retrieve.pytests/platform_integrations/test_save_entities.pytests/platform_integrations/test_subscribe.pytests/platform_integrations/test_sync.pytests/unit/test_cli.pytests/unit/test_clustering.pytests/unit/test_combine_guidelines.pytests/unit/test_guidelines.pytests/unit/test_mcp_server.pytests/unit/test_phoenix_sync.py
💤 Files with no reviewable changes (1)
- altk_evolve/schema/tips.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
altk_evolve/sync/phoenix_sync.py (1)
38-47: Backward-compat shim looks good; note default-filter caveat.
SyncResult.tips_generatedcorrectly emits aDeprecationWarningwithstacklevel=2and returnsguidelines_generated. Worth noting:DeprecationWarningis silenced by default outside__main__in CPython, so library consumers may not see it unless they enable warnings or you temporarily switch toFutureWarningfor higher visibility during the transition release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@altk_evolve/sync/phoenix_sync.py` around lines 38 - 47, The deprecation warning in the SyncResult.tips_generated property uses DeprecationWarning which is ignored by default by most users; change the warning class to FutureWarning (or emit both) in the tips_generated property so the message is visible during the transition, e.g., replace DeprecationWarning with FutureWarning while keeping stacklevel=2 and the return of self.guidelines_generated; ensure the docstring still notes the temporary compatibility alias and that guidelines_generated is the new name.tests/unit/test_phoenix_sync.py (1)
79-88: Deprecation-warning test is solid.
warnings.simplefilter("always")insidecatch_warningsensures theDeprecationWarningis captured regardless of the ambient filter configuration, and the assertions verify both count and category. Minor optional improvement: also assert a substring of the message (e.g.,"tips_generated") to guard against accidental message regressions.Optional tightening
assert len(caught) == 1 assert caught[0].category is DeprecationWarning + assert "tips_generated" in str(caught[0].message)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_phoenix_sync.py` around lines 79 - 88, The test currently verifies a DeprecationWarning is emitted when accessing SyncResult.tips_generated but doesn't assert the warning message; update the test_sync_result_tips_generated_warns_and_returns_guidelines_count to also assert that the captured warning's message text contains a relevant substring (e.g., "tips_generated") by checking str(caught[0].message) includes "tips_generated" after the existing category assertion, referencing the SyncResult.tips_generated access point.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@altk_evolve/frontend/api/routes.py`:
- Around line 213-218: The code currently always constructs
Guideline(content=req.content, **guideline_meta) which fails for legacy/compact
guideline entities missing rationale, category, or trigger; modify the logic to
only invoke the structured Guideline validation when those detailed metadata
keys are present (e.g., check if 'rationale' in req.metadata and 'category' in
req.metadata and 'trigger' in req.metadata) and otherwise perform only the
generic entity validation (e.g., ensure req.type == "guideline" and req.content
exists) or skip structured validation so compact guidelines (type + content)
remain accepted; update the branch around Guideline(...) and
req.metadata/guideline_meta to conditionally call Guideline.
---
Nitpick comments:
In `@altk_evolve/sync/phoenix_sync.py`:
- Around line 38-47: The deprecation warning in the SyncResult.tips_generated
property uses DeprecationWarning which is ignored by default by most users;
change the warning class to FutureWarning (or emit both) in the tips_generated
property so the message is visible during the transition, e.g., replace
DeprecationWarning with FutureWarning while keeping stacklevel=2 and the return
of self.guidelines_generated; ensure the docstring still notes the temporary
compatibility alias and that guidelines_generated is the new name.
In `@tests/unit/test_phoenix_sync.py`:
- Around line 79-88: The test currently verifies a DeprecationWarning is emitted
when accessing SyncResult.tips_generated but doesn't assert the warning message;
update the test_sync_result_tips_generated_warns_and_returns_guidelines_count to
also assert that the captured warning's message text contains a relevant
substring (e.g., "tips_generated") by checking str(caught[0].message) includes
"tips_generated" after the existing category assertion, referencing the
SyncResult.tips_generated access point.
🪄 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: 64ca6a67-e10a-437e-bcb3-6c73d015bb44
📒 Files selected for processing (20)
CHANGELOG.mdREADME.mdaltk_evolve/cli/cli.pyaltk_evolve/config/llm.pyaltk_evolve/frontend/api/routes.pyaltk_evolve/sync/phoenix_sync.pydocs/guides/configuration.mddocs/guides/low-code-tracing.mddocs/guides/phoenix-sync.mdexamples/low_code/litellm_demo.pyexamples/low_code/manual_phoenix_demo.pytests/e2e/test_e2e_pipeline.pytests/platform_integrations/test_entity_io_core.pytests/platform_integrations/test_retrieve.pytests/platform_integrations/test_sync.pytests/unit/test_cli.pytests/unit/test_combine_guidelines.pytests/unit/test_llm_config.pytests/unit/test_mcp_server.pytests/unit/test_phoenix_sync.py
✅ Files skipped from review due to trivial changes (6)
- CHANGELOG.md
- examples/low_code/manual_phoenix_demo.py
- tests/unit/test_llm_config.py
- tests/unit/test_mcp_server.py
- README.md
- docs/guides/phoenix-sync.md
🚧 Files skipped from review as they are similar to previous changes (8)
- examples/low_code/litellm_demo.py
- altk_evolve/config/llm.py
- docs/guides/configuration.md
- tests/e2e/test_e2e_pipeline.py
- tests/platform_integrations/test_sync.py
- altk_evolve/cli/cli.py
- docs/guides/low-code-tracing.md
- tests/unit/test_cli.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
altk_evolve/frontend/api/routes.py (1)
212-224: LGTM — conditional structured validation preserves compact guideline entities.The gating on
{"rationale", "category", "trigger"} <= guideline_meta.keys()correctly keeps legacy/compacttype: "guideline"entities (justtype+content) accepted while still enforcing the fullGuidelineschema for generation-shaped payloads, addressing the prior review feedback.One minor consideration: partial structured metadata (e.g., caller supplies
rationaleandcategorybut omitstrigger) will silently skip validation rather than surface a 422. If you want to catch half-populated payloads, consider validating when any of the structured fields is present:Optional stricter gating
- guideline_meta = {k: v for k, v in req.metadata.items() if k != "content"} - structured_fields = {"rationale", "category", "trigger"} - if structured_fields <= guideline_meta.keys(): - Guideline(content=req.content, **guideline_meta) + guideline_meta = {k: v for k, v in req.metadata.items() if k != "content"} + structured_fields = {"rationale", "category", "trigger"} + if structured_fields & guideline_meta.keys(): + Guideline(content=req.content, **guideline_meta)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@altk_evolve/frontend/api/routes.py` around lines 212 - 224, The current gating only runs Guideline validation when all structured_fields are present, so partial structured metadata (e.g., missing "trigger") is ignored; change the condition in the entity_type == "guideline" block (where guideline_meta and structured_fields are defined) to run validation when any of the structured fields is present (e.g., check intersection or use any(...) over structured_fields against guideline_meta.keys()), then attempt Guideline(...) and keep the existing logger.error and HTTPException path for failures to surface a 422 for half-populated payloads.tests/platform_integrations/test_entity_io_core.py (1)
144-148: Usefile_assertionsfor this test-data setup too.This changed guideline file is still created manually with
mkdir()/write_text(). Keep platform integration setup consistent by using the shared fixture.♻️ Proposed refactor
- def test_loads_from_nested_type_dirs(self, temp_project_dir): - (temp_project_dir / "guideline").mkdir() - (temp_project_dir / "guideline" / "guideline.md").write_text("---\ntype: guideline\n---\n\nKeep it simple.\n") - (temp_project_dir / "preference").mkdir() - (temp_project_dir / "preference" / "pref.md").write_text("---\ntype: preference\n---\n\nUse snake_case.\n") + def test_loads_from_nested_type_dirs(self, temp_project_dir, file_assertions): + file_assertions.write_text( + temp_project_dir / "guideline" / "guideline.md", + "---\ntype: guideline\n---\n\nKeep it simple.\n", + ) + file_assertions.write_text( + temp_project_dir / "preference" / "pref.md", + "---\ntype: preference\n---\n\nUse snake_case.\n", + )As per coding guidelines, “Use fixtures from
conftest.pyto create test data (bob_fixtures, file_assertions, etc.) - do not manually create files”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/platform_integrations/test_entity_io_core.py` around lines 144 - 148, In test_loads_from_nested_type_dirs replace the manual temp_project_dir.mkdir()/write_text() calls with the shared file_assertions fixture to create the same files; specifically, use file_assertions to create "guideline/guideline.md" and "preference/pref.md" with the same frontmatter and body instead of calling (temp_project_dir / ...).mkdir() and .write_text(), so the test uses the standard fixture-based test-data setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@altk_evolve/frontend/api/routes.py`:
- Around line 212-224: The current gating only runs Guideline validation when
all structured_fields are present, so partial structured metadata (e.g., missing
"trigger") is ignored; change the condition in the entity_type == "guideline"
block (where guideline_meta and structured_fields are defined) to run validation
when any of the structured fields is present (e.g., check intersection or use
any(...) over structured_fields against guideline_meta.keys()), then attempt
Guideline(...) and keep the existing logger.error and HTTPException path for
failures to surface a 422 for half-populated payloads.
In `@tests/platform_integrations/test_entity_io_core.py`:
- Around line 144-148: In test_loads_from_nested_type_dirs replace the manual
temp_project_dir.mkdir()/write_text() calls with the shared file_assertions
fixture to create the same files; specifically, use file_assertions to create
"guideline/guideline.md" and "preference/pref.md" with the same frontmatter and
body instead of calling (temp_project_dir / ...).mkdir() and .write_text(), so
the test uses the standard fixture-based test-data setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f16ecea9-e551-4fd6-bf45-a040c006963c
📒 Files selected for processing (8)
altk_evolve/frontend/api/routes.pytests/platform_integrations/test_audit.pytests/platform_integrations/test_entity_io_core.pytests/platform_integrations/test_publish.pytests/platform_integrations/test_retrieve.pytests/platform_integrations/test_save_entities.pytests/platform_integrations/test_subscribe.pytests/platform_integrations/test_sync.py
✅ Files skipped from review due to trivial changes (3)
- tests/platform_integrations/test_subscribe.py
- tests/platform_integrations/test_audit.py
- tests/platform_integrations/test_save_entities.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/platform_integrations/test_sync.py
- tests/platform_integrations/test_publish.py
Collaborator
Author
|
should we merge this refactor first? @visahak |
Collaborator
|
Looking at it now. |
illeatmyhat
previously approved these changes
Apr 24, 2026
Collaborator
SummaryThis PR renames the “tips” surface to “guidelines” across the runtime, CLI, MCP server, docs, examples, and tests. It also includes a follow-up API fix so compact guideline entities can still be Findings
Testing
Test Errors
|
Collaborator
|
There is a PR for the two of the tests here #220 (7 and 1) |
Collaborator
Author
|
Addressed this review point on this branch. I updated the Codex sharing integration tests to use the renamed seeded fixtures (guideline-one.md / guideline-two.md) instead of stale tip-one.md / tip-two.md paths in tests/platform_integrations/test_codex_sharing.py (the assertions and repo mutation steps now match tests/platform_integrations/conftest.py again). Verification run:
The two additional failures mentioned in the thread (test_sync.py symlink behavior and Codex uninstall hook pruning) are tracked separately in #220. |
gaodan-fang
force-pushed
the
codex/locate-keyword-tips-references
branch
from
2e0ca27 to
15a5b3e
Compare
visahak
approved these changes
Apr 25, 2026
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
3 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
tipsmodule paths in favor of the newguidelinesstructure.Testing
Summary by CodeRabbit
Breaking Changes
Updates
Tests & Compatibility