fix(lists): add ProjectListsStore and fix ProjectListEntriesStore - #2183
fix(lists): add ProjectListsStore and fix ProjectListEntriesStore#2183jaylfc wants to merge 1 commit into
Conversation
Create lists_store.py with ProjectListsStore (list CRUD) and ProjectListEntriesStore (entry lifecycle, ordering, reorder, next-position). Register lst and ent id prefixes. Add unit tests for both stores covering add/get round-trip, position ordering, original_text immutability on update, delete, reorder, and sequential _get_next_position.
📝 WalkthroughWalkthroughAdds asynchronous SQLite-backed stores for project lists and list entries, including CRUD operations, filtering, timestamps, ordering, reordering, generated IDs, and comprehensive pytest coverage. ChangesProject lists persistence
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ProjectListsStore
participant ProjectListEntriesStore
participant SQLite
Caller->>ProjectListsStore: create or update project list
ProjectListsStore->>SQLite: persist project_lists row
SQLite-->>ProjectListsStore: return list record
Caller->>ProjectListEntriesStore: add, filter, or reorder entries
ProjectListEntriesStore->>SQLite: persist or query project_list_entries
SQLite-->>ProjectListEntriesStore: return ordered entry records
ProjectListEntriesStore-->>Caller: return entry results
Possibly related PRs
🚥 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 |
PR Summary by QodoAdd ProjectListsStore and stabilize ProjectListEntriesStore behaviors
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
|
Reviewed. The staging here is legitimate and I am fine with it, but I want to say why explicitly, because on the surface this looks like a PR I closed a few hours ago.
And this PR fixes a module that does not parse, which is strictly worse than an unwired store, so landing it improves One request, small but do it: add a trailer to the PR body making the staging explicit and auditable: That turns "this store is unreachable" from something a reviewer has to notice into something the author declared. I am carding a CI check for exactly this ( On the dependency, so it cannot quietly drop: until CI is still running (shards pending, Kilo and CodeRabbit in progress). I will arm auto-merge once required checks are green, assuming no bot turns up something substantive. |
|
nemotron-ultra-orB review VERDICT: LGTM with minor concerns
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
|
nemotron-ultra-kilo review VERDICT: Functional implementation with several correctness and safety concerns needing attention.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tinyagentos/projects/lists_store.py`:
- Around line 127-153: Update add_entry to call _get_next_position(project_id,
list_id) when position is None, storing the returned value instead of defaulting
to 0; preserve explicitly supplied positions. Extend
tests/projects/test_lists_store.py to create entries without positions and
verify they receive distinct sequential positions.
🪄 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 Plus
Run ID: 36091bf9-9064-497e-be67-dfc81c93edc4
📒 Files selected for processing (3)
tests/projects/test_lists_store.pytinyagentos/projects/ids.pytinyagentos/projects/lists_store.py
| async def add_entry( | ||
| self, | ||
| list_id: str, | ||
| project_id: str, | ||
| text: str, | ||
| original_text: str, | ||
| author_kind: str, | ||
| author_id: str, | ||
| category: str | None = None, | ||
| position: int | None = None, | ||
| ) -> dict: | ||
| entry_id = new_id("ent") | ||
| now = time.time() | ||
|
|
||
| await self._db.execute( | ||
| "INSERT INTO project_list_entries " | ||
| "(id, list_id, project_id, text, original_text, category, status, " | ||
| "done, author_kind, author_id, position, created_at, updated_at) " | ||
| "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", | ||
| ( | ||
| entry_id, list_id, project_id, text, original_text, | ||
| category, "new", 0, author_kind, author_id, | ||
| position if position is not None else 0, now, now, | ||
| ), | ||
| ) | ||
| await self._db.commit() | ||
| return await self.get_entry(entry_id) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Generate a position when callers omit one.
Line 149 always stores 0 for omitted positions, so multiple entries in the same list tie at position 0. Use _get_next_position(project_id, list_id) instead, and extend tests/projects/test_lists_store.py to create entries without explicit positions.
Proposed fix
entry_id = new_id("ent")
now = time.time()
+ if position is None:
+ position = await self._get_next_position(project_id, list_id)
await self._db.execute(
@@
- position if position is not None else 0, now, now,
+ position, now, now,📝 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.
| async def add_entry( | |
| self, | |
| list_id: str, | |
| project_id: str, | |
| text: str, | |
| original_text: str, | |
| author_kind: str, | |
| author_id: str, | |
| category: str | None = None, | |
| position: int | None = None, | |
| ) -> dict: | |
| entry_id = new_id("ent") | |
| now = time.time() | |
| await self._db.execute( | |
| "INSERT INTO project_list_entries " | |
| "(id, list_id, project_id, text, original_text, category, status, " | |
| "done, author_kind, author_id, position, created_at, updated_at) " | |
| "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", | |
| ( | |
| entry_id, list_id, project_id, text, original_text, | |
| category, "new", 0, author_kind, author_id, | |
| position if position is not None else 0, now, now, | |
| ), | |
| ) | |
| await self._db.commit() | |
| return await self.get_entry(entry_id) | |
| async def add_entry( | |
| self, | |
| list_id: str, | |
| project_id: str, | |
| text: str, | |
| original_text: str, | |
| author_kind: str, | |
| author_id: str, | |
| category: str | None = None, | |
| position: int | None = None, | |
| ) -> dict: | |
| entry_id = new_id("ent") | |
| now = time.time() | |
| if position is None: | |
| position = await self._get_next_position(project_id, list_id) | |
| await self._db.execute( | |
| "INSERT INTO project_list_entries " | |
| "(id, list_id, project_id, text, original_text, category, status, " | |
| "done, author_kind, author_id, position, created_at, updated_at) " | |
| "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", | |
| ( | |
| entry_id, list_id, project_id, text, original_text, | |
| category, "new", 0, author_kind, author_id, | |
| position, now, now, | |
| ), | |
| ) | |
| await self._db.commit() | |
| return await self.get_entry(entry_id) |
🤖 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 `@tinyagentos/projects/lists_store.py` around lines 127 - 153, Update add_entry
to call _get_next_position(project_id, list_id) when position is None, storing
the returned value instead of defaulting to 0; preserve explicitly supplied
positions. Extend tests/projects/test_lists_store.py to create entries without
positions and verify they receive distinct sequential positions.
Code Review by Qodo
1. Entry position always 0
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| class ProjectListsStore(BaseStore): | ||
| SCHEMA = LISTS_SCHEMA | ||
|
|
||
| async def create_list( | ||
| self, | ||
| project_id: str, | ||
| title: str, | ||
| created_by: str, | ||
| description: str = "", | ||
| status: str = "active", | ||
| ) -> dict: | ||
| list_id = new_id("lst") | ||
| now = time.time() | ||
| await self._db.execute( | ||
| "INSERT INTO project_lists " | ||
| "(id, project_id, title, description, status, created_by, created_at, updated_at) " | ||
| "VALUES (?, ?, ?, ?, ?, ?, ?, ?)", | ||
| (list_id, project_id, title, description, status, created_by, now, now), | ||
| ) | ||
| await self._db.commit() | ||
| return await self.get_list(list_id) | ||
|
|
||
| async def get_list(self, list_id: str) -> dict | None: | ||
| async with self._db.execute( | ||
| "SELECT * FROM project_lists WHERE id = ?", (list_id,) | ||
| ) as cur: | ||
| row = await cur.fetchone() | ||
| if row is None: | ||
| return None | ||
| keys = [d[0] for d in cur.description] | ||
| return dict(zip(keys, row)) | ||
|
|
||
| async def list_lists(self, project_id: str) -> list[dict]: | ||
| async with self._db.execute( | ||
| "SELECT * FROM project_lists WHERE project_id = ? ORDER BY created_at ASC", | ||
| (project_id,), | ||
| ) as cur: | ||
| rows = await cur.fetchall() | ||
| keys = [d[0] for d in cur.description] | ||
| return [dict(zip(keys, r)) for r in rows] | ||
|
|
||
| async def update_list( | ||
| self, | ||
| list_id: str, | ||
| title: str | None = None, | ||
| description: str | None = None, | ||
| status: str | None = None, | ||
| ) -> dict | None: | ||
| sets = [] | ||
| params = [] | ||
| if title is not None: | ||
| sets.append("title = ?") | ||
| params.append(title) | ||
| if description is not None: | ||
| sets.append("description = ?") | ||
| params.append(description) | ||
| if status is not None: | ||
| sets.append("status = ?") | ||
| params.append(status) | ||
| if not sets: | ||
| return await self.get_list(list_id) | ||
| sets.append("updated_at = ?") | ||
| params.append(time.time()) | ||
| params.append(list_id) | ||
| await self._db.execute( | ||
| f"UPDATE project_lists SET {', '.join(sets)} WHERE id = ?", params | ||
| ) | ||
| await self._db.commit() | ||
| return await self.get_list(list_id) | ||
|
|
||
| async def delete_list(self, list_id: str) -> bool: | ||
| cursor = await self._db.execute( | ||
| "DELETE FROM project_lists WHERE id = ?", (list_id,) | ||
| ) | ||
| await self._db.commit() | ||
| return cursor.rowcount > 0 | ||
|
|
||
|
|
||
| class ProjectListEntriesStore(BaseStore): | ||
| SCHEMA = """ | ||
| CREATE TABLE IF NOT EXISTS project_list_entries ( | ||
| id TEXT PRIMARY KEY, | ||
| list_id TEXT NOT NULL, | ||
| project_id TEXT NOT NULL, | ||
| text TEXT NOT NULL, | ||
| original_text TEXT NOT NULL, | ||
| category TEXT, | ||
| status TEXT NOT NULL DEFAULT 'new', | ||
| done INTEGER NOT NULL DEFAULT 0, | ||
| author_kind TEXT NOT NULL, | ||
| author_id TEXT NOT NULL, | ||
| edited_by TEXT, | ||
| position INTEGER NOT NULL, | ||
| created_at REAL NOT NULL, | ||
| updated_at REAL NOT NULL | ||
| ); | ||
| CREATE INDEX IF NOT EXISTS idx_entries_list ON project_list_entries(list_id); | ||
| CREATE INDEX IF NOT EXISTS idx_entries_project ON project_list_entries(project_id); | ||
| CREATE INDEX IF NOT EXISTS idx_entries_list_project ON project_list_entries(list_id, project_id); | ||
| CREATE INDEX IF NOT EXISTS idx_entries_status ON project_list_entries(project_id, status); | ||
| """ |
There was a problem hiding this comment.
2. Project list stores not wired 📜 Skill insight ⌂ Architecture
The new ProjectListsStore and ProjectListEntriesStore are added but are not instantiated/attached to app.state in the application lifecycle, so they cannot be used by routes via request.app.state and won’t be initialized/closed with the app. This will lead to runtime errors or missing functionality once routes attempt to use these stores.
Agent Prompt
## Issue description
New stores `ProjectListsStore` and `ProjectListEntriesStore` are introduced but are not wired into the FastAPI app lifecycle (`create_app` + lifespan) via `app.state`.
## Issue Context
The codebase pattern is: instantiate store objects in `create_app`, attach them to `app.state.<name>`, `await store.init()` inside the lifespan startup sequence, and `await store.close()` during lifespan shutdown.
## Fix Focus Areas
- tinyagentos/app.py[396-418]
- tinyagentos/app.py[503-590]
- tinyagentos/app.py[1411-1448]
- tinyagentos/app.py[1596-1606]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| async def get_entry(self, entry_id: str) -> dict | None: | ||
| async with self._db.execute( | ||
| "SELECT * FROM project_list_entries WHERE id = ?", (entry_id,) | ||
| ) as cur: | ||
| row = await cur.fetchone() | ||
| if row is None: | ||
| return None | ||
| keys = [d[0] for d in cur.description] | ||
| return dict(zip(keys, row)) |
There was a problem hiding this comment.
3. Cursor metadata after close 🐞 Bug ☼ Reliability
ProjectListEntriesStore.get_entry() accesses cur.description after exiting the cursor context manager, making row-to-dict conversion depend on cursor metadata remaining available after the cursor is closed. This is inconsistent with other stores and can break entry reads depending on cursor behavior.
Agent Prompt
### Issue description
`ProjectListEntriesStore.get_entry()` reads `cur.description` after leaving `async with self._db.execute(...) as cur:`. That pattern depends on cursor metadata still being readable after the context manager closes the cursor.
### Issue Context
Other stores either (a) return the row inside the context or (b) copy `cur.description` to a local variable before leaving the context.
### Fix Focus Areas
- tinyagentos/projects/lists_store.py[155-163]
### Suggested fix
Move `keys = ...` and the `dict(zip(...))` construction inside the `async with` block, or capture `desc = cur.description` inside the block and use it after the block.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| await self._db.execute( | ||
| "INSERT INTO project_list_entries " | ||
| "(id, list_id, project_id, text, original_text, category, status, " | ||
| "done, author_kind, author_id, position, created_at, updated_at) " | ||
| "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", | ||
| ( | ||
| entry_id, list_id, project_id, text, original_text, | ||
| category, "new", 0, author_kind, author_id, | ||
| position if position is not None else 0, now, now, | ||
| ), |
There was a problem hiding this comment.
4. Entry position always 0 🐞 Bug ≡ Correctness
ProjectListEntriesStore.add_entry() writes position=0 whenever the caller omits position, so multiple entries will share the same position and won’t naturally append by position. This defeats the store’s own ordering rule (ORDER BY position, created_at) and leaves the existing _get_next_position() helper unused.
Agent Prompt
### Issue description
`add_entry()` defaults `position` to 0 when the caller does not supply one. This causes repeated inserts to collide on position and undermines the intended positional ordering.
### Issue Context
The store already defines `_get_next_position()` and `list_entries()` sorts by `position ASC, created_at ASC`, so the natural behavior when `position` is omitted should be to append at the end by assigning the next position.
### Fix Focus Areas
- tinyagentos/projects/lists_store.py[127-153]
- tinyagentos/projects/lists_store.py[165-192]
- tinyagentos/projects/lists_store.py[255-262]
### Suggested fix
In `add_entry()`, when `position is None`, compute `position = await self._get_next_position(project_id, list_id)` and insert using that value (optionally within a transaction if concurrent inserts are expected).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| async def _get_next_position(self, project_id: str, list_id: str | None = None) -> int: | ||
| async with self._db.execute( | ||
| "SELECT MAX(position) + 1 FROM project_list_entries " | ||
| "WHERE project_id = ? AND list_id = ?", | ||
| (project_id, list_id or ""), | ||
| ) as cur: |
There was a problem hiding this comment.
5. Next-position none never matches 🐞 Bug ≡ Correctness
ProjectListEntriesStore._get_next_position() accepts list_id=None but still filters with `list_id = ?` using an empty-string substitute, so calls with None will always return 0 regardless of existing entries. Either list_id should be required, or the SQL should omit the list_id predicate when list_id is None.
Agent Prompt
### Issue description
`_get_next_position(project_id, list_id=None)` currently binds `""` for `list_id` but still executes `... WHERE project_id = ? AND list_id = ?`, which can never match normal rows (schema stores real list IDs).
### Issue Context
The function signature suggests `None` is a supported input; the current SQL doesn’t implement any meaningful `None` behavior.
### Fix Focus Areas
- tinyagentos/projects/lists_store.py[255-262]
### Suggested fix
Choose one:
1) Make `list_id` required (remove `| None` and default), and always query with `list_id = ?`, OR
2) If `None` means “any list in project”, build SQL conditionally:
- when `list_id is None`: `WHERE project_id = ?`
- else: `WHERE project_id = ? AND list_id = ?`
Add/adjust a unit test to lock in the intended semantics.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @pytest.mark.asyncio | ||
| async def test_update_list_changes_fields(lists_store): | ||
| l = await lists_store.create_list( | ||
| project_id="prj-1", title="Original", created_by="u" | ||
| ) | ||
| updated = await lists_store.update_list(l["id"], title="Renamed", status="archived") | ||
| assert updated["title"] == "Renamed" | ||
| assert updated["status"] == "archived" | ||
| assert updated["updated_at"] > l["updated_at"] | ||
|
|
There was a problem hiding this comment.
6. Flaky updated_at assertion 🐞 Bug ☼ Reliability
test_update_list_changes_fields asserts updated_at is strictly greater than the prior value without controlling clock resolution, so create/update can share the same timestamp on coarse time sources. This can intermittently fail CI while the implementation remains correct for >= semantics.
Agent Prompt
### Issue description
The test asserts `updated_at` strictly increases across two `time.time()` calls without guaranteeing a measurable time delta.
### Issue Context
On some systems/configurations, consecutive `time.time()` calls can return the same value.
### Fix Focus Areas
- tests/projects/test_lists_store.py[52-61]
### Suggested fix
Prefer a deterministic approach:
- Relax to `>=` if strict monotonicity is not required, OR
- Monkeypatch `time.time()` to return controlled increasing values during the test (avoids adding sleeps).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| row = await cur.fetchone() | ||
| if row is None: | ||
| return None | ||
| keys = [d[0] for d in cur.description] |
There was a problem hiding this comment.
WARNING: cur.description accessed outside async with block
get_entry reads cur.description after the cursor context manager exits (line 162), while get_list reads it inside the block (line 53). If the cursor is closed on exit, cur.description may be unavailable, causing a runtime error. Move the key extraction inside the async with block to match get_list.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| await self._db.commit() | ||
| return cursor.rowcount == 1 | ||
|
|
||
| async def reorder_entries(self, project_id: str, entries: list[dict]) -> None: |
There was a problem hiding this comment.
WARNING: reorder_entries is not atomic — partial reorders can persist
Multiple UPDATE statements are executed in a loop without an explicit transaction. If an error occurs mid-loop, already-executed writes remain committed while later ones are lost, leaving entries in a partially reordered state. Wrap the loop in BEGIN/ROLLBACK/COMMIT (see todo_store.py:reorder_items for the established pattern).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ( | ||
| entry_id, list_id, project_id, text, original_text, | ||
| category, "new", 0, author_kind, author_id, | ||
| position if position is not None else 0, now, now, |
There was a problem hiding this comment.
WARNING: add_entry defaults position to 0, risking collision
When position=None, all entries receive position=0. The private _get_next_position helper exists but is never called by add_entry, so callers that omit position create duplicate positions and break ordering. Either wire _get_next_position into add_entry or remove the optionality.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| async with self._db.execute( | ||
| "SELECT MAX(position) + 1 FROM project_list_entries " | ||
| "WHERE project_id = ? AND list_id = ?", | ||
| (project_id, list_id or ""), |
There was a problem hiding this comment.
WARNING: _get_next_position converts None list_id to empty string
list_id or "" silently turns a missing list_id into "", which could match entries with an empty list_id and return an incorrect next position. Raise ValueError or skip the list_id filter when list_id is None.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Warnings Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 93.7K · Output: 16.6K · Cached: 608K |
|
nemotron-super review VERDICT: Blocking issue found: position defaults to 0 instead of next position in list.
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
Not arming auto-merge. I said I would once checks were green if no bot found anything substantive. Checks are green, and Qodo found something substantive that I verified against the code. It is real, and slightly worse than reported. Every entry added without an explicit position gets position 0. position if position is not None else 0,So three entries added with the default land at positions 0, 0, 0. A list store whose entire job is ordered entries silently produces an unordered list, and nothing errors. Callers that do not pass And the fix is already written but never wired. That is the same orphan pattern as #2172, one level down: the right code exists, nothing reaches it. It is also why the tests pass — they presumably pass explicit positions, so the default path is never exercised. Fix: default to the next position rather than 0. if position is None:
position = await self._get_next_position(project_id, list_id)And a test that would have caught it, which matters more than the fix: add three entries without passing Two smaller things while you are in there:
Everything I said earlier stands: the staging is legitimate, the tests are a genuine improvement on #2172, and this fixes a module that does not parse. Fix the position default and I will merge it. |
|
Holding this rather than merging. All four warnings are real, I checked each against the branch, and one of them is described wrongly by the bot in a way that matters. This is a brand new store that nothing depends on yet, so the cost of fixing now is a short delay, and the cost of merging is four known defects sitting in a foundation other code gets built on. That trade is not close. 1. position if position is not None else 0,
2. "WHERE project_id = ? AND list_id = ?", (project_id, list_id or "")
3. async with self._db.execute(...) as cur:
row = await cur.fetchone()
if row is None:
return None
keys = [d[0] for d in cur.description]The 4. Kilo says "partial reorders can persist". As written the loop issues every UPDATE inside one implicit transaction and commits once, so a clean run is atomic and that description is not right. The actual hazard is the failure path: if one UPDATE raises, the earlier ones stay pending with no rollback, and the next unrelated I am pointing this out because "not atomic" would send someone rewriting the commit structure, which is not the bug. On the green. 19/19 passed with all four of these present, so the tests do not cover position assignment for a second entry, do not cover the NULL-list path, and do not exercise the reorder failure path. Whatever fixes the above should add those, and the position test in particular must fail against this commit. Fix-forward card to follow, and this PR closes in favour of it. |
|
Superseding my 03:59 hold: no separate fix-forward card, @hognek fix-forward on THIS branch instead, same batch as the others. The fix is small and the rest of the slice is sound. Required, both verified still present at head 26ed84c:
Also add the PR-body trailer from my earlier comment: Store-Unwired-Intentionally: ProjectListsStore, wiring lands in tsk-epm5m3. Adjudicated bot findings so you do not chase ghosts: the cur.description-after-close warning is a false positive on CPython sqlite3 (move it inside the block as a style nit if you like); the nemotron SQL-injection claim is false (columns hardcoded, values parameterized); kilo's reorder atomicity claim is wrong as stated but the real hazard is an exception mid-loop leaving a half-applied reorder in the open transaction for a later unrelated commit to flush: try/except + rollback is the fix. Reorder payload validation vs the todo_store pattern can ride along or go to a follow-up. |
|
@hognek nudge: head is still 26ed84c, unchanged since my 15:16 fix-forward routing, and this PR is holding a throttle slot. The two required fixes are small and unchanged: lists_store.py:149 add_entry must call _get_next_position when position is None (it currently defaults every entry to 0, and _get_next_position has zero call sites), and lists_store.py:255 _get_next_position should take a required list_id rather than binding list_id or empty-string on the None path. Plus the mandatory test: add three entries without positions, assert distinct ascending positions, proven failing against the current head first. For the record so it is not re-attempted the wrong way: a board card asking a free lane to fix-forward this branch cannot work (the harness builds a fresh worktree and its own branch), so I have closed that card. This one needs you or me. |
|
Superseded by #2200 |
- add_entry: replace hardcoded 0 default with await _get_next_position() - _get_next_position: make list_id a required param, remove dead or '' binding - New test: 3 entries without positions assert distinct ascending [0,1,2] (test fails against current head where all get position 0) Closes #2183
|
Superseded by #2200, now merged (squash of fix/lists-add-entry-position). #2200 carries both lists_store fixes plus the ids.py lst prefix; verified at code level with red-first test proofs before merge. Follow-ups from the review (store wiring + route-layer validation, reorder list scoping, position atomicity) are being carded on the board rather than reopened here. |
2 participants
Autonomous build of board card tsk-jos7xm.
Create lists_store.py with ProjectListsStore (list CRUD) and
ProjectListEntriesStore (entry lifecycle, ordering, reorder, next-position).
Register lst and ent id prefixes. Add unit tests for both stores covering
add/get round-trip, position ordering, original_text immutability on update,
delete, reorder, and sequential _get_next_position.
Files:
tests/projects/test_lists_store.py | 218 ++++++++++++++++++++++++++++++
tinyagentos/projects/ids.py | 2 +-
tinyagentos/projects/lists_store.py | 262 ++++++++++++++++++++++++++++++++++++
3 files changed, 481 insertions(+), 1 deletion(-)
Summary by CodeRabbit
New Features
Tests