fix(todo): guard create_todo against 6-char id collisions#696
Open
SuperMarioYL wants to merge 1 commit into
Open
fix(todo): guard create_todo against 6-char id collisions#696SuperMarioYL wants to merge 1 commit into
SuperMarioYL wants to merge 1 commit into
Conversation
Todo ids are `str(uuid.uuid4())[:6]` slugs (~16.7M space), so two todos for the same agent can collide on their first six hex chars. create_todo assigned the id with no uniqueness check, so a collision silently overwrote the existing todo while still reporting success — losing the prior todo's data. notes uses the same 6-char scheme and already guards this via _generate_note_id (added in usestrix#630, after note ids were aligned to the todo scheme in 940319f); todo, the original of that scheme, never got the guard. Add _generate_todo_id, which retries to an unused slug and returns None when the space is exhausted, and surface exhaustion as a per-task error (mirroring the module's existing bulk-error handling in update/mark/ delete) instead of overwriting. Add regression tests covering collision retry, exhaustion, and the end-to-end no-overwrite guarantee.
Contributor
Greptile SummaryThis PR prevents todo ID collisions from overwriting existing todos. The main changes are:
Confidence Score: 5/5The changed flow looks mergeable after a small cleanup to partial-create diagnostics.
strix/tools/todo/tools.py Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
strix/tools/todo/tools.py:353
**Partial Create Looks Failed**
When a batched create has both successful tasks and an ID-generation exhaustion, this returns `success: false` even though `created` contains persisted todos. The create-todo renderer branches on `success` and falls back to a generic `Failed to create todo` message because this response has `errors` but no top-level `error`, so users lose the IDs for the todos that were actually created.
Reviews (1): Last reviewed commit: "fix(todo): guard create_todo against 6-c..." | Re-trigger Greptile |
| if created: | ||
| _persist() | ||
| response: dict[str, Any] = { | ||
| "success": len(errors) == 0, |
Contributor
There was a problem hiding this comment.
When a batched create has both successful tasks and an ID-generation exhaustion, this returns success: false even though created contains persisted todos. The create-todo renderer branches on success and falls back to a generic Failed to create todo message because this response has errors but no top-level error, so users lose the IDs for the todos that were actually created.
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/tools/todo/tools.py
Line: 353
Comment:
**Partial Create Looks Failed**
When a batched create has both successful tasks and an ID-generation exhaustion, this returns `success: false` even though `created` contains persisted todos. The create-todo renderer branches on `success` and falls back to a generic `Failed to create todo` message because this response has `errors` but no top-level `error`, so users lose the IDs for the todos that were actually created.
How can I resolve this? If you propose a fix, please make it concise.
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
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.
What
create_todogenerates a per-agent todo id with no uniqueness check:Ids are only 6 hex chars (
str(uuid.uuid4())[:6], ~16.7M space), so twotodos for the same agent can collide on their first six chars. When that
happens the second assignment silently overwrites the existing todo in
agent_todos, yet the tool still returnssuccess: Trueand reports it ascreated — the prior todo's data is lost with no error.
Why it matters (and the parity gap)
The sibling
notestool uses the same 6-char scheme and already guardsagainst exactly this:
Git history shows the guard was a deliberate fix —
940319f("Align note IDswith todo IDs (6-char hex)") aligned note ids to the todo scheme, then #630
("avoid note ID collisions") added the retry guard to
notes.todo— theoriginal of that 6-char scheme — was never given the same protection.
This is orthogonal to #663 / #670, which fix the id lookup path
(
_normalize_todo_idsmangling numeric-looking ids). This PR fixes the idgeneration path (
create_todo), a different function with no overlap.Reproduction (current
main)Forcing a collision (existing id
abcdef, nextuuid4()also maps toabcdef):The fix
_generate_todo_id(agent_todos), which retries to an unused slug andreturns
Noneif the space is exhausted (mirrorsnotes._generate_note_id).create_todonow surfaces an exhausted id space as a per-task error,mirroring the module's existing bulk-error handling in
update/mark/delete, instead of overwriting. The normal-path response shape isunchanged (
errorsonly appears when a task actually fails).Tests
New
tests/test_todo_id_generation.py(separate file to avoid colliding with#663's
tests/test_todo.py) covers: collision retry, id-space exhaustion, theend-to-end no-overwrite guarantee, and the exhaustion-reports-error path. Full
suite:
88 passed.ruff,mypy strix/, andbanditare clean.