Adds codex support, Adds multi account view, small perf and security fixes#92
Adds codex support, Adds multi account view, small perf and security fixes#92bberenberg wants to merge 5 commits into
Conversation
Greptile SummaryThis PR adds three major features: (1) a Codex-backed LLM provider with model selection and an MCP-based tool bridge for the interactive agent, (2) a unified "All accounts" inbox view with persisted account selection and account-aware actions, and (3) email privacy hardening via remote image blocking (regex-based replacement + tightened CSP The abstraction of Confidence Score: 4/5Safe to merge after addressing the fragile Codex auth string-match (P1); remaining findings are P2 improvements. The PR has one P1 issue: the getCodexAuthStatus auth check relies on matching the literal string 'Logged in' in CLI stdout, which will silently misreport auth state if the CLI output ever changes. The multi-account and LLM abstraction changes are well-structured and the image-privacy layering (content replacement + CSP) is correct. All other findings are P2 — none block correctness today. src/main/services/codex-cli.ts (auth detection), src/main/agents/providers/codex-agent-provider.ts (MCP bridge auth), src/main/services/email-sync.ts (newMessageIds dedup)
|
| Filename | Overview |
|---|---|
| src/main/services/codex-cli.ts | New Codex SDK integration: CLI path resolution, prompt flattening, thread invocation, and auth status check — auth detection relies on fragile string matching ("Logged in") that could silently break if CLI output changes. |
| src/main/services/llm-service.ts | Clean provider-agnostic LLM dispatch layer; correctly initialised at startup via index.ts; normalises Anthropic and Codex responses to a shared LlmResponse type. |
| src/main/agents/providers/codex-agent-provider.ts | Codex interactive agent provider with MCP HTTP bridge for tool execution; bridge binds to localhost-only random port but lacks an auth token — any co-located process can call email tools during an active session. |
| src/shared/email-image-privacy.ts | Regex-based remote image replacement with data-URI placeholders; correctly handles tracking pixels and clamped dimensions, but srcset attributes are not replaced (CSP blocks the load but shows broken-image icon instead of placeholder). |
| src/main/services/email-sync.ts | Switches to unfiltered Gmail history (fixes missed label removals) and adds startup reconciliation of visible inbox labels; newMessageIds can accumulate duplicate IDs from both messagesAdded and labelAdded(INBOX) paths causing redundant Gmail API calls. |
| src/renderer/store/index.ts | Adds account-aware email lookup map, custom areThreadingEmailsEqual equality function to reduce re-renders, persisted account selection, and multi-account support for isSentEmail/groupByThread — well-structured changes. |
| src/main/services/prefetch-service.ts | Adds usage-limit backoff with 30-min cooldown, caps analysis backfill to 25 newest emails per run, and removes the archive-ready auto-queuing to reduce background API pressure. |
| src/main/db/index.ts | Extracts parseLabelIds helper, adds getVisibleInboxLabelStates for reconciliation; the LIKE '%"INBOX"%' pattern works for current data but is slightly fragile compared to json_each. |
Sequence Diagram
sequenceDiagram
participant Renderer
participant MainIPC
participant LlmService
participant AnthropicSvc
participant CodexCli
participant CodexAgent as CodexAgentProvider
participant McpBridge as MCP HTTP Bridge
participant ToolExecutor
Renderer->>MainIPC: drafts:generate / drafts:refine
MainIPC->>LlmService: createMessage(params, options)
alt currentProvider == anthropic
LlmService->>AnthropicSvc: createMessage()
AnthropicSvc-->>LlmService: Message
else currentProvider == codex
LlmService->>CodexCli: createCodexMessage()
CodexCli->>CodexCli: buildCodexPrompt()
CodexCli-->>LlmService: LlmResponse
end
LlmService-->>MainIPC: LlmResponse
MainIPC-->>Renderer: draft text
Renderer->>MainIPC: agent:run providerIds=codex
MainIPC->>CodexAgent: run(params)
CodexAgent->>McpBridge: startToolBridge(tools)
McpBridge->>McpBridge: listen on 127.0.0.1:0
CodexAgent->>CodexCli: createCodexClient(mcp_servers)
CodexCli-->>CodexAgent: thread
CodexAgent->>CodexCli: thread.runStreamed(prompt)
loop streaming events
CodexCli-->>CodexAgent: ThreadEvent
CodexAgent->>McpBridge: mcp_tool_call
McpBridge->>ToolExecutor: toolExecutor(name, args)
ToolExecutor-->>McpBridge: result
McpBridge-->>CodexCli: MCP response
CodexAgent-->>Renderer: AgentEvent via IPC
end
CodexAgent->>McpBridge: close()
Reviews (1): Last reviewed commit: "Add unified account inbox view" | Re-trigger Greptile
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (!state.completedToolIds.has(item.id)) { | ||
| state.completedToolIds.add(item.id); | ||
| yield { | ||
| type: "tool_call_end", | ||
| toolCallId: item.id, | ||
| result: { | ||
| status: item.status, | ||
| changes: item.changes, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟡 file_change emits tool_call_end immediately without checking completion status
In mapCodexItemEvent, the file_change case emits both tool_call_start and tool_call_end on the very first event (e.g., item.started) because the guard only checks !state.completedToolIds.has(item.id), which is always true initially. This is inconsistent with every other item type handler: mcp_tool_call checks item.status !== "in_progress" (codex-agent-provider.ts:335), web_search checks eventType === "item.completed" (codex-agent-provider.ts:357), and command_execution checks item.status !== "in_progress" (codex-agent-provider.ts:379). The result is that tool_call_end fires with potentially stale/incomplete item.status and item.changes data from the item.started event, and subsequent item.updated/item.completed events for the same item are silently ignored.
| if (!state.completedToolIds.has(item.id)) { | |
| state.completedToolIds.add(item.id); | |
| yield { | |
| type: "tool_call_end", | |
| toolCallId: item.id, | |
| result: { | |
| status: item.status, | |
| changes: item.changes, | |
| }, | |
| }; | |
| } | |
| if (eventType === "item.completed" && !state.completedToolIds.has(item.id)) { | |
| state.completedToolIds.add(item.id); | |
| yield { | |
| type: "tool_call_end", | |
| toolCallId: item.id, | |
| result: { | |
| status: item.status, | |
| changes: item.changes, | |
| }, | |
| }; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| env: buildCodexEnv(), | ||
| ...(config ? { config } : {}), | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
authenticated: combined.includes("Logged in") hardcodes the CLI's exact output phrase. If the Codex CLI ever changes this string (e.g. to "Authenticated", "Signed in", or a localized variant), the check silently returns authenticated: false even when the user is properly logged in — causing CodexAgentProvider.isAvailable() to fail and blocking all Codex-backed flows without any actionable error.
A more robust approach is to rely on the exit code alone: exit 0 → authenticated, non-zero → not authenticated (the current code already returns authenticated: false for non-zero exits). The string check adds fragility without much safety benefit, since a healthy login status command shouldn't exit 0 while unauthenticated.
resolve({
cliAvailable: true,
authenticated: true, // exit 0 from "login status" means authenticated
});| @@ -939,10 +1033,19 @@ class EmailSyncService { | |||
| }); | |||
| } | |||
| } catch (err) { | |||
There was a problem hiding this comment.
newMessageIds can contain the same ID twice in one history batch
After the switch to unfiltered history, an email that transitions from archived → inbox can appear in both messagesAdded (with labels.includes("INBOX")) and labelAdded(INBOX) in the same history window. Both branches push to newMessageIds, so the same email is fetched from Gmail and processed twice. saveEmail is an upsert so data stays consistent, but it doubles the Gmail API calls for those messages and may push duplicates through onNewEmails to the renderer.
A cheap fix is to deduplicate before the loop:
const uniqueNewMessageIds = [...new Set(changes.newMessageIds)];
for (const id of uniqueNewMessageIds) {| }); | ||
|
|
||
| await new Promise<void>((resolve, reject) => { | ||
| httpServer.once("error", reject); | ||
| httpServer.listen(0, "127.0.0.1", () => { | ||
| httpServer.removeListener("error", reject); | ||
| resolve(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The bridge is bound to 127.0.0.1 which prevents remote access, but any other local process running during an active agent session can connect to http://127.0.0.1:{port}/mcp and invoke all registered email tools (read thread, send mail, archive, etc.) without any credential check. The port is ephemeral and unlisted, so real-world exploitability is very low, but it's worth noting as the tool set has meaningful side effects (email send, archive).
A low-overhead mitigation is to add a randomly-generated bearer token that the Codex client must present; the StreamableHTTPServerTransport can check it in the request handler before forwarding to MCP.
| /** | ||
| * Replace remote <img src="https://..."> loads with a local placeholder. | ||
| * Tiny tracking pixels are removed outright. | ||
| */ | ||
| export function replaceRemoteImageSources(html: string, useLightMode: boolean): string { | ||
| if (!html.includes("<img")) return html; | ||
|
|
||
| return html.replace( | ||
| /<img\b[^>]*\bsrc\s*=\s*(["'])(https?:\/\/[^"']+)\1[^>]*>/gi, | ||
| (fullTag: string) => { | ||
| if (isLikelyTrackingPixel(fullTag)) { | ||
| return ""; | ||
| } | ||
|
|
||
| const width = clampDimension( | ||
| extractNumericAttribute(fullTag, "width"), | ||
| DEFAULT_PLACEHOLDER_WIDTH, | ||
| MAX_PLACEHOLDER_WIDTH, |
There was a problem hiding this comment.
srcset attribute bypasses the privacy replacement
replaceRemoteImageSources only targets the src attribute. Emails that use <img src="..." srcset="https://cdn.example.com/img@2x.jpg 2x"> will have their src replaced with a placeholder but their srcset still pointing to an external URL. The CSP change (img-src 'self' data: blob:) correctly blocks the load at the browser level, so there is no privacy leak — but the browser will show a broken placeholder image icon instead of the SVG placeholder, and the tracking pixel check won't suppress these images.
Consider also stripping or replacing srcset in the same replacement pass.
| export function getVisibleInboxLabelStates(accountId: string, limit: number): EmailLabelState[] { | ||
| const db = getDatabase(); | ||
| const stmt = db.prepare(` | ||
| SELECT id, label_ids as labelIds | ||
| FROM emails | ||
| WHERE account_id = ? AND (label_ids IS NULL OR label_ids LIKE '%"INBOX"%') | ||
| ORDER BY date DESC | ||
| LIMIT ? | ||
| `); |
There was a problem hiding this comment.
label_ids LIKE '%"INBOX"%' relies on the serialized JSON containing the exact substring "INBOX". This works for standard JSON arrays today, but a hypothetical future label whose ID contains the substring INBOX could produce false positives. A more robust check uses SQLite's json_each:
WHERE account_id = ? AND (
label_ids IS NULL OR EXISTS (
SELECT 1 FROM json_each(label_ids) WHERE value = 'INBOX'
)
)SQLite has supported json_each since 3.38 (2022), and better-sqlite3 bundles its own SQLite, so this should be available.
|
big change but i generally like the direction. each of the features in your title should be a separate PR. mind kicking those off? the codex one overlaps a bunch with an ollama / open source agent PR im working on, so let me get that one in first, but the other ones seem easier to handle. |
|
Breaking this out as requested. I split the non-Codex parts of this PR into three focused draft PRs:
I left the Codex/provider-routing work out of these follow-up PRs for now because of the overlap with the Ollama / open source agent work mentioned above. I also folded in the directly relevant review feedback while splitting:
Closing this larger PR in favor of the smaller replacements above. |
2 participants
Summary
Changes
Tests
The code in this PR was largely written by AI but I have been using this locally for a few days now.