WIP DONOT MERGE : Feature main rebase#20
Conversation
|
Warning Review limit reached
More reviews will be available in 46 minutes and 4 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds tasking project, task, and audit APIs with new schemas, migrations, docs, and tests. CI triggers, tagging, ignore rules, editor settings, and dependency metadata are also updated. ChangesTasking MVP rollout
Sequence Diagram(s)sequenceDiagram
participant Client
participant submit_task
participant TaskingTaskRepository
participant TaskingLock
participant TaskingTask
participant TaskingFeedback
Client->>submit_task: POST /tasks/{task_number}/submit
submit_task->>TaskingTaskRepository: submit(...)
TaskingTaskRepository->>TaskingLock: verify active lock
TaskingTaskRepository->>TaskingTask: update status and last_mapper_id
TaskingTaskRepository->>TaskingFeedback: insert feedback when provided
TaskingTaskRepository->>TaskingLock: extend expiry or release lock
TaskingTaskRepository-->>submit_task: TaskResponse
submit_task-->>Client: response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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 |
susrisha
force-pushed
the
feature-main-rebase
branch
from
6a5c26e to
b3a0457
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
docs/requirements/project-roles-integration.md-15-19 (1)
15-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAllow project leads in the requirements text.
This section only grants user search and role management to workspace leads, but the repository gate also allows project-level leads to manage project roles. Leaving the doc narrower will push the UI/permission requirements out of sync with the implemented auth model.
🤖 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 `@docs/requirements/project-roles-integration.md` around lines 15 - 19, Update the requirements text to include project-level leads alongside workspace leads for user search and role assignment. In the “User Search” and “Role Assignment” sections, make sure the wording around who can manage project roles matches the implemented authorization model, so the doc reflects both lead types consistently.tests/integration/test_audit_flow.py-5-5 (1)
5-5: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRegister the
integrationmarker in pytest config.CI is already emitting
PytestUnknownMarkWarningfor this marker, so these tests are adding noise until the marker is declared inpytest.iniorpyproject.toml.🤖 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/integration/test_audit_flow.py` at line 5, The integration tests are using the pytest mark on pytestmark, but the marker is not registered, causing PytestUnknownMarkWarning in CI. Add the integration marker declaration to the shared pytest configuration (pytest.ini or pyproject.toml) so pytest recognizes it, and keep the test module’s pytestmark usage unchanged.Source: Pipeline failures
tests/integration/test_projects_flow.py-5-7 (1)
5-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRegister the
integrationpytest marker.
pyproject.tomldefinestool.pytest.ini_options, but nomarkerslist. Addintegrationthere so--strict-markerswon’t reject these tests.🤖 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/integration/test_projects_flow.py` around lines 5 - 7, The module-level pytestmark in test_projects_flow.py uses the integration marker, but that marker is not registered in the pytest configuration. Add integration to the markers list under tool.pytest.ini_options in pyproject.toml so --strict-markers accepts the tests marked by pytestmark and any other integration-marked tests.Source: Pipeline failures
tests/integration/test_projects_flow.py-125-132 (1)
125-132: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid
id(self)for per-test project names.
id(self)can be reused between test instances, so this fixture can intermittently hit the duplicate-name 409 path instead of creating a fresh project. Use a UUID or monotonic counter here.Suggested fix
+from uuid import uuid4 ... async def fresh_project(self, client, as_lead, seeded_workspace_id): r = await client.post( API.format(wid=seeded_workspace_id), - json={"name": f"AOI shapes {id(self)}"}, + json={"name": f"AOI shapes {uuid4()}"}, )🤖 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/integration/test_projects_flow.py` around lines 125 - 132, The fresh_project fixture in test_projects_flow uses id(self) to generate project names, which can be reused across test instances and cause intermittent duplicate-name conflicts. Update the fixture to use a unique per-test value such as a UUID or a monotonic counter when building the name in fresh_project, while keeping the existing client.post and 201 assertion flow unchanged.
🧹 Nitpick comments (1)
api/src/tasking/projects/repository.py (1)
378-408: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCollapse the completed-count lookup into the existing aggregate query.
list_projects()already batches total task counts, butdone_qadds another query per project. At the max page size, one list request can fan out into hundreds of extraCOUNT(*)queries.Suggested direction
- text( - "SELECT project_id, COUNT(*) FROM tasking_tasks " - "WHERE project_id = ANY(:ids) GROUP BY project_id" - ), + text( + "SELECT project_id, " + "COUNT(*) AS total_count, " + "SUM(CASE WHEN status = 'completed' THEN 1 ELSE 0 END) AS completed_count " + "FROM tasking_tasks " + "WHERE project_id = ANY(:ids) " + "GROUP BY project_id" + ),Then build both
task_countandpercent_completedfrom that single result set.🤖 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 `@api/src/tasking/projects/repository.py` around lines 378 - 408, In list_projects() in ProjectRepository, the completed-task lookup is still doing one COUNT(*) query per project via done_q, which defeats the existing batching. Fold completed counts into the same aggregate query used for counts by selecting both total tasks and completed tasks per project from tasking_tasks, then compute pct from that single result set when building ProjectListItem objects.
🤖 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 @.github/workflows/tag.yml:
- Around line 17-20: The tag workflow uses a mutable actions/checkout version in
a job with contents: write, so update the Checkout code step in tag.yml to
reference a specific commit SHA instead of v4. Keep the existing checkout
behavior the same, but pin the action in this workflow to reduce supply-chain
risk in the release-tagging job.
- Around line 35-42: The Force update tag step currently retags whatever commit
the workflow checked out, which can move env tags backward if an older run
finishes last. Update the tagging logic in the workflow step named Force update
tag so it points to the branch head for the target environment branch instead of
the local checkout commit; use the existing ENV_TAG handling and git tag/git
push flow, but resolve the remote branch tip before force-moving the tag.
In `@alembic_osm/versions/9221408912dd_add_user_role_table.py`:
- Around line 28-45: The `users` bootstrap in this migration is not safely
reversible because `upgrade()` may create a stub table, but `downgrade()` always
drops `auth_uid_unique` without knowing whether `users` was Rails-owned or newly
created. Update the migration around `upgrade()`/`downgrade()` to track whether
the stub `users` table was created here, and only reverse that exact work on
downgrade; otherwise leave an existing Rails-owned `users` table untouched. If
ownership cannot be tracked cleanly, move the fresh-database bootstrap logic out
of this reversible Alembic migration path.
In `@alembic_osm/versions/a1b2c3d4e5f6_tasking_mvp_schema.py`:
- Around line 77-95: The downgrade logic for the teams/team_user tables is
dropping shared tables unconditionally, even when this revision did not create
them. Update the `downgrade()` path in `tasking_mvp_schema` to mirror the
ownership checks used in `upgrade()`, so `op.drop_table`/related drops only run
when this revision actually created `teams` and `team_user`. Use the existing
`insp.has_table(...)` checks (and any revision-local ownership marker you add)
to gate the drop behavior for both tables.
In `@api/core/security.py`:
- Around line 125-134: The member-mapping loop in the security lookup is only
handling one identity shape, so deployments that return `id` and `display_name`
are skipped and later reported as missing users. Update the logic around the
user row parsing to accept both variants in the existing lookup path, using
`user_id`/`username` and falling back to `id`/`display_name` before building
`TdeiProjectGroupUser`. Keep the handling in the same section that appends to
`out` so auto-provisioning works for both upstream identity formats.
In `@api/src/tasking/audit/repository.py`:
- Around line 148-157: The audit filtering logic in the repository methods is
reading the wrong JSONB key for task audits. Update the predicates in the
relevant repository functions that build the `where` clause and params so they
query `details->>'taskNumber'` instead of `details->>'task_number'`, matching
the shape written by the task repository. Make the same fix in both affected
filter paths (including the task-level JSON fallback) so `task_number` filtering
matches the events produced by the task writers.
In `@api/src/tasking/projects/dtos.py`:
- Around line 60-70: ProjectUpdateRequest.name is missing the same blank/strip
validation used on the create DTO, so PATCH can accept whitespace-only names
that later become empty after TaskingProjectRepository.patch() trims them.
Update ProjectUpdateRequest to apply the existing blank-name validator on name
(using the same validator/helper as the create request) so PATCH rejects
whitespace-only input before persistence.
In `@api/src/tasking/projects/repository.py`:
- Around line 1033-1062: The last-LEAD guard is racing with the role mutation
because _lead_count is checked in a separate statement from the UPDATE/DELETE,
so concurrent demotions/removals can both pass and leave no LEADs. Fix this by
making the check and mutation atomic in the repository methods that touch
project roles (the UPDATE path here and the related delete paths noted in the
comment), using a transaction-safe row lock or equivalent serialization around
the project’s role rows before calling _lead_count and applying the change. Keep
the existing ProjectRoleType.LEAD validation, but ensure the read of lead count
and the write happen under the same lock/transaction so only one request can
demote/remove the last lead at a time.
- Around line 500-536: The project role seeding logic in the repository creation
flow allows a `role_assignments` upsert to set the creator to a non-LEAD role
before the creator-specific insert runs. Update the `repository.py` project
creation path so the creator’s auto-assigned LEAD role always wins, either by
skipping the creator from the `role_assignments` loop or by ensuring the creator
insert/upsert explicitly overwrites any existing role. Use the
`role_assignments` handling and the creator seed insert in the same code path to
keep the creator guaranteed as LEAD.
In `@api/src/tasking/tasks/repository.py`:
- Around line 596-639: Update the task listing query in the repository method
that builds TaskListResponse so locked_by_user_id is applied in the database
query before count, limit, and offset are calculated. Move the lock condition
into the same where clause used for total_q and rows, alongside the existing
status_filter and last_mapper_id handling, so pagination.total reflects only
matching locked tasks and pages are filled correctly. Keep the response mapping
in _to_task_response unchanged, but remove the Python-side filtering loop for
locked_by_user_id.
- Around line 483-587: The idempotency replay logic in the save path is still
subject to a concurrent retry race because the initial SELECT on
tasking_task_save_idempotency happens before the write path. Update the save
flow to make the idempotency lookup/write atomic in the repository method that
builds SaveTasksResponse: either lock the idempotency row/key before proceeding
or switch to an insert-first/ON CONFLICT style readback so only one request can
create the record and the loser reuses the stored response. Keep the existing
body_hash mismatch check and replayed=True behavior, but ensure the concurrent
loser returns the persisted response instead of falling through to task creation
or raising an insert conflict.
- Around line 152-180: The grid generation logic is silently truncating results
when the safety cap is hit, so update the cell-building path in the repository
tasking code to detect that condition and fail explicitly instead of returning a
partial list. In the AOI grid loop that appends to cells and checks cell_cap,
raise an error or return a clear failure indicator when the cap is reached, and
make sure the caller can distinguish “complete grid” from “truncated grid” so
incomplete task sets are never saved as successful.
In `@api/src/tasking/tasks/routes.py`:
- Around line 172-182: The get_task_geojson route currently bypasses the same
authorization and tenancy checks used by the other task endpoints. Update this
handler to enforce validate_token and assert_workspace_visible before calling
task_repo.get_task_geojson, matching the existing task route pattern so
/boundary.geojson cannot be fetched by guessing IDs. Keep the fix localized to
get_task_geojson and reuse the same dependency/guard flow already applied in the
neighboring task routes.
In `@docs/tasking-mvp/tasking-mvp.openapi.yaml`:
- Around line 363-367: The OpenAPI spec is missing documentation for the task
grid workflow, so generated clients will not include the POST
/workspaces/{workspace_id}/tasking/projects/{project_id}/tasks/grid endpoint.
Update the tasking section around the Tasks paths to add the /tasks/grid
operation alongside the existing /tasks/validate and /tasks/save routes,
matching the behavior exercised by tests/integration/test_tasks_flow.py. Ensure
the new path includes the correct request/response schema and placement within
the tasking API group so it is discoverable in generated clients.
- Around line 793-796: The OpenAPI doc currently references a non-existent
/assignable-users path, leaving the user-search flow undocumented. Add the
actual user-search contract to this spec (or define the missing path here) and
update the team-management/role-assignment section so it points to the
documented operation instead of an absent external endpoint.
- Around line 1523-1570: The OpenAPI schemas for ProjectCreateRequest,
ProjectRoleAssignment, and the related models use camelCase and stricter
required fields that do not match the tested API contract. Update the affected
schema definitions to use the same snake_case field names used by the
integration tests (for example review_required, role_assignments,
feature_collection, task_count, task_boundary_type, task_number,
reason_category, and pagination.page_size) and make osmChangesetId optional on
submit if the API accepts it that way. Align the referenced component schemas
and any request/response models in the affected sections so generated clients
serialize and validate the same payload shape as the server.
In `@tests/integration/conftest.py`:
- Around line 376-445: The auth fixtures in as_lead, as_contributor,
as_validator, and as_outsider all overwrite validate_token via _override_token,
so they cannot be used together in the same test without the last one winning.
Refactor these fixtures to separate user creation from request identity
selection, and update multi-actor tests like test_lists_task_events to use
extra_user_factory with explicit override_user calls instead of depending on
multiple auth fixtures at once.
In `@tests/integration/test_audit_flow.py`:
- Around line 44-64: The _open_project_with_tasks helper is missing the setup
needed before task saving and activation. Update this helper to match the
task-flow preconditions used in test_tasks_flow by including the required source
when calling the /tasks/save endpoint and by seeding the contributor/validator
assignments before calling activate on the project. Use the existing helper flow
and symbols like _open_project_with_tasks, _fc, TASK_A, and TASK_B to keep the
audit test aligned with the expected task lifecycle.
---
Minor comments:
In `@docs/requirements/project-roles-integration.md`:
- Around line 15-19: Update the requirements text to include project-level leads
alongside workspace leads for user search and role assignment. In the “User
Search” and “Role Assignment” sections, make sure the wording around who can
manage project roles matches the implemented authorization model, so the doc
reflects both lead types consistently.
In `@tests/integration/test_audit_flow.py`:
- Line 5: The integration tests are using the pytest mark on pytestmark, but the
marker is not registered, causing PytestUnknownMarkWarning in CI. Add the
integration marker declaration to the shared pytest configuration (pytest.ini or
pyproject.toml) so pytest recognizes it, and keep the test module’s pytestmark
usage unchanged.
In `@tests/integration/test_projects_flow.py`:
- Around line 5-7: The module-level pytestmark in test_projects_flow.py uses the
integration marker, but that marker is not registered in the pytest
configuration. Add integration to the markers list under tool.pytest.ini_options
in pyproject.toml so --strict-markers accepts the tests marked by pytestmark and
any other integration-marked tests.
- Around line 125-132: The fresh_project fixture in test_projects_flow uses
id(self) to generate project names, which can be reused across test instances
and cause intermittent duplicate-name conflicts. Update the fixture to use a
unique per-test value such as a UUID or a monotonic counter when building the
name in fresh_project, while keeping the existing client.post and 201 assertion
flow unchanged.
---
Nitpick comments:
In `@api/src/tasking/projects/repository.py`:
- Around line 378-408: In list_projects() in ProjectRepository, the
completed-task lookup is still doing one COUNT(*) query per project via done_q,
which defeats the existing batching. Fold completed counts into the same
aggregate query used for counts by selecting both total tasks and completed
tasks per project from tasking_tasks, then compute pct from that single result
set when building ProjectListItem objects.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75c7ed33-7154-45a1-851a-1b2518d16a0d
⛔ Files ignored due to path filters (2)
docs/db-schema/db-schema.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
.github/workflows/ci.yml.github/workflows/tag.yml.gitignore.vscode/extensions.jsonalembic_osm/versions/9221408912dd_add_user_role_table.pyalembic_osm/versions/a1b2c3d4e5f6_tasking_mvp_schema.pyalembic_task/versions/add6266277c7_.pyalembic_task/versions/c5121cbba124_initial_task_schema.pyapi/core/security.pyapi/main.pyapi/src/tasking/__init__.pyapi/src/tasking/audit/__init__.pyapi/src/tasking/audit/dtos.pyapi/src/tasking/audit/repository.pyapi/src/tasking/audit/routes.pyapi/src/tasking/audit/schemas.pyapi/src/tasking/projects/__init__.pyapi/src/tasking/projects/dtos.pyapi/src/tasking/projects/repository.pyapi/src/tasking/projects/routes.pyapi/src/tasking/projects/schemas.pyapi/src/tasking/tasks/__init__.pyapi/src/tasking/tasks/dtos.pyapi/src/tasking/tasks/repository.pyapi/src/tasking/tasks/routes.pyapi/src/tasking/tasks/schemas.pydocs/requirements/project-roles-integration.mddocs/tasking-mvp/tasking-mvp.openapi.jsondocs/tasking-mvp/tasking-mvp.openapi.yamlpyproject.tomltests/conftest.pytests/integration/__init__.pytests/integration/conftest.pytests/integration/test_audit_flow.pytests/integration/test_projects_flow.pytests/integration/test_tasks_flow.pytests/unit/__init__.pytests/unit/conftest.pytests/unit/test_aoi_normalisation.pytests/unit/test_dtos_validation.pytests/unit/test_project_routes.pytests/unit/test_user_info_gates.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
docs/requirements/project-roles-integration.md-15-19 (1)
15-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAllow project leads in the requirements text.
This section only grants user search and role management to workspace leads, but the repository gate also allows project-level leads to manage project roles. Leaving the doc narrower will push the UI/permission requirements out of sync with the implemented auth model.
🤖 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 `@docs/requirements/project-roles-integration.md` around lines 15 - 19, Update the requirements text to include project-level leads alongside workspace leads for user search and role assignment. In the “User Search” and “Role Assignment” sections, make sure the wording around who can manage project roles matches the implemented authorization model, so the doc reflects both lead types consistently.tests/integration/test_audit_flow.py-5-5 (1)
5-5: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRegister the
integrationmarker in pytest config.CI is already emitting
PytestUnknownMarkWarningfor this marker, so these tests are adding noise until the marker is declared inpytest.iniorpyproject.toml.🤖 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/integration/test_audit_flow.py` at line 5, The integration tests are using the pytest mark on pytestmark, but the marker is not registered, causing PytestUnknownMarkWarning in CI. Add the integration marker declaration to the shared pytest configuration (pytest.ini or pyproject.toml) so pytest recognizes it, and keep the test module’s pytestmark usage unchanged.Source: Pipeline failures
tests/integration/test_projects_flow.py-5-7 (1)
5-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRegister the
integrationpytest marker.
pyproject.tomldefinestool.pytest.ini_options, but nomarkerslist. Addintegrationthere so--strict-markerswon’t reject these tests.🤖 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/integration/test_projects_flow.py` around lines 5 - 7, The module-level pytestmark in test_projects_flow.py uses the integration marker, but that marker is not registered in the pytest configuration. Add integration to the markers list under tool.pytest.ini_options in pyproject.toml so --strict-markers accepts the tests marked by pytestmark and any other integration-marked tests.Source: Pipeline failures
tests/integration/test_projects_flow.py-125-132 (1)
125-132: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid
id(self)for per-test project names.
id(self)can be reused between test instances, so this fixture can intermittently hit the duplicate-name 409 path instead of creating a fresh project. Use a UUID or monotonic counter here.Suggested fix
+from uuid import uuid4 ... async def fresh_project(self, client, as_lead, seeded_workspace_id): r = await client.post( API.format(wid=seeded_workspace_id), - json={"name": f"AOI shapes {id(self)}"}, + json={"name": f"AOI shapes {uuid4()}"}, )🤖 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/integration/test_projects_flow.py` around lines 125 - 132, The fresh_project fixture in test_projects_flow uses id(self) to generate project names, which can be reused across test instances and cause intermittent duplicate-name conflicts. Update the fixture to use a unique per-test value such as a UUID or a monotonic counter when building the name in fresh_project, while keeping the existing client.post and 201 assertion flow unchanged.
🧹 Nitpick comments (1)
api/src/tasking/projects/repository.py (1)
378-408: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCollapse the completed-count lookup into the existing aggregate query.
list_projects()already batches total task counts, butdone_qadds another query per project. At the max page size, one list request can fan out into hundreds of extraCOUNT(*)queries.Suggested direction
- text( - "SELECT project_id, COUNT(*) FROM tasking_tasks " - "WHERE project_id = ANY(:ids) GROUP BY project_id" - ), + text( + "SELECT project_id, " + "COUNT(*) AS total_count, " + "SUM(CASE WHEN status = 'completed' THEN 1 ELSE 0 END) AS completed_count " + "FROM tasking_tasks " + "WHERE project_id = ANY(:ids) " + "GROUP BY project_id" + ),Then build both
task_countandpercent_completedfrom that single result set.🤖 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 `@api/src/tasking/projects/repository.py` around lines 378 - 408, In list_projects() in ProjectRepository, the completed-task lookup is still doing one COUNT(*) query per project via done_q, which defeats the existing batching. Fold completed counts into the same aggregate query used for counts by selecting both total tasks and completed tasks per project from tasking_tasks, then compute pct from that single result set when building ProjectListItem objects.
🤖 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 @.github/workflows/tag.yml:
- Around line 17-20: The tag workflow uses a mutable actions/checkout version in
a job with contents: write, so update the Checkout code step in tag.yml to
reference a specific commit SHA instead of v4. Keep the existing checkout
behavior the same, but pin the action in this workflow to reduce supply-chain
risk in the release-tagging job.
- Around line 35-42: The Force update tag step currently retags whatever commit
the workflow checked out, which can move env tags backward if an older run
finishes last. Update the tagging logic in the workflow step named Force update
tag so it points to the branch head for the target environment branch instead of
the local checkout commit; use the existing ENV_TAG handling and git tag/git
push flow, but resolve the remote branch tip before force-moving the tag.
In `@alembic_osm/versions/9221408912dd_add_user_role_table.py`:
- Around line 28-45: The `users` bootstrap in this migration is not safely
reversible because `upgrade()` may create a stub table, but `downgrade()` always
drops `auth_uid_unique` without knowing whether `users` was Rails-owned or newly
created. Update the migration around `upgrade()`/`downgrade()` to track whether
the stub `users` table was created here, and only reverse that exact work on
downgrade; otherwise leave an existing Rails-owned `users` table untouched. If
ownership cannot be tracked cleanly, move the fresh-database bootstrap logic out
of this reversible Alembic migration path.
In `@alembic_osm/versions/a1b2c3d4e5f6_tasking_mvp_schema.py`:
- Around line 77-95: The downgrade logic for the teams/team_user tables is
dropping shared tables unconditionally, even when this revision did not create
them. Update the `downgrade()` path in `tasking_mvp_schema` to mirror the
ownership checks used in `upgrade()`, so `op.drop_table`/related drops only run
when this revision actually created `teams` and `team_user`. Use the existing
`insp.has_table(...)` checks (and any revision-local ownership marker you add)
to gate the drop behavior for both tables.
In `@api/core/security.py`:
- Around line 125-134: The member-mapping loop in the security lookup is only
handling one identity shape, so deployments that return `id` and `display_name`
are skipped and later reported as missing users. Update the logic around the
user row parsing to accept both variants in the existing lookup path, using
`user_id`/`username` and falling back to `id`/`display_name` before building
`TdeiProjectGroupUser`. Keep the handling in the same section that appends to
`out` so auto-provisioning works for both upstream identity formats.
In `@api/src/tasking/audit/repository.py`:
- Around line 148-157: The audit filtering logic in the repository methods is
reading the wrong JSONB key for task audits. Update the predicates in the
relevant repository functions that build the `where` clause and params so they
query `details->>'taskNumber'` instead of `details->>'task_number'`, matching
the shape written by the task repository. Make the same fix in both affected
filter paths (including the task-level JSON fallback) so `task_number` filtering
matches the events produced by the task writers.
In `@api/src/tasking/projects/dtos.py`:
- Around line 60-70: ProjectUpdateRequest.name is missing the same blank/strip
validation used on the create DTO, so PATCH can accept whitespace-only names
that later become empty after TaskingProjectRepository.patch() trims them.
Update ProjectUpdateRequest to apply the existing blank-name validator on name
(using the same validator/helper as the create request) so PATCH rejects
whitespace-only input before persistence.
In `@api/src/tasking/projects/repository.py`:
- Around line 1033-1062: The last-LEAD guard is racing with the role mutation
because _lead_count is checked in a separate statement from the UPDATE/DELETE,
so concurrent demotions/removals can both pass and leave no LEADs. Fix this by
making the check and mutation atomic in the repository methods that touch
project roles (the UPDATE path here and the related delete paths noted in the
comment), using a transaction-safe row lock or equivalent serialization around
the project’s role rows before calling _lead_count and applying the change. Keep
the existing ProjectRoleType.LEAD validation, but ensure the read of lead count
and the write happen under the same lock/transaction so only one request can
demote/remove the last lead at a time.
- Around line 500-536: The project role seeding logic in the repository creation
flow allows a `role_assignments` upsert to set the creator to a non-LEAD role
before the creator-specific insert runs. Update the `repository.py` project
creation path so the creator’s auto-assigned LEAD role always wins, either by
skipping the creator from the `role_assignments` loop or by ensuring the creator
insert/upsert explicitly overwrites any existing role. Use the
`role_assignments` handling and the creator seed insert in the same code path to
keep the creator guaranteed as LEAD.
In `@api/src/tasking/tasks/repository.py`:
- Around line 596-639: Update the task listing query in the repository method
that builds TaskListResponse so locked_by_user_id is applied in the database
query before count, limit, and offset are calculated. Move the lock condition
into the same where clause used for total_q and rows, alongside the existing
status_filter and last_mapper_id handling, so pagination.total reflects only
matching locked tasks and pages are filled correctly. Keep the response mapping
in _to_task_response unchanged, but remove the Python-side filtering loop for
locked_by_user_id.
- Around line 483-587: The idempotency replay logic in the save path is still
subject to a concurrent retry race because the initial SELECT on
tasking_task_save_idempotency happens before the write path. Update the save
flow to make the idempotency lookup/write atomic in the repository method that
builds SaveTasksResponse: either lock the idempotency row/key before proceeding
or switch to an insert-first/ON CONFLICT style readback so only one request can
create the record and the loser reuses the stored response. Keep the existing
body_hash mismatch check and replayed=True behavior, but ensure the concurrent
loser returns the persisted response instead of falling through to task creation
or raising an insert conflict.
- Around line 152-180: The grid generation logic is silently truncating results
when the safety cap is hit, so update the cell-building path in the repository
tasking code to detect that condition and fail explicitly instead of returning a
partial list. In the AOI grid loop that appends to cells and checks cell_cap,
raise an error or return a clear failure indicator when the cap is reached, and
make sure the caller can distinguish “complete grid” from “truncated grid” so
incomplete task sets are never saved as successful.
In `@api/src/tasking/tasks/routes.py`:
- Around line 172-182: The get_task_geojson route currently bypasses the same
authorization and tenancy checks used by the other task endpoints. Update this
handler to enforce validate_token and assert_workspace_visible before calling
task_repo.get_task_geojson, matching the existing task route pattern so
/boundary.geojson cannot be fetched by guessing IDs. Keep the fix localized to
get_task_geojson and reuse the same dependency/guard flow already applied in the
neighboring task routes.
In `@docs/tasking-mvp/tasking-mvp.openapi.yaml`:
- Around line 363-367: The OpenAPI spec is missing documentation for the task
grid workflow, so generated clients will not include the POST
/workspaces/{workspace_id}/tasking/projects/{project_id}/tasks/grid endpoint.
Update the tasking section around the Tasks paths to add the /tasks/grid
operation alongside the existing /tasks/validate and /tasks/save routes,
matching the behavior exercised by tests/integration/test_tasks_flow.py. Ensure
the new path includes the correct request/response schema and placement within
the tasking API group so it is discoverable in generated clients.
- Around line 793-796: The OpenAPI doc currently references a non-existent
/assignable-users path, leaving the user-search flow undocumented. Add the
actual user-search contract to this spec (or define the missing path here) and
update the team-management/role-assignment section so it points to the
documented operation instead of an absent external endpoint.
- Around line 1523-1570: The OpenAPI schemas for ProjectCreateRequest,
ProjectRoleAssignment, and the related models use camelCase and stricter
required fields that do not match the tested API contract. Update the affected
schema definitions to use the same snake_case field names used by the
integration tests (for example review_required, role_assignments,
feature_collection, task_count, task_boundary_type, task_number,
reason_category, and pagination.page_size) and make osmChangesetId optional on
submit if the API accepts it that way. Align the referenced component schemas
and any request/response models in the affected sections so generated clients
serialize and validate the same payload shape as the server.
In `@tests/integration/conftest.py`:
- Around line 376-445: The auth fixtures in as_lead, as_contributor,
as_validator, and as_outsider all overwrite validate_token via _override_token,
so they cannot be used together in the same test without the last one winning.
Refactor these fixtures to separate user creation from request identity
selection, and update multi-actor tests like test_lists_task_events to use
extra_user_factory with explicit override_user calls instead of depending on
multiple auth fixtures at once.
In `@tests/integration/test_audit_flow.py`:
- Around line 44-64: The _open_project_with_tasks helper is missing the setup
needed before task saving and activation. Update this helper to match the
task-flow preconditions used in test_tasks_flow by including the required source
when calling the /tasks/save endpoint and by seeding the contributor/validator
assignments before calling activate on the project. Use the existing helper flow
and symbols like _open_project_with_tasks, _fc, TASK_A, and TASK_B to keep the
audit test aligned with the expected task lifecycle.
---
Minor comments:
In `@docs/requirements/project-roles-integration.md`:
- Around line 15-19: Update the requirements text to include project-level leads
alongside workspace leads for user search and role assignment. In the “User
Search” and “Role Assignment” sections, make sure the wording around who can
manage project roles matches the implemented authorization model, so the doc
reflects both lead types consistently.
In `@tests/integration/test_audit_flow.py`:
- Line 5: The integration tests are using the pytest mark on pytestmark, but the
marker is not registered, causing PytestUnknownMarkWarning in CI. Add the
integration marker declaration to the shared pytest configuration (pytest.ini or
pyproject.toml) so pytest recognizes it, and keep the test module’s pytestmark
usage unchanged.
In `@tests/integration/test_projects_flow.py`:
- Around line 5-7: The module-level pytestmark in test_projects_flow.py uses the
integration marker, but that marker is not registered in the pytest
configuration. Add integration to the markers list under tool.pytest.ini_options
in pyproject.toml so --strict-markers accepts the tests marked by pytestmark and
any other integration-marked tests.
- Around line 125-132: The fresh_project fixture in test_projects_flow uses
id(self) to generate project names, which can be reused across test instances
and cause intermittent duplicate-name conflicts. Update the fixture to use a
unique per-test value such as a UUID or a monotonic counter when building the
name in fresh_project, while keeping the existing client.post and 201 assertion
flow unchanged.
---
Nitpick comments:
In `@api/src/tasking/projects/repository.py`:
- Around line 378-408: In list_projects() in ProjectRepository, the
completed-task lookup is still doing one COUNT(*) query per project via done_q,
which defeats the existing batching. Fold completed counts into the same
aggregate query used for counts by selecting both total tasks and completed
tasks per project from tasking_tasks, then compute pct from that single result
set when building ProjectListItem objects.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75c7ed33-7154-45a1-851a-1b2518d16a0d
⛔ Files ignored due to path filters (2)
docs/db-schema/db-schema.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
.github/workflows/ci.yml.github/workflows/tag.yml.gitignore.vscode/extensions.jsonalembic_osm/versions/9221408912dd_add_user_role_table.pyalembic_osm/versions/a1b2c3d4e5f6_tasking_mvp_schema.pyalembic_task/versions/add6266277c7_.pyalembic_task/versions/c5121cbba124_initial_task_schema.pyapi/core/security.pyapi/main.pyapi/src/tasking/__init__.pyapi/src/tasking/audit/__init__.pyapi/src/tasking/audit/dtos.pyapi/src/tasking/audit/repository.pyapi/src/tasking/audit/routes.pyapi/src/tasking/audit/schemas.pyapi/src/tasking/projects/__init__.pyapi/src/tasking/projects/dtos.pyapi/src/tasking/projects/repository.pyapi/src/tasking/projects/routes.pyapi/src/tasking/projects/schemas.pyapi/src/tasking/tasks/__init__.pyapi/src/tasking/tasks/dtos.pyapi/src/tasking/tasks/repository.pyapi/src/tasking/tasks/routes.pyapi/src/tasking/tasks/schemas.pydocs/requirements/project-roles-integration.mddocs/tasking-mvp/tasking-mvp.openapi.jsondocs/tasking-mvp/tasking-mvp.openapi.yamlpyproject.tomltests/conftest.pytests/integration/__init__.pytests/integration/conftest.pytests/integration/test_audit_flow.pytests/integration/test_projects_flow.pytests/integration/test_tasks_flow.pytests/unit/__init__.pytests/unit/conftest.pytests/unit/test_aoi_normalisation.pytests/unit/test_dtos_validation.pytests/unit/test_project_routes.pytests/unit/test_user_info_gates.py
🛑 Comments failed to post (18)
.github/workflows/tag.yml (2)
17-20: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,120p' .github/workflows/tag.yml rg -n "uses:\s*actions/checkout@" .github/workflowsRepository: TaskarCenterAtUW/workspaces-backend
Length of output: 1824
Pin
actions/checkoutto a commit SHA. In.github/workflows/tag.yml, this job hascontents: write, so the mutablev4ref adds avoidable supply-chain risk in a release-tagging workflow.🧰 Tools
🪛 zizmor (1.26.1)
[warning] 17-20: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 18-18: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/tag.yml around lines 17 - 20, The tag workflow uses a mutable actions/checkout version in a job with contents: write, so update the Checkout code step in tag.yml to reference a specific commit SHA instead of v4. Keep the existing checkout behavior the same, but pin the action in this workflow to reduce supply-chain risk in the release-tagging job.Source: Linters/SAST tools
35-42: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Tag the branch head, not the event checkout.
This step force-moves
dev/stage/prodto whatever commit this workflow run checked out. If two PRs merge into the same branch close together, a slower older run can finish last and move the env tag backward.Suggested hardening
jobs: update-tag: + concurrency: + group: update-tag-${{ github.event.pull_request.base.ref }} + cancel-in-progress: false if: github.event.pull_request.merged == true runs-on: ubuntu-latest @@ - name: Force update tag if: ${{ env.ENV_TAG != '' }} + env: + TARGET_BRANCH: ${{ github.event.pull_request.base.ref }} run: | git config user.name "github-actions[bot]" git config user.email "github-actions[bot]`@users.noreply.github.com`" - echo "Targeting tag: ${{ env.ENV_TAG }}" - git tag -f $ENV_TAG - git push origin $ENV_TAG --force + git fetch origin "refs/heads/$TARGET_BRANCH:refs/remotes/origin/$TARGET_BRANCH" + echo "Targeting tag: $ENV_TAG -> origin/$TARGET_BRANCH" + git tag -f "$ENV_TAG" "origin/$TARGET_BRANCH" + git push origin "refs/tags/$ENV_TAG" --force📝 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.- name: Force update tag if: ${{ env.ENV_TAG != '' }} env: TARGET_BRANCH: ${{ github.event.pull_request.base.ref }} run: | git config user.name "github-actions[bot]" git config user.email "github-actions[bot]`@users.noreply.github.com`" git fetch origin "refs/heads/$TARGET_BRANCH:refs/remotes/origin/$TARGET_BRANCH" echo "Targeting tag: $ENV_TAG -> origin/$TARGET_BRANCH" git tag -f "$ENV_TAG" "origin/$TARGET_BRANCH" git push origin "refs/tags/$ENV_TAG" --force🧰 Tools
🪛 zizmor (1.26.1)
[warning] 40-40: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/tag.yml around lines 35 - 42, The Force update tag step currently retags whatever commit the workflow checked out, which can move env tags backward if an older run finishes last. Update the tagging logic in the workflow step named Force update tag so it points to the branch head for the target environment branch instead of the local checkout commit; use the existing ENV_TAG handling and git tag/git push flow, but resolve the remote branch tip before force-moving the tag.alembic_osm/versions/9221408912dd_add_user_role_table.py (1)
28-45: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
This migration cannot safely reverse its
usersbootstrap.
upgrade()conditionally creates/patchesusers, butdowngrade()always dropsauth_uid_uniqueand never removes a stubuserstable it may have created. That means a downgrade will either mutate a Rails-owned table or leave behind schema that did not exist before the upgrade. Please track ownership explicitly or move the fresh-DB bootstrap out of this reversible migration path.Also applies to: 87-89
🤖 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 `@alembic_osm/versions/9221408912dd_add_user_role_table.py` around lines 28 - 45, The `users` bootstrap in this migration is not safely reversible because `upgrade()` may create a stub table, but `downgrade()` always drops `auth_uid_unique` without knowing whether `users` was Rails-owned or newly created. Update the migration around `upgrade()`/`downgrade()` to track whether the stub `users` table was created here, and only reverse that exact work on downgrade; otherwise leave an existing Rails-owned `users` table untouched. If ownership cannot be tracked cleanly, move the fresh-database bootstrap logic out of this reversible Alembic migration path.alembic_osm/versions/a1b2c3d4e5f6_tasking_mvp_schema.py (1)
77-95: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Don't drop
teams/team_userunless this revision created them.The
has_table(...)guards makeupgrade()tolerant of shared installs, butdowngrade()drops these tables whenever they exist. On any database where they predated this revision, rolling back here will delete unrelated membership data. The downgrade needs the same ownership awareness as the upgrade.Also applies to: 526-540
🤖 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 `@alembic_osm/versions/a1b2c3d4e5f6_tasking_mvp_schema.py` around lines 77 - 95, The downgrade logic for the teams/team_user tables is dropping shared tables unconditionally, even when this revision did not create them. Update the `downgrade()` path in `tasking_mvp_schema` to mirror the ownership checks used in `upgrade()`, so `op.drop_table`/related drops only run when this revision actually created `teams` and `team_user`. Use the existing `insp.has_table(...)` checks (and any revision-local ownership marker you add) to gate the drop behavior for both tables.api/core/security.py (1)
125-134: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Read both upstream identity variants here.
The docstring says deployments can return
id/display_name, but this loop only readsuser_id/username. On those deployments, valid members get skipped and project creation falls through to a falsemissing_user_idserror instead of auto-provisioning them.Suggested fix
for row in page: - uid = row.get("user_id") + uid = row.get("user_id") or row.get("id") if not uid: continue out.append( TdeiProjectGroupUser( auth_uid=str(uid), email=row.get("email"), - display_name=(row.get("username")), + display_name=row.get("username") or row.get("display_name"), ) )📝 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.for row in page: uid = row.get("user_id") or row.get("id") if not uid: continue out.append( TdeiProjectGroupUser( auth_uid=str(uid), email=row.get("email"), display_name=row.get("username") or row.get("display_name"), )🤖 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 `@api/core/security.py` around lines 125 - 134, The member-mapping loop in the security lookup is only handling one identity shape, so deployments that return `id` and `display_name` are skipped and later reported as missing users. Update the logic around the user row parsing to accept both variants in the existing lookup path, using `user_id`/`username` and falling back to `id`/`display_name` before building `TdeiProjectGroupUser`. Keep the handling in the same section that appends to `out` so auto-provisioning works for both upstream identity formats.api/src/tasking/audit/repository.py (1)
148-157: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Audit task-number filters are reading the wrong JSON key.
These predicates use
details->>'task_number', but the task audit writers storetaskNumber. As a result, project audit filtering bytask_numberwill never match task events written byapi/src/tasking/tasks/repository.py, and the task-level JSON fallback misses those rows too.Also applies to: 223-227
🤖 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 `@api/src/tasking/audit/repository.py` around lines 148 - 157, The audit filtering logic in the repository methods is reading the wrong JSONB key for task audits. Update the predicates in the relevant repository functions that build the `where` clause and params so they query `details->>'taskNumber'` instead of `details->>'task_number'`, matching the shape written by the task repository. Make the same fix in both affected filter paths (including the task-level JSON fallback) so `task_number` filtering matches the events produced by the task writers.api/src/tasking/projects/dtos.py (1)
60-70: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Apply the blank-name validator to PATCH too.
ProjectUpdateRequest.namemisses the strip/blank check used on create.TaskingProjectRepository.patch()trims before persisting, so" "becomes""and a blank project name can be stored.Suggested fix
class ProjectUpdateRequest(WireModel): @@ instructions: Optional[str] = PydField(default=None, max_length=10_000) lock_timeout_hours: Optional[int] = PydField(default=None, ge=1, le=720) review_required: Optional[bool] = None + + `@field_validator`("name") + `@classmethod` + def _name_not_blank(cls, v: str | None) -> str | None: + if v is None: + return None + v = v.strip() + if not v: + raise ValueError("name cannot be blank") + return v📝 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.class ProjectUpdateRequest(WireModel): """Body for `PATCH /workspaces/{wid}/tasking/projects/{pid}`. All fields optional; per-status mutability is enforced in the repository layer. """ name: Optional[str] = PydField(default=None, min_length=1, max_length=255) instructions: Optional[str] = PydField(default=None, max_length=10_000) lock_timeout_hours: Optional[int] = PydField(default=None, ge=1, le=720) review_required: Optional[bool] = None `@field_validator`("name") `@classmethod` def _name_not_blank(cls, v: str | None) -> str | None: if v is None: return None v = v.strip() if not v: raise ValueError("name cannot be blank") return v🤖 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 `@api/src/tasking/projects/dtos.py` around lines 60 - 70, ProjectUpdateRequest.name is missing the same blank/strip validation used on the create DTO, so PATCH can accept whitespace-only names that later become empty after TaskingProjectRepository.patch() trims them. Update ProjectUpdateRequest to apply the existing blank-name validator on name (using the same validator/helper as the create request) so PATCH rejects whitespace-only input before persistence.api/src/tasking/projects/repository.py (2)
500-536: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't let
role_assignmentsoverride the creator's auto-LEAD.If the request includes the creator in
role_assignmentsasvalidator/contributor, that upsert runs first and the later creator seed doesON CONFLICT DO NOTHING. The creator is then not guaranteed to be a project LEAD.Suggested fix
await self.session.execute( text( "INSERT INTO tasking_project_roles " "(project_id, user_auth_uid, role) " "VALUES (:pid, :uid, 'lead') " - "ON CONFLICT DO NOTHING" + "ON CONFLICT (project_id, user_auth_uid) DO UPDATE " + "SET role = 'lead', updated_at = NOW()" ), { "pid": project.id, "uid": str(current_user.user_uuid), }, )📝 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.# Seed project-level role overrides. if body.role_assignments: from sqlalchemy import text for ra in body.role_assignments: await self.session.execute( text( "INSERT INTO tasking_project_roles " "(project_id, user_auth_uid, role) " "VALUES (:pid, :uid, :role) " "ON CONFLICT (project_id, user_auth_uid) " "DO UPDATE SET role = EXCLUDED.role, " " updated_at = NOW()" ), { "pid": project.id, "uid": str(ra.user_id), "role": ra.role, }, ) # Creator is auto-assigned the LEAD role on the project, # mirroring the workspace-creator auto-LEAD convention. from sqlalchemy import text await self.session.execute( text( "INSERT INTO tasking_project_roles " "(project_id, user_auth_uid, role) " "VALUES (:pid, :uid, 'lead') " "ON CONFLICT (project_id, user_auth_uid) DO UPDATE " "SET role = 'lead', updated_at = NOW()" ), { "pid": project.id, "uid": str(current_user.user_uuid), }, )🤖 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 `@api/src/tasking/projects/repository.py` around lines 500 - 536, The project role seeding logic in the repository creation flow allows a `role_assignments` upsert to set the creator to a non-LEAD role before the creator-specific insert runs. Update the `repository.py` project creation path so the creator’s auto-assigned LEAD role always wins, either by skipping the creator from the `role_assignments` loop or by ensuring the creator insert/upsert explicitly overwrites any existing role. Use the `role_assignments` handling and the creator seed insert in the same code path to keep the creator guaranteed as LEAD.
1033-1062: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize the last-LEAD check with the mutation.
These paths read the lead count and then update/delete in separate statements. Two concurrent demotions/removals can both observe
lead_count > 1and commit, leaving the project with zero LEADs despite the guard.Also applies to: 1161-1215, 1232-1253
🤖 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 `@api/src/tasking/projects/repository.py` around lines 1033 - 1062, The last-LEAD guard is racing with the role mutation because _lead_count is checked in a separate statement from the UPDATE/DELETE, so concurrent demotions/removals can both pass and leave no LEADs. Fix this by making the check and mutation atomic in the repository methods that touch project roles (the UPDATE path here and the related delete paths noted in the comment), using a transaction-safe row lock or equivalent serialization around the project’s role rows before calling _lead_count and applying the change. Keep the existing ProjectRoleType.LEAD validation, but ensure the read of lead count and the write happen under the same lock/transaction so only one request can demote/remove the last lead at a time.api/src/tasking/tasks/repository.py (3)
152-180: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't return a truncated grid as a successful result.
When
cell_capis hit, this returns the first 50k cells with no error or flag. Clients can then save an incomplete task set while thinking the AOI was fully covered.Suggested fix
if isinstance(piece, ShapelyPolygon) and piece.area > 0: cells.append(piece) if len(cells) >= cell_cap: - return cells + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=( + "Grid would exceed the maximum supported " + "cell count; reduce the AOI or increase " + "the grid size." + ), + )📝 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.# Safety cap for accidental large-AOI + small-cell combinations. cell_cap = 50_000 y = miny while y < maxy: x = minx while x < maxx: cell = ShapelyPolygon( [ (x, y), (x + lon_step, y), (x + lon_step, y + lat_step), (x, y + lat_step), (x, y), ] ) if cell.intersects(aoi): clipped = cell.intersection(aoi) if not clipped.is_empty and clipped.area > 0: # `intersection` can return a Polygon, MultiPolygon, # or GeometryCollection; retain polygon pieces only. geoms = ( list(clipped.geoms) if hasattr(clipped, "geoms") else [clipped] ) for piece in geoms: if isinstance(piece, ShapelyPolygon) and piece.area > 0: cells.append(piece) if len(cells) >= cell_cap: raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=( "Grid would exceed the maximum supported " "cell count; reduce the AOI or increase " "the grid size." ), )🤖 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 `@api/src/tasking/tasks/repository.py` around lines 152 - 180, The grid generation logic is silently truncating results when the safety cap is hit, so update the cell-building path in the repository tasking code to detect that condition and fail explicitly instead of returning a partial list. In the AOI grid loop that appends to cells and checks cell_cap, raise an error or return a clear failure indicator when the cap is reached, and make sure the caller can distinguish “complete grid” from “truncated grid” so incomplete task sets are never saved as successful.
483-587: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The idempotency replay path still races under concurrent retries.
Two identical
saverequests can both miss the initial lookup and enter the write path. The loser then hits a later insert/conflict instead of replaying the stored response, so a legitimate retry can surface as an error.🧰 Tools
🪛 ast-grep (0.44.0)
[info] 581-581: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.(use-jsonify)
🤖 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 `@api/src/tasking/tasks/repository.py` around lines 483 - 587, The idempotency replay logic in the save path is still subject to a concurrent retry race because the initial SELECT on tasking_task_save_idempotency happens before the write path. Update the save flow to make the idempotency lookup/write atomic in the repository method that builds SaveTasksResponse: either lock the idempotency row/key before proceeding or switch to an insert-first/ON CONFLICT style readback so only one request can create the record and the loser reuses the stored response. Keep the existing body_hash mismatch check and replayed=True behavior, but ensure the concurrent loser returns the persisted response instead of falling through to task creation or raising an insert conflict.
596-639: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Apply
locked_by_user_idbefore counting and paginating.
total,LIMIT, andOFFSETare computed before the lock filter, then the filter runs in Python. That makespagination.totalwrong and can produce empty/short pages even when matching locked tasks exist later in the result set.🤖 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 `@api/src/tasking/tasks/repository.py` around lines 596 - 639, Update the task listing query in the repository method that builds TaskListResponse so locked_by_user_id is applied in the database query before count, limit, and offset are calculated. Move the lock condition into the same where clause used for total_q and rows, alongside the existing status_filter and last_mapper_id handling, so pagination.total reflects only matching locked tasks and pages are filled correctly. Keep the response mapping in _to_task_response unchanged, but remove the Python-side filtering loop for locked_by_user_id.api/src/tasking/tasks/routes.py (1)
172-182: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Protect
/boundary.geojsonwith the same auth and workspace gate as the other task routes.This endpoint skips
validate_tokenandassert_workspace_visible, even though the repository assumes the route layer already enforced tenancy. Right now, anyone who can guess IDs can fetch task geometry without authorization.Suggested fix
async def get_task_geojson( workspace_id: int, project_id: int, task_number: int, + current_user: UserInfo = Depends(validate_token), + workspace_repo: WorkspaceRepository = Depends(get_workspace_repo), task_repo: TaskingTaskRepository = Depends(get_task_repo), ): + await assert_workspace_visible(workspace_id, current_user, workspace_repo) return await task_repo.get_task_geojson(workspace_id, project_id, task_number)📝 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.`@router.get`( "/tasks/{task_number}/boundary.geojson", response_model=TaskBoundariesFeatureCollection, ) async def get_task_geojson( workspace_id: int, project_id: int, task_number: int, current_user: UserInfo = Depends(validate_token), workspace_repo: WorkspaceRepository = Depends(get_workspace_repo), task_repo: TaskingTaskRepository = Depends(get_task_repo), ): await assert_workspace_visible(workspace_id, current_user, workspace_repo) return await task_repo.get_task_geojson(workspace_id, project_id, task_number)🧰 Tools
🪛 Ruff (0.15.18)
[warning] 180-180: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
🤖 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 `@api/src/tasking/tasks/routes.py` around lines 172 - 182, The get_task_geojson route currently bypasses the same authorization and tenancy checks used by the other task endpoints. Update this handler to enforce validate_token and assert_workspace_visible before calling task_repo.get_task_geojson, matching the existing task route pattern so /boundary.geojson cannot be fetched by guessing IDs. Keep the fix localized to get_task_geojson and reuse the same dependency/guard flow already applied in the neighboring task routes.docs/tasking-mvp/tasking-mvp.openapi.yaml (3)
363-367: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Document the
/tasks/gridendpoint.
tests/integration/test_tasks_flow.pyexercisesPOST /workspaces/{workspace_id}/tasking/projects/{project_id}/tasks/grid, but this spec jumps straight from AOI to/tasks/validateand/tasks/save. Clients generated from this file will miss the grid workflow entirely.🤖 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 `@docs/tasking-mvp/tasking-mvp.openapi.yaml` around lines 363 - 367, The OpenAPI spec is missing documentation for the task grid workflow, so generated clients will not include the POST /workspaces/{workspace_id}/tasking/projects/{project_id}/tasks/grid endpoint. Update the tasking section around the Tasks paths to add the /tasks/grid operation alongside the existing /tasks/validate and /tasks/save routes, matching the behavior exercised by tests/integration/test_tasks_flow.py. Ensure the new path includes the correct request/response schema and placement within the tasking API group so it is discoverable in generated clients.
793-796: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add the user-search contract instead of referencing a missing path.
This section tells consumers to use
/assignable-users, anddocs/requirements/project-roles-integration.mdmakes user search a required capability, but there is no such path anywhere in this spec. That leaves the role-assignment flow undocumented.🤖 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 `@docs/tasking-mvp/tasking-mvp.openapi.yaml` around lines 793 - 796, The OpenAPI doc currently references a non-existent /assignable-users path, leaving the user-search flow undocumented. Add the actual user-search contract to this spec (or define the missing path here) and update the team-management/role-assignment section so it points to the documented operation instead of an absent external endpoint.
1523-1570: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The schema casing and required fields do not match the tested API.
The integration tests in this PR use snake_case payload/response fields such as
review_required,role_assignments,feature_collection,task_count,task_boundary_type,task_number,reason_category, andpagination.page_size, while these schemas publish camelCase equivalents and makeosmChangesetIdmandatory on/submit. One side of the contract is wrong; as written, generated clients will serialize the wrong JSON.Also applies to: 1599-1618, 1658-1688, 1692-1734, 1975-1981
🤖 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 `@docs/tasking-mvp/tasking-mvp.openapi.yaml` around lines 1523 - 1570, The OpenAPI schemas for ProjectCreateRequest, ProjectRoleAssignment, and the related models use camelCase and stricter required fields that do not match the tested API contract. Update the affected schema definitions to use the same snake_case field names used by the integration tests (for example review_required, role_assignments, feature_collection, task_count, task_boundary_type, task_number, reason_category, and pagination.page_size) and make osmChangesetId optional on submit if the API accepts it that way. Align the referenced component schemas and any request/response models in the affected sections so generated clients serialize and validate the same payload shape as the server.tests/integration/conftest.py (1)
376-445: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
These auth fixtures are mutually exclusive in a single test.
Each fixture eagerly overwrites
app.dependency_overrides[validate_token]during setup, so requestingas_leadandas_contributortogether leaves whichever fixture runs last as the active caller.tests/integration/test_audit_flow.pyalready depends on both intest_lists_task_events, so the actor is currently order-dependent. Please separate user creation from identity selection, or reuseextra_user_factoryplus explicitoverride_usercalls for multi-actor tests.🤖 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/integration/conftest.py` around lines 376 - 445, The auth fixtures in as_lead, as_contributor, as_validator, and as_outsider all overwrite validate_token via _override_token, so they cannot be used together in the same test without the last one winning. Refactor these fixtures to separate user creation from request identity selection, and update multi-actor tests like test_lists_task_events to use extra_user_factory with explicit override_user calls instead of depending on multiple auth fixtures at once.tests/integration/test_audit_flow.py (1)
44-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This helper misses the preconditions for
saveandactivate.It posts
/tasks/savewithout thesourcefield, then activates a project that only has the creator's auto-leadrole. The task-flow helper intests/integration/test_tasks_flow.pyexplicitly seeds contributor/validator assignments before activation for this reason. As written, this helper should fail before any audit assertions run.🤖 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/integration/test_audit_flow.py` around lines 44 - 64, The _open_project_with_tasks helper is missing the setup needed before task saving and activation. Update this helper to match the task-flow preconditions used in test_tasks_flow by including the required source when calling the /tasks/save endpoint and by seeding the contributor/validator assignments before calling activate on the project. Use the existing helper flow and symbols like _open_project_with_tasks, _fc, TASK_A, and TASK_B to keep the audit test aligned with the expected task lifecycle.
3 participants
WIP For rebasing with main. Donot merge
Summary
/api/v1/*routes..gitignore/VS Code config cleanup, and a new GitHub Actions workflow to auto-tag environments on merged PRs.shapelyas a dependency.