Add pluggable inbound-webhook framework + Granola meeting summarizer - #4
Open
Miyamura80 wants to merge 8 commits into
Open
Add pluggable inbound-webhook framework + Granola meeting summarizer#4Miyamura80 wants to merge 8 commits into
Miyamura80 wants to merge 8 commits into
Conversation
Introduces webhooks/ as a place to mount custom webhook listeners on the same Bun.serve instance. First source is Granola: when a meeting wraps, the emitter POSTs to /api/webhooks/granola and a Codex-generated summary is streamed into the configured Slack channel. - Shared X-Webhook-Secret auth and Redis NX/EX dedup per source - Handler returns 200 immediately and processes in the background - Failures post a⚠️ notice to the same channel so issues are visible - Extracted streamCodex into lib/codex.ts so mentions and webhooks share it
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="webhooks/granola.ts">
<violation number="1" location="webhooks/granola.ts:42">
P2: Validate payload types before property access; non-object JSON can currently throw and return 500.</violation>
<violation number="2" location="webhooks/granola.ts:65">
P2: Normalize `attendees` to an array before joining; current code can throw before the catch block.</violation>
</file>
<file name="lib/codex.ts">
<violation number="1" location="lib/codex.ts:35">
P2: Validate the Codex subprocess exit code before returning; non-zero exits are currently treated as success.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Fix all with cubic | Re-trigger cubic
Addresses three review issues from cubic on PR #4: - webhooks/granola.ts: reject non-object JSON bodies (null, arrays, primitives) before field access, instead of letting a TypeError bubble up as a 500. - webhooks/granola.ts: normalize `attendees` with Array.isArray before calling .join, so a malformed (non-array, non-empty) value can no longer throw outside the surrounding try/catch in the background task. - lib/codex.ts: await the subprocess and throw on non-zero exit so silent codex failures surface as errors instead of empty summaries. Callers (webhook handler, onNewMention) now see the failure and can react.
Capture the macOS deploy setup that was previously only tribal knowledge: - deploy/com.edison.assistant-bot.plist: LaunchAgent template (RunAtLoad + KeepAlive, direct bun exec, no secrets — env comes from .env). - docs/DEPLOY.md: install/manage commands plus two gotchas hit in practice: (1) a checkout on an external/removable volume needs Full Disk Access on the bun binary or launchd gets EPERM reading files (TCC); (2) the codex CLI hangs in dyld if its signing cert is revoked — upgrade to a clean build. - CLAUDE.md: link to the new deploy doc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deployment captured in-repo (
|
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Addresses two cubic review comments on 0bb7814: - deploy/com.edison.assistant-bot.plist: parameterize user-specific fields with __USER__ and __WORKDIR__ placeholders instead of hardcoding one operator's username in StandardOut/ErrPath, PATH, HOME, and WorkingDirectory. Label stays as com.edison.assistant-bot (reverse-DNS prefix refers to the org, not the local user). - docs/DEPLOY.md: install step now renders the template via sed so all placeholders get substituted in one command. Documents which fields are parameterized and why. - docs/DEPLOY.md: rewrite the revoked-cert diagnostic. The old command relied on `readlink -f` (GNU-only; BSD readlink on macOS errors with "illegal option -- f") and manually joined path segments in a way that double-nested inside the codex npm package. New command uses `find` under `npm root -g` to locate the arch-specific binary — BSD-safe, arch-agnostic (arm64 and x86_64), and resilient to whether npm dedups the platform packages flat or under codex/node_modules.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Addresses two cubic review comments on 36b5fae: - docs/DEPLOY.md: the previous "if deploying for a different user, replace $USER with the target username" note was misleading — the surrounding commands still targeted the current user's ~, $(id -u), and log paths. Replace it with a directive to run the whole install sequence as the target user so every reference resolves consistently. - docs/DEPLOY.md: the codex signing-cert diagnostic used the interactive shell's `npm root -g`, which nvm/nodenv/asdf can shadow. The bot spawns bare `codex` under the LaunchAgent's fixed PATH, so the diagnostic must resolve npm through that same PATH — otherwise it may inspect a different codex install than the one the bot actually runs, giving a false negative on a revoked cert. Rewrite to reproduce the bot's PATH and resolve npm via `command -v` under it (BSD-safe, arch-agnostic).
Dedup previously wrote a 7-day completion marker before processing began and never released it, so a crash, restart, or hung codex silently dropped the meeting for a week — the sender's retries got a 200 "Duplicate". Claim a 15-minute lease instead, promote it to the 7-day marker only after the summary posts, and delete it on failure so a retry can get through. streamCodex had no timeout and discarded stderr, which is exactly how the revoked-signing-cert hang presented in production: no output, no error, no diagnostic. Add a wall-clock timeout (10 min default) that kills the child and unblocks the read even when a grandchild holds the pipe open, capture the tail of stderr into the thrown error, and kill the process on early consumer exit. Also de-duplicate the JSONL parse path, which fixed a missing messageCount increment in the trailing-buffer branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HZdVoF8VB8L1q4zUkgKSM1
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
|
|
||
| const exitCode = await proc.exited; | ||
| await stderrDrained; |
There was a problem hiding this comment.
P1: A Codex run can hang forever after its main process exits if a descendant still holds stderr open. Bound the stderr drain with timeoutFired too, so the advertised wall-clock timeout still ends the stream.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/codex.ts, line 113:
<comment>A Codex run can hang forever after its main process exits if a descendant still holds stderr open. Bound the stderr drain with `timeoutFired` too, so the advertised wall-clock timeout still ends the stream.</comment>
<file context>
@@ -1,53 +1,128 @@
+ }
+
+ const exitCode = await proc.exited;
+ await stderrDrained;
+
+ if (exitCode !== 0) {
</file context>
Suggested change
| await stderrDrained; | |
| const stderrResult = await Promise.race([stderrDrained, timeoutFired]); | |
| if (stderrResult === TIMED_OUT) { | |
| throw new Error( | |
| `codex timed out after ${timeoutMs}ms${stderrDetail(stderrTail)}`, | |
| ); | |
| } |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| ${payload.transcript}${payload.notes ? `\n\n--- Notes ---\n${payload.notes}` : ""}`; | ||
|
|
||
| try { | ||
| await channel.post(streamCodex(prompt)); |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
Untrusted webhook content (transcript/notes) is embedded directly into a live codex exec prompt and then executed in tool-enabled read mode. This allows prompt-injection instructions inside meeting text to steer the model away from pure summarization behavior.
Impact: a malicious meeting payload can coerce the Codex subprocess to read sensitive local workspace content and emit it into the Slack post, crossing from low-trust webhook input into higher-trust local runtime data.
Reviewed by Cursor Security Reviewer for commit a3d550d. Configure here.
Review found two races in the previous commit. The dedup lease was released and promoted unconditionally, so a run that outlived its 15-minute lease could clobber the claim a retry had already taken: both jobs post, and the retry loses the ability to clean up. Each claim now carries a unique token and promote/release run as a Lua compare-and-set, so a stale run's writes are no-ops. Waiting for stdout EOF also hung whenever a descendant inherited the pipe and outlived codex. Reported for stderr, but stdout is the worse case: a fully successful run blocked until the 10-minute timeout and was then reported as a timeout. Stop reading stdout once the process has exited and its buffered output has drained, and bound the stderr wait in the failure path, where it is only needed to enrich the error. Verified against a local Redis (stale promote/release leave the retry's lease intact) and a stubbed codex: descendant-held pipes now finish in ~1s on success and ~6s on failure, with 200 buffered messages arriving intact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HZdVoF8VB8L1q4zUkgKSM1
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The grace period was armed once when codex exited, so it also counted time the generator spent suspended at a yield waiting on Slack. A consumer slower than the window could resume to find the grace already expired and drop still-buffered messages. Arm it fresh per read instead, so it measures only time spent waiting on the pipe. Truncation was not reproducible before this change — an already queued read wins the race by argument order — but that made correctness depend on microtask ordering rather than on the intended condition. Verified with a stubbed codex that exits mid-stream while a descendant holds the pipe, against consumers pausing 1.5s and 5s between chunks: all messages arrive in both cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HZdVoF8VB8L1q4zUkgKSM1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two things: a pluggable inbound-webhook framework (with the first source, a Granola meeting summarizer), and the macOS deployment work needed to actually run it as a managed service.
1. Generic webhook framework
Each source is a factory
(ctx: { bot }) => { path, handler }exported fromwebhooks/<name>.ts.index.tsmounts every factory underBun.serve({ routes }). Adding a new source = drop a file + one line inindex.ts.Shared conventions (documented in
CLAUDE.md):X-Webhook-Secret: $WEBHOOK_SECRET.SET key NX EX 604800(7-day TTL) — retried deliveries return 200 without re-processing.:warning:notice to the same Slack channel so issues are visible instead of silently swallowed.2. Granola source
POST /api/webhooks/granolaaccepts{ meeting_id, title, transcript, notes?, attendees? }, asks Codex for a Slack-formatted wrap-up (header + 3-5 bullets + optional action items), and streams it intoGRANOLA_SLACK_CHANNEL_ID(#edison-os-updates) viabot.channel("slack:Cxxx").post(streamCodex(...)).Refactor: extracted
streamCodexout ofindex.tsintolib/codex.tssoonNewMentionand webhook handlers share one implementation, and it now awaits the subprocess and throws on non-zero exit instead of silently yielding an empty summary.3. Deployment (launchd)
The bot now runs as a launchd-managed service — auto-starts, auto-restarts on crash, survives reboot.
deploy/com.edison.assistant-bot.plistis a checked-in template (no secrets; env comes from.envinWorkingDirectory), anddocs/DEPLOY.mdcovers install/manage plus two non-obvious gotchas that cost real debugging time:read()-ing file contents on external/removable volumes —lsworks,read()returnsEPERM. Symptom is a PID with an empty log and nothing listening on 3123. Fix is granting FDA to thebunbinary.CSSMERR_TP_CERT_REVOKEDand stalled in_dyld_startbeforemain()on every invocation, including--version.npm install -g @openai/codex@latest(0.144.0) fixes it. Includes thespctl --assessdiagnostic.Files
webhooks/types.ts—WebhookContext,WebhookRoute,WebhookFactorywebhooks/granola.ts— Granola handlerlib/codex.ts— sharedstreamCodexindex.ts— wires the webhooks array intoBun.serve({ routes })deploy/com.edison.assistant-bot.plist— LaunchAgent templatedocs/DEPLOY.md— install, management, and the two gotchas above.env.example— addsWEBHOOK_SECRETandGRANOLA_SLACK_CHANNEL_IDCLAUDE.md— documents the inbound-webhook conventions + linksdocs/DEPLOY.mdSetup
.env:brew services start redis).docs/DEPLOY.md.Test plan
POST /api/webhooks/slackandPOST /api/webhooks/granola.X-Webhook-Secretreturns401.200immediately and a streamed wrap-up lands in#edison-os-updates.meeting_idreturns200 Duplicate (already processed)and does not post a second message.400.bunx tsc --noEmitis clean.KeepAliveretries while the external volume mounts.@mentionflow still streams a Codex reply (regression check on thestreamCodexextraction) — not yet verified, see below.Known follow-ups (not in this PR)
@mentionpath is unverified. The Slack app's Request URL still needs pointing at the current public endpoint for both Event Subscriptions and Interactivity & Shortcuts, with Socket Mode off.X-Webhook-Secretis a static, replayable bearer compared non-constant-time, and the JSON body has no size cap. Should move to HMAC-over-body + timestamp.Generated by Claude Code