connectors - #3
Closed
kirtimanmishrazipstack wants to merge 3 commits into
Closed
kirtimanmishrazipstack wants to merge 3 commits into
kirtimanmishrazipstack wants to merge 3 commits into
Conversation
This was referenced Oct 30, 2025
5 tasks done
This was referenced Apr 28, 2026
ritwik-g
pushed a commit
that referenced
this pull request
May 5, 2026
…o 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>
kirtimanmishrazipstack
added a commit
that referenced
this pull request
May 7, 2026
* 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>
This was referenced May 26, 2026
kirtimanmishrazipstack
added a commit
that referenced
this pull request
Jun 1, 2026
- NotificationBuffer.dispatch_attempts + NOTIFICATION_MAX_DISPATCH_ATTEMPTS: _dispatch_group dead-letters rows past the cap and increments on each SENDING claim, bounding the reaper reclaim loop so a lost terminal callback can't redeliver forever (self-review #3). - Delete the orphaned synchronous notification_v2/provider/ cluster — zero callers after the batched dispatch_notifications path replaced it (#2). - Fold dispatch_attempts into 0002_notification_batching; refresh lifecycle db_comments + BufferStatus docstring. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
muhammad-ali-e
added a commit
that referenced
this pull request
Jun 8, 2026
…test-infra fix) Seven of Vishnu's findings against ``524ae9184`` addressed. Three flagged IMPORTANT (silent-failure + missing test coverage), four SUGGESTION (drift hazard, comment/behaviour mismatch, hollow canary, duplicate-test cleanup). The other three (hand-built fixture, SDK1 ``dict[str, Any]`` boundary, ``as_header`` TypedDict refactor) deferred — see PR thread acknowledgments. * **#11a (SUGGESTION, drift)** — ``workers/queue_backend/dispatch.py`` still hand-built the fairness header instead of calling ``fairness.as_header()``. Wire-format encoding now has a single source so the two sites can't drift. ``FAIRNESS_HEADER_NAME`` import dropped (no longer used here). * **#9 (SUGGESTION, comment/behaviour mismatch)** — ``ExecutionDispatcher._build_send_kwargs`` was forwarding ``{}`` as-is despite the docstring calling it "likely indicates a producer-side build bug". Changed ``if headers is not None`` -> ``if headers``: falsy is dropped so the on-wire shape matches the no-headers baseline and a miswired producer surfaces immediately. Docstring rewritten to describe the new contract. * **#3 (SUGGESTION, wording)** — ``canary_helpers`` docstring overstated adoption. Softened to "intended single home" and named the two characterisation walkers still inlining the logic as a known follow-up. * **#5 + #6 (IMPORTANT cleanup + SUGGESTION fixture)** — six structurally-identical ``*_forwards_headers`` / ``*_omits_headers_when_none`` tests collapsed to three parametrized methods over the three dispatch entry points. Fixture now uses ``FairnessKey(...).as_header()`` rather than hand-built dicts, so the wire shape exercised matches what real producers emit (including ``pipeline_priority``). Net: ~60 LOC removed, per-method failure granularity preserved via parametrize IDs. Also added empty-dict drop assertions covering #9. * **#7 (SUGGESTION, missing combined test)** — new ``test_dispatch_with_callback_combines_headers_and_callbacks`` passes ``on_success``, ``on_error``, ``task_id``, and ``headers`` together and asserts all four land on the same ``send_task`` call. A key-merge regression in ``_build_send_kwargs`` would have slipped through the single-kwarg forwarding tests. * **#8 (SUGGESTION, hollow canary)** — the ``execute_extraction`` dispatch canary only ever asserted the empty (passing) case against the live tree. Added a positive- detection unit test feeding ``ast.parse`` of a known-bad snippet and a blind-spot lock test (constant ref, f-string, ``apply_async`` all evade the detector — documenting the scope so a future widening intentionally trips the asserts). * **#4 (IMPORTANT, untested helper)** — ``_fairness_headers`` in ``structure_tool_task`` was untested; a regression flipping ``NON_API`` -> ``API`` or dropping ``headers=`` at any of the three call sites would have stayed green. Added focused unit tests in new ``test_structure_tool_task.py`` (wire shape, org_id propagation, ``NON_API`` not ``API``) and extended ``test_sanity_phase5.TestStructureToolSingleDispatch`` to assert ``dispatch.call_args.kwargs["headers"]`` carries the expected shape. * **#2 (IMPORTANT, vacuous-pass)** — ``iter_production_trees`` warned-and-continued on ``SyntaxError`` but neither canary module promoted the warning to error. A botched merge in a production file would have dropped silently from the audit set and every canary would have passed vacuously over a smaller tree. Added ``pytestmark = pytest.mark.filterwarnings( "error::UserWarning")`` on both ``test_executor_dispatch`` and ``test_fairness_key``, plus a new ``test_canary_helpers.py`` that unit-tests both the warn-on-broken behaviour and the promote-to-error contract the canary modules rely on. **Bundled test-infra fix (unrelated but unblocks CI):** the ``test_callback_sanity.TestEagerHealthcheckRoundTrip`` tests selected the healthcheck task via ``endswith(".healthcheck")`` against ``eager_app.tasks``, which is a shared celery global registry containing ``callback.worker.healthcheck``, ``executor.worker.healthcheck``, ``file_processing.worker.healthcheck`` etc. The bare ``next(...)`` returned whichever was inserted first — non-deterministic across pytest module-collection orders. Without this fix, the new tests added in this commit perturb the collection profile enough to flip the failure rate from ~10% to nearly 100%. Replaced with exact-name lookup ``name == "callback.worker.healthcheck"``. Identical fix already landed on the UN-3513 branch (see #2020). Test count: 31 -> 42 on the UN-3508-touched modules. Full workers suite: 6 failures pre-existing baseline, unchanged by this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
muhammad-ali-e
added a commit
that referenced
this pull request
Jun 8, 2026
* UN-3508 [FEAT] Plumb fairness through ExecutionDispatcher
Phase 5.2 of the PG Queue rollout (epic UN-3445). Adds fairness-header
support to the third dispatch path (sdk1's ExecutionDispatcher) so
``execute_extraction`` tasks emitted by file_processing carry the same
routing metadata as workflow-execution dispatches that go through
queue_backend.
What
* sdk1/execution/dispatcher.py: ``dispatch``, ``dispatch_async``,
``dispatch_with_callback`` all accept an optional ``headers`` kwarg.
When non-None, forwarded to Celery's send_task; when None, omitted
so the call shape stays identical to pre-Phase-5.2 for callers that
don't opt in (sdk1's existing tests remain green unchanged).
* queue_backend/fairness.py: new ``FairnessKey.as_header()`` method
returns the wire-ready ``{"x-fairness-key": ...}`` dict. Producers
no longer need to reference ``FAIRNESS_HEADER_NAME`` directly —
keeps the additive-only canary in test_fairness_key.py happy.
* file_processing/structure_tool_task.py: small ``_fairness_headers``
helper builds the header (defaulting workload_type to NON_API;
propagating the real type is Phase 6 work). All three
``dispatcher.dispatch(...)`` sites (lines 468, 507, 720) now pass
``headers=_fairness_headers(organization_id)``.
* tests/test_executor_dispatch.py: new file. Covers header forwarding
through all three dispatcher methods (including the "omit when
None" pre-existing shape preservation), the FairnessKey.as_header()
shape, and an AST inventory canary that forbids raw
``*.send_task("execute_extraction", ...)`` outside
ExecutionDispatcher.
Why
UN-3501 plumbed fairness on bare dispatch() call sites. The
``execute_extraction`` task is the most workflow-execution-y dispatch
in the codebase but bypasses queue_backend (uses ExecutionDispatcher
directly), so it had no fairness header. The canary in
test_fairness_key.py audits only bare-name dispatch() and missed it.
No regression risk
* Additive: ``headers`` is optional and defaults to None on all three
dispatcher methods; the existing 78 sdk1 tests pass unchanged.
* Producer-side only — no consumer reads ``x-fairness-key`` yet.
* No queue routing, task name, or args/kwargs change.
Test count: workers seam suite 53 -> 60 (new test_executor_dispatch.py
with 7 tests). sdk1 dispatcher suite 80/80 green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* UN-3508 [REFACTOR] Extract shared canary helpers to drop SonarCloud duplication
SonarCloud flagged 7.8% duplicated lines in the new
test_executor_dispatch.py — the file-walking helper and skip-dir
constants were copy-pasted from test_fairness_key.py.
Move them into tests/canary_helpers.py:
* WORKERS_ROOT, DEFAULT_SKIP_TOP_DIRS constants.
* iter_production_trees(skip_top_dirs=…) generator.
Both canary tests use relative imports (from .canary_helpers import …)
to keep one canonical import path — tests/ is already a package via
__init__.py, no pyproject change needed. (An earlier attempt added
pythonpath = ["tests"], reverted — it would have created a second
top-level import path for every test file and a dual-module-object
hazard.)
The fairness canary widens its skip set with ``queue_backend`` (where
the seam legitimately defines fairness constants); the executor canary
keeps the default. Tests stay at 60/60 — pure dedup, no behavioural
change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* UN-3508 [FIX] Address 14 PR review findings (HIGH/MED/NIT)
* dispatcher.py: factor _build_send_kwargs helper; document headers kwarg on dispatch_with_callback; reference FAIRNESS_HEADER_NAME symbol instead of bare string; document empty-dict caller-bug semantic
* structure_tool_task.py: narrow _fairness_headers return type; replace 'Phase 6 work' with TODO(UN-3504) anchor
* fairness.py: concrete as_header() docstring with explicit shape
* canary_helpers.py: surface SyntaxError via UserWarning (real silent-failure bug; canaries no longer pass vacuously on unparseable files)
* test_executor_dispatch.py: switch to dict[str, Any] dropping type-ignore; use WorkloadType.NON_API.value instead of invalid 'etl' literal; new test_dispatch_async_omits_headers_when_none; tighten canary docstring + note blind spots; drop plan-stage vocab; reorder relative import; new test_fairness_header_shape_orgless for org_id=None case
Tests: workers 60 -> 62, sdk1 dispatcher 80/80 green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* UN-3508 [DOCS] Fix iter_production_trees docstring: 'Yield' -> 'Return a list'
Greptile P2: function builds and returns a list — it is not a
generator — but the docstring opened with 'Yield ...', which would
mislead a reader into expecting lazy consumption / generator semantics
(early break, send(), etc.).
Pure docstring fix, no behaviour change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* UN-3508 [FIX] Address vishnuszipstack review (7 real fixes + bundled test-infra fix)
Seven of Vishnu's findings against ``524ae9184`` addressed. Three
flagged IMPORTANT (silent-failure + missing test coverage), four
SUGGESTION (drift hazard, comment/behaviour mismatch, hollow canary,
duplicate-test cleanup). The other three (hand-built fixture,
SDK1 ``dict[str, Any]`` boundary, ``as_header`` TypedDict refactor)
deferred — see PR thread acknowledgments.
* **#11a (SUGGESTION, drift)** — ``workers/queue_backend/dispatch.py``
still hand-built the fairness header instead of calling
``fairness.as_header()``. Wire-format encoding now has a single
source so the two sites can't drift. ``FAIRNESS_HEADER_NAME``
import dropped (no longer used here).
* **#9 (SUGGESTION, comment/behaviour mismatch)** —
``ExecutionDispatcher._build_send_kwargs`` was forwarding ``{}``
as-is despite the docstring calling it "likely indicates a
producer-side build bug". Changed ``if headers is not None`` ->
``if headers``: falsy is dropped so the on-wire shape matches the
no-headers baseline and a miswired producer surfaces immediately.
Docstring rewritten to describe the new contract.
* **#3 (SUGGESTION, wording)** — ``canary_helpers`` docstring
overstated adoption. Softened to "intended single home" and
named the two characterisation walkers still inlining the logic
as a known follow-up.
* **#5 + #6 (IMPORTANT cleanup + SUGGESTION fixture)** — six
structurally-identical ``*_forwards_headers`` /
``*_omits_headers_when_none`` tests collapsed to three
parametrized methods over the three dispatch entry points.
Fixture now uses ``FairnessKey(...).as_header()`` rather than
hand-built dicts, so the wire shape exercised matches what real
producers emit (including ``pipeline_priority``). Net: ~60 LOC
removed, per-method failure granularity preserved via parametrize
IDs. Also added empty-dict drop assertions covering #9.
* **#7 (SUGGESTION, missing combined test)** — new
``test_dispatch_with_callback_combines_headers_and_callbacks``
passes ``on_success``, ``on_error``, ``task_id``, and ``headers``
together and asserts all four land on the same ``send_task``
call. A key-merge regression in ``_build_send_kwargs`` would
have slipped through the single-kwarg forwarding tests.
* **#8 (SUGGESTION, hollow canary)** — the
``execute_extraction`` dispatch canary only ever asserted the
empty (passing) case against the live tree. Added a positive-
detection unit test feeding ``ast.parse`` of a known-bad snippet
and a blind-spot lock test (constant ref, f-string,
``apply_async`` all evade the detector — documenting the scope
so a future widening intentionally trips the asserts).
* **#4 (IMPORTANT, untested helper)** — ``_fairness_headers`` in
``structure_tool_task`` was untested; a regression flipping
``NON_API`` -> ``API`` or dropping ``headers=`` at any of the
three call sites would have stayed green. Added focused unit
tests in new ``test_structure_tool_task.py`` (wire shape,
org_id propagation, ``NON_API`` not ``API``) and extended
``test_sanity_phase5.TestStructureToolSingleDispatch`` to assert
``dispatch.call_args.kwargs["headers"]`` carries the expected
shape.
* **#2 (IMPORTANT, vacuous-pass)** — ``iter_production_trees``
warned-and-continued on ``SyntaxError`` but neither canary
module promoted the warning to error. A botched merge in a
production file would have dropped silently from the audit set
and every canary would have passed vacuously over a smaller
tree. Added ``pytestmark = pytest.mark.filterwarnings(
"error::UserWarning")`` on both ``test_executor_dispatch`` and
``test_fairness_key``, plus a new ``test_canary_helpers.py``
that unit-tests both the warn-on-broken behaviour and the
promote-to-error contract the canary modules rely on.
**Bundled test-infra fix (unrelated but unblocks CI):** the
``test_callback_sanity.TestEagerHealthcheckRoundTrip`` tests
selected the healthcheck task via ``endswith(".healthcheck")``
against ``eager_app.tasks``, which is a shared celery global
registry containing ``callback.worker.healthcheck``,
``executor.worker.healthcheck``,
``file_processing.worker.healthcheck`` etc. The bare ``next(...)``
returned whichever was inserted first — non-deterministic across
pytest module-collection orders. Without this fix, the new tests
added in this commit perturb the collection profile enough to
flip the failure rate from ~10% to nearly 100%. Replaced with
exact-name lookup ``name == "callback.worker.healthcheck"``.
Identical fix already landed on the UN-3513 branch (see #2020).
Test count: 31 -> 42 on the UN-3508-touched modules. Full workers
suite: 6 failures pre-existing baseline, unchanged by this commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
muhammad-ali-e
added a commit
that referenced
this pull request
Jun 9, 2026
…ing nit) Seven of Vishnu's PR review findings addressed, all backward-compat with main-branch consumers. The three [Important] design-redesign findings (#1 status __post_init__, #2 alias-pair invariant, #3 to_api_dict/to_json dead code) are deferred to a follow-up shared-infra dataclass ticket because they would either fire warning noise on existing call sites (``worker_base.py:211/222``, ``worker_patterns.py:241`` pass wrong-enum status) or change the wire/cache contract — neither acceptable mid-flight while keeping zero regression on main. Changes in this commit are either: * Pure additive (test methods, docstrings, observability) * Or provably equivalent wire output (the typed-count refactor) So a rolling deploy where old workers and new workers run concurrently sees identical wire shapes and identical behaviour for all current valid data; the only observable differences are log content (better context on the existing warning) and the presence of a new opt-in classmethod that nothing currently calls. * **Vishnu #8 [Suggestion]** — ``SkipReason`` docstring claimed "StrEnum semantics" but the class is ``(str, Enum)``, not ``enum.StrEnum``. The two differ on ``__str__``. Rewrote the docstring to describe the actual behaviour. * **Vishnu #4a [Important — log context]** — ``_parse_skipped`` now accepts an optional ``file_execution_id`` kwarg that ``from_dict`` threads through. The warning emitted for unknown wire values now carries the file identifier, so a real rolling-deploy incident is debuggable rather than a context-free warning. Optional kwarg with default — any existing caller passing one positional arg still works. * **Vishnu #9 [Suggestion]** — added ``BatchExecutionResult.from_file_results(...)`` classmethod that derives counters from typed file results. Purely additive: no existing caller uses it; the constructor signature is unchanged so producers that need their own counter semantics keep working. * **Vishnu #11 [Suggestion]** — ``process_file_batch_api`` was computing ``skipped_already_completed`` by string-matching the wire dicts AFTER already calling ``from_dict`` on them. Refactored to count from the typed list (single ``from_dict`` pass, enum compare). Provably equivalent for all current wire data. * **Vishnu #4 [Important — test gap]** — added ``test_from_dict_unknown_skipped_is_lenient`` covering the one documented crash-prevention path. A regression to bare ``SkipReason(raw)`` would have re-introduced the rolling-deploy crash and kept every other test green. * **Vishnu #5 [Important — failure-aggregation gap]** — added ``test_process_file_batch_api_batch_wrapper_failure_aggregation`` that drives one success + one failure through the batch wrapper. The existing success-only test never exercised ``failed_files += 1``. * **Vishnu #6 [Important — populated round-trip gap]** — added ``test_round_trip_with_populated_file_results`` and ``test_from_file_results_derives_counters``. The existing ``BatchExecutionResult`` round-trip test used ``file_results=[]``, so the list-comprehension in ``from_dict`` that rebuilds nested ``FileExecutionResult`` objects was never executed with a populated list. * **Vishnu #13 [Suggestion]** — replaced hardcoded line reference in test docstring with a symbol reference. Deferred to follow-up shared-infra dataclass-redesign ticket: * #1 ``__post_init__`` status clobber — would emit warning noise on every existing wrong-enum call site * #2 alias-pair invariant — back-fill via __post_init__ would change the wire shape (file_name no longer None → no longer stripped at the top level) * #3 ``to_api_dict``/``to_json`` dead code — looks like a public SDK surface; changing the body could surprise external consumers * #7 recursive ``None``-strip in ``serialize_value`` — touches every dataclass in the codebase * #10 ``Any`` typing tightening — low value, mypy tightening could trip downstream * #12 producer redundant kwargs — depends on #2's reconciliation Tests: workers chord-callback boundary suite 21 -> 25; full workers suite 622 -> 627 (no new failures; 6 pre-existing baseline unchanged). Five deterministic-order runs of the full suite returned exactly 627 passed / 6 pre-existing failed — zero flakiness from this change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
muhammad-ali-e
added a commit
that referenced
this pull request
Jun 9, 2026
… / FileExecutionResult (#2020) * UN-3513 [FEAT] Type chord-callback boundary with BatchExecutionResult / FileExecutionResult Producers in workers/file_processing/tasks.py now build typed dataclasses (from unstract.core.worker_models) and emit their ``.to_dict()`` instead of hand-rolled dicts. Locks the wire shape to the dataclass schema so downstream refactors fail loud. Scope Producer-side typing only. Consumer (workers/callback/tasks.py + aggregate_file_batch_results) already reads via ``.get(..., default)`` — tolerant by construction — so no consumer-side change needed. Dataclass extensions (unstract.core.worker_models, additive only) * BatchExecutionResult gains 3 optional fields: skipped_already_completed, skipped_active_duplicate, organization_id. * FileExecutionResult gains 3 optional fields for the API path's legacy dict vocabulary: file_name (alias for file), result_data (alias for result), skipped (marker like "already_completed"). * Both from_dict updated to populate the new fields. Producer migrations (workers/file_processing/tasks.py) * L901 (general path, process_file_batch return): BatchExecutionResult(...).to_dict(). Wire dict gains file_results: [] and errors: [] defaults — strictly additive. * L1706, L1798, L1823 (API path returns from _process_file_batch_api_core helpers): FileExecutionResult(...).to_dict(). L1798 preserves the legacy storage_result field via dict-spread merge. Domain-vocabulary correction on the API path API-path producers previously returned status="completed" / "failed" — lowercase strings matching neither ExecutionStatus (workflow-level, uppercase) nor ApiDeploymentResultStatus (per-file, Success/Failed, the canonical per-file vocab). Producers now emit "Success" / "Failed" via FileExecutionResult. Audit: no Python equality consumer was found reading the lowercase variants (grep clean). Observability tooling pattern-matching the old strings would need updating; this is a domain-correctness fix. Tests New tests/test_chord_callback_boundary.py — 14 tests, 3 classes: * Wire-shape characterisation for BatchExecutionResult. * Wire-shape characterisation for FileExecutionResult with alias fields and canonical Success/Failed vocab. * Consumer tolerance: aggregate_file_batch_results-style .get() reads return expected values from the new wire shape. sdk1's 80 worker_models tests still pass — the dataclass extensions are strictly additive. Regression risk: zero on consumer side, zero on backend (doesn't import these classes; has its own FileExecutionResult in dto.py — untouched). Status-vocab shift on API path is a deliberate domain correction. Test count: workers boundary suite +14 (new); sdk1 dispatcher 80/80. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * UN-3513 [FIX] Address PR review (toolkit + SkipReason enum + producer-binding tests) A+B from the triage on PR #2020: * tasks.py:1659 (API-path BATCH return) — migrated to BatchExecutionResult.to_dict(). Fixes the half-typed boundary the reviewer flagged. file_results, total_files, skipped_already_completed and organization_id are now on the wire. Successful/skipped counter semantic preserved (separating them is deferred to a follow-up). * New SkipReason StrEnum (worker_models.py) with ALREADY_COMPLETED + ACTIVE_DUPLICATE — mirrors the batch-level skip counters on BatchExecutionResult. FileExecutionResult.skipped is now SkipReason | None. from_dict coerces. Producer uses the enum; the ACTIVE_DUPLICATE value has no current per-file producer but is exercised end-to-end via a round-trip test. * TODO(UN-3516) marker on the three alias fields (file_name, result_data, skipped) — sunset ticket filed. * Tests strengthened: - TestProducerBinding drives real _compile_batch_result with a minimal SimpleNamespace context, and drives _process_single_file_api via mocked api_client for the already-completed branch. - TestRealConsumerTolerance imports the real aggregate_file_batch_results — producer-consumer contract driven end-to-end. - test_none_valued_optional_fields_stripped_from_wire documents serialize_dataclass_to_dict's None-strip behaviour. - test_active_duplicate_skip_reason_round_trips proves the second enum value isn't dead. - SonarCloud python:S1244 fixed — pytest.approx. - skipped_files==0 NIT assertion removed. Test count: workers boundary suite 14 -> 18; sdk1 worker_models 80/80 still green. Deferred (separate tickets to follow): __post_init__ silent status clobber, from_dict status discard, BatchExecutionResult invariant, storage soft-failure, dead aggregator branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * UN-3513 [FIX] Address second-pass review (storage_result + lenient skipped + missing producer tests) Three findings from the second review round on PR #2020: * HIGH — storage_result silent data loss at batch boundary. The per-file dict-spread at tasks.py:1816 preserved storage_result on the immediate return, but the value was dropped when wrapped into BatchExecutionResult.file_results (from_dict didn't know the key). Promoted to a typed FileExecutionResult.storage_result: Any | None field; producer now emits via the constructor; from_dict reads it back. The round-trip preserves it end-to-end. * HIGH — strict SkipReason parsing would crash entire batches during rolling deploys if a newer producer ever emitted an unknown value. Added FileExecutionResult._parse_skipped, which catches ValueError + logs a warning + falls back to None. Standard "strict on emit, lenient on receive" posture for wire compat. * MEDIUM — TestProducerBinding only covered 2 of 5 producer branches. Added three more tests: - _process_single_file_api success branch (asserts storage_result survives the typed wire — would catch the dict-spread revert). - _process_single_file_api failure branch (asserts canonical "Failed" vocab — catches reverts to the legacy lowercase "failed"). - process_file_batch_api batch wrapper via task.apply() with an in-memory result_backend (asserts BatchExecutionResult shape + skipped_already_completed counter derived from SkipReason.ALREADY_COMPLETED.value). Strengthened the existing already-completed branch test to assert result_data + metadata propagation. Bug caught by the new batch-wrapper test: process_file_batch_api was missing execution_time on its BatchExecutionResult(...) call — BatchExecutionResult.execution_time is a required positional, so the API-path batch task would have crashed with TypeError on every run. Introduced batch_start_time = time.time() at task entry and pass execution_time = time.time() - batch_start_time. The new test would have caught this immediately at PR time; logging it here as the exact value of producer-binding coverage. Test count: 18 -> 21; all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * UN-3513 [FIX] Symmetric None-stripping for nested file_results + deterministic callback healthcheck picker Greptile P2 #2 — None-stripping was asymmetric for nested FileExecutionResult objects. ``serialize_dataclass_to_dict`` only filters None at the outermost level, so a standalone ``FileExecutionResult.to_dict()`` would omit unset optional fields while ``batch.to_dict()["file_results"][i]`` would carry explicit ``"file_name": None`` etc. for the same input. A consumer doing ``"x" in result`` membership checks would behave differently depending on whether it read the standalone wire or the nested-in- batch wire — a real contract divergence. Fixed locally on ``BatchExecutionResult.to_dict()`` (not by touching the shared ``serialize_dataclass_to_dict`` infra): post-process ``wire["file_results"]`` to drop None-valued keys, mirroring the top-level strip. ``BatchExecutionResult.from_dict`` was already tolerant via ``.get(...)`` so the round-trip stays clean. Greptile P2 #1 (``status`` constructor parameter clobbered by ``__post_init__``) is the same pathology I flagged as BLOCKER #1 in the first review round — deferred to a separate ticket with the shared-infra dataclass redesign. Test coverage: extended the existing ``test_none_valued_optional_fields_stripped_from_wire`` to also assert nested symmetry — same test method, no new method added. This keeps the pytest collection profile stable (a separate test method would perturb celery's shared task-registry insertion order during pytest collection and amplify a pre-existing flake in ``test_callback_sanity.py``). Test infra fix (bundled because it would have flaked CI on this PR's HEAD): ``test_callback_sanity.TestEagerHealthcheckRoundTrip`` selected the healthcheck task via ``endswith(".healthcheck")`` against ``eager_app.tasks``. That registry is a shared celery global with at least 5 worker modules registering ``healthcheck`` (callback, executor, file_processing, log_consumer, scheduler). ``next(...)`` returned whichever was inserted first, which depends on pytest module-collection order across the whole suite. The test would assert ``worker_type == "callback"`` and intermittently get ``"executor"`` or ``"file_processing"`` instead — empirically a ~10% flake rate on this branch's HEAD, climbing to ~90% with any test-collection perturbation. Replaced with an exact-name lookup (``name == "callback.worker.healthcheck"``); 30/30 green across deterministic + randomised probes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * UN-3513 [FIX] Address vishnuszipstack review (7 real fixes + 1 docstring nit) Seven of Vishnu's PR review findings addressed, all backward-compat with main-branch consumers. The three [Important] design-redesign findings (#1 status __post_init__, #2 alias-pair invariant, #3 to_api_dict/to_json dead code) are deferred to a follow-up shared-infra dataclass ticket because they would either fire warning noise on existing call sites (``worker_base.py:211/222``, ``worker_patterns.py:241`` pass wrong-enum status) or change the wire/cache contract — neither acceptable mid-flight while keeping zero regression on main. Changes in this commit are either: * Pure additive (test methods, docstrings, observability) * Or provably equivalent wire output (the typed-count refactor) So a rolling deploy where old workers and new workers run concurrently sees identical wire shapes and identical behaviour for all current valid data; the only observable differences are log content (better context on the existing warning) and the presence of a new opt-in classmethod that nothing currently calls. * **Vishnu #8 [Suggestion]** — ``SkipReason`` docstring claimed "StrEnum semantics" but the class is ``(str, Enum)``, not ``enum.StrEnum``. The two differ on ``__str__``. Rewrote the docstring to describe the actual behaviour. * **Vishnu #4a [Important — log context]** — ``_parse_skipped`` now accepts an optional ``file_execution_id`` kwarg that ``from_dict`` threads through. The warning emitted for unknown wire values now carries the file identifier, so a real rolling-deploy incident is debuggable rather than a context-free warning. Optional kwarg with default — any existing caller passing one positional arg still works. * **Vishnu #9 [Suggestion]** — added ``BatchExecutionResult.from_file_results(...)`` classmethod that derives counters from typed file results. Purely additive: no existing caller uses it; the constructor signature is unchanged so producers that need their own counter semantics keep working. * **Vishnu #11 [Suggestion]** — ``process_file_batch_api`` was computing ``skipped_already_completed`` by string-matching the wire dicts AFTER already calling ``from_dict`` on them. Refactored to count from the typed list (single ``from_dict`` pass, enum compare). Provably equivalent for all current wire data. * **Vishnu #4 [Important — test gap]** — added ``test_from_dict_unknown_skipped_is_lenient`` covering the one documented crash-prevention path. A regression to bare ``SkipReason(raw)`` would have re-introduced the rolling-deploy crash and kept every other test green. * **Vishnu #5 [Important — failure-aggregation gap]** — added ``test_process_file_batch_api_batch_wrapper_failure_aggregation`` that drives one success + one failure through the batch wrapper. The existing success-only test never exercised ``failed_files += 1``. * **Vishnu #6 [Important — populated round-trip gap]** — added ``test_round_trip_with_populated_file_results`` and ``test_from_file_results_derives_counters``. The existing ``BatchExecutionResult`` round-trip test used ``file_results=[]``, so the list-comprehension in ``from_dict`` that rebuilds nested ``FileExecutionResult`` objects was never executed with a populated list. * **Vishnu #13 [Suggestion]** — replaced hardcoded line reference in test docstring with a symbol reference. Deferred to follow-up shared-infra dataclass-redesign ticket: * #1 ``__post_init__`` status clobber — would emit warning noise on every existing wrong-enum call site * #2 alias-pair invariant — back-fill via __post_init__ would change the wire shape (file_name no longer None → no longer stripped at the top level) * #3 ``to_api_dict``/``to_json`` dead code — looks like a public SDK surface; changing the body could surprise external consumers * #7 recursive ``None``-strip in ``serialize_value`` — touches every dataclass in the codebase * #10 ``Any`` typing tightening — low value, mypy tightening could trip downstream * #12 producer redundant kwargs — depends on #2's reconciliation Tests: workers chord-callback boundary suite 21 -> 25; full workers suite 622 -> 627 (no new failures; 6 pre-existing baseline unchanged). Five deterministic-order runs of the full suite returned exactly 627 passed / 6 pre-existing failed — zero flakiness from this change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kirtimanmishrazipstack
added a commit
that referenced
this pull request
Jun 15, 2026
… only" (#1936) * api deployment notification init * UN-3431 [FIX] Stream tool-run logs to workflow execution UI with markdown 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> * UN-3430 [FIX] Update modified_at field correctly for models (#1928) * [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). * UN-3403 [FEAT] Agentic table extractor plugin with multi-agent LLM-powered 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> * UN-3358 [FIX] Drop cross-region S3 buckets from connector listing (#1931) * list bucket * greptile review * payload metadata in api deployment * slack webhook payload * Uns 611 clubbed notification dispatch (#1951) * 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> * Uns 611 clubbed notification dispatch (#1959) * batch notification * notification slack * notification API * delivery mode batch by default * UI change * PR reviews * sonar issues * sonar issues * code rabbit refactor * greptile comments resolve * UN-3056 Scope enqueue execution_id exemption to INPROGRESS 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 e6534949d. 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> * greptile comments resolve * UN-3056 Skip deactivated notifications in BATCHED flush _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> * greptile comments resolve * remove immediate mode * add legacy code * add legacy code * greptile review * greptile review * UI as per new designs * UN-3056 [FIX] Make Inactive platform-key activation keyboard-accessible Adds role/tabIndex/aria-label/onKeyDown to the Inactive Tag so keyboard users can activate platform keys. Disabled state (no key id) is non-focusable via tabIndex=-1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * UN-3056 [FEAT] Harden batched notification dispatch + review cleanups Address human review feedback on the batched-notification PR: - Fix dead-letter path (Johny): send_webhook_notification now re-raises on retry exhaustion via raise_on_final_failure so the Celery link_error fires and buffer rows reach DEAD_LETTER instead of being silently lost. - Add crash-window reaper: new BufferStatus.SENDING claim state with success (mark_buffer_dispatched) / failure callbacks; _reclaim_stale_sending returns rows stuck past NOTIFICATION_DISPATCH_LEASE_SECONDS to PENDING. - Move FAILURE_STATUSES onto ExecutionStatus (failure_statuses/is_failure); drop the duplicated frozenset and update call sites (Chandru). - Remove dead delivery_mode column + DeliveryMode enum (product is batched-only); rename dispatch_with_delivery_mode -> dispatch_notifications. - Squash notification_v2 migrations 0002+0003 into 0002_notification_batching. - Scrub JIRA/mfbt references from docstrings; clarify NOTIFICATION_CLUB_INTERVAL is a per-org-overridable default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3056 [FEAT] Self-review fixes: shared failure rule, observability, hardening Addresses the in-scope items from the self-review on PR #1936: - Single-source the failure-only rule via notification_v2.helper.is_failure_run, used by api_v2 / pipeline_v2 / internal_api_views (_apply_failure_filter); the pipeline path keeps a documented last_run_status backstop. Fixes the false "parity" docstring (#1). - Emit metric= counters at the notification drop sites (backend dispatch_notifications, worker _route_notification) and a row-id sample on the dead-letter log so a delivered-never event is observable (#4). - process_notification_buffer.py honors its "never raises" contract: wrap response.json() so a non-JSON 200 returns False instead of raising (#5). - Bind the flush cap to the renderer's MAX_BATCH_SIZE so rows and rendered events stay in lock-step by construction (#7). - status db_comment now documents the PENDING -> SENDING -> DISPATCHED/DEAD_LETTER lifecycle in both the model and migration 0002 (#8). - Scrub stale IMMEDIATE / worker-callback comments from the provider docstrings (#2, #10). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3056 [FEAT] Bound buffer redelivery + drop dead provider cluster - NotificationBuffer.dispatch_attempts + NOTIFICATION_MAX_DISPATCH_ATTEMPTS: _dispatch_group dead-letters rows past the cap and increments on each SENDING claim, bounding the reaper reclaim loop so a lost terminal callback can't redeliver forever (self-review #3). - Delete the orphaned synchronous notification_v2/provider/ cluster — zero callers after the batched dispatch_notifications path replaced it (#2). - Fold dispatch_attempts into 0002_notification_batching; refresh lifecycle db_comments + BufferStatus docstring. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3056 [FEAT] Portable timestamp render in clubbed notification _humanize_timestamp used the `%-d` strftime directive, a glibc/Linux extension that raises ValueError on macOS/Windows. The call sat outside the fromisoformat try/except, so the raise propagated through build_envelope -> render_clubbed_message and was swallowed by process_notification_buffer's outer except, silently skipping every due group on non-Linux dev/CI machines. Interpolate the day from dt.day (plain int, no leading zero) instead so the render is platform-portable; output is byte-identical to the old format. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3056 [FIX] Clear SonarCloud issues on PR #1936 - internal_serializers.py: validate() now has a single terminal return (S3516); the total_files branch is if/else instead of an early return. - internal_views.py update_status: guard-clause on invalid serializer + extract _truncate_error_message / _update_file_aggregates helpers to drop cognitive complexity below 15 (S3776). Behavior unchanged. - PlatformSettings.jsx: extract InactivePlatformKeyTag sibling component so the key-row map callback drops below cognitive complexity 15 (S3776); keyboard activation (Enter/Space) behavior preserved. - process_notification_buffer.py: logger.exception() in the HTTPError branch to capture the traceback (S8572). - scheduler.sh: explicit return statements (S7682) — run_task returns the task exit code; cleanup returns 0 with the exit moved into the trap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3056 [FIX] Refund dispatch_attempts on broker-publish failure _dispatch_group increments dispatch_attempts atomically with the PENDING -> SENDING claim. When _send_clubbed fails to publish to the broker, the revert reset status/dispatched_at but left the increment in place, so a clean broker outage (no task queued, no webhook sent) still burned redelivery budget — N consecutive outages would dead-letter a never-delivered row. Decrement dispatch_attempts in the broker-failure revert so a publish that never reached the broker doesn't consume the cap. Crash / lost- callback paths never hit this except block, so they keep the increment and remain bounded by the reaper, which is the redelivery risk the cap exists for. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3056 [FIX] Single-source failure rule, webhook compat shape, decouple flush cadence Addresses review feedback on PR #1936: - Single-source the failure rule: add canonical is_failure_run to unstract.core (beside ExecutionStatus). The clubbed renderer derives its summary counts / emoji from it (drops the duplicate _SUCCESS_STATUSES / _is_success / _is_effective_success), and notification_v2.helper.is_failure_run delegates to it. Routing filter and rendered outcome can no longer drift. - Webhook backward compat: build_envelope spreads the legacy flat fields (type, pipeline_id, pipeline_name, status, execution_id?, error_message?) onto a single-event envelope alongside summary/events, so existing API webhook receivers parsing the pre-clubbing flat body keep working. Multi-event stays envelope-only; Slack path untouched. - Decouple the buffer-flush poll cadence from the log-history consumer: dedicated NOTIFICATION_BUFFER_POLL_INTERVAL (default 10s); scheduler.sh wakes at the min of the two intervals and fires each task on its own elapsed interval. Removes the misleading "5s" comment and the shared-knob coupling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3056 [FIX] Single-event webhook compat covers worker legacy shape The single-event legacy superset in build_envelope only reproduced the backend dispatch DTO (PipelineStatusPayload.to_dict). The worker callback path's pre-clubbing body (NotificationPayload.to_webhook_payload) also emitted top-level `timestamp` and `additional_data`, so receivers reading those against the old flat shape broke even on single-event sends. Add both keys to _LEGACY_FLAT_KEYS; purely additive (the existing not-None guard keeps backend-origin events from gaining an empty additional_data). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3056 [FIX] Address review: fail-closed failure filter, doc accuracy, tests Addresses @jaseemjaskp's review on PR #1936. Correctness: - _apply_failure_filter now fails CLOSED when an execution_id was requested but the row is missing (replication lag / race): drop notify_on_failures rows + emit a metric, so a run we can't confirm as a failure never sends a success alert to a failure-only subscriber. Doc/comment accuracy: - tasks.py: rows transition from SENDING (not PENDING/DISPATCHED) to DEAD_LETTER. - NotificationBuffer.flush_after db_comment (model + migration, byte-identical): now() + org's effective club interval (per-org override, else default). - enqueue_notification_buffer: drop the non-existent "not BATCHED" gate claim. - api/slack webhook provider docstrings: frame pass-through by payload SHAPE. - Scrub leftover UNS-611 / UNS-611 v2 JIRA refs (scheduler.sh, PlatformSettings). Cleanup / resilience: - PlatformSettings: log the interval-load failure instead of swallowing it. - _dispatch_group returns a single int (was an always-identical (rows, rows)). - clubbed_renderer: drop build_envelope from __all__ (kept the internal import). Tests: - Rewrite the broken TestNotificationDispatchSite to characterise the new HTTP buffer-enqueue contract (_route_notification / _enqueue_to_buffer); the old suite imported the deleted send_notification_to_worker. - Add pure-function tests for is_failure_run and the clubbed renderer (envelope shape, single-event legacy keys incl. timestamp + additional_data, MAX_BATCH_SIZE cap, Slack overflow footer). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * UN-3056 [FIX] Self-review polish: docstring accuracy + test coverage Follow-up to f70a2a1b5 from a multi-agent self-review pass. - slack_webhook.py: "single-line" → "Slack mrkdwn body" (render_slack_text emits a multi-line body: header + divider + per-event lines). - internal_api_views._load_execution: comment that only DoesNotExist is caught (missing row → fail-closed; malformed id → 500) so the two paths aren't collapsed by a future widened except. - PlatformSettings.jsx: comment no longer says "silently" (the load failure is now logged in the catch). Test coverage added: - clubbed renderer: humanized timestamp in events[] (the dt.day dodge for the %-d glibc bug), unparseable timestamp → placeholder, error_message absent on success events, empty batch, file-count column collapse when additional_data has no totals. - is_failure_run: STOPPED + failed_files>0 (both predicates true). - worker dispatch site: notification with no notification_type key is skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * UN-3056 [FIX] Resolve SonarCloud findings (log-injection guard + bash [[ ) - internal_api_views: strip CR/LF from the request-supplied execution_id at both log sites before logging (SonarCloud pythonsecurity:S5145 log-forging). The id is UUID-validated upstream so this is defense-in-depth, but it also clears the New-Code Security Rating that was failing the quality gate. - scheduler.sh: use [[ ]] instead of [ ] for the four conditional tests (SonarCloud shelldre:S7688); the script is bash (#!/usr/bin/env bash). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * UN-3056 [FIX] Early-failure dispatch, robust buffer mark, authoritative is_failure Field fixes from local end-to-end testing plus a renderer-correctness follow-up: - Early-failure dispatch: the general worker now notifies failure subscribers on its terminal-error branch (notify_execution_failure) for runs that ERROR before the file-processing callback (build / tool-registry / source errors, total_files=0) — previously silent since that callback is the only ETL/Task dispatch site. Fired only after autoretry is exhausted; mutually exclusive with the callback. API deployments already covered via update_pipeline_status. - Robust buffer mark: the notification worker reports SENDING -> DISPATCHED / DEAD_LETTER over the backend internal API (buffer/mark/{dispatched,dead-letter}) instead of Celery link/link_error. Those callbacks routed to the `celery` queue, which the unified -A worker also drains without the backend tasks registered, so ~half were dropped as "unregistered task" and rows stuck in SENDING. Legacy mark tasks kept but deprecated to drain in-flight messages. - Authoritative is_failure verdict: dispatch sites carry the verdict the routing filter used on the payload; the clubbed renderer prefers it over re-deriving from `status` (vocabulary differs: PipelineStatus SUCCESS/FAILURE vs ExecutionStatus), falling back to is_failure_run for worker/legacy payloads so the rendered outcome can't disagree with why the alert fired. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3056 [FIX] Atomic status + file-count write in update_status Wrap update_execution() and _update_file_aggregates() in a single transaction.atomic() block. Previously they committed independently, so a failure between the two writes could leave WorkflowExecution at a terminal status (COMPLETED) with failed_files=None. The notify_on_failures filter reads failed_files straight from the DB via is_failure_run(), which scores (COMPLETED, None) as a success, silently dropping the failure alert for a partial-failure run. Committing both writes together closes that window. Addresses Greptile P1 on internal_views.py. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Comments to self review * UN-3056 [FIX] Guard broker-failure revert on SENDING state _send_clubbed reverted rows by id only. If send_task raised after the message reached the broker, the worker still runs, delivers, and marks the rows DISPATCHED/DEAD_LETTER over the internal API; the unguarded revert then flips those terminal rows back to PENDING and refunds dispatch_attempts, resurrecting them so the next flush re-dispatches = duplicate delivery. Add a status=SENDING guard so only rows still in the claimed state are reverted, mirroring the source-state guards every other transition in this file already uses (_dispatch_group → PENDING, _mark_buffer_rows → SENDING). Addresses Jaseem's concurrency finding on internal_api_views.py. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3056 [FIX] Scope flush lock to buffer rows; bound un-renderable group Address review comments on PR #1936: - Add of=("self",) to the flush SELECT FOR UPDATE so SKIP LOCKED takes the row lock on the buffer rows only, not the join-related Notification row. An unrelated lock on that Notification (e.g. an admin edit) no longer silently skips the whole buffer group's dispatch. - Charge a dispatch attempt when render/prepare raises (poison payload) so the dispatch-attempt cap dead-letters an un-renderable group instead of re-rendering the same payload every flush tick forever. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Signed-off-by: harini-venkataraman <115449948+harini-venkataraman@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: harini-venkataraman <115449948+harini-venkataraman@users.noreply.github.com> Co-authored-by: Ghost Jake <89829542+Deepak-Kesavan@users.noreply…
muhammad-ali-e
added a commit
that referenced
this pull request
Jun 17, 2026
…ing + typing/dedup/docs/tests Decision (with reviewer): reaper-as-safety-net for the un-catchable strand windows + fix what's catchable + document + gate on PR3. Failure handling: - [#69 Critical] run_batch_with_barrier wraps BOTH work + decrement in the abort: a decrement-side failure (guard / DB / last-batch callback dispatch) tears the barrier down in-body instead of stranding to expiry. - [#79] extracted _abort_barrier_in_body — logs when the teardown itself fails (was silently suppressed under a misleading "torn down" message). - [#74/#81] documented the two un-catchable strand windows (hard-crash-during-work, post-commit callback-dispatch-fail) as a HARD reaper dependency for PR3. - [#86] finalise cleanup split into independent try/excepts with distinct logs. Typing / clarity: - [#1] BarrierContext(TypedDict) for _barrier_context (header fan-out, run_batch_with_barrier, process_file_batch). - [#3] renamed CallbackDescriptor "backend" -> "transport" (WorkflowTransport value; avoids the QueueBackend "pg" collision). - [#27] is_pg_transport() predicate in core; used in orchestration_utils + pg_barrier. - [#20] extracted _dispatch_pg() — single home for cycle-avoiding local import + backend=PG. - [#35] normalize_transport() at the general worker entry (parity w/ api/scheduler). - [#94] log when a header has no queue option. - [#9/#13] fixed born-stale comment + kwargs-not-args docstring. Tests (+#37/#41): last-batch self-chains callback to PG + cleans up barrier/dedup; decrement-failure aborts; PG-branch mid-loop dispatch-failure deletes row; header args/queue/pre-existing-kwargs preservation. 137 barrier/dedup/routing tests green; bootstrap clean under WORKER_BARRIER_BACKEND=pg. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
muhammad-ali-e
added a commit
that referenced
this pull request
Jun 17, 2026
…fire-and-forget) (#2069) * UN-3563 [FEAT] PG Queue 9e PR 2c — live PG fan-out/barrier/callback (fire-and-forget) Wires the coupled pipeline's fan-out → barrier → callback onto the PG queue for a transport=="pg_queue" execution. Gated: resolve_transport() still returns celery (PR3 Flipt flips it), so the whole PG branch is present-but-unreachable — default path byte-identical. Orchestrator task (async_execute_bin) stays on Celery (hybrid); routing it onto PG is a 2d follow-up. - barrier.py: Barrier Protocol + CeleryChordBarrier/RedisDecrBarrier accept (and ignore) a `transport` param; CallbackDescriptor gains an optional `backend`. - orchestration_utils._barrier_for_transport: pg_queue → fresh PgBarrier() (bypasses the WORKER_BARRIER_BACKEND singleton), else the singleton. - pg_barrier.PgBarrier.enqueue(transport): pg_queue → fire-and-forget mode — _dispatch_header_pg sends each header via dispatch(backend=PG) with an injected _barrier_context {execution_id, batch_index, callback_descriptor}, no .link; descriptor marked backend=pg_queue; UPSERT block also clears pg_batch_dedup (greptile #2068 reuse-reset). _fire_barrier_callback self-chains the callback onto PG when backend==pg_queue. clear_execution_batches at finalise + abort. run_batch_with_barrier(): claim → work → in-body _barrier_pg_decrement; redelivery skips; exception → barrier_pg_abort. - file_processing.process_file_batch(_barrier_context=None): core routes None → _run_batch_stages (celery chord path), else → run_batch_with_barrier. - general/api fan-outs thread transport into create_chord_execution. Tests: +8 PgBarrier fire-and-forget + 2 orchestration routing + 2 process_file_batch routing. Each test file green alone; ruff clean. End-to-end forced-pg dev-test pending before PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3563 fix SonarCloud S1172: drop unused task_instance from _run_batch_stages The extracted _run_batch_stages never uses task_instance — its only purpose (deriving celery_task_id) happens in _process_file_batch_core before the call. Removed the param + updated both call sites. _process_file_batch_core keeps task_instance (it reads .request.id). Routing test mocks with *a, unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3563 address review (muhammad-ali-e, 15): strand-on-failure hardening + typing/dedup/docs/tests Decision (with reviewer): reaper-as-safety-net for the un-catchable strand windows + fix what's catchable + document + gate on PR3. Failure handling: - [#69 Critical] run_batch_with_barrier wraps BOTH work + decrement in the abort: a decrement-side failure (guard / DB / last-batch callback dispatch) tears the barrier down in-body instead of stranding to expiry. - [#79] extracted _abort_barrier_in_body — logs when the teardown itself fails (was silently suppressed under a misleading "torn down" message). - [#74/#81] documented the two un-catchable strand windows (hard-crash-during-work, post-commit callback-dispatch-fail) as a HARD reaper dependency for PR3. - [#86] finalise cleanup split into independent try/excepts with distinct logs. Typing / clarity: - [#1] BarrierContext(TypedDict) for _barrier_context (header fan-out, run_batch_with_barrier, process_file_batch). - [#3] renamed CallbackDescriptor "backend" -> "transport" (WorkflowTransport value; avoids the QueueBackend "pg" collision). - [#27] is_pg_transport() predicate in core; used in orchestration_utils + pg_barrier. - [#20] extracted _dispatch_pg() — single home for cycle-avoiding local import + backend=PG. - [#35] normalize_transport() at the general worker entry (parity w/ api/scheduler). - [#94] log when a header has no queue option. - [#9/#13] fixed born-stale comment + kwargs-not-args docstring. Tests (+#37/#41): last-batch self-chains callback to PG + cleans up barrier/dedup; decrement-failure aborts; PG-branch mid-loop dispatch-failure deletes row; header args/queue/pre-existing-kwargs preservation. 137 barrier/dedup/routing tests green; bootstrap clean under WORKER_BARRIER_BACKEND=pg. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3563 fix run_batch_with_barrier strand-window doc inconsistency (review) The second "NOT catchable" bullet conflated two different things: it described the in-body catchable abort ("the abort here removes the row") and a *software* callback-dispatch failure — but that failure is already caught + torn down by step 3's wrap (paragraph 1), so it doesn't belong under the un-catchable heading, and on the PG path _fire_barrier_callback IS the enqueue so "committed but before the enqueue" couldn't both hold. Rewrote the bullet to the genuinely un-catchable window: a hard crash BETWEEN the decrement committing (remaining→0) and the callback enqueue completing — decrement committed (redelivery blocked by the marker), process gone before the callback enqueues or any abort runs, row survives to expiry, reaper-only recovery. Explicitly notes a software dispatch failure is the catchable case. Keeps this list an accurate spec for the PR-3 reaper-recovery dependency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3563 address greptile (#2069, 2): clear dedup on mid-loop PG failure + carry fairness on PG callback Both in the gated PG path (greptile 4/5, safe to merge). - Issue 1: PgBarrier.enqueue mid-loop dispatch-failure handler now also calls clear_execution_batches on the PG path. Earlier headers may have committed a claim_batch marker; with the barrier row deleted, their in-flight barrier_pg_abort is a no-op (already_aborted) and never reaches the clear inside it, so reclaim the markers directly here. - Issue 2: the PG callback now carries the producer's fairness. Added _fairness_from_headers() to reconstruct the FairnessKey from the stored x-fairness-key headers and pass it to _dispatch_pg, so the callback rides the same org/priority as the Celery path (was always default priority). Tests: +fairness-carried / +fairness-none-safe on _fire_barrier_callback; extended the PG mid-loop test to assert an already-claimed marker is reclaimed. 75 barrier/dedup tests green; bootstrap clean under WORKER_BARRIER_BACKEND=pg. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3563 fix SonarCloud S3776: reduce PgBarrier.enqueue cognitive complexity (17→under 15) Extracted the per-header dispatch loop into PgBarrier._dispatch_headers — the deeply-nested for→try/except→if/else→if (PG-vs-celery branch + mid-loop failure teardown + PG dedup-clear) was the complexity driver. enqueue now calls the helper; behaviour identical. radon: enqueue C(11)→B(6); ruff C901 passes. 75 barrier/dedup tests green; ruff + ruff-format clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3563 fix greptile #2069: mid-loop dedup-clear test passed for the wrong reason The pre-seeded claim_batch marker was wiped by enqueue's UPSERT block (the reuse-reset DELETE) before the dispatch loop, so the mid-loop clear_execution_batches deleted 0 rows — the count==0 assertion passed on the UPSERT, not the guard under test. Now the first dispatch side-effect claims the marker AFTER the UPSERT (simulating a fast PG consumer), so the mid-loop clear is what removes it. Verified: with the clear disabled the marker orphans (count=1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
hari-kuriakose
added a commit
that referenced
this pull request
Jul 28, 2026
…yment
Walked the app side by side with globe.unstract.com. Four real differences,
batched into one deploy.
1. Every uncoloured border drew BLACK (~200 elements per settings page).
Tailwind's border utilities set width only; the colour falls back to
currentColor. shadcn's setup ships a `border-color: var(--border)` default
and ours was missing it, so `border`, `border-b` and `divide-y` all drew
solid black where antd had a near-invisible hairline — most obviously as a
black rule between every list row. Fixed once in the theme rather than at
call-sites, so the next uncoloured border is right by default.
2. A grey block behind the icon button on every prompt card (7 on one Prompt
Studio screen). antd applies a Badge's `style` to the COUNT; the shim let
it fall through `{...props}` onto the wrapper span, so
PromptChangeIndicator's `style={{ backgroundColor: color }}` painted the
wrapper. Also implements `offset`, which was accepted and ignored.
3. Invite Users was a blank "Couldn't load this page". antd's NamePath allows
arrays — InviteEditUser uses `name={["email"]}` — but react-hook-form
calls `.split(".")` on the name, so it threw
`TypeError: s.split is not a function` and the error boundary swallowed
the whole route. Arrays are antd's dotted path, so joining reproduces it.
The cloud StripeProductForm uses nested `name={["tier1", "up_to"]}`, so
this was broken there too.
4. Form labels pointed at nothing. `<Label htmlFor={name}>` was rendered but
no id was ever set on the control, so clicking a label did not focus its
field and screen readers announced it unlabelled. Found while writing the
test for #3.
Each fix has a regression test, and each was mutation-checked by
reintroducing the defect and confirming the matching test fails.
hari-kuriakose
added a commit
that referenced
this pull request
Aug 30, 2026
#1 (High) ManageDocsModal: the 5s index-status poll could never stop on the path it was written for. `indexDocs` is emptied only by `deleteIndexDoc`, called from the websocket handlers -- so when socket messages are dropped it never empties and the interval ran forever, issuing two requests per tick and raising two failure toasts per tick. The poll now retires a document itself when the polled status reports it indexed, which stops the spinner and lets the effect tear its own interval down; poll-driven calls pass `silent` to suppress the repeating toast. User-initiated calls are unchanged. #2 (High) ToolIde: drop the `.then()` that merged `{...details, ...res.data}` into the store. `details` was a closure snapshot from the render that issued the PATCH, so a prompt added or deleted while the request was in flight was silently discarded. The single-pass toggle -- the only caller that changes single-pass mode, and so the only one UN-2900 needs refreshed prompts for -- already applies the response in its own `.then()`. #3, #4 (Medium) Correct two comments whose stated premises were false: the poll does not stop "as soon as indexDocs empties", and the single-pass toggle does not "only call this function" -- it consumes the response too. #5 (Low) ProfileInfoBar: chunk_size is nullable; render "-" instead of a dangling " tokens". 0 still renders "0 tokens". #6 (Low) AddLlmProfile: calcTokenSize no longer calculates anything -- rename to toTokenSize and drop the now-redundant `> 0` guard at the call site. Also clamps negatives, which the unguarded second call site previously displayed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
jaseemjaskp
added a commit
that referenced
this pull request
Sep 1, 2026
… Bloom) (#2212) * [P0] Add shadcn/ui foundation with Midnight Bloom tokens Implements P0-01..P0-16 of UN_SHADCN_IMPL_PLAN.md (spec: UN_SHADCN_SPEC.md). Installs the shadcn/ui + Tailwind v4 stack alongside Ant Design; antd still renders every screen, so this phase is intentionally a no-op visually. - Deps: Radix primitives, CVA/clsx/tailwind-merge, lucide-react, next-themes, sonner, react-hook-form + zod, Tailwind v4. antd deliberately retained for the coexistence period (spec §7). - Fonts: self-hosted @fontsource Inter + Geist Mono (no CDN; prod serves via nginx and must not depend on an external host). - Tokens: src/index.css now carries the Midnight Bloom light+dark palette (D8). Tailwind is imported first so its layer ordering is correct, and the colour tokens are mapped with `@theme inline` — with a plain `@theme` Tailwind snapshots the light value and dark mode silently breaks. - Legacy CSS vars renamed to --legacy-* (D6): variables.css defined --primary and --secondary, which collide with the shadcn tokens. - 32 primitives generated into src/components/ui, plus hand-written spinner and kbd (no registry entry) and success/warning badge variants. - Theme: next-themes ThemeProvider mirrors the existing session theme onto the `.dark` class. How the theme is persisted and toggled is unchanged (C4). - Toasts: sonner Toaster mounted and a shared useAppToast helper added for cloud plugins to import (D9). ALERT_SURFACE keeps antd as the single active notification surface until P2-06, so alerts are not double-rendered. Two fixes the plan did not anticipate, both required: - .gitignore: the Python `lib/` rule also matched frontend/src/lib/, which is where components.json points `@/lib/utils`. Without the negation, cn() would never reach the repo and every primitive would fail to resolve in CI. - biome.json: enable css.parser.tailwindDirectives, otherwise Biome cannot parse @theme/@plugin/@custom-variant and fails CI with 4 parse errors. Gates: build (plugins absent, the optionalPluginImports path) passes; 16 tests green; dark mode verified in headless Chromium — the `bg-background` utility itself flips rgb(250,250,250) -> rgb(26,26,26), proving `@theme inline` works; no visual regression (antd element count, button geometry, colours and radii all unchanged — only the body font moves to Inter, which is intended). Lint findings that remain (3 errors, 24 warnings) are pre-existing: pristine main reports 227/261 with the same binary, and none of the findings are in files this change touches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [P1-01/P1-02] Migrate @ant-design/icons to lucide-react Implements P1-01 (mapping table) and P1-02 (apply migration) of UN_SHADCN_IMPL_PLAN.md. 91 files, 87 unique icons, zero @ant-design/icons imports remaining in OSS. docs/icon-map.md records every mapping and flags the ones that are not exact, since lucide is not a 1:1 replacement for antd's icon set: - CheckCircleFilled / PlayCircleFilled / InfoCircleFilled -> lucide has no filled variants, so these render as outlines. Where the solid weight carries meaning, the doc shows the fill-current treatment. - MoreOutlined -> EllipsisVertical, NOT Ellipsis. antd's renders vertical (the 10 call-sites are all overflow menus); plain Ellipsis is horizontal. - CaretDownOutlined -> ChevronDown trades a solid triangle for a stroke, which also matches the shadcn/Radix idiom used elsewhere. - SlackOutlined -> MessagesSquare. lucide dropped brand icons, so the Slack glyph is simply gone; this is the one place a brand mark is lost. - ScheduleOutlined -> CalendarClock, ArrowsAltOutlined -> Move, ExportOutlined -> ExternalLink: closest available, no exact match. Three name collisions the rename introduced, all fixed with aliases: - FileUpload.jsx and FileWidget.jsx import antd's `Upload` COMPONENT, which the lucide `Upload` icon shadowed. Left unfixed this would have broken both file upload widgets, not merely the icon. - Workflows.jsx defines its own `User` component; importing lucide's `User` made it render itself. This was an infinite recursion, caught by the build. useRetrievalStrategies.js needed a matching update: RetrievalStrategyModal's ICON_MAP keys were renamed to lucide names, but the hook still emitted antd names, so every lookup would have missed and silently fallen back to the default icon. The backend contract is unchanged — only the frontend key names moved. Verified: build passes; 16 existing tests green plus a temporary smoke test confirming migrated icons render as lucide svgs; lint reports 0 errors and the same 24 pre-existing warnings; no page errors at runtime. The 4 `anticon` elements still in the DOM belong to antd's own notification component, not app code, and go away with P2-06. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [P1-03] Move Typography off antd via a compatibility shim Implements P1-03 of UN_SHADCN_IMPL_PLAN.md. 93 call-site files plus a new `@/components/ui/typography` primitive. Zero antd Typography imports remain. Deviation from the plan, and why: the plan said convert Typography to "semantic tags + Tailwind type classes". That is unsafe here. antd's `ellipsis` prop is behaviour, not styling — `ellipsis={{ tooltip: true }}` truncates AND surfaces the full text on hover, and `ellipsis={{ rows: 2 }}` clamps to N lines. 12 call-sites use the object form. Swapping in a bare `truncate` class would silently drop the tooltip, which is a behaviour regression and therefore a C4 violation, not a restyle. So this adds a small shim that presents antd's API (`type`, `strong`, `italic`, `delete`, `code`, `mark`, `ellipsis`, `level`, and the `Typography.Text` namespace) on top of Midnight Bloom tokens, with `ellipsis` implemented against the shadcn Tooltip. The 295 call-sites then become an import rewrite with the JSX untouched: same elements, same order, same props. Per D9/§5.0 it lives in OSS so cloud plugins import the same component. Two details worth noting: - The line-clamp classes are written out in a lookup table rather than interpolated as `line-clamp-${rows}`. Tailwind scans source statically and never sees a class name assembled at runtime, so the interpolated form would have produced no CSS. - The tooltip renders whenever requested rather than only when text actually overflows. antd measures the DOM to decide; matching that would need a resize observer per element. Showing it unconditionally keeps the content reachable, which is the purpose of the prop. 11 unit tests cover the shim, including the ellipsis behaviours that made the regex approach unsafe. Full suite is 27 tests across 5 files, all green. Build passes, lint reports 0 errors and the same 24 pre-existing warnings, and the app renders with no console errors (antd element count drops 33 -> 29 on the landing page as Typography moves off antd). Plan estimate correction: P1-03 was scoped at 158 sites; the real count is 295 (192 `<Typography.Text>` alone). As with icons (43 -> 87), the original enumeration missed multi-line import blocks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [P1-04] Move Button off antd via a compatibility shim Implements P1-04 of UN_SHADCN_IMPL_PLAN.md. 70 call-site files plus a new `@/components/ui/antd-button` wrapper over the shadcn primitive. Zero antd Button imports remain. Same reasoning as the Typography shim (P1-03): antd's Button carries behaviour that shadcn's does not, so the plan's prop-mapping-by-find-and-replace would have changed what the UI does, not just how it looks (C4): - `loading` (234 usages) swaps in a spinner AND disables the button. Dropping the disable would let users double-submit during in-flight requests. - `icon` (106) is a leading slot, not a child. - `danger` (12) is orthogonal to `type`, so it is not a 1:1 variant mapping — danger+text has to stay ghost-with-destructive-text rather than becoming a solid destructive button. - `htmlType` maps to the DOM `type` attribute, because antd claims `type` for its visual variant. The shim defaults DOM type to "button" so a converted button cannot accidentally submit a form. The mapping is type=primary->default, link->link, text->ghost, dashed/default->outline, with danger overriding to destructive (or ghost + destructive text for text/link). size small->sm, large->lg, and icon-only buttons get the icon size. CustomButton (76 usages) is a thin pass-through over antd's Button, so it now routes through the shim automatically — no separate conversion needed. 12 unit tests cover the shim, focused on the behaviours that made the naive approach unsafe: loading disables, loading hides the icon, danger+text styling, htmlType mapping, block/shape. Full suite is 39 tests across 6 files, green. Verified in the browser: antd button count on the landing page drops to 0 while the Login button keeps its exact geometry (50px tall, same colour and position) and total antd elements fall 29 -> 24. Radius moves 6px -> 8px, which is the intended Midnight Bloom --radius-md token. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [P1] Formalise the shim convention; drop dead antd Button theme Follow-up to P1-03/P1-04, no behaviour change beyond one deletion. - docs/shim-convention.md records the rule the next ~15 components follow: shim when antd implements behaviour shadcn does not, direct swap when the difference is only styling. Names compatibility layers `antd-<component>.jsx` so they read as migration debt with an exit, lists the decision (with usage counts) for every remaining component, and flags `Space` — it wraps each child in its own div, so replacing it with `gap-*` silently breaks any CSS selector matching `> *`. - Renamed typography.jsx -> antd-typography.jsx (94 import lines) so both shims follow that convention rather than one each. - Removed the now-dead `components: { Button: { colorPrimary: "#092C4C" } }` override from ConfigProvider. No antd Buttons remain after P1-04, so it styled nothing. Worth stating plainly, because the P1-04 message did not: that override was painting every antd primary button the old Unstract navy. They now take --primary from Midnight Bloom, so primary buttons across the authenticated app move navy #092C4C -> violet #6f5cef. That is the intended end state under D8, but it is a site-wide colour change and the earlier "geometry preserved" note only covered the unauthenticated landing page, where the Login control is not an antd Button. Build, 39 tests and lint all green after the rename. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [P1-05] Move Space/Row/Col/Flex off antd via a layout shim Implements P1-05 of UN_SHADCN_IMPL_PLAN.md. 74 call-site files plus `@/components/ui/antd-layout`. Zero antd Space/Row/Col/Flex imports remain. The plan classified these as a direct swap to flex/grid utilities. They are not, for a concrete reason: antd's `Space` wraps every child in its own `.ant-space-item` div, and Row/Col emit `.ant-row`/`.ant-col`. This repo has 20 hand-written CSS rules that select those internals — e.g. `.ant-space .ant-space-item .ant-card` in onBoard.css and `.file-history-modal .action-buttons .ant-space`. Collapsing the wrappers into `gap-*` on the parent deletes the elements those selectors match, so the styling silently stops applying: a regression, not a restyle (C4). 22 Space call-sites also build children from `.map()` or conditionals, where per-child wrappers change what `> *` matches. So the shim keeps antd's DOM shape, including the `ant-*` class names the existing CSS targets, while dropping the antd dependency. Those class names are emitted deliberately and go away in P4 when the dependent CSS is cleaned up. Details preserved: antd's size tokens (small/middle/large -> 8/16/24px) and numeric/array sizes; Space's falsy-child filtering, so conditional children do not leave empty gaps; the 24-column Col basis with span/offset as percentages; and Row's negative-margin + Col-padding gutter model. 11 unit tests cover the shim, centred on the wrapper-div structure that the existing CSS depends on. Full suite is 50 tests across 7 files, green. Build passes and lint is back to the 24-warning baseline with none in the new file (the two I introduced were single-line if-returns, now braced). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [P1-06] Move remaining leaf components off antd Implements P1-06 of UN_SHADCN_IMPL_PLAN.md, completing phase P1. 51 call-site files plus `@/components/ui/antd-leaves` covering Tag, Spin, Alert, Image, Divider, Empty, Avatar and Progress. These are the "direct swap" tier of docs/shim-convention.md — none of them carry behaviour the shadcn primitives lack. They are still gathered behind one module so ~60 call-sites convert by import instead of hand-rewriting JSX, which keeps the diff mechanical (C4). Checked before deciding, per the convention: `Spin` has ZERO `spinning={...}` usages, so there is no overlay mode to reproduce and no wrapper is needed — every site is a bare indicator. Most already route through the existing SpinnerLoader widget, which now picks up the shim automatically. Mapping notes: - Tag colour tokens fold onto Badge variants (success/green -> success, error/red -> destructive, and so on). One call-site passes a raw `rgb(45, 183, 245)`, which antd would have applied directly, so unrecognised colours fall through to inline style rather than being dropped. - Alert keeps message/description/showIcon/closable/banner, with its own dismiss state so `closable` still works. - Image does not reimplement antd's `preview` lightbox: no call-site enables it. If one appears later it needs a real implementation, not a prop no-op. Build passes, 50 tests green, lint back to the 24-warning baseline with none in the new file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [P2] Move overlays and notifications off antd Implements P2-01..P2-06 of UN_SHADCN_IMPL_PLAN.md. 76 call-site files plus `@/components/ui/antd-overlays` (Modal, Tooltip, Dropdown, Popconfirm, Popover, Collapse) and `@/hooks/useConfirm`. P2-01 useConfirm: promise-returning confirm dialog over AlertDialog, so `if (await confirm({...}))` replaces antd's callback-style Modal.confirm. OSS owned per D9 because the 3 cloud Modal.confirm sites must import it rather than reimplement it. It resolves false on Escape and outside-click, so the promise can never dangle. P2-02..P2-05 overlays. Behaviours preserved that a prop swap would have lost: - Modal renders an OK/Cancel footer BY DEFAULT and only omits it for footer={null}. Call-sites relying on the implicit footer keep their buttons. - The legacy `visible` alias still works alongside `open` (2 sites use it). - destroyOnClose unmounts the body, which Radix does not do on its own. - confirmLoading disables OK, matching the Button shim's loading semantics. - closable={false} hides the close affordance; this shadcn DialogContent renders it unconditionally, so it is suppressed by class rather than prop. - Dropdown accepts antd's `menu={{ items }}` data shape and maps it onto Radix's composed children. - Popconfirm routes onto AlertDialog so inline confirms and useConfirm() share one behaviour rather than diverging. P2-06 notifications: sonner is now the only surface. The ALERT_SURFACE flag, antd's notification.useNotification(), the Close/Close All buttons and contextHolder are all removed. showAppToast now accepts a React node so the rendered markdown + Execution/Request ID lines carry over unchanged, and a `message` export mirrors antd's imperative message.* API for the 3 files that used it. Toaster is positioned top-right to match where antd's stack appeared (sonner defaults to bottom-right) — C4. Verified in the browser: 2 sonner toasts render, 0 antd notifications, and total antd elements on the landing page fall 24 -> 3. Build passes, 68 tests across 9 files green (18 new), lint back at the 24-warning baseline with none in the new files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [P3-01..P3-03] Move Form and data-entry controls off antd Implements P3-01 (pattern), P3-02 (bulk Form conversion) and P3-03 (input controls). 61 call-site files plus `@/components/ui/antd-form` and `@/components/ui/antd-inputs`. This is the phase the plan flagged as highest risk, and the reason is the imperative form API: the codebase drives antd Forms through a form instance — setFieldsValue on edit, `await form.validateFields().catch(() => null)` as the submit guard, resetFields on cancel — across 14 useForm() sites and 102 Form.Items. Hand-rewriting those onto raw react-hook-form would be 102 independent chances to change submit or validation behaviour, and one missed guard silently submits invalid data. So antd's Form surface is reimplemented on react-hook-form and call-sites convert by import alone. docs/form-pattern.md records the pattern, with GroupCreateEditModal as the worked reference (it exercises setFieldsValue, the validateFields guard, resetFields and a required rule). The load-bearing detail: validateFields REJECTS when invalid. Two tests pin it — one asserts the rejection reaches `.catch()`, one asserts onFinish does not fire while a required field is empty. antd rule objects (required/min/max/ pattern/custom validator) are translated to RHF options, and a thrown validator error becomes the inline message. P3-03 covers Input (+ TextArea 14 sites, Password, Search), Select, Checkbox, Switch, Radio and InputNumber. The awkward part is onChange shape: antd hands a DOM event to Input but a raw value to Select/Switch, and gives Checkbox an event with target.checked where Radix gives a boolean. Call-sites are written against antd's convention, so the shim rebuilds those shapes instead of rewriting ~90 handlers. Select accepts both `options` data and Select.Option children (6 files use the latter). Build passes, 78 tests across 10 files green (10 new for the Form shim), lint back at the 24-warning baseline. antd importers now 73 files, down from 163 at the start of P1. Note: the new tests use @testing-library/user-event v13's direct API, not v14's `.setup()` — this repo is on v13. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [P3-04..P4] Remove Ant Design entirely from the OSS frontend Completes P3-04, P3-05 and all of P4. antd, @ant-design/icons, @rjsf/antd and @react-awesome-query-builder/antd are gone from package.json, and `grep -rl "from 'antd'"` over src/ returns nothing. P3-05 (RJSF) turned out far smaller than D3 assumed. RjsfFormLayout already supplies its own `widgets` and `templates` for every field type, so @rjsf/antd was contributing only theme chrome — swapping the import to @rjsf/core is the whole change. There was no widget registry to rebuild. P3-04 (date/time) follows D7 deliberately: the pickers are rebuilt on native date/datetime-local/time inputs, but they still EXCHANGE MOMENT OBJECTS, because call-sites are written as `value={moment(v)}` and `onChange={(d) => onChange(d?.toISOString())}`. Dropping moment would change timezone/DST behaviour, which D7 says needs its own reviewed pass — so this change stays confined to the widget layer and moment remains a dependency. P4 adds the shared DataTable (D5/D9) over TanStack + shadcn table, presenting antd's Table API (columns/dataSource/rowKey/rowSelection/pagination/loading) so all 16 call-sites convert by import and both repos share one table implementation. antd-structure covers the remaining Card, Tabs, List, Layout, Upload, Result, Drawer, Menu, Segmented, Pagination, Steps, Tree and Skeleton. Final removals: - ConfigProvider dropped from App.jsx; next-themes already owns theming. - theme.useToken() replaced by the --card CSS variable. - The three deep imports (antd/es/tabs/TabPane x2, antd/es/input/Search) now resolve to Tabs.TabPane and Input.Search on the shims. - antd-vendor manual chunk, the antd optimizeDeps entries, and the Less preprocessor option (antd was the only Less consumer) removed from vite.config.js. - Query builder swapped to @react-awesome-query-builder/ui, promoted to a direct dependency because the cloud overlay has no manifest of its own (D4). Note on the DOM: three `ant-row`/`ant-col` elements still appear at runtime. Those are emitted deliberately by the P1-05 layout shim because 20 hand-written CSS rules select them; they are our class names, not antd. They go away when that CSS is cleaned up. P4 exit gate: 0 antd imports in src, 0 antd entries in package.json, build passes, 78 tests green, no runtime page errors. Lint shows 3 errors and 26 warnings, all pre-existing — the errors are two SVG assets byte-identical to main, and none of the findings are in files this migration added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [Phase C] Add shared components the cloud plugins need Additions to the OSS shim layer surfaced while converting the enterprise plugins. They live here rather than in the plugins per D9/§5.0 — a component needed by more than one call-site is OSS-owned, so the two repos cannot drift. antd-structure gains four components used only by cloud plugins today: - Descriptions (4 sites) — label/value grid - Statistic (2) — figure with prefix/suffix/precision - FloatButton (2) — fixed-position action button - Transfer (2) — dual list with move-between controls - Badge — antd's count/dot overlay. Note this is NOT shadcn's Badge, which is a pill label; antd calls that one Tag. Naming them apart avoids a confusing collision later. useAppToast gains a `notification` export mirroring antd's imperative notification API, including the useNotification() hook form that returns [api, contextHolder]. antd's config shape is `{ message, description }` while sonner takes a title plus `{ description }`, so the remap happens here instead of at each call-site. Verified both ways: the OSS build passes with src/plugins absent (the optionalPluginImports path), and the P0-G2 overlay build passes with all 53 plugins present and antd uninstalled. 78 tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [P4-09 + gaps] Complete the CSS cleanup, shim tests, icon map and scroll-lock check Closes the four items I previously reported as complete but had not actually finished. Each is now verified against the plan's own criteria rather than by assertion. P4-09 — final cleanup. Its verify command is `grep -rn "legacy-" src/` -> 0. It was 72. The 42 remaining var() references across 8 legacy variables are now mapped onto Midnight Bloom semantic tokens and variables.css is deleted: --legacy-page-bg-1/2/3 -> var(--card) / var(--background) / var(--muted) --legacy-white -> var(--card) (it flipped to #000 in dark, so it was a surface, not white) --legacy-black -> var(--foreground) (flipped to #fff in dark) --legacy-border-color-* -> var(--border) --legacy-font-family -> var(--font-sans) --legacy-font-size/weight-* -> literals; Tailwind stock matches them exactly Visible effect: the page background moves #e9e9e9 -> #fafafa, and body background now follows the theme, which the legacy vars only did for a few surfaces. Dark mode re-verified end to end after the file was removed. docs/icon-map.md was stale — it documented 43 icons from the first enumeration pass, but the real set is 116 (87 OSS, 87 cloud, overlapping). Regenerated from the verified map with true pre-migration usage counts pulled from git, and 27 inexact pairs called out with the reason each differs: lucide has NO filled variants (8 icons render lighter), it dropped brand icons (Slack is simply gone), and several are approximations (FilePdf -> FileText loses the format hint). This is the artifact a reviewer needs to sanity-check those calls. Four shims had no tests, which contradicts the rule in shim-convention.md that every shim must cover the behaviours justifying it. Added 67 tests: - antd-inputs (14) — the onChange CONVENTIONS, which differ per component and which Radix inverts: Input gets an event, InputNumber a number, Checkbox an event with target.checked, Switch a boolean. - antd-datetime (14) — the D7 contract, i.e. onChange hands back a MOMENT so `date?.toISOString()` at the call-sites keeps working. - antd-leaves (17) — including the raw rgb() Tag colour that must not be dropped just because it is not a known token. - antd-structure (22) — DataTable's antd column/render contract, and Badge's count/overflow/showZero rules. P2-02's deferred `body { overflow: hidden }` check is done. Radix's dialog scroll-lock also sets body overflow and restores the prior value on close; the risk was it restoring the wrong one and leaving the fixed app shell scrollable. Three tests pin it: overflow stays hidden before/during/after, survives repeated cycles, and is NOT left hidden on pages that never pinned it. The index.css comment now records the outcome instead of reading as a TODO. One real bug surfaced while writing these tests: the Tabs shim passed both `value` and `defaultValue` to Radix, and a present-but-undefined `value` makes Radix treat the component as controlled — which would have frozen every uncontrolled tab set. Now it passes exactly one. Test suite: 148 tests across 15 files, up from 78. Build passes, lint at the 24-warning baseline with zero findings in migration files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [fix] Commit the lockfile changes from the antd removal The dev-deploy frontend image build failed: error: lockfile had changes, but lockfile is frozen process "/bin/sh -c bun install --frozen-lockfile --ignore-scripts" did not complete successfully: exit code: 1 `bun remove antd @ant-design/icons @rjsf/antd @react-awesome-query-builder/antd` and the `@tanstack/react-table` / `@react-awesome-query-builder/ui` additions updated bun.lock in the working tree, but that file was never staged — every earlier commit staged explicit paths and bun.lock was not among them. So the committed lockfile still listed antd as a root dependency and was missing @tanstack/react-table, which is exactly the desync --frozen-lockfile exists to catch. Nothing about the migration changes; this is the manifest edits reaching git. Why local checks did not catch it: `bun install --frozen-lockfile` in the worktree passes, because it validates the WORKING lockfile, which was already correct. Only a clean checkout — i.e. Docker — sees the committed one. Verified the fix by copying package.json + bun.lock into an empty directory and running the container's exact command there: exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [fix] Layout and Sider shims dropped antd's sizing behaviour Two bugs found by comparing the dev deployment against production. Both are in the P4 structure shim, and both produce a page that is "correct" in the DOM but broken on screen — so no test, build or lint caught them. 1. Layout had no flex-grow. antd's Layout is `flex: auto`; mine computed `flex: 0 1 auto` and resolved to height 0. Every descendant using `flex: 1` then collapsed: on the dashboard, `.metrics-dashboard-container` was 0px tall while its child was 212px, so the whole page rendered at y=858, below a clipped viewport. The content was in the DOM the entire time, which is why it looked like a data problem rather than a CSS one. Layout.Content had the same issue (`flex-1` vs antd's `flex: auto`). 2. Layout.Sider ignored `collapsed` / `collapsedWidth`. It always applied `width`, so with a stored `collapsed: true` preference the rail sat at the full 240px while SideNavBar hid every label behind `!collapsed` — an icons-only sidebar in an expanded gutter. `collapsible` and `collapsedWidth` were also leaking onto the DOM as invalid attributes. Layout now also switches to a row when it contains a Sider, matching antd's hasSider auto-detection. That is done via an explicit `__isSider` marker rather than `c.type === Layout.Sider`: the identity check is fragile because Sider is assigned after Layout and does not survive HMR or wrapping. 7 regression tests cover both: flex-auto on Layout and Content, row/column switching, collapsed vs expanded width, and no antd-only props reaching the DOM. Full suite 155 tests, build and lint green. Worth noting for the remaining review: this is the class of defect the shim unit tests structurally cannot catch. They assert rendered output in jsdom, which has no layout engine — height 0 and height 212 look identical there. Only a real browser shows it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [fix] Detect Sider at runtime so Layout lays out as a row Follow-up to the previous Layout fix, which was only half right. `flex-auto` landed and the Sider collapse fix worked (the rail correctly renders at 65px now), but the dashboard was still empty: `.metrics-dashboard-container` remained 0px against production's 607px. The reason is the OTHER half of antd's Layout behaviour. A Layout containing a Sider lays out as a ROW; mine stayed a column, so the content area got no height. My first attempt inferred this from `React.Children`, which cannot work here: PageLayout renders `<SideNavBar>`, and the Sider lives *inside* that component. Compile-time child inspection can never see it. antd solves this with runtime context, so this does too — a Sider registers itself with the nearest ancestor Layout on mount, however deeply nested. The `__isSider` marker from the previous commit is gone; it was unreachable. Confirmed against production, whose outer Layout is `ant-layout ant-layout-has-sider` with `flex-direction: row` at 713px, versus mine at `flex-col` and 0px. The new test renders a Sider inside another component, matching how the real app does it — the earlier test passed a Sider as a direct child, which is exactly the case that already worked and why the bug survived. 156 tests, build and lint green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [fix] Modal `centered` broke the dialog's centring transform Third layout defect found by comparing the dev deployment against production. On the workflows page the "Create Prompt Studio" dialog rendered at y=-109 with `transform: none` — pinned to the top of the viewport with its header clipped off-screen. Cause: shadcn's DialogContent is ALREADY centred, via `top-[50%] translate-y-[-50%]`. My Modal shim treated antd's `centered` prop as something it had to implement and appended `top-1/2 -translate-y-1/2` — the same geometry spelled differently. tailwind-merge sees two competing translate/top utilities, keeps one, and the dialog ends up with no transform at all. antd's `centered` is therefore a no-op here: the base component already does it. The prop is still destructured so it cannot land on the DOM as an invalid attribute, with a comment explaining why it is deliberately unused — otherwise this looks like an oversight and gets "fixed" back. Two regression tests: the base translate utilities must survive alongside `centered`, and the conflicting spelling must be absent. Audited the other shims for the same pattern (a wrapper adding positioning utilities on top of a shadcn primitive's own). The remaining `absolute`/`fixed` classes in antd-leaves and antd-structure are on elements those shims create themselves, so there is nothing to conflict with. 158 tests, build and lint green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [fix] List ignored antd's grid prop, stacking adapter cards Fourth defect found against the live deployment. The Add LLM / Add Connector pickers render `<List grid={{ gutter: 16, column: 4 }}>`. antd switches to an n-column grid for that; my shim always rendered a divided vertical list, so every adapter appeared one-per-row in a 600px scroller instead of 4-up. Measured in the browser: `.list-of-srcs` children all sat at the same x with display:block. The shim now honours `grid.column` (grid + grid-cols-n) and `grid.gutter` (gap), and keeps the stacked divide-y list when no grid prop is passed. Column classes are written out in a lookup rather than interpolated, since Tailwind scans statically — same reasoning as the line-clamp table in antd-typography. Two tests: grid mode applies grid-cols-4 and the gutter and drops divide-y; non-grid mode still stacks. 160 tests, build and lint green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [fix] Tall modals overflowed the viewport with no scrollable body Fifth defect found against the live deployment. The Add LLM adapter settings form (RJSF) rendered 1109px tall in an 800px viewport. The dialog was pushed to y=-194 and the Submit button sat off-screen, so the form could be filled in but never saved. antd wraps modal content in `.ant-modal-body`, and this app's CSS caps that element — `.add-source-modal .ant-modal-body { height: 695px; overflow: hidden auto }`, `.retrieval-strategy-modal .ant-modal-body { max-height: 70vh }` and several more. My Modal shim rendered children directly into DialogContent, so none of those rules matched anything and nothing constrained the height. Content is now wrapped in a `.ant-modal-body` element. The class name is what makes the existing per-modal CSS work again; the `max-h-[70vh] overflow-y-auto` on it is the fallback for modals that never had a bespoke rule. Found while verifying P3-05: the RJSF form itself is correct on @rjsf/core — 9 inputs, 3 required markers, descriptions, prefilled defaults, password reveal, and Test Connection / Submit / Close all render. It was only unreachable. 161 tests, build and lint green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [fix] Emit the antd class names the app's own CSS still targets This is the systemic cause behind most of the layout defects found against the live deployment, rather than another one-off. The app has ~200 hand-written CSS rules that target antd's internal class names — `.ant-card-body`, `.ant-modal-content`, `.ant-tabs-nav`, `.ant-table-body`, `.ant-btn`, `.ant-typography` and ~65 more. antd emitted those elements; my shims did not, so every one of those rules silently matched nothing. 109 of them set layout properties (height, overflow, display, flex, padding), which is exactly why screens looked structurally right in the DOM and wrong on screen. Measured before and after: **109 dead layout rules across 53 classes → 8 across 8**. The 8 that remain are leaf styling on features this app does not currently render (card meta, textarea counters, tab overflow controls). The shims now emit the class names alongside their Tailwind classes. This is deliberate coupling to the legacy CSS, not an accident, and it is temporary: when that CSS is eventually rewritten against the design tokens, the hooks come out. The P1-05 layout shim already did this for `.ant-space-item`/`.ant-row`; this extends the same approach to the rest. Also fixed while here: Divider, Radio.Group/Radio, Segmented items, Popover inner, Result subtitle, Dropdown menu items and Collapse header/content were missing their hooks. Found by static audit rather than by opening screens — the previous five bugs were each discovered one page at a time, which does not scale and would have missed the ones on screens nobody happened to visit. 161 tests, build and lint green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [fix] Collapse.Panel and Card.Meta were undefined, crashing Prompt Studio Most severe defect so far: opening any Prompt Studio project showed "Couldn't load this page" and rendered nothing. Cause: PromptCardItems.jsx and NotesCard.jsx render `<Collapse.Panel>`, and SetOrg.jsx renders `<Card.Meta>`. Neither sub-component existed on the shims, so React received `undefined` as an element type and threw error #130. That does not degrade one component — it takes down the entire route. Collapse now supports both antd forms: the `items` data prop and the legacy `<Collapse><Collapse.Panel header=…>` children, including `showArrow={false}` which PromptCardItems relies on. Card.Meta renders avatar/title/description. Added a completeness guard (shim-completeness.test.jsx) instead of only fixing the two. It scans the app source for every `<Foo.Bar>` usage and asserts the shims actually expose it. The per-component tests could not have caught this: nothing in them rendered Collapse.Panel, so its absence was invisible until a real page tried. The guard covers 14 sub-components today and fails loudly for any future gap. It earned its place immediately — it caught that my first Collapse.Panel assignment had not landed (biome had reordered the export block my patch anchored to, so the edit silently no-opped). 176 tests across 16 files, build and lint at the 24-warning baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [fix] Modal.useModal was undefined, crashing 12 components on click Found by static audit rather than by clicking: scanning for `Foo.bar(...)` calls on shim components turned up two undefined statics. ConfirmModal calls `Modal.useModal()` and then `modal.confirm({...})`. Neither existed, so every consumer threw a TypeError the moment its button was clicked. That is 12 components — delete actions across prompt studio, workflows, manage docs, LLM profiles, custom synonyms and the top nav. useModal now returns `[api, contextHolder]` and implements confirm/info/ success/error/warning/destroyAll on AlertDialog, so it shares behaviour with useConfirm() instead of becoming a second confirm pattern. Escape and outside-click resolve as Cancel. Modal.confirm is implemented too — the fully-imperative form callable outside React, which mounts its own root. No OSS call-site uses it today, but the cloud plugins have three. Extended the completeness guard to cover static calls, not just `<Foo.Bar>` JSX. It now strips comments before scanning: a doc comment mentioning `Modal.confirm` is not a call-site, and flagging it would teach people to ignore the test. 180 tests across 16 files, build and lint at the 24-warning baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [fix] App CSS `top: 20px` fought the Dialog's centring Eleventh defect. The workflows "Create Prompt Studio" dialog rendered at y=-127 with its header clipped off-screen, even after the earlier centring fix. That earlier fix was correct — the classes were right this time. The override came from the app's own stylesheet: .prompt-studio-modal { padding: 10px; top: 20px; } antd's modal wrapper is statically positioned, so `top: 20px` read as "20px from the top of the viewport" and worked. The shadcn Dialog is `position: fixed` and centres itself with `top: 50%` + `translateY(-50%)`, so the same rule overrode the centring while the transform still applied — pulling the dialog 127px above the viewport. Removed the rule and left a comment explaining why, since it looks arbitrary otherwise. Centring is the component's job now. Added css-collisions.test.js rather than only fixing the one rule: it scans every stylesheet for a modal/dialog ROOT selector setting top/bottom/transform and fails with the offending file and rule. It deliberately ignores inner elements (`__body`, descendant selectors, `.ant-*`), which cannot fight the root's positioning. The remaining `.retrieval-strategy-modal__*` rules are inner elements and are correctly not flagged. This is the third distinct failure mode that jsdom cannot see (height 0, dead CSS hooks, and now positional overrides), so it is worth having a static guard rather than relying on someone opening the right screen. 182 tests across 17 files, build and lint at the 24-warning baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [fix] Buttons dropped refs, leaving every Dropdown trigger inert Twelfth defect, and the most silent one yet: Prompt Studio's Export button did nothing. No menu, no error, no network request — I instrumented fetch and XHR to confirm zero calls were made. Export is the child of a `<Dropdown>`, and Radix renders its trigger with `asChild`, attaching handlers through a ref. Neither CustomButton nor the base shadcn Button forwarded refs, so the ref went nowhere and the trigger was never wired up. A dropped ref throws nothing and logs nothing, which is why this survived 182 passing tests and a full route sweep — the page rendered fine, the button just wasn't connected to anything. Both now forward refs. That covers the 24 Dropdown call-sites, plus Popover and Tooltip triggers that use the same asChild mechanism. Audited the other primitives: Badge, Kbd, Label, Skeleton and Spinner are also plain functions, but none is used with asChild anywhere, so they are not causing breakage. Left alone rather than changed speculatively. Four regression tests: the base Button and CustomButton each forward to a real DOM node, a Dropdown wrapping CustomButton gets aria-haspopup/data-state (proving Radix wired the trigger), and the menu actually opens on click. 186 tests across 18 files, build and lint at the 24-warning baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * [fix] Add Dropdown.Button — the split-button shim was missing Found by the shim-completeness guard once the enterprise plugins were overlaid: ReviewHeader.jsx:910 renders <Dropdown.Button>Download File</...>, and Dropdown.Button was undefined. That is React error #130, which takes down the whole manual-review route rather than just the button — the same failure mode as the Collapse.Panel bug. Dropdown.Button is NOT Dropdown. In <Dropdown> the child IS the trigger, so naively aliasing the two would make "Download File" open a menu instead of downloading. antd's split button keeps the halves separate: children is a real action button wired to onClick, and only the chevron opens the menu. The three new tests pin exactly that separation, since it is the one thing an alias would silently get wrong. The chevron half carries aria-label="More actions" so both halves stay distinguishable by accessible name. * [fix] RangePicker silently ignored five props its call-sites pass The shim accepted `presets`, `disabledDate`, `allowClear`, `onOk` and `format` and did nothing with them. Nothing crashed, so this survived the migration invisibly — but three of the five are behaviour, not decoration: - `presets` MetricsDashboard's "Last 7/30/90 Days" buttons never rendered. Those are the primary way the range gets set, so the control looked finished while its main affordance was missing. - `disabledDate` MetricsDashboard uses it to block future dates. Ignored, users could query tomorrow. Now probed outward from today and mapped onto the inputs' min/max, which is the bound a native input can actually enforce. - `allowClear` antd defaults to true; MetricsDashboard passes false because its handler drops anything that is not a complete pair. Emitting null there strands it on a stale range. `onOk` now fires when a range becomes complete (there is no popup confirm button to hang it off). `format` and `size` are destructured to keep them off the DOM. Also stops forcing moment on the way out. ExecutionLogs holds moment, MetricsDashboard holds dayjs; the shim rebuilt every emitted date as moment, handing MetricsDashboard a type it never opted into. It happens not to break because that code only calls .toISOString(), which both implement — but it quietly reverses D7's promise that this layer does not change what flows through it. Emitted dates are now cloned from the caller's own instance. Each of the five behaviours has a test, and each was mutation-checked: the prop was re-broken one at a time and the matching test failed every time, so these assert the fix rather than restating it. * [fix] Rebuilding a caller's date via `new sample.constructor(iso)` is broken Live check on the dashboard caught this: the preset buttons rendered, but `disabledDate` produced no `max` bound, so future dates were still pickable — the very thing the previous commit claimed to fix. Cause: `new sample.constructor(isoish)` looks like a reasonable way to rebuild a date in the caller's library. It is wrong for both libraries in use. dayjs's internal constructor takes a config OBJECT, so handed a string it ignores it and returns TODAY. moment's returns an object that throws on .format(). So the disabledDate probe compared today against today on every iteration, never crossed the boundary, and yielded no bound. Now clones the caller's instance and re-points it field by field, which both libraries support (dayjs setters return a new instance, moment's mutate and return this; assigning the result covers both). The result is asserted to land on the exact requested instant before it is returned. The reason this got through: the test used a hand-written dayjs-shaped stub whose constructor DID accept a date string, so it validated the stub rather than the shim. Replaced with the real dayjs and moment, plus a case pinning the actual predicate MetricsDashboard passes. Re-broken deliberately to confirm the new test fails against the old approach. * [fix] Every dayjs-valued date field was rendering today's date Caught by driving the deployed dashboard: its range read 28 Jul → 28 Jul when the default is "last 30 days", and clicking a preset appeared to do nothing because the fields already showed today either way. `moment(dayjsInstance)` is the culprit. It does not throw and does not report invalid — it silently returns a moment for TODAY. `toInputValue` called it for anything that was not already a moment, so every dayjs value rendered as today with nothing to indicate it. MetricsDashboard holds dayjs, so its whole range was wrong on screen while its state was correct. Values exposing valueOf() (dayjs, moment, Date) are now normalised through the epoch instant before parsing. Strings are unaffected: String.valueOf() returns the string, so ISO parsing is unchanged. This one predates the previous two commits — the presets and disabledDate work was correct, but sat on top of a display path that had been broken for every dayjs caller since the shim was written. Verified across dayjs, moment, ISO string, Date, unparseable and null; the two new tests fail when the old moment(value) call is put back. * [P3-04] Give RangePicker a real calendar popup Two native date inputs were parity with nothing — antd's RangePicker has always been a two-month calendar with a preset sidebar, so the inputs were a downgrade users would notice. New component for us, not new capability. Adds a shadcn Calendar over react-day-picker v10 (which ships no stylesheet, so every colour is a Midnight Bloom token and it tracks light/dark), and rebuilds RangePicker as a single `ant-picker-range` trigger opening a popover: preset sidebar on the left, two months on the right. The external contract is unchanged and still mutation-tested: moment/dayjs tuples in and out, presets, allowClear, onOk, disabledDate. Two things the calendar does BETTER than the inputs it replaces: - `disabledDate` is per-date in antd, and a calendar greys out individual days. The native inputs could only approximate it by probing outward for a min/max bound. - the whole range is one control, so there is no half-updated state between two separate fields. Two library behaviours worth recording, both found by probing rather than assuming: - react-day-picker reports {from, to} with BOTH set to the clicked day on EVERY click; it does not distinguish opening a range from closing one. Taken at face value, onOk fires on click one and click two restarts instead of completing. An explicit anchor restores antd's semantics and orders the ends so a backwards selection still yields start <= end. - two months plus outside-day overflow means one date can appear twice in the DOM, so the test helper takes the non-outside cell. All six behaviours were re-verified by mutation. The first pass missed one: swapping likeSample for moment inside the disabledDate path went undetected, because the library-preservation tests only covered onChange. Added a test asserting the type the predicate itself receives. bun.lock is updated (not package-lock.json, which is gitignored here) — `bun install --frozen-lockfile` is what the Docker build runs, and an npm-only install would have failed it the way a missing bun.lock did before. * [test] Pin the RangePicker against MetricsDashboard's real prop shape The unit tests each drive one prop in isolation, which is how the earlier dayjs display bug slipped through: every individual assertion passed while the combination users actually see was broken. This renders the exact props MetricsDashboard passes — dayjs values, its disabledDate predicate, allowClear={false}, size, and all three presets — then drives the whole interaction: open the trigger, confirm two months and the preset sidebar, click a preset, and assert the emitted pair is dayjs and spans exactly 7 days. Cheap to run and it fails on any of the regressions this branch has already hit once. * [fix] Calendar popover stacked its months and ran off the screen Live check on the deployed dashboard: the popover opened 250px wide and ~700px tall, bottom edge at 1070px in a ~780px window — the two months were stacked in a column instead of sitting side by side, and the bottom of the calendar was unreachable. Cause: `sm:flex-row` on the months and `sm:flex-col` on the preset sidebar. Tailwind's `sm:` measures the VIEWPORT, but this content lives inside a popover whose own width is what decides the layout. On a wide screen the breakpoint matched and still produced a stacked column, because the popover never gets the viewport's width. Both are now unconditional rows. Worth noting how close this came to shipping: the screenshot was clipped at the viewport edge, so the popover looked plausible until its geometry was measured. jsdom has no layout engine and could never have caught it. The added guard asserts the class contract rather than the geometry — it fails if a `sm:` variant reappears in the popover — and was confirmed by reintroducing the bug. * [fix] Declare dayjs — it was reaching the app only through antd MetricsDashboard.jsx and RecentActivity.jsx both `import dayjs from "dayjs"`, but dayjs was never in package.json. `npm ls dayjs` showed the only path: frontend -> react-js-cron@5.2.0 -> antd@5.29.3 -> dayjs@1.11.19 That is a live hazard rather than a tidiness issue: antd itself is already undeclared (the migration removed it) and survives only because react-js-cron still depends on it. The moment react-js-cron is replaced — which the antd removal work requires anyway — antd goes, dayjs goes with it, and two production components stop resolving an import they have always relied on. Pinned to the exact version already resolving (1.11.19, not a caret range) so declaring it changes nothing at runtime today; it only removes the dependency on a transitive path we intend to delete. * [P4-08] Drop antd entirely — replace the cron picker, guard against its return antd was still in the tree. Every plan task about removing it passed — P4-05 (no imports), P4-08 (not in the manifest), the P4 exit gate — because they all check source imports and package.json. None of them look at the RESOLVED tree, and antd was reachable transitively: frontend -> react-js-cron@5.2.0 -> antd@5.29.3 58MB of framework serving one 38-line component with a single call-site. Replaces react-js-cron with a hand-rolled picker on the existing primitives rather than another library: a new dependency is a new transitive surface, which is the thing being removed. It offers hourly/daily/weekly/monthly plus a custom expression, and validates through cronstrue — the same parser the call-site already uses for its summary, so anything the dialog accepts the surrounding UI can render. Invalid expressions cannot leave the dialog. The contract is unchanged: `setCronValue` still receives a 5-field string. Checked before removing, since the OSS package.json is the only manifest (plan §3.3) and dropping a dep drops it for the cloud plugins too: - react-js-cron is antd's ONLY dependent (npm ls antd) - no cloud plugin imports react-js-cron - no app CSS targets its class surface Result: `npm ls antd` empty, no node_modules/antd, no @ant-design, and no antd runtime markers in any built chunk. Bundle 6.0MB -> 5.7MB, build 45s -> 27s. Adds a fourth static guard beside the existing three. A one-time grep proves today is clean and does nothing about the next dependency bump — and this defect survived the entire migration precisely because nothing failed. It checks all three ways antd can come back (declared, transitive, imported); each was verified by reintroducing it and watching the matching case fail. Also covers the nested-dialog case: CronGenerator renders as a sibling after EtlTaskDeploy's own Modal, so production stacks two Radix dialogs. The existing scroll-lock test covers one. These assert the inner opens over the outer, stays interactive, and leaves body overflow hidden on unmount (P0-12). * [fix] Four UI defects found comparing dev against the reference deployment Walked the app side by side with globe.unstract.com. Four real differences, batched into one deploy. 1. Every uncoloured border drew BLACK (~200 elements per settings page). Tailwind's border utilities set width only; the colour falls back to currentColor. shadcn's setup ships a `border-color: var(--border)` default and ours was missing it, so `border`, `border-b` and `divide-y` all drew solid black where antd had a near-invisible hairline — most obviously as a black rule between every list row. Fixed once in the theme rather than at call-sites, so the next uncoloured border is right by default. 2. A grey block behind the icon button on every prompt card (7 on one Prompt Studio screen). antd applies a Badge's `style` to the COUNT; the shim let it fall through `{...props}` onto the wrapper span, so PromptChangeIndicator's `style={{ backgroundColor: color }}` painted the wrapper. Also implements `offset`, which was accepted and ignored. 3. Invite Users was a blank "Couldn't load this page". antd's NamePath allows arrays — InviteEditUser uses `name={["email"]}` — but react-hook-form calls `.split(".")` on the name, so it threw `TypeError: s.split is not a function` and the error boundary swallowed the whole route. Arrays are antd's dotted path, so joining reproduces it. The cloud StripeProductForm uses nested `name={["tier1", "up_to"]}`, so this was broken there too. 4. Form labels pointed at nothing. `<Label htmlFor={name}>` was rendered but no id was ever set on the control, so clicking a label did not focus its field and screen readers announced it unlabelled. Found while writing the test for #3. Each fix has a regression test, and each was mutation-checked by reintroducing the defect and confirming the matching test fails. * [fix] Replace hardcoded antd blues with design tokens Comparing dev against the reference surfaced link buttons still painted antd's #1677ff — the Create Prompt Studio dialog renders its actions in antd blue rather than Midnight Bloom purple. antd is gone, so nothing supplies those colours any more; they were simply hardcoded, and 42 `!important` declarations in PromptStudioModal.css meant they beat the token styling wherever they appeared. 26 declarations across 8 files, all now `var(--primary)`. Also converts the black-alpha text in that file — `rgba(0,0,0,.45)` and `rgba(0,0,0,.88)` — to `--muted-foreground` / `--foreground`. Those are invisible-on-invisible in dark mode, which the old antd theme never had to handle. Colour-only; no structural change, and the full suite is unaffected. * [fix] Loader centring and an unscrollable sidebar Two reported issues, both traced to the same kind of cause: markup or sizing the shim quietly changed. LOADER — the logo was neither horizontally centred nor aligned with its text. `.center` (index.css) sizes itself with `width/height: inherit`, which does not resolve to the viewport; it collapsed to the height of its own content (measured: 24px), so the box landed wherever it fell rather than mid-screen. LazyOutlet.css already carries a patch for `.center` for exactly this reason. Scoped the fix to `.generic-loader` so the other consumer — FileSystem's inline empty state — keeps sizing to its container, and made `.spinner-box` a centred flex column so the logo, pulse dots and text share one axis. Measured after: logo centre-x 768 in a 1536 viewport, and equal to the text's centre-x. SIDEBAR — bottom menu items were cut off and unreachable. Two causes: 1. The Sider shim rendered children bare. antd wraps them in `.ant-layout-sider-children`, and SideNavBar.css depends on that wrapper: it is the flex column which clamps `.sidebar-content-wrapper` so its `overflow-y: auto` has something to scroll against. Without it the wrapper grew to its full 929px content height inside a 668px rail, so `auto` never engaged. 2. `.side-bar.ant-layout-sider-collapsed .sidebar-content-wrapper` set `overflow-y: hidden` under a comment saying "hide scrollbar". The scrollbar was already hidden by `scrollbar-width: none`; that rule was disabling SCROLLING, so the collapsed rail — which is how the sidebar actually renders — could not reach its lower entries on any window shorter than ~1000px. Verified live: both states now scroll, scrollbar still hidden. The reference deployment does not overflow only because its window is taller (766px vs 721px); it has the same content height, so this was latent there too. The Sider wrapper has a regression test, mutation-checked by removing the wrapper and confirming it fails. * fix(frontend): unify primary colour on Midnight Bloom tokens The migration left two competing definitions of "primary". The Button shim maps antd `type="primary"` to the shadcn `default` variant (`--primary`, violet), but CustomButton.css then repainted it navy `#092c4c` — so the 24 CustomButton call-sites rendered a different colour from every plain `<Button type="primary">`. Rather than retokenise that override, it is deleted along with the stylesheet: the shim already produces the right colour, and a second definition would only drift again. Sidebar: `.side-bar` hardcoded `#0d3a63` while `.topNav` (PageLayout.css) uses `var(--primary)`, so the two bars meeting at the top-left corner were different colours. Now both are `var(--primary)`. Deliberately NOT `var(--sidebar)` — that token is white in light mode, and every rule in SideNavBar.css assumes a dark surface. The navy-derived values that no longer work on violet are rebased with it: - `.space-styles` hover/active `#005b82` → a white overlay, which tints whatever `--primary` is in either mode. - `.sidebar-item-text` / `.sidebar-antd-icon` `#b4c2cf` measured 6.41:1 on navy but only 2.6:1 on violet — under even the 3:1 large-text floor. White is 4.72:1 and clears AA; the intermediate tints (#e8e5fb, #dcd8f9) top out at 3.82:1 and do not. Also implements antd's `showCount`, which the Input/TextArea shim dropped into `...props`: three modals asked for a counter and rendered none while `maxLength` still silently truncated typing. Remaining navy in CardGridView/ListView/SetOrg is text, not a surface, so it maps to `--foreground` (or `--primary` for the icon hover accent). Tests 225 → 229; lint holds at the 24-warning baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(frontend): retire antd's default palette for Midnight Bloom tokens Follow-up to the primary-colour unification. That change only swept the navy family, so antd's stock palette survived — including #1890ff on the sidebar that had just become violet. Sidebar icons were the visible regression. They are inline SVG assets with `fill="#90A4B7"` baked in, a slate grey chosen for navy where it measured 4.54:1. On `--primary` violet it falls to 1.84:1 and the icons all but disappear. The assets are data-URI <img> sources, so `color` cannot reach them; instead the `brightness(0) invert(1)` filter that the hover rule already used is applied at rest and dimmed to opacity 0.85 to match `.sidebar-item-text`, with the active state going to full white so the inactive/active distinction the hover rule drew is preserved. The rest is a role-based conversion, not a find-and-replace: - #1890ff / #1677ff as an accent, link or active state -> `--primary` - the same in `outline:` (focus rings) -> `--ring` - #40a9ff (antd's hover blue) -> `--violet-300` - #52c41a -> `--success`, #faad14 -> `--warning` - `.sidebar-toggle-icon.pinned` -> white; it only needs to read as "on" against the unpinned 60% white, not introduce a third brand colour Chart and per-type icon palettes (MetricsChart, RecentActivity, the Agency progress stroke) are deliberately multi-colour scales and keep their literals — tokenising them would collapse distinct series into one colour. Tests hold at 229; lint at the 24-warning baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(frontend): restore input border colour, pointer cursors, dragger and modal width Five defects, several sharing one root cause. 1. Input borders looked uncoloured on the left and right. The default-border-colour rule added earlier sat OUTSIDE any cascade layer. Unlayered CSS beats layered CSS regardless of specificity, so it outranked Tailwind's `utilities` layer and repainted every explicitly coloured border. `border-input` was the casualty: form controls resolved to `--border` (#e5e5e5) instead of `--input` (#d3d3d3), leaving inputs a full shade lighter than the antd reference — 1.21 vs 1.41 contrast against the page. The thin left/right edges wash out first at fractional device-pixel widths, which is why only they looked absent. Moving the rule into `@layer base` keeps it as the fallback for uncoloured borders while letting `border-*` utilities win, which is what shadcn intends. The 0.8px border width is NOT a bug: at dpr 1.25 Chrome snaps 1px to one device pixel and reports it back as 0.8px. The antd reference computes 0.8px too. 2. Buttons showed no hand cursor. Tailwind v4 dropped v3's preflight `cursor: pointer` on `<button>`, and antd had set it on every control. Added to the Button variants and to the other primitives that read as clickable: Select trigger and items, dropdown items, checkbox, radio, tab triggers. `disabled:pointer-events-none` still wins for disabled. 3. The emoji picker could not be dismissed with Esc or an outside click. antd call-sites pass `open` and drive it from the trigger themselves; Radix reads a bare `open` as fully controlled, so it had nowhere to report a close. The Popover shim now always supplies `onOpenChange`, and consumes antd-only props (`trigger`, `arrow`, `overlayClassName`) that were landing on the DOM. 4. The same picker was cut off: shadcn's PopoverContent is a fixed `w-72` (288px) with `p-4`, and the picker is wider and brings its own chrome. Now `w-auto` with a viewport-aware max and collision padding. 5. Import Project: the modal rendered at 570px instead of the requested 600 and drifted off-centre, because `width` was applied as `maxWidth` only and shadcn's `w-full max-w-lg` stayed in charge. And `Upload.Dragger` was aliased to `Upload` — an inline span, so the drop zone had no border, no fill, and no drag handlers at all. Dropping a file navigated the browser away to render it. Both fixed; Upload now handles drops. Tests 229 -> 232, mutation-verified; lint at the 24-warning baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(frontend): guard the cascade-layer and cursor regressions Both defects shipped, and neither is visible to jsdom — there is no cascade-layer resolution and no cursor — so they are asserted the way `no-antd.test.js` asserts its invariant: against the source text and the emitted class list. The border one had been reported twice. Unlayered CSS outranks every layered rule regardless of specificity, so the universal border-color selector silently beat Tailwind's `utilities` layer and repainted `border-input`; both mutations (unwrapping @layer base, dropping cursor-pointer) fail these tests. Co-Authored-By: Claude Opus 5 (1M context) <nore…
This was referenced Sep 2, 2026
muhammad-ali-e
added a commit
that referenced
this pull request
Sep 16, 2026
…ranches, stale comments Review findings on #2284, highest first. #1 (High) — half the rolling-deploy shim had no gating test that runs in the default lane. The create_workflow_execution response write had none at all, and the PgBarrier descriptor was pinned only by a test behind the barrier_db fixture, which skips wherever Postgres is absent. Descriptor construction moves to build_callback_descriptor(), pinned by a DB-free suite (workers/tests/test_legacy_transport_shim.py); the response write is pinned by a new backend case. All four writes now fail a required build if dropped early. #2 (High) — _process_file_batch_core's Args entries still described barrier_context=None as the supported Celery chord path while the body treats it as a malformed payload. Both entries rewritten; the log line now carries execution/workflow ids and the batch size. #3 — process_file_batch_django_compat delegates with two arguments, so after this PR every call landed in that malformed-payload branch: a false ERROR, and a full batch run with no claim (no pg_batch_dedup marker, so a redelivery re-runs it — LLM spend twice and a duplicate destination write with use_file_history=False). Its only producer was the Celery chord this PR removes, so it now refuses, the same fail-fast choice step_execution makes. Its MRQ helpers are left in place per the no-removal rule. #5 — PG_TRANSPORT_CALLBACK_KWARG became unconditional but its consumers still branched on it, leaving an else branch that swallows a failed finalization and strands the execution. The marker is now popped for wire compatibility only; the duplicate guard and the re-raise are unconditional. is_pg on _update_execution_status_unified becomes raise_on_failure (default True); the one caller inside an except block opts out so a raise cannot mask the original error. #6 — the removal checklist now lists all five artefact groups, and EXECUTION_EXCLUDED_PARAMS names the key through LEGACY_TRANSPORT_KEY instead of a hardcoded literal grep would miss. #7 — CallbackDescriptor.transport is Literal["pg_queue"] and required, not NotRequired[str]: it is mandatory this release, "celery" must never be written, and the follow-up removal becomes a type error at the write site. #8 — the Barrier protocol's "two call sites still program against it" was untrue; both construct PgBarrier concretely. Corrected, with what actually keeps it. #10 — executor_dispatch_fakes reimplemented the queue-naming rule and accepted any enqueue signature. queue_for now delegates to a new production helper queue_for_executor() (also used by the three dispatch sites), and the fake mirrors the QueueTransport protocol's keyword-only key set. #11, #14 — stale "PG only" / "No-op on Celery" comments at the sites this PR made unconditional, a citation of the deleted Celery max_retries=0 wrapper, and a cross-reference to a chart file that lives in the cloud repo. Five tests that asserted the removed Celery branches are restated against the collapsed behaviour rather than deleted. Verification: workers 1481 passed / 1 skipped; backend shim, dispatch-orchestrator and async-wait tests pass; pre-commit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
muhammad-ali-e
added a commit
that referenced
this pull request
Sep 18, 2026
…, backend and SDK (#2284) * UN-4078 [MISC] Delete the SDK Celery ExecutionDispatcher and retarget its tests First slice of the Celery transport removal. `ExecutionDispatcher` published `execute_extraction` to RabbitMQ and blocked on an `AsyncResult`; nothing has selected it since UN-4046 made `get_executor_dispatcher()` return the PG request-reply dispatcher unconditionally. It had zero non-test importers. Deleted: `unstract/sdk1/.../execution/dispatcher.py` (316 lines), its export from the package `__init__`, and the `TestExecutionDispatcher` suite. The 11 workers test files that imported it are retargeted rather than dropped — they encode live product behaviour (which queue each operation lands on, what the payload carries), and that behaviour survives on `PgExecutionDispatcher`: - Queue naming moves to `unstract.workflow_execution.executor_rpc.QUEUE_PREFIX`, the surviving definition. The `celery_executor_*` names are NOT dead Celery surface — `worker-pg-executor` subscribes to exactly those strings — so the assertions pin the literal wire names. - New `tests/executor_dispatch_fakes.py` holds the shared fake transport, an eager variant that runs the task in-process (replacing the `send_task` monkey-patching four round-trip tests did), and a correctly-shaped callback signature builder. - The header-forwarding suite is replaced, not ported: PG dispatch takes no `headers=` by design, so the new tests assert org routing rides the payload AND that passing `headers=` still raises — the exact mistake that broke three call sites during UN-4046. - The raw-`send_task` canary is kept and its message updated; it matters more now, since a raw send_task publishes to a broker nothing drains. Two docstrings that claimed executor calls "go through Celery" were corrected — they became false with this change, not merely dated. Verification: workers 1517 passed / 1 skipped. sdk1 unchanged at 20 pre-existing failures (`test_llm_compat`, `test_litellm_cohere_timeout`), reproduced on a stashed tree to confirm they predate this work. Pre-commit clean on all 17 files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * UN-4078 [MISC] Delete the Celery execution transport Removes the Celery fan-out substrate and the per-execution transport field. Nothing has selected either since UN-4046 made `select_backend()` return PG unconditionally, but both were still compiled, imported and shipped — and importable Celery surface has already cost two incidents (UN-3779's hardcoded dispatcher, and the `headers=` breakage at three call sites during UN-4046). Deleted outright: - `queue_backend/redis_barrier.py` (761 lines) — the DECR-counter substrate - `CeleryChordBarrier` + `BarrierBackend` + `get_barrier()` + the `WORKER_BARRIER_BACKEND` env selector - `WorkflowTransport`, `DEFAULT_WORKFLOW_TRANSPORT`, `normalize_transport`, `is_pg_transport` (88 lines of `unstract/core/data_models.py`) - `barrier_pg_decr_and_check` — a thin @worker_task wrapper reachable only as a Celery `.link`; the decrement runs in-body via `_barrier_pg_decrement` Collapsed: the 10 `is_pg_transport` branch sites, the `transport` parameter threaded through the general/api/scheduler orchestrators, the `transport` field on `WorkflowContextData` and `FileProcessingContext`, the `is_pg` flag on the file-batch path, and the `transport` key the backend wrote into the dispatch payload. `_barrier_for_transport` becomes `PgBarrier()`. Two guards become unconditional rather than PG-gated: the terminal-execution skip and the duplicate-destination-write check. Both were only ever no-ops on Celery. This also starts retiring a documented live hazard. `DEFAULT_WORKFLOW_TRANSPORT` was `"celery"`, so a payload that lost the field during a rolling deploy selected a Celery fan-out with no consumers. Worth correcting the in-code note while deleting it: it claimed `CeleryChordBarrier` was selected and no PG rows were written, so the reaper could not see the strand. Neither held — every PG worker sets `WORKER_BARRIER_BACKEND=pg`, so `PgBarrier` was selected, and its `_reset_barrier` UPSERT is unconditional, so the barrier row was written and the reaper did see it. Real behaviour was a ~2.5h delayed ERROR. The reads are removed here, but the writes are NOT: a pre-UN-4078 worker still defaults an absent field to Celery, so during a rolling deploy it would publish to a RabbitMQ with no consumers. The next commit keeps writing the field for one release; the hazard is gone only once that shim is removed. `EXECUTION_EXCLUDED_PARAMS` deliberately keeps its `"transport"` entry: a rolling deploy can still have an un-upgraded producer emitting the field, and without the exclusion it reaches the legacy `execute_workflow` signature and raises. Tests: four whole suites deleted with their substrate (redis barrier, barrier backend selection, barrier differential, workflow-context transport). The rest are retargeted rather than dropped — the chord canary now asserts *zero* chord call sites (with a non-vacuity lock, since a zero-expectation assertion is exactly what a path bug passes silently), and the Celery-gated guard tests are replaced by the unconditional behaviour they now have. Verification: workers 1438 passed / 1 skipped / 0 failed. Pre-commit clean. The backend suite cannot start in this environment (`AppRegistryNotReady`, reproduced on a stashed tree — pre-existing), so the four backend edits are CI-verified only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * UN-4078 [MISC] Keep writing the transport field for one release as a rolling-deploy shim The previous commit removed every read of the `transport` payload field and every write. Removing the writes in the same release is unsafe: a pre-UN-4078 worker still treats an absent field as "celery" (DEFAULT_WORKFLOW_TRANSPORT) and, during a Kubernetes RollingUpdate, can receive a payload from an upgraded producer. It then publishes to RabbitMQ, which has no Celery consumers, so: - general workflows are marked ERROR by the reaper after ~2.5h, even with every file processed; - API deployments take no orchestration claim, so nothing sweeps them and they stay EXECUTING. The window is real: old file-processing pods keep draining claimed batches for their full grace period (up to 9120s). The four writes are restored as `transport: "pg_queue"`, all via one constant, LEGACY_TRANSPORT_KEY / LEGACY_TRANSPORT_VALUE in unstract.core.data_models, so the follow-up removal is a single grep: - backend WorkflowHelper orchestrator dispatch payload - backend create_workflow_execution response - workers scheduler async_execute_bin kwargs - workers PgBarrier callback descriptor (CallbackDescriptor gains `transport: NotRequired[str]`) Nothing on the new side reads the field. Tests pin each write so the shim cannot be dropped early; the scheduler cases also assert a stale "celery" in the backend response is never forwarded. Remove in the release after UN-4078: the writes, the constants, the `transport` entry in EXECUTION_EXCLUDED_PARAMS, and test_legacy_transport_shim.py. Verification: workers 1478 passed / 1 skipped / 0 failed; backend dispatch tests 5 passed (with backend/sample.env loaded); pre-commit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * UN-4078 [MISC] Address standardized review: shim gating tests, dead branches, stale comments Review findings on #2284, highest first. #1 (High) — half the rolling-deploy shim had no gating test that runs in the default lane. The create_workflow_execution response write had none at all, and the PgBarrier descriptor was pinned only by a test behind the barrier_db fixture, which skips wherever Postgres is absent. Descriptor construction moves to build_callback_descriptor(), pinned by a DB-free suite (workers/tests/test_legacy_transport_shim.py); the response write is pinned by a new backend case. All four writes now fail a required build if dropped early. #2 (High) — _process_file_batch_core's Args entries still described barrier_context=None as the supported Celery chord path while the body treats it as a malformed payload. Both entries rewritten; the log line now carries execution/workflow ids and the batch size. #3 — process_file_batch_django_compat delegates with two arguments, so after this PR every call landed in that malformed-payload branch: a false ERROR, and a full batch run with no claim (no pg_batch_dedup marker, so a redelivery re-runs it — LLM spend twice and a duplicate destination write with use_file_history=False). Its only producer was the Celery chord this PR removes, so it now refuses, the same fail-fast choice step_execution makes. Its MRQ helpers are left in place per the no-removal rule. #5 — PG_TRANSPORT_CALLBACK_KWARG became unconditional but its consumers still branched on it, leaving an else branch that swallows a failed finalization and strands the execution. The marker is now popped for wire compatibility only; the duplicate guard and the re-raise are unconditional. is_pg on _update_execution_status_unified becomes raise_on_failure (default True); the one caller inside an except block opts out so a raise cannot mask the original error. #6 — the removal checklist now lists all five artefact groups, and EXECUTION_EXCLUDED_PARAMS names the key through LEGACY_TRANSPORT_KEY instead of a hardcoded literal grep would miss. #7 — CallbackDescriptor.transport is Literal["pg_queue"] and required, not NotRequired[str]: it is mandatory this release, "celery" must never be written, and the follow-up removal becomes a type error at the write site. #8 — the Barrier protocol's "two call sites still program against it" was untrue; both construct PgBarrier concretely. Corrected, with what actually keeps it. #10 — executor_dispatch_fakes reimplemented the queue-naming rule and accepted any enqueue signature. queue_for now delegates to a new production helper queue_for_executor() (also used by the three dispatch sites), and the fake mirrors the QueueTransport protocol's keyword-only key set. #11, #14 — stale "PG only" / "No-op on Celery" comments at the sites this PR made unconditional, a citation of the deleted Celery max_retries=0 wrapper, and a cross-reference to a chart file that lives in the cloud repo. Five tests that asserted the removed Celery branches are restated against the collapsed behaviour rather than deleted. Verification: workers 1481 passed / 1 skipped; backend shim, dispatch-orchestrator and async-wait tests pass; pre-commit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
...
Why
...
How
...
Relevant Docs
Related Issues or PRs
Dependencies Versions / Env Variables
Notes on Testing
...
Screenshots
...
Checklist
I have read and understood the Contribution Guidelines.