Rca prompt v2 comparison - #457
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRewrites build_rca_prompt to accept (source, title, payload) with embedded serialized/truncated payloads, adds get_alert_field (dot-path payload extractor with DB fallback and descriptive errors), extends JSON truncation with depth limits, migrates provider tasks to the unified builder, and adds an opt-in eager message persist. ChangesUnified RCA Prompt Builder with Alert Payload Extraction
Sequence Diagram(s)sequenceDiagram
participant RawPayload
participant build_rca_prompt
participant truncate_json_fields
participant extract_rail_text
RawPayload->>build_rca_prompt: (source, title, payload, user_id)
build_rca_prompt->>truncate_json_fields: serialize & conditional truncation
build_rca_prompt->>extract_rail_text: collect attacker-controlled strings
truncate_json_fields-->>build_rca_prompt: truncated payload JSON
extract_rail_text-->>build_rca_prompt: rail_text
build_rca_prompt-->>RawPayload: (rca_prompt, rail_text)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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. Comment |
OlivierTrudeau
marked this pull request as draft
May 28, 2026 22:00
![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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/chat/backend/agent/tools/alert_payload_tool.py`:
- Around line 52-155: The get_alert_field function is doing too many things;
split it into small helper functions to reduce complexity and make testing
easier: create validate_get_alert_field_inputs(json_path, user_id, incident_id)
to perform the initial checks and return early errors;
fetch_payload_for_incident(cursor, conn, incident_id, user_id,
_SOURCE_TABLE_MAP) to encapsulate the DB logic (direct id lookup, org fallback,
use set_rls_context and db_pool) and return the raw payload or None;
parse_json_payload(payload) to handle string vs already-parsed JSON and JSON
errors; traverse_json_path(payload, json_path) to walk dot/index parts and
return either the found value or structured error messages; and
format_tool_output(value, MAX_TOOL_OUTPUT_CHARS) to serialize/truncate results;
keep exception logging via logger.exception in get_alert_field and call these
helpers in sequence (preserving existing symbols get_alert_field,
_SOURCE_TABLE_MAP, set_rls_context, db_pool, MAX_TOOL_OUTPUT_CHARS) so behavior
is unchanged but modularized.
- Around line 94-105: The current fallback in the block that sets payload_row
(uses table, user_id, org_id and ORDER BY received_at DESC) is too broad and can
return a payload from a different incident; replace it with a stricter fallback:
fetch the incident row from incidents to get the incident timestamp (e.g.,
created_at) and any provider-specific id (source_alert_id/source_event_id) and
first attempt a lookup by that source-specific identifier; if that fails, query
the payload table for rows matching the same org_id and user_id but constrain by
a tight time window around the incident timestamp (e.g., received_at BETWEEN
created_at - window AND created_at + window) and, if available, match
provider/source fields; if still no match, return None (fail-closed) instead of
returning the latest row. Ensure you modify the code that references
payload_row, cursor.execute(...) and the SELECT payload FROM {table} ... ORDER
BY received_at DESC LIMIT 1 block to implement these stricter checks.
In `@server/chat/backend/agent/tools/cloud_tools.py`:
- Around line 2374-2376: get_alert_field is being wrapped without injecting the
required incident_id, causing runtime "No incident context" errors; update the
wrapping so the function receives incident_id from the current tool/agent
context before further wrapping. Concretely, change the line building _ctx so
that you inject the incident id into get_alert_field (e.g., build a small
wrapper around get_alert_field that reads incident_id from the runtime
context/args and calls get_alert_field(incident_id=..., json_path=...) or extend
with_user_context usage to include incident_id), then apply
with_completion_notification and wrap_func_with_capture as before (symbols:
get_alert_field, with_user_context, with_completion_notification,
wrap_func_with_capture, args_schema).
In `@server/chat/backend/agent/tools/output_sanitizer.py`:
- Around line 31-35: The current truthy check on max_list_items causes incorrect
behavior for 0 and negative values; change the logic so you explicitly handle
None vs numeric values: in the block that uses max_list_items and data (the code
calling truncate_json_fields on list items), replace the truthy test with an
explicit None check (e.g., if max_list_items is not None:) and then coerce/clamp
numeric inputs (e.g., if max_list_items < 0: max_list_items = 0) so that
max_list_items==0 yields an empty items slice plus the "... [N more items]"
sentinel and None continues to mean "no length limit." Ensure you still call
truncate_json_fields for each item and use data[:max_list_items] when
max_list_items is numeric; leave behavior unchanged when max_list_items is None.
In `@server/chat/background/rca_prompt_builder.py`:
- Around line 310-323: The guardrail field match is case-sensitive because
_RAIL_FIELDS contains mixed-case entries (e.g., "ruleName") while _collect
lowercases keys; update the logic so matching is normalized: either convert all
entries in _RAIL_FIELDS to lowercase (canonicalize the tuple) or compare
key.lower().rstrip('_') against a lowercased set created from _RAIL_FIELDS;
ensure this change is applied where _RAIL_FIELDS is referenced in the _collect
function so fields like "ruleName" are correctly detected and added to parts.
In `@server/chat/background/task.py`:
- Around line 464-468: The eager append path assumes each entry in messages is a
dict and blindly sets message_number=1; fix by first normalizing/validating the
messages list (e.g., when messages is a list, filter or coerce only items where
isinstance(item, dict) and 'sender' in item and 'text' in item), then check for
any item with sender == "user" using that cleaned list, and if none compute the
next message_number dynamically (e.g., max(int(m.get("message_number", 0)) for m
in cleaned_messages) + 1 or fallback to len(cleaned_messages)+1) before
appending the new dict; ensure you use the cleaned list for both the existence
check and the append to avoid exceptions and duplicate numbering in
functions/variables around messages, initial_message, and the cursor write.
In `@server/routes/netdata/tasks.py`:
- Around line 315-317: The RCA prompt is being built from the normalized `data`
which can drop raw webhook fields; update the call to `build_rca_prompt` in
`server/routes/netdata/tasks.py` to pass the original raw `payload` instead of
`data` so Netdata uses the unmodified webhook payload (keep
`build_rca_prompt("netdata", ..., user_id=user_id)` and the returned
`rca_prompt, rail_text` unchanged).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e67406e5-8bea-4611-831a-63dc88faa49d
📒 Files selected for processing (20)
server/chat/backend/agent/tools/alert_payload_tool.pyserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/backend/agent/tools/output_sanitizer.pyserver/chat/backend/agent/tools/trigger_rca_tool.pyserver/chat/background/rca_prompt_builder.pyserver/chat/background/task.pyserver/routes/bigpanda/tasks.pyserver/routes/datadog/tasks.pyserver/routes/dynatrace/tasks.pyserver/routes/grafana/tasks.pyserver/routes/incidentio/tasks.pyserver/routes/jenkins/tasks.pyserver/routes/netdata/helpers.pyserver/routes/netdata/tasks.pyserver/routes/newrelic/tasks.pyserver/routes/opsgenie/tasks.pyserver/routes/pagerduty/tasks.pyserver/routes/sentry/tasks.pyserver/routes/spinnaker/tasks.pyserver/routes/splunk/tasks.py
💤 Files with no reviewable changes (1)
- server/routes/netdata/helpers.py
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
OlivierTrudeau
marked this pull request as ready for review
May 29, 2026 21:58
Contributor
Author
|
@CodeRabbit ai review |
Contributor
|
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/chat/background/rca_prompt_builder.py (1)
398-416: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPreserve
alert_servicefor Aurora Learn and prediscovery lookups.
alert_serviceis now always"", so both retrieval paths lose a high-signal discriminator and will return less relevant context for generic titles like CPU, latency, or deploy alerts. Please derive it from common payload fields or pass it in explicitly from callers.♻️ One low-impact option
- alert_service = "" + alert_service = "" + if isinstance(payload, dict): + for key in ("service", "host", "app", "application", "component"): + value = payload.get(key) + if isinstance(value, str) and value.strip(): + alert_service = value.strip() + break🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/chat/background/rca_prompt_builder.py` around lines 398 - 416, alert_service is hard-coded to "" before calling _get_similar_good_rcas_context and _get_prediscovery_context which removes a high-signal discriminator; change the code in rca_prompt_builder (the block creating alert_service) to derive the service name from the incoming alert payload (e.g., check common fields like payload.get("service"), payload.get("resource"), payload.get("component") or a nested metadata/service field) and fall back to an explicit parameter if the caller provides one, then pass that derived alert_service into _get_similar_good_rcas_context and _get_prediscovery_context; if callers don't supply service, update the call sites to accept and forward the service when available.
♻️ Duplicate comments (1)
server/chat/backend/agent/tools/alert_payload_tool.py (1)
92-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed instead of guessing from the newest row in the time window.
This fallback still picks whichever payload for the same user happened most recently in the 10-minute window. If two alerts from the same source arrive close together,
get_alert_field()can drill into the wrong incident payload and send the RCA down a false path.🔧 Minimal safe fix
- # Fallback: time-window constrained lookup around incident creation - if not payload_row or not payload_row[0]: - if incident_created_at: - window_start = incident_created_at - _PAYLOAD_LOOKUP_WINDOW - window_end = incident_created_at + _PAYLOAD_LOOKUP_WINDOW - cursor.execute( - f"SELECT payload FROM {table} " - f"WHERE user_id = %s AND received_at BETWEEN %s AND %s " - f"ORDER BY received_at DESC LIMIT 1", - (user_id, window_start, window_end), - ) - payload_row = cursor.fetchone() + # Fail closed until we have a source-specific fallback that can uniquely + # correlate the incident to a stored payload row. + if not payload_row or not payload_row[0]: + return None, f"Error: No payload found in {table} for source_alert_id {source_alert_id}."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/chat/backend/agent/tools/alert_payload_tool.py` around lines 92 - 103, The fallback lookup currently picks the most recent payload in the time window which can return the wrong incident; change the logic in the block that queries payload_row (symbols: payload_row, incident_created_at, _PAYLOAD_LOOKUP_WINDOW, cursor, table, user_id, get_alert_field) so it does NOT silently choose the newest row when multiple candidate payloads exist: fetch all matching rows (or COUNT them) for the given user_id BETWEEN window_start and window_end and if more than one result is returned, treat it as no unambiguous match (leave payload_row as None / return None) instead of selecting the latest; only accept and assign payload_row if exactly one matching payload is found (or else fail closed).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/components/tool-calls/ToolExecutionWidget.tsx`:
- Around line 193-198: The parse-failure branch currently overwrites any
existing command label with "get alert field"; instead, when
JSON.parse(normalizedInput) throws, do not change command so the precomputed
tool.command (or prior default) is preserved—only assign command when you
successfully parse and extract a non-empty json_path from parsed.json_path or
parsed.kwargs?.json_path; keep the existing fallback behavior elsewhere that
collapses raw JSON to the default name if needed. Ensure you reference the
normalizedInput parse block, the parsed variable, and the command variable in
your change.
In `@server/chat/background/rca_prompt_builder.py`:
- Around line 366-378: The code truncates payload fields in build_rca_prompt()
using truncate_json_fields even for chat-origin payloads which cannot be
recovered by get_alert_field; modify the logic so that if payload.get("source")
== "chat" (or payload["source"] == "chat") you do not perform aggressive
truncation—always preserve full chat fields like "issue_description" (or at
minimum include the untruncated "issue_description") and skip the second-stage
deep truncation (max_field_length=80, max_depth=1); keep references to
PAYLOAD_CHAR_THRESHOLD, truncate_json_fields, and get_alert_field to locate
where to change behavior.
---
Outside diff comments:
In `@server/chat/background/rca_prompt_builder.py`:
- Around line 398-416: alert_service is hard-coded to "" before calling
_get_similar_good_rcas_context and _get_prediscovery_context which removes a
high-signal discriminator; change the code in rca_prompt_builder (the block
creating alert_service) to derive the service name from the incoming alert
payload (e.g., check common fields like payload.get("service"),
payload.get("resource"), payload.get("component") or a nested metadata/service
field) and fall back to an explicit parameter if the caller provides one, then
pass that derived alert_service into _get_similar_good_rcas_context and
_get_prediscovery_context; if callers don't supply service, update the call
sites to accept and forward the service when available.
---
Duplicate comments:
In `@server/chat/backend/agent/tools/alert_payload_tool.py`:
- Around line 92-103: The fallback lookup currently picks the most recent
payload in the time window which can return the wrong incident; change the logic
in the block that queries payload_row (symbols: payload_row,
incident_created_at, _PAYLOAD_LOOKUP_WINDOW, cursor, table, user_id,
get_alert_field) so it does NOT silently choose the newest row when multiple
candidate payloads exist: fetch all matching rows (or COUNT them) for the given
user_id BETWEEN window_start and window_end and if more than one result is
returned, treat it as no unambiguous match (leave payload_row as None / return
None) instead of selecting the latest; only accept and assign payload_row if
exactly one matching payload is found (or else fail closed).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b0b14dc-8124-4548-8916-f11caa1b8f60
📒 Files selected for processing (8)
client/src/components/tool-calls/ToolExecutionWidget.tsxserver/chat/backend/agent/tools/alert_payload_tool.pyserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/backend/agent/tools/output_sanitizer.pyserver/chat/backend/agent/tools/trigger_rca_tool.pyserver/chat/background/rca_prompt_builder.pyserver/chat/background/task.pyserver/routes/netdata/tasks.py
damianloch
requested changes
May 30, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/chat/background/task.py (1)
456-491:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't write the RCA prompt to
chat_sessions.messagesbefore the input rail runs.This bypasses the existing safety boundary: if the workflow later blocks on
rail_text, the attacker-controlled payload has already been persisted to chat history.server/chat/backend/agent/workflow.py:1096-1103intentionally saves user text only after the rail passes to avoid exactly that.Suggested fix
- # Eagerly persist the initial user message so it's visible in the UI immediately (opt-in) - if os.environ.get("DISPLAY__RCA_USER_MSG", "").lower() in ("1", "true", "yes"): - try: - with db_pool.get_admin_connection() as conn: - with conn.cursor() as cursor: - set_rls_context(cursor, conn, user_id, log_prefix="[BackgroundChat:EagerMsg]") - cursor.execute("SELECT messages FROM chat_sessions WHERE id = %s", (session_id,)) - row = cursor.fetchone() - messages = row[0] if row and row[0] else [] - if isinstance(messages, str): - messages = json.loads(messages) - if not isinstance(messages, list): - messages = [] - has_user_message = any( - isinstance(m, dict) and m.get("sender") == "user" - for m in messages - ) - if not has_user_message: - existing_numbers = [ - int(m.get("message_number", 0)) - for m in messages - if isinstance(m, dict) - ] - next_num = max(existing_numbers, default=0) + 1 - messages.append({ - "sender": "user", - "text": initial_message, - "message_number": next_num, - }) - cursor.execute( - "UPDATE chat_sessions SET messages = %s::jsonb WHERE id = %s", - (json.dumps(messages), session_id), - ) - conn.commit() - except Exception as e: - logger.warning(f"[BackgroundChat] Failed to eagerly persist user message: {e}") + # Keep persistence behind the existing input-rail gate inside the workflow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/chat/background/task.py` around lines 456 - 491, The current eager-persist block gated by DISPLAY__RCA_USER_MSG writes the user/ RCA prompt into chat_sessions.messages immediately, which bypasses the input-rail safety check; instead, stop persisting the prompt here and only persist after the rail has been validated. Remove or disable the eager write in the block around db_pool.get_admin_connection()/set_rls_context and either (a) move the persistence logic to the existing workflow save path (the code in server/chat/backend/agent/workflow.py that writes user text after rail_text passes), or (b) add a guard that explicitly verifies the rail has passed for session_id (e.g., query workflow/rail status) before updating chat_sessions.messages; reference DISPLAY__RCA_USER_MSG, initial_message, chat_sessions messages, get_admin_connection, and set_rls_context when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/chat/background/rca_prompt_builder.py`:
- Around line 405-410: The alert_service assignment uses
(payload.get("metadata", {}) or {}).get("service", "") which fails if
payload["metadata"] is not a dict; change the logic in the alert_service
construction to first read metadata = payload.get("metadata") and coerce it to a
dict only when it's a dict (e.g., metadata_dict = metadata if
isinstance(metadata, dict) else {}), then compute alert_service =
payload.get("service") or payload.get("resource") or payload.get("component") or
metadata_dict.get("service", "") or ""; update the code around the alert_service
variable in rca_prompt_builder.py to use that guarded metadata lookup so
AttributeError cannot occur.
- Around line 367-368: The current truncation slices the serialized JSON blob
(variables serialized, json_content) which can break JSON syntax; instead parse
the payload back to an object, walk string fields (or specific fields used by
get_alert_field) and truncate their values so the total serialized size is <=
CHAT_PAYLOAD_MAX, then re-serialize to json_content; update the logic around
payload_size/CHAT_PAYLOAD_MAX in rca_prompt_builder.py to perform field-level
truncation (preferably only on large text fields) to preserve valid JSON shape
and maintain compatibility with get_alert_field.
---
Outside diff comments:
In `@server/chat/background/task.py`:
- Around line 456-491: The current eager-persist block gated by
DISPLAY__RCA_USER_MSG writes the user/ RCA prompt into chat_sessions.messages
immediately, which bypasses the input-rail safety check; instead, stop
persisting the prompt here and only persist after the rail has been validated.
Remove or disable the eager write in the block around
db_pool.get_admin_connection()/set_rls_context and either (a) move the
persistence logic to the existing workflow save path (the code in
server/chat/backend/agent/workflow.py that writes user text after rail_text
passes), or (b) add a guard that explicitly verifies the rail has passed for
session_id (e.g., query workflow/rail status) before updating
chat_sessions.messages; reference DISPLAY__RCA_USER_MSG, initial_message,
chat_sessions messages, get_admin_connection, and set_rls_context when making
the change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a229749-e9a9-496c-a343-d31766d2f358
📒 Files selected for processing (5)
.env.exampleclient/src/components/tool-calls/ToolExecutionWidget.tsxserver/chat/backend/agent/tools/alert_payload_tool.pyserver/chat/background/rca_prompt_builder.pyserver/chat/background/task.py
|
damianloch
approved these changes
Jun 1, 2026
4 tasks
6 tasks
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.
Make one consolidated build rca prompt for the user message which just uses the raw webhook instead of parsing it and potentially losing important info
Summary by CodeRabbit
New Features
Improvements
Behavior