feat(notifications): outbound webhook (#560)#594
Conversation
Settings → Notifications tab lets users configure a single webhook URL +
enabled toggle that receives JSON payloads on three events:
- batch.completed (only on full COMPLETED, not PARTIAL/FAILED/CANCELLED)
- blocker.created
- pr.merged (only on successful merges)
The Test button posts a sample {"event": "test"} payload and surfaces the
HTTP status code via toast.
Implementation:
- Per-workspace .codeframe/notifications_config.json (atomic write),
matching the workspace_config pattern from #556. No SQLite migration.
- WebhookNotificationService gains a generic send_event(payload, url)
that returns WebhookSendResult (ok/status_code/error) and a fire-and-
forget send_event_background wrapper. Legacy send_blocker_notification
API preserved for the existing in-agent blocker integration.
- Dispatch helpers in core/blockers.py, core/conductor.py (4 BATCH sites),
and ui/routers/pr_v2.py — all wrapped in try/except so a misconfigured
webhook never breaks the triggering operation. 5s timeout.
- 47 new tests across config storage, send_event, settings endpoints,
and per-trigger dispatch wiring.
Closes #560.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements end-to-end outbound webhook notifications: per-workspace config persistence, a webhook sender with payload formatters, best-effort dispatch on blocker.create, batch.completed, and pr.merged, settings API endpoints (get/put/test), and a Notifications settings UI with tests. ChangesWebhook Notifications Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Review — feat(notifications): outbound webhook (#560)Overall: This is a well-scoped implementation that cleanly follows the established patterns for workspace config, service layers, and tab UI. Architecture is sound — core is headless, dispatch is fire-and-forget, failures never bubble into callers. The test coverage is comprehensive. A few items to address before merge. Issues1. URL validation — SSRF risk (low but real)
from urllib.parse import urlparse
parsed = urlparse(url)
if parsed.scheme not in ('http', 'https'):
raise ValueError(f"Webhook URL must use http or https, got: {parsed.scheme!r}")2.
3. In except Exception: # pragma: no cover -- guarded by send_event_background alreadyThe justification is incorrect — 4. Timestamp interoperability
def _utc_iso_now() -> str:
return datetime.now(timezone.utc).strftime('%Y-%m-%dT%H:%M:%SZ')Not a blocker but may cause consumer-side parsing warnings. Minor / Nice to have
What's well done
Summary: Items 1 (URL validation) and 2 (CLI-context documentation) are worth addressing before merge. Items 3 and 4 are small but clean fixes. Everything else is polish. |
Demo verification (Phase 11) — end-to-end outcome evidenceStarted a local HTTP receiver on SetupAC1 — Webhook URL saves and persists
AC2 — Test button fires a payload and surfaces the HTTP statusReceiver got: {"event": "test", "timestamp": "2026-05-22T18:06:04.034448+00:00"}AC3 — Events fire on real triggersBlocker created ( {"event": "blocker.created", "blocker_id": "a1349c6a-8ea8-4dce-b1f4-c0a62302d015", "task_id": null, "timestamp": "..."}Batch completion ( {"event": "batch.completed", "batch_id": "demo-batch-2", "task_count": 5, "timestamp": "..."}Negative case ( AC4 — Failures don't break the triggering operationWith the webhook URL changed to AC5 — Tests pass
The PR-merge webhook ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/ui/test_settings_notifications.py (1)
39-47: ⚡ Quick winAdd a contract test that
workspace_pathis required.Because the fixture overrides
get_v2_workspace, these tests won’t catch regressions where the router stops requiringworkspace_path. Add one non-overridden test asserting request failure whenworkspace_pathis missing.As per coding guidelines,
codeframe/ui/routers/**/*.py: Web UI API client must requireworkspace_pathquery parameter for all endpoints.🤖 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/ui/test_settings_notifications.py` around lines 39 - 47, Add a new test (not using the client fixture that overrides get_v2_workspace) that mounts the settings_v2.router on a FastAPI app and issues a request to one of its endpoints without providing the workspace_path query parameter, asserting the response is a 400/422 (validation error) or otherwise indicates a missing required query param; specifically, create an app, include settings_v2.router, do NOT set app.dependency_overrides[get_v2_workspace], use TestClient to call an endpoint from settings_v2.router without workspace_path, and assert the request fails to ensure the router enforces the workspace_path requirement.
🤖 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 `@codeframe/core/notifications_config.py`:
- Around line 51-55: load_notifications_config currently assumes
json.loads(path.read_text()) returns a dict and calls data.get(...), which will
raise for non-object JSON (e.g., []); modify load_notifications_config to
validate that data is a mapping (e.g., isinstance(data, dict)) and if not, treat
it as an empty dict before accessing keys so that "webhook_url" and
"webhook_enabled" are read safely (return None and False respectively when
payload is not an object).
In `@codeframe/notifications/webhook.py`:
- Around line 275-293: send_event_background currently returns early when no
running asyncio loop, dropping webhooks; modify it so the sync call path still
queues delivery by, on RuntimeError, spawning a daemon thread that calls
asyncio.run(self.send_event(payload, url=url)) (or equivalent thread-target
wrapper) so the send runs asynchronously without blocking the caller. Update the
except RuntimeError block in send_event_background to create and start that
daemon Thread rather than logging and returning, keeping the existing
loop.create_task behavior when a running loop exists.
In `@codeframe/ui/routers/settings_v2.py`:
- Around line 433-496: Wrap calls to load_notifications_config,
save_notifications_config, and WebhookNotificationService.send_event in
try/except blocks inside get_notification_settings,
update_notification_settings, and test_notification_webhook respectively; on
exception, log the error (use logger.exception or request-scoped logger) and
raise HTTPException(status_code=500, detail=api_error(str(e),
ErrorCodes.SERVER_ERROR)) so the response follows the router's api_error payload
pattern, preserving the existing behavior for validation (e.g., keep the 400
when no URL) and include the exception message in the api_error detail for
debugging.
In `@tests/notifications/test_webhook_send_event.py`:
- Around line 111-119: The test is incorrectly decorated with
`@pytest.mark.asyncio` so an event loop exists and the "outside loop" branch isn't
exercised; change test_send_event_background_outside_loop_logs_and_returns to a
synchronous test by removing the `@pytest.mark.asyncio` decorator and converting
async def to def, instantiate WebhookNotificationService and call
svc.send_event_background({"event":"test"}) exactly as before (optionally keep
assertions or caplog checks) so the call runs outside any event loop and
exercises the intended branch in
WebhookNotificationService.send_event_background.
---
Nitpick comments:
In `@tests/ui/test_settings_notifications.py`:
- Around line 39-47: Add a new test (not using the client fixture that overrides
get_v2_workspace) that mounts the settings_v2.router on a FastAPI app and issues
a request to one of its endpoints without providing the workspace_path query
parameter, asserting the response is a 400/422 (validation error) or otherwise
indicates a missing required query param; specifically, create an app, include
settings_v2.router, do NOT set app.dependency_overrides[get_v2_workspace], use
TestClient to call an endpoint from settings_v2.router without workspace_path,
and assert the request fails to ensure the router enforces the workspace_path
requirement.
🪄 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: 953e7b11-c3e1-4e93-9d82-70e970fc428e
📒 Files selected for processing (20)
CLAUDE.mdcodeframe/core/blockers.pycodeframe/core/conductor.pycodeframe/core/notifications_config.pycodeframe/notifications/webhook.pycodeframe/ui/routers/pr_v2.pycodeframe/ui/routers/settings_v2.pydocs/PRODUCT_ROADMAP.mdtests/core/test_blockers_webhook.pytests/core/test_conductor_webhook.pytests/core/test_notifications_config.pytests/notifications/test_webhook_send_event.pytests/ui/test_pr_v2_webhook.pytests/ui/test_settings_notifications.pyweb-ui/src/__tests__/components/settings/NotificationsTab.test.tsxweb-ui/src/__tests__/components/settings/SettingsPage.test.tsxweb-ui/src/app/settings/page.tsxweb-ui/src/components/settings/NotificationsTab.tsxweb-ui/src/lib/api.tsweb-ui/src/types/index.ts
Internal review found four issues; this commit addresses all of them.
1. **SSRF guard (high)**: `PUT /api/v2/settings/notifications` now rejects
URLs whose scheme isn't `http(s)` or that lack a host. Previously a user
could enter `file:///etc/passwd` and aiohttp would attempt the request.
2. **CLI sync-context delivery (high)**: `send_event_background` now
spawns a daemon thread when no event loop is running. Previously
webhooks would silently never fire from `cf work batch run` and other
CLI-initiated batch / blocker creation paths — a functional defect
against AC3 of the ticket. Verified end-to-end against a local
receiver: both `blocker.created` and `batch.completed` events arrive
from sync Python with no event loop.
3. **Incorrect `# pragma: no cover` (low)**: Removed from `blockers.py`.
The justification was wrong — the import inside the helper can fail
independently of the network-level guard in `send_event_background`.
4. **Timestamp interoperability (low)**: Payload `timestamp` now uses
`YYYY-MM-DDTHH:MM:SSZ` instead of `…+00:00` offset format. Slack,
Discord, and Zapier all prefer the Z suffix. Sub-second precision was
dropped along with the offset.
New tests:
- 5 endpoint tests for URL scheme/host validation (file://, ftp://,
http://, https://, missing host)
- 1 test confirming `send_event_background` runs in a daemon thread
when called from sync context
- 1 test confirming the Z-suffix timestamp format
Review feedback addressed (commit 6d7f1fc)Internal Claude review flagged 4 issues; all fixed.
Re-demo: CLI sync-context (the AC3 defect)Note the Tests
|
Follow-up Review — feat(notifications): outbound webhook (#560)Reviewing against the prior review comment from the initial cut. The author also ran a complete end-to-end demo (AC1–AC5) with a live HTTP receiver, which is strong evidence of correctness. Prior Issues — Status1. SSRF URL validation ✅ Resolved 2. CLI sync dispatch (fire-and-forget from non-async context) ✅ Resolved 3. pragma: no cover inconsistency ✅ Not present 4. Timestamp Z suffix ✅ Resolved Remaining Concern (one item)Non-dict JSON in The Minimal fix: data = json.loads(path.read_text())
if not isinstance(data, dict):
raise ValueError(f"Expected JSON object, got {type(data).__name__}")CodeRabbit flagged this too. Small but real gap — a manually edited config file could trigger it. What is Well Done
Verdict: The one remaining open item ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@codeframe/ui/routers/settings_v2.py`:
- Around line 450-480: The PUT-only validation in _validate_webhook_url allows
persisted/hand-edited webhook URLs to bypass checks; ensure validation runs at
send/load time by invoking _validate_webhook_url (or its logic) from every
runtime send path before creating/using WebhookNotificationService: update
test_notification_webhook to call _validate_webhook_url on the loaded URL and
modify the dispatcher in codeframe/core/blockers.py (where notifications are
read from notifications_config.json and sent) to re-validate and reject/skip
invalid entries, or centralize validation in the shared load/send routine so
persisted configs cannot bypass the scheme and host checks.
🪄 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: a347c4ad-df45-4eb1-a1be-c6d8024d8e1f
📒 Files selected for processing (5)
codeframe/core/blockers.pycodeframe/notifications/webhook.pycodeframe/ui/routers/settings_v2.pytests/notifications/test_webhook_send_event.pytests/ui/test_settings_notifications.py
Address remaining review feedback from CodeRabbit and the follow-up
Claude review:
1. **Non-dict JSON crashes `load_notifications_config`**: a config file
containing valid-but-non-object JSON (`[]`, `null`, `42`) caused
`data.get(...)` to raise `AttributeError` uncaught, violating the
"never raises" contract. `isinstance(data, dict)` guard added; the
shared except clause now catches it via re-raised ValueError.
2. **Defence-in-depth URL validation at load/send time**: the PUT
endpoint validates URL scheme/host, but a hand-edited or
pre-migration `.codeframe/notifications_config.json` could bypass
that. `is_webhook_active` now re-runs scheme/host validation and
returns None (skipping dispatch) if a stored URL is unsafe. The
`/test` endpoint also re-validates before sending.
3. **Structured exception handling for endpoints**: `PUT /notifications`
and `POST /notifications/test` now wrap I/O failures in the standard
`api_error(...)` shape used elsewhere in the router, instead of
bubbling raw 500s.
New tests:
- 3 corrupt-JSON cases in test_notifications_config.py (list/null/int)
- 3 unsafe-URL cases in is_webhook_active (file://, schemeless, valid
https as positive control)
- 1 endpoint test rejecting an unsafe stored URL at /test time
130/130 webhook-related v2 tests pass.
Round 2 — addressing CodeRabbit + follow-up Claude review (commit 77adedb)CodeRabbit (cross-family review) and the follow-up Claude review converged on the same remaining concern after my last fixup, plus two adjacent items. All three are addressed.
New tests (8)
Backend: 130/130 webhook-related v2 tests pass. Lint clean. |
Code Review — feat(notifications): outbound webhook (#560)Overall this is a solid implementation. The architecture respects the core-headless contract, the fire-and-forget isolation is thorough, the defense-in-depth URL validation is a nice touch, and the test coverage is comprehensive across all layers. A few issues worth addressing before merge: Issues1. Unused expression in sync-dispatch thread test In threading.active_count() - threads_before # informational onlyThis computes a value but discards it — it is not an assertion and does nothing. If you want to verify no threads leak, turn it into 2. Private-network SSRF not blocked
3. Test button tooltip vs. disabled state edge case In const canTest =
!testing && trimmedUrl.length > 0 && (data.webhook_url ?? '') === trimmedUrl;When a user appends a trailing space to a saved URL, const canTest = !testing && !dirty && trimmedUrl.length > 0 && !!data.webhook_url;4. The class docstring still reads "Async webhook notification service for blocker alerts" — the class is now general-purpose (batch / blocker / PR / test events). The module-level docstring was updated; the class docstring was not. 5. The payload only has Nitpicks
What is working well
Items 1 and 3 are the ones I would most want addressed before landing; 2 is worth a TODO comment at minimum. |
Address remaining follow-up Claude review items. 1. **Useless expression in sync-dispatch test**: replaced `threading.active_count() - threads_before # informational only` with a real assertion that waits up to ~1s for the daemon thread to join and asserts no thread leak. 2. **Private-network SSRF gap**: added explicit TODO in `_is_safe_webhook_url` noting that RFC-1918 and loopback addresses are intentionally permitted for self-hosted single-user use; flagged for a socket-level check if CodeFRAME ever runs multi-tenant. 3. **Stale class docstring**: `WebhookNotificationService` is no longer blocker-only; docstring now describes the general-purpose `send_event` role alongside the legacy `send_blocker_notification`. 4. **`format_batch_payload` self-documenting status field**: added `"status": "completed"` so consumers don't need out-of-band knowledge that the event only fires for COMPLETED today; forward-compatible if PARTIAL/FAILED are added later. 5. **`pr_url` no longer carries an unparseable sentinel**: when GitHubIntegration can't be constructed, the payload now sets `pr_url: None` instead of `"pr#N"`. Added a new always-present `pr_number` field so consumers can branch on it without parsing a URL. 6. **`canTest` requires !dirty**: defence-in-depth so the Test button never fires against a value other than the persisted/displayed URL, even via programmatic state edits. Test updates: 2 new payload-shape tests (`status`, optional `pr_url`), 1 thread-leak assertion, plus the PR-merge tests updated to the new `format_pr_payload(pr_number, pr_url)` signature. 132/132 backend webhook tests pass; 12/12 frontend NotificationsTab tests pass; lint clean; build clean.
Round 3 — polish for follow-up Claude review (commit 97989f9)The latest review caught a useless expression, several payload/API design improvements, and a minor doc/SSRF gap. All addressed.
Tests
Payload changes (additive, backward-compatible for existing consumers)
Both changes are documented in code via the comments on the format helpers. |
Code Review — Round 4 (commit 97989f9)This review checks that all Round 3 items are correctly implemented in the current code. Round 3 Items — All Resolved
Spot-checks on the Current Diff
One Minor Observation (no action required)
VerdictAll prior review items across four rounds are resolved. The implementation is clean: core is headless, dispatch is consistently fire-and-forget, URL validation is defence-in-depth at both the API layer and the send layer, and the 60+ new tests cover the full surface. Ready to merge. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@codeframe/core/notifications_config.py`:
- Around line 137-141: The current logger.warning call in the webhook validation
branch logs the raw url (see _is_safe_webhook_url and the logger.warning call),
which may leak secrets; change it to log a redacted representation instead—parse
the provided url (using urllib.parse or equivalent) and reconstruct only the
safe parts (e.g., scheme://host[:port] and path/query replaced with "<redacted>"
or omitted) and use that redacted string in the warning message so the log still
indicates which host/scheme was refused without exposing tokens or credentials.
In `@codeframe/ui/routers/pr_v2.py`:
- Around line 198-205: The docstring describing the helper is incorrect about
the pr_url fallback: update the helper's docstring to reflect that when
GitHubIntegration() fails the code sets pr_url = None (not "pr#N"); locate the
block using GitHubIntegration, the pr_url variable and pr_number and change the
docstring text to state that the canonical github.com URL is returned when
GITHUB_REPO is configured, otherwise pr_url will be None and consumers should
rely on the always-present pr_number field.
🪄 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: 1c80de8c-8629-4a02-ae2a-81e2986d9bd2
📒 Files selected for processing (10)
codeframe/core/notifications_config.pycodeframe/notifications/webhook.pycodeframe/ui/routers/pr_v2.pycodeframe/ui/routers/settings_v2.pytests/core/test_notifications_config.pytests/notifications/test_webhook_send_event.pytests/ui/test_pr_v2_webhook.pytests/ui/test_settings_notifications.pyweb-ui/src/__tests__/components/settings/NotificationsTab.test.tsxweb-ui/src/components/settings/NotificationsTab.tsx
) Two new CodeRabbit findings on the latest commits: 1. **URL credential leak in logs** (Major): `is_webhook_active` was logging the full rejected URL via `%s`. Webhook URLs commonly embed credentials in the path/query (Slack `T*/B*/...` tokens, signed Zapier hooks, GitHub PAT URLs). Added `_redact_url_for_log` that returns just `scheme://host` for log output. New test asserts path-embedded tokens and query parameters never appear in log text. 2. **Stale docstring** (Minor): `_dispatch_pr_merged_webhook` docstring still said the fallback was `pr#N`, but round-3 changed it to `pr_url = None` (with `pr_number` always present). Docstring now matches the implementation.
Round 4 — secret-leak guard + stale docstring (commit 3c022c5)CodeRabbit's late-arriving review on the round-2/3 commits caught two more items.
Both threads marked resolved via GraphQL. Backend webhook tests: 104/104 passing (1 new redaction test). Lint clean. |
Code Review — feat(notifications): outbound webhook (#560)Context: This is my first review pass. CodeRabbit has done three passes and the author has iterated on the feedback. The current diff reflects those fixes (non-object JSON guard, daemon-thread sync path, URL redaction in logs, What's working well
Issues to address1. In url = (cfg["webhook_url"] or "").strip()
if not url:
raise HTTPException(400, ...)
_validate_webhook_url(url) # ← return value thrown away
svc = WebhookNotificationService(webhook_url=url, timeout=5)
2. In loop.create_task(self.send_event(payload, url=url))
task = loop.create_task(self.send_event(payload, url=url))
task.add_done_callback(lambda t: t.exception() if not t.cancelled() else None)Minor, but worth one line to prevent confusing log noise in production. 3. Missing type annotation on def _dispatch_batch_completed_webhook(
workspace: Workspace, event_type, batch_id: str, task_count: int
) -> None:
4. Remaining CodeRabbit nitpick (open) The suggestion from the first CodeRabbit pass to add a contract test that 5. The PR description says config is persisted "via SummaryStrong implementation — the layered security, failure isolation, and test depth are all well above par for a fire-and-forget notification feature. Items 1–3 above are small fixes (one line each); item 4 is a pre-existing open suggestion; item 5 is a note for future work. Ready to merge once items 1–3 are addressed. |
…560) Latest claude-review caught three small follow-ups: 1. **Python 3.11+ task-exception log noise**: ``loop.create_task`` result was discarded in the async path of ``send_event_background``. If ``send_event`` ever raises through its catch-all, Python 3.11+ emits ``Task exception was never retrieved`` to the logs. Added a no-op done-callback that calls ``.exception()`` to consume the result. 2. **Missing type annotation**: ``_dispatch_batch_completed_webhook``'s ``event_type`` parameter now annotated as ``events.EventType``. 3. **Discarded return-value clarity**: added an inline comment in the ``/notifications/test`` endpoint explaining that we call ``_validate_webhook_url`` for its raise side-effect, not its return. New test: - ``test_send_event_background_task_has_done_callback`` verifies the done-callback is registered on the created task (via task repr, which CPython exposes the callback list through). Also documented why the "require workspace_path" contract test suggested by CodeRabbit isn't feasible at the endpoint level — the ``get_v2_workspace`` dependency falls back to default-workspace resolution, so it'd need a separate router-wide fix, not a #560 test.
Round 5 — claude-review nits (commit 2044c7c)Three small follow-ups from the most recent claude review.
About the open CodeRabbit nitpick (workspace_path contract test)I investigated this — Inline comment added to the test file explaining this — won't reopen the conversation. Tests
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/notifications/test_webhook_send_event.py (1)
188-188: 💤 Low valueConsider a less fragile assertion for callback presence.
The test relies on the exact string
"send_event_background.<locals>.<lambda>"appearing in the task's repr, which depends on Python's internal naming conventions. This assertion could break if the function is renamed, the lambda is moved, or Python's repr format changes.While this approach works for now, a more robust alternative might be to check the task's
_callbacksattribute directly (though still implementation-dependent), or to accept that verifying done-callback registration is inherently fragile and document the limitation.🤖 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/notifications/test_webhook_send_event.py` at line 188, The assertion that the done-callback exists is fragile because it depends on the lambda's repr; instead modify the test around send_event_background and captured_tasks to check callback registration more robustly by inspecting the Task object's _callbacks (or documenting the limitation): access captured_tasks[0] (the asyncio.Task returned by send_event_background) and assert that any(callback for callback in task._callbacks if getattr(callback, "__name__", "") or repr(callback) matches a looser pattern) — i.e., replace the exact string check for "send_event_background.<locals>.<lambda>" with a check that task._callbacks is non-empty or contains a callable whose repr/name includes "send_event_background" or "lambda", so the test no longer relies on Python's precise repr format.
🤖 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 `@codeframe/core/notifications_config.py`:
- Around line 139-140: The current redaction leaks basic-auth credentials
because _redact_url_for_log (and its other callers in the same block) uses
parsed.netloc which retains user:password@host; update _redact_url_for_log to
reconstruct the netloc using parsed.hostname (and parsed.port if present)
instead of parsed.netloc, explicitly omitting parsed.username and
parsed.password so credentials are never included in logs, and ensure every
callsite that logs URLs uses this updated helper.
In `@tests/notifications/test_webhook_send_event.py`:
- Around line 191-194: The test cancels a pending task but does not await it,
which can produce "Task was destroyed but it is pending" warnings; after calling
captured_tasks[0].cancel() in the except asyncio.TimeoutError block, await the
cancelled task (e.g., await captured_tasks[0]) and handle asyncio.CancelledError
if needed so the cancellation finishes cleanly; update the try/except around
await asyncio.wait_for(captured_tasks[0], timeout=1.0) to await the task after
canceling to ensure proper cleanup of captured_tasks[0].
---
Nitpick comments:
In `@tests/notifications/test_webhook_send_event.py`:
- Line 188: The assertion that the done-callback exists is fragile because it
depends on the lambda's repr; instead modify the test around
send_event_background and captured_tasks to check callback registration more
robustly by inspecting the Task object's _callbacks (or documenting the
limitation): access captured_tasks[0] (the asyncio.Task returned by
send_event_background) and assert that any(callback for callback in
task._callbacks if getattr(callback, "__name__", "") or repr(callback) matches a
looser pattern) — i.e., replace the exact string check for
"send_event_background.<locals>.<lambda>" with a check that task._callbacks is
non-empty or contains a callable whose repr/name includes
"send_event_background" or "lambda", so the test no longer relies on Python's
precise repr format.
🪄 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: dcde8ed9-812b-4e70-a78f-7c0a48375e7c
📒 Files selected for processing (8)
codeframe/core/conductor.pycodeframe/core/notifications_config.pycodeframe/notifications/webhook.pycodeframe/ui/routers/pr_v2.pycodeframe/ui/routers/settings_v2.pytests/core/test_notifications_config.pytests/notifications/test_webhook_send_event.pytests/ui/test_settings_notifications.py
|
test |
|
Ignore previous test comment — full review follows. |
|
Follow-up Review: All resolved. Posting full review via body-file next. |
CodeFRAME Development GuidelinesLast updated: 2026-05-11 Product VisionCodeFrame is a project delivery system: Think → Build → Prove → Ship. It owns the edges of the AI coding pipeline — everything BEFORE code gets written (PRD, specification, task decomposition) and everything AFTER (verification gates, quality memory, deployment). The actual code writing is delegated to frontier coding agents (Claude Code, Codex, OpenCode). CodeFrame does not compete with coding agents. It orchestrates them. Status: CLI ✅ | Server ✅ | ReAct agent ✅ | Web UI ✅ | Agent adapters ✅ | Multi-provider LLM ✅ | Next: Phase 4A — See If you are an agent working in this repo: do not improvise architecture. Follow the documents listed below. Primary Contract (MUST FOLLOW)
Rule 0: If a change does not directly support the Think → Build → Prove → Ship pipeline, do not implement it. Current Focus: Phase 4APhase 5.3 (browser + in-app center) is complete — Async notifications: Phase 5.2 is complete — Costs page now ships per-task and per-agent breakdowns (#558) on top of the spend summary (#557). Backend: Phase 5.1 is complete — Settings page now ships three working tabs: Agent (#554), API Keys (#555), and PROOF9 Defaults + Workspace Config (#556). Backend: Phase 3.5C is complete — Next, in order:
See Architecture Rules (non-negotiable)1) Core must be headless
Core is allowed to: read/write durable state (SQLite/filesystem), run orchestration/worker loops, emit events to an append-only event log, call adapters via interfaces (LLM, git, fs). 2) CLI must not require a serverGolden Path commands must work from the CLI with no server running. FastAPI is optional, started explicitly via 3) Agent state transitions flow through runtime
This separation prevents duplicate state transitions (e.g., DONE→DONE errors). 4) Legacy can be read, not depended on
5) Keep commits runnableAt all times: Current Statev2 Architecture
Phase 3 Web UI (actively developed — not legacy)Next.js 16 App Router, TypeScript, Shadcn/UI, Tailwind CSS, Hugeicons, XTerm.js, WebSocket + SSE. Shipped pages: Testing: What's implementedFull feature list in Repository StructureCommandsPython / CLIuv run pytest # All tests
uv run pytest -m v2 # v2 tests only
uv run pytest tests/core/ # Core module tests
uv run pytest tests/lifecycle/ # Lifecycle tests (no live API calls — uses MockProvider)
uv run ruff check .
# Web UI
cd web-ui && npm test
cd web-ui && npm run buildGolden Path CLI# Workspace
cf init <repo> [--detect | --tech-stack "..." | --tech-stack-interactive]
cf status
# PRD
cf prd add <file.md>
cf prd show
# Tasks
cf tasks generate
cf tasks list [--status READY]
cf tasks show <id>
# Work — single task
cf work start <task-id> [--execute] [--engine react|plan] [--verbose] [--dry-run]
cf work start <task-id> --execute --stall-timeout 120 --stall-action retry|blocker|fail
cf work start <task-id> --execute --llm-provider openai --llm-model gpt-4o
cf work stop <task-id>
cf work resume <task-id>
cf work follow <task-id> [--tail 50]
cf work diagnose <task-id>
# Work — batch
cf work batch run [<id>...] [--all-ready] [--engine react|plan]
cf work batch run --strategy serial|parallel|auto [--max-parallel 4] [--retry 3]
cf work batch run --all-ready --llm-provider openai --llm-model qwen2.5-coder:7b
cf work batch status|cancel|resume [batch_id]
# Blockers
cf blocker list
cf blocker show <id>
cf blocker answer <id> "answer"
# Quality / State
cf review && cf patch export && cf commit
cf checkpoint create|list|restore
cf summary
# Environment
cf env check|install|doctor
# GitHub PR
cf pr create|status|checks|mergeNote: What NOT to do
Testing / DemoingQuality check (covers both backend and web UI)uv run pytest && uv run ruff check .
cd web-ui && npm test && npm run buildNew v2 tests: add Demoing against a sample project (e.g.,
|
| Doc | Purpose |
|---|---|
docs/VISION.md |
North star: Think → Build → Prove → Ship thesis |
docs/PRODUCT_ROADMAP.md |
Current roadmap — Phase 3.5/4/5 web product completeness |
docs/GOLDEN_PATH.md |
CLI-first workflow contract |
docs/CLI_WIREFRAME.md |
Command → module mapping |
docs/AGENT_SYSTEM_REFERENCE.md |
Component table, model selection, execution flows, self-correction |
docs/REACT_AGENT_ARCHITECTURE.md |
ReAct deep-dive: tools, editor, token management |
docs/PHASE_3_UI_ARCHITECTURE.md |
Web UI architecture (Next.js, pages, components) |
docs/PHASE_2_DEVELOPER_GUIDE.md |
Server layer + v2 router patterns |
docs/PHASE_2_CLI_API_MAPPING.md |
CLI to API endpoint mapping |
docs/QUICKSTART.md |
User-facing quickstart guide |
Archived (completed plans, old gap analyses): docs/archive/
Legacy (v1 reference only): SPRINTS.md, sprints/, specs/, CODEFRAME_SPEC.md
|
[PLACEHOLDER - will be replaced] |
|
Follow-up Review - feat(notifications): outbound webhook All four issues from my prior review are resolved. Here is a pass over the updated diff. Resolved
Minor observations (non-blocking) Duplicated scheme set - _ALLOWED_SCHEMES in notifications_config.py and _ALLOWED_WEBHOOK_SCHEMES in settings_v2.py both encode frozenset. This is the correct trade-off given the core-headless constraint, but a brief comment at one or both sites would prevent a future reader from consolidating them incorrectly. /test endpoint ignores webhook_enabled - The test endpoint fires against a configured URL regardless of the enabled flag. This is the right UX call (validate the URL before toggling delivery on), and test_test_works_even_when_enabled_flag_is_false documents the intent. Worth keeping explicit in the endpoint docstring so it does not get fixed later. Thread-count assertion in test_send_event_background_outside_loop_runs_in_thread - The threading.active_count() delta assertion is slightly fragile under parallel test runners where other threads may be concurrently active. Not a real concern for the current test setup, but if this test ever flakes the assertion could be relaxed to <= 1. Overall Ready to merge. The security handling (scheme validation, log redaction), the correct dual async/sync dispatch path, and the test breadth across all three dispatch sites are all well done. No blocking issues. |
|
[] |
|
DELETING_TEST_COMMENT |
CodeFRAME Development GuidelinesLast updated: 2026-05-11 Product VisionCodeFrame is a project delivery system: Think → Build → Prove → Ship. It owns the edges of the AI coding pipeline — everything BEFORE code gets written (PRD, specification, task decomposition) and everything AFTER (verification gates, quality memory, deployment). The actual code writing is delegated to frontier coding agents (Claude Code, Codex, OpenCode). CodeFrame does not compete with coding agents. It orchestrates them. Status: CLI ✅ | Server ✅ | ReAct agent ✅ | Web UI ✅ | Agent adapters ✅ | Multi-provider LLM ✅ | Next: Phase 4A — See If you are an agent working in this repo: do not improvise architecture. Follow the documents listed below. Primary Contract (MUST FOLLOW)
Rule 0: If a change does not directly support the Think → Build → Prove → Ship pipeline, do not implement it. Current Focus: Phase 4APhase 5.3 is complete — Async notifications cover both surfaces:
Phase 5.2 is complete — Costs page now ships per-task and per-agent breakdowns (#558) on top of the spend summary (#557). Backend: Phase 5.1 is complete — Settings page now ships three working tabs: Agent (#554), API Keys (#555), and PROOF9 Defaults + Workspace Config (#556). Backend: Phase 3.5C is complete — Next, in order:
See Architecture Rules (non-negotiable)1) Core must be headless
Core is allowed to: read/write durable state (SQLite/filesystem), run orchestration/worker loops, emit events to an append-only event log, call adapters via interfaces (LLM, git, fs). 2) CLI must not require a serverGolden Path commands must work from the CLI with no server running. FastAPI is optional, started explicitly via 3) Agent state transitions flow through runtime
This separation prevents duplicate state transitions (e.g., DONE→DONE errors). 4) Legacy can be read, not depended on
5) Keep commits runnableAt all times: Current Statev2 Architecture
Phase 3 Web UI (actively developed — not legacy)Next.js 16 App Router, TypeScript, Shadcn/UI, Tailwind CSS, Hugeicons, XTerm.js, WebSocket + SSE. Shipped pages: Testing: What's implementedFull feature list in Repository StructureCommandsPython / CLIuv run pytest # All tests
uv run pytest -m v2 # v2 tests only
uv run pytest tests/core/ # Core module tests
uv run pytest tests/lifecycle/ # Lifecycle tests (no live API calls — uses MockProvider)
uv run ruff check .
# Web UI
cd web-ui && npm test
cd web-ui && npm run buildGolden Path CLI# Workspace
cf init <repo> [--detect | --tech-stack "..." | --tech-stack-interactive]
cf status
# PRD
cf prd add <file.md>
cf prd show
# Tasks
cf tasks generate
cf tasks list [--status READY]
cf tasks show <id>
# Work — single task
cf work start <task-id> [--execute] [--engine react|plan] [--verbose] [--dry-run]
cf work start <task-id> --execute --stall-timeout 120 --stall-action retry|blocker|fail
cf work start <task-id> --execute --llm-provider openai --llm-model gpt-4o
cf work stop <task-id>
cf work resume <task-id>
cf work follow <task-id> [--tail 50]
cf work diagnose <task-id>
# Work — batch
cf work batch run [<id>...] [--all-ready] [--engine react|plan]
cf work batch run --strategy serial|parallel|auto [--max-parallel 4] [--retry 3]
cf work batch run --all-ready --llm-provider openai --llm-model qwen2.5-coder:7b
cf work batch status|cancel|resume [batch_id]
# Blockers
cf blocker list
cf blocker show <id>
cf blocker answer <id> "answer"
# Quality / State
cf review && cf patch export && cf commit
cf checkpoint create|list|restore
cf summary
# Environment
cf env check|install|doctor
# GitHub PR
cf pr create|status|checks|mergeNote: What NOT to do
Testing / DemoingQuality check (covers both backend and web UI)uv run pytest && uv run ruff check .
cd web-ui && npm test && npm run buildNew v2 tests: add Demoing against a sample project (e.g.,
|
| Doc | Purpose |
|---|---|
docs/VISION.md |
North star: Think → Build → Prove → Ship thesis |
docs/PRODUCT_ROADMAP.md |
Current roadmap — Phase 3.5/4/5 web product completeness |
docs/GOLDEN_PATH.md |
CLI-first workflow contract |
docs/CLI_WIREFRAME.md |
Command → module mapping |
docs/AGENT_SYSTEM_REFERENCE.md |
Component table, model selection, execution flows, self-correction |
docs/REACT_AGENT_ARCHITECTURE.md |
ReAct deep-dive: tools, editor, token management |
docs/PHASE_3_UI_ARCHITECTURE.md |
Web UI architecture (Next.js, pages, components) |
docs/PHASE_2_DEVELOPER_GUIDE.md |
Server layer + v2 router patterns |
docs/PHASE_2_CLI_API_MAPPING.md |
CLI to API endpoint mapping |
docs/QUICKSTART.md |
User-facing quickstart guide |
Archived (completed plans, old gap analyses): docs/archive/
Legacy (v1 reference only): SPRINTS.md, sprints/, specs/, CODEFRAME_SPEC.md
CodeFRAME v2 — Golden Path Contract (CLI-first)This document is the contract for CodeFRAME v2 development. Rule 0 (the only rule that matters):
This applies to both humans and agentic coding assistants. GoalsWhat "done" looks like (Enhanced MVP definition)CodeFRAME can run a complete end-to-end AI-driven development workflow from the CLI on a target repo:
No UI is required. Non-Goals (explicitly forbidden until Golden Path works)Do not build or refactor:
These may be revisited only after Golden Path is working and stable. Golden Path CLI Flow (the only flow that matters)0) Preconditions
1) Initialize a workspaceCommand:
Required behavior:
Artifacts:
2) AI-driven PRD generation and refinementCommands:
Required behavior for
3) Intelligent task generation with dependency analysisCommands:
Required behavior:
4) Batch task execution with orchestrationCommands:
Required behavior for batch execution:
5) Enhanced human-in-loop blocker resolutionCommands:
Required behavior:
6) Integrated Git workflow and PR managementCommands:
Required behavior:
7) Enhanced verification and quality gatesCommands:
Required behavior:
8) Integrated artifact and commit managementCommands:
Required behavior:
9) Comprehensive checkpointing and state managementCommands:
Required behavior:
State Machine (authoritative)Statuses:
Allowed transitions (comprehensive):
The CLI is the authority for transitions. PR Workflow Integration:
Implementation PrinciplesCore-first (no FastAPI in the core)
CLI-first (server optional)
Salvage safely
Keep it runnable
Acceptance Checklist (Enhanced MVP - must pass)Status: 🔄 Enhanced MVP Partially Complete 📊 Current Implementation StatusOverall Assessment: Enhanced MVP is ~60% complete with solid foundation but critical gaps remaining. ✅ Fully Implemented Phases:
|
test headingBody text |
Follow-up Review — feat(notifications): outbound webhook (#560)All four issues from my prior review are resolved. Here is a pass over the updated diff. Resolved1. SSRF validation — Dual-layer defence is solid: 2. CLI sync context — 3. 4. Timestamp format — Minor observations (non-blocking)Duplicated scheme set —
Thread-count assertion in OverallReady to merge. The security handling (scheme validation, log redaction), the correct dual async/sync dispatch path, and the test breadth across all three dispatch sites are all well done. No blocking issues. |
Follow-up Review — feat(notifications): outbound webhook (#560)All four issues from my prior review are resolved. Here is a pass over the updated diff. Resolved1. SSRF validation — Dual-layer defence is solid: 2. CLI sync context — 3. 4. Timestamp format — Minor observations (non-blocking)Duplicated scheme set —
Thread-count assertion in OverallReady to merge. The security handling (scheme validation, log redaction), the correct dual async/sync dispatch path, and the test breadth across all three dispatch sites are all well done. No blocking issues. |
|
test with frozenset: |
Follow-up Review — feat(notifications): outbound webhook (#560)All four issues from my prior review are resolved. Here is a pass over the updated diff. Resolved1. SSRF validation — Dual-layer defence is solid: 2. CLI sync context — 3. 4. Timestamp format — |
Follow-up Review — feat(notifications): outbound webhook (#560)All four issues from my prior review are resolved. Here is a pass over the updated diff. Resolved1. SSRF validation — Dual-layer defence is solid: 2. CLI sync context — 3. 4. Timestamp format — Minor observations (non-blocking)Duplicated scheme set —
Thread-count assertion in OverallReady to merge. The security handling (scheme validation, log redaction), the correct dual async/sync dispatch path, and the test breadth across all three dispatch sites are all well done. No blocking issues. |
Two more CodeRabbit findings on the latest commits: 1. **Major: basic-auth credentials leak through `_redact_url_for_log`**: ``parsed.netloc`` preserves ``user:password@host``, so a webhook URL like ``https://admin:secret@host/path`` was still logging ``admin:secret@host``. Switched to ``parsed.hostname`` (which strips auth and port) and re-attach the port explicitly. Added two tests: one verifies basic-auth credentials don't appear in logs, the other verifies the port is still kept (non-secret, useful for ops). 2. **Minor: cancelled task not awaited**: the `test_send_event_background_task_has_done_callback` cleanup called ``task.cancel()`` without awaiting, which can emit ``Task was destroyed but it is pending`` warnings on loop close. Now awaits the cancelled task to let cancellation propagate cleanly. Also added a comment to ``_ALLOWED_SCHEMES`` noting it is intentionally duplicated with ``settings_v2.py`` because core cannot import from the UI layer (architecture rule #1). 136/136 webhook tests pass.
Round 6 — basic-auth log leak + clean async cancel (commit 77459ef)CodeRabbit caught two more on the previous fix.
Also addressed Claude's nitpick about the duplicated Tests
|
Follow-up Review (Round 3) — feat(notifications): outbound webhook (#560)Reviewed the latest push (77459ef). This is a clean resolution of all outstanding items from the prior two rounds. Prior Issues — Confirmed ResolvedNon-dict JSON guard ✅ Defense-in-depth URL validation at load/send time ✅ Error response shape ✅ Log redaction ✅ (bonus addition) One Small ObservationIn Ship-Ready Assessment
Ready to merge. The |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@codeframe/core/notifications_config.py`:
- Around line 169-174: The _redact_url_for_log function currently accesses
parsed.port which can raise ValueError for malformed ports and bubble up into
is_webhook_active; modify _redact_url_for_log to guard access to parsed.port
(e.g., wrap reading parsed.port in a try/except ValueError or validate
parsed.netloc) and treat any invalid port as unparseable so the function returns
"<unparseable>" (or falls back to the safe string) instead of raising; ensure
you reference parsed.scheme and parsed.hostname as before and keep the existing
returned format when the port is valid.
In `@tests/core/test_notifications_config.py`:
- Line 154: The test functions like test_unsafe_url_log_does_not_leak_basic_auth
(and the other newly added test at the same region) are missing type
annotations; update their signatures to include parameter types (e.g.,
workspace: WorkspaceType, caplog: logging.LogCaptureFixture or appropriate types
used in tests) and an explicit return type of None. Locate the test functions by
the names test_unsafe_url_log_does_not_leak_basic_auth and the other test around
the same block, add parameter annotations for each parameter and append -> None
to the function definition to satisfy the project's typing guideline.
🪄 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: 0473157f-29ca-4446-bdb4-9cfd5fec0749
📒 Files selected for processing (3)
codeframe/core/notifications_config.pytests/core/test_notifications_config.pytests/notifications/test_webhook_send_event.py
…560) `urlparse(...).port` raises ``ValueError`` for non-numeric port strings (e.g. ``file://host:abc/x``). Without this guard, ``_redact_url_for_log`` would raise inside ``is_webhook_active``'s "fail-safe" branch, turning a clean refusal-to-dispatch into an exception caught only by the downstream dispatch-helper try/except. Now returns ``<unparseable>`` for both malformed schemes and malformed ports. Two new tests: - ``test_redact_url_for_log_handles_malformed_port`` — direct unit test - ``test_is_webhook_active_returns_none_for_malformed_port`` — verifies the integration path doesn't crash on a hand-edited URL with a bogus port.
Code Review — PR #594: Outbound Webhook NotificationsThis PR has gone through several CodeRabbit review rounds, and each iteration has clearly addressed the flagged issues. Here is my independent assessment of the current state. What is Working WellArchitecture compliance — Defense-in-depth security — URL validation runs at three independent layers: router PUT,
Atomic writes + credential redaction — Test coverage — 60+ tests spanning config, dispatch, endpoints, UI, and security edge cases is well above the bar for a feature of this scope. Remaining ConcernsFragile lambda repr assertion (CodeRabbit flagged in 4th review — still present) In assert "send_event_background.<locals>.<lambda>" in repr(captured_tasks[0])This couples the test to CPython's internal Task repr format, which could silently change across minor versions. The assertion exists to verify the done-callback is attached (preventing Python 3.11+ log noise), but a more stable check would be: # More stable: check _callbacks is non-empty
assert captured_tasks[0]._callbacks, "expected done-callback to be registered"
# At module top:
import logging
logger = logging.getLogger(__name__)
# Then in except:
except Exception:
logger.warning("Failed to dispatch blocker.created webhook...", exc_info=True)Legacy The old method at Minor Observations
SummaryThe implementation is solid and all critical issues from prior reviews have been addressed. The two items worth fixing before merge are the fragile lambda repr assertion and the missing module-level logger in |
Round 7 — parsed.port crash guard (commit 3279f17)CodeRabbit on the previous fix caught one real and one style finding.
Both threads resolved. Backend: 20/20 config tests pass (added 2 new tests for malformed-port handling). Lint clean. |
1 participant
Summary
Closes #560.
Adds outbound webhook notifications: a single user-configurable URL in Settings → Notifications that receives JSON payloads on three events:
batch.completed— only on fullBATCH_COMPLETED(not PARTIAL/FAILED/CANCELLED, per ticket scope)blocker.createdpr.merged— only on successful mergesThe Test button posts a sample payload and surfaces the HTTP status code via toast.
Implementation notes
.codeframe/notifications_config.jsonviaatomic_write_json, mirroring theworkspace_configpattern from [Phase 5.1] Settings page: PROOF9 defaults and workspace config #556. No SQLite migration — the Traycer plan'sworkspace_settingstable was unnecessary now that the JSON-on-disk convention is established.WebhookNotificationServicegains a genericsend_event(payload, url) → WebhookSendResultandsend_event_background(...)wrapper. The legacysend_blocker_notification(_background)API used bybackend_worker_agent.pyandfrontend_worker_agent.pyis preserved untouched.core/blockers.py:create(), fourBATCH_COMPLETEDemit sites incore/conductor.py, andui/routers/pr_v2.py:merge_pull_requestafter a successful merge. All wrapped intry/except— a misconfigured webhook can never break the triggering operation. 5s timeout, fire-and-forget.GET / PUT /api/v2/settings/notificationsandPOST /api/v2/settings/notifications/test, added to the existingsettings_v2.py(no new router file).NotificationsTab.tsxregistered as the 5th settings tab. URL input + enable toggle + Test button (disabled when URL is unsaved or empty). Save/Discard mirrorWorkspaceConfigTab.tsx.Test plan
tests/core/test_notifications_config.py— 9 tests: load/save roundtrip, defaults, corrupt JSON fallback,is_webhook_activesemanticstests/notifications/test_webhook_send_event.py— 13 tests: 2xx/5xx/timeout/ClientError/no-URL paths, URL override, background scheduling, payload formatterstests/ui/test_settings_notifications.py— 10 tests: GET defaults, PUT persist, PUT empty/whitespace, GET-after-PUT, POST /test (200/500/no-URL/disabled-flag)tests/core/test_blockers_webhook.py— 4 tests: no dispatch when disabled, dispatch when enabled, failure swallowedtests/core/test_conductor_webhook.py— 7 tests: fires only onBATCH_COMPLETED, not on PARTIAL/FAILED/CANCELLED, failure swallowedtests/ui/test_pr_v2_webhook.py— 4 tests: dispatch with full GitHub URL, fallback URL when env unconfigured, failure swallowedweb-ui/src/__tests__/components/settings/NotificationsTab.test.tsx— 12 tests: empty/loaded state, dirty-state Save/Test toggling, success/error toasts for each test-button outcome, Discard semanticsSettingsPage.test.tsxupdated for the new tabnpm run buildsucceedsKnown limitations
BLOCKER_WEBHOOK_URLenv var pattern.TODO: encrypt at restwill be a follow-up.Summary by CodeRabbit
New Features
Documentation
Tests