Create the first liking algorithm - #56
Merged
Merged
Conversation
…rules for different actions
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a pluggable like action-generator subsystem (interface, deterministic implementation, registry, validators), centralizes validation helpers used across models, threads run_id/turn_number into like generation calls, updates models to use shared validators, and adds/updates tests and docs to reflect these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant CS as Command Service
participant Agent as SocialMediaAgent
participant Reg as Generator Registry
participant Det as DeterministicLikeGenerator
CS->>Agent: like_posts(feed, run_id, turn_number)
Agent->>Reg: get_like_generator(mode="deterministic")
Reg->>Reg: validate_behavior_mode(mode)
Reg->>Reg: return cached or lazy-load impl
Reg-->>Agent: LikeGenerator instance
Agent->>Det: generate(candidates=feed, run_id, turn_number, agent_handle)
Det->>Det: score posts (recency + social proof)
Det->>Det: select top-K and build GeneratedLike objects
Det-->>Agent: list[GeneratedLike]
Agent-->>CS: return likes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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: 2
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@simulation/core/action_generators/like/algorithms/deterministic.py`:
- Around line 66-72: The _recency_score function currently parses created_at
with datetime.strptime and calls .timestamp() on a naive datetime which is
interpreted in the local TZ; make it timezone-stable by treating the parsed
datetime as UTC before timestamping: after parsing in _recency_score convert dt
to UTC via dt.replace(tzinfo=timezone.utc) (and ensure timezone is imported from
datetime), then call .timestamp() and return that float; keep the fallback
return 0.0 for parse errors and continue using CREATED_AT_FORMAT for parsing.
In `@tests/simulation/core/test_action_generators_registry.py`:
- Around line 33-36: Update the test so it actually verifies the default is
deterministic: call get_like_generator() and compare it to an explicit
deterministic instantiation (e.g., get_like_generator(mode="deterministic")) or,
if the returned LikeGenerator exposes a flag/property, assert that
generator.deterministic (or similar) is True; reference get_like_generator and
LikeGenerator to locate the test and replace the simple isinstance assertion
with an equality or property assertion against the explicit deterministic call.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@simulation/core/action_generators/like/algorithms/deterministic.py`:
- Around line 1-5: The module docstring in deterministic.py contains a typo: the
sentence "scoring.s" has an extraneous "s" at the end; edit the top-level
docstring to change "scoring.s" to "scoring." so the description reads correctly
and retains the existing wording and punctuation.
- Around line 55-63: The _score_post function currently has an unused
agent_handle parameter; remove agent_handle from the _score_post signature and
any type hints, and update its call site(s) (notably generate()) to stop passing
agent_handle, ensuring the scoring logic remains unchanged; if instead you
intend to keep agent_handle for future personalization, add a brief docstring
note in _score_post stating it's reserved for future use and explain why it is
unused to avoid confusion.
- Around line 75-82: _docstring the deterministic created_at format in
_derive_created_at to make the synthetic ID explicit for traceability: update
the _derive_created_at function's docstring to state that it returns a
deterministic synthetic identifier (e.g.,
"det_{run_id}_turn{turn_number}_{agent_handle}_{post_index}") used for
GeneratedLike.created_at and that it intentionally does not follow
CREATED_AT_FORMAT timestamp pattern; mention that this is by design for
reproducibility and note the alternative (producing CREATED_AT_FORMAT
timestamps) if future code needs parseable timestamps so maintainers know the
option.
* Refactor validators to use centralized tooling * add validation for non-negative params * update rules * update validators * update rules
Contributor
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 (1)
simulation/core/models/posts.py (1)
69-75:⚠️ Potential issue | 🟡 MinorAvoid mutating validator input; make a shallow copy instead.
model_validator(mode="before")with in-place mutation violates Pydantic v2 best practices. The mutated value can be reused by other validators in union validation paths, causing side effects. Additionally, ifidis present but empty/whitespace, the current logic won't fall back touri.🔧 Proposed safer implementation
`@model_validator`(mode="before") `@classmethod` def set_id_from_uri(cls, data: dict) -> dict: """Set id from uri if not provided.""" - if isinstance(data, dict) and "uri" in data and "id" not in data: - data["id"] = data["uri"] - return data + if not isinstance(data, dict): + return data + data = dict(data) + existing_id = data.get("id") + if "uri" in data and ( + existing_id is None + or (isinstance(existing_id, str) and not existing_id.strip()) + ): + data["id"] = data["uri"] + return data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/core/models/posts.py` around lines 69 - 75, The model_validator set_id_from_uri currently mutates the incoming data dict in-place; change it to operate on a shallow copy to avoid side effects by first copying the input (e.g., new = dict(data) or data.copy()) and then set id from uri only when id is missing or blank/whitespace (check for not data.get("id") or str(data.get("id")).strip() == ""), and return the copied dict; keep the decorator model_validator(mode="before") and the method signature set_id_from_uri for easy location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/validation_utils.py`:
- Around line 135-143: Replace the manual None check in validate_turn_number
with the shared "cannot be None" validator used elsewhere (e.g., call
validate_not_none(turn_number, "turn_number")) and then call
validate_nonnegative_value(turn_number, "turn_number"); this ensures the error
messages match the standardized "cannot be None" / "must be >= 0" wording and
removes the custom "turn_number is invalid" text.
- Around line 83-103: The validate_non_empty_iterable function currently treats
iterators incorrectly; update it to fast-path when the argument is a Collection
by checking len(iterable) and raising if zero, and for non-Collection iterables
(iterators) perform a safe "peek" by calling next() inside a try/except to
detect emptiness then rechain the consumed item back into the iterator (e.g., by
yielding the item first via itertools.chain or creating a new iterator that
yields the peeked item then the rest) so the function correctly raises
ValueError when the iterator is empty and otherwise returns an equivalent
iterable; use the function name validate_non_empty_iterable, the typing Iterable
and Collection checks, and ensure None is still handled as before.
In `@tests/db/repositories/test_generated_bio_repository_integration.py`:
- Around line 211-213: The test redundantly checks length after asserting
equality; remove the `assert len(retrieved.generated_bio) >= 2299` or replace it
with an exact length assertion against the known fixture (use
`len(expected_bio)`), i.e., keep the equality `assert retrieved.generated_bio ==
expected_bio` and either delete the length assertion or change it to `assert
len(retrieved.generated_bio) == len(expected_bio)` to avoid the magic number;
look for `expected_bio` and `retrieved.generated_bio` in the test to implement
this change.
---
Outside diff comments:
In `@simulation/core/models/posts.py`:
- Around line 69-75: The model_validator set_id_from_uri currently mutates the
incoming data dict in-place; change it to operate on a shallow copy to avoid
side effects by first copying the input (e.g., new = dict(data) or data.copy())
and then set id from uri only when id is missing or blank/whitespace (check for
not data.get("id") or str(data.get("id")).strip() == ""), and return the copied
dict; keep the decorator model_validator(mode="before") and the method signature
set_id_from_uri for easy location.
---
Duplicate comments:
In `@tests/simulation/core/test_action_generators_registry.py`:
- Around line 33-36: The test test_default_mode_is_deterministic currently only
checks the returned type; update it to assert the default behavior is
deterministic by either comparing the default call to an explicit deterministic
call (e.g. assert get_like_generator() ==
get_like_generator(mode="deterministic")) or by checking a deterministic
attribute on the returned object (e.g. assert generator.mode ==
"deterministic"); modify the assertion in that test accordingly and reference
get_like_generator and LikeGenerator to locate the change.
Comment on lines
+83
to
+103
| def validate_non_empty_iterable(iterable: Iterable[T], field_name: str) -> Iterable[T]: | ||
| """Validate that an iterable is not None and not empty. | ||
|
|
||
| Prefer passing a Collection (list, set, etc.) so the check does not consume | ||
| iterators. For iterators, the check uses truthiness (empty iterator is falsy). | ||
|
|
||
| Args: | ||
| iterable: The iterable to validate. | ||
| field_name: Name of the field (for error messages). | ||
|
|
||
| Returns: | ||
| The value unchanged. | ||
|
|
||
| Raises: | ||
| ValueError: When iterable is None or empty. | ||
| """ | ||
| if iterable is None: | ||
| raise ValueError(f"{field_name} cannot be None") | ||
| if not iterable: | ||
| raise ValueError(f"{field_name} cannot be empty") | ||
| return iterable |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the validation_utils.py file
find . -name "validation_utils.py" -type fRepository: METResearchGroup/social_agent_simulation_platform
Length of output: 118
🏁 Script executed:
# Read the validation_utils.py file to verify the code and line numbers
cat -n ./lib/validation_utils.py | head -120Repository: METResearchGroup/social_agent_simulation_platform
Length of output: 4276
🏁 Script executed:
# Verify the bool(iterator) behavior with empty and non-empty iterators
python3 << 'EOF'
# Test 1: Empty iterator
empty_iter = iter([])
print(f"bool(empty iterator): {bool(empty_iter)}")
# Test 2: Non-empty iterator
non_empty_iter = iter([1, 2, 3])
print(f"bool(non-empty iterator): {bool(non_empty_iter)}")
# Test 3: Empty list (for comparison)
empty_list = []
print(f"bool(empty list): {bool(empty_list)}")
# Test 4: Non-empty list (for comparison)
non_empty_list = [1, 2, 3]
print(f"bool(non-empty list): {bool(non_empty_list)}")
EOFRepository: METResearchGroup/social_agent_simulation_platform
Length of output: 201
🏁 Script executed:
# Search for tests related to validate_non_empty_iterable
find . -name "*.py" -type f -path "*/test*" | head -20Repository: METResearchGroup/social_agent_simulation_platform
Length of output: 943
🏁 Script executed:
# Search for tests of validate_non_empty_iterable
rg "validate_non_empty_iterable" --type pyRepository: METResearchGroup/social_agent_simulation_platform
Length of output: 350
🏁 Script executed:
# Check the usage in simulation/core/validators.py
cat -n ./simulation/core/validators.py | grep -A 5 -B 5 "validate_non_empty_iterable"Repository: METResearchGroup/social_agent_simulation_platform
Length of output: 886
🏁 Script executed:
# Check if there are tests that specifically test validate_non_empty_iterable
rg "validate_non_empty_iterable\|validate_uris_exist" tests/ --type py -A 3Repository: METResearchGroup/social_agent_simulation_platform
Length of output: 75
🏁 Script executed:
# Search for any tests that might use validate_uris_exist or test the validators module
rg "validate_uris_exist\|test.*validators" tests/ --type pyRepository: METResearchGroup/social_agent_simulation_platform
Length of output: 75
🏁 Script executed:
# Search for usage of validate_uris_exist throughout the codebase
rg "validate_uris_exist" --type py -B 2 -A 2Repository: METResearchGroup/social_agent_simulation_platform
Length of output: 1018
🏁 Script executed:
# Check if collections.abc.Collection and itertools are available/imported elsewhere in the codebase
rg "from collections.abc import|import itertools" --type py | head -20Repository: METResearchGroup/social_agent_simulation_platform
Length of output: 740
🏁 Script executed:
# Check the complete usage context in feed_post_repository.py
rg -B 10 -A 10 "validate_uris_exist" db/repositories/feed_post_repository.pyRepository: METResearchGroup/social_agent_simulation_platform
Length of output: 1330
🏁 Script executed:
# Check if there are any test files that specifically test validation_utils
find tests -name "*validation*" -o -name "*validator*"Repository: METResearchGroup/social_agent_simulation_platform
Length of output: 210
🏁 Script executed:
# Check the test files for any tests related to validate_non_empty_iterable or validate_uris_exist
rg "validate_non_empty_iterable\|validate_uris_exist\|validation_utils" tests/simulation/core/test_action_generators_validators.py tests/simulation/core/test_agent_action_rules_validator.pyRepository: METResearchGroup/social_agent_simulation_platform
Length of output: 75
🏁 Script executed:
# Let's verify the actual problem by testing what happens when validate_non_empty_iterable receives an empty iterator
python3 << 'EOF'
from typing import Iterable, TypeVar
T = TypeVar("T")
def validate_non_empty_iterable(iterable: Iterable[T], field_name: str) -> Iterable[T]:
"""Current implementation from the code."""
if iterable is None:
raise ValueError(f"{field_name} cannot be None")
if not iterable:
raise ValueError(f"{field_name} cannot be empty")
return iterable
# Test 1: Empty iterator (should fail but doesn't)
try:
empty_iter = iter([])
result = validate_non_empty_iterable(empty_iter, "test_field")
print("✗ Empty iterator PASSED validation (BUG!)")
print(f" Returned: {result}")
except ValueError as e:
print(f"✓ Empty iterator caught: {e}")
# Test 2: Non-empty iterator (should pass)
try:
non_empty_iter = iter([1, 2, 3])
result = validate_non_empty_iterable(non_empty_iter, "test_field")
print("✓ Non-empty iterator PASSED validation")
except ValueError as e:
print(f"✗ Non-empty iterator failed: {e}")
# Test 3: Empty list (should fail and does)
try:
empty_list = []
result = validate_non_empty_iterable(empty_list, "test_field")
print("✗ Empty list PASSED validation")
except ValueError as e:
print(f"✓ Empty list caught: {e}")
EOFRepository: METResearchGroup/social_agent_simulation_platform
Length of output: 274
🏁 Script executed:
# Verify the proposed imports are standard library and available
python3 << 'EOF'
from collections.abc import Collection, Iterable
import itertools
print("✓ All imports from proposed fix are available")
print(f" Collection: {Collection}")
print(f" Iterable: {Iterable}")
print(f" itertools: {itertools}")
EOFRepository: METResearchGroup/social_agent_simulation_platform
Length of output: 281
Fix empty-iterator validation (current check never fails for iterators).
Empty iterators are always truthy in Python, so bool(iterator) returns True regardless of content. This means empty iterators currently pass validation, contradicting the docstring claim that "the check uses truthiness (empty iterator is falsy)."
Update the function to fast-path Collections with len() and peek-and-rechain iterators:
🛠️ Proposed fix (Collection fast-path + iterator peek)
-from typing import Any, Iterable, TypeVar, overload
+from collections.abc import Collection, Iterable
+from typing import Any, TypeVar, overload
+import itertools-def validate_non_empty_iterable(iterable: Iterable[T], field_name: str) -> Iterable[T]:
+def validate_non_empty_iterable(iterable: Iterable[T], field_name: str) -> Iterable[T]:
"""Validate that an iterable is not None and not empty.
- Prefer passing a Collection (list, set, etc.) so the check does not consume
- iterators. For iterators, the check uses truthiness (empty iterator is falsy).
+ Prefer passing a Collection (list, set, etc.) so the check does not consume
+ iterators. For iterators, we peek one item and re-chain it.
@@
if iterable is None:
raise ValueError(f"{field_name} cannot be None")
- if not iterable:
- raise ValueError(f"{field_name} cannot be empty")
- return iterable
+ if isinstance(iterable, Collection):
+ if len(iterable) == 0:
+ raise ValueError(f"{field_name} cannot be empty")
+ return iterable
+ iterator = iter(iterable)
+ try:
+ first = next(iterator)
+ except StopIteration:
+ raise ValueError(f"{field_name} cannot be empty")
+ return itertools.chain([first], iterator)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def validate_non_empty_iterable(iterable: Iterable[T], field_name: str) -> Iterable[T]: | |
| """Validate that an iterable is not None and not empty. | |
| Prefer passing a Collection (list, set, etc.) so the check does not consume | |
| iterators. For iterators, the check uses truthiness (empty iterator is falsy). | |
| Args: | |
| iterable: The iterable to validate. | |
| field_name: Name of the field (for error messages). | |
| Returns: | |
| The value unchanged. | |
| Raises: | |
| ValueError: When iterable is None or empty. | |
| """ | |
| if iterable is None: | |
| raise ValueError(f"{field_name} cannot be None") | |
| if not iterable: | |
| raise ValueError(f"{field_name} cannot be empty") | |
| return iterable | |
| def validate_non_empty_iterable(iterable: Iterable[T], field_name: str) -> Iterable[T]: | |
| """Validate that an iterable is not None and not empty. | |
| Prefer passing a Collection (list, set, etc.) so the check does not consume | |
| iterators. For iterators, we peek one item and re-chain it. | |
| Args: | |
| iterable: The iterable to validate. | |
| field_name: Name of the field (for error messages). | |
| Returns: | |
| The value unchanged. | |
| Raises: | |
| ValueError: When iterable is None or empty. | |
| """ | |
| if iterable is None: | |
| raise ValueError(f"{field_name} cannot be None") | |
| if isinstance(iterable, Collection): | |
| if len(iterable) == 0: | |
| raise ValueError(f"{field_name} cannot be empty") | |
| return iterable | |
| iterator = iter(iterable) | |
| try: | |
| first = next(iterator) | |
| except StopIteration: | |
| raise ValueError(f"{field_name} cannot be empty") | |
| return itertools.chain([first], iterator) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/validation_utils.py` around lines 83 - 103, The
validate_non_empty_iterable function currently treats iterators incorrectly;
update it to fast-path when the argument is a Collection by checking
len(iterable) and raising if zero, and for non-Collection iterables (iterators)
perform a safe "peek" by calling next() inside a try/except to detect emptiness
then rechain the consumed item back into the iterator (e.g., by yielding the
item first via itertools.chain or creating a new iterator that yields the peeked
item then the rest) so the function correctly raises ValueError when the
iterator is empty and otherwise returns an equivalent iterable; use the function
name validate_non_empty_iterable, the typing Iterable and Collection checks, and
ensure None is still handled as before.
Comment on lines
+135
to
+143
| def validate_turn_number(turn_number: int | None) -> None: | ||
| """Validate that turn_number is a non-negative integer. | ||
|
|
||
| Raises: | ||
| ValueError: If turn_number is None or negative. | ||
| """ | ||
| if turn_number is None: | ||
| raise ValueError("turn_number is invalid") | ||
| validate_nonnegative_value(turn_number, "turn_number") |
Contributor
There was a problem hiding this comment.
Use shared validators for consistent turn_number messaging.
The None branch currently raises "turn_number is invalid", which diverges from the standardized "cannot be None" / "must be >= 0" messaging used elsewhere.
✅ Suggested adjustment for consistent errors
def validate_turn_number(turn_number: int | None) -> None:
@@
- if turn_number is None:
- raise ValueError("turn_number is invalid")
- validate_nonnegative_value(turn_number, "turn_number")
+ validate_nonnegative_value(
+ validate_not_none(turn_number, "turn_number"),
+ "turn_number",
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/validation_utils.py` around lines 135 - 143, Replace the manual None
check in validate_turn_number with the shared "cannot be None" validator used
elsewhere (e.g., call validate_not_none(turn_number, "turn_number")) and then
call validate_nonnegative_value(turn_number, "turn_number"); this ensures the
error messages match the standardized "cannot be None" / "must be >= 0" wording
and removes the custom "turn_number is invalid" text.
Comment on lines
+211
to
+213
| expected_bio = long_bio_text.strip() | ||
| assert retrieved.generated_bio == expected_bio | ||
| assert len(retrieved.generated_bio) >= 2299 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove or tighten the redundant length assertion.
Since you already assert equality with expected_bio, the len >= 2299 check adds no extra confidence and introduces a magic number. Consider asserting exact length against expected_bio or dropping the length check.
♻️ Suggested tweak
expected_bio = long_bio_text.strip()
assert retrieved.generated_bio == expected_bio
- assert len(retrieved.generated_bio) >= 2299
+ assert len(retrieved.generated_bio) == len(expected_bio)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expected_bio = long_bio_text.strip() | |
| assert retrieved.generated_bio == expected_bio | |
| assert len(retrieved.generated_bio) >= 2299 | |
| expected_bio = long_bio_text.strip() | |
| assert retrieved.generated_bio == expected_bio | |
| assert len(retrieved.generated_bio) == len(expected_bio) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/db/repositories/test_generated_bio_repository_integration.py` around
lines 211 - 213, The test redundantly checks length after asserting equality;
remove the `assert len(retrieved.generated_bio) >= 2299` or replace it with an
exact length assertion against the known fixture (use `len(expected_bio)`),
i.e., keep the equality `assert retrieved.generated_bio == expected_bio` and
either delete the length assertion or change it to `assert
len(retrieved.generated_bio) == len(expected_bio)` to avoid the magic number;
look for `expected_bio` and `retrieved.generated_bio` in the test to implement
this change.
This was referenced Feb 26, 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
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.
PR Description
This PR introduces a hardcoded liking algorithm and sets up the wiring to (1) add liking to the app (removing the stubbed functionality in
simulation/core/models/agents.pyand (2) introduce alternative algorithms in the future (we use a hardcoded one, insimulation/core/action_generators/like/algorithms/deterministic.py, but add a registry,simulation/core/action_generators/registry.py, for future implementations).Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests
Documentation