FEA-1444: Codex hook ingestion + dedup + opt-in toggle#259
Conversation
- Add codex-hook-handler.js wrapper that mirrors upstream hook-handler.js semantics but injects __provider: "codex" so the sidecar can stamp the session row with harness='codex'. Zero-dep plain JS, fail-silent. - Split agent-monitor-hooks.ts: pure install/uninstall logic moved to new agent-monitor-hooks-core.ts (electron-free, testable under tsx --test). Shell retains electron-store + path resolution + gatewayLog. - Add installCodexHooks / uninstallCodexHooks targeting $CODEX_HOME/hooks.json with the narrower Codex hook event set (SessionStart, UserPromptSubmit, PreToolUse, PostToolUse, Stop). SessionStart uses the "startup|resume" matcher per Codex's reference contract. - Filename-boundary predicate (isClaudeEntry vs isCodexEntry) prevents cross-matching between the two handler families. - New codexOptIn electron-store flag (default false) gates Codex hook installation. Behavior change: the master hooks toggle is unchanged for existing users; Codex hooks only install when both flags are true. Deferred to a follow-up: event-level dedup against the existing codex-watcher rollout-tail path, and the renderer UI to flip codexOptIn. - syncAgentMonitorHooksOnBoot reconciles Codex state with persisted intent (installs when opted in, uninstalls stale entries when opted out) so a failed file-layer mutation is eventually consistent on the next launch. - setAgentMonitorCodexHooksOptIn persists intent before applying side effects so a failure (e.g., locked hooks.json) cannot silently lose the user's opt-out. - build-agent-monitor.mjs gains a surgical patch to the generated server/routes/hooks.js that calls stmts.setSessionHarness when the inbound payload carries __provider: "codex". Patch is hash-anchored and hard-gated; future upstream drift fails the build loudly. - Bump desktop version to 0.15.98 per repo CI rule. Testing: - pnpm -C apps/desktop typecheck: clean - pnpm -C apps/desktop lint: clean - pnpm -C apps/desktop build:agent-monitor: clean (patch + hard-gates pass) - New test/agent-monitor-hooks-core.test.ts: 10/10 pass — covers predicate identity, matcher rules per harness, idempotent install, in-place self-heal of moved handler paths, narrow-scope uninstall, foreign-entry preservation, hooks-block cleanup, and Codex/Claude cross-installation independence. - test/agent-monitor-wiring-static.test.ts: 27/27 pass after retargeting three moved-string assertions to the new core file. - Full desktop test suite: 1825/1827 pass. The 2 remaining failures (python3 version-string parsing, telemetry healthcheck dedupe) are pre-existing flakes in unrelated files I did not touch. - Independent code-reviewer agent surfaced a Medium correctness finding (failed opt-out silently lost) and a Low note (function exported without IPC). Both addressed in this commit; the IPC wiring is an intentional follow-up. Risks: - Codex hooks are gated behind a new opt-in flag that has no UI yet, so default behavior is unchanged for all existing users. Risk surface is limited to anyone who flips the flag via dev console / tests until the UI ships. - The build-script patch depends on patchHooksTranscriptOutsideTx (FEA- 1363) running first; ordering enforced in materializeAgentMonitor and the patch is itself idempotent. A future upstream bump that drops the processEventCore anchor will fail the build hard rather than silently dropping the harness stamp. - The agent-monitor-hooks.ts → agent-monitor-hooks-core.ts split is a refactor of stable code; semantic equivalence verified by the existing wiring-static test plus the new core test.
Layer 2 — Event-level dedup against rollout-tail:
- New filterEventsAlreadyCapturedByHooks in codex-import.js. Runs before
importSession in importCodexSession; queries existing events for rows
matching (session_id, event_type, COALESCE(tool_name, ''), created_at
truncated to whole seconds) and drops duplicates. Hook-handler-inserted
rows land first (real-time); the ~5s-later rollout-tail no-ops on match.
- The dedup statement is cached per-dbModule via WeakMap so a batch of
many changed sessions compiles the SQL once, not once per session.
- Best-effort: any prepare/query failure is swallowed and the event is
kept. False negatives (cosmetic duplicates) are preferred over false
positives (silently dropped events). Sub-second timestamp drift handled
by the substr(?, 1, 19) match.
Layer 3 — Renderer opt-in toggle:
- apps/desktop/src/main/app.ts: new IPC handlers
desktop:{get,set}-agent-monitor-codex-hooks-opt-in that call the
already-exported isAgentMonitorCodexHooksOptIn /
setAgentMonitorCodexHooksOptIn. Gated on the master Agent Dashboard
flag, matching the Claude pair shape exactly.
- apps/desktop/src/main/preload.ts: mirror bridge methods exposing the
two channels to the renderer.
- apps/desktop/src/renderer/index.html: new #codexDashConsent toggle row
directly below the existing #claudeDashConsent. Same visibility
semantics (shown only on agent-settings route), same disabled-when-
master-off behavior, same hint text reset on master flag re-enable
(Codex hint reset now mirrors Claude's after a review-flagged gap).
refreshCodexHooksToggle is called when the user toggles the Claude
hooks toggle so the sibling's on/off badge stays in sync.
Test:
- apps/desktop/test/agent-session-event-dedup.test.ts (new): 4 unit
tests on the filter against a sandbox SQLite DB. Covers (1) duplicate
filter with sub-second drift, (2) non-duplicate survival across
mixed events, (3) best-effort behavior on DB errors, (4) empty-events
no-op. Uses the conditional-skip pattern (skipReason ? { skip: ... }
: undefined) because Node test runner treats skip: null as "skip with
no reason" rather than "don't skip" — gotcha learned during debugging.
Independent code review (second pass on the cumulative diff) surfaced:
- Medium: Codex hint stale after master flag re-enable (Claude hint was
reset in the else branch, Codex hint wasn't). Fixed.
- Low: Comment inaccuracy about which toggle "unblocks the master flag"
in wireClaudeHooksToggle. Rewritten.
- Low: Dedup statement prepared once per session inside the batch loop
instead of once per process. Cached via WeakMap.
All three addressed in this commit.
Testing:
- pnpm -C apps/desktop typecheck: clean
- pnpm -C apps/desktop lint: clean
- pnpm -C apps/desktop build:agent-monitor: clean (Layer 1 hard-gates
+ sidecar SQLite gate continue to pass)
- pnpm -C apps/desktop test (full suite): 1945/1945 pass, 0 fail
- New agent-session-event-dedup.test.ts: 4/4 pass
- agent-monitor-hooks-core.test.ts (from Layer 1): 10/10 pass (no
regression)
Risks:
- Cross-second timestamp drift between hook and rollout-tail can defeat
dedup. Documented design tradeoff: false negatives are cosmetic
duplicates; false positives would silently drop events. Will revisit
with a content-hash dedup key in a follow-up if cosmetic duplication
becomes visible in practice.
- IPC handler test coverage deferred. Both new handlers are shallow
plumbing that call already-tested functions (covered by Layer 1's
agent-monitor-hooks-core.test.ts). Typecheck validates the signatures
+ preload bridge types.
- Renderer JS duplicates the Claude/Codex toggle pattern. Per CLAUDE.md
this is the kind of duplication worth extracting, but inline renderer
JS makes a shared helper awkward; deferred to a separate cleanup
ticket if the pattern repeats with a third harness.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8db7ef93b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| stmt = dbModule.db.prepare( | ||
| "SELECT 1 FROM events " + | ||
| "WHERE session_id = ? AND event_type = ? " + | ||
| "AND COALESCE(tool_name, '') = COALESCE(?, '') " + | ||
| "AND substr(COALESCE(created_at, ''), 1, 19) = substr(COALESCE(?, ''), 1, 19) " + | ||
| "LIMIT 1", |
There was a problem hiding this comment.
When Codex hooks are disabled, skipped, or miss one event, this query still treats any existing events row as a hook duplicate because it has no hook-source discriminator. In a live rollout file that is re-imported after a previous poll, a newly appended event with the same session id, event type, tool name, and second as an earlier rollout-imported event is filtered out before importSession() sees it, so rapid repeated tool calls can disappear from the timeline instead of being backfilled by the watcher.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the latest commit on this branch. Gated the filter on whether ~/.codex/hooks.json actually contains our codex-hook-handler.js. When it doesn't, there cannot be any hook-sourced rows in the events table to dedup against — the filter is now a no-op in that case. This eliminates the false-positive vector for the ~99% of users who haven't opted into Codex hooks.
New test "dedup: gate-off when Codex hooks are not installed — filter is a no-op" seeds a row that the unguarded filter would have treated as a hook duplicate, points CODEX_HOME at a directory with no hooks.json, and asserts the incoming rollout-tail event survives.
Known v1 limitation (your cases (b) and (c) — hooks installed but a specific event missed or skipped): rapid same-second same-tool calls in that scenario can still collapse. The proper fix is a source column on the events table (so hook-inserted rows are explicitly discriminable from rollout-tail rows); that's invasive enough to merit its own FEA and will be filed as a follow-up.
thadeusb
left a comment
thadeusb
left a comment
There was a problem hiding this comment.
I have enough to write the review. Let me confirm my two findings are solid:
Codex P2 (dedup false-positive on rapid same-second events): - The codex-import.js filter had no hook-source discriminator: any existing events row matching (session_id, event_type, tool_name, created_at-truncated-to-second) was treated as a hook duplicate. For users who haven't opted into Codex hooks (~99% of installs), the row always came from a prior rollout-tail import, so the filter was silently dropping legitimate distinct events on re-imports of a changed rollout file. - Gate the filter on whether ~/.codex/hooks.json actually contains our codex-hook-handler.js. When it doesn't, there can't be any hook-sourced rows to dedup against, so the filter is a no-op. The detection result is cached at module level and exposed via resetCodexHooksInstalledCache for test setup. - Known v1 limitation: when hooks ARE installed but miss/skip a specific event (the reviewer's case (b)/(c)), rapid same-second same-tool calls in that session can still collapse. Tracked as a follow-up FEA for a real `source` column on the events table. Codex P2 (Codex hook config hint accuracy): - Codex's canonical feature flag is `[features].hooks`, not `[features].codex_hooks` (which is a deprecated alias). - For non-managed command hooks, Codex requires the user to trust the installed command via `/hooks` before they fire — without that, the install in ~/.codex/hooks.json is silently skipped and the user sees no real-time ingestion. - Hint text + tooltip updated to spell out both requirements so a user flipping the toggle doesn't end up in a silent-fallback state. New test: - agent-session-event-dedup.test.ts: "gate-off when Codex hooks are not installed — filter is a no-op". Seeds a row that would normally be treated as a hook duplicate, points CODEX_HOME at a directory with no hooks.json, and asserts the incoming rollout-tail event survives. Bump desktop version 0.15.100 → 0.15.102 (0.15.101 reserved for FEA-1461 PR #258). Testing: - pnpm -C apps/desktop typecheck: clean - pnpm -C apps/desktop lint: clean - pnpm -C apps/desktop build:agent-monitor: clean - agent-session-event-dedup.test.ts: 5/5 pass (was 4 before; new gate-off coverage) - Full suite: 1842 + 104 = 1946/1946 pass, 0 fail Risks: - Install detection cached at module load — a user opting into Codex hooks AFTER the sidecar started won't trigger dedup until the next app boot. Acceptable for v1 (the sidecar restarts on toggle change via agent-monitor-sidecar.ts anyway, so the cache is refreshed). - The install probe reads ~/.codex/hooks.json with a substring match for "codex-hook-handler.js". A user who manually edits hooks.json to a different filename containing that substring would trip the gate as a false positive. Not a realistic risk; substring match is intentionally lenient to handle both userData-relative and resources-relative paths.
origin/main moved to 0.15.101 while this branch was open; the version-bump-check on the previous push tripped because the branch was at the same 0.15.101. Bumping to 0.15.103 — 0.15.102 is held by FEA-1444 PR #259.
|
@shafty023 — passing this over to you for review and merge. Some extra context, since you're the original inspiration for the feature: Background. PR #259 lands Codex hook ingestion into the Agent Monitor sidecar — the idea we borrowed from your Why you're the right reviewer. You wrote the upstream Codex hook code we adapted from, and you're the top contributor to closedloop-electron — so you have ground truth on both ends of this. Codex flagged two P2s on the latest commit; both are addressed in What's in the latest push.
Handoff terms. Please review, fix anything you want directly on the branch, and merge yourself when you're satisfied. Don't pass back to me — I'm explicitly handing you merge authority on this one. If you see something that affects the broader telemetry direction (e.g., we should be doing the dedup the way your workflow repo does it), feel free to restructure; you have better context on that than I do. Slack me if you hit something that needs a product decision. CI for the latest push is still propagating; should be green by the time you look. Thanks for taking it. |
| return stmt; | ||
| } | ||
|
|
||
| function filterEventsAlreadyCapturedByHooks(dbModule, session) { |
There was a problem hiding this comment.
This filter looks for session.events, but the rollout-tail parser returns the normalized importer shape with toolUses, apiErrors, toolResultErrors, etc., and the watcher passes that object into importCodexSession. That makes the new dedup path a no-op for real rollout imports. Even after wiring the right shape, tuple matching by session/event/tool/second does not satisfy PLN-767's durable external_event_id contract and can both drop legitimate same-second tool events and miss duplicates when timestamps drift. Please move dedup to the event identity layer: add events.external_event_id plus a partial unique index, derive the same ID in hook and rollout paths, insert with INSERT OR IGNORE, and cover the real parser-to-importer path.
|
|
||
| let cachedCodexHooksInstalled = null; | ||
|
|
||
| function codexHooksInstalled() { |
There was a problem hiding this comment.
codexHooksInstalled() caches the first hooks-installed result for the sidecar process, but the hook opt-in can change while that process is running. If startup imports happen before opt-in, dedup stays off and hook plus rollout rows duplicate after enabling; if startup sees hooks installed, disabling can leave dedup on and suppress rollout fallback events. Drop the process-lifetime cache or invalidate it from the toggle/restart path; durable external_event_id dedup would avoid reading hook config state entirely.
| if (isAgentMonitorCodexHooksOptIn()) { | ||
| installCodexHooks(); | ||
| } else { | ||
| uninstallCodexHooks(); |
There was a problem hiding this comment.
This puts Codex cleanup in the same transaction as enabling Claude hooks. If ~/.codex/hooks.json is malformed, locked, or unwritable, installHooks() may already have written Claude hooks, then uninstallCodexHooks() throws before enabled is stored, leaving global Claude hooks installed while the app reports tracking disabled. Codex opt-out cleanup should be best-effort/reconciled separately and must not fail the Claude enable path.
| export function setAgentMonitorCodexHooksOptIn( | ||
| optIn: boolean, | ||
| ): AgentMonitorHooksResult { | ||
| store().set("codexOptIn", optIn); |
There was a problem hiding this comment.
This stores codexOptIn before install/uninstall succeeds. On a failed opt-out, the UI can report tracking off while stale global Codex hooks still execute and forward session payloads; on a failed opt-in, boot can later install hooks for an action the UI told the user failed. Make the persisted flag reflect effective hook state, or store an explicit pending/error state surfaced by IPC/UI, and add failure regression coverage.
| handler: string, | ||
| hookType: string, | ||
| ): string { | ||
| return `ELECTRON_RUN_AS_NODE=1 "${execPath}" "${handler}" ${JSON.stringify(hookType)}`; |
There was a problem hiding this comment.
The hook command is stored in global Claude/Codex config and executed by a shell, but double quotes do not escape command substitution, backticks, or embedded quotes in execPath/handler. An app or userData path containing those characters can turn a hook fire into unintended shell execution. Shell-escape each dynamic token with a POSIX-safe quoting helper, or use a structured argv hook format if supported, and add tests for spaces, quotes, $(), and backticks.
| return; | ||
| } | ||
| try { | ||
| const optedIn = await api.getAgentMonitorCodexHooksOptIn(); |
There was a problem hiding this comment.
After Agent Dashboard is enabled, this always enables the Codex toggle after reading opt-in. PLN-767 requires the control to be disabled with an explanatory tooltip when Codex CLI is not detected; otherwise users without codex can install hooks and see an enabled state even though no hooks will run. Reuse the existing CLI detection/binary-path flow or expose focused availability IPC, and cover present/missing Codex states.
| non-managed command hook via Codex's `/hooks` slash command; | ||
| without trust, Codex skips the command silently and this falls | ||
| back to the rollout-tail watcher. --> | ||
| <div id="codexDashConsent" style="display:none;align-items:center;gap:10px;padding:10px 14px;border-bottom:1px solid var(--border,#2a2a2a);font-size:13px"> |
There was a problem hiding this comment.
This wires the Codex opt-in only into the Agent Dashboard consent bar, but PLN-767 specified the user path as Settings -> Feature Flags and called for IPC coverage around that toggle. As shipped, the required settings flow is absent and the tests do not exercise the user path that enables/disables Codex hooks. Wire this through the existing Feature Flags flow or update the requirement before merge, and add an existing-pattern IPC/renderer test for the toggle side effects.
| " // payload. When present, mark the session row as a Codex session so", | ||
| " // the dashboard groups it with the rollout-tail-imported rows and", | ||
| " // any harness-scoped UI affordances apply.", | ||
| " if (session && data && data.__provider === \"codex\") {", |
There was a problem hiding this comment.
__provider comes from the JSON body received by /api/hooks/event, so a local process can forge a hook POST and mark any session as Codex by setting this field. That can poison harness grouping and any future harness-scoped behavior. Derive provider from trusted server-side handler/route metadata, or strip untrusted __provider before generated route logic, and add an abuse regression for forged hook payloads.
| return path.join(os.homedir(), ".codex"); | ||
| } | ||
|
|
||
| // Codex's experimental hook config (gated by `[features].codex_hooks = true` |
There was a problem hiding this comment.
This still documents the deprecated [features].codex_hooks key after the UI was corrected to canonical [features].hooks plus /hooks trust. Keeping the old key in setup comments will keep future fixes/docs drifting back to the alias. Update these setup comments, and the matching core-file header, to the canonical key and trust requirement.
3 participants
Summary
End-to-end Codex hook ingestion for the Agent Monitor sidecar — three layers in one PR so the feature is user-visible on merge:
87a601c): Codex hook installer + payload handler + harness stamp. Mirrors the existing Claude hook installer, writes to~/.codex/hooks.json, ships a smallcodex-hook-handler.jswrapper that injects__provider: "codex"so the sidecar can stampharness='codex'on the session row.8db7ef9): Event-level dedup against the existingcodex-watcher.jsrollout-tail importer. Hook events land first (real-time); rollout-tail's ~5s catchup no-ops on the duplicate. Without this, opting into hooks would create 2 rows per event.8db7ef9): Renderer opt-in toggle directly below the existing Claude hooks toggle in the Agent Dashboard view + IPC handlers + preload bridge. User can flip it without editing JSON.See FEA-1444 and PLN-767 v2.
What ships to users
SessionStart(vs ~5s typical from rollout-tail) and event timelines update in real time.Test plan
pnpm -C apps/desktop typecheck— cleanpnpm -C apps/desktop lint— cleanpnpm -C apps/desktop build:agent-monitor— clean (Layer 1 hard-gates + SQLite gate pass)test/agent-session-event-dedup.test.ts— 4/4 pass (sub-second drift, mixed events, error best-effort, empty no-op)test/agent-monitor-hooks-core.test.ts(Layer 1) — 10/10 pass~/.codex/hooks.jsonpopulated, (2) session appears in dashboard withharness=codexinstantly, (3) no duplicate event rows after rollout-tail catches upRisks
[features].codex_hooks = truein~/.codex/config.toml. The toggle's help text spells this out. Programmatic toggle of that Codex flag is intentionally out of scope (too version-sensitive).agent-monitor-hooks.tswas split into electron-freeagent-monitor-hooks-core.ts+ electron-aware shell during Layer 1. This is a refactor of stable code; behavior verified byagent-monitor-wiring-static.test.ts(3 string-match assertions retargeted to the core file).Out of scope (documented for follow-ups)
agent_spawned,review_lane_failed, etc.). His Codex/goalorchestrator's state, not generic.codex-watcher.jsfallback. Stays for users on Codex versions without the experimental hook system.[features].codex_hooks = truetoggle in Codex's config.toml.🤖 Generated with Claude Code