Merged
Conversation
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: Roman Shemet <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates Cactus telemetry and cloud-handoff behavior across the CLI and C++ core, adding new knobs for framework metadata, cache location, and forcing cloud handoff.
Changes:
- Add
--force-handoffto the Pythontranscribecommand (viaCACTUS_FORCE_HANDOFF) and default-disable cloud telemetry in tests unless explicitly enabled. - Extend telemetry to include framework name/version and allow setting cache location + cloud key at runtime.
- Enhance streaming cloud handoff to capture an
api_key_hashfrom the cloud response and forward it into telemetry.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| python/src/cli.py | Adds --force-handoff env flag for transcribe; flips test telemetry behavior to opt-in (--enable-telemetry). |
| cactus/telemetry/telemetry.h | Exposes new telemetry setters for environment and cloud key. |
| cactus/telemetry/telemetry.cpp | Adds framework/version fields, custom cache location support, and setters for environment/cloud key. |
| cactus/ffi/cactus_transcribe.cpp | Reads CACTUS_FORCE_HANDOFF to override the handoff threshold. |
| cactus/ffi/cactus_stream.cpp | Introduces CloudResponse (transcript + api key hash) and wires hash into telemetry. |
| cactus/ffi/cactus_init.cpp | Adds exported FFI function to set telemetry environment parameters. |
| cactus/ffi/cactus_ffi.h | Declares the new FFI API for telemetry environment configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+849
to
+850
| else: | ||
| os.environ.pop("CACTUS_FORCE_HANDOFF", None) |
There was a problem hiding this comment.
--force-handoff sets CACTUS_FORCE_HANDOFF in the current process, but the Android transcription path builds an explicit list of exported env vars for the adb shell command and does not include CACTUS_FORCE_HANDOFF. As a result, --force-handoff will work for local/iOS runs but not for --android. Consider adding CACTUS_FORCE_HANDOFF to the exported env list for the Android shell command (or passing the full environment through consistently).
Suggested change
| else: | |
| os.environ.pop("CACTUS_FORCE_HANDOFF", None) |
Comment on lines
350
to
356
| struct CloudJob { | ||
| uint64_t id; | ||
| std::future<std::string> result; | ||
| std::future<CloudResponse> result; | ||
| }; | ||
| std::vector<CloudJob> pending_cloud_jobs; | ||
| std::vector<std::pair<uint64_t, std::string>> completed_cloud_results; | ||
| std::vector<std::pair<uint64_t, CloudResponse>> completed_cloud_results; | ||
|
|
There was a problem hiding this comment.
CloudResponse is defined inside #ifdef CACTUS_USE_CURL, but it is used unconditionally in CactusStreamTranscribeHandle (std::future<CloudResponse> and completed_cloud_results) and in build_stream_response. When CACTUS_USE_CURL is not defined, this will not compile. Move the CloudResponse type definition outside the #ifdef, or provide a non-cURL fallback definition under #else so non-cURL builds still compile.
Comment on lines
+921
to
+929
| void setTelemetryEnvironment(const char* framework_str, const char* cache_location_str) { | ||
| if (framework_str && framework_str[0] != '\0') { | ||
| framework = framework_str; | ||
| } | ||
|
|
||
| if (cache_location_str && cache_location_str[0] != '\0') { | ||
| custom_cache_location = cache_location_str; | ||
| } | ||
| } |
There was a problem hiding this comment.
setTelemetryEnvironment mutates global std::string state (framework, custom_cache_location) that is read from other telemetry paths (e.g., get_telemetry_dir() / payload construction) without any synchronization. If this API can be called after telemetry has started (or concurrently with recording/flushing), it introduces a C++ data race (undefined behavior). Consider guarding these globals with a mutex, or document/enforce that this must be called before telemetry::init() and before any other telemetry calls.
Comment on lines
+931
to
+935
| void setCloudKey(const char* key) { | ||
| if (key && key[0] != '\0') { | ||
| cloud_key = key; | ||
| } | ||
| } |
There was a problem hiding this comment.
setCloudKey assigns to the global cloud_key std::string without synchronization, while send_batch_to_cloud() reads cloud_key when building the payload. If setCloudKey can run concurrently with telemetry recording/flushing, this is a data race (undefined behavior). Protect access with a mutex (or switch to an atomic/immutable snapshot approach), or restrict/ensure calls happen before any telemetry work begins.
Comment on lines
99
to
+103
| static std::string get_telemetry_dir() { | ||
| if (!custom_cache_location.empty()) { | ||
| mkdir(custom_cache_location.c_str(), 0755); | ||
| return custom_cache_location; | ||
| } |
There was a problem hiding this comment.
In the custom cache path case, mkdir(custom_cache_location.c_str(), 0755); ignores the return value and does not create parent directories. If the directory cannot be created (missing parents/permissions), telemetry caching will silently fail and later file IO may break. Consider creating parent directories (like the default path does) and handling/logging mkdir errors.
Comment on lines
+128
to
+131
| const char* force_handoff_env = std::getenv("CACTUS_FORCE_HANDOFF"); | ||
| if (force_handoff_env && std::string(force_handoff_env) == "1") { | ||
| cloud_handoff_threshold = 0.0001f; | ||
| } |
There was a problem hiding this comment.
This runs getenv() and constructs a temporary std::string on every cactus_transcribe() call. In streaming mode this function is called frequently per chunk, so this adds avoidable overhead. Consider checking the env var without allocating (e.g., compare force_handoff_env[0] == '1' && force_handoff_env[1] == '\0') and/or caching the parsed flag once.
Member
Author
There was a problem hiding this comment.
Good catch, changing now.
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: justinl66 <[email protected]>
… scripts Signed-off-by: jakmro <[email protected]>
Signed-off-by: justinl66 <[email protected]>
Signed-off-by: jakmro <[email protected]>
ncylich
pushed a commit
that referenced
this pull request
Feb 24, 2026
* Add basic telemetry setup Signed-off-by: justinl66 <[email protected]> * Add init error logging Signed-off-by: justinl66 <[email protected]> * Update device + project ID Signed-off-by: justinl66 <[email protected]> * Align telemetry POSTs to supabase schema Signed-off-by: justinl66 <[email protected]> * Fixing 35-char UUID format bug to make it the correct 36 Signed-off-by: Roman Shemet <[email protected]> * Update telemetry Signed-off-by: justinl66 <[email protected]> * Fix transcription telemetry event Signed-off-by: justinl66 <[email protected]> * Update telemetry to work with new RLS policy Signed-off-by: justinl66 <[email protected]> * Update response time logging Signed-off-by: justinl66 <[email protected]> * Fix transcribe events Signed-off-by: justinl66 <[email protected]> * Fix init error logging Signed-off-by: justinl66 <[email protected]> * Fix token double count during stream transcription Signed-off-by: justinl66 <[email protected]> * Update telemetry with embedding logs Signed-off-by: justinl66 <[email protected]> * Fix streaming transcription telemetry Signed-off-by: justinl66 <[email protected]> * Remove stray third_party folder Signed-off-by: justinl66 <[email protected]> * Remove duplicate telemetry recording, Point to main DB Signed-off-by: justinl66 <[email protected]> * Remove stray third party repos Signed-off-by: justinl66 <[email protected]> * Remove unneeded telemetry Signed-off-by: justinl66 <[email protected]> * Add proper NULL logging Signed-off-by: justinl66 <[email protected]> * Add init NULL logging Signed-off-by: justinl66 <[email protected]> * Remove dropped columns from payload Signed-off-by: justinl66 <[email protected]> * Update json parsing and fix session metrics Signed-off-by: justinl66 <[email protected]> * Make cactus clean clear cactus cache folder Signed-off-by: justinl66 <[email protected]> * Add version logging to telemetry Signed-off-by: justinl66 <[email protected]> * Remove id from projects payload for telemetry Signed-off-by: justinl66 <[email protected]> * Add set telemetry environment in FFI Signed-off-by: justinl66 <[email protected]> * Parse cloud handoff response and log Signed-off-by: justinl66 <[email protected]> * Update cloud key logging Signed-off-by: justinl66 <[email protected]> * Add --enable-telemetry flag for cactus tests Signed-off-by: justinl66 <[email protected]> * Add force handoff flag to CLI Signed-off-by: justinl66 <[email protected]> * Remove unneeded third party repos Signed-off-by: justinl66 <[email protected]> * Update string overhead for transcribe Signed-off-by: justinl66 <[email protected]> * Add force handoff to android flags Signed-off-by: justinl66 <[email protected]> * Fix CloudResponse ifdef issue & mkdir error handling Signed-off-by: justinl66 <[email protected]> * Remove unneeded third party repos Signed-off-by: justinl66 <[email protected]> * Write to key_hash column instead of cloud_key Signed-off-by: justinl66 <[email protected]> * Remove unneeded third party repos Signed-off-by: justinl66 <[email protected]> * Add CACTUS_NO_CLOUD_TELE environment variable to Android and iOS test scripts Signed-off-by: jakmro <[email protected]> * Fix JSON parsing for telemetry logs Signed-off-by: justinl66 <[email protected]> * update sdks Signed-off-by: jakmro <[email protected]> --------- Signed-off-by: justinl66 <[email protected]> Signed-off-by: Roman Shemet <[email protected]> Signed-off-by: jakmro <[email protected]> Co-authored-by: Roman Shemet <[email protected]> Co-authored-by: jakmro <[email protected]>
cattermelon1234
pushed a commit
to cattermelon1234/cactus
that referenced
this pull request
Feb 28, 2026
* Add basic telemetry setup Signed-off-by: justinl66 <[email protected]> * Add init error logging Signed-off-by: justinl66 <[email protected]> * Update device + project ID Signed-off-by: justinl66 <[email protected]> * Align telemetry POSTs to supabase schema Signed-off-by: justinl66 <[email protected]> * Fixing 35-char UUID format bug to make it the correct 36 Signed-off-by: Roman Shemet <[email protected]> * Update telemetry Signed-off-by: justinl66 <[email protected]> * Fix transcription telemetry event Signed-off-by: justinl66 <[email protected]> * Update telemetry to work with new RLS policy Signed-off-by: justinl66 <[email protected]> * Update response time logging Signed-off-by: justinl66 <[email protected]> * Fix transcribe events Signed-off-by: justinl66 <[email protected]> * Fix init error logging Signed-off-by: justinl66 <[email protected]> * Fix token double count during stream transcription Signed-off-by: justinl66 <[email protected]> * Update telemetry with embedding logs Signed-off-by: justinl66 <[email protected]> * Fix streaming transcription telemetry Signed-off-by: justinl66 <[email protected]> * Remove stray third_party folder Signed-off-by: justinl66 <[email protected]> * Remove duplicate telemetry recording, Point to main DB Signed-off-by: justinl66 <[email protected]> * Remove stray third party repos Signed-off-by: justinl66 <[email protected]> * Remove unneeded telemetry Signed-off-by: justinl66 <[email protected]> * Add proper NULL logging Signed-off-by: justinl66 <[email protected]> * Add init NULL logging Signed-off-by: justinl66 <[email protected]> * Remove dropped columns from payload Signed-off-by: justinl66 <[email protected]> * Update json parsing and fix session metrics Signed-off-by: justinl66 <[email protected]> * Make cactus clean clear cactus cache folder Signed-off-by: justinl66 <[email protected]> * Add version logging to telemetry Signed-off-by: justinl66 <[email protected]> * Remove id from projects payload for telemetry Signed-off-by: justinl66 <[email protected]> * Add set telemetry environment in FFI Signed-off-by: justinl66 <[email protected]> * Parse cloud handoff response and log Signed-off-by: justinl66 <[email protected]> * Update cloud key logging Signed-off-by: justinl66 <[email protected]> * Add --enable-telemetry flag for cactus tests Signed-off-by: justinl66 <[email protected]> * Add force handoff flag to CLI Signed-off-by: justinl66 <[email protected]> * Remove unneeded third party repos Signed-off-by: justinl66 <[email protected]> * Update string overhead for transcribe Signed-off-by: justinl66 <[email protected]> * Add force handoff to android flags Signed-off-by: justinl66 <[email protected]> * Fix CloudResponse ifdef issue & mkdir error handling Signed-off-by: justinl66 <[email protected]> * Remove unneeded third party repos Signed-off-by: justinl66 <[email protected]> * Write to key_hash column instead of cloud_key Signed-off-by: justinl66 <[email protected]> * Remove unneeded third party repos Signed-off-by: justinl66 <[email protected]> * Add CACTUS_NO_CLOUD_TELE environment variable to Android and iOS test scripts Signed-off-by: jakmro <[email protected]> * Fix JSON parsing for telemetry logs Signed-off-by: justinl66 <[email protected]> * update sdks Signed-off-by: jakmro <[email protected]> --------- Signed-off-by: justinl66 <[email protected]> Signed-off-by: Roman Shemet <[email protected]> Signed-off-by: jakmro <[email protected]> Co-authored-by: Roman Shemet <[email protected]> Co-authored-by: jakmro <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.