TTS Evaluation: TTS Results signed URLs #666
Merged
AkhileshNegi merged 7 commits intomainfrom Mar 12, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds optional signed-URL support to TTS and STT evaluation endpoints via new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as TTS/STT API
participant DB as Database
participant Storage as Cloud Storage
Client->>API: GET /tts_evaluation/{run_id}?include_signed_url=true
API->>DB: Query results / dataset info
DB-->>API: Results & file identifiers
alt include_signed_url == true
loop per result/sample
API->>Storage: get_signed_url(object_store_url)
Storage-->>API: signed_url or None
end
else
API->>API: Skip signed URL generation
end
API->>Client: Respond with public models (include signed_url where present)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_stt_evaluation.py (1)
741-775: Assert the signed-URL lookup was actually attempted.Right now these tests pass even if the endpoint skips signed URL generation entirely, because the only checked outcome is
signed_url is None. Add call assertions so they verify the graceful-fallback path, not just the final state.Suggested test hardening
assert data["id"] == dataset.id assert len(data["samples"]) == 1 assert data["samples"][0]["id"] == sample.id assert data["samples"][0]["signed_url"] is None + mock_get_cloud_storage.assert_called_once() + mock_storage.get_signed_url_or_none.assert_called_once()assert data["id"] == run.id assert len(data["results"]) == 1 assert data["results"][0]["id"] == result.id assert data["results"][0]["sample"]["signed_url"] is None + mock_get_cloud_storage.assert_called_once() + mock_storage.get_signed_url_or_none.assert_called_once()Also applies to: 1074-1123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/api/routes/test_stt_evaluation.py` around lines 741 - 775, The test currently only checks that signed_url is None but doesn't verify that signed URL generation was attempted; update the test to assert that the mock storage's get_signed_url_or_none method was called for the sample (use mock_storage.get_signed_url_or_none.assert_called_once_with(...) or similar) referencing the sample's storage path/identifier (sample or sample.storage_path) and the include_signed_url flow triggered by the client.get to /api/v1/evaluations/stt/datasets/{dataset.id}; do the same assertion in the other affected block (lines ~1074-1123) to ensure the graceful-fallback path is exercised rather than skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/app/tests/api/routes/test_stt_evaluation.py`:
- Around line 741-775: The test currently only checks that signed_url is None
but doesn't verify that signed URL generation was attempted; update the test to
assert that the mock storage's get_signed_url_or_none method was called for the
sample (use mock_storage.get_signed_url_or_none.assert_called_once_with(...) or
similar) referencing the sample's storage path/identifier (sample or
sample.storage_path) and the include_signed_url flow triggered by the client.get
to /api/v1/evaluations/stt/datasets/{dataset.id}; do the same assertion in the
other affected block (lines ~1074-1123) to ensure the graceful-fallback path is
exercised rather than skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cd28afe-a277-4f68-82ea-a2189c47dad6
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_stt_evaluation.py
vprashrex
approved these changes
Mar 12, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/tests/api/routes/test_stt_evaluation.py (2)
743-774:⚠️ Potential issue | 🟡 MinorTest no longer validates failure handling - it tests the wrong contract.
The test
test_get_stt_dataset_signed_url_failureis named to test failure scenarios, but now mocksget_signed_urlto returnNone(line 744). The actualCloudStorage.get_signed_url()raisesCloudStorageErroron failure—it never returnsNone.This means:
- The test doesn't actually test the exception handling path
- If production code doesn't catch exceptions, this test would pass even though real failures would cause 500 errors
Once the production code is fixed to catch exceptions, update this test to simulate the actual failure behavior.
🧪 Proposed fix to test actual exception handling
def test_get_stt_dataset_signed_url_failure( ... ) -> None: """Test getting an STT dataset when signed URL generation fails.""" - # Mock cloud storage to raise an exception + # Mock cloud storage to raise CloudStorageError (actual failure behavior) mock_storage = MagicMock() - mock_storage.get_signed_url.return_value = None + mock_storage.get_signed_url.side_effect = Exception("AWS presign error") mock_get_cloud_storage.return_value = mock_storage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/api/routes/test_stt_evaluation.py` around lines 743 - 774, The test test_get_stt_dataset_signed_url_failure currently mocks get_signed_url to return None but CloudStorage.get_signed_url raises CloudStorageError on failure; update the test to simulate the real failure by configuring mock_get_cloud_storage() (the MagicMock returned) so its get_signed_url side_effect raises CloudStorageError, then assert the route /api/v1/evaluations/stt/datasets/{dataset.id} (the handler that calls CloudStorage.get_signed_url) still returns 200 and that the sample's signed_url is None (verifying the production code catches CloudStorageError).
1076-1123:⚠️ Potential issue | 🟡 MinorSame issue: test
test_get_stt_run_signed_url_failureno longer tests actual failure scenario.Line 1077 mocks
return_value = Noneinstead of simulating the actual exception thatget_signed_urlraises on failure. This test should be updated along with the production code fix.🧪 Proposed fix
- mock_storage.get_signed_url.return_value = None + mock_storage.get_signed_url.side_effect = Exception("AWS presign error")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/api/routes/test_stt_evaluation.py` around lines 1076 - 1123, Update the test to simulate a real signed URL failure by having the mocked storage client's get_signed_url raise an exception (e.g., mock_storage.get_signed_url.side_effect = Exception("signed url error")) instead of returning None, and ensure the production handler that builds run responses (the STT run retrieval logic used by the /api/v1/evaluations/stt/runs endpoint and any function that calls storage.get_signed_url) catches that exception and sets sample["signed_url"] = None so the test asserts remain valid; use mock_get_cloud_storage to return the mock_storage so the raised exception flows into the endpoint call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/routes/stt_evaluations/dataset.py`:
- Around line 171-174: Wrap the call to storage.get_signed_url(...) in a
try/except that catches CloudStorageError and falls back to signed_url = None so
a failure to generate a signed URL doesn't crash the request; update the block
where storage and s.file_id are checked (variables: storage, s, file_map,
signed_url) to mirror the defensive pattern used in
backend/app/crud/stt_evaluations/result.py, logging or swallowing the
CloudStorageError as appropriate and ensuring signed_url is set to None on
error.
In `@backend/app/api/routes/tts_evaluations/result.py`:
- Around line 111-119: When include_signed_url is true, guard the cloud storage
flow to avoid raising unhandled exceptions: check result.object_store_url for
truthiness before calling get_cloud_storage or storage.get_signed_url; wrap
calls to get_cloud_storage(...) and storage.get_signed_url(...) in try/except
that catches ValueError and CloudStorageError (and any initialization errors),
log the error, and leave signed_url = None on failure; then pass signed_url
(possibly None) into TTSResultPublic.from_model so the endpoint returns a
graceful success response instead of a 500.
In `@backend/app/crud/stt_evaluations/result.py`:
- Line 106: The signed_url assignment using storage.get_signed_url may raise
CloudStorageError and must be wrapped in a try/except: catch
backend.app.core.cloud.storage.CloudStorageError (or the imported alias) around
the call in result.py where signed_url =
storage.get_signed_url(file.object_store_url), log the error (or debug) and set
signed_url = None on failure so a single URL failure doesn't raise a 500; mirror
the defensive pattern used in services/stt_evaluations/helpers.py and
crud/stt_evaluations/batch.py.
In `@backend/app/crud/tts_evaluations/result.py`:
- Around line 133-138: In the loop that builds results (where results = [] and
you call TTSResultPublic.from_model), guard against None and storage failures by
only calling storage.get_signed_url when result.object_store_url is not None,
and wrap that call in a try/except that catches CloudStorageError (or the
storage client's specific exception) to set signed_url = None on failure; also
consider logging the exception for debugging before continuing so a single
failed signed-URL generation won’t crash the whole request.
---
Outside diff comments:
In `@backend/app/tests/api/routes/test_stt_evaluation.py`:
- Around line 743-774: The test test_get_stt_dataset_signed_url_failure
currently mocks get_signed_url to return None but CloudStorage.get_signed_url
raises CloudStorageError on failure; update the test to simulate the real
failure by configuring mock_get_cloud_storage() (the MagicMock returned) so its
get_signed_url side_effect raises CloudStorageError, then assert the route
/api/v1/evaluations/stt/datasets/{dataset.id} (the handler that calls
CloudStorage.get_signed_url) still returns 200 and that the sample's signed_url
is None (verifying the production code catches CloudStorageError).
- Around line 1076-1123: Update the test to simulate a real signed URL failure
by having the mocked storage client's get_signed_url raise an exception (e.g.,
mock_storage.get_signed_url.side_effect = Exception("signed url error")) instead
of returning None, and ensure the production handler that builds run responses
(the STT run retrieval logic used by the /api/v1/evaluations/stt/runs endpoint
and any function that calls storage.get_signed_url) catches that exception and
sets sample["signed_url"] = None so the test asserts remain valid; use
mock_get_cloud_storage to return the mock_storage so the raised exception flows
into the endpoint call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33aea131-3ee0-4f02-8ca5-73b1f6b1012d
📒 Files selected for processing (5)
backend/app/api/routes/stt_evaluations/dataset.pybackend/app/api/routes/tts_evaluations/result.pybackend/app/crud/stt_evaluations/result.pybackend/app/crud/tts_evaluations/result.pybackend/app/tests/api/routes/test_stt_evaluation.py
Comment on lines
+171
to
+174
| if storage and s.file_id in file_map: | ||
| signed_url = storage.get_signed_url( | ||
| file_map[s.file_id].object_store_url | ||
| ) |
There was a problem hiding this comment.
Missing error handling for storage.get_signed_url() will cause request failures.
Same issue as in backend/app/crud/stt_evaluations/result.py: the get_signed_url method raises CloudStorageError on AWS failures. Without try/except, any URL generation failure will return a 500 error to the client instead of gracefully setting signed_url = None for that sample.
🛡️ Proposed fix to restore defensive error handling
samples = []
for s in sample_records:
signed_url = None
- if storage and s.file_id in file_map:
- signed_url = storage.get_signed_url(
- file_map[s.file_id].object_store_url
- )
+ if storage and s.file_id in file_map:
+ try:
+ signed_url = storage.get_signed_url(
+ file_map[s.file_id].object_store_url
+ )
+ except Exception:
+ logger.warning(
+ f"[get_dataset] Failed to generate signed URL for file {s.file_id}"
+ )
+ signed_url = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/routes/stt_evaluations/dataset.py` around lines 171 - 174,
Wrap the call to storage.get_signed_url(...) in a try/except that catches
CloudStorageError and falls back to signed_url = None so a failure to generate a
signed URL doesn't crash the request; update the block where storage and
s.file_id are checked (variables: storage, s, file_map, signed_url) to mirror
the defensive pattern used in backend/app/crud/stt_evaluations/result.py,
logging or swallowing the CloudStorageError as appropriate and ensuring
signed_url is set to None on error.
Comment on lines
+111
to
+119
| signed_url = None | ||
| if include_signed_url: | ||
| storage = get_cloud_storage( | ||
| session=session, project_id=auth_context.project_.id | ||
| ) | ||
| signed_url = storage.get_signed_url(result.object_store_url) | ||
|
|
||
| return APIResponse.success_response( | ||
| data=TTSResultPublic.from_model(result, signed_url=signed_url) |
There was a problem hiding this comment.
Missing error handling for cloud storage operations will cause 500 errors.
Multiple unhandled exception sources:
get_cloud_storage()can raiseValueErrorif project not found, or re-raise initialization errors (seebackend/app/core/cloud/storage.py:267-284).storage.get_signed_url()raisesCloudStorageErroron AWS failures.result.object_store_urlmay beNonefor pending/failed results.
All of these will return 500 errors instead of gracefully returning signed_url = None.
🛡️ Proposed fix with comprehensive error handling
signed_url = None
if include_signed_url:
- storage = get_cloud_storage(
- session=session, project_id=auth_context.project_.id
- )
- signed_url = storage.get_signed_url(result.object_store_url)
-
- return APIResponse.success_response(
- data=TTSResultPublic.from_model(result, signed_url=signed_url)
- )
+ if result.object_store_url:
+ try:
+ storage = get_cloud_storage(
+ session=session, project_id=auth_context.project_.id
+ )
+ signed_url = storage.get_signed_url(result.object_store_url)
+ except Exception:
+ logger.warning(
+ f"[get_result] Failed to generate signed URL for result {result_id}"
+ )
+ signed_url = None
+
+ return APIResponse.success_response(
+ data=TTSResultPublic.from_model(result, signed_url=signed_url)
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/routes/tts_evaluations/result.py` around lines 111 - 119,
When include_signed_url is true, guard the cloud storage flow to avoid raising
unhandled exceptions: check result.object_store_url for truthiness before
calling get_cloud_storage or storage.get_signed_url; wrap calls to
get_cloud_storage(...) and storage.get_signed_url(...) in try/except that
catches ValueError and CloudStorageError (and any initialization errors), log
the error, and leave signed_url = None on failure; then pass signed_url
(possibly None) into TTSResultPublic.from_model so the endpoint returns a
graceful success response instead of a 500.
| logger.warning( | ||
| f"[get_results_by_run_id] Failed to generate signed URL: {e}" | ||
| ) | ||
| signed_url = storage.get_signed_url(file.object_store_url) if storage else None |
There was a problem hiding this comment.
Missing error handling for storage.get_signed_url() will cause request failures.
The get_signed_url method raises CloudStorageError on AWS failures (see backend/app/core/cloud/storage.py:241-247). Without try/except, a single URL generation failure will crash the entire request with a 500 error instead of gracefully returning None for that sample's signed_url.
This contradicts the PR description's claim of "improved error handling...to gracefully handle failures without raising exceptions" and breaks the established codebase pattern (see backend/app/services/stt_evaluations/helpers.py:27-35 and backend/app/crud/stt_evaluations/batch.py:96-106).
🛡️ Proposed fix to restore defensive error handling
results = []
for result, sample, file in rows:
- signed_url = storage.get_signed_url(file.object_store_url) if storage else None
+ signed_url = None
+ if storage:
+ try:
+ signed_url = storage.get_signed_url(file.object_store_url)
+ except Exception:
+ logger.warning(
+ f"[get_results_by_run_id] Failed to generate signed URL for file {file.id}"
+ )
+ signed_url = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/crud/stt_evaluations/result.py` at line 106, The signed_url
assignment using storage.get_signed_url may raise CloudStorageError and must be
wrapped in a try/except: catch backend.app.core.cloud.storage.CloudStorageError
(or the imported alias) around the call in result.py where signed_url =
storage.get_signed_url(file.object_store_url), log the error (or debug) and set
signed_url = None on failure so a single URL failure doesn't raise a 500; mirror
the defensive pattern used in services/stt_evaluations/helpers.py and
crud/stt_evaluations/batch.py.
Comment on lines
+133
to
+138
| results = [] | ||
| for result in rows: | ||
| signed_url = ( | ||
| storage.get_signed_url(result.object_store_url) if storage else None | ||
| ) | ||
| results.append(TTSResultPublic.from_model(result, signed_url=signed_url)) |
There was a problem hiding this comment.
Missing error handling and null check for signed URL generation.
Two issues:
storage.get_signed_url()raisesCloudStorageErroron AWS failures, which will crash the entire request.result.object_store_urlmay beNonefor pending/failed TTS results that haven't generated audio yet. Callingget_signed_url(None)will fail.
🛡️ Proposed fix with error handling and null check
results = []
for result in rows:
- signed_url = (
- storage.get_signed_url(result.object_store_url) if storage else None
- )
+ signed_url = None
+ if storage and result.object_store_url:
+ try:
+ signed_url = storage.get_signed_url(result.object_store_url)
+ except Exception:
+ logger.warning(
+ f"[get_results_by_run_id] Failed to generate signed URL for result {result.id}"
+ )
+ signed_url = None
results.append(TTSResultPublic.from_model(result, signed_url=signed_url))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/crud/tts_evaluations/result.py` around lines 133 - 138, In the
loop that builds results (where results = [] and you call
TTSResultPublic.from_model), guard against None and storage failures by only
calling storage.get_signed_url when result.object_store_url is not None, and
wrap that call in a try/except that catches CloudStorageError (or the storage
client's specific exception) to set signed_url = None on failure; also consider
logging the exception for debugging before continuing so a single failed
signed-URL generation won’t crash the whole request.
vprashrex
approved these changes
Mar 12, 2026
| storage = get_cloud_storage( | ||
| session=session, project_id=auth_context.project_.id | ||
| ) | ||
| signed_url = storage.get_signed_url(result.object_store_url) |
Collaborator
There was a problem hiding this comment.
if result.object_store url is None then it would raise some issue . so add some condition if result.object_store_url is not None. if its not an issue then u can ignore it
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/app/api/routes/stt_evaluations/dataset.py (1)
171-174:⚠️ Potential issue | 🟠 MajorGuard signed URL generation so one sample does not fail the entire dataset response.
Lines 171-174 still call
storage.get_signed_url(...)without a fallback. Sinceget_signed_urlraisesCloudStorageError, any AWS presign failure will turn this endpoint into a 500 instead of returning that sample withsigned_url=None.🛡️ Suggested fix
for s in sample_records: signed_url = None if storage and s.file_id in file_map: - signed_url = storage.get_signed_url( - file_map[s.file_id].object_store_url - ) + try: + signed_url = storage.get_signed_url( + file_map[s.file_id].object_store_url + ) + except CloudStorageError: + logger.warning( + f"[get_dataset] Failed to generate signed URL for file {s.file_id}" + )Also add
CloudStorageErrorto the imports for this module. As per coding guidelines, "Prefix all log messages with the function name in square brackets:logger.info(f\"[function_name] Message {mask_string(sensitive_value)}\")".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/stt_evaluations/dataset.py` around lines 171 - 174, Wrap the call to storage.get_signed_url(file_map[s.file_id].object_store_url) in a try/except that catches CloudStorageError and on exception sets signed_url = None for that sample (so one failing presign doesn't 500 the whole response) and logs the failure using the local logger with the current function name in square brackets (e.g. logger.error(f"[function_name] Failed to presign {s.file_id}: {mask_string(err)}")). Also add CloudStorageError to the module imports. Ensure you still assign signed_url when the call succeeds and reference storage.get_signed_url, CloudStorageError, s and file_map to locate the code.backend/app/crud/stt_evaluations/result.py (1)
7-7:⚠️ Potential issue | 🟠 MajorHandle
get_signed_url()exceptions per row.Line 106 still calls
storage.get_signed_url(...)directly.backend/app/core/cloud/storage.pyraisesCloudStorageErroron presign failures, so one bad file will 500 the whole run response instead of returning that sample withsigned_url=None, which is the behavior described in this PR.🛡️ Suggested fix
-from app.core.cloud.storage import CloudStorage +from app.core.cloud.storage import CloudStorage, CloudStorageError @@ - signed_url = storage.get_signed_url(file.object_store_url) if storage else None + signed_url = None + if storage: + try: + signed_url = storage.get_signed_url(file.object_store_url) + except CloudStorageError: + logger.warning( + f"[get_results_by_run_id] Failed to generate signed URL for file {file.id}" + )As per coding guidelines, "Prefix all log messages with the function name in square brackets:
logger.info(f\"[function_name] Message {mask_string(sensitive_value)}\")".Also applies to: 106-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/crud/stt_evaluations/result.py` at line 7, Wrap the per-row call to storage.get_signed_url(...) in a try/except that catches CloudStorageError so a single presign failure sets signed_url = None for that sample and does not abort the whole response; log the exception using the module logger and follow the prefix rule (logger.error(f"[<function_name>] ...")) and mask sensitive values with mask_string(...) in the message. Locate the call to storage.get_signed_url and replace the direct call with the try/except block catching CloudStorageError, assign signed_url = None on error, continue processing remaining rows, and ensure the log message includes the function name in square brackets and uses mask_string for any sensitive data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/tests/api/routes/test_stt_evaluation.py`:
- Around line 3-10: The mocks in test_stt_evaluation.py are returning None for
the failure paths but the real get_signed_url() raises CloudStorageError on
failure; update the failing-path mocks to raise CloudStorageError instead of
returning None so the tests exercise the actual error-handling path; locate the
mock/patch of get_signed_url() in the tests and replace the return value with
raising CloudStorageError (import or reference CloudStorageError) so routes/CRUD
code hit the same exception flow as production.
---
Duplicate comments:
In `@backend/app/api/routes/stt_evaluations/dataset.py`:
- Around line 171-174: Wrap the call to
storage.get_signed_url(file_map[s.file_id].object_store_url) in a try/except
that catches CloudStorageError and on exception sets signed_url = None for that
sample (so one failing presign doesn't 500 the whole response) and logs the
failure using the local logger with the current function name in square brackets
(e.g. logger.error(f"[function_name] Failed to presign {s.file_id}:
{mask_string(err)}")). Also add CloudStorageError to the module imports. Ensure
you still assign signed_url when the call succeeds and reference
storage.get_signed_url, CloudStorageError, s and file_map to locate the code.
In `@backend/app/crud/stt_evaluations/result.py`:
- Line 7: Wrap the per-row call to storage.get_signed_url(...) in a try/except
that catches CloudStorageError so a single presign failure sets signed_url =
None for that sample and does not abort the whole response; log the exception
using the module logger and follow the prefix rule
(logger.error(f"[<function_name>] ...")) and mask sensitive values with
mask_string(...) in the message. Locate the call to storage.get_signed_url and
replace the direct call with the try/except block catching CloudStorageError,
assign signed_url = None on error, continue processing remaining rows, and
ensure the log message includes the function name in square brackets and uses
mask_string for any sensitive data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: edff079e-0082-40d4-aa2c-2b99773293e6
📒 Files selected for processing (5)
backend/app/api/routes/stt_evaluations/dataset.pybackend/app/crud/stt_evaluations/result.pybackend/app/crud/tts_evaluations/cron.pybackend/app/crud/tts_evaluations/result.pybackend/app/tests/api/routes/test_stt_evaluation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/crud/tts_evaluations/result.py
Comment on lines
+3
to
+10
| import pytest | ||
| from fastapi.testclient import TestClient | ||
| from sqlmodel import Session | ||
|
|
||
| from app.models import EvaluationDataset, EvaluationRun, File, FileType | ||
| from app.models.stt_evaluation import STTSample, STTResult, EvaluationType | ||
| from app.core.util import now | ||
| from app.crud.language import get_language_by_locale | ||
| from app.models import EvaluationDataset, EvaluationRun, File, FileType | ||
| from app.models.stt_evaluation import EvaluationType, STTResult, STTSample |
There was a problem hiding this comment.
Keep the failure-path mocks exception-based so these tests cover the real contract.
Line 744 and Line 1077 now return None, but get_signed_url() fails by raising CloudStorageError, not by returning None. This change stops both tests from exercising the actual graceful-error-handling path, so they will no longer catch the 500 regression in the route/CRUD code.
🧪 Suggested fix
+from app.core.cloud.storage import CloudStorageError
from app.core.util import now
@@
- mock_storage.get_signed_url.return_value = None
+ mock_storage.get_signed_url.side_effect = CloudStorageError("S3 error")
@@
- mock_storage.get_signed_url.return_value = None
+ mock_storage.get_signed_url.side_effect = CloudStorageError("S3 error")Also applies to: 744-745, 1077-1078
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/tests/api/routes/test_stt_evaluation.py` around lines 3 - 10, The
mocks in test_stt_evaluation.py are returning None for the failure paths but the
real get_signed_url() raises CloudStorageError on failure; update the
failing-path mocks to raise CloudStorageError instead of returning None so the
tests exercise the actual error-handling path; locate the mock/patch of
get_signed_url() in the tests and replace the return value with raising
CloudStorageError (import or reference CloudStorageError) so routes/CRUD code
hit the same exception flow as production.
This was referenced Mar 17, 2026
Merged
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
2 participants
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.
Summary
Target issue is #667
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
New Features
include_signed_urlparameter to evaluation endpoints, allowing users to optionally retrieve signed URLs for generated audio files in responses.Bug Fixes
Summary by CodeRabbit
New Features
Bug Fixes