tsk-mz3diu [OPEN] Project create: name uniqueness + dup auto-reject - #2168
Conversation
📝 WalkthroughWalkthroughProject creation now detects case-insensitive name and slug collisions, raises structured ChangesProject collision handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant create_project
participant ProjectStore
participant SQLite
Client->>create_project: Submit project name and slug
create_project->>ProjectStore: Create project
ProjectStore->>SQLite: Check name and insert project
SQLite-->>ProjectStore: Conflict or success
ProjectStore-->>create_project: ProjectConflict
create_project->>ProjectStore: Check candidate values
ProjectStore-->>create_project: Available suggestions
create_project-->>Client: Return 409 conflict response
Suggested reviewers: 🚥 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 QodoEnforce case-insensitive project name uniqueness with actionable 409 conflicts
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }, | ||
| status_code=409, | ||
| ) | ||
| except ValueError as e: |
There was a problem hiding this comment.
WARNING: Dead except ValueError block — ProjectConflict inherits from ValueError and is caught first, so this handler no longer catches slug collisions.
If store.create_project() no longer raises any other ValueError, this block is unreachable and misleading.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return await store.get_project_by_slug(value) is None | ||
|
|
||
|
|
||
| async def _free_suggestions(store, field: str, taken: str) -> list[str]: |
There was a problem hiding this comment.
WARNING: _free_suggestions lacks error handling around its database calls.
If any query inside the suggestion loops fails (e.g., connection drop, disk I/O error), the exception propagates out of _free_suggestions and turns the intended 409 into a 500. Wrap the DB calls in a try/except and return an empty or fallback suggestions list instead.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| import pytest_asyncio | ||
|
|
||
| from tinyagentos.projects.project_store import ProjectStore | ||
| from tinyagentos.projects.project_store import ProjectConflict, ProjectStore |
There was a problem hiding this comment.
SUGGESTION: ProjectConflict is imported but unused in this file.
No test currently references ProjectConflict (the existing duplicate-slug test still expects ValueError). Either remove the unused import or add a test that asserts ProjectConflict is raised for duplicate names.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
nemotron-ultra-orB review VERDICT: Significant race conditions in name/slug uniqueness checks; missing tests for new conflict handling; fragile error message parsing.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 71.7K · Output: 14.4K · Cached: 365.4K |
Code Review by Qodo
1. Racy name uniqueness
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| from tinyagentos.projects.project_store import ProjectConflict | ||
| from tinyagentos.projects.task_store import _ELEMENT_CLEAR |
There was a problem hiding this comment.
1. projectconflict imported from store 📜 Skill insight ⌂ Architecture
tinyagentos/routes/projects.py directly imports ProjectConflict from tinyagentos.projects.project_store, which violates the requirement that routes should not directly import store modules/classes. This increases coupling and bypasses the intended request.app.state.<store> access pattern.
Agent Prompt
## Issue description
Route modules must not directly import store modules/classes. `tinyagentos/routes/projects.py` imports `ProjectConflict` from `tinyagentos.projects.project_store`.
## Issue Context
The compliance rule requires stores be accessed via `request.app.state` and avoids direct store imports in route modules to reduce coupling.
## Fix Focus Areas
- tinyagentos/routes/projects.py[25-26]
- tinyagentos/projects/project_store.py[14-25]
## Suggested approach
- Create a small non-store module for shared exceptions, e.g. `tinyagentos/projects/errors.py`.
- Move `ProjectConflict` there.
- Update imports:
- Store: `from tinyagentos.projects.errors import ProjectConflict`
- Route: `from tinyagentos.projects.errors import ProjectConflict`
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Enforce case-insensitive name uniqueness via a query check (not a | ||
| # schema constraint) so existing duplicate names are not destructively | ||
| # rejected on upgrade. A UNIQUE(slug) constraint remains as the | ||
| # backstop for slug collisions caught via IntegrityError below. | ||
| if await self.get_project_by_name(name) is not None: | ||
| raise ProjectConflict("name", name) |
There was a problem hiding this comment.
3. No test for name uniqueness 📜 Skill insight ▣ Testability
The PR adds case-insensitive project name uniqueness enforcement, but no regression test was added to ensure duplicate names (including case variations) are rejected. This risks the behavior silently regressing in future changes.
Agent Prompt
## Issue description
A bug-fix/behavior-fix was added (case-insensitive project name uniqueness), but there is no regression test that would fail without this change.
## Issue Context
The store now checks `get_project_by_name(name)` before insert and raises `ProjectConflict("name", name)` on collision. This should be covered by tests, especially for case-insensitive collisions.
## Fix Focus Areas
- tinyagentos/projects/project_store.py[152-183]
- tests/projects/test_project_store.py[1-50]
## Suggested approach
- Add a new test, e.g.:
- Create project with `name="MyProj"`
- Attempt to create `name="myproj"` (same slug different) and assert it raises `ProjectConflict` (or at least `ValueError`) and that `e.field == "name"`.
- Optionally add a route-level test asserting `/api/projects` returns 409 and includes `suggestions`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Enforce case-insensitive name uniqueness via a query check (not a | ||
| # schema constraint) so existing duplicate names are not destructively | ||
| # rejected on upgrade. A UNIQUE(slug) constraint remains as the | ||
| # backstop for slug collisions caught via IntegrityError below. | ||
| if await self.get_project_by_name(name) is not None: | ||
| raise ProjectConflict("name", name) |
There was a problem hiding this comment.
4. Racy name uniqueness 🐞 Bug ☼ Reliability
ProjectStore.create_project enforces name uniqueness via a pre-INSERT query, but without a DB constraint or write lock, concurrent creates can both pass the check and insert duplicate (case-insensitive) names. This violates the new uniqueness guarantee and can persist duplicate names in the database.
Agent Prompt
## Issue description
`ProjectStore.create_project()` checks name availability with `get_project_by_name()` and then inserts, but this is not atomic; concurrent requests can create duplicate names.
## Issue Context
Slug uniqueness is protected by a schema UNIQUE constraint and handled via `IntegrityError`, but name uniqueness is only an application-level pre-check.
## Fix Focus Areas
- tinyagentos/projects/project_store.py[26-41]
- tinyagentos/projects/project_store.py[152-183]
- tinyagentos/projects/project_store.py[203-211]
## Suggested fix
Implement an atomic enforcement strategy for case-insensitive name uniqueness, e.g.:
1) **Preferred (if migration allows):** add a unique index on `LOWER(name)` (case-insensitive) and catch `sqlite3.IntegrityError` to raise `ProjectConflict("name", name)` similarly to slug.
2) **If you cannot add the index due to existing duplicates:** wrap the `get_project_by_name()` check + INSERT in a transaction that prevents concurrent writers (SQLite: `BEGIN IMMEDIATE` / `COMMIT`, and rollback on failure) so only one creator can pass the check.
Also consider adding/adjusting tests to cover concurrent creates with the same name using separate tasks/connections.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| async def _free_suggestions(store, field: str, taken: str) -> list[str]: | ||
| """Generate 2-3 free suggestions for a collided name or slug. | ||
|
|
||
| Each candidate is verified against the store so it is genuinely free. | ||
| Formats: numeric suffix (<value>-2), prefixed (<prefix>-<value>), | ||
| and short random suffix (<value>-<shortid>). | ||
| """ | ||
| suggestions: list[str] = [] | ||
|
|
||
| # 1. Numeric suffix: <value>-2, <value>-3, ... | ||
| for i in range(2, 10): | ||
| cand = f"{taken}-{i}" | ||
| if await _is_field_free(store, field, cand): | ||
| suggestions.append(cand) | ||
| break | ||
|
|
||
| # 2. Prefixed: <prefix>-<value> | ||
| for prefix in ("team", "new", "app"): | ||
| cand = f"{prefix}-{taken}" | ||
| if await _is_field_free(store, field, cand): | ||
| suggestions.append(cand) | ||
| break | ||
|
|
||
| # 3. Short random suffix: <value>-<shortid> | ||
| for _ in range(20): | ||
| shortid = uuid.uuid4().hex[:5] | ||
| cand = f"{taken}-{shortid}" | ||
| if await _is_field_free(store, field, cand): | ||
| suggestions.append(cand) | ||
| break |
There was a problem hiding this comment.
5. Invalid slug suggestions 🐞 Bug ≡ Correctness
The 409 handler generates slug suggestions by prefixing/appending to the taken slug without validating candidates against the API’s slug regex/length limit. For near-max-length slugs, returned suggestions can be unusable because the create endpoint will reject them during request validation.
Agent Prompt
## Issue description
`_free_suggestions()` returns candidate slugs that may not satisfy the `CreateProjectIn.slug` validator (regex + max length). This can produce conflict responses containing suggestions that the API will reject.
## Issue Context
Slug validity is enforced by `_SLUG_RE` and the `CreateProjectIn` validator, but `_free_suggestions()` only checks for availability in the DB.
## Fix Focus Areas
- tinyagentos/routes/projects.py[31-86]
- tinyagentos/routes/projects.py[41-72]
## Suggested fix
When `field == "slug"`:
- Filter candidates by `_SLUG_RE.fullmatch(cand)` before calling `_is_field_free()`.
- Ensure candidates stay within the 63-character limit. For suffix/prefix generation, truncate the base portion to leave room for `-2`, `team-`, `-<shortid>`, etc.
- If no valid suggestions can be generated, return an empty list (or only valid ones) rather than invalid suggestions.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
nemotron-ultra-kilo review VERDICT: Request changes — race condition in name uniqueness check, missing tests for new conflict handling, and suggestion generation has TOCTOU issues.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tinyagentos/projects/project_store.py (1)
163-181: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce case-insensitive project name uniqueness in the database.
create_projectchecksget_project_by_nameseparately from theINSERT, so concurrent create requests can both pass the lookup and then both insert duplicateLOWER(name)values. Add a normalized-name unique key/index in a migration after resolving existing duplicates, and translate itsIntegrityErrortoProjectConflict; the existing name lookup can then remain only as a 409 hint for existing legacy duplicates.🤖 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/project_store.py` around lines 163 - 181, Update the project schema migration and create_project flow to enforce case-insensitive name uniqueness at the database level: resolve existing duplicate LOWER(name) values before adding a normalized-name unique key or index, then handle its sqlite3.IntegrityError as ProjectConflict("name", name) while retaining get_project_by_name as the legacy-duplicate hint. Keep the existing slug uniqueness handling unchanged.
🤖 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/project_store.py`:
- Around line 203-210: Replace the ASCII-only LOWER(name) comparison used by
get_project_by_name and project creation collision checks with Unicode-aware
casefolded key handling. Add or reuse a persisted normalized casefold key for
each project, ensure it is populated and uniquely constrained, and query it
consistently so names such as straße and STRASSE collide.
---
Outside diff comments:
In `@tinyagentos/projects/project_store.py`:
- Around line 163-181: Update the project schema migration and create_project
flow to enforce case-insensitive name uniqueness at the database level: resolve
existing duplicate LOWER(name) values before adding a normalized-name unique key
or index, then handle its sqlite3.IntegrityError as ProjectConflict("name",
name) while retaining get_project_by_name as the legacy-duplicate hint. Keep the
existing slug uniqueness handling unchanged.
🪄 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: 3e666950-10e4-4ab4-8782-27f07008c243
📒 Files selected for processing (3)
tests/projects/test_project_store.pytinyagentos/projects/project_store.pytinyagentos/routes/projects.py
| async def get_project_by_name(self, name: str) -> dict | None: | ||
| async with self._db.execute( | ||
| "SELECT * FROM projects WHERE LOWER(name) = LOWER(?)", (name,) | ||
| ) as cur: | ||
| row = await cur.fetchone() | ||
| if row is None: | ||
| return None | ||
| return _row_to_project(row, cur.description) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -eu
ast-grep outline tinyagentos/projects/project_store.py --items all
rg -n -C3 'enable_load_extension|load_extension|create_collation|casefold|class CreateProjectIn|name.*(pattern|max_length)' tinyagentosRepository: jaylfc/taOS
Length of output: 6645
🏁 Script executed:
#!/usr/bin/env bash
set -eu
printf '--- project_store.py relevant sections ---\n'
sed -n '1,180p' tinyagentos/projects/project_store.py
printf '\n--- routes/projects.py CreateProjectIn section ---\n'
sed -n '1,140p' tinyagentos/routes/projects.py
printf '\n--- project route create project call sites ---\n'
rg -n -C4 'CreateProjectIn|create_project\(' tinyagentos/routes/projects.py tinyagentos -g '*.py'Repository: jaylfc/taOS
Length of output: 240
🏁 Script executed:
#!/usr/bin/env bash
set -eu
printf '%s\n' '--- project_store.py relevant sections ---'
sed -n '1,180p' tinyagentos/projects/project_store.py
printf '%s\n' '--- routes/projects.py CreateProjectIn section ---'
sed -n '1,140p' tinyagentos/routes/projects.py
printf '%s\n' '--- project route create project call sites ---'
rg -n -C4 'CreateProjectIn|create_project\(' tinyagentos/routes/projects.py tinyagentos -g '*.py'Repository: jaylfc/taOS
Length of output: 17422
🌐 Web query:
SQLite LOWER() function ASCII only UNICODE NOT IGNORE_CASE documentation
💡 Result:
The SQLite lower function, by default, is designed to convert only ASCII characters (the 26 letters of the English alphabet) to lowercase [1][2][3]. It does not provide full Unicode support for case conversion [2][4]. Key points regarding its behavior include: 1. Scope: The default built-in lower function handles 7-bit ASCII characters only [1][2][4]. Non-ASCII characters (such as accented characters or those from other scripts) remain unchanged when passed to this function [4]. 2. Unicode Support: SQLite does not include full Unicode case folding by default because the necessary tables are very large and Unicode case-folding rules can evolve, which could disrupt applications [2]. 3. Enabling Full Unicode: To perform case conversions on non-ASCII Unicode characters, you must compile SQLite with the SQLITE_ENABLE_ICU option and link it against the International Components for Unicode (ICU) library [5][2]. When this extension is loaded, the lower function is enhanced to support the full range of Unicode characters [5]. 4. Extension Usage: The ICU extension allows for both general case mapping (one argument: lower('ABC') -> 'abc') and language-specific case mapping (two arguments: lower('I', 'tr_tr') -> 'ı') [5]. For more information on the ICU extension and its implementation, you can refer to the official SQLite documentation on built-in scalar functions and the ICU extension files [1][5].
Citations:
- 1: https://sqlite.org/lang_corefunc.html
- 2: https://sqlite.org/quirks.html
- 3: https://database.guide/how-sqlite-lower-works/
- 4: http://www.iljitsch.com/2023/09-05-looking-at-sqlite-unicode-behavior.html
- 5: https://www.sqlite.org/src/dir?ci=trunk&name=ext%2Ficu
🌐 Web query:
SQLite LOWER('ß') ASCII only behavior
💡 Result:
In SQLite, the default built-in LOWER function operates exclusively on ASCII characters [1][2]. Because the character 'ß' (German Eszett) is a non-ASCII Unicode character, the default LOWER function will not transform it and will return the character unchanged [1][3]. This behavior is a deliberate design choice in SQLite to maintain a lightweight footprint and avoid the complexities and potential database corruption issues that could arise if built-in case-folding rules changed alongside evolving Unicode standards [4][5]. To achieve full Unicode-aware case folding for characters like 'ß', you have two primary options: 1. Load the ICU (International Components for Unicode) extension: This official SQLite extension provides enhanced versions of the UPPER and LOWER functions as well as the LIKE operator, which support full Unicode case mapping and locale-aware comparisons [1][6][7][8]. 2. Application-level handling: You can manually implement case folding in your application code or use custom user-defined SQL functions and collations registered with the database connection to handle specific requirements, such as mapping 'ß' to 'ss' [4][9][2].
Citations:
- 1: https://sqlite.org/lang_corefunc.html
- 2: https://stackoverflow.com/questions/2666990/does-sqlite-handle-non-english-locales-out-of-the-box
- 3: https://www.iljitsch.com/2023/09-05-looking-at-sqlite-unicode-behavior.html
- 4: https://sqlite.org/forum/info/2d109564bb7cc203c1410b1f6961e3bdeae388c775f96687527b5461b12d3aae?t=c
- 5: https://sqlite.org/forum/forumpost/24efd5aa505c732f?raw=
- 6: https://www.sqlite.org/lang_expr.html
- 7: https://www.sqlite.org/src/artifact?ci=trunk&filename=ext%2Ficu%2FREADME.txt
- 8: https://sqlite.org/cgi/src/dir?ci=tip&name=ext/icu
- 9: https://stackoverflow.com/questions/24262572/sqlite-german-special-chars
🌐 Web query:
Python sqlite3 create_function unicode casefold example
💡 Result:
To create a custom SQL function in Python's sqlite3 module for unicode-aware case-insensitive comparisons, you can use the connection.create_function method to register Python's str.casefold function [1][2]. The casefold method is generally preferred over lower for caseless matching, as it is more aggressive and correctly handles special Unicode characters (such as the German 'ß', which becomes 'ss') [1][2]. Example implementation: import sqlite3 def casefold_func(s): return s.casefold if s is not None else None # Connect to the database conn = sqlite3.connect(":memory:") # Register the function # deterministic=True is recommended for functions that always return # the same output for the same input, allowing SQLite to optimize queries. conn.create_function("CASEFOLD", 1, casefold_func, deterministic=True) # Usage in SQL cursor = conn.cursor cursor.execute("CREATE TABLE users (name TEXT)") cursor.execute("INSERT INTO users (name) VALUES ('Straße')") # Query using the registered function cursor.execute("SELECT * FROM users WHERE CASEFOLD(name) = CASEFOLD(?)", ("STRASSE",)) print(cursor.fetchone) # Output: ('Straße',) conn.close Key details: 1. Method Signature: The create_function method takes the name of the function as it will appear in SQL, the number of arguments (narg), and the Python callable [3][4]. 2. Deterministic Parameter: Setting deterministic=True (available in Python 3.8+) is beneficial for performance if your function's output depends only on its inputs, as it enables SQLite to optimize queries involving the function [1][3]. 3. Handling None: Ensure your Python function handles potential NULL (None) values if your database columns are nullable [3][5]. 4. Alternative approach: If you intend to use this for sorting or ordering, you might consider implementing a custom collation using connection.create_collation instead, which defines how strings are compared rather than transforming them [1][2].
Citations:
- 1: https://dev.to/shallowdepth/5-ways-to-implement-case-insensitive-search-in-sqlite-with-full-unicode-support-53p2
- 2: https://shallowdepth.online/posts/2022/01/5-ways-to-implement-case-insensitive-search-in-sqlite-with-full-unicode-support/
- 3: https://docs.python.org/3/library/sqlite3.html
- 4: https://docs.python.org/3.11/library/sqlite3.html
- 5: https://docs.python.org/3.9/library/sqlite3.html
Avoid ASCII-only SQLite LOWER() for project name collision checks.
CreateProjectIn.name has no ASCII constraint, so names like straße and STRASSE could both pass the schema and pass LOWER() lookup separately in plain SQLite. Store/collate a Unicode casefolded key, or enforce ASCII-only project names.
🤖 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/project_store.py` around lines 203 - 210, Replace the
ASCII-only LOWER(name) comparison used by get_project_by_name and project
creation collision checks with Unicode-aware casefolded key handling. Add or
reuse a persisted normalized casefold key for each project, ensure it is
populated and uniquely constrained, and query it consistently so names such as
straße and STRASSE collide.
1 participant
Autonomous build of board card tsk-mz3diu.
Files:
tests/projects/test_project_store.py | 2 +-
tinyagentos/projects/project_store.py | 33 +++++++++++++++++++--
tinyagentos/routes/projects.py | 54 +++++++++++++++++++++++++++++++++++
3 files changed, 86 insertions(+), 3 deletions(-)
Summary by Gitar
ProjectConflictexception inProjectStoreThis will update automatically on new commits.
Summary by CodeRabbit
New Features
Bug Fixes