Feat/summary generalized

[FloChehab]

Generalize summary service a bit to handle dictaphone more easily and behave more like the future AI gateway / STT Service.

• Created v2 tasks : separate transcribe & summarize (that are on specific queues but reuse the same workers for our production need at la stuite by default). In v2, webhook calls are also made from their own task / queue, to avoid an AI retry if webhook fails. For this a new celerySummaryBackend deployement is introduced (for the general task of so called "summary" service.
• Created pydantic models for API calls and webhook payloads (reused in dictaphone)
• Made the summary service multi-tenant (with backward compatibility) : each "tenant" has an id, an api_key, a webhook url and a webhook api_key.
• Added v2 routes for creating v2 tasks
• V2 transcribe task supports loading data from a URL

[FloChehab]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Walkthrough

This pull request introduces multi-tenant support and API versioning (v1/v2) to the summary/transcription backend service. Changes span infrastructure deployment, API layer restructuring, and core application logic. A new Celery worker deployment (celery-summary-backend) is added to process webhook queues. The authentication system transitions from single bearer token validation to multi-tenant API key validation. Configuration is refactored to support multiple authorized tenants with per-tenant webhook endpoints and credentials. File service gains cloud storage URL support alongside MinIO integration. A webhook-based v2 workflow is introduced for asynchronous task result delivery, including new Pydantic models for v2 requests/responses and discriminated union types for webhook payloads.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Feat/summary generalized' clearly summarizes the primary change: generalizing the summary service to support multi-tenancy and new v2 task/API capabilities.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check	✅ Passed	The PR description clearly outlines the changes: multi-tenancy support, v2 tasks with separate transcribe/summarize queues, webhook handling improvements, new Pydantic models, and URL-based data loading for transcription.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 12

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/summary/summary/core/webhook_service.py (1)

48-105: 🧹 Nitpick | 🔵 Trivial

Consider extracting duplicate response handling logic.

Both call_webhook_v1 and call_webhook_v2 have nearly identical response parsing and logging code (lines 61-73 vs 93-105). Consider extracting this into a helper function to reduce duplication.

♻️ Example refactor

def _log_webhook_response(response):
    """Log webhook response details."""
    try:
        response_data = response.json()
        document_id = response_data.get("id", "N/A")
    except (json.JSONDecodeError, AttributeError):
        document_id = "Unable to parse response"
        response_data = response.text

    logger.info(
        "Delivery success | Document %s submitted (HTTP %s)",
        document_id,
        response.status_code,
    )
    logger.debug("Full response: %s", response_data)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/summary/summary/core/webhook_service.py` around lines 48 - 105, Extract
the duplicated response parsing and logging in call_webhook_v1 and
call_webhook_v2 into a single helper (e.g., _log_webhook_response) that accepts
the HTTP response, performs the try/except JSON parsing to get document_id and
response_data, and runs the logger.info and logger.debug calls; then replace the
repeated blocks in both call_webhook_v1 and call_webhook_v2 with a call to
_log_webhook_response(response) to remove duplication while keeping behavior
identical.

src/helm/env.d/dev-keycloak/values.meet.yaml.gotmpl (1)

161-183: 🧹 Nitpick | 🔵 Trivial

Consider extracting shared env vars to reduce duplication.

The envVars block is duplicated nearly identically across summary, celeryTranscribe, celerySummarize, and celerySummaryBackend. While this is common in Helm values files, it increases the risk of configuration drift when updating shared settings like AUTHORIZED_TENANTS or Redis URLs.

Consider using YAML anchors or a shared values structure if your Helm chart supports it, though this is optional for dev environments.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/helm/env.d/dev-keycloak/values.meet.yaml.gotmpl` around lines 161 - 183,
The envVars block under the summary release is duplicated across summary,
celeryTranscribe, celerySummarize and celerySummaryBackend which risks
configuration drift; refactor by extracting the common envVars into a shared
value (e.g., a top-level key like sharedEnvVars) or use YAML anchors and
reference it from each release, then remove the duplicated envVars entries in
the summary, celeryTranscribe, celerySummarize, and celerySummaryBackend
sections so they inherit the shared settings (preserve any per-release overrides
such as replicas or specific keys).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/helm/meet/templates/celery_summary_backend_deployment.yaml`:
- Around line 57-72: Remove the unused HTTP port declaration from the Celery
worker container: delete the ports block that references
.Values.celerySummaryBackend.service.targetPort in the celerySummaryBackend
container spec so the pod no longer declares an unused "http" containerPort;
keep the probe blocks (liveness/readiness/startup) as-is so they can still be
enabled via .Values.celerySummaryBackend.probes, and ensure no other references
to that containerPort (in probes or elsewhere) depend on it before removing.

In `@src/helm/meet/values.yaml`:
- Around line 936-1044: The doc comments for the Celery summary backend contain
a consistent typo: replace every occurrence of "celerySummyBackend" in the
`@param` and `@extra` annotations with "celerySummaryBackend" so the annotations
match the actual YAML key celerySummaryBackend and the comment header; update
annotations referenced near the existing header and keys (e.g., occurrences
around the section header and annotations for image, dpAnnotations, command,
args, replicas, envVars, podAnnotations, service, probes, resources,
persistence, etc.) to fix the spelling across the block.

In `@src/summary/summary/api/route/tasks_v2.py`:
- Around line 46-59: The tenant validation in get_task_status using
AsyncResult(task_id) assumes task.args exists and contains a dict with
"tenant_id", which can raise when args is None or malformed; update
get_task_status to defensively handle missing or non-list task.args and missing
"tenant_id" by checking task.args is a list with at least one element, then
using a safe lookup (e.g., get on the first arg or try/except KeyError) or
wrapping the tenant_id extraction in a try/except and return HTTPException(404)
for any failure; reference AsyncResult, get_task_status, verify_tenant_api_key
and settings.get_authorized_tenant to locate and fix the validation logic so
malformed/missing args never raise unhandled exceptions.
- Around line 39-41: The summarize_v2_task is being enqueued without specifying
the v2 queue; update the call to summarize_v2_task.apply_async so it explicitly
targets the v2 summarize queue (e.g., pass the appropriate queue/routing_key
argument such as queue="summarize-v2") while keeping the existing args payload
({**request.model_dump(), "tenant_id": request_tenant.id}) unchanged so tasks
from this endpoint are routed to the v2 worker.

In `@src/summary/summary/core/celery_worker.py`:
- Around line 443-478: The current v2 worker only enqueues
TranscribeWebhookSuccessPayload on success, so when transcribe_audio or
format_transcript raises a terminal error we must catch it and enqueue a
TranscribeWebhookFailurePayload via call_webhook_v2_task.apply_async with the
tenant_id so tenants receive a final failure webhook; wrap the
transcribe_audio/format_transcript block (references: TranscribeTaskV2Payload,
transcribe_audio, format_transcript, TranscribeWebhookSuccessPayload,
call_webhook_v2_task.apply_async) in a try/except, build and queue a
TranscribeWebhookFailurePayload containing error details and source_id/tenant_id
on exception, and mirror the same pattern for the summarize flow (lines
~501-514) using SummarizeWebhookFailurePayload so both v2 paths always emit a
terminal success or failure webhook.
- Around line 143-149: The logger.exception call currently logs
cloud_storage_url verbatim (logger.exception(..., filename, cloud_storage_url))
which may expose presigned credentials; update the code in celery_worker.py to
redact the URL before logging by creating a redacted_cloud_storage_url (e.g.,
strip the query string or replace sensitive query parameters/signature with
"<REDACTED>" using url parsing) and pass filename and redacted_cloud_storage_url
to logger.exception instead of the raw cloud_storage_url so logs never contain
the presigned token.
- Around line 409-420: The task decorator on call_webhook_v2_task is misleading
because max_retries=3 does nothing without Celery retry wiring; update the
decorator to include bind=True and autoretry_for=(Exception,) with
retry_kwargs={'max_retries': 3} (or mirror the pattern used in the other task
that uses autoretry_for) so Celery will requeue on failure, and ensure the body
still calls
call_webhook_v2(payload=webhook_payload_adapter.validate_python(payload),
tenant_id=tenant_id); alternatively, if Celery-level retries are not desired,
remove max_retries from the decorator to avoid the misleading docstring.

In `@src/summary/summary/core/config.py`:
- Around line 132-157: The legacy_default_tenant_config model validator
currently reads legacy env values via os.getenv(), which bypasses
pydantic-settings; change it to read those values from the validator's data dict
(e.g. api_key = data.get("app_api_key"), webhook_api_key =
data.get("webhook_api_key"), webhook_url = data.get("webhook_url")) and keep the
rest of the logic: log the deprecation, append an AuthorizedTenant (using
V1_DEFAULT_TENANT_ID and SecretStr for secrets) to the authorized_tenants list
and store it back as a tuple in data["authorized_tenants"] so the
backward-compat path works with SettingsConfigDict-loaded .env values.

In `@src/summary/summary/core/file_service.py`:
- Around line 102-151: Validate the URL scheme at the start of
_download_from_cloud_storage_url by parsing cloud_storage_url (urlparse) and
rejecting any scheme not in an allowlist (e.g., "https" and optionally "http")
with a ValueError/FileServiceException; then enforce a download size limit by
either checking response.headers.get("Content-Length") and aborting if it
exceeds a new configurable attribute (e.g., self._max_download_bytes) or by
tracking a running byte counter while iterating response.iter_content and
raising FileServiceException (and cleaning up the partial temp file) if the
counter exceeds self._max_download_bytes; ensure to log the reason and include
the cloud_storage_url and actual size when raising the exception.

In `@src/summary/summary/core/models.py`:
- Around line 41-50: The validator validate_language (decorated with
`@field_validator`("language")) contains an unnecessary "v is not None" check
because the language field is required; remove that None guard and simply check
if v not in settings.whisperx_allowed_languages, raising the same ValueError if
invalid, then return v—update only the body of validate_language to perform the
membership check and return the value.

In `@src/summary/summary/core/security.py`:
- Around line 11-19: Tests are missing for the verify_tenant_api_key function to
assert behavior when the API key is invalid or missing; add unit/endpoint tests
that call the summary endpoints (or directly call verify_tenant_api_key) with
(1) an invalid Authorization Bearer token and (2) no Authorization header and
assert the response status and message are correct, referencing
verify_tenant_api_key and settings.authorized_tenant_api_keys to construct test
inputs; also confirm whether an unknown API key should return 401 vs 403 for
your multi-tenant model—if your design treats unknown API keys as authentication
failures, update tests to expect 401 (and update verify_tenant_api_key if
necessary), otherwise keep 403 and document the choice.

In `@src/summary/summary/core/shared_models.py`:
- Around line 12-17: The WordSegment and Segment Pydantic models (e.g.,
WordSegment.score, WordSegment.speaker, and Segment.words) currently use typed
unions like "float | None = Field(...)" which in Pydantic v2 makes the field
required; update these nullable fields to include default=None in their
Field(...) declarations so they become optional (e.g., Field(default=None, ...))
to avoid validation failures when WhisperX omits score, speaker, or words during
WhisperXResponse(**transcription_res.model_dump()).

---

Outside diff comments:
In `@src/helm/env.d/dev-keycloak/values.meet.yaml.gotmpl`:
- Around line 161-183: The envVars block under the summary release is duplicated
across summary, celeryTranscribe, celerySummarize and celerySummaryBackend which
risks configuration drift; refactor by extracting the common envVars into a
shared value (e.g., a top-level key like sharedEnvVars) or use YAML anchors and
reference it from each release, then remove the duplicated envVars entries in
the summary, celeryTranscribe, celerySummarize, and celerySummaryBackend
sections so they inherit the shared settings (preserve any per-release overrides
such as replicas or specific keys).

In `@src/summary/summary/core/webhook_service.py`:
- Around line 48-105: Extract the duplicated response parsing and logging in
call_webhook_v1 and call_webhook_v2 into a single helper (e.g.,
_log_webhook_response) that accepts the HTTP response, performs the try/except
JSON parsing to get document_id and response_data, and runs the logger.info and
logger.debug calls; then replace the repeated blocks in both call_webhook_v1 and
call_webhook_v2 with a call to _log_webhook_response(response) to remove
duplication while keeping behavior identical.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 96faa8d0-8a7b-4a82-8821-52385a2d2e1b

📥 Commits

Reviewing files that changed from the base of the PR and between 4d98ed4 and 22cbf1c.

📒 Files selected for processing (19)

• bin/Tiltfile
• src/helm/env.d/dev-keycloak/values.meet.yaml.gotmpl
• src/helm/meet/templates/_helpers.tpl
• src/helm/meet/templates/celery_summary_backend_deployment.yaml
• src/helm/meet/values.yaml
• src/summary/summary/api/main.py
• src/summary/summary/api/route/tasks.py
• src/summary/summary/api/route/tasks_v2.py
• src/summary/summary/core/celery_worker.py
• src/summary/summary/core/config.py
• src/summary/summary/core/file_service.py
• src/summary/summary/core/models.py
• src/summary/summary/core/security.py
• src/summary/summary/core/shared_models.py
• src/summary/summary/core/types.py
• src/summary/summary/core/webhook_service.py
• src/summary/summary/main.py
• src/summary/tests/api/test_api_health.py
• src/summary/tests/conftest.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
python - <<'PY'
import importlib.util
import os
import pathlib
import subprocess
import sys
import tempfile

if importlib.util.find_spec("pydantic_settings") is None:
    subprocess.check_call(
        [sys.executable, "-m", "pip", "install", "--quiet", "pydantic-settings>=2,<3"]
    )

from pydantic import model_validator
from pydantic_settings import BaseSettings, SettingsConfigDict

class Probe(BaseSettings):
    model_config = SettingsConfigDict(env_file=".env")
    sentinel: str = "ok"

    `@model_validator`(mode="before")
    `@classmethod`
    def show_inputs(cls, data):
        print("validator_data =", data)
        print("os.getenv('APP_API_TOKEN') =", os.getenv("APP_API_TOKEN"))
        return data

with tempfile.TemporaryDirectory() as tmp:
    pathlib.Path(tmp, ".env").write_text("APP_API_TOKEN=from_dotenv\n")
    os.chdir(tmp)
    Probe()
PY

Repository: suitenumerique/meet

Length of output: 976

---

Read legacy settings from the data parameter, not os.getenv().

Lines 137–139 use os.getenv() to read deprecated config, but this bypasses the env file loaded by SettingsConfigDict(env_file=".env"). Deployments with APP_API_TOKEN / WEBHOOK_* only in .env will fail: os.getenv() returns None, so authorized_tenants stays empty, and validation fails at lines 162–163. This breaks the backward-compat path this PR adds.

The data parameter passed to this validator already contains the env file values (loaded by pydantic-settings), so read directly from data instead:

api_key = data.get("app_api_key")
webhook_api_key = data.get("webhook_api_key")
webhook_url = data.get("webhook_url")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/summary/summary/core/config.py` around lines 132 - 157, The
legacy_default_tenant_config model validator currently reads legacy env values
via os.getenv(), which bypasses pydantic-settings; change it to read those
values from the validator's data dict (e.g. api_key = data.get("app_api_key"),
webhook_api_key = data.get("webhook_api_key"), webhook_url =
data.get("webhook_url")) and keep the rest of the logic: log the deprecation,
append an AuthorizedTenant (using V1_DEFAULT_TENANT_ID and SecretStr for
secrets) to the authorized_tenants list and store it back as a tuple in
data["authorized_tenants"] so the backward-compat path works with
SettingsConfigDict-loaded .env values.

[FloChehab]

I'd consider this as ok

[Leobouloc]

Nice work. I would need a second read as I went through the second half quickly.

My comments are essentially about naming and documentation.

[Leobouloc]

Generally speaking. Perhaps we could use one of "Transcribe" and "Transcript" ("Transcribe" would become "MakeTranscript"), to limit the terms when refering to the same task (same for summart)

[FloChehab]

Rushing a bit to merge today, we could make alias routes in the future (i agree with the less words to say the same things)

[Leobouloc]

return a 202 code ?

[FloChehab]

Yup, should have done that, will do in another PR

[Leobouloc]

return a 500 code ?

[FloChehab]

Hum, I think 200 is ok, but open to fix that in another PR

[lebaudantoine]

I haven’t fully reviewed the code, but I’ve verified that there are no regressions in V1. I’m in favor of merging this as soon as possible and then quickly migrating LaSuite Meet to avoid maintaining both versions for too long.

My comments are nitpicking comments that can be ignored

[lebaudantoine]

not sure whether it's relevant in this context

[FloChehab]

I agree, it makes life easier when develloping dictaphone.

[lebaudantoine]

I guess there’s quite a bit of duplicated code, especially around task status and key validation, which are core features that could eventually be extracted into a shared service (later).

[FloChehab]

Yes aggree

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE