ROB-3618: Add AI usage tracking via HolmesUsageEvents - #1969
Merged
moshemorad merged 32 commits intoMay 11, 2026
Merged
Conversation
…ture/cost) Wires every LLM-consuming entry point through a shared usage recorder so each request lands as one row in the new HolmesUsageEvents Supabase table. Enables per-account / per-cluster / per-user / per-feature cost reporting, plus thumbs-up/down feedback on individual chat turns. Why: per requirements doc (Hypatos / Adobe Firefly / Playtika), customers need to see "cost per alert investigation", "questions asked this week", "who used Holmes from Teams vs UI", "thumbs-up rate by feature" — none of which are answerable today because Holmes persists no AI metrics. What's covered: POST /api/chat (direct + worker path), scheduled prompts, POST /api/agui/chat, POST /api/checks/execute. CLI flows are intentionally out of scope (no FastAPI server -> no recorder hook). How it works: - New holmes/core/usage_recorder.py with UsageRecorderState dataclass + stream_with_usage_recording (streaming) + record_from_llm_result (non-streaming) + record_error. Every entry point builds one UsageRecorderState and either wraps its stream or calls record_from_llm_result. The recorder fires fire-and-forget on a daemon thread so telemetry never blocks the response. - SupabaseDal.record_usage_event / record_feedback write through the existing self.client (same email+password user-JWT pattern as upsert_holmes_status). No new env vars, no service_role key. RLS mirrors HolmesStatus's policy. - ChatRequest gains 6 optional fields (request_type, request_source, source_ref, conversation_id, conversation_source, meta). All additive / backwards-compatible — old clients keep working. - LLMResult gains a finish_reason field, captured from the last LLM iteration's response.choices[0].finish_reason. - POST /api/feedback endpoint accepts request_id + sentiment + optional category/comment, UPDATEs the matching event row. Schema (separately applied to Supabase): HolmesUsageEvents covers account_id (uuid), cluster_id, user_id (uuid), conversation_id + conversation_source discriminator, request_type, request_source, source_ref, model, provider, is_robusta_model, full RequestStats fields, iterations, tool_call_count, duration_ms, is_streaming, finish_reason, feedback_* columns, and a meta JSONB for forward-compat. Optional HolmesUsageEventCalls per-LLM-call child table is gated by HOLMES_RECORD_LLM_CALLS=true (not enabled in v1). Tests: - tests/core/test_usage_recorder.py — 15 tests covering streaming / non-streaming / error / approval paths, disabled-DAL no-op, and fire-and-forget thread mode. - tests/core/test_supabase_dal_usage.py — 11 tests covering payload shape, account-scoped UPDATE, NULL handling, error swallowing. - tests/test_tool_calling_llm.py — all 41 existing tests still pass (one regression I caught and fixed: finish_reason capture now guards with isinstance(str) so MagicMock'd test responses don't break pydantic validation). Plan: ~/.claude/plans/does-holmes-save-any-sunny-globe.md Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
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:
WalkthroughAdds a Holmes usage-recording subsystem and integrates it across LLM call sites: new UsageRecorderState, streaming and non-streaming recording helpers, Supabase persistence for usage events and feedback, ChatRequest metadata fields, and wiring into server endpoints, checks/health, workers, scheduled prompts, and an experimental AG-UI. ChangesHolmes usage recording (single cohort)
Sequence DiagramsequenceDiagram
participant Client
participant Server as /api/chat
participant Recorder as stream_with_usage_recording
participant LLM as LLM Stream
participant State as UsageRecorderState
participant DAL as SupabaseDal
participant DB as Supabase
Client->>Server: POST /api/chat (streaming)
Server->>State: build UsageRecorderState(model, user_id, request_type, conversation_id...)
Server->>Recorder: wrap(LLM stream, State)
Recorder->>LLM: iterate stream
loop stream events
LLM-->>Recorder: event (CHUNK / TOOL_CALL / TOOL_RESULT)
Recorder->>State: update stats/tool_call_count/iterations
Recorder-->>Client: forward event
end
LLM-->>Recorder: terminal event (ANSWER_END / APPROVAL_REQUIRED / ERROR)
Recorder->>State: set status, finish_reason, inject request_id into event.metadata
Recorder->>DAL: _fire(State) [background thread]
DAL->>DB: INSERT/UPDATE HolmesUsageEvents
DB-->>DAL: result/exception
Recorder-->>Client: stream end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
❌ Deploy Preview for holmes-docs failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
server.py (1)
673-681: Misleading variable name and silent exception swallowing.Two issues:
- The variable
body_user_idis actually read fromquery_params, not the request body—the name is misleading.- The
try-except-passsilently swallows all exceptions. While this is best-effort, logging at debug level would aid troubleshooting without breaking the flow.♻️ Proposed fix
- user_id: Optional[str] = None - # Mirror /api/chat's user_id resolution path: prefer header passthrough - # (Robusta relay token) but accept a body field if the caller set one. - try: - body_user_id = http_request.query_params.get("user_id") - if body_user_id: - user_id = body_user_id - except Exception: - pass + # Mirror /api/chat's user_id resolution: check query param (e.g. Robusta relay). + user_id: Optional[str] = http_request.query_params.get("user_id")Note:
query_params.get()already returnsNoneon missing keys and doesn't raise, so the try/except is unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 673 - 681, The code uses a misleading variable name and silently swallows exceptions: rename the local variable currently called body_user_id to something accurate like query_user_id (or query_param_user_id) to reflect that it comes from http_request.query_params, remove the unnecessary try/except around http_request.query_params.get("user_id") since .get() won't raise, and if you must keep error handling, replace the bare except/pass with a debug-level log via the existing logger (e.g., process_logger.debug or similar) including the exception details; update the assignment to set user_id = query_user_id when present and ensure no silent failures occur.holmes/core/usage_recorder.py (1)
212-217: Consider sorting__all__for consistency.Static analysis flags that
__all__is not sorted. This is a minor style nit.🧹 Sorted __all__
__all__ = [ "UsageRecorderState", + "record_error", + "record_from_llm_result", "stream_with_usage_recording", - "record_from_llm_result", - "record_error", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/usage_recorder.py` around lines 212 - 217, The module-level __all__ list is not sorted; reorder the __all__ assignment in holmes/core/usage_recorder.py to be alphabetically sorted (case-insensitive) for consistency and to satisfy static analysis—update the __all__ containing "UsageRecorderState", "stream_with_usage_recording", "record_from_llm_result", "record_error" so the entries are sorted (e.g., "record_error", "record_from_llm_result", "stream_with_usage_recording", "UsageRecorderState").experimental/ag-ui/server-agui.py (1)
151-154: Avoid silentexcept ...: passfor user attribution extraction.If
input_datashape changes, this currently dropsuser_idsilently and degrades telemetry quality without any signal. Log at least debug/warning and narrow the caught exception type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/server-agui.py` around lines 151 - 154, The current silent except around the agui_user_id extraction hides errors; change the try/except around the getattr(input_data, "user_id", None) or ctx.get("user_id") expression to only catch specific errors (e.g., AttributeError and TypeError), and emit a debug or warning log that includes the exception details and context (input_data repr and ctx) so telemetry degradation is visible; keep the fallback behavior (None) but do not swallow unexpected exceptions silently—use the module logger (or processLogger) to log the message and exception for the agui_user_id extraction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/checks/checks_api.py`:
- Line 25: Move the inline "import litellm" into module scope alongside the
existing "from litellm.exceptions import AuthenticationError" import so all
litellm imports are consistent; then remove the redundant inline import from the
function where it's currently declared (the function that currently references
AuthenticationError), leaving only the module-level "import litellm" and the
existing "from litellm.exceptions import AuthenticationError".
In `@holmes/core/supabase_dal.py`:
- Around line 925-927: The UPDATE in record_feedback currently only filters by
account_id and request_id, allowing cross-user overwrites; modify the query in
record_feedback (the .eq(...).eq("request_id", request_id).execute() chain) to
also include a filter for the passed user_id (e.g., add .eq("user_id", user_id))
so the UPDATE targets the specific user's feedback record within the account
before executing.
---
Nitpick comments:
In `@experimental/ag-ui/server-agui.py`:
- Around line 151-154: The current silent except around the agui_user_id
extraction hides errors; change the try/except around the getattr(input_data,
"user_id", None) or ctx.get("user_id") expression to only catch specific errors
(e.g., AttributeError and TypeError), and emit a debug or warning log that
includes the exception details and context (input_data repr and ctx) so
telemetry degradation is visible; keep the fallback behavior (None) but do not
swallow unexpected exceptions silently—use the module logger (or processLogger)
to log the message and exception for the agui_user_id extraction.
In `@holmes/core/usage_recorder.py`:
- Around line 212-217: The module-level __all__ list is not sorted; reorder the
__all__ assignment in holmes/core/usage_recorder.py to be alphabetically sorted
(case-insensitive) for consistency and to satisfy static analysis—update the
__all__ containing "UsageRecorderState", "stream_with_usage_recording",
"record_from_llm_result", "record_error" so the entries are sorted (e.g.,
"record_error", "record_from_llm_result", "stream_with_usage_recording",
"UsageRecorderState").
In `@server.py`:
- Around line 673-681: The code uses a misleading variable name and silently
swallows exceptions: rename the local variable currently called body_user_id to
something accurate like query_user_id (or query_param_user_id) to reflect that
it comes from http_request.query_params, remove the unnecessary try/except
around http_request.query_params.get("user_id") since .get() won't raise, and if
you must keep error handling, replace the bare except/pass with a debug-level
log via the existing logger (e.g., process_logger.debug or similar) including
the exception details; update the assignment to set user_id = query_user_id when
present and ensure no silent failures occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39b64311-a04f-47ba-a923-d7ef4c077bbb
📒 Files selected for processing (12)
experimental/ag-ui/server-agui.pyholmes/checks/checks.pyholmes/checks/checks_api.pyholmes/core/conversations_worker/worker.pyholmes/core/models.pyholmes/core/scheduled_prompts/executor.pyholmes/core/supabase_dal.pyholmes/core/tool_calling_llm.pyholmes/core/usage_recorder.pyserver.pytests/core/test_supabase_dal_usage.pytests/core/test_usage_recorder.py
The FE needs the chat-call's request_id to send POST /api/feedback
{request_id, sentiment} when the user clicks thumbs up/down. Previously
the recorder generated request_id internally but never sent it back.
Streaming path: stream_with_usage_recording now injects state.request_id
into the terminal event's metadata dict before yielding it. The SSE
formatter then ships it as part of metadata in ai_answer_end (and
approval_required / error). Same treatment for all three terminals so
feedback works on paused / errored chats too.
Non-streaming path: chat() copies llm_call.metadata, drops in
recorder_state.request_id, and uses the merged dict as
ChatResponse.metadata. Same shape on the wire as the streaming case.
Tests: 4 new cases in test_usage_recorder.py covering injection into
ANSWER_END / APPROVAL_REQUIRED / ERROR plus the missing-metadata case
where _inject_request_id has to create the dict. All 71 tests
(usage_recorder + supabase_dal_usage + tool_calling_llm) pass.
FE contract: read response.metadata.request_id from ai_answer_end (or
the JSON body for non-stream); save it; POST it back to /api/feedback.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: alonelish <alon.elish@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
holmes/core/usage_recorder.py (2)
158-158: 💤 Low valuePotential issue with falsy
num_llm_callsvalue.The expression
data.get("num_llm_calls", state.iterations) or state.iterationswill fall back tostate.iterationsifnum_llm_callsis0(falsy), even though 0 iterations might be a legitimate value. In practice, 0 iterations is unlikely, but this pattern could mask edge cases.Consider using an explicit
Nonecheck if 0 should be preserved:raw = data.get("num_llm_calls") state.iterations = raw if raw is not None else state.iterations🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/usage_recorder.py` at line 158, The current assignment uses a falsy-or fallback which will overwrite a legitimate 0 value: replace the expression that sets state.iterations (the line using data.get("num_llm_calls", state.iterations) or state.iterations) with an explicit None check: retrieve raw = data.get("num_llm_calls") and assign state.iterations = raw if raw is not None else state.iterations so that 0 is preserved while still falling back when the key is absent or None.
231-236: 💤 Low valueMinor:
__all__is not sorted (RUF022).Static analysis flags that
__all__entries are not alphabetically sorted. The current order (dataclass first, then helpers) seems intentional and logical, so this is purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/usage_recorder.py` around lines 231 - 236, The __all__ list in usage_recorder.py is not alphabetically sorted; update the module-level __all__ (which currently lists "UsageRecorderState", "stream_with_usage_recording", "record_from_llm_result", "record_error") so its entries are sorted alphabetically (e.g., "record_error", "record_from_llm_result", "stream_with_usage_recording", "UsageRecorderState") to satisfy RUF022; keep the same symbols and casing, only reorder the list.server.py (1)
681-686: ⚡ Quick winConsider logging the exception in the
try-except-passblock.The static analysis flags this as
try-except-pass(S110). While the intent is best-effortuser_idextraction, silently swallowing exceptions could hide bugs. Consider logging at debug level for diagnosability:try: body_user_id = http_request.query_params.get("user_id") if body_user_id: user_id = body_user_id except Exception: - pass + logging.debug("Failed to extract user_id from query params", exc_info=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 681 - 686, Replace the silent except in the body_user_id extraction with logging so exceptions aren’t swallowed: change the block around http_request.query_params.get("user_id") / body_user_id to catch the exception as e and call the existing logger at debug level (e.g., logger.debug("failed to read user_id from query_params: %s", e, exc_info=True)) before continuing; keep the best-effort behavior of assigning user_id when body_user_id is present and do not re-raise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@holmes/core/usage_recorder.py`:
- Line 158: The current assignment uses a falsy-or fallback which will overwrite
a legitimate 0 value: replace the expression that sets state.iterations (the
line using data.get("num_llm_calls", state.iterations) or state.iterations) with
an explicit None check: retrieve raw = data.get("num_llm_calls") and assign
state.iterations = raw if raw is not None else state.iterations so that 0 is
preserved while still falling back when the key is absent or None.
- Around line 231-236: The __all__ list in usage_recorder.py is not
alphabetically sorted; update the module-level __all__ (which currently lists
"UsageRecorderState", "stream_with_usage_recording", "record_from_llm_result",
"record_error") so its entries are sorted alphabetically (e.g., "record_error",
"record_from_llm_result", "stream_with_usage_recording", "UsageRecorderState")
to satisfy RUF022; keep the same symbols and casing, only reorder the list.
In `@server.py`:
- Around line 681-686: Replace the silent except in the body_user_id extraction
with logging so exceptions aren’t swallowed: change the block around
http_request.query_params.get("user_id") / body_user_id to catch the exception
as e and call the existing logger at debug level (e.g., logger.debug("failed to
read user_id from query_params: %s", e, exc_info=True)) before continuing; keep
the best-effort behavior of assigning user_id when body_user_id is present and
do not re-raise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bed73c48-c458-4834-8187-24f3e7c1f75d
📒 Files selected for processing (3)
holmes/core/usage_recorder.pyserver.pytests/core/test_usage_recorder.py
Server-internal calls (title generation, classification, summarization, follow-up suggestion ranking, etc.) currently use an 'internal_' prefix on request_source as a convention. That works but forces every dashboard to do `request_source NOT LIKE 'internal\_%'` and is easy to forget. Adds a dedicated boolean is_internal column. Dashboards default-filter with `WHERE NOT is_internal`. Index-friendly (low-cardinality boolean), self-documenting in schema, removes string-pattern coupling. Backwards-compatible: when chat_request.is_internal is unset, the server falls back to detecting the legacy 'internal_' prefix on request_source. Existing FE clients keep working unchanged. New FE code can set is_internal=true explicitly and stop relying on naming. Wiring: - ChatRequest gains optional is_internal: bool field - UsageRecorderState carries it; to_kwargs includes it - SupabaseDal.record_usage_event accepts and writes it - server._build_chat_recorder_state derives it (explicit > prefix > false) - ConversationWorker forwards is_internal from task.user_message_data - Scheduled prompts and health checks NOT marked internal — their output is user-facing (alert routing, scheduled-prompt results), so request_type='scheduled_prompt'/'health_check' is the right discriminator for those, not is_internal. Tests: - 2 new in test_usage_recorder.py for default-false / round-trip - New test_chat_recorder_state.py with 6 tests covering all 4 cases of the prefix-fallback derivation (explicit true / explicit false with prefix / unset with prefix / unset without prefix / unset without request_source) plus a smoke test on the rest of the wiring DB-side: ALTER TABLE HolmesUsageEvents ADD COLUMN is_internal boolean NOT NULL DEFAULT false (run separately by Alon). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
holmes/core/supabase_dal.py (1)
892-929:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope feedback updates by
user_id.
record_feedback()still matches only onaccount_id+request_id, so anyone who learns a request id can overwrite another user's feedback within the same account.Proposed fix
- self.client.table(HOLMES_USAGE_EVENTS_TABLE).update( - { - "feedback_sentiment": sentiment, - "feedback_category": category, - "feedback_comment": comment, - "feedback_at": datetime.now().isoformat(), - } - ).eq("account_id", self.account_id).eq( - "request_id", request_id - ).execute() + query = ( + self.client.table(HOLMES_USAGE_EVENTS_TABLE) + .update( + { + "feedback_sentiment": sentiment, + "feedback_category": category, + "feedback_comment": comment, + "feedback_at": datetime.now().isoformat(), + } + ) + .eq("account_id", self.account_id) + .eq("request_id", request_id) + ) + if user_id: + query = query.eq("user_id", user_id) + query.execute()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/supabase_dal.py` around lines 892 - 929, In record_feedback, scope the UPDATE by user_id to prevent cross-user overwrites: when calling self.client.table(...).update(...) before .execute(), add an .eq("user_id", user_id) predicate to the existing chain (the call that currently ends with .eq("account_id", self.account_id).eq("request_id", request_id).execute()); also validate that user_id is present (log a warning and return) if it's None so you don't run an account/request-level update without a user constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@holmes/core/supabase_dal.py`:
- Around line 892-929: In record_feedback, scope the UPDATE by user_id to
prevent cross-user overwrites: when calling self.client.table(...).update(...)
before .execute(), add an .eq("user_id", user_id) predicate to the existing
chain (the call that currently ends with .eq("account_id",
self.account_id).eq("request_id", request_id).execute()); also validate that
user_id is present (log a warning and return) if it's None so you don't run an
account/request-level update without a user constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f5ebe00-3171-460e-bfeb-b481d3322680
📒 Files selected for processing (7)
holmes/core/conversations_worker/worker.pyholmes/core/models.pyholmes/core/supabase_dal.pyholmes/core/usage_recorder.pyserver.pytests/core/test_usage_recorder.pytests/test_chat_recorder_state.py
🚧 Files skipped from review as they are similar to previous changes (2)
- holmes/core/conversations_worker/worker.py
- holmes/core/models.py
5 fixes from CodeRabbit review (3 reviews on commits 0cbf22c, c934138, 6c80253): - holmes/checks/checks_api.py: move `import litellm` to module scope per CLAUDE.md import rule (was inline inside _resolve_provider). - holmes/core/supabase_dal.py: scope record_feedback UPDATE by user_id when provided. Defense in depth — even though v1's auth model assumes rater == asker, adding `.eq("user_id", user_id)` when present prevents cross-user overwrites if a request_id ever leaks. Skip the filter when user_id is None to keep the path open for future system / scheduled flows. - server.py /api/feedback: drop the unnecessary try/except around http_request.query_params.get("user_id") (.get() doesn't raise), rename body_user_id → user_id directly, simplify to a single expression. - experimental/ag-ui/server-agui.py: narrow the user_id extraction except to AttributeError/TypeError and log at debug instead of silent pass. Adds an isinstance(ctx, dict) guard since ctx is opaque per the AG-UI protocol. - holmes/core/usage_recorder.py _capture_terminal: replace `data.get("num_llm_calls", state.iterations) or state.iterations` with an explicit None check so a legitimate 0 isn't silently overwritten. Edge case (the agentic loop always makes >=1 call) but cheap to fix. Not addressed (intentional): - __all__ in usage_recorder.py is not alphabetically sorted (RUF022). Current order is intentional (public class first, then helpers grouped by usage frequency: streaming most common, then non-streaming, then error path). CodeRabbit acknowledged this is "low value" / cosmetic. All 79 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
Collaborator
Author
|
Walked through all three CodeRabbit reviews — addressed in commit 4567656. Summary: Fixed (5)
Not addressed (intentional)
That groups them by usage frequency / typical reading order, which I find easier to scan than alphabetical. Happy to flip if maintainers prefer the lint-clean version — I'll add a All 79 tests still pass. Plan file lives at |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/core/supabase_dal.py`:
- Around line 907-910: The docstring is stale: update the text around the
parameter `user_id` (the two docstrings currently claiming it is accepted but
not written) to state that when provided it is used to scope/limit updates via
the `.eq("user_id", user_id)` filter so only rows belonging to that user are
affected; remove the "not currently written" wording and clarify the exact
scoping behavior and any semantics (e.g., optional parameter, when omitted no
user filter is applied) for both occurrences referencing `user_id` and the
`.eq("user_id", user_id)` filter.
In `@holmes/core/usage_recorder.py`:
- Around line 221-231: The _fire function currently spawns an unbounded daemon
thread per event which can exhaust resources; modify _fire and the surrounding
UsageRecorderState to use a bounded worker pool instead (e.g., a shared
ThreadPoolExecutor or a small fixed-size worker queue) so calls to
state.dal.record_usage_event are submitted to the executor rather than starting
a new Thread; ensure the executor is created once (lazily or on recorder init),
shut down cleanly on teardown, and keep state.to_kwargs() usage when submitting
tasks so behavior is unchanged while preventing unbounded thread growth.
- Around line 108-135: The stream can end without a terminal event so
state.status remains the default "success" and gets fired incorrectly; update
the finalization logic to detect non-terminal termination and set an appropriate
status (e.g., "error" or "incomplete") before calling _fire(state). Concretely,
keep the saw_terminal flag used in the for msg in stream loop and in the finally
block check if not saw_terminal (and/or state.status == "success") then set
state.status = "error" (or "incomplete") so only true terminal events (handled
in StreamEvents.ANSWER_END, APPREVAL_REQUIRED, ERROR branches) leave status as
their intended value prior to calling _fire(state).
In `@server.py`:
- Around line 694-701: Require and validate user_id before calling
dal.record_feedback: change the extraction from
http_request.query_params.get("user_id") to explicitly check presence (e.g.,
user_id = http_request.query_params.get("user_id"); if not user_id: return a
400/Bad Request with a clear message), and only then call dal.record_feedback
with user_id (preserving use of req.request_id, req.sentiment, req.category,
req.comment). This makes /api/feedback reject requests missing user_id instead
of falling back to account+request_id behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6eeaed19-71ff-46eb-b323-5ab090bcb8f4
📒 Files selected for processing (5)
experimental/ag-ui/server-agui.pyholmes/checks/checks_api.pyholmes/core/supabase_dal.pyholmes/core/usage_recorder.pyserver.py
alonelish
changed the title
…mcnulty-822c30 Signed-off-by: alonelish <alon.elish@gmail.com> # Conflicts: # holmes/core/scheduled_prompts/executor.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server.py (1)
699-717:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire
user_idhere instead of silently acceptingNone.
SupabaseDal.record_feedback()only applies the per-user predicate whenuser_idis present. Leaving this optional reintroduces the same-account overwrite gap for any caller that knows arequest_id. Since this is a user-facing endpoint, reject missinguser_idwith a 400 and keep the DAL fallback for non-HTTP callers only if they truly need it.Proposed fix
`@app.post`("/api/feedback") def feedback(req: FeedbackRequest, http_request: Request) -> dict: @@ # Mirror /api/chat's user_id resolution: pull from the request's query # params (e.g. when posted by the Robusta relay). `.get()` returns None # for missing keys without raising, so no try/except needed. user_id: Optional[str] = http_request.query_params.get("user_id") + if not user_id: + raise HTTPException(status_code=400, detail="missing user_id") dal.record_feedback( request_id=req.request_id, sentiment=req.sentiment, category=req.category, comment=req.comment,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 699 - 717, The feedback endpoint (function feedback) currently allows user_id to be None which bypasses SupabaseDal.record_feedback's per-user predicate; change the endpoint to validate that http_request.query_params.get("user_id") is present and return HTTP 400 (bad request) if missing, before calling dal.record_feedback, so only callers that supply a user_id can update feedback via /api/feedback while keeping the DAL behavior unchanged for non-HTTP callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server.py`:
- Around line 699-717: The feedback endpoint (function feedback) currently
allows user_id to be None which bypasses SupabaseDal.record_feedback's per-user
predicate; change the endpoint to validate that
http_request.query_params.get("user_id") is present and return HTTP 400 (bad
request) if missing, before calling dal.record_feedback, so only callers that
supply a user_id can update feedback via /api/feedback while keeping the DAL
behavior unchanged for non-HTTP callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 188af96c-be0d-430b-8bf8-49abc363ddaa
📒 Files selected for processing (3)
holmes/core/conversations_worker/worker.pyholmes/core/scheduled_prompts/executor.pyserver.py
✅ Files skipped from review due to trivial changes (1)
- holmes/core/scheduled_prompts/executor.py
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/core/conversations_worker/worker.py
3 fixes:
- usage_recorder: stream_with_usage_recording now downgrades status
from default 'success' to 'aborted' in the finally block when no
terminal event was seen (e.g. client disconnect mid-stream). Without
this the row was recorded as success even though the chat never
completed. CodeRabbit catch.
- supabase_dal: record_feedback docstring was stale ("user_id is
accepted but not currently written") — updated to describe the
defense-in-depth user_id scoping that was added in 4567656.
- server.py /api/feedback: require user_id (return 400 if missing).
The DAL keeps `if user_id:` guarding for future system/scheduled
feedback flows, but the user-facing endpoint must always have it
to preserve the cross-user defense.
Tests:
- test_stream_without_terminal_event_still_records renamed to
..._as_aborted and now asserts the downgraded status
- New test_terminal_event_keeps_its_explicit_status sanity-checks
that the downgrade doesn't fire when a terminal event was seen
Not addressed (intentional):
- usage_recorder thread-pool refactor (CodeRabbit "🏗️ Heavy lift").
Per-event daemon thread is fine at Holmes' expected volume.
Reasoning posted in PR reply: at peak ~100 chats/min and a 60s
Supabase outage, peak in-flight threads is ~100 (~800KB total).
Daemon-thread overhead is negligible. A bounded ThreadPoolExecutor
becomes worth it if sustained traffic ever exceeds ~1000/min OR
Supabase outages become routine — neither is the case for v1.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: alonelish <alon.elish@gmail.com>
…ck_chat'
The Robusta runner's Slack handler currently doesn't populate request_type /
user_id / conversation_id structurally on /api/chat — instead it prepends a
fixed marker to the `ask` text before sending:
"**@user_U0AKMP2CZ97** • 2026-05-04T05:10:04Z\n\nhigh cpu in pod alert"
Without runner changes, those rows land in HolmesUsageEvents with NULL
request_type / user_id and look identical to a generic web client. That
prevents dashboards from grouping by client surface.
This change adds a heuristic in _build_chat_recorder_state that:
1. Detects the prefix via regex (matches '**@user_U<ID>** • <ISO ts>').
2. When matched and the caller didn't explicitly set request_type, tags
the row as request_type='slack_chat' (matches the existing _chat suffix
convention used by user_chat / agui_chat).
3. Captures the parsed slack_user_id and slack_triggered_at into
meta.slack so dashboards can drill in (per-Slack-user cost, etc.)
without waiting for the runner to send them as structured fields.
Caller-supplied request_type still wins, so scheduled prompts / agui /
checks remain unaffected. Slack metadata extraction also runs even when
request_type was overridden, so the meta.slack signal is preserved
regardless.
Caveats documented inline:
- Heuristic / fragile if the runner format ever changes.
- Doesn't recover conversation_id (Slack thread_ts isn't in the ask text),
so multi-turn Slack threads still won't group until the runner sends it.
- Doesn't fix the SaaS-side timeout that's causing 5006 errors mid-stream.
Long-term proper fix is the runner sending request_type / user_id /
conversation_id explicitly. This heuristic is a tactical workaround until
that ships.
Tests:
- 6 new cases in tests/test_chat_recorder_state.py covering: prefix sets
request_type; user_id/ts captured in meta; explicit request_type wins
over detection; no prefix uses default; meta merges with FE-supplied
meta; partial / non-matching prefix doesn't false-positive.
86 tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: alonelish <alon.elish@gmail.com>
Follow-up to f14450b: the previous commit only set request_type='slack_chat' when the Slack-runner prefix was detected, leaving request_source NULL. That worked for the FE's request_type-based client grouping but left an asymmetry — other request_source-driven dashboards saw NULL for every Slack row. Now request_source defaults to 'slack' when auto-detection fires AND the caller didn't explicitly send a value. Caller-supplied request_source still wins, so when the runner eventually ships finer values like 'slack_mention' / 'slack_alert_investigation' those override the default with no code change here. Tests: - New test_slack_prefix_sets_request_source_to_slack - New test_explicit_request_source_wins_over_slack_default (caller-wins) - Updated test_no_slack_prefix_... and test_partial_slack_prefix_... to also assert request_source is None when the prefix doesn't match 47 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
Reverts the 400-on-missing-user_id check from 0f8bd38. The check was added in response to a CodeRabbit suggestion about cross-user tampering defense, but the simpler model is fine for v1: - account_id-scoped RLS already prevents cross-account writes. - (account_id, request_id) in the UPDATE WHERE clause prevents another account's request_id from matching. - The remaining "another user in the same account spoofs your thumb" attack vector is low-impact (worst case: skews the attacker's own account's analytics). When user_id IS supplied as a query param, the DAL still adds .eq("user_id", user_id) as defense-in-depth (record_feedback unchanged). Just no longer required. Endpoint now succeeds with or without ?user_id=... Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
…mcnulty-822c30 Signed-off-by: alonelish <alon.elish@gmail.com> # Conflicts: # holmes/core/conversations_worker/worker.py
The worker's _run_chat_and_publish path bypassed server.py::chat() and
called request_ai.call_stream() directly, so the recorder wrapper never
fired. Result: every worker-driven (new conversation API) turn was
silently missing from HolmesUsageEvents — only title-generation /
classifier sub-calls that go through server.py::chat() were tracked.
Fix:
- Promote build_chat_recorder_state, detect_slack_origin, and
resolve_provider from server.py-private helpers to public
usage_recorder.py exports, parameterized on dal so any caller
(server, worker, scheduled prompts, agui, checks) can use them.
- Wrap the worker's call_stream() output with
stream_with_usage_recording before passing it to publisher.consume,
mirroring the wiring in server.py::chat() for the streaming path.
- Worker passes self.dal and is_streaming=True into the helper; the
ChatRequest already carries conversation_source='conversations' and
request_type='user_chat' from the existing construction at
worker.py:624-652.
Tests:
- tests/core/conversations_worker/test_worker_usage_recorder.py (new):
asserts the wrapper is called with the raw stream, the publisher
consumes the wrapped stream, and the helper receives the worker's
dal + is_streaming=True.
- tests/test_chat_recorder_state.py: re-pointed at the public helper,
added coverage for explicit conversation_source='conversations'
(worker-set) winning over the chat_history default.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: alonelish <alon.elish@gmail.com>
… turns
After the previous fix, worker-driven conversations were producing
HolmesUsageEvents rows but with NULL user_id and NULL request_source.
Root cause: the FE writes both onto the Conversations row when it
creates a chat (user_id as a column, request_source under metadata) but
doesn't repeat them in every per-turn user_message event's data. The
worker was reading only from the per-event blob, so follow-up turns
came in with both fields NULL.
Fix:
- ConversationTask gains an optional ``user_id`` field; the
Conversations row's ``user_id`` column is surfaced onto the task in
``_build_task_from_conversation_row``.
- ``_process_conversation`` now resolves both fields with a per-event-
wins fallback: ``data.get("user_id") or task.user_id`` and
``data.get("request_source") or task.metadata.get("request_source")``.
The Conversations row is the fallback, not an override, so any future
flow that legitimately wants to override per-turn (e.g. a chat
pivoting from alert_investigation to freeform) still can.
- source_ref / meta / is_internal stay per-event only — they're
per-turn signals, not Conversation-level state.
Tests:
- test_worker_lifecycle.py: extend existing row-parsing tests to assert
user_id round-trips off the row, and stays None when the row omits it.
- test_worker_usage_recorder.py: five new tests pinning the four
fallback combinations (row-only, event-only, both, neither) for both
user_id and request_source.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: alonelish <alon.elish@gmail.com>
Two bugs surfaced during PR review:
1. record_feedback wrote feedback_at via datetime.now().isoformat() —
a NAIVE local-time ISO string. Postgres timestamptz then interprets
the value relative to the *server* timezone, which differs from the
Holmes pod's TZ. Both interpretations produce shifted timestamps.
Fix: datetime.now(timezone.utc).isoformat() so the column always
stores the actual feedback moment regardless of pod / DB timezone.
2. ConversationWorker hard-coded request_type='user_chat' on the
ChatRequest it constructed. That defeated build_chat_recorder_state's
auto-detection logic — the helper only auto-classifies (Slack-prefix
→ 'slack_chat', etc.) when chat_request.request_type is falsy, so a
hard-coded value short-circuited every detection path. Today only
/api/chat sees the Slack prefix, but the runner could route Slack
through Conversations at any time without a code change here, and
then those rows would be mis-tagged. Fix: pass data.get("request_type")
through (None when absent) so the helper handles the default + Slack
detection consistently for both /api/chat and the worker path.
Tests:
- test_supabase_dal_usage.py: existing record_feedback test now asserts
feedback_at carries a UTC offset (+00:00 or Z), not a naive string.
- test_worker_usage_recorder.py: three new tests pinning the worker's
request_type passthrough behavior — FE-set value wins, unset stays
None for the helper, Slack-shaped ask propagates with request_type
still None so the helper can detect it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: alonelish <alon.elish@gmail.com>
Three byte-equivalent copies of the same litellm-provider lookup had accumulated: - holmes/core/usage_recorder.py:resolve_provider (the canonical, public) - holmes/checks/checks_api.py:_resolve_provider (private duplicate) - experimental/ag-ui/server-agui.py inline try/except (third copy) Replaced both copies with imports of the canonical helper. Drops the now-unused `import litellm` from checks_api.py and the local `import litellm as _litellm` from server-agui.py. Behavior unchanged — the bodies were already identical; the only practical effect is that future tweaks to provider resolution (e.g. handling a litellm API shift) need to land in exactly one place. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
Both entries already use the Grafana icon and live alongside other Grafana-family integrations; the prefix in the display name was just duplicating that signal. Brings them in line with how the rest of the catalog labels integrations (e.g. just 'Loki' / 'Tempo' rather than 'Grafana Loki' / 'Grafana Tempo'). IDs unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
moshemorad
requested changes
May 6, 2026
moshemorad
left a comment
moshemorad
left a comment
Collaborator
There was a problem hiding this comment.
Great work, left couple of comments let me know if something isn't clear.
…ctly Per review feedback (Moshe Morad on PR HolmesGPT#1969): the /api/feedback endpoint was a thin pass-through with no LLM/tool involvement, and using a Postgres RPC instead lets the function scope by `auth.uid()` (JWT-derived) rather than the FE-supplied query-param `user_id` Holmes was trusting blindly. Strictly stronger user-scoping. Removed from Holmes: - server.py: `class FeedbackRequest`, `def feedback`, the BaseModel import that became unused. - holmes/core/supabase_dal.py: `def record_feedback`, the `timezone` import (only used here). Replaced with a 6-line comment pointer to the RPC for future readers. - tests/core/test_supabase_dal_usage.py: the four record_feedback tests (no Holmes-side code path remains to unit-test). - Stale doc strings referencing `POST /api/feedback` updated to point at the RPC (server.py response-metadata comment, usage_recorder.py stream-wrapper docstring, test module docstring). The RPC body lives in the migration script (see plan file). FE migration from any prior endpoint to `supabase.rpc('record_feedback', ...)` is on the FE team — coordinating with this PR's deploy. Tests: 144 pass (was 148; the 4 dropped were record_feedback unit tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
Per review feedback (Moshe Morad on PR HolmesGPT#1969): the dataclass had 18 fields with only group-level comments, making it hard for callers to know which to set, what valid values are, and which are filled in later by the wrapper. Each field now carries a 1–8 line comment covering: - what the value represents - where it comes from (entry point vs wrapper-filled) - the canonical / accepted values for enum-ish fields (request_type, request_source, conversation_source, status, finish_reason, ...) - what gets written to the DB if the field is left at its default - cross-references to the helpers that fill mutable fields The class-level docstring also gained a clearer three-group breakdown (required identity, optional identity, mutable runtime) so a reader scanning the file knows up front which fields belong to which phase. No behavior change — pure documentation. All 144 existing tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
Per review feedback (Moshe Morad on PR HolmesGPT#1969): the recorder's `status` column had five string-literal values ('success' / 'approval_required' / 'error' / 'rate_limited' / 'aborted') sprinkled across the field default, three branches of stream_with_usage_recording, and both record_from_llm_result / record_error. Easy to typo, hard to discover the full set, no compiler help. Introduced RequestStatus(str, Enum) — same pattern as ConversationStatus in conversations_worker/models.py. Subclassing str keeps the values JSON-serializable as plain strings (verified: json.dumps produces "success", not "RequestStatus.SUCCESS") and equality with old string literals still works, so existing tests that assert `kwargs["status"] == "success"` keep passing without changes. Also dropped the unused HOLMES_USAGE_EVENT_CALLS_TABLE constant from supabase_dal.py — leftover from the child-table approach we explicitly dropped earlier in this PR. No code referenced it. All 144 existing tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
Per review feedback (Moshe Morad on PR HolmesGPT#1969): the DAL had a 20-kwarg signature that mirrored every field on UsageRecorderState plus a to_kwargs() method on the state class to pack the dict. Net cost: adding a new field meant editing the dataclass, the to_kwargs method, the DAL signature, the DAL body, and any test that constructed kwargs. Five places, one logical change. Refactor: record_usage_event(state) takes the entire state object positionally and reads fields off it. The DAL is now the single place that knows the column shape — adding a new field is "add it on the state, read it here, write the migration." Net diff: - Drops UsageRecorderState.to_kwargs() (~24 lines). - Replaces the 20-kwarg DAL signature with `state: UsageRecorderState` (~50 lines lighter, more readable). - Adds duration_ms as a @Property on the state — was previously computed inside to_kwargs(), now the DAL just reads state.duration_ms. - TYPE_CHECKING import in supabase_dal.py for UsageRecorderState (no runtime circular-import risk since usage_recorder also TYPE_CHECKING- imports SupabaseDal). - Tests refactored: kwargs-based call sites become `mock_dal.record_usage_event(_make_state(...))`, and the recorder tests assert on `state.dal.record_usage_event.call_args.args[0]` (the live state object) instead of the old kwargs dict. Added one new test for state.stats=None handling and one for the duration_ms property type contract. 146 pass (was 144). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
Per review feedback (Moshe Morad on PR HolmesGPT#1969): the helper was building RequestStats from llm_result.model_dump() by manually filtering the dict against a hardcoded set of 9 field names. Cleaner and self-maintaining: ask Pydantic to do the include-filter using RequestStats.model_fields. state.stats = RequestStats( **llm_result.model_dump(include=set(RequestStats.model_fields)) ) LLMResult IS-A RequestStats but adds extra fields (tool_calls, messages, finish_reason, ...). The `include=` parameter tells model_dump() to emit only the keys that match RequestStats's own model_fields, so the extras get dropped without us hardcoding which keys belong to stats. When RequestStats grows a new column, this code stays correct automatically. No behavior change — same fields end up on state.stats. Net −10 lines. All 146 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
Per review feedback (Moshe Morad on PR HolmesGPT#1969): the two private helpers that operate solely on the state are a natural fit as methods. Before After ────────────────────────── ────────────────────────── _fire(state) state._fire() _capture_terminal(state, data) state._capture_terminal(data) Public functions stay at module level — they're either operating on external inputs (stream_with_usage_recording's primary arg is a stream, not state) or are simple recording entry points where the function signature reads naturally: record_from_llm_result(state, llm_result) # stays a function record_error(state, exc) # stays a function stream_with_usage_recording(stream, state) # stays a function This addresses Moshe's "lots of functions take state" concern for the helpers that genuinely belong on the class without converting the whole module to OO style — the data/behavior split (passive UsageRecorderState + recording functions over it) is the same pattern Holmes uses elsewhere (build_chat_messages, format_tool_result_data, etc.). _inject_request_id stays a function too — it operates on the stream event's data dict, not on state, and only takes request_id as an arg. Updated supabase_dal.py docstring to reference UsageRecorderState._fire instead of the old module path. All 146 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
Before this change: when the agentic loop raised an exception after some
iterations had already succeeded (e.g. rate-limit on iteration 3 after
1 and 2 burned real tokens), the recorder fired a row with stats=None
and status=error — the tokens those successful iterations consumed were
silently lost from analytics.
Root cause: call_stream accumulates `stats` in a local variable inside
the generator function. The accumulated total only escapes via the
terminal event's metadata.costs. When an exception unwinds the frame,
the local `stats` is GC'd; the recorder has nothing to record.
But the data is already on the wire — call_stream emits a TOKEN_COUNT
event after every successful iteration with metadata.costs set to the
running cumulative total. The wrapper just wasn't listening.
Fix: one new branch in stream_with_usage_recording for TOKEN_COUNT that
calls the freshly extracted state._capture_costs(). Each TOKEN_COUNT
overwrites state.stats with the latest cumulative figure, so when the
loop later raises, state.stats holds iterations-1..N-1's cost. Also
benefits the client-disconnect case (was: status=aborted with $0; now:
status=aborted with real partial cost).
Refactor: split _capture_terminal into _capture_costs (pure cost
extraction, called from both TOKEN_COUNT and terminal branches) +
the iteration / finish_reason extraction (terminal-only).
Tests:
- test_token_count_event_captures_cumulative_costs — verifies
TOKEN_COUNT updates state.stats and ANSWER_END's terminal capture
works alongside it (last cumulative wins; equal in success case).
- test_partial_costs_captured_when_loop_raises_mid_iteration — the
case the user asked about: 2 iterations succeed (TOKEN_COUNT each),
iteration 3 raises before its TOKEN_COUNT, recorder fires with
partial cost preserved and status=error.
- test_partial_costs_captured_on_client_disconnect — same partial
capture for the abort path; status=aborted with real cost (was
$0 before this change).
149 pass (was 146).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: alonelish <alon.elish@gmail.com>
Avi-Robusta
previously approved these changes
May 7, 2026
Avi-Robusta
left a comment
Avi-Robusta
left a comment
Collaborator
There was a problem hiding this comment.
Looked at moshes comments and your changes since then
LGTM
I saw some git actions here are failing,
Check if they are the ones that normally fail for you or if this code introduced new issues there
moshemorad
requested changes
May 10, 2026
Per review feedback (Moshe Morad on PR HolmesGPT#1969): _fire previously spawned a fresh OS thread per HolmesUsageEvents write. Switch to a module-level ThreadPoolExecutor(max_workers=4, thread_name_prefix="usage-recorder") that all states share. Net behavior changes: - Caps concurrent recorder writes at 4 (caps blast radius if Supabase is slow or supabase-py's connection pool is contended). - Removes per-request thread-spawn overhead (~50–200μs). - Cleaner shutdown semantics — Python's atexit handler drains live executors, so we lose fewer rows on graceful shutdown than the previous daemon-thread fire-and-forget shape (which was dropped abruptly on process exit). - Under burst, additional submissions queue inside the executor rather than spawning unbounded threads. Threads are spawned lazily on first submit, so importing the module doesn't start any. max_workers=4 is sized for Holmes' single-pod-per- customer load: one Supabase write is ~50–200ms, so 4 workers handle ~80 events/sec sustained — well above current request rates and below supabase-py's default pool size, so we won't starve foreground writes. submit() raises RuntimeError if the executor is shut down (process exiting); _fire catches that explicitly and accepts the loss (same fate as in-flight rows on the previous daemon-thread shape). Tests: _patch_inline_thread now stubs the executor with an inline submit() that mimics ThreadPoolExecutor.submit's swallow-target-exception semantic (real executor parks them on the Future). Added one new test covering the executor-shutdown branch. 156 pass (was 155). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
moshemorad
approved these changes
May 11, 2026
This was referenced Jun 30, 2026
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
3 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
Wires every LLM-consuming entry point through a new shared usage recorder so each request lands as one row in a new
HolmesUsageEventsSupabase table. Enables per-account / per-cluster / per-user / per-feature cost reporting plus thumbs-up/down feedback on individual chat turns — none of which is answerable today (Holmes persists no AI metrics).Driven by the AI Metrics requirements doc (Hypatos / Adobe Firefly / Playtika / Cynet asks).
What's covered
POST /api/chat(direct)chat()chat()request_type='scheduled_prompt')POST /api/agui/chat(experimental)request_type='agui_chat')POST /api/checks/executerequest_type='health_check')POST /api/feedbackholmes ask,holmes check,holmes interactive)How it works
holmes/core/usage_recorder.py(new) —UsageRecorderStatedataclass +stream_with_usage_recording(streaming) +record_from_llm_result(non-streaming) +record_error. Single source of truth used by all four call sites. Fire-and-forget on a daemon thread so telemetry never blocks the response.SupabaseDal.record_usage_event/record_feedbackwrite through the existingself.client(same email+password user-JWT pattern asupsert_holmes_status). No new env vars, no service_role key. RLS mirrorsHolmesStatus'sFOR ALL TO public USING (account_id IN get_user_accounts() OR is_support())policy.ChatRequestgains 6 optional fields:request_type,request_source,source_ref,conversation_id,conversation_source,meta. All additive — old clients keep working.LLMResultgains afinish_reasonfield captured from the last LLM iteration'sresponse.choices[0].finish_reason.POST /api/feedbackaccepts{request_id, sentiment, category?, comment?}and UPDATEs the matching event row.Schema (separately applied to Supabase)
HolmesUsageEventscovers identity (account_iduuid,cluster_id,user_iduuid,conversation_id,conversation_sourcediscriminator,request_id), classification (request_type,request_source,source_ref,model,provider,is_robusta_model), the fullRequestStatsset (token counts, costs, peaks, compactions), shape/outcome (iterations,tool_call_count,duration_ms,is_streaming,finish_reason),feedback_*columns, and ametaJSONB for forward-compat.Optional
HolmesUsageEventCallsper-LLM-call child table is gated byHOLMES_RECORD_LLM_CALLS=true— table created with RLS but emission not enabled in v1 (deferred — each check is one LLM call so child rows would near-duplicate the parent).Test plan
tests/core/test_usage_recorder.py(15 tests covering streaming/non-streaming/error/approval paths, disabled-DAL no-op, fire-and-forget threading).tests/core/test_supabase_dal_usage.py(11 tests covering payload shape, account-scoped UPDATE, NULL handling, error swallowing).finish_reasoncapture (one regression I introduced and fixed:isinstance(str)guard soMagicMock'd test responses don't break pydantic validation)./bin/sh, tempfile permission semantics) — verified by checking thatholmes/main.pyand other files I never touched have the same pre-existing failures.Deployment notes
HolmesUsageEvents+ RLS).self.client, no new env vars to provision.request_source/source_ref/user_id/conversation_idon/api/chatbody.dataofuser_messageevents written toConversationEventsfor the worker path.conversation_idthe FE writes to itsChatHistoryrow soLEFT JOIN ChatHistoryworks.Out of scope (per plan)
DB_HOLMES_USAGE_EVENTS_SELECTpermission action gets minted)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests