Enforce PY-1 import contracts with import-linter - #154
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR introduces architectural boundaries through import-linter configuration, refactors action generation from agent instance methods to module-level functions, creates API-specific error classes for cleaner route error handling, and simplifies validation logic in run configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
simulation/api/services/run_query_service.py (1)
122-128:⚠️ Potential issue | 🟡 MinorStale docstring: references removed
RunNotFoundError.Line 124 still documents
RunNotFoundErrorbut the function now raisesApiRunNotFoundError. Update the docstring to match.📝 Proposed fix
- RunNotFoundError: If the run does not exist. + ApiRunNotFoundError: If the run does not exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/api/services/run_query_service.py` around lines 122 - 128, Update the Raises section of the docstring to reflect the actual exception type thrown: replace the stale RunNotFoundError entry with ApiRunNotFoundError (or remove the old reference), ensuring the docstring for the function that calls engine.get_run(run_id) and raises ApiRunNotFoundError(run_id) correctly documents ValueError for empty run_id and ApiRunNotFoundError for missing runs.simulation/api/routes/simulation.py (1)
276-290:⚠️ Potential issue | 🟡 MinorUnreachable
exceptblock —DEFAULT_SIMULATION_CONFIGis a constant.
return DEFAULT_SIMULATION_CONFIGon Line 282 cannot raise, so theexcept Exceptionblock (Lines 283–290) is dead code. Either remove the try/except or, if you want a defensive wrapper for future changes, leave a comment explaining why.♻️ Suggested simplification
`@timed`(attach_attr="duration_ms", log_level=None) async def _execute_get_default_config( request: Request, ) -> DefaultConfigSchema | Response: """Fetch default config and convert unexpected failures to HTTP responses.""" - try: - return DEFAULT_SIMULATION_CONFIG - except Exception: - logger.exception("Unexpected error while fetching default config") - return _error_response( - status_code=500, - code="INTERNAL_ERROR", - message="Internal server error", - detail=None, - ) + return DEFAULT_SIMULATION_CONFIG🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/api/routes/simulation.py` around lines 276 - 290, The try/except in _execute_get_default_config is dead because returning DEFAULT_SIMULATION_CONFIG cannot raise; remove the try/except and simply return DEFAULT_SIMULATION_CONFIG, or if you want to keep a defensive wrapper for future changes, replace the broad try/except with a short explanatory comment above the return explaining the defensive intent and why exceptions are expected in future, or narrow the catch to specific operations and ensure any error handling uses logger.exception and _error_response only when real fallible work is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyproject.toml`:
- Around line 112-115: The import-linter config lists ai_tools in root_packages
but it isn't constrained by any forbidden_modules contract; either add ai_tools
to the appropriate forbidden_modules lists (e.g., include "ai_tools" in the same
contract entries that restrict lib, ml_tooling, or simulation.core.models) so it
inherits the intended isolation, or add a short clarifying comment next to
root_packages explaining that ai_tools is intentionally unconstrained because
the directory is currently empty and reserved for future work; update the
pyproject.toml import-linter section accordingly and reference the ai_tools
identifier and the forbidden_modules contract names when making the change.
In `@simulation/core/agent_actions.py`:
- Around line 20-71: These three functions (generate_likes, generate_comments,
generate_follows) duplicate the same guard + generator retrieval +
generator.generate call; extract a small helper (e.g., _generate_action) that
accepts candidates, run_id, turn_number, agent_handle and a generator
factory/function (get_like_generator, get_comment_generator,
get_follow_generator) and performs the guard and delegation, then have each
specific function call _generate_action with the appropriate factory to remove
the repeated boilerplate.
- Line 7: Remove the unnecessary future import by deleting the line "from
__future__ import annotations" in this module; since the project targets Python
3.12 (py312) the postponed evaluation of annotations is redundant—no other code
changes are needed in functions/classes in this file (just remove that import).
In `@simulation/core/models/runs.py`:
- Around line 45-48: Duplicate one-line validators validate_feed_algorithm found
in RunConfig and Run should be replaced by a single module-level validator
function: create a top-level function (e.g., validate_feed_algorithm_field) that
calls validate_non_empty_string(..., "feed_algorithm"), then reference that
function from both `@field_validator` decorators on the feed_algorithm field in
RunConfig and Run so both models reuse the same validator implementation; update
imports/annotations if needed and remove the duplicate class methods.
- Around line 32-33: The call to validate_non_empty_iterable(...) currently
discards its return value before using v in the list comprehension; update the
code in the Metric keys validators (the function where
validate_non_empty_iterable(v, "metric_keys") and the
Run.validate_metric_keys_run method) to capture the returned iterable (e.g., v =
validate_non_empty_iterable(v, "metric_keys")) and then iterate that returned
value when building the list of validate_non_empty_string(item, "metric_keys");
alternatively, if the helper is intended only to assert non-emptiness, rename it
to assert_non_empty_iterable or add a clarifying comment and keep current
use—ensure validate_non_empty_iterable's contract is respected by either using
its return value or changing its name/semantics.
In `@tests/simulation/core/test_command_service.py`:
- Around line 289-310: Introduce a reusable contextmanager or pytest fixture
(e.g., patch_action_generators) that patches the three targets
"simulation.core.command_service.generate_likes",
"simulation.core.command_service.generate_comments", and
"simulation.core.command_service.generate_follows" with provided mocks, then
replace the repeated with-patch blocks in the tests that call
command_service._simulate_turn (and the other two test sites) by using this
helper to reduce boilerplate and ensure consistent patch targets; name the
helper clearly (patch_action_generators) and have it accept the three mock
objects and yield control so existing test bodies remain unchanged.
In `@tests/simulation/core/test_social_media_agent.py`:
- Around line 1-5: The test file name doesn't reflect the module under test;
rename the file from test_social_media_agent.py to test_agent_actions.py so
pytest discovers it as testing simulation.core.agent_actions, and update the
file-level docstring to "Tests for simulation.core.agent_actions"; ensure any
imports (e.g., generate_follows) or external references to the old filename
(CI/test runners) are updated accordingly.
---
Outside diff comments:
In `@simulation/api/routes/simulation.py`:
- Around line 276-290: The try/except in _execute_get_default_config is dead
because returning DEFAULT_SIMULATION_CONFIG cannot raise; remove the try/except
and simply return DEFAULT_SIMULATION_CONFIG, or if you want to keep a defensive
wrapper for future changes, replace the broad try/except with a short
explanatory comment above the return explaining the defensive intent and why
exceptions are expected in future, or narrow the catch to specific operations
and ensure any error handling uses logger.exception and _error_response only
when real fallible work is present.
In `@simulation/api/services/run_query_service.py`:
- Around line 122-128: Update the Raises section of the docstring to reflect the
actual exception type thrown: replace the stale RunNotFoundError entry with
ApiRunNotFoundError (or remove the old reference), ensuring the docstring for
the function that calls engine.get_run(run_id) and raises
ApiRunNotFoundError(run_id) correctly documents ValueError for empty run_id and
ApiRunNotFoundError for missing runs.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
docs/SUGGESTED_CUSTOM_LINTERS.mdis excluded by!**/*.md
📒 Files selected for processing (16)
.github/workflows/ci.yml.pre-commit-config.yamlpyproject.tomlsimulation/api/errors.pysimulation/api/routes/simulation.pysimulation/api/services/agent_command_service.pysimulation/api/services/metadata_service.pysimulation/api/services/run_execution_service.pysimulation/api/services/run_query_service.pysimulation/core/agent_actions.pysimulation/core/command_service.pysimulation/core/models/agents.pysimulation/core/models/runs.pytests/api/test_run_query_service.pytests/simulation/core/test_command_service.pytests/simulation/core/test_social_media_agent.py
💤 Files with no reviewable changes (1)
- simulation/core/models/agents.py
Comment on lines
+289
to
+310
| with ( | ||
| patch( | ||
| "simulation.core.command_service.generate_likes", mock_generate_likes | ||
| ), | ||
| patch( | ||
| "simulation.core.command_service.generate_comments", | ||
| mock_generate_comments, | ||
| ), | ||
| patch( | ||
| "simulation.core.command_service.generate_follows", | ||
| mock_generate_follows, | ||
| ), | ||
| ): | ||
| action_history_store = Mock() | ||
| result = command_service._simulate_turn( | ||
| run_id=sample_run.run_id, | ||
| turn_number=0, | ||
| agents=[agent], | ||
| feed_algorithm="chronological", | ||
| action_history_store=action_history_store, | ||
| turn_metric_keys=DEFAULT_TURN_METRIC_KEYS, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Repeated triple-patch blocks — consider a shared helper or fixture.
The same three patch("simulation.core.command_service.generate_*", ...) context managers appear in three tests. A small helper (e.g., a @contextmanager or a pytest fixture yielding the three mocks) would reduce boilerplate and make it easier to keep the patch targets consistent if the module path changes.
♻️ Example helper
from contextlib import contextmanager
`@contextmanager`
def patch_action_generators(likes_mock, comments_mock, follows_mock):
with (
patch("simulation.core.command_service.generate_likes", likes_mock),
patch("simulation.core.command_service.generate_comments", comments_mock),
patch("simulation.core.command_service.generate_follows", follows_mock),
):
yieldThen in tests:
with patch_action_generators(mock_generate_likes, mock_generate_comments, mock_generate_follows):
result = command_service._simulate_turn(...)Also applies to: 401-422, 634-654
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/simulation/core/test_command_service.py` around lines 289 - 310,
Introduce a reusable contextmanager or pytest fixture (e.g.,
patch_action_generators) that patches the three targets
"simulation.core.command_service.generate_likes",
"simulation.core.command_service.generate_comments", and
"simulation.core.command_service.generate_follows" with provided mocks, then
replace the repeated with-patch blocks in the tests that call
command_service._simulate_turn (and the other two test sites) by using this
helper to reduce boilerplate and ensure consistent patch targets; name the
helper clearly (patch_action_generators) and have it accept the three mock
objects and yield control so existing test bodies remain unchanged.
This was referenced Mar 19, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
import-linterand PY-1 import contracts (configured inpyproject.toml).import-linter (PY-1)) and CI (newimport_lintjob).Notable refactors
simulation.core.modelspure by moving action generation intosimulation/core/agent_actions.py.feeds.*/simulation.core.*by moving metadata listing intosimulation/api/services/metadata_service.pyand translating service errors into API-layer exceptions.simulation/core/models/runs.py(validation remains at API boundary).How to verify
uv sync --extra testuv run lint-imports --config pyproject.tomluv run pre-commit run --all-filesuv run pytestSummary by CodeRabbit
New Features
Bug Fixes
Chores