Add Talos runtime evaluation contract - #13
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds typed Talos models, makes compute_talos_level policy-aware, introduces talos.runtime for centralized atomic persistence and evaluation (explain_level/evaluate_snapshot/IO helpers), updates package exports to re-export models and runtime helpers, and adjusts tests to exercise the new modules and behaviors. ChangesTalos Architecture Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_scorer.py (1)
13-15: 💤 Low valueConsider adding a test for
write_block_cyclespolicy override.This test validates
friction_ratiofrom the policy but doesn't exercisewrite_block_cycles. Adding a test that passes a policy with a differentwrite_block_cyclesvalue (e.g.,write_block_cycles=3) would confirm both policy fields affect the calculation as intended.📝 Example test for `write_block_cycles`
def test_policy_can_raise_write_block_threshold() -> None: # With write_block_cycles=3, cycles=2 should yield level 2 instead of level 3 assert compute_talos_level(10, 10, [], 0, 2, policy=TalosPolicy(write_block_cycles=3)) == 2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_scorer.py` around lines 13 - 15, Add a unit test to exercise the TalosPolicy.write_block_cycles override: create a test (e.g., test_policy_can_raise_write_block_threshold) that calls compute_talos_level with cycles set lower than the default threshold but below the policy's write_block_cycles (for example, cycles=2 and policy=TalosPolicy(write_block_cycles=3)) and assert the returned level reflects the policy override (expected level 2 per the example). Locate usages around test_policy_can_raise_friction_threshold and add this new test so both friction_ratio and write_block_cycles fields are validated.talos/models.py (1)
45-48: 💤 Low valueType coercion with
list()could mask invalid input types.Lines 45 and 48 wrap
list()around the result of.get(). While this defensively creates a new list (avoiding mutation), it will also silently convert non-list iterables:
list("abc")→["a", "b", "c"]list((1, 2))→[1, 2]If stricter type checking is desired, consider explicit validation:
at_cap_val = payload.get("at_cap_projects", []) if not isinstance(at_cap_val, list): raise TypeError(f"at_cap_projects must be a list, got {type(at_cap_val)}") at_cap_projects = at_cap_val.copy()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@talos/models.py` around lines 45 - 48, The current assignments use list(payload.get(...)) which will silently convert non-list iterables; instead, fetch the raw values from payload (e.g., at_cap_val = payload.get("at_cap_projects", []) and proj_val = payload.get("projects", [])), validate with isinstance(..., list) and raise a TypeError if not a list, then assign copies (e.g., at_cap_projects = at_cap_val.copy(), projects = proj_val.copy()) so callers cannot mutate internal state; update the code paths that set at_cap_projects and projects in talos/models.py to follow this pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_models.py`:
- Around line 9-22: The test_snapshot_round_trip currently only asserts source
== "rowan", leaving serialization unchecked; update test_snapshot_round_trip to
assert the full round-trip by comparing the original dict input to
TalosSnapshot.from_dict(...) with snapshot.to_dict(), ensuring fields like
wip_total, global_max, at_cap_projects, backlog_total, backlog_delta, projects,
source, and timestamp match; use TalosSnapshot.from_dict and snapshot.to_dict to
perform the equality check (or assert each key individually) so the
serialization/deserialization contract is fully validated.
- Around line 30-42: The test_evaluation_to_dict currently only asserts the
"reasons" field; update test_evaluation_to_dict to validate the full
serialization by comparing the output of TalosEvaluation.to_dict() against the
expected dict built from the same constructor inputs (level, previous_level,
cycles_at_level, wip_total, global_max, backlog_total, backlog_delta,
at_cap_projects, reasons). Locate the test function test_evaluation_to_dict and
the TalosEvaluation.to_dict usage and add assertions that each key/value pair in
the produced dict equals the corresponding input value (or perform a single
equality check against the complete expected dict) to ensure all fields are
covered.
---
Nitpick comments:
In `@talos/models.py`:
- Around line 45-48: The current assignments use list(payload.get(...)) which
will silently convert non-list iterables; instead, fetch the raw values from
payload (e.g., at_cap_val = payload.get("at_cap_projects", []) and proj_val =
payload.get("projects", [])), validate with isinstance(..., list) and raise a
TypeError if not a list, then assign copies (e.g., at_cap_projects =
at_cap_val.copy(), projects = proj_val.copy()) so callers cannot mutate internal
state; update the code paths that set at_cap_projects and projects in
talos/models.py to follow this pattern.
In `@tests/test_scorer.py`:
- Around line 13-15: Add a unit test to exercise the
TalosPolicy.write_block_cycles override: create a test (e.g.,
test_policy_can_raise_write_block_threshold) that calls compute_talos_level with
cycles set lower than the default threshold but below the policy's
write_block_cycles (for example, cycles=2 and
policy=TalosPolicy(write_block_cycles=3)) and assert the returned level reflects
the policy override (expected level 2 per the example). Locate usages around
test_policy_can_raise_friction_threshold and add this new test so both
friction_ratio and write_block_cycles fields are validated.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c52d9dbb-b60d-4763-b52a-217932f1c5b7
📒 Files selected for processing (8)
talos/__init__.pytalos/models.pytalos/runtime.pytalos/scorer.pytests/test_cycles.pytests/test_models.pytests/test_runtime.pytests/test_scorer.py
Summary
Testing
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests