Repository navigation
WS-ARCH-001-02A: expose TASK submission context API - #314
Conversation
|
Warning Review limit reached
Next review available in: 19 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe PR adds a public TASK submission-context API, repository-owned locking and validation for initial and revision submissions, architecture-boundary checks, PostgreSQL state and concurrency tests, and updated implementation documentation. ChangesTASK submission-context API
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant TaskRepository
participant PostgreSQL
Caller->>TaskRepository: lock_submission_context(request)
TaskRepository->>PostgreSQL: Lock task, assignment, and latest submission
PostgreSQL-->>TaskRepository: Return locked records
TaskRepository->>TaskRepository: Validate lifecycle, ownership, and predecessor lineage
TaskRepository-->>Caller: Return context facts or unavailable error
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
backend/app/modules/tasks/api/submission_context.py (1)
20-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the accepted failure codes from the
TaskSubmissionContextFailurealias.Lines 21-24 duplicate the literal members declared at lines 11-14. If a future code is added to the alias only, the constructor rejects it at runtime while the type checker accepts it. Derive the set from the alias to keep one source of truth.
♻️ Proposed refactor
-from typing import Literal, Protocol +from typing import Literal, Protocol, get_args @@ class TaskSubmissionContextUnavailable(RuntimeError): """Report one stable failure without exposing TASK persistence.""" def __init__(self, code: TaskSubmissionContextFailure) -> None: - if code not in { - "task_submission_context_invalid", - "task_submission_predecessor_changed", - }: + if code not in get_args(TaskSubmissionContextFailure): raise ValueError("task submission context failure code is invalid") super().__init__(code) self.code = code🤖 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 `@backend/app/modules/tasks/api/submission_context.py` around lines 20 - 27, Update the validation in TaskSubmissionContextFailure.__init__ to derive the accepted failure-code set from the TaskSubmissionContextFailure alias instead of duplicating its string literals. Preserve the existing ValueError for unsupported codes and assignment of the validated code.backend/tests/architecture/test_module_boundaries.py (1)
196-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the import scan found imports.
If
rglobmatches no file, orexact_source_importsreturns an empty set,importsstays empty and theall(...)assertion passes without proving anything. Add a non-empty check so the boundary proof cannot pass vacuously.♻️ Proposed refactor
- imports = set() + api_files = list((ROOT / "backend/app/modules/tasks/api").rglob("*.py")) + assert api_files + imports: set[str] = set() - for path in (ROOT / "backend/app/modules/tasks/api").rglob("*.py"): + for path in api_files: imports.update(boundary.exact_source_imports(path, ROOT)) + assert imports assert all(🤖 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 `@backend/tests/architecture/test_module_boundaries.py` around lines 196 - 208, Update test_tasks_public_api_has_no_private_or_mutable_dependency to assert that imports is non-empty after scanning the tasks API files, before the existing all(...) boundary assertion, so the architecture proof cannot pass vacuously.backend/app/modules/tasks/repository.py (1)
118-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstrings for the new and changed
TaskRepositorymethods do not follow the file convention. Every other method inTaskRepositorydocuments its parameters and return value with Google-styleArgsandReturnssections. The two methods touched here omit the new parameters and the raised failure type.
backend/app/modules/tasks/repository.py#L118-L122: addArgs,Returns, andRaisessections tolock_submission_context, and nameTaskSubmissionContextUnavailablewith its two failure paths.backend/app/modules/tasks/repository.py#L254-L268: addfor_updateandpopulate_existingto theArgssection ofget_latest_submission_for_task.🤖 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 `@backend/app/modules/tasks/repository.py` around lines 118 - 122, Update TaskRepository.lock_submission_context in backend/app/modules/tasks/repository.py:118-122 with Google-style Args, Returns, and Raises sections, documenting both TaskSubmissionContextRequest and TaskSubmissionContextFacts and naming TaskSubmissionContextUnavailable for its two failure paths. Update get_latest_submission_for_task in backend/app/modules/tasks/repository.py:254-268 by documenting the for_update and populate_existing parameters in its Args section.backend/tests/test_tasks.py (1)
1747-1766: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCancel
contender_callin the cleanup block.If
_wait_for_task_database_lockraises theAssertionErrorat line 1566, or if line 1760 fails,contender_callis still pending. Thefinallyblock then callsawait contender.close()while that task holds the session and waits on a database lock. The close can hang or raise, and asyncio reports a destroyed pending task. Cancel the task before closing the sessions.♻️ Proposed refactor
holder = db_session.get_session_factory()() contender = db_session.get_session_factory()() + contender_call: asyncio.Task[Any] | None = None try: held_facts = await TaskRepository(holder).lock_submission_context(request) @@ contender_facts = await contender_call assert contender_facts == held_facts + contender_call = None finally: + if contender_call is not None: + contender_call.cancel() + with suppress(asyncio.CancelledError): + await contender_call await holder.close() await contender.close()This uses
contextlib.suppress, so addfrom contextlib import suppressto the imports.🤖 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 `@backend/tests/test_tasks.py` around lines 1747 - 1766, Cancel the pending contender_call in the finally cleanup before closing holder and contender sessions, and suppress the resulting cancellation exception using contextlib.suppress. Add the suppress import if needed, ensuring cleanup cannot hang when _wait_for_task_database_lock or an assertion fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/app/modules/tasks/api/submission_context.py`:
- Around line 20-27: Update the validation in
TaskSubmissionContextFailure.__init__ to derive the accepted failure-code set
from the TaskSubmissionContextFailure alias instead of duplicating its string
literals. Preserve the existing ValueError for unsupported codes and assignment
of the validated code.
In `@backend/app/modules/tasks/repository.py`:
- Around line 118-122: Update TaskRepository.lock_submission_context in
backend/app/modules/tasks/repository.py:118-122 with Google-style Args, Returns,
and Raises sections, documenting both TaskSubmissionContextRequest and
TaskSubmissionContextFacts and naming TaskSubmissionContextUnavailable for its
two failure paths. Update get_latest_submission_for_task in
backend/app/modules/tasks/repository.py:254-268 by documenting the for_update
and populate_existing parameters in its Args section.
In `@backend/tests/architecture/test_module_boundaries.py`:
- Around line 196-208: Update
test_tasks_public_api_has_no_private_or_mutable_dependency to assert that
imports is non-empty after scanning the tasks API files, before the existing
all(...) boundary assertion, so the architecture proof cannot pass vacuously.
In `@backend/tests/test_tasks.py`:
- Around line 1747-1766: Cancel the pending contender_call in the finally
cleanup before closing holder and contender sessions, and suppress the resulting
cancellation exception using contextlib.suppress. Add the suppress import if
needed, ensuring cleanup cannot hang when _wait_for_task_database_lock or an
assertion fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c2a4695-7e62-490e-8da1-c952ca2f20dd
📒 Files selected for processing (8)
.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/chunks/WS-ARCH-001-02A-task-submission-context-api.md.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/evidence/WS-ARCH-001-02A-task-manifest.mdbackend/app/modules/tasks/api/__init__.pybackend/app/modules/tasks/api/submission_context.pybackend/app/modules/tasks/repository.pybackend/tests/architecture/test_module_boundaries.pybackend/tests/test_tasks.pydocs/architecture_lockdown.md
2 participants
Chunk
WS-ARCH-001-02A — TASK Submission Context Public API
Goal
Expose immutable, dependency-safe TASK facts and a transaction-bound owner-local capability for task, assignment, predecessor, and Submission lifecycle context without changing live submission behavior.
Human-approved intent
Continue the modular-monolith correction incrementally. TASKS owns Submission creation and lifecycle truth; cross-module consumers must use typed public contracts rather than TASK ORM models, repositories, or sessions.
What changed
Why it changed
ART preparation cannot safely stop importing private TASK persistence until TASKS exposes its own authoritative transaction-bound facts.
Design chosen
Frozen, slotted UUID/value facts and a Protocol define the public surface. The implementation remains owner-local in TaskRepository and returns only TASK-owned facts plus locked PROJECT/CHECKER reference selectors. No ORM row, session, mutable policy body, or concrete service crosses the boundary.
Alternatives rejected
Scope control
No AUTH/ART activation, route/schema change, Submission persistence change, PROJECT/CHECKER canonical fact claim, dependency addition, or CI workflow change.
Product behavior
Unchanged. This installs a hidden structural capability only.
Acceptance criteria proof
Tests/checks run
Test delta
Additive tests only. No skips, xfails, removals, weakened assertions, or lowered thresholds.
CI integrity
No CI files or package scripts changed. The contract preserves the 90% subsystem threshold and now fails fast unless the required PostgreSQL test URL is supplied.
Reviewer results
Architecture PASS; security PASS; product/ops PASS after contributor/state corrections; QA PASS WITH LOW RISKS; senior engineering PASS; CI integrity PASS; docs PASS; reuse PASS; test-delta PASS WITH LOW RISKS.
External review
GitHub hosted checks and CodeRabbit are pending on this PR.
Remaining risks
The two new PostgreSQL tests were not executed locally because this worktree has no test database URL. Hosted CI is the required proof before merge.
Follow-up work
On human merge, WS-ARCH-001-02B becomes eligible. It adds the PROJECT locked-policy capability; this PR does not start it.
Human review focus
Review the public fact boundary, the task/assignment/predecessor lock order, crossed-state failures, and the PostgreSQL race proof.
Human merge ownership
A human owns the merge decision. This agent will not merge the PR without explicit approval for this PR.
Summary by CodeRabbit
New Features
Documentation
Tests