Feature/opensearch victorops connectors - #410
Open
Apok21 wants to merge 5 commits into
Open
Conversation
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:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (35)
WalkthroughAdds OpenSearch and VictorOps as connected-account providers with registration infrastructure, backend HTTP clients and Flask route blueprints, client-side proxy routes and typed service clients, authentication UI pages with state management, webhook ingestion with Celery task processing, and RCA agent tool integration with skill documentation. ChangesOpenSearch and VictorOps Integration
Sequence DiagramsequenceDiagram
participant Frontend as Frontend (Browser)
participant NextAPI as Next.js API Routes
participant FlaskBackend as Flask Backend
participant OpenSearchClient as OpenSearch Server
participant VictorOpsAPI as VictorOps API
participant Celery as Celery Task Queue
participant PostgreSQL as PostgreSQL
Frontend->>NextAPI: POST /api/opensearch/connect
NextAPI->>FlaskBackend: forward to /opensearch/connect
FlaskBackend->>OpenSearchClient: probe cluster health
FlaskBackend->>PostgreSQL: store credentials
Frontend->>NextAPI: POST /api/victorops/webhook/user123
NextAPI->>FlaskBackend: forward to /victorops/webhook/user123
FlaskBackend->>Celery: enqueue process_victorops_event task
Celery->>PostgreSQL: upsert incident, persist event
Celery->>PostgreSQL: write lifecycle events, broadcast SSE
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 23
🤖 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/app/api/connected-accounts/`[provider]/route.ts:
- Around line 505-543: Replace the direct fetch calls in the provider ===
'opensearch' and provider === 'victorops' branches with the shared proxy helper
forwardRequest from "`@/lib/backend-proxy`": call forwardRequest with the same
backend path (e.g. "/opensearch/disconnect" and "/victorops"), method "DELETE"
and propagate authHeaders, then handle the returned Response the same way you do
now (read body or text on non-ok, log backend error, and return
NextResponse.json with the response body and the original response status).
Update the two branches in route.ts to use forwardRequest instead of fetch to
ensure consistent auth/header behavior.
In `@client/src/app/api/opensearch/`[...path]/route.ts:
- Line 37: The code directly calls fetch(url, options) and must instead use the
shared proxy utility; import forwardRequest from '`@/lib/backend-proxy`' and
replace the direct call to fetch in the route handler with await
forwardRequest(url, options) (preserving method, headers, body and any
auth/internal-header behavior), then use the returned Response the same way you
did with fetch; ensure the import for forwardRequest is added at the top and
remove the direct fetch usage so backend calls go through the proxy contract.
In `@client/src/app/api/victorops/route.ts`:
- Line 35: Replace the direct fetch call to `${API_BASE_URL}/victorops` with the
centralized proxy helper forwardRequest from '`@/lib/backend-proxy`': import
forwardRequest and call forwardRequest('/victorops', options) (or
forwardRequest({ path: '/victorops', ...options }) depending on the helper
signature) instead of fetch so the request goes through the backend-proxy flow
and preserves required headers/auth; update the code that reads `response` to
handle the Response returned by forwardRequest exactly as before.
In `@client/src/app/api/victorops/webhook-url/route.ts`:
- Around line 16-21: Replace the direct fetch to BACKEND_URL with the
centralized proxy helper: import forwardRequest from '`@/lib/backend-proxy`' and
call it to forward the GET to '/victorops/webhook-url' (do not use BACKEND_URL).
Pass the original request context and preserve headers/auth (authHeaders),
method 'GET', credentials and cache policy (no-store) via forwardRequest's
options so the call uses the shared proxy behavior instead of a raw fetch.
In `@client/src/app/opensearch/auth/page.tsx`:
- Around line 66-67: The code currently emits a generic
CustomEvent("providerStateChanged"); update both the connect and disconnect
paths in client/src/app/opensearch/auth/page.tsx (the places invoking
window.dispatchEvent, e.g., the connect/disconnect handlers) to dispatch a
provider-scoped event instead — replace the CustomEvent and/or generic name with
window.dispatchEvent(new Event('openSearchStateChanged')) (or the exact provider
event name your revalidation expects) at both locations (including the other
occurrence around lines 93-94) so listeners tied to the OpenSearch provider
receive the state change.
In `@client/src/app/victorops/auth/page.tsx`:
- Line 34: Replace the generic CustomEvent dispatch call that emits
'providerStateChanged' with a provider-scoped Event named
'victorOpsStateChanged' so provider-specific revalidation hooks run; locate the
window.dispatchEvent(new CustomEvent('providerStateChanged')) call in page.tsx
and change it to use new Event('victorOpsStateChanged') per the frontend
disconnect action guideline.
- Around line 40-42: The code currently stores the entire VictorOpsStatus under
CACHE_KEY; instead serialize and persist a minimal non-sensitive CachedStatus
containing only the fields needed for immediate UI hydration (e.g. id, status,
lastUpdated or similar) rather than the full VictorOpsStatus object, omit any
tokens/account/user metadata, and continue to trigger a background revalidation
to fetch the live VictorOpsStatus. Update the localStorage writes around
CACHE_KEY (both the block using result and the similar write at lines ~49-50) to
map result -> CachedStatus before JSON.stringify, and update any consumers to
hydrate from CachedStatus then replace with the live fetch.
In `@client/src/components/victorops/VictorOpsWebhookStep.tsx`:
- Around line 146-156: The copy Button in VictorOpsWebhookStep is icon-only and
missing an accessible label; update the Button component (inside
VictorOpsWebhookStep where copied, handleCopy, and webhookUrl are used) to
include an aria-label (and optional title) that describes the action, e.g.
aria-label={copied ? "Copied webhook URL" : "Copy webhook URL"} (or similar
dynamic text) while keeping the existing onClick behavior so screen readers can
discover the control.
In `@client/src/lib/services/victorops.ts`:
- Around line 39-41: The disconnect method in the VictorOps service currently
only calls apiRequest; after the delete completes you must dispatch the provider
state-change event so connectors refresh immediately—update the async
disconnect() function (the disconnect symbol in victorops service) to call
window.dispatchEvent(new Event('victoropsStateChanged')) after the successful
apiRequest resolution (and only after no error) so the provider state-change is
emitted.
In `@server/celery_config.py`:
- Around line 174-179: The logging.warning call in the VictorOps import except
block uses an f-string which eagerly interpolates and triggers Ruff G004; change
it to use percent-style lazy formatting so the exception is only formatted if
the log level is enabled (update the logging.warning call in the try/except
around import routes.victorops.tasks to logging.warning("Failed to import
VictorOps tasks: %s", e)). Ensure similar logging calls follow this pattern
(e.g., logging.info) if applicable.
In `@server/chat/backend/agent/tools/opensearch_tool.py`:
- Around line 128-158: The exception logging in list_opensearch_indices
currently includes the full exception (logger.warning and the f"Failed to list
OpenSearch indices: {exc}" return), which can leak endpoints/credentials; change
the logger call to avoid printing the exception details (e.g. log only a
sanitized message and the exception type via type(exc).__name__ or omit exc
entirely) and replace the returned string so it does not include exc details
(return a generic "Failed to list OpenSearch indices" message). Update
references in the list_opensearch_indices function (logger.warning and the
failure return) and ensure consistency with how search_opensearch handles
errors.
- Around line 64-122: The search_opensearch function currently surfaces full
exception text that may include the OpenSearch endpoint (via the
OpenSearchClient error message); change the exception handling around
_get_client and client.search in search_opensearch so that any logged/returned
error message is generic (e.g., "Error connecting to OpenSearch" or "OpenSearch
query failed") and does NOT include the client's endpoint or other
infra/credential details; keep the original exception text available only for
internal debugging if needed (e.g., store it in a debug log guarded by a
non-production flag or remove it entirely) and ensure references to _get_client,
OpenSearchClient, and client.search are updated accordingly.
In `@server/chat/backend/agent/tools/victorops_tool.py`:
- Around line 88-110: The get_victorops_teams function currently calls
logger.exception which emits full stack traces and may leak sensitive values
like api_id/api_key; change the exception handling in get_victorops_teams (and
mirror the same pattern used in get_victorops_incidents) to avoid logging the
traceback or any auth details by logging a generic error message and a safely
truncated/escaped exception string (e.g., logger.error or logger.warning without
exc_info=True, and include only str(exc)[:N]) so no stack or request payloads
are recorded; ensure any error logging does not include client attributes or
kwargs that could contain credentials.
- Around line 54-85: Remove the unused import VictorOpsAPIError from
get_victorops_incidents and replace the logger.exception call with a
non-stack-trace log to avoid exposing VictorOpsClient credentials: call
logger.warning (or logger.error without exc_info) with a sanitized message
referencing the function name and user_id (no client object, no stack trace, and
no full exception/credential content), and return the same safe json error
payload; use _get_client only to check connectivity but never include its
internals in logs.
In `@server/chat/background/rca_prompt_builder.py`:
- Around line 936-977: The function build_victorops_rca_prompt is annotated to
return str but currently returns whatever build_rca_prompt returns (a tuple),
causing callers expecting a string to receive a tuple; fix by calling
build_rca_prompt and returning only the string message (e.g., unpack or index
the first element) so build_victorops_rca_prompt actually returns a str, and
keep the signature typed as str; reference build_victorops_rca_prompt and
build_rca_prompt when making this change.
In `@server/connectors/opensearch_connector/client.py`:
- Around line 66-67: The _request method currently returns resp.json() directly
which can raise ValueError on empty or non-JSON bodies and escape as an
unexpected exception; update the _request implementation to catch JSON decode
errors (ValueError or json.JSONDecodeError) around resp.json(), and convert them
into an OpenSearchError (same as other error flows) so callers get a consistent
OpenSearchError contract; reference the _request function, the resp.json() call,
and the OpenSearchError exception class when making the change, and ensure
existing except blocks for requests.exceptions.ConnectTimeout remain unchanged.
In `@server/main_compute.py`:
- Line 223: The VictorOps webhook route was added to _OPEN_PREFIXES and
currently skips X-Internal-Secret checks, allowing forged events; update the
VictorOps webhook handler to perform HMAC-based signature verification using the
per-user webhook secret stored with the credentials: retrieve the secret for the
provided user_id (same place you check credentials), compute the HMAC of the raw
request body (use a fixed algorithm like SHA256), compare it against the
incoming signature header (e.g. X-Splunk-Signature or a configured header) using
a timing-safe compare, and return 401/400 on missing or mismatched signatures;
keep the route in _OPEN_PREFIXES but ensure the new verification runs before
processing events and add concise error logging on failure.
In `@server/routes/connector_status.py`:
- Around line 519-534: The current _check_opensearch and _check_victorops only
validate presence of secrets and must be changed to perform a lightweight live
auth check: update _check_opensearch to attempt a minimal OpenSearch client/auth
call (e.g., ping or info) using creds["endpoint"], creds["username"],
creds["password"] and return {"connected": True, "endpoint": endpoint} on
success or {"connected": False, "error": "<short message>"} on failure; likewise
replace _check_victorops to call the VictorOps/Splunk On-Call auth/heartbeat
endpoint using creds["api_id"] and creds["api_key"] and return connected
True/False based on the HTTP/auth response, catching network/auth exceptions and
returning connected False with a concise error note; keep function names
(_check_opensearch, _check_victorops) and surface only minimal info to avoid
leaking secrets.
In `@server/routes/opensearch/opensearch_routes.py`:
- Around line 67-72: The logger.info call is logging credential data — remove
the username from the log message by editing the logger.info invocation that
currently references sanitize(user_id), sanitize(endpoint), and
sanitize(username); change the format string to only include user_id and
endpoint (and keep sanitize(user_id) and sanitize(endpoint) as the only
arguments), and scan nearby code for any other uses of sanitize(username) in
logs and remove them to ensure no credentials are written to logs.
- Around line 54-56: The request param parsing currently misinterprets string
booleans and can raise ValueError on int(...) leading to 500s; add small
input-parsing helpers (e.g., parse_bool_param(value, default) and
parse_int_param(value, default)) and use them for the verify_ssl and max_retries
assignments and the other occurrence at lines ~189-190: parse_bool_param should
accept bools and common truthy/falsey strings
("true","false","1","0","yes","no") case-insensitively and return the default
for unrecognized values; parse_int_param should try int() in a try/except and
return the default if conversion fails (or optionally return a 400-friendly
error upstream), then replace direct bool(...) and int(...) calls with these
helpers (referencing verify_ssl, max_retries and the same param names used
later).
In `@server/routes/victorops/tasks.py`:
- Around line 367-369: The current check only uses alert_phase == "TRIGGERED" so
repeated TRIGGERED events re-enqueue summary/RCA jobs; change the block around
alert_phase == "TRIGGERED" to only schedule when the incident is actually new by
adding a dedupe guard: verify a stable indicator (e.g., incident_id) has not
already had a summary/RCA scheduled before enqueuing — implement/use helper
functions like has_scheduled_summary(incident_id) and
mark_summary_scheduled(incident_id) (or check a persistent incident flag such as
incident.get("summary_scheduled")/incident["first_triggered"]) immediately
before the enqueue call so the job is only created once per incident, and set
the marker after scheduling.
In `@server/routes/victorops/victorops_routes.py`:
- Around line 100-105: The code builds webhook_url from base_url but does not
handle the case where both NGROK_URL and NEXT_PUBLIC_BACKEND_URL are empty,
producing an invalid relative URL; update the logic in victorops_routes.py (the
ngrok_url, backend_url, base_url, webhook_url logic around user_id) to validate
base_url after selection and, if empty or falsy, raise/return a clear
configuration error (e.g., throw ValueError or return a 500/400 config error
response) instead of producing /victorops/webhook/<user_id>, and include a
descriptive message indicating the missing NGROK_URL/NEXT_PUBLIC_BACKEND_URL
configuration.
- Around line 182-186: The metadata currently includes dict(request.headers)
which can leak sensitive headers; update the metadata passed to
process_victorops_event.delay so it does not forward full request.headers — keep
request.remote_addr but replace headers with a minimal, whitelisted subset
(e.g., select "User-Agent", "Accept", "Content-Type") or an empty dict, and
construct that filtered_headers before calling process_victorops_event.delay
(search for metadata and the call to process_victorops_event.delay in
victorops_routes.py to locate 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: 7c0d2ba2-d928-4be6-a196-64bbabd7bd74
⛔ Files ignored due to path filters (2)
client/public/opensearch.svgis excluded by!**/*.svgclient/public/victorops.svgis excluded by!**/*.svg
📒 Files selected for processing (32)
client/src/app/api/connected-accounts/[provider]/route.tsclient/src/app/api/opensearch/[...path]/route.tsclient/src/app/api/victorops/route.tsclient/src/app/api/victorops/webhook-url/route.tsclient/src/app/opensearch/auth/page.tsxclient/src/app/victorops/auth/page.tsxclient/src/components/connectors/ConnectorRegistry.tsclient/src/components/victorops/VictorOpsConnectedView.tsxclient/src/components/victorops/VictorOpsConnectionStep.tsxclient/src/components/victorops/VictorOpsWebhookStep.tsxclient/src/lib/services/opensearch.tsclient/src/lib/services/victorops.tsserver/celery_config.pyserver/chat/backend/agent/skills/integrations/opensearch/SKILL.mdserver/chat/backend/agent/skills/integrations/victorops/SKILL.mdserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/backend/agent/tools/opensearch_tool.pyserver/chat/backend/agent/tools/victorops_tool.pyserver/chat/background/rca_prompt_builder.pyserver/connectors/opensearch_connector/__init__.pyserver/connectors/opensearch_connector/client.pyserver/main_compute.pyserver/routes/connector_status.pyserver/routes/opensearch/__init__.pyserver/routes/opensearch/opensearch_routes.pyserver/routes/victorops/__init__.pyserver/routes/victorops/tasks.pyserver/routes/victorops/victorops_helpers.pyserver/routes/victorops/victorops_routes.pyserver/utils/db/db_utils.pyserver/utils/providers.pyserver/utils/secrets/secret_ref_utils.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
server/routes/connector_status.py (1)
519-545:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not report failed live probes as
connected=True.Both helpers still return
connected=Truefor timeouts, DNS/TLS failures, 5xxs, and a mistyped OpenSearch endpoint because only401/403force a failure. That breaks this endpoint’s live-status contract and will surface unusable connectors as available to the frontend and connected-count logic.Suggested fix
def _check_opensearch(creds: Dict[str, Any]) -> Dict[str, Any]: @@ - except Exception: - pass - # Network unreachable — fall back to credential existence (validated at connect time) - return {"connected": True, "endpoint": endpoint} + except Exception: + return {"connected": False} + return {"connected": False} @@ def _check_victorops(creds: Dict[str, Any]) -> Dict[str, Any]: @@ - except Exception: - pass - # Network unreachable — fall back to credential existence (validated at connect time) - return {"connected": True} + except Exception: + return {"connected": False} + return {"connected": False}Also applies to: 548-571
🤖 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/routes/connector_status.py` around lines 519 - 545, The _check_opensearch helper currently treats timeouts/DNS/TLS errors and non-401/403 non-OK responses as "connected": True; change it so that only a successful r.ok returns {"connected": True, "endpoint": endpoint} and all other outcomes (r.status_code not ok, 401/403, or any Exception) return {"connected": False}; remove the current fallback that returns True on exceptions. Apply the same change to the sibling helper referenced around lines 548-571 (the equivalent live-probe function) so failed live probes are never reported as connected.
🤖 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/app/opensearch/auth/page.tsx`:
- Around line 64-70: The code always writes localStorage key
"isOpenSearchConnected" as the literal string "true" causing mismatch with the
actual connect result; change the write to reflect result.connected (e.g., set
"isOpenSearchConnected" to String(result.connected) or
JSON.stringify(result.connected)) and ensure the object saved under CACHE_KEY
also uses result.connected so both localStorage entries match; keep the existing
window.dispatchEvent calls (openSearchStateChanged/providerStateChanged), toast
and setPassword untouched.
In `@server/chat/backend/agent/tools/victorops_tool.py`:
- Around line 14-15: The GetVictorOpsIncidentsArgs schema currently allows any
integer for limit; add validation to the Field (e.g., use ge=1, le=100) on the
limit attribute and also clamp/validate the value at runtime before calling
client.get_incidents (e.g., ensure limit = max(1, min(limit, 100))) to avoid
passing negative or out-of-range values and prevent invalid slicing like
incidents[:-1]; update both the schema declaration for limit and the code path
that calls client.get_incidents and slices incidents.
- Around line 72-81: The current logic only halves the incidents once and may
still exceed MAX_OUTPUT_SIZE; update the trimming logic around MAX_OUTPUT_SIZE
and json.dumps so you repeatedly reduce the payload until it fits: repeatedly
serialize incidents (results_str = json.dumps(incidents)) and if its length >
MAX_OUTPUT_SIZE, remove items (e.g., pop from the end) or progressively reduce
each incident's heavy fields until json.dumps(...) <= MAX_OUTPUT_SIZE, ensuring
you keep at least one incident (use max(1, ...)) and update the returned "count"
and "results" accordingly; reference the variables/functionality around
incidents, results_str, MAX_OUTPUT_SIZE, limit and the return json.dumps block
when making the change.
In `@server/connectors/opensearch_connector/client.py`:
- Around line 160-169: The constructor currently only does endpoint.rstrip("/")
and skips validation; call the static method normalize_endpoint(raw: str) from
the class constructor (instead of endpoint.rstrip("/")) so the constructor uses
the same logic that enforces scheme and netloc and raises ValueError for invalid
endpoints; locate the class constructor in client.py and replace the simple
strip with a call to self.normalize_endpoint(...) (or
ClassName.normalize_endpoint(...)) so all client instances consistently validate
endpoints on creation.
In `@server/routes/opensearch/opensearch_routes.py`:
- Line 13: Remove the unused import by deleting hash_for_log from the import
statement that currently imports sanitize and hash_for_log from
utils.log_sanitizer; keep sanitize if used elsewhere (e.g., in this file's
functions) and ensure only used symbols remain imported to avoid lint errors
referencing hash_for_log.
In `@server/routes/victorops/tasks.py`:
- Around line 431-432: In the except block that currently does "except Exception
as exc: raise self.retry(exc=exc)" (inside the Celery task using self.retry),
change the re-raise to use exception chaining by raising the retry from the
original exception: use "raise self.retry(exc=exc) from exc" so the original
exception is preserved and satisfies B904; update the except block surrounding
the retry call (the one referencing self.retry) accordingly.
In `@server/routes/victorops/victorops_routes.py`:
- Around line 129-137: The webhook secret comparison in the VictorOps route uses
a simple != comparison between provided and webhook_secret which is vulnerable
to timing attacks; replace the direct comparison with a constant-time comparison
using hmac.compare_digest (import hmac if needed) to compare provided and
webhook_secret (the variables referenced in this block) before returning the 403
in the branch inside the code that reads request.headers.get("X-Webhook-Secret",
""); keep the same logging and response behavior but use hmac.compare_digest for
the equality check.
---
Duplicate comments:
In `@server/routes/connector_status.py`:
- Around line 519-545: The _check_opensearch helper currently treats
timeouts/DNS/TLS errors and non-401/403 non-OK responses as "connected": True;
change it so that only a successful r.ok returns {"connected": True, "endpoint":
endpoint} and all other outcomes (r.status_code not ok, 401/403, or any
Exception) return {"connected": False}; remove the current fallback that returns
True on exceptions. Apply the same change to the sibling helper referenced
around lines 548-571 (the equivalent live-probe function) so failed live probes
are never reported as connected.
🪄 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: 669cb0b4-cb1a-4355-befe-75f10f95d25d
📒 Files selected for processing (18)
.env.exampleclient/src/app/api/connected-accounts/[provider]/route.tsclient/src/app/api/opensearch/[...path]/route.tsclient/src/app/api/victorops/route.tsclient/src/app/api/victorops/webhook-url/route.tsclient/src/app/opensearch/auth/page.tsxclient/src/app/victorops/auth/page.tsxclient/src/components/victorops/VictorOpsWebhookStep.tsxdocker-compose.prod-local.ymldocker-compose.yamlserver/celery_config.pyserver/chat/backend/agent/tools/opensearch_tool.pyserver/chat/backend/agent/tools/victorops_tool.pyserver/connectors/opensearch_connector/client.pyserver/routes/connector_status.pyserver/routes/opensearch/opensearch_routes.pyserver/routes/victorops/tasks.pyserver/routes/victorops/victorops_routes.py
Apok21
force-pushed
the
feature/opensearch-victorops-connectors
branch
from
May 17, 2026 11:48
3ea97ad to
3fb832a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
client/src/lib/services/victorops.ts (1)
39-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit
victoropsStateChangedafter successful disconnect.
disconnect()does not notify listeners after token removal, so event-driven revalidation can stay stale.Suggested fix
async disconnect(): Promise<void> { await apiRequest(API_BASE, { method: 'DELETE', cache: 'no-store' }); + if (typeof window !== 'undefined') { + window.dispatchEvent(new Event('victoropsStateChanged')); + } },As per coding guidelines: "Connector disconnect must trigger
window.dispatchEvent(new Event('<name>StateChanged'))to notify listeners".🤖 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 `@client/src/lib/services/victorops.ts` around lines 39 - 41, The disconnect method currently calls apiRequest(API_BASE, { method: 'DELETE', cache: 'no-store' }) but does not notify listeners; after the awaited API call in the disconnect() function (i.e., once apiRequest resolves successfully), dispatch window.dispatchEvent(new Event('victoropsStateChanged')) so consumers can revalidate; ensure the dispatch happens only after the await completes (and not on error).
🤖 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/app/victorops/auth/page.tsx`:
- Around line 50-57: The cached status read using
localStorage.getItem(CACHE_KEY) can throw on JSON.parse which prevents the
subsequent await fetchAndUpdateStatus() from running; wrap the JSON.parse/cached
handling in a try/catch (or otherwise guard parsing) inside the block that
checks skipCache and globalThis.window, call setStatus(minimal) only on
successful parse, and in the catch log or remove the corrupted CACHE_KEY entry
so the code still always reaches and executes fetchAndUpdateStatus()
(references: CACHE_KEY, VictorOpsStatus, setStatus, fetchAndUpdateStatus,
skipCache).
In `@server/chat/backend/agent/skills/integrations/opensearch/SKILL.md`:
- Line 23: The Markdown headings in SKILL.md (e.g., the "Overview" heading and
the other top-level headings noted in the comment) are missing a blank line
beneath them which triggers markdownlint MD022; edit SKILL.md and insert a
single blank line immediately after each heading (for example after "Overview"
and the other headings referenced) so every heading is followed by an empty
line, then re-run linting.
In `@server/chat/backend/agent/skills/integrations/victorops/SKILL.md`:
- Around line 23-39: In SKILL.md add a single blank line immediately after each
section heading to satisfy markdownlint MD022: after the "Overview" heading and
after the "Instructions" subheadings including "Tool Usage", "RCA Workflow", and
the "Context to gather" line so the file renders correctly; locate the headings
near the get_victorops_incidents and get_victorops_teams descriptions and insert
one empty line below each heading.
In `@server/chat/backend/agent/tools/cloud_tools.py`:
- Around line 1779-1781: Replace f-string logging with parameterized logging to
avoid Ruff G004: change logging.info(f"Added 2 OpenSearch tools for user
{user_id}") to logging.info("Added 2 OpenSearch tools for user %s", user_id) and
logging.debug(f"OpenSearch tools not added - user {user_id} not connected to
OpenSearch") to logging.debug("OpenSearch tools not added - user %s not
connected to OpenSearch", user_id); also find the similar f-string usage later
(around the other logging call flagged) and convert it the same way so all
logging calls use "%s" placeholders and pass variables as additional arguments
instead of f-strings.
In `@server/chat/backend/agent/tools/victorops_tool.py`:
- Around line 39-47: The _get_client function assumes creds contains "api_id"
and "api_key" and will raise KeyError on malformed data; update _get_client to
validate that creds is truthy and contains both keys (or use creds.get(...) and
check) and return None if either key is missing so callers keep the normal error
path; apply the same defensive check to the two other locations in this file
where VictorOpsClient(api_id=..., api_key=...) is constructed (the similar
blocks around the other VictorOps client creations) to avoid KeyError on partial
credential records.
In `@server/routes/connector_status.py`:
- Around line 519-546: The _check_opensearch function currently returns
connected: True on exceptions and on non-OK responses (except 401/403), causing
false positives; update it so only a successful HTTP OK (r.ok) returns
{"connected": True, "endpoint": endpoint} and any exception or any non-OK status
(including network errors) returns {"connected": False}; specifically, change
the except block to return {"connected": False} and after the request treat any
non-OK status as connected: False (only 200-level responses should yield
connected True). Apply the same fix to the analogous checker in the 548-572
range so both live-check functions only report connected when r.ok.
In `@server/routes/opensearch/opensearch_routes.py`:
- Around line 29-37: Validate that creds contains the required keys before
constructing the OpenSearchClient in _make_client: check for "endpoint",
"username", and "password" (and any other required keys) and if any are missing
raise a clear, catchable exception (e.g., ValueError("missing opensearch
credentials: ...")) or return None so callers can respond with a controlled "not
connected" response; update callers that call _make_client (e.g., the functions
that perform search and indices operations) to catch this exception/None and
return the expected "not connected" result instead of letting a KeyError bubble
up.
In `@server/routes/victorops/tasks.py`:
- Around line 95-118: The _record_primary_alert function currently commits after
two SQL statements but does not rollback on failure; update the except block in
_record_primary_alert to call conn.rollback() before logging/returning so the
connection is returned to a clean state when either cursor.execute call fails;
reference the existing variables cursor and conn and keep the existing
logger.warning call after the rollback.
- Around line 208-240: The except block in _run_correlation_check (around
AlertCorrelator.correlate and handle_correlated_alert) swallows DB exceptions
but does not roll back the connection, leaving the transaction aborted; update
the except handler to call conn.rollback() (before logging/returning) so the DB
transaction is reset, then continue to return False as before (use the existing
corr_exc variable and ensure conn.rollback() is invoked prior to the
logger.warning and return).
In `@server/routes/victorops/victorops_routes.py`:
- Around line 29-31: Replace the direct user-only token lookup
(get_token_data(user_id, "victorops")) with the connector's standard user-first
then org-fallback lookup by calling the shared helper for VictorOps credentials
(e.g., _get_stored_victorops_credentials or the module's equivalent) so it
checks user token first and then looks up an org-scoped active credential
(org_id, provider="victorops", is_active=TRUE, secret_ref IS NOT NULL); make the
same replacement for the other occurrence referenced (lines ~140-146) so both
status and webhook validation use the two-step lookup.
---
Duplicate comments:
In `@client/src/lib/services/victorops.ts`:
- Around line 39-41: The disconnect method currently calls apiRequest(API_BASE,
{ method: 'DELETE', cache: 'no-store' }) but does not notify listeners; after
the awaited API call in the disconnect() function (i.e., once apiRequest
resolves successfully), dispatch window.dispatchEvent(new
Event('victoropsStateChanged')) so consumers can revalidate; ensure the dispatch
happens only after the await completes (and not on error).
🪄 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: 3c690484-2980-4a54-a156-7643a7e0693b
⛔ Files ignored due to path filters (2)
client/public/opensearch.svgis excluded by!**/*.svgclient/public/victorops.svgis excluded by!**/*.svg
📒 Files selected for processing (35)
.env.exampleclient/src/app/api/connected-accounts/[provider]/route.tsclient/src/app/api/opensearch/[...path]/route.tsclient/src/app/api/victorops/route.tsclient/src/app/api/victorops/webhook-url/route.tsclient/src/app/opensearch/auth/page.tsxclient/src/app/victorops/auth/page.tsxclient/src/components/connectors/ConnectorRegistry.tsclient/src/components/victorops/VictorOpsConnectedView.tsxclient/src/components/victorops/VictorOpsConnectionStep.tsxclient/src/components/victorops/VictorOpsWebhookStep.tsxclient/src/lib/services/opensearch.tsclient/src/lib/services/victorops.tsdocker-compose.prod-local.ymldocker-compose.yamlserver/celery_config.pyserver/chat/backend/agent/skills/integrations/opensearch/SKILL.mdserver/chat/backend/agent/skills/integrations/victorops/SKILL.mdserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/backend/agent/tools/opensearch_tool.pyserver/chat/backend/agent/tools/victorops_tool.pyserver/chat/background/rca_prompt_builder.pyserver/connectors/opensearch_connector/__init__.pyserver/connectors/opensearch_connector/client.pyserver/main_compute.pyserver/routes/connector_status.pyserver/routes/opensearch/__init__.pyserver/routes/opensearch/opensearch_routes.pyserver/routes/victorops/__init__.pyserver/routes/victorops/tasks.pyserver/routes/victorops/victorops_helpers.pyserver/routes/victorops/victorops_routes.pyserver/utils/db/db_utils.pyserver/utils/providers.pyserver/utils/secrets/secret_ref_utils.py
Comment on lines
+50
to
+57
| if (!skipCache && globalThis.window !== undefined) { | ||
| const cached = localStorage.getItem(CACHE_KEY); | ||
| if (cached) { | ||
| const minimal = JSON.parse(cached) as VictorOpsStatus; | ||
| setStatus(minimal); | ||
| } | ||
| } | ||
| await fetchAndUpdateStatus(); |
Contributor
There was a problem hiding this comment.
Handle corrupted cached status without blocking live refresh.
If localStorage contains malformed JSON, JSON.parse throws and Line 57 never runs, so live status fetch is skipped. This leaves the page stuck on stale state until cache is manually cleared.
Suggested fix
if (!skipCache && globalThis.window !== undefined) {
const cached = localStorage.getItem(CACHE_KEY);
if (cached) {
- const minimal = JSON.parse(cached) as VictorOpsStatus;
- setStatus(minimal);
+ try {
+ const minimal = JSON.parse(cached) as Partial<VictorOpsStatus>;
+ if (typeof minimal.connected === "boolean") {
+ setStatus({ connected: minimal.connected });
+ } else {
+ localStorage.removeItem(CACHE_KEY);
+ }
+ } catch {
+ localStorage.removeItem(CACHE_KEY);
+ }
}
}
await fetchAndUpdateStatus();📝 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
| if (!skipCache && globalThis.window !== undefined) { | |
| const cached = localStorage.getItem(CACHE_KEY); | |
| if (cached) { | |
| const minimal = JSON.parse(cached) as VictorOpsStatus; | |
| setStatus(minimal); | |
| } | |
| } | |
| await fetchAndUpdateStatus(); | |
| if (!skipCache && globalThis.window !== undefined) { | |
| const cached = localStorage.getItem(CACHE_KEY); | |
| if (cached) { | |
| try { | |
| const minimal = JSON.parse(cached) as Partial<VictorOpsStatus>; | |
| if (typeof minimal.connected === "boolean") { | |
| setStatus({ connected: minimal.connected }); | |
| } else { | |
| localStorage.removeItem(CACHE_KEY); | |
| } | |
| } catch { | |
| localStorage.removeItem(CACHE_KEY); | |
| } | |
| } | |
| } | |
| await fetchAndUpdateStatus(); |
🤖 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 `@client/src/app/victorops/auth/page.tsx` around lines 50 - 57, The cached
status read using localStorage.getItem(CACHE_KEY) can throw on JSON.parse which
prevents the subsequent await fetchAndUpdateStatus() from running; wrap the
JSON.parse/cached handling in a try/catch (or otherwise guard parsing) inside
the block that checks skipCache and globalThis.window, call setStatus(minimal)
only on successful parse, and in the catch log or remove the corrupted CACHE_KEY
entry so the code still always reaches and executes fetchAndUpdateStatus()
(references: CACHE_KEY, VictorOpsStatus, setStatus, fetchAndUpdateStatus,
skipCache).
|
|
||
| # OpenSearch Integration | ||
|
|
||
| ## Overview |
Contributor
There was a problem hiding this comment.
Add blank lines after headings to satisfy markdownlint (MD022).
These headings are missing a blank line below them, which triggers lint warnings.
Proposed fix
## Overview
+
OpenSearch integration for querying log data during Root Cause Analysis. OpenSearch is a REMOTE service — do NOT search the local filesystem. Use ONLY the tools listed below.
@@
### Tool Usage (use in this order)
+
1. `list_opensearch_indices()` — Discover available indices. Call first to understand what data exists.
@@
### Common Query Patterns
+
- Error search: `search_opensearch(query='error AND service:api', start_time='now-1h')`
@@
### Time Format
+
- Relative: `now-1h`, `now-30m`, `now-6h`, `now-1d`
@@
## Important Rules
+
- OpenSearch is a REMOTE service. Never try to access data from the local filesystem.Also applies to: 28-28, 32-32, 39-39, 57-57
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 23-23: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 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/skills/integrations/opensearch/SKILL.md` at line
23, The Markdown headings in SKILL.md (e.g., the "Overview" heading and the
other top-level headings noted in the comment) are missing a blank line beneath
them which triggers markdownlint MD022; edit SKILL.md and insert a single blank
line immediately after each heading (for example after "Overview" and the other
headings referenced) so every heading is followed by an empty line, then re-run
linting.
Comment on lines
+1779
to
+1781
| logging.info(f"Added 2 OpenSearch tools for user {user_id}") | ||
| else: | ||
| logging.debug(f"OpenSearch tools not added - user {user_id} not connected to OpenSearch") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining G004-style patterns in this file.
rg -n 'logging\.(debug|info|warning|error|critical)\(f"' server/chat/backend/agent/tools/cloud_tools.pyRepository: Arvo-AI/aurora
Length of output: 12775
Use lazy logging placeholders instead of f-strings.
Lines 1779, 1781, and 1986 use f-string logging, which triggers Ruff G004 and may fail lint checks. Convert to %s-style parameterized logging.
Proposed fix
- logging.info(f"Added 2 OpenSearch tools for user {user_id}")
+ logging.info("Added 2 OpenSearch tools for user %s", user_id)
else:
- logging.debug(f"OpenSearch tools not added - user {user_id} not connected to OpenSearch")
+ logging.debug(
+ "OpenSearch tools not added - user %s not connected to OpenSearch",
+ user_id,
+ )
@@
- logging.info(f"Added Splunk On-Call (VictorOps) tools for user {user_id}")
+ logging.info("Added Splunk On-Call (VictorOps) tools for user %s", user_id)📝 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
| logging.info(f"Added 2 OpenSearch tools for user {user_id}") | |
| else: | |
| logging.debug(f"OpenSearch tools not added - user {user_id} not connected to OpenSearch") | |
| logging.info("Added 2 OpenSearch tools for user %s", user_id) | |
| else: | |
| logging.debug( | |
| "OpenSearch tools not added - user %s not connected to OpenSearch", | |
| user_id, | |
| ) |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 1779-1779: Logging statement uses f-string
(G004)
[warning] 1781-1781: Logging statement uses f-string
(G004)
🤖 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/cloud_tools.py` around lines 1779 - 1781,
Replace f-string logging with parameterized logging to avoid Ruff G004: change
logging.info(f"Added 2 OpenSearch tools for user {user_id}") to
logging.info("Added 2 OpenSearch tools for user %s", user_id) and
logging.debug(f"OpenSearch tools not added - user {user_id} not connected to
OpenSearch") to logging.debug("OpenSearch tools not added - user %s not
connected to OpenSearch", user_id); also find the similar f-string usage later
(around the other logging call flagged) and convert it the same way so all
logging calls use "%s" placeholders and pass variables as additional arguments
instead of f-strings.
Comment on lines
+39
to
+47
| def _get_client(user_id: str): | ||
| from utils.auth.token_management import get_token_data | ||
| from routes.victorops.victorops_helpers import VictorOpsClient | ||
|
|
||
| creds = get_token_data(user_id, "victorops") | ||
| if not creds: | ||
| return None | ||
| return VictorOpsClient(api_id=creds["api_id"], api_key=creds["api_key"]) | ||
|
|
Contributor
There was a problem hiding this comment.
Harden _get_client against partial credential records.
_get_client assumes api_id/api_key exist; malformed stored data will raise KeyError before the tool returns its normal JSON error payload. Return None when either field is missing (or validate before indexing) so both callers stay on the expected error path.
Proposed fix
def _get_client(user_id: str):
from utils.auth.token_management import get_token_data
from routes.victorops.victorops_helpers import VictorOpsClient
creds = get_token_data(user_id, "victorops")
- if not creds:
+ if not creds or not creds.get("api_id") or not creds.get("api_key"):
return None
return VictorOpsClient(api_id=creds["api_id"], api_key=creds["api_key"])Also applies to: 62-64, 92-94
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 39-39: Missing return type annotation for private function _get_client
(ANN202)
🤖 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/victorops_tool.py` around lines 39 - 47, The
_get_client function assumes creds contains "api_id" and "api_key" and will
raise KeyError on malformed data; update _get_client to validate that creds is
truthy and contains both keys (or use creds.get(...) and check) and return None
if either key is missing so callers keep the normal error path; apply the same
defensive check to the two other locations in this file where
VictorOpsClient(api_id=..., api_key=...) is constructed (the similar blocks
around the other VictorOps client creations) to avoid KeyError on partial
credential records.
Comment on lines
+519
to
+546
| def _check_opensearch(creds: Dict[str, Any]) -> Dict[str, Any]: | ||
| """Validate OpenSearch connection. | ||
|
|
||
| Attempts a live cluster health call. Falls back to credential existence when | ||
| the cluster is unreachable from this network (e.g. behind a VPN). Only | ||
| returns connected=False for missing credentials or explicit 401/403 responses. | ||
| """ | ||
| endpoint = creds.get("endpoint") | ||
| username = creds.get("username") | ||
| password = creds.get("password") | ||
| if not endpoint or not username or not password: | ||
| return {"connected": False} | ||
| try: | ||
| r = requests.get( | ||
| f"{endpoint}/_cluster/health", | ||
| auth=(username, password), | ||
| timeout=HTTP_TIMEOUT, | ||
| verify=creds.get("verify_ssl", True), | ||
| ) | ||
| if r.status_code in (401, 403): | ||
| return {"connected": False} | ||
| if r.ok: | ||
| return {"connected": True, "endpoint": endpoint} | ||
| except Exception: | ||
| pass | ||
| # Network unreachable — fall back to credential existence (validated at connect time) | ||
| return {"connected": True, "endpoint": endpoint} | ||
|
|
Contributor
There was a problem hiding this comment.
Do not mark failed live checks as connected.
Both checkers currently return connected: true for any exception and for non-ok responses that are not 401/403. That creates false positives and breaks this endpoint’s “live connection status” contract.
Suggested fix
def _check_opensearch(creds: Dict[str, Any]) -> Dict[str, Any]:
@@
- try:
+ try:
r = requests.get(
@@
- if r.status_code in (401, 403):
+ if r.status_code in (401, 403):
return {"connected": False}
if r.ok:
return {"connected": True, "endpoint": endpoint}
- except Exception:
- pass
- # Network unreachable — fall back to credential existence (validated at connect time)
- return {"connected": True, "endpoint": endpoint}
+ return {"connected": False}
+ except (requests.exceptions.ConnectTimeout, requests.exceptions.ReadTimeout, requests.exceptions.ConnectionError):
+ # Optional fallback for private/VPN-only clusters
+ return {"connected": True, "endpoint": endpoint}
+ except Exception:
+ return {"connected": False}
@@
def _check_victorops(creds: Dict[str, Any]) -> Dict[str, Any]:
@@
- try:
+ try:
r = requests.get(
@@
if r.status_code in (401, 403):
return {"connected": False}
if r.ok:
return {"connected": True}
- except Exception:
- pass
- # Network unreachable — fall back to credential existence (validated at connect time)
- return {"connected": True}
+ return {"connected": False}
+ except (requests.exceptions.ConnectTimeout, requests.exceptions.ReadTimeout, requests.exceptions.ConnectionError):
+ return {"connected": True}
+ except Exception:
+ return {"connected": False}Also applies to: 548-572
🧰 Tools
🪛 Ruff (0.15.12)
[error] 542-543: try-except-pass detected, consider logging the exception
(S110)
[warning] 542-542: Do not catch blind exception: Exception
(BLE001)
🤖 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/routes/connector_status.py` around lines 519 - 546, The
_check_opensearch function currently returns connected: True on exceptions and
on non-OK responses (except 401/403), causing false positives; update it so only
a successful HTTP OK (r.ok) returns {"connected": True, "endpoint": endpoint}
and any exception or any non-OK status (including network errors) returns
{"connected": False}; specifically, change the except block to return
{"connected": False} and after the request treat any non-OK status as connected:
False (only 200-level responses should yield connected True). Apply the same fix
to the analogous checker in the 548-572 range so both live-check functions only
report connected when r.ok.
Comment on lines
+29
to
+37
| def _make_client(creds: Dict[str, Any]) -> OpenSearchClient: | ||
| return OpenSearchClient( | ||
| endpoint=creds["endpoint"], | ||
| username=creds["username"], | ||
| password=creds["password"], | ||
| index_pattern=creds.get("index_pattern", "*"), | ||
| verify_ssl=creds.get("verify_ssl", True), | ||
| max_retries=creds.get("max_retries", 2), | ||
| ) |
Contributor
There was a problem hiding this comment.
Guard against incomplete stored credentials before client construction.
_make_client indexes required fields directly; if token data is partially present, search/indices can fail with uncaught KeyError instead of returning a controlled “not connected” response. Validate required keys before _make_client (or inside it with a handled error path).
Also applies to: 186-188, 229-231
🤖 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/routes/opensearch/opensearch_routes.py` around lines 29 - 37, Validate
that creds contains the required keys before constructing the OpenSearchClient
in _make_client: check for "endpoint", "username", and "password" (and any other
required keys) and if any are missing raise a clear, catchable exception (e.g.,
ValueError("missing opensearch credentials: ...")) or return None so callers can
respond with a controlled "not connected" response; update callers that call
_make_client (e.g., the functions that perform search and indices operations) to
catch this exception/None and return the expected "not connected" result instead
of letting a KeyError bubble up.
Comment on lines
+95
to
+118
| def _record_primary_alert(cursor, conn, user_id, org_id, incident_db_id, event_db_id, | ||
| incident_title, service_name, severity, alert_metadata): | ||
| """Insert the primary alert row and update affected_services.""" | ||
| try: | ||
| cursor.execute( | ||
| """INSERT INTO incident_alerts | ||
| (user_id, org_id, incident_id, source_type, source_alert_id, | ||
| alert_title, alert_service, alert_severity, | ||
| correlation_strategy, correlation_score, alert_metadata) | ||
| VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s)""", | ||
| ( | ||
| user_id, org_id, incident_db_id, "victorops", event_db_id, | ||
| incident_title, service_name, severity, "primary", 1.0, | ||
| json.dumps(alert_metadata), | ||
| ), | ||
| ) | ||
| cursor.execute( | ||
| "UPDATE incidents SET affected_services = ARRAY[%s] WHERE id = %s", | ||
| (service_name, incident_db_id), | ||
| ) | ||
| conn.commit() | ||
| except Exception as e: | ||
| logger.warning("[VICTOROPS] Failed to record primary alert: %s", e) | ||
|
|
Contributor
There was a problem hiding this comment.
Rollback on DB write failure in _record_primary_alert.
If either SQL statement fails, the connection remains in an aborted transaction state; subsequent DB operations in the same task can fail unexpectedly. Add conn.rollback() in the except branch before continuing.
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 95-95: Missing return type annotation for private function _record_primary_alert
Add return type annotation: None
(ANN202)
[warning] 116-116: Do not catch blind exception: Exception
(BLE001)
🤖 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/routes/victorops/tasks.py` around lines 95 - 118, The
_record_primary_alert function currently commits after two SQL statements but
does not rollback on failure; update the except block in _record_primary_alert
to call conn.rollback() before logging/returning so the connection is returned
to a clean state when either cursor.execute call fails; reference the existing
variables cursor and conn and keep the existing logger.warning call after the
rollback.
Comment on lines
+208
to
+240
| try: | ||
| correlator = AlertCorrelator() | ||
| correlation_result = correlator.correlate( | ||
| cursor=cursor, | ||
| user_id=user_id, | ||
| source_type="victorops", | ||
| source_alert_id=event_db_id, | ||
| alert_title=incident_title, | ||
| alert_service=service_name, | ||
| alert_severity=severity, | ||
| alert_metadata=alert_metadata, | ||
| org_id=org_id, | ||
| ) | ||
| if correlation_result.is_correlated: | ||
| handle_correlated_alert( | ||
| cursor=cursor, | ||
| user_id=user_id, | ||
| incident_id=correlation_result.incident_id, | ||
| source_type="victorops", | ||
| source_alert_id=event_db_id, | ||
| alert_title=incident_title, | ||
| alert_service=service_name, | ||
| alert_severity=severity, | ||
| correlation_result=correlation_result, | ||
| alert_metadata=alert_metadata, | ||
| raw_payload=payload, | ||
| org_id=org_id, | ||
| ) | ||
| conn.commit() | ||
| return True | ||
| except Exception as corr_exc: | ||
| logger.warning("[VICTOROPS] Correlation check failed, proceeding normally: %s", corr_exc) | ||
| return False |
Contributor
There was a problem hiding this comment.
Rollback before continuing after correlation-path DB exceptions.
_run_correlation_check catches DB-related exceptions and proceeds, but without conn.rollback() the transaction may stay aborted and break the later upsert/lifecycle writes. Roll back in the except path before returning False.
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 238-238: Do not catch blind exception: Exception
(BLE001)
🤖 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/routes/victorops/tasks.py` around lines 208 - 240, The except block in
_run_correlation_check (around AlertCorrelator.correlate and
handle_correlated_alert) swallows DB exceptions but does not roll back the
connection, leaving the transaction aborted; update the except handler to call
conn.rollback() (before logging/returning) so the DB transaction is reset, then
continue to return False as before (use the existing corr_exc variable and
ensure conn.rollback() is invoked prior to the logger.warning and return).
Comment on lines
+29
to
+31
| creds = get_token_data(user_id, "victorops") | ||
| if not creds: | ||
| return jsonify({"connected": False}) |
Contributor
There was a problem hiding this comment.
Use user-first + org-fallback credential lookup.
Both status and webhook validation only check user-scoped tokens. This breaks org-shared credentials (status shows disconnected; webhook returns 404 even when org token exists).
Based on learnings: In connector routes, _get_stored_<provider>_credentials is expected to do standard two-step lookup—user token first, then org-scoped fallback (org_id, provider, is_active=TRUE, secret_ref IS NOT NULL).
Also applies to: 140-146
🤖 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/routes/victorops/victorops_routes.py` around lines 29 - 31, Replace
the direct user-only token lookup (get_token_data(user_id, "victorops")) with
the connector's standard user-first then org-fallback lookup by calling the
shared helper for VictorOps credentials (e.g., _get_stored_victorops_credentials
or the module's equivalent) so it checks user token first and then looks up an
org-scoped active credential (org_id, provider="victorops", is_active=TRUE,
secret_ref IS NOT NULL); make the same replacement for the other occurrence
referenced (lines ~140-146) so both status and webhook validation use the
two-step lookup.
Apok21
force-pushed
the
feature/opensearch-victorops-connectors
branch
from
June 11, 2026 11:12
e2b22e3 to
42a83c7
Compare
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
client/src/lib/services/victorops.ts (1)
39-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit connector state-change events after disconnect.
Line 39-41 completes the DELETE but does not dispatch a state-change event, so listeners relying on event-triggered revalidation won’t update immediately after VictorOps disconnect.
Suggested fix
async disconnect(): Promise<void> { await apiRequest(API_BASE, { method: 'DELETE', cache: 'no-store' }); + if (typeof window !== 'undefined') { + window.dispatchEvent(new Event('victoropsStateChanged')); + window.dispatchEvent(new CustomEvent('providerStateChanged')); + } },As per coding guidelines, "Connector disconnect must trigger
window.dispatchEvent(new Event('<name>StateChanged'))," and based on learnings,providerStateChangedshould remain present in connector flows.🤖 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 `@client/src/lib/services/victorops.ts` around lines 39 - 41, The disconnect method (async function disconnect) currently issues the DELETE via apiRequest(API_BASE) but does not emit the required connector state-change event; after the await apiRequest(...) return, add the standard state-change notifications by calling the providerStateChanged helper (if exported/imported in this module) and then dispatching window.dispatchEvent(new Event('victoropsStateChanged')) so listeners revalidate immediately. Ensure providerStateChanged is invoked before/after the window event consistent with other connectors.Sources: Coding guidelines, Learnings
🤖 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/app/opensearch/auth/page.tsx`:
- Around line 30-36: The code parses localStorage cached status with
JSON.parse(cached) and applies setStatus/setEndpoint without guarding parse
failures, which can abort the live refresh; wrap the JSON.parse(cached) and
subsequent setStatus/setEndpoint calls in a try/catch (or use a safe JSON parse
helper) so a malformed cache is ignored (optionally clear the bad cache) and
does not prevent calling openSearchService.getStatus(); ensure
openSearchService.getStatus() is always invoked even if parsing fails so the
live connector status path continues.
- Line 69: The toast call currently always shows "OpenSearch connected"; change
the logic around the toast invocation in the component handling the connection
result to check result.connected and only show the success toast (using
clusterName ?? endpoint) when result.connected is true, otherwise show a
failure/error toast (e.g., "OpenSearch connection failed") with relevant details
from result (such as result.error or other diagnostic info); update the code
referencing toast, result, clusterName, endpoint, and result.connected
accordingly so the message reflects the actual connection outcome.
In `@server/celery_config.py`:
- Around line 226-230: The logging call inside the GitHub webhook import
exception handler uses an f-string which eagerly formats the message; update the
exception handler around importlib.import_module("tasks.github_webhook_tasks")
to use lazy %-style logging (e.g., logging.warning("Failed to import GitHub
webhook dispatcher task: %s", e)) so the exception is formatted only if the
warning is emitted, matching the VictorOps block's pattern.
In `@server/chat/backend/agent/tools/cloud_tools.py`:
- Around line 1042-1050: The cache_key construction in the tool selection branch
(variables cache_key, tool_capture) omits the pinned trigger/action id, so
cached toolsets can be reused with a stale action; update the cache key
generation in both branches to incorporate the effective trigger_action_id
(check state_context.trigger_action_id via getattr(state_context,
'trigger_action_id', None) and fall back to the module-level _pinned_trigger
lookup for the user if present) so the key includes the pinned action id (e.g.,
append ":trigger_action_id={trigger_id}" to the cache_key); ensure both the
nocapture and capture cache_key paths are changed and mirror the same lookup
logic used where _pinned_trigger is set.
---
Duplicate comments:
In `@client/src/lib/services/victorops.ts`:
- Around line 39-41: The disconnect method (async function disconnect) currently
issues the DELETE via apiRequest(API_BASE) but does not emit the required
connector state-change event; after the await apiRequest(...) return, add the
standard state-change notifications by calling the providerStateChanged helper
(if exported/imported in this module) and then dispatching
window.dispatchEvent(new Event('victoropsStateChanged')) so listeners revalidate
immediately. Ensure providerStateChanged is invoked before/after the window
event consistent with other connectors.
🪄 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: 768aac9d-9e32-43f9-b7c7-c731ee6b92e0
⛔ Files ignored due to path filters (2)
client/public/opensearch.svgis excluded by!**/*.svgclient/public/victorops.svgis excluded by!**/*.svg
📒 Files selected for processing (32)
.env.exampleclient/src/app/api/connected-accounts/[provider]/route.tsclient/src/app/api/opensearch/[...path]/route.tsclient/src/app/api/victorops/route.tsclient/src/app/api/victorops/webhook-url/route.tsclient/src/app/opensearch/auth/page.tsxclient/src/app/victorops/auth/page.tsxclient/src/components/connectors/ConnectorRegistry.tsclient/src/components/victorops/VictorOpsConnectedView.tsxclient/src/components/victorops/VictorOpsConnectionStep.tsxclient/src/components/victorops/VictorOpsWebhookStep.tsxclient/src/lib/services/opensearch.tsclient/src/lib/services/victorops.tsdocker-compose.prod-local.ymldocker-compose.yamlserver/celery_config.pyserver/chat/backend/agent/skills/integrations/opensearch/SKILL.mdserver/chat/backend/agent/skills/integrations/victorops/SKILL.mdserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/backend/agent/tools/opensearch_tool.pyserver/chat/backend/agent/tools/victorops_tool.pyserver/chat/background/rca_prompt_builder.pyserver/connectors/opensearch_connector/__init__.pyserver/connectors/opensearch_connector/client.pyserver/main_compute.pyserver/routes/connector_status.pyserver/routes/opensearch/__init__.pyserver/routes/opensearch/opensearch_routes.pyserver/routes/victorops/__init__.pyserver/routes/victorops/tasks.pyserver/routes/victorops/victorops_helpers.pyserver/routes/victorops/victorops_routes.py
💤 Files with no reviewable changes (10)
- server/routes/opensearch/init.py
- server/routes/victorops/victorops_helpers.py
- server/routes/victorops/init.py
- server/routes/connector_status.py
- server/main_compute.py
- server/chat/background/rca_prompt_builder.py
- server/connectors/opensearch_connector/client.py
- server/routes/opensearch/opensearch_routes.py
- server/routes/victorops/tasks.py
- server/routes/victorops/victorops_routes.py
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
♻️ Duplicate comments (1)
client/src/lib/services/victorops.ts (1)
39-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit connector state-change events after disconnect.
Line 39-41 completes the DELETE but does not dispatch a state-change event, so listeners relying on event-triggered revalidation won’t update immediately after VictorOps disconnect.
Suggested fix
async disconnect(): Promise<void> { await apiRequest(API_BASE, { method: 'DELETE', cache: 'no-store' }); + if (typeof window !== 'undefined') { + window.dispatchEvent(new Event('victoropsStateChanged')); + window.dispatchEvent(new CustomEvent('providerStateChanged')); + } },As per coding guidelines, "Connector disconnect must trigger
window.dispatchEvent(new Event('<name>StateChanged'))," and based on learnings,providerStateChangedshould remain present in connector flows.🤖 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 `@client/src/lib/services/victorops.ts` around lines 39 - 41, The disconnect method (async function disconnect) currently issues the DELETE via apiRequest(API_BASE) but does not emit the required connector state-change event; after the await apiRequest(...) return, add the standard state-change notifications by calling the providerStateChanged helper (if exported/imported in this module) and then dispatching window.dispatchEvent(new Event('victoropsStateChanged')) so listeners revalidate immediately. Ensure providerStateChanged is invoked before/after the window event consistent with other connectors.Sources: Coding guidelines, Learnings
🤖 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/app/opensearch/auth/page.tsx`:
- Around line 30-36: The code parses localStorage cached status with
JSON.parse(cached) and applies setStatus/setEndpoint without guarding parse
failures, which can abort the live refresh; wrap the JSON.parse(cached) and
subsequent setStatus/setEndpoint calls in a try/catch (or use a safe JSON parse
helper) so a malformed cache is ignored (optionally clear the bad cache) and
does not prevent calling openSearchService.getStatus(); ensure
openSearchService.getStatus() is always invoked even if parsing fails so the
live connector status path continues.
- Line 69: The toast call currently always shows "OpenSearch connected"; change
the logic around the toast invocation in the component handling the connection
result to check result.connected and only show the success toast (using
clusterName ?? endpoint) when result.connected is true, otherwise show a
failure/error toast (e.g., "OpenSearch connection failed") with relevant details
from result (such as result.error or other diagnostic info); update the code
referencing toast, result, clusterName, endpoint, and result.connected
accordingly so the message reflects the actual connection outcome.
In `@server/celery_config.py`:
- Around line 226-230: The logging call inside the GitHub webhook import
exception handler uses an f-string which eagerly formats the message; update the
exception handler around importlib.import_module("tasks.github_webhook_tasks")
to use lazy %-style logging (e.g., logging.warning("Failed to import GitHub
webhook dispatcher task: %s", e)) so the exception is formatted only if the
warning is emitted, matching the VictorOps block's pattern.
In `@server/chat/backend/agent/tools/cloud_tools.py`:
- Around line 1042-1050: The cache_key construction in the tool selection branch
(variables cache_key, tool_capture) omits the pinned trigger/action id, so
cached toolsets can be reused with a stale action; update the cache key
generation in both branches to incorporate the effective trigger_action_id
(check state_context.trigger_action_id via getattr(state_context,
'trigger_action_id', None) and fall back to the module-level _pinned_trigger
lookup for the user if present) so the key includes the pinned action id (e.g.,
append ":trigger_action_id={trigger_id}" to the cache_key); ensure both the
nocapture and capture cache_key paths are changed and mirror the same lookup
logic used where _pinned_trigger is set.
---
Duplicate comments:
In `@client/src/lib/services/victorops.ts`:
- Around line 39-41: The disconnect method (async function disconnect) currently
issues the DELETE via apiRequest(API_BASE) but does not emit the required
connector state-change event; after the await apiRequest(...) return, add the
standard state-change notifications by calling the providerStateChanged helper
(if exported/imported in this module) and then dispatching
window.dispatchEvent(new Event('victoropsStateChanged')) so listeners revalidate
immediately. Ensure providerStateChanged is invoked before/after the window
event consistent with other connectors.
🪄 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: 768aac9d-9e32-43f9-b7c7-c731ee6b92e0
⛔ Files ignored due to path filters (2)
client/public/opensearch.svgis excluded by!**/*.svgclient/public/victorops.svgis excluded by!**/*.svg
📒 Files selected for processing (32)
.env.exampleclient/src/app/api/connected-accounts/[provider]/route.tsclient/src/app/api/opensearch/[...path]/route.tsclient/src/app/api/victorops/route.tsclient/src/app/api/victorops/webhook-url/route.tsclient/src/app/opensearch/auth/page.tsxclient/src/app/victorops/auth/page.tsxclient/src/components/connectors/ConnectorRegistry.tsclient/src/components/victorops/VictorOpsConnectedView.tsxclient/src/components/victorops/VictorOpsConnectionStep.tsxclient/src/components/victorops/VictorOpsWebhookStep.tsxclient/src/lib/services/opensearch.tsclient/src/lib/services/victorops.tsdocker-compose.prod-local.ymldocker-compose.yamlserver/celery_config.pyserver/chat/backend/agent/skills/integrations/opensearch/SKILL.mdserver/chat/backend/agent/skills/integrations/victorops/SKILL.mdserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/backend/agent/tools/opensearch_tool.pyserver/chat/backend/agent/tools/victorops_tool.pyserver/chat/background/rca_prompt_builder.pyserver/connectors/opensearch_connector/__init__.pyserver/connectors/opensearch_connector/client.pyserver/main_compute.pyserver/routes/connector_status.pyserver/routes/opensearch/__init__.pyserver/routes/opensearch/opensearch_routes.pyserver/routes/victorops/__init__.pyserver/routes/victorops/tasks.pyserver/routes/victorops/victorops_helpers.pyserver/routes/victorops/victorops_routes.py
💤 Files with no reviewable changes (10)
- server/routes/opensearch/init.py
- server/routes/victorops/victorops_helpers.py
- server/routes/victorops/init.py
- server/routes/connector_status.py
- server/main_compute.py
- server/chat/background/rca_prompt_builder.py
- server/connectors/opensearch_connector/client.py
- server/routes/opensearch/opensearch_routes.py
- server/routes/victorops/tasks.py
- server/routes/victorops/victorops_routes.py
🛑 Comments failed to post (5)
client/src/app/opensearch/auth/page.tsx (3)
25-50:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftShared root cause: connector status loading is not wired to
revalidateOnEventscontract.Both connector auth pages use one-off/manual status fetch patterns instead of the required event-driven revalidation path.
As per coding guidelines, "Connector status queries must use
revalidateOnEventswith the provider's state event".Source: Coding guidelines
30-36:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winShared root cause: unguarded localStorage JSON parsing can block live connector status refresh.
Both auth pages parse cached status without isolating parse failures, so a malformed cache can bypass the subsequent live status fetch path for that run.
🤖 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 `@client/src/app/opensearch/auth/page.tsx` around lines 30 - 36, The code parses localStorage cached status with JSON.parse(cached) and applies setStatus/setEndpoint without guarding parse failures, which can abort the live refresh; wrap the JSON.parse(cached) and subsequent setStatus/setEndpoint calls in a try/catch (or use a safe JSON parse helper) so a malformed cache is ignored (optionally clear the bad cache) and does not prevent calling openSearchService.getStatus(); ensure openSearchService.getStatus() is always invoked even if parsing fails so the live connector status path continues.
69-69:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the connect toast conditional on the actual connection result.
At Line 69, the UI always reports “OpenSearch connected” even when
result.connectedis false.🤖 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 `@client/src/app/opensearch/auth/page.tsx` at line 69, The toast call currently always shows "OpenSearch connected"; change the logic around the toast invocation in the component handling the connection result to check result.connected and only show the success toast (using clusterName ?? endpoint) when result.connected is true, otherwise show a failure/error toast (e.g., "OpenSearch connection failed") with relevant details from result (such as result.error or other diagnostic info); update the code referencing toast, result, clusterName, endpoint, and result.connected accordingly so the message reflects the actual connection outcome.server/celery_config.py (1)
226-230:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse lazy % formatting in logging for consistency.
Line 230 uses f-string interpolation in the warning message, which eagerly formats even when the log level might suppress the message (Ruff G004). The VictorOps import block above (line 182) was corrected to use lazy
%sformatting. Apply the same pattern here for consistency.♻️ Suggested fix
try: importlib.import_module("tasks.github_webhook_tasks") logging.info("GitHub webhook dispatcher task imported successfully") except ImportError as e: - logging.warning(f"Failed to import GitHub webhook dispatcher task: {e}") + logging.warning("Failed to import GitHub webhook dispatcher task: %s", e)🧰 Tools
🪛 Ruff (0.15.15)
[warning] 230-230: Logging statement uses f-string
(G004)
🤖 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/celery_config.py` around lines 226 - 230, The logging call inside the GitHub webhook import exception handler uses an f-string which eagerly formats the message; update the exception handler around importlib.import_module("tasks.github_webhook_tasks") to use lazy %-style logging (e.g., logging.warning("Failed to import GitHub webhook dispatcher task: %s", e)) so the exception is formatted only if the warning is emitted, matching the VictorOps block's pattern.server/chat/backend/agent/tools/cloud_tools.py (1)
1042-1050:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winCache key omits pinned
trigger_action_id, so cached tools can execute the wrong action.Line 1047/Line 1049 build the cache key without
trigger_action_id, but Line 1608 pinsaction_idinto_pinned_trigger. For the same user/mode/background flags, a cached toolset can be reused with a stale action ID and dispatch the wrong action.Proposed fix
- rca_flag = getattr(state_context, 'trigger_rca_requested', False) if state_context else False - is_background = getattr(state_context, 'is_background', False) if state_context else False + rca_flag = getattr(state_context, 'trigger_rca_requested', False) if state_context else False + is_background = getattr(state_context, 'is_background', False) if state_context else False + action_id = getattr(state_context, 'trigger_action_id', None) if state_context else None @@ - cache_key = f"{user_id}:nocapture:{mode_suffix}:background={is_background}:rca={rca_flag}:postmortem={is_postmortem_action}:is_rca_ctx={is_rca_context}" + cache_key = ( + f"{user_id}:nocapture:{mode_suffix}:background={is_background}:rca={rca_flag}:" + f"postmortem={is_postmortem_action}:is_rca_ctx={is_rca_context}:action_id={action_id or ''}" + ) @@ - cache_key = f"{user_id}:capture:{id(tool_capture)}:{mode_suffix}:background={is_background}:rca={rca_flag}:postmortem={is_postmortem_action}:is_rca_ctx={is_rca_context}" + cache_key = ( + f"{user_id}:capture:{id(tool_capture)}:{mode_suffix}:background={is_background}:rca={rca_flag}:" + f"postmortem={is_postmortem_action}:is_rca_ctx={is_rca_context}:action_id={action_id or ''}" + )Also applies to: 1608-1617
🤖 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/cloud_tools.py` around lines 1042 - 1050, The cache_key construction in the tool selection branch (variables cache_key, tool_capture) omits the pinned trigger/action id, so cached toolsets can be reused with a stale action; update the cache key generation in both branches to incorporate the effective trigger_action_id (check state_context.trigger_action_id via getattr(state_context, 'trigger_action_id', None) and fall back to the module-level _pinned_trigger lookup for the user if present) so the key includes the pinned action id (e.g., append ":trigger_action_id={trigger_id}" to the cache_key); ensure both the nocapture and capture cache_key paths are changed and mirror the same lookup logic used where _pinned_trigger is set.
Adds two new connectors to Aurora: **OpenSearch** (log analytics for RCA) - Backend connector client with cluster info, health check, search, and index listing (server/connectors/opensearch_connector/) - REST routes for connect/disconnect/status/search/indices with full RBAC enforcement (server/routes/opensearch/) - LangChain tools: search_opensearch, list_opensearch_indices (server/chat/backend/agent/tools/opensearch_tool.py) - Agent skill with RCA workflow guidance (server/chat/backend/agent/skills/integrations/opensearch/SKILL.md) - Frontend auth page and API proxy (client/src/app/opensearch/, client/src/app/api/opensearch/) - ConnectorRegistry entry, service helpers **Splunk On-Call / VictorOps** (real-time incident alerting + RCA) - VictorOps REST API client with credential validation (server/routes/victorops/victorops_helpers.py) - Routes for connect/disconnect/status/webhook with RBAC (server/routes/victorops/victorops_routes.py) - Celery task for async webhook processing (server/routes/victorops/tasks.py) - LangChain tools: get_victorops_incidents, get_victorops_teams (server/chat/backend/agent/tools/victorops_tool.py) - Agent skill with RCA workflow guidance (server/chat/backend/agent/skills/integrations/victorops/SKILL.md) - Frontend: connection wizard (API ID + Key), webhook setup step, connected view, and auth page (client/src/app/victorops/, client/src/components/victorops/) - ConnectorRegistry entry, service helpers **Shared changes** - Added 'opensearch' and 'victorops' to CONNECTOR_DIRS in providers.py - Registered both blueprints and CORS rules in main_compute.py - Extended connector_status.py, cloud_tools.py, rca_prompt_builder.py, secret_ref_utils.py with new connector support Co-authored-by: Cursor <cursoragent@cursor.com>
Restores AtlassianConnectPage.tsx, confluence_connector/client.py, and atlassian_routes.py to their upstream/main state. The Atlassian Cloud email + API token improvements (Basic auth support for .atlassian.net URLs) were inadvertently included from the personal fork's working directory and belong in a separate PR. Co-authored-by: Cursor <cursoragent@cursor.com>
…VictorOps connectors
Frontend:
- Replace raw fetch calls in API proxy routes with forwardRequest from @/lib/backend-proxy
- Dispatch providerStateChanged alongside scoped connector events so the
connectors dashboard refreshes on connect/disconnect
- Limit localStorage cache to { connected: boolean } for both connectors
- Write CACHE_KEY immediately after successful VictorOps connect
- Remove duplicate victorOpsStateChanged dispatch from victorops service disconnect()
- Add aria-label/title to VictorOps webhook copy button for accessibility
Backend:
- Add optional webhook secret validation via VICTOROPS_WEBHOOK_SECRET env var
- Guard against empty base_url in get_webhook_url
- Strip sensitive request headers from webhook metadata passed to Celery tasks
- Fix RCA prompt tuple unpacking bug in victorops tasks
- Add incident_was_inserted guard to prevent duplicate RCA scheduling
- Harden verifySsl/maxRetries/size parameter parsing in OpenSearch connect route
- Remove username from connection log line
- Handle non-JSON responses in OpenSearch connector client
- Redact endpoint URLs from OpenSearch/VictorOps tool exception logs
- Replace logger.exception with logger.warning using generic messages
- Remove unused **kwargs from get_victorops_incidents/get_victorops_teams
- Fix Ruff G004: convert f-string logging to %s-style in celery_config
Config:
- Document VICTOROPS_WEBHOOK_SECRET in .env.example
- Add VICTOROPS_WEBHOOK_SECRET to aurora-server env in both docker-compose files
Co-authored-by: Cursor <cursoragent@cursor.com>
…onnectors TypeScript: - Replace window with globalThis/globalThis.window in opensearch and victorops auth pages to satisfy S7764 (14 occurrences) - Remove redundant double-cast in victorops/auth/page.tsx (S4325) - Mark all props as readonly in VictorOpsConnectedView and VictorOpsConnectionStep interfaces (S6759) Python: - Extract DEFAULT_TIMESTAMP_FIELD constant to replace 3 duplicate "@timestamp" literals in opensearch_tool.py (S1192) - Switch logger.error to logger.exception in opensearch_routes._get_creds (S8572) - Remove trivial except VictorOpsAPIError: raise in victorops_helpers.py (S2737) - Reduce cognitive complexity in three functions (S3776): - opensearch_tool.py: extract _format_hit helper - opensearch_connector/client.py: extract _http_error_to_opensearch_error and combine ConnectTimeout/ReadTimeout into a single except clause - victorops/tasks.py: extract _record_primary_alert, _write_lifecycle_events, and _schedule_summary_and_rca helpers Co-authored-by: Cursor <cursoragent@cursor.com>
…tors Backend: - Use hmac.compare_digest for constant-time webhook secret validation in victorops_routes.py to prevent timing-based side-channel attacks - Extract _run_correlation_check and _process_victorops_event_in_db helpers in tasks.py to reduce cognitive complexity of the Celery task function - Use raise...from exc for proper exception chaining in tasks.py - Add ge=1, le=100 field constraints to VictorOps tool limit param; replace one-shot trim with iterative halving to respect agent response budget - Fix build_victorops_rca_prompt return type annotation (str -> tuple[str, str]) - Use normalize_endpoint() static method in OpenSearchClient constructor - Remove unused hash_for_log import from opensearch_routes.py Frontend: - Simplify typeof globalThis.window !== undefined checks to globalThis.window !== undefined in opensearch and victorops auth pages - Fix isOpenSearchConnected localStorage value to reflect actual connection state instead of hardcoded true Co-authored-by: Cursor <cursoragent@cursor.com>
Apok21
force-pushed
the
feature/opensearch-victorops-connectors
branch
from
June 16, 2026 17:50
42a83c7 to
d458f91
Compare
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (4)
server/routes/victorops/tasks.py (1)
95-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback after swallowed DB exceptions to avoid aborted-transaction cascades.
At Line 116 and Line 238, DB exceptions are logged and execution continues without
conn.rollback(). On psycopg2/PostgreSQL this leaves the transaction aborted, so later statements on the same connection fail and can cause partial processing/retries.Suggested fix
def _record_primary_alert(cursor, conn, user_id, org_id, incident_db_id, event_db_id, incident_title, service_name, severity, alert_metadata): @@ - except Exception as e: + except Exception as e: + conn.rollback() logger.warning("[VICTOROPS] Failed to record primary alert: %s", e) @@ def _run_correlation_check( @@ - except Exception as corr_exc: + except Exception as corr_exc: + conn.rollback() logger.warning("[VICTOROPS] Correlation check failed, proceeding normally: %s", corr_exc) return FalseAlso applies to: 208-240
🤖 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/routes/victorops/tasks.py` around lines 95 - 118, In the _record_primary_alert function's except block, add a call to conn.rollback() after logging the exception to properly rollback the aborted transaction on PostgreSQL. This prevents the transaction from remaining in an aborted state, which would cause subsequent statements on the same connection to fail. The same fix must also be applied to the exception handler at the location referenced in "Also applies to: 208-240" to ensure all swallowed database exceptions properly rollback their transactions.server/chat/backend/agent/skills/integrations/victorops/SKILL.md (1)
23-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd blank lines after section headings to satisfy markdownlint (MD022).
Lines 23, 28, 32, and 38 are missing blank lines immediately after the headings, triggering markdownlint MD022 warnings. Insert one blank line after each heading. (Note: Past comments indicated this was addressed, but the violations persist in the current code.)
📝 Proposed fix
## Overview + Splunk On-Call (formerly VictorOps) is an on-call incident management platform. Use this integration during RCA to retrieve incident history, check which teams are on-call, and correlate the current alert with past incidents. This is a REMOTE API — do NOT search the local filesystem. ## Instructions ### Tool Usage + 1. `get_victorops_incidents()` — Retrieve recent incidents from Splunk On-Call. Use during RCA to find similar past incidents or check incident history. 2. `get_victorops_teams()` — List teams and on-call schedules. Use to identify who was on-call when the incident triggered. ### RCA Workflow + - **Read-only**: Only query incident data during RCA. Never create, acknowledge, or resolve incidents automatically. - Use `get_victorops_incidents` to find related past incidents and identify patterns. - Cross-reference incident timeline with metrics from connected monitoring tools (Datadog, Grafana, etc.). - Note the routing key and escalation path to understand which service or team is responsible. ### Context to gather + - Recent incidents for the same service/routing key - Incident frequency and recurring patterns - Team ownership and on-call rotation at time of incident🤖 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/skills/integrations/victorops/SKILL.md` around lines 23 - 41, The markdown file has section headings without blank lines immediately following them, which violates markdownlint rule MD022. In the SKILL.md file, add one blank line after each of the following headings: "## Overview", "## Instructions", "### Tool Usage", and "### RCA Workflow". This ensures proper markdown formatting and satisfies the linter requirements.server/chat/backend/agent/skills/integrations/opensearch/SKILL.md (1)
23-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd blank lines after section headings to satisfy markdownlint (MD022).
Lines 23, 28, 32, 39, and 57 are missing blank lines immediately after the headings, triggering markdownlint MD022 warnings. Insert one blank line after each heading.
📝 Proposed fix
## Overview + OpenSearch integration for querying log data during Root Cause Analysis. OpenSearch is a REMOTE service — do NOT search the local filesystem. Use ONLY the tools listed below. ## Instructions ### Tool Usage + 1. `list_opensearch_indices()` — Discover available indices. Call first to understand what data exists. 2. `search_opensearch(query='error', start_time='now-1h')` — Search for logs matching a Lucene query. ### Common Query Patterns + - Error search: `search_opensearch(query='error AND service:api', start_time='now-1h')` - Specific service: `search_opensearch(query='kubernetes.labels.app:payment-service AND level:error', start_time='now-30m')` - HTTP 5xx: `search_opensearch(query='http.response.status_code:>=500', start_time='now-1h')` - Exception: `search_opensearch(query='exception OR stacktrace', start_time='now-2h', end_time='now')` - Specific index: `search_opensearch(query='NullPointerException', index='app-logs-*', start_time='now-1h')` ### Time Format + - Relative: `now-1h`, `now-30m`, `now-6h`, `now-1d` - Absolute: ISO-8601 strings (`2024-01-15T10:00:00Z`) ## RCA Investigation Workflow @@ ## Important Rules + - OpenSearch is a REMOTE service. Never try to access data from the local filesystem.🤖 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/skills/integrations/opensearch/SKILL.md` around lines 23 - 62, The markdown file has several section headings that violate markdownlint rule MD022 by missing blank lines immediately after them. Insert one blank line after each of the following headings: "## Overview", "### Tool Usage (use in this order)", "### Common Query Patterns", "### Time Format", and "## Important Rules". This will ensure proper markdown formatting and satisfy the linter requirements.client/src/app/victorops/auth/page.tsx (1)
48-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle corrupted cached status without blocking live refresh.
If
localStoragecontains malformed JSON,JSON.parseon line 53 throws and the outercatchblock on line 58 preventsfetchAndUpdateStatus()(line 57) from executing, leaving the page stuck on stale state with an error toast until cache is manually cleared.🔒 Proposed fix to isolate cache parsing errors
const loadStatus = async (skipCache = false) => { try { if (!skipCache && globalThis.window !== undefined) { const cached = localStorage.getItem(CACHE_KEY); if (cached) { - const minimal = JSON.parse(cached) as VictorOpsStatus; - setStatus(minimal); + try { + const minimal = JSON.parse(cached) as { connected: boolean }; + setStatus(minimal); + } catch { + localStorage.removeItem(CACHE_KEY); + } } } await fetchAndUpdateStatus(); } catch {🤖 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 `@client/src/app/victorops/auth/page.tsx` around lines 48 - 65, The current implementation has a nested try-catch where cache parsing errors in the inner block get caught by the outer catch handler, preventing fetchAndUpdateStatus() from executing and leaving the page stuck with stale cached data. Isolate the localStorage cache parsing logic (the JSON.parse of cached data and setStatus call) into its own inner try-catch block within loadStatus, so that if JSON.parse throws due to corrupted cache, it can be handled independently without affecting the fetchAndUpdateStatus() call which must always execute regardless of cache issues. Ensure fetchAndUpdateStatus() remains in the outer try block so that only real fetch failures trigger the error toast.
🤖 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/connectors/ConnectorRegistry.ts`:
- Around line 162-182: The opensearch and victorops connector registrations in
the register method calls are missing the stateEvent property required for
event-triggered status revalidation. Add a stateEvent field to both connector
objects with appropriate provider-specific state event names (following the
naming convention used by other connectors in the registry). If the
ConnectorConfig type does not include a stateEvent field, extend that interface
to support this property.
In `@server/chat/backend/agent/tools/opensearch_tool.py`:
- Around line 46-60: The _get_client function checks if creds is falsy but does
not validate that individual required credential fields exist before accessing
them. Lines 54-56 use bracket notation to access creds["endpoint"],
creds["username"], and creds["password"] without checking if these keys are
present, which will raise KeyError on partially corrupted token data instead of
the intended RuntimeError. Fix this by adding validation after the creds check
to ensure endpoint, username, and password keys exist in the dictionary, and
raise RuntimeError with a descriptive message if any required field is missing.
Alternatively, use the .get() method with appropriate error handling for these
required fields, similar to how optional fields are handled for index_pattern,
verify_ssl, and max_retries.
In `@server/chat/backend/agent/tools/victorops_tool.py`:
- Around line 39-46: The _get_client function validates only whether creds
exists but then accesses creds["api_id"] and creds["api_key"] using bracket
notation on line 46, which raises KeyError if either key is missing from
partially corrupted token data. Replace the bracket notation with the .get()
method (following the pattern already used in is_victorops_connected at line 33)
to safely retrieve both the api_id and api_key values, returning None if any
required credential field is missing.
In `@server/chat/background/rca_prompt_builder.py`:
- Line 480: The build_rca_prompt function call in the VictorOps prompt builder
is passing arguments in the wrong order, causing alert_details to be mapped to
the title parameter and providers to be mapped to the payload parameter. This
results in payload being None, which later causes failures when payload.get(...)
is called. Fix the argument order in the build_rca_prompt call to correctly map
alert_details as the payload and providers as the appropriate parameter,
ensuring none of the arguments are None when the function executes.
In `@server/connectors/opensearch_connector/client.py`:
- Line 1: The module docstring for the OpenSearch client claims support for both
Basic auth and AWS SigV4, but the implementation only supports HTTP Basic auth.
Update the docstring to accurately reflect the supported authentication methods
by removing the reference to AWS SigV4 and keeping only the Basic auth
capability claim to prevent misleading integration expectations.
In `@server/main_compute.py`:
- Line 30: The root logger is configured with DEBUG level in the main_compute.py
entrypoint, which exposes verbose framework logs in production and violates
backend logging guidelines. Change the level parameter from logging.DEBUG to
logging.INFO in the root logger configuration while keeping the stdout handler
change intact. This ensures the backend maintains the standard INFO-level
logging for production environments.
In `@server/routes/opensearch/opensearch_routes.py`:
- Around line 21-26: The _get_creds function currently only checks the
requesting user's own token via get_token_data but does not fall back to
org-level shared credentials, which causes status/search/indices endpoints to
fail for other authorized users. Modify _get_creds to implement the standard
two-step credential lookup pattern: first attempt to retrieve the user's own
opensearch token via get_token_data(user_id, "opensearch"), and if that returns
None, fall back to an org-scoped database query for active secrets filtered by
org_id, provider = 'opensearch', is_active = TRUE, and secret_ref IS NOT NULL.
Return the org secret if found, otherwise return None.
- Around line 48-53: Add validation to ensure the JSON payload is a dictionary
and that string fields are actually strings before calling `.strip()`.
Specifically, after calling request.get_json(), validate that the result is a
dict; then for fields like endpoint, username, and indexPattern that require
`.strip()`, check that the retrieved value is a string before applying the
method. If validation fails, return a 400 Bad Request response with a
descriptive error message instead of allowing the code to proceed, which could
cause 500 errors when calling methods on non-string values.
- Around line 57-58: The current boolean parsing for verify_ssl silently treats
unknown string values as False instead of rejecting them, creating a security
risk where typos disable TLS verification. Modify the string parsing logic in
the isinstance check to validate that raw_verify_ssl.strip().lower() is
explicitly in the set of accepted true values ("1", "true", "yes", "on"), and
for any other string value (including unknown strings), raise an appropriate
HTTP 400 error to reject the invalid input rather than defaulting to False.
- Line 6: Remove the deprecated `Dict` import from the import statement on line
6, replacing it with the built-in `dict` type. Then update all type hints
throughout the file that use `Dict[str, Any]` (appearing at lines 21, 29, and
105) to use the modern syntax `dict[str, Any]` instead. Finally, remove the
redundant exception parameter `, exc` from the two logger.exception() calls at
lines 121 and 174, since the logging framework automatically captures the
traceback when using logger.exception().
In `@server/routes/victorops/victorops_routes.py`:
- Around line 130-138: The webhook validation in the victorops webhook endpoint
currently makes the secret validation optional when VICTOROPS_WEBHOOK_SECRET is
not configured, which allows forged events from anyone knowing a valid user_id.
Modify the webhook secret validation logic (the `if webhook_secret:` check and
the validation block) to fail closed by requiring the secret to be configured
and validated outside of dev/test environments. Instead of skipping validation
when the secret is empty, check the environment context and reject webhook
payloads if the secret is not configured in non-development/test environments,
while only permitting missing secrets in development/test modes.
---
Duplicate comments:
In `@client/src/app/victorops/auth/page.tsx`:
- Around line 48-65: The current implementation has a nested try-catch where
cache parsing errors in the inner block get caught by the outer catch handler,
preventing fetchAndUpdateStatus() from executing and leaving the page stuck with
stale cached data. Isolate the localStorage cache parsing logic (the JSON.parse
of cached data and setStatus call) into its own inner try-catch block within
loadStatus, so that if JSON.parse throws due to corrupted cache, it can be
handled independently without affecting the fetchAndUpdateStatus() call which
must always execute regardless of cache issues. Ensure fetchAndUpdateStatus()
remains in the outer try block so that only real fetch failures trigger the
error toast.
In `@server/chat/backend/agent/skills/integrations/opensearch/SKILL.md`:
- Around line 23-62: The markdown file has several section headings that violate
markdownlint rule MD022 by missing blank lines immediately after them. Insert
one blank line after each of the following headings: "## Overview", "### Tool
Usage (use in this order)", "### Common Query Patterns", "### Time Format", and
"## Important Rules". This will ensure proper markdown formatting and satisfy
the linter requirements.
In `@server/chat/backend/agent/skills/integrations/victorops/SKILL.md`:
- Around line 23-41: The markdown file has section headings without blank lines
immediately following them, which violates markdownlint rule MD022. In the
SKILL.md file, add one blank line after each of the following headings: "##
Overview", "## Instructions", "### Tool Usage", and "### RCA Workflow". This
ensures proper markdown formatting and satisfies the linter requirements.
In `@server/routes/victorops/tasks.py`:
- Around line 95-118: In the _record_primary_alert function's except block, add
a call to conn.rollback() after logging the exception to properly rollback the
aborted transaction on PostgreSQL. This prevents the transaction from remaining
in an aborted state, which would cause subsequent statements on the same
connection to fail. The same fix must also be applied to the exception handler
at the location referenced in "Also applies to: 208-240" to ensure all swallowed
database exceptions properly rollback their transactions.
🪄 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: 40507b10-614c-4668-9aeb-f74b636ade0f
⛔ Files ignored due to path filters (2)
client/public/opensearch.svgis excluded by!**/*.svgclient/public/victorops.svgis excluded by!**/*.svg
📒 Files selected for processing (35)
.env.exampleclient/src/app/api/connected-accounts/[provider]/route.tsclient/src/app/api/opensearch/[...path]/route.tsclient/src/app/api/victorops/route.tsclient/src/app/api/victorops/webhook-url/route.tsclient/src/app/opensearch/auth/page.tsxclient/src/app/victorops/auth/page.tsxclient/src/components/connectors/ConnectorRegistry.tsclient/src/components/victorops/VictorOpsConnectedView.tsxclient/src/components/victorops/VictorOpsConnectionStep.tsxclient/src/components/victorops/VictorOpsWebhookStep.tsxclient/src/lib/services/opensearch.tsclient/src/lib/services/victorops.tsdocker-compose.prod-local.ymldocker-compose.yamlserver/celery_config.pyserver/chat/backend/agent/skills/integrations/opensearch/SKILL.mdserver/chat/backend/agent/skills/integrations/victorops/SKILL.mdserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/backend/agent/tools/opensearch_tool.pyserver/chat/backend/agent/tools/victorops_tool.pyserver/chat/background/rca_prompt_builder.pyserver/connectors/opensearch_connector/__init__.pyserver/connectors/opensearch_connector/client.pyserver/main_compute.pyserver/routes/connector_status.pyserver/routes/opensearch/__init__.pyserver/routes/opensearch/opensearch_routes.pyserver/routes/victorops/__init__.pyserver/routes/victorops/tasks.pyserver/routes/victorops/victorops_helpers.pyserver/routes/victorops/victorops_routes.pyserver/utils/db/db_utils.pyserver/utils/providers.pyserver/utils/secrets/secret_ref_utils.py
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 11
♻️ Duplicate comments (4)
server/routes/victorops/tasks.py (1)
95-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback after swallowed DB exceptions to avoid aborted-transaction cascades.
At Line 116 and Line 238, DB exceptions are logged and execution continues without
conn.rollback(). On psycopg2/PostgreSQL this leaves the transaction aborted, so later statements on the same connection fail and can cause partial processing/retries.Suggested fix
def _record_primary_alert(cursor, conn, user_id, org_id, incident_db_id, event_db_id, incident_title, service_name, severity, alert_metadata): @@ - except Exception as e: + except Exception as e: + conn.rollback() logger.warning("[VICTOROPS] Failed to record primary alert: %s", e) @@ def _run_correlation_check( @@ - except Exception as corr_exc: + except Exception as corr_exc: + conn.rollback() logger.warning("[VICTOROPS] Correlation check failed, proceeding normally: %s", corr_exc) return FalseAlso applies to: 208-240
🤖 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/routes/victorops/tasks.py` around lines 95 - 118, In the _record_primary_alert function's except block, add a call to conn.rollback() after logging the exception to properly rollback the aborted transaction on PostgreSQL. This prevents the transaction from remaining in an aborted state, which would cause subsequent statements on the same connection to fail. The same fix must also be applied to the exception handler at the location referenced in "Also applies to: 208-240" to ensure all swallowed database exceptions properly rollback their transactions.server/chat/backend/agent/skills/integrations/victorops/SKILL.md (1)
23-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd blank lines after section headings to satisfy markdownlint (MD022).
Lines 23, 28, 32, and 38 are missing blank lines immediately after the headings, triggering markdownlint MD022 warnings. Insert one blank line after each heading. (Note: Past comments indicated this was addressed, but the violations persist in the current code.)
📝 Proposed fix
## Overview + Splunk On-Call (formerly VictorOps) is an on-call incident management platform. Use this integration during RCA to retrieve incident history, check which teams are on-call, and correlate the current alert with past incidents. This is a REMOTE API — do NOT search the local filesystem. ## Instructions ### Tool Usage + 1. `get_victorops_incidents()` — Retrieve recent incidents from Splunk On-Call. Use during RCA to find similar past incidents or check incident history. 2. `get_victorops_teams()` — List teams and on-call schedules. Use to identify who was on-call when the incident triggered. ### RCA Workflow + - **Read-only**: Only query incident data during RCA. Never create, acknowledge, or resolve incidents automatically. - Use `get_victorops_incidents` to find related past incidents and identify patterns. - Cross-reference incident timeline with metrics from connected monitoring tools (Datadog, Grafana, etc.). - Note the routing key and escalation path to understand which service or team is responsible. ### Context to gather + - Recent incidents for the same service/routing key - Incident frequency and recurring patterns - Team ownership and on-call rotation at time of incident🤖 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/skills/integrations/victorops/SKILL.md` around lines 23 - 41, The markdown file has section headings without blank lines immediately following them, which violates markdownlint rule MD022. In the SKILL.md file, add one blank line after each of the following headings: "## Overview", "## Instructions", "### Tool Usage", and "### RCA Workflow". This ensures proper markdown formatting and satisfies the linter requirements.server/chat/backend/agent/skills/integrations/opensearch/SKILL.md (1)
23-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd blank lines after section headings to satisfy markdownlint (MD022).
Lines 23, 28, 32, 39, and 57 are missing blank lines immediately after the headings, triggering markdownlint MD022 warnings. Insert one blank line after each heading.
📝 Proposed fix
## Overview + OpenSearch integration for querying log data during Root Cause Analysis. OpenSearch is a REMOTE service — do NOT search the local filesystem. Use ONLY the tools listed below. ## Instructions ### Tool Usage + 1. `list_opensearch_indices()` — Discover available indices. Call first to understand what data exists. 2. `search_opensearch(query='error', start_time='now-1h')` — Search for logs matching a Lucene query. ### Common Query Patterns + - Error search: `search_opensearch(query='error AND service:api', start_time='now-1h')` - Specific service: `search_opensearch(query='kubernetes.labels.app:payment-service AND level:error', start_time='now-30m')` - HTTP 5xx: `search_opensearch(query='http.response.status_code:>=500', start_time='now-1h')` - Exception: `search_opensearch(query='exception OR stacktrace', start_time='now-2h', end_time='now')` - Specific index: `search_opensearch(query='NullPointerException', index='app-logs-*', start_time='now-1h')` ### Time Format + - Relative: `now-1h`, `now-30m`, `now-6h`, `now-1d` - Absolute: ISO-8601 strings (`2024-01-15T10:00:00Z`) ## RCA Investigation Workflow @@ ## Important Rules + - OpenSearch is a REMOTE service. Never try to access data from the local filesystem.🤖 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/skills/integrations/opensearch/SKILL.md` around lines 23 - 62, The markdown file has several section headings that violate markdownlint rule MD022 by missing blank lines immediately after them. Insert one blank line after each of the following headings: "## Overview", "### Tool Usage (use in this order)", "### Common Query Patterns", "### Time Format", and "## Important Rules". This will ensure proper markdown formatting and satisfy the linter requirements.client/src/app/victorops/auth/page.tsx (1)
48-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle corrupted cached status without blocking live refresh.
If
localStoragecontains malformed JSON,JSON.parseon line 53 throws and the outercatchblock on line 58 preventsfetchAndUpdateStatus()(line 57) from executing, leaving the page stuck on stale state with an error toast until cache is manually cleared.🔒 Proposed fix to isolate cache parsing errors
const loadStatus = async (skipCache = false) => { try { if (!skipCache && globalThis.window !== undefined) { const cached = localStorage.getItem(CACHE_KEY); if (cached) { - const minimal = JSON.parse(cached) as VictorOpsStatus; - setStatus(minimal); + try { + const minimal = JSON.parse(cached) as { connected: boolean }; + setStatus(minimal); + } catch { + localStorage.removeItem(CACHE_KEY); + } } } await fetchAndUpdateStatus(); } catch {🤖 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 `@client/src/app/victorops/auth/page.tsx` around lines 48 - 65, The current implementation has a nested try-catch where cache parsing errors in the inner block get caught by the outer catch handler, preventing fetchAndUpdateStatus() from executing and leaving the page stuck with stale cached data. Isolate the localStorage cache parsing logic (the JSON.parse of cached data and setStatus call) into its own inner try-catch block within loadStatus, so that if JSON.parse throws due to corrupted cache, it can be handled independently without affecting the fetchAndUpdateStatus() call which must always execute regardless of cache issues. Ensure fetchAndUpdateStatus() remains in the outer try block so that only real fetch failures trigger the error toast.
🤖 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/connectors/ConnectorRegistry.ts`:
- Around line 162-182: The opensearch and victorops connector registrations in
the register method calls are missing the stateEvent property required for
event-triggered status revalidation. Add a stateEvent field to both connector
objects with appropriate provider-specific state event names (following the
naming convention used by other connectors in the registry). If the
ConnectorConfig type does not include a stateEvent field, extend that interface
to support this property.
In `@server/chat/backend/agent/tools/opensearch_tool.py`:
- Around line 46-60: The _get_client function checks if creds is falsy but does
not validate that individual required credential fields exist before accessing
them. Lines 54-56 use bracket notation to access creds["endpoint"],
creds["username"], and creds["password"] without checking if these keys are
present, which will raise KeyError on partially corrupted token data instead of
the intended RuntimeError. Fix this by adding validation after the creds check
to ensure endpoint, username, and password keys exist in the dictionary, and
raise RuntimeError with a descriptive message if any required field is missing.
Alternatively, use the .get() method with appropriate error handling for these
required fields, similar to how optional fields are handled for index_pattern,
verify_ssl, and max_retries.
In `@server/chat/backend/agent/tools/victorops_tool.py`:
- Around line 39-46: The _get_client function validates only whether creds
exists but then accesses creds["api_id"] and creds["api_key"] using bracket
notation on line 46, which raises KeyError if either key is missing from
partially corrupted token data. Replace the bracket notation with the .get()
method (following the pattern already used in is_victorops_connected at line 33)
to safely retrieve both the api_id and api_key values, returning None if any
required credential field is missing.
In `@server/chat/background/rca_prompt_builder.py`:
- Line 480: The build_rca_prompt function call in the VictorOps prompt builder
is passing arguments in the wrong order, causing alert_details to be mapped to
the title parameter and providers to be mapped to the payload parameter. This
results in payload being None, which later causes failures when payload.get(...)
is called. Fix the argument order in the build_rca_prompt call to correctly map
alert_details as the payload and providers as the appropriate parameter,
ensuring none of the arguments are None when the function executes.
In `@server/connectors/opensearch_connector/client.py`:
- Line 1: The module docstring for the OpenSearch client claims support for both
Basic auth and AWS SigV4, but the implementation only supports HTTP Basic auth.
Update the docstring to accurately reflect the supported authentication methods
by removing the reference to AWS SigV4 and keeping only the Basic auth
capability claim to prevent misleading integration expectations.
In `@server/main_compute.py`:
- Line 30: The root logger is configured with DEBUG level in the main_compute.py
entrypoint, which exposes verbose framework logs in production and violates
backend logging guidelines. Change the level parameter from logging.DEBUG to
logging.INFO in the root logger configuration while keeping the stdout handler
change intact. This ensures the backend maintains the standard INFO-level
logging for production environments.
In `@server/routes/opensearch/opensearch_routes.py`:
- Around line 21-26: The _get_creds function currently only checks the
requesting user's own token via get_token_data but does not fall back to
org-level shared credentials, which causes status/search/indices endpoints to
fail for other authorized users. Modify _get_creds to implement the standard
two-step credential lookup pattern: first attempt to retrieve the user's own
opensearch token via get_token_data(user_id, "opensearch"), and if that returns
None, fall back to an org-scoped database query for active secrets filtered by
org_id, provider = 'opensearch', is_active = TRUE, and secret_ref IS NOT NULL.
Return the org secret if found, otherwise return None.
- Around line 48-53: Add validation to ensure the JSON payload is a dictionary
and that string fields are actually strings before calling `.strip()`.
Specifically, after calling request.get_json(), validate that the result is a
dict; then for fields like endpoint, username, and indexPattern that require
`.strip()`, check that the retrieved value is a string before applying the
method. If validation fails, return a 400 Bad Request response with a
descriptive error message instead of allowing the code to proceed, which could
cause 500 errors when calling methods on non-string values.
- Around line 57-58: The current boolean parsing for verify_ssl silently treats
unknown string values as False instead of rejecting them, creating a security
risk where typos disable TLS verification. Modify the string parsing logic in
the isinstance check to validate that raw_verify_ssl.strip().lower() is
explicitly in the set of accepted true values ("1", "true", "yes", "on"), and
for any other string value (including unknown strings), raise an appropriate
HTTP 400 error to reject the invalid input rather than defaulting to False.
- Line 6: Remove the deprecated `Dict` import from the import statement on line
6, replacing it with the built-in `dict` type. Then update all type hints
throughout the file that use `Dict[str, Any]` (appearing at lines 21, 29, and
105) to use the modern syntax `dict[str, Any]` instead. Finally, remove the
redundant exception parameter `, exc` from the two logger.exception() calls at
lines 121 and 174, since the logging framework automatically captures the
traceback when using logger.exception().
In `@server/routes/victorops/victorops_routes.py`:
- Around line 130-138: The webhook validation in the victorops webhook endpoint
currently makes the secret validation optional when VICTOROPS_WEBHOOK_SECRET is
not configured, which allows forged events from anyone knowing a valid user_id.
Modify the webhook secret validation logic (the `if webhook_secret:` check and
the validation block) to fail closed by requiring the secret to be configured
and validated outside of dev/test environments. Instead of skipping validation
when the secret is empty, check the environment context and reject webhook
payloads if the secret is not configured in non-development/test environments,
while only permitting missing secrets in development/test modes.
---
Duplicate comments:
In `@client/src/app/victorops/auth/page.tsx`:
- Around line 48-65: The current implementation has a nested try-catch where
cache parsing errors in the inner block get caught by the outer catch handler,
preventing fetchAndUpdateStatus() from executing and leaving the page stuck with
stale cached data. Isolate the localStorage cache parsing logic (the JSON.parse
of cached data and setStatus call) into its own inner try-catch block within
loadStatus, so that if JSON.parse throws due to corrupted cache, it can be
handled independently without affecting the fetchAndUpdateStatus() call which
must always execute regardless of cache issues. Ensure fetchAndUpdateStatus()
remains in the outer try block so that only real fetch failures trigger the
error toast.
In `@server/chat/backend/agent/skills/integrations/opensearch/SKILL.md`:
- Around line 23-62: The markdown file has several section headings that violate
markdownlint rule MD022 by missing blank lines immediately after them. Insert
one blank line after each of the following headings: "## Overview", "### Tool
Usage (use in this order)", "### Common Query Patterns", "### Time Format", and
"## Important Rules". This will ensure proper markdown formatting and satisfy
the linter requirements.
In `@server/chat/backend/agent/skills/integrations/victorops/SKILL.md`:
- Around line 23-41: The markdown file has section headings without blank lines
immediately following them, which violates markdownlint rule MD022. In the
SKILL.md file, add one blank line after each of the following headings: "##
Overview", "## Instructions", "### Tool Usage", and "### RCA Workflow". This
ensures proper markdown formatting and satisfies the linter requirements.
In `@server/routes/victorops/tasks.py`:
- Around line 95-118: In the _record_primary_alert function's except block, add
a call to conn.rollback() after logging the exception to properly rollback the
aborted transaction on PostgreSQL. This prevents the transaction from remaining
in an aborted state, which would cause subsequent statements on the same
connection to fail. The same fix must also be applied to the exception handler
at the location referenced in "Also applies to: 208-240" to ensure all swallowed
database exceptions properly rollback their transactions.
🪄 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: 40507b10-614c-4668-9aeb-f74b636ade0f
⛔ Files ignored due to path filters (2)
client/public/opensearch.svgis excluded by!**/*.svgclient/public/victorops.svgis excluded by!**/*.svg
📒 Files selected for processing (35)
.env.exampleclient/src/app/api/connected-accounts/[provider]/route.tsclient/src/app/api/opensearch/[...path]/route.tsclient/src/app/api/victorops/route.tsclient/src/app/api/victorops/webhook-url/route.tsclient/src/app/opensearch/auth/page.tsxclient/src/app/victorops/auth/page.tsxclient/src/components/connectors/ConnectorRegistry.tsclient/src/components/victorops/VictorOpsConnectedView.tsxclient/src/components/victorops/VictorOpsConnectionStep.tsxclient/src/components/victorops/VictorOpsWebhookStep.tsxclient/src/lib/services/opensearch.tsclient/src/lib/services/victorops.tsdocker-compose.prod-local.ymldocker-compose.yamlserver/celery_config.pyserver/chat/backend/agent/skills/integrations/opensearch/SKILL.mdserver/chat/backend/agent/skills/integrations/victorops/SKILL.mdserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/backend/agent/tools/opensearch_tool.pyserver/chat/backend/agent/tools/victorops_tool.pyserver/chat/background/rca_prompt_builder.pyserver/connectors/opensearch_connector/__init__.pyserver/connectors/opensearch_connector/client.pyserver/main_compute.pyserver/routes/connector_status.pyserver/routes/opensearch/__init__.pyserver/routes/opensearch/opensearch_routes.pyserver/routes/victorops/__init__.pyserver/routes/victorops/tasks.pyserver/routes/victorops/victorops_helpers.pyserver/routes/victorops/victorops_routes.pyserver/utils/db/db_utils.pyserver/utils/providers.pyserver/utils/secrets/secret_ref_utils.py
🛑 Comments failed to post (11)
client/src/components/connectors/ConnectorRegistry.ts (1)
162-182:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the provider state events to the new registry entries.
The new connectors are registered without their provider-specific state event names, so event-triggered status revalidation cannot be wired consistently for these providers. Add the state event values here, and extend
ConnectorConfigif the type has not been updated yet.Proposed fix
this.register({ id: "opensearch", name: "OpenSearch", description: "Connect your OpenSearch cluster to search logs and traces during incident RCA. Aurora queries your logs to find error patterns correlated with alerts.", iconPath: "/opensearch.svg", iconBgColor: "bg-white dark:bg-white", category: "Observability", path: "/opensearch/auth", storageKey: "isOpenSearchConnected", + stateEvent: "opensearchStateChanged", }); this.register({ id: "victorops", name: "Splunk On-Call", description: "Connect Splunk On-Call (VictorOps) to receive real-time incident alerts. Automatically trigger AI-powered RCA when incidents fire.", iconPath: "/victorops.svg", iconBgColor: "bg-white dark:bg-white", category: "Incident Management", path: "/victorops/auth", storageKey: "isVictorOpsConnected", + stateEvent: "victoropsStateChanged", });As per coding guidelines, “Add connector provider to
ConnectorRegistry.tswith properstateEventname.”📝 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.this.register({ id: "opensearch", name: "OpenSearch", description: "Connect your OpenSearch cluster to search logs and traces during incident RCA. Aurora queries your logs to find error patterns correlated with alerts.", iconPath: "/opensearch.svg", iconBgColor: "bg-white dark:bg-white", category: "Observability", path: "/opensearch/auth", storageKey: "isOpenSearchConnected", stateEvent: "opensearchStateChanged", }); this.register({ id: "victorops", name: "Splunk On-Call", description: "Connect Splunk On-Call (VictorOps) to receive real-time incident alerts. Automatically trigger AI-powered RCA when incidents fire.", iconPath: "/victorops.svg", iconBgColor: "bg-white dark:bg-white", category: "Incident Management", path: "/victorops/auth", storageKey: "isVictorOpsConnected", stateEvent: "victoropsStateChanged", });🤖 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 `@client/src/components/connectors/ConnectorRegistry.ts` around lines 162 - 182, The opensearch and victorops connector registrations in the register method calls are missing the stateEvent property required for event-triggered status revalidation. Add a stateEvent field to both connector objects with appropriate provider-specific state event names (following the naming convention used by other connectors in the registry). If the ConnectorConfig type does not include a stateEvent field, extend that interface to support this property.Source: Coding guidelines
server/chat/backend/agent/tools/opensearch_tool.py (1)
46-60:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate individual credential fields to prevent KeyError on partial token data.
Lines 54-56 access
creds["endpoint"],creds["username"], andcreds["password"]with bracket notation after only checkingif not creds:. If stored token data is partially corrupted (e.g., missingusernameorpasswordkeys), this raisesKeyErrorinstead of the intendedRuntimeError, breaking the tool's error contract with the LLM.🛡️ Recommended fix
def _get_client(user_id: str): """Build an OpenSearchClient from stored credentials.""" from connectors.opensearch_connector.client import OpenSearchClient creds = get_token_data(user_id, "opensearch") - if not creds: + if not creds or not creds.get("endpoint") or not creds.get("username") or not creds.get("password"): raise RuntimeError("OpenSearch credentials not found. Please connect OpenSearch first.") return OpenSearchClient( endpoint=creds["endpoint"], username=creds["username"], password=creds["password"], index_pattern=creds.get("index_pattern", "*"), verify_ssl=creds.get("verify_ssl", True), max_retries=creds.get("max_retries", 2), )📝 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.def _get_client(user_id: str): """Build an OpenSearchClient from stored credentials.""" from connectors.opensearch_connector.client import OpenSearchClient creds = get_token_data(user_id, "opensearch") if not creds or not creds.get("endpoint") or not creds.get("username") or not creds.get("password"): raise RuntimeError("OpenSearch credentials not found. Please connect OpenSearch first.") return OpenSearchClient( endpoint=creds["endpoint"], username=creds["username"], password=creds["password"], index_pattern=creds.get("index_pattern", "*"), verify_ssl=creds.get("verify_ssl", True), max_retries=creds.get("max_retries", 2), )🧰 Tools
🪛 Ruff (0.15.17)
[warning] 46-46: Missing return type annotation for private function
_get_client(ANN202)
[warning] 52-52: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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/opensearch_tool.py` around lines 46 - 60, The _get_client function checks if creds is falsy but does not validate that individual required credential fields exist before accessing them. Lines 54-56 use bracket notation to access creds["endpoint"], creds["username"], and creds["password"] without checking if these keys are present, which will raise KeyError on partially corrupted token data instead of the intended RuntimeError. Fix this by adding validation after the creds check to ensure endpoint, username, and password keys exist in the dictionary, and raise RuntimeError with a descriptive message if any required field is missing. Alternatively, use the .get() method with appropriate error handling for these required fields, similar to how optional fields are handled for index_pattern, verify_ssl, and max_retries.server/chat/backend/agent/tools/victorops_tool.py (1)
39-46:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate individual credential fields to prevent KeyError on partial token data.
Line 46 accesses
creds["api_id"]andcreds["api_key"]with bracket notation after only checkingif not creds:. If stored token data is partially corrupted or missing either key, this raisesKeyErrorinstead of returningNone, breaking the tool's error-handling contract. Theis_victorops_connectedfunction (line 33) already demonstrates the correct pattern using.get().🛡️ Recommended fix
def _get_client(user_id: str): from utils.auth.token_management import get_token_data from routes.victorops.victorops_helpers import VictorOpsClient creds = get_token_data(user_id, "victorops") - if not creds: + if not creds or not creds.get("api_id") or not creds.get("api_key"): return None return VictorOpsClient(api_id=creds["api_id"], api_key=creds["api_key"])🧰 Tools
🪛 Ruff (0.15.17)
[warning] 39-39: Missing return type annotation for private function
_get_client(ANN202)
🤖 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/victorops_tool.py` around lines 39 - 46, The _get_client function validates only whether creds exists but then accesses creds["api_id"] and creds["api_key"] using bracket notation on line 46, which raises KeyError if either key is missing from partially corrupted token data. Replace the bracket notation with the .get() method (following the pattern already used in is_victorops_connected at line 33) to safely retrieve both the api_id and api_key values, returning None if any required credential field is missing.server/chat/background/rca_prompt_builder.py (1)
480-480:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
build_rca_promptargument order in VictorOps prompt builder.At Line 480,
build_rca_promptis called with the wrong parameter mapping (alert_detailsastitle,providersaspayload). With current call sites, this can passNoneas payload and fail onpayload.get(...), which prevents RCA task triggering.Suggested fix
- return build_rca_prompt("victorops", alert_details, providers, user_id) + return build_rca_prompt("victorops", alert_details["title"], payload, user_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/background/rca_prompt_builder.py` at line 480, The build_rca_prompt function call in the VictorOps prompt builder is passing arguments in the wrong order, causing alert_details to be mapped to the title parameter and providers to be mapped to the payload parameter. This results in payload being None, which later causes failures when payload.get(...) is called. Fix the argument order in the build_rca_prompt call to correctly map alert_details as the payload and providers as the appropriate parameter, ensuring none of the arguments are None when the function executes.server/connectors/opensearch_connector/client.py (1)
1-1:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix misleading auth capability claim in module docstring.
Line [1] states SigV4 support, but this implementation only performs HTTP Basic auth. Update the docstring to avoid incorrect integration expectations.
Suggested patch
-"""OpenSearch REST API client supporting Basic auth and AWS SigV4.""" +"""OpenSearch REST API client supporting HTTP Basic authentication."""📝 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."""OpenSearch REST API client supporting HTTP Basic authentication."""🤖 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/connectors/opensearch_connector/client.py` at line 1, The module docstring for the OpenSearch client claims support for both Basic auth and AWS SigV4, but the implementation only supports HTTP Basic auth. Update the docstring to accurately reflect the supported authentication methods by removing the reference to AWS SigV4 and keeping only the Basic auth capability claim to prevent misleading integration expectations.server/main_compute.py (1)
30-30:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the server root logger at INFO.
Setting the root logger to
DEBUGin the Flask entrypoint can expose verbose framework/app debug logs in production. Keep the stdout handler change, but restore the backend’s INFO-level default.Proposed fix
logging.basicConfig( - level=logging.DEBUG, + level=logging.INFO, format="%(asctime)s - %(name)s - %(levelname)s - %(message)s", handlers=[logging.StreamHandler(sys.stdout)], )As per coding guidelines, Python backend code should use logging at INFO level.
🤖 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/main_compute.py` at line 30, The root logger is configured with DEBUG level in the main_compute.py entrypoint, which exposes verbose framework logs in production and violates backend logging guidelines. Change the level parameter from logging.DEBUG to logging.INFO in the root logger configuration while keeping the stdout handler change intact. This ensures the backend maintains the standard INFO-level logging for production environments.Source: Coding guidelines
server/routes/opensearch/opensearch_routes.py (4)
6-6:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Verify the Ruff warnings reported for this file are resolved. ruff check server/routes/opensearch/opensearch_routes.py --select UP035,TRY401Repository: Arvo-AI/aurora
Length of output: 1896
🏁 Script executed:
cat -n server/routes/opensearch/opensearch_routes.py | head -35Repository: Arvo-AI/aurora
Length of output: 1492
🏁 Script executed:
sed -n '20,30p' server/routes/opensearch/opensearch_routes.pyRepository: Arvo-AI/aurora
Length of output: 411
🏁 Script executed:
sed -n '100,110p' server/routes/opensearch/opensearch_routes.pyRepository: Arvo-AI/aurora
Length of output: 468
🏁 Script executed:
sed -n '115,125p' server/routes/opensearch/opensearch_routes.pyRepository: Arvo-AI/aurora
Length of output: 507
🏁 Script executed:
sed -n '170,180p' server/routes/opensearch/opensearch_routes.pyRepository: Arvo-AI/aurora
Length of output: 701
Fix Ruff warnings: remove deprecated
typing.Dictand redundant exception parameters in logging calls.The file contains three Ruff violations:
- UP035 at line 6:
typing.Dictis deprecated; usedictinstead (affecting type hints at lines 21, 29, 105)- TRY401 at lines 121, 174: exception objects are redundant in
logger.exception()calls—the logging framework captures the traceback automaticallyApply the cleanup:
- Remove
Dictfrom the import statement at line 6- Replace all
Dict[str, Any]withdict[str, Any]at lines 21, 29, 105- Remove
, excfrom the exception logging calls at lines 121 and 174Proposed Ruff cleanup
-from typing import Any, Dict, Optional +from typing import Any, Optional @@ -def _get_creds(user_id: str) -> Optional[Dict[str, Any]]: +def _get_creds(user_id: str) -> Optional[dict[str, Any]]: @@ -def _make_client(creds: Dict[str, Any]) -> OpenSearchClient: +def _make_client(creds: dict[str, Any]) -> OpenSearchClient: @@ - token_payload: Dict[str, Any] = { + token_payload: dict[str, Any] = { @@ - except Exception as exc: - logger.exception("[OPENSEARCH] Failed to store credentials for %s: %s", sanitize(user_id), exc) + except Exception: + logger.exception("[OPENSEARCH] Failed to store credentials for %s", sanitize(user_id)) @@ - except Exception as exc: - logger.exception("[OPENSEARCH] Disconnect failed for %s: %s", sanitize(user_id), exc) + except Exception: + logger.exception("[OPENSEARCH] Disconnect failed for %s", sanitize(user_id))🧰 Tools
🪛 Ruff (0.15.17)
[warning] 6-6:
typing.Dictis deprecated, usedictinstead(UP035)
🤖 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/routes/opensearch/opensearch_routes.py` at line 6, Remove the deprecated `Dict` import from the import statement on line 6, replacing it with the built-in `dict` type. Then update all type hints throughout the file that use `Dict[str, Any]` (appearing at lines 21, 29, and 105) to use the modern syntax `dict[str, Any]` instead. Finally, remove the redundant exception parameter `, exc` from the two logger.exception() calls at lines 121 and 174, since the logging framework automatically captures the traceback when using logger.exception().Source: Linters/SAST tools
21-26:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve the org-level credential fallback in
_get_creds.This helper only checks the requesting user’s token, so org-shared OpenSearch credentials can appear disconnected and make
status/search/indicesfail for other authorized users. Mirror the established provider pattern: user token first, then an org-scoped active secret lookup filtered byorg_id,provider = 'opensearch',is_active = TRUE, andsecret_ref IS NOT NULL.Based on learnings, connector route helpers are expected to use “the standard two-step credential lookup: (1) check the requesting user’s own token first via
get_token_data(user_id, provider), then (2) fall back to an org-scoped DB query”.🤖 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/routes/opensearch/opensearch_routes.py` around lines 21 - 26, The _get_creds function currently only checks the requesting user's own token via get_token_data but does not fall back to org-level shared credentials, which causes status/search/indices endpoints to fail for other authorized users. Modify _get_creds to implement the standard two-step credential lookup pattern: first attempt to retrieve the user's own opensearch token via get_token_data(user_id, "opensearch"), and if that returns None, fall back to an org-scoped database query for active secrets filtered by org_id, provider = 'opensearch', is_active = TRUE, and secret_ref IS NOT NULL. Return the org secret if found, otherwise return None.Source: Learnings
48-53:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate JSON shape and string fields before
.strip()or client calls.A truthy non-object JSON body, or values like
{"endpoint": null}/{"query": 123}, can raise 500s before returning validation errors. Reject non-dict payloads and non-string fields with 400s.Proposed validation hardening
- data = request.get_json(force=True, silent=True) or {} + data = request.get_json(force=True, silent=True) or {} + if not isinstance(data, dict): + return jsonify({"error": "JSON body must be an object"}), 400 + for field in ("endpoint", "username", "password", "indexPattern"): + if field in data and not isinstance(data[field], str): + return jsonify({"error": f"{field} must be a string"}), 400 raw_endpoint = data.get("endpoint", "").strip() username = data.get("username", "").strip() password = data.get("password", "") index_pattern = data.get("indexPattern", "*").strip() or "*"- data = request.get_json(force=True, silent=True) or {} + data = request.get_json(force=True, silent=True) or {} + if not isinstance(data, dict): + return jsonify({"error": "JSON body must be an object"}), 400 + for field in ("query", "index", "startTime", "endTime", "timestampField"): + if field in data and data[field] is not None and not isinstance(data[field], str): + return jsonify({"error": f"{field} must be a string"}), 400 query = data.get("query", "").strip()Also applies to: 190-202
🤖 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/routes/opensearch/opensearch_routes.py` around lines 48 - 53, Add validation to ensure the JSON payload is a dictionary and that string fields are actually strings before calling `.strip()`. Specifically, after calling request.get_json(), validate that the result is a dict; then for fields like endpoint, username, and indexPattern that require `.strip()`, check that the retrieved value is a string before applying the method. If validation fails, return a 400 Bad Request response with a descriptive error message instead of allowing the code to proceed, which could cause 500 errors when calling methods on non-string values.
57-58:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject unknown
verifySslstrings instead of treating them asFalse.The current parser stores
verify_ssl=Falsefor typos like"treu"or arbitrary strings, silently disabling TLS verification. Accept explicit true/false spellings or return 400.Proposed strict boolean parsing
if isinstance(raw_verify_ssl, bool): verify_ssl = raw_verify_ssl elif isinstance(raw_verify_ssl, str): - verify_ssl = raw_verify_ssl.strip().lower() in {"1", "true", "yes", "on"} + normalized_verify_ssl = raw_verify_ssl.strip().lower() + if normalized_verify_ssl in {"1", "true", "yes", "on"}: + verify_ssl = True + elif normalized_verify_ssl in {"0", "false", "no", "off"}: + verify_ssl = False + else: + return jsonify({"error": "verifySsl must be a boolean"}), 400 else: return jsonify({"error": "verifySsl must be a boolean"}), 400🤖 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/routes/opensearch/opensearch_routes.py` around lines 57 - 58, The current boolean parsing for verify_ssl silently treats unknown string values as False instead of rejecting them, creating a security risk where typos disable TLS verification. Modify the string parsing logic in the isinstance check to validate that raw_verify_ssl.strip().lower() is explicitly in the set of accepted true values ("1", "true", "yes", "on"), and for any other string value (including unknown strings), raise an appropriate HTTP 400 error to reject the invalid input rather than defaulting to False.server/routes/victorops/victorops_routes.py (1)
130-138:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFail closed when the webhook secret is not configured.
Because
/victorops/webhook/bypassesX-Internal-Secret, leavingVICTOROPS_WEBHOOK_SECRETempty makes this endpoint accept forged events from anyone who knows a validuser_id. Require the secret outside dev/test before accepting webhook payloads.Proposed fix
webhook_secret = os.getenv("VICTOROPS_WEBHOOK_SECRET", "") + if not webhook_secret and os.getenv("AURORA_ENV", "production") != "dev": + logger.error("[VICTOROPS] Webhook secret is not configured; rejecting external webhook") + return jsonify({"error": "Webhook authentication is not configured"}), 503 + if webhook_secret: provided = request.headers.get("X-Webhook-Secret", "") if not provided or not hmac.compare_digest(provided, webhook_secret): logger.warning("[VICTOROPS] Webhook rejected: invalid or missing X-Webhook-Secret for user %s", sanitize(user_id)) return jsonify({"error": "Forbidden"}), 403📝 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.# Optional shared-secret validation. Set VICTOROPS_WEBHOOK_SECRET in the # environment and configure the same value as a custom header # X-Webhook-Secret in Splunk On-Call's outbound webhook settings. webhook_secret = os.getenv("VICTOROPS_WEBHOOK_SECRET", "") if not webhook_secret and os.getenv("AURORA_ENV", "production") != "dev": logger.error("[VICTOROPS] Webhook secret is not configured; rejecting external webhook") return jsonify({"error": "Webhook authentication is not configured"}), 503 if webhook_secret: provided = request.headers.get("X-Webhook-Secret", "") if not provided or not hmac.compare_digest(provided, webhook_secret): logger.warning("[VICTOROPS] Webhook rejected: invalid or missing X-Webhook-Secret for user %s", sanitize(user_id)) return jsonify({"error": "Forbidden"}), 403🤖 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/routes/victorops/victorops_routes.py` around lines 130 - 138, The webhook validation in the victorops webhook endpoint currently makes the secret validation optional when VICTOROPS_WEBHOOK_SECRET is not configured, which allows forged events from anyone knowing a valid user_id. Modify the webhook secret validation logic (the `if webhook_secret:` check and the validation block) to fail closed by requiring the secret to be configured and validated outside of dev/test environments. Instead of skipping validation when the secret is empty, check the environment context and reject webhook payloads if the secret is not configured in non-development/test environments, while only permitting missing secrets in development/test modes.
Contributor
|
Hi! Thanks for your contribution. Before we can merge this, we need you to sign our Contributor License Agreement (CLA) for legal purposes. This is a one-time requirement for external contributors — it ensures that contributions are properly licensed and that both parties are protected. I'll send the document separately. Once signed, we're good to go on this and any future PRs. |
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
Changes
Backend
server/connectors/opensearch_connector/— cluster client (connect, health check, search, list indices)server/routes/opensearch/— REST routes: connect, disconnect, status, search, indices (RBAC enforced)server/routes/victorops/— REST routes + Celery task for async webhook ingestionserver/chat/backend/agent/tools/opensearch_tool.py—search_opensearch,list_opensearch_indicesLangChain toolsserver/chat/backend/agent/tools/victorops_tool.py—get_victorops_incidents,get_victorops_teamsLangChain toolsserver/chat/backend/agent/skills/integrations/opensearch/SKILL.md— RCA guidance skill (rca_priority: 3)server/chat/backend/agent/skills/integrations/victorops/SKILL.md— incident context skill (rca_priority: 4)main_compute.py,connector_status.py,cloud_tools.py,providers.pyFrontend
client/src/app/opensearch/auth/— connect/disconnect pageclient/src/app/victorops/auth/— connect/disconnect page + webhook URL setup stepclient/src/components/victorops/— VictorOpsConnectionStep, VictorOpsConnectedView, VictorOpsWebhookStepclient/src/lib/services/— opensearch.ts, victorops.ts API service helpersConnectorRegistry.tsTest plan
search_opensearch,list_opensearch_indices)server/tests/architectural/test_connector_rbac.py)Summary by CodeRabbit
VICTOROPS_WEBHOOK_SECRETconfiguration and ensured webhook processing tasks are loaded.