feat: incident.io connector - #287
Merged
Merged
Conversation
- Routes: connect (API key validation), status, disconnect, webhook, alerts list, webhook-url, RCA settings (with postback toggle) - Celery tasks: webhook event processing with upsert on incident_id, alert correlation, incident creation, RCA pipeline, post-back task - Agent tools: list_incidentio_incidents, get_incidentio_incident, get_incidentio_timeline for RCA investigation - RCA prompt builder adapter for incident.io event payloads - DB: incidentio_alerts table with RLS, unique index on (org_id, incident_id) - Wiring: secret_ref_utils, main_compute, cloud_tools, task.py _RCA_SOURCES Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Auth page with API key connection flow and status caching - Webhook step component with RCA toggle, post-back toggle, webhook URL copy - Incidents list page with severity/status badges and pagination - Service layer (incident-io.ts) with typed API client - Next.js API proxy routes: connect, status, alerts, webhook-url, rca-settings - ConnectorRegistry registration (Incident Management category) - Connected-accounts disconnect handler - SVG icon asset Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Registers incidentio as an integration skill so SkillRegistry can detect connection status and expose tools during RCA. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Register Celery task so webhooks are processed (celery_config.py) - Add Next.js webhook proxy route with public middleware bypass - Route webhook URLs through frontend proxy (ngrok/production) - Fix source display names (incident.io, PagerDuty, OpsGenie, etc.) - Fetch raw payload for incident.io alerts in incident detail view - Replace logo SVG with official incident.io flame mark - Add incident.io icon for tool call widgets (CommandLogo) - Add fuzzy matching for skill IDs to handle LLM variations - Improve skill index format to show exact load_skill() calls - Better error messages when LLM tries load_skill on cloud providers - Add incidentio to RBAC connector test coverage - Improve error logging in incident.io API client - Redesign auth page with logo, stepper UI, clearer instructions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a full incident.io integration across frontend (pages, components, client API), backend (Flask blueprint, DB schema, Celery tasks), agent tools/skills, and wiring (CORS, secrets, connector registry, middleware tweak). Changes
Sequence Diagram(s)sequenceDiagram
participant User as Browser
participant Client as Frontend App
participant ClientAPI as Next API (/api/incident-io)
participant Server as Backend (/incidentio)
participant DB as Database/Vault
participant IncidentIO as incident.io API
rect rgba(100,200,100,0.5)
User->>Client: Submit API key
Client->>ClientAPI: POST /api/incident-io/connect
ClientAPI->>Server: POST /incidentio/connect
Server->>IncidentIO: Verify API key (list_incidents)
IncidentIO-->>Server: OK
Server->>DB: Store api_key / webhook_secret
Server-->>ClientAPI: {connected:true}
ClientAPI-->>Client: Update UI / localStorage
end
rect rgba(100,150,200,0.5)
IncidentIO->>Server: POST /incidentio/alerts/webhook/<user_id>
Server->>Server: Validate HMAC, parse payload
Server->>DB: Upsert incidentio_alerts
Server->>Server: Enqueue process_incidentio_event
end
rect rgba(200,150,100,0.5)
Server->>Server: process_incidentio_event (normalize, correlate)
alt RCA enabled
Server->>Server: Create incident, enqueue RCA generation
Server->>Server: Schedule postback_rca_to_incidentio
Server->>IncidentIO: POST timeline update (RCA)
end
end
rect rgba(150,100,200,0.5)
User->>Client: View /incident-io/incidents
Client->>ClientAPI: GET /api/incident-io/alerts
ClientAPI->>Server: GET /incidentio/alerts
Server->>DB: Query incidentio_alerts (org scope, pagination)
Server-->>ClientAPI: {alerts, total, limit, offset}
ClientAPI-->>Client: Render incident list
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/middleware.ts (1)
21-33:⚠️ Potential issue | 🟠 MajorAvoid making
webhook-urlpublic via prefix matching.Because public routes are checked with
startsWith, Line 21 also matches/api/incident-io/alerts/webhook-url. That endpoint should remain authenticated unless it performs its own auth checks.Suggested fix
- "/api/incident-io/alerts/webhook", // incident.io webhook POSTs + "/api/incident-io/alerts/webhook/", // incident.io webhook POSTs with userIdOr make the public-route matcher path-segment aware:
- const isPublicRoute = publicRoutes.some(route => - nextUrl.pathname.startsWith(route) - ) + const isPublicRoute = publicRoutes.some(route => + nextUrl.pathname === route || nextUrl.pathname.startsWith(`${route}/`) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/middleware.ts` around lines 21 - 33, The current public route check in the default export auth((req) => ...) uses startsWith which incorrectly treats "/api/incident-io/alerts/webhook" as matching longer paths like "/api/incident-io/alerts/webhook-url"; update the isPublicRoute logic (which references publicRoutes and is used inside the exported auth middleware) to be path-segment aware by checking either exact equality or a segment boundary, e.g. treat a route as public only if nextUrl.pathname === route or nextUrl.pathname.startsWith(route + '/'), so only the intended route (not prefixes) is exposed as public.
🧹 Nitpick comments (10)
server/utils/db/db_utils.py (1)
859-866: Add org-scoped indexes for the Incident.io alerts list path.The current
(user_id, received_at)and single-columnseverityindexes do not fit org-scoped list queries ordered by recency. Add anorg_id+received_atindex, and optionally includeseverityfor filtered lists.Proposed index additions
CREATE UNIQUE INDEX IF NOT EXISTS idx_incidentio_alerts_org_incident ON incidentio_alerts(org_id, incident_id); + CREATE INDEX IF NOT EXISTS idx_incidentio_alerts_org_received + ON incidentio_alerts(org_id, received_at DESC); + CREATE INDEX IF NOT EXISTS idx_incidentio_alerts_org_severity_received + ON incidentio_alerts(org_id, severity, received_at DESC); CREATE INDEX IF NOT EXISTS idx_incidentio_alerts_user_id ON incidentio_alerts(user_id, received_at DESC); CREATE INDEX IF NOT EXISTS idx_incidentio_alerts_status ON incidentio_alerts(incident_status); CREATE INDEX IF NOT EXISTS idx_incidentio_alerts_severity ON incidentio_alerts(severity);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/db/db_utils.py` around lines 859 - 866, The current indexes on incidentio_alerts (idx_incidentio_alerts_user_id, idx_incidentio_alerts_severity) do not support org-scoped list queries ordered by recency; add a composite index on (org_id, received_at DESC) (e.g. create index IF NOT EXISTS idx_incidentio_alerts_org_received_at ON incidentio_alerts(org_id, received_at DESC)) to support ORDER BY received_at for org-scoped queries, and optionally add a covering index including severity (e.g. idx_incidentio_alerts_org_received_at_severity ON incidentio_alerts(org_id, received_at DESC, severity)) to accelerate org+severity filtered lists; update migration where other indexes (idx_incidentio_alerts_org_incident, idx_incidentio_alerts_user_id, idx_incidentio_alerts_status, idx_incidentio_alerts_severity) are defined.client/src/app/api/incident-io/alerts/webhook/[userId]/route.ts (1)
14-21: Consider adding a fetch timeout like the other incident.io proxy routes.The webhook proxy uses a bare
fetchwith noAbortController/timeout, whereas sibling routes (status, webhook-url) use a 15s timeout. Since incident.io will retry failed webhooks, a hung upstream connection here can tie up the Node.js proxy unnecessarily. Also,await response.json()will throw on empty/non-JSON bodies and get mapped to a 502 (losing the actual upstream status) — consider forwarding text or tolerating empty bodies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/api/incident-io/alerts/webhook/`[userId]/route.ts around lines 14 - 21, The current POST proxy in route.ts uses a bare fetch and then awaits response.json(), which can hang and throw on empty/non-JSON bodies; update the fetch to use an AbortController with a 15s timeout (match other incident.io proxy routes) and ensure the response handling tolerates empty or non-JSON bodies by reading response.text(), attempting JSON.parse only if non-empty/valid JSON, and otherwise forwarding the raw text (or empty string) in NextResponse.json/NextResponse.text while preserving response.status; adjust the logic around the fetch call and the NextResponse.json(response body, { status: response.status }) return to use this safer pattern and make sure to clear the timeout/abort controller appropriately.server/chat/backend/agent/skills/integrations/incidentio/SKILL.md (1)
10-16: Redundantallowed-toolsfrontmatter key.Line 16's
allowed-toolsduplicates thetools:list on lines 10–13 and isn't a recognized loader field (vs. thetoolslist consumed bySkillMetadata). Either drop it or document its purpose to avoid drift if the tool set changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/chat/backend/agent/skills/integrations/incidentio/SKILL.md` around lines 10 - 16, The frontmatter contains a redundant allowed-tools key that duplicates the tools list and isn't consumed by SkillMetadata; remove the allowed-tools line from SKILL.md (or if you intend it to serve a different purpose, document that purpose and keep it in sync) and ensure the canonical tools are declared only under the tools key so SkillMetadata (and any loaders) use a single source of truth; check references to allowed-tools and update any docs or consumers to rely on tools instead.client/src/app/incident-io/auth/page.tsx (2)
25-69: Consider caching only the minimal{ connected }shape.
fetchAndUpdateStatuswrites the fullIncidentIoStatus(which may include a transienterrorstring from the backend) intolocalStorage. On next load,loadStatusrestores thaterrorinto component state, potentially surfacing a stale error UI. Per the established connector caching pattern (PR#164learning), persist only the minimal non-sensitive connection flags.♻️ Proposed change
- setStatus(result); if (typeof window !== "undefined") { - localStorage.setItem(CACHE_KEY, JSON.stringify(result)); + localStorage.setItem( + CACHE_KEY, + JSON.stringify({ connected: result.connected }), + ); if (result.connected) {Apply the same in
handleConnectwhen writing tolocalStorage.Based on learnings: PR
#164minimal-CachedStatuspattern — persist only non-sensitive connection flags, not transient error strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/incident-io/auth/page.tsx` around lines 25 - 69, The code currently persists the entire IncidentIoStatus (including transient error strings) to localStorage in fetchAndUpdateStatus and restores it in loadStatus, causing stale errors to reappear; change the caching behavior so that fetchAndUpdateStatus (and handleConnect where you write to localStorage) only persist the minimal shape { connected } (and optional non-sensitive flags if needed) to CACHE_KEY, while still calling setStatus(result) with the full result in-memory; update loadStatus to parse and apply only the { connected } cached shape (and derive wasCachedConnected from that) instead of restoring any error fields from localStorage.
93-93: Preferunknownoveranyin catch clauses.Under strict TS the project uses elsewhere, catch variables should be typed
unknown(with narrowing) rather thananyto avoid bypassing type checks.- } catch (err: any) { + } catch (err: unknown) { console.error("incident.io connection failed", err);
getUserFriendlyErroralready acceptsunknown-compatible input elsewhere in the codebase; verify the signature before applying.As per coding guidelines: "Use strict mode in TypeScript, ESLint with next/core-web-vitals rules…"
Also applies to: 132-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/incident-io/auth/page.tsx` at line 93, Replace the catch variable typing from "err: any" to "err: unknown" in the try/catch blocks (the catch at the location where the code calls getUserFriendlyError and the similar one around line 132); then narrow the error before using it—e.g., check "err instanceof Error" or otherwise type-guard before accessing properties or passing to functions—note that getUserFriendlyError accepts unknown-compatible input so you can pass the narrowed value or the unknown directly after proper guarding in the functions handling user-facing messages.client/src/lib/services/incident-io.ts (1)
93-115: Error-swallowing inupdateRcaSettingsbreaks caller error flows.
getRcaSettings,getWebhookUrl, and especiallyupdateRcaSettingsconvert failures into anullreturn. This is the root cause of the success-toast-on-failure bug inIncidentIoWebhookStep.tsx(see comment on that file), since callers ofupdateRcaSettingstypically usetry/catchto detect failure.Either:
- Re-throw in
updateRcaSettingsso callers can distinguish success/failure via exceptions (consistent withconnect()above), or- Keep
null-on-error but document it clearly and require every call site toif (!result)handle the failure.Option 1 aligns with the symmetric usage pattern of
connect()and would have prevented the downstream UI bug.♻️ Proposed change (option 1)
- async updateRcaSettings(settings: Partial<IncidentIoRcaSettings>): Promise<IncidentIoRcaSettings | null> { - try { - return await apiRequest<IncidentIoRcaSettings>(`${API_BASE}/rca-settings`, { - method: 'PUT', - body: JSON.stringify(settings), - cache: 'no-store', - }); - } catch (error) { - console.error('[incidentIoService] Failed to update RCA settings:', error); - return null; - } - }, + async updateRcaSettings(settings: Partial<IncidentIoRcaSettings>): Promise<IncidentIoRcaSettings> { + const raw = await apiRequest<IncidentIoRcaSettings>(`${API_BASE}/rca-settings`, { + method: 'PUT', + body: JSON.stringify(settings), + cache: 'no-store', + }); + if (!raw) throw new Error('Empty response from rca-settings'); + return raw; + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/lib/services/incident-io.ts` around lines 93 - 115, The updateRcaSettings function currently swallows errors and returns null which breaks caller try/catch flows; change updateRcaSettings to re-throw the caught error after logging (i.e., catch + processLogger/console.error then throw error) so callers can detect failures via exceptions consistent with connect(); apply the same pattern to getRcaSettings and getWebhookUrl for consistency (or alternatively document-and-enforce null-on-error everywhere), and ensure the apiRequest usage and return types (IncidentIoRcaSettings | null) are updated if you keep throwing so callers no longer rely on null checks.server/chat/backend/agent/tools/incidentio_tool.py (1)
94-132: Minor: redundantisinstancecheck.
severity = incident.get("severity") or {}andinc_type = incident.get("incident_type") or {}always produce a dict, making theisinstance(..., dict)guards on lines 123-124 dead branches. Either drop the guards or don't coerce to{}upstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/chat/backend/agent/tools/incidentio_tool.py` around lines 94 - 132, The code in _format_incident_summary sets severity = incident.get("severity") or {} and inc_type = incident.get("incident_type") or {}, so the subsequent isinstance(..., dict) checks are unnecessary; simplify by removing the isinstance guards and directly use severity.get("name") and inc_type.get("name") (or fallback to str(...) only if you choose not to coerce upstream), i.e., keep the current coercion to {} and change the "severity" and "type" fields to use severity.get("name", str(severity)) and inc_type.get("name", str(inc_type)) or simply severity.get("name") / inc_type.get("name") with your preferred fallback, updating _format_incident_summary accordingly.server/routes/incidentio/tasks.py (2)
338-349: Swallowed exception onrca_celery_task_idupdate — at least log it.The best-effort update of
incidents.rca_celery_task_idsilently discards any DB error, so a failure here means the incident row never records the task id and cancellation support won't work — with no trace to diagnose. Ruff also flags this (S110), and it conflicts with theserver/**/*.pyguideline to use try/except with logging.♻️ Proposed fix
- except Exception: - pass + except Exception as exc: + logger.warning("[INCIDENTIO] Failed to persist rca_celery_task_id for incident %s: %s", incident_id, exc)As per coding guidelines: "Implement Flask error handlers and use try/except with logging in Python code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/incidentio/tasks.py` around lines 338 - 349, The DB update for incidents.rca_celery_task_id (using db_pool.get_admin_connection() and cursor.execute(...) with task.id and incident_id) is swallowing all exceptions; replace the bare except: pass with a try/except that logs the error (e.g., logger.exception(...) or current_app.logger.exception(...)) including the exception/traceback and context (task.id and incident_id) so failures to set rca_celery_task_id are recorded for debugging.
361-410: Postback retry window is likely too short; also replace string-basedRetrydetection.Two related concerns in
postback_rca_to_incidentio:
With
max_retries=2anddefault_retry_delay=120, the task will give up after roughly0s → 120s → 240s(~4 minutes). RCA background chats can easily exceed this for non-trivial incidents, so the postback will silently be skipped (line 388 logs, but nothing is posted back). Consider increasingmax_retries(e.g. 5–10) and/or raising the countdown, or keying the poll off the summarization-complete signal instead of a blind poll.Lines 407–410 catch
Exceptionand try to distinguish Celery'sRetryvia"retry" in str(type(exc).__name__).lower(). This works today but is fragile (any future renaming or subclassing breaks it) and also triggers Ruff B904. Prefer an explicitexcept Retry: raisebefore the broad handler, which also lets you useraise ... from exccleanly.♻️ Proposed cleaner structure
-@celery_app.task( - bind=True, max_retries=2, default_retry_delay=120, name="incidentio.postback_rca" -) +@celery_app.task( + bind=True, max_retries=6, default_retry_delay=120, name="incidentio.postback_rca" +) def postback_rca_to_incidentio( self, user_id: str, aurora_incident_id: str, incidentio_incident_id: str, ) -> None: """Post RCA results back to incident.io timeline once analysis completes.""" + from celery.exceptions import Retry try: ... - except Exception as exc: - if "retry" not in str(type(exc).__name__).lower(): - logger.exception("[INCIDENTIO] Postback failed: %s", exc) - raise self.retry(exc=exc) - raise + except Retry: + raise + except Exception as exc: + logger.exception("[INCIDENTIO] Postback failed: %s", exc) + raise self.retry(exc=exc) from excPlease confirm the typical upper bound of RCA completion time in your environment so the retry budget can be tuned accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/incidentio/tasks.py` around lines 361 - 410, The postback_rca_to_incidentio task uses a very small retry window (max_retries=2, default_retry_delay=120) and a fragile string-based detection of Celery retries; increase the retry budget (e.g., set max_retries to 6–10 and/or default_retry_delay to a larger value like 300, or better yet switch to event/signal-based polling) and replace the string-check with an explicit Retry exception handler: import Retry from celery.exceptions, add an except Retry: raise block before the broad except Exception, then in the except Exception block log the error and call self.retry(exc=exc, countdown=...) (use raise ... from exc if re-raising) so retry semantics are explicit and robust; locate these changes in postback_rca_to_incidentio, updating max_retries/default_retry_delay, the self.retry calls, and the exception handling.server/routes/incidentio/incidentio_routes.py (1)
81-83: Strayfprefix on a format-less string.
f"/incident_updates"has no placeholders — drop thef(Ruff F541).- return self._request("GET", f"/incident_updates", params={"incident_id": incident_id}).json() + return self._request("GET", "/incident_updates", params={"incident_id": incident_id}).json()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/incidentio/incidentio_routes.py` around lines 81 - 83, In get_incident_updates, remove the unnecessary f-string prefix from the literal f"/incident_updates" (it has no placeholders); change the call in method get_incident_updates (which calls self._request("GET", f"/incident_updates", params=...)) to use a plain string "/incident_updates" to satisfy Ruff F541.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/components/connectors/ConnectorRegistry.ts`:
- Around line 150-161: The registry entry created via this.register for the
connector with id "incidentio" is missing a stateEvent property, so add a
stateEvent to that registration (in the same object passed to
ConnectorRegistry.register) to enable event-driven revalidation; use a unique,
consistent event name such as "incidentio:state_changed" (or follow the existing
pattern used by other providers, e.g., "<id>:state_changed") tied to the
storageKey "isIncidentIoConnected" so connect/disconnect emits the expected
event.
In `@client/src/components/incident-io/IncidentIoWebhookStep.tsx`:
- Around line 64-111: The success toasts in handleRcaToggle and
handlePostbackToggle are firing even when incidentIoService.updateRcaSettings
returns null (it swallows errors), so change both handlers to only show the
success toast when result is truthy; if result is null, do not show the success
toast and instead show the destructive failure toast (same content used in the
existing catch blocks). Locate handleRcaToggle and handlePostbackToggle and wrap
the toast-success calls inside the existing "if (result) { ... }" blocks (and
add an else branch to toast the failure) so the UI only reports success when the
service actually returned a result.
In `@server/chat/backend/agent/skills/load_skill_tool.py`:
- Around line 67-76: The fuzzy-match in load_skill_tool.py is too permissive:
tighten the normalization/matching in the block that iterates
registry.get_all_skill_ids() so it only accepts exact-equality after
normalization or a startswith match when both normalized strings meet a minimum
length (e.g., len(normalized) >= 4 and len(candidate_norm) >= 4); also ensure
deterministic tie-breaking when multiple candidates match (for example, pick the
exact-equal first, otherwise choose the lexicographically smallest or shortest
candidate) and keep the existing logger.info("load_skill fuzzy matched '%s' ->
'%s'", skill_id, candidate) when a match is accepted.
In `@server/chat/backend/agent/tools/incidentio_tool.py`:
- Around line 152-156: _truncate_output currently slices the raw JSON string and
appends an invalid suffix, producing malformed JSON; change it so the function
returns valid JSON by wrapping the (possibly truncated) serialized content in a
new JSON object with explicit fields (e.g., {"truncated": true/false, "output":
"<escaped string>"}), reserve space for the wrapper when truncating (use
MAX_OUTPUT_SIZE minus overhead), and perform json.dumps once on that wrapper;
update references to _truncate_output and MAX_OUTPUT_SIZE accordingly so
downstream consumers always receive parseable JSON.
- Around line 66-91: The v2 API requires bracketed filter keys, so in the
request-building code inside list_incidentio_incidents (where params are
populated) replace any use of params["status"] with
params["status_category[one_of]"] (values like "live", "closed", "declined") and
replace params["severity"] with params["severity[one_of]"] (severity IDs); keep
the rest of the request logic and the /incident_updates with incident_id
unchanged so filtering is sent correctly to _api_request.
In `@server/chat/background/rca_prompt_builder.py`:
- Around line 1221-1261: The code sets severity = severity_obj.get("name",
"unknown") which makes severity always truthy and causes "unknown" to be
injected into status and labels; change the default to an empty string (use
severity_obj.get("name", "") or equivalent) and only append the severity text to
status and set alert_details['labels']['severity'] when severity is non-empty
(mirror the existing inc_type guard). Update the logic around status
construction (the f"{status} (severity: {severity})" expression) to include the
"(severity: ...)" suffix conditionally when severity is truthy, and keep
references to severity_obj, severity, alert_details and labels to locate the
changes.
In `@server/routes/incidentio/incidentio_routes.py`:
- Around line 201-235: The get_alerts handler accepts unbounded/negative limit
and offset from request.args (variables limit and offset) which are passed
directly into the SQL LIMIT/OFFSET, enabling expensive/full-table scans;
validate and clamp these values before use: enforce non-negative offset and a
reasonable max for limit (e.g., max_limit constant or config), coerce non-int
inputs to defaults, and replace the current use of limit/offset with the
sanitized values (refer to get_alerts, the variables limit and offset, and the
db cursor.execute calls that supply LIMIT %s OFFSET %s).
---
Outside diff comments:
In `@client/src/middleware.ts`:
- Around line 21-33: The current public route check in the default export
auth((req) => ...) uses startsWith which incorrectly treats
"/api/incident-io/alerts/webhook" as matching longer paths like
"/api/incident-io/alerts/webhook-url"; update the isPublicRoute logic (which
references publicRoutes and is used inside the exported auth middleware) to be
path-segment aware by checking either exact equality or a segment boundary, e.g.
treat a route as public only if nextUrl.pathname === route or
nextUrl.pathname.startsWith(route + '/'), so only the intended route (not
prefixes) is exposed as public.
---
Nitpick comments:
In `@client/src/app/api/incident-io/alerts/webhook/`[userId]/route.ts:
- Around line 14-21: The current POST proxy in route.ts uses a bare fetch and
then awaits response.json(), which can hang and throw on empty/non-JSON bodies;
update the fetch to use an AbortController with a 15s timeout (match other
incident.io proxy routes) and ensure the response handling tolerates empty or
non-JSON bodies by reading response.text(), attempting JSON.parse only if
non-empty/valid JSON, and otherwise forwarding the raw text (or empty string) in
NextResponse.json/NextResponse.text while preserving response.status; adjust the
logic around the fetch call and the NextResponse.json(response body, { status:
response.status }) return to use this safer pattern and make sure to clear the
timeout/abort controller appropriately.
In `@client/src/app/incident-io/auth/page.tsx`:
- Around line 25-69: The code currently persists the entire IncidentIoStatus
(including transient error strings) to localStorage in fetchAndUpdateStatus and
restores it in loadStatus, causing stale errors to reappear; change the caching
behavior so that fetchAndUpdateStatus (and handleConnect where you write to
localStorage) only persist the minimal shape { connected } (and optional
non-sensitive flags if needed) to CACHE_KEY, while still calling
setStatus(result) with the full result in-memory; update loadStatus to parse and
apply only the { connected } cached shape (and derive wasCachedConnected from
that) instead of restoring any error fields from localStorage.
- Line 93: Replace the catch variable typing from "err: any" to "err: unknown"
in the try/catch blocks (the catch at the location where the code calls
getUserFriendlyError and the similar one around line 132); then narrow the error
before using it—e.g., check "err instanceof Error" or otherwise type-guard
before accessing properties or passing to functions—note that
getUserFriendlyError accepts unknown-compatible input so you can pass the
narrowed value or the unknown directly after proper guarding in the functions
handling user-facing messages.
In `@client/src/lib/services/incident-io.ts`:
- Around line 93-115: The updateRcaSettings function currently swallows errors
and returns null which breaks caller try/catch flows; change updateRcaSettings
to re-throw the caught error after logging (i.e., catch +
processLogger/console.error then throw error) so callers can detect failures via
exceptions consistent with connect(); apply the same pattern to getRcaSettings
and getWebhookUrl for consistency (or alternatively document-and-enforce
null-on-error everywhere), and ensure the apiRequest usage and return types
(IncidentIoRcaSettings | null) are updated if you keep throwing so callers no
longer rely on null checks.
In `@server/chat/backend/agent/skills/integrations/incidentio/SKILL.md`:
- Around line 10-16: The frontmatter contains a redundant allowed-tools key that
duplicates the tools list and isn't consumed by SkillMetadata; remove the
allowed-tools line from SKILL.md (or if you intend it to serve a different
purpose, document that purpose and keep it in sync) and ensure the canonical
tools are declared only under the tools key so SkillMetadata (and any loaders)
use a single source of truth; check references to allowed-tools and update any
docs or consumers to rely on tools instead.
In `@server/chat/backend/agent/tools/incidentio_tool.py`:
- Around line 94-132: The code in _format_incident_summary sets severity =
incident.get("severity") or {} and inc_type = incident.get("incident_type") or
{}, so the subsequent isinstance(..., dict) checks are unnecessary; simplify by
removing the isinstance guards and directly use severity.get("name") and
inc_type.get("name") (or fallback to str(...) only if you choose not to coerce
upstream), i.e., keep the current coercion to {} and change the "severity" and
"type" fields to use severity.get("name", str(severity)) and
inc_type.get("name", str(inc_type)) or simply severity.get("name") /
inc_type.get("name") with your preferred fallback, updating
_format_incident_summary accordingly.
In `@server/routes/incidentio/incidentio_routes.py`:
- Around line 81-83: In get_incident_updates, remove the unnecessary f-string
prefix from the literal f"/incident_updates" (it has no placeholders); change
the call in method get_incident_updates (which calls self._request("GET",
f"/incident_updates", params=...)) to use a plain string "/incident_updates" to
satisfy Ruff F541.
In `@server/routes/incidentio/tasks.py`:
- Around line 338-349: The DB update for incidents.rca_celery_task_id (using
db_pool.get_admin_connection() and cursor.execute(...) with task.id and
incident_id) is swallowing all exceptions; replace the bare except: pass with a
try/except that logs the error (e.g., logger.exception(...) or
current_app.logger.exception(...)) including the exception/traceback and context
(task.id and incident_id) so failures to set rca_celery_task_id are recorded for
debugging.
- Around line 361-410: The postback_rca_to_incidentio task uses a very small
retry window (max_retries=2, default_retry_delay=120) and a fragile string-based
detection of Celery retries; increase the retry budget (e.g., set max_retries to
6–10 and/or default_retry_delay to a larger value like 300, or better yet switch
to event/signal-based polling) and replace the string-check with an explicit
Retry exception handler: import Retry from celery.exceptions, add an except
Retry: raise block before the broad except Exception, then in the except
Exception block log the error and call self.retry(exc=exc, countdown=...) (use
raise ... from exc if re-raising) so retry semantics are explicit and robust;
locate these changes in postback_rca_to_incidentio, updating
max_retries/default_retry_delay, the self.retry calls, and the exception
handling.
In `@server/utils/db/db_utils.py`:
- Around line 859-866: The current indexes on incidentio_alerts
(idx_incidentio_alerts_user_id, idx_incidentio_alerts_severity) do not support
org-scoped list queries ordered by recency; add a composite index on (org_id,
received_at DESC) (e.g. create index IF NOT EXISTS
idx_incidentio_alerts_org_received_at ON incidentio_alerts(org_id, received_at
DESC)) to support ORDER BY received_at for org-scoped queries, and optionally
add a covering index including severity (e.g.
idx_incidentio_alerts_org_received_at_severity ON incidentio_alerts(org_id,
received_at DESC, severity)) to accelerate org+severity filtered lists; update
migration where other indexes (idx_incidentio_alerts_org_incident,
idx_incidentio_alerts_user_id, idx_incidentio_alerts_status,
idx_incidentio_alerts_severity) are defined.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4789a9f0-2587-4214-ba43-bb40984b9bea
⛔ Files ignored due to path filters (3)
client/public/incidentio-icon.pngis excluded by!**/*.pngclient/public/incidentio-round.svgis excluded by!**/*.svgclient/public/incidentio.svgis excluded by!**/*.svg
📒 Files selected for processing (31)
client/src/app/api/connected-accounts/[provider]/route.tsclient/src/app/api/incident-io/alerts/route.tsclient/src/app/api/incident-io/alerts/webhook-url/route.tsclient/src/app/api/incident-io/alerts/webhook/[userId]/route.tsclient/src/app/api/incident-io/connect/route.tsclient/src/app/api/incident-io/rca-settings/route.tsclient/src/app/api/incident-io/status/route.tsclient/src/app/incident-io/auth/page.tsxclient/src/app/incident-io/incidents/page.tsxclient/src/app/incidents/components/IncidentCard.tsxclient/src/components/connectors/ConnectorRegistry.tsclient/src/components/incident-io/IncidentIoWebhookStep.tsxclient/src/components/tool-calls/CommandLogo.tsxclient/src/lib/services/incident-io.tsclient/src/middleware.tsserver/celery_config.pyserver/chat/backend/agent/skills/integrations/incidentio/SKILL.mdserver/chat/backend/agent/skills/load_skill_tool.pyserver/chat/backend/agent/skills/registry.pyserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/backend/agent/tools/incidentio_tool.pyserver/chat/background/rca_prompt_builder.pyserver/chat/background/task.pyserver/main_compute.pyserver/routes/incidentio/__init__.pyserver/routes/incidentio/incidentio_routes.pyserver/routes/incidentio/tasks.pyserver/routes/incidents_routes.pyserver/tests/test_connector_rbac.pyserver/utils/db/db_utils.pyserver/utils/secrets/secret_ref_utils.py
CodeQL fixes: - Don't expose exception details to users (info exposure via str(exc)) — sanitize error messages in connect/status routes - Remove logging of secret deletion count (clear-text sensitive data) - Replace empty except with logged warning in tasks.py Code quality fixes: - Remove unused `json` import from incidentio_routes.py - Remove unused `result` variable (list_incidents validation call) - Remove unused `List` import from incidentio_tool.py - Remove unused `IncidentIoRcaSettings` import from WebhookStep - Replace empty except with explicit body extraction fallback Bug fixes: - Toggle handlers now show error toast when updateRcaSettings returns null (service catches errors internally and returns null, not an exception) - _truncate_output now produces valid JSON instead of broken suffix - Bound limit/offset query params to prevent unbounded DB queries - Severity defaults to empty string instead of "unknown" to avoid unconditionally appending "(severity: unknown)" to status Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Tighten fuzzy match in load_skill: require min 4 chars for prefix matching, prefer exact normalized match, deterministic ordering - Fix incident.io v2 API filter params: use status_category[one_of] and severity[one_of] instead of bare names (bare names are silently ignored by the v2 API, returning unfiltered results) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/chat/backend/agent/skills/load_skill_tool.py (2)
112-114:⚠️ Potential issue | 🟠 MajorAvoid returning raw exception messages from the tool.
{e}can expose internal connector/API details through the agent response. Keep details in logs and return a generic error to the caller.Proposed fix
- except Exception as e: - logger.error(f"Error loading skill '{skill_id}': {e}", exc_info=True) - return json.dumps({"error": f"Failed to load skill: {e}"}) + except Exception: + logger.exception("Error loading skill '%s'", skill_id) + return json.dumps({ + "error": "Failed to load skill. Please retry or contact support if the issue persists." + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/chat/backend/agent/skills/load_skill_tool.py` around lines 112 - 114, The except block in load_skill_tool currently returns the raw exception message to the caller; instead keep the detailed exception in the logger (logger.error(..., exc_info=True)) and return a generic JSON error string (e.g., {"error":"Failed to load skill"}) using the same return path, ensuring you do not interpolate {e} into the returned payload; reference the except block handling for skill_id and the function load_skill_tool to locate and update the return value only.
55-85:⚠️ Potential issue | 🟡 MinorCanonicalize before checking the per-session dedupe.
Line 59 checks the raw input before the fuzzy match rewrites aliases to the canonical skill ID. So after loading
incident.io, a laterincidentdotio/incident_iocall can bypass the cache and return duplicate skill context.Proposed fix
- # Dedup: if this skill was already loaded in this session, return short ack key = _session_key(user_id, session_id) - with _loaded_skills_lock: - already_loaded = _loaded_skills.get(key, set()) - if skill_id in already_loaded: - return f"Skill '{skill_id}' is already loaded in this conversation — no need to reload." try: from .registry import SkillRegistry registry = SkillRegistry.get_instance() @@ if best_match: logger.info("load_skill fuzzy matched '%s' -> '%s'", skill_id, best_match) skill_id = best_match + + # Dedup after canonicalizing fuzzy aliases. + with _loaded_skills_lock: + already_loaded = _loaded_skills.get(key, set()) + if skill_id in already_loaded: + return f"Skill '{skill_id}' is already loaded in this conversation — no need to reload." result = registry.load_skill(skill_id, user_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/chat/backend/agent/skills/load_skill_tool.py` around lines 55 - 85, The dedupe check uses the raw skill_id before the fuzzy/canonical rewrite, allowing aliases like "incidentdotio" to bypass _loaded_skills; move or duplicate the canonicalization/fuzzy-match logic so that you compute the canonical_skill_id (using the same normalization and registry matching used later) before calling _session_key and checking _loaded_skills under _loaded_skills_lock; then use that canonical_skill_id (not the original input) for the dedupe lookup and when inserting into _loaded_skills so all alias forms map to the same canonical skill identifier.
🧹 Nitpick comments (3)
server/routes/incidentio/incidentio_routes.py (2)
80-81: Remove the no-op f-string flagged by Ruff.Line 81 has no interpolation and is reported as F541.
Proposed cleanup
def get_incident_updates(self, incident_id: str) -> Dict[str, Any]: - return self._request("GET", f"/incident_updates", params={"incident_id": incident_id}).json() + return self._request("GET", "/incident_updates", params={"incident_id": incident_id}).json()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/incidentio/incidentio_routes.py` around lines 80 - 81, The function get_incident_updates currently uses a no-op f-string in f"/incident_updates" which triggers Ruff F541; change it to a normal string literal "/incident_updates" in the call to self._request within get_incident_updates and keep the params argument intact (i.e., update the call in get_incident_updates to use "/incident_updates" while leaving the surrounding logic and the _request invocation unchanged).
167-169: Prefix the unused unpacked value.
deleted_countis not used and Ruff reports it; use a dummy name to keep the side-effect call unchanged.Proposed cleanup
- success, deleted_count = delete_user_secret(user_id, "incidentio") + success, _deleted_count = delete_user_secret(user_id, "incidentio")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/incidentio/incidentio_routes.py` around lines 167 - 169, The tuple unpack from delete_user_secret(user_id, "incidentio") binds an unused variable deleted_count which Ruff flags; change the unpack to use a dummy/wildcard name (e.g., _ or _deleted_count) so the side-effect call to delete_user_secret(user_id, "incidentio") remains unchanged but the unused value is ignored, updating the assignment in the same try block where success and deleted_count are currently set.server/utils/db/db_utils.py (1)
885-892: Add an org-scoped recency index for alert listing.
GET /incidentio/alertsfilters byorg_idand orders byreceived_at DESC, but the new table only has auser_idrecency index plus standalone status/severity indexes. Asincidentio_alertsgrows, org-scoped reads will do extra filtering/sorting.Suggested index additions
CREATE INDEX IF NOT EXISTS idx_incidentio_alerts_user_id ON incidentio_alerts(user_id, received_at DESC); + CREATE INDEX IF NOT EXISTS idx_incidentio_alerts_org_received + ON incidentio_alerts(org_id, received_at DESC); + CREATE INDEX IF NOT EXISTS idx_incidentio_alerts_org_severity_received + ON incidentio_alerts(org_id, severity, received_at DESC); CREATE INDEX IF NOT EXISTS idx_incidentio_alerts_status ON incidentio_alerts(incident_status);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/db/db_utils.py` around lines 885 - 892, The listing query for GET /incidentio/alerts filters by org_id and orders by received_at DESC but the schema only has a recency index on user_id and standalone indexes on incident_status/severity, so add an org-scoped recency index to avoid large-sort and filter costs: create an index on incidentio_alerts(org_id, received_at DESC) (e.g., name it idx_incidentio_alerts_org_received_at) alongside the existing idx_incidentio_alerts_org_incident and others so org-scoped, newest-first scans use the index directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes/incidentio/incidentio_routes.py`:
- Around line 179-199: The alert_webhook route currently enqueues work without
verifying the requester; before calling process_incidentio_event.delay you must
validate the provider signature/HMAC using a per-user or per-connection secret
obtained from get_token_data(user_id, "incidentio"); extract the signature
header (e.g., X-Signature or similar) from request.headers, compute the HMAC of
the raw request body with the secret, compare using a timing-safe comparison,
and return a 401/403 when missing or invalid; ensure OPTIONS handling still
returns CORS and that missing webhook secret in creds also blocks processing.
In `@server/routes/incidentio/tasks.py`:
- Around line 220-240: The transaction can remain aborted on failure and must be
rolled back; update the except block that catches Exception around the two
cursor.execute calls (the INSERT into incident_alerts and the UPDATE incidents)
to call conn.rollback() before logging so the pooled connection isn't returned
in a failed transaction state — keep the conn.commit() after the successful
executes and ensure the except references conn.rollback() prior to
logger.warning.
- Around line 379-392: The query is selecting non-existent columns "summary" and
"status" from incidents so the SELECT should read the actual columns
aurora_summary and aurora_status; update the cursor.execute call (and subsequent
unpacking) to SELECT aurora_summary, aurora_status FROM incidents WHERE id = %s
(using aurora_incident_id), then set summary, status = row (and adjust the
empty-check to use aurora_summary existence), and keep the existing status check
(status not in ("analyzed", "completed")) and retry logic intact.
- Around line 253-254: The current except block in process_incidentio_event
catches database exceptions with "except Exception as db_exc:" and only logs
them, which suppresses failures and prevents Celery retries; change this to
re-raise the error after logging (e.g., log via logger.exception and then
"raise") or call the Celery retry mechanism from the task (e.g., self.retry or
raise Retry) so that database failures propagate to Celery's retry path instead
of being swallowed.
---
Outside diff comments:
In `@server/chat/backend/agent/skills/load_skill_tool.py`:
- Around line 112-114: The except block in load_skill_tool currently returns the
raw exception message to the caller; instead keep the detailed exception in the
logger (logger.error(..., exc_info=True)) and return a generic JSON error string
(e.g., {"error":"Failed to load skill"}) using the same return path, ensuring
you do not interpolate {e} into the returned payload; reference the except block
handling for skill_id and the function load_skill_tool to locate and update the
return value only.
- Around line 55-85: The dedupe check uses the raw skill_id before the
fuzzy/canonical rewrite, allowing aliases like "incidentdotio" to bypass
_loaded_skills; move or duplicate the canonicalization/fuzzy-match logic so that
you compute the canonical_skill_id (using the same normalization and registry
matching used later) before calling _session_key and checking _loaded_skills
under _loaded_skills_lock; then use that canonical_skill_id (not the original
input) for the dedupe lookup and when inserting into _loaded_skills so all alias
forms map to the same canonical skill identifier.
---
Nitpick comments:
In `@server/routes/incidentio/incidentio_routes.py`:
- Around line 80-81: The function get_incident_updates currently uses a no-op
f-string in f"/incident_updates" which triggers Ruff F541; change it to a normal
string literal "/incident_updates" in the call to self._request within
get_incident_updates and keep the params argument intact (i.e., update the call
in get_incident_updates to use "/incident_updates" while leaving the surrounding
logic and the _request invocation unchanged).
- Around line 167-169: The tuple unpack from delete_user_secret(user_id,
"incidentio") binds an unused variable deleted_count which Ruff flags; change
the unpack to use a dummy/wildcard name (e.g., _ or _deleted_count) so the
side-effect call to delete_user_secret(user_id, "incidentio") remains unchanged
but the unused value is ignored, updating the assignment in the same try block
where success and deleted_count are currently set.
In `@server/utils/db/db_utils.py`:
- Around line 885-892: The listing query for GET /incidentio/alerts filters by
org_id and orders by received_at DESC but the schema only has a recency index on
user_id and standalone indexes on incident_status/severity, so add an org-scoped
recency index to avoid large-sort and filter costs: create an index on
incidentio_alerts(org_id, received_at DESC) (e.g., name it
idx_incidentio_alerts_org_received_at) alongside the existing
idx_incidentio_alerts_org_incident and others so org-scoped, newest-first scans
use the index directly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 74122053-5d98-44b6-bb2f-9966421f20b4
📒 Files selected for processing (7)
client/src/components/incident-io/IncidentIoWebhookStep.tsxserver/chat/backend/agent/skills/load_skill_tool.pyserver/chat/backend/agent/tools/incidentio_tool.pyserver/chat/background/rca_prompt_builder.pyserver/routes/incidentio/incidentio_routes.pyserver/routes/incidentio/tasks.pyserver/utils/db/db_utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/components/incident-io/IncidentIoWebhookStep.tsx
- server/chat/background/rca_prompt_builder.py
OlivierTrudeau
requested changes
Apr 21, 2026
…ector - tasks.py: fix critical bug — SELECT aurora_summary/aurora_status instead of nonexistent summary/status columns, preventing RCA postback from working - tasks.py: add conn.rollback() on failed alert-link transaction - tasks.py: re-raise DB exceptions so Celery retry path triggers - incidentio_routes.py: add HMAC webhook authentication (generates webhook_secret at connect, verifies X-Aurora-Signature on inbound webhooks) - incidentio_routes.py: fix no-op f-string (Ruff F541) - middleware.ts: make public route matching segment-aware so /webhook doesn't accidentally expose /webhook-url as public - load_skill_tool.py: move dedup check after fuzzy canonicalization to prevent alias bypass; sanitize error message to not expose internals Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sing source names - Remove Next.js webhook proxy route (webhook/[userId]/route.ts) — webhooks now go directly to backend like all other connectors - Update webhook URL generation to match BigPanda pattern (backend-direct) - Remove incident.io webhook from middleware public routes (no longer needed) - Add missing SOURCE_DISPLAY_NAMES: Datadog, Dynatrace, CloudBees Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ardcoded map Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
server/utils/db/db_utils.py (1)
870-893: Unique index on(org_id, incident_id)may not prevent duplicates whenincident_idis NULL.
incident_idis nullableVARCHAR(255), and PostgreSQL treats NULLs as distinct in unique indexes. If a webhook ever lacksincident.id(and_extract_incident_fieldsfalls back toNone), theON CONFLICT (org_id, incident_id)upsert intasks.pywill not match any prior NULL row, producing duplicates. Comparenewrelic_eventsabove (lines 619–620) which guards this withWHERE issue_id IS NOT NULL.Proposed fix
CREATE UNIQUE INDEX IF NOT EXISTS idx_incidentio_alerts_org_incident - ON incidentio_alerts(org_id, incident_id); + ON incidentio_alerts(org_id, incident_id) WHERE incident_id IS NOT NULL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/db/db_utils.py` around lines 870 - 893, The unique index idx_incidentio_alerts_org_incident on incidentio_alerts can still allow duplicate rows when incident_id IS NULL; change the index creation in the incidentio_alerts DDL to be conditional like the newrelic_events example: create the unique index ON incidentio_alerts(org_id, incident_id) WHERE incident_id IS NOT NULL so only non-NULL incident_id values are enforced as unique, and keep the upsert in tasks.py that uses ON CONFLICT (org_id, incident_id) unchanged (or ensure _extract_incident_fields never returns None if you prefer a different approach).server/routes/incidentio/tasks.py (1)
409-413: Replace substring matching with explicitisinstancecheck for CeleryRetryexceptions.The current code uses
"retry" not in str(type(exc).__name__).lower()to filterRetryexceptions, which is fragile and can match unrelated exceptions with "retry" in their name. Use explicit exception type checking instead:Proposed fix
+from celery.exceptions import Retry + + except Retry: + raise except Exception as exc: - if "retry" not in str(type(exc).__name__).lower(): - logger.exception("[INCIDENTIO] Postback failed: %s", exc) - raise self.retry(exc=exc) - raise + logger.exception("[INCIDENTIO] Postback failed: %s", exc) + raise self.retry(exc=exc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/incidentio/tasks.py` around lines 409 - 413, Replace the fragile substring check in the except block with an explicit isinstance check for Celery's Retry exception: import Retry from celery.exceptions and change the condition around logger.exception/self.retry to use "if not isinstance(exc, Retry):" so only non-Retry exceptions are logged and retried (and existing "raise" remains to re-raise Retry exceptions); update the except block that currently references self.retry and logger.exception accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/chat/backend/agent/tools/cloud_tools.py`:
- Around line 1513-1517: The ListIncidents tool description in cloud_tools.py is
out of sync with the schema: ListIncidentsArgs.status documents "declined" as a
valid status but the description only mentions "live/closed"; update the
description string (the description passed where the tool is defined) to list
all supported statuses — e.g., "live, closed, declined" — or use a neutral
phrasing like "Filter by status (e.g., live, closed, declined) or severity" so
it matches ListIncidentsArgs.status and avoids steering the LLM away from
supported values.
In `@server/routes/incidentio/incidentio_routes.py`:
- Around line 196-206: The current webhook handler (in incidentio_routes.py)
only verifies signatures when webhook_secret is present, allowing unsigned
webhooks to pass when webhook_secret is missing; change the logic to
fail-closed: if webhook_secret is falsy, log a warning and return a 401 error
(similar to the missing-signature path) before enqueuing
process_incidentio_event.delay(...), then continue to require signature
(request.headers.get("X-Aurora-Signature")), compute expected via
hmac.new(webhook_secret.encode(), request.get_data(),
hashlib.sha256).hexdigest(), and validate with hmac.compare_digest; ensure both
the missing-secret and missing/invalid-signature cases return 401 and are logged
with user_id for traceability.
In `@server/routes/incidentio/tasks.py`:
- Around line 279-288: The type hint for incident_id in _trigger_rca_pipeline is
incorrect (currently int) — change it to uuid.UUID or Union[uuid.UUID, str] to
match incidents.id and the actual value passed (incident_row[0]); update the
function signature in server/routes/incidentio/tasks.py and add the necessary
import (import uuid or from typing import Union) so downstream uses like
str(incident_id) remain valid and type-check cleanly.
- Around line 155-192: The upsert that writes to incidentio_alerts isn't
committed before calling AlertCorrelator.correlate/handle_correlated_alert, so
any exception leaves the transaction aborted and the alert lost; update the
control flow to either (a) commit the transaction immediately after the
incidentio_alerts upsert (before creating AlertCorrelator/calling
correlator.correlate) so the alert row is durable, or (b) in the except block
that catches correlation errors, call conn.rollback() and then re-open a fresh
transaction (or re-run the upsert) before proceeding to the _should_trigger_rca
check or the subsequent incidents INSERT; reference the AlertCorrelator class,
correlator.correlate, handle_correlated_alert, _should_trigger_rca, and use
conn.commit()/conn.rollback() accordingly.
---
Nitpick comments:
In `@server/routes/incidentio/tasks.py`:
- Around line 409-413: Replace the fragile substring check in the except block
with an explicit isinstance check for Celery's Retry exception: import Retry
from celery.exceptions and change the condition around
logger.exception/self.retry to use "if not isinstance(exc, Retry):" so only
non-Retry exceptions are logged and retried (and existing "raise" remains to
re-raise Retry exceptions); update the except block that currently references
self.retry and logger.exception accordingly.
In `@server/utils/db/db_utils.py`:
- Around line 870-893: The unique index idx_incidentio_alerts_org_incident on
incidentio_alerts can still allow duplicate rows when incident_id IS NULL;
change the index creation in the incidentio_alerts DDL to be conditional like
the newrelic_events example: create the unique index ON
incidentio_alerts(org_id, incident_id) WHERE incident_id IS NOT NULL so only
non-NULL incident_id values are enforced as unique, and keep the upsert in
tasks.py that uses ON CONFLICT (org_id, incident_id) unchanged (or ensure
_extract_incident_fields never returns None if you prefer a different approach).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 21a48e00-9552-49fa-a34c-2fb07758912c
📒 Files selected for processing (11)
client/src/app/incidents/components/IncidentCard.tsxclient/src/components/connectors/ConnectorRegistry.tsclient/src/components/tool-calls/CommandLogo.tsxclient/src/middleware.tsserver/chat/backend/agent/skills/load_skill_tool.pyserver/chat/backend/agent/tools/cloud_tools.pyserver/main_compute.pyserver/routes/incidentio/incidentio_routes.pyserver/routes/incidentio/tasks.pyserver/utils/db/db_utils.pyserver/utils/secrets/secret_ref_utils.py
✅ Files skipped from review due to trivial changes (1)
- client/src/middleware.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- server/main_compute.py
- client/src/components/connectors/ConnectorRegistry.ts
- server/utils/secrets/secret_ref_utils.py
- server/chat/backend/agent/skills/load_skill_tool.py
- client/src/app/incidents/components/IncidentCard.tsx
OlivierTrudeau
requested changes
Apr 21, 2026
Keep normalization (strip dots/dashes/underscores) for typo tolerance but drop the startsWith prefix fallback — it can silently match the wrong connector (e.g. "google" → "google_chat", "cloud" → "cloudbees"). If normalized name doesn't exact-match, the model gets an error with valid IDs and retries correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace broken X-Aurora-Signature with incident.io's actual Svix webhook signing (webhook-id, webhook-timestamp, webhook-signature) - Add webhook secret input field so users paste incident.io's signing secret from their endpoint settings - Add PUT /webhook-secret endpoint to store it - Default automatic RCA to enabled on new connections - Consolidate API key permission requirements into one place - Fix broken docs link (incident.io/docs/webhooks → docs.incident.io) - Add cursor-based pagination to list_incidentio_incidents (after param, has_more, total_count in response) — max page_size raised to 100 - Fix truncation to drop incidents from the array instead of chopping raw JSON mid-field - Use SAVEPOINT for correlation so failures don't discard the alert INSERT - Fix incident_id type hint (UUID, not int) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Sanitize user_id from URL path before logging (truncate to UUID length) - Stop logging exception details from IncidentioAPIError (may contain HTTP response body with internal info) - Use allowlist lookup for user-facing error messages instead of flowing exception string through str(exc) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/chat/backend/agent/tools/incidentio_tool.py (1)
156-168:⚠️ Potential issue | 🟠 MajorEnsure
_truncate_outputalways returns valid JSON (regression).Line 167 can return a raw string slice, which may be malformed JSON. That breaks the tool contract for
get_incidentio_incident/get_incidentio_timelinewhen payloads exceed the limit.🔧 Proposed fix
def _truncate_output(data: Any) -> str: output = json.dumps(data, default=str) if len(output) <= MAX_OUTPUT_SIZE: return output if isinstance(data, dict) and "incidents" in data and isinstance(data["incidents"], list): items = data["incidents"] while items and len(json.dumps(data, default=str)) > MAX_OUTPUT_SIZE: items.pop() data["truncated"] = True data["total_returned"] = len(items) return json.dumps(data, default=str) - return output[:MAX_OUTPUT_SIZE] + return json.dumps({ + "truncated": True, + "partial": output[:MAX_OUTPUT_SIZE], + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/chat/backend/agent/tools/incidentio_tool.py` around lines 156 - 168, _truncate_output currently may return a raw string slice (output[:MAX_OUTPUT_SIZE]) which can produce malformed JSON; update _truncate_output so every return is valid JSON: keep the existing incidents-list branch behavior but for the generic truncation case return json.dumps({ "truncated": True, "truncated_output": output[:MAX_OUTPUT_SIZE] }) (or similar wrapper) instead of returning the raw slice, ensuring functions like get_incidentio_incident / get_incidentio_timeline always receive valid JSON; make this change in the _truncate_output function and remove the raw-slice return path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes/incidentio/incidentio_routes.py`:
- Around line 382-386: In update_rca_settings, the get_user_preference call for
"incidentio_rca_enabled" uses default=False which conflicts with
get_rca_settings (default=True); change the default in update_rca_settings to
True so the response aligns with get_rca_settings — update the call to
get_user_preference(user_id, "incidentio_rca_enabled", default=True)
(referencing update_rca_settings, get_rca_settings, and the
"incidentio_rca_enabled" key).
---
Duplicate comments:
In `@server/chat/backend/agent/tools/incidentio_tool.py`:
- Around line 156-168: _truncate_output currently may return a raw string slice
(output[:MAX_OUTPUT_SIZE]) which can produce malformed JSON; update
_truncate_output so every return is valid JSON: keep the existing incidents-list
branch behavior but for the generic truncation case return json.dumps({
"truncated": True, "truncated_output": output[:MAX_OUTPUT_SIZE] }) (or similar
wrapper) instead of returning the raw slice, ensuring functions like
get_incidentio_incident / get_incidentio_timeline always receive valid JSON;
make this change in the _truncate_output function and remove the raw-slice
return path.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c6c48d6e-fb3b-4ba6-a94f-75ff6f95b34f
📒 Files selected for processing (7)
client/src/app/incident-io/auth/page.tsxclient/src/components/incident-io/IncidentIoWebhookStep.tsxclient/src/lib/services/incident-io.tsserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/backend/agent/tools/incidentio_tool.pyserver/routes/incidentio/incidentio_routes.pyserver/routes/incidentio/tasks.py
✅ Files skipped from review due to trivial changes (1)
- client/src/app/incident-io/auth/page.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/components/incident-io/IncidentIoWebhookStep.tsx
- server/chat/backend/agent/tools/cloud_tools.py
Remove explicit OPTIONS from route methods — Flask-CORS and the @require_permission decorator already handle CORS preflight, so mixing safe+unsafe methods in the same decorator was triggering S3752 security hotspots. Extract forwardAuthenticatedRequest() in backend-proxy.ts and collapse all 5 incident-io API routes from ~300 lines of duplicated auth/timeout/error boilerplate to ~30 lines total (9.9% → ~1%). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The status caching, localStorage management, and disconnect logic was duplicated across connector auth pages. This hook centralizes the pattern so incident-io (and future connectors) reuse it instead of duplicating ~40 lines of identical cache/localStorage/event code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Store fetchStatus in a ref so the refresh callback has a stable dependency chain. The previous version created a new fetchStatus reference each render, which cascaded through refresh → loadStatus → useEffect → re-render endlessly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Field extraction now handles the public_incident.* event envelope where incident.io nests incident data under the event-type key. Preferences are now org-scoped: all org members read/write the same row, and both store and get set myapp.current_org_id for RLS. Also fixes inconsistent default for rcaEnabled between GET and PUT endpoints. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SonarCloud S5145: key and user_id traced from request input were logged without sanitization. Since the callers always pass hardcoded keys, just remove the interpolated values from log messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ctor # Conflicts: # client/src/lib/backend-proxy.ts # server/utils/auth/stateless_auth.py # server/utils/db/db_utils.py
…add webhook-secret route - Add /incidentio/alerts/webhook/ to _OPEN_PREFIXES so external webhooks bypass the X-Internal-Secret check (same as all other webhook connectors) - Add public_incident.incident_created_v2 to is_new_incident check so v2 event types trigger RCA - Add missing Next.js API route for PUT /api/incident-io/webhook-secret Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Explain that the secret is recommended but optional, what it does (cryptographic verification), and what happens without it (any request accepted). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Backend now returns hasWebhookSecret from the webhook-url endpoint. Frontend shows a green checkmark and "configured" message when a secret is set, with an option to update it. When no secret is set, shows the full explanation of what it does and why it's recommended. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- S3776: Reduce cognitive complexity in tasks.py (extract _resolve_incident_object, _safe_name, _build_alert_metadata, _try_correlate, _create_and_link_incident, _store_and_process_event, _upsert_alert), stateless_auth.py (extract _parse_preference_value, simplify get_user_preference), rca_prompt_builder.py (extract _incidentio_dict_name, _incidentio_format_roles, _incidentio_format_custom_fields) - S3358: Replace nested ternary with independent conditional blocks in incidents page - S1192: Extract duplicated string literals to constants in incidentio_tool.py - S1481: Fix unused variable in incidentio_routes.py - S7764: Use globalThis instead of window in use-connector-auth.ts - S7735: Flip negated ternary conditions in auth page - S6759: Make all component props readonly in IncidentIoWebhookStep - S2486: Add error variable to empty catch blocks - S6853: Use Label with htmlFor for proper label association - S6479: Use stable keys instead of array indices for list rendering Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- S7727: Wrap extractTextFromNode in arrow function when passed to .map() to prevent extra arguments (index, array) being forwarded - S7502: Save asyncio.create_task() result in module-level set to prevent premature garbage collection of the websocket send task Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ctor # Conflicts: # client/src/app/incidents/components/IncidentCard.tsx # server/chat/backend/agent/tools/cloud_tools.py
…identio-connector
…xt in get_alerts - use-connector-auth: always revalidate connection status with server even when cache says connected, preventing stale "connected" UI - incidentio_routes: replace raw SET myapp.current_org_id with set_rls_context for consistency with all other routes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
OlivierTrudeau
approved these changes
Apr 23, 2026
…ctor # Conflicts: # server/tests/test_connector_rbac.py
|
OlivierTrudeau
approved these changes
Apr 24, 2026
This was referenced May 14, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
load_skill('incidentio')with fuzzy ID matchinglist_incidentio_incidents,get_incidentio_incident,get_incidentio_timelineWhat's included
Backend
celery_config.pyFrontend
SOURCE_DISPLAY_NAMESmap for proper casing (incident.io, PagerDuty, OpsGenie, etc.)Agent / Skills
SKILL.mdfor RCA and interactive chat guidanceload_skillto handle LLM ID variationsload_skill()callsload_skillon cloud providersTest plan
load_skill('incidentio')loads successfully in interactive chatincident_io,incidentdotiovariations🤖 Generated with Claude Code
Summary by CodeRabbit