UN-3056 [FEAT] Filter notifications by run outcome#1936
UN-3056 [FEAT] Filter notifications by run outcome#1936kirtimanmishrazipstack wants to merge 36 commits into
Conversation
…down rendering (#1927) * [FIX] Make tool-run logs visible in workflow execution UI Two stacked gaps were keeping tool-level log lines (Processing prompt, Running LLM completion, lookup calls, etc.) out of the workflow execution logs UI and the execution_log DB table for API / workflow runs: 1. Empty log_events_id. structure_tool_task seeded LOG_EVENTS_ID in StateStore but never threaded it into pipeline_ctx / agentic_ctx. ExecutorToolShim.stream_log gated publishing on self.log_events_id, so every tool-level log was dropped before it ever reached the broker. 2. Wrong payload shape. Even with the channel threaded, stream_log used LogPublisher.log_progress(...) whose payload omits execution_id / organization_id / file_execution_id. get_validated_log_data (log_utils.py) requires those IDs and LogType == LOG to persist to execution_log, so tool-level messages were silently filtered at the Redis->DB drain step — orchestration logs persisted, tool logs did not. Fixes: - ExecutionContext gains execution_id + file_execution_id, populated in structure_tool_task for both the legacy pipeline and agentic contexts. - LegacyExecutor caches the three IDs on self during execute() and passes them into every ExecutorToolShim construction (~7 callsites). - ExecutorToolShim.stream_log now dual-emits: PROGRESS (unchanged, drives the IDE prompt-card live progress pane) plus LOG carrying the workflow IDs (feeds the workflow execution logs UI and persists to execution_log via the existing drain). LOG emission is gated on execution_id + organization_id being present, so bare IDE test runs without a workflow still behave as before. Rendering polish - The LogModal and pipeline LogsModal now pipe log text through the existing CustomMarkdown renderer, so backticked identifiers render as inline-code pills and embedded newlines break lines. This lets multi-line structured events (e.g. the lookup pre-call trio) surface as a single row with readable inner formatting. - Prompt-key mentions inside legacy_executor tool logs are wrapped in backticks for consistency with the rest of the log surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [FIX] Wrap prompt_name in backticks in remaining stream_log calls Completes the consistency pass on tool-run log formatting: the table- and line-item-extraction success and error paths still emitted prompt names without backticks, so the markdown-rendered logs UI showed them as bare text instead of inline-code pills. Matches the pattern already applied to the other 9 stream_log calls in this file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [FIX] Validate URL schemes in CustomMarkdown link renderer Workflow logs rendered via CustomMarkdown can contain tool-generated or user-derived content, so an untrusted \`[text](url)\` sequence could inject a \`javascript:\` or \`data:\` scheme and get clickable through antd \`Typography.Link\`. Allow-list the safe external schemes (http, https, mailto, tel) before rendering as a link; everything else falls back to plain text while still honouring the existing internal-path branch used for in-app navigation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [FIX] Thread workflow IDs into remaining shim/context callsites Addresses CodeRabbit review gaps so the log-plumbing fix is consistent across every pre-dispatch and plugin-dispatch path: - `table_ctx` / `line_item_ctx` in `legacy_executor.py` now carry `log_events_id`, `execution_id`, `file_execution_id` from context so downstream table/line-item plugins that build their own `ExecutorToolShim` pass the `execution_id + organization_id` gate and emit workflow LOG payloads. - `structure_tool_task.py` threads the same IDs into the bare pre-dispatch shim, so `X2Text.process()` calls during agentic extraction reach the workflow logs UI. - `LogsModal.jsx` stores the raw log string in row data and lets the column renderer wrap it in `CustomMarkdown` — the previous map stored a `<CustomMarkdown />` element that was then passed back into `CustomMarkdown.text`, producing `[object Object]` for multi-row lookups. - Dropped `getattr(context, ...)` on `execution_id` / `file_execution_id` now that they are dataclass fields — matches the direct access used for `organization_id`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [REFACTOR] Trim overly specific comments in log-plumbing changes Pass through the new comments added across this PR and either remove or tighten the ones that restate what the code already shows. Keep only the WHY lines that protect future readers from missing a non-obvious constraint (XSS guard in CustomMarkdown, dual PROGRESS/LOG emission in the shim, pre-dispatch shim needing workflow IDs so X2Text logs are not silently dropped). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [REFACTOR] Extract isSafeExternalUrl into shared helpers module Moves the URL scheme allow-list check out of CustomMarkdown into helpers/urlSafety.js so any future component that renders links from user- or tool-derived content can reuse the same guard instead of re-implementing it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [FIX] Tighten URL guard, split publish try/excepts, and extract shim builder Addresses the must-fix and worth-doing comments from the PR review: Security - CustomMarkdown: treat protocol-relative URLs (`//host/...`) as external, not internal, so they can no longer skip the scheme guard via the `startsWith("/")` branch. - `isSafeExternalUrl`: drop the `window.location.origin` base so bare strings ("javascript", "../foo") fail to parse instead of silently resolving to `https://<origin>/...` and passing the scheme check. Silent failure + comment accuracy - ExecutorToolShim.stream_log: split the PROGRESS and LOG publish paths into separate try/except blocks so a LogDataDTO validation failure on the LOG payload is no longer mis-attributed to "progress publish failed". Corrected the inline comments — the DB drop is driven by LogPublisher's `payload.type == 'LOG'` check, and only `execution_id` + `organization_id` are strictly required. Refactor - New `LegacyExecutor._build_shim()` helper — all seven ExecutorToolShim callsites now share one construction path so the workflow-ID plumbing can't drift out of sync across sites again. - Thread `execution_id` / `file_execution_id` into the seven self-dispatched sub-`ExecutionContext`s alongside `log_events_id`, matching the table/line-item sites and keeping the context consistent for any downstream consumer that reads the IDs from the context rather than from the executor instance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [FIX] Address remaining type-design and silent-failure comments - ExecutionContext: drop the BE-coupled inline comment, document the new IDs in the Attributes block, and enforce the invariant that execution_id implies organization_id via __post_init__. - ExecutorToolShim: typed the three new IDs as str | None instead of str = "" so the signature matches the Optional semantics already enforced by the runtime guards. - LegacyExecutor: move per-request state to __init__ so _log_component is no longer a class-level mutable default shared across instances; stop silently coercing None IDs to ""; add a one-shot warning when a tool-sourced run lands without workflow IDs so the silent-no-persist case is visible in GKE logs. - structure_tool_task: emit the same warning when LOG_EVENTS_ID is absent from StateStore. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [FIX] Surface first publish failure per shim at WARN Both PROGRESS and LOG publish paths previously swallowed every broker failure at DEBUG, so a misconfigured or down Redis broker meant every tool-level log silently vanished with no operator-visible signal. Track a per-shim _progress_publish_failed / _log_publish_failed flag and log the first failure at WARNING (with traceback), then downgrade subsequent failures on the same shim back to DEBUG. Preserves the non-fatal semantics of the publish path while making broker outages visible in GKE logs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* [FIX] Auto-bump modified_at on QuerySet.update() and bulk_update() Django's auto_now=True only fires on Model.save(); QuerySet.update() and bulk_update() bypass save(), so BaseModel.modified_at silently stayed at the creation time for every bulk-path write. Audit trail drifted. Introduce BaseModelQuerySet that injects modified_at=timezone.now() into both paths, and expose it via BaseModelManager. Migrate all custom managers on BaseModel subclasses to compose BaseModelManager so their querysets inherit the overrides. Drop the ad-hoc modified_at=now() kwarg in FileHistoryHelper now that the queryset handles it. * [FIX] Materialize objs in BaseModelQuerySet.bulk_update to support generators Addresses PR review: if callers pass a non-rewindable iterable (generator, queryset iterator), the modified_at stamping loop would exhaust it before super().bulk_update() saw it, silently updating zero rows. list(objs) up front keeps generator callers working. Also drop the mock-based unit test — it needed django.setup() at module import which isn't viable without pytest-django, and proper DB-backed coverage is tracked separately. * [FIX] Auto-inject modified_at into BaseModel.save(update_fields=...) Django only runs auto_now for fields listed in update_fields, so every save(update_fields=["foo"]) on a BaseModel subclass silently drops the modified_at bump — same family of bug as QuerySet.update/bulk_update. Override BaseModel.save() to add modified_at to update_fields whenever the caller supplies a restricted list without it. Also drop two dead manual-assignment lines (execution.modified_at = timezone.now() before save()) that were redundant with auto_now on a full save(). * [FIX] Auto-bump modified_at on upsert bulk_create and drop workarounds QuerySet.bulk_create(update_conflicts=True, update_fields=[...]) runs an UPDATE on conflict with only the listed fields — same auto_now-bypass as save(update_fields=...) and QuerySet.update(). Patch BaseModelQuerySet's bulk_create to inject modified_at into update_fields on upsert. With that in place, the explicit "modified_at" entries in dashboard_metrics upsert callers are redundant. Drop them. * [REFACTOR] Tighten BaseModel auto-bump helpers and edge cases - Extract `_with_modified_at` helper; single source of truth for the "inject modified_at into a partial field list" rule across `bulk_update`, `bulk_create` and `BaseModel.save`. - Preserve Django's documented `save(update_fields=[])` no-op (signals-only save, no column writes) instead of rewriting it to `["modified_at"]`. Apply the same guard to `bulk_create(update_conflicts=True, update_fields=[])`. - Match Django's positional `save()` signature (`force_insert`, `force_update`, `using`, `update_fields`) so callers passing flags positionally still hit the auto-bump override. - Skip the per-obj `modified_at` stamp + `objs` materialization in `bulk_update` when the caller already listed `modified_at` — lets the opt-in path stay O(1) before the `super()` delegation. - Docstring corrections: "previous save() timestamp" (not just creation time); manager-level convention note; precise `auto_now` semantics (attribute still updates in-memory, just isn't persisted without `update_fields` inclusion).
…wered table extraction (#1914) * Execution backend - revamp * async flow * Streaming progress to FE * Removing multi hop in Prompt studio ide and structure tool * UN-3234 [FIX] Add beta tag to agentic prompt studio navigation item * Added executors for agentic prompt studio * Added executors for agentic prompt studio * Removed redundant envs * Removed redundant envs * Removed redundant envs * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Removed redundant envs * Removed redundant envs * Removed redundant envs * Removed redundant envs * Removed redundant envs * Removed redundant envs * Removed redundant envs * Removed redundant envs * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Removed redundant envs * adding worker for callbacks * adding worker for callbacks * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * adding worker for callbacks * adding worker for callbacks * adding worker for callbacks * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Pluggable apps and plugins to fit the new async prompt execution architecture * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Pluggable apps and plugins to fit the new async prompt execution architecture * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Pluggable apps and plugins to fit the new async prompt execution architecture * adding worker for callbacks * adding worker for callbacks * adding worker for callbacks * adding worker for callbacks * adding worker for callbacks * adding worker for callbacks * adding worker for callbacks * adding worker for callbacks * fix: write output files in agentic extraction pipeline Agentic extraction returned early without writing INFILE (JSON) or METADATA.json, causing destination connectors to read the original PDF and fail with "Expected tool output type: TXT, got: application/pdf". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * UN-3266 fix: replace hardcoded /tmp paths with secure temp dirs in tests (#1850) * UN-3266 fix: replace hardcoded /tmp paths with secure temp dirs in tests Replace hardcoded /tmp/ paths (SonarCloud S5443 security hotspots) with pytest's tmp_path fixture or module-level tempfile.mkdtemp() constants in all affected test files to avoid world-writable directory vulnerabilities. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update docs * UN-3266 fix: remove dead code with undefined names in fetch_response Remove unreachable code block after the async callback return in fetch_response that still referenced output_count_before and response from the old synchronous implementation, causing ruff F821 errors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Un 3266 fix security hotspot tmp paths (#1851) * UN-3266 fix: replace hardcoded /tmp paths with secure temp dirs in tests Replace hardcoded /tmp/ paths (SonarCloud S5443 security hotspots) with pytest's tmp_path fixture or module-level tempfile.mkdtemp() constants in all affected test files to avoid world-writable directory vulnerabilities. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * UN-3266 fix: resolve ruff linting failures across multiple files - B026: pass url positionally in worker_celery.py to avoid star-arg after keyword - N803: rename MockAsyncResult to mock_async_result in test_tasks.py - E501/I001: fix long line and import sort in llm_whisperer helper - ANN401: replace Any with object|None in dispatcher.py; add noqa in test helpers - F841: remove unused workflow_id and result assignments Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * UN-3266 fix: resolve SonarCloud bugs S2259 and S1244 in PR #1849 - S2259: guard against None after _discover_plugins() in loader.py to satisfy static analysis on the dict[str,type]|None field type - S1244: replace float equality checks with pytest.approx() in test_answer_prompt.py and test_phase2h.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * UN-3266 fix: resolve SonarCloud code smells in PR #1849 - S5799: Merge all implicit string concatenations in log messages (legacy_executor.py, tasks.py, dispatcher.py, orchestrator.py, registry.py, variable_replacement.py, structure_tool_task.py) - S1192: Extract duplicate literal to _NO_CELERY_APP_MSG constant in dispatcher.py - S1871: Merge identical elif/else branches in tasks.py and test_sanity_phase6j.py - S1186: Add comment to empty stub method in test_sanity_phase6a.py - S1481: Remove unused local variables in test_sanity_phase6d/e/f/g/h/j and test_phase5d.py - S117: Rename PascalCase local variables to snake_case in test_sanity_phase3/5/6i.py - S5655: Broaden tool type annotation to StreamMixin in IndexingUtils.generate_index_key and PlatformHelper.get_adapter_config - docker:S7031: Merge consecutive RUN instructions in worker-unified.Dockerfile - javascript:S1128: Remove unused pollForCompletion import in usePromptRun.js Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * UN-3266 fix: wrap long log message in dispatcher.py to fix E501 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * UN-3266 fix: resolve remaining SonarCloud S117 naming violations Rename PascalCase local variables to snake_case to comply with S117: - legacy_executor.py: rename tuple-unpacked _get_prompt_deps() results (AnswerPromptService→answer_prompt_svc, RetrievalService→retrieval_svc, VariableReplacementService→variable_replacement_svc, LLM→llm_cls, EmbeddingCompat→embedding_compat_cls, VectorDB→vector_db_cls) and update all downstream usages including _apply_type_conversion and _handle_summarize - test_phase1_log_streaming.py: rename Mock* local variables to mock_* snake_case equivalents - test_sanity_phase3.py: rename MockDispatcher→mock_dispatcher_cls and MockShim→mock_shim_cls across all 10 test methods - test_sanity_phase5.py: rename MockShim→mock_shim, MockX2Text→mock_x2text in 6 test methods; MockDispatcher→mock_dispatcher_cls in dispatch test; fix LLM_cls→llm_cls, EmbeddingCompat→embedding_compat_cls, VectorDB→vector_db_cls in _mock_prompt_deps helper Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * UN-3266 fix: resolve remaining SonarCloud code smells in PR #1849 - test_sanity_phase2/4.py, test_answer_prompt.py: rename PascalCase local variables in _mock_prompt_deps/_mock_deps to snake_case (RetrievalService→retrieval_svc, VariableReplacementService→ variable_replacement_svc, Index→index_cls, LLM_cls→llm_cls, EmbeddingCompat→embedding_compat_cls, VectorDB→vector_db_cls, AnswerPromptService→answer_prompt_svc_cls) — fixes S117 - test_sanity_phase3.py: remove unused local variable "result" — fixes S1481 - structure_tool_task.py: remove redundant json.JSONDecodeError from except clause (subclass of ValueError) — fixes S5713 - shared/workflow/execution/service.py: replace generic Exception with RuntimeError for structure tool failure — fixes S112 - run-worker-docker.sh: define EXECUTOR_WORKER_TYPE constant and replace 10 literal "executor" occurrences — fixes S1192 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * UN-3266 fix: resolve SonarCloud cognitive complexity and code smell violations - Reduce cognitive complexity in answer_prompt.py: - Extract _build_grammar_notes, _run_webhook_postprocess helpers - _is_safe_public_url: extracted _resolve_host_addresses helper - handle_json: early-return pattern eliminates nesting - construct_prompt: delegates grammar loop to _build_grammar_notes - Reduce cognitive complexity in legacy_executor.py: - Extract _execute_single_prompt, _run_table_extraction helpers - Extract _run_challenge_if_enabled, _run_evaluation_if_enabled - Extract _inject_table_settings, _finalize_pipeline_result - Extract _convert_number_answer, _convert_scalar_answer - Extract _sanitize_dict_values helper - _handle_answer_prompt CC reduced from 50 to ~7 - Reduce CC in structure_tool_task.py: guard-clause refactor - Reduce CC in backend: dto.py, deployment_helper.py, api_deployment_views.py, prompt_studio_helper.py - Fix S117: rename PascalCase local vars in test_answer_prompt.py - Fix S1192: extract EXECUTOR_WORKER_TYPE constant in run-worker.sh - Fix S1172: remove unused params from structure_tool_task.py - Fix S5713: remove redundant JSONDecodeError in json_repair_helper.py - Fix S112/S5727 in test_execution.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * UN-3266 fix: remove unused RetrievalStrategy import from _handle_answer_prompt Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * UN-3266 fix: rename UsageHelper params to lowercase (N803) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * UN-3266 fix: resolve remaining SonarCloud issues from check run 66691002192 - Add @staticmethod to _sanitize_null_values (fixes S2325 missing self) - Reduce _execute_single_prompt params from 25 to 11 (S107) by grouping services as deps tuple and extracting exec params from context.executor_params - Add NOSONAR suppression for raise exc in test helper (S112) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * UN-3266 fix: remove unused locals in _handle_answer_prompt (F841) execution_id, file_hash, log_events_id, custom_data are now extracted inside _execute_single_prompt from context.executor_params. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: resolve Biome linting errors in frontend source files Auto-fixed 48 lint errors across 56 files: import ordering, block statements, unused variable prefixing, and formatting issues. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: replace dynamic import of SharePermission with static import in Workflows Resolves vite build warning about SharePermission.jsx being both dynamically and statically imported across the codebase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: resolve SonarCloud warnings in frontend components - Remove unnecessary try-catch around PostHog event calls - Flip negated condition in PromptOutput.handleTable for clarity Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Address PR #1849 review comments: fix null guards, dead code, and test drift - Remove redundant inline `import uuid as _uuid` in views.py (use module-level uuid) - URL-encode DB_USER in worker_celery.py result backend connection string - Remove misleading task_queues=[Queue("executor")] from dispatch-only Celery app - Remove dead `if not tool:` guards after objects.get() (already raises DoesNotExist) - Move profile_manager/default_profile null checks before first dereference - Reorder ProfileManager.objects.get before mark_document_indexed in tasks.py - Handle ProfileManager.DoesNotExist as warning, not hard failure - Wrap PostHog analytics in try/catch so failures don't block prompt execution - Handle pending-indexing 200 response in usePromptRun.js (clear RUNNING status) - Reset formData when metadata is missing in ConfigureDs.jsx - Fix test_should_skip_extraction tests: function now takes 1 arg (outputs only) - Fix agentic routing tests: mock X2Text.process, remove stale platform_helper kwarg Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix missing llm_usage_reason for summarize LLM usage tracking Add PSKeys.LLM_USAGE_REASON to usage_kwargs in _handle_summarize() so summarization costs appear under summarize_llm in API response metadata. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * UN-3266 [FIX] Fix single-pass extraction routing in LegacyExecutor - Route _handle_structure_pipeline to _handle_single_pass_extraction when is_single_pass=True (was always calling _handle_answer_prompt) - Delegate _handle_single_pass_extraction to cloud plugin via ExecutorRegistry, falling back to _handle_answer_prompt if plugin not installed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fixing API depployment response mismatches * Add complete_vision() method to SDK1 LLM for multimodal completions Adds a new complete_vision() method alongside existing complete() that accepts pre-built multimodal messages (text + image_url) in OpenAI-style format. LiteLLM auto-translates for Anthropic/Bedrock/Vertex providers. This enables the agentic table extractor plugin to send page images alongside text prompts for VLM-based table detection and extraction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * UN-3266 [FIX] Gate Run button by agentic table readiness checklist - PromptCardItems loads AgenticTableChecklist plugin and owns the isAgenticTableReady state, rendering the checklist above the prompt text area and delegating the settings gear visibility to the plugin. - Header and PromptOutput disable their Run buttons when isAgenticTableReady is false (default true for non-agentic types). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * [FIX] Use correct primary key field in prompt count subquery (#1905) ToolStudioPrompt uses prompt_id as its primary key, not id. Count("id") causes FieldError on the list endpoint (500). Co-authored-by: Chandrasekharan M <chandrasekharan@zipstack.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * [FIX] Add agentic_table as valid enforce_type choice The cloud build adds "agentic_table" to the prompt enforce_type dropdown, but the OSS ToolStudioPrompt model rejected it as an invalid choice. Add AGENTIC_TABLE to EnforceType and ship a matching migration so the value can be persisted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * UN-3266 [FIX] Wire agentic_table enforce_type to executor dispatch The single-prompt run flow had no branch for prompts with enforce_type=agentic_table, so clicking Run silently fell through to the legacy prompt-service path and never invoked the agentic_table executor. Adds an AGENTIC_TABLE constant to TSPKeys, includes it in the OperationNotSupported guard, and dispatches to PayloadModifier.execute_agentic_table when the plugin is available so the result still flows through _handle_response. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * UN-3266 [FIX] Add agentic_table queue to executor worker defaults The ExecutionDispatcher derives the queue name from the executor name (celery_executor_{name}), so dispatches to the agentic_table executor land on celery_executor_agentic_table. The local docker-compose default only listed celery_executor_legacy and celery_executor_agentic, so no worker consumed the new queue and dispatch hung for the full 1-hour result timeout. Adds the missing queue to the docker-compose default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * UN-3266 [FIX] Dispatch agentic_table prompts to executor on IDE Run The IDE Run button was building a legacy answer_prompt payload for agentic_table prompts, so the agentic table executor was never invoked. Branch fetch_response on enforce_type so agentic_table prompts are built via the cloud payload_modifier plugin and dispatched directly to celery_executor_agentic_table. Add the enforce_type to the OSS dropdown choices and the JSON-dump set in OutputManagerHelper so the persisted output is parseable by the FE table renderer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * UN-3266 [FIX] Reshape agentic_table executor output in IDE callback The agentic_table executor returns {"output": {"tables": [...], "page_count": ..., "headers": [...], ...}}, but OutputManagerHelper.handle_prompt_output_update reads outputs[prompt.prompt_key] when persisting prompt output. Without a reshape the table list never lands under the prompt key and the FE sees an empty result. When cb_kwargs carries is_agentic_table=True and prompt_key (set by the cloud build_agentic_table_payload), reshape outputs to {prompt_key: tables} before calling update_prompt_output. The executor itself also shapes its envelope, so this is a defensive double-keying that keeps the legacy answer_prompt path untouched. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fixing timeout issues * API deployment fixes for Agentic table extractor * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fixing syntax issues * Fix agentic_table executor reading INFILE after JSON overwrite Read from SOURCE instead of INFILE when dispatching to the agentic_table executor. INFILE gets overwritten with JSON output by the regular pipeline, causing PDFium parse errors when the agentic_table executor tries to process it as a PDF. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Signed-off-by: harini-venkataraman <115449948+harini-venkataraman@users.noreply.github.com> Co-authored-by: Ghost Jake <89829542+Deepak-Kesavan@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Ritwik G <100672805+ritwik-g@users.noreply.github.com> Co-authored-by: Chandrasekharan M <chandrasekharan@zipstack.com>
…fy-on-API-deployment-failures
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughImplements clubbed (buffered) notification delivery with org-configurable batch intervals, adds delivery modes and failure-only routing, persists per-execution file counts, introduces NotificationBuffer model/endpoints and enqueue/dispatch helpers, normalizes clubbed webhook envelopes and Slack rendering, and wires worker/frontend to use the new flow. ChangesCore models, enums and buffering
Buffer enqueue, dispatch and helper utilities
Clubbed envelope rendering and provider changes
Notification dispatch flow and payload enrichment
Execution file-count denormalization and status updates
Worker routing and scheduling changes
Frontend: settings and notification UI
Sequence DiagramssequenceDiagram
participant Producer as Notifier Source
participant Backend as Backend API
participant BufferAPI as Buffer Enqueue API
participant DB as NotificationBuffer
participant Scheduler as Log Consumer / Scheduler
participant Celery as Celery Worker
participant Webhook as External Webhook
Producer->>Backend: Execution completes / trigger notification
Backend->>Backend: Load execution, compute file counts
Backend->>Backend: Filter notifications (notify_on_failures logic)
Backend->>BufferAPI: POST enqueue_notification_buffer (payload + counts)
BufferAPI->>DB: Persist NotificationBuffer row (PENDING, flush_after)
Scheduler->>DB: process_notification_buffer (groups due flushes)
DB->>DB: Claim rows FOR UPDATE SKIP LOCKED
DB->>DB: build_envelope() / render_clubbed_message()
DB->>DB: Mark rows DISPATCHED
DB->>Celery: Enqueue single clubbed send task (with buffer ids)
Celery->>Webhook: POST clubbed payload
Webhook-->>Celery: 200 OK
Celery->>DB: Success -> leave DISPATCHED / failure -> revert to PENDING or mark DEAD_LETTER
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
…fy-on-API-deployment-failures
* UN-3439 [FIX] Accept wildcard subdomain origins in SocketIO and Django CORS (#1938) * UN-3439 [FIX] Accept wildcard subdomain origins in SocketIO and Django CORS Production socket connections were failing for `*.env.us-central.unstract.com` because python-socketio does exact-string comparison on `cors_allowed_origins`, so a literal `*` pattern silently rejected every real subdomain. - Add `CORS_ALLOWED_ORIGIN_REGEXES` derived from `WEB_APP_ORIGIN_URL_WITH_WILD_CARD`. - Wire SocketIO via `_RegexOrigin` whose `__eq__` does the regex match — single list entry covers all wildcard subdomains, no library subclass needed. - Normalize `WEB_APP_ORIGIN_URL` through `urlparse` so trailing slashes / paths in env are stripped (also fixes the `…com//oauth-status/` double-slash). - Add startup guard for malformed env values. Resolves item #1 of UN-3439. Items #2/#3 (decoupling indexing from Socket.io, fallback) are owned separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * UN-3439 [FIX] Address PR review: canonical origin, fullmatch, unhashable RegexOrigin, tests Addresses five review comments on #1938: 1. coderabbitai (Major) — RFC 6454 canonicalization. Browsers serialize `Origin` headers with a lowercase host and no explicit default ports; `parsed_url.netloc` preserved both, so `https://APP.EXAMPLE.COM:443` would silently fail to match the browser's `https://app.example.com`. Switch to `parsed_url.hostname` + drop default ports, and reject non-http(s) schemes at startup. 2. greptile (P2) — `re.fullmatch` instead of `re.match`. With `re.match` plus `$`, a candidate ending in `\n` matches because `$` is allowed before an optional trailing newline. `fullmatch` removes the ambiguity. 3. self — `_RegexOrigin.__hash__` violated `a == b ⇒ hash(a) == hash(b)` (one fixed pattern hash vs. many matching strings). Today this is masked because python-socketio uses linear `__eq__` on a list, but if the allow-list is ever wrapped in a set, every legitimate subdomain would silently be rejected — exactly the failure mode UN-3439 closes. Make instances unhashable so the contract can't be broken. 4. self — No regression tests. Add `backend/utils/tests/test_cors_origin.py` (33 cases) covering: regex match/no-match, lookalike spoofing, scheme mismatch, trailing-newline rejection, non-string equality protocol, unhashability, ReDoS bounds, URL normalization (case, default ports, trailing slash, paths, queries), startup-guard rejections (empty, no-scheme, non-browser-scheme, no-host), and end-to-end via the same `RegexOrigin` path SocketIO uses. 5. self — Over-clever wildcard-to-regex builder. The `split('*').join(re.escape, ...)` construction generalised to N wildcards but the input has exactly one; replace with a direct rf-string that's self-evident on review. Refactor for testability: extract `RegexOrigin` and `normalize_web_app_origin` into `backend/utils/cors_origin.py` (Django-free, importable from settings and tests). Settings now delegates to one helper call; `log_events.py` imports `RegexOrigin`. No behavioural change beyond what each comment fixes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * UN-3439 [FIX] Address SonarCloud quality gate The Sonar quality gate failed with C reliability + 5 security hotspots, all on the new test file: - S905 (Bug, Major) — `{ro}` flagged as no-side-effect statement (Sonar doesn't see the implicit `__hash__` call). Drove the C reliability rating. Fix: use `len({ro})` so the side effect is via an explicit function call; test still asserts the same `TypeError`. - S5727 (Code Smell, Critical) — `assert ro != None` is tautological and doesn't exercise `__eq__`. Switch to `(ro == None) is False` which directly tests that `NotImplemented` falls back to identity-equality. - S5332 × 5 (Hotspots) — `http://` and `ftp://` literals in test data. These are intentional inputs proving the rejection logic. Annotate with `# NOSONAR` and an explanatory comment so the hotspots can be marked reviewed. No production code changed; tests still 33/33 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * UN-3439 [FIX] Remove last S5727 code smell — test __eq__ via dunder Sonar S5727 correctly inferred that ``ro == None`` is statically always False (NotImplemented falls back to identity), making the assertion look tautological. The intent is to lock the protocol contract: ``__eq__`` must return the ``NotImplemented`` sentinel for non-strings. Test that directly via ``ro.__eq__(None) is NotImplemented`` instead of going through ``==``. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * UN-3439 [FIX] Address remaining CodeRabbit nits — port validation, ReDoS bound Two minor follow-ups from the second CodeRabbit pass: - `parsed.port` is a property that raises ValueError on malformed/out-of-range inputs (e.g. `:abc`, `:99999`). That bypassed our normalized config-error message and surfaced as a stack trace. Wrap the access and re-raise with the same actionable text. Adds two test cases (`https://example.com:abc`, `https://example.com:99999`) to lock the new behaviour. - The 50ms ReDoS timing bound is too tight for noisy CI runners. Loosen to 500ms — still orders of magnitude below what catastrophic backtracking would produce. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ReverseMerge: V0.161.4 hotfix (#1943) * Change csp to report only * [HOTFIX] Bool-parse ENABLE_HIGHLIGHT_API_DEPLOYMENT env var (v0.161.4) (#1939) [HOTFIX] Bool-parse ENABLE_HIGHLIGHT_API_DEPLOYMENT env var (#1937) [FIX] Bool-parse ENABLE_HIGHLIGHT_API_DEPLOYMENT env var os.environ.get returns the raw string when the variable is set, so ENABLE_HIGHLIGHT_API_DEPLOYMENT="False" was truthy in Python (any non-empty string is truthy). Wrap in CommonUtils.str_to_bool so "False" / "false" / "0" actually evaluate to False. The setting is consumed by the cloud configuration plugin's spec default (ConfigSpec.default in plugins/configuration/cloud_config.py) on cloud and on-prem builds. With this fix, an admin who explicitly sets the env var to a falsy string sees highlight data stripped as expected. Co-authored-by: vishnuszipstack <117254672+vishnuszipstack@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Deepak K <89829542+Deepak-Kesavan@users.noreply.github.com> Co-authored-by: vishnuszipstack <117254672+vishnuszipstack@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * UN-3448 [FIX] Remove vestigial `uv pip install` line in uv-lock-automation workflow (#1941) * UN-3448 [FIX] Add --system flag to uv pip install in uv-lock-automation workflow Modern uv requires uv pip install to run inside a virtual environment OR with the explicit --system flag. The workflow currently has neither, so it errors out: error: No virtual environment found for Python 3.12.9; run `uv venv` to create an environment, or pass `--system` to install into a non-virtual environment This breaks every PR that touches a pyproject.toml (the workflow's paths filter triggers on those). Last successful run was 2026-04-01, before a behaviour change in uv or astral-sh/setup-uv@v7. The --system flag is exactly what the error message suggests and is correct here — we install pip into the runner's system Python; the downstream uv-lock.sh script creates its own venvs as needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * UN-3448 [FIX] Remove vestigial `uv pip install` line per review Per @jaseemjaskp's review: the pre-step `uv pip install ... pip` does nothing useful for this workflow. The downstream uv-lock.sh script uses uv sync at line 74, which manages its own venvs internally and never invokes pip directly: $ grep -rn 'pip' docker/scripts/uv-lock-gen/ docker/scripts/uv-lock-gen/uv-lock.sh:2:set -o pipefail Only match is pipefail (shell option), no real pip references. Removing the line entirely is cleaner than papering over with --system. The line was likely copy-pasted from a sibling workflow that legitimately needed pip in the system Python. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ReverseMerge: V0.163.2 hotfix (#1946) * [HOTFIX] Use importlib.util.find_spec for pluggable worker discovery (#1918) * [FIX] Use importlib.util.find_spec for pluggable worker discovery _verify_pluggable_worker_exists() previously checked for the literal file `pluggable_worker/<name>/worker.py` on disk, which breaks when the plugin has been compiled to a .so (Nuitka, Cython, or any C extension) — the module is perfectly importable but the pre-check rejects it because only the .py extension is considered. Replace the filesystem check with importlib.util.find_spec(), which is Python's standard way to ask "is this module resolvable by the import system?". It honors every registered finder — source .py, compiled .so, bytecode .pyc, namespace packages, zipimports — so the function now matches what its docstring claims: verifying the module can be loaded, not that a specific file extension is present. Behavior is preserved for existing deployments: - Images with no `pluggable_worker/<name>/` subpackage → find_spec raises ModuleNotFoundError (ImportError subclass) → returns False. - Images with source .py → find_spec resolves the .py → returns True. - Images with compiled .so → find_spec resolves the .so → returns True. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [FIX] Handle ValueError from find_spec in pluggable worker verification Greptile-flagged edge case: importlib.util.find_spec() can raise ValueError (not just ImportError) when sys.modules has a partially initialised module entry with __spec__ = None from a prior failed import. Broaden the except to catch both. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [FIX] Resolve api-deployment worker directory from enum import path worker.py:452 did worker_type.value.replace("-", "_") to derive the on-disk dir name. All WorkerType enum values already use underscores, so the replace was a no-op; for API_DEPLOYMENT whose dir is "api-deployment" (hyphen), it resolved to "api_deployment" and the os.path.exists() check failed. Boot then logged a spurious "❌ Worker directory not found: /app/api_deployment" at ERROR level. The task registration path (builder + celery autodiscover via to_import_path) is unaffected, so this was purely log noise — but noise at ERROR level that masks real failures in log scans. Fix: derive the directory from the authoritative to_import_path() which already handles the hyphen case (api_deployment -> api-deployment). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [HOTFIX] Add IAM Role / Instance Profile auth mode to AWS Bedrock adapter (#1944) * [FEAT] Allow Bedrock to fall through to boto3's default credential chain Match the S3/MinIO connector pattern: when AWS access keys are left blank on the Bedrock LLM and embedding adapter forms, drop them from the kwargs dict so boto3's default credential chain handles authentication. This unlocks IAM role / instance profile / IRSA / AWS Profile scenarios on hosts that already have ambient AWS credentials (e.g. EKS workers with IRSA, EC2 with an instance profile). - llm1/static/bedrock.json: clarify access-key descriptions to mention IRSA and instance profile (already non-required at v0.163.2 base). - embedding1/static/bedrock.json: drop aws_access_key_id and aws_secret_access_key from top-level required; same description fix; expose aws_profile_name for parity with the LLM form. - base1.py: AWSBedrockLLMParameters and AWSBedrockEmbeddingParameters now strip empty access-key values from the validated kwargs before returning, so empty strings don't override boto3's default chain. AWSBedrockEmbeddingParameters fields gain explicit None defaults and an aws_profile_name field. Backward-compatible: existing adapters with access keys filled in continue to work unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [FEAT] Add Authentication Type selector to Bedrock adapter form Add an explicit `auth_type` selector with two options, making the auth choice clear to users: - "Access Keys" (default): existing flow, keys required - "IAM Role / Instance Profile (on-prem AWS only)": no fields; relies on boto3's default credential chain (IRSA on EKS, task role on ECS, instance profile on EC2). Description on the selector explicitly notes this option is only for AWS-hosted Unstract deployments. The form-only auth_type field is stripped before LiteLLM validation in both AWSBedrockLLMParameters.validate() and AWSBedrockEmbeddingParameters. validate(). Empty access keys continue to be stripped so boto3 falls through to the default chain even when the access_keys arm is selected without values (matches the S3/MinIO connector pattern). Backward-compatible: legacy adapters without auth_type behave as "Access Keys" mode (the default), and existing keys are forwarded unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [REVIEW] Address Bedrock auth_type review feedback Fixes the P0/P1 issues raised by greptile-apps and jaseemjaskp on PR #1944. Behaviour fixes: - Stale-key leak in IAM Role mode: switching an existing adapter from Access Keys to IAM Role would carry truthy stored access keys through the strip-empty-only loop, so boto3 silently authenticated with the old long-lived credentials instead of falling through to the host's IRSA / instance-profile identity. Both LLM and embedding paths were affected. - Silent acceptance of unknown auth_type: a typo (e.g. "access_key") or a malformed payload from a non-UI client passed through the dict comprehension untouched, with no enum guard. - Cross-field validation gap: explicit Access Keys mode with blank or whitespace-only values silently fell through to the default credential chain instead of surfacing the misconfiguration. Implementation: - Add a module-level _resolve_bedrock_aws_credentials helper used by both AWSBedrockLLMParameters.validate() and AWSBedrock EmbeddingParameters.validate(), so the auth-type contract is expressed once. - Validates auth_type against an allowlist (None | "access_keys" | "iam_role"); raises ValueError on anything else. - iam_role: unconditionally drops aws_access_key_id and aws_secret_access_key. - access_keys (explicit): requires non-blank values; raises ValueError if either is empty or whitespace-only. - Legacy (auth_type absent): retains the lenient strip behaviour so pre-PR adapter configurations continue to deserialise unchanged. - Restore aws_region_name as required (no `= None` default) on AWSBedrockEmbeddingParameters; only credentials may legitimately be absent. - Drop the orphan aws_profile_name field from embedding1/static/bedrock.json: it was added for parity with the LLM form but lives outside the auth_type oneOf and contradicts the selector's "no further input" semantics. The LLM form already had aws_profile_name pre-PR and is left alone for backwards compatibility. Tests: - New tests/test_bedrock_adapter.py covers 15 cases across LLM and embedding adapters: legacy-no-auth-type, explicit access_keys with valid/blank/whitespace keys, iam_role with stale/no keys, unknown auth_type rejection, cross-field validation, and preservation of unrelated params (model_id, aws_profile_name, region, thinking). Skipped (P2 nice-to-have): - Comment-scope clarification, MinIO reference rewording, validate-mutates-caller'\''s-dict, and the LLM form description nit about aws_profile_name visibility. These don'\''t change behaviour and can be addressed in a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --------- Co-authored-by: Chandrasekharan M <117059509+chandrasekharan-zipstack@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Athul <89829560+athul-rs@users.noreply.github.com> * batch notification --------- Co-authored-by: ali <117142933+muhammad-ali-e@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Ritwik G <100672805+ritwik-g@users.noreply.github.com> Co-authored-by: Deepak K <89829542+Deepak-Kesavan@users.noreply.github.com> Co-authored-by: vishnuszipstack <117254672+vishnuszipstack@users.noreply.github.com> Co-authored-by: Praveen Kumar <praveen@zipstack.com> Co-authored-by: Chandrasekharan M <117059509+chandrasekharan-zipstack@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Athul <89829560+athul-rs@users.noreply.github.com>
…m:Zipstack/unstract into UN-3056-Notify-on-API-deployment-failures
…fy-on-API-deployment-failures
…fy-on-API-deployment-failures
* batch notification * notification slack
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/workflow_manager/internal_serializers.py (1)
176-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the file aggregates against
total_files.These fields are validated independently right now, so impossible payloads like
total_files=1, successful_files=2, failed_files=2will pass and then be persisted. That will skew the new outcome-based notification logic.Suggested fix
class WorkflowExecutionStatusUpdateSerializer(serializers.Serializer): """Serializer for updating workflow execution status.""" @@ failed_files = serializers.IntegerField(required=False, min_value=0) attempts = serializers.IntegerField(required=False, min_value=0) execution_time = serializers.FloatField(required=False, min_value=0) + + def validate(self, attrs): + total_files = attrs.get("total_files") + successful_files = attrs.get("successful_files") + failed_files = attrs.get("failed_files") + + if (successful_files is not None or failed_files is not None) and total_files is None: + raise serializers.ValidationError( + {"total_files": "total_files is required when file aggregates are provided."} + ) + + if total_files is not None: + if successful_files is not None and successful_files > total_files: + raise serializers.ValidationError( + {"successful_files": "successful_files cannot exceed total_files."} + ) + if failed_files is not None and failed_files > total_files: + raise serializers.ValidationError( + {"failed_files": "failed_files cannot exceed total_files."} + ) + if ( + successful_files is not None + and failed_files is not None + and successful_files + failed_files > total_files + ): + raise serializers.ValidationError( + "successful_files + failed_files cannot exceed total_files." + ) + + return attrs🤖 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/workflow_manager/internal_serializers.py` around lines 176 - 184, The serializer currently validates total_files, successful_files and failed_files independently; add a validate(self, data) method in the same serializer (where status, error_message, total_files, successful_files, failed_files, attempts, execution_time are defined) that, when total_files is provided, enforces that successful_files and failed_files are each <= total_files (if present) and that (successful_files + failed_files) <= total_files; also handle the case where only one of successful_files/failed_files is present by ensuring it does not exceed total_files, and raise serializers.ValidationError with a clear message on violation so invalid aggregates like total_files=1, successful_files=2 are rejected before persisting.
🧹 Nitpick comments (2)
backend/notification_v2/views.py (1)
56-68: ⚡ Quick winUse tuple for
permission_classesclass attribute.Class attributes that are collections should be immutable (tuples) rather than mutable (lists) to avoid potential issues and follow best practices.
♻️ Proposed fix
- permission_classes = [IsAuthenticated, IsOrganizationAdmin] + permission_classes = (IsAuthenticated, IsOrganizationAdmin)🤖 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/notification_v2/views.py` around lines 56 - 68, The class attribute permission_classes on NotificationSettingsView is currently a list; change it to an immutable tuple to follow best practices by replacing the mutable list [IsAuthenticated, IsOrganizationAdmin] with a tuple (IsAuthenticated, IsOrganizationAdmin) so permission_classes is not modifiable at runtime and matches other DRF class-attribute patterns.backend/notification_v2/tasks.py (1)
46-50: ⚡ Quick winCombine the implicitly concatenated strings.
The two string literals on line 47 are implicitly concatenated. While valid Python, this can be error-prone and less readable.
♻️ Proposed fix
logger.warning( - "metric=notification_batch_dispatched_total result=dead_letter rows=%d " "exc=%r", + "metric=notification_batch_dispatched_total result=dead_letter rows=%d exc=%r", updated, exc, )🤖 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/notification_v2/tasks.py` around lines 46 - 50, The logger.warning call currently uses two implicitly concatenated string literals; replace them with a single combined format string in the logger.warning invocation so the message is explicit and readable (keep the format placeholders and the same arguments: updated and exc), e.g., a single string like "metric=notification_batch_dispatched_total result=dead_letter rows=%d exc=%r" passed to logger.warning with updated and exc.
🤖 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 `@backend/notification_v2/clubbed_renderer.py`:
- Line 3: Update the docstring in clubbed_renderer.py to replace the ambiguous
multiplication symbol "×" with a plain ASCII "x" so it satisfies Ruff rule
RUF002; locate the module-level or function/class docstring that contains the
sentence "The same envelope shape feeds every channel × mode cell so receivers
never" and change "×" to "x" (i.e., "channel x mode") to avoid the lint failure.
In `@backend/notification_v2/helper.py`:
- Around line 34-41: The current auth_sig is built by joining fields with "|"
which is ambiguous because authorization_key/header may contain "|" — change the
construction in helper.py where the hash is computed (the block that builds raw
from notification.authorization_type/authorization_key/authorization_header and
returns hashlib.sha256(...).hexdigest()) to encode the three parts as an
unambiguous structured value before hashing (e.g., create a fixed-order
list/tuple of the three parts with fallback to _AUTH_SIG_NONE and then
JSON-serialize it with stable separators, or use a length-prefixed
concatenation) and hash that encoded representation so different tuples can
never collide due to delimiter characters.
In `@backend/notification_v2/internal_api_views.py`:
- Around line 423-425: The batching flush currently groups rows by
(organization_id, webhook_url, auth_sig) but then uses rows[0].platform when
calling render_clubbed_message in _dispatch_group, causing mixed-platform
batches; update the flush query that persists/groups batches to include platform
in its grouping key (and add the corresponding DB index change) so grouping is
done by (organization_id, webhook_url, auth_sig, platform), and ensure
_dispatch_group (and any other consumer like the code around lines ~496-500)
reads platform from the grouped key rather than assuming rows[0].platform.
In `@backend/notification_v2/migrations/0002_notification_notify_on_failures.py`:
- Around line 10-21: The add-field migration currently creates a BooleanField
notify_on_failures which only distinguishes ALL vs FAILURES_ONLY; change this to
a tri-state field (e.g. models.CharField with choices or a small IntegerField)
on the Notification model in this migration so it can represent ALL,
FAILURES_ONLY, and SUCCESS_ONLY (use explicit choices like ("all","ALL"),
("failures","FAILURES_ONLY"), ("success","SUCCESS_ONLY")), set a sensible
default (e.g. "all"), and update the db_comment to document the three modes;
ensure the migration operation uses the new field type and name
notify_on_failures so downstream code can read the string/enum value rather than
a boolean.
In `@backend/notification_v2/migrations/0003_add_notification_buffer.py`:
- Around line 14-28: The migration adds a non-null CharField delivery_mode to
the notification model with default="BATCHED", which will change existing rows
to BATCHED on deploy; instead, modify the migration to preserve existing
behavior by performing a two-step change: 1) add delivery_mode as nullable (or
without a DB-level default) and include a RunPython data migration that sets
delivery_mode="IMMEDIATE" for existing Notification rows that should remain
immediate, and 2) then add a subsequent migration to set default="BATCHED" and
make the field non-nullable for future records; reference the migration
module/migration class in 0003_add_notification_buffer.py and the model name
"notification" and field name "delivery_mode" when implementing the nullable
field + RunPython backfill, or alternatively write a single migration that uses
RunPython before altering the field to set existing rows to "IMMEDIATE".
In `@backend/notification_v2/models.py`:
- Around line 58-65: The boolean field notify_on_failures on the model cannot
represent the three states required (ALL / FAILURES_ONLY / SUCCESS_ONLY); change
this to a tri-state enum field (e.g., a CharField or IntegerField with explicit
choices like NOTIFY_ALL, NOTIFY_FAILURES_ONLY, NOTIFY_SUCCESS_ONLY) and rename
the DB column/field to something clearer if helpful (e.g., notify_mode or
notify_condition) so intent is explicit; update the corresponding serializer(s)
and any filters/UI code that read/write notify_on_failures to accept and
validate the new enum values and migrate existing boolean data to the new enum
values in a migration.
In `@backend/notification_v2/provider/webhook/api_webhook.py`:
- Line 15: format_payload() is unconditionally wrapping self.payload which
causes double-enveloping on already-enveloped payloads or repeated send() calls;
change format_payload (and any callers like the constructor assignment
self.payload = self.format_payload() and the send() path at the 24-30 block) to
first detect whether the payload is already in the expected envelope shape
(e.g., check for the envelope root key/structure) and return it unchanged if so,
otherwise wrap it; ensure the envelope-detection logic is deterministic and
idempotent so multiple calls to format_payload/send() do not alter an
already-correctly-enveloped payload.
In `@backend/pipeline_v2/notification.py`:
- Around line 14-15: Remove ExecutionStatus.STOPPED from the set treated as
failures: update the _FAILURE_STATUSES definition to exclude
ExecutionStatus.STOPPED and similarly remove/adjust any other checks that
include ExecutionStatus.STOPPED (the second occurrence around the block handling
audience selection at lines 56-60) so that STOPPED executions are no longer
routed to failure-only subscriptions; ensure only true failure statuses (e.g.,
ExecutionStatus.ERROR) remain in _FAILURE_STATUSES and that any conditional
logic using that set (in notification audience selection) treats STOPPED as
non-failure/catch-all.
In `@backend/workflow_manager/workflow_v2/models/execution.py`:
- Around line 440-441: The current mapping sets successful = e.successful_files
or 0 and failed = e.failed_files or 0 which coerces NULL/None to 0 and can hide
unknown historical counts; change these assignments to preserve NULL/None (e.g.,
successful = e.successful_files if e.successful_files is not None else None, and
likewise for failed) and update any downstream status logic that treats 0 as "no
failures" to explicitly handle None as "unknown" (so PARTIAL_SUCCESS isn't
lost). Ensure references to successful and failed in the execution status
computation explicitly check for None versus integer values.
In
`@frontend/src/components/pipelines-or-deployments/notification-modal/CreateNotification.jsx`:
- Line 15: The component currently sends a boolean notify_on_failures which
can't represent SUCCESS_ONLY—replace this with a notify_on enum carrying one of
"ALL", "FAILURES_ONLY", or "SUCCESS_ONLY": update the component state/props
default (in CreateNotification.jsx) to hold notify_on instead of
notify_on_failures, map the form inputs (checkboxes/radio/select) to produce the
correct enum value, and change all payload constructions and update/create API
calls (the places around the existing notify_on_failures usage and the other
block referenced later in the file) to include notify_on with the proper enum
string; also adjust any validation/serialization logic that reads
notify_on_failures to use notify_on.
In `@frontend/src/components/settings/platform/PlatformSettings.jsx`:
- Around line 61-77: The effect in useEffect that fetches org-scoped batch
interval should guard on sessionDetails?.orgId and include proper deps and
cancellation: at the top of the effect return early if !sessionDetails?.orgId,
add sessionDetails?.orgId and axiosPrivate to the dependency array, and
implement request cancellation (e.g., AbortController or axios cancel token) so
in-flight responses don't call setBatchIntervalMinutes after unmount or when
orgId changes; update references to axiosPrivate and setBatchIntervalMinutes
accordingly.
In `@workers/shared/patterns/notification/helper.py`:
- Around line 77-84: The except block in _enqueue_to_buffer() is swallowing
enqueue failures (logging then returning False) which causes
_route_notification() to treat BATCHED delivery as successful; instead,
propagate the failure so a retrying caller can act (or implement local
retry/backoff). Replace the logger.error+return False with logger.exception(...)
to include stack context and then re-raise the exception (or raise a specific
EnqueueError) so _route_notification() sees the failure; make the same change
for the other similar block referenced (lines ~104-106) to avoid silent drops.
---
Outside diff comments:
In `@backend/workflow_manager/internal_serializers.py`:
- Around line 176-184: The serializer currently validates total_files,
successful_files and failed_files independently; add a validate(self, data)
method in the same serializer (where status, error_message, total_files,
successful_files, failed_files, attempts, execution_time are defined) that, when
total_files is provided, enforces that successful_files and failed_files are
each <= total_files (if present) and that (successful_files + failed_files) <=
total_files; also handle the case where only one of
successful_files/failed_files is present by ensuring it does not exceed
total_files, and raise serializers.ValidationError with a clear message on
violation so invalid aggregates like total_files=1, successful_files=2 are
rejected before persisting.
---
Nitpick comments:
In `@backend/notification_v2/tasks.py`:
- Around line 46-50: The logger.warning call currently uses two implicitly
concatenated string literals; replace them with a single combined format string
in the logger.warning invocation so the message is explicit and readable (keep
the format placeholders and the same arguments: updated and exc), e.g., a single
string like "metric=notification_batch_dispatched_total result=dead_letter
rows=%d exc=%r" passed to logger.warning with updated and exc.
In `@backend/notification_v2/views.py`:
- Around line 56-68: The class attribute permission_classes on
NotificationSettingsView is currently a list; change it to an immutable tuple to
follow best practices by replacing the mutable list [IsAuthenticated,
IsOrganizationAdmin] with a tuple (IsAuthenticated, IsOrganizationAdmin) so
permission_classes is not modifiable at runtime and matches other DRF
class-attribute patterns.
🪄 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: d8dfdd8e-ab77-48bb-a35e-1d2d9ef6fd9a
📒 Files selected for processing (38)
backend/api_v2/notification.pybackend/backend/settings/base.pybackend/configuration/enums.pybackend/notification_v2/clubbed_renderer.pybackend/notification_v2/enums.pybackend/notification_v2/helper.pybackend/notification_v2/internal_api_views.pybackend/notification_v2/internal_serializers.pybackend/notification_v2/internal_urls.pybackend/notification_v2/migrations/0002_notification_notify_on_failures.pybackend/notification_v2/migrations/0003_add_notification_buffer.pybackend/notification_v2/models.pybackend/notification_v2/provider/webhook/api_webhook.pybackend/notification_v2/provider/webhook/slack_webhook.pybackend/notification_v2/provider/webhook/webhook.pybackend/notification_v2/serializers.pybackend/notification_v2/tasks.pybackend/notification_v2/urls.pybackend/notification_v2/views.pybackend/pipeline_v2/dto.pybackend/pipeline_v2/notification.pybackend/workflow_manager/internal_serializers.pybackend/workflow_manager/internal_views.pybackend/workflow_manager/workflow_v2/migrations/0020_workflowexecution_file_counts.pybackend/workflow_manager/workflow_v2/models/execution.pyfrontend/src/components/pipelines-or-deployments/notification-modal/CreateNotification.jsxfrontend/src/components/settings/platform/PlatformSettings.jsxunstract/core/src/unstract/core/data_models.pyworkers/callback/tasks.pyworkers/log_consumer/process_notification_buffer.pyworkers/log_consumer/scheduler.shworkers/notification/providers/_clubbed_format.pyworkers/notification/providers/api_webhook.pyworkers/notification/providers/slack_webhook.pyworkers/scheduler/tasks.pyworkers/shared/api/internal_client.pyworkers/shared/clients/execution_client.pyworkers/shared/patterns/notification/helper.py
|
| Filename | Overview |
|---|---|
| backend/notification_v2/internal_api_views.py | Central piece of new functionality — buffer enqueue, group-level flush, and GC. PENDING rows for deactivated notifications are never GC'd, creating unbounded table growth. |
| backend/notification_v2/helper.py | Old NotificationHelper replaced by dispatch_with_delivery_mode + enqueue; auth_sig and flush_after computed at write time. Clean redesign. |
| backend/notification_v2/models.py | Adds notify_on_failures bool, delivery_mode, and NotificationBuffer model with partial covering index. Well-structured. |
| backend/notification_v2/tasks.py | New mark_buffer_dead_letter task used as Celery link_error callback; parameter names (request, exc, traceback) suggest on_failure semantics but link_error only passes the failed task UUID as first arg. buffer_row_ids from kwargs works correctly regardless. |
| unstract/core/src/unstract/core/notification_clubbed_renderer.py | Shared renderer producing the canonical {summary, events} envelope with Slack mrkdwn output. |
| workers/shared/patterns/notification/helper.py | Old direct-dispatch (send_notification_to_worker) removed in favour of _route_notification to _enqueue_to_buffer POST. execution_id forwarding added to GET calls so the backend can apply notify_on_failures filter. |
Sequence Diagram
sequenceDiagram
participant W as Worker
participant BE as Backend API
participant DB as NotificationBuffer
participant Broker as Celery Broker
W->>BE: "GET /notifications/?execution_id=X"
BE->>DB: Filter Notification (is_active, notify_on_failures)
BE-->>W: "[{notification_id, platform, ...}]"
loop each notification
W->>BE: POST /buffer/enqueue/
BE->>DB: "INSERT NotificationBuffer(status=PENDING)"
BE-->>W: "{buffer_row_id}"
end
Note over BE,DB: Scheduler fires every interval
W->>BE: POST /buffer/process/
BE->>DB: "GROUP BY (org,url,auth_sig,platform) WHERE flush_after<=now"
loop each ready group
BE->>DB: SELECT FOR UPDATE SKIP LOCKED
BE->>DB: "UPDATE status=DISPATCHED"
BE->>Broker: on_commit to send_webhook_notification
end
BE->>DB: GC old DISPATCHED/DEAD_LETTER rows
BE-->>W: "{dispatched_groups, dispatched_rows}"
alt retries exhausted
Broker->>BE: mark_buffer_dead_letter(buffer_row_ids)
BE->>DB: "UPDATE status=DEAD_LETTER"
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
backend/notification_v2/internal_api_views.py:382-387
**PENDING rows for deactivated notifications accumulate forever**
When a notification is deactivated (`is_active=False`), the flush job's `notification__is_active=True` filter prevents those PENDING rows from ever being dispatched — but `_gc_terminal_rows()` only deletes DISPATCHED and DEAD_LETTER rows. The orphaned PENDING rows grow without bound, bloating `notification_buffer` and the `idx_notif_buffer_pending` partial index, and adding noise to every flush-tick GROUP BY scan. A GC pass for old PENDING rows belonging to inactive notifications closes the gap.
```suggestion
cutoff = timezone.now() - timedelta(days=settings.NOTIFICATION_BUFFER_RETENTION_DAYS)
deleted_count, _ = NotificationBuffer.objects.filter(
status__in=[BufferStatus.DISPATCHED.value, BufferStatus.DEAD_LETTER.value],
created_at__lt=cutoff,
).delete()
# Also GC PENDING rows whose source notification has been deactivated.
# These are never dispatched (notification__is_active=True filter in
# _dispatch_group) and the terminal-row filter above never reaches them,
# so without this clause they accumulate indefinitely.
inactive_deleted, _ = NotificationBuffer.objects.filter(
status=BufferStatus.PENDING.value,
notification__is_active=False,
created_at__lt=cutoff,
).delete()
return int(deleted_count) + int(inactive_deleted)
```
### Issue 2 of 2
backend/notification_v2/tasks.py:24-35
**Misleading parameter names for a `link_error` callback**
Celery's `link_error` passes the **UUID string of the failed task** as the sole positional argument — not `(request, exc, traceback)` as an `on_failure` handler would. So `request` receives the failed task's ID, while `exc` and `traceback` are always `None`. The function works correctly because `buffer_row_ids` is bound via `kwargs` and is the only field the logic touches, but the signature creates a false impression that callers can recover the exception object from `exc`, which would silently be `None`.
Reviews (13): Last reviewed commit: "greptile review" | Re-trigger Greptile
Review triage summaryThanks to @coderabbitai and @greptile-apps for the thorough pass. Status of each item: Fixed in this branch
Stale / wont fix (replies posted on each thread with reasoning)\n- Tri-state
|
|
Tip For best results, initiate chat on the files or code changes.
All the fixes look solid:
The reasoning on the won't-fix items is sound — the Noted on the
|
|
Thanks for the thorough triage write-up. All of my P1 and P2 items are confirmed addressed:
The "wont fix" reasoning all holds up:
LGTM from my side. The implementation is solid and the edge cases I flagged are all properly handled. Tip: You can customize Greptile's behavior for this repo with |
|
Tip For best results, initiate chat on the files or code changes.
Fixes confirmed:
Won't-fix reasoning is sound:
|
Keep execution_id in _ENQUEUE_REQUIRED_FIELDS as the canonical required set; carve out the INPROGRESS exemption at the validator instead of dropping it broadly. Non-INPROGRESS callers (COMPLETED / ERROR / STOPPED / PARTIAL_SUCCESS) once again get a loud 400 if they omit execution_id, addressing Greptile's silent-failure concern on e653494. Extends the comment above the tuple to also flag the consumer-side gap: INPROGRESS buffer rows ship with execution_id=null, so API receivers cannot correlate them with execution logs until the producer-reorder follow-up (UN-3056) lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@greptileai thanks for the scope-pushback — fully agree on the silent-failure surface. Resolved in Point 1 — scope of the relaxation. Point 2 — consumer-side correlation gap. Confirmed: INPROGRESS buffer rows do store Point 3 — producer location. Confirmed: Point 4 — tracking. UN-3056 is the open follow-up. The producer reorder + the consumer-side correlation gap close together there. |
|
Thanks for the thorough write-up on all four points. The reasoning is solid. One thing worth flagging: commit # `execution_id` is intentionally optional: the scheduler INPROGRESS path
# (workers/scheduler/tasks.py, UN-2850 / #1562) fires before WorkflowExecution
# is created. Renderer falls back to `—` for missing values.
_ENQUEUE_REQUIRED_FIELDS = (
"notification_id",
"pipeline_id",
"pipeline_name",
"status",
"platform",
)If the INPROGRESS-exemption approach is landing in a follow-up push, that's fine — but the described behavior (hard 400 for non-INPROGRESS callers missing On points 2–4: the acknowledgement that Effectively LGTM once the INPROGRESS-exemption commit lands and is visible on the branch. |
|
@greptileai you were right that Current # `execution_id` is required except for INPROGRESS, which fires from the
# scheduler (workers/scheduler/tasks.py, UN-2850) before WorkflowExecution
# exists. INPROGRESS rows therefore store execution_id=null — receivers
# cannot correlate with execution logs until the producer-reorder lands
# (UN-3056).
_ENQUEUE_REQUIRED_FIELDS = (
"notification_id",
"pipeline_id",
"pipeline_name",
"status",
"platform",
"execution_id",
)
|
_dispatch_group's lock query did not check notification.is_active, so
PENDING NotificationBuffer rows tied to a deactivated source notification
still dispatched on the next flush tick (up to one NOTIFICATION_CLUB_INTERVAL
of stale traffic). IMMEDIATE deactivation is instant because the GET
notifications endpoint filters by is_active=True; this restores the same
expectation for BATCHED.
Also adds select_related("notification") so the later rows[0].notification
read is part of the same query rather than a per-group round-trip.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@backend/notification_v2/helper.py`:
- Around line 71-91: The current build_webhook_headers(Notification) recreates
headers from the live Notification and can mix enqueue-time auth_sig buckets
with updated credentials; change the dispatch to use the snapshot stored on the
buffer row instead of reading Notification at flush time: stop calling
build_webhook_headers(notification) directly in
internal_api_views._dispatch_group and instead read auth_type, auth_key,
auth_header (or auth_sig) that were saved on the buffer row when enqueuing and
pass those into a new helper (or overload build_webhook_headers to accept raw
auth fields) so headers are deterministic per row; alternatively implement logic
in the edit path to re-key/rebucket pending buffer rows when Notification auth
is changed so queued rows never use current Notification state at flush time.
- Around line 149-150: When computing flush_after, honor the per-event
delivery_mode instead of always using
get_org_club_interval_seconds(organization): if delivery_mode equals IMMEDIATE
(or the equivalent enum/constant used in your codebase) set flush_after to
timezone.now() (or skip batching) so the event is delivered immediately;
otherwise compute interval_seconds = get_org_club_interval_seconds(organization)
and set flush_after = timezone.now() + timedelta(seconds=interval_seconds).
Update the code paths that set flush_after (and any callers that assume
batching) to handle the IMMEDIATE branch accordingly.
In `@workers/shared/patterns/notification/helper.py`:
- Line 61: Update the exception handler line that currently reads "except
Exception: # noqa: BLE001 — propagate any failure, don't classify" so the noqa
suppression is standalone and the rationale is a separate comment line;
specifically, change the handler to have the inline "# noqa: BLE001" only on the
"except Exception:" line (no extra text after the noqa) and add a following
comment line like "# propagate any failure, don't classify" above the handler
body; locate the "except Exception" block in helper.py (the except Exception
handler) and make this two-line comment split to satisfy Sonar.
🪄 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: dac68d3e-c63e-4a54-9c37-26332e88cf26
📒 Files selected for processing (15)
backend/notification_v2/clubbed_renderer.pybackend/notification_v2/enums.pybackend/notification_v2/helper.pybackend/notification_v2/internal_api_views.pybackend/notification_v2/provider/__init__.pybackend/notification_v2/provider/notification_provider.pybackend/notification_v2/provider/registry.pybackend/notification_v2/provider/webhook/__init__.pybackend/notification_v2/provider/webhook/api_webhook.pybackend/notification_v2/provider/webhook/slack_webhook.pybackend/notification_v2/provider/webhook/webhook.pyunstract/core/src/unstract/core/notification_clubbed_renderer.pyworkers/notification/providers/api_webhook.pyworkers/notification/providers/slack_webhook.pyworkers/shared/patterns/notification/helper.py
💤 Files with no reviewable changes (5)
- backend/notification_v2/provider/registry.py
- backend/notification_v2/provider/notification_provider.py
- backend/notification_v2/provider/webhook/webhook.py
- backend/notification_v2/provider/webhook/api_webhook.py
- backend/notification_v2/provider/webhook/slack_webhook.py
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/notification_v2/clubbed_renderer.py
- backend/notification_v2/enums.py
- unstract/core/src/unstract/core/notification_clubbed_renderer.py
- workers/notification/providers/api_webhook.py
- backend/notification_v2/internal_api_views.py
- workers/notification/providers/slack_webhook.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
workers/shared/patterns/notification/helper.py (1)
77-105: 💤 Low valueImport
NotificationTypefromunstract.core.data_modelsand replace the hardcoded"WEBHOOK"string.The file already imports from
unstract.core.data_models(line 11); simply addNotificationTypeto that import list and useNotificationType.WEBHOOK.valueat line 90. This prevents silent breakage if the backend ever changes the literal value and keeps the worker and backend aligned.🤖 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 `@workers/shared/patterns/notification/helper.py` around lines 77 - 105, Update the _route_notification function to use the enum instead of a hardcoded string: add NotificationType to the existing import from unstract.core.data_models and replace the comparison notification.get("notification_type") != "WEBHOOK" with notification.get("notification_type") != NotificationType.WEBHOOK.value; this keeps the worker aligned with backend literal values and avoids brittle string checks while preserving the existing behavior and exception handling in _route_notification and its call to _enqueue_to_buffer.
🤖 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 `@workers/shared/patterns/notification/helper.py`:
- Around line 77-105: Update the _route_notification function to use the enum
instead of a hardcoded string: add NotificationType to the existing import from
unstract.core.data_models and replace the comparison
notification.get("notification_type") != "WEBHOOK" with
notification.get("notification_type") != NotificationType.WEBHOOK.value; this
keeps the worker aligned with backend literal values and avoids brittle string
checks while preserving the existing behavior and exception handling in
_route_notification and its call to _enqueue_to_buffer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73ef426c-df79-43aa-98b0-9ead57a43fc5
📒 Files selected for processing (2)
backend/notification_v2/internal_api_views.pyworkers/shared/patterns/notification/helper.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/notification_v2/internal_api_views.py
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
…m:Zipstack/unstract into UN-3056-Notify-on-API-deployment-failures
|



What
notify_on_failuresboolean toNotification(defaultFalse). WhenTrue, fire only on terminal failures (ERROR/STOPPEDfor any pipeline type, or any file errored in aCOMPLETEDrun). Surface as a "Notify on failures only" checkbox in the notification create/edit form.NotificationBuffertable; a periodic worker flush groups events by(org, webhook_url, auth_sig, platform)and ships them as one clubbed message per group perNOTIFICATION_CLUB_INTERVALwindow. Interval is configurable per-org from Platform Settings (1–120 minutes, default 5).notification_v2/provider/subtree,NotificationHelper,split_by_delivery_mode, and the worker'ssend_notification_to_worker+get_webhook_headers. Single dispatch path.Why
How
On terminal failure (
ERROR/STOPPED, or any file errored in aCOMPLETEDrun) all active rows fire. On terminal success the queryset excludesnotify_on_failures=Truerows. Defaulting toFalsekeeps existing rows on the previous "every completion" behavior.The failure-status set is consolidated into a single
notification_v2.enums.FAILURE_STATUSES(frozen set of{ERROR, STOPPED}) used by all three dispatch sites (api_v2/notification.py,pipeline_v2/notification.py, worker callback).Dispatch flow:
PipelineNotification.send/APINotification.sendand the worker callback path all funnel intonotification_v2.helper.enqueuewhich writes aNotificationBufferrow. Theworkers/log_consumer/scheduler.shsidecar periodically calls the internal/v1/webhook/buffer/process/endpoint; the backend groups PENDING rows whoseMIN(flush_after) ≤ NOW()by(org, webhook_url, auth_sig, platform), renders one combined message viaunstract.core.notification_clubbed_renderer({summary, events}envelope; Slack mrkdwn body forplatform=SLACK), and queues a singlesend_webhook_notificationCelery task per group withmax_retries = max()across the rows. Concurrency-safe viaSELECT … FOR UPDATE OF o SKIP LOCKED.Can this PR break any existing features. If yes, please list possible items. If no, please explain why.
Two caveats for operators:
0003_add_notification_buffer.py). This PR introduces theNotificationBuffertable; every notification now clubs dispatches at the per-orgNOTIFICATION_CLUB_INTERVALcadence (default 5 min) instead of firing synchronously per run. The previous synchronous-dispatch code paths have been removed — there is no opt-out to per-event dispatch. Thedelivery_modecolumn is added with defaultBATCHEDand stays on the model for backward DB compat (existing rows are auto-backfilled toBATCHED); no code readsIMMEDIATEand the field is no longer surfaced through the internal notifications API or settable through the publicNotificationSerializer. The column + enum value will be dropped in a follow-up schema migration once deploys are coordinated.{summary, events}shape. Any prior consumer that parsed the old flat per-run payload needs to readbody["events"][0]instead ofbodydirectly. See the in-tree plan file (UNS-611 v2.5/v2.6) for the contract.The
notify_on_failuresfilter itself defaults toFalseand changes no existing caller signatures.Database Migrations
backend/notification_v2/migrations/0002_notification_notify_on_failures.py— adds the boolean column.backend/notification_v2/migrations/0003_add_notification_buffer.py— addsdelivery_mode(defaultBATCHED) and createsNotificationBuffer. See the operator note in "Can this PR break any existing features" above.idx_notif_buffer_pendingpartial index in0003includesplatformso SLACK and API rows on the same (org, url, auth) split into separate dispatches at flush time.Env Config
NOTIFICATION_CLUB_INTERVAL(seconds; default 300 = 5 min). Per-org override available in Platform Settings (1–120 minutes).Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Manual:
NOTIFICATION_CLUB_INTERVAL(default 5 min, or the per-org override set in Platform Settings).Screenshots
Checklist
I have read and understood the Contribution Guidelines.