ROB-3618: Add chat-analytics fields to HolmesChat schemas#2058
Merged
ROB-3618: Add chat-analytics fields to HolmesChat schemas#2058
Conversation
Declare request_type, request_source, source_ref, conversation_id, conversation_source, and meta as optional fields on HolmesChatParams and HolmesChatRequest so they're typed and discoverable end-to-end. The fields already passed through via extra="allow" + Pydantic v1 dict() semantics; this change makes them first-class. HolmesIssueChat* inherits the fields automatically. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:0371c0a
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:0371c0a me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:0371c0a
docker push me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:0371c0aPatch Helm values in one line: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set runner.image=me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:0371c0a |
|
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 Holmes feedback support and extends Holmes chat params/requests with optional request/conversation metadata, an internal flag, and free-form meta; implements a playbook action that POSTs feedback to Holmes, adds tests, and exposes the action in Helm values. ChangesHolmes feedback + chat metadata
Sequence Diagram(s)sequenceDiagram
participant Playbook as Playbook Action
participant Event as ExecutionEvent
participant Client as Holmes HTTP Client
participant Holmes as Holmes Server
participant Store as Findings Store
Playbook->>Event: receive HolmesFeedbackParams
Playbook->>Client: build HolmesFeedbackRequest (validate, strip runner-only fields)
Client->>Holmes: POST /api/feedback (timeout)
Holmes-->>Client: 200 / 4xx / 5xx
alt 2xx success
Client-->>Playbook: response
Playbook->>Store: add_finding(HolmesFeedbackResult)
else 5xx or exception
Client-->>Playbook: error
Playbook->>Playbook: handle_holmes_error (log request_id, raise ActionException)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Proxies user feedback (thumbs up/down) on a Holmes answer from the FE to the Holmes server's POST /api/feedback, which UPDATEs the HolmesUsageEvents row keyed by request_id. Mirrors the holmes_oauth pattern (small synchronous POST with raise_for_status pass-through), with pydantic-level validation of request_id presence and sentiment via Literal["thumbs_up", "thumbs_down"] so bad payloads fail at param construction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/robusta/core/reporting/holmes.py (1)
70-80: Align feedbacksentimenttype with the params contract.
HolmesFeedbackRequest.sentimentis currently unconstrainedstr; mirroring theLiteralcontract avoids accidental invalid payloads if this model is used outside the current action path.💡 Proposed refactor
-from typing import Any, Dict, List, Optional, Union +from typing import Any, Dict, List, Literal, Optional, Union @@ - sentiment: str + sentiment: Literal["thumbs_up", "thumbs_down"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/robusta/core/reporting/holmes.py` around lines 70 - 80, HolmesFeedbackRequest currently types sentiment as plain str; change it to a constrained Literal to match the params contract (e.g., Literal['positive','negative','neutral'] or the exact allowed tokens used elsewhere) and import typing.Literal; update the model declaration (class HolmesFeedbackRequest) to use that Literal type for sentiment and adjust any callers/validators to conform to the narrowed set so invalid payloads are caught at model validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/robusta/core/model/base_params.py`:
- Around line 251-252: The request_id field currently typed as plain str accepts
empty strings; update the model that declares request_id and sentiment to
validate non-empty ids by either changing request_id's type to
pydantic.constr(min_length=1) or by using Field(..., min_length=1) on
request_id, and add/import the needed pydantic symbol; alternatively add a
`@validator` for "request_id" that raises a ValueError on empty/whitespace-only
strings so malformed feedback is rejected at validation time.
---
Nitpick comments:
In `@src/robusta/core/reporting/holmes.py`:
- Around line 70-80: HolmesFeedbackRequest currently types sentiment as plain
str; change it to a constrained Literal to match the params contract (e.g.,
Literal['positive','negative','neutral'] or the exact allowed tokens used
elsewhere) and import typing.Literal; update the model declaration (class
HolmesFeedbackRequest) to use that Literal type for sentiment and adjust any
callers/validators to conform to the narrowed set so invalid payloads are caught
at model validation.
🪄 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: 3228b0cc-f9b3-4d64-82e2-144c6b2fba0a
📒 Files selected for processing (4)
src/robusta/core/model/base_params.pysrc/robusta/core/playbooks/internal/ai_integration.pysrc/robusta/core/reporting/holmes.pytests/test_ai_integration.py
The Helm chart's allowlist gates which action names the relay can forward to the runner. Without this entry the relay rejects holmes_feedback before it reaches the action handler. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Optional boolean alongside the other chat-analytics fields (request_source, conversation_id, etc.) so the FE can mark internal/test traffic and the Holmes server can filter it out of usage dashboards. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- HolmesFeedbackParams.request_id: enforce non-empty via Field(min_length=1) so empty strings are rejected at param construction. - HolmesFeedbackRequest: mirror the params contract — sentiment is now Literal["thumbs_up", "thumbs_down"] and request_id has min_length=1, so the request body class can't be misused outside the action path. - Add a test covering the empty-request_id rejection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alonelish
changed the title
Holmes' POST /api/feedback reads user_id from request.query_params, not the JSON body (mirrors /api/chat's resolution path used by the relay). Posting it in the body returns 400 "missing user_id". Pull user_id out of the action params and forward it via the URL query string. - Declare user_id: Optional[str] on HolmesFeedbackParams so it's discoverable; pop it before constructing the request body. - Update tests to assert user_id is on params= and not in the body. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- handle_holmes_error: log the upstream response body on HTTPError so
4xx/5xx diagnostics aren't lost in the generic "Holmes internal
configuration error" branch (e.g. {"detail":"missing user_id"}
would otherwise be invisible). Benefits all Holmes proxy actions,
not just feedback.
- holmes_feedback: drop the no-op Finding on success. The FE shows
the "Thanks" toast optimistically and never reads the response;
emitting a TYPE_NONE finding for every thumb click was UI noise.
Replaced with an info log line carrying request_id + sentiment.
- Tests updated to assert add_finding is no longer called.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The feedback flow has moved off the runner: the FE now records thumbs up/down directly via a Supabase RPC, bypassing the relay → runner → Holmes path. Drop the runner-side proxy: - HolmesFeedbackParams and HolmesFeedbackRequest schemas - holmes_feedback action handler - 7 unit tests - helm chart lightActions allowlist entry Kept from earlier PR commits: - Chat-analytics fields on HolmesChat schemas (request_source, conversation_id, source_ref, conversation_source, request_type, is_internal, meta) — still useful for /api/chat - handle_holmes_error response-body logging — benefits all Holmes proxy actions (chat, oauth, conversation, issue_chat) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Avi-Robusta
approved these changes
May 7, 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
Declare 6 optional analytics fields on the legacy chat path so they're typed and discoverable end-to-end:
request_typerequest_sourcesource_refconversation_idconversation_sourcemetaAdded to
HolmesChatParams(src/robusta/core/model/base_params.py) andHolmesChatRequest(src/robusta/core/reporting/holmes.py).HolmesIssueChatParams/HolmesIssueChatRequestinherit them automatically.Why
The Holmes server's
/api/chatendpoint is being extended to capture these fields for usage slicing (UI surface, multi-turn grouping, etc.). The FE will start sending them. Both schemas already hadextra = "allow"and the runner is on Pydantic v1, so the fields functionally pass through today — this change just makes them first-class so they're typed, IDE-autocompletable, and self-documenting at both runner boundaries.Notes for reviewers
Optional[...] = None— fully backwards-compatible. Old clients that don't send them keep working; the runner forwardsnullfor missing ones.holmes_chatalready usesparams.dict()→HolmesChatRequest(**params_dict), so declared fields propagate without extra plumbing.holmes_issue_chatuses an explicit kwarg constructor and does not populate the new fields; they default toNoneon that path. This is intentional — only the legacy/api/chatpath is in scope.extra = "allow"is preserved on both classes as a safety net for future fields.Test plan
HolmesChatParams.__fields__andHolmesChatRequest.__fields__both contain the 6 new field names.HolmesChatParams(ask="x", conversation_id="ch-1", request_source="alert_investigation", source_ref="issue-2", meta={"k":"v"}), dump via.dict(exclude={"holmes_url","render_graph_images"}), pass intoHolmesChatRequest(**...), and confirm the JSON includes the new fields.HolmesChatParams(ask="x")only and confirm all 6 fields serialize asnull.🤖 Generated with Claude Code