Lists store hardening: list-scoped reorder + atomic position allocation - #2265
Conversation
reorder_entries now requires list_id, scopes UPDATE by list_id, and returns False (rolling back) when any supplied entry id does not belong to that list, preventing silent corruption of sibling lists in the same project. add_entry allocates position atomically inside the INSERT via COALESCE((SELECT MAX(position)+1 ...), 0) when position is None, eliminating the concurrent-duplicate-position race that occurred when MAX(position)+1 was read in a separate query.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe store now assigns omitted entry positions within the target list and project. Reordering requires List ordering
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
|
|
nemotron-ultra-kilo review VERDICT: The changes introduce a race condition in auto-position assignment, a test that doesn't exercise the new code path, and a breaking API change without full test coverage.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
PR Summary by QodoHarden lists store: list-scoped reorder + atomic position allocation
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/projects/test_lists_store.py (1)
280-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest rollback after a preceding valid update.
This test submits only the sibling-list entry, so no prior update exists to roll back. Submit
afirst with a changed position, then submitb, and assert thatastill has position0. This verifies the required all-or-nothing behavior.Proposed test change
result = await entries_store.reorder_entries( project_id="prj-1", list_id="lst-A", - entries=[{"id": b["id"], "position": 99}], + entries=[ + {"id": a["id"], "position": 1}, + {"id": b["id"], "position": 99}, + ], ) + a_after = await entries_store.get_entry(a["id"]) b_after = await entries_store.get_entry(b["id"]) + assert a_after["position"] == 0, "prior updates must roll back" assert b_after["position"] == 0, "sibling-list entry must not be moved" assert result is False, "reorder should signal that id does not belong to list"🤖 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/projects/test_lists_store.py` around lines 280 - 288, Update the reorder_entries test to submit valid entry a with a changed position before sibling-list entry b in the same request. After the call, assert a remains at position 0 and b is unchanged, and preserve the assertion that result is False to verify the operation rolls back all updates when any entry does not belong to the list.
🤖 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.
Nitpick comments:
In `@tests/projects/test_lists_store.py`:
- Around line 280-288: Update the reorder_entries test to submit valid entry a
with a changed position before sibling-list entry b in the same request. After
the call, assert a remains at position 0 and b is unchanged, and preserve the
assertion that result is False to verify the operation rolls back all updates
when any entry does not belong to the list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88085a26-7d64-4d1a-ae1a-e6e739623658
📒 Files selected for processing (2)
tests/projects/test_lists_store.pytinyagentos/projects/lists_store.py
|
nemotron-ultra-orB review VERDICT: Approved with race condition concern in auto-position assignment
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
Code Review by Qodo
1. Misleading concurrency test
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| await asyncio.sleep(0) | ||
| return val | ||
|
|
||
| monkeypatch.setattr(entries_store, "_get_next_position", slow_next_position) |
There was a problem hiding this comment.
1. Misleading concurrency test 🐞 Bug ⚙ Maintainability
test_concurrent_add_entry_distinct_positions monkeypatches _get_next_position to widen a race window, but add_entry no longer calls _get_next_position when position is None, so the patch never runs and the test’s intent is obscured. The same file also claims implicit positions are assigned “via _get_next_position”, which is no longer true, making the tests harder to maintain and reason about.
Agent Prompt
## Issue description
`test_concurrent_add_entry_distinct_positions` monkeypatches `ProjectListEntriesStore._get_next_position`, but `add_entry(..., position=None)` now assigns positions inside the SQL `INSERT` and never calls `_get_next_position`. This makes the monkeypatch (and its `asyncio.sleep(0)`) dead code and leaves misleading test comments.
## Issue Context
The store implementation switched implicit position allocation to `COALESCE((SELECT MAX(position)+1 ...), 0)` within the `INSERT`, bypassing `_get_next_position`.
## Fix Focus Areas
- tests/projects/test_lists_store.py[223-226]
- tests/projects/test_lists_store.py[292-324]
- tinyagentos/projects/lists_store.py[141-166]
## Suggested fix
- Update the docstring/comment in `test_add_entries_without_positions_gets_ascending` to describe SQL-based allocation (not `_get_next_position`).
- In `test_concurrent_add_entry_distinct_positions`, either:
- remove the `_get_next_position` monkeypatch entirely (simplest), or
- replace it with instrumentation that actually affects the current code path (e.g., patching/observing the DB execute boundary), if you still want a deterministic concurrency regression gate.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Reviewed at dfc9651, lead-completed at 4d5e21e. Verification:
Auto-merge armed on green. |
1 participant
CARD TITLE (intent, not commit subject): Lists store hardening: list-scoped reorder + atomic position allocation
Autonomous build of board card tsk-237k2v.
reorder_entries now requires list_id, scopes UPDATE by list_id,
and returns False (rolling back) when any supplied entry id does
not belong to that list, preventing silent corruption of sibling
lists in the same project.
add_entry allocates position atomically inside the INSERT via
COALESCE((SELECT MAX(position)+1 ...), 0) when position is None,
eliminating the concurrent-duplicate-position race that occurred
when MAX(position)+1 was read in a separate query.
Files:
tests/projects/test_lists_store.py | 72 +++++++++++++++++++++++++++++++++++++
tinyagentos/projects/lists_store.py | 49 +++++++++++++++++--------
2 files changed, 106 insertions(+), 15 deletions(-)
Summary by CodeRabbit
Bug Fixes
Improvements