Skip to content

Conversation

@TommrraraSnow
Copy link
Member

Summary

  • add a dedicated buff_engine package with declarative definitions and runtime services
  • persist buff metadata and triggers via a SQLite-backed registry
  • cover the new engine, registry, and store with focused pytest cases

Testing

  • uv run pytest tests/test_buff_engine.py

https://chatgpt.com/codex/tasks/task_e_68e3f8848598832d9ebfd27e571827b5

@github-actions
Copy link

github-actions bot commented Oct 6, 2025

🤖 Hi @TommrraraSnow, I've received your request, and I'm working on it now! You can track my progress in the logs for more details.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📋 Review Summary

This pull request introduces a well-designed and modular buff engine. The separation of definitions, registry, runtime services, and storage is clean and promotes extensibility. The code is generally of high quality and includes a good set of initial tests.

🔍 General Feedback

  • The core logic is sound, but the BuffStore is missing the implementation of stacking rules, which is a key part of buff management.
  • The use of SQLite for the registry is appropriate, and the schema is well-defined.
  • The testing strategy is good, covering unit and integration aspects of the new engine. Expanding test coverage for edge cases would be beneficial.

self._store.setdefault(instance.owner_id, []).append(instance)

def remove(self, owner_id: str, predicate: Callable[[BuffInstance], bool]) -> int:
instances = self._store.get(owner_id, [])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 The add method in BuffStore currently appends new buff instances without considering stacking rules defined in BuffDefinition (e.g., max_stacks, stacking_rule). This could lead to incorrect behavior where buffs stack indefinitely or don't refresh as expected. The store should probably handle logic for refreshing durations, incrementing stacks, or replacing instances based on the buff's stacking rule.

if isinstance(value, str):
value = json.loads(value)
return tuple(value or ())

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 The pattern if isinstance(value, str): value = json.loads(value) is repeated in _ensure_sequence, _ensure_tuple, _ensure_mapping, and _selector_from_payload. To improve code reuse, this logic could be extracted into a utility function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants