feat: add Twilio webhook transport#10
Conversation
DeetGates
left a comment
DeetGates
left a comment
There was a problem hiding this comment.
Changes requested on 15b9810.
CI status:
- GitHub CI ❌ failing
- current failure is Ruff line-length in
webhook/server.py
Approval is blocked on red CI.
Additional blockers from review:
-
CLAUDE.mdwas not updated for webhook mode.- The current file still says replies must use the
send_messageMCP tool and must not print to stdout. - That is wrong for this PR's Twilio transport, where the webhook path captures Claude stdout as the reply body.
- The PR description explicitly called for webhook-mode instructions, but the branch still has the old bridge/poller-only guidance.
- The current file still says replies must use the
-
install.sh --transport twiliodoes not validate the required Twilio env vars.- The requested behavior was to validate
TWILIO_AUTH_TOKENandPUBLIC_WEBHOOK_URL. - The current script installs and starts the webhook service without checking either one.
- As written, it can start in an unsafe/misconfigured state and rely on runtime behavior instead of installer validation.
- The requested behavior was to validate
-
Test coverage is incomplete against the requested contract.
- I do see tests for: valid POST, disallowed JID, bad signature, no auth token, Claude timeout.
- But I do not see coverage proving the disallowed-JID path makes no Claude call.
- I also do not see tests covering the installer transport split (
--transport twiliovs default bridge) or the updatedCLAUDE.mdbehavior.
Other checked points that do look right in the current branch:
- bad Twilio signature returns 403 when
TWILIO_AUTH_TOKENis set - missing
TWILIO_AUTH_TOKENlogs a warning path and skips validation Fromnormalization convertswhatsapp:+15551234567to15551234567@s.whatsapp.net- disallowed
ALLOWED_CHATSreturns empty TwiML - webhook path writes inbound and outbound rows into a
messagestable withis_from_me - Claude is invoked with
BOT_WORKING_DIRas cwd and stdout is used as reply text - empty stdout / timeout return empty TwiML without raising through the request
- default install path remains bridge-oriented and does not install the webhook service
Once CI is green and the blockers above are fixed, I’m happy to re-review.
![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: 15b981007c
ℹ️ 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".
| echo "Setting up webhook virtualenv..." | ||
| python3 -m venv "$WEBHOOK_VENV_DIR" | ||
| "$WEBHOOK_VENV_DIR/bin/pip" install --upgrade pip | ||
| "$WEBHOOK_VENV_DIR/bin/pip" install "fastapi>=0.110" "uvicorn[standard]>=0.27" "twilio>=8.0" |
There was a problem hiding this comment.
webhook/server.py handles Twilio payloads with await request.form(), but the Twilio install path only installs FastAPI/Uvicorn/Twilio. In a fresh ./install.sh --transport twilio deployment, inbound webhook requests can fail at form parsing because python-multipart is not installed, which prevents any message from reaching Claude. Please include python-multipart in the webhook venv dependencies (and keep requirements.txt aligned for CI).
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| install_webhook_service() { | ||
| local bot_dir="${BOT_WORKING_DIR:-$REPO_ROOT}" |
There was a problem hiding this comment.
This default points Twilio mode at the repo root, so webhook/server.py runs claude -p under the existing root CLAUDE.md instructions (bridge flow: send via MCP, not stdout). The webhook transport expects the opposite behavior (stdout reply), and treats empty stdout as failure, so default Twilio installs can silently return empty TwiML replies unless operators override BOT_WORKING_DIR manually.
Useful? React with 👍 / 👎.
DeetGates
left a comment
DeetGates
left a comment
There was a problem hiding this comment.
Changes requested on 6d4737c.
CI status:
- GitHub CI ❌ failing
- current failure: workflow now runs
pip install -r webhook/requirements.txt, but that file does not exist on the branch
Approval is blocked on red CI.
Checked requested fixes:
CLAUDE.mdworkflow is now transport-aware, good- disallowed-JID test now asserts
subprocess.runwas not called, good
Codex inline comments assessment against the current branch:
- P1 (
python-multipart/ webhook dependency install) is still effectively valid.- The branch moved CI to
pip install -r webhook/requirements.txt, but there is nowebhook/requirements.txtfile, so CI fails immediately. - Also,
setup_webhook_venv()still only installsfastapi,uvicorn[standard], andtwilio; it does not installpython-multipart, whichawait request.form()relies on.
- The branch moved CI to
- P2 (Twilio-specific
BOT_WORKING_DIRdefault) still looks valid.install_webhook_service()still defaultsBOT_WORKING_DIRto$REPO_ROOT.- That means a default Twilio install still points Claude at the root
CLAUDE.md, which now contains mixed bridge/webhook instructions instead of a Twilio-specific prompt file. - I would not approve without either a Twilio-specific prompt path/default or another concrete mechanism that guarantees stdout-mode behavior by default.
Once CI is green and those remaining issues are addressed, I’m happy to re-review.
DeetGates
left a comment
DeetGates
left a comment
There was a problem hiding this comment.
Changes requested on 53b8bda.
CI status:
- GitHub CI ✅ (
cicompleted successfully)
Re-review:
webhook/requirements.txtnow exists and includespython-multipartsetup_webhook_venv()now installspython-multipartCLAUDE.mdtransport guidance is now internally consistent
Checked inline comments from chatgpt-codex-connector[bot] against the current branch state:
- P1 is no longer applicable.
python-multipartis now present in bothwebhook/requirements.txtand the Twilio installer venv setup.
- P2 still appears valid on the current branch.
install_webhook_service()still defaultsBOT_WORKING_DIRto$REPO_ROOT.- The new comment explains that webhook prompts override transport mode, but that is only a comment; it does not change the actual default runtime path or provide a Twilio-specific prompt file/default.
- As shipped, a default Twilio install still points Claude at the root prompt file, so the behavior Codex flagged is not concretely eliminated by code/config, only documented.
Because that remaining Codex concern still stands on current branch state, I am not approving this round.
DeetGates
left a comment
DeetGates
left a comment
There was a problem hiding this comment.
Approved on a4b6b93.
CI status:
- GitHub CI ✅ (
cicompleted successfully)
Re-review:
- checked all inline comments from
chatgpt-codex-connector[bot]against the current branch state - P1 is resolved:
python-multipartis now present in bothwebhook/requirements.txtandsetup_webhook_venv() - P2 is resolved:
webhook/CLAUDE.mdnow provides webhook-only stdout instructions, andwebhook/server.pydefaultsBOT_WORKING_DIRtopathlib.Path(__file__).parent, so webhook mode picks up the webhook-specific prompt automatically CLAUDE.mdtransport guidance remains consistent for the shared repo docs
Codex comments assessment:
- P1 no longer applies on the current branch because webhook runtime/CI deps now include
python-multipart - P2 no longer applies on the current branch because webhook runtime now defaults to the
webhook/prompt directory instead of the repo-root prompt
Verdict:
- no remaining valid
chatgpt-codex-connector[bot]inline blockers found on the current branch state
3 participants
Adds a Twilio webhook ingress transport alongside the existing bridge transport.
Includes:
--transport twilio