fix(core): dedupe blocker creation — one escalation, one blocker/webhook (#735)

[frankbria]

Summary

Fixes #735 — every blocked run produced two OPEN blockers and two blocker.created webhooks.

Root cause: adapters (verification_wrapper, react_agent, plan agent, builtin) persist a blocker before returning AgentResult(status="blocked"), then runtime.execute_agent (runtime.py:825) creates a second one from result.blocker_question. blockers.create had no dedupe and fired the outbound webhook per call.

Fix

Dedupe in blockers.create on (workspace_id, task_id, question, status=OPEN) — the acceptance criteria explicitly allows this. If a matching OPEN blocker already exists, return it without inserting a row, emitting a BLOCKER_CREATED event, or firing a webhook. First writer wins, so the agent-origin blocker is kept over the runtime's human-origin duplicate.

Single-point fix — covers every double-create path (verification_wrapper, react, plan, external), not just runtime.

Acceptance criteria

• Blocker creation deduped on (task_id, question, OPEN) — first writer owns it.
• One escalation → exactly one OPEN blocker and one webhook.

Verification (outcome evidence)

Simulating the real adapter→runtime double-call:

adapter blocker: 3f4ff428 origin: agent
runtime blocker: 3f4ff428 origin: agent   # same blocker returned
same blocker returned: True
OPEN blockers in workspace: 1
blocker.created webhooks fired: 1

Only one BLOCKER_CREATED event emitted. Tests: tests/core/test_blockers_webhook.py (dedupe, distinct-question, answered-status, null-task_id cases) + 99 adjacent blocker/runtime/adapter tests pass; ruff clean.

Known limitations

• Read-then-insert with no UNIQUE index; safe here because the adapter and runtime run sequentially in one process per run. Truly concurrent cross-process creates could still race (noted with a ponytail: comment).

[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: 43 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: 349ef14c-ab82-4fa5-8c6f-b3c4a9d6e34d

📥 Commits

Reviewing files that changed from the base of the PR and between 481fffc and 243d115.

📒 Files selected for processing (2)

• codeframe/core/blockers.py
• tests/core/test_blockers_webhook.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/735-dedupe-blocker-creation

---

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

[claude]

Review — fix(core): dedupe blocker creation (#735)

Overall: The fix is correct, well-scoped, and ready to ship. One non-standard comment prefix and a couple of missing edge-case tests are the only items worth addressing.

---

What's right

Single-point fix covers all callers. Deduping inside blockers.create() is the right level — verification_wrapper, react_agent, plan agent, and the runtime all go through this function, so there's no need to patch each caller individually.

NULL task_id handled correctly. Using IS NULL vs = ? in _find_open_duplicate is the correct SQLite idiom. Easy to get wrong, didn't.

Column ordering matches _row_to_blocker. The SELECT list in _find_open_duplicate is positionally identical to the other queries in the file, and _row_to_blocker uses positional indexing (row[0]…row[8]). ✓

Test matrix is solid for the core cases. Deduped, distinct questions, answered-status scoping, and null task_id are all covered.

---

Issues / suggestions

1. Non-standard comment prefix — minor

# ponytail: read-then-insert, no UNIQUE index — safe here because ...

ponytail: isn't a convention in this codebase. The content is valid and worth keeping (it's the WHY + known limitation). Use # NOTE: or just plain prose:

# NOTE: read-then-insert with no UNIQUE index — safe because the two callers
# run sequentially in the same process. Concurrent cross-process creates could
# race; a partial UNIQUE index on (workspace_id, task_id, question) WHERE status='OPEN'
# would make this atomic.

2. Two edge cases not tested — low risk but worth having

The WHERE clause is correct (verified by reading the query), but there are no tests that exercise:

• Same question, different task IDs on the same workspace — should produce two separate blockers:

  def test_dedupe_scoped_to_task_id(workspace):
      a = blockers.create(workspace, question="Q?", task_id="t-1")
      b = blockers.create(workspace, question="Q?", task_id="t-2")
      assert a.id != b.id

• Same question, different workspaces — should produce two separate blockers (workspace_id is in the WHERE, so this is safe, but a test would confirm it doesn't accidentally cross workspace boundaries).

3. Mock assertions outside with block — cosmetic

In test_duplicate_open_blocker_is_deduped, the assertions on second, list_open, and instance.send_event_background all run after the with patch(...) block exits. This works (the mock object persists in memory), but convention is to keep mock assertions inside the context manager so they're clearly paired with the patch scope:

with patch("codeframe.notifications.webhook.WebhookNotificationService") as MockSvc:
    instance = MockSvc.return_value
    first = blockers.create(...)
    second = blockers.create(...)
    assert second.id == first.id
    assert second.created_by == first.created_by
    instance.send_event_background.assert_called_once()

assert len(blockers.list_open(workspace)) == 1  # no webhook dependency, fine outside

---

Not an issue (for completeness)

• origin = BlockerOrigin(created_by) runs before the dedupe check. This means an invalid created_by value raises ValueError even when a duplicate exists and we'd return early. This is correct behavior — fail fast on bad input.
• Performance of added SELECT. blockers.create() is a rare human-in-the-loop event path, so the extra query cost is negligible.
• conn.close() error path. _find_open_duplicate follows the existing file-wide pattern of calling conn.close() only on the happy path. Not a regression.

---

Verdict: Approve with the above suggestions. The two missing edge-case tests (#2) are the only items I'd encourage adding before merge; the rest are cosmetic.

[frankbria]

Thanks for the review. Addressed in 243d115:

• Brainstorming: Integrate remaining general concepts into specification #2 (edge-case tests): added test_dedupe_scoped_to_task_id (same question, different tasks → separate blockers) and test_dedupe_does_not_cross_workspaces.
• Add Claude Code GitHub Workflow #1 (comment prefix): switched ponytail: → NOTE: and folded in the partial-UNIQUE-index upgrade path.
• feat(sprint-4): Multi-Agent Coordination System - P0 & P1 Complete #3 (mock assertion scope): left as-is — cosmetic, and the post-with assertions don't depend on the patch being active.

All findings were minor/cosmetic; no correctness changes to the fix itself.

[claude]

Follow-up — all previous items addressed ✓

Checked commit 243d115 against my earlier review:

• Add Claude Code GitHub Workflow #1 (comment prefix): ponytail: → NOTE: with the partial-UNIQUE-index upgrade path included. ✓
• Brainstorming: Integrate remaining general concepts into specification #2 (edge-case tests): test_dedupe_scoped_to_task_id and test_dedupe_does_not_cross_workspaces are both present and correct. ✓
• feat(sprint-4): Multi-Agent Coordination System - P0 & P1 Complete #3 (mock scope): Left cosmetic — acceptable, no correctness concern.

No new issues introduced. The fix is minimal, single-point, and the test matrix now covers all the meaningful boundary conditions. Approving.