Add chat-analytics fields to HolmesChat schemas#2058
Open
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:c66806e
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:c66806e me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:c66806e
docker push me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:c66806ePatch 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:c66806e |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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. Changes
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 200 or non-fatal 4xx
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 reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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
Comment on lines
+251
to
+252
| request_id: str | ||
| sentiment: Literal["thumbs_up", "thumbs_down"] |
There was a problem hiding this comment.
Enforce non-empty request_id at validation time.
request_id: str still accepts empty strings, so malformed feedback can pass model validation and be forwarded.
💡 Proposed fix
- request_id: str
+ request_id: str = Field(..., min_length=1)📝 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.
Suggested change
| request_id: str | |
| sentiment: Literal["thumbs_up", "thumbs_down"] | |
| request_id: str = Field(..., min_length=1) | |
| 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/model/base_params.py` around lines 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.
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
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