fix(tasks): cascade dependency cleanup on delete (#724)

[frankbria]

What & why

tasks.delete() left the deleted id in every other task's depends_on, and its docstring pointed users to a delete_cascade() that doesn't exist anywhere. A dependent kept a depends_on entry referencing the now-missing task; readiness requires deps.issubset(completed), so that dependent could never become ready — deleting a task silently and permanently stranded its dependents.

Fixes #724 [P0.13].

Change

tasks.delete() now strips the deleted id from every dependent's depends_on in the same transaction as the row delete (so no dangling reference is ever left), for all callers (CLI, API, agent). The dangling delete_cascade() docstring reference is removed and the cascade behavior documented.

Tests (tests/core/test_task_dependencies.py::TestDeleteCascade)

• Delete strips the id from a dependent's depends_on (→ [], not stranded).
• Other dependencies are preserved (only the deleted id is removed).
• Deleting a task with no dependents still works.

35 passed across the tasks suites. ruff + mypy clean.

Demo

before delete: B.depends_on = ['<A-id>']   (contains A)
after  delete: B.depends_on = []           (A stripped -> B not stranded)

Acceptance criteria

• Deleting a task strips its id from all dependents' depends_on (default behavior, all callers)
• The dangling delete_cascade() docstring reference is resolved (removed)
• Test: after deleting a dependency, the former dependent is no longer stranded

Summary by CodeRabbit

• Bug Fixes
  • Deleting a task now automatically removes it from other tasks’ dependency lists, preventing broken or dangling references.
  • Related tasks keep their other existing dependencies intact after a delete.
  • Delete behavior remains consistent when removing a task with no dependents.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 53 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 52e35e5a-ac12-4e61-a118-8a86a17000de

📥 Commits

Reviewing files that changed from the base of the PR and between 45a8db4 and 6edf4ad.

📒 Files selected for processing (2)

• codeframe/core/tasks.py
• tests/core/test_task_dependencies.py

Walkthrough

The delete() task function was modified to cascade cleanup of dependency references: before deleting a task, it now removes the task's id from every other task's depends_on list and updates their updated_at timestamps. New tests validate this cascading behavior.

Changes

Delete cascade cleanup

Layer / File(s)	Summary
Cascade delete on task removal codeframe/core/tasks.py	delete() queries all tasks, strips the deleted task's id from any dependent's depends_on list, updates updated_at, then deletes the target task; docstring updated accordingly.
Cascade delete test coverage tests/core/test_task_dependencies.py	Adds TestDeleteCascade verifying id removal from dependents, preservation of unrelated dependencies, and correct return value when no dependents exist.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant TasksDelete as tasks.delete()
  participant DB

  Caller->>TasksDelete: delete(task_id)
  TasksDelete->>DB: query all tasks (id, depends_on)
  DB-->>TasksDelete: task list
  TasksDelete->>DB: update depends_on/updated_at for dependents
  TasksDelete->>DB: delete target task row
  DB-->>TasksDelete: deletion result
  TasksDelete-->>Caller: True/False

Loading

Poem

A task hops off to burrow's end,
but leaves no dangling thread, my friend —
depends_on lists are swept up clean,
the tidiest patch you've ever seen.
Thump thump! 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Linked Issues check	❓ Inconclusive	The cascade cleanup and docstring fix match #724, but the summary doesn't confirm the required readiness/reachability test.	Add or document a test showing a former dependent becomes ready or reachable after deleting its dependency.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately summarizes the main change: cascading dependency cleanup on task delete.
Out of Scope Changes check	✅ Passed	No unrelated changes are evident; edits are limited to task deletion logic and matching dependency tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/p0.13-delete-cascade-deps

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

---

Code Review — fix(tasks): cascade dependency cleanup on delete (#724)

This is a clean, well-scoped fix for a real P0 correctness bug. The change is small, the intent is clear, and the tests cover the right cases. A few observations below.

---

Whats good

• Atomic fix: stripping dependents inside the same transaction as the DELETE is the right call — the comment explaining why (no dangling reference can survive a rollback) is appreciated.
• Docstring overhaul: the old "Use delete_cascade()" note was actively misleading; replacing it with the actual cascade behavior is correct.
• Tests: TestDeleteCascade covers the three meaningful cases (single dep, partial dep, no deps). test_delete_keeps_other_dependencies also implicitly validates that ordering is preserved by the list-comprehension filter.

---

Concerns and suggestions

1. Cascade runs even when the task does not exist (minor correctness issue)

The SELECT + UPDATE loop runs unconditionally before the DELETE. If task_id is not found (DELETE returns rowcount == 0 and returns False), the cascade updates have already been committed. In practice this is harmless — tasks depending on a ghost ID were already broken — but it is semantically wrong to mutate state when the function returns False. Consider checking existence first.

2. Full-workspace table scan

This fetches every task in the workspace and decodes every depends_on JSON blob in Python. For workspaces with hundreds of tasks this works fine; for larger ones it is wasteful. A LIKE pre-filter would reduce Python-side decoding while remaining correct.

Not a blocker for this PR, but worth a TODO comment if not addressed now.

3. The deleted task itself appears in the SELECT

Adding AND id != ? to the SELECT is a no-op correctness-wise but communicates intent and avoids a pointless JSON decode on the row about to be deleted.

4. get_dependents() cross-reference

get_dependents() (line 725) returns exactly the tasks that have task_id in their depends_on. You cannot reuse it inside delete() because it opens its own connection (breaking the transaction), but a brief comment pointing to it would help the next reader understand the shape of the data.

---

Test gap worth adding

The tests do not assert that cascade-updated tasks have their updated_at bumped. This is the only signal a cache or UI would use to detect a stale read.

---

Summary

The bug fix is correct and the core approach is sound. Items 1 (pre-existence check) and 3 (exclude self from scan) are small and worth fixing in this PR. Item 2 (scan efficiency) can be a follow-up. The updated_at test is a nice-to-have. Happy to approve once item 1 is addressed — it is the only case where the current code has an observable semantic difference from what the docstring promises.

[claude]

Code review follow-up — adding code examples missing from the earlier comment due to shell formatting restrictions.

Item 1 (blocking): The cascade SELECT+UPDATE runs before the DELETE, so if task_id is absent the function mutates other tasks then returns False. Fix: add a SELECT 1 guard before the cascade loop and return False early if the row is missing.

Item 3 (trivial): Add AND id != ? to the cascade SELECT to skip decoding the row that is about to be deleted.

Suggested test: Create A and B where B depends on A, capture b.updated_at before the delete, call tasks.delete(A), then assert the refreshed updated_at is greater than before. This verifies the timestamp is actually bumped on cascade-updated rows.

Items 2 and 4 from the main review comment are optional or follow-up. Fix is otherwise correct and well-tested.

[frankbria]

Thanks — addressed the review:

• Add Claude Code GitHub Workflow #1 (mutate-when-absent) ✅ Reordered: DELETE first, then cascade only if deleted (rowcount>0). A delete that finds nothing no longer touches dependents. New test test_delete_missing_task_does_not_touch_dependents pins it.
• feat(sprint-4): Multi-Agent Coordination System - P0 & P1 Complete #3 (self in scan) ✅ Free from the reorder — the deleted row is gone before the SELECT, so it can't appear.
• feat: Project schema refactoring with source types and workspace management #4 (get_dependents cross-ref) ✅ Added a comment pointing to get_dependents() as the read-side counterpart.
• Brainstorming: Integrate remaining general concepts into specification #2 (scan efficiency) — added a comment noting the full scan is fine for typical counts + the LIKE-prefilter upgrade path; left as follow-up per your note.
• Test gap (updated_at) ✅ test_delete_strips_id_from_dependents now asserts the dependent's updated_at is bumped.

[claude]

Code Review: fix(tasks): cascade dependency cleanup on delete

Summary: Clean, targeted P0 fix. The approach is correct and the test coverage is solid. A few observations below.

---

What's good

• Transaction atomicity is right: DELETE + cascade UPDATEs share one conn.commit(), so no dangling reference can survive a rollback or crash mid-cascade.
• fetchall() before the UPDATE loop: Consuming the SELECT cursor fully before issuing UPDATE statements avoids a cursor conflict. Good defensive practice.
• dep_json or empty-list idiom: Safe NULL-handling, consistent with the rest of the module.
• updated_at bump on dependents: Correct - cache/UI consumers need to detect the cascade edit, and the test verifies this.
• Test coverage: Four meaningful cases (strip, no-op on missing, preserve other deps, no-dependents). The test_delete_missing_task_does_not_touch_dependents case is especially important - it guards the if-deleted guard from regressing.

---

Minor observations

1. SELECT scans all workspace tasks unnecessarily - The cascade scan fetches every task in the workspace, including those with empty depends_on. A trivial pre-filter avoids loading empty-list rows: add AND depends_on != '[]' AND depends_on IS NOT NULL to the WHERE clause. Not critical for current workloads, but the existing get_dependents already does an in-Python filter - this would be the SQL-level equivalent and costs nothing.

2. No test for multi-hop chains (optional) - A->B->C: deleting B should strip B from C's deps but leave A unaffected. The current fix handles this correctly by design (it scans all dependents, not just direct ones), but there is no test that makes this explicit. Worth adding if this edge case matters to you.

3. Comment block length - The inline comment is 5 lines - thorough, but longer than the code it guards. The key sentence is the first one; the rest is context already in the docstring and PR description. Minor style point only.

---

Verdict

No correctness issues. The atomicity model is sound, the fix matches the bug description, and the tests cover the right cases. The SELECT pre-filter optimization is the only concrete improvement I would suggest, and it is strictly optional given the comment already flags it. Ready to merge once CI is green.

[claude]

Code Review — fix(tasks): cascade dependency cleanup on delete (#724)

Summary: Clean, targeted P0 fix. The approach is correct and the test coverage is solid. A few observations below.

---

What's good

• Transaction atomicity is right: DELETE + cascade UPDATEs share one conn.commit(), so no dangling reference can survive a rollback or crash mid-cascade.
• fetchall() before the UPDATE loop: Consuming the SELECT cursor fully before issuing UPDATE statements avoids a cursor conflict. Good defensive practice.
• dep_json or "[]" idiom: Safe NULL-handling, consistent with the rest of the module.
• updated_at bump on dependents: Correct — cache/UI consumers need to detect the cascade edit, and the test verifies this.
• Test coverage: Four meaningful cases (strip, no-op on missing, preserve other deps, no-dependents). The test_delete_missing_task_does_not_touch_dependents case is especially important — it guards the if deleted: guard from regressing.

---

Minor observations

1. SELECT scans all workspace tasks unnecessarily

The cascade scan fetches every task in the workspace, including those with empty depends_on:

cursor.execute(
    "SELECT id, depends_on FROM tasks WHERE workspace_id = ?",
    (workspace.id,),
)

The comment notes this, but a trivial pre-filter avoids loading empty-list rows:

cursor.execute(
    "SELECT id, depends_on FROM tasks "
    "WHERE workspace_id = ? AND depends_on != '[]' AND depends_on IS NOT NULL",
    (workspace.id,),
)

Not critical for current workloads, but the existing get_dependents already does an in-Python filter — this would be the SQL-level equivalent and costs nothing.

2. No test for multi-hop chains (optional)

A->B->C (C depends on B, B depends on A): deleting B should strip B from C's deps but leave A unaffected. The current fix handles this correctly by design (it scans all dependents, not just direct ones), but there's no test that makes this explicit. Worth adding if this edge case matters to you.

3. Comment block length

The inline comment is 5 lines — thorough, but longer than the code it guards. The key sentence is the first one; the rest is context that's already in the docstring and PR description. Minor style point only.

---

Verdict

No correctness issues. The atomicity model is sound, the fix matches the bug description, and the tests cover the right cases. The SELECT pre-filter optimization is the only concrete improvement I'd suggest, and it's strictly optional given the comment already flags it. Ready to merge once CI is green.