Open
Conversation
Core detection and scanning infrastructure: - PII detection (email, phone, SSN, IP, credit cards) - Secrets detection (API keys, tokens, connection strings) - Prompt injection detection with structural analysis - Invisible character and encoding attack detection - Malicious URL detection and dangerous code patterns - Output injection detection for AI responses - Security event logging system - Support for warn, sanitize, and block action modes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Settings infrastructure: - LLM Guard settings tab in Settings Modal - Settings persistence in settingsStore - Configuration import/export functionality - Per-session security policies support - Custom regex patterns configuration - TypeScript types for security features Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Visual components for security features: - LlmGuardIndicator with real-time status - SecurityBadge for session list indicators - SecurityEventsPanel for Right Bar - ScanProgressIndicator for input feedback - SanitizedContentDiff for visual diff view - FindingDetails for security findings - SensitiveContentOverlay for input highlighting - SecurityRecommendationsPanel - SessionSecurityModal for per-session policies Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Integration layer: - useSensitiveContentDetection hook for real-time preview - useSecurityToasts hook for notifications - Keyboard shortcuts for security features - Group Chat inter-agent protection - Security recommendations system - App.tsx and store integration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
StdoutHandler.ts: - Move synopsis logging after sanitization to avoid logging raw sensitive content - Use guardedText preview instead of raw event.text/streamedText RightPanel.tsx: - Extract loadSecurityEventIds as useCallback for reuse - Update refreshSecurityEvents to also refresh badge by calling loadSecurityEventIds group-chat-router.ts: - Add post-loop check when all mentioned participants are blocked - Reset state to idle and remove power block when no participants respond - Emit system warning message when round is cancelled due to all blocks - Fix historyContext sanitization bypass by rebuilding context per-participant - Use sanitized message in historyContext to prevent raw message leakage Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Enhanced `docs.json` to include LLM Guard in the Encore Features section. - Updated `encore-features.md` to reflect the addition of LLM Guard with a description and quick start guide. - Improved `llm-guard.md` with detailed configuration options and detection types for better user guidance.
Skip logging prompt preview on Windows when LLM Guard detected findings, even in warn mode where effectivePrompt may still contain sensitive content. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use activeSession.id instead of activeTab?.agentSessionId to properly scope security events to the current session. The Maestro session ID matches what's used in logSecurityEvent, so events are correctly filtered to the current session. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add llmGuard to EncoreFeatureFlags type - Remove promptLlmGuardEnabled prop (not in AppModals) - Remove spellcheck handlers (deleted in rc) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a new LLM Guard security subsystem: local input/output scanning (PII, secrets, injection, malicious URLs, dangerous code), pre/post/inter-agent pipelines, custom patterns, config import/export, event logging/export, recommendations, IPC/preload APIs, renderer UI (indicator/badge/panel/diff), and process-manager integrations. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Renderer
participant Preload as Preload (maestro.security)
participant Main as Main (IPC)
participant ProcessMgr as Process Manager
participant Logger as Security Logger
User->>Renderer: type input / click spawn
Renderer->>Preload: scanInputPreview (optional)
Preload->>Main: invoke scan preview
Renderer->>Main: invoke process:spawn (config + sessionSecurityPolicy)
Main->>ProcessMgr: runLlmGuardPre(prompt)
ProcessMgr->>Logger: logSecurityEvent (input_scan) if findings
alt Pre-scan blocked
ProcessMgr-->>Renderer: return blocked error/response
else Pre-scan allowed (maybe sanitized)
ProcessMgr->>ProcessMgr: spawn child with llmGuardState
ProcessMgr-->>Main: stream stdout/stderr chunks
Main->>Main: runLlmGuardPost on chunks
Main->>Logger: logSecurityEvent (output_scan / blocked/warn)
Logger-->>Renderer: emit security:event
Renderer->>Renderer: update UI (indicator, badge, toasts, panel)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Greptile SummaryThis PR introduces LLM Guard, a comprehensive built-in security layer for Maestro that scans all AI inputs and outputs for sensitive content. The feature includes PII detection and anonymization, secrets scanning (50+ token types), prompt injection detection, dangerous code scanning, malicious URL detection, inter-agent scanning for Group Chat, per-session security policies, audit log export, and a settings UI — all behind an Encore feature flag. The architecture is well-thought-out and the breadth of detection patterns is impressive. However, there are two significant issues that should be addressed before shipping:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Renderer
participant InputGuard as Input Guard (Main)
participant Process as Agent Process
participant OutputGuard as Output Guard (Main)
participant SecurityLog as Security Logger
User->>Renderer: Type prompt
Renderer->>Renderer: scanInputPreview (lightweight PII/secret scan)
Renderer->>Renderer: Show SensitiveContentPill indicators
User->>Renderer: Submit prompt
Renderer->>InputGuard: runLlmGuardPre(text, config)
InputGuard->>InputGuard: anonymizePii → PiiVault
InputGuard->>InputGuard: redactSecrets
InputGuard->>InputGuard: detectPromptInjection
InputGuard->>InputGuard: structuralAnalysis
InputGuard->>InputGuard: invisibleCharDetection
InputGuard->>InputGuard: scanUrls
InputGuard->>InputGuard: checkBannedContent
InputGuard->>InputGuard: applyCustomPatterns
alt blocked
InputGuard-->>Renderer: Block + reason
else sanitized/warned
InputGuard->>SecurityLog: logSecurityEvent (input_scan)
InputGuard->>Process: sanitizedText
end
Process->>OutputGuard: streaming chunk (thinking-chunk)
OutputGuard->>OutputGuard: runLlmGuardPost(chunk)
OutputGuard->>SecurityLog: logSecurityEvent (per chunk — duplicate risk)
OutputGuard->>Renderer: guardedChunk
Process->>OutputGuard: final result
OutputGuard->>OutputGuard: runLlmGuardPost(fullText)
OutputGuard->>OutputGuard: deanonymizePii (from vault)
OutputGuard->>OutputGuard: redactSecrets
OutputGuard->>OutputGuard: detectPiiLeakage
OutputGuard->>OutputGuard: detectOutputInjection
OutputGuard->>OutputGuard: scanCode
OutputGuard->>OutputGuard: scanUrls
OutputGuard->>SecurityLog: logSecurityEvent (output_scan)
OutputGuard->>Renderer: guardedResult
Note over SecurityLog: Findings include raw values<br/>persisted to ~/.maestro/security-events.jsonl
Renderer->>Renderer: Show SecurityBadge / SecurityEventsPanel
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
src/__tests__/renderer/components/SecurityEventsPanel.test.ts-104-116 (1)
104-116:⚠️ Potential issue | 🟡 MinorMissing
vi.useFakeTimers()beforevi.setSystemTime().
vi.setSystemTime()requires fake timers to be enabled first viavi.useFakeTimers(). Without it, the mocked time won't take effect andDate.now()insidebuildExportFilterswill use real system time.Proposed fix
it('sets startDate for 7d date range', () => { const now = Date.now(); + vi.useFakeTimers(); vi.setSystemTime(now); const filters = buildExportFilters('7d', '', '', new Set(), 0);Apply similar fix to the
30dtest case at line 118.Also applies to: 118-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/SecurityEventsPanel.test.ts` around lines 104 - 116, The tests call vi.setSystemTime() but never enable fake timers; before calling vi.setSystemTime() in the 'sets startDate for 7d date range' test (and likewise the '30d' test), call vi.useFakeTimers() to enable fake timers, then set the system time, and keep the existing vi.useRealTimers() at the end to restore real timers; update references around buildExportFilters, vi.setSystemTime, vi.useFakeTimers, and vi.useRealTimers accordingly.src/renderer/components/InputArea.tsx-993-993 (1)
993-993:⚠️ Potential issue | 🟡 MinorDisable browser spellcheck in terminal mode.
This textarea is shared with shell input, so
spellCheck={true}will underline commands, flags, and paths as misspellings. Gate it to AI mode (spellCheck={!isTerminalMode}) or make it configurable per input mode.💡 Suggested fix
- spellCheck={true} + spellCheck={!isTerminalMode}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/InputArea.tsx` at line 993, The textarea in InputArea.tsx currently sets spellCheck={true}, causing terminal/shell inputs to be underlined; change the prop to be conditional (e.g., spellCheck={!isTerminalMode}) or otherwise gate it by the input mode flag used in this component (reference the InputArea component and the textarea/hidden input element that renders user input and the isTerminalMode state/prop) so browser spellchecking is disabled when in terminal mode and enabled for AI/text modes.src/renderer/components/ScanProgressIndicator.tsx-51-109 (1)
51-109:⚠️ Potential issue | 🟡 MinorReset scan state when the subscription scope changes.
Cleanup unsubscribes but
isScanningandcontentLengthpersist acrossenabled,sessionId, ortabIdchanges. If the user switches tabs mid-scan or toggles the feature, the old scanning indicator renders for the new scope until another event arrives, since the render check only verifies!enabled || !isScanning.🧭 Suggested reset on scope change
useEffect(() => { + setIsScanning(false); + setContentLength(0); + scanStartTimeRef.current = null; + if (hideTimeoutRef.current) { + clearTimeout(hideTimeoutRef.current); + hideTimeoutRef.current = null; + } + if (!enabled || !window.maestro?.security?.onScanProgress) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/ScanProgressIndicator.tsx` around lines 51 - 109, When the subscription scope changes (useEffect deps enabled, sessionId, tabId) we must reset lingering scan state so the old indicator doesn't persist: at the start of the effect (or in the cleanup before calling unsubscribe()) clear any hideTimeoutRef (clearTimeout and null it), setIsScanning(false), setContentLength(0) and reset scanStartTimeRef.current = 0 (or null). Ensure these resets happen when enabled becomes false or when the effect re-subscribes so the UI is in a clean state until a new scan_start event updates it.docs/llm-guard.md-175-215 (1)
175-215:⚠️ Potential issue | 🟡 MinorAdd blank lines around tables in “Dangerous Code Detection”.
The tables in this section are not surrounded by blank lines (MD058), which will keep docs lint noisy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/llm-guard.md` around lines 175 - 215, The markdown tables in the "Dangerous Code Detection" section (the Shell Commands, SQL Injection, Command Injection, Sensitive File Access, and Network Operations tables) lack surrounding blank lines which triggers MD058; add a single blank line before and after each table so each table block is separated from surrounding paragraphs and headings, ensuring the tables render cleanly and satisfy the linter.docs/llm-guard.md-26-35 (1)
26-35:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks.
These fences are missing a language tag (MD040). Use
textfor diagrams/example blocks to satisfy markdownlint and improve rendering consistency.Also applies to: 238-244, 248-254, 258-264, 268-274, 278-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/llm-guard.md` around lines 26 - 35, The fenced ASCII diagram blocks in docs/llm-guard.md are missing language identifiers (MD040); update each triple-backtick fence for the diagram examples (the block that starts with the ASCII flowchart like "┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐") to use a language tag of text (i.e., change ``` to ```text) for the block shown and apply the same change to the other diagram blocks indicated (lines referenced in the review: 238-244, 248-254, 258-264, 268-274, 278-284) so markdownlint MD040 is satisfied and rendering is consistent.src/renderer/components/FindingDetails.tsx-259-262 (1)
259-262:⚠️ Potential issue | 🟡 MinorPotential runtime error if email format is malformed.
The
split('@')call assumes a valid email format after theincludes('@')check. If the value is"@"or ends with@,localcould be empty, causinglocal[0]to be undefined.🛡️ Defensive fix
// For email, show first char and domain if (type.includes('EMAIL') && value.includes('@')) { const [local, domain] = value.split('@'); + if (!local || !domain) return value.slice(0, 4) + '***'; return local[0] + '***@' + domain; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/FindingDetails.tsx` around lines 259 - 262, The email-masking branch in FindingDetails.tsx assumes a non-empty local part and uses value.split('@') and local[0], which can throw if value is "@" or ends with "@"; update the EMAIL handling (the if block that checks type.includes('EMAIL')) to defensively split using value.split('@', 2), verify both local and domain are non-empty, and if local is empty or undefined fall back to a safe mask (e.g., '***@' + domain or '***' if domain missing) or return a sanitized placeholder; ensure you reference and replace uses of local[0] with a guarded access so no runtime undefined access occurs.src/main/process-manager/handlers/StdoutHandler.ts-550-563 (1)
550-563:⚠️ Potential issue | 🟡 MinorAdd
.catch()handler for unhandled promise rejection.The
logSecurityEvent(...).then(...)chain lacks a.catch()handler. IflogSecurityEventrejects, this creates an unhandled promise rejection that could crash the process or go unnoticed.🛡️ Proposed fix
logSecurityEvent({ sessionId, tabId: managedProcess.tabId, eventType: guardResult.blocked ? 'blocked' : 'output_scan', findings: guardResult.findings, action, originalLength: resultText.length, sanitizedLength: guardResult.sanitizedResponse.length, }).then((event) => { // Emit to ProcessManager listeners for real-time UI forwarding const eventData: SecurityEventData = { sessionId: event.sessionId, tabId: event.tabId, eventType: event.eventType, findingTypes: event.findings.map((f) => f.type), findingCount: event.findings.length, action: event.action, originalLength: event.originalLength, sanitizedLength: event.sanitizedLength, }; this.emitter.emit('security-event', eventData); + }).catch((error) => { + logger.error('[LLMGuard] Failed to log security event', 'LLMGuard', { + sessionId, + error: error instanceof Error ? error.message : String(error), + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/handlers/StdoutHandler.ts` around lines 550 - 563, The promise returned by logSecurityEvent(...) is not handling rejections; append a .catch(...) to the existing .then(...) chain on logSecurityEvent to prevent unhandled promise rejections, and inside the catch call the handler's logging facility (e.g., this.processLogger.error or this.logger.error, falling back to console.error) to record the error and context (include event.sessionId/event.tabId and the error). Ensure the catch does not rethrow so it won't crash the process, but still surfaces the failure in logs.src/__tests__/main/security/recommendations.test.ts-571-583 (1)
571-583:⚠️ Potential issue | 🟡 MinorThis test never checks that items are actually actionable.
startsWithActionWordis computed and then discarded, so the case currently only assertsitem.length > 5. That lets passive filler text pass even though the test name promises stronger coverage.🧪 Tighten the assertion
for (const rec of recommendations) { + let actionableItemCount = 0; for (const item of rec.actionItems) { // Action items should start with action verbs or referential phrases // Include common action starters from the recommendations const startsWithActionWord = /^(Review|Consider|Enable|Add|Check|Verify|Use|Ensure|Lower|Define|Be|Current|Sanitize)/i.test( item ); // Items should be non-empty meaningful strings expect(item.length).toBeGreaterThan(5); - // At least some items should be actionable - // (not all items start with verbs - some provide context like "Current threshold: 70%") + if (startsWithActionWord) { + actionableItemCount += 1; + } } + expect(actionableItemCount).toBeGreaterThan(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/security/recommendations.test.ts` around lines 571 - 583, The test computes startsWithActionWord but never asserts it; update the loop over recommendations/rec.actionItems to assert actionability by either (a) adding expect(startsWithActionWord).toBeTruthy() for items that should be actionable, or (b) collect the boolean into an array per rec and assert at least one true (e.g., ensure each rec has at least one actionable item), referencing the existing startsWithActionWord variable and the rec.actionItems iteration to locate where to add the assertion.
🧹 Nitpick comments (12)
src/__tests__/renderer/components/SecurityEventsPanel.test.ts (2)
28-95: Consider importing actual helper functions from the component.The test file duplicates the logic from
SecurityEventsPanel(e.g.,buildExportFilters,generateExportFilename,toggleEventType). This creates a maintenance risk: if the component's logic changes, these tests may still pass while the actual component is broken.If these helpers are already extracted in the component, import them directly. Otherwise, consider extracting them to a shared utility file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/SecurityEventsPanel.test.ts` around lines 28 - 95, The test duplicates helper logic (buildExportFilters, generateExportFilename, getMimeType, toggleEventType and related types like ExportFilterOptions/ExportFormat), which risks desynchronization; replace the local helper copies by importing the real implementations from SecurityEventsPanel (or move them to a shared util and import that util in both the component and this test), update the test imports to reference those exported functions/types, and remove the duplicated functions so tests exercise the actual production logic.
341-368: Some tests verify JavaScript arithmetic rather than component behavior.Tests like "handles maximum value" (asserting
100/100 === 1) and "handles increments of 5" test basic division. Consider focusing test effort on actual component integration or edge cases that could fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/SecurityEventsPanel.test.ts` around lines 341 - 368, The current tests under "confidence slider" assert basic arithmetic instead of component behavior; replace those unit checks with integration tests that render the SecurityEventsPanel and simulate slider interactions: mount/render SecurityEventsPanel, locate the slider input, fire change events for min (0), max (100), and increments (0,5,...,100), and assert the component's slider change handler (e.g., onConfidenceChange or handleConfidenceChange) is invoked with the expected decimal values (value/100) and that component state/props reflect the decimal (0–1) rather than asserting raw division; also include edge-case assertions for clamping and step handling.src/main/security/llm-guard/patterns-metadata.json (1)
1-14: Consider clarifying deduplication in metadata.The pattern count discrepancy is intentional: the generator script deduplicates patterns across sources, combining 64 gitleaks rules and 1606 secrets-patterns-db patterns into 1652 unique patterns. While the script logs this as "(deduplicated)", the metadata JSON provides no indication of this process. Consider adding a
"deduplicatedCount": 18field to make the metadata self-explanatory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/security/llm-guard/patterns-metadata.json` around lines 1 - 14, The metadata JSON lacks an explicit field documenting how many patterns were removed during deduplication, causing the totals (64 + 1606 = 1670 vs "totalPatterns": 1652) to be unclear; update src/main/security/llm-guard/patterns-metadata.json by adding a "deduplicatedCount" field (e.g., 18) alongside "totalPatterns" and/or a short "note" field to indicate that totalPatterns is the deduplicated count so readers and tooling can understand the discrepancy when inspecting the "sources" and "totalPatterns" values.src/main/preload/process.ts (1)
54-56: Collapse prompt-delivery flags into one mode field.
sendPromptViaStdinandsendPromptViaStdinRawallow contradictory combinations and force the main handler to invent precedence. A singlepromptDeliveryMode?: 'argv' | 'stdin' | 'stdin-raw'keeps the IPC contract unambiguous; if you keep the booleans, at least reject invalid combinations explicitly.♻️ Suggested API shape
- // Stdin options for prompt delivery - sendPromptViaStdin?: boolean; - sendPromptViaStdinRaw?: boolean; + // Prompt delivery mode + promptDeliveryMode?: 'argv' | 'stdin' | 'stdin-raw';As per coding guidelines, "Surface assumptions early before implementing non-trivial work. Explicitly state assumptions as: 'Assumptions: 1) X, 2) Y. Correct me now or I proceed.' Never silently fill in ambiguous requirements."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/preload/process.ts` around lines 54 - 56, The two boolean flags sendPromptViaStdin and sendPromptViaStdinRaw create ambiguous/contradictory states; replace them with a single union field promptDeliveryMode?: 'argv' | 'stdin' | 'stdin-raw' (or, if you prefer a minimal change, add explicit validation in the IPC input parsing to reject simultaneous true/contradictory combinations) so the main handler (e.g., the function that reads IPC payloads and delivers prompts — locate uses of sendPromptViaStdin/sendPromptViaStdinRaw in the preload/process.ts handler) no longer needs to invent precedence; update all consumers to read promptDeliveryMode and branch on its three values (or throw/return an error on invalid boolean combos) and add tests for each mode and for invalid combinations.src/renderer/components/SanitizedContentDiff.tsx (2)
784-802: Unused variablecolors(same issue).The
colorsvariable is declared at line 786 but never used, similar to line 720.♻️ Remove unused variable
{sanitizedSegments.map((segment, idx) => { if (segment.type === 'added') { - const colors = getHighlightColor(segment.findingType || ''); return ( <span key={idx}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SanitizedContentDiff.tsx` around lines 784 - 802, In SanitizedContentDiff's mapping over sanitizedSegments (the map that handles segment.type === 'added'), remove the unused declaration const colors = getHighlightColor(segment.findingType || '') (or instead apply it if you intended to use its values); specifically update the added-segment branch inside the sanitizedSegments.map so there are no unused variables (refer to getHighlightColor and the added-segment span in the SanitizedContentDiff component).
718-736: Unused variablecolors.The
colorsvariable is declared at line 720 but never used in the rendering logic for removed segments.♻️ Remove unused variable
{originalSegments.map((segment, idx) => { if (segment.type === 'removed') { - const colors = getHighlightColor(segment.findingType || ''); return ( <span key={idx}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SanitizedContentDiff.tsx` around lines 718 - 736, In SanitizedContentDiff component, the variable colors produced by calling getHighlightColor(segment.findingType || '') inside the originalSegments.map for removed segments is unused; remove that unused declaration (the call to getHighlightColor stored in colors) from the removed-branch so you only keep the rendering logic that uses segment and theme, or if intended, replace the current hardcoded styles with values from colors returned by getHighlightColor; update the removed-segment branch (the mapping over originalSegments and the removed-case) accordingly to eliminate the unused variable.scripts/refresh-llm-guard-patterns.mjs (2)
84-86: TOML regex parsing may miss patterns with embedded quotes.The regex
section.match(/^regex\s*=\s*'''([^']+)'''/m)uses[^']+which works for triple-quoted strings without any single quotes. However, some TOML patterns may contain single quotes within the triple-quoted block. Consider using a non-greedy match or handling escaped quotes.♻️ More robust triple-quote handling
- const regexMatch = - section.match(/^regex\s*=\s*'''([^']+)'''/m) || section.match(/^regex\s*=\s*"([^"]+)"/m); + const regexMatch = + section.match(/^regex\s*=\s*'''([\s\S]*?)'''/m) || section.match(/^regex\s*=\s*"([^"]+)"/m);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/refresh-llm-guard-patterns.mjs` around lines 84 - 86, The current parsing of triple-quoted TOML regex in scripts/refresh-llm-guard-patterns.mjs uses section.match(...) with the pattern /^regex\s*=\s*'''([^']+)'''/m which fails when the block contains single quotes; update the matching logic used around regexMatch to capture triple-quoted content more robustly (for example use a non-greedy, dot-all aware capture or a pattern that allows internal single quotes) so rule.regex = regexMatch[1] receives the full content; adjust the first branch that sets regexMatch (and/or add a separate branch for triple-single-quoted blocks) to use a safer capture for section and ensure flags allow newlines.
41-61: Consider adding a request timeout.The
httpsGetfunction has no timeout, which could cause the script to hang indefinitely if the upstream server becomes unresponsive. For a developer tool this is low priority, but adding a timeout would improve reliability.♻️ Add timeout handling
function httpsGet(url) { return new Promise((resolve, reject) => { - https - .get(url, { headers: { 'User-Agent': 'Maestro-LLMGuard-Refresher' } }, (res) => { + const req = https.get( + url, + { headers: { 'User-Agent': 'Maestro-LLMGuard-Refresher' }, timeout: 30000 }, + (res) => { if (res.statusCode === 301 || res.statusCode === 302) { return resolve(httpsGet(res.headers.location)); } // ... rest of handler - }) - .on('error', reject); + } + ); + req.on('error', reject); + req.on('timeout', () => { + req.destroy(); + reject(new Error(`Request timeout: ${url}`)); + }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/refresh-llm-guard-patterns.mjs` around lines 41 - 61, The httpsGet function lacks a timeout and can hang; update httpsGet to accept an optional timeout (e.g., default 10000 ms) and apply it to the outgoing request by storing the returned ClientRequest from https.get, calling request.setTimeout(timeout, ...) to abort the request and reject with a timeout Error, and ensure the redirect branch (the recursive call to httpsGet(res.headers.location)) passes the same timeout; also clear or disable the timeout on successful response end and wire request.on('error') and response.on('error') to reject as before.src/renderer/components/SecurityBadge.tsx (1)
86-88: UnusedlastEventTimeRef.The
lastEventTimeRefis written to at line 111 but never read. If this is for future debugging or features, consider adding a comment; otherwise, it can be removed.♻️ Remove if not needed
// Timeout refs for auto-dismiss const dismissTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); - const lastEventTimeRef = useRef<number | null>(null);And remove line 111:
- lastEventTimeRef.current = Date.now();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SecurityBadge.tsx` around lines 86 - 88, The ref lastEventTimeRef declared alongside dismissTimeoutRef in SecurityBadge is never read; either remove the declaration lastEventTimeRef and the assignment that writes to it (the unused write near the event handler), or if you intend to keep it for future use, add a concise comment above the declaration explaining its purpose (e.g., reserved for rate-limiting/debugging) so linters reviewers know it’s intentional; update or remove any imports/usages related to lastEventTimeRef (and keep dismissTimeoutRef unchanged).src/__tests__/main/security/llm-guard/config-export.test.ts (1)
18-56: Round-trip coverage still missesoutput.detectOutputInjection.The shared
validConfigfixture never enables that new output flag, so export/import/validation can accidentally drop it and this suite still passes. Please add it to at least one happy-path fixture and assert it survives the round-trip.Also applies to: 250-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/security/llm-guard/config-export.test.ts` around lines 18 - 56, Add the missing output.detectOutputInjection flag to the test fixture and assert it survives the round-trip: update the validConfig object in src/__tests__/main/security/llm-guard/config-export.test.ts to include output.detectOutputInjection: true (or the expected boolean) alongside the other output flags, then update/add assertions in the relevant round-trip export/import/validation tests (the ones that reference validConfig and the export/import functions around lines 250-338) to explicitly check that importedConfig.output.detectOutputInjection equals the original value so the test fails if the flag is dropped.src/renderer/components/LlmGuardIndicator.tsx (1)
23-79: Either render or remove the disabled-state path.Line 211 returns
nullwhenenabledis false, so these branches are now unreachable:LlmGuardStatus['disabled'],getStatusConfig('disabled'), and the disabled tooltip copy. Should I remove these now-unused elements, or is the intent to show the dimmed indicator after all?As per coding guidelines: "After refactoring, identify now-unreachable code, list it explicitly, ask 'Should I remove these now-unused elements: [list]?'"
Also applies to: 177-180, 209-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/LlmGuardIndicator.tsx` around lines 23 - 79, The refactor left the 'disabled' branch unreachable—decide whether to render a dimmed indicator when enabled is false or remove the unused disabled code: either (A) update LlmGuardIndicator to render the disabled state instead of returning null (so LlmGuardStatus 'disabled', getStatusConfig('disabled'), and the disabled tooltip copy remain used), or (B) remove the unused elements: the 'disabled' union member in LlmGuardStatus, the 'disabled' case in getStatusConfig, and the disabled tooltip/text used by the component (and any related constants/imports); pick one approach and apply consistently in the LlmGuardIndicator component and its tooltip handling.src/main/security/llm-guard/code-scanner.ts (1)
11-12: The default API doesn't enforce the "code-context only" contract.The matcher records
inCodeBlock, butscanCode()defaultscodeBlocksOnlytofalse, andcontainsDangerousCode()inherits that behavior. Plain prose like "don't run rm -rf /" will still be flagged unless every caller opts in, which contradicts the module description.Also applies to: 389-404, 428-431, 482-484
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/security/llm-guard/code-scanner.ts` around lines 11 - 12, The scanner currently records inCodeBlock but leaves scanCode() and containsDangerousCode() defaulting codeBlocksOnly to false, so plain prose can be flagged; change the default behavior to enforce the "code-context only" contract by setting codeBlocksOnly default to true in scanCode() and containsDangerousCode(), update any internal calls to pass that default, and ensure the matcher continues to check the inCodeBlock flag (e.g., the matcher and its inCodeBlock checks in the scanning flow) so patterns are only flagged when inCodeBlock is true unless a caller explicitly overrides codeBlocksOnly=false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/ipc/handlers/process.test.ts`:
- Line 435: The test includes a GitHub-PAT-shaped literal in the test fixture
under the prompt property which triggers secret scanners; replace the hardcoded
"ghp_..." token by constructing it from non-secret pieces (e.g., concatenate
"ghp_" + "1234..." or use a clearly non-PAT placeholder like "<GITHUB_TOKEN>")
so the test string still exercises the same behavior without a PAT-shaped
literal. Locate the prompt value in the failing test (the prompt property in the
test fixture in process.test.ts) and update it to build or use a safe
placeholder instead of a single PAT-looking literal.
In `@src/__tests__/renderer/components/SanitizedContentDiff.test.ts`:
- Around line 16-249: The tests reimplement helpers (sortFindingsByPosition,
buildOriginalSegments, buildSanitizedSegments, buildInlineSegments,
reconstructContentFromFindings) instead of exercising the real logic; update the
test to import and use the real SanitizedContentDiff component or the production
helper exports instead of local copies: remove the duplicated helper
implementations from SanitizedContentDiff.test.ts, add an import for
SanitizedContentDiff (or export the helper functions from the production module
and import those), then render the component with test data (or call the
exported helpers) and assert against the real outputs so the suite validates the
actual implementation rather than a mirrored copy.
In `@src/__tests__/renderer/hooks/useSecurityToasts.test.ts`:
- Around line 23-24: securityEventCallback is nullable but called directly in
many test locations; add a small helper function (e.g.,
invokeSecurityEvent(event: SecurityEventData)) that asserts
securityEventCallback is non-null (throw or use expect to fail) and then calls
it, update beforeEach to still assign securityEventCallback, and replace every
direct invocation of securityEventCallback(...) in the test file (references:
securityEventCallback, beforeEach) with invokeSecurityEvent(...) so calls are
null-safe and fail clearly if not initialized.
In `@src/main/group-chat/group-chat-router.ts`:
- Around line 832-835: The current code only omits the last raw moderator entry
(chatHistory.slice(-15, -1)), which lets older raw moderator turns re-enter
prompts; instead iterate over chatHistory (loaded via
readLog(updatedChat.logPath)) and either (A) replace/sanitize every
moderator-type entry before using historyExceptLast/constructing per-participant
historyContext, or (B) persist sanitized moderator entries back into the log
when sanitization occurs so subsequent reads only see safe content; update the
logic around chatHistory, historyExceptLast, and any similar blocks (also at the
other occurrence around lines 926-933) to apply sanitization to all historical
moderator messages rather than special-casing only the newest row.
In `@src/main/process-manager/handlers/ExitHandler.ts`:
- Around line 65-86: The promise returned by logSecurityEvent(...) in
ExitHandler.ts lacks rejection handling; update the chain on the call site (the
logSecurityEvent(...).then(...) block that emits via
this.emitter.emit('security-event', ...)) to append .catch((err) =>
captureException(err, { extra: { sessionId, tabId: managedProcess.tabId,
eventType: guardResult.blocked ? 'blocked' : 'output_scan' }})); also add the
import for captureException from the Sentry utility (import { captureException }
from '../../utils/sentry') so any persistence errors are captured with context.
In `@src/main/process-manager/handlers/StderrHandler.ts`:
- Around line 37-61: applyOutputGuard currently only logs warnings for LLMGuard
findings; update it to mirror the guarded-exit flow by calling
logSecurityEvent(...) and emitting the live 'security-event' via the same
emitter used elsewhere whenever guardResult.findings.length > 0 (before
returning). Use managedProcess and sessionId to populate the event (include
toolType, findings array, sanitizedResponse and blockReason when present), and
ensure the blocked branch also records/emits the same security event payload
prior to returning the blocked message so guarded Codex stderr events are
persisted and forwarded to the live stream.
In `@src/main/security/llm-guard/code-scanner.ts`:
- Around line 250-259: The shebang regex in CODE_BLOCK_PATTERNS.shebang is
declared but never used; update isInCodeBlock and extractCodeBlocks to consult
CODE_BLOCK_PATTERNS.shebang so files that start with a shebang are treated as
code blocks when codeBlocksOnly is enabled. Concretely, add a check in
isInCodeBlock to test the surrounding text/line against
CODE_BLOCK_PATTERNS.shebang (e.g., test the line at the index or file start) and
extend extractCodeBlocks to include matches of CODE_BLOCK_PATTERNS.shebang as
code blocks (capture the shebang line and the following script content as a
single block or until the next blank delimiter) so raw scripts like
"#!/bin/bash\nrm -rf /" are recognized and not filtered out.
In `@src/main/security/llm-guard/config-export.ts`:
- Around line 221-237: The current validation for settings.banTopicsPatterns
uses validateRegex() but only pushes warnings, allowing invalid patterns to be
kept by extractConfig() and rendering bans inert in checkBannedContent(); change
the logic so invalid regexes are treated as validation errors (push to errors
instead of warnings) and prevent successful import when any banTopicsPatterns
entry fails validateRegex(), or alternatively filter out invalid patterns before
returning from extractConfig() and add an error describing which indices were
invalid; update code paths referencing settings.banTopicsPatterns,
validateRegex, extractConfig, and checkBannedContent to ensure invalid patterns
cannot be persisted as a successful import.
- Around line 175-181: The output boolean whitelist (outputBooleans) used during
import validation omits detectOutputInjection, allowing non-boolean values to
pass through and be cast into runtime config by extractConfig(); fix this by
adding 'detectOutputInjection' to the outputBooleans array so
LlmGuardConfig.output is validated as a boolean, and ensure extractConfig() uses
that same whitelist when normalizing/casting the imported output object.
In `@src/main/security/llm-guard/custom-patterns.ts`:
- Around line 45-57: validatePattern currently only checks syntactic validity
via RegExp and will accept catastrophic/unsafe regexes; update validatePattern
to reject unsafe patterns by integrating a safe-regex check (or using an
RE2-based validator) before returning valid, and apply the same safety gating to
the places mentioned (the regex creation/usage in testPattern and
applyCustomPatterns). Concretely: add a safeRegex check step in validatePattern
(reject on unsafe), or if you prefer RE2, attempt to compile with RE2 and reject
if it indicates unsupported/unsafe constructs; additionally, when executing
patterns in testPattern and applyCustomPatterns run them in a guarded context
(worker or timeout-limited execution) so that even if an unsafe pattern slipped
through it won't hang the main process. Ensure you reference the existing
function names validatePattern, testPattern, and applyCustomPatterns when making
changes so the new validation and guarded execution are applied consistently.
In `@src/main/security/llm-guard/index.ts`:
- Around line 1919-1926: The current logic sanitizes all custom pattern matches
only when effectiveConfig.action === 'sanitize', ignoring each
CustomPattern.action; update the handling in the blocks around
applyCustomPatterns and sanitizeCustomPatternMatches so you split customFindings
by their own action values (e.g., group into warn/block/sanitize) and: push
warn/block findings into findings as before, but only pass the subset with
action === 'sanitize' into sanitizeCustomPatternMatches to update
sanitizedPrompt; apply the same split-and-select-by-CustomPattern.action change
in all three locations that call applyCustomPatterns (the code using
effectiveConfig.customPatterns, the post-scan path, and the inter-agent path) so
per-pattern actions are honored.
- Around line 1788-1813: mergeSecurityPolicy currently ignores
sessionPolicy.groupChat so session-level interAgentScanEnabled (and other
groupChat overrides) are dropped; fix by deep-merging groupChat like
input/output/thresholds: when returning the normalized policy, include a
groupChat property that spreads normalized.groupChat and then overlays
sessionPolicy.groupChat (use sessionPolicy.groupChat || {}), ensuring any nested
flags such as interAgentScanEnabled are applied; update mergeSecurityPolicy to
reference normalized, sessionPolicy, and groupChat so session overrides are
honored.
- Around line 1632-1658: The banTopicsPatterns loop currently constructs and
executes user-supplied regexes (new RegExp(...), regex.exec(text)) which can
cause catastrophic ReDoS; instead validate or sandbox patterns before running
them: use a safe-regex check (or an allowlist) to skip/flag unsafe patterns at
load time, or run each match operation in a time-limited/sandboxed worker/child
process and skip patterns that exceed a short timeout; also avoid unbounded
global matching by removing the global flag usage pattern or limiting iterations
and zero-length match handling around regex.exec in the banTopicsPatterns
processing so findings.push only runs for validated, non-blocking patterns.
In `@src/main/security/llm-guard/url-scanner.ts`:
- Around line 79-82: URL_REGEX fails to match percent-encoded hostnames (e.g.,
paypa%6c.com) because host label character classes only allow [a-zA-Z0-9-];
update URL_REGEX to allow percent-encoding sequences in host labels by replacing
the host-label character class (currently [a-zA-Z0-9-]) with an alternation that
accepts either a normal label char or a percent-encoded byte (e.g.,
(?:%[0-9A-Fa-f]{2}|[A-Za-z0-9-])). Keep the surrounding domain/IPv4 and
port/grouping logic intact so functions referencing URL_REGEX and the
encoded_hostname heuristic continue to operate correctly.
In `@src/main/security/llm-guard/vault.ts`:
- Around line 6-12: add stores the caller-owned LlmGuardVaultEntry objects
directly and toJSON returns the same references, allowing external mutation to
alter vault state; update add(entry: LlmGuardVaultEntry) to push a defensive
copy of the entry (e.g., clone fields or use structured cloning) into
this.entries, and change toJSON(): LlmGuardVaultSnapshot to return a deep-cloned
snapshot (not the original this.entries array or its objects) so callers receive
immutable copies; reference the add and toJSON methods and the
LlmGuardVaultEntry/LlmGuardVaultSnapshot types when making these changes.
In `@src/main/security/security-logger.ts`:
- Around line 115-119: The SecurityEvent being constructed currently spreads in
params (including findings) and writes raw finding.value into the audit log;
change the code that builds the event (where SecurityEvent is created with
...params, id: uuidv4(), timestamp: Date.now()) to sanitize findings before
persisting: replace params.findings with a mapped array that omits or masks the
raw value and instead stores metadata (type, severity, location/context) and a
redactedPreview (e.g., masked string or snippet with sensitive characters
replaced) or a deterministic hash; apply the same redaction logic wherever
findings are serialized/ exported (the other occurrences you noted) so no raw
finding.value is ever written to or exported from the security-events.jsonl
store.
- Around line 131-147: When appendEventToFile or any listener in eventListeners
fails, do not just console.error; capture and report the exception to Sentry
using the project's Sentry utility (e.g. Sentry.captureException or the app's
reportError helper), mark the logger as degraded (set a module-level flag like
loggingDegraded = true or call an existing setLoggingDegraded API) and propagate
the failure to the caller (reject/throw the error) instead of silently resolving
the SecurityEvent; for listener delivery, collect listener errors while still
invoking remaining listeners and, after the loop, report/aggregate and rethrow a
combined error so callers see the persistent/audit failure.
In `@src/renderer/components/LlmGuardIndicator.tsx`:
- Around line 113-118: The recentWarnings counter never increments because
events use event.action for non-blocking outcomes rather than event.eventType;
update the logic in LlmGuardIndicator (where setRecentBlocked and
setRecentWarnings are changed) to treat non-blocking detections by checking
event.action === 'warned' (and optionally 'sanitized' if you want sanitized to
show a warning) and call setRecentWarnings((prev)=>prev+1) for those cases while
leaving event.eventType === 'blocked' to drive setRecentBlocked.
In `@src/renderer/components/RightPanel.tsx`:
- Around line 235-244: The useEffect in RightPanel calls loadSecurityEventIds
and then unconditionally subscribes via window.maestro.security.onSecurityEvent;
guard against a missing preload bridge by checking for window.maestro and
window.maestro.security (or using optional chaining) before calling
onSecurityEvent, only subscribe when present, and return a safe no-op
unsubscribe (e.g., () => {}) if the API is absent; keep loadSecurityEventIds()
behavior unchanged and reference the existing useEffect, loadSecurityEventIds,
onSecurityEvent, and unsubscribe variables when applying the change.
- Around line 225-231: The loadSecurityEventIds function currently swallows all
exceptions when calling window.maestro.security.getEvents; change it to
explicitly handle expected/recoverable cases (e.g., check for a known
"no-permission" or "not-available" error and simply return) and for all other
errors report them (send to Sentry or the app logger) and rethrow or propagate
so they are visible; update the catch in loadSecurityEventIds to reference
window.maestro.security.getEvents, call setSecurityEventIds on success,
log/report unexpected errors with contextual text (e.g., "Failed to load
security events") and then rethrow or allow the error to bubble.
---
Minor comments:
In `@docs/llm-guard.md`:
- Around line 175-215: The markdown tables in the "Dangerous Code Detection"
section (the Shell Commands, SQL Injection, Command Injection, Sensitive File
Access, and Network Operations tables) lack surrounding blank lines which
triggers MD058; add a single blank line before and after each table so each
table block is separated from surrounding paragraphs and headings, ensuring the
tables render cleanly and satisfy the linter.
- Around line 26-35: The fenced ASCII diagram blocks in docs/llm-guard.md are
missing language identifiers (MD040); update each triple-backtick fence for the
diagram examples (the block that starts with the ASCII flowchart like
"┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐") to use a
language tag of text (i.e., change ``` to ```text) for the block shown and apply
the same change to the other diagram blocks indicated (lines referenced in the
review: 238-244, 248-254, 258-264, 268-274, 278-284) so markdownlint MD040 is
satisfied and rendering is consistent.
In `@src/__tests__/main/security/recommendations.test.ts`:
- Around line 571-583: The test computes startsWithActionWord but never asserts
it; update the loop over recommendations/rec.actionItems to assert actionability
by either (a) adding expect(startsWithActionWord).toBeTruthy() for items that
should be actionable, or (b) collect the boolean into an array per rec and
assert at least one true (e.g., ensure each rec has at least one actionable
item), referencing the existing startsWithActionWord variable and the
rec.actionItems iteration to locate where to add the assertion.
In `@src/__tests__/renderer/components/SecurityEventsPanel.test.ts`:
- Around line 104-116: The tests call vi.setSystemTime() but never enable fake
timers; before calling vi.setSystemTime() in the 'sets startDate for 7d date
range' test (and likewise the '30d' test), call vi.useFakeTimers() to enable
fake timers, then set the system time, and keep the existing vi.useRealTimers()
at the end to restore real timers; update references around buildExportFilters,
vi.setSystemTime, vi.useFakeTimers, and vi.useRealTimers accordingly.
In `@src/main/process-manager/handlers/StdoutHandler.ts`:
- Around line 550-563: The promise returned by logSecurityEvent(...) is not
handling rejections; append a .catch(...) to the existing .then(...) chain on
logSecurityEvent to prevent unhandled promise rejections, and inside the catch
call the handler's logging facility (e.g., this.processLogger.error or
this.logger.error, falling back to console.error) to record the error and
context (include event.sessionId/event.tabId and the error). Ensure the catch
does not rethrow so it won't crash the process, but still surfaces the failure
in logs.
In `@src/renderer/components/FindingDetails.tsx`:
- Around line 259-262: The email-masking branch in FindingDetails.tsx assumes a
non-empty local part and uses value.split('@') and local[0], which can throw if
value is "@" or ends with "@"; update the EMAIL handling (the if block that
checks type.includes('EMAIL')) to defensively split using value.split('@', 2),
verify both local and domain are non-empty, and if local is empty or undefined
fall back to a safe mask (e.g., '***@' + domain or '***' if domain missing) or
return a sanitized placeholder; ensure you reference and replace uses of
local[0] with a guarded access so no runtime undefined access occurs.
In `@src/renderer/components/InputArea.tsx`:
- Line 993: The textarea in InputArea.tsx currently sets spellCheck={true},
causing terminal/shell inputs to be underlined; change the prop to be
conditional (e.g., spellCheck={!isTerminalMode}) or otherwise gate it by the
input mode flag used in this component (reference the InputArea component and
the textarea/hidden input element that renders user input and the isTerminalMode
state/prop) so browser spellchecking is disabled when in terminal mode and
enabled for AI/text modes.
In `@src/renderer/components/ScanProgressIndicator.tsx`:
- Around line 51-109: When the subscription scope changes (useEffect deps
enabled, sessionId, tabId) we must reset lingering scan state so the old
indicator doesn't persist: at the start of the effect (or in the cleanup before
calling unsubscribe()) clear any hideTimeoutRef (clearTimeout and null it),
setIsScanning(false), setContentLength(0) and reset scanStartTimeRef.current = 0
(or null). Ensure these resets happen when enabled becomes false or when the
effect re-subscribes so the UI is in a clean state until a new scan_start event
updates it.
---
Nitpick comments:
In `@scripts/refresh-llm-guard-patterns.mjs`:
- Around line 84-86: The current parsing of triple-quoted TOML regex in
scripts/refresh-llm-guard-patterns.mjs uses section.match(...) with the pattern
/^regex\s*=\s*'''([^']+)'''/m which fails when the block contains single quotes;
update the matching logic used around regexMatch to capture triple-quoted
content more robustly (for example use a non-greedy, dot-all aware capture or a
pattern that allows internal single quotes) so rule.regex = regexMatch[1]
receives the full content; adjust the first branch that sets regexMatch (and/or
add a separate branch for triple-single-quoted blocks) to use a safer capture
for section and ensure flags allow newlines.
- Around line 41-61: The httpsGet function lacks a timeout and can hang; update
httpsGet to accept an optional timeout (e.g., default 10000 ms) and apply it to
the outgoing request by storing the returned ClientRequest from https.get,
calling request.setTimeout(timeout, ...) to abort the request and reject with a
timeout Error, and ensure the redirect branch (the recursive call to
httpsGet(res.headers.location)) passes the same timeout; also clear or disable
the timeout on successful response end and wire request.on('error') and
response.on('error') to reject as before.
In `@src/__tests__/main/security/llm-guard/config-export.test.ts`:
- Around line 18-56: Add the missing output.detectOutputInjection flag to the
test fixture and assert it survives the round-trip: update the validConfig
object in src/__tests__/main/security/llm-guard/config-export.test.ts to include
output.detectOutputInjection: true (or the expected boolean) alongside the other
output flags, then update/add assertions in the relevant round-trip
export/import/validation tests (the ones that reference validConfig and the
export/import functions around lines 250-338) to explicitly check that
importedConfig.output.detectOutputInjection equals the original value so the
test fails if the flag is dropped.
In `@src/__tests__/renderer/components/SecurityEventsPanel.test.ts`:
- Around line 28-95: The test duplicates helper logic (buildExportFilters,
generateExportFilename, getMimeType, toggleEventType and related types like
ExportFilterOptions/ExportFormat), which risks desynchronization; replace the
local helper copies by importing the real implementations from
SecurityEventsPanel (or move them to a shared util and import that util in both
the component and this test), update the test imports to reference those
exported functions/types, and remove the duplicated functions so tests exercise
the actual production logic.
- Around line 341-368: The current tests under "confidence slider" assert basic
arithmetic instead of component behavior; replace those unit checks with
integration tests that render the SecurityEventsPanel and simulate slider
interactions: mount/render SecurityEventsPanel, locate the slider input, fire
change events for min (0), max (100), and increments (0,5,...,100), and assert
the component's slider change handler (e.g., onConfidenceChange or
handleConfidenceChange) is invoked with the expected decimal values (value/100)
and that component state/props reflect the decimal (0–1) rather than asserting
raw division; also include edge-case assertions for clamping and step handling.
In `@src/main/preload/process.ts`:
- Around line 54-56: The two boolean flags sendPromptViaStdin and
sendPromptViaStdinRaw create ambiguous/contradictory states; replace them with a
single union field promptDeliveryMode?: 'argv' | 'stdin' | 'stdin-raw' (or, if
you prefer a minimal change, add explicit validation in the IPC input parsing to
reject simultaneous true/contradictory combinations) so the main handler (e.g.,
the function that reads IPC payloads and delivers prompts — locate uses of
sendPromptViaStdin/sendPromptViaStdinRaw in the preload/process.ts handler) no
longer needs to invent precedence; update all consumers to read
promptDeliveryMode and branch on its three values (or throw/return an error on
invalid boolean combos) and add tests for each mode and for invalid
combinations.
In `@src/main/security/llm-guard/code-scanner.ts`:
- Around line 11-12: The scanner currently records inCodeBlock but leaves
scanCode() and containsDangerousCode() defaulting codeBlocksOnly to false, so
plain prose can be flagged; change the default behavior to enforce the
"code-context only" contract by setting codeBlocksOnly default to true in
scanCode() and containsDangerousCode(), update any internal calls to pass that
default, and ensure the matcher continues to check the inCodeBlock flag (e.g.,
the matcher and its inCodeBlock checks in the scanning flow) so patterns are
only flagged when inCodeBlock is true unless a caller explicitly overrides
codeBlocksOnly=false.
In `@src/main/security/llm-guard/patterns-metadata.json`:
- Around line 1-14: The metadata JSON lacks an explicit field documenting how
many patterns were removed during deduplication, causing the totals (64 + 1606 =
1670 vs "totalPatterns": 1652) to be unclear; update
src/main/security/llm-guard/patterns-metadata.json by adding a
"deduplicatedCount" field (e.g., 18) alongside "totalPatterns" and/or a short
"note" field to indicate that totalPatterns is the deduplicated count so readers
and tooling can understand the discrepancy when inspecting the "sources" and
"totalPatterns" values.
In `@src/renderer/components/LlmGuardIndicator.tsx`:
- Around line 23-79: The refactor left the 'disabled' branch unreachable—decide
whether to render a dimmed indicator when enabled is false or remove the unused
disabled code: either (A) update LlmGuardIndicator to render the disabled state
instead of returning null (so LlmGuardStatus 'disabled',
getStatusConfig('disabled'), and the disabled tooltip copy remain used), or (B)
remove the unused elements: the 'disabled' union member in LlmGuardStatus, the
'disabled' case in getStatusConfig, and the disabled tooltip/text used by the
component (and any related constants/imports); pick one approach and apply
consistently in the LlmGuardIndicator component and its tooltip handling.
In `@src/renderer/components/SanitizedContentDiff.tsx`:
- Around line 784-802: In SanitizedContentDiff's mapping over sanitizedSegments
(the map that handles segment.type === 'added'), remove the unused declaration
const colors = getHighlightColor(segment.findingType || '') (or instead apply it
if you intended to use its values); specifically update the added-segment branch
inside the sanitizedSegments.map so there are no unused variables (refer to
getHighlightColor and the added-segment span in the SanitizedContentDiff
component).
- Around line 718-736: In SanitizedContentDiff component, the variable colors
produced by calling getHighlightColor(segment.findingType || '') inside the
originalSegments.map for removed segments is unused; remove that unused
declaration (the call to getHighlightColor stored in colors) from the
removed-branch so you only keep the rendering logic that uses segment and theme,
or if intended, replace the current hardcoded styles with values from colors
returned by getHighlightColor; update the removed-segment branch (the mapping
over originalSegments and the removed-case) accordingly to eliminate the unused
variable.
In `@src/renderer/components/SecurityBadge.tsx`:
- Around line 86-88: The ref lastEventTimeRef declared alongside
dismissTimeoutRef in SecurityBadge is never read; either remove the declaration
lastEventTimeRef and the assignment that writes to it (the unused write near the
event handler), or if you intend to keep it for future use, add a concise
comment above the declaration explaining its purpose (e.g., reserved for
rate-limiting/debugging) so linters reviewers know it’s intentional; update or
remove any imports/usages related to lastEventTimeRef (and keep
dismissTimeoutRef unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8425a2bd-1f32-40cf-96da-e564200f67db
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (92)
docs/docs.jsondocs/features.mddocs/llm-guard.mdpackage.jsonscripts/refresh-llm-guard-patterns.mjssrc/__tests__/main/app-lifecycle/window-manager.test.tssrc/__tests__/main/ipc/handlers/process.test.tssrc/__tests__/main/ipc/handlers/security.test.tssrc/__tests__/main/preload/security.test.tssrc/__tests__/main/process-manager/handlers/ExitHandler.test.tssrc/__tests__/main/process-manager/handlers/StdoutHandler.test.tssrc/__tests__/main/security/llm-guard.test.tssrc/__tests__/main/security/llm-guard/config-export.test.tssrc/__tests__/main/security/llm-guard/recommendations.test.tssrc/__tests__/main/security/recommendations.test.tssrc/__tests__/main/security/security-logger.test.tssrc/__tests__/renderer/components/FindingDetails.test.tssrc/__tests__/renderer/components/LlmGuardIndicator.test.tssrc/__tests__/renderer/components/RightPanel.test.tsxsrc/__tests__/renderer/components/SanitizedContentDiff.test.tssrc/__tests__/renderer/components/SanitizedContentDiff.test.tsxsrc/__tests__/renderer/components/ScanProgressIndicator.test.tssrc/__tests__/renderer/components/SecurityBadge.test.tssrc/__tests__/renderer/components/SecurityEventsPanel.test.tssrc/__tests__/renderer/hooks/useSecurityToasts.test.tssrc/__tests__/setup.tssrc/main/app-lifecycle/window-manager.tssrc/main/group-chat/group-chat-router.tssrc/main/index.tssrc/main/ipc/handlers/index.tssrc/main/ipc/handlers/process.tssrc/main/ipc/handlers/security.tssrc/main/preload/index.tssrc/main/preload/process.tssrc/main/preload/security.tssrc/main/process-listeners/forwarding-listeners.tssrc/main/process-manager/handlers/ExitHandler.tssrc/main/process-manager/handlers/StderrHandler.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/main/process-manager/index.tssrc/main/process-manager/spawners/ChildProcessSpawner.tssrc/main/process-manager/types.tssrc/main/security/llm-guard/code-scanner.tssrc/main/security/llm-guard/config-export.tssrc/main/security/llm-guard/custom-patterns.tssrc/main/security/llm-guard/generated-patterns.tssrc/main/security/llm-guard/index.tssrc/main/security/llm-guard/patterns-metadata.jsonsrc/main/security/llm-guard/recommendations.tssrc/main/security/llm-guard/types.tssrc/main/security/llm-guard/url-scanner.tssrc/main/security/llm-guard/vault.tssrc/main/security/security-logger.tssrc/renderer/App.tsxsrc/renderer/components/FindingDetails.tsxsrc/renderer/components/InputArea.tsxsrc/renderer/components/LlmGuardIndicator.tsxsrc/renderer/components/MainPanel.tsxsrc/renderer/components/QuickActionsModal.tsxsrc/renderer/components/RightPanel.tsxsrc/renderer/components/SanitizedContentDiff.tsxsrc/renderer/components/ScanProgressIndicator.tsxsrc/renderer/components/SecurityBadge.tsxsrc/renderer/components/SecurityEventsPanel.tsxsrc/renderer/components/SecurityRecommendationsPanel.tsxsrc/renderer/components/SensitiveContentOverlay.tsxsrc/renderer/components/SensitiveContentPill.tsxsrc/renderer/components/SessionItem.tsxsrc/renderer/components/SessionList/SessionContextMenu.tsxsrc/renderer/components/SessionList/SessionList.tsxsrc/renderer/components/SessionSecurityModal.tsxsrc/renderer/components/Settings/SettingsModal.tsxsrc/renderer/components/Settings/tabs/LlmGuardTab.tsxsrc/renderer/components/Settings/tabs/index.tssrc/renderer/components/ToggleButtonGroup.tsxsrc/renderer/constants/modalPriorities.tssrc/renderer/global.d.tssrc/renderer/hooks/agent/useMergeTransferHandlers.tssrc/renderer/hooks/batch/useAutoRunHandlers.tssrc/renderer/hooks/index.tssrc/renderer/hooks/input/index.tssrc/renderer/hooks/input/useInputProcessing.tssrc/renderer/hooks/input/useSensitiveContentDetection.tssrc/renderer/hooks/remote/useRemoteHandlers.tssrc/renderer/hooks/security/index.tssrc/renderer/hooks/security/useSecurityToasts.tssrc/renderer/hooks/settings/useSettings.tssrc/renderer/stores/agentStore.tssrc/renderer/stores/modalStore.tssrc/renderer/stores/settingsStore.tssrc/renderer/types/index.tssrc/renderer/utils/sessionHelpers.ts
Comment on lines
+16
to
+249
| function sortFindingsByPosition(findings: Finding[]): Finding[] { | ||
| return [...findings].sort((a, b) => a.start - b.start); | ||
| } | ||
|
|
||
| // Get highlight color for finding type (mirrors component logic) | ||
| function getHighlightColor(type: string): { bg: string; text: string } { | ||
| // High severity: injection, jailbreak | ||
| if (type.includes('INJECTION') || type.includes('JAILBREAK')) { | ||
| return { bg: 'rgba(239, 68, 68, 0.3)', text: '#ef4444' }; | ||
| } | ||
| // Secrets | ||
| if ( | ||
| type.startsWith('SECRET_') || | ||
| type.includes('PASSWORD') || | ||
| type.includes('TOKEN') || | ||
| type.includes('KEY') | ||
| ) { | ||
| return { bg: 'rgba(245, 158, 11, 0.3)', text: '#f59e0b' }; | ||
| } | ||
| // PII | ||
| if (type.startsWith('PII_')) { | ||
| return { bg: 'rgba(139, 92, 246, 0.3)', text: '#8b5cf6' }; | ||
| } | ||
| // Invisible characters | ||
| if (type.startsWith('INVISIBLE_')) { | ||
| return { bg: 'rgba(236, 72, 153, 0.3)', text: '#ec4899' }; | ||
| } | ||
| // Default | ||
| return { bg: 'rgba(59, 130, 246, 0.3)', text: '#3b82f6' }; | ||
| } | ||
|
|
||
| // Segment type for rendering | ||
| interface TextSegment { | ||
| type: 'normal' | 'removed' | 'added'; | ||
| text: string; | ||
| findingType?: string; | ||
| replacement?: string; | ||
| } | ||
|
|
||
| // Build segments from original content and findings (mirrors component logic) | ||
| function buildOriginalSegments(content: string, findings: Finding[]): TextSegment[] { | ||
| const sortedFindings = sortFindingsByPosition(findings); | ||
| const segments: TextSegment[] = []; | ||
| let currentPos = 0; | ||
|
|
||
| for (const finding of sortedFindings) { | ||
| // Only process findings with replacements (i.e., actually sanitized) | ||
| if (!finding.replacement) continue; | ||
|
|
||
| // Add normal text before this finding | ||
| if (finding.start > currentPos) { | ||
| segments.push({ | ||
| type: 'normal', | ||
| text: content.slice(currentPos, finding.start), | ||
| }); | ||
| } | ||
|
|
||
| // Add the removed (original) text | ||
| if (finding.start >= 0 && finding.end <= content.length) { | ||
| segments.push({ | ||
| type: 'removed', | ||
| text: content.slice(finding.start, finding.end), | ||
| findingType: finding.type, | ||
| replacement: finding.replacement, | ||
| }); | ||
| } | ||
|
|
||
| currentPos = finding.end; | ||
| } | ||
|
|
||
| // Add remaining normal text | ||
| if (currentPos < content.length) { | ||
| segments.push({ | ||
| type: 'normal', | ||
| text: content.slice(currentPos), | ||
| }); | ||
| } | ||
|
|
||
| return segments; | ||
| } | ||
|
|
||
| // Build segments from sanitized content and findings (mirrors component logic) | ||
| function buildSanitizedSegments(content: string, findings: Finding[]): TextSegment[] { | ||
| const sortedFindings = sortFindingsByPosition(findings).filter((f) => f.replacement); | ||
|
|
||
| if (sortedFindings.length === 0) { | ||
| return [{ type: 'normal', text: content }]; | ||
| } | ||
|
|
||
| const segments: TextSegment[] = []; | ||
| let currentPos = 0; | ||
| let offset = 0; | ||
|
|
||
| for (const finding of sortedFindings) { | ||
| if (!finding.replacement) continue; | ||
|
|
||
| const originalLength = finding.end - finding.start; | ||
| const replacementLength = finding.replacement.length; | ||
| const adjustedStart = finding.start + offset; | ||
| const adjustedEnd = adjustedStart + replacementLength; | ||
|
|
||
| // Add normal text before this replacement | ||
| if (adjustedStart > currentPos) { | ||
| segments.push({ | ||
| type: 'normal', | ||
| text: content.slice(currentPos, adjustedStart), | ||
| }); | ||
| } | ||
|
|
||
| // Add the replacement text (marked as added) | ||
| if (adjustedStart >= 0 && adjustedEnd <= content.length) { | ||
| segments.push({ | ||
| type: 'added', | ||
| text: content.slice(adjustedStart, adjustedEnd), | ||
| findingType: finding.type, | ||
| }); | ||
| } | ||
|
|
||
| currentPos = adjustedEnd; | ||
| offset += replacementLength - originalLength; | ||
| } | ||
|
|
||
| // Add remaining normal text | ||
| if (currentPos < content.length) { | ||
| segments.push({ | ||
| type: 'normal', | ||
| text: content.slice(currentPos), | ||
| }); | ||
| } | ||
|
|
||
| return segments; | ||
| } | ||
|
|
||
| // Inline diff segment (mirrors component logic) | ||
| interface InlineSegment { | ||
| type: 'normal' | 'removed' | 'added'; | ||
| text: string; | ||
| findingType?: string; | ||
| } | ||
|
|
||
| // Build inline diff segments (mirrors component logic) | ||
| function buildInlineSegments( | ||
| originalContent: string, | ||
| sanitizedContent: string, | ||
| findings: Finding[] | ||
| ): InlineSegment[] { | ||
| const sortedFindings = sortFindingsByPosition(findings).filter((f) => f.replacement); | ||
|
|
||
| if (sortedFindings.length === 0) { | ||
| return [{ type: 'normal', text: originalContent }]; | ||
| } | ||
|
|
||
| const segments: InlineSegment[] = []; | ||
| let currentPos = 0; | ||
|
|
||
| for (const finding of sortedFindings) { | ||
| if (!finding.replacement) continue; | ||
|
|
||
| // Add normal text before this finding | ||
| if (finding.start > currentPos) { | ||
| segments.push({ | ||
| type: 'normal', | ||
| text: originalContent.slice(currentPos, finding.start), | ||
| }); | ||
| } | ||
|
|
||
| // Add the removed text (strikethrough) | ||
| segments.push({ | ||
| type: 'removed', | ||
| text: originalContent.slice(finding.start, finding.end), | ||
| findingType: finding.type, | ||
| }); | ||
|
|
||
| // Add the replacement text | ||
| segments.push({ | ||
| type: 'added', | ||
| text: finding.replacement, | ||
| findingType: finding.type, | ||
| }); | ||
|
|
||
| currentPos = finding.end; | ||
| } | ||
|
|
||
| // Add remaining normal text | ||
| if (currentPos < originalContent.length) { | ||
| segments.push({ | ||
| type: 'normal', | ||
| text: originalContent.slice(currentPos), | ||
| }); | ||
| } | ||
|
|
||
| return segments; | ||
| } | ||
|
|
||
| // Reconstruct content from findings (mirrors SecurityEventsPanel helper) | ||
| function reconstructContentFromFindings( | ||
| findings: Finding[], | ||
| totalLength: number | ||
| ): { original: string; sanitized: string } { | ||
| if (findings.length === 0) { | ||
| return { original: '', sanitized: '' }; | ||
| } | ||
|
|
||
| const sorted = [...findings].sort((a, b) => a.start - b.start); | ||
| const parts: string[] = []; | ||
| const sanitizedParts: string[] = []; | ||
| let lastEnd = 0; | ||
|
|
||
| for (const finding of sorted) { | ||
| if (finding.start > lastEnd) { | ||
| const gapSize = Math.min(finding.start - lastEnd, 20); | ||
| const gap = gapSize > 10 ? '... ' : ''; | ||
| parts.push(gap); | ||
| sanitizedParts.push(gap); | ||
| } | ||
|
|
||
| parts.push(finding.value); | ||
| sanitizedParts.push(finding.replacement || finding.value); | ||
| lastEnd = finding.end; | ||
| } | ||
|
|
||
| if (lastEnd < totalLength) { | ||
| const remaining = totalLength - lastEnd; | ||
| if (remaining > 10) { | ||
| parts.push(' ...'); | ||
| sanitizedParts.push(' ...'); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| original: parts.join(''), | ||
| sanitized: sanitizedParts.join(''), | ||
| }; | ||
| } |
There was a problem hiding this comment.
These tests mirror the implementation instead of exercising it.
The file never imports SanitizedContentDiff; it reimplements the sorting/segment/reconstruction helpers locally and asserts against those copies. If the production component diverges, this suite can stay green while the UI is broken. Please render the real component or export the production helpers and test those directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/renderer/components/SanitizedContentDiff.test.ts` around lines
16 - 249, The tests reimplement helpers (sortFindingsByPosition,
buildOriginalSegments, buildSanitizedSegments, buildInlineSegments,
reconstructContentFromFindings) instead of exercising the real logic; update the
test to import and use the real SanitizedContentDiff component or the production
helper exports instead of local copies: remove the duplicated helper
implementations from SanitizedContentDiff.test.ts, add an import for
SanitizedContentDiff (or export the helper functions from the production module
and import those), then render the component with test data (or call the
exported helpers) and assert against the real outputs so the suite validates the
actual implementation rather than a mirrored copy.
Comment on lines
+23
to
+24
| let securityEventCallback: ((event: SecurityEventData) => void) | null = null; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking TypeScript strict null settings:"
rg -n --glob 'tsconfig*.json' '"strict"\s*:\s*true|"strictNullChecks"\s*:\s*true'
echo
echo "Finding nullable callback invocations in this test file:"
rg -n -C1 'securityEventCallback\(' src/__tests__/renderer/hooks/useSecurityToasts.test.tsRepository: RunMaestro/Maestro
Length of output: 1408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the test file to see the setup and actual usage pattern
echo "=== Lines around the callback declaration (lines 20-35) ==="
sed -n '20,35p' src/__tests__/renderer/hooks/useSecurityToasts.test.ts
echo
echo "=== Sample call sites to verify the pattern (lines 118-125) ==="
sed -n '118,125p' src/__tests__/renderer/hooks/useSecurityToasts.test.ts
echo
echo "=== Check for any beforeEach setup that might assign the callback ==="
rg -n 'beforeEach|securityEventCallback\s*=' src/__tests__/renderer/hooks/useSecurityToasts.test.ts | head -20
echo
echo "=== Check the actual type definition of SecurityEventData ==="
rg -rn 'type SecurityEventData|interface SecurityEventData' --glob '**/*.{ts,tsx}'Repository: RunMaestro/Maestro
Length of output: 1359
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if the test file has any `@ts-ignore` or `@ts-expect-error` comments
echo "=== Checking for TypeScript suppression comments in the test file ==="
rg -n '@ts-ignore|@ts-expect-error|@ts-nocheck' src/__tests__/renderer/hooks/useSecurityToasts.test.ts
echo
echo "=== Check if this test file is in the tsconfig include/exclude ==="
cat tsconfig.json | grep -A 10 -B 5 '"include"\|"exclude"'
echo
echo "=== Verify the full callback assignment pattern in beforeEach ==="
sed -n '26,40p' src/__tests__/renderer/hooks/useSecurityToasts.test.tsRepository: RunMaestro/Maestro
Length of output: 131
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read tsconfig.json to check includes/excludes
echo "=== Full tsconfig.json ==="
cat tsconfig.json
echo
echo "=== Full beforeEach and afterEach setup (lines 26-72) ==="
sed -n '26,72p' src/__tests__/renderer/hooks/useSecurityToasts.test.tsRepository: RunMaestro/Maestro
Length of output: 1703
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are test-specific tsconfigs
echo "=== Looking for test-specific tsconfig files ==="
find . -maxdepth 2 -name '*tsconfig*.json' -type f
echo
echo "=== Check vitest configuration (if any) ==="
rg -n 'vitest|test' vite.config.ts vite.config.js vitest.config.ts vitest.config.js 2>/dev/null | head -20
echo
echo "=== Verify by checking one call site more carefully (line 119-125) ==="
sed -n '119,125p' src/__tests__/renderer/hooks/useSecurityToasts.test.tsRepository: RunMaestro/Maestro
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's a test configuration or if tests are run without type checking
echo "=== Check vite.config file for vitest/test setup ==="
find . -maxdepth 2 -name 'vite.config.*' -exec cat {} \; 2>/dev/null
echo
echo "=== Check package.json for test script and type-check command ==="
rg -n '"test"|"typecheck"|"type-check"' package.json | head -20
echo
echo "=== Verify the actual pattern at one call site with more context (lines 115-130) ==="
sed -n '115,130p' src/__tests__/renderer/hooks/useSecurityToasts.test.tsRepository: RunMaestro/Maestro
Length of output: 10512
Add a helper function to safely invoke the security event callback.
securityEventCallback is nullable but invoked directly at 16+ call sites without null narrowing. While the callback is guaranteed to be assigned in beforeEach before each test, the pattern is unsafe and violates strict null-safety principles. Use a small helper that asserts callback presence before invocation.
Suggested fix
let securityEventCallback: ((event: SecurityEventData) => void) | null = null;
+
+function emitSecurityEvent(event: SecurityEventData): void {
+ expect(securityEventCallback).toBeTruthy();
+ securityEventCallback!(event);
+}
...
- act(() => {
- securityEventCallback(event);
- });
+ act(() => {
+ emitSecurityEvent(event);
+ });Applies to: 121, 148, 173, 198, 223, 248, 282, 302, 322, 344, 368, 392, 416, 443, 460, 487
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/renderer/hooks/useSecurityToasts.test.ts` around lines 23 - 24,
securityEventCallback is nullable but called directly in many test locations;
add a small helper function (e.g., invokeSecurityEvent(event:
SecurityEventData)) that asserts securityEventCallback is non-null (throw or use
expect to fail) and then calls it, update beforeEach to still assign
securityEventCallback, and replace every direct invocation of
securityEventCallback(...) in the test file (references: securityEventCallback,
beforeEach) with invokeSecurityEvent(...) so calls are null-safe and fail
clearly if not initialized.
Comment on lines
+65
to
+86
| logSecurityEvent({ | ||
| sessionId, | ||
| tabId: managedProcess.tabId, | ||
| eventType: guardResult.blocked ? 'blocked' : 'output_scan', | ||
| findings: guardResult.findings, | ||
| action, | ||
| originalLength: resultText.length, | ||
| sanitizedLength: guardResult.sanitizedResponse.length, | ||
| }).then((event) => { | ||
| // Emit to ProcessManager listeners for real-time UI forwarding | ||
| const eventData: SecurityEventData = { | ||
| sessionId: event.sessionId, | ||
| tabId: event.tabId, | ||
| eventType: event.eventType, | ||
| findingTypes: event.findings.map((f) => f.type), | ||
| findingCount: event.findings.length, | ||
| action: event.action, | ||
| originalLength: event.originalLength, | ||
| sanitizedLength: event.sanitizedLength, | ||
| }; | ||
| this.emitter.emit('security-event', eventData); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the ExitHandler.ts file and surrounding context
cd /repo && cat -n src/main/process-manager/handlers/ExitHandler.ts | head -100Repository: RunMaestro/Maestro
Length of output: 117
🏁 Script executed:
# Find logSecurityEvent definition
rg -n "logSecurityEvent" --type ts -A 5 -B 2Repository: RunMaestro/Maestro
Length of output: 50374
🏁 Script executed:
# Check imports in ExitHandler.ts
cd /repo && head -50 src/main/process-manager/handlers/ExitHandler.tsRepository: RunMaestro/Maestro
Length of output: 117
🏁 Script executed:
# Search for captureException usage patterns in process-manager
rg -n "captureException" src/main/process-manager --type ts -B 2 -A 2Repository: RunMaestro/Maestro
Length of output: 1038
🏁 Script executed:
# Check if sentry.ts is imported in ExitHandler.ts
rg -n "from.*sentry" src/main/process-manager/handlers/ExitHandler.tsRepository: RunMaestro/Maestro
Length of output: 44
🏁 Script executed:
# Get the exact code section from ExitHandler.ts around lines 65-86
sed -n '1,150p' src/main/process-manager/handlers/ExitHandler.ts | tail -100Repository: RunMaestro/Maestro
Length of output: 3661
🏁 Script executed:
# Check how other files handle logSecurityEvent - specifically look at StdoutHandler for comparison
sed -n '535,560p' src/main/process-manager/handlers/StdoutHandler.tsRepository: RunMaestro/Maestro
Length of output: 963
🏁 Script executed:
# Look at process.ts to see the void logSecurityEvent pattern
sed -n '235,260p' src/main/ipc/handlers/process.tsRepository: RunMaestro/Maestro
Length of output: 973
🏁 Script executed:
# Check ExitHandler.ts imports to verify if captureException is imported
head -50 src/main/process-manager/handlers/ExitHandler.tsRepository: RunMaestro/Maestro
Length of output: 1827
🏁 Script executed:
# Look for the exact line numbers of the code in ExitHandler.ts to confirm the snippet
rg -n "logSecurityEvent" src/main/process-manager/handlers/ExitHandler.ts -A 25 -B 2Repository: RunMaestro/Maestro
Length of output: 2204
🏁 Script executed:
# Check if StdoutHandler.ts has the same issue with logSecurityEvent
grep -n "captureException" src/main/process-manager/handlers/StdoutHandler.tsRepository: RunMaestro/Maestro
Length of output: 44
🏁 Script executed:
# Check the full logSecurityEvent call in StdoutHandler for comparison
sed -n '535,570p' src/main/process-manager/handlers/StdoutHandler.tsRepository: RunMaestro/Maestro
Length of output: 1218
🏁 Script executed:
# Verify the captureException import path in src/main/utils/sentry.ts
head -20 src/main/utils/sentry.tsRepository: RunMaestro/Maestro
Length of output: 620
🏁 Script executed:
# Check if StdoutHandler has similar promise handling issues
grep -n "\.then\|\.catch" src/main/process-manager/handlers/StdoutHandler.ts | head -20Repository: RunMaestro/Maestro
Length of output: 89
🏁 Script executed:
# Check if captureException is actually exported from sentry.ts
grep -n "export.*captureException" src/main/utils/sentry.tsRepository: RunMaestro/Maestro
Length of output: 104
🏁 Script executed:
# Check if StdoutHandler imports captureException
head -50 src/main/process-manager/handlers/StdoutHandler.ts | grep -n "import"Repository: RunMaestro/Maestro
Length of output: 749
🏁 Script executed:
# Verify the exact current state - does ExitHandler have .catch on logSecurityEvent?
sed -n '65,87p' src/main/process-manager/handlers/ExitHandler.tsRepository: RunMaestro/Maestro
Length of output: 848
Add rejection handling to the security event promise chain.
The logSecurityEvent(...).then(...) call has no .catch() handler. If persistence fails, the rejection goes unhandled in the main process and the renderer never receives the live security event emission. Add .catch(captureException) with Sentry context.
🔧 Suggested fix
- logSecurityEvent({
+ void logSecurityEvent({
sessionId,
tabId: managedProcess.tabId,
eventType: guardResult.blocked ? 'blocked' : 'output_scan',
findings: guardResult.findings,
action,
originalLength: resultText.length,
sanitizedLength: guardResult.sanitizedResponse.length,
- }).then((event) => {
- // Emit to ProcessManager listeners for real-time UI forwarding
- const eventData: SecurityEventData = {
- sessionId: event.sessionId,
- tabId: event.tabId,
- eventType: event.eventType,
- findingTypes: event.findings.map((f) => f.type),
- findingCount: event.findings.length,
- action: event.action,
- originalLength: event.originalLength,
- sanitizedLength: event.sanitizedLength,
- };
- this.emitter.emit('security-event', eventData);
- });
+ })
+ .then((event) => {
+ // Emit to ProcessManager listeners for real-time UI forwarding
+ const eventData: SecurityEventData = {
+ sessionId: event.sessionId,
+ tabId: event.tabId,
+ eventType: event.eventType,
+ findingTypes: event.findings.map((f) => f.type),
+ findingCount: event.findings.length,
+ action: event.action,
+ originalLength: event.originalLength,
+ sanitizedLength: event.sanitizedLength,
+ };
+ this.emitter.emit('security-event', eventData);
+ })
+ .catch((error) => {
+ captureException(error, {
+ operation: 'llm-guard-log-security-event',
+ sessionId,
+ toolType: managedProcess.toolType,
+ });
+ });import { captureException } from '../../utils/sentry';📝 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
| logSecurityEvent({ | |
| sessionId, | |
| tabId: managedProcess.tabId, | |
| eventType: guardResult.blocked ? 'blocked' : 'output_scan', | |
| findings: guardResult.findings, | |
| action, | |
| originalLength: resultText.length, | |
| sanitizedLength: guardResult.sanitizedResponse.length, | |
| }).then((event) => { | |
| // Emit to ProcessManager listeners for real-time UI forwarding | |
| const eventData: SecurityEventData = { | |
| sessionId: event.sessionId, | |
| tabId: event.tabId, | |
| eventType: event.eventType, | |
| findingTypes: event.findings.map((f) => f.type), | |
| findingCount: event.findings.length, | |
| action: event.action, | |
| originalLength: event.originalLength, | |
| sanitizedLength: event.sanitizedLength, | |
| }; | |
| this.emitter.emit('security-event', eventData); | |
| }); | |
| void logSecurityEvent({ | |
| sessionId, | |
| tabId: managedProcess.tabId, | |
| eventType: guardResult.blocked ? 'blocked' : 'output_scan', | |
| findings: guardResult.findings, | |
| action, | |
| originalLength: resultText.length, | |
| sanitizedLength: guardResult.sanitizedResponse.length, | |
| }) | |
| .then((event) => { | |
| // Emit to ProcessManager listeners for real-time UI forwarding | |
| const eventData: SecurityEventData = { | |
| sessionId: event.sessionId, | |
| tabId: event.tabId, | |
| eventType: event.eventType, | |
| findingTypes: event.findings.map((f) => f.type), | |
| findingCount: event.findings.length, | |
| action: event.action, | |
| originalLength: event.originalLength, | |
| sanitizedLength: event.sanitizedLength, | |
| }; | |
| this.emitter.emit('security-event', eventData); | |
| }) | |
| .catch((error) => { | |
| captureException(error, { | |
| operation: 'llm-guard-log-security-event', | |
| sessionId, | |
| toolType: managedProcess.toolType, | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/process-manager/handlers/ExitHandler.ts` around lines 65 - 86, The
promise returned by logSecurityEvent(...) in ExitHandler.ts lacks rejection
handling; update the chain on the call site (the logSecurityEvent(...).then(...)
block that emits via this.emitter.emit('security-event', ...)) to append
.catch((err) => captureException(err, { extra: { sessionId, tabId:
managedProcess.tabId, eventType: guardResult.blocked ? 'blocked' : 'output_scan'
}})); also add the import for captureException from the Sentry utility (import {
captureException } from '../../utils/sentry') so any persistence errors are
captured with context.
P0 Security Fix: - Redact sensitive values (PII, secrets) before persisting to disk - Only store redacted findings in security-events.jsonl and exports - Keep full values in memory for UI display - Report audit write failures to Sentry P1 Security Fixes: - Fix double-scanning of streaming chunks by storing guarded text - Add IPC config sanitization for security handlers - Guard window.maestro.security before subscribing in RightPanel Other Security Improvements: - Prevent vault state mutation via shared object references - Fix LlmGuardIndicator to track 'sanitized'/'warned' actions - Add vi.useFakeTimers() before vi.setSystemTime() in tests Test Fixes: - Update tests to match implementation (SECRET_STRIPE_KEY, SECRET_SLACK_TOKEN) - Skip terminal mode stripping test (feature not implemented) - Use token building pattern to avoid secret scanner triggers Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/renderer/components/LlmGuardIndicator.tsx (1)
125-127: Minor: Scanning animation timeout not cleaned up on unmount.The
setTimeoutat line 127 isn't tracked for cleanup. If the component unmounts within 500ms of an event, React will log a "Can't perform a React state update on an unmounted component" warning.💡 Optional fix
+const scanTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); ... // Show scanning animation briefly setIsScanning(true); -setTimeout(() => setIsScanning(false), 500); +if (scanTimeoutRef.current) clearTimeout(scanTimeoutRef.current); +scanTimeoutRef.current = setTimeout(() => setIsScanning(false), 500); ... return () => { unsubscribe(); + if (scanTimeoutRef.current) clearTimeout(scanTimeoutRef.current); if (clearTimeoutRef.current) clearTimeout(clearTimeoutRef.current); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/LlmGuardIndicator.tsx` around lines 125 - 127, The setTimeout used to toggle setIsScanning in the LlmGuardIndicator component can run after unmount and cause a React state update warning; capture the timeout ID (e.g., store in a useRef or local variable) when calling setTimeout inside the handler that triggers the scanning animation and clear it on unmount (or in a useEffect cleanup) using clearTimeout(timeoutId); ensure any existing timeout is cleared before starting a new one so setIsScanning is never called after the component is unmounted.src/__tests__/main/process-manager/handlers/StdoutHandler.test.ts (1)
135-147: Skipped test should reference a tracking issue.The
it.skipwithTODO: Terminal mode sequence stripping feature not implemented yetleaves an orphaned reminder. Consider creating an issue and referencing it, or removing the skipped test if the feature is intentionally deferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/process-manager/handlers/StdoutHandler.test.ts` around lines 135 - 147, Create a tracking issue for the terminal mode sequence stripping feature and update the skipped test (the "should strip leaked terminal mode sequences in plain text mode" test using createTestContext and handler.handleData in StdoutHandler.test.ts) to reference that issue; specifically, replace the orphaned TODO by either adding the issue ID to the it.skip description or a brief inline comment pointing to the issue, or remove the skipped test if you intend to defer and not keep the reminder in tests. Ensure the reference clearly includes the issue identifier so future authors can find the task.src/main/process-manager/handlers/StdoutHandler.ts (1)
335-339: Chunk-level guarding creates a potential security gap for patterns spanning chunk boundaries.The code intentionally skips re-scanning the final accumulated
streamedText(lines 406–411) to avoid duplicate scans. However, this means patterns that span chunk boundaries—likeuser@examsplit across two streaming chunks—won't be caught by either the chunk-level scan (event.textat line 339) or the final result path (which trusts pre-guardedstreamedText).For a security-critical feature like LLM Guard, consider whether a full re-scan of the accumulated text at the final result path would be worth the slight performance cost, or explicitly document that cross-chunk detection gaps are accepted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/handlers/StdoutHandler.ts` around lines 335 - 339, The current chunk-level guarding in StdoutHandler (use of applyOutputGuard on event.text and assigning managedProcess.streamedText) can miss patterns spanning chunk boundaries; update the final emit path to re-run the guard over the accumulated managedProcess.streamedText before trusting/returning it (i.e., call applyOutputGuard(sessionId, managedProcess, managedProcess.streamedText) and use that result for the final emit instead of the previously stored guardedChunk), or if keeping the optimization, add explicit documented justification and a unit/integration test showing cross-chunk patterns are acceptable; adjust emitter.emit usage accordingly to emit the re-guarded final string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/security/security-logger.ts`:
- Around line 201-205: appendEventToFile currently calls fs.appendFile directly
which can interleave when logSecurityEvent is invoked concurrently (from
StdoutHandler.applyOutputGuard). Protect writes by serializing them: implement a
single shared async queue or mutex used by appendEventToFile (or a new helper
used by logSecurityEvent) so each JSONL line is written one-at-a-time to the
path returned by getSecurityEventsPath(); ensure the queue/mutex is
module-scoped (shared across calls), await the lock before calling
fs.appendFile, release it after write, and propagate/log errors so failures
don’t silently drop events.
---
Nitpick comments:
In `@src/__tests__/main/process-manager/handlers/StdoutHandler.test.ts`:
- Around line 135-147: Create a tracking issue for the terminal mode sequence
stripping feature and update the skipped test (the "should strip leaked terminal
mode sequences in plain text mode" test using createTestContext and
handler.handleData in StdoutHandler.test.ts) to reference that issue;
specifically, replace the orphaned TODO by either adding the issue ID to the
it.skip description or a brief inline comment pointing to the issue, or remove
the skipped test if you intend to defer and not keep the reminder in tests.
Ensure the reference clearly includes the issue identifier so future authors can
find the task.
In `@src/main/process-manager/handlers/StdoutHandler.ts`:
- Around line 335-339: The current chunk-level guarding in StdoutHandler (use of
applyOutputGuard on event.text and assigning managedProcess.streamedText) can
miss patterns spanning chunk boundaries; update the final emit path to re-run
the guard over the accumulated managedProcess.streamedText before
trusting/returning it (i.e., call applyOutputGuard(sessionId, managedProcess,
managedProcess.streamedText) and use that result for the final emit instead of
the previously stored guardedChunk), or if keeping the optimization, add
explicit documented justification and a unit/integration test showing
cross-chunk patterns are acceptable; adjust emitter.emit usage accordingly to
emit the re-guarded final string.
In `@src/renderer/components/LlmGuardIndicator.tsx`:
- Around line 125-127: The setTimeout used to toggle setIsScanning in the
LlmGuardIndicator component can run after unmount and cause a React state update
warning; capture the timeout ID (e.g., store in a useRef or local variable) when
calling setTimeout inside the handler that triggers the scanning animation and
clear it on unmount (or in a useEffect cleanup) using clearTimeout(timeoutId);
ensure any existing timeout is cleared before starting a new one so
setIsScanning is never called after the component is unmounted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 37aefe89-174a-4d0e-b907-89677e0f3839
📒 Files selected for processing (11)
src/__tests__/main/ipc/handlers/process.test.tssrc/__tests__/main/process-manager/handlers/StdoutHandler.test.tssrc/__tests__/main/security/llm-guard.test.tssrc/__tests__/main/security/security-logger.test.tssrc/__tests__/renderer/components/SecurityEventsPanel.test.tssrc/main/ipc/handlers/security.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/main/security/llm-guard/vault.tssrc/main/security/security-logger.tssrc/renderer/components/LlmGuardIndicator.tsxsrc/renderer/components/RightPanel.tsx
✅ Files skipped from review due to trivial changes (1)
- src/tests/main/security/security-logger.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/security/llm-guard/vault.ts
- src/tests/main/ipc/handlers/process.test.ts
- StderrHandler: Log security events + emit to live stream (matching StdoutHandler) - config-export: Add detectOutputInjection to output boolean validation - config-export: Treat invalid banTopicsPatterns regex as errors (not warnings) - url-scanner: Update URL_REGEX to detect percent-encoded hostnames - group-chat-router: Hoist getLlmGuardSettingsCallback outside participant loop - RightPanel: Report security event loading failures to Sentry - llm-guard: Fix mergeSecurityPolicy to include groupChat overrides Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add a write queue to security-logger.ts to serialize concurrent appendFile calls. This prevents JSONL line interleaving when multiple security events are logged simultaneously from different handlers. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/security/llm-guard/config-export.ts (1)
36-47: Add ReDoS detection for user-supplied regex patterns during import validation.This function accepts user patterns from imported configs that are later executed at runtime in
applyCustomPatterns. While the code includes guards against zero-length match infinite loops, it lacks protection against catastrophic backtracking patterns that can cause CPU exhaustion.Consider using the
redos-detectorlibrary (v6.1.4+) to validate patterns at import time:import { isSafe } from 'redos-detector'; const result = isSafe(new RegExp(pattern, 'gi')); if (!result.safe) { return { valid: false, error: `Pattern is vulnerable to ReDoS: ${result.score}` }; }Alternatively, integrate the ESLint plugin
eslint-plugin-redos-detectorfor linting, or add a brief comment documenting that patterns are expected to come only from trusted admin imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/security/llm-guard/config-export.ts` around lines 36 - 47, Update validateRegex to detect ReDoS-vulnerable patterns by using the redos-detector isSafe check after constructing the RegExp; inside validateRegex (and before returning valid:true) call isSafe(new RegExp(pattern, 'gi')) and if result.safe is false return { valid: false, error: `Pattern is vulnerable to ReDoS: ${result.score}` }. Keep existing empty-pattern and RegExp-construction guards, and ensure this validation protects patterns that will later be executed in applyCustomPatterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/process-manager/handlers/StderrHandler.ts`:
- Around line 49-54: The current StderrHandler only logs LLMGuard events when
guardResult.findings.length > 0 which can miss cases where guardResult.blocked
or guardResult.warned is true but findings is empty; update the conditional that
surrounds the logger.warn call (and any event emission) to mirror StdoutHandler
by checking (guardResult.findings.length > 0 || guardResult.blocked ||
guardResult.warned), keep the same payload shape (sessionId,
managedProcess.toolType, findings mapped to types) and ensure any emitted
security event uses the same condition so blocked/warned states are always
logged/emitted.
- Around line 56-82: The emitted security-event payload is inconsistent with
StdoutHandler: instead of passing the raw findings array, transform
guardResult.findings into the same SecurityEventData shape used by StdoutHandler
(include findingTypes: array of unique finding.type values and findingCount:
findings.length), include tabId in the object passed to logSecurityEvent, and
emit that transformed object via this.emitter.emit('security-event', sessionId,
transformedEvent). Keep existing fields (eventType, action, originalLength,
sanitizedLength, toolType) but replace findings with findingTypes and
findingCount; use guardResult.findings to compute those values and reference
logSecurityEvent, this.emitter.emit, securityEvent (rename/replace with
transformedEvent), guardResult.findings, managedProcess.tabId, and sessionId to
locate where to change the code.
In `@src/main/security/llm-guard/config-export.ts`:
- Around line 308-314: The current import logic in config-export.ts overwrites
original custom pattern IDs (settings.customPatterns -> config.customPatterns),
which breaks deduplication in mergeCustomPatterns and lookup by f.patternId;
instead preserve each pattern.id from the import payload and only generate a new
id when a collision with an existing pattern is detected (e.g., check
existingPatterns.has(p.id) before insert); update the mapping logic that creates
config.customPatterns to copy the original id unchanged and add
collision-detection/generation fallback so mergeCustomPatterns and the finding
lookup by f.patternId continue to work correctly.
---
Nitpick comments:
In `@src/main/security/llm-guard/config-export.ts`:
- Around line 36-47: Update validateRegex to detect ReDoS-vulnerable patterns by
using the redos-detector isSafe check after constructing the RegExp; inside
validateRegex (and before returning valid:true) call isSafe(new RegExp(pattern,
'gi')) and if result.safe is false return { valid: false, error: `Pattern is
vulnerable to ReDoS: ${result.score}` }. Keep existing empty-pattern and
RegExp-construction guards, and ensure this validation protects patterns that
will later be executed in applyCustomPatterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4af8410-4bd3-4d51-81c0-5697caf3c11b
📒 Files selected for processing (7)
src/__tests__/main/security/llm-guard/config-export.test.tssrc/main/group-chat/group-chat-router.tssrc/main/process-manager/handlers/StderrHandler.tssrc/main/security/llm-guard/config-export.tssrc/main/security/llm-guard/index.tssrc/main/security/llm-guard/url-scanner.tssrc/renderer/components/RightPanel.tsx
✅ Files skipped from review due to trivial changes (2)
- src/renderer/components/RightPanel.tsx
- src/tests/main/security/llm-guard/config-export.test.ts
Comment on lines
+49
to
+54
| if (guardResult.findings.length > 0) { | ||
| logger.warn('[LLMGuard] Stderr output findings detected', 'LLMGuard', { | ||
| sessionId, | ||
| toolType: managedProcess.toolType, | ||
| findings: guardResult.findings.map((finding) => finding.type), | ||
| }); |
There was a problem hiding this comment.
Security event logging gated on findings.length > 0 may miss blocked/warned states.
If guardResult.blocked or guardResult.warned is true but findings.length === 0 (theoretically possible from config rules), the security event won't be logged or emitted. The StdoutHandler uses findings.length > 0 || guardResult.blocked || guardResult.warned as the condition.
🔧 Suggested fix
- if (guardResult.findings.length > 0) {
+ if (guardResult.findings.length > 0 || guardResult.blocked || guardResult.warned) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/process-manager/handlers/StderrHandler.ts` around lines 49 - 54, The
current StderrHandler only logs LLMGuard events when guardResult.findings.length
> 0 which can miss cases where guardResult.blocked or guardResult.warned is true
but findings is empty; update the conditional that surrounds the logger.warn
call (and any event emission) to mirror StdoutHandler by checking
(guardResult.findings.length > 0 || guardResult.blocked || guardResult.warned),
keep the same payload shape (sessionId, managedProcess.toolType, findings mapped
to types) and ensure any emitted security event uses the same condition so
blocked/warned states are always logged/emitted.
Comment on lines
+308
to
+314
| if (Array.isArray(settings.customPatterns)) { | ||
| // Deep clone custom patterns and regenerate IDs to avoid conflicts | ||
| config.customPatterns = (settings.customPatterns as CustomPattern[]).map((p) => ({ | ||
| ...p, | ||
| id: `pattern_${Date.now()}_${Math.random().toString(36).substring(2, 9)}`, | ||
| })); | ||
| } |
There was a problem hiding this comment.
ID regeneration breaks pattern merging and finding lookups.
Regenerating custom pattern IDs on import causes several downstream failures:
-
mergeCustomPatterns(index.ts:1847-1868): Usespattern.idas a Map key to deduplicate global and session patterns. Regenerated IDs will never match existing patterns, causing duplicates instead of updates. -
Finding blocking patterns (index.ts:2018-2021): Looks up patterns by matching
f.patternIdfrom findings againstpattern.id. If IDs are regenerated after patterns run, the lookup fails and block actions are skipped. -
Re-importing exported configs: Exporting preserves original IDs, but re-importing generates new ones, so round-trip semantics are inconsistent.
If the goal is conflict avoidance, consider preserving the original ID and only generating new IDs when a collision is detected with existing patterns.
🛠️ Suggested fix to preserve original IDs
if (Array.isArray(settings.customPatterns)) {
- // Deep clone custom patterns and regenerate IDs to avoid conflicts
- config.customPatterns = (settings.customPatterns as CustomPattern[]).map((p) => ({
- ...p,
- id: `pattern_${Date.now()}_${Math.random().toString(36).substring(2, 9)}`,
- }));
+ // Deep clone custom patterns, preserving IDs for merge/lookup compatibility
+ config.customPatterns = (settings.customPatterns as CustomPattern[]).map((p) => ({
+ ...p,
+ }));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (Array.isArray(settings.customPatterns)) { | |
| // Deep clone custom patterns and regenerate IDs to avoid conflicts | |
| config.customPatterns = (settings.customPatterns as CustomPattern[]).map((p) => ({ | |
| ...p, | |
| id: `pattern_${Date.now()}_${Math.random().toString(36).substring(2, 9)}`, | |
| })); | |
| } | |
| if (Array.isArray(settings.customPatterns)) { | |
| // Deep clone custom patterns, preserving IDs for merge/lookup compatibility | |
| config.customPatterns = (settings.customPatterns as CustomPattern[]).map((p) => ({ | |
| ...p, | |
| })); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/security/llm-guard/config-export.ts` around lines 308 - 314, The
current import logic in config-export.ts overwrites original custom pattern IDs
(settings.customPatterns -> config.customPatterns), which breaks deduplication
in mergeCustomPatterns and lookup by f.patternId; instead preserve each
pattern.id from the import payload and only generate a new id when a collision
with an existing pattern is detected (e.g., check existingPatterns.has(p.id)
before insert); update the mapping logic that creates config.customPatterns to
copy the original id unchanged and add collision-detection/generation fallback
so mergeCustomPatterns and the finding lookup by f.patternId continue to work
correctly.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/security/security-logger.ts (1)
286-295: Overly broad error suppression inclearAllEvents.The catch block silently ignores all errors, not just
ENOENT(file not found). Other errors like permission denied (EACCES) or disk full (ENOSPC) should be surfaced.♻️ Suggested fix
export async function clearAllEvents(): Promise<void> { clearEvents(); try { const filePath = getSecurityEventsPath(); await fs.writeFile(filePath, '', 'utf-8'); - } catch { - // Ignore file errors (file may not exist) + } catch (error: unknown) { + const code = (error as NodeJS.ErrnoException).code; + if (code !== 'ENOENT') { + // Report non-ENOENT errors (permission issues, disk full, etc.) + console.error('[SecurityLogger] Failed to clear events file:', error); + captureException(error as Error, { context: 'security-logger-clear' }); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/security/security-logger.ts` around lines 286 - 295, The catch in clearAllEvents silently swallows all errors from fs.writeFile; update clearAllEvents to only ignore "ENOENT" (file not found) and rethrow or surface any other errors (e.g., by rethrowing the caught error) so permission or disk errors aren't hidden—inspect the thrown error's code property after calling getSecurityEventsPath() and fs.writeFile(filePath, '', 'utf-8') and conditionally suppress only ENOENT, otherwise throw the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/security/security-logger.ts`:
- Around line 286-295: The catch in clearAllEvents silently swallows all errors
from fs.writeFile; update clearAllEvents to only ignore "ENOENT" (file not
found) and rethrow or surface any other errors (e.g., by rethrowing the caught
error) so permission or disk errors aren't hidden—inspect the thrown error's
code property after calling getSecurityEventsPath() and fs.writeFile(filePath,
'', 'utf-8') and conditionally suppress only ENOENT, otherwise throw the error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71d1afb2-e5ef-4fc4-b571-4ad25adc1575
📒 Files selected for processing (1)
src/main/security/security-logger.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit