Refactor job status handling to align with RQ standards#274
Refactor job status handling to align with RQ standards#274AnkushMalaker wants to merge 14 commits intopre-release-candidatefrom
Conversation
- Updated job status checks across various modules to use "started" and "finished" instead of "processing" and "completed" for consistency with RQ's naming conventions. - Adjusted related logging and response messages to reflect the new status terminology. - Simplified Docker Compose project name handling in test scripts to avoid conflicts and improve clarity in test environment setup.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughComprehensive refactoring normalizing job status terminology from RQ non-standard terms (processing/completed/cancelled) to canonical RQ statuses (started/finished/canceled), updating MongoDB database naming from "friend-lite" to "chronicle", removing dynamic Docker Compose project name handling, adding audio resampling capabilities with FFmpeg, enhancing session lifecycle management with explicit completion reasons, and updating extensive test infrastructure accordingly. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes This is a substantial, multi-pattern refactoring affecting nearly every component in the system. The changes are heterogeneous, spanning database configuration, status terminology normalization across services/controllers/workers/routes/frontend, Docker Compose project naming, audio processing, session lifecycle management, test infrastructure hardening, and extensive robot test updates. While individual changes follow consistent patterns (status name replacements), each affected area requires separate reasoning to ensure completeness and correctness of the migration. The density and breadth of logic modifications, combined with cascading effects across multiple architectural layers, justifies a critical complexity rating. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ 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. Comment |
- Increased `SPEECH_INACTIVITY_THRESHOLD_SECONDS` to 20 seconds in `docker-compose-test.yml` for better audio duration handling during tests. - Refactored session handling in `session_controller.py` to clarify client ID usage. - Updated `conversation_utils.py` to track speech activity using audio timestamps, enhancing accuracy in inactivity detection. - Simplified test scripts by removing unnecessary `COMPOSE_PROJECT_NAME` references, aligning with the new project naming convention. - Adjusted integration tests to reflect changes in inactivity timeout and ensure proper handling of audio timestamps.
- Updated `worker_orchestrator.py` to use `logger.exception` for improved error logging. - Changed default MongoDB database name from "friend-lite" to "chronicle" in multiple files for consistency. - Added a new method `close_stream_without_stop` in `audio_stream_client.py` to handle abrupt WebSocket disconnections. - Enhanced audio validation in `audio_utils.py` to support automatic resampling of audio data if sample rates do not match. - Improved logging in various modules to provide clearer insights during audio processing and event dispatching.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
extras/speaker-recognition/src/simple_speaker_recognition/api/routers/identification.py (1)
433-472: Ensure backward compatibility for thewordsresponse field.The code currently returns
wordsas full word objects (dictionaries containing timestamps and metadata). If this field previously returned a list of strings, this is a breaking change for API clients expectinglist[str].Consider either:
- Keeping
wordsas a list of strings and introducing a new field likeword_segmentsfor the full objects- Documenting this as a breaking change and updating client code and API versioning accordingly
Example backward-compatible approach
if speaker_info and confidence >= similarity_threshold: # Identified speaker enhanced_segments.append({ "text": segment_text, "start": round(start_time, 3), "end": round(end_time, 3), "speaker": speaker_label, "identified_as": speaker_info["name"], "speaker_id": speaker_info["id"], "confidence": round(float(confidence), 3), "status": "identified", - "words": segment_words + "words": [w.get("word", "") for w in segment_words], + "word_segments": segment_words }) else: # Unknown speaker enhanced_segments.append({ "text": segment_text, "start": round(start_time, 3), "end": round(end_time, 3), "speaker": speaker_label, "identified_as": None, "speaker_id": None, "confidence": round(float(confidence), 3) if confidence else 0.0, "status": "unknown", - "words": segment_words + "words": [w.get("word", "") for w in segment_words], + "word_segments": segment_words })tests/integration/integration_test.robot (1)
199-219: Confusing use of "started" in comments.Comments like "may still be started" and "memory started to be in progress" are unclear. The status name "started" shouldn't replace descriptive terms like "processing" in comments.
Suggested fix
- # Check if conversation has memory count (may still be started) + # Check if conversation has memory count (may still be processing) ... - # Verify memory extraction status (allow for memory started to be in progress) + # Verify memory extraction status (allow for memory processing to be in progress) ... - Log Memory extraction verification passed (may still be started) INFO + Log Memory extraction verification passed (may still be processing) INFObackends/advanced/src/advanced_omi_backend/models/job.py (1)
49-53: Exception handling defeats the fallback logic.The
raiseon line 53 immediately re-raises theConfigurationError, making the fallback assignment on line 52 useless. The exception propagates up, andinit_beanieis never called with the fallback database.🐛 Proposed fix: remove the re-raise to use the fallback
try: database = client.get_default_database(mongodb_database) except ConfigurationError: database = client[mongodb_database] - raise - _beanie_initialized = TrueAlso note:
_beanie_initialized = Trueis set twice (lines 54 and 61). The first assignment on line 54 can be removed as it's redundant.backends/advanced/src/advanced_omi_backend/routers/modules/obsidian_routes.py (1)
175-185: Add "started" to the meta status allowlist.Producers write
job.meta["status"] = "started"(obsidian_jobs.py:37), but the allowlist on line 184 only accepts("running", "finished", "failed", "canceled"). This means the meta override check will never match when a job is in the started state, and the granular status update feature won't work as intended. Either add"started"to the allowlist or update producers to write"running"instead.backends/advanced/webui/src/pages/Queue.tsx (1)
889-895: Canceled jobs are treated as active in several filters.Canceled is terminal in RQ; leaving it out keeps conversations/jobs “active” indefinitely. Consider treating
canceledas terminal alongsidefinished/failed.✅ Suggested adjustment
- job.status !== 'finished' && - job.status !== 'failed' + job.status !== 'finished' && + job.status !== 'failed' && + job.status !== 'canceled'- const hasActiveJob = jobs.some(j => j.status !== 'finished' && j.status !== 'failed'); + const hasActiveJob = jobs.some(j => + j.status !== 'finished' && j.status !== 'failed' && j.status !== 'canceled' + );- const allJobsComplete = jobs.every(j => j.status === 'finished' || j.status === 'failed'); + const allJobsComplete = jobs.every(j => + j.status === 'finished' || j.status === 'failed' || j.status === 'canceled' + );Also applies to: 1092-1096, 1534-1537
backends/advanced/src/advanced_omi_backend/controllers/queue_controller.py (1)
533-541: Passend_reasonparameter throughstart_post_conversation_jobs()to avoid job ID collision and incorrect labeling.
open_conversation_job(streaming) callsstart_post_conversation_jobs()and then separately enqueuesdispatch_conversation_complete_event_jobwith the same job_id but different parameters—causing a collision. Additionally, hardcoding"file_upload"mislabels streaming conversations that end for different reasons (user_stopped, inactivity_timeout, etc.). Add an optionalend_reasonparameter tostart_post_conversation_jobs()with a default of"file_upload", and pass the dynamic reason from streaming contexts.Suggested direction
-def start_post_conversation_jobs( - conversation_id: str, - user_id: str, - transcript_version_id: Optional[str] = None, - depends_on_job = None, - client_id: Optional[str] = None -) -> Dict[str, str]: +def start_post_conversation_jobs( + conversation_id: str, + user_id: str, + transcript_version_id: Optional[str] = None, + depends_on_job = None, + client_id: Optional[str] = None, + end_reason: Optional[str] = None, +) -> Dict[str, str]:- "file_upload", # Explicit end_reason for file upload processing + end_reason or "file_upload",
🤖 Fix all issues with AI agents
In `@backends/advanced/src/advanced_omi_backend/routers/modules/queue_routes.py`:
- Around line 467-471: The API broke compatibility by renaming include_completed
→ include_finished; update the backend to accept both names and preserve
behavior: add an optional include_completed: Optional[bool] field to the
FlushAllJobsRequest Pydantic model (and any equivalent request models used by
the same endpoints), then ensure request handling uses include_flag =
include_finished or include_completed (so include_completed still triggers
flushing finished jobs) and mark include_completed as deprecated in a comment;
alternatively, update the web UI (Queue.tsx) to send include_finished and
correct its comment—prefer adding include_completed to FlushAllJobsRequest and
mapping it to include_finished in the router handlers to avoid breaking clients.
In `@backends/advanced/src/advanced_omi_backend/utils/conversation_utils.py`:
- Around line 462-480: track_speech_activity is returning mixed timestamp types
(audio-relative speech_end vs wall-clock time), causing
update_job_progress_metadata's inactivity_seconds = time.time() -
last_meaningful_speech_time to be incorrect; fix by normalizing
last_meaningful_speech_time to always be wall-clock: when speech_end is present,
convert it to wall-clock using the stream start time (e.g., wall_clock =
stream_start_time + speech_end) before returning/setting
last_meaningful_speech_time, and when speech_end is missing keep using
time.time(); update the track_speech_activity docstring to state it returns
wall-clock timestamps and ensure update_job_progress_metadata continues to
compute inactivity_seconds against that consistent wall-clock value.
In `@backends/advanced/src/advanced_omi_backend/workers/conversation_jobs.py`:
- Around line 689-707: The duplicate enqueue causing job_id collision is created
when dispatch_conversation_complete_event_job is enqueued twice with the same
job_id pattern event_complete_{conversation_id[:12]} and a hardcoded end_reason;
update start_post_conversation_jobs to accept an end_reason parameter (change
its signature to include end_reason), replace the hardcoded "file_upload" inside
start_post_conversation_jobs with the passed end_reason so the correct reason is
used, and remove the redundant enqueue block that enqueues
dispatch_conversation_complete_event_job a second time (the block creating
default_queue and dispatch_job) so only the single, parameterized enqueue
remains.
In `@backends/advanced/src/advanced_omi_backend/workers/speaker_jobs.py`:
- Around line 211-215: The loop building actual_words can crash when seg.words
is None; update both extraction sites (the block that iterates
transcript_version.segments to populate actual_words and the similar code around
lines 355-357) to guard with "or []" so you iterate safely: use "for seg in
transcript_version.segments or []" and "for word in seg.words or []" (and apply
the same or [] fallback to any other word-list extraction path in
speaker_jobs.py where actual_words is constructed).
In `@backends/advanced/upload_files.py`:
- Around line 324-332: The polling logic currently treats only "processing" as
an in-progress status but the backend now emits "started" which triggers
"Unknown job status" warnings; update the status checks so that "started" is
treated the same as "processing" (i.e., include "started" in the in-progress
branch) and ensure the completion branch still checks for status == "finished"
(see variables and logic around status, start_time, logger, and job_status in
the polling loop). Locate the polling loop that evaluates status and the
"Unknown job status" warning and add "started" to the accepted in-progress
statuses (or adjust the condition to check status in {"processing", "started"})
so the loop continues polling without spurious warnings; keep the finalization
code that runs when status == "finished" (files, completed, failed) unchanged.
In `@backends/advanced/webui/src/pages/Queue.tsx`:
- Around line 132-138: The flush UI and payload still use
include_completed/completed; update the state and any payloads to use
include_finished/finished instead: change the useState initial key
include_completed to include_finished in the flushSettings object and rename all
references to include_completed (and UI labels saying “completed”) to
include_finished (and labels saying “finished”) so the checkbox toggles are sent
as include_finished to the backend; ensure any places that build the flush API
payload or read flushSettings (e.g., the components and handlers around the
flush logic where include_completed is referenced) are updated accordingly,
including the other occurrences noted.
In `@tests/integration/conversation_queue.robot`:
- Around line 80-82: The log messages in the Robot test use awkward phrasing
("Waiting for initial upload started to complete..." and "Waiting for initial
conversation started to complete..."); update the Log entries' message strings
to clearer, grammatical phrases (for example, replace "Waiting for initial
upload started to complete..." with "Waiting for initial upload to complete..."
and "Waiting for initial conversation started to complete..." with "Waiting for
initial conversation to complete" or "Waiting for conversation setup to
complete") so the intent of the Sleep/wait is unambiguous; locate and edit the
Log lines in the conversation_queue.robot test (the Log statements containing
those exact strings) to use the improved wording.
In `@tests/integration/integration_test.robot`:
- Around line 63-64: Update the confusing inline comment above the Sleep
keyword: replace "post-started jobs" with the more descriptive "post-processing
jobs" so the comment reads "Wait for post-processing jobs" to clarify what the
test is waiting for; locate the line containing the Sleep call in
integration_test.robot and edit only the comment text (not the Sleep keyword or
arguments).
- Around line 33-36: Replace the awkward Log messages around the Upload Audio
File step to use clear, grammatical status phrases: update the Log before
calling the Upload Audio File keyword (which assigns ${conversation} from Upload
Audio File ${TEST_AUDIO_FILE} ${TEST_DEVICE_NAME}) to something like "Starting
audio upload" and change the Log after it to "Audio upload completed,
conversation created" (or similar concise, present/past-tense status text) so
the test reads clearly.
- Line 25: Update the test documentation string in the integration_test.robot's
[Documentation] setting to replace the ambiguous phrase "audio started pipeline"
with "audio processing pipeline" (or similar wording that uses "processing"
instead of the job-status word "started") so the test describes pipeline
activity clearly; locate the [Documentation] entry in
tests/integration/integration_test.robot and update its text accordingly.
- Around line 119-127: Replace ambiguous uses of the word "started" that treat
it as a noun with clearer phrasing: change occurrences of "audio started
pipeline" to "audio start pipeline" or "audio start workflow", change "full
started" to "start completed" or "start completion", and change log text
"waiting for full started" to "waiting for start completion" (also consider
"fully started" only where describing a state). Update the Log and descriptive
strings in the test file that contain "started" so they use "start", "start
completion", "start event", or "fully started" as appropriate to clarify you
mean the pipeline start activity rather than a job status.
In `@tests/integration/websocket_transcription_e2e_test.robot`:
- Around line 136-137: The comment above the Wait For Job Status step is
grammatically incorrect; update the comment that precedes the Wait For Job
Status call for ${conv_job}[job_id] (the line referencing the inactivity
timeout) to a clear, concise description such as indicating we are waiting for
the conversation to finish due to the inactivity timeout so it accurately
documents the Wait For Job Status ${conv_job}[job_id] finished timeout=60s call.
In `@tests/resources/audio_keywords.robot`:
- Around line 59-65: Update the misleading status wording in the test assertions
and docstrings so they reflect the actual lifecycle state being waited on
("finished" or "completed" rather than "started"); specifically, change the
error string in the Should Not Be Equal call ("Conversation not found after
upload and started") to something like "Conversation not found after upload and
finished/completed", and update any docstrings or messages associated with the
Wait Until Keyword Succeeds / Check job status and Get Conversation By ID steps
(also the similar occurrence noted at the second location) to use consistent
"finished"/"completed" wording instead of "started".
In `@tests/resources/conversation_keywords.robot`:
- Line 126: Update the docstring on the test case that currently reads "Create a
test conversation by started a test audio file" to correct the grammar: change
"by started" to "by starting" so the line reads "Create a test conversation by
starting a test audio file" (edit the [Documentation] string in
tests/resources/conversation_keywords.robot).
In `@tests/resources/memory_keywords.robot`:
- Line 184: Replace the grammatically incorrect error string "Memory job failed
during started: ${error_info}" used as the error_message literal with a clear
phrasing such as "Memory job failed to start: ${error_info}" (locate the token
error_message=Memory job failed during started: ${error_info} in the
tests/resources/memory_keywords.robot diff and update that literal).
- Around line 173-177: The IF condition duplicates the same check ('${status}'
== 'finished' twice); update the condition to correctly check for the alternate
terminal status by replacing the second comparison with '${status}' ==
'completed' (i.e., IF '${status}' == 'finished' or '${status}' ==
'completed') so the IF in the memory job success block properly handles both
terminal states; ensure the surrounding IF/Log/BREAK/END block remains
unchanged.
🧹 Nitpick comments (18)
backends/advanced/worker_orchestrator.py (1)
216-216: Remove redundant exception object fromlogger.exceptioncalls.
logger.exception()automatically logs the full exception including traceback. Including{e}in the f-string duplicates the exception's string representation in the output.Suggested fix
- logger.exception(f"❌ Orchestrator error: {e}") + logger.exception("❌ Orchestrator error")- logger.exception(f"Fatal error: {e}") + logger.exception("Fatal error")Also applies to: 236-236
backends/advanced/src/advanced_omi_backend/utils/job_utils.py (1)
44-49: Include canceled sessions as natural exits (avoid false zombie errors).Line 47 only treats
"finalizing"and"finished"as natural termination. If sessions can also end as"canceled"(per standardized statuses), this path will log zombie errors for clean cancellations. Consider including"canceled"(and"cancelled"if that spelling exists) in the allowlist.Suggested tweak
- if session_status and session_status.decode() in ["finalizing", "finished"]: + if session_status and session_status.decode() in ["finalizing", "finished", "canceled"]:backends/advanced/src/advanced_omi_backend/plugins/test_event/event_storage.py (1)
130-143: Remove extraneousfprefixes from strings without placeholders.Lines 131 and 143 use f-strings but contain no interpolated values. This is flagged by static analysis (Ruff F541).
Suggested fix
# Add before serialization - logger.debug(f"💾 STORAGE: Serializing event data...") + logger.debug("💾 STORAGE: Serializing event data...") try: data_json = json.dumps(data) metadata_json = json.dumps(metadata) if metadata else None except Exception as e: logger.error( f"💾 STORAGE: JSON serialization failed for event '{event}': {e}", exc_info=True ) raise # Add before database operation - logger.debug(f"💾 STORAGE: Inserting into plugin_events table...") + logger.debug("💾 STORAGE: Inserting into plugin_events table...")backends/advanced/src/advanced_omi_backend/plugins/test_event/plugin.py (1)
131-131: Remove extraneousfprefix from string without placeholders.Same as in
event_storage.py, this f-string has no interpolation.Suggested fix
# Add before storage - logger.info(f" 💾 Storing event to SQLite database...") + logger.info(" 💾 Storing event to SQLite database...")backends/advanced/src/advanced_omi_backend/plugins/router.py (3)
143-151: Consider log levels for high-frequency events.The logging is helpful for debugging. However, if many plugins are registered, the per-plugin condition checks at INFO level could be verbose in production. Consider using DEBUG level for lines 148 and 150 to reduce noise while retaining observability.
♻️ Suggested change
if not plugin.enabled: - logger.info(f" ⊘ Skipping '{plugin_id}': disabled") + logger.debug(f" ⊘ Skipping '{plugin_id}': disabled") continue # Check execution condition (wake_word, etc.) - logger.info(f" → Checking execution condition for '{plugin_id}'") + logger.debug(f" → Checking execution condition for '{plugin_id}'") if not await self._should_execute(plugin, data): - logger.info(f" ⊘ Skipping '{plugin_id}': condition not met") + logger.debug(f" ⊘ Skipping '{plugin_id}': condition not met") continue
178-183: Consider usinglogger.exception()for exception logging.Using
logger.exception()is the idiomatic way to log exceptions with tracebacks, and it automatically includesexc_info=True. This aligns with the pattern used inworker_orchestratorper the PR description.♻️ Suggested change
except Exception as e: - # CRITICAL: Log exception details - logger.error( - f" ✗ Plugin '{plugin_id}' FAILED with exception: {e}", - exc_info=True - ) + logger.exception(f" ✗ Plugin '{plugin_id}' FAILED with exception: {e}")
185-189: Minor: Log message may be misleading.The message says "executed successfully" but
len(results)counts all plugins that returned a result, including those whereresult.success=False. Consider rewording to "responded" or count only successful results.♻️ Suggested fix
# Add at end + successful_count = sum(1 for r in results if r.success) logger.info( f"🔌 ROUTER: Dispatch complete for '{event}': " - f"{len(results)} plugin(s) executed successfully" + f"{len(results)} plugin(s) responded, {successful_count} successful" )backends/advanced/src/advanced_omi_backend/utils/conversation_utils.py (3)
84-87: Consider DEBUG level for verbose diagnostic logs and simplify the condition.
- Logging word details and settings at INFO level may be too verbose for production. Consider using
logger.debug()for these diagnostic messages.- The condition
if words and len(words) > 0is redundant—if words:alone suffices since an empty list is falsy.- Logging transcript words (even first 3) at INFO level could inadvertently expose sensitive user speech data in production logs.
Suggested fix
- logger.info(f"🔬 analyze_speech: words_list_length={len(words)}, settings={settings}") - if words and len(words) > 0: - logger.info(f"📝 First 3 words: {words[:3]}") + logger.debug(f"🔬 analyze_speech: words_list_length={len(words)}, settings={settings}") + if words: + logger.debug(f"📝 First 3 words: {words[:3]}")
105-109: Reduce logging verbosity and avoid logging full word objects.This logs at INFO level with full word objects (
first_word,last_word) that contain transcript text. For production, consider:
- Using
logger.debug()for timing investigation logs- Logging only timing values (start/end) rather than full word objects to avoid exposing transcript content
Suggested fix
# Debug logging for timestamp investigation - logger.info( + logger.debug( f"🕐 Speech timing: start={speech_start:.2f}s, end={speech_end:.2f}s, " - f"duration={speech_duration:.2f}s (first_word={valid_words[0]}, last_word={valid_words[-1]})" + f"duration={speech_duration:.2f}s" )
119-119: Consider DEBUG level for threshold comparison logging.This diagnostic log would be more appropriate at DEBUG level in production environments.
tests/integration/websocket_streaming_tests.robot (1)
141-145: Derive the timeout from the inactivity setting to avoid drift.
Hard-coding 90s will go stale if the threshold changes again. Consider computing the timeout fromSPEECH_INACTIVITY_THRESHOLD_SECONDS.Suggested change
- # Timeout needs: (audio send time ~60s) + (silence timeout 20s) + (buffer 10s) = 90s - Wait For Job Status ${conv_job_id} finished timeout=90s interval=2s + # Timeout needs: (audio send time ~60s) + (silence timeout) + (buffer 10s) + ${inactivity_s}= Set Variable %{SPEECH_INACTIVITY_THRESHOLD_SECONDS} + ${timeout_s}= Evaluate int($inactivity_s) + 70 + Wait For Job Status ${conv_job_id} finished timeout=${timeout_s}s interval=2sbackends/advanced/docker-compose-test.yml (1)
198-200: Keep both services in sync.The threshold and comments match the backend service configuration, which is correct. Consider extracting this into an
.envfile or YAML anchor to avoid duplication drift.tests/bin/start-rebuild-containers.sh (2)
19-21: Consider usingdocker compose downinstead ofstop.
stoponly stops containers but leaves them and their state. For a rebuild workflow where you want clean state,downremoves containers and networks. This prevents stale container state from affecting rebuilt images.Suggested change
# Stop containers echo "🛑 Stopping containers..." -docker compose -f docker-compose-test.yml stop +docker compose -f docker-compose-test.yml down
27-29: Fixed sleep may be insufficient for startup.The docker-compose-test.yml defines
start_period: 60sfor the backend healthcheck, but this script only waits 5s before starting health checks. While the retry loop provides additional buffer, consider aligning the initial wait or documenting why 5s is sufficient.backends/advanced/src/advanced_omi_backend/utils/audio_utils.py (1)
146-173: Normalize auto-resample errors intoAudioValidationError.Right now, FFmpeg failures raise
RuntimeError, which may bypass any existing validation error handling. Consider wrapping to keep error classification consistent.♻️ Suggested guard
- processed_audio = await resample_audio_with_ffmpeg( - audio_data=processed_audio, - input_sample_rate=sample_rate, - input_channels=channels, - input_sample_width=sample_width, - target_sample_rate=expected_sample_rate, - target_channels=1 if convert_to_mono else channels - ) + try: + processed_audio = await resample_audio_with_ffmpeg( + audio_data=processed_audio, + input_sample_rate=sample_rate, + input_channels=channels, + input_sample_width=sample_width, + target_sample_rate=expected_sample_rate, + target_channels=1 if convert_to_mono else channels + ) + except Exception as e: + raise AudioValidationError(f"Auto-resample failed: {e}") from etests/.env.test (1)
20-20: Add trailing newline for POSIX compliance.The dotenv-linter flagged a missing blank line at EOF. Some tools and POSIX standards expect files to end with a newline.
🔧 Suggested fix
TEST_DB_NAME=test_db +tests/bin/status-containers.sh (1)
24-37: Add curl timeouts to avoid hanging health checks.Without a timeout, a stalled localhost request can block the script indefinitely.
Proposed tweak
-if curl -s http://localhost:8001/health > /dev/null 2>&1; then +if curl -s --max-time 3 http://localhost:8001/health > /dev/null 2>&1; then @@ -if curl -s http://localhost:8001/readiness > /dev/null 2>&1; then +if curl -s --max-time 3 http://localhost:8001/readiness > /dev/null 2>&1; thenbackends/advanced/src/advanced_omi_backend/services/audio_stream/producer.py (1)
219-242: Use explicit Optional type forcompletion_reason(RUF013).Line 219 uses an implicit Optional; Ruff flags this. Prefer
Optional[str](orstr | Noneif the project is 3.10+).♻️ Proposed fix
+from typing import Optional @@ - async def finalize_session(self, session_id: str, completion_reason: str = None): + async def finalize_session(self, session_id: str, completion_reason: Optional[str] = None):
| class FlushAllJobsRequest(BaseModel): | ||
| confirm: bool | ||
| include_failed: bool = False # By default, preserve failed jobs for debugging | ||
| include_completed: bool = False # By default, preserve completed jobs for debugging | ||
| include_finished: bool = False # By default, preserve finished jobs for debugging | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and verify the file exists and check its relevant sections
fd "queue_routes.py" | head -5Repository: SimpleOpenSoftware/chronicle
Length of output: 146
🏁 Script executed:
# Read the specific lines mentioned in the review comment
sed -n '467,471p' backends/advanced/src/advanced_omi_backend/routers/modules/queue_routes.pyRepository: SimpleOpenSoftware/chronicle
Length of output: 298
🏁 Script executed:
# Check the other mentioned lines
sed -n '588,593p' backends/advanced/src/advanced_omi_backend/routers/modules/queue_routes.py
sed -n '672,676p' backends/advanced/src/advanced_omi_backend/routers/modules/queue_routes.pyRepository: SimpleOpenSoftware/chronicle
Length of output: 568
🏁 Script executed:
# Search for any use of include_completed in the codebase
rg "include_completed" --no-heading -nRepository: SimpleOpenSoftware/chronicle
Length of output: 776
🏁 Script executed:
# Search for include_finished usage
rg "include_finished" --no-heading -nRepository: SimpleOpenSoftware/chronicle
Length of output: 762
🏁 Script executed:
# Check if there's a web UI and look for any API client code
find . -type f -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" | grep -E "(web|ui|client)" | head -10Repository: SimpleOpenSoftware/chronicle
Length of output: 695
🏁 Script executed:
# Check the third location mentioned: 672-676
sed -n '672,676p' backends/advanced/src/advanced_omi_backend/routers/modules/queue_routes.pyRepository: SimpleOpenSoftware/chronicle
Length of output: 264
🏁 Script executed:
# Verify the comment in Queue.tsx line 137 context
sed -n '135,140p' backends/advanced/webui/src/pages/Queue.tsxRepository: SimpleOpenSoftware/chronicle
Length of output: 404
🏁 Script executed:
# Search for any validation or field aliasing in the model to see if there's already compatibility
sed -n '467,480p' backends/advanced/src/advanced_omi_backend/routers/modules/queue_routes.pyRepository: SimpleOpenSoftware/chronicle
Length of output: 606
Breaking API change: include_completed → include_finished breaks flushing finished jobs in the web UI.
The web UI sends include_completed but the API expects include_finished. Pydantic silently ignores the unknown field, causing finished jobs to never flush even when the checkbox is set. The UI's comment at line 137 is also incorrect—it claims the API expects include_completed, but the API actually expects include_finished.
Either accept both fields for backward compatibility or update the web UI to send include_finished.
Backward compatibility approach
class FlushAllJobsRequest(BaseModel):
confirm: bool
include_failed: bool = False # By default, preserve failed jobs for debugging
include_finished: bool = False # By default, preserve finished jobs for debugging
+ include_completed: bool = False # Backward compatibility for older clients- if request.include_finished:
+ if request.include_finished or request.include_completed:
registries.append(("finished", FinishedJobRegistry(queue=queue)))Affects: lines 467-471, 588-593, 672-676, and backends/advanced/webui/src/pages/Queue.tsx lines 137, 554, 2568-2569.
🤖 Prompt for AI Agents
In `@backends/advanced/src/advanced_omi_backend/routers/modules/queue_routes.py`
around lines 467 - 471, The API broke compatibility by renaming
include_completed → include_finished; update the backend to accept both names
and preserve behavior: add an optional include_completed: Optional[bool] field
to the FlushAllJobsRequest Pydantic model (and any equivalent request models
used by the same endpoints), then ensure request handling uses include_flag =
include_finished or include_completed (so include_completed still triggers
flushing finished jobs) and mark include_completed as deprecated in a comment;
alternatively, update the web UI (Queue.tsx) to send include_finished and
correct its comment—prefer adding include_completed to FlushAllJobsRequest and
mapping it to include_finished in the router handlers to avoid breaking clients.
backends/advanced/src/advanced_omi_backend/workers/conversation_jobs.py
Outdated
Show resolved
Hide resolved
| if not actual_words and transcript_version.segments: | ||
| actual_words = [] | ||
| for seg in transcript_version.segments: | ||
| for word in seg.words: | ||
| actual_words.append({ |
There was a problem hiding this comment.
Guard against None word lists.
If legacy data or the speaker service returns null for words, iteration will fail. Use or [] to harden both extraction paths.
✅ Safe fallback
- for seg in transcript_version.segments:
- for word in seg.words:
+ for seg in transcript_version.segments:
+ for word in (seg.words or []):- words_data = seg.get("words", [])
+ words_data = seg.get("words") or []Also applies to: 355-357
🤖 Prompt for AI Agents
In `@backends/advanced/src/advanced_omi_backend/workers/speaker_jobs.py` around
lines 211 - 215, The loop building actual_words can crash when seg.words is
None; update both extraction sites (the block that iterates
transcript_version.segments to populate actual_words and the similar code around
lines 355-357) to guard with "or []" so you iterate safely: use "for seg in
transcript_version.segments or []" and "for word in seg.words or []" (and apply
the same or [] fallback to any other word-list extraction path in
speaker_jobs.py where actual_words is constructed).
| # Check completion status (RQ standard: "finished") | ||
| if status == "finished": | ||
| elapsed = time.time() - start_time | ||
| logger.info(f"🎉 Job completed successfully in {elapsed:.0f}s!") | ||
|
|
||
| # Show final file status summary | ||
| files = job_status.get("files", []) | ||
| completed = len([f for f in files if f.get("status") == "completed"]) | ||
| completed = len([f for f in files if f.get("status") == "finished"]) | ||
| failed = len([f for f in files if f.get("status") == "failed"]) |
There was a problem hiding this comment.
Update polling to accept started status as in RQ.
You switched completion to finished, but the polling loop still treats processing as in-progress. If the backend now emits started, the loop will warn on “Unknown job status” while still polling.
Proposed fix
- elif status in ["queued", "processing"]:
+ elif status in ["queued", "started", "processing"]:🤖 Prompt for AI Agents
In `@backends/advanced/upload_files.py` around lines 324 - 332, The polling logic
currently treats only "processing" as an in-progress status but the backend now
emits "started" which triggers "Unknown job status" warnings; update the status
checks so that "started" is treated the same as "processing" (i.e., include
"started" in the in-progress branch) and ensure the completion branch still
checks for status == "finished" (see variables and logic around status,
start_time, logger, and job_status in the polling loop). Locate the polling loop
that evaluates status and the "Unknown job status" warning and add "started" to
the accepted in-progress statuses (or adjust the condition to check status in
{"processing", "started"}) so the loop continues polling without spurious
warnings; keep the finalization code that runs when status == "finished" (files,
completed, failed) unchanged.
| # Wait for conversation to complete started (inactivity timeout) | ||
| Wait For Job Status ${conv_job}[job_id] finished timeout=60s interval=2s |
There was a problem hiding this comment.
Grammatically awkward comment.
The comment on Line 136 reads "Wait for conversation to complete started" which doesn't make sense. This appears to be an incomplete edit.
Suggested fix
- # Wait for conversation to complete started (inactivity timeout)
+ # Wait for conversation to finish (inactivity timeout)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Wait for conversation to complete started (inactivity timeout) | |
| Wait For Job Status ${conv_job}[job_id] finished timeout=60s interval=2s | |
| # Wait for conversation to finish (inactivity timeout) | |
| Wait For Job Status ${conv_job}[job_id] finished timeout=60s interval=2s |
🤖 Prompt for AI Agents
In `@tests/integration/websocket_transcription_e2e_test.robot` around lines 136 -
137, The comment above the Wait For Job Status step is grammatically incorrect;
update the comment that precedes the Wait For Job Status call for
${conv_job}[job_id] (the line referencing the inactivity timeout) to a clear,
concise description such as indicating we are waiting for the conversation to
finish due to the inactivity timeout so it accurately documents the Wait For Job
Status ${conv_job}[job_id] finished timeout=60s call.
| Wait Until Keyword Succeeds 60s 5s Check job status ${transcript_job_id} finished | ||
| ${job}= Get Job Details ${transcript_job_id} | ||
|
|
||
| # Get the completed conversation | ||
| # Get the finished conversation | ||
| ${conversation}= Get Conversation By ID ${job}[result][conversation_id] | ||
| Should Not Be Equal ${conversation} ${None} Conversation not found after upload and processing | ||
| Should Not Be Equal ${conversation} ${None} Conversation not found after upload and started | ||
|
|
There was a problem hiding this comment.
Clarify status wording in error/doc strings.
The error message says “started” even though the test waits for “finished,” and the docstring reads “complete started,” which is confusing. Please align the strings with the actual lifecycle.
💡 Suggested wording fix
- Should Not Be Equal ${conversation} ${None} Conversation not found after upload and started
+ Should Not Be Equal ${conversation} ${None} Conversation not found after upload and transcription finished
...
- [Documentation] Upload audio file and wait for complete started including memory extraction.
+ [Documentation] Upload audio file and wait for processing to finish, including memory extraction.Also applies to: 71-71
🤖 Prompt for AI Agents
In `@tests/resources/audio_keywords.robot` around lines 59 - 65, Update the
misleading status wording in the test assertions and docstrings so they reflect
the actual lifecycle state being waited on ("finished" or "completed" rather
than "started"); specifically, change the error string in the Should Not Be
Equal call ("Conversation not found after upload and started") to something like
"Conversation not found after upload and finished/completed", and update any
docstrings or messages associated with the Wait Until Keyword Succeeds / Check
job status and Get Conversation By ID steps (also the similar occurrence noted
at the second location) to use consistent "finished"/"completed" wording instead
of "started".
|
|
||
| Create Test Conversation | ||
| [Documentation] Create a test conversation by processing a test audio file | ||
| [Documentation] Create a test conversation by started a test audio file |
There was a problem hiding this comment.
Fix docstring grammar.
Line 126 reads “by started”; should be “by starting.”
✏️ Proposed fix
- [Documentation] Create a test conversation by started a test audio file
+ [Documentation] Create a test conversation by starting a test audio file📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [Documentation] Create a test conversation by started a test audio file | |
| [Documentation] Create a test conversation by starting a test audio file |
🤖 Prompt for AI Agents
In `@tests/resources/conversation_keywords.robot` at line 126, Update the
docstring on the test case that currently reads "Create a test conversation by
started a test audio file" to correct the grammar: change "by started" to "by
starting" so the line reads "Create a test conversation by starting a test audio
file" (edit the [Documentation] string in
tests/resources/conversation_keywords.robot).
| # Success case - job finished | ||
| IF '${status}' == 'finished' or '${status}' == 'finished' | ||
| Log Memory job finished successfully | ||
| BREAK | ||
| END |
There was a problem hiding this comment.
Duplicate condition in status check.
The condition checks the same value twice: '${status}' == 'finished' or '${status}' == 'finished'. This appears to be a copy-paste error from migrating away from completed.
Proposed fix
- IF '${status}' == 'finished' or '${status}' == 'finished'
+ IF '${status}' == 'finished'
Log Memory job finished successfully
BREAK
END📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Success case - job finished | |
| IF '${status}' == 'finished' or '${status}' == 'finished' | |
| Log Memory job finished successfully | |
| BREAK | |
| END | |
| # Success case - job finished | |
| IF '${status}' == 'finished' | |
| Log Memory job finished successfully | |
| BREAK | |
| END |
🤖 Prompt for AI Agents
In `@tests/resources/memory_keywords.robot` around lines 173 - 177, The IF
condition duplicates the same check ('${status}' == 'finished' twice); update
the condition to correctly check for the alternate terminal status by replacing
the second comparison with '${status}' == 'completed' (i.e., IF '${status}'
== 'finished' or '${status}' == 'completed') so the IF in the memory job success
block properly handles both terminal states; ensure the surrounding
IF/Log/BREAK/END block remains unchanged.
| ${result}= Create Dictionary | ||
| ... success=${False} | ||
| ... error_message=Memory job failed during processing: ${error_info} | ||
| ... error_message=Memory job failed during started: ${error_info} |
There was a problem hiding this comment.
Grammatically incorrect error message.
The message "Memory job failed during started" doesn't make sense. It appears the status terminology was updated but the phrasing wasn't adjusted accordingly.
Proposed fix
- ... error_message=Memory job failed during started: ${error_info}
+ ... error_message=Memory job failed while running: ${error_info}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ... error_message=Memory job failed during started: ${error_info} | |
| ... error_message=Memory job failed while running: ${error_info} |
🤖 Prompt for AI Agents
In `@tests/resources/memory_keywords.robot` at line 184, Replace the grammatically
incorrect error string "Memory job failed during started: ${error_info}" used as
the error_message literal with a clear phrasing such as "Memory job failed to
start: ${error_info}" (locate the token error_message=Memory job failed during
started: ${error_info} in the tests/resources/memory_keywords.robot diff and
update that literal).
- Updated `run_compose_command` to support separate build commands for services, including profile management for backend and speaker-recognition services. - Improved error handling and output streaming during Docker command execution. - Added `ensure_docker_network` function to verify and create the required Docker network before starting services. - Refactored configuration files to utilize `oc.env` for environment variable management, ensuring better compatibility and flexibility across different environments.
|
| Metric | Count |
|---|---|
| ✅ Passed | 72 |
| ❌ Failed | 20 |
| 📊 Total | 92 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-no-api - HTML reports
- robot-test-results-xml-no-api - XML output
- Added support for the CONFIG_FILE environment variable to allow specifying custom configuration files for testing. - Implemented logic to handle both absolute paths and relative filenames for the configuration file, improving flexibility in configuration management.
- Replaced CONFIG_FILE with TEST_CONFIG_FILE in both run-no-api-tests.sh and run-robot-tests.sh to standardize configuration file usage. - Updated paths to point to mock and deepgram-openai configuration files inside the container, improving clarity and consistency in test setups.
|
| Metric | Count |
|---|---|
| ✅ Passed | 59 |
| ❌ Failed | 33 |
| 📊 Total | 92 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-no-api - HTML reports
- robot-test-results-xml-no-api - XML output
|
| Metric | Count |
|---|---|
| ✅ Passed | 72 |
| ❌ Failed | 20 |
| 📊 Total | 92 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-no-api - HTML reports
- robot-test-results-xml-no-api - XML output
- Updated `upload_and_process_audio_files` to return appropriate HTTP status codes based on upload results: 400 for all failures, 207 for partial successes, and 200 for complete success. - Enhanced error messages in the audio upload tests to provide clearer feedback on upload failures, including specific error details for better debugging. - Adjusted test scripts to ensure consistent handling of conversation IDs in job metadata, improving validation checks for job creation.
|
| Metric | Count |
|---|---|
| ✅ Passed | 76 |
| ❌ Failed | 16 |
| 📊 Total | 92 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-no-api - HTML reports
- robot-test-results-xml-no-api - XML output
…anagement - Updated `upload_and_process_audio_files` to check for transcription provider availability before enqueueing jobs, enhancing error handling and logging. - Modified `start_post_conversation_jobs` to conditionally enqueue memory extraction jobs based on configuration, improving flexibility in job management. - Enhanced event dispatch job dependencies to only include jobs that were actually enqueued, ensuring accurate job tracking. - Added `is_transcription_available` function to check transcription provider status, improving modularity and clarity in the transcription workflow.
|
| Metric | Count |
|---|---|
| ✅ Passed | 77 |
| ❌ Failed | 15 |
| 📊 Total | 92 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-no-api - HTML reports
- robot-test-results-xml-no-api - XML output
- Updated integration tests to filter plugin events by conversation ID, ensuring accurate event tracking and reducing noise from fixture events. - Improved error messages in event verification to include conversation ID context, enhancing clarity during test failures. - Refactored audio upload handling to check for transcription job creation, allowing for more robust conversation polling and error reporting. - Added new keyword to verify conversation end reasons, improving test coverage for conversation state validation.
|
| Metric | Count |
|---|---|
| ✅ Passed | 80 |
| ❌ Failed | 12 |
| 📊 Total | 92 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-no-api - HTML reports
- robot-test-results-xml-no-api - XML output
|
| Metric | Count |
|---|---|
| ✅ Passed | 114 |
| ❌ Failed | 6 |
| 📊 Total | 120 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-pr-labeled - HTML reports
- robot-test-results-xml-pr-labeled - XML output
- Added mock speaker recognition client to facilitate testing without resource-intensive dependencies. - Updated Docker Compose configurations to include mock speaker client for test environments. - Refactored audio segment reconstruction to ensure precise clipping based on time boundaries. - Improved error handling in transcription jobs and speaker recognition workflows to enhance robustness. - Adjusted integration tests to utilize real-time pacing for audio chunk streaming, improving test accuracy.
- Introduced logging for audio chunk requests to improve traceability. - Replaced manual audio chunk processing with a dedicated `reconstruct_audio_segment` function for better clarity and efficiency. - Improved error handling during audio reconstruction to provide more informative responses in case of failures. - Cleaned up imports and removed redundant code related to audio chunk calculations.
- Replaced direct import of mock client with a structured import from the new testing module. - Introduced a dedicated `mock_speaker_client.py` to provide a mock implementation for speaker recognition, facilitating testing without heavy dependencies. - Added an `__init__.py` file in the testing directory to organize testing utilities and mocks.
|
| Metric | Count |
|---|---|
| ✅ Passed | 80 |
| ❌ Failed | 14 |
| 📊 Total | 94 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-no-api - HTML reports
- robot-test-results-xml-no-api - XML output
|
| Metric | Count |
|---|---|
| ✅ Passed | 80 |
| ❌ Failed | 14 |
| 📊 Total | 94 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-no-api - HTML reports
- robot-test-results-xml-no-api - XML output
|
| Metric | Count |
|---|---|
| ✅ Passed | 112 |
| ❌ Failed | 10 |
| 📊 Total | 122 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-pr-labeled - HTML reports
- robot-test-results-xml-pr-labeled - XML output
|
| Metric | Count |
|---|---|
| ✅ Passed | 114 |
| ❌ Failed | 8 |
| 📊 Total | 122 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-pr-labeled - HTML reports
- robot-test-results-xml-pr-labeled - XML output
…ve transcript handling - Added a new `words` field to the `Conversation` model for storing word-level timestamps. - Updated methods to handle word data during transcript version creation, ensuring compatibility with speaker recognition. - Refactored conversation job processing to utilize the new word structure, improving data integrity and access. - Enhanced speaker recognition job to read words from the new standardized location, ensuring backward compatibility with legacy data.
|
| Metric | Count |
|---|---|
| ✅ Passed | 80 |
| ❌ Failed | 12 |
| 📊 Total | 92 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-no-api - HTML reports
- robot-test-results-xml-no-api - XML output
|
| Metric | Count |
|---|---|
| ✅ Passed | 113 |
| ❌ Failed | 7 |
| 📊 Total | 120 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-pr-labeled - HTML reports
- robot-test-results-xml-pr-labeled - XML output
- Added a new endpoint to reprocess speaker identification for existing transcripts, creating a new version with re-identified speakers. - Introduced a method to calculate proportional timeouts based on audio duration, improving handling of varying audio lengths. - Updated the speaker recognition client to utilize calculated timeouts during service calls, enhancing responsiveness. - Refactored conversation and memory controllers to support the new speaker reprocessing functionality, ensuring user access control and job chaining for memory updates. - Removed unfiltered memory retrieval endpoint to streamline memory management and focus on user-specific data access.
|
| Metric | Count |
|---|---|
| ✅ Passed | 80 |
| ❌ Failed | 12 |
| 📊 Total | 92 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-no-api - HTML reports
- robot-test-results-xml-no-api - XML output
|
| Metric | Count |
|---|---|
| ✅ Passed | 114 |
| ❌ Failed | 6 |
| 📊 Total | 120 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html-pr-labeled - HTML reports
- robot-test-results-xml-pr-labeled - XML output
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.