feat: implement automated incident post-mortem generator#103
Open
Omkarop0808 wants to merge 5 commits into
Open
feat: implement automated incident post-mortem generator#103Omkarop0808 wants to merge 5 commits into
Omkarop0808 wants to merge 5 commits into
Conversation
|
@Omkarop0808 is attempting to deploy a commit to the SK Faizanuddin's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Env & Dependencies backend/.env.example, backend/package.json |
Added GROQ_API_KEY and optional integration env vars (Slack, SMTP, Confluence, GitHub) to env example; added nodemailer dependency. |
Generation Core backend/lib/postmortem-generator.js |
New module: extracts/enriches incident context, builds structured LLaMA prompt, calls GROQ LLaMA API with auth/timeouts/error handling, validates/formats resulting markdown, and exports generation utilities. |
Publishing & Persistence backend/lib/postmortem-publisher.js |
New module: generates filenames, safely saves markdown to sentinel-postmortems/ (collision handling), and publishes concurrently to Slack, Email (SMTP), Confluence, and GitHub with per-target status and error handling. |
Backend Routes & Integration backend/index.js, backend/routes/postmortem.routes.js |
Mounted /api/postmortem router; exposed aiLogs via app.locals; added endpoints (POST /generate, GET /list, GET /:filename) with auth, validation, and mapped error responses. |
Frontend Hook & Page sentinel-frontend/hooks/usePostMortemGeneration.ts, sentinel-frontend/app/dashboard/incidents/page.tsx |
New hook to POST generate and GET/download markdown; tracks per‑incident generating state, errors and last result; integrated into incidents page with success/error banners and download action. |
Frontend Components Prop Changes sentinel-frontend/components/incidents/IncidentTable.tsx, sentinel-frontend/components/incidents/IncidentDetail.tsx |
Added onGeneratePostMortem and isGeneratingPostMortem props and propagated them through rows/detail; added Generate Post‑Mortem button and UI guards. |
Minor / Infra sentinel-frontend/hooks/useTheme.ts, sentinel-postmortems/.gitkeep |
Added "use client" directive to useTheme and .gitkeep placeholder to track sentinel-postmortems/ directory. |
Sequence Diagram
sequenceDiagram
participant User as Frontend User
participant Client as React Client
participant API as Backend API
participant AiLogs as aiLogs Store
participant GROQ as GROQ LLaMA API
participant FileSystem as File System
participant Services as External Services (Slack/Email/Confluence/GitHub)
User->>Client: Click "Generate Post‑Mortem"
Client->>API: POST /api/postmortem/generate { incidentId }
API->>AiLogs: Resolve incident by ID
AiLogs-->>API: Incident context (timeline, metadata)
API->>API: Build prompt & enrich context
API->>GROQ: POST chat/completions (system + prompt)
GROQ-->>API: Generated markdown
API->>API: Validate/format markdown, compute metadata
API->>FileSystem: Save markdown to sentinel-postmortems/
FileSystem-->>API: File path
API->>Services: Publish concurrently (Slack, Email, Confluence, GitHub)
Services-->>API: Per-target status
API-->>Client: PostmortemResponse (metadata + publish status)
Client-->>User: Show success + download option
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
- feat: implement autonomous service healing with AI-driven monitoring #10 — modifies
backend/index.jsand exposesaiLogs; directly related to the new postmortem routes usingapp.locals.aiLogs. - Feature/incident history page #56 — also touches server wiring (WebSocket/broadcast) in
backend/index.js; overlaps with same file integration points. - Feature/sentinel dashboard and hero overhaul #6 — overlaps frontend incident UI changes (incidents page / components) that the postmortem UI integrates with.
Suggested labels
hard
Suggested reviewers
- mdhaarishussain
Poem
🐰 I hopped through logs and timestamps bright,
Collected clues by moonlit byte,
LLaMA stitched the tale just right,
Saved our lessons in markdown light,
Now incidents sleep calm at night.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 68.42% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The pull request title 'feat: implement automated incident post-mortem generator' is concise, specific, and clearly summarizes the main change—adding LLaMA-powered post-mortem generation to the incident management system. |
| Description check | ✅ Passed | The pull request description is comprehensive and well-structured, following the template with detailed sections including description, related issues, type of change, testing approach, checklist completion, screenshots, and implementation notes. |
| Linked Issues check | ✅ Passed | The PR fully implements the requirements from issue #75: captures incident context, generates structured markdown with all 8 required sections, includes multi-target publishing support (Slack, Email, Confluence, GitHub), provides frontend integration with per-incident loading states, and meets the 15-second generation latency target. |
| Out of Scope Changes check | ✅ Passed | All changes are directly related to implementing the post-mortem generator feature. The addition of 'use client' directive to useTheme.ts and ID type conversion fixes are necessary bug fixes for proper functionality and Next.js compatibility. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
- 📝 Generate docstrings (stacked PR)
- 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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 @coderabbitai help to get the list of available commands and usage tips.
![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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sentinel-frontend/app/dashboard/incidents/page.tsx (1)
3-4:⚠️ Potential issue | 🟡 MinorRemove unused imports flagged by the linter.
DashboardHeader(Line 3) andIncidentCard(Line 4) are imported but never used in this file.🧹 Proposed fix
-import { DashboardHeader } from "@/components/layout/DashboardHeader"; -import { IncidentCard } from "@/components/dashboard/IncidentCard";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sentinel-frontend/app/dashboard/incidents/page.tsx` around lines 3 - 4, The file imports unused symbols DashboardHeader and IncidentCard; remove those import entries to satisfy the linter: delete DashboardHeader and IncidentCard from the import statements (or remove the entire import lines if they become empty) so only actually used exports remain imported in page.tsx.
🧹 Nitpick comments (7)
backend/.env.example (1)
29-29:GROQ_API_KEYis misplaced under the JWT configuration section.This key is unrelated to JWT and belongs in the "POST-MORTEM GENERATOR CONFIGURATION" block (lines 47–70) alongside the other post-mortem integration variables. Placing it here is confusing.
♻️ Suggested move
JWT_ACCESS_EXPIRY=15m JWT_REFRESH_EXPIRY=7d -GROQ_API_KEY=your_groq_api_key_hereThen add it at the top of the post-mortem configuration block:
# ============================================ # POST-MORTEM GENERATOR CONFIGURATION # ============================================ +# Required: GROQ API key for LLaMA-based post-mortem generation +GROQ_API_KEY=your_groq_api_key_here + # Optional: Slack integration for post-mortem notifications🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/.env.example` at line 29, Move the GROQ_API_KEY entry out of the JWT configuration block into the POST-MORTEM GENERATOR CONFIGURATION block: locate the GROQ_API_KEY variable in the current JWT section and cut it, then paste it at the top of the post-mortem configuration block (alongside other post-mortem integration variables) so that GROQ_API_KEY lives with related keys; ensure spacing/comments remain consistent and update any surrounding comments if necessary to reflect the new grouping.backend/index.js (1)
344-345:app.locals.aiLogsis set inside thelistencallback — move it beforeapp.listen()to avoid a race window.Between
app.listen()binding the port and the callback firing, incoming requests could hit/api/postmortem/generateand findapp.locals.aiLogsundefined. The route middleware gracefully falls back to an empty array, so this isn't a crash, but the fix is trivial:♻️ Suggested fix
+// Make aiLogs available to routes +app.locals.aiLogs = aiLogs; + const server = app.listen(PORT, '0.0.0.0', () => { console.log(`🚀 Sentinel Backend running on http://0.0.0.0:${PORT}`); }); - -// Make aiLogs available to routes -app.locals.aiLogs = aiLogs;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/index.js` around lines 344 - 345, Move the assignment that sets app.locals.aiLogs (currently done inside the app.listen() callback) to a point before calling app.listen() so that aiLogs is initialized on the Express app prior to the server binding; locate the aiLogs variable and the line "app.locals.aiLogs = aiLogs" and place that assignment immediately after aiLogs is created/initialized and before the call to app.listen(), ensuring routes like /api/postmortem/generate always see app.locals.aiLogs.backend/lib/postmortem-publisher.js (2)
93-154: SlackwebhookUrlis not validated — an attacker with access to setSLACK_WEBHOOK_URLto an arbitrary URL could exfiltrate post-mortem data (SSRF).This is somewhat mitigated since env vars are admin-controlled, but a URL allowlist or at minimum a
hooks.slack.comdomain check would add defense-in-depth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/postmortem-publisher.js` around lines 93 - 154, In publishToSlack, validate the SLACK_WEBHOOK_URL before using it: parse webhookUrl (in publishToSlack) using URL, ensure it uses https and its hostname matches an allowlist (e.g., exact or suffix match like "hooks.slack.com" or other approved hostnames), and if validation fails return { success: false, skipped: true, reason: 'invalid webhook url' } (and log) instead of calling axios.post; this defends against SSRF while leaving the existing payload/axios.post behavior unchanged.
228-234: Markdown-to-Confluence conversion is very rudimentary — lists, code blocks, links, and tables will render as raw text.The current regex chain only handles headings, bold, and italic. Post-mortems will contain bullet lists, code fences, and links, all of which will appear broken in Confluence. Consider using a library like
marked+ a Confluence storage format renderer, or document this as a known limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/postmortem-publisher.js` around lines 228 - 234, The current naive conversion in the try block of postmortem-publisher.js (building storageFormat from postmortem.markdown via simple regexes) doesn't handle lists, code fences, links, tables, etc.; replace this with a proper markdown-to-Confluence conversion by integrating a markdown parser and a Confluence storage formatter (for example use marked or markdown-it to parse postmortem.markdown and then render to Confluence storage format via an existing renderer or a small transformer), or explicitly document the limitation where storageFormat is produced so callers know these elements will not render correctly; update the code that constructs storageFormat and any surrounding error handling to use the chosen parser/renderer and ensure postmortem.markdown is fully converted before publishing.backend/lib/postmortem-generator.js (2)
160-183: Section validation is strict — consider a warning/fallback instead of throwing.If the LLM omits or renames even one section (e.g. "Remediation" instead of "Remediation Steps"),
formatMarkdownthrows and the entire generation fails. Combined with the tightmax_tokenslimit, this is likely to frustrate users. Consider logging a warning for missing sections while still returning the generated content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/postmortem-generator.js` around lines 160 - 183, The formatMarkdown function currently enforces all required section titles and throws an Error if any are missing; change this to be tolerant: in formatMarkdown, when foundSections.length < requiredSections.length, replace the thrown Error with a warning log (use the existing logger or console.warn) that lists missingSections, and then continue to return the original markdown (or a sanitized/fallback version) instead of throwing; keep the requiredSections array and foundSections detection as-is but ensure formatMarkdown returns the generated content when sections are missing so generation doesn't fail (refer to function name formatMarkdown and the requiredSections/foundSections variables).
219-227: Double invocation ofextractIncidentContext— context from line 221 is unused beyond being re-extracted insidebuildPrompt.
generatePostMortemcallsextractIncidentContext(incident)at line 221, then passes the result tobuildPrompt(context)at line 224. ButbuildPromptinternally callsextractIncidentContext(incident)again (line 68), processing the already-enriched object a second time. Either pass the rawincidentdirectly tobuildPrompt, or refactorbuildPromptto accept a pre-extracted context.♻️ Option A: skip the redundant extraction
try { - // Extract context - const context = extractIncidentContext(incident); - // Build prompt - const prompt = buildPrompt(context); + const prompt = buildPrompt(incident);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/postmortem-generator.js` around lines 219 - 227, The code double-extracts the incident context: generatePostMortem calls extractIncidentContext(incident) then passes that to buildPrompt(context), but buildPrompt re-invokes extractIncidentContext(incident). Refactor buildPrompt to accept a pre-extracted context (change its signature to buildPrompt(context)) and remove the internal call to extractIncidentContext inside buildPrompt so it uses the provided context; ensure generatePostMortem continues to call extractIncidentContext once and passes that context into buildPrompt, and update any other callers of buildPrompt to pass the context rather than the raw incident.sentinel-frontend/hooks/usePostMortemGeneration.ts (1)
60-85: Download errors are silently swallowed.
downloadPostMortemcatches all errors and only logs toconsole.error(Line 83). The caller inpage.tsx(handleDownloadPostMortem) has no way to know the download failed, so the user gets no feedback. Consider either re-throwing or exposing a download error state so the UI can display a notification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sentinel-frontend/hooks/usePostMortemGeneration.ts` around lines 60 - 85, downloadPostMortem currently swallows errors and only console.errors them, so the caller handleDownloadPostMortem cannot react; change downloadPostMortem in usePostMortemGeneration.ts to propagate failures by either (A) re-throwing the caught error after logging (so callers awaiting downloadPostMortem can catch it), or (B) add and update a downloadError state (e.g., const [downloadError, setDownloadError] = useState<Error|null>(null)) and expose that state from the hook so page.tsx can show UI feedback; ensure you also clear downloadError on successful download and update handleDownloadPostMortem to await the promise and handle the thrown error or read the exposed downloadError state to display a notification to the user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/lib/postmortem-generator.js`:
- Line 132: The current hard timeout value (timeout: 15000) in the GROQ request
config is too short given the noted 10–15s generation time; change the timeout
setting used in postmortem generation (the timeout property in the
request/config object in backend/lib/postmortem-generator.js, where the GROQ
call is made) to a safer value such as 30000 (30s) or higher to avoid
intermittent failures.
- Around line 124-126: The current max_tokens: 2048 in
backend/lib/postmortem-generator.js is too low and can truncate an 8-section
post-mortem; update the max_tokens setting to 4096 (or higher) in the request
payload where max_tokens is defined, and also make the output validation in
formatMarkdown (or the function that validates/throws on missing sections)
degrade gracefully by detecting missing sections and either re-prompting the
model or filling placeholders instead of immediately throwing; locate the
max_tokens constant/field and the formatMarkdown validation function to
implement these two changes.
- Around line 141-152: The catch block in backend/lib/postmortem-generator.js
currently handles timeouts, 429 and 5xx but not authentication failures; update
the catch (error) handler used in the LLaMA API request (the error handling
block that checks error.code and error.response?.status) to explicitly detect
error.response?.status === 401 || error.response?.status === 403 and throw a
clear Error (e.g., "LLaMA API authentication failed: invalid or expired API
key") so callers get a specific message for invalid/expired GROQ_API_KEY;
include the original status or error message in the thrown Error for
diagnostics.
In `@backend/lib/postmortem-publisher.js`:
- Around line 61-75: The collision loop is rebuilding baseName from the mutated
filename which causes suffixes to compound; capture the original filename before
the loop (e.g., const originalFilename = filename or compute originalBaseName =
filename.replace('.md','')) and inside the while loop derive baseName from that
original value instead of from the changing filename, then set filename =
`${baseName}-${counter}.md` and update filepath accordingly; keep counter and
fs.access logic the same so sequential suffixes (name-1.md, name-2.md, ...) are
produced.
- Line 181: The code is calling a non-existent method
nodemailer.createTransporter which causes a TypeError; update the transport
creation to call nodemailer.createTransport instead (replace any use of
createTransporter with createTransport when constructing the transporter
variable), ensuring the options passed remain unchanged and tests or runtime
that reference transporter continue to work.
In `@backend/routes/postmortem.routes.js`:
- Around line 127-141: The code is using stats.birthtime which is unreliable on
many Linux filesystems; update the postmortems mapping (the async map that
builds the postmortems array) to use stats.mtime.toISOString() for createdAt (or
better named modifiedAt) instead of stats.birthtime, and make the identical
change in the GET /:filename handler where the file stat is read (replace any
stats.birthtime usage with stats.mtime.toISOString()); ensure the returned
property name remains consistent with the API contract.
- Around line 97-104: The response currently returns error.stack to clients in
the res.status(...).json(...) block (using statusCode, errorCode, error.message,
error.stack), which exposes internal paths; change this to only include the
stack when running in development (e.g. check process.env.NODE_ENV ===
'development' or app.get('env')) and omit or replace details in production —
return details: undefined or remove the field unless the environment check
passes so production responses never leak error.stack.
- Around line 38-39: The comparison using aiLogs.find(log => log.id ===
incidentId) can fail when incidentId and log.id have different types (string vs
number); update the find predicate in the postmortem route to normalize types
before comparison (e.g., coerce both sides to the same type using
Number(incidentId) or String(log.id)) so incidentId, aiLogs, and the find
callback consistently match; reference the incidentId variable, aiLogs array,
and the find/log.id expression when making the change.
- Around line 61-75: The response currently exposes an absolute server file path
via publishingStatus.file?.path in the res.json call; instead derive and return
only a safe filename or relative path. Update the code in postmortem.routes.js
where res.json is built (the block that references publishingStatus.file?.path)
to compute a sanitized value (e.g., use
path.basename(publishingStatus.file?.path) or strip the server root to make it
relative), handle cases when publishingStatus.file is undefined/null, and return
that filename/relativePath in the postmortem.filePath field rather than the
absolute filesystem path.
- Around line 23-49: The post-mortem routes are missing authentication; wrap the
route handlers for router.post('/generate'), router.get('/list') and
router.get('/:filename') with the existing requireAuth middleware (imported from
backend/auth/middleware.js) so only authenticated users can call them; update
the router declarations to apply requireAuth (either as a per-route second
argument or apply it to a router-level use) and ensure the requireAuth import is
added at the top of postmortem.routes.js.
In `@sentinel-frontend/app/dashboard/incidents/page.tsx`:
- Around line 117-126: The success/error timeouts in handleGeneratePostMortem
(and the other handler around lines 137-157) race because each call creates an
independent setTimeout; fix this by adding useRef hooks (e.g., successTimeoutRef
and errorTimeoutRef) to store the current timeout IDs, call clearTimeout on the
appropriate ref before creating a new timeout, assign the new ID to the ref, and
ensure you clear any remaining timeouts in a cleanup effect (useEffect return)
so setShowSuccessMessage/setShowErrorMessage cannot be dismissed by a prior
timeout.
- Around line 117-126: In handleGeneratePostMortem in page.tsx, remove or rename
the unused catch parameter (e.g., catch { ... } or catch (_error) { ... }) to
fix the lint error and prevent the unused variable warning, and replace the raw
setTimeout calls with timeout IDs stored in a useRef (e.g., successTimeoutRef
and errorTimeoutRef) so you can clearTimeout before setting a new timeout and
clear them in a useEffect cleanup when the component unmounts; ensure you clear
the relevant ref timeout before calling
setShowSuccessMessage/setShowErrorMessage to avoid firing state updates on an
unmounted component.
In `@sentinel-frontend/components/incidents/IncidentDetail.tsx`:
- Around line 82-97: Extract the repeated conversion into a single local
constant (e.g., postMortemId) inside the IncidentDetail component: compute it
once from incident.id (convert string IDs using Number(...) or
parseInt(...,10)), then validate it with Number.isFinite or Number.isInteger (or
Number.isNaN check) to ensure it is a valid numeric id; use that single
validated postMortemId for the onClick handler, for the
isGeneratingPostMortem(postMortemId) call, and for the disabled/label logic, and
if the id is invalid disable the button and avoid calling onGeneratePostMortem.
- Around line 10-11: The incident ID is a string (Incident.id) but the props
onGeneratePostMortem and isGeneratingPostMortem expect numbers, and parseInt on
the mock IDs (e.g., "inc-1") yields 0; update the prop types to accept string
IDs instead of number (change onGeneratePostMortem?: (incidentId: string) =>
void and isGeneratingPostMortem?: (incidentId: string) => boolean) and remove
the parseInt usage where you call onGeneratePostMortem/isGeneratingPostMortem
(use incident.id directly), or alternatively ensure the data layer always
supplies numeric IDs—prefer changing the callback signatures to string and
passing incident.id to onGeneratePostMortem/isGeneratingPostMortem to fix the
mismatch.
In `@sentinel-frontend/hooks/usePostMortemGeneration.ts`:
- Line 37: The fetch calls in generatePostMortem and downloadPostMortem hardcode
"http://localhost:4000"; change them to use a configurable api base (e.g., const
apiBase = process.env.NEXT_PUBLIC_API_URL || '') defined at module scope or from
a shared config, and build URLs as `${apiBase}/api/postmortem/generate` and
`${apiBase}/api/postmortem/download` (or use relative paths like
`/api/postmortem/...` when relying on Next.js rewrites) so the code works in
deployed environments.
- Around line 51-54: In the catch block inside usePostMortemGeneration replace
the `err: any` with `err: unknown` and narrow it before reading message; e.g.
check `err instanceof Error` (or test for a `.message` property) to build
`errorMessage` as a string, call `setError(errorMessage)`, and then rethrow
`err`; update the catch in the function `usePostMortemGeneration` to avoid `any`
by converting unknown to a safe string for logging and state.
- Around line 43-46: The error path currently calls response.json() which can
throw if the body isn't JSON; update the error handling in
usePostMortemGeneration (the fetch response branch using response.ok) to try
parsing JSON inside a try/catch, and if JSON parse fails use response.text() (or
response.statusText) as a fallback, then throw a new Error that includes the
HTTP status and the parsed fallback message; ensure you reference the same
response variable and replace the direct await response.json() call with the
safe-parse flow so non-JSON error bodies (e.g., HTML 502) don't cause an
unexpected SyntaxError.
---
Outside diff comments:
In `@sentinel-frontend/app/dashboard/incidents/page.tsx`:
- Around line 3-4: The file imports unused symbols DashboardHeader and
IncidentCard; remove those import entries to satisfy the linter: delete
DashboardHeader and IncidentCard from the import statements (or remove the
entire import lines if they become empty) so only actually used exports remain
imported in page.tsx.
---
Nitpick comments:
In `@backend/.env.example`:
- Line 29: Move the GROQ_API_KEY entry out of the JWT configuration block into
the POST-MORTEM GENERATOR CONFIGURATION block: locate the GROQ_API_KEY variable
in the current JWT section and cut it, then paste it at the top of the
post-mortem configuration block (alongside other post-mortem integration
variables) so that GROQ_API_KEY lives with related keys; ensure spacing/comments
remain consistent and update any surrounding comments if necessary to reflect
the new grouping.
In `@backend/index.js`:
- Around line 344-345: Move the assignment that sets app.locals.aiLogs
(currently done inside the app.listen() callback) to a point before calling
app.listen() so that aiLogs is initialized on the Express app prior to the
server binding; locate the aiLogs variable and the line "app.locals.aiLogs =
aiLogs" and place that assignment immediately after aiLogs is
created/initialized and before the call to app.listen(), ensuring routes like
/api/postmortem/generate always see app.locals.aiLogs.
In `@backend/lib/postmortem-generator.js`:
- Around line 160-183: The formatMarkdown function currently enforces all
required section titles and throws an Error if any are missing; change this to
be tolerant: in formatMarkdown, when foundSections.length <
requiredSections.length, replace the thrown Error with a warning log (use the
existing logger or console.warn) that lists missingSections, and then continue
to return the original markdown (or a sanitized/fallback version) instead of
throwing; keep the requiredSections array and foundSections detection as-is but
ensure formatMarkdown returns the generated content when sections are missing so
generation doesn't fail (refer to function name formatMarkdown and the
requiredSections/foundSections variables).
- Around line 219-227: The code double-extracts the incident context:
generatePostMortem calls extractIncidentContext(incident) then passes that to
buildPrompt(context), but buildPrompt re-invokes
extractIncidentContext(incident). Refactor buildPrompt to accept a pre-extracted
context (change its signature to buildPrompt(context)) and remove the internal
call to extractIncidentContext inside buildPrompt so it uses the provided
context; ensure generatePostMortem continues to call extractIncidentContext once
and passes that context into buildPrompt, and update any other callers of
buildPrompt to pass the context rather than the raw incident.
In `@backend/lib/postmortem-publisher.js`:
- Around line 93-154: In publishToSlack, validate the SLACK_WEBHOOK_URL before
using it: parse webhookUrl (in publishToSlack) using URL, ensure it uses https
and its hostname matches an allowlist (e.g., exact or suffix match like
"hooks.slack.com" or other approved hostnames), and if validation fails return {
success: false, skipped: true, reason: 'invalid webhook url' } (and log) instead
of calling axios.post; this defends against SSRF while leaving the existing
payload/axios.post behavior unchanged.
- Around line 228-234: The current naive conversion in the try block of
postmortem-publisher.js (building storageFormat from postmortem.markdown via
simple regexes) doesn't handle lists, code fences, links, tables, etc.; replace
this with a proper markdown-to-Confluence conversion by integrating a markdown
parser and a Confluence storage formatter (for example use marked or markdown-it
to parse postmortem.markdown and then render to Confluence storage format via an
existing renderer or a small transformer), or explicitly document the limitation
where storageFormat is produced so callers know these elements will not render
correctly; update the code that constructs storageFormat and any surrounding
error handling to use the chosen parser/renderer and ensure postmortem.markdown
is fully converted before publishing.
In `@sentinel-frontend/hooks/usePostMortemGeneration.ts`:
- Around line 60-85: downloadPostMortem currently swallows errors and only
console.errors them, so the caller handleDownloadPostMortem cannot react; change
downloadPostMortem in usePostMortemGeneration.ts to propagate failures by either
(A) re-throwing the caught error after logging (so callers awaiting
downloadPostMortem can catch it), or (B) add and update a downloadError state
(e.g., const [downloadError, setDownloadError] = useState<Error|null>(null)) and
expose that state from the hook so page.tsx can show UI feedback; ensure you
also clear downloadError on successful download and update
handleDownloadPostMortem to await the promise and handle the thrown error or
read the exposed downloadError state to display a notification to the user.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
sentinel-frontend/hooks/usePostMortemGeneration.ts (1)
21-22:publishing.file.pathshould be optional.
path: stringis typed as always-present even whensuccessisfalse. If the backend fails to write the file,pathwill be absent on the response, causing runtime issues in code that accesseslastGenerated?.publishing.file.path.♻️ Proposed fix
- file: { success: boolean; path: string }; + file: { success: boolean; path?: string };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sentinel-frontend/hooks/usePostMortemGeneration.ts` around lines 21 - 22, Change the publishing.file typing so path is not assumed present when success is false: update the type used in usePostMortemGeneration.ts for publishing.file to either make path optional (path?: string) or, better, use a discriminated union ({ success: true; path: string } | { success: false }) for publishing.file; then adjust any consumers (e.g., accesses like lastGenerated?.publishing.file.path) to either rely on the discriminant or guard for path presence.backend/lib/postmortem-generator.js (1)
67-69:buildPromptredundantly callsextractIncidentContexton an already-extracted context.
generatePostMortem(line 238) extracts context and then passes the result tobuildPrompt(line 241). ButbuildPromptcallsextractIncidentContextagain on that already-enriched object. This is harmless (fields are idempotent) but wasteful and confusing. Consider accepting the pre-extracted context directly.♻️ Suggested fix
-function buildPrompt(incident) { - const context = extractIncidentContext(incident); +function buildPrompt(context) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/postmortem-generator.js` around lines 67 - 69, buildPrompt redundantly re-runs extractIncidentContext on an object that generatePostMortem already pre-extracted; update buildPrompt to accept the pre-extracted context instead of calling extractIncidentContext again. Change the buildPrompt signature to take the context produced by extractIncidentContext (adjust any callers like generatePostMortem to pass that context), remove the internal call to extractIncidentContext from buildPrompt, and ensure new parameter name (e.g., incidentContext) is used consistently inside buildPrompt to build the prompt.backend/routes/postmortem.routes.js (2)
90-102: Error-to-status-code mapping viamessage.includes()is fragile.If the upstream error messages in
postmortem-generator.jsare refactored (e.g., "timed out" instead of "timeout"), the classification silently falls back to 500. Consider using custom error classes or error codes instead of string matching for more reliable dispatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/postmortem.routes.js` around lines 90 - 102, The current error classification in postmortem.routes.js relies on fragile string matching of error.message; update the system to use explicit error types or codes by defining custom Error subclasses (e.g., LlamaTimeoutError, RateLimitError, ServiceUnavailableError, InvalidIncidentDataError) or by setting an error.code property inside postmortem-generator.js when throwing errors, then replace message.includes checks in the route handler with instanceof checks or direct equality checks against error.code (e.g., if (error instanceof LlamaTimeoutError) or if (error.code === 'LLAMA_TIMEOUT')) so classification is robust to text changes; ensure postmortem-generator.js throws the new error types or sets error.code accordingly and update the route handler in postmortem.routes.js to handle those types/codes and fall back to 500 otherwise.
9-18: Prefer readingreq.app.locals.aiLogsdirectly in handlers instead of caching in a module-level variable.The module-level
let aiLogsis updated by middleware on every request, but handlers could just readreq.app.locals.aiLogsdirectly. This avoids the mutable module-scope variable anti-pattern and is more idiomatic Express.♻️ Suggested change
Remove lines 9–18 and reference
req.app.locals.aiLogsin handlers:-// Get aiLogs from parent scope (will be injected) -let aiLogs = []; - -// Middleware to inject aiLogs -router.use((req, res, next) => { - if (req.app.locals.aiLogs) { - aiLogs = req.app.locals.aiLogs; - } - next(); -});Then in the handler (line 40):
- const incident = aiLogs.find(log => Number(log.id) === Number(incidentId)); + const aiLogs = req.app.locals.aiLogs || []; + const incident = aiLogs.find(log => Number(log.id) === Number(incidentId));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/postmortem.routes.js` around lines 9 - 18, Remove the module-level mutable variable aiLogs and the router.use middleware that assigns it; instead, update any route handlers that currently read the module variable (the handlers referencing aiLogs) to read req.app.locals.aiLogs directly (e.g., replace uses of aiLogs with req.app.locals.aiLogs in those handler functions). Ensure you delete the top-level "let aiLogs = []" and the middleware block "router.use((req, res, next) => { ... })" and verify all references in functions/controllers within this file now access req.app.locals.aiLogs.backend/lib/postmortem-publisher.js (1)
181-190:secure: falseis hardcoded — will break or downgrade security for port 465.Port 465 requires implicit TLS (
secure: true), while port 587 uses STARTTLS withsecure: false. The current hardcoding works for the default 587 but will fail if a user configuresSMTP_PORT=465.♻️ Suggested fix
+ const port = parseInt(smtpConfig.port) || 587; const transporter = nodemailer.createTransport({ host: smtpConfig.host, - port: parseInt(smtpConfig.port) || 587, - secure: false, + port, + secure: port === 465, auth: { user: smtpConfig.user, pass: smtpConfig.pass } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/postmortem-publisher.js` around lines 181 - 190, The transporter creation hardcodes secure: false which breaks implicit TLS on port 465; update the nodemailer.createTransport call to derive the secure flag from smtpConfig.port (parseInt(smtpConfig.port) === 465 ? true : false) so that when smtpConfig.port equals 465 the transport uses secure: true and otherwise uses secure: false (keep existing auth/host/port handling and reuse the same smtpConfig.port parsing logic used for the port field).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/lib/postmortem-generator.js`:
- Line 76: The template currently labels the start timestamp with "(MTTR total
time)" which is incorrect; locate the string containing "Start:
${context.timestamp} (MTTR total time)" in postmortem-generator.js and remove or
correct the parenthetical — either delete the "(MTTR total time)" after
context.timestamp or move/replace it with a proper MTTR reference next to
context.duration (e.g., "Resolution: ${context.duration} (MTTR)"); ensure you
update any related text so context.timestamp and context.duration are labeled
accurately.
- Around line 24-33: The current duration calculation can produce negative
values when incident.resolvedAt is earlier than incident.timestamp; update the
block that computes diffMs (using incident.timestamp and incident.resolvedAt) to
guard against negative durations by checking if diffMs <= 0 and in that case set
duration and mttr to a safe fallback like 'N/A' (or clamp to '0s' if preferred),
otherwise proceed with the existing minutes/seconds computation; ensure the
variables diffMs, duration and mttr are assigned in both branches so no
stale/negative strings are returned.
In `@backend/lib/postmortem-publisher.js`:
- Around line 286-301: The PUT to GitHub Contents API in postmortem-publisher.js
must include the existing file's sha when updating; change the logic around the
axios.put call in the publish routine to first attempt a GET of
`repos/${githubRepo}/contents/postmortems/${filename}?ref=${githubBranch}`
(using the same headers) to retrieve the file `sha` if it exists, and if found
add that `sha` to the PUT payload so the update succeeds; if the GET returns 404
treat it as a create (no sha) and proceed, and if the PUT returns 422 implement
a fallback to fetch the latest sha and retry the PUT once. Ensure the request
headers and timeout used for GET/PUT match the existing ones.
- Around line 229-235: The code is injecting raw LLM-generated markdown into
Confluence storageFormat via regex replacements on postmortem.markdown, which
allows unescaped HTML/macro/script content to be embedded; to fix, HTML-escape
the source markdown (create/Use an escapeHtml or encodeEntities helper) on
postmortem.markdown before performing the heading/bold/italic .replace() calls
so that any <script> tags or entities are neutralized, then run the existing
regex conversions to produce storageFormat.
In `@sentinel-frontend/hooks/usePostMortemGeneration.ts`:
- Around line 31-33: Replace the single scalar loading state with a collection:
change generatingIncidentId / setGeneratingIncidentId to track a Set<number>
(e.g., generatingIncidentIds) and update it with functional updaters to add the
incident id when starting and remove only that id in the request's finally block
so concurrent spinners are independent; likewise replace lastGenerated /
setLastGenerated with a Map<number, PostMortemResponse> (or attach the
incidentId to the response) so each request stores its own result instead of
clobbering the other; ensure all state updates use the setter's previous-state
callback to avoid races and update any helper like isGenerating to check
generatingIncidentIds.has(id).
---
Duplicate comments:
In `@backend/routes/postmortem.routes.js`:
- Line 220: Replace usage of fs.Stats.birthtime with fs.Stats.mtime for
createdAt to match the `/list` behavior; in the function constructing the
postmortem metadata (where createdAt is set from stats), change createdAt:
stats.birthtime.toISOString() to use stats.mtime (e.g., createdAt:
stats.mtime.toISOString()) and ensure the same conversion is applied
consistently wherever file timestamps are serialized.
- Around line 142-147: In the /list route in
backend/routes/postmortem.routes.js, don't return the server absolute path
(remove or replace the property that uses filepath) — only expose the filename
field — and replace stats.birthtime with stats.mtime when building the returned
object (use stats.mtime.toISOString() for createdAt) so you stop leaking
internal paths and avoid unreliable birthtime on Linux; update the returned
object that currently contains filename, path: filepath, createdAt:
stats.birthtime.toISOString(), size: stats.size accordingly.
In `@sentinel-frontend/hooks/usePostMortemGeneration.ts`:
- Around line 46-49: In usePostMortemGeneration where the fetch error path calls
response.json(), wrap the response.json() call in a try/catch and handle
non-JSON bodies: attempt JSON.parse via await response.json() inside try, and on
catch read fallbackText = await response.text() (or use
response.status/statusText) and include that fallback in the thrown Error;
update the throw to include response.status and either the parsed
errorData.error?.message or the fallbackText so a SyntaxError from
response.json() won't mask the real HTTP error.
- Line 54: In the usePostMortemGeneration hook update the catch binding from
err: any to err: unknown and then narrow the type before accessing .message; for
example change the catch clause in the generate/post-mortem flow inside
usePostMortemGeneration to catch (err: unknown) and then use a type guard like
if (err instanceof Error) { /* use err.message */ } else { /* handle or
stringify err */ } wherever .message is referenced.
- Around line 85-87: The downloadPostMortem function currently only logs errors
to console and creates a Blob even if data.content is missing; update
downloadPostMortem to call setError(...) in its catch block (same style as
generatePostMortem) so the UI shows failures, and add a guard before creating
the Blob to ensure data.content is defined/valid (e.g., return/setError if
missing) to avoid generating a Blob containing "undefined"; locate these changes
in the downloadPostMortem function and reference setError, generatePostMortem,
and data.content when applying the fix.
---
Nitpick comments:
In `@backend/lib/postmortem-generator.js`:
- Around line 67-69: buildPrompt redundantly re-runs extractIncidentContext on
an object that generatePostMortem already pre-extracted; update buildPrompt to
accept the pre-extracted context instead of calling extractIncidentContext
again. Change the buildPrompt signature to take the context produced by
extractIncidentContext (adjust any callers like generatePostMortem to pass that
context), remove the internal call to extractIncidentContext from buildPrompt,
and ensure new parameter name (e.g., incidentContext) is used consistently
inside buildPrompt to build the prompt.
In `@backend/lib/postmortem-publisher.js`:
- Around line 181-190: The transporter creation hardcodes secure: false which
breaks implicit TLS on port 465; update the nodemailer.createTransport call to
derive the secure flag from smtpConfig.port (parseInt(smtpConfig.port) === 465 ?
true : false) so that when smtpConfig.port equals 465 the transport uses secure:
true and otherwise uses secure: false (keep existing auth/host/port handling and
reuse the same smtpConfig.port parsing logic used for the port field).
In `@backend/routes/postmortem.routes.js`:
- Around line 90-102: The current error classification in postmortem.routes.js
relies on fragile string matching of error.message; update the system to use
explicit error types or codes by defining custom Error subclasses (e.g.,
LlamaTimeoutError, RateLimitError, ServiceUnavailableError,
InvalidIncidentDataError) or by setting an error.code property inside
postmortem-generator.js when throwing errors, then replace message.includes
checks in the route handler with instanceof checks or direct equality checks
against error.code (e.g., if (error instanceof LlamaTimeoutError) or if
(error.code === 'LLAMA_TIMEOUT')) so classification is robust to text changes;
ensure postmortem-generator.js throws the new error types or sets error.code
accordingly and update the route handler in postmortem.routes.js to handle those
types/codes and fall back to 500 otherwise.
- Around line 9-18: Remove the module-level mutable variable aiLogs and the
router.use middleware that assigns it; instead, update any route handlers that
currently read the module variable (the handlers referencing aiLogs) to read
req.app.locals.aiLogs directly (e.g., replace uses of aiLogs with
req.app.locals.aiLogs in those handler functions). Ensure you delete the
top-level "let aiLogs = []" and the middleware block "router.use((req, res,
next) => { ... })" and verify all references in functions/controllers within
this file now access req.app.locals.aiLogs.
In `@sentinel-frontend/hooks/usePostMortemGeneration.ts`:
- Around line 21-22: Change the publishing.file typing so path is not assumed
present when success is false: update the type used in
usePostMortemGeneration.ts for publishing.file to either make path optional
(path?: string) or, better, use a discriminated union ({ success: true; path:
string } | { success: false }) for publishing.file; then adjust any consumers
(e.g., accesses like lastGenerated?.publishing.file.path) to either rely on the
discriminant or guard for path presence.
| INCIDENT DATA: | ||
| - Container: ${context.container} | ||
| - Severity: ${context.severity} | ||
| - Start: ${context.timestamp} (MTTR total time) |
Contributor
There was a problem hiding this comment.
Misleading prompt label: (MTTR total time) next to the start timestamp.
The parenthetical (MTTR total time) is appended to the start timestamp, but MTTR represents the resolution duration (which is in context.duration). This may confuse the LLM and produce inaccurate timeline references.
- - Start: ${context.timestamp} (MTTR total time)
+ - Start: ${context.timestamp}📝 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
| - Start: ${context.timestamp} (MTTR total time) | |
| - Start: ${context.timestamp} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/lib/postmortem-generator.js` at line 76, The template currently
labels the start timestamp with "(MTTR total time)" which is incorrect; locate
the string containing "Start: ${context.timestamp} (MTTR total time)" in
postmortem-generator.js and remove or correct the parenthetical — either delete
the "(MTTR total time)" after context.timestamp or move/replace it with a proper
MTTR reference next to context.duration (e.g., "Resolution: ${context.duration}
(MTTR)"); ensure you update any related text so context.timestamp and
context.duration are labeled accurately.
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 (1)
sentinel-frontend/app/dashboard/incidents/page.tsx (1)
3-4:⚠️ Potential issue | 🟡 MinorUnused imports
DashboardHeaderandIncidentCard— lint warnings.The pipeline flags both as unused. Remove them to clean up.
-import { DashboardHeader } from "@/components/layout/DashboardHeader"; -import { IncidentCard } from "@/components/dashboard/IncidentCard";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sentinel-frontend/app/dashboard/incidents/page.tsx` around lines 3 - 4, Remove the unused imports DashboardHeader and IncidentCard from the top of page.tsx; locate the import statement that currently reads "import { DashboardHeader } from '@/components/layout/DashboardHeader'; import { IncidentCard } from '@/components/dashboard/IncidentCard';" and delete those import specifiers (or the entire import lines if they contain only those symbols) so the file no longer imports DashboardHeader or IncidentCard.
🧹 Nitpick comments (2)
backend/lib/postmortem-publisher.js (1)
63-76: Filename collision loop has no upper bound.The
while (true)loop incrementscounterindefinitely. Although practically it will find a free name quickly, a defensive upper limit would prevent a runaway loop in pathological scenarios (e.g., filesystem errors returning something other than ENOENT).♻️ Optional safeguard
let counter = 1; - while (true) { + const MAX_COLLISION_ATTEMPTS = 1000; + while (counter <= MAX_COLLISION_ATTEMPTS) { try { await fs.access(filepath); filename = `${originalBaseName}-${counter}.md`; filepath = path.join(dir, filename); counter++; } catch { break; } } + if (counter > MAX_COLLISION_ATTEMPTS) { + throw new Error(`Too many filename collisions for ${originalFilename}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/postmortem-publisher.js` around lines 63 - 76, The filename collision loop using while (true) with variables filename/originalBaseName/filepath/dir/counter has no upper bound; replace it with a bounded loop by introducing a maxAttempts constant (e.g., 1000) and iterate while counter <= maxAttempts, updating filename/filepath as now, and if the limit is reached throw or return a clear error; additionally, when awaiting fs.access, inspect the caught error (err.code) and only treat ENOENT as "file doesn't exist"—rethrow other errors so filesystem anomalies don't drive an endless loop.backend/lib/postmortem-generator.js (1)
186-196: Section detection viamarkdown.includes(section)is fragile.
includesperforms a case-sensitive substring match. If the LLM outputs"Root cause analysis"(lowercase 'c') or if the text"Executive Summary"appears in the body of another section rather than as a heading, the detection gives false positives/negatives. A heading-aware regex would be more robust.for (const section of requiredSections) { - if (markdown.includes(section)) { + const sectionRegex = new RegExp(`^#{1,3}\\s*${section}`, 'im'); + if (sectionRegex.test(markdown)) { foundSections.push(section);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/postmortem-generator.js` around lines 186 - 196, The current loop using markdown.includes(section) to populate foundSections and missingSections is fragile; replace the substring check with a heading-aware, case-insensitive regex test that matches section headings (e.g. anchors like ^#{1,6}\s*section\s*$ or ^\s*section\s*:\s*$) so you only detect top-level headings instead of incidental occurrences; update the loop that iterates requiredSections to build a RegExp from each required section name (with caseInsensitive and multiline flags) and use regex.test(markdown) to decide whether to push into foundSections or missingSections (references: requiredSections, foundSections, missingSections, markdown.includes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/lib/postmortem-generator.js`:
- Around line 243-248: generatePostMortem currently calls
extractIncidentContext(incident) and stores it in context but then passes that
to buildPrompt which itself calls extractIncidentContext again, so the first
call and the context variable are unused; fix by choosing one approach: either
1) remove the initial extractIncidentContext call in generatePostMortem and pass
the raw incident into buildPrompt(incident), or 2) refactor buildPrompt to
accept an already-extracted context (remove its internal call to
extractIncidentContext) and update generatePostMortem to pass the context
variable and delete the duplicate extraction; ensure you remove the unused
context variable or duplicate extraction accordingly and adjust any function
signatures or callers to match.
- Around line 88-89: The template mapping over context.reasoningChain should
guard against missing description fields to avoid rendering "- undefined";
update the mapping used in backend/lib/postmortem-generator.js (the
reasoningChain.map callback) to use a fallback like r.description || r.summary
|| r.note || 'No description provided' (or default to the overall fallback '- AI
analysis completed' when the array is empty) so each entry prints a sensible
string instead of "undefined".
In `@backend/lib/postmortem-publisher.js`:
- Around line 181-190: The transport is created with secure: false hardcoded
which breaks implicit TLS (SMTPS) on port 465; update the
nodemailer.createTransport call in postmortem-publisher.js to compute secure
based on smtpConfig.port (e.g., secure = parseInt(smtpConfig.port) === 465) and
use that value instead of the literal false, ensuring the auth/port parsing
around transporter creation (smtpConfig.port, parseInt(...)) uses the same
parsed port value.
In `@sentinel-frontend/hooks/usePostMortemGeneration.ts`:
- Around line 46-60: In usePostMortemGeneration, the code calls response.json()
then falls back to response.text(), but the body stream is consumed by the first
read; instead read the body once into a text variable (e.g., const bodyText =
await response.text()), then try to parse JSON from that text
(JSON.parse(bodyText)) to extract errorData.error?.message and if parsing fails
use bodyText || response.statusText; update the error handling in the non-ok
branch to use this single-read approach so you don't attempt to read the
response body twice.
---
Outside diff comments:
In `@sentinel-frontend/app/dashboard/incidents/page.tsx`:
- Around line 3-4: Remove the unused imports DashboardHeader and IncidentCard
from the top of page.tsx; locate the import statement that currently reads
"import { DashboardHeader } from '@/components/layout/DashboardHeader'; import {
IncidentCard } from '@/components/dashboard/IncidentCard';" and delete those
import specifiers (or the entire import lines if they contain only those
symbols) so the file no longer imports DashboardHeader or IncidentCard.
---
Duplicate comments:
In `@backend/lib/postmortem-generator.js`:
- Line 83: The "Start: ${context.timestamp} (MTTR total time)" prompt label is
misleading because MTTR is a duration available as context.duration, not the
start timestamp; update the template in postmortem-generator.js to remove the
"(MTTR total time)" parenthetical from the Start line and either (a) label the
start line simply as the start time using context.timestamp or (b) move/attach
the "(MTTR total time)" note to the line that prints context.duration (e.g., the
duration/MTTR line) so the label accurately references context.duration rather
than context.timestamp.
In `@sentinel-frontend/app/dashboard/incidents/page.tsx`:
- Around line 118-128: handleGeneratePostMortem schedules show/hide timers with
setTimeout but never clears previous timers, so rapid successive calls can hide
a newer banner early; modify the component to keep two refs (e.g.,
successTimeoutRef and errorTimeoutRef via useRef<number | null>) and before
creating a new setTimeout call clearTimeout on the existing id and replace it
with the new one when using setShowSuccessMessage and setShowErrorMessage in
handleGeneratePostMortem, and additionally clear both timeouts in a cleanup
useEffect on unmount to avoid leaks; update references where
setLastGeneratedIncidentId is set to ensure the success timeout corresponds to
the current action.
- Line 124: The catch block in
sentinel-frontend/app/dashboard/incidents/page.tsx currently declares an unused
binding via "catch (error) {" which triggers the lint "'error' is defined but
never used"; update the catch clause to remove the unused binding (use "catch {"
instead) within the same try/catch that contains the error handling logic so the
block still executes but no unused variable is declared.
In `@sentinel-frontend/components/incidents/IncidentDetail.tsx`:
- Around line 10-17: The Incident.id is declared as string while the callbacks
onGeneratePostMortem and isGeneratingPostMortem expect numbers, so update the
contract to be consistent: change the callback prop types to accept id: string |
number (or change Incident.id to number if you prefer numeric IDs), update the
component signature (IncidentDetailProps) to reflect the new type for
onGeneratePostMortem and isGeneratingPostMortem, and adjust the usage of
postMortemId/isValidId accordingly (either keep the parseInt conversion inside
IncidentDetail and pass a number to callbacks, or pass the original string
through and let downstream handlers convert/validate). Ensure callers of
onGeneratePostMortem and isGeneratingPostMortem are updated to the new type.
---
Nitpick comments:
In `@backend/lib/postmortem-generator.js`:
- Around line 186-196: The current loop using markdown.includes(section) to
populate foundSections and missingSections is fragile; replace the substring
check with a heading-aware, case-insensitive regex test that matches section
headings (e.g. anchors like ^#{1,6}\s*section\s*$ or ^\s*section\s*:\s*$) so you
only detect top-level headings instead of incidental occurrences; update the
loop that iterates requiredSections to build a RegExp from each required section
name (with caseInsensitive and multiline flags) and use regex.test(markdown) to
decide whether to push into foundSections or missingSections (references:
requiredSections, foundSections, missingSections, markdown.includes).
In `@backend/lib/postmortem-publisher.js`:
- Around line 63-76: The filename collision loop using while (true) with
variables filename/originalBaseName/filepath/dir/counter has no upper bound;
replace it with a bounded loop by introducing a maxAttempts constant (e.g.,
1000) and iterate while counter <= maxAttempts, updating filename/filepath as
now, and if the limit is reached throw or return a clear error; additionally,
when awaiting fs.access, inspect the caught error (err.code) and only treat
ENOENT as "file doesn't exist"—rethrow other errors so filesystem anomalies
don't drive an endless loop.
Comment on lines
+88
to
+89
| Investigation Timeline: | ||
| ${context.reasoningChain.map(r => `- ${r.description}`).join('\n') || '- AI analysis completed'} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what fields reasoningChain entries actually have
echo "=== Backend reasoningChain usage ==="
rg -n 'reasoningChain' --type=js -C5 -g '!node_modules'
echo ""
echo "=== Frontend reasoningChain/reasoning usage ==="
rg -n 'reasoningChain|reasoning' --type=ts --type=tsx -C3 -g '!node_modules' | head -100Repository: SKfaizan-786/sentinel-devops-agent
Length of output: 1798
🏁 Script executed:
#!/bin/bash
echo "=== Search for incident object creation and reasoningChain assignment ==="
rg -n 'incident\s*=' --type=js -C3 -g '!node_modules' | head -60
echo ""
echo "=== Search for reasoningChain being assigned/built ==="
rg -n 'reasoningChain\s*[:=]' --type=js -C3 -g '!node_modules'
echo ""
echo "=== Look for test files or mock data ==="
fd -t f -e 'test\.js' -e 'mock\.js' -e 'fixture.*\.js' | head -20
echo ""
echo "=== Check for API or data sources handling incident ==="
rg -n '\.reasoningChain' --type=js -C2 -g '!node_modules'Repository: SKfaizan-786/sentinel-devops-agent
Length of output: 2125
🏁 Script executed:
#!/bin/bash
echo "=== Search for aiLogs definition and initialization ==="
rg -n 'aiLogs\s*[:=]|const aiLogs|let aiLogs|var aiLogs' --type=js -C5 -g '!node_modules'
echo ""
echo "=== Search for where reasoningChain objects are created ==="
rg -n 'reasoningChain' --type=js -B5 -A5 -g '!node_modules'
echo ""
echo "=== Check for any TypeScript definitions or schema files ==="
fd -t f \( -name '*.ts' -o -name '*.d.ts' \) | head -20
echo ""
echo "=== Look for database or data structure files ==="
fd -t f \( -name '*schema*' -o -name '*model*' -o -name '*type*' \) | head -20Repository: SKfaizan-786/sentinel-devops-agent
Length of output: 4213
🏁 Script executed:
#!/bin/bash
echo "=== Search for where items are pushed to aiLogs ==="
rg -n 'aiLogs\.push\|aiLogs\[' --type=js -C5 -g '!node_modules'
echo ""
echo "=== Search for reasoningChain property being set ==="
rg -n 'reasoningChain\s*:' --type=js -C3 -g '!node_modules'
echo ""
echo "=== Search for incident object being created with properties ==="
rg -n 'const incident\s*=|new.*Incident' --type=js -C5 -g '!node_modules'Repository: SKfaizan-786/sentinel-devops-agent
Length of output: 3668
🏁 Script executed:
#!/bin/bash
echo "=== Search for API endpoints/handlers ==="
rg -n 'app\.post\|app\.put\|router\.post\|router\.put' --type=js -C3 -g '!node_modules' | head -80
echo ""
echo "=== Search for any place where reasoningChain is mentioned ==="
rg -n 'reasoningChain' --type=js -B10 -A10 -g '!node_modules'
echo ""
echo "=== Check backend/index.js for how incidents are created ==="
sed -n '40,150p' backend/index.jsRepository: SKfaizan-786/sentinel-devops-agent
Length of output: 6451
🏁 Script executed:
#!/bin/bash
echo "=== List all JS/TS files in backend ==="
find backend -type f \( -name '*.js' -o -name '*.ts' \) | head -30
echo ""
echo "=== Check if frontend directory exists ==="
ls -la frontend/ 2>/dev/null || echo "No frontend directory"
echo ""
echo "=== Search for description property usage ==="
rg -n '\.description' --type=js -C2 -g '!node_modules' | head -50
echo ""
echo "=== Search for where incident object properties are set ==="
rg -n '\{\s*.*id.*timestamp' --type=js -C3 -g '!node_modules' | head -50Repository: SKfaizan-786/sentinel-devops-agent
Length of output: 4040
🏁 Script executed:
#!/bin/bash
echo "=== List CLI files ==="
find cli -type f 2>/dev/null | head -20
echo ""
echo "=== Check cli/src/commands.js for incident structure ==="
cat cli/src/commands.js 2>/dev/null | head -200Repository: SKfaizan-786/sentinel-devops-agent
Length of output: 7249
🏁 Script executed:
#!/bin/bash
echo "=== Check all route files ==="
for file in backend/routes/*.js; do
echo "=== $file ==="
cat "$file" | head -100
done
echo ""
echo "=== Search for app.post, app.put, app.patch in index.js ==="
grep -n "app\.\(post\|put\|patch\)" backend/index.js | head -20Repository: SKfaizan-786/sentinel-devops-agent
Length of output: 12206
🏁 Script executed:
#!/bin/bash
echo "=== Check kestra-webhook endpoint and surrounding code ==="
sed -n '155,250p' backend/index.js
echo ""
echo "=== Check if there are any other POST endpoints that add to aiLogs ==="
sed -n '199,290p' backend/index.jsRepository: SKfaizan-786/sentinel-devops-agent
Length of output: 6898
🏁 Script executed:
#!/bin/bash
echo "=== Search for reasoningChain usage in external/API context ==="
rg -n 'reasoningChain|reasoning' --type=js -B3 -A3 -g '!node_modules' | head -100
echo ""
echo "=== Check if there are any type definitions or JSDoc comments ==="
sed -n '1,70p' backend/lib/postmortem-generator.js
echo ""
echo "=== Check what properties are expected on reasoningChain items ==="
grep -A20 "reasoningChain" backend/lib/postmortem-generator.js | head -30Repository: SKfaizan-786/sentinel-devops-agent
Length of output: 4541
Add fallback properties for reasoningChain entries to prevent "undefined" strings in the timeline.
Line 89 assumes all reasoningChain entries have a description property. If this property is missing, the template will render "- undefined". The proposed fix adds reasonable fallbacks:
Suggested fix
-${context.reasoningChain.map(r => `- ${r.description}`).join('\n') || '- AI analysis completed'}
+${context.reasoningChain.map(r => `- ${r.description || r.event || r.action || JSON.stringify(r)}`).join('\n') || '- AI analysis completed'}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/lib/postmortem-generator.js` around lines 88 - 89, The template
mapping over context.reasoningChain should guard against missing description
fields to avoid rendering "- undefined"; update the mapping used in
backend/lib/postmortem-generator.js (the reasoningChain.map callback) to use a
fallback like r.description || r.summary || r.note || 'No description provided'
(or default to the overall fallback '- AI analysis completed' when the array is
empty) so each entry prints a sensible string instead of "undefined".
Comment on lines
+243
to
+248
| try { | ||
| // Extract context | ||
| const context = extractIncidentContext(incident); | ||
|
|
||
| // Build prompt | ||
| const prompt = buildPrompt(context); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
extractIncidentContext is called twice — the result on line 245 is unused.
generatePostMortem calls extractIncidentContext(incident) on line 245, stores it in context, but then passes context (the enriched object) into buildPrompt(context) on line 248. Inside buildPrompt, extractIncidentContext is called again on the already-enriched object. The context variable on line 245 is never used elsewhere.
This is wasteful and confusing. Either pass the raw incident to buildPrompt directly, or refactor buildPrompt to accept an already-extracted context.
♻️ Option A: pass raw incident to buildPrompt
// Extract context
- const context = extractIncidentContext(incident);
-
- // Build prompt
- const prompt = buildPrompt(context);
+ // Build prompt (buildPrompt calls extractIncidentContext internally)
+ const prompt = buildPrompt(incident);♻️ Option B: have buildPrompt accept pre-extracted context
-function buildPrompt(incident) {
- const context = extractIncidentContext(incident);
+function buildPrompt(context) {
return `You are an expert SRE ...Then in generatePostMortem:
const context = extractIncidentContext(incident);
const prompt = buildPrompt(context);📝 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
| try { | |
| // Extract context | |
| const context = extractIncidentContext(incident); | |
| // Build prompt | |
| const prompt = buildPrompt(context); | |
| try { | |
| // Build prompt (buildPrompt calls extractIncidentContext internally) | |
| const prompt = buildPrompt(incident); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/lib/postmortem-generator.js` around lines 243 - 248,
generatePostMortem currently calls extractIncidentContext(incident) and stores
it in context but then passes that to buildPrompt which itself calls
extractIncidentContext again, so the first call and the context variable are
unused; fix by choosing one approach: either 1) remove the initial
extractIncidentContext call in generatePostMortem and pass the raw incident into
buildPrompt(incident), or 2) refactor buildPrompt to accept an already-extracted
context (remove its internal call to extractIncidentContext) and update
generatePostMortem to pass the context variable and delete the duplicate
extraction; ensure you remove the unused context variable or duplicate
extraction accordingly and adjust any function signatures or callers to match.
Comment on lines
+181
to
+190
| try { | ||
| const transporter = nodemailer.createTransport({ | ||
| host: smtpConfig.host, | ||
| port: parseInt(smtpConfig.port) || 587, | ||
| secure: false, | ||
| auth: { | ||
| user: smtpConfig.user, | ||
| pass: smtpConfig.pass | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
secure: false is hardcoded — breaks SMTPS on port 465.
If an operator configures SMTP_PORT=465 (implicit TLS), the connection will fail because secure must be true for port 465. Derive it from the port instead.
♻️ Suggested fix
+ const smtpPort = parseInt(smtpConfig.port) || 587;
const transporter = nodemailer.createTransport({
host: smtpConfig.host,
- port: parseInt(smtpConfig.port) || 587,
- secure: false,
+ port: smtpPort,
+ secure: smtpPort === 465,
auth: {
user: smtpConfig.user,
pass: smtpConfig.pass
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/lib/postmortem-publisher.js` around lines 181 - 190, The transport is
created with secure: false hardcoded which breaks implicit TLS (SMTPS) on port
465; update the nodemailer.createTransport call in postmortem-publisher.js to
compute secure based on smtpConfig.port (e.g., secure =
parseInt(smtpConfig.port) === 465) and use that value instead of the literal
false, ensuring the auth/port parsing around transporter creation
(smtpConfig.port, parseInt(...)) uses the same parsed port value.
Comment on lines
+46
to
+60
| if (!response.ok) { | ||
| // Safely parse error response | ||
| let errorMessage = `Failed to generate post-mortem (HTTP ${response.status})`; | ||
| try { | ||
| const errorData = await response.json(); | ||
| errorMessage = errorData.error?.message || errorMessage; | ||
| } catch { | ||
| // If JSON parse fails, try to get text or use status text | ||
| try { | ||
| const errorText = await response.text(); | ||
| errorMessage = errorText || response.statusText || errorMessage; | ||
| } catch { | ||
| errorMessage = response.statusText || errorMessage; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
response.text() will fail after response.json() consumes the body stream.
In the Fetch API, once the response body is read (even if response.json() throws a parse error), the stream is consumed. The fallback response.text() on line 55 will return an empty string or throw because the body has already been read. The outer catch block on line 57 will then catch this and use response.statusText, which is the effective fallback.
Consider reading the body as text first, then parsing as JSON:
♻️ Suggested fix
if (!response.ok) {
- // Safely parse error response
- let errorMessage = `Failed to generate post-mortem (HTTP ${response.status})`;
- try {
- const errorData = await response.json();
- errorMessage = errorData.error?.message || errorMessage;
- } catch {
- // If JSON parse fails, try to get text or use status text
- try {
- const errorText = await response.text();
- errorMessage = errorText || response.statusText || errorMessage;
- } catch {
- errorMessage = response.statusText || errorMessage;
- }
- }
+ let errorMessage = `Failed to generate post-mortem (HTTP ${response.status})`;
+ try {
+ const rawText = await response.text();
+ try {
+ const errorData = JSON.parse(rawText);
+ errorMessage = errorData.error?.message || errorMessage;
+ } catch {
+ errorMessage = rawText || response.statusText || errorMessage;
+ }
+ } catch {
+ errorMessage = response.statusText || errorMessage;
+ }
throw new Error(errorMessage);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sentinel-frontend/hooks/usePostMortemGeneration.ts` around lines 46 - 60, In
usePostMortemGeneration, the code calls response.json() then falls back to
response.text(), but the body stream is consumed by the first read; instead read
the body once into a text variable (e.g., const bodyText = await
response.text()), then try to parse JSON from that text (JSON.parse(bodyText))
to extract errorData.error?.message and if parsing fails use bodyText ||
response.statusText; update the error handling in the non-ok branch to use this
single-read approach so you don't attempt to read the response body twice.
- Change prop types to accept string IDs instead of numbers - Convert string IDs to numbers only when calling backend API - Add validation to ensure IDs can be converted to valid numbers - Fixes issue where mock IDs like 'inc-1' would fail parseInt This allows frontend to work with both mock data (string IDs) and real backend data (numeric IDs converted to strings in parseInsight)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
sentinel-frontend/app/dashboard/incidents/page.tsx (1)
141-149:lastGeneratedis an unstable function reference —useCallbackis invalidated on every render.
usePostMortemGenerationreturnslastGeneratedas an inline arrow function in its return statement, so its identity changes on every render. Listing it in thisuseCallbackdependency array meanshandleDownloadPostMortemis re-created on every render, defeating the memoisation. The same applies toisGeneratingbeing passed asisGeneratingPostMortem(Line 293), which will causeIncidentTableto see a new prop reference on every render.The fix belongs in
usePostMortemGeneration.ts— wrap bothisGeneratingandlastGeneratedinuseCallback:// in usePostMortemGeneration.ts const isGenerating = useCallback( (incidentId: number) => generatingIncidentIds.has(incidentId), [generatingIncidentIds] ); const lastGenerated = useCallback( (incidentId: number) => lastGeneratedMap.get(incidentId) ?? null, [lastGeneratedMap] );Once those are stable,
lastGeneratedcan safely remain in thisuseCallbackdependency array.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sentinel-frontend/app/dashboard/incidents/page.tsx` around lines 141 - 149, The issue is that usePostMortemGeneration returns unstable function references (lastGenerated and isGenerating) which causes consumers like handleDownloadPostMortem and the IncidentTable prop isGeneratingPostMortem to be re-created every render; to fix, update usePostMortemGeneration to memoize those functions with useCallback (wrap isGenerating and lastGenerated with useCallback and list their underlying maps/sets like generatingIncidentIds and lastGeneratedMap as dependencies) so their identities remain stable and then keep lastGenerated and isGeneratingPostMortem in the dependent components' dependency arrays without causing unnecessary re-renders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sentinel-frontend/app/dashboard/incidents/page.tsx`:
- Line 293: The build breaks because usePostMortemGeneration provides
isGenerating as (incidentId: number) => boolean while IncidentTable expects
(incidentId: string) => boolean; fix by wrapping the hook's isGenerating in a
string-to-number adapter before passing to IncidentTable (same pattern used in
handleGeneratePostMortem): create a small wrapper like isGenerating={(id:
string) => hookIsGenerating(Number(id))} so IncidentTable receives the correct
signature while delegating to usePostMortemGeneration's isGenerating.
- Around line 123-127: The current validation path sets showErrorMessage but
never sets postMortemError so the UI banner (guarded by showErrorMessage &&
postMortemError) never appears; add a local state (e.g., validationError or
reuse localError) to hold the validation message when numericId is invalid, set
it where Number.isFinite(numericId) check fails (alongside
setShowErrorMessage(true)), and update the banner condition and render logic to
display validationError when postMortemError is null (i.e., change the banner
guard from showErrorMessage && postMortemError to showErrorMessage &&
(postMortemError || validationError) and prefer showing postMortemError but fall
back to validationError); reference the existing hooks/vars numericId,
incidentId, setShowErrorMessage, postMortemError, generatePostMortem and
showErrorMessage to locate where to set and read this new state.
---
Duplicate comments:
In `@sentinel-frontend/app/dashboard/incidents/page.tsx`:
- Line 135: The catch block currently declares an unused binding ("catch
(error)") causing a lint warning; update the catch clause in the async code (the
catch following the try in page.tsx) to drop the unused identifier and use a
bare catch (i.e., "catch {") so the block executes the same cleanup/handling
without the unused "error" variable.
- Line 126: The three setTimeout calls that call
setShowErrorMessage/setShowSuccessMessage create stale timers that may run after
the component unmounts; capture each timer ID (from the calls at the three
locations where setTimeout is used) into a variable and clear them in a cleanup
(e.g., useEffect return or an explicit cleanup function) using
clearTimeout(timerId) when the component unmounts or before starting a new timer
so setShowErrorMessage/setShowSuccessMessage cannot be invoked on an unmounted
component; update the code paths that schedule timeouts to store and clear the
timer IDs (referencing the setShowErrorMessage and setShowSuccessMessage call
sites) accordingly.
---
Nitpick comments:
In `@sentinel-frontend/app/dashboard/incidents/page.tsx`:
- Around line 141-149: The issue is that usePostMortemGeneration returns
unstable function references (lastGenerated and isGenerating) which causes
consumers like handleDownloadPostMortem and the IncidentTable prop
isGeneratingPostMortem to be re-created every render; to fix, update
usePostMortemGeneration to memoize those functions with useCallback (wrap
isGenerating and lastGenerated with useCallback and list their underlying
maps/sets like generatingIncidentIds and lastGeneratedMap as dependencies) so
their identities remain stable and then keep lastGenerated and
isGeneratingPostMortem in the dependent components' dependency arrays without
causing unnecessary re-renders.
Comment on lines
+123
to
+127
| if (!Number.isFinite(numericId) || numericId <= 0) { | ||
| console.error('Invalid incident ID:', incidentId); | ||
| setShowErrorMessage(true); | ||
| setTimeout(() => setShowErrorMessage(false), 5000); | ||
| return; |
Contributor
There was a problem hiding this comment.
Silent failure on invalid ID: the error banner is never shown to the user.
When validation fails here, setShowErrorMessage(true) is set but postMortemError (from the hook) is still null because generatePostMortem was never called. The banner at Line 175 is guarded by showErrorMessage && postMortemError, so it stays hidden — the user sees nothing.
Introduce a local validationError state (or repurpose a unified localError state) to carry the message, and use it in the error banner alongside postMortemError:
🔧 Proposed fix
+ const [localError, setLocalError] = useState<string | null>(null);
...
if (!Number.isFinite(numericId) || numericId <= 0) {
console.error('Invalid incident ID:', incidentId);
+ setLocalError('Invalid incident ID provided.');
setShowErrorMessage(true);
setTimeout(() => setShowErrorMessage(false), 5000);
return;
}- {showErrorMessage && postMortemError && (
+ {showErrorMessage && (postMortemError || localError) && (
...
- <p className="text-xs text-muted-foreground">{postMortemError}</p>
+ <p className="text-xs text-muted-foreground">{postMortemError ?? localError}</p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sentinel-frontend/app/dashboard/incidents/page.tsx` around lines 123 - 127,
The current validation path sets showErrorMessage but never sets postMortemError
so the UI banner (guarded by showErrorMessage && postMortemError) never appears;
add a local state (e.g., validationError or reuse localError) to hold the
validation message when numericId is invalid, set it where
Number.isFinite(numericId) check fails (alongside setShowErrorMessage(true)),
and update the banner condition and render logic to display validationError when
postMortemError is null (i.e., change the banner guard from showErrorMessage &&
postMortemError to showErrorMessage && (postMortemError || validationError) and
prefer showing postMortemError but fall back to validationError); reference the
existing hooks/vars numericId, incidentId, setShowErrorMessage, postMortemError,
generatePostMortem and showErrorMessage to locate where to set and read this new
state.
| onSort={handleSort} | ||
| sortConfig={sortConfig} | ||
| onGeneratePostMortem={handleGeneratePostMortem} | ||
| isGeneratingPostMortem={isGenerating} |
Contributor
There was a problem hiding this comment.
Pipeline failure: isGenerating signature mismatch — this breaks the build.
usePostMortemGeneration exposes isGenerating: (incidentId: number) => boolean, but IncidentTable declares the prop as (incidentId: string) => boolean. Passing the hook function directly triggers TS2322.
Apply the same string-to-number conversion pattern already used in handleGeneratePostMortem:
🔧 Proposed fix
- isGeneratingPostMortem={isGenerating}
+ isGeneratingPostMortem={(incidentId: string) => isGenerating(parseInt(incidentId, 10))}📝 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
| isGeneratingPostMortem={isGenerating} | |
| isGeneratingPostMortem={(incidentId: string) => isGenerating(parseInt(incidentId, 10))} |
🧰 Tools
🪛 GitHub Actions: Lint & Test
[error] 293-293: TS2322: Type '(incidentId: number) => boolean' is not assignable to type '(incidentId: string) => boolean'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sentinel-frontend/app/dashboard/incidents/page.tsx` at line 293, The build
breaks because usePostMortemGeneration provides isGenerating as (incidentId:
number) => boolean while IncidentTable expects (incidentId: string) => boolean;
fix by wrapping the hook's isGenerating in a string-to-number adapter before
passing to IncidentTable (same pattern used in handleGeneratePostMortem): create
a small wrapper like isGenerating={(id: string) => hookIsGenerating(Number(id))}
so IncidentTable receives the correct signature while delegating to
usePostMortemGeneration's isGenerating.
mdhaarishussain
requested changes
Feb 23, 2026
mdhaarishussain
left a comment
mdhaarishussain
left a comment
Collaborator
There was a problem hiding this comment.
Requesting changes before this can be merged.
Blocking points still present:
- Frontend type mismatch/build stability issue in incidents integration (
isGeneratingcontract mismatch). - Post-mortem route flow still has unresolved auth/error-handling robustness gaps.
- API contract and failure-path handling are not yet reliable enough for production behavior.
Please resolve these blockers and I’ll re-review immediately.
Collaborator
Follow-up on requested changes (detailed)This clarifies the remaining blockers behind my Blocking items
Acceptance criteria
Once these are addressed, I’ll re-check immediately. |
Collaborator
|
Lets wrap it up @Omkarop0808 |
mdhaarishussain
approved these changes
Mar 8, 2026
Collaborator
|
Please Resolve The Merge Conflicts !!! @Omkarop0808 |
Contributor
Author
|
@mdhaarishussain currently my exam are ongoing so it's very difficult to do so but I will try if I get time. |
Collaborator
Apertre Ends today so we need to get this merged before 12 am |
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.
📋 Description
This PR implements an Automated Incident Post-Mortem Generator that uses LLaMA AI to automatically generate comprehensive post-mortem documents for resolved incidents. When an incident is resolved, users can click a button to generate a professional markdown document containing 8 structured sections: Executive Summary, Incident Timeline, Root Cause Analysis, Impact Assessment, Remediation Steps, Contributing Factors, Lessons Learned, and Prevention Measures.
The feature includes:
sentinel-postmortems/directory🔗 Related Issues
Closes #75
🎯 Type of Change
🧪 How Has This Been Tested?
Manual Testing:
/dashboard/incidents.mdformat with proper markdown contentTest Configuration:
✅ Checklist
📸 Screenshots
1. Incidents List Page
Shows both incidents in the table with expandable rows
2. Expanded Incident with Generate Button
Timeline and "Generate Post-Mortem" button visible
3. Generating State (Per-Incident Loading)
Only the specific incident shows "Generating..." while others remain normal
4. Success Notification
Green notification with download button appears after generation
5. Generated Post-Mortem File
Markdown file with all 8 sections properly formatted
6. File Explorer
Generated files stored in sentinel-postmortems/ directory
📝 Additional Notes
Implementation Details:
Backend:
backend/lib/postmortem-generator.js- Core generation logic using GROQ LLaMA APIbackend/lib/postmortem-publisher.js- File storage and multi-target publishingbackend/routes/postmortem.routes.js- RESTful API endpointsnodemailerdependency for email publishing supportFrontend:
sentinel-frontend/hooks/usePostMortemGeneration.ts- Custom hook for generation and downloadBug Fixes:
useTheme.tsto fix Next.js build errorEnvironment Variables:
Add to
backend/.env:Key Features:
Future Enhancements:
Summary by CodeRabbit
New Features
Chores
Other